fix: don't timeout-flush unambiguous escape sequences split across reads#819
fix: don't timeout-flush unambiguous escape sequences split across reads#819
Conversation
@opentui/core
@opentui/react
@opentui/solid
@opentui/core-darwin-arm64
@opentui/core-darwin-x64
@opentui/core-linux-arm64
@opentui/core-linux-x64
@opentui/core-win32-arm64
@opentui/core-win32-x64
commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #818 where CSI (and similar) escape sequences split across two stdin reads were being prematurely flushed as "unknown" responses due to the 10ms timeout. The timeout exists to detect lone ESC keypresses, but once a framing byte is seen (e.g., ESC[ for CSI), the sequence is unambiguous and should wait for completion.
Changes:
- Remove
forceFlushhandling fromss3,csi,osc,dcs,apc,esc_less_mouse, andesc_less_x10_mousestates — these now always callmarkPending()and return when data runs out - Retain
forceFlushhandling inescandesc_recoverystates where a lone ESC is genuinely ambiguous - Add inline comments explaining why each state no longer honors timeout-flushing
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ESC[ already seen — unambiguously CSI. Wait for final byte | ||
| // or interruption by a new ESC (handled below). Don't | ||
| // timeout-flush; split sequences across reads are common on | ||
| // Windows Terminal with kitty keyboard. | ||
| this.markPending() | ||
| return |
There was a problem hiding this comment.
The maxPendingBytes overflow (64MB) is the hard safety net, but in practice any new input breaks it out — either a new ESC interrupts at line 759, or any byte in the final-byte range (0x40-0x7E) terminates the CSI. A secondary timeout would just reintroduce the race for a scenario that doesn't happen with real terminals.
| if (this.cursor >= bytes.length) { | ||
| if (!this.forceFlush) { | ||
| this.markPending() | ||
| return | ||
| } | ||
|
|
||
| this.emitOpaqueResponse("unknown", bytes.subarray(this.unitStart, this.cursor)) | ||
| this.state = { tag: "ground" } | ||
| this.consumePrefix(this.cursor) | ||
| continue | ||
| // ESC[ already seen — unambiguously CSI. Wait for final byte | ||
| // or interruption by a new ESC (handled below). Don't | ||
| // timeout-flush; split sequences across reads are common on | ||
| // Windows Terminal with kitty keyboard. | ||
| this.markPending() | ||
| return |
e1521b2 to
71b5fc8
Compare
|
I agree with the csi part, but I don't think its true for osc / dcs / apc (or ss3).
I think we can:
I can also try to clean this up for you tomorrow 😉 |
|
thanks @simonklee I've in theory actioned what you said if you can take another look |
Thanks — i'm looking at this, but it's a bit of a rabbit hole. I'll try to clean up my patch later tonight. |
Keep split CSI sequences resumable across timeout while flushing unsafe framed escape states. Also stop timed-out pending CSI from re-arming the parser wakeup loop until new bytes arrive.
Timed-out generic CSI prefixes are ambiguous, but reply families can be safely deferred when their protocol windows are known to be active. Add parser-level protocol context and explicit CSI sub-states so timeout deferral is driven by byte state + negotiated protocol state. Keep generic CSI conservative (flush on timeout) to prevent stale-prefix input capture, allow deferred reassembly for kitty keyboard, SGR mouse, private capability replies, pixel resolution responses, and explicit-width CPR only.
1fba2e7 to
43ce950
Compare
I'm going to have another look and do some more interactive testing with different setups before merging this tomorrow. |
…o#819) Timed-out generic CSI prefixes are ambiguous, but reply families can be safely deferred when their protocol windows are known to be active. Add parser-level protocol context and explicit CSI sub-states so timeout deferral is driven by byte state + negotiated protocol state. Keep generic CSI conservative (flush on timeout) to prevent stale-prefix input capture, allow deferred reassembly for kitty keyboard, SGR mouse, private capability replies, pixel resolution responses, and explicit-width CPR only. Fixes anomalyco#818 --------- Co-authored-by: Simon Klee <hello@simonklee.dk>
Summary
escandesc_recoverystates still honorforceFlushsince a lone ESC is genuinely ambiguousFixes #818