fix(web): break the terminal resize loop properly
The previous 'stabilize useMutation results' fix addressed one cause
of /shell/resize being spammed (the mutation object identity), but
not the actual root cause: a closed loop between xterm's onResize
callback, ResizeObserver, and the backend.
The full loop:
1. ResizeObserver fires
2. fitAddon.fit() runs
3. fit() calls terminal.resize(cols, rows)
4. terminal.onResize(cb) fires the listener
5. cb calls resize.mutate() — POSTs to /shell/resize
6. server calls pty.Setsize
7. bash receives SIGWINCH and redraws the prompt
8. the new output writes to the terminal, which can shift a
scrollbar / padding by a few pixels
9. ResizeObserver fires again
-> repeat
Fixes applied to TerminalPanel's terminal-init useEffect:
- Drop the terminal.onResize subscription entirely. Per the xterm
+ FitAddon pattern, the right way to push size to the backend
is: from the ResizeObserver path, call fit(), then read
terminal.cols/rows synchronously and call resize.mutate().
- Dedupe by (cols, rows) — if the size hasn't changed since the
last reported size, skip the mutation.
- Dedupe the ResizeObserver callback by container
getBoundingClientRect — sub-pixel changes (e.g. a scrollbar
appearing) won't trigger a redundant fit().
- Depend on the stable sendResize function (the destructured
mutate from useMutation) rather than the whole resize object,
so unrelated render churn can't tear down and re-create the
terminal.
- Run an initial fit() at mount so the backend learns the real
size before the user starts typing.
The 100ms debounce was a band-aid that didn't actually break the
loop — it only slowed it down.
This commit is contained in:
@@ -29,6 +29,9 @@ export function TerminalPanel({ workspaceId, shellId }: TerminalPanelProps) {
|
|||||||
const restart = useShellRestart(workspaceId);
|
const restart = useShellRestart(workspaceId);
|
||||||
|
|
||||||
const resize = useShellResize(workspaceId, shellId);
|
const resize = useShellResize(workspaceId, shellId);
|
||||||
|
// Destructure the mutate function so the effect can depend on the
|
||||||
|
// stable function reference alone, not the whole mutation result.
|
||||||
|
const sendResize = resize.mutate;
|
||||||
|
|
||||||
// Initialize xterm once and keep it alive across WS status changes.
|
// Initialize xterm once and keep it alive across WS status changes.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
@@ -37,8 +40,6 @@ export function TerminalPanel({ workspaceId, shellId }: TerminalPanelProps) {
|
|||||||
|
|
||||||
let terminal: Terminal | undefined;
|
let terminal: Terminal | undefined;
|
||||||
let fitAddon: FitAddon | undefined;
|
let fitAddon: FitAddon | undefined;
|
||||||
let debounceRef: number | undefined;
|
|
||||||
let removeResize: (() => void) | undefined;
|
|
||||||
|
|
||||||
try {
|
try {
|
||||||
terminal = new Terminal({
|
terminal = new Terminal({
|
||||||
@@ -62,19 +63,6 @@ export function TerminalPanel({ workspaceId, shellId }: TerminalPanelProps) {
|
|||||||
terminal.loadAddon(fitAddon);
|
terminal.loadAddon(fitAddon);
|
||||||
terminal.open(container);
|
terminal.open(container);
|
||||||
|
|
||||||
removeResize = terminal.onResize(({ cols, rows }) => {
|
|
||||||
if (!workspaceId || !shellId) return;
|
|
||||||
if (cols <= 0 || rows <= 0) return;
|
|
||||||
if (debounceRef !== undefined) {
|
|
||||||
window.clearTimeout(debounceRef);
|
|
||||||
}
|
|
||||||
debounceRef = window.setTimeout(() => {
|
|
||||||
debounceRef = undefined;
|
|
||||||
if (!workspaceId || !shellId) return;
|
|
||||||
resize.mutate({ cols, rows });
|
|
||||||
}, 100);
|
|
||||||
}).dispose;
|
|
||||||
|
|
||||||
terminalRef.current = terminal;
|
terminalRef.current = terminal;
|
||||||
fitAddonRef.current = fitAddon;
|
fitAddonRef.current = fitAddon;
|
||||||
} catch {
|
} catch {
|
||||||
@@ -84,46 +72,102 @@ export function TerminalPanel({ workspaceId, shellId }: TerminalPanelProps) {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// --- Resize loop: fit() -> terminal.resize() -> onResize() -> mutate.
|
||||||
|
//
|
||||||
|
// Listening to terminal.onResize to push the new size to the backend
|
||||||
|
// creates a closed loop:
|
||||||
|
// fit() -> terminal.resize(cols, rows) -> onResize(cb) fires
|
||||||
|
// -> cb calls resize.mutate() -> server calls pty.Setsize
|
||||||
|
// -> bash receives SIGWINCH and redraws -> new output via WS
|
||||||
|
// -> terminal.write(...) may change layout (scrollbar/padding) by
|
||||||
|
// a few pixels -> ResizeObserver fires again -> fit() again.
|
||||||
|
//
|
||||||
|
// The right pattern (per the xterm + FitAddon docs): only call
|
||||||
|
// fit() from the ResizeObserver, then read terminal.cols/rows
|
||||||
|
// synchronously and push to the backend. Skip when the size hasn't
|
||||||
|
// changed. Skip when the container's getBoundingClientRect hasn't
|
||||||
|
// changed (so sub-pixel oscillation doesn't trigger).
|
||||||
|
|
||||||
|
const lastReportedSize = { cols: 0, rows: 0 };
|
||||||
|
const lastRect = { width: -1, height: -1 };
|
||||||
let rafId: number | undefined;
|
let rafId: number | undefined;
|
||||||
|
|
||||||
|
const reportSize = () => {
|
||||||
|
if (!workspaceId || !shellId) return;
|
||||||
|
const cols = terminal!.cols;
|
||||||
|
const rows = terminal!.rows;
|
||||||
|
if (cols <= 0 || rows <= 0) return;
|
||||||
|
if (cols === lastReportedSize.cols && rows === lastReportedSize.rows) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
lastReportedSize.cols = cols;
|
||||||
|
lastReportedSize.rows = rows;
|
||||||
|
sendResize({ cols, rows });
|
||||||
|
};
|
||||||
|
|
||||||
|
const doFit = () => {
|
||||||
|
const rect = container.getBoundingClientRect();
|
||||||
|
// Skip if the container's outer rect didn't actually move. This
|
||||||
|
// stops ResizeObserver firings triggered by sub-pixel internal
|
||||||
|
// changes (e.g. scrollbar appearance) from feeding fit().
|
||||||
|
if (
|
||||||
|
rect.width === lastRect.width &&
|
||||||
|
rect.height === lastRect.height
|
||||||
|
) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
lastRect.width = rect.width;
|
||||||
|
lastRect.height = rect.height;
|
||||||
|
|
||||||
|
try {
|
||||||
|
fitAddon!.fit();
|
||||||
|
} catch {
|
||||||
|
// fit() throws when the container has zero dimensions; ignore
|
||||||
|
// and wait for the next ResizeObserver tick.
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
// Push the new size to the backend immediately, deduped by cols/rows.
|
||||||
|
reportSize();
|
||||||
|
};
|
||||||
|
|
||||||
const scheduleFit = () => {
|
const scheduleFit = () => {
|
||||||
if (rafId !== undefined) return;
|
if (rafId !== undefined) return;
|
||||||
rafId = requestAnimationFrame(() => {
|
rafId = requestAnimationFrame(() => {
|
||||||
rafId = undefined;
|
rafId = undefined;
|
||||||
try {
|
doFit();
|
||||||
fitAddon?.fit();
|
|
||||||
} catch {
|
|
||||||
// fit() throws when the container has zero dimensions; ignore
|
|
||||||
// and wait for the next ResizeObserver tick.
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
const resizeObserver = new ResizeObserver(scheduleFit);
|
const resizeObserver = new ResizeObserver(scheduleFit);
|
||||||
resizeObserver.observe(container);
|
resizeObserver.observe(container);
|
||||||
|
|
||||||
const handleResize = () => {
|
const handleWindowResize = () => {
|
||||||
scheduleFit();
|
scheduleFit();
|
||||||
};
|
};
|
||||||
|
|
||||||
window.addEventListener("resize", handleResize);
|
window.addEventListener("resize", handleWindowResize);
|
||||||
|
|
||||||
|
// Run an initial fit so the backend knows the size as soon as the
|
||||||
|
// terminal mounts. Without this, the user would type into a default
|
||||||
|
// 80x24 terminal and the first line of output would wrap.
|
||||||
|
scheduleFit();
|
||||||
|
|
||||||
return () => {
|
return () => {
|
||||||
if (rafId !== undefined) {
|
if (rafId !== undefined) {
|
||||||
cancelAnimationFrame(rafId);
|
cancelAnimationFrame(rafId);
|
||||||
}
|
}
|
||||||
if (debounceRef !== undefined) {
|
|
||||||
window.clearTimeout(debounceRef);
|
|
||||||
debounceRef = undefined;
|
|
||||||
}
|
|
||||||
removeResize?.();
|
|
||||||
resizeObserver.disconnect();
|
resizeObserver.disconnect();
|
||||||
window.removeEventListener("resize", handleResize);
|
window.removeEventListener("resize", handleWindowResize);
|
||||||
terminal?.dispose();
|
terminal?.dispose();
|
||||||
fitAddon?.dispose();
|
fitAddon?.dispose();
|
||||||
terminalRef.current = null;
|
terminalRef.current = null;
|
||||||
fitAddonRef.current = null;
|
fitAddonRef.current = null;
|
||||||
};
|
};
|
||||||
}, [resize, workspaceId, shellId]);
|
// Depend on `resize.mutate` (the stable function from useMutation)
|
||||||
|
// rather than the whole `resize` object, so this effect doesn't
|
||||||
|
// re-run on every render if anything else in the mutation result
|
||||||
|
// changes by reference.
|
||||||
|
}, [sendResize, workspaceId, shellId]);
|
||||||
|
|
||||||
// Reset per-workspace state and wire the active socket to xterm.
|
// Reset per-workspace state and wire the active socket to xterm.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
|||||||
Reference in New Issue
Block a user