refactor(#276): extract WorkbenchSession — phase 3a

CHANGELOG.md

@@ -10,6 +10,21 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
 ## [Unreleased]
 
 ### Changed
+- **Workbench run lifecycle extracted into a route-scoped `WorkbenchSession`**
+  (#276 Phase 3a). `run`/`runScript`/`runEntry`/`cancel` orchestration now
+  lives in `src/ui/workbench/workbench-session.ts`; the run bookkeeping
+  fields (`runT0`/`runQueryId`/`runTick`) and the in-flight `AbortController`
+  are private session state (the `RunState` cast and `AppState.abortController`
+  are gone — issue rule 5: cancellation belongs to its owner). The session is
+  the sole production writer of the `running` signal; the three run-coupled
+  reactive effects register through `session.attachShell(...)` with captured
+  disposers (idempotent on re-render — also fixes a latent double-registration
+  on the connect → re-render path), and `destroy()` releases effects, the
+  elapsed ticker, and any in-flight operation (unit-proven; no production
+  caller until Phase 5's route shells). The architecture guard now carries a
+  rule list: route sessions (`ui/workbench` ↔ `ui/dashboard`) must not import
+  each other, and the dashboard route must not import the editor. Behavior
+  byte-identical.
 - **Authentication and connection lifecycle extracted into `ConnectionSession`**
   (#276 Phase 2). OAuth PKCE login/refresh, Basic-auth probing, IdP config
   resolution, identity, sign-in/out, and the cross-tab dashboard auth handoff

build/check-boundaries.mjs

@@ -1,10 +1,13 @@
-// Architecture boundary guard (issue #276 Phase 0). Day-1 rule only: modules
-// under src/application/ must not import from src/ui/ or src/editor/ — the
-// route/session layer coordinates state and network, it does not reach into
-// DOM rendering or the CodeMirror editor adapters. `import type` counts too:
-// a type-only import still couples the layers at compile time. Later phases
-// (#276 Phases 3-5) extend the rule set (e.g. route sessions must not import
-// each other) — add checks here rather than growing a second script.
+// Architecture boundary guard (issue #276). Rule list, grown per phase:
+//   Phase 0 — src/application/ must not import src/ui/ or src/editor/: the
+//   service layer coordinates state and network, it does not reach into DOM
+//   rendering or the CodeMirror editor adapters.
+//   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.
+// `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.
 //
 // Hand-rolled regex scan, no AST parser: the codebase has no exotic import
 // syntax, so scanning for import/export specifiers is enough and keeps this
@@ -15,10 +18,16 @@ import path from 'node:path';
 import { fileURLToPath } from 'node:url';
 
 const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
-const appDir = path.join(repoRoot, 'src', 'application');
-const FORBIDDEN = ['src/ui', 'src/editor'];
 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). */
+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' },
+];
+
 function collectFiles(dir) {
   const out = [];
   for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
@@ -29,12 +38,6 @@ function collectFiles(dir) {
   return out;
 }
 
-const files = fs.existsSync(appDir) ? collectFiles(appDir) : [];
-if (files.length === 0) {
-  console.log('check-boundaries: no files under src/application/ yet');
-  process.exit(0);
-}
-
 // Matches, in order: static `import ... from '...'` (incl. `import type`),
 // `export ... from '...'` (incl. `export type`), a bare side-effect
 // `import '...'`, and dynamic `import('...')`. Each pattern requires only
@@ -71,25 +74,38 @@ function resolveRelative(fromFile, spec) {
 }
 
 const violations = [];
-for (const file of files) {
-  const source = fs.readFileSync(file, 'utf8');
-  for (const spec of extractSpecifiers(source)) {
-    if (!spec.startsWith('.')) continue; // bare/package specifiers can't reach src/ui or src/editor
-    const resolved = resolveRelative(file, spec);
-    const relResolved = path.relative(repoRoot, resolved).split(path.sep).join('/');
-    const inForbidden = FORBIDDEN.some((f) => relResolved === f || relResolved.startsWith(`${f}/`));
-    if (inForbidden) {
-      const relFile = path.relative(repoRoot, file).split(path.sep).join('/');
-      violations.push(`${relFile} → ${spec} (resolved: ${relResolved})`);
+let checkedFiles = 0;
+let activeRules = 0;
+for (const rule of RULES) {
+  const ruleDir = path.join(repoRoot, rule.dir);
+  const files = fs.existsSync(ruleDir) ? collectFiles(ruleDir) : [];
+  if (files.length === 0) continue; // directory not born yet — rule activates with it
+  activeRules += 1;
+  checkedFiles += files.length;
+  for (const file of files) {
+    const source = fs.readFileSync(file, 'utf8');
+    for (const spec of extractSpecifiers(source)) {
+      if (!spec.startsWith('.')) continue; // bare/package specifiers can't reach src dirs
+      const resolved = resolveRelative(file, spec);
+      const relResolved = path.relative(repoRoot, resolved).split(path.sep).join('/');
+      const hit = rule.forbidden.find((f) => relResolved === f || relResolved.startsWith(`${f}/`));
+      if (hit) {
+        const relFile = path.relative(repoRoot, file).split(path.sep).join('/');
+        violations.push(`${relFile} → ${spec} (resolved: ${relResolved}; ${rule.dir} must not import ${hit} — ${rule.why})`);
+      }
     }
   }
 }
 
 if (violations.length) {
-  console.error('check-boundaries: src/application/ must not import src/ui/ or src/editor/ (issue #276, day-1 rule):');
+  console.error('check-boundaries: layer-boundary violations (issue #276):');
   for (const line of violations) console.error(`  ${line}`);
   process.exit(1);
 }
 
-console.log(`check-boundaries: OK (${files.length} file${files.length === 1 ? '' : 's'} under src/application/, no ui/editor imports)`);
+if (checkedFiles === 0) {
+  console.log('check-boundaries: no files under any guarded directory yet');
+  process.exit(0);
+}
+console.log(`check-boundaries: OK (${checkedFiles} file${checkedFiles === 1 ? '' : 's'} across ${activeRules} active rule${activeRules === 1 ? '' : 's'}, no violations)`);
 process.exit(0);

src/state.ts

@@ -210,7 +210,6 @@ export interface AppState {
   bannerDismissedFor: Signal<string | null>;
   serverVersion: string | null;
   running: Signal<boolean>;
-  abortController: AbortController | null;
   schemaGraphAbortController: AbortController | null;
   resultView: Signal<'table' | 'json' | 'panel' | 'filter'>;
   exporting: Signal<boolean>;
@@ -394,11 +393,10 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState
     // Run state (signals): `running` flips the Run button + results pane via
     // effects; `resultView` is the active Table/JSON/Chart tab. Via `.value`.
     running: signal(false),
-    abortController: null,
     // In-flight schema-lineage fetch (issue #124's inline drawer graph) — its own
-    // AbortController, separate from `abortController` (run/script) and the
-    // export controllers, since a graph fetch isn't gated by `running` and a
-    // second click/drag must be able to supersede an in-flight one.
+    // AbortController, separate from the workbench session's own run/script
+    // controller and the export controllers, since a graph fetch isn't gated by
+    // `running` and a second click/drag must be able to supersede an in-flight one.
     schemaGraphAbortController: null,
     resultView: signal<'table' | 'json' | 'panel' | 'filter'>('table'),
     // True while a streaming Export (issue #87) is in flight — separate from
@@ -564,9 +562,12 @@ export function patchSpecDraft(
   return { ok: true, invalidTab: null, spec: tab.specParsed! };
 }
 
-/** The saved query a tab is linked to (via tab.savedId), or null. */
+/** The saved query a tab is linked to (via tab.savedId), or null. Narrowed to
+ *  exactly the slice it reads (`savedQueries`) — not the full `AppState` —
+ *  so a caller (e.g. `WorkbenchSession`'s own state slice) can pass a Pick
+ *  without a bridging cast. */
 export function savedForTab(
-  state: AppState, tab: Pick<QueryTab, 'savedId'> | null | undefined,
+  state: Pick<AppState, 'savedQueries'>, tab: Pick<QueryTab, 'savedId'> | null | undefined,
 ): SavedQueryV2 | null {
   return (tab && tab.savedId && state.savedQueries.find((q) => q.id === tab.savedId)) || null;
 }
@@ -851,9 +852,11 @@ export function markLibrarySaved(state: AppState): void {
 }
 
 // Push one history entry (most-recent first, capped at 50). Internal — the
-// exported recorders below supply the sql/rows/ms.
+// exported recorders below supply the sql/rows/ms. Narrowed to `history`
+// (the only field it reads/writes) so `recordScriptHistory` below can pass a
+// narrower-than-`AppState` slice through unchanged.
 function pushHistory(
-  state: AppState, sql: string | null | undefined, rows: number | null, ms: number,
+  state: Pick<AppState, 'history'>, sql: string | null | undefined, rows: number | null, ms: number,
   save: SaveJSON, now: number,
 ): void {
   const s = String(sql || '').trim();
@@ -890,9 +893,12 @@ export function recordHistory(
 }
 
 /** Record a successful multiquery script run as one history entry (the whole
- *  script text); per-statement row counts aren't meaningful, so rows is null. */
+ *  script text); per-statement row counts aren't meaningful, so rows is null.
+ *  Narrowed to `Pick<AppState, 'history'>` — the only field it reads/writes —
+ *  so a caller (e.g. `WorkbenchSession`'s own state slice) can pass a Pick
+ *  without a bridging cast. */
 export function recordScriptHistory(
-  state: AppState, sql: string, ms: number, save: SaveJSON = saveJSON, now: number = Date.now(),
+  state: Pick<AppState, 'history'>, sql: string, ms: number, save: SaveJSON = saveJSON, now: number = Date.now(),
 ): void {
   pushHistory(state, sql, null, Math.round(ms), save, now);
 }