refactor(plugins): plugin system overhaul — phases 1-4 + 6 integrated - #428
Merged
Conversation
Phase 2: chart-registry.ts is now a thin shim that registers lightweight plugin entries with pluginRegistry and delegates all lookups via a Proxy. The static chartRegistry object, getChartConfig, and all helper functions continue to work unchanged for consumers. Phase 3: ChartType union is now derived from a single CHART_TYPES constant in plugins/chart-types.ts. Startup validation in plugins/index.ts warns if any declared type lacks a registered plugin. All 1971+ existing tests pass unchanged. New tests verify delegation behavior and CHART_TYPES/plugin registry alignment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 2: chart-registry.ts is now a thin shim that registers lightweight plugin entries with pluginRegistry and delegates all lookups via a Proxy. The static chartRegistry object, getChartConfig, and all helper functions continue to work unchanged for consumers. Phase 3: ChartType union is now derived from a single CHART_TYPES constant in plugins/chart-types.ts. Startup validation in plugins/index.ts warns if any declared type lacks a registered plugin. All 1971+ existing tests pass unchanged. New tests verify delegation behavior and CHART_TYPES/plugin registry alignment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Added settingsSchema field to ChartPluginConfig interface - Created settings/ directory with Zod schemas for all 17 chart types - Updated all plugin components to parse settings via schema (no more `as` casts) - 75 new tests covering defaults, validation, passthrough, and coercion Closes #420 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… refactor/plugin-system-integration
alfredo1996
added this to the
v1.1 — Post-Launch: Chart Excellence & Community milestone
alfredo1996
added
enhancement
This was referenced Apr 7, 2026
…(Phase 5) Each of the 17 plugins now bundles its chart options via getChartOptions() from @neoboard/components, replacing scattered lookups. Adds deprecation comment to the component package's chart-options index. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… (Phase 7) Replace all imports from @/lib/chart-registry with @/lib/chart-helpers. The new module delegates to pluginRegistry and includes lightweight plugin registration for test environments. - Create app/src/lib/chart-helpers.ts with helper functions - Create app/src/lib/__tests__/chart-helpers.test.ts with 21 tests - Migrate 15 consumer files from chart-registry to chart-helpers - Delete chart-registry.ts and its 3 test files - Update test mocks to include getChartOptions for plugin imports 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 5 minutes and 34 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 (4)
WalkthroughReplaces the legacy static chart registry with a plugin-backed helper layer, adds per-plugin Zod settings schemas and option metadata, updates many consumers to use Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Plugin Component
participant Schema as Settings Schema (Zod)
participant Helper as Chart Helpers
participant Registry as Plugin Registry
Component->>Component: receive settings (raw)
Component->>Schema: schema.parse(raw)
Schema-->>Component: parsed & coerced settings
Component->>Helper: getChartOptions(type)
Helper->>Registry: pluginRegistry.get(type)
Registry-->>Helper: plugin (options, settingsSchema, capabilities)
Helper-->>Component: options array
Component->>Helper: chartSupportsClickAction(type) / chartSupportsStyling(type) / supportsColumnMapping(type)
Helper->>Registry: pluginRegistry.get(type)
Registry-->>Helper: plugin.capabilities
Helper-->>Component: capability booleans
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/plugins/graph.tsx (1)
36-49:⚠️ Potential issue | 🟠 MajorMove
graphSettingsSchema.parse()to only where it's used—after theconnectionIdbranch.Line 36 parses settings unconditionally, but
GraphExplorationWrapperreceivesrawdirectly (line 48) and doesn't use the parsed result. OnlyGraphChartusessettings.layoutandsettings.showLabels(lines 59–60). If parsing fails on legacy settings, it unnecessarily blocks the exploration path.Proposed fix
function GraphPluginComponent({ data, settings: raw, @@ }: PluginProps) { - const settings = graphSettingsSchema.parse(raw); const graphData = (data ?? { nodes: [], edges: [] }) as { nodes: GraphNode[]; edges: GraphEdge[]; }; if (connectionId) { @@ /> ); } + const settings = graphSettingsSchema.parse(raw); return ( <GraphChart nodes={graphData.nodes ?? []}🤖 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 36 - 49, Move the call to graphSettingsSchema.parse(raw) out of the unconditional top-level code and only parse when rendering the GraphChart path; keep passing raw directly to GraphExplorationWrapper without parsing. Specifically, remove or defer graphSettingsSchema.parse(raw) near the top, and instead call graphSettingsSchema.parse(raw) right before rendering GraphChart (the branch that reads settings.layout and settings.showLabels), referencing the existing symbols graphSettingsSchema.parse, GraphExplorationWrapper, GraphChart, settings, and raw so legacy/invalid raw settings no longer block the connectionId exploration path.
🧹 Nitpick comments (9)
app/src/plugins/settings/parameter-select.ts (1)
14-20: Add range consistency validation (max >= min,step > 0).
The schema currently permits invalid range configs that can break parameter rendering logic.Suggested refine
export const parameterSelectSettingsSchema = z .object({ @@ rangeMin: z.coerce.number().default(0), rangeMax: z.coerce.number().default(100), - rangeStep: z.coerce.number().default(1), + rangeStep: z.coerce.number().positive().default(1), @@ }) + .refine((v) => v.rangeMax >= v.rangeMin, { + message: "rangeMax must be greater than or equal to rangeMin", + path: ["rangeMax"], + }) .passthrough();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/settings/parameter-select.ts` around lines 14 - 20, Add validation to the parameter schema to enforce range consistency: ensure rangeMax >= rangeMin and rangeStep > 0. Update the Zod schema that defines rangeMin, rangeMax, and rangeStep (the object using z.coerce.number().default(...)) to include a .refine() or .superRefine() on the schema to check these conditions and return descriptive errors for the fields (e.g., referencing rangeMin/rangeMax/rangeStep) so invalid configs are rejected at validation time.app/src/plugins/settings/line.ts (1)
15-15: ConstrainlineWidthto valid finite positive values.
Current coercion accepts values that can break rendering semantics (e.g., negative or non-finite). Add bounds.Suggested constraint
- lineWidth: z.coerce.number().default(2), + lineWidth: z.coerce.number().finite().positive().default(2),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/settings/line.ts` at line 15, The current schema lineWidth: z.coerce.number().default(2) allows negative, zero, or non-finite values; update the schema to enforce finite, positive bounds (e.g., replace with z.coerce.number().finite().positive().default(2) or z.coerce.number().finite().min( Number.EPSILON ).default(2)) so only valid positive finite widths are accepted while keeping the default of 2; change the expression containing lineWidth accordingly.app/src/plugins/settings/iframe.ts (1)
8-12: Tighten iframe setting validation for URL and sandbox.
z.string().optional()allows malformed URLs and arbitrary sandbox tokens. Consider validating URL format and constraining sandbox tokens to known values to prevent invalid/surprising runtime behavior.Suggested schema hardening
export const iframeSettingsSchema = z .object({ - url: z.string().optional(), + url: z.string().url().optional(), iframeTitle: z.string().optional(), - sandbox: z.string().optional(), + sandbox: z + .string() + .regex( + /^(allow-forms|allow-modals|allow-orientation-lock|allow-pointer-lock|allow-popups|allow-popups-to-escape-sandbox|allow-presentation|allow-same-origin|allow-scripts|allow-top-navigation|allow-top-navigation-by-user-activation)(\s+(allow-forms|allow-modals|allow-orientation-lock|allow-pointer-lock|allow-popups|allow-popups-to-escape-sandbox|allow-presentation|allow-same-origin|allow-scripts|allow-top-navigation|allow-top-navigation-by-user-activation))*$/, + ) + .optional(), }) .passthrough();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/settings/iframe.ts` around lines 8 - 12, The current schema leaves url and sandbox too loose: replace the url field's z.string().optional() with z.string().url().optional() (or z.string().refine(...) if you need custom checks) to enforce valid URL format, and restrict sandbox by replacing z.string().optional() with either z.enum([...]).optional() for single known tokens or z.string().optional().refine(val => val.split(/\s+/).every(t => allowedSandboxTokens.has(t))) where allowedSandboxTokens is the set of standard iframe sandbox tokens (e.g., allow-forms, allow-modals, allow-pointer-lock, allow-popups, allow-popups-to-escape-sandbox, allow-presentation, allow-same-origin, allow-scripts, allow-storage-access-by-user-activation); keep iframeTitle as-is or add z.string().min(1).optional() if empty titles should be rejected. Ensure you update the schema that defines these fields (the entries named url, iframeTitle, sandbox) using zod validators mentioned above.app/src/components/widget-editor-modal.tsx (1)
404-408: Type cast may be unnecessary.If
getAllChartTypes()already returnsChartType[](orreadonly ChartType[]), the castas ChartType[]is redundant. If it returnsstring[], consider updatinggetAllChartTypesreturn type instead of casting at call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor-modal.tsx` around lines 404 - 408, The code is using a redundant type cast on getAllChartTypes() in the selected chart types memo block; either remove the unnecessary "as ChartType[]" cast where getAllChartTypes() is used in widget-editor-modal (alongside getCompatibleChartTypes and selectedConnection), or update the getAllChartTypes() function signature to return ChartType[] (or readonly ChartType[]) so callers don't need casts—pick one: if the function already returns ChartType[], remove the cast; if it returns string[], change its return type to ChartType[] and adjust its implementation accordingly.app/src/components/__tests__/card-container.test.tsx (1)
256-268: Stale comment — test doesn't exercise form widget.The comment on line 258 says "Need to add 'form' to the mock chart-helpers" but the test uses
chartType: "bar", which is already mocked. Either update the comment to match the test intent or change the test to actually exercise the form path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/__tests__/card-container.test.tsx` around lines 256 - 268, The inline comment in the test "renders chart for form widgets without querying" is stale: it suggests adding "form" to the mock chart-helpers but the test creates a widget via createWidget with chartType: "bar". Update the test to be consistent by either (A) changing the comment to reflect that this test covers a bar chart path, or (B) if you intend to exercise the form widget path, change the widget creation in this test to use chartType: "form" (and adjust any mock/setup for chart-helpers accordingly) so the CardContainer render with widget={widget} and previewData triggers the form-specific code path.app/src/plugins/index.ts (1)
65-75: Consider bidirectional validation.The current check warns when
CHART_TYPESentries lack a plugin, but doesn't catch plugins registered without a correspondingCHART_TYPESentry. If the intent is strict synchronization, consider also checking the inverse:for (const t of pluginRegistry.getTypes()) { if (!CHART_TYPES.includes(t as typeof CHART_TYPES[number])) { console.warn(`Plugin "${t}" registered but not in CHART_TYPES`); } }This would catch orphaned plugins. If external/dynamic plugins are expected, the current one-way check is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/index.ts` around lines 65 - 75, Add a reverse validation to ensure no plugin is registered without a corresponding CHART_TYPES entry: after computing registeredTypes (from pluginRegistry.getTypes()) iterate over pluginRegistry.getTypes() and warn when a type is not included in CHART_TYPES (use CHART_TYPES.includes to check). Keep the original one-way check and add this inverse loop to catch orphaned plugins (log with a clear message like `Plugin "${t}" registered but not in CHART_TYPES"`).app/src/plugins/bar.tsx (1)
30-30: Chart error boundary already handles parse failures gracefully.The error boundary at
ChartRenderercatches render errors and displays a fallback UI instead of crashing the dashboard. WhilebarSettingsSchema.parse(raw)can still throw on invalid settings, usingsafeParsewith defaults would be a better UX — allowing the widget to render with safe defaults instead of showing an error state.Apply this refactor across all plugin render paths for consistency:
Suggested improvement
- const settings = barSettingsSchema.parse(raw); + const parsed = barSettingsSchema.safeParse(raw); + const settings = parsed.success ? parsed.data : barSettingsSchema.parse({});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/bar.tsx` at line 30, Replace the direct parsing call so invalid settings don't throw: instead of barSettingsSchema.parse(raw) use barSettingsSchema.safeParse(raw) and, when safeParse returns success === false, fall back to a defaults object (or merged defaults) so the widget renders with safe defaults; update the render path that calls barSettingsSchema.parse(raw) and mirror the same safeParse+defaults pattern across other plugin renderers (e.g., any code invoked by ChartRenderer) so parsing failures are handled gracefully rather than letting parse throw.app/src/lib/chart-helpers.ts (1)
27-31: Consider deriving column mapping support from plugin capabilities.The hardcoded
COLUMN_MAPPING_TYPESSet requires manual updates when adding charts that support column mapping. Consider adding asupportsColumnMappingcapability flag to plugin definitions in a future phase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/chart-helpers.ts` around lines 27 - 31, COLUMN_MAPPING_TYPES is currently a hardcoded Set limiting which charts support column mapping; instead, update chart-helpers.ts to derive supported chart types from the plugin registry by checking a new supportsColumnMapping flag on each plugin definition (e.g., plugins.map(p => p.type) where p.supportsColumnMapping === true) and build COLUMN_MAPPING_TYPES from that dynamic list; modify any consumers of COLUMN_MAPPING_TYPES to import the built set or a helper function like getColumnMappingTypes() so future plugins simply declare supportsColumnMapping without changing this module.app/src/plugins/settings/__tests__/settings-schemas.test.ts (1)
414-446: Consider adding explicit field validation for form and table schemas.Both
formSettingsSchemaandtableSettingsSchematests rely entirely on passthrough behavior. If these schemas are intentionally minimal, this is fine. However, if specific fields likefields,submitLabel,pageSize, orshowRowNumbersshould be typed/validated, consider adding schema-level definitions and corresponding tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/settings/__tests__/settings-schemas.test.ts` around lines 414 - 446, Tests for formSettingsSchema and tableSettingsSchema only verify passthrough behavior; if fields like fields, submitLabel, pageSize, or showRowNumbers should be validated, update the schemas (formSettingsSchema and tableSettingsSchema) to define those properties (e.g., fields as array of objects with name/type, submitLabel as string, pageSize as number, showRowNumbers as boolean) instead of passthrough, and add corresponding unit tests that assert valid inputs are accepted and invalid values are rejected (e.g., missing required keys, wrong types) to ensure schema-level validation covers these fields.
🤖 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/lib/__tests__/chart-helpers.test.ts`:
- Around line 5-34: Tests fail because transitive plugin imports use
next/dynamic (and conventionally next/navigation) but those modules aren't
mocked; add mocks alongside the existing vi.mock calls in chart-helpers.test.ts.
Mock "next/dynamic" to return a passthrough/default that returns the provided
component (e.g., vi.mock("next/dynamic", () => ({ default: (fn) => fn }))) and
mock "next/navigation" with the minimal exports your tests might expect (e.g.,
stubbed hooks like useRouter/useParams or empty functions) using
vi.mock("next/navigation", () => (/* stubs */)); place these mocks near the
other vi.mock(...) calls so the plugin imports in "@/plugins/index" don't error.
In `@app/src/lib/chart-plugin-registry.ts`:
- Around line 102-103: The settingsSchema property is currently typed as
z.ZodType without generics, causing implicit any; update the declaration for
settingsSchema to include explicit generics (for example z.ZodType<unknown,
z.ZodTypeDef, unknown>) so TypeScript no longer infers any and the plugin
settings have a precise, non-implicit type; adjust the settingsSchema
declaration (symbol: settingsSchema) in the chart-plugin-registry to use the
explicit zod generics.
In `@app/src/plugins/settings/json.ts`:
- Line 8: Constrain the initialExpanded schema to non-negative integers by
changing the validator for initialExpanded (used in
app/src/plugins/settings/json.ts) to coerce to a number then enforce integer and
non-negative constraints (e.g., use .int()/.safe() or .min(0) as appropriate)
and keep the default 2; update the schema entry for initialExpanded to use the
strengthened chain so fractional or negative values are rejected at parse time.
In `@app/src/plugins/settings/pie.ts`:
- Line 15: The topN schema currently coerces any number (including negatives and
decimals); update the validator for the topN property to coerce to a number,
enforce integer values and allow zero by using .int().min(0) while keeping
.optional(), i.e., replace the current z.coerce.number().optional() for topN
with a coercion that calls .int().min(0) so decimals and negatives are rejected
but 0 is accepted as "show all".
In `@app/src/plugins/single-value.tsx`:
- Line 36: Replace the brittle call to singleValueSettingsSchema.parse(raw) with
singleValueSettingsSchema.safeParse(raw) and, if safeParse returns success:
false, assign a predefined fallback defaults object (e.g.,
singleValueDefaultSettings) to settings; otherwise use the parsed data. Update
the code around the settings variable so it uses safeParse(raw).data when
success is true and singleValueDefaultSettings (or a minimal default literal
matching the schema) when success is false to avoid throwing on legacy/invalid
enum values.
---
Outside diff comments:
In `@app/src/plugins/graph.tsx`:
- Around line 36-49: Move the call to graphSettingsSchema.parse(raw) out of the
unconditional top-level code and only parse when rendering the GraphChart path;
keep passing raw directly to GraphExplorationWrapper without parsing.
Specifically, remove or defer graphSettingsSchema.parse(raw) near the top, and
instead call graphSettingsSchema.parse(raw) right before rendering GraphChart
(the branch that reads settings.layout and settings.showLabels), referencing the
existing symbols graphSettingsSchema.parse, GraphExplorationWrapper, GraphChart,
settings, and raw so legacy/invalid raw settings no longer block the
connectionId exploration path.
---
Nitpick comments:
In `@app/src/components/__tests__/card-container.test.tsx`:
- Around line 256-268: The inline comment in the test "renders chart for form
widgets without querying" is stale: it suggests adding "form" to the mock
chart-helpers but the test creates a widget via createWidget with chartType:
"bar". Update the test to be consistent by either (A) changing the comment to
reflect that this test covers a bar chart path, or (B) if you intend to exercise
the form widget path, change the widget creation in this test to use chartType:
"form" (and adjust any mock/setup for chart-helpers accordingly) so the
CardContainer render with widget={widget} and previewData triggers the
form-specific code path.
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 404-408: The code is using a redundant type cast on
getAllChartTypes() in the selected chart types memo block; either remove the
unnecessary "as ChartType[]" cast where getAllChartTypes() is used in
widget-editor-modal (alongside getCompatibleChartTypes and selectedConnection),
or update the getAllChartTypes() function signature to return ChartType[] (or
readonly ChartType[]) so callers don't need casts—pick one: if the function
already returns ChartType[], remove the cast; if it returns string[], change its
return type to ChartType[] and adjust its implementation accordingly.
In `@app/src/lib/chart-helpers.ts`:
- Around line 27-31: COLUMN_MAPPING_TYPES is currently a hardcoded Set limiting
which charts support column mapping; instead, update chart-helpers.ts to derive
supported chart types from the plugin registry by checking a new
supportsColumnMapping flag on each plugin definition (e.g., plugins.map(p =>
p.type) where p.supportsColumnMapping === true) and build COLUMN_MAPPING_TYPES
from that dynamic list; modify any consumers of COLUMN_MAPPING_TYPES to import
the built set or a helper function like getColumnMappingTypes() so future
plugins simply declare supportsColumnMapping without changing this module.
In `@app/src/plugins/bar.tsx`:
- Line 30: Replace the direct parsing call so invalid settings don't throw:
instead of barSettingsSchema.parse(raw) use barSettingsSchema.safeParse(raw)
and, when safeParse returns success === false, fall back to a defaults object
(or merged defaults) so the widget renders with safe defaults; update the render
path that calls barSettingsSchema.parse(raw) and mirror the same
safeParse+defaults pattern across other plugin renderers (e.g., any code invoked
by ChartRenderer) so parsing failures are handled gracefully rather than letting
parse throw.
In `@app/src/plugins/index.ts`:
- Around line 65-75: Add a reverse validation to ensure no plugin is registered
without a corresponding CHART_TYPES entry: after computing registeredTypes (from
pluginRegistry.getTypes()) iterate over pluginRegistry.getTypes() and warn when
a type is not included in CHART_TYPES (use CHART_TYPES.includes to check). Keep
the original one-way check and add this inverse loop to catch orphaned plugins
(log with a clear message like `Plugin "${t}" registered but not in
CHART_TYPES"`).
In `@app/src/plugins/settings/__tests__/settings-schemas.test.ts`:
- Around line 414-446: Tests for formSettingsSchema and tableSettingsSchema only
verify passthrough behavior; if fields like fields, submitLabel, pageSize, or
showRowNumbers should be validated, update the schemas (formSettingsSchema and
tableSettingsSchema) to define those properties (e.g., fields as array of
objects with name/type, submitLabel as string, pageSize as number,
showRowNumbers as boolean) instead of passthrough, and add corresponding unit
tests that assert valid inputs are accepted and invalid values are rejected
(e.g., missing required keys, wrong types) to ensure schema-level validation
covers these fields.
In `@app/src/plugins/settings/iframe.ts`:
- Around line 8-12: The current schema leaves url and sandbox too loose: replace
the url field's z.string().optional() with z.string().url().optional() (or
z.string().refine(...) if you need custom checks) to enforce valid URL format,
and restrict sandbox by replacing z.string().optional() with either
z.enum([...]).optional() for single known tokens or
z.string().optional().refine(val => val.split(/\s+/).every(t =>
allowedSandboxTokens.has(t))) where allowedSandboxTokens is the set of standard
iframe sandbox tokens (e.g., allow-forms, allow-modals, allow-pointer-lock,
allow-popups, allow-popups-to-escape-sandbox, allow-presentation,
allow-same-origin, allow-scripts, allow-storage-access-by-user-activation); keep
iframeTitle as-is or add z.string().min(1).optional() if empty titles should be
rejected. Ensure you update the schema that defines these fields (the entries
named url, iframeTitle, sandbox) using zod validators mentioned above.
In `@app/src/plugins/settings/line.ts`:
- Line 15: The current schema lineWidth: z.coerce.number().default(2) allows
negative, zero, or non-finite values; update the schema to enforce finite,
positive bounds (e.g., replace with
z.coerce.number().finite().positive().default(2) or
z.coerce.number().finite().min( Number.EPSILON ).default(2)) so only valid
positive finite widths are accepted while keeping the default of 2; change the
expression containing lineWidth accordingly.
In `@app/src/plugins/settings/parameter-select.ts`:
- Around line 14-20: Add validation to the parameter schema to enforce range
consistency: ensure rangeMax >= rangeMin and rangeStep > 0. Update the Zod
schema that defines rangeMin, rangeMax, and rangeStep (the object using
z.coerce.number().default(...)) to include a .refine() or .superRefine() on the
schema to check these conditions and return descriptive errors for the fields
(e.g., referencing rangeMin/rangeMax/rangeStep) so invalid configs are rejected
at validation time.
🪄 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: 85eff30e-5f65-4226-9e59-38f2b71219be
📒 Files selected for processing (70)
app/src/app/(dashboard)/widget-lab/page.tsxapp/src/components/__tests__/card-container-states.test.tsxapp/src/components/__tests__/card-container.test.tsxapp/src/components/__tests__/chart-error-boundary.test.tsxapp/src/components/card-container.tsxapp/src/components/chart-renderer.tsxapp/src/components/graph-exploration-wrapper.tsxapp/src/components/save-template-dialog.tsxapp/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/__tests__/query-editor-panel.test.tsxapp/src/components/widget-editor/chart-type-selector.tsxapp/src/components/widget-editor/query-editor-panel.tsxapp/src/components/widget-editor/styling-rules-editor.tsxapp/src/lib/__tests__/chart-helpers.test.tsapp/src/lib/__tests__/chart-registry-mapping.test.tsapp/src/lib/__tests__/chart-registry.test.tsapp/src/lib/__tests__/widget-utils.test.tsapp/src/lib/capture-preview.tsapp/src/lib/chart-helpers.tsapp/src/lib/chart-plugin-registry.tsapp/src/lib/chart-registry.tsapp/src/lib/query-templates.tsapp/src/lib/widget-actions.tsapp/src/lib/widget-utils.tsapp/src/plugins/__tests__/bar.test.tsxapp/src/plugins/__tests__/chart-types.test.tsapp/src/plugins/__tests__/markdown.test.tsxapp/src/plugins/__tests__/plugin-options.test.tsapp/src/plugins/__tests__/registry.test.tsapp/src/plugins/bar.tsxapp/src/plugins/chart-types.tsapp/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/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/settings/__tests__/settings-schemas.test.tsapp/src/plugins/settings/bar.tsapp/src/plugins/settings/form.tsapp/src/plugins/settings/gauge.tsapp/src/plugins/settings/graph.tsapp/src/plugins/settings/iframe.tsapp/src/plugins/settings/index.tsapp/src/plugins/settings/json.tsapp/src/plugins/settings/line.tsapp/src/plugins/settings/map.tsapp/src/plugins/settings/markdown.tsapp/src/plugins/settings/parameter-select.tsapp/src/plugins/settings/pie.tsapp/src/plugins/settings/radar.tsapp/src/plugins/settings/sankey.tsapp/src/plugins/settings/single-value.tsapp/src/plugins/settings/sunburst.tsapp/src/plugins/settings/table.tsapp/src/plugins/settings/treemap.tsapp/src/plugins/single-value.tsxapp/src/plugins/sunburst.tsxapp/src/plugins/table.tsxapp/src/plugins/transforms/shared.tsapp/src/plugins/treemap.tsxapp/src/stores/widget-editor-store.tscomponent/src/components/composed/chart-options/index.ts
💤 Files with no reviewable changes (3)
- app/src/lib/tests/chart-registry.test.ts
- app/src/lib/tests/chart-registry-mapping.test.ts
- app/src/lib/chart-registry.ts
- capture-preview: access isECharts via capabilities object - card-container: fallback to transform when transformWithMapping undefined - chart-renderer: accept string type, remove unused ChartType import - graph plugin: explicit type annotation for onNodeSelect callback Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/card-container.tsx (1)
38-40: Decouple column-mapping capability from hard-coded setLine 40 relies on
chartSupportsColumnMapping(type)backed by a hard-coded set["bar", "line", "pie"]inapp/src/lib/chart-helpers.ts. No plugins currently declaresupportsColumnMapping: true, so no drift exists today. However, the set is not registry-driven—future plugins declaring the capability will silently fail to work unless the set is manually updated. Consider deriving this from plugin registrations to prevent silent capability misses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/card-container.tsx` around lines 38 - 40, The supportsColumnMapping function currently proxies to chartSupportsColumnMapping(type) which uses a hard-coded set; update supportsColumnMapping to consult the plugin registry/manifest for the chart type’s declared capability (e.g., check each registered plugin or pluginRegistry/PluginManifest entry for supportsColumnMapping: true for that chart type) and fall back to chartSupportsColumnMapping(type) if no registration exists, so future plugins that declare supportsColumnMapping are honored without updating the hard-coded list.
🤖 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/components/card-container.tsx`:
- Around line 38-40: The supportsColumnMapping function currently proxies to
chartSupportsColumnMapping(type) which uses a hard-coded set; update
supportsColumnMapping to consult the plugin registry/manifest for the chart
type’s declared capability (e.g., check each registered plugin or
pluginRegistry/PluginManifest entry for supportsColumnMapping: true for that
chart type) and fall back to chartSupportsColumnMapping(type) if no registration
exists, so future plugins that declare supportsColumnMapping are honored without
updating the hard-coded list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a8c3b57-ee31-4ff6-bde7-d807c505f0c8
📒 Files selected for processing (4)
app/src/components/card-container.tsxapp/src/components/chart-renderer.tsxapp/src/lib/capture-preview.tsapp/src/plugins/graph.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/lib/capture-preview.ts
- app/src/components/chart-renderer.tsx
- app/src/plugins/graph.tsx
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- chart-helpers.test.ts: add next/dynamic and next/navigation mocks (Critical) - single-value.tsx: use safeParse with fallback for resilience (Major) - json settings: constrain initialExpanded to non-negative integers (Minor) - pie settings: constrain topN to non-negative integers (Minor) - chart-plugin-registry: tighten settingsSchema type to Record<string, unknown> (Minor) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/lib/chart-plugin-registry.ts (1)
102-103:⚠️ Potential issue | 🟡 MinorAvoid possible implicit
anyinsettingsSchematyping.Line 103 sets only the first
ZodTypegeneric. In some Zod versions, omitted generics can still fall back toany. Please lock all generics explicitly to preserve strict typing.♻️ Proposed change
- settingsSchema?: z.ZodType<Record<string, unknown>>; + settingsSchema?: z.ZodType< + Record<string, unknown>, + z.ZodTypeDef, + Record<string, unknown> + >;For the exact Zod version used in this repo, what are the default generic parameters of `ZodType`, and does `ZodType<Record<string, unknown>>` leave any generic as `any`?As per coding guidelines, "**/*.{ts,tsx}: TypeScript must be strict. No
anywithout a comment explaining why."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/chart-plugin-registry.ts` around lines 102 - 103, The settingsSchema property currently uses a single ZodType generic which may leave other ZodType generic parameters as implicit any; update the type annotation to explicitly specify all generics (e.g. use z.ZodType<Record<string, unknown>, z.ZodTypeDef, Record<string, unknown>>) so both the output and input types and the Definition type are locked down for settingsSchema (reference symbol: settingsSchema, type: z.ZodType).
🧹 Nitpick comments (4)
app/src/plugins/single-value.tsx (1)
36-40: Cache default settings once instead of parsing on each render.Line 39 runs
singleValueSettingsSchema.parse({})on every render when parse fails. Move this default parse to module scope and reuse it for a cheaper, cleaner fallback path.Proposed refactor
+const defaultSingleValueSettings = singleValueSettingsSchema.parse({}); + function SingleValuePluginComponent({ data, settings: raw, @@ const parsed = singleValueSettingsSchema.safeParse(raw); const settings = parsed.success ? parsed.data - : singleValueSettingsSchema.parse({}); + : defaultSingleValueSettings;🤖 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 36 - 40, The code calls singleValueSettingsSchema.parse({}) on every render as a fallback; create a module-scope constant (e.g., DEFAULT_SINGLE_VALUE_SETTINGS) initialized once by calling singleValueSettingsSchema.parse({}) and then change the fallback in the component to use that constant instead of invoking parse({}) repeatedly (update the settings assignment that currently references parsed/ singleValueSettingsSchema.parse({}) to use DEFAULT_SINGLE_VALUE_SETTINGS).app/src/lib/__tests__/chart-helpers.test.ts (3)
209-216: Type count assertion is good but brittle.Hard-coded
17will fail if chart types are added/removed. The loop checkingCHART_TYPESinclusion is the more valuable assertion.♻️ Alternative: derive count from CHART_TYPES
describe("getAllChartTypes", () => { it("returns all 17 registered types", () => { const types = getAllChartTypes(); - expect(types.length).toBe(17); + expect(types.length).toBe(CHART_TYPES.length); for (const t of CHART_TYPES) { expect(types).toContain(t); } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/__tests__/chart-helpers.test.ts` around lines 209 - 216, The test for getAllChartTypes uses a brittle hard-coded 17; change the count assertion to derive the expected value from CHART_TYPES (e.g., compare types.length to CHART_TYPES.length) and keep the existing loop that ensures every CHART_TYPES entry is present; update the assertion that references the literal 17 to use CHART_TYPES.length so the test won't break when chart types are added or removed.
184-188:getChartDefaultstest coverage is minimal.Only tests
barreturning an empty object. If any chart type has non-trivial defaults in the future, consider adding more cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/__tests__/chart-helpers.test.ts` around lines 184 - 188, The test for getChartDefaults only asserts the "bar" case returns {}, so expand coverage by adding additional assertions for other chart types (e.g., "line", "pie", "scatter" or any types referenced in getChartDefaults) to ensure any non-trivial defaults are validated; update the test in chart-helpers.test.ts to include these cases (preferably table-driven/parameterized tests) and assert the expected default objects returned by getChartDefaults for each chart type.
25-39: Simplify thenext/dynamicmock.The mock works but is more elaborate than necessary. The try/catch and Promise-checking logic always returns
Stubregardless of the outcome. A simpler mock achieves the same:♻️ Simplified mock
vi.mock("next/dynamic", () => ({ - default: (fn: () => Promise<{ default: unknown }>) => { - try { - const mod = fn(); - if ( - mod && - typeof (mod as Promise<{ default: unknown }>).then === "function" - ) - return Stub; - } catch { - /* noop */ - } - return Stub; - }, + default: () => Stub, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/__tests__/chart-helpers.test.ts` around lines 25 - 39, The next/dynamic mock is overly complex and always returns Stub; simplify by replacing the current implementation in the vi.mock("next/dynamic", ...) block so its default export directly returns Stub (remove the try/catch and Promise-checking logic). Locate the mock where default is defined and change it to a minimal stub-returning implementation referencing the existing Stub symbol so tests remain identical but code is clearer.
🤖 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/lib/chart-plugin-registry.ts`:
- Around line 102-103: The settingsSchema property currently uses a single
ZodType generic which may leave other ZodType generic parameters as implicit
any; update the type annotation to explicitly specify all generics (e.g. use
z.ZodType<Record<string, unknown>, z.ZodTypeDef, Record<string, unknown>>) so
both the output and input types and the Definition type are locked down for
settingsSchema (reference symbol: settingsSchema, type: z.ZodType).
---
Nitpick comments:
In `@app/src/lib/__tests__/chart-helpers.test.ts`:
- Around line 209-216: The test for getAllChartTypes uses a brittle hard-coded
17; change the count assertion to derive the expected value from CHART_TYPES
(e.g., compare types.length to CHART_TYPES.length) and keep the existing loop
that ensures every CHART_TYPES entry is present; update the assertion that
references the literal 17 to use CHART_TYPES.length so the test won't break when
chart types are added or removed.
- Around line 184-188: The test for getChartDefaults only asserts the "bar" case
returns {}, so expand coverage by adding additional assertions for other chart
types (e.g., "line", "pie", "scatter" or any types referenced in
getChartDefaults) to ensure any non-trivial defaults are validated; update the
test in chart-helpers.test.ts to include these cases (preferably
table-driven/parameterized tests) and assert the expected default objects
returned by getChartDefaults for each chart type.
- Around line 25-39: The next/dynamic mock is overly complex and always returns
Stub; simplify by replacing the current implementation in the
vi.mock("next/dynamic", ...) block so its default export directly returns Stub
(remove the try/catch and Promise-checking logic). Locate the mock where default
is defined and change it to a minimal stub-returning implementation referencing
the existing Stub symbol so tests remain identical but code is clearer.
In `@app/src/plugins/single-value.tsx`:
- Around line 36-40: The code calls singleValueSettingsSchema.parse({}) on every
render as a fallback; create a module-scope constant (e.g.,
DEFAULT_SINGLE_VALUE_SETTINGS) initialized once by calling
singleValueSettingsSchema.parse({}) and then change the fallback in the
component to use that constant instead of invoking parse({}) repeatedly (update
the settings assignment that currently references parsed/
singleValueSettingsSchema.parse({}) to use DEFAULT_SINGLE_VALUE_SETTINGS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6547b8cc-b587-41f6-9fd9-06c635c074dc
⛔ Files ignored due to path filters (2)
app/package-lock.jsonis excluded by!**/package-lock.jsonapp/tsconfig.tsbuildinfois excluded by!app/tsconfig.tsbuildinfo
📒 Files selected for processing (6)
app/next-env.d.tsapp/src/lib/__tests__/chart-helpers.test.tsapp/src/lib/chart-plugin-registry.tsapp/src/plugins/settings/json.tsapp/src/plugins/settings/pie.tsapp/src/plugins/single-value.tsx
✅ Files skipped from review due to trivial changes (1)
- app/src/plugins/settings/json.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/plugins/settings/pie.ts
…tubs - Pie plugin: restore label "Pie Chart" (was incorrectly "Pie / Doughnut") - plugins/index.ts: unregister stubs before registering real plugins (chart-helpers.ts stubs could prevent real plugins from loading) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- parameter-select settings: add missing types (date-range, date-relative, cascading-select) to parameterType enum — Zod was silently defaulting to "select" which prevented specialized pickers from rendering - widget-utils test: update label assertion to match restored "Pie Chart" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 8, 2026
This was referenced Apr 9, 2026
This was referenced Apr 24, 2026
5 tasks
alfredo1996
added a commit
that referenced
this pull request
May 10, 2026
…ration refactor(plugins): plugin system overhaul — phases 1-4 + 6 integrated
5 tasks
Merged
9 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.
Summary
Integration branch combining all completed plugin system refactor phases into a single PR for release/1.1.
Closes #415, Closes #416, Closes #417, Closes #419, Closes #420
Phases Included
Phase 1: Extract transforms (#415, PR #424 ✅ merged)
app/src/plugins/transforms/Phase 2+3: Delegation shim + ChartType derivation (#417, #419, PR #426)
chart-registry.ts→ Proxy delegating topluginRegistryCHART_TYPESconst array as single source of truthPhase 4: Typed settings with Zod (#420, PR #427)
ascastsPhase 6: Connector registry alignment (#416, PR #425 ✅ merged)
unregister()on ConnectorRegistryConnectorFormFieldinterface +formFieldson pluginsStats
Remaining phases (separate PRs)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes