perf(account): make the crew manifest dense enough to stop scrolling - #169
Merged
Conversation
The row's height floor was never the STATUS cell — it is the NAME cell's two lines plus cell padding. Round 1 collapsed a cell that had 30px of slack in a 50px block, so the pitch did not move. Replaces #167's height-band assertion with a pitch ceiling. The band required a located row to be 10-30px taller than a no-location row, which is two lines by definition: it passed at 75px and would pass at 95px.
89px to 50px per character column, with the verb moved into a per-character accessible name — the same trade the unlink beside it already makes.
On the common account every row reads ok, costing a header, a column and 82px of a 320px viewport to report the absence of news. The column now renders when at least one character is not ok, so its presence is the signal. `map on|off` varies per character while the chip reads ok either way, so the cell's accessible name was its only home; it moves to a visually-hidden span in the NAME cell at zero vertical cost. `aria-describedby` cannot dangle: a contact remedy implies attention or stalled, never ok, so a remedy existing implies the column renders.
539px of chrome sat above the first manifest row at 1440x900 — 60% of the fold — and 171px of it was a rule-head, a two-row definition grid and a collapsed margin holding two facts. The verdict joins the h1's line at zero vertical cost; tier and Discord become one meta line under the lede. The <dt>s were the only thing naming those values, so each fact keeps a visually-hidden label. The confirm cost keeps its own line via .page__meta-item > .confirm-cost: without it the revealed sentence re-centres the button out from under a stationary pointer and the pointerLeave disarms the control (#112), pinned by a boundingBox test at 700px.
The success criterion is characters above the fold, so assert that directly — at 1440x900, 1280x800 and 390x844, not only the desktop case. Re-baselines the 320px horizontal gate onto a ten-character seed. Its previous 250px threshold came from a one-character account; a realistic account measured 258px, so the number the gate protected had never been taken at the size that matters.
…stale comments Whole-branch review of the manifest-density work (a4a292b..7c5654f) came back SHIP with only Minors: Task 4 duplicated .facts__lead/.facts__lead > .confirm-cost as .page__meta-item variants instead of renaming them, leaving the originals with no consumer; the all-ok manifest caption asserted every character is healthy even at zero characters, where showStatusColumn is vacuously false; and three comments described a `.facts` grid or a "block below" layout that Task 4's fold into the page head replaced. - Remove the dead .facts__lead / .facts__lead > .confirm-cost rules and point every comment that cited them (confirm-submit.tsx, globals.css's discord-id comment) at the live .page__meta-item selectors instead. - Give the zero-character case its own caption sentence instead of routing it through the all-healthy branch. - Correct standing.tsx's `.facts` grid reference, globals.css's "block below" verdict comment, and the .verdict margin-top comment (dead in practice — every call site is inside .page__head-row, which zeroes it) to match the post-fold layout. - Document that .page__meta-item > .confirm-cost's `>` combinator depends on ConfirmArmScope staying a pass-through with no wrapper element (the #112 fix goes dark silently otherwise).
The no-STATUS-column sentence is selected by `showStatusColumn`, a `.some()` over the crew — false for an empty account as readily as for a healthy one. The zero-character branch added in post-review cleanup was the only user-visible copy on this branch with no test behind it; removing it turns this test red.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 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 (5)
Comment |
guarzo
enabled auto-merge (squash)
August 7, 2026 18:10
This was referenced Aug 7, 2026
guarzo
added a commit
that referenced
this pull request
Aug 8, 2026
Sweep item #6 put one paragraph per non-main character under the crew manifest, explaining what pressing "make main" would do to the account's tier. But `previewMainChange` has only three outcomes, so the text has only three possible values — on a real 13-character account it rendered as thirteen paragraphs, eleven of them byte-identical after the name prefix. The comment claiming "up to nine of them can apply at once" understated it; the real ceiling is the crew size. Collapsing the duplicates was the obvious fix and the wrong one. The sentence is noise on a row nobody is acting on: it describes a hypothetical press of a button the member is not pressing, on a character that is not their main, and it landed directly on top of three rounds of density work (#169, #173, #174) whose whole point was reclaiming fold space. So the feature comes out rather than getting reformatted — including the per-page `getMainChangeContext` query that only ever backed it, which takes /account's parallel read burst from four connections back to three, and `previewMainChange` in core, whose only consumer this was. `decideTier`, which the membership job actually runs on, is untouched. The e2e assertion that scoped to the removed `.table-notes` container would have passed vacuously afterwards; it now counts the contact-remedy element directly.
guarzo
added a commit
that referenced
this pull request
Aug 8, 2026
Findings 3.1-3.4 of docs/design-walkthrough.md. 3.1 The health verdict right-aligned to `.page--narrow`'s 912px cap while the manifest ran full-measure, a measured 287px seam. The cap lands on `.page__head` (`.page--narrow > :where(*)` reaches direct children only, and `.page__head-row` is a grandchild), so `full-measure` goes there. `.page__meta` takes an explicit max-width so it does not inherit the wider measure from its newly-uncapped parent. 3.1b The lede restated the sections beneath it. Deleting it invalidated the anchor in e2e/account.spec.ts that proved the manifest opts out of the narrow cap; re-anchored on `.page__meta`, which still takes the cap, rather than on a bare pixel figure that would pass even if `page--narrow` were deleted wholesale. 3.2 Ruling R2: `main` and `unlink` were 40 permanent controls across 20 rows, in the widest column, for actions that are rare and destructive. They move into a per-row disclosure. Hand-rolled in character-row.tsx rather than reusing `Disclosure as="row"`, which puts its toggle in the row's first cell — here the portrait, the identity a screen-reader user scans for first. A row with neither action (a single-character account) renders no toggle onto an empty drawer. Per R2 the drawer's controls take the 36px standalone grade, which `--quiet` would otherwise cost them. 3.3 Measured rather than assumed, the density having been worked twice already (#169, #174). At 320px on a faulted crew the actions cell drops from ~155px to 113px and total forced scroll from 299px to 257px against a 286px region — under one region width for the first time, so a member on the narrowest viewport reaches the end of a row in one swipe. 3.4 An alt's location line is elided when it reads identically to the main's, text and offline flag both. The main keeps its own line: it is the anchor the comparison is stated against, and eliding it too would leave a co-located crew with no location stated anywhere. No main, or a main with no reading, switches elision off entirely. Implemented at the /account call site because CharacterLocation is shared with the /admin/accounts crew table, which is unchanged. Fold-count assertions that used bare `tbody tr` were silently counting the new hidden drawer rows, whose all-zero rects clear any fold trivially; they now exclude `.drawer-row`.
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.
A member with 10 characters still had to scroll the account page's crew manifest. #167 collapsed the STATUS cell to one chip, but visible density barely moved. This round started by measuring why.
What the measurement found
The row's height floor was never STATUS — it was the NAME cell's two lines plus vertical padding. And #167's geometry test asserted row height against a "no-location reference row + 10-30px" band, which encodes two lines as acceptable, so it passed while the page stayed sparse.
The larger surprise: the row was only part of the problem. 539px of chrome sat above the first row. Folding STANDING into the page head reclaimed 155px of that — a bigger win than anything done to the row itself.
Result
The success criterion here is characters above the fold, not lines per cell.
How it was done
perf— tighter manifest padding (--s-2/--s-3) and a smaller.char-linegap. New.log--manifestclass rather than borrowing.log--dense, whosevertical-alignandtr.drawer-rowoverride mean nothing here.feat— MAKE MAIN →main, with the accessible name kept asmake {name} main(WCAG 2.5.3 label-in-name: visible text is a subset of the accessible name).feat— STATUS column renders only when some character is notok. When it is absent, each row states its own token/standings/map state, and the<caption>says so.feat— STANDING folded into the page head.test— fold-count gate at three viewports, plus a re-baselined horizontal gate.Constraints honoured
<td>. Per a11y(account): separate the unlink control from the LINKED it sits beside #108/a11y(admin/accounts): say what unlinking a member's Discord costs #111/refactor(a11y): scope ConfirmCost to its own control, not to the whole scope #112, arming a control in a table cell disarms it. Themaincontrol is always visible;.page__meta-item > .confirm-costkeeps the refactor(a11y): scope ConfirmCost to its own control, not to the whole scope #112 disarm behaviour, and a geometry test pins it..char__locationuntouched — it is shared with the admin members drawer.src/core/untouched. Pure logic → vitest, geometry → Playwright only.Two deliberate deviations from the plan
.log--roster. That name already belongs to the payouts participant table (payouts/[id]/page.tsx,globals.css), so it shipped as.log--manifest. Following the plan literally would have coupled two unrelated tables.< 0.5to< 0.6. Not a threshold nudged to fit noise:unlinkis gated oncharacters.length > 1with noisMainexclusion, so a ten-character seed structurally renders it on main's row where the old one-character seed could not. The load-bearingmaxScrollLeftgate was untouched.Where to look first
src/app/account/page.tsx— the twoshowStatusColumngates. The entire a11y argument rests on them staying strict complements: when the column is absent, the per-row text must appear, and vice versa.e2e/account.spec.ts— the fold gate's.char__locationprecondition. This is what separates it from feat(account): collapse the character manifest to one status chip per row #167's vacuous band assertion; without it the gate would pass on rows that render no location at all.src/app/globals.css—.page__meta-item > .confirm-costworks only becauseConfirmArmScoperenders no DOM element. A comment records that dependency; if that ever changes, this rule silently stops applying.Verification
All seven gates on the final commit:
typecheck,lint,format:check,build,check-node-version.sh,npm test(1247/1247 across 81 files),npm run test:e2e(271/271).Each new gate was mutation-proved rather than merely observed green — the fold gate was run against an impossible target, the horizontal gate against a threshold of 1, and the caption test with its production branch deleted. All went red as required. That discipline is the direct response to #167's test passing for the wrong reason.