Repository navigation
[STA-6940] Prepare file drops for element owners (PR 2 of 6) - #25748
Conversation
…-pr2-drop-plumbing
Review summaryThis PR went through an independent code review, a fix round, a second review of the fixes, a CI fix, and a rendered check in a real Orca window. The reviews were done by separate agents that 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 (31)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change adds a shared main-process queue for preparing dropped paths, exposes it through the filesystem APIs, and adds renderer-owned drop handling. Preload and document-level handlers coordinate native file drops with renderer owners and legacy targets. The change also disables Monaco drop-into-editor behavior and narrows workspace drag recognition to typed payloads. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No issue identified here needs to block merging; real OS-drop behavior on the reported untested platforms remains unverified. 🚥 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 |
ELI5
An OS file drop can reach a hidden chat, another terminal split, or the active workspace because Orca intercepts the real drop and broadcasts a reconstructed destination. This is PR 2 of 6 for STA-6940: it adds the preparation and ownership foundation while keeping desktop OS file drops on their existing path. Independent review found that the new web navigation guard overwrote feedback’s accepting drag cursor, so dragging a screenshot onto feedback could stop working; the unused owner hook also silently lost files with no readable path, sequenced sibling roots independently, and lacked a synchronous destination snapshot.
CI then failed at
2e3f19a7b0d37ae97a8c21a268b5ee48d69a5627because the new file-path function was added at the top level of the web API, breaking its existing namespace list. The desktop bridge now lives underwindow.api.fs; web omits it, and the owner hook checks that it actually exists before resolving paths. This correction adds no user-visible behavior.What Changed
window.api.fs.getPathForFileon desktop and a window-localfs:prepareDroppedPathsrequest returning successful paths and failures. The request and legacy relay reuse one screenshot-copy queue, with the existing byte budgets, eight-pending limit, two-minute timeout, reload cancellation, and seven-day expiry. Agent consumers cannot receive an uncopied temporary original; main-process readers may receive budget-limited originals.useOsFileDropOwner(ref, options)hook. Its callback ref registers the marker and native capture listeners together; the nearest registered root claims and resolves paths synchronously after the file-count cap. A required sequence created once by the destination is passed to each sibling root, keeping preparation results and asynchronous application in drop order while independent destinations continue. Arbitration and detach suppression stay local to each root.captureDestination(event)callback. It runs during the drop before preparation starts and passes the captured value toonDrop, so PR5 can retain an explorer folder even if the target row is recycled while preparation waits.There is no change to desktop OS-file destinations in this PR. Web feedback continues to accept screenshot attachments, unowned web file drops cannot navigate the browser away, and external plain text cannot impersonate a workspace-status drag. No notices were added.
Why
Keeping preparation separate from delivery lets the element under the cursor own its destination without losing screenshot copying. Adding another window-wide subscriber would retain the destination-guessing problem. The feedback fix belongs to its existing accepting handler, keeping web refusal elsewhere unchanged. A destination owns its sequence because a pane’s title and content, or an editor’s tab strip and group, represent the same destination even though their roots are siblings. Capturing a folder value during the event avoids reading a changed row after file preparation.
PR3 migrates chats, workspace composers, and feedback; PR4 migrates terminals and title bars; PR5 migrates explorer, sidebar, and editor groups; PR6 removes the legacy broadcast and markers.
Differences from the common pattern:
Linked Issue
STA-6940 — PR 2 of 6; the full issue is not closed by this plumbing change.
Visual Proof
This PR should change nothing about how desktop drops behave. A separate QA agent ran it as a hidden Orca dev window (head
2e3f19a7b0d, isolated profile) and simulated OS file drops through the Chrome DevTools Protocol. Every existing drop path still works as before. The later namespace move and main merges do not touch desktop delivery.1. File dropped on a terminal pane. The path is pasted on the prompt line, not run.
2. File dropped on a folder row in the file explorer. It is imported into that folder (
inbox/notes2.txt).3. File dropped on the editor content area. It opens as its own tab; its text is not inserted into the file already open, so Monaco's own drop-into-editor stayed off.
4. File dropped on the editor tab strip. It opens as a tab.
5. Image dropped on the feedback dialog. It attaches to the feedback form.
Known and unchanged here: a non-image file dropped on the feedback dialog still opens in the editor behind it; PR 3 (#25781) fixes that. Not covered: the web client in a real browser (DOM-level tests only), Linux, Windows, SSH and WSL hosts.
Testing
Maintenance validation on macOS with Node 26.5.0 and Vitest 5.0.3, after merging main as fetched for this update (
35e173a5b745684e79e8c1f74107e0feb45d0aa6) into this branch at83919811ffe568bc72c3c48811c197f0975406ea: 45 Vitest files passed, 367 tests passed. The original 44-file PR2 suite passed all 365 tests; the additional web API composition file passed both tests. All named files were checked for existence and the array was checked for a non-empty, exact 44-file list before the suite ran.pnpm install --frozen-lockfilepassed. Deleted bothconfig/tsconfig.*.tsbuildinfocaches, then ranpnpm tc:nodeandpnpm tc:webone at a time; both passed. Fullnode_modules/.bin/oxlinton all 31 files changed from the merged main commit passed.pnpm run check:code-quality:changedpassed with zero new findings in all six scans and the assertion rationale check.pnpm run check:react-doctor:changedpassed with the same two existing editor complexity warnings.git diff --checkand the staged whitespace check passed. The install and later pnpm commands added a local@pnpm/exeblock; the lockfile was restored and has no difference from the merged main commit.Main merged without conflicts or manual resolutions. This maintenance update adds no user-visible change and makes no feature edits.
CI attempt 1 on this head: full-unit run 37546923187 did not go green. Repository guards, static analysis and the full project typecheck, Linux and Windows packaging, relay integration, and unit shards 2/5, 3/5, and 4/5 passed. Shards 1/5 and 5/5 failed on issues inherited from the merged main snapshot:
config/scripts/vitest-sqlite-runtime-boundary.test.ts:70found three database-backed tests missing from the list assigned to Node:orchestration-party-location.test.ts,runtime-orchestration-sender-names.test.ts, andstructured-agent-session-mobile-restore.test.ts. Its test and runtime lists are unchanged from merged main. Main advanced while CI ran and fixed exactly these entries in #26010, commitfaed899cd3088eb740adfa789d141fe0be97fd6e; that later commit is not in this head.EEXISTfromsrc/main/skills/skill-bundle-extraction.ts:183. The originating test isskill-bundle-creation.test.ts, “rejects conflicting staging roots without changing their imported namespace.” The test, extraction, tar implementation, and CI runtime files are byte-identical to the merged main commit. This attribution is based on the CI error and unchanged sources; a separate clean-main run was not performed locally.Neither failed job reports a runner shutdown, cancellation, or artifact download failure. No source patch or CI rerun was made for these main-side failures; the coordinator will handle a separate full rerun and any follow-up to main. Mobile Checks and PR test LoC passed; the Headless Node server workflow was skipped.
Final verification failed because the two unit shards failed; unit-selection evidence passed.
Final CI result for every full-unit workflow job
No application or Electron window was launched. Real OS drops, rendered Electron behavior, browser drag enforcement, and Linux/Windows/SSH/WSL runtime flows were not verified locally. Packaging and the full repository suite were not run locally. No new user-visible strings were added, so localization checks were not rerun for this maintenance update.
Exact 44-file suite, run through Bash so the file array expands correctly:
Additional namespace regression:
Review
Self-reviewed the preparation boundary, sender validation, global copy serialization, preserved legacy delivery, migration barriers, and guard interaction. Addressed all independent-review findings plus the synchronous destination snapshot without migrating any desktop surface.
Agent skill upstream boundary
Notes
Preparation remains local to the dropping desktop window and is not added to remote procedure calls. Existing downstream host uploads and path mapping remain in place. No active-pane or focused-runtime fallback was added. No documentation or lockfile changes are included.
Checklist