Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Course Builder case-study metadata functions and server-only wrappers, migrates multiple pages from Sanity/GROQ to the Course Builder DB for case studies, updates the post schema to include Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Build Process
participant GS as getStaticProps
participant CB as Course Builder DB
participant MDX as MDX Serializer
participant Response as Static Response
Build->>GS: request case-study [slug]
GS->>CB: getCourseBuilderCaseStudy(slug)
CB-->>GS: CourseBuilderCaseStudy or null
alt found
GS->>GS: normalize props (title, subtitle, seo, coverImage, author)
GS->>MDX: serialize(caseStudy.body ?? '')
MDX-->>GS: MDX content
GS->>Response: return props + revalidate: 60
Response-->>Build: Static HTML (cached)
else not found
GS-->>Response: notFound: true
Response-->>Build: 404
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/schemas/post.ts (1)
4-11:⚠️ Potential issue | 🟠 MajorUpdate the remaining
PostTypeallowlist.
PostTypeSchemaaccepts'case-study'now, butsrc/lib/format-content-as-prompt.tsstill only allowsarticle,lesson,podcast,tip, andcourse. Any case study that flows through that formatter will still be rejected as invalid until the new type is added there too.♻️ Suggested follow-up
- !['article', 'lesson', 'podcast', 'tip', 'course'].includes( + !['article', 'lesson', 'podcast', 'tip', 'course', 'case-study'].includes(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/post.ts` around lines 4 - 11, PostTypeSchema now includes 'case-study' but the formatter function formatContentAsPrompt still validates against an older allowlist; update the validator/allowedTypes array (or the explicit literals) inside formatContentAsPrompt to include 'case-study' so the runtime validation matches PostTypeSchema and case-study posts are accepted; search for any variables/constants named allowedPostTypes, allowedTypes, or the literal union inside formatContentAsPrompt and add 'case-study' there (and update any related TypeScript types or zod checks if present).
🧹 Nitpick comments (2)
src/lib/get-course-builder-metadata.ts (1)
615-630: Move the shared case-study contract intosrc/schemas.
CourseBuilderCaseStudyis now a public shape consumed by this loader, the wrapper, and multiple pages. Defining it as an ad-hoc type in the data layer makes it harder to validate the runtime payload and easier for consumers to drift. ACourseBuilderCaseStudySchemainsrc/schemaswith an inferred type would fit the existing pattern better.As per coding guidelines, "Extract all Zod schemas and TypeScript types into src/schemas" and "Export inferred types from Zod schemas".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/get-course-builder-metadata.ts` around lines 615 - 630, The type CourseBuilderCaseStudy is defined ad-hoc in get-course-builder-metadata.ts; move this contract into a shared Zod schema file (create CourseBuilderCaseStudySchema in src/schemas) and export the inferred TypeScript type (e.g., export type CourseBuilderCaseStudy = z.infer<typeof CourseBuilderCaseStudySchema>); update get-course-builder-metadata.ts to import CourseBuilderCaseStudySchema (or the exported CourseBuilderCaseStudy type) and replace the local CourseBuilderCaseStudy declaration with the imported schema/type so runtime validation and a single canonical shape are used across the loader, wrapper, and pages.src/pages/case-studies/[slug].tsx (1)
271-274: Prefer author data from the fetched record over a hardcoded default.
getCourseBuilderCaseStudy()already returnsfeaturedInstructors, so hardcoding"Meg Cumby"here makes every dynamic case-study page render the same attribution regardless of content. Map the first instructor when present, or omit the author block when the data is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/case-studies/`[slug].tsx around lines 271 - 274, The current author const is hardcoded to "Meg Cumby"; instead use the fetched record's featuredInstructors from getCourseBuilderCaseStudy() — map the first instructor when present (e.g., set author.name = featuredInstructors[0].name and populate other AuthorResource fields if available) and fall back to omitting the author block (or setting author to undefined/null) when featuredInstructors is empty; update the code that renders the author block to handle the missing author value safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/get-course-builder-metadata.ts`:
- Around line 652-664: The single-item SQL query in
get-course-builder-metadata.ts selects case-study posts without checking publish
state, allowing drafts/archived slugs to be returned; update the WHERE clause on
the egghead_ContentResource alias (cr) to only match published items (e.g., add
AND cr.state = 'published' or, if state is stored in fields, AND
JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.state')) = 'published') so the query in
the block that selects cr.* for post/case-study slugs filters out unpublished
entries.
In `@src/pages/case-studies/index.tsx`:
- Around line 80-85: The code is collapsing loader failures (null) into an empty
list by using (caseStudies || []), which hides errors and can cache an empty
page; update the logic around getCourseBuilderCaseStudies() so you explicitly
detect a failure (caseStudies === null) and propagate it (throw or return an
error/Response) instead of treating it as an empty array, then only run the
filter when caseStudies is an actual Array (use Array.isArray(caseStudies) or
similar) before creating filteredCaseStudies; reference
getCourseBuilderCaseStudies, caseStudies, filteredCaseStudies, and
HIDDEN_CASE_STUDIES when making this change.
In `@src/pages/cloudflare.tsx`:
- Around line 244-250: The getStaticProps implementation is using unsafe
fallbacks for caseStudy.coverImage and caseStudy.publishedAt which contradict
the CaseStudyResource type and leads to runtime issues when the page reads
caseStudy.coverImage.url and formats publishedAt; update the getStaticProps
function to explicitly validate that caseStudy.coverImage and
caseStudy.publishedAt are present and valid (e.g., check
caseStudy.coverImage?.url and a parsable date for publishedAt) and if they are
missing return { notFound: true } or otherwise supply a safe, explicit fallback
handled by the renderer; remove the current "|| {}" and "|| ''" fallbacks in the
caseStudy mapping so the props reflect validated data and adjust the render path
only if you choose to support graceful fallbacks.
---
Outside diff comments:
In `@src/schemas/post.ts`:
- Around line 4-11: PostTypeSchema now includes 'case-study' but the formatter
function formatContentAsPrompt still validates against an older allowlist;
update the validator/allowedTypes array (or the explicit literals) inside
formatContentAsPrompt to include 'case-study' so the runtime validation matches
PostTypeSchema and case-study posts are accepted; search for any
variables/constants named allowedPostTypes, allowedTypes, or the literal union
inside formatContentAsPrompt and add 'case-study' there (and update any related
TypeScript types or zod checks if present).
---
Nitpick comments:
In `@src/lib/get-course-builder-metadata.ts`:
- Around line 615-630: The type CourseBuilderCaseStudy is defined ad-hoc in
get-course-builder-metadata.ts; move this contract into a shared Zod schema file
(create CourseBuilderCaseStudySchema in src/schemas) and export the inferred
TypeScript type (e.g., export type CourseBuilderCaseStudy = z.infer<typeof
CourseBuilderCaseStudySchema>); update get-course-builder-metadata.ts to import
CourseBuilderCaseStudySchema (or the exported CourseBuilderCaseStudy type) and
replace the local CourseBuilderCaseStudy declaration with the imported
schema/type so runtime validation and a single canonical shape are used across
the loader, wrapper, and pages.
In `@src/pages/case-studies/`[slug].tsx:
- Around line 271-274: The current author const is hardcoded to "Meg Cumby";
instead use the fetched record's featuredInstructors from
getCourseBuilderCaseStudy() — map the first instructor when present (e.g., set
author.name = featuredInstructors[0].name and populate other AuthorResource
fields if available) and fall back to omitting the author block (or setting
author to undefined/null) when featuredInstructors is empty; update the code
that renders the author block to handle the missing author value safely.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1640340a-315e-46f0-ad1c-70a6e72f005b
📒 Files selected for processing (7)
src/lib/get-course-builder-metadata.tssrc/lib/load-course-builder-metadata-wrapper.tssrc/pages/case-studies/[slug].tsxsrc/pages/case-studies/index.tsxsrc/pages/cloudflare.tsxsrc/schemas/post.tssrc/server/routers/feature-flag.ts
| const [rows] = await conn.execute<RowDataPacket[]>( | ||
| ` | ||
| SELECT cr.* | ||
| FROM egghead_ContentResource cr | ||
| WHERE cr.type = 'post' | ||
| AND JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.postType')) = 'case-study' | ||
| AND ( | ||
| cr.id = ? | ||
| OR JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.slug')) = ? | ||
| ) | ||
| LIMIT 1 | ||
| `, | ||
| [slug, slug], |
There was a problem hiding this comment.
Filter unpublished case studies in the single-item query.
This lookup matches any case-study row regardless of state. Because /case-studies/[slug] uses fallback: 'blocking' and /cloudflare calls this helper directly, a known draft or archived slug can still be rendered publicly.
🔒 Suggested fix
WHERE cr.type = 'post'
AND JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.postType')) = 'case-study'
+ AND JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.state')) = 'published'
AND (
cr.id = ?
OR JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.slug')) = ?
)📝 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.
| const [rows] = await conn.execute<RowDataPacket[]>( | |
| ` | |
| SELECT cr.* | |
| FROM egghead_ContentResource cr | |
| WHERE cr.type = 'post' | |
| AND JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.postType')) = 'case-study' | |
| AND ( | |
| cr.id = ? | |
| OR JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.slug')) = ? | |
| ) | |
| LIMIT 1 | |
| `, | |
| [slug, slug], | |
| const [rows] = await conn.execute<RowDataPacket[]>( | |
| ` | |
| SELECT cr.* | |
| FROM egghead_ContentResource cr | |
| WHERE cr.type = 'post' | |
| AND JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.postType')) = 'case-study' | |
| AND JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.state')) = 'published' | |
| AND ( | |
| cr.id = ? | |
| OR JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.slug')) = ? | |
| ) | |
| LIMIT 1 | |
| `, | |
| [slug, slug], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/get-course-builder-metadata.ts` around lines 652 - 664, The
single-item SQL query in get-course-builder-metadata.ts selects case-study posts
without checking publish state, allowing drafts/archived slugs to be returned;
update the WHERE clause on the egghead_ContentResource alias (cr) to only match
published items (e.g., add AND cr.state = 'published' or, if state is stored in
fields, AND JSON_UNQUOTE(JSON_EXTRACT(cr.fields, '$.state')) = 'published') so
the query in the block that selects cr.* for post/case-study slugs filters out
unpublished entries.
| const caseStudies = await getCourseBuilderCaseStudies() | ||
|
|
||
| // Filter out hidden case studies (e.g. cloudflare) | ||
| const filteredCaseStudies = (caseStudies || []).filter( | ||
| (cs) => !HIDDEN_CASE_STUDIES.includes(cs.slug), | ||
| ) |
There was a problem hiding this comment.
Don't collapse loader failures into an empty list.
getCourseBuilderCaseStudies() currently returns null for loader failures as well as "no rows". (caseStudies || []) erases that distinction and can cache a blank /case-studies page during ISR. This page needs an explicit success signal for the empty state instead of treating every null as "no content".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/case-studies/index.tsx` around lines 80 - 85, The code is
collapsing loader failures (null) into an empty list by using (caseStudies ||
[]), which hides errors and can cache an empty page; update the logic around
getCourseBuilderCaseStudies() so you explicitly detect a failure (caseStudies
=== null) and propagate it (throw or return an error/Response) instead of
treating it as an empty array, then only run the filter when caseStudies is an
actual Array (use Array.isArray(caseStudies) or similar) before creating
filteredCaseStudies; reference getCourseBuilderCaseStudies, caseStudies,
filteredCaseStudies, and HIDDEN_CASE_STUDIES when making this change.
| caseStudy: { | ||
| title: caseStudy.title, | ||
| subTitle: caseStudy.subtitle || '', | ||
| author: {name: 'Meg Cumby'}, | ||
| seo: caseStudy.seo || {}, | ||
| coverImage: caseStudy.coverImage || {}, | ||
| publishedAt: caseStudy.publishedAt || '', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '70,87p' src/pages/cloudflare.tsx
printf '\n---\n'
sed -n '244,251p' src/pages/cloudflare.tsxRepository: skillrecordings/egghead-next
Length of output: 1135
🏁 Script executed:
#!/bin/bash
# Get the full getStaticProps function to understand the complete context
rg -A 40 "export.*getStaticProps" src/pages/cloudflare.tsx | head -80Repository: skillrecordings/egghead-next
Length of output: 922
🏁 Script executed:
#!/bin/bash
# Search for caseStudy type definition or interface
rg "caseStudy.*:" src/pages/cloudflare.tsx | head -20Repository: skillrecordings/egghead-next
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Check if there's any type definition imported or defined for caseStudy
rg -B 5 "interface.*[Cc]aseStudy|type.*[Cc]aseStudy" src/pages/cloudflare.tsxRepository: skillrecordings/egghead-next
Length of output: 338
🏁 Script executed:
#!/bin/bash
# Find CaseStudyResource type definition
find src -name "*case-stud*" -type f | head -10Repository: skillrecordings/egghead-next
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Search for getCourseBuilderCaseStudy function
rg "getCourseBuilderCaseStudy" src --type ts --type tsx | head -10Repository: skillrecordings/egghead-next
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for getCourseBuilderCaseStudy function definition
rg "getCourseBuilderCaseStudy" src --type=ts -A 10 | head -40Repository: skillrecordings/egghead-next
Length of output: 2072
🏁 Script executed:
#!/bin/bash
# Search for CaseStudyResource type or interface definition
rg "CaseStudyResource" src -A 5 | head -50Repository: skillrecordings/egghead-next
Length of output: 1541
🏁 Script executed:
#!/bin/bash
# List files in pages directory to find case-studies
ls -la src/pages/ | grep -i caseRepository: skillrecordings/egghead-next
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Get the full CaseStudyResource type definition
rg -A 15 "export type CaseStudyResource" src/pages/case-studies/[slug].tsxRepository: skillrecordings/egghead-next
Length of output: 361
🏁 Script executed:
#!/bin/bash
# Find getCourseBuilderCaseStudy function definition in lib
rg -A 20 "export.*getCourseBuilderCaseStudy|function getCourseBuilderCaseStudy" src/libRepository: skillrecordings/egghead-next
Length of output: 1495
Validate required hero fields before returning props.
CaseStudyResource type requires coverImage and publishedAt as non-optional fields, but getStaticProps applies unsafe fallbacks (|| {} and || ''). The render path at lines 70–87 directly accesses caseStudy.coverImage.url and calls new Date(publishedAt).toLocaleDateString() without guards, which will produce undefined for the image source and "Invalid Date" in the UI when these fields are missing or falsy.
Add a guard in getStaticProps to ensure both fields are present before returning props, or handle missing data gracefully in the render path:
Suggested fix
if (!caseStudy) {
return {notFound: true}
}
+ if (!caseStudy.coverImage?.url || !caseStudy.publishedAt) {
+ return {notFound: true}
+ }
const mdxSource = await serialize(caseStudy.body ?? '', {Then remove the unsafe fallbacks in the props mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/cloudflare.tsx` around lines 244 - 250, The getStaticProps
implementation is using unsafe fallbacks for caseStudy.coverImage and
caseStudy.publishedAt which contradict the CaseStudyResource type and leads to
runtime issues when the page reads caseStudy.coverImage.url and formats
publishedAt; update the getStaticProps function to explicitly validate that
caseStudy.coverImage and caseStudy.publishedAt are present and valid (e.g.,
check caseStudy.coverImage?.url and a parsable date for publishedAt) and if they
are missing return { notFound: true } or otherwise supply a safe, explicit
fallback handled by the renderer; remove the current "|| {}" and "|| ''"
fallbacks in the caseStudy mapping so the props reflect validated data and
adjust the render path only if you choose to support graceful fallbacks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/[post].tsx (1)
40-42: Consider co-locating this constant with related routing logic or documenting its usage.The exclusion list will likely grow as more pages are migrated to dedicated static routes. For maintainability, consider adding a brief comment explaining which pages correspond to each excluded slug, or extract this to a shared config if other routes need the same exclusions.
📝 Example with documentation
// Slugs that have dedicated static pages and must not be generated by this dynamic route -const EXCLUDED_POST_SLUGS = ['cloudflare'] +const EXCLUDED_POST_SLUGS = [ + 'cloudflare', // src/pages/cloudflare.tsx - Cloudflare case study +]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/`[post].tsx around lines 40 - 42, The EXCLUDED_POST_SLUGS constant (used to prevent dynamic generation for slugs like 'cloudflare') should be either documented in-place with a brief comment mapping each slug to its dedicated static page and rationale, or extracted to a shared routing config (e.g., export const EXCLUDED_POST_SLUGS from a central routes/config module) so other route handlers can import the same list; update the [post] page to import the constant from the shared module (or add the explanatory comment above EXCLUDED_POST_SLUGS), and update any other routing code that needs the same exclusions to use that shared export to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/`[post].tsx:
- Around line 40-50: getStaticPaths is currently letting case-study slugs into
the dynamic [post] route because getAllPostSlugs() returns all posts without
filtering by postType; update the data layer so getAllPostSlugs() only returns
posts with postType === 'post' (or add a postType filter arg and call it from
getStaticPaths), ensuring case-study slugs are not included in the paths (so
EXCLUDED_POST_SLUGS is no longer the only protection); reference getAllPostSlugs
and getStaticPaths when making the change.
---
Nitpick comments:
In `@src/pages/`[post].tsx:
- Around line 40-42: The EXCLUDED_POST_SLUGS constant (used to prevent dynamic
generation for slugs like 'cloudflare') should be either documented in-place
with a brief comment mapping each slug to its dedicated static page and
rationale, or extracted to a shared routing config (e.g., export const
EXCLUDED_POST_SLUGS from a central routes/config module) so other route handlers
can import the same list; update the [post] page to import the constant from the
shared module (or add the explanatory comment above EXCLUDED_POST_SLUGS), and
update any other routing code that needs the same exclusions to use that shared
export to avoid duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| // Slugs that have dedicated static pages and must not be generated by this dynamic route | ||
| const EXCLUDED_POST_SLUGS = ['cloudflare'] | ||
|
|
||
| export const getStaticPaths: GetStaticPaths = async () => { | ||
| const postSlugs = await getAllPostSlugs() | ||
| return { | ||
| paths: postSlugs.map(({slug}) => ({ | ||
| params: {post: slug}, | ||
| })), | ||
| paths: postSlugs | ||
| .filter(({slug}) => !EXCLUDED_POST_SLUGS.includes(slug)) | ||
| .map(({slug}) => ({ | ||
| params: {post: slug}, | ||
| })), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if getAllPostSlugs filters by postType and verify case-study handling
echo "=== Checking getAllPostSlugs implementation ==="
rg -n -A 20 'getAllPostSlugs' src/lib/posts
echo ""
echo "=== Checking for case-study postType handling in getStaticProps ==="
rg -n -C 5 'case-study' src/pages/\[post\].tsx || echo "No case-study handling found"
echo ""
echo "=== Checking case-studies route implementation ==="
fd -t f '\[slug\].tsx' src/pages/case-studies --exec cat {}Repository: skillrecordings/egghead-next
Length of output: 9073
🏁 Script executed:
cat -n src/pages/\[post\].tsx | head -150Repository: skillrecordings/egghead-next
Length of output: 5797
Add case-study filtering to prevent routing conflicts.
The getStaticPaths filter currently excludes only 'cloudflare', but getAllPostSlugs() returns all posts with type='post' without filtering by postType. After the schema expansion to include 'case-study', case-study slugs will generate static paths in this route, conflicting with the dedicated /case-studies/[slug] route. While course redirects are handled (lines 127-135), case-study posts have no equivalent handling and will render incorrectly.
Choose one approach:
- Filter by
postTypeingetAllPostSlugs()to exclude case-studies - Add redirect logic for case-studies (similar to course handling)
- Return
notFound: trueingetStaticPropswhenpost.fields.postType === 'case-study'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/`[post].tsx around lines 40 - 50, getStaticPaths is currently
letting case-study slugs into the dynamic [post] route because getAllPostSlugs()
returns all posts without filtering by postType; update the data layer so
getAllPostSlugs() only returns posts with postType === 'post' (or add a postType
filter arg and call it from getStaticPaths), ensuring case-study slugs are not
included in the paths (so EXCLUDED_POST_SLUGS is no longer the only protection);
reference getAllPostSlugs and getStaticPaths when making the change.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes