fix(terminal): stop dropped/duplicated chars on macOS fast typing#904
fix(terminal): stop dropped/duplicated chars on macOS fast typing#904vincentzzh424 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesPTY input channeling and terminal IME dedup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 3
🤖 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-tauri/src/modules/pty/mod.rs`:
- Around line 126-142: The PTY write path in the write handler uses
spawn_blocking per invoke, which can let concurrent jobs reach
session.writer.lock() out of order and scramble input. Update the PTY module’s
write flow to serialize writes per session by routing bytes through a single
queued writer or dedicated writer task, and ensure the existing write_all/error
handling remains inside that ordered path rather than spawning a separate
blocking job for each chunk.
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 279-294: The current kc=229 dedup logic in rendererPool’s keydown
handling uses only single-slot state (`selfSentChar`/`selfSentAt` and
`lastOdChar`), which lets a later burst overwrite an earlier pending character
and causes duplicates. Replace those scalar fields with a FIFO queue of pending
`{char, at}` entries in the same keydown/onData flow, and update the matching
logic to consume the oldest matching entry rather than overwriting state. Make
sure the new queue-based handling preserves the existing preventDefault/return
false behavior in the keydown path while correctly deduplicating fast
interleaved input.
- Around line 266-270: The 229 keydown handling in rendererPool’s input path is
using a single shared slot for selfSentChar/lastOdChar, so earlier characters
can be overwritten before onData reads them. Update the keydown tracking in the
relevant rendererPool logic to preserve 229 inputs per event in FIFO order (or
another per-event queue) and consume them in the same order in the onData path,
instead of storing only one pending char.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f142fed3-7be5-4abd-a872-44a7b610f51d
📒 Files selected for processing (2)
src-tauri/src/modules/pty/mod.rssrc/modules/terminal/lib/rendererPool.ts
Two correctness issues flagged by review on crynta#904: 1. keyCode-229 dedup used a single scalar slot (selfSentChar/lastOdChar), so a fast burst like `cd` overwrote the pending char before its trailing onData arrived, leaking the older char through and doubling it (cd -> ccd). Replace with per-direction FIFO queues of {char, at} and consume the oldest matching entry within the 200ms window. 2. Each pty_write invoke spawned an independent spawn_blocking job that contended for session.writer.lock() with no ordering guarantee, so a burst of keystrokes could reach the PTY out of order and scramble input. Route all writes through a per-session mpsc channel drained by a single dedicated writer thread; pty_write now does a non-blocking send. Verified: cargo clippy --locked (clean), cargo test --locked (220 passed), pnpm check-types (clean), biome lint (0 new errors). Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed the three CodeRabbit review comments in 1. PTY write ordering ( 2 & 3. Single-slot kc=229 dedup ( Verified: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
gh pr diff 904 --repo crynta/terax-ai > /tmp/pr904.diff 2>&1 || true
wc -l /tmp/pr904.diff
echo "---"
sed -n '1,400p' /tmp/pr904.diffLength of output: 12637 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
git clone --depth 50 --branch $(gh pr view 904 --repo crynta/terax-ai --json headRefName -q .headRefName) https://github.com/crynta/terax-ai.git repo 2>&1 | tail -5
cd repo
echo "---writer usage in session.rs---"
rg -n 'writer' src-tauri/src/modules/pty/session.rs
echo "---DA reply / reader thread context---"
rg -n 'reader|DA reply|thread::spawn' -A5 -B5 src-tauri/src/modules/pty/session.rs | head -150Length of output: 992 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf repo
git clone --depth 1 https://github.com/crynta/terax-ai.git repo 2>&1 | tail -5
cd repo
git fetch origin refs/pull/904/head:pr904 --depth 50 2>&1 | tail -10
git checkout pr904
echo "---writer usage in session.rs---"
rg -n 'writer' src-tauri/src/modules/pty/session.rs
echo "---thread::spawn / reader thread context---"
rg -n 'thread::spawn|fn spawn|read_to' -A5 -B5 src-tauri/src/modules/pty/session.rs | head -200Length of output: 2411 🏁 Script executed: #!/bin/bash
cd /tmp/repo
rg -n 'pty_write|writeToPty' -A5 -B2 src/modules/terminal --type=ts | head -100Length of output: 5364
1. Write ordering — the dedicated 2 & 3. kc=229 dedup — the FIFO-queue approach ( Two minor, non-blocking notes for awareness (not asking for changes):
Nice diagnosis on the ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-tauri/src/modules/pty/session.rs`:
- Around line 43-47: The new comments in Session cleanup are using em dash
characters, which violates the repo text convention. Update the comment block in
Session::drop-related cleanup so the prose uses plain hyphens or commas instead
of em dashes, and keep the wording otherwise unchanged. Use the existing symbols
write_tx, master, and the surrounding reader/writer thread comments to locate
the affected text.
- Around line 147-151: The PTY input queue in `session.rs` is unbounded because
`mpsc::channel` lets `pty_write` keep accepting data even when the writer can’t
drain it, causing potential RAM growth. Update the dedicated writer path around
`write_tx`/`write_rx` to use a bounded queue or enforce a byte cap, and make
`pty_write` surface backpressure when the limit is hit instead of always
succeeding. Keep the serialized write behavior in `writer.lock()` intact while
preventing large pastes or stalled PTYs from accumulating unlimited `Vec<u8>`
buffers.
- Around line 156-160: The PTY writer loop in session handling keeps draining
`write_rx` after `write_all` or the writer lock fails, so `pty_write` can
continue accepting data even though writes are no longer possible. Update the
write loop in `session.rs` to stop processing immediately on either
`writer_for_writer.lock()` failure or `w.write_all(&bytes)` error, letting the
receiver close so `pty_write` sees the channel-closed error. Use the existing
`write_rx`, `writer_for_writer`, and `pty_write` flow to locate the fix.
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 290-295: The repeated-key handling in rendererPool’s onDataQueue
matching is too broad because findIndex by char can remove an older
same-character entry instead of the current keystroke. Update the logic around
slot.onDataQueue and the event.key handling to match a specific keystroke
instance in order, not just by character value, so later repeats within the 200
ms window are not dropped.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0032db21-30bf-4146-a83e-676f7410fc54
📒 Files selected for processing (3)
src-tauri/src/modules/pty/mod.rssrc-tauri/src/modules/pty/session.rssrc/modules/terminal/lib/rendererPool.ts
Address the second round of CodeRabbit review on crynta#904: - Bound the PTY input queue with a byte cap (WRITE_QUEUE_CAP, 16 MiB) tracked via an AtomicUsize. pty_write reserves capacity before enqueueing and rolls back on send failure, so a stuck PTY (child hung, pipe full) surfaces backpressure instead of growing memory unbounded. - The writer thread now breaks on write or lock failure. Dropping the receiver closes the channel, so later pty_write calls get a channel-closed error rather than silently discarding input. - Remove em dashes from touched comments per the repo-wide convention (TERAX.md: no em-dash anywhere). Verified: cargo clippy --locked (clean), cargo test --locked (221 passed), pnpm check-types (clean), biome lint (0 new errors). Co-Authored-By: Claude <noreply@anthropic.com>
|
Ready for review. All CodeRabbit threads (both rounds) are resolved:
All local checks green: Two things still need a maintainer (both require admin on
@crynta when you have a moment. |
macOS WKWebView tags ordinary fast typing with keyCode 229 ("Process"),
not just IME. xterm.js skips its keydown->onData path for keyCode 229 and
reads chars from textarea input events, which the browser coalesces under
fast typing -- so some keystrokes never emit onData (chars dropped, e.g.
cdcdcd came out ccccc) and at slow speed both our self-send and xterm's
onData fire (chars doubled, e.g. c came out cc).
Self-send keyCode-229 single-char keystrokes directly to the PTY and
dedup against xterm's onData with a short window in both orderings, so
exactly one copy of each char is sent. Real IME composition (isComposing)
still goes through xterm's compositionend path unchanged.
Also move pty_write off the webview main thread (async + spawn_blocking)
so a burst of keystrokes can't serialize writes there.
Co-Authored-By: Claude <noreply@anthropic.com>
Two correctness issues flagged by review on crynta#904: 1. keyCode-229 dedup used a single scalar slot (selfSentChar/lastOdChar), so a fast burst like `cd` overwrote the pending char before its trailing onData arrived, leaking the older char through and doubling it (cd -> ccd). Replace with per-direction FIFO queues of {char, at} and consume the oldest matching entry within the 200ms window. 2. Each pty_write invoke spawned an independent spawn_blocking job that contended for session.writer.lock() with no ordering guarantee, so a burst of keystrokes could reach the PTY out of order and scramble input. Route all writes through a per-session mpsc channel drained by a single dedicated writer thread; pty_write now does a non-blocking send. Verified: cargo clippy --locked (clean), cargo test --locked (220 passed), pnpm check-types (clean), biome lint (0 new errors). Co-Authored-By: Claude <noreply@anthropic.com>
Address the second round of CodeRabbit review on crynta#904: - Bound the PTY input queue with a byte cap (WRITE_QUEUE_CAP, 16 MiB) tracked via an AtomicUsize. pty_write reserves capacity before enqueueing and rolls back on send failure, so a stuck PTY (child hung, pipe full) surfaces backpressure instead of growing memory unbounded. - The writer thread now breaks on write or lock failure. Dropping the receiver closes the channel, so later pty_write calls get a channel-closed error rather than silently discarding input. - Remove em dashes from touched comments per the repo-wide convention (TERAX.md: no em-dash anywhere). Verified: cargo clippy --locked (clean), cargo test --locked (221 passed), pnpm check-types (clean), biome lint (0 new errors). Co-Authored-By: Claude <noreply@anthropic.com>
vincentzzh424
force-pushed
the
fix/terminal-mac-fast-input
branch
from
443ff4b to
b10cbf9
Compare
1 participant
Problem
On macOS, fast typing in the terminal drops or duplicates characters:
cdcdcd...comes outccccc...(every other char dropped).c->cc).shift + @/ symbols are hard to type.Other terminals (Terminal.app, iTerm, Alacritty) don't have this.
Root cause
macOS WKWebView tags ordinary fast typing with
keyCode 229("Process"), not only IME composition. xterm.js skips its ownkeydown -> onDatapath forkeyCode 229and instead reads the typed char from textareainputevents:onDatafor some of them -> keystrokes dropped.onData, but our keydown handler also fires -> each char doubled.The existing guard
if (event.isComposing || event.keyCode === 229) return false;returnedfalse(telling xterm to skip) for all 229 keys, which silently swallowed the fast-typing ones.Verified with instrumentation: 101 keydowns for
cdcdcd..., only 62onDatafired, every dropped key hadkeyCode 229andisComposing === false.Fix
src/modules/terminal/lib/rendererPool.ts:keyCode === 229&&!isComposing&& single-char keys, forwardevent.keyto the PTY directly and tell xterm not to process it.onDatawith a short window in both orderings (onData-first andkeydown-first), so exactly one copy of each char is sent whether the OS fires the textarea input before or after the keydown.isComposing) still flows through xterm'scompositionendpath unchanged.src-tauri/src/modules/pty/mod.rs:pty_writeoff the webview main thread (async+spawn_blocking) so a burst of keystrokes can't serialize writes on the main thread.Verification
Tested manually on macOS (Apple Silicon) across all previously-broken cases, now correct:
cdcdcd...->cdcdcd...(wasccccc...)shift + @,!,~symbols -> correct (was flaky)Instrumentation during debugging confirmed
dup = 0(no doubling), self-send count matched the number of keys xterm dropped.Checks
cargo clippy --locked-> cleancargo test --locked-> 220 passedpnpm check-types-> cleanbiome lint-> clean (0 new errors)🤖 Generated with Claude Code
Summary by CodeRabbit