Repository navigation
Conversation
7247e2b to
453ccaf
Compare
|
The final branch is The shared owner comparison is also used by the terminal-drop fix. Local review found no remaining production issues. Validation: 172 unit tests across 14 files, both actual Explorer cases repeated three times each, and all 15 broader SSH cases pass. The parent independently passes its rebuilt 13-case lane. Before/after images and parent-red/final-green receipts are in the PR description. @coderabbitai review |
✅ Action performedReview finished.
|
|
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; 3 remain after this review. 📝 WalkthroughWalkthroughFile-explorer drag-and-drop now passes source host and workspace metadata to the move handler. The handler checks source and destination ownership before a move and reports a translated error when hosts do not match. A shared host-matching function also validates terminal drops. Tests cover metadata propagation, host validation, and managed workspace moves. CI routes the new Docker-backed end-to-end test through the dedicated OrCAD job. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No merge-blocking issue was identified in the reviewed change; normal checks remain appropriate. 🚥 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 |
059769d to
faee64c
Compare
453ccaf to
39f7799
Compare
|
Updated child 114 affected tests / nine files, typecheck/changed-quality/build/hooks pass. Both Explorer cases and the terminal case each pass three actual renderer/shell repetitions (9/9, zero skips, 2.1 minutes). The original unchanged memory-threshold CI failure is documented and passes locally without a product patch. @coderabbitai review |
✅ Action performedReview finished.
|
faee64c to
f84c269
Compare
f84c269 to
b0b0ce3
Compare
|
Closing this one. Thanks for the careful work, but the cost-benefit doesn't hold up.
If cross-instance drags turn out to matter, a smaller follow-up could refuse a drop only when it carries a stamp naming a different host. That would never block an unstamped same-workspace move. |
2 participants
ELI5
Dragging a remote file into desktop Files could move a different desktop file at the same address. Files now refuses the move and says “Move files from the same host as this workspace.” Same-host moves still work on desktop and managed hosts.
What Changed
Directory-row and background drops pass the decoded drag-source owner into the existing move handler. Before moving or asking editors to save, the handler compares that owner with both cached source and destination owners. Background destinations use their directory-cache owner. The existing workspace-generation checks, editor transaction, undo/redo, path limits and self-drop protection remain in use. Terminal and Explorer drops share the same host comparison, including nested SSH server identity. The refusal is translated in all six locales.
The Docker lane runs both real Explorer drop cases. Main (#26838) now contains the existing catalog-recovery fixture correction: a desktop checkout makes its host-header assertion exercise two populated hosts; the sidebar intentionally omits headers for a single populated host. The original host-name and folder-owner assertions remain.
Why
An absolute path identifies a file only on its host. Looking up the dragged path in the destination tree can find an unrelated file. Checking the existing source metadata preserves the file's actual owner and reuses the terminal-drop rule. Transferring files between hosts would need a separate copy flow.
Linked Issue
Found during SSH/Remote testing; internal contribution, no separate issue. Stacked on #26830 to reuse its optional nested-SSH source namespace. Merge #26830 first. Parent is rebased onto main
049bf808ab24. Latest child head39f779916c32d0b03c0a350ba4363edb5ea9b1d9follows parentfaee64cc483f; both fix cancelled toast-animation proof waits, and the parent fixes missing source metadata in the header positive fixture. Production assertions remain intact.Visual Proof
Before: an actual managed Files row supplies its runtime owner. The desktop drop moves DESKTOP_MOVE_OWNER into MOVE_DESTINATION, while independent managed reads still return REMOTE_MOVE_OWNER. The moved desktop file is opened through Files below.
After: the same drop shows the refusal, leaves the desktop source intact and creates no destination file.
Testing
Exercised hidden macOS Electron renderers and an owned Linux Docker SSH host. HTML5 drag events use the actual source row and destination surface, followed by independent filesystem/server reads. This does not claim a physical OS drag journey.
Matching rebuilt parent fails after moving the desktop marker; seven ownership regressions fail on the parent.
172 focused tests across 14 files pass: source/destination provenance, nested SSH namespaces, unknown owners, stale generations, guarded undo/redo, row/background metadata dispatch, terminal compatibility, editor moves, native imports and CI routing.
Both rendered Explorer cases pass all three repetitions each, six total in 1.5 minutes. Every case verifies cross-host refusal plus desktop and managed same-host moves, with actual owner metadata and source/destination contents.
Full
pnpm tc,pnpm exec oxlint, changed-code quality, localization catalogs, fresh CLI/relay/Electron builds and commit hooks pass.The broader conversion/update/stdio/port/catalog/idle/reboot/menu/terminal-drop/Explorer-drop suite passes all 15 cases, with zero skips, in four minutes: explorer-move-owner-broader-runtime.log.
Latest rebased fixture follow-up passes 114 affected tests / nine files (including the unchanged memory test), full typecheck, changed-code quality, fresh E2E build and hooks. Both Explorer cases and the inherited terminal case each pass all three real rendered repetitions: nine total, zero skips, 2.1 minutes.
Review
P1 / Medium merge risk; parent #26830 is also Medium. Self-review checked rejection before writes and editor-save requests, cached directory provenance, same-host acceptance, namespace collisions, stale generations, undo/redo, bounded decoding, folder workspaces, localization and platform assumptions. No remaining production findings. CodeRabbit reviewed the complete 22-file child diff at
453ccaf24255and reports no actionable comments / Minimal reviewer risk. The project merge-risk estimate stays Medium because the shared owner comparison also serves terminal drops. Previous-head CI failed on the inherited header fixture and cancelled toast-animation waits in both drop specs. Those fixtures are corrected. The unchanged flatten-retained-slice heap threshold also failed in Node shard 5/5; it passes in the latest local affected suite, with no speculative product change. CodeRabbit’s latest review explicitly covers39f779916c32d0b03c0a350ba4363edb5ea9b1d9and reports no actionable comments / Minimal reviewer risk. Fresh CI remains queued or running. The generic docstring-coverage warning remains, following the repository’s concise-comment rule.Agent skill upstream boundary
Notes
No RPC, stream opcode or persistent-state change. The source metadata remains version 1; the optional server field comes from #26830. Native Windows and physical OS drags have not been exercised locally. All mutation fixtures are disposable and owned by the tests.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred). Local gates are listed above; CI covers remaining checks.