fix(Countdown): stop the ring rendering NaN and negative dash lengths, and make twenty-two snapshot cases assert something - #480
Merged
Conversation
#454's Tier 1, worked through the rule this branch adds to `PORTING.md` §7: on a ported component a vacuous test is a diagnosis before it is a repair. The seven cases had three different causes and only one was a test bug. **Countdown — ours, and the component was wrong.** `calculateTimeFraction` guarded `seconds < 0` but not `seconds === 0`, which is the prop's **default**. So `<B24Countdown use-circle />` — the minimal documented usage — computed `0 / 0` and rendered `stroke-dasharray="NaN 283"`, an invalid SVG value that was the only dasharray in either snapshot file: no snapshot has ever shown a valid ring. `seconds` is typed `number | string`, so any non-numeric string reached the same place. Both now return an empty ring, with four unit cases. **Empty — upstream has the prop, we removed it on purpose.** Upstream's Empty carries `avatar?: AvatarProps` and renders it through `UAvatar`; ours does not, by a divergence the ledger entry for `86cd25c5` states in as many words. The case passed the prop anyway, so it fell through to the root element and the snapshot pinned `avatar="[object Object]"`. Removed, citing the record — without checking upstream first, deleting it would have erased the only evidence of a dropped feature, had it been one. **The five `descriptionKey` cases — inherited, wrong in both trees.** They set the prop to the value it already has, on items with no such field. Checking upstream turned up its own spec doing exactly the same, so this is not a porting slip; it is a fixture that improves on theirs. Fixed the way `ContextMenu` and `CommandPalette` already do it: items that carry a description, read through a *different* key, which is what proves the key is looked up at all. That fifth outcome — upstream is wrong too — is now the fifth branch of the rule. Upstream is the control, not the authority: comparing answers where the mistake happened, and settles nothing about whether it is one. One case did not survive its own review. A `seconds: -1` unit case went in alongside the others and mutation showed it passes with the negative guard deleted — the formula reaches the same value unaided. Removed rather than kept: adding a vacuous test while fixing vacuous tests is the one outcome this issue cannot afford. The guard stays, now documented as decorative. Refs #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
The vue project's copies landed with the fixes; these are the same six files in the other project. `Countdown` is the one worth looking at — `NaN 283` becomes `283 283`, so both projects now record a ring that a browser can actually draw. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
…isfy the preconditions #454's Tier 2, minus the Table cluster. Seven cases, and checking upstream first changed the answer for one of them. Six were inherited. `Alert`'s `close` slot, `CommandPalette`'s `empty`, `Progress`'s `status`, `Tabs`'s `custom`, and `Stepper`'s `valueKey` all have byte-identical cases in upstream's own specs, passing a slot with nothing to render it into or a `defaultValue` that selects the step already active. So these are not porting slips; they are fixtures that improve on upstream's. Each now sets what its branch needs — `close: true`, a search term nothing matches, `status` with a value, a `defaultValue` naming the item that carries the slot, `'Shipping'` instead of index 0. `DescriptionList` is ours, and the answer was different. The first attempt passed `items` on the theory that the default slot renders per item; the snapshot did not move. `DescriptionListSlots` declares `legend`, `text`, `leading`, `label`, `description`, `actions`, `content-top`, `content`, `content-bottom` and `footer` — everything renders per item — and no `default` at all. Vue was dropping the slot silently and the case had been recording `with empty items`. Removed: unlike Empty's `avatar`, there is no upstream to appeal to and nowhere in the design for the slot to go. Snapshot duplicates across the corpus: 912 entries in 299 groups before this issue, 886 in 295 after Tier 1 and this. Refs #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
… 295 that exist The guard #454 asks for, built after measuring what it would have to hold. The issue names fourteen specs. Sweeping the corpus for its own symptom — an entry byte-identical to a sibling in the same file — found **883 entries in 295 groups**, a fifth of every snapshot here. Demanding uniqueness would have meant 883 red entries and the rule being deleted within the week, and it would have been wrong: most are not defects. A `renderEach` matrix produces variants cheaply, and a `color` that only tints a popup is genuinely indistinguishable while the popup is closed. The cheapness is the problem — nothing told you which entries were distinct from each other. So the guard pins what exists and fails on what is new. Three properties: - it parsed something before concluding anything, since a parser that stops matching turns "no new collisions" into "nothing compared"; - no group appears that the baseline does not list; - **no baseline entry has stopped being a collision.** That third one is what makes the list a record rather than a licence: fixing a case fails here until its line is removed, so the list can only shrink. `scripts/regen-indistinguishable-baseline.mjs` rewrites it, and says in its header what it is not for — making a red build green. The guard failing means a new case proves nothing; regenerating past it records that as acceptable. Also in this commit, the last two Tier 2 cases, both inherited from upstream's specs unchanged. `NavigationMenu`'s dynamic `custom` slot fires for an item carrying `slot: 'custom'` and no fixture had one — this was the only item-based spec with no `slot:` key at all. `ChatMessages`'s `files` slot needs a message part of `type: 'file'`; every fixture had text parts only. Its `viewport` case is removed rather than fixed. The slot sits inside `<Presence :present="showAutoScroll">`, driven by how far the user has scrolled — geometry happy-dom does not compute, so the branch is unreachable in this environment at any fixture. Same shape as the Table virtualization cases, and worth naming as its own category: not a fixture to fix, an environment that cannot reach the branch. 883 entries in 295 groups now, from 912 in 299 before this issue. Refs #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
The custom-slot case now has an item that selects it, in both projects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
#454's Table cluster. Five items, and not one of them was a fixture that needed a value — each was a branch nothing in the corpus could arrive at. **The actions menu never opened.** Every label in the column's `items` array appeared zero times across the snapshot file, so the whole array was dead while looking like coverage. `open: true, portal: false` on the `B24DropdownMenu`, as DropdownMenu's own spec does, takes that from 0 to 40 occurrences. **Sorting was unreachable.** `column.getIsSorted()` returned false in all 35 `renderEach` entries, so both branches of every sortable header were dead code. `sorting` is a `defineModel`, so two cases hand the state in directly. **And reaching them was not enough**, which the issue could not have seen from outside: the fixture gave `asc` and unsorted the *same* icon, so even a sorted case could only ever have distinguished `desc`. Three states, three icons now — verified by comparing the three snapshot bodies pairwise rather than by assuming. **The expanded slot** renders inside `v-if="row.getIsExpanded()"`; `expanded` is a `defineModel` too. **Virtualization plus row pinning** is removed. Under happy-dom the virtualiser sizes its window from scroll geometry the environment does not compute and renders a single row, so there is nothing to pin and the case was byte-identical to `with virtualize`. Same shape as ChatMessages' viewport slot: a branch this environment cannot reach, not a fixture to fix. `with row pinning` on its own asserts something and stays. Filed along the way: #479. `aria-sort` appears zero times in `Table.vue`, so sorting changes the header icon for a sighted user and tells a screen reader nothing. Upstream does not emit it either, so it is inherited rather than lost in the port. It surfaced because I measured `aria-sort` to check the sorted branch had been reached, got zero, and found the branch *was* reached and the metric was wrong. Refs #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
Review of #480 found the previous commit's `Countdown` note wrong on both halves, and a second invalid dash length next to the one it fixed. The `total < 0` guard was called decorative on the strength of a single value. `-1` does reach 1 unaided, and so does every whole number, because `totalSeconds` floors and the fraction is `Math.floor(total) / total`. Fractions do not: without the guard `seconds="-0.5"` renders an empty ring and `seconds="-0.2"` renders `stroke-dasharray="-4245 283"`. The comment invited a future reader to delete a load-bearing line, and the one test that would have caught that had been removed for being vacuous — it was, at `-1`. The formula also overshoots at the end of a normal countdown. It subtracts a tick's worth of arc so the ring keeps step with the digits, so when `totalSeconds` reaches 0 it yields `-1 / total` and the final frame rendered `stroke-dasharray="-28 283"`. A dash length may not be negative; clamp to [0, 1]. Both are covered by cases that fail when their guard is removed, and by a shared `expectValidDashArray` that rejects NaN and negative lengths. Refs #454
…t it Three holes review found in the guard added by the previous commit. `readdirSync` is not recursive, so the guard read `test/components/__snapshots__` and nothing else. `test/components/content/__snapshots__` was outside it, with two unrecorded collisions in it — `ContentToc with title` and `ContentSearch with size md`. The walk now covers everything under `test/`, and the baseline grows by those two groups. The self-check counted distinct bodies, not entries, so a format change that lost every duplicate — a fifth of the corpus — would have passed it. It counts matches now. None of the guard's own properties were tested: deleting the stale half of the comparison, or collapsing the key to the filename, left the suite green. The comparison moves into `scripts/indistinguishable-snapshots.mjs`, shared with the regenerator so the two cannot drift, and a `describe` block exercises it against synthetic groups. `key` sorts the names itself rather than trusting its caller. Also reconciles the three different corpus figures that were in circulation. Measured at the merge base: 916 of 4361 entries in 301 groups. After this branch: 876 in 293. Refs #454
The previous commit removed `with viewport slot` on the grounds that the slot cannot render under happy-dom. It can. `showAutoScroll` is set by a `scroll` handler comparing `scrollHeight` against `scrollTop + clientHeight`, all three of which happy-dom reports as 0 — but stubbing `scrollHeight` and dispatching the event runs the component's own handler and opens the `<Presence>`. It is not a `renderEach` case, since that matrix cannot dispatch anything, so it becomes a test of its own that asserts the button is hidden first and shown after. Removing the threshold comparison fails it. Also shortens the five identical `descriptionKey` comments, which said the same seven lines in five files. Refs #454
This was referenced Aug 24, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Resolves #454
Follow-ups filed: #477 (Calendar prop leak), #479 (Table has no
aria-sort).Type of change
revert(Scope): ...)Description
#454 describes a spec case whose fixture never satisfies its precondition: the snapshot records the fallback, the case reads as coverage, and nothing fails when it stops being true. It lists fourteen.
Two things changed the shape of the work before any of them were touched.
Measuring first. The issue suggests a guard that groups snapshot entries by identical body. Building it first, as a measurement, found 916 of 4361 entries in 301 groups — a fifth of the corpus — against the fourteen named. So the fourteen are a hand-picked sample, and "fix them all" was never the task.
Checking upstream first, at the maintainer's instruction: our components we analyse ourselves; upstream's we check against upstream. That turned out to matter more than anything else here.
Twenty-two cases with one symptom and seven different causes
CountdownEmptyDescriptionListTableactionsTablesortingChatMessagesviewportThirteen of twenty-two would have been misdiagnosed as test bugs and "fixed" by adjusting fixtures. Including, most importantly, the one where the component is broken.
The component defect
Countdown's guard checkedseconds < 0but not=== 0— and0is the default. So<B24Countdown use-circle />, the minimal usage from the docs, computed0 / 0and renderedstroke-dasharray="NaN 283", an invalid SVG value that was the only dasharray in either snapshot file: no snapshot had ever rendered a valid ring.secondsis typednumber | string, so any non-numeric string reached the same place.Review then found two more things in the same eight lines, both of which the first attempt got wrong:
total < 0guard is load-bearing, not decorative as an earlier revision of this branch claimed. That claim rested on trying-1, which reaches 1 unaided — as does every whole number, sincetotalSecondsfloors and the fraction isMath.floor(total) / total. Fractions do not: without the guardseconds="-0.5"renders an empty ring andseconds="-0.2"rendersstroke-dasharray="-4245 283". The vacuous test had been deleted and a comment left inviting the next reader to delete the guard.totalSeconds === 0it yields-1 / totaland the last frame renderedstroke-dasharray="-28 283". Clamped to[0, 1].Every one of these has a case that fails when its guard is removed, verified by removing it.
Where upstream was the answer, and where it wasn't
Empty's['with avatar', …]looked like a dropped prop — upstream'sEmptydoes carryavatar?: AvatarPropsand renders it throughUAvatar. It is not: the ledger entry for86cd25c5says "b24ui'sEmptydiverges … noavatar". Removed, citing the record. Without checking, deleting the case would have erased the only evidence of a missing feature had it been one.DescriptionListis ours, so there was nobody to ask. First attempt passeditemson the theory the slot renders per item; the snapshot did not move.DescriptionListSlotsdeclares ten slots and nodefaultat all — Vue was dropping it silently.Thirteen others —
Alert'sclose,CommandPalette'sempty,Progress'sstatus,Tabs'scustom,ContextMenu's,NavigationMenu's,Stepper'svalueKey, the fivedescriptionKeycases,Table'sexpanded— have byte-identical cases in upstream's own specs. Not porting slips; fixtures that now improve on upstream's.The Table cluster, where reaching the branch was not enough
The actions menu never opened, so every label in the column's
itemsarray appeared zero times — the whole array was dead while looking like coverage.open: true, portal: falsetakes it to 40.Sorting was unreachable in all 35 entries.
sortingis adefineModel, so two cases hand the state in. And that alone would not have been enough: the fixture gaveascand unsorted the same icon, so even a sorted case could only ever have distinguisheddesc. Three icons now, verified by comparing the three snapshot bodies pairwise rather than by assuming.Virtualization plus row pinning is removed: under happy-dom the virtualiser renders a single row, so there is nothing to pin.
One case that was wrongly declared unreachable
ChatMessages'viewportslot was removed on the grounds that happy-dom cannot open it. Review disagreed and was right.showAutoScrollcomes from ascrollhandler comparingscrollHeightagainstscrollTop + clientHeight; happy-dom reports all three as 0, but stubbingscrollHeightand dispatching the event runs the component's own handler. It is not arenderEachcase — that matrix cannot dispatch anything — so it is a test of its own, asserting hidden before and shown after.The guard
test/utils/indistinguishable-snapshots.spec.ts, overscripts/indistinguishable-snapshots.mjsshared with the regenerator (pnpm snapshots:baseline), with a 293-group baseline.It deliberately does not demand every entry be unique — that would be 876 red entries and the rule repealed within the week. Most collisions are not defects: a
renderEachmatrix is cheap, and acolorthat only tints a closed popup is genuinely indistinguishable. The cheapness is the problem; nothing told you which entries were distinct.So it pins what exists and fails on a new collision. Three properties:
The third is what makes the baseline a record rather than a licence: it can only shrink. 916 entries in 301 groups at the merge base, 876 in 293 now.
Review found three holes in the first version, all fixed here:
readdirSyncis not recursive, sotest/components/content/__snapshots__was outside the guard entirely — with two unrecorded collisions in it. The walk now covers everything undertest/.describeblock over synthetic groups; both mutations fail it.PORTING.md§7 gains the rule this work produced, because the next person will hit the same fork:
With the two edges it does not cover: a component absent from upstream is not automatically ours (§1 makes renames mandatory), and the procedure finds what a port dropped, never what it added.
Also found
Omitprops fall throughCalendar'somittedPropsand render asnextmonth="[object Object]".viewControlis in the list; its four siblings are not.aria-sortappears zero times inTable.vue. Sorting changes the icon for a sighted user and tells a screen reader nothing. Upstream does not emit it either. It surfaced because I usedaria-sortas a metric for "did the sorted branch render", got zero, and found the branch had rendered and the metric was wrong.Checklist
Gate run locally:
lint0 ·typecheck0 ·test314 files, 7168 passed, 6 skipped.