Add polished theme and motion examples - #79
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 7ceac8c
☁️ Nx Cloud last updated this comment at |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds five themed interactive chart conformance cases, CSS-variable tooltip theming, theme and motion documentation, catalog validation, Chromium preview retries, and refreshed benchmark metadata. ChangesThemed chart and motion coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Catalog
participant CaseMount
participant ChartDefinition
participant Renderer
participant ConformanceTests
Catalog->>CaseMount: mount a conformance case
CaseMount->>ChartDefinition: create chart scene and application state
CaseMount->>Renderer: render and animate chart marks
Renderer-->>CaseMount: report settled state
ConformanceTests->>CaseMount: inspect targets, state, geometry, and cleanup
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
benchmarks/conformance/cases/120-themed-interactive-area/tanstack.ts (1)
216-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: inject the card stylesheet once per document.
Every mount appends an identical class-scoped
<style>block. The rules are not mount-scoped, so a page with several mounts holds several duplicate copies. Consider a document-level guard, for example adata-themed-area-stylesmarker on the first injected element, and skip injection when the marker exists. Keep the current behavior if per-mount teardown simplicity is the goal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/120-themed-interactive-area/tanstack.ts` around lines 216 - 311, Update the stylesheet injection around the `document.createElement('style')` call to add the themed-area CSS only once per document, using a document-level marker such as `data-themed-area-styles` on the injected style element and skipping creation when that marker already exists. Preserve the current stylesheet contents and mounting behavior.benchmarks/conformance/cases/121-active-bar-dashboard/recharts.ts (1)
47-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnmemoized
rowsfeeds hook dependency arrays in both implementations. Each component callsdashboardRows(input.revision)directly in the render body, so the returned array has a new identity on every render. Both components then list that array in a hook dependency array, which defeats the memoization.
benchmarks/conformance/cases/121-active-bar-dashboard/recharts.ts#L47-L59: wrapdashboardRows(input.revision)inuseMemokeyed oninput.revision, so the effect at lines 52-54 runs only when the metric or revision changes, and remove the duplicateonMetricChangecall inselectMetric.benchmarks/conformance/cases/121-active-bar-dashboard/view.tsx#L63-L67: wrapdashboardRows(input.revision)inuseMemokeyed oninput.revision, so theuseImperativeHandledependency list at line 125 stops changing on every render.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/121-active-bar-dashboard/recharts.ts` around lines 47 - 59, Memoize the dashboardRows result by input.revision in benchmarks/conformance/cases/121-active-bar-dashboard/recharts.ts:47-59 and benchmarks/conformance/cases/121-active-bar-dashboard/view.tsx:63-67, so the dependent hooks receive a stable rows reference. In recharts.ts, remove the duplicate onMetricChange call from selectMetric while preserving the effect-driven update.benchmarks/conformance/cases/121-active-bar-dashboard/tanstack.test.ts (1)
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the motion state is applied to the element
settleChartMotionreads.
svg?.setAttribute(...)is a no-op if the query returnsnull, and the test still passes.settle()passeschartHost.current(theonRendercontainer) tosettleChartMotion, which resolves when the motion state isfinishedornull. Both the "attribute applied" path and the "element missing" path therefore satisfyawait settled, so the test does not prove that the settle path observes the finished state.Assert that
svgexists before setting the attribute.💚 Proposed test fix
const hosts = container.querySelectorAll('.ts-chart-host') const svg = container.querySelector('svg.ts-chart') expect(hosts).toHaveLength(1) - svg?.setAttribute('data-ts-motion-state', 'finished') + expect(svg).not.toBeNull() + svg!.setAttribute('data-ts-motion-state', 'finished') const settled = handle.driver?.settle?.()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/121-active-bar-dashboard/tanstack.test.ts` around lines 66 - 71, Ensure the test asserts that the queried `svg` element exists before setting its motion state attribute. Update the test around `settleChartMotion` and `settle()` so a missing `svg` fails the test rather than allowing optional chaining to mask the issue, while preserving the existing settled Promise assertions.benchmarks/conformance/cases/123-active-donut-metric/chart.ts (2)
172-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompute the total once.
browserTotal(rows)runs twice on lines 177 and 178.♻️ Proposed change
const selected = rows.find((row) => row.id === activeId) ?? rows[0]! + const total = browserTotal(rows) return { selected, - total: browserTotal(rows), - share: selected.visitors / browserTotal(rows), + total, + share: selected.visitors / total, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/123-active-donut-metric/chart.ts` around lines 172 - 180, Update donutSummary to compute browserTotal(rows) once in a local total variable, then reuse that variable for both the returned total and share calculations.
41-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive the active arc from
selectedto keep the fallback consistent.Line 47 filters arcs by
activeId. Line 48 falls back torows[0]whenactiveIdmatches no row. If an unknownactiveIdreaches this function, the center metric shows the first row while no wedge or ring renders. The arc count then drops from 7 to 5 and breaks the geometry contract incase.json.Filter the arcs with
selected.idso both branches use the same row.♻️ Proposed change
const arcs = pie(rows, { value: 'visitors', gapAngle }) - const active = arcs.filter((row) => row.id === activeId) const selected = rows.find((row) => row.id === activeId) ?? rows[0]! + const active = arcs.filter((row) => row.id === selected.id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/123-active-donut-metric/chart.ts` around lines 41 - 48, Update activeDonutDefinition so the active arc filter uses selected.id instead of activeId, ensuring the fallback selected row also produces its corresponding arc and preserves the expected geometry.benchmarks/conformance/cases/123-active-donut-metric/recharts.ts (1)
496-531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffShare the anchor-resolution geometry helpers between both renderers. Both files define
svgPathPointandcenterwith copied logic, but the sampling grids differ: 32×32 inrecharts.tsand 10×10 inview.tsx. The two renderers can therefore resolve different points inside the same wedge, which reduces geometry parity for the sharedwedge:anchor. The 32×32 grid also runs about 961isPointInFillcalls per target resolution.
benchmarks/conformance/cases/123-active-donut-metric/recharts.ts#L496-L531: movesvgPathPoint,radialPathPoint, andcenterinto a shared module underbenchmarks/conformance/shared/, and import them here.benchmarks/conformance/cases/123-active-donut-metric/view.tsx#L258-L292: delete the localsvgPathPointandcentercopies and import the shared helpers so both renderers use one sampling resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/123-active-donut-metric/recharts.ts` around lines 496 - 531, Extract svgPathPoint, radialPathPoint, and center from benchmarks/conformance/cases/123-active-donut-metric/recharts.ts#L496-L531 into a shared module under benchmarks/conformance/shared/, then import the shared helpers in recharts.ts. In benchmarks/conformance/cases/123-active-donut-metric/view.tsx#L258-L292, remove the local svgPathPoint and center implementations and import the shared versions so both renderers use the same sampling resolution and geometry logic.benchmarks/conformance/cases/123-active-donut-metric/tanstack.test.ts (1)
84-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a driver assertion for legend selection.
The test verifies layout and DOM structure only. The
case.jsonscenario depends ondriver.readState()returningactiveId,focusedId, and thetooltipobject with the same keys in both implementations. Click a legend button and comparereadState()between both mounts. This catches contract drift betweenview.tsxandrecharts.tsbefore the browser conformance run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/123-active-donut-metric/tanstack.test.ts` around lines 84 - 138, Extend the active donut test around the existing legend button assertions to click a legend button and read the driver state from each mount. Compare the resulting readState() values, including activeId, focusedId, and tooltip with matching keys, so both TanStack and Recharts implementations satisfy the scenario contract before cleanup.benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx (2)
19-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
metricsso the per-carduseMemoworks.
premiumKpisForRevision(input.revision)returns new metric objects on every render. TheuseMemoinKpiCarddepends onmetric, so its dependency changes on every render andpremiumKpiDefinitionruns again.Chartthen receives a newdefinitionobject on every render, which can restart the spring transition.♻️ Proposed fix
- const metrics = premiumKpisForRevision(input.revision) + const metrics = useMemo( + () => premiumKpisForRevision(input.revision), + [input.revision], + )Also applies to: 109-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx` at line 19, Memoize the result of premiumKpisForRevision in the view component so metrics remains referentially stable when input.revision has not changed. Update the relevant useMemo dependency list around metrics and preserve the existing KpiCard metric dependency behavior, preventing unnecessary premiumKpiDefinition and Chart definition recalculation.
233-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared KPI styles and layout into one module. The TanStack view and the Plot reference each define an identical style constant and an identical
fullLayoutfunction. A change to one copy silently breaks visual parity with the other, and the color-scheme assertions inplot.test.tsandtanstack.test.tsthen diverge.
benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx#L233-L364: movepremiumKpiStylesinto a new shared module in this case directory, for examplestyles.ts, and import it.benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx#L212-L231: movefullLayoutand thefullGapconstant into the same shared module and import them.benchmarks/conformance/cases/122-premium-kpi-sparklines/plot.ts#L301-L432: deletepremiumKpiPlotStylesand import the shared constant.benchmarks/conformance/cases/122-premium-kpi-sparklines/plot.ts#L280-L299: delete the localfullLayoutandfullGapand import the shared versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx` around lines 233 - 364, Extract the duplicated KPI layout and styles into a shared module: in benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx:212-231 and :233-364, move fullLayout, fullGap, and premiumKpiStyles into the new module and import them; in plot.ts:280-299 and :301-432, remove the local fullLayout, fullGap, and premiumKpiPlotStyles definitions and import the shared versions, preserving identical behavior and styling.packages/charts-core/src/runtime.test.ts (1)
365-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one assertion for an inherited override.
These assertions verify the inline
var(...)declarations and fallback strings. They do not verify that a--ts-chart-tooltip-*value set on the chart container reaches the tooltip. Add a browser-level check for at least one token, or confirm that the existing browser conformance suite already covers this behavior.Based on the PR objective, inherited tooltip variables are part of the public behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/charts-core/src/runtime.test.ts` around lines 365 - 389, Add a browser-level assertion in the tooltip runtime test that sets at least one --ts-chart-tooltip-* custom property on the chart container and verifies the tooltip inherits and uses that value. Reuse the existing tooltip setup and preserve the current inline var(...) fallback assertions.benchmarks/conformance/cases/124-theme-palette-matrix/plot.ts (1)
119-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid assigning
undefinedto style properties.
Object.assigncopiesundefinedintopanel.style, which the CSSOM stringifies to"undefined"and then rejects as an invalid value. The panel element is recreated on every render, so no stale value survives, but the intent is clearer if the preview-only properties are set conditionally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/124-theme-palette-matrix/plot.ts` around lines 119 - 134, Update the Object.assign call configuring panel.style to omit preview-only properties when preview is true instead of assigning undefined. Keep the existing non-preview gridTemplateColumns, border, and borderRadius values, and apply those properties conditionally while preserving all other style assignments.benchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.ts (1)
69-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
resolveTarget.The driver in
view.tsx(Lines 63-71) parses apalette:<id>anchor and returns the panel center. No test exercises this path. A wrong prefix or a wrongdata-palette-treatmentselector would returnnulland browser behavior scenarios would fail without a unit-level signal. Add one assertion for a valid anchor and one for an unknown anchor.💚 Suggested addition
expect(handle.driver?.readState()).toMatchObject({ rowCount: 8, paletteCount: 3, svgCount: 3, palettes: ['neutral', 'vibrant', 'monochrome'], }) + expect( + handle.driver?.resolveTarget({ anchor: 'palette:vibrant' }), + ).not.toBeNull() + expect( + handle.driver?.resolveTarget({ anchor: 'palette:unknown' }), + ).toBeNull()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.ts` around lines 69 - 105, Add coverage for the driver's resolveTarget behavior in the existing test: assert that a valid palette:<id> anchor resolves to the corresponding panel center, and that an unknown anchor returns null. Use the mounted handle/driver and existing palette fixture symbols, without changing the current rendering assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/conformance/cases/120-themed-interactive-area/recharts.ts`:
- Around line 96-101: Update the cleanup effect around animationTracker to
dispose the current tracker and then reset animationTracker.current to null,
allowing the lazy initializer to create a usable tracker on remount while
preserving the existing cleanup behavior.
In `@benchmarks/conformance/cases/120-themed-interactive-area/tanstack.test.ts`:
- Around line 219-256: Update the reduced-motion test around mount and cleanup
so handle.destroy() runs in the finally block, including when assertions or
setup fail. Make the mocked matchMedia function return matches: true only when
the query is '(prefers-reduced-motion: reduce)' and false for other preference
queries, while preserving the existing restoration and container removal.
- Around line 48-52: Update the second assertion in the axis expectations to
guard the optional `axis`, `ticks`, and `values` fields using a nested matcher,
while preserving the `toHaveLength(3)` assertion for `values`. Keep the existing
`definition.x` access and axis shape checks unchanged.
In `@benchmarks/conformance/cases/123-active-donut-metric/recharts.ts`:
- Around line 454-464: Update sectorAngles to calculate the drawable sweep as
360 minus the non-zero item count multiplied by gapAngle, and base sector spans
on that sweep rather than the full 360 degrees. Keep the first sector anchored
at 90 degrees, apply one gapAngle offset before each later non-zero sector, and
preserve correct handling of zero-valued rows.
In `@benchmarks/conformance/cases/124-theme-palette-matrix/model.ts`:
- Around line 97-104: Keep paletteValue in model.ts unchanged and document that
consumers must opt into both color schemes. In view.tsx lines 124-141, add
colorScheme: 'light dark' to the section style object; in plot.ts lines 119-134,
add the same declaration to the Object.assign(panel.style, …) call.
In `@docs/reference/focus-and-interaction.md`:
- Around line 777-792: Clarify the inherited tooltip-token documentation around
the variable table to note that tokens defined only on the chart container are
unavailable when the portal fallback mounts the tooltip under
ownerDocument.body. Link to or briefly reference the existing portal guidance
and state that body-level tokens are required to preserve themed styling when
Popover is unavailable.
In `@scripts/catalog-definition-shapes.test.mjs`:
- Around line 60-62: Update classifyDefinitions to recognize defineChart calls
with the supported two-argument (definition, options) form, so the five new
cases include all four such calls while preserving existing one-argument
classification and counts.
---
Nitpick comments:
In `@benchmarks/conformance/cases/120-themed-interactive-area/tanstack.ts`:
- Around line 216-311: Update the stylesheet injection around the
`document.createElement('style')` call to add the themed-area CSS only once per
document, using a document-level marker such as `data-themed-area-styles` on the
injected style element and skipping creation when that marker already exists.
Preserve the current stylesheet contents and mounting behavior.
In `@benchmarks/conformance/cases/121-active-bar-dashboard/recharts.ts`:
- Around line 47-59: Memoize the dashboardRows result by input.revision in
benchmarks/conformance/cases/121-active-bar-dashboard/recharts.ts:47-59 and
benchmarks/conformance/cases/121-active-bar-dashboard/view.tsx:63-67, so the
dependent hooks receive a stable rows reference. In recharts.ts, remove the
duplicate onMetricChange call from selectMetric while preserving the
effect-driven update.
In `@benchmarks/conformance/cases/121-active-bar-dashboard/tanstack.test.ts`:
- Around line 66-71: Ensure the test asserts that the queried `svg` element
exists before setting its motion state attribute. Update the test around
`settleChartMotion` and `settle()` so a missing `svg` fails the test rather than
allowing optional chaining to mask the issue, while preserving the existing
settled Promise assertions.
In `@benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx`:
- Line 19: Memoize the result of premiumKpisForRevision in the view component so
metrics remains referentially stable when input.revision has not changed. Update
the relevant useMemo dependency list around metrics and preserve the existing
KpiCard metric dependency behavior, preventing unnecessary premiumKpiDefinition
and Chart definition recalculation.
- Around line 233-364: Extract the duplicated KPI layout and styles into a
shared module: in
benchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsx:212-231 and
:233-364, move fullLayout, fullGap, and premiumKpiStyles into the new module and
import them; in plot.ts:280-299 and :301-432, remove the local fullLayout,
fullGap, and premiumKpiPlotStyles definitions and import the shared versions,
preserving identical behavior and styling.
In `@benchmarks/conformance/cases/123-active-donut-metric/chart.ts`:
- Around line 172-180: Update donutSummary to compute browserTotal(rows) once in
a local total variable, then reuse that variable for both the returned total and
share calculations.
- Around line 41-48: Update activeDonutDefinition so the active arc filter uses
selected.id instead of activeId, ensuring the fallback selected row also
produces its corresponding arc and preserves the expected geometry.
In `@benchmarks/conformance/cases/123-active-donut-metric/recharts.ts`:
- Around line 496-531: Extract svgPathPoint, radialPathPoint, and center from
benchmarks/conformance/cases/123-active-donut-metric/recharts.ts#L496-L531 into
a shared module under benchmarks/conformance/shared/, then import the shared
helpers in recharts.ts. In
benchmarks/conformance/cases/123-active-donut-metric/view.tsx#L258-L292, remove
the local svgPathPoint and center implementations and import the shared versions
so both renderers use the same sampling resolution and geometry logic.
In `@benchmarks/conformance/cases/123-active-donut-metric/tanstack.test.ts`:
- Around line 84-138: Extend the active donut test around the existing legend
button assertions to click a legend button and read the driver state from each
mount. Compare the resulting readState() values, including activeId, focusedId,
and tooltip with matching keys, so both TanStack and Recharts implementations
satisfy the scenario contract before cleanup.
In `@benchmarks/conformance/cases/124-theme-palette-matrix/plot.ts`:
- Around line 119-134: Update the Object.assign call configuring panel.style to
omit preview-only properties when preview is true instead of assigning
undefined. Keep the existing non-preview gridTemplateColumns, border, and
borderRadius values, and apply those properties conditionally while preserving
all other style assignments.
In `@benchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.ts`:
- Around line 69-105: Add coverage for the driver's resolveTarget behavior in
the existing test: assert that a valid palette:<id> anchor resolves to the
corresponding panel center, and that an unknown anchor returns null. Use the
mounted handle/driver and existing palette fixture symbols, without changing the
current rendering assertions.
In `@packages/charts-core/src/runtime.test.ts`:
- Around line 365-389: Add a browser-level assertion in the tooltip runtime test
that sets at least one --ts-chart-tooltip-* custom property on the chart
container and verifies the tooltip inherits and uses that value. Reuse the
existing tooltip setup and preserve the current inline var(...) fallback
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae25c56d-f701-4daf-97fe-97357e5139e3
⛔ Files ignored due to path filters (5)
benchmarks/conformance/previews/120-themed-interactive-area.svgis excluded by!**/*.svgbenchmarks/conformance/previews/121-active-bar-dashboard.svgis excluded by!**/*.svgbenchmarks/conformance/previews/122-premium-kpi-sparklines.svgis excluded by!**/*.svgbenchmarks/conformance/previews/123-active-donut-metric.svgis excluded by!**/*.svgbenchmarks/conformance/previews/124-theme-palette-matrix.svgis excluded by!**/*.svg
📒 Files selected for processing (72)
.changeset/polished-tooltips-theme.mdAPI-FRICTION.mdbenchmarks/comparison/bundle-baseline.jsonbenchmarks/conformance/DEFINITION-COVERAGE-AUDIT.mdbenchmarks/conformance/DEFINITION-COVERAGE-OVERVIEW.mdbenchmarks/conformance/DEFINITION-COVERAGE-PLAN.mdbenchmarks/conformance/cases/120-themed-interactive-area/animation.test.tsbenchmarks/conformance/cases/120-themed-interactive-area/animation.tsbenchmarks/conformance/cases/120-themed-interactive-area/case.jsonbenchmarks/conformance/cases/120-themed-interactive-area/chart.tsbenchmarks/conformance/cases/120-themed-interactive-area/model.tsbenchmarks/conformance/cases/120-themed-interactive-area/recharts.tsbenchmarks/conformance/cases/120-themed-interactive-area/tanstack.test.tsbenchmarks/conformance/cases/120-themed-interactive-area/tanstack.tsbenchmarks/conformance/cases/121-active-bar-dashboard/case.jsonbenchmarks/conformance/cases/121-active-bar-dashboard/chart.tsbenchmarks/conformance/cases/121-active-bar-dashboard/model.tsbenchmarks/conformance/cases/121-active-bar-dashboard/recharts.tsbenchmarks/conformance/cases/121-active-bar-dashboard/tanstack.test.tsbenchmarks/conformance/cases/121-active-bar-dashboard/tanstack.tsbenchmarks/conformance/cases/121-active-bar-dashboard/view.tsxbenchmarks/conformance/cases/122-premium-kpi-sparklines/case.jsonbenchmarks/conformance/cases/122-premium-kpi-sparklines/chart.tsbenchmarks/conformance/cases/122-premium-kpi-sparklines/model.test.tsbenchmarks/conformance/cases/122-premium-kpi-sparklines/model.tsbenchmarks/conformance/cases/122-premium-kpi-sparklines/plot.test.tsbenchmarks/conformance/cases/122-premium-kpi-sparklines/plot.tsbenchmarks/conformance/cases/122-premium-kpi-sparklines/tanstack.test.tsbenchmarks/conformance/cases/122-premium-kpi-sparklines/tanstack.tsbenchmarks/conformance/cases/122-premium-kpi-sparklines/view.tsxbenchmarks/conformance/cases/123-active-donut-metric/case.jsonbenchmarks/conformance/cases/123-active-donut-metric/chart.tsbenchmarks/conformance/cases/123-active-donut-metric/layout.tsbenchmarks/conformance/cases/123-active-donut-metric/model.tsbenchmarks/conformance/cases/123-active-donut-metric/recharts.tsbenchmarks/conformance/cases/123-active-donut-metric/tanstack.test.tsbenchmarks/conformance/cases/123-active-donut-metric/tanstack.tsbenchmarks/conformance/cases/123-active-donut-metric/view.tsxbenchmarks/conformance/cases/124-theme-palette-matrix/case.jsonbenchmarks/conformance/cases/124-theme-palette-matrix/chart.tsbenchmarks/conformance/cases/124-theme-palette-matrix/model.tsbenchmarks/conformance/cases/124-theme-palette-matrix/plot.test.tsbenchmarks/conformance/cases/124-theme-palette-matrix/plot.tsbenchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.tsbenchmarks/conformance/cases/124-theme-palette-matrix/tanstack.tsbenchmarks/conformance/cases/124-theme-palette-matrix/view.tsxbenchmarks/conformance/catalog-index.jsonbenchmarks/conformance/definition-coverage-roadmap.jsonbenchmarks/conformance/definition-coverage-roadmap.test.tsbenchmarks/conformance/previews/manifest.jsondocs/comparison.mddocs/config.jsondocs/examples/index.mddocs/examples/themes-and-motion.mddocs/guides/dynamic-data-and-animation.mddocs/guides/themes-and-styling.mddocs/reference/focus-and-interaction.mdllms.txtpackages/charts-core/docs/comparison.mdpackages/charts-core/docs/config.jsonpackages/charts-core/docs/examples/index.mdpackages/charts-core/docs/examples/themes-and-motion.mdpackages/charts-core/docs/guides/dynamic-data-and-animation.mdpackages/charts-core/docs/guides/themes-and-styling.mdpackages/charts-core/docs/reference/focus-and-interaction.mdpackages/charts-core/llms.txtpackages/charts-core/src/runtime.test.tspackages/charts-core/src/tooltip.tsscripts/catalog-definition-shapes.test.mjsscripts/catalog-preview.mjsscripts/catalog-preview.test.mjsscripts/measure-bundles.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/conformance/cases/123-active-donut-metric/tanstack.test.ts`:
- Around line 84-101: Extend the test around sectorAngles to also inspect the
TanStack active overlay definition or mounted overlay using the same rows, and
assert its start and end angles against the native sectorAngles reference. Keep
the existing Recharts angle assertions, but ensure the test fails when TanStack
overlay angles regress.
In `@benchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.ts`:
- Around line 113-134: Wrap the mount and panel assertions in a try/finally
within the test callback, and move handle.destroy() plus container.remove() into
the finally block. Ensure cleanup runs even when mounting or any expectation
fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5c74e37-5722-434d-87a6-836dcd53d094
📒 Files selected for processing (15)
API-FRICTION.mdbenchmarks/conformance/cases/120-themed-interactive-area/recharts.tsbenchmarks/conformance/cases/120-themed-interactive-area/tanstack.test.tsbenchmarks/conformance/cases/123-active-donut-metric/recharts.tsbenchmarks/conformance/cases/123-active-donut-metric/tanstack.test.tsbenchmarks/conformance/cases/124-theme-palette-matrix/model.tsbenchmarks/conformance/cases/124-theme-palette-matrix/plot.tsbenchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.tsbenchmarks/conformance/cases/124-theme-palette-matrix/view.tsxbenchmarks/conformance/previews/manifest.jsondocs/reference/focus-and-interaction.mdpackages/charts-core/docs/reference/focus-and-interaction.mdscripts/catalog-definition-shapes.test.mjsscripts/catalog-preview.mjsscripts/catalog-preview.test.mjs
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/charts-core/docs/reference/focus-and-interaction.md
- docs/reference/focus-and-interaction.md
- benchmarks/conformance/previews/manifest.json
- benchmarks/conformance/cases/124-theme-palette-matrix/view.tsx
- benchmarks/conformance/cases/124-theme-palette-matrix/model.ts
- benchmarks/conformance/cases/120-themed-interactive-area/tanstack.test.ts
- benchmarks/conformance/cases/123-active-donut-metric/recharts.ts
- benchmarks/conformance/cases/124-theme-palette-matrix/plot.ts
- benchmarks/conformance/cases/120-themed-interactive-area/recharts.ts
- API-FRICTION.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/conformance/cases/123-active-donut-metric/chart.ts`:
- Around line 46-47: Compute the fallback-aware selected row before calling
activeDonutArcs, then pass selected.id instead of the original activeId so the
overlay matches the center text when the ID is unknown. Add a regression test
covering an unknown active ID and verifying the fallback row’s active wedge or
ring is rendered.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90b0934a-696e-4d51-9903-7abea16e93f4
📒 Files selected for processing (7)
API-FRICTION.mdbenchmarks/conformance/cases/123-active-donut-metric/chart.tsbenchmarks/conformance/cases/123-active-donut-metric/tanstack.test.tsbenchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.tsbenchmarks/conformance/previews/manifest.jsonscripts/catalog-preview.mjsscripts/catalog-preview.test.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- benchmarks/conformance/cases/124-theme-palette-matrix/tanstack.test.ts
- benchmarks/conformance/previews/manifest.json
- benchmarks/conformance/cases/123-active-donut-metric/tanstack.test.ts
- API-FRICTION.md
- scripts/catalog-preview.test.mjs
- scripts/catalog-preview.mjs
Merged
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
1 participant
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.
Summary
Release
Validation
Summary by CodeRabbit