Skip to content

refactor(#276): Phase 4 — parameter, export, document, saved-query, schema-graph, preferences services#290

Merged
BorisTyshkevich merged 6 commits into
mainfrom
refactor/phase4-services-276
Jul 17, 2026
Merged

refactor(#276): Phase 4 — parameter, export, document, saved-query, schema-graph, preferences services#290
BorisTyshkevich merged 6 commits into
mainfrom
refactor/phase4-services-276

Conversation

@BorisTyshkevich

Copy link
Copy Markdown
Collaborator

What & why

Part of #276 — the rest of Phase 4 in one PR (user decision at the 4A gate), structured as four extraction commits + a review commit so each piece is independently readable and revertable. Merged so far: #278 (0–1), #279 (2), #281 (3a), #282 (3b), #289 (4A). Design rulings for every extraction are pinned in the issue's Phase-4 comment and were plan-stressed before implementation.

Commit 1 — WorkbenchParameterSession (app.params, 31-test spec): the {name:Type} prepare/gate/exec-view/hardening/recents policy. Thinner cut per plan review: the var strip stays a DOM view calling session methods (no speculative view-model API). Live-state accessors (never snapshots — the filter bar/dashboard/detached view write varValues directly); app.hardenedVars aliases the session's own Set. Both consumers re-pointed at once: the workbench session's param hooks (zero edits to that file) and export's direct calls.

Commit 2 — ExportService + ExportSink (app.exports, 40-test spec): pure move of the re-pointed bodies. Transport deps are ch-client + the live ctx accessor, not app.exec (raw-Response hold-back inspection is incompatible with parsed results). Consolidates the last two duplicated cancellation-state copies from the Phase-0 map. exporting stays an AppState signal with the service as sole writer. The picker-before-auth ordering (transient user activation) is now pinned by an explicit test. The two ~350-line app.test.ts export describes migrated to the in-memory-sink spec; 8 thin integration tests remain.

Commit 3 — QueryDocumentSession + SavedQueryService (app.queryDoc/app.saved, 20+22-test specs): Spec evaluation/document lifecycle (diagnostics typed with the core SpecValidationDiagnostic — no editor imports, check:arch-enforced) and create/commit/history/share persistence returning discriminated-union results. The shell keeps the exact toast messages and the post-commit repaint cascade (a hand-repeated convention at ≥5 call sites — deliberately not service-owned). Prerequisite type-narrowing of six state.ts signatures to exact Picks (type-only). No service-to-service dependency: the shell sequences evaluate → commit.

Commit 4 — SchemaGraphSession + AppPreferences (app.graph/app.prefs, 25+13-test specs; built in a parallel worktree, hand-merged, merge verified against both parents by a dedicated review finder): the lineage lifecycle moved on the WorkbenchSession template (session mutates tab.result in place + injected repaint hook; explicitly-captured tab reference; reference-identity stale guard verbatim; expand() returns data — the shell keeps the open-window-before-await popup-blocker invariant); AppState.schemaGraphAbortController deleted (session-private, zero external refs). AppPreferences is the deliberately narrow typed persistence surface.

Commit 5 — review fixes (three-finder full-diff pass): restored a lost end-to-end pin (exportEntry unavailable in Spec mode — the unit gate can't catch that wiring), trimmed AppPreferences' eight zero-caller setters (speculative surface), added the missing fake-app Partial-override plumbing for graph/prefs/exports, removed five dead app.ts imports.

Invariants pinned throughout: sessionParams/needsSession/sessionParamsFor stay app.ts-local (their home is a Phase-5 decision — resolved a genuine 4B/4C ownership collision); recordHistory's conditional History repaint stays shell; activateInvalidSpecDraft stays shell (>80% DOM). Known pre-existing wart preserved verbatim: exportDirect names files with ambient Date.now() rather than the injected wallNow().

Verification: full gate green (113 files / 3,214 tests + tsc + check:arch), build green, e2e chromium+webkit 80/80 (the accepted signal on this shared box per #289). app.test.ts changed only in the export describes; every other suite passes unmodified. app.ts: 2,964 → 1,761 lines.

Acceptance criteria completed (cumulative): export in ExportService with injectable sink ✅; parameter policy outside the var-strip renderer ✅; QueryDocumentSession represents open-document behavior ✅; saved-query/history/share behind a focused service ✅; schema-graph lifecycle outside app.ts ✅; a unit test can execute a query, run a dashboard tile, validate parameters, and export to an in-memory sink without DOM ✅.

Remaining for #276: Phase 5 (wire session destroy()s + catalog.invalidate() into real lifecycle, delete the App delegates, split renderApp into route shells, rewrite docs/ARCHITECTURE.md).

Checklist

  • npm test passes (the per-file coverage gate is non-negotiable)
  • Tests added/updated in the same change as the code
  • npm run build succeeds (single-file dist/sql.html)
  • Layers kept honest: pure logic in src/core/, network in src/net/ (injected fetch), DOM in src/ui/
  • No new runtime dependency (or it's a deliberate, justified addition — see CONTRIBUTING)
  • README / CHANGELOG.md ([Unreleased]) updated if behavior or the deployed surface changed
  • Reconciled affected tracked work (roadmap Roadmap to 1.0.0 #68, the issue body, ADR/CHANGELOG) if this change reshaped it

🤖 Generated with Claude Code

BorisTyshkevich and others added 6 commits July 17, 2026 13:57
src/application/workbench-parameter-session.ts (app.params) owns the
{name:Type} parameter policy: analysis/prepare/gate/exec-view (#173/
#165), input-vs-execute hardening (#170), schema-inferred enum options
(#172 v2), recent-value recording/clearing (#171), and the varValues/
filterActive/varRecent persistence wrappers. Thinner cut per plan
review: renderVarStrip stays in app.ts as the DOM view calling session
methods; the full view-model API is deferred. Live-state accessors
(never snapshots — filter-bar/dashboard/results write varValues
directly); app.hardenedVars aliases the session's own Set (tests call
.has() on it). Both consumers re-pointed in one change: the workbench
session's param hooks and the export block's direct calls
(workbench-session.ts itself: zero edits). sessionParams/needsSession/
sessionParamsFor stay app.ts-local through Phase 4 (plan ruling — their
final home is a Phase-5 decision). saveVarRecent has two documented
entry points (App delegate + internal hook reading the live app
property) to preserve app.test.ts's property-mock semantics unchanged.

app.test.ts unmodified. Gate: 108 files / 3135 tests, build,
check:arch green. New spec: 31 tests, 100/100/100/100.

Part of #276 (Phase-4 combined PR, commit 1 of 4).

Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
src/application/export-service.ts (app.exports) owns direct + script
export: picker-first ordering (transient user activation — now pinned
by an explicit pickFile-before-ensureConfig test), hold-back-buffer
streaming with mid-stream ClickHouse exception excision, .partial
rename semantics, per-statement script files + directory naming,
stop-on-first-failure, and BOTH formerly-module-scope cancellation
state groups (now service-private — the last of the three duplicated
cancellation copies from the Phase-0 map). Transport deps are ch-client
fns + the live ctx accessor, NOT app.exec (raw-Response hold-back is
incompatible with parsed results); ExportSink { pickFile, pickDirectory }
is the injectable filesystem seam (issue §5); state.exporting stays an
AppState signal with the service as sole writer (running's precedent);
F6 waveMs handling preserved. showExportProgress (DOM banner),
downloadFile (Blob+anchor), canExport checks stay app.ts.

Test migration: the two ~700-line app.test.ts export describes (40
scenarios) moved to tests/unit/export-service.test.ts against an
in-memory sink; app.test.ts keeps 8 thin integration checks (wiring,
signal→button effect, one round trip per kind). Coverage: app.ts
unchanged, export-service.ts 100/93.6/100/100.

Known pre-existing wart preserved verbatim (out of scope): exportDirect
names files with ambient Date.now() rather than the injected wallNow().

Gate: 109 files / 3142 tests, build, check:arch green.

Part of #276 (Phase-4 combined PR, commit 2 of 4).

Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ase 4C

src/application/query-document-session.ts (app.queryDoc) owns the Spec
evaluation/document lifecycle over QueryTab objects: apply/evaluate
draft, revalidate-all, reveal-first-error, validator registration, and
the editor-mode policy gate — diagnostics typed with core/spec-draft's
SpecValidationDiagnostic (never an editor import; check:arch enforces).
src/application/saved-query-service.ts (app.saved) owns create/commit
(validation-before-persist), history recording, and pure share-URL
building — all returning discriminated-union results; the shell keeps
the exact toast messages and the post-commit DOM cascade (which is a
hand-repeated convention across ≥5 call sites, deliberately NOT
service-owned per plan review). No service-to-service dependency: the
shell sequences evaluate → commit. Prerequisite type-narrowing in
state.ts (createSavedQuery/commitSavedQuery/patchSavedSpec/
tabsForSaved/invalidSpecTabForSaved/recordHistory → exact Picks,
type-only). sessionParams/needsSession/sessionParamsFor untouched
(Phase-5 decision). activateInvalidSpecDraft stays shell (>80% DOM).

app.test.ts: zero edits (all 276 tests pass unmodified). Gate: 111
files / 3184 tests, build, check:arch green. New specs: 20 + 22 tests,
both files 100/100/100/100.

Part of #276 (Phase-4 combined PR, commit 3 of 4).

Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
src/application/schema-graph-session.ts (app.graph) owns the schema
lineage operation lifecycle: the two-phase progressive draw with the
reference-identity stale guard moved verbatim (session mutates
tab.result in place + injected renderResults hook — the WorkbenchSession
template; explicitly-captured tab reference, never lazy active-tab),
expand() as a plain async data return (the shell keeps opening the
window synchronously BEFORE the await — popup-blocker invariant — and
maps SchemaGraphAuthRequiredError to the view message), last-clicked-
wins node-detail guard behind an opaque token, and the abort state now
session-private (AppState.schemaGraphAbortController deleted — zero
external refs, same shape as Phase 3a's abortController deletion).
The workbench session's cancelSchemaGraph hook re-points.

src/application/app-preferences.ts (app.prefs) is the deliberately
NARROW typed preference surface (plan ruling): per-key setters for the
true preference keys only (theme/layout px/splits/sidePanel/rowLimit/
dashLayout/dashCols); the generic saveJSON/saveStr passthroughs the
state.ts domain helpers depend on stay untouched; toggleTheme's
state+persist half moved, its DOM half stays composed in app.ts.

Implemented in an isolated worktree in parallel with 4C; merged with
three expected conflicts (app.ts import blocks, fake-app stubs)
resolved by union. app.test.ts: zero edits across both parts.

Gate on the merged tree: 113 files / 3222 tests, build, check:arch
green. New specs: 25 + 13 tests; schema-graph-session.ts
100/97.3/100/100, app-preferences.ts 100/100/100/100.

Part of #276 (Phase-4 combined PR, commit 4 of 4).

Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Three-finder pass over the combined four-commit diff:
- Restored the end-to-end 'exportEntry unavailable in Spec mode' pin
  lost in the export-test migration (the unit-level gate can't catch a
  wiring regression between queryDoc's editor-mode policy and the
  export gate).
- AppPreferences trimmed to its real API ({ save, toggleTheme }): the
  eight per-key setters had zero production callers — speculative
  surface (CLAUDE.md rule 5), documented in the interface.
- fake-app: graph/prefs/exports gained the same Partial-override merge
  plumbing as their sibling sessions (one-method stubs now typecheck).
- Five dead imports removed from app.ts (KEYS, recordScriptHistory,
  mergedSourceSql, queryName, ResultSchemaGraph).

The hand-merged 4D conflict resolutions verified clean against both
parents by the merge-fidelity finder (savePref keys all valid
PreferenceKeys, toggleTheme sequence byte-identical, sessionParams
family still app.ts-local, no TDZ hazards, no stranded markers).

Gate: 113 files / 3214 tests, build green.

Part of #276 (Phase-4 combined PR).

Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@BorisTyshkevich
BorisTyshkevich merged commit 4d273a4 into main Jul 17, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant