feat(routing): implement dynamic routing using sanity cms - #2
Open
srikishore5727 wants to merge 2 commits into
Open
srikishore5727 wants to merge 2 commits into
srikishore5727 wants to merge 2 commits into
Conversation
Change SummaryThis PR implements CMS-driven page routing by integrating Sanity with Next.js. It introduces a Page document schema registered in the Sanity schema index and a Next.js dynamic route that fetches page data by slug using GROQ, returning 404 when not found. File Changes
|
PR ScorecardScoreScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
|
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| export default async function Page({ params }: any) { | ||
| const { slug } = await params | ||
| // GROQ Query | ||
| const query = `*[_type == "page" && slug.current == $slug][0]` |
There was a problem hiding this comment.
Severity: 🟠 Major
Add automated coverage for the new CMS-driven routing (happy path + 404) by extracting fetch logic into a testable function and mocking client.fetch.
Suggested Change:
// Example direction:
// - move `PAGE_BY_SLUG_QUERY` + `client.fetch` into `lib/sanity/queries.ts` as `getPageBySlug(slug)`
// - unit test `getPageBySlug` by mocking `client.fetch`
// - optionally test the page component by asserting it calls `notFound()` when null is returned
PR OverviewPR Type: Feature Focus Areas for Architect Review
PR InsightsPotential PR Improvements
Strengths
|
|
Tip Need another review? Tag me and say rereview for re-analysis after you have fixed all the issues. @cw-pr-agent rereview |
Collaborator
|
@cw-pr-agent rereview |
1 similar comment
|
@cw-pr-agent rereview |
Change SummaryImplements Sanity-driven dynamic routing via the File Changes
Based on de57284...f8711bf |
PR ScorecardScoreScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
Issue NotesCode Correctness & Design Quality
Test Quality & Coverage
💬 Minor Issues (Nitpicks)Code Readability & Maintainability
Based on 6bce809...f8711bf |
Comment on lines
+13
to
+16
| const PAGE_BY_SLUG_QUERY = | ||
| '*[_type == "page" && slug.current == $slug][0]{title, description}' | ||
|
|
||
| export default async function Page({ params }: PageRouteProps) { |
There was a problem hiding this comment.
Severity: 🟠 Major
Add automated tests for CMS-driven routing (happy path + 404) to prevent regressions; simplest path is extracting getPageBySlug(slug) and unit testing it by mocking client.fetch.
Comment on lines
+4
to
+7
| type PageRouteProps = { | ||
| params: Promise<{ slug: string }> | ||
| } | ||
|
|
There was a problem hiding this comment.
Severity: 🟠 Major
params in Next.js App Router is an object, not a Promise—type it as { slug: string } and drop the await to avoid misleading types.
type PageRouteProps = { params: { slug: string } }
...
const { slug } = params
Comment on lines
+24
to
+28
| try { | ||
| page = await client.fetch<PageDoc | null>(PAGE_BY_SLUG_QUERY, { slug }) | ||
| } catch { | ||
| notFound() | ||
| } |
There was a problem hiding this comment.
Severity: 🟠 Major
Don’t convert fetch/permission/network errors into a 404—reserve notFound() for “no document”, and throw (or log + throw) on real failures so outages surface as 500s.
try {
page = await client.fetch<PageDoc | null>(PAGE_BY_SLUG_QUERY, { slug })
} catch (err) {
throw err
}
PR OverviewPR Type: Feature Focus Areas for Architect Review
Rereview ImpressionsProgress Since Last Review
New Issues Introduced (if any)
Remaining Concerns
PR InsightsPotential PR Improvements
PR Strengths
|
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
3 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.
What does this PR do?
title,slug,description,heroBanner)app/[slug]/page.tsxto render pages based on CMS slugstitlefield in the Page schemaWhat steps does your reviewer have to take to test this PR manually?
npm installto install dependenciesnpm run devand openhttp://localhost:3000http://localhost:3000/studioand verify Page appears in the left sidebarhomeabouthttp://localhost:3000/homehttp://localhost:3000/abouthome-pageand republishhttp://localhost:3000/home-pageworkshttp://localhost:3000/homereturns 404/app/home/page.tsxor/app/about/page.tsx.env.localPull Request standards checklist - Please check off
Testing checklist - Please check off
Definition of Done - Please check off
Outcome: The CMS now fully controls which pages exist and what their URLs are, with Next.js dynamically rendering content based on Sanity data.