tui: image-input UX — drag-drop attach, inline numbered chips, vision fallback, cursor + copy fixes#247
Conversation
… fallback, cursor + copy fixes Improves the image/PDF attachment flow and two long-standing composer papercuts. - Drag-and-drop: dropping an image/PDF onto the composer attaches it instead of erroring as an unknown "/…" command. The dropped path is detected on paste and on submit (absolute/escaped/quoted paths to an existing image or PDF). - Inline numbered chips: staged attachments render as compact [Image #1] / [Doc #1] chips INSIDE the input box; the verbose "Attached … (image/png)" system line is dropped. Backspace on an empty composer removes the last chip (/image clear still drops all). - Vision fallback: a real multimodal model absent from the curated catalog (custom / openai-compatible / local ids) now accepts images via a conservative name match; text-only ids stay refused, and the catalog stays authoritative. - Blinking composer cursor, including when the box is empty (previously no caret was drawn for a focused, empty box). - Ctrl+E releases/recaptures the mouse so native drag-select + copy works (mouse capture otherwise intercepts terminal text selection). Tests: dropped-path detection, vision-by-name + unknown-model fallback, mouse-release capture toggle, attachment removal, and updated chip-render tests.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a two-tier ChangesModel Registry Vision Capability
TUI Attachment and Composer UX
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
APPROVE
Walked the 10 files (416 +, 57 -) at 6f87cc3 against the diff base 2d62387 (#244 merge). The five features are clean, the new code is well-tested, and the failures I can see are limited to one Windows path-unescape edge case and a couple of conservative heuristic choices. Three follow-ups for a later PR; no blockers.
What works
1. Drag-and-drop attach is conservative and tested. droppedAttachmentPath (internal/tui/image_attach.go:23-52) only matches a single-line, single-existing, image/PDF-extension file. Multi-line content, non-existent paths, non-image files, real slash-commands, and empty input all return ok=false, so text pastes and /help-style inputs are untouched. TestDroppedAttachmentPath exercises every branch: backslash-escaped / single-quoted / double-quoted / plain path positives; slash-command / non-existent / plain text / non-image / multi-line / empty negatives. The paste-path integration in clipboard.go:routePaste is a clean 4-line addition with the right comment about why the check is needed (so a /Users/… dropped path doesn't get submitted as an unknown slash-command). Solid.
2. Numbered inline chips replace the verbose "Attached X" system line. The chip rendering moved from a separate row in footerView to inside composerBox (model.go:2142-2148), above the input line. The numbered format ([Image #1], [Doc #1]) hides the long filename while still letting a user count attachments. The two appendImageNotice calls in handleImageCommand and handleDocumentAttach are removed, so the system log is no longer polluted with per-attach confirmations — a real UX win. removeLastAttachment (image_attach.go:138-151) drops a doc before an image (matching the render order) and keeps pendingImages and pendingImageLabels in lockstep; TestRemoveLastAttachment pins both invariants and the empty-set case.
3. Vision fallback is fail-closed at the right places. SupportsVision now: catalog-known vision → true; catalog-known non-vision → false (authoritative, never overridden by the heuristic); catalog-unknown → visionCapableByName. The heuristic is conservative and well-commented: Gemini, Claude 3+/4+/opus/sonnet/haiku, GPT-4o/4.1/4-turbo/4-vision/5, o1/o3/o4, Grok 4 (and *-vision variants), and the common open multimodal families (llava, pixtral, internvl, minicpm-v, moondream, bakllava, *-vl, *vl-*, *vision*). The comment on the SupportsVision function and the function comment on visionCapableByName both call out the conservative intent. TestVisionCapableByName covers both directions, and TestSupportsVisionFallbackForUnknownModels covers the catalog + name-fallback composition. The fact that the test list explicitly includes gpt-oss:120b, kimi-for-coding, deepseek-coder, qwen2.5-coder, codestral, mistral-large as text-only is the right discipline.
4. Composer cursor now blinks, including for an empty box. composerBlinkMsg / composerBlinkCmd (model.go:108-117) drive a custom 530ms tick that toggles m.composerCursorVisible. The cursor is rendered via composerCursor(...) only when the flag is on, so an empty focused box now shows a blinking caret, the previous bug. textinput.Blink is still in Init() because the onboarding API-key input is a real textinput.Model (onboarding.go:56,107) that needs it — both blinks coexist cleanly. renderComposerVisualLine and renderComposerInput thread cursorVisible through, and the empty-box branch in renderComposerInput adds a single space when the caret is blinked off so the placeholder column stays stable. No new lint findings.
5. Ctrl+E releases mouse capture for native text selection. Toggling m.mouseReleased flips wantsMouseCapture() to false in mouse.go:wantsMouseCapture, which means the app stops requesting the terminal's mouse-capture mode, so the user can drag-select and copy text natively. The system notice is honest about what got toggled and how to toggle it back. The ⌥Option drag escape is documented in the PR body and a one-liner would be nice in the source too. TestMouseReleaseDisablesCapture is the right minimal test.
Non-blocking follow-ups
-
Unconditional backslash unescape breaks Windows paths with spaces.
unescapeDroppedPath(image_attach.go:64-78) drops a backslash before every following byte, which is correct for the macOS Terminal escape (Screenshot\ 2026.png→Screenshot 2026.png) but mangles a Windows path that already contains backslashes as path separators —C:\Users\test dir\file.pngbecomesC:Users estfile.png(the leadingC:\becomesC:, the\becomes justdroppedAttachmentPathreturnsfalse, so the path is treated as a text paste — a UX bug, not a security hole, but real for Windows users dropping a file whose path contains a space. Two reasonable fixes: (a) only unescape when the unescaped form actually exists on disk; (b) keep anif runtime.GOOS == "windows"short-circuit that skips the unescape. A regression test in the existingTestDroppedAttachmentPaththat creates a path containing a space on Windows (or skips viat.Skipon Unix) would lock the behavior in. -
The vision heuristic has implicit forward assumptions on
grok-4andclaude-3/claude-4. A hypotheticalgrok-4-textorclaude-3-textwould match the first heuristic clause (strings.Contains(id, "grok-4")/strings.Contains(id, "claude-3")) and be incorrectly marked as vision-capable. The catalog is authoritative, so the actual blast radius is "models the catalog doesn't know about yet" — which is the heuristic's whole purpose. Worth a one-line comment invisionCapableByNamecalling out that the family-substring matches assume "all members of family X are multimodal" and that a future text-only sibling would need either a tighter match (e.g.grok-4-vision) or a catalog entry to override. Not a blocker; the current test list is the strongest signal of intent. -
droppedAttachmentPathdoesn't try the original (unescaped) form on stat failure. Closely related to #1 — when the unescape mangles a Windows path with spaces, the function gives up rather than trying the original input. Even without a runtime GOOS check, the function could: (a) stat the unescaped form, (b) if that fails AND the original was different, stat the original. Catches the Windows-path case without needing platform-specific code.
The shape is right. Happy to see this land once #1 has at least a regression test (the fix can be a follow-up).
|
Verdict: APPROVE. Walked the 10 files (416 +, 57 -) at What works
Non-blocking follow-ups
The shape is right. Happy to see this land once #1 has at least a regression test (the fix can be a follow-up). |
…n fallback visionCapableByName treated any id containing "vision" as multimodal, so a model named "...vision-less..." (i.e. WITHOUT vision) was wrongly classified vision- capable — which silently skipped the non-vision image gate and broke TestRunExecDropsImagesOnNonVisionModelWithWarning in internal/cli. Add mentionsVision, which still matches the "vision" word but excludes the negated forms (vision-less / visionless / no-vision / non-vision). Used for both the grok-vision and the general open-multimodal fallback. Added negation cases to the name table.
… unescape)
unescapeDroppedPath stripped backslashes to undo Unix terminal drag-drop escaping
("\ " -> " "), but on Windows the backslash is the path separator, so a plain
dropped path like C:\Users\x\file.png became C:Usersxfile.png and never resolved
-- TestDroppedAttachmentPath failed on windows-latest.
No-op the unescape on Windows (dropped paths there arrive quoted or plain), and
gate the Unix-only backslash-escaped positive case behind runtime.GOOS.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
No blocking findings from diff review.
Reviewed the drag-drop attachment flow, inline image chips, vision fallback, cursor/backspace behavior, and copy handling. git diff --check passed for this PR. I did not run the full branch test suite.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
APPROVE (re-review)
Two new commits land on top of the original 6f87cc3 review: f79c5e6 (vision-less name fix) and 0c69d78 (Windows path unescape fix). Both address the concrete bugs in my prior follow-ups; no new regressions spotted.
f79c5e6 — mentionsVision excludes negated "vision" forms
internal/modelregistry/vision.go:71-83 introduces a small helper that requires the word "vision" but rejects six negated variants (vision-less, visionless, no-vision, non-vision, novision, nonvision). The two call sites in visionCapableByName (the grok clause and the open-multimodal family clause) now route through it. The new test cases in TestVisionCapableByName (my-custom-vision-less-model, no-vision-model, grok-vision-less) confirm the negated forms are now text-only.
This was a real bug — a model explicitly named for LACKING vision was being treated as multimodal, which silently skipped the non-vision image gate and broke the TestRunExecDropsImagesOnNonVisionModelWithWarning test. Fixed cleanly.
0c69d78 — Windows unescape is a no-op
internal/tui/image_attach.go:57-63 short-circuits unescapeDroppedPath on runtime.GOOS == "windows". The test in internal/tui/dropped_attachment_test.go:24-32 now conditionally excludes the "backslash-escaped drop" positive case on Windows, with a comment explaining why. Backslash remains a path separator on Windows, so the unescape was correctly diagnosed as a Unix-only convention.
Verified TestVisionCapableByName and the modelregistry suite (44 tests) pass against origin/feat/image-input-ux. The TUI build couldn't be exercised in this session because the branch is stacked on the not-yet-merged #246, but the change is local to two files and the test surface is complete.
Out of scope, still
My prior follow-up #3 (droppedAttachmentPath doesn't try the original form on stat failure) is a P3 and the runtime.GOOS gate makes the Windows case reachable. Not gating; can land as a follow-up.
Happy to see this go in.
|
Re-review posted (id Both follow-ups from the original review are now addressed:
Verified The third follow-up (try the original on stat failure) is a P3 and the runtime.GOOS gate makes the Windows case reachable; happy to see this land without it. |
3 participants
Improves the image/PDF attachment flow and two long-standing composer papercuts. Independent of the sub-agent delegation PR; touches only
internal/tui+internal/modelregistry.What's new
/…command. The dropped path is detected on paste and on submit (absolute / backslash-escaped / quoted paths that resolve to an existing image or PDF).[Image #1]/[Doc #1]chips inside the input box; the verboseAttached … (image/png)system line is gone. Backspace on an empty composer removes the last chip (/image clearstill drops all).Ctrl+Emouse release — toggles terminal mouse capture off so native drag-select + copy works, then back on for clickable UI. (Holding ⌥Option while dragging also works without toggling.)Tests
dropped_attachment_test.go— drop-path detection (escaped/quoted/plain positives; text, non-existent, non-image, multi-line negatives).vision_name_test.go— vision-by-name families + unknown-model fallback + text-only stays refused.mouse_release_test.go— capture forced off when released.attachment_remove_test.go— last-chip removal ordering (doc before image, labels in lockstep).image_attach_test.gochip-render assertions for the numbered format.gofmt·go vet·go build ./...·go test ./internal/tui/... ./internal/modelregistry/... -raceall green.Summary by CodeRabbit
Release Notes
#N], [Doc#N])