v0.67.0 — Test Health, part 1

Part 1: make `apps/web`'s suite real (findings 1–3)

Findings 1, 2 and 3 above, done. The remaining findings — the 131 fixed-window slices, apps/web's || true lint, the end-to-end suite that does not exist, and the coverage thresholds nobody checks — are untouched here and still want the spec the roadmap asks for. This part needed no spec: finding 1 states its own fix.

apps/web/package.json's test script has been echo "No tests yet" && exit 0 for the whole life of the app. Underneath it sit 803 files and 8,635 tests. CI runs pnpm test, which is pnpm -r run test, which runs that echo — so none of them have ever gated anything.

Run them and 106 fail. That number is the interesting part, because it is not a measure of how broken the app is. It is a measure of what a suite drifts into when nothing reads it: 60 are one Node 26 detail that no product code touches, 38 are guards whose assertions still describe code that was deliberately refactored releases ago, 2 are a cross-repo contract that cannot run outside a developer's machine, and only 6 are the suite doing its job — four of them flagging three real defects, and two guards mis-detecting their own subject.

The bulk of this repo's web tests are source-text guards: they read a .tsx off disk and assert a regex against it. That style is why the suite is so large (it needs no DOM) and it is also why it rots — a guard that pins useCharacters() fails when the call gains an argument, whether or not the thing it was guarding still holds. Every stale assertion below was re-pointed at the contract it was written to protect, not simply loosened until it passed; where a refactor moved the contract somewhere better, the assertion moved with it. Five of finding 5's fixed-window slices are bounded properly as a side effect; the other ~126 are not.

The last task turns the script on, so this cannot happen again.

Two of the findings were misdiagnosed, and the corrections matter to the spec

Finding 2 blames the missing jsdom override, and that is not it. Those files already carry @vitest-environment jsdom docblocks and always did. Node 26 defines globalThis.localStorage as an accessor that warns once and returns undefined unless the process was started with --localstorage-file — and vitest's jsdom environment SKIPS any window key the global already defines, so jsdom's real Storage never lands. Adding a jsdom override would have fixed nothing. sessionStorage, which Node does not define, worked throughout. It is still one file's worth of fix, as finding 2 says; it is a different file.

Finding 3 lists styleGuide.actionIconTooltips among the guards that are working, and it is the opposite. Its one offender is labeled twice over — wrapped in a <Tooltip> AND carrying an aria-label — and the guard sees neither, because the props arrive through a spread and the Tooltip sits past its 220-character look-behind. zIndexLiterals and noUppercaseTransform are exactly as finding 3 describes them.

Give the suite somewhere to run

[x] 1. Add apps/web/vitest.config.ts and a repo-root vitest.setup.ts. The app has no vitest config, so a bare vitest run there would load vite.config.ts, which throws without VITE_CLERK_PUBLISHABLE_KEY and pulls in the hosted-modules plugin — neither of which CI's test step has. The setup file installs an in-memory localStorage. (Done. 60 of the 106 failures were this one thing, and none of them were about the app. Node 26 defines localStorage and then refuses to back it. Without --localstorage-file the global still EXISTS, as an accessor that warns once and returns undefined — and "exists but undefined" is the worst of the three possible states, because it defeats every guard written for the other two. zustand's persist reads the bare global and dies on storage.setItem, at module-eval time, so initiativeStore, layoutStore and drawAction failed whole-file rather than test-by-test.* The @vitest-environment jsdom files failed for the same reason wearing a disguise: vitest's jsdom environment copies window properties onto the global but SKIPS any key the global already has, so Node's dud shadowed jsdom's real Storage and never got replaced. sessionStorage — which Node does not define — worked fine throughout, which is why the failures looked arbitrary rather than systemic. The config is not optional and not merely convenient. Vitest takes the first of vitest.config.* / vite.config.* and does not merge them, so without one pnpm test in apps/web would load the Vite config — which throws unless VITE_CLERK_PUBLISHABLE_KEY is set and installs the hosted-modules plugin. CI's test step runs before either exists, so the suite would have failed on config load rather than on a test. The setup file is wired into the root config too, so a repo-wide vitest run from the root behaves identically to pnpm test.* One assertion underneath was genuinely stale rather than blocked, and only became visible once the file could run: characterUIPrefs' "fills missing keys with defaults (forward-compat for added prefs)" re-listed the key set as a literal, so the one test whose subject is that adding a pref is safe was the one test adding a pref broke. It now states itself against DEFAULT_CHARACTER_UI_PREFS.)

What the guards caught

[x] 2. PermissionsPanel hardcodes zIndex: 10000. The z-index ratchet (zIndexLiterals.guard) exists to stop exactly this; Z_INDEX.TOP is that number. (Done, and it is a one-line change with a two-release backstory. v0.39.0 swept every ad-hoc 9999 / 10000 / 100000 into Z_INDEX.TOP and left a ratcheting guard so the sweep could not be undone. v0.61.0 then added this panel with a raw 10000 — the guard fired, and nothing was listening. Same value, so nothing renders differently; what changes is that the next one gets caught.)
[x] 3. Seven surfaces re-introduced forced uppercase. Style Guide 2.0.0 retired text-transform: uppercase; theme/noUppercaseTransform has been failing since. (Done — six files, seven sites, all of them the same small dimmed section caption: the cross-layer pick menu's group header, the permissions divider, and five character-sheet subheadings. This one changes what users see, which is the point. Every label was already authored in its displayed case ("Who can use this character", "Class abilities", "Subclass: …"), so removing the transform does not leave anything mis-cased — it just stops the app shouting six captions the style guide decided it would not shout. There was no allowlist to add them to, deliberately: the guard was written as a flat prohibition because a caption is exactly the kind of thing that gets an exemption once and then always.)*
[x] 4. Grid auto-detect has never worked on a real image. The spacing estimator's score shrinks with input length, so it returns null for anything wider than about 150px and "Detect Grid Size" always throws. (Done. This is the one item here that a user could have reported as a bug, and the test that would have caught it shipped in the same commit as the defect. Two normalisations, applied one after the other. centerAndNormalize divided the whole series by its total energy, so every element already scaled as 1/sqrt(N) — and then the correlation sum was divided by the overlap length again. The resulting score falls away roughly as 1/N while the accept threshold stayed a fixed 0.01, which made the detector's answer a function of image size rather than of image content: correct at ~64 samples, a harmonic at ~100, null from ~150 up. Analysis runs at up to 1200px, so every real map took the null branch and autoDetectGridSizeFromImageUrl threw "No clear grid lines detected in image" every time.* It is now a per-lag Pearson correlation over the overlapping window — in [-1, 1], meaning the same thing at any length, which is what makes a fixed threshold legitimate. A 1200×900 map with a 50px grid drawn over textured content now comes back as 50px. Two things the old code could not have got right and the new code has to. A grid at 20px correlates with itself at 40 and 60, so highest-score-wins reports a grid two or three times too large; the fundamental is now preferred over any harmonic it is a whole multiple of. And confidence is the peak's height above the median score rather than the peak alone, because a smooth signal correlates well with itself at every lag and would otherwise read as very confident.* detectGridFromImageData now also requires BOTH axes to find a period and to agree within 20%. One axis is stripes, not a grid — and an 8-bit gradient has a genuinely periodic derivative, so without this it reported a confident 160px grid for a picture with no grid in it at all. Its flat other axis is the tell.)

Guards that were wrong about their own subject

[x] 5. Two false positives. The unlabeled-ActionIcon ratchet cannot see a label that arrives through a props spread, and the seedSystems prefix guard now matches a different loop that happens to use the same variable name. (Done. Both are the same failure mode in different clothes: a text scan whose window is the wrong size — too small in one case, the whole file in the other. SheetHeader's build toggle is labeled twice over — wrapped in a <Tooltip> AND carrying an aria-label — and the guard could see neither, because the toggle builds one shared props object and spreads it into either a <Button> or an <ActionIcon> depending on the surface. The Tooltip's opening tag is past the 220-character look-behind and the aria-label is inside the spread. Widening the window would have fixed this one site and gone on needing widening, so the guard now resolves {...name} to its declaration and reads the object literal. Verified the other way too: an ActionIcon spreading a label-less object is still reported. The seedSystems one was one variable name away from being a real regression, which is the part worth noticing. It asserts conditions are not namespaced by looking for `${systemId}_${c.id` in the whole file — and a currencies loop was added later whose item is also c and which is supposed to be namespaced. Conditions are still unprefixed; the assertion now reads only the conditions block, and re-namespacing them still fails it.)*

Assertions that outlived their code

[x] 6. Artwork, portraits and token rendering. Nine assertions across six files, three root causes. (Done. Every one of these describes a refactor that made the code better and left a guard describing the old shape. artworkType filtering was retired in favour of ID lookups — getPortraitArt / getPrimaryTokenArt replaced artworks.find(a => a.artworkType === 'portrait') everywhere, and the artwork-type dropdown went with it. Four assertions across ObjectLayer, ObjectLayerArtworkEditor and TokenHoverCard were still looking for the string comparisons. Object artwork is now covered by the generic first-artwork fallback rather than by name, and the cutout preference — the part that actually matters — is unchanged, so they assert that instead. The editor's test now pins what the dropdown was there to set: artwork created on the objects layer is typed object without asking.* The portrait-crop contract grew a legitimate exception. SlotPreview no longer renders <PortraitImage> because it became the crop EDITOR — it holds a live draft the user drags and wheels, which a render-only component cannot. Three assertions failed on that, and the fix is not to drop the site: it gets its own case asserting the thing the contract is really about, that it takes the artwork and styles it with the same cropImageStyle / naturalCropImageStyle, so the preview and the result cannot diverge. The third failure was cosmetic — the import list grew to four names and reformatted, so a regex over one import line missed it.* Two GeneralTab guards. resolveTokenRenderDefaults gained a second argument, so a regex expecting }) to be followed immediately by .facingLogic stopped matching code that still asks the resolver exactly as intended. And the faction ring's === false rule moved into factionRing() when the ring stopped taking its geometry from the art's border width — where utils/faction.test.ts already tests it properly, which is a better home than a regex over a 4,000-line component. The layer's guard now pins what the layer is still responsible for.)*
[x] 7. Chat, layout and navigation. Seventeen assertions across thirteen files. (Done. Three of these are worth reading; the rest are the same story in miniature. One pair of tests had been asserting opposite things about the same file. ConnectionStatusDot.test.ts requires ContextPanel to render the dot; v0.20.0's chat-header uncrowding removed it and left ContextPanelChatHeader.test.ts asserting it is NOT there. Both have sat in the repo ever since, only one of them ever run, and neither could have been satisfied at the same time as the other. The dot lives in the global nav now, where it is the popover trigger for connected players and session tools rather than a decoration beside a gear, and the test says so.* A dependency array whose absence is the fix. Two files pinned the character-context effect's deps as ending setContextCollapsed, leftContexts]. leftContexts was deliberately removed: depending on it re-ran the effect on every left-panel change and re-asserted the impersonated token's sheet, so opening another character from the global nav flashed and snapped back. Every read inside the effect goes through getState() for that reason. Re-pointing the assertion at the current list would have made putting it back a silent pass, so there is now a negative case as well.* A fixed-size window, widened twice, drifted a third time. The same file slices 2,000 characters from a marker comment (it was 1,200 until v0.31.0) and by v0.61.0 the deps had drifted past that too — failing an assertion about code that had not changed. It now reads to ]);, which is where the effect actually ends.* The remainder: the initiative tab's early return gained || chatTab === 'combat' when v0.37.0 added the combat log (three files); PLAY_TOPBAR_HEIGHT moved into a leaf module to break a circular import; the mobile chat-view list became the store's CHAT_VIEW_ORDER; the tool palette moved to top: 52px when the PlayTopBar was retired and its z-index: 400 became var(--z-floating-window) in v0.39.0's sweep; useCharacters() gained a campaign scope; isBackground became an inline check; freshImpersonatedToken was deleted as unused in v0.29.2 — its freshness contract survives on the selected token, which is what the bar renders now; and the property increment prompt is a portaled Modal rather than a Popover, which prevents the parent-collapse bug the test names by a better mechanism than the one it was checking for.)
[x] 8. Actions, monsters and the long tail. Twelve assertions across eleven files. (Done. One of these was not stale but EXPIRED, which is a different thing and got a different treatment. v0.20.0's "generated action code must await roll()" rule no longer exists. roll returned a Promise then, and an un-awaited one printed as [object Promise] in chat and reached applyDamage as a Promise. It is synchronous now — rollSimple returns number — and the editor's own completion entry says "SYNCHRONOUS … await is optional" and omits the keyword from its example. The guard was failing on a snippet that is correct, for a rule that was repealed. Re-pointing it at the current text would have meant asserting the opposite of its own name, so it is rewritten as the fact that replaced it (roll returns a number, and the docs say so) and moved to utils/actionRollSync.test.ts. A file called InsertMonster.awaitRoll asserting that awaiting is unnecessary is most of how these go stale in the first place.* Three more fixed-size windows. AddBar sliced 3,000 characters from a heading to look for a try { that had drifted forty lines past the cut; useTokens.queryGate sliced 1,200 from useQuery({ to look for enabled: !!mapId, which is the last key and now sits behind a longer comment. Both are anchored on the call they are about instead. The third was a blanket negative rather than a window: DrawingTransparentFill banned format="hex" from the whole toolbar, and v0.42.0's in-shape text added a FONT colour input which is correctly hex — text has no transparent fill to express. It now checks the two inputs that need alpha.* AddBar's other assertion is the one that would have mattered most: the disabled check it pinned, !newBar.name || !newBar.valueFunction, would today disable the button for a complete property-backed bar. The check became bar-type aware and the test had not. The rest: Generate Map and Generate Monster both moved out of their old menus into CreateTokenChooser, which is now the single "add something to the map" entry point; the 5e saving-throw gate became usesFiveEMonsters() covering both editions rather than a bare === 'dnd5e-2014'; brush hit-testing moved into utils/drawingHitTest.ts, where it is unit-tested rather than regexed; async-action toasts route through notify() since v0.62.0; the characters list response gained a campaign-scoped narrowing between the filter and res.json; and the generated attack message says "Damage →" rather than "Damage:", which was never the subject — the dice notation is.)
[x] 9. Make the cross-repo module contract tests skip when the sibling repos are absent. They read vtt-modules and system5 from beside this checkout. CI has neither, and would fail on them forever. (Done, following the pattern ui-components' FeatPowerModalFilters.test.ts already uses to reach into vtt-modules: check existence up front, it.skip when missing, and read the files lazily INSIDE each test — it.skip still runs the body during collection, so a top-level read would ENOENT before the skip took effect. Skipping rather than deleting, because the contract is real: this is the platform's half of an agreement with two other repos, and nothing on this side would notice if their half moved. It just cannot be a CI gate from here — and, less obviously, cannot run from a git worktree either, whose siblings are other worktrees. Two tests, both green on a developer's machine and skipped everywhere else.)

Turn it on

[x] 10. apps/web's test script becomes vitest run. (Done. pnpm test now runs apps/web on top of everything else and exits 0 — 804 files and 8,640 tests by the end of this part, up from the 803/8,635 it found, the difference being tasks 11 and 12's own guards. Roughly 20 seconds wall-clock: the suite is almost entirely pure functions and file reads, and the two cross-repo tests skip. Turning it on immediately found a bug in task 1, which is the whole argument for turning it on. setupFiles is resolved against each project's root, not against the config file that declares it — and packages/create-purple-cat and scripts/setup have no config of their own, so they inherit the root one while rooted in their own directories. './vitest.setup.ts' resolved inside THEM, and both suites failed to collect. Neither is something anyone runs by hand; the only reason it surfaced at all is that pnpm test is now something whose exit code matters. Both configs pass an absolute path now, and task 1's commit carries the fix.)*

What turning it on immediately caught

Both of these are v0.66.0's, both landed on main, and neither was visible to anyone. That is not a complaint about v0.66.0 — it is the entire argument of this release, arriving on schedule.

[x] 11. Close the z-index ratchet's numeric-separator hole. zIndexLiterals.guard matches zIndex(?::\s*|=\{)(-?\d+)\b, and _ is a word character — so \d+\b cannot match 10_000 at all. v0.66.0's LibraryReviewRow wrote exactly that and the guard reported nothing. (Done. The pattern takes [\d_]* and strips separators before parseInt, and the literal becomes Z_INDEX.TOP — the same value, and the same fix task 2 made for PermissionsPanel. A guard that exempts one spelling is worse than no guard, because the exemption is invisible: the file reads as "no hardcoded z-indexes anywhere" and was quietly false. The guard now has a case asserting it can see the separator form, so the hole cannot reopen silently — that case is the part worth keeping, more than the one-character regex change.)*
[x] 12. LibraryReviewRow hand-rolls a save on blur. asyncFieldSweep.contract is v0.51.0's ratchet over the app-wide field sweep — its holdout list may shrink and must never grow. v0.66.0's review queue grew it. (Done, by building the wrapper that was missing rather than by widening the list. The guard's own docblock says why the hand-rolled version is not merely duplicated code, and this one proved it. save() was setSaving(true) / try / finally — with no catch. A correction the server rejected set saving back to false, left the row displaying the value it had failed to store, and surfaced as an unhandled rejection in the console. For a queue whose entire purpose is recording corrections, silently discarding one is the worst available outcome: the user cannot tell it from success.* There was no AsyncTagsInput to adopt — the family had TextInput, NumberInput, Textarea and Select — so it is added, modelled on AsyncSelect. A TagsInput belongs in the commit-on-change half of that split, not the commit-on-blur half: its onChange fires when a tag is FINISHED (Enter, comma, the ✕), so each change is already a deliberate act, and a blur commit adds nothing but a way to lose the last tag by clicking elsewhere. The "each save is a round trip" worry the old comment names is real and is what debounceMs is for; the row passes 600ms. One thing an array wrapper needs that the other four never did. useAsyncFieldResync compares with Object.is, which for an array is identity — so the natural initialValue={item.tags ?? []} hands it a fresh array every render, the resync fires, reset re-renders, and the component loops forever. Stabilising the array inside the wrapper (element-wise equal keeps the previous identity) means callers get to write the obvious thing; a useMemo at each call site would be a rule to remember, and this is a footgun that costs a hung tab.* The Type select beside it went through the same save() and gets AsyncSelect for the same reason. disabled={saving} is gone with them — each field owns its own in-flight state instead of freezing its neighbour, which is what let one slow tag save block a type correction.)