Repository navigation
Conversation
Refs apache#6131 Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed range: merge-base 2463d86 .. head 2c768ff (one commit, 4 files). This is the first review, so there are no earlier findings to track.
Behavior. The change is small. The existing Tab branch in onInputKeyDown now also matches unmodified ArrowRight, and the hard-coded Tab kbd now shows the localized promptSuggestionAccept string. Which keys get captured and when:
- Empty draft only.
nextPrompt.textis defined only when the draft text is empty (usePromptSuggestionreturnsundefinedonceinput.text.length > 0), andonInputChangedismisses on every edit. So Right Arrow is only intercepted when the editor is empty. Caret position and selection don't matter there, and the arrow would not move the caret anyway. Once a draft exists, Right Arrow is never captured. The new "overwrite a started draft" test checks this (defaultPrevented === false). - Modifiers. Shift, Alt, Ctrl and Meta all fall through to native behavior. Tests cover each one.
- IME.
compositionStartdismisses the offer and setscompositionActiveRef. Composing keydowns are also stopped by the native listener before they reach React. Tested. - Trigger menus. The
aria-expanded !== 'true'guard and thedefaultPreventedcheck leave the mention and skill menus in charge of their own arrows. Tested. - Other handlers. Nothing else in
packages/ui/srcorapps/desktop/srchandlesArrowRightkeydown, so there is no conflict. Tab, Escape dismissal and ArrowUp/Down history are unchanged. - RTL and locale. UI locales are zh-Hans, zh-Hant and en. Because interception only happens on an empty editor, the physical-vs-logical arrow question has no caret-movement impact. Showing "→" in the hint is fine.
Tests. I built the PR and ran composer-shortcuts and prompt-suggestion: 42 pass, 0 fail. With the composer.tsx change reverted, the new ArrowRight acceptance test fails, so the test does guard the behavior. I did not run the Storybook test; it adds Undo and caret-at-end checks in Chromium.
Findings.
- P3 (optional hardening, also applies to the existing Tab path):
event.preventDefault()runs beforeacceptNextPrompt()checks its own guards (textPort.getValue().length, composition). If the DOM ever holds text before React re-renders and dismisses the offer, the key is swallowed and nothing is accepted. I did not reproduce this; it looks like a narrow race at most. One fix: haveacceptNextPromptreturn a boolean and callpreventDefaultonly when it accepted.
No P0-P2 defects found.
CI and mergeability. GitHub reports the PR as MERGEABLE, with state BLOCKED (review required). The test job fails at "Check packaged artifact budget" because node_modules/marked@18.0.11 was added to the packaged closure. This PR changes no dependencies, and the latest main run fails the same way, so the failure is not caused by this PR. label passes.
| if (!event.defaultPrevented && nextPrompt.text && !compositionActiveRef.current | ||
| && event.currentTarget.getAttribute('aria-expanded') !== 'true') { | ||
| if (event.key === 'Tab' && !event.shiftKey && !event.altKey && !event.ctrlKey && !event.metaKey) { | ||
| if ((event.key === 'ArrowRight' || event.key === 'Tab') |
There was a problem hiding this comment.
P3 (optional, also applies to the existing Tab path): preventDefault() runs before acceptNextPrompt() re-checks textPort.getValue().length and composition. If those guards bail, the arrow is swallowed with no effect. Having acceptNextPrompt() return whether it accepted, and calling preventDefault() only in that case, would keep native caret movement as the fallback. I did not reproduce this; it is a narrow DOM/state race at most.
There was a problem hiding this comment.
Addressed in 7ab620e.
acceptNextPrompt() now returns whether it accepted the suggestion, and the shortcut handler calls preventDefault() only on success. If the live draft has already changed, Right Arrow and Tab keep their native behavior.
Added deterministic component regressions for both keys: update the live draft through the production imperative handle and dispatch the shortcut before React commits the render that retires the offer. Both tests fail on the previous code because the event is consumed, and pass with this fix while preserving the arriving draft and sending nothing.
Validation: 723 shared UI tests passed; the built WorkHub Storybook scenario passed in light/dark themes at 720px and 1280px, including acceptance, caret placement, Undo, and explicit send. Full build, lint, formatting, typecheck, and Knip for Desktop/UI also passed.
For the earlier CI failure, I checked both the PR job and the main job: both fail Check packaged artifact budget on node_modules/marked@18.0.11. This PR changes neither dependencies nor the packaging ledger; that upstream failure remains separate from this fix.
Posted by OpenAI Codex on behalf of MasamiYui.
Refs apache#6131 Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.Incremental review:
2c768ffb..7ab620e6, one commit (+22/-3). Our optional P3 is fixed, and I found no P0–P2 issue.
acceptNextPrompt now returns whether it inserted the suggestion, and onInputKeyDown calls preventDefault() only when it did. If the live draft has text, IME composition is active, or there is no offer, Right Arrow and Tab keep their native behavior. The Escape and modifier paths are unchanged. The new test covers the race where the draft changes before React removes the old offer, for both keys. It asserts the event is not prevented, the draft is kept, and nothing is sent.
CI test fails at check:artifact-budget (marked@18.0.11). Main fails the same way, so this PR did not cause it; #6129 refreshes that ledger.
Summary
Accept an existing next-prompt suggestion with an unmodified Right Arrow, filling the empty draft without sending it. Keep Tab acceptance, Escape dismissal, IME/menu priority, and normal modified-arrow behavior. Show the localized Right Arrow / Tab hint in Simplified Chinese, Traditional Chinese, and English. Consume these keys only when a suggestion is actually accepted, preserving native behavior if a draft arrives before the offer disappears.
Fixes #6131
Verification
npm run lint,npm run format:check,npm run build, andnpm run typecheck.apps/desktopandpackages/ui, locale hygiene, andgit diff --check.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex implemented the change, added tests, ran validation, and prepared this PR at MasamiYui's request. The commit includes a
Generated-by: OpenAI Codextrailer.Checklist
Does this PR entail a change in behavior?