[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
packages/pluggableWidgets/custom-chart-web/src/hooks/useCustomChart.ts	Removed computed().get() misuse, dead containerStyle, hardcoded layoutOptions/configOptions
packages/pluggableWidgets/custom-chart-web/src/controllers/CustomChartControllerHost.ts	Replaced as Data[] cast with toPlotlyData() boundary helper
packages/pluggableWidgets/custom-chart-web/src/utils/toPlotlyData.ts	New type-boundary helper
packages/pluggableWidgets/custom-chart-web/src/hooks/__tests__/useCustomChart.spec.ts	New — B1/B2/B3 coverage
packages/pluggableWidgets/custom-chart-web/src/utils/__tests__/toPlotlyData.spec.ts	New — B4 round-trip coverage
packages/pluggableWidgets/custom-chart-web/CHANGELOG.md	Fixed entry for B3 (Modeler panels)
packages/pluggableWidgets/chart-playground-web/src/helpers/prettifyJson.ts	Extracted shared helper
packages/pluggableWidgets/chart-playground-web/src/helpers/useComposedEditorController.ts	onViewSelectChange memoized; documented lint suppression; prettifyJson deduplicated
packages/pluggableWidgets/chart-playground-web/src/helpers/useV2EditorController.ts	keyBox eliminated; reaction uses fireImmediately; onViewSelectChange memoized
packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/prettifyJson.spec.ts	New — B9 coverage
packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/useComposedEditorController.spec.ts	New — B10 coverage
packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/useV2EditorController.spec.ts	New — B10/B11 coverage
packages/shared/charts/package.json	@types/jest moved to devDependencies
packages/shared/charts/tsconfig.build.json	Dropped "types": ["jest"] from production build
packages/shared/charts/src/model/stores/__tests__/EditableChart.store.spec.ts	New — 8-case store spec
packages/pluggableWidgets/custom-chart-web/openspec/**	OpenSpec change artefacts

Skipped (out of scope): pnpm-lock.yaml, dist/

⚠️ CI checks — gh pr checks required interactive approval and could not be run. Verify green before merging.

---

Findings

⚠️ Low — useComposedEditorController spec covers only identity stability, not view-switching

File: packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/useComposedEditorController.spec.ts
Note: The V2 spec verifies that calling onViewSelectChange("config") updates both viewSelectValue and the displayed value. The V1 spec only asserts onViewSelectChange reference stability. Because V1's useEffect(() => setInput(code), [code]) pattern is different from V2's MobX reaction (and the design doc explicitly lists B11 as V2-only), the view-switching path in V1 is unverified. If the [code] dep ever silently breaks (e.g. code identity is stable across a key change), no test would catch it. Consider adding one case:

it("updates value when the selected view changes", () => {
    const data = makeData();
    const { result } = renderHook(() => useComposedEditorController(data));

    act(() => result.current.onViewSelectChange("config"));

    expect(result.current.viewSelectValue).toBe("config");
    expect(JSON.parse(result.current.value)).toEqual({ b: 2 });
});

---

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

Files:

• packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/stubObjectURL.ts
• packages/pluggableWidgets/custom-chart-web/src/hooks/__tests__/stubObjectURL.ts

The two files are near-identical (only the comment differs). If the shim ever needs updating (e.g. to return a stable blob URL), both copies would need to change. Not a blocker given separate package boundaries, but worth noting for future consolidation into a shared test-utils package or a jest.setup.ts in each package.

---

Positives

• B3 fix is user-visible and correctly landed: the CHANGELOG entry in custom-chart-web is under [Unreleased] > Fixed with a plain user-facing description — exactly right. The two other packages (internal-only changes) have no changelog, also correct per repo convention.
• @types/jest packaging fix is clean: moving to devDependencies and dropping "types": ["jest"] from tsconfig.build.json together ensures jest globals don't leak into the production type declaration output — the right two-file change, neither step alone would be sufficient.
• keyBox elimination is the correct trade-off: keeping useState as the single source of truth and using fireImmediately: true in the reaction is simpler and eliminates the manual runInAction sync. The useV2EditorController spec's "tracks the selected view without desyncing" case validates this end-to-end.
• toPlotlyData boundary helper: localising the as Data[] cast in a named, documented function with a JSDoc explaining why the unavoidable conversion exists is the right call — far better than re-adding runtime validation against Plotly's sprawling Data union.
• EditableChartStore spec coverage: 8 characterisation cases covering valid/out-of-range/invalid-JSON/non-object paths for setDataAt, plus null-guard for setLayout/setConfig and JSON getters. The SetupHost + observable.box harness is reusable and avoids hand-rolling a full MobX environment.