Bug korean ibus input still duplicated or droppe - #11011
Conversation
|
Caution Review failedFailed to post review comments. We encountered an issue with GitHub. Use ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout. (17)
🧰 Additional context used📓 Path-based instructions (4)**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,sh,ps1}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-07-22T18:28:59.997ZApplied to files:
🪛 ast-grep (0.45.0)tests/e2e/terminal-ibus-hangul-native.spec.ts[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) tests/e2e/terminal-ime-byte-reader.ts[warning] 68-68: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns. (regexp-from-variable) 🪛 Betterleaks (1.7.0)config/patches/@xterm__xterm@6.1.0-beta.287.patch[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 🪛 zizmor (1.28.0).github/workflows/terminal-ime-e2e.yml[warning] 60-60: ad-hoc installation of packages (adhoc-packages): installs a package outside of a lockfile (adhoc-packages) 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:
📝 WalkthroughWalkthroughAdds expanded IME composition de-duplication tests and new Playwright helpers for boundary tracing, exact-byte capture, and synthetic event sequences. Adds native IBus Hangul tests with isolated X11/D-Bus process management and evidence collection. Adds a GitHub Actions workflow that builds and runs deterministic and native tests on Ubuntu, uploads results, and validates workflow configuration and process cleanup patterns. 🚥 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: 4
🧹 Nitpick comments (7)
src/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.ts (1)
208-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting observable output alongside xterm internals.
These two tests rely solely on private xterm fields (
_compositionHelper._compositionPosition,_pendingCompositionFinalizations) that are only meaningful with the local@xterm/xtermpatch; the beta version bump or patch rebase will silently break them (or worse, pass vacuously if the field disappears andtoHaveLengththrows vs. changes shape). Adding anemitted-based expectation in the deferred-position test would keep the behavioral contract covered even if internals move.Also applies to: 293-305
tests/e2e/terminal-ime-byte-reader.ts (1)
69-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDedupe by sequence number before counting.
matchAllover the whole scrollback can yield the samePREFIX:n:line twice (e.g. redraw/reflow), makingresults.lengthreachexpectedLineCountwith duplicated entries.♻️ Dedupe by sequence
- results = [...terminal.matchAll(resultPattern)] - .sort((left, right) => Number(left[1]) - Number(right[1])) - .map((match) => match[2]) + const bySequence = new Map<number, string>() + for (const match of terminal.matchAll(resultPattern)) { + bySequence.set(Number(match[1]), match[2]) + } + results = [...bySequence.entries()] + .sort(([left], [right]) => left - right) + .map(([, hex]) => hex)config/scripts/run-terminal-ibus-hangul-e2e.mjs (1)
283-285: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the inner-session argument and surface failures as an exit code.
process.argv[3]is assumed present; when missing,path.join(undefined, ...)throws a confusingTypeError. A rejection from either branch also becomes an unhandled rejection rather than a clean non-zero exit.♻️ Suggested hardening
-const insideSession = process.argv[2] === insideSessionFlag -const exitCode = insideSession ? await runInsideSession(process.argv[3]) : await runOuter() -process.exitCode = exitCode +const insideSession = process.argv[2] === insideSessionFlag +try { + if (insideSession && !process.argv[3]) { + throw new Error(`${insideSessionFlag} requires an evidence directory argument`) + } + process.exitCode = insideSession ? await runInsideSession(process.argv[3]) : await runOuter() +} catch (error) { + console.error(`[terminal-ime] ${error instanceof Error ? error.message : error}`) + process.exitCode = 1 +}tests/e2e/terminal-ibus-hangul-native.spec.ts (2)
27-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA 1 ms default key delay is likely to make the native run flaky.
xdotool type --delay 1sends keystrokes faster than IBus/Hangul engine round-trips in CI, which is exactly the timing window where drops/duplication become nondeterministic and hard to distinguish from the bug under test. A higher default (e.g. 20-30 ms) keeps the suite meaningful while remaining fast at 5 repetitions; the env override still allows stress runs at 1 ms.Also applies to: 73-83
155-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth tests inherit whatever IME mode the previous one left behind.
The exact-byte test toggles
Hangultwice (net no-op), so the sentence test only passes because of that accidental symmetry. Explicitly (re)selecting Hangul mode at the start of each scenario — e.g. insidefocusNativeTerminalWindowafteribus engine hangul— removes the ordering dependency in thisserialdescribe.config/scripts/terminal-ime-e2e-workflow.test.mjs (2)
51-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSource-substring assertions on the runner are formatting-brittle.
These match exact prettier output of
run-terminal-ibus-hangul-e2e.mjs; a reflow (e.g. the ibus arg array wrapping onto multiple lines) fails the test without any behavior change. Prefer whitespace-tolerant regexes for the positive checks, keeping only thekillall/pkill/--replacenegative guards as literals.
25-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the located steps exist, and match the native step loosely.
If the install step is renamed or restructured,
installRunisundefinedandtoContainfails with an unhelpful message.runs.indexOf(...)also requires the run block to be byte-identical.♻️ Proposed change
const installRun = runs.find((run) => run.includes('apt-get install')) + expect(installRun).toBeDefined() expect(installRun).toContain('ibus-hangul')- const nativeIndex = runs.indexOf('pnpm run test:e2e:terminal-ime-native') + const nativeIndex = runs.findIndex((run) => run.includes('test:e2e:terminal-ime-native'))
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71ef897d-a7da-4d1f-ad0c-daa4b485cf0e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.github/workflows/terminal-ime-e2e.ymlconfig/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/scripts/run-terminal-ibus-hangul-e2e.mjsconfig/scripts/terminal-ime-e2e-workflow.test.mjspackage.jsonsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.tstests/e2e/terminal-ibus-hangul-native.spec.tstests/e2e/terminal-ime-boundary-probe.tstests/e2e/terminal-ime-byte-reader.tstests/e2e/terminal-ime-exact-byte.spec.tstests/e2e/terminal-ime-observed-event-sequences.ts
14e5ae7 to
84bf20b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
config/patches/@xterm__xterm@6.1.0-beta.287.patch (3)
28-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
instanceof CompositionHelperguards suggest the field type may not need to be widened, or the checks are redundant dead branches.
_handleTextAreaBlur, thecompositionendlistener, the dispose registration,_keyPress, and_inputEventall gate new behavior behindthis._compositionHelper instanceof CompositionHelper. Since_compositionHelperis only ever constructed viathis._instantiationService.createInstance(CompositionHelper, ...), theinstanceofcheck should always be true, and theelse/fallback branches (e.g. theelse { this._compositionHelper!.compositionend(); }in the listener) become unreachable dead code.If
_compositionHelper's declared type is a broader interface intentionally (e.g. to support future/alternate implementations), please note that in a comment; otherwise consider dropping the repeated runtime checks in favor of the concrete type, which would simplify all five call sites.
330-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNewly added early-return guard in
_finalizeCompositionlooks unreachable.private _finalizeComposition(waitForPropagation: boolean, endData: string = ''): void { const wasComposing = this._isComposing; ... if (waitForPropagation && !wasComposing) { return; }
_finalizeComposition(true, ...)is only invoked fromcompositionend(), which already returns early unlessthis._isComposingis true — sowasComposingwill always betrueon that path. All other callers (keydown(),blur()) passwaitForPropagation=false. This makes the guard dead code as currently wired.
425-465: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftComplex textarea/keypress/input reconciliation heuristic has no accompanying unit test in this diff.
_sendPendingComposition,_getPendingTextareaInput,_updatePostCompositionInputExpectation, and_getCompositionInputimplement an intricate reconciliation between deferred textarea snapshots and observedkeypress/inputcandidates (including anextCompositionStartinterruption case for back-to-back compositions). This is exactly the kind of logic prone to subtle regressions across browsers/IMEs, and none of the files selected for this "Additional reviewed files" cohort exercise it directly.Per the PR objectives, this is covered by
terminal-ime-xterm-composition-deduplication.test.tsand the native IBus Hangul e2e in other layers of the stack — worth double-checking those specifically exercise: (a) rapid back-to-back compositions wherenextCompositionStartgets set mid-flush, and (b) thepending.inputData || pending.keypressDataprecedence over the textarea-derived candidate..github/workflows/terminal-ime-e2e.yml (1)
58-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
node-gypthrough the repo lockfile.This workflow installs
node-gyp@11.5.0globally, while the repo tracksnode-gyp@12.3.0underpnpm. Add the required version as a workspace/tooling dependency and invoke it viapnpm exec, or at least keep the pinned global package audited/integrity-checked.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab04e538-9c5b-4205-a305-61b9138337cb
📒 Files selected for processing (4)
.github/workflows/terminal-ime-e2e.ymlconfig/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/scripts/run-terminal-ibus-hangul-e2e.mjsconfig/scripts/terminal-ime-e2e-workflow.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- config/scripts/terminal-ime-e2e-workflow.test.mjs
- config/scripts/run-terminal-ibus-hangul-e2e.mjs
84bf20b to
7312a49
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/e2e/terminal-ime-byte-reader.ts (1)
55-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuote the terminal command for its actual shell.
JSON.stringifydoes not shell-escapereader.scriptPath: POSIX shells still expand$and backticks, while Windows command parsing differs. Build the command with platform-specific quoting behind aprocess.platformcheck.As per coding guidelines, “Keep platform-dependent behavior behind runtime checks for macOS, Linux, and Windows.”
Source: Coding guidelines
config/patches/@xterm__xterm@6.1.0-beta.287.patch (1)
106-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComposition-transaction rewrite (
CompositionHelper.ts) looks internally consistent with the bundle changes.The new transaction model (
_pendingComposition,_compositionTransactionId,_isAwaitingCompositionEnd,keypress()/input()reconciliation,blur()/dispose()) is mirrored correctly in bothlib/xterm.jsandlib/xterm.mjs, andCoreBrowserTerminal.tsgates the new API surface behindinstanceof CompositionHelperconsistently in both bundles. One minor observation: in_finalizeComposition(waitForPropagation, endData), the guardif (waitForPropagation && !wasComposing) return;appears unreachable given current call sites —waitForPropagation=trueis only passed fromcompositionend(), which already returns early unlessthis._isComposingwastrue, sowasComposingwill always betruein that branch today. Not a correctness bug, just dead defensive code that could be simplified or left with a comment explaining it's for future-proofing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 401e1e71-a4b3-4b55-80b8-776ea401de1b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.github/workflows/terminal-ime-e2e.ymlconfig/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/scripts/run-terminal-ibus-hangul-e2e.mjsconfig/scripts/terminal-ime-e2e-workflow.test.mjspackage.jsonsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.tstests/e2e/terminal-ibus-hangul-native.spec.tstests/e2e/terminal-ime-boundary-probe.tstests/e2e/terminal-ime-byte-reader.tstests/e2e/terminal-ime-exact-byte.spec.tstests/e2e/terminal-ime-observed-event-sequences.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/e2e/terminal-ime-observed-event-sequences.ts
- package.json
- config/scripts/terminal-ime-e2e-workflow.test.mjs
- config/scripts/run-terminal-ibus-hangul-e2e.mjs
- tests/e2e/terminal-ime-boundary-probe.ts
- tests/e2e/terminal-ime-exact-byte.spec.ts
- src/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.ts
| function runXdotool(...args: string[]): void { | ||
| execFileSync('xdotool', args, { stdio: 'pipe' }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
site:nodejs.org/api child_process execFileSync timeout behavior
💡 Result:
In Node.js, the timeout option for child_process.execFileSync specifies the maximum number of milliseconds the child process is allowed to run [1][2]. If the process runs longer than this duration, the parent process sends the signal specified by the killSignal property, which defaults to SIGTERM [1][2]. Important behavioral details regarding this timeout include: 1. Method Blocking: Because execFileSync is a synchronous method, it blocks the Node.js event loop until the child process has fully closed [1][2]. Even after the timeout has been triggered and the kill signal sent, the method will not return until the child process has completely exited [1][2]. 2. Signal Handling: If the child process intercepts and handles the kill signal (e.g., SIGTERM) and does not exit, the parent process will continue to wait indefinitely until the child process eventually exits [1][2]. 3. Error Handling: If the child process times out, or if it exits with a non-zero exit code, execFileSync will throw an Error [1][2]. This error object contains the full result of the underlying child_process.spawnSync call, providing access to the captured output and exit information [1][2].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant e2e test without running repository code.
if command -v fd >/dev/null 2>&1; then
file="$(fd 'terminal-ibus-hangul-native\.spec\.ts$' . | head -n1 || true)"
else
file="$(find . -type f -name 'terminal-ibus-hangul-native.spec.ts' | head -n1 || true)"
fi
if [ -z "${file:-}" ]; then
echo "Relevant file not found"
exit 0
fi
echo "FILE=$file"
wc -l "$file"
echo "--- overview ---"
ast-grep outline "$file" --view compact || true
echo "--- relevant lines ---"
sed -n '1,110p' "$file" | nl -ba
echo "--- execFileSync usages ---"
rg -n "execFileSync|runXdotool|runIbus|timeout:|killSignal:" "$file"Repository: stablyai/orca
Length of output: 488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./tests/e2e/terminal-ibus-hangul-native.spec.ts"
echo "--- lines 1-110 ---"
awk '{printf "%5d %s\n", NR, $0}' "$file" | sed -n '1,120p'
echo "--- execFileSync usages ---"
grep -nE "execFileSync|runXdotool|runIbus|timeout:|killSignal:" "$file" || true
echo "--- file metadata ---"
stat "$file"Repository: stablyai/orca
Length of output: 5678
Bound synchronous native commands.
xdotool windowfocus --sync and the ibus engine commands invoke blocking execFileSync calls without a timeout. Use a shared execFileSync wrapper with a finite timeout so a stalled D-Bus/X11 command cannot occupy the full E2E workflow timeout.
Also applies to lines 65-67.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
| test.describe('Native IBus Hangul terminal input @headful', () => { | ||
| test.skip( | ||
| process.env.ORCA_E2E_NATIVE_IBUS_HANGUL !== '1', | ||
| 'Run through config/scripts/run-terminal-ibus-hangul-e2e.mjs' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'terminal-ibus-hangul-native.spec.ts' . || true
echo "== relevant file excerpt =="
if [ -f tests/e2e/terminal-ibus-hangul-native.spec.ts ]; then
sed -n '1,230p' tests/e2e/terminal-ibus-hangul-native.spec.ts | cat -n
fi
echo "== referenced runner script =="
fd -a 'run-terminal-ibus-hangul-e2e.mjs' . || true
if [ -f config/scripts/run-terminal-ibus-hangul-e2e.mjs ]; then
sed -n '1,240p' config/scripts/run-terminal-ibus-hangul-e2e.mjs | cat -n
fi
echo "== usage of ORCA_E2E_NATIVE_IBUS_HANGUL =="
rg -n "ORCA_E2E_NATIVE_IBUS_HANGUL|xdo|xdotool|ibus" . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || trueRepository: stablyai/orca
Length of output: 23939
Skip this Linux-native suite outside Linux at the spec level.
tests/e2e/terminal-ibus-hangul-native.spec.ts can enter this describe when ORCA_E2E_NATIVE_IBUS_HANGUL=1 is set on macOS or Windows, but focusNativeTerminalWindow() immediately runs xdotool/ibus before the runner’s Linux check runs.
Proposed fix
test.skip(
- process.env.ORCA_E2E_NATIVE_IBUS_HANGUL !== '1',
+ process.platform !== 'linux' || process.env.ORCA_E2E_NATIVE_IBUS_HANGUL !== '1',
'Run through config/scripts/run-terminal-ibus-hangul-e2e.mjs'
)📝 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.
| test.describe('Native IBus Hangul terminal input @headful', () => { | |
| test.skip( | |
| process.env.ORCA_E2E_NATIVE_IBUS_HANGUL !== '1', | |
| 'Run through config/scripts/run-terminal-ibus-hangul-e2e.mjs' | |
| ) | |
| test.describe('Native IBus Hangul terminal input `@headful`', () => { | |
| test.skip( | |
| process.platform !== 'linux' || process.env.ORCA_E2E_NATIVE_IBUS_HANGUL !== '1', | |
| 'Run through config/scripts/run-terminal-ibus-hangul-e2e.mjs' | |
| ) |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
Source: Coding guidelines
| process.stdout.write(resultPrefix + ':' + receivedLineCount + ':' + line.toString('hex') + '\\n') | ||
| if (receivedLineCount === expectedLineCount) { | ||
| process.exit(0) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
site:nodejs.org/api process.exit stdout writes may be truncated
💡 Result:
Calling process.exit in Node.js can cause output written to process.stdout and process.stderr to be truncated or lost [1][2]. This occurs because writes to these streams are often asynchronous and may span multiple ticks of the Node.js event loop [1][2]. When process.exit is invoked, it forces the process to terminate immediately, interrupting any pending asynchronous I/O operations before they can complete [1][2]. To avoid this issue, it is generally recommended to avoid calling process.exit explicitly [1]. Instead, you should allow the process to exit naturally by ensuring no additional work is pending in the event loop [1]. If a specific exit code is required, you can set the process.exitCode property and allow the process to finish gracefully [1][2]. If termination is necessary due to an error, throwing an uncaught error is considered a safer practice than calling process.exit [1][2].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced test file around the reported lines.
if [ -f tests/e2e/terminal-ime-byte-reader.ts ]; then
echo "== file exists =="
wc -l tests/e2e/terminal-ime-byte-reader.ts
echo "== lines 1-120 =="
cat -n tests/e2e/terminal-ime-byte-reader.ts | sed -n '1,120p'
else
echo "referenced file missing"
fd -a 'terminal-ime-byte-reader\.ts' .
fi
echo "== related process.exit writes in referenced file =="
rg -n "process\.exit|stdout\.write|resultPrefix|receivedLineCount" tests/e2e/terminal-ime-byte-reader.ts || trueRepository: stablyai/orca
Length of output: 4492
🌐 Web query:
Node.js process.stdout.write callback synchronous false process.exit callback
💡 Result:
In Node.js, process.stdout.write does not guarantee synchronous completion, and its behavior—whether it behaves synchronously or asynchronously—depends on the underlying file descriptor, such as whether it is connected to a TTY, a file, or a pipe [1][2]. Regarding your query: 1. Asynchronous Behavior and Callbacks: While process.stdout.write(chunk, [encoding], [callback]) accepts an optional callback, this callback indicates only that the data has been handed off to the operating system; it does not guarantee that the data has been fully flushed or received by the destination [1][3]. 2. Process Exit: Calling process.exit forces the process to terminate immediately [4][5]. If there are asynchronous I/O operations pending—including writes to process.stdout—they will be abandoned, which can lead to truncated or missing output [4][5][6]. 3. Synchronicity: Writes to process.stdout are generally synchronous when the output is a file or a TTY (on POSIX systems), but they are often asynchronous when the output is a pipe [1][2]. Because this behavior is platform-dependent and dynamic, relying on the synchronous nature of a write is not recommended [2][6]. Recommendation: To ensure all data is written before a process exits, avoid calling process.exit explicitly [4][5]. Instead, allow the Node.js process to exit naturally by letting the event loop empty [4][5]. If you must exit with a specific code, use process.exitCode and ensure no further work is scheduled [4][5]. For cases where you must explicitly handle cleanup, you can monitor the stream's state (such as writableLength), but note that there is no universal "fireproof" API in Node.js to guarantee that stdout/stderr have been fully flushed to a pipe before process termination [3][7].
Citations:
- 1: https://www.thenodebook.com/process-os/standard-io
- 2: https://sxlijin.github.io/2024-10-09-node-stdout-disappearing-bytes
- 3: writable.write callback is called before the buffer is flushed nodejs/node#20687
- 4: https://nodejs.org/api/process.html
- 5: https://nodejs.org/dist/latest/docs/api/process.html
- 6: Inconsistent output depending on if it's piped somewhere or not nodejs/node#53242
- 7: https://stackoverflow.com/questions/42047997/flush-drain-stdout-stderr-in-node-js-process-before-exiting
🌐 Web query:
Node.js fs writeFileSync stdout write close drain process.exit documentation
💡 Result:
In Node.js, calling process.exit forces the process to terminate as quickly as possible, which can lead to truncated output or lost data if there are pending asynchronous I/O operations, including writes to process.stdout or process.stderr [1][2]. Key technical considerations: 1. Asynchronous nature of stdio: While writes to process.stdout may appear synchronous when directed to a terminal (TTY), they are frequently asynchronous when piped to other processes or files [3][4][5]. Because process.exit terminates the event loop immediately, it does not wait for these pending asynchronous operations to complete [1][2]. 2. Limitations of write callbacks: Using a callback with process.stdout.write does not guarantee that the data has been fully flushed to the underlying OS buffers before process.exit is called [6]. The process will still exit abruptly if the event loop is stopped by process.exit [3]. 3. The drain event: The 'drain' event is used for backpressure management—it signals that a writable stream's buffer has been emptied and it is safe to resume writing after a previous write call returned false [3]. It is not a mechanism for ensuring all data is flushed before an exit, and it should not be relied upon to keep the process alive [3]. 4. Recommended practice: The official Node.js documentation advises against calling process.exit explicitly [1][2]. Instead, developers should: - Use process.exitCode to set the desired exit status [1][2]. - Allow the process to exit naturally by ensuring no further work is scheduled on the event loop; this allows pending I/O operations and streams to flush gracefully [3][1][2]. - If an error must terminate the process, throwing an uncaught error is considered safer than using process.exit [1][2].
Citations:
- 1: https://nodejs.org/api/process.html
- 2: https://nodejs.org/dist/latest/docs/api/process.html
- 3: https://www.thenodebook.com/process-os/standard-io
- 4: Stdout is not completely flushed on process exit nodejs/node#2972
- 5: https://sxlijin.github.io/2024-10-09-node-stdout-disappearing-bytes
- 6: documentation on Stream.write misses that callback does not ensure that data written to process.stdout/.stderr is flushed nodejs/node#3670
Flush the final byte record before terminating.
process.exit(0) exits before any pending stdout writes from the PTY shell may complete, so the last expected line can be lost. Set process.exitCode = 0 and remove the explicit process.exit(0) so the reader can exit naturally after the final stream write completes.
7312a49 to
7aa6838
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/patches/@xterm__xterm@6.1.0-beta.287.patch (1)
44-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated
instanceof CompositionHelperguards suggest the interface is missing the new members.
blur(),dispose(),keypress(),input(), and theCompositionEvent-accepting overload ofcompositionend()are all guarded byinstanceof CompositionHelperat four separate call sites. Since_compositionHelperis always constructed viathis._instantiationService.createInstance(CompositionHelper, ...), these branches likely only exist for TypeScript type-narrowing against a narrowerICompositionHelperinterface. Adding these methods to that interface would remove the repeated runtime checks and the risk that a future call site forgets the guard (silently falling back to old, now-incomplete behavior).♻️ Suggested direction
- private _handleTextAreaBlur(): void { - if (this._compositionHelper instanceof CompositionHelper) { - this._compositionHelper.blur(); - } + private _handleTextAreaBlur(): void { + this._compositionHelper?.blur();(assuming
blur/dispose/keypress/input/compositionend(ev)are added toICompositionHelper)Also applies to: 56-61, 69-73, 82-84, 92-100
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bc2e4a0-6940-4733-bd8a-1fe441d8813b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.github/workflows/terminal-ime-e2e.ymlconfig/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/scripts/run-terminal-ibus-hangul-e2e.mjsconfig/scripts/terminal-ime-e2e-workflow.test.mjspackage.jsonsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.tstests/e2e/terminal-ibus-hangul-native.spec.tstests/e2e/terminal-ime-boundary-probe.tstests/e2e/terminal-ime-byte-reader.tstests/e2e/terminal-ime-exact-byte.spec.tstests/e2e/terminal-ime-observed-event-sequences.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- package.json
- tests/e2e/terminal-ime-exact-byte.spec.ts
- config/scripts/terminal-ime-e2e-workflow.test.mjs
- tests/e2e/terminal-ime-observed-event-sequences.ts
- config/scripts/run-terminal-ibus-hangul-e2e.mjs
- src/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.ts
- tests/e2e/terminal-ime-boundary-probe.ts
- Add deterministic xterm boundary tests and a Linux/X11 native IBus Hangul E2E runner - Patch xterm composition handling and expand unit coverage for commit deduplication and retention - Wire a scheduled GitHub Action to install IME tooling, run the new suites, and archive evidence
7aa6838 to
a767606
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| }) | ||
|
|
||
| function nativeRepetitions(): number { | ||
| const parsed = Number(process.env.ORCA_E2E_NATIVE_IBUS_REPETITIONS ?? DEFAULT_REPETITIONS) | ||
| return Number.isInteger(parsed) && parsed > 0 | ||
| ? Math.min(parsed, MAX_REPETITIONS) | ||
| : DEFAULT_REPETITIONS |
There was a problem hiding this comment.
IBUS_ENABLE_SYNC_MODE=1 disables the async timing the original bug depended on
The original defect (dropped/duplicated Hangul) was caused by asynchronous IBus event ordering — deferred finalizers seeing stale state and multiple paths racing to emit the same commit. Forcing IBUS_ENABLE_SYNC_MODE=1 on both the outer session (in run-terminal-ibus-hangul-e2e.mjs) and the Electron app eliminates that async interleaving entirely. This means the native suite validates the new queue logic against a sequential event stream, not the actual race-prone stream real users encounter. A test run that passes under sync mode cannot confirm the fix handles the asynchronous case that caused the original issue.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| }) | ||
|
|
||
| it('preserves composition-first order when keypress overlaps its suffix', async () => { | ||
| it('keeps a following keypress even when it matches the composition suffix', async () => { | ||
| const { emitted, terminal, textarea } = openTerminal() | ||
| startComposition(textarea, '가한') | ||
| await nextEventLoop() | ||
|
|
||
| textarea.dispatchEvent(new CompositionEvent('compositionend', { bubbles: true })) | ||
| dispatchCompositionEvent(textarea, 'compositionend') | ||
| dispatchKeypress(textarea, '한') | ||
| await nextEventLoop() | ||
|
|
||
| expect(emitted.join('')).toBe('가한') | ||
| expect(emitted.join('')).toBe('가한한') | ||
| terminal.dispose() |
There was a problem hiding this comment.
Three existing tests were renamed and their expected output was inverted:
'keeps a following keypress even when it matches the composition suffix': previously'가한', now'가한한''keeps a following keypress even when it matches the composition prefix': previously'한a', now'한a한'- Ordering of composition text vs following keypress: previously
'한a', now'a한'
The removed suppression logic existed to handle macOS and Windows IMEs that fire a real keypress event whose text is the committed composition text (i.e. the suffix). Emitting both means users on those platforms could see duplicated characters after every IME commit. The PR description acknowledges this risk explicitly, but the test suite that validates macOS/Windows/WSL paths is described as "deterministic/static review only." Consider whether the overlap detection can be scoped to the Hangul-specific path (e.g. guarded by isComposing === false on the following keypress) to preserve safety for other IMEs.
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| persist-credentials: false | ||
|
|
There was a problem hiding this comment.
actions/checkout@v6, actions/setup-node@v6, pnpm/action-setup@v6, and actions/upload-artifact@v7 all use floating version tags. A compromised or updated tag could silently swap in malicious code on future workflow runs. Pinning each action to a full commit SHA (e.g. actions/checkout@<sha>) is the standard supply-chain hardening practice for third-party GitHub Actions.
| '한abc글', | ||
| dispatchObservedIbusHangulMixedSequence, | ||
| (trace) => { | ||
| const commits = trace.dom | ||
| .filter((event) => event.type === 'input' && event.inputType === 'insertText') | ||
| .map((event) => event.data) | ||
| expect(commits).toEqual(expect.arrayContaining(['한', '글'])) |
There was a problem hiding this comment.
test.skip(process.platform !== 'linux', ...) means the deterministic exact-byte scenarios — which now embed the new suffix/prefix-pass-through behavior — never run on macOS or Windows CI. If the new composition reconciliation emits extra characters for macOS IME sequences, the exact-byte suite will not catch it. Consider whether any platform-agnostic subset of these scenarios can run unconditionally, or add a separate macOS run in the workflow.
Greptile SummaryThis PR patches the vendored xterm.js bundle and adds a matching test infrastructure to fix duplicated/dropped Korean (Hangul) IBus input on Linux/X11. The core change replaces a single global
Confidence Score: 3/5This is an experimental fix with real unknowns: the test suite forces synchronous IBus mode, which does not replicate the async race that caused the original bug; and three tests were deliberately changed to allow keypresses that were previously suppressed, introducing a concrete risk of duplicated characters on macOS and Windows IMEs. The xterm composition logic change is embedded entirely in a minified bundle, making the reconciliation queue and FIFO loop unverifiable at the source level. The native test suite gates on IBUS_ENABLE_SYNC_MODE=1 rather than the default async mode — so passing 30/30 trials under that setup says less than it appears. The deliberate removal of suffix/prefix keypress deduplication — necessary for Hangul final-consonant transfer — directly changes behavior for code paths used by other IMEs on other platforms, and no automated cross-platform validation exists for those paths. These concerns compound rather than cancel each other. Files Needing Attention: config/patches/@xterm__xterm@6.1.0-beta.287.patch (unreadable minified diff), tests/e2e/terminal-ibus-hangul-native.spec.ts (IBUS_ENABLE_SYNC_MODE concern), src/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.ts (inverted suffix/prefix deduplication expectations)
|
| Filename | Overview |
|---|---|
| config/patches/@xterm__xterm@6.1.0-beta.287.patch | Core composition logic fix embedded in a minified bundle — entirely unreadable at the source level; changes to FIFO per-transaction state and the reconciliation loop cannot be algorithmically reviewed |
| src/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-deduplication.test.ts | Comprehensive new unit tests for Korean/CJK composition; three existing tests deliberately inverted their expected output to remove suffix/prefix keypress deduplication — a behavioral change that may regress macOS/Windows IMEs |
| tests/e2e/terminal-ibus-hangul-native.spec.ts | New native IBus Hangul e2e spec, but forces IBUS_ENABLE_SYNC_MODE=1 which eliminates the async timing that caused the original bug; tests skipped unless ORCA_E2E_NATIVE_IBUS_HANGUL=1 |
| config/scripts/run-terminal-ibus-hangul-e2e.mjs | Well-structured isolated X11/D-Bus/IBus session runner with proper process-group cleanup; minor bug where evidence.ibusGroupBeforeCleanup is overwritten in the finally block |
| .github/workflows/terminal-ime-e2e.yml | New workflow for Linux/X11 IME testing with correct read-only permissions; all four third-party actions use floating version tags instead of pinned SHAs — supply chain risk |
| tests/e2e/terminal-ime-exact-byte.spec.ts | Deterministic exact-byte Hangul/Japanese/Chinese scenarios; skipped on non-Linux which prevents catching regressions on macOS/Windows where the removed deduplication may matter |
| tests/e2e/terminal-ime-boundary-probe.ts | Clean DOM event and onData trace probe; accesses internal pane state via window globals but is test-only and isolated to the probe install/dispose pattern |
| tests/e2e/terminal-ime-byte-reader.ts | Writes a CJS hex-reporter script to the worktree and sends it to the terminal PTY for exact-byte verification; uses JSON.stringify for path quoting and cleans up via rmSync in finally |
| tests/e2e/terminal-ime-observed-event-sequences.ts | Synthetic IBus event sequence replay using DOM APIs; textarea.value is never updated to reflect the first composition commit in the retained variant, which may not faithfully replicate real IBus sequences |
| config/scripts/terminal-ime-e2e-workflow.test.mjs | Vitest-based workflow structural test that validates apt-get packages, step ordering, and runner script contents by reading live files |
| package.json | Adds test:e2e:terminal-ime-native script pointing to the new runner |
Sequence Diagram
sequenceDiagram
participant IBus as IBus (native)
participant Chromium as Chromium/X11
participant xterm as xterm.js patch
participant Queue as FIFO Pending Queue
participant Terminal as terminal.onData()
participant PTY as PTY
IBus->>Chromium: compositionstart
Chromium->>xterm: compositionstart event
xterm->>Queue: create composition transaction
IBus->>Chromium: compositionupdate(한)
Chromium->>xterm: compositionupdate + input(insertCompositionText)
xterm->>Queue: update current transaction
IBus->>Chromium: compositionend (deferred finalizer)
Chromium->>xterm: compositionend
xterm->>Queue: enqueue pending finalization (scoped to transaction)
note over IBus,xterm: Async: finalizer may see stale textarea state
Chromium->>xterm: input(insertText, 한)
xterm->>Queue: reconcile: match insertText to pending transaction
Queue->>Terminal: emit 한 (exactly once)
IBus->>Chromium: next compositionstart (글)
Chromium->>xterm: compositionstart
xterm->>Queue: new transaction (old transaction finalized)
IBus->>Chromium: compositionend(글) + insertText(글)
Chromium->>xterm: compositionend + input(insertText, 글)
xterm->>Queue: reconcile: emit 글
Queue->>Terminal: emit 글
Terminal->>PTY: forward 한abc글\r
Comments Outside Diff (2)
-
config/scripts/run-terminal-ibus-hangul-e2e.mjsThe
finallyblock re-captures and overwritesevidence.ibusGroupBeforeCleanup, which was already populated in thetryblock (line 169). Only the later snapshot (taken just before cleanup, after the tests finish) survives into the JSON artifact. The earlier snapshot — which reflects the live IBus group during the test run — is silently lost. Using a distinct field name preserves both captures for debugging. -
config/patches/@xterm__xterm@6.1.0-beta.287.patch, line 1-5 (link)The diff modifies
lib/xterm.js(a single-line minified bundle). All of the FIFO queue, per-transaction state, and reconciliation loop changes are embedded in this unreadable form. There is no corresponding diff of the TypeScript source or a human-readable re-write of the patched functions, so the composition logic cannot be reviewed for correctness at the algorithmic level. Upstream Fix dropped and duplicated IME composition commits xtermjs/xterm.js#6060 reportedly contains the readable source; linking to and including a summary of the source-level diff in the PR description would significantly improve reviewability.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "fix(terminal): reject stale IME composit..." | Re-trigger Greptile
2 participants
Caution
This PR must be further verified on an independent Linux/X11 machine with a real IBus Hangul input method before merge. The current implementation is an experimental, best-effort reconciliation queue/loop that repeatedly tries to merge asynchronous composition, keypress,
input, and textarea observations. The same-host results are encouraging, but we are not yet sure that this model is generally valid for every native IME event order.It can also cause regression in other language (e.g. Chinese, Japanese) and regression on other platforms (macOS, Windows)
Summary
Fixes #9862.
The investigation localized the corruption to xterm's composition reconciliation:
composition*/input(insertText)commits were correct.terminal.onData()already contained the same dropped or duplicated text later observed at the PTY, so Orca's PTY forwarding was not the failing boundary.insertText, and both the timer andinput(insertText)path emitting the same commit.The xterm dependency patch now:
input, and textarea observations before emitting;This PR also adds:
terminal.onData()-> PTY exact-byte scenarios;xdotoolsequences;The underlying xterm work was also ported upstream as xtermjs/xterm.js#6060.
What has been verified
The Codex session records contain the following same-host results from Ubuntu/X11 with IBus 1.5.29, ibus-hangul 1.5.5, and Electron 43.1.0:
한abc글exact bytesFor the patched exact-byte run:
terminal.onData()emitted exactly 30 copies of한abc글\r;ed959c616263eab8800aon all 30 trials.The recorded automated checks also report:
git diff --checkwas clean;What is not yet verified
Testing
The dedicated workflow is intended to run, in order:
test-results/.Before merge, please run the native suite on a separate Linux/X11 machine and inspect the DOM,
terminal.onData(), and PTY evidence rather than accepting retries alone as proof.AI Review Report
Codex was used for reproduction, boundary tracing, implementation, test generation, and an adversarial static/test review. The evidence above separates observed native results from synthetic tests and static reasoning; it does not claim universal IME correctness.
Security Audit
The native runner starts its own Xvfb/D-Bus/IBus process group, records owned PIDs, and terminates only that group. The workflow has read-only repository permissions. Automated review comments about workflow credential persistence and runner hardening still need to be resolved separately.
Notes
This should be treated as an experimental fix with useful regression infrastructure, not as a fully validated general solution. Independent Linux verification is the next required confidence step.