Skip to content

core: add stdin tokenizer/router - #770

Merged
kommander merged 28 commits into
anomalyco:mainfrom
simonklee:stdin-unified
Mar 9, 2026
Merged

kommander merged 28 commits into
anomalyco:mainfrom
simonklee:stdin-unified

Conversation

@simonklee

@simonklee simonklee commented Mar 2, 2026 •

Copy link
Copy Markdown
Member

Fixes #748 + related #705, #682, #727, #686, OpenCode 15760

We currently have a chunk-level either/or stdin split in CliRenderer:

  • if chunk looks like mouse, route mouse and return early
  • otherwise send the whole chunk to generic stdin buffering
    That design is unsafe for mixed chunks and can silently drop input (e.g. mouse + key in same read).
    Examples:
  • \x1b[<64;10;5Mx -> trailing x can be dropped
  • x\x1b[<64;10;5M -> mouse can be swallowed by non-mouse path

I want to have one canonical stdin pipeline, but i tried to add this a merge-safe way, which is why this PR is a bit larger and more of a refactor than just a straight parser swap.

main uses a split stdin pipeline:

  • raw bytes are decoded to UTF-8 too early, before the parser stack can understand the protocol
  • mouse handling is pre-filtered before buffering
  • KeyHandler reparses strings the parser stack already understands

This makes input harder to reason about and can lose details for mixed protocol chunks, X10 mouse payload bytes, delayed escape continuations, and legacy high-byte/meta cases. For instance, issue 661, i now realize is most likely a case of utf8 decoding happening on X10 mouse bytes

The boundary is:

  • StdinParser owns byte framing and protocol recognition. It calls parseKeypress() and MouseParser internally and creates fully typed StdinEvent objects (key, mouse, paste, response).
  • KeyHandler owns event dispatch and turns ParsedKey into KeyEvent. It is not a parser.
  • Renderer owns application routing only, switching on event.type and forwarding to the right handler.

This removes:

  • StdinBuffer string tokenizer and its even emitter
  • setEncoding("utf8") on stdin (decoded strings before framing, wrong order for a binary protocol)
  • MouseParser instance in renderer that consumed mouse bytes before they ever reached the buffer
  • parseKeypress() call inside KeyHandler.processInput()
  • Paste-size coupling to the parser's main byte buffer

Also:

  • A Clock interface (I used a similar abstraction in my last Go project) to manually advance time in tests without relying on wall clock time. I only used this in the relevant tests, but if we like this we can expand it to other tests that use timers. Side-effect is that it makes timing tests quick and deterministic, which is a nice bonus.
  • In main we're not testing the stdin ReadStream directly AFAIK, which is why the setEncoding issues have gone unnoticed, i think. I haven't addressed this yet. - Added many tests to the parser :)
  • Its a large PR, but the lines added is deceptive. The new parser is a larger because we're operating on bytes, but other things are mostly glue + tests + comments.

In one sentence: main decodes first, parses later. In the PR we parse bytes first, decode only when semantics are known.

@pkg-pr-new

pkg-pr-new Bot commented Mar 2, 2026 •

Copy link
Copy Markdown
@opentui/core

npm i https://pkg.pr.new/@opentui/core@ea7a053

@opentui/react

npm i https://pkg.pr.new/@opentui/react@ea7a053

@opentui/solid

npm i https://pkg.pr.new/@opentui/solid@ea7a053

@opentui/core-darwin-arm64

npm i https://pkg.pr.new/@opentui/core-darwin-arm64@ea7a053

@opentui/core-darwin-x64

npm i https://pkg.pr.new/@opentui/core-darwin-x64@ea7a053

@opentui/core-linux-arm64

npm i https://pkg.pr.new/@opentui/core-linux-arm64@ea7a053

@opentui/core-linux-x64

npm i https://pkg.pr.new/@opentui/core-linux-x64@ea7a053

@opentui/core-win32-arm64

npm i https://pkg.pr.new/@opentui/core-win32-arm64@ea7a053

@opentui/core-win32-x64

npm i https://pkg.pr.new/@opentui/core-win32-x64@ea7a053

commit: ea7a053

@simonklee
simonklee marked this pull request as ready for review March 3, 2026 06:16
@simonklee
simonklee marked this pull request as draft March 5, 2026 19:39
@simonklee
simonklee force-pushed the stdin-unified branch 3 times, most recently from 22ad217 to e14b29e Compare March 7, 2026 08:00
@simonklee
simonklee marked this pull request as ready for review March 8, 2026 08:50
Comment thread packages/core/src/lib/parse.keypress.ts Resolved
Comment thread packages/core/src/lib/parse.keypress.ts Resolved
simonklee added 12 commits March 8, 2026 10:41
Parse raw terminal input into typed StdinEvent objects (key,
mouse, paste, response) via a byte-oriented state machine. The
parser owns key and mouse decoding directly — the renderer
routes pre-parsed events without payload decoding or reparsing.

Stream bracketed paste through a collector that keeps only a
small tail for end-marker detection, so arbitrary paste sizes
don't grow the parser buffer.
Only recover [<...M/m as a delayed escape sequence immediately after a timed-out lone ESC, so literal [< input is not
misclassified.
Keep ESC b and ESC f as literal Meta chords instead of rewriting them globally,
and reset parser mouse button state when mouse mode toggles or the renderer
resizes. Also recover delayed X10 mouse continuations after a timed-out ESC so
chunk boundaries do not leak mouse bytes as keypresses.
@kommander
kommander merged commit ab03f41 into anomalyco:main Mar 9, 2026
8 checks passed
simonklee pushed a commit that referenced this pull request Mar 23, 2026
…ation (#858)

## Problem

`resume()` calls `stdin.resume()` then defers listener registration into
a `setImmediate`:

```ts
this.stdin.resume()
this.addExitListeners()
setImmediate(() => {
  while (this.stdin.read() !== null) {}
  this.stdin.on("data", this.stdinListener)
})
```

The stream starts flowing immediately, but `stdinListener` isn't
attached until the next macrotask. Any keystrokes arriving in that gap
are silently lost — which is exactly the window where a user types after
`fg`.

`setupInput()` has a milder variant of the same bug: `resume()` before
`on("data")`.

The drain loop was introduced in #331 to discard stale input from child
process suspension. The placement made it ineffective: `read()` returns
null in flowing mode because data goes through events, not pull. Stale
bytes were only "discarded" by accident — emitted as `data` events with
no listener present.

**Repro:** in the test suite, `mockInput.pressKey()` right after
`renderer.resume()` is silently dropped. The existing tests masked this
by yielding a macrotask (`await new Promise(r => setImmediate(r))`)
before asserting.

## Fix

Reorder `resume()` to: drain → register listener → resume stream:

```ts
while (this.stdin.read() !== null) {}
this.stdin.on("data", this.stdinListener)
this.stdin.resume()
```

The ordering is load-bearing:

1. **Drain first** — `read()` pulls from the internal buffer in paused
mode, discarding stale input (#331's scenario).
2. **Register listener second** — must come after the drain. Adding a
`data` listener can auto-resume a paused Readable, flushing stale bytes
through the listener before the drain runs.
3. **Resume last** — starts flowing mode. New data hits the registered
listener with zero gap.

`setupInput()` just swaps the two lines (no drain needed at init).

## Tests

**Modified:** removed three `setImmediate` yield workarounds (2 in
`renderer.control`, 1 in `renderer.input`). The tests now assert
synchronous delivery — they'd fail if the fix were wrong.

**New (4):**
- "keystrokes received immediately after resume() without yielding" —
core invariant.
- "keystrokes survive multiple rapid suspend/resume cycles" — 5 cycles,
no accumulation.
- "input buffered during suspension is drained on resume" — `push()`
bytes into stdin during suspension, verifies they're discarded. Directly
tests the #331 scenario.
- "suspend/resume does not leak stdin listeners" — 10 cycles,
`listenerCount("data")` stays constant.

## Background

I hit this in 0.1.88 while building a TUI on OpenTUI and patched the
bundled JS. When 0.1.90 shipped, `setupInput()` had its `setImmediate`
removed but the ordering was still wrong, and `resume()` still defers
via `setImmediate`. I've been running a patched build since.

The `setImmediate` + drain pattern was introduced in #770 (building on
#331). The intent was sound — this fix keeps the drain but moves it to
where `read()` actually works.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

High-frequency scroll events cause input starvation and dropped keyboard data

2 participants