Conversation
WalkthroughInitializes a mode-aware color system and color-mode store with server snapshot support, integrates ColorModeProvider into root and SSR, migrates many styles to new CSS tokens, updates components to use the new tokens, and adds related tests including a visual-regression spec for dark mode. Changes
Sequence Diagram(s)sequenceDiagram
participant SSR as Server (SSR)
participant Store as ColorModeStore
participant Provider as ColorModeProvider
participant Router as StaticRouter / Routes
participant Browser as Client
SSR->>Store: defineColorModeStore()
Store-->>SSR: return store (getSnapshot, getServerSnapshot)
SSR->>Provider: wrap Router with store
Provider->>Router: render routes -> HTML
SSR-->>Browser: HTML + CSS variables (data-mode)
Browser->>Store: init client store (defineColorModeStore)
Store->>Store: subscribeToStorageAndSystemChanges()
Store->>Browser: emit state changes
Browser->>DOM: apply [data-mode] attributes and CSS variables
sequenceDiagram
participant User
participant Picker as ColorModePicker
participant Store as ColorModeStore
participant System as System Preference
participant DOM as Document
System->>Store: prefers-color-scheme change
Store->>DOM: set [data-mode]=light|dark
User->>Picker: select mode ("system"|"dark"|"light")
Picker->>Store: dispatch(mode)
Store->>Store: update state + localStorage
Store->>DOM: emit state change -> update [data-mode]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 #6854 +/- ##
==========================================
+ Coverage 93.98% 94.27% +0.29%
==========================================
Files 539 561 +22
Lines 53121 53641 +520
Branches 5158 5178 +20
==========================================
+ Hits 49924 50571 +647
+ Misses 3188 3061 -127
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We have playwright tests set up, would you like to add one or two tests for dark mode specifically? |
I'm getting this error from playwright (probably linked to some ESM vs CommonJS issue?), it seems to not accepting the export interface ColorModeState {
mode: ColorMode;
origin: ColorModeOrigin;
}from I need to investigate on it |
7f6c7b3 to
e6a5825
Compare
Bundle ReportChanges will decrease total bundle size by 1.57MB (-56.57%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
e6a5825 to
7210d3f
Compare
7210d3f to
9e0bdcb
Compare
…ce SVG icon with react-icons
…ariables for improved theming
There was a problem hiding this comment.
@PavelVanecek dunno if this is sufficient and it was what you expected
|
@PavelVanecek I think its ready, let me know |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @www/src/entry-server.tsx:
- Around line 9-13: The ColorMode store created by defineColorModeStore() is
never disposed during SSR causing retained subscriptions; fix by assigning the
store to a variable (e.g., const colorModeStore = defineColorModeStore()), pass
that variable into <ColorModeProvider store={colorModeStore}> for
renderToString, then immediately after rendering call the store's cleanup method
(e.g., colorModeStore.dispose() or the appropriate unsubscribe/destroy method
implemented for subscribeToStorageAndSystemChanges) to remove listeners and
prevent access to browser APIs on the server.
In @www/test/routes/index.spec.tsx:
- Around line 77-85: The test setup currently stubs matchMedia but doesn't
enable fake timers or clean up the stub; update the beforeEach in the test file
to call vi.useFakeTimers() before stubbing matchMedia (so Redux
autoBatchEnhancer works), and add an afterEach that calls vi.useRealTimers() and
removes the global stub (use vi.unstubGlobal('matchMedia') or delete (global as
any).matchMedia) to avoid polluting other tests.
- Around line 10-14: The ColorMode store created by defineColorModeStore() is
never disposed, causing memory leaks; update the test to capture the store in a
variable (e.g., const store = defineColorModeStore()), pass that store into
<ColorModeProvider store={store}> instead of calling defineColorModeStore
inline, and ensure you call store.dispose() in your test teardown (afterEach or
finally block) or after unmounting the render result; reference the
functions/props defineColorModeStore, ColorModeProvider and the store.dispose()
method when making the change.
🧹 Nitpick comments (4)
www/src/views/IndexView/IndexView.css (1)
133-134: Consider migrating remaining hardcoded colors for completeness.While not critical, lines 133-134 still use hardcoded color values (
#f5f5f5andrgba(0, 0, 0, 0.1)) for the hover state. For full dark mode support, these could be migrated to CSS variables likevar(--color-surface-hover)and a themed shadow variable.www/src/views/ExamplesIndexView.css (1)
25-25: Consider theming the remaining hardcoded colors.For complete dark mode coverage, consider migrating:
- Line 25:
box-shadow: 0 4px 12px rgba(0, 0, 0, 0.1)to a themed shadow variable- Lines 48-49: The gradient background could benefit from theming or verification that it looks acceptable in both modes
Also applies to: 48-49
www/src/layouts/frame.css (1)
4-4: Consider theming the box-shadow for completeness.Line 4's
box-shadow: 0 0 4px rgba(0, 0, 0, 0.25)uses a hardcoded shadow color. For full dark mode support, this could be migrated to a themed shadow variable or adjusted to work well in both light and dark modes.www/src/utils/CodeMirrorEditor.css (1)
3-3: LGTM! Good adoption of semantic color tokens.The migration to CSS variables is well-executed:
--color-surface-sunkencorrectly indicates a recessed toolbar area--color-border-2and--color-text-2are appropriate for secondary UI elements- Brand color on hover maintains consistency with the design system
Minor nit: Line 29 still uses hardcoded
color: white. Consider using a token likevar(--color-white)for consistency with the primitive color system, though this is purely stylistic since white on brand backgrounds is typically safe.Also applies to: 17-22, 27-29
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
test-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-webkit-linux.pngis excluded by!**/*.pngwww/src/components/Shared/targetBlank.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
test-vr/tests/www/dark-mode.spec-vr.tsxwww/src/app.tsxwww/src/components/Navigation.tsxwww/src/components/Playground/SourceCodeEditor.tsxwww/src/components/Shared/SidebarNav/SidebarNav.csswww/src/components/Shared/TargetBlankLink.csswww/src/components/Shared/TargetBlankLink.module.csswww/src/components/Shared/TargetBlankLink.tsxwww/src/components/color-mode/ColorModePicker.module.csswww/src/components/color-mode/ColorModePicker.tsxwww/src/components/color-mode/ColorModeWatcher.tsxwww/src/components/color-mode/defineColorModeStore.tswww/src/components/navigation.csswww/src/containers/Root.tsxwww/src/entry-server.tsxwww/src/layouts/RechartsLogo.tsxwww/src/layouts/frame.csswww/src/styles/_scaffolding.csswww/src/styles/_variables.csswww/src/styles/container.csswww/src/utils/CodeMirrorEditor.csswww/src/utils/CopyIcon.tsxwww/src/views/ExampleView.csswww/src/views/ExamplesIndexView.csswww/src/views/IndexView/IndexView.csswww/src/views/IndexView/index.tsxwww/test/color-mode.client.spec.tsxwww/test/color-mode.server.spec.tsxwww/test/navigation.spec.tsxwww/test/routes/index.spec.tsx
💤 Files with no reviewable changes (2)
- www/test/navigation.spec.tsx
- www/src/components/Shared/TargetBlankLink.css
🧰 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/test/color-mode.client.spec.tsxwww/test/routes/index.spec.tsxwww/src/utils/CopyIcon.tsxwww/src/layouts/RechartsLogo.tsxwww/src/components/color-mode/ColorModeWatcher.tsxwww/src/views/IndexView/index.tsxtest-vr/tests/www/dark-mode.spec-vr.tsxwww/src/components/Navigation.tsxwww/src/components/Playground/SourceCodeEditor.tsxwww/test/color-mode.server.spec.tsxwww/src/components/color-mode/ColorModePicker.tsxwww/src/components/Shared/TargetBlankLink.tsxwww/src/entry-server.tsxwww/src/containers/Root.tsxwww/src/components/color-mode/defineColorModeStore.tswww/src/app.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
www/test/color-mode.client.spec.tsxwww/test/routes/index.spec.tsxwww/src/utils/CopyIcon.tsxwww/src/layouts/RechartsLogo.tsxwww/src/components/color-mode/ColorModeWatcher.tsxwww/src/views/IndexView/index.tsxtest-vr/tests/www/dark-mode.spec-vr.tsxwww/src/components/Navigation.tsxwww/src/components/Playground/SourceCodeEditor.tsxwww/test/color-mode.server.spec.tsxwww/src/components/color-mode/ColorModePicker.tsxwww/src/components/Shared/TargetBlankLink.tsxwww/src/entry-server.tsxwww/src/containers/Root.tsxwww/src/components/color-mode/defineColorModeStore.tswww/src/app.tsx
**/*.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.client.spec.tsxwww/test/routes/index.spec.tsxwww/test/color-mode.server.spec.tsx
🧠 Learnings (20)
📚 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.client.spec.tsxwww/test/routes/index.spec.tsxtest-vr/tests/www/dark-mode.spec-vr.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.client.spec.tsxwww/test/routes/index.spec.tsxtest-vr/tests/www/dark-mode.spec-vr.tsxwww/test/color-mode.server.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.client.spec.tsxwww/test/routes/index.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.client.spec.tsxwww/test/routes/index.spec.tsxtest-vr/tests/www/dark-mode.spec-vr.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.client.spec.tsxwww/test/routes/index.spec.tsxtest-vr/tests/www/dark-mode.spec-vr.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.client.spec.tsxwww/test/routes/index.spec.tsxtest-vr/tests/www/dark-mode.spec-vr.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 : Update Storybook stories when APIs have been changed to ensure they work as expected
Applied to files:
www/test/color-mode.client.spec.tsxwww/test/routes/index.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} : Use the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
Applied to files:
www/test/color-mode.client.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} : Use `userEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })` or the `userEventSetup` helper function from `test/helper/userEventSetup.ts` when creating userEvent instances
Applied to files:
www/test/color-mode.client.spec.tsxwww/test/routes/index.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.client.spec.tsxwww/test/routes/index.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.client.spec.tsxwww/test/routes/index.spec.tsxtest-vr/tests/www/dark-mode.spec-vr.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.client.spec.tsxwww/test/routes/index.spec.tsxwww/test/color-mode.server.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/routes/index.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 **/*.{ts,tsx} : All imports from `recharts` must use the public API entry point (e.g., `import { TooltipIndex } from 'recharts'`). Imports from internal paths like `recharts/types/*` or `recharts/src/*` are not allowed and will fail the linter.
Applied to files:
www/src/layouts/RechartsLogo.tsx
📚 Learning: 2025-11-19T14:08:01.728Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6659
File: www/src/components/GuideView/Performance/index.tsx:232-250
Timestamp: 2025-11-19T14:08:01.728Z
Learning: In Recharts version 3.4.2, object-as-prop optimizations were introduced to reduce unnecessary re-renders when new object references are passed as props. This affects the recommendation for the `react-perf/jsx-no-new-object-as-prop` ESLint rule.
Applied to files:
www/src/layouts/RechartsLogo.tsx
📚 Learning: 2025-12-16T08:12:13.355Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:13.355Z
Learning: In the recharts codebase, files in the `test` folder are allowed to import from internal paths (e.g., `../../src/state/cartesianAxisSlice`) and do not need to use the public API entry point (`src/index.ts`). The public API import restriction applies only to non-test code.
Applied to files:
www/src/layouts/RechartsLogo.tsx
📚 Learning: 2025-11-23T13:30:10.395Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.395Z
Learning: The `TooltipIndex` type from recharts is defined in `src/state/tooltipSlice.ts` but is not currently exported from the public API entry points. It should not be imported from `recharts/types/state/tooltipSlice` as this is an internal implementation path. An ESLint rule is needed to prevent regressions.
Applied to files:
www/src/layouts/RechartsLogo.tsx
📚 Learning: 2025-12-25T23:55:37.579Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6824
File: package.json:0-0
Timestamp: 2025-12-25T23:55:37.579Z
Learning: In the recharts repository, the `engines` field in package.json specifies requirements for library consumers and should only be changed with major version releases to avoid breaking changes. For development environment Node version requirements, use the `.nvmrc` file instead, which standardizes the development environment without impacting library consumers.
Applied to files:
www/src/layouts/RechartsLogo.tsx
📚 Learning: 2026-01-06T22:33:55.414Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6855
File: www/src/docs/api/AreaChartAPI.tsx:422-438
Timestamp: 2026-01-06T22:33:55.414Z
Learning: In Recharts 3, components can render inside any chart type as long as the parent provides the necessary context. For example, Funnel can render inside AreaChart, BarChart, ComposedChart, LineChart, and ScatterChart because they all provide cartesian context. This is a change from Recharts 2, which had stricter parent-child component limitations.
Applied to files:
www/src/layouts/RechartsLogo.tsxwww/src/components/Navigation.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:
test-vr/tests/www/dark-mode.spec-vr.tsxwww/test/color-mode.server.spec.tsx
🧬 Code graph analysis (6)
www/test/routes/index.spec.tsx (3)
www/src/components/color-mode/ColorModeProvider.tsx (1)
ColorModeProvider(7-15)www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(82-119)www/src/routes/index.tsx (1)
routes(6-125)
test-vr/tests/www/dark-mode.spec-vr.tsx (1)
www/src/containers/Root.tsx (1)
Root(8-28)
www/src/components/Navigation.tsx (1)
www/src/components/color-mode/ColorModePicker.tsx (1)
ColorModePicker(18-44)
www/test/color-mode.server.spec.tsx (1)
www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(82-119)
www/src/containers/Root.tsx (2)
www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(82-119)www/src/components/color-mode/ColorModeProvider.tsx (1)
ColorModeProvider(7-15)
www/src/app.tsx (1)
www/src/containers/Root.tsx (1)
Root(8-28)
⏰ 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
🔇 Additional comments (38)
www/src/utils/CopyIcon.tsx (1)
9-9: LGTM! Color token migration aligns with PR objectives.The stroke color update from
var(--text-color)tovar(--color-text-1)correctly migrates to the new CSS variable system introduced in this PR for dark mode support.www/src/layouts/RechartsLogo.tsx (1)
37-37: LGTM! CSS variable updates are consistent and correct.The updates from
--brand-primaryto--color-solid-brand-1align with the new color mode token system introduced in this PR. Bothstrokeandfillproperties have been consistently updated with appropriate fallback values.Also applies to: 61-61
www/src/components/color-mode/ColorModePicker.module.css (1)
9-11: LGTM! Clean migration to CSS variable.The hover color change from a hardcoded value to
var(--color-text-1)correctly integrates with the new theming system.www/src/views/IndexView/index.tsx (1)
67-67: LGTM! Consistent theming migration.The inline style color update from
'black'to'var(--color-text-1)'aligns with the broader color mode implementation.www/src/views/IndexView/IndexView.css (1)
9-9: LGTM! Consistent color variable migration.All four color updates correctly use the new theming variables (
--color-border-3,--color-solid-info-1,--color-text-2), maintaining consistency with the dark mode implementation.Also applies to: 34-34, 39-39, 137-137
www/src/views/ExamplesIndexView.css (1)
15-16: LGTM! Comprehensive color variable migration.All six color updates properly utilize the new theming system (
--color-surface-base,--color-border-3,--color-solid-brand-1,--color-surface-sunken,--color-text-1), ensuring consistent dark mode support across the examples grid.Also applies to: 26-26, 32-32, 36-36, 60-60
www/src/layouts/frame.css (1)
3-3: LGTM! Clean header theming migration.Both color updates (
--color-surface-raisedfor the header background and--color-solid-info-1for the logo link) correctly integrate with the new color mode system.Also applies to: 23-23
www/src/views/ExampleView.css (1)
16-16: LGTM! Color token migration aligns with the new theming system.The change from
--brand-primaryto--color-solid-brand-1is consistent with the comprehensive color token overhaul for dark mode support.www/src/styles/container.css (1)
21-21: LGTM! Semantic color token updates align with the new system.The migration from legacy tokens (
--title-color,--border-color) to semantic tokens (--color-text-1,--color-border-3) properly supports the color mode architecture.Also applies to: 29-29
www/test/color-mode.client.spec.tsx (1)
1-1: LGTM! Import cleanup reflects the removal of feature flag stubbing.The removal of
beforeEachfrom imports is correct since the feature flag stub forVITE_IS_DARKMODE_ENABLEDis no longer needed, aligning with dark mode being made a permanent feature. The test suite maintains comprehensive coverage of color mode functionality.www/src/components/Navigation.tsx (2)
28-28: LGTM! ColorModePicker now permanently available in navigation.The removal of conditional rendering aligns with dark mode being made a permanent feature. The
ColorModePickercomponent relies onuseColorModeStore()which expects aColorModeProviderancestor—this dependency is satisfied by the provider setup inRoot.tsxas indicated in the PR changes.Also applies to: 154-154
42-42: LGTM! Color token update aligns with the new theming system.The migration from
--brand-infoto--color-solid-info-1is consistent with the comprehensive color token overhaul for dark mode support.www/src/components/Shared/SidebarNav/SidebarNav.css (1)
26-26: LGTM! Color token migration supports dark mode theming.The update to
--color-solid-brand-1for both hover and active link states aligns with the new semantic token system and enables proper dark mode support.Also applies to: 31-31
test-vr/tests/www/dark-mode.spec-vr.tsx (2)
10-15: LGTM - Visual regression test structure is appropriate.The test correctly mounts the Root component, waits for the UI to be ready, and captures a screenshot for visual regression. The assertion for the 'system' button ensures the color mode picker is rendered before capturing the snapshot.
1-15: Verify the file naming convention for visual regression tests.Based on learnings, visual regression tests should follow the naming convention
*.spec.ts. This file is nameddark-mode.spec-vr.tsxwith a-vrsuffix and.tsxextension. While the.tsxextension is necessary for JSX syntax, please confirm whether the-vrsuffix aligns with the project's naming conventions for visual regression tests.Based on learnings, visual regression test naming conventions should be verified.
www/test/color-mode.server.spec.tsx (2)
1-4: LGTM - Correct server-side test setup.The
@vitest-environment nodedirective is correctly used to test the color mode store in a server environment, ensuring it works without browser APIs.
6-13: LGTM - Server-side store behavior is correctly tested.The test validates that the color mode store can be instantiated in a Node environment and returns the expected default state (system origin with light mode). The cleanup via
dispose()is properly performed.www/src/components/color-mode/ColorModePicker.tsx (1)
20-20: LGTM - Correct SSR pattern for useSyncExternalStore.Adding
store.getServerSnapshotas the third parameter touseSyncExternalStoreis the correct pattern for supporting server-side rendering and hydration. This ensures the color mode state is consistent between server and client renders.www/src/app.tsx (1)
10-10: LGTM - Provider responsibility correctly moved to Root component.The refactoring to remove
ColorModeProviderfromapp.tsxis correct, as theRootcomponent now handles the provider setup internally (including store initialization and cleanup). This provides cleaner separation of concerns and enables consistent server-side rendering.www/src/components/color-mode/ColorModeWatcher.tsx (1)
20-20: LGTM - Correct SSR pattern for useSyncExternalStore.Adding
store.getServerSnapshotas the third parameter ensures proper server-side rendering and hydration support. This change is consistent with the pattern used inColorModePicker.tsxand follows React 18's SSR best practices.www/src/containers/Root.tsx (2)
9-17: LGTM! Proper store lifecycle management.The color mode store is correctly initialized once via
useStateand properly cleaned up on unmount through thedispose()method. This ensures no memory leaks from event listeners or storage subscriptions.
20-26: LGTM! Correct provider integration.The
ColorModeProviderwrapper correctly provides the store to all child components, ensuring consistent color mode context throughout the application.www/src/styles/_scaffolding.css (1)
31-32: LGTM! CSS token migration for dark mode support.The CSS variable updates correctly migrate from the old naming convention to the new color-mode-aware token system. The new tokens (e.g.,
--color-text-3,--color-surface-base,--color-solid-brand-1) follow a more systematic naming pattern that supports light and dark modes.Also applies to: 48-48, 52-52, 119-120
www/src/components/Playground/SourceCodeEditor.tsx (2)
18-41: LGTM! Syntax highlighting migrated to themeable CSS variables.The hardcoded color values have been properly replaced with CSS variable references, enabling dynamic theming for dark mode support. The tag-to-color mappings remain functionally unchanged.
146-149: LGTM! Editor gutters now use themeable colors.The gutter background and border colors correctly reference the new CSS variable system, ensuring the editor UI adapts to the selected color mode.
www/src/entry-server.tsx (1)
9-13: No action needed. ThedefineColorModeStoreimplementation is already properly guarded for SSR withtypeof window === 'undefined'checks ingetColorModeState(),typeof document === 'undefined'checks inupdateColorModeInDOM(), andtypeof window === 'undefined'checks insubscribeToStorageAndSystemChanges(). Browser API access (localStorage, window.matchMedia, document) is prevented during server-side rendering.Likely an incorrect or invalid review comment.
www/src/components/navigation.css (1)
23-23: LGTM! Token migration is correct.The color variable replacements properly map the old semantic tokens to the new color system:
--brand-info→--color-solid-info-1for interactive/accent elements--brand-primary→--color-solid-brand-1for brand-colored active states#fff→--color-surface-basefor adaptive backgroundThese changes align with the new light/dark mode token definitions in
_variables.css.Also applies to: 30-30, 93-94, 109-109, 135-135
www/src/components/color-mode/defineColorModeStore.ts (3)
38-50: LGTM! Proper SSR handling with safe defaults.The SSR guard returns a sensible default state (
lightmode fromsystemorigin) whenwindowis unavailable. Theas constassertion on line 50 is appropriate here per guidelines.
68-71: Verify: System preference change clears user's explicit choice.When the OS color scheme changes,
clearStoredColorMode()is called, which erases any explicit user preference from localStorage. This means if a user manually selects "dark mode" but their OS is set to light, and then they change their OS to dark mode, their explicit "dark" preference is cleared.Is this the intended behavior? Alternative approach: only update the mode without clearing storage, preserving the user's explicit choice.
82-118: LGTM! Well-structured store implementation.The store correctly implements the
useSyncExternalStorecontract:
getSnapshotandgetServerSnapshotreturn consistent statesubscribeproperly manages listener lifecycledispatchhandles both explicit mode selection and system fallbackdisposeexposes cleanup for proper resource managementThe immediate
updateColorModeInDOM()call at line 94 ensures DOM synchronization at store creation, with the document guard preventing SSR issues.www/src/styles/_variables.css (4)
5-18: LGTM! Well-structured primitive color scale.The gray scale from 50-950 provides good granularity for building semantic tokens. The naming follows established conventions (similar to Tailwind's color system).
38-41: Good approach: Light mode as default.Using
:root, [data-mode='light']ensures light mode tokens are the default fallback, which is correct for cases where thedata-modeattribute hasn't been set yet (e.g., during initial render before JS hydration).
59-70: Verify: Chart colors are identical in light and dark modes.The
--color-chart-*tokens have the same values in both light and dark mode blocks. Is this intentional for brand consistency, or should the dark mode have adjusted values for better contrast against dark backgrounds?Some of these colors (e.g.,
#413ea0for chart-9) may have reduced visibility on dark backgrounds.Also applies to: 104-115
84-127: LGTM! Dark mode semantic tokens are well-defined.The dark mode properly inverts the surface hierarchy and adjusts text/border colors for appropriate contrast. Code block colors are thoughtfully adjusted for dark backgrounds.
www/src/components/Shared/TargetBlankLink.module.css (1)
1-6: LGTM! Clean CSS module for link+icon layout.The flexbox layout with center alignment and gap is appropriate for displaying a link with an inline external-link icon. The
nowrapprevents the icon from separating from the text on line breaks.www/src/components/Shared/TargetBlankLink.tsx (3)
1-4: LGTM! Imports are clean.The imports are well-organized: React for type annotations, the external link icon from react-icons, and the CSS module for styling.
6-10: LGTM! Type definition is explicit and consistent.The prop types are well-defined with explicit typing. Using
React.ReactNodeis consistent withReact.MouseEventfor the onClick handler.
14-17: Addaria-hidden="true"to the icon for better accessibility.The
<LuExternalLink />icon on line 16 is decorative since the visible link text already describes the destination. Mark the icon asaria-hidden="true"to prevent redundant announcements to screen readers.♿ Proposed accessibility fix
<a href={href} target="_blank" rel="noopener noreferrer" onClick={onClick} className={styles.anchor}> {children} - <LuExternalLink /> + <LuExternalLink aria-hidden="true" /> </a>
| beforeEach(() => { | ||
| vi.stubGlobal('matchMedia', (query: string) => ({ | ||
| media: query, | ||
| matches: false, | ||
| addEventListener: vi.fn(), | ||
| removeEventListener: vi.fn(), | ||
| dispatchEvent: vi.fn(), | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Add fake timers setup and cleanup matchMedia stub.
Based on learnings, all tests should use vi.useFakeTimers() due to Redux autoBatchEnhancer dependency. Additionally, the matchMedia stub should be cleaned up after tests to avoid polluting the global environment.
⏱️ Proposed fix to add fake timers and cleanup
describe('routes', () => {
beforeEach(() => {
+ vi.useFakeTimers();
vi.stubGlobal('matchMedia', (query: string) => ({
media: query,
matches: false,
addEventListener: vi.fn(),
removeEventListener: vi.fn(),
dispatchEvent: vi.fn(),
}));
});
+
+ afterEach(() => {
+ vi.restoreAllMocks();
+ vi.useRealTimers();
+ });Based on learnings, use vi.useFakeTimers() in all tests due to Redux autoBatchEnhancer dependency.
📝 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.
| beforeEach(() => { | |
| vi.stubGlobal('matchMedia', (query: string) => ({ | |
| media: query, | |
| matches: false, | |
| addEventListener: vi.fn(), | |
| removeEventListener: vi.fn(), | |
| dispatchEvent: vi.fn(), | |
| })); | |
| }); | |
| beforeEach(() => { | |
| vi.useFakeTimers(); | |
| vi.stubGlobal('matchMedia', (query: string) => ({ | |
| media: query, | |
| matches: false, | |
| addEventListener: vi.fn(), | |
| removeEventListener: vi.fn(), | |
| dispatchEvent: vi.fn(), | |
| })); | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| vi.useRealTimers(); | |
| }); |
🤖 Prompt for AI Agents
In @www/test/routes/index.spec.tsx around lines 77 - 85, The test setup
currently stubs matchMedia but doesn't enable fake timers or clean up the stub;
update the beforeEach in the test file to call vi.useFakeTimers() before
stubbing matchMedia (so Redux autoBatchEnhancer works), and add an afterEach
that calls vi.useRealTimers() and removes the global stub (use
vi.unstubGlobal('matchMedia') or delete (global as any).matchMedia) to avoid
polluting other tests.
There was a problem hiding this comment.
there is a vitest config option called mockRestore that should restore the mocks automatically
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for Vitest configuration file to see if mockRestore or restoreMocks is configured
fd -e config.js -e config.ts vitest.config
fd -e config.js -e config.ts vite.config
Length of output: 18
🏁 Script executed:
#!/bin/bash
# Check the Vitest configuration to see if mockRestore is already configured
cat www/vite.config.ts
Length of output: 2895
🏁 Script executed:
#!/bin/bash
# Search for vitest test configuration in package.json or other config files
cat www/package.json | jq '.vitest // empty'
# Also check if there's a test config embedded in vite.config
grep -A 20 "test:" www/vite.config.ts
# Look for any vitest.config files in the www directory
find www -name "vitest.config.*" -o -name "vitest.*.config.*"
Length of output: 0
🌐 Web query:
Vitest restoreMocks mockRestore configuration option
💡 Result:
What it does
- restoreMocks: true makes Vitest call .mockRestore() on all mocks/spies before each test — this clears call history, restores implementations to their originals, and (for spies) restores original property descriptors. [1][2]
How it differs from other options
- clearMocks: true only calls .mockClear() (clears call history).
- mockReset: true calls .mockReset() (clears history and resets implementation to an empty function or the original implementation for vi.fn(impl)). restoreMocks additionally restores original descriptors for spied-on object methods. [2][3]
Enable in config (example)
- vitest.config.ts:
export default defineConfig({
test: { restoreMocks: true }
})
This will run mockRestore automatically before each test. [1]
Sources
[1] Vitest config (restoreMocks).
[2] Vitest mock API (mockRestore behavior).
[3] Vitest config (mockReset / clearMocks descriptions).
@cloud-walker, you're absolutely right! Vitest has a restoreMocks configuration option that can automatically call .mockRestore() before each test, which would handle the cleanup of vi.stubGlobal('matchMedia') automatically.
However, I noticed that your www/vite.config.ts doesn't currently have a test: configuration block with this option enabled. You can add it like this:
export default defineConfig(() => ({
// ... existing config
test: {
restoreMocks: true,
},
}));One important note: while restoreMocks handles mock cleanup, fake timers need separate cleanup with vi.useRealTimers(). Based on the project's testing patterns, you'd still need:
beforeEach(() => {
vi.useFakeTimers();
vi.stubGlobal('matchMedia', (query: string) => ({
media: query,
matches: false,
addEventListener: vi.fn(),
removeEventListener: vi.fn(),
dispatchEvent: vi.fn(),
}));
});
afterEach(() => {
vi.useRealTimers(); // Still needed - not covered by restoreMocks
// vi.restoreAllMocks() can be removed if you add restoreMocks: true to config
});So with restoreMocks: true in the config, you can skip the manual vi.restoreAllMocks() call, but you'll still need the vi.useRealTimers() cleanup for fake timers.
🧠 Learnings used
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`
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
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
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
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`
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
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 `userEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })` or the `userEventSetup` helper function from `test/helper/userEventSetup.ts` when creating userEvent instances
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 the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
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`.
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
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.
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
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Add tests when adding new functionality or changing existing functionality; prefer unit tests for helper functions or RTL tests for rendering
|
need to run vr tests and update snapshots it looks like though? |
|
I'm not a vr testing expert, maybe I need to lax a bit the precision of the diff? |
|
Your VR test has whole page on it, it will fail on every single change and it will fail on the github star counter. Make a test of a single chart instead. |
|
I discovered: toHaveScreenshot({mask: [...]}) I think it may be better to keep the home screenshot to cover more component types, but let me know |
|
lets also see if I fixed the issue with just the github star button masking |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
www/test/routes/index.spec.tsx (1)
9-18: Consider usingafterEachfor more robust cleanup.The current pattern of calling
dispose()at the end of each test helper works, but if an assertion fails,dispose()won't be called. Consider extracting cleanup to anafterEachhook for more reliable resource management.♻️ Optional refactor using afterEach
+let disposeColorModeStore: (() => void) | null = null; + function baseRender(url: string) { const colorModeStore = defineColorModeStore(); + disposeColorModeStore = colorModeStore.dispose; const renderResult = render( <ColorModeProvider store={colorModeStore}> <MemoryRouter initialEntries={[url]}>{routes()}</MemoryRouter> </ColorModeProvider>, ); expect(renderResult.getByRole('main')).toBeInTheDocument(); - return { screen: renderResult, dispose: colorModeStore.dispose } as const; + return { screen: renderResult } as const; }Then in the describe block:
describe('routes', () => { beforeEach(() => { vi.stubGlobal('matchMedia', (query: string) => ({ ... })); }); + afterEach(() => { + disposeColorModeStore?.(); + disposeColorModeStore = null; + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
test-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (3)
test-vr/tests/www/dark-mode.spec-vr.tsxwww/src/views/IndexView/index.tsxwww/test/routes/index.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- test-vr/tests/www/dark-mode.spec-vr.tsx
🧰 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/views/IndexView/index.tsxwww/test/routes/index.spec.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
www/src/views/IndexView/index.tsxwww/test/routes/index.spec.tsx
**/*.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/routes/index.spec.tsx
🧠 Learnings (15)
📚 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/routes/index.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/routes/index.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/routes/index.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/routes/index.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/routes/index.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/routes/index.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/routes/index.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/routes/index.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/routes/index.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 : Update Storybook stories when APIs have been changed to ensure they work as expected
Applied to files:
www/test/routes/index.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} : Use `userEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })` or the `userEventSetup` helper function from `test/helper/userEventSetup.ts` when creating userEvent instances
Applied to files:
www/test/routes/index.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/routes/index.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} : Use the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
Applied to files:
www/test/routes/index.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/routes/index.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/routes/index.spec.tsx
🧬 Code graph analysis (1)
www/test/routes/index.spec.tsx (4)
www/src/components/color-mode/defineColorModeStore.ts (1)
defineColorModeStore(82-119)www/src/views/IndexView/index.tsx (1)
render(13-110)www/src/components/color-mode/ColorModeProvider.tsx (1)
ColorModeProvider(7-15)www/src/routes/index.tsx (1)
routes(6-125)
⏰ 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
🔇 Additional comments (4)
www/src/views/IndexView/index.tsx (2)
33-33: Good addition of test hook for VR test stability.Adding
data-testidenables the visual regression tests to mask this iframe, addressing the brittleness concern mentioned in PR comments about the GitHub star counter.
68-68: CSS variable usage aligns with dark mode theming.Replacing hardcoded
'black'withvar(--color-text-1)properly integrates this element with the new color mode system.www/test/routes/index.spec.tsx (2)
86-94:matchMediastub is appropriate for testing color mode detection.The stub provides the necessary interface for the color mode store to function in tests. The
matches: falsevalue simulates a system preference for light mode.Consider adding
vi.unstubAllGlobals()in anafterEachto ensure clean state between test files, though this may already be handled by your vitest config.
20-25: Test helpers consistently updated with new pattern.The destructuring pattern with
{ screen, dispose }and explicitdispose()calls is consistently applied across all test helpers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test-vr/tests/www/dark-mode.spec-vr.tsx (1)
1-1: Unused React import.The explicit
Reactimport is not used in this file. Modern JSX transforms (React 17+) don't require it for JSX syntax.🔧 Suggested fix
-import * as React from 'react'; import { expect, test } from '@playwright/experimental-ct-react';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
test-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/dark-mode.spec-vr.tsx-snapshots/dark-mode-snapshot-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (2)
test-vr/tests/www/dark-mode.spec-vr.tsxwww/src/views/IndexView/index.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/views/IndexView/index.tsxtest-vr/tests/www/dark-mode.spec-vr.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
www/src/views/IndexView/index.tsxtest-vr/tests/www/dark-mode.spec-vr.tsx
🧠 Learnings (8)
📚 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:
test-vr/tests/www/dark-mode.spec-vr.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:
test-vr/tests/www/dark-mode.spec-vr.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:
test-vr/tests/www/dark-mode.spec-vr.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:
test-vr/tests/www/dark-mode.spec-vr.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:
test-vr/tests/www/dark-mode.spec-vr.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:
test-vr/tests/www/dark-mode.spec-vr.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:
test-vr/tests/www/dark-mode.spec-vr.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:
test-vr/tests/www/dark-mode.spec-vr.tsx
🧬 Code graph analysis (1)
test-vr/tests/www/dark-mode.spec-vr.tsx (1)
www/src/containers/Root.tsx (1)
Root(8-28)
⏰ 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
🔇 Additional comments (4)
www/src/views/IndexView/index.tsx (3)
28-34: LGTM!The
data-testidattribute is correctly added for the visual regression test masking. This allows the flaky GitHub star counter to be excluded from snapshot comparisons.
68-68: LGTM!Good migration to the new CSS variable
var(--color-text-1)for theme-aware text color. This aligns with the color-mode system being introduced in this PR.
91-91: LGTM!The
data-testid="user-item"attribute correctly enables masking of sponsor logos in visual regression tests, reducing flakiness from external image changes.test-vr/tests/www/dark-mode.spec-vr.tsx (1)
10-17: [Your rewritten review comment text here]
[Exactly ONE classification tag]
Description
I think I landed a good spot for the color naming / architecture
Still lot of details work to do, I will continue with new PRs
Related Issue
#6825
Motivation and Context
To make space for the dark mode
How Has This Been Tested?
Manually with my "designer" eyes 😄
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Styling
Tests
✏️ Tip: You can customize this high-level summary in your review settings.