Skip to content

Commit 046f969

Browse files
yordan-styordan-st
committed
chore(custom-chart-web): add openspec change for WC-3488
1 parent 7d2003d commit 046f969

5 files changed

Lines changed: 283 additions & 0 deletions

File tree

‎packages/pluggableWidgets/custom-chart-web/openspec/changes/fix-rework-cleanup/.openspec.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: tdd-refactor
2+
created: 2026-07-07

‎packages/pluggableWidgets/custom-chart-web/openspec/changes/fix-rework-cleanup/design.md‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
## Test Cases
2+
3+
Most findings are internal cleanups; some are structurally verifiable by unit test,
4+
others (B5 packaging, B7 comment) are verified by build/inspection and carry no test.
5+
The central net-new coverage is **B6** (`EditableChartStore`), which also acts as the
6+
regression net for **B4** (data round-trips through the store before the Plotly cast).
7+
8+
Test files:
9+
10+
- `packages/shared/charts/src/model/stores/__tests__/EditableChart.store.spec.ts` (new — B6)
11+
- `packages/pluggableWidgets/custom-chart-web/src/hooks/__tests__/useCustomChart.spec.ts` (new — B1/B2/B3)
12+
- `packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/prettifyJson.spec.ts` (new — B9)
13+
14+
### Reproduction Tests
15+
16+
- **B3 — playgroundData carries real layout/config options** (unit)
17+
- **Given**: `useCustomChart` rendered with a `CustomChartContainerProps` whose adapter
18+
parses a non-empty `layoutStatic` / `configurationOptions` (e.g. `{ "title": "X" }` /
19+
`{ "displaylogo": true }`).
20+
- **When**: the hook returns `playgroundData`.
21+
- **Then**: `playgroundData.layoutOptions` equals the adapter's `layout` (contains
22+
`title: "X"`) and `playgroundData.configOptions` equals the adapter's `config`
23+
(contains `displaylogo: true`) — **not** `{}`.
24+
25+
- **B6 — store reset() loads props into layout/config/data** (unit)
26+
- **Given**: a fresh `EditableChartStore` wired to a `ComputedAtom` whose props are
27+
`{ layout: { a: 1 }, config: { b: 2 }, data: [{ x: [1] }] }`.
28+
- **When**: `setup()` autorun runs (or `reset` is called directly).
29+
- **Then**: `store.layout`, `store.config`, `store.data` equal those inputs.
30+
31+
### Edge Cases
32+
33+
- **B6 — setDataAt updates a valid index with valid JSON** (unit)
34+
- **Given**: store with `data = [{ x: [1] }, { x: [2] }]`.
35+
- **When**: `setDataAt(1, '{"x":[9]}')`.
36+
- **Then**: `store.data[1]` deep-equals `{ x: [9] }`; index 0 unchanged; `data` is a new
37+
array reference (observable.ref replaced, not mutated).
38+
39+
- **B6 — setDataAt ignores out-of-range index** (unit)
40+
- **Given**: store with `data` of length 2.
41+
- **When**: `setDataAt(5, '{"x":[9]}')` and `setDataAt(-1, '{"x":[9]}')`.
42+
- **Then**: `store.data` is unchanged (no throw).
43+
44+
- **B6 — setDataAt swallows invalid JSON and warns** (unit)
45+
- **Given**: store with a valid `data` array; `console.warn` spied.
46+
- **When**: `setDataAt(0, "{ not json ")`.
47+
- **Then**: `store.data` unchanged, no throw, `console.warn` called once.
48+
49+
- **B6 — setDataAt rejects non-object JSON (array / primitive)** (unit)
50+
- **Given**: store with valid `data`.
51+
- **When**: `setDataAt(0, "[1,2,3]")` and `setDataAt(0, "42")`.
52+
- **Then**: `store.data` unchanged (guard requires a non-array object).
53+
54+
- **B6 — setLayout / setConfig ignore null, replace on object** (unit)
55+
- **Given**: store with `layout = { a: 1 }`.
56+
- **When**: `setLayout(null as any)` then `setLayout({ c: 3 })`.
57+
- **Then**: after null, `layout` unchanged; after object, `layout` deep-equals `{ c: 3 }`
58+
and is a fresh reference. Same for `setConfig`.
59+
60+
- **B6 — JSON getters serialize current state** (unit)
61+
- **Given**: store with `layout = { a: 1 }`, `config = { b: 2 }`, `data = [{ x: [1] }]`.
62+
- **When**: read `layoutJson`, `configJson`, `dataJson`.
63+
- **Then**: `layoutJson === '{"a":1}'`, `configJson === '{"b":2}'`,
64+
`dataJson` deep-equals `['{"x":[1]}']`.
65+
66+
- **B9 — shared prettifyJson formats valid and flags invalid** (unit)
67+
- **Given**: the extracted `prettifyJson` helper.
68+
- **When**: called with `'{"a":1}'` and with `"{ bad"`.
69+
- **Then**: valid input returns 2-space-indented JSON (`'{\n "a": 1\n}'`); invalid input
70+
returns `'{ "error": "invalid JSON" }'`.
71+
72+
### Regression Tests
73+
74+
- **B1 — playgroundData is a plain reactive object, no stale caching** (unit)
75+
- **Given**: `useCustomChart` rendered inside an `observer` (mirrors `CustomChart.tsx`).
76+
- **When**: the store's `data` changes (e.g. via `setDataAt`) and the component re-renders.
77+
- **Then**: `playgroundData.plotData` reflects the new `store.data` (reactivity preserved
78+
after removing the `computed(...).get()` wrapper). `playgroundData.type === "editor.data.v2"`.
79+
80+
- **B2 — hook return shape has no containerStyle** (unit)
81+
- **Given**: `useCustomChart` rendered.
82+
- **When**: inspect the returned object keys.
83+
- **Then**: keys are exactly `{ playgroundData, ref }`; `containerStyle` absent. (Compile-time
84+
guarantee via `UseCustomChartReturn`; asserted at runtime as regression guard.)
85+
86+
- **B4 — store data round-trips into Plotly Data via the boundary helper** (unit)
87+
- **Given**: `store.data = [{ type: "bar", x: [1], y: [2] }]`.
88+
- **When**: mapped through `toPlotlyData(store.data)`.
89+
- **Then**: output deep-equals the input traces (identity mapping, correctly typed as
90+
`Data[]`) — no data dropped or reshaped.
91+
92+
- **B10 — onViewSelectChange keeps a stable identity across renders** (unit)
93+
- **Given**: `useV2EditorController` (and `useComposedEditorController`) rendered.
94+
- **When**: the component re-renders without `store`/`key` changing.
95+
- **Then**: the `onViewSelectChange` reference is unchanged between renders (memoized like
96+
`onEditorChange`).
97+
98+
- **B11 — single source of truth for active key in V2 controller** (unit)
99+
- **Given**: `useV2EditorController` rendered.
100+
- **When**: `onViewSelectChange` selects `"config"`, then trace index `0`.
101+
- **Then**: `viewSelectValue` and the editor code both follow the selected key with no
102+
desync; changing the key updates the displayed code (the MobX reaction still fires from
103+
the single retained key representation).
104+
105+
## Notes
106+
107+
- **B5** (`@types/jest` → devDependencies, drop from `tsconfig.build.json`) and **B7**
108+
(document the `set-state-in-effect` suppression): no unit test — verified by
109+
`pnpm --filter @mendix/shared-charts build` succeeding and code inspection. Track in tasks.md.
110+
- **B8** (silent `catch {}` in the editor controllers): **out of scope**, deferred to WC-3348.
111+
- **B10/B11**: if a memoization/identity assertion proves brittle in RTL, fall back to
112+
asserting observable behavior (no desync, correct code shown) rather than reference equality.
113+
- The `EditableChartStore` needs a `SetupComponentHost` + `ComputedAtom` test harness; check
114+
`@mendix/widget-plugin-mobx-kit` for an existing test helper before hand-rolling one.

‎packages/pluggableWidgets/custom-chart-web/openspec/changes/fix-rework-cleanup/proposal.md‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
## Why
2+
3+
The custom-chart / playground editor rework PR merged to `main` with an unresolved
4+
code review from Leonardo de Souza. Eleven Critical/Important/Minor findings shipped
5+
unfixed and are still live on `main` (re-verified 2026-07-06, WC-3488). The most
6+
visible impact:
7+
8+
- The playground sidebar's **"Modeler Layout"** and **"Modeler Configuration"** panels
9+
render empty — they read `layoutOptions` / `configOptions`, which the custom-chart
10+
widget hardcodes to `{}` (B3).
11+
- A `computed(fn).get()` misuse allocates a throwaway MobX atom on every render,
12+
defeating caching and providing no reactivity (B1).
13+
- The store that parses user JSON (`EditableChartStore`) has **zero unit tests** after
14+
a 402-line spec (`mergeChartProps.spec.ts`) was deleted with no replacement (B6).
15+
16+
The rest are correctness/maintainability debt: an unsound cast that hides invalid user
17+
JSON, a misplaced `@types/jest` runtime dependency, dead code, duplication, and
18+
undocumented lint suppressions.
19+
20+
## Root Cause
21+
22+
Per finding (all confirmed by file:line inspection, lines current on branch off
23+
`origin/main`):
24+
25+
- **B1** `custom-chart-web/src/hooks/useCustomChart.ts:54-62` — a plain object is wrapped
26+
in `computed((): PlaygroundData => ({...})).get()`, creating a new computed atom each
27+
render. `CustomChart.tsx` is already `observer`-wrapped, so reactivity is handled there.
28+
- **B2** `useCustomChart.ts:13-32,35,53,65` + `CustomChart.tsx:11` — `getContainerStyle`
29+
and the returned `containerStyle` are dead; `CustomChart.tsx` destructures only
30+
`{ playgroundData, ref }` and computes its own style via `constructWrapperStyle(props)`.
31+
- **B3** `useCustomChart.ts:59-60` — `layoutOptions: {}` / `configOptions: {}` are hardcoded.
32+
The controller host exposes `adapter: ChartPropsController` with `layout` / `config`
33+
getters that hold the real parsed values.
34+
- **B4** `custom-chart-web/src/controllers/CustomChartControllerHost.ts:34` — `store.data`
35+
(typed `Array<Record<string, unknown>>`) is cast `as Data[]`, suppressing the mismatch
36+
with Plotly's `Data` type.
37+
- **B5** `shared/charts/package.json:38` + `shared/charts/tsconfig.build.json:10` —
38+
`@types/jest` sits in `dependencies` and `"types": ["jest"]` is in the production build
39+
tsconfig, shipping test types as a runtime dependency.
40+
- **B6** `mergeChartProps.spec.ts` deleted (last seen in commit `e79bd67d9`); the only
41+
remaining custom-chart-web test is `src/utils/utils.spec.ts`. `EditableChartStore` (which
42+
handles user JSON edits) is untested.
43+
- **B7** `chart-playground-web/src/helpers/useComposedEditorController.ts:83` —
44+
`eslint-disable react-hooks/set-state-in-effect` with no explanation.
45+
- **B9** duplicate `prettifyJson` in `useComposedEditorController.ts:33-39` and
46+
`useV2EditorController.ts:32-38` (identical bodies).
47+
- **B10** `onViewSelectChange` is not memoized while `onEditorChange` is `useCallback`-wrapped,
48+
in both `useComposedEditorController.ts:44-51` and `useV2EditorController.ts:44-54`.
49+
- **B11** `useV2EditorController.ts:41-42` — the active editor key lives in both a React
50+
`useState` (`key`) and a MobX `observable.box` (`keyBox`): two sources of truth kept in
51+
sync by hand.
52+
53+
## What Changes
54+
55+
- **B1** Replace the `computed(...).get()` wrapper in `useCustomChart.ts` with a plain
56+
object literal.
57+
- **B2** Delete `getContainerStyle`, the `containerStyle` field on `UseCustomChartReturn`,
58+
and its return-value entry.
59+
- **B3** Destructure `adapter` from the controller host and pass `adapter.layout` /
60+
`adapter.config` as `layoutOptions` / `configOptions`.
61+
- **B4** Route `store.data` → Plotly `Data[]` through a single named boundary helper
62+
(e.g. `toPlotlyData`) instead of a bare `as Data[]`. No runtime validation added — the
63+
store already guards on write (`setDataAt` parses, type-checks, and warns on invalid
64+
JSON); Plotly's `Data` is a large union that is impractical to validate exhaustively.
65+
This localizes and documents the unavoidable type conversion in one place.
66+
- **B5** Move `@types/jest` to `devDependencies`; drop `"jest"` from `tsconfig.build.json`'s
67+
`types`.
68+
- **B6** Add a unit spec for `EditableChartStore` covering `setLayout` / `setConfig` /
69+
`setDataAt` / `reset`, the JSON getters, and invalid/out-of-range input.
70+
- **B7** Add a comment documenting why the `set-state-in-effect` suppression is safe.
71+
- **B9** Extract `prettifyJson` to one shared helper; import in both controllers.
72+
- **B10** Wrap `onViewSelectChange` in `useCallback` in both controllers.
73+
- **B11** Collapse `key` to a single source of truth in `useV2EditorController`.
74+
75+
## Impact
76+
77+
- **In scope:** `custom-chart-web` (hooks, controllers, `CustomChart.tsx`),
78+
`chart-playground-web` (helpers only — **not** `CodeEditor.tsx`), `shared/charts`
79+
(`EditableChartStore`, `package.json`, `tsconfig.build.json`).
80+
- **Out of scope (do not touch):** anything WC-3348 (PR #2310) delivered — `CodeEditor.tsx`,
81+
the editor restore, editor-side JSON lint. The silent empty-JSON `catch {}`
82+
(`useComposedEditorController.ts:75-76`, `useV2EditorController.ts:85-86`) is explicitly
83+
deferred to WC-3348 per the ticket; **B8 is not fixed here**.
84+
- **Not breaking.** No public widget prop, XML key, or MPK output changes. B3 restores an
85+
intended behavior (populated Modeler panels); B1/B2/B4/B9/B10/B11 are internal; B5 is a
86+
packaging fix; B6/B7 add tests/docs.
87+
- **Must not break:** live chart rendering and click events (B1/B2/B4), the V1 and V2
88+
playground editors' data/layout/config round-trip (B3/B9/B10/B11), and the
89+
`shared/charts` production build (B5).
90+
- Internal-only changes → **no changelog entry** expected for any of the three packages
91+
(confirm at PR time). Version bumps at release only.

‎packages/pluggableWidgets/custom-chart-web/openspec/changes/fix-rework-cleanup/tasks.md‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
## 1. Test Setup
2+
3+
<!-- RED: Write failing tests first. B6 first — it is the safety net for B1/B3/B4. -->
4+
5+
- [x] 1.1 Add `EditableChart.store.spec.ts` (B6): `reset` loads props; `setDataAt` valid
6+
index/valid JSON; out-of-range index no-op; invalid JSON swallowed + `console.warn`;
7+
non-object JSON rejected; `setLayout`/`setConfig` null-guard + replace; JSON getters.
8+
Find/reuse a `SetupComponentHost` + `ComputedAtom` harness from `widget-plugin-mobx-kit`.
9+
→ 8 tests, harness = `SetupHost` subclass + `{ get: () => box.get() }` atom.
10+
- [x] 1.2 Add `useCustomChart.spec.ts` (B1/B2/B3): playgroundData has real `layoutOptions`/
11+
`configOptions` from adapter (not `{}`); return shape is exactly `{ playgroundData, ref }`
12+
(no `containerStyle`); plotData stays reactive to `store.data` under an `observer`.
13+
→ reactivity asserted via an `observer` Probe + `waitFor` (setup autorun is post-render).
14+
Added `^src/` moduleNameMapper to widget jest.config.js (source uses baseUrl imports).
15+
- [x] 1.3 Add `prettifyJson.spec.ts` (B9): formats valid JSON 2-space; returns error object
16+
string on invalid.
17+
- [x] 1.4 Add controller tests (B4/B10/B11): `toPlotlyData` identity round-trip;
18+
`onViewSelectChange` stable identity across renders (both controllers); V2 key has no
19+
desync when switching layout→config→trace. → shared-charts barrel pulls plotly, so specs
20+
import `./stubObjectURL` first to shim `URL.createObjectURL` in the test env.
21+
22+
## 2. Implementation
23+
24+
<!-- GREEN: make tests pass with minimal code -->
25+
26+
- [x] 2.1 **B6** — implement/confirm `EditableChartStore` behavior so store spec passes (store
27+
already exists; adjust only if a test exposes a real defect — do not weaken tests).
28+
→ store behavior already correct; spec is characterization coverage, no source change.
29+
- [x] 2.2 **B1** — replace `computed((): PlaygroundData => ({...})).get()` in `useCustomChart.ts`
30+
with a plain object literal.
31+
- [x] 2.3 **B3** — destructure `adapter` from `CustomChartControllerHost`; pass `adapter.layout`
32+
/ `adapter.config` as `layoutOptions` / `configOptions`.
33+
- [x] 2.4 **B2** — delete `getContainerStyle`, the `containerStyle` field on
34+
`UseCustomChartReturn`, and its return entry. → also dropped now-unused `CSSProperties` import.
35+
- [x] 2.5 **B4** — add `toPlotlyData(data: Array<Record<string, unknown>>): Data[]` boundary
36+
helper; use it in `CustomChartControllerHost.viewModelAtom` instead of `as Data[]`.
37+
- [x] 2.6 **B9** — extract `prettifyJson` to one shared helper; import in both
38+
`useComposedEditorController.ts` and `useV2EditorController.ts`; remove duplicates.
39+
- [x] 2.7 **B10** — wrap `onViewSelectChange` in `useCallback` in both controllers.
40+
- [x] 2.8 **B11** — collapse `key`/`keyBox` in `useV2EditorController` to a single source of
41+
truth. → DEVIATION: kept the React `useState` `key` (it drives render; the box could not)
42+
and removed `keyBox`. The MobX `reaction` now keys off `key` via the effect deps and uses
43+
`{ fireImmediately: true }` to re-sync editor input on view switch. Removed `observable`/
44+
`runInAction` imports.
45+
46+
## 3. Refactoring
47+
48+
<!-- REFACTOR: clean up while keeping tests green. Non-test findings live here. -->
49+
50+
- [x] 3.1 **B5** — move `@types/jest` from `dependencies` to `devDependencies` in
51+
`shared/charts/package.json`; remove `"jest"` from `types` in `tsconfig.build.json`.
52+
- [x] 3.2 **B7** — add a comment above the `react-hooks/set-state-in-effect` disable in
53+
`useComposedEditorController.ts` explaining why it's safe (syncs editor input to external
54+
code changes; risk = overwriting in-progress edits, accepted).
55+
- [x] 3.3 Confirm B8 left untouched (out of scope, WC-3348); no stray edits to `CodeEditor.tsx`.
56+
57+
## 4. Verification
58+
59+
- [x] 4.1 All new tests passing.
60+
- [x] 4.2 Full test suite green for the three packages (custom-chart-web 33, chart-playground-web
61+
6, shared/charts 45) — no regressions. Lint clean on all changed files.
62+
- [x] 4.3 shared-charts build succeeds after B5 tsconfig change (`tsc -p tsconfig.build.json` + `pnpm build` both clean).
63+
- [ ] 4.4 Runtime check (optional, `/debug-widget`): playground Modeler Layout/Config panels
64+
populate (B3) in the WC-3348 test project.
65+
- [x] 4.5 Changelog: added a "Fixed" entry to custom-chart-web for the restored Modeler
66+
Layout/Configuration panels (B3, user-visible). All other findings internal → no entry.
67+
chart-playground-web + shared/charts: no changelog. Version bumps at release only.
68+
69+
## Notes
70+
71+
<!-- Track test failures, refactoring decisions, blockers. -->
72+
73+
- Order rationale: B6 store spec first = safety net before touching the B1/B3/B4 cluster that
74+
feeds the store into the chart/playground.
75+
- If B10/B11 identity assertions prove brittle in RTL, switch to behavior assertions per design.md.

‎packages/pluggableWidgets/custom-chart-web/openspec/config.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
schema: tdd-refactor

0 commit comments

Comments
 (0)