Prevent crash from invalid theme class during initialization #3089
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds validated theme handling and persistence in the frontend provider and removes two dependencies from frontend/package.json: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/wrappers/provider.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/wrappers/provider.tsx
|
Thanks for the feedback. Agreed that forcing the theme breaks ModeToggle. I’ll update the PR to validate persisted theme values and preserve theme switching instead of forcing a single theme. |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please also fix all sonar wanrings
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/wrappers/provider.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/wrappers/provider.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/wrappers/provider.tsx
🧬 Code graph analysis (1)
frontend/src/wrappers/provider.tsx (1)
frontend/src/hooks/useDjangoSession.ts (1)
useDjangoSession(10-83)
🔇 Additional comments (3)
frontend/src/wrappers/provider.tsx (3)
11-12: LGTM! Clean type-safe theme definition.The const assertion and derived
ValidThemetype provide compile-time safety for theme values throughout the component.
44-49: NextThemesProvider configuration is correct.The configuration properly restricts allowed themes via
themes={VALID_THEMES}and uses a consistentstorageKeythat matchesgetValidTheme(). ThedefaultTheme={theme}provides a validated fallback when no theme is stored or after invalid values are cleaned up.Note:
enableSystemdefaults totruein next-themes, so the provider will detect and respect the user's system theme preference when no explicit theme is stored.
38-38: The code pattern is correct and follows next-themes best practices. TheReact.useStatelazy initialization properly callsgetValidThemeonly once on client mount. While a theoretical hydration difference exists (server returns'dark'whenwindowis undefined, client might readlocalStorage),next-themesis specifically designed to handle this scenario: it suppresses theme application during hydration and reconciles the state afterward without warnings.Additionally,
ModeToggle.tsximplements the standard hydration safety pattern with themountedstate check, preventing any issues when reading the current theme. No actual hydration mismatches are evident in the codebase.
|
I noticed a low-severity SonarCloud warning suggesting Please let me know if you’d prefer a different approach. |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
I don't think I understand the code and the issues it fixes.
Also have some doubts about specific code parts:
| if (stored) { | ||
| localStorage.removeItem('__nest_theme__') | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
I don't think it's a good idea.
frontend/src/wrappers/provider.tsx
Outdated
| // It ensures the session is synced with Django when the app starts. | ||
| // AppInitializer is mounted once. Its job is to call useDjangoSession(), | ||
| // which syncs the GitHub access token (stored in the NextAuth session) with the Django session. | ||
| const VALID_THEMES = ['dark', 'light'] as const |
There was a problem hiding this comment.
Don't remove existing comments.
frontend/src/wrappers/provider.tsx
Outdated
| // It ensures the session is synced with Django when the app starts. | ||
| // AppInitializer is mounted once. Its job is to call useDjangoSession(), | ||
| // which syncs the GitHub access token (stored in the NextAuth session) with the Django session. | ||
| const VALID_THEMES = ['dark', 'light'] as const |
There was a problem hiding this comment.
Why do you need const twice here?
|
Thanks for the detailed feedback - that helps. The issue this PR fixes is a runtime crash caused by next-themes applying a persisted theme value directly as a CSS class during initial render. If the stored value contains spaces (for example "Default Theme"), the browser throws an InvalidCharacterError and the app crashes before rendering. The cleanup logic only removes the persisted value when it’s invalid (i.e. not one of ['dark', 'light']). This allows next-themes to fall back to a safe default and prevents the crash. Valid user preferences are not touched. I agree the intent wasn’t clear enough from the code. I’ll:
Please let me know if you’d prefer a different approach (for example sanitizing instead of removing). I’m happy to adjust. |
|
Closing this PR since it contains multiple unrelated changes from an earlier branch. |
|
You should never close a PR unless you're asked to. PR is a "shell" for your code to be reviewed/merged. As an author you can do whatever you need w/ your branch linked to this PR. |
|
Thanks for clarifying, that makes sense - appreciate it. I closed the PR earlier because the branch had accumulated multiple unrelated commits from experimentation, and I didn’t want to risk merging unintended changes. I understand now that the PR can remain open while I clean up the branch history. I’ll rebase this PR onto a clean state and update it accordingly. |
SuyashJain17
force-pushed
the
fix-css-safe-theme
branch
from
1d329ce to
b1ad641
Compare
|
|
Removed the dependency cleanup commit so this PR focuses only on the theme initialization crash fix. |
| /** | ||
| * next-themes applies the persisted theme value directly as a CSS class | ||
| * during initial render. | ||
| * | ||
| * If the stored value contains spaces (e.g. "Default Theme"), the browser | ||
| * throws InvalidCharacterError and the app crashes before rendering. | ||
| * | ||
| * To prevent this, we validate the persisted value and remove it only | ||
| * if it is invalid, allowing next-themes to safely fall back to default. | ||
| */ |
There was a problem hiding this comment.
@SuyashJain17 can you provide steps how to replicate this issue you're trying to fix? 👀
Current default theme is set to "dark", so I'm not sure how it's possible to have it set to something like "Default Theme", unless user goes in and manually updates it in local storage.
Is that the scenario you're working with?
Even if so, I believe toggling the theme toggle again - would update the value.
However, even if we'd want to add extra validation, I believe the solution can be much much simpler - like a 1-2 lines change 🤔
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
There's also unresolved Sonar issue still - you're required to resolve them all before we review, no matter how high the priority is on them.
3 participants
Proposed change
Resolves #3082
This PR prevents a runtime crash caused by invalid theme values being applied as CSS class names during initial render.
next-themesapplies the persisted theme value directly to theclassattribute. If the stored value contains spaces (for example"Default Theme"), this results in anInvalidCharacterErrorand the app crashes before rendering.The fix validates persisted theme values and falls back safely to a CSS-safe default during initialization.
Checklist
make check-testlocally and all tests passed