Repository navigation
⌨️ feat: Keyboard Submit, Focus and Escape for Ask User Cards - #17231
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 545e487041
ℹ️ 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".
| /** Batches whose arrival already placed focus. A batch remounts when it moves | ||
| * between the composer popover and the chat card; only its first arrival may | ||
| * take focus, or every move would pull the user back onto its options. */ | ||
| const arrivalFocused = new Set<string>(); |
There was a problem hiding this comment.
This module-level mutable set is new UI state outside the feature's Jotai state, so it survives provider, conversation, and session lifetimes and is never cleared; every focused batch remains registered for the tab lifetime, and a later reuse of an action ID will incorrectly suppress arrival focus. Keep the focus-once flag alongside the existing per-action Jotai form state so its ownership and lifecycle are scoped to the feature.
AGENTS.md reference: AGENTS.md:L162-L167
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f779636: the focus-once flag now lives in the batch's Jotai form atom (askQuestionsFormFamily) and resets with the form on submit.
| className="popover border-border-light bg-surface-secondary rounded-theme-popover flex max-h-[60vh] flex-col border p-2 shadow-lg [view-transition-name:ask-question]" | ||
| onKeyDown={handlePopoverKeyDown} | ||
| onKeyDown={(e) => { | ||
| if (isCancelKey(e.nativeEvent)) { |
There was a problem hiding this comment.
When focus is inside a single-question popover control and an IME composition is active, Escape is meant to cancel that composition, but this branch collapses the popover because it does not check e.nativeEvent.isComposing. Both chat-card Escape handlers already apply that guard; add it here as well so an IME cancellation does not unexpectedly move the question to the chat.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f779636: popover Escape now requires !isComposing, with a test that fails without it.
| event.key !== 'Enter' || | ||
| event.shiftKey || | ||
| event.altKey || | ||
| question.multiSelect === true || |
There was a problem hiding this comment.
When an unselected single-select option is focused on the last step (including a one-question batch), holding Enter first triggers the button's default click and selects it; the next auto-repeat keydown then sees aria-pressed="true" here and immediately submits the batch. The same held key therefore both chooses and irreversibly confirms an answer, so repeated keydown events should be ignored before calling confirmStep() while a fresh second Enter should remain supported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f779636: the picked-option confirm ignores event.repeat, so a held Enter only picks; a fresh second Enter still submits (unit test plus e2e ask-batch-completes-from-keyboard).
| event.shiftKey || | ||
| event.altKey || |
There was a problem hiding this comment.
When this batch is rendered in the composer popover, its single-line answer input is inside ChatForm's <form>. Returning here for Shift+Enter or Alt+Enter without preventing the native default permits implicit form submission, so during a paused run—especially when the action slot contains only a non-submit Stop control—the form can route a preserved composer draft instead of leaving the modifier chord inert. Prevent the default Enter action for these modifier cases before returning; only the unmodified/Ctrl/Cmd paths should continue to confirmStep().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f779636: Shift, Alt and repeated Enter now preventDefault before returning, so the ChatForm cannot submit implicitly; covered by a test that fails without it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7796360dd
ℹ️ 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".
| * steer/queue actions. | ||
| */ | ||
| const handleAnswerKeyDown = (e: KeyboardEvent<HTMLTextAreaElement>) => { | ||
| const action = resolveComposerKeyDown(e.nativeEvent, { |
There was a problem hiding this comment.
When submitMessage is rebound to a non-Enter chord such as Ctrl+J, pressing that chord in this answer textarea does nothing: resolveComposerKeyDown immediately returns none for every non-Enter key, while the window shortcut handler deliberately yields submitMessage whenever a non-main editing field has focus (useKeyboardShortcuts.ts:1131-1135). Handle a matching non-Enter submitOverride locally so the answer box actually follows the user's configured submit binding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3bc4589: a submit chord rebound off Enter now submits from the single-question answer box and confirms the batched answer field (shared isReboundSubmitChord), with tests that fail without it.
The interactive ask_user_question card's answer textarea had no key handling, so the answer could only be submitted by clicking. Route its keydown through resolveComposerKeyDown so it follows the composer's Enter-to-send preference, a rebound submit chord, and IME composition. The run is paused rather than generating, so it resolves as idle and Ctrl/Cmd+Enter submits instead of reaching the during-run actions.
- Focus the first question's controls when a batch arrives, only when focus is idle (body, or the empty composer that sent the turn), and focus into the next step on advance instead of the fieldset: the picked or first option, else the answer field. - Enter on the already-picked single-select option confirms the step; Enter on an unpicked option stays its click, so a pick never submits by itself (WCAG 3.2.2). Multi-select rows only toggle. - Ctrl/Cmd+Enter confirm from the answer field like Enter; Shift+Enter never does.
Review findings on the batched ask_user_question card, and what this commit does about each. 1. Held or double-tapped Enter could pick answers and submit the batch. After Enter confirmed a step, focus moved to the next step's first option. A keyboard Enter on a focused button is its click, so the key repeat of a held Enter (or a quick double-tap) picked that option and advanced again. On the last step the next repeat landed on the now-pressed option, confirmed it, and submitted the whole batch with answers the user never chose. Fix: after a step change, focus goes to the step's picked option, else its answer field. An unpicked option is only a focus target when a batch first arrives. Repeats now reach a text field, which can page on but never picks an option or submits on the user's behalf. 2. Focus-on-arrival ran on every mount, not only on arrival. Its effect has empty deps, so moving the batch between the composer popover and the chat card, or remounting a message that still holds an unanswered batch (reopening a conversation, a Share view), pulled focus onto an option of that batch. The next Enter or Space would then answer it. Fix: `AskUserQuestions` takes a `focusOnArrival` prop, set only by the surface showing the live pause (the popover, or the chat card while it is live and visible), so other mounts never take focus. Focus is placed at most once per batch, tracked by action id, so a move between surfaces is not treated as a new arrival. Tests: a held Enter from an answer field picks nothing and never submits; a surface that isn't the live pause leaves focus alone; a remount of the same batch doesn't take focus again. All three fail without this change. The Chat/Messages/Content, Chat/Input, Chat/ask and hooks/Input suites pass, apart from TokenUsage/index.spec.tsx, which fails the same way without this change. tsc, ESLint and Prettier are clean on the changed files.
Escape in the chat card skips the question or batch, but only before anything is entered, so a stray Escape cannot discard an answer and resume the run. Escape with focus inside the composer popover moves the question to the chat, as it already did from the composer, and returns focus to the composer once the popover closes. Also drops the em dashes from the batched card's comments.
Ignore auto-repeated Enter before confirming a picked option, so a held Enter cannot both pick and submit the last question. In the answer field, Shift, Alt and repeated Enter now prevent the enclosing chat form's implicit submit instead of returning early. Escape that cancels an IME composition no longer moves the popover to the chat, and the arrival-focus flag lives in the batch's Jotai form state.
The window shortcut handler yields submitMessage in every editing field except the composer, so a chord such as Ctrl+J did nothing in the ask answer fields. Both the single-question answer box and the batched answer field now act on it, through a shared isReboundSubmitChord.
3bc4589 to
79c7051
Compare
Pull Request
Summary
The
ask_user_questionchat cards had almost no keyboard support. The single-question card's answer box ignored Enter, so the only way to send an answer was to click Submit. In the batched card, focus landed on the step'sfieldsetafter each question, so every step needed a Tab before it could be answered. Neither card had a way to cancel from the keyboard.This PR carries @dev-elie's work from #16832 with their commits intact, and adds Escape. The single-question answer box now follows the composer's Enter table: Enter submits with Enter-to-send on, Ctrl/Cmd+Enter submits with it off, Shift+Enter never submits, and an Enter that confirms an IME composition is ignored. The batched card takes focus on arrival only when focus is idle, moves it to a real control on each step, and lets Enter on the picked option confirm it. Escape in a chat card skips the question or batch, but only before anything has been entered. Escape with focus inside the composer popover moves the question to the chat, matching what Escape already does from the composer, and puts focus back in the composer. A held Enter never both picks and submits, Shift/Alt/held Enter in the popover's answer field can no longer submit the chat form underneath it, and a submit shortcut rebound off Enter (for example Ctrl+J) works in both answer fields.
Fixes #16816
Supersedes #16832
How it works
Escape never throws away input. With a typed answer, a checked option or a partial batch, Escape in a chat card does nothing, because Skip resumes the run with a decline that can't be undone. In the popover, Escape is a move rather than a decline, so it always applies. A batch keeps the composer disabled while its popover is open, so focus goes back to the composer in an effect after the popover closes, not in the key handler.
Type of change
Testing
Tested environments/configuration:
openaigpt-5.1for the Enter and focus behavior (from ⌨️ feat: Keyboard Submit and Focus for Ask User Question Cards #16832)Automated tests:
e2e/specs/mock/scenarios/ask-keyboard.spec.ts: a batch answered and submitted from the keyboard alone, Escape in the popover moving the batch to the chat with focus back in the composer, and Escape in the chat card skipping only while nothing is answered. All pass in the three projects.AskUserQuestionPopover.test.tsx, plus Escape, held-Enter, implicit-submit and rebound-shortcut cases inAskUserQuestion.test.tsx,AskUserQuestions.test.tsxandshortcuts.spec.ts. Each fails with its source change reverted.npx jest src/components/Chat/Messages/Content src/components/Chat/Input src/components/Chat/ask src/hooks/Input src/utils/shortcuts: 159 suites, 3099 tests pass.npx tsc --noEmit -p client/tsconfig.jsonand ESLint on the changed files are clean.Screenshots / recordings
No visual change. The cards look the same; only keyboard behavior changes.
Risk / compatibility
AskUserQuestionstakes two optional props:focusOnArrival(defaultfalse) andonEscape(defaults to Skip).Checklist