Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 46 additions & 18 deletions app/src/components/card-container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -302,18 +302,33 @@ export function CardContainer({
/>
);
}
const mappedData = (
chartConfig.transformWithMapping ?? chartConfig.transform
)(previewData, columnMapping);
let mappedData: unknown;
try {
mappedData = (chartConfig.transformWithMapping ?? chartConfig.transform)(
previewData,
columnMapping,
);
} catch (err) {
console.error(
"Chart transform failed for " + widget.chartType + ":",
err,
);
mappedData = previewData;
}
Comment on lines +311 to +317

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

// Skip transforms for graph charts — their data shape is incompatible with tabular transforms
const transformedData =
dataTransforms.length && widget.chartType !== "graph"
? applyTransforms(
mappedData as Record<string, unknown>[],
dataTransforms,
allParamValues,
)
: mappedData;
let transformedData: unknown = mappedData;
if (dataTransforms.length && widget.chartType !== "graph") {
try {
transformedData = applyTransforms(
mappedData as Record<string, unknown>[],
dataTransforms,
allParamValues,
);
} catch (err) {
console.error("Data transform failed:", err);
transformedData = mappedData;
}
}
const availableColumns = extractColumnNames(previewData);
return (
<div className="h-full w-full flex flex-col">
Expand Down Expand Up @@ -586,16 +601,29 @@ export function CardContainer({
);
}

const mappedData = (
chartConfig.transformWithMapping ?? chartConfig.transform
)(rawData, columnMapping);
const transformedData = dataTransforms.length
? applyTransforms(
let mappedData: unknown;
try {
mappedData = (chartConfig.transformWithMapping ?? chartConfig.transform)(
rawData,
columnMapping,
);
} catch (err) {
console.error("Chart transform failed for " + widget.chartType + ":", err);
mappedData = rawData;
}
let transformedData: unknown = mappedData;
if (dataTransforms.length) {
try {
transformedData = applyTransforms(
mappedData as Record<string, unknown>[],
dataTransforms,
allParamValues,
)
: mappedData;
);
} catch (err) {
console.error("Data transform failed:", err);
transformedData = mappedData;
}
}
const availableColumns = extractColumnNames(rawData);

return (
Expand Down
20 changes: 14 additions & 6 deletions app/src/lib/__tests__/plugin/chart-helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,24 +182,32 @@ describe("chartRequiresQuery", () => {
// getChartDefaults
// ---------------------------------------------------------------------------
describe("getChartDefaults", () => {
it("returns empty object", () => {
expect(getChartDefaults("bar")).toEqual({});
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");
});

it("returns empty object for unknown type", () => {
expect(getChartDefaults("unknown_type_xyz")).toEqual({});
});
});

// ---------------------------------------------------------------------------
// supportsColumnMapping
// ---------------------------------------------------------------------------
describe("supportsColumnMapping", () => {
it("returns true for bar, line, pie", () => {
it("returns true for types with transformWithMapping", () => {
// bar, line, pie all define transformWithMapping
expect(supportsColumnMapping("bar")).toBe(true);
expect(supportsColumnMapping("line")).toBe(true);
expect(supportsColumnMapping("pie")).toBe(true);
});

it("returns false for table, json", () => {
expect(supportsColumnMapping("table")).toBe(false);
expect(supportsColumnMapping("json")).toBe(false);
it("returns false for types without transformWithMapping", () => {
// markdown has no transformWithMapping (content-only widget)
expect(supportsColumnMapping("markdown")).toBe(false);
expect(supportsColumnMapping("iframe")).toBe(false);
});
});

Expand Down
19 changes: 17 additions & 2 deletions app/src/lib/plugin/chart-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,15 +283,30 @@ export function chartRequiresQuery(type: string): boolean {
* Get default chart settings for a chart type.
* Returns an empty object — defaults are managed by Zod schemas in plugins.
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function getChartDefaults(_type: string): Record<string, unknown> {
export function getChartDefaults(type: string): Record<string, unknown> {
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
}
}
return {};
}

/**
* Check whether a chart type supports column mapping.
* Uses the plugin's transformWithMapping function as the signal —
* if a plugin provides it, column mapping is supported.
* Falls back to the hardcoded set for plugins that haven't adopted yet.
*/
export function supportsColumnMapping(type: string): boolean {
const plugin = pluginRegistry.get(type);
if (plugin) {
return typeof plugin.transformWithMapping === "function";
}
return COLUMN_MAPPING_TYPES.has(type);
}

Expand Down
26 changes: 25 additions & 1 deletion app/src/lib/plugin/chart-plugin-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@
requiresQuery: true,
};

export function defineChartPlugin(config: ChartPluginConfig): ChartPlugin {

Check failure on line 134 in app/src/lib/plugin/chart-plugin-registry.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=alfredo1996_neoboard&issues=AZ3IvZtNKzKY1UxreH5S&open=AZ3IvZtNKzKY1UxreH5S&pullRequest=615
// Validation
// ── Validation ──────────────────────────────────────────────────────
if (!config.type || config.type.trim() === "") {
throw new Error("Chart plugin: type is required and cannot be empty");
}
Expand All @@ -143,6 +143,30 @@
throw new Error("Chart plugin: transform must be a function");
}

// Validate options shape if provided
if (config.options) {
for (const opt of config.options) {
if (!opt.key || !opt.label || !opt.type) {
console.warn(
'Chart plugin "' + config.type + '": option missing key/label/type:',
opt,
);
}
}
}

// Validate compatibleWith entries
if (config.compatibleWith) {
for (const ct of config.compatibleWith) {
if (typeof ct !== "string" || ct.trim() === "") {
console.warn(
'Chart plugin "' + config.type + '": invalid compatibleWith entry:',
ct,
);
}
}
}

// supportsStyling defaults to true if stylingTargets is provided, false otherwise
const stylingFromTargets =
config.stylingTargets && config.stylingTargets.length > 0;
Expand Down
56 changes: 47 additions & 9 deletions app/src/plugins/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,40 @@ for (const plugin of BUILT_IN_PLUGINS) {
// Same-type duplicates without overrides throw loudly so operators spot
// the conflict at startup instead of debugging a silent replacement.
for (const { plugin, overrides } of EXTERNAL_PLUGINS) {
if (pluginRegistry.has(plugin.type)) {
if (!overrides) {
throw new Error(
`External plugin "${plugin.type}" conflicts with an existing plugin. ` +
`Set "overrides": true in neoboard-plugins.json to replace the built-in.`,
try {
if (!plugin || typeof plugin !== "object" || !plugin.type) {
console.error(
"External plugin skipped: invalid plugin object (missing type)",
);
continue;
}
pluginRegistry.unregister(plugin.type);
if (pluginRegistry.has(plugin.type)) {
if (!overrides) {
console.error(
'External plugin "' +
plugin.type +
'" conflicts with an existing plugin. ' +
'Set "overrides": true in neoboard-plugins.json to replace it. Skipping.',
);
continue;
}
pluginRegistry.unregister(plugin.type);
}
pluginRegistry.register(plugin);
} catch (err) {
console.error(
"External plugin registration failed for type " +
JSON.stringify(plugin?.type) +
":",
err,
);
// Continue loading remaining plugins — one broken plugin shouldn't crash the app
}
pluginRegistry.register(plugin);
}

// ── Startup validation ──────────────────────────────────────────────────
// Verify that every chart type declared in CHART_TYPES has a registered
// plugin. A mismatch is a dev-time bug, not a runtime error.

// 1. Every CHART_TYPES entry must have a registered plugin.
const registeredTypes = new Set(pluginRegistry.getTypes());
for (const t of CHART_TYPES) {
if (!registeredTypes.has(t)) {
Expand All @@ -97,6 +116,25 @@ for (const t of CHART_TYPES) {
}
}

// 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',
);
}
}
}
}
Comment on lines +119 to +136

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.ts

Repository: alfredo1996/neoboard

Length of output: 1805


🏁 Script executed:

head -60 connection/src/index.ts | cat -n

Repository: alfredo1996/neoboard

Length of output: 1764


🏁 Script executed:

rg -n "export.*connectorRegistry" connection/src/index.ts

Repository: alfredo1996/neoboard

Length of output: 46


🏁 Script executed:

rg -A5 "connectorRegistry," connection/src/index.ts

Repository: alfredo1996/neoboard

Length of output: 196


🏁 Script executed:

grep -r "from.*connection" app/src/lib --include="*.ts" --include="*.tsx" | head -10

Repository: 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.


// Re-export for convenience
export { pluginRegistry } from "./registry";
export { CHART_TYPES, type ChartType } from "./chart-types";
45 changes: 40 additions & 5 deletions cli/src/lib/manifest.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
/**
* Read/write helpers for neoboard-plugins.json and neoboard-connectors.json.
*
* Writes use atomic temp-file + rename to prevent corruption from
* concurrent processes or crashes mid-write.
*/

import { readFileSync, writeFileSync, existsSync } from "node:fs";
import {
readFileSync,
writeFileSync,
renameSync,
existsSync,
unlinkSync,
} from "node:fs";
import { dirname, join } from "node:path";

export interface ManifestEntry {
package: string;
Expand All @@ -13,7 +23,31 @@ export interface ManifestEntry {
type ManifestKey = "plugins" | "connectors";

/**
* Read entries from a manifest file. Returns empty array if file missing.
* Atomically write JSON to a file: write to temp file, then rename.
* Rename is atomic on POSIX and near-atomic on Windows.
*/
function atomicWriteJson(filePath: string, data: unknown): void {
const tmpPath = join(
dirname(filePath),
".tmp-" + Date.now() + "-" + Math.random().toString(36).slice(2),
);
try {
writeFileSync(tmpPath, JSON.stringify(data, null, 2) + "\n");
renameSync(tmpPath, filePath);
} catch (err) {
// Clean up temp file on failure
try {
unlinkSync(tmpPath);
} catch {
// ignore cleanup errors
}
throw err;
}
}

/**
* Read entries from a manifest file. Returns empty array if file missing
* or corrupted (with a warning for corruption).
*/
export function readManifest(
filePath: string,
Expand All @@ -23,7 +57,8 @@ export function readManifest(
try {
const raw = JSON.parse(readFileSync(filePath, "utf-8"));
return Array.isArray(raw[key]) ? raw[key] : [];
} catch {
} catch (err) {
console.warn("Failed to parse manifest " + filePath + ":", err);
return [];
}
}
Expand All @@ -42,7 +77,7 @@ export function addToManifest(
return false; // already exists
}
entries.push(entry);
writeFileSync(filePath, JSON.stringify({ [key]: entries }, null, 2) + "\n");
atomicWriteJson(filePath, { [key]: entries });
return true;
}

Expand All @@ -59,6 +94,6 @@ export function removeFromManifest(
const entries = readManifest(filePath, key);
const filtered = entries.filter((e) => e.package !== packageName);
if (filtered.length === entries.length) return false; // not found
writeFileSync(filePath, JSON.stringify({ [key]: filtered }, null, 2) + "\n");
atomicWriteJson(filePath, { [key]: filtered });
return true;
}
Loading
Loading