Desktop: route native drops to the drop zone under the cursor by NilayYadav · Pull Request #8265 · unslothai/unsloth · GitHub
Skip to content

Desktop: route native drops to the drop zone under the cursor - #8265

Merged
danielhanchen merged 15 commits into
unslothai:mainfrom
NilayYadav:fix-native-drop-target-routing
Aug 10, 2026
Merged

Desktop: route native drops to the drop zone under the cursor#8265
danielhanchen merged 15 commits into
unslothai:mainfrom
NilayYadav:fix-native-drop-target-routing

Conversation

@NilayYadav

Copy link
Copy Markdown
Collaborator

Tauri delivers OS drops window-wide and suppresses the webview's own drop events, so the chat-wide handler claimed every drop on the chat route. A PDF dropped on the New Project dialog's Sources panel was attached to the chat behind the dialog instead.

Adds a native drop target registry that resolves the innermost registered element under the drop position, and makes the chat handler defer to it. Wires up the RAG sources panel (via the existing nativePathLease upload path, so no size limit) and the img2img source picker.

Also fixes the drop position conversion. Tauri types the position as physical on every platform, but wry only produces one on WebView2; macOS and GTK already report CSS pixels. Scaling those halved every hit test on a HiDPI display. The same bug was already shipped in the training dataset drop zone, which this fixes too.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@chatgpt-codex-connector

Copy link
Copy Markdown

@danielhanchen

Copy link
Copy Markdown
Member

Pushed one fix on top of this: registerNativeDropTarget added the element to the map synchronously, but the onDragDropEvent install behind it is async. In that window nativeDropTargetAt already reported a hit, so the chat-wide handler and the dataset handler both stepped aside for a listener that could not deliver yet, and the drop landed nowhere.

nativeDropTargetAt now returns null until the drop listener is actually installed, so during setup the window-wide handlers keep the drop instead of it being lost. Outside Tauri the flag starts true, since the DOM routes drops itself and there is nothing to wait on.

Also added tests/native-drop-target-readiness.test.ts with a stubbed @tauri-apps/api/window that holds the install unresolved, covering both halves: no claim while pending, claim plus delivery once installed. It fails on the previous behaviour.

Verified locally: npm run typecheck clean, npm test 1474 passing, npm run build clean. I also checked the position claim against wry 0.55.1 and it holds up: wkwebview/drag_drop.rs reports NSView points, webkitgtk/drag_drop.rs reports GTK widget coordinates, and only webview2/drag_drop.rs reports device pixels from ScreenToClient, so the unconditional divide really was halving every hit test on HiDPI macOS and Linux.

The failing Backend CI jobs are unrelated, tests/test_diffusion_training.py is failing the same way on main.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd682b8eef

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +99 to +100
if (isExpired(entry, Date.now())) {
throw new Error("The drop expired. Add it again from the project.");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep staged tokens alive through the upload loop

When a user creates a project with multiple desktop-dropped sources shortly before the 30-second expiry-grace boundary, uploads run sequentially and this check is repeated only when each file's turn begins. If an earlier upload crosses that boundary, every later native source is skipped after the project has already been created, even though all were accepted when Create was clicked. Fresh evidence beyond the earlier stale-token comment is this per-entry expiry recheck inside the sequential upload loop; the token lifetime needs to cover the complete staged upload or be refreshed while it runs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not taking this one. TOKEN_TTL is 15 minutes (native_intents.rs:22) while LEASE_TTL is 2 minutes (native_backend_lease.rs:11), so pre-minting every lease before the loop would shorten the window to 2 minutes for the whole batch and make long uploads fail more often, not less. That is why leases are minted per file as its turn comes. Reaching the current case needs a dialog left open for about 14.5 minutes, and the outcome is a named toast per skipped file plus a project that already exists, so the source can be added straight from its Sources panel. There is also no token refresh command to call.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

1 similar comment
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd682b8eef

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +83 to +84
.catch(() => {
listening = false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retry listener setup while registered targets remain

If the startup onDragDropEvent installation rejects after a drop target has mounted, that target's registration has already called listen() while listening was true, so this catch merely resets the flag and nothing initiates another attempt. The target then remains in the map while ready stays false, causing every drop over it to fall through to the window-wide handler until the component happens to unmount and remount; retry from this catch when targets.size > 0.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not taking this one. The only ways to reach that catch are the dynamic import("@tauri-apps/api/window") failing or onDragDropEvent rejecting, both of which mean the bundle or the event permissions are broken app-wide rather than for one session. listening is reset, so the next target that mounts does retry. Retrying from the catch while targets.size > 0 would spin against a permanently failing install with no backoff. The fallback in the meantime is also the pre-PR behaviour, the window-wide handler, not a lost drop.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from NilayYadav Aug 10, 2026
@unslothai unslothai deleted a comment from NilayYadav Aug 10, 2026
@unslothai unslothai deleted a comment from NilayYadav Aug 10, 2026
@unslothai unslothai deleted a comment from NilayYadav Aug 10, 2026
danielhanchen added a commit to Datta0/unsloth-staging-3 that referenced this pull request Aug 10, 2026
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 8523f3e582

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70628aba35

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const readFile = useCallback(
(file: File | undefined | null) => {
if (!file || !file.type.startsWith("image/")) {
if (file) toast.error("Please choose an image file");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Invalidate native reads when an equal-valued slot is reused

When an index-keyed reference picker is reused for another slot while both slots have the same value—especially the cleared "" entries retained by images-page.tsx—this equality return does not advance selection. If an earlier slot is removed while a native read is pending, that read therefore remains valid and invokes the old index-capturing onChange, updating or losing the image in the wrong slot. The guard needs a stable slot identity rather than relying only on the displayed value changing.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not taking this one. The case is real but it needs two reference slots holding the identical value, a native drop on one of them, and that same slot removed inside the read window; the outcome is the dropped image landing in the adjacent identical slot. The fix asked for is a stable slot identity, and there is none to give: referenceImages is a string[] where the index is the identity, so closing it properly means restructuring that state to carry ids and updating every reader, the add and remove buttons and the send path. That is a change to a shared page well outside a drop-routing PR, and riskier than the residual it removes. Worth doing on its own if you want it closed. The value-change guard in e5b2fdd already covers the case that loses a distinct image.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 10, 2026
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 9f23a3b84f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danielhanchen

Copy link
Copy Markdown
Member

Reviews have converged on 9f23a3b84. Six fixes landed on top of your branch across the last few rounds, all on the desktop drop paths this PR introduces:

  • fd682b8ee a target was added to the registry synchronously while its onDragDropEvent install was still pending, so the window-wide handlers stepped aside for a listener that could not deliver and the drop was lost. nativeDropTargetAt now reports nothing until the listener is installed.
  • 8523f3e58 commit did not update stagedRef.current, so two native drops settling in the same tick both merged against the pre-drop list and the second published an array missing the first drop's files.
  • 8523f3e58 over events published a fresh state object per cursor move, rerendering ChatPage at drag frequency. It now returns the previous object when nothing changed, so React bails out.
  • 8523f3e58 the image picker's read guard was per instance, and it only accepted the formats the picker advertises, not the narrower set native_path_policy.rs allows.
  • e5b2fdde4 the reference slots are keyed by index, so removing one keeps the picker mounted while a different image shifts into it. A read in flight then overwrote a reference the user never touched.
  • 9f23a3b84 the Windows conversion divided by the monitor scale, which yields logical window pixels rather than the CSS pixels elementFromPoint and getBoundingClientRect use. Webview zoom moves the two apart in both directions, so it now takes devicePixelRatio, with the monitor scale as the fallback. Same distinction app/provider.tsx:78 already draws for layout.

Three items were left unfixed with the reasoning in their threads: pre-minting leases across the upload loop would shorten the window from the 15 minute token TTL to the 2 minute lease TTL and make long uploads fail more often; retrying the listener install from its catch would spin against a failure that is app-wide anyway; and closing the last identical-valued reference slot case needs referenceImages restructured to carry stable ids, which is worth its own PR.

I confirmed the position claim against wry 0.55.1 rather than taking it on trust. wkwebview/drag_drop.rs reports NSView points and webkitgtk/drag_drop.rs reports GTK widget coordinates, both already CSS pixels, while only webview2/drag_drop.rs reports device pixels from ScreenToClient. The unconditional divide really was halving every hit test on HiDPI macOS and Linux.

Verification on the final head: typecheck clean, 1477 frontend tests passing, build clean. Since the org queue is backed up I also ran the change on ubuntu-latest, macos-14 and windows-latest plus the studio Playwright suite on a scratch repo, and all four were green.

The head commit is a comment-tightening pass with no code change, AST-verified, so I have not asked for another review round on it.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: ee68a3b01c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danielhanchen

Copy link
Copy Markdown
Member

@danielhanchen
danielhanchen merged commit f4bd4e8 into unslothai:main Aug 10, 2026
40 checks passed
danielhanchen added a commit that referenced this pull request Aug 19, 2026
* Desktop: make every drop zone take a drop again (#9036)

Tauri delivers OS file drops window-wide and suppresses the webview's own
drop events, so a zone wired only to `onDrop` does nothing in the desktop
app: no drag-over border, and the file is silently ignored.

Native drop routing (#8265) was only ever adopted by the shared image
picker and the create-project dialog. Every other file drop zone still
relied on HTML5 handlers that never fire, which is why this reads as
intermittent: the same file works when it lands on the chat, covered by
the window-wide handler, and does nothing anywhere else.

Adds `useNativeFileDrop`, which claims the native drop for an element and
returns drag-over state plus the HTML5 handlers the web build still needs,
then adopts it in the zones that were dead:

- Projects -> Sources, which also had no drag-over styling at all
- Data Recipes unstructured seed
- Diffusion training images
- Video and audio reference pickers

Documents upload by lease rather than an inline read, since the native
reader only serves media inline, so the recipe seed route now accepts
`nativePathLease` the way the RAG upload routes already do. The native
path policy accepts video containers so the reference picker can register
what it is given.

Also stops two silent discards with the same symptom: a claimed zone that
refused a drop while disabled, and compare mode disabling the window-wide
handler outright. Both now say what happened.

`native-dropzone-coverage.test.ts` walks src/ and fails if a zone reads
files from a drag payload without either claiming the native drop or
explicitly deferring to the window handler.

* Desktop: refuse a dropped model in compare too, and validate before reading

Two things the first pass got wrong.

Keeping the window-wide listener on outside single chat also handed it
model drops, so a GGUF dropped on a compare or project view would load
and replace the active model. Nothing happened there before, so that is
not a change this should be making. The refusal now covers every kind the
handler would act on, models included.

The recipe seed route also moved its extension check after the read, so a
rejected 500 MB upload was pulled into memory first. Back to validating
the filename before reading a byte, as it was.

* Desktop: size the native video cap to the largest client-side limit

64 MB sat under the reference picker's own 72 MB, so a clip the picker
accepts was refused on drop. The cap is a backstop; callers keep theirs.

* Desktop: drop the diffusion zone from this pass, and bound the native read

Two review findings, both correct.

The diffusion dataset zone accepts .bmp, .m4v, .caption and .jsonl, which
the chat attachment policy rejects outright, and .txt, which registers but
cannot be read inline. Captions beside images are the documented workflow
there, so wiring that zone to the attachment path would have uploaded the
images and silently lost the captions. It needs its own registration and
upload policy, which is more than this belongs to, so it goes back to the
picker it had.

The recipe seed route also read a dropped path in full before checking any
limit, so a multi-gigabyte local file went into backend memory before the
413. It now refuses on the stat and bounds the read by what the block has
left, in case the file grows in between.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Desktop: keep a busy drop zone hit-testable, and size the video cap to the raw limit

A disabled seed zone carried pointer-events-none, which takes it out of
elementFromPoint, so nativeDropTargetAt could not find the target it had
just registered. The disabled message was unreachable and the drop fell
through to the window handler instead.

MAX_NATIVE_VIDEO_BYTES was set to the reference picker's 96 MiB, but that
cap bounds the data URL, not the file: the picker's own raw limit is
75497280 bytes. Rust was reading and base64-encoding up to 96 MiB, 128 MiB
across the bridge, for clips the picker then rejected.

* Match the document-refusal test to the message it now returns

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Studio: attach video to a chat, and say why when it is unavailable (#9057)

* Studio: attach video to a chat, and say why when it is unavailable

llama.cpp takes video through its OpenAI-compatible chat endpoint as an
`input_video` content part, but only when the mmproj declares video, the
binary was built with video support and ffmpeg is installed. It reports
that verdict at /props under modalities.video. Nothing about the GGUF
alone can tell us, so that is what Studio now reads.

Frontend: video joins the drop classifier and its own pending queue
beside images and audio, and a VideoAttachmentAdapter takes one clip per
message from the picker or a drop. When the model cannot take video the
adapter names all three possible causes instead of letting llama-server
refuse the request later.

Backend: video_base64 on the chat request is forwarded whole as an
input_video part, since llama-server owns the frame sampling and there is
nothing useful to transcode. has_video_input rides the same path as
has_audio_input out to the model row.

Compare mode is left out on purpose: video_base64 targets the single
loaded GGUF, so at most one side could answer. Dropping a clip there now
says so rather than ignoring the file.

Size caps line up across the three hops (64 MB in the desktop reader, in
the composer and in the route) so no hop accepts what the next refuses.

* Studio: carry the video capability through, and cover the passthrough path

Three review findings, all correct.

syncModelCapabilities took has_video_input but never copied it into the
row, and /api/models/list omits it for the active GGUF, so the adapter read
false after every load and refused video even when /props reported it. The
feature did not work in its main path.

The tool and response_format passthrough returns before the injection and
forwards an explicit field list, so a clip rode along nowhere and the model
answered without it. Refused now, the way audio already is there.

The size cap floored the base64 inflation, so a clip of exactly the size
the composer allows was refused with a 413, and the data URI header was
counted against the payload. Padded ceiling, measured after stripping.

* Studio: carry the video capability through every hop, and refuse it where it cannot be served

Three separate places map backend capability flags onto a model row and
each one dropped the video flag: the direct status adoption, the queued-run
capability Pick, and (fixed earlier) syncModelCapabilities. The adapter
reads that row, so any of them leaves video refused on a model that
supports it. Covered by a rule rather than three spot checks.

Injection lives in the GGUF branch, so an external provider or a local
transformers model answered as if no clip were attached. Both now refuse,
as does token counting, which cannot inject the frames it would need to
count. The size check also moved ahead of the automatic model switch so an
oversized clip does not evict a working model before the 413, and video now
votes in the pre-switch projector requirement alongside audio.

The video drain's read-failure toast said 'audio', inherited from the
audio drain it was cloned from.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Studio: name the attached modality in the pre-switch refusal

Adding video to require_vision made the shared rejection reachable for a
request carrying only a clip, but its text is fixed at 'image or audio
input', so the user who attached a video was told about modalities the
request never carried. The label now follows what is attached, and defaults
to the existing wording so the image-only callers are unchanged.

* Match the document-refusal test to the message it now returns

* Send the API key on the /props readback and skip video in the context recount

The /props probe went out without an Authorization header, so under
UNSLOTH_DIRECT_STREAM=1 llama-server answered 401 and video capability
never came back. Context recount already bails on images and audio
because toOpenAIMessages has no branch for them; video has the same
property and was missing the bail, so the usage bar priced a text-only
prompt and stringified megabytes of base64 on the UI thread.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Send a picked clip under the container its extension names

The accept list carries extensions as well as mime types because the
browser's answer is unreliable for mkv and some mov files, and the
picker takes those files on the extension. Only an empty type was being
replaced, so a clip the browser called application/octet-stream kept
that type into the attachment, and the request builder recognises a file
part only when its mimeType matches ^video/. The clip was attached, sent
and dropped, and the model answered as though nothing were there, which
is the silent drop this PR exists to remove. The table mirrors the one
in native_intents.rs, so a clip read by the desktop reader and one
picked in the browser reach the route the same way.

---------

Co-authored-by: shimmyshimmer <182633334+shimmyshimmer@users.noreply.github.com>
Co-authored-by: Unsloth <michaelhan@Michaels-MacBook-Pro.local>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Han <danielhanchen@gmail.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: shimmyshimmer <182633334+shimmyshimmer@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Unsloth <michaelhan@Michaels-MacBook-Pro.local>
Co-authored-by: Daniel Han <danielhanchen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants