Priyam/dark mode enable#214
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a ThemeContext (ThemeProvider + useTheme), global light/dark CSS, expanded color tokens, a DarkModeToggle FAB component (with tests), MUI theme wiring in App, theming updates in HomeContainer and tests, Storybook compatibility shim, and several defensive property-access fixes. Changes
Sequence DiagramsequenceDiagram
actor User
participant Toggle as "DarkModeToggle\n(FAB)"
participant ThemeCtx as "ThemeProvider\n(useTheme)"
participant MUI as "MUI ThemeProvider\n(createTheme)"
participant LocalStorage as "localStorage"
participant Document as "Document DOM"
User->>Toggle: click
Toggle->>ThemeCtx: call toggleTheme()
ThemeCtx->>ThemeCtx: update isDarkMode state
ThemeCtx->>LocalStorage: persist preference
ThemeCtx->>Document: update root class (dark-mode/light-mode)
ThemeCtx->>MUI: provide updated color tokens (createTheme)
MUI->>Document: re-render themed components
Document->>User: UI updates with new theme
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Summary of ChangesHello @priyam731, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust dark mode feature, significantly enhancing the user experience by allowing theme customization and persistence. It establishes a scalable theme management system using React Context, ensuring that future UI elements can easily adapt to the chosen theme. The changes span new component development, state management, global styling, and updates to existing components to fully integrate the new theming capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dark mode feature, which is a great enhancement. The implementation uses a React Context for theme management and persists the user's preference in localStorage, which is well done. However, there are a few areas for improvement. Most critically, the Material-UI theme is created statically and does not update when the theme is toggled, which will lead to UI inconsistencies. I've also identified opportunities to improve code maintainability by removing duplicated color definitions, enhance component logic for animations, and strengthen the test suite for the new components. Additionally, there are some minor issues in the global styles that could affect performance and code clarity.
app/containers/App/index.js
Outdated
| </StyledEngineProvider> | ||
| <ThemeProvider> | ||
| <StyledEngineProvider injectFirst> | ||
| <MUIThemeProvider theme={theme}> |
There was a problem hiding this comment.
The MUIThemeProvider is receiving a static theme object that is created outside the component. This means the Material-UI components will not react to theme changes (light/dark mode), as the theme is only created once with the default light mode colors. This will cause visual inconsistencies when switching to dark mode.
To fix this, the MUI theme should be created dynamically based on the current theme from themeContext. This can be achieved by creating the theme inside a component that has access to the theme context (using useTheme()) and memoizing it with useMemo. This ensures the MUI theme updates whenever the application's theme changes.
| colors: isDarkMode | ||
| ? { | ||
| primary: '#2d3748', | ||
| secondary: '#4a5568', | ||
| background: '#1a202c', | ||
| surface: '#2d3748', | ||
| text: '#f7fafc', | ||
| textSecondary: '#cbd5e0', | ||
| border: '#4a5568', | ||
| shadow: 'rgba(0, 0, 0, 0.3)', | ||
| hover: '#374151', | ||
| accent: '#60a5fa', | ||
| success: '#48bb78', | ||
| error: '#f56565', | ||
| warning: '#ed8936' | ||
| } | ||
| : { | ||
| primary: '#fcedda', | ||
| secondary: '#f8c49c', | ||
| background: '#ffffff', | ||
| surface: '#f8f9fa', | ||
| text: '#212529', | ||
| textSecondary: '#6c757d', | ||
| border: '#e0e0e0', | ||
| shadow: 'rgba(0, 0, 0, 0.1)', | ||
| hover: '#f1f3f5', | ||
| accent: '#1890ff', | ||
| success: '#28a745', | ||
| error: '#dc3545', | ||
| warning: '#ffc107' | ||
| } |
There was a problem hiding this comment.
The color palettes for light and dark modes are hardcoded here, but they are also defined in app/themes/colors.js. This duplication makes maintenance harder as you have to update colors in two places. You should import the colors from app/themes/colors.js to have a single source of truth.
First, add the import:
import colors from '@app/themes/colors';Then, use the imported colors as suggested.
| colors: isDarkMode | |
| ? { | |
| primary: '#2d3748', | |
| secondary: '#4a5568', | |
| background: '#1a202c', | |
| surface: '#2d3748', | |
| text: '#f7fafc', | |
| textSecondary: '#cbd5e0', | |
| border: '#4a5568', | |
| shadow: 'rgba(0, 0, 0, 0.3)', | |
| hover: '#374151', | |
| accent: '#60a5fa', | |
| success: '#48bb78', | |
| error: '#f56565', | |
| warning: '#ed8936' | |
| } | |
| : { | |
| primary: '#fcedda', | |
| secondary: '#f8c49c', | |
| background: '#ffffff', | |
| surface: '#f8f9fa', | |
| text: '#212529', | |
| textSecondary: '#6c757d', | |
| border: '#e0e0e0', | |
| shadow: 'rgba(0, 0, 0, 0.1)', | |
| hover: '#f1f3f5', | |
| accent: '#1890ff', | |
| success: '#28a745', | |
| error: '#dc3545', | |
| warning: '#ffc107' | |
| } | |
| colors: isDarkMode ? colors.theme.darkMode : colors.theme.lightMode |
| const handleToggle = () => { | ||
| setIsAnimating(true); | ||
| toggleTheme(); | ||
| setTimeout(() => setIsAnimating(false), 500); | ||
| }; |
There was a problem hiding this comment.
Using setTimeout with a hardcoded duration to reset the animation state is fragile. If the animation duration in the CSS (0.5s) is changed, this setTimeout duration (500ms) will also need to be updated manually. A more robust approach is to use the onAnimationEnd event on the animated element to reset the state.
You can update handleToggle as suggested, and then add the onAnimationEnd handler to your IconWrapper to reset the animation state:
<IconWrapper animate={isAnimating} onAnimationEnd={() => setIsAnimating(false)}>
<Icon fontSize="large" />
</IconWrapper> const handleToggle = () => {
setIsAnimating(true);
toggleTheme();
};| it('should show sun icon in dark mode', () => { | ||
| renderWithTheme('dark'); | ||
| const button = screen.getByRole('button'); | ||
| expect(button).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should show moon icon in light mode', () => { | ||
| renderWithTheme('light'); | ||
| const button = screen.getByRole('button'); | ||
| expect(button).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
These tests only check for the presence of the button, not the specific icon for each theme mode. To make these tests more meaningful, you should assert that the correct icon is rendered for each mode. Material-UI icons have a data-testid you can use for this.
| it('should show sun icon in dark mode', () => { | |
| renderWithTheme('dark'); | |
| const button = screen.getByRole('button'); | |
| expect(button).toBeInTheDocument(); | |
| }); | |
| it('should show moon icon in light mode', () => { | |
| renderWithTheme('light'); | |
| const button = screen.getByRole('button'); | |
| expect(button).toBeInTheDocument(); | |
| }); | |
| it('should show sun icon in dark mode', () => { | |
| renderWithTheme('dark'); | |
| expect(screen.getByTestId('Brightness7Icon')).toBeInTheDocument(); | |
| expect(screen.queryByTestId('Brightness4Icon')).not.toBeInTheDocument(); | |
| }); | |
| it('should show moon icon in light mode', () => { | |
| renderWithTheme('light'); | |
| expect(screen.getByTestId('Brightness4Icon')).toBeInTheDocument(); | |
| expect(screen.queryByTestId('Brightness7Icon')).not.toBeInTheDocument(); | |
| }); |
| it('should show tooltip on hover', () => { | ||
| renderWithTheme(); | ||
| const button = screen.getByRole('button'); | ||
| fireEvent.mouseOver(button); | ||
| // Material-UI tooltips appear after a delay | ||
| }); |
There was a problem hiding this comment.
This test for the tooltip is incomplete. Since Material-UI tooltips appear asynchronously after a delay, you need to use an async test with await and a findBy* query to correctly test for the tooltip's appearance.
it('should show tooltip on hover', async () => {
renderWithTheme();
const button = screen.getByRole('button');
fireEvent.mouseOver(button);
const tooltip = await screen.findByRole('tooltip');
expect(tooltip).toBeInTheDocument();
expect(tooltip).toHaveTextContent('Switch to Dark Mode');
});| * { | ||
| transition: background-color var(--transition-speed) ease, color var(--transition-speed) ease, | ||
| border-color var(--transition-speed) ease, box-shadow var(--transition-speed) ease; | ||
| } |
There was a problem hiding this comment.
Using the universal selector (*) to apply transitions to all elements can negatively impact performance, especially on pages with many elements. It forces the browser to track transition properties for everything, which can be costly. It's better to apply transitions more selectively only to the elements that need to be animated during the theme change.
| /* Smooth animations for theme switch */ | ||
| @keyframes fadeIn { | ||
| from { | ||
| opacity: 0; | ||
| transform: translateY(10px); | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| transform: translateY(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
The @keyframes fadeIn is defined here but does not appear to be used anywhere in the application. This is dead code and should be removed to keep the codebase clean.
![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 (2)
app/global-styles.js (1)
14-50:⚠️ Potential issue | 🟠 MajorRespect prefers-reduced-motion for global transitions/animations.
Global transitions and animations can be problematic for motion-sensitive users. Gate them behind
prefers-reduced-motionor disable them when reduced motion is requested.🧩 Suggested accessibility guard
html, body { height: 100vh; width: 100vw; margin: 0; padding: 0; transition: background-color var(--transition-speed) ease, color var(--transition-speed) ease; } + `@media` (prefers-reduced-motion: reduce) { + html, + body, + html.light-mode `#app`, + html.dark-mode `#app`, + html.dark-mode * { + transition: none !important; + animation: none !important; + } + }Also applies to: 82-91
app/containers/HomeContainer/index.js (1)
24-47:⚠️ Potential issue | 🟠 MajorProp‑driven card styling isn’t applied everywhere (dark mode gaps).
CustomCard/CustomCardHeadernow rely on theme props, butrenderRepoListandrenderErrorStatestill render them without those props, which can leave parts of the UI un-themed (or generate invalid CSS likeborder: 1px solid undefined) in dark mode. Please threadthemeColorsinto those helpers or provide defaults.✅ Suggested fix: pass theme colors into all CustomCard usages
- {renderRepoList(reposData, loading, repoName)} - {renderErrorState(repoName, loading, reposError)} + {renderRepoList(reposData, loading, repoName, themeColors)} + {renderErrorState(repoName, loading, reposError, themeColors)}-const renderRepoList = (reposData, loading, repoName) => { +const renderRepoList = (reposData, loading, repoName, themeColors) => { const items = get(reposData, 'items', []); const totalCount = get(reposData, 'totalCount', 0); return ( <If condition={!isEmpty(items) || loading}> - <CustomCard> + <CustomCard + bgcolor={themeColors.surface} + color={themeColors.text} + bordercolor={themeColors.border} + shadowcolor={themeColors.shadow} + > <If condition={!loading} otherwise={renderSkeleton()}>-const renderErrorState = (repoName, loading, reposError) => { +const renderErrorState = (repoName, loading, reposError, themeColors) => { let repoError; let messageId; if (reposError) { repoError = reposError; messageId = 'error-message'; } else if (isEmpty(repoName)) { repoError = 'repo_search_default'; messageId = 'default-message'; } return ( <If condition={!loading && repoError}> - <CustomCard color={reposError ? 'red' : 'grey'}> - <CustomCardHeader title={translate('repo_list')} /> - <Divider sx={{ mb: 1.25 }} light /> + <CustomCard + bgcolor={themeColors.surface} + color={reposError ? 'red' : themeColors.textSecondary} + bordercolor={themeColors.border} + shadowcolor={themeColors.shadow} + > + <CustomCardHeader title={translate('repo_list')} titlecolor={themeColors.text} /> + <Divider sx={{ mb: 1.25, borderColor: themeColors.border }} /> <T data-testid={messageId} id={repoError} text={repoError} /> </CustomCard> </If> ); };
🤖 Fix all issues with AI agents
In `@app/components/DarkModeToggle/index.js`:
- Around line 62-66: The timeout started in handleToggle (setTimeout(() =>
setIsAnimating(false), 500)) can call setState after the component unmounts;
store the timer id (e.g., using a ref like animationTimerRef) when you call
setTimeout, and clear it in a useEffect cleanup (and before setting a new
timeout when handleToggle runs again) using
clearTimeout(animationTimerRef.current) to avoid calling setIsAnimating after
unmount; update handleToggle to save the returned id and add a useEffect that
returns a cleanup which clears the timer.
In `@app/components/DarkModeToggle/tests/index.test.js`:
- Around line 64-69: The test "should show tooltip on hover" currently has no
assertion; after calling renderWithTheme() and fireEvent.mouseOver(button)
(where button is obtained via screen.getByRole('button')), make the test async
and await an assertion that the tooltip appears — e.g. await
screen.findByRole('tooltip') or await screen.findByText('<expected tooltip
text>') — to assert the Material-UI tooltip is present in the DOM; update the
test function signature to async and add the awaited assertion after
fireEvent.mouseOver(button).
- Around line 44-55: The test's assertions read localStorage synchronously but
the component writes it inside a useEffect, causing flakiness; update the test
in index.test.js (the "should toggle theme when clicked" spec that uses
renderWithTheme and the button from getByRole) to wrap the post-click
localStorage assertions in async waitFor calls (await waitFor(() => {
expect(localStorage.getItem('theme')).toBe('dark') }) and similarly for 'light')
so the test waits for the effect to run after fireEvent.click.
🧹 Nitpick comments (4)
app/contexts/themeContext.js (2)
47-83: Centralize theme tokens to avoid drift between ThemeContext and colors.js.The color palette is duplicated here and in
app/themes/colors.js. Reuse the shared tokens to prevent divergence.♻️ Suggested consolidation
-import React, { createContext, useState, useEffect, useContext, useMemo } from 'react'; +import React, { createContext, useState, useEffect, useContext, useMemo } from 'react'; +import themeColors from '@app/themes/colors'; ... const theme = useMemo( () => ({ isDarkMode, toggleTheme, - colors: isDarkMode - ? { - primary: '#2d3748', - secondary: '#4a5568', - background: '#1a202c', - surface: '#2d3748', - text: '#f7fafc', - textSecondary: '#cbd5e0', - border: '#4a5568', - shadow: 'rgba(0, 0, 0, 0.3)', - hover: '#374151', - accent: '#60a5fa', - success: '#48bb78', - error: '#f56565', - warning: '#ed8936' - } - : { - primary: '#fcedda', - secondary: '#f8c49c', - background: '#ffffff', - surface: '#f8f9fa', - text: '#212529', - textSecondary: '#6c757d', - border: '#e0e0e0', - shadow: 'rgba(0, 0, 0, 0.1)', - hover: '#f1f3f5', - accent: '#1890ff', - success: '#28a745', - error: '#dc3545', - warning: '#ffc107' - } + colors: isDarkMode ? themeColors.theme.darkMode : themeColors.theme.lightMode }), - [isDarkMode] + [isDarkMode, toggleTheme] );
10-15: useTheme guard is ineffective with a non-null default context.
createContext({...})guarantees a truthy value, so the guard never fires. ConsidercreateContext(null)and update the hook to throw on null, or remove the check.app/global-styles.js (1)
82-91: fadeIn keyframes are unused.Either apply the animation to a selector (e.g.,
#app) or remove the keyframes to avoid dead CSS.🧹 Cleanup option
- /* Smooth animations for theme switch */ - `@keyframes` fadeIn { - from { - opacity: 0; - transform: translateY(10px); - } - to { - opacity: 1; - transform: translateY(0); - } - }app/components/DarkModeToggle/index.js (1)
7-24: Prevent custom style props from leaking to the DOM.
bgcolor,hoverbg, andshadowcolorcan be forwarded to the underlying<button>which may cause React warnings and DOM noise. Filter them viashouldForwardProp.🧩 Suggested prop filtering
-const ToggleButton = styled(IconButton)` +const ToggleButton = styled(IconButton, { + shouldForwardProp: (prop) => !['bgcolor', 'hoverbg', 'shadowcolor'].includes(prop) +})` && { position: fixed; bottom: 2rem;
| const handleToggle = () => { | ||
| setIsAnimating(true); | ||
| toggleTheme(); | ||
| setTimeout(() => setIsAnimating(false), 500); | ||
| }; |
There was a problem hiding this comment.
Clear the animation timeout on unmount to avoid setState after unmount.
If the component unmounts within 500ms, the timeout callback will still fire.
🧹 Suggested cleanup
export const DarkModeToggle = () => {
const { isDarkMode, toggleTheme, colors } = useTheme();
const [isAnimating, setIsAnimating] = React.useState(false);
+ const timeoutRef = React.useRef();
+ React.useEffect(
+ () => () => {
+ if (timeoutRef.current) clearTimeout(timeoutRef.current);
+ },
+ []
+ );
const handleToggle = () => {
setIsAnimating(true);
toggleTheme();
- setTimeout(() => setIsAnimating(false), 500);
+ if (timeoutRef.current) clearTimeout(timeoutRef.current);
+ timeoutRef.current = setTimeout(() => setIsAnimating(false), 500);
};🤖 Prompt for AI Agents
In `@app/components/DarkModeToggle/index.js` around lines 62 - 66, The timeout
started in handleToggle (setTimeout(() => setIsAnimating(false), 500)) can call
setState after the component unmounts; store the timer id (e.g., using a ref
like animationTimerRef) when you call setTimeout, and clear it in a useEffect
cleanup (and before setting a new timeout when handleToggle runs again) using
clearTimeout(animationTimerRef.current) to avoid calling setIsAnimating after
unmount; update handleToggle to save the returned id and add a useEffect that
returns a cleanup which clears the timer.
| it('should toggle theme when clicked', () => { | ||
| renderWithTheme(); | ||
| const button = screen.getByRole('button', { name: /toggle dark mode/i }); | ||
|
|
||
| // Click to switch to dark mode | ||
| fireEvent.click(button); | ||
| expect(localStorage.getItem('theme')).toBe('dark'); | ||
|
|
||
| // Click to switch back to light mode | ||
| fireEvent.click(button); | ||
| expect(localStorage.getItem('theme')).toBe('light'); | ||
| }); |
There was a problem hiding this comment.
Make the theme-toggle assertion resilient to effect timing.
localStorage is updated in a useEffect, which may not flush synchronously. Use waitFor to avoid flakiness.
✅ Suggested fix
-import { render, screen, fireEvent } from '@testing-library/react';
+import { render, screen, fireEvent, waitFor } from '@testing-library/react';
...
- it('should toggle theme when clicked', () => {
+ it('should toggle theme when clicked', async () => {
renderWithTheme();
const button = screen.getByRole('button', { name: /toggle dark mode/i });
// Click to switch to dark mode
fireEvent.click(button);
- expect(localStorage.getItem('theme')).toBe('dark');
+ await waitFor(() => expect(localStorage.getItem('theme')).toBe('dark'));
// Click to switch back to light mode
fireEvent.click(button);
- expect(localStorage.getItem('theme')).toBe('light');
+ await waitFor(() => expect(localStorage.getItem('theme')).toBe('light'));
});📝 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.
| it('should toggle theme when clicked', () => { | |
| renderWithTheme(); | |
| const button = screen.getByRole('button', { name: /toggle dark mode/i }); | |
| // Click to switch to dark mode | |
| fireEvent.click(button); | |
| expect(localStorage.getItem('theme')).toBe('dark'); | |
| // Click to switch back to light mode | |
| fireEvent.click(button); | |
| expect(localStorage.getItem('theme')).toBe('light'); | |
| }); | |
| it('should toggle theme when clicked', async () => { | |
| renderWithTheme(); | |
| const button = screen.getByRole('button', { name: /toggle dark mode/i }); | |
| // Click to switch to dark mode | |
| fireEvent.click(button); | |
| await waitFor(() => expect(localStorage.getItem('theme')).toBe('dark')); | |
| // Click to switch back to light mode | |
| fireEvent.click(button); | |
| await waitFor(() => expect(localStorage.getItem('theme')).toBe('light')); | |
| }); |
🤖 Prompt for AI Agents
In `@app/components/DarkModeToggle/tests/index.test.js` around lines 44 - 55, The
test's assertions read localStorage synchronously but the component writes it
inside a useEffect, causing flakiness; update the test in index.test.js (the
"should toggle theme when clicked" spec that uses renderWithTheme and the button
from getByRole) to wrap the post-click localStorage assertions in async waitFor
calls (await waitFor(() => { expect(localStorage.getItem('theme')).toBe('dark')
}) and similarly for 'light') so the test waits for the effect to run after
fireEvent.click.
| it('should show tooltip on hover', () => { | ||
| renderWithTheme(); | ||
| const button = screen.getByRole('button'); | ||
| fireEvent.mouseOver(button); | ||
| // Material-UI tooltips appear after a delay | ||
| }); |
There was a problem hiding this comment.
The tooltip test has no assertion.
It currently can pass even if the tooltip is broken. Add an assertion (or remove the test).
🧪 Suggested assertion
-import { render, screen, fireEvent } from '@testing-library/react';
+import { render, screen, fireEvent, act } from '@testing-library/react';
...
it('should show tooltip on hover', () => {
+ jest.useFakeTimers();
renderWithTheme();
const button = screen.getByRole('button');
fireEvent.mouseOver(button);
- // Material-UI tooltips appear after a delay
+ act(() => {
+ jest.runAllTimers();
+ });
+ expect(screen.getByRole('tooltip')).toHaveTextContent(/switch to dark mode/i);
+ jest.useRealTimers();
});🤖 Prompt for AI Agents
In `@app/components/DarkModeToggle/tests/index.test.js` around lines 64 - 69, The
test "should show tooltip on hover" currently has no assertion; after calling
renderWithTheme() and fireEvent.mouseOver(button) (where button is obtained via
screen.getByRole('button')), make the test async and await an assertion that the
tooltip appears — e.g. await screen.findByRole('tooltip') or await
screen.findByText('<expected tooltip text>') — to assert the Material-UI tooltip
is present in the DOM; update the test function signature to async and add the
awaited assertion after fireEvent.mouseOver(button).
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)
app/containers/App/index.js (1)
36-48:⚠️ Potential issue | 🟠 MajorMUI theme is static and won't respond to dark mode toggle.
The
createThemecall on Lines 36–48 uses fixedcolors.primary/colors.secondaryvalues and is created once at module scope. When the user toggles dark mode viaThemeProvider(custom context), the MUIthemeobject passed to<MUIThemeProvider>remains unchanged. This means MUI components (e.g.,CssBaseline,Container, any MUI buttons/inputs) will always render with the light palette, causing visual inconsistencies in dark mode.To fix this,
createThemeshould be called reactively inside the component tree using the current dark mode state fromuseTheme(), or the customThemeContextcolors should be fed into MUI's palette dynamically.Also applies to: 80-118
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 72-76: Update README references to point to the actual theme
implementation and state management: replace the mention of app/themes/index.js
as the source of light/dark palettes with app/contexts/themeContext.js and note
that the DarkModeToggle component (app/components/DarkModeToggle/index.js)
reads/updates the theme via the custom ThemeContext (exported from
themeContext.js) rather than Material-UI's ThemeProvider; also clarify that
Material-UI's ThemeProvider is only used to apply the theme object generated
from ThemeContext, not to hold the dark mode state.
- Around line 79-84: The README example imports DarkModeToggle as a default but
the component is exported as a named export (export const DarkModeToggle in
app/components/DarkModeToggle/index.js); update the usage example to use a named
import for DarkModeToggle (i.e., import { DarkModeToggle } from '...') so the
example matches the actual export and will work when rendering <DarkModeToggle
/>.
🧹 Nitpick comments (4)
app/components/ProtectedRoute/index.js (1)
15-19:hasOwnPropertycheck is redundant here.
Object.keys(routeConstants)on Line 14 already returns only own enumerable properties, so thehasOwnPropertyguard on Line 18 can never be false. Not harmful, but it adds noise without providing any actual safety benefit.app/i18n.js (1)
24-31:hasMessagecheck on Line 25 is alwaystrue.Since
keyis sourced fromObject.keys(messages)(Line 34), it is guaranteed to be an own property ofmessages. ThehasDefaultMessagecheck on Line 26, however, is genuinely useful sincedefaultFormattedMessagesmay not contain the same keys.app/containers/App/index.js (2)
89-89: Redundantlodash/mapwrappingObject.keys().
map(Object.keys(routeConfig))just shallow-copies the array —Object.keys(routeConfig)alone is sufficient here.- of={map(Object.keys(routeConfig))} + of={Object.keys(routeConfig)}
90-112:hasOwnPropertyguard on Line 92 is redundant — keys come fromObject.keys(routeConfig).Same pattern as in the other files;
Object.keysonly yields own enumerable properties.Also, consider using
routeKeyinstead ofindexfor the Reactkeyprop (Line 101) — it's more semantically meaningful and stable, even though the route list is static.- key={index} + key={routeKey}
- Add ReactDOM.render polyfill using createRoot API - Add ReactDOM.unmountComponentAtNode polyfill - Fixes 'react_dom.render is not a function' error - Fixes 'react_dom.unmountComponentAtNode is not a function' error - All 3 Storybook tests now passing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.storybook/preview.js:
- Around line 11-26: The polyfill for ReactDOM.render calls callback immediately
after scheduling root.render, causing the callback to run before the DOM commit;
update the ReactDOM.render implementation (the polyfill wrapping
rootsMap/createRoot/root.render) to call root.render inside flushSync so the
render is committed before invoking the callback, e.g. import flushSync from
React and use flushSync(() => root.render(element)); then call callback() after
flushSync returns to preserve legacy semantics.
🧹 Nitpick comments (1)
.storybook/preview.js (1)
5-9: Consider upgrading Storybook instead of maintaining this polyfill.Storybook 6.5.16 + React 19 is an unsupported combination. While Storybook 8 provides better React 18/19 compatibility via
@storybook/react-dom-shim, full React 19 support is still being finalized (targeted for Storybook 9.0). An upgrade to Storybook 8 would reduce maintenance burden, but expect possible edge cases until Storybook 9 lands. This is worth tracking as follow-up work.
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)
app/containers/App/index.js (1)
74-83:⚠️ Potential issue | 🟠 MajorOpen redirect via
redirect_uriquery parameter.
routeToReplaceis taken directly from user-controlled query string and passed tohistory.replace()without validation. An attacker could craft a URL like?redirect_uri=//evil.comor?redirect_uri=javascript:...to redirect users to a malicious site. This is pre-existing code, but worth flagging since it's a security concern.Consider validating that
routeToReplaceis a relative path (e.g., starts with/and not//):Proposed fix
if (location.search.includes('?redirect_uri=')) { const routeToReplace = new URLSearchParams(location.search).get('redirect_uri'); - history.replace(routeToReplace); + if (routeToReplace && routeToReplace.startsWith('/') && !routeToReplace.startsWith('//')) { + history.replace(routeToReplace); + } }
🤖 Fix all issues with AI agents
In `@app/utils/testUtils.js`:
- Around line 39-43: Tests using CustomThemeProvider in app/utils/testUtils.js
cause persistent side effects because ThemeProvider (in themeContext.js) writes
localStorage['theme'] and mutates document.documentElement.classList; update the
test utility to prevent cross-test pollution by either mocking localStorage and
window.matchMedia when mounting CustomThemeProvider or adding cleanup logic that
runs afterEach to remove localStorage.removeItem('theme') and restore
document.documentElement.classList to its original state; specifically, locate
CustomThemeProvider usage in testUtils.js and add setup/teardown helpers that
reset localStorage and clear any theme-related classes and provide a safe mock
for window.matchMedia so tests run deterministically.
🧹 Nitpick comments (2)
app/containers/App/index.js (1)
136-139:AppContent.propTypesdeclares props that the component never receives.
AppContentis rendered as<AppContent />with no props (Line 148). ThelocationandhistorypropTypes appear to be leftovers from the previousAppfunction signature. This is dead code and should be removed to avoid confusion.Proposed fix
-AppContent.propTypes = { - location: PropTypes.object, - history: PropTypes.object -};app/utils/testUtils.js (1)
26-32:renderWithIntldoes not include theme providers.Components that depend on
CustomThemeProvider(viauseTheme) or MUI'sThemeProviderwill fail if rendered withrenderWithIntlinstead ofrenderProvider. This may be intentional for lightweight i18n-only tests, but it's worth noting — any component usinguseTheme()fromthemeContextwill getundefinedand likely throw.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 16-18: The workflow currently uses the pull_request_target trigger
together with actions/checkout@v4 checking out the PR head SHA, which allows
untrusted PR code to run with repository secrets; fix by changing the trigger
from pull_request_target to pull_request (update the top-level on: to
pull_request) so the PR runs with the contributor’s context and cannot access
secrets, and remove any checkout of the PR head SHA pattern used with
pull_request_target; alternatively, if secrets (e.g.,
SONAR_TOKEN/SONAR_HOST_URL) are required, split into two workflows: keep an
untrusted build/test workflow triggered by pull_request that checks out and runs
tests (no secrets), and create a separate trusted workflow (e.g., triggered by
workflow_run or on push to main) that consumes artifacts and runs
secret-requiring steps like SonarQube scanning.
1 participant
Description
Added dark mode toggle functionality to the application. Users can now switch between light and dark themes.
Changes:
Steps to Reproduce / Test
GIF's
Live Link
http://wednesday-react-template.s3-website.ap-south-1.amazonaws.com/priyam/dark_mode-enable
Summary by CodeRabbit
New Features
Refactor
Style
Tests
Documentation