diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index f2a932c..13f1d5e 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -324,6 +324,46 @@ The convergent rules and citations live in that doc; the spec (§06) was amended --- +## M4.5 · Snippet-library consolidation ✅ (done) + +**Why out of band:** a spec-vs-implementation audit after M4 found §02 features that no +later milestone owned — the **Selected-Snippet Metadata Panel** (inline name + comment +editing, timestamps, linked datasets) and the **Duplicate** operation. Without them a +snippet could only ever carry its auto-generated date-time name (no rename, no annotation, +no copy), a sharp edge for a _snippet manager_. Closed before M5 since the spec text +already existed and the work was core-first and cheap. + +**Core** + +- `snippet.ts` — `duplicateSnippet(source, {now,id})`: independent copy carrying both spec + versions, comment, tags, and dataset refs; "(copy)" name; fresh identity/timestamps; + cloned mutable members. + +**App** + +- `SnippetStore` — `renameSnippet`, `setComment` (both advance `modified` per §02 → Sort, + no editor-buffer touch), `duplicateActiveSnippet` (flushes the live buffer first, prepends + the copy, makes it active). +- `SnippetLibrary` — the metadata panel below the list: Name + Comment auto-save (debounced + while typing, flushed on blur), read-only Created/Modified, Linked Datasets list, and + Duplicate / Delete. Duplicate raises a success toast (the copy isn't self-evident, unlike + Create); §02-compliant. + +**Also fixed (§03C divergence):** the preview debounced _every_ change, so a snippet +load / Draft↔Published switch incurred a 300 ms blank instead of the spec's **immediate** +render. `LivePreview` now renders immediately on `bufferEpoch`/`editorView` change and +debounces only keystroke (`shownText`-only) changes. + +**Still deferred to M5/M6 (per §02):** Search, Sort controls + persistence, two distinct +empty-state messages, Storage Monitor. + +**Verified:** `typecheck` + `test` (287 passing — `snippet` duplicate factory, +`SnippetStore` rename/comment/duplicate, a `SnippetLibrary` render test guarding the +auto-save effect against a render loop) + `eslint` clean + `build` (PWA, 41 precache +entries). + +--- + ## M5 · Settings + Import/Export **Goal:** preferences and whole-workspace backup/transfer. diff --git a/docs/architecture/01-state-and-stores.md b/docs/architecture/01-state-and-stores.md index ae5f695..f7c125a 100644 --- a/docs/architecture/01-state-and-stores.md +++ b/docs/architecture/01-state-and-stores.md @@ -472,6 +472,14 @@ reset: () => set({ snippets: [], activeSnippetId: null, draftSpec: '' }); - Do persistence and external sync (IndexedDB, `localStorage`, URL hash, theme) in startup `subscribe` listeners via `infrastructure/` adapters. - Debounce expensive reactions (auto-save, re-render) inside the subscriber. +- Advance a snippet's `modified` on **every** save the library sorts by — draft + auto-save, inline name/comment edits, publish, revert, rename-propagation — so + Modified-descending keeps the just-touched snippet on top (spec §02 → Sort). +- Bump `SnippetStore.bufferEpoch` only on a _programmatic_ buffer load (select / + create / duplicate / revert / hydrate) — it is the "reload the editor, this isn't + a keystroke" signal consumed by both the Monaco buffer and the preview's + immediate-render path (arch 05 §5). Metadata edits (name/comment) advance + `modified` but must **not** bump it — they aren't in the spec buffer. - Import singleton store hooks directly in the leaves that need shared state. **Don't** diff --git a/docs/architecture/05-rendering-theming-preview.md b/docs/architecture/05-rendering-theming-preview.md index b5b6a37..b324abd 100644 --- a/docs/architecture/05-rendering-theming-preview.md +++ b/docs/architecture/05-rendering-theming-preview.md @@ -333,6 +333,28 @@ useSettingsStore.subscribe((s, prev) => { }); ``` +### Implemented policy: what renders immediately vs. debounced + +> The service above is a **sketch**; the shipped renderer lives inline in +> `LivePreview.tsx` (one `setTimeout` whose delay is computed per change) and +> subscribes to the stores via hooks rather than startup subscribers. When it is +> extracted into a service, preserve this policy. + +The debounce exists to stay out of the way **while typing** — nothing else. So the +delay is `0` (immediate) for everything except keystrokes (spec §03C): + +- **Immediate** — a _programmatic buffer load_ (`SnippetStore.bufferEpoch` changed: + select / create / duplicate / revert / hydrate) or a _Draft↔Published switch_ + (`editorView` changed). These are the cases §03C names; the editor and preview + both key off `bufferEpoch` to tell a load from a keystroke. +- **Debounced** — a keystroke (only `shownText` changed). This is the churn the + debounce protects against. + +Detect "this was a keystroke" by elimination: `shownText` changed but `bufferEpoch` +and `editorView` did **not**. Fit-mode and theme changes currently fall through the +debounce too (harmless; not typing) — flush them if instant feedback is wanted, but +never debounce a load or a view switch. + ### Busy indicator `setBusy(true/false)` toggles store state that the preview reads to overlay a @@ -532,7 +554,7 @@ bookkeeping. Gate the observer to responsive modes (Original needs no re-fit). | View teardown | `view.finalize()` before each re-render and on unmount | the renderer's `RenderHandle` | | Theming | Vega `Config` per UI theme, applied at embed time | `chartConfigFor()` in `src/core/vega-themes.ts` | | Field names | `escapeVegaField` on every data-derived `field:` | `src/core/rendering.ts` | -| Debounce | `createDebouncedRenderer`, delay from `renderDebounce` setting | `src/app/services/debounced-renderer.ts` | +| Debounce | Inline timer; `0` on buffer-load/view-switch, `renderDebounce` on keystroke (§5) | `LivePreview.tsx` (service not yet extracted) | | Spec prep | `prepareSpecForRender` (pure, on a copy) | `src/core/rendering.ts` (see _Live Preview_) | | Errors | One error field, cleared on success, empty = nothing | `PreviewStore.error` | | Container fit | Inner host + frame (out-specify `.vega-embed`); resize via synthetic `window:resize` | §8 (`LivePreview` + `chart-renderer`) | diff --git a/docs/architecture/10-interaction-and-feedback.md b/docs/architecture/10-interaction-and-feedback.md index 04f9ac5..8c7e511 100644 --- a/docs/architecture/10-interaction-and-feedback.md +++ b/docs/architecture/10-interaction-and-feedback.md @@ -252,6 +252,29 @@ Not features to add later — the baseline every surface is built on. --- +## 7. Revealed actions & destructive affordances + +How row/list actions appear, and how dangerous ones signal themselves. (Pairs with the +iconography contract, [arch 09 §5](09-visual-design.md).) + +- **Reveal-on-hover is a per-surface choice, not a default.** Hiding a control until hover + cuts clutter in a **dense, repeated** list the user inevitably traverses (the snippet-row + delete) — there, arrival is guaranteed, so discoverability isn't lost. But a **rare or + load-bearing** action must stay **always-visible**, or it becomes effectively unreachable + (NN/g #6 — recognition over recall; a feature you can't see you can't use). Decide per + surface; when in doubt, show it. +- **A hover-revealed control must also reveal on keyboard focus.** Gate visibility on + `:hover` **and** `:focus-within`/`:focus-visible`, never hover alone — otherwise the + action is mouse-only and invisible to keyboard users (WCAG 2.1.1). The snippet row reveals + its delete on `.item:hover` _and_ `.delete:focus-visible`. +- **Destructive controls signal danger on hover _and_ focus.** A delete/remove affordance + reddens to `--support-error` on both `:hover` and `:focus-visible` — not colour-by-mouse + only — so the warning reaches keyboard users at parity. Colour is a _reinforcement_ here, + never the sole signal: the control still carries its label/`aria-label` and the + consequential ones still route through a confirm dialog (§4). + +--- + ## Do / Don't **Do** diff --git a/docs/spec/02-snippet-library.md b/docs/spec/02-snippet-library.md index 3288653..23da7fe 100644 --- a/docs/spec/02-snippet-library.md +++ b/docs/spec/02-snippet-library.md @@ -61,7 +61,7 @@ The library provides the lifecycle operations for snippets. An operation whose o - **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. +- **Delete**: permanently removes the active snippet after the user confirms a warning that the action cannot be undone. After deletion the newest remaining snippet becomes active (so the editor and detail panel stay populated); if none remain, no snippet is active. A toast confirms the deletion. - These operations never affect other snippets. ## Naming & Tags diff --git a/src/app/components/LivePreview.tsx b/src/app/components/LivePreview.tsx index 2fffdf7..40606be 100644 --- a/src/app/components/LivePreview.tsx +++ b/src/app/components/LivePreview.tsx @@ -78,6 +78,14 @@ export function LivePreview() { // Datasets feed reference resolution (spec §04 step 1). Re-rendering on a // dataset change keeps a referencing chart live as its data is edited. const datasets = useDatasetStore(useShallow((s) => s.datasets)); + // A programmatic buffer load (select/create/revert/hydrate — `bufferEpoch`) or a + // Draft/Published switch (`editorView`) must render *immediately*, not after the + // typing debounce (spec §03C). Keystrokes change only `shownText`, so when these + // two are unchanged the change is typing and the debounce applies. + const bufferEpoch = useSnippetStore((s) => s.bufferEpoch); + const editorView = useSnippetStore((s) => s.editorView); + // Seed with a sentinel epoch so the very first paint counts as a load (immediate). + const lastLoadRef = useRef({ bufferEpoch: -1, editorView }); const error = usePreviewStore((s) => s.error); const setError = usePreviewStore((s) => s.setError); @@ -90,6 +98,12 @@ export function LivePreview() { if (!node) return; const text = shownText.trim(); + // Immediate on snippet load / view switch (spec §03C), debounced while typing. + const prevLoad = lastLoadRef.current; + const immediate = bufferEpoch !== prevLoad.bufferEpoch || editorView !== prevLoad.editorView; + lastLoadRef.current = { bufferEpoch, editorView }; + const delay = immediate ? 0 : RENDER_DEBOUNCE_MS; + // The debounced body is async; wrap in a void IIFE so the timer callback // returns void (it handles its own errors internally — nothing awaits it). const timer = setTimeout(() => { @@ -148,10 +162,10 @@ export function LivePreview() { } } })(); - }, RENDER_DEBOUNCE_MS); + }, delay); return () => clearTimeout(timer); - }, [shownText, fitMode, uiTheme, datasets, setError]); + }, [shownText, fitMode, uiTheme, datasets, setError, bufferEpoch, editorView]); // Re-fit the chart when its container resizes (e.g. a pane drag). Vega doesn't // observe the element, so we do: one observer on the stable host node for the diff --git a/src/app/components/SnippetLibrary.module.css b/src/app/components/SnippetLibrary.module.css index fb0d01d..4ab3936 100644 --- a/src/app/components/SnippetLibrary.module.css +++ b/src/app/components/SnippetLibrary.module.css @@ -6,6 +6,10 @@ .createNew { flex: 0 0 auto; + display: inline-flex; + align-items: center; + justify-content: center; + gap: var(--space-2); margin: var(--space-4); height: 40px; padding: 0 var(--space-5); @@ -16,7 +20,6 @@ font: inherit; font-weight: 600; cursor: pointer; - text-align: center; transition: background var(--dur-fast) var(--ease); } @@ -130,17 +133,23 @@ } /* Linked-datasets indicator (spec §02): icon + count, meaning carried by the - icon + accessible label, not colour. Pushed to the row's trailing edge. */ + icon + accessible label, not colour. Sits inline with the date as one grouped + metadata run, separated by a middot, rather than orphaned at the row's edge. */ .datasets { flex: 0 0 auto; - margin-left: auto; display: inline-flex; align-items: center; gap: var(--space-1); - font-size: 11px; + font-size: 12px; color: var(--text-secondary); } +.datasets::before { + content: '·'; + margin-right: var(--space-2); + color: var(--text-placeholder); +} + .delete { flex: 0 0 auto; align-self: center; @@ -151,11 +160,12 @@ background: none; color: var(--text-secondary); cursor: pointer; - font-size: 12px; - padding: var(--space-1); + padding: var(--space-2); border-radius: var(--radius); opacity: 0; - transition: opacity var(--dur-fast) var(--ease); + transition: + opacity var(--dur-fast) var(--ease), + color var(--dur-fast) var(--ease); } .item:hover .delete, @@ -163,6 +173,157 @@ opacity: 1; } -.delete:hover { +/* Destructive intent reddens on hover AND keyboard focus, not colour-by-mouse-only + (arch 10 — destructive controls signal danger on hover/focus). */ +.delete:hover, +.delete:focus-visible { color: var(--support-error); } + +/* Selected-Snippet Metadata Panel (spec §02) — pinned below the list, the active + snippet's editable Name/Comment, read-only timestamps, linked datasets, and + Duplicate/Delete. Capped height with its own scroll so a long comment never + pushes the list away entirely. */ +.meta { + flex: 0 0 auto; + max-height: 45%; + overflow: auto; + display: flex; + flex-direction: column; + gap: var(--space-3); + padding: var(--space-4); + border-top: var(--border-width) solid var(--border); + background: var(--layer-01); +} + +.metaField { + display: flex; + flex-direction: column; + gap: var(--space-1); +} + +.metaLabel { + font-size: 11px; + font-weight: 600; + letter-spacing: 0.02em; + text-transform: uppercase; + color: var(--text-secondary); +} + +.metaName, +.metaComment { + width: 100%; + padding: var(--space-2) var(--space-3); + border: var(--border-width) solid var(--border); + border-radius: var(--radius); + background: var(--layer-02, var(--background)); + color: var(--text-primary); + font: inherit; + font-size: 13px; +} + +.metaName { + font-weight: 500; +} + +.metaComment { + resize: vertical; + min-height: 2.4em; + line-height: 1.4; +} + +.metaName:focus-visible, +.metaComment:focus-visible { + outline: 2px solid var(--focus); + outline-offset: 1px; + border-color: var(--accent); +} + +/* Read-only Created / Modified, as compact label→value rows. */ +.metaTimes { + margin: 0; + display: flex; + flex-direction: column; + gap: var(--space-1); +} + +.metaTimes > div { + display: flex; + justify-content: space-between; + gap: var(--space-3); + font-size: 11px; +} + +.metaTimes dt { + color: var(--text-secondary); +} + +.metaTimes dd { + margin: 0; + color: var(--text-primary); + white-space: nowrap; +} + +.metaLinked { + display: flex; + flex-direction: column; + gap: var(--space-2); +} + +.linkedList { + list-style: none; + margin: 0; + padding: 0; + display: flex; + flex-direction: column; + gap: var(--space-1); +} + +.linkedItem { + display: flex; + align-items: center; + gap: var(--space-2); + font-size: 12px; + color: var(--text-secondary); +} + +.linkedName { + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.metaActions { + display: flex; + gap: var(--space-2); + margin-top: var(--space-1); +} + +.metaAction { + flex: 1 1 auto; + height: 32px; + padding: 0 var(--space-3); + border: var(--border-width) solid var(--border); + border-radius: var(--radius); + background: var(--background); + color: var(--text-primary); + font: inherit; + font-size: 12px; + font-weight: 500; + cursor: pointer; + transition: + background var(--dur-fast) var(--ease), + border-color var(--dur-fast) var(--ease), + color var(--dur-fast) var(--ease); +} + +.metaAction:hover { + background: var(--layer-02, var(--layer-01)); + border-color: var(--border-strong, var(--border)); +} + +.metaDanger:hover { + color: var(--support-error); + border-color: var(--support-error); +} diff --git a/src/app/components/SnippetLibrary.test.tsx b/src/app/components/SnippetLibrary.test.tsx new file mode 100644 index 0000000..3c01962 --- /dev/null +++ b/src/app/components/SnippetLibrary.test.tsx @@ -0,0 +1,105 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { createSnippet } from '@core/snippet'; +import { useSnippetStore } from '../stores/SnippetStore'; +import { SnippetLibrary } from './SnippetLibrary'; + +// React 19 wants this flag set for act() to drive effects without warnings. +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +let container: HTMLDivElement; +let root: Root; + +beforeEach(() => { + useSnippetStore.getState().reset(); + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); +}); + +afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.useRealTimers(); +}); + +/** Set a controlled input/textarea's value the way React expects, then fire input. */ +function typeInto(el: HTMLInputElement | HTMLTextAreaElement, value: string) { + const proto = + el instanceof HTMLTextAreaElement ? HTMLTextAreaElement.prototype : HTMLInputElement.prototype; + // The native value setter bypasses React 19's input value tracking so the + // synthetic input event registers as a real change. + // eslint-disable-next-line @typescript-eslint/unbound-method + const setter = Object.getOwnPropertyDescriptor(proto, 'value')!.set!; + setter.call(el, value); + el.dispatchEvent(new Event('input', { bubbles: true })); +} + +describe('SnippetLibrary metadata panel (spec §02)', () => { + test('renders the active snippet name, comment, and linked datasets without looping', async () => { + const s = { + ...createSnippet({ id: 'a', name: 'Bar chart', now: new Date('2026-01-01T00:00:00Z') }), + comment: 'a note', + datasetRefs: ['Sales'], + }; + useSnippetStore.getState().hydrate([s], 'a'); + + // If the auto-save effect looped, this act() would throw "Maximum update depth". + await act(async () => { + root.render(); + await Promise.resolve(); + }); + + const name = container.querySelector('input') as HTMLInputElement; + const comment = container.querySelector('textarea') as HTMLTextAreaElement; + expect(name.value).toBe('Bar chart'); + expect(comment.value).toBe('a note'); + expect(container.textContent).toContain('Linked datasets'); + expect(container.textContent).toContain('Sales'); + }); + + test('auto-saves an inline name edit after the debounce', () => { + vi.useFakeTimers(); + const s = createSnippet({ id: 'a', name: 'Old', now: new Date('2026-01-01T00:00:00Z') }); + useSnippetStore.getState().hydrate([s], 'a'); + + act(() => { + root.render(); + }); + + const name = container.querySelector('input') as HTMLInputElement; + act(() => typeInto(name, 'Renamed')); + // Before the debounce fires, the store is unchanged. + expect(useSnippetStore.getState().snippets[0].name).toBe('Old'); + + act(() => { + vi.advanceTimersByTime(500); + }); + expect(useSnippetStore.getState().snippets[0].name).toBe('Renamed'); + }); + + test('Duplicate adds an independent copy and makes it active', async () => { + const s = createSnippet({ id: 'a', name: 'Chart', now: new Date('2026-01-01T00:00:00Z') }); + useSnippetStore.getState().hydrate([s], 'a'); + + await act(async () => { + root.render(); + await Promise.resolve(); + }); + + const dup = [...container.querySelectorAll('button')].find( + (b) => b.textContent === 'Duplicate', + )!; + await act(async () => { + dup.click(); + await Promise.resolve(); + }); + + const { snippets, activeSnippetId } = useSnippetStore.getState(); + expect(snippets).toHaveLength(2); + const active = snippets.find((x) => x.id === activeSnippetId)!; + expect(active.name).toBe('Chart (copy)'); + expect(active.id).not.toBe('a'); + }); +}); diff --git a/src/app/components/SnippetLibrary.tsx b/src/app/components/SnippetLibrary.tsx index 5ee4b9c..8d03e53 100644 --- a/src/app/components/SnippetLibrary.tsx +++ b/src/app/components/SnippetLibrary.tsx @@ -1,49 +1,32 @@ /** * Snippet Library — the left pane (spec §02). * - * M1 scope: the always-visible list with a pinned "Create New Snippet" item, + * The always-visible list with a pinned "Create New Snippet" item, * selection/highlight, and delete. Each row carries a secondary metadata line — - * draft status indicator, relative date, and size (spec §02). Search, sort - * controls, the metadata panel, the dataset icon, and the storage monitor arrive - * in later milestones. + * draft status indicator, relative date, and size (spec §02). Below the list, the + * Selected-Snippet Metadata Panel exposes the active snippet's editable Name and + * Comment (auto-saved), its timestamps and linked datasets, and the Duplicate / + * Delete operations. Search, sort controls, and the storage monitor arrive in + * later milestones. */ +import { useEffect, useState } from 'react'; import { useShallow } from 'zustand/react/shallow'; -import { formatSnippetSize, hasUnpublishedChanges, snippetSizeBytes } from '@core/snippet'; +import { + formatSnippetSize, + hasUnpublishedChanges, + snippetSizeBytes, + type Snippet, +} from '@core/snippet'; import { confirm } from '../stores/ConfirmStore'; import { notify } from '../stores/NotificationStore'; -import { useSnippetStore } from '../stores/SnippetStore'; +import { selectActiveSnippet, useSnippetStore } from '../stores/SnippetStore'; +import { Icon } from './Icon'; import styles from './SnippetLibrary.module.css'; -/** A small database glyph for the linked-datasets indicator (icons keep their own - * rounded geometry per the design tokens). Inherits `currentColor` so it themes. */ -function DatasetIcon() { - return ( - - ); -} +/** Auto-save settle time for the metadata panel's Name/Comment fields, mirroring + * the editor's draft auto-save (spec §02 → "edits save automatically"). */ +const META_AUTOSAVE_MS = 400; /** Compact relative date for the list (full date formatting lands in M5). */ function relativeDate(iso: string): string { @@ -62,12 +45,119 @@ function relativeDate(iso: string): string { return then.toLocaleDateString(); } +/** Absolute date-time for the metadata panel's read-only timestamps (the user's + * date-format setting wires in at M5; until then, the locale default). */ +function formatTimestamp(iso: string): string { + return new Date(iso).toLocaleString(); +} + +/** + * Selected-Snippet Metadata Panel (spec §02). Keyed by snippet id by its caller, + * so switching the active snippet remounts it and the local field state re-seeds + * cleanly. Name/Comment edits auto-save: debounced while typing and flushed on + * blur (so clicking away to another snippet commits before this unmounts). Both + * advance the snippet's modified time via the store (§02 → Sort). + */ +function SnippetMeta({ + snippet, + onDuplicate, + onDelete, +}: { + snippet: Snippet; + onDuplicate: () => void; + onDelete: () => void; +}) { + const renameSnippet = useSnippetStore((s) => s.renameSnippet); + const setComment = useSnippetStore((s) => s.setComment); + const [name, setName] = useState(snippet.name); + const [comment, setCommentLocal] = useState(snippet.comment); + + useEffect(() => { + if (name === snippet.name) return; + const t = setTimeout(() => renameSnippet(snippet.id, name), META_AUTOSAVE_MS); + return () => clearTimeout(t); + }, [name, snippet.id, snippet.name, renameSnippet]); + + useEffect(() => { + if (comment === snippet.comment) return; + const t = setTimeout(() => setComment(snippet.id, comment), META_AUTOSAVE_MS); + return () => clearTimeout(t); + }, [comment, snippet.id, snippet.comment, setComment]); + + return ( +
+ + +