Repository navigation
Keep workspace file drops on the terminal's host - #26830
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkspace file-drag metadata now includes an optional runtime-environment ID alongside the workspace and execution host. File-explorer selections resolve an owner only when their paths share compatible ownership. Internal terminal drops validate source ownership against the target host and runtime environment before capturing the drop target. Rejected drops return a translated same-host message. Tests cover ownership resolution, accepted and rejected drops, and localized messages. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Dragging files from a local workspace that a managed server owns into that server's terminal can be wrongly refused with the "same host" message. Fix the source host stamping before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 21 files. (6 skipped: 6 unsupported.)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/ssh-orcad-terminal-drop-owner.spec.ts (1)
190-221: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the managed-to-desktop drop case assert the refusal.
The poll on lines 190-198 accepts either outcome: a refusal toast, or a pasted
filePath. If the drop is pasted, theelsebranch runs. Line 221 then fails, but only after the shell has already runcaton the desktop file. Line 220 can still pass when the desktop and remote files have the same path. The refusal is the required behavior for this test, so test for it directly. Wait for the toast. Then assert that the terminal output afterREMOTE_DROP_RESULTdoes not containfilePath. Remove theelsebranch.Proposed fix
- await expect - .poll(async () => { ... return refused > 0 || echo.includes(filePath) }) - .toBe(true) - const refused = page.getByText('Drop files from the same host as this terminal.', { - exact: true - }) - if (await refused.count()) { - await page.keyboard.press('Control+c') - await expect(refused).toBeVisible() - } else { ... } + const refused = page.getByText('Drop files from the same host as this terminal.', { + exact: true + }) + await expect(refused).toBeVisible() + expect( + ((await terminalText(page, floatingId)).split('REMOTE_DROP_RESULT').at(-1) ?? '') + ).not.toContain(filePath) + await page.keyboard.press('Control+c')
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
700a7659-9bd8-46a4-b757-2f51025a6f00
📒 Files selected for processing (26)
.github/workflows/e2e.ymlconfig/scripts/ci-e2e-job-selection.mjsconfig/scripts/ci-e2e-job-selection.test.mjsconfig/scripts/pr-e2e-source-routing.mjssrc/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-row-drag.test.tsxsrc/renderer/src/components/right-sidebar/FileExplorerRow.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsxsrc/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsxsrc/renderer/src/components/terminal-pane/terminal-drop-handler.tssrc/renderer/src/components/terminal-pane/terminal-drop-internal-handler.test.tssrc/renderer/src/components/terminal-pane/terminal-drop-internal-rejection-message.tssrc/renderer/src/components/terminal-pane/terminal-drop-local-workspace.test.tssrc/renderer/src/components/terminal-pane/terminal-drop-runtime-catalog-owner.test.tssrc/renderer/src/components/terminal-pane/terminal-drop-source-owner.test.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/fr.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/lib/workspace-file-drag-source-runtime.test.tssrc/renderer/src/lib/workspace-file-drag-source.tssrc/renderer/src/lib/workspace-file-drag.test.tssrc/renderer/src/lib/workspace-file-drag.tstests/e2e/ssh-orcad-idle-exit.spec.tstests/e2e/ssh-orcad-terminal-drop-owner.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
3773300 to
6d0355f
Compare
|
Addressed the refusal assertion finding in The strengthened rendered test passes all three runs (50.7 seconds), including desktop and managed same-host controls. The rebased ownership/routing subset passes 149 tests across 10 files; full typecheck, lint, changed-code checks, fresh CLI/relay/Electron builds and commit hooks pass. Rendered test receipt. @coderabbitai review |
|
6d0355f to
b52d712
Compare
059769d to
faee64c
Compare
|
Review-loop follow-up on 148 affected tests / 16 files pass, full typecheck/lint/changed-quality/build/hooks pass, and all three real rendered shell controls pass (50.4 seconds). Previous CI failures are retained and described in the PR. @coderabbitai review |
|
CodeRabbit has paused automatic reviews after the earlier commits. Please review the complete current diff at @coderabbitai full review |
✅ Action performedFull review finished. |
|
faee64c to
f84c269
Compare
|
Rebased onto current main The previous CI Docker lane passed 19 cases and failed only this spec’s screenshot wait after the refusal and no-path assertions passed. It waited on toast animation promises until the toast disappeared. The test now checks mounted/full-opacity and captures the refusal immediately, before shell cancellation. Ownership assertions and both real same-host shell controls remain. 161 focused tests, full typecheck, oxlint, changed-code checks, fresh CLI/relay/Electron/server builds and hooks pass. Three actual hidden-renderer/Linux-host repetitions pass (57.5 seconds); screenshot inspected. No product code changed during this rebase follow-up. @coderabbitai review |
✅ Action performedReview finished.
|
A workspace file dragged into a terminal now pastes only when its source host (and paired-server namespace) matches the receiving terminal's transport; otherwise the drop is refused with a toast.
f84c269 to
b0b0ce3
Compare
|
Rebased onto main The previous Docker lane passed22 cases; the mounted refusal toast stayed opacity0 until expiry in the hidden Linux renderer. Trace and failure screenshots inspected. The ownership proof now uses the supported reduced-motion preference while keeping the full-opacity/direct-refusal/no-foreign-path checks and both same-host shell controls. Three actual repeats pass52.6s; final fresh-build drop and terminal-root controls pass2/2 in33.6s.170 affected tests and final132 neighbor tests, tc/quality/builds/hooks pass. @coderabbitai review |
✅ Action performedReview finished.
|
|
Reviewing the current published head I am qualifying this published head with fresh focused tests and a freshly built hidden-renderer drop journey. The previous proof spec is used locally for validation; it was removed from the published branch by the concurrent update. @coderabbitai full review |
✅ Action performedFull review finished. |
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:
398de439-900a-4b8f-a14e-d1becf65d926
📒 Files selected for processing (21)
src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-row-drag.test.tsxsrc/renderer/src/components/right-sidebar/FileExplorerRow.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsxsrc/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsxsrc/renderer/src/components/terminal-pane/TerminalPaneHeaderOverlay.test.tsxsrc/renderer/src/components/terminal-pane/terminal-drop-handler.tssrc/renderer/src/components/terminal-pane/terminal-drop-internal-handler.test.tssrc/renderer/src/components/terminal-pane/terminal-drop-internal-rejection-message.tssrc/renderer/src/components/terminal-pane/terminal-drop-local-workspace.test.tssrc/renderer/src/components/terminal-pane/terminal-drop-runtime-catalog-owner.test.tssrc/renderer/src/components/terminal-pane/terminal-drop-source-owner.test.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/fr.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/lib/workspace-file-drag-source-runtime.test.tssrc/renderer/src/lib/workspace-file-drag-source.tssrc/renderer/src/lib/workspace-file-drag.test.tssrc/renderer/src/lib/workspace-file-drag.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.
|
Published review fix at Addressed the full-review finding: Source Control/combined-diff source stamping now converts a projected local workspace owner to its explicit server’s canonical runtime host. The real source writer and terminal handler reproduce the false refusal before the fix, then accept the paired-local drop afterward. The desktop control stays local with a foreign server focused. 203 tests/19 files, full typecheck, oxlint/changed-code gates, fresh E2E build and hooks pass. Three actual Files-drop journeys pass51.4s. A further real Source Control drag is fenced by its own shell marker and reads the managed host’s contents (17.2s). No native OS drag or Windows runtime claim. @coderabbitai review |
|
…cs (#30897) The daily full E2E run on main fails these specs every time because the product moved on and PR CI only runs specs a PR touches: - editor header menu renamed to File actions (#30129) - terminal attention: an on-screen bell leaves no unread marker (#28637) - terminal file drops carry their source host (#26830) - single-choice questions auto-submit (#26288) - phone terminal rows carry customTitle (#30037) - gh/git preflight re-reads the login shell PATH (#25183) - board drag starts after the target column is in view - chat history anchor read after scroll settles
ELI5
Dragging a remote file into a desktop terminal could paste a path naming an unrelated desktop file. The terminal now refuses files from another or unknown host with “Drop files from the same host as this terminal.” Same-host drops still work across workspaces.
What Changed
The internal drop handler compares the existing source metadata with the receiving terminal’s transport before writing any path. A managed server’s local terminal belongs to that server; an optional server ID also distinguishes identical SSH target IDs owned by different servers. Explorer selections retain the cached rows’ owner, and Source Control and combined-diff drags require known workspace ownership. Existing stale-pane checks, path limits, and shell escaping remain in use. The refusal is translated in all six locales.
Why
An absolute path identifies a file only on its owning host. Checking the receiving transport and existing drag metadata prevents the active workspace from assigning another host’s file to the terminal. Copying files between hosts would require a separate transfer flow.
Linked Issue
Internal contribution found during SSH testing; no separate issue. The former parent #26668 has merged. This PR targets main directly; the template asks maintainers to avoid needless issues.
Visual Proof
Before: the managed Files drag names its remote owner, but the desktop terminal reads DESKTOP_BROWSER_DROP_OWNER. An independent managed-host read returns REMOTE_BROWSER_DROP_OWNER.
After, freshly built published head
b0b0ce387c777efa29ac49a71120f377140bc56f: the drop shows the refusal and inserts no foreign path. Desktop-to-desktop and managed-to-managed controls still read their own contents.Testing
Fresh qualification of published head
b0b0ce387c777efa29ac49a71120f377140bc56f:Native Windows and native OS dragging were not exercised locally.
Review
P1 / Medium merge risk. Self-review checked rejection before writes, nested SSH server identity, unknown owners, same-host acceptance, stale panes and partial writes, bounded decoding, cache provenance, localization, and optional-field compatibility. No remaining actionable finding.
CodeRabbit’s incremental review explicitly covers the published head and reports no actionable comments / Minimal reviewer risk. The subsequent full review found a false refusal for Source Control/diff files whose workspace is marked local on its paired server. This PR merged at
b0b0ce387c777efa29ac49a71120f377140bc56fbefore the correction was published. The correction is published in #26931, based on current main, with matching rendered before/after proof and regression tests. Project merge risk remains Medium because ownership checks span drag producers and terminal transports.The generic docstring and issue warnings conflict with this repository’s concise-comment and maintainer issue guidance; no boilerplate comments or invented issue were added.
Agent skill upstream boundary
Notes
The new drag field is optional and preserves payload version 1. No RPC, stream opcode, or persistent-state change. Nested SSH drops without verifiable server identity are refused. Fixtures use owned disposable files.
The unpublished local candidate
6ad7398e45bcwas preserved after a protected push rejected the concurrent branch change. Its results are not attributed to this published head; the fresh results above qualifyb0b0ce387c777efa29ac49a71120f377140bc56fdirectly.Checklist