Keep IME candidate confirmation from sending or stopping chat - #1038
Keep IME candidate confirmation from sending or stopping chat#1038PeterDaveHello wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesInput action handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoPrevent IME Enter confirmation from sending/stopping chat
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Greptile SummaryThe PR prevents Enter from triggering chat actions while an IME composition is active, including the legacy key-code fallback.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/components/InputBox/index.jsx | Replaces the inline Enter-key condition with the extracted IME-aware action predicate. |
| src/components/InputBox/input-action.mjs | Defines deterministic classification for click, Enter, Shift+Enter, and active-composition events. |
| tests/unit/components/input-box-action.test.mjs | Covers supported chat actions and the IME conditions that must suppress them. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
E[Input event] --> C{Click?}
C -->|Yes| A[Handle chat action]
C -->|No| K{Keydown?}
K -->|No| I[Ignore]
K -->|Yes| M{Composing or keyCode 229?}
M -->|Yes| I
M -->|No| N{Enter without Shift?}
N -->|Yes| A
N -->|No| I
Reviews (2): Last reviewed commit: "Keep IME candidate confirmation from sen..." | Re-trigger Greptile
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/components/input-box-action.test.mjs (1)
5-10: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the modern and legacy Enter paths separately.
The current fixture sets both
key: 'Enter'andkeyCode: 13. The test passes if either detection branch is removed. Add one key-only case and one keyCode-only case.Proposed test adjustment
assert.equal( - shouldHandleInputAction({ type: 'keydown', key: 'Enter', keyCode: 13, shiftKey: false }), + shouldHandleInputAction({ type: 'keydown', key: 'Enter', shiftKey: false }), true, ) + assert.equal( + shouldHandleInputAction({ type: 'keydown', keyCode: 13, shiftKey: false }), + true, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/components/input-box-action.test.mjs` around lines 5 - 10, The test “input actions handle button clicks and plain Enter keys” must isolate both Enter detection branches. Keep the click assertion, then add separate keydown cases: one with only key set to “Enter” and one with only keyCode set to 13, with the other Enter field omitted, and assert both are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/InputBox/input-action.mjs`:
- Line 4: Update shouldHandleInputAction to read the IME composition state from
event.nativeEvent?.isComposing while retaining the keyCode === 229 guard, so
composing Enter events are ignored. Add a test invoking shouldHandleInputAction
with a native event object whose isComposing is true.
---
Nitpick comments:
In `@tests/unit/components/input-box-action.test.mjs`:
- Around line 5-10: The test “input actions handle button clicks and plain Enter
keys” must isolate both Enter detection branches. Keep the click assertion, then
add separate keydown cases: one with only key set to “Enter” and one with only
keyCode set to 13, with the other Enter field omitted, and assert both are
handled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a96a3f3c-55a4-458c-a797-43095c21db96
📒 Files selected for processing (3)
src/components/InputBox/index.jsxsrc/components/InputBox/input-action.mjstests/unit/components/input-box-action.test.mjs
CJK input methods use Enter to confirm candidate text. Treating that keydown as an input action can submit incomplete text or stop an active response. Read composition state from both direct and synthetic native events, retain the legacy keyCode 229 fallback, and test each Enter path alone.
c117c3a to
a3f3274
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an IME (CJK) usability bug in the InputBox: confirming an IME candidate with Enter during active composition no longer triggers the extension’s “send/stop” action.
Changes:
- Introduces a shared
shouldHandleInputActionhelper to gate click/Enter handling and ignore Enter during IME composition (including legacykeyCode === 229). - Updates
InputBoxto use the helper instead of inline Enter/click logic. - Adds focused unit tests to prevent regressions across
key,keyCode, Shift+Enter, composition flags, and unrelated keys.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/components/input-box-action.test.mjs | Adds unit coverage for the new input-action gating logic (Enter, Shift+Enter, IME composition, legacy 229). |
| src/components/InputBox/input-action.mjs | Implements shouldHandleInputAction to ignore IME composition Enter keydowns while preserving click and plain Enter behavior. |
| src/components/InputBox/index.jsx | Replaces inline click/Enter handling with shouldHandleInputAction to prevent unintended send/stop during IME composition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Problem
CJK input methods commonly use Enter to confirm candidate text while composition is still active. The input box treated every unshifted Enter as a chat action, which could send incomplete text or stop an active response instead of confirming the IME candidate.
With Preact compat, composition state may be exposed on the synthetic event or its underlying native keyboard event. Some browser and IME combinations also use the legacy
keyCodevalue 229.Changes
keyCode === 229IME fallback.keyandkeyCodeEnter paths independently so either path cannot regress unnoticed.Validation
node --test tests/unit/components/input-box-action.test.mjs— 6 tests passed in an isolated source mirror.node --checkpassed for the changed JavaScript modules.npm test,npm run test:coverage,npm run lint,npm run build, artifact checks, and native Chromium/Firefox IME smoke tests. This execution environment could not obtain a complete repository checkout or install its dependencies, and the repository'spr-testsworkflow did not start automatically for this connector-created PR.