fix: harden plugin infrastructure — safety and validation - #615
Merged
Conversation
1. External plugin try/catch — broken plugin no longer crashes app 2. Plugin config validation — warns on missing option keys 3. Transform error handling — fallback to raw data on crash 4. Atomic manifest writes — temp file + rename pattern 5. Codegen package resolution — verify packages installed Closes #614 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds runtime validation and non-fatal warnings for plugin configs, hardens external plugin registration, wraps chart/data transforms in render-time try/catch fallbacks, makes manifest writes atomic, and makes codegen scripts verify referenced packages are installed. Changes
Sequence Diagram(s)(Skipped.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🧹 Nitpick comments (5)
app/src/plugins/index.ts (2)
75-105: Minor: destructuring happens outside thetry.
for (const { plugin, overrides } of EXTERNAL_PLUGINS)destructures before entering thetryblock. If any element ofEXTERNAL_PLUGINSisnull/undefined(e.g. due to a botched generator output), the loop aborts and remaining plugins won't load — defeating the point of the per-iteration try/catch. The codegen makes this unlikely, but a tiny shape guard would close the gap.Suggested tweak
-for (const { plugin, overrides } of EXTERNAL_PLUGINS) { - try { +for (const entry of EXTERNAL_PLUGINS) { + try { + if (!entry || typeof entry !== "object") { + console.error("External plugin skipped: invalid entry", entry); + continue; + } + const { plugin, overrides } = entry; if (!plugin || typeof plugin !== "object" || !plugin.type) {🤖 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 75 - 105, The loop currently destructures with "for (const { plugin, overrides } of EXTERNAL_PLUGINS)" before the try, so a null/undefined entry will throw outside the try and abort plugin loading; change to iterate over entries (e.g., "for (const entry of EXTERNAL_PLUGINS)") and inside the try guard the shape (e.g., check entry != null and then const { plugin, overrides } = entry) before using plugin, keeping the existing validations and pluginRegistry.register/unregister logic to ensure each plugin iteration is individually protected by the try/catch.
95-95: Consider validating external plugin shape beforeregister().
pluginRegistry.register(plugin)only stores byplugin.type; it does not re-run thedefineChartPluginfield checks (transform/label/component). A malformed external plugin that survivesimportwill register successfully and crash later insidechart-renderer.tsx. CallingdefineChartPlugin(plugin)here (or a lighter-weight shape check) would surface the issue at startup with the existing try/catch catching it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/index.ts` at line 95, Validate the external plugin shape before registering: call defineChartPlugin(plugin) (or a lightweight shape validation that ensures transform/label/component fields exist) right before pluginRegistry.register(plugin) so malformed plugins are detected at startup and will be caught by the existing try/catch; this change should be applied around the plugin registration logic where pluginRegistry.register(plugin) is invoked to avoid downstream crashes in chart-renderer.tsx.app/src/components/card-container.tsx (2)
305-331: DRY: preview and live transform paths are duplicates — extract a helper.The two blocks are almost identical (mapping + applyTransforms with the same fallback semantics). A small helper avoids drift if either path's error handling evolves.
Sketch
function runChartTransforms( chartConfig: NonNullable<ReturnType<typeof getChartConfig>>, raw: unknown, mapping: ColumnMapping, transforms: Transform[], paramValues: Record<string, unknown>, chartType: string, ): { data: unknown; transformError: Error | null } { let mapped: unknown; let transformError: Error | null = null; try { mapped = (chartConfig.transformWithMapping ?? chartConfig.transform)(raw, mapping); } catch (err) { console.error("Chart transform failed for " + chartType + ":", err); transformError = err instanceof Error ? err : new Error(String(err)); mapped = raw; } if (transforms.length && chartType !== "graph") { try { mapped = applyTransforms(mapped as Record<string, unknown>[], transforms, paramValues); } catch (err) { console.error("Data transform failed:", err); transformError ??= err instanceof Error ? err : new Error(String(err)); } } return { data: mapped, transformError }; }Also applies to: 604-626
🤖 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 305 - 331, Extract the duplicated mapping+transform logic into a helper (e.g., runChartTransforms) that accepts chartConfig, raw data, columnMapping, dataTransforms, allParamValues, and chartType and returns { data: unknown; transformError: Error | null }; inside it call (chartConfig.transformWithMapping ?? chartConfig.transform)(raw, mapping) with try/catch (log "Chart transform failed for " + chartType + ":" and fallback mapped = raw), then if transforms.length and chartType !== "graph" call applyTransforms(mapped as Record<string, unknown>[], transforms, paramValues) with try/catch (log "Data transform failed:" and preserve the first error via transformError), and return the final mapped and transformError; replace both duplicated blocks (the preview path and the live path references to mappedData/transformedData) to call runChartTransforms and use its returned data and transformError.
322-326: CastmappedData as Record<string, unknown>[]is unsafe after fallback.When the chart transform throws,
mappedDatais reassigned topreviewData/rawData(whatever shape that may be —unknown). The subsequentapplyTransforms(mappedData as Record<string, unknown>[], …)then operates on unvalidated input.applyTransformsdoes guard withArray.isArray(data)and returns the input unchanged, so it won't throw — but the cast is still misleading. Consider passing throughArray.isArray(mappedData) ? mappedData : []or letting the helper above narrow once.Also applies to: 617-621
🤖 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 322 - 326, The code unsafely casts mappedData to Record<string, unknown>[] before calling applyTransforms; instead, ensure mappedData is an array or pass an empty array to avoid operating on unknown shapes — replace applyTransforms(mappedData as Record<string, unknown>[], dataTransforms, allParamValues) with applyTransforms(Array.isArray(mappedData) ? mappedData : [], dataTransforms, allParamValues) (and make the same change for the other occurrence where mappedData/fallback data is cast before applyTransforms) so applyTransforms receives a correctly typed array or an empty array.cli/src/lib/manifest.ts (1)
29-46: Atomic on rename, but not crash-safe withoutfsync.
renameSyncis atomic on POSIX, but the temp file's bytes may still be in the page cache when the rename returns. A crash between write and a kernel flush can leave the manifest pointing at zero-length content. For true crash atomicity,fsyncthe temp fd before rename, thenfsyncthe parent directory. If the goal here is just "no torn writes from concurrent CLI processes," the current approach is fine — flag this as out of scope if so.Hardened version (optional)
-import { - readFileSync, - writeFileSync, - renameSync, - existsSync, - unlinkSync, -} from "node:fs"; +import { + readFileSync, + writeFileSync, + renameSync, + existsSync, + unlinkSync, + openSync, + fsyncSync, + closeSync, +} from "node:fs"; @@ try { writeFileSync(tmpPath, JSON.stringify(data, null, 2) + "\n"); + const fd = openSync(tmpPath, "r+"); + try { fsyncSync(fd); } finally { closeSync(fd); } renameSync(tmpPath, filePath); + const dirFd = openSync(dirname(filePath), "r"); + try { fsyncSync(dirFd); } finally { closeSync(dirFd); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/lib/manifest.ts` around lines 29 - 46, The current atomicWriteJson may rename a temp file before its bytes hit disk; to make it crash-safe, replace the writeFileSync step with an explicit open/write/fdatasync/close sequence on the temp file (use functions referenced in atomicWriteJson: openSync, writeSync, fsyncSync or fdatasyncSync, closeSync), then call renameSync(file) and finally fsync the parent directory by opening dirname(filePath) for reading, fsyncing that dir fd, and closing it; ensure you still unlink the temp file and rethrow on errors and keep the existing cleanup/rename logic intact (or leave as-is and mark as out-of-scope if you only care about concurrent-process safety).
🤖 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/components/card-container.tsx`:
- Around line 311-317: The catch that handles chart transform failures currently
only logs and falls back to previewData (mappedData) — instead set a component
state (e.g., call setTransformError(String(err)) or setTransformError(err))
inside that catch (where mappedData = previewData is done) and clear that state
on successful transform (after mappedData is computed); then update the renderer
JSX to show an inline warning banner (reuse the truncation notice
markup/structure used around lines 631–639) when transformError is truthy so the
affected widget displays the error message (include widget.chartType and the
transformError text) instead of silently hiding the problem.
In `@scripts/generate-connector-imports.mjs`:
- Around line 220-238: runGenerator uses await import("node:module") which
causes a syntax error because runGenerator is not async; replace the dynamic
import with a top-level static import of createRequire from "node:module" and
remove the await; inside the loop call createRequire(manifestPath) and then
require.resolve(entry.package) as the single resolution path (you can drop the
import.meta.resolve branch), pushing the same error message into errors when
require.resolve fails.
In `@scripts/generate-plugin-imports.mjs`:
- Around line 220-239: The loop that verifies packages uses a dynamic top-level
await import("node:module") inside runGenerator/entries which causes a
SyntaxError; replace the dynamic await import with a static top-level import of
createRequire (e.g., import { createRequire } from "node:module") and then call
createRequire(manifestPath). Ensure the code in the entries loop uses the
imported createRequire instead of awaiting import, leaving import.meta.resolve
usage as-is and still pushing the same error message when
require.resolve(entry.package) fails.
---
Nitpick comments:
In `@app/src/components/card-container.tsx`:
- Around line 305-331: Extract the duplicated mapping+transform logic into a
helper (e.g., runChartTransforms) that accepts chartConfig, raw data,
columnMapping, dataTransforms, allParamValues, and chartType and returns { data:
unknown; transformError: Error | null }; inside it call
(chartConfig.transformWithMapping ?? chartConfig.transform)(raw, mapping) with
try/catch (log "Chart transform failed for " + chartType + ":" and fallback
mapped = raw), then if transforms.length and chartType !== "graph" call
applyTransforms(mapped as Record<string, unknown>[], transforms, paramValues)
with try/catch (log "Data transform failed:" and preserve the first error via
transformError), and return the final mapped and transformError; replace both
duplicated blocks (the preview path and the live path references to
mappedData/transformedData) to call runChartTransforms and use its returned data
and transformError.
- Around line 322-326: The code unsafely casts mappedData to Record<string,
unknown>[] before calling applyTransforms; instead, ensure mappedData is an
array or pass an empty array to avoid operating on unknown shapes — replace
applyTransforms(mappedData as Record<string, unknown>[], dataTransforms,
allParamValues) with applyTransforms(Array.isArray(mappedData) ? mappedData :
[], dataTransforms, allParamValues) (and make the same change for the other
occurrence where mappedData/fallback data is cast before applyTransforms) so
applyTransforms receives a correctly typed array or an empty array.
In `@app/src/plugins/index.ts`:
- Around line 75-105: The loop currently destructures with "for (const { plugin,
overrides } of EXTERNAL_PLUGINS)" before the try, so a null/undefined entry will
throw outside the try and abort plugin loading; change to iterate over entries
(e.g., "for (const entry of EXTERNAL_PLUGINS)") and inside the try guard the
shape (e.g., check entry != null and then const { plugin, overrides } = entry)
before using plugin, keeping the existing validations and
pluginRegistry.register/unregister logic to ensure each plugin iteration is
individually protected by the try/catch.
- Line 95: Validate the external plugin shape before registering: call
defineChartPlugin(plugin) (or a lightweight shape validation that ensures
transform/label/component fields exist) right before
pluginRegistry.register(plugin) so malformed plugins are detected at startup and
will be caught by the existing try/catch; this change should be applied around
the plugin registration logic where pluginRegistry.register(plugin) is invoked
to avoid downstream crashes in chart-renderer.tsx.
In `@cli/src/lib/manifest.ts`:
- Around line 29-46: The current atomicWriteJson may rename a temp file before
its bytes hit disk; to make it crash-safe, replace the writeFileSync step with
an explicit open/write/fdatasync/close sequence on the temp file (use functions
referenced in atomicWriteJson: openSync, writeSync, fsyncSync or fdatasyncSync,
closeSync), then call renameSync(file) and finally fsync the parent directory by
opening dirname(filePath) for reading, fsyncing that dir fd, and closing it;
ensure you still unlink the temp file and rethrow on errors and keep the
existing cleanup/rename logic intact (or leave as-is and mark as out-of-scope if
you only care about concurrent-process safety).
🪄 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: b15e07fc-e784-49a4-947c-f03739b638e5
📒 Files selected for processing (6)
app/src/components/card-container.tsxapp/src/lib/plugin/chart-plugin-registry.tsapp/src/plugins/index.tscli/src/lib/manifest.tsscripts/generate-connector-imports.mjsscripts/generate-plugin-imports.mjs
Comment on lines
+311
to
+317
| } catch (err) { | ||
| console.error( | ||
| "Chart transform failed for " + widget.chartType + ":", | ||
| err, | ||
| ); | ||
| mappedData = previewData; | ||
| } |
There was a problem hiding this comment.
Errors aren't surfaced in the widget UI — falls short of the linked objective.
Issue #614 calls for "surfacing errors in the affected widget rather than crashing the dashboard." Right now, a transform failure silently falls back to raw data and only logs to the console — users see what looks like a working chart with subtly wrong data (or, when chart transform fails, the chart-shaped renderer receives raw rows of an unexpected shape and may itself misrender). At minimum, render an inline warning banner (similar to the truncation notice at lines 631–639) when transformError is set.
Also applies to: 610-613
🤖 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 311 - 317, The catch that
handles chart transform failures currently only logs and falls back to
previewData (mappedData) — instead set a component state (e.g., call
setTransformError(String(err)) or setTransformError(err)) inside that catch
(where mappedData = previewData is done) and clear that state on successful
transform (after mappedData is computed); then update the renderer JSX to show
an inline warning banner (reuse the truncation notice markup/structure used
around lines 631–639) when transformError is truthy so the affected widget
displays the error message (include widget.chartType and the transformError
text) instead of silently hiding the problem.
6. Validate compatibleWith at startup — warns if plugin references
a connector type that doesn't exist (e.g., "mongodb" when only
neo4j and postgresql are registered).
7. supportsColumnMapping now checks plugin.transformWithMapping
instead of a hardcoded set. Falls back to old set for unregistered
types. New plugins automatically get column mapping if they
provide transformWithMapping.
8. getChartDefaults extracts defaults from Zod settingsSchema —
was returning empty object, now parses {} through the schema
to get actual default values.
9. Connector formFields validated on registration — warns on:
missing key/label/type, duplicate keys, select fields with
no options. Category validated against enum.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/lib/plugin/chart-helpers.ts (1)
205-226:⚠️ Potential issue | 🟠 MajorStubs incorrectly report
supportsColumnMappingfor types that don't support it.The stubs attach
transformWithMapping: identityto all types exceptmarkdown/iframe. This causessupportsColumnMapping()to returntruewhenever the stub is in the registry—which happens before real plugins load (e.g., SSR or early imports). However, only types in the hardcodedCOLUMN_MAPPING_TYPES(["bar", "line", "pie"]) should report support at that stage. This breaks the fallback logic.Only attach
transformWithMappingto stubs for types inCOLUMN_MAPPING_TYPES:Proposed fix
- transformWithMapping: - def.type === "markdown" || def.type === "iframe" - ? nullTransform - : identity, + // Only types that actually support column mapping get a stub + // transformWithMapping; otherwise leave it undefined so + // supportsColumnMapping() returns false until the real plugin loads. + transformWithMapping: + def.type === "bar" || def.type === "line" || def.type === "pie" + ? identity + : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/plugin/chart-helpers.ts` around lines 205 - 226, The stub plugins in the LIGHTWEIGHT_DEFS loop are incorrectly assigning transformWithMapping: identity for every non-markdown/iframe type, causing supportsColumnMapping() to report true prematurely; update the defineChartPlugin call so transformWithMapping is identity only when def.type is one of the COLUMN_MAPPING_TYPES (e.g., ["bar","line","pie"]) and nullTransform otherwise (keep transform behavior as-is), i.e., change the transformWithMapping ternary to check membership in COLUMN_MAPPING_TYPES; this affects the loop that registers via pluginRegistry.register(defineChartPlugin(...)) where transformWithMapping, nullTransform, identity, and COLUMN_MAPPING_TYPES are referenced.
🧹 Nitpick comments (4)
app/src/lib/plugin/chart-helpers.ts (1)
286-297: PrefersafeParseand surface schema misconfig.The bare
try/catchsilently swallows every Zod failure, so a plugin author who ships asettingsSchemarequiring fields gets{}back forever with no signal. UsesafeParseand at least warn so it's diagnosable in dev.♻️ Proposed refactor
const plugin = pluginRegistry.get(type); if (plugin?.settingsSchema) { - try { - // Parse empty object through the Zod schema to extract defaults - return plugin.settingsSchema.parse({}) as Record<string, unknown>; - } catch { - // Schema requires fields — can't extract defaults - } + const result = plugin.settingsSchema.safeParse({}); + if (result.success) { + return result.data as Record<string, unknown>; + } + console.warn( + `getChartDefaults("${type}"): settingsSchema has no extractable defaults`, + result.error.issues, + ); } return {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/plugin/chart-helpers.ts` around lines 286 - 297, getChartDefaults currently swallows Zod errors by parsing inside a try/catch and returning {} silently; change it to call plugin.settingsSchema.safeParse({}) inside getChartDefaults (using the plugin from pluginRegistry.get(type)) and if result.success return the parsed value, otherwise surface the schema misconfiguration by logging a warning (include the chart type and result.error) so authors can diagnose during development (optionally gate the warning behind NODE_ENV !== 'production' if you want no noise in prod).app/src/lib/__tests__/plugin/chart-helpers.test.ts (2)
199-212: Test relies on real plugins replacing stubs — add a guardrail.These cases pass because
import "@/plugins/index"swaps the stubs for real plugins (markdown/iframe don't definetransformWithMapping). If someone later mocks@/plugins/indexor reorders imports, the stubs atchart-helpers.tsLines 216–219 currently settransformWithMapping: identityfor everything except markdown/iframe and the test would still pass — but for the wrong reason. Once the stub bug is fixed (see chart-helpers.ts review), consider adding a positive assertion that exercises the fallback path, e.g. an unknown type returnsfalseand a registered-but-no-mapping type (likesingle-valueonce stubs are corrected) returnsfalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/__tests__/plugin/chart-helpers.test.ts` around lines 199 - 212, Add explicit guardrail assertions to the supportsColumnMapping tests so they don't pass due to swapped plugin stubs: in the "returns false" spec for supportsColumnMapping add an assertion that an unknown widget type (e.g., "unknown-type") returns false and also assert a registered-but-no-mapping widget type (use a type known to be registered but without transformWithMapping such as "single-value" or "markdown" depending on your stubs) returns false; reference supportsColumnMapping and chart-helpers.ts to locate the function and ensure these two negative cases exercise the fallback path rather than relying on external plugin import ordering.
185-194: Assertion is too loose—tighten to verify actual defaults are extracted.
typeof defaults === "object"passes fornulland arrays, so this test would miss if the Zod parsing silently fails. The bar plugin's schema has concrete defaults (e.g.,showLegend: true,barWidth: 0,orientation: "vertical"). Assert at least one of them:♻️ Proposed refactor
it("returns defaults from settings schema when available", () => { const defaults = getChartDefaults("bar"); - // Bar plugin has a Zod schema with defaults — should extract them - expect(typeof defaults).toBe("object"); + // Bar plugin has a Zod schema with defaults — should extract them + expect(defaults).not.toBeNull(); + expect(Array.isArray(defaults)).toBe(false); + expect(defaults).toHaveProperty("showLegend", true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/__tests__/plugin/chart-helpers.test.ts` around lines 185 - 194, The test in chart-helpers.test.ts uses a too-loose assertion (expect(typeof defaults).toBe("object")) which can pass for null/arrays and won't catch Zod parsing failures; update the test that calls getChartDefaults("bar") to assert concrete expected default fields from the bar plugin (for example verify defaults.showLegend === true and/or defaults.barWidth === 0 and/or defaults.orientation === "vertical") and keep the unknown-type test as-is; locate the getChartDefaults call in the "returns defaults from settings schema when available" test and replace the generic type check with one or more strict equality checks against known default properties.connection/src/generalized/connector-plugin.ts (1)
166-176: Avoid duplicating the category list — derive from the type union.
validCategoriesat Line 166 is a hand-maintained copy of thecategoryunion declared at Line 35. Easy to drift. Pull it into a sharedconstand derive both the type and runtime list from it.♻️ Proposed refactor
+export const CONNECTOR_CATEGORIES = ["database", "graph", "api", "file"] as const; +export type ConnectorCategory = (typeof CONNECTOR_CATEGORIES)[number]; + export interface ConnectorPlugin { ... - category: "database" | "graph" | "api" | "file"; + category: ConnectorCategory; ... } ... - const validCategories = ["database", "graph", "api", "file"]; - if (plugin.category && !validCategories.includes(plugin.category)) { + if (plugin.category && !CONNECTOR_CATEGORIES.includes(plugin.category)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/src/generalized/connector-plugin.ts` around lines 166 - 176, The code duplicates the category list by defining a hard-coded validCategories array while there is already a category union type; replace the duplicate by introducing a single exported const (e.g., CATEGORY_LIST) containing the allowed category strings, change the existing category union type to be derived from that const (using typeof CATEGORY_LIST[number]), and then use that const in the runtime check instead of validCategories—update references to validCategories and the union type so both compile and validate from the same source of truth (affecting the existing validCategories variable, the category union, and the plugin.category check).
🤖 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/index.ts`:
- Around line 119-136: The hardcoded KNOWN_CONNECTORS set duplicates connector
types and misses dynamically registered connectors; replace it by importing the
canonical CONNECTOR_TYPES (from connection/src/connector-types.ts) and use that
collection (e.g., new Set(CONNECTOR_TYPES)) when validating
plugin.compatibleWith in the loop over pluginRegistry.getTypes(); update the
validation code that references KNOWN_CONNECTORS to reference the new
Set(CONNECTOR_TYPES) so registerConnector() additions are recognized.
In `@connection/src/generalized/connector-plugin.ts`:
- Around line 130-149: The duplicate-key logic is running even when field.key is
falsy, causing undefined/"" to be added and spurious duplicate warnings; update
the loop over plugin.formFields (the keys Set, the field variable, and the
duplicate check) so that you only perform keys.has(field.key),
keys.add(field.key), and the duplicate warning when field.key is truthy (i.e.,
skip both the duplicate detection and the add if !field.key), while keeping the
existing missing-key/type/label warning using plugin.type and field for context.
---
Outside diff comments:
In `@app/src/lib/plugin/chart-helpers.ts`:
- Around line 205-226: The stub plugins in the LIGHTWEIGHT_DEFS loop are
incorrectly assigning transformWithMapping: identity for every
non-markdown/iframe type, causing supportsColumnMapping() to report true
prematurely; update the defineChartPlugin call so transformWithMapping is
identity only when def.type is one of the COLUMN_MAPPING_TYPES (e.g.,
["bar","line","pie"]) and nullTransform otherwise (keep transform behavior
as-is), i.e., change the transformWithMapping ternary to check membership in
COLUMN_MAPPING_TYPES; this affects the loop that registers via
pluginRegistry.register(defineChartPlugin(...)) where transformWithMapping,
nullTransform, identity, and COLUMN_MAPPING_TYPES are referenced.
---
Nitpick comments:
In `@app/src/lib/__tests__/plugin/chart-helpers.test.ts`:
- Around line 199-212: Add explicit guardrail assertions to the
supportsColumnMapping tests so they don't pass due to swapped plugin stubs: in
the "returns false" spec for supportsColumnMapping add an assertion that an
unknown widget type (e.g., "unknown-type") returns false and also assert a
registered-but-no-mapping widget type (use a type known to be registered but
without transformWithMapping such as "single-value" or "markdown" depending on
your stubs) returns false; reference supportsColumnMapping and chart-helpers.ts
to locate the function and ensure these two negative cases exercise the fallback
path rather than relying on external plugin import ordering.
- Around line 185-194: The test in chart-helpers.test.ts uses a too-loose
assertion (expect(typeof defaults).toBe("object")) which can pass for
null/arrays and won't catch Zod parsing failures; update the test that calls
getChartDefaults("bar") to assert concrete expected default fields from the bar
plugin (for example verify defaults.showLegend === true and/or defaults.barWidth
=== 0 and/or defaults.orientation === "vertical") and keep the unknown-type test
as-is; locate the getChartDefaults call in the "returns defaults from settings
schema when available" test and replace the generic type check with one or more
strict equality checks against known default properties.
In `@app/src/lib/plugin/chart-helpers.ts`:
- Around line 286-297: getChartDefaults currently swallows Zod errors by parsing
inside a try/catch and returning {} silently; change it to call
plugin.settingsSchema.safeParse({}) inside getChartDefaults (using the plugin
from pluginRegistry.get(type)) and if result.success return the parsed value,
otherwise surface the schema misconfiguration by logging a warning (include the
chart type and result.error) so authors can diagnose during development
(optionally gate the warning behind NODE_ENV !== 'production' if you want no
noise in prod).
In `@connection/src/generalized/connector-plugin.ts`:
- Around line 166-176: The code duplicates the category list by defining a
hard-coded validCategories array while there is already a category union type;
replace the duplicate by introducing a single exported const (e.g.,
CATEGORY_LIST) containing the allowed category strings, change the existing
category union type to be derived from that const (using typeof
CATEGORY_LIST[number]), and then use that const in the runtime check instead of
validCategories—update references to validCategories and the union type so both
compile and validate from the same source of truth (affecting the existing
validCategories variable, the category union, and the plugin.category check).
🪄 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: 5a9cb3b3-ac44-439e-8882-adbd01b15c0a
📒 Files selected for processing (4)
app/src/lib/__tests__/plugin/chart-helpers.test.tsapp/src/lib/plugin/chart-helpers.tsapp/src/plugins/index.tsconnection/src/generalized/connector-plugin.ts
Comment on lines
+119
to
+136
| // 2. Validate compatibleWith references actual connector types. | ||
| const KNOWN_CONNECTORS = new Set(["neo4j", "postgresql"]); | ||
| for (const type of pluginRegistry.getTypes()) { | ||
| const plugin = pluginRegistry.get(type); | ||
| if (plugin?.compatibleWith) { | ||
| for (const ct of plugin.compatibleWith) { | ||
| if (!KNOWN_CONNECTORS.has(ct)) { | ||
| console.warn( | ||
| 'Plugin "' + | ||
| type + | ||
| '" declares compatibleWith "' + | ||
| ct + | ||
| '" but no such connector is registered', | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate canonical connector type sources and any registry exports the app could use.
fd -t f 'connector-types'
rg -nP --type=ts -C2 '\bCONNECTOR_TYPES\b'
rg -nP --type=ts -C2 'createConnectorRegistry|connectorRegistry'Repository: alfredo1996/neoboard
Length of output: 14318
🏁 Script executed:
cat -n app/src/plugins/index.ts | sed -n '100,150p'Repository: alfredo1996/neoboard
Length of output: 1522
🏁 Script executed:
head -50 app/src/plugins/index.tsRepository: alfredo1996/neoboard
Length of output: 1805
🏁 Script executed:
head -60 connection/src/index.ts | cat -nRepository: alfredo1996/neoboard
Length of output: 1764
🏁 Script executed:
rg -n "export.*connectorRegistry" connection/src/index.tsRepository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
rg -A5 "connectorRegistry," connection/src/index.tsRepository: alfredo1996/neoboard
Length of output: 196
🏁 Script executed:
grep -r "from.*connection" app/src/lib --include="*.ts" --include="*.tsx" | head -10Repository: alfredo1996/neoboard
Length of output: 1054
Replace hardcoded KNOWN_CONNECTORS with canonical type source.
Line 120 duplicates the connector list from connection/src/connector-types.ts and won't catch dynamically registered connectors via registerConnector(). Import the canonical CONNECTOR_TYPES instead:
Proposed fix
// 2. Validate compatibleWith references actual connector types.
+import { CONNECTOR_TYPES } from "@neoboard/connection/connector-types";
-const KNOWN_CONNECTORS = new Set(["neo4j", "postgresql"]);
+const KNOWN_CONNECTORS = new Set(CONNECTOR_TYPES);
for (const type of pluginRegistry.getTypes()) {📝 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
| // 2. Validate compatibleWith references actual connector types. | |
| const KNOWN_CONNECTORS = new Set(["neo4j", "postgresql"]); | |
| for (const type of pluginRegistry.getTypes()) { | |
| const plugin = pluginRegistry.get(type); | |
| if (plugin?.compatibleWith) { | |
| for (const ct of plugin.compatibleWith) { | |
| if (!KNOWN_CONNECTORS.has(ct)) { | |
| console.warn( | |
| 'Plugin "' + | |
| type + | |
| '" declares compatibleWith "' + | |
| ct + | |
| '" but no such connector is registered', | |
| ); | |
| } | |
| } | |
| } | |
| } | |
| import { CONNECTOR_TYPES } from "@neoboard/connection/connector-types"; | |
| // 2. Validate compatibleWith references actual connector types. | |
| const KNOWN_CONNECTORS = new Set(CONNECTOR_TYPES); | |
| for (const type of pluginRegistry.getTypes()) { | |
| const plugin = pluginRegistry.get(type); | |
| if (plugin?.compatibleWith) { | |
| for (const ct of plugin.compatibleWith) { | |
| if (!KNOWN_CONNECTORS.has(ct)) { | |
| console.warn( | |
| 'Plugin "' + | |
| type + | |
| '" declares compatibleWith "' + | |
| ct + | |
| '" but no such connector is registered', | |
| ); | |
| } | |
| } | |
| } | |
| } |
🤖 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 119 - 136, The hardcoded
KNOWN_CONNECTORS set duplicates connector types and misses dynamically
registered connectors; replace it by importing the canonical CONNECTOR_TYPES
(from connection/src/connector-types.ts) and use that collection (e.g., new
Set(CONNECTOR_TYPES)) when validating plugin.compatibleWith in the loop over
pluginRegistry.getTypes(); update the validation code that references
KNOWN_CONNECTORS to reference the new Set(CONNECTOR_TYPES) so
registerConnector() additions are recognized.
Comment on lines
+130
to
+149
| const keys = new Set<string>(); | ||
| for (const field of plugin.formFields) { | ||
| if (!field.key || !field.label || !field.type) { | ||
| console.warn( | ||
| 'Connector "' + | ||
| plugin.type + | ||
| '": formField missing key/label/type:', | ||
| field, | ||
| ); | ||
| } | ||
| if (keys.has(field.key)) { | ||
| console.warn( | ||
| 'Connector "' + | ||
| plugin.type + | ||
| '": duplicate formField key "' + | ||
| field.key + | ||
| '"', | ||
| ); | ||
| } | ||
| keys.add(field.key); |
There was a problem hiding this comment.
Duplicate-key check leaks undefined keys.
When a field is missing key (already warned at Line 132), keys.add(field.key) still inserts undefined/"", and the next missing-key field then triggers a misleading "duplicate formField key 'undefined'" warning. Skip duplicate detection (and the add) when the key is falsy.
🛡️ Proposed fix
for (const field of plugin.formFields) {
if (!field.key || !field.label || !field.type) {
console.warn(
'Connector "' +
plugin.type +
'": formField missing key/label/type:',
field,
);
+ // Skip duplicate-key tracking when key is missing
+ continue;
}
if (keys.has(field.key)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/src/generalized/connector-plugin.ts` around lines 130 - 149, The
duplicate-key logic is running even when field.key is falsy, causing
undefined/"" to be added and spurious duplicate warnings; update the loop over
plugin.formFields (the keys Set, the field variable, and the duplicate check) so
that you only perform keys.has(field.key), keys.add(field.key), and the
duplicate warning when field.key is truthy (i.e., skip both the duplicate
detection and the add if !field.key), while keeping the existing
missing-key/type/label warning using plugin.type and field for context.
The previous commit used `await import("node:module")` inside a sync
function, causing "SyntaxError: Unexpected reserved word" in CI.
Fixed by importing createRequire at the top level and using
req.resolve() synchronously.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… shortcuts guide New chart docs: - gantt.mdx — query format, options, features - circle-packing.mdx — hierarchical data, options, features - choropleth.mdx — country name mapping, options, gradient New guide: - keyboard-shortcuts.mdx — all shortcuts for edit/view mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 26, 2026
This was referenced Apr 30, 2026
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
Critical safety fixes for the chart and connector plugin infrastructure based on a comprehensive audit.
Changes
1. External plugin crash prevention (
plugins/index.ts)2. Plugin config validation (
chart-plugin-registry.ts)3. Transform error handling (
card-container.tsx)4. Atomic manifest writes (
cli/src/lib/manifest.ts)5. Codegen package verification (
generate-*-imports.mjs)Package "x" is not installed. Run: npm install xTest plan
Closes #614
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
Tests