Repository navigation
Conversation
58c0e59 to
792739c
Compare
nwparker
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughRuntime file requests now carry an expected runtime-environment ID, and SSH ownership checks compare that ID with the active environment. Editor session persistence now groups eligible duplicate records and remaps references to surviving records. Closing an editor file can sweep eligible same-document siblings. Hydration and batch tab reconciliation remove clean, draft-free editor records that have no corresponding tabs. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Closing an SSH-through-runtime editor document can leave its mirrored host tab open, allowing the tab to reappear. This is a bounded close-workflow issue; the change is mergeable with owner awareness, though the close path should be corrected. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, testing, visual proof, review, and compatibility considerations. However, the Linked Issue section lists only references and does not provide the required directly linked issue in the expected form.
✨ 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 |
There was a problem hiding this comment.
Compared with BearMett’s #21124 and the editor-close fixes now on main. The combined close path preserves pinned/dirty/read-only siblings, foreign records reusing a bare ID, conflicting recorded owners, and different captured connection generations. It retains main’s backing-file deletion guards and tab-order cleanup. Nested SSH operations use the recorded runtime and target; verification reads are scoped by the complete workspace selector.
Final assembled verification: 6,313 tests pass (five skips) in 563 suites, plus 48 upstream authority tests and 81 serial compatibility tests. Typecheck and changed-code quality pass (147 files, zero findings). Fresh hidden macOS and isolated Windows/Linux each pass all four native flows. Windows: https://github.com/stablyai/orca/actions/runs/37556420437. Linux: https://github.com/stablyai/orca/actions/runs/37557129429. Current-head CI is green on 5253faa8865c; merge recommended.
The repeated ARM runner shutdowns were traced to duplicated audit bundles by module and resource logging. A separate build-time fix is under verification in #26049; it preserves application behavior. Contributor originals should close after their replacements merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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:
0b1ed5b8-6554-493d-894a-f42929b49dfc
📒 Files selected for processing (9)
config/scripts/vitest-sqlite-database-runtime-files.mjssrc/main/runtime/rpc/methods/agent-launch.tssrc/renderer/src/components/editor/closed-editor-model-lifetime.test.tssrc/renderer/src/components/editor/editor-model-host-isolation.test.tssrc/renderer/src/components/editor/monaco-cursor-line-lifetime.test.tssrc/renderer/src/runtime/close-mirrored-editor-tab.test.tssrc/renderer/src/runtime/close-mirrored-editor-tab.tssrc/renderer/src/store/slices/editor/actions/close-file-action.tssrc/renderer/src/store/slices/editor/file-ids/editor-file-ids.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.
| const capturedHost = parseExecutionHostId(getOpenFileExecutionHostId(file)) | ||
| const runtimeEnvironmentId = | ||
| file.runtimeEnvironmentId === undefined | ||
| ? getRuntimeEnvironmentIdForWorktree(state, worktreeId) | ||
| : file.runtimeEnvironmentId | ||
| file.runtimeEnvironmentId !== undefined || hasOpenFileExecutionHostEvidence(file) | ||
| ? capturedHost?.kind === 'runtime' | ||
| ? capturedHost.environmentId | ||
| : null | ||
| : getRuntimeEnvironmentIdForWorktree(state, worktreeId) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'mirroredFromRuntimeSession\s*:\s*true' --type=ts -g '!**/*.test.ts'
rg -nP -C3 'operationProvenance' src/renderer/src/runtime --type=ts -g '!**/*.test.ts'Repository: stablyai/orca
Length of output: 3230
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- PR diff: close handler ---'
git diff --unified=50 3fb72d135de28d61c0bbbda7bb3a40be363a7cae 5253faa8865cef563fc657038d62160d560eb66d -- src/renderer/src/runtime/close-mirrored-editor-tab.ts
printf '%s\n' '--- current handler ---'
nl -ba src/renderer/src/runtime/close-mirrored-editor-tab.ts | sed -n '1,180p'
printf '%s\n' '--- helper declarations and uses ---'
rg -n -C 5 'getOpenFileExecutionHostId|hasOpenFileExecutionHostEvidence|parseExecutionHostId|getRuntimeEnvironmentIdForWorktree|notifyHostOfMirroredEditorClose' src/renderer/src
printf '%s\n' '--- runtime tab builder ---'
nl -ba src/renderer/src/runtime/web-session-tabs-sync/tab-builders.ts | sed -n '75,165p'
printf '%s\n' '--- operation provenance and route data declarations/usages ---'
rg -n -C 4 'operationProvenance|runtimeEnvironmentId|externalSshTargetId|executionHostId' src/renderer/src/runtime src/renderer/src 2>/dev/null | head -n 450
printf '%s\n' '--- named test fixture ---'
rg -n -C 12 'HOSTS\[3\]|executionHostId: .ssh:target.|hub-a' src/renderer/src --glob '*editor-model-host-isolation.test.ts' --glob '*close-mirrored*' --glob '*web-session-tabs-sync*'
printf '%s\n' '--- changed files in PR ---'
git diff --name-only 3fb72d135de28d61c0bbbda7bb3a40be363a7cae 5253faa8865cef563fc657038d62160d560eb66dRepository: stablyai/orca
Length of output: 45667
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- production mirrored tab builder ---'
nl -ba src/renderer/src/runtime/web-session-tabs-sync/tab-builders.ts | sed -n '85,155p'
printf '%s\n' '--- host ownership helper ---'
nl -ba src/renderer/src/lib/unified-tab-host-ownership.ts | sed -n '1,180p'
printf '%s\n' '--- operation provenance declarations and production assignments ---'
rg -n -C 3 'operationProvenance' src/renderer/src --glob '*.ts' --glob '*.tsx' -g '!**/*.test.ts'
printf '%s\n' '--- route declarations and producers ---'
rg -n -C 4 'WorktreeOperationRoute|generation:.*route|route:' src/renderer/src/lib/editor-file-operation-owner.ts src/renderer/src/store/slices/editor src/renderer/src/runtime/web-session-tabs-sync src/renderer/src/lib/workspace-session-editor-records.ts
printf '%s\n' '--- close handler tests, relevant cases ---'
rg -n -C 9 'runtimeEnvironmentId|externalSshTargetId|operationProvenance|executionHostId|mirroredFromRuntimeSession|close intent' src/renderer/src/runtime/close-mirrored-editor-tab.test.ts
printf '%s\n' '--- runtime tab snapshot mapping and sync application ---'
rg -n -C 5 'build.*Editor|mirroredFromRuntimeSession|runtimeEnvironmentId: environmentId|tab-builders' src/renderer/src/runtime/web-session-tabs-sync --glob '*.ts' -g '!**/*.test.ts'
printf '%s\n' '--- persisted editor host fields ---'
rg -n -C 4 'externalSshTargetId|runtimeEnvironmentId' src/renderer/src/lib/workspace-session-editor-records.ts src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts src/renderer/src/store/slices/editor --glob '*.ts' -g '!**/*.test.ts' | head -n 240Repository: stablyai/orca
Length of output: 42341
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- route contract and capture implementation ---'
nl -ba src/renderer/src/lib/worktree-operation-route.ts | sed -n '1,240p'
printf '%s\n' '--- editor operation provenance implementation ---'
nl -ba src/renderer/src/lib/editor-file-operation-owner.ts | sed -n '1,90p'
nl -ba src/renderer/src/lib/editor-file-operation-owner.ts | sed -n '175,235p'
printf '%s\n' '--- operation route capture callers in open-file flow ---'
rg -n -C 3 'captureEditorFileOperationProvenance|operationProvenance:' src/renderer/src/store/slices/editor src/renderer/src/lib --glob '*.ts' --glob '*.tsx' -g '!**/*.test.ts' | head -n 220
printf '%s\n' '--- persistence owner projection and record shape ---'
nl -ba src/renderer/src/lib/editor-file-operation-owner.ts | sed -n '20,52p'
nl -ba src/renderer/src/lib/workspace-session-editor-records.ts | sed -n '1,220p'
printf '%s\n' '--- exact nested route fixture ---'
nl -ba src/renderer/src/components/editor/editor-model-host-isolation.test.ts | sed -n '24,52p'
printf '%s\n' '--- close test owner cases ---'
nl -ba src/renderer/src/runtime/close-mirrored-editor-tab.test.ts | sed -n '1,230p'Repository: stablyai/orca
Length of output: 44310
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- explicit route resolver ---'
rg -n -C 8 'resolveExplicitWorktreeOperationRouteResult|resolveActiveWorkspaceRoute|runtimeEnvironmentId' src/renderer/src/lib/worktree-operation-catalog-route.ts
printf '%s\n' '--- provenance capture route selection ---'
nl -ba src/renderer/src/lib/editor-file-operation-owner.ts | sed -n '64,112p'
printf '%s\n' '--- open-file provenance attachment ---'
nl -ba src/renderer/src/store/slices/editor/actions/open-file-apply.ts | sed -n '45,78p'
printf '%s\n' '--- persisted owner restoration ---'
rg -n -C 5 'externalSshTargetId|runtimeEnvironmentId' src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts src/renderer/src/lib/workspace-session.ts src/renderer/src/shared/workspace-session-state-types.ts 2>/dev/nullRepository: stablyai/orca
Length of output: 10262
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- explicit route resolver implementation ---'
nl -ba src/renderer/src/lib/worktree-operation-catalog-route.ts | sed -n '1,145p'
printf '%s\n' '--- exact worktree route implementation ---'
rg -n 'function resolveExactWorktreeRoute|export function resolveExactWorktreeRoute' src/renderer/src/lib/worktree-owner-route.ts
nl -ba src/renderer/src/lib/worktree-owner-route.ts | sed -n '1,180p'Repository: stablyai/orca
Length of output: 8592
Use the captured runtime owner when closing mirrored files.
When a mirrored file has an SSH route with a runtime owner, getOpenFileExecutionHostId(file) returns the SSH host. The current branch then sets runtimeEnvironmentId to null and returns false without recording a close intent. The host tab remains open, so its next snapshot can reopen the file. Persisted files with both runtimeEnvironmentId and externalSshTargetId reach the same branch.
Use the runtime ID from the captured route first. Otherwise, use the explicit file runtime ID. Keep worktree inference only when neither is available.
🐛 Suggested fix
-import { parseExecutionHostId } from '../../../shared/execution-host'
import {
- getOpenFileExecutionHostId,
hasOpenFileExecutionHostEvidence
} from '@/lib/unified-tab-host-ownership'
@@
- const capturedHost = parseExecutionHostId(getOpenFileExecutionHostId(file))
- const runtimeEnvironmentId =
- file.runtimeEnvironmentId !== undefined || hasOpenFileExecutionHostEvidence(file)
- ? capturedHost?.kind === 'runtime'
- ? capturedHost.environmentId
- : null
- : getRuntimeEnvironmentIdForWorktree(state, worktreeId)
+ const capturedRoute = file.operationProvenance?.generation.route
+ const runtimeEnvironmentId = capturedRoute
+ ? capturedRoute.runtimeEnvironmentId
+ : file.runtimeEnvironmentId !== undefined
+ ? file.runtimeEnvironmentId
+ : hasOpenFileExecutionHostEvidence(file)
+ ? null
+ : getRuntimeEnvironmentIdForWorktree(state, worktreeId)
Adapt the document identity, persistence dedupe, and orphan boundaries from #21124. Preserve current workspace-selection behavior, scope closes when legacy IDs repeat across workspaces, and use the captured file owner for mirrored closes. Co-authored-by: 김영민 <agomdole@gmail.com>
5253faa to
4979062
Compare
nwparker
left a comment
nwparker
left a comment
There was a problem hiding this comment.
Adversarial self-review of rebased head 4979062c7e61.
Rebased onto main 00984ccb2504, including #24863 and #26088. The rebase initially lost an editor matching guard that existed only in an earlier merge resolution. Restored it in a separate commit: raw document identity, recorded ownership, and captured connection/pairing generations must all match before sibling records close together. Existing main close behavior is preserved. Low–medium risk once current-head CI passes; merge before #25969.
Validation of the assembled rebased application tree: 6,117 tests passed (four skips), full typecheck, zero changed-code quality findings across 151 files, fresh app/CLI builds, and all four hidden macOS flows. The final main refresh is CI-only and its 82 planner tests pass. Earlier Windows/Linux native runs were on the previous base; current-head CI must pass before merge.
Contributor references and coauthor trailers are preserved. The original damaged Windows+WSL profile and a native nested SSH recovery/save journey remain unverified; this does not justify closing #21489. No PRs were merged or superseded contributor PRs closed during this rebase.
This branch has not been deployed
1 participant
ELI5
Closing a file could leave duplicate records behind, so the supposedly closed editor returned on the next snapshot or restart. Closing a document now removes its clean duplicates while preserving pinned siblings and unsaved work.
What Changed
Use one document identity for persistence and close decisions: workspace, path, execution owner, and read-only behavior. Merge equivalent saved records while retaining divergent drafts and their disk baselines. Prune clean editor records only after complete hydration or batch reconciliation proves they have no tab. Close every matching mirrored host tab using the file’s captured owner. Reads and writes to SSH reached through a paired host retain that recorded runtime and target, including during verification. Conflicting owner records survive until explicitly resolved or closed.
Why
Fixing only the visible tab leaves the backing record available to resurrect it. This addresses the backing records and their writers. Missing workspace inventories remain unknown; dirty, pending-draft, pinned, and read-only siblings survive. A single-workspace reconcile does not prune records during an in-flight open.
Adapted from BearMett’s #21124; 김영민 is credited as coauthor. Adversarial review found and fixed an additional cross-workspace bare-ID deletion, preserved current stale-selection/floating-workspace behavior, and corrected mirrored closes when workspace focus names another host. Historical owner recovery is a separate follow-up.
Linked Issue
Refs #21489, #21121, #17185, #17778, #16205.
Visual Proof
Real hidden Electron renderer and actual close button, with duplicate records injected into a CLI-created disposable workspace backed by a real local file. Before: one duplicate editor remains after closing its sibling. After: the editor is gone and the workspace shows its terminal.
Testing
4979062c7e61; wait for current-head checks before merging.Review
Compared with BearMett’s #21124 and the editor-close fixes now on main. The combined close path preserves pinned/dirty/read-only siblings, foreign records reusing a bare ID, conflicting recorded owners, and different captured connection generations. It retains main’s backing-file deletion guards and tab-order cleanup. Nested SSH operations use the recorded runtime and target; verification reads are scoped by the complete workspace selector.
The series includes local/direct SSH isolation, folder workspaces, and remote authority coverage. Real Windows+WSL reproduction of the original damaged profile and a native nested SSH recovery/save journey remain unverified. #25938 and the independent CI memory fix #26049 are merged.
Rebased onto main
00984ccb2504, including #24863 and #26088. The rebase initially lost an editor matching guard that existed only in an earlier merge resolution. Restored it in a separate commit: raw document identity, recorded ownership, and captured connection/pairing generations must all match before sibling records close together. Existing main close behavior is preserved. Low–medium risk once current-head CI passes; merge before #25969.Agent skill upstream boundary
Checklist