diff --git a/packages/extension/src/__tests__/service_worker.test.ts b/packages/extension/src/__tests__/service_worker.test.ts index f2f330d4..98ade4ab 100644 --- a/packages/extension/src/__tests__/service_worker.test.ts +++ b/packages/extension/src/__tests__/service_worker.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" import { enhancedSettings } from "@/services/settings/enhancedSettings" import { Settings } from "@/services/settings/settings" import { ServiceWorkerCommand } from "@/services/ipc" +import { openOptionPage } from "@/services/chrome" import { NEW_HUB_URL, VERSION, SHORTCUT_NO_SELECTION_BEHAVIOR } from "@/const" // Imported before any vi.doMock, so this is the real event name table. import { ANALYTICS_EVENTS } from "@/services/analytics" @@ -138,12 +139,8 @@ describe("Service Worker Migration", () => { expect(mockEnhancedSettings.get).toHaveBeenCalled() expect(mockSettings.set).not.toHaveBeenCalled() - expect(chrome.tabs.create).toHaveBeenCalledWith( - expect.objectContaining({ - url: expect.stringContaining( - `addPageRule=${encodeURIComponent(newUrl)}`, - ), - }), + expect(openOptionPage).toHaveBeenCalledWith( + expect.stringContaining(`addPageRule=${encodeURIComponent(newUrl)}`), ) }) diff --git a/packages/extension/src/components/option/editor/CommandList.tsx b/packages/extension/src/components/option/editor/CommandList.tsx index c4356f20..cdea1bcb 100644 --- a/packages/extension/src/components/option/editor/CommandList.tsx +++ b/packages/extension/src/components/option/editor/CommandList.tsx @@ -277,8 +277,9 @@ export const CommandList = ({ control }: CommandListProps) => { } } - // location.search is fixed at page load (set via chrome.tabs.create), so it is - // safe to read it without adding it to the dependency array. + // location.search is fixed at page load (the service worker opens or navigates + // the tab with these params via openOptionPage), so it is safe to read it + // without adding it to the dependency array. useEffect(() => { if (editParamHandledRef.current) return const params = new URLSearchParams(location.search) diff --git a/packages/extension/src/components/option/editor/PageRuleList.tsx b/packages/extension/src/components/option/editor/PageRuleList.tsx index a63834c8..9c0dfb8e 100644 --- a/packages/extension/src/components/option/editor/PageRuleList.tsx +++ b/packages/extension/src/components/option/editor/PageRuleList.tsx @@ -143,8 +143,9 @@ export const PageRuleList = ({ }) // Auto-open the dialog when addPageRule or editPageRule URL param is set. - // location.search is fixed at page load (set via chrome.tabs.create), so it is - // safe to read it without adding it to the dependency array. + // location.search is fixed at page load (the service worker opens or navigates + // the tab with these params via openOptionPage), so it is safe to read it + // without adding it to the dependency array. useEffect(() => { if (editParamHandledRef.current) return const params = new URLSearchParams(location.search) diff --git a/packages/extension/src/service_worker.ts b/packages/extension/src/service_worker.ts index 602f4b7d..10f29071 100644 --- a/packages/extension/src/service_worker.ts +++ b/packages/extension/src/service_worker.ts @@ -21,7 +21,12 @@ import { CACHE_SECTIONS } from "@/services/settings/settingsCache" import * as PageActionServiceWorker from "@/services/pageAction/serviceWorker" import { ServiceWorkerData } from "@/services/serviceWorkerData" import { ContextMenu } from "@/services/contextMenus" -import { closeWindow, windowExists, getCurrentTab } from "@/services/chrome" +import { + closeWindow, + windowExists, + getCurrentTab, + openOptionPage, +} from "@/services/chrome" import { WindowStackManager } from "@/services/windowStackManager" import { PopupAutoClose } from "@/services/popupAutoClose" import { findMatchingPageRule, isEmpty } from "@/lib/utils" @@ -155,9 +160,9 @@ const commandFuncs = { [ServiceWorkerCommand.execApi]: ActionHelper.execApi, [ServiceWorkerCommand.openOption]: (): boolean => { - chrome.tabs.create({ - url: OPTION_PAGE_PATH, - }) + openOptionPage().catch((e) => + console.error("Failed to open option page", e), + ) return false }, @@ -174,16 +179,16 @@ const commandFuncs = { const pageRules = settings.pageRules ?? [] const matchingRule = findMatchingPageRule(pageRules, param.url) if (matchingRule == null) { - chrome.tabs.create({ - url: `${OPTION_PAGE_PATH}?addPageRule=${encodeURIComponent(param.url)}#pageRules`, - }) + await openOptionPage( + `${OPTION_PAGE_PATH}?addPageRule=${encodeURIComponent(param.url)}#pageRules`, + ) } else { - chrome.tabs.create({ - url: `${OPTION_PAGE_PATH}?editPageRule=${encodeURIComponent(matchingRule.urlPattern)}#pageRules`, - }) + await openOptionPage( + `${OPTION_PAGE_PATH}?editPageRule=${encodeURIComponent(matchingRule.urlPattern)}#pageRules`, + ) } } - add() + add().catch((e) => console.error("Failed to open option page", e)) return false }, @@ -391,9 +396,7 @@ const updateActiveTabId = async (activeTabId?: number) => { } chrome.action.onClicked.addListener(() => { - chrome.tabs.create({ - url: OPTION_PAGE_PATH, - }) + openOptionPage().catch((e) => console.error("Failed to open option page", e)) }) chrome.windows.onFocusChanged.addListener(async (windowId: number) => { @@ -766,7 +769,9 @@ chrome.commands.onCommand.addListener(handleShortcutCommand) // Expose only in the E2E worker for Playwright evaluation, without a message endpoint. if (import.meta.env.MODE === "e2e") { - Object.assign(globalThis, { __e2eHandleShortcutCommand: handleShortcutCommand }) + Object.assign(globalThis, { + __e2eHandleShortcutCommand: handleShortcutCommand, + }) } // Side panel is an optional capability (e.g. not available on Opera), so diff --git a/packages/extension/src/services/__tests__/chrome.openOptionPage.test.ts b/packages/extension/src/services/__tests__/chrome.openOptionPage.test.ts new file mode 100644 index 00000000..d386f02f --- /dev/null +++ b/packages/extension/src/services/__tests__/chrome.openOptionPage.test.ts @@ -0,0 +1,107 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" +import { openOptionPage } from "../chrome" + +vi.mock("../offscreenClipboard", () => ({ readOffscreenClipboard: vi.fn() })) + +const BASE_URL = "chrome-extension://test-extension-id/" +const OPTION_URL = `${BASE_URL}src/options_page.html` + +/** + * Mock chrome.tabs.query to return all tabs, or the active tab of the last + * focused window when queried with `active: true`. + */ +const mockTabs = (tabs: Partial[], currentWindowId = 1) => { + vi.mocked(chrome.tabs.query).mockImplementation((( + info: chrome.tabs.QueryInfo, + ) => + Promise.resolve( + info.active + ? [{ id: 100, windowId: currentWindowId, url: "https://example.com/" }] + : tabs, + )) as typeof chrome.tabs.query) +} + +describe("openOptionPage", () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(chrome.runtime.getURL).mockImplementation( + (path: string) => `${BASE_URL}${path}`, + ) + vi.mocked(chrome.tabs.create).mockImplementation(() => + Promise.resolve({ id: 20 } as chrome.tabs.Tab), + ) + vi.mocked(chrome.tabs.update).mockImplementation( + async (tabId) => ({ id: tabId }) as chrome.tabs.Tab, + ) + ;(chrome.windows as any).update = vi.fn().mockResolvedValue({}) + }) + + it("COP-01: creates a new tab when no options page is open", async () => { + mockTabs([{ id: 1, windowId: 1, url: "https://example.com/" }]) + + const result = await openOptionPage() + + expect(chrome.tabs.create).toHaveBeenCalledWith({ + url: "src/options_page.html", + }) + expect(chrome.tabs.update).not.toHaveBeenCalled() + expect(result).toEqual({ tab: { id: 20 }, created: true }) + }) + + it("COP-02: activates the existing tab without navigating", async () => { + mockTabs([{ id: 5, windowId: 2, url: `${OPTION_URL}#commands` }]) + + const result = await openOptionPage() + + expect(chrome.tabs.create).not.toHaveBeenCalled() + expect(chrome.tabs.update).toHaveBeenCalledWith(5, { active: true }) + expect(chrome.windows.update).toHaveBeenCalledWith(2, { focused: true }) + expect(result).toEqual({ tab: { id: 5 }, created: false }) + }) + + it("COP-03: navigates the existing tab when a query is given", async () => { + mockTabs([{ id: 5, windowId: 2, url: OPTION_URL }]) + + await openOptionPage("src/options_page.html?addPageRule=x#pageRules") + + expect(chrome.tabs.create).not.toHaveBeenCalled() + expect(chrome.tabs.update).toHaveBeenCalledWith(5, { + active: true, + url: `${OPTION_URL}?addPageRule=x#pageRules`, + }) + expect(chrome.windows.update).toHaveBeenCalledWith(2, { focused: true }) + }) + + it("COP-04: prefers the tab in the last focused window", async () => { + mockTabs( + [ + { id: 5, windowId: 2, url: OPTION_URL }, + { id: 6, windowId: 3, url: OPTION_URL }, + ], + 3, + ) + + await openOptionPage() + + expect(chrome.tabs.update).toHaveBeenCalledWith(6, { active: true }) + expect(chrome.windows.update).toHaveBeenCalledWith(3, { focused: true }) + }) + + it("COP-05: reuses a tab that is still loading the options page", async () => { + mockTabs([{ id: 5, windowId: 1, url: "", pendingUrl: OPTION_URL }]) + + await openOptionPage() + + expect(chrome.tabs.create).not.toHaveBeenCalled() + expect(chrome.tabs.update).toHaveBeenCalledWith(5, { active: true }) + }) + + it("COP-06: does not reuse the excluded tab", async () => { + mockTabs([{ id: 5, windowId: 1, url: OPTION_URL }]) + + const result = await openOptionPage(undefined, 5) + + expect(chrome.tabs.update).not.toHaveBeenCalled() + expect(result).toEqual({ tab: { id: 20 }, created: true }) + }) +}) diff --git a/packages/extension/src/services/chrome.ts b/packages/extension/src/services/chrome.ts index 5f764efa..5cc35769 100644 --- a/packages/extension/src/services/chrome.ts +++ b/packages/extension/src/services/chrome.ts @@ -1,7 +1,12 @@ import { sleep, toUrl, isOverBytes, needsClipboard } from "@/lib/utils" import type { ScreenSize } from "@/services/dom" import type { ShowToastParam, UrlParam, WindowLayer } from "@/types" -import { POPUP_OFFSET, POPUP_TYPE, WINDOW_STATE } from "@/const" +import { + OPTION_PAGE_PATH, + POPUP_OFFSET, + POPUP_TYPE, + WINDOW_STATE, +} from "@/const" import { PopupOption } from "@/services/option/defaultSettings" import { ServiceWorkerData } from "@/services/serviceWorkerData" import { WindowStackManager } from "@/services/windowStackManager" @@ -507,6 +512,42 @@ export async function getCurrentTab(): Promise { return tab } +/** + * Open the options page, reusing an existing options page tab if any. + * A tab in the last focused window is preferred over tabs in other windows. + * @param path - The options page path. It may include a query and a hash. + * @param excludeTabId - A tab not to reuse, e.g. one that is being closed. + * @returns The tab showing the options page, and whether it was newly created. + */ +export async function openOptionPage( + path: string = OPTION_PAGE_PATH, + excludeTabId?: number, +): Promise<{ tab: chrome.tabs.Tab | undefined; created: boolean }> { + const optionPageUrl = chrome.runtime.getURL(OPTION_PAGE_PATH) + const [tabs, currentTab] = await Promise.all([ + chrome.tabs.query({}), + getCurrentTab(), + ]) + // A tab that is still loading has its destination only in pendingUrl. + const candidates = tabs.filter( + (t) => + t.id !== excludeTabId && + (t.pendingUrl || t.url)?.startsWith(optionPageUrl), + ) + const existing = + candidates.find((t) => t.windowId === currentTab?.windowId) ?? candidates[0] + if (existing?.id == null) { + const tab = await chrome.tabs.create({ url: path }) + return { tab, created: true } + } + // Navigate only when parameters are given, to keep the current page state. + const props: chrome.tabs.UpdateProperties = { active: true } + if (path !== OPTION_PAGE_PATH) props.url = chrome.runtime.getURL(path) + const tab = await chrome.tabs.update(existing.id, props) + await chrome.windows.update(existing.windowId, { focused: true }) + return { tab: tab ?? existing, created: false } +} + /** * Close a window. * @param windowId - The ID of the window to close. diff --git a/packages/extension/src/services/hub/__tests__/serviceWorker.test.ts b/packages/extension/src/services/hub/__tests__/serviceWorker.test.ts index ee31d06b..9b7b31cc 100644 --- a/packages/extension/src/services/hub/__tests__/serviceWorker.test.ts +++ b/packages/extension/src/services/hub/__tests__/serviceWorker.test.ts @@ -18,6 +18,7 @@ import { Settings } from "@/services/settings/settings" import { sendEvent, getOrCreateClientId } from "@/services/analytics" import { Ipc, TabCommand } from "@/services/ipc" import { calcContentHash } from "@/services/hub/commandContent" +import { openOptionPage } from "@/services/chrome" vi.mock("@/services/storage", () => ({ Storage: { @@ -35,6 +36,8 @@ vi.mock("@/services/storage", () => ({ }, })) +vi.mock("@/services/chrome", () => ({ openOptionPage: vi.fn() })) + vi.mock("@/services/settings/settings", () => ({ Settings: { addCommands: vi.fn(), updateCommandId: vi.fn() }, })) @@ -84,6 +87,13 @@ type MessageListener = ( sendResponse: (response?: unknown) => void, ) => boolean +function mockOpenOptionPage(tab: Partial, created = true) { + vi.mocked(openOptionPage).mockResolvedValue({ + tab: tab as chrome.tabs.Tab, + created, + }) +} + function getRegisteredListener(): MessageListener { initHubExternalListener() const calls = vi.mocked(chrome.runtime.onMessageExternal.addListener).mock @@ -719,10 +729,7 @@ describe("handleEditCommand", () => { }) it("EC-02: creates options tab with correct URL and responds with result:true", async () => { - vi.mocked(chrome.tabs.create).mockImplementation((_opts, cb) => { - cb?.({ id: 42 } as chrome.tabs.Tab) - return Promise.resolve({ id: 42 } as chrome.tabs.Tab) - }) + mockOpenOptionPage({ id: 42 }) const sendResponse = vi.fn() const result = handleEditCommand( "cmd-123", @@ -730,13 +737,9 @@ describe("handleEditCommand", () => { sendResponse, ) expect(result).toBe(true) - expect(chrome.tabs.create).toHaveBeenCalledWith( - { - url: expect.stringContaining( - "src/options_page.html?editCommand=cmd-123&syncHub=1#commands", - ), - }, - expect.any(Function), + expect(openOptionPage).toHaveBeenCalledWith( + "src/options_page.html?editCommand=cmd-123&syncHub=1#commands", + undefined, ) await vi.waitFor(() => expect(sendResponse).toHaveBeenCalledWith({ result: true }), @@ -744,10 +747,7 @@ describe("handleEditCommand", () => { }) it("EC-03: responds with error when tab creation returns no id", async () => { - vi.mocked(chrome.tabs.create).mockImplementation((_opts, cb) => { - cb?.({} as chrome.tabs.Tab) - return Promise.resolve({} as chrome.tabs.Tab) - }) + mockOpenOptionPage({}) const sendResponse = vi.fn() handleEditCommand( "cmd-1", @@ -761,6 +761,38 @@ describe("handleEditCommand", () => { }), ) }) + + it("EC-05: does not reuse the edit tab of the cancelled session", async () => { + mockOpenOptionPage({ id: 77 }) + const sender = { tab: { id: 10 } as chrome.tabs.Tab } + handleEditCommand("cmd-1", sender, vi.fn()) + await vi.mocked(openOptionPage).mock.results.at(-1)?.value + + handleEditCommand("cmd-2", sender, vi.fn()) + + expect(chrome.tabs.remove).toHaveBeenCalledWith(77) + expect(openOptionPage).toHaveBeenLastCalledWith( + "src/options_page.html?editCommand=cmd-2&syncHub=1#commands", + 77, + ) + }) + + it("EC-04: responds with error when opening the options page fails", async () => { + vi.mocked(openOptionPage).mockRejectedValue(new Error("No window")) + const sendResponse = vi.fn() + handleEditCommand( + "cmd-1", + { tab: { id: 10 } as chrome.tabs.Tab }, + sendResponse, + ) + await vi.waitFor(() => + expect(sendResponse).toHaveBeenCalledWith({ + result: false, + error: "Failed to create edit tab", + }), + ) + expect(chrome.runtime.onConnectExternal.removeListener).toHaveBeenCalled() + }) }) // --------------------------------------------------------------------------- @@ -769,10 +801,7 @@ describe("handleEditCommand", () => { describe("onMessageExternal - EditCommand routing", () => { it("EC-R-01: opens options page with editCommand and syncHub params", async () => { - vi.mocked(chrome.tabs.create).mockImplementation((_opts, cb) => { - cb?.({ id: 42 } as chrome.tabs.Tab) - return Promise.resolve({ id: 42 } as chrome.tabs.Tab) - }) + mockOpenOptionPage({ id: 42 }) const listener = getRegisteredListener() const sendResponse = vi.fn() const result = listener( @@ -795,14 +824,11 @@ describe("onMessageExternal - EditCommand routing", () => { sendResponse, ) expect(result).toBe(false) - expect(chrome.tabs.create).not.toHaveBeenCalled() + expect(openOptionPage).not.toHaveBeenCalled() }) it("EC-R-03: ignores hub-edit port when sender tab id does not match", () => { - vi.mocked(chrome.tabs.create).mockImplementation((_opts, cb) => { - cb?.({ id: 42 } as chrome.tabs.Tab) - return Promise.resolve({ id: 42 } as chrome.tabs.Tab) - }) + mockOpenOptionPage({ id: 42 }) const listener = getRegisteredListener() listener( { action: "EditCommand", id: "cmd-123" }, @@ -825,10 +851,7 @@ describe("onMessageExternal - EditCommand routing", () => { }) it("EC-R-04: ignores hub-edit port from wrong origin", () => { - vi.mocked(chrome.tabs.create).mockImplementation((_opts, cb) => { - cb?.({ id: 42 } as chrome.tabs.Tab) - return Promise.resolve({ id: 42 } as chrome.tabs.Tab) - }) + mockOpenOptionPage({ id: 42 }) const listener = getRegisteredListener() listener( { action: "EditCommand", id: "cmd-123" }, @@ -851,10 +874,7 @@ describe("onMessageExternal - EditCommand routing", () => { it("EC-R-05: hub-edit connect listener times out after EDIT_CONNECT_TIMEOUT_MS", () => { vi.useFakeTimers() - vi.mocked(chrome.tabs.create).mockImplementation((_opts, cb) => { - cb?.({ id: 42 } as chrome.tabs.Tab) - return Promise.resolve({ id: 42 } as chrome.tabs.Tab) - }) + mockOpenOptionPage({ id: 42 }) const listener = getRegisteredListener() listener( { action: "EditCommand", id: "cmd-123" }, @@ -881,11 +901,8 @@ describe("editCommandToHub", () => { targetUrl: "https://example.com", } as any - function setupConnectedHubEditPort() { - vi.mocked(chrome.tabs.create).mockImplementation((_opts, cb) => { - cb?.({ id: 77 } as chrome.tabs.Tab) - return Promise.resolve({ id: 77 } as chrome.tabs.Tab) - }) + async function setupConnectedHubEditPort(created = true) { + mockOpenOptionPage({ id: 77 }, created) const listener = getRegisteredListener() listener( { action: "EditCommand", id: "cmd-1" }, @@ -903,6 +920,8 @@ describe("editCommandToHub", () => { onDisconnect: { addListener: vi.fn() }, } hubEditConnectListener(port as any) + // Wait until the opened options page tab is registered to the session. + await vi.mocked(openOptionPage).mock.results.at(-1)?.value return port } @@ -914,7 +933,7 @@ describe("editCommandToHub", () => { }) it("EH-02: on ack, updates storage excluding locale/targetUrl", async () => { - const port = setupConnectedHubEditPort() + const port = await setupConnectedHubEditPort() let onAckMessage: ((msg: unknown) => void) | undefined vi.mocked(port.onMessage.addListener).mockImplementation((fn) => { onAckMessage = fn @@ -949,9 +968,27 @@ describe("editCommandToHub", () => { }) }) - it("EH-03: responds with false when ack timeout passes", () => { + it("EH-02-a: keeps the reused options page tab open after ack", async () => { + const port = await setupConnectedHubEditPort(false) + let onAckMessage: ((msg: unknown) => void) | undefined + vi.mocked(port.onMessage.addListener).mockImplementation((fn) => { + onAckMessage = fn + }) + const response = vi.fn() + + editCommandToHub(editParam, sender, response) + onAckMessage?.({ type: "edit-command-ack" }) + + await vi.waitFor(() => { + expect(response).toHaveBeenCalledWith(true) + expect(chrome.tabs.update).toHaveBeenCalledWith(10, { active: true }) + }) + expect(chrome.tabs.remove).not.toHaveBeenCalled() + }) + + it("EH-03: responds with false when ack timeout passes", async () => { vi.useFakeTimers() - const port = setupConnectedHubEditPort() + const port = await setupConnectedHubEditPort() const response = vi.fn() editCommandToHub(editParam, sender, response) @@ -962,9 +999,9 @@ describe("editCommandToHub", () => { vi.useRealTimers() }) - it("EH-04: rejects duplicate request while previous ack is pending", () => { + it("EH-04: rejects duplicate request while previous ack is pending", async () => { vi.useFakeTimers() - setupConnectedHubEditPort() + await setupConnectedHubEditPort() const firstResponse = vi.fn() const secondResponse = vi.fn() diff --git a/packages/extension/src/services/hub/serviceWorker.ts b/packages/extension/src/services/hub/serviceWorker.ts index 2f987c68..687f4cc9 100644 --- a/packages/extension/src/services/hub/serviceWorker.ts +++ b/packages/extension/src/services/hub/serviceWorker.ts @@ -7,6 +7,7 @@ import { SCREEN, } from "@/const" import { Ipc, TabCommand } from "@/services/ipc" +import { openOptionPage } from "@/services/chrome" import type { Sender, ConfirmCommandUpdateProps } from "@/services/ipc" import { Storage, LOCAL_STORAGE_KEY } from "@/services/storage" import type { SubmitCommandInput } from "@/services/hubShare" @@ -554,6 +555,8 @@ export function handleEditCommand( } // Cancel any in-progress session; the new request takes over (last-write-wins). + // Its edit tab is being closed, so it must not be reused for this request. + const closingTabId = _editSession?.editTabId cancelEditSession() const newSession: EditSession = { @@ -602,26 +605,35 @@ export function handleEditCommand( if (_editSession === newSession) _editSession = undefined }, EDIT_CONNECT_TIMEOUT_MS) - chrome.tabs.create( - { - url: `${OPTION_PAGE_PATH}?editCommand=${encodeURIComponent(id)}&syncHub=1#commands`, - }, - (tab) => { + const failToOpen = () => { + cleanupHubEditConnectListener() + if (_editSession === newSession) _editSession = undefined + sendResponse({ result: false, error: "Failed to create edit tab" }) + } + + openOptionPage( + `${OPTION_PAGE_PATH}?editCommand=${encodeURIComponent(id)}&syncHub=1#commands`, + closingTabId, + ) + .then(({ tab, created }) => { if (_editSession !== newSession) { - if (tab?.id != null) chrome.tabs.remove(tab.id) + if (created && tab?.id != null) chrome.tabs.remove(tab.id) sendResponse({ result: false, error: "Edit session timed out" }) return } if (tab?.id == null) { - cleanupHubEditConnectListener() - _editSession = undefined - sendResponse({ result: false, error: "Failed to create edit tab" }) + failToOpen() return } - newSession.editTabId = tab.id + // Close the tab after editing only if it was opened for this edit, so + // that an options page the user already had open is kept. + if (created) newSession.editTabId = tab.id sendResponse({ result: true }) - }, - ) + }) + .catch((err) => { + console.error("[handleEditCommand] Failed to open options page:", err) + failToOpen() + }) return true }