feat: Add Light/Dark Theme Toggle Using React Context - #4
SaumyaDwivedi179 wants to merge 4 commits into
Conversation
Change SummaryThis PR adds app-wide light/dark theming using a new React File Changes
|
PR ScorecardScoreScoring MethodologyCommunication ScoreThe overall communication score is a weighted average:
Formula: Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
Each category score is the average of its subcategories. Score Bands: 90-100 Excellent | 75-89 Good | 60-74 Adequate | 40-59 Needs Work | 0-39 Critical PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
|
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| export const ThemeProvider = ({ children }) => { | ||
| const [currentTheme, setCurrentTheme] = useState(() => { | ||
| const savedTheme = localStorage.getItem(THEME_STORAGE_KEY) || DEFAULT_THEME; | ||
| document.documentElement.setAttribute("data-theme", savedTheme); | ||
| return savedTheme; | ||
| }); | ||
|
|
||
| const toggleTheme = () => { | ||
| setCurrentTheme((previousTheme) => { | ||
| const nextTheme = previousTheme === THEME_LIGHT ? THEME_DARK : THEME_LIGHT; | ||
| localStorage.setItem(THEME_STORAGE_KEY, nextTheme); | ||
| document.documentElement.setAttribute("data-theme", nextTheme); | ||
| return nextTheme; | ||
| }); |
There was a problem hiding this comment.
Severity: 🟠 Major
Robustness: Guard DOM + localStorage access to prevent runtime crashes
localStorage access can throw (blocked storage / privacy mode) and document mutations will throw in non-browser environments (tests/SSR). Right now a failure here would break the whole app.
Why this matters: Theme should be a non-critical enhancement; it shouldn’t crash the app when storage is unavailable.
Suggested fix:
const readStoredTheme = () => {
if (typeof window === 'undefined') return DEFAULT_THEME;
try {
return window.localStorage.getItem(THEME_STORAGE_KEY);
} catch {
return null;
}
};
const writeStoredTheme = (nextTheme) => {
if (typeof window === 'undefined') return;
try {
window.localStorage.setItem(THEME_STORAGE_KEY, nextTheme);
} catch {
// Ignore storage errors; theme should still work for the current session
}
};Then use these helpers anywhere you read/write localStorage, and guard any document access with typeof document !== 'undefined'.
| it('toggles theme from light to dark', async () => { | ||
| render( | ||
| <ThemeProvider> | ||
| <TestConsumer /> | ||
| </ThemeProvider> | ||
| ) | ||
|
|
||
| const toggleBtn = screen.getByTestId('toggle') | ||
| fireEvent.click(toggleBtn) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByTestId('theme')).toHaveTextContent('dark') | ||
| expect(document.documentElement.getAttribute('data-theme')).toBe('dark') | ||
| expect(localStorage.setItem).toHaveBeenCalledWith('theme', 'dark') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Test Design: Prefer userEvent over fireEvent for user interactions
fireEvent.click bypasses parts of the event sequence that real users trigger.
Why this matters: userEvent better models real behavior and catches issues around focus/disabled state/etc.
Suggested fix:
import userEvent from '@testing-library/user-event';
it('toggles theme from light to dark', async () => {
const user = userEvent.setup();
render(
<ThemeProvider>
<TestConsumer />
</ThemeProvider>
);
const toggleButton = screen.getByRole('button', { name: /toggle/i });
await user.click(toggleButton);
await waitFor(() => {
expect(document.documentElement.getAttribute('data-theme')).toBe('dark');
});
});
| const [currentTheme, setCurrentTheme] = useState(() => { | ||
| const savedTheme = localStorage.getItem(THEME_STORAGE_KEY) || DEFAULT_THEME; | ||
| document.documentElement.setAttribute("data-theme", savedTheme); | ||
| return savedTheme; | ||
| }); |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Best Practices: Avoid render-phase side effects (move setAttribute to an effect)
Calling document.documentElement.setAttribute(...) inside the useState initializer is a render-phase side effect.
Why this matters: Render-phase side effects can behave unexpectedly under StrictMode and make the initial render harder to reason about.
Suggested fix:
import { createContext, useContext, useEffect, useState } from 'react';
export const ThemeProvider = ({ children }) => {
const [currentTheme, setCurrentTheme] = useState(() => {
const savedTheme = normalizeTheme(readStoredTheme());
return savedTheme;
});
useEffect(() => {
if (typeof document !== 'undefined') {
document.documentElement.setAttribute('data-theme', currentTheme);
}
writeStoredTheme(currentTheme);
}, [currentTheme]);
const toggleTheme = () => {
setCurrentTheme((previousTheme) =>
previousTheme === THEME_LIGHT ? THEME_DARK : THEME_LIGHT
);
};
return (
<ThemeContext.Provider value={{ theme: currentTheme, toggleTheme }}>
{children}
</ThemeContext.Provider>
);
};
| @@ -0,0 +1,33 @@ | |||
| import { createContext, useContext, useState } from "react"; | |||
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Consistency: Align quote style / import style across new files
This file uses double quotes while others use single quotes, and some imports include .jsx while others omit it.
Why this matters: Keeping these consistent reduces review noise and prevents churn in future diffs.
Suggested fix: Pick one style and apply it across the new/modified files (e.g., change this to single quotes and align whether you include .jsx in local imports).
| border: '1px solid rgba(255, 255, 255, 0.1)', | ||
| boxShadow: '0 25px 45px rgba(0,0,0,0.2)' | ||
| }}> | ||
| <AppContext /> |
There was a problem hiding this comment.
Severity: 🟠 Major
Coverage: Add a test that covers the actual visible theme toggle UI
Current tests cover the context behavior via a test-only consumer, but nothing asserts that clicking the real toggle button updates theme.
Why this matters: This is the primary user-facing behavior; an accidental UI regression (wrong handler/label/placement) wouldn’t be caught.
Suggested fix: Add src/components/ThemeToggleButton.test.jsx (or rename-aligned path) and test the real button:
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { ThemeProvider } from '../context/ThemeContext.jsx';
import ThemeToggleButton from './AppContext.jsx';
it('toggles theme when the user clicks the toggle button', async () => {
const user = userEvent.setup();
render(
<ThemeProvider>
<ThemeToggleButton />
</ThemeProvider>
);
const button = screen.getByRole('button', { name: /dark/i });
await user.click(button);
expect(document.documentElement.getAttribute('data-theme')).toBe('dark');
});
| const TestConsumer = () => { | ||
| const { theme, toggleTheme } = useTheme() | ||
| return ( | ||
| <div> | ||
| <span data-testid="theme">{theme}</span> | ||
| <button onClick={toggleTheme} data-testid="toggle">Toggle</button> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Test Design: Avoid data-testid when a user-facing query is available
The test component is queried via data-testid, even though the button has visible text and a semantic role.
Why this matters: Role/text queries make tests more resilient and closer to how users and assistive tech interact with the UI.
Suggested fix:
const TestConsumer = () => {
const { theme, toggleTheme } = useTheme();
return (
<div>
<span>{theme}</span>
<button onClick={toggleTheme}>Toggle</button>
</div>
);
};
// usage
const toggleButton = screen.getByRole('button', { name: /toggle/i });
expect(screen.getByText('light')).toBeInTheDocument();
| }}> | ||
| <div style={{ width: '100%', maxWidth: '500px' }}> | ||
| {/* Header Component */} | ||
| <div className="app-container" data-theme={theme}> |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Architecture: Single source of truth for theme application
Theme is being applied in two places: the provider sets documentElement[data-theme], while AppContent sets data-theme on .app-container. This can diverge over time.
Why this matters: When theme is duplicated, debugging "why is the UI in dark mode but button says light" becomes much harder.
Suggested fix (pick one approach):
- If you keep
document.documentElementas the source of truth (recommended with your current CSS), remove the container attribute + theuseTheme()usage:
function AppContent() {
// remove: const { theme } = useTheme();
return (
<div className="app-container">
<div className="app-wrapper">
{/* ... */}
</div>
</div>
);
}Also remove the now-unused import useTheme from this file.
| } | ||
| })() | ||
|
|
||
| Object.defineProperty(window, 'localStorage', { value: localStorageMock }) |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Test Design: Don’t permanently override globals at module scope
window.localStorage is replaced at module scope and never restored, which can leak into other test files and create order-dependent failures.
Why this matters: Global leakage leads to flaky test suites as the codebase grows.
Suggested fix (one option): stub/restore within the suite lifecycle:
let originalLocalStorage;
beforeEach(() => {
originalLocalStorage = window.localStorage;
vi.stubGlobal('localStorage', localStorageMock);
});
afterEach(() => {
vi.unstubAllGlobals();
vi.clearAllMocks();
});Alternatively, move this into a shared test setup file if multiple suites need it.
| ); | ||
| }; | ||
|
|
||
| export const useTheme = () => useContext(ThemeContext); |
There was a problem hiding this comment.
Severity: 🟠 Major
Robustness: Make useTheme fail fast when used outside the provider
useTheme() can return undefined if a component calls it without a surrounding ThemeProvider, and destructuring (const { theme } = useTheme()) will crash.
Why this matters: This becomes a sharp edge as the app grows; an explicit error makes misconfiguration obvious and avoids mysterious runtime failures.
Suggested fix:
const ThemeContext = createContext(undefined);
export const useTheme = () => {
const context = useContext(ThemeContext);
if (!context) {
throw new Error('useTheme must be used within a ThemeProvider');
}
return context;
};
| it('provides default theme if no localStorage', () => { | ||
| render( | ||
| <ThemeProvider> | ||
| <TestConsumer /> | ||
| </ThemeProvider> | ||
| ) | ||
|
|
||
| expect(screen.getByTestId('theme')).toHaveTextContent('light') | ||
| expect(document.documentElement.getAttribute('data-theme')).toBe('light') | ||
| }) |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Coverage: Add a test for invalid persisted theme values
There’s no test verifying that an unexpected persisted value falls back to a known theme.
Why this matters: This protects against corrupted storage, manual edits, and older versions that may have stored different values.
Suggested fix:
it('falls back to default theme when localStorage value is invalid', () => {
localStorage.setItem('theme', 'blue');
render(
<ThemeProvider>
<TestConsumer />
</ThemeProvider>
);
expect(screen.getByText('light')).toBeInTheDocument();
expect(document.documentElement.getAttribute('data-theme')).toBe('light');
});
| import { ThemeProvider, useTheme } from './context/ThemeContext.jsx'; | ||
| import AppContext from './components/AppContext.jsx'; |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Clarity: Rename AppContext to reflect what it actually exports
src/components/AppContext.jsx exports ThemeToggleButton, but it’s imported and rendered as AppContext. This makes the intent harder to follow and suggests it’s providing context rather than UI.
Why this matters: Clear naming reduces cognitive load and prevents misuse as more context providers are introduced.
Suggested fix:
// file: src/components/ThemeToggleButton.jsx
export default function ThemeToggleButton() { /* ... */ }
// App.jsx
import ThemeToggleButton from './components/ThemeToggleButton.jsx';
// ...
<ThemeToggleButton />
| const handleMouseEnter = (event) => { | ||
| event.target.style.transform = `scale(${BUTTON_SCALE_HOVER})`; | ||
| }; | ||
|
|
||
| const handleMouseLeave = (event) => { | ||
| event.target.style.transform = `scale(${BUTTON_SCALE_NORMAL})`; | ||
| }; |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Best Practices: Avoid imperative DOM mutation for hover effects
event.target.style.transform = ... is imperative DOM manipulation and can become brittle (also breaks if the event target changes due to nested elements).
Why this matters: CSS is the most reliable way to express purely visual hover behavior and avoids mixing styling logic into JS.
Suggested fix:
- Remove
handleMouseEnter/handleMouseLeave - Add hover styling in CSS:
.theme-toggle-button {
transform: scale(1);
transition: transform 0.3s ease;
}
.theme-toggle-button:hover {
transform: scale(1.05);
}
| const [currentTheme, setCurrentTheme] = useState(() => { | ||
| const savedTheme = localStorage.getItem(THEME_STORAGE_KEY) || DEFAULT_THEME; | ||
| document.documentElement.setAttribute("data-theme", savedTheme); | ||
| return savedTheme; | ||
| }); |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Robustness: Validate persisted theme values before applying
savedTheme is used as-is. If localStorage.theme is an unexpected value (e.g. "blue"), the UI can desync (toggle label and CSS var resolution won’t match intended states).
Why this matters: Persisted values are external input; validating prevents hard-to-debug styling states.
Suggested fix:
const normalizeTheme = (value) => {
if (value === THEME_DARK) return THEME_DARK;
if (value === THEME_LIGHT) return THEME_LIGHT;
return DEFAULT_THEME;
};
// usage
const savedTheme = normalizeTheme(readStoredTheme());
| <button | ||
| onClick={toggleTheme} | ||
| className="theme-toggle-button" | ||
| style={{ | ||
| position: 'absolute', | ||
| top: BUTTON_POSITION_TOP, | ||
| right: BUTTON_POSITION_RIGHT, | ||
| padding: '10px 20px', | ||
| borderRadius: '8px', | ||
| border: 'none', | ||
| cursor: 'pointer', | ||
| fontSize: '14px', | ||
| fontWeight: '600', | ||
| transition: 'all 0.3s ease', | ||
| backgroundColor: 'var(--theme-button-background)', | ||
| color: 'var(--theme-button-text)', | ||
| boxShadow: '0 2px 8px rgba(0,0,0,0.2)', | ||
| }} |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Best Practices: Move inline style object to CSS (avoid recreating large objects each render)
The button already has className="theme-toggle-button", but most styling is still inline. This recreates a large object every render and spreads styling concerns across JS and CSS.
Why this matters: Centralized styles are easier to maintain, theme, and review; it also reduces unnecessary re-renders caused by new object identities when passing styles down.
Suggested fix:
return (
<button
onClick={toggleTheme}
className="theme-toggle-button"
>
{buttonLabel}
</button>
);.theme-toggle-button {
position: absolute;
top: 20px;
right: 20px;
padding: 10px 20px;
border-radius: 8px;
border: none;
cursor: pointer;
font-size: 14px;
font-weight: 600;
transition: all 0.3s ease;
background-color: var(--theme-button-background);
color: var(--theme-button-text);
box-shadow: 0 2px 8px rgba(0, 0, 0, 0.2);
}
PR AnalysisFocus Areas for Architect Review
RecommendationAction: REQUEST CHANGES Quick Wins:
Author InsightsPR Type: feat Missing Skills
Strengths
|
| @@ -0,0 +1,47 @@ | |||
| import { useTheme } from '../context/ThemeContext'; | |||
|
|
|||
| const THEME_LIGHT = "light"; | |||
There was a problem hiding this comment.
Better to Create a Enum for Theme
| const BUTTON_POSITION_TOP = "20px"; | ||
| const BUTTON_POSITION_RIGHT = "20px"; | ||
| const BUTTON_SCALE_HOVER = "1.05"; | ||
| const BUTTON_SCALE_NORMAL = "1"; |
There was a problem hiding this comment.
No need to create this constants, i want to understand the purpose of these constants.
| const BUTTON_SCALE_HOVER = "1.05"; | ||
| const BUTTON_SCALE_NORMAL = "1"; | ||
|
|
||
| export default function ThemeToggleButton() { |
There was a problem hiding this comment.
File is named AppContext.jsx but exports ThemeToggleButton. Import name doesn't match the actual component. Confusing for developers
| export default function ThemeToggleButton() { | ||
| const { theme, toggleTheme } = useTheme(); | ||
|
|
||
| const handleMouseEnter = (event) => { |
There was a problem hiding this comment.
Simply use css classes no need to write js for hover effect
| <button | ||
| onClick={toggleTheme} | ||
| className="theme-toggle-button" | ||
| style={{ |
There was a problem hiding this comment.
Don't write inline CSS unless not completely required.
And Button component should have some variants like in your case there should be two variants light and dark
and the css should apply conditionally.
| const ThemeContext = createContext(); | ||
|
|
||
| const THEME_STORAGE_KEY = "theme"; | ||
| const THEME_LIGHT = "light"; |
There was a problem hiding this comment.
Create Enum for theme
|
|
||
| const ThemeContext = createContext(); | ||
|
|
||
| const THEME_STORAGE_KEY = "theme"; |
There was a problem hiding this comment.
put it into constant file
|
Tip Need another review? Tag me and say rereview for re-analysis after you have fixed all the issues. @cw-pr-agent rereview |
2 participants
What This PR Does
ThemeContextwithThemeProvideranduseThemehook.AppContext.jsx) to switch between light and dark themes.--bg-color,--text-color,--card-bg, and--card-borderfor consistent theming.App.jsxto dynamically apply the current theme class (light/dark) to<body>or top container.background,color,border) to use CSS variables, making theme switching global and seamless.Manual Testing Steps (Reviewer)
npm install&npm run devto start the app.Files Changed
src/context/ThemeContext.jsx→ Added ThemeProvider & useTheme hooksrc/components/AppContext.jsx→ Added toggle button UIsrc/App.jsx→ Wrapped with ThemeProvider & applied theme stylessrc/index.css→ Added CSS variables for light/dark themes✅ Definition of Done
Code Quality
Testing