Desktop: route native drops to the drop zone under the cursor - #8265
Conversation
|
Pushed one fix on top of this:
Also added Verified locally: The failing Backend CI jobs are unrelated, |
There was a problem hiding this comment.
💡 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".
| if (isExpired(entry, Date.now())) { | ||
| throw new Error("The drop expired. Add it again from the project."); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 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".
| .catch(() => { | ||
| listening = false; |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 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"); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Reviews have converged on
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 I confirmed the position claim against wry 0.55.1 rather than taking it on trust. 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. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
* 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>

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.