From 1a2c0d594834f9987dbcfc4e3fe6afd1c0f39330 Mon Sep 17 00:00:00 2001 From: isamu Date: Thu, 24 Sep 2026 19:47:39 +0900 Subject: [PATCH 1/2] refactor(view): take the highlight and arrow-key helpers from MulmoClaude Both were lifted out of this View in MulmoClaude and given tests; the logic here was still inline. The pure parts move out, with their tests. - src/vue/keyboardNav.ts: getArrowKeyOffset and isWithinSheetBounds. - src/vue/cellHighlights.ts: clearCellHighlights, applyCellHighlights, highlightCell, over a minimal DOM surface so a test needs no jsdom. - View.vue calls them; the watch body and the arrow-key switch go away. Behaviour-preserving, and shown to be: the old inline switch and bounds check were copied verbatim into a throwaway harness and run beside the helpers over generated keys, coordinates and sheet shapes (including negative indices, a sparse row and a zero-length row). Identical on every case. The harness is deleted; what survives it is the generated ground it covered, which the ported tests already assert directly. Co-Authored-By: Claude Opus 5 (1M context) --- src/vue/View.vue | 87 +++-------------- src/vue/cellHighlights.ts | 78 +++++++++++++++ src/vue/keyboardNav.ts | 38 +++++++ tests/vue/test_cellHighlights.ts | 163 +++++++++++++++++++++++++++++++ tests/vue/test_keyboardNav.ts | 92 +++++++++++++++++ 5 files changed, 384 insertions(+), 74 deletions(-) create mode 100644 src/vue/cellHighlights.ts create mode 100644 src/vue/keyboardNav.ts create mode 100644 tests/vue/test_cellHighlights.ts create mode 100644 tests/vue/test_keyboardNav.ts diff --git a/src/vue/View.vue b/src/vue/View.vue index 71bdc3a..04eb626 100644 --- a/src/vue/View.vue +++ b/src/vue/View.vue @@ -129,6 +129,8 @@ import { columnToIndex, indexToColumn, } from "../engine"; +import { applyCellHighlights, clearCellHighlights } from "./cellHighlights"; +import { getArrowKeyOffset, isWithinSheetBounds } from "./keyboardNav"; // Import all spreadsheet functions to populate the function registry import "../engine/functions"; @@ -752,47 +754,13 @@ watch( watch( [miniEditorOpen, miniEditorCell, referencedCells, renderedHtml], () => { - // Remove previous highlights - const prevEditingCell = - tableContainer.value?.querySelector(".cell-editing"); - if (prevEditingCell) { - prevEditingCell.classList.remove("cell-editing"); - } - - const prevReferencedCells = - tableContainer.value?.querySelectorAll(".cell-referenced"); - if (prevReferencedCells) { - prevReferencedCells.forEach((cell) => - cell.classList.remove("cell-referenced"), - ); - } - - if (miniEditorOpen.value && tableContainer.value) { - const table = tableContainer.value.querySelector("#spreadsheet-table"); - if (table) { - // Highlight the selected cell - if (miniEditorCell.value) { - const row = table.querySelectorAll("tr")[miniEditorCell.value.row]; - if (row) { - const cell = row.querySelectorAll("td")[miniEditorCell.value.col]; - if (cell) { - cell.classList.add("cell-editing"); - } - } - } - - // Highlight referenced cells - for (const ref of referencedCells.value) { - const row = table.querySelectorAll("tr")[ref.row]; - if (row) { - const cell = row.querySelectorAll("td")[ref.col]; - if (cell) { - cell.classList.add("cell-referenced"); - } - } - } - } - } + clearCellHighlights(tableContainer.value); + if (!miniEditorOpen.value) return; + applyCellHighlights( + tableContainer.value, + miniEditorCell.value, + referencedCells.value, + ); }, { flush: "post" }, ); @@ -813,42 +781,13 @@ function handleKeyboardNavigation(event: KeyboardEvent) { } const { row, col } = miniEditorCell.value; - let newRow = row; - let newCol = col; - - // Determine new position based on arrow key - switch (event.key) { - case "ArrowUp": - newRow = Math.max(0, row - 1); - break; - case "ArrowDown": - newRow = row + 1; - break; - case "ArrowLeft": - newCol = Math.max(0, col - 1); - break; - case "ArrowRight": - newCol = col + 1; - break; - default: - return; // Not an arrow key, ignore - } + const nextCell = getArrowKeyOffset(event.key, row, col); + if (!nextCell) return; // Not an arrow key, ignore // Get current sheet data to validate bounds try { const sheets = JSON.parse(editableData.value); - const currentSheet = sheets[activeSheetIndex.value]; - - if (!currentSheet || !currentSheet.data) return; - - // Validate new position is within bounds - if ( - newRow < 0 || - newRow >= currentSheet.data.length || - newCol < 0 || - !currentSheet.data[newRow] || - newCol >= currentSheet.data[newRow].length - ) { + if (!isWithinSheetBounds(sheets[activeSheetIndex.value], nextCell.row, nextCell.col)) { return; // Out of bounds, ignore } @@ -856,7 +795,7 @@ function handleKeyboardNavigation(event: KeyboardEvent) { event.preventDefault(); // Move to new cell - openMiniEditor(newRow, newCol); + openMiniEditor(nextCell.row, nextCell.col); } catch (error) { console.error("Failed to navigate cells:", error); } diff --git a/src/vue/cellHighlights.ts b/src/vue/cellHighlights.ts new file mode 100644 index 0000000..ddce5c0 --- /dev/null +++ b/src/vue/cellHighlights.ts @@ -0,0 +1,78 @@ +/** + * DOM helpers for the mini-editor cell-highlight pass. Extracted from + * the post-flush watch in `src/vue/View.vue`, which + * had a cognitive complexity of 30 driven by four levels of nested + * optional chaining + loops. + * + * These helpers are side-effectful by nature (they add/remove CSS + * classes on DOM nodes) but each one is small enough that its + * behaviour is obvious. Unit-testable with a minimal mock DOM; see + * `tests/vue/test_cellHighlights.ts`. + */ + +/** Minimal DOM surface the helpers need. Defined here so tests can + * pass plain objects without pulling in jsdom. */ +export interface HighlightableElement { + classList: { add: (cls: string) => void; remove: (cls: string) => void }; +} + +export interface HighlightableRow { + querySelectorAll: (selector: string) => ArrayLike; +} + +export interface HighlightableTable { + querySelectorAll: (selector: string) => ArrayLike; +} + +export interface HighlightableContainer { + // Overload: the spreadsheet root container is known to return a + // table when asked for the table id, so callers can keep the + // result strongly typed without casting. + querySelector: ((selector: "#spreadsheet-table") => HighlightableTable | null) & ((selector: string) => HighlightableElement | null); + querySelectorAll: (selector: string) => ArrayLike & Iterable; +} + +export interface CellCoord { + row: number; + col: number; +} + +const CELL_EDITING = "cell-editing"; +const CELL_REFERENCED = "cell-referenced"; + +/** Remove both kinds of highlight classes from the container. */ +export function clearCellHighlights(container: HighlightableContainer | null | undefined): void { + if (!container) return; + container.querySelector(`.${CELL_EDITING}`)?.classList.remove(CELL_EDITING); + for (const cell of container.querySelectorAll(`.${CELL_REFERENCED}`)) { + cell.classList.remove(CELL_REFERENCED); + } +} + +/** Add `className` to the at (row, col) of the given table. + * No-op if the row or cell doesn't exist. */ +export function highlightCell(table: HighlightableTable | null | undefined, coord: CellCoord, className: string): void { + if (!table) return; + const rows = table.querySelectorAll("tr"); + const row = rows[coord.row]; + if (!row) return; + const cells = row.querySelectorAll("td"); + const cell = cells[coord.col]; + if (!cell) return; + cell.classList.add(className); +} + +/** Apply the editing cell + referenced cells highlights. Looks up + * the #spreadsheet-table inside the container and no-ops if the + * table hasn't rendered yet. */ +export function applyCellHighlights( + container: HighlightableContainer | null | undefined, + editingCell: CellCoord | null, + references: readonly CellCoord[], +): void { + if (!container) return; + const table = container.querySelector("#spreadsheet-table"); + if (!table) return; + if (editingCell) highlightCell(table, editingCell, CELL_EDITING); + for (const ref of references) highlightCell(table, ref, CELL_REFERENCED); +} diff --git a/src/vue/keyboardNav.ts b/src/vue/keyboardNav.ts new file mode 100644 index 0000000..cce8ce4 --- /dev/null +++ b/src/vue/keyboardNav.ts @@ -0,0 +1,38 @@ +// Pure helpers behind the spreadsheet mini-editor's arrow-key +// navigation. Lifted out of View.vue so each rule can be unit-tested +// without spinning up Vue or a DOM. + +export interface CellPosition { + row: number; + col: number; +} + +// Sheet shape we actually rely on — the mini-editor only reads +// `data` as a 2D array, so the type is intentionally loose to match +// what arrives from `JSON.parse(editableData.value)`. +export interface SheetLike { + data?: unknown[][]; +} + +export function getArrowKeyOffset(key: string, row: number, col: number): CellPosition | null { + switch (key) { + case "ArrowUp": + return { row: Math.max(0, row - 1), col }; + case "ArrowDown": + return { row: row + 1, col }; + case "ArrowLeft": + return { row, col: Math.max(0, col - 1) }; + case "ArrowRight": + return { row, col: col + 1 }; + default: + return null; + } +} + +export function isWithinSheetBounds(sheet: SheetLike | null | undefined, row: number, col: number): boolean { + if (!sheet?.data) return false; + if (row < 0 || row >= sheet.data.length) return false; + const rowData = sheet.data[row]; + if (!rowData) return false; + return col >= 0 && col < rowData.length; +} diff --git a/tests/vue/test_cellHighlights.ts b/tests/vue/test_cellHighlights.ts new file mode 100644 index 0000000..6650791 --- /dev/null +++ b/tests/vue/test_cellHighlights.ts @@ -0,0 +1,163 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { + applyCellHighlights, + clearCellHighlights, + highlightCell, + type HighlightableContainer, + type HighlightableElement, + type HighlightableTable, +} from "../../src/vue/cellHighlights.js"; + +// Minimal mock DOM: each "element" tracks the classes added/removed +// on it, so the test can assert the helper manipulated the right +// node without pulling in jsdom. +function makeCell(): HighlightableElement & { classes: Set } { + const classes = new Set(); + return { + classes, + classList: { + add: (cls: string) => { + classes.add(cls); + }, + remove: (cls: string) => { + classes.delete(cls); + }, + }, + }; +} + +function makeRow(cellCount: number): { + cells: ReturnType[]; + row: { querySelectorAll: (_: string) => ArrayLike }; +} { + const cells = Array.from({ length: cellCount }, () => makeCell()); + return { + cells, + row: { querySelectorAll: () => cells }, + }; +} + +function makeTable(rowsAndCols: number[][]): { + rows: ReturnType[]; + table: HighlightableTable; +} { + const rows = rowsAndCols.map((cols) => makeRow(cols.length)); + return { + rows, + table: { querySelectorAll: () => rows.map((rowItem) => rowItem.row) }, + }; +} + +function cellOf(rows: ReturnType[], rowIndex: number, colIndex: number): ReturnType { + const row = rows[rowIndex]; + assert.ok(row, `mock table has no row ${rowIndex}`); + const cell = row.cells[colIndex]; + assert.ok(cell, `mock row ${rowIndex} has no cell ${colIndex}`); + return cell; +} + +// Build a HighlightableContainer with the given querySelector / +// querySelectorAll responses. Arrays are already iterable so we can +// return them directly. +function makeContainer(opts: { + onQuerySelector?: (sel: string) => HighlightableElement | HighlightableTable | null; + onQueryAll?: (sel: string) => HighlightableElement[]; +}): HighlightableContainer { + // The `querySelector` overloaded signature can't be satisfied by + // a single closure, so we cast the callable to the interface + // slot — the test itself validates runtime behaviour. + const queryFn = opts.onQuerySelector ?? (() => null); + return { + querySelector: queryFn as HighlightableContainer["querySelector"], + querySelectorAll: (sel) => opts.onQueryAll?.(sel) ?? [], + }; +} + +describe("highlightCell", () => { + it("adds className to the correct cell", () => { + const { table, rows } = makeTable([[1, 2, 3]]); + highlightCell(table, { row: 0, col: 1 }, "cell-editing"); + assert.ok(cellOf(rows, 0, 1).classes.has("cell-editing")); + assert.ok(!cellOf(rows, 0, 0).classes.has("cell-editing")); + }); + + it("is a no-op for a null table", () => { + assert.doesNotThrow(() => highlightCell(null, { row: 0, col: 0 }, "x")); + }); + + it("is a no-op when row is out of range", () => { + const { table, rows } = makeTable([[1]]); + highlightCell(table, { row: 5, col: 0 }, "x"); + assert.equal(cellOf(rows, 0, 0).classes.size, 0); + }); + + it("is a no-op when col is out of range", () => { + const { table, rows } = makeTable([[1, 2]]); + highlightCell(table, { row: 0, col: 99 }, "x"); + assert.equal(cellOf(rows, 0, 0).classes.size, 0); + assert.equal(cellOf(rows, 0, 1).classes.size, 0); + }); +}); + +describe("clearCellHighlights", () => { + it("removes both editing and referenced classes", () => { + const editing = makeCell(); + editing.classes.add("cell-editing"); + const ref1 = makeCell(); + ref1.classes.add("cell-referenced"); + const ref2 = makeCell(); + ref2.classes.add("cell-referenced"); + const container = makeContainer({ + onQuerySelector: (sel) => (sel === ".cell-editing" ? editing : null), + onQueryAll: (sel) => (sel === ".cell-referenced" ? [ref1, ref2] : []), + }); + clearCellHighlights(container); + assert.ok(!editing.classes.has("cell-editing")); + assert.ok(!ref1.classes.has("cell-referenced")); + assert.ok(!ref2.classes.has("cell-referenced")); + }); + + it("is a no-op when container is null", () => { + assert.doesNotThrow(() => clearCellHighlights(null)); + }); + + it("is a no-op when there are no previous highlights", () => { + const container = makeContainer({}); + assert.doesNotThrow(() => clearCellHighlights(container)); + }); +}); + +describe("applyCellHighlights", () => { + it("adds cell-editing + cell-referenced in one pass", () => { + const { table, rows } = makeTable([ + [1, 2, 3], + [1, 2, 3], + ]); + const container = makeContainer({ + onQuerySelector: (sel) => (sel === "#spreadsheet-table" ? table : null), + }); + applyCellHighlights(container, { row: 0, col: 1 }, [{ row: 1, col: 2 }]); + assert.ok(cellOf(rows, 0, 1).classes.has("cell-editing")); + assert.ok(cellOf(rows, 1, 2).classes.has("cell-referenced")); + }); + + it("no-op when container is null", () => { + assert.doesNotThrow(() => applyCellHighlights(null, { row: 0, col: 0 }, [])); + }); + + it("no-op when #spreadsheet-table is missing", () => { + const container = makeContainer({}); + assert.doesNotThrow(() => applyCellHighlights(container, { row: 0, col: 0 }, [])); + }); + + it("skips editing cell when null, still applies references", () => { + const { table, rows } = makeTable([[1, 2]]); + const container = makeContainer({ + onQuerySelector: () => table, + }); + applyCellHighlights(container, null, [{ row: 0, col: 1 }]); + assert.ok(!cellOf(rows, 0, 0).classes.has("cell-editing")); + assert.ok(cellOf(rows, 0, 1).classes.has("cell-referenced")); + }); +}); diff --git a/tests/vue/test_keyboardNav.ts b/tests/vue/test_keyboardNav.ts new file mode 100644 index 0000000..8226b28 --- /dev/null +++ b/tests/vue/test_keyboardNav.ts @@ -0,0 +1,92 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { getArrowKeyOffset, isWithinSheetBounds } from "../../src/vue/keyboardNav.js"; + +describe("getArrowKeyOffset", () => { + it("ArrowUp decrements row", () => { + assert.deepEqual(getArrowKeyOffset("ArrowUp", 3, 5), { row: 2, col: 5 }); + }); + + it("ArrowDown increments row (no upper clamp — bounds check happens later)", () => { + assert.deepEqual(getArrowKeyOffset("ArrowDown", 3, 5), { row: 4, col: 5 }); + }); + + it("ArrowLeft decrements col", () => { + assert.deepEqual(getArrowKeyOffset("ArrowLeft", 3, 5), { row: 3, col: 4 }); + }); + + it("ArrowRight increments col", () => { + assert.deepEqual(getArrowKeyOffset("ArrowRight", 3, 5), { row: 3, col: 6 }); + }); + + it("ArrowUp clamps row at 0", () => { + assert.deepEqual(getArrowKeyOffset("ArrowUp", 0, 5), { row: 0, col: 5 }); + }); + + it("ArrowLeft clamps col at 0", () => { + assert.deepEqual(getArrowKeyOffset("ArrowLeft", 3, 0), { row: 3, col: 0 }); + }); + + it("returns null for non-arrow keys", () => { + for (const key of ["Enter", "Tab", "Escape", "a", " ", "Shift"]) { + assert.equal(getArrowKeyOffset(key, 3, 5), null, `expected null for ${JSON.stringify(key)}`); + } + }); + + it("returns null for empty string", () => { + assert.equal(getArrowKeyOffset("", 3, 5), null); + }); +}); + +describe("isWithinSheetBounds", () => { + const sheet = { + data: [ + [1, 2, 3], + [4, 5, 6], + ], + }; + + it("accepts an in-range cell", () => { + assert.equal(isWithinSheetBounds(sheet, 0, 0), true); + assert.equal(isWithinSheetBounds(sheet, 1, 2), true); + }); + + it("rejects negative row", () => { + assert.equal(isWithinSheetBounds(sheet, -1, 0), false); + }); + + it("rejects negative col", () => { + assert.equal(isWithinSheetBounds(sheet, 0, -1), false); + }); + + it("rejects row past data length", () => { + assert.equal(isWithinSheetBounds(sheet, 2, 0), false); + }); + + it("rejects col past row length", () => { + assert.equal(isWithinSheetBounds(sheet, 0, 3), false); + }); + + it("rejects when sheet is undefined", () => { + assert.equal(isWithinSheetBounds(undefined, 0, 0), false); + }); + + it("rejects when sheet is null", () => { + assert.equal(isWithinSheetBounds(null, 0, 0), false); + }); + + it("rejects when sheet.data is missing", () => { + assert.equal(isWithinSheetBounds({}, 0, 0), false); + }); + + it("rejects when the target row entry is missing (sparse array)", () => { + // eslint-disable-next-line no-sparse-arrays + const sparse = { data: [[1, 2], , [3, 4]] as unknown[][] }; + assert.equal(isWithinSheetBounds(sparse, 1, 0), false); + }); + + it("handles a row of zero length (col always rejected)", () => { + const empty = { data: [[]] }; + assert.equal(isWithinSheetBounds(empty, 0, 0), false); + }); +}); From d136440e29df6b034ebaf5decbc304bb49a6df04 Mon Sep 17 00:00:00 2001 From: isamu Date: Thu, 24 Sep 2026 19:58:23 +0900 Subject: [PATCH 2/2] test: run the node:test files under every tests/ directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The view helpers arrived in tests/vue, which the engine-only glob did not reach — a test that is not run is not a test. Co-Authored-By: Claude Opus 5 (1M context) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 9094c84..358b8e7 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "typecheck": "vue-tsc --noEmit", "lint": "eslint src demo", "test": "vitest run", - "test:engine": "tsx --test \"tests/engine/test_*.ts\"", + "test:engine": "tsx --test \"tests/**/test_*.ts\"", "test:watch": "vitest", "test:fixtures": "tsx tests/engine/run-all-fixtures.ts", "test:calculator": "tsx tests/engine/run-calculator-tests.ts",