Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .changeset/window-spacer-measured-mean.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
---
"@pretable/core": minor
"@pretable/react": minor
---

Windowed spacers are sized from what rows have measured, not from the default
row height.

A windowed grid reserves the unloaded regions as spacers, and `getWindowSpacers`
reports those regions as row **counts** — how many rows sit before and after the
loaded window. The controller turned a count into pixels by multiplying by
`defaultRowHeight`. Its own comment said so: _"Row counts, not pixel heights."_

That is the region's real height only on a grid whose rows are all the default
height. On a grid whose rows wrap it is a systematic understatement of the whole
scroll extent, by the ratio between a wrapped row and the unwrapped default —
and the retained-measurement cache, which knows exactly what those rows were
worth, was never consulted for geometry at all. It is keyed by row identity
while the spacer arrives as a count, so the two systems had no way to meet.

The controller now prices a spacer's rows at
`RowHeightIndex.getMeasuredHeightMean()` — the mean of every height the DOM has
reported, the retained heights of evicted rows included — falling back to
`defaultRowHeight` until something has been measured. A grid that has measured
nothing, and every grid with no window at all, is byte-for-byte unchanged.

It remains an **estimate**: a count cannot say which rows are out there, so the
extent tracks the result's size without reproducing its height. The docs
previously claimed the spacer "reproduces the region's height precisely" where
retained heights were exact, which the code could not do and now does not claim.
`eviction.mdx` and `windowing.mdx` say what it actually computes, and that the
viewport anchor is what absorbs the residual.

The mean is aggregated structurally — every hash node in the persistent height
index carries the sum of its values beside the count it already carried — rather
than threaded as a running total through `measure`, `apply`, retention eviction
and the cooperative replacement builder, so a copy-on-write rebuild cannot leave
it stale.
4 changes: 3 additions & 1 deletion apps/website/content/docs/server-data/eviction.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ So the discriminator is the window, and everything below rides the same honesty

Retention is bounded, because a ledger of every row a long-lived grid has ever shown is the memory eviction exists to bound. Past the bound — a hundred thousand retained measurements — the coldest entries go first, and a row returning from beyond it is estimated again like any row the grid has never seen.

**The scroll position does not move under the reader.** An evicted region does not stop occupying space: windowing's spacers cover it, sized from the population rather than from what is loaded. Where the retained heights are exact the spacer reproduces the region's height precisely, so the scroll extent is the same number after the eviction as before it and nothing shifts at all. Where the spacer is an estimate — rows that were never measured, or measurements that fell past the retention bound — the geometry genuinely does change, and the viewport anchor absorbs it: the row the reader is looking at keeps the position on screen it had, while the coordinates around it are rebuilt. Without the spacer the extent would collapse to the loaded rows, which is what makes that a claim rather than a tautology.
**The scroll position does not move under the reader.** An evicted region does not stop occupying space: windowing's spacers cover it, sized from the population rather than from what is loaded. What the grid knows about that region is how many rows are in it, never which ones, so it sizes the spacer at the mean height of every row it has actually measured — the ones on screen and the retained heights of the ones that have left — and falls back to the theme's row height until it has measured anything at all. That keeps the scroll extent tracking the size of the result even on a grid whose rows wrap well past the default height, which sizing at the default does not.

It is an estimate, and it stays one. The evicted rows are not all worth the mean, so the geometry genuinely does change across an eviction, and it changes again as more rows are measured and the mean improves. The viewport anchor is what absorbs that: the row the reader is looking at keeps the position on screen it had, while the coordinates around it are rebuilt. Without the spacer the extent would collapse to the loaded rows, which is what makes that a claim rather than a tautology.

**A cell selection survives its rows being released.** This is the hard one, because a cell range is defined by its two endpoint rows, and endpoints are exactly what eviction takes away. A range therefore records the dataset span it covers as well as its endpoints, and the span is what answers questions while the rows are gone: how many rows are selected is arithmetic over the span with nothing loaded — `getCellSelectionSummary()` on the grid handle reads it — and whether a rendered row is selected is containment on that row's dataset position. The ordinary sliding case is the one that matters and is covered: a slide that clears the range's start while its end is still loaded keeps the range whole rather than collapsing it onto the survivor, and scrolling back repaints the rows that were never unselected. A row proven **deleted** inside the loaded span still prunes the range, which is the same rule from the other side.

Expand Down
2 changes: 2 additions & 0 deletions apps/website/content/docs/server-data/windowing.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ Two things change, and they are the same claim seen from two sides.

**The scroll extent describes the population.** The unmaterialized regions are reserved as spacers — `start` rows ahead of the window, and whatever the total says follows its end behind it — so the scrollbar measures 480 rows while a hundred are in memory, and a reader dragging it is moving through the result rather than through your cache. A total on its own never does this: as [Totals and honesty](/docs/server-data/totals) says, a grid with 200 rows and a claimed 10,000 scrolls 200 rows. The window is the part that says where the other rows would be.

A spacer is a row count, not a pixel height, so the grid prices those rows at the mean height of the rows it has measured — the theme's row height until it has measured one. The extent is therefore an estimate of the result's height, accurate about how many rows are out there and approximate about how tall they are. [Eviction](/docs/server-data/eviction#what-survives) covers what that costs and what absorbs it.

**Every row reports its dataset position.** `aria-rowindex` on a body row counts from the population, not from the array: with the window above, the row holding record 100 publishes `aria-rowindex="102"` — one for a zero-based index becoming ARIA's one-based one, and one for the header row, which is always row 1.

Those two are gated together, by one rule:
Expand Down
135 changes: 135 additions & 0 deletions docs/superpowers/plans/2026-08-17-spacer-accuracy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
# Spacer accuracy, and the claims that rest on it

> **For agentic workers:** REQUIRED SUB-SKILL: superpowers:subagent-driven-development.

**Goal:** Make the windowed spacer reflect what rows actually measure, and make every published claim about it true.

**Context:** An audit of the eviction project found the spacer never consults a retained height, a published exactness claim the code cannot honour, and the test that appeared to prove otherwise feeding the planner a number production never computes.

---

## The defect

`packages/renderer-dom/src/row-layout-controller.ts` (~line 716):

```ts
const leadingHeight = Math.max(0, (spacers?.leadingRows ?? 0) * defaultRowHeight);
const trailingHeight = Math.max(0, (spacers?.trailingRows ?? 0) * defaultRowHeight);
```

The comment states it plainly — *"Row counts, not pixel heights."* Three consequences:

1. **The retained-measurement cache is never read for spacer geometry.** It is keyed by row identity (`row-height-index.ts:1146` `retainMeasurement`); `getWindowSpacers` (`renderer-dom/src/types.ts:386`) supplies only `leadingRows`/`trailingRows` counts. The two systems cannot meet.
2. **`estimate()` floors at `defaultRowHeight`**, so every evicted row is understated whenever rows wrap. A 10,000-row grid averaging 96px against a 48px default publishes an extent about **half** the truth, and it moves every time the window moves. This is the wrapped-text case — the feature's entire differentiator.
3. **Spec §4's cost model is inverted.** It claims evicting *measured* rows is free and only unmeasured rows cost an anchor correction. In practice every eviction costs one.

### The published claim is false

`apps/website/content/docs/server-data/eviction.mdx`:

> *"sized from the population rather than from what is loaded. Where the retained heights are exact the spacer reproduces the region's height precisely, so the scroll extent is the same number after the eviction as before it and nothing shifts at all."*

It reproduces the region's height precisely only when every evicted row measured exactly `defaultRowHeight`.

### And the test cannot see it

`packages/layout-core/src/__tests__/eviction-anchor.test.ts:99-105` calls `planViewport({ leadingHeight: sumHeights(0, EVICT_BEFORE) })` — the exact sum of the evicted rows' measured heights. `planViewport` is pure and uses what it is handed. **The controller never computes that number.** The assertion is real; the quantity is not the one the product produces. Same shape as the row-height-error proxy this repo already fixed.

---

### Task 1: A test that drives the real spacer

**Do this before changing any production code.**

The existing anchor test exercises `planViewport` directly, so it can never see this bug. Add coverage in `packages/renderer-dom` that drives `createRowLayoutController` with `getWindowSpacers` returning nonzero `leadingRows`, **varied row heights that are not the default**, and measurements retained for the evicted rows.

Assert the published `totalHeight` against the truth — the sum of what those rows actually measured.

- [ ] **Step 1: Write it and watch it fail.** Expected: the extent is short by `(measured − default) × leadingRows`. Report the actual numbers, not just red/green.
- [ ] **Step 2: If it passes, STOP and report.** The diagnosis is wrong and the rest of this plan is void.

Fixture requirements, because this repo has shipped four vacuous tests in a week:
- Heights must **differ from `defaultRowHeight`**, or the bug is invisible by construction.
- Heights must **vary between rows** (`30 + ((i * 7) % 23)` is the established idiom), or arithmetic errors land on multiples of the row height and look right.
- The spacer must be **nonzero**, or every conversion is an identity.

### Task 2: Decide how the spacer learns heights

**This is the plan's one real design decision. Resolve it before implementing.**

**Option A — calibrated mean (recommended).** Give `RowHeightIndex` a running sum and count of retained measurements, exposed as a mean. The controller multiplies the spacer's row count by that instead of `defaultRowHeight`, falling back to `defaultRowHeight` when nothing has been measured.

- No consumer API change; no new information required from anyone.
- Turns a systematic understatement into an unbiased estimate: a 96px-average grid gets a ~96px-per-row spacer instead of 48.
- Still an **estimate**. Rows are not uniform, so the extent will not be exact — and anchoring is what absorbs the residual, which is precisely what `eviction-anchor.test.ts` was written to prove and would now be proving about a real quantity.
- The index already tracks `measurementCacheCount` (`row-height-index.ts:134`), so the shape exists.

**Option B — exact per-region.** `getWindowSpacers` carries heights, or row keys, rather than counts. Exact when the consumer knows what it evicted — but it is a public API change, it pushes bookkeeping onto every consumer, and a consumer that windows without having ever rendered a row has no heights to give.

**Option C — neither; correct the claim only.** Cheapest, and leaves the differentiator understated by 2× on wrapped grids.

**Recommendation: A, and C regardless** — A does not make the spacer exact, so the exactness claim has to go either way.

- [ ] Decide, and record the reasoning in the commit message.

### Task 3: Implement

- [ ] Implement the chosen option.
- [ ] **Mutate:** revert the calibration and confirm Task 1's test reddens with the specific pixel gap. Report both directions verbatim.
- [ ] Confirm a grid with **no** retained measurements is byte-for-byte unchanged — that is the local-mode and cold-start regression guard.

### Task 4: Make the published claims true

- [ ] `eviction.mdx` — replace the exactness claim with what the code does: the spacer is estimated from what rows have actually measured, and the anchor absorbs the residual. Say the extent is an estimate that improves as more rows are measured.
- [ ] `docs/superpowers/specs/2026-08-14-eviction-design.md` §4 — the cost model is inverted; correct it.
- [ ] Check for other places asserting spacer exactness (`grep -rn "precisely\|exact" apps/website/content/docs/server-data/`).

### Task 5: Verify

Baselines must be **measured on `origin/main` first** — numbers in this document may be stale.

```bash
npx vitest run --root packages/renderer-dom
npx vitest run --root packages/layout-core
npx vitest run --root packages/grid-core
pnpm --filter @pretable/react test
pnpm --filter @pretable/app-bench test
./node_modules/.bin/playwright test
pnpm --filter @pretable/app-website test
```

Then `pnpm build && pnpm api && pnpm api:check`, in that order.

Changeset: **minor** for affected public packages (pre-1.0; breaking ships as minor, never major).

---

## What this does NOT fix

Stated so the next reader does not assume otherwise:

- **Memory is still unmeasured.** See the separate plan item below. The spacer is about *geometry*, not about bytes.
- **No evictor ships.** Spec §3 remains absent; consumers do the releasing.
- The spacer remains an **estimate** under Option A. Exactness needs Option B and consumer cooperation.

---

## Next, after the spacer

Ranked. Each is independent.

1. **Measure memory, or stop claiming it.** `resident-cap-memory.spec.ts` runs an *append* script through an adapter that passes no `resultMeta`, so eviction is structurally unreachable; doubling resident rows moved the heap by −0.28 MB. Either instrument the windowed harness — which does evict — and assert that heap falls when the window shrinks, or withdraw the bounded-memory claim until something proves it. **This is the feature's central premise and nothing tests it.**
2. **`PretableCellRangeFor` is missing `datasetRowSpan`** (`react/src/surface-types.ts`). It is the type the docs tell controlled consumers to use, and the value crosses via an `as unknown as` launder. A consumer following the documented recipe who rebuilds range objects loses the span with no type error, and it presents as "eviction doesn't work". Replace the six structural re-declarations with one shared interface.
3. **The Tab branch still has the `-1` sentinel bug** that #453 removed from the page keys (`pretable-surface.tsx:7473`). Latent — needs `tabBehavior="wrap-rows"` — but it is the identical defect.
4. **`verified` reaches no UI.** A public field with no consumer and no documentation; the live-region announcement states an unverified count as fact. Either wire it into the announcement or stop paying for it.
5. **Delete dead weight.** `getScrollTopForIndexedFocus` has zero callers, is exported, and its signature invites the coordinate-space bug the `ScrollRequest` seam exists to prevent. `sameDatasetRowSpan` duplicates `sameSpan` byte-for-byte.
6. **Bench specs are neither typechecked nor linted** — `apps/bench/tsconfig.json` includes only `["src", "vite.config.ts"]`, and lint is `eslint src`. CI now runs those 9 specs; nothing checks them.

**Not on this list, and deliberately:** #452, #457 and #458 are open perf issues filed against the comparative bench. #457 — *S2 sort at 50k rows never settles* — reads as more serious than anything above. They are a different thread and want their own triage.

## Two decisions owed by a human

Carried from the P0 fixes, unchanged:

1. Whether to add a **consumer-supplied population token**, the only thing that closes the equal-insert-and-delete gap a size comparison cannot see.
2. Whether `indexedRangeContainsCell` becomes **tri-state**, so an unconfirmed span paints distinctly rather than as ordinary selection. UI and aria consequences beyond the engine.
Loading