diff --git a/docs/architecture/07-naming-and-relationships.md b/docs/architecture/07-naming-and-relationships.md index c69bf78..7ada1a1 100644 --- a/docs/architecture/07-naming-and-relationships.md +++ b/docs/architecture/07-naming-and-relationships.md @@ -132,9 +132,14 @@ always mirrors the dataset names actually referenced in the published spec. ### 3.1 Extracting referenced names from a spec (pure — `src/core/spec-refs.ts`) A Vega-Lite spec can reference named data in several places: the top-level -`data`, per-layer `data`, `data` inside `spec`/`facet`/`hconcat`/`vconcat`, and -named entries in top-level `datasets`. Rather than enumerate Vega-Lite's grammar, -we walk the spec recursively and collect every `{ data: { name } }` we find. +`data`, per-layer `data`, `data` inside `spec`/`facet`/`hconcat`/`vconcat`, and a +lookup transform's `from.data`. A spec may also define its OWN inline datasets via +a top-level `datasets` map — those are self-defined, not library references. +Rather than enumerate Vega-Lite's grammar, we walk the spec recursively and +collect every `{ data: { name } }` — but **prune two keys**: never recurse into a +`data` object's payload (its `values`/rows) or the top-level `datasets` map, +because those 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. This is pure, deterministic, and the most heavily unit-tested function here. ```ts @@ -142,8 +147,10 @@ This is pure, deterministic, and the most heavily unit-tested function here. type Json = unknown; -/** Collects every dataset name referenced by `{ data: { name } }` anywhere in the spec. */ +/** Collects every library dataset name referenced by `{ data: { name } }`, excluding self-defined ones. */ export function extractDatasetRefs(spec: Json): string[] { + const root = typeof spec === 'string' ? safeParse(spec) : spec; + const selfDefined = selfDefinedNames(root); // names from the spec's own top-level `datasets` const names = new Set(); const walk = (node: Json): void => { @@ -155,13 +162,17 @@ export function extractDatasetRefs(spec: Json): string[] { const obj = node as Record; const data = obj.data as Record | undefined; if (data && typeof data === 'object' && typeof data.name === 'string') { - names.add(data.name); + if (!selfDefined.has(data.name)) names.add(data.name); + } + // Prune: a `data` payload and the `datasets` map hold user data, not refs. + for (const key of Object.keys(obj)) { + if (key === 'data' || key === 'datasets') continue; + walk(obj[key]); } - for (const key of Object.keys(obj)) walk(obj[key]); } }; - walk(typeof spec === 'string' ? safeParse(spec) : spec); + walk(root); return [...names]; } @@ -193,11 +204,20 @@ export function recomputeDatasetRefs(spec: Json): string[] { agreeing with what the renderer actually resolves. - Recompute and store `datasetRefs` on **publish**, not on every keystroke — the draft can be transiently invalid, and only the published spec is shared. +- Prune the **same two keys** (`data`, `datasets`) in all three ref walks — + extraction here, `renameDatasetInSpec`, and the renderer's `resolveDatasetRefs` + (`src/core/rendering.ts`). They must agree on what counts as a reference; if one + descends into data payloads and another doesn't, extraction and rendering + disagree and a row field named `data` either gets counted, rewritten, or throws + `DatasetNotFoundError`. **Don't** - Don't let two code paths each have their own idea of "referenced names". - Renamer and ref-recomputer must use the same extractor. + Renamer, ref-recomputer, and renderer must use the same walk shape. +- 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. --- @@ -323,19 +343,21 @@ The spec rewrite is pure; the orchestration reads and writes stores. /** Returns a copy of `spec` with every data.name === oldName replaced by newName. */ export function renameDatasetInSpec(spec: Json, oldName: string, newName: string): Json { const obj = typeof spec === 'string' ? safeParse(spec) : spec; + const selfDefined = selfDefinedNames(obj); // never rename a spec's own inline dataset name const rewrite = (node: Json): Json => { if (Array.isArray(node)) return node.map(rewrite); if (node && typeof node === 'object') { const out: Record = {}; for (const [k, v] of Object.entries(node as Record)) { - if ( - k === 'data' && - v && - typeof v === 'object' && - (v as Record).name === oldName - ) { - out[k] = { ...(v as object), name: newName }; + // A `data` object is a reference site: rename a matching name, but never + // recurse into its payload. `datasets` (self-defined inline data) is left + // whole. Same prune as extractDatasetRefs — see §3.1. + if (k === 'data' && v && typeof v === 'object' && !Array.isArray(v)) { + const dv = v as Record; + out[k] = dv.name === oldName && !selfDefined.has(oldName) ? { ...dv, name: newName } : dv; + } else if (k === 'data' || k === 'datasets') { + out[k] = v; } else { out[k] = rewrite(v); } diff --git a/docs/architecture/10-interaction-and-feedback.md b/docs/architecture/10-interaction-and-feedback.md index 4d1b72a..0be59bd 100644 --- a/docs/architecture/10-interaction-and-feedback.md +++ b/docs/architecture/10-interaction-and-feedback.md @@ -43,6 +43,28 @@ nature of the message, not by convenience. consent-for-destruction in a toast. - **Errors persist; success fades.** An error/warning toast waits for the user (a critical message must not vanish on a timer); success/info auto-dismiss (~6s). +- **Toasts sit bottom-right**, not top-right. Carbon's default is the top, but our header's + action cluster (Publish/Revert, theme/datasets) lives top-right — a toast there covers the + control the user just used. Bottom-anchored, the stack grows upward with the newest toast + nearest the corner, and still clears the centered confirm dialog. (`Toaster.module.css`.) +- **Toast copy: title states it, message adds to it.** Every toast renders a `title` and a + `message`. The title is the short headline — the action or what stopped, **no terminal + period** ("Snippet published", "Storage full"). The message is **one short sentence that + must not paraphrase the title** (Carbon, `components/notification/usage.mdx` §Body + content: _"Don't repeat or paraphrase the title"_); it carries the **consequence** for a + success ("Your draft is now the published version") or the **next step** for a fixable + error. Name the specific item in the message when toasts can stack — a delete confirms + _which_ one went ("…removed `\"Sales\"`…"). +- **Toast only what the user can't already see.** A success toast is for an outcome with no + strong on-screen cue: a **side effect** (Extract creates a dataset off-screen while the + user is in the editor), a **disappearance** (delete), or a **state flip** (publish/revert). + An action whose result is immediately visible — a created snippet opening in the editor, a + new dataset shown selected — is confirmed by that visible change; adding a toast is noise + (NN/g aesthetic-and-minimalist; Carbon `notification/usage.mdx` "Deciding what to use"). + An **invisible** outcome that still shouldn't toast is **copy-to-clipboard**: confirm it + _inline on the control_ ("Copied") with a polite `aria-live` announcement for assistive + tech, never a toast-per-copy. This refines the spec's earlier blanket "every action + toasts" (spec §01F/§02/§05, reconciled). - **The same failure can light up two channels.** An unrenderable spec shows the _same_ message inline in both the editor (§03E) and the preview (§04) — one producer (`PreviewStore`), two subscribers. That's intentional, not duplication. diff --git a/docs/spec/01-application-shell.md b/docs/spec/01-application-shell.md index 616e313..4be85b5 100644 --- a/docs/spec/01-application-shell.md +++ b/docs/spec/01-application-shell.md @@ -99,13 +99,17 @@ Transient toast messages appear in a corner of the screen to confirm actions or - Each toast auto-dismisses after a few seconds, and can also be dismissed manually via its close control. - Multiple toasts stack rather than replacing one another, and appear/disappear with a brief fade. +Toasts confirm outcomes the user **cannot already see** — a side effect, a disappearance, or a state change with no strong on-screen cue. An action whose result is immediately visible (a newly created snippet opening in the editor; a new dataset shown selected in its detail pane) is confirmed by that visible change, not by an added toast, which would only be noise (Nielsen Norman "aesthetic and minimalist design"; Carbon notification usage; see _docs/architecture/10 → Toast copy_). Assistive-technology users still receive an announcement on the changed region. + Events that raise toasts include: -- Snippet actions: creating, duplicating, deleting, publishing a draft, and reverting/discarding a draft. -- Dataset actions: creating, deleting, and extracting inline data into a dataset. +- Snippet actions: **duplicating** (the copy is easily mistaken for an edit of the original), **deleting** (the snippet disappears), **publishing** a draft, and **reverting** a draft. _Creating_ a snippet opens it in the editor and needs no toast. +- Dataset actions: **deleting**, and **extracting** inline data into a dataset (the dataset is created off-screen while the user is in the editor). _Creating_ a dataset in the Datasets modal is confirmed by the new dataset appearing selected. - Import/Export: results of an import (success/partial/failure) and confirmation of an export. - Errors: spec/data validation failures and local-storage capacity warnings. +**Copy Reference** (Datasets) is the exception that proves the rule: a clipboard write is invisible, but the universal pattern confirms it _inline on the control_ ("Copied"), announced politely to assistive tech — a toast per copy would be noise. + ## G. Offline & Installable Astrolabe is local-first and usable without a network connection. diff --git a/docs/spec/02-snippet-library.md b/docs/spec/02-snippet-library.md index fcc2627..3288653 100644 --- a/docs/spec/02-snippet-library.md +++ b/docs/spec/02-snippet-library.md @@ -57,9 +57,9 @@ When a snippet is active, a metadata panel (within the left pane) exposes its ed ## Snippet Operations -The library provides the lifecycle operations for snippets. Each operation gives clear feedback via a toast notification (see _Application Shell & Navigation_). +The library provides the lifecycle operations for snippets. An operation whose outcome the user can't already see confirms it with a toast; an operation whose result is immediately visible needs none (see _Application Shell & Navigation_ → Toasts). -- **Create New**: starts a new snippet from a small sample Vega-Lite bar-chart template (a few inline category/value rows), assigns it an auto-generated default name (see _Naming & Tags_), saves it, and makes it the active snippet. +- **Create New**: starts a new snippet from a small sample Vega-Lite bar-chart template (a few inline category/value rows), assigns it an auto-generated default name (see _Naming & Tags_), saves it, and makes it the active snippet. Opening in the editor _is_ the confirmation, so no toast is raised. - **Duplicate**: creates an independent copy of the active snippet with a name suffixed "(copy)". The copy carries over the specification, comment, tags, and dataset references, gets fresh created/modified timestamps and a new identity, and becomes the active snippet. A success toast confirms the duplication. - **Delete**: permanently removes the active snippet after the user confirms a warning that the action cannot be undone. After deletion no snippet is active. A toast confirms the deletion. - These operations never affect other snippets. diff --git a/docs/spec/05-datasets.md b/docs/spec/05-datasets.md index b6e3d9a..df6dc72 100644 --- a/docs/spec/05-datasets.md +++ b/docs/spec/05-datasets.md @@ -84,11 +84,12 @@ The detail pane for a selected dataset shows: ## Actions -Each action raises a confirming toast (or an error toast on failure). +A destructive or off-screen outcome raises a confirming toast; an action whose result is immediately visible is confirmed by that change. Any action may raise an error toast on failure (see _Application Shell & Navigation_ → Toasts). - **Copy Reference** — copies the by-name reference object to the clipboard, ready to paste into a spec: `{ "data": { "name": "MyDataset" } }` -- **New / Create New** — opens the create form in the detail pane with fields: **name** (required, unique), **source** toggle (Inline / URL), the **data** (a paste area for inline, a URL field for URL source), and an optional **comment**. Save is disabled until a name and valid data/URL are present. On success the new dataset is selected. + The clipboard write is invisible, so it is confirmed _inline on the control_ ("Copied"), announced politely to assistive technology — not a toast. +- **New / Create New** — opens the create form in the detail pane with fields: **name** (required, unique), **source** toggle (Inline / URL), the **data** (a paste area for inline, a URL field for URL source), and an optional **comment**. Save is disabled until a name and valid data/URL are present. On success the new dataset is shown selected in the detail pane — that visible result is the confirmation, so no toast is raised (a dataset created _off-screen_ via Extract does toast; see _Spec Editor_). - **Edit** — rename, edit the comment, and update the data (re-paste inline data or refresh the URL). Updating inline data re-profiles it; the modified timestamp advances. - **Delete** — asks for confirmation ("Delete \"Name\"? This cannot be undone."), then removes the dataset and clears the selection. diff --git a/src/app/components/DatasetsModal.tsx b/src/app/components/DatasetsModal.tsx index 5b87459..b4ded64 100644 --- a/src/app/components/DatasetsModal.tsx +++ b/src/app/components/DatasetsModal.tsx @@ -191,8 +191,16 @@ function DatasetDetail({ confirmLabel: 'Delete', danger: true, }); - // TODO (M6, spec §05): success toast on delete. - if (ok) remove(dataset.id); + if (!ok) return; + const removedName = dataset.name; + remove(dataset.id); + // Confirm the deletion (spec §05). The message names which dataset went + // (council toast-copy rule, docs/architecture/10 → Toast copy). + notify({ + kind: 'success', + title: 'Dataset deleted', + message: `"${removedName}" was permanently removed.`, + }); }; return ( @@ -203,6 +211,13 @@ function DatasetDetail({ + {/* The clipboard write is invisible, so the success is confirmed inline + ("Copied") rather than by a toast (docs/architecture/10 → Toast copy). + This polite live region announces it to assistive tech, which the + button's visual label swap alone would not reliably do. */} + + {copied ? 'Reference copied to clipboard' : ''} + diff --git a/src/app/components/SnippetLibrary.tsx b/src/app/components/SnippetLibrary.tsx index c326cd7..5ee4b9c 100644 --- a/src/app/components/SnippetLibrary.tsx +++ b/src/app/components/SnippetLibrary.tsx @@ -11,6 +11,7 @@ import { useShallow } from 'zustand/react/shallow'; import { formatSnippetSize, hasUnpublishedChanges, snippetSizeBytes } from '@core/snippet'; import { confirm } from '../stores/ConfirmStore'; +import { notify } from '../stores/NotificationStore'; import { useSnippetStore } from '../stores/SnippetStore'; import styles from './SnippetLibrary.module.css'; @@ -73,18 +74,28 @@ export function SnippetLibrary() { const handleDelete = async (id: string, name: string) => { // In-app confirmation (docs/architecture/03 → confirmation dialogs). - // TODO: surface a deletion toast (spec §02) once the toast system lands (M6). const ok = await confirm({ title: 'Delete snippet', message: `Delete "${name}"? This cannot be undone.`, confirmLabel: 'Delete', danger: true, }); - if (ok) removeSnippet(id); + if (!ok) return; + removeSnippet(id); + // Confirm the deletion (spec §02). The message names which snippet went — + // useful when toasts stack (council toast-copy rule, docs/architecture/10). + notify({ + kind: 'success', + title: 'Snippet deleted', + message: `"${name}" was permanently removed from your library.`, + }); }; return (
+ {/* Create raises no toast: the new snippet opens in the editor, so the + result is already on-screen (spec §02; docs/architecture/10 → Toast + copy). Delete/duplicate toast because the outcome isn't visible. */} diff --git a/src/app/components/SpecEditor.tsx b/src/app/components/SpecEditor.tsx index 9effc77..aa10e0b 100644 --- a/src/app/components/SpecEditor.tsx +++ b/src/app/components/SpecEditor.tsx @@ -27,6 +27,7 @@ import { openModal } from '../modals/ModalCoordinator'; import { useAppStore } from '../stores/AppStore'; import { confirm } from '../stores/ConfirmStore'; import { hasInlineData } from '../stores/ExtractStore'; +import { notify } from '../stores/NotificationStore'; import { usePreviewStore } from '../stores/PreviewStore'; import { selectActiveSnippet, selectShownText, useSnippetStore } from '../stores/SnippetStore'; import { SegmentedControl, type SegmentedOption } from './SegmentedControl'; @@ -64,7 +65,14 @@ function EditorToolbar() { const handlePublish = () => { if (!useSnippetStore.getState().activeSnippetId) return; useSnippetStore.getState().publish(); - // TODO: success toast "Snippet published" once the toast system lands (M6, spec §03D). + // Success confirmation (spec §03D). Per the council's toast-copy rule + // (docs/architecture/10 → Toast copy), the title states the action and the + // message adds the consequence rather than paraphrasing it. + notify({ + kind: 'success', + title: 'Snippet published', + message: 'Your draft is now the published version.', + }); }; const handleRevert = async () => { @@ -77,7 +85,11 @@ function EditorToolbar() { }); if (ok) { useSnippetStore.getState().revert(); - // TODO: success toast "Draft reverted" once the toast system lands (M6, spec §03D). + notify({ + kind: 'success', + title: 'Draft reverted', + message: 'The editor was restored to the last published version.', + }); } }; diff --git a/src/app/components/Toaster.module.css b/src/app/components/Toaster.module.css index 119013d..c9d5bfb 100644 --- a/src/app/components/Toaster.module.css +++ b/src/app/components/Toaster.module.css @@ -1,10 +1,14 @@ .region { position: fixed; - top: var(--space-5); + /* Bottom-right, not top-right: the header's action cluster (Publish/Revert, + theme/datasets controls) lives top-right, and a toast there lands on top of + the control the user just used. Bottom-anchored, the stack grows upward and + the newest toast sits nearest the corner (docs/architecture/10 → Toasts). */ + bottom: var(--space-5); right: var(--space-5); /* Above the confirm backdrop (z 1000) so a failure stays visible and - dismissible even with a confirmation open; top-right won't block the - centered dialog. */ + dismissible even with a confirmation open; the corner clears the centered + dialog. */ z-index: 1100; display: flex; flex-direction: column; diff --git a/src/app/components/Toaster.tsx b/src/app/components/Toaster.tsx index 341c642..e785c57 100644 --- a/src/app/components/Toaster.tsx +++ b/src/app/components/Toaster.tsx @@ -2,8 +2,9 @@ * Toaster — renders the NotificationStore as a stack of toasts (spec §10, * design language → Toasts). Mounted once at the app root, beside ConfirmDialog. * - * The non-blocking counterpart to ConfirmDialog: top-right, stacked newest-last, - * each dismissible. Error/warning toasts persist until dismissed (Carbon: a + * The non-blocking counterpart to ConfirmDialog: bottom-right, stacked + * newest-nearest-the-corner, each dismissible (placement rationale in + * Toaster.module.css). Error/warning toasts persist until dismissed (Carbon: a * critical message shouldn't vanish on a timer); success/info auto-dismiss. A * failure that carries diagnostic `detail` exposes it under a collapsed * "Technical details" disclosure — available to report, without shouting. diff --git a/src/app/stores/DatasetStore.ts b/src/app/stores/DatasetStore.ts index 842b84e..93a03b0 100644 --- a/src/app/stores/DatasetStore.ts +++ b/src/app/stores/DatasetStore.ts @@ -221,6 +221,10 @@ export const useDatasetStore = create((set, get) => ({ now, }); get().add(dataset); + // Switching to the detail view with the new dataset selected IS the success + // confirmation, so no toast is raised — the result is on-screen (spec §05; + // docs/architecture/10 → Toast copy). Extract-to-dataset, which creates a + // dataset off-screen, does toast (see ExtractStore). set({ view: 'detail', form: EMPTY_FORM, formError: null }); return true; }, diff --git a/src/app/stores/ExtractStore.ts b/src/app/stores/ExtractStore.ts index 3909668..cda40b5 100644 --- a/src/app/stores/ExtractStore.ts +++ b/src/app/stores/ExtractStore.ts @@ -17,6 +17,7 @@ import type { DataFormat } from '@core/format-detection'; import { createDataset } from '@core/dataset'; import { isNameTaken } from '@core/naming'; import { useDatasetStore } from './DatasetStore'; +import { notify } from './NotificationStore'; import { useSnippetStore } from './SnippetStore'; /** The inline `data` block of a parsed spec, if it carries `values`. */ @@ -123,8 +124,14 @@ export const useExtractStore = create((set, get) => ({ spec.data = { name }; useSnippetStore.getState().replaceActiveDraft(JSON.stringify(spec, null, 2), now); - // TODO (M6, spec §03F): success toast "Dataset created" — deferred with the - // other success toasts (see SnippetStore publish/revert breadcrumbs). + // Success confirmation (spec §03F). Title states the action; the message + // adds the consequence — the spec was rewritten to reference the new dataset + // by name (council toast-copy rule, docs/architecture/10 → Toast copy). + notify({ + kind: 'success', + title: 'Dataset created', + message: `The spec now references "${name}" instead of its inline data.`, + }); set(INITIAL); return true; }, diff --git a/src/core/rendering.test.ts b/src/core/rendering.test.ts index 213c6d9..43e8d78 100644 --- a/src/core/rendering.test.ts +++ b/src/core/rendering.test.ts @@ -214,6 +214,25 @@ describe('prepareSpecForRender — dataset resolution (spec §04 Rendering Contr }; expect(out.data).toEqual({ values: [{ a: 1 }], foo: 1 }); }); + + test('a "data" field buried in inline rows is not resolved and does not throw', () => { + // The inline rows carry a column named `data` whose value looks like a + // reference object. It is payload; resolution must not descend into it. + const spec = { data: { values: [{ data: { name: 'Missing' } }] }, mark: 'bar' }; + const out = prepareSpecForRender(spec, { datasets }) as { data: { values: unknown[] } }; + expect(out.data.values).toEqual([{ data: { name: 'Missing' } }]); + }); + + test('resolves a reference inside a lookup transform (from.data)', () => { + const spec = { + data: { name: 'JsonDs' }, + transform: [{ lookup: 'id', from: { data: { name: 'CsvDs' }, key: 'id', fields: ['x'] } }], + }; + const out = prepareSpecForRender(spec, { datasets }) as unknown as { + transform: Array<{ from: { data: unknown } }>; + }; + expect(out.transform[0].from.data).toEqual({ values: 'a,b\n1,2', format: { type: 'csv' } }); + }); }); describe('escapeVegaField', () => { diff --git a/src/core/rendering.ts b/src/core/rendering.ts index 9bbb6f8..b5318bd 100644 --- a/src/core/rendering.ts +++ b/src/core/rendering.ts @@ -149,11 +149,12 @@ function resolvedData(dataset: ResolvableDataset, rest: SpecNode): SpecNode { /** * Replace every named-data reference in `node` with its library dataset's - * contents, recursing through the entire spec (arrays and objects) so refs - * anywhere are resolved — matching `extractDatasetRefs`. A self-defined name is - * left untouched; an unknown library name throws `DatasetNotFoundError`. Matching - * is case-insensitive, mirroring naming.ts. Mutates in place; the caller already - * works on a copy. + * contents, recursing through the spec (arrays and objects) so refs anywhere are + * resolved — matching `extractDatasetRefs`, including pruning the `data` and + * top-level `datasets` payload keys so resolution never descends into user data + * rows. A self-defined name is left untouched; an unknown library name throws + * `DatasetNotFoundError`. Matching is case-insensitive, mirroring naming.ts. + * Mutates in place; the caller already works on a copy. */ function resolveDatasetRefs( node: unknown, @@ -177,15 +178,16 @@ function resolveDatasetRefs( } } - // TODO: this walks EVERY key, so it also descends into inlined data payloads - // (the just-resolved `values`, a spec's `datasets`/`data.values`). That's - // wasteful for large inline data on every debounced render, and a row with a - // field literally named `data` holding `{ name: "x" }` would be spuriously - // resolved or throw DatasetNotFoundError. extractDatasetRefs shares this broad - // walk. A scoped walk (recurse only into the known sub-spec/container keys + - // `transform[].lookup.from`, never into data payloads) would be safer and - // faster — change deliberately, with tests for where refs may legally appear. - for (const key of Object.keys(node)) resolveDatasetRefs(node[key], byName, selfDefined); + // Recurse into every key except the two that hold data payloads (`data` — + // resolved/captured above; `datasets` — the spec's own inline data). Pruning + // them avoids descending into the just-resolved `values` and into user data + // rows, where a field named `data` holding `{ name: "x" }` would otherwise be + // spuriously resolved or throw DatasetNotFoundError. extractDatasetRefs prunes + // the same two keys so resolution and extraction stay in agreement. + for (const key of Object.keys(node)) { + if (key === 'data' || key === 'datasets') continue; + resolveDatasetRefs(node[key], byName, selfDefined); + } } /** diff --git a/src/core/spec-refs.test.ts b/src/core/spec-refs.test.ts index 1f60bc1..c4e89f1 100644 --- a/src/core/spec-refs.test.ts +++ b/src/core/spec-refs.test.ts @@ -47,6 +47,28 @@ describe('extractDatasetRefs', () => { // "foo" is self-defined and not a library dependency; "Library" is. expect(extractDatasetRefs(spec)).toEqual(['Library']); }); + + test('a data row carrying a field literally named "data" is not a reference', () => { + // The inline rows happen to have a column called `data` whose value looks + // like a reference object — it is payload, not a library dependency. + const spec = { + data: { values: [{ data: { name: 'NotARef' } }, { data: { name: 'AlsoNot' } }] }, + mark: 'bar', + }; + expect(extractDatasetRefs(spec)).toEqual([]); + }); + + test('does not descend into a self-defined datasets payload', () => { + const spec = { datasets: { local: [{ data: { name: 'Buried' } }] }, mark: 'bar' }; + expect(extractDatasetRefs(spec)).toEqual([]); + }); + + test('collects a reference from a lookup transform (from.data)', () => { + const spec = { + transform: [{ lookup: 'id', from: { data: { name: 'Lookup' }, key: 'id', fields: ['x'] } }], + }; + expect(extractDatasetRefs(spec)).toEqual(['Lookup']); + }); }); describe('recomputeDatasetRefs', () => { @@ -105,4 +127,22 @@ describe('renameDatasetInSpec', () => { expect(out.data.name).toBe('Old'); // self-defined — left untouched expect(Object.keys(out.datasets)).toEqual(['Old']); }); + + test('does not rewrite a "data" field buried in inline data rows', () => { + const spec = { + data: { name: 'Old', values: [{ data: { name: 'Old' } }] }, + mark: 'bar', + }; + const out = renameDatasetInSpec(spec, 'Old', 'New'); + expect(out.data.name).toBe('New'); // the real reference is renamed + expect(out.data.values[0].data.name).toBe('Old'); // the row payload is left alone + }); + + test('renames a reference inside a lookup transform (from.data)', () => { + const spec = { + transform: [{ lookup: 'id', from: { data: { name: 'Old' }, key: 'id' } }], + }; + const out = renameDatasetInSpec(spec, 'Old', 'New'); + expect(out.transform[0].from.data.name).toBe('New'); + }); }); diff --git a/src/core/spec-refs.ts b/src/core/spec-refs.ts index 69b15ae..8227af4 100644 --- a/src/core/spec-refs.ts +++ b/src/core/spec-refs.ts @@ -4,10 +4,19 @@ * * Portable core: no browser APIs, no React, no store access. A Vega-Lite spec * references named data through `{ "data": { "name": "MyDataset" } }`, which can - * appear at the top level, per-layer, or inside `spec`/`facet`/concat children. - * Rather than enumerate the grammar, we walk the spec recursively and collect - * every `{ data: { name } }` we find — the single source of truth for "what does - * this spec reference", which the renderer's resolution must agree with. + * appear at the top level, per-layer, inside `spec`/`facet`/concat children, or in + * a lookup transform's `from.data`. Rather than enumerate the grammar, we walk the + * spec recursively and collect every `{ data: { name } }` we find — the single + * source of truth for "what does this spec reference", which the renderer's + * resolution must agree with. + * + * The walk recurses into every key EXCEPT two, which hold user data payloads + * rather than nested specs: a `data` object (its `name` is captured at the parent + * site; its `values`/`format` are payload, never a nested ref) and a top-level + * `datasets` map (the spec's own inline data). Pruning those is what keeps a data + * *row* that happens to carry a field literally named `data: { name: "x" }` from + * being misread as a library reference. The renderer's resolution prunes the same + * two keys so the two stay in lockstep. * * A spec may be stored as an **object** or as **JSON text** (see spec §09A); we * normalize once at the boundary (unparseable text → no refs / unchanged spec) so @@ -66,12 +75,14 @@ export function extractDatasetRefs(spec: Json): string[] { if (data && typeof data === 'object' && typeof data.name === 'string') { if (!selfDefined.has(data.name)) names.add(data.name); } - // TODO: walks every key, so it also descends into data payloads - // (`data.values`, top-level `datasets`). A row with a field named `data` - // holding `{ name: "x" }` is falsely counted as a reference. Shared with - // rendering.ts resolveDatasetRefs — scope both to the keys where refs can - // legally appear, together and with tests. Benign for typical data. - for (const key of Object.keys(obj)) walk(obj[key]); + // Recurse into every key except the two that hold data payloads (`data` — + // captured above; `datasets` — the spec's own inline data). Pruning them + // keeps the walk out of user data rows, where a field named `data` would + // otherwise be misread as a reference. Kept in step with rendering.ts. + for (const key of Object.keys(obj)) { + if (key === 'data' || key === 'datasets') continue; + walk(obj[key]); + } } }; @@ -105,15 +116,16 @@ export function renameDatasetInSpec(spec: T, oldName: string, newName: string if (node && typeof node === 'object') { const out: Record = {}; for (const [k, v] of Object.entries(node as Record)) { - if ( - k === 'data' && - v && - typeof v === 'object' && - !Array.isArray(v) && - (v as Record).name === oldName && - !selfDefined.has(oldName) - ) { - out[k] = { ...v, name: newName }; + // A `data` object is a reference site, not a container: rename a matching + // name and stop — never recurse into its payload. A `datasets` map is the + // spec's own inline data: leave it whole. Pruning both (mirroring + // extractDatasetRefs) keeps rename out of user data rows, where a field + // named `data` would otherwise be rewritten as if it were a reference. + if (k === 'data' && v && typeof v === 'object' && !Array.isArray(v)) { + const dv = v as Record; + out[k] = dv.name === oldName && !selfDefined.has(oldName) ? { ...dv, name: newName } : dv; + } else if (k === 'data' || k === 'datasets') { + out[k] = v; } else { out[k] = rewrite(v); } diff --git a/styles/base.css b/styles/base.css index b5932ae..f878665 100644 --- a/styles/base.css +++ b/styles/base.css @@ -63,3 +63,23 @@ body { transition-duration: 0.01ms !important; } } + +/* + * Visually-hidden but available to assistive tech — the standard sr-only recipe. + * Used for polite `aria-live`/`role="status"` announcements whose visible + * confirmation lives elsewhere (e.g. Copy Reference shows "Copied" on the button + * for sighted users; this announces it to a screen reader). See + * docs/architecture/10 → Toast copy (the "toast only what can't be seen" rule). + */ +.visually-hidden { + position: absolute; + width: 1px; + height: 1px; + margin: -1px; + padding: 0; + border: 0; + overflow: hidden; + clip: rect(0 0 0 0); + clip-path: inset(50%); + white-space: nowrap; +}