From 714050fe8a1164c76af511f2763d2801e638492a Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Mon, 28 Sep 2026 16:26:00 +0200 Subject: [PATCH] fix(terminal): merge-time fixes for #494 - Skip and latch a bounded Shell window once the browser is at xterm's scrollback cap (scrollback + rows): a 1 MiB window of short lines can carry more rows than the browser can ever hold, so it replayed and re-captured on every scroll-to-top with no 60 s back-off. - Label a replayed bounded window 'tail' even when the capture was byte-capped, so the banner keeps offering Load full history instead of calling the rest unrecoverable. - Pin GET /terminal?full=1&tail= in the route tests: full-history source, truncationReason 'tail', and the closing relative cursor move survive the cut. - Log the bounded skip via _logScrollRouting('repull-skipped-bounded'). Co-Authored-By: Claude Opus 5.5 (1M context) --- src/web/public/app.js | 24 +++++++-- test/routes/session-routes.test.ts | 47 ++++++++++++++++++ test/shell-scroll-history-pull.test.ts | 67 +++++++++++++++++++++++++- test/terminal-scroll-routing.test.ts | 2 +- 4 files changed, 134 insertions(+), 6 deletions(-) diff --git a/src/web/public/app.js b/src/web/public/app.js index a8bda99f..56997f21 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -6379,18 +6379,29 @@ class CodemanApp { // is left as the load that produced it set it: re-labelling it from this // payload would call a terminal that holds ALL of a Load full history pull // "the most recent 1 MiB". - if (boundedShellPull && windowRows <= this.terminal.buffer.active.length) { + // + // A browser already at xterm's cap buys nothing either. xterm keeps at most + // `scrollback + rows` rows (DEFAULT_SCROLLBACK 50k) while tmux keeps 100k + // lines by default, so a 1 MiB window of short lines can render to more rows + // than the browser can ever hold, and `windowRows <= rowsNow` then never + // comes true: without this every scroll-to-top would reset and re-parse it. + const rowsNow = this.terminal.buffer.active.length; + const scrollbackCap = this.terminal.options?.scrollback || 0; + const browserFull = scrollbackCap > 0 && rowsNow >= scrollbackCap + this.terminal.rows; + if (boundedShellPull && (windowRows <= rowsNow || browserFull)) { // An untruncated window IS all of tmux's history, so nothing is missing, // and the next burst of output can put more in tmux than the browser has: // keep the normal 4 s cooldown. A truncated one is the opposite case, since // the gesture can never reach anything older than what the browser already // shows, and every ask costs the server a synchronous capture-pane of the // whole history (`tail` is applied after the capture): back off to 60 s. + // A full browser backs off too, since no window can ever fit in it. // Trade-off: only a successful replay clears that latch, so a tab switch or // burst that shrinks the browser's buffer below the window can leave a // scroll-to-top inert for up to a minute. Load full history (`force`) // bypasses the cooldown, and the latch is bounded, never permanent. - if (payload.truncated) (this._fullHistoryRepullUseless ||= new Set()).add(sessionId); + if (payload.truncated || browserFull) (this._fullHistoryRepullUseless ||= new Set()).add(sessionId); + this._logScrollRouting?.('repull-skipped-bounded'); return; } if (this._replayWouldShrinkBuffer(buffer, windowRows)) { @@ -6404,7 +6415,14 @@ class CodemanApp { this._setHistoryTruncation(sessionId, { ...payload, exhausted: true }); return; } - this._setHistoryTruncation(sessionId, payload); + // A bounded window that was cut is always recoverable: a capture over the + // byte cap keeps `truncationReason: 'capped'` through the tail cut, and that + // would tell the user the rest "cannot be recovered" and drop Load full + // history, whose unbounded pull returns up to the cap itself. + this._setHistoryTruncation( + sessionId, + boundedShellPull && payload.truncated ? { ...payload, truncationReason: 'tail' } : payload + ); this._fullHistoryRepullUseless?.delete(sessionId); const rowsBefore = this.terminal.buffer.active.length; const replayStartedAt = performance.now(); diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index 2db9dd1d..cc9c9422 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -920,6 +920,53 @@ describe('session-routes', () => { expect(res.headers['server-timing']).toMatch(/^capture;dur=\d+\.\d, prepare;dur=\d+\.\d, total;dur=\d+\.\d$/); }); + it('full reload with a tail (?full=1&tail=) cuts the full capture to its newest bytes, cursor restore intact', async () => { + // A Shell scroll-to-top asks for exactly this (`_maybeRefetchFullHistory`): + // tmux's whole scrollback, bounded to the tab-switch tail size. The client + // relies on all three answers below, so a refactor that dropped the tail on + // a full capture (an unbounded pull from an ordinary scroll) or cut off the + // closing cursor move (a caret parked below the prompt) must fail here. + const tail = 1024 * 1024; + const oldestMarker = 'BOUNDED_OLDEST_LINE_00001'; + const newestMarker = 'BOUNDED_NEWEST_LINE_40000'; + const rows: string[] = [oldestMarker]; + for (let i = 2; i < 40_000; i++) rows.push(`shell history line ${String(i).padStart(5, '0')} lorem ipsum`); + rows.push(newestMarker); + // What formatCursorRestore appends: up from the last row, then the column. + const cursorRestore = '\x1b[3A\r\x1b[2C'; + const fullHistoryCapture = `${rows.join('\r\n')}${cursorRestore}`; + expect(fullHistoryCapture.length).toBeGreaterThan(tail); + + harness.ctx._session.mode = 'shell'; + harness.ctx._session.terminalBuffer = ''; + const captureSpy = vi.fn((_name: string, opts?: { fullHistory?: boolean }) => + opts?.fullHistory ? fullHistoryCapture : 'only the visible frame' + ); + (harness.ctx.mux as { captureActivePaneBuffer?: unknown }).captureActivePaneBuffer = captureSpy; + + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${harness.ctx._sessionId}/terminal?full=1&tail=${tail}`, + }); + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.body); + // Still the scrollback, not the visible frame a plain `?tail=` gets. + expect(captureSpy).toHaveBeenCalledWith( + harness.ctx._session.muxName, + expect.objectContaining({ fullHistory: true }) + ); + expect(body.data.source).toBe('mux-full-history'); + // Recoverable, not 'capped': Load full history can still bring the rest back. + expect(body.data.truncated).toBe(true); + expect(body.data.truncationReason).toBe('tail'); + expect(body.data.fullSize).toBe(fullHistoryCapture.length); + expect(body.data.terminalBuffer.length).toBeLessThanOrEqual(tail); + expect(body.data.terminalBuffer).toContain(newestMarker); + expect(body.data.terminalBuffer).not.toContain(oldestMarker); + expect(body.data.terminalBuffer.endsWith(`${newestMarker}${cursorRestore}`)).toBe(true); + }); + it('full reload (?full=1) returns the tmux capture ALONE — byte history is not duplicated', async () => { // The full-history capture is the rendered form of everything already in // the byte buffer; prepending the byte history would replay the whole diff --git a/test/shell-scroll-history-pull.test.ts b/test/shell-scroll-history-pull.test.ts index ac9de9b0..10b730aa 100644 --- a/test/shell-scroll-history-pull.test.ts +++ b/test/shell-scroll-history-pull.test.ts @@ -12,7 +12,9 @@ * * The gesture now pulls a BOUNDED window (`?full=1&tail=TERMINAL_TAIL_SIZE`), * the button stays the unbounded path, and a window the browser already holds - * in full is not rewritten. + * in full is not rewritten. Neither is one a browser at xterm's scrollback cap + * could never hold, and a window cut from a byte-capped capture is still labelled + * recoverable, since Load full history can reach past it. * * ORDER MATTERS: that skip must run BEFORE the downgrade guard. The guard reads * "smaller than the browser" as "tmux has nothing more to give", which is true of @@ -104,7 +106,12 @@ const TAIL_CUT = { function makeApp( mode: string, - { bufferRows, capture, payload = {} }: { bufferRows: number; capture: string; payload?: Record } + { + bufferRows, + capture, + payload = {}, + scrollback = 0, + }: { bufferRows: number; capture: string; payload?: Record; scrollback?: number } ) { const urls: string[] = []; const app = { @@ -119,6 +126,8 @@ function makeApp( terminal: { cols: 80, rows: 30, + // xterm's scrollback option; 0 leaves the browser-cap check out of a test. + options: { scrollback }, buffer: { active: { length: bufferRows } }, scrollToLine: vi.fn(), scrollToTop: vi.fn(), @@ -183,6 +192,60 @@ describe('shell scroll-up pulls a bounded window of tmux history', () => { expect(app._fullHistoryRepullUseless.has('s1')).toBe(false); // Nothing was written, so the banner state is left exactly as it was. expect(app._setHistoryTruncation).not.toHaveBeenCalled(); + // …but the skip is visible to someone diagnosing "scroll-to-top does nothing". + expect(app._logScrollRouting).toHaveBeenCalledWith('repull-skipped-bounded'); + }); + + it("a browser at xterm's scrollback cap stops replaying a window it can never hold, and backs off", async () => { + // xterm keeps at most `scrollback + rows` rows while tmux keeps 100k lines, so + // a 1 MiB window of short lines can carry more rows than the browser ever will. + // `windowRows <= rows held` then never comes true, and every scroll-to-top + // past the cooldown reset and re-parsed the window. Untruncated on purpose: + // the back-off has to come from the full browser, not from `truncated`. + const { app } = makeApp('shell', { bufferRows: 40, capture: lines(2000), scrollback: 1000 }); + + // The first pull has room to grow, so it replays. + await refetch.call(app); + expect(app._resetTerminalForReplay).toHaveBeenCalledTimes(1); + expect(app._fullHistoryRepullUseless.has('s1')).toBe(false); + // xterm kept only the last `scrollback + rows` of the 2000 rows written. + app.terminal.buffer.active.length = 1000 + 30; + + // Past the 4 s cooldown: the browser is full, so nothing is replayed and the + // session backs off for a minute. + app._fullHistoryRepullAt.set('s1', Date.now() - 5000); + await refetch.call(app); + expect(app._fetchTerminalCapture).toHaveBeenCalledTimes(2); + expect(app._resetTerminalForReplay).toHaveBeenCalledTimes(1); + expect(app.chunkedTerminalWrite).toHaveBeenCalledTimes(1); + expect(app._fullHistoryRepullUseless.has('s1')).toBe(true); + + // So a scroll 10 s later does not even ask the server for another capture. + app._fullHistoryRepullAt.set('s1', Date.now() - 10_000); + await refetch.call(app); + expect(app._fetchTerminalCapture).toHaveBeenCalledTimes(2); + expect(app._resetTerminalForReplay).toHaveBeenCalledTimes(1); + }); + + it('a replayed window cut from a byte-capped capture still offers Load full history', async () => { + // The route keeps `truncationReason: 'capped'` through the tail cut when the + // full capture exceeded the byte cap. On a bounded window that is not "gone for + // good": the unbounded pull behind the button returns up to the cap itself. + const capped = { ...TAIL_CUT, truncationReason: 'capped', fullSize: 40 * 1024 * 1024 }; + const { app } = makeApp('shell', { bufferRows: 40, capture: lines(300), payload: capped }); + + await refetch.call(app); + + expect(app._resetTerminalForReplay).toHaveBeenCalledTimes(1); + expect(app._setHistoryTruncation).toHaveBeenCalledWith('s1', expect.objectContaining({ truncationReason: 'tail' })); + const notice = computeNotice(app._historyTruncation.get('s1') as Record); + expect(notice.visible).toBe(true); + expect(notice.canLoadMore).toBe(true); + + // The button's own unbounded pull is the one place 'capped' is the truth. + const button = makeApp('shell', { bufferRows: 40, capture: lines(300), payload: capped }); + await refetch.call(button.app, { force: true }); + expect(computeNotice(button.app._historyTruncation.get('s1') as Record).canLoadMore).toBe(false); }); }); diff --git a/test/terminal-scroll-routing.test.ts b/test/terminal-scroll-routing.test.ts index d2dd206a..511736a8 100644 --- a/test/terminal-scroll-routing.test.ts +++ b/test/terminal-scroll-routing.test.ts @@ -114,7 +114,7 @@ describe('full-history re-pull downgrade guard (issue #205 round 2)', () => { // Also anchored on the open paren: the guard is handed the rows the caller // already estimated, and this test is about ORDER, not the argument list. const guard = source.indexOf('this._replayWouldShrinkBuffer(buffer', start); - const boundedSkip = source.indexOf('boundedShellPull && windowRows <=', start); + const boundedSkip = source.indexOf('boundedShellPull && (windowRows <= rowsNow || browserFull)', start); const reset = source.indexOf('this._resetTerminalForReplay()', start); expect(start).toBeGreaterThan(-1);