From 92dbe6413f30b66b635620f133329b105b747ee2 Mon Sep 17 00:00:00 2001 From: lior Date: Wed, 29 Jul 2026 12:10:04 +0300 Subject: [PATCH 1/2] fix(terminal): retain session replay ownership --- src/web/public/app.js | 6 +++--- src/web/public/terminal-ui.js | 13 ++++++++++--- test/terminal-flush-budget.test.ts | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/web/public/app.js b/src/web/public/app.js index 6f953be8..5278b8f3 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -4359,9 +4359,9 @@ class CodemanApp { bufferWasEmpty = true; } - // Buffer load complete — unblock live SSE writes. chunkedTerminalWrite calls - // _finishBufferLoad internally (discarding queued events to prevent duplicate - // content); if we skipped the write (cache hit or empty), call it here. + // Buffer load complete — unblock live SSE writes. selectSession owns this + // gate across every cache/fetch/replay stage, so its chunked writes leave + // the transaction open and we finish it exactly once here. // COD-144: when the load painted nothing, FLUSH the queued events instead of // discarding — a new session's prompt arrives only as a queued SSE event. if (this._isLoadingBuffer) { diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 3a9de819..ef7ec1df 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -2376,11 +2376,18 @@ Object.assign(CodemanApp.prototype, { // Generation counter: if a newer chunkedTerminalWrite starts (tab switch), // older writes abort instead of continuing to push stale data into the terminal. const writeGen = ++this._chunkedWriteGen; - const bufferLoadOwner = this._beginBufferLoad(loadOwner); + // A caller-provided owner means a wider operation (selectSession) already + // owns the live-output gate. Do not close that transaction after this one + // replay; the caller still has resize/fetch/reconciliation work to finish. + const ownsBufferLoad = loadOwner == null; + const bufferLoadOwner = ownsBufferLoad ? this._beginBufferLoad() : loadOwner; + const finishOwnedBufferLoad = () => { + if (ownsBufferLoad) this._finishBufferLoad(bufferLoadOwner); + }; return new Promise((resolve) => { if (!buffer || buffer.length === 0) { - this._finishBufferLoad(bufferLoadOwner); + finishOwnedBufferLoad(); resolve(); return; } @@ -2392,7 +2399,7 @@ Object.assign(CodemanApp.prototype, { const finish = () => { // Only finish if we're still the active write — a newer write owns buffer load state if (this._chunkedWriteGen === writeGen) { - this._finishBufferLoad(bufferLoadOwner); + finishOwnedBufferLoad(); } resolve(); }; diff --git a/test/terminal-flush-budget.test.ts b/test/terminal-flush-budget.test.ts index 1af721ab..b1695fc7 100644 --- a/test/terminal-flush-budget.test.ts +++ b/test/terminal-flush-budget.test.ts @@ -98,6 +98,20 @@ describe('terminal flush budget', () => { expect(finishBufferLoad).toHaveBeenCalledOnce(); }); + it('leaves a caller-owned buffer load open after replaying a chunk', async () => { + const { app } = loadTerminalUiHarness('codex'); + const beginBufferLoad = vi.fn(() => 'nested-owner'); + const finishBufferLoad = vi.fn(); + app._beginBufferLoad = beginBufferLoad; + app._finishBufferLoad = finishBufferLoad; + app.terminal.write = (_data: string, callback?: () => void) => callback?.(); + + await app.chunkedTerminalWrite('cached frame', 32 * 1024, 'select-owner'); + + expect(beginBufferLoad).not.toHaveBeenCalled(); + expect(finishBufferLoad).not.toHaveBeenCalled(); + }); + it('keeps stale buffer load owners from finishing a newer load', () => { const { app } = loadTerminalUiHarness('codex'); From 3bdc4eded36c8899d2120e11a08edac2e33b569f Mon Sep 17 00:00:00 2001 From: lior Date: Wed, 29 Jul 2026 12:11:06 +0300 Subject: [PATCH 2/2] fix(terminal): keep manual history scroll anchored --- src/web/public/terminal-ui.js | 50 ++++++++++++++---------------- test/terminal-flush-budget.test.ts | 24 ++++++++++++-- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index ef7ec1df..80e4d2d3 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -15,10 +15,6 @@ (function (global) { const TERMINAL_QUERY_RESPONSE_PATTERN = /^\x1b\[[\?>=]?[\d;]*[cnR]$/; const TERMINAL_OSC_RESPONSE_PATTERN = /^\x1b\][\d;]*[^\x07\x1b]*(?:\x07|\x1b\\)$/; - // Grace window after a manual scroll-up gesture during which sticky-scroll is - // suppressed, so high-frequency Codex status redraws don't snap the viewport - // back to the bottom while the user is inspecting earlier output. - const USER_SCROLL_STICKY_SUPPRESS_MS = 1500; // Mobile browsers synthesize trusted mouse events after touchend. During this // short window, only the app's synthetic tap-to-position mouse event should // reach xterm. @@ -60,7 +56,6 @@ global.CodemanTerminalInput = { isTerminalQueryResponse, shouldSuppressTerminalQueryResponse, - USER_SCROLL_STICKY_SUPPRESS_MS, TOUCH_COMPAT_MOUSE_SUPPRESS_MS, }; global.CODEMAN_XTERM_THEMES = CODEMAN_XTERM_THEMES; @@ -403,8 +398,7 @@ Object.assign(CodemanApp.prototype, { this._sendSyntheticSgrWheel(ev.clientX, ev.clientY, lines); return; } - this._noteTerminalUserScroll(lines); - this.terminal.scrollLines(lines); + this._scrollTerminalLines(lines); }, { passive: false } ); @@ -429,7 +423,7 @@ Object.assign(CodemanApp.prototype, { if (!isTouching && Math.abs(velocity) > 0.3) { // Momentum phase — convert pixel velocity to lines const lines = Math.round(velocity / cellHeight()); - if (lines !== 0) this.terminal.scrollLines(lines); + if (lines !== 0) this._scrollTerminalLines(lines); velocity *= 0.92; scrollFrame = requestAnimationFrame(scrollLoop); } else if (!isTouching) { @@ -490,8 +484,7 @@ Object.assign(CodemanApp.prototype, { const ch = cellHeight(); const lines = Math.trunc(pixelAccum / ch); if (lines !== 0) { - this._noteTerminalUserScroll(lines); - this.terminal.scrollLines(lines); + this._scrollTerminalLines(lines); pixelAccum -= lines * ch; } } @@ -580,7 +573,7 @@ Object.assign(CodemanApp.prototype, { this._chunkedWriteGen = 0; this._bufferLoadSeq = 0; this._bufferLoadOwner = null; - this._lastUserScrollUpAt = null; + this._terminalScrollLocked = false; // Handle resize with throttling for performance this._resizeTimeout = null; @@ -1959,20 +1952,25 @@ Object.assign(CodemanApp.prototype, { return buffer.viewportY >= buffer.baseY - 2; }, - // Record manual scroll gestures so sticky-scroll can give an upward scroll a - // short grace window (see _hasRecentUserScrollUp). A downward scroll that - // lands back at the bottom clears the suppression immediately. + // Keep history anchored after an upward gesture until the reader explicitly + // returns to the bottom. A timer is insufficient here: long-running terminal + // output used to reclaim the viewport after 1.5s while the user was reading. _noteTerminalUserScroll(lines) { if (lines < 0) { - this._lastUserScrollUpAt = performance.now(); + this._terminalScrollLocked = true; } else if (this.isTerminalAtBottom()) { - this._lastUserScrollUpAt = null; + this._terminalScrollLocked = false; } }, - _hasRecentUserScrollUp() { - if (typeof this._lastUserScrollUpAt !== 'number') return false; - return performance.now() - this._lastUserScrollUpAt < window.CodemanTerminalInput.USER_SCROLL_STICKY_SUPPRESS_MS; + _scrollTerminalLines(lines) { + if (!lines || !this.terminal) return; + this.terminal.scrollLines(lines); + this._noteTerminalUserScroll(lines); + }, + + _shouldPreserveTerminalScroll() { + return this._terminalScrollLocked === true; }, batchTerminalWrite(data) { @@ -2176,10 +2174,12 @@ Object.assign(CodemanApp.prototype, { const activeSession = this.activeSessionId && this.sessions ? this.sessions.get(this.activeSessionId) : null; const MAX_FRAME_BYTES = activeSession?.mode === 'codex' ? 32768 : 65536; let deferred = false; - // If the user recently scrolled up, remember the viewport so we can restore - // it after the write — Codex status redraws would otherwise jump it. + // While the reader owns history, remember the viewport so Codex status + // redraws cannot move it. const preserveViewportY = - this._hasRecentUserScrollUp() && this.terminal.buffer?.active ? this.terminal.buffer.active.viewportY : null; + this._shouldPreserveTerminalScroll() && this.terminal.buffer?.active + ? this.terminal.buffer.active.viewportY + : null; if (_joinedLen <= MAX_FRAME_BYTES) { this.terminal.write(joined); @@ -2211,10 +2211,8 @@ Object.assign(CodemanApp.prototype, { ); // Sticky scroll: if user was at bottom, keep them there after new output. - // Give manual scroll-up gestures a short grace window so high-frequency - // Codex status ticks do not snap the viewport back while the user is - // trying to inspect earlier output. - if (this._wasAtBottomBeforeWrite && !this._hasRecentUserScrollUp()) { + // A manual history scroll owns the viewport until it reaches the bottom. + if (this._wasAtBottomBeforeWrite && !this._shouldPreserveTerminalScroll()) { this.terminal.scrollToBottom(); } diff --git a/test/terminal-flush-budget.test.ts b/test/terminal-flush-budget.test.ts index b1695fc7..5e1dcd19 100644 --- a/test/terminal-flush-budget.test.ts +++ b/test/terminal-flush-budget.test.ts @@ -127,12 +127,12 @@ describe('terminal flush budget', () => { expect(app._bufferLoadOwner).toBe(null); }); - it('does not snap back to bottom during Codex Working redraws right after the user scrolls up', () => { + it('does not snap back to bottom during Codex Working redraws while history is scroll-locked', () => { const { app } = loadTerminalUiHarness('codex'); const scrollToBottom = vi.fn(); app.terminal.scrollToBottom = scrollToBottom; app._wasAtBottomBeforeWrite = true; - app._lastUserScrollUpAt = 0; + app._terminalScrollLocked = true; app.pendingWrites.push('\x1b[55;1H\x1b[2m• Working (6s)'); app.flushPendingWrites(); @@ -151,11 +151,29 @@ describe('terminal flush budget', () => { buffer.viewportY = line; }); app._wasAtBottomBeforeWrite = true; - app._lastUserScrollUpAt = 0; + app._terminalScrollLocked = true; app.pendingWrites.push('\x1b[55;1H\x1b[2m• Working (6s)'); app.flushPendingWrites(); expect(buffer.viewportY).toBe(40); }); + + it('keeps the history lock until a downward scroll actually reaches the bottom', () => { + const { app } = loadTerminalUiHarness('codex'); + const buffer = { viewportY: 40, baseY: 50 }; + app.terminal.buffer = { active: buffer }; + app.terminal.scrollLines = vi.fn((lines: number) => { + buffer.viewportY = Math.max(0, Math.min(buffer.baseY, buffer.viewportY + lines)); + }); + + app._scrollTerminalLines(-2); + expect(app._shouldPreserveTerminalScroll()).toBe(true); + + app._scrollTerminalLines(2); + expect(app._shouldPreserveTerminalScroll()).toBe(true); + + app._scrollTerminalLines(20); + expect(app._shouldPreserveTerminalScroll()).toBe(false); + }); });