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
23 changes: 23 additions & 0 deletions app/src/components/dashboard-container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
import {
buildCsvString,
triggerDownload,
triggerSvgDownload,
buildExportFilename,
exportChartToSvg,
} from "@neoboard/components";
import { interpolateTitle } from "@/lib/widget/interpolate-title";
import { buildExportData } from "@/lib/widget/card-utils";
Expand All @@ -15,6 +17,7 @@
isWidgetTemplateOutdated,
} from "@/lib/widget/widget-utils";
import { isDataWidget } from "@/lib/widget/widget-actions";
import { getChartConfig } from "@/lib/plugin/chart-helpers";
import type {
DashboardPage,
DashboardWidget,
Expand Down Expand Up @@ -167,7 +170,20 @@
triggerDownload(csv, filename);
}

function exportWidgetSvg(widget: DashboardWidget) {
const el = document.querySelector(`[data-widget-id="${widget.id}"]`);
if (!el) return;
// Find the ECharts container (div with data-testid="base-chart")
const chartEl = el.querySelector<HTMLElement>('[data-testid="base-chart"]');
if (!chartEl) return;
const svg = exportChartToSvg(chartEl);
if (!svg) return;
const title = (widget.settings?.title as string) || widget.chartType;
const filename = buildExportFilename(title, "svg", page.title);
triggerSvgDownload(svg, filename);
}

const buildActions = (widget: DashboardWidget) => {

Check failure on line 186 in app/src/components/dashboard-container.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

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

See more on https://sonarcloud.io/project/issues?id=alfredo1996_neoboard&issues=AZ4Xza7KQ0tf3dxS1eGV&open=AZ4Xza7KQ0tf3dxS1eGV&pullRequest=720
const actions = [];

if (isDataWidget(widget.chartType)) {
Expand All @@ -177,6 +193,13 @@
});
}

if (getChartConfig(widget.chartType)?.capabilities.isECharts) {
actions.push({
label: "Export SVG",
onClick: () => exportWidgetSvg(widget),
});
}

if (onSaveAsTemplate) {
actions.push({
label: "Save to Widget Lab",
Expand Down
1 change: 1 addition & 0 deletions component/src/charts/__tests__/echarts-mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,6 @@ export function registerEChartsMocks() {

vi.mock("echarts/renderers", () => ({
CanvasRenderer: vi.fn(),
SVGRenderer: vi.fn(),
}));
}
50 changes: 49 additions & 1 deletion component/src/charts/base-chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
MarkLineComponent,
GraphicComponent,
} from "echarts/components";
import { CanvasRenderer } from "echarts/renderers";
import { CanvasRenderer, SVGRenderer } from "echarts/renderers";
import type { EChartsOption } from "echarts";
import { cn } from "@/lib/utils";
import type { BaseChartProps, EChartsClickEvent } from "./types";
Expand Down Expand Up @@ -46,6 +46,7 @@
MarkLineComponent,
GraphicComponent,
CanvasRenderer,
SVGRenderer,
]);

// Register NeoBoard themes once at module load
Expand Down Expand Up @@ -279,9 +280,56 @@
);
}

/**
* Export an ECharts chart to SVG by creating an offscreen instance with the
* SVG renderer and the same options as the visible chart.
*
* @param container - The DOM element containing the ECharts canvas
* @returns SVG string or null if the chart instance can't be found
*/
function exportChartToSvg(container: HTMLElement): string | null {
const instance = echarts.getInstanceByDom(container);
if (!instance) return null;

const options = instance.getOption();
const { width, height } = instance.getDom().getBoundingClientRect();

// Create an offscreen container for the SVG renderer
const offscreen = document.createElement("div");
offscreen.style.width = `${width}px`;
offscreen.style.height = `${height}px`;
offscreen.style.position = "absolute";
offscreen.style.left = "-9999px";
document.body.appendChild(offscreen);

try {
const themeName = isDarkMode() ? THEME_DARK : THEME_LIGHT;
const svgInstance = echarts.init(offscreen, themeName, {
renderer: "svg",
width,
height,
});
svgInstance.setOption(options);

// Extract the rendered SVG from the offscreen container
const svgEl = offscreen.querySelector("svg");
if (!svgEl) {
svgInstance.dispose();
return null;
}

const svgString = new XMLSerializer().serializeToString(svgEl);
svgInstance.dispose();
return svgString;
} finally {
document.body.removeChild(offscreen);

Check warning on line 325 in component/src/charts/base-chart.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`.

See more on https://sonarcloud.io/project/issues?id=alfredo1996_neoboard&issues=AZ4Xza_0Q0tf3dxS1eGW&open=AZ4Xza_0Q0tf3dxS1eGW&pullRequest=720
}
}
Comment on lines +290 to +327

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 | ⚖️ Poor tradeoff

Dispose the ECharts instance in all code paths to prevent memory leaks.

If svgInstance.setOption(options) (line 312) or any subsequent operation throws an exception, svgInstance.dispose() won't be called, leaving the ECharts instance alive in memory. Each failed export attempt will leak an instance.

🔧 Proposed fix to ensure disposal in all paths
   try {
     const themeName = isDarkMode() ? THEME_DARK : THEME_LIGHT;
     const svgInstance = echarts.init(offscreen, themeName, {
       renderer: "svg",
       width,
       height,
     });
-    svgInstance.setOption(options);
-
-    // Extract the rendered SVG from the offscreen container
-    const svgEl = offscreen.querySelector("svg");
-    if (!svgEl) {
+    try {
+      svgInstance.setOption(options);
+
+      // Extract the rendered SVG from the offscreen container
+      const svgEl = offscreen.querySelector("svg");
+      if (!svgEl) {
+        return null;
+      }
+
+      return new XMLSerializer().serializeToString(svgEl);
+    } finally {
       svgInstance.dispose();
-      return null;
     }
-
-    const svgString = new XMLSerializer().serializeToString(svgEl);
-    svgInstance.dispose();
-    return svgString;
   } finally {
     document.body.removeChild(offscreen);
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@component/src/charts/base-chart.tsx` around lines 290 - 327, In
exportChartToSvg, ensure the SVG-mode ECharts instance (svgInstance) is always
disposed to avoid leaks: move creation of svgInstance and any calls that can
throw (echarts.init and svgInstance.setOption) into a try block and call
svgInstance.dispose() in a finally block (or track svgInstance and dispose it in
the existing finally before removing the offscreen element); reference
svgInstance and the exportChartToSvg function to locate where to add the
guaranteed dispose.


export {
BaseChart,
CHART_COLORS_FALLBACK as CHART_COLORS,
resolveChartColors,
useDarkMode,
exportChartToSvg,
};
1 change: 1 addition & 0 deletions component/src/charts/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export {
CHART_COLORS,
resolveChartColors,
useDarkMode,
exportChartToSvg,
} from "./base-chart";
export {
THEME_LIGHT,
Expand Down
15 changes: 15 additions & 0 deletions component/src/lib/__tests__/export-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest";
import {
buildCsvString,
triggerDownload,
triggerSvgDownload,
buildExportFilename,
escapeCsvCell,
} from "../export-utils";
Expand Down Expand Up @@ -207,3 +208,17 @@ describe("triggerDownload", () => {
expect(typeof triggerDownload).toBe("function");
});
});

describe("triggerSvgDownload", () => {
it("is a function", () => {
expect(typeof triggerSvgDownload).toBe("function");
});
});

describe("buildExportFilename — svg extension", () => {
it("builds filename with svg extension", () => {
expect(buildExportFilename("Sales Chart", "svg", "Dashboard")).toBe(
"dashboard_sales-chart.svg",
);
});
});
7 changes: 7 additions & 0 deletions component/src/lib/export-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,10 @@ export function triggerPngDownload(dataUrl: string, filename: string): void {
a.click();
document.body.removeChild(a);
}

/**
* Trigger an SVG file download from an SVG string.
*/
export function triggerSvgDownload(svgString: string, filename: string): void {
triggerDownload(svgString, filename, "image/svg+xml");
}
9 changes: 8 additions & 1 deletion component/src/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
// Utility functions
export { cn } from "../lib/utils";
export { substituteParams } from "../lib/param-substitute";
export { buildCsvString, triggerDownload, triggerPngDownload, buildExportFilename, escapeCsvCell } from "../lib/export-utils";
export {
buildCsvString,
triggerDownload,
triggerPngDownload,
triggerSvgDownload,
buildExportFilename,
escapeCsvCell,
} from "../lib/export-utils";
1 change: 1 addition & 0 deletions component/vitest.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,5 @@ vi.mock("echarts/components", () => ({

vi.mock("echarts/renderers", () => ({
CanvasRenderer: vi.fn(),
SVGRenderer: vi.fn(),
}));
Loading