[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).

[iobuhov]

LGTM

[github-actions]

AI Code Review

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

---

What was reviewed

File	Change
chart-playground-web/src/helpers/useComposedEditorController.ts	onViewSelectChange wrapped in useCallback; imports prettifyJson from shared helper
chart-playground-web/src/helpers/useV2EditorController.ts	onViewSelectChange memoized; keyBox synced dual-state pattern
chart-playground-web/src/helpers/__tests__/editorJson.spec.ts	New spec for shared prettifyJson helper
chart-playground-web/src/helpers/__tests__/stubObjectURL.ts	URL.createObjectURL shim for test env
chart-playground-web/src/helpers/__tests__/useComposedEditorController.spec.ts	Added identity-stability test; switched from plotly mocks to shim
chart-playground-web/src/helpers/__tests__/useV2EditorController.spec.ts	Added identity + view-switch tests; proper SetupHost harness
custom-chart-web/src/hooks/useCustomChart.ts	Dead code removed; computed().get() replaced with plain object; layoutOptions/configOptions fixed
custom-chart-web/src/controllers/CustomChartControllerHost.ts	store.data as Data[] replaced by toPlotlyData() boundary helper
custom-chart-web/src/utils/toPlotlyData.ts	New named cast boundary with JSDoc
custom-chart-web/src/utils/__tests__/toPlotlyData.spec.ts	New spec for boundary helper
custom-chart-web/src/hooks/__tests__/useCustomChart.spec.ts	New spec: layout/config passthrough, dead-field absence, reactivity under observer
custom-chart-web/src/hooks/__tests__/stubObjectURL.ts	URL.createObjectURL shim
custom-chart-web/CHANGELOG.md	"Fixed" entry for Modeler Layout/Configuration panels
shared/charts/src/model/stores/__tests__/EditableChart.store.spec.ts	New spec: 8 cases covering reset, setDataAt, setLayout, setConfig, JSON getters
shared/charts/package.json	@types/jest moved to devDependencies
shared/charts/tsconfig.build.json	"types": ["jest"] removed from production build tsconfig
custom-chart-web/openspec/changes/fix-rework-cleanup/	OpenSpec design/proposal/tasks artifacts

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

---

Findings

⚠️ Low — tasks.md documents B11 as "removed keyBox" but keyBox is still present

File: packages/pluggableWidgets/chart-playground-web/src/helpers/useV2EditorController.ts lines 35, 47, 92, 105
Note: tasks.md (B11 deviation note) states: "kept the React useState key… and removed keyBox. … Removed observable/runInAction imports." The actual code retains keyBox, keeps observable/runInAction, and still syncs two sources of truth manually in onViewSelectChange. The code works correctly (the two values are updated synchronously in the same handler, and the new "tracks the selected view" test confirms there's no desync) — but the documentation drift could mislead future readers into thinking the dual-source-of-truth issue was resolved when it wasn't. Either update the tasks.md deviation note to accurately describe what was done, or add an inline comment explaining why keyBox is still needed alongside the React state (the reason is sound: reaction needs a MobX-tracked observable, and React useState isn't one).

---

Positives

• The SetupHost subclass + observable.box test harness for EditableChartStore is clean and mirrors the production wiring without any hand-rolled mocks.
• stubObjectURL shims are scoped to __tests__/ and document exactly why they exist, making it easy to remove them if jsdom ever gains the API.
• toPlotlyData correctly localizes and documents the unavoidable Record<string, unknown>[] → Data[] boundary rather than scattering casts, and the JSDoc explains the store-side validation that makes this safe.
• Changelog entry is correctly scoped to the single user-visible fix (B3 empty-panel bug); all other findings are internal and appropriately omitted.
• reaction return value used directly as the useEffect cleanup — correct MobX/React pattern with no leaked reaction.
• Moving @types/jest out of dependencies and dropping it from tsconfig.build.json is the right fix; the production build no longer ships test-only types.