improve: charts — table move, error boundary, options split, dark mode, graph/map styling - #282
Merged
Merged
Conversation
Closes #257 - Add ChartErrorBoundary class component wrapping all chart output - On render crash: shows "Chart failed to render" with error message instead of crashing the entire dashboard - Logs error + component stack to console for debugging - Key includes widgetId for proper reset on type/widget change - Add 3 tests: error fallback, normal render, unknown type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract ChartErrorBoundary from chart-renderer.tsx into its own file so SonarCloud measures coverage on the boundary code alone, not the entire switch statement. Add 3 direct unit tests for the boundary component (render children, error fallback, console.error logging). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes TS2786 in CI strict mode — `never` return type can't be used as JSX. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract ChartErrorBoundary to own file and use a thin wrapper pattern (ChartRenderer → ChartRendererInner) instead of renderChart() to keep the switch statement unchanged from the base branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hook (#277) Closes #260 Replace the MutationObserver on <html class="dark"> with an event-driven approach that listens to: 1. `neoboard-theme-change` custom event (from app's useTheme) 2. OS `prefers-color-scheme` media query changes 3. `storage` events (cross-tab theme sync) The hook still reads <html class="dark"> as the source of truth, but reacts to events rather than polling the DOM via MutationObserver. This is more reliable and integrates with the app's theme system without creating a dependency on it. Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…275) * refactor(component): split chart-options-schema into per-chart files Closes #258 Split the 1090-line monolithic chart-options-schema.ts into 20 focused files: - chart-options/shared.ts — ChartOptionDef type, shared constants, reusable option arrays - chart-options/{bar,line,pie,...}.ts — 17 per-chart option definitions - chart-options/index.ts — registry, getChartOptions(), getDefaultChartSettings() - chart-options-schema.ts — backward-compatible re-export shim All 2,752 tests pass (1194 component + 1558 app). Zero API changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(component): add missing ChartOptionDef import, fix implicit any in test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Closes #259 Graph chart: - Add stylingRules/paramValues props to GraphChartProps - Evaluate rules against node.value in toNvlNode() - Rule color takes priority over explicit node.color and label palette - 3 new tests: rule match, priority over explicit color, no-value skip Map chart: - Add stylingRules/paramValues props to MapChartProps - Evaluate rules against marker.value in marker creation loop - Rule color takes priority over explicit marker.color - 3 new tests (TDD: written RED first, then implemented GREEN) Registry + renderer: - Set supportsStyling: true for graph and map - Add stylingTargets with "Node Color" / "Marker Color" - Forward stylingRules/paramValues in chart-renderer.tsx - Update 4 existing tests that expected graph/map to be unstyled Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses CodeRabbit review on PR #274 — content widgets (markdown/iframe) were missing the `meta` prop, causing error boundary key collisions when multiple content widgets exist on the same dashboard. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 48 seconds. ⌛ 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 (2)
WalkthroughRefactors ChartRenderer to wrap renders with ChartErrorBoundary and change its signature; forwards Changes
Sequence DiagramsequenceDiagram
actor User
participant ChartRenderer
participant ChartErrorBoundary
participant GraphOrMap as GraphChart/MapChart
participant StylingEvaluator as Styling Rule<br/>Evaluator
participant Canvas as Chart Canvas
User->>ChartRenderer: Request render (type, data, stylingRules, paramValues)
ChartRenderer->>ChartErrorBoundary: Render child (chartType)
ChartErrorBoundary->>GraphOrMap: Render chart with stylingRules & paramValues
GraphOrMap->>GraphOrMap: Iterate nodes/markers
alt item has value
GraphOrMap->>StylingEvaluator: resolveStylingRuleColor(value, rules, paramValues)
StylingEvaluator-->>GraphOrMap: color (if matched)
GraphOrMap->>GraphOrMap: apply rule color (highest priority)
else no value
GraphOrMap->>GraphOrMap: fall back to explicit color/default
end
GraphOrMap->>Canvas: Draw chart with resolved colors
Canvas-->>User: Display chart
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
component/src/components/composed/chart-options/gauge.ts (1)
60-68: Consider a structured zones type for better UX.Using a text field for JSON input is functional but error-prone. The description helpfully shows the expected format, but users could easily mistype the JSON.
If
ChartOptionDefsupports a structured array/object type, that would provide validation and a better editing experience. This is a minor UX improvement that could be deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/components/composed/chart-options/gauge.ts` around lines 60 - 68, The "thresholdZones" option currently uses a freeform text field with JSON which is error-prone; change its ChartOptionDef from type "text" to a structured array/object type (e.g., an array of zone objects) so the editor can validate and provide a better UX, update its default from "" to a typed default like [] or [{value:30,color:"#67e0e3"},...], replace the JSON description with a schema/field-level descriptions for each zone property, and ensure any parsing code that reads "thresholdZones" (refer to the "thresholdZones" key in gauge.ts and any code that consumes ChartOptionDef) handles the new typed structure instead of raw JSON text.component/src/charts/map-chart.tsx (1)
43-46: Narrow prop docs to color-only to match behavior.The current comment says marker color/size, but this implementation path only drives marker color.
✏️ Suggested doc-only patch
- /** Rule-based styling rules for marker color/size */ + /** Rule-based styling rules for marker color */ stylingRules?: StylingRule[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/map-chart.tsx` around lines 43 - 46, Update the prop JSDoc in map-chart.tsx to accurately reflect that the stylingRules prop only controls marker color (not size); change the comment on stylingRules to mention color-only rule-based styling and optionally clarify paramValues is used for rule evaluation. Specifically update the doc for the stylingRules property (and adjust paramValues comment if present) so it references marker color only, using the existing identifiers stylingRules and paramValues to locate the props.component/src/charts/graph-chart.tsx (1)
130-133: Align styling docs with implemented behavior.Current wording says rules affect “color/size”, but this path applies only color. Tightening these comments will avoid API confusion.
✏️ Suggested doc-only patch
- /** Rule-based styling rules for node color/size */ + /** Rule-based styling rules for node color */ stylingRules?: StylingRule[];- * Color priority: explicit node.color > last-label color from palette > undefined. + * Color priority: styling rule > explicit node.color > last-label color from palette > undefined.Also applies to: 250-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/graph-chart.tsx` around lines 130 - 133, The JSDoc for stylingRules is incorrect: it claims rules affect “color/size” but the implemented logic only applies color; update the comments for stylingRules (and the duplicate docs around the second occurrence) to state these rules control node color only (e.g., "Rule-based styling rules for node color") and keep paramValues doc as-is; locate the docs next to the stylingRules and paramValues declarations in GraphChart (symbols: stylingRules, paramValues) and adjust both occurrences so the API docs reflect actual behavior.component/src/components/composed/chart-options/index.ts (1)
61-65: Consider adding accessibility options to all ECharts chart registries.
gauge,sankey,sunburst,radar, andtreemapcurrently missaccessibilityOptions, which creates inconsistent option parity with other ECharts charts.♻️ Suggested registry alignment
- gauge: [...gaugeOptions, ...behaviorOptions, ...appearanceOptions], - sankey: [...sankeyOptions, ...behaviorOptions, ...appearanceOptions], - sunburst: [...sunburstOptions, ...behaviorOptions, ...appearanceOptions], - radar: [...radarOptions, ...behaviorOptions, ...appearanceOptions], - treemap: [...treemapOptions, ...behaviorOptions, ...appearanceOptions], + gauge: [ + ...gaugeOptions, + ...behaviorOptions, + ...appearanceOptions, + ...accessibilityOptions, + ], + sankey: [ + ...sankeyOptions, + ...behaviorOptions, + ...appearanceOptions, + ...accessibilityOptions, + ], + sunburst: [ + ...sunburstOptions, + ...behaviorOptions, + ...appearanceOptions, + ...accessibilityOptions, + ], + radar: [ + ...radarOptions, + ...behaviorOptions, + ...appearanceOptions, + ...accessibilityOptions, + ], + treemap: [ + ...treemapOptions, + ...behaviorOptions, + ...appearanceOptions, + ...accessibilityOptions, + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/components/composed/chart-options/index.ts` around lines 61 - 65, The chart registries for gauge, sankey, sunburst, radar, and treemap are missing accessibilityOptions causing inconsistent option parity; update each registry entry (symbols: gauge, sankey, sunburst, radar, treemap) to include accessibilityOptions alongside their existing arrays (e.g., change gauge: [...gaugeOptions, ...behaviorOptions, ...appearanceOptions] to include ...accessibilityOptions) so all ECharts chart registries uniformly include accessibilityOptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@component/src/components/composed/chart-options/iframe.ts`:
- Around line 23-30: The sandbox option (key "sandbox", label "Sandbox Policy")
currently defaults to "allow-scripts allow-popups", which is too permissive;
change its default to a restrictive value (e.g., an empty string or no flags) so
embedded iframes are isolated by default, and update the description to instruct
users to explicitly opt-in to specific sandbox flags when needed; ensure you
only modify the default and description for the "sandbox" option so existing
callers/readers see the safer default behavior.
In `@component/src/components/composed/chart-options/table.ts`:
- Around line 81-85: The default value for the "groupBy" option (key: "groupBy",
type: "column-multi-select") is currently an empty string which can cause type
mismatches; change its default to an empty array (e.g., []) so the multi-select
initializes as an array type and update any related typing or consumers that
assume an array (look for references to "groupBy" in the same module and any
getters/validators that expect an array) to ensure consistency.
---
Nitpick comments:
In `@component/src/charts/graph-chart.tsx`:
- Around line 130-133: The JSDoc for stylingRules is incorrect: it claims rules
affect “color/size” but the implemented logic only applies color; update the
comments for stylingRules (and the duplicate docs around the second occurrence)
to state these rules control node color only (e.g., "Rule-based styling rules
for node color") and keep paramValues doc as-is; locate the docs next to the
stylingRules and paramValues declarations in GraphChart (symbols: stylingRules,
paramValues) and adjust both occurrences so the API docs reflect actual
behavior.
In `@component/src/charts/map-chart.tsx`:
- Around line 43-46: Update the prop JSDoc in map-chart.tsx to accurately
reflect that the stylingRules prop only controls marker color (not size); change
the comment on stylingRules to mention color-only rule-based styling and
optionally clarify paramValues is used for rule evaluation. Specifically update
the doc for the stylingRules property (and adjust paramValues comment if
present) so it references marker color only, using the existing identifiers
stylingRules and paramValues to locate the props.
In `@component/src/components/composed/chart-options/gauge.ts`:
- Around line 60-68: The "thresholdZones" option currently uses a freeform text
field with JSON which is error-prone; change its ChartOptionDef from type "text"
to a structured array/object type (e.g., an array of zone objects) so the editor
can validate and provide a better UX, update its default from "" to a typed
default like [] or [{value:30,color:"#67e0e3"},...], replace the JSON
description with a schema/field-level descriptions for each zone property, and
ensure any parsing code that reads "thresholdZones" (refer to the
"thresholdZones" key in gauge.ts and any code that consumes ChartOptionDef)
handles the new typed structure instead of raw JSON text.
In `@component/src/components/composed/chart-options/index.ts`:
- Around line 61-65: The chart registries for gauge, sankey, sunburst, radar,
and treemap are missing accessibilityOptions causing inconsistent option parity;
update each registry entry (symbols: gauge, sankey, sunburst, radar, treemap) to
include accessibilityOptions alongside their existing arrays (e.g., change
gauge: [...gaugeOptions, ...behaviorOptions, ...appearanceOptions] to include
...accessibilityOptions) so all ECharts chart registries uniformly include
accessibilityOptions.
🪄 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: 883fe174-3016-47f9-b938-abeb7093838f
📒 Files selected for processing (32)
app/src/components/chart-renderer.tsxapp/src/lib/__tests__/chart-registry.test.tsapp/src/lib/__tests__/widget-actions.test.tsapp/src/lib/chart-registry.tsapp/src/stores/__tests__/widget-editor-store.test.tscomponent/src/charts/__tests__/base-chart.test.tsxcomponent/src/charts/__tests__/graph-chart.test.tsxcomponent/src/charts/__tests__/map-chart.test.tsxcomponent/src/charts/base-chart.tsxcomponent/src/charts/graph-chart.tsxcomponent/src/charts/map-chart.tsxcomponent/src/components/composed/__tests__/chart-options-schema.test.tscomponent/src/components/composed/chart-options-schema.tscomponent/src/components/composed/chart-options/bar.tscomponent/src/components/composed/chart-options/form.tscomponent/src/components/composed/chart-options/gauge.tscomponent/src/components/composed/chart-options/graph.tscomponent/src/components/composed/chart-options/iframe.tscomponent/src/components/composed/chart-options/index.tscomponent/src/components/composed/chart-options/json.tscomponent/src/components/composed/chart-options/line.tscomponent/src/components/composed/chart-options/map.tscomponent/src/components/composed/chart-options/markdown.tscomponent/src/components/composed/chart-options/parameter-select.tscomponent/src/components/composed/chart-options/pie.tscomponent/src/components/composed/chart-options/radar.tscomponent/src/components/composed/chart-options/sankey.tscomponent/src/components/composed/chart-options/shared.tscomponent/src/components/composed/chart-options/single-value.tscomponent/src/components/composed/chart-options/sunburst.tscomponent/src/components/composed/chart-options/table.tscomponent/src/components/composed/chart-options/treemap.ts
Comment on lines
+23
to
+30
| key: "sandbox", | ||
| label: "Sandbox Policy", | ||
| type: "text", | ||
| default: "allow-scripts allow-popups", | ||
| category: "Security", | ||
| description: | ||
| "HTML sandbox attributes controlling what the embedded page can do. Restrict for untrusted content.", | ||
| }, |
There was a problem hiding this comment.
Harden iframe sandbox default (currently too permissive).
Line 26 enables scripts and popups by default, which weakens isolation for untrusted embeds. Use a restrictive default and let users opt in only when needed.
Proposed fix
{
key: "sandbox",
label: "Sandbox Policy",
type: "text",
- default: "allow-scripts allow-popups",
+ default: "",
category: "Security",
description:
"HTML sandbox attributes controlling what the embedded page can do. Restrict for untrusted content.",
},📝 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
| key: "sandbox", | |
| label: "Sandbox Policy", | |
| type: "text", | |
| default: "allow-scripts allow-popups", | |
| category: "Security", | |
| description: | |
| "HTML sandbox attributes controlling what the embedded page can do. Restrict for untrusted content.", | |
| }, | |
| key: "sandbox", | |
| label: "Sandbox Policy", | |
| type: "text", | |
| default: "", | |
| category: "Security", | |
| description: | |
| "HTML sandbox attributes controlling what the embedded page can do. Restrict for untrusted content.", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/components/composed/chart-options/iframe.ts` around lines 23 -
30, The sandbox option (key "sandbox", label "Sandbox Policy") currently
defaults to "allow-scripts allow-popups", which is too permissive; change its
default to a restrictive value (e.g., an empty string or no flags) so embedded
iframes are isolated by default, and update the description to instruct users to
explicitly opt-in to specific sandbox flags when needed; ensure you only modify
the default and description for the "sandbox" option so existing callers/readers
see the safer default behavior.
feat(app): add error boundary for chart rendering
…t to array - Set iframe sandbox default to "" (fully sandboxed) instead of "allow-scripts allow-popups" for security - Set table groupBy default to [] (array) to match column-multi-select type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
Mar 31, 2026
- Fix TableRenderer import to use local path (component/ move is in #282) - Add null guard to getChartConfig() in component field test - Rewrite useClickAction tests to import actual hook and exercise real functions instead of only mocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-integration # Conflicts: # component/src/components/composed/chart-options-schema.ts
|
6 tasks
alfredo1996
pushed a commit
that referenced
this pull request
Mar 31, 2026
…t options The syncToUrl option was added to the monolithic chart-options-schema in PR #284, but when PR #282 split it into per-chart files, the new parameter-select.ts didn't include it. This caused the test to fail on all branches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
Mar 31, 2026
Graph and map charts now have stylingTargets (added in #282). Update tests to expect true for chartSupportsStyling and non-empty styling targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6 tasks
alfredo1996
pushed a commit
that referenced
this pull request
May 10, 2026
- Fix TableRenderer import to use local path (component/ move is in #282) - Add null guard to getChartConfig() in component field test - Rewrite useClickAction tests to import actual hook and exercise real functions instead of only mocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
added a commit
that referenced
this pull request
May 10, 2026
improve: charts — table move, error boundary, options split, dark mode, graph/map styling
alfredo1996
pushed a commit
that referenced
this pull request
May 10, 2026
…t options The syncToUrl option was added to the monolithic chart-options-schema in PR #284, but when PR #282 split it into per-chart files, the new parameter-select.ts didn't include it. This caused the test to fail on all branches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 10, 2026
Graph and map charts now have stylingTargets (added in #282). Update tests to expect true for chartSupportsStyling and non-empty styling targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Integration branch for chart improvements. Contains:
PRs still pending
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements