From b75181b725d2e3fd9a65c638f80f1f5ac6ba1868 Mon Sep 17 00:00:00 2001 From: arkon Date: Wed, 10 Jun 2026 02:07:26 +0200 Subject: [PATCH] fix(terminal): address re-review findings on the pane-buffer rework Follow-up to the PR #112 re-review (all six prior blockers were already resolved; these are new issues the rework introduced): - app.js: define the missing `_scheduleTerminalRepaint()` helper. It was called from both WebGL-fallback paths (onContextLoss + long-task trip) but defined nowhere, so each fallback threw `TypeError` and lost the post-fallback repaint, leaving a stale/blank terminal. Implemented as an rAF-debounced full refresh (matches the old inline `terminal.refresh`). - app.js: clear terminal load-state on the two post-write stale-select early-returns (cached-buffer + rewrite branches), matching the other four checks. Switching away from a mid-loading tab no longer leaks a permanent `.tab-loading` spinner / `aria-busy=true`. - terminal-ui.js + app.js: gate the post-resize TUI-redraw settle on an actual dimension change. `sendResize` now returns whether dims changed; a same-size tab switch sends no SIGWINCH, so the wait is skipped instead of charging a flat tax on every non-shell switch. Literal hoisted to `TUI_REDRAW_SETTLE_MS`. - tmux-manager.ts: `resizeWindow()` uses a non-blocking `exec` instead of `execSync` so the interactive WS/HTTP resize path can't stall the Fastify event loop on a slow/hung tmux. Sole caller already fire-and- forgets the result; test updated to assert the async dispatch. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/tmux-manager.ts | 23 ++++++++++++--------- src/web/public/app.js | 39 ++++++++++++++++++++++++++++------- src/web/public/constants.js | 1 + src/web/public/terminal-ui.js | 11 ++++++++-- test/tmux-manager.test.ts | 9 +++++--- 5 files changed, 60 insertions(+), 23 deletions(-) diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index e48cf7d9..cc44b855 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -2137,16 +2137,19 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { return false; } - try { - execSync(`${this.tmux()} resize-window -t ${shellescape(muxName)} -x ${cols} -y ${rows}`, { - timeout: EXEC_TIMEOUT_MS, - stdio: 'ignore', - }); - return true; - } catch (err) { - console.error('[TmuxManager] Failed to resize tmux window:', err); - return false; - } + // Fire-and-forget: this runs on the interactive resize path (WS {t:'z'} and + // HTTP /resize), so use a non-blocking exec — a slow/hung tmux must not stall + // the Fastify event loop while other sessions' input/SSE are served. The sole + // caller (Session.resize) ignores the result, and under `window-size manual` + // the subsequent ptyProcess.resize is subordinate to this authoritative size. + exec( + `${this.tmux()} resize-window -t ${shellescape(muxName)} -x ${cols} -y ${rows}`, + { timeout: EXEC_TIMEOUT_MS }, + (err) => { + if (err) console.error('[TmuxManager] Failed to resize tmux window:', err); + } + ); + return true; } isAvailable(): boolean { diff --git a/src/web/public/app.js b/src/web/public/app.js index 338fd481..4bd03afc 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -702,6 +702,22 @@ class CodemanApp { this._webglLongTaskObserver = null; } + /** + * Repaint the full terminal viewport after a renderer swap (WebGL → canvas/DOM). + * Scheduled on the next frame so it lands after the addon teardown settles, and + * debounced so the context-loss and long-task fallback paths can't double-fire. + * No-ops safely if the terminal isn't ready. + */ + _scheduleTerminalRepaint() { + if (this._terminalRepaintScheduled) return; + this._terminalRepaintScheduled = true; + const raf = typeof requestAnimationFrame === 'function' ? requestAnimationFrame : (cb) => setTimeout(cb, 0); + raf(() => { + this._terminalRepaintScheduled = false; + try { this.terminal?.refresh(0, this.terminal.rows - 1); } catch {} + }); + } + _disableWebGLSticky(reason) { try { localStorage.setItem('codeman-webgl-disabled', JSON.stringify({ reason, at: Date.now() })); @@ -3008,7 +3024,7 @@ class CodemanApp { // a small region with empty rows below the status bar. // sendResize is a no-op on the server when dims haven't changed, so // calling it every tab switch is cheap. - await this.sendResize(sessionId, { forceHttp: true }).catch(() => {}); + const dimsChanged = await this.sendResize(sessionId, { forceHttp: true }).catch(() => false); if (this._isStaleSelect(selectGen)) { this._clearTerminalLoadState(sessionId, selectGen); return; @@ -3027,7 +3043,10 @@ class CodemanApp { this._setTerminalLoadState(sessionId, selectGen, 'replaying'); this._resetTerminalForReplay(); await this.chunkedTerminalWrite(cachedBuffer, TERMINAL_CHUNK_SIZE, bufferLoadOwner); - if (this._isStaleSelect(selectGen)) return; + if (this._isStaleSelect(selectGen)) { + this._clearTerminalLoadState(sessionId, selectGen); + return; + } this.terminal.scrollToBottom(); _crashDiag.log('CACHE_DONE'); } else if (sessionIsBusy) { @@ -3037,11 +3056,12 @@ class CodemanApp { } // Give TUI sessions a short chance to redraw after resize before the - // fresh buffer fetch captures the live mux pane. Shell sessions and - // snapshot restores do not need this delay, so terminal content can - // appear immediately when switching between shells. - if (session?.mode !== 'shell') { - await new Promise((resolve) => setTimeout(resolve, 400)); + // fresh buffer fetch. Only needed when the resize actually changed + // dimensions (a real SIGWINCH → Ink redraw); a same-size tab switch sent + // no resize, so waiting would just add latency. Shell sessions never need + // it, so terminal content can appear immediately when switching shells. + if (session?.mode !== 'shell' && dimsChanged) { + await new Promise((resolve) => setTimeout(resolve, TUI_REDRAW_SETTLE_MS)); if (this._isStaleSelect(selectGen)) { this._clearTerminalLoadState(sessionId, selectGen); return; @@ -3075,7 +3095,10 @@ class CodemanApp { } // Use chunked write for large buffers to avoid UI jank await this.chunkedTerminalWrite(data.terminalBuffer, TERMINAL_CHUNK_SIZE, bufferLoadOwner); - if (this._isStaleSelect(selectGen)) return; + if (this._isStaleSelect(selectGen)) { + this._clearTerminalLoadState(sessionId, selectGen); + return; + } // Ensure terminal is scrolled to bottom after buffer load this.terminal.scrollToBottom(); } diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 9e5b7723..3c7b538b 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -57,6 +57,7 @@ const TERMINAL_CHUNK_SIZE = 32 * 1024; // 32KB chunks for terminal buffer l const TERMINAL_TAIL_SIZE = 1024 * 1024; // 1MB tail for initial load (more scrollback on tab switch) const SYNC_WAIT_TIMEOUT_MS = 50; // Wait timeout for terminal sync const STATS_POLLING_INTERVAL_MS = 2000; // System stats polling +const TUI_REDRAW_SETTLE_MS = 400; // Grace for a TUI to redraw after a real resize, before fetching its buffer // Z-index base values for layered floating windows const ZINDEX_SUBAGENT_BASE = 1000; diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index c970b882..4dafcca3 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -1906,7 +1906,13 @@ Object.assign(CodemanApp.prototype, { // terminal size matches what we report to the server PTY. if (this.fitAddon) this.fitAddon.fit(); const dims = this.getTerminalDimensions(); - if (!dims) return; + if (!dims) return false; + // Did the dimensions actually change since the last resize we sent? Callers + // use this to skip work (e.g. the post-resize TUI-redraw settle) when no + // real SIGWINCH was triggered — switching tabs at the same browser size is + // a no-op on the server and needs no redraw grace. + const prev = this._lastResizeDims; + const changed = !prev || prev.cols !== dims.cols || prev.rows !== dims.rows; // Update _lastResizeDims so the throttledResize handler won't redundantly // clear the terminal for the same dimensions (which would blank the screen // without a subsequent Ink redraw to repaint it). @@ -1915,7 +1921,7 @@ Object.assign(CodemanApp.prototype, { if (!options.forceHttp && this._wsReady && this._wsSessionId === sessionId) { try { this._ws.send(JSON.stringify({ t: 'z', c: dims.cols, r: dims.rows })); - return; + return changed; } catch { // Fall through to HTTP POST } @@ -1925,6 +1931,7 @@ Object.assign(CodemanApp.prototype, { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(dims), }); + return changed; }, /** diff --git a/test/tmux-manager.test.ts b/test/tmux-manager.test.ts index 9398f969..cd1ad7a0 100644 --- a/test/tmux-manager.test.ts +++ b/test/tmux-manager.test.ts @@ -9,7 +9,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { TmuxManager, formatPaneSnapshot, parsePaneList, resolveActivePaneTarget } from '../src/tmux-manager.js'; -import { execSync } from 'node:child_process'; +import { execSync, exec } from 'node:child_process'; // ============================================================================ // Unit Tests (mocked) @@ -64,6 +64,7 @@ vi.mock('node:fs/promises', async () => { describe('TmuxManager (unit)', () => { let manager: TmuxManager; const mockedExecSync = vi.mocked(execSync); + const mockedExec = vi.mocked(exec); beforeEach(() => { vi.clearAllMocks(); @@ -131,9 +132,11 @@ describe('TmuxManager (unit)', () => { it('resizes the tmux window when Codeman accepts a desktop resize', () => { expect(manager.resizeWindow('codeman-abc12345', 140, 42)).toBe(true); - expect(mockedExecSync).toHaveBeenCalledWith( + // Non-blocking exec (not execSync) on the interactive resize hot path. + expect(mockedExec).toHaveBeenCalledWith( "tmux -L 'codeman' resize-window -t 'codeman-abc12345' -x 140 -y 42", - expect.objectContaining({ stdio: 'ignore' }) + expect.objectContaining({ timeout: expect.any(Number) }), + expect.any(Function) ); }); });