fix(account): tell a member at a glance whether their access is healthy - #71
Conversation
The account page stated every fact about a member's access and ranked none of them, so a healthy account and one with three dead tokens looked identical above the fold. Adds one derived verdict line under the h1, and fixes the four states the critique found unreadable behind it. - computeAccountHealth (src/core/account-health.ts) splits what a member must fix from what they can only be told about: token faults, missing scopes and the five clearable contacts codes count as attention, while the transient retries, dry_run and unrecognized codes count as stalled. The remedy copy for the latter says "nothing to do here", and a headline demanding action over copy denying it teaches members to ignore headlines. - Contacts remediation prose moves out of the CONTACTS cell to below the table. Inside the horizontally-scrolling Scroller it drove rows to ~340px at 320px wide while staying off-screen and unreachable. - The six result codes that fell through to a bare `result.replace(/_/g," ")` get real copy, in the three groups src/jobs/contacts.ts implies. - ContactRemedy renders the re-authorize control only when the caller opts in. /auth/eve/link links a character to the clicking user's account, so on the admin table it could not fix the row it sat under. - Zero-character accounts get a real empty state; the verdict line and the "Last pushed" block, which had nothing true to say, are suppressed. - Discord state stops being stated twice, ~800px apart.
- The stalled verdict said "nothing here is the member's to do", but an unrecognized code's remedy tells them to ask an admin. That is an action, just not a self-service one. The classification is unchanged; the wording is now about who owns the fix. - contactRemedyId claimed its element "always exists in the DOM", mirroring CONTACTS_NOTE_ID. It does not: the <p> only renders when hasContactRemedy is true. The behaviour was already correct, since the aria-describedby reference is gated on the same predicate, so the id cannot dangle.
onMapAcl is the obvious fourth input and is deliberately excluded. wanderer_acl_observation is a delete-and-replace snapshot, so a character legitimately off the ACL has no row and is indistinguishable from a job that has not run (src/services/account-view.ts:26-29). Counting it would raise an alarm the data cannot substantiate, for every member who is correctly off the map. Without this note the omission reads as an oversight and invites a fix that would make the verdict lie.
The verdict line landed as a plain .st token at --t-label (11px). The tier badge a block below it is .tier--lead: 14px, 600 weight, inside a gold border and a 14% fill. So on an account with three broken characters the loudest thing above the fold was the one fact that needed no action. Adds Status size="lead", mirroring the size="lead" the Tier badge already has in the same module, and applies it to the degraded and stalled verdicts only. Nominal and first-sync-pending keep the plain token: a page with nothing wrong should not open with a large line saying so. The lead token drops uppercase and the wide tracking. Both are legible devices at 11px where they mark a value in a table; at body size they turn a sentence into what reads like a section heading. Verified at 1280px and 320px. .st is nowrap, so the copy has a width budget rather than a wrap; recorded in the rule. npx tsc --noEmit: clean npx vitest run: 53 files, 463 tests passed npm run lint: 0 errors, 3 pre-existing warnings npm run test:e2e: 56 passed
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change centralizes contact-sync result types, adds account-health derivation, separates status and remedy rendering, and updates account and admin pages with new remediation, verdict, layout, and Discord reporting behavior. ChangesAccount health and contact remediation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AccountPage
participant computeAccountHealth
participant ContactState
participant ContactRemedy
AccountPage->>computeAccountHealth: character health inputs
computeAccountHealth-->>AccountPage: AccountHealth verdict
AccountPage->>ContactState: contact result and target
AccountPage->>ContactRemedy: result, detail, and label
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
`contact_sync_state.last_result` was bare `text()` and `recordResult` took a bare `string`, so the nine codes the contacts job writes existed only as scattered literals. Nothing connected the writer to the three readers that branch on them. Adds `src/core/contact-result.ts` holding CONTACT_SYNC_RESULTS and the derived ContactSyncResult union, narrows the column with `$type<>()`, and types `recordResult`'s parameter. Not a pgEnum, unlike token_status. That column is a closed set the tier state machine branches on, where a value outside it has no sensible handling and the database refusing it is a feature. This vocabulary is open: ContactRemedy already has designed copy for a code it does not recognize, and a pgEnum would make every new code a migration and turn a rolling deploy into a write failure instead of one vague line of UI. `npm run db:generate` confirms: "No schema changes, nothing to migrate". Readers keep `string | null`, documented at both service-layer boundaries. A row can hold a code this build has never heard of, so handing readers the union would let a switch look exhaustive when it is not, and would make the deliberate fallback branch read as dead code. The sharpest catch is MEMBER_FIXABLE in account-health.ts: it was `Set<string>`, so a typo compiled and its only symptom was a verdict that under-counted forever while the character's own row still showed the fault. Now constructed as `Set<ContactSyncResult>` and declared `ReadonlySet<string>`, so the typo check happens at construction while `.has()` still takes the widened value a reader holds. tests/contact-result.test.ts closes the gap the types cannot: readers take `string`, so no type forces them to grow a branch. It iterates the vocabulary and demands real UI copy plus a verdict classification for every code. Both guards were mutation-tested, not assumed. Adding "quota_exceeded" to the union failed the ContactRemedy and verdict cases; typo'ing a write-site literal to "missing_scop" failed tsc with a did-you-mean. The ContactState case cannot detect drift (its fallback renders the same text as several real branches) and says so rather than implying coverage it does not have. npx tsc --noEmit: clean npx vitest run: 54 files, 490 tests passed npm run lint: 0 errors, 3 pre-existing warnings npm run test:e2e: 56 passed npm run db:generate: no schema changes
Three files conflicted, all in the blocks main rewrote under me: - src/app/account/page.tsx — main swapped hand-rolled `.notice` paragraphs for the new <Notice> primitive and moved the shell nav to href-based matching (current="/account"). Kept both: the first-sync notice is now a <Notice>, still gated on health.firstSyncPending so it does not restate a verdict that already leads with the fault. - src/app/admin/accounts/page.tsx — main renamed RowDisclosure to Disclosure. Took the rename; the contact-state imports are unchanged. - src/app/_components/ui.tsx — main added its own contract comment above Status. Folded the size="lead" paragraph into it rather than leaving two adjacent JSDoc blocks on one function. Also formatted two files that were already over the 90-column limit before the merge: my it.each list in tests/account-health.test.ts and the resolved admin accounts page. Verified after the merge, not before: npx tsc --noEmit clean npx vitest run 57 files, 580 tests passed npm run lint 0 errors, 3 pre-existing img warnings npm run format:check clean after the two files above npm run test:e2e 74 passed
|
@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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/account/contact-state.tsx (1)
95-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReword the label remedies so they do not end with the transient-failure phrase.
missing_labelandlabel_mismatchare member-fixable.src/core/account-health.tsline 75-81 puts both inMEMBER_FIXABLE, so they raise theattentioncount and drive thedegradedverdict.Both remedies end with "nothing to do here". Line 180 uses the same phrase for
token_refresh_failedandsync_failed, which count asstalledand need no member action at all. The phrase now means two different things.
src/core/account-health.tslines 38-43 identify this exact combination as the failure to avoid: a headline that demands attention above copy that says the opposite.The member must rename or create a label in game for these two codes. State that the sync needs no further action, not that the member has none.
✏️ Proposed copy fix
if (result === "missing_label") { return ( <span className="dim"> Create a contact label named <code className="literal">{`"${label}"`}</code> in - game. The next sync picks it up on its own — nothing to do here. + game. The next sync picks it up on its own — no further step after that. </span> ); }Your label is named <code className="literal">{`"${candidates[0]}"`}</code>. It must be exactly <code className="literal">{`"${label}"`}</code> — capitalization and spaces both count. Rename it in game. The next sync picks - it up on its own — nothing to do here. + it up on its own — no further step after that.Apply the same wording to the remaining two
label_mismatchbranches at lines 133 and 139.🤖 Prompt for 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. In `@src/app/account/contact-state.tsx` around lines 95 - 144, Reword the remedies in the missing_label branch and all three label_mismatch branches to state that the member must create or rename the label in game, after which the next sync needs no further action. Replace the misleading “nothing to do here” wording while preserving the existing label-specific instructions and candidate rendering.
🤖 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/account/page.tsx`:
- Around line 207-220: The Notice component that renders when
health.firstSyncPending is true unconditionally promises Discord role updates,
but this message is misleading when Discord is not linked. Update the
conditional rendering of the Notice component to additionally check that
view.discordLinked is true, so the Discord role update promise only displays
when Discord is actually linked to the account.
---
Outside diff comments:
In `@src/app/account/contact-state.tsx`:
- Around line 95-144: Reword the remedies in the missing_label branch and all
three label_mismatch branches to state that the member must create or rename the
label in game, after which the next sync needs no further action. Replace the
misleading “nothing to do here” wording while preserving the existing
label-specific instructions and candidate rendering.
🪄 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: df54826a-8343-4d5b-a26e-0f422b852e97
📒 Files selected for processing (14)
e2e/account.spec.tssrc/app/_components/ui.tsxsrc/app/account/contact-state.tsxsrc/app/account/page.tsxsrc/app/admin/accounts/page.tsxsrc/app/globals.csssrc/core/account-health.tssrc/core/contact-result.tssrc/db/schema.tssrc/jobs/contacts.tssrc/services/account-view.tstests/account-health.test.tstests/account-page.test.tstests/contact-result.test.ts
…ther side Two findings, both real, but the second one names the wrong file to change. 1. The first-sync notice promised Discord roles on an account with no Discord link, rendering directly above the "Link Discord" button it was contradicting. Worse than merely false: it told the member the one step they still owed was already handled. The clause is now conditional, and the unlinked branch spends it pointing at the control instead of dropping the subject. 2. "nothing to do here" carried two meanings. CodeRabbit proposed rewording the four label branches. Blame says those are shipped copy from #53 and #60, where the phrase means "nothing more to do *in authGD*" and sits after an explicit "rename it in game". The collision came from the transient branch this PR added, which reused the phrase for codes that ask nothing of anyone. Reworded that side to "No action needed." and left the shipped copy alone. Guards, both mutation-tested rather than assumed: - e2e/account.spec.ts gains the linked case and extends the unlinked one. Forcing the conditional to always-linked fails the unlinked assertion with the old sentence quoted back. - tests/account-page.test.ts now asserts the transient codes do *not* contain "othing to do here", so the phrase cannot drift back. Verified: npx tsc --noEmit clean npx vitest run 57 files, 580 tests passed npm run lint 0 errors, 3 pre-existing img warnings npm run format:check clean npm run test:e2e 75 passed Not fixed, out of scope: the label remedies use em dashes in member-facing copy, which DESIGN.md's copy rules ban. That is shipped code this PR does not touch.
|
Both findings taken, but the second one from the other side. 1. Discord promise on an unlinked account — valid, and worse than stated. The notice renders directly above the
2. "nothing to do here" — the phrase collision is real, but the direction is backwards. Guards, mutation-tested rather than assumed:
Separately, out of scope here: the label remedies use em dashes in member-facing copy, which our copy rules ban. Shipped code this PR does not touch. |
1 participant
What this is
An
$impeccable critiqueof/accountfound that a member could not tell at a glance whether their access was healthy. The page listed facts (token state, contacts state, map state, tier, Discord) but never drew a conclusion from them, so answering "is anything wrong with me?" meant reading a six-column table and knowing which values were bad. This implements the five fixes that critique recommended.What changed
A derived verdict, one line under the
<h1>. New pure modulesrc/core/account-health.tscomputes it. One derivation now feeds three surfaces that previously each decided for themselves: the verdict line, the first-sync notice, and the colour grade onAdd character(gold only when there is nothing else to do first).The verdict splits two states that look alike and are not:
token_refresh_failedandsync_failedretry themselves,dry_runisSYNC_MODE, an unrecognized code goes to an admin.They are counted separately because
ContactRemedytells the member "nothing to do here" for the second group. A headline demanding attention above copy saying the opposite teaches members that the headline is noise.Map ACL state is deliberately not an input.
wanderer_acl_observationis a delete-and-replace snapshot, soonMapAcl === falsecannot distinguish "legitimately off the map" from "the job has not run" — a verdict counting it would alarm every member who is correctly off the map. The reasoning is recorded in the module so nobody "fixes" it later.Remediation prose moved out of the table cell. At 320px the manifest lives in a horizontally-scrolling
Scroller; prose inside a<td>drove row height to ~340px while staying off-screen and unreachable. It now renders as a keyed list below the table at all widths, wired back to its row witharia-describedby.Copy for the six result codes that had none. They previously fell through to
result.replace(/_/g, " ")— a bare, alarm-red code with zero guidance. Each now gets one of three treatments matching who owns the fix, and unrecognized codes get a sentence instead of a raw string.A re-authorize control where re-authorizing is the fix, gated behind
showReauthwhich defaults to off./auth/eve/linklinks a character to the clicking user's account. The admin accounts table renders this same component for other members' characters, so an ungated control there would have started a link flow against the admin's own account instead of fixing the row it sat under. Found during review.A real empty state for the zero-character account, and the verdict suppressed there entirely — "nominal" would be true and useless.
Verdict weight. The verdict initially rendered at
--t-label(11px) while.tier--leadis 14px inside a gold border and fill, so the loudest thing on a degraded page was the fact that needed no action.Statusnow takessize="lead"(mirroringTier's existingsize="lead"), applied to the degraded and stalled states only.Reviewer focus
src/core/account-health.ts— theattention/stalledsplit, and thefirstSyncPendingflag being deliberately independent of the verdict. A character linked seconds ago has no scopes yet, so it needs attention and is waiting on its first run; collapsing those into one priority-ordered state silently dropped the notice telling the member the wait is minutes rather than broken.tests/account-health.test.tspins that case.showReauthdefault. Off is the safe default precisely because the unsafe caller is the one that would forget to pass it.Verification
All run on the final tree:
Six states were checked in the browser at 1280px and 320px: nominal, degraded, stalled, first-sync-pending, zero characters, and label mismatch.
Not done
no-img-elementlint warnings are pre-existing onmain.lastResultis baretext()atsrc/db/schema.ts:142; structural drift is compiler-caught, but a genuinely new code is handled by design rather than by the type, and is pinned by asome_new_codetest case.Summary by CodeRabbit
New Features
Bug Fixes
Tests