Changed light mode icon because it was misleading #2736
Conversation
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaced the dark-mode icon in ModeToggle (sun → lightbulb), added snapshot skeleton rendering and stable keys in SkeletonsBase, replaced LoadingSpinner with SnapshotSkeleton in snapshots page, and updated unit tests to match these UI changes and async waits. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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 |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@dhirukumar Like the icon 👌🏼 , but you also need to fix tests.
Please run make check-test locally and address found issues
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ModeToggle.test.tsx (1)
10-36: Button and icon mocks align withModeToggleusage; minor future‑drift risk in prop typingThe
Buttonmock now correctly passesclassNamethrough to the underlying<button>and acceptsisIconOnly, and theFontAwesomeIconmock exposingdata-testid="icon-..."is a clean way to assert on the rendered icon names. The only potential concern is that the custom prop shape for the mockedButtoncan drift from the real@heroui/buttonAPI over time (e.g., if the real component adds required props). If this becomes an issue, consider deriving the props from the real component viajest.requireActual+React.ComponentProps<typeof RealButton>rather than duplicating a partial signature here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ModeToggle.test.tsx(5 hunks)frontend/src/components/SkeletonsBase.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/SkeletonsBase.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/ModeToggle.test.tsx (1)
frontend/src/components/ModeToggle.tsx (1)
ModeToggle(7-40)
🔇 Additional comments (3)
frontend/__tests__/unit/components/ModeToggle.test.tsx (3)
41-77: Icon rendering tests correctly validate the new light/dark icon mappingThe added
beforeEach(jest.clearAllMocks)keeps theme mocks isolated between tests, and the first two tests now clearly assert the intended behavior: light theme shows the moon icon and dark theme shows the lightbulb icon viaicon-moon/icon-lightbulbtest IDs. This directly exercises the UX change described in the PR and should catch regressions if the icons or aria‑labels are altered.
79-117: Theme toggle interaction tests remain accurate and focusedBoth interaction tests correctly verify that clicking the button calls
setThemewith the opposite theme ('dark'from light,'light'from dark), which matchesModeToggle’s logic. Using a dedicatedsetThemeMockper test keeps expectations clear and avoids cross‑test leakage.
119-129: Based on my verification through web search, I can confirm the technical foundation of the original review comment:React Testing Library does wrap
render()inact(), which flushes synchronous effects immediately. This means by the time the test can inspect the DOM, any synchronoususeEffectthat setsmountedtotruewill have already executed. The reviewer's concern about test fragility is technically sound.However, I cannot fully verify:
- The exact implementation of the ModeToggle component
- Whether this test is currently passing or failing
- The full test file context and setup
The "mounted guard" pattern is indeed a legitimate SSR/hydration safety measure rather than a user-observable intermediate state, which supports the reviewer's recommendation. Since I cannot access the repository to definitively confirm the component code and current test status, I must mark this for manual verification:
Remove the "should not render before mounted" test—it attempts to observe a pre-effect state that RTL's
act()wrapper prevents from being observableReact Testing Library wraps
render()inact(), which flushes synchronoususeEffectcallbacks before returning. By the time this test can inspectcontainer.firstChild, any synchronous effect settingmountedtotruehas already run. Since themountedguard exists to prevent SSR hydration mismatches (not to create a visible intermediate state), this test cannot reliably observe the pre-mounted condition and should be removed.- test('should not render before mounted', () => { - useThemeMock.mockReturnValue({ - theme: 'light', - setTheme: jest.fn(), - }) - - const { container } = render(<ModeToggle />) - - // Component returns null initially (before useEffect runs) - expect(container.firstChild).toBeNull() - })
dhirukumar
force-pushed
the
change-lightmode-icon
branch
from
9c23622 to
5aa6388
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/ModeToggle.test.tsx (2)
52-58: Consider wrapping icon assertion in waitFor for consistency.While the button assertion is correctly wrapped in
waitFor, the icon query on line 57 is outside the async block. Although the icon should be available when the button is rendered, wrapping both assertions in the samewaitForwould be more consistent with the async pattern and could prevent potential flakiness.- await waitFor(() => { - const button = screen.getByRole('button', { name: /enable dark mode/i }) - expect(button).toBeInTheDocument() - }) - - const icon = screen.getByTestId('icon-moon') - expect(icon).toBeInTheDocument() + await waitFor(() => { + const button = screen.getByRole('button', { name: /enable dark mode/i }) + expect(button).toBeInTheDocument() + const icon = screen.getByTestId('icon-moon') + expect(icon).toBeInTheDocument() + })
69-75: Consider wrapping icon assertion in waitFor for consistency.Same pattern as the light mode test: the icon query should ideally be inside the
waitForblock alongside the button assertion for consistency.- await waitFor(() => { - const button = screen.getByRole('button', { name: /enable light mode/i }) - expect(button).toBeInTheDocument() - }) - - const icon = screen.getByTestId('icon-lightbulb') - expect(icon).toBeInTheDocument() + await waitFor(() => { + const button = screen.getByRole('button', { name: /enable light mode/i }) + expect(button).toBeInTheDocument() + const icon = screen.getByTestId('icon-lightbulb') + expect(icon).toBeInTheDocument() + })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ModeToggle.test.tsx(5 hunks)frontend/src/components/SkeletonsBase.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/SkeletonsBase.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/ModeToggle.test.tsx (1)
frontend/src/components/ModeToggle.tsx (1)
ModeToggle(7-40)
🔇 Additional comments (5)
frontend/__tests__/unit/components/ModeToggle.test.tsx (5)
1-1: LGTM! Added waitFor for async test assertions.The addition of
waitForto the imports is appropriate for handling asynchronous rendering behavior in the updated tests.
31-35: LGTM! FontAwesomeIcon mock enables icon verification.The mock correctly extracts
icon.iconNamefrom FontAwesome icon objects and creates test IDs, enabling the tests to verify which icon (moon or lightbulb) is rendered.
40-42: LGTM! Proper test isolation.The
beforeEachhook ensures each test starts with a clean slate by clearing all mocks.
78-96: LGTM! Click interaction test properly handles async rendering.The test correctly waits for the button to be rendered before interacting with it and verifies the theme toggle behavior.
98-116: LGTM! Click interaction test properly handles async rendering.The test correctly waits for the button to be rendered before interacting with it and verifies the theme toggle behavior from dark to light mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ModeToggle.test.tsx (1)
40-76: Light/dark render tests accurately exercise ARIA labels and iconsClearing mocks in
beforeEachplus usingwaitForaround the button queries gives you clean, deterministic tests even if ModeToggle’s mounted/theme logic ever becomes asynchronous. The expectations for:
- light theme → “enable dark mode” +
icon-moon- dark theme → “enable light mode” +
icon-lightbulbalign well with the UX intent from the linked issue.
If you want to simplify slightly, you could replace the
waitForblocks withfindByRole/findByTestId(e.g.,const button = await screen.findByRole(...)), but the current approach is perfectly fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ModeToggle.test.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/__tests__/unit/components/ModeToggle.test.tsx
🔇 Additional comments (3)
frontend/__tests__/unit/components/ModeToggle.test.tsx (3)
10-35: Mocks for Button and FontAwesomeIcon are well-shaped for these testsThe
@heroui/buttonmock now correctly acceptsclassNameand forwards it, and theFontAwesomeIconmock exposingdata-testid="icon-${iconName}"neatly decouples assertions from FontAwesome internals while still using realiconNamevalues. This should make the tests less brittle around styling and icon implementation details.
78-116: Theme toggle interaction tests correctly cover both directionsThe click tests for switching:
- light → dark (
expect(setThemeMock).toHaveBeenCalledWith('dark'))- dark → light (
...With('light'))are straightforward and correctly wrap the initial render in
waitForto ensure the button is present before interacting. This gives good coverage that the control actually drivessetThemewith the expected target theme.
118-129: New “render after mount” test now matches RTL/React effect timingThe
should render after mounttest correctly asserts thatModeToggleis rendered (non‑nullcontainer.firstChildand a visible button) immediately afterrender, which matches React Testing Library’s behavior of flushinguseEffectwithinact. This replaces the earlier, incorrect “pre‑mount” expectation and should prevent the prior false‑negative test behavior.
|
2 participants
Description
This PR updates the Light Mode toggle icon in the UI.
What Was Changed
Replaced the old sun/gear-style icon (which looked like a settings icon):
Added the new light bulb icon (more intuitive for Light Mode):
before
After
🧠 Why This Change
The previous icon was visually misleading and confused users as it closely resembled the settings icon.
The new icon provides clearer UX and better represents Light Mode functionality.
🧪 Testing
Verified UI behavior in both Light and Dark modes
Ensured toggle works with no functional regressions
📝 Checklist
I've read and followed the contributing guidelines
I've run make check-test locally; all checks and tests passed.
Closes #2713