refactor(ui): ConfirmDialog gains the keyboard contract, and two confirm() sites move onto it (#1924) by dolho · Pull Request #2780 · Abilityai/trinity

dolho · 2026-09-14T11:41:06Z

Related to #1924

Draft, and stacked on #2778 (refactor/1923-modal-keyboard-contract). Base is that branch, so this diff is only the ConfirmDialog work. Merge #2778 first.

Partial by design: the primitive half is done; 2 of ~9 call sites are migrated. See What's left.

The thread changes what this issue is

The title reads "replace confirm() calls". The ruling (webmixgamer, 2026-08-21) — re-verified on dev before starting — says the primitive is the blocker:

ConfirmDialog.vue has no focus trap, no initial focus on the safe action, no Esc handler, and its DOM order is confirm-first — so this issue's AC "Initial focus lands on the safe action" needs the primitive itself fixed, not just the call sites.

Confirmed: zero Escape / @keydown / focus() / role="dialog" in the file, and variant="danger" at :58 ahead of variant="secondary" at :66.

Also honoured from the thread: /m's approval submit step (#2370) is inline p19-shaped by design and is NOT re-modalised; ent#523's Reset is not a caller (operator ruled it acts without confirmation).

The confirm-first order is fixed without moving any button

ConfirmDialog now adopts BaseModal (from #1923) and marks its confirm data-destructive — which is exactly what focusTrap.initialFocusIndex skips.

So focus lands on Cancel while the DOM order and sm:flex-row-reverse stay as they were and nothing moves on screen. Reordering the buttons was the obvious fix and the worse one: it would change the rendered layout of a dialog seven components already use.

Those seven all gain Esc-to-close, a focus trap and focus return for free. None of them change.

Every data-testid preserved. confirm-dialog-backdrop is the one that goes — BaseModal owns the overlay now. I checked before removing it: no test or component referenced it, and the two that are used (confirm-dialog, confirm-dialog-confirm) are untouched.

Call sites migrated

site	verb	consequence restated
`SystemViewEditor` delete-view	Delete view / Keep view	"The view and its saved tag filter are removed. Agents and tags are not affected."
`GitPanel` clear-PAT	Clear token / Keep token	"This agent falls back to the platform's shared token, which may not reach its repository."

Both previously ran through native confirm() — no named verb, no consequence, unsafe action focused.

What's left

views/Settings.vue — 3 native confirm()
components/SchedulesPanel.vue — 3 native confirm()
views/MobileAdmin.vue — the bespoke Emergency-Stop overlay
GitConflictModal.vue Force Replace / Force Push, ExecutionsPanel.vue Stop — single-click destructive, no confirmation step

A note on the baseline

ConfirmDialog improved (raw_gray 9 → 7 — its bespoke backdrop and the gray-500/gray-900 overlay went away), and a stale ceiling fails the ratchet too, so the baseline had to move.

I edited it by hand rather than regenerating. A wholesale scan-raw-colors --baseline run:

deleted the entire refrozen provenance block — every named-increase note (_2718_note, _2616_note, ent554, _ent553_note, 2638, named_increases…), i.e. the record of why each ceiling sits where it does;
absorbed canvas/CanvasDocument.vue and views/SharedCanvas.vue — two files that landed on dev after this branch was cut and have nothing to do with it.

Scoped to the two entries that actually moved, per the rule the 2638 note already states. Verified after: 11 refrozen keys intact, neither canvas file present.

Verification

check	result
Full frontend suite	2870 / 2870
`npm run build`	clean
Raw-color ratchet	green (caught the stale ceiling first — that is how the improvement was found)
Loading-gate + scanline ratchets	green
Step 10 hygiene	staged by path; gitlinks clean; no markers

Not verified: the DOM behaviour itself — Esc firing, focus actually landing on Cancel, focus returning to the trigger. This repo has no jsdom/happy-dom and no @vue/test-utils (see #2778), so the rules are unit-tested and the wiring needs a browser. e2e/settings-tabs.spec.js touches ConfirmDialog and would be the place.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ

dolho · 2026-09-14T14:14:18Z

Review — `/review` pass

The load-bearing claim here is that data-destructive moves focus without moving the button. That is exactly the kind of thing a source-text test can assert while the DOM disagrees, so I checked both halves.

Attribute reaches the DOM. BaseButton is a single-root <button> with no inheritAttrs: false, so a bare data-destructive on the component lands on the element. Confirmed in the browser — the dialog opens with focus on Cancel, and document.activeElement.hasAttribute("data-destructive") is false:

opened: {open:true, focused:"Cancel", focusedDestructive:false, bodyOverflow:"hidden"}
Esc:    {open:false, focusReturned:"Revoke"}

Visual order preserved. DOM order stays confirm-first and sm:flex-row-reverse still renders Confirm on the right, so the change is focus-only as the comment claims.

Removed testid. data-testid="confirm-dialog-backdrop" is gone with the bespoke backdrop. Grepped src/, e2e/ and tests/ — no consumers. Safe.

Baseline. ConfirmDialog 9 → 7 is a genuine improvement (the backdrop div went away), and lowering it rather than leaving a stale ceiling is the right half of the ratchet. The _1923_1924_note records why it was hand-edited instead of regenerated — good, that is the trap #1924's own note describes.

[I1] The one rule most worth an e2e is the one with no e2e

"Initial focus on the safe action" is a security-adjacent behaviour: it is what stops a reflexive Enter from confirming a destructive action. It is currently guarded by a unit test over plain objects (isDestructive) plus my manual check above. A one-assertion e2e — open any destructive confirm, assert activeElement is Cancel — would make it permanent. Same coverage gap as #2778 [I3]; either PR is a fine home for it.

Clean

No critical findings.
Scope is tight: 4 files, all on the stated intent.
GitPanel / SystemViewEditor conversions replace confirm() without changing the verbs they guard.

dolho · 2026-09-18T08:56:24Z

/review — post-rebase (stacked on `refactor/1923-modal-keyboard-contract`)

Scope: CLEAN vs the commit; PARTIAL vs #1924 by declared design (Settings ×5, SchedulesPanel ×4 confirm(), MobileAdmin overlay, GitConflictModal/ExecutionsPanel single-click remain — needs a tracked follow-up so Related to doesn't strand them). Plan completion: 2 done / 1 changed / 5 not done (stated).

Rebase note: raw-color-baseline.json conflict resolved by keeping both dev's _ent625_note and this branch's _1923_1924_note; ratchet green.

Critical

None.

Informational

[I1] Double focus trap in FirstRunOverlay's confirm (7/10) — FirstRunOverlay.vue trapRoot() returns the confirm-dialog content while confirming and its document-level Tab handler wraps first/last itself; ConfirmDialog now sits inside BaseModal, whose overlay onKeydown also intercepts Tab and wraps. Both run per keystroke: Tab from Confirm → BaseModal moves to Cancel → FRO sees activeElement === last and moves back. Net: Tab sticks on Confirm, Shift+Tab on Cancel (nuisance, not lockout). Not browser-proven. Fix: return early for Tab in FirstRunOverlay.onKeydown when confirming, drop the trapRoot() confirm branch.
[I2] Nested modal releases the scroll lock early (8/10) — SystemViewEditor is a BaseModal; cancelling its nested ConfirmDialog clears body.overflow while the editor stays open. Pre-existing BaseModal shape (refactor(ui): a shared modal shell with Esc, focus trap and focus return (#1923) #2778 I4), first exercised here — ref-count it.
[I3] Nothing pins data-destructive on the confirm button (8/10) — a source-grep guard (the test_1535 idiom) would catch a future edit re-focusing the destructive button for all 10 callers.
[I4] Baseline diff carries —→— re-escaping (9/10) — JSON-equivalent noise from the rebase, not a defect.

Clean

No API calls; title/message via {{ }}; confirm-dialog-backdrop testid removal has no references; data-destructive falls through to BaseButton's root <button>.

@keydown

…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>

@keydown

…urn (#1923) (#2778) * refactor(ui): a shared modal shell with Esc, focus trap and focus return (#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 * fix(ui): BaseModal — shared ref-counted scroll lock, focusable overlay, 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@keydown

…irm() sites move onto it (#1924) Stacked on #1923 — `ConfirmDialog` adopts the `BaseModal` shell built there. The issue reads as "replace confirm() calls", but the thread rules otherwise (webmixgamer, 2026-08-21), and I re-verified it still holds on dev: ConfirmDialog.vue has no focus trap, no initial focus on the safe action, no Esc handler, and its DOM order is confirm-first — so this issue's AC "Initial focus lands on the safe action" needs the primitive itself fixed, not just the call sites. Confirmed: zero Escape / @keydown / focus() / role="dialog" in the file, and `variant="danger"` at :58 ahead of `variant="secondary"` at :66. **The confirm-first DOM order is fixed WITHOUT reordering the buttons.** The confirm is marked `data-destructive`, which is what `focusTrap.initialFocusIndex` skips, so focus lands on Cancel while the DOM order and `sm:flex-row-reverse` stay exactly as they were and nothing moves on screen. Reordering was the obvious fix and the worse one: it would have changed the rendered layout of a dialog seven components already use. Adopting BaseModal gives all seven existing callers Esc-to-close, a focus trap and focus return at once — none of them change. Every `data-testid` is preserved. `confirm-dialog-backdrop` is the one that goes (BaseModal owns the overlay now); checked first — no test or component referenced it, and the two that ARE used (`confirm-dialog`, `confirm-dialog-confirm`) are untouched. Call sites migrated here: `SystemViewEditor` delete-view and `GitPanel` clear-PAT. Each names the verb ("Delete view", "Clear token"), restates the consequence, and offers a named safe action instead of "Cancel/OK". Remaining on the issue and NOT done here: Settings.vue (3) and SchedulesPanel.vue (3), plus MobileAdmin's bespoke overlay and the two unconfirmed single-click actions. Not touched, by the thread's ruling: `/m`'s approval submit step (#2370) is an inline p19-shaped flow by design and must not be re-modalised. **Baseline edited by hand, not regenerated.** ConfirmDialog IMPROVED (raw_gray 9 -> 7 — its bespoke backdrop and the gray-500/gray-900 overlay went away), and a stale ceiling fails the guard too. A wholesale `--baseline` run deleted the entire `refrozen` provenance block and absorbed canvas/CanvasDocument.vue and views/SharedCanvas.vue — two files that landed on dev after this branch was cut — so the edit is scoped to the two entries that actually moved, per the rule the 2638 note already states. Related to #1924 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ

vybe · 2026-09-22T16:53:36Z

merge-train: rebased onto dev and retargeted — mechanical. The parent refactor/1923-modal-keyboard-contract squash-merged as #2778, so this branch was carrying its pre-squash commits and read as 5 conflicts against dev. Pushed git rebase --onto origin/dev origin/refactor/1923-modal-keyboard-contract: the single #1924 commit replayed cleanly, no content changed. Base is now dev. The rest of the stack (#2781 → #2787) gets the same treatment one at a time as each parent lands.

vybe

merge-train: validated (lane B), rebased onto dev, all checks green on the retargeted run.

dolho force-pushed the refactor/1923-modal-keyboard-contract branch from b9a41bd to 40c5321 Compare September 18, 2026 08:48

dolho force-pushed the refactor/1924-confirmdialog-destructive branch from f70cc87 to 39bb7b1 Compare September 18, 2026 08:48

dolho marked this pull request as ready for review September 18, 2026 08:48

dolho force-pushed the refactor/1923-modal-keyboard-contract branch from 40c5321 to c371b8a Compare September 21, 2026 10:14

dolho force-pushed the refactor/1924-confirmdialog-destructive branch from 39bb7b1 to e824ed2 Compare September 21, 2026 10:17

dolho requested a review from vybe September 21, 2026 11:37

dolho added a commit that referenced this pull request Sep 21, 2026

train: #2780

9464f5a

dolho mentioned this pull request Sep 21, 2026

DO NOT MERGE — merge train: 2778,2780,2781,2782,2783,2784,2785,2787,2922,2920,2924,2927 #2928

Closed

vybe changed the base branch from refactor/1923-modal-keyboard-contract to dev September 22, 2026 16:53

vybe force-pushed the refactor/1924-confirmdialog-destructive branch from e824ed2 to 26cfb9c Compare September 22, 2026 16:53

vybe approved these changes Sep 22, 2026

View reviewed changes

vybe merged commit 92d13fc into dev Sep 22, 2026
24 of 25 checks passed

Navigation Menu

refactor(ui): ConfirmDialog gains the keyboard contract, and two confirm() sites move onto it (#1924) - #2780

refactor(ui): ConfirmDialog gains the keyboard contract, and two confirm() sites move onto it (#1924)#2780
vybe merged 1 commit into
devfrom
refactor/1924-confirmdialog-destructive

dolho commented Sep 14, 2026

Uh oh!

dolho commented Sep 14, 2026

Uh oh!

dolho commented Sep 18, 2026

Uh oh!

vybe commented Sep 22, 2026

Uh oh!

vybe left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dolho commented Sep 14, 2026

The thread changes what this issue is

The confirm-first order is fixed without moving any button

Call sites migrated

What's left

A note on the baseline

Verification

Uh oh!

dolho commented Sep 14, 2026

Review — /review pass

[I1] The one rule most worth an e2e is the one with no e2e

Clean

Uh oh!

dolho commented Sep 18, 2026

/review — post-rebase (stacked on refactor/1923-modal-keyboard-contract)

Critical

Informational

Clean

Uh oh!

vybe commented Sep 22, 2026

Uh oh!

vybe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Review — `/review` pass

/review — post-rebase (stacked on `refactor/1923-modal-keyboard-contract`)