Repository navigation
fix(desktop): re-send terminal PTY dims when a hidden pane becomes visible - #5725
Conversation
…sible The v2 resize scheduler already sees both edges of a CSS hide/show (the ResizeObserver reports 0x0 on hide, real dims on reveal) but only sent a resize when the fitted dimensions changed. Track the hide edge and force the notification on reveal even when dims are unchanged — VS Code's setVisible -> _resize parity — so the host session resyncs a PTY that was resized elsewhere (e.g. another window on the same terminal) while this pane was hidden. The re-send is a kernel no-op when both ends already agree (same-size TIOCSWINSZ delivers no SIGWINCH; verified against a real pty), so idle reveals stay silent.
|
Ready to review this PR? Stage has broken it down into 2 individual chapters for you:
Chapters generated by Stage for commit 375f928 on Jul 17, 2026 1:19am UTC. |
📝 WalkthroughWalkthroughTerminal resize handling adds an optional forced-notification flag and enables it for scheduled measurements, allowing ChangesTerminal resize notification flow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR fixes a PTY dimension desync that occurs when a terminal pane hidden via CSS becomes visible again. By latching a
Confidence Score: 4/5Safe to merge — the change is minimal, correctly scoped, and the forceNotify path is a no-op at the kernel level when both sides already agree on terminal size. One file is touched with 13 lines added. The hide→reveal flag logic is straightforward: revealPending is latched on a zero-size observation and consumed atomically in run(), so no state leaks across scheduler lifetimes. The only note is that createResizeScheduler and its new revealPending branch have no dedicated unit tests, making future regressions harder to catch early. apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts — specifically the untested hide/reveal cycle through the new revealPending flag.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Adds revealPending flag to createResizeScheduler that forces onResize to fire on reveal even when terminal dims are unchanged, mirroring VS Code's setVisible behavior. Logic is correct and well-scoped. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant RO as ResizeObserver
participant RS as createResizeScheduler
participant MAR as measureAndResize
participant CB as onResize
Note over RO,CB: Pane hidden via CSS (container collapses to 0x0)
RO->>RS: "observe entry width=0 height=0"
RS->>RS: "revealPending = true"
RS->>RS: dispose clears debounce timeout
RS-->>RO: return early no setTimeout
Note over RO,CB: Pane becomes visible again
RO->>RS: "observe entry width=W height=H"
RS->>RS: dispose then setTimeout run 75ms
RS->>RS: "run reads forceNotify=true resets revealPending=false"
RS->>MAR: "measureAndResize with forceNotify=true"
MAR->>MAR: fitAddon.fit may or may not change cols/rows
MAR->>CB: onResize called unconditionally via forceNotify
CB-->>MAR: PTY dims re-sent to host TIOCSWINSZ
%%{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 RO as ResizeObserver
participant RS as createResizeScheduler
participant MAR as measureAndResize
participant CB as onResize
Note over RO,CB: Pane hidden via CSS (container collapses to 0x0)
RO->>RS: "observe entry width=0 height=0"
RS->>RS: "revealPending = true"
RS->>RS: dispose clears debounce timeout
RS-->>RO: return early no setTimeout
Note over RO,CB: Pane becomes visible again
RO->>RS: "observe entry width=W height=H"
RS->>RS: dispose then setTimeout run 75ms
RS->>RS: "run reads forceNotify=true resets revealPending=false"
RS->>MAR: "measureAndResize with forceNotify=true"
MAR->>MAR: fitAddon.fit may or may not change cols/rows
MAR->>CB: onResize called unconditionally via forceNotify
CB-->>MAR: PTY dims re-sent to host TIOCSWINSZ
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:197-215
**No test coverage for `revealPending` path**
`createResizeScheduler` and `measureAndResize` are not covered by any test file in the `terminal/` directory — the 264-test suite exercises other modules. The hide→reveal→`forceNotify` flow is the core invariant of this fix, and it's easy to accidentally regress (e.g., `revealPending` not being reset before `measureAndResize` is called, or the flag surviving a scheduler recreation). A lightweight unit test that stubs `ResizeObserver` entries and asserts `onResize` is called after a 0→positive cycle would anchor the behavior.
Reviews (1): Last reviewed commit: "fix(desktop): re-send terminal PTY dims ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const run = () => { | ||
| timeoutId = null; | ||
| measureAndResize(runtime, onResize); | ||
| const forceNotify = revealPending; | ||
| revealPending = false; | ||
| measureAndResize(runtime, onResize, { forceNotify }); | ||
| }; | ||
|
|
||
| const observe: ResizeObserverCallback = (entries) => { | ||
| if ( | ||
| entries.some( | ||
| (entry) => | ||
| entry.contentRect.width <= 0 || entry.contentRect.height <= 0, | ||
| ) | ||
| ) { | ||
| revealPending = true; | ||
| dispose(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
revealPending path
createResizeScheduler and measureAndResize are not covered by any test file in the terminal/ directory — the 264-test suite exercises other modules. The hide→reveal→forceNotify flow is the core invariant of this fix, and it's easy to accidentally regress (e.g., revealPending not being reset before measureAndResize is called, or the flag surviving a scheduler recreation). A lightweight unit test that stubs ResizeObserver entries and asserts onResize is called after a 0→positive cycle would anchor the behavior.
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: 197-215
Comment:
**No test coverage for `revealPending` path**
`createResizeScheduler` and `measureAndResize` are not covered by any test file in the `terminal/` directory — the 264-test suite exercises other modules. The hide→reveal→`forceNotify` flow is the core invariant of this fix, and it's easy to accidentally regress (e.g., `revealPending` not being reset before `measureAndResize` is called, or the flag surviving a scheduler recreation). A lightweight unit test that stubs `ResizeObserver` entries and asserts `onResize` is called after a 0→positive cycle would anchor the behavior.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Architecture diagram
sequenceDiagram
participant CSS as CSS Display
participant RO as ResizeObserver
participant Sched as createResizeScheduler
participant RMS as measureAndResize
participant Term as xterm.js Terminal
participant PTY as PTY Process
Note over CSS,PTY: Terminal pane visibility lifecycle
RO->>RO: Observe container element
alt Pane hidden via CSS (zero-size container)
CSS-->>RO: contentRect = 0x0
RO->>Sched: observe(entry)
Sched->>Sched: Set revealPending = true
Sched->>Sched: Debounce pause
Note over Sched: No resize notification sent<br/>while hidden
end
alt Pane becomes visible again
CSS-->>RO: contentRect = real dimensions
RO->>Sched: observe(entry)
Sched->>Sched: Schedule run()
Sched->>RMS: run() with forceNotify=true
Note over RMS: forceNotify overrides<br/>size-change check
RMS->>Term: measure container dims
Term->>Term: terminal.resize(cols, rows)
alt forceNotify is true
RMS->>RMS: Force emit onResize
RMS->>PTY: Send TIOCSWINSZ
Note over PTY: Resizes kernel pty
alt PTY size actually changed while hidden
PTY->>PTY: Deliver SIGWINCH
PTY->>Term: Program repaints TUI
else Same size (no change)
Note over PTY: Kernel no-op,<br/>no SIGWINCH sent
end
else Size actually changed
RMS->>RMS: Emit onResize (normal path)
RMS->>PTY: Send TIOCSWINSZ
end
end
Note over Term,PTY: Debounce and parser-idle gating unchanged<br/>from previous implementation
|
CDP verification (real dev app, live PTY): instrumented
The reveal-edge send carries dims identical to pre-hide, i.e. the |
…er observer-driven fits The ResizeObserver only fires on genuine container changes, so an unconditional post-fit notify covers the reveal edge without tracking it: dims either changed (was already sent) or it's a reveal/sub-cell wiggle, where a same-size re-send is a kernel no-op on both ends. Matches VS Code, whose resize debouncer also has no changed-dims gate. CDP re-verified: idle 0 sends, reveal 1 same-dims send, 3x hide/show thrash collapses to 1.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Summary
setVisible→_resizebehavior.forceNotify) instead of gated on changed dims. The observer only fires on genuine container changes, so this covers the reveal edge with no state — dims either changed (a resize was being sent anyway) or it's a reveal / sub-cell wiggle, where a same-size re-send is a kernel no-op on both ends (hostmodeTracker.resizealso early-returns on same dims). VS Code's resize debouncer likewise has no changed-dims gate. Net +9/−2 in one file.Notes
TIOCSWINSZdelivers no SIGWINCH (verified against a real pty + Node child), so idle reveals stay invisible to running programs.nudgeprotocol message, a rows−1→rows resize dance, a reveal-edge latch) were prototyped and rejected in favor of this VS Code-parity re-send.Testing
bun test src/renderer/lib/terminal/(264 tests), desktop typecheck, repo lint all greenSummary by CodeRabbit