diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 0862ac31fe7..a256da5112c 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -703,6 +703,7 @@ jobs: src/main/providers/windows-conpty-wide-char-duplication.node-pty.test.ts src/main/providers/pty-repaint-wide-char-buffer.node-pty.test.ts src/shared/child-process/windows-command-line.win32.test.ts + src/main/agent-hooks/windows-hook-payload-delivery.test.ts src/main/windows/windows-pty-job.win32.test.ts src/main/windows/windows-host-job.win32.test.ts src/main/wsl/wsl-runner.test.ts diff --git a/docs/reference/windows-process-enumeration.md b/docs/reference/windows-process-enumeration.md index 115cc7040a9..e4c91329c46 100644 --- a/docs/reference/windows-process-enumeration.md +++ b/docs/reference/windows-process-enumeration.md @@ -46,6 +46,40 @@ risk — but the headroom is roughly 2x on time and 1.7x on bytes, not the ~4x t parse, and the read rejects, so a busy host loses the table rather than receiving a wrong one. +## When a read wedges + +The vendored reader pushes every callback onto a module-global queue and drains +that queue only when the request holding its `requestInProgress` latch +completes. If a Toolhelp32 snapshot never comes back — an EDR hook, a restricted +token, a worker that dies — the latch is stuck for the life of the process and +every later call parks another closure in that queue. + +Two guards, and they work together: + +- a **3 s deadline** on each read, so a caller gets a rejection instead of a + promise that never settles; +- a **sticky wedge**: once a read misses its deadline and has not called back, + the module refuses every further read until that read's callback fires. + +The wedge used to be a 30 s cooldown that let one probe through per window. That +bounded the *rate* of new callbacks but not the total: a permanently wedged +reader retained one more closure every 30 s for as long as the app ran, and each +probe also blocked its caller for the full 3 s deadline first. Gating on the +outstanding read instead bounds retention at exactly one callback, and gives up +nothing on recovery — a probe queued behind the latch could never have observed +recovery anyway, whereas the stuck callback firing is the drain itself. On the +relay's bare addon, which has no queue of its own, it is also what keeps Orca +from re-entering `CreateToolhelp32Snapshot` while a call is still running. + +That last part is not just tidiness. The addon runs each read as a +`Napi::AsyncWorker`, so a wedged read holds a libuv threadpool slot for good. On +the relay the JS queue is not there to absorb the retries, so one probe per +window would have pinned all four default threads inside ~2 minutes — hanging +every async `fs` and DNS call in that process, not only the process table. + +A wedge does **not** engage the PowerShell fallback; see the next section for +why only absence does. + ## The relay has no binding, and falls back Relay deployment installs only `node-pty` and `@parcel/watcher` on the remote diff --git a/src/main/agent-hooks/installer-utils.test.ts b/src/main/agent-hooks/installer-utils.test.ts index 88c73550da2..3cf2525ba1e 100644 --- a/src/main/agent-hooks/installer-utils.test.ts +++ b/src/main/agent-hooks/installer-utils.test.ts @@ -604,7 +604,7 @@ describe('wrapPosixHookCommand', () => { }) const qualifiedWindowsPowerShellCommand = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand \S+$/ + /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ function decodeWindowsHookCommand(command: string): string { const encodedCommand = command.match(/ -EncodedCommand (\S+)$/)?.[1] @@ -762,7 +762,7 @@ describe('wrapRuntimeHomeHookCommand', () => { // applies to the exact same string (#16003). const command = wrapRuntimeHomeHookCommand('claude-hook') - expect(command).toContain('powershell.exe" -NoProfile -WindowStyle Hidden -EncodedCommand ') + expect(command).toContain('powershell.exe" -NoProfile -EncodedCommand ') expect(command).not.toMatch(/-ExecutionPolicy/i) }) diff --git a/src/main/agent-hooks/windows-powershell-hook-launcher.test.ts b/src/main/agent-hooks/windows-powershell-hook-launcher.test.ts index e58fd25c960..df307d87035 100644 --- a/src/main/agent-hooks/windows-powershell-hook-launcher.test.ts +++ b/src/main/agent-hooks/windows-powershell-hook-launcher.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import { encodeWindowsPowerShellHookCommand, + getWindowsPowerShellExecutablePath, WINDOWS_POWERSHELL_HOOK_SWITCHES, wrapWindowsPowerShellEncodedCommand } from './windows-powershell-hook-launcher' @@ -12,38 +13,41 @@ function decodePayload(command: string): string { } /* - * Two reproduced Windows failures constrain this one string, in opposite - * directions: + * #16003 — endpoint security (Kaspersky, Windows 11) denies process creation + * for the encoded launcher shape whatever the payload decodes to, and no + * exclusion re-enabled it. Measured on the reporting host: the denied shape + * exits 126 with or without the policy switch and with or without `-NoProfile`, + * while `-NoProfile -EncodedCommand` alone runs 5/5. So the command line spells + * neither the policy bypass (moved into the payload by #16576) nor the denied + * window switch. * - * #16003 — endpoint security (Kaspersky Premium, Windows 11) denies process - * creation for `-ExecutionPolicy Bypass -WindowStyle Hidden -EncodedCommand` - * whatever the payload decodes to, and no exclusion re-enabled it. The triple - * must stop being spelled. - * - * #14815 (+ #14828, #15117, #15447, #15767) — without `-WindowStyle Hidden` - * every hook event allocates a console that takes foreground and eats the - * user's keystrokes, and strands a visible window outright when the hook blocks - * on stdin. Window suppression must stay. - * - * Both hold only if the flag that leaves the command line is the policy bypass, - * which is the one with an exact in-payload equivalent. + * The counter-pressure is real and is NOT resolved by these assertions: + * #14815 (+ #14828, #15117, #15447, #15767) reported consoles taking + * foreground. The previous suppression switch was that fix, but its behavior + * was never measured — see the launcher's comment. These tests pin the shape + * so it cannot be restored silently; whether a console appears is a question + * for a live window measurement, which no unit test here can answer. */ describe('windows PowerShell hook launcher', () => { - it('never spells the denied flag triple on the command line', () => { + it('never spells a denied flag on the command line', () => { const command = wrapWindowsPowerShellEncodedCommand('exit 0') + const switches = command.replace(/ -EncodedCommand \S+$/, '') expect(WINDOWS_POWERSHELL_HOOK_SWITCHES).not.toMatch(/-ExecutionPolicy/i) - expect(command.replace(/ -EncodedCommand \S+$/, '')).not.toMatch(/-ExecutionPolicy/i) + expect(switches).not.toMatch(/-ExecutionPolicy/i) + // Why: measured exit 126 for the denied switch paired with -EncodedCommand. + expect(WINDOWS_POWERSHELL_HOOK_SWITCHES).toBe('-NoProfile') + expect(switches).toBe(`${getWindowsPowerShellExecutablePath()} -NoProfile`) }) - it('keeps hiding the console window on every hook event (#14815)', () => { - // Dropping this flag is not a cosmetic flash: the console takes foreground - // and swallows what the user is typing into Orca (#14828), and never closes - // at all when the hook blocks reading stdin (#14815). + it('pins the exact launcher shape so a flag cannot come back unnoticed', () => { + // Why this is worth a test: #16576 round 3 dropped a required launcher + // invariant by accident and updated the tests to match, so nothing caught + // it. Restoring either denied switch re-breaks every hook on an AV host. const command = wrapWindowsPowerShellEncodedCommand('exit 0') - expect(WINDOWS_POWERSHELL_HOOK_SWITCHES).toBe('-NoProfile -WindowStyle Hidden') - expect(command).toMatch(/ -NoProfile -WindowStyle Hidden -EncodedCommand [A-Za-z0-9+/=]+$/) + expect(WINDOWS_POWERSHELL_HOOK_SWITCHES).toBe('-NoProfile') + expect(command).toMatch(/ -NoProfile -EncodedCommand [A-Za-z0-9+/=]+$/) }) it('keeps the execution-policy bypass, in the payload where AV cannot read it', () => { diff --git a/src/main/agent-hooks/windows-powershell-hook-launcher.ts b/src/main/agent-hooks/windows-powershell-hook-launcher.ts index aeec04380db..b9a9f6dd208 100644 --- a/src/main/agent-hooks/windows-powershell-hook-launcher.ts +++ b/src/main/agent-hooks/windows-powershell-hook-launcher.ts @@ -14,26 +14,33 @@ export function getWindowsPowerShellExecutablePath(): string { * Switches for the PowerShell that relays hook output and exit status * (#14818 — conhost does neither). * - * `-WindowStyle Hidden` stays. It is the shipped fix for #14815 and its four - * duplicates (#14828, #15117, #15447, #15767): without it Windows allocates a - * console per hook event, which steals foreground from whatever the user is - * typing into, and strands a permanently visible window whenever a hook blocks - * reading stdin (see hook-stdin-contract.ts). Those are reported, reproduced - * user-facing failures on every hook event of every managed agent. + * The command line spells no flag beyond `-NoProfile`, because AV denies the + * combinations. #16003 measured, on the reporting Kaspersky host: * - * `-ExecutionPolicy Bypass` is the flag that leaves the command line, because - * it is the only one of the three with an exact in-payload equivalent (below): - * it costs nothing to move. #16003 measured `-NoProfile -ExecutionPolicy Bypass - * -WindowStyle Hidden -EncodedCommand` as denied at CreateProcess (exit 126, - * Kaspersky Premium on Windows 11) no matter what the payload decodes to, so - * the triple has to stop being spelled; dropping the policy flag breaks it. + * -NoProfile -ExecutionPolicy Bypass -WindowStyle Hidden -EncodedCommand 126 + * -NoProfile -WindowStyle Hidden -EncodedCommand 126 + * -WindowStyle Hidden -EncodedCommand 126 + * -NoProfile -EncodedCommand 0 (5/5) + * -NoProfile -ExecutionPolicy Bypass -Command 0 (5/5) * - * What is not established: that the remaining pair clears that AV signature — - * the reporter measured no two-flag encoded shape. If it turns out not to, the - * answer is another launcher shape that still suppresses the window, not - * trading a reproduced regression for an unmeasured hope. + * The denial is at CreateProcess and is independent of the payload: `exit 0` is + * denied too, and bash reports it as `Permission denied`. So `-WindowStyle + * Hidden` + `-EncodedCommand` is the pair that has to stop being spelled. + * #16576 removed `-ExecutionPolicy Bypass` (kept in-payload below), which was + * the one flag of the three NOT in the signature — hooks kept failing. + * + * `-WindowStyle Hidden` was the shipped fix for #14815 (+#14828, #15117, + * #15447, #15767). Removing it is a real tradeoff and is recorded as such: its + * suppression was never measured — #14825 confirmed it *visually*, #16576's + * author stated it "remains unverified on a real box", and #15506's author + * argued it cannot help a `.cmd` child that has no console to inherit. The + * console is allocated by the parent chain, not by this command line. + * + * Do not restore the flag to fix a console report. That trades every hook on an + * AV host for a flicker. The answer is to shorten the interpreter chain — the + * shipped doctrine of #15520 and #15595 — or a launcher that owns no console. */ -export const WINDOWS_POWERSHELL_HOOK_SWITCHES = '-NoProfile -WindowStyle Hidden' +export const WINDOWS_POWERSHELL_HOOK_SWITCHES = '-NoProfile' // Why: redirected PowerShell progress becomes CLIXML that can corrupt merged JSON // output. It must be the FIRST statement: Set-ExecutionPolicy autoloads diff --git a/src/main/browser/browser-clicked-link-routing.test.ts b/src/main/browser/browser-clicked-link-routing.test.ts index eee5eea35f1..24c26471c3a 100644 --- a/src/main/browser/browser-clicked-link-routing.test.ts +++ b/src/main/browser/browser-clicked-link-routing.test.ts @@ -64,7 +64,7 @@ describe('browser clicked-link routing', () => { vi.restoreAllMocks() }) - it('routes plain target=_blank links back into the current Orca tab', () => { + it('routes plain target=_blank links into a new Orca tab', () => { const link = document.createElement('a') link.href = 'https://docs.example.com/guide' link.target = '_blank' @@ -73,9 +73,8 @@ describe('browser clicked-link routing', () => { const { event, open } = clickLink(link) - expect(event.defaultPrevented).toBe(false) - expect(open).not.toHaveBeenCalled() - expect(link.getAttribute('target')).toBe('_self') + expect(event.defaultPrevented).toBe(true) + expect(open).toHaveBeenCalledWith('https://docs.example.com/guide', FOREGROUND_FRAME_NAME) }) it('routes the host-platform modifier without trusting an emulated guest user agent', () => { @@ -127,9 +126,11 @@ describe('browser clicked-link routing', () => { expect(clickLink(cancelled).open).not.toHaveBeenCalled() expect(cancelled.getAttribute('target')).toBe('_blank') - expect(clickLink(rewritten).open).not.toHaveBeenCalled() - expect(rewritten.href).toBe('https://example.com/rewritten') - expect(rewritten.getAttribute('target')).toBe('_self') + // The page's own handler ran first, so routing must follow the rewritten href. + expect(clickLink(rewritten).open).toHaveBeenCalledWith( + 'https://example.com/rewritten', + FOREGROUND_FRAME_NAME + ) }) it('routes SVG links but leaves download links and links without href alone', () => { @@ -145,8 +146,10 @@ describe('browser clicked-link routing', () => { document.body.append(svgLink, areaDownload, noHref) installRouting() - expect(clickLink(svgLink).open).not.toHaveBeenCalled() - expect(svgLink.getAttribute('target')).toBe('_self') + expect(clickLink(svgLink).open).toHaveBeenCalledWith( + 'https://example.com/svg', + FOREGROUND_FRAME_NAME + ) expect(clickLink(areaDownload).open).not.toHaveBeenCalled() expect(clickLink(noHref).open).not.toHaveBeenCalled() }) @@ -195,7 +198,7 @@ describe('browser clicked-link routing', () => { expect(script).not.toContain('BrowserClickedLinkRoutingState') }) - it('routes plain iframe target=_blank links into the top-level guest', () => { + it('routes plain iframe target=_blank links into a new Orca tab', () => { const link = document.createElement('a') link.href = 'https://example.com/from-frame' link.target = '_blank' @@ -204,9 +207,8 @@ describe('browser clicked-link routing', () => { const { event, open } = clickLink(link) - expect(event.defaultPrevented).toBe(false) - expect(open).not.toHaveBeenCalled() - expect(link.getAttribute('target')).toBe('_top') + expect(event.defaultPrevented).toBe(true) + expect(open).toHaveBeenCalledWith('https://example.com/from-frame', FOREGROUND_FRAME_NAME) }) it('routes explicit iframe new-tab gestures through one-use frame names', () => { diff --git a/src/main/browser/browser-clicked-link-routing.ts b/src/main/browser/browser-clicked-link-routing.ts index 42a8bbf8232..ee6f2fc43d5 100644 --- a/src/main/browser/browser-clicked-link-routing.ts +++ b/src/main/browser/browser-clicked-link-routing.ts @@ -75,8 +75,9 @@ export function installBrowserClickedLinkRouting( const baseTarget = document.querySelector('base[target]')?.getAttribute('target') ?? '' const ownTarget = link.getAttribute('target') const effectiveTarget = (ownTarget === null ? baseTarget : ownTarget).trim().toLowerCase() - const opensNewContext = middleClick || modifierClick - if (!opensNewContext && effectiveTarget !== '_blank') { + // target=_blank is a new-tab request, exactly as it is in every other + // browser; the modifiers are the other two ways to ask for one. + if (!(middleClick || modifierClick || effectiveTarget === '_blank')) { return } @@ -96,13 +97,6 @@ export function installBrowserClickedLinkRouting( return } - if (!opensNewContext) { - // Why: changing only the browsing context keeps Chromium's native anchor - // navigation, including referrer policy, attribution, and history. - link.setAttribute('target', '_self') - return - } - // Why: Electron reports direct link clicks and featureless window.open() // with the same disposition. The private frame name preserves that one // distinction without weakening OAuth popups that need window.opener. @@ -118,8 +112,8 @@ export function installBrowserClickedLinkRouting( } /** - * Keeps plain target=_blank clicks inside the top-level guest when Electron's - * isolated-world API cannot target a child frame. + * Same routing for links inside child frames, which Electron's isolated-world + * API cannot reach, so this runs in the page world against a one-use token. */ export function installBrowserIframeClickedLinkRouting( frameName: string, @@ -161,8 +155,7 @@ export function installBrowserIframeClickedLinkRouting( const baseTarget = document.querySelector('base[target]')?.getAttribute('target') ?? '' const ownTarget = link.getAttribute('target') const effectiveTarget = (ownTarget === null ? baseTarget : ownTarget).trim().toLowerCase() - const opensNewContext = middleClick || modifierClick - if (!opensNewContext && effectiveTarget !== '_blank') { + if (!(middleClick || modifierClick || effectiveTarget === '_blank')) { return } @@ -182,13 +175,6 @@ export function installBrowserIframeClickedLinkRouting( return } - if (!opensNewContext) { - // Why: WebContents isolated worlds only cover the main frame. Rewriting - // to `_top` preserves native anchor semantics without opening a popup. - link.setAttribute('target', '_top') - return - } - // Why: child-frame code runs in the page world, so each token is one-use. // A page that observes a real click cannot replay it to create more tabs. event.preventDefault() diff --git a/src/main/browser/browser-manager-popup-child.test.ts b/src/main/browser/browser-manager-popup-child.test.ts new file mode 100644 index 00000000000..bf4caf2b0ba --- /dev/null +++ b/src/main/browser/browser-manager-popup-child.test.ts @@ -0,0 +1,253 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const browserMocks = vi.hoisted(() => ({ + appGetPathMock: vi.fn(() => '/downloads'), + menuBuildFromTemplateMock: vi.fn(), + guestOffMock: vi.fn(), + guestOnMock: vi.fn(), + guestSetBackgroundThrottlingMock: vi.fn(), + guestSetWindowOpenHandlerMock: vi.fn(), + guestOpenDevToolsMock: vi.fn(), + webContentsFromIdMock: vi.fn(), + browserWindowFromWebContentsMock: vi.fn(), + screenGetCursorScreenPointMock: vi.fn(() => ({ x: 0, y: 0 })), + shellOpenExternalMock: vi.fn(), + openPopupWithOriginBarMock: vi.fn() +})) + +vi.mock('electron', () => ({ + app: { getPath: browserMocks.appGetPathMock }, + BrowserWindow: { fromWebContents: browserMocks.browserWindowFromWebContentsMock }, + clipboard: { writeText: vi.fn() }, + shell: { openExternal: browserMocks.shellOpenExternalMock }, + Menu: { buildFromTemplate: browserMocks.menuBuildFromTemplateMock }, + screen: { getCursorScreenPoint: browserMocks.screenGetCursorScreenPointMock }, + webContents: { fromId: browserMocks.webContentsFromIdMock } +})) + +vi.mock('./popup-origin-bar-window', () => ({ + openPopupWithOriginBar: browserMocks.openPopupWithOriginBarMock +})) + +import { browserManager } from './browser-manager' +import { + createDownloadItem, + getDownloadItemEventHandler, + rendererWebContentsId, + resetBrowserManagerMocks, + resetBrowserManagerState +} from './browser-manager-test-harness' + +const { + guestOffMock, + guestOnMock, + guestSetBackgroundThrottlingMock, + guestSetWindowOpenHandlerMock, + guestOpenDevToolsMock, + webContentsFromIdMock, + shellOpenExternalMock +} = browserMocks + +describe('browserManager popup child policies', () => { + beforeEach(() => { + resetBrowserManagerMocks(browserMocks) + resetBrowserManagerState() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('attaches guest policies to created popup child windows', () => { + const rendererSendMock = vi.fn() + const childSetBackgroundThrottlingMock = vi.fn() + const childSetWindowOpenHandlerMock = vi.fn() + const childOnMock = vi.fn() + const childOffMock = vi.fn() + const childOpenDevToolsMock = vi.fn() + const childGuest = { + id: 4040, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: childSetBackgroundThrottlingMock, + setWindowOpenHandler: childSetWindowOpenHandlerMock, + on: childOnMock, + off: childOffMock, + openDevTools: childOpenDevToolsMock + } + const guest = { + id: 404, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: guestOnMock, + off: guestOffMock, + openDevTools: guestOpenDevToolsMock + } + webContentsFromIdMock.mockImplementation((id: number) => { + if (id === guest.id) { + return guest + } + if (id === rendererWebContentsId) { + return { isDestroyed: vi.fn(() => false), send: rendererSendMock } + } + return null + }) + + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'browser-1', + webContentsId: guest.id, + rendererWebContentsId + }) + + const didCreateWindowHandler = guestOnMock.mock.calls.find( + ([event]) => event === 'did-create-window' + )?.[1] as ((window: { webContents: typeof childGuest }) => void) | undefined + expect(didCreateWindowHandler).toBeTypeOf('function') + + didCreateWindowHandler?.({ webContents: childGuest }) + + expect(childSetBackgroundThrottlingMock).toHaveBeenCalledWith(false) + expect(childSetWindowOpenHandlerMock).toHaveBeenCalledTimes(1) + expect(childOnMock.mock.calls.filter(([event]) => event === 'did-create-window')).toHaveLength( + 1 + ) + expect(childOnMock.mock.calls.filter(([event]) => event === 'will-navigate')).toHaveLength(1) + expect(childOnMock.mock.calls.filter(([event]) => event === 'will-redirect')).toHaveLength(1) + + const childWindowOpenHandler = childSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { + url: string + }) => { action: 'allow' | 'deny' } + expect(childWindowOpenHandler({ url: 'https://identity.example.com/login' })).toMatchObject({ + action: 'allow' + }) + expect(childWindowOpenHandler({ url: 'file:///etc/passwd' })).toEqual({ action: 'deny' }) + expect(rendererSendMock).toHaveBeenCalledWith('browser:popup', { + browserPageId: 'browser-1', + origin: 'null', + action: 'blocked' + }) + browserManager.notifyPermissionDenied({ + guestWebContentsId: childGuest.id, + permission: 'notifications', + rawUrl: 'https://identity.example.com/login' + }) + expect(rendererSendMock).toHaveBeenCalledWith('browser:permission-denied', { + browserPageId: 'browser-1', + permission: 'notifications', + origin: 'https://identity.example.com' + }) + + const childDidFailLoadHandler = childOnMock.mock.calls.find( + ([event]) => event === 'did-fail-load' + )?.[1] as + | (( + event: Electron.Event, + errorCode: number, + errorDescription: string, + validatedURL: string, + isMainFrame: boolean + ) => void) + | undefined + childDidFailLoadHandler?.( + {} as Electron.Event, + -105, + 'Name not resolved', + 'https://identity.example.com/unavailable', + true + ) + expect(rendererSendMock).not.toHaveBeenCalledWith( + 'browser:guest-load-failed', + expect.anything() + ) + + const childDownloadItem = createDownloadItem() + browserManager.handleGuestWillDownload({ + guestWebContentsId: childGuest.id, + item: childDownloadItem + }) + expect(rendererSendMock).toHaveBeenCalledWith( + 'browser:download-requested', + expect.objectContaining({ browserPageId: 'browser-1' }) + ) + const childDownloadDoneHandler = getDownloadItemEventHandler(childDownloadItem, 'once', 'done') + childDownloadDoneHandler?.({} as Electron.Event, 'completed') + + const managerState = browserManager as unknown as { + popupOwnerContextByGuestId: Map + } + expect(managerState.popupOwnerContextByGuestId.has(childGuest.id)).toBe(true) + + const cleanupChildOnMock = vi.fn() + const cleanupChildGuest = { + ...childGuest, + id: 4041, + on: cleanupChildOnMock, + off: vi.fn(), + setBackgroundThrottling: vi.fn(), + setWindowOpenHandler: vi.fn() + } + const childDidCreateWindowHandler = childOnMock.mock.calls.find( + ([event]) => event === 'did-create-window' + )?.[1] as ((window: { webContents: typeof cleanupChildGuest }) => void) | undefined + childDidCreateWindowHandler?.({ webContents: cleanupChildGuest }) + expect(managerState.popupOwnerContextByGuestId.has(cleanupChildGuest.id)).toBe(true) + const cleanupChildWindowOpenHandler = cleanupChildGuest.setWindowOpenHandler.mock + .calls[0][0] as (details: { url: string }) => { action: 'allow' | 'deny' } + expect( + cleanupChildWindowOpenHandler({ url: 'https://identity.example.com/continue' }) + ).toMatchObject({ action: 'allow' }) + const cleanupChildDestroyedHandler = cleanupChildOnMock.mock.calls.find( + ([event]) => event === 'destroyed' + )?.[1] as (() => void) | undefined + cleanupChildDestroyedHandler?.() + expect(managerState.popupOwnerContextByGuestId.has(cleanupChildGuest.id)).toBe(false) + + const replacementGuest = { + ...guest, + id: 405, + on: vi.fn(), + off: vi.fn(), + setBackgroundThrottling: vi.fn(), + setWindowOpenHandler: vi.fn() + } + webContentsFromIdMock.mockImplementation((id: number) => { + if (id === guest.id) { + return guest + } + if (id === replacementGuest.id) { + return replacementGuest + } + if (id === rendererWebContentsId) { + return { isDestroyed: vi.fn(() => false), send: rendererSendMock } + } + return null + }) + browserManager.attachGuestPolicies(replacementGuest as never) + browserManager.registerGuest({ + browserPageId: 'browser-1', + webContentsId: replacementGuest.id, + rendererWebContentsId + }) + + expect(childWindowOpenHandler({ url: 'https://identity.example.com/next' })).toEqual({ + action: 'deny' + }) + expect(shellOpenExternalMock).toHaveBeenCalledWith('https://identity.example.com/next') + expect(managerState.popupOwnerContextByGuestId.has(childGuest.id)).toBe(false) + + const childDestroyedHandler = childOnMock.mock.calls.find( + ([event]) => event === 'destroyed' + )?.[1] as (() => void) | undefined + childDestroyedHandler?.() + expect(managerState.popupOwnerContextByGuestId.has(childGuest.id)).toBe(false) + + browserManager.unregisterAll() + + expect(childOffMock).toHaveBeenCalledWith('did-create-window', expect.any(Function)) + expect(childOffMock).toHaveBeenCalledWith('will-navigate', expect.any(Function)) + expect(childOffMock).toHaveBeenCalledWith('will-redirect', expect.any(Function)) + }) +}) diff --git a/src/main/browser/browser-manager-popup-routing.test.ts b/src/main/browser/browser-manager-popup-routing.test.ts index 89b4d2fe2a8..a3083005016 100644 --- a/src/main/browser/browser-manager-popup-routing.test.ts +++ b/src/main/browser/browser-manager-popup-routing.test.ts @@ -40,9 +40,8 @@ vi.mock('./popup-origin-bar-window', () => ({ })) import { browserManager } from './browser-manager' +import { MAX_PAGE_INITIATED_TABS_PER_WINDOW } from './browser-page-initiated-tab-budget' import { - createDownloadItem, - getDownloadItemEventHandler, rendererWebContentsId, resetBrowserManagerMocks, resetBrowserManagerState @@ -169,11 +168,12 @@ describe('browserManager', () => { expect(rendererSendMock).not.toHaveBeenCalled() }) - it('keeps featureless window.open popups in-app for every disposition', () => { + it('keeps opener-dependent window.open popups in-app for every disposition', () => { // Regression guard for the reverted #8332: gating the allow on // disposition === 'new-window' silently broke featureless window.open() - // OAuth flows (disposition 'foreground-tab'), whose returned handle must - // stay live. Disposition is a UX hint, not a trust signal. + // OAuth flows, whose returned handle must stay live. A named target, a + // features string, and a blank URL each mark such a flow, so all three keep + // a real child window no matter which disposition Chromium reports. const guest = { id: 140, isDestroyed: vi.fn(() => false), @@ -199,14 +199,186 @@ describe('browserManager', () => { features: string disposition: string }) => { action: 'allow' | 'deny' } + const openerDependentOpens = [ + { url: 'https://sso.example.com/auth', frameName: 'ssoWindow', features: '' }, + { url: 'https://sso.example.com/auth', frameName: '', features: 'width=500,height=600' }, + { url: 'about:blank', frameName: '', features: '' } + ] for (const disposition of ['foreground-tab', 'background-tab', 'new-window']) { - expect( - handler({ url: 'https://sso.example.com/auth', frameName: '', features: '', disposition }) - ).toMatchObject({ action: 'allow' }) + for (const open of openerDependentOpens) { + expect(handler({ ...open, disposition })).toMatchObject({ action: 'allow' }) + } } expect(shellOpenExternalMock).not.toHaveBeenCalled() }) + it('routes unnamed featureless window.open safely, including after renderer destruction', () => { + const rendererSendMock = vi.fn() + let rendererDestroyed = false + const guest = { + id: 142, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: guestOnMock, + off: guestOffMock, + openDevTools: guestOpenDevToolsMock + } + webContentsFromIdMock.mockImplementation((id: number) => { + if (id === guest.id) { + return guest + } + if (id === rendererWebContentsId) { + return { isDestroyed: vi.fn(() => rendererDestroyed), send: rendererSendMock } + } + return null + }) + + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'browser-1', + webContentsId: guest.id, + rendererWebContentsId + }) + + const handler = guestSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { + url: string + frameName: string + features: string + disposition: string + }) => { action: 'allow' | 'deny' } + for (const disposition of ['foreground-tab', 'background-tab']) { + expect( + handler({ + url: 'https://docs.example.com/guide', + frameName: '', + features: '', + disposition + }) + ).toEqual({ action: 'deny' }) + } + + expect(rendererSendMock).toHaveBeenCalledWith('browser:open-link-in-orca-tab', { + browserPageId: 'browser-1', + url: 'https://docs.example.com/guide' + }) + expect(rendererSendMock).toHaveBeenCalledWith('browser:popup', { + browserPageId: 'browser-1', + origin: 'https://docs.example.com', + action: 'opened-in-orca' + }) + expect(openPopupWithOriginBarMock).not.toHaveBeenCalled() + expect(shellOpenExternalMock).not.toHaveBeenCalled() + rendererDestroyed = true + expect( + handler({ + url: 'https://docs.example.com/guide', + frameName: '', + features: '', + disposition: 'foreground-tab' + }) + ).toEqual({ action: 'deny' }) + expect(openPopupWithOriginBarMock).not.toHaveBeenCalled() + expect(shellOpenExternalMock).not.toHaveBeenCalled() + }) + + it('shares the page-initiated tab budget across the whole opener popup tree', () => { + const rendererSendMock = vi.fn() + const guest = { + id: 150, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: guestOnMock, + once: vi.fn(), + off: guestOffMock, + openDevTools: guestOpenDevToolsMock + } + const guestsById = new Map([[guest.id, guest]]) + webContentsFromIdMock.mockImplementation((id: number) => { + if (id === rendererWebContentsId) { + return { isDestroyed: vi.fn(() => false), send: rendererSendMock } + } + return guestsById.get(id) ?? null + }) + + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'browser-1', + webContentsId: guest.id, + rendererWebContentsId + }) + + type WindowOpenHandler = (details: { + url: string + frameName: string + features: string + disposition: string + }) => { + action: 'allow' | 'deny' + createWindow?: (options: Record) => unknown + } + const handler = guestSetWindowOpenHandlerMock.mock.calls[0][0] as WindowOpenHandler + + // A named child popup keeps a real child window; each one used to start with a fresh budget. + const openNamedChild = (index: number): WindowOpenHandler => { + const childSetWindowOpenHandlerMock = vi.fn() + const child = { + id: 1500 + index, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: vi.fn(), + setWindowOpenHandler: childSetWindowOpenHandlerMock, + on: vi.fn(), + once: vi.fn(), + off: vi.fn(), + openDevTools: vi.fn() + } + guestsById.set(child.id, child) + openPopupWithOriginBarMock.mockReturnValueOnce({ + contentWebContents: child, + close: vi.fn(), + onClosed: vi.fn() + }) + const result = handler({ + url: `https://sso.example.com/child-${index}`, + frameName: `child-${index}`, + features: '', + disposition: 'new-window' + }) + expect(result).toMatchObject({ action: 'allow' }) + result.createWindow?.({}) + return childSetWindowOpenHandlerMock.mock.calls[0][0] as WindowOpenHandler + } + + const childHandlers = [openNamedChild(0), openNamedChild(1), openNamedChild(2)] + for (const [childIndex, childHandler] of childHandlers.entries()) { + for (let openIndex = 0; openIndex < MAX_PAGE_INITIATED_TABS_PER_WINDOW; openIndex++) { + expect( + childHandler({ + url: `https://docs.example.com/${childIndex}-${openIndex}`, + frameName: '', + features: '', + disposition: 'foreground-tab' + }) + ).toEqual({ action: 'deny' }) + } + } + + const routedTabs = rendererSendMock.mock.calls.filter( + ([channel]) => channel === 'browser:open-link-in-orca-tab' + ) + expect(routedTabs).toHaveLength(MAX_PAGE_INITIATED_TABS_PER_WINDOW) + expect(rendererSendMock).toHaveBeenCalledWith('browser:popup', { + browserPageId: 'browser-1', + origin: 'https://docs.example.com', + action: 'blocked' + }) + expect(openPopupWithOriginBarMock).toHaveBeenCalledTimes(childHandlers.length) + }) + it('keeps plain links current and routes explicit new-tab gestures to Orca tabs', async () => { const rendererSendMock = vi.fn() const executeJavaScriptInIsolatedWorldMock = vi.fn().mockResolvedValue(undefined) @@ -589,197 +761,4 @@ describe('browserManager', () => { }) ) }) - - it('attaches guest policies to created popup child windows', () => { - const rendererSendMock = vi.fn() - const childSetBackgroundThrottlingMock = vi.fn() - const childSetWindowOpenHandlerMock = vi.fn() - const childOnMock = vi.fn() - const childOffMock = vi.fn() - const childOpenDevToolsMock = vi.fn() - const childGuest = { - id: 4040, - isDestroyed: vi.fn(() => false), - getType: vi.fn(() => 'webview'), - setBackgroundThrottling: childSetBackgroundThrottlingMock, - setWindowOpenHandler: childSetWindowOpenHandlerMock, - on: childOnMock, - off: childOffMock, - openDevTools: childOpenDevToolsMock - } - const guest = { - id: 404, - isDestroyed: vi.fn(() => false), - getType: vi.fn(() => 'webview'), - setBackgroundThrottling: guestSetBackgroundThrottlingMock, - setWindowOpenHandler: guestSetWindowOpenHandlerMock, - on: guestOnMock, - off: guestOffMock, - openDevTools: guestOpenDevToolsMock - } - webContentsFromIdMock.mockImplementation((id: number) => { - if (id === guest.id) { - return guest - } - if (id === rendererWebContentsId) { - return { isDestroyed: vi.fn(() => false), send: rendererSendMock } - } - return null - }) - - browserManager.attachGuestPolicies(guest as never) - browserManager.registerGuest({ - browserPageId: 'browser-1', - webContentsId: guest.id, - rendererWebContentsId - }) - - const didCreateWindowHandler = guestOnMock.mock.calls.find( - ([event]) => event === 'did-create-window' - )?.[1] as ((window: { webContents: typeof childGuest }) => void) | undefined - expect(didCreateWindowHandler).toBeTypeOf('function') - - didCreateWindowHandler?.({ webContents: childGuest }) - - expect(childSetBackgroundThrottlingMock).toHaveBeenCalledWith(false) - expect(childSetWindowOpenHandlerMock).toHaveBeenCalledTimes(1) - expect(childOnMock.mock.calls.filter(([event]) => event === 'did-create-window')).toHaveLength( - 1 - ) - expect(childOnMock.mock.calls.filter(([event]) => event === 'will-navigate')).toHaveLength(1) - expect(childOnMock.mock.calls.filter(([event]) => event === 'will-redirect')).toHaveLength(1) - - const childWindowOpenHandler = childSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { - url: string - }) => { action: 'allow' | 'deny' } - expect(childWindowOpenHandler({ url: 'https://identity.example.com/login' })).toMatchObject({ - action: 'allow' - }) - expect(childWindowOpenHandler({ url: 'file:///etc/passwd' })).toEqual({ action: 'deny' }) - expect(rendererSendMock).toHaveBeenCalledWith('browser:popup', { - browserPageId: 'browser-1', - origin: 'null', - action: 'blocked' - }) - browserManager.notifyPermissionDenied({ - guestWebContentsId: childGuest.id, - permission: 'notifications', - rawUrl: 'https://identity.example.com/login' - }) - expect(rendererSendMock).toHaveBeenCalledWith('browser:permission-denied', { - browserPageId: 'browser-1', - permission: 'notifications', - origin: 'https://identity.example.com' - }) - - const childDidFailLoadHandler = childOnMock.mock.calls.find( - ([event]) => event === 'did-fail-load' - )?.[1] as - | (( - event: Electron.Event, - errorCode: number, - errorDescription: string, - validatedURL: string, - isMainFrame: boolean - ) => void) - | undefined - childDidFailLoadHandler?.( - {} as Electron.Event, - -105, - 'Name not resolved', - 'https://identity.example.com/unavailable', - true - ) - expect(rendererSendMock).not.toHaveBeenCalledWith( - 'browser:guest-load-failed', - expect.anything() - ) - - const childDownloadItem = createDownloadItem() - browserManager.handleGuestWillDownload({ - guestWebContentsId: childGuest.id, - item: childDownloadItem - }) - expect(rendererSendMock).toHaveBeenCalledWith( - 'browser:download-requested', - expect.objectContaining({ browserPageId: 'browser-1' }) - ) - const childDownloadDoneHandler = getDownloadItemEventHandler(childDownloadItem, 'once', 'done') - childDownloadDoneHandler?.({} as Electron.Event, 'completed') - - const managerState = browserManager as unknown as { - popupOwnerContextByGuestId: Map - } - expect(managerState.popupOwnerContextByGuestId.has(childGuest.id)).toBe(true) - - const cleanupChildOnMock = vi.fn() - const cleanupChildGuest = { - ...childGuest, - id: 4041, - on: cleanupChildOnMock, - off: vi.fn(), - setBackgroundThrottling: vi.fn(), - setWindowOpenHandler: vi.fn() - } - const childDidCreateWindowHandler = childOnMock.mock.calls.find( - ([event]) => event === 'did-create-window' - )?.[1] as ((window: { webContents: typeof cleanupChildGuest }) => void) | undefined - childDidCreateWindowHandler?.({ webContents: cleanupChildGuest }) - expect(managerState.popupOwnerContextByGuestId.has(cleanupChildGuest.id)).toBe(true) - const cleanupChildWindowOpenHandler = cleanupChildGuest.setWindowOpenHandler.mock - .calls[0][0] as (details: { url: string }) => { action: 'allow' | 'deny' } - expect( - cleanupChildWindowOpenHandler({ url: 'https://identity.example.com/continue' }) - ).toMatchObject({ action: 'allow' }) - const cleanupChildDestroyedHandler = cleanupChildOnMock.mock.calls.find( - ([event]) => event === 'destroyed' - )?.[1] as (() => void) | undefined - cleanupChildDestroyedHandler?.() - expect(managerState.popupOwnerContextByGuestId.has(cleanupChildGuest.id)).toBe(false) - - const replacementGuest = { - ...guest, - id: 405, - on: vi.fn(), - off: vi.fn(), - setBackgroundThrottling: vi.fn(), - setWindowOpenHandler: vi.fn() - } - webContentsFromIdMock.mockImplementation((id: number) => { - if (id === guest.id) { - return guest - } - if (id === replacementGuest.id) { - return replacementGuest - } - if (id === rendererWebContentsId) { - return { isDestroyed: vi.fn(() => false), send: rendererSendMock } - } - return null - }) - browserManager.attachGuestPolicies(replacementGuest as never) - browserManager.registerGuest({ - browserPageId: 'browser-1', - webContentsId: replacementGuest.id, - rendererWebContentsId - }) - - expect(childWindowOpenHandler({ url: 'https://identity.example.com/next' })).toEqual({ - action: 'deny' - }) - expect(shellOpenExternalMock).toHaveBeenCalledWith('https://identity.example.com/next') - expect(managerState.popupOwnerContextByGuestId.has(childGuest.id)).toBe(false) - - const childDestroyedHandler = childOnMock.mock.calls.find( - ([event]) => event === 'destroyed' - )?.[1] as (() => void) | undefined - childDestroyedHandler?.() - expect(managerState.popupOwnerContextByGuestId.has(childGuest.id)).toBe(false) - - browserManager.unregisterAll() - - expect(childOffMock).toHaveBeenCalledWith('did-create-window', expect.any(Function)) - expect(childOffMock).toHaveBeenCalledWith('will-navigate', expect.any(Function)) - expect(childOffMock).toHaveBeenCalledWith('will-redirect', expect.any(Function)) - }) }) diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 1e04cbdf0cc..7ac64b485b9 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -43,6 +43,11 @@ import { buildBrowserClickedLinkRoutingScript, buildBrowserIframeClickedLinkRoutingScript } from './browser-clicked-link-routing' +import { + createPageInitiatedTabBudget, + type PageInitiatedTabBudget +} from './browser-page-initiated-tab-budget' +import { isNewBrowserTabPopupIntent } from './browser-popup-new-tab-intent' import { cleanElectronUserAgent } from './browser-session-ua' import { getBrowserSessionUserAgentMode } from './browser-session-user-agent-mode' import { googleAuthUserAgent, isGoogleAuthUrl } from './browser-google-auth-ua' @@ -233,6 +238,8 @@ export class BrowserManager { // Why: reverse map gives O(1) guest→tab lookups on every mouse/load/permission/popup event. private readonly tabIdByWebContentsId = new Map() private readonly popupOwnerContextByGuestId = new Map() + // Why: keyed by the opener tree's root so named child popups can't each mint a fresh tab quota. + private readonly pageInitiatedTabBudgetByRootGuestId = new Map() // Why: guests are keyed by page id but renderer visibility by workspace id; bridge the mismatch to activate the right tab before capture. private readonly workspaceIdByPageId = new Map() private readonly sessionProfileIdByPageId = new Map() @@ -394,6 +401,16 @@ export class BrowserManager { return null } + /** Shared across the whole opener tree, so a chain of popups draws from one budget. */ + private tryConsumePageInitiatedTab(rootGuestWebContentsId: number): boolean { + let budget = this.pageInitiatedTabBudgetByRootGuestId.get(rootGuestWebContentsId) + if (!budget) { + budget = createPageInitiatedTabBudget() + this.pageInitiatedTabBudgetByRootGuestId.set(rootGuestWebContentsId, budget) + } + return budget.tryConsume(Date.now()) + } + private resolveRendererForBrowserTab(browserTabId: string): Electron.WebContents | null { const rendererWebContentsId = this.rendererWebContentsIdByTabId.get(browserTabId) if (!rendererWebContentsId) { @@ -758,8 +775,9 @@ export class BrowserManager { this.attachGuestPolicies(window.webContents, this.resolvePopupOwnerContext(guest.id)) } guest.on('did-create-window', handleDidCreateWindow) - guest.setWindowOpenHandler(({ url, frameName }) => { - const browserTabId = this.resolveBrowserTabIdForGuestWebContentsId(guest.id) + guest.setWindowOpenHandler(({ url, frameName, disposition, features }) => { + const ownerContext = this.resolvePopupOwnerContext(guest.id) + const browserTabId = ownerContext?.browserTabId ?? null const browserUrl = normalizeBrowserNavigationUrl(url) const externalUrl = normalizeExternalBrowserUrl(url) const expectedClickedLinkFrameName = this.clickedLinkFrameNameByGuestId.get(guest.id) @@ -784,6 +802,33 @@ export class BrowserManager { return { action: 'deny' } } + // Why: an unnamed, featureless window.open() is Chromium's own new-tab shape, so an Orca tab is + // the honest presentation; a floating origin-bar window is not. Opener-dependent shapes are + // excluded by isNewBrowserTabPopupIntent and still get a real child window below. + if ( + ownerContext && + externalUrl && + isNewBrowserTabPopupIntent({ frameName, disposition, features }) + ) { + // Why: one activation lets a page loop window.open, and each routed tab persists into + // workspace session state, so it survives the quit that used to clear popup windows. + if (!this.tryConsumePageInitiatedTab(ownerContext.rootGuestWebContentsId)) { + this.forwardOrQueuePopupEvent(guest.id, { + origin: safeOrigin(externalUrl), + action: 'blocked' + }) + return { action: 'deny' } + } + if (this.openLinkInOrcaTab(ownerContext.browserTabId, externalUrl)) { + this.forwardOrQueuePopupEvent(guest.id, { + origin: safeOrigin(externalUrl), + action: 'opened-in-orca' + }) + } + // Why: a recognized new-tab intent must never fall through to a native popup if its renderer vanished mid-open. + return { action: 'deny' } + } + // Why: file URLs are fine for in-pane previews, but must not spawn native child windows targeting local paths. const canOpenAsChild = Boolean(externalUrl || browserUrl === ORCA_BROWSER_BLANK_URL) if (browserTabId && canOpenAsChild) { @@ -1243,6 +1288,7 @@ export class BrowserManager { this.clickedLinkFrameNameByGuestId.delete(guestWebContentsId) this.offscreenGuestIds.delete(guestWebContentsId) this.popupOwnerContextByGuestId.delete(guestWebContentsId) + this.pageInitiatedTabBudgetByRootGuestId.delete(guestWebContentsId) this.authUserAgentOverrideStateByGuestId.delete(guestWebContentsId) this.pendingNavigationByGuestId.delete(guestWebContentsId) // Why: a popup must stop inheriting authorization the moment its owner retires, before Chromium destroys the child. @@ -1444,6 +1490,7 @@ export class BrowserManager { this.clickedLinkFrameNameByGuestId.clear() this.tabIdByWebContentsId.clear() this.popupOwnerContextByGuestId.clear() + this.pageInitiatedTabBudgetByRootGuestId.clear() this.worktreeIdByTabId.clear() this.sessionProfileIdByPageId.clear() this.userAgentModeByPageId.clear() diff --git a/src/main/browser/browser-page-initiated-tab-budget.test.ts b/src/main/browser/browser-page-initiated-tab-budget.test.ts new file mode 100644 index 00000000000..c2dc7618a7f --- /dev/null +++ b/src/main/browser/browser-page-initiated-tab-budget.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest' +import { + createPageInitiatedTabBudget, + MAX_PAGE_INITIATED_TABS_PER_WINDOW, + PAGE_INITIATED_TAB_WINDOW_MS +} from './browser-page-initiated-tab-budget' + +describe('page-initiated tab budget', () => { + it('absorbs a window.open loop fired from a single activation', () => { + const budget = createPageInitiatedTabBudget() + const granted = Array.from({ length: 12 }, () => budget.tryConsume(1_000)).filter(Boolean) + + expect(granted).toHaveLength(MAX_PAGE_INITIATED_TABS_PER_WINDOW) + }) + + it('refills once the rolling window has passed, so real browsing is unaffected', () => { + const budget = createPageInitiatedTabBudget() + for (let i = 0; i < MAX_PAGE_INITIATED_TABS_PER_WINDOW; i++) { + expect(budget.tryConsume(1_000)).toBe(true) + } + expect(budget.tryConsume(1_000 + PAGE_INITIATED_TAB_WINDOW_MS - 1)).toBe(false) + expect(budget.tryConsume(1_000 + PAGE_INITIATED_TAB_WINDOW_MS)).toBe(true) + }) + + it('slides rather than resetting, so a paced flood cannot outrun the cap', () => { + const budget = createPageInitiatedTabBudget(2, 1_000) + + expect(budget.tryConsume(0)).toBe(true) + expect(budget.tryConsume(900)).toBe(true) + // The first grant has aged out by 1_000 but the second has not. + expect(budget.tryConsume(1_000)).toBe(true) + expect(budget.tryConsume(1_100)).toBe(false) + }) +}) diff --git a/src/main/browser/browser-page-initiated-tab-budget.ts b/src/main/browser/browser-page-initiated-tab-budget.ts new file mode 100644 index 00000000000..d15a5c9ba4c --- /dev/null +++ b/src/main/browser/browser-page-initiated-tab-budget.ts @@ -0,0 +1,26 @@ +/** Bounded so one hostile click cannot plant an unbounded number of tabs. */ +export const MAX_PAGE_INITIATED_TABS_PER_WINDOW = 4 +export const PAGE_INITIATED_TAB_WINDOW_MS = 2_000 + +export type PageInitiatedTabBudget = { + /** Records the grant when it returns true; call only when the tab is actually being opened. */ + tryConsume: (now: number) => boolean +} + +/** Rolling window, so it absorbs a same-tick `window.open` loop without capping real browsing. */ +export function createPageInitiatedTabBudget( + maxPerWindow = MAX_PAGE_INITIATED_TABS_PER_WINDOW, + windowMs = PAGE_INITIATED_TAB_WINDOW_MS +): PageInitiatedTabBudget { + let grants: number[] = [] + return { + tryConsume: (now) => { + grants = grants.filter((grantedAt) => now - grantedAt < windowMs) + if (grants.length >= maxPerWindow) { + return false + } + grants.push(now) + return true + } + } +} diff --git a/src/main/browser/browser-popup-new-tab-intent.ts b/src/main/browser/browser-popup-new-tab-intent.ts new file mode 100644 index 00000000000..316614b4aab --- /dev/null +++ b/src/main/browser/browser-popup-new-tab-intent.ts @@ -0,0 +1,17 @@ +/** + * Whether a `window.open()` asks for a new tab rather than a popup window. + * + * Orca answers a tab by denying, which hands the page `null`, so named and featured opens — whose + * flow may use that handle — stay popups. + */ +export function isNewBrowserTabPopupIntent(details: { + frameName: string + disposition: string + features: string +}): boolean { + return ( + details.frameName === '' && + details.features.trim() === '' && + (details.disposition === 'foreground-tab' || details.disposition === 'background-tab') + ) +} diff --git a/src/main/claude/hook-service.test.ts b/src/main/claude/hook-service.test.ts index 315ee6dd228..531c092e3eb 100644 --- a/src/main/claude/hook-service.test.ts +++ b/src/main/claude/hook-service.test.ts @@ -40,9 +40,7 @@ describe('getWindowsManagedLifecycleHook', () => { const hook = getWindowsManagedLifecycleHook(scriptPath) expect(hook.args).toBeUndefined() - expect(hook.command).toMatch( - /\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand / - ) + expect(hook.command).toMatch(/\/powershell\.exe -NoProfile -EncodedCommand /) expect(hook.command).not.toContain(scriptPath) // Why: Git Bash/MSYS mangles backslash paths and slash-prefixed switches. expect(hook.command.replace(/-EncodedCommand \S+$/, '')).not.toMatch(/\\| \/[a-zA-Z]+( |$)/) @@ -384,9 +382,7 @@ describe('ClaudeHookService.install', () => { for (const eventName of ['UserPromptSubmit', 'Stop', 'StopFailure']) { const hook = settings.hooks[eventName]?.[0]?.hooks?.[0] expect(hook?.args).toBeUndefined() - expect(hook?.command).toMatch( - /\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand / - ) + expect(hook?.command).toMatch(/\/powershell\.exe -NoProfile -EncodedCommand /) expect(hook?.command).not.toContain(scriptPath) const encoded = hook?.command.match(/-EncodedCommand (\S+)$/)?.[1] diff --git a/src/main/codex/hook-service-managed-install.test.ts b/src/main/codex/hook-service-managed-install.test.ts index 5d0021f9499..3e242dc2cf4 100644 --- a/src/main/codex/hook-service-managed-install.test.ts +++ b/src/main/codex/hook-service-managed-install.test.ts @@ -31,7 +31,7 @@ import { CodexHookService } from './hook-service' import { runExclusivelyForCodexTrustConfig } from './codex-trust-config-mutation-queue' const WINDOWS_POWERSHELL_LAUNCHER = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand \S+$/ + /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ const homes = setupCodexHookHomes(homedirMock, getPathMock) diff --git a/src/main/command-code/hook-service.test.ts b/src/main/command-code/hook-service.test.ts index f5b0f83af62..9765ef5b2f4 100644 --- a/src/main/command-code/hook-service.test.ts +++ b/src/main/command-code/hook-service.test.ts @@ -21,7 +21,7 @@ vi.mock('os', async () => { import { CommandCodeHookService } from './hook-service' const WINDOWS_POWERSHELL_LAUNCHER = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand \S+$/ + /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ describe('CommandCodeHookService', () => { let homeDir: string diff --git a/src/main/cursor/hook-service.test.ts b/src/main/cursor/hook-service.test.ts index 36f6aa94b48..c6aef9aefc5 100644 --- a/src/main/cursor/hook-service.test.ts +++ b/src/main/cursor/hook-service.test.ts @@ -23,7 +23,7 @@ import { CURSOR_EVENTS, type CursorEvent } from './hook-events' const CURSOR_SCRIPT_FILE_NAME = process.platform === 'win32' ? 'cursor-hook.cmd' : 'cursor-hook.sh' const WINDOWS_POWERSHELL_LAUNCHER = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand \S+$/ + /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ type InstalledCursorHooks = { hooks: Record diff --git a/src/main/daemon/daemon-endpoint-errors.ts b/src/main/daemon/daemon-endpoint-errors.ts index 663b11ff3fa..b792f0e7044 100644 --- a/src/main/daemon/daemon-endpoint-errors.ts +++ b/src/main/daemon/daemon-endpoint-errors.ts @@ -1,4 +1,5 @@ import { DAEMON_ENDPOINT_LOST_MESSAGE } from './daemon-endpoint-ownership' +import { DaemonConnectionLostError } from './daemon-errors' import { DAEMON_UNAVAILABLE_RECONNECT_MESSAGE } from './types' /** @@ -17,6 +18,9 @@ export function isDaemonGoneError(err: unknown): boolean { if (!(err instanceof Error)) { return false } + if (err instanceof DaemonConnectionLostError) { + return true + } const errno = err as NodeJS.ErrnoException if ((errno.code === 'ENOENT' || errno.code === 'ECONNREFUSED') && errno.syscall === 'connect') { return true diff --git a/src/main/daemon/daemon-pty-adapter-concurrent-recovery.test.ts b/src/main/daemon/daemon-pty-adapter-concurrent-recovery.test.ts new file mode 100644 index 00000000000..12805c0b3ee --- /dev/null +++ b/src/main/daemon/daemon-pty-adapter-concurrent-recovery.test.ts @@ -0,0 +1,78 @@ +import { expect, it, vi } from 'vitest' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { DaemonPtyAdapter } from './daemon-pty-adapter' +import { DaemonServer } from './daemon-server' +import { createMockSubprocess } from './daemon-pty-adapter-test-harness' +import { getDaemonSocketPath } from './daemon-spawner' + +it('recovers concurrent spawns rejected by an adapter-initiated disconnect', async () => { + const concurrentSpawnCount = 70 + const dir = mkdtempSync(join(tmpdir(), 'daemon-concurrent-recovery-test-')) + const socketPath = getDaemonSocketPath(dir) + const tokenPath = join(dir, 'test.token') + const allPreparationsEntered = Promise.withResolvers() + const releaseHeldPreparations = Promise.withResolvers() + const spawnedSessionIds: string[] = [] + let preparationCount = 0 + + const spawnSubprocess: ConstructorParameters[0]['spawnSubprocess'] = ( + options + ) => { + spawnedSessionIds.push(options.sessionId) + return createMockSubprocess() + } + let server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess, + // Hold siblings until one retriable reply makes the adapter disconnect their shared socket. + preparePtySpawn: async () => { + const preparationNumber = ++preparationCount + if (preparationCount === concurrentSpawnCount) { + allPreparationsEntered.resolve() + } + await allPreparationsEntered.promise + if (preparationNumber === 1) { + throw new Error('Connection lost') + } + await releaseHeldPreparations.promise + } + }) + await server.start() + + const respawn = vi.fn(async () => { + await server.shutdown() + releaseHeldPreparations.resolve() + server = new DaemonServer({ socketPath, tokenPath, spawnSubprocess }) + await server.start() + }) + const adapter = new DaemonPtyAdapter({ socketPath, tokenPath, respawn }) + + try { + const sessionIds = Array.from( + { length: concurrentSpawnCount }, + (_, index) => `high-volume-${index}` + ) + const results = await Promise.all( + sessionIds.map((sessionId) => adapter.spawn({ sessionId, cols: 80, rows: 24 })) + ) + + expect(preparationCount).toBe(concurrentSpawnCount) + expect(respawn).toHaveBeenCalledTimes(1) + expect(respawn).toHaveBeenCalledWith('daemon_died') + expect(new Set(results.map(({ id }) => id))).toEqual(new Set(sessionIds)) + expect(new Set(spawnedSessionIds)).toEqual(new Set(sessionIds)) + + const later = await adapter.spawn({ sessionId: 'after-recovery', cols: 80, rows: 24 }) + expect(later.id).toBe('after-recovery') + expect(respawn).toHaveBeenCalledTimes(1) + expect(spawnedSessionIds).toHaveLength(concurrentSpawnCount + 1) + } finally { + adapter.dispose() + releaseHeldPreparations.resolve() + await server.shutdown() + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/src/main/droid/hook-service.test.ts b/src/main/droid/hook-service.test.ts index bd145fdc1de..7486a12b71b 100644 --- a/src/main/droid/hook-service.test.ts +++ b/src/main/droid/hook-service.test.ts @@ -25,7 +25,7 @@ vi.mock('os', async () => { import { DroidHookService } from './hook-service' const WINDOWS_POWERSHELL_LAUNCHER = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand \S+$/ + /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ describe('DroidHookService', () => { let homeDir: string diff --git a/src/main/gemini/hook-service.test.ts b/src/main/gemini/hook-service.test.ts index a835a6278f8..551c40bb0b1 100644 --- a/src/main/gemini/hook-service.test.ts +++ b/src/main/gemini/hook-service.test.ts @@ -26,7 +26,7 @@ vi.mock('os', async (importOriginal) => { import { GeminiHookService } from './hook-service' const WINDOWS_POWERSHELL_LAUNCHER = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand \S+$/ + /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ describe('GeminiHookService', () => { let homeDir: string diff --git a/src/main/grok/hook-service.test.ts b/src/main/grok/hook-service.test.ts index 4b1625614ac..251ae64fa33 100644 --- a/src/main/grok/hook-service.test.ts +++ b/src/main/grok/hook-service.test.ts @@ -33,7 +33,7 @@ import { POSIX_HOOK_STDIN_READER } from '../agent-hooks/hook-stdin-contract' const GROK_SCRIPT_FILE_NAME = process.platform === 'win32' ? 'grok-hook.cmd' : 'grok-hook.sh' const WINDOWS_POWERSHELL_LAUNCHER = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -WindowStyle Hidden -EncodedCommand \S+$/ + /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ // Why (#14828): Windows registers the bare script path when it is cmd-safe and only falls back // to the encoded launcher for a profile path that is not (#6078). windows-hook-launcher-chain diff --git a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts index 41c108a5cbc..5568030c17a 100644 --- a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts +++ b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts @@ -393,6 +393,81 @@ describe('renderer breadcrumb IPC routing', () => { ]) }) + // Why: a #185 crash loop emits a cascade notice per cascade, back to back, + // and uncoalesced they would flush the 30-entry ring holding the trail. + it('coalesces react commit cascade notices from one driver into one slot', () => { + emitRendererBreadcrumb({ + name: 'react_commit_cascade', + data: { commits: 40, rendererSurface: 'main', driverFrame: 'tabs.ts:12:3 bump' } + }) + emitRendererBreadcrumb({ + name: 'react_commit_cascade', + data: { commits: 41, rendererSurface: 'main', driverFrame: 'tabs.ts:12:3 bump' } + }) + + expect(recordCrashBreadcrumbMock).not.toHaveBeenCalled() + expect( + recordCoalescedCrashBreadcrumbMock.mock.calls.map( + (call) => (call[0] as { coalesceKey: string }).coalesceKey + ) + ).toEqual([ + 'react_commit_cascade:main:tabs.ts:12:3 bump', + 'react_commit_cascade:main:tabs.ts:12:3 bump' + ]) + }) + + // Why keyed and not name-only: coalescing keeps only the newest payload, so + // one surface or driver would erase the other's evidence. + it('keeps different cascade surfaces and drivers in separate slots', () => { + emitRendererBreadcrumb({ + name: 'react_commit_cascade', + data: { commits: 40, rendererSurface: 'main', driverFrame: 'tabs.ts:12:3 bump' } + }) + emitRendererBreadcrumb({ + name: 'react_commit_cascade', + data: { commits: 40, rendererSurface: 'dashboard-popout', driverFrame: 'tabs.ts:12:3 bump' } + }) + emitRendererBreadcrumb({ + name: 'react_commit_cascade', + data: { commits: 40, rendererSurface: 'main', storeWrites: 0 } + }) + + expect( + new Set( + recordCoalescedCrashBreadcrumbMock.mock.calls.map( + (call) => (call[0] as { coalesceKey: string }).coalesceKey + ) + ).size + ).toBe(3) + }) + + // Why asserted here: the key ends in `stack`, which is what buys the driver + // frames the 4000-char budget instead of the 240-char one details get. + it('keeps the full driver stack on a react commit cascade notice', () => { + const driverStack = `tabs.ts:12:3 bump\n${'tabs.ts:99:1 other\n'.repeat(60)}` + emitRendererBreadcrumb({ + name: 'react_commit_cascade', + data: { commits: 40, rendererSurface: 'main', driverFrame: 'tabs.ts:12:3 bump', driverStack } + }) + + const recorded = recordCoalescedCrashBreadcrumbMock.mock.calls[0]?.[0] as + | { data: { driverStack: string } } + | undefined + expect(recorded?.data.driverStack.length).toBeGreaterThan(240) + }) + + // Why uncoalesced: it fires at most once per renderer, and it is the signal + // that the diagnostic itself never wired up. + it('records the uninstalled self-check notice directly', () => { + emitRendererBreadcrumb({ name: 'react_commit_cascade_uninstalled' }) + + expect(recordCrashBreadcrumbMock).toHaveBeenCalledWith( + 'react_commit_cascade_uninstalled', + undefined + ) + expect(recordCoalescedCrashBreadcrumbMock).not.toHaveBeenCalled() + }) + it('records non-error renderer breadcrumbs without coalescing', () => { emitRendererBreadcrumb({ name: 'renderer_bootstrap_started', data: { dev: true } }) diff --git a/src/main/ipc/crash-reporting-renderer-breadcrumbs.ts b/src/main/ipc/crash-reporting-renderer-breadcrumbs.ts index 2b2129436fb..160cb93dd87 100644 --- a/src/main/ipc/crash-reporting-renderer-breadcrumbs.ts +++ b/src/main/ipc/crash-reporting-renderer-breadcrumbs.ts @@ -48,12 +48,14 @@ function recordRendererBreadcrumbTrace( // suppressed count instead. const DUPLICATE_TAB_OWNER_BREADCRUMB = 'terminal_tab_id_owned_by_multiple_worktrees' const PARK_VERDICT_CHURN_BREADCRUMB = 'terminal_park_verdict_churn' +const REACT_COMMIT_CASCADE_BREADCRUMB = 'react_commit_cascade' const COALESCED_RENDERER_BREADCRUMB_NAMES = new Set([ 'renderer_error', 'renderer_unhandled_rejection', 'terminal_safe_fit_retry_exhausted', DUPLICATE_TAB_OWNER_BREADCRUMB, PARK_VERDICT_CHURN_BREADCRUMB, + REACT_COMMIT_CASCADE_BREADCRUMB, TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB ]) const RENDERER_BREADCRUMB_COALESCE_MS = 30_000 @@ -82,6 +84,13 @@ function rendererBreadcrumbCoalesceKey( if (name === PARK_VERDICT_CHURN_BREADCRUMB) { return `${name}:${String(data?.trigger ?? '')}` } + // Why keyed on surface and driver frame: the popout and the main window can + // cascade independently, and two different driving writes are two different + // bugs that last-write coalescing would collapse into one. A driverless crumb + // (storeWrites 0, a useState loop) keys separately for the same reason. + if (name === REACT_COMMIT_CASCADE_BREADCRUMB) { + return `${name}:${String(data?.rendererSurface ?? '')}:${String(data?.driverFrame ?? '')}` + } // Preserve distinct GPU failures and atlas-reset triggers while coalescing each storm. if (name === TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB) { const kind = String(data?.kind ?? '') diff --git a/src/main/windows/windows-process-table.test.ts b/src/main/windows/windows-process-table.test.ts index 919a80deb1c..8bbd0ed7ccb 100644 --- a/src/main/windows/windows-process-table.test.ts +++ b/src/main/windows/windows-process-table.test.ts @@ -184,7 +184,7 @@ describe('PowerShell fallback when the native binding is absent', () => { }) }) -describe('wedge cooldown', () => { +describe('sticky wedge', () => { let platform: PropertyDescriptor | undefined beforeEach(() => { @@ -204,7 +204,7 @@ describe('wedge cooldown', () => { // The vendored reader latches a global while a request is in flight and // drains its queue only when that request completes. In the wedge this // guards against it never does, so every retry would add a closure that is - // never called. One probe per cooldown bounds that. + // never called. vi.useFakeTimers() const getAllProcesses = vi.fn(() => {}) __setWindowsProcessTreeLoaderForTests(() => ({ @@ -218,15 +218,15 @@ describe('wedge cooldown', () => { await firstAssertion expect(getAllProcesses).toHaveBeenCalledTimes(1) - await expect(readWindowsProcessTableFresh()).rejects.toThrow(/cooling down/) + await expect(readWindowsProcessTableFresh()).rejects.toThrow(/wedged/) expect(getAllProcesses).toHaveBeenCalledTimes(1) }) - it('lets exactly one caller probe when the cooldown expires', async () => { - // Without claiming the recovery slot before probing, every concurrent - // caller passes the cooldown check at expiry and each enqueues a callback - // into the still-latched native queue -- so each cycle leaks another batch - // rather than bounding it to one probe. + it('never re-enters the reader while the timed-out read is still out', async () => { + // A cooldown bounds the RATE of new callbacks, not the total: one probe per + // window still retains one more closure in the still-latched native queue + // every window, for the life of the app. Only the outstanding read can + // reopen the gate, so a permanent wedge retains exactly one callback. vi.useFakeTimers() const getAllProcesses = vi.fn(() => {}) __setWindowsProcessTreeLoaderForTests(() => ({ @@ -240,16 +240,72 @@ describe('wedge cooldown', () => { await wedgeAssertion expect(getAllProcesses).toHaveBeenCalledTimes(1) - await vi.advanceTimersByTimeAsync(30_000) - const attempts = [ - readWindowsProcessTableFresh().catch(() => 'rejected'), - readWindowsProcessTableFresh().catch(() => 'rejected'), - readWindowsProcessTableFresh().catch(() => 'rejected') - ] - await vi.advanceTimersByTimeAsync(3_000) - await Promise.all(attempts) + // Four windows of the cooldown this replaced, each with concurrent callers. + // Rejection reasons are deliberately not asserted here: the leak this test + // pins is the call count, and it must fail on that alone. + for (let window = 0; window < 4; window += 1) { + await vi.advanceTimersByTimeAsync(30_000) + const attempts = [ + readWindowsProcessTableFresh().catch(() => 'rejected'), + readWindowsProcessTableFresh().catch(() => 'rejected'), + readWindowsProcessTableFresh().catch(() => 'rejected') + ] + // Long enough for a probe's own deadline, had one been let through. + await vi.advanceTimersByTimeAsync(3_000) + expect(await Promise.all(attempts)).toEqual(['rejected', 'rejected', 'rejected']) + } - // One recovery probe, not three. + // Still the one callback the wedge is holding -- not one more per window. + expect(getAllProcesses).toHaveBeenCalledTimes(1) + }) + + it('resumes as soon as the timed-out read finally calls back', async () => { + // The stuck request completing is what drains the vendored queue, so it is + // the only honest evidence the reader recovered. Waiting out a wall clock + // afterwards would strand a reader that is already answering. + vi.useFakeTimers() + let stuck: ((rows: typeof NATIVE | undefined) => void) | undefined + const getAllProcesses = vi.fn((cb: (rows: typeof NATIVE | undefined) => void) => { + if (stuck) { + cb(NATIVE) + return + } + stuck = cb + }) + __setWindowsProcessTreeLoaderForTests(() => ({ + ProcessDataFlag: { None: 0, Memory: 1, CommandLine: 2 }, + getAllProcesses + })) + + const wedge = readWindowsProcessTableFresh() + const wedgeAssertion = expect(wedge).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(3_000) + await wedgeAssertion + await expect(readWindowsProcessTableFresh()).rejects.toThrow(/wedged/) + + stuck?.(NATIVE) + await expect(readWindowsProcessTableFresh()).resolves.toHaveLength(NATIVE.length) + expect(getAllProcesses).toHaveBeenCalledTimes(2) + }) + + it('ignores a pending timeout from before a test reset', async () => { + vi.useFakeTimers() + const getAllProcesses = vi.fn((_cb: (rows: typeof NATIVE | undefined) => void) => {}) + __setWindowsProcessTreeLoaderForTests(() => ({ + ProcessDataFlag: { None: 0, Memory: 1, CommandLine: 2 }, + getAllProcesses + })) + + const staleRead = readWindowsProcessTableFresh() + const staleAssertion = expect(staleRead).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(0) + expect(getAllProcesses).toHaveBeenCalledTimes(1) + resetWindowsProcessTableForTests() + await vi.advanceTimersByTimeAsync(3_000) + await staleAssertion + + getAllProcesses.mockImplementation((cb) => cb(NATIVE)) + await expect(readWindowsProcessTableFresh()).resolves.toHaveLength(NATIVE.length) expect(getAllProcesses).toHaveBeenCalledTimes(2) }) diff --git a/src/main/windows/windows-process-table.ts b/src/main/windows/windows-process-table.ts index e03ecee6e29..35dab09d100 100644 --- a/src/main/windows/windows-process-table.ts +++ b/src/main/windows/windows-process-table.ts @@ -146,20 +146,18 @@ function loadWindowsProcessTree(): WindowsProcessTreeModule | null { const WINDOWS_PROCESS_QUERY_TIMEOUT_MS = 3_000 /** - * How long to stop calling the reader after it misses its deadline. - * - * Why a cooldown and not just the deadline: a timed-out call leaves its - * callback in the vendored module's queue, and that queue only drains when the - * latched request finally completes -- which, in the wedge this guards against, - * never happens. Retrying at the caller's poll rate would then add a closure - * per tick forever. One probe per cooldown bounds it. + * Reads that missed their deadline and have not called back yet. + * Refusing re-entry bounds both vendored callbacks and relay addon workers to + * one; read ids keep a late callback from clearing a newer wedge. */ -const WINDOWS_PROCESS_QUERY_COOLDOWN_MS = 30_000 +const unreturnedReads = new Set() +let readSequence = 0 +let nativeReaderEpoch = 0 -let wedgedUntilMs = 0 -// Why a generation: a request that already lost its deadline must not later -// clear or re-arm the wedge on behalf of the request that replaced it. -let readGeneration = 0 +function resetNativeReaderState(): void { + nativeReaderEpoch += 1 + unreturnedReads.clear() +} function readNativeRows(): Promise { const native = moduleLoader() @@ -177,19 +175,13 @@ function readNativeRows(): Promise { // tree dead. "Unavailable" has to stay distinguishable from "empty". return Promise.reject(new Error('windows process table unavailable')) } - const startedAt = Date.now() - if (startedAt < wedgedUntilMs) { - return Promise.reject(new Error('windows process table is cooling down after a timeout')) + if (unreturnedReads.size > 0) { + return Promise.reject( + new Error('windows process table is wedged: an earlier read has not returned') + ) } - if (wedgedUntilMs > 0) { - // Coming out of a wedge: re-arm the cooldown BEFORE probing, so exactly one - // caller gets through. Without this every concurrent caller passes the - // check above at expiry, each enqueues a callback into the still-latched - // native queue, and each cooldown cycle leaks another batch rather than - // bounding it to one probe. - wedgedUntilMs = startedAt + WINDOWS_PROCESS_QUERY_COOLDOWN_MS - } - const generation = ++readGeneration + const readId = ++readSequence + const readerEpoch = nativeReaderEpoch // Why always both flags: each adds an OpenProcess per process (Memory a // GetProcessMemoryInfo, CommandLine a PEB read), so asking for less would be // cheaper -- 15.9ms p50 versus 30.6ms at 1050 processes. But every read shares @@ -204,18 +196,19 @@ function readNativeRows(): Promise { let deadline: ReturnType | undefined try { deadline = setTimeout(() => { - if (generation === readGeneration) { - wedgedUntilMs = Date.now() + WINDOWS_PROCESS_QUERY_COOLDOWN_MS + // Test resets invalidate deadlines owned by the prior injected reader. + if (readerEpoch === nativeReaderEpoch) { + unreturnedReads.add(readId) } reject(new Error('windows process table timed out')) }, WINDOWS_PROCESS_QUERY_TIMEOUT_MS) deadline.unref?.() native.getAllProcesses((processes) => { clearTimeout(deadline) - // A callback proves the reader is answering, so stop refusing. - if (generation === readGeneration) { - wedgedUntilMs = 0 - } + // A callback proves this read drained, so stop refusing. Unconditional: + // dropping an id that was never added is a no-op, and only the read + // that actually wedged can be holding the gate shut. + unreturnedReads.delete(readId) if (!processes) { reject(new Error('windows process table returned no snapshot')) return @@ -303,7 +296,7 @@ export function __setWindowsProcessTreeLoaderForTests( ): void { moduleLoader = loader ?? loadWindowsProcessTree cachedModule = undefined - wedgedUntilMs = 0 + resetNativeReaderState() snapshotReader.reset() } @@ -314,7 +307,7 @@ export function __setWindowsProcessTreeRequireForTests( requireNative = resolve ?? requireFromMain moduleLoader = loadWindowsProcessTree cachedModule = undefined - wedgedUntilMs = 0 + resetNativeReaderState() snapshotReader.reset() } @@ -330,5 +323,5 @@ export function __setWindowsProcessTableCimScanForTests( export function resetWindowsProcessTableForTests(): void { snapshotReader.reset() cachedModule = undefined - wedgedUntilMs = 0 + resetNativeReaderState() } diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-state-badge.tsx b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-state-badge.tsx index cd2135b9873..0e05cb0803d 100644 --- a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-state-badge.tsx +++ b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-state-badge.tsx @@ -1,26 +1,14 @@ import React from 'react' -import { cn } from '@/lib/utils' +import { + getWorkItemStateTone, + WorkItemStateBadge as SharedWorkItemStateBadge +} from '@/components/github/work-item-state-badge' import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' -import { getStateLabel } from '@/components/github/work-item-state-presentation' +// Why: closed issues stay neutral here (may be completed/resolved); red is reserved +// for PR closed-without-merge. export function getStateTone(item: GitHubWorkItem): string { - if (item.type === 'pr') { - if (item.state === 'merged') { - return 'border-purple-500/30 bg-purple-500/10 text-purple-600 dark:text-purple-300' - } - if (item.state === 'draft') { - return 'border-slate-500/30 bg-slate-500/10 text-slate-600 dark:text-slate-300' - } - if (item.state === 'closed') { - return 'border-rose-500/30 bg-rose-500/10 text-rose-600 dark:text-rose-300' - } - return 'border-emerald-500/30 bg-emerald-500/10 text-emerald-600 dark:text-emerald-300' - } - if (item.state === 'closed') { - // Why: keep closed issues neutral (may be completed/resolved); red is reserved for PR closed-without-merge. - return 'border-ring/50 bg-primary/10 text-foreground' - } - return 'border-emerald-500/30 bg-emerald-500/10 text-emerald-600 dark:text-emerald-300' + return getWorkItemStateTone(item, 'neutral') } export function WorkItemStateBadge({ @@ -30,15 +18,5 @@ export function WorkItemStateBadge({ item: GitHubWorkItem className?: string }): React.JSX.Element { - return ( - - {getStateLabel(item)} - - ) + return } diff --git a/src/renderer/src/components/github/review-state-presentation.ts b/src/renderer/src/components/github/review-state-presentation.ts new file mode 100644 index 00000000000..ac9ef42629e --- /dev/null +++ b/src/renderer/src/components/github/review-state-presentation.ts @@ -0,0 +1,41 @@ +import { GitMerge, GitPullRequestClosed, GitPullRequestDraft, type LucideIcon } from 'lucide-react' +import type { HostedReviewState } from '../../../../shared/hosted-review' + +/** Every review surface renders state from here; `unknown` means we have a number but no fetched state. */ +export type ReviewStateForDisplay = HostedReviewState | 'unknown' | null | undefined + +/** Also the tone for an open issue, which reads the same as an open review. */ +export const OPEN_REVIEW_STATE_TONE = + 'border-emerald-500/30 bg-emerald-500/10 text-emerald-600 dark:text-emerald-300' + +/** Outline-pill tone: purple merged, rose closed, slate draft, emerald open, neutral unknown. */ +export function getReviewStateTone(state: ReviewStateForDisplay): string { + if (state === 'merged') { + return 'border-purple-500/30 bg-purple-500/10 text-purple-600 dark:text-purple-300' + } + if (state === 'draft') { + return 'border-slate-500/30 bg-slate-500/10 text-slate-600 dark:text-slate-300' + } + if (state === 'closed') { + return 'border-rose-500/30 bg-rose-500/10 text-rose-600 dark:text-rose-300' + } + if (state === 'open') { + return OPEN_REVIEW_STATE_TONE + } + return 'border-border bg-background text-muted-foreground' +} + +// Why: the glyph must carry review state — tone alone made a draft with failing +// checks render as a red PR icon, which reads as closed. +export function getReviewStateIcon(state: ReviewStateForDisplay): LucideIcon | null { + if (state === 'merged') { + return GitMerge + } + if (state === 'closed') { + return GitPullRequestClosed + } + if (state === 'draft') { + return GitPullRequestDraft + } + return null +} diff --git a/src/renderer/src/components/github/work-item-state-badge.tsx b/src/renderer/src/components/github/work-item-state-badge.tsx new file mode 100644 index 00000000000..4286a7a2c2d --- /dev/null +++ b/src/renderer/src/components/github/work-item-state-badge.tsx @@ -0,0 +1,48 @@ +import React from 'react' +import { cn } from '@/lib/utils' +import { getStateLabel } from '@/components/github/work-item-state-presentation' +import { + getReviewStateTone, + OPEN_REVIEW_STATE_TONE +} from '@/components/github/review-state-presentation' +import type { GitHubWorkItem } from '../../../../shared/github/work-item-types' + +/** + * Whether a closed ISSUE reads as failure or as neutral completion. PR state tone + * never varies — only surfaces disagree on what a closed issue means. + */ +export type ClosedIssueTone = 'destructive' | 'neutral' + +export function getWorkItemStateTone(item: GitHubWorkItem, closedIssue: ClosedIssueTone): string { + if (item.type === 'pr') { + return getReviewStateTone(item.state) + } + if (item.state === 'closed') { + return closedIssue === 'neutral' + ? 'border-ring/50 bg-primary/10 text-foreground' + : 'border-rose-500/30 bg-rose-500/10 text-rose-600 dark:text-rose-300' + } + return OPEN_REVIEW_STATE_TONE +} + +export function WorkItemStateBadge({ + item, + className, + closedIssue +}: { + item: GitHubWorkItem + className?: string + closedIssue: ClosedIssueTone +}): React.JSX.Element { + return ( + + {getStateLabel(item)} + + ) +} diff --git a/src/renderer/src/components/pull-request-page/presentation/state-badge.tsx b/src/renderer/src/components/pull-request-page/presentation/state-badge.tsx index 62d38f67806..e50b76344e4 100644 --- a/src/renderer/src/components/pull-request-page/presentation/state-badge.tsx +++ b/src/renderer/src/components/pull-request-page/presentation/state-badge.tsx @@ -1,25 +1,12 @@ import React from 'react' -import { cn } from '@/lib/utils' -import { getStateLabel } from '@/components/github/work-item-state-presentation' +import { + getWorkItemStateTone, + WorkItemStateBadge as SharedWorkItemStateBadge +} from '@/components/github/work-item-state-badge' import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' export function getStateTone(item: GitHubWorkItem): string { - if (item.type === 'pr') { - if (item.state === 'merged') { - return 'border-purple-500/30 bg-purple-500/10 text-purple-600 dark:text-purple-300' - } - if (item.state === 'draft') { - return 'border-slate-500/30 bg-slate-500/10 text-slate-600 dark:text-slate-300' - } - if (item.state === 'closed') { - return 'border-rose-500/30 bg-rose-500/10 text-rose-600 dark:text-rose-300' - } - return 'border-emerald-500/30 bg-emerald-500/10 text-emerald-600 dark:text-emerald-300' - } - if (item.state === 'closed') { - return 'border-rose-500/30 bg-rose-500/10 text-rose-600 dark:text-rose-300' - } - return 'border-emerald-500/30 bg-emerald-500/10 text-emerald-600 dark:text-emerald-300' + return getWorkItemStateTone(item, 'destructive') } /** Filled counterpart of `getStateTone` for high-emphasis surfaces like the PR page header. */ @@ -42,15 +29,5 @@ export function WorkItemStateBadge({ item: GitHubWorkItem className?: string }): React.JSX.Element { - return ( - - {getStateLabel(item)} - - ) + return } diff --git a/src/renderer/src/components/sidebar/worktree-review-helpers.tsx b/src/renderer/src/components/sidebar/worktree-review-helpers.tsx index 96e64fb4423..26a16c9f629 100644 --- a/src/renderer/src/components/sidebar/worktree-review-helpers.tsx +++ b/src/renderer/src/components/sidebar/worktree-review-helpers.tsx @@ -1,5 +1,6 @@ -import { GitMerge, GitPullRequestClosed, GitPullRequestDraft } from 'lucide-react' +import { GitMerge } from 'lucide-react' import { cn } from '@/lib/utils' +import { getReviewStateIcon } from '@/components/github/review-state-presentation' import { PullRequestIcon } from './WorktreeCardHelpers' import type { WorktreeCardPrDisplay } from './worktree-card-pr-display' @@ -23,23 +24,6 @@ export function getProviderName(review: WorktreeCardPrDisplay): string { return 'GitHub' } -// Why: the glyph must carry review state — tone alone made a draft with failing -// checks render as a red PR icon, which reads as closed. -function getStateIcon( - state: WorktreeCardPrDisplay['state'] -): React.ComponentType<{ className?: string }> | null { - if (state === 'merged') { - return GitMerge - } - if (state === 'closed') { - return GitPullRequestClosed - } - if (state === 'draft') { - return GitPullRequestDraft - } - return null -} - // Why: checks only gate a review that is actually open; draft/closed/merged keep // their state tone so the glyph agrees with its tooltip. A stateless row (folder // cards render one while a linked review is loading or its details failed) has no @@ -88,6 +72,6 @@ export function ReviewIcon({ }): React.JSX.Element { const providerIcon = variant === 'provider' && review.provider === 'gitlab' ? GitMerge : PullRequestIcon - const Icon = getStateIcon(review.state) ?? providerIcon + const Icon = getReviewStateIcon(review.state) ?? providerIcon return } diff --git a/src/renderer/src/components/terminal-pane/terminal-park-verdict-flip-telemetry.ts b/src/renderer/src/components/terminal-pane/terminal-park-verdict-flip-telemetry.ts index 17b53d53785..13642519029 100644 --- a/src/renderer/src/components/terminal-pane/terminal-park-verdict-flip-telemetry.ts +++ b/src/renderer/src/components/terminal-pane/terminal-park-verdict-flip-telemetry.ts @@ -10,13 +10,12 @@ * one tab as "damping did not reach the oscillating input". */ import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' +import { REACT_NESTED_UPDATE_LIMIT } from '../../../../shared/react-update-depth-attribution' export const TERMINAL_TAB_PARK_FLIP_WINDOW_MS = 60_000 /** Flips per window that no sane park policy should reach. Breadcrumb only. */ export const TERMINAL_TAB_PARK_FLIP_NOTICE_LIMIT = 12 -/** react-dom 19.2.x nested commit limit. */ -const REACT_NESTED_UPDATE_LIMIT = 50 /** Measured upper bound after the passive-effect pin engages. */ const PARK_PIN_SETTLE_COMMITS = 6 /** Worst-case commits from pane, watcher, and store work per verdict flip. */ diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row-data.ts b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row-data.ts index 75103a98d0c..1f9d3d2ce52 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row-data.ts +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row-data.ts @@ -1,9 +1,6 @@ import { translate } from '@/i18n/i18n' import type { WorkspaceCleanupCandidate } from '../../../../shared/workspace-cleanup' -import { - getWorkspaceCleanupGitLabel, - type WorkspaceCleanupReviewInfo -} from './workspace-cleanup-presentation' +import { getWorkspaceCleanupGitLabel } from './workspace-cleanup-presentation' import { formatUnpushedCommitCount, formatWorkspaceCleanupContextDetail, @@ -172,13 +169,6 @@ function hasGitStatusPill(candidate: WorkspaceCleanupCandidate): boolean { return (candidate.git.upstreamAhead ?? 0) > 0 || candidate.git.clean === false } -export function getReviewPillTone(reviewInfo: WorkspaceCleanupReviewInfo): StatusPillTone { - if (reviewInfo.state === 'open' || reviewInfo.state === 'draft') { - return 'review' - } - return 'neutral' -} - export function getContextCount(candidate: WorkspaceCleanupCandidate): number { return ( candidate.localContext.terminalTabCount + diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx index d9deb16e8e9..9d4ee5a6e2f 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx @@ -247,4 +247,78 @@ describe('CandidateRow', () => { expect(container?.querySelector(`[aria-label="Select ${remoteName}"]`)).not.toBeNull() expect(container?.querySelector(`[aria-label="Host: builder"]`)).not.toBeNull() }) + + it('colors the review pill by state the way the PR page and item dialog do', () => { + const states = [ + { state: 'merged' as const, label: 'PR #1', hue: 'purple' }, + { state: 'closed' as const, label: 'PR #2', hue: 'rose' }, + { state: 'open' as const, label: 'PR #3', hue: 'emerald' }, + { state: 'draft' as const, label: 'PR #4', hue: 'slate' } + ] + + for (const { state, label, hue } of states) { + const candidate = makeCandidate() + + act(() => { + root?.render( + + ) + }) + + const pill = container?.querySelector(`[aria-label*="${label}"]`) + expect(pill?.className).toContain(`text-${hue}-`) + expect(pill?.className).not.toContain('text-muted-foreground') + } + }) + + it('leaves a review with an unfetched state neutral', () => { + const candidate = makeCandidate() + + act(() => { + root?.render( + + ) + }) + + const pill = container?.querySelector('[aria-label*="PR #9"]') + expect(pill?.className).toContain('text-muted-foreground') + }) }) diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx index bec347d570d..fd4c442d55b 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx @@ -36,10 +36,13 @@ import { getCandidateFactStatuses, getContextCount, getDirtyGitLabel, - getReviewPillTone, getWorkspaceCleanupBlockerLabels, shouldShowGitMetadataChip } from './workspace-cleanup-candidate-row-data' +import { + getReviewStateIcon, + getReviewStateTone +} from '@/components/github/review-state-presentation' import { StatusPill } from './workspace-cleanup-status-pill' import { WorkspaceCleanupMetadataChip } from './workspace-cleanup-metadata-chip' import { WorkspaceCleanupForgetLocallyButton } from './workspace-cleanup-forget-locally-button' @@ -255,10 +258,10 @@ export const CandidateRow = React.memo(function CandidateRow({ ) : null} {reviewInfo.label ? ( ) : null} diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx index dbffc7214e5..379a9d60d10 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx @@ -218,4 +218,43 @@ describe('WorkspaceCleanupConfirmRemove', () => { '1 workspace currently shows risk and may need a force delete' ) }) + + it('gives the review pill a text equivalent for its state color without repeating the number', () => { + const candidate = makeFacetCandidate({ + worktreeId: 'repo-1::/repo/merged-review', + displayName: 'Merged review', + path: '/repo/merged-review' + }) + + act(() => { + root.render( + + ) + }) + + const srText = container.querySelector('span.sr-only')?.textContent ?? '' + expect(srText).toContain('Merged') + expect(srText).not.toContain('PR #15716') + }) }) diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx index c7ccae4a8fe..97a31aedc5c 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx @@ -15,11 +15,12 @@ import { getCandidateFactStatuses, formatContextDetailLabels, getDirtyGitLabel, - getReviewPillTone, shouldShowGitMetadataChip } from './workspace-cleanup-candidate-row-data' import type { WorkspaceCleanupReviewInfo } from './workspace-cleanup-presentation' import { formatWorkspaceCleanupRelativeTime } from './workspace-cleanup-relative-time' +import { getReviewStateTone } from '@/components/github/review-state-presentation' +import { getReviewStateSrText } from './workspace-cleanup-row-labels' import { StatusPill } from './workspace-cleanup-status-pill' import { WorkspaceCleanupCandidateList } from './workspace-cleanup-candidate-list' import { @@ -255,7 +256,10 @@ function ConfirmRemoveRow({ ))} {reviewInfo.label ? ( - {reviewInfo.label} + + {reviewInfo.label} + {getReviewStateSrText(reviewInfo)} + ) : null} {contextPillLabels.map((label) => ( {label} diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-metadata-chip.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-metadata-chip.tsx index 5fea9257680..bae88d8fc03 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-metadata-chip.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-metadata-chip.tsx @@ -8,12 +8,15 @@ export function WorkspaceCleanupMetadataChip({ icon: Icon, label, value, - tone = 'neutral' + tone = 'neutral', + toneClassName }: { icon: LucideIcon label: string value?: string tone?: StatusPillTone + /** Overrides `tone` for chips whose color comes from a domain state (e.g. review state). */ + toneClassName?: string }): React.JSX.Element { return ( @@ -25,7 +28,8 @@ export function WorkspaceCleanupMetadataChip({ tone === 'ready' && 'border-[color:color-mix(in_srgb,var(--git-decoration-added)_45%,transparent)] bg-[color:color-mix(in_srgb,var(--git-decoration-added)_10%,transparent)] text-[var(--git-decoration-added)]', tone === 'review' && 'bg-muted text-foreground', - tone === 'destructive' && 'border-destructive/30 text-destructive' + tone === 'destructive' && 'border-destructive/30 text-destructive', + toneClassName )} aria-label={label} > diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-row-labels.ts b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-row-labels.ts index 887cb1c17d0..b7b897f1a22 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-row-labels.ts +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-row-labels.ts @@ -1,9 +1,20 @@ import type { WorkspaceCleanupReviewInfo } from './workspace-cleanup-presentation' +import { getWorkspaceCleanupReviewStateLabel } from './workspace-cleanup-facet-labels' export function formatCompactActivityLabel(label: string): string { return label === 'Just now' ? 'now' : label.replace(/ ago$/, '') } export function getReviewTooltip(reviewInfo: WorkspaceCleanupReviewInfo): string { - return [reviewInfo.label, reviewInfo.state, reviewInfo.title].filter(Boolean).join(' · ') + return [reviewInfo.label, getReviewStateSrText(reviewInfo)].filter(Boolean).join(' · ') +} + +/** Text equivalent for the state color on pills that render the number visibly. */ +export function getReviewStateSrText(reviewInfo: WorkspaceCleanupReviewInfo): string { + if (!reviewInfo.state) { + return '' + } + return [getWorkspaceCleanupReviewStateLabel(reviewInfo.state), reviewInfo.title] + .filter(Boolean) + .join(' · ') } diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-status-pill.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-status-pill.tsx index 1e5786fda49..0a5efdd2bdc 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-status-pill.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-status-pill.tsx @@ -4,10 +4,13 @@ import type { StatusPillTone } from './workspace-cleanup-candidate-row-data' export function StatusPill({ children, - tone = 'neutral' + tone = 'neutral', + toneClassName }: { children: React.ReactNode tone?: StatusPillTone + /** Overrides `tone` for pills whose color comes from a domain state (e.g. review state). */ + toneClassName?: string }): React.JSX.Element { return ( {children} diff --git a/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts index 15a632a25e2..4f3450dae85 100644 --- a/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts @@ -89,7 +89,20 @@ export function registerBrowserStateIpcBridge( if (!sourcePage || getRuntimeEnvironmentIdForWorktree(store, sourcePage.worktreeId)) { return } - store.createBrowserTab(sourcePage.worktreeId, url, { title: url }) + // Why: the link inherits the opener's cookie jar. Falling back to the default profile would let + // a page in an isolated session hand its links to the default one, silently crossing profiles. + const sourceTab = (store.browserTabsByWorktree[sourcePage.worktreeId] ?? []).find( + (tab) => tab.id === sourcePage.workspaceId + ) + store.createBrowserTab(sourcePage.worktreeId, url, { + title: url, + ...(sourceTab + ? { + sessionProfileId: sourceTab.sessionProfileId, + sessionPartition: sourceTab.sessionPartition + } + : {}) + }) }) ) } diff --git a/src/renderer/src/hooks/ipc-events/browser-state-open-link-profile.test.ts b/src/renderer/src/hooks/ipc-events/browser-state-open-link-profile.test.ts new file mode 100644 index 00000000000..b15a5be158c --- /dev/null +++ b/src/renderer/src/hooks/ipc-events/browser-state-open-link-profile.test.ts @@ -0,0 +1,104 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { createBrowserTabMock, storeState } = vi.hoisted(() => ({ + createBrowserTabMock: vi.fn(), + storeState: { + value: {} as Record + } +})) + +vi.mock('../../store', () => ({ + useAppStore: { getState: () => storeState.value } +})) +vi.mock('@/lib/worktree-runtime-owner', () => ({ + getRuntimeEnvironmentIdForWorktree: () => null +})) +vi.mock('@/components/browser-pane/describe-page/live-browser-url-registry', () => ({ + rememberLiveBrowserUrl: vi.fn() +})) +vi.mock('./browser-automation-bootstrap-lease', () => ({ + acquireBrowserAutomationBootstrapLease: vi.fn() +})) + +import { registerBrowserStateIpcBridge } from './browser-state-ipc-bridge' + +const noopUnsubscribe = (): void => {} + +function captureOpenLinkHandler(): (event: { browserPageId: string; url: string }) => void { + let handler: ((event: { browserPageId: string; url: string }) => void) | null = null + const browserApi = new Proxy( + { + onOpenLinkInOrcaTab: (callback: (event: { browserPageId: string; url: string }) => void) => { + handler = callback + return noopUnsubscribe + } + } as Record, + { + get: (target, property) => + property in target ? target[property as string] : () => noopUnsubscribe + } + ) + const api = new Proxy({ browser: browserApi } as Record, { + get: (target, property) => + property in target + ? target[property as string] + : new Proxy({}, { get: () => () => noopUnsubscribe }) + }) + ;(globalThis as { window?: unknown }).window = { api } + + registerBrowserStateIpcBridge([], () => false) + if (!handler) { + throw new Error('Expected the bridge to subscribe to browser:open-link-in-orca-tab') + } + return handler +} + +describe('link-opened Orca tabs', () => { + beforeEach(() => { + createBrowserTabMock.mockReset() + }) + + it('inherits the opener tab session so an isolated profile cannot leak into the default one', () => { + storeState.value = { + browserPagesByWorkspace: { + 'workspace-1': [{ id: 'page-1', workspaceId: 'workspace-1', worktreeId: 'worktree-1' }] + }, + browserTabsByWorktree: { + 'worktree-1': [ + { + id: 'workspace-1', + sessionProfileId: 'profile-client-a', + sessionPartition: 'persist:orca-browser-session-client-a' + } + ] + }, + createBrowserTab: createBrowserTabMock + } + + captureOpenLinkHandler()({ browserPageId: 'page-1', url: 'https://docs.example.com/guide' }) + + expect(createBrowserTabMock).toHaveBeenCalledWith( + 'worktree-1', + 'https://docs.example.com/guide', + expect.objectContaining({ + sessionProfileId: 'profile-client-a', + sessionPartition: 'persist:orca-browser-session-client-a' + }) + ) + }) + + it('leaves the profile unset when the opener tab is gone, so the user default still applies', () => { + storeState.value = { + browserPagesByWorkspace: { + 'workspace-1': [{ id: 'page-1', workspaceId: 'missing', worktreeId: 'worktree-1' }] + }, + browserTabsByWorktree: {}, + createBrowserTab: createBrowserTabMock + } + + captureOpenLinkHandler()({ browserPageId: 'page-1', url: 'https://docs.example.com/guide' }) + + const options = createBrowserTabMock.mock.calls[0][2] as Record + expect('sessionProfileId' in options).toBe(false) + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-install-order.test.ts b/src/renderer/src/lib/react-commit-cascade-install-order.test.ts new file mode 100644 index 00000000000..2f5a1227e89 --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-install-order.test.ts @@ -0,0 +1,137 @@ +import { existsSync, readFileSync } from 'node:fs' +import { join, resolve, dirname } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * react-dom reads __REACT_DEVTOOLS_GLOBAL_HOOK__ once, at its own module + * evaluation, and enters the graph through several component modules rather + * than just the entry's import of react-renderer-root. ES evaluation is + * depth-first in source order, so only the FIRST import statement is guaranteed + * to beat it — and only if that module's own graph cannot reach react-dom + * first, which is why the shim has no imports at all. + * + * Nothing else fails when either property stops holding: the diagnostic simply + * records nothing. These tests are the failure. + */ +const RENDERER_ROOT = resolve(__dirname, '..') +const SHIM_MODULE = 'react-devtools-commit-hook-shim' +const SHIM_IMPORT = `import './lib/${SHIM_MODULE}'` +const OBSERVER_IMPORT = "import './lib/react-commit-cascade-observer'" + +/** Entries whose crash reports reach the breadcrumb pipe. */ +const INSTRUMENTED_ENTRIES = ['main.tsx', 'popout.tsx'] +/** + * The web preload stubs crashReports.recordBreadcrumb to a no-op + * (src/renderer/src/web/preload-api/web-diagnostics-api.ts), so instrumenting + * the web entry would cost commits and record nothing. + */ +const UNINSTRUMENTED_ENTRIES = [join('web', 'main.tsx')] + +function firstImportStatement(source: string): string | undefined { + return source + .split('\n') + .map((line) => line.trim()) + .find((line) => line.startsWith('import ')) +} + +function importedSpecifiers(source: string): string[] { + return Array.from(source.matchAll(/(?:^|\n)\s*(?:import|export)[^\n]*?from\s*'([^']+)'/g)) + .map((match) => match[1] as string) + .concat( + Array.from(source.matchAll(/(?:^|\n)\s*import\s*'([^']+)'/g)).map( + (match) => match[1] as string + ) + ) +} + +/** Mirrors the alias in config/vitest.config.ts and electron.vite.config.ts. */ +function resolveLocalModule(specifier: string, fromFile: string): string | undefined { + const base = specifier.startsWith('@/') + ? join(RENDERER_ROOT, specifier.slice(2)) + : specifier.startsWith('.') + ? join(dirname(fromFile), specifier) + : undefined + if (base === undefined) { + return undefined + } + // Only TS: a .css or asset import cannot pull react-dom into the graph. + return [`${base}.ts`, `${base}.tsx`, join(base, 'index.ts')].find((candidate) => + existsSync(candidate) + ) +} + +/** Bare specifiers stay as-is; local ones are followed so a hop cannot hide react-dom. */ +function transitiveSpecifiers(entryFile: string): Set { + const seen = new Set() + const specifiers = new Set() + const queue = [entryFile] + while (queue.length > 0) { + const file = queue.pop() as string + if (seen.has(file)) { + continue + } + seen.add(file) + for (const specifier of importedSpecifiers(readFileSync(file, 'utf8'))) { + specifiers.add(specifier) + const local = resolveLocalModule(specifier, file) + if (local) { + queue.push(local) + } + } + } + return specifiers +} + +describe('react commit cascade shim install order', () => { + for (const entry of INSTRUMENTED_ENTRIES) { + it(`keeps the import-free shim as the first import in ${entry}`, () => { + const source = readFileSync(join(RENDERER_ROOT, entry), 'utf8') + + expect(source).toContain(SHIM_IMPORT) + expect(firstImportStatement(source)).toBe(SHIM_IMPORT) + }) + + // Why separate: the observer wraps a property react-dom re-reads per commit, + // so it only has to be in the graph, not first in it. + it(`still installs the observer in ${entry}`, () => { + expect(readFileSync(join(RENDERER_ROOT, entry), 'utf8')).toContain(OBSERVER_IMPORT) + }) + } + + for (const entry of UNINSTRUMENTED_ENTRIES) { + it(`leaves ${entry} uninstrumented, where breadcrumbs are a no-op`, () => { + const source = readFileSync(join(RENDERER_ROOT, entry), 'utf8') + + expect(source).not.toContain('react-commit-cascade-observer') + }) + } + + // Why this is the real ratchet: one import here that transitively reaches + // react-dom would let react-dom evaluate first and read no hook, killing the + // diagnostic in production with every other test still green. + it('keeps the shim free of imports, react-dom above all', () => { + const shimFile = join(RENDERER_ROOT, 'lib', `${SHIM_MODULE}.ts`) + + const graph = transitiveSpecifiers(shimFile) + expect(Array.from(graph).filter((specifier) => specifier.includes('react-dom'))).toEqual([]) + expect(Array.from(graph)).toEqual([]) + // Why a source scan too: the specifier regex only sees an import whose `from` + // shares a line with the keyword, and a multi-line import is the common form + // here — so the graph check alone would miss the one edit that matters. + // Why `from` must be followed by a quote: bare `\bfrom\b` also matched the + // word in a comment, and a ratchet that fails on prose gets deleted. + // Why the `import(` arm: a top-level `await import(...)` makes this module + // async, so react-dom evaluates before the hook is installed. + expect(readFileSync(shimFile, 'utf8')).not.toMatch( + /^\s*(?:import|export)\b[\s\S]*?\bfrom\s*(?:\/\*[\s\S]*?\*\/\s*)*['"]|^\s*import\s*['"]|\bimport\s*\(/m + ) + }) + + it('follows local hops when checking a graph for react-dom', () => { + const observerFile = join(RENDERER_ROOT, 'lib', 'react-commit-cascade-observer.ts') + + // The observer's own graph is allowed to be wide; this only proves the walker + // above sees past the first hop, so an empty shim graph means something. + expect(transitiveSpecifiers(observerFile).size).toBeGreaterThan(3) + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-observer.react185.test.tsx b/src/renderer/src/lib/react-commit-cascade-observer.react185.test.tsx new file mode 100644 index 00000000000..a8c4e7c4c58 --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-observer.react185.test.tsx @@ -0,0 +1,169 @@ +/** @vitest-environment happy-dom */ +// Why first: react-dom below reads __REACT_DEVTOOLS_GLOBAL_HOOK__ at module +// evaluation. The shim has no imports of its own, so unlike the observer it +// cannot drag the real breadcrumb recorder in ahead of the vi.mock below. +import './react-devtools-commit-hook-shim' +import { act, useLayoutEffect } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { create } from 'zustand' +import { withReactCommitCascadeWriteProbe } from '../store/react-commit-cascade-write-probe' +import { + REACT_CASCADING_LANES, + REACT_COMMIT_CASCADE_BREADCRUMB, + REACT_COMMIT_CASCADE_NOTICE_LIMIT, + resetReactCommitCascadeTelemetryForTests +} from './react-commit-cascade-telemetry' +import { + installReactCommitCascadeObserver, + resetReactCommitCascadeObserverForTests +} from './react-commit-cascade-observer' +import type { ReactDevtoolsCommitHook } from './react-devtools-commit-hook-shim' + +;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +const recordBreadcrumb = vi.fn() +vi.mock('@/lib/crash-breadcrumb-recorder', () => ({ + recordRendererCrashBreadcrumb: (name: string, data?: unknown) => recordBreadcrumb(name, data) +})) + +/** + * A single-driver cascade measures 438 bytes here, where every frame carries this + * file's long basename; a production bundle name is shorter. Wide enough to sit + * still, narrow enough that a raw stack or an uncapped key list breaks it. + */ +const MAX_CASCADE_CRUMB_BYTES = 768 + +/** Enough to pass the notice limit while staying under React's own 50-commit bail. */ +const CASCADE_TICKS = REACT_COMMIT_CASCADE_NOTICE_LIMIT + 5 + +type CascadeState = { ticks: number; bump: () => void } + +const useCascadeStore = create()( + withReactCommitCascadeWriteProbe((set) => ({ + ticks: 0, + bump: () => { + set({ ticks: useCascadeStore.getState().ticks + 1 }) + } + })) +) + +/** + * Layout effect, not passive: only the synchronous counter throws #185. A + * useEffect loop leaves `pendingLanes: 0` at commit time (measured) because + * passive effects flush after the callback, and React only console.errors it. + */ +function RunawayLayoutEffectPane(): React.JSX.Element { + const ticks = useCascadeStore((state) => state.ticks) + useLayoutEffect(() => { + if (ticks < CASCADE_TICKS) { + useCascadeStore.getState().bump() + } + }) + return
{ticks}
+} + +let host: HTMLDivElement +let root: Root + +const commitHook = (globalThis as { __REACT_DEVTOOLS_GLOBAL_HOOK__?: ReactDevtoolsCommitHook }) + .__REACT_DEVTOOLS_GLOBAL_HOOK__ + +beforeEach(() => { + recordBreadcrumb.mockReset() + resetReactCommitCascadeTelemetryForTests() + resetReactCommitCascadeObserverForTests() + useCascadeStore.setState({ ticks: 0 }) + // Why cleared rather than deleting the global: react-dom captured this hook + // OBJECT at its own module evaluation, so a replacement object would never be + // called — and reinstalling over the last test's wrapper double-counts commits. + if (commitHook) { + commitHook.onCommitFiberRoot = undefined + } + installReactCommitCascadeObserver() + host = document.createElement('div') + document.body.appendChild(host) + root = createRoot(host) +}) + +afterEach(() => { + act(() => { + root.unmount() + }) + host.remove() +}) + +describe('react commit cascade observer', () => { + it('counts a genuine synchronous cascade and names the store write driving it', () => { + act(() => { + root.render() + }) + + const cascadeCalls = recordBreadcrumb.mock.calls.filter( + ([name]) => name === REACT_COMMIT_CASCADE_BREADCRUMB + ) + expect(cascadeCalls).toHaveLength(1) + + const payload = (cascadeCalls[0]?.[1] ?? {}) as Record + expect(payload.commits).toBe(REACT_COMMIT_CASCADE_NOTICE_LIMIT) + expect(payload.pendingLanes).toBeGreaterThan(0) + expect(payload.storeWrites).toBeGreaterThan(0) + // The middleware boundary is elided, so this is the code that called `set`. + expect(String(payload.driverFrame)).toContain('bump') + expect(payload.changedKeys).toBe('ticks') + expect(payload.rendererSurface).toBe('main') + }) + + // Why counted from outside our own wrapper: a bundler change that installs the + // observer twice counts every commit twice and fires the crumb at half the real + // depth, which no other assertion here can tell from a correct run. + it('counts exactly one commit per hook invocation', () => { + let cascadingInvocations = 0 + const observed = commitHook?.onCommitFiberRoot + if (commitHook) { + // The mount commit holds no cascading lanes, so only these are countable. + commitHook.onCommitFiberRoot = (rendererId, fiberRoot, priorityLevel, didError) => { + const lanes = (fiberRoot as { pendingLanes?: number } | null)?.pendingLanes ?? 0 + if ((lanes & REACT_CASCADING_LANES) !== 0) { + cascadingInvocations += 1 + } + observed?.call(commitHook, rendererId, fiberRoot, priorityLevel, didError) + } + } + let invocationsAtReport: number | undefined + recordBreadcrumb.mockImplementation((name: string) => { + if (name === REACT_COMMIT_CASCADE_BREADCRUMB && invocationsAtReport === undefined) { + invocationsAtReport = cascadingInvocations + } + }) + + act(() => { + root.render() + }) + + expect(invocationsAtReport).toBe(REACT_COMMIT_CASCADE_NOTICE_LIMIT) + }) + + // Why a byte budget: driverStack's key ends in `stack`, which buys 4000 chars, + // so a raw stack or an uncapped key list could be added here unnoticed. + it('keeps the crumb inside a flat byte budget', () => { + act(() => { + root.render() + }) + + const payload = recordBreadcrumb.mock.calls.find( + ([name]) => name === REACT_COMMIT_CASCADE_BREADCRUMB + )?.[1] + expect(JSON.stringify(payload).length).toBeLessThan(MAX_CASCADE_CRUMB_BYTES) + }) + + it('stays silent for a render that settles well below the notice limit', () => { + useCascadeStore.setState({ ticks: CASCADE_TICKS }) + + act(() => { + root.render() + }) + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-observer.test.ts b/src/renderer/src/lib/react-commit-cascade-observer.test.ts new file mode 100644 index 00000000000..2928afdcebc --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-observer.test.ts @@ -0,0 +1,307 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + REACT_COMMIT_CASCADE_BREADCRUMB, + REACT_COMMIT_CASCADE_NOTICE_LIMIT, + resetReactCommitCascadeTelemetryForTests +} from './react-commit-cascade-telemetry' +import { + REACT_COMMIT_CASCADE_INSTALL_CHECK_MS, + REACT_COMMIT_CASCADE_UNINSTALLED_BREADCRUMB, + installReactCommitCascadeObserver, + resetReactCommitCascadeObserverForTests +} from './react-commit-cascade-observer' +import { + ensureReactDevtoolsCommitHook, + type ReactDevtoolsCommitHook +} from './react-devtools-commit-hook-shim' + +const recordBreadcrumb = vi.fn() +vi.mock('@/lib/crash-breadcrumb-recorder', () => ({ + recordRendererCrashBreadcrumb: (name: string, data?: unknown) => recordBreadcrumb(name, data) +})) + +const CASCADING_ROOT = { pendingLanes: 2 } + +function readHook(): ReactDevtoolsCommitHook { + return (globalThis as { __REACT_DEVTOOLS_GLOBAL_HOOK__?: ReactDevtoolsCommitHook }) + .__REACT_DEVTOOLS_GLOBAL_HOOK__ as ReactDevtoolsCommitHook +} + +beforeEach(() => { + recordBreadcrumb.mockClear() + resetReactCommitCascadeTelemetryForTests() + resetReactCommitCascadeObserverForTests() + // Why the delete: install wraps whatever is there, so a hook left over from + // the previous test would stack a second counter onto the same callback. + delete (globalThis as { __REACT_DEVTOOLS_GLOBAL_HOOK__?: unknown }).__REACT_DEVTOOLS_GLOBAL_HOOK__ + installReactCommitCascadeObserver() +}) + +afterEach(() => { + vi.useRealTimers() +}) + +describe('react devtools commit hook shim', () => { + // Why asserted: without a pre-existing hook react-dom injects nothing, so a + // packaged build only gets commit callbacks if the shim installs its own. + it('exposes what react-dom requires to inject', () => { + const hook = readHook() + + expect(hook.supportsFiber).toBe(true) + expect(hook.isDisabled).toBe(false) + expect(hook.inject?.({})).toBeGreaterThan(0) + expect(typeof hook.onCommitFiberRoot).toBe('function') + }) + + // Why asserted: react-dom calls this once per DELETED FIBER, guarded only by + // a typeof check. Defining it would put our shim on every unmount path. + it('leaves the per-deleted-fiber callbacks undefined', () => { + const hook = readHook() as Record + + expect(hook.onCommitFiberUnmount).toBeUndefined() + expect(hook.onPostCommitFiberRoot).toBeUndefined() + expect(hook.setStrictMode).toBeUndefined() + }) + + // Why asserted: react-refresh captures this property and calls + // oldOnCommitFiberRoot.apply(this, arguments) with no typeof guard, unlike the + // fallback it gives onScheduleFiberRoot. + it('ships a callable onCommitFiberRoot before anything wraps it', () => { + delete (globalThis as { __REACT_DEVTOOLS_GLOBAL_HOOK__?: unknown }) + .__REACT_DEVTOOLS_GLOBAL_HOOK__ + const hook = ensureReactDevtoolsCommitHook() + + expect(typeof hook?.onCommitFiberRoot).toBe('function') + expect(() => hook?.onCommitFiberRoot?.(1, null, undefined, false)).not.toThrow() + }) +}) + +describe('installReactCommitCascadeObserver', () => { + it('breadcrumbs once the commit callback reaches the notice limit', () => { + const hook = readHook() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT; commit += 1) { + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + } + + expect(recordBreadcrumb).toHaveBeenCalledTimes(1) + expect(recordBreadcrumb.mock.calls[0]?.[0]).toBe(REACT_COMMIT_CASCADE_BREADCRUMB) + }) + + it('ignores a commit whose root reports no cascading lanes', () => { + const hook = readHook() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT * 2; commit += 1) { + hook.onCommitFiberRoot?.(1, { pendingLanes: 0 }, undefined, false) + } + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + // Why asserted: a future React that stops exposing pendingLanes must make the + // diagnostic go quiet, not fire on a depth it can no longer evaluate. + it('ends the cascade when pendingLanes is not readable', () => { + const hook = readHook() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + } + + hook.onCommitFiberRoot?.(1, { renamedLanes: 2 }, undefined, false) + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + } + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + // Why a string and not undefined: '2' & 42 is 2, so a bitmask-only guard would + // keep the cascade alive on a value React never meant as a lane. + it('ends the cascade when pendingLanes is not a number', () => { + const hook = readHook() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + } + + hook.onCommitFiberRoot?.(1, { pendingLanes: '2' }, undefined, false) + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + } + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + it('chains onto an existing callback instead of replacing it', () => { + const previous = vi.fn() + const hook = readHook() + hook.onCommitFiberRoot = previous + resetReactCommitCascadeObserverForTests() + installReactCommitCascadeObserver() + + hook.onCommitFiberRoot?.(7, CASCADING_ROOT, undefined, true) + + expect(previous).toHaveBeenCalledWith(7, CASCADING_ROOT, undefined, true) + }) + + // Why asserted: our throw would otherwise land in react-dom's own catch and + // silently unhook DevTools and Fast Refresh from the rest of the chain. + it('still calls the chained hook when our own work throws', () => { + const previous = vi.fn() + const hook = readHook() + hook.onCommitFiberRoot = previous + resetReactCommitCascadeObserverForTests() + installReactCommitCascadeObserver() + + const hostile = { + get pendingLanes(): number { + throw new Error('hostile root') + } + } + expect(() => hook.onCommitFiberRoot?.(1, hostile, undefined, false)).not.toThrow() + expect(previous).toHaveBeenCalledTimes(1) + }) + + it('is idempotent, so a second install cannot double-count commits', () => { + installReactCommitCascadeObserver() + const hook = readHook() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + } + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) +}) + +// Why: a reshuffled import or a bundler hoist disables the diagnostic with no +// test failure. This is what makes that visible in the field instead. +describe('install self-check', () => { + it('breadcrumbs when no commit ever reached the hook', () => { + vi.useFakeTimers() + resetReactCommitCascadeObserverForTests() + installReactCommitCascadeObserver() + + vi.advanceTimersByTime(REACT_COMMIT_CASCADE_INSTALL_CHECK_MS) + + expect(recordBreadcrumb).toHaveBeenCalledWith( + REACT_COMMIT_CASCADE_UNINSTALLED_BREADCRUMB, + undefined + ) + }) + + it('stays silent once any commit has been seen', () => { + vi.useFakeTimers() + resetReactCommitCascadeObserverForTests() + installReactCommitCascadeObserver() + readHook().onCommitFiberRoot?.(1, { pendingLanes: 0 }, undefined, false) + + vi.advanceTimersByTime(REACT_COMMIT_CASCADE_INSTALL_CHECK_MS) + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + // Why asserted: a hook that refuses the assignment is exactly the case the + // self-check exists for, so it must arm on the failure path too. + it('breadcrumbs when the hook refuses the callback assignment', () => { + vi.useFakeTimers() + resetReactCommitCascadeObserverForTests() + const frozenHook = Object.freeze({ isDisabled: false, supportsFiber: true }) + ;(globalThis as { __REACT_DEVTOOLS_GLOBAL_HOOK__?: unknown }).__REACT_DEVTOOLS_GLOBAL_HOOK__ = + frozenHook + + expect(() => installReactCommitCascadeObserver()).not.toThrow() + vi.advanceTimersByTime(REACT_COMMIT_CASCADE_INSTALL_CHECK_MS) + + expect((frozenHook as { onCommitFiberRoot?: unknown }).onCommitFiberRoot).toBeUndefined() + expect(recordBreadcrumb).toHaveBeenCalledWith( + REACT_COMMIT_CASCADE_UNINSTALLED_BREADCRUMB, + undefined + ) + }) + + it('breadcrumbs when no hook can be reached at all', () => { + vi.useFakeTimers() + resetReactCommitCascadeObserverForTests() + Object.defineProperty(globalThis, '__REACT_DEVTOOLS_GLOBAL_HOOK__', { + configurable: true, + get() { + throw new Error('no devtools global') + } + }) + + expect(() => installReactCommitCascadeObserver()).not.toThrow() + vi.advanceTimersByTime(REACT_COMMIT_CASCADE_INSTALL_CHECK_MS) + + expect(recordBreadcrumb).toHaveBeenCalledWith( + REACT_COMMIT_CASCADE_UNINSTALLED_BREADCRUMB, + undefined + ) + }) +}) + +/** + * Why an allocation assertion and not a timing one: this callback runs on every + * commit the app makes, so the cost that matters is garbage. Measured on this + * loop, an escaping object literal, a template string and a per-call closure + * each cost more than 3.8 MB where the current code costs tens of KB. + */ +describe('commit hook cost', () => { + const HOT_COMMITS = 1_000_000 + /** ~30x the measured zero-allocation baseline, ~4x under the cheapest regression. */ + const MAX_HEAP_GROWTH_BYTES = 1_000_000 + + function measureHeapGrowth(commit: () => void): number { + const collectGarbage = (globalThis as { gc?: () => void }).gc + // --expose-gc is pinned in config/vitest.config.ts execArgv. + expect(typeof collectGarbage).toBe('function') + for (let warmup = 0; warmup < 50_000; warmup += 1) { + commit() + } + collectGarbage?.() + const before = process.memoryUsage().heapUsed + for (let iteration = 0; iteration < HOT_COMMITS; iteration += 1) { + commit() + } + return process.memoryUsage().heapUsed - before + } + + it('allocates nothing on an ordinary commit', () => { + const hook = readHook() + const quietRoot = { pendingLanes: 0 } + + const growth = measureHeapGrowth(() => hook.onCommitFiberRoot?.(1, quietRoot, undefined, false)) + + expect(growth).toBeLessThan(MAX_HEAP_GROWTH_BYTES) + }) + + // Why separately: the cascading branch runs the counter, the arm check and the + // report guard, none of which may start allocating once a cascade is deep. + it('allocates nothing while a cascade is being counted', () => { + const hook = readHook() + + const growth = measureHeapGrowth(() => + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + ) + + expect(growth).toBeLessThan(MAX_HEAP_GROWTH_BYTES) + }) +}) + +// Why asserted: main.tsx imports this module for its side effect alone, so +// deleting the bare install call would disable the diagnostic in production. +describe('module self-install', () => { + it('counts commits after nothing but an import', async () => { + vi.useFakeTimers() + delete (globalThis as { __REACT_DEVTOOLS_GLOBAL_HOOK__?: unknown }) + .__REACT_DEVTOOLS_GLOBAL_HOOK__ + vi.resetModules() + + await import('./react-commit-cascade-observer') + + const hook = readHook() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT; commit += 1) { + hook.onCommitFiberRoot?.(1, CASCADING_ROOT, undefined, false) + } + + expect(recordBreadcrumb).toHaveBeenCalledWith( + REACT_COMMIT_CASCADE_BREADCRUMB, + expect.objectContaining({ commits: REACT_COMMIT_CASCADE_NOTICE_LIMIT }) + ) + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-observer.ts b/src/renderer/src/lib/react-commit-cascade-observer.ts new file mode 100644 index 00000000000..bfac75898bd --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-observer.ts @@ -0,0 +1,101 @@ +/** + * Installs the per-commit hook that feeds the cascade telemetry. + * + * Why the devtools callback and not or a root effect: Profiler is + * stripped from production react-dom, and a dependency-less useLayoutEffect + * fires per render of its OWN component, not per commit — measured, a root + * effect saw 1 of 11 commits a leaf drove. react-dom calls onCommitFiberRoot + * once per commit per root in both bundles, inside its own try/catch, and hands + * us the FiberRoot whose `pendingLanes` drive the reset rule. + * + * Ordering: this module only WRAPS `onCommitFiberRoot`, which react-dom re-reads + * at every commit, so it may be imported anywhere. The deadline belongs to + * react-devtools-commit-hook-shim, which must merely exist before react-dom's + * module evaluation and is therefore the entries' first import. + */ +import { observeReactCommit } from '@/lib/react-commit-cascade-telemetry' +import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' +import { + ensureReactDevtoolsCommitHook, + type ReactDevtoolsCommitHook +} from '@/lib/react-devtools-commit-hook-shim' + +export const REACT_COMMIT_CASCADE_UNINSTALLED_BREADCRUMB = 'react_commit_cascade_uninstalled' + +/** Long enough that any live renderer has committed; short enough to reach a crash report. */ +export const REACT_COMMIT_CASCADE_INSTALL_CHECK_MS = 10_000 + +type FiberRootLike = { pendingLanes?: unknown } + +let installed = false +let commitsSeen = 0 +let installCheckTimer: ReturnType | undefined + +/** + * A reshuffled import or a bundler hoist would disable the diagnostic with no + * test failure. Make that visible in the field instead of silent. + */ +function scheduleInstallSelfCheck(): void { + if (installCheckTimer !== undefined || typeof setTimeout !== 'function') { + return + } + installCheckTimer = setTimeout(() => { + if (commitsSeen === 0) { + recordRendererCrashBreadcrumb(REACT_COMMIT_CASCADE_UNINSTALLED_BREADCRUMB) + } + }, REACT_COMMIT_CASCADE_INSTALL_CHECK_MS) + // Why unref where available: the check must never hold a renderer teardown open. + ;(installCheckTimer as { unref?: () => void })?.unref?.() +} + +export function installReactCommitCascadeObserver(): void { + if (installed) { + return + } + try { + const hook = ensureReactDevtoolsCommitHook() + if (hook) { + installObserverOnHook(hook) + // Why only after the assignment: a hook that refuses it stays uninstalled, + // so a later call can retry rather than being gated out. + installed = true + } + } catch { + // A renderer without a patchable global still has to boot. + } + // Why outside the try: a failed install is exactly when the crumb matters. + scheduleInstallSelfCheck() +} + +function installObserverOnHook(hook: ReactDevtoolsCommitHook): void { + const previous = hook.onCommitFiberRoot + // Fixed arity, not rest args: this runs on every commit and must not allocate. + hook.onCommitFiberRoot = (rendererId, root, priorityLevel, didError) => { + // Why our own try/catch when react-dom already has one: theirs would + // swallow the rest of this callback, silently unhooking DevTools and + // Fast Refresh from the chain below. + try { + commitsSeen += 1 + const pendingLanes = (root as FiberRootLike | null)?.pendingLanes + // Why 0 and not a skip: a React shape change that hides pendingLanes must + // end the cascade, not freeze a stale depth that later commits push over + // the limit. 0 is the lane value that means "nothing pending", so it + // takes the same reset path and this fails safe to no crumb. + observeReactCommit(root, typeof pendingLanes === 'number' ? pendingLanes : 0) + } catch { + // Best-effort crash evidence only. + } + previous?.call(hook, rendererId, root, priorityLevel, didError) + } +} + +export function resetReactCommitCascadeObserverForTests(): void { + installed = false + commitsSeen = 0 + if (installCheckTimer !== undefined) { + clearTimeout(installCheckTimer) + installCheckTimer = undefined + } +} + +installReactCommitCascadeObserver() diff --git a/src/renderer/src/lib/react-commit-cascade-store-write-samples.test.ts b/src/renderer/src/lib/react-commit-cascade-store-write-samples.test.ts new file mode 100644 index 00000000000..17489399883 --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-store-write-samples.test.ts @@ -0,0 +1,144 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { create } from 'zustand' +import { withReactCommitCascadeWriteProbe } from '../store/react-commit-cascade-write-probe' +import { + MAX_REPORTED_CHANGED_KEYS, + MAX_SAMPLED_WRITES, + armReactCommitCascadeWriteSampling, + noteReactCommitCascadeStoreWrite, + readReactCommitCascadeWriteSummary, + resetReactCommitCascadeWriteSamples +} from './react-commit-cascade-store-write-samples' + +// Not an intersection with ErrorConstructor: both fields have to stay optional +// so the absent-captureStackTrace platform can be simulated. +type ErrorWithCapture = { + captureStackTrace?: (target: object, constructorOpt?: unknown) => void + stackTraceLimit?: number +} +const errorWithCapture = Error as unknown as ErrorWithCapture +const originalCapture = errorWithCapture.captureStackTrace +const originalLimit = errorWithCapture.stackTraceLimit + +/** Stands in for the zustand slice action that calls `set`. */ +function storeAction(): void { + noteReactCommitCascadeStoreWrite(storeAction, { ticks: 1 }) +} +function runawayEffect(): void { + storeAction() +} +function paneRender(): void { + runawayEffect() +} + +beforeEach(() => { + resetReactCommitCascadeWriteSamples() +}) + +afterEach(() => { + resetReactCommitCascadeWriteSamples() + errorWithCapture.captureStackTrace = originalCapture + errorWithCapture.stackTraceLimit = originalLimit +}) + +describe('driver stack frames', () => { + // Why more than one frame: frame 0 is the slice action every cascade shares. + // The caller above it is the loop we are actually hunting. + it('names callers above the frame the write came from', () => { + armReactCommitCascadeWriteSampling() + paneRender() + + const summary = readReactCommitCascadeWriteSummary() + const frames = summary.driverStack?.split('\n') ?? [] + expect(summary.driverFrame).toContain('runawayEffect') + expect(frames[0]).toBe(summary.driverFrame) + expect(frames.length).toBeGreaterThan(1) + expect(summary.driverStack).toContain('paneRender') + expect(summary.driverFrame).not.toContain('paneRender') + }) + + // Why: the redaction that strips paths is keyed on the detail NAME, so a path + // inside this value would ship a developer's home directory. + it('keeps every reported frame path-free', () => { + armReactCommitCascadeWriteSampling() + paneRender() + + expect(readReactCommitCascadeWriteSummary().driverStack).not.toContain('/') + }) +}) + +describe('capture failures', () => { + // Why asserted: the limit is process-global, so leaking a raised value would + // make every unrelated throw in the app capture eight frames forever. + it('restores Error.stackTraceLimit when the capture throws', () => { + errorWithCapture.stackTraceLimit = 3 + errorWithCapture.captureStackTrace = () => { + throw new Error('capture refused') + } + armReactCommitCascadeWriteSampling() + + expect(() => storeAction()).not.toThrow() + expect(errorWithCapture.stackTraceLimit).toBe(3) + expect(readReactCommitCascadeWriteSummary().storeWrites).toBe(1) + }) + + // Why asserted: no sample is ever pushed without captureStackTrace, so a cap + // bound to samples.length would leave key collection running for the whole + // cascade on any engine that lacks the API. + it('stops collecting after the sample cap when captureStackTrace is unavailable', () => { + delete errorWithCapture.captureStackTrace + armReactCommitCascadeWriteSampling() + for (let write = 0; write < 100; write += 1) { + noteReactCommitCascadeStoreWrite(storeAction, { [`key${write}`]: write }) + } + // Restored before asserting: vitest builds its own failure stacks with it. + errorWithCapture.captureStackTrace = originalCapture + + const summary = readReactCommitCascadeWriteSummary() + expect(summary.storeWrites).toBe(100) + expect(summary.driverFrame).toBeUndefined() + expect(summary.storeWriteSites).toBe(0) + expect(summary.changedKeys?.split(',')).toHaveLength(MAX_SAMPLED_WRITES) + expect(summary.changedKeys).not.toContain('key99') + expect(summary.changedKeys?.length).toBeLessThan(240) + }) + + // Why still asserted: one write can change many keys, so the report cap is + // what keeps the detail inside its 240-char budget. + it('caps reported changed keys from a single wide write', () => { + const wide = Object.fromEntries( + Array.from({ length: 40 }, (_, index) => [`key${index}`, index]) + ) + armReactCommitCascadeWriteSampling() + noteReactCommitCascadeStoreWrite(storeAction, wide) + + const summary = readReactCommitCascadeWriteSummary() + expect(summary.changedKeys?.split(',')).toHaveLength(MAX_REPORTED_CHANGED_KEYS) + expect(summary.changedKeys?.length).toBeLessThan(240) + }) +}) + +// Why measured through the real middleware: the arming condition is what keeps +// an ordinary write down to one boolean read, and nothing else asserts it end to end. +describe('disarmed cost', () => { + const WRITE_FLOOD = 10_000 + + it('captures no stack across a write flood with no cascade', () => { + const useFloodStore = create<{ ticks: number }>()( + withReactCommitCascadeWriteProbe(() => ({ ticks: 0 })) + ) + let captures = 0 + errorWithCapture.captureStackTrace = (target, boundary) => { + captures += 1 + originalCapture?.(target, boundary) + } + + for (let write = 0; write < WRITE_FLOOD; write += 1) { + useFloodStore.setState({ ticks: write }) + } + + expect(captures).toBe(0) + expect(useFloodStore.getState().ticks).toBe(WRITE_FLOOD - 1) + expect(readReactCommitCascadeWriteSummary().storeWrites).toBe(0) + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-store-write-samples.ts b/src/renderer/src/lib/react-commit-cascade-store-write-samples.ts new file mode 100644 index 00000000000..ef8565255a8 --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-store-write-samples.ts @@ -0,0 +1,157 @@ +/** + * Samples the store writes made during a suspected React commit cascade. + * + * Why sampling writes and not the throw site: React #185 lands on whichever + * fiber dispatched after a root-global counter tripped, so the reported + * component is a bystander (see shared/react-update-depth-attribution.ts). The + * writes that kept scheduling are the only frames that identify the loop. + * + * Cost discipline: the armed flag is the only thing the store-write path reads + * when no cascade is suspected, capture is capped per cascade, and frames are + * formatted at report time so a sampled write pays for nothing it may not use. + */ + +/** + * Read on every store write, so it is a plain field load on a shared object + * rather than a cross-module call. + */ +export const reactCommitCascadeWriteProbe = { armed: false } + +/** Frame 0 is the store action; the rest reach up into whatever loops over it. */ +const CAPTURE_STACK_FRAME_LIMIT = 8 +/** + * Frame 0 alone names a slice action, which every cascade shares. Two frames of + * caller is what distinguishes the effect driving the loop from its neighbours. + */ +const REPORTED_FRAMES_PER_SITE = 3 +/** Whole frames only, inside the 4000-char budget a `stack`-suffixed detail gets. */ +const MAX_DRIVER_STACK_CHARS = 3_800 +/** A loop repeats; six samples name every distinct participant worth naming. */ +export const MAX_SAMPLED_WRITES = 6 +/** Keeps `changedKeys` inside the 240-char detail cap without truncating mid-key. */ +export const MAX_REPORTED_CHANGED_KEYS = 12 + +type SampledWrite = { stack?: string } + +let storeWrites = 0 +let samples: SampledWrite[] = [] +let changedKeys: Set | null = null + +export type ReactCommitCascadeWriteSummary = { + storeWrites: number + storeWriteSites: number + driverFrame: string | undefined + driverStack: string | undefined + changedKeys: string | undefined +} + +export function armReactCommitCascadeWriteSampling(): void { + reactCommitCascadeWriteProbe.armed = true +} + +export function resetReactCommitCascadeWriteSamples(): void { + reactCommitCascadeWriteProbe.armed = false + storeWrites = 0 + samples = [] + changedKeys = null +} + +/** + * Call only while armed. `boundary` is the wrapping `set` function, so V8 elides + * our own frames and the first captured frame is the real caller. Typed as + * `object` because zustand's `set` is an overload set, not a plain signature. + */ +export function noteReactCommitCascadeStoreWrite(boundary: object, partial: unknown): void { + storeWrites += 1 + // Why the write count and not samples.length: samples only grows where + // Error.captureStackTrace exists, so that cap would never engage without it and + // key collection would run for the whole cascade. + if (storeWrites > MAX_SAMPLED_WRITES) { + return + } + // Why object-only: a functional updater's keys are unknowable without running it. + if (partial && typeof partial === 'object') { + changedKeys ??= new Set() + for (const key of Object.keys(partial)) { + changedKeys.add(key) + } + } + const capture = Error as ErrorConstructor & { + captureStackTrace?: (target: object, constructorOpt?: unknown) => void + stackTraceLimit?: number + } + if (typeof capture.captureStackTrace !== 'function') { + return + } + const previousLimit = capture.stackTraceLimit + const sample: SampledWrite = {} + try { + capture.stackTraceLimit = CAPTURE_STACK_FRAME_LIMIT + capture.captureStackTrace(sample, boundary) + samples.push(sample) + } catch { + // Best-effort crash evidence only. + } finally { + capture.stackTraceLimit = previousLimit + } +} + +/** `at fn (/Users/me/app/src/x.ts:1:2)` becomes `x.ts:1:2 fn`. */ +function reduceFrame(frame: string): string | undefined { + const match = /^\s*at\s+(?:(.+?)\s+\()?(.+?):(\d+):(\d+)\)?\s*$/.exec(frame) + if (!match) { + return undefined + } + const [, fn, location, line, column] = match + // Why basename only: the redaction that strips paths is keyed on the detail + // NAME, so a full path inside this value would ship a developer's home dir. + const basename = location?.split(/[/\\]/).pop() ?? '' + return fn ? `${basename}:${line}:${column} ${fn}` : `${basename}:${line}:${column}` +} + +/** One reduced frame group per distinct call site, deepest frame first. */ +function sampledSiteFrames(): string[][] { + const sites: string[][] = [] + const seen = new Set() + for (const sample of samples) { + const frames = (sample.stack?.split('\n') ?? []) + .map(reduceFrame) + .filter((frame): frame is string => frame !== undefined) + .slice(0, REPORTED_FRAMES_PER_SITE) + const site = frames[0] + if (site === undefined || seen.has(site)) { + continue + } + seen.add(site) + sites.push(frames) + } + return sites +} + +/** Drops whole frames, so a truncated stack never ends mid-frame. */ +function joinWithinBudget(frames: string[]): string | undefined { + const kept: string[] = [] + let remaining = MAX_DRIVER_STACK_CHARS + for (const frame of frames) { + if (frame.length + 1 > remaining) { + break + } + kept.push(frame) + remaining -= frame.length + 1 + } + return kept.length > 0 ? kept.join('\n') : undefined +} + +/** Formats only here, so an unreported cascade pays nothing for its samples. */ +export function readReactCommitCascadeWriteSummary(): ReactCommitCascadeWriteSummary { + const sites = sampledSiteFrames() + const keys = changedKeys ? Array.from(changedKeys).slice(0, MAX_REPORTED_CHANGED_KEYS) : [] + return { + storeWrites, + storeWriteSites: sites.length, + // Why frame 0 stays alone: the main-side coalesce key clusters on it. + driverFrame: sites[0]?.[0], + driverStack: joinWithinBudget(sites.flat()), + changedKeys: keys.length > 0 ? keys.join(',') : undefined + } +} diff --git a/src/renderer/src/lib/react-commit-cascade-telemetry.test.ts b/src/renderer/src/lib/react-commit-cascade-telemetry.test.ts new file mode 100644 index 00000000000..89e0456c2ce --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-telemetry.test.ts @@ -0,0 +1,339 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { sanitizeCrashReportDetails } from '../../../shared/crash-report-redaction' +import { REACT_NESTED_UPDATE_LIMIT } from '../../../shared/react-update-depth-attribution' +import { + REACT_CASCADING_LANES, + setReactCommitCascadeRendererSurface, + REACT_COMMIT_CASCADE_ARM_COMMITS, + REACT_COMMIT_CASCADE_BREADCRUMB, + REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS, + REACT_COMMIT_CASCADE_NOTICE_LIMIT, + createReactCommitCascadeState, + recordReactCommit, + resetReactCommitCascadeTelemetryForTests, + type ReactCommitCascadeState +} from './react-commit-cascade-telemetry' +import { + noteReactCommitCascadeStoreWrite, + reactCommitCascadeWriteProbe, + readReactCommitCascadeWriteSummary +} from './react-commit-cascade-store-write-samples' + +const recordBreadcrumb = vi.fn() +vi.mock('@/lib/crash-breadcrumb-recorder', () => ({ + recordRendererCrashBreadcrumb: (name: string, data?: unknown) => recordBreadcrumb(name, data) +})) + +const ROOT = { pendingLanes: 2 } +const OTHER_ROOT = { pendingLanes: 2 } +/** SyncLane; what react-dom leaves pending on a cascading commit. */ +const SYNC_LANE = 2 +/** Any lane outside React's nested-update mask ends the cascade. */ +const IDLE_LANE = 1_073_741_824 + +function cascadePayload(callIndex = 0): Record { + const call = recordBreadcrumb.mock.calls[callIndex] + return (call?.[1] ?? {}) as Record +} + +function driveCommits(args: { + state: ReactCommitCascadeState + count: number + root?: unknown + lanes?: number + nowMs?: number + onCommit?: () => void +}): void { + const readNowMs = (): number => args.nowMs ?? 1_000 + for (let index = 0; index < args.count; index += 1) { + recordReactCommit({ + state: args.state, + root: args.root ?? ROOT, + pendingLanes: args.lanes ?? SYNC_LANE, + readNowMs + }) + args.onCommit?.() + } +} + +beforeEach(() => { + recordBreadcrumb.mockClear() + resetReactCommitCascadeTelemetryForTests() +}) + +// Why asserted: both thresholds only mean anything relative to React's own bail. +// If the notice limit ever meets it, the crumb stops beating the throw. +describe('cascade thresholds', () => { + it('derives arm and notice limits from React commit budget', () => { + expect(REACT_COMMIT_CASCADE_NOTICE_LIMIT).toBeLessThan(REACT_NESTED_UPDATE_LIMIT) + expect(REACT_COMMIT_CASCADE_ARM_COMMITS).toBeLessThan(REACT_COMMIT_CASCADE_NOTICE_LIMIT) + expect(REACT_CASCADING_LANES).toBe(42) + }) +}) + +describe('recordReactCommit', () => { + it('stays silent below the notice limit', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1 }) + + expect(recordBreadcrumb).not.toHaveBeenCalled() + expect(state.commits).toBe(REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1) + }) + + it('fires exactly once at the notice limit and not again in the same cascade', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT * 3 }) + + expect(recordBreadcrumb).toHaveBeenCalledTimes(1) + expect(recordBreadcrumb.mock.calls[0]?.[0]).toBe(REACT_COMMIT_CASCADE_BREADCRUMB) + expect(cascadePayload().commits).toBe(REACT_COMMIT_CASCADE_NOTICE_LIMIT) + }) + + it('emits flat primitives only', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT }) + + const payload = cascadePayload() + expect( + Object.values(payload).every((value) => value === null || typeof value !== 'object') + ).toBe(true) + expect(payload).toMatchObject({ + commits: REACT_COMMIT_CASCADE_NOTICE_LIMIT, + commitBudget: REACT_NESTED_UPDATE_LIMIT, + pendingLanes: SYNC_LANE, + storeWrites: 0, + storeWriteSites: 0, + rendererSurface: 'main' + }) + }) + + // Why this is the whole reset rule: React zeroes its counter the moment a + // commit leaves no sync lanes pending, so a lull must not accumulate. + it('ends the cascade when the root stops holding cascading lanes', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1 }) + + driveCommits({ state, count: 1, lanes: 0 }) + + expect(state.commits).toBe(0) + expect(state.cascadeRoot).toBeNull() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1 }) + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + it('ignores lanes outside React nested-update mask', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT * 2, lanes: IDLE_LANE }) + + expect(state.commits).toBe(0) + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + // Why per root: React's counter is root-global but not cross-root, so two + // roots alternating commits are not one cascade. + it('restarts the count when a different root commits', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1 }) + + driveCommits({ state, count: 1, root: OTHER_ROOT }) + + expect(state.commits).toBe(1) + expect(state.cascadeRoot).toBe(OTHER_ROOT) + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + it('honours explicit limit overrides', () => { + const state = createReactCommitCascadeState() + for (let index = 0; index < 4; index += 1) { + recordReactCommit({ + state, + root: ROOT, + pendingLanes: SYNC_LANE, + readNowMs: () => 1_000, + noticeLimit: 4, + armCommits: 2 + }) + } + + expect(cascadePayload().commits).toBe(4) + }) + + // Why asserted: a clock read per commit is the one cost this design refuses. + it('reads the clock only at arm and at report', () => { + const state = createReactCommitCascadeState() + const readNowMs = vi.fn(() => 1_000) + for (let index = 0; index < REACT_COMMIT_CASCADE_NOTICE_LIMIT; index += 1) { + recordReactCommit({ state, root: ROOT, pendingLanes: SYNC_LANE, readNowMs }) + } + + expect(readNowMs).toHaveBeenCalledTimes(2) + }) +}) + +// Why asserted: the popout is a separate BrowserWindow with its own module +// state, so the payload is the only thing that says which window cascaded. +describe('renderer surface', () => { + it('reports the surface the cascade happened on', () => { + setReactCommitCascadeRendererSurface('dashboard-popout') + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT }) + + expect(cascadePayload().rendererSurface).toBe('dashboard-popout') + }) +}) + +describe('driver attribution', () => { + it('arms write sampling at the arm floor, not before', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_ARM_COMMITS - 1 }) + expect(reactCommitCascadeWriteProbe.armed).toBe(false) + + driveCommits({ state, count: 1 }) + expect(reactCommitCascadeWriteProbe.armed).toBe(true) + }) + + it('names the driving write with a path-free frame', () => { + const state = createReactCommitCascadeState() + function drivingWrite(): void { + noteReactCommitCascadeStoreWrite(drivingWrite, { tabs: [] }) + } + driveCommits({ + state, + count: REACT_COMMIT_CASCADE_NOTICE_LIMIT, + onCommit: () => { + if (reactCommitCascadeWriteProbe.armed) { + drivingWrite() + } + } + }) + + const payload = cascadePayload() + expect(payload.storeWrites).toBeGreaterThan(0) + // Why the caller and not drivingWrite itself: captureStackTrace elides the + // boundary frame, so in production the first frame is whoever called `set`. + expect(String(payload.driverFrame)).toContain('onCommit') + expect(payload.changedKeys).toBe('tabs') + // Why: the path redaction is keyed on the detail NAME, so a path inside the + // value would ship a developer's home directory. + expect(String(payload.driverStack)).not.toContain('/') + // Why sanitize instead of matching the key name: the 4000-char budget is + // decided by a camel-split rule in crash-report-redaction, so a hand-copied + // regex here can pass while the real rule drops the frames to 240. + // Why resolve the key from the payload: hard-coding it here is the same + // drift this assertion exists to catch. + const stackKey = Object.keys(payload).find( + (key) => key !== 'driverFrame' && String(payload[key]).includes(String(payload.driverFrame)) + ) + expect(stackKey).toBeDefined() + const longFrames = 'a'.repeat(600) + const sanitized = sanitizeCrashReportDetails({ ...payload, [stackKey as string]: longFrames }) + expect(String(sanitized[stackKey as string])).toHaveLength(longFrames.length) + }) + + it('clears samples when a cascade ends', () => { + const state = createReactCommitCascadeState() + function drivingWrite(): void { + noteReactCommitCascadeStoreWrite(drivingWrite, { tabs: [] }) + } + driveCommits({ + state, + count: REACT_COMMIT_CASCADE_NOTICE_LIMIT, + onCommit: () => { + if (reactCommitCascadeWriteProbe.armed) { + drivingWrite() + } + } + }) + recordBreadcrumb.mockClear() + + driveCommits({ state, count: 1, lanes: 0 }) + driveCommits({ + state, + count: REACT_COMMIT_CASCADE_NOTICE_LIMIT, + nowMs: 1_000 + REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS + 1 + }) + + expect(cascadePayload().storeWrites).toBe(0) + expect(cascadePayload().driverFrame).toBeUndefined() + }) +}) + +describe('report throttle', () => { + it('suppresses cascades inside the interval and carries the count forward', () => { + const state = createReactCommitCascadeState() + for (let cascade = 0; cascade < 3; cascade += 1) { + driveCommits({ state, count: 1, lanes: 0 }) + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT }) + } + + expect(recordBreadcrumb).toHaveBeenCalledTimes(1) + expect(state.suppressed).toBe(2) + + driveCommits({ state, count: 1, lanes: 0 }) + driveCommits({ + state, + count: REACT_COMMIT_CASCADE_NOTICE_LIMIT, + nowMs: 1_000 + REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS + 1 + }) + + expect(recordBreadcrumb).toHaveBeenCalledTimes(2) + expect(cascadePayload(1).suppressed).toBe(2) + expect(state.suppressed).toBe(0) + }) +}) + +/** + * Why dimensional and not just functional: a cascade that ends releases its + * samples, but a renderer that oscillates for minutes runs that arm/end cycle + * thousands of times, and anything the reset misses accumulates there. + */ +describe('repeated arm and end cycles', () => { + const CYCLES = 1_000 + /** The steady state is a few KB of live objects; a per-cycle leak is megabytes. */ + const MAX_HEAP_GROWTH_BYTES = 1_000_000 + + function drivingWrite(): void { + noteReactCommitCascadeStoreWrite(drivingWrite, { tabs: [], panes: [] }) + } + + function runCascadeCycle(state: ReactCommitCascadeState): void { + driveCommits({ + state, + count: REACT_COMMIT_CASCADE_NOTICE_LIMIT, + onCommit: () => { + if (reactCommitCascadeWriteProbe.armed) { + drivingWrite() + } + } + }) + driveCommits({ state, count: 1, lanes: 0 }) + } + + it('leaves nothing behind across a thousand cascades', () => { + const collectGarbage = (globalThis as { gc?: () => void }).gc + // --expose-gc is pinned in config/vitest.config.ts execArgv. + expect(typeof collectGarbage).toBe('function') + const state = createReactCommitCascadeState() + for (let warmup = 0; warmup < 20; warmup += 1) { + runCascadeCycle(state) + } + collectGarbage?.() + const before = process.memoryUsage().heapUsed + + for (let cycle = 0; cycle < CYCLES; cycle += 1) { + runCascadeCycle(state) + } + collectGarbage?.() + + expect(process.memoryUsage().heapUsed - before).toBeLessThan(MAX_HEAP_GROWTH_BYTES) + expect(reactCommitCascadeWriteProbe.armed).toBe(false) + expect(readReactCommitCascadeWriteSummary()).toMatchObject({ + storeWrites: 0, + storeWriteSites: 0, + driverFrame: undefined, + driverStack: undefined, + changedKeys: undefined + }) + expect(state.cascadeRoot).toBeNull() + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-telemetry.ts b/src/renderer/src/lib/react-commit-cascade-telemetry.ts new file mode 100644 index 00000000000..73051d21599 --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-telemetry.ts @@ -0,0 +1,205 @@ +/** + * Counts consecutive same-root commits that keep scheduling synchronous work, + * and breadcrumbs the cascade before React #185 throws. + * + * This mirrors React's own accounting rather than approximating it with a time + * window: react-dom resets its nested-update counter the moment a commit leaves + * no sync lanes pending, and increments it otherwise. Reading `root.pendingLanes` + * at commit time reproduces that reset exactly. + * + * Known over-count: React also requires the committed lanes to intersect its own + * mask, which we cannot read from the devtools callback. Dropping that term can + * only make us count a commit React would have skipped, so we fire early, never + * late — the safe direction for a diagnostic that must beat the throw. + * + * Out of scope by construction: a passive-effect (useEffect) loop reports + * pendingLanes 0 here, because passive effects flush after the commit callback. + * That is correct — React tracks those in nestedPassiveUpdateCount, which only + * console.errors in development and never throws #185. + */ +import { compactBreadcrumbData } from '@/lib/crash-breadcrumb-data' +import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' +import { + armReactCommitCascadeWriteSampling, + readReactCommitCascadeWriteSummary, + resetReactCommitCascadeWriteSamples +} from '@/lib/react-commit-cascade-store-write-samples' +import { readStoreListenerCount } from '@/store/store-listener-census' +import type { RendererSurface } from '@/lib/renderer-memory-sampling' +import { REACT_NESTED_UPDATE_LIMIT } from '../../../shared/react-update-depth-attribution' + +export const REACT_COMMIT_CASCADE_BREADCRUMB = 'react_commit_cascade' + +/** + * SyncLane | InputContinuousLane | DefaultLane. A root still holding any of + * these after a commit is what React counts as a nested update. + */ +export const REACT_CASCADING_LANES = 42 + +/** Headroom for the crumb to be built and sent before React throws above 50. */ +const REACT_COMMIT_CASCADE_NOTICE_HEADROOM = 10 +export const REACT_COMMIT_CASCADE_NOTICE_LIMIT = + REACT_NESTED_UPDATE_LIMIT - REACT_COMMIT_CASCADE_NOTICE_HEADROOM +/** + * Nothing in the app legitimately reaches 20 consecutive same-root cascading + * commits — the two deliberate oscillation loops are capped at 8 and 3 — and it + * leaves 20 commits for the sampled writes to land inside the loop. + */ +export const REACT_COMMIT_CASCADE_ARM_COMMITS = 20 +/** Matches RENDERER_BREADCRUMB_COALESCE_MS; main drops anything faster anyway. */ +export const REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS = 30_000 + +export type ReactCommitCascadeState = { + /** + * Held strongly, and never dereferenced. The next commit that leaves no + * cascading lanes clears the slot — milliseconds away in a live renderer — + * so the only tree this pins is a React root unmounted mid-cascade, such as + * the per-decoration roots in useDiffCommentDecorator. + */ + cascadeRoot: unknown + commits: number + reported: boolean + armedAtMs: number | null + lastReportedAtMs: number | null + /** Cascades that reached the notice limit while the report interval was live. */ + suppressed: number +} + +export function createReactCommitCascadeState(): ReactCommitCascadeState { + return { + cascadeRoot: null, + commits: 0, + reported: false, + armedAtMs: null, + lastReportedAtMs: null, + suppressed: 0 + } +} + +const sharedState = createReactCommitCascadeState() +let rendererSurface: RendererSurface = 'main' + +export function setReactCommitCascadeRendererSurface(surface: RendererSurface): void { + rendererSurface = surface +} + +export function resetReactCommitCascadeTelemetryForTests(): void { + Object.assign(sharedState, createReactCommitCascadeState()) + rendererSurface = 'main' + resetReactCommitCascadeWriteSamples() +} + +function endCascade(state: ReactCommitCascadeState): void { + state.cascadeRoot = null + state.commits = 0 + state.reported = false + state.armedAtMs = null + resetReactCommitCascadeWriteSamples() +} + +function reportCascade(state: ReactCommitCascadeState, pendingLanes: number, nowMs: number): void { + const suppressed = state.suppressed + state.suppressed = 0 + state.lastReportedAtMs = nowMs + const writes = readReactCommitCascadeWriteSummary() + recordRendererCrashBreadcrumb( + REACT_COMMIT_CASCADE_BREADCRUMB, + compactBreadcrumbData({ + commits: state.commits, + commitBudget: REACT_NESTED_UPDATE_LIMIT, + elapsedMs: state.armedAtMs === null ? undefined : nowMs - state.armedAtMs, + pendingLanes, + // Why 0 is worth shipping: it says the loop is useState-driven, not store-driven. + storeWrites: writes.storeWrites, + storeWriteSites: writes.storeWriteSites, + driverFrame: writes.driverFrame, + driverStack: writes.driverStack, + changedKeys: writes.changedKeys, + storeListeners: readStoreListenerCount() ?? undefined, + rendererSurface, + suppressed: suppressed > 0 ? suppressed : undefined + }) + ) +} + +function recordCommit( + state: ReactCommitCascadeState, + root: unknown, + pendingLanes: number, + readNowMs: () => number, + noticeLimit: number, + armCommits: number, + minReportIntervalMs: number +): void { + if ((pendingLanes & REACT_CASCADING_LANES) === 0) { + if (state.cascadeRoot !== null) { + endCascade(state) + } + return + } + if (root !== state.cascadeRoot) { + endCascade(state) + state.cascadeRoot = root + } + + state.commits += 1 + if (state.commits < armCommits) { + return + } + if (state.commits === armCommits) { + state.armedAtMs = readNowMs() + armReactCommitCascadeWriteSampling() + return + } + if (state.reported || state.commits < noticeLimit) { + return + } + + state.reported = true + const nowMs = readNowMs() + // Why the renderer throttles too: nothing rate-limits this pipe, and every + // call is a structured clone plus an ipcRenderer.send. + const sinceReportMs = state.lastReportedAtMs === null ? null : nowMs - state.lastReportedAtMs + if (sinceReportMs !== null && sinceReportMs >= 0 && sinceReportMs < minReportIntervalMs) { + state.suppressed += 1 + return + } + reportCascade(state, pendingLanes, nowMs) +} + +/** Injectable entry point; production uses observeReactCommit. */ +export function recordReactCommit(args: { + state: ReactCommitCascadeState + root: unknown + pendingLanes: number + readNowMs: () => number + noticeLimit?: number + armCommits?: number + minReportIntervalMs?: number +}): void { + recordCommit( + args.state, + args.root, + args.pendingLanes, + args.readNowMs, + args.noticeLimit ?? REACT_COMMIT_CASCADE_NOTICE_LIMIT, + args.armCommits ?? REACT_COMMIT_CASCADE_ARM_COMMITS, + args.minReportIntervalMs ?? REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS + ) +} + +/** + * Per-commit hot path: one property read, one mask, two compares, one + * increment. No clock read and no allocation until a cascade arms. + */ +export function observeReactCommit(root: unknown, pendingLanes: number): void { + recordCommit( + sharedState, + root, + pendingLanes, + Date.now, + REACT_COMMIT_CASCADE_NOTICE_LIMIT, + REACT_COMMIT_CASCADE_ARM_COMMITS, + REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS + ) +} diff --git a/src/renderer/src/lib/react-devtools-commit-hook-shim.ts b/src/renderer/src/lib/react-devtools-commit-hook-shim.ts new file mode 100644 index 00000000000..7afde839020 --- /dev/null +++ b/src/renderer/src/lib/react-devtools-commit-hook-shim.ts @@ -0,0 +1,70 @@ +/** + * Guarantees __REACT_DEVTOOLS_GLOBAL_HOOK__ exists, and nothing else. + * + * Split out from the observer because the two halves have different deadlines: + * react-dom reads this global ONCE, at its own module evaluation, so a fresh + * shim must beat that import — but wrapping `onCommitFiberRoot` can happen at + * any time, since react-dom re-reads the property at every commit. This module + * therefore installs itself on import and has NO imports of its own, so a bare + * side-effect import cannot be outrun by a transitive graph that reaches + * react-dom, and cannot drag the breadcrumb recorder in ahead of any vi.mock. + */ + +/** + * Only what react-dom and react-refresh require. onCommitFiberUnmount, + * onPostCommitFiberRoot and setStrictMode are deliberately absent: react-dom + * guards each with a typeof check, and calls onCommitFiberUnmount once per + * DELETED FIBER, which is genuinely hot when terminal panes unmount. + */ +export type ReactDevtoolsCommitHook = { + isDisabled?: boolean + supportsFiber?: boolean + renderers?: Map + inject?: (renderer: unknown) => number + onCommitFiberRoot?: ( + rendererId: number, + root: unknown, + priorityLevel: unknown, + didError: boolean + ) => void +} + +// Indexed, not a global augmentation: bippy (a react-grab dependency) already +// declares this global with a narrower callback signature. +type GlobalWithReactDevtoolsHook = Record + +function createCommitHookShim(): ReactDevtoolsCommitHook { + const renderers = new Map() + return { + isDisabled: false, + supportsFiber: true, + renderers, + inject: (renderer) => { + const rendererId = renderers.size + 1 + renderers.set(rendererId, renderer) + return rendererId + }, + // Why a noop and not undefined: react-refresh captures this property and + // calls oldOnCommitFiberRoot.apply(this, arguments) with no typeof guard. + onCommitFiberRoot: () => {} + } +} + +/** Returns the existing hook untouched, or installs a minimal one. */ +export function ensureReactDevtoolsCommitHook(): ReactDevtoolsCommitHook | undefined { + try { + const globalWithHook = globalThis as unknown as GlobalWithReactDevtoolsHook + const existing = globalWithHook.__REACT_DEVTOOLS_GLOBAL_HOOK__ + if (existing) { + return existing + } + const hook = createCommitHookShim() + globalWithHook.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook + return hook + } catch { + // A renderer without a patchable global still has to boot. + return undefined + } +} + +ensureReactDevtoolsCommitHook() diff --git a/src/renderer/src/main.tsx b/src/renderer/src/main.tsx index cff89bd6b38..825ff536b6f 100644 --- a/src/renderer/src/main.tsx +++ b/src/renderer/src/main.tsx @@ -1,3 +1,9 @@ +// Why first, and why the import-free shim: react-dom reads +// __REACT_DEVTOOLS_GLOBAL_HOOK__ once at module evaluation, so the global has to +// exist before it. The observer below only wraps a property react-dom re-reads +// per commit, so its own import graph can evaluate whenever it likes. +import './lib/react-devtools-commit-hook-shim' +import './lib/react-commit-cascade-observer' import './assets/main.css' import { StrictMode } from 'react' diff --git a/src/renderer/src/popout.tsx b/src/renderer/src/popout.tsx index 00470df1374..3c84112d1a3 100644 --- a/src/renderer/src/popout.tsx +++ b/src/renderer/src/popout.tsx @@ -1,3 +1,9 @@ +// Why first, and why the import-free shim: react-dom reads +// __REACT_DEVTOOLS_GLOBAL_HOOK__ once at module evaluation, so the global has to +// exist before it. The observer below only wraps a property react-dom re-reads +// per commit, so its own import graph can evaluate whenever it likes. +import './lib/react-devtools-commit-hook-shim' +import './lib/react-commit-cascade-observer' import './assets/main.css' import { StrictMode, useEffect } from 'react' @@ -15,6 +21,7 @@ import { translate } from './i18n/i18n' import { useAppStore } from './store' import type { GlobalSettings } from '../../shared/global-settings-types' import { getOrCreateRendererRoot } from './lib/react-renderer-root' +import { setReactCommitCascadeRendererSurface } from './lib/react-commit-cascade-telemetry' // Why: the pop-out window is a separate BrowserWindow with its own React root, // so it must run the same renderer bootstrap as main.tsx (crash diagnostics, @@ -22,6 +29,7 @@ import { getOrCreateRendererRoot } from './lib/react-renderer-root' // window. It shares the preload/window.api but not the DOM or JS context. recordRendererCrashBreadcrumb('popout_bootstrap_started', { dev: import.meta.env.DEV }) installRendererCrashDiagnostics('dashboard-popout') +setReactCommitCascadeRendererSurface('dashboard-popout') function applyPopoutAppearance(settings: GlobalSettings | null): void { applyDocumentTheme(settings?.theme ?? 'system', { disableTransitions: false }) diff --git a/src/renderer/src/store/index.ts b/src/renderer/src/store/index.ts index 3fd5b308585..35bc1845736 100644 --- a/src/renderer/src/store/index.ts +++ b/src/renderer/src/store/index.ts @@ -52,61 +52,64 @@ import { registerRuntimeHttpLinkBrowserOpener } from '@/lib/http-link-routing' import { installStoreListenerCensus } from './store-listener-census' +import { withReactCommitCascadeWriteProbe } from './react-commit-cascade-write-probe' import { registerRendererMemoryProfileContributor, summarizeStateCollectionSizes } from '@/lib/renderer-memory-profile' import { estimateStateCollectionKB } from '@/lib/state-collection-byte-estimate' -export const useAppStore = create()((...a) => { - // Why: the inner api is only reachable here, before create() copies subscribe onto the hook. - installStoreListenerCensus(a[2]) - return { - ...createRepoSlice(...a), - ...createSparsePresetsSlice(...a), - ...createWorktreeSlice(...a), - ...createTerminalSlice(...a), - ...createTabsSlice(...a), - ...createUISlice(...a), - ...createSettingsSlice(...a), - ...createKeybindingsSlice(...a), - ...createGitHubSlice(...a), - ...createHostedReviewSlice(...a), - ...createLinearSlice(...a), - ...createPreflightSlice(...a), - ...createJiraSlice(...a), - ...createEditorSlice(...a), - ...createStatsSlice(...a), - ...createMemorySlice(...a), - ...createWorkspaceSpaceSlice(...a), - ...createClaudeUsageSlice(...a), - ...createCodexUsageSlice(...a), - ...createOpenCodeUsageSlice(...a), - ...createBrowserSlice(...a), - ...createRateLimitSlice(...a), - ...createSshSlice(...a), - ...createRuntimeEnvironmentSshSlice(...a), - ...createAgentStatusSlice(...a), - ...createPaneForegroundAgentSlice(...a), - ...createDiffCommentsSlice(...a), - ...createDetectedAgentsSlice(...a), - ...createRuntimeDetectedAgentsSlice(...a), - ...createWorktreeNavHistorySlice(...a), - ...createDictationSlice(...a), - ...createWorkspaceCleanupSlice(...a), - ...createWorkspaceCleanupBrowseSlice(...a), - ...createRuntimeStatusSlice(...a), - ...createPullRequestGenerationSlice(...a), - ...createCommitMessageGenerationSlice(...a), - ...createPinnedTabCloseConfirmSlice(...a), - ...createRecentlyClosedTabsSlice(...a), - ...createOrcaProfilesSlice(...a), - ...createNewIssueDraftSlice(...a), - ...createTaskCreationDraftsSlice(...a), - ...createRemoteServerUpdatesSlice(...a), - ...createTerminalQuickCommandHostsSlice(...a) - } -}) +export const useAppStore = create()( + withReactCommitCascadeWriteProbe((...a) => { + // Why: the inner api is only reachable here, before create() copies subscribe onto the hook. + installStoreListenerCensus(a[2]) + return { + ...createRepoSlice(...a), + ...createSparsePresetsSlice(...a), + ...createWorktreeSlice(...a), + ...createTerminalSlice(...a), + ...createTabsSlice(...a), + ...createUISlice(...a), + ...createSettingsSlice(...a), + ...createKeybindingsSlice(...a), + ...createGitHubSlice(...a), + ...createHostedReviewSlice(...a), + ...createLinearSlice(...a), + ...createPreflightSlice(...a), + ...createJiraSlice(...a), + ...createEditorSlice(...a), + ...createStatsSlice(...a), + ...createMemorySlice(...a), + ...createWorkspaceSpaceSlice(...a), + ...createClaudeUsageSlice(...a), + ...createCodexUsageSlice(...a), + ...createOpenCodeUsageSlice(...a), + ...createBrowserSlice(...a), + ...createRateLimitSlice(...a), + ...createSshSlice(...a), + ...createRuntimeEnvironmentSshSlice(...a), + ...createAgentStatusSlice(...a), + ...createPaneForegroundAgentSlice(...a), + ...createDiffCommentsSlice(...a), + ...createDetectedAgentsSlice(...a), + ...createRuntimeDetectedAgentsSlice(...a), + ...createWorktreeNavHistorySlice(...a), + ...createDictationSlice(...a), + ...createWorkspaceCleanupSlice(...a), + ...createWorkspaceCleanupBrowseSlice(...a), + ...createRuntimeStatusSlice(...a), + ...createPullRequestGenerationSlice(...a), + ...createCommitMessageGenerationSlice(...a), + ...createPinnedTabCloseConfirmSlice(...a), + ...createRecentlyClosedTabsSlice(...a), + ...createOrcaProfilesSlice(...a), + ...createNewIssueDraftSlice(...a), + ...createTaskCreationDraftsSlice(...a), + ...createRemoteServerUpdatesSlice(...a), + ...createTerminalQuickCommandHostsSlice(...a) + } + }) +) registerHttpLinkStoreAccessor(() => useAppStore.getState()) registerRuntimeHttpLinkBrowserOpener(async (request) => { diff --git a/src/renderer/src/store/react-commit-cascade-write-probe.test.ts b/src/renderer/src/store/react-commit-cascade-write-probe.test.ts new file mode 100644 index 00000000000..0c18706e976 --- /dev/null +++ b/src/renderer/src/store/react-commit-cascade-write-probe.test.ts @@ -0,0 +1,73 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { create, type StoreApi, type UseBoundStore } from 'zustand' +import { withReactCommitCascadeWriteProbe } from './react-commit-cascade-write-probe' + +const { probe, noteWrite } = vi.hoisted(() => ({ + probe: { armed: false }, + noteWrite: vi.fn() +})) +vi.mock('@/lib/react-commit-cascade-store-write-samples', () => ({ + reactCommitCascadeWriteProbe: probe, + noteReactCommitCascadeStoreWrite: (boundary: object, partial: unknown) => + noteWrite(boundary, partial) +})) + +type ProbedState = { ticks: number; label?: string } + +function createProbedStore(): UseBoundStore> { + return create()( + withReactCommitCascadeWriteProbe(() => ({ ticks: 0, label: 'initial' })) + ) +} + +beforeEach(() => { + probe.armed = false + noteWrite.mockReset() +}) + +describe('withReactCommitCascadeWriteProbe', () => { + // Why this matters most: this wraps every write the app makes. A diagnostic + // that throws here would drop the write and take app state with it. + it('lands the write even when the sampler throws', () => { + noteWrite.mockImplementation(() => { + throw new Error('sampler exploded') + }) + const useProbedStore = createProbedStore() + probe.armed = true + + expect(() => useProbedStore.setState({ ticks: 3 })).not.toThrow() + expect(useProbedStore.getState().ticks).toBe(3) + }) + + // Why asserted: create() copies api.setState onto the hook AFTER the creator + // runs, so a middleware that forgets this line leaves store.setState unprobed. + it('routes the api setState through the probe', () => { + const useProbedStore = createProbedStore() + probe.armed = true + + useProbedStore.setState({ ticks: 1 }) + + expect(noteWrite).toHaveBeenCalledTimes(1) + // The boundary must be the wrapper itself, so V8 elides it from the sample. + expect(noteWrite.mock.calls[0]?.[0]).toBe(useProbedStore.setState) + expect(noteWrite.mock.calls[0]?.[1]).toEqual({ ticks: 1 }) + }) + + it('passes the replace flag through', () => { + const useProbedStore = createProbedStore() + probe.armed = true + + useProbedStore.setState({ ticks: 2 }, true) + + expect(useProbedStore.getState()).toEqual({ ticks: 2 }) + }) + + it('costs nothing but a flag read while unarmed', () => { + const useProbedStore = createProbedStore() + + useProbedStore.setState({ ticks: 4 }) + + expect(noteWrite).not.toHaveBeenCalled() + expect(useProbedStore.getState().ticks).toBe(4) + }) +}) diff --git a/src/renderer/src/store/react-commit-cascade-write-probe.ts b/src/renderer/src/store/react-commit-cascade-write-probe.ts new file mode 100644 index 00000000000..229c46dcb0c --- /dev/null +++ b/src/renderer/src/store/react-commit-cascade-write-probe.ts @@ -0,0 +1,37 @@ +/** + * zustand middleware that samples store writes during a suspected React commit + * cascade, so the crash report names the code driving it. + * + * Why middleware and not a setState patch: create() hands slices the `set` + * closure built before `api` exists (zustand/esm/vanilla.mjs), so patching + * useAppStore.setState afterwards misses every slice-internal set() — and the + * slices are where the writes are. Wrapping the creator catches both, the same + * way store-listener-census has to reach the inner api to see hook subscribers. + * + * Cost when no cascade is suspected: one boolean field load per write. + */ +import { + noteReactCommitCascadeStoreWrite, + reactCommitCascadeWriteProbe +} from '@/lib/react-commit-cascade-store-write-samples' +import type { StateCreator } from 'zustand' + +export function withReactCommitCascadeWriteProbe( + createState: StateCreator +): StateCreator { + return (set, get, api) => { + const wrapped = ((partial: unknown, replace?: unknown): void => { + if (reactCommitCascadeWriteProbe.armed) { + try { + noteReactCommitCascadeStoreWrite(wrapped, partial) + } catch { + // This is the app's universal write path: a diagnostic that throws here + // would drop the write itself. + } + } + ;(set as (nextPartial: unknown, nextReplace?: unknown) => void)(partial, replace) + }) as typeof set + api.setState = wrapped as typeof api.setState + return createState(wrapped, get, api) + } +} diff --git a/src/shared/react-update-depth-attribution.ts b/src/shared/react-update-depth-attribution.ts index 7d79a1d36ad..98b45d478bd 100644 --- a/src/shared/react-update-depth-attribution.ts +++ b/src/shared/react-update-depth-attribution.ts @@ -4,6 +4,13 @@ * next after the counter trips. The catching boundary is a bystander picked by commit order. */ +/** + * react-dom 19.2.x NESTED_UPDATE_LIMIT. getRootForUpdatedFiber throws once the + * root-global nested-update counter passes it. Every diagnostic that budgets + * commits against React's bail must derive its threshold from this one number. + */ +export const REACT_NESTED_UPDATE_LIMIT = 50 + export const UNRELIABLE_BOUNDARY_ATTRIBUTION = 'unreliable' export type CrashReportAttribution = typeof UNRELIABLE_BOUNDARY_ATTRIBUTION diff --git a/src/shared/runtime-client-export-parity.test.ts b/src/shared/runtime-client-export-parity.test.ts index ad3d9708eec..2b86d2e6e99 100644 --- a/src/shared/runtime-client-export-parity.test.ts +++ b/src/shared/runtime-client-export-parity.test.ts @@ -158,6 +158,7 @@ type RuntimeTypeInventory = [ Runtime.RuntimeTerminalSplit, Runtime.RuntimeTerminalState, Runtime.RuntimeTerminalSummary, + Runtime.RuntimeTerminalUnavailableReason, Runtime.RuntimeTerminalVisualGroupNode, Runtime.RuntimeTerminalVisualLayout, Runtime.RuntimeTerminalVisualLayoutNode, @@ -225,8 +226,11 @@ describe('runtime client public export parity', () => { 'BROWSER_UNAVAILABLE_ERROR_CODE', 'COMPUTER_ERROR_CODES', 'HEADLESS_RUNTIME_WINDOW_ID', + 'TERMINAL_PTY_DEGRADATION_CAPABILITY', + 'TERMINAL_UNAVAILABLE_ERROR_CODE', 'UNPUBLISHED_WORKTREE_PUBLICATION_EPOCH', - 'browserUnavailableMessage' + 'browserUnavailableMessage', + 'terminalUnavailableMessage' ]) expect(Object.keys(RemoteClient).sort()).toEqual([ 'RemoteRuntimeClientError', diff --git a/tests/e2e/browser-tab.spec.ts b/tests/e2e/browser-tab.spec.ts index 4988b76ccc2..00075b4af19 100644 --- a/tests/e2e/browser-tab.spec.ts +++ b/tests/e2e/browser-tab.spec.ts @@ -680,7 +680,7 @@ test.describe('Browser Tab', () => { } }) - test('plain links stay current while explicit new-tab gestures activate Orca tabs', async ({ + test('every new-tab link gesture activates an Orca tab and never a native window', async ({ electronApp, orcaPage }) => { @@ -698,23 +698,20 @@ test.describe('Browser Tab', () => { const baseWindowCount = await electronApp.evaluate( ({ BaseWindow }) => BaseWindow.getAllWindows().length ) - const baseTabCount = await orcaPage.locator('[data-tab-id]').count() - await clickBrowserLink(orcaPage, sourceTab!.id, '#external-link') - + // A plain target=_blank click is a new-tab request, in the main frame and in an iframe; + // the source tab must stay put rather than navigate away under it. const sourceTabLocator = orcaPage.locator(`[data-tab-id="${sourceTab!.id}"]`) - await expect(sourceTabLocator).toContainText('Linked destination', { timeout: 10_000 }) - await expect(orcaPage.locator('[data-tab-id]')).toHaveCount(baseTabCount) + await clickBrowserLink(orcaPage, sourceTab!.id, '#external-link') + await expectBrowserTabActive(orcaPage, 'Linked destination') + await expect(sourceTabLocator).toContainText('Source page') + await switchToBrowserTab(orcaPage, worktreeId, sourceTab!.id) - await clickBrowserLink(orcaPage, sourceTab!.id, '#return-link') - await expect(sourceTabLocator).toContainText('Source page', { timeout: 10_000 }) await clickBrowserLink(orcaPage, sourceTab!.id, '#frame-link', { frameSelector: '#link-frame' }) - await expect(sourceTabLocator).toContainText('Frame destination', { timeout: 10_000 }) - await expect(orcaPage.locator('[data-tab-id]')).toHaveCount(baseTabCount) - - await clickBrowserLink(orcaPage, sourceTab!.id, '#return-link') - await expect(sourceTabLocator).toContainText('Source page', { timeout: 10_000 }) + await expectBrowserTabActive(orcaPage, 'Frame destination') + await expect(sourceTabLocator).toContainText('Source page') + await switchToBrowserTab(orcaPage, worktreeId, sourceTab!.id) await clickBrowserLink(orcaPage, sourceTab!.id, '#frame-modifier-link', { frameSelector: '#link-frame',