feat(app): migrate all 13 remaining charts to plugin system (#220) - #380
Merged
Conversation
Fourth PR in the plugin system epic. Completes the migration — all
17 chart types now use the plugin registry. The switch statement in
chart-renderer.tsx is entirely gone.
New plugins (13):
- single-value.tsx, graph.tsx, map.tsx, table.tsx, json.tsx,
parameter-select.tsx, form.tsx, iframe.tsx, gauge.tsx, sankey.tsx,
sunburst.tsx, radar.tsx, treemap.tsx
chart-renderer.tsx changes:
- Removed the switch statement (was 540+ lines, now 130 lines total)
- Removed all dynamic imports for individual chart components
- Removed unused type imports
- Only delegates to pluginRegistry.get(type) now; unknown types
return a helpful "Unknown chart type" error state
Component wiring:
- Plugin adapter receives { data, settings, stylingRules, paramValues,
colorScales, onClick, onChartClick, connectionId, widgetId, resultId,
query, autoFit, clickableColumns, colorThresholds } from the renderer
- Each plugin picks the props it needs and calls the underlying component
- handleEChartsClick (ECharts wrapper) vs onChartClick (raw row callback)
are both passed — plugins pick the right one (e.g. map uses raw,
bar uses ECharts wrapper)
Special handling:
- Graph: dual path (GraphExplorationWrapper for widgets with connectionId,
GraphChart for previews) preserved
- Table/ParameterSelect/Form: use app-local components (imported from
@/components/*) rather than @neoboard/components
All tests pass (1877) + TypeScript clean + ESLint clean.
Related: #220, epic #221
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughReplaces legacy switch-based chart rendering with a plugin-driven renderer that looks up Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChartRenderer as ChartRenderer (UI)
participant PluginRegistry as pluginRegistry
participant Plugin as PluginComponent
participant ChartLib as Chart/Widget lib
User->>ChartRenderer: request render chart(type, props)
ChartRenderer->>PluginRegistry: pluginRegistry.get(type)
PluginRegistry-->>ChartRenderer: plugin (component, meta)
ChartRenderer->>Plugin: render component with props (data, settings, onChartClick...)
Plugin->>ChartLib: render visualization / widget (may use dynamic import)
ChartLib-->>User: interactive chart displayed / emits events
ChartLib->>Plugin: event callback (e.g., click)
Plugin->>ChartRenderer: (via onChartClick) propagate interaction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/plugins/form.tsx`:
- Around line 37-42: The capabilities object in the plugin incorrectly
advertises supportsClickAction: true while the form widget renderer
(app/src/components/form-widget-renderer.tsx) does not call onClick or
onChartClick and only accepts connectionId, query, and settings; change
capabilities.supportsClickAction to false to stop exposing an unimplemented
feature, and if you later implement click handling ensure
form-widget-renderer.tsx invokes onClick/onChartClick and then flip
supportsClickAction back to true.
In `@app/src/plugins/graph.tsx`:
- Around line 70-74: The onNodeSelect handler currently forwards only nodeId to
onChartClick (ids[0]) which is too narrow; update the onNodeSelect
implementation so that when ids.length > 0 it looks up the full node payload for
ids[0] (e.g., via the component's nodes prop or a getNodeById helper) and calls
onChartClick with that full node object (falling back to { nodeId: ids[0] } if
the node payload is unavailable) so rules that depend on other node fields
receive the complete payload.
In `@app/src/plugins/json.tsx`:
- Line 22: The runtime value for settings.initialExpanded must be validated
before passing to the component: in app/src/plugins/json.tsx read
settings.initialExpanded, coerce to a Number (e.g., via Number(...) or
parseInt), verify it is a finite integer (Number.isFinite and
Math.floor/Number.isInteger and enforce a sensible min like 0), and if
validation fails fallback to 2; then pass that validated value instead of the
raw (settings.initialExpanded as number) into initialExpanded.
In `@app/src/plugins/map.tsx`:
- Around line 50-58: The onMarkerClick handler currently maps marker "m" to a
reduced object with only id/label/lat/lng before calling onChartClick, which
strips other fields used by sourceField-based rules; instead pass the full
marker payload to onChartClick (i.e., call onChartClick(m) or spread the entire
m object) so all marker properties are preserved; update the onMarkerClick
invocation where onChartClick is referenced to forward the whole marker object
(variable m) rather than a partial subset.
In `@app/src/plugins/table.tsx`:
- Around line 37-45: The onCellClick handler currently only forwards
_clickedColumn and _clickedValue to onChartClick; update it to also pass the
full clicked row (or rowIndex) so downstream consumers can access id/slug/etc.
Modify the onCellClick branch that invokes onChartClick to include info.row (or
info.rowIndex) merged into the payload alongside _clickedColumn and
_clickedValue, ensuring the onChartClick signature is preserved and callers in
table-renderer.tsx and composed/data-grid.tsx receive the row data.
- Around line 9-10: The TableRenderer component is imported statically but must
be loaded with next/dynamic and ssr: false like other chart plugins; replace the
direct import of TableRenderer with a dynamic import using next/dynamic (e.g.,
const TableRenderer = dynamic(() => import(".../table-renderer"), { ssr: false
})) and update any references to the existing TableRenderer symbol to use this
dynamic variable so the component is client-only.
🪄 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: 5e5f19d6-43ad-4976-9158-b94198e7e6de
📒 Files selected for processing (15)
app/src/components/chart-renderer.tsxapp/src/plugins/form.tsxapp/src/plugins/gauge.tsxapp/src/plugins/graph.tsxapp/src/plugins/iframe.tsxapp/src/plugins/index.tsapp/src/plugins/json.tsxapp/src/plugins/map.tsxapp/src/plugins/parameter-select.tsxapp/src/plugins/radar.tsxapp/src/plugins/sankey.tsxapp/src/plugins/single-value.tsxapp/src/plugins/sunburst.tsxapp/src/plugins/table.tsxapp/src/plugins/treemap.tsx
Comment on lines
+37
to
+42
| capabilities: { | ||
| supportsClickAction: true, | ||
| supportsStyling: false, | ||
| isECharts: false, | ||
| requiresQuery: false, | ||
| }, |
There was a problem hiding this comment.
Don't advertise click actions for form widgets yet.
app/src/components/form-widget-renderer.tsx:32-36 only accepts connectionId, query, and settings, and its submit path at lines 413-461 never invokes onClick or onChartClick. Keeping supportsClickAction: true will expose a capability that never fires.
Suggested change
capabilities: {
- supportsClickAction: true,
+ supportsClickAction: false,
supportsStyling: false,
isECharts: false,
requiresQuery: false,📝 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
| capabilities: { | |
| supportsClickAction: true, | |
| supportsStyling: false, | |
| isECharts: false, | |
| requiresQuery: false, | |
| }, | |
| capabilities: { | |
| supportsClickAction: false, | |
| supportsStyling: false, | |
| isECharts: false, | |
| requiresQuery: false, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/form.tsx` around lines 37 - 42, The capabilities object in
the plugin incorrectly advertises supportsClickAction: true while the form
widget renderer (app/src/components/form-widget-renderer.tsx) does not call
onClick or onChartClick and only accepts connectionId, query, and settings;
change capabilities.supportsClickAction to false to stop exposing an
unimplemented feature, and if you later implement click handling ensure
form-widget-renderer.tsx invokes onClick/onChartClick and then flip
supportsClickAction back to true.
Comment on lines
+70
to
+74
| onNodeSelect={ | ||
| onChartClick | ||
| ? (ids) => { | ||
| if (ids.length) onChartClick({ nodeId: ids[0] }); | ||
| } |
There was a problem hiding this comment.
Graph preview click payload is too narrow for rule resolution.
Forwarding only nodeId can break rules that rely on other node fields. Pass the selected node payload when available.
💡 Suggested fix
onNodeSelect={
onChartClick
? (ids) => {
- if (ids.length) onChartClick({ nodeId: ids[0] });
+ if (!ids.length) return;
+ const selectedNode = graphData.nodes?.find((n) => n.id === ids[0]);
+ onChartClick(
+ selectedNode
+ ? { ...selectedNode, nodeId: ids[0] }
+ : { nodeId: ids[0] },
+ );
}
: undefined
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/graph.tsx` around lines 70 - 74, The onNodeSelect handler
currently forwards only nodeId to onChartClick (ids[0]) which is too narrow;
update the onNodeSelect implementation so that when ids.length > 0 it looks up
the full node payload for ids[0] (e.g., via the component's nodes prop or a
getNodeById helper) and calls onChartClick with that full node object (falling
back to { nodeId: ids[0] } if the node payload is unavailable) so rules that
depend on other node fields receive the complete payload.
| <div className="h-full overflow-auto"> | ||
| <JsonViewer | ||
| data={data} | ||
| initialExpanded={(settings.initialExpanded as number) ?? 2} |
There was a problem hiding this comment.
Validate initialExpanded at runtime before passing it through.
A type assertion won’t coerce runtime data. Guard to a finite number and fallback to 2.
💡 Suggested fix
function JsonPluginComponent({ data, settings }: PluginComponentProps) {
+ const initialExpanded =
+ typeof settings.initialExpanded === "number" &&
+ Number.isFinite(settings.initialExpanded)
+ ? settings.initialExpanded
+ : 2;
+
return (
<div className="h-full overflow-auto">
<JsonViewer
data={data}
- initialExpanded={(settings.initialExpanded as number) ?? 2}
+ initialExpanded={initialExpanded}
/>
</div>
);
}📝 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
| initialExpanded={(settings.initialExpanded as number) ?? 2} | |
| function JsonPluginComponent({ data, settings }: PluginComponentProps) { | |
| const initialExpanded = | |
| typeof settings.initialExpanded === "number" && | |
| Number.isFinite(settings.initialExpanded) | |
| ? settings.initialExpanded | |
| : 2; | |
| return ( | |
| <div className="h-full overflow-auto"> | |
| <JsonViewer | |
| data={data} | |
| initialExpanded={initialExpanded} | |
| /> | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/json.tsx` at line 22, The runtime value for
settings.initialExpanded must be validated before passing to the component: in
app/src/plugins/json.tsx read settings.initialExpanded, coerce to a Number
(e.g., via Number(...) or parseInt), verify it is a finite integer
(Number.isFinite and Math.floor/Number.isInteger and enforce a sensible min like
0), and if validation fails fallback to 2; then pass that validated value
instead of the raw (settings.initialExpanded as number) into initialExpanded.
Comment on lines
+50
to
+58
| onMarkerClick={ | ||
| onChartClick | ||
| ? (m) => | ||
| onChartClick({ | ||
| id: m.id, | ||
| label: m.label, | ||
| lat: m.lat, | ||
| lng: m.lng, | ||
| }) |
There was a problem hiding this comment.
Preserve full marker payload for click actions.
Only forwarding id/label/lat/lng drops other marker fields, which can break sourceField-based rule resolution. Forward the whole marker payload.
💡 Suggested fix
onMarkerClick={
onChartClick
? (m) =>
onChartClick({
+ ...m,
id: m.id,
label: m.label,
lat: m.lat,
lng: m.lng,
})
: undefined
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/map.tsx` around lines 50 - 58, The onMarkerClick handler
currently maps marker "m" to a reduced object with only id/label/lat/lng before
calling onChartClick, which strips other fields used by sourceField-based rules;
instead pass the full marker payload to onChartClick (i.e., call onChartClick(m)
or spread the entire m object) so all marker properties are preserved; update
the onMarkerClick invocation where onChartClick is referenced to forward the
whole marker object (variable m) rather than a partial subset.
Comment on lines
+9
to
+10
| import type { StylingRule, ColorScaleConfig } from "@neoboard/components"; | ||
| import { TableRenderer } from "@/components/table-renderer"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check current state of the table.tsx file
cd app/src/plugins && head -20 table.tsxRepository: alfredo1996/neoboard
Length of output: 760
🏁 Script executed:
# Find and read the TableRenderer component to understand what it is
fd -e tsx -e ts "table-renderer" app/src/components/ && cat app/src/components/table-renderer.tsx | head -50Repository: alfredo1996/neoboard
Length of output: 1776
🏁 Script executed:
# Verify the exact path and check if TableRenderer is defined in multiple places
find app/src -name "*table-renderer*" -type fRepository: alfredo1996/neoboard
Length of output: 101
🏁 Script executed:
# Check how other plugins import their rendering components
fd -type f "app/src/plugins/*.tsx" | head -5Repository: alfredo1996/neoboard
Length of output: 234
🏁 Script executed:
# Look at a few other plugins to understand the import pattern
rg "import.*from.*components" app/src/plugins/ -A 2Repository: alfredo1996/neoboard
Length of output: 4127
🏁 Script executed:
# Check if TableRenderer has "ssr" concerns (i.e., does it use browser APIs, DOM, etc.)
rg "useEffect|useRef|useState|window|document|DOM" app/src/components/table-renderer.tsx | head -10Repository: alfredo1996/neoboard
Length of output: 283
🏁 Script executed:
# Check how other custom component renderer imports (parameter-widget-renderer, form-widget-renderer, etc.) are used in their plugins
rg "parameter-widget-renderer|form-widget-renderer|graph-exploration-wrapper" app/src/plugins/ -B 2 -A 5Repository: alfredo1996/neoboard
Length of output: 2414
🏁 Script executed:
# Check if those components use next/dynamic
rg "next/dynamic" app/src/plugins/Repository: alfredo1996/neoboard
Length of output: 823
🏁 Script executed:
# Let's see the full context of how table.tsx uses TableRenderer in the JSX
sed -n '30,60p' app/src/plugins/table.tsxRepository: alfredo1996/neoboard
Length of output: 856
🏁 Script executed:
# Check if parameter-select and form plugins also need next/dynamic, or if they have special handling
head -20 app/src/plugins/parameter-select.tsxRepository: alfredo1996/neoboard
Length of output: 780
🏁 Script executed:
# Check if form.tsx has next/dynamic
head -20 app/src/plugins/form.tsxRepository: alfredo1996/neoboard
Length of output: 545
🏁 Script executed:
# Let's verify the exact guideline scope: does it apply to all chart components or only chart visualizations?
grep -r "chart components must use" app/src/ 2>/dev/null | head -5Repository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
# Check CLAUDE.md for clarification on what "chart components" means
cat CLAUDE.md | grep -A 2 -B 2 "chart.*dynamic\|dynamic.*ssr"Repository: alfredo1996/neoboard
Length of output: 359
🏁 Script executed:
# Confirm TableRenderer is a chart/visualization component by checking plugin classification
rg "defineChartPlugin" app/src/plugins/table.tsx -A 3Repository: alfredo1996/neoboard
Length of output: 324
Use next/dynamic with ssr: false for TableRenderer.
TableRenderer is a chart visualization component and must follow the next/dynamic pattern with ssr: false like all other chart plugins in this codebase (bar, pie, treemap, radar, gauge, line, sunburst, sankey, map, graph, single-value). Per CLAUDE.md: "Chart components MUST use next/dynamic with ssr: false. No exceptions."
Suggested change
+import dynamic from "next/dynamic";
import type { StylingRule, ColorScaleConfig } from "@neoboard/components";
-import { TableRenderer } from "@/components/table-renderer";
+const TableRenderer = dynamic(
+ () =>
+ import("@/components/table-renderer").then((m) => ({
+ default: m.TableRenderer,
+ })),
+ { ssr: false },
+);📝 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
| import type { StylingRule, ColorScaleConfig } from "@neoboard/components"; | |
| import { TableRenderer } from "@/components/table-renderer"; | |
| import dynamic from "next/dynamic"; | |
| import type { StylingRule, ColorScaleConfig } from "@neoboard/components"; | |
| const TableRenderer = dynamic( | |
| () => | |
| import("@/components/table-renderer").then((m) => ({ | |
| default: m.TableRenderer, | |
| })), | |
| { ssr: false }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/table.tsx` around lines 9 - 10, The TableRenderer component
is imported statically but must be loaded with next/dynamic and ssr: false like
other chart plugins; replace the direct import of TableRenderer with a dynamic
import using next/dynamic (e.g., const TableRenderer = dynamic(() =>
import(".../table-renderer"), { ssr: false })) and update any references to the
existing TableRenderer symbol to use this dynamic variable so the component is
client-only.
Comment on lines
+37
to
+45
| onCellClick={ | ||
| onChartClick | ||
| ? (info) => | ||
| onChartClick({ | ||
| _clickedColumn: info.column, | ||
| _clickedValue: info.value, | ||
| }) | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
Preserve the full row for table click actions.
The plugin contract here is supposed to forward a raw row callback, but this only emits _clickedColumn and _clickedValue. With app/src/components/table-renderer.tsx:42-47 and component/src/components/composed/data-grid.tsx:88,368 only exposing { column, value }, any action that needs another field from the clicked row (id, slug, etc.) will break after this migration. Please thread row or rowIndex through the table stack and merge it into the onChartClick payload here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/table.tsx` around lines 37 - 45, The onCellClick handler
currently only forwards _clickedColumn and _clickedValue to onChartClick; update
it to also pass the full clicked row (or rowIndex) so downstream consumers can
access id/slug/etc. Modify the onCellClick branch that invokes onChartClick to
include info.row (or info.rowIndex) merged into the payload alongside
_clickedColumn and _clickedValue, ensuring the onChartClick signature is
preserved and callers in table-renderer.tsx and composed/data-grid.tsx receive
the row data.
Turbopack (Next.js 16 default) doesn't correctly handle CJS require() for transpiled monorepo packages, causing createConnectionModule to be undefined at runtime. This breaks all database queries. Changes: - app/package.json: `next dev --webpack` instead of `--turbopack` - app/e2e/global-setup.ts: explicit `--webpack` for local E2E dev mode - app/next.config.ts: add @neoboard/connection to transpilePackages, remove it from serverExternalPackages (it's an internal package) - app/src/lib/connection-adapter.ts: clean require() with docs E2E tests now pass locally (table, single-value, JSON, Neo4j bar chart verified). The --webpack flag can be removed once Turbopack fixes transpilePackages for CJS monorepo packages. See: vercel/next.js#85316 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/lib/connection-adapter.ts (1)
21-23: Consider adding anexportsfield toconnection/package.json.These internal
/src/paths work currently becausetranspilePackagesincludes@neoboard/connectionand the package lacks anexportsfield. However, if the exports field is ever added or the internal structure changes, these imports will break. Adding explicit subpath exports provides stability and clarifies the public API surface of the connection package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/connection-adapter.ts` around lines 21 - 23, The code imports internal modules (factory, interfaces, ConnectionModuleConfig) from `@neoboard/connection/src/`..., which will break if that package adds an exports field; update the connection package to declare explicit subpath exports for the needed modules (e.g., export "./adapters/factory", "./generalized/interfaces", "./ConnectionModuleConfig") in its package.json exports map, or change these imports to use public entrypoints provided by `@neoboard/connection`; ensure the symbols referenced here (factory, interfaces, config) are reachable via the declared exports and update the import statements accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/lib/connection-adapter.ts`:
- Around line 21-23: The code imports internal modules (factory, interfaces,
ConnectionModuleConfig) from `@neoboard/connection/src/`..., which will break if
that package adds an exports field; update the connection package to declare
explicit subpath exports for the needed modules (e.g., export
"./adapters/factory", "./generalized/interfaces", "./ConnectionModuleConfig") in
its package.json exports map, or change these imports to use public entrypoints
provided by `@neoboard/connection`; ensure the symbols referenced here (factory,
interfaces, config) are reachable via the declared exports and update the import
statements accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f552c084-6e0a-4b05-9180-4ab42bc5b803
📒 Files selected for processing (4)
app/e2e/global-setup.tsapp/next.config.tsapp/package.jsonapp/src/lib/connection-adapter.ts
✅ Files skipped from review due to trivial changes (1)
- app/package.json
…rops - New: plugins/utils.ts — PluginProps type + useEChartsClick() hook - All 16 plugins use PluginProps (no more per-plugin interfaces) - chart-renderer passes only onChartClick (removed handleEChartsClick) - ECharts plugins wrap onChartClick via useEChartsClick() internally - Removed unused useMemo, EChartsClickEvent imports from renderer Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/plugins/graph.tsx (1)
59-65:⚠️ Potential issue | 🟠 MajorGraph preview click payload is too narrow for rule resolution.
Forwarding only
nodeIdcan break rules that rely on other node fields. Pass the selected node payload when available.💡 Suggested fix
onNodeSelect={ onChartClick ? (ids) => { - if (ids.length) onChartClick({ nodeId: ids[0] }); + if (!ids.length) return; + const selectedNode = graphData.nodes?.find((n) => n.id === ids[0]); + onChartClick( + selectedNode + ? { ...selectedNode, nodeId: ids[0] } + : { nodeId: ids[0] }, + ); } : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/graph.tsx` around lines 59 - 65, The current onNodeSelect handler only forwards nodeId to onChartClick which is too narrow; change the handler (the onNodeSelect block where onChartClick is used) to resolve the full selected node object (e.g., look up the node by ids[0] from the graph data structure in this component such as nodes or nodesMap) and call onChartClick with the entire node payload (e.g., onChartClick({ node: selectedNode })) instead of only { nodeId: ids[0] } so rule resolution has access to all node fields.
🧹 Nitpick comments (1)
app/src/plugins/single-value.tsx (1)
32-41: Value normalization logic is correct but could be simplified.The double type check (lines 33-36 and 39-40) handles the case where
normalizeValuereturns aboolean, which must be stringified forSingleValueChart. The logic is correct, though slightly verbose.Optional simplification
- const raw = data ?? 0; - const val = - typeof raw === "number" || typeof raw === "string" - ? raw - : (normalizeValue(raw) ?? String(raw)); + const raw = data ?? 0; + const normalized = normalizeValue(raw); + const val: string | number = + typeof normalized === "number" || typeof normalized === "string" + ? normalized + : String(normalized ?? raw); return ( <SingleValueChart - value={ - typeof val === "number" || typeof val === "string" ? val : String(val) - } + value={val}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/single-value.tsx` around lines 32 - 41, Simplify the duplicate type checks around raw/val by normalizing once and then only stringifying booleans: compute raw, call normalizeValue(raw) into a single variable (e.g., normalized), set val = typeof raw === "number" || typeof raw === "string" ? raw : (normalized ?? String(raw)), then when passing to SingleValueChart convert only boolean values with String(val) (or compute finalValue = typeof val === "boolean" ? String(val) : val) and use finalValue as the value prop; reference symbols: raw, normalizeValue, val, SingleValueChart.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/plugins/graph.tsx`:
- Around line 59-65: The current onNodeSelect handler only forwards nodeId to
onChartClick which is too narrow; change the handler (the onNodeSelect block
where onChartClick is used) to resolve the full selected node object (e.g., look
up the node by ids[0] from the graph data structure in this component such as
nodes or nodesMap) and call onChartClick with the entire node payload (e.g.,
onChartClick({ node: selectedNode })) instead of only { nodeId: ids[0] } so rule
resolution has access to all node fields.
---
Nitpick comments:
In `@app/src/plugins/single-value.tsx`:
- Around line 32-41: Simplify the duplicate type checks around raw/val by
normalizing once and then only stringifying booleans: compute raw, call
normalizeValue(raw) into a single variable (e.g., normalized), set val = typeof
raw === "number" || typeof raw === "string" ? raw : (normalized ?? String(raw)),
then when passing to SingleValueChart convert only boolean values with
String(val) (or compute finalValue = typeof val === "boolean" ? String(val) :
val) and use finalValue as the value prop; reference symbols: raw,
normalizeValue, val, SingleValueChart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f055459-0d71-4c77-900b-b86fc26c5e5d
📒 Files selected for processing (19)
app/src/components/chart-renderer.tsxapp/src/plugins/bar.tsxapp/src/plugins/form.tsxapp/src/plugins/gauge.tsxapp/src/plugins/graph.tsxapp/src/plugins/iframe.tsxapp/src/plugins/json.tsxapp/src/plugins/line.tsxapp/src/plugins/map.tsxapp/src/plugins/markdown.tsxapp/src/plugins/parameter-select.tsxapp/src/plugins/pie.tsxapp/src/plugins/radar.tsxapp/src/plugins/sankey.tsxapp/src/plugins/single-value.tsxapp/src/plugins/sunburst.tsxapp/src/plugins/table.tsxapp/src/plugins/treemap.tsxapp/src/plugins/utils.ts
✅ Files skipped from review due to trivial changes (1)
- app/src/plugins/form.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- app/src/plugins/json.tsx
- app/src/plugins/iframe.tsx
- app/src/plugins/map.tsx
- app/src/plugins/radar.tsx
- app/src/plugins/sunburst.tsx
- app/src/plugins/parameter-select.tsx
- app/src/plugins/gauge.tsx
- app/src/plugins/treemap.tsx
|
9 tasks
This was referenced Apr 9, 2026
alfredo1996
added a commit
that referenced
this pull request
May 10, 2026
feat(app): migrate all 13 remaining charts to plugin system (#220)
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
Fourth PR in the plugin system epic. Completes the migration — all 17 chart types now use the plugin registry. The switch statement in chart-renderer.tsx is entirely gone.
Charts migrated (13 new plugins)
single-value, graph, map, table, json, parameter-select, form, iframe, gauge, sankey, sunburst, radar, treemap
chart-renderer.tsx
Plugin adapter contract
Each plugin receives:
```ts
{ data, settings, stylingRules, paramValues, colorScales, onClick,
onChartClick, connectionId, widgetId, resultId, query, autoFit,
clickableColumns, colorThresholds }
```
Plugins pick what they need. ECharts plugins use `onClick` (wrapped event), while map/table/graph/form use `onChartClick` (raw row callback).
Special handling
Test plan
Related: #220, epic #221
🤖 Generated with Claude Code
Summary by CodeRabbit