From cd3aebc6afd56e425c3c7c4ab91708a2c3591620 Mon Sep 17 00:00:00 2001 From: provi Date: Tue, 9 Jun 2026 22:04:08 -0700 Subject: [PATCH 1/3] refactor(terminal): consolidate scroll math into one helper module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four views each rolled their own scroll logic (only journal used the ScrollableRegion widget). Extract widgets/scroll.ts with the shared primitives and route every view through it — behavior-preserving: - scroll.ts: clampOffset (free-scroll range), offsetToShow (cursor-into-view, stateful), windowFor (stateless visible window), pageIndicator. - browse-model: ensureCursorVisible → offsetToShow (was the same math inline). - settings-scene: visible-window calc → windowFor. - ScrollableRegion (journal): scroll/indicator now call clampOffset/pageIndicator; gains ensureVisible() for cursor lists. - detail-model / detail-renderer: clamp + page indicator → clampOffset/pageIndicator. detail keeps its own scroll fields rather than folding fully onto ScrollableRegion: its scroll state is intertwined with a rich model AND a pre-existing viewportHeight(20)-vs-renderer-visibleRows split, which a full fold would have to reconcile — a behavior change, not a pure refactor. Sharing the math is safe and captures the duplication; the full state-fold is a separate follow-up. Adds scroll.test.ts (the helper math). Suite + verifier green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../terminal/src/__tests__/scroll.test.ts | 53 +++++++++++++++++++ .../terminal/src/scenes/dict/browse-model.ts | 7 +-- .../terminal/src/scenes/dict/detail-model.ts | 6 +-- .../src/scenes/dict/detail-renderer.ts | 4 +- .../src/scenes/settings/settings-scene.ts | 11 ++-- packages/terminal/src/widgets/scroll.ts | 43 +++++++++++++++ packages/terminal/src/widgets/scrollable.ts | 27 +++++----- 7 files changed, 123 insertions(+), 28 deletions(-) create mode 100644 packages/terminal/src/__tests__/scroll.test.ts create mode 100644 packages/terminal/src/widgets/scroll.ts diff --git a/packages/terminal/src/__tests__/scroll.test.ts b/packages/terminal/src/__tests__/scroll.test.ts new file mode 100644 index 0000000..8dde298 --- /dev/null +++ b/packages/terminal/src/__tests__/scroll.test.ts @@ -0,0 +1,53 @@ +import { describe, test, expect } from "bun:test"; +import { clampOffset, offsetToShow, windowFor, pageIndicator } from "../widgets/scroll.ts"; + +describe("clampOffset", () => { + test("clamps into [0, contentLength - viewport]", () => { + expect(clampOffset(-3, 10, 4)).toBe(0); + expect(clampOffset(99, 10, 4)).toBe(6); // max = 10 - 4 + expect(clampOffset(2, 10, 4)).toBe(2); + }); + test("content shorter than viewport → 0", () => { + expect(clampOffset(5, 3, 10)).toBe(0); + }); +}); + +describe("offsetToShow (cursor-into-view, stateful)", () => { + test("keeps a visible cursor's offset unchanged", () => { + expect(offsetToShow(3, 2, 5)).toBe(2); // 3 in [2,7) + }); + test("scrolls up when cursor is above the window", () => { + expect(offsetToShow(1, 4, 5)).toBe(1); + }); + test("scrolls down so the cursor sits at the bottom of the window", () => { + expect(offsetToShow(9, 2, 5)).toBe(5); // 9 - 5 + 1 + }); +}); + +describe("windowFor (stateless window)", () => { + test("everything fits → full range", () => { + expect(windowFor(0, 10, 7)).toEqual({ start: 0, end: 7 }); + }); + test("focused near the top stays at 0", () => { + expect(windowFor(1, 5, 7)).toEqual({ start: 0, end: 5 }); + }); + test("focused at the end scrolls to keep it visible", () => { + expect(windowFor(6, 5, 7)).toEqual({ start: 2, end: 7 }); + }); + test("clamps so the window never exceeds total", () => { + const w = windowFor(6, 5, 7); + expect(w.end - w.start).toBe(5); + expect(w.end).toBeLessThanOrEqual(7); + }); +}); + +describe("pageIndicator", () => { + test("'1/1' when content fits", () => { + expect(pageIndicator(0, 5, 20)).toBe("1/1"); + }); + test("page math over multiple viewports", () => { + expect(pageIndicator(0, 42, 20)).toBe("1/3"); + expect(pageIndicator(20, 42, 20)).toBe("2/3"); + expect(pageIndicator(40, 42, 20)).toBe("3/3"); + }); +}); diff --git a/packages/terminal/src/scenes/dict/browse-model.ts b/packages/terminal/src/scenes/dict/browse-model.ts index aaf3985..0b5046e 100644 --- a/packages/terminal/src/scenes/dict/browse-model.ts +++ b/packages/terminal/src/scenes/dict/browse-model.ts @@ -2,6 +2,7 @@ import type { Hexagram } from "@iching/core"; import { GUA, searchHexagrams } from "@iching/core"; +import { offsetToShow } from "../../widgets/scroll.ts"; export class BrowseModel { /** All 64 hexagrams */ @@ -92,10 +93,6 @@ export class BrowseModel { /** Ensure cursor is within visible scroll window */ private ensureCursorVisible(): void { - if (this.cursor < this.scrollOffset) { - this.scrollOffset = this.cursor; - } else if (this.cursor >= this.scrollOffset + this.viewportHeight) { - this.scrollOffset = this.cursor - this.viewportHeight + 1; - } + this.scrollOffset = offsetToShow(this.cursor, this.scrollOffset, this.viewportHeight); } } diff --git a/packages/terminal/src/scenes/dict/detail-model.ts b/packages/terminal/src/scenes/dict/detail-model.ts index f15099f..f39cf38 100644 --- a/packages/terminal/src/scenes/dict/detail-model.ts +++ b/packages/terminal/src/scenes/dict/detail-model.ts @@ -2,6 +2,7 @@ import { type HexagramDetail, type GlyphEntry, buildHexagramDetail } from "@iching/core"; import type { GlyphAnimator } from "../../glyph-anim/types.ts"; +import { clampOffset } from "../../widgets/scroll.ts"; export type DetailFocus = "content" | "derived"; @@ -94,12 +95,11 @@ export class DetailModel { } scrollUp(n = 1): void { - this.scrollOffset = Math.max(0, this.scrollOffset - n); + this.scrollOffset = clampOffset(this.scrollOffset - n, this.contentHeight, this.viewportHeight); } scrollDown(n = 1): void { - const max = Math.max(0, this.contentHeight - this.viewportHeight); - this.scrollOffset = Math.min(max, this.scrollOffset + n); + this.scrollOffset = clampOffset(this.scrollOffset + n, this.contentHeight, this.viewportHeight); } pageUp(): void { diff --git a/packages/terminal/src/scenes/dict/detail-renderer.ts b/packages/terminal/src/scenes/dict/detail-renderer.ts index 8be6301..fa096dd 100644 --- a/packages/terminal/src/scenes/dict/detail-renderer.ts +++ b/packages/terminal/src/scenes/dict/detail-renderer.ts @@ -10,6 +10,7 @@ import { stringWidth, centerPad } from "../../layout/measure.ts"; import { wordWrap } from "./word-wrap.ts"; import { GLYPHS } from "../../glyphs.ts"; import { tr } from "../../i18n/messages.ts"; +import { pageIndicator } from "../../widgets/scroll.ts"; const FOOTER_ROWS = 2; const PADDING = 2; @@ -301,9 +302,10 @@ function renderFooter( ? `[↑↓] ${tr(language, "verb.select")} · [enter] ${tr(language, "verb.open")} · [tab] ${tr(language, "verb.scroll")} · [esc] ${tr(language, "verb.back")}` : `[↑↓] ${tr(language, "verb.scroll")} · [tab] ${tr(language, "verb.derived")} · [enter] ${tr(language, "verb.open")} · [esc] ${tr(language, "verb.back")}`; + // Hidden when content fits (vs the region's "1/1"); shows the page otherwise. const indicator = model.contentHeight > model.viewportHeight - ? `${Math.floor(model.scrollOffset / model.viewportHeight) + 1}/${Math.ceil(model.contentHeight / model.viewportHeight)}` + ? pageIndicator(model.scrollOffset, model.contentHeight, model.viewportHeight) : ""; frame.writeText(footerRow, 1, keys, { fg: t.secondary }); diff --git a/packages/terminal/src/scenes/settings/settings-scene.ts b/packages/terminal/src/scenes/settings/settings-scene.ts index 486e6f4..7ad13f6 100644 --- a/packages/terminal/src/scenes/settings/settings-scene.ts +++ b/packages/terminal/src/scenes/settings/settings-scene.ts @@ -18,6 +18,7 @@ import { renderYarrowFieldStrip, drawApertureCursor } from "../yarrow/field-rend import { YarrowAutoPreview, YarrowManualPreview } from "../yarrow/yarrow-previews.ts"; import { LINE_WIDTH } from "../../glyphs.ts"; import { tr, type MessageKey } from "../../i18n/messages.ts"; +import { windowFor } from "../../widgets/scroll.ts"; // ── Setting definitions ────────────────────────────────────────────── @@ -219,12 +220,12 @@ export class SettingsScene implements Scene { const settingsStartRow = row; const availSettingRows = footerSepRow - settingsStartRow; const maxVisible = Math.max(1, Math.floor(availSettingRows / rowsPerSetting)); - const needsScroll = maxVisible < this.rows.length; // Window that keeps the focused row in view, scrolling no more than needed. - const scrollStart = needsScroll - ? Math.max(0, Math.min(this.focusedRow - maxVisible + 1, this.rows.length - maxVisible)) - : 0; - const scrollEnd = needsScroll ? scrollStart + maxVisible : this.rows.length; + const { start: scrollStart, end: scrollEnd } = windowFor( + this.focusedRow, + maxVisible, + this.rows.length, + ); for (let i = scrollStart; i < scrollEnd; i++) { const setting = this.rows[i]; diff --git a/packages/terminal/src/widgets/scroll.ts b/packages/terminal/src/widgets/scroll.ts new file mode 100644 index 0000000..3160b34 --- /dev/null +++ b/packages/terminal/src/widgets/scroll.ts @@ -0,0 +1,43 @@ +// Scroll / viewport math shared by list and content views. +// +// Two patterns live here: +// - free-scroll: a scroll offset over `contentLength` lines (ScrollableRegion, +// detail content) — clamp the offset, render a page indicator. +// - cursor-into-view: a focused index that must stay within a viewport window +// (browse list, settings rows) — derive the offset/window from the cursor. + +/** Clamp a scroll offset into the valid range `[0, max(0, contentLength - viewport)]`. */ +export function clampOffset(offset: number, contentLength: number, viewport: number): number { + return Math.max(0, Math.min(offset, Math.max(0, contentLength - viewport))); +} + +/** + * New scroll offset that keeps `cursor` within a `viewport`-sized window, + * scrolling only when the cursor leaves the window (stateful list navigation). + */ +export function offsetToShow(cursor: number, offset: number, viewport: number): number { + if (cursor < offset) return cursor; + if (cursor >= offset + viewport) return cursor - viewport + 1; + return offset; +} + +/** + * Stateless visible window `[start, end)` of at most `viewport` items over + * `total`, positioned to include `cursor` with minimal scrolling. Used where the + * offset isn't retained between renders (e.g. the settings rows). + */ +export function windowFor( + cursor: number, + viewport: number, + total: number, +): { start: number; end: number } { + if (viewport >= total) return { start: 0, end: total }; + const start = Math.max(0, Math.min(cursor - viewport + 1, total - viewport)); + return { start, end: start + viewport }; +} + +/** Page indicator like `"2/5"` for a free-scroll region; `"1/1"` when it all fits. */ +export function pageIndicator(offset: number, contentLength: number, viewport: number): string { + if (contentLength <= viewport) return "1/1"; + return `${Math.floor(offset / viewport) + 1}/${Math.ceil(contentLength / viewport)}`; +} diff --git a/packages/terminal/src/widgets/scrollable.ts b/packages/terminal/src/widgets/scrollable.ts index 115260c..fe1dafc 100644 --- a/packages/terminal/src/widgets/scrollable.ts +++ b/packages/terminal/src/widgets/scrollable.ts @@ -1,4 +1,7 @@ -// ScrollableRegion — virtual content with viewport windowing +// ScrollableRegion — virtual content with viewport windowing. +// Scroll math lives in ./scroll.ts so list views (browse/settings) share it. + +import { clampOffset, offsetToShow, pageIndicator } from "./scroll.ts"; export class ScrollableRegion { contentLines: string[]; @@ -13,12 +16,12 @@ export class ScrollableRegion { /** Scroll up by n lines (default 1) */ scrollUp(n = 1): void { - this.scrollOffset = Math.max(0, this.scrollOffset - n); + this.scrollOffset = clampOffset(this.scrollOffset - n, this.contentLines.length, this.viewportHeight); } /** Scroll down by n lines (default 1) */ scrollDown(n = 1): void { - this.scrollOffset = Math.min(this.maxOffset(), this.scrollOffset + n); + this.scrollOffset = clampOffset(this.scrollOffset + n, this.contentLines.length, this.viewportHeight); } /** Scroll up by one page */ @@ -38,7 +41,12 @@ export class ScrollableRegion { /** Scroll to the last page */ scrollToBottom(): void { - this.scrollOffset = this.maxOffset(); + this.scrollOffset = clampOffset(Infinity, this.contentLines.length, this.viewportHeight); + } + + /** Adjust the offset to keep a cursor index within the viewport (list navigation). */ + ensureVisible(cursor: number): void { + this.scrollOffset = offsetToShow(cursor, this.scrollOffset, this.viewportHeight); } /** Return the visible slice of content */ @@ -51,15 +59,6 @@ export class ScrollableRegion { /** Scroll position indicator, e.g. "3/42" */ scrollIndicator(): string { - const total = this.contentLines.length; - if (total <= this.viewportHeight) return "1/1"; - const page = Math.floor(this.scrollOffset / this.viewportHeight) + 1; - const totalPages = Math.ceil(total / this.viewportHeight); - return `${page}/${totalPages}`; - } - - /** Maximum valid scroll offset */ - private maxOffset(): number { - return Math.max(0, this.contentLines.length - this.viewportHeight); + return pageIndicator(this.scrollOffset, this.contentLines.length, this.viewportHeight); } } From 45af60977f2f190be06ea896b645d50ae18cc2b5 Mon Sep 17 00:00:00 2001 From: provi Date: Tue, 9 Jun 2026 22:22:17 -0700 Subject: [PATCH 2/3] refactor(terminal): journal reuses ScrollableRegion.ensureVisible (refactor lens) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /refactor sibling sweep on the scroll consolidation: journal-scene owned a ScrollableRegion but hand-rolled ensureCursorVisible() as a verbatim copy of offsetToShow — the exact math the region now exposes as ensureVisible(). Delegate to it. Behavior-identical (character-for-character same offset math); full suite green (572 tests). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/terminal/src/scenes/journal/journal-scene.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/terminal/src/scenes/journal/journal-scene.ts b/packages/terminal/src/scenes/journal/journal-scene.ts index 5ccbfe3..323c23f 100644 --- a/packages/terminal/src/scenes/journal/journal-scene.ts +++ b/packages/terminal/src/scenes/journal/journal-scene.ts @@ -185,12 +185,9 @@ export class JournalScene implements Scene { } private ensureCursorVisible(): void { - const viewportH = this.scroll.viewportHeight; - if (this.cursor < this.scroll.scrollOffset) { - this.scroll.scrollOffset = this.cursor; - } else if (this.cursor >= this.scroll.scrollOffset + viewportH) { - this.scroll.scrollOffset = this.cursor - viewportH + 1; - } + // Same cursor-into-view math the dict browse list uses; the ScrollableRegion + // this scene already owns exposes it (offsetToShow), so don't re-derive it. + this.scroll.ensureVisible(this.cursor); } } From e98bf59f2b45a405e409eb5f04b33b765c212f1a Mon Sep 17 00:00:00 2001 From: provi Date: Tue, 9 Jun 2026 22:47:36 -0700 Subject: [PATCH 3/3] refactor(terminal): delete dead commentary-scroll state in the cast scene MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commentaryScrollOffset was written at three sites (exploration-mode ↑/↓ handlers, setFocusedHex reset, constructor init) and read by nothing — no renderer ever consumed it, so the arrows mutated invisible state. Remove the field and all writers. Key routing is unchanged: in exploration mode ↑/↓ now fall through every remaining handler and do nothing, same as the user ever saw. Surfaced by the /refactor sibling sweep on the scroll consolidation; if commentary scrolling is wanted later, it should be built on widgets/scroll.ts. Co-Authored-By: Claude Fable 5 --- packages/terminal/src/scenes/cast/cast-scene.ts | 13 ++----------- packages/terminal/src/scenes/cast/model.ts | 2 -- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/packages/terminal/src/scenes/cast/cast-scene.ts b/packages/terminal/src/scenes/cast/cast-scene.ts index c3c0172..2526b6f 100644 --- a/packages/terminal/src/scenes/cast/cast-scene.ts +++ b/packages/terminal/src/scenes/cast/cast-scene.ts @@ -147,7 +147,7 @@ export class CastScene implements Scene { return { type: "home" }; } - // Exploration mode: arrow keys switch focus, scroll commentary + // Exploration mode: left/right arrows switch focus if (this.model.explorationMode && key.type === "arrow") { if (key.direction === "left" && this.model.focusedHex === "becoming") { this.setFocusedHex("primary"); @@ -157,14 +157,6 @@ export class CastScene implements Scene { this.setFocusedHex("becoming"); return; } - if (key.direction === "up") { - this.model.commentaryScrollOffset = Math.max(0, this.model.commentaryScrollOffset - 1); - return; - } - if (key.direction === "down") { - this.model.commentaryScrollOffset++; - return; - } } // Enter in exploration mode opens dictionary for focused hex @@ -199,11 +191,10 @@ export class CastScene implements Scene { /** * Switch focus to a hexagram — updates focusedHex, creates matching - * glyph animator, resets scroll. Single method for all focus changes. + * glyph animator. Single method for all focus changes. */ private setFocusedHex(hex: "primary" | "becoming"): void { this.model.focusedHex = hex; - this.model.commentaryScrollOffset = 0; const entry = hex === "primary" ? this.model.primaryGlyphEntry : this.model.becomingGlyphEntry; diff --git a/packages/terminal/src/scenes/cast/model.ts b/packages/terminal/src/scenes/cast/model.ts index e1951ca..ba3f932 100644 --- a/packages/terminal/src/scenes/cast/model.ts +++ b/packages/terminal/src/scenes/cast/model.ts @@ -57,7 +57,6 @@ export class CastModel { // Interactive exploration (after becoming reveal) explorationMode: boolean; focusedHex: "primary" | "becoming"; - commentaryScrollOffset: number; // Intention text for this cast intention?: string; @@ -107,6 +106,5 @@ export class CastModel { this.explorationMode = false; this.focusedHex = "primary"; - this.commentaryScrollOffset = 0; } }