From 418bf23cd85649c9f172ea5522ff1673e7c66a7e Mon Sep 17 00:00:00 2001 From: Oleh Omelchenko Date: Tue, 9 Jun 2026 16:51:43 +0300 Subject: [PATCH] Fix documentation drift surfaced by the full-docs consistency review --- docs/IMPLEMENTATION-PLAN.md | 19 +++++- docs/architecture/01-state-and-stores.md | 8 +-- docs/architecture/02-persistence.md | 52 ++++++++-------- docs/architecture/03-modal-system.md | 59 +++++++++---------- .../05-rendering-theming-preview.md | 16 +++-- .../07-naming-and-relationships.md | 24 ++++---- 6 files changed, 96 insertions(+), 82 deletions(-) diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md index 0384bbf..d2be505 100644 --- a/docs/IMPLEMENTATION-PLAN.md +++ b/docs/IMPLEMENTATION-PLAN.md @@ -47,7 +47,7 @@ doc before implementing. | **M0** | Skeleton ✅ | Repo builds, tests run, empty shell renders | — | | **M1** | **MVP core loop** | Author a Vega-Lite snippet, see it render live, it persists | §02, §03A–C, §04, §09A | | **M1.5** | Visual design foundation ✅ | Apply the design language: tokens, IBM Plex, restyled M1 surfaces, chart theme | [arch 09](architecture/09-visual-design.md) | -| **M2** | Editor robustness | Draft/Published, validation, schema autocomplete, fit modes | §03D–E, §04, §07(editor) | +| **M2** | Editor robustness ✅ | Draft/Published, validation, schema autocomplete, fit modes | §03D–E, §04, §07(editor) | | **M3** | Datasets ✅ | Named reusable data + reference resolution in preview | §05, §03F, §09B | | **M4** | Chart Builder | No-JSON chart composition from a dataset | §06 | | **M5** | Settings + Import/Export | Preferences + workspace backup/transfer | §07, §08, §09C | @@ -149,7 +149,7 @@ and the companion `visual-specimen.html`. persisted via the `ui.theme` settings key (a minimal forward-compatible `settings-store` adapter the full M5 UserSettings store will absorb). Hydrated before first paint (no FOUC). Justified: the theme system was already complete, - so dogfooding dark mode through M2–M4 beat waiting for the Settings modal. + so dogfooding dark mode through M2–M4 beat waiting for the full settings UI. **Core** @@ -242,7 +242,7 @@ user-verified in the running app. --- -## M3 · Datasets +## M3 · Datasets ✅ (done) **Goal:** named, reusable data that snippets reference by name; preview resolves the reference. @@ -471,6 +471,19 @@ offline reload works; install as standalone; reduced-motion honored. **⏳ Still --- +## Open divergences (spec ↔ code) + +Surfaced by the full-docs consistency review — each needs a deliberate resolution, +not a silent drift: + +- **Storage-monitor scope.** Spec §02 frames the indicator as the **snippet** storage + budget specifically ("datasets are stored separately with far greater capacity"); the + shipped `readStorageEstimate` instead reports **whole-origin** usage/quota (snippets + + datasets + everything). Decide: scope the estimate to the snippet store, or update spec + §02 to the whole-origin reading. Noted at [arch 02 §6](architecture/02-persistence.md). + +--- + ## Architecture reference The **how** behind each milestone is documented self-containedly in diff --git a/docs/architecture/01-state-and-stores.md b/docs/architecture/01-state-and-stores.md index 3e8ce90..dd025ea 100644 --- a/docs/architecture/01-state-and-stores.md +++ b/docs/architecture/01-state-and-stores.md @@ -34,7 +34,7 @@ hook. The state object holds both **data fields** and **action functions**. import { create } from 'zustand'; import type { UiTheme } from '@core/theme'; // defined in core; charts key off it too -export type ModalName = 'datasets' | 'settings' | 'about' | 'donate' | 'chartBuilder' | 'extract'; +export type ModalName = 'datasets' | 'about' | 'donate' | 'chartBuilder' | 'extract'; export interface AppState { uiTheme: UiTheme; @@ -113,7 +113,7 @@ Services, orchestration, infrastructure, and tests use the store object directly no React involved. This is the property that lets our logic live outside components: ```ts -openModal('settings'); // via the modal coordinator (doc 03) +openModal('datasets'); // via the modal coordinator (doc 03) const theme = useAppStore.getState().uiTheme; // snapshot read const unsub = useAppStore.subscribe((s, prev) => { /* react to changes */ @@ -361,7 +361,7 @@ Subscribers read state and write to `src/app/infrastructure/` adapters (IndexedD ### Theme → document (the minimal example, already wired) ```ts -// src/main.tsx +// src/app/orchestration/theme.ts (wired from main.tsx at startup) const applyTheme = (t: string) => { document.documentElement.dataset.theme = t; }; @@ -475,7 +475,7 @@ reset: () => set({ snippets: [], activeSnippetId: null, draftSpec: '' }); - In components, **select narrowly**; use `useShallow` for object/array selections. Outside components, use `getState()` / `subscribe()`. - Split durable domain state into feature stores (`useSnippetStore`, `useDatasetStore`, - `useSettingsStore`); keep `useAppStore` for thin cross-cutting UI state. + `useUserSettingsStore`); keep `useAppStore` for thin cross-cutting UI state. - Put every shared-state mutation behind a named action on the store so it's testable without a DOM (`getState().action()`). - Do persistence and external sync (IndexedDB, `localStorage`, URL hash, theme) in diff --git a/docs/architecture/02-persistence.md b/docs/architecture/02-persistence.md index 6bc5494..68754ce 100644 --- a/docs/architecture/02-persistence.md +++ b/docs/architecture/02-persistence.md @@ -142,7 +142,7 @@ There are two ways to implement the split; pick per store: 1. **Two object stores** (`datasets` for metadata, `dataset-payloads` keyed by the same id for `data`) — strongest separation; a `getAll` on metadata never touches payload bytes. 2. **One store, lazy field** — keep `data` in the record but set it to `null` on the bulk list load and fetch it per-id on demand. -Astrolabe uses the **lazy-field** approach for datasets (one store, simpler), with `data === null` signalling "summary loaded, payload not yet." +**Target, not yet built.** Datasets currently load in full via `loadDatasets()` (`infrastructure/dataset-store.ts`); the lazy-field approach below is the **intended direction** for when dataset size demands it, not a description of shipped code. The plan: keep `data` in the one store but set it to `null` on the bulk list load (`data === null` = "summary loaded, payload not yet") and fetch per-id on demand. ```ts // src/app/infrastructure/dataset-store.ts @@ -351,44 +351,46 @@ Astrolabe has three tiers with different capacities and risk profiles: | Tier | Backing | Holds | Budget & behavior | | -------------------- | -------------------- | ---------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | **Snippet store** | IndexedDB `snippets` | All snippet records | Practical budget ~**5 MB**. A storage monitor estimates usage and surfaces a warning as it fills. Snippets are user-authored and irreplaceable, so we fail **loudly**. | -| **Dataset store** | IndexedDB `datasets` | All dataset payloads | Separate, **high-capacity**; suited to large payloads. Lazily loaded (§3). | +| **Dataset store** | IndexedDB `datasets` | All dataset payloads | Separate, **high-capacity**; suited to large payloads. Loaded in full today; lazy loading is a §3 target. | | **Settings & prefs** | localStorage | `UserSettings` + app/UI prefs (§5) | Small; effectively unbounded for this use. | Splitting snippets and datasets into separate stores means a few large datasets can't crowd out the snippet budget, and the snippet monitor can report a meaningful "how full is my library" number without summing dataset bytes. ### Estimating usage -Use the Storage Manager API where available, with a manual byte-sum fallback for the snippet tier so the ~5 MB budget is always reportable. +Read the origin's **Storage Manager** estimate (`navigator.storage.estimate()` → `usage`/`quota`) and derive a presentation-ready summary in the **pure core**, so the thresholds are unit-tested without a browser and the adapter only does I/O. When the API is missing or rejects, the summary is marked unusable rather than guessed. ```ts -// src/app/infrastructure/storage-monitor.ts -export interface StorageReport { - snippetBytes: number; // estimated bytes used by the snippet tier - snippetBudget: number; // 5 MB practical budget - ratio: number; // snippetBytes / snippetBudget, clamped to >= 0 - warn: boolean; // ratio crossed the warning threshold +// src/core/storage-estimate.ts — pure, unit-tested (no I/O) +export type StorageLevel = 'ok' | 'warning' | 'critical'; + +export interface StorageSummary { + usedBytes: number; + quotaBytes: number; + fraction: number; // usedBytes / quotaBytes, clamped 0..1 + level: StorageLevel; // escalates from `fraction` + available: boolean; // false when usage/quota are missing/invalid } -const SNIPPET_BUDGET = 5 * 1024 * 1024; -const WARN_AT = 0.8; +export const WARNING_THRESHOLD = 0.8; // begin warning +export const CRITICAL_THRESHOLD = 0.95; // escalate to "nearly full" -export async function reportSnippetUsage(snippets: Snippet[]): Promise { - // Cheap, deterministic estimate: serialize the records we hold. - const snippetBytes = snippets.reduce((n, s) => n + new Blob([JSON.stringify(s)]).size, 0); - const ratio = snippetBytes / SNIPPET_BUDGET; - const report: StorageReport = { - snippetBytes, - snippetBudget: SNIPPET_BUDGET, - ratio, - warn: ratio >= WARN_AT, - }; - if (report.warn) { - console.warn(`[storage] snippet tier ${(ratio * 100).toFixed(0)}% of ${SNIPPET_BUDGET} bytes`); - } - return report; +export function summarizeStorage(input: { usage?: number; quota?: number }): StorageSummary { + // Clamp the fraction, derive the level, and mark `available: false` when + // usage/quota are absent or non-finite — we don't warn on data we lack. +} + +// src/app/infrastructure/storage-estimate.ts — the only browser-touching part +export async function readStorageEstimate(): Promise { + const storage = typeof navigator !== 'undefined' ? navigator.storage : undefined; + if (!storage || typeof storage.estimate !== 'function') return summarizeStorage({}); + const { usage, quota } = await storage.estimate(); + return summarizeStorage({ usage, quota }); } ``` +> **Open divergence (spec §02 ↔ code).** Spec §02 frames the monitor as the **snippet** storage budget specifically ("datasets are stored separately with far greater capacity"). The shipped `readStorageEstimate` instead reports **whole-origin** `usage`/`quota` (snippets + datasets + everything the origin holds). Reconcile deliberately — either scope the estimate to the snippet store, or update spec §02 to describe the whole-origin reading. + ### Fail loudly, never silently lose data When a write would exceed quota, IndexedDB rejects with a `QuotaExceededError`. The adapter must **propagate** this so the UI can tell the user to export and prune — it must never swallow the error and pretend the save succeeded. diff --git a/docs/architecture/03-modal-system.md b/docs/architecture/03-modal-system.md index 5fd012a..77446d0 100644 --- a/docs/architecture/03-modal-system.md +++ b/docs/architecture/03-modal-system.md @@ -36,11 +36,12 @@ registry, coordinator, and shell are exhaustively type-checked. // src/app/modals/types.ts export type ModalName = | 'datasets' // Datasets manager (list / detail / new-dataset form) - | 'settings' // Appearance, editor, performance, formatting prefs | 'about' // About & Help (shortcuts, privacy) | 'donate' // Donate | 'chartBuilder' // Visual no-JSON chart composition for a dataset | 'extract'; // Extract inline spec data into a new dataset +// Settings is NOT a modal — preferences are distributed to per-pane disclosure +// popovers (spec §01C/§07; see components/SettingsPopover). export type ActiveModal = ModalName | null; ``` @@ -76,8 +77,8 @@ export interface ModalConfig { init?: (arg?: string) => void; /** Serializable snapshot of in-progress edits, used to detect unsaved - * changes on close. OMIT for modals that apply immediately (settings, - * about, donate) — omission opts out of the discard-confirmation. */ + * changes on close. OMIT for modals with no in-progress edits to guard + * (about, donate) — omission opts out of the discard-confirmation. */ getState?: () => Record | null; /** Whether the modal's primary action (Save / Apply) should be blocked @@ -88,16 +89,24 @@ export interface ModalConfig { getError?: () => string | null; /** Whether this modal is reflected in the URL hash (back/forward, reload - * restore). Datasets and Chart Builder are navigable; Donate is not. */ + * restore). Datasets and Chart Builder are navigable; About/Donate/Extract + * are not (spec §01E). */ isUrlNavigable?: boolean; } ``` +> **Shipped divergence.** The implemented registry (`modals/modal-registry.ts`) **omits +> `hasError`/`getError`**: each modal renders its **own action row** inside its body (the +> multi-view Datasets manager doesn't fit a single shell-level Save/Cancel), so validity is +> each modal's own concern. The shipped `ModalConfig` keeps only `getState` (close-time +> unsaved-change detection) plus `init`/`isUrlNavigable`. The generic-footer sketch through +> the rest of this section is retained as the simpler pattern for a single-action modal — +> treat it as illustrative, not a description of current code. + ### Example entries ```ts import { DatasetsModal } from '../components/DatasetsModal'; -import { SettingsModal } from '../components/SettingsModal'; import { ChartBuilderModal } from '../components/ChartBuilderModal'; import { ExtractModal } from '../components/ExtractModal'; import { DonateModal } from '../components/DonateModal'; @@ -105,7 +114,6 @@ import { AboutModal } from '../components/AboutModal'; import { useDatasetStore } from '../stores/DatasetStore'; import { useChartBuilderStore } from '../stores/ChartBuilderStore'; import { useExtractStore } from '../stores/ExtractStore'; -import { useSettingsStore } from '../stores/SettingsStore'; // Per-modal transient state lives in the relevant feature store; the registry // reads it via `getState()` (Zustand), never through component hooks. @@ -152,22 +160,8 @@ export const MODAL_REGISTRY: Record = { getError: () => (useExtractStore.getState().name.trim() ? null : 'modals.extract.nameRequired'), }, - // Applies immediately — no getState, so closing never prompts. - settings: { - name: 'settings', - title: 'modals.settings.title', - component: SettingsModal, - isUrlNavigable: true, - init: () => useSettingsStore.getState().loadFromPrefs(), - }, - - // Pure info modals — no state, no validity, not navigable for donate. - about: { - name: 'about', - title: 'modals.about.title', - component: AboutModal, - isUrlNavigable: true, - }, + // Pure info modals — no state, no validity, not navigable. + about: { name: 'about', title: 'modals.about.title', component: AboutModal }, donate: { name: 'donate', title: 'modals.donate.title', component: DonateModal }, }; ``` @@ -297,9 +291,9 @@ export function hasUnsavedChanges(): boolean { ``` The snapshot is taken once on open and compared on close. Modals without -`getState` (settings, about, donate) snapshot to `null`, so `hasUnsavedChanges` -short-circuits and they close instantly — correct, because they either apply -immediately or hold nothing to lose. +`getState` (about, donate) snapshot to `null`, so `hasUnsavedChanges` +short-circuits and they close instantly — correct, because they hold nothing to +lose. ### Validity passthrough @@ -512,8 +506,8 @@ entry. Three properties force the split: component; its title/message/labels are supplied at the call site. There's nothing to register. - **Stacks _above_ a feature modal.** The discard-changes prompt must appear over - an already-open Datasets/Settings modal — which directly violates the feature - layer's "at most one open" rule. So confirmations live on a higher z-layer + an already-open feature modal (e.g. Datasets or Chart Builder) — which directly + violates the feature layer's "at most one open" rule. So confirmations live on a higher z-layer (`z-index: 1000`, above the future modal shell). - **Not navigable.** A confirmation is never a URL destination or a reload-restore target; it only exists for the duration of one decision. @@ -579,15 +573,16 @@ tool. The coordinator is the join point for navigation: - `openModal` calls `syncModalToUrl`; navigable modals write a hash - (`#datasets`, `#datasets/dataset-`, `#datasets/dataset-/build`, - `#settings`). Non-navigable modals (donate) write nothing. + (`#datasets`, `#datasets/dataset-`, `#datasets/dataset-/build`). + Non-navigable modals (about, donate, extract) write nothing. - `closeModal` calls `clearModalFromUrl`, returning to the underlying workspace hash. - On load, the URL restorer reads the hash and calls `openModal(name, arg)` to rehydrate the right modal and sub-target. -- The global key handler maps `Cmd/Ctrl+K` → `toggleDatasets()`, - `Cmd/Ctrl+,` → `openModal('settings')`, and `Escape` → `closeModal()` (the - Escape binding is a no-op when `activeModal` is `null`). +- The global key handler maps `Cmd/Ctrl+K` → `toggleDatasets()` and `Escape` → + `closeModal()` (a no-op when `activeModal` is `null`). `Cmd/Ctrl+,` opens the + editor **settings popover**, not a modal (`openSettingsPopover('editor-settings')`; + settings are distributed — spec §01C/§07). Because all of these call the same coordinator functions, browser Back/Forward, keyboard shortcuts, and in-app triggers stay consistent — they diff --git a/docs/architecture/05-rendering-theming-preview.md b/docs/architecture/05-rendering-theming-preview.md index b324abd..80176ef 100644 --- a/docs/architecture/05-rendering-theming-preview.md +++ b/docs/architecture/05-rendering-theming-preview.md @@ -335,10 +335,12 @@ 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 service above is a **sketch** — its `useEditorStore`/`useSettingsStore` are +> illustrative placeholders; the real inputs are `useAppStore` (`previewFitMode` + +> `uiTheme`) and `useSnippetStore` (draft text / `bufferEpoch`). 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): @@ -403,8 +405,10 @@ It does two deterministic things, on a **deep copy** of the spec: referenced dataset's actual contents (inline values, raw CSV/TSV text, or a URL reference), recursing into layered/concat/child sub-specs. 2. **Fit-mode sizing** — rewrites `width`/`height` per the active fit mode using - Vega-Lite's `"container"` keyword (Original = untouched; Width/Height/Full set - the corresponding dimension(s) to `"container"`), recursing the same way. + Vega-Lite's `"container"` keyword (Original = untouched; Width sets + `width:"container"` and **removes** `height`; Height sets `height:"container"` + and **removes** `width`; Full sets both — see spec §04 → Fit-mode sizing), + recursing the same way. This is _content_ preparation, not embedding, and it is fully covered by the _Live Preview_ spec. The only invariant this doc cares about: diff --git a/docs/architecture/07-naming-and-relationships.md b/docs/architecture/07-naming-and-relationships.md index ea5860f..7824db1 100644 --- a/docs/architecture/07-naming-and-relationships.md +++ b/docs/architecture/07-naming-and-relationships.md @@ -301,10 +301,10 @@ silent. Crucially, names are reserved _as we go_ — within a single import, two incoming `Sales` datasets become `Sales 2` and `Sales 3`, not two `Sales 2`. ```ts -// src/app/services/ImportService.ts +// src/core/import-normalize.ts -import { makeUniqueName } from '../../core/naming'; -import type { Dataset } from '../../core/types'; +import { makeUniqueName } from './naming'; +import type { Dataset } from './dataset'; export interface DatasetRename { from: string; @@ -492,15 +492,15 @@ export function renameDatasetEverywhere( ## 7. Where things live -| Concern | Location | Pure? | Tested | -| ------------------------------------------------------------------------ | ----------------------------------------- | ------------------------------ | ---------------- | -| `makeUniqueName`, `isNameTaken` | `src/core/naming.ts` | yes | unit | -| `extractDatasetRefs`, `recomputeDatasetRefs` | `src/core/spec-refs.ts` | yes | unit | -| `renameDatasetInSpec` | `src/core/spec-refs.ts` | yes | unit | -| `snippetsReferencingDataset`, `datasetUsageCounts` (reverse-lookup scan) | `src/core/relationships.ts` | yes | unit | -| `findSnippetsReferencingDataset`, usage count (snapshot wrappers) | `src/app/services/RelationshipService.ts` | no (reads store) | integration | -| `renameDatasetEverywhere` → `{ updated }` | `src/app/services/RelationshipService.ts` | no (mutates stores) | integration | -| `dedupeIncomingDatasetNames` | `src/app/services/ImportService.ts` | nearly (uses `makeUniqueName`) | unit/integration | +| Concern | Location | Pure? | Tested | +| ------------------------------------------------------------------------ | ----------------------------------------- | ------------------- | ----------- | +| `makeUniqueName`, `isNameTaken` | `src/core/naming.ts` | yes | unit | +| `extractDatasetRefs`, `recomputeDatasetRefs` | `src/core/spec-refs.ts` | yes | unit | +| `renameDatasetInSpec` | `src/core/spec-refs.ts` | yes | unit | +| `snippetsReferencingDataset`, `datasetUsageCounts` (reverse-lookup scan) | `src/core/relationships.ts` | yes | unit | +| `findSnippetsReferencingDataset`, usage count (snapshot wrappers) | `src/app/services/RelationshipService.ts` | no (reads store) | integration | +| `renameDatasetEverywhere` → `{ updated }` | `src/app/services/RelationshipService.ts` | no (mutates stores) | integration | +| `dedupeIncomingDatasetNames` | `src/core/import-normalize.ts` | yes | unit | The dividing line: anything that takes plain data and returns plain data is **core** and unit-tested in isolation; anything that reaches into a Zustand store