Review 5548
imdreamrunner · open · view on GitHub
Verdict: approve (drafted; no public PR action)
Versions. Review Loop 1.4.0 · Component Audit Rubric 1.12. Both were pinned at gate 1 and held through two critic passes.
Head reviewed. 3acbde9441de1e4214f0662e2093bd18d0b5b1c3 — every claim below is against this exact commit. Merge-base: c78ac8f46bcbe8418baa5b390297393dd2dad733.
Post-review head movement. GitHub advanced the PR to 1bd9da727745cf014cfd44df19b81f081dcc39bc after this review and its cleanup completed. That newer head was not reviewed; this verdict remains pinned to 3acbde9441de1e4214f0662e2093bd18d0b5b1c3.
Lane. Full. The diff has three runtime decisions, two effects with popstate listeners, SSR architecture, intentional first-paint changes, responsive layout, and a performance claim.
Safety. The five changed paths are docsite source and a test. No dependency, lockfile, workflow, package script, executable tool, credential, environment, or build-network change appears. The exact fork head matched the requested SHA before any install or build.
Description and test plan. The body names the person-visible failure, three causes, intended trade-off, and a broad production-build interaction plan. It is unusually strong. One precision limit remains: the absolute 0.2281 / 0.2061 values do not name viewport or cache state, and independent Chromium runs confirmed the direction but not those exact magnitudes.
A person opening a component or theme reference saw chrome and a footer around an empty article, then watched the article arrive and move the page. On a phone the server-rendered footer also used its desktop grid until hydration, squeezing and overlapping its three regions.
VERDICT: clear
The server now sends a complete default document instead of waiting for query-string state. After hydration, each page adopts a requested tab or theme and listens for browser history changes; user choices still write the URL. The footer keeps one semantic tree and lets CSS choose the mobile or desktop layout, so the first paint already has the correct geometry.
- Component tabs start on Overview, adopt
?tab=after hydration, preserve unrelated params, and followpopstate— source. -
/themesstatically seeds Neutral, while the client adopts?theme=and history changes — page · client. -
SiteFooterrenders one tree and swaps grid/flex geometry in CSS at 768px — source.
The 99-line source guard is evidence for these decisions, not a fourth runtime decision. All three trace to the stated first-paint problem; the “over two” split smell does not fire.
VERDICT: clear
OWNER: component/theme page owns its selected view; SiteFooter owns its responsive layout
TIER 1: Next static shell + App Router URL output; CSS media query for responsive geometry
TIER 2: none
SEAMS: prerender → hydration; direct query deep link; click/replace; popstate; docs/site/blog/not-found footer hosts
BEHAVIOR UNIT: inline — each URL sync is small and inseparable from its page state
The two effects synchronize with browser-owned URL/history state, clean up their listeners, and perform no layout read, focus, announcement, or analytics. Identical-value sets on bare URLs bail out; only deep links render a different selected view. CSS owns the footer layout because JavaScript cannot know viewport width during prerender.
VERDICT: clear
Every component reference and the shared /themes page now has useful article content in the first static frame. The footer fix reaches every docs, marketing, blog, and not-found page; mobile readers get a centered stack immediately, while desktop geometry remains unchanged.
A query deep link intentionally shows the static default until hydration, then adopts Properties or the requested theme. Direct interaction, unrelated-query preservation, URL output, and popstate all held at the exact head.
VERDICT: clear
No published package API, docsite component prop, export, route pathname, or metadata helper changed. The new TabName type and URL readers are internal.
Nothing new ossifies as consumer surface.
VERDICT: clear
No target, token, CSS variable, variant, or theme override changed. The footer uses existing semantic CSS variables and StyleX-supported media queries.
VERDICT: clear
- API — no.
- Visual — yes, intentionally: the first frame gains the article, and the mobile footer changes from a collapsed desktop grid to its stacked design. The desktop footer is unchanged.
- Theme — no; all existing targets and token resolution remain intact.
- Behavior — query deep links settle from the static default after hydration rather than from a blank hole. The selected destination and URL behavior are preserved.
VERDICT: clear — intended visual and loading behavior only
Effects. Two mount/history effects were added. Each reads window.location, sets one primitive state value, attaches one popstate listener, and removes it on cleanup. No layout effect, observer, timer, forced layout read, or work growing with page content was added.
Response trade-off. DateInput HTML grew from 244,303B to 251,628B gzip (+7,325B), because the article is now in the static shell. Themes fell 52,918B → 52,334B gzip and changed from private, no-store to one-day shared caching. Route JavaScript was effectively flat: DateInput +72B gzip, themes +170B gzip.
This is the feature's necessary transfer: useful initial HTML replaces a later hole without moving equivalent work into a growing hot path.
VERDICT: clear — bounded HTML increase, no meaningful JavaScript or runtime regression
VISUAL CHECK: manual frames required
WHY: initial pixels and footer layout intentionally change; the exact-head visual job was skipped.
All frames use Chromium, the same canary build target, scripts blocked for the static-shell frame, and canonical sensor receipts. Before/after receipts match on every required sensor except Build. The exact reviewed arm is banked here (SHA-256 d6e4e7f76491a878eb5cc75f6eb27dc096438c61976662650cb1705a4193734c). Contact sheet.
| Case | Merge-base | Exact head | Observed change |
|---|---|---|---|
| Component, 390×844 |  |
 |
413 → 2,905 visible text chars; 1,076 → 4,975px document; DateInput + Usage absent → present |
| Themes, 390×844 |  |
 |
413 → 3,131 chars; 1,076 → 6,830px; Neutral explorer absent → present |
| Footer, 390×844 |  |
 |
342px grid collapsed to 95.4/4.9/4.9/4.9/232px tracks → centered flex stack with 320px link measure |
| Footer, 1440×900 control |  |
 |
identical 1180×233 outer box; 225/225/225/225/232 top tracks; 675px links; 232px social; 283×4 legal tracks |
Receipts: component before · component after · themes before · themes after · mobile footer before · after · desktop before · after.
The author’s 740×700 comparison implies two 370×700 arms, so the independent reproduction used that viewport, 1.5Mbps down / 0.75Mbps up, 40ms latency, 6× CPU, and both cold and exact-route-primed caches. It waited for semantic hydration state rather than a fixed delay. Re-run with the banked probe:
ASTRYX_ROOT=/path/to/astryx ARM=head PORT=62454 OUT=. \
node assets/pr-5548/cls-warm.cjsRoute-assets-primed inclusive layout-shift session window:
| Route | Merge-base | Head |
|---|---|---|
/components/DateInput |
0.600000 | 0.002443 |
/components/DateInput?tab=properties |
0.600000 | 0 |
/themes |
0.472867 | 0.006629 |
/themes?theme=butter |
0.556622 | 0.006629 |
Chromium labelled these automated navigation-window shifts hadRecentInput; standards CLS excludes those entries and was 0 in all four head cells. Cold 390×844 runs independently measured head at 0.000247 (component) and 0.000299 (themes); desktop measured 0.000197 and 0.001155. The direction, static-shell cause, and effectively-zero head are reproduced. The body’s exact 0.2281 / 0.2061 magnitudes remain setup-sensitive and were not independently reproduced as those numbers.
Full machine-readable evidence: measurement summary · base bundles · head bundles.
VERDICT: clear — both required page endpoints and both footer widths are independently evidenced
Both arms emit the same title, description, canonical, OpenGraph contract, and cache revalidation period. Component canonicals remain /components/<name>; every ?tab= state shares it. Themes remains canonical /themes; ?theme= is a client-selected view, not a separately indexable page. Moving query selection client-side therefore loses no canonical page content.
The head’s static initial HTML contains the default component/theme content; scripts-disabled frames prove it without depending on hydration.
VERDICT: clear
No string, role, accessible name, focus path, key handling, directional icon, locale data, or translation key changed. The footer retains one contentinfo landmark and the same Astryx Link/Button controls; only their responsive placement changes.
The page-state effects move neither focus nor announcements. Three fresh component hydrations and the driven component/theme flows produced no hydration or page errors; the only local 404s were the expected absent Vercel analytics endpoints and are excluded from the receipts.
VERDICT: clear
- Step-0 safety: clean; exact head verified before dependency/build execution.
-
static-shell.test.ts: 1 file, 2 tests passed. - Docsite typecheck: passed after the fresh worktree generated registries and built package declarations.
- Workspace package build: passed.
- Exact-head docsite production build: passed;
/themeschanged from partial prerender at the merge-base to static at the head. - Merge-base docsite production build: passed with the same canary target.
-
git diff --check: passed. - Exact-head CI: build, lint, test, docsite-test, dependency and scope checks green; visual/a11y/RTL jobs skipped by path filter.
- Design owner approval exists on the exact head.
- Component navigation evidence: JSON. Theme selection/history evidence: JSON.
| Slot | Verdict |
|---|---|
| Problem | clear |
| Solution | clear — three decisions, one stated problem |
| Architecture | clear |
| Impact | clear |
| API | clear — no API change |
| Theme targets | clear — none |
| Breaking | clear — intended first-paint/mobile visual change |
| Performance | clear — bounded HTML cost, bundle flat |
| Visual | clear — exact-arm manual frames and receipts |
| A11y & i18n | clear |
GOAL: met. The scripts-blocked initial DOM contains complete component and theme content at the head, both deep-link flows settle correctly, and the large measured geometry shift collapses to effectively zero.
DISPOSITION. No blocking finding. The exact absolute CLS values are under-specified by viewport/cache and remain unreproduced; accepted as an evidence-precision note because two independent arm measurements prove the mechanism and outcome, and it does not change code correctness.
ADVICE: omitted. No code remedy is required. The nearest nits are a stale themes-page comment, a narrow media-query guard, and a TabName cast; none has a user-facing consequence in this change.
AUTHOR CAN PROCEED: yes for the reviewed head. 3acbde9441de1e4214f0662e2093bd18d0b5b1c3 can land after the normal repository gate. The newer 1bd9da727745cf014cfd44df19b81f081dcc39bc head is outside this review and needs fresh validation before this verdict is applied to it.
WORST OUTCOME: none found. The remaining 29px→35px mobile theme-heading handoff measured 0 standards CLS and 0.006629 inclusive; deep links were no worse than bare routes.
JUDGEMENT NEEDED: none — defect fix against the existing loading and responsive-layout contracts; the design owner has approved this head.
Pass 1 requested changes on three claims: the mobile Themes heading, unmeasured query deep links, and insufficient footer evidence. The exact-arm matrix refuted the first two and the receipt-backed footer pairs closed the third. Pass 2 withdrew all three blocks and the split concern, and returned PASS — approve is correct. It required the teammate full-review link and the footer frames in the public draft; both are included below.
No blocking findings.
| Footer | Before | After |
|---|---|---|
| 390×844 initial HTML |  |
 |
| 1440×900 desktop control |  |
 |
Thanks, this is good. The static shell now contains both pages, the deep-link flows hold, and the mobile footer fixes without moving desktop geometry.
[Reviewed by Robohands]
apps/docsite/src/app/(site)/themes/page.tsx:12 — This page no longer reads the query itself; ThemePackagePage does.
- The source allowlist catches the current direct named imports but not default/namespace imports; no current importer evades it.
-
TabNameis declared but the state remainsstringand is narrowed with a cast. - The mobile theme selector’s synthetic 390×844 click was intercepted by its existing floating toolbar after Playwright scrolled it; state/URL output was verified through the existing option event and this is not caused by the diff.
Nothing was posted. This review stayed read-only by request. Critic pass 2 added the required full-review link and footer frames to the draft; its first-pass request-changes verdict was withdrawn after measurement.
TIME total 101m
setup 15m exact worktree, fresh wiki clones, safe install, package outputs
reading 23m kit, rubric, instructions, description/test plan, diff, architecture
measuring 48m two production arms, 8 frames, receipts, CLS/cache matrix, interactions
writing 9m R16 presentation, public draft, wiki record
waste 24m network-idle hang, one occupied port, and two probe rewrites/rebuilds
warm main reused: no — exact merge-base/head were built in one isolated worktree
re-measures: 3 — fixed static-script receipt execution, semantic hydration readiness, cache state
- The author’s exact
0.2281and0.2061magnitudes, because viewport and cache state were not named and Chromium’shadRecentInputclassification changed the standards result. The outcome and near-zero head were verified independently. - No second browser engine was run; the fix depends on Next prerendering and CSS media queries, not an engine-specific API.
Current title: fix(docsite): stabilize PPR loading states
Head reviewed: 1bd9da727745cf014cfd44df19b81f081dcc39bc
Current PR base: 824a181dba0ba57c89a52e84f818a5d1f096e930
Post-review head movement. After all checks below completed, GitHub advanced the PR to 6bfd9a8a67c3361878e73033f400bffea4ef450d. That newer head is not reviewed; every Round 2 verdict and measurement remains pinned to 1bd9da727745cf014cfd44df19b81f081dcc39bc.
Verdict: approve with one non-blocking tall-viewport nit (drafted; no public action)
LOOP VERSION: 1.5.0
AUDIT RUBRIC: 1.12
The original Round 1 gate, version, evidence, and wording above are preserved unchanged. This is a fresh correction under the current loop because the branch was force-rebased and replaced: 3acbde9 is not an ancestor of 1bd9da7.
LANE: full. The replacement changes SSR/PPR ownership and the intended loading pixels on two routes; one check cannot settle it.
Step 0 passed before install/build. The current PR patch changes four docsite source/test files only. No package manifest, lockfile, workflow, lifecycle script, executable tool, dependency, credential, environment read, or build-network path changed.
The apparent large 3acbde9..1bd9da7 history is the rebase, not this PR. The review compared the old patch (c78ac8..3acbde9) with the replacement patch (824a181..1bd9da7). Current patch SHA-256: 0a9161d9efeb409f054e96d9bee0db3f071f1c76384323ad83e5d9fe0b900bec. Revision content delta SHA-256: f83e25adc663cdc616236ff8ceeb713f93ed7db69bd9afd6d94ae49d6f23413e.
The current patch is one commit, four files, +341/−123. Relative to the previously reviewed file contents it is four files, +333/−251:
- component pages restore
useSearchParams, keep the title/package line static, and put only tabs/panel behind a named Skeleton fallback; - themes restore server
searchParamsresolution and add a responsive Skeleton fallback; - the two client URL-sync Effects/listeners introduced by
3acbde9are removed; - the regression gate changes from a repo-wide hook allowlist to three source-shape assertions;
-
SiteFooter.tsxis byte-identical to3acbde9; because its Section padding dependency changed underneath it, the critic remeasured the exact deployed current head: the prior 1180×233 desktop geometry/tracks and 390×365 mobile stack still match; - metadata helpers, component route metadata, and the root layout are byte-identical to
3acbde9.
One base-only change reached ComponentDetailClient: Anatomy content and hook typedefs landed on main between revisions. It changes settled article size, so old page-height/bundle numbers were not reused; the exact current base was rebuilt and remeasured.
The current body accurately describes the replacement architecture, native deep-link preservation, named fallbacks, and test surface. Its remaining precision issue is the unqualified “CLS 0”: it holds to rounding on the measured mobile and 1440×900/1200 paths, but not once a CSS viewport is tall enough to expose the fixed-height fallback’s footer.
WHY 1: Empty PPR holes put the footer where the article will arrive, and mobile initially receives desktop footer geometry. WHY 2: When streamed content arrives, the reader’s page moves while they are orienting or reaching for content. WHY 3: A reference site exists for scanning and acting on documentation; losing visual position during load adds repeat work and makes the surface feel unreliable.
USER-FACING PROBLEM: A reader opening component or theme documentation sees an empty/overlapping shell and then loses visual position when the requested content arrives.
PROBLEM SEVERITY: harmful friction. The task remains completable, but the exact current base measured component CLS 0.589 mobile / 0.212 desktop and themes 0.135–0.138 desktop, well beyond the good band.
VERDICT: clear
The page keeps URL state in Next, where direct links and soft navigation already live. Static content above the query boundary renders immediately; an existing Skeleton pattern reserves the query-dependent region until its exact panel arrives. CSS continues to own footer responsiveness, so no viewport-dependent markup waits for hydration.
SOLUTION (2 changed runtime decisions in this revalidation)
1. replace the full component-page/static-default approach with a narrow query PPR boundary and matched fallback
2. replace client theme-query adoption with server query resolution and a responsive PPR fallback
BURDEN: medium. Two fixed-geometry fallbacks and two narrow Suspense boundaries add maintenance and visual-test burden, but the revision removes two state-sync Effects/listeners, adds no state or public surface, and uses existing Next/Skeleton/StyleX mechanisms.
BURDEN MATCH: proportionate. This addresses measured harmful friction, not polish, and the normal mobile/desktop shift falls by two to four orders of magnitude.
VERDICT: clear
OWNER: each route owns its query boundary; SiteFooter owns responsive layout
TIER 1: Next cacheComponents/Suspense + Astryx Skeleton + App Router query state
TIER 2: none
SEAMS: static shell, streamed boundary, direct query, router.replace, no-tab page, mobile/desktop/tall viewport
BEHAVIOR UNIT: framework boundary — no new Effect state machine; query behavior stays in existing Next APIs
| Seam | Driven result |
|---|---|
direct ?tab=properties
|
Properties selected with no default-tab handoff |
| click Overview / Properties | selected panel and URL agree; unrelated foo=keep survives |
direct ?theme=butter
|
Butter selected server-side |
| theme selection | Stone selection updates both state and URL |
| no-tab component |
useAppShellMobile title + Usage in scripts-blocked HTML; no fallback |
| PPR fallback accessibility | named status node in the AX tree; all Skeleton blocks aria-hidden
|
No new owner or extension mechanism appears. The design treatment is already covered by Design Conventions’ approved Loading (Skeleton style) representation and Skeleton’s own “known dimensions” guidance; this is not an uncovered design decision.
VERDICT: clear
At 390×844 and 1440×900, readers now see a named, responsive loading shape rather than the footer occupying the article. Direct Properties and Butter links land directly on their requested state rather than briefly showing Overview/Neutral.
At 1440×1280, the fixed 900/1000px reserves end above the viewport bottom. A reader on a tall display or zoomed-out page sees the footer during loading and then sees it jump below the fold when content arrives: current-head CLS is 0.0487 on component routes and 0.0523–0.0561 on theme routes. This remains a large improvement over the exact base (0.1492 / 0.0906–0.0956) and stays in the “good” band, so it is a non-blocking precision/far-side finding.
VERDICT: note — the zero-shift result is viewport-conditional
No package API, route pathname, docsite prop, export, default, or public type changes. No new surface ossifies.
VERDICT: clear
No theme target, token, component variable, or override changes. New fallback paint uses the existing Skeleton target and semantic spacing/radius tokens. Responsive layout is StyleX media-query CSS.
VERDICT: clear
BEHAVIOR: Compared with Round 1, direct query state is again authoritative on the server/client; the one-frame Overview/Neutral handoff is removed. Compared with the exact PR base, behavior is preserved. API: no — no public signature/export/default changes. VISUAL: yes, intentionally — empty holes become responsive Skeleton states; Round 1’s full article first frame becomes a Skeleton for tabbed component pages. THEME: no — existing targets and token resolution remain.
VERDICT: clear
EFFECTS: zero added or changed in the current PR patch. Relative to 3acbde9, the revision removes the component/theme URL-sync Effects and their two popstate listeners. The existing theme carousel observer is byte-identical to the current base and outside this revalidation.
| Metric | Exact base 824a181
|
Current 1bd9da7
|
Delta |
|---|---|---|---|
| DateInput route JS, gzip | 1,279,564B | 1,279,645B | +81B |
| Themes route JS, gzip | 1,080,081B | 1,080,162B | +81B |
| DateInput HTML, gzip | 274,080B | 274,637B | +557B |
| Themes HTML, gzip | 53,382B | 54,145B | +763B |
Themes remains request-dependent and private, no-store, matching the exact current base. Round 1’s static/shared-cache improvement is deliberately given up to keep the server-selected deep-link model; it is not a regression from main.
VERDICT: clear
VISUAL CHECK: manual frames required
WHY: the replacement intentionally changes both loading surfaces; pr-visual skipped on this head.
| Exact base | Current head |
|---|---|
 |
 |
 |
 |
| Exact base | Current head |
|---|---|
 |
 |
 |
 |
| Fallback | Loaded endpoint |
|---|---|
|
 |
|
 |
| 1440×1280 fallback | Settled |
|---|---|
 |
 |
Sensor receipts. Each PNG has a neighboring .sensors.json proving exact base/head SHA, route, Astryx theme, light mode, LTR, viewport/DPR/media, semantic state, geometry, reduced motion, settled fonts, and zero page errors. The decisive current fallback receipts also record the AX status name and footer geometry. Machine-readable summary · reproduction.
Matched 1.5Mbps / 40ms / 6× CPU results:
| Viewport | Route | Base | Current |
|---|---|---|---|
| 390×844 | DateInput / Properties | 0.5889 | 0.0001 |
| 390×844 | Themes / Butter | 0.0003 / 0* | 0.0003 / 0 |
| 1440×900 | DateInput / Properties | 0.2121 | 0.0002 |
| 1440×900 | Themes / Butter | 0.1354 / 0.1380 | 0.0009 / 0.0036 |
| 1440×1200 | DateInput / Properties | 0.1591 | 0.0001 |
| 1440×1200 | Themes / Butter | 0.1016 / 0.1037 | 0.0008 / 0.0029 |
| 1440×1280 | DateInput / Properties | 0.1492 | 0.0487 |
| 1440×1280 | Themes / Butter | 0.0906 / 0.0956 | 0.0523 / 0.0561 |
* Chromium marked the mobile base theme replacement as recent-input in one run; its inclusive window was 0.3377. Current stays 0 either way.
VERDICT: note — normal endpoints are stable; the fixed reserve leaves a measurable tall-viewport footer jump
The fallback region exposes one named status (“Loading component documentation” / “Loading theme explorer”) in Chromium’s AX tree; each decorative Skeleton is aria-hidden. Reduced motion removes all Skeleton animations in the captured accessibility path. The loaded status disappears when the real content arrives. This uses Design Conventions’ approved “Loading (Skeleton style)” representation: “a skeleton placeholder stands in for content not yet arrived; dimensions stay stable.”
No string catalog, focus, keyboard, role of existing interactive content, direction, or locale behavior changes. Direct query state and component interactions were driven with zero page errors.
VERDICT: clear
- Step-0 current-patch safety: clean.
-
static-shell.test.ts: 3/3 passed. - docsite typecheck: passed.
- focused ESLint on all four files: passed.
- workspace package build: passed.
- exact current-head docsite production build: passed; DateInput and Themes remain partial prerender routes with named static fallbacks.
- exact current-base docsite production build: passed with the same canary target.
- current-head CI: build, lint, test, docsite-test, dependency and scope checks green; visual/a11y/RTL skipped by path filter.
- direct component/theme queries, component URL preservation and clicks, theme selection, no-tab static output, and three fresh hydration runs passed.
-
SiteFootersource is byte-identical to Round 1; its changed Section-padding dependency was remeasured at the exact deployed current head: 1180×233 with the same desktop tracks, and 390×365 with the same 320px mobile link measure. Metadata builder, component route metadata, and root layout remain byte-identical; those checks were not repeated.
| Slot | Verdict |
|---|---|
| Problem | clear — harmful friction demonstrated |
| Solution | clear — medium burden, proportionate |
| Architecture | clear — framework-owned query boundary; no Effect state machine |
| Impact | note — tall-viewport residual shift |
| API | clear |
| Theming | clear |
| Breaking | clear — intended loading-state change |
| Performance | clear — +81B gzip JS, no new Effects |
| Visual | note — 0.049–0.056 CLS at 1440×1280 |
| A11y & i18n | clear |
GOAL: partly met. Normal mobile, desktop, and 1200px-tall endpoints are effectively zero; at 1280px high, the fixed reserve exposes the footer and leaves 0.049–0.056 CLS.
DISPOSITION: tall-viewport shift → non-blocking on this internal-owner PR. It remains in the good CLS band and improves the exact base by roughly 46–67%; the public review names it rather than repeating “0”.
ADVICE: bounded direction. Design Conventions defines Skeleton loading as “a skeleton placeholder stands in for content not yet arrived; dimensions stay stable.” Either scope the body’s zero claim to its measured viewport or size the reserve from the viewport so the footer starts below the fold. Acceptance: the tall-viewport row no longer exposes/moves the footer.
AUTHOR CAN PROCEED: yes at the reviewed head. 1bd9da727745cf014cfd44df19b81f081dcc39bc fixes the harmful normal-viewport regression; the remaining objective defect is a non-blocking far-side case with clear acceptance criteria. No verdict is applied to the later 6bfd9a8a67c3361878e73033f400bffea4ef450d.
WORST OUTCOME: “A reader on a tall display sees the footer during loading and then sees it jump below the fold when the panel hydrates.” → note/approve, because the page remains usable, CLS remains in the good band, and the exact base is materially worse.
JUDGEMENT NEEDED: none. Skeleton loading is an existing approved representation in Design Conventions. The prior design-owner approval does not cover this head, but no new design concept remains for a human to choose.
-
[not blocking] fixed fallback heights stop before a tall viewport does
→ a reader at 1440×1280 sees the footer flash and move on hydration (CLS 0.0487–0.0561) ·
ComponentDetailClient.tsx:72,themes/page.tsx:52
Thanks, this is much cleaner. The PPR skeletons preserve native deep links and keep normal mobile/desktop loads stable. One tall-viewport nit inline.
[Reviewed by Robohands]
apps/docsite/src/components/component-detail/ComponentDetailClient.tsx:72 — Readers with tall CSS viewports can see the footer jump on hydration (0–0.061 CLS); could this reserve track viewport height?
Pass 1 correctly identified the fixed-height far side and invalid prior premises, but proposed a design hold. Pass 2 withdrew that hold because Design Conventions already owns the Skeleton loading representation. It required durable embedded pixels, probabilistic tall-viewport wording, a citation for stable dimensions, the tall outcome in WORST OUTCOME, and current-head footer remeasurement; all are incorporated here. Final critic disposition: PASS — approve with one non-blocking inline.
TIME revalidation 58m
setup 9m fresh metadata/wiki, replacement-patch isolation, exact worktree
reading 10m current rules, current patch, base evolution, source
measuring 29m 12 frames, 32 CLS runs, AX, interactions, bundles, two builds
writing 10m Round 2 correction + critic reconciliation
waste 8m two readiness selectors assumed visible desktop theme options
- The body’s historical
0.2281/0.2061numbers remain under-specified by viewport and cache state; the exact current base/head comparison above replaces them for this round. - No second browser engine was run; the changed mechanism is Next PPR plus CSS/DOM geometry, not an engine-specific API.
Head reviewed: 6bfd9a8a67c3361878e73033f400bffea4ef450d
Previously reviewed head: 1bd9da727745cf014cfd44df19b81f081dcc39bc
Current PR base: 5b5f1903a8eeccde0e404062a44a168e81677279
Verdict: request changes (drafted; no public PR action)
LOOP VERSION: 1.5.0
AUDIT RUBRIC: 1.12
Round 1 and Round 2 above remain unchanged and pinned to their own heads. GitHub still resolved to the requested exact target immediately before this record was written.
The branch was force-rebased again, so the direct old-head/new-head history contains unrelated main commits. The review unit is the old patch (824a181..1bd9da7) against the current patch (5b5f190..6bfd9a8). The four PR files are byte-identical between the two bases; their revision-content delta is exactly 4 files, +181/−86:
-
ThemeHeading.tsxextracts the heading, description, docs link and mode-control slot; - the
/themesfallback now mirrors the resolved desktop/mobile composition and adds theme-card/preview Skeletons; -
ThemePackagePage.tsxadopts the shared heading; -
static-shell.test.tschanges source-shape assertions for the extraction.
ComponentDetailClient.tsx and SiteFooter.tsx are byte-identical to the reviewed Round 2 head, so component behavior, footer behavior and their evidence premises did not change and were not re-reviewed. Current patch · revision-content delta.
Step 0 passed before install/build/server/browser: only docsite source and a test changed. No package, lockfile, workflow, lifecycle script, executable tool, dependency, credential, environment or build-network trigger changed.
The mobile fallback passes literal isMobile and renders display-2. The streamed resolved page instead passes isMobile={isMobile} from useMediaQuery; that hook’s server snapshot defaults to false, so the resolved server markup renders display-3 until hydration.
On both /themes and /themes?theme=butter at 390×844, the production timeline recorded:
fallback 35px font / 43.984px line box
streamed resolved boundary 29px font / 36px line box
hydrated settled 35px font / 43.984px line box
A mobile reader therefore sees the page title shrink and re-expand, moving the copy and actions below it on the exact handoff this PR is meant to stabilize. The route’s total measured CLS was 0.0185–0.0198; that score also contains site-shell movement, so it is not attributed wholly to the heading. The three-phase heading geometry itself is directly observed.
Source chain: mobile fallback → type choice → resolved mobile call.
VERDICT: blocking. The two responsive blocks are already selected by CSS. Passing fixed desktop/mobile values at their respective call sites removes the server/client disagreement; the regression test should assert that fallback and resolved variants agree, not only count component names.
The expanded fallback now exposes a real Learn how theming works link inside the temporary Suspense subtree. With the fallback held on a throttled production load, the link was focused successfully on both /themes and /themes?theme=butter; when the resolved boundary replaced it, document.activeElement became BODY in both runs.
A keyboard reader who reaches the link during a slower load loses their place and must navigate from the page start again. The fallback AX subtree also confirms the named status contains the heading, description and link rather than only decorative Skeletons. AX/focus receipt.
VERDICT: blocking. Keep interactive content outside the replaceable boundary, omit it from the fallback, or deliberately restore focus to the corresponding resolved control.
| Case | Fallback | Settled | Result |
|---|---|---|---|
390×844 /themes
|
 |
 |
first/final heading geometry matches; timeline catches the intermediate 35→29→35 phase |
1440×900 /themes
|
 |
 |
heading y 203→201; 40px fallback controls settle to 36px; copy/link move 4px |
Every PNG has a neighboring sanitized .sensors.json proving exact SHA, viewport, media, theme, semantic state, fonts, animation state and geometry. Round 3 machine-readable summary.
The expanded theme fallback fixes Round 2’s tall-theme far side:
| Viewport | /themes |
Butter |
|---|---|---|
| 390×844 | 0.01853 | 0.01976 |
| 1440×900 | 0.00094 | 0.00360 |
| 1440×1200 | 0.00079 | 0.00286 |
| 1440×1280 | 0.00077 | 0.00268 |
Round 2’s 1440×1280 theme values were 0.0523–0.0561. The unchanged component route retains its Round 2 tall-viewport note; this delta neither fixes nor worsens it.
Current-base route payload: exact head adds 1,899B gzip HTML (53,429B → 55,328B) for the richer fallback while route JavaScript is flat (−148B gzip, 1,120,224B → 1,120,076B; 47 → 46 script requests). Base receipt · head receipt.
The fallback’s status now contains real heading/copy/link content in addition to hidden Skeletons. The driven focus handoff confirms that focusing its docs link and then replacing the boundary drops focus to BODY on both bare and Butter routes; this is the second blocking finding.
The changed source-shape test passes while the real responsive variants disagree. That is evidence the gate misses the broken invariant, not a separate user-facing defect.
- exact target and GitHub head: matched
6bfd9a8a67c3361878e73033f400bffea4ef450dbefore finalization; - Step-0 current-patch safety: passed;
-
static-shell.test.ts: 3/3 passed; - workspace package build: passed;
- exact-head docsite production build: passed;
/themesremains partial prerender; - docsite typecheck: passed;
- focused ESLint on all four delta files: passed;
- production Chromium: mobile/desktop fallback and settled frames, two mobile phase timelines, eight CLS routes, fallback AX, and focus handoff on bare/Butter routes passed without page errors;
- exact-head CI: build, Storybook, scope, dependency, docsite-test, lint and test passed; visual/a11y/RTL jobs skipped by path filter;
- isolated current-base workspace/docsite production builds: passed;
/themesstayed partial prerender; - exact-base/head
/themesroute payload: +1,899B gzip HTML, −148B gzip JavaScript, one fewer script request.
The focused critic returned request changes. Its primary prediction — mobile display-2 → display-3 → display-2 — was confirmed in exact-head production Chromium. Its fallback-link focus-loss concern was also confirmed on both bare and Butter routes. The 40→36px control mismatch was confirmed but is folded into the heading geometry issue rather than filed separately. Its component tall-viewport point is unchanged Round 2 evidence, not a Round 3 delta finding.
GOAL: not met on mobile. The taller fallback closes the prior theme-footer jump, but the shared heading still changes geometry twice during the PPR/hydration handoff.
AUTHOR CAN PROCEED: yes. Both impact-led blockers have complete outcome criteria and JUDGEMENT NEEDED: none; remedy confidence is not a hold. The request-changes verdict remains.
WORST OUTCOME: a mobile reader sees the title and content contract then expand during load; a keyboard reader who reaches the fallback docs link loses focus to the page body when the boundary resolves.
JUDGEMENT NEEDED: none. Skeleton loading remains an approved existing representation; this is an implementation mismatch.
-
[blocking] mobile fallback and resolved server markup choose different heading types
→ a mobile reader sees 35/44px → 29/36px → 35/44px and the content below reflows ·
ThemePackagePage.tsx:916 -
[blocking] the temporary fallback contains a focusable link that is destroyed on replacement
→ a keyboard reader loses focus to
BODYand must navigate from the page start again ·themes/page.tsx:218
Thanks — the expanded fallback closes the tall theme-page jump, but two handoff problems remain. On mobile the title renders 35/44px → 29/36px → 35/44px across fallback, stream and hydration. A keyboard reader who focuses the fallback docs link also loses focus to the page body when the boundary resolves. Two blocking inlines.
[Reviewed by Robohands]
apps/docsite/src/components/ThemePackagePage.tsx:916 — Mobile readers see this title shrink from 35/44px to 29/36px when the streamed server markup replaces the fallback, then grow back after hydration, moving the content below it. useMediaQuery’s server snapshot is false; could the desktop/mobile call sites pass fixed values so their server markup matches the fallback?
apps/docsite/src/app/(site)/themes/page.tsx:218 — Keyboard readers can focus this fallback’s docs link, but when the resolved boundary replaces it focus drops to BODY on both bare and Butter routes. Could the fallback avoid temporary interactive content, or keep this link outside the replaced subtree so focus survives?
Nothing was posted. This review remained read-only by request.
- No second browser engine was run; the confirmed mismatches follow React server snapshot, DOM replacement and CSS geometry rather than an engine-specific API.
#5548 fix(docsite): stabilize PPR loading states by imdreamrunner (bucket: internal)
0d19a67ba94364ead8ea3e7be9c9e7d7c58d1c6e
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full WHY: This is an R1e re-review with an unresolved focus block, intentional loading-state pixels, shared responsive layout, and more than one runtime decision.
WHY 1: Query-dependent PPR sections previously left empty holes, and the footer initially rendered desktop geometry on phones. WHY 2: Streamed content moved the page while readers were orienting or navigating it, and temporary focusable content could disappear under keyboard focus. WHY 3: A reference site must preserve a reader’s visual and keyboard position while the requested documentation arrives. USER-FACING PROBLEM: A reader opening component or theme documentation sees an unstable loading page and can lose their place while trying to navigate it. PROBLEM SEVERITY: harmful friction — the page remains available, but readers repeat navigation or lose visual position during load.
VERDICT: clear
The static shell now includes stable headings and geometry while only query-dependent content streams. The theme fallback and resolved page share the same responsive layout parts, and the footer uses CSS rather than a server-unknown viewport value. Temporary controls are intended to stay out of keyboard focus until the resolved content arrives.
SOLUTION (3 decisions · ~821 production-file additions of 919 total additions)
- Narrow the component-page query boundary and render a geometry-preserving Skeleton fallback.
- Share the theme explorer’s responsive shell between fallback and resolved content, while suppressing temporary link focus.
- Render one footer tree and switch its responsive geometry with CSS.
BURDEN: medium — two loading surfaces and ten internal layout primitives add maintenance, but no public surface, new state, or new Effect. BURDEN MATCH: proportionate — the shared structure prevents the fallback and resolved layouts from drifting while fixing measured loading instability across two routes and the global footer.
VERDICT: BLOCKS — the theme fallback still mounts a keyboard-focusable Carousel that is destroyed when the boundary resolves.
OWNER: each route owns its PPR boundary; ThemeExplorerLayout owns shared responsive geometry; Carousel owns scroll focusability. TIER 1: Next Suspense/PPR, Astryx Skeleton, Carousel, and CSS media queries. TIER 2: none. SEAMS: static fallback → streamed content; mobile/desktop; direct query links; component tab selection; global footer hosts. BEHAVIOR UNIT: framework boundary — no new Effect state machine; the loading handoff is exercised in production Chromium.
| seam | driven result |
|---|---|
| mobile heading fallback → resolved | 35px font, 43.984px line box, x=24 and y=324 throughout |
| desktop heading fallback → resolved | 29px font, 36px line box, x=41 and y=201 throughout |
| temporary docs link |
tabIndex=-1; no longer sequentially focusable |
| temporary mobile Carousel | its inner scroller remains tabIndex=0; focus moves to BODY when replacement removes it |
The heading extraction fixes the responsive server-snapshot mismatch. The fallback’s use of the real Carousel crosses the same replacement seam as the removed link, so the prior focus defect remains at a different descendant.
VERDICT: BLOCKS — temporary interactive ownership still sits inside the replaced Suspense subtree.
The heading no longer shrinks and grows during the mobile handoff, and the docs link no longer enters sequential focus. A keyboard reader can still tab to the fallback Carousel’s unnamed scroll container; when the resolved explorer replaces it, focus drops to the page body and they must navigate forward again.
The component-page and footer implementations are byte-identical to the prior reviewed head, and main did not move under any of the seven changed files after that review.
VERDICT: BLOCKS — the prior focus-loss outcome remains reachable.
No package API, route pathname, exported type, prop, default, or theme surface changes. The extracted layout components are internal docsite modules.
OSSIFICATION: none — no consumer-facing surface is added or changed.
VERDICT: clear
No theme target, token, component variable, or override changes. New fallback styles use semantic spacing, radius, border, surface, and shadow tokens; fixed widths/heights are layout constraints permitted by rubric T2b.
VERDICT: clear
BEHAVIOR: yes — the new theme loading state adds a temporary keyboard stop that disappears and drops focus. API: no — no public signature, route, export, or default changes. VISUAL: yes, intentionally — empty holes become Skeleton layouts; the pushed fix keeps mobile and desktop heading geometry stable across replacement. THEME: no — existing targets, tokens, and overrides continue to resolve.
VERDICT: BLOCKS — focus management regresses during the loading handoff.
EFFECTS: zero added or changed. The existing theme-carousel observer is unchanged from the PR base.
RENDER: the loading state renders 17 bounded Skeletons and one Carousel only until the PPR boundary resolves; no growing render loop was added. LISTENERS/OBSERVERS: none added or changed. LAYOUT: no JavaScript layout reads or writes were added; responsive switching is CSS-owned. BUNDLE: no dependency or package-surface change; exact-head production build and remote build passed.
VERDICT: clear
VISUAL CHECK: manual frames required WHY: the PR intentionally replaces empty PPR holes with visible responsive Skeleton layouts, and stable visual regression was skipped on this head.
| Case | Prior head 6bfd9a8
|
Current head 0d19a67
|
|---|---|---|
| 390×844 fallback | |
 |
| 1440×900 fallback | |
 |
SENSOR RECEIPT: both use /themes, Astryx light rendering, LTR, 390×844 at DPR 1, reduced motion, eight visible of 17 total Skeletons, one named loading status, loaded fonts, zero page errors, and a 342×947.984 fallback box at x=24/y=304. Current SHA is 0d19a67ba94364ead8ea3e7be9c9e7d7c58d1c6e; the current receipt additionally proves the docs link is tabIndex=-1 and the Carousel scroller is the sole remaining tabIndex=0 descendant.
Current receipts: mobile · desktop · focus handoff.
VERDICT: clear — exact-head mobile and desktop fallback pixels are durable and the responsive heading geometry holds.
Auto-covered: exact-head CI build, lint, test, and docsite-test passed; component-scoped pr-a11y was skipped by the path filter.
Browser-checked: the fallback docs link is tabIndex=-1, but Carousel intentionally hardcodes its inner scroller to tabIndex=0 (Carousel.tsx:524, backed by its focusability test). On a throttled 390×844 production load, the fallback had exactly that one sequential focus target; focusing it and waiting for replacement left document.activeElement === BODY with no page errors.
No translation catalog, locale behavior, directional interaction, or existing accessible name changed.
VERDICT: BLOCKS — a keyboard focus path still terminates at the page body.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — fallback Carousel preserves the focus-loss class |
| ARCHITECTURE | BLOCKS — temporary interactive owner is inside replacement |
| IMPACT | BLOCKS — keyboard position is lost |
| API | clear |
| THEMING | clear |
| BREAKING | BLOCKS — focus management |
| PERFORMANCE | clear |
| VISUAL | clear — current mobile/desktop frames and receipts are durable |
| A11Y & I18N | BLOCKS — focus drops to body |
GOAL: partly met — the exact-head heading remains 35/43.984px at x=24/y=324 across mobile fallback and resolution, but the fallback still offers a keyboard stop that disappears. DISPOSITION: fallback Carousel focus loss → blocks now; the prior heading mismatch is resolved. ADVICE: proven remedy — keep the temporary skeleton strip in shared CSS geometry without mounting the focusable Carousel; acceptance is zero sequential-focus descendants in the fallback and preserved active focus through replacement. AUTHOR CAN PROCEED: yes — remove the temporary tab stop and add a focused production/browser regression that fails if fallback replacement drops focus. WORST OUTCOME: “A keyboard reader can still tab to the fallback Carousel’s unnamed scroll container; when the resolved explorer replaces it, focus drops to the page body.” → request changes.
JUDGEMENT NEEDED: none — this is the same previously reviewed focus-management defect, not a new API or design decision.
request changes
- The fallback Carousel remains a temporary keyboard stop
→ a keyboard reader loses focus to the page body when the PPR boundary resolves ·
apps/docsite/src/app/(site)/themes/page.tsx:173
Thanks — cixzhang’s prior review asked for stable heading geometry and preserved focus. The heading now holds, but the fallback Carousel remains a tab stop; replacing it still drops focus to the page body. Could we keep every temporary fallback descendant out of the tab order?
[Reviewed by Robohands]
-
apps/docsite/src/app/(site)/themes/page.tsx:173— Carousel keeps this temporary skeleton strip tabbable; replacement drops focus toBODY. Could the fallback use inert shared layout?
- The component-page and footer files are byte-identical to the prior reviewed head; their prior evidence remains applicable.
- The focused source-shape test passes, but it checks the docs link and shared component names rather than enumerating every temporary focus target.
- The exact-head mobile and desktop heading timelines both stayed geometrically stable.
TIME total 25m setup/rules 7m kit, rubric, prior review, isolated worktree; warm main reused: yes install/build/server 4m APFS clone hit, workspace + docsite production build; docsite 62459, Storybook 62458 reserved unused browser/a11y 5m heading and focus handoffs in exact-head Chromium screenshot capture 2m 2 current sensor-gated frames; 0 post-finding re-measures focused tests 1m static-shell Vitest and changed-file ESLint code/history 4m current diff, prior-head delta, Carousel contract, main movement critique+wiki 2m one failed durability gate, revision, append, rebase, push CI wait 0m exact-head remote CI already green waste 4m wrong focused-test invocation and three probe wiring/readiness corrections
- No second browser engine was run; managed macOS supports Chromium only.