fix(account): stop the gold CTA from firing on three states it says it excludes - #132
Closed
guarzo wants to merge 1 commit into
Closed
fix(account): stop the gold CTA from firing on three states it says it excludes#132guarzo wants to merge 1 commit into
guarzo wants to merge 1 commit into
Conversation
…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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This was referenced Aug 6, 2026
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 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. Based on
design-sweep/2026-08-06-1(#131), not onmain.Merge #131 first, and prefer a merge commit or rebase over squash. Squash-merging #131 rewrites the commit this branch is based on, and this PR will then show conflicts against changes that are, in substance, already there.