Skip to content

[WC-3345] fix: custom chart store#2165

Merged
iobuhov merged 9 commits intomainfrom
3345/charts-fix
May 7, 2026
Merged

[WC-3345] fix: custom chart store#2165
iobuhov merged 9 commits intomainfrom
3345/charts-fix

Conversation

@iobuhov
Copy link
Copy Markdown
Collaborator

@iobuhov iobuhov commented Apr 7, 2026

Pull request type

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


Description

Switch to central store.

@iobuhov iobuhov requested a review from a team as a code owner April 7, 2026 16:02
@gjulivan gjulivan force-pushed the 3345/charts-fix branch 2 times, most recently from 25fdd9d to fc284e9 Compare April 21, 2026 12:41
gjulivan
gjulivan previously approved these changes Apr 21, 2026
leonardomendix
leonardomendix previously approved these changes May 6, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

AI Code Review

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


What was reviewed

File Change
packages/shared/charts/src/model/stores/EditableChart.store.ts New MobX store for editable chart state (layout/config/data as objects)
packages/shared/charts/src/helpers/playground-context.ts Adds PlaygroundDataV1/V2 union type and exports
packages/shared/charts/src/main.ts Exports EditableChartStore and new types
packages/pluggableWidgets/custom-chart-web/src/controllers/CustomChartControllerHost.ts Wires up EditableChartStore into controller host
packages/pluggableWidgets/custom-chart-web/src/controllers/ChartPropsController.ts Removes mergeChartProps, renames ChartPropsPlotlyChartProps
packages/pluggableWidgets/custom-chart-web/src/controllers/PlotlyController.ts Uses renamed PlotlyChartProps
packages/pluggableWidgets/custom-chart-web/src/hooks/useCustomChart.ts Builds PlaygroundDataV2 payload using central store
packages/pluggableWidgets/custom-chart-web/src/CustomChart.tsx Wraps component in observer HOC
packages/pluggableWidgets/custom-chart-web/src/components/PlotlyChart.ts Renames interface, removes unused width/height fields
packages/pluggableWidgets/custom-chart-web/src/utils/utils.ts Removes mergePlaygroundState/mergeChartProps
packages/pluggableWidgets/chart-playground-web/src/components/Playground.tsx Adds V1/V2 dispatch via EditorGen1/EditorGen2
packages/pluggableWidgets/chart-playground-web/src/helpers/useV2EditorController.ts New hook: drives ComposedEditor from EditableChartStore via MobX reaction
packages/pluggableWidgets/chart-playground-web/src/helpers/useComposedEditorController.ts Adds local input state to decouple typing from store writes
packages/pluggableWidgets/chart-playground-web/src/components/CodeEditor.tsx Replaces CodeMirror with plain <textarea>
packages/pluggableWidgets/chart-playground-web/src/components/ComposedEditor.tsx Switches from defaultEditorValue to controlled value prop
packages/pluggableWidgets/charts-web/CHANGELOG.md Documents CodeMirror removal
Deleted: mergeChartProps.spec.ts, mergePlaygroundState.ts, mergeChartProps.spec.ts.snap Removes tests and utilities that no longer exist

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


Findings

⚠️ Low — computed().get() called inline in render — creates a new computed each render

File: packages/pluggableWidgets/custom-chart-web/src/hooks/useCustomChart.ts line 54–62
Note: computed(() => ({ ... })).get() is called directly in the render path on every render, which creates a throwaway ComputedAtom each time and defeats MobX memoization. Since Container is already an observer, reading store properties directly (or via a useComputed/stable ref) is sufficient. The pattern also carries no reactivity benefit since the atom is discarded immediately.
Fix:

// Option A: read store props directly (observer tracks them automatically)
const playgroundData: PlaygroundData = {
    type: "editor.data.v2",
    store,
    plotData: store.data,
    layoutOptions: {},
    configOptions: {}
};

// Option B: use useMemo if reference-stability of the object matters
const playgroundData = useMemo<PlaygroundData>(
    () => ({ type: "editor.data.v2", store, plotData: store.data, layoutOptions: {}, configOptions: {} }),
    [store]
);

⚠️ Low — MobX observable.box created with stale initial value

File: packages/pluggableWidgets/chart-playground-web/src/helpers/useV2EditorController.ts line 42
Note: useState(() => observable.box<ConfigKey>(key))[0] initialises the box with the value of key at first render (always "layout"). This is intentional (the box and the React state are kept in sync via onViewSelectChange), but the relationship is subtle: forgetting runInAction(() => keyBox.set(newKey)) in onViewSelectChange would silently desync the reaction. Consider deriving keyBox from a ref or a single source of truth to make the coupling explicit.


⚠️ Low — Inline style on <textarea> contradicts styling guidelines

File: packages/pluggableWidgets/chart-playground-web/src/components/CodeEditor.tsx line 15
Note: The SCSS/Atlas UI guidelines say to avoid inline styles for static design properties. The CHANGELOG notes this is a deliberate compatibility downgrade, so it is understandable in context, but the static fontFamily: "monospace" and width: "100%" should move to the existing Playground.scss or a scoped class (e.g. .widget-chart-playground-code-editor) to stay consistent with the repo conventions.


⚠️ Low — Missing CHANGELOG entries for custom-chart-web and chart-playground-web

File: packages/pluggableWidgets/custom-chart-web/CHANGELOG.md, packages/pluggableWidgets/chart-playground-web/CHANGELOG.md
Note: Both packages have empty [Unreleased] sections. custom-chart-web received a meaningful behaviour change (playground now mutates layout/config/data through a central store instead of a local merge step), and chart-playground-web switched from CodeMirror to a plain textarea. Run pnpm -w changelog or add entries manually.


Positives

  • EditableChartStore correctly uses makeAutoObservable with explicit per-member overrides, matching the MobX patterns used elsewhere in the repo.
  • The V1/V2 dispatch in Playground.tsx uses Object.hasOwn rather than in or property access, which is the right way to narrow before casting.
  • autorun in EditableChartStore.setup() is properly returned via disposeBatch, preventing memory leaks on unmount.
  • Deleting mergeChartProps together with its test file keeps the test suite honest — no orphaned tests.
  • The reaction-based sync in useV2EditorController correctly returns the disposer from useEffect, so the reaction is cleaned up when the component unmounts.

@iobuhov iobuhov merged commit 21210d8 into main May 7, 2026
16 of 19 checks passed
@iobuhov iobuhov deleted the 3345/charts-fix branch May 7, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants