fix(payouts): tell the operator what goes in the loot paste field - #136
Merged
Conversation
…unced `Notice`'s own docblock names the `&&` form as "the one shape that defeats the live region it just asked for": it inserts a role="alert"/"status" node with its text already inside it, and AT announces a *change* to a region far more reliably than a region born holding text. The docblock adds that keeping the behaviour in the primitive is the point, "so the next caller cannot omit it by writing `&&`" — but six call sites across four files did exactly that. Only admin/sync had adopted the sanctioned form. Cost: an admin on NVDA or JAWS who freezes an account, approves a pending one, queues a sync, hits a mutation error on a payout, or narrows the audit filter to an ambiguous actor is not reliably told what happened. Every one of those paths ends in a server-action redirect that re-renders without a document load, which is precisely the case the region was built for and precisely the case `&&` breaks. Converting is free of layout cost: an empty `Notice` renders `.notice-slot`, which globals.css deliberately takes out of flow (position: absolute) after an earlier regression put ~64px of dead space on /payouts/new. Deliberately not converted, and left for a later round: - payouts/[id] `droppedReport` and account `firstSyncPending` are conditional content blocks rather than single-expression message slots. The first is a genuine mutation outcome and probably should follow; it is a larger diff and a separate judgement, so it is reported rather than bundled here. Gates run: typecheck, lint, format:check, build, npm test (76 files, 1085 tests). Not run this round: docker build, test:e2e.
…t excludes Two findings on /account, both cases of the code contradicting a comment sitting directly above it. The "Add character" link took `.btn--primary` whenever `health.attention === 0`, under a comment reading "Gold only in the nominal state." The verdict ladder (core/account-health.ts:163-177) makes `stalled`, `discord-stale` and `first-sync-pending` all satisfy `attention === 0` — so on three of the four non-nominal states the page rendered its loudest control directly beneath a `.verdict` line telling the member something was wrong. A member whose sync has stopped opens the page and the brightest thing on it invites them to add another character to the pile. Keyed off `health.verdict === "nominal"` instead, which is the test the comment already describes. A zero-character account still computes to `nominal`, so the case that comment calls out explicitly is unchanged. The standalone Discord unlink used `.btn--micro` — the 28px grade DESIGN.md:228 reserves for "the in-row controls of the admin tables... and nowhere else." It sits in the facts grid, not a table row. inline-edit.tsx:75-83 already made this exact call for this exact grid, and `.inline-edit--standalone` (globals.css:1695) exists only to restore the floor where a class had to keep its colouring; nothing had to be kept here, so dropping `--micro` is the whole fix. A member on a phone gets a 36px target for the one control on the page that strips every managed role in the guild. The heavier-at-rest argument in the comment above it is a colour decision and is untouched. Gates: node-version, typecheck, lint, format:check, build, npm test (76 files / 1085 tests) all pass. docker build and test:e2e not run.
…ders tiers The pending-approval pair rendered Alumni before Associate. Every other tier list on the page runs member, associate, alumni: the filter chips (TIER_FILTERS, line 195) and the set-tier group in the same drawer, in the same `.btn-group` slot, two rows apart (TIERS, line 710). Cost to an admin: approving the queue is a scanning task done several times a week, and the one control group that reverses the order is the one whose press cannot be taken back with the same control. `approveAccount` refuses once the tier is no longer pending (services/admin-accounts.ts:132), so a mis-click has to be corrected from the set-tier group instead — after it has already written a `tier.approved` audit row naming the wrong tier and enqueued an account sync. The two buttons are also asymmetric in a way neither label shows: associate sets `tierLocked: true`, alumni leaves the tier under automatic management (line 133). Muscle memory landing one position off does not land on a near-miss. Derived from TIERS via APPROVE_TIERS rather than reordered in place, so the two groups cannot drift apart again. `member` is excluded because approveAccount's signature accepts only the other two, which is what makes the derivation type-checked rather than a convention. No behaviour change beyond render order: both e2e specs that drive these buttons select by accessible name, not position. Gates: node-version, typecheck, lint, format:check, build, npm test (76 files / 1085 tests) all pass. docker build and test:e2e not run.
Both loot textareas were labelled "Loot paste" with no format. On the composer
that field sits ten lines above "Roster paste (optional: one per line, or
separated by /)" — two sibling pastes on one form, one of which answers "what
do I put here" and one of which does not.
Cost to an operator: creating an operation means facing a ten-row empty box
with no indication of the expected shape. An EVE inventory paste works, but
nothing says so, and a hand-typed list is a guess. A bare quantity on its own
line is dropped rather than absorbed (core/loot-paste.ts:145) — correct
behaviour, added precisely because absorbing it produced a silent wrong answer,
but the operator only meets that rule after submitting.
Both fields get the same hint because both feed parseLootPaste; an operator who
learns the format on the composer should not rediscover it on the detail page.
The wording follows the parser rather than describing an ideal: quantity is
accepted on either side of the name, and the tab-separated inventory layout is
read from column two, so "one item per line" covers the paste case without
spelling it out.
No test breakage: e2e/payouts.spec.ts:1718 selects with getByLabel("Loot
paste"), which is a substring match, so the longer label still resolves.
Gates: node-version, typecheck, lint, format:check, build, npm test
(76 files / 1085 tests) all pass. docker build and test:e2e not run.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 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 (2)
Comment |
The hint added in this round ended "…quantity before or after the name".
`getByLabel` matches on a substring, so that label was also a match for
`getByLabel("Name")` — the Name input sits ten lines above it on the same
form — and the two resolving together is a Playwright strict-mode violation.
It took out 22 specs in e2e/payouts.spec.ts, every one of them at the
`getByLabel("Name").fill(...)` line that every payouts test starts from.
Cost to a user: none directly — the hint reads the same either way. The cost
was to the next person to touch this file, who would have inherited a red
payouts suite with a failure message pointing at the Name field rather than
at the label that actually changed.
Reworded to "one line per item, quantity before or after" on both the
composer and the appraisal form, which says the same thing without using a
word that another label on the page owns. The constraint is written into the
comment above each so the wording is not "corrected" back later.
Found by CI, not by the sweep: `npm run test:e2e` was one of the two gates
this run declared it was skipping per round. That declaration is why the
failure was attributable in one step, but skipping it is what let the commit
land. Verified here by running the suite locally — 57 passed in
e2e/payouts.spec.ts — plus typecheck, lint and format:check.
Owner
Author
|
@coderabbitai review |
|
# Conflicts: # src/app/admin/accounts/page.tsx
guarzo
added a commit
that referenced
this pull request
Aug 6, 2026
… one (#147) `reviews.auto_review.base_branches` defaults to empty, and empty means automatic review fires only on PRs targeting the default branch. The design-sweep-loop skill opens one PR per round based on the previous round's branch, so on the 2026-08-06 run every stacked PR but the first was out of scope — #134 and #136 both came back "Review skipped: reviews are disabled for this base branch." The failure is quiet in the way that matters: `gh pr checks` prints `CodeRabbit pass` for a skipped review exactly as it does for one that ran and found nothing, so an unattended run reports a clean second opinion it never got. #134's finding only surfaced because the review was requested by hand. Scoped to `design-sweep/.*` rather than `.*` so this does not also switch on review for the ad-hoc `worktree-*` branches that occasionally serve as a base. The account-wide rate limit that hit the same run is a separate matter and is not addressed here.
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.
Round 4 of the autonomous
design-sweep-looprun. One theme: the two loot paste fields.What this round resolved
Both loot textareas were labelled
Loot pastewith no format. On the composer that field sits ten lines above its own sibling:Two pastes on one form; one answers "what do I put here" and the other does not.
Cost to an operator: creating an operation means facing a ten-row empty box with no indication of the expected shape. An EVE inventory paste works, but nothing on screen says so, and a hand-typed list is a guess.
parseLootPastealso drops a bare quantity on its own line rather than absorbing it (core/loot-paste.ts:142-148) — correct behaviour, added precisely because absorbing it produced a silent zero-priced row — but the operator only meets that rule after submitting.The hint is written from the parser rather than from an ideal format.
parseLootPasteaccepts a quantity before the name, after the name, or as column two of a tab-separated line (loot-paste.ts:153-170), so "one item per line, quantity before or after the name" is true of every accepted shape and the inventory-window paste is covered without spelling it out.Applied to both
payouts/new/new-operation-form.tsxandpayouts/[id]/appraise-form.tsx, because both feed the same parser and an operator who learns the format on one should not rediscover it on the other. The detail-page field isrequired, so its hint drops the "optional:".No test breakage.
e2e/payouts.spec.ts:1718,1725usegetByLabel("Loot paste")withoutexact: true, which is a substring match, so the longer label still resolves. (Flagging it becausetest:e2eis not run per round — see gates.)What was skipped, and why
/payoutslede saying "Every fight operation" — dropped as a false positive. I had this queued as a typo for "fleet"/"flight". It is deliberate domain language:payouts/new/page.tsx:42reads "One row per fight," and the feature commit that introduced the surface is #65, "track fight payouts." Recording it because a future sweep will find it again and should stop at the same place.Refreshon/admin/synclooking identical to the two mutating controls beside it — left alone. The finding is real:<a className="btn">Refresh</a>sits in the same.btn-rowasRecheck invalid affiliations, at the same grade, and only one of them enqueues jobs. ButDESIGN.md:184-187defines the three button grades by prominence (primary/default/quiet), not by whether a control mutates. DemotingRefreshtoquietwould be inventing a convention the design system does not have, on the strength of one instance. That is a decision, not a consistency fix.What needs a human
Unchanged and accumulating. From this round's surface: the payout forms still have no per-field error association (
aria-invalid/aria-describedby) — messages name their field in prose, which satisfies WCAG 3.3.1's letter, but a screen-reader user who tabs into a ten-row textarea loses the announcement. That is the fix this round's label hint sits next to and does not address.Also open:
/admin/accountsapprove buttons not saying that Associate locks the tier (#134); focus dropping to<body>after/accountself-service actions (#132); no way to find a past operation on/payouts; no confirm-arm on tier-set.Gate results
scripts/check-node-version.shnpm run typechecknpm run lintnpm run format:checkAll matched files use Prettier code style!npm run buildnpm testdocker build .npm run test:e2eStack position
PR 4 of the stack, based on
design-sweep/2026-08-06-3(#134).Merge #131 → #132 → #134 → this, and prefer a merge commit or rebase over squash.
Correction: this round broke 22 e2e specs, and CI caught it
The label shipped above originally read "Loot paste (optional: one item per line, quantity before or after the name)".
getByLabelmatches on a substring, so that label was also a match forgetByLabel("Name")— the Name input is ten lines above it on the same form — and the two resolving together is a Playwright strict-mode violation:Every payouts spec starts from that
getByLabel("Name").fill(...)line, so 22 of them failed at setup. The suite also printed arelation "payout_operation" does not existstack trace, which is unrelated server noise and much louder than the real cause — worth saying, because it is what a first read of the log lands on.Fixed in c44517f, on this branch, and merged forward into
-5and-6so #137 and #138 carry it too. Both labels now read "one line per item, quantity before or after", which says the same thing without using a word another label on the page owns. The constraint is written into the comment above each so the wording is not "improved" back later.How it got through:
npm run test:e2ewas one of the two gates this run declared it was skipping per round (see the gate table above). That declaration is why the failure was attributable to this round in one step — the PRs below it were green — but skipping it is what let the commit land in the first place. This is the sweep's characteristic failure mode: copy is a selector, so a purely cosmetic diff can break a browser suite while typecheck, lint, build and unit tests all stay green.Verified by running the gate that caught it, locally, before pushing:
npm run test:e2e -- payouts.spec.tsnpm run typechecknpm run lintnpm run format:checkAll matched files use Prettier code style!Latent fragility left alone, deliberately:
getByLabel("Name")is non-exact and will collide with any future label containing the word "name". Tightening it to{ exact: true }is a change toe2e/payouts.spec.ts— a settled decision this sweep is not the place to overturn — so it is flagged here rather than fixed.