Repository navigation
Fix multi-select prompt ignoring space and submitting nil on Enter - #141
Conversation
The multi-select branch of the dialogue prompt handler only recognised a space bar press as Ch == ' '. Input backends deliver the space bar as Key=KeySpace, which that check missed, so pressing space in an ask_user_question multi-select prompt never toggled anything, and Enter with nothing checked resolved the prompt with nil. tui_prompter treats a nil result as "prompt dismissed", so the agent was told the user cancelled when they had in fact pressed Enter without being able to tick anything. Accept Key=KeySpace, keep Ch == ' ' for synthetic and legacy events, and make PreparePromptSelect leave the prompt open when nothing is selected rather than resolving it with nil. Single-select always returns the label under the cursor, so only a multi-select with no boxes checked is affected. The handler unit test now covers both space encodings and asserts that an empty Enter keeps the prompt active, and an end-to-end test drives <enter>, <space>, <enter> through handlertest.RunHandlerSequence and checks the rendered checkbox frames. Fixes unstablebuild#138 Signed-off-by: drakeo338 <[email protected]>
There was a problem hiding this comment.
This also needs to be changed. In a multi-select prompt, Selected() returns only the checked labels, not the option under the cursor.
• If nothing is checked (cursor on Other), pressing <enter> causes the extension to panic (index out of range.
• If say "A" is checked, but cursor is on "Other", pressing <enter> causes promptLabel to be "A", so
handler.go:325 sends ["A", ]. The typed text is attributed to the wrong option.
• Suggested fix: take the label from the option under the cursor, s.options[s.cursor].Label, through a new Selection.CursorLabel() method.
Add an integration test for multi-select plus <enter> on "Other", with and without other boxes checked.
There was a problem hiding this comment.
Done in 4804231: the label now comes from the option under the cursor through a new Selection.CursorLabel(). Added the integration test for multi-select + enter on Other, with nothing checked (was the index-out-of-range panic) and with A checked (was attributed to A).
| if ev.Ch == ' ' { | ||
| // The space bar arrives as Key=KeySpace; Ch=' ' is also | ||
| // accepted for synthetic and legacy events. | ||
| if ev.Key == term.KeySpace || ev.Ch == ' ' { |
There was a problem hiding this comment.
you should check that ev.Mod == 0, otherwise <ctrl-space> and <meta-space> also toggle it.
There was a problem hiding this comment.
Done in 4804231, the toggle now requires ev.Mod == 0, with a test for ctrl-space and alt-space (meta arrives as alt here).
StartPromptInput read the option label for a RequiresInput selection from Selected()[0]. In multi-select mode Selected() returns only the checked labels, not the option under the cursor, so pressing <enter> on "Other" with nothing checked indexed an empty slice and panicked, and with another option checked it attributed the typed text to that checked option instead of "Other". Add Selection.CursorLabel(), which always reads the option at the cursor, and use it in StartPromptInput. Also require ev.Mod == 0 before treating a key event as the space bar toggle, so ctrl-space and meta-space, which are already bound to other actions in this handler, no longer also toggle the checkbox under the cursor. Signed-off-by: drakeo338 <[email protected]>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3 participants
Fixes #138.
The multi-select handler only matched space as
Ch == ' ', but the terminal sends it asKey=KeySpace, so space never toggled a box. It now accepts both. Enter with nothingchecked keeps the prompt open instead of resolving it with nil, which the agent read as
a cancel. The tests now send the real
Key: KeySpaceevent, and a new end-to-end testdrives enter, space, enter and checks the rendered checkboxes.