Repository navigation
[STA-6940] Deliver chat, workspace composer, and feedback file drops at their elements (3/6) - #25781
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.
Review summaryThis PR went through an independent code review, a fix round, a re-review, a review of its merge resolutions, two rendered checks in a real Orca window, and a second fix round for what the rendered check found. The reviewers and the rendered-check agents 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
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe changes add element-owned file-drop handling for workspace composers, native chat panes, and the feedback dialog. Drop processing captures a destination and checks ownership or connection state before applying prepared paths. Native chat attachment ownership now resolves from known worktrees and execution hosts. The changes also add tests for destination changes, drop ownership, rejected drops, and file drops during composition. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change moves file drops to element-owned handlers with destination and ownership checks, and it adds tests. No concrete merge-blocking risk was identified in the supplied files. 🚥 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 |
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.
Keep element-owned destinations alongside paired-server attachment uploads, pending chips, and composer remount settlement. Update fixtures for pending chips and cover destination changes and the accepting drag overlay.
1 participant
ELI5
A file dragged onto a chat could go to a different mounted composer because delivery was broadcast to the window and filtered by routing markers or whichever composer mounted last. A non-image file dropped onto feedback could also open in the editor behind the dialog. Earlier versions of this migration used the task-source repository as a folder project's attachment destination, rejected files in floating chat without a catalog row, and could keep refusing files after its composer became available again.
This is PR 3 of 6 for STA-6940: native chat, workspace cards with an attachment destination, and feedback handle file drops on the element under the cursor. Chat without a usable composer and the current quick-create workspace card refuse files with the “not allowed” cursor, without a toast or an accepting ring.
Dependencies: PR1 (#25749) and PR2 (#25748) are both MERGED, squashed into main as
5345ba34bf2and5b1bb78d14b. PR3 has no pending dependency. Its original implementation starts after stacking merge1146abc847f6946c268d67ef319a8fd83e7c8874; later main syncs, including0c95de36943,89ffd1cc68c, ande0e04d71063, remain in history. The final sync merges main1617ff32eff18756c2a39d63f9512cbe7ac0f692into the reviewed heade0e04d7106358493503ec729174248efb46e887cin merge commit91fc85018e9685b6206adf17c3d2122c523eab21. History is preserved without rebasing or force-pushing.This final sync adds no feature beyond the changes already on main. It preserves main's paired-server attachment support (#25146), including pending file chips, host previews, and uploads that finish while a prompt card replaces the composer, alongside PR3's element-owned drop delivery.
What Changed
dropEffect = nonewithout a toast. Full composer attachment destinations retain element-owned delivery.use-os-file-drop-owner.tsand its refused-drop regression test; main contains PR2's older boolean-only version. In feedback-drop tests, keep the element-owner assertion and omit main's now-obsolete comment describing window capture. Main's other changes remain, including the new resume test. Remove that test's obsoletedropScopeKeyprop because PR3 removed it from the field;composerScopeKeyanddraftScopeKeyare untouched.a3ecee0dce1): the only text conflict was the import list ofNativeChatComposerField.tsx; it keeps PR3'sflushSyncimport (used to settle an interrupted composition) and drops theImageOfficon import, because main replaced the old one-line attachment notice with its notice card. Everything else from both sides stays: no drop-scope prop or drop-routing markers on the field, and main's notice card and @-file suggestion menu render as on main. PR3's own composition-drop test was updated to main's new field props (the notice list from main's real notice hook, and the @-file suggestion props); no production code changed in this merge beyond the import line.1617ff32eff1, merge91fc85018e9):native-chat-attachment-upload.ts: retain PR3's explicit host lookup, shared local-floating-workspace rule, and folder-aware destination; add main's paired-server session owner, capability check, and upload functions. The focused runtime never becomes a fallback destination.use-native-chat-external-attachments.ts: keep the destination captured before file preparation and alldestinationIsCurrentchecks; run main's paired-server upload through that captured delivery. Pending chips still settle into their captured chat while its composer is unmounted, as on main; a changed destination refuses them. Local reads and SSH uploads retain their mounted-composer checks.NativeChatComposer.tsxandNativeChatComposerField.tsx: keep main's pending chips, attachment host previews, hidden rich-text image chips, Send hold, and prompt recall, together with PR3's element-owner hook and interrupted-composition settlement. Neither the old broadcast subscriber nor the field's drop-routing markers return.git grep -n dropScopeKeyis empty, and production chat/composer code has noonFileDropsubscriber. Folder-project attachment destinations remain the recorded folder path.Why
The browser identifies the element receiving a drop. Binding delivery to that element's captured destination removes the need for every mounted composer to reconstruct ownership from a window-wide broadcast. Ending an interrupted composition before a replacement begins gives its attachment queue a definite end. Refusing the prompt-less quick-create card avoids accepting files into state that cannot be shown or delivered.
Differences from the common pattern:
Linked Issue
STA-6940
Visual Proof
Separate QA agents ran this branch as a hidden Orca dev window with an isolated profile, native chat and structured native chat on, and simulated OS file drops through the Chrome DevTools Protocol. The chats were confirmed to be restructured Claude chats (structured agent sessions, no terminal). The first run was on head
ff58070250; a second run re-checked the two fixes on headc62129a7dfe.1. A drop on chat B leaves chat A untouched. After a file was dropped on the other chat, chat A still holds only its own dropped file and image.
2. Image paste from the real clipboard. Exactly one pasted image (blue) is added next to the dropped one.
3. Feedback dialog. A dropped image attaches to the form; a text file dropped on the dialog afterwards no longer opens in the editor behind it.
4. Drop during a restarted input-method composition (fix). The file dropped during the first composition is attached exactly once, and does not reappear after a later composition.
5. New-workspace card (fix). Dragging a file over the quick-create card no longer shows an accepting drop ring; dropping it attaches nothing.
Not covered: a chat in the "cannot take files" state (not reachable through the UI; covered by DOM tests), real SSH, WSL and remote-runtime hosts, Linux and Windows.
Testing
Final sync at
91fc85018e9: 84 explicitly named files, 791 tests passed. Every file was checked to exist before runningORCA_BACKGROUND_LAUNCH=1 bash /tmp/sta-6940-pr3-tests.sh; that script uses a Bash file array andnode_modules/.bin/vitest run --config config/vitest.config.ts. The list includes the full prior 58-file PR3 suite and main's changed attachment, pending-chip, composer, draft, prompt-recall, and lifecycle tests. The final sync adds four regression cases within the existing test files; no app was launched.Exact named Vitest files
Regression evidence:
ff58070250088eb5d0709890060341fac2bf572c: 2 expected failures, 5 passes. The failed cases are composition restart without an end and quick-create's accepting cursor; the five normal/double/cancel/blur composition cases already pass. Both files pass after the fixes./drop/notes.txtin quick-create's otherwise-unused attachment state. Source inspection of the real footer, quick creation and folder creation confirms the missing display/delivery destination. Files were temporarily swapped for this check and restored before final verification.ff58070250passed 46 files / 381 tests and full CI.Final sync local checks:
pnpm install --frozen-lockfilepassed; deletedconfig/tsconfig.*.tsbuildinfo;pnpm tc:webandpnpm tc:nodepassed, one at a time. The first web check caught the two PR3 fixtures missing main's required pending-chip controls; both were corrected before the passing checks and 791-test run. Fulloxlinton all 44 changed source files passed.pnpm run check:code-quality:changedpassed with zero new findings before the commit; the pre-push CI checks also run it against the merged main base.pnpm run check:react-doctor:changedagainst the merged base passed with zero errors and four warnings (the card and composer complexity, sequential local-file reads, and synchronous composition settlement). Its pre-commit scan included main's unrelated changes because the merge was still pending; neither of its two unchanged-main errors is in PR3's final diff, and the final 44-file scan passes. pnpm's incidental@pnpm/exeblock is restored, so the committed lockfile is unchanged.orca-ci-checks --no-typecheckpassed all 22 checks before push, including localization verification, full repository lint, type-aware lint, and both changed-code gates against main1617ff32eff1; the scoped typechecks above already passed.Previous maintenance checks:
Previous maintenance checks:
pnpm install --frozen-lockfileand deletedconfig/tsconfig.*.tsbuildinfo. After the first merge,pnpm tc:nodepassed butpnpm tc:webfailed with one error that was on main itself, not from this PR: main'snative-chat-composer-field-resume.test.tsx(added by #25835) still passed theonAcceptMentionprop that #26017 had renamed. This PR did not carry that fix; once #26097 landed on main it was merged in, and on the final head0c95de36943pnpm tc:webandpnpm tc:nodeboth pass. Fulloxlinton all 43 changed source files passed;pnpm run check:code-quality:changedreported zero new findings;pnpm run check:react-doctor:changedon the 43 PR files reported zero errors, three warnings (the same as before). pnpm's incidental@pnpm/exelockfile block was restored; there is no lockfile change.Earlier maintenance also passed:
pnpm install --frozen-lockfile; deletedconfig/tsconfig.*.tsbuildinfobefore checking.pnpm tc:web, thenpnpm tc:node, one at a time. The first web check caught two missing required props in the new test fixtures; the final checks both pass.node_modules/.bin/oxlinton all 43 changed source files, including line limits; formatting and normal commit hooks passed. After CI caught a broadobjectparameter in the new card test, it was replaced with a named drag-transfer type; full lint and the anti-slop check then passed on all 46 currently changed source files, both new regression files passed (7 tests), andpnpm tc:webpassed again.pnpm run check:code-quality:changed: zero new findings against maind3e1494674f3.pnpm run check:react-doctor:changed: zero errors, three warnings (card complexity, sequential file reads, and synchronous settlement skipping view transitions). The synchronous update occurs only at an irregular composition restart, where the old draft must be committed before the next composition owns the field.@pnpm/exelockfile block; there is no lockfile change.Current-head CI (
91fc85018e9685b6206adf17c3d2122c523eab21): pending the final-sync push. It will be watched with one blockinggh pr checks 25781 --watch --fail-fast --interval 60command; no live UI run is part of this maintenance.Earlier full CI passed at
0c95de36943in PR Checks run 37584573067, attempt 1, including all five unit-test shards. The prior main-only typecheck failure was fixed on main by #26097, then merged here; PR3 did not carry an independent main fix. These are historical results, not validation of the new merge head.Local tests ran on macOS with
ORCA_BACKGROUND_LAUNCH=1. No local app build or live Electron, Linux, Windows, WSL, SSH or remote-runtime session was exercised. Browser hit-testing across Electron's isolated worlds and real OS drag gestures remain unverified by this worker.Review
The native chat scope suite retains authorization, owner-change and SSH-upload coverage. Composition tests mount the real field, editor, draft store, pane owner and attachment hooks. Quick-create tests mount the real card sections and use its real composer state with a selected local project. No new feature, documentation file, routing fallback or user-visible string is introduced.
NativeChatComposer.tsxremains shorter than the main version merged here.Agent skill upstream boundary
Notes
PR1 and PR2 are merged; PR3 has no pending dependency and remains ready for review. The final main sync preserves both the reviewed drop migration and main's attachment behavior. This worker does not merge the PR. The quick-create refusal and interrupted-composition settlement remain the two explicitly requested QA fixes.
Checklist