Repository navigation
Conversation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/paired-remote-terminal-session-exit-reopen.spec.ts (1)
237-248: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPoll for recovery instead of sleeping for 45 seconds.
Every passing run waits the full 45 seconds before checking the recovery conditions. Poll both counts with a bounded 60-second timeout so fast recoveries finish early and recoveries that take slightly longer can still pass. The test’s 300-second budget allows a longer poll; the fixed wait does not itself threaten that budget. The small observation dump has no separately established workflow impact.
Suggested fix
// The reopen: the user clicks the workspace in the sidebar again. await clickSidebarWorkspaceRow(client, worktreeId) - await client.page.waitForTimeout(45_000) + await Promise.all([ + expect + .poll(() => readHostTerminalCount(host, worktreeId), { + timeout: 60_000, + message: 'Reopened workspace has no live host surface' + }) + .toBeGreaterThan(0), + expect + .poll( + () => readClientWorkspaceState(client, worktreeId).then((state) => state.tabIds.length), + { timeout: 60_000, message: 'Reopened workspace renders no tabs' } + ) + .toBeGreaterThan(0) + ]) const observed = await readClientWorkspaceState(client, worktreeId)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2d258272-7470-4b78-abf6-b28f621c603e
📥 Commits
Reviewing files that changed from the base of the PR and between 9fdd90f and ddabb367f0308d403dfb4e8e825f2c354a99f573.
📒 Files selected for processing (5)
src/renderer/src/runtime/web-runtime-session-tab-lifecycle.tssrc/renderer/src/runtime/web-session-close-intent.tssrc/renderer/src/runtime/web-session-tabs-sync-emptied-workspace-tombstone.test.tssrc/renderer/src/runtime/web-session-tabs-sync/apply-preparation-base.tstests/e2e/paired-remote-terminal-session-exit-reopen.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| // Why remember the emptying here: this is the one place that knows the | ||
| // close is the user's own. When the intents cover every terminal tab the | ||
| // row still holds, this close empties the workspace, and the mirror's | ||
| // affirming-empty frame may then leave its closed-last-terminal tombstone | ||
| // row. A session the host ends on its own never passes through here, so | ||
| // its emptying leaves no marker and the row is deleted instead. | ||
| const rowTabs = state.tabsByWorktree[args.worktreeId] ?? [] | ||
| const terminalTabsWithoutCloseIntent = rowTabs.filter((tab) => { | ||
| if (!isWebTerminalSurfaceTabId(tab.id)) { | ||
| return false | ||
| } | ||
| return !closeIntentTabIds.has(toHostSessionTabId(tab.id)) | ||
| }) | ||
| if (terminalTabsWithoutCloseIntent.length === 0) { | ||
| rememberUserEmptiedWorktree(args.worktreeId) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the user-empty marker when a close fails or the host refuses it.
The code writes the marker before the RPC call. Two paths can leave the terminal alive but keep the marker:
- The host refuses the close and republishes the snapshot (lines 165-172).
- The RPC call fails with an error other than
tab_not_found(the catch block).
In both paths, the code clears the close intents but leaves the marker. The marker clears only when a nonempty terminal row lands in prepareWebSessionTabsSnapshotBase. If the terminal later exits on the host before such a snapshot is applied, the code keeps the tombstone, and the workspace stays empty. Call forgetUserEmptiedWorktree(args.worktreeId) in the refusal branch and in the non-definitive branch of the catch block.
| export function rememberUserEmptiedWorktree(worktreeId: string): void { | ||
| const trimmed = worktreeId.trim() | ||
| if (trimmed) { | ||
| userEmptiedWorktreeIds.add(trimmed) | ||
| } | ||
| } | ||
|
|
||
| export function forgetUserEmptiedWorktree(worktreeId: string): void { | ||
| userEmptiedWorktreeIds.delete(worktreeId) | ||
| } | ||
|
|
||
| export function wasWorktreeEmptiedByUserClose(worktreeId: string): boolean { | ||
| return userEmptiedWorktreeIds.has(worktreeId) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'rememberUserEmptiedWorktree|forgetUserEmptiedWorktree|wasWorktreeEmptiedByUserClose|worktreeId.*trim\(' src/renderer/src/runtime/web-session-close-intent.ts src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-base.tsRepository: stablyai/orca
Length of output: 1406
🏁 Script executed:
printf '%s\n' '--- intent ---'
nl -ba src/renderer/src/runtime/web-session-close-intent.ts | sed -n '1,220p'
printf '%s\n' '--- lifecycle ---'
nl -ba src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts | sed -n '1,180p'
printf '%s\n' '--- sync prep ---'
nl -ba src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-base.ts | sed -n '145,215p'
printf '%s\n' '--- all relevant ID declarations and constructors ---'
rg -n -g '*.{ts,tsx}' 'worktreeId\s*[:=]|worktreeId\??:|worktree_id|worktree ID|worktreeId.*(trim|normalize)|trim\(\).*worktree' src/renderer/src src/shared src/main | head -250
printf '%s\n' '--- worktreeId references in changed area ---'
rg -n -g '*.{ts,tsx}' 'worktreeId' src/renderer/src/runtime | head -250Repository: stablyai/orca
Length of output: 41469
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- relevant files ---'
rg --files src | rg 'worktree|workspace|runtime-types|session-tabs-sync' | head -250
printf '%s\n' '--- selector and ID validation symbols ---'
rg -n -g '*.{ts,tsx}' 'toRuntimeWorktreeSelector|is.*Worktree|validate.*worktree|normalize.*worktree|canonical.*worktree|worktree.*(trim|parse)|worktreeId.*(trim|normalize)' src | head -300
printf '%s\n' '--- worktree ID inputs and state construction ---'
rg -n -g '*.{ts,tsx}' 'tabsByWorktree|activeWorktreeId|worktreeId:' src/renderer/src src/shared | head -350
printf '%s\n' '--- selector definition and runtime type declarations ---'
rg -n -g '*.{ts,tsx}' 'export function toRuntimeWorktreeSelector|type Runtime.*Worktree|interface .*Worktree|worktreeId: string' src/renderer/src/runtime src/shared | head -250
printf '%s\n' '--- lifecycle callers ---'
rg -n -g '*.{ts,tsx}' 'closeWebRuntimeSessionTab\(|activateWebRuntimeSessionTab\(' src/renderer/src | head -250Repository: stablyai/orca
Length of output: 41459
🏁 Script executed:
printf '%s\n' '--- runtime selector ---'
nl -ba src/renderer/src/runtime/runtime-worktree-selector.ts
printf '%s\n' '--- snapshot apply entry and worktree ID source ---'
rg -n 'prepare.*Snapshot|apply.*Snapshot|worktreeId|snapshot' src/renderer/src/runtime/web-session-tabs-sync.ts src/renderer/src/runtime/web-session-tabs-sync src/renderer/src/runtime/web-runtime-session-snapshot.ts | head -160
printf '%s\n' '--- runtime worktree selector, catalog, and contracts ---'
rg -n 'RuntimeWorktree(List|Create)|worktreeId|id:' src/renderer/src/web/preload-api/web-runtime-worktree-catalog.ts src/shared/runtime-worktree-contracts.ts src/shared/worktree src/renderer/src/store/slices/worktrees-create*.ts src/renderer/src/store/slices/worktrees-fetch*.ts 2>/dev/null | head -240
printf '%s\n' '--- lifecycle call site context ---'
nl -ba src/renderer/src/runtime/browser-workspace-tab-close.ts | sed -n '70,115p'
nl -ba src/renderer/src/components/terminal/terminal-tab-actions.ts | sed -n '155,195p'
printf '%s\n' '--- representative worktree ID constructors ---'
rg -n -g '*.{ts,tsx}' 'id:\s*`(folder|worktree|repo)|worktreeId:\s*`|worktreeId\s*=\s*`|return\s+`(folder|worktree|repo)|\\btrim\\(\\)' src/renderer/src/store src/renderer/src/web src/shared/worktree src/shared/folder-workspace-types.ts | head -260
printf '%s\n' '--- worktree list API and ID schema candidates ---'
rg -n -g '*.{ts,tsx}' 'z\.string\(\).*worktree|worktree.*z\.string|worktreeId.*z\.|id: z\.string|id:\s*string' src/shared/runtime-worktree-contracts.ts src/shared/worktree src/preload/api src/renderer/src/web/preload-api | head -220Repository: stablyai/orca
Length of output: 41494
🏁 Script executed:
printf '%s\n' '--- shared worktree identity type and constructors ---'
nl -ba src/shared/worktree/types.ts | sed -n '45,95p'
rg -n -g '*.{ts,tsx}' 'WORKTREE_ID_SEPARATOR|::\$\{|make.*Worktree.*Id|worktree.*Id.*=.*path|id:\s*.*repo.*path|id:\s*.*worktreeId' src/shared/worktree src/renderer/src/store/slices src/main src/relay | head -240
printf '%s\n' '--- worktree identity producers ---'
rg -n -g '*.{ts,tsx}' 'id:\s*[^,]*::|WORKTREE_ID_SEPARATOR|worktreeIdFrom|toWorktreeId|createWorktree.*id|path.*worktree.*id' src | head -260
printf '%s\n' '--- snapshot result contract and producers ---'
rg -n 'RuntimeMobileSessionTabsResult|session\.tabs\.list|listSessionTabs|worktree:.*worktreeId|worktree: .*selector' src/shared src/renderer/src/runtime src/relay src/main | head -250
printf '%s\n' '--- workspace path input normalization ---'
rg -n -g '*.{ts,tsx}' 'realpath|resolve\(|normalize.*path|trim\(\)' src/shared/worktree src/renderer/src/store/slices/worktrees-create* src/renderer/src/store/slices/worktrees-fetch* src/relay/git-handler-worktree-list.ts src/relay/workspace-session-handler.ts | head -260Repository: stablyai/orca
Length of output: 41679
🏁 Script executed:
printf '%s\n' '--- canonical worktree ID implementation ---'
nl -ba src/shared/worktree/id.ts | sed -n '1,115p'
printf '%s\n' '--- runtime path normalization implementation ---'
rg -n 'function normalizeRuntimePathForComparison|export function normalizeRuntimePathForComparison' src/shared
printf '%s\n' '--- runtime snapshot contract ---'
rg -n -C 8 'type RuntimeMobileSessionTabsResult|interface RuntimeMobileSessionTabsResult|RuntimeMobileSessionTabsResult\s*=' src/shared
printf '%s\n' '--- worktree selector consumers ---'
rg -n -g '*.{ts,tsx}' 'RUNTIME_WORKTREE_ID_SELECTOR_PREFIX|startsWith\\(.id:|id:.*worktree|worktreeSelector|resolve.*Worktree.*Selector' src/renderer/src src/shared src/relay src/main | head -220
printf '%s\n' '--- concrete repo/worktree path assignment ---'
rg -n -g '*.{ts,tsx}' 'repo\.path\s*=|path:\s*.*realpath|realpathSync|realpath\(|worktreeIdForRepo|makeWorktreeId|worktreeIdFrom|createWorktreeId' src/main src/relay src/renderer/src/store src/shared/worktree | head -260
printf '%s\n' '--- repository path and ID validation entry points ---'
rg -n -g '*.{ts,tsx}' 'repoPath.*trim|path.*trim\(\)|z\.string\(\).*path|path: z\.string|worktree.*path.*(schema|parse|validate)' src/main src/relay src/shared src/renderer/src/store | head -260Repository: stablyai/orca
Length of output: 41782
🏁 Script executed:
printf '%s\n' '--- path comparison normalization ---'
nl -ba src/shared/cross-platform-path.ts | sed -n '1,90p'
printf '%s\n' '--- ID constructor callers ---'
rg -n -F 'getRepoMainWorktreeId(' src
printf '%s\n' '--- repo path input schemas ---'
nl -ba src/main/ipc/repos/repo-ipc-arg-schemas.ts | sed -n '35,105p'
nl -ba src/main/ipc/repos/repo-ipc-arg-schemas.ts | sed -n '170,200p'
printf '%s\n' '--- snapshot contract and repository-owned producers ---'
nl -ba src/shared/runtime-session-contracts.ts | sed -n '248,280p'
rg -n -F 'RuntimeMobileSessionTabsResult' src/main src/relay src/shared/runtime-session-contracts.ts
rg -n -F 'worktreeId:' src/relay src/main/runtime src/main/ipc | rg 'session|snapshot|worktree' | head -160
printf '%s\n' '--- git worktree path parsing ---'
nl -ba src/main/git/worktree-list-reader.ts | sed -n '195,260p'
printf '%s\n' '--- marker and related exact-key operations ---'
nl -ba src/renderer/src/runtime/web-session-close-intent.ts | sed -n '18,38p;158,175p'
nl -ba src/renderer/src/runtime/web-session-tabs-sync/apply-snapshot.ts | sed -n '1,65p'
nl -ba src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-base.ts | sed -n '175,201p'Repository: stablyai/orca
Length of output: 42494
🏁 Script executed:
printf '%s\n' '--- snapshot producer ---'
nl -ba src/main/runtime/orca-runtime-schedule-mobile-session-tabs-changed.ts | sed -n '1,155p'
printf '%s\n' '--- snapshot result finalization ---'
nl -ba src/main/runtime/runtime-mobile-session-result-finalization.ts
printf '%s\n' '--- runtime worktree identity passed into snapshot producer ---'
rg -n -F 'getSnapshot(' src/main/runtime
rg -n -F 'buildSnapshot(' src/main/runtime/orca-runtime-schedule-mobile-session-tabs-changed.ts src/main/runtime/orca-runtime-prune-mobile-session-tab-group-layout.ts
printf '%s\n' '--- repo path source schema and path resolution ---'
rg -n -C 3 'path: z\.string\(\)\.min\(1\)' src/main/ipc/repos src/main/ipc | head -100
printf '%s\n' '--- path trailing slash helper only ---'
rg -n -F 'function trimRuntimePathTrailingSlash' src/shared/cross-platform-path.ts
nl -ba src/shared/cross-platform-path.ts | sed -n '105,140p'Repository: stablyai/orca
Length of output: 17823
Keep the exact worktree ID in the marker set.
When a worktree path ends in whitespace, the close path can store a trimmed ID while snapshot application queries and clears with the original ID. An affirming-empty snapshot can then remove the row instead of keeping its closed-terminal tombstone. A later snapshot with that same ID cannot clear the marker either. Store the original ID and use trim() only to reject blank input.
Suggested fix
export function rememberUserEmptiedWorktree(worktreeId: string): void {
- const trimmed = worktreeId.trim()
- if (trimmed) {
- userEmptiedWorktreeIds.add(trimmed)
+ if (worktreeId.trim()) {
+ userEmptiedWorktreeIds.add(worktreeId)
}
}📝 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.
| export function rememberUserEmptiedWorktree(worktreeId: string): void { | |
| const trimmed = worktreeId.trim() | |
| if (trimmed) { | |
| userEmptiedWorktreeIds.add(trimmed) | |
| } | |
| } | |
| export function forgetUserEmptiedWorktree(worktreeId: string): void { | |
| userEmptiedWorktreeIds.delete(worktreeId) | |
| } | |
| export function wasWorktreeEmptiedByUserClose(worktreeId: string): boolean { | |
| return userEmptiedWorktreeIds.has(worktreeId) | |
| export function rememberUserEmptiedWorktree(worktreeId: string): void { | |
| if (worktreeId.trim()) { | |
| userEmptiedWorktreeIds.add(worktreeId) | |
| } | |
| } | |
| export function forgetUserEmptiedWorktree(worktreeId: string): void { | |
| userEmptiedWorktreeIds.delete(worktreeId) | |
| } | |
| export function wasWorktreeEmptiedByUserClose(worktreeId: string): boolean { | |
| return userEmptiedWorktreeIds.has(worktreeId) |
…t-side The mirror wrote its closed-last-terminal tombstone row for any host snapshot affirming zero terminals, but the host cannot tell a user close from a session it ended itself. After a remote session exited cleanly, every seeder read the tombstone as a deliberate emptying: the workspace reopened empty, with no terminal, no agent, and no error, and only an app restart recovered it. Remember the user's own emptying instead: the tab-close path marks the worktree when its close intents cover every terminal tab the row still holds, and only that marker may leave the tombstone row. A host-side exit deletes the row, so the existing after-wake ensure re-creates the surface on the next activation. The marker clears when a terminal row returns or the worktree's close intents do.
Address review feedback on the remote-workspace reseed change. - Keep the exact worktree ID in the user-emptied marker. Snapshot application checks and clears the marker with the ID it was given, so a trimmed key never matched a padded worktree ID; trim() now only rejects blank input. - Forget the marker when the host refuses a close and republishes its snapshot, and when the close RPC fails with a non-definitive error. The terminal stays alive in both cases, so the user did not empty the workspace. Tests cover the exact-ID round trip and the marker after each close outcome.
ddabb36 to
08a0a55
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve the missing-versus-empty row distinction at… · apply-preparation-base.ts:191
src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-base.ts:191
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the missing-versus-empty row distinction at
apply-preparation-base.ts:191.The
[]selected for a user-empty snapshot can compare equal to a missing row, sowithWorktreeEntryskips inserting the tombstone. Conversely, an existing empty row can compare equal to thenullselected for a host-side exit, so deletion is skipped. Wake seeding uses row presence and can therefore make the wrong reseeding decision.Distinguish nullish values from explicit empty arrays while preserving
null/undefinedequality and nonempty-row comparisons.Suggested fix
const left = a ?? [] const right = b ?? [] + if ((a == null) !== (b == null)) { + return false + } if (left.length !== right.length) {
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
082feb1a-5864-4e30-a8ac-14cc61129ed2
📥 Commits
Reviewing files that changed from the base of the PR and between ddabb367f0308d403dfb4e8e825f2c354a99f573 and 08a0a55.
📒 Files selected for processing (5)
src/renderer/src/runtime/web-runtime-session-tab-activate-close.test.tssrc/renderer/src/runtime/web-runtime-session-tab-lifecycle.tssrc/renderer/src/runtime/web-session-close-intent.test.tssrc/renderer/src/runtime/web-session-close-intent.tssrc/renderer/src/runtime/web-session-tabs-sync-emptied-workspace-tombstone.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
…session-exit-reseed # Conflicts: # src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts
… row sameTerminalTabs treated a missing row as equal to the explicit empty row, so a user-emptied workspace whose row was missing never got its tombstone. A missing row now differs from an explicit empty one; a null frame against an existing tombstone stays a no-op, so a synthesized unpublished frame cannot undo the user's own emptying.
Summary
When a remote workspace's last terminal session exits on its own (the agent process ends, the user exits the session), the client mirror wrote its closed-last-terminal tombstone row — so every seeder read the workspace as deliberately emptied, and reopening it showed an empty workspace: no terminal, no agent chat, no error. Only restarting the app recovered it. The tombstone now records only the user's own emptying; a host-side exit deletes the row and the existing after-wake ensure re-creates the surface on the next activation.
The incident
Reported twice against the same flow (a Proxmox LXC environment recipe, per-workspace cloud VM):
session-exited ... code 0, cause "exited".running, the serve stays up, the pairing connection stays healthy (verified: the app re-authenticated within seconds after a forced serve restart).Root cause
#21630taught the mirror to keep an explicit empty terminal row when a host snapshot affirms the workspace holds zero terminals, because a deleted row reads as "never initialized" and re-seeds the workspace on every focus (STA-6173). The empty row is the tombstone every seeder honors.But a host snapshot affirming zero terminals cannot distinguish two opposite situations:
Both produce the same affirming-empty frame, so both got the tombstone, and the workspace could never regain a surface mid-run. App restart escaped it only because startup hydration takes a different seeding path.
The e2e test added here reproduces the exact sequence against a headless paired runtime: create a workspace terminal, open the workspace from the sidebar, end the session cleanly server-side, reopen from the sidebar. On the pre-fix tree the host terminal count after reopen is 0 and the client renders no tabs and no errors.
The change
The client knows one thing the snapshot cannot: when a close is the user's own.
closeWebRuntimeSessionTabnow records, alongside its close intents, that this close covered every terminal tab the row still held — the user emptied the workspace. The mirror's affirming-empty decision consults that marker:ensureWebRuntimeWorktreeTerminalAfterWake, already wired to every sidebar activation) reads "never initialized", re-creates the surface on the host, and the workspace comes backThe marker clears when a terminal row returns for the worktree or when the worktree's close intents clear, so a later host-side exit still reseeds.
This stays inside
#21630's own vocabulary: removal frames and synthesized unpublished frames still delete, a never-initialized worktree still gets no row, and only positive evidence of the user's own close may tombstone.Tests
web-session-tabs-sync-emptied-workspace-tombstone.test.ts: the tombstone case now models the user's close (the old case asserted a host retraction as a user tombstone — the conflation itself); new cases for a host-side exit deleting the row and for the marker clearing once a terminal returns.paired-remote-terminal-session-exit-reopen.spec.ts(e2e, electron-headless): the incident end to end, red on the pre-fix tree, green after.typecheck:node,typecheck:web, and the changed-code quality gate pass.Scope and known edges
closeWebRuntimeSessionTabdoes not set the marker, so such a workspace may reseed on the next activation rather than staying empty. That is the pre-fix(session): remember closed and initialized runtime tabs #21630 behavior for that path only, never a strand.