feat(redux): implement dark/light theme toggle with redux toolkit - #6
vidyashreebv wants to merge 24 commits into
Conversation
Change SummaryThis PR introduces Redux Toolkit state management to support a global dark/light theme mode with persistence to Note (description vs diff): The PR description mentions PropTypes added to all components; in this diff it’s added to 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}
| className="text-7xl font-bold bg-linear-to-r from-green-600 to-emerald-600 bg-clip-text text-transparent tabular transition-all duration-300" | ||
| style={{ fontVariantNumeric: 'tabular' }} |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Correctness: Invalid tabular number styling
tabular and fontVariantNumeric: 'tabular' are unlikely to do what you intend. The CSS keyword is tabular-nums, and Tailwind’s utility is tabular-nums.
Why this matters: Without proper tabular numerals, the counter can visually “jump” width as digits change.
Suggested fix:
<div
className="text-7xl font-bold bg-linear-to-r from-green-600 to-emerald-600 bg-clip-text text-transparent tabular-nums transition-all duration-300"
aria-live="polite"
aria-label={displayAriaLabel}
>
{value}
</div>(And remove the inline style entirely.)
| const createMockStore = (initialTheme = 'light') => { | ||
| return configureStore({ | ||
| reducer: { | ||
| theme: themeReducer, | ||
| }, | ||
| preloadedState: { | ||
| theme: { mode: initialTheme }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| const renderWithStore = (component, store) => { | ||
| return render(<Provider store={store}>{component}</Provider>) | ||
| } |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Structure: Reusable Redux test helpers should be centralized
createMockStore / renderWithStore are likely to be reused as more Redux-connected components are added.
Why this matters: Keeping these helpers in one place prevents duplication and keeps tests focused on behavior.
Suggested fix:
// src/test/testUtils.js
import { render } from '@testing-library/react'
import { Provider } from 'react-redux'
export const renderWithStore = (ui, store) => {
return render(<Provider store={store}>{ui}</Provider>)
}Then import it in tests instead of redefining per file.
| const themeMode = useSelector(selectThemeMode) | ||
| const [initialValue, setInitialValue] = useState(10) | ||
| const [step, setStep] = useState(5) | ||
| const [key, setKey] = useState(0) |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Clarity: Non-descriptive state name key
key doesn’t communicate that it’s used to force-remount the Counter.
Why this matters: Readers have to infer intent from usage; clearer naming makes maintenance easier.
Suggested fix:
const [counterInstanceKey, setCounterInstanceKey] = useState(0)
// ...
setCounterInstanceKey((prevKey) => prevKey + 1)
// ...
<Counter key={counterInstanceKey} initialValue={initialValue} step={step} />
| setTheme: (state, action) => { | ||
| state.mode = action.payload | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', state.mode) | ||
| } | ||
| }, |
There was a problem hiding this comment.
Severity: 🟠 Major
Robustness: Persisted theme / setTheme payload is not validated
setTheme writes action.payload directly to state, and getInitialTheme trusts whatever is in storage. Any unexpected value (typo, manual edit, stale data) can put the app into an unsupported state.
Why this matters: Unsupported values can lead to UI desyncs (no .dark class, wrong icon/label, etc.) and make bugs harder to diagnose.
Suggested fix:
const THEME_MODES = {
light: 'light',
dark: 'dark',
}
const isThemeMode = (value) => value === THEME_MODES.light || value === THEME_MODES.dark
// in getInitialTheme()
const savedTheme = safeStorageGet('theme')
return isThemeMode(savedTheme) ? savedTheme : THEME_MODES.light
// in setTheme reducer
setTheme: (state, action) => {
state.mode = isThemeMode(action.payload) ? action.payload : THEME_MODES.light
},
| @@ -0,0 +1,100 @@ | |||
| import { describe, it, expect} from 'vitest' | |||
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Test Design: Minor import formatting
There’s a small spacing issue in the Vitest import (expect}) that makes the file slightly harder to scan.
Suggested fix:
import { describe, it, expect } from 'vitest'
| export const ThemeToggle = () => { | ||
| const dispatch = useDispatch() | ||
| const themeMode = useSelector(selectThemeMode) | ||
|
|
||
| const handleClickToggleTheme = () => { | ||
| dispatch(toggleTheme()) | ||
| } | ||
|
|
||
| const isDarkMode = themeMode === 'dark' | ||
|
|
||
| return ( | ||
| <button | ||
| onClick={handleClickToggleTheme} | ||
| aria-label={`Switch to ${isDarkMode ? 'light' : 'dark'} mode`} | ||
| className="p-3 rounded-lg bg-white dark:bg-gray-800 shadow-md hover:shadow-lg transition-all duration-200 border border-gray-200 dark:border-gray-700" | ||
| > | ||
| {isDarkMode ? ( | ||
| <svg | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| strokeWidth={2} | ||
| stroke="currentColor" | ||
| className="w-6 h-6 text-yellow-500" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| d="M12 3v2.25m6.364.386l-1.591 1.591M21 12h-2.25m-.386 6.364l-1.591-1.591M12 18.75V21m-4.773-4.227l-1.591 1.591M5.25 12H3m4.227-4.773L5.636 5.636M15.75 12a3.75 3.75 0 11-7.5 0 3.75 3.75 0 017.5 0z" | ||
| /> | ||
| </svg> | ||
| ) : ( | ||
| <svg | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| strokeWidth={2} | ||
| stroke="currentColor" | ||
| className="w-6 h-6 text-gray-700" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| d="M21.752 15.002A9.718 9.718 0 0118 15.75c-5.385 0-9.75-4.365-9.75-9.75 0-1.33.266-2.597.748-3.752A9.753 9.753 0 003 11.25C3 16.635 7.365 21 12.75 21a9.753 9.753 0 009.002-5.998z" | ||
| /> | ||
| </svg> | ||
| )} | ||
| </button> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Consistency: ThemeToggle missing PropTypes
Other components were updated with PropTypes, but ThemeToggle doesn’t define any, creating inconsistent runtime validation.
Why this matters: Inconsistency makes it easier for components to silently drift in how they’re used and validated.
Suggested fix (even if there are no required props, allow an override like className):
import PropTypes from 'prop-types'
export const ThemeToggle = ({ className = '' }) => {
// ...
return (
<button
type="button"
onClick={handleClickToggleTheme}
aria-label={`Switch to ${isDarkMode ? 'light' : 'dark'} mode`}
className={`p-3 rounded-lg bg-white dark:bg-gray-800 shadow-md hover:shadow-lg transition-all duration-200 border border-gray-200 dark:border-gray-700 ${className}`}
>
{/* ... */}
</button>
)
}
ThemeToggle.propTypes = {
className: PropTypes.string,
}
| const getInitialTheme = () => { | ||
| if (globalThis.window !== undefined) { | ||
| const savedTheme = globalThis.localStorage.getItem('theme') | ||
| return savedTheme || 'light' | ||
| } | ||
| return 'light' |
There was a problem hiding this comment.
Severity: 🟠 Major
Robustness: localStorage access can throw and crash the app
localStorage.getItem / setItem can throw in some environments (privacy mode, blocked storage, quota exceeded). Right now an exception will bubble up and potentially break the app.
Why this matters: Theme toggle and initial load can hard-fail for a subset of users/browsers.
Suggested fix:
const safeStorageGet = (key) => {
try {
return globalThis.localStorage?.getItem(key) ?? null
} catch {
return null
}
}
const safeStorageSet = (key, value) => {
try {
globalThis.localStorage?.setItem(key, value)
} catch {
// ignore persistence failures
}
}Use safeStorageGet('theme') inside getInitialTheme() and safeStorageSet('theme', mode) wherever you persist.
| @supports (selector(&:where(.dark, .dark *))) { | ||
| @media (prefers-color-scheme: dark) { | ||
| :root { | ||
| color-scheme: dark; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Correctness: color-scheme doesn’t follow the selected theme
color-scheme: dark is currently tied to OS preference, not to your .dark class toggle. That means native controls (inputs, scrollbars) may stay “dark” even when the app is in light mode (or vice versa).
Why this matters: Users will see inconsistent theming for native UI elements.
Suggested fix:
:root {
color-scheme: light;
}
:root.dark {
color-scheme: dark;
}(Then you can drop the @media (prefers-color-scheme: dark) override unless you’re explicitly supporting “system” mode.)
| useEffect(() => { | ||
| const root = document.documentElement | ||
| if (themeMode === 'dark') { | ||
| root.classList.add('dark') | ||
| } else { | ||
| root.classList.remove('dark') | ||
| } | ||
| }, [themeMode]) |
There was a problem hiding this comment.
Severity: 🟠 Major
Robustness: Un-guarded document access breaks non-DOM environments
document.documentElement will throw in SSR or test environments that don’t provide a DOM.
Why this matters: It makes rendering App in SSR or certain test setups fail immediately.
Suggested fix:
useEffect(() => {
if (typeof document === 'undefined') return
const root = document.documentElement
root.classList.toggle('dark', themeMode === 'dark')
}, [themeMode])
| <div className="flex gap-3 flex-wrap"> | ||
| <div className="flex-1 min-w-[140px]"> | ||
| <label className="block text-xs font-medium text-gray-700 mb-1"> | ||
| <div className="flex-1 min-w-35"> |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Correctness: Non-generated Tailwind class may break layout
min-w-35 is likely not a generated Tailwind utility (unless you’ve explicitly extended the scale), so the min width constraint may silently stop applying.
Why this matters: The settings inputs can collapse and wrap unexpectedly, especially on narrow screens.
Suggested fix:
<div className="flex-1 min-w-[140px]">(Alternatively, use a known scale value like min-w-36 if you want a tokenized size.)
| it('toggles theme from light to dark when clicked', async () => { | ||
| const store = createMockStore('light') | ||
| renderWithStore(<ThemeToggle />, store) | ||
| const user = userEvent.setup() | ||
|
|
||
| const button = screen.getByRole('button') | ||
| await user.click(button) | ||
|
|
||
| const state = store.getState() | ||
| expect(state.theme.mode).toBe('dark') | ||
| }) |
There was a problem hiding this comment.
Severity: 🟡 Moderate
Test Design: Tests assert internal Redux state instead of user-visible behavior
These tests read store.getState() after clicks. That couples tests to implementation details and makes refactors (middleware, different storage, derived selectors) painful.
Why this matters: UI tests should primarily verify what the user can observe (aria-label, icon swap, root class), not internal store shape.
Suggested fix:
it('toggles aria-label when clicked', async () => {
const store = createMockStore('light')
renderWithStore(<ThemeToggle />, store)
const user = userEvent.setup()
const button = screen.getByRole('button', { name: 'Switch to dark mode' })
await user.click(button)
expect(screen.getByRole('button', { name: 'Switch to light mode' })).toBeInTheDocument()
})
| const getInitialTheme = () => { | ||
| if (globalThis.window !== undefined) { | ||
| const savedTheme = globalThis.localStorage.getItem('theme') | ||
| return savedTheme || 'light' | ||
| } | ||
| return 'light' | ||
| } | ||
|
|
||
| const initialState = { | ||
| mode: getInitialTheme(), | ||
| } | ||
|
|
||
| const themeSlice = createSlice({ | ||
| name: 'theme', | ||
| initialState, | ||
| reducers: { | ||
| toggleTheme: (state) => { | ||
| state.mode = state.mode === 'light' ? 'dark' : 'light' | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', state.mode) | ||
| } | ||
| }, | ||
| setTheme: (state, action) => { | ||
| state.mode = action.payload | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', state.mode) | ||
| } | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| export const { toggleTheme, setTheme } = themeSlice.actions | ||
| export const selectThemeMode = (state) => state.theme.mode | ||
| export default themeSlice.reducer |
There was a problem hiding this comment.
Severity: 🟠 Major
Coverage: Missing tests for theme persistence (read/write)
There are no tests verifying (1) initial theme is read from storage and validated, and (2) theme changes are persisted.
Why this matters: Persistence bugs are easy to introduce and hard to notice until a refresh; tests prevent regressions.
Suggested fix (example test cases):
import { describe, it, expect, vi, beforeEach } from 'vitest'
import { configureStore } from '@reduxjs/toolkit'
import themeReducer, { toggleTheme } from './themeSlice'
describe('theme persistence', () => {
beforeEach(() => {
vi.restoreAllMocks()
})
it('uses stored theme when valid', () => {
vi.spyOn(globalThis.localStorage, 'getItem').mockReturnValue('dark')
const store = configureStore({ reducer: { theme: themeReducer } })
expect(store.getState().theme.mode).toBe('dark')
})
it('persists theme on toggle', () => {
const setItemSpy = vi.spyOn(globalThis.localStorage, 'setItem')
const store = configureStore({ reducer: { theme: themeReducer } })
store.dispatch(toggleTheme())
expect(setItemSpy).toHaveBeenCalledWith('theme', store.getState().theme.mode)
})
})(If you move persistence into listener middleware, assert setItem calls via the configured store middleware instead.)
| const themeSlice = createSlice({ | ||
| name: 'theme', | ||
| initialState, | ||
| reducers: { | ||
| toggleTheme: (state) => { | ||
| state.mode = state.mode === 'light' ? 'dark' : 'light' | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', state.mode) | ||
| } | ||
| }, | ||
| setTheme: (state, action) => { | ||
| state.mode = action.payload | ||
| if (globalThis.window !== undefined) { | ||
| globalThis.localStorage.setItem('theme', state.mode) | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Severity: 🟠 Major
Best Practices: Reducers have side effects (writing to localStorage)
Reducers should remain pure; persisting to storage inside reducers makes behavior harder to test/debug and can break tooling assumptions.
Why this matters: Side effects in reducers complicate replay/debugging, can introduce hidden failures, and make unit tests brittle.
Recommended approach: keep reducers pure and move persistence to a listener middleware.
Suggested fix:
// themeSlice.js (reducers become pure)
reducers: {
toggleTheme: (state) => {
state.mode = state.mode === 'light' ? 'dark' : 'light'
},
setTheme: (state, action) => {
state.mode = isThemeMode(action.payload) ? action.payload : 'light'
},
},// store.js
import { configureStore, createListenerMiddleware } from '@reduxjs/toolkit'
import themeReducer, { toggleTheme, setTheme, selectThemeMode } from './slices/themeSlice'
const themeListener = createListenerMiddleware()
themeListener.startListening({
matcher: (action) => toggleTheme.match(action) || setTheme.match(action),
effect: async (action, listenerApi) => {
const mode = selectThemeMode(listenerApi.getState())
try {
globalThis.localStorage?.setItem('theme', mode)
} catch {
// ignore persistence failures
}
},
})
export const store = configureStore({
reducer: { theme: themeReducer },
middleware: (getDefaultMiddleware) =>
getDefaultMiddleware().prepend(themeListener.middleware),
})
| // Apply theme to document on mount and when theme changes | ||
| useEffect(() => { | ||
| const root = document.documentElement | ||
| if (themeMode === 'dark') { | ||
| root.classList.add('dark') | ||
| } else { | ||
| root.classList.remove('dark') | ||
| } | ||
| }, [themeMode]) |
There was a problem hiding this comment.
Severity: 🟠 Major
Architecture: Theme integration split across reducer + App DOM side effects
Theme persistence and DOM class toggling are spread across multiple layers (slice + App effect), which makes the feature harder to extend (e.g., adding “system” mode, syncing color-scheme, analytics, etc.).
Why this matters: Centralizing the “integration” side effects reduces duplication, avoids subtle ordering bugs, and keeps UI components focused on rendering.
Recommended approach: keep themeSlice pure, and use a single listener middleware to (1) persist to storage and (2) toggle document.documentElement.classList.
Suggested fix:
// store.js listener effect (extend persistence listener)
effect: async (action, listenerApi) => {
const mode = selectThemeMode(listenerApi.getState())
if (typeof document !== 'undefined') {
document.documentElement.classList.toggle('dark', mode === 'dark')
}
try {
globalThis.localStorage?.setItem('theme', mode)
} catch {
// ignore
}
}Then App can drop the theme-related useEffect entirely.
| // Apply theme to document on mount and when theme changes | ||
| useEffect(() => { | ||
| const root = document.documentElement | ||
| if (themeMode === 'dark') { | ||
| root.classList.add('dark') | ||
| } else { | ||
| root.classList.remove('dark') | ||
| } | ||
| }, [themeMode]) |
There was a problem hiding this comment.
Severity: 🟠 Major
Coverage: No test verifies .dark class toggling on the root element
The core visible behavior is the .dark class being added/removed on document.documentElement, but there’s no test asserting this behavior.
Why this matters: A regression here would make the UI appear “stuck” in one theme with no failing tests.
Suggested fix (integration-style):
import { render } from '@testing-library/react'
import { Provider } from 'react-redux'
import { configureStore } from '@reduxjs/toolkit'
import themeReducer, { setTheme } from './store/slices/themeSlice'
import App from './App'
it('adds and removes the dark class when theme changes', async () => {
const store = configureStore({ reducer: { theme: themeReducer } })
render(
<Provider store={store}>
<App />
</Provider>
)
store.dispatch(setTheme('dark'))
expect(document.documentElement.classList.contains('dark')).toBe(true)
store.dispatch(setTheme('light'))
expect(document.documentElement.classList.contains('dark')).toBe(false)
})(If you move DOM toggling to listener middleware, keep the same assertion—just ensure the store in the test includes that middleware.)
| export const store = configureStore({ | ||
| reducer: { | ||
| theme: themeReducer, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Architecture: Consider whether Redux is intentional project-wide
If theme is the only global state, a Context + persistence hook can be lighter. If Redux is being introduced as a foundation for more global features, then this is totally fine—just worth confirming the direction.
Why this matters: It affects long-term complexity and how future state is structured.
PR AnalysisFocus Areas for Architect Review
RecommendationAction: REQUEST CHANGES Quick Wins:
Author InsightsPR Type: feat Missing Skills
Strengths
|
chore(setup): initialize counter application with react vite and tailwind
…ings management - Complete refactor of context architecture and tooling migration - Split context into counterContext.js (context + hook) and CounterProvider.jsx (provider component) - Moved Settings component inside CounterProvider for proper encapsulation - Implemented applySettings() action to replace remount pattern - Added useEffect to sync count when initialValue/step change - Created Settings molecule component for settings UI - Counter component uses only state from context, implements handlers locally - App.jsx simplified to just wrap components with CounterProvider - Added parseFiniteNumber validation for empty/non-finite numeric inputs - Removed draft values update settings reactivity - changes only apply on "Apply" click - Removed ESLint, Prettier, and all related packages, and added biome and cspell
- Harden CounterProvider against invalid inputs and memoize context value - Sync draft inputs in Settings to prevent UI drift from source of truth - Replace hard-coded defaults in App with named constants - Update tests to use accessible queries instead of raw text assertions - Refactor CounterContext tests to focus on provider behavior and add integration test - Make package.json check script non-mutating with separate check:fix command
- Extract shared getCount helper in App and Counter test files to avoid duplication - Add proper assertions to CounterContext integration test for workflow validation - Sanitize CounterProvider props against invalid inputs and guard applySettings against null/non-objects
feat(context): implement context api state management for counter app
|
Tip Need another review? Tag me and say rereview for re-analysis after you have fixed all the issues. @cw-pr-agent rereview |
- Add themeSlice with toggleTheme/setTheme and localStorage persistence - Create accessible ThemeToggle button (type, SVG titles) and integrate with Redux - Apply/remove dark class on document.documentElement from App.jsx - Fix imports and provider wiring in main.jsx and tests; wrap tests with Provider/CounterProvider - Tailwind/PostCSS fixes: set dark-mode handling, remove incompatible @config, update index.css - Fix UI styling issues (gradient class, CSS import) and accessibility lint problems - Add/remove temporary debug logs while diagnosing theme behavior - Clear Vite cache and force rebuild to regenerate Tailwind dark variants - All tests passing (50/50)
3 participants
What does this PR do?
What steps does your reviewer have to take to test this PR manually?
feature/redux-theme-managementbranch and runnpm installnpm run devto start the development servernpm testto verify all 35 tests pass (27 existing + 8 new theme tests)Screenshots
Pull Request standards checklist - Please check off
Testing checklist - Please check off
Definition of Done - Please check off