Skip to content

Review 5434

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 2 revisions

PR

#5434 fix(visual-gate): freeze the clock at capture time, so date-driven stories stop reporting changed every day — by cixzhang (bucket: the maintainer)

HEAD REVIEWED

9d801ed1ccf4200c932781ef411a528790816c42 <- every claim below was verified at this commit; re-checked at the end and unmoved

Prior review: none — pulls/5434/reviews and pulls/5434/comments are both empty; the two issue comments are the Vercel and PR-Analysis bots. Nothing to extend or contradict.

PROBLEM

Whoever runs the release cut opens the gate report and finds lab-schedule--monthly in the changed list — every morning, and always would, because the story builds its events from new Date() and the captured day is never the baselined day. The cost is not the minutes spent tracing it: a gate that always says changed teaches the cut to skim the changed list, and skimming is how a real regression eventually gets promoted alongside a clock tick.

The brief's second exception applies too — a defect nobody has hit yet. core-calendar--default is in the same daily plan and renders the current month with today ringed; the baseline manifest's own decision log shows it was bootstrapped at 02:40 UTC today, so baseline and capture still share a UTC date and it starts churning the moment that rolls.

SOLUTION (1 decision · 15 runtime lines of 44)

  1. capture() and scout() run at a fixed instant, 13 May 2026 10:15 UTC,
     in a fixed timezone, UTC                                   [the fix]
       · the manifest records both — provenance for that decision, not a second one
       · docblock + README paragraph — 29 of the 44 added lines

Pinning the zone reads like a second decision and is not one: neither half produces determinism alone (an instant with no zone is a different wall-clock hour on every machine), and they cannot ship in either order. Measured — on a UTC host the zone pin moves 0 shots; on this Mac it moves 2.

Every decision traces to the stated problem; nothing piggybacked. The fix(visual-gate) type is honest — CI tooling only, no package change, so no changeset, and check-scope is green.

IMPACT

Nobody using Astryx. No package is touched — the diff is .github/scripts/ only — so nothing renders differently for an end user and nothing changes for a builder.

The person it reaches is the one reading a gate report at the cut. Tomorrow they see the date-driven stories in the changed list once; after they promote, they see a list that is empty unless something really moved. That is the whole change.

What landing this newly exposes: exactly four shots move on the first run — core-calendar--default and lab-schedule--monthly, light and dark. Not "every date-driven shot": the other date components in the daily plan (DateInput ×4, DateRangeInput, DateTimeInput, TimeInput ×4, Timestamp) render an empty closed field or a fixed value, and come out byte-identical either way. Those four are a deliberate rebaseline and the PR body says so plainly.

Reproduction: {review-root}/review-5434-evidence/run-three-captures.sh — three full 516-shot captures of the SAME Storybook build ({review-root}/worktrees/main/apps/storybook/dist, build-sha 07f6c334), the only variable being capture.mjs from this head versus origin/main; plus a fourth run under TZ=UTC to match the runner. Classified with {review-root}/probe-kit/visual-gate-classify-shots.mjs. Result: 516 total · 4 moved by the clock · 2 moved only by the timezone pin, and only on a non-UTC host · 0 unstable across two frozen passes.

API

no API change. Nothing in packages/ is touched, and .github/scripts/visual-gate/ exports nothing a consumer reaches.

USAGE

n/a — no API change.

THEME TARGETS

no new theme targets, and the freeze does not narrow what the gate can see. I ran the scout under both clocks and diffed the observations: the plan is identical — 516 shots, 197 stories, 0 added, 0 lost — and every observed target key and state matches. The only difference is the per-day date:2026-05-13 values on calendar-day, which no theme selects on and which churned daily before.

Reproduction: gate.mjs plan --json with a fresh --observations cache, run once from .github/scripts/visual-gate/ and once from a copy carrying origin/main's capture.mjs; set-diff of the shot keys, deep-diff of the two observation caches.

OSSIFICATION

n/a — no API and no theme targets. The instant is the durable choice here: one constant in one file, and changing it is the rebaseline the README now describes.

BREAKING

  • API — no. No published surface, no call site, no type.
  • Visual — yes, and only inside the gate: 4 baseline shots move once (Calendar, Schedule × light/dark). Nothing on any user's screen. Dispositioned in JUDGEMENT as finding 2.
  • Theme — no. Observed targets are identical under both clocks, so analyzeTargeting's "override bound to nothing" list cannot move.

Far side of the bound. The instant is the bound, so I drove it rather than sampling the middle: at 08:00Z nothing on the today cell is past yet, at 23:00Z everything is, and at the chosen 10:15Z the cell carries a past event, a future event and the today pill together. Read off the rendered frame, not off a transform string.

PERFORMANCE & RESOURCES

  • Gate runtime: no measurable cost. Three full 516-shot captures — 3m31s frozen, 3m55s frozen, 3m29s unfrozen. The clock is one init script per context, and there are two contexts.
  • Render / listeners / layout / bundle: n/a — no shipped code changes.
  • The one runtime question worth asking. setFixedTime freezes Date and leaves timers running, so any guard shaped "was X less than N ms ago" now sits permanently inside its own window. Six such sites, counted repo-wide rather than sampled: DropdownMenu.tsx:429, Popover.tsx:358, ComplexSelector.tsx:398, useMenuHover.ts:235, useMenuHover.ts:306, Toast.tsx:156. In principle a story with a play function could then photograph a state the product never reaches. Measured rather than reasoned: 516 shots captured twice under the frozen clock, 0 unstable and 0 differing for any non-date reason, over a plan that includes the 110 story files carrying play. No finding.

VISUAL EVIDENCE

Every frame captured in real Chromium and opened with the read tool. Durable copies, since the capture directories are in /tmp:

- Schedule at the frozen instant — the new baseline   after    {review-root}/review-5434-evidence/frames/after__schedule-monthly-light__frozen.png
- Schedule as CI holds it today                       before   {review-root}/review-5434-evidence/frames/before__schedule-monthly-light__ci-baseline.png
- Calendar at the frozen instant                      after    {review-root}/review-5434-evidence/frames/after__calendar-default-light__frozen.png
- Calendar as CI holds it today                       before   {review-root}/review-5434-evidence/frames/before__calendar-default-light__ci-baseline.png
- Timestamp, real CI baseline — reads 12:00 PM        control  {review-root}/review-5434-evidence/frames/control__timestamp-light__ci-baseline-utc.png
- Timestamp on this Mac, unfrozen — reads 5:00 AM     control  {review-root}/review-5434-evidence/frames/control__timestamp-light__this-mac-unfrozen.png

What the frames settle:

  1. The frozen Schedule is a real middle state, not an arbitrary epoch. May 2026, today pill on Wed 13, in the middle row of a six-row grid. On that cell 2:00 AM Company all hands is dimmed past, 4:00 AM Design review is coloured future, and two all-day chips sit above them. The author's claim, visible rather than argued.
  2. May 2026 renders nothing odd. Six-row grid, the same shape as the August baseline, so no height change; the 13th is mid-month and mid-week, and nowhere near a DST transition in any common zone (US ends 1 Nov, EU 25 Oct, AU ended 5 Apr).
  3. The timezone pin is a no-op on the runner, and that is checkable rather than assumed. The real CI baseline's Timestamp shot reads Mar 25, 2026, 12:00 PM for a fixed value of 2026-03-25T12:00:00Z — the runner is already UTC. The same shot on this Mac, unfrozen, reads 5:00 AM. That difference is the entire effect of the pin, and it is the effect the PR claims for it.
  4. Pre-existing, not this PR's. Four chips on one day overflow the day cell and overlap the date number in the row below — on the 13th in the frozen frame and equally on the 24th in the CI baseline. Schedule's, not the clock's, and not charged to this diff.

JUDGEMENT

approve — nothing blocks. Not posted as a review: the author is this account and the loop does not post on its own PR; the action taken is the merge. Risk class LOW on all four tests (no new API surface, no behaviour regression, no perf or resource cost, nothing got bigger) — with the caveat that the thing being changed is the release's safety net, which is why this review measured "does the gate still see everything" instead of arguing it. CI at this head: 18 checks, 15 pass, 3 skipped (pr-a11y, pr-rtl, Anchor review-clear chain — no components changed), none failing; mergeStateStatus CLEAN.

1. [not blocking] accept() does not carry frozenClock or timezoneId into the baseline
   → the next person who changes the instant sees several stories move with nothing
     in the baseline manifest to check the old instant against; the README's
     "recorded in every manifest" holds for the capture manifest and not for the
     promoted baseline                                        · lib/baseline.mjs:124-129
     ("manifest.viewport = currentManifest.viewport;" at :127 is the list it is missing
     from; settleMs is dropped the same way today, so this is the file's existing
     convention rather than a regression this PR introduces)

2. [not blocking] BREAKING · Visual — the 4 shots that move
   → deliberate, named in both the PR body and the README, and the promotion path
     (Visual Baseline: keys + reason) records the decision. Not worth an ask; recorded
     here so that approving is a choice about a known rebaseline rather than a
     discovery tomorrow morning.

Is this the fix, or a fix? The fix. The root cause is that the capture context was unpinned on the one axis every other determinism control already pins — network, animation, caret, viewport, Math.random. Looking for the next place the same failure can occur turns up a different shape rather than the same one: what is left unstable is self-animating content (the four excludeStories entries, plus the three stories the PR flags as still not reproducing), which no clock fixes.

Why not the alternative. Excluding the stories buys silence with the regression coverage of two core components and one lab component, and the exclusion list then grows every time someone writes another date-driven story — the config's own comment requires that list to stay reviewable rather than grow quietly. The freeze does the opposite of an exclusion: it turns three stories back into ordinary regression subjects, exactly as the seeded PRNG in the same file did for the chart stories. And it changes nothing about the gate's catch: same 516 shots, same 197 stories, same observed targets, with 4 of them comparable for the first time.

REVIEW

Nice — reproduced the two-day test, identical after, and exactly four shots rebaseline on the runner. accept drops frozenClock from the baseline manifest though.

INLINE (0-3)

  • .github/scripts/visual-gate/lib/baseline.mjs:127 — might want frozenClock and timezoneId on this list too.

EVIDENCE I DID NOT SPEND

  • A finding the second measurement killed. core-timestamp--default moved in my first sweep and is absent from the PR's table, which looked like a hole in the author's index sweep. Re-running the unfrozen arm under TZ=UTC, then reading the real CI baseline's own frame, showed it moves only on a non-UTC host — the runner already renders it at 12:00 PM. Found by measuring, killed by measuring a second way, and it never reached the review.
  • The freeze does not reach a Web Worker: one created inside the capture context read the real wall clock (2026-08-24T19:55:11Z against a frozen page clock). There are zero new Worker( / new SharedWorker( call sites in the repo — counted repo-wide and unbounded, not a truncated grep — so nothing rendered depends on it today. Worth knowing the day someone adds one.
  • The a11y audit's context (accessibility-audit.js:189) sets neither a clock nor a timezone, so date-driven stories stay date-driven there. It diffs violations rather than pixels, so it does not churn. Pre-existing, a different gate, not this PR's.
  • Drift sweep: no second copy of the instant anywhere in .github. The constant lives in one file and nothing else pins a clock or a zone, so there is no pair that can disagree later.

TIME

TIME  total 39m
  setup        4m   kit re-synced from the fork wiki (60c57d1), worktree at the PR head,
                    theme dists symlinked  (warm main reused: yes — its dist was the
                    Storybook for every arm, so no Storybook build at all)
  reading      9m   brief, critic rules, the gate's five lib files, the two stories
  measuring   13m   4 full 516-shot captures (frozen ×2, unfrozen, unfrozen under TZ=UTC),
                    2 scout passes, 4 single-story captures for the two-day test,
                    1 in-page clock probe.  Re-measures: 1, and it was the point — the
                    TZ=UTC rerun is what killed the Timestamp finding.
  writing     11m   draft, critic pass, rewrite, second critic pass
  waste       ~2m   symlinked main's node_modules into the worktree to borrow Playwright.
                    The harness forbids exactly this: vitest's global setup then ran a
                    `pnpm install` that would have deleted the warm donor's tree, and it
                    aborted only because there was no TTY. Removed; donor verified intact
                    (586 entries, Playwright resolvable, 6100 still serving). fast-install.py
                    was the right tool. Also reused a 23 Aug observations cache for the
                    first plan, before generating fresh ones for the scout comparison.

WHAT I COULD NOT VERIFY

  • The gate's own unit tests. The author reports 41 passing and CI's test job is green at this head, but I could not run them locally — the repo's vitest global setup wants a real install in the worktree and I did not have one.
  • The --tiers full row of the PR's sweep table (core-calendar--themed-selected-and-today-ring, the two DateRangeInput stories, core-powersearchwithtable--with-mixed-filters). I captured the daily plan only, so that row rests on the author's evidence rather than mine.
  • That the four moved shots reproduce identically on the ubuntu-arm64 runner. My captures are macOS, so pixels differ everywhere by construction. What I verified is the set of shots this change moves and the reason each one moves — not the bytes CI will produce tomorrow.

Clone this wiki locally