Refactor to route groups and server/client component split#294
Refactor to route groups and server/client component split#294bryceerobertson merged 16 commits intomainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…onal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add poster map page for printing physical posters
jakkdl
left a comment
There was a problem hiding this comment.
- Massive code duplication - server pages vs API routes
Every page now has ~100 lines of nearly identical Airtable fetching code duplicated
between:
- src/app/(main)/advisors/page.tsx (lines 27-89)
- src/app/api/advisors/route.ts
And the same pattern for 8 other pages. This is ~800+ lines of duplicated Airtable logic.
that... kinda explains the size of the PR 🙃
Addresses PR #294 review feedback about ~800+ lines of duplicated Airtable fetching logic between server pages and API routes. Each resource's fetch function and type now live in a single shared module, imported by both the page and the API route. Also fixes 5 broken API routes that had placeholder TABLE_IDs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move the ~40 lines of identical Airtable fetch boilerplate (credential check, URL construction, pagination loop, single retry) into a shared fetchAirtableRecords() function in src/lib/data/airtable.ts. Each data module now only contains its table/view IDs, type definitions, and transform logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All 9 API routes incorrectly treated zero records as a server error. Empty results are legitimate (e.g. no published jobs), so always return 200 with the data array. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove lastModified (always null, never read by any page) from advisors, funding, media-channels, projects, and self-study. Remove lastModified and the !Date it closes field from jobs (misleadingly named, unused). Remove Short URL from map's fetch list (fetched but never mapped to output). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflicts: - API routes: keep data module imports + jsonWithCache from main - Page files: keep (main)/ route group versions, delete old locations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This comment was written by Claude Code, not Bryce. These commits address @jakkdl's review feedback about the ~800+ lines of duplicated Airtable fetching logic across the 9 server pages and their corresponding API routes. The core fix: shared data modules. A new The API routes are now ~15 lines each (import, call, empty-result check, return), using Dead/misleading fields also removed: |
These tables are never empty, so zero results indicates a fetch failure (e.g. missing credentials, Airtable API error). The 500 is correct. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@jakkdl hopefully this did it 🤞🏼 I used your method of getting Claude Code to check itself |
8f1fe6e to
946bbc3
Compare
Applied content updates from main (featured card changes, ContributeButtons props, funding newsletter link, media-channels card swap) to the new (main)/ route group locations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Did some more fundamental reviewing with Claude as to what this PR is trying to achieve, and if it's going at it in the right way. It's somewhat unclear if the server/client component split is worth it, I think we wouldn't have much issues on airtable's end with their API caching, and there's a decent complexity cost in terms of doubling the number of files that has to be maintained. But LLM's will probably be able to handle it fine enough. The route groups seems less worth it when we only have a single weird page that doesn't want header/footer. I don't like that determining a big structural decision - I think we're better off with some small CSS magic or whatever on that page to hide the header/footer. I'll push a commit with some changes~ |
Move all pages out of (main)/ and (standalone)/ route groups. This is a pure rename commit - no code changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create LayoutShell client component to conditionally render nav/footer - poster-map now renders without nav/footer via pathname check - Fix type imports to use @/lib/data/* directly instead of API routes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- advisors: 80000_Hours_logo2.png → 80000-hours.png - advisors: dc409ec6a6a8a2083408f6dffc3f80c2.png → ai-safety-quest.png - funding: CG-LOGO.webp → coefficient-giving.webp - self-study: 2.png → alignment-forum.png - self-study: download-2.svg → blue-dot-impact.svg 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
jakkdl
left a comment
There was a problem hiding this comment.
I think we've collectively hammered Claude enough now that we can somewhat decently trust that this PR is good.
Replaces part of #288.
(main)route group with shared Navigation + Footer layout(standalone)route group for pages that need no nav/footer (used by poster map in a follow-up PR)absolute→fixed)Why: Server components fetch data on the server before sending the page to the browser — this means faster page loads, better SEO, and less JavaScript shipped to users. The route group structure lets different pages have different layouts (main site pages get nav + footer, while standalone pages like the poster map get nothing).
🤖 Generated with Claude Code