[codex] Fix terminal IME composition handling#3331
Open
dandacompany wants to merge 6 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR propagates IME/composition state into keyboard adapters and makes global, terminal, and TermWrap handlers consume composing events early. It adds a DefaultTermFontFamily constant and default config key Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
|
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/util/keyutil.ts (1)
243-243: ⚡ Quick winAdd
isComposingtoWaveKeyboardEventto removeanycasts in IME handling
frontend/types/gotypes.d.ts’sWaveKeyboardEventlacksisComposing, butfrontend/util/keyutil.tssets it via(rtn as any).isComposing(lines ~243 and ~272) andfrontend/app/store/keymodel.tsreads it via(waveEvent as any).isComposing(line ~420). AddisComposing?: booleanto the sharedWaveKeyboardEventtype so these casts can be removed.🤖 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 `@frontend/util/keyutil.ts` at line 243, The WaveKeyboardEvent type is missing isComposing which forces unsafe any casts in keyutil.ts and keymodel.ts; add an optional boolean property isComposing?: boolean to the WaveKeyboardEvent declaration in frontend/types/gotypes.d.ts, then update usages in frontend/util/keyutil.ts (where (rtn as any).isComposing is set around the IME handling lines) and frontend/app/store/keymodel.ts (where (waveEvent as any).isComposing is read) to remove the any casts and use the typed property directly.
🤖 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.
Nitpick comments:
In `@frontend/util/keyutil.ts`:
- Line 243: The WaveKeyboardEvent type is missing isComposing which forces
unsafe any casts in keyutil.ts and keymodel.ts; add an optional boolean property
isComposing?: boolean to the WaveKeyboardEvent declaration in
frontend/types/gotypes.d.ts, then update usages in frontend/util/keyutil.ts
(where (rtn as any).isComposing is set around the IME handling lines) and
frontend/app/store/keymodel.ts (where (waveEvent as any).isComposing is read) to
remove the any casts and use the typed property directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 013d572c-1605-4183-a9fd-5d35fbd62e80
📒 Files selected for processing (7)
frontend/app/store/keymodel.tsfrontend/app/view/term/ijson.tsxfrontend/app/view/term/term-model.tsfrontend/app/view/term/term.tsxfrontend/app/view/term/termwrap.tsfrontend/util/keyutil.tspkg/wconfig/defaultconfig/settings.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/app/view/term/term.tsx`:
- Line 299: The code currently sets termDisableWebGl via
termSettings?.["term:disablewebgl"] ?? true which makes termDisableWebGl true
(WebGL disabled) by default and useWebGl is computed as !termDisableWebGl; this
conflicts with the docs that state the default is false; update the
documentation to reflect the actual default (term:disablewebgl defaults to true)
or change the default in code/config to false to match the docs, and add a short
migration/release note documenting the default behavior change for the setting
(referencing the termDisableWebGl variable and the useWebGl computation) so
users are informed of the change.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9f41b37e-e7dc-43b7-aae1-e85de3f15e5d
📒 Files selected for processing (2)
frontend/app/view/term/term.tsxpkg/wconfig/defaultconfig/settings.json
| const termMacOptionIsMeta = globalStore.get(termMacOptionIsMetaAtom) ?? false; | ||
| const termCursorStyle = normalizeCursorStyle(globalStore.get(getOverrideConfigAtom(blockId, "term:cursor"))); | ||
| const termCursorBlink = globalStore.get(getOverrideConfigAtom(blockId, "term:cursorblink")) ?? false; | ||
| const termDisableWebGl = termSettings?.["term:disablewebgl"] ?? true; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about WebGL default change
# Check for WebGL-related documentation
rg -n -C3 --type=md 'webgl|WebGL' | head -50
# Check for any TODO/FIXME about WebGL defaults
rg -n 'TODO|FIXME' --iglob '*webgl*' --iglob '*term*'
# Look for changelog or migration guide entries
fd -e md -e txt | xargs rg -l 'changelog|CHANGELOG|migration|breaking'Repository: wavetermdev/waveterm
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the toggle logic around the referenced lines
sed -n '250,360p' frontend/app/view/term/term.tsx | nl -ba | sed -n '40,140p'
# 2) Search for the setting key and related strings
rg -n --hidden --no-ignore -S 'term:disablewebgl|disablewebgl|Default terminal rendering to non-WebGL|non-WebGL|useWebGl' frontend/app | head -200
# 3) Search docs/releases/changelog/migration/breaking notes
rg -n -S --hidden --no-ignore '(WebGL|webgl|non-WebGL|nonwebgl|disablewebgl|term:disablewebgl|breaking|migration|changelog|RELEASES\.md|Release note|release notes)' \
. | head -200
# 4) Specifically inspect RELEASES.md for any WebGL-related entry
if [ -f RELEASES.md ]; then
rg -n -S 'WebGL|webgl|non-WebGL|disablewebgl|term:disablewebgl|breaking|migration' RELEASES.md | head -200 || true
# show nearby sections if any match lines
MATCH_LINES=$(rg -n -S 'WebGL|webgl|non-WebGL|disablewebgl|term:disablewebgl|breaking|migration' RELEASES.md | cut -d: -f1 | head -20 | tr '\n' ' ')
if [ -n "${MATCH_LINES:-}" ]; then
for ln in $MATCH_LINES; do
echo "---- RELEASES.md around line $ln ----"
sed -n "$((ln-20)),$((ln+20))p" RELEASES.md
done
fi
else
echo "RELEASES.md not found at repo root."
fiRepository: wavetermdev/waveterm
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect toggle logic around referenced lines (with line numbers)
sed -n '260,340p' frontend/app/view/term/term.tsx | cat -n | sed -n '1,120p'
# 2) Search for the setting key and related strings (scoped to term code + docs)
rg -n -S --hidden --no-ignore 'term:disablewebgl|disablewebgl|Default terminal rendering to non-WebGL|non-WebGL|useWebGl|useWebGL' frontend/app . | head -200
# 3) Search docs/releases/changelog/migration/breaking notes
rg -n -S --hidden --no-ignore '(WebGL|webgl|non-WebGL|nonwebgl|disablewebgl|term:disablewebgl|breaking|migration|changelog|RELEASES\.md)' . | head -200
# 4) Specifically inspect RELEASES.md
if [ -f RELEASES.md ]; then
echo "---- RELEASES.md matches ----"
rg -n -S 'WebGL|webgl|non-WebGL|nonwebgl|disablewebgl|term:disablewebgl|breaking|migration|non-WebGL' RELEASES.md | head -200 || true
else
echo "RELEASES.md not found at repo root."
fiRepository: wavetermdev/waveterm
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show termdisablewebgl docs/defaults/schema with tight context (exclude node_modules)
rg -n -S 'term:disablewebgl' pkg/wconfig/defaultconfig/settings.json schema/settings.json docs/docs/config.mdx frontend/types/gotypes.d.ts frontend/app/view/term/termwrap.ts frontend/app/view/term/term.tsx
echo "---- docs/docs/config.mdx ----"
sed -n '40,90p' docs/docs/config.mdx
echo "---- pkg/wconfig/defaultconfig/settings.json ----"
sed -n '1,80p' pkg/wconfig/defaultconfig/settings.json
echo "---- schema/settings.json (term:disablewebgl section) ----"
python3 - <<'PY'
import json
from pathlib import Path
p = Path("schema/settings.json")
data = json.loads(p.read_text())
# brute-force search for key in nested structures
def find_key(obj, key):
if isinstance(obj, dict):
for k,v in obj.items():
if k == key:
return v
r = find_key(v, key)
if r is not None:
return r
elif isinstance(obj, list):
for it in obj:
r = find_key(it, key)
if r is not None:
return r
return None
v = find_key(data, "term:disablewebgl")
print(v)
PY
echo "---- frontend/app/view/term/termwrap.ts (renderer selection) ----"
sed -n '140,230p' frontend/app/view/term/termwrap.ts
# Search RELEASES.md for WebGL/non-WebGL without blowing up output (exclude node_modules)
if [ -f RELEASES.md ]; then
rg -n -S 'WebGL|webgl|non-WebGL|nonwebgl|disablewebgl|term:disablewebgl' RELEASES.md | head -200 || true
fi
# Also search docs/releases folder if exists (avoid node_modules)
if [ -d docs ]; then
rg -n -S '(WebGL|webgl|non-WebGL|nonwebgl|disablewebgl|term:disablewebgl)' docs | head -200 || true
fiRepository: wavetermdev/waveterm
Length of output: 23101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for release/note mentions (exclude node_modules)
for f in RELEASES.md docs/RELEASES.md CHANGELOG.md docs/CHANGELOG.md; do
if [ -f "$f" ]; then
echo "---- $f ----"
rg -n -S 'non-WebGL|nonwebgl|WebGL|webgl|term:disablewebgl|disablewebgl|disable WebGL' "$f" || true
fi
done
# Search docs + repo for the specific phrase the review comment mentioned
rg -n -S --hidden --no-ignore --glob '!**/node_modules/**' \
'Default terminal rendering to non-WebGL|non-WebGL|term:disablewebgl|disablewebgl|useWebGl' . | head -200
# Check config default description inconsistency
echo "---- docs/docs/config.mdx line with term:disablewebgl ----"
rg -n -S 'term:disablewebgl' docs/docs/config.mdxRepository: wavetermdev/waveterm
Length of output: 1621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "DefaultTermFontFamily" frontend/app/view/term/term.tsxRepository: wavetermdev/waveterm
Length of output: 207
Fix WebGL default inconsistency for term:disablewebgl (and document it)
frontend/app/view/term/term.tsx(299):termDisableWebGl = termSettings?.["term:disablewebgl"] ?? true=> WebGL is disabled when the setting is unset; (323)useWebGl: !termDisableWebGl=> defaults tofalse.pkg/wconfig/defaultconfig/settings.json(35) also sets"term:disablewebgl": true.docs/docs/config.mdx(63) statesterm:disablewebgldefault is false, which conflicts with the code/config defaults above.- No release/changelog/migration note mentioning
term:disablewebgl/ WebGL (or “non-WebGL”) was found inRELEASES.md/ docs release files.
Update the docs (and/or add release/migration notes if this default changed for existing users).
🤖 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 `@frontend/app/view/term/term.tsx` at line 299, The code currently sets
termDisableWebGl via termSettings?.["term:disablewebgl"] ?? true which makes
termDisableWebGl true (WebGL disabled) by default and useWebGl is computed as
!termDisableWebGl; this conflicts with the docs that state the default is false;
update the documentation to reflect the actual default (term:disablewebgl
defaults to true) or change the default in code/config to false to match the
docs, and add a short migration/release note documenting the default behavior
change for the setting (referencing the termDisableWebGl variable and the
useWebGl computation) so users are informed of the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@frontend/app/view/term/termwrap.ts`:
- Around line 521-531: The flushPendingCompositionSpace path
(flushPendingCompositionSpace) and the onData handler in initTerminal are
dropping user keystrokes because they return when this.loaded is false; change
the logic to buffer or queue incoming data instead of discarding it: when
pendingCompositionSpace exists or onData fires and this.loaded is false (and not
disposed), append the bytes to a short-lived buffer (e.g.,
this._startupInputBuffer) and avoid clearing them; once loaded becomes true
(after bootstrap completes) flush that buffer by calling sendTermData for each
queued chunk (or a single concatenated string), and ensure disposed still
prevents queuing/flush. Reference functions/properties:
flushPendingCompositionSpace, initTerminal/onData subscription,
pendingCompositionSpace, loaded, disposed, and sendTermData.
- Around line 541-548: The regex checks in isLikelyCompositionText are
triggering the no-control-regex lint rule; replace the control-character and
non-ASCII regex tests with explicit code-point scans over the input string
(data) instead: iterate through characters (e.g., for...of or index loop using
charCodeAt/codePointAt) to return false if any code point is in the C0/C1
control ranges (0x00–0x1F or 0x7F) or if the string is empty, and return true if
any code point is outside the ASCII range (> 0x7F); keep the same early-return
behavior and preserve the function name isLikelyCompositionText and its input
parameter data.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da994121-ce8d-4335-b0b3-a7e88d08c23f
📒 Files selected for processing (1)
frontend/app/view/term/termwrap.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/app/view/term/termwrap.ts`:
- Around line 552-560: Replace the control-character regex in
isCompositionSuffixData with an explicit code-point check like you did in
isLikelyCompositionText: iterate the string (for...of or Array.from) and return
false if any codePoint <= 0x1F or codePoint === 0x7F, keep the initial empty
check, and retain the final printable-range test (or replace it with an explicit
check that all codePoints are between 0x20 and 0x7E); update the function
isCompositionSuffixData accordingly so there are no control-regex uses.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bfe39f3a-8dbd-4316-aa92-833fcb36d927
📒 Files selected for processing (1)
frontend/app/view/term/termwrap.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.fixed-fontCSS declaration to use a validfont:property.Why
IME composition key events can flow through Wave keyboard handling and terminal custom key handling. In interactive terminal prompts, this can cause unexpected input while composing text with Korean, Japanese, Chinese, or other composition-based input methods.
The terminal font fallback also previously relied only on Hack/monospace. Keeping Hack first preserves the existing monospace behavior, while adding Noto CJK and common platform CJK fallbacks improves glyph coverage for multilingual terminal output.
Verification
npm run build:devin a clean worktree from this commitfs/path, circular chunk warning for cytoscape/mermaidsharppackage, but the build command still completed successfully