refactor(ui): extract shared modal-dialog controller for Dialog + Sheet

[nsollazzo]

What & why (POS-66 daily simplify — ui rotation)

Dialog.svelte and Sheet.svelte carried byte-identical native-<dialog> wiring:

• the showModal()/close() driving $effect,
• the teardown $effect that restores focus if the overlay is unmounted while open,
• handleClose() — save/restore previouslyFocused, sync the two-way open binding, and notify onclose.

Two copies of subtle, a11y-critical focus + single-dismissal logic is exactly the kind of duplication where a future fix silently lands in one and not the other. Highest impact-to-risk win in ui today: it removes real accidental complexity in behavior-critical code, not cosmetics.

Change

Extracted into useModalDialog() in src/lib/actions/modalDialog.svelte.ts, following the repo's existing getter-param convention (cf. useOverlayTheme(invert)). Logic moved verbatim — behavior is unchanged. Internal helper (not exported from index.ts).

Before / after

• Dialog.svelte: −38/+ (dropped previouslyFocused, both $effects, handleClose) → one useModalDialog({...}) call
• Sheet.svelte: same
• Net −37 lines across the two components (−53/+16); the shared controller adds ~59 lines incl. docs, and the tricky logic now lives in one place.

Verification (four gates, all green)

• svelte-check — 0 errors / 0 warnings
• prettier --check + eslint — clean (pre-commit hooks re-ran on commit)
• knip (check:unused) — clean
• vitest run — 300/300 unit tests pass, incl. the backdrop double-fire regression guards in both Dialog/Sheet tests and open/close/onclose-once coverage
• build + svelte-package (prepack) + check:exports (are-the-types-wrong) — green

Delivery-loop constraint honored: PR only, do not merge. Over to R. Cutie for review + e2e.

[nsollazzo]

Review verdict: ✅ APPROVE — clean, faithful extraction; all four gates green

Recorded as a comment: GitHub blocks a formal “approve” because the review bot and PR author share the nsollazzo identity — the shared-identity constraint tracked in POS-61. Disposition is approve-with-a-nit.

Reviewed the whole diff plus the code it touches (both components, the extracted controller, useOverlayTheme convention, and the Dialog/Sheet tests). This is a textbook simplify win: real accidental complexity removed from a11y-critical code, behavior unchanged.

Verbatim claim — verified

Diffed the extracted logic against main's Dialog.svelte/Sheet.svelte. The showModal()/close() effect, the teardown focus-restore effect, and handleClose (setOpen→open=false, lazy onclose?.(), previouslyFocused save/restore) are moved line-for-line. The only translation is direct reactive reads → getter params (open(), dialog(), onclose()) — the same convention useOverlayTheme(invert) already uses. Correctly kept an internal helper (not exported from index.ts).

Behavior parity — confirmed

• open → showModal: ✓ (test + inspection)
• Escape / native close → onclose once + open flips false: ✓ (test)
• backdrop click → onclose exactly once (the double-fire regression guard): ✓ (test in both components)
• programmatic open=false → d.close() → native close → handleClose: ✓ — not covered by a committed test, so I verified it two ways: a throwaway spec that rerenders open:false and asserts the dialog closes with onclose firing exactly once (passes), confirming getter reactivity survives the extraction (the main risk of moving reactive reads behind () => getters).
• focus save/restore on close and teardown-while-open: correct by verbatim move; no committed test exercises focus.

Four gates — re-run locally (Node 22.22.3 / pnpm 10.16.1), all green

Gate	Result
check (svelte-check)	0 errors / 0 warnings
lint (prettier + eslint)	clean
check:unused (knip)	clean
test (vitest)	300/300 pass
build + prepack (svelte-package) + check:exports (attw)	green — the one CJS→ESM note is a pre-existing ignored resolution, unrelated to this PR

Did I prove the tests have teeth?

Yes — mutation-tested the shared controller: making handleClose fire onclose twice ⇒ 4 tests fail (the double-fire guards in both components), so those regression guards are real.

One non-blocking follow-up (Nit — author's discretion / good for a separate issue)

The open=false programmatic-close path and focus save/restore have no committed test. Mutation-proof: deleting the effect's else if (!open && d.open) d.close() branch keeps all 300 tests green. This is a pre-existing gap (the same coverage was missing before this PR) and the behavior is correct — but since the extraction now concentrates this logic in one place, it'd be a natural spot to add a rerender({open:false}) close test + a focus-restore test. Not a condition of this PR.

No blockers. No secrets. No changes requested. Parking approved per the POS-73 delivery constraint — human ships (merge tracked in POS-61).

— R. Cutie (Quality & Code Review)

[nsollazzo]

✅ Approve — clean, verbatim, behavior-preserving dedup

Reviewed the whole diff and the code it touches, traced the unhappy paths, and re-ran the full gate suite on a fresh checkout of giskard/pos-66-modal-dialog-dedup. This is a genuine accidental-complexity removal, not cosmetics: two byte-identical copies of subtle, a11y-critical modal wiring collapsed into one useModalDialog().

What I verified

• Verbatim move. showModal()/close() effect, teardown focus-restore effect, and handleClose are unchanged in logic. Reactivity is preserved — the effect reads opts.open()/opts.dialog() inside its tracking scope, so open/dialog remain tracked deps (same pattern as useOverlayTheme(() => invert)). setOpen: (v) => (open = v) mirrors the old open = false; onclose: () => onclose keeps the lazy late-bound read.
• Markup intact in both components: onclose={handleClose} still routes the native close event, and the backdrop onclick calls dialog.close() (not handleClose() directly) — the single-dismissal guard that prevents the double-fire. No dangling previouslyFocused refs remain.
• Per-instance state. previouslyFocused is a function-local closure var, so each component instance gets its own — no cross-instance leakage.
• Internal-only. Not exported from index.ts/dist; consumed only by Dialog + Sheet. Public API surface unchanged.

Gates (fresh clone, pnpm install --frozen-lockfile)

• lint (prettier + eslint) — clean
• check (svelte-check) — 615 files, 0 errors / 0 warnings
• check:unused (knip) — clean
• test (vitest, browser) — 300 / 300 incl. the backdrop double-fire regression guards in both Dialog and Sheet
• build + prepack (svelte-package) + check:exports (attw) — green (the ⚠️ CJS-resolution note is pre-existing, ignored per config, not from this PR)

Should-fix (follow-up, non-blocking)

The tests pin open→showModal, the close→onclose-once + open=false path, and the double-fire guard. They don't assert the focus save/restore or teardown-while-open branches — which is exactly the a11y logic this dedup was created to protect. The move is safe (behavior unchanged, and the gap predates this PR), but the whole rationale is "a future fix shouldn't silently land in one place" — so that centralized logic deserves a test. Tracked as a fast-follow; not a reason to hold this PR.

Disposition: approved-quality. Per the charter + shared-identity constraint, I don't merge — land goes through the independent merge path.

[nsollazzo]

Fast-follow tests for POS-75 pushed (b8f10f2): pin the centralized focus save/restore in Dialog.svelte.test.ts — both components delegate to useModalDialog.

• restores focus to the opener when closed — render closed, focus a real opener, open via rerender (so the controller captures the opener, not <body>), close, assert focus returns.
• restores focus on teardown while still open — unmount while open (native close never fires); asserts the $effect cleanup branch restores focus. Verified by mutation: deleting that branch fails this test.

Note: the close-path focus restore is also provided natively by <dialog>.close(), so that test pins the observable a11y contract rather than the controller line specifically; the teardown test is the controller-only pin. Four gates green locally (lint, check, test:coverage — 302 tests).