Skip to content

Fix IME issues#2938

Merged
sawka merged 3 commits intomainfrom
sawka/ime
Feb 26, 2026
Merged

Fix IME issues#2938
sawka merged 3 commits intomainfrom
sawka/ime

Conversation

@sawka
Copy link
Member

@sawka sawka commented Feb 25, 2026

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 25, 2026

Walkthrough

This change modifies the IME (Input Method Editor) event handling in the terminal wrapper component. It replaces a blanket suppression approach for composition events with a time-based deduplication window of 20 milliseconds. The modification introduces conditional logic to suppress data during composition only when it occurs outside this window, extends the composition state reset to clear additional tracking fields, wraps the keydown handler with composition bypass logic, and adds logging for suppressed IME events. The public API signatures remain unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning No pull request description was provided by the author, making it impossible to assess whether the description relates to the changeset. Add a pull request description explaining the IME issues being fixed, the approach taken, and any relevant context for reviewers.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing IME (Input Method Editor) issues, which is the core focus of all modifications in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sawka/ime

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kilo-code-bot
Copy link
Contributor

kilo-code-bot bot commented Feb 25, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)
  • frontend/app/view/term/termwrap.ts - IME composition handling refactored and simplified

Review Notes

This PR refactors the IME (Input Method Editor) composition handling logic:

Changes:

  • Extracted IMEDedupWindowMs constant (20ms) for better maintainability
  • Simplified handleTermData() IME logic from ~30 lines to ~10 lines
  • Updated resetCompositionState() to reset all composition-related fields
  • Added isComposing check in attachCustomKeyEventHandler to prevent key events during composition

Logic:
The new approach suppresses data events when isComposing=true unless they occur within 20ms of a compositionend event. This handles the edge case where CapsLock input method switching causes spurious isComposing=true events immediately after composition ends.

Verification:

  • The logic is sound and correctly handles the composition lifecycle
  • State management is properly reset in resetCompositionState()
  • The refactoring maintains the same behavior while improving code clarity

No bugs or issues detected.

@sawka
Copy link
Member Author

sawka commented Feb 25, 2026

addresses #2915 and #2523

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 366-373: The dlog call inside the IME suppression branch is
logging raw IME payloads via data; change it to avoid exposing user text by
logging only metadata (e.g., data length, timeSinceCompositionEnd,
this.isComposing state, and IMEDedupWindowMs) or a masked representation instead
of the verbatim data. Update the logging call in the block where isComposing is
checked (referencing isComposing, lastCompositionEnd, IMEDedupWindowMs, and the
dlog invocation) to include those safe fields and remove the raw data value.

ℹ️ Review info

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ba7aea7 and a419cc8.

📒 Files selected for processing (1)
  • frontend/app/view/term/termwrap.ts

Comment on lines +366 to 373
// IME fix: suppress isComposing=true events unless they immediately follow
// a compositionend (within 20ms). This handles CapsLock input method switching
// where the composition buffer gets flushed as a spurious isComposing=true event
if (this.isComposing) {
dlog("Blocked data during composition:", data);
return;
}

// IME Deduplication (for Capslock input method switching)
// When switching input methods with Capslock during composition, some systems send the
// composed text twice. We allow the first send and block subsequent duplicates.
const IMEDedupWindowMs = 50;
const now = Date.now();
const timeSinceCompositionEnd = now - this.lastCompositionEnd;
if (timeSinceCompositionEnd < IMEDedupWindowMs && data === this.lastComposedText && this.lastComposedText) {
if (!this.firstDataAfterCompositionSent) {
// First send after composition - allow it but mark as sent
this.firstDataAfterCompositionSent = true;
dlog("First data after composition, allowing:", data);
} else {
// Second send of the same data - this is a duplicate from Capslock switching, block it
dlog("Blocked duplicate IME data:", data);
this.lastComposedText = ""; // Clear to allow same text to be typed again later
this.firstDataAfterCompositionSent = false;
const timeSinceCompositionEnd = Date.now() - this.lastCompositionEnd;
if (timeSinceCompositionEnd > IMEDedupWindowMs) {
dlog("Suppressed IME data (composing, not near compositionend):", data);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid logging raw IME input payloads.

Line 372 logs user-typed text verbatim. Even under debug logging, this can expose sensitive data/PII. Log metadata only (length/timing/state).

🔒 Suggested safe logging change
-                dlog("Suppressed IME data (composing, not near compositionend):", data);
+                dlog("Suppressed IME data (composing, not near compositionend)", {
+                    length: data.length,
+                    timeSinceCompositionEnd,
+                });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/term/termwrap.ts` around lines 366 - 373, The dlog call
inside the IME suppression branch is logging raw IME payloads via data; change
it to avoid exposing user text by logging only metadata (e.g., data length,
timeSinceCompositionEnd, this.isComposing state, and IMEDedupWindowMs) or a
masked representation instead of the verbatim data. Update the logging call in
the block where isComposing is checked (referencing isComposing,
lastCompositionEnd, IMEDedupWindowMs, and the dlog invocation) to include those
safe fields and remove the raw data value.

@sawka sawka merged commit 296760d into main Feb 26, 2026
8 checks passed
@sawka sawka deleted the sawka/ime branch February 26, 2026 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant