Conversation
Raised in field use. Dragging a file onto a terminal is how people already hand a path to a program, and it is the natural gesture for ctrl+t — which opens a prompt you have to TYPE a path into, and typing a path is the part nobody wants to do. Recorded with what is actually in the way rather than as a wish. A terminal delivers a drop as a PASTE, and dialog.Model.Update handles KeyPressMsg and nothing else, so a tea.PasteMsg arriving while the attach prompt is open is dropped on the floor. And the path arrives shell-quoted — backslash-escaped spaces on iTerm2 and Terminal.app, single quotes elsewhere, a file:// URL from some Linux terminals — so what lands is not a path a stat call will find. Unquoting is the actual work, and getting it wrong fails on exactly the files people drag. The entry also names the decision to make before any of it is written: a drop onto the composer with no prompt open could stage the attachment directly, but that needs a rule for telling a dropped path from pasted prose, or a paste that merely looks like a path silently becomes an attachment instead of the message somebody meant to send. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…placeholders Field review of what is still open. Four of the seven were already in the file and are now costed against the code rather than left as titles: - mute/unmute is half-built (reading is live; only the write RPC and a duration grammar are missing), and is the cheapest of the four commands - jump <date> needs no new RPC — getHistory already takes OffsetDate and the dialog cursor already passes one; the work is parsing and scrolling - theme <name> is mostly wired: one entry point, thirteen SetRoles call sites, a config key. What is missing is more than two palettes and any call to SetRoles after startup - reload-config shares that wiring, so it is one piece of work with theme Three were open and one was not recorded at all: v visual selection, which is the same need as mouse selection reached by the keyboard, and the largest of the three — the first thing in the thread grid that needs a region rather than a cursor. New, from field use: - Contacts. The reading works and the item is smaller than it looks: contacts.getContacts is called and seeds the peers manager. What is thin is adding one (unimplemented) and the overlay, which is pre-TUI-2.0. - The rail says nothing about the person in a 1:1 — a private chat gets files and links and no identity section at all. - A palette command for the cursored message's sender, in any chat type. The last two are one piece of work: both need users.getFullUser, which this client has never called. Two placeholders, not to be actioned: the ctrl+t attach surface goes to a designer first, and the drop-a-file item belongs in whatever replaces it rather than beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the code The designer's proposal for the ctrl+t surface, extracted from §16 of the original handoff. Archived verbatim in docs/handoff/ with its checksum, because that directory is the source of record for what was supplied — and because the existing archive README already says why a handoff copy is read rather than implemented from. Reviewed against the code. The geometry holds and both load-bearing claims check out: attach-file really is the only dialog.NewPrompt caller, and the glyphs really do exist. It also names the right defect — the old prompt hardcodes false for asPhoto, so ctrl+t always attached as a document while ctrl+v attached an image as a photo. Nine points recorded against it in TODO.md, none of them a reason to wait: - #7f8a93 and #9aa4ac are in no role; they fail TestNoColourLiteralsOutsideThePalette and are wrong under 256 colours and under the light theme - ^h and backspace are the same byte outside the Kitty protocol, and the picker binds both to different things - ^p moves the selection in the palette, which this component is a twin of, and expands the composer everywhere else - ▷ for video exists nowhere; media.go draws ▶ for video, animation, voice and audio alike - theme.OverlayInput is not dead — auth and search both call it — so it must not go with the prompt - the mode wiring should stay in textOverlayOpen (INSERT), not move to the palette's slot (COMMAND) - "after it closes" describes an expanded composer that did not ship: the header is "compose | sends as" and the chip ends in "esc to drop" - six rows rather than the palette's eight is deliberate and correct — the divider and state row cost the other two - the drop-a-file item is the same question and belongs in the same work Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answers rather than questions, so the component can be built from the entry. Keys. The spec puts ^p on the photo/document toggle, but the palette binds ^p/^n to selection movement (divergence 9) and this component is its twin by construction — a reader arrives having already learned it, and would press ^p to move up and silently change how their file sends. Selection movement takes ^p/^n; the toggle goes to ^t, the key that opened the surface. ^y is claimed by the emacs composer item, alt+p arrives as a composed character on macOS, ^i is Tab, ^s/^q are flow control, ^o is the composer's $EDITOR chord. ^h is dropped rather than bound. It is byte-identical to backspace outside the Kitty protocol, and the fix is a better reading of the key that already exists: with an empty tail, backspace removes the last path segment, which is what "up one directory" means and what a shell user's fingers already do. A terminal that conflates them now does the right thing by accident. A tenth point found working this through: "never past ~/" leaves no route to /etc/hosts and the spec offers no other one. The floor is the empty prompt — a leading / goes absolute, ~ goes home — and ^u clears the path, which every other text surface in this app already offers. Colours. Both literals become Dim, documented as "secondary copy", which is what a path's directory part and a directory's name both are. It widens the separation the spec was reaching for rather than approximating it: #9aa4ac against #c9ced4 was barely a difference. The decisive argument is the light theme, where roles invert and a literal cannot — two light greys on a #f4f6f8 panel. Noted rather than fixed: Ghost and Faint are adjacent under the 256-colour profile, so size and mtime read as one weight there. A property of the shipped palette, not of this component. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…minal Ctrl+T raised dialog.NewPrompt: a centred rounded box with a title, a blind 30-column field and a [ Cancel ] [ OK ] row. It was the last GUI dialog in the client — it had buttons, it had title case, it could not complete a path, and it could not tell you whether what you typed existed until after it had failed. internal/ui/components/attach replaces it, from a design supplied on 2026-09-02 and archived at docs/handoff/attach-picker.md. It is the command palette's twin rather than the dialog's: 60 cells, the same anchor, the same ▌ marker, the same key-hint footer, no buttons. A prompt row with a ghost completion, six entries carrying type glyph, name, size and mtime, a state row under a rule-soft divider, and ⇥ to complete. It fixes a defect nobody had reported. The prompt passed a hardcoded false for asPhoto, so ctrl+t always attached as a document while ctrl+v attached the very same image as a photo — two ways to attach one file that disagreed about what it was, and nothing on screen said so. The state row states the send mode before you commit and ^t changes it. Dropping a file on the terminal now works, which is what ctrl+t is for: a drop is a path arriving at the surface that collects paths. It comes as a shell-quoted PASTE rather than as keystrokes, in three spellings, and UnquotePath undoes all of them. A drop with no picker open stages the file only when the paste is unambiguously a path to a file that exists — LooksLikePath is deliberately strict, because a paste that merely resembles a path must not silently become an attachment instead of the message somebody meant to send. Ten departures from the supplied design, recorded as divergences 49 and 50: - two colour literals became Dim; neither was a role, and under the light theme they were light greys on a #f4f6f8 panel - ^p moved to ^t: ^p is the composer's expand chord and reads as "move up" on a list overlay - ^h is not bound at all — it is backspace's own byte outside the Kitty protocol. Backspace on an empty tail goes up a directory instead, which is what a shell user's fingers already do and makes the collision harmless - movement is the arrows only, the palette's rule, not the ^p/^n pair - ⌫ is not fenced at ~/, which had left no route to /etc/hosts - ▷ for video exists nowhere; media.go draws ▶ for video, animation, voice and audio alike - theme.OverlayInput is not dead and did not go with the prompt — auth and search both call it - the picker does not expand the composer: the chip is visible in the inline form already, and expanding would spend eight rows of twenty-four - six rows rather than eight, which holds the overlay at the palette's height - the cursor is drawn before the ghost suggestion, not after it: a cursor says where the next character goes Two things found on the way. enter consulted the typed path before the cursor, and with no tail that path IS the directory being browsed — so it re-entered the current folder forever and the cursor was never reachable. And the help card and README had been advertising ctrl+p/ctrl+n for the palette since the day palette.TestTheEmacsChordsDoNotNavigate removed them. Deleted with the prompt: dialog.KindPrompt, its Update and View branches, NewPrompt, the prompt-specific hint, and DialogResultMsg.Input. dialog.Kind is down to the two that are genuinely a two-button and a one-button question. Every guarantee those tests held is re-homed to the picker first. 71 mutants, no survivors. Two could not be killed at first, which was the code doing something twice: Close reset four things Open resets again, and ^t re-checked what AsPhoto already knows. Both are one line now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0696416fb9
ℹ️ 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".
| path := attach.UnquotePath(msg.Content) | ||
| m.notify("attached " + filepath.Base(path)) | ||
| return m.stageAttachment(path, attach.IsImage(path)) |
There was a problem hiding this comment.
When a paste contains an existing ~/file path, LooksLikePath accepts it by expanding ~, but this branch stages the unexpanded result from UnquotePath. Sending later passes that literal path to os.Stat, so the attachment fails even though the paste was recognized as valid; expand or otherwise normalize the path before handing it to the composer.
Useful? React with 👍 / 👎.
| if m.screen == ScreenMain && m.composer.ChatId() != 0 && | ||
| !m.composer.IsEditing() && attach.LooksLikePath(msg.Content) { |
There was a problem hiding this comment.
When a confirm or alert dialog is open and the user pastes an absolute path to an existing file, this top-level branch runs before the later blockedByDialog input guard, silently stages the attachment behind the dialog, and changes focus while leaving the modal visible. Require that no input-owning overlay/dialog is active before treating a paste as a direct attachment.
Useful? React with 👍 / 👎.
| // been picked — one answer to "is this a photo", not two. | ||
| func IsImage(name string) bool { | ||
| switch strings.ToLower(filepath.Ext(name)) { | ||
| case ".jpg", ".jpeg", ".png", ".gif", ".webp", ".bmp", ".tif", ".tiff": |
There was a problem hiding this comment.
Selecting or directly dropping a .tif/.tiff file marks it as a photo, but the existing clipboard implementation explicitly excludes TIFF because Telegram's uploaded-photo API rejects it. The picker therefore advertises a valid photo mode and defaults to it, only for sending to fail; classify TIFF as a document instead.
Useful? React with 👍 / 👎.
| entry := Entry{Name: name, Dir: item.IsDir()} | ||
| if info, err := item.Info(); err == nil { | ||
| entry.Size = info.Size() | ||
| entry.ModTime = info.ModTime() |
There was a problem hiding this comment.
For a symlink to a directory, os.DirEntry.IsDir() is false, so an arrow-selected directory symlink is rendered and handled as a file; pressing Enter returns an attachment action and eventually fails because the upload target is a directory. Use the followed FileInfo already obtained below to classify directory symlinks so users can navigate through them.
Useful? React with 👍 / 👎.
| m.cursor += delta | ||
| if m.cursor < 0 { | ||
| m.cursor = 0 | ||
| } | ||
| if m.cursor >= len(m.filtered) { | ||
| m.cursor = len(m.filtered) - 1 |
There was a problem hiding this comment.
When a directory has more than six matches, Down can move cursor beyond maxRows, while the view always renders only the first six entries. The selection marker then disappears even though Enter still attaches the off-screen entry; scroll the rendered window with the cursor or prevent selection from moving beyond displayed rows.
Useful? React with 👍 / 👎.
Ceesaxp
left a comment
Ceesaxp
left a comment
There was a problem hiding this comment.
Changes are needed for two P1 correctness issues and three narrower P2 issues in the new attachment path. The implementation and test coverage are otherwise strong. Local validation passed: go test -race ./..., go vet ./..., focused package tests, and git diff --check. CI is also green, but its Windows job only cross-builds and cannot catch the runtime path failure below. (Posted as COMMENT because GitHub does not allow the PR-owning account to submit REQUEST_CHANGES.)
| m.reload() | ||
| return m, ActionNone | ||
| } | ||
| return m, ActionAttach |
There was a problem hiding this comment.
[P1] Preserve the exact typed path before returning ActionAttach. This branch proves that m.typed names an existing file, but it returns only an action; the app then calls Chosen(), which reconstructs the path from the current case-insensitive cursor selection. On a case-sensitive filesystem containing both Foo and foo, typing the exact name foo leaves Foo as the first filtered row and stages Foo. I reproduced this with a focused test against this commit. That can send a different file than the path the user explicitly entered. Please retain the validated typed path (or make Chosen prefer it) and add a Foo/foo regression test.
| // filters on nothing. A path with no separator at all is a prefix in the | ||
| // current directory. | ||
| func splitPath(typed string) (dir, tail string) { | ||
| if i := strings.LastIndexByte(typed, '/'); i >= 0 { |
There was a problem hiding this comment.
[P1] Make path parsing work with native Windows paths. Windows is a published target, and config expands the default download directory to a backslash path such as C:\Users\me\Downloads. collapseHome turns that into \Downloads, Open appends '/', splitPath only recognizes '/', and expandHome only recognizes '/', so the first Ctrl+T read targets a literal relative ~\Downloads/ path and the picker opens in an error state. Raw C:... drops are also rejected or have their backslashes consumed by UnquotePath. The green Windows CI job only cross-builds. Please normalize native paths or use filepath volume/separator semantics throughout, and cover drive-root/backslash paths in platform-specific tests.
| continue | ||
| } | ||
|
|
||
| entry := Entry{Name: name, Dir: item.IsDir()} |
There was a problem hiding this comment.
[P2] Follow directory symlinks when deciding whether an entry is navigable. DirEntry.IsDir reports false for a symlink to a directory, so selecting a symlinked folder and pressing Enter takes the file branch and stages the link as an attachment; the later uploader then rejects the resolved directory. Typing the full link name happens to work because enter uses os.Stat, which makes the cursor and exact-path behaviors inconsistent. Classify the joined path with os.Stat (while keeping broken links non-directories) and add a symlinked-directory navigation test.
| // been picked — one answer to "is this a photo", not two. | ||
| func IsImage(name string) bool { | ||
| switch strings.ToLower(filepath.Ext(name)) { | ||
| case ".jpg", ".jpeg", ".png", ".gif", ".webp", ".bmp", ".tif", ".tiff": |
There was a problem hiding this comment.
[P2] Do not default TIFF files to photo mode. The existing clipboard path explicitly documents that InputMediaUploadedPhoto rejects TIFF and therefore excludes or converts it, but this helper marks .tif and .tiff as photos. Picker selection and direct drop consequently call SendPhotoMessage and fail instead of sending the file as a document. Align this list with the proven clipboard/send capabilities (or convert TIFF before staging) and add a TIFF classification test.
| entry.ModTime = info.ModTime() | ||
| } | ||
| if entry.Dir { | ||
| entry.Items = countItems(filepath.Join(expandHome(dir), name)) |
There was a problem hiding this comment.
[P2] Avoid enumerating every child directory synchronously on the Bubble Tea update path. Loading one directory currently performs one os.ReadDir for the listing plus another full os.ReadDir for every visible subdirectory, retaining each temporary entry slice just to compute a count. Open, Enter, Paste, and navigation all call this synchronously, so a directory with many large children or a network mount freezes the entire TUI and can allocate proportional to all immediate child contents. Please make counts lazy/asynchronous or omit them from the initial render so navigation remains bounded by the directory being opened.
1 participant
What
Ctrl+Traiseddialog.NewPrompt— a centred rounded box with a title, a blind 30-column field and a[ Cancel ] [ OK ]row. It was the last GUI dialog in the client: it had buttons, it had title case, it could not complete a path, and it could not tell you whether what you typed existed until after it had failed.internal/ui/components/attachreplaces it, from a design supplied 2026-09-02 and archived verbatim atdocs/handoff/attach-picker.md. It is the command palette's twin rather than the dialog's — 60 cells, the same anchor, the same▌marker, the same key-hint footer, no buttons anywhere.It fixes a defect nobody had reported. The prompt passed a hardcoded
falseforasPhoto, soctrl+talways attached as a document whilectrl+vattached the very same image as a photo. Two ways to attach one file that disagreed about what it was, and nothing on screen said so.Dropping a file on the terminal now works, which closes the item tracked separately in TODO.md. A drop arrives as a shell-quoted paste rather than as keystrokes, in three spellings — backslash-escaped (iTerm2, Terminal.app), quoted, and a percent-encoded
file://URL — andUnquotePathundoes all of them. A drop with no picker open stages the file only when the paste is unambiguously a path to a file that exists.Ten departures from the supplied design
Recorded as divergences 49 and 50. The spec was good and named the right defect; these are where it met the code.
#7f8a93,#9aa4acDimTestNoColourLiteralsOutsideThePaletterefuses them, and under the light theme they are light greys on a#f4f6f8panel^p^t^pis the composer's expand chord and reads as "move up" on a list overlay^h⌫on an empty tail,←^his backspace's own byte outside the Kitty protocol. Folding it into⌫makes the collision harmless↑↓↑↓only^p/^npair was taken off the palette as a second spelling; adding it here would put them out of step again~/~/floor left no route to/etc/hostsand no other one was offered▷▶▷exists nowhere;media.godraws▶for video, animation, voice and audio aliketheme.OverlayInputauthandsearchboth call itFound on the way
enterwas unreachable past the first row. It consulted the typed path before the cursor — but with no tail, that path is the directory being browsed, and it resolves on every keystroke. Enter re-entered the current folder forever. Caught by a test, not by reading.The help card had been lying for four phases. It advertised
ctrl+p / ctrl+nfor the palette;palette.TestTheEmacsChordsDoNotNavigateremoved those deliberately, one spelling per action. The README said the same. Both fixed, with a test.Deleted
dialog.KindPrompt, itsUpdateandViewbranches,NewPrompt, the prompt-specific hint, andDialogResultMsg.Input.dialog.Kindis down to the two that are genuinely a two-button and a one-button question. Every guarantee the three prompt tests held is re-homed to the picker first.Testing
Build, vet and the full suite green. New:
internal/ui/components/attach(three test files, colour profile pinned inTestMain, clock pinned for the mtime column) andinternal/app/attach_test.go.71 mutants, no survivors. Two could not be killed at first, which was the code doing something twice:
Closereset four thingsOpenresets again, and^tre-checked whatAsPhotoalready knows. Both are one line now.Frame integrity is asserted at 60 cells across ASCII, CJK, emoji, over-long and empty listings.
🤖 Generated with Claude Code