Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
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
33 changes: 26 additions & 7 deletions app/src/plugins/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,35 @@ 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 ──────────────────────────────────────────────────
Expand Down
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;
}
20 changes: 20 additions & 0 deletions scripts/generate-connector-imports.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,26 @@ export function runGenerator(opts = {}) {
return { ok: false, errors, wrote: false };
}

// Verify that all referenced packages are actually installed
for (const entry of entries) {
try {
import.meta.resolve?.(entry.package);
} catch {
try {
const { createRequire } = await import("node:module");
const require = createRequire(manifestPath);
require.resolve(entry.package);
} catch {
errors.push(
`Package "${entry.package}" is not installed. Run: npm install ${entry.package}`,
);
}
}
}
if (errors.length > 0) {
return { ok: false, errors, wrote: false };
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const source = renderSource(entries);

const existing = existsSync(outputPath)
Expand Down
21 changes: 21 additions & 0 deletions scripts/generate-plugin-imports.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,27 @@ export function runGenerator(opts = {}) {
return { ok: false, errors, wrote: false };
}

// Verify that all referenced packages are actually installed
for (const entry of entries) {
try {
import.meta.resolve?.(entry.package);
} catch {
// Fallback: try require.resolve via createRequire
try {
const { createRequire } = await import("node:module");
const require = createRequire(manifestPath);
require.resolve(entry.package);
} catch {
errors.push(
`Package "${entry.package}" is not installed. Run: npm install ${entry.package}`,
);
}
}
}
if (errors.length > 0) {
return { ok: false, errors, wrote: false };
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const source = renderSource(entries);

// Idempotent write — skip if content matches (preserves mtime for
Expand Down
Loading