Cancel first, everywhere - #8017
Merged
Merged
Conversation
The audit left one question open because it needed a product decision rather than a sweep: 42 core dialogs render [Save][Cancel] and 12 render [Cancel][Save], with the plugins 48 to the other way. Decision is cancel-first, matching the property-workers modal. This adds the rule before the fix, deliberately red, same as the original gate. Baseline: 42 violations in core, 48 in the plugins. The rule fires only when a row contains a .btn-cancel that is not the first action button; single-button rows and icon-only triggers are ignored. Ordering is the one button inconsistency users actually feel, because it moves the button under their cursor between one dialog and the next — which is why it was worth deciding deliberately rather than normalising whichever way a sweep happened to run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015sXLtgzZU8QL9m84GqMkoJ
Reorders the action rows so the dismissing button leads, matching the
property-workers modal and the 12 dialogs that already did it. This was the
majority-vs-convention question the audit deliberately left open: 42 core
dialogs rendered [Save][Cancel] against 12 the other way, and it is the one
button inconsistency users actually feel, because it moves the button under
their cursor between one dialog and the next.
A pure move — whole <button> elements swapped position, nothing retyped.
Verified by diffing the removed against the added lines across all 42 files:
every removed line reappears, so no id, binding, tooltip, icon or translate
pipe was altered in transit. That check is the point; a re-typed button is how
an id silently disappears and takes an e2e selector with it.
Two behaviours that reordering could have broken, both checked:
- cdkFocusInitial appears on only two templates and, in a dialog row, sits on
a .btn-cancel — which moves to first anyway, so no autofocus target moves.
- Exactly one e2e spec selects a dialog button by position rather than id:
calendar-compliance-view.spec.ts clicks actionButtons.nth(0) and asserts
nothing was deleted. Its dialog was already cancel-first, so it is
untouched by this change and nth(0) still resolves to Cancel.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015sXLtgzZU8QL9m84GqMkoJ
There was a problem hiding this comment.
Pull request overview
Adds an explicit “cancel first” convention to the existing dialog-button convention gate, so CI can flag templates where .btn-cancel is present but not the leading action (ignoring single-button rows and icon-only triggers).
Changes:
- Updates the script documentation/output messaging to include the cancel-first ordering convention.
- Collects action-row buttons during scanning and introduces a new
cancel-not-firstviolation rule. - Extends the end-of-run guidance text to mention ordering alongside the existing class system rules.
Suppressed comments (1)
eform-client/scripts/check-button-conventions.js:298
- The violation summary message says "then the confirming action" but this convention also applies to destructive actions (e.g., .btn-delete). Align the wording with the header comment so the guidance stays accurate.
'and one order: cancel first, then the confirming action.\n' +
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+198
to
218
| // ---- rule 3: cancel comes first ------------------------------------ | ||
| // A dialog's dismissing action leads; the confirming or destructive one | ||
| // follows. Mixed ordering is the single button inconsistency a user | ||
| // actually notices, because it moves the button under their cursor | ||
| // between one dialog and the next. | ||
| const actions = rowButtons.filter(b => | ||
| b.classes.some(c => APPROVED_CLASSES.includes(c)) && | ||
| !ICON_BUTTON_ATTRS.some(a => b.attrs.includes(a)) | ||
| ); | ||
| const cancelAt = actions.findIndex(b => b.classes.includes('btn-cancel')); | ||
| if (cancelAt > 0) { | ||
| const leading = actions[0].classes.filter(c => APPROVED_CLASSES.includes(c)).join(' '); | ||
| violations.push({ | ||
| line: actions[0].line, | ||
| rule: 'cancel-not-first', | ||
| detail: `.${leading} precedes .btn-cancel — cancel leads the row`, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // ---- rule 2: no references to classes nothing defines ------------------- |
This was referenced Aug 24, 2026
Merged
…dn't see Two blockers from the second review gate. The three-button leave-confirmation was the only 3-button row in the sweep, and my instruction to "keep the other two in relative order" satisfied the rule mechanically while making the dialog worse: it left Cancel / Save & Leave / Leave, putting "discard my unsaved work" in the rightmost slot that the other 89 dialogs use for Save. Reordered to Cancel / Leave / Save & Leave so the safe confirming action holds the position users have been trained to click. The gate now also treats a `.modal-footer` as an action row. That hole is not hypothetical — it is exactly why workflow's Remove-site dialog stayed cancel-last while Assign-site, opened from the same settings screen, moved to cancel-first. A user clicking one then the other would have watched Cancel jump sides, which is the precise failure this whole change set out to remove. The offending footer is fixed in its own repo; with the wider rule, core and all nine plugin modules still report clean, so those two were the only ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015sXLtgzZU8QL9m84GqMkoJ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The one open question from Button Drift — button order — is decided: cancel first, matching the property-workers modal.
It was left out of the consolidation deliberately. Ordering is the only button inconsistency a user actually feels, because it moves the button under their cursor between one dialog and the next, so it needed a decision rather than whichever way a sweep happened to run.
The split being fixed
[Cancel][Save][Save][Cancel]Cancel-first was the minority. 90 dialogs move.
Same shape as before: the rule lands red first
The gate learned
cancel-not-firstin its own commit, failing at 42 core violations, so the finish line is visible in CI before any template moves.The rule fires only when a row contains a
.btn-cancelthat is not the first action button — single-button rows and icon-only triggers are ignored.The reorder is a pure move
Whole
<button>elements swap position; nothing is retyped. Everyid, binding,cdkFocusInitial, tooltip, icon and translate pipe survives byte-identical — verified by checking the diff is symmetric, every removed line reappearing as an added one.cdkFocusInitialstays on whichever button already had it, so autofocus targets do not change.🤖 Generated with Claude Code
https://claude.ai/code/session_015sXLtgzZU8QL9m84GqMkoJ