Fluid full-screen animated TUIs: drop superseded frames to keep 60 fps and a responsive keyboard - #2489
Fluid full-screen animated TUIs: drop superseded frames to keep 60 fps and a responsive keyboard#2489s-celles wants to merge 36 commits into
Conversation
A queue item completes only when its write closure invokes the callback it is handed, and in the session write path that callback is wired to xterm's term.write(data, cb). When xterm accepts a write, parses it (its buffer drains to empty), yet never fires cb — observed against a full-screen DEC 2026 TUI on macOS, window visible and focused — the item never completes: queue.active stays set, queue.writing stays true, and every item behind it is stranded with nothing scheduled to resume it. Measured during a live freeze (writes issued vs callbacks fired instrumented on the term): term.write issued 103, callbacks fired 102 -> one callback lost xterm: _pendingData 0, _writeBuffer 0, _callbacks 0, not disposed queue: writing true, active present, no stepTimer/continuation/drainTimer 18 items / ~147 KB stranded behind it This is where the permanent freeze comes from. The completion callback is also where flow.written() and the IPC ack live, so a lost callback leaks the flow-control backlog: it never drains below the low watermark, the main process pauses the PTY at the high watermark and never resumes, and the TUI's own writes then block — its render loop stalls and its keyboard goes dead. The existing large-write watchdog does not help: it only flushes xterm's write buffer, which is already empty. Add a stall watchdog to the write queue. It fires only when the same item is still active, has made no progress since it was armed, and has nothing scheduled to advance it — a genuine wedge, never a slow-but-progressing drain. On fire it marks the item cancelled (so a late real callback is a no-op), acknowledges its unacknowledged bytes through the drop handler so flow control resumes, and advances the queue. A per-item progress sequence re-arms it across the steps of a multi-step item, so a stall on a later step is covered as well as the first. Timeout is 250ms, aligned with LARGE_WRITE_FLUSH_WATCHDOG_MS: above any legitimate single-write callback latency, short enough that a recovered stall reads as a brief hitch rather than a freeze. This is a safety net against a lost xterm.js write callback, not a cure for the loss itself; it converts a permanent freeze with a dead keyboard into a recoverable hitch. Assisted by AI.
It was hardcoded to false in xtermPerformance.ts with no way to change it, while rendererType and minimumContrastRatio are both exposed. It matters for content that varies the background per cell — animated backgrounds, heatmaps, ANSI art. The WebGL glyph cache is keyed on the background colour, so an opaque atlas bakes the background into every tile and each glyph is re-rasterised for every background it is drawn over. Measured against a real animated-background stream at 200x55: 114,496 rasterisations and 28 atlas pages, 65 ms per frame of glyph rasterisation alone. Defaults to false, so behaviour is unchanged unless the user opts in. Assisted by AI.
splitPendingMarkerSuffix looked for the LONGEST suffix accepted by isIncompleteEscapePrefix, but that predicate walks forward across complete sequences and reports the incomplete one it eventually reaches. It therefore answers true for every longer suffix that merely contains the incomplete tail — including the one starting at the chunk's first ESC. On escape-dense output the first ESC sits within a few bytes of the chunk start, so virtually the whole chunk was withheld as "pending incomplete escape", and it accumulated across chunks until one happened to end on a clean boundary. Measured on 12 MB of real TUI output in 64 KB chunks: 121 of 187 chunks emitted nothing, up to 14 consecutive chunks emitted nothing, state.pending reached 917 KB, and xterm then received a single 949 KB burst. Scanning shortest-suffix-first instead gives 0 empty emits, a maximum pending of 18 bytes, and byte-identical output. The last ESC in the chunk is the real split point: an incomplete CSI cannot contain a further ESC. Assisted by AI.
The write coalescer's slicer recognises alt-screen DECSET toggles (47, 1047, 1049) as frame boundaries but is blind to DEC 2026 synchronized output. A modern full-screen TUI enters the alternate screen once, then delimits every frame with `\x1b[?2026h` … `\x1b[?2026l`, so the slicer sees no boundary after startup and cuts the continuous frame stream by byte size — landing shard boundaries inside frames. That tears the display: a shard is handed to xterm across a setTimeout gap, and if the rest of the frame arrives after xterm's 1000ms synchronized-output timeout, xterm force-flushes a partial frame. Add a frame-boundary helper (syncFrameBoundary.ts) and apply it as the last step of slice-end selection: a cut that would fall inside an open 2026 block is pushed forward to just past its close, and an unterminated trailing frame is held whole for the next write. It only ever extends a slice, so it composes with the existing boundary rules. Assisted by AI.
Adds scripts/patch-xterm-sync-render.cjs (run from postinstall, after the existing webgl atlas patch) to render a synchronized-output frame the moment it closes instead of on the next debounced tick. xterm buffers rows while DEC 2026 synchronized output is on and, on close, schedules the paint through the render debouncer. Under a continuous full-screen animation the next frame opens a new 2026 block before that rAF fires, and `_renderRows` skips while sync is on, so the paint is dropped and the frame only appears on the 1000ms sync timeout — pinning the display at ~1fps. The patch renders synchronously when a sync buffer was just flushed, so a completed frame paints before the next can reopen the mode. Measured against a 30fps animated-background TUI: ~1fps to the frame arrival rate, coherent (no partial-frame tearing). Idempotent and marker-guarded, like patch-xterm-webgl-atlas.cjs; a version bump that moves the minified target fails the install rather than silently losing the fix. Upstreamable to xterm.js. Assisted by AI.
A full-screen animated TUI (e.g. TryIt.jl at 60 fps) ran at only ~24 fps in a local terminal because the SSH-tuned 1 MB flow-control watermark paused the PTY several times a second. A local shell has no network to overwhelm and its source blocks on its own write when the pipe fills, so the tight watermark only throttles it. Add LOCAL_FLOW_HIGH/LOW_WATER_MARK (32 MB / 16 MB) and apply them to `protocol === "local"` sessions on both sides of the flow-control loop: the renderer's output flow controller and the main-process PTY pause/ resume in terminalFlowAck. SSH and every other protocol keep the tight default, so multi-MB remote dumps stay bounded. The raised ceiling still caps memory — it relaxes back-pressure, it does not disable it. Measured: TryIt.jl at TRY_FPS=60 now paints a fluid 59.9 fps in a local Netcatty terminal (was ~24). Assisted by AI.
A full-screen animated TUI (e.g. TryIt.jl at 60 fps) emits ~140 KB per frame, each a DEC 2026 synchronized block that homes the cursor and repaints every cell. xterm.js can render that rate, but only if it never falls more than a frame or two behind; otherwise frames queue up and the display — and the keyboard echo waiting behind it — runs up to a second late. Raising the flow-control watermark bought the frame rate at the cost of that latency; lowering it fixed the latency but throttled the animation by pausing the source. Add a frame gate on the renderer's session-data path that breaks the trade-off. When output backlog is below a few frames it forwards frames straight through, so xterm stays fed at full rate. Above that it collapses runs of superseded full-repaint frames to the last — the next frame repaints every cell the previous drew, so the earlier ones are invisible — and acknowledges the dropped bytes so flow control stays balanced. The source is never paused, so the animation keeps 60 fps while the backlog (and latency) stays bounded. Collapsing is strict: only pure visual repaints whose successor homes the cursor are dropped, never frames carrying OSC or private-mode / alternate-screen state. With the local watermark lowered to 4 MB (backstop only; the gate keeps the real backlog near 0.5 MB), TryIt.jl at TRY_FPS=60 now renders a fluid ~60 fps with ~50 ms input-to-display latency, down from ~800 ms. Assisted by AI.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b30766cdd3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (complete) { | ||
| const backlog = getFlowControllerForTerm(term)?.pendingBytes() ?? 0; | ||
| if (backlog < FRAME_GATE_FORWARD_BACKLOG) { | ||
| forwardSessionData(ctx, term, complete, complete.length, state.meta); |
There was a problem hiding this comment.
When plugin processing expands or contracts a DEC 2026 chunk, ingressBytes deliberately differs from the rendered string length, but the gate discards that accounting and forwards complete.length; it also acknowledges collapsed output using the dropped character count. Such transformed frames therefore over- or under-ack the backend, which can prematurely resume output or leave the session paused at the high-water mark. Retain and apportion the original ingress units while buffering and dropping frames.
Useful? React with 👍 / 👎.
| if ( | ||
| next.start === cur.end | ||
| && isPureVisualPayload(cur.content) | ||
| && startsWithCursorHome(next.content) | ||
| ) { |
There was a problem hiding this comment.
DEC 2026 only makes updates atomic; it does not imply that a frame repaints every cell, and homing the cursor does not establish that either. Under backlog, a valid incremental successor such as HOME + one changed cell satisfies this condition, causing the preceding frame's changes elsewhere on screen to be silently discarded. Only collapse frames when the successor demonstrably clears or overwrites the complete viewport.
Useful? React with 👍 / 👎.
| const unacked = unackedBytesOf(item); | ||
| // Mark cancelled so a late real callback becomes a no-op, then advance. | ||
| item.cancelled = true; | ||
| queue.active = undefined; | ||
| queue.drainBytes = 0; | ||
| if (unacked > 0) { | ||
| queue.onDropped?.(unacked); | ||
| } | ||
| scheduleQueueDrain(term, queue, true); |
There was a problem hiding this comment.
If an xterm write callback is delayed beyond 250 ms on a slow machine or during a large parse, this treats an in-progress write as dropped, acknowledges its bytes, and starts subsequent writes while the original remains active inside xterm. The queue cannot distinguish that legitimate latency from a permanently lost callback, so this can defeat backpressure and skip the original item's completion bookkeeping during exactly the heavy-output scenario being optimized. Recovery needs a signal that xterm has actually drained or a substantially safer lifecycle mechanism rather than elapsed time alone.
Useful? React with 👍 / 👎.
| if (state.buffer && state.flushTimer === undefined) { | ||
| state.flushTimer = setTimeout(() => { | ||
| state.flushTimer = undefined; | ||
| drainFrameGate(ctx, term); | ||
| }, FRAME_GATE_FLUSH_MS); |
There was a problem hiding this comment.
When state.buffer contains only an unterminated DEC 2026 frame, each retry produces the same partial value and unconditionally schedules another retry eight milliseconds later. No timer can complete that frame—only new session data can—so a stalled or disconnected session with a partial frame generates roughly 125 wakeups per second indefinitely while its tab remains mounted. Schedule polling only for complete output held by backlog, since new arrivals already invoke drainFrameGate themselves.
Useful? React with 👍 / 👎.
Address automated review feedback on the frame gate: - Require proof of a full repaint before dropping a frame. Homing the cursor alone does not imply a full-screen repaint — a valid incremental successor (HOME + one changed cell) would otherwise discard the earlier frame's other changes. Also require the successor's payload to reach a viewport-sized threshold (cols * rows, one byte per cell), which a real repaint always clears and an incremental update never does. - Preserve flow-control ingress accounting through the gate. Track the ingress bytes attributable to the held buffer and apportion them exactly (via complements) across forwarded / dropped / held, instead of substituting rendered-string lengths — so a plugin-transformed chunk cannot over- or under-acknowledge the backend. - Stop busy-polling a lone trailing partial frame. Only schedule the retry timer when complete output is being held back by backlog; a partial can only be completed by new session data, which drains the gate itself. A session stalled mid-frame no longer wakes ~125x/second. Adds unit coverage for the size threshold and the ingress apportioning (sum-invariant, including ingress != length). Assisted-by: AI
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44a74dfcb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state.buffer = partial; | ||
| state.ingress = ingressPartial; |
There was a problem hiding this comment.
When a DEC 2026 opener is not followed promptly by its closer, partial is retained without being forwarded, acknowledged, timed out, or size-limited. A process interrupted or killed mid-frame therefore causes its prompt and all subsequent output to remain invisible indefinitely; additionally, an SSH frame larger than the 1 MiB flow watermark can deadlock because the backend pauses before the withheld closer can arrive. Add a timeout/size-based fail-open path that releases incomplete frames to xterm.
Useful? React with 👍 / 👎.
| && isPureVisualPayload(cur.content) | ||
| && startsWithCursorHome(next.content) | ||
| && next.content.length >= minSuccessorRepaintBytes |
There was a problem hiding this comment.
When backlog collapse runs, next.content.length >= cols * rows does not demonstrate a full-screen repaint because escape sequences are included in that length. For example, on an 80x24 terminal, updating only 100 cells with per-cell truecolor SGR sequences can exceed 1,920 characters while leaving most cells untouched; the preceding frame is then dropped and those untouched cells remain stale. Count actual cell writes/coverage or require a semantic clear/full repaint instead of raw payload length.
Useful? React with 👍 / 👎.
| const isPureVisualPayload = (content: string): boolean => | ||
| !content.includes("\x1b]") && !content.includes("\x1b[?"); |
There was a problem hiding this comment.
When a frame is superseded under backlog, this blacklist classifies every payload without OSC or private CSI as purely visual, even though it can contain BEL, DCS/APC commands, terminal queries, and other non-visual side effects. A synchronized frame carrying a bell or device-control request can therefore be dropped before xterm processes it, losing the notification or response. Parse an allowlist of cursor, SGR, erase, and cell-output operations rather than excluding only two prefixes.
Useful? React with 👍 / 👎.
Second round of review follow-ups on the animation frame gate: - Prove a full repaint by cell coverage, not payload length. The previous `content.length >= cols*rows` check was inflated by SGR escapes, so a small SGR-heavy update could masquerade as a full repaint. Instead simulate the frame's cursor moves, cell writes and erases and count covered cells. Full animated TUIs commonly repaint only the changed cells (TryIt.jl rewrites ~60% of the grid per frame), so the bar sits at 40% of the viewport — well above the few per-cent an incremental update touches, while still recognising real animation frames. Dropping is therefore best-effort for such TUIs (a cell only the dropped frame repainted shows one frame stale, imperceptible in motion); a TUI that emits true full repaints is dropped losslessly. - Restrict droppable payloads to an allowlist (cursor moves, SGR, erase, cell text) instead of only excluding OSC and private CSI. A frame carrying a BEL, device query/report, DCS/APC, or alternate-screen toggle is no longer dropped, so its side effect is never lost. - Fail open held output. A held buffer past a cap (kept below the SSH flow watermark, so a frame larger than it cannot deadlock) is released at once, and a lone trailing partial that never completes is released after a short timeout — so a process killed mid-frame never leaves its prompt hidden. Adds unit coverage for the allowlist, the cell-coverage counter, the coverage-gated collapse, and the exact ingress apportioning. Assisted-by: AI
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c34c6e2822
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| queue.active = undefined; | ||
| queue.drainBytes = 0; | ||
| if (unacked > 0) { | ||
| queue.onDropped?.(unacked); |
There was a problem hiding this comment.
When an xterm callback is delayed beyond 250 ms rather than permanently lost, this handler reports the write as dropped and acknowledges its bytes, but cancelling the queue item does not cancel the in-flight write closure. When the real callback eventually arrives, terminalSessionAttachment.ts still calls flow.written() and sends the IPC acknowledgement before invoking the now-ignored queue callback, so the same bytes are acknowledged twice. Under a slow large write this understates the renderer backlog and can prevent flow control from pausing subsequent output.
Useful? React with 👍 / 👎.
| "A", "B", "C", "D", "E", "F", "G", "H", "f", // cursor moves / positioning | ||
| "d", "`", // line / column position (VPA / HPA) | ||
| "m", // SGR | ||
| "J", "K", // erase in display / line |
There was a problem hiding this comment.
Allowing every J sequence classifies CSI 3 J as a droppable visual erase, although ED3 clears saved scrollback rather than merely repainting viewport pixels. If such a synchronized frame is followed by a frame that passes the repaint threshold, collapseAndSplit removes the ED3 frame entirely, leaving history that the application explicitly requested xterm to erase. Inspect the CSI parameters here and preserve stateful erase variants instead of treating all J commands alike.
Useful? React with 👍 / 👎.
| setTerminalWriteCoalescerFlushGate(term); | ||
| pendingTimestampSecondByTerm.delete(term); | ||
| resetDeferredTerminalWriteAck(term); | ||
| resetFrameGate(term); |
There was a problem hiding this comment.
If hibernation or detachment races with an incomplete DEC 2026 frame, its bytes live only in FrameGateState.buffer; the preceding coalescer/queue flush and flow release do not see them. Deleting the state here therefore loses already-delivered terminal output that was not included in the hibernation snapshot, and its ingress bytes are never acknowledged to the backend. Forward or explicitly drop-and-ack the gate buffer before clearing it.
Useful? React with 👍 / 👎.
| const state = frameGateStates.get(term); | ||
| const engaged = (state && state.buffer.length > 0) || data.includes("\x1b[?2026h"); |
There was a problem hiding this comment.
When the eight-byte DEC 2026 opener is split across two PTY data events, neither event contains the complete string, so both take the direct path and the frame gate never engages for that frame. PTY chunk boundaries are arbitrary, and a consistently aligned animated stream can therefore bypass the frame-dropping behavior this change is intended to provide. Preserve a short possible-opener suffix between calls or use the existing stateful escape parsing rather than relying on a chunk-local includes check.
Useful? React with 👍 / 👎.
…opener) Address further automated review feedback on the animation frame gate: - Never drop a frame carrying ED3. `CSI 3 J` clears the saved scrollback, a side effect a repaint does not restore, so a frame containing it is no longer classified as a droppable visual payload. - Flush the gate before resetting it. `resetFrameGate` now hands any held buffer to the caller before dropping the state: the write-context site forwards it to xterm, the hibernation site acknowledges its ingress. A reset racing an incomplete frame no longer loses buffered output or leaves the backend paused on unacknowledged bytes. - Engage on a split DEC 2026 opener. The gate now also engages when a chunk ends with a proper prefix of `ESC[?2026h`, and `collapseAndSplit` holds that trailing prefix back, so an opener cut across two PTY chunks reunites with the next chunk instead of bypassing the gate. Adds unit coverage for ED3 rejection, the split-opener hold and `endsWithSyncOpenerPrefix`. 736 terminal-runtime tests pass. Assisted-by: AI
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 651c447b84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (unacked > 0) { | ||
| queue.onDropped?.(unacked); |
There was a problem hiding this comment.
When an xterm write callback arrives after the 250 ms watchdog, this onDropped call immediately invokes controller.written and the IPC acknowledgement through the registered drop handler. The original callback is not cancelled, however, and writeSessionDataImmediate subsequently performs the same flow accounting before its queue continuation becomes a no-op. This double acknowledgement can consume bytes belonging to later writes and resume a flooding PTY while output remains queued; make the completion accounting idempotent across watchdog and late-callback paths.
Useful? React with 👍 / 👎.
| // Fired with no intervening drain: the partial is stuck — release it. | ||
| const stuck = getFrameGateState(term); | ||
| if (stuck.buffer) { | ||
| forwardSessionData(ctx, term, stuck.buffer, stuck.ingress, stuck.meta); | ||
| stuck.buffer = ""; |
There was a problem hiding this comment.
If a process exits after opening a DEC 2026 block but before sending its closer, onSessionExit writes the [session closed] line immediately while this fail-open timer remains armed. The timer then forwards the older partial frame 200 ms later, reversing terminal-output order and potentially overwriting or hiding the exit message. Reset or synchronously flush the frame gate before lifecycle lines are enqueued.
Useful? React with 👍 / 👎.
|
@binaricat could you take the lead on this? |
|
Thanks @s-celles — I took the maintainer lead and split the xterm-side synchronized-render fix into #2521 while preserving your authorship on the original implementation commit. #2521 contains only the narrow refresh-timing fix plus deterministic Electron coverage. It intentionally excludes frame dropping, flow-control changes, watchdogs, and settings. During review we also covered frames delivered in one input chunk, redundant close sequences, both xterm builds, install safety, and stale development caches. We are keeping #2489 open for now, but we are not planning to merge it as one bundle. This lets us review and land the safe, upstream-aligned part independently without asking you to keep iterating on the broader PR. Thank you for finding the xterm direction and for the work you already put into this. |
|
I will give it a try to see if, after your fixes, I'm still getting good fps for full TUI window with animated background while keeping inputs (mouse clicks and key pressed events reactives) Thanks @binaricat for your work on this and more broadly on Netcatty. |
- Make write-queue stall recovery safer: only force-complete when xterm's write buffer is idle; re-arm while a parse is still in flight so slow legitimate callbacks are not treated as lost. - Make flow-control ack exclusive between the stall watchdog and the normal write-completion path via tryClaimFlowAck, so a late xterm callback cannot double-ack after watchdog recovery. - Flush held DEC 2026 frame-gate output in writeTerminalLine so session-exit lifecycle lines never race a fail-open partial release. Already addressed earlier on this branch (verified): full-repaint coverage proof, ED3 non-drop, partial fail-open timeout/size cap. Assisted-by: AI
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfca22ff0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else if (final === "d") { row = (n0 ?? 1) - 1; clampRow(); } | ||
| else if (final === "J") { | ||
| const p = n0 ?? 0; | ||
| if (p === 2 || p === 3) { for (let r = 0; r < rows; r++) markRange(r, 0, cols - 1); } |
There was a problem hiding this comment.
When a later synchronized frame contains CSI 3 J, this branch marks every viewport cell, so makesFullRepaint returns true even though ED3 clears saved scrollback rather than repainting the visible grid. Under backlog collapse, collapseAndSplit can then drop the previous visual frame while the successor only clears scrollback, leaving stale or missing frame contents. Treat parameter 3 as non-coverage here, matching the ED3 side-effect handling in isDroppableVisualPayload.
Useful? React with 👍 / 👎.
| continue; | ||
| } | ||
| if (code === 0x7f) return false; // DEL | ||
| i++; // printable / UTF-8 byte → cell text |
There was a problem hiding this comment.
If a synchronized frame uses 8-bit C1 controls such as \x9b CSI or \x9d OSC, they skip the ESC parser and fall through here as printable cell text. xterm accepts those controls, so a frame that changes title, colors, cursor state, or other terminal state via C1 can be classified as droppable and removed before xterm sees the side effect; reject the C1 control range or parse it alongside the ESC forms before allowing frame drops.
Useful? React with 👍 / 👎.
ED3 (CSI 3 J) clears scrollback only, so it must not mark viewport cells for makesFullRepaint — otherwise collapseAndSplit can drop a prior visual frame whose content the successor never repaints. Also reject 8-bit C1 controls (CSI 0x9B, OSC 0x9D, …) in isDroppableVisualPayload so they are not misclassified as printable cell text and skipped before xterm sees their side effects.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75ce231743
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (data.startsWith(SYNC_CLOSE, i)) { | ||
| open = false; |
There was a problem hiding this comment.
When desiredEnd lands inside the closing ESC[?2026l marker, this branch treats the close as already consumed because its start is before to, so frameSafeSliceEnd returns the original cut and the large-write slicer can split the close sequence itself. Under the same delayed setTimeout slicing this helper is meant to avoid, xterm remains in synchronized-output mode until the next shard arrives and can still hit the sync timeout/tear; only toggle open after the whole marker is before to, or explicitly extend cuts that fall inside the close marker.
Useful? React with 👍 / 👎.
| * for animations, not a lossless guarantee — a cell the dropped frame alone | ||
| * repainted shows one frame stale, which is imperceptible in motion. | ||
| */ | ||
| const FULL_REPAINT_COVERAGE = 0.4; |
There was a problem hiding this comment.
Fresh evidence in this revision is that the drop predicate still accepts a successor after only 40% viewport coverage. When a DEC 2026 TUI emits changed-cell frames under backlog, any cell painted by the dropped frame but not by the successor remains at the pre-dropped contents, so the renderer can show stale cells while the backend has already acknowledged the skipped frame; require full coverage or prove the dropped frame's touched cells are covered by the successor before collapsing.
Useful? React with 👍 / 👎.
…epaint Do not slice mid ESC[?2026l when desiredEnd lands inside the close marker. Raise full-repaint drop coverage from 40% to near-full (>=99% or exact full) so incremental successors cannot discard prior frame cells.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91c47c9591
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Exact full coverage, or near-full (>= 99%) so tiny clamp/wrap off-by-ones | ||
| // do not block legitimate full repaints while partial paints still fail. | ||
| if (covered >= total) return true; | ||
| return covered >= Math.ceil(total * FULL_REPAINT_COVERAGE); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the near-full branch still treats less than 100% coverage as a full repaint. When a successor frame covers only 99% of an 80x24 viewport, collapseAndSplit may drop a predecessor that was the only frame to paint the remaining ~19 cells, leaving those cells stale while the skipped bytes have already been acknowledged; require complete coverage or prove that every cell touched by the dropped frame is covered by the successor.
Useful? React with 👍 / 👎.
| droppedChars: number, | ||
| heldChars: number, | ||
| ): FrameGateIngressSplit => { | ||
| const held = totalChars > 0 ? Math.round((total * heldChars) / totalChars) : 0; |
There was a problem hiding this comment.
Fresh evidence in this revision is that the split is still based only on character ratios. When plugin processing makes ingressBytes differ from rendered length and the held partial has a different ingress-per-character ratio than the complete/dropped frames, this proportional held calculation can acknowledge source bytes that are still buffered (or leave already-dropped bytes unacknowledged), so the backend can resume or remain paused at the wrong time; store ingress per buffered segment and split it at the actual frame boundaries instead of re-estimating from total character counts.
Useful? React with 👍 / 👎.
TUIs that emit synchronized output as 8-bit C1 CSI must keep frame boundaries whole the same way as the 7-bit ESC CSI form.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3690e6e408
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // split opener (`ESC[?2026h` cut across PTY chunks) so an aligned stream can | ||
| // never bypass the gate by landing the opener on a chunk boundary. | ||
| const engaged = (state && state.buffer.length > 0) | ||
| || data.includes("\x1b[?2026h") |
There was a problem hiding this comment.
Fresh evidence in this revision is that the frame-gate trigger still only checks for the 7-bit ESC[?2026h opener. When a TUI emits synchronized output using the 8-bit C1 CSI form (0x9B ?2026h/l), which xterm accepts and the new frame helpers parse elsewhere, chunks containing a complete C1 opener take the direct path and never enter the collapse/drop/fail-open gate, so C1 animated streams keep accumulating/pausing instead of being latency-bounded. Include the C1 opener in this engagement check as well.
Useful? React with 👍 / 👎.
Chunks with 8-bit C1 CSI synchronized-output openers now enter the collapse/drop path instead of bypassing it as ordinary output.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16283183a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Preserve ingress for releaseTerminalFlowBeforeHibernate so the backend | ||
| // is not left paused on flushed-but-unacked frame-gate bytes (Codex P2). | ||
| if (ingress > 0) { | ||
| frameGateHibernateFlushedIngress.set( | ||
| term, | ||
| (frameGateHibernateFlushedIngress.get(term) ?? 0) + ingress, | ||
| ); |
There was a problem hiding this comment.
When flushPendingTerminalWritesBeforeHibernate() is used by live context/snapshot/drain paths rather than followed by releaseTerminalFlowBeforeHibernate(), this hook deletes the frame-gate state and stores the ingress only for that later release. Those bytes were never passed through forwardSessionData()/ackTerminalSessionFlow(), so the main process keeps an unacked floor and can leave the session paused after enough such drains; have the flush path ack or return the flushed ingress for all callers, not only hibernation release.
Useful? React with 👍 / 👎.
| const close = data.indexOf(SYNC_CLOSE, end); | ||
| if (close === -1) return data.length; | ||
| return close + SYNC_CLOSE.length; |
There was a problem hiding this comment.
Fresh evidence in this revision is that isInsideSyncBlockAt() now recognizes C1 open/close markers, but frameSafeSliceEnd() still searches only for the 7-bit closer here. For a C1 synchronized frame whose desired cut lands inside the frame, this returns data.length instead of the C1 close, so writeLargeTerminalBatch() sends the entire remaining batch as one slice and loses the cooperative yield/backpressure behavior for long C1 animations; search both closer forms before falling back to the end of the data.
Useful? React with 👍 / 👎.
Search both 7-bit and C1 DEC 2026 closers when extending slices. Keep hibernate flush ingress tracking for later ACK.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741a911ec3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Route through the same scrollback-safe filter as live writes so a | ||
| // held HOME+CSI 2 J full redraw does not yank scrollback on hibernate | ||
| // (Codex P2 on e8c49563). | ||
| const filtered = filterTerminalSessionData(term, buffer); |
There was a problem hiding this comment.
When hibernation drains a frame-gate buffer that ends inside an escape sequence, a split DEC 2026 opener, or a held cursor-home, filterTerminalSessionData() can retain those bytes in its own pending filter state and return "" or only a prefix. This hook then deletes the frame-gate state and later treats the terminal as drained, so the snapshot and ingress ACK can lose bytes that were already delivered; flush/release the sync-block filter's pending output before considering the hibernate drain complete.
Useful? React with 👍 / 👎.
After writing a held frame-gate buffer through the filter, force-release pending ESC suffix/cursor-home so snapshot drains do not drop them.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e92729179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let row = 0; | ||
| let col = 0; |
There was a problem hiding this comment.
When a synchronized successor frame omits an explicit home/clear and only writes a screenful of printable cells, this scanner still starts coverage at row 0/col 0, so makesFullRepaint can return true even though xterm will start from the terminal's real cursor position. In cases where the cursor is not already home (for example after prior prompt output or at the bottom of the normal buffer), collapseAndSplit can drop the preceding frame while the successor wraps/scrolls from the wrong origin and does not overwrite the cells the dropped frame painted; require an explicit origin reset or track the actual cursor before treating printable coverage as a full repaint.
Useful? React with 👍 / 👎.
Do not treat a screenful of cells as a full repaint unless the frame homes/clears first, so coverage is not measured from the wrong origin.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b54464a33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const hasKnownCursorOrigin = (content: string): boolean => { | ||
| if ( | ||
| content.includes("\x1b[2J") | ||
| || content.includes("\x9b2J") | ||
| || content.includes("\x1b[H") |
There was a problem hiding this comment.
Fresh evidence in this revision is that hasKnownCursorOrigin is only a content-wide substring check. When a successor frame paints from the terminal's current cursor and emits CSI H only at the end (for example to park the cursor), this still lets makesFullRepaint simulate the preceding text from row 0/col 0 and drop the previous frame, even though xterm did not paint those cells from the origin. Track the origin reset in the coverage scan and only accept cells counted after a reset that precedes the paint.
Useful? React with 👍 / 👎.
CUP/ED2 must precede counted paints so a trailing CSI H cannot validate cells written from the wrong cursor origin.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d6c89e45e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| termRef.current.options.allowTransparency = | ||
| terminalSettings.allowTransparency; |
There was a problem hiding this comment.
When the user toggles Allow transparency while a terminal is already open, this assignment does not reliably apply the change: xterm documents allowTransparency as an option that must be set before Terminal.open() and cannot be changed later without reopening. The setting is persisted, but the current renderer/glyph atlas can keep using the old transparency mode, so users trying to fix animated-background performance will not see the effect until a new terminal is created; either recreate the terminal or make the UI describe this as new-session only.
Useful? React with 👍 / 👎.
xterm requires allowTransparency before Terminal.open(); runtime assignment does not reconfigure the renderer. Stop applying it on live sessions and document that the setting takes effect on new terminals only.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775c24614c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const p = n0 ?? 0; | ||
| // ED2 establishes a known origin (cursor typically homes) but must NOT | ||
| // count cells as coverage — scrolled-up filter may strip ED2 later. | ||
| if (p === 2) { originKnown = true; } |
There was a problem hiding this comment.
When a successor frame contains CSI 2J without also moving the cursor home, this marks the origin as known while the simulator continues from its assumed cursor position. ED2 clears the display but xterm does not move the cursor, so if the dropped predecessor was the frame that positioned the cursor, the successor's printable cells render from a different location after collapse; only a preceding CUP/home or tracked real cursor state should allow subsequent paints to prove a full repaint.
Useful? React with 👍 / 👎.
CSI 2J clears the display without moving the cursor in xterm. Only CUP/home establishes origin for repaint coverage so we do not drop predecessors when the successor paints from a non-home position after ED2 alone.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e837aa158a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // when the successor omits redundant SGR (Codex P2 on dd606f39). | ||
| if ( | ||
| next.start === cur.end | ||
| && isDroppableVisualPayload(cur.content) |
There was a problem hiding this comment.
When a DEC 2026 block is on the normal buffer and its printable payload writes past the bottom-right cell, xterm's delayed autowrap can scroll the buffer and add history even without \n/CSI S/CSI T. This drop predicate treats that predecessor as a purely visual payload and can remove it once the next frame repaints the viewport, so the visible cells recover but the scrollback mutation from the skipped frame is lost; track/simulate whether the current frame can wrap/scroll or only allow this in a no-scrollback context.
Useful? React with 👍 / 👎.
| } catch { | ||
| console.warn(`[patch-xterm-sync-render] skip (not found): ${file}`); | ||
| missing++; | ||
| continue; |
There was a problem hiding this comment.
If a future @xterm/xterm upgrade already includes this widened sync-render ternary but lacks Netcatty's marker, this marker-only check misses it and the following exact from match falls through to missing++, making postinstall fail instead of no-oping as intended. Treat the unmarked to form as already fixed before reporting an ambiguous/missing target so installs do not break once the upstream patch lands.
Useful? React with 👍 / 👎.
… patch Do not drop DEC 2026 predecessors that can delayed-autowrap past the bottom-right cell (scrollback would be lost). Treat an already-widened sync-render ternary without Netcatty's marker as already patched.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ba81f4295
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { complete, partial, dropped } = collapseAndSplit( | ||
| gate.buffer, |
There was a problem hiding this comment.
When a DEC 2026 synchronized-output chunk reaches writeSessionData, this call throws ReferenceError: gate is not defined before anything is forwarded or acknowledged, so full-screen synchronized TUIs stop rendering as soon as the new frame gate engages. The buffer passed to collapseAndSplit should come from the state created at the top of drainFrameGate.
Useful? React with 👍 / 👎.
collapseAndSplit was called with an undefined `gate` after the viewport argument was added, which threw ReferenceError as soon as DEC 2026 frames engaged. Pass state.buffer from getFrameGateState instead.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Makes full-screen animated TUIs (e.g. TryIt.jl
at 60 fps) render fluidly in a local terminal and keep the keyboard
responsive. The flow-control watermark alone is a pure throughput↔latency dial —
high gives 60 fps with ~800 ms input lag, low gives a responsive keyboard at
~30 fps. This adds a renderer-side frame gate that drops superseded
full-screen frames instead of pausing the source, so the animation keeps full
rate while the backlog (and the latency behind it) stays bounded.
Measured on an M4 MacBook Air with TryIt.jl at
TRY_FPS=60: ~60 fps with~50 ms input-to-display latency, down from ~800 ms, backlog capped near
0.5 MB.
Type of Change
Related Issue
Related to #2487.
Changes Made
terminalFrameGate) — the core fix.When several full-screen full-repaint frames are buffered, only the last is
visible, so the earlier ones are collapsed away and their bytes acknowledged.
The source is never paused. Collapsing is strict: only pure visual repaints
whose successor demonstrably repaints the whole screen (homes the cursor and
writes ≥ 1 byte per cell) are dropped, never frames carrying OSC or
private-mode / alternate-screen state. Flow-control ingress accounting is
apportioned exactly through the gate.
to overwhelm, so it gets a higher backstop watermark; SSH and every other
protocol keep the tight default.
slicer never cuts inside a synchronized-output frame.
watchdog force-completes an item whose
term.writecallback never fires.allowTransparencyas a setting.Happy to split this into smaller PRs (e.g. the frame gate on its own) if you'd
prefer to review it in pieces.
Screenshots / Demo
TryIt.jl at
TRY_FPS=60in a local terminal renders a fluid ~60 fps animatedbackground with immediate arrow-key /
Ctrl+Tresponse (was either laggy at60 fps or throttled to ~30 fps before).
Testing
npm run dev)npm run lint)npm test) — terminal runtime suite green, incl. newterminalFrameGateunit coverage (frame collapse, size threshold, exactingress apportioning)
npm run generate:capability-tools) — N/AChecklist
Assisted-by: AI