app.doc is dead; app.document is never assigned (silently falls back to global document everywhere)

[BorisTyshkevich]

Found while implementing #100 (detached-tab primitive) — three independent code-review passes flagged the same latent inconsistency in src/ui/app.js.

Problem. createApp(env) builds the app object with a doc: field (src/ui/app.js:56, doc = env.document || document), but every other module reads app.document instead — explain-graph.js, results.js, schema-detail.js, file-menu.js, shortcuts.js, app.js itself (lines ~1598, ~1602). app.document is never assigned anywhere, so app.document || document always falls through to the global document. Meanwhile app.doc is set but grep shows it's read nowhere outside app.js's own closure — specifically renderApp(app, helpers) at src/ui/app.js:1463, which destructures const { state, doc } = app;.

Why this is currently harmless. In production, env.document is unset, so doc already equals the global document — the two names happen to coincide. In tests, fake-app.js's makeApp() passes document (the global happy-dom document) as env.document too, so again they coincide. tests/unit/app.test.js's one real-createApp test does the same.

Why it's a footgun. If a future test (or a future real scenario — e.g. nesting a detached view inside another detached view) ever needs app.document to resolve to something other than the global document, app.document || document will silently ignore whatever was injected and use the wrong document. This is exactly the kind of bug the injected-seam pattern (CLAUDE.md rule 2) is supposed to prevent.

Verified fix. The naive one-line rename (doc: → document: and nothing else) is unsafe: renderApp destructures app.doc, so that call site has to move too. And not every || document fallback is dead code to delete — some are load-bearing, intentionally supporting a minimal or null app. Checked every call site's actual tests before deciding:

Core rename (required):

• src/ui/app.js:56 — drop doc,; add document: doc, to the app object literal. Nothing outside the renderApp destructure reads app.doc (verified: zero hits for grep -rn "app\.doc\b" besides that one spot), so no compatibility alias is needed.
• src/ui/app.js:1463 — const { state, doc } = app; → const { state, document: doc } = app; (keeps the local doc name so the two doc.documentElement... lines right below are untouched).

Fallback is dead code — safe to drop || document and use app.document directly:

• src/ui/app.js:1598,1602 — only ever run against the live, fully-built app.
• shortcuts.js:26 (openShortcuts), file-menu.js:71,228 (openFileMenu, openConfirm) — every call site in src/ passes the real app; every test goes through tests/helpers/fake-app.js's makeApp(), which always sets document. No test or design intent depends on the fallback firing.
• results.js:334,671,818 (openRowsViewer, expandDataPane, openCellDetail) — same: internal callers in results.js/app.js always pass the full app (only the document target varies, via the separate targetDoc parameter these functions already have — keep that part unchanged); tests/unit/results.test.js only ever builds its app via makeApp().

Fallback is intentional and must stay — these are shared primitives explicitly designed to tolerate a minimal or null app (verified by dedicated tests, not just current-caller behavior):

• detached-view.js:126 (openInDetachedTab) — tests/unit/detached-view.test.js:221 asserts openInDetachedTab(null, ...) does not throw, and several other cases (lines 55, 61, 74, 78, 87, 99) pass app stubs with no document key at all.

• explain-graph.js:309,571 (openPipelineFullscreen, openSchemaView) — tests/unit/explain-graph.test.js:172 has openPipelineFullscreen(null, 'digraph {}'); // null app → global document seam, and line 394 passes an openSchemaView stub with no document.

• schema-detail.js:27 (openDetailPane) — tests/unit/schema-detail.test.js:168 has openDetailPane({ actions: {} }, NODE, { ddl: '' }); // no document/detailDocument.

  For these three, leave the existing (app && app.document) || document (and, where present, the targetDoc || prefix) exactly as-is — only the meaning of app.document changes (from "never set, always falls through" to "set when app is real"), not the fallback chain itself.

No existing test needs to change either way: the "safe to drop" call sites are only exercised with document already set, and the "must keep" call sites' null/minimal-app tests keep passing because their fallback chain is untouched. Run npm test after implementing — tests/vitest.config.ts holds branches to a ≥90% per-file floor (not a literal 100%; see its own comment on v8's branch strictness), so this should hold or improve slightly, not regress.

One new regression test is worth adding precisely because today's tests don't distinguish app.document from the global document — add to tests/unit/app.test.js: createApp({ document: customDoc, ... }) (a second, distinct happy-dom document, e.g. via document.implementation.createHTMLDocument(''), the same technique detached-view.test.js's makeWin() already uses) then assert app.document === customDoc. That's the actual seam this issue is about — without it, a future accidental revert of the rename (or a copy-pasted doc: field on some other controller-shaped object) would go undetected again, the same way the original bug did.

Not fixed as part of #100 — it's a pre-existing, cross-cutting inconsistency unrelated to that issue's scope, and touching it would require re-verifying every app.document call site rather than the ones #100 already touched.