From 5c6419279eeb28a370cb2d060080269d540b0426 Mon Sep 17 00:00:00 2001 From: Ben Loe Date: Tue, 21 May 2024 08:57:42 -0700 Subject: [PATCH] DX - Refactor some of the sidebar frame function names to clarify behavior, and fix sidebar-sub-frame console error (#8475) * debug changes * make the code a bit more clear, and try-catch around the error related to sub-frames * only wrap the getConnectedTarget call in try catch * fix types * fix var --------- Co-authored-by: Ben Loe Co-authored-by: Todd Schiller --- src/__mocks__/@/utils/expectContext.ts | 5 +++- .../automationanywhere/aaFrameProtocol.ts | 29 +++++++++++++++---- .../google/sheets/core/useCurrentOrigin.ts | 4 +-- src/errors/contextInvalidated.ts | 4 +-- src/sidebar/ConnectedSidebar.tsx | 4 +-- src/sidebar/connectedTarget.tsx | 24 ++++++++++----- src/tinyPages/RestrictedUrlPopupApp.tsx | 4 +-- src/utils/expectContext.ts | 16 +++++++--- src/utils/sidePanelUtils.ts | 4 +-- 9 files changed, 67 insertions(+), 27 deletions(-) diff --git a/src/__mocks__/@/utils/expectContext.ts b/src/__mocks__/@/utils/expectContext.ts index e67742b464..e81fa5494b 100644 --- a/src/__mocks__/@/utils/expectContext.ts +++ b/src/__mocks__/@/utils/expectContext.ts @@ -18,4 +18,7 @@ export function expectContext() {} export function forbidContext() {} -export { isPageEditor, isBrowserSidebar } from "../../../utils/expectContext"; +export { + isPageEditorTopFrame, + isBrowserSidebarTopFrame, +} from "../../../utils/expectContext"; diff --git a/src/contrib/automationanywhere/aaFrameProtocol.ts b/src/contrib/automationanywhere/aaFrameProtocol.ts index 032fddb57b..174c1574e6 100644 --- a/src/contrib/automationanywhere/aaFrameProtocol.ts +++ b/src/contrib/automationanywhere/aaFrameProtocol.ts @@ -19,9 +19,10 @@ import { expectContext } from "@/utils/expectContext"; import { getConnectedTarget } from "@/sidebar/connectedTarget"; import { getCopilotHostData } from "@/contentScript/messenger/strict/api"; import { - SET_COPILOT_DATA_MESSAGE_TYPE, type ProcessDataMap, + SET_COPILOT_DATA_MESSAGE_TYPE, } from "@/contrib/automationanywhere/aaTypes"; +import { type TopLevelFrame } from "webext-messenger"; /** * `window.postMessage` data payload the Co-Pilot frame sends to the host application. @@ -121,11 +122,29 @@ export async function initCopilotMessenger(): Promise { // necessary to pass to the Co-Pilot frame. }); - // Note: This code can be run either in the sidebar or in a modal - const frame = await getConnectedTarget(); + let connectedTarget: TopLevelFrame | null = null; + try { + // Note: This code can be run either in the sidebar or in a modal. Currently, + // it sometimes also runs in nested frames in the MV3 sidebar, in which case + // the webext-messenger getTopLevelFrame() function currently cannot return + // a frameId/tabId, due to how it's implemented. It will throw an error, and + // in this specific case we don't want to be running the CoPilot Data + // initialization on a nested frame anyway, so we'll just eat the error and + // warn to console for now. + connectedTarget = await getConnectedTarget(); + } catch (error) { + console.warn( + "Error getting connected target, aborting co-pilot messenger initialization", + error, + ); + } + + if (!connectedTarget) { + return; + } - // Fetch the current data from the content script when the frame loads - const data = await getCopilotHostData(frame); + // Fetch the current data from the content script when the target frame loads + const data = await getCopilotHostData(connectedTarget); console.debug("Setting initial Co-Pilot data", { location: window.location.href, data, diff --git a/src/contrib/google/sheets/core/useCurrentOrigin.ts b/src/contrib/google/sheets/core/useCurrentOrigin.ts index ca1cbb7dd8..420e391da1 100644 --- a/src/contrib/google/sheets/core/useCurrentOrigin.ts +++ b/src/contrib/google/sheets/core/useCurrentOrigin.ts @@ -19,7 +19,7 @@ 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"; +import { isPageEditorTopFrame } from "@/utils/expectContext"; import { type Nullishable } from "@/utils/nullishUtils"; /** @@ -42,7 +42,7 @@ function useCurrentOrigin(): Nullishable { return browser.runtime.getURL(""); } - if (isPageEditor()) { + if (isPageEditorTopFrame()) { return "devtools://devtools"; } diff --git a/src/errors/contextInvalidated.ts b/src/errors/contextInvalidated.ts index 1dfea42742..ba85e98f5e 100644 --- a/src/errors/contextInvalidated.ts +++ b/src/errors/contextInvalidated.ts @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -import { isPageEditor } from "@/utils/expectContext"; +import { isPageEditorTopFrame } from "@/utils/expectContext"; import { getErrorMessage, getRootCause } from "./errorHelpers"; import { CONTEXT_INVALIDATED_ERROR } from "@/errors/knownErrorMessages"; @@ -31,7 +31,7 @@ const CONTEXT_INVALIDATED_NOTIFICATION_DURATION_MS = 20_000; * all communication becomes impossible. */ export async function notifyContextInvalidated(): Promise { - if (isPageEditor()) { + if (isPageEditorTopFrame()) { // It's one of the few contexts that stay open after invalidation, but it has its own InvalidatedContextGate return; } diff --git a/src/sidebar/ConnectedSidebar.tsx b/src/sidebar/ConnectedSidebar.tsx index 7513b6c9ac..dd869a9e2a 100644 --- a/src/sidebar/ConnectedSidebar.tsx +++ b/src/sidebar/ConnectedSidebar.tsx @@ -41,7 +41,7 @@ import { MOD_LAUNCHER } from "@/store/sidebar/constants"; import { ensureExtensionPointsInstalled } from "@/contentScript/messenger/api"; import { getReservedSidebarEntries } from "@/contentScript/messenger/strict/api"; import { - getConnectedTabIdMv3, + getConnectedTabIdForMV3SidebarTopFrame, getConnectedTarget, } from "@/sidebar/connectedTarget"; import useAsyncEffect from "use-async-effect"; @@ -112,7 +112,7 @@ const ConnectedSidebar: React.VFC = () => { details: chrome.webNavigation.WebNavigationFramedCallbackDetails, ) => { const { frameId, tabId, documentLifecycle } = details; - const connectedTabId = getConnectedTabIdMv3(); + const connectedTabId = getConnectedTabIdForMV3SidebarTopFrame(); if ( documentLifecycle === "active" && tabId === connectedTabId && diff --git a/src/sidebar/connectedTarget.tsx b/src/sidebar/connectedTarget.tsx index a97d832a66..bf14fa8b79 100644 --- a/src/sidebar/connectedTarget.tsx +++ b/src/sidebar/connectedTarget.tsx @@ -16,14 +16,21 @@ */ import { isMV3 } from "@/mv3/api"; -import { expectContext, isBrowserSidebar } from "@/utils/expectContext"; +import { expectContext, isBrowserSidebarTopFrame } from "@/utils/expectContext"; import { assertNotNullish } from "@/utils/nullishUtils"; import { once } from "lodash"; -import { type TopLevelFrame, getTopLevelFrame } from "webext-messenger"; +import { getTopLevelFrame, type TopLevelFrame } from "webext-messenger"; import { getTabUrl } from "webext-tools"; -export function getConnectedTabIdMv3(): number { +export function getConnectedTabIdForMV3SidebarTopFrame(): number { expectContext("sidebar"); + + if (location.pathname !== "/sidebar.html") { + throw new Error( + `getConnectedTabIdForMV3SidebarTopFrame can only be called from the MV3 sidebar's top frame. The current location is ${location.href}.`, + ); + } + const tabId = new URLSearchParams(window.location.search).get("tabId"); assertNotNullish( tabId, @@ -38,7 +45,7 @@ async function getConnectedTabIdMv2() { } export const getConnectedTabId = once( - isMV3() ? getConnectedTabIdMv3 : getConnectedTabIdMv2, + isMV3() ? getConnectedTabIdForMV3SidebarTopFrame : getConnectedTabIdMv2, ); /** @@ -47,9 +54,12 @@ export const getConnectedTabId = once( */ // TODO: Drop support for "content script iframes" because it doesn't belong to `@/sidebar/connectedTarget` // https://github.com/pixiebrix/pixiebrix-extension/pull/7354#discussion_r1461563961 -export const getConnectedTarget = - isMV3() && isBrowserSidebar() - ? (): TopLevelFrame => ({ tabId: getConnectedTabIdMv3(), frameId: 0 }) +export const getConnectedTarget: () => Promise = + isMV3() && isBrowserSidebarTopFrame() + ? async () => ({ + tabId: getConnectedTabIdForMV3SidebarTopFrame(), + frameId: 0, + }) : getTopLevelFrame; export async function getConnectedTargetUrl(): Promise { diff --git a/src/tinyPages/RestrictedUrlPopupApp.tsx b/src/tinyPages/RestrictedUrlPopupApp.tsx index dc71e25a8b..06bda24fd5 100644 --- a/src/tinyPages/RestrictedUrlPopupApp.tsx +++ b/src/tinyPages/RestrictedUrlPopupApp.tsx @@ -22,7 +22,7 @@ import { DISPLAY_REASON_EXTENSION_CONSOLE, DISPLAY_REASON_UNKNOWN, } from "@/tinyPages/restrictedUrlPopupConstants"; -import { isBrowserSidebar } from "@/utils/expectContext"; +import { isBrowserSidebarTopFrame } from "@/utils/expectContext"; import { getExtensionConsoleUrl } from "@/utils/extensionUtils"; import useOnMountOnly from "@/hooks/useOnMountOnly"; @@ -39,7 +39,7 @@ async function openInActiveTab(event: React.MouseEvent) { // TODO: Drop conditon after we drop the browser action popover since this // component will only be shown in the sidebar - if (!isBrowserSidebar()) { + if (!isBrowserSidebarTopFrame()) { window.close(); } } diff --git a/src/utils/expectContext.ts b/src/utils/expectContext.ts index a9f07a28bb..67906c5ce5 100644 --- a/src/utils/expectContext.ts +++ b/src/utils/expectContext.ts @@ -22,11 +22,19 @@ import { isWebPage, } from "webext-detect-page"; -export function isBrowserSidebar(): boolean { +/** + * Whether the current context is the top frame of the browser sidebar. + * + * Note: Returns false for nested frames in the MV3 sidebar, since generally their path won't match. + */ +export function isBrowserSidebarTopFrame(): boolean { return isExtensionContext() && location.pathname === "/sidebar.html"; } -export function isPageEditor(): boolean { +/** + * Whether the current context is the top frame of the page editor. + */ +export function isPageEditorTopFrame(): boolean { return location.pathname === "/pageEditor.html"; } @@ -56,9 +64,9 @@ const contextMap = { web: isWebPage, extension: isExtensionContext, background: isBackground, - pageEditor: isPageEditor, + pageEditor: isPageEditorTopFrame, contentScript: isContentScript, - sidebar: isBrowserSidebar, + sidebar: isBrowserSidebarTopFrame, } as const; /** diff --git a/src/utils/sidePanelUtils.ts b/src/utils/sidePanelUtils.ts index 007c1c436f..819601fe62 100644 --- a/src/utils/sidePanelUtils.ts +++ b/src/utils/sidePanelUtils.ts @@ -19,7 +19,7 @@ import { getErrorMessage } from "@/errors/errorHelpers"; import { isMV3 } from "@/mv3/api"; -import { forbidContext, isBrowserSidebar } from "@/utils/expectContext"; +import { forbidContext, isBrowserSidebarTopFrame } from "@/utils/expectContext"; import { type PageTarget, messenger, getThisFrame } from "webext-messenger"; import { isContentScript } from "webext-detect-page"; import { showSidebar } from "@/contentScript/messenger/strict/api"; @@ -32,7 +32,7 @@ export function isUserGestureRequiredError(error: unknown): boolean { } export async function openSidePanel(tabId: number): Promise { - if (isBrowserSidebar()) { + if (isBrowserSidebarTopFrame()) { console.warn( 'The sidePanel called "openSidePanel". This should not happen.', );