From df5e1c3ecd51b49146c5c447dcb28bb4a35bd7ea Mon Sep 17 00:00:00 2001 From: Brian Egan Date: Fri, 12 Jun 2026 14:20:09 +0200 Subject: [PATCH] fix(renderer): correct scrollback/screen row mapping during smooth scroll The renderer chose between scrollback and screen rows using the raw `y < viewportY` comparison, but computed the scrollback offset and screen row with `Math.floor(viewportY)`. When viewportY is fractional (during a smooth-scroll animation) the two disagree for the boundary row `y === Math.floor(viewportY)`: it is treated as scrollback and asked for offset `scrollbackLength`, which is one past the last valid index, so the WASM call returns null and that row is never repainted (keeping stale pixels). At the same time the top screen row (screen row 0) is dropped and every row below shifts up by one. Visually a line near the top of the viewport appears duplicated and overlaps its neighbours while scrolling. Floor viewportY once and use that single integer for both the boundary comparison and the offset/screen-row math, in the render loop and the hyperlink-hover scan. This matches every coordinate-mapping site in terminal.ts, which already floors before comparing. Adds renderer-viewport-boundary.test.ts covering the integer and fractional cases (no out-of-range scrollback offset, top screen row not dropped, fractional maps the same as floored). Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/renderer-viewport-boundary.test.ts | 133 +++++++++++++++++++++++++ lib/renderer.ts | 30 ++++-- 2 files changed, 153 insertions(+), 10 deletions(-) create mode 100644 lib/renderer-viewport-boundary.test.ts diff --git a/lib/renderer-viewport-boundary.test.ts b/lib/renderer-viewport-boundary.test.ts new file mode 100644 index 00000000..48debefe --- /dev/null +++ b/lib/renderer-viewport-boundary.test.ts @@ -0,0 +1,133 @@ +/** + * Renderer scrollback/screen boundary tests. + * + * BUG: When scrolled up during a smooth-scroll animation, viewportY is + * fractional (e.g. 2.5). The renderer decided whether a viewport row came + * from scrollback or the live screen using the raw `y < viewportY` + * comparison, but computed the scrollback offset / screen row using + * `Math.floor(viewportY)`. The two disagree for the boundary row + * y === Math.floor(viewportY): + * + * - it is treated as scrollback and asked for offset + * scrollbackLength - floor + floor === scrollbackLength, which is one + * past the last valid index (valid: 0 .. length-1). The WASM call + * returns null, so that row is never redrawn and keeps stale pixels + * from the previous frame. + * - screen row 0 (screenRow = floor - floor = 0) is never fetched, so the + * real top line of the screen is dropped and everything below shifts. + * + * Visually this looks like a line near the top of the viewport being + * duplicated and overlapping its neighbours while scrolling. + * + * Every coordinate-mapping site in terminal.ts already floors viewportY + * before comparing; the renderer must do the same. + */ + +import { describe, expect, test } from 'bun:test'; +import { CanvasRenderer } from './renderer'; +import type { IRenderable, IScrollbackProvider } from './renderer'; +import type { GhosttyCell } from './types'; + +function cell(codepoint: number): GhosttyCell { + return { + codepoint, + fg_r: 200, + fg_g: 200, + fg_b: 200, + bg_r: 0, + bg_g: 0, + bg_b: 0, + flags: 0, + width: 1, + hyperlink_id: 0, + grapheme_len: 0, + }; +} + +function makeLine(cols: number, codepoint: number): GhosttyCell[] { + return Array.from({ length: cols }, () => cell(codepoint)); +} + +interface Recording { + scrollbackOffsets: number[]; + screenRows: number[]; +} + +function renderAt(viewportY: number, opts: { cols: number; rows: number; scrollbackLength: number }) { + const { cols, rows, scrollbackLength } = opts; + const rec: Recording = { scrollbackOffsets: [], screenRows: [] }; + + const buffer: IRenderable = { + getLine(y: number) { + rec.screenRows.push(y); + // Only valid screen rows exist; out-of-range reads return null like WASM. + if (y < 0 || y >= rows) return null; + return makeLine(cols, 0x41 /* 'A' */); + }, + getCursor() { + return { x: 0, y: 0, visible: false }; + }, + getDimensions() { + return { cols, rows }; + }, + isRowDirty() { + return true; + }, + clearDirty() {}, + getGraphemeString() { + return 'A'; + }, + }; + + const scrollbackProvider: IScrollbackProvider = { + getScrollbackLine(offset: number) { + rec.scrollbackOffsets.push(offset); + if (offset < 0 || offset >= scrollbackLength) return null; // WASM returns null out of range + return makeLine(cols, 0x42 /* 'B' */); + }, + getScrollbackLength() { + return scrollbackLength; + }, + }; + + const canvas = document.createElement('canvas'); + const renderer = new CanvasRenderer(canvas, { devicePixelRatio: 1 }); + renderer.render(buffer, true, viewportY, scrollbackProvider); + + return rec; +} + +describe('Renderer scrollback/screen boundary', () => { + const cols = 20; + const rows = 10; + const scrollbackLength = 50; + + test('integer viewportY never requests an out-of-range scrollback offset', () => { + const rec = renderAt(3, { cols, rows, scrollbackLength }); + for (const offset of rec.scrollbackOffsets) { + expect(offset).toBeGreaterThanOrEqual(0); + expect(offset).toBeLessThan(scrollbackLength); + } + }); + + test('fractional viewportY never requests an out-of-range scrollback offset', () => { + const rec = renderAt(2.5, { cols, rows, scrollbackLength }); + for (const offset of rec.scrollbackOffsets) { + expect(offset).toBeGreaterThanOrEqual(0); + expect(offset).toBeLessThan(scrollbackLength); + } + }); + + test('fractional viewportY still renders the top screen row (no dropped line)', () => { + const rec = renderAt(2.5, { cols, rows, scrollbackLength }); + // floor(2.5) === 2 scrollback rows, so screen row 0 must be fetched. + expect(rec.screenRows).toContain(0); + }); + + test('fractional viewportY maps rows the same way as floor(viewportY)', () => { + const frac = renderAt(2.7, { cols, rows, scrollbackLength }); + const floored = renderAt(2, { cols, rows, scrollbackLength }); + expect(new Set(frac.scrollbackOffsets)).toEqual(new Set(floored.scrollbackOffsets)); + expect(new Set(frac.screenRows)).toEqual(new Set(floored.screenRows)); + }); +}); diff --git a/lib/renderer.ts b/lib/renderer.ts index 3b51bfdd..c4135c5f 100644 --- a/lib/renderer.ts +++ b/lib/renderer.ts @@ -361,19 +361,22 @@ export class CanvasRenderer { if (hyperlinkChanged) { // Find rows containing the old or new hovered hyperlink // Must check the correct buffer based on viewportY (scrollback vs screen) + // Floor viewportY once: the scrollback/screen boundary and the offset math + // must use the same integer, otherwise fractional values (during smooth + // scroll) read one row past the scrollback and drop the top screen row. + const flooredViewportY = Math.floor(viewportY); for (let y = 0; y < dims.rows; y++) { let line: GhosttyCell[] | null = null; // Same logic as rendering: fetch from scrollback or screen - if (viewportY > 0) { - if (y < viewportY && scrollbackProvider) { + if (flooredViewportY > 0) { + if (y < flooredViewportY && scrollbackProvider) { // This row is from scrollback - // Floor viewportY for array access (handles fractional values during smooth scroll) - const scrollbackOffset = scrollbackLength - Math.floor(viewportY) + y; + const scrollbackOffset = scrollbackLength - flooredViewportY + y; line = scrollbackProvider.getScrollbackLine(scrollbackOffset); } else { // This row is from visible screen - const screenRow = y - Math.floor(viewportY); + const screenRow = y - flooredViewportY; line = buffer.getLine(screenRow); } } else { @@ -441,6 +444,14 @@ export class CanvasRenderer { } } + // Floor viewportY once for row mapping. The scrollback/screen boundary + // comparison and the offset/screenRow math must use the SAME integer. + // During smooth scroll viewportY is fractional (e.g. 2.5); comparing rows + // against the raw value while indexing with the floored value read one row + // past the end of scrollback (returning null, leaving stale pixels) and + // dropped the top screen row, duplicating a line near the top of the view. + const flooredViewportY = Math.floor(viewportY); + // Render each line for (let y = 0; y < dims.rows; y++) { if (!rowsToRender.has(y)) { @@ -451,21 +462,20 @@ export class CanvasRenderer { // Fetch line from scrollback or visible screen let line: GhosttyCell[] | null = null; - if (viewportY > 0) { + if (flooredViewportY > 0) { // Scrolled up - need to fetch from scrollback + visible screen // When scrolled up N lines, we want to show: // - Scrollback lines (from the end) + visible screen lines // Check if this row should come from scrollback or visible screen - if (y < viewportY && scrollbackProvider) { + if (y < flooredViewportY && scrollbackProvider) { // This row is from scrollback (upper part of viewport) // Get from end of scrollback buffer - // Floor viewportY for array access (handles fractional values during smooth scroll) - const scrollbackOffset = scrollbackLength - Math.floor(viewportY) + y; + const scrollbackOffset = scrollbackLength - flooredViewportY + y; line = scrollbackProvider.getScrollbackLine(scrollbackOffset); } else { // This row is from visible screen (lower part of viewport) - const screenRow = viewportY > 0 ? y - Math.floor(viewportY) : y; + const screenRow = y - flooredViewportY; line = buffer.getLine(screenRow); } } else {