[WC-3488]: Charts rework & cleanup

[yordan-st]

Pull request type

Bug fix (non-breaking change which fixes an issue)

---

Description

WC-3488 — cleanup of the unresolved code review (Leo) on the already-merged custom-chart / playground editor rework. All findings shipped unfixed and were still live on main; this PR resolves them.

What & why, per finding:

• [Critical] computed().get() misuse — useCustomChart.ts wrapped a plain object in computed(() => ({...})).get(), allocating a throwaway MobX atom every render with no caching or reactivity benefit. Replaced with a plain object literal; reactivity is handled by the existing observer wrapper on CustomChart.tsx.
• [Critical] containerStyle dead code — the hook returned a containerStyle never consumed (CustomChart.tsx computes its own via constructWrapperStyle). Removed the computation, the return field, and the unused import.
• [Important] hardcoded layoutOptions: {} / configOptions: {} — this silently blanked the playground's Modeler Layout and Modeler Configuration panels. Now passes the adapter's real layout / config. (User-visible → changelog entry added.)
• [Important] store.data as Data[] unsound cast — replaced the bare cast with a single named boundary helper toPlotlyData(). No runtime validation added: the store already guards on write (setDataAt parses, type-checks, warns), and Plotly's Data union is impractical to validate exhaustively — the helper just localizes and documents the unavoidable conversion.
• [Important] @types/jest in dependencies — moved to devDependencies and dropped "types": ["jest"] from tsconfig.build.json so jest types no longer ship as a runtime dep / pollute the production build.
• [Important] deleted 402-line test, no replacement — added a unit spec for EditableChartStore (8 cases: reset, setDataAt valid/out-of-range/invalid-JSON/non-object, setLayout/setConfig null-guard, JSON getters).
• [Minor] undocumented eslint-disable react-hooks/set-state-in-effect — documented why it's safe and the accepted risk.
• [Minor] duplicate prettifyJson — extracted to one shared helper, imported in both editor controllers.
• [Minor] onViewSelectChange not memoized — wrapped in useCallback in both controllers.
• [Minor] key duplicated (React state + MobX box) — collapsed to a single source of truth in useV2EditorController (kept the React state that drives render; the reaction now re-subscribes on the key via its effect deps with fireImmediately).

Deliberately out of scope (deferred to WC-3348):

• The silent empty-JSON catch {} blocks in the editor controllers (Leo's last Minor). WC-3488 marks this as WC-3348's scope; left untouched here.
• CodeEditor.tsx and the editor restore — that was WC-3348 ([WC-3348] Charts: restore highlighted JSON editor in playground #2310).

Notes for reviewers: the OpenSpec change (custom-chart-web/openspec/changes/fix-rework-cleanup/) has the full proposal / test plan / task breakdown. Two test-infra additions (zero product impact): a ^src/ moduleNameMapper in custom-chart-web/jest.config.js (mirrors the tsconfig baseUrl the source already uses) and a stubObjectURL shim (the shared-charts barrel loads Plotly, which touches URL.createObjectURL on import in the test env).

---

What should be covered while testing?

Automated (green locally):

• custom-chart-web — 33 tests · chart-playground-web — 6 tests · shared/charts — 45 tests
• Lint clean on all changed files; shared/charts production build passes after the B5 tsconfig change.

Manual QA — Modeler panels (the user-visible fix):

1. Add a Custom chart widget to a page; give it static/sample data (e.g. [{"type":"bar","x":[1,2],"y":[3,4]}]) and enable the playground slot. Optionally set a Layout value like {"title":"Test"}.
2. Run the app, hard-reload the browser (widget JS is cached — Empty Cache and Hard Reload).
3. Open the chart playground → in the sidebar view selector pick Layout, then Configuration.
4. Expected: the Modeler Layout / Modeler Configuration panels show real JSON (width/height/autosize/font/margin, plus any layout you set) — not {}.

Regression checks:

• Custom chart still renders and click events still fire.
• In the playground, switch between Layout / a trace / Configuration — the editor content follows the selection with no stale/desynced text.
• Edit a value in the editor → chart updates; feed invalid JSON → chart doesn't crash (invalid input is ignored — this catch remains silent by WC-3348 scope decision).

[github-actions]

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge

---

What was reviewed

File	Change
custom-chart-web/src/hooks/useCustomChart.ts	Removed dead getContainerStyle / containerStyle; replaced computed().get() wrapper with plain object; wired adapter.layout/adapter.config into playgroundData
custom-chart-web/src/controllers/CustomChartControllerHost.ts	Replaced bare as Data[] cast with toPlotlyData() helper
custom-chart-web/src/utils/toPlotlyData.ts	New boundary helper documenting the unavoidable Record → Data[] conversion
custom-chart-web/src/utils/__tests__/toPlotlyData.spec.ts	New unit tests for the boundary helper
custom-chart-web/src/hooks/__tests__/useCustomChart.spec.ts	New unit tests for B1/B2/B3
custom-chart-web/src/hooks/__tests__/stubObjectURL.ts	jsdom shim for URL.createObjectURL (Plotly import side-effect)
custom-chart-web/CHANGELOG.md	Added "Fixed" entry for the user-visible empty Modeler panels
chart-playground-web/src/helpers/prettifyJson.ts	Extracted shared prettifyJson helper
chart-playground-web/src/helpers/useComposedEditorController.ts	Uses shared prettifyJson; memoized onViewSelectChange; documented set-state-in-effect suppression
chart-playground-web/src/helpers/useV2EditorController.ts	Removed keyBox (dual source of truth); reaction now keys off React key state with fireImmediately: true; memoized onViewSelectChange
chart-playground-web/src/helpers/__tests__/prettifyJson.spec.ts	New unit tests for the helper
chart-playground-web/src/helpers/__tests__/useComposedEditorController.spec.ts	New unit test: onViewSelectChange identity stability
chart-playground-web/src/helpers/__tests__/useV2EditorController.spec.ts	New unit tests: identity stability + view-switch desync regression
chart-playground-web/src/helpers/__tests__/stubObjectURL.ts	jsdom shim for URL.createObjectURL (same reason, different package)
shared/charts/src/model/stores/__tests__/EditableChart.store.spec.ts	New 8-case unit spec for EditableChartStore (replacing deleted spec)
shared/charts/package.json	Moved @types/jest from dependencies → devDependencies
shared/charts/tsconfig.build.json	Removed "types": ["jest"] from production tsconfig

Skipped (out of scope): pnpm-lock.yaml, openspec/ planning artifacts

---

Findings

⚠️ Low — console.warn spy can leak if an assertion fails before mockRestore()

File: packages/shared/charts/src/model/stores/__tests__/EditableChart.store.spec.ts line 66–74
Problem: warn.mockRestore() is called unconditionally at the end of the test body. If expect(warn).toHaveBeenCalledTimes(1) throws, mockRestore() is never reached and console.warn stays mocked for later tests in the run.
Fix: Use a try/finally block, or move mock setup/teardown into beforeEach/afterEach:

it("setDataAt swallows invalid JSON, keeps data, and warns", () => {
    const { store, dispose } = setupStore({ layout: {}, config: {}, data: [{ x: [1] }] });
    const before = store.data;
    const warn = jest.spyOn(console, "warn").mockImplementation(() => undefined);

    try {
        store.setDataAt(0, "{ not json ");
        expect(store.data).toBe(before);
        expect(warn).toHaveBeenCalledTimes(1);
    } finally {
        warn.mockRestore();
        dispose();
    }
});

---

⚠️ Low — stubObjectURL.ts is duplicated across two packages

File: custom-chart-web/src/hooks/__tests__/stubObjectURL.ts and chart-playground-web/src/helpers/__tests__/stubObjectURL.ts
Note: Both files are byte-for-byte equivalent (modulo the leading comment). This is the same smell that motivated extracting prettifyJson. Since both packages already import from @mendix/shared-charts, a shared test-utils export or a jest.setup.ts entry in the relevant packages' jest configs would avoid future drift. Not blocking — intra-package test helpers are commonly duplicated across packages in a monorepo, and there is no existing shared test-utils location for this shim.

---

Positives

• The useV2EditorController refactor is clean: collapsing key/keyBox into a single React state and replacing the keyBox-driven reaction with { fireImmediately: true } on the existing effect deps is exactly the right fix — fewer moving parts, same semantics.
• toPlotlyData is a textbook boundary helper: one named, documented location for the unavoidable cast, rather than inline as Data[] scattered through callers.
• The useCustomChart.spec.ts reactivity test (observer Probe + waitFor) correctly mirrors how CustomChart.tsx actually consumes the hook, making the regression test structurally sound.
• The EditableChart.store.spec.ts covers all the critical edge cases in the design doc (out-of-range index, invalid JSON, non-object JSON, null-guard) and uses the SetupHost/ComputedAtom harness correctly — no hand-rolled mock stores.
• Moving @types/jest to devDependencies and removing "types": ["jest"] from tsconfig.build.json is a correct packaging hygiene fix; test types have no business in a production build artifact.
• CHANGELOG entry is scoped precisely: only the user-visible B3 fix (empty Modeler panels) is mentioned; internal cleanups are correctly omitted.