mirror of
https://github.com/olehomelchenko/astrolabe.git
synced 2026-08-08 02:02:33 +00:00
Fix dataset-ref walk false positive and wire spec-mandated success toasts
- Prune the data payload and top-level datasets keys in all three ref walks (extractDatasetRefs, renameDatasetInSpec, resolveDatasetRefs) so a data row carrying a field named "data" is no longer misread as a library reference, spuriously rewritten, or made to throw DatasetNotFoundError. Adds tests, including a guard that lookup-transform refs (from.data) still resolve. - Wire the deferred success toasts now the Toaster has landed: publish, revert, extract-to-dataset, and snippet/dataset delete. Copy follows the council title-vs-message rule (title states the action, message adds the consequence). - Reconcile the spec's blanket toast mandate to "toast only what the user can't already see": no toast on visible-result creates (snippet, dataset form); Copy Reference stays inline and gains an aria-live announcement (new shared .visually-hidden utility) instead of a toast-per-copy. - Move the toast region to bottom-right so it stops covering the header action cluster (Publish/Revert, theme/datasets). - Update docs/spec 01F/02/05 and docs/architecture/07 + 10 to match.
This commit is contained in:
@@ -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({
|
||||
<button type="button" className={styles.action} onClick={() => void handleCopy()}>
|
||||
{copied ? 'Copied' : 'Copy Reference'}
|
||||
</button>
|
||||
{/* 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. */}
|
||||
<span role="status" className="visually-hidden">
|
||||
{copied ? 'Reference copied to clipboard' : ''}
|
||||
</span>
|
||||
<button type="button" className={styles.action} onClick={handleEdit}>
|
||||
Edit
|
||||
</button>
|
||||
|
||||
@@ -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 (
|
||||
<div className={styles.library}>
|
||||
{/* 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. */}
|
||||
<button className={styles.createNew} onClick={() => createSnippet()}>
|
||||
+ Create New Snippet
|
||||
</button>
|
||||
|
||||
@@ -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.',
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -221,6 +221,10 @@ export const useDatasetStore = create<DatasetState>((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;
|
||||
},
|
||||
|
||||
@@ -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<ExtractState>((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;
|
||||
},
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
+16
-14
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
+31
-19
@@ -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<T>(spec: T, oldName: string, newName: string
|
||||
if (node && typeof node === 'object') {
|
||||
const out: Record<string, Json> = {};
|
||||
for (const [k, v] of Object.entries(node as Record<string, Json>)) {
|
||||
if (
|
||||
k === 'data' &&
|
||||
v &&
|
||||
typeof v === 'object' &&
|
||||
!Array.isArray(v) &&
|
||||
(v as Record<string, Json>).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<string, Json>;
|
||||
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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user