Migrate CMS from Vite to Next.js - #142
Conversation
Replace the Vite SPA and standalone Bun API server with the Next.js App Router while preserving existing routes and behavior. - Add native pages, route handlers, navigation, and CDN rewrites - Align framework and shared dependency versions with yearn.fi - Update environment variables, scripts, documentation, and tooling - Preserve strict compiler checks and the Vercel Motion DOM pin
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Summary
What I verified working: all six routes resolve, including direct navigation and refresh; the CDN proxy retains its CORS and cache headers through both /cdn/* and /api/cdn/*; Tailwind v4 output is unchanged across the Vite→PostCSS adapter swap; bun run build and tsc --noEmit pass on a clean tree; no console errors or hydration warnings on any route. The rewrite change is a non-event because getPath() in api/cdn.ts already handles both the old query-param and the new path shape.
Issues
1. next is pinned to 16.2.6, which has 9 published advisories — packages/app/package.json:20
I queried the GitHub advisory API directly: 16.2.6 falls in the affected range of 9 advisories, 4 of them High, all patched in 16.2.11. I agree with the exploitability analysis — this app has no Server Actions, no middleware, no i18n, no remote image optimization, and its one rewrite has a fixed internal destination, so none of the four Highs are reachable today. But that argument is a snapshot of current configuration, and it expires quietly the first time someone adds middleware or a Server Action. This is also a brand-new direct dependency, so the pin is a choice being made fresh here rather than inherited.
- Pin
nextto16.2.11or later. - Done when:
packages/app/package.jsonpins ≥16.2.11,bun install --frozen-lockfilesucceeds, andbun run buildpasses.
2. Question: is installCommand set in the Vercel project settings? — packages/app/vercel.json (deleted)
The deleted file carried "installCommand": "bun install --frozen-lockfile" alongside the SPA rewrites. The rewrites are correctly replaced by Next routing plus the /cdn/:path* rewrite, but nothing in the repo replaces the install command — find . -name vercel.json -not -path '*/node_modules/*' now returns nothing repo-wide. I couldn't verify the project settings from outside the org, so this may already be handled.
If it isn't: combined with the root bunfig.toml (minimumReleaseAge, exact), a non-frozen install can resolve a tree differing from the committed lockfile — the exact drift your own test plan guards against locally.
- Confirm it's set in project settings and note that in the PR's risk section, or keep a minimal
packages/app/vercel.jsonwith just{"$schema": "https://openapi.vercel.sh/vercel.json", "installCommand": "bun install --frozen-lockfile"}. Don't restore the old/(.*)→/catch-all or duplicate the/cdnrewrite. - Done when: either that file exists, or the risk section documents the required Vercel settings (install command + the four
NEXT_PUBLIC_*vars) as a pre-merge step.
3. RestoreScroll is orphaned — scroll no longer resets on navigation — packages/app/src/components/RestoreScroll.tsx:4
The component was migrated (useLocation → usePathname) but its only render site was the deleted src/main.tsx. grep -rn RestoreScroll packages/app now returns only its own declaration. Next's built-in scroll handling targets the document scroller, but this app scrolls <div id="main-scroll" class="fixed inset-0 … overflow-y-auto"> (src/routes/Layout.tsx:17).
Reproduced against next start: scrolled /vaults to scrollTop: 1514, clicked a vault link, and the detail page landed at scrollTop: 104 instead of 0 — the vault name and chain: line are clipped above the fold. Next's scrollIntoView() fallback moves it partway but does not reset it. Small in itself; I'm flagging it mainly because it's the one place I could show "preserves existing UI" isn't quite true.
- Render
<RestoreScroll />insidesrc/routes/Layout.tsxabove<Header />, or delete the file if the reset is intentionally dropped — don't leave it migrated-but-unreferenced. Keep the#main-scrollid andscrollableTarget="main-scroll"inCollectionList.tsx:108working for infinite scroll. - Done when: from a scrolled-down
/vaults, clicking through to a detail page renders it at the top, andgrep -rn RestoreScroll packages/appshows either a render site or no file.
4. CLAUDE.md still documents the deleted Vite/Bun-server stack — CLAUDE.md:19
README.md and .env.example were both updated; the agent-facing instructions file was not, so the two now contradict each other. Stale: bun dev:server → "Bun API server (port 3001)" (script no longer exists), "Frontend (React + Vite)", "React Router DOM v7", "Build: Vite with React plugin", VITE_GITHUB_CLIENT_ID / VITE_GITHUB_REDIRECT_URI at port 3001, and Key Files pointing at packages/app/api/server.ts and packages/app/src/main.tsx — both deleted here.
- Update it in this PR: dev commands (
next dev/next build/next start, addtslint, dropdev:server), architecture (App Router, API handlers underapp/api/**on the same port),NEXT_PUBLIC_*names with the port-3000 callback, and repoint Key Files atapp/layout.tsxandapp/api/**/route.ts. Leave the CDN, schema, and chain-support sections alone — they're still accurate. - Done when:
grep -in 'vite\|VITE_\|3001\|dev:server\|api/server\|main.tsx\|react-router' CLAUDE.mdreturns nothing, and every path under "Key Files" exists.
5. Local dev no longer serves packages/cdn from disk — packages/app/next.config.ts:18
packages/app/cdn is a symlink to ../cdn that existed so the Vite dev server answered /cdn/vaults/1.json from the working tree (vite.config.ts proxied only /api). Next serves only public/, which holds just yearn.svg, so the new /cdn/:path* rewrite sends every request to the jsdelivr proxy — and VERCEL_GIT_COMMIT_SHA is unset locally, so it resolves @main. A developer editing packages/cdn on a branch now sees published main data instead, and dev requires network access to render anything. All four data hooks go through this path via getCdnUrl().
- Either add a development-only branch in
api/cdn.tsthat reads the localpackages/cdndirectory (Node runtime; the path is already validated against/^[a-zA-Z0-9/_.-]+$/and..), or, if the loss is intentional, delete the now-deadpackages/app/cdnsymlink and say so in the README. Production behavior must stay as-is:/cdn/*→/api/cdn/*→ jsdelivr pinned atVERCEL_GIT_COMMIT_SHA. - Done when: with
bun devrunning and a local edit topackages/cdn/vaults/1.json, the edited value appears in the UI — or the symlink is gone and the README states dev reads published data.
Suggestions
motion-domandframer-motionare no longer in lockstep (packages/app/package.json:18-19). The immediately preceding commit, 14b4d36 "Pin Motion DOM for Vercel builds", exists solely to hold these two together at 12.23.12/12.23.12. This PR movesframer-motionto 12.34.0 andmotion-domto 12.42.2 — 8 minors apart — while also replacing the build system that motivated the pin. Either re-sync them, or note that the workaround is obsolete under Next and why.- Duplicate
/→/vaultsredirect.next.config.ts:6-14andapp/page.tsx:4both do it; the config redirect is evaluated first, soapp/page.tsxis unreachable (I confirmed/returns a 307 from the config). Keep one. if (!mounted) return nullinsrc/routes/Layout.tsx:14disables SSR for every route. I confirmed the prerendered body contains zero application markup — just scripts. That's parity with the old Vite SPA, not a regression, so it isn't blocking. But it silently masks two things: the render-timesessionStorage.getIteminHeaderDraftCart.tsx:124and the unsuspendeduseSearchParams()inGithubSignIn.tsx. Anyone deleting the gate hits both at once. Either add a comment naming exactly what it guards, or fix both at the source (storage read in an effect;<Suspense>around the search-params consumer).- Dead
export const config = { runtime: 'edge' }still inapi/cdn.ts,api/ping.ts,api/pr.tsx,api/auth/github/callback.ts, now that the route wrappers all declareruntime = 'nodejs'. Worth removing, and worth a line in the PR body about the CDN proxy — the hot path for every metadata read — moving off the edge to Node serverless. biome.jsonturns offa11y/useSemanticElementsrepo-wide to silence components not in this diff, and leaves two newnoImgElementwarnings unaddressed. Suppressing an a11y rule to make an unrelated migration pass lint deserves a sentence of justification.
This review was conducted using the review-pr-workflow skill.
Resolve the requested migration follow-ups while preserving the client-first application behavior.\n\n- Upgrade patched Next.js and align Motion package versions\n- Restore custom-container scroll resets and local CDN development reads\n- Remove duplicate routing and obsolete runtime configuration\n- Refresh repository guidance and narrow migration lint exceptions
murderteeth
left a comment
There was a problem hiding this comment.
Summary
Round 2, against 4f8effb. All five issues from the previous review are fixed, verified rather than taken on trust: next is 16.2.11; scroll restore lands the detail page at scrollTop: 0 (was 104); /cdn/* under next dev returns cache-control: no-store, confirming the local read; CLAUDE.md matches the code; the Vercel install command is documented. Lint is clean, bun run build and tsc --noEmit pass.
Three things left. The first two are in the new local-CDN branch and need fixing together.
Issues
1. readLocalCdn(path) is returned without await, so its errors bypass the handler's error handling — packages/app/api/cdn.ts:95
Line 95 is inside the try opened at line 80, but return somePromise resolves the async function after the try block exits, so the catch at line 110 never sees the rejection. Confirmed in isolation: async function f(){ try { return boom() } catch(e){ return 'CAUGHT' } } rejects; return await boom() is caught.
Effect: GET /cdn/vaults under next dev returns a bare framework 500 with an empty body (server log shows EISDIR at line 29). Production returns 404 for the same request.
- Change line 95 to
return await readLocalCdn(path). - Keep the ENOENT → 404 path and the 200 response headers unchanged.
- Done when: a non-ENOENT
readFilefailure is answered by this handler rather than escaping to Next.
2. Only ENOENT maps to 404, so directory paths error where production 404s — packages/app/api/cdn.ts:38-43
packages/cdn/vaults is a real directory and vaults passes the path guard, so EISDIR is reachable through an ordinary URL and gets rethrown.
Fix this together with #1, and in this order: the outer catch returns e?.message, which for an fs error contains the absolute filesystem path. It can't reach the client today only because bug #1 stops it. Fixing the await alone turns a latent path disclosure into a live one.
- Map
ENOENT,EISDIR,ENOTDIR,ENAMETOOLONGandEACCESto the 404 production returns, and replacee?.messagein the outer catch with a generic body, logging the real error server-side. - Done when:
GET /cdn/vaultsreturns 404 undernext dev, and no response body contains a rawnode:fserror string.
3. postcss is pinned to 8.5.15, which has a high-severity advisory — packages/app/package.json:39
GHSA-r28c-9q8g-f849 (HIGH, CVSS 7.5, path traversal via sourceMappingURL auto-loading) affects <= 8.5.17, patched in 8.5.18.
This isn't new — it's been in the PR since 61f9371 and should have been in the last review. It was raised then: the dependency lens flagged it, the independent verifier refuted it as unexploitable, and it was dropped. Both agreed to raise it this round. Same package, same version, same advisory; what changed is the review, not the code. Apologies for the late arrival.
Not exploitable here — PostCSS only processes this repo's own stylesheet at build time — but it's a direct dependency this PR adds and the fix is a patch bump.
- Bump
postcssto>= 8.5.18and refreshbun.lock. - Done when:
packages/app/package.jsonpins>= 8.5.18andbun install --frozen-lockfilesucceeds. - No action needed on
next@16.2.11's transitivepostcss@8.4.31(bun.lock:301) — same range, not fixable from here, better left to a future Next bump.
This review was conducted using the review-pr-workflow skill.
Superseded by the round-2 review against 4f8effb. All five items from this review were fixed and verified.
Keep development CDN failures inside the route handler and prevent filesystem details from reaching clients.\n\n- Await local reads and normalize expected filesystem failures to 404\n- Log unexpected failures server-side with a generic 500 response\n- Add focused local CDN regression tests\n- Upgrade direct PostCSS to the patched 8.5.18 release
Summary
Migrate the CMS application from Vite and React Router to the Next.js App Router while preserving its existing client-first UI, public URLs, GitHub OAuth flow, CDN behavior, and Bun workspace. Align shared dependency versions with the current yearn.fi application so the two frontends use the same framework baseline.
How to review
Start with
packages/app/package.json,packages/app/next.config.ts, and the newpackages/app/approute tree to understand the framework boundary. Then review the converted API route handlers and the client layout/navigation changes; the metadata fetching model intentionally remains client-side.Happy path: start the production build, navigate directly to each collection/detail/global route, refresh the page, sign in through GitHub OAuth, and exercise the ping/CDN/PR endpoints. In development,
/cdn/*reads directly frompackages/cdn; production continues to proxy commit-pinned jsDelivr content.Test plan
bun install --frozen-lockfilebun run --cwd packages/app tslintbun run lintbun run buildpackages/cdn/api/ping, production CDN reads/CORS, OAuth redirect handling, and PR error responsesnext startand checked for browser console or hydration errorsRisk / impact
This changes the application runtime and deployment entry point, including OAuth callbacks and API routing. Public environment variables move from
VITE_*toNEXT_PUBLIC_*; deployments must provide the renamed values. The CDN proxy now runs as a Node.js route handler: development reads local workspace data, while production retains the commit-pinned jsDelivr source and existing cache/CORS behavior.Confirmed Vercel project settings:
packages/appbun run buildbun install --frozen-lockfileRequired public deployment variables:
NEXT_PUBLIC_CDN_URLNEXT_PUBLIC_ASSETS_CDN_URLNEXT_PUBLIC_GITHUB_CLIENT_IDNEXT_PUBLIC_GITHUB_REDIRECT_URIframer-motionandmotion-domare intentionally synchronized at12.42.2; the earlier standalone Motion DOM pin is no longer needed as a separate build workaround. Biome's semantic-element exception is limited to the two legacy hover primitives whose wrapper semantics cannot change without altering their interaction contracts; native image warnings have targeted inline rationale.Rollback is a revert of this PR and restoration of the prior Vite build command.