Skip to content

feat(web): attach files dropped anywhere in the chat column - #6245

Merged
github-actions[bot] merged 3 commits into
mainfrom
fix/page-wide-file-drop
Sep 3, 2026
Merged

feat(web): attach files dropped anywhere in the chat column#6245
github-actions[bot] merged 3 commits into
mainfrom
fix/page-wide-file-drop

Conversation

@yaoharry

@yaoharry yaoharry commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Related issue

No tracking issue — this came in as direct feedback ("can we allow dragging
images anywhere in the page, not just in the input?").

Summary

Dropping a file into a chat only worked on the composer box itself. Drop a
screenshot on the transcript or the empty space beside the composer and the drop
fell through to the browser, which navigated away from the session to render the
file and lost the page. A file dragged into a chat has no other meaning, so the
whole chat column now takes it.

  • New useFileDropTarget hook binds the drag listeners on a given element and
    reports whether a file drag is in flight over it. Both composers (in-session
    ChatPage and the new-chat landing screen) route it to their existing
    addFiles, so attachment validation, chips, and the upload path are
    unchanged.
  • The target is the chat column — [data-chat-surface], the main column holding
    the transcript and the composer, resolved from the composer card's ancestor;
    the landing surface on /. The shell around it (sidebar, workspace rail)
    keeps whatever drag behavior it had.
  • The drop cue moves from a badge inside the composer box to a FileDropOverlay
    portalled into the drop target, so it covers exactly what accepts the drop.
    The composer keeps its inset ring so the destination stays obvious.
  • Only drags carrying files (dataTransfer.types includes "Files") are
    claimed, so dragging selected text into the textarea keeps its native
    behavior. preventDefault on a file dragover is also what stops the
    browser from opening a dropped file over the app.

Test Plan

Automated:

  • pnpm vitest run src/hooks/useFileDropTarget.test.tsx src/pages/ChatPage.composer.test.tsx src/shell/NewChatDialog.test.tsx — 469 passed. New unit coverage: the drop lands anywhere inside the target, a drop outside it is neither claimed nor attached, the cue survives the pointer crossing child elements, a cancelled drag clears it, a non-file drag is ignored and left un-prevented, and the listeners unbind on unmount.
  • pytest tests/e2e_ui/chat/test_composer_attachments.py --ui-skip-build — 7 passed, including test_file_dropped_on_the_transcript_attaches (dispatches a real DataTransfer drag onto the transcript in Chromium, asserts the cue, that the app called preventDefault on the drop, and that the chip appears) and test_file_dropped_outside_the_chat_column_is_ignored (same drag on the sidebar: not claimed, no cue, no chip).
  • Proved the new e2e test is not vacuous: rebuilt with the pre-change ChatPage.tsx and it fails — element(s) not found … waiting for get_by_test_id("file-drop-overlay").

Manual:

  1. Open a session, drag an image file from the desktop over the transcript
    (not the composer) — the chat column dims with a dashed border and "Drop
    files here"; release, and the file appears as a chip in the composer. Before
    this change the browser navigated away and the session was gone.
  2. Drag over the sidebar or the workspace rail — no cue there, and a drop is not
    an attachment.
  3. On the new-chat landing screen (/), drop a file onto the heading — it
    attaches to the landing composer.
  4. Select text in the transcript and drag it into the composer textarea — still
    inserts the text (a non-file drag is not intercepted).
  5. Start a drag, press Esc / drag back out of the chat column — the cue clears.

Demo

  • Visual demo attached below
  • Non-visual evidence provided below or in Test Plan
  • Not applicable — no behavioral change

Dragging a file over the transcript — the cue spans the chat column, and the
sidebar / workspace rail stay out of it:

Drop overlay covering the chat column in a session

Released on the transcript, well outside the composer box — the file attaches:

File attached as a composer chip after dropping on the transcript

Same behavior on the new-chat landing screen:

Drop overlay on the new-chat landing surface

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

Manual verification covers what neither layer can: a real OS-to-browser drag.
Playwright can only synthesize a DataTransfer in the page, and jsdom has no
drag semantics at all, so the steps above were walked in a local dev build
(screenshots in Demo were captured from that build via Playwright).

Changelog

Drop an image anywhere in the chat to attach it, not just on the message box

Dropping a screenshot into a session only worked on the composer box
itself. Anywhere else — the transcript, the sidebar, the empty space
beside the composer — the drop fell through to the browser, which
navigated away from the session to render the file and lost the page. A
file dragged into a chat has no other meaning, so claim the whole page.

A shared `useWindowFileDrop` hook binds the drag listeners on `window`
and reports whether a file drag is in flight; both composers (in-session
and the new-chat landing) route it to their existing `addFiles`, so
validation and the chips are unchanged. The drop cue moves from a badge
inside the box to a page-wide `FileDropOverlay`, since the target is now
the page. Only drags carrying files are claimed, so dragging selected
text into the textarea keeps its native behavior.

Signed-off-by: harry-yao_data <harry.yao@databricks.com>
@github-actions github-actions Bot added the size/L Pull request size: L label Sep 3, 2026
@omnigent-ci

omnigent-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Review: page-wide file drop (useWindowFileDrop + FileDropOverlay)

1. Blocking issues

None. The one real hazard worth checking — the hook's own warning that "two live instances would both attach the same drop" — was verified against the actual mount topology and does not occur:

  • <Composer> is rendered exactly once (ChatPage.tsx:2301, inside a single MainAgentSurface instance); SubagentComposerTray is a label-only strip, not a second composer/hook.
  • NewChatLandingScreen is reached only via the early return if (!urlConvId) return <NewChatLandingScreen /> — it is not a modal, so the landing composer and the in-session composer are mutually exclusive and can never both mount.
  • No split-pane / multi-conversation view mounts a second composer.

So at most one hook instance is ever alive → no duplicate window listeners, no file attaching to multiple composers, no duplicate portals. preventDefault gating on carriesFiles is correct: a file drag can no longer navigate the browser away, and a text/selection drag (types: ["text/plain"]) early-returns un-prevented, preserving native drop-into-textarea.

2. Security vulnerabilities

None found. No new deserialization, injection, or boundary changes; addFiles runs the same validateAttachments path as before and the server still enforces limits authoritatively.

3. Non-blocking notes

  • dragend is effectively dead code for the primary use case. dragend fires only on the source element of an in-document drag; an OS-originated file drag never fires it on window. The end handler (the "Esc / drop-outside-window" safety net) therefore does nothing for real desktop-to-browser drags — clearing relies entirely on drop or a balanced final dragleave.
  • Window-exit dragleave is browser-unreliable. If the cursor leaves the window quickly the final dragleave may not fire (or fires with types emptied → carriesFiles false → depth never decrements). With no working dragend fallback, dragging a file in and leaving without dropping can strand the page-wide overlay until the next drag. The old per-composer handler had the same class of bug, but promoting it to a full-page dim/blur makes a stuck state far more prominent. Mitigating factor: the overlay is pointer-events-none, so it never blocks clicks and the next drag resets it. Consider a coarse fallback (clear on window blur, or a short post-dragover timeout).
  • The docstring's stopPropagation contract is a latent trap. The hook advises "a drop zone that wants a file for itself can stopPropagation before the event reaches window." A zone that does this for drop only (not also dragenter/dragleave) lets window see dragenter (overlay on) but never the matching drop, so depth is never reset and the overlay sticks. No such zone exists today, but the advice as written invites the bug — recommend zones stop all drag events, or add the fallback above.
  • Read-only sessions are now page-wide drop targets. The hook binds unconditionally and addFiles mutates attachment state, while the textarea/attach button are disabled for isReadOnly (ChatPage.tsx:5452, :5543). A user can drop anywhere on a read-only session, see chips attach, but cannot submit. The old handler had this only over the composer box; this PR expands the misleading target to the whole page. Low severity, but worth gating addFiles/the overlay on the same disabled condition.

4. Approach

Sound. There is no pre-existing window-level DnD or shared drop hook to reuse (TableBubbleMenu is ProseMirror row/column reordering, not file drop), so this hook is genuinely new, and folding the two duplicated per-composer handler blocks into one hook + one portalled overlay is a net simplification consistent with the repo's web/src/hooks/ conventions. The carriesFiles-via-dataTransfer.types signal is the correct mid-drag discriminator. No materially simpler alternative.

5. Summary

A clean, well-tested feature with strong test coverage (unit + a proven-non-vacuous e2e that dispatches a real DataTransfer drag). The headline multiple-binding risk is refuted by the actual single-instance mount topology, and there are no correctness or security blockers. The remaining items are low-severity polish: the dragend/window-exit dragleave combination can strand the page-wide overlay in edge cases (a coarse clear-on-blur fallback would close this), the stopPropagation docstring guidance is a latent trap, and read-only sessions now show an attach affordance they can't act on. Ship-able as-is; addressing the stuck-overlay fallback and the read-only gate would tighten it.Both cross-vendor reviews are now collected and fully consistent with the synthesized review I already posted — no new findings to incorporate. The final review is complete and stands as posted above.


Automated review by Polly · workflow run

The drop target was the whole window, which took in the sidebar and the
workspace rail — regions where a dropped file is not a chat attachment.
Bind the listeners to the chat column instead (`[data-chat-surface]`, the
main column that holds the transcript and the composer; the landing
screen's surface on `/`), and portal the drop cue into that element so it
covers exactly what accepts the drop. Everything outside keeps whatever
drag behavior it had.

Signed-off-by: harry-yao_data <harry.yao@databricks.com>
@github-actions github-actions Bot added size/XL Pull request size: XL and removed size/L Pull request size: L labels Sep 3, 2026
@yaoharry yaoharry changed the title feat(web): attach files dropped anywhere on the page feat(web): attach files dropped anywhere in the chat column Sep 3, 2026
Signed-off-by: harry-yao_data <harry.yao@databricks.com>
@yaoharry yaoharry added the automerge Automatically Run Merge CI label Sep 3, 2026
@github-actions
github-actions Bot enabled auto-merge (squash) September 3, 2026 07:00
@github-actions
github-actions Bot merged commit 3d21f65 into main Sep 3, 2026
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically Run Merge CI size/XL Pull request size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant