fix(admin): order the approve buttons the way the rest of the page orders tiers - #134
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 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)
📝 WalkthroughWalkthroughThe admin accounts page now derives approval tiers from ChangesApproval controls
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
…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.
1be12bc to
f1eb9d0
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/admin/accounts/page.tsx`:
- Around line 60-69: Update APPROVE_TIERS to derive its element type from TIERS
rather than hard-coding "associate" | "alumni". Keep member excluded at the type
level and runtime, while allowing any future non-member TIERS entry to flow
through with the type expected by approveAccount.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c908109c-0a52-4f31-896a-af2cc6e3b3df
📒 Files selected for processing (1)
src/app/admin/accounts/page.tsx
…lip past it Per CodeRabbit on #134. The predicate named its two survivors — `t is "associate" | "alumni"` — which is a legal narrowing for any wider union, so adding a fourth tier to TIERS would have put it in this array at runtime while the type still said the old pair. The `approveAction.bind` below would have gone on compiling and failed only when an admin pressed the button. The comment directly above claimed the opposite: "the type would reject it, which is the check that keeps this derivation honest if TIERS ever grows". It would not have. `Exclude<(typeof TIERS)[number], "member">` makes it true — verified by temporarily adding a "future" tier, which produces `TS2345: Argument of type '"associate" | "alumni" | "future"' is not assignable to parameter of type '"associate" | "alumni"'` at the bind, and by removing it again. The comment now says which failure is the guard rather than asserting that one exists.
|
Applied. The finding is correct and it falsified the comment sitting directly above the line — that comment claimed Verified rather than assumed. Temporarily adding a which is the failure the comment was claiming as its guard. Probe reverted; |
…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.
… 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.
# Conflicts: # src/app/admin/accounts/page.tsx
98f3931 to
01d29d7
Compare
01d29d7 to
a477e11
Compare
1 participant
Round 3 of the autonomous
design-sweep-looprun. One control group on/admin/accounts.What this round resolved
The pending-approval pair rendered Alumni, then Associate. Every other tier list on the page runs
member, associate, alumni:page.tsx:195)TIER_FILTERS.btn-groupslot (page.tsx:710)TIERSpage.tsx:684-707)The two drawer groups occupy the same position in the same container — an admin working the queue scans from one row's set-tier group straight into the next row's approve pair.
Cost to an admin: approving the queue is a scanning task done several times a week, and the one group that reverses the order is the one whose press cannot be undone with the same control.
approveAccountrefuses once the tier is no longerpending(services/admin-accounts.ts:132), so the pending buttons vanish on success. Correcting a mis-click means reaching for the set-tier group instead — after the wrong press has already written atier.approvedaudit row naming the wrong tier and enqueued an account sync.And the two buttons are not near-misses of each other.
services/admin-accounts.ts:133:Approving as Associate locks the tier out of automatic management. Approving as Alumni leaves it automatic. Landing one position off does not produce a slightly-wrong outcome; it produces a different kind of account.
Fixed by deriving
APPROVE_TIERSfromTIERSrather than reordering the literals, so the two groups cannot drift apart again.memberis excluded becauseapproveAction/approveAccountare typed"alumni" | "associate"— the exclusion is type-checked, not a convention someone has to remember.No behaviour change beyond render order. Both e2e specs that drive these buttons (
e2e/admin.spec.ts:504-724,e2e/shell.spec.ts:309) select by accessible name, not by position ornth.What was skipped, and why
The lock asymmetry itself is not fixed here, and it is the larger finding. Nothing in either button's label, tooltip, or surrounding text says that Associate locks the tier and Alumni does not. Every available fix ran into something the repo had already decided:
aria-describedbytoSubmit—submit.tsx:25-53deliberately exposes a closed prop set, with a written rationale for why props are opt-in and enumerated. Widening a shared primitive is round-one-shaped work and belongs in its own change with its own review.ConfirmCostdoes on/account(page.tsx:398), but there the note is wired withdescribedBy. An unassociated note here would be a new instance of the exact defect already filed as needs-a-human in fix(a11y): mount the Notice live regions so mutation results are announced #131 (payout forms name their field in prose with noaria-invalid/aria-describedbylink).So it is reported rather than guessed at. See below.
What needs a human
Approving as Associate locks the tier; approving as Alumni does not; neither button says so.
tierLocked: truetakes the account out of automatic tier management — the thingderole-don't-bootexists to drive — and the only way an admin learns which they chose is the lock indicator on<Tier>afterwards, plus anautobutton appearing in the drawer to undo it. The admin is being asked to make a durable policy decision through two buttons that differ only in a noun. The fix needs a decision about where the consequence goes (primitive, label, or associated hint), which is why this PR does not make it.Also still open from #131 and #132: focus dropping to
<body>after/accountself-service actions; the tier-set button disabling itself post-mutation; no way to find a past operation on/payouts; no per-field error association in the payout forms; no confirm-arm on tier-set.Gate results
scripts/check-node-version.shnpm run typechecknpm run lintnpm run format:checkprettier --write, re-checked cleannpm run buildnpm testdocker build .npm run test:e2eThe format failure is recorded rather than hidden: it was the loop's one permitted retry, the retry was a formatter run with no semantic content, and every gate was re-run afterwards.
Stack position
PR 3 of the stack. Based on
design-sweep/2026-08-06-2(#132), which is based ondesign-sweep/2026-08-06-1(#131).Merge #131, then #132, then this — and prefer a merge commit or rebase over squash. Squash-merging an earlier PR rewrites the commits the later ones are based on, and they will then show conflicts against changes that are, in substance, already there.
Summary by CodeRabbit