refactor(#588): decompose the createApp composition root (phase 4)

[BorisTyshkevich]

What & why

Part of #588, phase 4 of the #593 refactor umbrella (ADR-0004's vanilla-shell investment track). src/ui/app.ts's createApp was a god composition root: ~3,246 lines, a 112-member App bag assembled via ~130 scattered app.X = ... assignments and typed only by an as App cast. This decomposes it along the four extraction seams the issue specifies, plus typed staged construction, landing as five sequential gate-green commits (one per wave), a coordinator-caught e2e-fixture fix, and a reconcile commit — all pure refactor, no behavior change (one narrow, deliberate exception noted below).

Delivery contract claimed:

1. app.ts shrinks toward ≈1,800 lines — landed at 2,226 lines (from 3,246). Above the plan's own ≈1,950±100 forecast band; the plan flagged this criterion as directional, not exact, and the actual number is reported honestly rather than the forecast.
2. The App interface shrinks — 112 → 105 required members. openSavePopover, handleSqlPopState, focusDashboardMember, syncSqlRoute, rewriteWorkspaceRoute, sourceTabId, documentVisible, getLastCommittedToken, serializeWrite, flushWorkspaceWrites, refreshWorkspaceFromStore are gone from the flat bag (repointed to app.nav.*/app.workspaceSession.* at every production consumer — verified by a whole-tree grep during readiness review, zero stragglers beyond the two e2e fixtures fixed in 0d5e511); App gains nav/workspaceSession.
3. All four extractions preserve current behavior — verified: every moved function body diffed against origin/main and confirmed behaviorally byte-equivalent, except one intentional bookkeeping improvement (variable-strip's varStripSig/etc. keyed to strip-element identity instead of the old app.dom-reset coupling — documented and tested) and two deliberately preserved pre-existing defects, both moved verbatim and present on origin/main before this PR: anchoredPopover.close() can clobber a newer popover sharing the same dom ref slot (filed as anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603, pinned by a characterization test in popover.test.ts), and the Workbench var-strip rebuild signature folds in enum option count but not option identities, so a same-cardinality option-set change doesn't trigger a rebuild (filed as Workbench var-strip rebuild signature ignores enum option identities (only counts them) #605, ChatGPT pass 1 caught this — verified via git show origin/main:src/ui/app.ts before accepting; documented in a code comment at the signature site, no test added since fixing/characterizing it further would be scope creep for a pure structural extraction).
4. keyboardOwnerChannel exists in exactly one place (src/ui/keyboard-owner.ts) — the three prior copies (file-menu.ts/library-assign-menu.ts/dashboard.ts) were confirmed byte-identical before unifying.
5. Adding a new stateful toolbar button now takes fewer files, but less of a reduction than an earlier PR-description draft claimed (ChatGPT pass 1 caught this — verified against the code before accepting): workbench-shell.ts still builds and inserts the pre-existing runBtn/saveBtn/varStrip DOM elements directly (workbench-shell.ts:132/136/160) — the extraction moved each element's render/update logic into its own controller module, not the element's construction/insertion. So a genuinely new stateful button under this pattern still touches 4 files: its controller module, workbench-shell.ts (element build + insertion), app.ts (flat delegate wiring), and app.types.ts (the delegate's type). That is a real reduction from the old 4–5 (no WorkbenchShellDeps member, no forwarding-literal entry, no separate updater symbol living inside createApp's ~2,200-line body) but not the 3-file or 2-file path an earlier description draft claimed — corrected here rather than left overstated.
6. npm run check:types, npm run check:arch, npm run check:schemas, npm run check:examples, npm test, and npm run build are green at every wave and at the final head (see Tests below).

Design decisions (deviations from the issue's literal text, all verified against the code, not just argued)

• applyCommittedWorkspace stays in app.ts, not moved into workspace-session.ts as the issue's "zero DOM" framing implied — it renders tabs, cancels dashboard-tree clicks, and rewrites the route on lost selection, which is real UI orchestration. workspace-session.ts owns queueing/tokens/broadcast/refresh-scheduling/listeners/beforeunload/provisioning only, and calls applyCommittedWorkspace as an injected hook.
• Variable-strip extraction target is a new src/ui/workbench/variable-strip.ts, not the issue-suggested variable-bar.ts — that module's VariableBarApp port is deliberately adapter-facing with neutral names (Rename shared VariableBar activation port to caller-neutral names #478) and a private combo-field type; forcing Workbench-specific state into it would break that contract.
• Type relocations: WorkspaceChangedMessage → workspace-session.ts; SurfaceCommandPort/DashboardFocusOutcome/WorkspaceRouteStatus → main-surface.ts (with re-export shims so existing importers compile unchanged) — required because src/application/* cannot import src/ui/, including type-only, and these types previously lived in src/ui/.
• Full design rationale, the 30-row invariant map (enforcement + sabotage case per invariant), and every ChatGPT plan-review finding's disposition are recorded in the ship-log comment on Umbrella: V2 architecture refactor — shell primitives, composition root, state reactivity, transport adapter #593 (this PR is phase 4 of that umbrella).

Review process (High risk — large cross-cutting refactor of the composition root every test constructs)

1. Plan, fable model at high reasoning effort: full delivery plan with a 23-row invariant map, migration order, and per-wave port designs.
2. One independent ChatGPT plan-mode review (required for High risk) found 9 real gaps — most seriously, an internally-contradictory Wave 5 construction algorithm and an under-specified module boundary that would have leaked UI orchestration into src/application/. A second fable-model pass verified every finding against the actual code and produced a revised plan closing all 9 (exact construction pseudocode with a TDZ safety argument, frozen port interfaces, 7 additional invariants, replacement sabotage designs, an honest AC5 worked example, and 3 corrected module designs).
3. Implementation, 5 sequential sonnet subagents (one per wave, since all touch app.ts), each producing its own gate-green commit with new tests at this repo's coverage floors and sabotage-verified invariant tests.
4. Independent verification after every wave (coordinator, not the implementing agent): git diff/git log against the self-report, full local gate re-run from a clean state.
5. e2e regression found and fixed by the coordinator: two Playwright fixtures (import-example-dashboard.html, oauth-document-recovery/index.html) called the old flat app.rewriteWorkspaceRoute/app.syncSqlRoute directly — production call sites were repointed by the implementing waves but these two raw-ESM fixtures were missed. Fixed in 0d5e511; full e2e suite (chromium+webkit) is green.
6. One targeted readiness review (fable model, read-only): spot-checked ~12 of the 30 claimed invariants against the real code (all confirmed enforced as claimed), confirmed the member-disposition and dependency-boundary claims by direct grep/re-run, and found 3 minor cleanup items (6 orphaned imports, 2 stale comments, missing CHANGELOG/wiki reconciliation) — all fixed in ca92c0b.

Tests

• 5 new test files at this repo's 100/95/90/100 coverage floors: variable-strip.test.ts, save-controller.test.ts, keyboard-owner.test.ts, workspace-session.test.ts, surface-navigation.test.ts.
• Full suite: 212 test files / 6,971 tests passing, 100% statements/functions/lines, ≥90% branches per file.
• Every ordering/lifecycle invariant (30 total, I-1 through I-30) has a named production enforcement mechanism and a sabotage case that was mutated, confirmed to fail the specific named test, then reverted before commit — not left in the tree.
• Playwright e2e: 414 passed / 0 failed, chromium + webkit.
• Zero tests were deleted for convenience — the branch's test-migration rule (stated in every worker's prompt) required composition-level tests to be kept and only mechanically repointed; the sole deletion (app.test.ts's direct documentVisible default-reader test, whose member moved fully inside workspace-session.ts's private construction) has documented replacement composition coverage.

Follow-ups filed

• anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603 (inbox) — the pre-existing anchoredPopover.close() stale-clobber defect (moved verbatim, not fixed here; see above).
• Workbench var-strip rebuild signature ignores enum option identities (only counts them) #605 (inbox) — the pre-existing var-strip rebuild-signature gap (enum option count vs. identity; moved verbatim, not fixed here; see above).

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
• CHANGELOG.md ([Unreleased]) updated
• Reconciled affected tracked work (Umbrella: V2 architecture refactor — shell primitives, composition root, state reactivity, transport adapter #593 ship-log, .wiki/Architecture.md, .wiki/Source-Map.md)

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: ca92c0bc6d20cb0c5af38563a36cae0ee2449839

Findings

1. [Major] The variable-strip rebuild signature does not include the resolved enum option values, so a same-cardinality schema change leaves a stale dropdown.

   • src/ui/workbench/variable-strip.ts:197-207 says the signature folds in the resolved enum options, but the implementation appends only c.enumOptions.length.
   • The enum controller is then constructed with a snapshot of ctl.enumOptions around src/ui/workbench/variable-strip.ts:287; when inferred options change from, for example, ['a', 'b'] to ['c', 'd'], the signature is unchanged and the existing dropdown is retained with the old choices.
   • This appears to be preserved from the pre-extraction implementation rather than introduced here, but that still conflicts with the PR contract's claim that anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603 is the one deliberately preserved defect, and no new test covers same-cardinality option drift.
   • Action: include stable option identities in sigNew (not only their count) and add a test that changes inferred enum members without changing their count; alternatively, explicitly document and track this as another preserved defect and correct the misleading signature comment.

2. [Major] AC5's claimed “demonstrated 3-file path” for a new stateful toolbar button is not demonstrated by the current wiring.

   • src/ui/workbench/workbench-shell.ts:99-104 still carries the updater callbacks in WorkbenchShellDeps, and src/ui/workbench/workbench-shell.ts:131-151 explicitly creates and inserts the toolbar buttons.
   • variable-strip.ts and save-controller.ts consume existing DOM refs/controllers; they do not demonstrate adding a new button element. A genuinely new toolbar button still requires at least the controller module, app.ts wiring/delegate, app.types.ts state/contract, and workbench-shell.ts construction/insertion: four files, not three.
   • Action: revise AC5 to state the currently demonstrated four-file path, or land a button-owned-element seam plus a test/example that actually proves the claimed three-file (or designed two-file) workflow.

3. [Minor] app.types.ts still documents the removed cast-based construction.

   • src/ui/app.types.ts:1-6 says createApp uses const app = {} as App plus property assignment, directly contradicting Wave 5's let app: App + one typed object literal.
   • Action: update the contract header so future maintainers are not directed back to the mechanism this PR removes.

Areas checked with no finding

• Construction/TDZ: I found no inspected factory that synchronously invokes an app-dereferencing injected hook before the typed literal is assigned. The editor ports are constructed and their potentially synchronous onDocChange callbacks registered only after the literal exists, and the new composition test exercises that eager-callback case.
• Dependency direction: workspace-session.ts and surface-navigation.ts import neither src/ui/** nor src/editor/**, including type-only imports.
• Ordering/lifecycle invariants: the write queue, post-transform stale-route fence, post-commit broadcast ordering, refresh coalescing, beforeunload generation tokens, surface-generation checks, resolve-before-navigate, and tree-reveal-before-query-open are enforced in production code. The new module tests use controlled interleavings against the real modules rather than value comparisons derived from one fake source.
• Moved responsibility: I found no production shadow copy of the extracted responsibility clusters left in app.ts; remaining code is composition/delegation or DOM orchestration that belongs there.
• Preserved anchoredPopover.close() can clobber a newer popover sharing the same dom refKey #603 defect: the stale anchoredPopover.close() clobber behavior is clearly flagged in production and pinned by a characterization test; it was not silently changed.

GitHub Actions for this exact SHA are green across Vitest/coverage, build, release-bundle smoke, Docker smoke, and Playwright. I could not run a separate local test pass because this execution environment's git client could not resolve github.com; the review used the canonical PR/repository contents and exact-head CI results.

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: ca92c0bc6d20cb0c5af38563a36cae0ee2449839

Reviewed head: 15f798a6a7250425689f31cbd269a8d6e592399a

Result

No new actionable findings.

The new head is exactly one commit ahead of pass 1. That commit changes comments only in src/ui/app.types.ts and src/ui/workbench/variable-strip.ts; executable code and tests are unchanged from the previously reviewed SHA.

Pass-1 finding reassessment

1. Var-strip enum option identity gap — addressed for this PR's contract. The runtime defect remains unfixed, but the pass-1 alternative action was completed: the signature-site comment now states that only option count is included, the PR description now identifies this as a second preserved pre-existing defect, and open issue Workbench var-strip rebuild signature ignores enum option identities (only counts them) #605 tracks the behavior. This removes the misleading invariant claim without expanding this structural-refactor PR into a behavioral fix.

2. AC5 three-file claim — resolved. The PR description now states the demonstrated path is four files and explicitly accounts for workbench-shell.ts constructing/inserting the element.

3. Stale cast-based construction comment — resolved. app.types.ts now describes the typed object-literal/no-cast mechanism instead of the removed as App construction.

Regression review

I rechecked the complete updated PR using the pass-1 full review as the baseline. Because the old SHA is the merge base of the new head and the only new repository changes are comments, the previously inspected production paths are unchanged: staged construction/TDZ behavior, application-layer dependency direction, workspace queue and broadcast ordering, beforeunload generation tokens, surface-generation guards, resolve-before-navigate, tree-reveal-before-open, responsibility removal, and the separately tracked #603 popover defect.

GitHub Actions for 15f798a6a7250425689f31cbd269a8d6e592399a are green across Vitest/coverage, SPA build, release-bundle smoke, Docker smoke, and Playwright.

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: 15f798a6a7250425689f31cbd269a8d6e592399a

Reviewed head: 15f798a6a7250425689f31cbd269a8d6e592399a

Result

No new actionable findings.

The canonical PR head has not moved since pass 2. Comparing the two exact SHAs reports identical: zero commits and zero changed files. The complete PR implementation and tests are therefore byte-identical to the pass-2 review baseline.

Earlier finding reassessment

1. Var-strip enum option identity gap: still accurately disclosed as a deliberately preserved pre-existing defect and tracked separately as Workbench var-strip rebuild signature ignores enum option identities (only counts them) #605. It remains a real runtime defect, but it is no longer an undocumented or misleading claim in this PR.
2. AC5 file-count claim: remains corrected to the demonstrated four-file path, including workbench-shell.ts element construction/insertion.
3. Cast-based construction documentation: remains corrected; app.types.ts describes the typed object-literal/no-cast construction.

Regression review

Because there is no delta from the pass-2 SHA, no regression can have been introduced after that review. The previously inspected staged-construction/TDZ behavior, application dependency direction, workspace write/broadcast ordering, beforeunload generation tokens, surface-generation guards, resolve-before-navigate ordering, tree-reveal-before-open ordering, responsibility removal, and characterized #603 defect are unchanged.

GitHub Actions for 15f798a6a7250425689f31cbd269a8d6e592399a remain green across Vitest/coverage, SPA build, release-bundle smoke, Docker smoke, and Playwright.