fix(charts): forward trendEnabled and thresholdZones, and pick the numeric KPI column (#1397) - #1473
Merged
Conversation
…meric KPI column (#1397) Both options were present in the widget editor, implemented in the component library, and silently dropped in between. Setting either did nothing, with no error. The issue's diagnosis was wrong: it blamed the Zod schema stripping unknown keys. Both schemas end in .passthrough(), which preserves them — verified against the real schema. The value died at the plugin's explicit prop mapping, which never forwarded it. Adding the key to the schema, the fix as filed, would have changed nothing on screen. trendEnabled could never have been forwarded as-is either: SingleValueChart takes a computed { direction, label }, not a boolean, and it needs the previous period — which transformToValueData had already discarded by collapsing the result to one scalar. That collapse is also the worse bug. It took the first column positionally, so following the editor's own "requires 2 rows" instruction on a `label, value` query rendered the date as the KPI: `$2026-03`. It now picks the first numeric column, falling back to a non-numeric one only when the result has none, and carries the second row's value as `previous`. Adds a ratchet asserting every option a chart type advertises is read by its plugin. The one proposed in the issue — comparing option keys against settingsSchema — would have passed while both bugs were live, since the schema was never the seam. The ratchet found 14 unforwarded options, not 2. The remaining 10 are allowlisted and tracked in #1472; the list may only shrink. The seeded reference tiles needed no changes — they were authored correctly and only the plumbing was broken. Closes #1397 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alfredo1996
added
bug
WalkthroughThe PR forwards ChangesChart option forwarding and value trends
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PluginComponent
participant transformToValueData
participant SingleValueChart
PluginComponent->>transformToValueData: Transform query rows
transformToValueData-->>PluginComponent: Return value and previous
PluginComponent->>PluginComponent: Compute trend
PluginComponent->>SingleValueChart: Pass value and trend
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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/plugins/__tests__/option-forwarding.test.tsx`:
- Around line 75-87: Update the ratchet in the option-forwarding test around
pluginSource() and the allowlist expiry check so it ignores comments and
declarations, then validates executable reads of each option or its chart-prop
mapping using an AST-based predicate where possible. Ensure thresholdZones and
trendEnabled only satisfy the check when their runtime forwarding logic is
present, not merely when they appear in settings schemas or comments.
In `@app/src/plugins/single-value/component.tsx`:
- Around line 46-55: Add a jsdom test covering the zero-baseline branch in the
trend calculation, using a zero previous value and changed/equal current values
as appropriate. Assert the resulting direction and verify that SingleValueChart
receives no Infinity% label, following the existing test structure and writing
the test before implementation.
In `@app/src/plugins/single-value/transform.ts`:
- Around line 41-43: Update transformToValueData around the first-row inspection
to verify that records[0] is a non-null record before calling Object.keys or
reading its values. When the first row is null or otherwise invalid, return the
existing fallback value; preserve the current numeric-key selection for valid
record rows.
- Around line 46-55: Update the value construction in the exported transform to
ensure boolean results from normalizeValue are converted to strings before
returning, keeping SingleValueData.value restricted to string | number. Preserve
numeric conversion and the existing zero fallback, and add a test covering an
enabled: true input.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24752b8b-e699-450a-8748-ed75bc56c7ec
📒 Files selected for processing (9)
CHANGELOG.mdapp/src/lib/plugin/__tests__/chart-option-forwarding.test.tsapp/src/plugins/__tests__/option-forwarding.test.tsxapp/src/plugins/gauge/component.tsxapp/src/plugins/gauge/settings.tsapp/src/plugins/single-value/component.tsxapp/src/plugins/single-value/settings.tsapp/src/plugins/single-value/transform.tsapp/src/plugins/transforms/__tests__/remaining.test.ts
Comment on lines
+75
to
+87
| const el = screen.getByTestId("chart"); | ||
| expect(el.getAttribute("data-trend-direction")).toBe("up"); | ||
| expect(el.getAttribute("data-trend-label")).toBe("25.0%"); | ||
| }); | ||
|
|
||
| it("renders a downward trend when the value fell", () => { | ||
| render( | ||
| <SingleValueComponent | ||
| data={transformToValueData([...twoRows].reverse())} | ||
| settings={{ trendEnabled: true }} | ||
| />, | ||
| ); | ||
| expect( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the ratchet inspect executable option reads.
pluginSource() includes settings.ts and raw comments. thresholdZones and trendEnabled now occur in their schemas, so Line 131 passes even if their chart prop mapping is removed. Comments can also satisfy the current word match.
Exclude declarations and comments from this check. Assert a concrete settings read or chart-prop mapping, preferably with an AST-based predicate. Apply the same predicate to the allowlist expiry check.
🤖 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 `@app/src/plugins/__tests__/option-forwarding.test.tsx` around lines 75 - 87,
Update the ratchet in the option-forwarding test around pluginSource() and the
allowlist expiry check so it ignores comments and declarations, then validates
executable reads of each option or its chart-prop mapping using an AST-based
predicate where possible. Ensure thresholdZones and trendEnabled only satisfy
the check when their runtime forwarding logic is present, not merely when they
appear in settings schemas or comments.
Comment on lines
+46
to
+55
| if (previous === 0) { | ||
| return { | ||
| direction, | ||
| label: direction === "neutral" ? "no change" : undefined, | ||
| }; | ||
| } | ||
| const pct = Math.abs(delta / previous) * 100; | ||
| return { | ||
| direction, | ||
| label: direction === "neutral" ? "no change" : `${pct.toFixed(1)}%`, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test the zero-baseline trend branch.
When previous === 0, this branch omits a percentage label for changes and returns "no change" for equal values. The supplied tests do not cover this behavior.
Add a jsdom test with a zero previous value. Assert the direction and that no Infinity% label reaches SingleValueChart.
As per coding guidelines, “Every new behavior, bug fix, and edge case must have a test, written before implementation, following Red → Green → Refactor.”
🤖 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 `@app/src/plugins/single-value/component.tsx` around lines 46 - 55, Add a jsdom
test covering the zero-baseline branch in the trend calculation, using a zero
previous value and changed/equal current values as appropriate. Assert the
resulting direction and verify that SingleValueChart receives no Infinity%
label, following the existing test structure and writing the test before
implementation.
Source: Coding guidelines
Comment on lines
41
to
+43
| const first = records[0]; | ||
| const values = Object.values(first); | ||
| return normalizeValue(values[0]) ?? 0; | ||
| const keys = Object.keys(first); | ||
| const numericKey = keys.find((k) => toNumber(first[k]) !== null); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard non-record rows before inspecting keys.
toRecords() returns arrays unchanged. transformToValueData([null]) assigns null to first, then Line 42 calls Object.keys(first) and throws. This can prevent the widget from rendering for malformed connector output.
Validate or filter rows before reading keys. Return the existing fallback value when the first row is not a record.
🤖 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 `@app/src/plugins/single-value/transform.ts` around lines 41 - 43, Update
transformToValueData around the first-row inspection to verify that records[0]
is a non-null record before calling Object.keys or reading its values. When the
first row is null or otherwise invalid, return the existing fallback value;
preserve the current numeric-key selection for valid record rows.
Comment on lines
+46
to
+55
| const raw = key === undefined ? undefined : first[key]; | ||
| const value = | ||
| (numericKey ? toNumber(raw) : (normalizeValue(raw) ?? 0)) ?? 0; | ||
|
|
||
| const previous = | ||
| records.length > 1 && key !== undefined | ||
| ? (toNumber(records[1][key]) ?? undefined) | ||
| : undefined; | ||
|
|
||
| return { value: value as string | number, previous }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep SingleValueData.value within its declared type.
For { enabled: true }, normalizeValue(raw) returns true. Line 55 casts that boolean to string | number without converting it. Callers of this exported transform can therefore receive a runtime value excluded by SingleValueData.
Convert boolean fallback values to strings, or widen the interface and downstream chart contract. Add a boolean fallback test.
🤖 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 `@app/src/plugins/single-value/transform.ts` around lines 46 - 55, Update the
value construction in the exported transform to ensure boolean results from
normalizeValue are converted to strings before returning, keeping
SingleValueData.value restricted to string | number. Preserve numeric conversion
and the existing zero fallback, and add a test covering an enabled: true input.
|
5 tasks
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.
Closes #1397 · follow-up #1472
Important
The issue's diagnosis was wrong and its proposed fix was a no-op. Detail in this comment; summarised below. The acceptance criteria changed as a result.
What was broken
trendEnabled(single-value) andthresholdZones(gauge) were present in the widget editor, implemented in the component library, and silently discarded in between. Setting either did nothing, with no error — and the seeded reference dashboard shipped tiles demonstrating both, with thethresholdZones (3 bands)tile pixel-identical todefault.It was not the schema
The issue blames the plugin Zod schema
.strip()ing unknown keys. Both schemas end in.passthrough(), which preserves them. Against the real schema:The option arrives intact and dies at the plugin's explicit prop mapping —
GaugePluginComponentmaps eight settings onto<GaugeChart>andthresholdZonesisn't one. So the filed fix ("wire both through the plugin schemas") would have changed nothing visible. The keys are added to the schemas here for typing, but that is not what fixes it.trendEnabledwas never a forwardable propSingleValueCharttakestrend?: { direction, label }— a computed object, not a boolean. It needs the previous period, whichtransformToValueDatahad already thrown away by collapsing the result to a single scalar. So the option and the KPI bug below share one root.The worse half: the KPI showed a date
transformToValueDatatook the first column positionally. The editor says trend "requires 2 rows in the query result", so the natural query islabel, value— and the KPI rendereda date string with a currency prefix, as the headline metric. It now picks the first numeric column, falls back to a non-numeric one only when there is no numeric column (the legitimate "status text" case), and carries the second row's value as
previous.The ratchet — and why the proposed one wouldn't work
That would have passed while both bugs were live. The schema was never the seam, and it fails the other way too: a key in the schema is still dropped if the component doesn't forward it.
This PR asserts the seam that matters — is this option actually read by the plugin that advertises it — across all 20 chart types. It is source-based rather than importing
getChartOptions, because that barrel pulls in@neo4j-nvl, a browser-only WebGL dependency that cannot load in the node test environment. The parsing it relies on is itself guarded by a test that fails if any chart type yields zero option keys, so a refactor can't make it pass vacuously.Known limits, stated in the test: it catches "advertised but never wired", not "wired but misused", and not "wired to dead code".
Scope: 14, not 2
The ratchet found unforwarded options in 7 chart types. A first pass reported 29 — that over-counted, because
tableandformdelegate rendering toTableRenderer/FormWidgetRenderer, which do read their options; the check now follows that one hop.Fixed here:
gauge.thresholdZones,single-value.trendEnabled.Allowlisted and tracked in #1472:
graph(3),json(3),line(2),map(2).Allowlisted for a different reason:
parameter-select.defaultValue/.syncToUrlare consumed by dashboard-level code outside the render path —syncToUrlworks (#1388),defaultValuedoesn't, but that's #1421.The allowlist may only shrink: a separate test fails if an allowlisted key becomes read without being removed from the list.
Seeded tiles
No changes needed.
page-single-value-v-81andpage-gauge-v-89were authored correctly all along; only the plumbing between them and the chart was broken. They now demonstrate the real behaviour.Tests
transformToValueDatacases: numeric-column selection, numeric strings, the no-numeric-column fallback, andpreviousextraction$2026-03regressionVerification
app3540/3540 · typecheck 0 · lint 0🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes