refactor(app): move the image pipeline, page bodies, and inline components out of four route files - #244
Merged
Merged
Conversation
…oute file The route held `loadFonts`, the Vercel Blob / API retrieval pair, the `sharp` manipulation, and an 88-line `ImageResponse` element tree — 359 lines where the thin-route-file rule asks for composition. Font loading and the cover-image pipeline move to `thumbnail.png/_/helpers/`, the element tree to `thumbnail.png/_components/post-thumbnail-card.tsx`, and `GET` keeps both call sites where they were. Retrieval and manipulation stay separately callable on purpose: a single helper doing both would pull a `sharp` failure inside the `catch` that answers `notFound()`, turning a 500 into a 404. Refs: #187
…reader out `MaybeComments` and `MaybePayloadLivePreview` were declared inline in the route file; each moves beside the component it wraps. `readShareToken` moves to `[slug]/_/helpers/share-token-param.ts`, under the segment both the page and the thumbnail route beneath it can reach. `page.tsx` is left with `BlogPostPage` and `generateMetadata` — the two exports the router calls — and nothing else. Refs: #187
… own stylesheet `IndexPageMain` was the whole page body declared inline in `page.tsx`; it moves to `_components/index-page-main.tsx`, and every rule `page.module.css` held below its scope root moves to the paired `index-page-main.module.css`. A CSS Module hashes an `@scope` prelude per file, so the moved rules could not keep scoping to `.indexPage` — they would compile a root no element carries and apply to nothing. `<main>` therefore carries `.indexPageMain`, declared as a scope root and nothing else: it shipped class-less before, so any declaration on it would be a style change rather than a move. Refs: #187
…r out `Document` and `AuthProvider` were declared inline in `layout.tsx`; both move into `app/(app)/_components/`, joining the other root-layout sub-components. `layout.tsx` keeps the three stylesheet imports, `generateMetadata`, `viewport`, and `RootLayout`. `AuthProvider` takes no `...props` spread: it roots no element, returning `children` or `children` inside `<ClerkProvider>`. Refs: #187
The `👽` logger row, the negotiated-`<html lang>` file list, and the route-level-`<Suspense>` carve-out each named a file or a shape the moves changed. The carve-out is re-justified on its operative ground — the route owns the boundary, not the component — rather than removed: `<IndexPageMain>` now has a file, and the rule it justifies still holds. The two `index-page-main-loading` comments are corrected the same way: they mirror `index-page-main.module.css` now, and the loaded root carries a scope root that declares nothing rather than no class at all. Refs: #187
`thumbnail.png/route.tsx` stood at 104 lines against the plan's 80-line criterion. @axross chose to keep the number and condense the comments rather than amend it, so no statement moved and the three rationales a reader cannot recover from the code survive: why this route exists rather than `opengraph-image.ts`, why manipulation sits outside the `try`, and why the draft headers are spread rather than set conditionally. The rest — including `draftThumbnailHeaders`' doc comment — is what the trim cost. `Document` and `PostThumbnailCard` each spread `...props` after the attribute they control, so a caller-supplied `lang` or `style` would have beaten the negotiated locale and the card's own composition. The spread moves ahead of both. No caller passes either today, so nothing rendered changes. Refs: #187
The route-level-`<Suspense>` carve-out attached "conditionally" to the skeleton, which is not conditional: `posts/[slug]/page.tsx` mounts `<CommentsLoading>` as an unconditional fallback, and it is `<Comments>` that goes unrendered on a post with comments disabled. The clause now says so, leaving the paragraph's point — the route owns the pending state — intact. `post-thumbnail-card.tsx` is the first component here that renders styled DOM with no CSS-module sibling: Satori resolves no external stylesheet inside an `ImageResponse`, so the pairing rule cannot be satisfied and should not be. The File Naming section records the exception and its ground. Refs: #187
The previous commit's correction left three lines past the 80-column wrap every other paragraph in this document keeps. Reflowed; no wording changed. Refs: #187 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PGGnRkWCgkyk7uANcbydpz
Owner
Author
|
@claude review Generated by Claude Code |
Contributor
🚀 Preview deploymenthttps://btnopen-pr-244.vercel.app Deployed |
Moving `loadFonts` into a helper module two directories below the route stopped Next's output file tracing from resolving the font it reads, so the deployed bundle shipped without `ibm-plex-sans-jp-700.ttf` and `GET /posts/[slug]/thumbnail.png` answered 500 with ENOENT. Caught on the pull request's preview deployment; the index thumbnail, which loads no font, was unaffected. Nothing local could have caught it: `next build && next start` serves the whole source tree, so the file is there whatever the trace says. Only a deployment runs from the traced set. `outputFileTracingIncludes` is the framework's own mechanism for a file the trace misses, and it is deterministic where the tracer's resolution is a heuristic. The route's `.nft.json` now lists the font, and the index thumbnail route's still does not. Refs: #187 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PGGnRkWCgkyk7uANcbydpz
Contributor
🚀 Preview deploymenthttps://btnopen-pr-244.vercel.app Deployed |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code review4 important, 0 nits Reviewed Mandatory checks
Strengths
Findings
This reviewer is advisory and does not gate merges. |
Both modules were carved out of `route.tsx`, which the router never bundles for the client, so neither needed a fence where it used to live. As ordinary importable modules they do: `cover-image.ts` reads `vercelBlobToken` and pulls in `sharp` and `@vercel/blob`, and `fonts.ts` imports three Node built-ins. `@/runtime` is not itself fenced — it resolves a "client" runtime type and client components import it — so nothing covers these transitively. Every other server-only module under `app/` carries the fence; these two now match. Without it, an accidental import from a `"use client"` component fails with a Node-builtin resolution error rather than the build-time error the fence exists to produce. Refs: #187 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PGGnRkWCgkyk7uANcbydpz
Contributor
🚀 Preview deploymenthttps://btnopen-pr-244.vercel.app Deployed |
Reverts the comment trim from bcf96e1, leaving that commit's props-spread fixes in place. The route returns to 104 lines. Three independent reviewers flagged the trim against `software-development`'s "MUST NOT delete a comment that explains a 'why', an edge case, or non-obvious behavior", and @axross reversed the earlier decision to keep the 80-line number. The acceptance criterion in #187 is amended to the substantive test it already stated alongside the count — the route declares no function other than `GET` — which the file meets. What comes back matters most at two places: the note that the token-bearing thumbnail URL is itself the secret, which is the security rationale for the `noindex` header, and `draftThumbnailHeaders`' doc comment explaining why it ships `noindex` without `nofollow`. Refs: #187 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PGGnRkWCgkyk7uANcbydpz
Contributor
🚀 Preview deploymenthttps://btnopen-pr-244.vercel.app Deployed |
Owner
Author
|
@claude review Generated by Claude Code |
Code review (round 2)0 important, 0 nits Re-reviewed Mandatory checks
What I checked beyond the round-1 findings
No new issues found in this round. This reviewer is advisory and does not gate merges. |
/posts/%5Bslug%5D/thumbnail.png/route.tsx){kind=link}
`retrieveImageViaAPI` called `arrayBuffer()` on whatever the media API answered, so a media document outliving its stored file — a cleaned-up preview store, a half-finished upload — delivered an HTML error page as bytes rather than failing. Those bytes reached `sharp`, which rejected them as an unsupported image format outside the caller's `try`, making the response a 500 where #187 requires a 404. Checking `response.ok` puts the failure back inside that `try`, which already answers `notFound()`. The Vercel Blob path has always thrown on a missing blob; this brings the API path to the same contract. Measured on a production build: with the stored file removed the route answered 500 before and answers 404 after, and with the file present the response is byte-identical either way (sha256 044b4a20…). This is the one deliberate behaviour change in a change otherwise scoped as a pure extraction, and @axross chose to make it here rather than defer it to its own issue. Refs: #187 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PGGnRkWCgkyk7uANcbydpz
Contributor
🚀 Preview deploymenthttps://btnopen-pr-244.vercel.app Deployed |
Owner
Author
|
@claude review Generated by Claude Code |
Code review (round 3)0 important, 0 nits Re-reviewed Mandatory checks
What I checked
No new issues found in this round. This reviewer is advisory and does not gate merges. |
Contributor
🚀 Preview deploymentTorn down — the isolated Turso database ( |
This was referenced Aug 14, 2026
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.
Summary
Four route files under
app/held implementation rather than composition, against the thin-route-file MUST that the recorded by-kind grouping deviation explicitly leaves in force. Each now hands its substance to a component or helper module beside the route that owns it:posts/[slug]/thumbnail.png/route.tsxsharpcover-image pipeline into_/helpers/; theImageResponseelement tree into_components/post-thumbnail-card.tsxposts/[slug]/page.tsxMaybeCommentsandMaybePayloadLivePreviewbeside the components each wraps;readShareTokeninto_/helpers/(index)/page.tsxIndexPageMainand the 336 stylesheet lines it owns, into a component with its own paired CSS Modulelayout.tsxDocumentandAuthProviderinto_components/The extraction changes nothing rendered, returned, cached, or logged — that claim is measured, not asserted, and the evidence is below. One behaviour change rides along deliberately, decided by @axross: an unretrievable cover image now answers 404 rather than 500. See Risks §3.
The one place a mechanical move was impossible is the index page's stylesheet.
page.module.csswrapped every rule in@scope (.indexPage), and CSS Modules hashes that selector per file — so rules moved into a component's own stylesheet would have compiled a scope root no element carries and silently styled nothing. The moved rules therefore re-root to@scope (.indexPageMain)and<main>carries that class. It is safe to add becausepage.module.cssnever defined.main:<main className={css.main}>was renderingclass=undefined, which the loading skeleton's stylesheet already documented. The new class declares nothing, so it is a scope anchor and not a style change.Scope note: the tracking issue lists five files.
posts/[slug]/comments/route.tswas dropped at the clarifying gate — #174 already moved its Payload read into a repository, and what remains is the handler's own ordered CSRF/auth/validation guards plus the comment write, which belong in a handler.Related issues
Closes #187
Verification
Everything below was observed in this session, in a container whose Chromium had to be replaced first (it shipped Playwright revision 1194; 1.62.1 needs 1234, so the e2e suite could not launch at all before that).
Gates — all re-run at
3cc730a:npm run formatnpm run lintnpm run typechecknpm run test:unitnpm run buildPLAYWRIGHT_SERVER_MODE=production npm run test:e2emustscenarios at 100%.git statusshows no file changed under any__snapshots__/Thumbnail bytes — the strongest evidence here. Because the e2e baselines were generated on CI hardware, a stronger check was used: both thumbnail routes were served from a production build of the base revision, from this branch locally, and from the deployed preview, and the responses compared byte for byte.
7097ac83cc730a/thumbnail.png8cae8ec3…f838364/posts/declarative-ui/thumbnail.png044b4a20…d0bbeb2The second exercises the whole moved pipeline — cover-image retrieval, the
sharptint/blur/JPEG stage, data-URL encoding, font loading, and the Satori element tree. The preview column is what settles the plan's highest-flagged risk (Assumption 8) properly: a real deployment, running from the traced file set, not a local server that serves the whole source tree.The 404 branch, measured on a production build. With the stored cover file removed, the post thumbnail answered 500 before the fix and 404 after; with the file restored it returns the same
044b4a20…bytes as before. That is the whole observable difference the change makes.Index page — no automated coverage exists, so it was measured by hand. Four full-page screenshots (412px and 712px, light and dark, animations frozen) captured before and after. Two pairs are byte-identical; the other two differ only in the Next.js dev-tools indicator badge, which is dev-server chrome. Excluding that bottom strip, all four pairs are pixel-identical.
The glitch layers were probed directly rather than inferred: all three backdrop layers resolve their z-index, colour, mask, and ±0.262123% shadow offset from the new stylesheet; both
@keyframesre-hashed into it and are still referenced; and underprefers-reduced-motion: reduceboth glitch layers still computedisplay: none.Also verified: a published thumbnail carries no
X-Robots-Tag, on the preview as locally (the draft branch that addsnoindexis covered by thepost.share-link.valide2e scenario, which passes);npm run migrate:statusunchanged and no file added underpayload/migrations/; nodata-testidvalue or owner moved.Acceptance criteria: all twelve are met.
Review: three rounds of the independent review. Round 1 raised four Important findings, all addressed; rounds 2 and 3 came back clean.
Risks and breaking changes
No breaking changes. Five things worth a reviewer's attention:
1. A regression this branch introduced, found on the preview and fixed in
0356909. MovingloadFontsinto a helper two directories below the route stopped Next's output file tracing from resolving the font it reads, so the deployed bundle shipped withoutibm-plex-sans-jp-700.ttfandGET /posts/[slug]/thumbnail.pnganswered 500 with ENOENT on the first preview. The index thumbnail, which loads no font, was unaffected.Nothing local could have caught it:
next build && next startserves the whole source tree, so the file is present whatever the trace says. Only a deployment runs from the traced set.outputFileTracingIncludesis the framework's own mechanism for a file the trace misses, and it is deterministic where the tracer's path resolution is a heuristic. The route's.nft.jsonnow lists the font and the index thumbnail route's still does not.2. Two extracted modules were missing
import "server-only", fixed inf89ec45after the independent review raised it. Insideroute.tsxthe code sat in a Route Handler the router never bundles for the client, so no fence was needed; as ordinary importable modules they need one —cover-image.tsreadsvercelBlobTokenand pulls insharpand@vercel/blob,fonts.tsimports three Node built-ins.@/runtimeis not itself fenced, so nothing covered them transitively.3. The one deliberate behaviour change: 404 instead of 500 on an unretrievable cover image (
3cc730a).retrieveImageViaAPIcalledarrayBuffer()on whatever the media API answered, so a media document outliving its stored file delivered an HTML error page as bytes; those bytes failed later insidesharp, outside the caller'stry, making the response a 500 where #187 requires a 404. Aresponse.okcheck puts the failure back inside thattry, which already answersnotFound()— the same contract the Vercel Blob path has always had for a missing blob.Both earlier review rounds flagged the criterion as unmet and judged the behaviour pre-existing. @axross chose to fix it here rather than defer it, and chose fail over degrading to the logo-and-title composition. This is the only response-behaviour difference in the branch, and it is the reason the "no behaviour change" non-goal in #187 now carries one stated exception.
4. The comment trim was tried and reverted. An earlier round deleted explanatory comments from the thumbnail route to meet an "at most 80 lines" criterion. Three independent readers flagged that against
software-development's "MUST NOT delete a comment that explains a 'why'". @axross reversed the decision:17ec8dfrestores every comment, and the criterion in #187 is replaced by the substantive test it already stated alongside the count.5. Two latent props-contract fixes.
Documentspread{...props}afterlang, andPostThumbnailCardafterstyle, so a caller could have overridden the negotiated locale or replaced the card's whole composition. Both now spread first. No current caller passes either prop, so nothing rendered changes.Notes for reviewers
Start with
posts/[slug]/thumbnail.png/route.tsxand its three new modules — that is where a behaviour change would hide, and where both the regression and the intended change live. Thetry/catchsplit is the subtle part: cover-image retrieval fails intonotFound(), whilesharpmanipulation deliberately stays outside thetryso its failure stays a 500.3cc730awidens what counts as a retrieval failure without moving either call site.Then
(index)/_components/index-page-main.module.css. The 335 moved rule lines are byte-identical to their originals modulo indentation; only the@scopeprelude changed.The commits are one per extraction, so reviewing them in order is easier than reading the combined diff.
bcf96e1onward are review fixes, the tracing repair, and the 404 change rather than extraction work.Two things this change deliberately does not do: it does not touch
comments/route.ts(see Summary), and it does not delete the dead.headlinerule it moves — both are recorded non-goals.Unrelated but found while verifying, and worth knowing:
Check and Deployhas been failing onmainsince at least635d8f6, four commits back, which means Production Deployment has been skipped on each.Merge Checksis green throughout, so the pull requests looked fine. The full e2e suite passes locally on7097ac8in production mode, so the failure appears to be CI-environment-specific rather than a code regression.