From b62356ac5dfb4ed41f6c221630681681dc5bda40 Mon Sep 17 00:00:00 2001 From: Illia Obukhau <8282906+iobuhov@users.noreply.github.com> Date: Wed, 2 Jul 2025 12:32:59 +0200 Subject: [PATCH 1/2] fix: prevent false positive warnings on empty editor trace --- .../mergeChartProps.spec.ts.snap | 71 ++++ .../src/__tests__/mergeChartProps.spec.ts | 402 ++++++++++++++++++ .../src/controllers/ChartPropsController.ts | 27 +- .../custom-chart-web/src/utils/utils.ts | 33 ++ 4 files changed, 508 insertions(+), 25 deletions(-) create mode 100644 packages/pluggableWidgets/custom-chart-web/src/__tests__/__snapshots__/mergeChartProps.spec.ts.snap create mode 100644 packages/pluggableWidgets/custom-chart-web/src/__tests__/mergeChartProps.spec.ts diff --git a/packages/pluggableWidgets/custom-chart-web/src/__tests__/__snapshots__/mergeChartProps.spec.ts.snap b/packages/pluggableWidgets/custom-chart-web/src/__tests__/__snapshots__/mergeChartProps.spec.ts.snap new file mode 100644 index 0000000000..dae080df89 --- /dev/null +++ b/packages/pluggableWidgets/custom-chart-web/src/__tests__/__snapshots__/mergeChartProps.spec.ts.snap @@ -0,0 +1,71 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`mergeChartProps utility function Data merging behavior should only process traces that exist in chart props 1`] = `[]`; + +exports[`mergeChartProps utility function Data merging behavior should preserve original trace data when editor state has missing or falsy entries 1`] = `[]`; + +exports[`mergeChartProps utility function Edge cases should handle empty chart props data 1`] = `[]`; + +exports[`mergeChartProps utility function Edge cases should handle falsy values in editor state data without warnings 1`] = `[]`; + +exports[`mergeChartProps utility function Edge cases should handle null JSON parsing 1`] = `[]`; + +exports[`mergeChartProps utility function Edge cases should handle various malformed JSON types 1`] = ` +[ + [ + "Editor props for trace(1) is not a valid JSON:{invalid json}", + ], + [ + "Please make sure the props is a valid JSON string.", + ], + [ + "Editor props for trace(3) is not a valid JSON:undefined", + ], + [ + "Please make sure the props is a valid JSON string.", + ], +] +`; + +exports[`mergeChartProps utility function JSON parsing failures generate console warnings should handle mixed valid and invalid JSON 1`] = ` +[ + [ + "Editor props for trace(1) is not a valid JSON:invalid json", + ], + [ + "Please make sure the props is a valid JSON string.", + ], +] +`; + +exports[`mergeChartProps utility function JSON parsing failures generate console warnings should not warn for valid JSON 1`] = `[]`; + +exports[`mergeChartProps utility function JSON parsing failures generate console warnings should not warn when editor state has undefined entries 1`] = `[]`; + +exports[`mergeChartProps utility function JSON parsing failures generate console warnings should warn for invalid JSON in editor state data 1`] = ` +[ + [ + "Editor props for trace(0) is not a valid JSON:invalid json", + ], + [ + "Please make sure the props is a valid JSON string.", + ], +] +`; + +exports[`mergeChartProps utility function JSON parsing failures generate console warnings should warn for multiple invalid JSON entries 1`] = ` +[ + [ + "Editor props for trace(0) is not a valid JSON:invalid json", + ], + [ + "Please make sure the props is a valid JSON string.", + ], + [ + "Editor props for trace(1) is not a valid JSON:{broken: json}", + ], + [ + "Please make sure the props is a valid JSON string.", + ], +] +`; diff --git a/packages/pluggableWidgets/custom-chart-web/src/__tests__/mergeChartProps.spec.ts b/packages/pluggableWidgets/custom-chart-web/src/__tests__/mergeChartProps.spec.ts new file mode 100644 index 0000000000..222bd5adbe --- /dev/null +++ b/packages/pluggableWidgets/custom-chart-web/src/__tests__/mergeChartProps.spec.ts @@ -0,0 +1,402 @@ +import { EditorStoreState } from "@mendix/shared-charts/main"; +import { ChartProps } from "../components/PlotlyChart"; +import { mergeChartProps } from "../utils/utils"; + +// Mock console.warn to capture warnings +let consoleWarnSpy: jest.SpyInstance; +let consoleMockCalls: string[][]; + +beforeEach(() => { + consoleMockCalls = []; + consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation((...args: any[]) => { + consoleMockCalls.push(args.map(arg => String(arg))); + }); +}); + +afterEach(() => { + consoleWarnSpy.mockRestore(); +}); + +describe("mergeChartProps utility function", () => { + describe("JSON parsing failures generate console warnings", () => { + it("should warn for invalid JSON in editor state data", () => { + // Arrange + const chartProps: ChartProps = { + data: [{ x: [1, 2], y: [3, 4], type: "scatter" }], + layout: { title: "Test Chart" }, + config: { displayModeBar: false }, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: ["invalid json"] + }; + + // Act + mergeChartProps(chartProps, editorState); + + // Assert + expect(consoleMockCalls).toMatchSnapshot(); + }); + + it("should warn for multiple invalid JSON entries", () => { + // Arrange + const chartProps: ChartProps = { + data: [ + { x: [1], y: [2], type: "scatter" }, + { x: [3], y: [4], type: "bar" } + ], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: ["invalid json", "{broken: json}"] + }; + + // Act + mergeChartProps(chartProps, editorState); + + // Assert + expect(consoleMockCalls).toMatchSnapshot(); + }); + + it("should not warn when editor state has undefined entries", () => { + // Arrange + const chartProps: ChartProps = { + data: [{ x: [1], y: [2], type: "scatter" }], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: [] // Empty array, so data[0] will be undefined + }; + + // Act + mergeChartProps(chartProps, editorState); + + // Assert + expect(consoleMockCalls).toMatchSnapshot(); + }); + + it("should not warn for valid JSON", () => { + // Arrange + const chartProps: ChartProps = { + data: [{ x: [1], y: [2], type: "scatter" }], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: ['{"marker": {"color": "red"}}'] + }; + + // Act + mergeChartProps(chartProps, editorState); + + // Assert + expect(consoleMockCalls).toMatchSnapshot(); + }); + + it("should handle mixed valid and invalid JSON", () => { + // Arrange + const chartProps: ChartProps = { + data: [ + { x: [1], y: [2], type: "scatter" }, + { x: [3], y: [4], type: "bar" }, + { x: [5], y: [6], type: "scatter" as const } + ], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: [ + '{"marker": {"color": "red"}}', // Valid + "invalid json", // Invalid + '{"marker": {"color": "blue"}}' // Valid + ] + }; + + // Act + mergeChartProps(chartProps, editorState); + + // Assert + expect(consoleMockCalls).toMatchSnapshot(); + }); + }); + + describe("Data merging behavior", () => { + it("should merge valid editor state data with chart props", () => { + // Arrange + const chartProps: ChartProps = { + data: [{ x: [1, 2], y: [3, 4], type: "scatter", name: "original" }], + layout: { title: "Original Title" }, + config: { displayModeBar: true }, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: '{"title": "Modified Title"}', + config: '{"displayModeBar": false}', + data: ['{"marker": {"color": "red"}, "name": "modified"}'] + }; + + // Act + const result = mergeChartProps(chartProps, editorState); + + // Assert + expect(result.data[0]).toEqual({ + x: [1, 2], + y: [3, 4], + type: "scatter", + name: "modified", // Overridden by editor state + marker: { color: "red" } // Added by editor state + }); + expect(result.layout.title).toBe("Modified Title"); + expect(result.config.displayModeBar).toBe(false); + }); + + it("should use empty object for invalid JSON entries", () => { + // Arrange + const chartProps: ChartProps = { + data: [{ x: [1], y: [2], type: "scatter", name: "original" }], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: ["invalid json"] + }; + + // Act + const result = mergeChartProps(chartProps, editorState); + + // Assert + expect(result.data[0]).toEqual({ + x: [1], + y: [2], + type: "scatter", + name: "original" // Original properties preserved + }); + }); + + it("should only process traces that exist in chart props", () => { + // Arrange + const chartProps: ChartProps = { + data: [{ x: [1], y: [2], type: "scatter" }], // Only 1 trace + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: [ + '{"marker": {"color": "red"}}', // Will be processed + '{"marker": {"color": "blue"}}', // Will be ignored + "invalid json" // Will be ignored + ] + }; + + // Act + const result = mergeChartProps(chartProps, editorState); + + // Assert + expect(result.data).toHaveLength(1); // Only 1 trace in result + expect(result.data[0]).toEqual({ + x: [1], + y: [2], + type: "scatter", + marker: { color: "red" } + }); + expect(consoleMockCalls).toMatchSnapshot(); // No warnings for ignored entries + }); + + it("should preserve original trace data when editor state has missing or falsy entries", () => { + // Arrange + const chartProps: ChartProps = { + data: [ + { x: [1], y: [1], type: "scatter", name: "original1", marker: { color: "blue" } }, + { x: [2], y: [2], type: "bar", name: "original2" }, + { x: [3], y: [3], type: "scatter", name: "original3" } + ], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: [""] // Only one entry (empty string), so traces 1 and 2 will get undefined + }; + + // Act + const result = mergeChartProps(chartProps, editorState); + + // Assert - All original trace data should be preserved + expect(result.data[0]).toEqual({ + x: [1], + y: [1], + type: "scatter", + name: "original1", + marker: { color: "blue" } + }); + expect(result.data[1]).toEqual({ x: [2], y: [2], type: "bar", name: "original2" }); + expect(result.data[2]).toEqual({ x: [3], y: [3], type: "scatter", name: "original3" }); + expect(consoleMockCalls).toMatchSnapshot(); + }); + }); + + describe("Edge cases", () => { + it("should handle empty chart props data", () => { + // Arrange + const chartProps: ChartProps = { + data: [], // No traces + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: ["invalid json"] // Will be ignored + }; + + // Act + const result = mergeChartProps(chartProps, editorState); + + // Assert + expect(result.data).toHaveLength(0); + expect(consoleMockCalls).toMatchSnapshot(); + }); + + it("should handle null JSON parsing", () => { + // Arrange + const chartProps: ChartProps = { + data: [{ x: [1], y: [2], type: "scatter" }], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: ["null"] // Valid JSON that parses to null + }; + + // Act + const result = mergeChartProps(chartProps, editorState); + + // Assert + expect(result.data[0]).toEqual({ + x: [1], + y: [2], + type: "scatter" + // null gets merged, but doesn't add properties + }); + expect(consoleMockCalls).toMatchSnapshot(); + }); + + it("should handle various malformed JSON types", () => { + // Arrange + const chartProps: ChartProps = { + data: [ + { x: [1], y: [1], type: "scatter" }, + { x: [2], y: [2], type: "bar" }, + { x: [3], y: [3], type: "scatter" as const }, + { x: [4], y: [4], type: "scatter" } + ], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: [ + '{"valid": "json"}', // Valid + "{invalid json}", // Invalid + "", // Invalid empty string - treated as falsy, early return + "undefined" // Invalid + ] + }; + + // Act + mergeChartProps(chartProps, editorState); + + // Assert + expect(consoleMockCalls).toMatchSnapshot(); + }); + + it("should handle falsy values in editor state data without warnings", () => { + // Arrange + const chartProps: ChartProps = { + data: [ + { x: [1], y: [1], type: "scatter", name: "trace1" }, + { x: [2], y: [2], type: "bar", name: "trace2" }, + { x: [3], y: [3], type: "scatter", name: "trace3" } + ], + layout: {}, + config: {}, + width: 800, + height: 600 + }; + + const editorState: EditorStoreState = { + layout: "{}", + config: "{}", + data: [ + "", // Empty string - falsy + null as any, // null - falsy + undefined as any // undefined - falsy + ] + }; + + // Act + const result = mergeChartProps(chartProps, editorState); + + // Assert - No warnings should be generated for falsy values + expect(consoleMockCalls).toMatchSnapshot(); + // Original traces should be returned unchanged + expect(result.data[0]).toEqual({ x: [1], y: [1], type: "scatter", name: "trace1" }); + expect(result.data[1]).toEqual({ x: [2], y: [2], type: "bar", name: "trace2" }); + expect(result.data[2]).toEqual({ x: [3], y: [3], type: "scatter", name: "trace3" }); + }); + }); +}); diff --git a/packages/pluggableWidgets/custom-chart-web/src/controllers/ChartPropsController.ts b/packages/pluggableWidgets/custom-chart-web/src/controllers/ChartPropsController.ts index 522aab94ae..4ec41dc652 100644 --- a/packages/pluggableWidgets/custom-chart-web/src/controllers/ChartPropsController.ts +++ b/packages/pluggableWidgets/custom-chart-web/src/controllers/ChartPropsController.ts @@ -4,7 +4,7 @@ import { executeAction } from "@mendix/widget-plugin-platform/framework/execute- import { makeAutoObservable } from "mobx"; import { Config, Data, Layout } from "plotly.js-dist-min"; import { ChartProps } from "../components/PlotlyChart"; -import { parseConfig, parseData, parseLayout } from "../utils/utils"; +import { parseConfig, parseData, parseLayout, mergeChartProps } from "../utils/utils"; import { ControllerProps } from "./typings"; interface SizeProvider { @@ -158,29 +158,6 @@ export class ChartPropsController implements ReactiveController { get mergedProps(): ChartProps { const props = this.chartProps; const state = this.editorStateGate.props; - return { - ...props, - config: { - ...props.config, - ...parseConfig(state.config) - }, - layout: { - ...props.layout, - ...parseLayout(state.layout) - }, - data: props.data.map((trace, index) => { - let stateTrace: Data; - try { - stateTrace = JSON.parse(state.data[index]); - } catch { - console.warn(`Failed to parse data for trace(${index})`); - stateTrace = {}; - } - return { - ...trace, - ...stateTrace - } as Data; - }) - }; + return mergeChartProps(props, state); } } diff --git a/packages/pluggableWidgets/custom-chart-web/src/utils/utils.ts b/packages/pluggableWidgets/custom-chart-web/src/utils/utils.ts index 9e42663c40..c5eadc2810 100644 --- a/packages/pluggableWidgets/custom-chart-web/src/utils/utils.ts +++ b/packages/pluggableWidgets/custom-chart-web/src/utils/utils.ts @@ -1,4 +1,7 @@ +import { EditorStoreState } from "@mendix/shared-charts/main"; import { Config, Data, Layout } from "plotly.js-dist-min"; +import { ChartProps } from "../components/PlotlyChart"; + export function parseData(staticData?: string, attributeData?: string, sampleData?: string): Data[] { let finalData: Data[] = []; @@ -44,3 +47,33 @@ export function parseConfig(configOptions?: string): Partial { return {}; } } + +export function mergeChartProps(chartProps: ChartProps, editorState: EditorStoreState): ChartProps { + return { + ...chartProps, + config: { + ...chartProps.config, + ...parseConfig(editorState.config) + }, + layout: { + ...chartProps.layout, + ...parseLayout(editorState.layout) + }, + data: chartProps.data.map((trace, index) => { + let stateTrace: Data = {}; + try { + if (!editorState.data || !editorState.data[index]) { + return trace; + } + stateTrace = JSON.parse(editorState.data[index]); + } catch { + console.warn(`Editor props for trace(${index}) is not a valid JSON:${editorState.data[index]}`); + console.warn("Please make sure the props is a valid JSON string."); + } + return { + ...trace, + ...stateTrace + } as Data; + }) + }; +} From 16c25aa9b8251c012222fbfaff3b625b2b53af91 Mon Sep 17 00:00:00 2001 From: Illia Obukhau <8282906+iobuhov@users.noreply.github.com> Date: Fri, 11 Jul 2025 14:16:16 +0200 Subject: [PATCH 2/2] fix: change how editor state initialized --- packages/shared/charts/src/helpers/useEditorStore.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/shared/charts/src/helpers/useEditorStore.ts b/packages/shared/charts/src/helpers/useEditorStore.ts index e2ac492d8c..7610acf221 100644 --- a/packages/shared/charts/src/helpers/useEditorStore.ts +++ b/packages/shared/charts/src/helpers/useEditorStore.ts @@ -1,7 +1,7 @@ import { Data } from "plotly.js-dist-min"; -import { useState, useReducer, useEffect } from "react"; -import { EditorStore, EditorStoreState } from "./EditorStore"; +import { useEffect, useReducer, useState } from "react"; import { fallback, pprint } from "../utils/json"; +import { EditorStore, EditorStoreState } from "./EditorStore"; export type EditorStoreInitializer = () => EditorStoreState; @@ -37,6 +37,6 @@ export function initStateFromProps(data: Array>): EditorStoreIniti return () => ({ layout: pprint(fallback("")), config: pprint(fallback("")), - data: data.map(trace => pprint(fallback(trace.name))) + data: data.map(trace => pprint(JSON.stringify(trace))) }); }