From aeda3c6e32b8abdc194591ed7330fb7439845bd0 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 30 Jul 2026 21:00:36 -0700 Subject: [PATCH] fix(browser): resolve grab mode race conditions and teardown (#11661) - Serialize grab mode operations to prevent concurrent state changes - Wait for guest registration with timeout when page hasn't registered yet - Tear down armed overlay when disabling before selection starts - Distinguish injection-failed from not-ready errors in UI - Track arm generations to cancel stale operations after page switches - Add localized error messages for each failure reason --- src/main/browser/browser-manager-grab.test.ts | 8 + src/main/browser/browser-manager.ts | 11 +- src/main/ipc/browser.test.ts | 258 ++++++++++++++++++ src/main/ipc/browser.ts | 65 ++++- .../browser-pane/useGrabMode.test.ts | 187 +++++++++++++ .../components/browser-pane/useGrabMode.ts | 72 ++++- src/renderer/src/i18n/locales/en.json | 6 + src/renderer/src/i18n/locales/es.json | 6 + src/renderer/src/i18n/locales/ja.json | 6 + src/renderer/src/i18n/locales/ko.json | 6 + src/renderer/src/i18n/locales/zh.json | 6 + src/shared/browser-grab-types.ts | 6 +- 12 files changed, 618 insertions(+), 19 deletions(-) diff --git a/src/main/browser/browser-manager-grab.test.ts b/src/main/browser/browser-manager-grab.test.ts index 43652e6b9..f1e5b650d 100644 --- a/src/main/browser/browser-manager-grab.test.ts +++ b/src/main/browser/browser-manager-grab.test.ts @@ -195,6 +195,14 @@ describe('browserManager grab operations', () => { expect(selection.opId).toBe('op-1') }) + it('tears down an armed overlay when disabling before selection starts', async () => { + const result = await browserManager.setGrabMode('tab-1', false, guest) + + expect(result).toBe(true) + expect(guestExecuteJavaScriptMock).toHaveBeenCalledTimes(1) + expect(guestExecuteJavaScriptMock.mock.calls[0][0]).toContain('grab.cleanup()') + }) + it('returns false if injection fails', async () => { guestExecuteJavaScriptMock.mockRejectedValue(new Error('Injection failed')) const result = await browserManager.setGrabMode('tab-1', true, guest) diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 63ef02efc..7dafca617 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -1610,8 +1610,17 @@ export class BrowserManager { guest: Electron.WebContents ): Promise { if (!enabled) { + const hadActiveGrabOp = this.hasActiveGrabOp(browserTabId) this.cancelGrabOp(browserTabId, 'user') - return true + if (hadActiveGrabOp) { + return true + } + try { + await guest.executeJavaScript(buildGuestOverlayScript('teardown')) + return true + } catch { + return false + } } // Why: inject the overlay runtime eagerly on arm so the hover UI appears instantly; re-injection is idempotent/safe. try { diff --git a/src/main/ipc/browser.test.ts b/src/main/ipc/browser.test.ts index 7651d1306..c09fb2d7f 100644 --- a/src/main/ipc/browser.test.ts +++ b/src/main/ipc/browser.test.ts @@ -8,6 +8,8 @@ const { getGuestWebContentsIdMock, getWebContentsIdByTabIdMock, getWorktreeIdForTabMock, + getAuthorizedGuestMock, + setGrabModeMock, openDevToolsMock, setAnnotationViewportBridgeMock, cancelDownloadMock, @@ -22,6 +24,8 @@ const { getGuestWebContentsIdMock: vi.fn(), getWebContentsIdByTabIdMock: vi.fn(() => new Map()), getWorktreeIdForTabMock: vi.fn(), + getAuthorizedGuestMock: vi.fn(), + setGrabModeMock: vi.fn(), openDevToolsMock: vi.fn().mockResolvedValue(true), setAnnotationViewportBridgeMock: vi.fn().mockResolvedValue(true), cancelDownloadMock: vi.fn(), @@ -53,6 +57,8 @@ vi.mock('../browser/browser-manager', () => ({ getGuestWebContentsId: getGuestWebContentsIdMock, getWebContentsIdByTabId: getWebContentsIdByTabIdMock, getWorktreeIdForTab: getWorktreeIdForTabMock, + getAuthorizedGuest: getAuthorizedGuestMock, + setGrabMode: setGrabModeMock, openDevTools: openDevToolsMock, setAnnotationViewportBridge: setAnnotationViewportBridgeMock, cancelDownload: cancelDownloadMock @@ -79,6 +85,9 @@ describe('registerBrowserHandlers', () => { getWebContentsIdByTabIdMock.mockReset() getWebContentsIdByTabIdMock.mockReturnValue(new Map()) getWorktreeIdForTabMock.mockReset() + getAuthorizedGuestMock.mockReset() + setGrabModeMock.mockReset() + setGrabModeMock.mockResolvedValue(true) openDevToolsMock.mockReset() setAnnotationViewportBridgeMock.mockReset() cancelDownloadMock.mockReset() @@ -331,6 +340,255 @@ describe('registerBrowserHandlers', () => { } }) + it('waits for authorized registration even when an old guest is still live', async () => { + const guest = { id: 123 } as Electron.WebContents + getGuestWebContentsIdMock.mockReturnValue(122) + getAuthorizedGuestMock.mockReturnValueOnce(null).mockReturnValue(guest) + registerBrowserHandlers() + const sender = { + id: 91, + isDestroyed: () => false, + getType: () => 'window', + getURL: () => 'file:///renderer/index.html' + } as Electron.WebContents + const setGrabModeHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:setGrabMode' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { browserPageId: string; enabled: boolean } + ) => Promise + const registerHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:registerGuest' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { + browserPageId: string + workspaceId: string + worktreeId: string + webContentsId: number + } + ) => boolean + + const pendingResult = setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: true }) + await Promise.resolve() + expect(setGrabModeMock).not.toHaveBeenCalled() + + expect( + registerHandler( + { sender }, + { + browserPageId: 'page-1', + workspaceId: 'workspace-1', + worktreeId: 'worktree-1', + webContentsId: 123 + } + ) + ).toBe(true) + + await expect(pendingResult).resolves.toEqual({ ok: true }) + expect(setGrabModeMock).toHaveBeenCalledWith('page-1', true, guest) + }) + + it('returns not-ready when grab registration does not arrive', async () => { + vi.useFakeTimers() + try { + getGuestWebContentsIdMock.mockReturnValue(null) + getAuthorizedGuestMock.mockReturnValue(null) + registerBrowserHandlers() + const sender = { + id: 91, + isDestroyed: () => false, + getType: () => 'window', + getURL: () => 'file:///renderer/index.html' + } as Electron.WebContents + const setGrabModeHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:setGrabMode' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { browserPageId: string; enabled: boolean } + ) => Promise + + const pendingResult = setGrabModeHandler( + { sender }, + { browserPageId: 'page-1', enabled: true } + ) + await vi.advanceTimersByTimeAsync(1_001) + + await expect(pendingResult).resolves.toEqual({ ok: false, reason: 'not-ready' }) + expect(setGrabModeMock).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('does not enable grab mode after a pending request is cancelled', async () => { + const guest = { id: 123 } as Electron.WebContents + let registered = false + getAuthorizedGuestMock.mockImplementation(() => (registered ? guest : null)) + registerBrowserHandlers() + const sender = { + id: 91, + isDestroyed: () => false, + getType: () => 'window', + getURL: () => 'file:///renderer/index.html' + } as Electron.WebContents + const setGrabModeHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:setGrabMode' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { browserPageId: string; enabled: boolean } + ) => Promise + const registerHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:registerGuest' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { + browserPageId: string + workspaceId: string + worktreeId: string + webContentsId: number + } + ) => boolean + + const pendingEnable = setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: true }) + await expect( + setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: false }) + ).resolves.toEqual({ ok: true }) + + registered = true + expect( + registerHandler( + { sender }, + { + browserPageId: 'page-1', + workspaceId: 'workspace-1', + worktreeId: 'worktree-1', + webContentsId: 123 + } + ) + ).toBe(true) + + await expect(pendingEnable).resolves.toEqual({ ok: true }) + expect(setGrabModeMock).not.toHaveBeenCalled() + }) + + it('coalesces a cancelled pending enable into one later enable', async () => { + const guest = { id: 123 } as Electron.WebContents + let registered = false + getAuthorizedGuestMock.mockImplementation(() => (registered ? guest : null)) + registerBrowserHandlers() + const sender = { + id: 91, + isDestroyed: () => false, + getType: () => 'window', + getURL: () => 'file:///renderer/index.html' + } as Electron.WebContents + const setGrabModeHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:setGrabMode' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { browserPageId: string; enabled: boolean } + ) => Promise + const registerHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:registerGuest' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { + browserPageId: string + workspaceId: string + worktreeId: string + webContentsId: number + } + ) => boolean + + const firstEnable = setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: true }) + await setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: false }) + const latestEnable = setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: true }) + + registered = true + registerHandler( + { sender }, + { + browserPageId: 'page-1', + workspaceId: 'workspace-1', + worktreeId: 'worktree-1', + webContentsId: 123 + } + ) + + await expect(Promise.all([firstEnable, latestEnable])).resolves.toEqual([ + { ok: true }, + { ok: true } + ]) + expect(setGrabModeMock).toHaveBeenCalledTimes(1) + expect(setGrabModeMock).toHaveBeenCalledWith('page-1', true, guest) + }) + + it('serializes in-flight mode changes so a stale enable cannot tear down the latest one', async () => { + const guest = { id: 123 } as Electron.WebContents + let resolveFirstEnable!: (success: boolean) => void + const firstEnable = new Promise((resolve) => { + resolveFirstEnable = resolve + }) + getAuthorizedGuestMock.mockReturnValue(guest) + setGrabModeMock.mockReturnValueOnce(firstEnable).mockResolvedValue(true) + registerBrowserHandlers() + const sender = { + id: 91, + isDestroyed: () => false, + getType: () => 'window', + getURL: () => 'file:///renderer/index.html' + } as Electron.WebContents + const setGrabModeHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:setGrabMode' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { browserPageId: string; enabled: boolean } + ) => Promise + + const staleEnable = setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: true }) + await vi.waitFor(() => { + expect(setGrabModeMock).toHaveBeenCalledTimes(1) + }) + const disable = setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: false }) + const latestEnable = setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: true }) + + resolveFirstEnable(true) + await expect(Promise.all([staleEnable, disable, latestEnable])).resolves.toEqual([ + { ok: true }, + { ok: true }, + { ok: true } + ]) + + expect(setGrabModeMock.mock.calls).toEqual([ + ['page-1', true, guest], + ['page-1', true, guest] + ]) + }) + + it('distinguishes picker injection failure from guest readiness', async () => { + const guest = { id: 123 } as Electron.WebContents + getAuthorizedGuestMock.mockReturnValue(guest) + setGrabModeMock.mockResolvedValue(false) + registerBrowserHandlers() + const sender = { + id: 91, + isDestroyed: () => false, + getType: () => 'window', + getURL: () => 'file:///renderer/index.html' + } as Electron.WebContents + const setGrabModeHandler = handleMock.mock.calls.find( + ([channel]) => channel === 'browser:setGrabMode' + )?.[1] as ( + event: { sender: Electron.WebContents }, + args: { browserPageId: string; enabled: boolean } + ) => Promise + + await expect( + setGrabModeHandler({ sender }, { browserPageId: 'page-1', enabled: true }) + ).resolves.toEqual({ ok: false, reason: 'injection-failed' }) + }) + it('resolves worktree and any-tab registration waiters when a guest registers', async () => { vi.useFakeTimers() try { diff --git a/src/main/ipc/browser.ts b/src/main/ipc/browser.ts index f5c1c8fb2..246e821a2 100644 --- a/src/main/ipc/browser.ts +++ b/src/main/ipc/browser.ts @@ -45,6 +45,9 @@ let agentBrowserBridgeRef: AgentBrowserBridge | null = null const pendingTabRegistrations = new Map void>>() const pendingWorktreeTabRegistrations = new Map void>>() const pendingAnyTabRegistrations = new Set<() => void>() +const grabModeIntentByPageId = new Map() +const grabModeOperationByPageId = new Map>() +const GRAB_REGISTRATION_WAIT_MS = 1_000 function waitForRegistrationSet( registrationResolvers: Set<() => void>, @@ -100,6 +103,10 @@ export function waitForTabRegistration(browserPageId: string, timeoutMs = 8_000) if (isLiveBrowserWebContentsId(browserManager.getGuestWebContentsId(browserPageId))) { return Promise.resolve() } + return waitForNextTabRegistration(browserPageId, timeoutMs) +} + +function waitForNextTabRegistration(browserPageId: string, timeoutMs: number): Promise { let registrationResolvers = pendingTabRegistrations.get(browserPageId) if (!registrationResolvers) { registrationResolvers = new Set() @@ -110,6 +117,24 @@ export function waitForTabRegistration(browserPageId: string, timeoutMs = 8_000) }) } +function queueGrabModeOperation( + browserPageId: string, + operation: () => Promise +): Promise { + const previous = grabModeOperationByPageId.get(browserPageId) ?? Promise.resolve() + const result = previous.then(operation) + const completion = result.then( + () => {}, + () => {} + ) + grabModeOperationByPageId.set(browserPageId, completion) + return result.finally(() => { + if (grabModeOperationByPageId.get(browserPageId) === completion) { + grabModeOperationByPageId.delete(browserPageId) + } + }) +} + export function waitForWorktreeTabRegistration( worktreeId: string | undefined, timeoutMs = 8_000 @@ -168,6 +193,7 @@ function isTrustedBrowserRenderer(sender: Electron.WebContents): boolean { } export function registerBrowserHandlers(): void { + grabModeIntentByPageId.clear() ipcMain.removeHandler('browser:registerGuest') ipcMain.removeHandler('browser:unregisterGuest') ipcMain.removeHandler('browser:openDevTools') @@ -237,6 +263,7 @@ export function registerBrowserHandlers(): void { agentBrowserBridgeRef.onTabClosed(wcId) } browserManager.unregisterGuest(args.browserPageId) + grabModeIntentByPageId.delete(args.browserPageId) return true }) @@ -370,12 +397,44 @@ export function registerBrowserHandlers(): void { if (!isTrustedBrowserRenderer(event.sender)) { return { ok: false, reason: 'not-authorized' } } - const guest = browserManager.getAuthorizedGuest(args.browserPageId, event.sender.id) + const intent = { + generation: (grabModeIntentByPageId.get(args.browserPageId)?.generation ?? 0) + 1, + enabled: args.enabled + } + grabModeIntentByPageId.set(args.browserPageId, intent) + const isCurrentIntent = (): boolean => + grabModeIntentByPageId.get(args.browserPageId) === intent + let guest = browserManager.getAuthorizedGuest(args.browserPageId, event.sender.id) + if (!guest && args.enabled) { + // Why: fast file:// pages can expose the toolbar before did-attach registration reaches main. + await waitForNextTabRegistration(args.browserPageId, GRAB_REGISTRATION_WAIT_MS).catch( + () => {} + ) + if (!isCurrentIntent()) { + return { ok: true } + } + guest = browserManager.getAuthorizedGuest(args.browserPageId, event.sender.id) + } if (!guest) { + if (!args.enabled) { + return { ok: true } + } return { ok: false, reason: 'not-ready' } } - const success = await browserManager.setGrabMode(args.browserPageId, args.enabled, guest) - return success ? { ok: true } : { ok: false, reason: 'not-ready' } + return queueGrabModeOperation(args.browserPageId, async () => { + if (!isCurrentIntent()) { + return { ok: true } + } + guest = browserManager.getAuthorizedGuest(args.browserPageId, event.sender.id) + if (!guest) { + return args.enabled ? { ok: false, reason: 'not-ready' } : { ok: true } + } + const success = await browserManager.setGrabMode(args.browserPageId, args.enabled, guest) + if (!isCurrentIntent()) { + return { ok: true } + } + return success ? { ok: true } : { ok: false, reason: 'injection-failed' } + }) } ) diff --git a/src/renderer/src/components/browser-pane/useGrabMode.test.ts b/src/renderer/src/components/browser-pane/useGrabMode.test.ts index 048a3dc91..e912d74a0 100644 --- a/src/renderer/src/components/browser-pane/useGrabMode.test.ts +++ b/src/renderer/src/components/browser-pane/useGrabMode.test.ts @@ -84,4 +84,191 @@ describe('useGrabMode', () => { enabled: true }) }) + + it('reports a picker injection failure without calling it a readiness error', async () => { + const harness = createReactHookHarness() + vi.doMock('react', () => harness.react) + vi.doMock('@/hooks/useMountedRef', () => ({ + useMountedRef: () => ({ current: true }) + })) + vi.stubGlobal('window', { + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + api: { + browser: { + setGrabMode: vi.fn(async () => ({ ok: false, reason: 'injection-failed' })), + awaitGrabSelection: vi.fn(), + cancelGrab: vi.fn() + } + } + }) + const { useGrabMode } = await import('./useGrabMode') + const render = () => { + harness.beginRender() + // oxlint-disable-next-line react-hooks/rules-of-hooks -- test harness mocks React's hook dispatcher directly. + return useGrabMode('page-1') + } + + render().toggle() + await Promise.resolve() + + const grab = render() + expect(grab.state).toBe('error') + expect(grab.error).toBe('Could not start element selection on this page.') + }) + + it('arms immediately and treats a second toggle as cancellation while enable is pending', async () => { + const harness = createReactHookHarness() + let resolveEnable!: (result: { ok: true }) => void + const pendingEnable = new Promise<{ ok: true }>((resolve) => { + resolveEnable = resolve + }) + const setGrabMode = vi.fn(async ({ enabled }: { enabled: boolean }) => + enabled ? pendingEnable : ({ ok: true } as const) + ) + const awaitGrabSelection = vi.fn(() => new Promise(() => {})) + const cancelGrab = vi.fn() + vi.doMock('react', () => harness.react) + vi.doMock('@/hooks/useMountedRef', () => ({ + useMountedRef: () => ({ current: true }) + })) + vi.stubGlobal('window', { + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + api: { + browser: { setGrabMode, awaitGrabSelection, cancelGrab } + } + }) + const { useGrabMode } = await import('./useGrabMode') + const render = () => { + harness.beginRender() + // oxlint-disable-next-line react-hooks/rules-of-hooks -- test harness mocks React's hook dispatcher directly. + return useGrabMode('page-1') + } + + const grab = render() + grab.toggle() + expect(render().state).toBe('armed') + + grab.toggle() + expect(setGrabMode.mock.calls.filter(([args]) => args.enabled)).toHaveLength(1) + expect(render().state).toBe('idle') + + resolveEnable({ ok: true }) + await pendingEnable + await Promise.resolve() + + expect(awaitGrabSelection).not.toHaveBeenCalled() + await vi.waitFor(() => { + expect(cancelGrab).toHaveBeenCalledWith({ browserPageId: 'page-1' }) + }) + }) + + it('cancels the tab owning the grab when the page changes before effects run', async () => { + const harness = createReactHookHarness() + const setGrabMode = vi.fn(async () => ({ ok: true })) + const awaitGrabSelection = vi.fn(() => new Promise(() => {})) + const cancelGrab = vi.fn() + vi.doMock('react', () => harness.react) + vi.doMock('@/hooks/useMountedRef', () => ({ + useMountedRef: () => ({ current: true }) + })) + vi.stubGlobal('window', { + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + api: { + browser: { setGrabMode, awaitGrabSelection, cancelGrab } + } + }) + const { useGrabMode } = await import('./useGrabMode') + const render = (browserPageId: string) => { + harness.beginRender() + // oxlint-disable-next-line react-hooks/rules-of-hooks -- test harness mocks React's hook dispatcher directly. + return useGrabMode(browserPageId) + } + + render('page-1').toggle() + await vi.waitFor(() => { + expect(awaitGrabSelection).toHaveBeenCalledTimes(1) + }) + + render('page-2').cancel() + + expect(setGrabMode).toHaveBeenLastCalledWith({ + browserPageId: 'page-1', + enabled: false + }) + expect(cancelGrab).toHaveBeenCalledWith({ browserPageId: 'page-1' }) + }) + + it('ignores a stale screenshot after restarting on the same page', async () => { + const harness = createReactHookHarness() + let resolveScreenshot!: (result: { + ok: true + screenshot: { dataUrl: string; width: number; height: number } + }) => void + const pendingScreenshot = new Promise<{ + ok: true + screenshot: { dataUrl: string; width: number; height: number } + }>((resolve) => { + resolveScreenshot = resolve + }) + const awaitGrabSelection = vi + .fn() + .mockResolvedValueOnce({ + kind: 'selected', + payload: { + target: { + rectViewport: { x: 0, y: 0, width: 1, height: 1 } + } + } + }) + .mockImplementation(() => new Promise(() => {})) + const captureSelectionScreenshot = vi.fn(() => pendingScreenshot) + vi.doMock('react', () => harness.react) + vi.doMock('@/hooks/useMountedRef', () => ({ + useMountedRef: () => ({ current: true }) + })) + vi.stubGlobal('window', { + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + api: { + browser: { + setGrabMode: vi.fn(async () => ({ ok: true })), + awaitGrabSelection, + captureSelectionScreenshot, + cancelGrab: vi.fn() + } + } + }) + const { useGrabMode } = await import('./useGrabMode') + const render = () => { + harness.beginRender() + // oxlint-disable-next-line react-hooks/rules-of-hooks -- test harness mocks React's hook dispatcher directly. + return useGrabMode('page-1') + } + + render().toggle() + await vi.waitFor(() => { + expect(captureSelectionScreenshot).toHaveBeenCalledTimes(1) + }) + + render().cancel() + render().toggle() + await vi.waitFor(() => { + expect(awaitGrabSelection).toHaveBeenCalledTimes(2) + }) + + resolveScreenshot({ + ok: true, + screenshot: { dataUrl: 'data:image/png;base64,AA==', width: 1, height: 1 } + }) + await pendingScreenshot + + await vi.waitFor(() => { + const grab = render() + expect(grab.state).toBe('awaiting') + expect(grab.payload).toBeNull() + }) + }) }) diff --git a/src/renderer/src/components/browser-pane/useGrabMode.ts b/src/renderer/src/components/browser-pane/useGrabMode.ts index ccf9851ff..b3ee25805 100644 --- a/src/renderer/src/components/browser-pane/useGrabMode.ts +++ b/src/renderer/src/components/browser-pane/useGrabMode.ts @@ -1,9 +1,11 @@ import { useCallback, useEffect, useRef, useState } from 'react' import type { BrowserGrabPayload, + BrowserGrabRejectReason, BrowserGrabScreenshot } from '../../../../shared/browser-grab-types' import { useMountedRef } from '@/hooks/useMountedRef' +import { translate } from '@/i18n/i18n' import { isEditableKeyboardTarget } from './browser-keyboard' // --------------------------------------------------------------------------- @@ -32,6 +34,31 @@ function nextOpId(): string { return `grab-${++opIdCounter}-${Date.now()}` } +function getGrabEnableError(reason: BrowserGrabRejectReason): string { + switch (reason) { + case 'not-ready': + return translate( + 'auto.components.browser-pane.grab.errorNotReady', + "This page isn't ready for element selection yet." + ) + case 'not-authorized': + return translate( + 'auto.components.browser-pane.grab.errorNotAuthorized', + 'Browser access was denied.' + ) + case 'already-active': + return translate( + 'auto.components.browser-pane.grab.errorAlreadyActive', + 'Element selection is already active.' + ) + case 'injection-failed': + return translate( + 'auto.components.browser-pane.grab.errorInjectionFailed', + 'Could not start element selection on this page.' + ) + } +} + /** * Hook that drives the browser grab lifecycle for a single browser page. * @@ -45,6 +72,7 @@ export function useGrabMode(browserPageId: string): GrabModeHook { const [contextMenu, setContextMenu] = useState(false) const activeOpIdRef = useRef(null) const grabTabIdRef = useRef(null) + const armGenerationRef = useRef(0) const browserTabIdRef = useRef(browserPageId) // Why: toolbar/key handlers from the latest render can fire before passive // effects run after a page switch, so keep the target page current in render. @@ -58,6 +86,7 @@ export function useGrabMode(browserPageId: string): GrabModeHook { return () => { const grabTabId = grabTabIdRef.current if (grabTabId) { + armGenerationRef.current += 1 void window.api.browser.setGrabMode({ browserPageId: grabTabId, enabled: false }) void window.api.browser.cancelGrab({ browserPageId: grabTabId }) grabTabIdRef.current = null @@ -68,7 +97,9 @@ export function useGrabMode(browserPageId: string): GrabModeHook { const armAndAwait = useCallback(async () => { const tabId = browserTabIdRef.current + const armGeneration = (armGenerationRef.current += 1) grabTabIdRef.current = tabId + setState('armed') // Enable grab mode — injects the overlay const setResult = await window.api.browser.setGrabMode({ @@ -77,25 +108,28 @@ export function useGrabMode(browserPageId: string): GrabModeHook { }) if ( !mountedRef.current || + armGenerationRef.current !== armGeneration || browserTabIdRef.current !== tabId || grabTabIdRef.current !== tabId ) { - void window.api.browser.setGrabMode({ browserPageId: tabId, enabled: false }) - void window.api.browser.cancelGrab({ browserPageId: tabId }) - if (grabTabIdRef.current === tabId) { - grabTabIdRef.current = null + const supersededBySameTab = + armGenerationRef.current !== armGeneration && grabTabIdRef.current === tabId + if (!supersededBySameTab) { + void window.api.browser.setGrabMode({ browserPageId: tabId, enabled: false }) + void window.api.browser.cancelGrab({ browserPageId: tabId }) + if (grabTabIdRef.current === tabId) { + grabTabIdRef.current = null + } } return } if (!setResult.ok) { grabTabIdRef.current = null setState('error') - setError(`Cannot enable grab mode: ${setResult.reason}`) + setError(getGrabEnableError(setResult.reason)) return } - setState('armed') - // Generate opId and await selection const opId = nextOpId() activeOpIdRef.current = opId @@ -127,7 +161,11 @@ export function useGrabMode(browserPageId: string): GrabModeHook { } catch { // Screenshot failure is non-fatal } - if (!mountedRef.current || grabTabIdRef.current !== tabId) { + if ( + !mountedRef.current || + armGenerationRef.current !== armGeneration || + grabTabIdRef.current !== tabId + ) { return } @@ -146,20 +184,22 @@ export function useGrabMode(browserPageId: string): GrabModeHook { }, [mountedRef]) const toggle = useCallback(() => { - if (state === 'idle' || state === 'error') { + if ((state === 'idle' || state === 'error') && grabTabIdRef.current === null) { setError(null) setPayload(null) setContextMenu(false) void armAndAwait() } else { // Disable grab mode + const targetTabId = grabTabIdRef.current ?? browserTabIdRef.current + armGenerationRef.current += 1 void window.api.browser.setGrabMode({ - browserPageId: browserTabIdRef.current, + browserPageId: targetTabId, enabled: false }) if (activeOpIdRef.current) { void window.api.browser.cancelGrab({ - browserPageId: browserTabIdRef.current + browserPageId: targetTabId }) activeOpIdRef.current = null } @@ -172,13 +212,15 @@ export function useGrabMode(browserPageId: string): GrabModeHook { }, [state, armAndAwait]) const cancel = useCallback(() => { + const targetTabId = grabTabIdRef.current ?? browserTabIdRef.current + armGenerationRef.current += 1 void window.api.browser.setGrabMode({ - browserPageId: browserTabIdRef.current, + browserPageId: targetTabId, enabled: false }) if (activeOpIdRef.current) { void window.api.browser.cancelGrab({ - browserPageId: browserTabIdRef.current + browserPageId: targetTabId }) activeOpIdRef.current = null } @@ -204,8 +246,10 @@ export function useGrabMode(browserPageId: string): GrabModeHook { }, [armAndAwait]) const exit = useCallback(() => { + const targetTabId = grabTabIdRef.current ?? browserTabIdRef.current + armGenerationRef.current += 1 void window.api.browser.setGrabMode({ - browserPageId: browserTabIdRef.current, + browserPageId: targetTabId, enabled: false }) // Why: clear the active opId so that any in-flight result from the diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index edc3de12f..9e1cd9868 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -14327,6 +14327,12 @@ }, "undo": "Undo", "widthOption": "{{value0}} px" + }, + "grab": { + "errorNotReady": "This page isn't ready for element selection yet.", + "errorNotAuthorized": "Browser access was denied.", + "errorAlreadyActive": "Element selection is already active.", + "errorInjectionFailed": "Could not start element selection on this page." } }, "pluginCatalog": { diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index ee83b6d36..ae9d9dcff 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -14267,6 +14267,12 @@ }, "undo": "Deshacer", "widthOption": "{{value0}} px" + }, + "grab": { + "errorNotReady": "Esta página aún no está lista para seleccionar elementos.", + "errorNotAuthorized": "Se denegó el acceso al navegador.", + "errorAlreadyActive": "La selección de elementos ya está activa.", + "errorInjectionFailed": "No se pudo iniciar la selección de elementos en esta página." } }, "pluginCatalog": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index de0d97977..ac17b89b6 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -14267,6 +14267,12 @@ }, "undo": "元に戻す", "widthOption": "{{value0}} px" + }, + "grab": { + "errorNotReady": "このページではまだ要素を選択できません。", + "errorNotAuthorized": "ブラウザーへのアクセスが拒否されました。", + "errorAlreadyActive": "要素の選択はすでに有効です。", + "errorInjectionFailed": "このページで要素の選択を開始できませんでした。" } }, "pluginCatalog": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 2670e6df9..b48396752 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -14267,6 +14267,12 @@ }, "undo": "실행 취소", "widthOption": "{{value0}} px" + }, + "grab": { + "errorNotReady": "이 페이지에서는 아직 요소를 선택할 수 없습니다.", + "errorNotAuthorized": "브라우저 접근이 거부되었습니다.", + "errorAlreadyActive": "요소 선택이 이미 활성화되어 있습니다.", + "errorInjectionFailed": "이 페이지에서 요소 선택을 시작할 수 없습니다." } }, "pluginCatalog": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 79ac74754..75e5cea04 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -14267,6 +14267,12 @@ }, "undo": "撤销", "widthOption": "{{value0}} px" + }, + "grab": { + "errorNotReady": "此页面尚未准备好进行元素选择。", + "errorNotAuthorized": "浏览器访问被拒绝。", + "errorAlreadyActive": "元素选择已处于活动状态。", + "errorInjectionFailed": "无法在此页面上启动元素选择。" } }, "pluginCatalog": { diff --git a/src/shared/browser-grab-types.ts b/src/shared/browser-grab-types.ts index f1dd59656..09a0ae07b 100644 --- a/src/shared/browser-grab-types.ts +++ b/src/shared/browser-grab-types.ts @@ -121,7 +121,11 @@ export type BrowserSetGrabModeArgs = { } /** Why a grab IPC call was rejected before the operation could start. */ -export type BrowserGrabRejectReason = 'not-ready' | 'not-authorized' | 'already-active' +export type BrowserGrabRejectReason = + | 'not-ready' + | 'not-authorized' + | 'already-active' + | 'injection-failed' export type BrowserSetGrabModeResult = { ok: true } | { ok: false; reason: BrowserGrabRejectReason }