refactor(a11y): scope ConfirmCost to its own control, not to the whole scope - #112
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe confirmation system now tracks the armed control’s description ID. Discord unlink rows use individual cost notices with accessible associations and responsive styling. End-to-end tests cover visibility, isolation, disarming, and button position. ChangesDiscord unlink confirmation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin account page
participant Submit as ConfirmSubmit
participant Context as ArmContext
participant Cost as ConfirmCost
Admin->>Submit: Click Discord unlink control
Submit->>Context: Arm control ID and describedBy ID
Context-->>Cost: Provide armed control state
Cost->>Cost: Match description ID
Cost-->>Admin: Reveal matching cost notice
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
…e scope ConfirmCost read `armedId !== null` — "something in this scope is armed". That is correct in a scope holding one control and silently wrong in every other one, and #111 hit it: the admin accounts table wraps every row in one ConfirmArmScope, so it had to ship its cost sentence hidden always and asked for a per-control id. This is that id. ArmContext now carries the armed control's `describedBy` beside its useId, in one state object so a render can never see the new id with the previous description. A scope may now hold any number of controls and any number of cost sentences; controls sharing a sentence reveal it together. No behaviour change today. Both current call sites are scopes of one, so nothing observable differs, and there is no test: the difference needs a click, and the repo has no jsdom/testing-library — renderToStaticMarkup cannot arm a control. This lands as the footgun removal #111 asked for, not as a fix for a bug anyone can currently reach. The doc also records what #112 found while trying to use it in the admin table, which is the more expensive half of this to rediscover: revealing on arm inside a td widens the cell, the widening moves the armed button out from under a stationary mouse, pointerLeave fires, and the control disarms itself. The admin sentence stays visually-hidden always. Measured, not reasoned about — the first attempt appeared to work only because flex-basis:100% resolved against an inline-flex parent and squeezed the note to 77px instead of its intended 34ch.
guarzo
force-pushed
the
worktree-confirm-cost-control-scoped
branch
from
August 5, 2026 15:48
dd63131 to
2149ed8
Compare
guarzo
changed the title
guarzo
enabled auto-merge (squash)
August 5, 2026 15:49
guarzo
added a commit
that referenced
this pull request
Aug 6, 2026
…ilent failures (#129) * fix(payouts): make the create screen say less and collect more, and give finalize somewhere to land Seven changes to the payouts surface, all UI-side. The /payouts/new lede ran four lines to say what one says: creating an operation pays nobody. The optional battle report input comes back to that screen, validated http(s)-only before any appraisal call, because the value renders as a plain <a href> on the operation's own page. Notes stop hiding behind an edit toggle. They are a standing field on the operation, so the textarea is always open with a Save beside it, controlled because the form never unmounts and an uncontrolled one would snap back the instant the first save settled. Finalize and Unlock stop revealing their cost sentence on arm: revealing inside the .btn-row shoves the neighbouring control out from under the mouse, the same reflow #112 found in a table cell. The sentence stays in the DOM as the aria-describedby target, permanently visually-hidden, via a new alwaysHidden prop rather than a second hand-rolled span. Both controls also delete themselves on success, which left focus on <body> and nothing announced. Focus now goes to the operation heading and the outcome is announced, both from the action's own continuation and a live region on a wrapper that outlives the button, because an effect inside the button never commits at all. * fix(payouts): keep the finalize announcement alive for an operator who cannot unlock Any operator can finalize any draft, but only the creator or an admin can unlock it. For an operator who is neither, a successful finalize turns every disjunct of showLifecycle false at once and the whole lifecycle block goes away — taking the announcer with it. That operator, uniquely, heard nothing: the live region was unmounted by the very response it existed to describe. The announcer now sits outside the gate, so it outlives the block it describes. Covered by an e2e that hands the draft to a second operator and asserts the announcement lands while every lifecycle control is gone; verified by reintroducing the bug and watching it fail. Also corrects the comments this diff got wrong: the claim that the admin table would leak every row's cost (it matches per-control, as the paragraph above it says), the reflow rationale for alwaysHidden on Finalize/Unlock (those two can never render together, and the row grows rightward from a fixed left edge — the real reason is that prose appearing mid-press reads as an error), the "a useEffect never commits at all" wording, the claim that setNotes cannot reject (assertEditable throws PayoutLockedError on a stale page), and three enumerations that predate the battle report field. NotesForm's `value` prop is renamed `initialValue` to match what it does: it seeds state once and never re-syncs, which is what lets a half-typed note survive an unrelated revalidation. * fix(payouts): tell an operator their notes hit a freeze, instead of apologizing The notes textarea is the one editable field that sits open for as long as the operation is editable, so it is the one an operator can be mid-paragraph in when the operation freezes underneath them: a second tab, or another operator finalizing first. canEdit narrows that window and cannot close it. Uncaught, assertEditable's throw landed on error.tsx — "tell an admin and quote this reference" — for a race that was nobody's fault and no fault of ours. setNotesAction now catches PayoutLockedError and redirects as ?error=locked. This is a deliberate exception to the rule stated in this file, that input rejections redirect and lifecycle errors belong on error.tsx. That rule holds because no lifecycle error there has anything the operator typed at stake. This one does. The code is worded generically rather than about notes, so the other always-open editable fields can adopt it as they hit the same race. The text is lost either way — that is what the freeze means. What changes is that the operator learns the operation was finalized rather than being told we broke. Covered by an e2e that freezes the row out from under an open textarea; verified by disabling the catch and watching it fall back to the boundary.
guarzo
added a commit
that referenced
this pull request
Aug 6, 2026
* fix(a11y): give each Status tone its own shape, not just its own colour `.st::before` set one filled dot for every tone; `.st--off::before` was the only variant. For ok / warn / bad / neutral, colour was the only non-textual signal, which WCAG 1.4.1 does not allow. A member with red/green colour deficiency scanning a payouts list at 1am sees an identical dot beside every operation and has to read each row's word to tell a paid operation from an unpaid one; at payouts/page.tsx:167-190 three branches render wording that differs only by tone, so there the distinction was unrecoverable. Neutral is now a thin bar, ok a circle, warn a triangle, bad a square, off the hollow circle it already was. Width stays 0.5em in every rule — only height and radius change — so the mono advance the comment at globals.css:1680-1687 depends on never moves. The docblock at ui.tsx:223-225 already claimed the glyph carried the meaning. This makes that true; it was a design intent the CSS never implemented. * fix(a11y): make ConfirmSubmit's width reservation actually hold The reservation was `Math.max(label.length, confirmLabel.length) + 4` in `ch`. That cannot hold: `ch` is the advance of the "0" glyph alone, while the label carries `letter-spacing` the browser inserts between every character pair, so the shortfall scales with label length and a flat +4 cannot track it. It overshot short labels and undershot long ones — /admin/accounts cleared by ~2px by accident, "Replace roster" fell ~23px short. An operator who clicks the right-hand sixth of "Replace roster" arms it, the button shrinks out from under their stationary pointer, pointerLeave fires, the control disarms, and a destructive action reads as dead — so they press it again. That is the trap #112 fixed elsewhere, reintroduced by arithmetic. The wider label is now rendered as CSS generated content in flow but `visibility: hidden`, sharing grid cell 1/1 with the real text, so the browser measures it with the button's own font, weight, case transform and letter-spacing. `content: attr()` is not a DOM node, so it cannot reach `textContent` — several call sites assert exact button text with `toHaveText`, which does read hidden real elements and would have picked up a duplicate. The visible text carries its own `grid-area: 1 / 1`. As an anonymous grid item it was auto-placed into row 2, which reserved the width correctly and doubled the button's height; measured in a browser, not inferred. * fix(a11y): give sign out the same hit target as the links beside it A member ending their session on a phone at 1am aims at the smallest control in the header — the only one in the bar that is not a link, on all ten pages that render a header. `.shell__signout`'s button is `.btn--quiet .btn--micro`, which carries `min-height: 1.75rem`. DESIGN.md rations that 28px grade to `.btn--micro` in admin table rows and nowhere else; every link beside it is already `min-height: 2.25rem` with `padding: var(--s-2) var(--s-3)`, raised there by an earlier sweep that pinned the geometry in e2e/shell.spec.ts and did not carry the same fix across to the button. Scoped override on `.shell__signout .btn` rather than dropping `.btn--micro` from the markup, following `.inline-edit--standalone .btn--quiet`: the micro grade also carries `font-size: var(--t-label)`, which is what makes sign out read as one of the nav's own labels, so removing the class to fix the box would have changed the type. The new spec asserts both halves — 36px tall, and the same computed font size as the links — and fails at 28px with the CSS reverted. * refactor(ui): let ConfirmCost own its own register `.confirm-cost` had exactly one rule in globals.css and it was layout (`.facts__lead > .confirm-cost`, flex-basis). Everything about how the cost sentence *looks* came from `className="dim"` passed by hand — by all four call sites, identically, with no other value ever passed. That made `className` a required argument dressed as an optional one: the component decided when the sentence becomes visible but not what it looked like once it got there. A fifth call site that forgot the prop would have rendered the cost at full body size and full ink — reading as primary copy at the exact moment the reveal puts it in front of a member deciding whether to unlink their Discord or delete an operation. `.confirm-cost` now carries `.dim`'s two declarations itself and the passthrough is gone. No rendered text changes; the class list on each span loses only `dim`. * docs(ui): say which mechanism keeps the ghost label out of toHaveText Review misread the ConfirmSubmit docblock as claiming `visibility: hidden` is what stops Playwright picking up a duplicate. It does not, and the comment did not quite say so — "would otherwise pick up a duplicate" had no explicit antecedent, so the sentence read as being about the hidden box rather than about the alternative that was rejected. The real property is that `content: attr()` is never a DOM node, at any visibility. What that rules out is a second real `<span>`, which `toHaveText` would read (e2e/admin.spec.ts:569-571) and concatenate into the button text several call sites assert exactly. Comment only. * docs(css): correct why sign out uses a scoped override The comment on `.shell__signout .btn` said dropping `.btn--micro` from the markup would have changed the type. It would not: `.btn` sets `font-size: var(--t-label)`, the identical value `.btn--micro` sets, so the type register is the same either way. The override is still the right shape, for a reason the comment did not give: sign out is `.btn--quiet .btn--micro`, and `.btn--quiet` sets its own `min-height: 1.75rem`. Removing the micro class would have left the box exactly where it was — which is also why `.inline-edit--standalone .btn--quiet` is written as an override rather than a class removal, while `account/page.tsx:372-381` could just drop `--micro` and be done. Comment-only. The rule and the spec pinning it are unchanged.
guarzo
added a commit
that referenced
this pull request
Aug 7, 2026
…169) * perf(account): tighten the manifest row from 75px to 63px The row's height floor was never the STATUS cell — it is the NAME cell's two lines plus cell padding. Round 1 collapsed a cell that had 30px of slack in a 50px block, so the pitch did not move. Replaces #167's height-band assertion with a pitch ceiling. The band required a located row to be 10-30px taller than a no-location row, which is two lines by definition: it passed at 75px and would pass at 95px. * feat(account): shorten the manifest's make-main control to 'main' 89px to 50px per character column, with the verb moved into a per-character accessible name — the same trade the unlink beside it already makes. * feat(account): render the manifest STATUS column only on exception On the common account every row reads ok, costing a header, a column and 82px of a 320px viewport to report the absence of news. The column now renders when at least one character is not ok, so its presence is the signal. `map on|off` varies per character while the chip reads ok either way, so the cell's accessible name was its only home; it moves to a visually-hidden span in the NAME cell at zero vertical cost. `aria-describedby` cannot dangle: a contact remedy implies attention or stalled, never ok, so a remedy existing implies the column renders. * feat(account): fold STANDING into the page head 539px of chrome sat above the first manifest row at 1440x900 — 60% of the fold — and 171px of it was a rule-head, a two-row definition grid and a collapsed margin holding two facts. The verdict joins the h1's line at zero vertical cost; tier and Discord become one meta line under the lede. The <dt>s were the only thing naming those values, so each fact keeps a visually-hidden label. The confirm cost keeps its own line via .page__meta-item > .confirm-cost: without it the revealed sentence re-centres the button out from under a stationary pointer and the pointerLeave disarms the control (#112), pinned by a boundingBox test at 700px. * test(account): gate the fold count at three viewports The success criterion is characters above the fold, so assert that directly — at 1440x900, 1280x800 and 390x844, not only the desktop case. Re-baselines the 320px horizontal gate onto a ten-character seed. Its previous 250px threshold came from a one-character account; a realistic account measured 258px, so the number the gate protected had never been taken at the size that matters. * fix(account): post-review cleanup — dead CSS, a false caption, three stale comments Whole-branch review of the manifest-density work (a4a292b..7c5654f) came back SHIP with only Minors: Task 4 duplicated .facts__lead/.facts__lead > .confirm-cost as .page__meta-item variants instead of renaming them, leaving the originals with no consumer; the all-ok manifest caption asserted every character is healthy even at zero characters, where showStatusColumn is vacuously false; and three comments described a `.facts` grid or a "block below" layout that Task 4's fold into the page head replaced. - Remove the dead .facts__lead / .facts__lead > .confirm-cost rules and point every comment that cited them (confirm-submit.tsx, globals.css's discord-id comment) at the live .page__meta-item selectors instead. - Give the zero-character case its own caption sentence instead of routing it through the all-healthy branch. - Correct standing.tsx's `.facts` grid reference, globals.css's "block below" verdict comment, and the .verdict margin-top comment (dead in practice — every call site is inside .page__head-row, which zeroes it) to match the post-fold layout. - Document that .page__meta-item > .confirm-cost's `>` combinator depends on ConfirmArmScope staying a pass-through with no wrapper element (the #112 fix goes dark silently otherwise). * test(account): pin the manifest caption's zero-character branch The no-STATUS-column sentence is selected by `showStatusColumn`, a `.some()` over the crew — false for an empty account as readily as for a healthy one. The zero-character branch added in post-review cleanup was the only user-visible copy on this branch with no test behind it; removing it turns this test red.
guarzo
added a commit
that referenced
this pull request
Aug 8, 2026
) A page-by-page walkthrough of every rendered surface with the project owner, conducted against production screenshots. `docs/design-walkthrough.md` carries the findings as seven self-contained sessions so each is startable from a cold context; this commit is session 0, the cross-cutting part the other six depend on. Three rulings, recorded in DESIGN.md so a later sweep does not undo them: R1, hit targets. The 28px in-row grade is now scoped by the density reason rather than by the tag it lands in, so a `Disclosure as="row"` drawer takes the 36px standalone grade despite rendering a literal `<tr>`. This resolves a contradiction rather than reversing a decision: e2e/sync.spec.ts:1095 has been pinning the /admin/sync drawer to the standalone grade all along, and payouts/[id]/notes-form.tsx:90-95 reasons the same way for a panel field. Only /admin/accounts read the settled-decisions row as forbidding it. That row is marked AMENDED rather than deleted, per that file's own contract. R2, rare destructive controls do not hold permanent width in a scanning table. Records the existing reveal-on-arm constraint (#112) as a reason to move such controls rather than to progressively disclose them where they sit. R4, information may not live only in the assistive-tech channel. Two instances found, both the inverse of the usual defect: a working role="status" save confirmation marked .visually-hidden, and an affordance named in aria-label and absent from the visible summary. Copy: removes the four em dashes from rendered strings (the glyph stays as this app's null-value marker, per error.tsx:239), and drops the /payouts lede's first sentence, which restated its own table columns. The flat-pool note label changed shape, so its 12 e2e assertions move with it. The equivalent /account lede is deliberately left to session 3. e2e/account.spec.ts:1583 uses it as the narrow-capped sibling proving the manifest opts out of the cap, and finding 3.1 changes what that anchor should be; deleting it here would mean editing that test twice. Gates: typecheck, lint, format:check, 1290 unit tests across 83 files, 297 e2e, production build, and check-node-version all pass.
This was referenced Aug 8, 2026
guarzo
added a commit
that referenced
this pull request
Aug 9, 2026
…t status line Follow-up to #193, from the same critique. - admin/accounts: the token badge said "0/8 ok" in the exact case the badge turns red. A red marker and the word OK argued with each other in the one cell built to be glanced at; "healthy" is a word the numerator can be zero of, where "ok" is a verdict and reads as one no matter what precedes it. - admin/accounts: REVOKE drops to .btn--danger-quiet at rest and keeps full .btn--danger on arm, matching FREEZE and UNLINK. globals.css already records why: full --danger on a per-row control "made it the most saturated thing on the account page, permanently, which reads as a warning against an ordinary choice". Every row of an admin table can be an admin, and resting red as furniture spends the alarm channel the token badge two cells left needs when an account actually goes dark. - account: the per-row status summary renders only where a character deviates from nominal. For a managed, contacts-ok, on-ACL character the sentence can only be "token ok, standings ok, map on" — the same string on every row, under a heading that has already said all of them are healthy. The R4 parity fix it was added for is kept exactly where it was earned: a row whose map or standings state differs still says so, and the gate is on the element, so both channels drop it together. The <caption> states the rule so an unannotated row is readable as nominal rather than uncomputed. Two new e2e cases cover the gate, which no existing test reached: seedNominalCrew never seeds wandererAclObservation, so all ten of its characters are `map off` and none is nominal. The existing counts are unchanged for that reason, and their regexes are tightened from `map (on|off)` to `map off` to pin the precondition they now depend on. Not done, and why: putting the object in the Discord unlink's visible label ("unlink Discord") was tried and reverted. It widens the button ~64px, which at ~700px pushes the arming live region off the line; the line box grows on arm, align-items: center re-centres the button out from under a stationary pointer, and the pointerLeave disarms it. That is the #112 mechanism, caught by "arming the Discord unlink does not move it out from under the pointer". The twin-UNLINK finding was also weaker than it read: the two controls already differ in grade, size and accessible name, and only share the visible word. Reasoning left in a comment at the call site. Verification: typecheck, lint, format:check clean; 83 files / 1324 unit tests pass; e2e/account.spec.ts 71 passed.
guarzo
added a commit
that referenced
this pull request
Aug 9, 2026
…t status line (#194) Follow-up to #193, from the same critique. - admin/accounts: the token badge said "0/8 ok" in the exact case the badge turns red. A red marker and the word OK argued with each other in the one cell built to be glanced at; "healthy" is a word the numerator can be zero of, where "ok" is a verdict and reads as one no matter what precedes it. - admin/accounts: REVOKE drops to .btn--danger-quiet at rest and keeps full .btn--danger on arm, matching FREEZE and UNLINK. globals.css already records why: full --danger on a per-row control "made it the most saturated thing on the account page, permanently, which reads as a warning against an ordinary choice". Every row of an admin table can be an admin, and resting red as furniture spends the alarm channel the token badge two cells left needs when an account actually goes dark. - account: the per-row status summary renders only where a character deviates from nominal. For a managed, contacts-ok, on-ACL character the sentence can only be "token ok, standings ok, map on" — the same string on every row, under a heading that has already said all of them are healthy. The R4 parity fix it was added for is kept exactly where it was earned: a row whose map or standings state differs still says so, and the gate is on the element, so both channels drop it together. The <caption> states the rule so an unannotated row is readable as nominal rather than uncomputed. Two new e2e cases cover the gate, which no existing test reached: seedNominalCrew never seeds wandererAclObservation, so all ten of its characters are `map off` and none is nominal. The existing counts are unchanged for that reason, and their regexes are tightened from `map (on|off)` to `map off` to pin the precondition they now depend on. Not done, and why: putting the object in the Discord unlink's visible label ("unlink Discord") was tried and reverted. It widens the button ~64px, which at ~700px pushes the arming live region off the line; the line box grows on arm, align-items: center re-centres the button out from under a stationary pointer, and the pointerLeave disarms it. That is the #112 mechanism, caught by "arming the Discord unlink does not move it out from under the pointer". The twin-UNLINK finding was also weaker than it read: the two controls already differ in grade, size and accessible name, and only share the visible word. Reasoning left in a comment at the call site. Verification: typecheck, lint, format:check clean; 83 files / 1324 unit tests pass; e2e/account.spec.ts 71 passed.
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
1 participant
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.
Scope reduced. This started as "control-scoped
ConfirmCost+ the admin consequence sentence". #111 landed the admin sentence first, and investigating the overlap showed the reveal half is unsafe. What's left is one file.What lands
ConfirmCostreadctx.armedId !== null— "something in this scope is armed". Correct in a scope holding one control, silently wrong in every other one. #111 hit exactly this: the accounts table wraps every row in oneConfirmArmScope, so it shipped its sentence hidden always and asked for a per-control id.ArmContextnow carries the armed control'sdescribedBybeside itsuseId, in one state object so a render can never see the new id with the previous description.What this does NOT do
No behaviour change, and no test. Both call sites are scopes of one, so nothing observable differs today. The difference requires a click, and the repo has no jsdom or testing-library —
renderToStaticMarkup(the pattern intests/account-page.test.ts) cannot arm a control. Adding a test framework for a behaviour-neutral change wasn't worth it. This is the footgun removal #111 asked for, not a fix for a bug anyone can currently reach. Reviewers should weigh it on that basis, including "not worth landing".What was reverted, and why it's worth knowing
I tried to use this in the admin Discord cell so sighted admins would see the sentence too. It does not work, and #111's comment predicted the reason:
Confirmed by measurement. Revealing inside a
tdwidens the cell; the widening moves the armed button off the stationary mouse;pointerLeavefires; the control disarms. The reveal undoes the arm. MeasuredarmedCount=0, control snapped back to its rest box.The first version of that attempt appeared to pass — because of a bug in my own CSS.
flex-basis: 100%resolved against.inline-pair, which isinline-flexand shrink-to-fit, so the note was squeezed to 77px wide and 161px tall instead of the 34ch I'd documented;max-width: 34chnever bound. My tests asserted only "width > 1" and geometric containment, so neither noticed. Correcting the width to the intended value is what exposed the disarm.That finding is now in the
ConfirmCostdoc comment. It's the expensive half to rediscover, and the next person reaching for a reveal in a dense layout should get it for free.The member page can reveal because its Discord row is a
ddin a.factsgrid that already reserves a wrapping line, so nothing moves.Verification
npm test→ 75 files, 983 tests passed (117s)npm run test:e2e -- e2e/admin.spec.ts e2e/account.spec.ts→ 61 passed (1.0m)npm run typecheckclean;npm run format:checkclean;npm run lint→ 1 pre-existing<img>warning, untouched by this diff