Reflow terminal on layout-driven container resizes#3612
Conversation
The xterm resize path was gated on window.resize / resizeEpoch, so container resizes driven by CSS/flex layout (split-pane drag, sidebar collapse) changed the terminal's DOM size without firing fit() or sending terminalResize. The pty never resized, so no SIGWINCH reached the child. Observe the xterm container with a ResizeObserver (rAF-coalesced to one fit per frame) and refit + send terminalResize directly, instead of relying on window.resize. Also surface resize failures: when a process resize throws (e.g. a Bun build without terminal.resize), the frontend command swallows the error. Log it once per process via Effect.logWarning at the shared resizePtyProcess chokepoint so the cause is visible without spamming.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
vouch:unvouched
![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: e3e879db92
ℹ️ 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".
| fitTerminalSafely(fitAddon); | ||
| if (wasAtBottom) { | ||
| terminal.scrollToBottom(); | ||
| } | ||
| void resizeTerminal(terminal.cols, terminal.rows); |
There was a problem hiding this comment.
When switching away from a thread, retained terminal drawers are kept mounted but wrapped in className="hidden" in PersistentThreadTerminalDrawer; that display:none size change also reaches this observer. Because the callback still calls fit() and sends the resulting size to the server, background terminals can be resized to xterm's minimum/zero-container geometry and receive a SIGWINCH while hidden, which can break full-screen/background processes and corrupt wrapping until the thread is shown again. Skip the resize when the observed container size is 0 or pause the observer while the drawer is not visible.
Useful? React with 👍 / 👎.
ApprovabilityVerdict: Needs human review This PR adds new ResizeObserver behavior for terminal resizing. An unresolved review comment identifies a potential bug where hidden terminals could be resized to zero geometry, potentially corrupting terminal state. This concern should be addressed before merging. You can customize Macroscope's approvability policy. Learn more. |
1 participant
What
The terminal only reflowed (sent
SIGWINCHto the child process) when the OS window was resized. Resizing the terminal via layout — dragging a split-pane divider, collapsing the sidebar, dragging the editor↔terminal split — left the child process at its stale size.Why
TerminalViewport's resize effect was gated onwindow.resize/resizeEpoch. Those only advance on a realwindow.resize, the drag-handle release, or a visibility toggle. A CSS/flex layout change resizes the xterm DOM node without firingwindow.resize, sofitAddon.fit()and theterminalResizeRPC never ran → nopty.resize()→ noSIGWINCH.Change
ThreadTerminalDrawer.tsx— observe the xterm container with aResizeObserver(rAF-coalesced to one fit per frame) that refits and sendsterminalResizedirectly, instead of depending onwindow.resize. The existing window/drag/visibility path is left intact.Manager.ts— resize can fail permanently on a Bun build lackingterminal.resize, and the frontend command swallows that error. Log it once per process viaEffect.logWarningat the sharedresizePtyProcesschokepoint, so the cause is visible in logs without spamming on every resize.No new dependencies. No lockfile changes.
Testing
Ran a
SIGWINCHprobe in a terminal, then dragged a split divider (layout-only resize, no window resize):WINCHevents with the newcols × rowsthroughout the drag (cols reflow while rows stay fixed — the signature of a width-only layout resize, distinct from a window resize).Both
apps/webandapps/servertypecheck.Note
Low Risk
Scoped to terminal UI reflow and non-fatal resize logging; no auth, data, or API contract changes.
Overview
Fixes terminals staying at a stale column/row count when only layout changes (split-pane drag, sidebar collapse, editor↔terminal split), because refit/
terminalResizepreviously depended onwindow.resizeandresizeEpoch.ThreadTerminalDrawer.tsxadds aResizeObserveron the xterm container, rAF-coalesced like the existing resize effect, to runfitAddon.fit()andterminalResizewhen the container size changes without a window resize event.Manager.tslogsEffect.logWarningonce per PTY process inresizePtyProcesswhen resize fails permanently (e.g. Bun withoutterminal.resize), since the web client swallows resize errors.Reviewed by Cursor Bugbot for commit e3e879d. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Reflow terminal on layout-driven container resizes
ResizeObserverinThreadTerminalDrawer.tsxso the terminal refits and emits a resize to the backend when its container changes size (e.g. split-pane drag, sidebar collapse), not only on window resize.Manager.tsusing aWeakSet: whenprocess.resizefails, a warning is logged once per process with thread, terminal, pid, and cause.Macroscope summarized e3e879d.