diff --git a/src/tui/controller.ts b/src/tui/controller.ts index 9d5065d4..cca41feb 100644 --- a/src/tui/controller.ts +++ b/src/tui/controller.ts @@ -13,7 +13,7 @@ import { copyToClipboard } from '../clipboard.js'; import * as fs from 'fs'; import * as path from 'path'; import { humanFormatWorkItem, formatTitleOnlyTUI } from '../commands/helpers.js'; -import { createTuiState, rebuildTreeState, buildVisibleNodes, expandAncestorsForInProgress, isClosedStatus, enterMoveMode, exitMoveMode, sortBySortIndexDateAndId } from './state.js'; +import { createTuiState, rebuildTreeState, buildVisibleNodes, expandAncestorsForInProgress, isClosedStatus, enterMoveMode, exitMoveMode, sortBySortIndexDateAndId, incrementalExpand, incrementalCollapse } from './state.js'; import { createPersistence } from './persistence.js'; import { resolveWorklogDir } from '../worklog-paths.js'; import { createLayout } from './layout.js'; @@ -285,8 +285,9 @@ export class TuiController { for (const r of state.roots) state.expanded.add(r.id); } - // Flatten visible nodes for rendering (uses module-level VisibleNode type) - const buildVisible = () => buildVisibleNodes(state); + // Flatten visible nodes for rendering. Returns the cached result when + // available so scroll and detail-update events avoid a full tree traversal. + const buildVisible = () => state.cachedVisibleNodes ?? buildVisibleNodes(state); const help = listComponent.getFooter(); const detail = detailComponent.getDetail(); @@ -2938,10 +2939,18 @@ function updateDetailForIndex(idx: number, visible?: VisibleNode[]) { // Event handlers (named so they can be removed during cleanup) // Centralized list selection handler to keep detail updates/rendering // consistent across mouse and keyboard interactions. + // Uses the cached visible nodes (no tree traversal) for scroll/navigation. const updateListSelection = (idx: number, source?: string) => { + const scrollStart = perfEnabled ? performance.now() : null; const visible = buildVisible(); updateDetailForIndex(idx, visible); screen.render(); + if (perfEnabled && scrollStart !== null) { + const scrollEnd = performance.now(); + const dur = scrollEnd - scrollStart; + perfMetrics.push({ event: 'scroll', start: scrollStart, end: scrollEnd, duration: dur }); + debugLog(`scroll/select (${source ?? 'unknown'}) took ${dur.toFixed(2)} ms`); + } }; const listSelectHandler = (_el: any, idx: number) => { @@ -3095,7 +3104,7 @@ function updateDetailForIndex(idx: number, visible?: VisibleNode[]) { const visible = buildVisible(); const node = visible[idx]; if (node && node.hasChildren) { - state.expanded.add(node.item.id); + incrementalExpand(state, idx); renderListAndDetail(idx); } }); @@ -3107,15 +3116,14 @@ function updateDetailForIndex(idx: number, visible?: VisibleNode[]) { const node = visible[idx]; if (!node) return; if (node.hasChildren && state.expanded.has(node.item.id)) { - state.expanded.delete(node.item.id); + incrementalCollapse(state, idx); renderListAndDetail(idx); return; } // collapse parent if possible const parentIdx = findParentIndex(idx, visible); if (parentIdx >= 0) { - const parent = visible[parentIdx]; - state.expanded.delete(parent.item.id); + incrementalCollapse(state, parentIdx); renderListAndDetail(parentIdx); } }); @@ -3144,8 +3152,11 @@ function updateDetailForIndex(idx: number, visible?: VisibleNode[]) { debugLog(`Expand/collapse no-op took ${durEarly.toFixed(2)} ms (start=${start.toFixed(3)}ms end=${endEarly.toFixed(3)}ms)`); return; } - if (state.expanded.has(node.item.id)) state.expanded.delete(node.item.id); - else state.expanded.add(node.item.id); + if (state.expanded.has(node.item.id)) { + incrementalCollapse(state, idx); + } else { + incrementalExpand(state, idx); + } renderListAndDetail(idx); // persist state void persistence.savePersistedState(db.getPrefix?.() || undefined, { expanded: Array.from(state.expanded) }); diff --git a/src/tui/state.ts b/src/tui/state.ts index 27d48327..87a08399 100644 --- a/src/tui/state.ts +++ b/src/tui/state.ts @@ -13,6 +13,8 @@ export type TuiState = { expanded: Set; listLines: string[]; moveMode: MoveMode | null; + /** Cached result of buildVisibleNodes. Null when the cache is stale. */ + cachedVisibleNodes: VisibleNode[] | null; }; export type VisibleNode = { item: Item; depth: number; hasChildren: boolean }; @@ -68,6 +70,10 @@ export const rebuildTreeState = (state: TuiState): void => { for (const id of Array.from(state.expanded)) { if (!state.itemsById.has(id)) state.expanded.delete(id); } + + // Invalidate the visible-node cache so the next buildVisibleNodes call + // performs a full traversal and repopulates it. + state.cachedVisibleNodes = null; // Lightweight timing to help diagnose expensive tree rebuilds in the TUI try { const dur = Date.now() - t0; @@ -87,6 +93,7 @@ export const createTuiState = (items: Item[], showClosed: boolean, persistedExpa expanded: new Set(), listLines: [], moveMode: null, + cachedVisibleNodes: null, }; if (persistedExpanded && Array.isArray(persistedExpanded)) { @@ -111,9 +118,88 @@ export const buildVisibleNodes = (state: TuiState): VisibleNode[] => { for (const r of state.roots) visit(r, 0); try { (state as any).__lastBuildVisibleMs = Date.now() - t0; } catch (_) {} + state.cachedVisibleNodes = out; return out; }; +/** + * Build the visible VisibleNode sub-list for the children of a given item, + * traversing only nodes that are currently expanded. Used by incremental + * expand to splice in just the new subtree rather than rebuilding everything. + */ +function buildSubtreeNodes(state: TuiState, parentId: string, depth: number): VisibleNode[] { + const out: VisibleNode[] = []; + const children = (state.childrenMap.get(parentId) || []).slice().sort(sortBySortIndexDateAndId); + for (const child of children) { + const grandchildren = state.childrenMap.get(child.id) || []; + out.push({ item: child, depth, hasChildren: grandchildren.length > 0 }); + if (grandchildren.length > 0 && state.expanded.has(child.id)) { + out.push(...buildSubtreeNodes(state, child.id, depth + 1)); + } + } + return out; +} + +/** + * Incrementally expand the node at `nodeIdx` in the cached visible-node list. + * + * Instead of re-traversing the entire tree, this function: + * 1. Sets the node as expanded in `state.expanded`. + * 2. Builds only the newly visible subtree. + * 3. Splices the subtree into `state.cachedVisibleNodes`. + * + * Falls back to a full `buildVisibleNodes` traversal if the cache is stale. + */ +export const incrementalExpand = (state: TuiState, nodeIdx: number): VisibleNode[] => { + const cached = state.cachedVisibleNodes; + if (!cached) return buildVisibleNodes(state); + + const node = cached[nodeIdx]; + if (!node || !node.hasChildren) return cached; + if (state.expanded.has(node.item.id)) return cached; + + state.expanded.add(node.item.id); + const subtree = buildSubtreeNodes(state, node.item.id, node.depth + 1); + const newVisible = cached.slice(0, nodeIdx + 1).concat(subtree, cached.slice(nodeIdx + 1)); + state.cachedVisibleNodes = newVisible; + return newVisible; +}; + +/** + * Incrementally collapse the node at `nodeIdx` in the cached visible-node list. + * + * Instead of re-traversing the entire tree, this function: + * 1. Removes the node from `state.expanded`. + * 2. Finds the contiguous block of descendants that follow the node in the + * visible list (all nodes at a greater depth). + * 3. Removes that block from `state.cachedVisibleNodes`. + * + * Falls back to a full `buildVisibleNodes` traversal if the cache is stale. + */ +export const incrementalCollapse = (state: TuiState, nodeIdx: number): VisibleNode[] => { + const cached = state.cachedVisibleNodes; + if (!cached) return buildVisibleNodes(state); + + const node = cached[nodeIdx]; + if (!node) return cached; + + state.expanded.delete(node.item.id); + + // Find the end of the descendant block: all nodes with depth > node.depth + // that immediately follow the collapsed node are now hidden. + let endIdx = nodeIdx + 1; + while (endIdx < cached.length && cached[endIdx].depth > node.depth) endIdx++; + + if (endIdx === nodeIdx + 1) { + // No visible descendants – nothing to remove. + return cached; + } + + const newVisible = cached.slice(0, nodeIdx + 1).concat(cached.slice(endIdx)); + state.cachedVisibleNodes = newVisible; + return newVisible; +}; + export const expandAncestorsForInProgress = (state: TuiState): void => { const inProgressItems = state.currentVisibleItems.filter((item) => { return item.status === 'in-progress'; diff --git a/tests/tui/incremental-rendering.test.ts b/tests/tui/incremental-rendering.test.ts new file mode 100644 index 00000000..fb6a06ee --- /dev/null +++ b/tests/tui/incremental-rendering.test.ts @@ -0,0 +1,287 @@ +import { describe, it, expect } from 'vitest'; +import { performance } from 'perf_hooks'; +import { + createTuiState, + rebuildTreeState, + buildVisibleNodes, + incrementalExpand, + incrementalCollapse, +} from '../../src/tui/state.js'; + +// ── helpers ─────────────────────────────────────────────────────────────────── + +type WI = { + id: string; + title: string; + status: string; + parentId?: string | null; + sortIndex?: number; + createdAt?: string; +}; + +function makeItem(id: string, parentId?: string | null): WI { + return { + id, + title: `Item ${id}`, + status: 'open', + parentId: parentId ?? null, + sortIndex: 0, + createdAt: new Date().toISOString(), + }; +} + +// ── Unit tests: incremental expand ─────────────────────────────────────────── + +describe('incrementalExpand', () => { + it('returns cached nodes unchanged when node has no children', () => { + const items = [makeItem('r'), makeItem('c', 'r')]; + const state = createTuiState(items as any, false, []); + // prime the cache without expanding + buildVisibleNodes(state); + // node 0 ('r') has children but is not expanded — visible list = ['r'] + // Trying to expand a leaf at index 0 that has no children in visible list + // i.e. 'r' has hasChildren=true but its children are hidden — expand it + const initial = state.cachedVisibleNodes!.slice(); + expect(initial.length).toBe(1); + + const after = incrementalExpand(state, 0); + expect(after.length).toBe(2); + expect(after[0].item.id).toBe('r'); + expect(after[1].item.id).toBe('c'); + expect(state.expanded.has('r')).toBe(true); + expect(state.cachedVisibleNodes).toBe(after); + }); + + it('returns cache unchanged when node is already expanded', () => { + const items = [makeItem('r'), makeItem('c', 'r')]; + const state = createTuiState(items as any, false, ['r']); + buildVisibleNodes(state); + const before = state.cachedVisibleNodes!; + const after = incrementalExpand(state, 0); + expect(after).toBe(before); // same reference — no work done + }); + + it('falls back to full build when cache is stale', () => { + const items = [makeItem('r'), makeItem('c', 'r')]; + const state = createTuiState(items as any, false, []); + // do not call buildVisibleNodes — cache stays null + expect(state.cachedVisibleNodes).toBeNull(); + const after = incrementalExpand(state, 0); + expect(after.length).toBeGreaterThan(0); + expect(state.cachedVisibleNodes).toBe(after); + }); + + it('inserts nested subtree at the correct position', () => { + // root1 -> child1 -> grandchild1 + // root2 + const items = [ + makeItem('root1'), + makeItem('child1', 'root1'), + makeItem('grand1', 'child1'), + makeItem('root2'), + ]; + const state = createTuiState(items as any, false, ['root1']); + buildVisibleNodes(state); + // visible: root1, child1 (hasChildren=true but collapsed), root2 + expect(state.cachedVisibleNodes!.map(n => n.item.id)).toEqual(['root1', 'child1', 'root2']); + + // expand child1 (index 1) + const after = incrementalExpand(state, 1); + expect(after.map(n => n.item.id)).toEqual(['root1', 'child1', 'grand1', 'root2']); + }); +}); + +// ── Unit tests: incremental collapse ───────────────────────────────────────── + +describe('incrementalCollapse', () => { + it('removes visible descendants on collapse', () => { + const items = [makeItem('r'), makeItem('c', 'r')]; + const state = createTuiState(items as any, false, ['r']); + buildVisibleNodes(state); + expect(state.cachedVisibleNodes!.length).toBe(2); + + const after = incrementalCollapse(state, 0); + expect(after.length).toBe(1); + expect(after[0].item.id).toBe('r'); + expect(state.expanded.has('r')).toBe(false); + expect(state.cachedVisibleNodes).toBe(after); + }); + + it('returns cache unchanged when node has no visible descendants', () => { + const items = [makeItem('r'), makeItem('c', 'r')]; + const state = createTuiState(items as any, false, []); + buildVisibleNodes(state); + // 'r' not expanded — no visible descendants + const before = state.cachedVisibleNodes!; + const after = incrementalCollapse(state, 0); + expect(after).toBe(before); + }); + + it('falls back to full build when cache is stale', () => { + const items = [makeItem('r'), makeItem('c', 'r')]; + const state = createTuiState(items as any, false, ['r']); + state.cachedVisibleNodes = null; // simulate stale cache + const after = incrementalCollapse(state, 0); + expect(after.length).toBeGreaterThan(0); + }); + + it('collapses multiple levels of descendants at once', () => { + const items = [ + makeItem('r'), + makeItem('c', 'r'), + makeItem('gc', 'c'), + ]; + const state = createTuiState(items as any, false, ['r', 'c']); + buildVisibleNodes(state); + // visible: r, c, gc + expect(state.cachedVisibleNodes!.map(n => n.item.id)).toEqual(['r', 'c', 'gc']); + + const after = incrementalCollapse(state, 0); + expect(after.map(n => n.item.id)).toEqual(['r']); + }); +}); + +// ── Regression: cache is invalidated on rebuild ─────────────────────────────── + +describe('cache invalidation', () => { + it('sets cachedVisibleNodes to null when rebuildTreeState is called', () => { + const items = [makeItem('r')]; + const state = createTuiState(items as any, false, []); + buildVisibleNodes(state); + expect(state.cachedVisibleNodes).not.toBeNull(); + + rebuildTreeState(state); + expect(state.cachedVisibleNodes).toBeNull(); + }); + + it('rebuild followed by buildVisibleNodes repopulates the cache', () => { + const items = [makeItem('r'), makeItem('c', 'r')]; + const state = createTuiState(items as any, false, ['r']); + buildVisibleNodes(state); + expect(state.cachedVisibleNodes!.length).toBe(2); + + rebuildTreeState(state); + expect(state.cachedVisibleNodes).toBeNull(); + + const fresh = buildVisibleNodes(state); + expect(fresh.length).toBe(2); + expect(state.cachedVisibleNodes).toBe(fresh); + }); +}); + +// ── 30-item benchmark ───────────────────────────────────────────────────────── + +const ROOT_COUNT = 10; +const CHILDREN_PER_ROOT = 2; +const MAX_MEDIAN_LATENCY_MS = 200; + +describe('30-item benchmark', () => { + /** + * Build a 30-item tree: 10 root items, each with 2 children. + * Expand all roots, then measure expand/collapse latency with the + * incremental renderer. The median must stay under 200 ms. + */ + it('median expand/collapse latency under 200 ms for a 30-item tree', () => { + const items: WI[] = []; + const rootIds: string[] = []; + + for (let r = 0; r < ROOT_COUNT; r++) { + const rid = `root-${r}`; + rootIds.push(rid); + items.push(makeItem(rid)); + for (let c = 0; c < CHILDREN_PER_ROOT; c++) { + items.push(makeItem(`child-${r}-${c}`, rid)); + } + } + + const state = createTuiState(items as any, false, []); + + // Expand all roots so children are visible, then prime the cache. + for (const rid of rootIds) state.expanded.add(rid); + rebuildTreeState(state); + buildVisibleNodes(state); + + expect(state.cachedVisibleNodes!.length).toBe(ROOT_COUNT * (1 + CHILDREN_PER_ROOT)); + + // Measure collapse + expand for each root in sequence. + const durations: number[] = []; + + // locate the index of each root in the current visible list + for (const rid of rootIds) { + const visibleBeforeCollapse = state.cachedVisibleNodes!; + const rootIdx = visibleBeforeCollapse.findIndex(n => n.item.id === rid); + if (rootIdx < 0) continue; + + const t0 = performance.now(); + incrementalCollapse(state, rootIdx); + const t1 = performance.now(); + durations.push(t1 - t0); + + // root is now collapsed — expand it back + const visibleAfterCollapse = state.cachedVisibleNodes!; + const rootIdxAfter = visibleAfterCollapse.findIndex(n => n.item.id === rid); + if (rootIdxAfter < 0) continue; + + const t2 = performance.now(); + incrementalExpand(state, rootIdxAfter); + const t3 = performance.now(); + durations.push(t3 - t2); + } + + expect(durations.length).toBeGreaterThan(0); + + durations.sort((a, b) => a - b); + const medianMs = durations[Math.floor(durations.length / 2)]; + + expect( + medianMs, + `Median expand/collapse latency ${medianMs.toFixed(2)} ms must be < ${MAX_MEDIAN_LATENCY_MS} ms`, + ).toBeLessThan(MAX_MEDIAN_LATENCY_MS); + }); + + /** + * Smoke test: visible node count stays consistent through sequential + * expand / collapse operations. + */ + it('visible node count is consistent after repeated incremental expand/collapse', () => { + const items: WI[] = []; + const rootIds: string[] = []; + + for (let r = 0; r < ROOT_COUNT; r++) { + const rid = `root-${r}`; + rootIds.push(rid); + items.push(makeItem(rid)); + for (let c = 0; c < CHILDREN_PER_ROOT; c++) { + items.push(makeItem(`child-${r}-${c}`, rid)); + } + } + + const state = createTuiState(items as any, false, []); + rebuildTreeState(state); + buildVisibleNodes(state); + + // All roots collapsed initially — ROOT_COUNT visible items + expect(state.cachedVisibleNodes!.length).toBe(ROOT_COUNT); + + // Expand all roots one by one + for (const rid of rootIds) { + const idx = state.cachedVisibleNodes!.findIndex(n => n.item.id === rid); + incrementalExpand(state, idx); + } + expect(state.cachedVisibleNodes!.length).toBe(ROOT_COUNT * (1 + CHILDREN_PER_ROOT)); + + // Collapse all roots one by one + // Walk backwards so indices don't shift under us + for (const rid of [...rootIds].reverse()) { + const idx = state.cachedVisibleNodes!.findIndex(n => n.item.id === rid); + if (idx >= 0) incrementalCollapse(state, idx); + } + expect(state.cachedVisibleNodes!.length).toBe(ROOT_COUNT); + + // Cross-check against a fresh full traversal + const freshVisible = buildVisibleNodes( + createTuiState(items as any, false, []), + ); + expect(freshVisible.length).toBe(ROOT_COUNT); + }); +});