From 7558fb064aca9ee76b2bfd6fffdceeee657d9005 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:13:12 -0700 Subject: [PATCH] fix(browser): keep browser guests painting when the workbench is hidden (#14599) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(browser): keep browser guests painting when the workbench is hidden Chromium never paints inside a display:none subtree, so an Electron stops emitting CDP screencast frames the moment any ancestor is parked that way. Orca already models this per pane (browser-page-paintability.ts) and per worktree surface, using opacity:0 so a phone- or agent-driven page keeps compositing — but three ancestors above those layers still used `hidden` unconditionally: - the App-level terminal workbench container, hidden whenever activeView is not 'terminal' (opening Settings froze every mobile browser pane), - Terminal's root, hidden when there is no active worktree, - the split-surface wrapper, hidden when the active worktree has no layout. A pane-level escape hatch cannot override an ancestor, so all of them have to agree. Share one predicate across the chain and swap `hidden` for an out-of-flow transparent layer while a remote controller needs frames. The predicate ORs automation visibility with the mobile driver, matching the per-worktree gate. That term is load-bearing, not symmetry: agent-browser commands acquire a visibility lease and then capture, so gating on the mobile driver alone left automation from a non-workspace view capturing a blank surface. Mobile: a stream can report `ready` and then deliver no frames, which cleared the loading indicator and left an unexplained black rectangle. Key it off actually having pixels. That also retires the `ready` state and its ref. Co-authored-by: Kaylee Williams <65376239+KayleeWilliams@users.noreply.github.com> * fix(browser): keep paint retention off store hot paths --------- Co-authored-by: Kaylee Williams <65376239+KayleeWilliams@users.noreply.github.com> --- mobile/src/browser/MobileBrowserPane.tsx | 26 +-- .../mobile-browser-frameless-stream.test.tsx | 154 ++++++++++++++++++ .../src/app-shell/AppWorkspaceShell.tsx | 11 +- src/renderer/src/components/Terminal.tsx | 36 ++-- .../TerminalWorkbenchContainer.test.tsx | 105 ++++++++++++ .../components/TerminalWorkbenchContainer.tsx | 34 ++++ .../browser-guest-paint-retention.test.ts | 46 ++++++ .../browser-guest-paint-retention.ts | 62 +++++++ 8 files changed, 433 insertions(+), 41 deletions(-) create mode 100644 mobile/src/browser/mobile-browser-frameless-stream.test.tsx create mode 100644 src/renderer/src/components/TerminalWorkbenchContainer.test.tsx create mode 100644 src/renderer/src/components/TerminalWorkbenchContainer.tsx create mode 100644 src/renderer/src/components/browser-pane/browser-guest-paint-retention.test.ts create mode 100644 src/renderer/src/components/browser-pane/browser-guest-paint-retention.ts diff --git a/mobile/src/browser/MobileBrowserPane.tsx b/mobile/src/browser/MobileBrowserPane.tsx index 881c1416adb..6397f69c549 100644 --- a/mobile/src/browser/MobileBrowserPane.tsx +++ b/mobile/src/browser/MobileBrowserPane.tsx @@ -155,7 +155,6 @@ export function MobileBrowserPane({ const [frameMetadata, setFrameMetadata] = useState( cachedInitialFrame?.metadata ?? null ) - const [ready, setReady] = useState(cachedInitialFrame !== null) const [busy, setBusy] = useState(false) const [error, setError] = useState(null) const [dialog, setDialog] = useState(null) @@ -174,7 +173,6 @@ export function MobileBrowserPane({ const browserLayerRefs = useRef<[View | null, View | null]>([null, null]) const pendingFrameLayerRef = useRef(null) const visibleFrameLayerRef = useRef(0) - const readyRef = useRef(cachedInitialFrame !== null) const busyRef = useRef(false) const lastAppliedFrameAtRef = useRef(0) const pendingThrottledFrameRef = useRef<{ @@ -309,10 +307,6 @@ export function MobileBrowserPane({ busyRef.current = false setBusy(false) } - if (!readyRef.current) { - readyRef.current = true - setReady(true) - } }, []) const clearFrameThrottle = useCallback(() => { @@ -400,16 +394,12 @@ export function MobileBrowserPane({ frameMetadataRef.current = cachedFrame.metadata setFrameUri(cachedFrame.uri) setFrameMetadata(cachedFrame.metadata) - readyRef.current = true - setReady(true) } else { frameUriRef.current = null frameMountedRef.current = false setFrameUri(null) setFrameMetadata(null) frameMetadataRef.current = null - readyRef.current = false - setReady(false) } } else { frameMountedRef.current = true @@ -478,10 +468,6 @@ export function MobileBrowserPane({ } if (event.type === 'ready') { clearStartupTimer() - if (!readyRef.current) { - readyRef.current = true - setReady(true) - } if (busyRef.current) { busyRef.current = false setBusy(false) @@ -495,10 +481,6 @@ export function MobileBrowserPane({ } } else if (event.type === 'end') { clearStartupTimer() - if (readyRef.current) { - readyRef.current = false - setReady(false) - } if (busyRef.current) { busyRef.current = false setBusy(false) @@ -518,10 +500,6 @@ export function MobileBrowserPane({ } const message = event.message ?? event.error?.message ?? 'Browser stream failed.' if (shouldSurfaceBrowserError(message)) { - if (readyRef.current) { - readyRef.current = false - setReady(false) - } setError(message) } } @@ -1209,7 +1187,9 @@ export function MobileBrowserPane({ ) : null} {!renderedFrameSource || busy || error ? ( - {busy || (!ready && !error) ? ( + {/* Why: a stream can report ready and then deliver no frames, so key the + indicator off actually having pixels or it clears into a blank pane. */} + {busy || (!renderedFrameSource && !error) ? ( ) : null} {error ? {error} : null} diff --git a/mobile/src/browser/mobile-browser-frameless-stream.test.tsx b/mobile/src/browser/mobile-browser-frameless-stream.test.tsx new file mode 100644 index 00000000000..bf486d3c5c0 --- /dev/null +++ b/mobile/src/browser/mobile-browser-frameless-stream.test.tsx @@ -0,0 +1,154 @@ +import { Buffer } from 'buffer' +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { describe, expect, it, vi } from 'vitest' +import { + BrowserScreencastOpcode, + type BrowserScreencastFrame +} from '../transport/browser-screencast-protocol' +import type { RpcClient } from '../transport/rpc-client' +import { MobileBrowserPane, type MobileBrowserTab } from './MobileBrowserPane' + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + AppState: { currentState: 'active', addEventListener: () => ({ remove: () => {} }) }, + Image: 'Image', + PanResponder: { create: () => ({ panHandlers: {} }) }, + PixelRatio: { get: () => 2 }, + Platform: { OS: 'android' }, + Pressable: 'Pressable', + StyleSheet: { + absoluteFillObject: { position: 'absolute', top: 0, left: 0, right: 0, bottom: 0 }, + create: (styles: unknown) => styles + }, + Text: 'Text', + TextInput: 'TextInput', + View: 'View' +})) + +// Why: covers icons reached transitively too (the view-mode switch), not just the pane's own +// imports — vitest throws on the first unmocked export rather than rendering without it. +vi.mock('lucide-react-native', () => ({ + ArrowUp: 'ArrowUp', + ChevronLeft: 'ChevronLeft', + ChevronRight: 'ChevronRight', + Monitor: 'Monitor', + RefreshCw: 'RefreshCw', + Smartphone: 'Smartphone' +})) + +type Subscription = { + listener: (payload: unknown) => void + onBinaryFrame?: (frame: BrowserScreencastFrame) => void +} + +let pageCounter = 0 + +function makeFrame(): BrowserScreencastFrame { + return { + opcode: BrowserScreencastOpcode.Frame, + seq: 1, + format: 'jpeg', + metadata: { deviceWidth: 360, deviceHeight: 640, pageScaleFactor: 1 }, + image: new TextEncoder().encode('frame') + } +} + +function spinnerCount(renderer: ReactTestRenderer): number { + return renderer.root.findAllByType('ActivityIndicator').length +} + +async function renderPane(): Promise<{ renderer: ReactTestRenderer; stream: Subscription }> { + pageCounter += 1 + const subscriptions: Subscription[] = [] + const client = { + subscribe: ( + _method: string, + _params: unknown, + listener: (payload: unknown) => void, + options?: { onBinaryFrame?: (frame: BrowserScreencastFrame) => void } + ) => { + subscriptions.push({ listener, onBinaryFrame: options?.onBinaryFrame }) + return () => {} + }, + request: vi.fn() + } as unknown as RpcClient + + const tab: MobileBrowserTab = { + type: 'browser', + id: `tab-${pageCounter}`, + title: 'Dashboard', + browserWorkspaceId: 'bw-1', + browserPageId: `page-${pageCounter}`, + url: 'https://dashboard.example', + loading: false, + canGoBack: false, + canGoForward: false, + isActive: true + } + + let renderer: ReactTestRenderer + await act(async () => { + renderer = create( + createElement(MobileBrowserPane, { + client, + // Why: unique worktree id keeps each test on a cold module-level frame cache. + worktreeId: `wt-${pageCounter}`, + tab, + screencastSupported: true, + keyboardLift: 0, + bottomInset: 0, + onToast: () => {} + }), + { createNodeMock: () => ({ setNativeProps: () => {} }) } + ) + await Promise.resolve() + }) + const mounted: ReactTestRenderer = renderer + const viewport = mounted.root + .findAllByType('View') + .find((node) => typeof node.props.onLayout === 'function') + if (!viewport) { + throw new Error('Viewport with onLayout not found') + } + act(() => { + viewport.props.onLayout({ nativeEvent: { layout: { width: 360, height: 640 } } }) + }) + const stream = subscriptions[0] + if (!stream) { + throw new Error('browser.screencast subscription not created') + } + return { renderer: mounted, stream } +} + +describe('MobileBrowserPane with a stream that reports ready but sends no frames', () => { + // Why: a host that stops painting still reports `ready`, so the pane used to clear its + // indicator and leave an unexplained black rectangle. + it('keeps showing the loading indicator instead of an empty black pane', async () => { + const { renderer, stream } = await renderPane() + + act(() => { + stream.listener({ type: 'ready', tab: { url: 'https://dashboard.example' } }) + }) + + expect(spinnerCount(renderer)).toBeGreaterThan(0) + }) + + it('clears the indicator once real pixels arrive', async () => { + const { renderer, stream } = await renderPane() + + act(() => { + stream.listener({ type: 'ready', tab: { url: 'https://dashboard.example' } }) + }) + act(() => { + stream.onBinaryFrame?.(makeFrame()) + }) + + expect(spinnerCount(renderer)).toBe(0) + const source = renderer.root + .findAllByType('Image') + .map((image) => (image.props.source as { uri?: string } | null)?.uri) + .find((uri) => typeof uri === 'string') + expect(source).toContain(Buffer.from(makeFrame().image).toString('base64')) + }) +}) diff --git a/src/renderer/src/app-shell/AppWorkspaceShell.tsx b/src/renderer/src/app-shell/AppWorkspaceShell.tsx index 377386d6bbb..0d53810e7fa 100644 --- a/src/renderer/src/app-shell/AppWorkspaceShell.tsx +++ b/src/renderer/src/app-shell/AppWorkspaceShell.tsx @@ -5,6 +5,7 @@ import Sidebar from '../components/Sidebar' import RightSidebar from '../components/right-sidebar' import { RecoverableRenderErrorBoundary } from '../components/error-boundaries/RecoverableRenderErrorBoundary' import { FloatingTerminalToggleButton } from '../components/floating-terminal/FloatingTerminalToggleButton' +import { TerminalWorkbenchContainer } from '../components/TerminalWorkbenchContainer' import type { VirtualizedScrollAnchor } from '../hooks/useVirtualizedScrollAnchor' import { TitlebarLeftControls } from './TitlebarLeftControls' import { RightSidebarToggle, TitlebarMainStrip } from './TitlebarMainStrip' @@ -174,13 +175,7 @@ export function AppWorkspaceShell(props: { )}
{layout.shouldMountTerminalWorkbench ? ( -
+ -
+ ) : null} , so a remote controller needs + // them to drop `hidden` too — the per-worktree surface hatch cannot override an ancestor. + const retainBrowserGuestPaint = useAnyBrowserGuestNeedsPaint( + !renderedActiveWorktreeId || !effectiveActiveLayout + ) const activeWorktreeBrowserTabIdsKey = renderedActiveWorktreeId ? (browserTabsByWorktree[renderedActiveWorktreeId] ?? []).map((tab) => tab.id).join(',') : '' @@ -2411,7 +2419,15 @@ function Terminal(): React.JSX.Element | null { return (
@@ -2478,7 +2494,13 @@ function Terminal(): React.JSX.Element | null { {anyMountedWorktreeHasLayout ? (
{/* Why: absolutely position each mounted surface so hidden trees don't reflow the active one; the relative anchor sizes panes to the workspace body. */} {workspaceSurfaces @@ -2789,13 +2811,7 @@ const WorktreeSplitSurface = React.memo(function WorktreeSplitSurface({ backgroundMountTabIds: ReadonlySet | null activationDeferredMountTabIds: ReadonlySet | null }): React.JSX.Element { - const browserPageIds = useAppStore( - useShallow((state) => - (state.browserTabsByWorktree[worktreeId] ?? []).flatMap((tab) => - tab.pageIds && tab.pageIds.length > 0 ? tab.pageIds : [tab.activePageId ?? tab.id] - ) - ) - ) + const browserPageIds = useWorktreeBrowserPageIds(worktreeId) const hasAutomationVisibleBrowser = useBrowserAutomationVisibilityForAny(browserPageIds) const hasMobileDrivenBrowser = useBrowserMobileDriverForAny(browserPageIds) const shouldKeepPaintable = diff --git a/src/renderer/src/components/TerminalWorkbenchContainer.test.tsx b/src/renderer/src/components/TerminalWorkbenchContainer.test.tsx new file mode 100644 index 00000000000..8aff4765206 --- /dev/null +++ b/src/renderer/src/components/TerminalWorkbenchContainer.test.tsx @@ -0,0 +1,105 @@ +// @vitest-environment happy-dom +import { cleanup, render } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { BrowserTab as BrowserTabState } from '../../../shared/browser-workspace-types' + +type MockAppState = { browserTabsByWorktree: Record } + +const mocks = vi.hoisted(() => ({ state: null as MockAppState | null })) + +vi.mock('../store', () => ({ + useAppStore: (selector: (state: MockAppState) => unknown) => { + if (!mocks.state) { + throw new Error('mock app state not initialized') + } + return selector(mocks.state) + } +})) + +// Why: the driver and automation-lease modules are the real ones — mocking them would leave +// the wiring under test unproven, which is the whole point of this file. +const { setDriverForBrowserPage } = await import('../lib/pane-manager/browser-mobile-driver-state') +const { acquireBrowserAutomationVisibility, releaseBrowserAutomationVisibility } = + await import('./browser-pane/browser-automation-visibility') +const { TerminalWorkbenchContainer } = await import('./TerminalWorkbenchContainer') + +const PAGE_ID = 'page-1' + +function mountWorkbench(isVisible: boolean): HTMLElement { + mocks.state = { + browserTabsByWorktree: { + 'wt-1': [{ id: 'tab-1', activePageId: PAGE_ID }] as unknown as readonly BrowserTabState[] + } + } + const { container } = render( + + workbench + + ) + const node = container.querySelector('[data-terminal-workbench-container]') + if (!(node instanceof HTMLElement)) { + throw new Error('workbench container not rendered') + } + return node +} + +afterEach(() => { + cleanup() + setDriverForBrowserPage(PAGE_ID, { kind: 'idle' }) + mocks.state = null +}) + +describe('TerminalWorkbenchContainer', () => { + it('parks with display:none when nothing remote needs the guest painting', () => { + expect(mountWorkbench(false).className).toContain('hidden') + }) + + it('renders normally on the workspace view', () => { + const node = mountWorkbench(true) + expect(node.className).not.toContain('hidden') + expect(node.className).not.toContain('opacity-0') + expect(node.hasAttribute('inert')).toBe(false) + }) + + // Why: `hidden` is display:none, and Chromium emits no screencast frames from inside such a + // subtree — this is the exact regression that froze a phone's browser pane on Settings. + it('never applies display:none while a phone drives one of its pages', () => { + setDriverForBrowserPage(PAGE_ID, { kind: 'mobile', clientId: 'client-1' }) + const node = mountWorkbench(false) + expect(node.className).not.toContain('hidden') + expect(node.className).toContain('opacity-0') + }) + + // Why: the cold-start deadlock. A screencast cannot start until the guest registers, and the + // guest only mounts under an automation bootstrap lease — gating on the mobile driver alone + // means the guest never mounts, so the driver never flips. + it('never applies display:none while an automation lease holds one of its pages', () => { + const token = acquireBrowserAutomationVisibility(PAGE_ID) + try { + const node = mountWorkbench(false) + expect(node.className).not.toContain('hidden') + expect(node.className).toContain('opacity-0') + } finally { + releaseBrowserAutomationVisibility(token) + } + }) + + it('stays out of flow and non-interactive while painting hidden', () => { + // Why: the active page is a flex sibling — an in-flow workbench would halve its height, + // and a hittable one would swallow its clicks. + setDriverForBrowserPage(PAGE_ID, { kind: 'mobile', clientId: 'client-1' }) + const node = mountWorkbench(false) + expect(node.className).toContain('absolute') + expect(node.className).toContain('pointer-events-none') + expect(node.hasAttribute('inert')).toBe(true) + expect(node.getAttribute('aria-hidden')).toBe('true') + }) + + it('re-parks once the phone stops driving the page', () => { + setDriverForBrowserPage(PAGE_ID, { kind: 'mobile', clientId: 'client-1' }) + expect(mountWorkbench(false).className).not.toContain('hidden') + cleanup() + setDriverForBrowserPage(PAGE_ID, { kind: 'idle' }) + expect(mountWorkbench(false).className).toContain('hidden') + }) +}) diff --git a/src/renderer/src/components/TerminalWorkbenchContainer.tsx b/src/renderer/src/components/TerminalWorkbenchContainer.tsx new file mode 100644 index 00000000000..a3b54fb33e8 --- /dev/null +++ b/src/renderer/src/components/TerminalWorkbenchContainer.tsx @@ -0,0 +1,34 @@ +import type React from 'react' +import { useAnyBrowserGuestNeedsPaint } from './browser-pane/browser-guest-paint-retention' + +// Why: the outermost ancestor of every browser . Parking it with `hidden` whenever +// the user leaves the workspace view also stops the guest compositing, which silently kills +// screencast frames for a phone or an agent driving that page. +export function TerminalWorkbenchContainer({ + isVisible, + children +}: { + isVisible: boolean + children: React.ReactNode +}): React.JSX.Element { + const retainBrowserGuestPaint = useAnyBrowserGuestNeedsPaint(!isVisible) + return ( +
+ {children} +
+ ) +} diff --git a/src/renderer/src/components/browser-pane/browser-guest-paint-retention.test.ts b/src/renderer/src/components/browser-pane/browser-guest-paint-retention.test.ts new file mode 100644 index 00000000000..e5518dba201 --- /dev/null +++ b/src/renderer/src/components/browser-pane/browser-guest-paint-retention.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest' +import { collectBrowserPageIds } from './browser-guest-paint-retention' + +describe('collectBrowserPageIds', () => { + it('prefers the full page list so every guest under a tab is covered', () => { + expect( + collectBrowserPageIds([ + { id: 'tab-1', activePageId: 'page-a', pageIds: ['page-a', 'page-b'] } + ]) + ).toEqual(['page-a', 'page-b']) + }) + + // Why: a split tab can hold a background page a phone is driving while a different page is + // active; collecting only the active one would let that guest get parked. + it('does not drop background pages in favour of the active one', () => { + expect( + collectBrowserPageIds([{ id: 't', activePageId: 'p1', pageIds: ['p1', 'p2'] }]) + ).toContain('p2') + }) + + it('falls back to the active page id when the list is empty', () => { + expect(collectBrowserPageIds([{ id: 'tab-1', activePageId: 'page-a', pageIds: [] }])).toEqual([ + 'page-a' + ]) + }) + + // Why: legacy single-page tabs reuse the tab id as the page id. + it('falls back to the tab id when there is no active page', () => { + expect(collectBrowserPageIds([{ id: 'tab-1' }])).toEqual(['tab-1']) + expect(collectBrowserPageIds([{ id: 'tab-1', activePageId: null }])).toEqual(['tab-1']) + }) + + it('tolerates a missing worktree entry', () => { + expect(collectBrowserPageIds(undefined)).toEqual([]) + expect(collectBrowserPageIds(null)).toEqual([]) + }) + + it('flattens across tabs', () => { + expect( + collectBrowserPageIds([ + { id: 'tab-1', pageIds: ['a'] }, + { id: 'tab-2', pageIds: ['b', 'c'] } + ]) + ).toEqual(['a', 'b', 'c']) + }) +}) diff --git a/src/renderer/src/components/browser-pane/browser-guest-paint-retention.ts b/src/renderer/src/components/browser-pane/browser-guest-paint-retention.ts new file mode 100644 index 00000000000..d2e3e8876eb --- /dev/null +++ b/src/renderer/src/components/browser-pane/browser-guest-paint-retention.ts @@ -0,0 +1,62 @@ +import { useMemo } from 'react' +import { useShallow } from 'zustand/react/shallow' +import { useAppStore } from '../../store' +import { useBrowserMobileDriverForAny } from '../../lib/pane-manager/browser-mobile-driver-state' +import { useBrowserAutomationVisibilityForAny } from './browser-automation-visibility' + +// Why: Chromium never paints inside a display:none subtree, so a browser stops +// emitting screencast frames if ANY ancestor is parked that way — the pane-level hatch in +// browser-page-paintability.ts cannot override one. Every container from the app shell down +// to the guest therefore shares this predicate; if one of them keeps using `hidden`, a phone +// or an agent driving that page silently receives no frames. + +type BrowserTabPageIdSource = { + id: string + activePageId?: string | null + pageIds?: readonly string[] | null +} + +export function collectBrowserPageIds( + tabs: readonly BrowserTabPageIdSource[] | null | undefined +): string[] { + return (tabs ?? []).flatMap((tab) => + tab.pageIds && tab.pageIds.length > 0 ? tab.pageIds : [tab.activePageId ?? tab.id] + ) +} + +// Why: a stable identity keeps the disabled branch from re-running downstream shallow compares. +const NO_BROWSER_PAGE_IDS: string[] = [] +const NO_BROWSER_TABS_BY_WORKTREE: Record = {} + +export function useWorktreeBrowserPageIds(worktreeId: string): string[] { + return useAppStore( + useShallow((state) => collectBrowserPageIds(state.browserTabsByWorktree[worktreeId])) + ) +} + +export function useBrowserGuestPaintRetention(browserPageIds: readonly string[]): boolean { + const hasAutomationVisibleBrowser = useBrowserAutomationVisibilityForAny(browserPageIds) + const hasMobileDrivenBrowser = useBrowserMobileDriverForAny(browserPageIds) + return hasAutomationVisibleBrowser || hasMobileDrivenBrowser +} + +// Why: `enabled` gates a scan across every worktree's tabs, which only matters while the +// caller is hidden. Automation visibility is load-bearing and not just symmetry with the +// per-worktree gate: a cold screencast cannot start without it. Main asks the renderer to +// mount a hidden guest via browser:activateView, which takes an automation bootstrap lease — +// and the mobile driver flag only flips AFTER that guest registers and streaming begins. Gate +// on the driver alone and the guest never mounts, so the driver never flips: a deadlock that +// leaves the page unreachable from the phone entirely. +export function useAnyBrowserGuestNeedsPaint(enabled: boolean): boolean { + const browserTabsByWorktree = useAppStore((state) => + enabled ? state.browserTabsByWorktree : NO_BROWSER_TABS_BY_WORKTREE + ) + const browserPageIds = useMemo( + () => + enabled + ? Object.values(browserTabsByWorktree).flatMap((tabs) => collectBrowserPageIds(tabs)) + : NO_BROWSER_PAGE_IDS, + [browserTabsByWorktree, enabled] + ) + return useBrowserGuestPaintRetention(browserPageIds) +}