fix: chart review HIGH + MEDIUM bugs (bar, line, table) - #870
Merged
Conversation
Fixes five high-severity findings from the post-merge chart code review: 1. Bar/line transforms: silent Number() coercion. Replaced `Number(r[k]) || 0` with a `toSeriesNumber` helper that returns null for null/undefined/non-numeric values and preserves numeric zero. ECharts renders nulls as gaps, restoring the missing-vs-zero distinction so bad data isn't silently masquerading as 0. 2. Bar/line transforms: `Object.keys(records[0])` dropped sparse series. Added `collectAllKeys` that unions keys across every row, and mirrored the fix in the bar/line chart components for direct callers. 3. `contrastTextColor` crashed / produced invisible text on non-hex colors. Extracted to a single shared util in chart-utils with a defensive parser that accepts `#rgb`, `#rrggbb`, `rgb()`, `rgba()` (including percentage components), and falls back to `#000000` for anything unparseable. Removed the duplicate copies in `table-renderer.tsx` and `single-value-chart.tsx`. 4. Table widget: pagination flash on first render. When pagination is enabled, gate the DataGrid render until the container ResizeObserver has measured a height. Previously the table briefly mounted with the default `pageSize=10`, then immediately re-rendered with the dynamic page size, visibly snapping the row count. 5. Table widget: dead `enableGlobalFilter` UI. Removed the "Global Search" toggle from the table chart-options schema; the toggle wired `enableGlobalFilter` into TanStack Table's filter row model but no search input was ever rendered, so toggling it had no observable effect. Per-column filters cover the filtering need. Tests: 9 new contrast-text-color cases, 8 new shared-utils helper cases (collectAllKeys, toSeriesNumber), updated bar/line transform tests (now assert null for non-numeric input, added zero-preservation and sparse-series regression cases), updated chart-options-schema tests to assert the dead toggle is gone. All 1628 component tests + 2756 app tests pass; type checks clean; focused E2E (charts, styling-rules, widget-states) green.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR refactors chart data handling to support sparse rows, adds accessibility labels to bar and line charts, consolidates the color-contrast utility across components, and improves table rendering behavior. Key changes include shared transform utilities ( ChangesData transforms, chart accessibility, and table rendering refinements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Two MEDIUM findings from the chart code review (the third — line tooltip
HTML escaping — was a false positive; chart-utils.ts:191 already calls
escapeHtml on series names, and the bar empty-data placeholder also
already exists via buildEmptyDataOption()).
- BarChart / LineChart: when no explicit ariaDescription is provided,
derive one from the data shape ("Bar chart with N categories and M
series: ..."). The previous default of "Chart visualization" was
uninformative for screen-reader users.
- TableRenderer: add role="region" + aria-label to the scroll wrapper
("Table with N rows and M columns") — the underlying <table> had no
top-level label, so AT users hit the widget with no context. Callers
can override via settings.ariaLabel.
Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
alfredo1996
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@app/src/components/table-renderer.tsx`:
- Around line 295-296: The gate that prevents early DataGrid mounting should
treat non-positive heights as "not ready": update the awaitingHeight check (the
const awaitingHeight that currently tests enablePagination && containerHeight
=== undefined) to consider null/undefined and <= 0 (e.g., enablePagination &&
(containerHeight == null || containerHeight <= 0)); apply the same change to the
other similar check around the DataGrid mount (the second occurrence mentioned
at the other check) so DataGrid won't mount when the initial measured height is
0 or negative.
In `@app/src/plugins/transforms/shared-utils.ts`:
- Around line 76-79: toSeriesNumber currently treats whitespace-only strings as
0 because Number(" ") === 0; update the toSeriesNumber function to treat
blank-ish strings as missing by checking for string inputs and returning null
when raw is a string and raw.trim() === "". Keep existing checks
(null/undefined/""), then coerce non-blank strings/numbers as before (use
Number(raw) and Number.isFinite(n)) so behavior for valid numbers is unchanged.
In `@component/src/charts/chart-utils.ts`:
- Around line 120-136: The rgb/rgba parser currently only validates the first
three components (variables: rgb regex match, parts array) and thus accepts
malformed inputs; update the logic in chart-utils.ts to enforce exact arity and
valid alpha: when the input is an rgb(...) string require parts.length === 3,
and when it's rgba(...) require parts.length === 4, parse and clamp the first
three channels as before, then parse the fourth component as alpha (numeric,
allow percentage or 0–1 decimal) and reject (return null) if alpha is not a
finite number or out of range; ensure any extra channels cause rejection so only
properly formed rgb() or rgba() values are accepted.
In `@component/src/charts/line-chart.tsx`:
- Around line 98-109: The union-of-series-key logic (iterating data, skipping
"x", building seenKeys/seriesKeys) is duplicated in the options useMemo and the
autoAria useMemo; extract it into a single shared helper or a useMemo (e.g.,
create getSeriesKeys(data) or const seriesKeysMemo = useMemo(() => { ... },
[data])), return the array of series keys and reuse that symbol in both the
options and autoAria useMemos (replace the local seenKeys/seriesKeys blocks with
a reference to the shared seriesKeys), ensuring the dependency arrays reference
the shared memo if you use useMemo so behavior remains stable.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8674c617-0d37-4a03-b6fe-fcb220a3baa0
📒 Files selected for processing (17)
app/src/components/table-renderer.tsxapp/src/plugins/bar/transform.tsapp/src/plugins/line/transform.tsapp/src/plugins/transforms/__tests__/bar.test.tsapp/src/plugins/transforms/__tests__/line.test.tsapp/src/plugins/transforms/__tests__/shared.test.tsapp/src/plugins/transforms/shared-utils.tscomponent/src/charts/__tests__/bar-chart.test.tsxcomponent/src/charts/__tests__/contrast-text-color.test.tscomponent/src/charts/__tests__/line-chart.test.tsxcomponent/src/charts/bar-chart.tsxcomponent/src/charts/chart-utils.tscomponent/src/charts/index.tscomponent/src/charts/line-chart.tsxcomponent/src/charts/single-value-chart.tsxcomponent/src/components/composed/__tests__/chart-options-schema.test.tscomponent/src/components/composed/chart-options/table.ts
CI surfaced two issues after the MEDIUM push: 1. SonarCloud — 8% duplication on new code (>3% gate). The autoAria useMemo block was duplicated 21 lines × 2 between bar-chart.tsx and line-chart.tsx. Extracted to `buildAutoAriaDescription(chartType, data, labelKey, rowNoun)` in chart-utils.ts. 2. E2E — design-system.spec.ts "chart container has role='img' and auto-generated aria-label" asserted the old ECharts AriaComponent template (/This is a chart/). Our new BarChart now passes its own data-shape description through, so the aria-label is "Bar chart with 5 categories and 1 series: value" — strictly better for AT users. Updated the assertion to match the new format. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
component/src/charts/chart-utils.ts (1)
120-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
rgb()/rgba()arity and alpha before accepting the color.The parser accepts malformed values (e.g., extra channels or invalid alpha in
rgba()) because it only validatesparts.length < 3. This undermines the "unparseable input falls back safely" contract.🔧 Proposed fix (enforces exact arity and validates alpha)
- const rgb = /^rgba?\(([^)]+)\)$/i.exec(s); + const rgb = /^(rgb|rgba)\(([^)]+)\)$/i.exec(s); if (rgb) { - const parts = rgb[1].split(",").map((p) => p.trim()); - if (parts.length < 3) return null; + const fn = rgb[1].toLowerCase(); + const parts = rgb[2].split(",").map((p) => p.trim()); + if ( + (fn === "rgb" && parts.length !== 3) || + (fn === "rgba" && parts.length !== 4) + ) { + return null; + } const out: number[] = []; for (let i = 0; i < 3; i++) { const p = parts[i]; let n: number; if (p.endsWith("%")) { n = (Number(p.slice(0, -1)) / 100) * 255; } else { n = Number(p); } if (!Number.isFinite(n)) return null; out.push(Math.max(0, Math.min(255, n))); } + if (fn === "rgba") { + const alpha = Number(parts[3]); + if (!Number.isFinite(alpha) || alpha < 0 || alpha > 1) return null; + } return [out[0], out[1], out[2]]; }🤖 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 `@component/src/charts/chart-utils.ts` around lines 120 - 136, The rgb/rgba parser currently only checks parts.length < 3 and thus accepts extra channels or bad alpha; update the logic around the regex match (variable rgb), the parts array, and the out construction to enforce exact arity: require parts.length === 3 for rgb inputs and parts.length === 4 for rgba inputs, and when parts.length === 4 validate the alpha channel (parts[3]) is a finite number between 0 and 1 (reject otherwise). Keep existing numeric clamping for RGB channels (the out array push logic) but return null for any invalid alpha or wrong number of channels so malformed inputs are rejected.
🤖 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.
Duplicate comments:
In `@component/src/charts/chart-utils.ts`:
- Around line 120-136: The rgb/rgba parser currently only checks parts.length <
3 and thus accepts extra channels or bad alpha; update the logic around the
regex match (variable rgb), the parts array, and the out construction to enforce
exact arity: require parts.length === 3 for rgb inputs and parts.length === 4
for rgba inputs, and when parts.length === 4 validate the alpha channel
(parts[3]) is a finite number between 0 and 1 (reject otherwise). Keep existing
numeric clamping for RGB channels (the out array push logic) but return null for
any invalid alpha or wrong number of channels so malformed inputs are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17a1efab-b19e-4824-ac91-5295a571bac6
📒 Files selected for processing (4)
app/e2e/design-system.spec.tscomponent/src/charts/bar-chart.tsxcomponent/src/charts/chart-utils.tscomponent/src/charts/line-chart.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- component/src/charts/bar-chart.tsx
Four inline comments on the original push, three still valid; addressed
all three:
- table-renderer.tsx (MAJOR): the pagination "flash" gate only checked
`containerHeight === undefined`. A momentary 0-height measurement
before layout settled could still slip through and trigger the snap.
Treat 0/negative heights as "not ready" too; pass containerHeight
to DataGrid only when it is strictly positive.
- shared-utils.ts toSeriesNumber (MINOR): `Number(" ")` returns 0,
so whitespace-only strings were silently masquerading as real zeros.
Trim-check string inputs and return null for blanks. New test cases
cover spaces, tabs, newlines.
- chart-utils.ts parseColorToRgb (MINOR): `rgb(1,2,3,4,5)` was parsed
as `(1,2,3)` because we only validated the first three components.
Enforce strict arity (rgb=3, rgba=4) and validate alpha is in [0,1].
New tests cover wrong arity + out-of-range alpha + valid rgba.
The fourth comment (line-chart.tsx key-collection duplication) was
already addressed by 09319f5 — the autoAria copy of the loop was
removed when we extracted `buildAutoAriaDescription` into chart-utils.
Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
- chart-utils: split parseColorToRgb into parseHexColor + parseRgbFunctionColor helpers (Cognitive Complexity 25 -> below gate) - chart-utils: replace parseInt with Number.parseInt (6 minor smells) - line-chart: extract collectSeriesKeys + findLastNumericValue + buildSeries out of the options useMemo (Cognitive Complexity 18 -> below gate) - table-renderer: use <section aria-label> instead of <div role="region"> for native landmark semantics Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
SonarCloud flagged `seriesKeys.map(buildSeries)` because Array.map passes (element, index, array) and a direct function reference can pick up the third arg unintentionally. Wrap explicitly to make the (key, idx) contract the only thing buildSeries sees. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
|
This was referenced May 20, 2026
This was referenced Jun 19, 2026
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.
Summary
Five HIGH-severity and two MEDIUM-severity findings from the post-merge
chart code review (line / bar / table), all fixed in this PR.
HIGH
Object.keys(records[0])drops series absent from the first row (sparse data)collectAllKeysunions keys across every row; mirrored inbar-chart.tsx/line-chart.tsxcontrastTextColorcrashed / produced invisible text onrgb()/ named colorschart-utilswith defensive hex/rgb parser + safe fallback to#000000pageSize=10then snaps to dynamic size whenResizeObserverfiresDataGridmount untilcontainerHeightis measured (only when pagination is enabled)enableGlobalFilterUI — schema advertised "Global Search" but no input was ever renderedMEDIUM
ariaDescriptionstill wins<table>had no top-level aria-label, leaving AT users with no contextrole="region"+ auto-derivedaria-label("Table with N rows and M columns"); overridable viasettings.ariaLabelFalse positives (verified, no change needed)
buildEmptyDataOption()(theme-aware centered "No data" title) used atbar-chart.tsx:92.chart-utils.ts:191already callsescapeHtml(p.seriesName); numeric values + header + abs values in the percent formatter are all escaped too. TheendLabel: { formatter: "{a}" }token renders to canvas, not HTML.Test plan
component/— 476 chart tests pass (added 8 ARIA-description cases + 9 contrast-text-color cases)app/— passes (added 8 new shared-utils cases, updated 2 transform tests)app/+component/cleancharts.spec.ts+styling-rules.spec.ts+widget-states.spec.ts— 43 passed, 4 flaky retry-pass (pre-existing graph-login flake, unrelated), 3 skipped, 0 failuresNotes
"not-a-number"→0. Updated to assertnull(with comments explaining why) — this is the intentional behavior change.enableGlobalFilteris removed from the schema only; theDataGridprop stays in case downstream callers wire it up via a custom toolbar.Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests