refactor(ui): a shared modal shell with Esc, focus trap and focus return (#1923) - #2778
Conversation
Review —
|
b9a41bd to
40c5321
Compare
/review — post-rebase (onto
|
merge-train — not on the 2026-09-20 train; rides the next one once fixedValidated as lane B. No CRITICAL from the checklist, but the
Both were named by both prior The stated reason for leaving the wiring untested no longer holds. The docblock ( Also noted, no action required for the train:
|
|
merge-train: ejected from today's train — rides the next one once fixed. Nothing was pushed to this branch. The
The stated reason it can't be tested is false on this PR's own head. The docblocks at The one piece of execution evidence points at a component this PR doesn't touch. The 2026-09-14 Two named defects sit exactly in the untested wiring, which is why this matters:
What unblocks it: roughly a 40-line Ejected rather than repaired in-train because a new behavioural test is a design decision, not a mechanical fix. The partial scope (2 of 6 modals — |
|
merge-train: not on the 2026-09-21 train either — carried over unfixed. Nothing was pushed to this branch. This branch's last commit predates yesterday's ejection comment, so the findings there stand unchanged and I have not re-derived them. See the note above for the detail and for what unblocks it. Not a re-review, just the queue state: this is the second consecutive train this PR has missed. If any of yesterday's findings look wrong to you, say so on the thread and I will re-verify that specific point rather than leave it parked — a disputed finding is a faster conversation than another week in the queue. |
…urn (#1923) Six bespoke overlays each re-implemented the same markup and each omitted the same two behaviours: Esc did nothing, and Tab walked out behind the overlay. Verified still true on `dev` before starting — **zero** `Escape` references across all six files (the `@keydown` hits there are Enter-to-submit), no shell primitive, no focus trap anywhere. `BaseModal` owns exactly four things — overlay, Esc, focus, scroll lock — and nothing about content, so adopting it is deleting two wrapper divs rather than rewriting a dialog. It teleports to `<body>`: several of these modals are declared inside panels that establish a stacking context, where a `z-50` overlay nested in one renders BEHIND its siblings. **The decidable half is a separate pure module, and that is the point.** This repo's vitest runs `environment: 'node'` with no jsdom/happy-dom and no `@vue/test-utils` — I checked, because a focus trap written entirely inside an SFC would be a rule no unit test could reach, and source-text assertions would prove only that it had been TYPED. `utils/focusTrap.js` holds every decision as a function over plain data (tabbable filtering, the Tab wrap, the dismiss-key predicate, safe-action selection, backdrop identity) with 25 tests. What that leaves uncovered is the wiring itself — listener attachment, the focus() calls — which needs a browser and belongs to e2e. Said plainly rather than implied by a green suite. Three decisions worth naming: * initial focus goes to the SAFE action, and destructiveness is DECLARED (`data-destructive`) rather than guessed from label text, which would be wrong in every language but English. A dialog that opens with Delete focused turns a reflexive Enter into data loss. * backdrop dismissal compares identity against the overlay node, not a rectangle — a rectangle test mis-fires for a select popup or date picker rendered at the document root and closes the modal under the user. * a modified Escape (Ctrl/Cmd/Alt/Shift) does not dismiss; that is a browser or OS gesture, not an intent to close. Migrated in this commit: `SystemViewEditor.vue` and `NavBar.vue`'s build-info modal. The remaining four files are enumerated in the PR with their exact overlay bounds; they are 80-230 line tag surgeries and are deliberately left for a reviewed pass rather than done blind in one go. One acceptance-criteria item is stale: `views/Agents.vue` was deleted by ent#260 (the Agents page folded into the Dashboard list view), so its bulk-tag popover no longer exists. Related to #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ
…y, and a mounted spec for the wiring (#1923) Merge-train review on #2778, twice: the shell's rules were tested, its wiring was not, and the docblocks justified that with a claim that was false on this head — the repo mounts components (per-file jsdom opt-in, @vue/test-utils, #2918). Two defects sat in exactly that untested wiring. 1. The scroll lock was global state written per instance: `immediate: true` ran the else-branch on every mount and `onBeforeUnmount` cleared unconditionally, so a nested dialog (the #2780 shape — a ConfirmDialog inside another modal's slot) unlocked the page the moment the outer opened, and any modal's unmount clobbered another component's lock. Now `utils/focusTrap.js::createScrollLock` is a ref count over one value (first holder saves and hides, last release restores) and every BaseModal shares `bodyScrollLock`, taking and releasing exactly its own count. 2. The overlay had no `tabindex="-1"`, so `overlay.focus()` was a no-op and Esc did nothing once focus left a control (a click on dialog text lands focus on the nearest focusable ancestor, which was <body>). `tests/unit/baseModal.spec.js` mounts the shell: initial focus on the safe control, Tab wraps, Esc emits close and focus returns, the overlay is focusable, a modal with no tabbable child is still dismissable, backdrop semantics, the lock is taken/restored, two nested modals share one lock, and unmount releases only what the instance held. Mutation-checked: gutting @keydown, the focus return, the tabindex or the lock each turns it red. Also: the false "node-only vitest" claims are rewritten in all three docblocks; the overlay's p-4 now IS the 16px phone gutter (the two adopted panels drop their mx-4, which had doubled it to 32); the design-system docs name BaseModal as the modal shell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
40c5321 to
c371b8a
Compare
Re: merge-train ejection — fixed in
|
merge-train — on the 2026-09-21 train, nothing pushed to this branchFourth offer, and it rides this time. Validated as lane B; your two posted I verified by mutation rather than by reading, 12 of them, and every one turns the suite red (36/36 green unmutated):
Both adopters genuinely route through the shell — Two non-blocking notes, neither worth a push:
|
vybe
left a comment
vybe
left a comment
There was a problem hiding this comment.
merge-train: batch validated on train/20260921-1644 (#2939) — full suite green. Fourth offer, and the three defects the earlier trains ejected on are verified fixed at the source: 12 mutations against this head all go red (scroll-lock refcounting, the overlay's tabindex, the focus trap's Tab wrap, backdrop identity). baseModal.spec.js genuinely mounts and drives real events; zero readFileSync in either spec. Nothing pushed to this branch.
2 participants
Related to #1923
Premise re-checked before starting
The issue is from 2026-07-31. Before claiming it I verified it is still true on
dev:components/base/had Badge, Button, Card, Input, Select, Textarea, Toggle; no modalEscapereferences across all six (the@keydownhits are Enter-to-submit)views/Agents.vue:143bulk-tag popoversWhat landed
utils/focusTrap.js— every decision as a pure function over plain data, with 25 tests.components/base/BaseModal.vue— the wiring: overlay, Esc, click-outside, focus trap, initial focus, focus return, scroll lock,role="dialog"/aria-modal.Migrated:
SystemViewEditor.vue,NavBar.vue(build-info modal).Why the rules are a separate module
This repo's vitest runs
environment: 'node'with no jsdom/happy-dom and no@vue/test-utils— I checked rather than assumed. A focus trap written inside an SFC would therefore be a rule no unit test could reach, and source-text assertions would prove only that it had been typed — the exact weak-coverage pattern the merge-train playbook names as its most common ejection.So the rules live where they can be tested, and the SFC is the dispatcher (the ent#392 precedent this codebase already uses).
What that still leaves uncovered, stated plainly: the wiring — that the listener is attached, that
focus()is actually called, that focus returns to the trigger. That needs a browser.e2e/exists and is the right home; this PR does not add one.Three decisions worth review
data-destructive), not guessed from label text — guessing would be wrong in every language but English. A dialog opening with Delete focused turns a reflexive Enter into data loss.<select>popup or date picker rendered at the document root and closes the modal under the user.Teleport to="body"is load-bearing: several of these modals sit inside panels that establish a stacking context, where az-50overlay nested in one renders behind its siblings.Why partial
Each remaining migration is an 80–230 line open/close tag surgery in a file I have not read end to end. Doing four of those blind in one pass is how a subtle markup break ships — the build catches structural errors but not a panel whose footprint quietly changed. Bounds computed by indent matching, ready for a reviewed pass:
GitConflictModal.vue:2:116GitConflictModal.vue(parallel-history):120:195GitPanel.vue(initialize):35:171GitPanel.vue(PAT):366:451SchedulesPanel.vue(create/edit):50:284TasksPanel.vue(execution log):406The migration is mechanical: replace the outer
v-ifoverlay div with<BaseModal :model-value="…" panel-class="…" @close="…">, replace the panel div with a bare<div>, close with</BaseModal>. Both completed migrations show it.Also outstanding from the ACs: the keyboard-reachability items (
SystemViewsSidebar.vuenested<button>,InfoPanel.vueclickable<div>s,NavBar.vueuser dropdown) — untouched here.Verification
npm run buildBaseModalportalLoadingTreatment(scanline allowlist)🤖 Generated with Claude Code
https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ