Skip to content

Migrate CMS from Vite to Next.js - #142

Open
rossgalloway wants to merge 3 commits into
mainfrom
codex/migrate-to-next-js
Open

Migrate CMS from Vite to Next.js#142
rossgalloway wants to merge 3 commits into
mainfrom
codex/migrate-to-next-js

Conversation

@rossgalloway

@rossgalloway rossgalloway commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

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 new packages/app/app route 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 from packages/cdn; production continues to proxy commit-pinned jsDelivr content.

Test plan

  • Automated: bun install --frozen-lockfile
  • Automated: bun run --cwd packages/app tslint
  • Automated: bun run lint
  • Automated: bun run build
  • Automated: confirmed GitHub advisories do not affect the pinned Next.js version
  • Manual: smoke-tested direct navigation and refreshes for public and dynamic routes
  • Manual: verified custom-container scroll restoration from a scrolled collection to a detail page
  • Manual: verified local development CDN reads from packages/cdn
  • Manual: verified /api/ping, production CDN reads/CORS, OAuth redirect handling, and PR error responses
  • Manual: ran the production build with next start and checked for browser console or hydration errors

Risk / impact

This changes the application runtime and deployment entry point, including OAuth callbacks and API routing. Public environment variables move from VITE_* to NEXT_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:

  • Root directory: packages/app
  • Framework preset: Next.js
  • Build command: bun run build
  • Install command: bun install --frozen-lockfile

Required public deployment variables:

  • NEXT_PUBLIC_CDN_URL
  • NEXT_PUBLIC_ASSETS_CDN_URL
  • NEXT_PUBLIC_GITHUB_CLIENT_ID
  • NEXT_PUBLIC_GITHUB_REDIRECT_URI

framer-motion and motion-dom are intentionally synchronized at 12.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.

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
@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cms Ready Ready Preview Jul 31, 2026 10:28pm

Request Review

@socket-security

socket-security Bot commented Jul 23, 2026

Copy link
Copy Markdown

@socket-security

socket-security Bot commented Jul 23, 2026

Copy link
Copy Markdown

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.

View full report

@rossgalloway
rossgalloway marked this pull request as ready for review July 23, 2026 19:54
@rossgalloway
rossgalloway requested a review from murderteeth July 23, 2026 19:54

@murderteeth murderteeth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 advisoriespackages/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 next to 16.2.11 or later.
  • Done when: packages/app/package.json pins ≥16.2.11, bun install --frozen-lockfile succeeds, and bun run build passes.

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.json with just {"$schema": "https://openapi.vercel.sh/vercel.json", "installCommand": "bun install --frozen-lockfile"}. Don't restore the old /(.*)/ catch-all or duplicate the /cdn rewrite.
  • 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 navigationpackages/app/src/components/RestoreScroll.tsx:4

The component was migrated (useLocationusePathname) 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 /> inside src/routes/Layout.tsx above <Header />, or delete the file if the reset is intentionally dropped — don't leave it migrated-but-unreferenced. Keep the #main-scroll id and scrollableTarget="main-scroll" in CollectionList.tsx:108 working for infinite scroll.
  • Done when: from a scrolled-down /vaults, clicking through to a detail page renders it at the top, and grep -rn RestoreScroll packages/app shows either a render site or no file.

4. CLAUDE.md still documents the deleted Vite/Bun-server stackCLAUDE.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, add tslint, drop dev:server), architecture (App Router, API handlers under app/api/** on the same port), NEXT_PUBLIC_* names with the port-3000 callback, and repoint Key Files at app/layout.tsx and app/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.md returns nothing, and every path under "Key Files" exists.

5. Local dev no longer serves packages/cdn from diskpackages/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.ts that reads the local packages/cdn directory (Node runtime; the path is already validated against /^[a-zA-Z0-9/_.-]+$/ and ..), or, if the loss is intentional, delete the now-dead packages/app/cdn symlink and say so in the README. Production behavior must stay as-is: /cdn/*/api/cdn/* → jsdelivr pinned at VERCEL_GIT_COMMIT_SHA.
  • Done when: with bun dev running and a local edit to packages/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-dom and framer-motion are 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 moves framer-motion to 12.34.0 and motion-dom to 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 //vaults redirect. next.config.ts:6-14 and app/page.tsx:4 both do it; the config redirect is evaluated first, so app/page.tsx is unreachable (I confirmed / returns a 307 from the config). Keep one.
  • if (!mounted) return null in src/routes/Layout.tsx:14 disables 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-time sessionStorage.getItem in HeaderDraftCart.tsx:124 and the unsuspended useSearchParams() in GithubSignIn.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 in api/cdn.ts, api/ping.ts, api/pr.tsx, api/auth/github/callback.ts, now that the route wrappers all declare runtime = '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.json turns off a11y/useSemanticElements repo-wide to silence components not in this diff, and leaves two new noImgElement warnings 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 murderteeth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 handlingpackages/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 readFile failure is answered by this handler rather than escaping to Next.

2. Only ENOENT maps to 404, so directory paths error where production 404spackages/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, ENAMETOOLONG and EACCES to the 404 production returns, and replace e?.message in the outer catch with a generic body, logging the real error server-side.
  • Done when: GET /cdn/vaults returns 404 under next dev, and no response body contains a raw node:fs error string.

3. postcss is pinned to 8.5.15, which has a high-severity advisorypackages/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 postcss to >= 8.5.18 and refresh bun.lock.
  • Done when: packages/app/package.json pins >= 8.5.18 and bun install --frozen-lockfile succeeds.
  • No action needed on next@16.2.11's transitive postcss@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.

@murderteeth
murderteeth dismissed their stale review July 31, 2026 22:13

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants