Repository navigation
fix: recover deleted Active Server defaults and preserve editor hosts - #25938
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughSettings reads now reconcile the active runtime-environment preference with the saved environment registry. A registry watcher checks for removed environments, and environment listing reads settings before returning results. Editor operations now resolve routes using captured execution-host information, and editor tabs receive the matching open file’s execution-host ID. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Some folder-based editor operations can target the wrong folder when duplicate workspace IDs exist across hosts; resolve this pairing before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The changes in
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Select the folder path from the resolved owner. · editor-file-operation-owner.ts:194-196
src/renderer/src/lib/editor-file-operation-owner.ts:194-196
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSelect the folder path from the resolved owner.
If two folder records share an ID but have different hosts and paths,
resolveEditorOwnerRoutecan select the second record by host. WhenworktreePathis null, this lookup still returns the first record'sfolderPath.getEditorFileOperationContextthen pairs one host's route with another host's path. Use the same host-qualified folder owner for both values, and test same-ID folders with different paths.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
963c8691-e254-41e8-8ed6-a19c13617fd3
📥 Commits
Reviewing files that changed from the base of the PR and between 07981bd and b4cbc5c2dcc5828fec8bde0a44d65048e46743cd.
📒 Files selected for processing (11)
src/main/ipc/runtime-environment-connectivity-handlers.tssrc/main/ipc/runtime-environment-preference.test.tssrc/main/ipc/runtime-environment-preference.tssrc/main/ipc/runtime-environments.tssrc/main/ipc/settings.test.tssrc/main/ipc/settings.tssrc/renderer/src/lib/editor-file-operation-owner.tssrc/renderer/src/lib/unified-tab-host-ownership.tssrc/renderer/src/store/slices/editor-tab-host-ownership.test.tssrc/renderer/src/store/slices/editor/tabs/workspace-editor-item.tssrc/shared/runtime-environment-store.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
Co-authored-by: Chen <zwq19980411@gmail.com>
b4cbc5c to
4c35ea8
Compare
1 participant
ELI5
Deleting a saved server could leave Orca trying to use its old ID, so unrelated panes failed with “Unknown environment.” This clears the deleted default and keeps editor files and tabs attached to the machine that owns them when focus changes.
What Changed
Why
Repairing the saved preference fixes both existing stale defaults at startup and live CLI removal. Resetting to Local desktop avoids guessing which replacement server the user intended. Explicit remote file operations keep their own host and never fall back to executing locally. Editor routing reuses the existing ownership resolver instead of introducing another owner store.
Linked Issue
Refs #21489 for the reproduced deleted-default and editor-focus paths. Historical terminal/session corruption described in the comments is outside this change; see Review and Notes.
Visual Proof
Before: the deleted server ID remains selected, leaving the Active Server control blank.
After: CLI removal automatically resets the selection to Local desktop. The server list was refreshed for this screenshot.
Testing
Used
orca host list --jsonto inspect available hosts. In an isolated macOS app profile, added a disposable loopback server, seeded its saved Active Server preference through settings IPC, and removed it withorca environment rm. Before the fix, the retained selector reproduced the exactruntimeEnvironments:call/RuntimeEnvironmentStoreError: Unknown environmenterror. The rebuilt hidden app reset the running renderer automatically, restored Local desktop, and repaired a stale preference at restart.Also opened a real fixture file with a conflicting focused host: before, its file owner was local and its tab owner was the other runtime; after, both were local and the editor rendered the file contents. App identity was verified through CDP. Production host registrations were untouched; a Windows peer was checked read-only, not used for a full Windows-client/WSL reproduction.
Validation:
pnpm tc:node,pnpm tc:web, fullpnpm exec oxlint, the changed-code quality gate, formatting, andgit diff --checkpass. The 221 affected tests still pass; 17 shared live-turn tests pass after correcting two pre-existing fixture calls to the newer{ items }API in a separate test-only commit.pnpm buildpassed on the original base, including desktop, relay/CLI, web/mobile bundles, and macOS native builds. The separate mobile workspace required its frozen-lockfile dependency install first. Full repository lint/test coverage is left to CI.Review
Adversarial self-review found and fixed two P2 issues: watcher installation errors could abort runtime IPC registration (
runtime-environment-preference.ts), and suppressing folder focus without catalog disambiguation rejected valid same-ID folders on different hosts (editor-file-operation-owner.ts). Regressions demonstrate both failures and their recovery. Checked same-name replacement, unavailable/corrupt registries, offline saved hosts, SSH and nested runtime ownership, pairing-generation changes, and settings notifications.Contributor references:
Co-authored-bycommit trailer. Adapted its editor ownership approach. This version reuses the existing file-host helper, extends local/runtime/direct-SSH/nested-SSH coverage, and handles folder owner checks after focus changes. Its patch retains the existing single-runtime legacy fallback; this PR retains that compatibility behavior too.No unresolved correctness finding remains in the reviewed scope.
Agent skill upstream boundary
Notes
No remote protocol changes. Folder workspaces and SSH routes are covered. Full native Windows/Linux runs and the original Windows-client/WSL pairing flow remain untested locally. Existing terminal bindings, duplicated historical editor records, and abandoned saved host partitions are not migrated by this PR. If a native registry watcher fails, settings reads and host-list refresh still repair the preference.
Checklist
Recovery and merge scope
Existing broken Active Server preferences are repaired automatically; existing mis-owned terminals, duplicate editor records, and orphan host partitions are not migrated. #23349 is the only contributor PR superseded here. #9514, #21175, #21048, #21124, and #21125 remain separate work. #23749 / #23785 remain separate terminal-identity fixes.
The first CI runs failed on the unchanged mobile turn-disclosure test exceeding its line limit. Main fixed that in #25933; this branch is rebased onto that fix. Rebase also exposed two pre-existing shared live-turn fixture calls still using the old argument shape: their regression failed before correction, and both typechecks plus all 17 tests pass after correction. That repair is isolated in a test-only commit. Refreshed CI is green at
2faf2ce9378c: static analysis/typechecks, both packaging jobs, all five unit shards, relay integration and the aggregate gates passed. One unchanged relay subprocess test initially timed out in CI; its 28-test local suite and the failed-job rerun passed. The original Windows-client/WSL pairing flow remains untested locally.Current merge recommendation: merge this as a partial fix; refreshed CI is green. Keep #21489 open until historical host ownership drift and removed-host recovery are addressed. The implementation is contained and does not migrate historical saved-session records; local verification is green after rebase and the separate fixture correction. The original Windows-client/WSL pairing flow remains a validation gap.
Remaining root-fix review
Follow-up #25954 reproduces and prevents one server snapshot removing or rebinding another server’s terminal, including collisions across workspaces. It prevents new corruption and does not migrate old records. The rendered evidence uses synthetic hosts, not the reporter’s complete Windows/WSL pairing sequence.
#21125 needs further work before adoption: its exact recovered-draft parking function retained only 10 of 12 divergent drafts in a bounded reproduction; the reopen stack is also not durable across restart. Historical recovery must preserve all unsaved drafts before changing owners or deduplicating. #21124 remains a separate document-identity/deduplication fix.
#9514 and #21175 still address persisted partition retirement, with differing startup/ownership policies; #21175 also depends on #21113. #21048 still addresses transport retirement after CLI removal. None is superseded here. #23749/#23785 remain separate first-mount PTY identity work and retain their failed Linux validation gates. In #23749’s referenced log, the two ledger entries include a
--helpprobe and the actual launch; this needs an oracle review, not an assumption that two agents launched.Series status
#25954 now includes broader visibility-resume coverage: 320 tests across 42 suites pass, including stale-frame fencing and owner-confirmed removal. Its refreshed CI is the follow-up merge gate.
#25958 fixes a separately reproduced publication source: a local server advertised a paired host’s files and cold/legacy terminals as its own. In the production renderer, that advertisement created duplicate file records and replaced a WSL editor with a Mac-owned tab. The new producer-to-client regression preserves the original owner. Nine focused cases fail on the old publisher; 465 tests across 61 suites pass with the fix. This is prevention, not historical migration, and the complete original Windows/WSL pairing sequence remains unproved.
The remaining resolution gates for #21489 are safe recovery of historical records (including every divergent unsaved draft), reconciliation of partition retirement (#9514/#21175/#21113), transport retirement after CLI removal (#21048), and the original native Windows/WSL pairing journey. #21124’s document deduplication and #23749/#23785’s first-mount PTY identity work remain separate. No additional contributor PR is superseded by the follow-ups.
Final merge review
Recommend merging this PR at
2faf2ce9378c; its CI is green and GitHub reports it clean/mergeable. It repairs existing stale Active Server preferences and prevents the reproduced editor focus drift, while leaving historical session migration untouched.Hold draft follow-ups #25954 and #25958. Full #25954 CI exposed 12 deterministic returning-pane status failures when different hosts reuse a terminal ID; all 12 reproduce locally, including on the dependent publication branch. The receiver’s collision guard needs identity-scoping work before adoption. These findings do not touch this PR’s settings/editor implementation.