perf(workstation): replace rail history row divisor with line-aware budgeting - #2066
Conversation
…udgeting The stacked (rail) history panel estimated visible item count with a fixed lines-per-item divisor (1.4 with bucketing, 2 without), which under-fills the panel whenever the actual mix skews toward one-line rows even though the header says "more below". Replace the estimate with a line-aware window: grow the requested item count and measure each candidate's actual rendered line cost (mirroring the render path's own one-line/two-line collapse rule), keeping the largest candidate that still fits the panel's line budget. Also fixes a related bug in toFullGraphItems surfaced by the new regression tests: when a sticky bucket-header is needed and the window is pinned at the end of the commit list, the header used to eat a budget slot without the window sliding to compensate, silently trimming the selected commit off the render. Ports the same slide-until- visible fix toCompactItems already had.
![gfargo-horizon-agent[bot]](https://avatars.githubusercontent.com/in/4083186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Agent review (sonnet) — CONCERNS
REVIEW: CONCERNS
RESOLVES: full
The PR faithfully implements the approved plan's line-aware windowing and correctly fixes the panel under-fill bug, with solid new regression tests and a legitimate incidental fix to a selection-visibility bug in toFullGraphItems. However, fillStackedWindow's approach of calling getVisibleLogInkHistory up to ~20 times per render — each call rebuilding an uncached O(commits) array in toFullGraphItems — introduces a measured ~34x per-render slowdown (120ms+ at the default 1000-commit history size, 500ms+ at 8000 commits), a real regression for a PR titled 'perf'.
⚠️ fillStackedWindow scan multiplies an uncached O(commits) rebuild —src/workstation/surfaces/history/index.ts:561— getVisibleLogInkHistory -> toFullGraphItems rebuilds itsexpandedarray (iterating all of state.commits) on every call, uncached. fillStackedWindow now calls this up to lineBudget (~17-20) times per render. Benchmarked on this branch: at the default interactive history size (1000 commits, fullGraph=true by default) the scan took ~120ms vs ~3.5ms for the old single-call estimate; at 8000 commits (a few 'load more' pages) it took ~525ms — visible input lag on every cursor move in the rail tier. The plan's 'negligible' perf claim relied on the layout WeakMap cache, but the expanded-array rebuild (headers/commits/transitions + getDateBucket per commit) is not cached and dominates cost.⚠️ No regression test for non-monotonic line-cost windowing —src/workstation/surfaces/history/historyRender.test.ts:149— fillStackedWindow breaks its growth loop at the first candidate whose measured cost exceeds lineBudget rather than continuing to scan, so if re-centering or a sticky-header toggling on/off makes a larger window's cost dip back under budget, that fill opportunity is missed. Both new tests use monotonic cost sequences (uniform one-line, or strictly alternating one-/two-line), so this path has zero coverage.- 🧹 Floor candidate (n=2) not itself budget-checked —
src/workstation/surfaces/history/index.ts:559—bestis seeded from the n=2 candidate unconditionally before the loop's cost check runs, so if lineBudget < 4 (only possible below the documented 80x24 floor) and both floor items are 2-line, the returned window overflows. Not reachable at the supported floor, but the 'never overflow' framing isn't unconditional.
There was a problem hiding this comment.
🔎 Agent re-review (sonnet, delta) — LGTM
REVIEW: LGTM
RESOLVES: full
The revise fixes all three prior CONCERNS: it splits full-graph windowing into a cached context (buildFullGraphContext) plus a cheap per-size probe (createVisibleLogInkHistoryProbe), fixing the O(commits × probes) rebuild and adding a spy-based regression test proving the scan runs once; it adds a targeted regression test proving fillStackedWindow doesn't stop at the first over-budget candidate; and it raises the line budget floor to 4 so the unconditional 2-item floor can't exceed it at supported terminal sizes. Lint, tsc, and the full history test suite pass, and no unrelated files were touc
1 nit — 1 inline on the diff
There was a problem hiding this comment.
🔎 Agent re-review (sonnet, delta) — LGTM
REVIEW: LGTM
RESOLVES: full
The branch is identical to origin/main (PR #2066 already merged, single-commit diff, nothing new pushed) — there is no delta to review.
What
Replaces the rail-tier (
rowMode: 'stacked') history panel's fixed lines-per-item divisor (1.4with date bucketing,2without) with line-aware viewport budgeting: the window grows item-by-item, measuring each candidate's actual rendered line cost, and keeps the largest candidate that still fits the panel.Why
Closes #1820
Plane: OSS-1163
After #1810 collapsed ref-less bucketed commits to one line, most visible commits in rail mode are one line — but the fixed
/1.4divisor still assumed a mixed average, so it requested too few history items and left a large empty area at the bottom of the panel even when the header said "more below".How
src/workstation/surfaces/history/index.ts: addedstackedItemLineCost(mirrorsrenderStackedCommitHistoryRow's one-line/two-line collapse rule) andfillStackedWindow(grows the requested item count viagetVisibleLogInkHistory, measuring real line cost, keeping the largest candidate ≤ the line budget). ReplacedstackedDivisor/listRowswith this inrenderHistoryPanel. ThesinglerowMode path is unchanged.src/workstation/chrome/historyRows.ts: fixed a related bug intoFullGraphItemssurfaced by the new regression tests — when a sticky bucket-header is needed and the window is pinned at the end of the commit list, the header used to eat a budget slot without the window sliding to compensate, silently trimming the selected commit off the render. Ported the same slide-until-visible fixtoCompactItemsalready had.src/workstation/surfaces/history/historyRender.test.ts: added regression coverage for a mostly one-line window (asserts the panel fills its line budget instead of under-filling) and a mixed one-/two-line window (asserts no overflow and the selected commit stays visible at end-of-list).Testing
npm run build)npm run test:jest— full suite green aside from 5 unrelated suites killed by sandbox OOM; targetedhistory/historyRowssuites: 59/59 passing)npm run lint,eslinton changed files)🤖 Generated by the harbor agent loop. Reviewed by a human before merge.
Closes #1820