mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
test(terminal): follow the existing suites to the one geometry owner
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
abf1d1f1ca
commit
d9fa9ba1eb
@@ -14,6 +14,12 @@
|
|||||||
* already stood aside on the same condition, so this follows a rule the code
|
* already stood aside on the same condition, so this follows a rule the code
|
||||||
* had already established.
|
* 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
|
* 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
|
* box; see connection-indicator.test.ts), the same way terminal-buffer-flush
|
||||||
* extracts the real mixin methods from terminal-ui.js.
|
* extracts the real mixin methods from terminal-ui.js.
|
||||||
@@ -56,7 +62,13 @@ function makeApp(overrides: Record<string, unknown> = {}) {
|
|||||||
currentFetch = fetchMock;
|
currentFetch = fetchMock;
|
||||||
const app = {
|
const app = {
|
||||||
sendResize: mixin.sendResize,
|
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 }),
|
getTerminalDimensions: () => ({ cols: 120, rows: 40 }),
|
||||||
|
terminal: { cols: 120, rows: 40, resize: vi.fn() },
|
||||||
fitAddon: { fit: vi.fn() },
|
fitAddon: { fit: vi.fn() },
|
||||||
detachedSessions: new Set<string>(),
|
detachedSessions: new Set<string>(),
|
||||||
isSoloWindow: false,
|
isSoloWindow: false,
|
||||||
@@ -77,10 +89,16 @@ describe('detached sessions own their pane size', () => {
|
|||||||
expect(changed).toBe(false);
|
expect(changed).toBe(false);
|
||||||
// No request: the popup's size stands on the server.
|
// No request: the popup's size stands on the server.
|
||||||
expect(fetchMock).not.toHaveBeenCalled();
|
expect(fetchMock).not.toHaveBeenCalled();
|
||||||
// The LOCAL fit still runs, so the dashboard's own xterm stays correct and
|
// ⚠️ REVERSED by issue #464, deliberately. This used to assert that the
|
||||||
// tab-rail-resize's single settle-time refit is not swallowed. Same line the
|
// LOCAL fit still ran — "withhold the send, never the reflow" — on the
|
||||||
// mobile-keyboard guard draws: withhold the send, never the reflow.
|
// reasoning that it keeps the dashboard's own xterm correct. It does not:
|
||||||
expect((app.fitAddon as { fit: ReturnType<typeof vi.fn> }).fit).toHaveBeenCalled();
|
// 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<typeof vi.fn> }).fit).not.toHaveBeenCalled();
|
||||||
|
expect((app.terminal as { resize: ReturnType<typeof vi.fn> }).resize).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
it('the solo window still sizes the session it displays', async () => {
|
it('the solo window still sizes the session it displays', async () => {
|
||||||
|
|||||||
@@ -292,6 +292,10 @@ function loadRealSelectSessionHarness(options: { terminalFailure?: boolean } = {
|
|||||||
app._beginBufferLoad = vi.fn(() => 1);
|
app._beginBufferLoad = vi.fn(() => 1);
|
||||||
app._isLoadingBuffer = false;
|
app._isLoadingBuffer = false;
|
||||||
app.fitAddon = { fit: vi.fn() };
|
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(() => {
|
app.sendResize = vi.fn(() => {
|
||||||
resizeCalls++;
|
resizeCalls++;
|
||||||
return resizeCalls === 1 ? terminalBoundary.promise : Promise.resolve(false);
|
return resizeCalls === 1 ? terminalBoundary.promise : Promise.resolve(false);
|
||||||
|
|||||||
@@ -223,7 +223,12 @@ describe('mobile prompt composer', () => {
|
|||||||
|
|
||||||
it('wires session cleanup to composer draft cleanup', () => {
|
it('wires session cleanup to composer draft cleanup', () => {
|
||||||
const cleanupStart = appSource.indexOf(' _cleanupSessionData(sessionId) {');
|
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)');
|
expect(cleanup).toContain('KeyboardAccessoryBar.discardComposerDraft?.(sessionId)');
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -177,7 +177,11 @@ describe('selectSession font gate', () => {
|
|||||||
|
|
||||||
it('waits for the font before the first fit', () => {
|
it('waits for the font before the first fit', () => {
|
||||||
const wait = body.indexOf('await this._terminalFontReady');
|
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(wait).toBeGreaterThan(-1);
|
||||||
expect(fit).toBeGreaterThan(-1);
|
expect(fit).toBeGreaterThan(-1);
|
||||||
expect(wait).toBeLessThan(fit);
|
expect(wait).toBeLessThan(fit);
|
||||||
|
|||||||
@@ -74,6 +74,18 @@ function makeApp(opts: { teammates?: number; terminal?: ReturnType<typeof fakeTe
|
|||||||
}
|
}
|
||||||
const app = {
|
const app = {
|
||||||
applyTerminalFontWeights: mixin.applyTerminalFontWeights,
|
applyTerminalFontWeights: mixin.applyTerminalFontWeights,
|
||||||
|
// The REAL geometry chain, not stubs. A font change moves the cell size, so
|
||||||
|
// it moves cols/rows, and `applyTerminalFontWeights` now routes its refit
|
||||||
|
// through the one function that floors the result and reports it (#464).
|
||||||
|
// Wiring the real methods keeps `fit` an assertion about what the terminal
|
||||||
|
// actually did rather than about which helper happened to be called.
|
||||||
|
_refitAfterCellSizeChange: mixin._refitAfterCellSizeChange,
|
||||||
|
syncTerminalGeometry: mixin.syncTerminalGeometry,
|
||||||
|
_resizeTerminalTo: mixin._resizeTerminalTo,
|
||||||
|
getTerminalDimensions: mixin.getTerminalDimensions,
|
||||||
|
// No session: `_refitAfterCellSizeChange` then refits locally and sends
|
||||||
|
// nothing, which is what these cases are about.
|
||||||
|
activeSessionId: null,
|
||||||
_awaitTerminalFont: vi.fn(() => Promise.resolve()),
|
_awaitTerminalFont: vi.fn(() => Promise.resolve()),
|
||||||
terminal: opts.terminal === undefined ? fakeTerminal() : opts.terminal,
|
terminal: opts.terminal === undefined ? fakeTerminal() : opts.terminal,
|
||||||
fitAddon: { fit },
|
fitAddon: { fit },
|
||||||
|
|||||||
@@ -40,6 +40,14 @@ function loadKeyboardHandler(opts: { viewportY: number; baseY: number }) {
|
|||||||
const app: any = {
|
const app: any = {
|
||||||
terminal,
|
terminal,
|
||||||
fitAddon: { fit: () => calls.push('fit') },
|
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
|
// The real predicate (terminal-ui.js isTerminalAtBottom), reproduced so the
|
||||||
// test exercises the same tolerance the runtime uses.
|
// test exercises the same tolerance the runtime uses.
|
||||||
isTerminalAtBottom: () => terminal.buffer.active.viewportY >= terminal.buffer.active.baseY - 2,
|
isTerminalAtBottom: () => terminal.buffer.active.viewportY >= terminal.buffer.active.baseY - 2,
|
||||||
@@ -131,7 +139,7 @@ describe('keyboard settle preserves scroll intent (issue #259)', () => {
|
|||||||
kh._scheduleViewportSettle({});
|
kh._scheduleViewportSettle({});
|
||||||
settle();
|
settle();
|
||||||
|
|
||||||
expect(calls).toContain('fit');
|
expect(calls).toContain('syncTerminalGeometry');
|
||||||
expect(calls).not.toContain('scrollToBottom');
|
expect(calls).not.toContain('scrollToBottom');
|
||||||
expect(calls.some((c) => c.startsWith('scrollToLine'))).toBe(false);
|
expect(calls.some((c) => c.startsWith('scrollToLine'))).toBe(false);
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user