design sweep round 2: make /account's two controls do what the comments above them say - #133
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 31 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 |
9ebc812 to
84d0598
Compare
guarzo
changed the title
|
@coderabbitai review |
|
84d0598 to
bae7eec
Compare
bae7eec to
9ed04eb
Compare
…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.
9ed04eb to
5eb7d9d
Compare
1 participant
Round 2 of the autonomous
design-sweep-looprun. One surface —/account. Both findings are the same shape: the code contradicts a comment sitting directly above it.What this round resolved
The gold "Add character" CTA fires on three states its own comment excludes
The comment above it reads "Gold only in the nominal state." The condition was
health.attention === 0. Those are not the same test. The verdict ladder atsrc/core/account-health.ts:163-177:Three of the four non-nominal verdicts satisfy
attention === 0. Each of them renders a.verdictline ataccount/page.tsx:191-225telling the member something is wrong — and directly beneath it the page rendered its single loudest control inviting them to add another character.Cost to a user: a member whose contact sync has stopped, or whose Discord roles have gone stale, opens
/account, sees a warning, and the brightest thing on the page is Add character. That is precisely the inversion DESIGN.md's gold rationing and the "state before action" principle exist to prevent — and the comment shows someone already knew it; the condition just never expressed it.Now keyed off
health.verdict === "nominal". The zero-character case the comment calls out explicitly (computeAccountHealthhas no target and no fault to find →nominal) still gets gold, unchanged.The standalone Discord unlink used the in-row 28px grade
DESIGN.md:228-233:The Discord unlink is in a
<dd className="facts__lead">— the facts grid, not a table row — and carried.btn--micro(min-height: 1.75rem).The repo had already adjudicated this exact question.
payouts/[id]/inline-edit.tsx:75-83:.inline-edit--standalone(globals.css:1695) exists only to buy the floor back where a class had to keep its colouring. Nothing had to be kept here, so dropping--microis the entire fix.Cost to a user: a member on a phone gets a 28px target for the one control on the page that enqueues a deprovision stripping every managed role in the guild. The three character-row unlinks below it (
page.tsx:483/517/530) are genuinely in-row and are untouched.The "heavier at rest than the character-row unlinks" argument in the comment above is about
.btn--quiet/.btn--danger-quiet— a colour grade — and never depended on the size. It still holds.What was skipped
Nothing was skipped in this round. Both findings had an in-repo precedent that settled the fix rather than leaving it to taste, which is why they were cut together.
Checked before editing: no test in
tests/ore2e/pins either behaviour.e2e/payouts.spec.ts:1838asserts one.btn--primaryat a time, but on/payouts, not here.e2e/account.spec.ts:411-450drives the Discord unlink by accessible name and is indifferent to its size class.What needs a human
Unchanged from PR #131 and still the more important half of the output. The one that lands on this surface:
Focus is dropped to
<body>after every self-service action on/account.unlinkAction,setMainActionandunlinkDiscordActionall revalidate in place, and in each case the node holding focus is the one that unmounts on success. A keyboard or screen-reader member who just unlinked a character is silently returned to the top of the document with no confirmation it worked. PR #131 makes theNoticeregion able to announce; it does not give these three actions anything to announce, because they succeed by redirecting rather than by setting a message. Fixing it means deciding what receives focus and what it says —shapework, which needs a brief someone approves.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:e2eTree held only
src/app/account/page.tsxafter every gate.Stack position
PR 2 of the stack, re-opened from #132 after #131 (round 1) was merged mid-run, which auto-closed #132 along with its base branch. Retargeted at
main, which is correct now that round one is inmain— this branch already contains it, so the diff shown here is round two alone.The rest of the stack (#134 → #136 → #137 → #138) is still chained on the original branches and must merge in order after this one. Prefer a merge commit or rebase over squash: squashing rewrites the commits the later PRs are based on, and they will then show conflicts against changes that are, in substance, already there.