feat(account): reclaim fold space in the crew manifest (density round 3) - #174
Merged
Conversation
…floor
.table-notes used to cancel .table-note's own bottom margin for the last
child; deleting that container without a replacement left every drawer
row with s-4 of dead space under a row whose whole point is minimal
height. Adds .drawer-row .table-note { margin-bottom: 0 } and re-measures
the sub-row (37px at 1440/1280, 57px at 390, not the flat 53px the prior
comment claimed).
Waits on document.fonts.ready before the fold measurement, which removed
the run-to-run variance that made FAULTED_FOLD_TARGETS unraisable; raises
the floor to the now-stable observed minimum (7/6/5, five consecutive
runs each viewport) per Step 11's instruction to raise rather than leave
a floor below the truth.
Also re-indents two comment bodies whose opening `{/*` moved during the
prior commit's reindent but whose body lines did not follow, and updates
.table-note's docblock now that it also renders inside a table row.
The branch was cut before round 2 landed and #173 reworked the same faulted-crew layout. Rebasing onto it exposed three real interactions, none of which the pre-rebase measurements could have seen. #173's 320px gate counted `re-authorize` links across the whole manifest and asserted 1. Task 3 moved the remedy prose inside the manifest, and each contacts-faulted row's `ContactRemedy` carries its own link when the token is valid — so the honest count is 3. Scoped the existing assertion to `.status-line`, which is the cell the width measurement is of, and pinned the other two on the drawer rows rather than dropping them: a change that stops rendering them should fail here, not quietly loosen the line above. The 320px width budget itself is unaffected — a spanning sub-row adds no horizontal cost, and the gate still passes. Re-measured the fold on the new base: 7/6/5, identical across three runs and unchanged from the pre-rebase measurement. #173's -17px per faulted state is real but two faulted rows do not add up to a 63px character row, so the floor stands. Re-measured the sub-row: a flat 37px at 1440, 1280 and 390 alike, where it was 37/37/57 before. At 390 the table sits in a horizontal scroller, and #173's wider table gives the sub-row enough span that the prose stops wrapping. Corrected the comment, which claimed the wrap it no longer has.
The sub-row's prose lost its `{name}:` prefix when it moved under the
character it describes, correct for sighted users since the row above
already carries the name — but the table has no `<th scope="row">`, so a
screen reader in table mode gets a spanning cell naming no one, and in
links mode gets N identical "re-authorize" links. Restore the name as a
visually-hidden prefix and pin it with a Playwright assertion.
The faulted fold test already waited on `document.fonts.ready` before measuring row geometry (a font swap moves the row height the gate counts against the fold); the healthy fold test did not, leaving its 390x844 floor of 4 unmeasured against the actual layout. Adding the wait and measuring 3 runs at each viewport gives a stable 8/6/5 with no run-to-run variance, so raise the 390x844 floor from 4 to 5.
…leted block The "prose out of the table cells" bullet still cited the contactRemedies block below the Scroller, which this branch deleted — as written it read as a directive to revert the sub-row design it now describes elsewhere. Update the citation to the sub-row (page.tsx:830-853) and its measurement, keeping the real prohibition against folding the remedy back into the cell intact.
…lock The re-review caught one bad line range in the bullet rewritten last commit (830-853 ends mid-props; the block runs to 863). Checking it turned up finding 6, which the rewrite missed: it cited the deleted 566-586 block and closed by saying the prose "should stay exactly where it is" -- a directive against the change this branch just made, in the only tracked doc on this area now that the round-3 spec sits under gitignored docs/superpowers/. Finding 6 itself stays open: aria-describedby is still on the <td> and is still unreliably surfaced. Only its stale structure references are corrected, plus a note that the sub-row's new visually-hidden name prefix makes the character recoverable when the description is dropped -- which softens the finding's cost but does not close it.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 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 ignored due to path filters (1)
📒 Files selected for processing (6)
Comment |
guarzo
enabled auto-merge (squash)
August 7, 2026 22:59
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.
Pull request
Round 3 of the crew manifest density work, following #169. Success is measured as how many character rows clear the fold, not how many lines a cell renders.
What changed and why
1. The healthy verdict became a green chip with a count (
9a996f5). Previously every healthy row carried its own indicator; one chip saying how many characters are fine replaces N identical per-row marks.2. The manifest opts out of the page's narrow prose measure (
0b1fe84). The page caps content at a reading width, which is right for prose and wrong for a six-column table. The manifest now takes the wider region.3. Per-character remediation prose moved into a spanning sub-row (
e0d9c22,b73fd4e). It used to sit in a.table-notesblock below the table, prefixed with the character's name because it was far from the row it described. It now renders as atr.drawer-rowdirectly under its character, so adjacency replaces the prefix.The alternative — folding the remedy into the name cell — stays ruled out, and the code comment records why: it forces a viewport-dependent row height that is unreachable at 390px.
4. The row-hover tint no longer paints over drawer rows (⚠️ This fixes a second surface. The
f2527c7)..log/tr.drawer-rowrules are shared with the admin members drawer (disclosure.tsx), which had the same bug and is now fixed too. That is a deliberate behaviour change outside/account, not an accident.5. Reconciliation with #173 (
d57145e). This branch was cut before round 2 landed and was rebased onto #173, which had independently reworked the faulted-crew remedy layout. Details under Flags.6. Accessibility and doc fixes from final review (
161adfb,aaf364d,e625818,580a08f). Details under Flags.Measured results
Characters clearing the fold, three runs, zero variance:
The sub-row is a flat ~37px at all three viewports. Both 320px forced-horizontal-scroll gates pass with sub-rows present, so a spanning sub-row adds no horizontal cost.
What CI cannot check
The sub-row's accessible name. The manifest has no
<th scope="row">— every header isscope="col"— so when adjacency made the visible{name}:prefix redundant and it was dropped, no assistive-tech path could recover which character the prose belonged to. Table mode announced a 4-column-spanning cell with no character in it; links mode got N identical "re-authorize" links. Fixed with a.visually-hiddenname prefix (page.tsx:845), matching the idiom already used byConfirmCostand.facts__lead.That fix is claimed to cost zero pixels because
.visually-hiddenisposition: absolute. Rather than assert that, the new assertions were placed inside the 320px geometry test ahead of itspinGeometrycall, so that test proves it. Mutation-checked: deleting the span fails the name assertion withReceived: 0, and the faulted fold floors held at 7/6/5.Why the healthy fold floor moved. At 390px the healthy floor was 4 while the strictly harder faulted floor was 5 — incoherent, and it meant the healthy gate pinned nothing. The cause was not a stale number: the healthy fold test was missing the
document.fonts.readywait its faulted sibling already had, i.e. the same font-loading race that caused measurement variance earlier in this work. With the wait added, three repeats per viewport gave 8/6/5 at zero variance.That number is physically coherent rather than an artifact of the new wait: row ordering puts the faulted alts late in the list, so their sub-rows fall below the fold at 1280 and 390 and cost nothing, while at 1440 one lands above it and costs exactly one row. That is precisely the 8/6/5-vs-7/6/5 pattern observed.
Not re-attempted: putting character name and location on one line. That was implemented and rejected on measurement in an earlier round (at 320px a flex row forced 413px of horizontal scroll against a 286px scroll region, vs ~146px stacked). The horizontal-cost test pins it.
Deploy notes
None. No migration, no new or renamed secret, no
fly.tomlchange.Flags
Rebase interactions with #173. This branch was rebased onto #173, which reworked the same faulted-crew layout. Three things surfaced, all resolved:
re-authorizelink to 3. Resolved by scoping to.status-lineand pinning the twotr.drawer-rowlinks separately. This modifies an assertion test(account): gate the faulted crew's worst case, and stop paying 88px for it #173 landed one PR earlier — reviewer independently re-derived the arithmetic (1 status-line + 2 drawer-row = the old unscoped 3) and judged it re-expressed and strengthened, not weakened. Worth a second look from whoever owns test(account): gate the faulted crew's worst case, and stop paying 88px for it #173.A design call made mid-rebase, not in the plan. The "make main" consequence notes did not follow the remedy prose into sub-rows; they stay below the table. Reasoning is written into the code: a remedy marks a currently-broken row where adjacency is the point, while these notes describe what a control would do on a row that is fine, and up to nine can apply at once — nine sub-rows would spend the fold budget this round exists to reclaim. Deliberately conservative, but it is an asymmetry a reviewer may want to overturn.
A tracked doc described deleted code — twice.
docs/design-sweep/audit-account.mdstill described the pre-move design and read as a directive to revert this branch. This matters because the round-3 spec authorizing the move lives underdocs/superpowers/, which is gitignored, so that audit file is the only tracked document on this area. Both places are fixed (e625818,580a08f).Left open deliberately: finding 6 in that same audit doc —
aria-describedbyon a<td>is still unreliably surfaced by real screen readers. The new name prefix softens its cost but does not close it. Only its stale structure references were corrected.Latent bug, not fixed here (out of scope): the
unlinkcontrol renders on the main character's row, because it is gated only onview.characters.length > 1with noisMainexclusion.Known-slack pins: healthy fold floors now sit at zero margin over the faulted ones at 1280 and 390 (6/6, 5/5). That is where cross-machine font drift would surface first.