Skip to content

[WC-3348] Charts: restore highlighted JSON editor in playground#2310

Open
yordan-st wants to merge 9 commits into
mainfrom
fix/WC-3348-restore-playground-editor
Open

[WC-3348] Charts: restore highlighted JSON editor in playground#2310
yordan-st wants to merge 9 commits into
mainfrom
fix/WC-3348-restore-playground-editor

Conversation

@yordan-st

@yordan-st yordan-st commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Pull request type

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


Description

Charts v6.3.0 shipped the playground editor on CodeMirror, which threw a bundling error and broke the playground. As a stopgap (to unblock WC-3345), CodeMirror was replaced with a plain <textarea> in 6.3.1 — losing syntax highlighting and any feedback on invalid JSON.

This restores a real editor without reintroducing a CodeMirror-class bundling dependency. CodeEditor now uses react-simple-code-editor + highlight.js (JSON), adapting the lightweight pattern already used in rich-text-web. It adds inline invalid-JSON feedback (a sticky error banner above the editor) and keeps the existing prop contract (value / onChange / readOnly / height) so both playground panels work unchanged.

Scope is intentionally limited to the editor component. Leo's earlier review findings on the already-merged custom-chart / playground rework are tracked separately in WC-3488.

  • Added deps: react-simple-code-editor, highlight.js (tree-shaken core + JSON language only)
  • New unit tests for CodeEditor (highlighting, JSON lint, readOnly, height, no-CodeMirror guard)

What should be covered while testing?

Put a chart (e.g. Line chart) on a page, set Show playground slot = Yes, drop the Chart playground widget into the slot, configure a Series, and run the app (F5). Click Toggle Editor to open the sidebar.

  1. Syntax highlighting — the JSON in the editable "Custom settings" panel is colored (keys/strings/numbers), not plain text.
  2. Invalid JSON feedback — break the JSON (e.g. delete a }); a red error banner appears at the top of the editor. Fix it → the banner disappears.
  3. Live chart update — with the dropdown on Layout, set { "title": { "text": "TEST" } }; the chart title updates live.
  4. Read-only panel — the lower "Settings from the Studio Pro" panel is not editable.
  5. Keyboard — Tab inserts spaces inside the editor; Esc then Tab moves focus out.
  6. No regression — no CodeMirror bundling error in the browser console; the playground loads.

@yordan-st
yordan-st requested a review from a team as a code owner July 6, 2026 14:56
@yordan-st yordan-st changed the title [WC-3348]: restore highlighted JSON editor in charts playground [WC-3348] Charts: restore highlighted JSON editor in playground Jul 6, 2026
@github-actions

This comment has been minimized.

@yordan-st
yordan-st force-pushed the fix/WC-3348-restore-playground-editor branch from eea1883 to cd6e358 Compare July 8, 2026 12:10
@github-actions

This comment has been minimized.

@yordan-st
yordan-st force-pushed the fix/WC-3348-restore-playground-editor branch from cd6e358 to 653519e Compare July 10, 2026 13:10
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@iobuhov iobuhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Let's move the opensepc to "charts" package
  • Make sure we don't touch maps

@yordan-st
yordan-st force-pushed the fix/WC-3348-restore-playground-editor branch from 48a4c7e to 911fa17 Compare July 14, 2026 08:45
@github-actions github-actions Bot removed the maps-web label Jul 14, 2026
@yordan-st
yordan-st requested a review from iobuhov July 14, 2026 08:54
@github-actions

This comment has been minimized.

@yordan-st
yordan-st force-pushed the fix/WC-3348-restore-playground-editor branch from 2eb5f1b to b1fc11f Compare July 14, 2026 09:08
@github-actions

This comment has been minimized.

iobuhov
iobuhov previously approved these changes Jul 14, 2026

@iobuhov iobuhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yordan-st
yordan-st force-pushed the fix/WC-3348-restore-playground-editor branch from b1fc11f to 2c1ffb6 Compare July 21, 2026 12:33
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

…visible

  text

  The Editor's own root had a fixed height + overflow, which let the
  invisible textarea and the highlighted <pre> it overlays drift out of
  sync once content overflowed — causing unreachable scroll, misplaced
  caret, and wrong text selection. Move the fixed height/scroll to the
  outer wrapper and let the Editor's root grow with its content instead.
@yordan-st
yordan-st force-pushed the fix/WC-3348-restore-playground-editor branch from 12efb01 to 711200d Compare July 21, 2026 16:30
@github-actions

Copy link
Copy Markdown
Contributor

AI Code Review

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


What was reviewed

File Change
packages/pluggableWidgets/chart-playground-web/src/components/CodeEditor.tsx Replaced <textarea> with react-simple-code-editor + highlight.js; added JSON lint error banner and paste-caret fix
packages/pluggableWidgets/chart-playground-web/src/helpers/editorJson.ts New module — extracted prettifyJson and isEquivalentJson
packages/pluggableWidgets/chart-playground-web/src/helpers/useComposedEditorController.ts Added lastOwnEditRef to prevent store echo from clobbering the user's in-progress edit
packages/pluggableWidgets/chart-playground-web/src/helpers/useV2EditorController.ts Same echo-suppression pattern for MobX reaction path
packages/pluggableWidgets/chart-playground-web/src/ui/Playground.scss Added .widget-charts-playground-code-editor and its error banner styles
packages/pluggableWidgets/chart-playground-web/src/ui/Sidebar.scss Added flex layout to .widget-custom-config to clamp the editor to available height
packages/pluggableWidgets/chart-playground-web/src/components/__tests__/CodeEditor.spec.tsx New — 9 unit tests for CodeEditor
packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/useComposedEditorController.spec.ts New — echo-suppression test for V1 controller
packages/pluggableWidgets/chart-playground-web/src/helpers/__tests__/useV2EditorController.spec.ts New — echo-suppression test for V2 controller
packages/pluggableWidgets/chart-playground-web/CHANGELOG.md User-facing entry for restored editor
packages/pluggableWidgets/chart-playground-web/package.json Added highlight.js ^11.11.1 and react-simple-code-editor ^0.14.1
packages/shared/charts/src/main.ts Added EditorStore value export (was type-only)
packages/pluggableWidgets/maps-web/src/__tests__/Maps.spec.tsx Updated dynamic.available(…)dynamic(…) builder calls
packages/pluggableWidgets/maps-web/src/components/__tests__/LeafletMap.spec.tsx Same dynamic API update
packages/pluggableWidgets/chart-playground-web/openspec/** Design, proposal, tasks artifacts — out of scope for functional review

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

CI checks were not available in this environment; the findings below are based on static analysis only.


Findings

⚠️ Low — useComposedEditorController: lastOwnEditRef stores the fallback()-normalised string, not the raw user input

File: packages/pluggableWidgets/chart-playground-web/src/helpers/useComposedEditorController.ts line 67–70

Note: onEditorChange sets lastOwnEditRef.current = json where json = fallback(value). fallback converts "" and undefined to "{}" but otherwise returns value unchanged — so for a non-empty input json === value and there is no issue. However, the useEffect then compares code (the prettified store echo) against this ref via isEquivalentJson. For the V2 controller (lines 71–72 in useV2EditorController.ts), lastOwnEditRef.current = value (raw), which is the cleaner pattern. The asymmetry won't cause a bug in practice (since isEquivalentJson parses both sides), but a future reader may wonder why V1 stores the fallback-normalised form and V2 stores the raw input. Consider aligning them: store value (raw) in both, since isEquivalentJson already handles the comparison correctly regardless.


⚠️ Low — Error banner uses hardcoded hex colours instead of Atlas/SCSS variables

File: packages/pluggableWidgets/chart-playground-web/src/ui/Playground.scss lines 34–39

Note: The error banner uses color: #c00, background: #fdecea, and border-bottom: 1px solid #f5c6cb — all hardcoded. The rest of Playground.scss also uses hardcoded colours (pre-existing), so this is consistent with the file's current style, not a regression. It's worth noting for a future theming pass: if these were var(--mx-color-danger*) tokens they'd automatically pick up custom Atlas themes. Not blocking.


⚠️ Low — onPaste handler is attached to the container <div>, not the <textarea>, but assumes event.target instanceof HTMLTextAreaElement

File: packages/pluggableWidgets/chart-playground-web/src/components/CodeEditor.tsx lines 41–61

Note: This pattern is intentional (the event bubbles up from the textarea), and the instanceof guard makes it safe — any paste on the container that doesn't originate from the textarea is silently ignored. The code is correct. The only concern is that react-simple-code-editor renders a hidden <textarea> alongside a visible <pre>, and this pattern correctly targets the hidden textarea. Consider adding a brief comment noting that the handler is on the container to catch the bubbled event from the textarea, so a future reader doesn't mistakenly "fix" it by moving it onto <Editor> (which doesn't forward arbitrary onPaste props).


Positives

  • Echo-suppression pattern is correct and well-tested: Using a useRef to track the last own edit and skipping setInput when the store echoes the same content is a clean, non-hacky fix for the cursor-reset problem. Both V1 (useEffect path) and V2 (MobX reaction path) get the same treatment with dedicated regression tests.
  • isEquivalentJson handles the pretty-print discrepancy correctly: Comparing canonicalised JSON.stringify(JSON.parse(…)) output on both sides means reformatting (whitespace changes from the store's round-trip) correctly resolves as "equivalent" and the user's compact in-progress edit is preserved.
  • prettifyJson extracted into shared module: Moving the duplicate definition from both controllers into editorJson.ts is good housekeeping with no behaviour change.
  • Tree-shaken highlight.js import: Importing highlight.js/lib/core and registering only languages/json (not the full highlight.js bundle) keeps the widget's bundle footprint small — the right call for a Rollup-built widget.
  • role="alert" on the error banner: Correct ARIA usage — screen readers will announce JSON validation errors without polling.
  • CodeMirror guard as a package.json assertion test: Elegant way to enforce the "no CodeMirror" constraint without a runtime or build-time check that could bit-rot.
  • Paste caret fix is properly deferred via requestAnimationFrame: Correctly waits for the browser to apply the paste insertion before correcting the selection, and is thoroughly tested including the partial-paste case.

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.

2 participants