mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
fix(browser): keep browser guests painting when the workbench is hidden (#14599)
* fix(browser): keep browser guests painting when the workbench is hidden
Chromium never paints inside a display:none subtree, so an Electron <webview>
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>
This commit is contained in:
co-authored by
Kaylee Williams
parent
73bbe20ca7
commit
7558fb064a
@@ -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: {
|
||||
)}
|
||||
<div className="flex flex-1 min-w-0 min-h-0 flex-col">
|
||||
{layout.shouldMountTerminalWorkbench ? (
|
||||
<div
|
||||
className={
|
||||
layout.terminalWorkbenchVisible
|
||||
? 'flex flex-1 min-w-0 min-h-0'
|
||||
: 'hidden flex-1 min-w-0 min-h-0'
|
||||
}
|
||||
>
|
||||
<TerminalWorkbenchContainer isVisible={layout.terminalWorkbenchVisible}>
|
||||
<Suspense fallback={null}>
|
||||
<RecoverableRenderErrorBoundary
|
||||
boundaryId="terminal.workbench"
|
||||
@@ -198,7 +193,7 @@ export function AppWorkspaceShell(props: {
|
||||
<Terminal />
|
||||
</RecoverableRenderErrorBoundary>
|
||||
</Suspense>
|
||||
</div>
|
||||
</TerminalWorkbenchContainer>
|
||||
) : null}
|
||||
<Suspense fallback={null}>
|
||||
<RecoverableRenderErrorBoundary
|
||||
|
||||
@@ -4,7 +4,6 @@ import React, { useEffect, useCallback, useMemo, useRef, useState, Suspense } fr
|
||||
import { lazyWithRetry as lazy } from '@/lib/lazy-with-retry'
|
||||
import { createPortal } from 'react-dom'
|
||||
import { toast } from 'sonner'
|
||||
import { useShallow } from 'zustand/react/shallow'
|
||||
import {
|
||||
BACKGROUND_MOUNT_TERMINAL_WORKTREE_EVENT,
|
||||
TOGGLE_TERMINAL_PANE_EXPAND_EVENT,
|
||||
@@ -55,6 +54,10 @@ import {
|
||||
onBrowserDriverChange,
|
||||
useBrowserMobileDriverForAny
|
||||
} from '@/lib/pane-manager/browser-mobile-driver-state'
|
||||
import {
|
||||
useAnyBrowserGuestNeedsPaint,
|
||||
useWorktreeBrowserPageIds
|
||||
} from './browser-pane/browser-guest-paint-retention'
|
||||
import TerminalPaneOverlayLayer from './terminal-pane/TerminalPaneOverlayLayer'
|
||||
import {
|
||||
collectBrowserWebviewIds,
|
||||
@@ -445,6 +448,11 @@ function Terminal(): React.JSX.Element | null {
|
||||
const effectiveActiveLayout = renderedActiveWorktreeId
|
||||
? getEffectiveLayoutForWorktree(renderedActiveWorktreeId)
|
||||
: undefined
|
||||
// Why: both wrappers below sit above every browser <webview>, 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 (
|
||||
<div
|
||||
className={`flex flex-col flex-1 min-w-0 min-h-0 overflow-hidden${renderedActiveWorktreeId ? '' : ' hidden'}`}
|
||||
// Why: already out of flow via the workbench container when hidden, so retention only
|
||||
// has to drop `hidden` — it does not need to leave the flex column a second time.
|
||||
className={`flex flex-col flex-1 min-w-0 min-h-0 overflow-hidden${
|
||||
renderedActiveWorktreeId
|
||||
? ''
|
||||
: retainBrowserGuestPaint
|
||||
? ' opacity-0 pointer-events-none'
|
||||
: ' hidden'
|
||||
}`}
|
||||
data-rendered-active-worktree-id={renderedActiveWorktreeId ?? undefined}
|
||||
>
|
||||
<EditorAutosaveController />
|
||||
@@ -2478,7 +2494,13 @@ function Terminal(): React.JSX.Element | null {
|
||||
|
||||
{anyMountedWorktreeHasLayout ? (
|
||||
<div
|
||||
className={`relative flex flex-1 min-w-0 min-h-0 overflow-hidden${effectiveActiveLayout ? '' : ' hidden'}`}
|
||||
className={`relative flex flex-1 min-w-0 min-h-0 overflow-hidden${
|
||||
effectiveActiveLayout
|
||||
? ''
|
||||
: retainBrowserGuestPaint
|
||||
? ' opacity-0 pointer-events-none'
|
||||
: ' hidden'
|
||||
}`}
|
||||
>
|
||||
{/* 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<string> | null
|
||||
activationDeferredMountTabIds: ReadonlySet<string> | 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 =
|
||||
|
||||
@@ -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<string, readonly BrowserTabState[]> }
|
||||
|
||||
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(
|
||||
<TerminalWorkbenchContainer isVisible={isVisible}>
|
||||
<span>workbench</span>
|
||||
</TerminalWorkbenchContainer>
|
||||
)
|
||||
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')
|
||||
})
|
||||
})
|
||||
@@ -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 <webview>. 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 (
|
||||
<div
|
||||
className={
|
||||
isVisible
|
||||
? 'flex flex-1 min-w-0 min-h-0'
|
||||
: retainBrowserGuestPaint
|
||||
? // Why: absolute keeps the invisible workbench out of the flex column so the
|
||||
// active page (Settings, Tasks, …) still gets the full content area.
|
||||
'absolute inset-0 flex opacity-0 pointer-events-none'
|
||||
: 'hidden flex-1 min-w-0 min-h-0'
|
||||
}
|
||||
// Why: a paintable-but-hidden workbench must stay unreachable by Tab / assistive tech.
|
||||
inert={!isVisible}
|
||||
aria-hidden={!isVisible}
|
||||
data-terminal-workbench-container=""
|
||||
>
|
||||
{children}
|
||||
</div>
|
||||
)
|
||||
}
|
||||
@@ -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'])
|
||||
})
|
||||
})
|
||||
@@ -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 <webview> 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<string, BrowserTabPageIdSource[]> = {}
|
||||
|
||||
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)
|
||||
}
|
||||
Reference in New Issue
Block a user