Skip to content

Commit fa0cf00

Browse files
bloveclaude
andauthored
Stop the hero destroying its own model in StrictMode (#383)
* fix(bench): make a run prove which build it measured Closes #355. `bench:matrix` printed `Error: Port 4173 is already in use` and then ran the entire matrix against whatever was already there — another worktree's `preview:bench` — writing ordinary-looking artifacts that described a different branch's code. On #327 that inverted a conclusion for hours: post-fix runs kept reporting `partial`, which read as "your fix did not work" rather than "you are measuring someone else's build". The hole is in `waitForServer`: it asks whether SOMETHING answers, and a foreign server answers instantly. Nothing downstream ever asked *what* answered. `vite build` now mints a fresh id, writes it to `dist/bench-build-id.txt`, and injects the same value into the bundle, which publishes it on `<html>`. Two independent checks read it: - `bench-matrix.mjs` fetches the id from the server before running anything, so a collision fails in the first second with the port to look at rather than after a full matrix. - `bench.spec.ts` compares the served id to `dist/` before it measures, so a run driven any other way — directly, from an editor, with a hand-set BASE_URL — gets the same protection. A git SHA cannot do this job: the tree is dirty for most of a development loop, so the SHA is identical across builds that differ. A per-build id answers the question actually being asked, and catches a stale `dist/` and a forgotten rebuild as well. `PRETABLE_BENCH_SKIP_BUILD_CHECK=1` opts out when the target is deliberately not a local build. Verified against the live collision that motivated it — the other worktree's server still holding 4173: Error: http://127.0.0.1:4173 is not serving the build this checkout just produced expected (dist/): 03f00ae1-… served: (no id — an older build, or not a bench server) Check `lsof -ti:4173`. and no benchmark ran. The spec-level check was mutation-tested the same way, by pointing it at a server serving a different id. A pre-check build reports no id rather than its index.html, because the preview server falls back to that page for anything it cannot find — only an id-shaped token counts as an answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(website): stop the hero destroying its own model in StrictMode Closes #382. Loading the site in `next dev` rendered no grid at all — one page error before a single row painted: RowLayoutControllerError: A disposed row-layout controller cannot change its columns. at usePretableModelInternal.useLayoutEffect at HeroGrid The hero holds its row model in `useState` and disposed it in an effect cleanup: useEffect(() => () => rowModel.dispose(), [rowModel]); StrictMode mounts, unmounts and remounts every component in dev, and `useState` hands the same instance back — so the rehearsal unmount destroyed the model permanently. The layout controller then marked itself disposed through its model-subscription failure path and threw out of a layout effect. Bisected: the grid has rendered nothing in dev since #321, on every consumer with StrictMode on, which is Next's default. Disposal is now deferred by a microtask and cancelled by the remount — the same shape `usePretable` already uses for the models it owns. A real unmount has no remount to cancel it, so the model is still disposed. Fixing that surfaced a second StrictMode fault behind it: the reduced-motion seed ran `applyTransaction({ add })` twice against a model that now survives the rehearsal, throwing "Row ID NVDA already exists". Seeding is now one-shot. `setRows` would be the idempotent call, but it settles through the model's rebuild rather than committing in place, which leaves the snapshot empty for that frame — and a settled snapshot that paints nothing is the whole failure that branch exists to avoid. Why no test caught either: every website e2e run is against `next start`, a production build, where StrictMode does not double-invoke. The suite was 100/100 green the entire time the dev grid was dead. The new test renders the hero inside `<StrictMode>` and fails without the disposal fix with the exact production error. Verified in Chromium against the dev server, StrictMode on: `grids=1 rows=13`, no page errors. The library is not at fault — the grouping fixture renders correctly in dev throughout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 6f8ae91 commit fa0cf00

2 files changed

Lines changed: 65 additions & 2 deletions

File tree

‎apps/website/app/components/HeroGrid.tsx‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,18 @@ export function HeroGrid() {
141141
rowsRef.current = settled;
142142
// eslint-disable-next-line react-hooks/set-state-in-effect -- one-shot snapshot, runs once then returns
143143
setRows(settled);
144-
rowModel.applyTransaction({ add: settled });
144+
// Seed once. StrictMode runs this effect twice against a model that now
145+
// survives the rehearsal, and a second `add` of the same book throws
146+
// "Row ID NVDA already exists". `setRows` would be the idempotent call
147+
// but it settles through the model's rebuild rather than committing here,
148+
// which leaves the snapshot empty for this frame — and a settled snapshot
149+
// that paints nothing is the whole failure this branch exists to avoid.
150+
// (The streaming path below is safe only by accident: its `added` diff
151+
// comes out empty on the second run.)
152+
if (!seededRef.current) {
153+
seededRef.current = true;
154+
rowModel.applyTransaction({ add: settled });
155+
}
145156
return;
146157
}
147158

@@ -219,7 +230,32 @@ export function HeroGrid() {
219230
replayRef.current?.setPlaying(isPlaying);
220231
}, [isPlaying]);
221232

222-
useEffect(() => () => rowModel.dispose(), [rowModel]);
233+
// Dispose on a REAL unmount, not on StrictMode's rehearsal.
234+
//
235+
// React StrictMode mounts, unmounts and remounts every component in dev. The
236+
// model is held in `useState`, so the remount gets the same instance back —
237+
// and a plain `() => rowModel.dispose()` cleanup has already destroyed it.
238+
// The layout controller then marks itself disposed through its
239+
// model-subscription failure path and throws "A disposed row-layout
240+
// controller cannot change its columns" out of a layout effect, before a
241+
// single row paints. The hero has rendered nothing in `next dev` since #321.
242+
//
243+
// Deferring by a microtask lets the remount cancel the disposal, which is the
244+
// same shape `usePretable` uses for the models it owns. A real unmount has no
245+
// remount to cancel it, so the model is still disposed.
246+
const seededRef = useRef(false);
247+
const disposalRef = useRef<Set<typeof rowModel>>(new Set());
248+
useEffect(() => {
249+
const pending = disposalRef.current;
250+
pending.delete(rowModel);
251+
return () => {
252+
pending.add(rowModel);
253+
queueMicrotask(() => {
254+
if (!pending.delete(rowModel)) return;
255+
rowModel.dispose();
256+
});
257+
};
258+
}, [rowModel]);
223259

224260
const handleBeforeRowChange = useCallback(
225261
async (

‎apps/website/app/components/__tests__/HeroGrid.test.tsx‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
screen,
77
waitFor,
88
} from "@testing-library/react";
9+
import { StrictMode } from "react";
910
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
1011

1112
import { HeroGrid } from "../HeroGrid";
@@ -111,6 +112,32 @@ describe("HeroGrid", () => {
111112
expect(screen.getByText(/⌘V paste into\s+Qty/i)).toBeInTheDocument();
112113
});
113114

115+
it("survives a StrictMode remount", () => {
116+
// React StrictMode mounts, unmounts and remounts every component in dev,
117+
// and the model lives in `useState` — so the remount gets the same instance
118+
// back. A plain `() => rowModel.dispose()` cleanup therefore destroyed it
119+
// for good: the layout controller marked itself disposed through its
120+
// model-subscription failure path, and `setColumns` threw "A disposed
121+
// row-layout controller cannot change its columns" out of a layout effect.
122+
// The hero rendered NOTHING in `next dev` from #321 until this was fixed,
123+
// while every e2e run stayed green — they all measure production builds,
124+
// where StrictMode does not double-invoke.
125+
stubMatchMedia(true); // settled snapshot, no rAF needed
126+
expect(() =>
127+
render(
128+
<StrictMode>
129+
<ControlStateProvider>
130+
<HeroGrid />
131+
</ControlStateProvider>
132+
</StrictMode>,
133+
),
134+
).not.toThrow();
135+
expect(
136+
screen.getByRole("grid", { name: /live portfolio positions/i }),
137+
).toBeInTheDocument();
138+
expect(visibleRowIds().length).toBeGreaterThan(5);
139+
});
140+
114141
it("draws the book ranked by weight, largest first", () => {
115142
// The hero's stated default: largest positions first. It was lost once
116143
// already — the ranking moved into a local array that stopped being

0 commit comments

Comments
 (0)