Repository navigation
Conversation
Co-authored-by: Matthew Weeks <weeksling@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe PR introduces a comprehensive dark mode system to the Next.js application, including a theme toggle component, theme persistence via localStorage, CSS initialization script to prevent flash-on-load, global CSS variables for light/dark themes, and extends post discovery to support both Markdown and MDX file formats. Changes
Sequence DiagramssequenceDiagram
autonumber
participant Page Load
participant _document.js Script
participant localStorage
participant document.documentElement
participant CSS Engine
Page Load->>_document.js Script: Inject script before hydration
_document.js Script->>localStorage: Read 'theme' value
alt Theme stored or dark mode preferred
_document.js Script->>document.documentElement: Set data-theme="dark"
else Light mode
_document.js Script->>document.documentElement: Set data-theme="light"
end
_document.js Script->>CSS Engine: CSS variables apply (no flash)
Page Load->>Page Load: React hydrates with correct theme
sequenceDiagram
autonumber
participant User
participant ThemeToggle Component
participant document.documentElement
participant localStorage
participant CSS Engine
User->>ThemeToggle Component: Click theme toggle button
ThemeToggle Component->>ThemeToggle Component: Toggle dark state
ThemeToggle Component->>document.documentElement: Update data-theme attribute
ThemeToggle Component->>localStorage: Persist theme preference
document.documentElement->>CSS Engine: Trigger transition animation
CSS Engine->>User: Render updated theme colors
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- globals.css: define CSS custom properties for both themes (:root for light, [data-theme='dark'] for dark). Background, text, accent, border, and code block colors all theme-aware. - Home.module.css, Post.module.css: replace all hardcoded color values with var(--color-*) references. - components/ThemeToggle.js: fixed-position button in the upper right. Reads localStorage + prefers-color-scheme on mount, toggles data-theme attribute on <html> and persists to localStorage. Sun/moon character icons, no external deps. - pages/_document.js: inline blocking script sets data-theme before React hydrates, preventing flash of wrong theme on page load. - pages/_app.js: cleaned up stale @mdx-js/react import (removed in an earlier commit), re-enabled globals.css import, renders ThemeToggle on every page. https://claude.ai/code/session_014k68MZvxDeU9beQU8DLBrL
weeksling
marked this pull request as ready for review
April 13, 2026 17:09
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
next/lib/posts.js (1)
58-63:⚠️ Potential issue | 🟠 MajorGuard against
.md/.mdxslug collisions.Line 80 normalizes both
post.mdandpost.mdxto the same slug, while Line 85–Line 87 silently chooses the first match (.md). This creates ambiguous resolution and can emit duplicate static paths viagetAllSlugs()consumers.💡 Suggested fix (fail fast on duplicate slugs and ambiguous resolution)
const POST_EXTENSION_REGEX = /\.(md|mdx)$/ const POST_EXTENSIONS = ['.md', '.mdx'] @@ function stripPostExtension(filename) { return filename.replace(POST_EXTENSION_REGEX, '') } + +function assertUniqueSlugs(filenames) { + const slugCounts = new Map() + for (const filename of filenames) { + const slug = stripPostExtension(filename) + slugCounts.set(slug, (slugCounts.get(slug) || 0) + 1) + } + const duplicates = [...slugCounts.entries()] + .filter(([, count]) => count > 1) + .map(([slug]) => slug) + if (duplicates.length > 0) { + throw new Error( + `Duplicate post slugs found for multiple extensions: ${duplicates.join(', ')}` + ) + } +} @@ export function getAllPosts() { const dir = getPostsDir() const filenames = fs.readdirSync(dir).filter(isPostFilename) + assertUniqueSlugs(filenames) @@ export function getAllSlugs() { const dir = getPostsDir() - return fs + const filenames = fs .readdirSync(dir) .filter(isPostFilename) - .map(stripPostExtension) + assertUniqueSlugs(filenames) + return filenames.map(stripPostExtension) } @@ export function getPostBySlug(slug) { const dir = getPostsDir() - const filePath = POST_EXTENSIONS.map((ext) => path.join(dir, `${slug}${ext}`)).find( - (candidatePath) => fs.existsSync(candidatePath) - ) - if (!filePath) { + const matches = POST_EXTENSIONS + .map((ext) => path.join(dir, `${slug}${ext}`)) + .filter((candidatePath) => fs.existsSync(candidatePath)) + if (matches.length === 0) { throw new Error(`Post not found for slug "${slug}"`) } + if (matches.length > 1) { + throw new Error(`Ambiguous slug "${slug}": found both .md and .mdx`) + } + const filePath = matches[0]Also applies to: 79-80, 85-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@next/lib/posts.js` around lines 58 - 63, The code maps filenames -> post metadata and normalizes slugs (via toPostMeta/getAllSlugs) but silently lets post.md and post.mdx collapse to the same slug; change the mapping step that uses filenames and isPostFilename to detect duplicate normalized slugs after calling toPostMeta (or the slug-normalization function) and throw a clear error listing the conflicting filenames so the build fails fast on ambiguous resolution instead of picking the first file; update getAllSlugs to rely on the validated list (or explicitly reject duplicates) so consumers never receive duplicated/ambiguous slugs.
🧹 Nitpick comments (1)
next/components/ThemeToggle.js (1)
29-30: Set an explicit button type.Adding
type="button"prevents accidental form submission behavior if this component is reused inside a form context later.Proposed tweak
<button + type="button" onClick={toggle}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@next/components/ThemeToggle.js` around lines 29 - 30, The ThemeToggle component's button currently omits an explicit type which can cause accidental form submissions; update the button element in ThemeToggle (the <button onClick={toggle}> instance) to include type="button" so it will not submit parent forms when reused, ensuring the toggle handler remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@next/components/ThemeToggle.js`:
- Around line 6-15: Guard all access to browser-only APIs in the ThemeToggle
useEffect and any theme write paths (e.g., where localStorage.setItem is used)
by checking that window is defined and that window.matchMedia exists before
calling it, and wrap localStorage.getItem/setItem and matchMedia usage in a
try/catch to prevent exceptions in restricted environments; specifically update
the useEffect that calls localStorage.getItem, window.matchMedia, setDark and
document.documentElement.setAttribute to first confirm typeof window !==
'undefined' and typeof window.matchMedia === 'function' (or gracefully fall
back), and apply the same defensive checks around the theme-write logic
referenced at the other occurrence so reads/writes to localStorage and
document.documentElement are safely no-ops when unavailable.
In `@next/styles/globals.css`:
- Around line 27-28: The font-family declaration currently uses unquoted
multi-word family names which trips stylelint; update the font-family value in
the CSS rule (the font-family: ... declaration) to wrap multi-word font families
in quotes—e.g., "Segoe UI", "Fira Sans", "Droid Sans", "Helvetica Neue"—while
leaving single-word/family keywords like -apple-system, BlinkMacSystemFont,
Roboto, Oxygen, Ubuntu, Cantarell, and sans-serif unquoted.
---
Outside diff comments:
In `@next/lib/posts.js`:
- Around line 58-63: The code maps filenames -> post metadata and normalizes
slugs (via toPostMeta/getAllSlugs) but silently lets post.md and post.mdx
collapse to the same slug; change the mapping step that uses filenames and
isPostFilename to detect duplicate normalized slugs after calling toPostMeta (or
the slug-normalization function) and throw a clear error listing the conflicting
filenames so the build fails fast on ambiguous resolution instead of picking the
first file; update getAllSlugs to rely on the validated list (or explicitly
reject duplicates) so consumers never receive duplicated/ambiguous slugs.
---
Nitpick comments:
In `@next/components/ThemeToggle.js`:
- Around line 29-30: The ThemeToggle component's button currently omits an
explicit type which can cause accidental form submissions; update the button
element in ThemeToggle (the <button onClick={toggle}> instance) to include
type="button" so it will not submit parent forms when reused, ensuring the
toggle handler remains unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: bd20dbb3-0bd0-4688-b568-dd84bd68e845
📒 Files selected for processing (7)
next/components/ThemeToggle.jsnext/lib/posts.jsnext/pages/_app.jsnext/pages/_document.jsnext/styles/Home.module.cssnext/styles/Post.module.cssnext/styles/globals.css
Comment on lines
+6
to
+15
| useEffect(() => { | ||
| const stored = localStorage.getItem('theme') | ||
| const prefersDark = | ||
| stored === 'dark' || | ||
| (!stored && window.matchMedia('(prefers-color-scheme: dark)').matches) | ||
| setDark(prefersDark) | ||
| document.documentElement.setAttribute( | ||
| 'data-theme', | ||
| prefersDark ? 'dark' : 'light' | ||
| ) |
There was a problem hiding this comment.
Guard localStorage/matchMedia access to avoid runtime failures.
Theme init/write paths are currently unguarded. In restricted environments, this can throw and break mount-time logic.
Proposed hardening
useEffect(() => {
- const stored = localStorage.getItem('theme')
- const prefersDark =
- stored === 'dark' ||
- (!stored && window.matchMedia('(prefers-color-scheme: dark)').matches)
- setDark(prefersDark)
- document.documentElement.setAttribute(
- 'data-theme',
- prefersDark ? 'dark' : 'light'
- )
+ try {
+ const stored = localStorage.getItem('theme')
+ const supportsMatchMedia = typeof window.matchMedia === 'function'
+ const prefersDark =
+ stored === 'dark' ||
+ (!stored &&
+ supportsMatchMedia &&
+ window.matchMedia('(prefers-color-scheme: dark)').matches)
+ setDark(prefersDark)
+ document.documentElement.setAttribute(
+ 'data-theme',
+ prefersDark ? 'dark' : 'light'
+ )
+ } catch (_) {
+ setDark(false)
+ document.documentElement.setAttribute('data-theme', 'light')
+ }
}, [])
@@
- localStorage.setItem('theme', next ? 'dark' : 'light')
+ try {
+ localStorage.setItem('theme', next ? 'dark' : 'light')
+ } catch (_) {}Also applies to: 25-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@next/components/ThemeToggle.js` around lines 6 - 15, Guard all access to
browser-only APIs in the ThemeToggle useEffect and any theme write paths (e.g.,
where localStorage.setItem is used) by checking that window is defined and that
window.matchMedia exists before calling it, and wrap
localStorage.getItem/setItem and matchMedia usage in a try/catch to prevent
exceptions in restricted environments; specifically update the useEffect that
calls localStorage.getItem, window.matchMedia, setDark and
document.documentElement.setAttribute to first confirm typeof window !==
'undefined' and typeof window.matchMedia === 'function' (or gracefully fall
back), and apply the same defensive checks around the theme-write logic
referenced at the other occurrence so reads/writes to localStorage and
document.documentElement are safely no-ops when unavailable.
Comment on lines
27
to
28
| font-family: -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Oxygen, | ||
| Ubuntu, Cantarell, Fira Sans, Droid Sans, Helvetica Neue, sans-serif; |
There was a problem hiding this comment.
Fix font-family lint violations for quoted family names.
Stylelint will fail on multi-word font names without quotes.
Proposed fix
- font-family: -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Oxygen,
- Ubuntu, Cantarell, Fira Sans, Droid Sans, Helvetica Neue, sans-serif;
+ font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen,
+ Ubuntu, Cantarell, 'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif;📝 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.
Suggested change
| font-family: -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Oxygen, | |
| Ubuntu, Cantarell, Fira Sans, Droid Sans, Helvetica Neue, sans-serif; | |
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, | |
| Ubuntu, Cantarell, 'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif; |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 27-27: Expected quotes around "Segoe UI" (font-family-name-quotes)
(font-family-name-quotes)
[error] 28-28: Expected quotes around "Fira Sans" (font-family-name-quotes)
(font-family-name-quotes)
[error] 28-28: Expected quotes around "Droid Sans" (font-family-name-quotes)
(font-family-name-quotes)
[error] 28-28: Expected quotes around "Helvetica Neue" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@next/styles/globals.css` around lines 27 - 28, The font-family declaration
currently uses unquoted multi-word family names which trips stylelint; update
the font-family value in the CSS rule (the font-family: ... declaration) to wrap
multi-word font families in quotes—e.g., "Segoe UI", "Fira Sans", "Droid Sans",
"Helvetica Neue"—while leaving single-word/family keywords like -apple-system,
BlinkMacSystemFont, Roboto, Oxygen, Ubuntu, Cantarell, and sans-serif unquoted.
This branch had an error being deployed
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
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 kind of change does this PR introduce?
Bugfix.
This updates the Next blog post loader so it consistently handles both
.mdand.mdxfiles. Previously, post discovery and slug parsing only matched.md, which caused.mdxposts incontent/poststo be omitted from listings and static paths.Does this PR introduce a breaking change?
No.
Behavior is expanded to include
.mdxin addition to existing.mdsupport.What needs to be documented once your changes are merged?
No documentation updates are required, but contributors can now safely add posts with either
.mdor.mdxextensions.