Repository navigation
Show a preedit the IME resumes without a compositionstart - #13284
Conversation
📝 WalkthroughWalkthroughAdded Vitest and happy-dom regression tests for xterm IME preedit visibility. The tests replay synthetic and captured composition events, including resumptions without a second 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b85ba4a-b886-46cc-9723-4109489c3d77
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
config/patches/@xterm__xterm@6.1.0-beta.287.patchsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-resumed-preedit-visibility.test.ts
| it('shows a preedit the IME resumes with an update and no second compositionstart', async () => { | ||
| const rig = openTerminal() | ||
|
|
||
| await rig.replay(RESUMED_WITHOUT_START) | ||
|
|
||
| expect(displayedPreedit(rig.compositionView)).toEqual({ preedit: '네', shown: true }) | ||
| }) | ||
|
|
||
| it('shows a preedit the IME resumes after the Enter that committed the last syllable', async () => { | ||
| const rig = openTerminal() | ||
|
|
||
| await rig.replay(RESUMED_ACROSS_ENTER) | ||
|
|
||
| expect(displayedPreedit(rig.compositionView)).toEqual({ preedit: '제', shown: true }) | ||
| }) | ||
|
|
||
| it('still shows a preedit the IME reopens with a compositionstart', async () => { | ||
| const rig = openTerminal() | ||
|
|
||
| await rig.replay(REOPENED_WITH_START) | ||
|
|
||
| expect(displayedPreedit(rig.compositionView)).toEqual({ preedit: '글', shown: true }) | ||
| }) | ||
|
|
||
| it('hides the overlay once the composition ends and nothing resumes it', async () => { | ||
| const rig = openTerminal() | ||
|
|
||
| await rig.replay([ | ||
| { type: 'compositionstart', data: '' }, | ||
| { type: 'compositionupdate', data: '한' }, | ||
| { type: 'compositionend', data: '한' } | ||
| ]) | ||
|
|
||
| expect(displayedPreedit(rig.compositionView).shown).toBe(false) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required ordinary-input control and mutation check.
REOPENED_WITH_START is still an IME sequence. It is not a non-IME negative control.
Add an ordinary input sequence that proves normal input is unchanged. Add a mutation check that disables the .active restoration. The resumed-preedit test must then fail while the ordinary-input control still passes.
As per coding guidelines, “Pair the IME regression test with a genuine negative control proving ordinary non-IME input is unchanged” and “Include a mutation check.”
Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/terminal-ime-xterm-windows-resumed-preedit-trace.test.ts (1)
2-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the file header comment.
Keep one concise comment that states why this trace exists. The current block duplicates details that the test name and fixture already show.
As per coding guidelines, comments must be concise, limited to non-obvious information, and preferably one line.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 354672da-006a-4e55-9520-70846c112fb4
📒 Files selected for processing (2)
src/renderer/src/components/terminal-pane/__fixtures__/windows-wsl-hangul-resumed-preedit-trace.jsonsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-windows-resumed-preedit-trace.test.ts
82fc178 to
c1d9425
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/terminal-ime-xterm-windows-resumed-preedit-trace.test.ts (1)
28-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the replay harness into a shared module.
nextEventLoop,buildEvent, and the canvas stub at Lines 62-64 duplicate the harness interminal-ime-xterm-resumed-preedit-visibility.test.ts. Both files belong to the same cohort, and the stack lists this layer as dependent on the harness layer. A drift in one copy will silently weaken one of the two tests.Move the shared pieces into a dedicated module, for example
terminal-ime-replay-harness.ts, and import it from both tests. Do not name the modulehelpers,utils, orshared.As per coding guidelines: "Do not use vague names such as
helpers,utils,common,misc, orshared-stufffor files, folders, or modules; use concrete domain names instead."Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 731f9e76-3141-4e5d-a716-02a430ebc6a2
📒 Files selected for processing (2)
src/renderer/src/components/terminal-pane/__fixtures__/windows-wsl-hangul-resumed-preedit-trace.jsonsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-windows-resumed-preedit-trace.test.ts
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| document.body.replaceChildren() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Drain pending composition timers before you dispose the terminal.
afterEach is synchronous, and terminal.dispose() runs inline at Line 112 with no preceding await. xterm re-arms updateCompositionElements on a setTimeout(0) while a composition is open. A callback that fires after dispose touches detached DOM and can surface as an unhandled error, which makes the run flaky.
The companion test terminal-ime-xterm-resumed-preedit-visibility.test.ts already guards this case. It awaits nextEventLoop() twice before dispose. Apply the same guard here.
🛡️ Proposed fix: move dispose into an async afterEach
+const openTerminals: Terminal[] = []
+
describe('Windows/WSL Korean — recorded composition updates keep the preedit visible', () => {
beforeEach(() => {
vi.spyOn(HTMLCanvasElement.prototype, 'getContext').mockReturnValue({
measureText: () => ({ width: 10 })
} as unknown as CanvasRenderingContext2D)
})
- afterEach(() => {
+ afterEach(async () => {
+ // updateCompositionElements re-arms on a timer; let the pending one run before dispose.
+ await nextEventLoop()
+ await nextEventLoop()
+ while (openTerminals.length > 0) {
+ openTerminals.pop()?.dispose()
+ }
vi.restoreAllMocks()
document.body.replaceChildren()
})Then register the terminal after terminal.open(container) and drop the inline terminal.dispose() at Line 112:
const terminal = new Terminal()
terminal.open(container)
+ openTerminals.push(terminal)- terminal.dispose()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(() => { | |
| vi.restoreAllMocks() | |
| document.body.replaceChildren() | |
| }) | |
| afterEach(async () => { | |
| // updateCompositionElements re-arms on a timer; let the pending one run before dispose. | |
| await nextEventLoop() | |
| await nextEventLoop() | |
| while (openTerminals.length > 0) { | |
| openTerminals.pop()?.dispose() | |
| } | |
| vi.restoreAllMocks() | |
| document.body.replaceChildren() | |
| }) |
Typing 2-Set Korean shows committed syllables but not the in-progress jamo, so the user composes each syllable blind. Long-standing hole in the vendored terminal library, not a regression: the same test fails identically against the bundle this branch starts from. The `.active` class that CSS keys `display: block` off is added only in `compositionstart` and dropped in `_finalizeComposition`. Some IMEs (observed on Windows/WSL Korean) resume a composition with a bare `compositionupdate` and no second `compositionstart`, by which point `compositionend` has already hidden the overlay, so the resumed preedit is written into a hidden element and never positioned. `updateCompositionElements` also early-returned on `!_isComposing`, so it would not lay the overlay out either. Re-show the overlay on an update that carries data, and key the layout guard on the shown overlay instead. `_isComposing` is deliberately left alone, so no commit bookkeeping changes and `onData` stays byte-identical. The two guards are equivalent on every pre-existing path: `compositionstart` sets both, `_finalizeComposition` clears both. The bundle hunks are the same two edits applied to the shipped minified output; the sourcemaps are carried through unchanged.
…ows capture The synthetic test pins the shape; this replays events a real Microsoft Korean IME emitted on Windows/WSL. The capture holds three compositionupdates that resume a composition with no second compositionstart — the exact ordering that wrote the preedit into a hidden overlay. Without the fix all three report shown:false; with it all three are visible. Fixture derived from the sealed 11919-windows-wsl-current capture, which is read-only and unmodified. Co-authored-by: Orca <help@stably.ai>
… artifact The capture logs each event twice — a dispatch record and a batched next-frame re-log. Deriving from both replayed every event twice, which made three compositionupdates appear to land after a session had ended. Filtered to dispatch records the capture holds zero resumes and 11 balanced sessions, so the previous toHaveLength(3) was pinning an artifact of the derivation. Re-scoped to what the capture does prove: the preedit stays visible across all 37 real updates. Verified by reverting the patch that this passes either way, so it is coverage and the synthetic test remains the discriminator. Both facts are now stated in the file. Co-authored-by: Orca <help@stably.ai>
The previous commit accidentally reverted it: checking main's patch and lockfile into the worktree to test whether a test discriminates also stages them, so the commit that followed swept them up. Co-authored-by: Orca <help@stably.ai>
cd73ded to
5c07f0b
Compare
…3284) * fix(terminal): show a preedit the IME resumes without a compositionstart Typing 2-Set Korean shows committed syllables but not the in-progress jamo, so the user composes each syllable blind. Long-standing hole in the vendored terminal library, not a regression: the same test fails identically against the bundle this branch starts from. The `.active` class that CSS keys `display: block` off is added only in `compositionstart` and dropped in `_finalizeComposition`. Some IMEs (observed on Windows/WSL Korean) resume a composition with a bare `compositionupdate` and no second `compositionstart`, by which point `compositionend` has already hidden the overlay, so the resumed preedit is written into a hidden element and never positioned. `updateCompositionElements` also early-returned on `!_isComposing`, so it would not lay the overlay out either. Re-show the overlay on an update that carries data, and key the layout guard on the shown overlay instead. `_isComposing` is deliberately left alone, so no commit bookkeeping changes and `onData` stays byte-identical. The two guards are equivalent on every pre-existing path: `compositionstart` sets both, `_finalizeComposition` clears both. The bundle hunks are the same two edits applied to the shipped minified output; the sourcemaps are carried through unchanged. * test(terminal): prove the resumed-preedit fix against a recorded Windows capture The synthetic test pins the shape; this replays events a real Microsoft Korean IME emitted on Windows/WSL. The capture holds three compositionupdates that resume a composition with no second compositionstart — the exact ordering that wrote the preedit into a hidden overlay. Without the fix all three report shown:false; with it all three are visible. Fixture derived from the sealed 11919-windows-wsl-current capture, which is read-only and unmodified. Co-authored-by: Orca <help@stably.ai> * test(terminal): stop the recorded Hangul fixture pinning a derivation artifact The capture logs each event twice — a dispatch record and a batched next-frame re-log. Deriving from both replayed every event twice, which made three compositionupdates appear to land after a session had ended. Filtered to dispatch records the capture holds zero resumes and 11 balanced sessions, so the previous toHaveLength(3) was pinning an artifact of the derivation. Re-scoped to what the capture does prove: the preedit stays visible across all 37 real updates. Verified by reverting the patch that this passes either way, so it is coverage and the synthetic test remains the discriminator. Both facts are now stated in the file. Co-authored-by: Orca <help@stably.ai> * fix(terminal): restore the preedit visibility patch onto its own branch The previous commit accidentally reverted it: checking main's patch and lockfile into the worktree to test whether a test discriminates also stages them, so the commit that followed swept them up. Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
The bug
Typing 2-Set Korean into a terminal pane shows committed syllables but not the in-progress jamo. The user composes each syllable blind and only sees it once it commits.
This is a pre-existing limitation, not a regression
The added test fails identically against the bundle this branch starts from — i.e. against the restored patch that
#13282reverted to, with none of the reverted composition work present. Reverting the fix on this branch and reinstalling reproduces the same two failures. This hole predates the composition-ownership change and survived its revert.Mechanism
The
.activeclass is what the stylesheet keysdisplay: blockoff. In the vendored terminal library's composition helper:compositionstartsets_isComposing = trueand adds.active._finalizeCompositionclears_isComposingand removes.active.compositionupdatewritestextContentand callsupdateCompositionElements(), which early-returned on!this._isComposing.Some IMEs — observed in a recorded Windows/WSL Korean capture — resume a composition with a bare
compositionupdateand no secondcompositionstart. By that pointcompositionendhas already hidden the overlay. The resumed preedit is therefore written into an element that is stilldisplay: none, andupdateCompositionElementsdeclines to lay it out. The commit path is untouched, which is exactly why the committed text lands and the composing glyph does not.The fix
Two edits in the vendored helper:
compositionupdate, re-add.activewhen the update carries data.updateCompositionElements, guard on the shown overlay (classList.contains('active')) rather than_isComposing._isComposingis deliberately not set. No commit bookkeeping changes —_pendingCompositionStart,_compositionPosition, the deferred send, the transaction id — andonDatais byte-identical. No timers, noevent.key.The two guards are equivalent on every pre-existing path, because the only writers set and clear them together:
compositionstartsets both,_finalizeCompositionclears both. The new resumed-update path is the only place they diverge.Discrimination was proved, not argued
Not by reasoning about the guards — by an actual revert-and-restore cycle:
git checkout origin/main -- config/patches/@xterm__xterm@6.1.0-beta.287.patch pnpm-lock.yaml,pnpm install, confirmed the bundle no longer carries the guard, ran the test:pnpm install, confirmed the guard is back in the installed bundle, re-ran:Tests 4 passed (4).The suite has 4 arms. Two fail without the fix; two are controls that pass either way — a composition reopened with a
compositionstartmust keep working, and the overlay must still hide when a composition ends and nothing resumes it. Those controls are what stop the suite from degenerating into "always visible".The test asserts the displayed overlay (the
.activeclass plus its text). AssertingonDatahere would pass on the broken build, because the commit path is not what breaks.Note on the patch file
The src-patch toolchain and its regeneration script were removed by
#13282, soconfig/patches/@xterm__xterm@6.1.0-beta.287.patchis once again the only artifact. It was regenerated withpnpm patch/pnpm patch-commit: the same two edits were applied to the hand-authored source and to the shipped minifiedlib/xterm.jsandlib/xterm.mjs, verified as unique single-site replacements in each. The sourcemaps are carried through unchanged, so their offsets are stale by the length of the two insertions — the mapped source is still correct, only slightly shifted for code after the insertion point. The lockfile patch hash changes in all resolution keys, as expected.Verification
npx vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/ src/renderer/src/lib/pane-manager/— 312 files, 3907 passed, 1 skipped.rm -f config/*.tsbuildinfo && pnpm run typecheck— clean on a fresh buildinfo.npx oxlinton the changed file — exit 0.node config/scripts/check-react-doctor-changed.mjs— exit 0, 0 issues.Made with Orca 🐋