Repository navigation
fix(mobile): keep unmounted desktop browser tabs visible and closable - #31665
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
|
6dd0cd1 to
8955126
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a6382bc3-f50c-4ea1-bdd6-51b195a1a2b9
📒 Files selected for processing: 14
src/main/runtime/orca-runtime-tests/terminal-handles-and-agent-status-part-03.spec.tssrc/main/runtime/renderer-browser-accepted-epoch-review.test.tssrc/main/runtime/renderer-browser-lifecycle.test.tssrc/main/runtime/renderer-browser-ownership.test.tssrc/main/runtime/renderer-browser-session-reconciliation.test.tssrc/main/runtime/rpc/methods/session-tabs-move-validation.test.tssrc/main/runtime/runtime-mobile-session-projection-contract.tssrc/main/runtime/runtime-mobile-session-projection.tssrc/main/runtime/session-lifecycle/projection/prune-mobile-session-tab-group-layout.tssrc/main/runtime/session-lifecycle/projection/reconcile-headless-mobile-session-browser-tabs.tssrc/main/runtime/session-tabs/mobile-session-preservation.tssrc/main/runtime/session-tabs/mobile-session-publication-preservation.tssrc/main/runtime/session-tabs/structured-session-tab-close.tssrc/main/runtime/workspaces/workspace-file-session-targets.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
| protected isHeadlessOwnedMobileBrowserTab( | ||
| snapshot: RuntimeMobileSessionTabsSnapshot, | ||
| tab: RuntimeMobileSessionBrowserTab | ||
| ): boolean { | ||
| if (this.isHeadlessBuiltMobileSessionPublicationBase(snapshot.publicationEpoch)) { | ||
| return true | ||
| } | ||
| const rendererTabIdentityKeys = | ||
| this.getAcceptedRendererIdentityKeysForMobileSessionSnapshot(snapshot) | ||
| if (rendererTabIdentityKeys) { | ||
| return !getMobileSessionSnapshotTabIdentityKeys(tab).some((id) => | ||
| rendererTabIdentityKeys.has(id) | ||
| ) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C3 'headless-merge' src/main src/renderer src/shared -g '!**/*.test.ts' -g '!**/*.spec.ts'
rg -nP --type=ts -C3 'acceptedRendererMobileSnapshotByWorktree\.(set|delete)' src/mainRepository: stablyai/orca
Length of output: 16610
🏁 Script executed:
set -e
printf '%s\n' '--- preservation file ---'
nl -ba src/main/runtime/session-tabs/mobile-session-publication-preservation.ts | sed -n '1,245p'
printf '%s\n' '--- close routing references ---'
rg -n -F --glob '*.ts' -e 'isOffscreenMobileSessionBrowserTab' -e 'getListedBrowserTabsByPageId' -e 'closeSessionTab' src/main
printf '%s\n' '--- close routing definition ---'
nl -ba src/main/runtime/session-lifecycle/projection/structured-session-tab-close.ts | sed -n '120,215p'
printf '%s\n' '--- reconciliation complete relevant blocks ---'
nl -ba src/main/runtime/session-tabs/mobile-session-reconciliation.ts | sed -n '110,255p'
printf '%s\n' '--- diff from supplied merge base ---'
git diff --unified=35 0b82f88e9f2b6904f8ecf296b11d0581de4cfbb9 204797298ddffcf825a1f20ca60d8b12a95dbf89 -- src/main/runtime/session-tabs/mobile-session-publication-preservation.ts src/main/runtime/session-tabs/mobile-session-reconciliation.ts src/main/runtime/session-lifecycle/projection/structured-session-tab-close.tsRepository: stablyai/orca
Length of output: 40447
🏁 Script executed:
set -e
printf '%s\n' '--- close routing ---'
nl -ba src/main/runtime/session-tabs/structured-session-tab-close.ts | sed -n '135,205p'
printf '%s\n' '--- renderer ownership consumers ---'
rg -n -F --glob '*.ts' -e 'isRendererOwnedMobileBrowserTab' -e 'isHeadlessOwnedMobileBrowserTab' -e 'collectPublicMobileSessionTabIds' src/main
printf '%s\n' '--- consumer source ---'
for f in $(rg -l -F --glob '*.ts' -e 'isRendererOwnedMobileBrowserTab' -e 'collectPublicMobileSessionTabIds' src/main); do
echo "--- $f"
nl -ba "$f" | sed -n '1,260p'
done
printf '%s\n' '--- base merge implementation ---'
git show 0b82f88e9f2b6904f8ecf296b11d0581de4cfbb9:src/main/runtime/session-tabs/mobile-session-publication-preservation.ts | nl -ba | sed -n '35,145p'Repository: stablyai/orca
Length of output: 42557
Treat listed pages under a legacy :headless-merge: epoch as headless-owned.
When no accepted renderer identity pair exists, isRuntimeOwnedBrowserOnRendererPublication treats a :headless-merge: snapshot as runtime-owned, but isHeadlessOwnedMobileBrowserTab returns false. A listed offscreen page can therefore be treated as renderer-owned, routed to closeSessionTab, and retained by renderer-owned projection filters.
Use the offscreen inventory as the ownership proof for this legacy fallback:
Suggested fix
if (rendererTabIdentityKeys) {
return !getMobileSessionSnapshotTabIdentityKeys(tab).some((id) =>
rendererTabIdentityKeys.has(id)
)
}
- return false
+ // Legacy mixed snapshot: a live offscreen page is the only owner proof left.
+ return (
+ snapshot.publicationEpoch.includes(':headless-merge:') &&
+ typeof tab.browserPageId === 'string' &&
+ this.getListedBrowserTabsByPageId(snapshot.worktree).has(tab.browserPageId)
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected isHeadlessOwnedMobileBrowserTab( | |
| snapshot: RuntimeMobileSessionTabsSnapshot, | |
| tab: RuntimeMobileSessionBrowserTab | |
| ): boolean { | |
| if (this.isHeadlessBuiltMobileSessionPublicationBase(snapshot.publicationEpoch)) { | |
| return true | |
| } | |
| const rendererTabIdentityKeys = | |
| this.getAcceptedRendererIdentityKeysForMobileSessionSnapshot(snapshot) | |
| if (rendererTabIdentityKeys) { | |
| return !getMobileSessionSnapshotTabIdentityKeys(tab).some((id) => | |
| rendererTabIdentityKeys.has(id) | |
| ) | |
| } | |
| return false | |
| } | |
| protected isHeadlessOwnedMobileBrowserTab( | |
| snapshot: RuntimeMobileSessionTabsSnapshot, | |
| tab: RuntimeMobileSessionBrowserTab | |
| ): boolean { | |
| if (this.isHeadlessBuiltMobileSessionPublicationBase(snapshot.publicationEpoch)) { | |
| return true | |
| } | |
| const rendererTabIdentityKeys = | |
| this.getAcceptedRendererIdentityKeysForMobileSessionSnapshot(snapshot) | |
| if (rendererTabIdentityKeys) { | |
| return !getMobileSessionSnapshotTabIdentityKeys(tab).some((id) => | |
| rendererTabIdentityKeys.has(id) | |
| ) | |
| } | |
| // Legacy mixed snapshot: a live offscreen page is the only owner proof left. | |
| return ( | |
| snapshot.publicationEpoch.includes(':headless-merge:') && | |
| typeof tab.browserPageId === 'string' && | |
| this.getListedBrowserTabsByPageId(snapshot.worktree).has(tab.browserPageId) | |
| ) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
741e3088-811f-473f-bc96-685beecff97c
📒 Files selected for processing: 2
src/main/runtime/renderer-browser-lifecycle.test.tssrc/main/runtime/session-tabs/mobile-session-preservation.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Preserve renderer-owned browser rows during… · mobile-session-publication-preservation.ts:77-101
src/main/runtime/session-tabs/mobile-session-publication-preservation.ts:77-101
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve renderer-owned browser rows during snapshot merges.
When a non-client browser row has a non-legacy publication epoch with no accepted renderer identity set,
isRendererOwnedMobileBrowserTabclassifies it as renderer-owned.shouldPreserveHeadlessMobileSessionTabcurrently rejects it, so an incoming snapshot that omits the row can remove it from the merged snapshot.Apply the renderer-ownership check before the offscreen-backend guard. This is separate from the legacy
:headless-merge:fallback and browser-group restoration.Suggested fix
if ( liveClientPage?.workspaceId === snapshot.worktree && tab.placement?.kind === 'client' && sameRuntimeBrowserPlacement(liveClientPage.placement, tab.placement) ) { return true } + if (this.isRendererOwnedMobileBrowserTab(snapshot, tab)) { + return true + } // Why: headless offscreen browser tabs exist only server-side, so a renderer-graph merge must keep them, not prune as "not in the graph". if (!this.offscreenBrowserBackend) { return false }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
60e321e7-f66c-4143-93a5-c7d0b809ba67
📒 Files selected for processing: 1
src/renderer/src/components/browser-pane/assemble-chrome/BrowserAddressBar.tsx
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
9d4174f to
afb8f0a
Compare
[Medium impact] The PR appears safe to merge; no new blocking issue was found. SummaryKeeps unmounted desktop browser tabs visible on phones and sends Close to the owner of each tab.
No new actionable issues were found. The previous-review ancestry and merge-base checks did not pass, so the full PR diff was reviewed. In the PR description,
|
d9a2b14 to
e15b4e0
Compare
Adapt the community browser ownership fix from #14792 to the current runtime modules. Preserve renderer epochs and route mobile browser close through its owner. Co-authored-by: Bing.Z <zzb@gxsmjx.com> Co-authored-by: Cursor <cursoragent@cursor.com>
In serve mode the offscreen reconcile no longer checked for a window, so a closed window's browser rows stayed listed with no page behind them. Also drop the legacy :headless-merge: ownership fallback, which nothing produces.
The bridge inventory also lists an attached window's webviews. Only the offscreen reconcile filtered those out, so hydrate added a second row for the same page, and closing it unregistered the desktop's guest. Filter in the shared builder instead.
With a desktop window attached in serve mode the bridge inventory also lists the window's background webviews, which the renderer does not publish. Those became offscreen rows, and closing one unregistered the desktop's guest. Ask the backend which pages it owns.
…d dedupe serve rows
e15b4e0 to
058fae6
Compare
ELI5
Your phone should remember a desktop browser tab even when that workspace is not on screen. Keep those tabs in the mobile list and send Close to the desktop that owns them, so closing one from your phone stays closed when you return to the workspace.
What Changed
Carried onto current main by Kelvin (on top of Neil's commits):
BrowserBackend.ownsTab), so a desktop webview's page is listed once, under its renderer row.session.tabs.closeLifecyclecalls fail asstale-publication.BrowserBackend.ownsTab, notifier fields), plus a one-line delegate inprune-mobile-session-tab-group-layout.tsto stay under the 300-line lint cap.Why
Create a browser tab in folder workspace A, switch desktop to workspace B, then reload the desktop renderer so A has no mounted webview. Before this change, the actual iOS app reports 0 tabs for A even though the saved desktop browser record and unified tab remain. Runtime projection and reconciliation incorrectly treated absence from the live page registry as proof that every browser tab had closed.
The fix sits at the host's tab projection, which owns the decision of which browser rows exist. Mobile code is unchanged, so old phones just see rows they used to lose.
Linked Issue
Fixes #14598. Adapts the community fix in #14792 by @bbingz (Bing.Z), with the original Bing.Z and Cursor co-author credits preserved.
Visual Proof
Before: the saved desktop browser disappears from the actual phone list after its desktop renderer is unmounted.
After: the same workspace retains Example Domain with no mounted desktop webview.
The phone's real action menu can close it.
After Close, the desktop workspace remounts without restoring the browser.
Testing
TMPDIR=/private/tmp; the initial run's two unrelated failures compared/varagainst/private/var.git diff --checkpassed.49178f1e1b7passes all applicable GitHub checks, including the aggregate gate, Node 24 unit shards, relay integration, static/type checks, Linux/Windows packaging, WSL distro checks and mode-switch layout oracle.Native proof fixture: the available unsigned simulator binary lacks Keychain entitlement, so isolated pairing credential storage used a temporary AsyncStorage adapter. The actual app, built mobile web bundle, transport, runtime, and tab actions were exercised. The adapter and launch configuration are absent from this PR. Desktop renderer reload detaches the active browser screencast, so the after image also shows that streaming warning. That native run did not verify streaming recovery; subsequent transport and mounted-pane checks are described below. Android, Windows, Linux, and a separate remote host were not manually exercised.
Follow-up recovery and compatibility checks on unchanged head
49178f1e1b7:session.tabs.close, and observed no browser row or webview after workspace remount. This exercises the existing capability/optional-field path, not a released old binary.Page.enabletimeout; it did not reproduce in these six cycles or the preceding two successful exact-head repeats. No source change was made to mask that intermittent result.MobileBrowserPane, streaming hook, andDirectRpcClientwere mounted against the hidden desktop over real E2EE transport. Renderer reload produced the detach warning; the existing app foreground transition created exactly one replacement subscription, received fresh pixels, and cleared the warning. The same subscription and warning assertions pass against the baseline runtime. The saved head and baseline mounted-pane JPEGs contain identical page content (confirmed by SHA256 and decoded PNG inspection). Native component rendering and the AppState event were mocked in this check, so it is not native OS lifecycle proof.Further bounded checks on the unchanged head:
serveBrowserTabs.listUnstartedonto a newer renderer row. The baseline reproduces this too (and also replaces the identity). The PR preserves the newer identity/group; stale saved metadata is an existing neighboring limitation, documented for follow-up without expanding this ownership fix.Native OS re-entry remains unverified in this round: CoreSimulator create and boot calls stalled, including an isolated device set. Only owned pending calls were cancelled; existing visible simulators were left untouched. All desktop launches stayed hidden with
ORCA_BACKGROUND_LAUNCH=1. Android, Windows/Linux app launches, and a separate physical remote host remain outside the manual coverage. Final-head CI still reports 27 successful checks, with 24 skipped.Re-verified on rebased head
afb8f0a28bf(Kelvin, 2026-10-28). Later commits only change serve-mode refresh and the window-close republish, neither of which this desktop flow reaches. Setup: hidden dev desktop built from this branch, with folder workspaces A and B and the released Android app (OTA shell) paired over E2EE.Opened Example Domain in folder A, switched the desktop to B and reloaded the renderer, leaving zero webviews. The phone listed 2 tabs for A, including Example Domain.
Reloaded the desktop renderer again (0 webviews) and tapped Close in the phone's tab menu. The phone and the desktop's unified tabs for A both dropped to the terminal only.
Switched the desktop back to A. No browser tab came back, and the phone showed 1 tab.
Restarted the dev desktop on the same profile (on head
e15b4e09b2a). Folder A came back with only its terminal, so the close from the phone persisted.The iOS simulator could not be used this round: CoreSimulator is wedged on this machine, with every fleet simulator stuck in "Shutting Down".
Focused runtime/browser/session-tab and window-lifecycle suites: 1,831 passed, 3 skipped, 1 expected fail across 39 files. Four regression tests each fail on the previous head and pass now: serve epoch kept, window close pushes a newer-version drop, colliding ids listed once, and an omitted worktree's exited terminal pruned. Changed-file oxlint and oxfmt pass.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
review-loop (Codex + Claude), four rounds on the rebased branch:
Agent skill upstream boundary
Notes
Medium-low, reduced further by a reconnect fix. A delayed offscreen close across renderer detach/reconnect exposed a real group-projection bug: the surviving browser row remained in
tabsbut lost its group order entry when the renderer returned. The preserved-tab merge now reusesappendBrowserTabOrderandcollectBrowserGroupAssignmentfor preserved browser rows. Incoming renderer rows and their ordering remain authoritative. The regression exercises production runtime close, detach, return, snapshot versions and group membership; no replacement page or newer renderer row is lost.Validation: 1,482 runtime/lifecycle/ownership/reconciliation/epoch tests pass, two skipped, with
TMPDIR=/private/tmpon macOS. The first broader run's two failures were/varversus/private/varassertions in unrelated repository setup tests; canonical TMPDIR resolves both. Node typecheck passes across 17,338 files; changed-code quality and diff checks pass. Final-head GitHub CI passes after this production follow-up and the integration cleanup below.Earlier native iOS/macOS folder-workspace close/remount proof still applies to the unchanged close-routing behavior. The new group repair is covered by runtime integration tests, not a fresh native recording. Browser ownership spans projection, close, client placement and renderer generations, so broad platform/streaming-recovery coverage remains incomplete. No merge performed.
CI integration cleanup: current main pushed BrowserAddressBar and ClientHostedBrowserPagePane over the 400-line lint limit. Formatting-only JSX prop grouping clears both errors; the clean combined merge tree passes oxlint and formatting. No browser behavior changes in these follow-ups. Two retired Portuguese browser-zoom catalog entries were also removed to align with current main; full catalog validation passes. Final head
49178f1e1b7passes all applicable CI checks, including the aggregate gate.CI rerun provenance: the first full run had one failure in the unchanged structured-session option-retry test. Its focused file passed locally (19 checks), the exact case passed three repeated runs, and the full failed shard passed on rerun without source changes. The initial failure and successful rerun remain visible in Actions. No native-chat option code or tests were changed in this PR.
Follow-ups, not in this PR. Each is an edge case that main never reached because main hid every unmounted row:
collectPublicMobileSessionTabIds.Ensure no issues in: Security, Cross-platform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)