Repository navigation
feat(prompt-input): add the PromptInput toolbar parts - #118
Conversation
Add PromptInputToolbar with an add menu, icon buttons, model and mode pickers, a float above the box, a context chip and a note. The pickers are select-only comboboxes and the add menu is a menu button; both keep or return focus to their trigger and need no optional peer. A toolbar placed last in PromptInput takes its own row.
bfd2e10 to
a52195f
Compare
a52195f to
35dd216
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 (re-verified against branch head 35dd216).
-
should-fix a11y
src/ui/prompt-input-toolbar.tsx:156, 399(+use-dismissable-popover.ts:79-84) — popups the host opens don't get the dismissal net:closeOnEscape: falsedisables the hook's document-level Escape handler, so Escape is handled only on the trigger and inside the popup body, and Tab only inside the popup. A keyboard-only user with focus in the textarea cannot dismiss an open AddMenu/select at all (the only remaining dismissal is an outside pointerdown). Smallest fix: a capture-phase document Escape handler (with stopPropagation when open) plus root-level Tab handling; add the host-opened regression test. -
should-fix a11y
toolbar.tsx:494, 552— the trigger'saria-label={label}replaces the whole accessible name, so a screen-reader user never hears the chosen model/mode while the popup is closed (sighted users see "Claude Opus", they hear "Model"). Fix:aria-label={chosen ? \${label}: ${chosen.name}` : label}or a visually-hidden span +aria-labelledby` including the value; regression-test that the accessible name contains the chosen option. -
minor api-design — toolbar parts ignore
PromptInput'sdisabled(the context is module-private and no part reads it; each takes its owndisabled). Stories demonstrate the threading but no docstring states the contract — document it, or let parts consume the context. -
nits confirmed —
assignRefintoolbar.tsx:12-15is character-identical toprompt-input.tsx:209-212(consider one sharedsrc/lib/ref.tshelper; the same idiom recurs ~9× across the series); Home/End open a collapsed select but the docstring's opener list omits them (:510-513vs:361-365);PromptInputToolbar/Float/Notehave no unit tests (prompt-input-toolbar.test.tsxnever renders them); select-only combobox typeahead is absent (APG-optional, fine to defer — just note it).
35dd216 to
1fd1dcd
Compare
A toolbar popup the host opened stayed open when Escape was pressed in the textarea, because the popups opt out of the shared hook's Escape handling to run their own. Listen for Escape on the document in the bubble phase while open and close when focus is outside the popup, leaving focus where it is. Handled Escapes are skipped, so the suggestion list still closes first. The toolbar parts each took their own disabled flag and ignored the one on PromptInput, so a disabled prompt still offered a live add menu, selects and buttons. Expose usePromptInputDisabled and default each part's disabled to it; a part's own disabled still wins. Also say that Home and End open a select on the first or last option, and cover the toolbar, float and note parts with a test.
The select and add menu rows stacked a bold name over its description and marked the active row with an accent stripe as well as a fill. Draw each option as one row, the description inline and muted, mark the active row with the neutral fill alone, as Menu does, and keep the check for the chosen option. Rows sit flush with less padding.
1fd1dcd to
3329560
Compare
|
Thanks for the review. Here's what happened to each point. Fixed1. Escape in the textarea couldn't close a popup the host had opened (should-fix)
2. The toolbar ignored
3. Nits
Not changed, and why
Also in this PRA
Commits: |
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
Adds the PromptInput toolbar: add menu, model and mode selects, and the icon button.
Findings
-
src/ui/prompt-input-toolbar.tsx:278— Escape inside the add menu or a select closes a dialog behind it. Both handlersstopPropagationin the bubble phase (prompt-input-toolbar.tsx:569for the select). RadixDismissableLayerlistens for Escape ondocumentin the capture phase, so the dialog dismisses before this handler runs. Stop the key in capture, orpreventDefaultbefore the layer sees it, so one Escape closes only the popup. -
src/ui/prompt-input-toolbar.tsx:56— A host-opened popup closed from outside (the textarea) does not mark Escape handled.useCloseOnEscapeOutsidecallssetOpen(false)and leavesdefaultPreventedfalse, and it also listens in the bubble phase, so a dialog's capture listener has already run. The suggestion list stops Escape on the textarea. This path should too. -
src/ui/prompt-input-toolbar.tsx:313— A disabled prompt still accepts a choice while the popup is open.disabledis only on the trigger (:252,:544). The menu stays mounted whenopenis true, and the itemonClickchecks per-itemdisabledonly.choose(:478) does not check the select'sdisabledeither (:505).defaultOpen, a controlledopen, or disabling the prompt while the menu is up still adds a file or changes the model. Ignore the click when the part is disabled, and close the popup when it becomes disabled. -
src/ui/prompt-input-toolbar.tsx:218— Dismissing the add menu with a pointer press on a non-focusable node drops focus todocument.body. Outside dismiss callssetOpen(false)(use-dismissable-popover.ts:77) and does not focus the trigger;close()would. The note at:172is right when the press lands on the textarea. When it lands on something that does not take focus, return focus to the trigger. -
Commit
style(prompt-input): tighten the toolbar popupsusesstyle, whichCONTRIBUTING.mddoes not list.refactormatches that change.
3329560 to
e9d2552
Compare
A dialog dismisses on Escape from a capture listener on the document, so the popups' bubble-phase stopPropagation never kept it open: one Escape closed both the popup and the dialog around the prompt. While open, each popup now prevents Escape in the window's capture phase, ahead of the dialog. Escape from outside the popup, such as the textarea beside a popup the host opened, closes it there too, unless the suggestion list is showing and takes that Escape first. A disabled part now shows its popup closed, and turning disabled while open closes it for the owner, so nothing can be chosen from a popup in a disabled prompt, as the toolbar's docs already promised. The add menu kept the focus request of an ArrowUp the owner declined, so the owner's own later open moved focus into the menu. The request now lasts only for the render that answers it.
e9d2552 to
d2c36ab
Compare
|
Thanks for the review. Four of the five points are fixed. The outside press is by design. 1. Escape inside the add menu or a select also closed a dialog around the prompt
2. Escape from the textarea closed a host-opened popup without marking it handled
3. A disabled prompt still accepted a choice from an open popup
4. Closing the add menu with a press on plain content dropped focus to the body
5. The
Also in this PRThe add menu had the same stuck ArrowUp flag you found in #121's menu. If a controlled owner declined an ArrowUp open, the owner's own later open moved focus to the last item. The request now lasts only for the render that answers it. The test "an ArrowUp the host declined does not make its later open take focus" failed before the fix. Commits: |
While a toolbar popup was open it took every Escape in the window whose target lay outside the popup, closing itself and preventing the event. A dialog opened elsewhere on the page then saw its Escape as handled and stayed open while the popup behind it closed, and an Escape inside the add menu also closed a model list the host had opened beside it. A popup now closes on an outside Escape only when it comes from the same PromptInput, such as its textarea, and not from inside another open toolbar popup, which handles its own. Each popup's root carries data-state="open" while open, so that check reads the DOM. Escapes from anywhere else are left alone for the layer they were pressed in.
Only each select option prevented the mousedown that moves focus, so a press on a group heading, the list's padding or the gap between rows dropped focus to the page. The trigger, which holds the list's keys, then got none: the arrows did nothing and Escape closed the list without returning focus. The add menu had the same gap around its items. The listbox and the menu now prevent that mousedown as a whole, so focus stays on the select's trigger or the focused menu item wherever the press lands, and clicks still choose.
PromptInputAddMenu and the model and mode selects read ref from their props, which only React 19 passes to a function component; under React 18, inside the peer range, the ref was dropped with a warning. Wrap them, and the select they share, in forwardRef so a ref reaches each part's root element on both versions.
d2c36ab to
a06fe49
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.
|
a06fe49 to
227dc18
Compare
Superseded by the completed stacked review; blocking fixes were addressed and the rebased stack is being landed.
Add PromptInputToolbar with an add menu, icon buttons, model and mode pickers, a float above the box, a context chip and a note. The pickers are select-only comboboxes and the add menu is a menu button; both keep or return focus to their trigger and need no optional peer. A toolbar placed last in PromptInput takes its own row.
Stacked on #117: review only this commit. Part of #94.
Notes for review
disabled; PromptInput's context stays private, so a host disabling the prompt passesdisabledto the toolbar parts too.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 #103