feat: load-aware hero spinner + glitch-in reveal#445
Conversation
The hero spinner introduced in #441 was a zero-JS, CSS-only element that animated perpetually behind every thumbnail (raised in the #443 review), and it gave no signal that loading had finished. This makes the hero image-pair load-aware so the spinner reflects real load state and a genuine load is rewarded with a quick glitch-in. - Extract the image+spinner into a small `"use client"` child, `HeroImagePair`, rendered by `ThemeAwareHero`. The server component keeps ownership of the Media null-guard, `toRelativeSrc`, focal-point math, and the aspect-ratio wrapper, so the public props and both call sites (PostCard, posts/[slug]) are unchanged. - Cached case: a `useLayoutEffect` probes `img.complete` on the visible variant synchronously before paint (mirroring FadeReveal), so a warm image appears instantly — no spinner flash, no glitch. - Real-load case: the spinner shows while the visible variant streams in; its `onLoad` removes the spinner and applies the existing `.glitch-reveal` animation (reused, not reinvented) for the reveal. The hidden variant's onLoad is ignored so the visible variant drives the state. - `prefers-reduced-motion: reduce` is honored by the global media query in globals.css, which collapses `.glitch-reveal` to ~0ms: the image just appears, spinner still allowed. - The glitch class lives on a theme-agnostic `absolute inset-0` wrapper holding both variants — it reads correctly in light and dark, provides the positioning context the `fill` images need, and adds no layout shift. The variants stay free of opacity/transition classes so the View Transitions theme crossfade (#53) is not degraded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
) The visible/hidden discrimination keys off getComputedStyle(img).display, but jsdom never applies the Tailwind stylesheet so both variants reported display:"inline" in tests — the handleLoad early-return guard was never exercised and visibleImg() passed only by first-element fallthrough. Stub getComputedStyle so the dark variant reports display:"none" and assert load on the hidden variant is a no-op while load on the visible variant removes the spinner + applies .glitch-reveal (verified load-bearing via mutation check). Also drop the dead react-hooks/exhaustive-deps disable on the useLayoutEffect: the effect already has an explicit [] dep array, so the directive suppressed nothing and ESLint flagged it as an unused directive in a changed file. The adjacent set-state-in-effect disable is kept (legitimate). No runtime change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
julianken-bot
left a comment
julianken-bot
left a comment
There was a problem hiding this comment.
Re-review — load-aware hero spinner + glitch-in reveal
Verdict: APPROVE (1 SUGGESTION). Re-review of head b1d15d2. The two prior SUGGESTIONs are genuinely resolved, verified mechanically (not by trusting the PR's test-plan claims).
Verification ledger (every command run by me this turn, at head b1d15d2)
| Check | Result |
|---|---|
tsc --noEmit |
exit 0, clean |
eslint (3 changed files) |
exit 0 — 0 warnings in changed files |
eslint . (full repo) |
0 errors, 19 warnings, none in changed files |
vitest run |
666 passed / 43 files / 0 failures |
Mutation: delete guard HeroImagePair.tsx:101 |
hidden-variant test FAILS → guard is non-vacuous |
Mutation: disable cache probe (line 91 → if (false)) |
cached-instant test FAILS → probe is non-vacuous |
| R15 mermaid | {"ok": true, "total": 2} — both diagrams render |
| Worktree diff vs PR | identical (3 files, +305/−33); HEAD stable |
Resolved 1 — hidden-variant onLoad guard now has teeth. stubDarkHidden() stubs getComputedStyle so the dark variant reports display:"none"; deleting the line-101 early-return makes the hidden-variant test fail. Not a vacuous pass.
Resolved 2 — dead react-hooks/exhaustive-deps disable removed. Scoped lint on the changed files is exit-0, zero output. The remaining set-state-in-effect disable (line 92) is live and mirrors the FadeReveal mount-probe pattern — legitimate.
Findings
SUGGESTION — no-JS / hydration-failure spinner trade-off (HeroImagePair.tsx, see inline). The spinner is now z-10 and removed only by client state, so with JS disabled / on hydration failure it sits over a loaded hero (the old behind-the-image spinner degraded with zero JS). It's pointer-events-none and the repo declares no progressive-enhancement contract, so this is a degraded-state cosmetic trade-off — moving loading state client-side is the whole point of the PR (R9), not a defect. Cheap hardening if ever wanted: render the spinner behind the images so a loaded opaque image covers it without JS.
Out of scope: R13 no-fire; R14 shadow (glitch-reveal rule exists at globals.css:456, hero-spinner pre-existing); screenshots deferred to post-deploy (no preview env), expected; R11 no injection.
Bottom line
Clean re-review — both prior SUGGESTIONs genuinely fixed (mutation-verified), gates green by my own runs, diagrams render. The single new SUGGESTION is an accepted trade-off, not a blocker. APPROVE.
— @julianken-bot (opus, fresh context)
| {/* Centered loading spinner, shown only while the visible variant loads. */} | ||
| {!state.loaded && ( | ||
| <div | ||
| className="hero-spinner pointer-events-none absolute inset-0 z-10 flex items-center justify-center" |
There was a problem hiding this comment.
SUGGESTION — no-JS / hydration-failure trade-off introduced by this PR.
The previous spinner (base 61510a1) rendered behind both images with no z-index and no JS gate, so a loaded opaque object-cover image painted over it even with zero client JS. This spinner is z-10 (on top) and is removed only by client state ({!state.loaded && ...}), so on a hydration failure or with JS disabled it sits over a fully-loaded hero.
It's pointer-events-none (no interaction block) and the repo declares no progressive-enhancement contract, so this is degraded-state cosmetics, not a blocker — moving loading state client-side is the whole point of a load-aware spinner (reasonable trade-off, R9). Cheap hardening if you ever want the JS-off path: render the spinner behind the images (so the loaded image covers it as a fallback). Flagging so the trade-off is intentional rather than accidental.
|
@Mergifyio queue |
Merge Queue Status
Required conditions to enter a queue
|
Merge Queue Status
This pull request spent 57 seconds in the queue, including 9 seconds running CI. Required conditions to merge
|
2 participants
Diagrams
Load-aware state machine for the hero image-pair.
ThemeAwareHero(server) keeps the Media guard /toRelativeSrc/ focal-point / aspect-ratio wrapper and renders the newHeroImagePair(client), which owns this state:stateDiagram-v2 [*] --> Loading: SSR / first paint (loaded=false) Loading --> Instant: useLayoutEffect (pre-paint) - visible img.complete and naturalWidth>0 Loading --> Revealed: visible variant onLoad - spinner was shown, so real load state "Instant (cached)" as Instant state "Revealed (glitch-in)" as Revealed Instant --> [*]: no spinner, no glitch Revealed --> [*]: glitch-reveal once, spinner removed note right of Loading Spinner visible (z-10). Hidden display:none variant still loads; its onLoad is ignored - visible variant drives the state. end note note right of Revealed prefers-reduced-motion reduce collapses glitch-reveal to ~0ms via the global media query in globals.css, so image just appears. end noteflowchart LR PostCard --> TAH["ThemeAwareHero (server): guard, toRelativeSrc, focal point, aspect-ratio wrapper"] Slug["posts/[slug]"] --> TAH TAH -->|"lightSrc/darkSrc/alts/sizes/imgStyle"| HIP["HeroImagePair (use client): spinner + onLoad + glitch"] HIP --> Light["Image: object-cover dark:hidden"] HIP --> Dark["Image: hidden object-cover dark:block"]Summary
useLayoutEffectprobesimg.completeon the visible variant synchronously before paint (mirroringFadeReveal) — warm images appear instantly with no spinner flash and no glitch. A genuine load shows the spinner, then removes it and applies the existing.glitch-revealanimation (reused, not reinvented).fill,fetchPriority="high",sizes, focal-pointstyle, therelative+ aspect-ratio wrapper (CLS 0),toRelativeSrc, and theisMediaObjectguard are unchanged; the public props and both call sites need no edits. The glitch lives on a theme-agnosticabsolute inset-0wrapper so it reads in light and dark and does not degrade the View Transitions theme crossfade (Theme-aware hero images (light + dark) with View Transitions crossfade #53).prefers-reduced-motionis honored by the global media query.Screenshots
Pending — post-deploy verification (no per-PR preview env); orchestrator captures the spinner->glitch on a throttled load + cached-instant case after deploy.
Test plan
pnpm lint— green (0 errors; 19 pre-existing warnings in unrelated files, 0 in changed files). The review-flagged deadreact-hooks/exhaustive-depsdisable inHeroImagePair.tsxwas removed, so the changed files now lint clean (eslint src/components/HeroImagePair.tsx tests/unit/components/ThemeAwareHero.test.tsx→ exit 0, no output).pnpm typecheck(tsc --noEmit) — greenpnpm test:unit(vitest) — 666 passed (43 files), incl. new cases: spinner-while-loading, cached → no spinner + no glitch (stubbedcomplete/naturalWidth), real load → spinner removed +.glitch-revealapplied, and (added in review) the visible/hiddenonLoaddiscrimination —getComputedStylestubbed so the dark variant reportsdisplay:"none": load on the HIDDEN variant is a no-op (spinner stays, no glitch), load on the VISIBLE variant removes the spinner + applies.glitch-reveal. Kept variant/aspect/focal/View-Transitions assertions.pnpm build(throwaway env) — compiled successfully, all 67 routes prerenderedpnpm test:e2e— DEFERRED to CI. Local run needs a Postgres-seeded env (pnpm seed:testin global-setup); the throwaway DB has no live server. The kept spectests/e2e/public/theme-aware-hero.spec.tsis unchanged and CI-enforced (variant swap, no layout shift, no console errors).Plan reference
Out of plan — follow-up UX refinement to #441's hero spinner.
Closes #444