Repository navigation
feat(prompt-input): add PromptInput with token highlighting - #115
pratikbuilds wants to merge 9 commits into
Conversation
5eae539 to
02e6592
Compare
02e6592 to
225a189
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Automated multi-agent review: independent review + second-agent validation; only confirmed items listed (all re-verified against branch head 225a189).
- minor (process) the plan specifies four commits (core input / mirror highlighting / measured sizing fallback / suggestions, with "the fallback is its own commit") — this PR is one commit. Split before merge or record the exemption in the PR body.
- minor a11y
src/ui/prompt-input.tsx:317—aria-autocomplete="list"is hardcoded whilearia-controls/aria-activedescendantare gated onlistOpen: with no suggestions part mounted, the field still claims list completion. Gate it:aria-autocomplete={listOpen ? "list" : undefined}. - should-fix a11y
prompt-input.tsx:316-319— the suggestion listbox's open/closed state is never exposed (noaria-expanded/aria-haspopupanywhere; observation confirmed). Careful with the fix:aria-expandedon a textarea is invalid per ARIA 1.2 / ARIA-in-HTML (axearia-allowed-attr), androle="combobox"is not permitted ontextareaeither. Smallest valid fix:aria-haspopup="listbox"(valid on textbox) plus a visually-hidden live region announcing suggestions shown/hidden —aria-activedescendantalready carries the current option while navigating. - should-fix
prompt-input.tsx:507— if a host setsstatus="submitted"withoutonStop, a focused Send becomesdisabledand focus drops to<body>(Chrome). Keep the button focusable witharia-disabled+ guarded click (the pattern exists intool-approval.tsx:205-207); add theactiveElementregression test. - should-fix
prompt-input.tsx:399-405vs:439— the render-callback signature never exposesoptionId(listId, token), so a host rendering its own element structurally cannot match thearia-activedescendantid: a dangling descendant is guaranteed whenever that option is current. Either wrap render-prop children in the id-bearing<li role="option">insidePromptInputSuggestions, or passoptionId(...)to the callback. - minor
src/hooks/use-prompt-input.ts:100, 214-221—caretrefreshes only on textarea events; a host-drivenonValueChangewith focus retained leaves a stale caret, so suggestions can pop for the wrong position. Re-readselectionStartin the value-sync layout effect when no pick is pending. - test gap — the Safari post-composition
keyCode === 229guard (use-prompt-input.ts:176) is real code but has zero test coverage in either unit or e2e layers (nokeyCodeoccurrence in any test file). Add the unit case{ key: "Enter", keyCode: 229, isComposing: false }aftercompositionend, or an e2e Safari-style sequence. - nit
src/hooks/use-scroll-current-into-view.ts:31—inline: "center"contradicts the hook's own docstring ("only the rail's own scroll position moves") on the horizontal axis; a wide option in a narrow viewport can scroll a page ancestor.inline: "nearest"(or make the hook take options).
225a189 to
e83902b
Compare
|
Thanks for the review. Here's what happened to each point. Fixed1. A focused Send button that turned disabled dropped focus to the page (should-fix)
2. Screen readers couldn't tell when the suggestion list opened or closed (should-fix)
3. A custom suggestion renderer could leave
4. A value the host set left the caret position stale (minor)
5.
6. Safari's Enter right after typing with an IME was untested (test gap)
Not changed, and why
Commit: |
e83902b to
606dab6
Compare
|
One more commit on this PR, |
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
Adds PromptInput, with @mention and /command tokens drawn on a mirror under the textarea.
Findings
-
src/hooks/use-prompt-input.ts:158— Accepting a suggestion when whitespace already follows the token leaves the caret inside the token on theexecCommandpath, so the list stays open and Enter will not send.insertPromptTokenreuses that space and places the caret after it (prompt-tokens.ts:100-105).pickinserts only the token text and returns beforependingCaretcan move the caret. Withask @ad nowand the caret on the space after@ad, the text becomesask @Ada nowandselectionStartstays on that space.findActivePromptTokenstill sees@Ada, so a second Enter callspickagain. The end-of-draft case hides this because the inserted text includes a new trailing space, which is what the e2e covers. After a successfulinsertText, set the caret tonext.caret. -
src/lib/prompt-tokens.ts:101— A reused newline is treated like a space, so the fallback caret jumps to the next line.WHITESPACEmatches\n,spacedis true, andcaretistoken.length + 1, which skips the line break (insertPromptToken("ask @ad\nnext", …)lands onn). Keep the caret before a reused newline. TheexecCommandfix above should use that caret, or it will jump lines in browsers too. -
Commit
style(prompt-input): match the suggestion rows to the toolbar popupsusesstyle, whichCONTRIBUTING.mddoes not list.refactormatches that change.
606dab6 to
aef6a93
Compare
|
Thanks, all three addressed in aef6a93 (fix) and f849185 (reworded to
|
aef6a93 to
f0075c9
Compare
|
One more commit on this PR, found while fixing the toolbar's Escape handling in #118. Escape on the open suggestion list also closed a dialog around the prompt
Commit: |
PromptInput is a fully controlled form. PromptInputTextarea draws @mention and /command tokens on an aria-hidden mirror beneath transparent native text, grows through field-sizing with a measured fallback, and never submits during IME composition. usePromptInput drives PromptInputSuggestions from the keyboard, and PromptInputSubmit turns Send into Stop while a reply is running.
A focused Send that turned disabled dropped focus to the page. That happened in the normal flow: a host that clears the draft on submit makes Send unsendable when the reply ends or Stop is pressed. Send now uses aria-disabled for "nothing to send", and the form's guarded submit keeps it inert. A host's own disabled is still a real one. The suggestion list's state was invisible to assistive tech, and a textarea may not carry aria-expanded. The textarea now has aria-haspopup="listbox", and aria-autocomplete is set only while a suggestions part is mounted. A polite status announces how many suggestions are open and empties when the list closes. A value set by the host moved the caret without a select event, so suggestions opened for a stale position. The value-sync effect now reads the caret again while the textarea has focus. Document that a suggestion render callback must return a PromptInputSuggestion, warn in development when the current option's id is missing, and cover Safari's keyCode 229 Enter after composition.
Drop the accent stripe from the active suggestion, keeping the neutral fill alone, and tighten the rows to match the toolbar's popups.
Picking a suggestion through the browser's editing left the caret after the inserted text. When a following space was reused that is still inside the token, so the list stayed open and Enter picked again instead of sending. Move the caret to where insertPromptToken puts it. insertPromptToken reused a following line break like a space, putting the caret on the next line. Reuse only a space within the line; before a line break, add one so the caret stays on its line and outside the token.
Radix layers such as Dialog dismiss on an Escape they see in the document's capture phase, unless it is already prevented. That runs before React's handlers, so the list's stopPropagation came too late and Escape closed both the list and the dialog around the prompt. While the list is open, prevent Escape from the window's capture phase, which runs first. PromptInputTextarea skips the hook when a consumer's onKeyDown prevents a key, so let an Escape the list claimed through.
The key Escape dismissed and the row the arrows chose outlived their token. Escape was keyed on the token's start and text, so retyping the same token, or typing it again in the next draft, opened the list and closed it at once. The arrow choice survived until a pick, so a later token's list opened on that row and Enter picked it. Clear both whenever the caret is in no token: on change, select and blur, and when the host sets the value. Narrowing within one token keeps the caret inside it, so the choice still holds there.
Only each option cancelled mousedown. A press on the list's padding, the gaps between rows or its scrollbar moved focus out of the textarea, whose blur closed the list, so a long list could not be scrolled with its scrollbar. Cancel mousedown once on the listbox instead. Scrollbar dragging and wheel scrolling still work, and custom option content is covered too.
usePromptInput prevents Escape from the window's capture phase while its list is open, before any React handler runs. A host that builds its own field and skips textareaProps.onKeyDown for prevented keys would never close the list, and nothing said so. Document it on the hook and on textareaProps.
PromptInputTextarea read `ref` from its props, which React 19 passes but React 18 strips from a function component's props, so a host ref never reached the textarea on React 18. Wrap it in forwardRef; the ref still merges with the one the prompt keeps for itself.
f0075c9 to
68923bf
Compare
Round 3 fixesA fresh review pass over the stack turned up the points below; each is fixed in a new commit at the end of this branch, and the stack is rebased onto it.
|
PromptInput is a fully controlled form. PromptInputTextarea draws @mention and /command tokens on an aria-hidden mirror beneath transparent native text, grows through field-sizing with a measured fallback, and never submits during IME composition. usePromptInput drives PromptInputSuggestions from the keyboard, and PromptInputSubmit turns Send into Stop while a reply is running.
Stacked on #114: review only this commit. Part of #94.
Notes for review
statuslives onPromptInput(root), notPromptInputSubmit: Enter must see the status so it never sends mid-reply;PromptInputSubmittakes onlyonStop.canSubmiton PromptInput (defaultvalue.trim() !== "") lets a host send attachments with no text, e.g.canSubmit={value.trim() !== "" || files.length > 0}; Enter and Send both honour it; disabled, busy status and IME composition still block.Verification
bun run typecheckandbun run linton its own (the stack is checked commit by commit).bun run check,bun run build,bun run dep-guard,bun run contrast-testandbun run test:e2e(Playwright specs and the Storybook axe sweep in light and dark) pass.Closes #102