Skip to content

fix(core): prevent TCP chunk escape sequence breakage in InputParser#1964

Open
ionfwsrijan wants to merge 2 commits into
Karanjot786:mainfrom
ionfwsrijan:fix/1957-inputparser-tcp-escape-breakage
Open

fix(core): prevent TCP chunk escape sequence breakage in InputParser#1964
ionfwsrijan wants to merge 2 commits into
Karanjot786:mainfrom
ionfwsrijan:fix/1957-inputparser-tcp-escape-breakage

Conversation

@ionfwsrijan

@ionfwsrijan ionfwsrijan commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Fix: InputParser TCP chunk escape sequence breakage

Changes

packages/core/src/input/InputParser.ts

  • Replaced the fixed 200ms timeout for lone ESC bytes with a new _startEscapeTimeout() method that uses a 500ms timeout.
  • Added timeout restart in the escape buffer concatenation handler: each partial chunk resets the timer, preventing false standalone ESC emission.
  • Cleaned up the lone ESC code path by extracting timeout logic into its own method.

Fixes

  • Escape sequences fragmented across TCP chunks (SSH, serial connections) with >200ms gaps were incorrectly parsed as standalone Escape keypresses followed by garbage characters
  • After the initial ESC timeout fired, subsequent sequence bytes were processed as regular input, corrupting the input stream
  • 200ms window was too aggressive for high-latency network connections

Closes #1957

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of the Escape key so incomplete or delayed key sequences are buffered more reliably.
    • Restarts the Escape timeout when more input arrives, reducing misread key events.
    • Lone Escape presses now emit consistently after a short delay.

@ionfwsrijan ionfwsrijan requested a review from Karanjot786 as a code owner July 3, 2026 09:56
@github-actions github-actions Bot added type:bug +10 pts. Bug fix. area:core @termuijs/core labels Jul 3, 2026
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@ionfwsrijan, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 32 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: cef0eeba-e532-435d-a07d-41bc80546846

📥 Commits

Reviewing files that changed from the base of the PR and between 3833157 and 92bd707.

📒 Files selected for processing (2)
  • packages/core/src/input/InputParser.test.ts
  • packages/core/src/input/InputParser.ts
📝 Walkthrough

Walkthrough

Escape-key parsing in InputParser.ts was refactored to use a new _startEscapeTimeout() method that centralizes timeout management for buffered escape sequences. The lone-ESC inline setTimeout was replaced with buffering plus this helper, and fragmented escape parsing now restarts the timeout when bytes remain incomplete.

Changes

Escape Timeout Consolidation

Layer / File(s) Summary
New _startEscapeTimeout() helper
packages/core/src/input/InputParser.ts
Adds a private method that clears any existing timeout, sets a 500ms timer, and on expiry emits an escape key event from _escapeBuffer, then clears the buffer and timeout handle.
Wire helper into ESC handling paths
packages/core/src/input/InputParser.ts
Restarts the escape timeout via the new helper after _tryParseEscape() when buffered bytes remain incomplete, and replaces the lone-ESC inline setTimeout with buffering plus a call to _startEscapeTimeout().

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • Karanjot786/TermUI#770: Both PRs modify InputParser.ts's escape-sequence buffering/timeout emission logic.
  • Karanjot786/TermUI#1064: Both PRs change escape-buffering/parsing logic to prevent mis-emitting keys from split escape sequences.
  • Karanjot786/TermUI#1806: Both PRs modify escape-buffering/timeout logic including _tryParseEscape behavior.

Suggested labels: level:intermediate

Suggested reviewers: Karanjot786

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the InputParser escape-sequence fix.
Description check ✅ Passed It covers the change summary and linked issue, though some template sections like package list and checklist are omitted.
Linked Issues check ✅ Passed The changes match #1957 by buffering fragmented ESC sequences and restarting the timer so split control keys parse correctly.
Out of Scope Changes check ✅ Passed The patch stays focused on InputParser escape handling and adds no unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/core/src/input/InputParser.ts (1)

203-207: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Add the escape timeout in packages/core/src/input/InputParser.ts:203-207.
When the first chunk starts with \x1b but already has more bytes, _tryParseEscape() can leave _escapeBuffer populated without scheduling _startEscapeTimeout(), so a stalled sequence can keep swallowing later keystrokes.

Proposed fix
         if (str.startsWith('\x1b')) {
             this._escapeBuffer = data;
             this._tryParseEscape();
+            if (this._escapeBuffer.length > 0) {
+                this._startEscapeTimeout();
+            }
             return;
         }
🤖 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 `@packages/core/src/input/InputParser.ts` around lines 203 - 207, The
escape-sequence handling in InputParser._tryParseEscape can leave _escapeBuffer
set without starting the escape timeout when the first chunk begins with \x1b
and already contains more bytes. Update the early escape branch in InputParser
to schedule _startEscapeTimeout() whenever _escapeBuffer is populated from that
path, so stalled sequences will still time out and stop swallowing later
keystrokes.
🤖 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 `@packages/core/src/input/InputParser.ts`:
- Around line 314-336: Update the escape-sequence tests to match the new 500ms
behavior in InputParser._startEscapeTimeout so they still cover the standalone
Escape path; replace the existing 200ms/250ms timing in the relevant tests with
500ms-safe advances. Also extract the hardcoded 500 in _startEscapeTimeout into
a shared ESCAPE_TIMEOUT_MS constant (ideally reused with the paste timeout) so
the timeout value stays consistent across implementation and tests.

---

Outside diff comments:
In `@packages/core/src/input/InputParser.ts`:
- Around line 203-207: The escape-sequence handling in
InputParser._tryParseEscape can leave _escapeBuffer set without starting the
escape timeout when the first chunk begins with \x1b and already contains more
bytes. Update the early escape branch in InputParser to schedule
_startEscapeTimeout() whenever _escapeBuffer is populated from that path, so
stalled sequences will still time out and stop swallowing later keystrokes.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ef502fc8-bdbf-45bf-a39a-889bd73227bf

📥 Commits

Reviewing files that changed from the base of the PR and between 5f479e6 and 3833157.

📒 Files selected for processing (1)
  • packages/core/src/input/InputParser.ts

Comment thread packages/core/src/input/InputParser.ts
@github-actions github-actions Bot added the type:testing +10 pts. Tests. label Jul 3, 2026
@ionfwsrijan ionfwsrijan force-pushed the fix/1957-inputparser-tcp-escape-breakage branch from d119a7f to 92bd707 Compare July 3, 2026 10:23
@ionfwsrijan

Copy link
Copy Markdown
Contributor Author

@Karanjot786 Please review this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core @termuijs/core type:bug +10 pts. Bug fix. type:testing +10 pts. Tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Escape sequence split across TCP chunks causes arrow keys to be misinterpreted as literal characters

1 participant