diff --git a/docs/architecture/03-modal-system.md b/docs/architecture/03-modal-system.md index ba4f157..5c97738 100644 --- a/docs/architecture/03-modal-system.md +++ b/docs/architecture/03-modal-system.md @@ -75,7 +75,10 @@ export interface ModalConfig { component: ComponentType; // the body rendered inside the shell /** Initialize transient modal state when it opens. `arg` carries an - * optional sub-target (e.g. a dataset id for chartBuilder/extract). */ + * optional sub-target (e.g. a dataset id for chartBuilder/datasets). OMIT + * when a service seeds the store *before* `openModal` — Extract is seeded by + * `services/extract-action` from the editor cursor, and an `init` here would + * re-read and clobber that view-scoped capture. */ init?: (arg?: string) => void; /** Serializable snapshot of in-progress edits, used to detect unsaved diff --git a/docs/architecture/07-naming-and-relationships.md b/docs/architecture/07-naming-and-relationships.md index 67a0769..b5546f9 100644 --- a/docs/architecture/07-naming-and-relationships.md +++ b/docs/architecture/07-naming-and-relationships.md @@ -164,35 +164,18 @@ export function libraryRefName(data: unknown, selfDefined: ReadonlySet): References appear in several places — top-level `data`, per-layer `data`, `data` inside `spec`/`facet`/`hconcat`/`vconcat`, and a lookup transform's `from.data`. -Rather than enumerate the grammar, each pass walks the spec recursively but -**prunes two keys**: a `data` object's payload (its `values`/rows) and the -top-level `datasets` map hold user data, not nested specs. Without the prune, a -data _row_ carrying a field literally named `data: { name: "x" }` is misread as a -reference. +Rather than enumerate the grammar, the walk recurses the spec but **prunes two +keys**: a `data` object's payload (its `values`/rows) and the top-level `datasets` +map hold user data, not nested specs. Without the prune, a data _row_ carrying a +field literally named `data: { name: "x" }` is misread as a reference. -```ts -// src/core/spec-refs.ts — the recursive walk; classification routes through spec-data - -export function extractDatasetRefs(spec: Json): string[] { - const root = typeof spec === 'string' ? safeParse(spec) : spec; // unparseable → no refs - const selfDefined = selfDefinedNames(root); - const names = new Set(); - const walk = (node: Json): void => { - if (Array.isArray(node)) return void node.forEach(walk); - if (node && typeof node === 'object') { - const obj = node as Record; - const refName = libraryRefName(obj.data, selfDefined); - if (refName !== null) names.add(refName); - for (const key of Object.keys(obj)) { - if (key === 'data' || key === 'datasets') continue; // prune user-data payloads - walk(obj[key]); - } - } - }; - walk(root); - return [...names]; -} -``` +That walk is **one shared pair** in `core/spec-data`, not re-implemented per pass: +`forEachDataBinding(spec, visit)` (read-only, `visit` returns `true` to stop early) +and `mapDataBindings(spec, mapData)` (returns a copy). Both compute the spec's +self-defined names once and hand them to the callback, so a caller writes only its +own rule — _what is a reference_ (`libraryRefName`) or _what to rewrite_ — never the +walk. `extractDatasetRefs` collects through `forEachDataBinding`; `renameDatasetInSpec` +and `promoteSelfDefinedDataset` (§3.2) rewrite through `mapDataBindings`. `recomputeDatasetRefs` is the thin sorted/de-duped wrapper stored on `snippet.datasetRefs`, run on every draft change and on publish. @@ -211,22 +194,50 @@ export function extractDatasetRefs(spec: Json): string[] { programmatic extract/revert rewrites, never on raw keystrokes. That keeps the links in step with the edited draft while never recomputing from a transiently-invalid spec. -- Keep the three ref walks in lockstep — extraction here, `renameDatasetInSpec`, - and the renderer's `resolveDatasetRefs` (`src/core/rendering.ts`). They agree - because they share both halves: the `libraryRefName` classifier (what is a - reference) and the prune of the **same two keys** (`data`, `datasets`). If one - classified differently, or descended into data payloads while another didn't, a - row field named `data` would get counted, rewritten, or throw - `DatasetNotFoundError`. +- Route every binding pass through the shared `forEachDataBinding` / + `mapDataBindings`. They share both halves that must agree — the `libraryRefName` + classifier (what is a reference) and the prune of the **same two keys** (`data`, + `datasets`) — so extraction, rename, and the reverse extraction cannot drift. If + one classified differently, or descended into data payloads while another didn't, + a row field named `data` would get counted, rewritten, or throw + `DatasetNotFoundError`. (The renderer's `resolveDatasetRefs` mutates in place and + can throw mid-walk, so it keeps its own copy of the walk — the one exception, held + in step by the same prune-two-keys rule.) **Don't** - Don't let two code paths each have their own idea of "referenced names". Renamer, ref-recomputer, and renderer must use the same walk shape. +- Don't add a fresh prune-walk for a new binding pass — reuse the shared pair. A + hand-written copy is one classifier tweak away from disagreeing with the others. - Don't "enumerate the grammar" (scope the walk to a fixed list of container keys) to fix the payload-descent problem — pruning the two data-bearing keys stays correct as Vega-Lite's composition grammar grows; an allow-list rots. +### 3.2 Extracting embedded data into a dataset — the reverse (`spec-inline-data.ts`, `spec-refs.ts`) + +Extract-to-Dataset is the inverse of a reference: it lifts a view's **embedded** +data into a stored dataset and rewrites the spec to reference it by name. It is a +cursor-scoped editor action — `services/extract-action` resolves the focused view's +binding (`dataBindingAtPath`), seeds the modal, then opens it; the gate +`specHasExtractableData` hides the toolbar action when no view carries liftable +data. Two embedded shapes lift, both routing through the same `spec-data` classifier +so they never disagree with reference detection: + +- **Inline `values`** — `inlineValuesOf` captures the payload verbatim (a CSV/TSV + string is kept as-is); confirm rewrites that view's `data` block, at its anchor + path, to `{ name }`. +- **A self-defined `datasets` entry** the view references — `selfDefinedPayloadOf` + reads the named rows and `promoteSelfDefinedDataset` drops the `datasets` entry + (and the map when it empties). Keeping the name needs no reference rewrite — it + un-shadows onto the new library dataset; renaming rewrites every matching + reference. This is the reverse direction of the self-defined-vs-library + precedence in §3.1. + +A `lookup` transform's inline `from.data` lifts like any view binding — +`dataBindingAtPath` finds it. A `url`, a generator, or an existing library +reference carries nothing to lift. + --- ## 4. Reverse lookup: who uses this dataset? diff --git a/docs/ux-second-pass.md b/docs/ux-second-pass.md index dca1515..5365196 100644 --- a/docs/ux-second-pass.md +++ b/docs/ux-second-pass.md @@ -8,6 +8,13 @@ record the resolution into the contract (`docs/architecture/09`+`10` and the rel ## Open +- **Extract-to-Dataset has no keyboard accelerator** — its sibling editor actions (wrap / + config, in `spec-transform-actions` / `spec-config-actions`) register an F1-palette command + and a lightbulb; Extract is toolbar-only (`runExtract` in `services/extract-action.ts`), + because it opens a modal rather than making an in-place undoable edit, so the palette/lightbulb + fit awkwardly. Decide whether to add a palette command anyway for parity (a keyboard path to + open the modal at the cursor), or leave toolbar-only. + - **Storage-full copy implies a per-tier budget, but quota is whole-origin** — the messages say "snippet storage is full" / "dataset storage is full" and tell the user to delete that entity's items, yet IndexedDB quota is shared across the whole origin. Per-tier framing is diff --git a/src/app/components/ExtractModal.tsx b/src/app/components/ExtractModal.tsx index 965ced7..e72ccf9 100644 --- a/src/app/components/ExtractModal.tsx +++ b/src/app/components/ExtractModal.tsx @@ -1,7 +1,7 @@ /** * Extract-to-Dataset — the modal body (spec §03F). * - * Shows a read-only preview of the active snippet draft's inline data and asks + * Shows a read-only preview of the focused view's embedded data and asks * for a dataset name. On confirm it saves the data as a new dataset and rewrites * the draft to reference it by name (logic in ExtractStore), then force-closes * (the commit is the user's confirmation, so no discard prompt). Cancel leaves @@ -38,13 +38,13 @@ export function ExtractModal() { }; if (!source) { - return

This snippet has no inline data to extract.

; + return

This snippet has no embedded data to extract.

; } return (

- Save this snippet’s inline data as a reusable dataset. The spec will be rewritten to + Save this snippet’s embedded data as a reusable dataset. The spec will be rewritten to reference it by name.

diff --git a/src/app/components/SpecEditor.tsx b/src/app/components/SpecEditor.tsx index aba8e37..4543198 100644 --- a/src/app/components/SpecEditor.tsx +++ b/src/app/components/SpecEditor.tsx @@ -26,7 +26,7 @@ import '../infrastructure/monaco-env'; // side-effect: wire workers before creat import { configureVegaLiteJson } from '../infrastructure/monaco-schema'; import { configureJsonFormatter, installFormatOnPaste } from '../infrastructure/monaco-format'; import { parseChartSpecText } from '@core/chart-builder'; -import { openChartBuilderForEdit, openModal } from '../modals/ModalCoordinator'; +import { openChartBuilderForEdit } from '../modals/ModalCoordinator'; import { installSpecConfigActions, runExtractConfig, @@ -41,10 +41,11 @@ import { runWrap, } from '../services/spec-transform-actions'; import { configureSpecDatasetHints } from '../services/spec-dataset-hints'; +import { runExtract } from '../services/extract-action'; import { useAppStore } from '../stores/AppStore'; import { confirm } from '../stores/ConfirmStore'; import { useDatasetStore } from '../stores/DatasetStore'; -import { hasInlineData } from '../stores/ExtractStore'; +import { hasExtractableData } from '../stores/ExtractStore'; import { publishActiveSnippet } from '../services/snippet-actions'; import { notify } from '../stores/NotificationStore'; import { usePreviewStore } from '../stores/PreviewStore'; @@ -222,10 +223,11 @@ function EditorToolbar({ const draft = s.editorView === 'draft' ? s.draftText : active.draftSpec; return draft !== active.spec; }); - // Offer Extract only when the live draft carries top-level inline data to lift - // out (spec §03F → hidden when the spec has no inline data). + // Offer Extract only when the live draft carries data to lift out — inline + // `values` in any view, or a reference to a self-defined `datasets` entry (spec + // §03F → hidden when there is nothing extractable). const canExtract = useSnippetStore( - (s) => s.activeSnippetId !== null && hasInlineData(s.draftText), + (s) => s.activeSnippetId !== null && hasExtractableData(s.draftText), ); // Offer "Open in builder" only when the active snippet's published spec is @@ -248,6 +250,14 @@ function EditorToolbar({ if (snippet) openChartBuilderForEdit(snippet); }; + // Extract is scoped to the view at the cursor (services/extract-action), so it + // goes through the editor handle like the wrap/config actions, not a bare + // openModal — the service captures the focused binding before opening the modal. + const handleExtract = () => { + const editor = editorRef.current; + if (editor) runExtract(editor); + }; + // 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; @@ -315,8 +325,8 @@ function EditorToolbar({ {canExtract && (