refactor(#276): Phase 5 — composition cleanup, session teardown, architecture docs (closes #276)#292
Merged
Merged
Conversation
The Phase-2/4A temporary adapters are gone: consumers read app.conn.*/app.catalog.* directly. BootstrapApp/LoginApp/ShortcutsApp/ ResultsApp gained narrow conn Picks; loadConfig's one real caller (main.ts OAuth callback) renamed to conn.resolveConfig; the CM6 adapter's reference seam re-pointed to app.catalog (keeping its deliberately loose defensive shape); the e2e completions-write seam became window.__app.catalog.completions (still writable — the two-way accessors moved with the service). showLogin/signOut stay on App (they compose rendering — not pure forwards). ~150 test call sites re-pointed (incl. the 77 app.chCtx reads → app.conn.chCtx); fake-app keeps a convenience top-level chCtx override key feeding conn.chCtx so makeApp call sites didn't churn; one real coverage gap closed (catalog.docCache accessor now asserted directly). Gate: 113 files / 3214 tests, build, check:arch green. Part of #276 (Phase-5 PR, commit 1). Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…(2/5) Eleven more pure forwards gone; consumers carry narrow session Picks (filter-bar/results/dashboard/file-menu/panels/saved-history/splitters). app.saveVarRecent survives as the ONE deliberate delegate (the session's recents policy reads the live app property so the documented mock-substitution test seam keeps working — noted in both app.ts and app.types.ts). fake-app shrank to derive from the session stubs and gained a fresh per-makeApp stub layer for params/prefs, fixing a latent cross-test mock-bleed (module-level singleton vi.fn()s accumulated call counts across tests). App is down to ~64 top-level members. Gate: 113 files / 3214 tests, build, check:arch green. Part of #276 (Phase-5 PR, commit 2). Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…dary rules — phase 5 (3/5) ui/workbench/workbench-shell.ts (mountWorkbenchShell) now owns renderApp's entire former body — header, sidebar, splitters, workbench DOM, every reactive effect (subscription sets byte-identical, ordering comments preserved), attachShell, and the catalog bootstrap tail — behind a narrow WorkbenchShellDeps bag; app.ts's renderApp is a thin composition call. The shell forwards the app object to the existing render modules (documented crutch — narrowing those is follow-up, not this issue), and deliberately reads app.sqlEditor/app.specEditor LIVE (a test hot-swaps the port after mount — proven by a real failure). src/application/ch-session-params.ts is the sessionParams family's final home (pure, one implementation shared by the workbench hook and ExportService wiring — resolving the Phase-4 deferred decision). dashboard.ts now takes a narrow DashboardApp interface instead of App (the panel-registry seam keeps the one documented cast, results.ts's convention). ui/theme-toggle.ts extracts the shared flip+persist+DOM composition; App.toggleTheme survives (dashboard.test.ts pins the flat seam — judged, documented). check-boundaries gains file-target support and the final rules: neither shell (nor dashboard.ts) may import src/ui/app.ts — verified live with temp violations. app.ts: 1,687 → 1,434 lines. Gate: 115 files / 3231 tests, build, check:arch (4 rules) green. New specs: ch-session-params (12), theme-toggle (5); workbench-shell.ts 100/96.4/100/100. Part of #276 (Phase-5 PR, commit 3). Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
signOut is the one real end-of-session event in a single-route tab, and it now actually tears the session down: workbench.destroy() (aborts + KILLs an in-flight run, disposes the shell effects — re-attached by the next renderApp, so the session stays reusable), graph.cancel(), exportService.cancelExport()/cancelExportScript(), and catalog.invalidate() (a later sign-in — possibly a different server — never sees stale schema/reference/doc caches), all BEFORE credentials clear and the login screen renders. Previously a mid-flight query or export could land (and repaint) after the login screen was showing, and the catalog kept the old server's caches across account switches. First production callers for destroy()/invalidate() — the honest wiring point (there is no route-remount to hook; the dashboard is its own browser tab whose teardown the JS never observes). New integration test drives a deferred in-flight run through signOut (abort + server KILL + cache drop + login painted) and back through re-auth + re-render to a successful streamed run. Gate: 115 files / 3232 tests, build green. Part of #276 (Phase-5 PR, commit 4). Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…5 (5/5)
The old document described the pre-refactor god object as the intended
design ('the controller (ui/app.js)'). It now documents reality: the
layer diagram with src/application + the two route session/shell pairs,
the mechanically-enforced dependency rules (check-boundaries), the two
known pre-existing exceptions, per-service ownership incl. the single
live chCtx object and caller-owned cancellation, the signOut teardown
wiring (and why there is no deeper route-teardown to hook), the App
object's new composition-surface role with its one documented surviving
delegate, and the unchanged injected-seam/build story. CHANGELOG
updated for Phase 5.
Part of #276 (Phase-5 PR, commit 5).
Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
tests/e2e/editor.html hand-builds the __app literal the CM6 adapter mounts against; commit 1 re-pointed the adapter's reference reads to app.catalog.* and updated the spec's write site but not this fixture's construction — chromium+webkit editor specs (8) failed on missing highlighting/completion. The fixture now carries the catalog member shape. Full chromium+webkit e2e: 80/80. (Caught by the end-of-PR e2e run — the per-commit unit gates can't see raw-harness fixtures.) Part of #276 (Phase-5 PR). Claude-Session: ad39bf19-1690-43c7-abb4-f0084fca00a2 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Closes #276 — Phase 5, the final PR of the refactor (after #278, #279, #281, #282, #289, #290). Six commits:
app.conn.*/app.catalog.*through narrowPicks (BootstrapAppnests aconnslice;loadConfig's one real caller renamed toconn.resolveConfig); ~150 test sites re-pointed; the CM6 adapter's reference seam re-pointed with its defensive shape preserved.app.saveVarRecentis the one documented survivor (a mutable test seam the parameter session reads live).fake-appshrank to derive from session stubs — which exposed and fixed a latent cross-test mock-bleed (module-level singletonvi.fn()s accumulated call counts across tests).ui/workbench/workbench-shell.ts—renderApp's entire body (DOM + every effect, subscription sets byte-identical) behind a narrowWorkbenchShellDeps;renderAppis now a thin composition call.dashboard.tsnarrowed toDashboardApp.application/ch-session-params.tsis thesession_idhelpers' final home (one implementation shared by workbench + export wiring — resolving the deferred Phase-4 decision). Boundary rules extended: neither shell may importsrc/ui/app.ts(verified live with temp violations).signOut— a real behavior fix: an in-flight run is now aborted and server-KILLed, exports and lineage fetches cancel, and the schema/reference caches invalidate before the login screen renders (previously stale work could land after sign-out, and caches survived account switches). First production callers fordestroy()/invalidate()— the honest wiring point; there is no route-remount to hook. New integration test drives the full cycle including re-auth + a successful streamed run after re-render (the workbench session is reusable post-destroy).docs/ARCHITECTURE.mdrewritten — it still documented the pre-refactor god object as the intended design; it now describes the modular monolith (layer diagram withsrc/application+ route session/shell pairs, mechanically-enforced boundaries with the two documented pre-existing exceptions, per-service ownership, lifecycle wiring, theAppobject's composition-surface role).editor.htmlbuilds__appby hand and needed the newcatalogseam shape; caught by the end-of-PR chromium+webkit run (8 real failures → 80/80 after).Final review pass: zero findings — signOut ordering verified race-free (the fire-and-forget KILL captures
origin/token synchronously at call time, beforeconn.signOut()mutates them), the shell extraction diffed byte-faithful against main, delegate deletion verified complete repo-wide, all narrowed interfaces structurally verified, all ARCHITECTURE.md claims checked against code.Verification: full gate green (115 files / 3,232 tests + tsc +
check:arch, 14 files / 4 rules), build green, e2e chromium+webkit 80/80.#276 acceptance criteria — final audit
Every box in the issue checks: services constructible without
App/AppState/DOM ✅ · shared execution service across workbench/dashboard/detached reads ✅ · auth lifecycle inConnectionSessionwithout rendering ✅ · separate route sessions with owned cancellation + provendestroy()✅ ·ExportServicewith injectable sink ✅ · catalog/parameter/document/saved-query/schema-graph extractions ✅ ·src/application/**free ofuiimports (enforced) ✅ · route sessions mutually non-importing (enforced) ✅ · behavior/persisted formats/build compatibility preserved throughout ✅ · temporary adapters deleted (one documented survivor with its rationale in code) ✅ ·app.tsis composition/bootstrap (1,453 lines, from 2,964; the remaining bulk is the actions registry, popover/menu DOM, and render-module glue — no business workflows) ✅ ·docs/ARCHITECTURE.mdupdated ✅.Definition-of-done metrics:
app.tsis no longer the largest implementation file for workflow reasons; a unit test executes a query, runs a dashboard tile, validates parameters, and exports to an in-memory sink without DOM (see the service specs); a future MCP command can depend onapp.exec/app.conn/app.savedtyped services without importingapp.ts.Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updated if behavior or the deployed surface changed🤖 Generated with Claude Code