diff --git a/packages/codev/dashboard/__tests__/escapeBuffer.test.ts b/packages/codev/dashboard/__tests__/escapeBuffer.test.ts new file mode 100644 index 000000000..0e4d22890 --- /dev/null +++ b/packages/codev/dashboard/__tests__/escapeBuffer.test.ts @@ -0,0 +1,205 @@ +/** + * Unit tests for EscapeBuffer — ensures escape sequences split across + * WebSocket frames are reassembled before writing to xterm (Issue #630). + */ +import { describe, it, expect } from 'vitest'; +import { EscapeBuffer } from '../src/lib/escapeBuffer.js'; + +describe('EscapeBuffer', () => { + describe('plain text (no escape sequences)', () => { + it('passes through plain text unchanged', () => { + const buf = new EscapeBuffer(); + expect(buf.write('Hello, world!')).toBe('Hello, world!'); + }); + + it('passes through empty string', () => { + const buf = new EscapeBuffer(); + expect(buf.write('')).toBe(''); + }); + }); + + describe('complete escape sequences', () => { + it('passes through complete CSI sequence', () => { + const buf = new EscapeBuffer(); + expect(buf.write('\x1b[31mHello')).toBe('\x1b[31mHello'); + }); + + it('passes through multiple complete CSI sequences', () => { + const buf = new EscapeBuffer(); + expect(buf.write('\x1b[31mHello\x1b[0m')).toBe('\x1b[31mHello\x1b[0m'); + }); + + it('passes through complete OSC sequence with BEL', () => { + const buf = new EscapeBuffer(); + expect(buf.write('\x1b]0;title\x07')).toBe('\x1b]0;title\x07'); + }); + + it('passes through complete OSC sequence with ST', () => { + const buf = new EscapeBuffer(); + expect(buf.write('\x1b]0;title\x1b\\')).toBe('\x1b]0;title\x1b\\'); + }); + + it('passes through two-byte escape sequences (ESC =, ESC >)', () => { + const buf = new EscapeBuffer(); + expect(buf.write('\x1b=Hello')).toBe('\x1b=Hello'); + expect(buf.write('\x1b>World')).toBe('\x1b>World'); + }); + }); + + describe('split CSI sequences', () => { + it('buffers trailing ESC at end of chunk', () => { + const buf = new EscapeBuffer(); + // Frame 1: text ending with bare ESC + const out1 = buf.write('Hello\x1b'); + expect(out1).toBe('Hello'); + expect(buf.hasPending).toBe(true); + + // Frame 2: continuation completes the sequence + const out2 = buf.write('[31mWorld'); + expect(out2).toBe('\x1b[31mWorld'); + expect(buf.hasPending).toBe(false); + }); + + it('buffers incomplete CSI (ESC [) at end of chunk', () => { + const buf = new EscapeBuffer(); + const out1 = buf.write('Hello\x1b['); + expect(out1).toBe('Hello'); + + const out2 = buf.write('31m'); + expect(out2).toBe('\x1b[31m'); + }); + + it('buffers incomplete CSI with partial parameters', () => { + const buf = new EscapeBuffer(); + // Split mid-sequence: ESC [ 3 (missing final byte) + const out1 = buf.write('Hello\x1b[3'); + expect(out1).toBe('Hello'); + + const out2 = buf.write('1mWorld'); + expect(out2).toBe('\x1b[31mWorld'); + }); + + it('handles DA response split across frames', () => { + const buf = new EscapeBuffer(); + // DA response: ESC [ ? 6 c — split after ESC [ ? 6 + const out1 = buf.write('Hello\x1b[?6'); + expect(out1).toBe('Hello'); + + const out2 = buf.write('cWorld'); + expect(out2).toBe('\x1b[?6cWorld'); + }); + + it('handles multiple splits in sequence', () => { + const buf = new EscapeBuffer(); + // First split + const out1 = buf.write('A\x1b'); + expect(out1).toBe('A'); + + // Second chunk also has split + const out2 = buf.write('[31mB\x1b[3'); + expect(out2).toBe('\x1b[31mB'); + + // Third chunk completes + const out3 = buf.write('2mC'); + expect(out3).toBe('\x1b[32mC'); + }); + }); + + describe('split OSC sequences', () => { + it('buffers incomplete OSC', () => { + const buf = new EscapeBuffer(); + const out1 = buf.write('Hello\x1b]0;tit'); + expect(out1).toBe('Hello'); + + const out2 = buf.write('le\x07World'); + expect(out2).toBe('\x1b]0;title\x07World'); + }); + }); + + describe('split DCS sequences', () => { + it('buffers incomplete DCS', () => { + const buf = new EscapeBuffer(); + const out1 = buf.write('Hello\x1bPdata'); + expect(out1).toBe('Hello'); + + const out2 = buf.write('more\x1b\\World'); + expect(out2).toBe('\x1bPdatamore\x1b\\World'); + }); + }); + + describe('flush', () => { + it('returns empty string when no pending data', () => { + const buf = new EscapeBuffer(); + expect(buf.flush()).toBe(''); + }); + + it('returns and clears pending data', () => { + const buf = new EscapeBuffer(); + buf.write('Hello\x1b'); + expect(buf.flush()).toBe('\x1b'); + expect(buf.hasPending).toBe(false); + }); + + it('prevents stale pending bytes from leaking into next stream (reconnect scenario)', () => { + const buf = new EscapeBuffer(); + // Connection 1: data ends with incomplete escape + buf.write('Hello\x1b[3'); + expect(buf.hasPending).toBe(true); + + // Disconnect — flush discards stale bytes + buf.flush(); + expect(buf.hasPending).toBe(false); + + // Connection 2: fresh data should not be contaminated + const out = buf.write('World\x1b[31m!'); + expect(out).toBe('World\x1b[31m!'); + }); + }); + + describe('edge cases', () => { + it('handles chunk that is just ESC', () => { + const buf = new EscapeBuffer(); + const out = buf.write('\x1b'); + expect(out).toBe(''); + expect(buf.hasPending).toBe(true); + }); + + it('handles chunk that is just ESC [', () => { + const buf = new EscapeBuffer(); + const out = buf.write('\x1b['); + expect(out).toBe(''); + expect(buf.hasPending).toBe(true); + }); + + it('does not buffer complete sequence followed by text', () => { + const buf = new EscapeBuffer(); + const out = buf.write('\x1b[31mHello'); + expect(out).toBe('\x1b[31mHello'); + expect(buf.hasPending).toBe(false); + }); + + it('only buffers the LAST incomplete sequence', () => { + const buf = new EscapeBuffer(); + // Complete CSI + incomplete CSI at end + const out = buf.write('\x1b[31mHello\x1b['); + expect(out).toBe('\x1b[31mHello'); + expect(buf.hasPending).toBe(true); + }); + + it('handles box-drawing characters (code 0x2500) that trigger issue #630', () => { + // Box-drawing ─ (U+2500) is what the bug report mentioned: + // "Code 9472 = 0x2500 (box-drawing ─), currentState: 4 = escape state" + // This happens when ─ appears after a split ESC sequence + const buf = new EscapeBuffer(); + const boxChar = String.fromCharCode(0x2500); // ─ + + // ESC split before box-drawing char — without buffering, xterm would + // receive ESC then ─ in escape state, causing parsing error + const out1 = buf.write('test\x1b'); + expect(out1).toBe('test'); + + const out2 = buf.write(`[1m${boxChar}${boxChar}${boxChar}`); + expect(out2).toBe(`\x1b[1m${boxChar}${boxChar}${boxChar}`); + }); + }); +}); diff --git a/packages/codev/dashboard/__tests__/scrollController.test.ts b/packages/codev/dashboard/__tests__/scrollController.test.ts index f50032406..ad15323bf 100644 --- a/packages/codev/dashboard/__tests__/scrollController.test.ts +++ b/packages/codev/dashboard/__tests__/scrollController.test.ts @@ -336,7 +336,7 @@ describe('ScrollController', () => { expect(ctrl.state.wasAtBottom).toBe(true); }); - it('warns on unexpected scroll-to-top in interactive phase', () => { + it('warns on unexpected scroll-to-top but does not auto-correct (Issue #630)', () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); const { ctrl, term } = createController(); ctrl.enterInteractive(); @@ -350,10 +350,17 @@ describe('ScrollController', () => { term.buffer.active.viewportY = 0; term._triggerScroll(); + // Should warn but NOT auto-correct — viewportY=0 is also the normal + // state when a user intentionally scrolls to the top of history. + // Root causes are prevented upstream by EscapeBuffer and WebGL handler. expect(warnSpy).toHaveBeenCalledWith( expect.stringContaining('unexpected scroll-to-top'), expect.any(String), ); + // State IS updated (no correction, user may be at top intentionally) + expect(ctrl.state.viewportY).toBe(0); + expect(term.scrollToLine).not.toHaveBeenCalled(); + expect(term.scrollToBottom).not.toHaveBeenCalled(); warnSpy.mockRestore(); }); }); diff --git a/packages/codev/dashboard/src/components/Terminal.tsx b/packages/codev/dashboard/src/components/Terminal.tsx index 456aa66ef..3810a829d 100644 --- a/packages/codev/dashboard/src/components/Terminal.tsx +++ b/packages/codev/dashboard/src/components/Terminal.tsx @@ -11,6 +11,7 @@ import { useMediaQuery } from '../hooks/useMediaQuery.js'; import { MOBILE_BREAKPOINT } from '../lib/constants.js'; import { uploadPasteImage } from '../lib/api.js'; import { ScrollController } from '../lib/scrollController.js'; +import { EscapeBuffer } from '../lib/escapeBuffer.js'; /** * Floating controls overlay for terminal windows — refresh (re-fit + resize) @@ -246,8 +247,21 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi try { const webglAddon = new WebglAddon(); webglAddon.onContextLoss(() => { + // Save scroll position before renderer transition (Issue #630). + // WebGL context loss resets xterm's viewport, causing scroll-to-top. + const savedViewportY = term.buffer?.active?.viewportY ?? 0; + const savedBaseY = term.buffer?.active?.baseY ?? 0; + const wasAtBottom = !savedBaseY || savedViewportY >= savedBaseY; + webglAddon.dispose(); loadCanvasFallback(); + + // Restore scroll position after switching to canvas renderer + if (wasAtBottom) { + term.scrollToBottom(); + } else if (savedViewportY > 0) { + term.scrollToLine(savedViewportY); + } }); term.loadAddon(webglAddon); } catch { @@ -375,6 +389,10 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi return text; }; + // Buffer incomplete escape sequences to prevent xterm parsing errors + // from split WebSocket frames causing scroll-to-top (Issue #630). + const escBuf = new EscapeBuffer(); + /** Create a WebSocket connection, optionally resuming from a sequence number. */ const connect = (resumeSeq?: number) => { const wsUrl = resumeSeq !== undefined ? `${wsBase}?resume=${resumeSeq}` : wsBase; @@ -382,12 +400,15 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi ws.binaryType = 'arraybuffer'; wsRef.current = ws; - // Reset DA filter state and scroll controller for this connection. - // On reconnection, the controller needs to return to initial-load so - // beginReplay()/endReplay() can properly suppress fit during replay. + // Reset DA filter state, escape buffer, and scroll controller for this + // connection. On reconnection, the controller needs to return to + // initial-load so beginReplay()/endReplay() can properly suppress fit + // during replay. Flush escape buffer to discard stale pending bytes + // from the previous connection (CMAP feedback, Issue #630). rc.initialPhase = true; rc.initialBuffer = ''; if (rc.flushTimer) { clearTimeout(rc.flushTimer); rc.flushTimer = null; } + escBuf.flush(); scrollCtrl.reset(); const flushInitialBuffer = () => { @@ -457,20 +478,12 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi rc.flushTimer = setTimeout(flushInitialBuffer, 500); } } else { - const filtered = filterDA(text); - if (filtered) { - // Capture scroll position before write — term.write() can cause - // buffer reflow that resets viewportY to 0 without firing onScroll. - const preWriteViewportY = term.buffer?.active?.viewportY ?? 0; - const preWriteBaseY = term.buffer?.active?.baseY ?? 0; - term.write(filtered, () => { - const postViewportY = term.buffer?.active?.viewportY ?? 0; - if (postViewportY === 0 && preWriteViewportY > 0 && preWriteBaseY > 0) { - console.warn('[Terminal] write caused scroll-to-top, correcting:', - `was viewportY=${preWriteViewportY}, baseY=${preWriteBaseY}`); - scrollCtrl.scrollToBottom(); - } - }); + // Buffer through EscapeBuffer first (ensures complete escape + // sequences), then strip DA responses before writing to xterm. + const complete = escBuf.write(text); + if (complete) { + const filtered = filterDA(complete); + if (filtered) term.write(filtered); } } } else if (prefix === FRAME_CONTROL) { diff --git a/packages/codev/dashboard/src/lib/escapeBuffer.ts b/packages/codev/dashboard/src/lib/escapeBuffer.ts new file mode 100644 index 000000000..456554107 --- /dev/null +++ b/packages/codev/dashboard/src/lib/escapeBuffer.ts @@ -0,0 +1,94 @@ +/** + * EscapeBuffer — accumulates PTY data and ensures escape sequences + * are never written to xterm split across WebSocket frames. + * + * When data arrives, any trailing incomplete escape sequence is held + * back and prepended to the next chunk. This prevents xterm parsing + * errors that cause scroll-to-top (Issue #630). + */ +export class EscapeBuffer { + private pending = ''; + + /** + * Add incoming data and return the portion safe to write to xterm. + * Any trailing incomplete escape sequence is buffered internally. + */ + write(data: string): string { + data = this.pending + data; + this.pending = ''; + + // Find the last ESC in the data + const lastEsc = data.lastIndexOf('\x1b'); + if (lastEsc === -1) return data; + + // Check if the escape sequence starting at lastEsc is complete + const tail = data.substring(lastEsc); + if (isCompleteEscape(tail)) return data; + + // Incomplete escape sequence at the end — buffer it + this.pending = tail; + return data.substring(0, lastEsc); + } + + /** + * Flush any pending data (e.g., on disconnect or cleanup). + * Returns empty string if nothing is pending. + */ + flush(): string { + const data = this.pending; + this.pending = ''; + return data; + } + + /** Whether there is buffered data waiting for completion. */ + get hasPending(): boolean { + return this.pending.length > 0; + } +} + +/** + * Check if an escape sequence (starting with ESC) is complete. + * Returns false for incomplete sequences that need more data. + */ +function isCompleteEscape(seq: string): boolean { + if (seq.length < 2) return false; // Just ESC — need more data + + const second = seq.charCodeAt(1); + + // CSI: ESC [ + if (second === 0x5b) { + for (let i = 2; i < seq.length; i++) { + const c = seq.charCodeAt(i); + if (c >= 0x40 && c <= 0x7e) return true; // Final byte found + // Parameter bytes (0x30-0x3f) and intermediate bytes (0x20-0x2f) continue + if ((c >= 0x20 && c <= 0x3f)) continue; + // Unexpected byte — treat as complete to avoid infinite buffering + return true; + } + return false; // No final byte yet + } + + // OSC: ESC ] ... BEL(0x07) or ST(ESC \) + if (second === 0x5d) { + for (let i = 2; i < seq.length; i++) { + if (seq.charCodeAt(i) === 0x07) return true; + if (seq.charCodeAt(i) === 0x1b && i + 1 < seq.length && seq.charCodeAt(i + 1) === 0x5c) return true; + } + return false; + } + + // DCS(P), APC(_), PM(^), SOS(X): ESC ... ST(ESC \) + if (second === 0x50 || second === 0x5f || second === 0x5e || second === 0x58) { + for (let i = 2; i < seq.length; i++) { + if (seq.charCodeAt(i) === 0x1b && i + 1 < seq.length && seq.charCodeAt(i + 1) === 0x5c) return true; + } + return false; + } + + // Two-byte sequences: ESC + single character (0x20-0x7e) + // Includes: ESC =, ESC >, ESC 7, ESC 8, ESC M, etc. + if (second >= 0x20 && second <= 0x7e) return true; + + // Anything else — treat as complete to avoid infinite buffering + return true; +} diff --git a/packages/codev/dashboard/src/lib/scrollController.ts b/packages/codev/dashboard/src/lib/scrollController.ts index 8d21a802c..ab26b6a19 100644 --- a/packages/codev/dashboard/src/lib/scrollController.ts +++ b/packages/codev/dashboard/src/lib/scrollController.ts @@ -237,28 +237,14 @@ export class ScrollController { const baseY = this.term.buffer?.active?.baseY ?? 0; const viewportY = this.term.buffer?.active?.viewportY ?? 0; - // Detect and correct unexpected scroll-to-top in interactive phase. - // xterm can internally reset viewportY to 0 during buffer reflow - // (new output, fit, etc.). Restore the previous position. + // Detect unexpected scroll-to-top in interactive phase (diagnostic only). + // Root causes (split escape sequences, WebGL context loss) are prevented + // upstream by EscapeBuffer and the context loss handler (Issue #630). + // We don't auto-correct here because viewportY=0 is also the normal state + // when a user intentionally scrolls to the top of their history. if (viewportY === 0 && baseY > 0 && this._viewportY > 0) { - console.warn('[ScrollController] correcting unexpected scroll-to-top:', + console.warn('[ScrollController] unexpected scroll-to-top in interactive phase:', `was viewportY=${this._viewportY}, now viewportY=0, baseY=${baseY}`); - const restoreY = this._viewportY; - const wasBottom = this._wasAtBottom; - // Restore — use programmaticScroll to prevent recursion - if (wasBottom) { - this.programmaticScroll(() => this.term.scrollToBottom()); - this._viewportY = baseY; - this._wasAtBottom = true; - } else { - // Clamp to new baseY in case buffer shrank - const clampedY = Math.min(restoreY, baseY); - this.programmaticScroll(() => this.term.scrollToLine(clampedY)); - this._viewportY = clampedY; - this._wasAtBottom = clampedY >= baseY - 2; - } - this._baseY = baseY; - return; } this._baseY = baseY;