Repository navigation
fix(terminal): prevent resize from bricking the parser mid inline-image decode - #5352
Merged
Merged
Conversation
…ge decode A resize landing while an inline image (iTerm OSC 1337 / Kitty) is decoding threw 'improper continuation due to previous async handler, giving up parsing' and left the xterm parser permanently FAILed, bricking the terminal. Root cause: the image addon's inline-image handlers decode via createImageBitmap(...).then(...), an async parser handler that keeps the parser paused. fit() -> terminal.resize() -> WriteBuffer.flushSync() re-enters the parser synchronously, which is illegal while a handler is paused. Gate the fit on parser idleness: wrap the terminal's write() once at creation to count in-flight writes (xterm fires a write's callback only after its chunk and async handlers fully parse), and run the resize immediately when nothing is pending or park it until the count drops to zero.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds parser-idle coordination for terminal writes, defers resize and refit work until pending parser activity drains, and updates appearance-change paths to use the gated resize flow. ChangesParser-idle resize coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit 00db74f on Jul 4, 2026 5:50pm UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/desktop/src/renderer/lib/terminal/parser-idle-gate.ts`:
- Around line 57-71: The write gate in wrapWrite is being reopened before the
wrapped callback finishes, which lets runWhenParserIdle() execute re-entrantly
inside xterm’s write callback chain. Update wrapWrite in parser-idle-gate so
pending stays closed until after callback?.() unwinds, and only then schedule
flushQueued via queueMicrotask when pending reaches zero and queued work exists.
Use the existing wrapWrite and flushQueued symbols to keep the resize flush
fenced behind the callback.
🪄 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
Run ID: b48ca321-4b36-468e-9751-0ff0f0e20cc2
📒 Files selected for processing (4)
apps/desktop/src/renderer/lib/terminal/parser-idle-gate.test.tsapps/desktop/src/renderer/lib/terminal/parser-idle-gate.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Contributor
Greptile SummaryFixes terminal-bricking crashes caused by
Confidence Score: 4/5The core gate logic is correct and the bricking crash is genuinely fixed; the two findings are a broken type contract in font-settle.ts and a missing edge-case test, neither of which affects the crash path. The write-counting mechanism and microtask deferral are sound: the pending counter tracks all external writes, flushQueued guards against snuck-in writes, and coalescing is intentional. The scheduleFontSettleRefit refit callbacks now always return false and drop the 4th-argument channel, leaving font-settle.ts with a misleading boolean return type and a silenced onDimensionsChanged parameter. The test suite covers the main scenarios well but has no case for a write that arrives inside a settling callback. font-settle.ts (not in the diff) should have its refit: () => boolean signature updated to () => void; parser-idle-gate.test.ts would benefit from a test for the write-arrives-between-gate-clear-and-microtask-flush path.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/parser-idle-gate.ts | New module implementing write-counting gate and deferred-resize scheduling; logic is correct, microtask deferral guards against re-entrancy, and the coalescing behavior is intentional and well-documented. |
| apps/desktop/src/renderer/lib/terminal/parser-idle-gate.test.ts | Good coverage of the happy-path scenarios; missing a test for a write that arrives synchronously in a settling callback (between gate-clear and microtask flush), which is the trickiest edge case the gate is designed to handle. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Gate installed correctly before any write; measureAndResize now void + internally gates; scheduleFontSettleRefit refit callbacks always return false and no 4th arg is passed, leaving font-settle.ts onDimensionsChanged mechanism vestigial. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts | Removes the now-redundant synchronous post-fit dimension check; the onResize callback passed into updateRuntimeAppearance correctly handles deferred notification. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App as React Mount
participant RT as terminal-runtime
participant Gate as ParserIdleGate
participant XTerm as xterm Terminal
participant Img as ImageAddon (async)
participant Fit as FitAddon
App->>RT: createRuntime()
RT->>Gate: createParserIdleGate()
RT->>XTerm: "terminal.write = wrapWrite(gate, write)"
App->>RT: restoreBuffer (write inline image data)
RT->>Gate: "gate.pending++ (=1)"
RT->>XTerm: write(imageData, wrappedCb)
XTerm->>Img: createImageBitmap().then(...) [async, parser paused]
App->>RT: attachToContainer() to measureAndResize()
RT->>Gate: runWhenParserIdle(gate, fitFn)
Note over Gate: pending=1, park fitFn as gate.queued
Img-->>XTerm: bitmap decoded, parser returns to GROUND
XTerm-->>Gate: "wrappedCb fires, gate.pending-- (=0)"
Gate->>Gate: queueMicrotask(flushQueued)
Note over Gate: microtask: pending=0, queued set
Gate->>RT: fitFn() executes
RT->>Fit: fitAddon.fit()
Fit->>XTerm: terminal.resize(cols, rows) [safe: parser idle]
RT-->>App: onResize() sendResize to PTY
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App as React Mount
participant RT as terminal-runtime
participant Gate as ParserIdleGate
participant XTerm as xterm Terminal
participant Img as ImageAddon (async)
participant Fit as FitAddon
App->>RT: createRuntime()
RT->>Gate: createParserIdleGate()
RT->>XTerm: "terminal.write = wrapWrite(gate, write)"
App->>RT: restoreBuffer (write inline image data)
RT->>Gate: "gate.pending++ (=1)"
RT->>XTerm: write(imageData, wrappedCb)
XTerm->>Img: createImageBitmap().then(...) [async, parser paused]
App->>RT: attachToContainer() to measureAndResize()
RT->>Gate: runWhenParserIdle(gate, fitFn)
Note over Gate: pending=1, park fitFn as gate.queued
Img-->>XTerm: bitmap decoded, parser returns to GROUND
XTerm-->>Gate: "wrappedCb fires, gate.pending-- (=0)"
Gate->>Gate: queueMicrotask(flushQueued)
Note over Gate: microtask: pending=0, queued set
Gate->>RT: fitFn() executes
RT->>Fit: fitAddon.fit()
Fit->>XTerm: terminal.resize(cols, rows) [safe: parser idle]
RT-->>App: onResize() sendResize to PTY
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:297-303
**`scheduleFontSettleRefit` `refit` contract is now silently broken**
The `scheduleFontSettleRefit` signature in `font-settle.ts` is `refit: () => boolean` — it uses the return value to decide whether to call the 4th `onDimensionsChanged` argument. Both new call-sites always `return false`, and no 4th argument is passed, so `onDimensionsChanged` is permanently silenced in `scheduleFontSettleRefit`. This works today because `onResize` is now embedded inside the closure, but the contract mismatch means any future caller who passes a real 4th argument will silently receive no callbacks. The `font-settle.ts` signature should be updated to `refit: () => void` to reflect the new usage pattern (and `const changed = refit(); if (changed)…` should be removed).
### Issue 2 of 2
apps/desktop/src/renderer/lib/terminal/parser-idle-gate.test.ts:39-62
**Missing test for write that arrives between gate-clear and microtask flush**
The `flushQueued` guard (`if (gate.pending !== 0) return`) handles the case where a new write starts after the last in-flight write drains but before the queued microtask fires. This path is the most fragile because an xterm `write` arriving synchronously in the settling write's own callback (e.g., a status write triggered by an `onData` handler) would bump `pending` back to 1 just as the microtask is queued. `flushQueued` correctly bails and relies on the new write's own callback to re-trigger, but this code path has no dedicated test. Consider adding a case where `write2` is submitted inside the callback of `write1` to confirm the gate still re-fires once `write2` drains.
Reviews (1): Last reviewed commit: "fix(terminal): prevent resize from brick..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+297
to
+303
| scheduleFontSettleRefit( | ||
| runtime.terminal, | ||
| () => hostIsVisible(runtime.container), | ||
| () => measureAndResize(runtime), | ||
| onResize, | ||
| () => { | ||
| measureAndResize(runtime, onResize); | ||
| return false; | ||
| }, |
Contributor
There was a problem hiding this comment.
scheduleFontSettleRefit refit contract is now silently broken
The scheduleFontSettleRefit signature in font-settle.ts is refit: () => boolean — it uses the return value to decide whether to call the 4th onDimensionsChanged argument. Both new call-sites always return false, and no 4th argument is passed, so onDimensionsChanged is permanently silenced in scheduleFontSettleRefit. This works today because onResize is now embedded inside the closure, but the contract mismatch means any future caller who passes a real 4th argument will silently receive no callbacks. The font-settle.ts signature should be updated to refit: () => void to reflect the new usage pattern (and const changed = refit(); if (changed)… should be removed).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 297-303
Comment:
**`scheduleFontSettleRefit` `refit` contract is now silently broken**
The `scheduleFontSettleRefit` signature in `font-settle.ts` is `refit: () => boolean` — it uses the return value to decide whether to call the 4th `onDimensionsChanged` argument. Both new call-sites always `return false`, and no 4th argument is passed, so `onDimensionsChanged` is permanently silenced in `scheduleFontSettleRefit`. This works today because `onResize` is now embedded inside the closure, but the contract mismatch means any future caller who passes a real 4th argument will silently receive no callbacks. The `font-settle.ts` signature should be updated to `refit: () => void` to reflect the new usage pattern (and `const changed = refit(); if (changed)…` should be removed).
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+39
to
+62
| test("defers until in-flight writes drain (parser back in GROUND)", async () => { | ||
| const gate = createParserIdleGate(); | ||
| const fake = fakeWrite(); | ||
| const write = wrapWrite(gate, fake.raw); | ||
|
|
||
| // An inline-image write is mid-decode: its callback hasn't fired yet, so | ||
| // the parser is paused. Resizing now would re-enter parse and throw. | ||
| write("\x1b]1337;image-data\x07"); | ||
| expect(fake.hasPending()).toBe(true); | ||
|
|
||
| let ran = false; | ||
| runWhenParserIdle(gate, () => { | ||
| ran = true; | ||
| }); | ||
|
|
||
| // Must NOT run while the async write is still pending. | ||
| await flushMicrotasks(); | ||
| expect(ran).toBe(false); | ||
|
|
||
| // Parser drains → the parked work runs (after the settling microtask). | ||
| fake.drain(); | ||
| await flushMicrotasks(); | ||
| expect(ran).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
The flushQueued guard (if (gate.pending !== 0) return) handles the case where a new write starts after the last in-flight write drains but before the queued microtask fires. This path is the most fragile because an xterm write arriving synchronously in the settling write's own callback (e.g., a status write triggered by an onData handler) would bump pending back to 1 just as the microtask is queued. flushQueued correctly bails and relies on the new write's own callback to re-trigger, but this code path has no dedicated test. Consider adding a case where write2 is submitted inside the callback of write1 to confirm the gate still re-fires once write2 drains.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/parser-idle-gate.test.ts
Line: 39-62
Comment:
**Missing test for write that arrives between gate-clear and microtask flush**
The `flushQueued` guard (`if (gate.pending !== 0) return`) handles the case where a new write starts after the last in-flight write drains but before the queued microtask fires. This path is the most fragile because an xterm `write` arriving synchronously in the settling write's own callback (e.g., a status write triggered by an `onData` handler) would bump `pending` back to 1 just as the microtask is queued. `flushQueued` correctly bails and relies on the new write's own callback to re-trigger, but this code path has no dedicated test. Consider adding a case where `write2` is submitted inside the callback of `write1` to confirm the gate still re-fires once `write2` drains.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 4 files
Architecture diagram
sequenceDiagram
participant UI as Terminal UI (React)
participant Runtime as TerminalRuntime
participant Gate as ParserIdleGate
participant Xterm as xterm.js Terminal
participant Parser as EscapeSequenceParser
participant Image as @xterm/addon-image
participant Buffer as WriteBuffer
Note over UI,Buffer: Terminal initialization flow
UI->>Runtime: createRuntime()
Runtime->>Gate: createParserIdleGate()
Runtime->>Xterm: attach to container
Runtime->>Xterm: wrapWrite(gate, terminal.write)
Xterm->>Runtime: write() wrapped
Note over UI,Buffer: Image decode flow (OSC 1337 / Kitty APC)
UI->>Xterm: write(imageData)
Xterm->>Gate: pending++
Xterm->>Buffer: buffer image data
Buffer->>Parser: parse()
Parser->>Image: async handler (createImageBitmap)
Image-->>Parser: decode in progress (paused)
Note over UI,Buffer: Resize attempt during decode
UI->>Runtime: measureAndResize()
Runtime->>Gate: runWhenParserIdle(fn)
alt pending > 0 (parser busy)
Gate->>Gate: queue fn (latest-wins)
Gate-->>Runtime: deferred
else pending === 0 (parser idle)
Gate->>Gate: run fn immediately
Gate-->>Runtime: synchronous resize
end
Note over UI,Buffer: Async decode completes
Image->>Parser: .then() callback
Parser->>Buffer: parsing complete
Buffer->>Xterm: write callback fired
Xterm->>Gate: pending--
alt pending === 0 AND queued fn exists
Gate->>Gate: queueMicrotask(flushQueued)
Gate->>Gate: fn() runs (deferred resize)
Gate->>Runtime: run queued resize
Runtime->>Xterm: fitAddon.fit()
Xterm->>Buffer: flushSync()
Buffer->>Parser: synchronous parse (now safe)
Parser-->>Buffer: complete
Buffer-->>Xterm: done
Xterm-->>Runtime: resize applied
Runtime-->>UI: onResize callback
end
Note over UI,Buffer: Font settle refit flow
UI->>Runtime: scheduleFontSettleRefit()
Runtime->>Gate: runWhenParserIdle()
Gate->>Runtime: deferred or immediate resize
Note over UI,Buffer: Success path – parser never re-entered mid-decode
Note over UI,Buffer: Failure path (pre-fix, no gate)
UI->>Xterm: write(imageData)
Xterm->>Buffer: buffer
Buffer->>Parser: parse (async handler paused)
UI->>Xterm: resize() during decode
Xterm->>Buffer: flushSync()
Buffer->>Parser: re-enter parse() synchronously
alt parser detects improper continuation
Parser-->>Parser: throw error
Parser->>Parser: set state to FAIL
Xterm-->>UI: terminal bricked (all future writes throw)
end
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts`:
- Around line 107-108: The xterm write hook in the Terminal helpers is releasing
the parser gate too early, so resize work can run while inline image decoding is
still in flight. Update the `wrapWrite`/`createParserIdleGate` flow around
`xterm.write` so the gate stays pending until image/OSC async work has fully
completed, and only then allow `runWhenParserIdle` to proceed. Keep the fix
localized to the `xterm.write = wrapWrite(...)` setup and any related gate
completion logic.
🪄 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
Run ID: bb484219-4dc8-45ff-b97a-0511bc481ea9
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Terminals were crashing with:
This isn't a recoverable error: when it throws, xterm sets the parser to
FAIL, so every subsequent write throws too — the terminal is bricked until reload.Root cause
@xterm/addon-imageregisters async parser handlers for inline images. iTerm2 (OSC 1337) and Kitty (APC) decode viacreateImageBitmap(...).then(...), which keeps the escape-sequence parser paused while the bitmap decodes. (SIXEL's handler is synchronous and never triggers this.)During that paused window, any resize →
FitAddon.fit()→terminal.resize(), and xterm'sCoreTerminal.resize()unconditionally callsWriteBuffer.flushSync(), which re-entersparser.parse()synchronously. The parser detects an improper continuation and throws + permanently fails.flushSyncis documented as "unreliable with async parser handlers."The reported trace is the mount path: right after
createRuntimereplays the serialized scrollback (which can contain an inline image), the mount effect firesupdateAppearance→ fit while that image is still decoding.Fix
Never resize while a write is still being parsed.
parser-idle-gate.ts(new, xterm-free pure logic):wrapWritedecorates the terminal'swriteto count in-flight writes — xterm fires a write's callback only after that chunk and its async handlers fully parse, sopending === 0means the parser is back inGROUND.runWhenParserIdleruns the resize immediately when nothing is pending, or parks it (latest-wins) and flushes from the settling write's callback on a microtask, so the eventualflushSyncnever runs while a handler is paused — nor re-entrantly inside xterm's own write loop.terminal-runtime.ts: install the wrap once increateRuntime(before any write), so every write — live firehose, buffer restore, status notices — is counted and nothing can bypass it.measureAndResizegates the fit on the runtime's gate.terminal-runtime-registry.ts: drop the now-redundant post-fit dimension check (dimensions can settle asynchronously; theonResizecallback covers it).helpers.ts,v1-terminal-cache.ts,useTerminalLifecycle.ts): same gating for the legacy terminal, which loads the sameImageAddon— wrapxterm.writeat creation, route every fit (attach, ResizeObserver, font-settle, appearance, post-attach backend sync) through the gate, and cancel parked work on detach/dispose.updateAppearancenow reports dimension changes via callback instead of a synchronous return, since a gated fit can run after the call returns.Full inline-image support (iTerm2/Kitty/SIXEL) is retained.
Testing
parser-idle-gate.test.ts: runs-when-idle, defers-until-drain, runs-once, coalesces, preserves caller callback.bun run typecheckand Biome clean.Summary by cubic
Prevents terminal crashes when a resize lands during inline image decoding by deferring fits until the parser is idle. This removes “improper continuation” errors and keeps inline images (iTerm2/Kitty/SIXEL) working.
terminal.writeto count in‑flight parses from@xterm/addon-imageand deferfit()until write callbacks unwind (latest‑wins, microtask flush, re‑park when a new write arrives before the flush).measureAndResizeand the v1 cache; only callonResizewhen size changes, preserve scroll position; remove redundant post‑fit dimension check in the registry and the extra host‑visibility guard beforemeasureAndResize; drop dead v1lastCols/lastRows; font‑settle refits now report via the gated callback.Written for commit 00db74f. Summary will update on new commits.
Summary by CodeRabbit