a11y(discord): one control per state, not a status token beside a button - #115
Merged
Conversation
The Discord row rendered `linked` next to `unlink` when linked, but renders a bare `Link Discord` when not — no "not linked" token beside it. The row already trusted a verb to answer "is Discord linked?" in one state and not the other, and that asymmetry is what made the linked state read as cluttered. #108 widened the gap between the two halves; this removes the second half. - account/page.tsx and admin/accounts/page.tsx drop the `linked` Status. The admin else-branch keeps `none`, because an unlinked row has no control and the cell would otherwise be empty. - `.inline-pair` had exactly these two call sites, so the rule goes with them rather than sitting orphaned at the --s-3 #108 gave it. Its whole history was trying to hold apart two things that read as one object, which was the clue there should only have been one. Precedent: the sync-schedule section already drops its Discord row rather than render an inert "not linked" token, on the grounds that a nearby element states the fact with the action attached (e2e/account.spec.ts:289-292). Not done: making the `linked` token itself the button. A control labelled with a state does not say what pressing it does, and an admin column of identical `linked` buttons arming destructive actions against other people's accounts is a misclick surface the confirm step only catches after the fact. No test asserted on the removed token.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Discord account views no longer display a separate ChangesDiscord unlink UI
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
guarzo
added a commit
that referenced
this pull request
Aug 9, 2026
…188) * feat(admin): owner walkthrough session 4 — findings 4.1, 4.2, 4.3 Findings 4.1-4.3 of docs/design-walkthrough.md, on /admin/accounts. 4.4 (the nested crew scroller) is staged separately, as the session requires. 4.1 Ruling R1: the drawer takes the 36px standalone grade. The six drawer controls and NoteForm's Submit drop `.btn--micro` for plain `.btn` — the mechanism /admin/sync's drawer already uses and that e2e/sync.spec.ts already pins. The four controls in the collapsed row (revoke, grant, sync now, and the toggle) stay at 28px: those are the dense control-set rows the grade exists for. The 22-line defence in note-form.tsx is deleted rather than re-numbered — it named the call as one for a human, and the human has made it — and its replacement cites DESIGN.md and the sync test by name, per the session's stale-citation note. Measured, since 4.1 asks for it before committing to 36px. At 1280px `.note-form .field` renders 39.7px against `save note`'s 36px, so the gap closes from 11.7px to 3.7px rather than to zero: the session predicted a ~4px step would remain if the field really was 40px, and it does. At 320px the drawer is 65px taller (527.1 → 592.1) and its controls wrap to four flex lines instead of three — but `.drawer__ controls` goes 264.5px → 252px, so it now fits the 252px panel it used to overhang by 12.5px, and nothing new overflows the 286px region. At 1280px the drawer is 12px shorter (277.4 → 265.3). The narrow viewport pays height and gets its horizontal overhang back. 4.2 Ruling R2: unlink moves off the collapsed row into the drawer. It was the one rare, destructive control on this page still holding permanent width, and stacking the handle over the button set the row height for the whole table. The collapsed cell keeps the @handle — that argument was always about the handle, not the button: it is what tells an admin the link points where they think it does before they ever open the row. A row with no link mounts no Discord group at all, matching Session 3's judgement for /account's empty case. This forced the action's shape to change, which is why actions.ts, view.ts and confirm-group.tsx move: a drawer action cannot redirect, because that replaces the route tree and closes the drawer the admin opened to press the button. `unlinkDiscordAction` therefore returns an `ActionOutcome` through `useActionState` like its four drawer siblings, and the "four cell-level / four drawer" counts documented across those files — and twice more in page.tsx — become three and five. 4.3 The tier-lock note drops from ~40 words to 18. At `.drawer__note`'s 34ch measure it was rendering many lines deep in a control column. It loses the explicit `tierLabel("alumni")`: at this length naming the destination tier costs more than it explains, and "even if they leave the alliance" carries the case that mattered. The id and aria-describedby wiring are unchanged, and the note stays below its group — that position is load-bearing for column alignment. e2e re-anchored, not deleted: the unlink flow opens the drawer first, the Discord-column test keeps its #115 intent (a linked row says more than a bare "unlink") against what the cell now holds, and the drawer alignment test's comment no longer claims both controls are `.btn--micro`. The pin-overlap threshold moves 0.8 → 0.7 because wider drawer controls widen the first tier button; measured at 0.7669 after, and the test proves "an x-only measure gets this wrong", which needs a large majority rather than a specific fraction. * docs(accounts): correct the drawer-action counts R2 changed, and the ActionOutcome doc Moving `unlinkDiscordAction` into the drawer (ruling R2, previous commit) changed the split between cell-level and drawer-scoped actions on this page from four/four to three/five, but three comments still stated the old numbers. `view.ts:99` and `:105` are fixed here. `ActionOutcome`'s docblock also claimed `/account`'s separate `unlinkDiscordAction` follows the same "don't confirm a no-op" rule *through this type*. It doesn't — that control isn't in a drawer, so it stays a `redirect()`-shaped `Promise<void>` and expresses the no-op by redirecting to a bare `/account` with no `?done=` code (`account/actions.ts:102`). Same rule, different mechanism; the doc now says which. The third stale count, `page.tsx:342`, rides in the following commit rather than this one: that file also carries finding 4.4's per-cell labels, and splitting one comment word out of it was not worth the patch surgery. * fix(accounts): finding 4.4 — reflow the drawer's crew table instead of nesting a scrollbar The crew table inside each account's drawer had its own horizontal Scroller, which at narrow widths put a scrollbar inside the page's own scrolling region. A scrollbar inside a scrollbar has no good visual state, and the walkthrough files this as structural rather than proposing a fix. Two axes had to be closed, not one. Viewport: below 30rem the table reflows into one labelled block per character. Measured on this repo's `seedDenseWorld` fixture rather than the walkthrough's quoted figures — it puts the boundary at 440px (370.45px min-content), but the real min-content here is 395.77px and 440px still overflows by 26px; the true boundary is 465px, and 30rem takes a real margin above it. Content: `.char` sets `white-space: nowrap`, so a character name is one unbreakable word and the table's min-content tracks the longest name in the row set. EVE allows 37 characters, and at that length the table measures 616px — still overflowing by 206px at 480px, 86px at 600px and 2px at 700px, the whole band the breakpoint declares clear. Covering that by breakpoint alone would mean reflowing at ~44rem, where three of four columns fit side by side comfortably, so `.log--crew .char` unbinds the name instead. Scoped to this table: the account manifest shares `.char` and wants its name on one line. Both declarations are load-bearing — `overflow-wrap` alone is inert under `nowrap`, proven by removing `white-space` and watching all three widths fail. `display: grid`/`block` on a `<tr>`/`<td>` strips table semantics in every browser tested, so the hidden `<thead>`'s `<th scope="col">` associations go with it. `content: attr(data-label)` is not a fix — generated content is inconsistently exposed to assistive tech — so a real `.crew__label` element sits in each cell at every width and only its `display` trades places at the boundary, keeping both channels populated per ruling R4. Two specs are deleted rather than updated: both asserted the crew Scroller overflows and holds a tab stop at 320px, which is precisely the state this removes. The second was a WCAG 2.2 2.4.11 guard, and a WCAG guard is only safe to delete when the scenario cannot occur — not when the fixture happens not to reach it. The long-name spec added here is what makes that true, pinning tabIndex -1 and no scroll range at the three widths where an unbounded name still overflowed. A region that never takes a tab stop can never be the target of a sequential-focus scroll. Also carries the third stale drawer-action count from the previous commit (`page.tsx:342`, four -> five). * docs(design): record that /admin/accounts has adopted ruling R1 The Hit targets section named `/admin/accounts` as "the surface that diverged" in the present tense. Finding 4.1 fixed it, so that sentence now describes a state that no longer exists. Cites the pinning test by name rather than by line, per the walkthrough's own note about stale citations. * docs(css): retire the crew-overflow claims finding 4.4 made unreachable Two comment blocks justified themselves on a scenario this branch removed. `.log--sticky-head`'s scroll-margin rule explained `.scroller`'s presence in its selector list entirely on the crew Scroller's behalf — a tab stop parking flush under the sticky header at 320-390px. That stop no longer exists at any width, so the comment described a case a reader could not reproduce. The entry stays: it is a floor for any Scroller nested in a sticky-headed table, not a patch for the one that needed it, and it re-earns itself if another region overflows. The comment now says that instead. The `.drawer__crew` / `.scroller-frame` `min-width: 0` floors turned out to be weaker than I first wrote. I asserted they were still load-bearing, then measured: setting both back to `min-width: auto` and re-running the six 320px specs in `e2e/admin.spec.ts` passes all six, because 4.4 removed the wide descendant they were zeroing. Kept anyway, as a guard rather than as dead code, with the measurement recorded so the next person can delete them deliberately rather than discover it. Comment-only — no declaration changed. * docs(accounts): record the second unbounded-text axis in the crew table The WCAG 2.2 2.4.11 deletion rests on the crew scroller never taking a tab stop, so every source of unbounded width in that table has to be accounted for, not just the one the fix bounded. The name was one; `.char__location` is the other, and structure names are player-supplied and run past 150 characters. It is already safe: `max-width: 22rem` with `overflow: hidden` caps the used width, and that caps the intrinsic contribution with it. Injecting a 200-character location into an open drawer clips the line at its 352px ceiling and leaves the scroller's `scrollWidth` equal to `clientWidth` at 480, 600, 700, 768 and 1000px, tab stop withdrawn at all five. Recorded rather than pinned: the cap is `.char__location`'s own contract, and a spec here would fail for whoever legitimately changes it. Comment-only. * fix(accounts): bound the quoted contact label, the second text that reopened the crew scroller Finding 4.4 bounded the crew Name column and claimed on that basis that the crew Scroller can never take a tab stop, which is what retires the deleted WCAG 2.2 2.4.11 spec. The claim was wrong: the Standings cell renders ContactRemedy, which quotes the label back inside `code.literal`, and that is `white-space: pre` — wrapping suppressed exactly as the name cell was. Both sources are unbounded. STANDINGS_LABEL is `z.string().min(1)` with no ceiling and is operator-set; the candidate labels are raw player-set EVE contact labels. Measured before the fix on `.drawer__crew .scroller`, scrollWidth/clientWidth: 976/250 at 320px, 976/320 at 390px, 976/409 at 479px, 1205/410 at 480px, 1205/530 at 600px. This axis overflowed at every width, not only in the reflow band, so it was a wider hole than the name. `pre-wrap`, not `normal`: `pre` was set so a member comparing "AuthGD " with "AuthGD" can see the trailing space, and `normal` would collapse it — closing this hole by silently undoing that fix. `pre-wrap` keeps every space and only adds the ability to break. Each declaration measured alone at 320px: pre-wrap without overflow-wrap reports 976/250, overflow-wrap under `pre` reports 976/250. Both load-bearing. Pinned with a seeded label_mismatch row rather than recorded, because this is the premise the WCAG deletion rests on. The spec fails without the rule with tabindex="0" at all three widths. Also corrects the two comments that asserted the name was the only unbounded text in this table. Found by review, not by me — the branch shipped this as a silent gap. * docs(css): correct the whitespace example both code.literal comments gave 4c2fb40 justified choosing `pre-wrap` over `normal` by saying a member comparing "AuthGD " against "AuthGD" would lose the trailing space under `normal`. That is wrong, and it was wrong in the rule it borrowed it from — `code.literal`'s own comment has carried the same example since it was written. `ContactRemedy` renders the label inside quotes, so a single trailing space is interior, between the `D` and the closing `"`, and HTML whitespace collapsing does not touch it. Measured at 320px, painted width of one string minus the other: "AuthGD " vs "AuthGD" pre-wrap 7px normal 7px survives both "Auth GD" vs "Auth GD" pre-wrap 7px normal 0px collapsed "AuthGD " vs "AuthGD " pre-wrap 7px normal 0px collapsed So the rule IS load-bearing and `pre-wrap` IS the right value — but for repeated runs, not for a single trailing space. contact-label.ts:112 calls "Auth GD" against "Auth GD" a "spacing" difference and renders both to the member to compare; under `normal` they are painted identically, which is the bug the rule exists to prevent. The decision stands, its stated reason didn't. No declaration changes. Found by measuring a claim I had asserted rather than verified — the same failure this branch has now hit four times. * docs(css): record that the crew-cell bounds are per-selector, not general Both bounds added on this branch — `.log--crew .char` and `.log--crew code.literal` — name the two unbounded strings the crew table renders today. Neither is a general rule, so a new column or a new remedy branch with its own operator- or player-set string falls outside both and reopens the overflow with nothing to fail: the scroller quietly takes a tab stop again. Note that where someone adding a crew cell will read it. Comment only; no rule changes. * fix(accounts): four review findings, and the CI failure the merge exposed The 320px drawer test failed on the merge with main, not on either side alone. The region is height-capped against the chrome above it, and at 320px this drawer is 786.6px against a 576px region — globals.css blesses exactly that, so the fix is the test's positioning, not the drawer. It centred the drawer once and measured every control off that one scroll, which left `set tier` 12px above the region's top edge: a vertical clip failing a test whose every claim is horizontal. Each control now scrolls itself into view immediately before measuring. The horizontal guard stays non-vacuous — `coveredByPin` sets scrollLeft to the offset under test on the next line. Review findings: 1. The Solo Pilot drawer's "no Discord section" assertion is `toHaveCount(0)`, equally true of a drawer that never opened. Anchored on the note field first. 2. `narrowSnapshot` covered `.drawer__crew`, which also renders a sibling `Map observed <timestamp>` line. `toContain("Map")` passed off that line alone — the one column whose assertion survived deleting its label span. Scoped to `.log--crew`. 3. The unlink confirmation never painted. `revalidatePath` flips `discordLinked` false in the same response that carries the action's return value, so React commits both together and the whole Discord section went down with the press. Hoisting `ConfirmGroup` out of the conditional was not enough — measured, not reasoned: the `ConfirmingForm` whose effect reports the text upward unmounted too, and the sentence still never appeared. The form is now the group, and the gate sits inside it holding only what should disappear. Both live in a `.drawer__confirm` wrapper (`display: contents`), so the form remains a direct flex item of `.drawer__controls` and wraps at the widths already measured there; two `position: absolute` rules keep the emptied form and the empty notice host from each costing a never-linked row's drawer a 2rem flex gap. The e2e now asserts the sentence is visible AND that focus is on the host carrying it — a host that unmounted could not be focused, which is the half the button's disappearance says nothing about. Comments in actions.ts and view.ts that documented the unmount without acting on it now record the constraint. 4. `.crew__label` is `.log th` under another name: below 29.9375rem the crew table reflows to stacked blocks, the real thead goes `display: none`, and these spans are the only thing naming the columns. Added to the shared label register, so it picks up mono, size, uppercase and weight 600. It stays hidden above the breakpoint — the register sets no `display`. Gates: typecheck, lint, format:check, build, check-node-version all clean; npm test 1324 passed (83 files); npm run test:e2e 345 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.
First of two. This one is a deletion and needs no migration; the follow-up adds the Discord handle. Split so this can be looked at on its own before anyone plumbs a username through four layers.
Why
The unlinked state was already a single control — a bare
Link Discordon the account page,nonein the admin table. Neither renders a "not linked" token beside it. The linked state was the only one rendering a status token and a button, and that asymmetry is what made it read as cluttered. #108 widened the gap between the two halves; this removes the second half.The verb carries the state:
unlinkpresent means linked.What changed
src/app/account/page.tsx,src/app/admin/accounts/page.tsx— drop<Status>linked</Status>. The admin else-branch keepsnone: an unlinked row has no control, so something has to occupy the cell.src/app/globals.css—.inline-pairhad exactly these two call sites, so the rule goes with them rather than sitting orphaned at the--s-3a11y(account): separate the unlink control from the LINKED it sits beside #108 gave it. Its whole history was trying to hold apart two things that read as one object, which was the clue there should only have been one.Precedent: the sync-schedule section already makes this call — it drops its Discord row rather than render an inert "not linked" token, because a nearby element states the fact with the action attached (
e2e/account.spec.ts:289-292).Not done
Making the
linkedtoken itself the button (click it, confirm to unlink). A control labelled with a state doesn't say what pressing it does, and an admin column of identicallinkedbuttons arming destructive actions against other people's accounts is a misclick surface — the arm-then-confirm step catches it only after the user has learned the hard way.Rendered
Checked against a real render, not just assertions. Admin column now reads
UNLINK/NONE, one object per row. Account row readsDISCORD [UNLINK].One thing this surfaces, deliberately left alone:
unlinkis a full-grade.btn--microand is now the loudest thing in the Discord column, which reads a little like a second Actions column. It's still strictly quieter than the token-plus-button it replaces, and the follow-up PR adds@handletext that will make the button the detail rather than the whole cell — so I'd rather see it with the handle in place before deciding whether it wants.btn--quiethere.Verification
npm run format:check— cleannpm run typecheck— cleannpm run lint— 0 errors, 1 pre-existing warning (ui.tsx:109,<img>, untouched)npm test— 75 files, 983 passednpm run test:e2e -- account.spec.ts admin.spec.ts— 61 passedNo test asserted on the removed token, so nothing needed updating — which is itself worth a reviewer's eye.
Summary by CodeRabbit