Vail Platform Foundation: Convert Static Site to Next.js App Router#3
Conversation
❌ Deploy Preview for wurxx failed.
|
❌ Deploy Preview for wurx-can failed.
|
❌ Deploy Preview for wurx-otta failed.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
📝 Info: Static sitemap.xml will diverge from actual routes over time
The sitemap.xml is a manually maintained static file rather than being generated by Next.js's built-in sitemap support (via app/sitemap.ts). This means any future route additions will require manual sitemap updates, creating a risk of drift. For this foundation PR, the sitemap matches all declared routes correctly. A future ticket could migrate to the Next.js metadata API for automatic sitemap generation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (const term of bannedLegacyTerms) { | ||
| if (body.includes(term)) { | ||
| failures.push(`${file} contains banned legacy term: ${term}`); |
There was a problem hiding this comment.
🟡 Banned-term check regresses from case-insensitive to case-sensitive matching
The old check-site.js used case-insensitive regex patterns (/SummitLine/i, /Roofing/i, etc.) to catch legacy terms regardless of casing. The new code replaces these with String.includes(term), which is case-sensitive. This means variants like "roofing", "ROOFING", "summitline", or "Roof Repair" will no longer be caught, weakening the safety guardrail against legacy branding leaking into the codebase.
| for (const term of bannedLegacyTerms) { | |
| if (body.includes(term)) { | |
| failures.push(`${file} contains banned legacy term: ${term}`); | |
| for (const term of bannedLegacyTerms) { | |
| if (body.toLowerCase().includes(term.toLowerCase())) { | |
| failures.push(`${file} contains banned legacy term: ${term}`); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| <details className="group relative lg:hidden"> | ||
| <summary className="list-none rounded-full border border-forest/25 px-4 py-2 text-sm font-semibold text-forest marker:hidden"> | ||
| Menu | ||
| </summary> | ||
| <div className="absolute right-0 mt-3 w-[min(88vw,22rem)] rounded-3xl border border-ink/10 bg-white p-4 shadow-soft"> | ||
| <div className="grid gap-2"> | ||
| {publicRoutes.map((route) => ( | ||
| <Link key={route.href} href={route.href} className="rounded-2xl px-4 py-3 text-sm font-semibold text-ink hover:bg-oatmeal"> | ||
| {route.label} | ||
| </Link> | ||
| ))} | ||
| <div className="mt-2 grid grid-cols-2 gap-2"> | ||
| <Link href="/project-request" className="rounded-2xl bg-forest px-4 py-3 text-center text-sm font-semibold text-white"> | ||
| Start Project | ||
| </Link> | ||
| <Link href="/book-call" className="rounded-2xl border border-forest/25 px-4 py-3 text-center text-sm font-semibold text-forest"> | ||
| Book Call | ||
| </Link> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </details> |
There was a problem hiding this comment.
🚩 Mobile menu uses
Details
without close-on-navigate behaviorThe SiteHeader at components/site-header.tsx:30-51 uses a native <details>/<summary> element for the mobile menu. Since this is a server component (no "use client" directive), there's no JavaScript to close the menu when a user navigates to a new route via a <Link>. In Next.js App Router with client-side navigation, the <details open> attribute will persist across navigations, meaning the mobile menu stays open after clicking a link. This is a UX issue rather than a bug — it works functionally but feels broken on mobile. A future ticket should either add a client component wrapper or use a different disclosure pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (const file of fs.readdirSync(root, { recursive: true }).filter((name) => typeof name === "string" && /\.(ts|tsx|js|html|md|toml)$/.test(name))) { | ||
| if (file.includes("node_modules") || file.includes(".next") || file === "scripts/check-site.js") { | ||
| continue; | ||
| } | ||
| const body = read(file); | ||
| for (const term of bannedLegacyTerms) { | ||
| if (body.includes(term)) { | ||
| failures.push(`${file} contains banned legacy term: ${term}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Legacy static HTML files remain in the repo and are scanned by check-site.js
The old static HTML files (index.html, start-a-project.html, fix-list-builder.html, etc.) were not removed in this PR. The new check-site.js scans all .html files recursively for banned legacy terms via fs.readdirSync(root, { recursive: true }). Currently none of these old HTML files contain the banned terms (verified by grep), so the check passes. However, leaving stale HTML files alongside the Next.js app is confusing — they are superseded by Next.js routes and redirects but will still be served as static assets by Next.js if placed in public/ (they aren't, but the ambiguity is worth noting). The foundation report at docs/control/VAIL_PLATFORM_FOUNDATION_REPORT.md:150 acknowledges this explicitly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return [ | ||
| { | ||
| source: "/start-a-project", | ||
| destination: "/project-request", | ||
| permanent: true | ||
| }, | ||
| { | ||
| source: "/fix-list-builder", | ||
| destination: "/scope-builder", | ||
| permanent: true | ||
| }, | ||
| { | ||
| source: "/maintenance-plans", | ||
| destination: "/services/repairs-refreshes", | ||
| permanent: false | ||
| }, | ||
| { | ||
| source: "/inspection-report-repairs", | ||
| destination: "/services/pre-sale-improvements", | ||
| permanent: false | ||
| }, | ||
| { | ||
| source: "/renovations", | ||
| destination: "/services", | ||
| permanent: true | ||
| }, | ||
| { | ||
| source: "/thank-you", | ||
| destination: "/contact", | ||
| permanent: false | ||
| } | ||
| ]; |
There was a problem hiding this comment.
📝 Info: Redirect permanence is inconsistent across legacy routes
In next.config.ts:5-37, some legacy redirects use permanent: true (301) while others use permanent: false (307). Specifically /start-a-project, /fix-list-builder, and /renovations are permanent, while /maintenance-plans, /inspection-report-repairs, and /thank-you are temporary. This appears intentional — the first three are straightforward renamed routes, while the latter three map legacy concepts to different service categories where the mapping may change. Not a bug, but worth confirming the intent since permanent redirects are cached aggressively by browsers and search engines.
Was this helpful? React with 👍 or 👎 to provide feedback.
📝 WalkthroughWalkthroughThis PR migrates a static Netlify HTML site to a Next.js App Router platform foundation. It establishes build and styling infrastructure, introduces route definitions, builds a reusable component library, creates placeholder pages across multiple routes, and implements dynamic service routes with proper metadata generation. ChangesNext.js App Router Platform Foundation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/site-footer.tsx`:
- Around line 23-24: Replace the two hardcoded <Link> entries with links sourced
from the centralized route registry in lib/routes.ts: import the privacy and
terms route entries (e.g., constants or objects named like privacyRoute /
termsRoute or lookup by key) and render <Link href={privacyRoute.path}
...>Privacy</Link> and <Link href={termsRoute.path} ...>Terms</Link> (or use the
registry lookup method used elsewhere) so the footer uses the canonical route
definitions instead of hardcoded paths.
In `@lib/routes.ts`:
- Around line 42-50: The placeholderRoutes array is missing the "/how-it-works"
entry; update the exported constant placeholderRoutes to include "/how-it-works"
alongside the other strings so the route registry matches the placeholder page
set in the migration (modify the placeholderRoutes declaration to add the
"/how-it-works" string to the tuple).
In `@package.json`:
- Around line 13-30: package.json lists pinned dependencies/devDependencies but
the repository is missing a lockfile; run a fresh install to generate and commit
the lockfile (e.g., run npm install to produce package-lock.json or yarn install
to produce yarn.lock), ensure the generated lockfile is added to the repo
alongside package.json so transitive dependencies are pinned and then include
the lockfile in the commit that updates the "dependencies"/"devDependencies".
- Line 11: The "check" npm script currently runs "node scripts/check-site.js &&
npm run typecheck && npm run build" but omits linting; update the "check" script
in package.json to include the lint step (e.g., add "&& npm run lint" at the
appropriate place) so that lint runs as part of the main validation gate, and
confirm the "lint" npm script (npm script named "lint") exists and is correctly
defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b513fd4-2e58-4b67-aa84-2327f167bdb7
📒 Files selected for processing (30)
README.mdapp/book-call/page.tsxapp/callback/page.tsxapp/contact/page.tsxapp/globals.cssapp/how-it-works/page.tsxapp/layout.tsxapp/page.tsxapp/privacy/page.tsxapp/project-request/page.tsxapp/scope-builder/page.tsxapp/services/[slug]/page.tsxapp/services/page.tsxapp/terms/page.tsxcomponents/mobile-cta-bar.tsxcomponents/placeholder-page.tsxcomponents/site-footer.tsxcomponents/site-header.tsxdocs/control/VAIL_PLATFORM_FOUNDATION_REPORT.mdeslint.config.mjslib/routes.tsnetlify.tomlnext-env.d.tsnext.config.tspackage.jsonpostcss.config.jsscripts/check-site.jssitemap.xmltailwind.config.tstsconfig.json
📜 Review details
🧰 Additional context used
🪛 Stylelint (17.11.1)
app/globals.css
[error] 1-1: Unexpected unknown at-rule "@tailwind" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 2-2: Unexpected unknown at-rule "@tailwind" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 3-3: Unexpected unknown at-rule "@tailwind" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🔇 Additional comments (31)
lib/routes.ts (1)
1-40: LGTM!components/placeholder-page.tsx (1)
1-41: LGTM!components/site-footer.tsx (1)
1-22: LGTM!Also applies to: 25-40
components/site-header.tsx (1)
1-55: LGTM!components/mobile-cta-bar.tsx (1)
1-16: LGTM!app/page.tsx (1)
1-120: LGTM!app/book-call/page.tsx (1)
1-20: LGTM!app/callback/page.tsx (1)
1-20: LGTM!app/contact/page.tsx (1)
1-20: LGTM!app/how-it-works/page.tsx (1)
1-20: LGTM!app/privacy/page.tsx (1)
1-20: LGTM!app/project-request/page.tsx (1)
1-20: LGTM!app/scope-builder/page.tsx (1)
1-20: LGTM!app/terms/page.tsx (1)
1-20: LGTM!app/services/page.tsx (1)
1-29: LGTM!app/services/[slug]/page.tsx (1)
1-70: LGTM!next.config.ts (1)
1-41: LGTM!tailwind.config.ts (1)
1-24: LGTM!tsconfig.json (1)
1-28: LGTM!postcss.config.js (1)
1-7: LGTM!eslint.config.mjs (1)
1-15: LGTM!next-env.d.ts (1)
1-5: LGTM!netlify.toml (1)
1-7: LGTM!app/globals.css (2)
5-41: LGTM!
1-3: ⚡ Quick win<stylelint
@tailwindhandling in app/globals.css needs evidence from the repo. The current search only shows@tailwind base/components/utilitiesinapp/globals.cssand Tailwind/PostCSS config; it didn’t find Stylelint config or thescss/at-rule-no-unknown/ignoreAtRulesrule text. Add or adjust Stylelint config only if CI is actually running Stylelint with that rule and the file is included.app/layout.tsx (1)
1-33: LGTM!scripts/check-site.js (2)
5-83: LGTM!Also applies to: 98-120
86-96: ⚡ Quick winMissing input: Provide the original review comment inside
<review_comment>...</review_comment>(and any verification outputs you have), so I can rewrite it in the required format.docs/control/VAIL_PLATFORM_FOUNDATION_REPORT.md (1)
1-153: LGTM!README.md (1)
1-81: LGTM!sitemap.xml (1)
4-18: LGTM!
| <Link href="/privacy" className="hover:text-white">Privacy</Link> | ||
| <Link href="/terms" className="hover:text-white">Terms</Link> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove hardcoded legal links from the footer route list.
These links bypass the centralized route registry and can drift from route data over time. Prefer reading them from lib/routes.ts as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/site-footer.tsx` around lines 23 - 24, Replace the two hardcoded
<Link> entries with links sourced from the centralized route registry in
lib/routes.ts: import the privacy and terms route entries (e.g., constants or
objects named like privacyRoute / termsRoute or lookup by key) and render <Link
href={privacyRoute.path} ...>Privacy</Link> and <Link href={termsRoute.path}
...>Terms</Link> (or use the registry lookup method used elsewhere) so the
footer uses the canonical route definitions instead of hardcoded paths.
| export const placeholderRoutes = [ | ||
| "/project-request", | ||
| "/scope-builder", | ||
| "/book-call", | ||
| "/callback", | ||
| "/contact", | ||
| "/privacy", | ||
| "/terms" | ||
| ] as const; |
There was a problem hiding this comment.
Add the missing placeholder route for /how-it-works.
placeholderRoutes is missing /how-it-works, which makes the route registry inconsistent with the placeholder page set in this migration.
Suggested fix
export const placeholderRoutes = [
+ "/how-it-works",
"/project-request",
"/scope-builder",
"/book-call",
"/callback",
"/contact",
"/privacy",
"/terms"
] as const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const placeholderRoutes = [ | |
| "/project-request", | |
| "/scope-builder", | |
| "/book-call", | |
| "/callback", | |
| "/contact", | |
| "/privacy", | |
| "/terms" | |
| ] as const; | |
| export const placeholderRoutes = [ | |
| "/how-it-works", | |
| "/project-request", | |
| "/scope-builder", | |
| "/book-call", | |
| "/callback", | |
| "/contact", | |
| "/privacy", | |
| "/terms" | |
| ] as const; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/routes.ts` around lines 42 - 50, The placeholderRoutes array is missing
the "/how-it-works" entry; update the exported constant placeholderRoutes to
include "/how-it-works" alongside the other strings so the route registry
matches the placeholder page set in the migration (modify the placeholderRoutes
declaration to add the "/how-it-works" string to the tuple).
| "start": "next start", | ||
| "typecheck": "tsc --noEmit", | ||
| "lint": "eslint . --max-warnings=0", | ||
| "check": "node scripts/check-site.js && npm run typecheck && npm run build" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Include lint in the main validation gate.
check currently skips lint, so a single-command verification can pass with lint regressions.
Suggested update
- "check": "node scripts/check-site.js && npm run typecheck && npm run build"
+ "check": "node scripts/check-site.js && npm run typecheck && npm run lint && npm run build"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "check": "node scripts/check-site.js && npm run typecheck && npm run build" | |
| "check": "node scripts/check-site.js && npm run typecheck && npm run lint && npm run build" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 11, The "check" npm script currently runs "node
scripts/check-site.js && npm run typecheck && npm run build" but omits linting;
update the "check" script in package.json to include the lint step (e.g., add
"&& npm run lint" at the appropriate place) so that lint runs as part of the
main validation gate, and confirm the "lint" npm script (npm script named
"lint") exists and is correctly defined.
| "dependencies": { | ||
| "next": "15.3.4", | ||
| "react": "19.0.0", | ||
| "react-dom": "19.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@eslint/eslintrc": "3.3.1", | ||
| "@netlify/plugin-nextjs": "5.11.3", | ||
| "@types/node": "22.15.30", | ||
| "@types/react": "19.0.12", | ||
| "@types/react-dom": "19.0.4", | ||
| "autoprefixer": "10.4.21", | ||
| "eslint": "9.28.0", | ||
| "eslint-config-next": "15.3.4", | ||
| "postcss": "8.5.6", | ||
| "tailwindcss": "3.4.17", | ||
| "typescript": "5.8.3" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Commit the lockfile alongside these pinned dependencies.
Without package-lock.json, transitive dependency resolution can still drift between environments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 13 - 30, package.json lists pinned
dependencies/devDependencies but the repository is missing a lockfile; run a
fresh install to generate and commit the lockfile (e.g., run npm install to
produce package-lock.json or yarn install to produce yarn.lock), ensure the
generated lockfile is added to the repo alongside package.json so transitive
dependencies are pinned and then include the lockfile in the commit that updates
the "dependencies"/"devDependencies".
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a40cde74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,9 +1,19 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
After this migration the site is built from .next, and Next.js only serves static files from public/ (or metadata routes under app/). Keeping sitemap.xml at the repo root means /sitemap.xml will not be published, so crawlers lose the sitemap despite this file being updated. Move it to public/sitemap.xml or implement app/sitemap.ts so the route is actually served.
Useful? React with 👍 / 👎.
| publish = "." | ||
| command = "" | ||
| command = "npm run build" | ||
| publish = ".next" |
There was a problem hiding this comment.
Changing Netlify to publish .next stops serving root-level static files, and this commit does not add public/robots.txt or an app/robots.ts route. As a result, /robots.txt disappears in production, which removes the crawl policy and the sitemap discovery hint currently defined in the existing robots file.
Useful? React with 👍 / 👎.
| source: "/start-a-project", | ||
| destination: "/project-request", | ||
| permanent: true |
There was a problem hiding this comment.
Before this migration, the static site shipped concrete files like start-a-project.html, fix-list-builder.html, and thank-you.html, so those direct URLs were valid and may still be bookmarked or indexed. The new redirect table only matches extensionless paths (for example /start-a-project) and does not include .html variants, so these legacy links now return 404 after moving publish output to .next.
Useful? React with 👍 / 👎.
1 participant
Status
HOLD pending live verification.
Summary
Converts the Vail Renovations repo from a static Netlify HTML foundation into a Next.js App Router platform foundation with TypeScript, Tailwind configuration, public route placeholders, shared layout components, mobile CTAs, legacy route redirects and a platform foundation report.
What changed
@netlify/plugin-nextjs.docs/control/VAIL_PLATFORM_FOUNDATION_REPORT.md.Intentionally not built
Verification
Not run in this environment. The implementation environment could not install dependencies or run
npm run typecheck,npm run lint, ornpm run build.Required before merge:
GO/HOLD
HOLD until dependency install, typecheck, lint and build pass in CI or a local environment with npm registry access.
First blocker if HOLD
No package lock was generated and verification commands were not executed in this environment.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores