Repository navigation
fix(remote-browser): host client browser pages again when a paired host comes back after an outage - #25827
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📥 CommitsReviewing files that changed from the base of the PR and between 2d0b1c29d07e47a469873af482fc3b30fe1c2c11 and b2e29d1. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughRuntime status handling now prepares the client-hosted browser host after a restart or when runtime contact returns. End-to-end tests cover silent and reset network outages and verify recovery with a new host generation. The unavailable notice now distinguishes pages open on another desktop, offline hosts, and pages unavailable on the current desktop. Translations for these states were updated in six locales. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A page may remain unavailable after contact returns in the identified coalescing case. Resolve or explicitly accept that recovery risk before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 9 files. (4 skipped: 4 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/paired-client-hosted-browser-network-outage.spec.ts (1)
173-181: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCleanup failure can mask the real test failure and skip later cleanup.
Each cleanup step in
finallyis awaited in sequence without protection. Ifclient.dispose()orproxy?.close()throws, the remaining steps (host.dispose(),fixture.close()) are skipped. This leaks the headless host process and its profile, and the cleanup error replaces the original assertion error. Make each step independent so every resource is released.Proposed fix
} finally { - if (client) { - await cleanupE2EDaemons(client.userDataDir).catch(() => undefined) - await client.dispose() - } - await proxy?.close() - await host.dispose() - await fixture.close() + await Promise.allSettled([ + (async () => { + if (client) { + await cleanupE2EDaemons(client.userDataDir).catch(() => undefined) + await client.dispose() + } + })(), + proxy?.close(), + host.dispose(), + fixture.close() + ]) }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
24f77ebc-e334-4467-858b-c075ce4c18db
📒 Files selected for processing (14)
src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsxsrc/renderer/src/components/browser-pane/client-hosted-browser-unavailable-notice.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/fr.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/runtime/restored-client-hosted-browser-host-attach.tssrc/renderer/src/runtime/restored-client-hosted-browser-host-restart-attach.test.tssrc/renderer/src/store/slices/runtime-status-snapshot.test.tssrc/renderer/src/store/slices/runtime-status.tstests/e2e/helpers/freezable-tcp-proxy.tstests/e2e/paired-client-hosted-browser-network-outage.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
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:
b8d75095-0b34-453d-b7fd-ee6056c32a12
📒 Files selected for processing (5)
src/renderer/src/components/browser-pane/client-hosted-browser-unavailable-notice.test.tsxsrc/renderer/src/components/browser-pane/client-hosted-browser-unavailable-notice.tsxsrc/renderer/src/runtime/restored-client-hosted-browser-host-attach.tssrc/renderer/src/runtime/restored-client-hosted-browser-host-restart-attach.test.tssrc/renderer/src/store/slices/runtime-status.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
f1f0a82 to
2d0b1c2
Compare
Visual proof and final regression run (head
|
| Suite | Result |
|---|---|
all paired-client-hosted-browser* (10 specs, including the reset and silent outage tests) |
13/13 |
| paired terminal host-restart, client-restart, host-quit-reconnect, materialization-reconnect, serve-restart-binding | 6/6 |
| paired browser stream-reconnect, ghost-subscriber-rejoin, split-pane host-retired | 4/4 |
runtime-host-status-recovery (needs ORCA_E2E_WEB_CLIENT=1) |
2/2 |
unit: runtime-status*, restored-client-hosted*, components/browser-pane |
1055/1055 |
Skipped:
- the SSH client-hosted drop/reconnect spec (needs Docker);
- one serve-restart case that creates a Codex automation;
- the headful-only probe-gap spec.
Typecheck on current main fails in structured-agent-session-live-turn.test.ts. That's unrelated to this PR and is fixed in #25936.
…ailable after a network outage A paired desktop that loses its host for longer than the browser-host lease grace (25s reset cut, or a 100s silent cut the host's heartbeat reaps) tears down its client-hosted pages and never hosts them again, leaving 'Client-hosted browser unavailable'. Red on main. Extends the freezable TCP proxy with a full silent or reset link cut.
…st comes back after an outage When the network between a paired desktop and its Orca host dropped for longer than the browser-host lease grace, the desktop tore down its client-hosted pages and never offered to host them again, so the tab stayed on 'Client-hosted browser unavailable' until the app was relaunched. Regaining contact with a runtime now re-claims browser hosting the same way a runtime restart already did: if this desktop still has client-placed pages for that host, it starts its browser client host again and the host's existing recovery re-places them, reloading at their last URL. The notice now says the host is offline (and that the page reloads when it reconnects), names another desktop only when the placement belongs to one, and otherwise says the page is not available here right now.
…, and re-host from the notice The unavailable notice said the host was offline for any null status, which also covers a retired or refused pairing and a host whose runtime did not answer. It now uses the host connection state and excludes revoked verdicts. When the control link returns while the old browser host is still inside its lease reconnect, the contact-edge ensure is a no-op; if that reconnect then fails, nothing re-hosts. The notice now re-claims hosting on mount while the host is reachable, which is exactly that stranded state. Plain regained contact no longer queues a second preparation behind one already in flight; only a runtime restart does.
…tact A preparation still in flight when contact returns started while the host was unreachable, so it is the one most likely to fail. Coalescing the contact edge into it dropped the only trigger that could succeed and left the page unavailable. Always queue one follow-up preparation instead.
2d0b1c2 to
b2e29d1
Compare
ELI5
A remote browser tab in a paired workspace is drawn on your desktop. If the network went away for more than about 15 seconds (Wi‑Fi drop, laptop sleep), the desktop stopped hosting that page and never offered to host it again, so the tab said "Client-hosted browser unavailable … attached to a different desktop" until Orca was restarted. Now, when the remote comes back, the desktop hosts its pages again and they reload at their last address.
What Changed
Before: the desktop re-claimed browser hosting for a remote only when that remote restarted. Losing and regaining contact with the same remote did nothing, even though the outage had already torn the hosting down.
After: one rule in
setRuntimeEnvironmentStatus: when the runtime restarts or contact with it is regained, run the existing ensure (renamedensureBrowserClientHostOnRuntimeContact; its guards and coalescing are unchanged). If this desktop still has client-placed pages for that remote, it starts hosting again, and the remote's existing recovery re-places them, reloading at their last address. The pane picks up the new placement through its existing effect. The contact-regained condition is computed once and also drives the toast dismissal and the host-contact counter, replacing a duplicated expression.Message: the notice names "another desktop" only when the page actually belongs to a different desktop. While the remote is unreachable it says " is offline. This page will reload here once it reconnects"; otherwise "This page isn't available on this desktop right now". Translated in all locales.
Why
The root cause was a missing trigger, not missing machinery: the restart path already knew how to re-host. Alternatives considered:
Trade-off: after a long outage the page reloads at its last address. You stay signed in, but scroll position and half-typed forms are lost.
Compatibility: client-only, with no host or wire change. Older clients are unchanged.
Linked Issue
N/A (internal report).
Visual Proof
To follow in a comment (the notice before, and the page re-hosted after a 25s outage).
Testing
E2E,
tests/e2e/paired-client-hosted-browser-network-outage.spec.ts: a real paired host and desktop with isolated profiles, and a TCP proxy that cuts the link by reset (25s) or silently (100s, long enough for the host's heartbeat to reap the socket).All
tests/e2e/paired-client-hosted-browser*specs: 13/13.Unit: a new runtime-status test re-attaches when the same runtime answers again after contact was lost, through the real store slice and the real ensure, and goes red without the change. Related suites: 3166 passed.
pnpm tc, oxlint, oxfmt,check:code-quality:changedand the localization checks pass.Platforms: macOS desktop with a headless macOS remote. Windows host not exercised.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)