refactor: remove ASCII/LQIP image-preview experiment, add spinner#443
Merged
Conversation
The hero/post images carried a two-layer loading-state experiment: an ASCII-art halftone preview plus a legacy LQIP blur placeholder, wired through ThemeAwareHero -> ImageWithAsciiPreview -> AsciiPreviewImage with an OptimizedImage blur fallback. It was a lqip -> preview cutover that never finished, so every Media upload still dual-wrote both fields on beforeChange. The only user-visible result was a "weird ASCII thing" and a meaningful pile of components, hooks, scripts, CSS, and tests to carry. Rather than finish the cutover (#246), delete both sides of the experiment and replace the loading state with a standard, zero-JS CSS spinner rendered behind the two stacked theme variants — the opaque object-cover image paints over it once the active variant loads. This keeps ThemeAwareHero a server component (no onLoad, no client boundary) while cutting the render path down to plain next/image. The dark/light theme-aware hero, sharp, the hero-glow/hero-filter chrome, and the theme-switch E2E coverage are all core infrastructure and are kept; only the experiment is removed. A new forward migration drops the now-orphaned lqip/preview_color/preview_ascii columns (existing add-column migrations are left untouched), and payload-types is regenerated from the trimmed Media config. This supersedes #246 (we delete both sides instead of cutting over) and completes its "drop the lqip column" item. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
julianken-bot
approved these changes
Jun 1, 2026
julianken-bot
left a comment
julianken-bot
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVE
Clean, complete removal of the ASCII/LQIP image-preview experiment, replaced by a zero-JS CSS spinner. The deletion is internally consistent across all eight surfaces it touches — component, CSS, collection schema, generated types, forward migration, content-model doc, workflow, and scripts — with no orphaned consumers. Every acceptance check I ran this turn is green. One minor design-tradeoff SUGGESTION; not a defect.
Verification ledger (commands I ran this turn — not trusting the PR body)
| Check | Result |
|---|---|
Acceptance grep over src tests scripts .github |
exit 1, no matches |
pnpm typecheck @ b1c2662 |
clean, exit 0 |
pnpm test:unit |
43 files / 662 tests pass |
pnpm lint + lint:adp |
0 errors, 19 pre-existing warnings |
| Migration coherence | drops exactly lqip/preview_color/preview_ascii; names match the add-column migrations; existing bodies untouched; timestamp strictly last |
payload-types.ts |
lqip + preview removed from Media and MediaSelect — clean regen, not hand-edit |
| Orphan consumers | none |
Workflow/script coherence (backfill-*) |
none dangling |
CSS contract (.hero-spinner, border-border, border-t-accent) |
all resolvable via @theme inline / :root / .dark |
| HEAD stable | unchanged b1c2662 |
GitHub CI green on every verdict-relevant check (ESLint, TypeScript, Vitest, Next.js Build, Analyze Bundle, CodeQL).
Findings
1. [scope / R10] Large diff (122+/2055−, 27 files), but ~95% is mechanical deletion of one self-contained experiment; the load-bearing new code is a single component + one CSS rule + one migration. Reviewed in full; scope appropriate.
2. [SUGGESTION] src/components/ThemeAwareHero.tsx — the animate-spin spinner renders behind both <Image> variants and never unmounts, so the CSS animation runs perpetually on every hero and every PostCard thumbnail even after the opaque image fully covers it. Cost is small (GPU-composited transform), but it's an always-running animation on a permanently-invisible element across the listing grid. Pausing it post-load would require the onLoad/client boundary this PR deliberately removed — a conscious zero-JS tradeoff, not a bug. Noting for the next reader of the CSS.
Checked, NOT findings
- z-order — correct.
.hero-spinner { z-index: 0 }(DOM-first) vsnext/image fillatz-index: auto(DOM-later): both stack level 0, painted in tree order → images paint over the spinner. - "Infinite spinner on image 404" — NOT a regression (R7). The old
OptimizedImagepath also had noonErrorhandler. Pre-existing, excluded. - R16 screenshots — deferral accepted on merits. No per-PR preview env; the migration-bearing dev server would
drizzle pushagainst a remote DB and drop the columns pre-merge (a real hazard); loaded-UI delta is near-zero; spinner presence is unit-asserted. - R11 no injection; R15 no mermaid.
- R13 (
migrations/**,.github/workflows/**) and R14 (spinner className) shadow checks both PASS — repo uses Tailwind v4@theme inline, not the grep's assumed paths; verified the substance (every new class resolves).
Bottom line
Complete, coherent deletion with green typecheck/lint/unit and a verified migration. Schema-drop, type regen, and content-model doc all consistent; spinner z-order behaves as documented. APPROVE.
— @julianken-bot (opus, fresh context) · same-tier author caveat noted (R12)
Owner
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
This pull request spent 5 minutes 56 seconds in the queue, including 5 minutes 16 seconds running CI. Required conditions to merge
|
This was referenced Jun 1, 2026
mergify Bot
pushed a commit
that referenced
this pull request
Jun 1, 2026
* feat: load-aware hero spinner + glitch-in reveal (#444) 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> * test: cover hidden-variant onLoad guard + drop dead eslint-disable (#444) 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Diagrams
N/A — pure removal + render-path simplification; no new architecture, data flow, or state machine to diagram. The before/after is "ASCII halftone + LQIP blur layered components" → "two stacked
next/imagevariants with a CSS spinner behind them."Summary
Removes the unfinished ASCII/LQIP image-preview experiment and replaces the loading state with a standard, zero-JS CSS spinner.
Why: Hero/post images carried a two-layer placeholder experiment — an ASCII-art halftone preview plus a legacy LQIP blur — wired through
ThemeAwareHero → ImageWithAsciiPreview → AsciiPreviewImagewith anOptimizedImageblur fallback. It was alqip → previewcutover that never finished, so every Media upload still dual-wrote both fields onbeforeChange. The only user-visible effect was a "weird ASCII thing." This deletes both sides instead of finishing the cutover (supersedes #246).What changed:
ThemeAwareHero.tsxnow renders the two theme variants directly vianext/image(dark:hidden/hidden dark:block,fill,object-cover, focal-point style,relative+ aspect-ratio wrapper preserved). Removed the unusedASCII_LENGTHimport and thehasUsablePreviewdual-read. Added a CSS-only centered spinner rendered behind the two images, so the opaque loaded image paints over it — the component stays a server component (noonLoad, no client boundary).OptimizedImage,ImageWithAsciiPreview,AsciiPreviewImage;lib/preview/encode,lib/lqip/encode,lib/hooks/generate-preview,lib/hooks/generate-lqip; the "ASCII PREVIEW"globals.cssblock (incl.--preview-color+ reduced-motion sub-block);scripts/backfill-{lqip,previews}.ts+ theirpackage.jsonscripts;.github/workflows/backfill-previews.yml; the experiment unit/e2e tests.beforeChangelqip+preview hooks, thelqipfield, and thepreviewgroup fromMedia.ts; regeneratedpayload-types.ts(not hand-edited); updatedCONTENT_MODEL.md.20260601_000000_drop_media_lqip_preview.tsrunsDROP COLUMN IF EXISTS "lqip","preview_color","preview_ascii"onmedia(existing add-column migrations untouched; registered inmigrations/index.ts). Completes chore(media): file lqip→preview cutover follow-up #246's "drop the lqip column.""var(--accent)"instead ofpreview?.color || "var(--accent)"(the.hero-filter/.hero-glowCSS is kept).sharp(shared bypayload.config.ts+convert-hero-webp.ts+generate-rss-icon.ts),PostCard,TextureOverlay/scanline/card-*chrome, theme-switch E2E.Screenshots
Deferred to post-deploy verification (agreed with maintainer). This repo has no per-PR preview env (Cloud Run deploys on merge), and
.env.localtargets a remote DB the migration-bearing dev server must not run against (drizzlepushwould droplqip/preview_*remotely pre-merge). The loaded UI is visually unchanged — this removes a transient ASCII/blur placeholder and adds a CSS spinner; spinner presence is asserted inThemeAwareHero.test.tsx. Post-deploy, the orchestrator captures light+dark home+post against production to confirm no hero regression and correct spinner z-order in both themes.Test plan
pnpm typecheck— PASS (clean, no errors).pnpm lint(ESLint +lint:adpsketch/reference/affiliate/changelog checks) — PASS (0 errors; only pre-existing warnings).pnpm test:unit— PASS (43 files, 662 tests).ThemeAwareHero.test.tsxupdated: dropped theblurDataURL/lqip + ASCII dual-read assertions, kept variant / aspect-ratio / focal-point assertions, added a spinner-present assertion.pnpm build— PASS (Compiled successfully, 67/67 pages, exit 0). DB-connection noise at static-export time is the sandbox having no Postgres, not a code defect.pnpm test:e2e— DEFERRED-CI: requires a Postgres-seeded env (PlaywrightglobalSetupseeds the DB; webServer boots the app). The 4 E2E shards run in CI + are enforced by Mergify before merge. The kept hero specs (theme-aware-hero.spec.ts,homepage-hero.spec.ts) assert theme switching / hero presence and are unaffected.grep -rn "ascii-preview\|AsciiPreview\|ImageWithAsciiPreview\|OptimizedImage\|generate-lqip\|generate-preview\|lib/lqip\|lib/preview\|data-loaded\|--preview-color\|blurDataURL\|hero-loading" src tests scripts .github— CLEAN (no matches).Plan reference
Out of plan — remove failed image-preview experiment; replace with a standard spinner.
Closes #441
Closes #246
🤖 Generated with Claude Code