Repository navigation
Remove legacy OS file-drop routing (STA-6940 PR6/6) - #26385
Conversation
…2025/sta-6940-pr3-chat-composer-feedback
…lumbing' into brennanb2025/sta-6940-pr3-chat-composer-feedback
…-pr2-drop-plumbing
Preserve element-owned drops while integrating the upstream multi-file paperclip picker and all-or-nothing attachment limit. Resolve NativeChatComposer through its existing file-drop hook, keep the host-qualified attachment resolver with the current workspace state type, retain captured destinations and unmount checks through attachment reads and uploads, and keep attachment actions picker-only without a broadcast subscriber. Update picker fixtures and the upstream subscriber test. Validation: web and Node typechecks, full Oxlint on 39 PR3 files, and 372 tests across 45 explicitly named files pass.
…omposer-feedback' into brennanb2025/sta-6940-pr4-terminal-owners
Resolve NativeChatComposerField imports: keep PR3's flushSync (interrupted composition settlement) and drop the old ImageOff notice import that main's notice card replaced. Adapt PR3's IME file-drop test to main's notices and @-file suggestion props.
…or own OS drops STA-6940 PR5. The file explorer tree, the project sidebar, each editor group's tab strip and its editor area now take OS file drops on their own element through the shared owner hook, with identity from their own render (the explorer's shown workspace and target row, the tab strip's and editor group's worktree and group) instead of the active worktree. The explorer and sidebar broadcast subscribers and their legacy drop markers are deleted; editor-open moves out of useGlobalFileDrop into editor-dropped-file-open, which the legacy unmarked-chrome route still uses until PR6.
Review summaryThis PR went through an independent code review, a fix round, a focused re-review, and a rendered check in a real Orca window. The reviewers and the rendered-check agent did not write the code. Their reports are summarized here because they were kept off GitHub while the work was in progress. First review (head
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (54)
💤 Files with no reviewable changes (19)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe main window now registers dropped-path preparation instead of the native file-drop relay. The preload and UI APIs no longer provide the legacy file-drop subscription, and shared native-drop target and payload helpers have been removed. Renderer code no longer installs the global drop hook. Tests now cover preparation IPC and queue behavior, OS file-drop ownership, and cancellation of unowned drops. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established; normal checks can proceed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
…ment Upstream removed preload's document-level file-drop routing (stablyai#26385), which the offscreen page hooked to claim drops. The page element now listens for drops itself, like Orca's other drop owners, and the preload bridge resolves the files to paths for main. Claude-Session: https://claude.ai/code/session_01DvNJ16LEPYmdEkpQWtDL7n
…ment Upstream removed preload's document-level file-drop routing (stablyai#26385), which the offscreen page hooked to claim drops. The page element now listens for drops itself, like Orca's other drop owners, and the preload bridge resolves the files to paths for main. Claude-Session: https://claude.ai/code/session_01DvNJ16LEPYmdEkpQWtDL7n
…ment Upstream removed preload's document-level file-drop routing (stablyai#26385), which the offscreen page hooked to claim drops. The page element now listens for drops itself, like Orca's other drop owners, and the preload bridge resolves the files to paths for main. Claude-Session: https://claude.ai/code/session_01DvNJ16LEPYmdEkpQWtDL7n
…ment Upstream removed preload's document-level file-drop routing (stablyai#26385), which the offscreen page hooked to claim drops. The page element now listens for drops itself, like Orca's other drop owners, and the preload bridge resolves the files to paths for main. Claude-Session: https://claude.ai/code/session_01DvNJ16LEPYmdEkpQWtDL7n
…ment Upstream removed preload's document-level file-drop routing (stablyai#26385), which the offscreen page hooked to claim drops. The page element now listens for drops itself, like Orca's other drop owners, and the preload bridge resolves the files to paths for main. Claude-Session: https://claude.ai/code/session_01DvNJ16LEPYmdEkpQWtDL7n
ELI5
Files dropped from the operating system could reach a hidden chat, the active terminal instead of the pane under the cursor, or the active workspace instead of the one under the drop. Preload cancelled the real event, guessed its destination from element attributes, and sent it through main to window-wide subscribers; the remaining unowned areas still defaulted to opening files in the active editor.
This is PR 6 of 6 for STA-6940. PR1–PR5 are merged on main: the earlier PRs established ownership for chat, composers, feedback, terminals, explorer, sidebar, tab strips, and editors. This final PR removes the old route: dropping on empty chrome, the dashboard popout, or a terminal divider silently does nothing and shows the not-allowed cursor. Drops on an owned surface continue to go once to that surface.
PR5 merged as
d1e7d15f4c41b6d3245dba3e7b96e9865da49639; this PR has no open PR dependency. The final sync merged main commitb0fbcfd6a7e919743856dcd96198897baa92593e. The final merge preserved the 54-file PR6 diff exactly, with no overlapping-main exceptions. CI then found that removing the old drop subscription also removed an object used by an existing OS-resume test; a test-only correction keeps that object empty while the removed subscription stays absent.What Changed
window.api.ui.onFileDrop, its web stand-in, and both legacy Electron channels.fs:prepareDroppedPathsrequest. Keep its path limits, sender validation, macOS temporary-file copies, globally serialized copy queue, reload abort, expiry, and replacement-window cleanup.useGlobalFileDropand its app-shell mount, removing active-editor fallback and global rejection toasts. Move the rejection-message formatter and its tests tonative-file-drop-rejection-messagebecause the owners still report failures they accepted; keep existing translation keys and editor-open code.Why
The element receiving the browser event already knows its destination. A parallel window-wide route recreates the ambiguity the first five PRs removed. Unowned areas need only cancellation; they should neither select a destination nor produce a refusal toast.
Retained differences from the common pattern, as approved in the plan:
The preload release rule and legacy attributes were temporary stack scaffolding and are now gone.
Linked Issue
STA-6940: deliver operating-system file drops to the element under the cursor
Visual Proof
A separate QA agent ran this branch (head
b1b93fb8af0, before the test-only fix round) as a hidden Orca dev window with an isolated profile and two folder workspaces (ws-a and ws-b), and simulated OS file drops through the Chrome DevTools Protocol. 10 of 11 scenarios passed and 1 was not applicable: terminal pane body, the gap between two panes, empty window chrome, explorer folder rows in both workspaces, the editor area, the tab strip, the project sidebar, the new-workspace card, and Orca's own explorer and tab drags. Not applicable: a drop on an inactive pane's title bar, because this build only shows a title bar on the active pane (covered by DOM-level tests). No message or toast appeared in any scenario.1. File dropped on the left pane while the right pane is active. The quoted path lands at the left pane's prompt; the right pane is untouched and stays active.
2. File dropped on the gap between the two panes. Nothing happens: no path in either pane, no editor tab, no message. The same was checked on empty window chrome (top strip and status bar). Before this PR, a drop no surface owned could still be sent to the editor.
3. Explorer drop after switching to ws-b. The file lands in ws-b's
inbox, the workspace the explorer shows.4. File dropped on the editor content area. It opens as its own tab (
my report.txt); the already-openreadme.txtis unchanged.Not covered: real SSH, WSL and remote-runtime hosts, Linux and Windows (unit fixtures only).
Testing
Final GitHub Actions: passed: full run 37719775266, attempt 2 completed successfully, with all ten unit shards, relay integration, static analysis/typechecks, Linux and Windows packaging, and aggregate verification green; used one full rerun after the known Windows writer-stall flake, whose test is byte-identical to main (head
f1e8f1936dba63305ed7c4f3cb02b439c14508a3).I manually tested these changes locally
Automated tests updated and run.
The final review fixes change tests, comments, and obsolete timing entries only; the comment correction and final main sync introduce no additional production behavior change.
node_modules/.bin/vitest run --config config/vitest.config.tswith a non-empty, existence-checked Bash array containing all 77 requested suites, the surviving conflict paths, and all five suites using the terminal global-effects test helper; the two deleted legacy conflict paths were verified absent.orca-ci-checks: application typechecks passed, followed by 22/22 static checks passing on the committed merge with--no-typecheck, including lint, code-quality, React checks, and localization verification. The initial uncommitted-merge React check included main changes through the old ancestry; the final check uses the merged main commit and passes.git diff --checkpassed; the install-added@pnpm/exelockfile block was restored from main.tests/andsrc/found no readers of the removed attribute.pnpm run typecheck:e2e: it still fails with 245 existing diagnostics in 136 other files, with zero diagnostics in the three edited specs. The config documents that this repository-wide check is not yet enforced. Corrected the title spec's existing nullable lookup, which accounted for the initial run's additional error.Both new assertions failed before removal on PR5 head
2ef4f37349a. The finalized fixtures were also replayed with the baseline preload/shared modules restored from that head: two failures, followed by two passes after restoring the final sources (the owner case was excluded by the regression filter):src/preload/preload-file-drop-absence.test.ts: preload must register no document drop or dragover listener (PR5 registered both).src/renderer/src/lib/os-file-drop-preload-integration.test.tsx, “refuses unowned chrome without opening, uploading, or showing a toast”: an unowned drag/drop must end atdropEffect = none, without path resolution, preparation or relay delivery (PR5 kept the copy cursor).terminal-pane-element-file-drop.test.tsxadditionally asserts divider and unowned-chrome drops are cancelled withdropEffect = none, with no editor open, upload, preparation, terminal delivery or toast. The divider already refused on PR5; here that protection works without its temporary boundary.Exact 77-file Vitest list
Removed or replaced legacy assertions (PR3–PR5 fixtures)
NewWorkspaceComposerCard.file-drop.test.tsxandnative-chat-composer-composition.test.tsx: remove the assertions thatdata-native-file-drop-targetis absent. The attribute has been removed from all code and fixtures; live owner delivery assertions remain.useComposerState.integration.test.ts: remove bothonFileDrop-not-called assertions (after mounting and after unmounting two composers). The method no longer exists; composer lifecycle assertions remain.use-native-chat-file-attachment-actions.test.tsx: remove the obsolete “does not subscribe to legacy window drops” assertion/test. Picker delivery and cancellation tests remain.native-chat-composer-drop-scope.test.tsx: replace the two exact legacy send assertions for the unscoped-composer and editor control drops with no-send assertions. They are now unowned controls; both attachment caches must still stay empty. Remove the fixture's main-relay echo and singleton preload setup.native-chat-element-file-drop.test.tsx: replace the exact legacy terminal send assertion after dropping outside a portaled chat with no-send. The chat still attaches exactly once, and its drop cannot reach the ancestor. Remove legacy target attributes from the disabled-chat and workspace-card fixtures.native-chat-pane-file-drop.test.tsx: replace the absent legacy-target-attribute assertion on an empty chat with a present owner-attribute assertion, preserving its refusal boundary.terminal-pane-element-file-drop.test.tsx: remove the no-broadcast assertion and absent legacy-terminal-attribute assertion for each title/body case. The API/attribute are gone; exact preparation and terminal delivery counts, no legacy IPC, no activation, and inactive-pane isolation remain.use-terminal-pane-global-effects-file-drop.test.ts: remove the retired three-case no-onFileDrop-subscription suite and its callback fixture. The hook no longer has that API; element-owner tests cover active/hidden/retired panes.os-file-drop-preload-integration.test.tsx: replace the old unmarked-chrome copy-cursor/one-legacy-send assertions with guard refusal. Remove the nested-legacy-child test's copy cursor, one relay send, no preparation and no owner delivery assertions, since there are no legacy child barriers.Remaining
onFileDroptest stubs and legacy target fixtures were removed without changing their functional assertions. All other owner assertions remain. Shared breadcrumb/payload-construction/relay-validator suites and preload-release tests were removed with those deleted implementations; file limits are now tested directly by the retained path validator, copy/preparation semantics by the preparation suites, and request registration/sender/lifetime validation bydropped-path-preparation-ipc.test.ts. Legacy relay tests for target-field preservation, window broadcast order, rejection forwarding, send exceptions, and relay listener registration/removal have no remaining production contract.Review
The coordinator arranges independent review and rendered checks. This worker did not run review loops, mark the PR ready, merge, or enable auto-merge.
Agent skill upstream boundary
Notes
Read the remote wire compatibility guide and searched
src/main/runtime, relay code,mobile/, and the web client.terminal:file-dropandterminal:file-dropped-from-preloadwere local Electron IPC only; the web method was a no-op and no paired-client RPC, stream opcode, or host publication is removed.fs:prepareDroppedPathsand its response shape remain. Monaco'sdropIntoEditorremains disabled on all five editor surfaces; both sidebar status readers still require Orca's drag types, and the hybrid file/text tests pass.Local/SSH/runtime/WSL/folder ownership suites passed. Live Windows/Linux/SSH/mobile execution and real operating-system drag behavior were not verified. Author: @BrennanKB5.
Checklist