Repository navigation
Fix paired remote HTML browser ownership and focus - #13876
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:
📝 WalkthroughWalkthroughRuntime-owned file previews now open through the owning web runtime and preserve split placement metadata. Browser creation uses provisional IDs, placement reservations, guarded focus intents, host-ID reconciliation, snapshot refresh, and failure cleanup. Session synchronization preserves client-selected browser groups and rejects stale focus requests. Tests cover races, placement limits, cleanup, routing, and paired remote HTML browser focus and lifecycle behavior. A reliability gate records validation commands, evidence, budgets, promotion criteria, and known gaps. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
4e2550d to
4c9c025
Compare
Jinwoo-H
changed the title
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/renderer/src/runtime/web-session-intent-owner.test.ts (1)
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeed
executedas empty and push from inside the first operation.Line 100 pre-seeds
executedwith'first', but the first operation body does not push. The final assertion at Line 135 then confirms'first'even if the first operation never runs. Push from inside the operation so the assertion proves execution.♻️ Proposed change
- const executed = ['first'] + const executed: string[] = [] const first = runSerializedWebSessionFocusOperation(OWNER_A, WORKTREE_ID, async () => { await firstBarrier + executed.push('first') return 'first' })src/renderer/src/runtime/web-session-focus-intent.ts (1)
47-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn
voidfromrecordWebSessionFocusIntent. No caller consumes its returned token, and the token always failsisWebSessionFocusIntentTokenValidandisWebSessionFocusIntentTokenCurrent.tests/e2e/paired-remote-html-browser-focus.spec.ts (1)
70-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
session.tabs.listcall into a shared helper.The same
session.tabs.listRPC-call shape (selector,method: 'session.tabs.list',params: { worktree:id:${worktreeId}},timeoutMs: 15_000) is built inline insidepage.evaluate()five separate times in this file. Extract a shared function passed topage.evaluate()so the request shape has one source of truth.♻️ Proposed helper extraction
+async function listSessionTabs( + page: Page, + environmentId: string, + worktreeId: string +): Promise<Awaited<ReturnType<typeof window.api.runtimeEnvironments.call>>> { + return page.evaluate( + async ({ environmentId, worktreeId }) => { + return window.api.runtimeEnvironments.call({ + selector: environmentId, + method: 'session.tabs.list', + params: { worktree: `id:${worktreeId}` }, + timeoutMs: 15_000 + }) + }, + { environmentId, worktreeId } + ) +}Then replace each inline construction, e.g.:
- const response = await window.api.runtimeEnvironments.call({ - selector: environmentId, - method: 'session.tabs.list', - params: { worktree: `id:${worktreeId}` }, - timeoutMs: 15_000 - }) + const response = await listSessionTabs(page, environmentId, worktreeId)Also applies to: 121-129, 187-194, 209-215, 251-262
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d5bd2e2-11fa-483d-817d-b34159f6d5cc
📒 Files selected for processing (10)
config/reliability-gates.jsoncsrc/renderer/src/lib/file-preview.test.tssrc/renderer/src/lib/file-preview.tssrc/renderer/src/runtime/web-runtime-session.test.tssrc/renderer/src/runtime/web-runtime-session.tssrc/renderer/src/runtime/web-session-focus-intent.tssrc/renderer/src/runtime/web-session-intent-owner.test.tssrc/renderer/src/runtime/web-session-tabs-sync.test.tssrc/renderer/src/runtime/web-session-tabs-sync.tstests/e2e/paired-remote-html-browser-focus.spec.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/reliability-gates.jsonc (1)
3249-3249: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate the STA-3872 known gap. The new
electron-headlessrun is a hidden-window Electron test, not rendererlessorca serve, but Playwright still drives the real HTML DOM through CDP. It is therefore a second HTML UI journey. Distinguish hidden-window parity from the remaining rendererless headless-serve gap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0db9159b-d124-45df-88b2-7e9172c512de
📒 Files selected for processing (4)
config/reliability-gates.jsoncsrc/renderer/src/runtime/web-runtime-session.test.tssrc/renderer/src/runtime/web-runtime-session.tssrc/renderer/src/runtime/web-session-intent-owner.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/runtime/web-session-intent-owner.test.ts
- src/renderer/src/runtime/web-runtime-session.ts
25188e1 to
33480dd
Compare
|
@coderabbitai review |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/runtime/web-runtime-session.test.ts (1)
1845-1860: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated store fixture into a local factory.
Both race tests repeat the same
mocks.getState.mockReturnValue({...})block, the samecreateBrowserTabreturn value, and the sameapplyFreshWebSessionTabsSnapshotpass-through. A single helper keeps the two tests aligned when the store shape changes.♻️ Proposed helper
function stubStoreForBrowserFocusRace(): void { mocks.getState.mockReturnValue({ settings: { activeRuntimeEnvironmentId: ENVIRONMENT_ID }, activeWorktreeId: WORKTREE_ID, browserPagesByWorkspace: {}, remoteBrowserPageHandlesByPageId: {}, createBrowserTab: mocks.createBrowserTab, setRemoteBrowserPageHandle: mocks.setRemoteBrowserPageHandle, focusBrowserTabInWorktree: mocks.focusBrowserTabInWorktree, setActiveWorktree: mocks.setActiveWorktree }) mocks.createBrowserTab.mockReturnValue({ id: 'local-browser-workspace-1', activePageId: 'local-page-1', pageIds: ['local-page-1'] }) mocks.applyFreshWebSessionTabsSnapshot.mockImplementation((state) => state) }Also applies to: 1922-1937
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15983f54-5736-447c-977d-f36b2b99c34e
📥 Commits
Reviewing files that changed from the base of the PR and between 25188e10d267d057401b415ec40b84e8e4dcbaf9 and 33480ddc186bdb7dcd7a1c21d1017acfd24ddc31.
📒 Files selected for processing (4)
src/renderer/src/runtime/web-runtime-session.test.tssrc/renderer/src/runtime/web-runtime-session.tssrc/renderer/src/runtime/web-session-tabs-sync.test.tssrc/renderer/src/runtime/web-session-tabs-sync.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/runtime/web-session-tabs-sync.test.ts
- src/renderer/src/runtime/web-session-tabs-sync.ts
- src/renderer/src/runtime/web-runtime-session.ts
33480dd to
480cc0d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
480cc0d to
d24c605
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
d24c605 to
3df3d10
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/runtime/web-runtime-session.ts (1)
962-1007: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReconcile a successful activation in the caller.
Line 972 sets
notifyClients: false. Lines 991-1007 discard the activation result and do not refreshsession.tabs. The host can activate the tab while the requesting client remains on its prior tab until an unrelated publication arrives.After successful activation, apply a validated acknowledgment snapshot or call
refreshWebRuntimeSessionTabsSnapshot(). Keep the focus intent until that reconciliation completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f2e7e42-1427-4c3b-bcba-f6712d5261d0
📥 Commits
Reviewing files that changed from the base of the PR and between d24c60519a1c5761c8e480a42edd6299613a8fb0 and 3df3d10.
📒 Files selected for processing (6)
config/reliability-gates.jsoncsrc/renderer/src/lib/file-preview.test.tssrc/renderer/src/lib/file-preview.tssrc/renderer/src/runtime/web-runtime-session.test.tssrc/renderer/src/runtime/web-runtime-session.tstests/e2e/paired-remote-html-browser-focus.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/src/lib/file-preview.ts
- src/renderer/src/lib/file-preview.test.ts
- tests/e2e/paired-remote-html-browser-focus.spec.ts
- src/renderer/src/runtime/web-runtime-session.test.ts
|
Latest-head review follow-up: the outside-diff activation concern is not reproducible. Every production |
* Fix paired remote HTML browser preview * Register paired HTML preview reliability gate * Scope browser worktree selection guard * Harden paired HTML preview ordering * Simplify paired HTML preview ownership * Record final paired HTML preview evidence * fix(remote): simplify browser preview ownership * test(remote): refresh HTML preview evidence * fix(remote): reconcile browser create ordering * fix(remote): guard browser focus through reconcile * fix(remote): bound delayed browser focus * fix(remote): guard browser focus context * test(remote): record final HTML preview evidence * fix(remote): keep HTML previews in background * test(remote): cover browser focus modes * test(remote): record focused browser contracts
* Fix paired remote HTML browser preview * Register paired HTML preview reliability gate * Scope browser worktree selection guard * Harden paired HTML preview ordering * Simplify paired HTML preview ownership * Record final paired HTML preview evidence * fix(remote): simplify browser preview ownership * test(remote): refresh HTML preview evidence * fix(remote): reconcile browser create ordering * fix(remote): guard browser focus through reconcile * fix(remote): bound delayed browser focus * fix(remote): guard browser focus context * test(remote): record final HTML preview evidence * fix(remote): keep HTML previews in background * test(remote): cover browser focus modes * test(remote): record focused browser contracts (cherry picked from commit 70ad65f)
* Fix paired remote HTML browser preview * Register paired HTML preview reliability gate * Scope browser worktree selection guard * Harden paired HTML preview ordering * Simplify paired HTML preview ownership * Record final paired HTML preview evidence * fix(remote): simplify browser preview ownership * test(remote): refresh HTML preview evidence * fix(remote): reconcile browser create ordering * fix(remote): guard browser focus through reconcile * fix(remote): bound delayed browser focus * fix(remote): guard browser focus context * test(remote): record final HTML preview evidence * fix(remote): keep HTML previews in background * test(remote): cover browser focus modes * test(remote): record focused browser contracts
Summary
Issue
https://linear.app/stably/issue/STA-3872/unable-to-open-html-files-in-remote-server
Reproduction and authority
Invariant: one paired-client HTML preview creates one browser page on the worktree-owning runtime and one client mirror. It remains in the requested split without taking editor focus; an explicit browser click activates it on client and host; closing it preserves the editor and terminal.
Observed failure: the split appeared briefly and collapsed, two blank browser tabs could appear, and clicking them could return to the Claude/terminal tab.
Authority: the worktree-owning runtime owns browser creation and page identity. The requesting client owns only its local split placement and focus choice.
Current byte-identical Electron oracle:
tests/e2e/paired-remote-html-browser-focus.spec.ts(SHA-2562ec1dcf101f922c86a9e1e20c5a0cb785e478fd2c1fe0c6c9d89b43dd4048dd1).346e59c879: red in 11.8s; two client HTML browser workspaces appeared.focusOnCreate: falsedisabled: red in 12.3s; preview creation activated the browser instead of retaining editor focus.The earlier byte-identical oracle (
5806b23f…) separately proved the owning-runtime seam: affected main was red, the candidate was green, and disabling only owning-runtime routing was red with duplicate client workspaces.Root cause and design
The paired client previously opened/staged browser state locally even though the remote runtime owned the worktree and browser page. Host publication then introduced a second identity and host group placement, allowing reconciliation to duplicate, collapse, or refocus the client surface.
The fix follows ownership:
browser.tabCreateon the worktree owner.focusOnCreate: false, so the host creates them inactive and the client records no focus intent or temporary listener.Ordinary New Browser Tab actions retain their existing focused-create behavior and bounded listener. No new wire field, opcode, RPC method, retry protocol, deterministic page ID, receipt ledger, TTL, timer, or polling loop is introduced.
Alternatives considered
Risks and compatibility
orca serveremain explicit topology gaps; no Windows-specific primitive is causal here.Validation
Final merge base:
origin/main@346e59c879adpnpm typecheck: passedpnpm run check:code-quality:changed: passed with zero findingspnpm run check:max-lines-ratchet: passedpnpm run check:reliability-gates: passed (75 gates)git diff --check: passedVisual proof
N/A. The paired Electron/CDP oracle validates rendered HTML, split visibility, client/host state, focus transitions, identity cardinality, and close convergence; no screenshot is used as the sole oracle.
Do not merge automatically; this PR is ready for review.