fix(desktop): sync v1 terminal dimensions to backend on connect#3545
fix(desktop): sync v1 terminal dimensions to backend on connect#3545
Conversation
When panes mount during new-workspace / preset flows, attachToContainer's fit() can run before flex layout resolves, leaving the backend PTY spawned at stale defaults (shell prompt wraps wrong until a manual resize). Mirror v2's sendResize-on-open pattern: once createOrAttach succeeds, re-fit against the now-settled container and push the real dims.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes a race condition in the v1 terminal (desktop app) where Key observations:
Confidence Score: 5/5Safe to merge — the fix is narrow, well-scoped, and mirrors the existing v2 pattern with no new risk surface. Single-file change adds a small helper called in exactly two onSuccess callbacks. The zero-dimension guard prevents spurious RPCs on hidden/unmounted containers, the async ordering is correct at both call sites, and the worst-case failure mode (layout still partially unsettled) silently degrades back to the pre-fix behavior rather than introducing new bugs. Both P2 comments are non-blocking style suggestions. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts | Adds syncBackendDimensions helper called in both createOrAttach.onSuccess sites (initial attach and restart) to re-fit the terminal against the settled container and push correct PTY dimensions to the backend. Logic is correct; two non-blocking style suggestions noted. |
Sequence Diagram
sequenceDiagram
participant React as React Mount
participant ATC as attachToContainer
participant RO as ResizeObserver
participant COA as createOrAttach (IPC)
participant SBD as syncBackendDimensions
participant BE as Backend PTY
React->>ATC: attachToContainer(paneId, container, onResize)
ATC->>ATC: fitAddon.fit() (layout may not have settled)
ATC->>RO: register ResizeObserver
ATC-->>BE: resizeRef (if dims changed vs cache)
note over ATC,BE: Stale dims if flex not yet resolved
React->>COA: createOrAttach({ cols, rows, ... })
note over COA: async IPC — layout settles during this time
COA-->>React: onSuccess(result)
React->>SBD: syncBackendDimensions()
SBD->>SBD: guard clientWidth/clientHeight == 0?
SBD->>ATC: fitAddon.fit() (settled container)
SBD-->>BE: resizeRef({ cols: xterm.cols, rows: xterm.rows })
note over SBD,BE: Correct dims pushed after layout settled
RO-->>BE: future resizeRef on user window resize
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
Line: 268-272
Comment:
**Silent skip when layout still unresolved**
The guard bails out if `clientWidth === 0 || clientHeight === 0`, which is the right thing to do for a fully hidden/detached container. However, if the flex layout has only *partially* resolved (e.g. the container has the correct width but the height is still collapsing), the guard passes and `fitAddon.fit()` is called with an intermediate height — meaning the bug this PR is fixing could still manifest in that narrow timing window.
A small hardening option: also check that `xterm.cols > 0 && xterm.rows > 0` after `fitAddon.fit()` before calling `resizeRef`, so a degenerate fit result is never forwarded:
```suggestion
const syncBackendDimensions = () => {
if (container.clientWidth === 0 || container.clientHeight === 0) return;
fitAddon.fit();
if (xterm.cols > 0 && xterm.rows > 0) {
resizeRef.current({ paneId, cols: xterm.cols, rows: xterm.rows });
}
};
```
This is defensive rather than strictly required — in practice, a non-zero-size container should always produce valid dimensions from `fitAddon.fit()`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
Line: 394-397
Comment:
**`syncBackendDimensions` ordering in restart path**
In the initial attach `onSuccess`, `syncBackendDimensions` is called after `markTerminalSessionReady` and `startStream` — i.e. after the backend session is fully live. In the `restartTerminalSession` `onSuccess`, it is called first, before `pendingInitialStateRef.current = result` and `maybeApplyInitialState()`.
The restart path doesn't call `v1TerminalCache.startStream` / `setStreamReady` (the stream is already running from the previous session), so calling `resizeRef` at this point is safe — the backend session exists as soon as `createOrAttach` resolves. The ordering is fine for correctness, but a brief comment here (similar to the one in the initial-attach path) would make the intent clear to future readers.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): sync v1 terminal dimension..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const syncBackendDimensions = () => { | ||
| if (container.clientWidth === 0 || container.clientHeight === 0) return; | ||
| fitAddon.fit(); | ||
| resizeRef.current({ paneId, cols: xterm.cols, rows: xterm.rows }); | ||
| }; |
There was a problem hiding this comment.
The guard bails out if clientWidth === 0 || clientHeight === 0, which is the right thing to do for a fully hidden/detached container. However, if the flex layout has only partially resolved (e.g. the container has the correct width but the height is still collapsing), the guard passes and fitAddon.fit() is called with an intermediate height — meaning the bug this PR is fixing could still manifest in that narrow timing window.
A small hardening option: also check that xterm.cols > 0 && xterm.rows > 0 after fitAddon.fit() before calling resizeRef, so a degenerate fit result is never forwarded:
| const syncBackendDimensions = () => { | |
| if (container.clientWidth === 0 || container.clientHeight === 0) return; | |
| fitAddon.fit(); | |
| resizeRef.current({ paneId, cols: xterm.cols, rows: xterm.rows }); | |
| }; | |
| const syncBackendDimensions = () => { | |
| if (container.clientWidth === 0 || container.clientHeight === 0) return; | |
| fitAddon.fit(); | |
| if (xterm.cols > 0 && xterm.rows > 0) { | |
| resizeRef.current({ paneId, cols: xterm.cols, rows: xterm.rows }); | |
| } | |
| }; |
This is defensive rather than strictly required — in practice, a non-zero-size container should always produce valid dimensions from fitAddon.fit().
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
Line: 268-272
Comment:
**Silent skip when layout still unresolved**
The guard bails out if `clientWidth === 0 || clientHeight === 0`, which is the right thing to do for a fully hidden/detached container. However, if the flex layout has only *partially* resolved (e.g. the container has the correct width but the height is still collapsing), the guard passes and `fitAddon.fit()` is called with an intermediate height — meaning the bug this PR is fixing could still manifest in that narrow timing window.
A small hardening option: also check that `xterm.cols > 0 && xterm.rows > 0` after `fitAddon.fit()` before calling `resizeRef`, so a degenerate fit result is never forwarded:
```suggestion
const syncBackendDimensions = () => {
if (container.clientWidth === 0 || container.clientHeight === 0) return;
fitAddon.fit();
if (xterm.cols > 0 && xterm.rows > 0) {
resizeRef.current({ paneId, cols: xterm.cols, rows: xterm.rows });
}
};
```
This is defensive rather than strictly required — in practice, a non-zero-size container should always produce valid dimensions from `fitAddon.fit()`.
How can I resolve this? If you propose a fix, please make it concise.| return; | ||
| } | ||
| setConnectionError(null); | ||
| syncBackendDimensions(); |
There was a problem hiding this comment.
syncBackendDimensions ordering in restart path
In the initial attach onSuccess, syncBackendDimensions is called after markTerminalSessionReady and startStream — i.e. after the backend session is fully live. In the restartTerminalSession onSuccess, it is called first, before pendingInitialStateRef.current = result and maybeApplyInitialState().
The restart path doesn't call v1TerminalCache.startStream / setStreamReady (the stream is already running from the previous session), so calling resizeRef at this point is safe — the backend session exists as soon as createOrAttach resolves. The ordering is fine for correctness, but a brief comment here (similar to the one in the initial-attach path) would make the intent clear to future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
Line: 394-397
Comment:
**`syncBackendDimensions` ordering in restart path**
In the initial attach `onSuccess`, `syncBackendDimensions` is called after `markTerminalSessionReady` and `startStream` — i.e. after the backend session is fully live. In the `restartTerminalSession` `onSuccess`, it is called first, before `pendingInitialStateRef.current = result` and `maybeApplyInitialState()`.
The restart path doesn't call `v1TerminalCache.startStream` / `setStreamReady` (the stream is already running from the previous session), so calling `resizeRef` at this point is safe — the backend session exists as soon as `createOrAttach` resolves. The ordering is fine for correctness, but a brief comment here (similar to the one in the initial-attach path) would make the intent clear to future readers.
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!
Manually port upstream 867ef87 (superset-sh#3545) to fork's useTerminalLifecycle. Straight cherry-pick failed because fork has diverged around cold-restore handling — notably the 2nd createOrAttach success handler does NOT call markTerminalSessionReady / startStream here (deferred until the real shell spawns), so upstream's syncBackendDimensions() call point shifts up by a few lines. Adds the same syncBackendDimensions() helper and calls it at both createOrAttach onSuccess sites so the backend PTY sees the settled container dims before flex layout has a chance to resize (preset tabs, new workspace bulk creation were spawning at stale defaults).
…m-sync upstream取り込み: v1 terminal dimensions sync fix (superset-sh#3545, 手動移植)
…al dim sync) - 1979f4c fix(desktop): v2 sidebar section count reflects visually grouped workspaces (superset-sh#3544) → PR #315 (clean cherry-pick) - 867ef87 fix(desktop): sync v1 terminal dimensions to backend on connect (superset-sh#3545) → PR #316 (manual port for fork cold-restore divergence)
1 participant
Summary
attachToContainer'sfitAddon.fit()can run before flex layout resolves. The backend PTY gets spawned at stale (often default) dimensions, so the initial shell prompt wraps at the wrong column until the user manually resizes.terminal-ws-transport.ts(sendResizeon WebSocketopen): oncecreateOrAttachsucceeds, re-fit against the now-settled container and push the real dims to the backend.onSuccesssites — initial attach andrestartTerminalSession— via a smallsyncBackendDimensionshelper.Test plan
tput cols; tput linesmatches the visible grid immediately, with no manual resize.printf '=%.0s' {1..$(tput cols)}; echo— line fills exactly one row (no early wrap / overflow).Summary by cubic
Sync v1 terminal cols/rows to the backend right after connect to prevent shells spawning at stale sizes. Re-fit after layout settles so the PTY starts with correct dimensions and prompts don’t wrap.
syncBackendDimensionsto callfitAddon.fit()and send resize aftercreateOrAttachand onrestartTerminalSession, mirroring v2’s send-resize-on-open and guarding against zero-sized containers.Written for commit 7e785d5. Summary will update on new commits.
Summary by CodeRabbit