From 070e8da81b31a97d42f4809164414ee65a9a6f32 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Wed, 9 Sep 2026 15:24:55 +0200 Subject: [PATCH] fix(terminal): yield only the resize send, and take sizing back on redock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous commit found four defects in it. The guard sat above the local fit, so it suppressed a reflow as well as the server write. tab-rail-resize performs its single settle-time refit through sendResize and has no fallback for a truthy activeSessionId, so dragging the rail stopped reflowing a detached session's terminal in the dashboard. The mobile-keyboard guard fourteen lines below already draws the line correctly — withhold the send, never the reflow — and the guard now sits after the fit. _lastResizeDims is one value for the whole window, and both guards skip updating it, so while a popup owns a session that value no longer describes the PTY. _redock repaired it only for the active session. Pop out A, switch to B, close the popup: selecting A later found unchanged dimensions, returned "unchanged", and selectSession skipped its 400ms redraw wait — while the server, comparing against the real pane, did resize and did raise SIGWINCH, so the fetch painted the pre-redraw frame. _redock now clears the record on every path, active or not. _redock could also fire a resize for a session already gone: _onSessionDeleted redocks before cleanup, so the id can be dead and the request is a guaranteed 404. It now checks the session still exists. restoreTerminalSize — the header's redraw button and Ctrl+Shift+R — silently did nothing for a detached session while still reporting success with dimensions nothing was set to. It now says the session is sized by its own window, where the same button works. The `force` comment claimed a client-side dedupe that does not exist; the deduplication is server-side against the real pane. Corrected to say what the flag actually buys. The _redock doc comment now records that the function writes to the server and is not idempotent. Tests: _redock was the untested half and is the half three of these defects sit in. It now has coverage for clearing the stale record on both the active and inactive paths, re-asserting only for the session being shown, and staying silent for a deleted session. The existing sendResize test now asserts the local fit still runs. --- src/web/public/app.js | 25 +++++-- src/web/public/terminal-ui.js | 18 ++++- test/detached-session-pane-sizing.test.ts | 89 ++++++++++++++++++++++- 3 files changed, 120 insertions(+), 12 deletions(-) diff --git a/src/web/public/app.js b/src/web/public/app.js index b495fb8c..617b22f9 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -1332,7 +1332,11 @@ class CodemanApp { this._redock(id); } - /** Clear all dashboard-side detached state/timers for a session. */ + /** Clear all dashboard-side detached state/timers for a session, and take its + * sizing back: the popup owned the pane while it was open, so the dashboard's + * record of it is stale and the session it is showing needs re-measuring. + * ⚠️ Not idempotent — each call re-asserts, so a path that redocks twice for + * one close sends two SIGWINCHs. */ _redock(id) { const t = this._detachWatchTimers.get(id); if (t) { clearInterval(t); this._detachWatchTimers.delete(id); } @@ -1340,12 +1344,19 @@ class CodemanApp { this._detachOrphanStrikes.delete(id); this.detachedWindows.delete(id); this._markDetached(id, false); - // Sizing comes back with the session. While the popup owned it the dashboard - // sent no resizes, so the PTY still holds the popup's geometry; the session - // this window is actually showing has to be re-sized to this window, and - // `force` is required because the dimensions the dashboard last sent are - // the ones it is about to send again. - if (id === this.activeSessionId) { + // While the popup owned this session the dashboard sent no resizes, so + // `_lastResizeDims` — one value for the whole window — no longer describes + // the PTY, which the popup has been sizing. Clearing it makes the next + // sendResize report truthfully, on every redock path rather than only the + // active one: `selectSession` reads that answer to decide whether to wait + // for the TUI's redraw, and a false "unchanged" makes it fetch the frame + // before the redraw lands. + this._lastResizeDims = null; + // Sizing comes back with the session. `force` buys a guaranteed repaint for + // the case where popup and dashboard happened to agree on a size; the server + // already resizes on its own comparison against the real pane whenever the + // two differ. + if (this.sessions.has(id) && id === this.activeSessionId) { this.sendResize(id, { force: true })?.catch?.(() => {}); } } diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 1c30ee80..b5b4d723 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -3846,6 +3846,14 @@ Object.assign(CodemanApp.prototype, { return; } + // The pane belongs to the popup showing it, so this window has nothing to + // restore. Say so rather than reporting a size that was never sent — the + // same button in that window does the job. + if (!this.isSoloWindow && this.detachedSessions?.has(this.activeSessionId)) { + this.showToast('This session is sized by its own window', 'warning'); + return; + } + const dims = this.getTerminalDimensions(); if (!dims) { this.showToast('Could not determine terminal size', 'error'); @@ -4827,16 +4835,20 @@ Object.assign(CodemanApp.prototype, { * @returns {Promise} Whether dimensions changed from the last send */ async sendResize(sessionId, options = {}) { + // Fit terminal to container before reading dimensions — ensures local + // terminal size matches what we report to the server PTY. + if (this.fitAddon) this.fitAddon.fit(); // One PTY cannot hold two sizes. A detached session is owned by its own // window, and the dashboard's terminal is narrower than that window because // the session rail takes width the popup does not have — so both sizing it // makes the CLI draw frames that fit neither, which garbles the popup. The // dashboard yields; the solo window sizes what it alone displays. // (_maybeRefetchFullHistory already stands aside for the same reason.) + // ⚠️ AFTER the fit, never before: the local reflow keeps the dashboard's own + // xterm right, and only the SERVER write is the dashboard's to withhold — + // the mobile-keyboard guard below draws exactly this line. tab-rail-resize + // performs its one settle-time refit through this call and has no fallback. if (!this.isSoloWindow && this.detachedSessions?.has(sessionId)) return false; - // Fit terminal to container before reading dimensions — ensures local - // terminal size matches what we report to the server PTY. - if (this.fitAddon) this.fitAddon.fit(); const dims = this.getTerminalDimensions(); if (!dims) return false; // Did the dimensions actually change since the last resize we sent? Callers diff --git a/test/detached-session-pane-sizing.test.ts b/test/detached-session-pane-sizing.test.ts index 65563691..c30e9eba 100644 --- a/test/detached-session-pane-sizing.test.ts +++ b/test/detached-session-pane-sizing.test.ts @@ -75,9 +75,12 @@ describe('detached sessions own their pane size', () => { const changed = await app.sendResize(SESSION); expect(changed).toBe(false); - // No measurement and no request: the popup's size stands. + // No request: the popup's size stands on the server. expect(fetchMock).not.toHaveBeenCalled(); - expect((app.fitAddon as { fit: ReturnType }).fit).not.toHaveBeenCalled(); + // The LOCAL fit still runs, so the dashboard's own xterm stays correct and + // tab-rail-resize's single settle-time refit is not swallowed. Same line the + // mobile-keyboard guard draws: withhold the send, never the reflow. + expect((app.fitAddon as { fit: ReturnType }).fit).toHaveBeenCalled(); }); it('the solo window still sizes the session it displays', async () => { @@ -97,3 +100,85 @@ describe('detached sessions own their pane size', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); }); + +/** + * `_redock` lives on the CodemanApp class rather than the terminal mixin, so it + * needs app.js loaded. Same `vm` approach as terminal-flush-budget.test.ts. + */ +function loadAppClass() { + const dir = resolve(import.meta.dirname, '../src/web/public'); + const context = vm.createContext({ + console: { ...console, log: vi.fn(), warn: vi.fn(), error: vi.fn() }, + performance: { now: () => 0 }, + setInterval: vi.fn(), + clearInterval: vi.fn(), + setTimeout, + clearTimeout, + requestAnimationFrame: vi.fn(), + HTMLCanvasElement: class HTMLCanvasElement {}, + WebSocket: { OPEN: 1 }, + fetch: vi.fn(), + document: { addEventListener: vi.fn(), getElementById: () => null, querySelector: () => null }, + localStorage: { length: 0, key: vi.fn(), getItem: vi.fn(), setItem: vi.fn(), removeItem: vi.fn() }, + window: { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + MobileDetection: { isTouchDevice: () => false }, + }); + const constants = readFileSync(resolve(dir, 'constants.js'), 'utf8'); + const appSource = readFileSync(resolve(dir, 'app.js'), 'utf8'); + vm.runInContext(`${constants}\n${appSource}\nglobalThis.__CodemanApp = CodemanApp;`, context); + return (context as { __CodemanApp: { prototype: Record } }).__CodemanApp; +} + +describe('redock takes the sizing back', () => { + const CodemanApp = loadAppClass(); + + function makeDashboard(activeSessionId: string | null) { + const app = Object.create(CodemanApp.prototype) as Record; + app.detachedSessions = new Set([SESSION]); + app.detachedWindows = new Map(); + app._detachWatchTimers = new Map(); + app._redockGrace = new Map(); + app._detachOrphanStrikes = new Map(); + app.sessions = new Map([[SESSION, { id: SESSION }]]); + app.activeSessionId = activeSessionId; + app._lastResizeDims = { cols: 120, rows: 40 }; + app.$ = () => null; + app.sendResize = vi.fn(() => Promise.resolve(true)); + return app; + } + + it('clears the stale dimensions so the next send reports truthfully', () => { + // The popup sized the pane while it owned the session, so this window's one + // global record of "what the PTY holds" is wrong. Left in place, the next + // sendResize returns "unchanged" and selectSession skips its redraw wait. + const app = makeDashboard(SESSION); + app._redock(SESSION); + expect(app._lastResizeDims).toBeNull(); + }); + + it('clears them even when the redocked session is not the active one', () => { + // Pop out A, switch to B, close the popup: no resize is due, but the stale + // record still has to go or selecting A later lies about it. + const app = makeDashboard('some-other-session'); + app._redock(SESSION); + expect(app._lastResizeDims).toBeNull(); + expect(app.sendResize).not.toHaveBeenCalled(); + }); + + it('re-asserts this window size for the session it is showing', () => { + const app = makeDashboard(SESSION); + app._redock(SESSION); + expect(app.sendResize).toHaveBeenCalledWith(SESSION, { force: true }); + // Un-marked first, or the yield in sendResize would swallow the re-assert. + expect(app.detachedSessions.has(SESSION)).toBe(false); + }); + + it('sends nothing for a session that is gone', () => { + // _onSessionDeleted redocks before cleanup, so the id can already be dead; + // the resize would be a guaranteed 404. + const app = makeDashboard(SESSION); + app.sessions.delete(SESSION); + app._redock(SESSION); + expect(app.sendResize).not.toHaveBeenCalled(); + }); +});