From d9fa9ba1eb448f7d45365b119cd62260b8de878b Mon Sep 17 00:00:00 2001 From: Rounak Datta Date: Tue, 22 Sep 2026 13:22:28 +0530 Subject: [PATCH] test(terminal): follow the existing suites to the one geometry owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate caught fourteen failures the focused tests could not: every harness that builds a partial app out of cherry-picked mixin methods, and every source guard that named `fitAddon.fit()` by hand. Most are wiring — `syncTerminalGeometry`, `_refitAfterCellSizeChange` and `_resizeTerminalTo` added to the fakes so the real chain runs rather than a stub of it. `file-browser-search` is the one that shows why it matters: without the method on the fake, selectSession's unconditional call threw into its own catch and every later assertion in the file measured a load that never happened. Two are not wiring. `detached-session-pane-sizing` pinned the behaviour this change deliberately reverses. It asserted the LOCAL fit still runs for a session owned by its own window — "withhold the send, never the reflow" — so the assertion is restated rather than patched, with the reason beside it and in the file's docblock: a reflow the PTY is never told about leaves this xterm rendering a CLI's frames against a shape that does not exist, and the popup that owns the pane is drawing for its own width regardless. The old rule bought a garbled frame, not a correct one. `mobile-prompt-composer` sliced `_cleanupSessionData` as a fixed 1200-character window, so the assertion depended on how much unrelated code sat above the line it cared about. It reads the whole method now. `terminal-scroll-intent` records `syncTerminalGeometry` rather than `fit`, under its own name: recording a bare fit there would name the very thing the subject was changed to stop doing. Co-Authored-By: Claude Opus 5 (1M context) --- test/detached-session-pane-sizing.test.ts | 26 +++++++++++++++++++---- test/file-browser-search.test.ts | 4 ++++ test/mobile-prompt-composer.test.ts | 7 +++++- test/terminal-font-settle.test.ts | 6 +++++- test/terminal-font-weight.test.ts | 12 +++++++++++ test/terminal-scroll-intent.test.ts | 10 ++++++++- 6 files changed, 58 insertions(+), 7 deletions(-) diff --git a/test/detached-session-pane-sizing.test.ts b/test/detached-session-pane-sizing.test.ts index c30e9eba..10b7e4a6 100644 --- a/test/detached-session-pane-sizing.test.ts +++ b/test/detached-session-pane-sizing.test.ts @@ -14,6 +14,12 @@ * already stood aside on the same condition, so this follows a rule the code * had already established. * + * ⚠️ It returns early BEFORE the local fit, not after (issue #464). The earlier + * rule was "withhold the send, never the reflow", which leaves this window's + * xterm at a shape the PTY was never told about — and a CLI computes its + * repaints from the shape it was told, so that reflow bought a garbled frame + * rather than a correct one. Withhold both, or neither. + * * Loaded via `vm` with a stubbed context (no jsdom — jsdom is broken on this * box; see connection-indicator.test.ts), the same way terminal-buffer-flush * extracts the real mixin methods from terminal-ui.js. @@ -56,7 +62,13 @@ function makeApp(overrides: Record = {}) { currentFetch = fetchMock; const app = { sendResize: mixin.sendResize, + // The real chain: sendResize fits, floors and applies through one function + // now, so the harness must let it (#464). + syncTerminalGeometry: mixin.syncTerminalGeometry, + _resizeTerminalTo: mixin._resizeTerminalTo, + _onPtyGeometryReport: vi.fn(), getTerminalDimensions: () => ({ cols: 120, rows: 40 }), + terminal: { cols: 120, rows: 40, resize: vi.fn() }, fitAddon: { fit: vi.fn() }, detachedSessions: new Set(), isSoloWindow: false, @@ -77,10 +89,16 @@ describe('detached sessions own their pane size', () => { expect(changed).toBe(false); // No request: the popup's size stands on the server. expect(fetchMock).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(); + // ⚠️ REVERSED by issue #464, deliberately. This used to assert that the + // LOCAL fit still ran — "withhold the send, never the reflow" — on the + // reasoning that it keeps the dashboard's own xterm correct. It does not: + // it leaves this xterm at a shape the PTY was never told about, and Claude + // Code computes every repaint from the shape it WAS told, so the frames + // land on rows nothing erased. The popup that owns the PTY is drawing for + // its own width either way, so the dashboard's reflow was a reflow nothing + // was rendering for. Withholding the resize means withholding all of it. + expect((app.fitAddon as { fit: ReturnType }).fit).not.toHaveBeenCalled(); + expect((app.terminal as { resize: ReturnType }).resize).not.toHaveBeenCalled(); }); it('the solo window still sizes the session it displays', async () => { diff --git a/test/file-browser-search.test.ts b/test/file-browser-search.test.ts index d6e95a58..81519b26 100644 --- a/test/file-browser-search.test.ts +++ b/test/file-browser-search.test.ts @@ -292,6 +292,10 @@ function loadRealSelectSessionHarness(options: { terminalFailure?: boolean } = { app._beginBufferLoad = vi.fn(() => 1); app._isLoadingBuffer = false; app.fitAddon = { fit: vi.fn() }; + // selectSession fits through the one owner now, which also applies the floor + // it reports to the PTY (#464). Without it on the fake, the unconditional + // call throws into selectSession's catch and nothing after it runs. + app.syncTerminalGeometry = vi.fn(() => ({ cols: 120, rows: 40 })); app.sendResize = vi.fn(() => { resizeCalls++; return resizeCalls === 1 ? terminalBoundary.promise : Promise.resolve(false); diff --git a/test/mobile-prompt-composer.test.ts b/test/mobile-prompt-composer.test.ts index 9cf26f69..8429740d 100644 --- a/test/mobile-prompt-composer.test.ts +++ b/test/mobile-prompt-composer.test.ts @@ -223,7 +223,12 @@ describe('mobile prompt composer', () => { it('wires session cleanup to composer draft cleanup', () => { const cleanupStart = appSource.indexOf(' _cleanupSessionData(sessionId) {'); - const cleanup = appSource.slice(cleanupStart, cleanupStart + 1200); + expect(cleanupStart, '_cleanupSessionData not found — renamed?').toBeGreaterThan(-1); + // The whole method, not a fixed byte window. A 1200-character slice made + // this assertion depend on how much OTHER code sat above the line it cares + // about, so an unrelated addition near the top of the method failed it. + const cleanup = appSource.slice(cleanupStart, appSource.indexOf('\n }\n', cleanupStart)); + expect(cleanup.length, 'method body did not terminate').toBeGreaterThan(0); expect(cleanup).toContain('KeyboardAccessoryBar.discardComposerDraft?.(sessionId)'); }); diff --git a/test/terminal-font-settle.test.ts b/test/terminal-font-settle.test.ts index b92960fc..ee66e334 100644 --- a/test/terminal-font-settle.test.ts +++ b/test/terminal-font-settle.test.ts @@ -177,7 +177,11 @@ describe('selectSession font gate', () => { it('waits for the font before the first fit', () => { const wait = body.indexOf('await this._terminalFontReady'); - const fit = body.indexOf('if (this.fitAddon) this.fitAddon.fit();'); + // `syncTerminalGeometry()` replaced the bare `fitAddon.fit()` here: it fits + // AND applies the floor it reports, so xterm and the PTY cannot disagree + // (#464). The gate this test guards is unchanged — the font must be + // measured before the terminal is. + const fit = body.indexOf('this.syncTerminalGeometry();'); expect(wait).toBeGreaterThan(-1); expect(fit).toBeGreaterThan(-1); expect(wait).toBeLessThan(fit); diff --git a/test/terminal-font-weight.test.ts b/test/terminal-font-weight.test.ts index 14605ef7..e04efc6e 100644 --- a/test/terminal-font-weight.test.ts +++ b/test/terminal-font-weight.test.ts @@ -74,6 +74,18 @@ function makeApp(opts: { teammates?: number; terminal?: ReturnType Promise.resolve()), terminal: opts.terminal === undefined ? fakeTerminal() : opts.terminal, fitAddon: { fit }, diff --git a/test/terminal-scroll-intent.test.ts b/test/terminal-scroll-intent.test.ts index 293605ee..85af51aa 100644 --- a/test/terminal-scroll-intent.test.ts +++ b/test/terminal-scroll-intent.test.ts @@ -40,6 +40,14 @@ function loadKeyboardHandler(opts: { viewportY: number; baseY: number }) { const app: any = { terminal, fitAddon: { fit: () => calls.push('fit') }, + // The settle refits through the one function that also applies the floor + // it reports (#464), so that is what the fake has to offer. Recorded under + // its own name rather than 'fit': a bare fit here would be the divergence + // this test's subject was changed to avoid. + syncTerminalGeometry: () => { + calls.push('syncTerminalGeometry'); + return { cols: 80, rows: 24 }; + }, // The real predicate (terminal-ui.js isTerminalAtBottom), reproduced so the // test exercises the same tolerance the runtime uses. isTerminalAtBottom: () => terminal.buffer.active.viewportY >= terminal.buffer.active.baseY - 2, @@ -131,7 +139,7 @@ describe('keyboard settle preserves scroll intent (issue #259)', () => { kh._scheduleViewportSettle({}); settle(); - expect(calls).toContain('fit'); + expect(calls).toContain('syncTerminalGeometry'); expect(calls).not.toContain('scrollToBottom'); expect(calls.some((c) => c.startsWith('scrollToLine'))).toBe(false); });