Merged
Conversation
Contributor
WalkthroughAdds a treeshaking test to CI and project config, introduces a new BoxPlotExample component and its VR test, refactors ErrorBar tests to a new render-test pattern using createSelectorTestCase, and updates AxisTicks documentation about niceTicks availability. Changes
Sequence Diagram(s)(omitted — changes do not introduce a multi-component sequential control flow requiring visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
PavelVanecek
commented
Mar 12, 2026
Collaborator
Author
There was a problem hiding this comment.
The "max" value here is 49 but the domain only goes up to 40, this is the bug.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
www/src/docs/exampleComponents/BarChart/BoxPlotExample.tsx (1)
55-57: Avoid teaching@ts-expect-errorin the public snippet.This raw example currently depends on Recharts spreading datum fields directly onto the shape props. If
payloadis available onBarShapePropshere, prefer that over suppressing the type error so the snippet stays within the documented custom-shape contract.♻️ Possible cleanup
const BoxShape = (props: BarShapeProps) => { - // `@ts-expect-error` Recharts does spread datum on the props but the types don't reflect that - const entry: BoxPlotDatum = props; + const entry = props.payload as BoxPlotDatum;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/src/docs/exampleComponents/BarChart/BoxPlotExample.tsx` around lines 55 - 57, The snippet currently suppresses a type error in BoxShape by casting props to BoxPlotDatum; instead, use the payload supplied on the Recharts shape props (i.e., read const entry = props.payload as BoxPlotDatum) and remove the // `@ts-expect-error` line so the example follows the documented custom-shape contract; also add a narrow runtime guard (check props.payload exists) or a sensible fallback to keep types safe and avoid teaching reliance on ts-expect-error while still supporting the BarShapeProps → BoxPlotDatum mapping.test/cartesian/ErrorBar.spec.tsx (2)
1932-1941: Keep this selector case aligned with the other three.This branch is now the odd one out: it still uses
toHaveBeenLastCalledWith(...), and it also dropped the final post-rerender spy-count assertion that the parallel domain-extension tests keep. Restoring both keeps the selector coverage consistent.🧪 Suggested assertion update
rerender(secondRender); - expect(spy).toHaveBeenLastCalledWith([0, 3600]); + expectLastCalledWith(spy, [0, 3600]); + expect(spy).toHaveBeenCalledTimes(7);As per coding guidelines, "Use the
expectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion" and "Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/ErrorBar.spec.tsx` around lines 1932 - 1941, The test case selectAxisDomainIncludingNiceTicks uses expect(spy).toHaveBeenLastCalledWith(...) and is missing the final spy call count check; replace the toHaveBeenLastCalledWith assertion with the project helper expectLastCalledWith(spy, [0, 3600]) and add the final assertion verifying the spy call count (e.g., expect(spy).toHaveBeenCalledTimes(4)) so this selector test matches the pattern used by the other three tests.
1324-1333: UseexpectLastCalledWithfor these selector assertions.These new cases switched back to
toHaveBeenLastCalledWith(...)even though the file already imports the typed helper.🧰 Minimal change
- expect(spy).toHaveBeenLastCalledWith([0, 3600]); + expectLastCalledWith(spy, [0, 3600]);As per coding guidelines, "Use the
expectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion."Also applies to: 1498-1508, 1678-1688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/ErrorBar.spec.tsx` around lines 1324 - 1333, In the "selectAxisDomainIncludingNiceTicks" test replace the assertions that call expect(spy).toHaveBeenLastCalledWith(...) with the typed helper expectLastCalledWith(spy, ...); specifically update the two occurrences in the test that assert the spy's last call (after rerender and initial checks) to use expectLastCalledWith(spy, [0, 3600]) and likewise for the initial last-call assertion, and apply the same replacement pattern for the other tests mentioned (the ones around the same file that reference spy and toHaveBeenLastCalledWith) so they use expectLastCalledWith(spy, ...); locate these by the test name selectAxisDomainIncludingNiceTicks, the spy variable, rerender/secondRender usage and replace the matcher only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@www/src/docs/exampleComponents/BarChart/BoxPlotExample.tsx`:
- Around line 103-107: The YAxis auto-scaling is clipping the top whisker
because ErrorBar's ranged values (whiskerDataKey) don't expand the axis; update
the example to pin the YAxis domain to the static dataset bounds so whiskers are
visible. Concretely, modify the component that renders YAxis (symbol: YAxis) to
include an explicit domain covering the dataset (e.g., domain including 0 and
the known max 49) or compute min/max from the static data and pass domain={[min,
max]}; keep the Bar/ErrorBar usage (symbols: Bar, ErrorBar, whiskerDataKey,
boxDataKey, BoxShape, DefaultZIndexes) unchanged aside from the YAxis domain
change. If you prefer not to hardcode bounds, derive the bounds from the
example's data array and set YAxis domain accordingly before rendering.
---
Nitpick comments:
In `@test/cartesian/ErrorBar.spec.tsx`:
- Around line 1932-1941: The test case selectAxisDomainIncludingNiceTicks uses
expect(spy).toHaveBeenLastCalledWith(...) and is missing the final spy call
count check; replace the toHaveBeenLastCalledWith assertion with the project
helper expectLastCalledWith(spy, [0, 3600]) and add the final assertion
verifying the spy call count (e.g., expect(spy).toHaveBeenCalledTimes(4)) so
this selector test matches the pattern used by the other three tests.
- Around line 1324-1333: In the "selectAxisDomainIncludingNiceTicks" test
replace the assertions that call expect(spy).toHaveBeenLastCalledWith(...) with
the typed helper expectLastCalledWith(spy, ...); specifically update the two
occurrences in the test that assert the spy's last call (after rerender and
initial checks) to use expectLastCalledWith(spy, [0, 3600]) and likewise for the
initial last-call assertion, and apply the same replacement pattern for the
other tests mentioned (the ones around the same file that reference spy and
toHaveBeenLastCalledWith) so they use expectLastCalledWith(spy, ...); locate
these by the test name selectAxisDomainIncludingNiceTicks, the spy variable,
rerender/secondRender usage and replace the matcher only.
In `@www/src/docs/exampleComponents/BarChart/BoxPlotExample.tsx`:
- Around line 55-57: The snippet currently suppresses a type error in BoxShape
by casting props to BoxPlotDatum; instead, use the payload supplied on the
Recharts shape props (i.e., read const entry = props.payload as BoxPlotDatum)
and remove the // `@ts-expect-error` line so the example follows the documented
custom-shape contract; also add a narrow runtime guard (check props.payload
exists) or a sensible fallback to keep types safe and avoid teaching reliance on
ts-expect-error while still supporting the BarShapeProps → BoxPlotDatum mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b3967de-90cd-4873-99d9-4d7f0d8b10bb
⛔ Files ignored due to path filters (3)
test-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (8)
.github/workflows/ci.ymlpackage.jsontest-vr/tests/www/BarChartApiExamples.spec-vr.tsxtest/cartesian/ErrorBar.spec.tsxvitest.config.mtswww/src/components/GuideView/AxisTicks/index.tsxwww/src/docs/exampleComponents/BarChart/BoxPlotExample.tsxwww/src/docs/exampleComponents/BarChart/index.tsx
Comment on lines
+103
to
+107
| <YAxis width="auto" /> | ||
| <CartesianGrid vertical={false} /> | ||
| <Bar dataKey={boxDataKey} shape={BoxShape}> | ||
| <ErrorBar dataKey={whiskerDataKey} width={0} zIndex={DefaultZIndexes.bar - 1} /> | ||
| </Bar> |
Contributor
There was a problem hiding this comment.
This example still renders a clipped whisker.
Category C has max: 49, but the highest non-whisker value in the current dataset is only 40, so the auto YAxis can still stop short of the upper whisker while ranged-bar ErrorBars do not extend the domain. Please either pin the example domain to the known bounds of this static dataset or hold the example back until the domain fix lands.
📌 Temporary docs-side workaround
- <YAxis width="auto" />
+ <YAxis width="auto" domain={[0, 50]} />📝 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.
Suggested change
| <YAxis width="auto" /> | |
| <CartesianGrid vertical={false} /> | |
| <Bar dataKey={boxDataKey} shape={BoxShape}> | |
| <ErrorBar dataKey={whiskerDataKey} width={0} zIndex={DefaultZIndexes.bar - 1} /> | |
| </Bar> | |
| <YAxis width="auto" domain={[0, 50]} /> | |
| <CartesianGrid vertical={false} /> | |
| <Bar dataKey={boxDataKey} shape={BoxShape}> | |
| <ErrorBar dataKey={whiskerDataKey} width={0} zIndex={DefaultZIndexes.bar - 1} /> | |
| </Bar> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@www/src/docs/exampleComponents/BarChart/BoxPlotExample.tsx` around lines 103
- 107, The YAxis auto-scaling is clipping the top whisker because ErrorBar's
ranged values (whiskerDataKey) don't expand the axis; update the example to pin
the YAxis domain to the static dataset bounds so whiskers are visible.
Concretely, modify the component that renders YAxis (symbol: YAxis) to include
an explicit domain covering the dataset (e.g., domain including 0 and the known
max 49) or compute min/max from the static data and pass domain={[min, max]};
keep the Bar/ErrorBar usage (symbols: Bar, ErrorBar, whiskerDataKey, boxDataKey,
BoxShape, DefaultZIndexes) unchanged aside from the YAxis domain change. If you
prefer not to hardcode bounds, derive the bounds from the example's data array
and set YAxis domain accordingly before rendering.
Collaborator
Author
There was a problem hiding this comment.
I'll fix this in next pr
Contributor
There was a problem hiding this comment.
@PavelVanecek, understood! Would you like me to open a GitHub issue to track the YAxis domain clipping bug so it doesn't get lost before the follow-up PR lands?
🧠 Learnings used
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: Provide a simple, declarative, and composable way to build charts in Recharts
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6855
File: www/src/docs/api/AreaChartAPI.tsx:422-438
Timestamp: 2026-01-06T22:34:01.675Z
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.
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: Value consistency, usability, and performance in Recharts implementation
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: When making changes to Recharts code, prefer current best practices as described in CONTRIBUTING.md and try to improve code style where relevant to the current task
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.
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6896
File: src/chart/AreaChart.tsx:0-0
Timestamp: 2026-01-17T03:11:01.347Z
Learning: In recharts (TypeScript 5.7.2+), generic forwardRef chart components use explicit type annotations with ReactNode return type to avoid `as` assertions. Pattern: `export const Chart: <T>(props: Props<T> & { ref?: Ref<El> }) => ReactNode = forwardRef<El, Props<unknown>>((props, ref) => ...)`. This is acceptable because TS 5.1+ allows ReactNode as valid JSX return type.
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.
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6896
File: src/chart/AreaChart.tsx:0-0
Timestamp: 2026-01-17T03:23:05.586Z
Learning: In recharts, generic forwardRef chart components are an exception to the "no `as` assertions" guideline. Due to TypeScript 4.x compatibility requirements, the pattern `export const Chart = forwardRef<El, Props<unknown>>(...) as <T>(props: Props<T> & { ref?: Ref<El> }) => React.ReactElement` is acceptable until the minimum TypeScript version can be bumped to 5.1+ (which requires ReactNode return type support). This will be addressed in a future major version (tracked in issue `#6898`).
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2026-02-26T22:44:25.602Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : All imports from `recharts` must use the public API entry point. Do not import from internal paths like `recharts/types/*` or `recharts/src/*`
Learnt from: cloud-walker
Repo: recharts/recharts PR: 6882
File: www/src/docs/apiExamples/ReferenceDot/ReferenceDotExample.tsx:55-55
Timestamp: 2026-01-19T20:58:49.284Z
Learning: In recharts documentation examples under www/src/docs, allow hardcoded color literals (e.g., 'red', 'blue') alongside CSS variables. Do not flag these example files as theming inconsistencies; they are intentional for illustrating styling approaches in documentation and should not trigger theming checks.
Collaborator
Author
There was a problem hiding this comment.
Yes please create a new issue
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo recharts/recharts \
--title "BoxPlotExample: YAxis auto-scaling clips top whisker due to ErrorBar ranged values not extending domain" \
--body "## Summary
The \`BoxPlotExample\` added in `#7130` renders a clipped top whisker for category \`C\` (which has \`max: 49\`). When a ranged \`Bar\` uses \`ErrorBar\`, the axis domain is not extended to account for the whisker values, so the \`YAxis\` auto-scaling stops short and the upper whisker is clipped.
## Reproduction
See the \`BoxPlotExample\` in \`www/src/docs/exampleComponents/BarChart/BoxPlotExample.tsx\` (introduced in `#7130`). The \`whiskerDataKey\` computes \`[q3 - min, max - q3]\` error values, but these do not expand the \`YAxis\` domain beyond the ranged bar's upper bound (\`q3\`).
## Expected behavior
The \`YAxis\` domain should be extended to cover the full whisker range (i.e., up to \`max: 49\` for category C), so all whiskers are fully visible.
## Temporary workaround (docs side)
Pin the domain manually on \`YAxis\`:
\`\`\`tsx
<YAxis width="auto" domain={[0, 50]} />
\`\`\`
## Related
- PR: https://github.com/recharts/recharts/pull/7130
- Review comment: https://github.com/recharts/recharts/pull/7130#discussion_r2925187100
Requested by `@PavelVanecek`." \
--assignee PavelVanecek
Length of output: 48
@PavelVanecek, done! I've opened recharts/recharts#7134 to track the YAxis domain clipping bug. It includes a summary of the root cause, reproduction steps, the temporary domain workaround, and backlinks to this PR and review comment. It's been assigned to you.
🧠 Learnings used
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: Provide a simple, declarative, and composable way to build charts in Recharts
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6855
File: www/src/docs/api/AreaChartAPI.tsx:422-438
Timestamp: 2026-01-06T22:34:01.675Z
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.
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: Value consistency, usability, and performance in Recharts implementation
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: When making changes to Recharts code, prefer current best practices as described in CONTRIBUTING.md and try to improve code style where relevant to the current task
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: Do not attempt to fix too much style inconsistency at once in Recharts, or when it is not related to the current changes
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: Do not focus too much on fitting the existing Recharts coding style if it is not ideal; prefer current best practices
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-29T02:45:55.748Z
Learning: Ensure accessibility is important in Recharts components and implementation
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6896
File: src/chart/AreaChart.tsx:0-0
Timestamp: 2026-01-17T03:23:05.586Z
Learning: In recharts, generic forwardRef chart components are an exception to the "no `as` assertions" guideline. Due to TypeScript 4.x compatibility requirements, the pattern `export const Chart = forwardRef<El, Props<unknown>>(...) as <T>(props: Props<T> & { ref?: Ref<El> }) => React.ReactElement` is acceptable until the minimum TypeScript version can be bumped to 5.1+ (which requires ReactNode return type support). This will be addressed in a future major version (tracked in issue `#6898`).
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.
Learnt from: cloud-walker
Repo: recharts/recharts PR: 6882
File: www/src/docs/apiExamples/ReferenceDot/ReferenceDotExample.tsx:55-55
Timestamp: 2026-01-19T20:58:49.284Z
Learning: In recharts documentation examples under www/src/docs, allow hardcoded color literals (e.g., 'red', 'blue') alongside CSS variables. Do not flag these example files as theming inconsistencies; they are intentional for illustrating styling approaches in documentation and should not trigger theming checks.
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6750
File: src/state/selectors/axisSelectors.ts:593-602
Timestamp: 2025-12-08T08:23:26.261Z
Learning: In the recharts codebase, `DataKey<any>` is an intentional exception to the "no any" rule while proper typing is being developed. This should not be flagged in reviews.
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.
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6896
File: src/chart/AreaChart.tsx:0-0
Timestamp: 2026-01-17T03:11:01.347Z
Learning: In recharts (TypeScript 5.7.2+), generic forwardRef chart components use explicit type annotations with ReactNode return type to avoid `as` assertions. Pattern: `export const Chart: <T>(props: Props<T> & { ref?: Ref<El> }) => ReactNode = forwardRef<El, Props<unknown>>((props, ref) => ...)`. This is acceptable because TS 5.1+ allows ReactNode as valid JSX return type.
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2026-02-26T22:44:25.602Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : All imports from `recharts` must use the public API entry point. Do not import from internal paths like `recharts/types/*` or `recharts/src/*`
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7130 +/- ##
==========================================
- Coverage 89.61% 89.59% -0.03%
==========================================
Files 535 536 +1
Lines 40315 40401 +86
Branches 5492 5499 +7
==========================================
+ Hits 36130 36197 +67
- Misses 4177 4196 +19
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportBundle size has no change ✅ |
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Member
|
are our integ tests borked? |
ckifer
approved these changes
Mar 13, 2026
Collaborator
Author
|
Yeah something went wrong. I'm looking into it |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cartesian/ErrorBar.spec.tsx`:
- Line 1332: Replace the untyped last-call assertion
expect(spy).toHaveBeenLastCalledWith([0, 3600]) with the typed helper
expectLastCalledWith already imported in the file; update the call that
references spy to use expectLastCalledWith(spy, [0, 3600]) and make the same
replacement for the other occurrences that use toHaveBeenLastCalledWith with spy
(also noted elsewhere in the file).
- Around line 1932-1942: The test 'selectAxisDomainIncludingNiceTicks' is
missing an assertion that verifies the selector call count after rerender;
update the test to assert the spy's total call count after calling
rerender(secondRender) (e.g., expect(spy).toHaveBeenCalledTimes(4)) so it
matches the sibling selector-domain tests and uses the same helper matcher for
consistency—locate the assertions around selectAxisDomainIncludingNiceTicks,
spy, rerender and secondRender and add the call-count assertion immediately
after the rerender and before the expect(spy).toHaveBeenLastCalledWith(...)
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c10ebdda-9fc8-4992-abaf-133de05130ad
⛔ Files ignored due to path filters (3)
test-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (8)
.github/workflows/ci.ymlpackage.jsontest-vr/tests/www/BarChartApiExamples.spec-vr.tsxtest/cartesian/ErrorBar.spec.tsxvitest.config.mtswww/src/components/GuideView/AxisTicks/index.tsxwww/src/docs/exampleComponents/BarChart/BoxPlotExample.tsxwww/src/docs/exampleComponents/BarChart/index.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- test-vr/tests/www/BarChartApiExamples.spec-vr.tsx
- www/src/docs/exampleComponents/BarChart/BoxPlotExample.tsx
- .github/workflows/ci.yml
- package.json
| expect(spy).toHaveBeenCalledTimes(3); | ||
|
|
||
| rerender(secondRender); | ||
| expect(spy).toHaveBeenLastCalledWith([0, 3600]); |
Contributor
There was a problem hiding this comment.
Use expectLastCalledWith helper for last-call assertions.
These assertions should use the typed helper that is already imported in this file.
Suggested patch
- expect(spy).toHaveBeenLastCalledWith([0, 3600]);
+ expectLastCalledWith(spy, [0, 3600]);- expect(spy).toHaveBeenLastCalledWith([0, 3600]);
+ expectLastCalledWith(spy, [0, 3600]);- expect(spy).toHaveBeenLastCalledWith([0, 3600]);
+ expectLastCalledWith(spy, [0, 3600]);As per coding guidelines: Use the expectLastCalledWith helper function instead of expect(spy).toHaveBeenLastCalledWith(...) for better typing and autocompletion.
Also applies to: 1507-1507, 1687-1687
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cartesian/ErrorBar.spec.tsx` at line 1332, Replace the untyped last-call
assertion expect(spy).toHaveBeenLastCalledWith([0, 3600]) with the typed helper
expectLastCalledWith already imported in the file; update the call that
references spy to use expectLastCalledWith(spy, [0, 3600]) and make the same
replacement for the other occurrences that use toHaveBeenLastCalledWith with spy
(also noted elsewhere in the file).
Comment on lines
+1932
to
+1942
| test('selectAxisDomainIncludingNiceTicks', () => { | ||
| const { spy, rerender } = renderTestCase(state => | ||
| selectAxisDomainIncludingNiceTicks(state, 'xAxis', 0, useIsPanorama()), | ||
| ); | ||
| expect(spy).toHaveBeenCalledWith([0, 3400]); | ||
| expect(spy).toHaveBeenCalledTimes(3); | ||
|
|
||
| rerender(secondRender); | ||
|
|
||
| expect(spy).toHaveBeenLastCalledWith([0, 3600]); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add post-rerender selector call-count assertion in this block.
The sibling selector-domain tests assert rerender call count; this one should do the same, and also use the helper matcher for consistency.
Suggested patch
rerender(secondRender);
- expect(spy).toHaveBeenLastCalledWith([0, 3600]);
+ expectLastCalledWith(spy, [0, 3600]);
+ expect(spy).toHaveBeenCalledTimes(7);As per coding guidelines: Verify the number of selector calls using the spy object from createSelectorTestCase to spot unnecessary re-renders and improve performance.
📝 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.
Suggested change
| test('selectAxisDomainIncludingNiceTicks', () => { | |
| const { spy, rerender } = renderTestCase(state => | |
| selectAxisDomainIncludingNiceTicks(state, 'xAxis', 0, useIsPanorama()), | |
| ); | |
| expect(spy).toHaveBeenCalledWith([0, 3400]); | |
| expect(spy).toHaveBeenCalledTimes(3); | |
| rerender(secondRender); | |
| expect(spy).toHaveBeenLastCalledWith([0, 3600]); | |
| }); | |
| test('selectAxisDomainIncludingNiceTicks', () => { | |
| const { spy, rerender } = renderTestCase(state => | |
| selectAxisDomainIncludingNiceTicks(state, 'xAxis', 0, useIsPanorama()), | |
| ); | |
| expect(spy).toHaveBeenCalledWith([0, 3400]); | |
| expect(spy).toHaveBeenCalledTimes(3); | |
| rerender(secondRender); | |
| expectLastCalledWith(spy, [0, 3600]); | |
| expect(spy).toHaveBeenCalledTimes(7); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cartesian/ErrorBar.spec.tsx` around lines 1932 - 1942, The test
'selectAxisDomainIncludingNiceTicks' is missing an assertion that verifies the
selector call count after rerender; update the test to assert the spy's total
call count after calling rerender(secondRender) (e.g.,
expect(spy).toHaveBeenCalledTimes(4)) so it matches the sibling selector-domain
tests and uses the same helper matcher for consistency—locate the assertions
around selectAxisDomainIncludingNiceTicks, spy, rerender and secondRender and
add the call-count assertion immediately after the rerender and before the
expect(spy).toHaveBeenLastCalledWith(...) check.
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This suffers from a bug where errorbar whiskers don't extend the domain if used in a ranged bar chart. I will fix that in separate PR.
Also some minor docs and build improvements.
Summary by CodeRabbit
New Features
Documentation
Tests