Skip to content

fix(desktop): sync v1 terminal dimensions to backend on connect #3545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kitenite merged 1 commit into from
Apr 18, 2026
+13 −0
Merged

fix(desktop): sync v1 terminal dimensions to backend on connect #3545

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions ...n/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,17 @@ export function useTerminalLifecycle({

const { xterm, fitAddon, searchAddon } = cached;

// Called after createOrAttach resolves: re-fit against the now-settled
// container and push dims to the backend. Guards against stale sizes
// from attachToContainer's fit running before flex layout resolved
// (e.g. preset tabs, new workspace bulk creation). Mirrors v2's
// terminal-ws-transport sendResize-on-open.
const syncBackendDimensions = () => {
if (container.clientWidth === 0 || container.clientHeight === 0) return;
fitAddon.fit();
resizeRef.current({ paneId, cols: xterm.cols, rows: xterm.rows });
};
Comment on lines +268 to +272
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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:

Suggested change
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.


// Attach the wrapper div to the live container.
// The cache creates a ResizeObserver that calls fitAddon.fit() and
// forwards resize events to the backend — no separate resize handler needed.
Expand Down Expand Up @@ -383,6 +394,7 @@ export function useTerminalLifecycle({
return;
}
setConnectionError(null);
syncBackendDimensions();
Comment on lines 394 to +397
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

pendingInitialStateRef.current = result;
maybeApplyInitialState();
if (!command) {
Expand Down Expand Up @@ -595,6 +607,7 @@ export function useTerminalLifecycle({
v1TerminalCache.startStream(paneId);
v1TerminalCache.setStreamReady(paneId);
markTerminalSessionReady(paneId);
syncBackendDimensions();

const storedColdRestore = coldRestoreState.get(paneId);
if (storedColdRestore?.isRestored) {
Expand Down
Loading