refactor(#276): Phase 5 — composition cleanup, session teardown, architecture docs (closes #276)

CHANGELOG.md

@@ -10,6 +10,23 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
 ## [Unreleased]
 
 ### Changed
+- **The app.ts → services refactor is complete** (#276, Phase 5). The
+  temporary `App` delegates from Phases 2–4 are deleted — consumers read
+  `app.conn`/`app.catalog`/`app.params`/`app.queryDoc`/`app.prefs` directly
+  through narrow interfaces (`app.saveVarRecent` survives as the one
+  documented exception); the workbench DOM + effects moved into
+  `ui/workbench/workbench-shell.ts` behind a narrow deps bag, making
+  `renderApp` a thin composition call; the dashboard shell is typed against
+  a narrow `DashboardApp`; the per-tab ClickHouse `session_id` helpers got
+  their final home (`application/ch-session-params.ts`, one shared
+  implementation); the boundary guard now also forbids either shell from
+  importing `src/ui/app.ts`. **Behavior fix**: signing out now tears the
+  session down — an in-flight run is aborted and server-`KILL`ed, 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).
+  `docs/ARCHITECTURE.md` rewritten to describe the actual modular-monolith
+  shape (it still documented the pre-refactor god object as the design).
 - **Phase 4 of the app.ts → services refactor complete** (#276): five more
   focused modules under `src/application/`, all constructible without
   `App`/`AppState`/DOM, behavior byte-identical. `WorkbenchParameterSession`

build/check-boundaries.mjs

@@ -5,6 +5,10 @@
 //   Phase 3 — the route sessions must not import each other's implementation
 //   modules (ui/workbench ↔ ui/dashboard), and the dashboard route must not
 //   depend on the editor ports at all.
+//   Phase 5 — neither route shell (ui/workbench/**, ui/dashboard.ts +
+//   ui/dashboard/**) may import src/ui/app.ts: both receive everything they
+//   need injected (a narrow deps bag / the App type only) — reaching back
+//   into app.ts itself would recreate the coupling this phase removes.
 // `import type` counts too: a type-only import still couples the layers at
 // compile time. Extend RULES below in later phases rather than growing a
 // second script.
@@ -21,17 +25,37 @@ const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'
 const SOURCE_EXT = /\.(ts|tsx|js|mjs)$/;
 
 /** Each rule: every source file under `dir` must not import anything that
- *  resolves under any of `forbidden` (directories, repo-relative). */
+ *  resolves under any of `forbidden` (directories OR single files,
+ *  repo-relative — `dir` itself may also name a single file, e.g.
+ *  `src/ui/dashboard.ts`, since that route shell has no dedicated directory
+ *  of its own the way `src/ui/workbench/**`/`src/ui/dashboard/**` do). */
 const RULES = [
   { dir: 'src/application', forbidden: ['src/ui', 'src/editor'], why: 'issue #276 day-1 rule' },
-  { dir: 'src/ui/workbench', forbidden: ['src/ui/dashboard'], why: 'issue #276 Phase 3: route sessions must not import each other' },
-  { dir: 'src/ui/dashboard', forbidden: ['src/ui/workbench', 'src/editor'], why: 'issue #276 Phase 3: route sessions must not import each other; dashboard has no editor' },
+  {
+    dir: 'src/ui/workbench',
+    forbidden: ['src/ui/dashboard', 'src/ui/app.ts'],
+    why: 'issue #276 Phase 3/5: route sessions must not import each other, and the shell must not reach back into app.ts (everything it needs is injected)',
+  },
+  {
+    dir: 'src/ui/dashboard',
+    forbidden: ['src/ui/workbench', 'src/editor', 'src/ui/app.ts'],
+    why: 'issue #276 Phase 3/5: route sessions must not import each other, dashboard has no editor, and the shell must not reach back into app.ts',
+  },
+  {
+    // The dashboard route's own shell file (no dedicated directory, unlike
+    // its `dashboard-session.ts` runtime under `src/ui/dashboard/`) — same
+    // Phase 5 rule as the workbench shell above.
+    dir: 'src/ui/dashboard.ts',
+    forbidden: ['src/ui/app.ts'],
+    why: 'issue #276 Phase 5: the dashboard shell must not reach back into app.ts (everything it needs is injected)',
+  },
 ];
 
-function collectFiles(dir) {
+function collectFiles(target) {
+  if (fs.statSync(target).isFile()) return SOURCE_EXT.test(target) ? [target] : [];
   const out = [];
-  for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
-    const full = path.join(dir, entry.name);
+  for (const entry of fs.readdirSync(target, { withFileTypes: true })) {
+    const full = path.join(target, entry.name);
     if (entry.isDirectory()) out.push(...collectFiles(full));
     else if (SOURCE_EXT.test(entry.name)) out.push(full);
   }

docs/ARCHITECTURE.md

@@ -1,56 +1,123 @@
 # Architecture
 
+A modular ES-module SPA that builds to one self-contained HTML file served from
+ClickHouse. No framework; state reactivity is `@preact/signals-core`
+(ADR-0001), strict TypeScript throughout (ADR-0002). This document reflects
+the post-#276 shape: a **modular monolith** — explicit application services and
+route-scoped sessions behind a small composition root.
+
 ## Layers
 
 ```
-core/   pure logic         — strings/numbers/JWT/PKCE/SQL-tokenize/stream-parse
-net/    integration        — OAuth (config+flow) and the ClickHouse® HTTP client
-ui/     presentation       — hyperscript (h), icons, and render modules
-state   model              — the state object and pure operations over it
-main    bootstrap          — OAuth callback handling + initial render
+core/          pure logic (no DOM, no globals, no imports from other layers)
+net/           integration: OAuth + the ClickHouse HTTP client (fetch injected via ctx)
+application/   route-agnostic services & sessions (no App, no DOM, no ui/editor imports)
+ui/workbench/  the workbench route: session (run lifecycle) + shell (DOM + effects)
+ui/dashboard/  the dashboard route: session (tile/filter runtime); ui/dashboard.ts is its shell
+ui/*           render modules (hyperscript), editor ports live in editor/
+ui/app.ts      composition/bootstrap: constructs everything, wires routes
+state.ts       the shared signal-backed model + pure ops
+main.ts        page bootstrap: OAuth callback, share links, route dispatch
 ```
 
-The dependency direction is strictly downward: `ui` → `net`/`state`/`core`,
-`net` → `core`, `core` → nothing. There are no cycles, so esbuild bundles
-`main.js` into one IIFE.
+Dependency direction is strictly downward. Enforced mechanically by
+`build/check-boundaries.mjs` (runs in `pretest` as `check:arch`):
 
-## The injected-seam pattern
+- `src/application/**` never imports `src/ui/**` or `src/editor/**` (type-only
+  imports count).
+- `src/ui/workbench/**` and `src/ui/dashboard/**` never import each other,
+  never import the editor (dashboard), and never import `src/ui/app.ts` —
+  shells receive everything injected.
 
-Every side effect is passed in, never imported directly by logic:
+Two known, deliberate exceptions predate #276 and are out of its scope:
+`core/saved-io.ts` imports a type from `editor/spec-editor.types.js`, and
+`editor/codemirror-adapter.ts` imports `ui/dom.js` + `ui/dnd-mime.ts`.
 
-- `loadOAuthConfig(fetchFn, basePath)` and all of `net/` take `fetchFn`.
-- `ch-client` functions take a `ctx = { fetch, origin, getToken, refresh,
-  onSignedOut }`.
-- `createApp(env)` injects `document`, `window`, `location`, `fetch`, `crypto`,
-  `sessionStorage`.
-- `generatePKCE(cryptoObj)`, `storage` helpers `(…, store)`, `timeAgo(ts, now)`
-  all accept their dependency.
+## The services (`src/application/`)
 
-This is why the suite needs no network/DOM mocking libraries — plain stubs
-suffice, and coverage is genuine.
+Each is a `create*(deps)` factory taking a narrow dependency bag — never the
+`App` object or the full `AppState` (narrow `Pick`-shaped state slices are
+structurally satisfied by `AppState`). Side effects are always injected
+(fetch via the ClickHouse `ctx`, clocks, `uid`, storage, timers), so every
+service is tested with plain stubs at the per-file coverage gate.
+
+| Module | Owns |
+|---|---|
+| `query-execution-service` (`app.exec`) | the shared request/stream/normalize read core + the script transport loop (retry classification, stop-on-first-failure, per-attempt `query_id`); stateless `kill(queryId)` — cancellation is caller-owned (`AbortController`s live with the owning session) |
+| `connection-session` (`app.conn`) | auth + connection lifecycle: OAuth PKCE login/refresh, Basic probing, IdP config, identity, token storage, sign-out, the cross-tab dashboard auth handoff, and **the single live `chCtx` object** (mutated in place — `authConfirmed` by `net/ch-client`, `origin` by sign-in/handoff — never reconstructed) |
+| `schema-catalog-service` (`app.catalog`) | server version, schema tree, lazy columns, SQL reference/completions, entity-doc cache, `invalidate()` |
+| `workbench-parameter-session` (`app.params`) | `{name:Type}` analysis/prepare/gate policy, input-vs-execute hardening, enum inference, recent values; reads the live shared `AppState` slices through accessors |
+| `export-service` (`app.exports`) | direct + script export behind an injectable `ExportSink` (`pickFile`/`pickDirectory`); hold-back exception inspection, `.partial` semantics, its own cancellation state |
+| `query-document-session` (`app.queryDoc`) | Spec evaluation/diagnostics/dirty flags over `QueryTab`s, editor-mode policy |
+| `saved-query-service` (`app.saved`) | create/commit saved queries (validate-before-persist), history recording, share-URL building — typed results; the shell renders messages |
+| `schema-graph-session` (`app.graph`) | lineage load/expand/node-detail lifecycle with stale-request guards; abort state is session-private |
+| `app-preferences` (`app.prefs`) | typed preference persistence (`save(name, value)` + `toggleTheme()`) |
+| `ch-session-params` | pure helpers minting/attaching the per-tab ClickHouse HTTP `session_id` (TEMPORARY/SET stickiness), shared by the workbench hooks and export wiring |
+
+## Route sessions and shells
 
-## The controller (`ui/app.js`)
+- `ui/workbench/workbench-session.ts` (`app.workbench`) owns the running
+  operation: `run`/`runScript`/`runEntry`/`cancel`, the private run
+  bookkeeping (`runT0`/`runQueryId`/ticker) and the in-flight
+  `AbortController`. It is the **sole production writer of the `running`
+  signal**. DOM stays out via injected hooks; the three run-coupled reactive
+  effects register through `attachShell(...)` with captured disposers
+  (idempotent on re-render).
+- `ui/workbench/workbench-shell.ts` (`mountWorkbenchShell(deps)`) builds the
+  workbench DOM (header, sidebar, splitters, tabs, toolbar, var strip,
+  results) and registers every other effect. `ui/app.ts`'s `renderApp` is a
+  thin call into it.
+- `ui/dashboard/dashboard-session.ts` owns the dashboard runtime: the 6-way
+  tile pool, wave generations (reserved at wave creation), per-slot
+  cancellation, filter waves and merging, `destroy()`. Its input is an
+  explicit `DashboardRuntimeInput` built by the shell from the favorites
+  list — a stored dashboard document can replace that source without touching
+  the session. `ui/dashboard.ts` is its shell (own header; no sidebar), typed
+  against a narrow `DashboardApp`, not `App`.
 
-`createApp(env)` returns the `app` object that every render module receives:
+Lifecycle ownership: **cancellation state always lives with the session that
+owns the operation** (issue #276 rule 5) — never in the transport service.
+`destroy()`/`invalidate()` are wired where a session really ends today:
+`signOut` tears down the workbench session, cancels graph/export work, and
+invalidates the catalog before the login screen renders. There is no
+route-remount mechanism — the dashboard is its own browser tab whose closure
+JS never observes — so no teardown theater beyond that.
 
-- `app.state` — the state model
-- `app.dom` — live references to mounted regions, repopulated by `renderApp`
-- `app.actions` — callbacks (run, newTab, share, toggleSaved, loadColumns, …)
-- `app.chCtx` — the ClickHouse fetch context
-- persistence: `app.savePref(name, value)` (raw prefs), `app.saveJSON` (lists)
+## The `App` object
 
-Render modules import nothing from `app.js` (avoiding a cycle); they take `app`
-as a parameter.
+`createApp(env)` still returns one `app` object, but it is now a composition
+surface, not a controller: the session/service members above, the shared
+`state`, the live `dom` region dictionary (reset wholesale per mount), the
+injected env seams (Chart, Dagre, editor ports, pickers, clocks), the
+`actions` registry (the DOM↔session event boundary render modules call
+through), and a handful of genuinely shell-owned composites (`showLogin`,
+`signOut`, `toggleTheme`, `openDashboard`, `renderApp`/`renderDashboard`).
+Render modules keep taking `app` as a parameter and import nothing from
+`app.ts`; most declare their own narrow `Pick`-shaped interfaces
+(`LoginApp`, `ShortcutsApp`, `ResultsApp`, `DashboardApp`, …). One deliberate
+delegate survives with a documented reason: `app.saveVarRecent` (a mutable
+test seam the parameter session reads live).
+
+## Injected-seam pattern
+
+Unchanged from the beginning and now applied uniformly: every side effect is
+passed in, never imported — `createApp(env)` injects
+`document/window/location/fetch/crypto/sessionStorage`, `ch-client` functions
+take a `ctx = {fetch, origin, getToken, refresh, authHeader, onSignedOut}`,
+and every `create*Service(deps)` receives its transport/clock/uid/storage
+explicitly. The suite needs no network/DOM mocking libraries — plain stubs
+suffice, and coverage is genuine.
 
 ## Query execution
 
-`runQuery` (in `net/ch-client.js`) streams ClickHouse's
-`JSONStringsEachRowWithProgress` format: each newline-delimited object is folded
-into the result via the pure `applyStreamLine`. TSV/JSON output formats are
-fetched as raw text. A single automatic token refresh fires on 401/403 or a
-`token_verification_exception` body.
+`runQuery` in `net/ch-client.ts` streams `JSONStringsEachRowWithProgress`,
+folded via the pure `applyStreamLine`; a single automatic token refresh on
+401/403/`token_verification_exception` (before `authConfirmed` flips, an auth
+failure signs out; after, it is a query error).
 
 ## Build
 
-`build/build.mjs` runs esbuild (bundle + minify, IIFE) and inlines the script +
-`styles.css` into `build/template.html`, producing `dist/sql.html`.
+`build/build.mjs` runs esbuild (bundle + minify, IIFE), inlines the script and
+`styles.css` into `build/template.html` → a single `dist/sql.html`. Four
+bundled runtime dependencies (CodeMirror 6, Chart.js, dagre,
+`@preact/signals-core`); the served page makes zero third-party requests.

src/application/app-preferences.ts

@@ -9,7 +9,8 @@
 // each preference. Every write site except `toggleTheme` already mutates its
 // own state field itself (splitters.ts sets `ctx.state.sidebarPx` before
 // calling `ctx.save(...)`; dashboard.ts sets `state.dashLayout`/`dashCols`
-// before `app.savePref(...)`; app.ts's `setResultRowLimit` sets
+// before calling `app.prefs.save(...)` directly — #276 Phase 5 deleted the
+// flat `App.savePref` delegate; app.ts's `setResultRowLimit` sets
 // `state.resultRowLimit` first) — so `save(name, value)` is a pure typed
 // persist call, no state slice needed. `toggleTheme` is the one exception
 // (issue ruling): the state flip AND the persist happen together here: the
@@ -43,12 +44,12 @@ export interface AppPreferencesDeps {
 
 export interface AppPreferences {
   /** Generic persist-only setter — the exact `(name, value)` shape app.ts's
-   *  public `savePref` has always exposed (dashboard.ts/saved-history.ts/
-   *  splitters.ts call it through that unchanged delegate). This IS the
-   *  service's write API: per-key typed setters were considered and dropped
-   *  (review) — every real call site already holds a validated
-   *  `{name, value}` pair, so a per-key surface would ship with zero
-   *  callers (CLAUDE.md rule 5: no speculative primitives). */
+   *  former `App.savePref` delegate used to expose (#276 Phase 5 deleted it;
+   *  dashboard.ts/saved-history.ts/splitters.ts's callers call `app.prefs.save`
+   *  directly now). This IS the service's write API: per-key typed setters
+   *  were considered and dropped (review) — every real call site already
+   *  holds a validated `{name, value}` pair, so a per-key surface would ship
+   *  with zero callers (CLAUDE.md rule 5: no speculative primitives). */
   save(name: PreferenceKey, value: unknown): void;
   /** Flips `state.theme` light↔dark AND persists it in one call (issue
    *  ruling — the one preference whose state mutation moves here, not just

src/application/ch-session-params.ts

@@ -0,0 +1,68 @@
+// The ClickHouse HTTP `session_id` policy (#276 Phase 5 final home) —
+// `sessionParams`/`needsSession`/`sessionParamsFor` moved out of app.ts
+// wholesale (byte-identical logic) into their own constructible-without-App
+// module so both app.ts's workbench-hook wiring and the ExportService dep
+// wiring share ONE implementation instead of two independently-maintained
+// copies of the same three functions.
+//
+// A ClickHouse HTTP session ties a tab's requests together so session state —
+// temporary tables, SET settings — survives across the separate HTTP requests
+// of a multiquery script (and across successive runs in the tab). ClickHouse's
+// HTTP interface runs one statement per request and is otherwise stateless, so
+// without this a `CREATE TEMPORARY TABLE …; INSERT …; SELECT …` script can't
+// see its own temp table. The id is per-tab (lazily minted) so tabs don't share
+// state and never collide on the per-session lock (only one query runs at a
+// time, guarded by `running`). No `session_timeout` override is needed:
+// ClickHouse resets the idle timer when each query is *released* (end of the
+// request, not the start) and cancels it while a query runs, so the default
+// (60s) never lapses between a script's back-to-back statements.
+//
+// Only TEMPORARY tables and session `SET`s need a session; permanent DDL/DML
+// and SELECTs are global. So we attach a session_id ONLY when the SQL needs
+// one — or when the tab already opened one (sticky, so a temp table / SET
+// from an earlier run stays visible to later runs in that tab). Ordinary
+// scripts run session-LESS, which avoids the session lock / replica-affinity
+// reset that intermittently surfaces as a "Network error". (Schema /
+// reference loads are always session-less — they fan out in parallel and
+// would deadlock on the lock.)
+
+import { leadingKeyword } from '../core/sql-split.js';
+
+/** The one field this module reads/writes on a tab — real callers always pass
+ *  a `state.ts` `QueryTab` (whose own `chSession?: string` satisfies this
+ *  structurally), but the policy itself needs nothing else from a tab. */
+export interface ChSessionCarrier {
+  chSession?: string;
+}
+
+export interface ChSessionParamsDeps {
+  /** A unique id for the `session_id` (mints `uid('sess-')`). Same seam
+   *  app.ts's own `uid` already is (crypto.randomUUID, with a non-secure-
+   *  context fallback) — injected here rather than re-derived. */
+  uid: (prefix: string) => string;
+}
+
+export interface ChSessionParams {
+  sessionParams(tab: ChSessionCarrier): { session_id: string };
+  needsSession(sqls: string[]): boolean;
+  sessionParamsFor(tab: ChSessionCarrier, sqls: string[]): Record<string, string>;
+}
+
+/** Build a `ChSessionParams` bound to `deps`. Trivial constructor — no
+ *  validation, no defaulting. */
+export function createChSessionParams(deps: ChSessionParamsDeps): ChSessionParams {
+  function sessionParams(tab: ChSessionCarrier): { session_id: string } {
+    tab.chSession = tab.chSession || deps.uid('sess-');
+    return { session_id: tab.chSession };
+  }
+
+  function needsSession(sqls: string[]): boolean {
+    return sqls.some((s) => /\bTEMPORARY\b/i.test(s) || leadingKeyword(s) === 'SET');
+  }
+
+  function sessionParamsFor(tab: ChSessionCarrier, sqls: string[]): Record<string, string> {
+    return tab.chSession != null || needsSession(sqls) ? sessionParams(tab) : {};
+  }
+
+  return { sessionParams, needsSession, sessionParamsFor };
+}