mirror of
https://github.com/olehomelchenko/astrolabe.git
synced 2026-08-08 02:02:33 +00:00
Docs: lesson roadmap; knip learn entry; drop dead export
This commit is contained in:
@@ -0,0 +1,411 @@
|
||||
# Engineering Review — 2026-07-02
|
||||
|
||||
> Point-in-time record. Full-project engineering review requested by the maintainer, with a
|
||||
> specific question attached: _"as we work session to session, we may forget to look back
|
||||
> and see the bigger picture — the end-of-session skill checks aim to mitigate it, but I'm
|
||||
> not sure to what extent it is successful."_
|
||||
>
|
||||
> **Method:** five independent clean-context reviewers (code quality, test suite,
|
||||
> documentation, build/tooling/delivery, cross-session coherence), each required to cite
|
||||
> file/line/commit evidence, followed by an adversarial verification wave that reproduced
|
||||
> every load-bearing claim (rebuilt the bundle, re-ran greps and git archaeology, hand-walked
|
||||
> the flagged logic, re-checked the live site). Findings below are only those that survived
|
||||
> verification; where a reviewer's number was wrong, the corrected number is used.
|
||||
> Reviewed state: working tree at `c5e4c4c` plus the uncommitted spec-params/editor WIP.
|
||||
|
||||
## Verdict
|
||||
|
||||
The codebase itself is in excellent shape — the architecture contract holds under grep, not
|
||||
just in prose, and the per-session review machinery demonstrably works at the session scale.
|
||||
The problems are concentrated at two horizons the per-session view cannot see: **delivery**
|
||||
(one measurable production defect: the marketing landing executes 1.3 MB gzipped of Monaco +
|
||||
Vega it never uses) and **look-back**. The maintainer's fear is confirmed, mechanically: no
|
||||
instrument — on-demand or scheduled — owns the whole-project view, so coherence work happens
|
||||
only when a session happens to collide with it. (Maintainer clarification after the first
|
||||
draft: the eng-council sweep and the ux-second-pass batch were designed as on-demand
|
||||
session guardrails, not periodic instruments — which sharpens the finding rather than
|
||||
softening it: the on-demand instruments have each fired once, and nothing at all runs on a
|
||||
cadence.)
|
||||
|
||||
## Scorecard
|
||||
|
||||
| Dimension | Grade | One-line summary |
|
||||
| ----------------------- | ----- | ------------------------------------------------------------------------------------- |
|
||||
| Code quality | A | Zero `any`/suppressions, layering verifies by grep, error discipline is real |
|
||||
| Test suite | A− | 1,366 tests, uniform harness, deterministic; one committed coverage hole that matters |
|
||||
| Documentation | A− | Symbol-level accuracy at scale; drift is localized fossils plus one governance gap |
|
||||
| Build / delivery | B | Strong local gates; landing bundle defect shipped because no gate measures output |
|
||||
| Cross-session coherence | B− | A− for per-session machinery, D for look-back machinery |
|
||||
|
||||
## Critical findings (all independently verified)
|
||||
|
||||
### C1. The landing and /learn/ eagerly load and execute Monaco (940 kB gz) and Vega (285 kB gz)
|
||||
|
||||
Verified on the live site and reproduced from a clean build. `dist/index.html` modulepreloads
|
||||
both chunks and the landing entry's static import graph executes them
|
||||
(`main-*.js` ends with bare `import"./vega-*.js";import"./monaco-*.js"`). `/learn/` is worse:
|
||||
both arrive as direct `<script type="module">` tags. Landing eager JS today: ~4.7 MB raw /
|
||||
~1.30 MB gzip; the intended payload is ~255 kB raw / ~81 kB gzip — a 16× reduction available.
|
||||
|
||||
The source-level lazy-loading discipline is correct (`LandingChart.tsx` dynamic-imports
|
||||
chart-renderer; nothing in `src/landing`/`src/learn` mentions Monaco). Two bundling-level
|
||||
causes defeat it:
|
||||
|
||||
- **Preload-helper placement.** The object-form `manualChunks` in `vite.config.ts` leads
|
||||
Rollup to emit Vite's `__vitePreload` helper _inside_ the `monaco` chunk;
|
||||
`LandingChart-*.js` begins `import{_ as h}from"./monaco-*.js"`, so every chunk that uses
|
||||
`import()` statically depends on all of Monaco. (Attribution to the object form
|
||||
specifically is plausible-but-untested; the observed mechanism is fully reproduced.)
|
||||
- **`vega-scale` edge.** `Landing.tsx` → `@core/vega-themes` → `theme-controls.ts:27`
|
||||
`import { scheme } from 'vega-scale'` merges into the `vega` manual chunk, dragging all of
|
||||
Vega into the landing's static graph. The comment at `theme-controls.ts:21-22` ("keeps the
|
||||
umbrella vega out of core") is true at source level and defeated by chunking. The
|
||||
`examples` chunk has a second, independent edge into the vega chunk.
|
||||
|
||||
**Fix:** break the `vega-scale` edge (lazy-import or move scheme resolution off the
|
||||
landing-reachable path); switch to function-form `manualChunks` and confirm the helper lands
|
||||
in a shared micro-chunk; and — the durable part — add a ~10-line post-build assertion that
|
||||
`dist/index.html` and `dist/learn/index.html` reference neither `monaco-*` nor `vega-*`.
|
||||
That assertion is the missing gate for this entire regression class.
|
||||
|
||||
### C2. The project's operating regime is recorded nowhere the machinery reads
|
||||
|
||||
The 2026-06-10 phase shift — spec follows code; docs/spec is no longer the authoritative
|
||||
contract — appears in exactly one repo location: `docs/exploration/chart-builder-enhancement-scope.md:673`,
|
||||
inside the directory whose charter says nothing there is kept current. Meanwhile CLAUDE.md
|
||||
("authoritative behavioral specification… This is the contract"), AGENTS.md ("Spec is the
|
||||
contract"), SOUL.md, arch 00 ("the spec wins"), and `.claude/skills/alignment/SKILL.md`
|
||||
rules #3/#17 all still assert the old regime. Two reviewers independently converged on this,
|
||||
and the verification wave reproduced the greps.
|
||||
|
||||
This is the worst coherence defect because it corrupts the corrective machinery itself: the
|
||||
wrap-up protocol's whole design is clean-context subagents judging against the recorded
|
||||
contract — and the recorded contract is wrong. **Fix:** one paragraph in CLAUDE.md/AGENTS.md
|
||||
stating the regime (spec is descriptive, kept current with code; on conflict fix the spec),
|
||||
softened phrasing in `docs/spec/README.md` and arch 00, and an update to the alignment skill.
|
||||
|
||||
### C3. The spec stopped absorbing new behavior at a verifiable cutover (2026-06-21 → 06-25)
|
||||
|
||||
Adjudicated timeline, commit-dated: through `d76a7a5` (06-21) every feature landed with its
|
||||
spec section in the same commit (Theme Builder, FontAsset, data inspector, onboarding, URL
|
||||
datasets — all specified at full fidelity). From `c19857b` (06-25) onward the discipline
|
||||
inverted: 12 of 13 feature commits through 06-30 updated `docs/architecture/` in-commit and
|
||||
**zero** touched `docs/spec/`. Unspecified user-facing surfaces at HEAD:
|
||||
|
||||
- the entire **/learn/** section (`grep -ril learn docs/spec/` → nothing);
|
||||
- the **composition wireframe** (drag reorder / pull-out / stack / Simplify — zero matches);
|
||||
- **editor transform actions and CodeLens scaffolds** (wrap/simplify/add-view, add-transform);
|
||||
- the **multi-view Data Inspector view picker** (`docs/spec/04-live-preview.md:84` references
|
||||
"the chosen view's table" — the chooser it refers to is specified nowhere);
|
||||
- the current uncommitted params-scaffolding work continues the pattern (arch 08 only).
|
||||
|
||||
Even under the spec-follows-code regime this is debt: the "then amend the spec to match"
|
||||
half of the bargain has not happened for ten days of features. Root cause is structural, not
|
||||
negligence: `/doc-update` flushes what a session remembers and `/alignment` reviews diffs —
|
||||
no instrument owns "the spec describes the product." Alignment rule #17 covers removed/moved
|
||||
surfaces only, not never-specified additions.
|
||||
|
||||
## Significant findings
|
||||
|
||||
### S1. No instrument owns the big picture on any cadence
|
||||
|
||||
- **Eng-council sweep:** once, 2026-06-12 (`0e225d7`). Since then non-test source grew
|
||||
~**+76%** (16.8k → 29.7k ts/tsx LOC; ~21.3k → ~37k all-source). `docs/codebase-metrics.md`,
|
||||
whose stated purpose is the trend, holds two same-day rows. Specific unswept accretion: the
|
||||
six-module `spec-*` service family plus `editor-snippet`/`editor-cursor-lens` (~1,900 LOC,
|
||||
mostly post-sweep) — which alignment rule #16 already notes "has grown by copy-paste twice."
|
||||
- **Batched council pass over `docs/ux-second-pass.md`:** once, 2026-06-13 (`92bfe88`,
|
||||
76→25 lines — the design worked). The file has regrown 25→48; oldest open item is from
|
||||
06-16.
|
||||
- **TODO gardening:** never. Lifecycle is bimodal — 56 added / 43 removed over history
|
||||
(~71–77% resolution, usually within 0–3 days, at least 10 by dedicated commits), but every
|
||||
TODO that survived its first week is still alive. Oldest two are from 06-12:
|
||||
`src/app/modals/ModalCoordinator.ts:36` (a **latent init-ordering bug**, not polish) and
|
||||
`ChartBuilderModal.tsx:524`. The open set is increasingly the hard/ambiguous residue no
|
||||
session claims.
|
||||
|
||||
**Fix direction (maintainer to ratify):** give each instrument a trigger — re-sweep when
|
||||
source LOC grows ~25% past the last metrics row; batch council pass when ux-second-pass open
|
||||
items exceed a count or age past two weeks; a spec-reconciliation clause in `/doc-update`
|
||||
("did this session add user-facing behavior? name the spec section or write it"); TODO
|
||||
gardening as a standing sweep agenda item.
|
||||
|
||||
### S2. The scaffold insertion math is untested, duplicated, and validity-critical
|
||||
|
||||
Verified: `runAddTransform` (`spec-transform-scaffold.ts:130-146`, committed `c5e4c4c`) and
|
||||
`runAddParam` (`spec-param-scaffold.ts:109-142`, WIP) compute insert offsets, indentation,
|
||||
and the trailing-comma decision; a bug here writes **invalid JSON into the user's editor**.
|
||||
No test exercises the composition — no `SpecEditor.test.tsx` exists, so there is no indirect
|
||||
path either. The create-property block is a verbatim 7-line duplicate between the two files;
|
||||
the completion providers share ~35 structurally identical lines. Hand-walking the current
|
||||
logic found **no live bug** — this is a coverage hole, not a defect — but a future inversion
|
||||
of the ternary or offset drift ships silently. The repo's own TODOs
|
||||
(`spec-param-scaffold.ts:33`, `spec-params.test.ts:6`) already point at the fix: extract a
|
||||
neutral core module (working name `spec-snippet`) computing
|
||||
`(text, host offset, existing-array?) → {insertOffset, snippetText}` and table-test it over
|
||||
empty-host / host-with-following-key / compact-one-line / host-as-last-property.
|
||||
|
||||
Related: `spec-transform-actions.ts` (663 lines, largest service, no test file) carries pure
|
||||
unexported helpers with real branching — `reindent`, `defaultFacet`, `defaultRepeat` (:106,
|
||||
:123, :136) — one layer short of tested core, mitigated by `buildNext` being a thin
|
||||
dispatcher over tested core wrappers.
|
||||
|
||||
### S3. Documentation drift cluster (6/6 claims verified)
|
||||
|
||||
All in otherwise highly accurate docs; each is a fossil a maintainer would act on:
|
||||
|
||||
1. **5 MB budget fossil.** `docs/spec/09-data-model.md` §E and `08-import-export.md` describe
|
||||
a budget-with-fill-warnings storage monitor that spec 02/10, arch 02 §6, and the code all
|
||||
contradict (the monitor is a composition breakdown; the only 5 MB logic is the import
|
||||
pre-check in `transfer.ts:42`). Arch 02:445's "80% threshold" Do-line contradicts the same
|
||||
doc's line 425 and matches nothing in code.
|
||||
2. **"Not installable" is false.** Arch 10:625 claims the manifest ships no icons; four icons
|
||||
ship (`vite.config.ts:85-89`, files in `public/`), and manual-verification actively tests
|
||||
install.
|
||||
3. **The ajv layer was never built.** Arch 08 presents it as planned-M2 and claims "we
|
||||
deliberately do better: map ajv errors to editor positions" — no such module exists; ajv
|
||||
isn't even in `package.json`. The current WIP on arch 08 does not touch this.
|
||||
4. **Wrong store name in three docs.** Arch 01/03/04 name `useSettingsPopoverStore` /
|
||||
`openSettingsPopover`; the code is `usePopoverStore` / `openPopover`
|
||||
(`PopoverStore.ts:22,30`, called from `EventRouter.ts:96`). Grep by the documented name
|
||||
finds nothing.
|
||||
5. **Arch 03's "closed union" is stale.** Five modals listed; `modals/types.ts` has six
|
||||
(`themeBuilder`). Same doc's add-a-modal checklist instructs `hasError`/`getError` on
|
||||
`ModalConfig` — fields the real type doesn't have; following it fails typecheck.
|
||||
6. **Spec 09's UserSettings omits `ui.dataInspectorOpen`/`ui.dataInspectorHeight`**, which
|
||||
spec 04 mandates and `settings-store.ts:158-183` persists (they're also absent from
|
||||
`core/settings.ts`'s type — the code half of the same gap).
|
||||
|
||||
### S4. Delivery gates never observe build output, and caching is misconfigured
|
||||
|
||||
- All quality gates live in the (excellent) pre-commit hook — lint-staged + full typecheck +
|
||||
the whole test suite — but nothing anywhere runs or inspects `vite build`. Cloudflare's
|
||||
build fails safe on compile errors (previous deploy stays live) but silently; the escape
|
||||
class is green-but-wrong output, which is exactly how C1 shipped. Cheapest durable fix: the
|
||||
C1 post-build assertion in the `build` script; optionally a minimal CI workflow
|
||||
(typecheck + lint + test + build + assertion) to cover `--no-verify` and web edits.
|
||||
- Hashed `/assets/*` are served `cache-control: max-age=14400, must-revalidate` (Cloudflare
|
||||
Pages default; no `public/_headers` exists). Content-hashed files are the textbook
|
||||
`max-age=31536000, immutable` case; today every returning visitor revalidates a 3.6 MB
|
||||
chunk every 4 hours. HTML cache headers are correct.
|
||||
|
||||
### S5. Module-size outliers in the chart builder
|
||||
|
||||
`src/core/chart-builder.ts` (1,851 lines: catalog + rules + smart defaults + assembly/parse)
|
||||
and `ChartBuilderModal.tsx` (1,653 lines, ~25 private subcomponents; 2.1× the next-largest
|
||||
component). Internally cohesive, well-documented — maintainability risk, not defect. Split on
|
||||
next substantive touch (`chart-builder/{catalog,rules,defaults,assemble}.ts`; hoist the
|
||||
modal's sections into sibling files).
|
||||
|
||||
### S6. Smaller verified items
|
||||
|
||||
- `noUncheckedIndexedAccess` is off in a parser-heavy codebase (jsonc-parser node walking in
|
||||
`spec-params.ts`, `spec-cursor.ts`, `spec-insert.ts`) that is exactly its target class.
|
||||
Enable and sweep; if parser noise proves disproportionate, record the rejection.
|
||||
- `ajv` is imported by `spec-params.test.ts:1` but undeclared — a phantom transitive
|
||||
dependency; add it to devDependencies (found during verification).
|
||||
- `npm audit`: monaco 0.54.0 bundles a vulnerable DOMPurify (moderate; fix is 0.55.1, flagged
|
||||
breaking — plan deliberately); esbuild advisory is dev-only/Windows-only.
|
||||
- Landing/learn light-dark toggle duplicated since 06-25, TODO'd in both copies and reworded
|
||||
06-28 instead of extracted — a breadcrumb treated as the deliverable.
|
||||
|
||||
## Minor findings
|
||||
|
||||
- Per-editor CodeLens providers register on the global `'json'` language
|
||||
(`editor-cursor-lens.ts:83`) while closing over one editor's cursor — safe with today's
|
||||
single editor (verified: one `monaco.editor.create`), latent cross-wiring if a second JSON
|
||||
editor appears. One guard line: `if (model !== editor.getModel()) return`.
|
||||
- One avoidable non-null assertion in the WIP: `spec-transform-scaffold.ts:176`.
|
||||
- `navigator.platform` (deprecated) in `EventRouter.ts:38`.
|
||||
- Hooks are untested as a class; `useFocusTrap` (a11y-load-bearing, shared by all overlays)
|
||||
is the one worth a cheap happy-dom test. `ModalCoordinator`'s open/close/replace sequencing
|
||||
likewise (~5 tests).
|
||||
- No coverage reporting exists; for a philosophy that is explicitly proportional ("core
|
||||
hardest"), nothing measures the proportion. `@vitest/coverage-v8` scoped to
|
||||
`src/core` + `src/app/{stores,services}`, no thresholds, visibility only.
|
||||
- localStorage write failures are console-only (`settings-store.ts:80`, `ux-prefs.ts:80`) —
|
||||
judged an acceptable, explicit low-stakes swallow; noted so the asymmetry with the
|
||||
IndexedDB path stays a decision.
|
||||
- `/learn/` is reachable only from the landing; nothing in the app links it. Deliberate or
|
||||
forgotten is unrecorded — which is itself the gap.
|
||||
- Doc/housekeeping nits: pre-commit hook is stronger than AGENTS.md documents (says eslint;
|
||||
runs typecheck + tests too — drift in the good direction); PWA precache carries ~150–200 kB
|
||||
of landing/learn assets the `/app/`-scoped SW can never serve; spec 00 still calls settings
|
||||
a modal; spec 02 overstates the "imported" tag (only foreign/older shapes are tagged — spec
|
||||
08 has it right); `docs/embedding-vega-lite.md` (447 lines) is orphaned — index it with a
|
||||
charter line or move it; `core/settings.ts:64` and `HeaderControls.tsx:2` cite the old
|
||||
pre-move path of the theming scope doc; IMPLEMENTATION-PLAN files a completed item under
|
||||
"Next"; empty `ops/` directory; `vega-themes` pinned exact with no recorded rationale;
|
||||
manual-verification.md lacks checks for the shipped theming/export surface (Theme Builder
|
||||
gallery, font upload incl. variable fonts, PNG/SVG export, clipboard).
|
||||
|
||||
## Strengths (verified, and worth keeping deliberate)
|
||||
|
||||
1. **The architecture contract verifies by grep, both directions.** No browser APIs, React,
|
||||
or Monaco in `src/core/`; storage APIs confined to `infrastructure/`; landing/learn import
|
||||
no stores/orchestration/components.
|
||||
2. **TypeScript rigor at the top of the distribution.** Zero `any`, zero
|
||||
`@ts-ignore`/`@ts-expect-error`, exactly one non-null assertion across ~30k lines;
|
||||
type-aware ESLint (incl. `no-floating-promises`) passes clean; every intentionally
|
||||
unawaited promise is an explicit `void` with a comment.
|
||||
3. **Error-handling discipline is real.** Every swallowed catch inspected carries a rationale
|
||||
naming the sanctioned fallback; real failures route to notifications with actionable copy;
|
||||
quota errors are normalized and surfaced.
|
||||
4. **Concurrency/resource engineering above typical app code.** `LivePreview`'s generation
|
||||
token + render mutex (with reasoning written down), full Monaco disposable cleanup, the
|
||||
finalize-before-unsubscribe edge handled in chart-renderer.
|
||||
5. **The test suite is engineered, not accumulated.** 1,366 tests / 8.5s, zero snowflake
|
||||
harnesses across all 17 component test files, injected clocks and fake timers keyed to
|
||||
exported constants, fresh `IDBFactory` per test (including interrupted-upgrade
|
||||
self-healing), tests that assert contracts with the why inline
|
||||
(`spec-refs.test.ts`, `chart-renderer.test.ts`).
|
||||
6. **Session-scale coherence is solved.** All 12 stores share one canonical shape and cite
|
||||
their canonical sibling in doc comments; modals go through one registry/coordinator/shell;
|
||||
subtraction happens (`PreviewStore` deleted when obsoleted). The "written by strangers"
|
||||
failure mode is absent.
|
||||
7. **The breadcrumb→fix pipeline works.** At least 10 TODOs resolved by dedicated commits
|
||||
(quota rollback, hash routing, splitter ARIA, field-name escaping…); the eng-council→law
|
||||
loop is real (sweep findings became alignment rules #15–#17; skills keep evolving).
|
||||
8. **Docs are accurate at symbol level at scale** — of ~35 spot-checked claims (constants,
|
||||
record shapes, hash grammar, keyboard maps, API surfaces), nearly all exact — and the
|
||||
"shipped divergence" fencing discipline keeps pedagogical sketches from lying.
|
||||
9. **PWA configuration is best-practice**: `registerType: 'prompt'` properly consumed with a
|
||||
durable update toast, SW scope narrowed to `/app/`, sophisticated font precache strategy
|
||||
with reasoning in the config.
|
||||
|
||||
## Recommended sequence
|
||||
|
||||
1. **Record the regime** (C2): CLAUDE.md/AGENTS.md paragraph + alignment-skill update +
|
||||
soften spec README/arch 00. Smallest fix, unblocks every future clean-context review.
|
||||
_Done same day: CLAUDE.md, AGENTS.md, SOUL.md, spec README, and alignment rules #3/#17
|
||||
now state the spec-follows-code regime; #17 extended to cover never-specified additions._
|
||||
2. **Fix the landing bundle and add the post-build assertion** (C1): the only
|
||||
production-visible defect, and the assertion closes the gate gap (S4) for free. Add
|
||||
`public/_headers` for immutable assets while in there.
|
||||
_Done same day: `schemeColors` split into `core/scheme-colors.ts`; function-form
|
||||
`manualChunks` with explicit homes for the preload helper and the shared light packages
|
||||
(vega-themes, vega-expression, vega-util, json-stringify-pretty-compact — Rollup was
|
||||
absorbing each into the nearest vendor chunk); `scripts/check-light-entries.mjs` gates
|
||||
every build; `public/_headers` ships immutable caching. Landing eager JS measured
|
||||
~98 kB gzipped, down from ~1,300 kB; all three entries smoke-tested on the production
|
||||
build (charts + Monaco render, zero console errors)._
|
||||
3. **Spec back-fill sprint** (C3): specify /learn/, the wireframe, editor actions/scaffolds,
|
||||
and the view picker; add the spec-reconciliation clause to `/doc-update`.
|
||||
4. **Give the periodic instruments triggers** (S1) and run the overdue ones: an eng-council
|
||||
re-sweep (which naturally absorbs S5's splits and the `spec-*` family consolidation), a
|
||||
ux-second-pass batch pass, and TODO gardening — starting with `ModalCoordinator.ts:36`.
|
||||
5. **Extract and test the scaffold insertion core** (S2) before the params WIP is committed —
|
||||
the TODOs already name the module.
|
||||
6. **Doc drift batch** (S3 + minors): one session, mostly deletions of fossils.
|
||||
|
||||
---
|
||||
|
||||
## Addendum: subtraction audit (same day)
|
||||
|
||||
Follow-up requested by the maintainer: how much of the codebase is duplication, boilerplate,
|
||||
or unnecessary wrapping — with the explicit instruction that **deliberate architecture layers
|
||||
are not exempt**; "less LoC = less surface for bugs" outranks ceremony. Two auditors (a
|
||||
jscpd-quantified duplication pass and a wrapper/indirection pass with the contract layers on
|
||||
trial); the largest verbatim-copy claims were independently re-diffed before inclusion.
|
||||
|
||||
### The numbers
|
||||
|
||||
- **Production duplication is low: ~0.9%** by jscpd at min-tokens 50 (268 duplicated lines
|
||||
over ~29.5k production TS/TSX; roughly double counting renamed semantic twins). Test-file
|
||||
duplication is ~5.4% but is the documented harness convention — benign. CSS Modules are
|
||||
the highest-duplication format at 5.4% and deserve one deliberate pass.
|
||||
- **Pure ceremony is ~1.5% of non-test LoC.** The plumbing layers (infrastructure +
|
||||
orchestration + modals + hooks, ~2.8k lines, ~9% of src) are overwhelmingly load-bearing:
|
||||
`db.ts`'s promise wrapping/self-healing/quota normalization, `url-hash.ts`'s total-parse
|
||||
grammar, `remote-data.ts`'s size-cap and error classification, the modal coordinator's
|
||||
snapshot/confirm/URL lifecycle all earn their lines on inspection. No generic machinery
|
||||
with a single instantiation was found — `editor-cursor-lens` has three real consumers,
|
||||
`wireEntityWriteThrough` four, and every modal-registry field varies across entries.
|
||||
- **Confidently deletable today, behavior identical: ~450–500 LoC**, plus ~100–150 more
|
||||
behind design decisions.
|
||||
|
||||
The diagnosis, verbatim from the audit because it generalizes: **the abstractions are
|
||||
right; the residue is photocopied instantiation files and hand-unrolled adapter pairs that
|
||||
the abstractions should have absorbed.** Nothing calls for architectural change — only
|
||||
tidying inside the architecture. Notably, one suspicion from the main review was stale: the
|
||||
orchestration wirers were already consolidated behind `wireEntityWriteThrough`; the leftover
|
||||
per-entity files are the residue that consolidation should have deleted.
|
||||
|
||||
### The cut list (verified; ranked by value)
|
||||
|
||||
Do right away — all mechanical, most protected by existing tests:
|
||||
|
||||
1. **Per-preference persistence chain** (~90–110 LoC; the compounding win). Five
|
||||
near-identical load/save pairs in `settings-store.ts:116-184`, four identical 10-line
|
||||
init/wire pairs in `orchestration/preferences.ts`, same shape again in
|
||||
`orchestration/settings.ts`/`snippet-sort.ts`, ten sequential calls in `main.tsx`.
|
||||
Collapse: a `uiSlicePref(key, validate, fallback)` adapter factory + a
|
||||
`wireStorePref(store, selector, save)` orchestration helper. Every future preference then
|
||||
costs ~8 lines instead of ~30 across four files. `theme.ts` (FOUC ordering) and
|
||||
`panes.ts` (debounce) stay bespoke — they carry real variation.
|
||||
2. **Entity-adapter photocopies** (~55–60 LoC). `snippet-store.ts` / `dataset-store.ts` /
|
||||
`theme-store.ts` / `font-store.ts` are the identical 28-line load/save/delete triple
|
||||
differing only in store name, migrate fn, and version constant (re-diffed: confirmed).
|
||||
Collapse: `makeEntityAdapter<T>(storeName, version, migrate)` in `db.ts` + four 3-line
|
||||
instantiations. Migration files stay — real per-entity logic.
|
||||
3. **Wirer residue files** (~55 LoC). `dataset-persistence.ts` / `theme-persistence.ts` /
|
||||
`font-persistence.ts` are 25-line modules whose body is one 6-line
|
||||
`wireEntityWriteThrough` call. Move the calls into `entity-persistence.ts` or
|
||||
`startup.ts` (their only caller). `snippet-persistence.ts` stays — the debounced
|
||||
autosave is real logic.
|
||||
4. **`editor.addAction` ceremony** (~55 LoC). Thirteen copies of the same 6-line
|
||||
registration object across `spec-transform-actions.ts:524-585` and
|
||||
`spec-config-actions.ts:210-235` → one table + `.map(addAction)`.
|
||||
5. **Scaffold-twin merge** (~40–50 LoC; overlaps main-review S2 and shares its fix). The
|
||||
completion prologue, suggestion mapping, and create-property block are verbatim between
|
||||
`spec-transform-scaffold.ts` and `spec-param-scaffold.ts`. Collapse into
|
||||
`editor-snippet.ts` helpers; the pure indent math moves to the TODO'd core module, where
|
||||
it also becomes testable.
|
||||
6. **Core jsonc-helper copies** (~25–30 LoC). `objectKeys` byte-identical between
|
||||
`spec-params.ts:85-93` and `spec-data-transforms.ts:103-111` (re-diffed: confirmed);
|
||||
`paramsArrayNode`/`transformArrayNode` identical modulo key string; two more shared
|
||||
shapes. Extract/share — both sides have strong tests.
|
||||
7. **localStorage adapter shell** (~28 LoC). `readRaw`/`writeRaw`/`available` identical
|
||||
between `settings-store.ts` and `ux-prefs.ts` modulo type name and log tag (re-diffed:
|
||||
confirmed) → `jsonLocalRecord<T>(key, tag)` in the same layer.
|
||||
8. **Batch of micro-cuts** (~90 LoC): `UrlStateSync`'s argument-ignoring
|
||||
`syncModalToUrl`/`clearModalFromUrl` wrappers (delete when fixing the
|
||||
`ModalCoordinator.ts:36` ordering TODO they paper over); `DatasetStore`'s
|
||||
thrice-repeated save epilogue; `readTextFile`/`readBinaryFile` 1:1 renames of `File`
|
||||
methods; `initPersistentStorage` pass-through; `modals/types.ts` folded into the
|
||||
registry; `SpecEditor`'s seven subscribe/dispose pairs → array; the landing's four-times
|
||||
repeated `shot` block + TODO'd `UiTheme` redeclaration; `Icon.tsx`'s four-times copied
|
||||
panel frame; one dead CSS rule (`ChartBuilderModal.module.css:213`).
|
||||
|
||||
Needs design thought (~100–150 LoC more): the splitter triple (`ResizeHandle` /
|
||||
`PaneSplitHandle` / `InspectorSplitHandle` share a near-verbatim `role="separator"` block —
|
||||
worthwhile but untested a11y surface, do with manual keyboard verification); the theme-panel
|
||||
Color+Size+Weight row groups (60–100 LoC vs greppability of accessible names); the modal
|
||||
master-detail CSS block; the landing/learn theme-toggle hook (blocked on where a shared
|
||||
React hook may live under the entry-isolation rule — needs a maintainer call).
|
||||
|
||||
Adjudicated and acquitted (challenged per the widened mandate, earn their lines): the modal
|
||||
registry/coordinator/shell trio (every registry field varies; flattest honest version saves
|
||||
only ~35 lines); the Zustand store shapes (a generic collection-slice helper would cost more
|
||||
in typing than the ~60 lines it saves and obscure real per-store invariants); core purity
|
||||
shims (minimal — the altitude problem runs the other way, app services holding pure logic);
|
||||
the non-photocopy infrastructure adapters (real normalization, migration, error mapping).
|
||||
|
||||
### Wrap-up follow-ups (recorded from the params-feature review passes, same day)
|
||||
|
||||
The params WIP was closed through the full wrap-up protocol (alignment + eng-council, both
|
||||
clean-context). Fixed in that pass: an `isUnitNode` bug (nested layer containers offered
|
||||
schema-invalid selection params), the S2 extraction (`core/spec-snippet.ts`, insertion math
|
||||
now table-tested applied-and-reparsed), spec §03 scaffolding coverage, the transforms
|
||||
catalog upgraded to real-schema validation, `ajv` declared. Deferred with recorded homes:
|
||||
|
||||
- `src/core/lesson-parse.ts` exports five unused types (knip) — verify and unexport. Same
|
||||
check for `LensFactory` in `editor-cursor-lens.ts:24` (exported, consumed only in-module).
|
||||
- `spec-transform-actions.ts` has an internal ~8-line clone (~:269-276 vs ~:346-353) —
|
||||
fold into the existing 663-LOC consolidation item.
|
||||
- The Ajv compile boilerplate now lives in two test files; a third schema-validating
|
||||
catalog test should extract a shared helper — and at that point promote the rule
|
||||
"seeded editor catalogs validate against the bundled VL schema, not just JSON.parse"
|
||||
to an `/alignment` check.
|
||||
- A small knip config (ignore `@fontsource/*`, `marked` false positives) would make
|
||||
future dead-export sweeps one command.
|
||||
Reference in New Issue
Block a user