Repository navigation
Allow paired-server files to drop into their own terminals - #26931
Conversation
|
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; 8 remain after this review. 📝 WalkthroughWalkthroughThe workspace drag-source writer now maps a local execution host to a runtime execution-host ID when the worktree has an explicit runtime environment ID. Tests cover terminal drops from workspaces with no runtime owner or a paired hub owner while the active runtime environment is foreign-hub. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable issue remains in the supplied review evidence; the change is mergeable after normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description explains the user impact, implementation, rationale, and testing. However, it omits several required template sections, including an actual linked issue, Visual Proof or N/A, AI Disclo… | Add the missing required sections and complete them. Provide an actual linked issue under “Linked Issue,” add Visual Proof or explicitly state N/A with a reason, complete the testing and checklist items, and address the remaining template s… |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the primary change: allowing paired-server files to drop into their own terminals. |
Full details: Description check
Explanation
The description explains the user impact, implementation, rationale, and testing. However, it omits several required template sections, including an actual linked issue, Visual Proof or N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. It references merged PR #26830 instead of the required issue link.
Resolution
Add the missing required sections and complete them. Provide an actual linked issue under “Linked Issue,” add Visual Proof or explicitly state N/A with a reason, complete the testing and checklist items, and address the remaining template sections or mark them not applicable.
- Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
- Commit to this branch
- Create a new PR
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
Follow-up to the full-review finding on merged #26830. Current head The same real Source Control row/terminal fixture fails against the uncorrected main writer: it stamps 203 tests/19 files, full typecheck, oxlint/changed-code gates and fresh CLI/relay/Electron/server builds pass on current main077; all nine unchanged server native targets verify. The original finding’s reviewer confirmed the correction, but the original PR merged before it could receive this commit. @coderabbitai review |
|
|
Review handoff: the reported paired-local ownership finding is fixed. The original reviewer confirmed the correction at7ccc095; all three changed files are byte-identical at this PR’s23e7503 head. Final manual review,203 tests and matching real rendered failure/success pass. CodeRabbit’s new-PR review was rejected by the repository’s review quota: it reports the next included review in25 minutes (about18:39UTC). No fresh automated-review success is claimed. CI guards pass; static analysis remains queued/running at this checkpoint. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
What the user sees
Before: in a browser client connected to a paired Orca server, dragging a changed file from Source Control (or the combined diff) into a terminal of that same workspace showed "file is on a different host" and pasted nothing. Dragging the same file from the File Explorer worked.
After: the Source Control drag pastes the path just as the File Explorer drag does. Drops from a different machine are still refused.
What changed
Every file drag records which machine the file lives on, and since #26830 the terminal compares that with the machine it runs on. A paired server's own workspaces can reach the client marked "local" (local to the server) together with a separate "owned by server X" field. The terminal already counts "local + server X" as server X. The Source Control drag ignored the server field and labelled the file as this computer's.
The Source Control drag now applies the same rule as the terminal: "local" plus an explicit server owner means that server. It uses the existing owner lookups and adds no new resolver. Desktop-local workspaces still record "local", even when another server is focused. SSH workspaces are unchanged.
Side effects, both in the safe direction: native-chat drops already expected the server's ID, so Source Control drags into chat now work as well. Dropping such a file on the browser pane, which would have opened a same-named file on this computer, is now refused.
Why this approach
The fix belongs where the drag is labelled, so that every drop target sees the same answer. Loosening the terminal check would weaken the protection #26830 added. The drag label only passes between parts of the same window and is never sent to a server, so mixed client and server versions are unaffected.
Testing
Follow-up to #26830.