refactor(#586): SurfaceLifecycle primitive and docked right-inspector slot (phase 1)

[BorisTyshkevich]

What & why

Closes #586. Part of #593 (phase 1 of 8 — the ADR-0004 vanilla-shell investment track).

Replaces six copy-pasted overlay lifecycles with one SurfaceLifecycle primitive, and gives
.main-row a real shell-owned docked right slot instead of three independently mounted
position: fixed overlays on document.body.

• src/ui/surface-lifecycle.ts (new) — idempotent single-funnel close(), capture-phase
  Escape with an explicit escapePolicy: 'always' | 'focus-inside' | 'none', optional
  keyboard-owner acquisition, and a returnFocusTo element-or-resolver contract (the resolver
  is called at close time, so it can hand back whatever is on screen then rather than a
  possibly-detached element captured at open time).
• src/ui/inspector-host.ts (new) — the "one occupant of the shared dock" manager, keyed by
  a WeakMap per host element rather than a module global.
• src/ui/app-shell.ts — inspectorHost + inspectorResize mount as real layout siblings
  of queryHost/dashboardHost, with a new 'rightInspector' splitter axis and folded state
  expressed as hidden (the pattern showHost already uses).
• Cell drawer, rows viewer and Reference pane all migrate onto both primitives.
• Deleted: isTopDrawer, the .cd-backdrop DOM probes and CSS, the 'docPane' splitter
  branch, and drawer.ts's stateKey plumbing.
• cellDrawerPx/docPanePx collapse into one rightInspectorPx preference — compat read
  order rightInspectorPx → docPanePx → cellDrawerPx → 480px default, with a single
  canonical write (splitters.ts:140).
• Per refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586's "do not delete, rewrite" caveat, the header comments in drawer.ts,
  doc-pane.ts and results.ts that documented the old three-surface split as deliberate
  (Version-exact ClickHouse reference documentation in Workbench #60/Cell-detail right-hand drawer: add horizontal resize #101/Panels: visualization registry + Panel drawer tab + Library panel field #166/Docs Phase 1: rich function reference in CM6 hover and docs pane #313) are rewritten to describe the shared-lifecycle model, so a future reader
  cannot conclude the split is still intentional.

Two deliberate behaviour changes

1. Docked surfaces are now non-modal. They no longer acquire the keyboard owner. The
   pre-refactor(shell): one SurfaceLifecycle primitive and a docked right-inspector slot #586 modal cell drawer disabled every app shortcut (Run, Save, …) while open, because
   shortcuts.ts:264 bails when ownsKeyboard(app). Consequence: app.ts's Query↔Dashboard
   surface transition, sign-out, and connection-scope teardown now close whichever surface
   occupies the dock (closeInspector) rather than only Reference — the old code's comment
   explicitly relied on cell-detail's modality to explain why it didn't need closing there.
2. The dock holds one occupant, so opening Cell while Rows is showing replaces Rows instead
   of stacking a second panel. Tool-registry/tab persistence semantics belong to Unify cell detail, rows and documentation in a foldable desktop right inspector #488, which
   this issue deliberately does not implement.

The one surviving non-docked case — a cell-detail drawer opened inside a real detached browser
tab — keeps a self-contained overlay (renamed .cell-detail-overlay), still built on
SurfaceLifecycle.

Review findings fixed in ad88a88

• Two assertions had become unfalsifiable. Deleting .cd-backdrop repo-wide (AC3) left
  tests/e2e/editor-docs.spec.js's .cd-backdrop count probe and
  tests/unit/doc-pane.test.ts's querySelector('.cd-backdrop') passing unconditionally — a
  scrim reintroduced under any other class name would still have reported green. Re-pointed at
  real rendered claims: the editor is the topmost element at its own centre
  (elementFromPoint), and the inspector host's child is the panel, not a wrapper.
• AC2's "not a position: fixed overlay" had no automated coverage. happy-dom cannot
  evaluate CSS layout, so the unit suite could only prove DOM sibling order, hidden and the
  inline width write — the geometric claim that is this phase's whole point rested on one manual
  browser check. Adds tests/e2e/inspector-dock-layout.spec.js (chromium + webkit): folded
  contributes zero layout width, opening narrows .query-host and the inspector's box never
  intersects it, and dragging the handle resizes live.
• A lifecycle leak. openSurfaceLifecycle installs its capture-phase Escape listener before
  showInInspector's return value reveals whether a shell is mounted, so a failed mount left a
  permanent unclosable document listener — doc-pane.ts skipped only its panes bookkeeping and
  results.ts didn't check the return value at all. Both now tear the lifecycle down, covered by
  tests asserting the same listener reference is added and removed.

Verification beyond a green suite

Because two of this PR's central claims are the kind that pass vacuously, both were
sabotage-checked rather than trusted:

• forcing .inspector-host to position: fixed fails the new geometry spec (2 of 3 tests);
• reordering the compat chain so legacy keys win fails tests/unit/state.test.ts.

The compat-order tests use three distinct values (500/420/560) per source, so they discriminate
ordering rather than passing on equal expected values.

Full gate, run explicitly (.npmrc sets ignore-scripts=true, so npm test alone does not run
pretest): npm test 203 files / 6696 tests, coverage 100 / 96.86 / 100 / 100 with
perFile: true; npm run check:types clean; npm run check:arch clean (185 files, 9 rules);
npm run build clean; npx playwright test --project=chromium --project=webkit 404 passed,
4 skipped, 0 failed.

Follow-up filed: #595 (inbox) — DashboardApp.prefs is vestigial now that Dashboard's
cell-detail always docks; deliberately not folded into this PR.

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 (or it's a deliberate, justified addition — see CONTRIBUTING)
• README / CHANGELOG.md ([Unreleased]) updated if behavior or the deployed surface changed
• Reconciled affected tracked work (roadmap Roadmap to 1.0.0 #68, the issue body, ADR/CHANGELOG) if this change reshaped it

🤖 Generated with Claude Code

https://claude.ai/code/session_01Da66KLYSmCey6Gi7RMFGcf

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: ad88a88d25dbd96f0562c00b5140d6c2270e6ce0

I reviewed the canonical 31-file PR diff against base c470de091c1e48241fb5e757488c1e7904e88651, the current PR discussion/history, the new lifecycle/host primitives, all changed production files, and the changed tests/fixtures. The Actions run for this head is green. I could not independently clone and execute the suite in this runtime because outbound DNS to github.com is blocked, so the findings below are code-path analyses not locally reproduced failures.

1. [major] Closing or unmounting the inspector does not cancel an active shell resize drag

startDrag() deliberately returns a cancel function because a right-side surface can disappear while the mouse is still down. The surviving detached-overlay path stores that function and cancels/reverts it from its close lifecycle. The new shell path in app-shell.ts, however, calls doStartDrag(e, 'rightInspector', dragCtx) and discards the return value; releaseInspector() only hides/clears the host.

Reproduction path:

1. Open Cell/Rows/Reference.
2. Press and hold the inspector separator and move it.
3. While still holding the mouse, close with Escape, sign out, switch surfaces, or otherwise replace/fold the occupant.
4. The window mousemove/mouseup listeners remain active. Later pointer movement still mutates state.rightInspectorPx and the hidden host; the eventual mouseup persists an abandoned width.

This is specifically the lifecycle leak that the old attachDrawerResize() cancellation protected against, now reintroduced at the shell ownership boundary.

Action: make the shell own the active inspector-drag cancellation handle, invoke it from inspector fold/teardown and shell dispose(), and restore the pre-drag transient state on cancellation. Add a test that closes mid-drag, then sends later mousemove/mouseup events and asserts no style/state mutation and no preference write.

2. [major] The overlay-era 92vw maximum is unsafe for a docked layout, and the width is not re-clamped when the inspector opens

clampDrawerWidth() still permits 0.92 * viewportWidth. That was reasonable for a fixed overlay, but the new host is flex: 0 0 auto beside a non-shrinking sidebar and two separators. At a 1280px viewport, the inspector alone may claim about 1178px; adding even the minimum 180px sidebar and handles leaves no usable center surface. The current geometry test only uses the default width.

There is a second stale-width path: app-shell.ts clamps once when the shell mounts, while showInInspector() merely unhides the existing host. If the browser moves from a wide monitor to a narrow viewport while the inspector is folded, reopening uses the old inline width without a fresh clamp, despite the state comment claiming the full clamp applies whenever the inspector is opened.

Action: use a dock-aware temporary display clamp based on the actual .main-row space and a defensible center minimum; preserve the user's preferred width separately. Recompute on unfold and viewport/layout changes, not only at shell construction. Add browser tests for maximum/legacy widths and for resize-after-mount while folded and while open.

3. [major] Cell and Rows consume Escape globally even though the dock is non-modal; mobile is simultaneously full-screen but leaves background shortcuts active

Both docked Cell and Rows use escapePolicy: 'always' while deliberately not acquiring the keyboard owner. Consequently, an Escape pressed in the editor or results is captured by the inspector first and cannot execute the existing global running-query cancel action. The added unit test explicitly locks in app.actions.cancel not being called. The absence of stacked drawers does not justify global Escape capture; stacking and focus scope are independent concerns.

The same document-based dock branch is also used at mobile widths, where CSS turns .inspector-host into a fixed, full-screen surface. That branch still acquires no keyboard owner, so hardware-keyboard application shortcuts remain active behind a visually full-screen panel. This is a mobile behavior change even though mobile redesign is outside this phase.

Action: for the desktop non-modal dock, use focus-inside semantics so Escape outside preserves global behavior. For the mobile full-screen presentation, explicitly retain the modal ownership/focus behavior or introduce a presentation-aware lifecycle policy. Add real-browser tests covering Escape with focus inside vs. outside, running-query cancellation, and mobile hardware-keyboard behavior.

4. [minor] The compatibility read order selects non-empty invalid values before validating them

createState() chooses the first truthy stored string and only then calls parseInt()/clamp(). A malformed non-empty rightInspectorPx such as "bad" therefore blocks a valid docPanePx/cellDrawerPx fallback and produces NaN; clamp() preserves NaN, and the shell attempts to set a NaNpx width. This contradicts the documented “real/valid value wins” migration rule.

Action: parse and validate each candidate independently, choose the first finite valid width, then clamp it. Add cases for malformed canonical and legacy values, whitespace, and out-of-range numbers.

Verdict

Not ready to merge until the three major lifecycle/geometry/input-scope issues are addressed. The primitive extraction and failed-mount listener fix are directionally sound, and the current CI run is green, but the uncovered transition states can leave stale global listeners/preferences or make the center surface unusable.

[BorisTyshkevich]

Disposition of the ChatGPT pass-1 review

Every finding was verified against the code before being accepted — none were taken on trust.
All four were real. Three are fixed here; the fourth is deferred by an explicit owner
decision, recorded rather than dropped.

#	Finding	Status
1	Closing the inspector does not cancel an active resize drag	Fixed — 8539d81
2a	The 92vw maximum starves the centre now that the host takes real layout space	Fixed — 8539d81
2b	Width is not re-clamped on unfold, and two comments asserted otherwise	Fixed — 8539d81
3	escapePolicy: 'always' on a non-modal dock; mobile full-screen but non-modal	Deferred to #488 (owner decision)
4	Compat read order picks a non-empty invalid value over a valid fallback	Fixed — 8539d81

Verification of each accepted finding

• 1 — splitters.ts:115 startDrag() returns stop; app-shell.ts:216 discarded it, and
  releaseInspector only hid the host. Confirmed reachable: close mid-drag and the mouseup
  persists an abandoned width via ctx.save('rightInspectorPx', …). The shell now owns the
  cancellation handle, invoked from releaseInspector (the single choke point all close paths
  funnel through) and from dispose(), reverting to the pre-drag width. Fixing this surfaced a
  second bug the fix itself would have introduced — the handle must also clear on a normal
  mouseup, or a later ordinary fold would revert an already-persisted resize.
• 2a — clampDrawerWidth was clamp(px, 320, viewportWidth * 0.92), reachable via
  dragValue's 'rightInspector' branch by dragging the handle to the far left. Replaced with a
  dock-aware ceiling reserving a 320px centre minimum (the inspector's own existing floor, not an
  invented number) plus the tracked sidebar width and handles. The clamp changes only the
  displayed width — the persisted preference is never narrowed, so a trip through a narrow
  viewport no longer costs the user their width.
• 2b — showInInspector only set hidden = false, so the sole clamp ever applied was at
  shell mount. Re-clamp now runs on unfold and on window resize. Both misleading comments
  (state.ts:655, splitters.ts:36) are now true statements. Worth noting on its own: the read
  order had a sabotage-proof test, but nothing checked whether the comments describing the
  clamp were accurate — and they weren't.
• 4 — || short-circuits on any non-empty string, so "bad" in the canonical key both
  blocked a valid legacy fallback and produced NaN (clamp preserves NaN), which the shell
  then wrote as a literal "NaNpx" width. Each candidate is now parsed and validated
  independently; precedence among valid values is unchanged.

Finding 3 — deferred, with the consequence stated

Recorded in #488's body under
"Inherited from #586 — two deliberate deferrals", with the file:line starting state, and it falls
under #488's existing acceptance criteria 9–11.

Being explicit about what that means for this PR, so it reads as a decision and not an oversight:
this ships a mobile full-screen panel that leaves hardware-keyboard shortcuts live behind it.
Pre-#586 the mobile cell drawer was full-screen and modal, so that is a real mobile behaviour
change against one of #586's own non-goals. On desktop, Escape with focus in the editor closes
the docked panel rather than reaching the global running-query cancel.

The desktop half cannot be fixed in isolation: switching Cell/Rows to 'focus-inside' also
requires auto-focus-into-panel-on-open, or "click a cell, immediately press Escape" stops
working — and that focus contract belongs to #488, not to this shell-primitive phase.

Anti-vacuity checks

Two claims of this PR are the kind that can pass without testing anything, so both were
sabotage-checked rather than trusted, as was each new guard from the fixes:

Sabotage	Result
Force .inspector-host to position: fixed	Geometry spec fails (2 of 3)
Reorder the compat chain so legacy keys win	state.test.ts fails
Remove the drag-cancel hook from releaseInspector	app-shell.test.ts + inspector-host.test.ts fail
Neutralise the dock-aware ceiling	9 unit failures
Revert the unfold re-clamp hook	1 failure
Revert the window resize listener	1 failure

An earlier round had already fixed two assertions that had become unfalsifiable once
.cd-backdrop was deleted repo-wide, plus a capture-phase Escape listener that leaked when
showInInspector failed to mount.

Gate at this head

npm test 6720 tests / 203 files, coverage 100 / 96.87 / 100 / 100 perFile;
npm run check:types, npm run check:arch (185 files, 9 rules), npm run build all clean;
npx playwright test --project=chromium --project=webkit 414 passed / 4 skipped / 0 failed.
Each command run explicitly, since .npmrc sets ignore-scripts=true and npm test alone does
not run pretest.

Follow-up filed: #595 (inbox) — the now-vestigial DashboardApp.prefs field.

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: ad88a88d25dbd96f0562c00b5140d6c2270e6ce0

Reviewed head: dfd91413e92a2ebe297ec54f97e191b9b282a744

I compared the two new commits (8539d817448b60773417bbfc4f0ce112af68a9ab and dfd91413e92a2ebe297ec54f97e191b9b282a744) against the prior head, then re-inspected the complete 31-file PR and the current CI result. The canonical Actions run for this head is green. A local checkout/test run was not possible because this runtime still cannot resolve github.com, so the findings below are based on the canonical source, diff, tests, and CI metadata.

Prior findings reassessed

• Pass-1 finding 1 (active resize survives close): fixed. The shell now retains the drag cancel handle, releaseInspector() and shell disposal invoke it, the pre-drag preference is restored, and normal mouseup clears the cancellation handle.
• Pass-1 finding 2 (unsafe/stale width clamp): partially fixed. The new displayed-width clamp accounts for sidebar/handle reservation and is rerun on unfold and window resize. Two reachable layout transitions still violate the claimed centre-width invariant, detailed below.
• Pass-1 finding 3 (global Escape capture and mobile full-screen non-modality): unresolved. It is documented as deferred, but the user-visible regressions remain in this PR.
• Pass-1 finding 4 (invalid canonical preference produces NaN): the NaN path is fixed. Fully nonnumeric values now fall through; numeric-prefix garbage is still accepted as valid, detailed below.

1. [major] The new clamp still does not guarantee CENTRE_MIN_PX when the desktop row cannot fit both 320px minima

clampDockedInspectorWidth() computes ceiling = totalWidth - reservedPx - 320, but then calls clamp(px, 320, ceiling). When ceiling < 320, the generic clamp returns the 320px inspector floor, sacrificing the centre minimum that the helper and comments claim to guarantee.

This is not confined to the mobile layout. The full-screen override begins at 768px, while the dock remains active above it:

• At 900px with the default 248px sidebar, the new e2e test itself calculates a 318px centre and asserts only that the inspector is 320px.
• At 769px with the default sidebar, the centre is about 769 - 248 - 14 - 320 = 187px.
• At 1024px with the allowed 420px sidebar, the centre is about 1024 - 420 - 14 - 320 = 270px.

The test named “protect the centre surface” only requires queryBox.width > 200, not >= CENTRE_MIN_PX, so it cannot enforce the stated invariant.

Action: define an explicit fallback for the infeasible range instead of relying on a clamp with hi < lo: switch to the full-screen presentation, fold/narrow the sidebar, permit a smaller inspector, or raise the presentation breakpoint based on the actual reserved width. Add browser coverage at 769/900/1024px and with sidebarPx = 420, asserting the centre width against the actual contract constant.

2. [major] A live left-sidebar resize does not re-clamp an already-open inspector

The new re-clamp runs at shell construction, inspector unfold, and window.resize. The sidebar splitter's dragCtx.apply('col', value) only changes sidebar.style.width; it does not call reclampInspectorWidth(). Therefore the reservation used to choose the inspector width becomes stale while both panels are visible.

Concrete path at 1280px: open a 600px inspector with the default 248px sidebar (centre ≈418px), then drag the sidebar to its allowed 420px maximum. The inspector remains 600px and the centre falls to roughly 246px until an unrelated window resize or fold/unfold occurs.

The new e2e sidebar test avoids this path by closing the inspector, changing the sidebar, and reopening it. The unit test likewise changes state.sidebarPx while folded, so neither covers the live transition.

Action: re-clamp the displayed inspector width during the 'col' splitter apply path (without mutating the saved inspector preference), and add a real-browser test that drags .col-resize while the inspector remains open.

3. [major] Cell and Rows still capture Escape globally, and mobile remains full-screen but non-modal

Cell and Rows still use escapePolicy: 'always' with no keyboard-owner acquisition on the docked path. Escape pressed in the editor/results is therefore consumed by the inspector before the existing running-query cancel action. Below 768px, CSS changes the same host into a fixed full-screen panel, but hardware-keyboard application shortcuts remain live behind it.

The strongest counterargument is that changing to focus-inside requires a deliberate focus-on-open policy so immediate Escape behavior is defined. That makes this a cross-cutting design decision, but deferring it does not remove the current regressions: desktop Escape outside the panel no longer has its prior global meaning, and mobile behavior changed despite mobile being a non-goal.

Action: resolve the presentation/focus policy before merge, or explicitly separate mobile and desktop lifecycle policies in this phase. Add Chromium/WebKit coverage for Escape with focus inside and outside, running-query cancellation, and mobile hardware-keyboard shortcuts.

4. [minor] The compatibility parser still treats malformed numeric prefixes as valid values

firstValidPx() uses parseInt(raw, 10) and accepts any finite result. Values such as "420px" or "1e3" therefore win over valid legacy fallbacks (420 and 1, respectively) instead of being treated as corrupt. This is narrower than the original NaN bug, but it contradicts the new fail-closed/“valid candidate” contract.

Action: validate the complete trimmed string before conversion, then select the first fully valid finite numeric candidate. Add trailing-junk and exponent-form regression cases.

Verdict

The drag-cancellation and core NaN fixes are sound, but the width fix is incomplete across reachable desktop layouts and live sidebar resizing, and the pass-1 Escape/mobile regression remains unresolved. Changes are still required before merge.

[BorisTyshkevich]

Disposition of the ChatGPT pass-2 review

All four findings were verified against the code. All four are real. By owner decision none is
fixed in this PR — each is filed as tracked work, because none is a regression against pre-#586
behaviour and finding 1's remedy is a product decision that touches #487/#488.

#	Finding	Verified	Disposition
1	Centre minimum not held when the row cannot fit both minima	yes	#597
2	A live sidebar drag leaves the open inspector's width stale	yes	#598
3	Global Escape capture; mobile full-screen but non-modal	yes (unchanged from pass 1)	#488 — deferred earlier by owner decision
4	Compatibility parser accepts malformed numeric prefixes	yes	folded into #591 (phase 3)

Verification detail

1 — confirmed, and the band is wider than the review states. clamp is
Math.max(lo, Math.min(hi, v)) (src/core/format.ts:8), so when
ceiling = totalWidth - reservedPx - CENTRE_MIN_PX drops below 320 the inspector's own floor wins
and the centre absorbs the shortfall. The centre only reaches CENTRE_MIN_PX at
totalWidth >= reservedPx + 640 — 902px with the default 248px sidebar, 1074px with
dragValue's 420px maximum. So the unprotected band is 769–901px by default and 769–1073px with
a wide sidebar, starting at 769px because the full-screen override is @media (max-width: 768px).
At 769px the centre is 187px.

splitters.ts:88-91 justifies the behaviour by delegating it to "styles.css's full-screen mobile
override under MOBILE_BREAKPOINT_PX" — arithmetically false, since that override ends at 768px
and the case persists to 901px. And
tests/e2e/inspector-dock-layout.spec.js:125 asserts toBeGreaterThan(200) in a test named for
protecting the centre, so it passes at 250px as readily as at 318px and cannot enforce the
invariant. Both captured in #597.

2 — confirmed. dragCtx.apply's 'col' branch (app-shell.ts:182-186) only sets
sidebar.style.width; the re-clamp runs at construction, unfold and window resize but not here.
At 1280px, a 600px inspector with the sidebar dragged 248→420 leaves the centre at ≈246px until an
unrelated resize or fold. Both new tests avoid the live path — the e2e one closes and reopens the
inspector, the unit one changes state.sidebarPx while folded. Captured in #598.

4 — confirmed and narrow. parseInt accepts "420px"→420, "1e3"→1, "0x10"→0,
"12abc"→12, so a corrupt canonical value still beats a valid legacy one and silently yields the
320 floor. Folded into #591, whose subject is precisely fail-closed decoding of persisted values.

Why none is a regression

Pre-#586 this surface was a position: fixed overlay: at 769px it covered ~707px and obscured the
centre completely, and the sidebar's width did not interact with it at all. Findings 1 and 2 are
incomplete realisations of an invariant this PR introduces, not behaviour that got worse.
That is the basis for filing rather than blocking.

Accepted cost of merging as-is

src/ui/splitters.ts:88-91 still asserts a guarantee the code does not provide. This is the third
comment in #586 to do so — the other two (state.ts:655, splitters.ts:36) were fixed in
8539d81. Recorded in #597's acceptance criteria.

Two things checked and found correct

So they are not re-litigated later: the drag path and the re-clamp path use the same reservation
formula (state.sidebarPx + HANDLE_PX * 2 against win.innerWidth), so they cannot disagree; and
the Dashboard-surface sidebar-hiding rules are confined to the mobile block, so reservedPx is
honest on desktop, where the dock is active.

Additionally filed from my own review

Not raised by either ChatGPT pass: HANDLE_PX = 7 (app-shell.ts:109) and
.col-resize, .inspector-resize { width: 7px } (styles.css:901) are independent declarations of
the same layout constant, with nothing linking them. A CSS-only edit silently invalidates
reservedPx and narrows the centre with no failing test. Folded into #592 (phase 8), whose
purpose is mechanically forbidding exactly this kind of regrowth.

Also noted there: the Math.min(totalWidth * 0.92, …) term in the new ceiling is effectively dead
— the 92vw side only wins above a ~6425px viewport (first binding around 8K), it is untested, and
its comment overstates its role.