-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#7378: Fix page editor detection #7391
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,14 +57,6 @@ describe("useCurrentOrigin", () => { | |
expect(result.current).toBe("chrome-extension://abcxyz/"); | ||
}); | ||
|
||
test("if devtools page, should return devtools origin", async () => { | ||
setContext("devToolsPage"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The page editor test is below. We have no React code running on devtools_page |
||
const { result } = renderHook(() => useCurrentOrigin()); | ||
// Wait for origin to load (async state) | ||
await waitForEffect(); | ||
expect(result.current).toBe(DEVTOOLS_ORIGIN); | ||
}); | ||
|
||
test("if page editor page, should return devtools origin", async () => { | ||
setContext("extension"); | ||
location = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,11 @@ | |
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
import { isDevToolsPage, isOptionsPage } from "webext-detect-page"; | ||
import { isOptionsPage } from "webext-detect-page"; | ||
import { useEffect } from "react"; | ||
import notify from "@/utils/notify"; | ||
import useAsyncState from "@/hooks/useAsyncState"; | ||
import { isPageEditor } from "@/utils/expectContext"; | ||
|
||
/** | ||
* Get the current origin for where code is running. Used to set the origin on the | ||
|
@@ -40,14 +41,7 @@ function useCurrentOrigin(): string | null { | |
return browser.runtime.getURL(""); | ||
} | ||
|
||
if ( | ||
// Checks location.pathname against the 'devtools_page' value in manifest.json | ||
// This won't match for dev tools panels created by the devtool | ||
// page (i.e. the Page Editor) | ||
isDevToolsPage() || | ||
// Check for the page editor pagePath in the location pathname | ||
location.pathname === "/pageEditor.html" | ||
) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've been bitten by this before |
||
if (isPageEditor()) { | ||
return "devtools://devtools"; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,7 +52,7 @@ const startWatching = once(async () => { | |
}); | ||
|
||
export default function useCurrentUrl(): string { | ||
expectContext("devTools"); | ||
expectContext("pageEditor"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
const [url, setUrl] = useState(tabUrl); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,13 +20,16 @@ import { | |
isContentScript, | ||
isExtensionContext, | ||
isWebPage, | ||
contextNames, | ||
} from "webext-detect-page"; | ||
|
||
function isBrowserSidebar(): boolean { | ||
return isExtensionContext() && location.pathname === "/sidebar.html"; | ||
} | ||
|
||
export function isPageEditor(): boolean { | ||
return location.pathname === "/pageEditor.html"; | ||
} | ||
|
||
/** | ||
* Accepts 'This is my error' | new Error('This is my error') | Error; | ||
* The constructor would be used to create a custom error with the default message | ||
|
@@ -48,18 +51,15 @@ function createError( | |
return new errorOrCtor(defaultMessage); | ||
} | ||
|
||
// eslint-disable-next-line local-rules/persistBackgroundData -- Static | ||
const contexts = [...contextNames, "sidebar"] as const; | ||
|
||
// eslint-disable-next-line local-rules/persistBackgroundData -- Functions | ||
const contextMap = new Map<(typeof contexts)[number], () => boolean>([ | ||
["web", isWebPage], | ||
["extension", isExtensionContext], | ||
["background", isBackground], | ||
["contentScript", isContentScript], | ||
["sidebar", isBrowserSidebar], | ||
["devTools", () => "devtools" in chrome], | ||
]); | ||
const contextMap = { | ||
web: isWebPage, | ||
extension: isExtensionContext, | ||
background: isBackground, | ||
pageEditor: isPageEditor, | ||
contentScript: isContentScript, | ||
sidebar: isBrowserSidebar, | ||
} as const; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It turns out we were making a mistake here. Due to the Map type, all the contexts were specified and allowed by TypeScript, but then fail at runtime. |
||
|
||
/** | ||
* @example expectContext('extension') | ||
|
@@ -68,10 +68,10 @@ const contextMap = new Map<(typeof contexts)[number], () => boolean>([ | |
* @example expectContext('extension', new Error('Wrong context and this is my custom error')) | ||
*/ | ||
export function expectContext( | ||
context: (typeof contexts)[number], | ||
context: keyof typeof contextMap, | ||
error?: ErrorBaseType, | ||
): void { | ||
const isContext = contextMap.get(context); | ||
const isContext = contextMap[context]; | ||
if (!isContext) { | ||
throw new TypeError(`Context "${context}" not found`); | ||
} | ||
|
@@ -91,10 +91,10 @@ export function expectContext( | |
* @example forbidContext('extension', new Error('Wrong context and this is my custom error')) | ||
*/ | ||
export function forbidContext( | ||
context: (typeof contexts)[number], | ||
context: keyof typeof contextMap, | ||
error?: ErrorBaseType, | ||
): void { | ||
const isContext = contextMap.get(context); | ||
const isContext = contextMap[context]; | ||
if (!isContext) { | ||
throw new TypeError(`Context "${context}" not found`); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated bugfix, but should be straightforward