Fixed header Nest logo adaption to theme(light/dark)#4374
Fixed header Nest logo adaption to theme(light/dark)#4374pranav-871 wants to merge 1 commit intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughThe PR addresses a navbar logo visibility issue by implementing theme-aware logo selection in the Header component. The Header now uses the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 4/5
- This PR looks safe to merge overall, with only a minor user-facing issue reported and no clear functional breakage.
- In
frontend/src/components/Header.tsx, deriving the logo fromresolvedThemeduring SSR can cause a light-to-dark logo swap on hydration for dark-theme users, leading to a brief visual flicker. - Given the moderate confidence and low-to-mild severity (4/10), this is a polish/regression risk rather than a merge-blocking defect.
- Pay close attention to
frontend/src/components/Header.tsx- SSR theme resolution can produce a hydration-time logo flicker for dark mode users.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/Header.tsx">
<violation number="1" location="frontend/src/components/Header.tsx:28">
P2: Theme-dependent logo is computed from `resolvedTheme` during SSR/initial render; next-themes returns `undefined` on the server, so this renders the light logo and then swaps on the client for dark users, causing flicker/hydration mismatch. Guard theme-dependent rendering until mounted or use a client-only render for the logo.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/pages/Header.test.tsx (1)
257-267:⚠️ Potential issue | 🟡 MinorTest name is inconsistent with assertion; test relies on implicit undefined behavior.
The test is named
'renders logo_dark.png image in both desktop and mobile header'but assertslogo_light.png. Additionally, per the context fromjest.setup.ts, the globaluseThememock returns{ theme: 'light', setTheme: jest.fn() }withoutresolvedTheme. This causesresolvedThemeto beundefinedin tests, which makes the logo default tologo_light.png- but this is accidental, not intentional.To properly test the theme-based logo behavior:
🔧 Suggested fix: rename test and add explicit theme testing
- it('renders logo_dark.png image in both desktop and mobile header', () => { + it('renders logo_light.png when theme is not dark', () => { renderWithSession(<Header isGitHubAuthEnabled />) const logoImages = screen.getAllByRole('img', { name: /owasp logo/i }) expect(logoImages.length).toBe(2) for (const logo of logoImages) { expect(logo).toHaveAttribute('src', '/img/logo_light.png') expect(logo).toBeInTheDocument() } })Consider adding a test for dark theme by mocking
resolvedTheme:it('renders logo_dark.png when theme is dark', () => { const { useTheme } = jest.requireMock('next-themes') useTheme.mockReturnValue({ resolvedTheme: 'dark', setTheme: jest.fn() }) renderWithSession(<Header isGitHubAuthEnabled />) const logoImages = screen.getAllByRole('img', { name: /owasp logo/i }) for (const logo of logoImages) { expect(logo).toHaveAttribute('src', '/img/logo_dark.png') } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/Header.test.tsx` around lines 257 - 267, The test titled 'renders logo_dark.png image in both desktop and mobile header' is misleading and relies on undefined resolvedTheme; update the existing test for Header to explicitly mock next-themes' useTheme to return { resolvedTheme: 'light', setTheme: jest.fn() } and rename it to reflect that it expects '/img/logo_light.png'; then add a new test that mocks useTheme to return { resolvedTheme: 'dark', setTheme: jest.fn() } and asserts both Header img elements have src '/img/logo_dark.png' so behavior for Header and useTheme is deterministic.
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ScrollToTop.test.tsx (1)
27-35: Remove the deadglobalThisscroll setup — test should use onlywindowsince the component does.The
ScrollToTopcomponent readswindow.scrollYand attaches its listener towindow. TheglobalThiscode at lines 27–28 is dead code; only thewindowdispatch triggers the component's listener. Additionally, the two different scroll values (400 vs 500) are unnecessary.Simplify to:
♻️ Refactor
test('The button should become visible after scrolling past the threshold', async () => { const { getByLabelText } = render(<ScrollToTop />) const button = getByLabelText(/scroll to top/i) - Object.defineProperty(globalThis, 'scrollY', { value: 400, writable: true }) - globalThis.dispatchEvent(new Event('scroll')) - Object.defineProperty(window, 'scrollY', { - value: 500, + value: 400, writable: true, }) window.dispatchEvent(new Event('scroll'))Note: Other tests in this file still use
globalThis-only scroll simulation (lines 47, 63, 71, 87, 104, 108, 116). If this fix was necessary to make the test pass, verify whether those tests need the same adjustment or if they're passing incorrectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ScrollToTop.test.tsx` around lines 27 - 35, Remove the dead globalThis scroll setup and use window consistently in the ScrollToTop unit test: delete the Object.defineProperty(globalThis, 'scrollY', ...) and its dispatch, convert the window block to the single necessary action (set window.scrollY to one value, e.g., 500, and dispatch new Event('scroll')) so the component's window listener is actually exercised; also scan other tests in this file that simulate scroll via globalThis (lines noted in review) and update them to use window if they exercise the ScrollToTop component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/Header.tsx`:
- Around line 26-28: The logoSrc is computed from resolvedTheme immediately,
causing a hydration mismatch; add a mounted-check state (e.g., isMounted via
useState and set true in useEffect) and only compute or render logoSrc after
isMounted is true (or fall back to null/placeholder until mounted) so that
useTheme().resolvedTheme is stable on the client before choosing between
'/img/logo_dark.png' and '/img/logo_light.png'; update references to
resolvedTheme and logoSrc in the Header component to honor isMounted.
---
Outside diff comments:
In `@frontend/__tests__/unit/pages/Header.test.tsx`:
- Around line 257-267: The test titled 'renders logo_dark.png image in both
desktop and mobile header' is misleading and relies on undefined resolvedTheme;
update the existing test for Header to explicitly mock next-themes' useTheme to
return { resolvedTheme: 'light', setTheme: jest.fn() } and rename it to reflect
that it expects '/img/logo_light.png'; then add a new test that mocks useTheme
to return { resolvedTheme: 'dark', setTheme: jest.fn() } and asserts both Header
img elements have src '/img/logo_dark.png' so behavior for Header and useTheme
is deterministic.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/ScrollToTop.test.tsx`:
- Around line 27-35: Remove the dead globalThis scroll setup and use window
consistently in the ScrollToTop unit test: delete the
Object.defineProperty(globalThis, 'scrollY', ...) and its dispatch, convert the
window block to the single necessary action (set window.scrollY to one value,
e.g., 500, and dispatch new Event('scroll')) so the component's window listener
is actually exercised; also scan other tests in this file that simulate scroll
via globalThis (lines noted in review) and update them to use window if they
exercise the ScrollToTop component.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8480b194-7851-460b-97b6-e3e72caa11ef
📒 Files selected for processing (3)
frontend/__tests__/unit/components/ScrollToTop.test.tsxfrontend/__tests__/unit/pages/Header.test.tsxfrontend/src/components/Header.tsx
| const { resolvedTheme } = useTheme() | ||
|
|
||
| const logoSrc = resolvedTheme === 'dark' ? '/img/logo_dark.png' : '/img/logo_light.png' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
next-themes resolvedTheme undefined hydration mismatch
💡 Result:
The "next-themes resolvedTheme undefined hydration mismatch" error occurs because resolvedTheme from useTheme returns undefined during server-side rendering (SSR) in Next.js. This creates a mismatch between server-rendered HTML (where theme-dependent UI renders with undefined/default values) and client-side hydration (where localStorage/system preference resolves the theme). This is expected behavior documented in next-themes README: theme values like resolvedTheme are undefined on the server since localStorage and prefers-color-scheme are unavailable. Rendering UI based on them directly causes hydration warnings/errors. ## Primary Fix: Client-Only Rendering for Theme-Dependent UI Delay rendering components using resolvedTheme until client-side mount using useState + useEffect: tsx 'use client'; import { useTheme } from 'next-themes'; import { useEffect, useState } from 'react'; function ThemedComponent { const { resolvedTheme, setTheme } = useTheme; const [mounted, setMounted] = useState(false); useEffect( => { setMounted(true); }, []); if (!mounted) { return
Citations:
- 1: https://registry.npmjs.org/next-themes
- 2: https://www.npmjs.com/package/next-themes
- 3: https://github.com/pacocoursey/next-themes
- 4: https://stackoverflow.com/questions/77026759/using-next-themes-for-dark-mode-generates-hydration-failed-error
- 5: [Bug]: Hydration failed pacocoursey/next-themes#317
- 6: useTheme make react hydration error in next.js 13 pacocoursey/next-themes#153
- 7: [Bug]: Example in README > Examples > Avoid Hydration Mismatch > Images does not work on initial load pacocoursey/next-themes#311
🏁 Script executed:
find . -name "Header.tsx" -type fRepository: OWASP/Nest
Length of output: 90
🏁 Script executed:
find . -name "layout.tsx" -o -name "layout.ts" | head -5Repository: OWASP/Nest
Length of output: 290
🏁 Script executed:
find . -name "_app.tsx" -o -name "_app.ts" -o -name "app.tsx" | head -5Repository: OWASP/Nest
Length of output: 36
🏁 Script executed:
cat -n ./frontend/src/components/Header.tsxRepository: OWASP/Nest
Length of output: 11748
🏁 Script executed:
find . -name "layout.tsx" -path "*/src/app/*" | grep -E "src/app/layout\.tsx|src/app/\[.*\]/layout\.tsx" | head -1Repository: OWASP/Nest
Length of output: 83
🏁 Script executed:
find . -path "*/src/app/layout.tsx" -type fRepository: OWASP/Nest
Length of output: 83
🏁 Script executed:
cat -n ./frontend/src/app/layout.tsxRepository: OWASP/Nest
Length of output: 3338
Add mounted state check before calculating logoSrc to prevent hydration mismatch.
The resolvedTheme from useTheme() is undefined until the theme resolves from localStorage on the client. Currently, logoSrc is calculated immediately (defaulting to light logo), which causes:
- Server renders with light logo
- Client hydrates with light logo
- Theme resolves to dark → Image switches to dark logo → hydration mismatch and flash of wrong logo
Since the file is already a client component with useState and useEffect imported, add a mounted check:
Suggested fix
export default function Header({ isGitHubAuthEnabled }: { readonly isGitHubAuthEnabled: boolean }) {
const pathname = usePathname()
const { resolvedTheme } = useTheme()
+ const [mounted, setMounted] = useState(false)
- const logoSrc = resolvedTheme === 'dark' ? '/img/logo_dark.png' : '/img/logo_light.png'
const [mobileMenuOpen, setMobileMenuOpen] = useState(false)
const toggleMobileMenu = () => setMobileMenuOpen(!mobileMenuOpen)
+ useEffect(() => {
+ setMounted(true)
+ }, [])
+
+ const logoSrc = mounted && resolvedTheme === 'dark' ? '/img/logo_dark.png' : '/img/logo_light.png'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/Header.tsx` around lines 26 - 28, The logoSrc is
computed from resolvedTheme immediately, causing a hydration mismatch; add a
mounted-check state (e.g., isMounted via useState and set true in useEffect) and
only compute or render logoSrc after isMounted is true (or fall back to
null/placeholder until mounted) so that useTheme().resolvedTheme is stable on
the client before choosing between '/img/logo_dark.png' and
'/img/logo_light.png'; update references to resolvedTheme and logoSrc in the
Header component to honor isMounted.
1 participant
Proposed change
Making the OWASP Nest Header logo adaptable to light/dark modes
Resolves #4318
Summary
This PR fixes the header logo to adapt correctly based on the selected theme.
Changes
Header.tsxusinguseTheme()Headertests to reflect correct behaviorScrollToToptest by simulating scroll behavior in the test environmentTesting
ScrollToToptest now passes after fixNote
Some unrelated frontend tests are failing locally due to timeouts and environment-specific issues (e.g., async handling and react-aria focus behavior). These appear to be unrelated to the changes introduced in this PR.
Checklist
make check-testlocally: all warnings addressed, tests passed