Conversation
WalkthroughAdds a color-mode system to www: a store (defineColorModeStore), provider, picker and watcher components, integrates the provider into app hydration and navigation, adds tests, and updates .gitignore to ignore Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(230,240,255,0.6)
participant App as App (app.tsx)
participant Provider as ColorModeProvider
participant Store as defineColorModeStore()
end
rect rgba(240,255,230,0.6)
participant System as System Prefs (matchMedia)
participant Storage as localStorage
participant DOM as Document (data-mode)
participant Picker as ColorModePicker
end
App->>Store: create store (defineColorModeStore)
Store->>Storage: read 'recharts-color-mode'
Store->>System: query prefers-color-scheme
alt stored value exists
Store->>Store: use stored mode (origin: storage)
else
Store->>Store: derive mode from system (origin: system)
end
Store->>DOM: set data-mode attribute (if enabled)
Store->>System: listen for changes
Store->>Storage: listen for storage events
App->>Provider: wrap Root with Provider(store)
Picker->>Store: subscribe via useSyncExternalStore
Picker->>Picker: render current mode
User->>Picker: click -> dispatch(mode/system)
Picker->>Store: dispatch(action)
alt action is system
Store->>Storage: remove stored key
else set explicit mode
Store->>Storage: write stored mode
end
Store->>DOM: update data-mode
Store->>Picker: notify subscribers -> re-render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6828 +/- ##
==========================================
- Coverage 93.63% 93.61% -0.02%
==========================================
Files 526 531 +5
Lines 47530 47678 +148
Branches 5078 5116 +38
==========================================
+ Hits 44503 44635 +132
- Misses 3020 3036 +16
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8a43ac8 to
f0ad384
Compare
…eProvider and introduce new color mode handling
f0ad384 to
f16332d
Compare
| } catch { | ||
| /* istanbul ignore next */ | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Failed to clear color mode in localStorage, skipping.'); |
There was a problem hiding this comment.
dunno what is the practise here about logging
…te UI accordingly
…ark mode environment
|
I moved the main logic inside an external store to centralise all the things, codecov is mentioning coverage issues that I can address but I think are very rare and stupid corner cases to check, let me know. As I said here: #6828 (comment) to me the work batch its enough to mergeable without waiting all the rest of the work, let me know also on that. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
www/src/app.tsx (1)
11-16: Consider store disposal for complete lifecycle management.The store is created during hydration (line 13) but never disposed. For a typical SPA this is fine since the app remains mounted, but consider whether there are scenarios (tests, HMR, etc.) where the store's event listeners should be cleaned up.
This is likely acceptable for production use but worth considering for completeness.
www/test/color-mode.spec.tsx (1)
17-17: Consider adding explicit return type annotation.The function lacks an explicit return type. While the implicit
voidis correct, the coding guidelines recommend explicit typing for non-trivial functions.🔎 Proposed fix
-function setupEnvironment(props: { preferredColorMode: 'light' | 'dark'; storedColorMode?: 'light' | 'dark' }) { +function setupEnvironment(props: { preferredColorMode: 'light' | 'dark'; storedColorMode?: 'light' | 'dark' }): void {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
www/test/__snapshots__/navigation.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
www/.gitignorewww/src/app.tsxwww/src/components/Navigation.tsxwww/src/components/color-mode/ColorModePicker.tsxwww/src/components/color-mode/ColorModeProvider.tsxwww/src/components/color-mode/ColorModeWatcher.tsxwww/src/components/color-mode/defineColorModeStore.tswww/src/components/color-mode/index.tswww/test/color-mode.spec.tsxwww/test/navigation.spec.tswww/test/navigation.spec.tsx
💤 Files with no reviewable changes (1)
- www/test/navigation.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas constAll imports from
rechartsmust use the public API entry point (e.g.,import { TooltipIndex } from 'recharts'). Imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed and will fail the linter.
Files:
www/src/components/color-mode/ColorModeProvider.tsxwww/src/components/color-mode/index.tswww/src/components/color-mode/ColorModePicker.tsxwww/src/components/color-mode/ColorModeWatcher.tsxwww/test/color-mode.spec.tsxwww/src/components/Navigation.tsxwww/src/app.tsxwww/test/navigation.spec.tsxwww/src/components/color-mode/defineColorModeStore.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
www/src/components/color-mode/ColorModeProvider.tsxwww/src/components/color-mode/index.tswww/src/components/color-mode/ColorModePicker.tsxwww/src/components/color-mode/ColorModeWatcher.tsxwww/test/color-mode.spec.tsxwww/src/components/Navigation.tsxwww/src/app.tsxwww/test/navigation.spec.tsxwww/src/components/color-mode/defineColorModeStore.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When running unit tests, prefer to run a single test file using
npm run test -- path/to/TestFile.spec.tsxrather than running all tests withnpm testUnit tests should be placed in the
testdirectory, with some tests also allowed inwww/test. Test files follow the naming convention*.spec.tsx.
Files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
🧠 Learnings (16)
📚 Learning: 2025-11-25T01:23:14.977Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:23:14.977Z
Learning: ALWAYS unset `NODE_ENV` environment variable when working in this project
Applied to files:
www/.gitignore
📚 Learning: 2025-12-14T13:58:58.361Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6771
File: src/shape/Curve.tsx:39-40
Timestamp: 2025-12-14T13:58:58.361Z
Learning: In the recharts codebase, `useAppSelector` and hooks like `useChartLayout()` are designed to return `undefined` when used outside Redux Provider context, rather than throwing errors. This allows components like `Curve` that call these hooks to work standalone by falling back to prop values.
Applied to files:
www/src/components/color-mode/ColorModeProvider.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use `vi.useFakeTimers()` in all tests due to Redux autoBatchEnhancer dependency on timers and `requestAnimationFrame`
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Verify the number of selector calls using the spy object from `createSelectorTestCase` to spot unnecessary re-renders and improve performance
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to storybook/stories/**/*.stories.tsx : Use Storybook for smoke tests and add play functions with assertions for actual tests
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-12-26T15:59:11.254Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.254Z
Learning: Applies to **/*.spec.{ts,tsx} : Unit tests should be placed in the `test` directory, with some tests also allowed in `www/test`. Test files follow the naming convention `*.spec.tsx`.
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-12-26T15:59:11.254Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.254Z
Learning: Applies to **/*.stories.{ts,tsx} : When adding new Storybook stories, prioritize high-fidelity examples that you want published on the website and in the Storybook UI. Use low-fidelity tests in unit tests or visual regression tests instead to avoid exceeding the Chromatic open source plan limits.
Applied to files:
www/test/color-mode.spec.tsx
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to test/component/**/*.spec.tsx : Use React Testing Library for testing component interactions and behavior upon rendering
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-12-26T15:59:11.254Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.254Z
Learning: Applies to test-vr/**/*.spec.ts : Visual regression tests should follow the naming convention `*.spec.ts` and be placed in the `test-vr` directory. When updating snapshots, new files created in `test-vr/__snapshots__` should be committed to the repository.
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to test/**/*.spec.{ts,tsx} : Aim for 100% unit test code coverage when writing new code
Applied to files:
www/test/color-mode.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Aim for 100% unit test code coverage when writing new code
Applied to files:
www/test/color-mode.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Call `vi.runOnlyPendingTimers()` to advance timers after renders when not using `createSelectorTestCase` helper, and avoid `vi.runAllTimers()` to prevent infinite loops
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : When testing tooltips on hover, use `vi.runOnlyPendingTimers()` after each `userEvent.hover()` call or use the `showTooltip` helper function from `tooltipTestHelpers.ts` to account for requestAnimationFrame delays
Applied to files:
www/test/color-mode.spec.tsx
📚 Learning: 2025-12-16T08:12:06.809Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:06.809Z
Learning: In tests (files under any test directory, e.g., test/, __tests__/, etc.), allow importing internal module paths (e.g., ../../src/...) and do not require imports to use the public API entry point (src/index.ts). The public API import restriction should apply only to non-test code. During reviews, accept internal imports in test files and enforce the public API pattern only for non-test code.
Applied to files:
www/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Mock `getBoundingClientRect` in tests using the helper function provided in `test/helper/MockGetBoundingClientRect.ts`
Applied to files:
www/test/navigation.spec.tsx
🧬 Code graph analysis (7)
www/src/components/color-mode/ColorModeProvider.tsx (1)
www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(49-100)
www/src/components/color-mode/ColorModePicker.tsx (1)
www/src/components/color-mode/ColorModeProvider.tsx (1)
useColorModeStore(23-29)
www/src/components/color-mode/ColorModeWatcher.tsx (2)
www/src/components/color-mode/defineColorModeStore.ts (1)
ColorModeState(3-6)www/src/components/color-mode/ColorModeProvider.tsx (1)
useColorModeStore(23-29)
www/test/color-mode.spec.tsx (4)
www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(49-100)www/src/components/color-mode/ColorModeProvider.tsx (1)
ColorModeProvider(7-15)www/src/components/color-mode/ColorModePicker.tsx (1)
ColorModePicker(5-29)www/src/components/color-mode/ColorModeWatcher.tsx (1)
ColorModeWatcher(18-22)
www/src/components/Navigation.tsx (1)
www/src/components/color-mode/ColorModePicker.tsx (1)
ColorModePicker(5-29)
www/src/app.tsx (3)
www/src/components/color-mode/ColorModeProvider.tsx (1)
ColorModeProvider(7-15)www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(49-100)www/src/containers/Root.tsx (1)
Root(5-12)
www/test/navigation.spec.tsx (3)
www/src/navigation.ts (2)
getAllNavigationItems(107-131)normalizePathnameToLocale(138-147)www/src/components/color-mode/ColorModeProvider.tsx (1)
ColorModeProvider(7-15)www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(49-100)
🔇 Additional comments (10)
www/.gitignore (1)
11-11: LGTM!Adding
*.localto gitignore aligns with Vite's recommended practice for handling local environment files. This enables developers to test features locally (via.env.local) without accidentally committing sensitive or environment-specific configuration.www/src/components/color-mode/defineColorModeStore.ts (3)
11-37: Good defensive localStorage handling.The error handling with try-catch blocks and console warnings is appropriate for localStorage operations, which can fail in various scenarios (quota exceeded, privacy mode, etc.). The validation in
getStoredColorMode(lines 14-15) also prevents invalid values from being used.
69-72: Verify the intent of clearing stored preference on system change.When the system color scheme changes, this code clears any explicitly stored user preference (line 70). This means if a user manually selected "light mode" and their system preference later changes to dark, their explicit choice is lost and they'd need to re-select it.
This behavior might be intentional, but consider whether it's the desired UX. An alternative would be to only listen to system changes when
origin === 'system'(no stored preference exists).
77-99: Well-designed store API.The store interface is cleanly designed for React's
useSyncExternalStorehook, with proper:
getSnapshot()for reading current statesubscribe()returning an unsubscribe functiondispose()for cleanupdispatch()for state mutationsThe use of
as const(line 99) is appropriate here.www/src/components/color-mode/ColorModeWatcher.tsx (1)
1-22: LGTM! Clean render prop pattern.This component follows the render prop pattern correctly and uses
useSyncExternalStoreappropriately. The documentation comment is helpful and includes a clear example.www/src/components/color-mode/ColorModeProvider.tsx (1)
1-29: LGTM! Standard React context pattern.The provider and hook implementation follows standard React patterns. The error thrown by
useColorModeStore(line 26) when used outside the provider is appropriate here, as the color mode system requires the provider to be present, unlike recharts components that can work standalone.The documentation comment (lines 20-21) helpfully directs users to
ColorModeWatcherfor general use cases.www/src/components/Navigation.tsx (1)
8-8: LGTM! Clean feature flag integration.The ColorModePicker is appropriately integrated into both desktop and mobile navigation behind the
VITE_IS_DARKMODE_ENABLEDfeature flag. The implementation is consistent across both navigation contexts.Also applies to: 27-27, 151-151
www/src/components/color-mode/ColorModePicker.tsx (1)
5-29: LGTM! Clean toggle implementation.The color mode cycling logic is clear and well-structured (system → light → dark → system). The use of
React.ComponentPropsWithRef<'button'>properly types the button props, and the component correctly integrates with the store viauseSyncExternalStore.www/src/components/color-mode/index.ts (1)
1-4: LGTM! Clean barrel export.This barrel export consolidates the color-mode public API, allowing consumers to import from a single entry point. This is a good practice for module organization.
www/test/color-mode.spec.tsx (1)
1-270: Comprehensive test coverage with excellent structure.The test suite thoroughly exercises the color mode system with well-organized test cases covering all major scenarios:
- System and storage origins with light/dark modes
- Cross-tab synchronization via storage events
- System preference changes
- Listener subscription lifecycle
- UI components (ColorModePicker, ColorModeWatcher)
The tests use appropriate patterns with React Testing Library, clear assertions, and good separation of concerns.
Bundle ReportBundle size has no change ✅ |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
www/src/components/color-mode/defineColorModeStore.ts (1)
38-44: Consider removing redundantas constassertion.The
as constassertion on line 43 is redundant since the function already has an explicit return typeColorModeState. The object literal already matches the expected type without the assertion.🔎 Proposed fix
function getColorModeState(): ColorModeState { const storedMode = getStoredColorMode(); return { origin: storedMode != null ? 'storage' : 'system', mode: storedMode ?? (window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'), - } as const; + }; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
www/src/components/color-mode/defineColorModeStore.tswww/test/color-mode.spec.tsxwww/test/navigation.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- www/test/navigation.spec.tsx
- www/test/color-mode.spec.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas constAll imports from
rechartsmust use the public API entry point (e.g.,import { TooltipIndex } from 'recharts'). Imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed and will fail the linter.
Files:
www/src/components/color-mode/defineColorModeStore.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
www/src/components/color-mode/defineColorModeStore.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, Test, Pack
|
This should get deployed to staging https://recharts.github.io/recharts/canary/www/ in couple minutes: https://github.com/recharts/recharts/actions/runs/20586840515 |
Description
Add
ColorModePickermodule (behind a feature flag for now), that aims to let the end user control the current color-mode of the docs.Related Issue
Motivation and Context
Follow up of #6825
How Has This Been Tested?
Screenshots (if appropriate):
TBD
Checklist:
ColorModePickermoduleSummary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.