From be8cfc402dcf8770bbda39ee6bf22e2585048af5 Mon Sep 17 00:00:00 2001 From: Oleh Omelchenko Date: Sat, 20 Jun 2026 12:36:37 +0300 Subject: [PATCH] Chart Builder: open a snippet in the builder to edit it in place --- docs/IMPLEMENTATION-PLAN.md | 16 +- docs/architecture/03-modal-system.md | 7 + .../10-interaction-and-feedback.md | 16 + .../chart-builder-enhancement-scope.md | 30 +- docs/spec/06-chart-builder.md | 10 + .../components/ChartBuilderModal.module.css | 13 + src/app/components/ChartBuilderModal.tsx | 19 +- src/app/components/SpecEditor.tsx | 38 +- src/app/modals/ModalCoordinator.ts | 25 ++ src/app/stores/ChartBuilderStore.test.ts | 67 ++++ src/app/stores/ChartBuilderStore.ts | 81 ++++ src/app/stores/SnippetStore.test.ts | 46 +++ src/app/stores/SnippetStore.ts | 45 +++ src/core/chart-builder.test.ts | 285 ++++++++++++++ src/core/chart-builder.ts | 353 +++++++++++++++++- src/core/rendering.ts | 9 + 16 files changed, 1046 insertions(+), 14 deletions(-) diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index 33d3690..fe80ce8 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -515,11 +515,17 @@ is in the maintained plan, not the archive. **Chart Builder** (`exploration/chart-builder-enhancement-scope.md`): -- **Open-in-builder + builder starter examples (3B/3C)** — the next builder slice in the - scope doc: hydrate the **builder** from an existing spec (strict spec→config parse), behind - a starter-example front door. Distinct from the shipped onboarding gallery - (`core/examples.ts` → `Onboarding.tsx`), which seeds a snippet straight into the editor, not - the builder — there is no spec→builder-config path yet. +- ~~**Open-in-builder (3C, strict hydration)**~~ ✅ shipped (2026-06-18): an editor-toolbar + **Open in builder** action reopens a builder-representable snippet to **edit it in place** + (`parseChartSpec` is the strict inverse of `buildChartSpec`, gated by re-assemble + + deep-compare). Only dataset-referencing specs hydrate (the builder's data model). See the + scope doc status log. +- **Builder starter examples (3B)** — a small set of curated starters, one per covered FT + intent, **authored in the builder dialect so they reopen via 3C**. Reshaped by 3C's + data-model edge: a builder-openable starter must reference a dataset, so 3B ships paired + sample datasets (or is reframed) — decide its shape now that the gate is live. Distinct from + the shipped onboarding gallery (`core/examples.ts` → `Onboarding.tsx`), which seeds an + inline-data snippet straight into the editor (Monaco-only, never the builder). - ~~**Transform-aware data inspector** — show resolved post-transform rows~~ ✅ shipped: an Input | Resolved data inspector below the Live Preview and Chart Builder charts, with a draggable height divider (spec §04; arch 05 → "data inspector rides the boundary"). diff --git a/docs/architecture/03-modal-system.md b/docs/architecture/03-modal-system.md index e70e318..ba4f157 100644 --- a/docs/architecture/03-modal-system.md +++ b/docs/architecture/03-modal-system.md @@ -284,6 +284,13 @@ export function toggleDatasets(): void { } ``` +> **Pre-hydrate variant — open without `init`.** When a modal must open onto state the +> caller already loaded (the Chart Builder's _Open in builder_ edit flow — `openChartBuilderForEdit` +> hydrates the builder from a snippet first), the opener sets the active modal, snapshots, and +> syncs the URL **itself** and **skips the registry `init`** — running `init` would re-derive the +> default state and clobber the hydration. Such an opener is the exception, not the rule: ordinary +> opens go through `openModal` so `init` is the single place transient state is seeded. + ### Change detection ```ts diff --git a/docs/architecture/10-interaction-and-feedback.md b/docs/architecture/10-interaction-and-feedback.md index 93e41a6..9b96308 100644 --- a/docs/architecture/10-interaction-and-feedback.md +++ b/docs/architecture/10-interaction-and-feedback.md @@ -495,6 +495,22 @@ call the same functions as the visible control — one code path, two doors. _(Consulted via /council → NN/g #6/#7, Carbon menu-buttons/overflow-menu. This bullet is the contract; cite it, not the source.)_ +**Resolved — content-gated toolbar actions hide; state-gated actions disable.** Two ways a +toolbar action can be inapplicable, with opposite affordances. **State-gated** — applicable +to this object in principle, just inert right now (Revert with no draft changes, Config in +the read-only Published view) → **disabled**: the user can act (edit / switch view) and it +lights up. **Content-gated** — inapplicable to _this spec's shape_, and nothing the user can +do in the moment changes that (_Extract to Dataset_ needs inline data; _Open in builder_ +needs a builder-representable spec referencing an existing dataset) → **hidden**. A +permanently-disabled control the user cannot enable reads as broken or teasing, not as +guidance (NN/g #6 — a disabled state must imply "do X and this becomes available"; Carbon +button states). So _Open in builder_ (spec §06) sits in the editor toolbar beside _Extract +to Dataset_ and follows its visibility — present only when the active snippet round-trips +through the builder and its dataset exists. This refines "disabled is for temporarily +unavailable actions" (below) from the unbuilt-feature case to the per-spec case. +_(Consulted via /council → NN/g #4/#6, Carbon button usage/states. This bullet is the +contract; cite it, not the source.)_ + **Resolved — field→channel assignment: explicit choice, visible armed state.** Clicking a shelf field with no channel armed opens an explicit **channel chooser** (the channels that accept the field; an occupied one is labelled with what it replaces) — never a silent diff --git a/docs/exploration/chart-builder-enhancement-scope.md b/docs/exploration/chart-builder-enhancement-scope.md index 4a5f266..6723efb 100644 --- a/docs/exploration/chart-builder-enhancement-scope.md +++ b/docs/exploration/chart-builder-enhancement-scope.md @@ -25,6 +25,32 @@ Newest first. The at-a-glance build-order tracker is §4; per-item detail is §3. This log is the quick "where are we" — read it first. +- **2026-06-18 (3C open-in-builder, edit-in-place)** — **the builder became create-and-revise.** + Core: `parseChartSpec` / `parseChartSpecText` (`chart-builder.ts`) — the strict inverse of + `buildChartSpec`, gated by **re-assemble + canonical deep-compare** (ignoring key order, + `$schema`, the injected `mark.tooltip`), so the gate widens for free as the dialect grows and + never lets the builder overwrite a richer spec. `unescapeVegaField` added beside its escape + twin (`rendering.ts`). Save semantics chosen with the user = **edit in place** (not + always-create): `SnippetStore.replaceSnippetSpec` republishes the built spec into both the + snippet's versions (auto names re-derive like publish; user names kept), and + `ChartBuilderStore` gained `editingSnippetId`/`editingSnippetName` + `openForEdit`/`saveEdits`. + Entry point: an **editor-toolbar** _Open in builder_ action (`SpecEditor`), beside _Extract to + Dataset_, shown only when the published spec round-trips _and_ its dataset exists — **hidden + otherwise** (content-gated, like Extract). The modal's primary becomes **Save changes** with an + "Editing _name_" banner; a dedicated `openChartBuilderForEdit` coordinator opener hydrates + before showing the modal so the registry's default `init` can't clobber it. **Data-model edge + (called out, not a bug):** the builder references a dataset by name, so only dataset-referencing + specs hydrate — inline-data snippets (incl. the onboarding gallery) stay Monaco-only. This + reshapes **3B**: builder-openable starters need a paired dataset, so 3B's shape is decided + _after_ this lands (see §3). Spec §06 gained an _Open in builder_ section. **Placement settled + by council:** an initial library metadata-panel placement (beside Duplicate/Delete) tested + undiscoverable; `/council` (NN/g #6 recognition / #4 consistency; Carbon button usage) put it in + the editor toolbar and settled the hidden-vs-disabled question via the toolbar's own precedent — + **content-gated** actions hide (Extract, Open-in-builder), **state-gated** ones disable (Revert, + Config). Recorded as a contract rule in `architecture/10` §5; the parked `ux-second-pass.md` row + is closed. Verified: typecheck + full tests (1089) + eslint + prettier. **Owed:** a visual/manual + pass on the live edit flow (the toolbar action, the "Editing" banner, Save changes). + - **2026-06-13 (guidance: reason over role, not raw type)** — closed a **false-positive class** in `builderWarnings` (eng-council + council consult). A histogram (bar, binned-Q X, count Y) tripped "two measures → scatter" because `isMeasureMapping`/`effectiveType` ignored `bin`. @@ -599,8 +625,8 @@ Phase 2 2A value-or-field channels (Property model) ✓ done 2B field shelf + in-place type cycling ✓ done (field-first + on-chart shelves) Phase 3 3D entry points & discoverability ✓ done (2026-06-12) 3A intent-first front door (Tier C) ✓ done (2026-06-13); persistent strip - 3B starter examples ← next; pairs with 3C - 3C open in builder (strict hydration) ← added 2026-06-11; pairs with 3B + 3C open in builder (strict hydration) ✓ done (2026-06-18); edit-in-place + 3B starter examples ← next; now hydrates via 3C Marks +rect (Heatmap) ✓ done (2026-06-13) Phase 4 (gated) theta/facets/styling-overrides/undo/lookup — decide after Phase 3 diff --git a/docs/spec/06-chart-builder.md b/docs/spec/06-chart-builder.md index 2c47943..a2c16cb 100644 --- a/docs/spec/06-chart-builder.md +++ b/docs/spec/06-chart-builder.md @@ -164,6 +164,16 @@ Selecting "Create Snippet" produces the final artifact: - Links the snippet to the dataset by recording the dataset reference, so the bidirectional snippet↔dataset relationship is established (see _Datasets_). - Closes the builder; the newly created snippet becomes the active snippet in the library/editor. **No success toast** — the result is immediately visible (the new snippet opens in the editor), so a toast would be noise (architecture 10 §1, "toast only what the user can't already see"). This refines the earlier blanket "every action toasts" rule, consistent with the Extract-to-dataset / publish reconciliation. +## Open in builder (edit in place) + +The builder is also the way to **revise** a chart it could have produced — not only create one. The **editor toolbar** (top of the Spec Editor, alongside the Draft/Published toggle, Config, Revert, and Publish) offers an **Open in builder** action that reopens the active snippet in the builder, populated from its spec — the visual counterpart to editing the same chart's JSON, placed where that editing happens. + +- **When it is offered.** Only when the active snippet's **published spec** is **losslessly representable** in the builder's dialect _and_ the dataset it references still exists. Losslessness is judged by parsing the spec back to a builder configuration and **re-assembling it for an exact comparison** against the original (ignoring key order, the `$schema` stamp, and the builder's injected `tooltip`), not by enumerating supported features — so the gate stays correct automatically as the builder's dialect grows. A spec the builder cannot reproduce exactly (hand-authored richness, an unsupported channel/mark, inline `data.values`, a `url` source) stays **Monaco-only**, and the action is **hidden** for it — the same content-gated treatment as _Extract to Dataset_ (a permanently-disabled control the user can't enable in the moment would read as broken; see _Interaction & Feedback_ → action visibility). The builder references a dataset **by name** — its sole data model — so only a snippet that references a saved dataset can hydrate. +- **What opening does.** Hydrates the builder from the snippet — mark, encodings, transforms, sort/stack, title/subtitle, size — and loads the referenced dataset's columns. The configuration pane names the snippet under edit ("Editing _name_"), and the dataset picker behaves as a **rebase** (built-on work) for any subsequent dataset switch — a loaded chart is never treated as a fresh default. +- **An edit session is builder-local, not part of the URL.** Like the builder's in-progress configuration generally, the "editing _name_" context is transient: a reload, Back, or shared link reopens the builder as a fresh **create** flow on the same dataset rather than restoring the edit. No data is lost — the published snippet is untouched until _Save changes_ — and re-entering the edit is one click from the toolbar. +- **Saving.** The primary action becomes **Save changes** (in place of "Create Snippet"): it **republishes** the built spec into the same snippet — overwriting **both** its published and draft versions, so there is no pending draft to reconcile — while keeping the snippet's identity, timestamps' `created`, and dataset links. A user-chosen name is preserved; an auto-named snippet re-derives its name from the new content (as _Publish_ does). The edited snippet becomes the active snippet. As with Create, there is no success toast (the result is immediately visible) and the action is gated on the same validation (at least one channel bound). +- **The JSON stays the document.** Because Open-in-builder is strict, the builder never silently overwrites a richer spec it cannot represent — it is a view that emits the spec, never a competing source of truth. + ## Closing - The builder can be dismissed without creating anything (close control / modal dismissal). diff --git a/src/app/components/ChartBuilderModal.module.css b/src/app/components/ChartBuilderModal.module.css index d56a9c2..d26db5b 100644 --- a/src/app/components/ChartBuilderModal.module.css +++ b/src/app/components/ChartBuilderModal.module.css @@ -58,6 +58,19 @@ gap: var(--space-3); } +/* Edit-in-place banner (spec §06 → Open in builder): names the snippet the builder + is editing, so the primary "Save changes" action reads unambiguously. */ +.editingBanner { + margin: 0; + font-size: 12px; + color: var(--text-secondary); +} + +.editingBanner strong { + color: var(--text-primary); + font-weight: 600; +} + /* ── Intent front door (spec §06 → Intent) ─────────────────────────────── A persistent "what do you want to show?" strip under the dataset picker. A quiet accent-soft wash marks it as the guided on-ramp without competing with diff --git a/src/app/components/ChartBuilderModal.tsx b/src/app/components/ChartBuilderModal.tsx index 5e8dbd1..73ddd88 100644 --- a/src/app/components/ChartBuilderModal.tsx +++ b/src/app/components/ChartBuilderModal.tsx @@ -1466,6 +1466,12 @@ export function ChartBuilderModal() { const setStack = useChartBuilderStore((s) => s.setStack); const applyWarningFix = useChartBuilderStore((s) => s.applyWarningFix); const runCreate = useChartBuilderStore((s) => s.createSnippet); + const runSave = useChartBuilderStore((s) => s.saveEdits); + // Edit-in-place (spec §06 → Open in builder): when a snippet was opened in the + // builder, the primary action *saves back* to it instead of creating a new snippet. + const editingSnippetId = useChartBuilderStore((s) => s.editingSnippetId); + const editingSnippetName = useChartBuilderStore((s) => s.editingSnippetName); + const editing = editingSnippetId !== null; // Validity + guidance + which chart-level controls apply are derived from the // stable `config` reference via useMemo, NOT a store selector that would build a @@ -1526,6 +1532,11 @@ export function ChartBuilderModal() {
{fixAnnouncement}
+ {editing && ( +

+ Editing {editingSnippetName} +

+ )} @@ -1610,7 +1621,7 @@ export function ChartBuilderModal() { {!valid && (

- Map at least one channel to a column to create a snippet. + Map at least one channel to a column to {editing ? 'save changes' : 'create a snippet'}.

)}
@@ -1622,12 +1633,12 @@ export function ChartBuilderModal() { size="lg" disabled={!valid} aria-describedby={!valid ? 'cb-create-hint' : undefined} - // The create is the user's confirmation — close with no discard prompt. + // The commit is the user's confirmation — close with no discard prompt. onClick={() => { - if (runCreate()) void closeModal(true); + if (editing ? runSave() : runCreate()) void closeModal(true); }} > - Create Snippet + {editing ? 'Save changes' : 'Create Snippet'}
diff --git a/src/app/components/SpecEditor.tsx b/src/app/components/SpecEditor.tsx index 6e68579..454ba9e 100644 --- a/src/app/components/SpecEditor.tsx +++ b/src/app/components/SpecEditor.tsx @@ -13,7 +13,8 @@ * inline near the editor (spec §03E), mirroring the preview via PreviewStore. */ -import { useEffect, useRef, type RefObject } from 'react'; +import { useEffect, useMemo, useRef, type RefObject } from 'react'; +import { useShallow } from 'zustand/react/shallow'; // `edcore.main` is the full standalone editor — every feature contribution // (folding, suggest widget, word operations like Cmd+Backspace, find, bracket // colorization, multi-cursor, …) — but WITHOUT the `monaco-editor` barrel's @@ -24,7 +25,8 @@ import 'monaco-editor/esm/vs/language/json/monaco.contribution'; import '../infrastructure/monaco-env'; // side-effect: wire workers before create import { configureVegaLiteJson } from '../infrastructure/monaco-schema'; import { configureJsonFormatter, installFormatOnPaste } from '../infrastructure/monaco-format'; -import { openModal } from '../modals/ModalCoordinator'; +import { parseChartSpecText } from '@core/chart-builder'; +import { openChartBuilderForEdit, openModal } from '../modals/ModalCoordinator'; import { installSpecConfigActions, runExtractConfig, @@ -33,6 +35,7 @@ import { } from '../services/spec-config-actions'; import { useAppStore } from '../stores/AppStore'; import { confirm } from '../stores/ConfirmStore'; +import { useDatasetStore } from '../stores/DatasetStore'; import { hasInlineData } from '../stores/ExtractStore'; import { publishActiveSnippet } from '../services/snippet-actions'; import { notify } from '../stores/NotificationStore'; @@ -193,6 +196,26 @@ function EditorToolbar({ (s) => s.activeSnippetId !== null && hasInlineData(s.draftText), ); + // Offer "Open in builder" only when the active snippet's published spec is + // losslessly representable in the builder dialect AND its referenced dataset + // exists — the gate the builder's openForEdit enforces (spec §06 → Open in + // builder). Content-gated like Extract, so it is **hidden** when inapplicable + // (the toolbar's convention for content-gated actions — vs Revert/Config, which + // disable because they are merely state-gated; arch 10 §5). `datasetNames` is a + // shallow-stable string[] so the memo doesn't churn (MEMORY → stable selectors). + const activeSpec = useSnippetStore((s) => selectActiveSnippet(s)?.spec ?? null); + const datasetNames = useDatasetStore(useShallow((s) => s.datasets.map((d) => d.name))); + const canOpenInBuilder = useMemo(() => { + if (activeSpec === null) return false; + const config = parseChartSpecText(activeSpec); + return config !== null && datasetNames.includes(config.datasetName); + }, [activeSpec, datasetNames]); + + const handleOpenInBuilder = () => { + const snippet = selectActiveSnippet(useSnippetStore.getState()); + if (snippet) openChartBuilderForEdit(snippet); + }; + // Publish + its success toast live in one place (services/snippet-actions) so // the button and the Cmd/Ctrl+S shortcut (EventRouter) behave identically. const handlePublish = publishActiveSnippet; @@ -239,6 +262,17 @@ function EditorToolbar({ to two lines and Revert/Publish stay on one row. The label rides in `title` (and the accessible name) when only the icon shows. Publish — the one primary action — keeps its label at every width. */} + {canOpenInBuilder && ( + + )} {canExtract && (