fix(charts): reject long-format results in bar and line (#1400) - #1468
Conversation
alfredo1996
added
bug
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughBar and line charts now validate mapped plotted columns for numeric values. Long-format results with non-numeric series columns show validation errors. Tests cover mappings and sparse data. Chart Reference queries now return separate wide-format series columns. ChangesChart validation and reference updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 1 minute. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 58 minutes. |
A `category, series, value` result — what GROUP BY a, b naturally produces — rendered a chart that looked plausible and was wrong. resolveValueKeys treats every non-label column as a value series, so `series` became a series whose cells all coerced to null: duplicated x labels, a ghost legend entry with no bars, and no stacking despite stackMode being set. Line drew one value line zig-zagging across repeated x values, which reads as a spiky time series but is artifact. validateNumericValueColumns rejects a value column that has non-null cells and none that parse, naming it and showing the pivot that fixes it. An all-null column stays legal — that is a sparse series, what a LEFT JOIN produces, and the reason collectAllKeys unions keys. Resolution goes through resolveValueKeys, the same call the transform makes, so the validator cannot disagree with what is plotted. validate() now takes the column mapping, as transformWithMapping already did. Without it the validator has to assume positional defaults and would reject a result whose text columns the user had already mapped away. The 11 reference tiles using this shape are rewritten to wide format so the stackMode tiles demonstrate real stacking. page-line-v-28's rightAxisSeries: "shipped" now names a column that exists. Closes #1400 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dabf8a3 to
62c8b15
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…-long-format-reject # Conflicts: # CHANGELOG.md
|
2 participants
Closes #1400
The bug
A long-format result —
category, series, value, whatGROUP BY a, bnaturally produces — rendered a chart that looked plausible and was wrong.Bar: 12 bars with duplicated x labels, a ghost
serieslegend swatch with no bars, and no stacking at all despitestackModebeing set.Line: worse — a single revenue line drawn across duplicated x values, interleaving delivered → shipped → delivered, which reads as a violently spiky time series but is pure artifact.
resolveValueKeystreats every non-label column as a value series, soseriesbecame a series of its own;toSeriesNumberturned'delivered'intonull, which is why the legend entry existed with no bars.validateBarDataonly counted columns, so nothing rejected it.The fix — reject, don't pivot
validateNumericValueColumnsintransforms/shared-utils.ts, wired into both validators. Rejection rule: a column is flagged only when it has at least one non-null cell and none that parse. An entirely null column stays legal — that is a sparse series, what aLEFT JOINproduces, and the reasoncollectAllKeysunions keys in the first place.Resolution goes through
resolveValueKeys— the same call the transform makes — so the validator cannot disagree with what is actually plotted.Pivoting long→wide (the issue's option 2) is deliberately not here: it's a new feature against the no-new-features directive and overlaps #1425, already in v1.6. Even with a pivot, a genuinely non-numeric value column must still be rejected rather than coerced to null, so this lands either way.
Two things the issue didn't anticipate
1.
validatecouldn't see the column mapping. Its signature was(data: unknown), so a validator had to assume positional defaults and would wrongly reject a result whose text columns the user had already mapped away.columnMappingwas already in scope at bothcard-containercall sites — used on the very next line for the transform — so the signature is widened to(data, mapping?)to matchtransformWithMapping.2. 11 reference tiles used this exact shape (5 bar, 6 line). Left alone they would have gone from wrong chart to "Incompatible data format", which is worse for a reference dashboard. All 11 are rewritten to wide format with
SUM(...) FILTER (WHERE ...), so thestackModetiles finally demonstrate real stacking. Side effect:page-line-v-28setsrightAxisSeries: "shipped", a column that did not exist under the old query — that tile starts working too.Tests
19 new: 10 for the helper (including both mapping paths and the sparse-column cases), 3 per validator, 3 jsdom.
Note
jsdom coverage of any validator was fictional before this PR.
chart-helpersregisters lightweight stub plugins when the real plugin modules haven't loaded, andLightDefcarries novalidatefield — sochartConfig.validateisundefinedin every jsdom test. My first attempt passed a long-format result straight through to a rendered chart for exactly that reason. The test now attaches the realvalidateBarDataviaimportActualand says why; importing the plugin module itself would drag ECharts into jsdom for no benefit.Verification
app3499/3499 · typecheck + lint clean · chart E2E (charts, chart-export, heavy-widgets, widget-editor) 48 passed, 3 skipped, 1 flaky —chart-exportPNG-download timing, unrelated spec, green on retry.Follow-up worth filing
ColumnMapping.groupByexists to designate a series column butresolveValueKeysonly excludes the label key — so even a user who correctly declaresgroupBy: "series"gets it plotted as a series. This PR rejects that case with a clear message rather than rendering garbage, which is the right outcome while no pivot exists. WiringgroupByup properly belongs with the long→wide pivot in #1425.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Examples