fix(a11y): give the audit filter links and the payments toggle their hit target - #141
Conversation
…hit target `.json > summary` was measured at 18.6px and fixed with a 28px floor when it was the only in-cell disclosure anybody had put a ruler against. Two siblings with the identical shape were left behind: `.cell-link` — every actor, action and target in the audit log is a filter control and the whole content of its `<td>`, so WCAG 2.5.8's inline-target exception does not cover it. An admin tapping a name to filter by it aims at a ~19px box in a table whose rows sit edge to edge with no gap, so a mistap does not land on nothing: it lands on the neighbouring row and silently applies the wrong filter, on the one page whose whole job is identifying precisely who did what. `.log summary` — `PaymentHistory`'s "payments (N)" toggle is styled by this rule, not by `.json`, and inherited the same bare line box. Fixed with padding rather than `align-items: center`, because the wrapped-long-name case the rule already documents still has to hold. DESIGN.md:227-232 commits to 28px for in-row controls and both of these are that. Found by a design-sweep re-review; no test pinned either, both are CSS-only.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
…correctly (#143) The Corp share row read `12.5% (4,318,206.71 ISK + remainder)`, which reads as "this much, and then some rounding on top". It is the opposite: corpAmount is totalValue minus every participant's amount (services/payout-view.ts:215-218), so the remainder is already inside the figure — e2e/payouts.spec.ts:609-621 asserts exactly that, to the cent. The reader is an FC checking a split before finalizing, and this row is the only place the corp's cut appears as ISK; the old string invited them to add a fudge factor to an exact number, so a split that reconciles looks like it does not. Now ", remainder included". The word `remainder` is kept because payouts.spec.ts:619 locates the cell by it. The duplicate-name and roster-clash notices ended with "remove one before finalizing" and "Check before finalizing" outside any editability gate, so a finalized or payment-frozen operation instructed an action that assertEditable rejects, with the control already removed from the page. Someone auditing a completed payout after a dispute either hunts for a button that was deliberately taken away or concludes the operation is still a draft. The notices themselves are worth keeping after finalizing — they explain why one pilot appears twice in a split that already paid out — so only the imperative changes, on a new `amendable` rather than on `canEdit`: `canEdit` folds in the viewer's role, which is right for showing controls and wrong for copy, since a member reading a draft cannot remove a duplicate but the roster is not settled. Skipped: the exclude/include toggle's missing announcement, filed alongside these as a copy fix. It is not one — those controls are server actions with revalidation, so a fix needs #128's `?done=&at=` contract, and it is one of five controls on this page in the same state. Fixing one leaves four inconsistent with it. See #141.
1 participant
Round 7 of the autonomous
design-sweep-looprun, from a re-review pass after rounds 1–6. Branched offmain, not stacked — see Why this one isn't stacked below.One sentence: two in-cell controls never got the hit-target fix their sibling got, and this applies it.
What this round resolved
globals.css:2476-2489already contains the argument, written out and measured, for exactly this problem:That fix was applied to
.json > summaryalone, because it was the only one anybody had put a ruler against. Two controls with the identical shape were left at the bare line box..cell-link— the audit log's actor, action and target filtersEvery resolved name in the audit log is a filter control: pressing it narrows the log to that actor or target. Each is the sole content of its
<td>, so the inline-link exception doesn't reach it any more than it reached.json > summary. At--t-datathe target was the text line, around 19px.Cost to a user: audit rows sit edge to edge with no gap between
.log tdborders. A mistap doesn't land on nothing — it lands on the neighbouring row and silently applies the wrong filter. That's on the one page in the app whose entire job is identifying precisely who did what to whom..log summary—PaymentHistory's "payments (N)" toggleStyled by
.log summary, not by.json, so it never picked up the sibling fix. Same bare line box.Fixed with
paddingrather thanalign-items: center, because the rule directly above it already documents why it usesflex-start— a long character name wraps to two lines and a centred marker floats away from the first. Centring to reach the height would have undone a decision the file explains three lines earlier..json > summarystill overrides both properties and is unchanged.What was skipped
Nothing in this round. Both items had the fix already written and justified one selector away, which is why they were cut together.
Findings this re-review raised that turned out to be stale
Worth recording, because they were filed against the tree as it stood before #128 and #139 merged:
.btn--microhardcoding0.6875rem/0.08eminstead of the tokens — already tokenized on currentmain. Only.copy-result'sletter-spacingstill hardcodes; that's round 8.What needs a human
The announcement-and-focus family on
/payouts/[id]. Five controls unmount or rename themselves on success with nothing announcing the result: delete a pool, remove a participant, mark paid, revert, and the exclude/include toggle (which changes its own accessible name while focused —confirm-submit.tsx:154-156states as settled fact for this codebase that a name change on an already-focused control is not reliably re-announced).This was out of reach when rounds 1–6 ran. #128 changed that:
ConfirmNoticeis now a shared primitive with two consumers and a documented?done=&at=contract. Extending it to these five means touching server actions to redirect with params and writing the payouts confirmation copy — bounded work with precedent, but it's one coherent round for all five, not a patch on one. Fixing the exclude/include toggle alone would leave four controls inconsistent with it, which is worse than leaving all five.Gate results
scripts/check-node-version.shnpm run typechecknpm run lintnpm run format:checkAll matched files use Prettier code style!npm testnpm run test:e2e -- admin.spec.ts payouts.spec.tsdocker build .npm run test:e2eThe browser specs were run for this round specifically rather than skipped, because round 4 of this run shipped a label change that broke 22 payouts specs and none of the per-round gates caught it. This round changes no copy, but the same class of surface is involved.
Tree held only
src/app/globals.cssafter every gate.Why this one isn't stacked
Rounds 1–6 were a GitHub native stack. That bought conflict-freedom and cost automated review: CodeRabbit disables auto-review on any PR whose base isn't the default branch, so five of the six were never reviewed until they were triggered by hand — and the base branch can't be retargeted afterwards, because the API refuses to move a stacked PR's base.
This round branches off
maininstead. It carries no dependency on #133/#134/#136/#137/#138, and it gets reviewed.