feat(account): owner walkthrough session 3 — the /account block - #186
Merged
Conversation
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`.
📝 WalkthroughWalkthroughThe account manifest now uses per-character action drawers. It adds location elision for matching alternate characters, updates responsive styling, and expands E2E coverage for drawer behavior, row composition, and layout boundaries. ChangesAccount manifest drawers
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccountPage
participant CharacterRow
participant ActionsToggle
participant ActionsDrawer
AccountPage->>CharacterRow: render character cells and conditional actions
CharacterRow->>ActionsToggle: expose character-specific accessible control
ActionsToggle->>CharacterRow: change disclosure state
CharacterRow->>ActionsDrawer: lazily mount actions after opening
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Comment |
guarzo
enabled auto-merge (squash)
August 8, 2026 19:46
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@e2e/account.spec.ts`:
- Around line 1855-1857: Replace the broad manifest row count assertion in the
account test with an assertion targeting the solo character row’s identifying
content, so the test fails when only the empty-state row is rendered. Keep the
existing action-button and drawer-row assertions unchanged.
- Around line 1963-1980: Strengthen the assertion in the test “main with no
location reading elides nothing” by verifying that the single rendered
.char__location row belongs to the alt character and contains “Home Astrahus”.
Retain the count check only if useful, and use the existing character-specific
selectors or manifest helpers to distinguish the alt row from the main row.
- Around line 710-713: Update the unlink locator in the walkthrough after
opening the Pilot Alt actions drawer to use the exact character-specific
accessible name “unlink Pilot Alt” with exact matching, so it continues
targeting the intended control if additional unlink actions appear.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77641b5c-bfc6-49ec-bc72-f3042cb0de8b
📒 Files selected for processing (4)
e2e/account.spec.tssrc/app/account/character-row.tsxsrc/app/account/page.tsxsrc/app/globals.css
Comment on lines
+710
to
+713
| // Walkthrough 3.2: unlink lives in the alt's actions drawer now, mounted | ||
| // only once that drawer opens. | ||
| await altRow.getByRole("button", { name: "Pilot Alt actions" }).click(); | ||
| const unlink = page.getByRole("button", { name: "unlink" }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the test around line 713 and its seed, plus every bare "unlink" locator in the suite.
sed -n '660,740p' e2e/account.spec.ts
rg -n 'name: "unlink"' e2e/Repository: guarzo/authGD
Length of output: 4024
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target and sibling tests ---'
sed -n '320,445p' e2e/account.spec.ts
printf '%s\n' '--- target test ---'
sed -n '695,730p' e2e/account.spec.ts
printf '%s\n' '--- unlink references and accessible-name candidates ---'
rg -n -C 3 'unlink|Discord|actions' --glob '*.{ts,tsx,js,jsx}' src e2eRepository: guarzo/authGD
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- account fixture definition ---'
rg -n -C 8 'function seedMember|const seedMember|seedMember\s*=' e2e
printf '%s\n' '--- character unlink implementation ---'
rg -n -C 8 'restName|confirm unlink|unlink Pilot|unlinkAction|character.*unlink' src/app --glob '*.{ts,tsx}'
printf '%s\n' '--- account Discord unlink implementation ---'
rg -n -C 6 'unlink Discord|Discord.*unlink|discord.*unlink' src/app --glob '*.{ts,tsx}'Repository: guarzo/authGD
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- complete seedMember setup ---'
sed -n '21,105p' e2e/helpers.ts
printf '%s\n' '--- account Discord conditional and character drawer ---'
sed -n '350,440p' src/app/account/page.tsx
sed -n '748,812p' src/app/account/page.tsx
printf '%s\n' '--- read-only name-resolution check ---'
python3 - <<'PY'
from pathlib import Path
test = Path("e2e/account.spec.ts").read_text()
helpers = Path("e2e/helpers.ts").read_text()
page = Path("src/app/account/page.tsx").read_text()
target = test[test.index('test("unlinking a character that already left'):test.index('/**', test.index('test("unlinking a character that already left'))]
fixture = helpers[helpers.index("export async function seedMember"):helpers.index("\n}", helpers.index("export async function seedMember")) + 2]
print("target uses seedMember without discord options:", 'discord' not in target.lower())
print("seedMember fixture contains discordLink insertion:", "discordLink" in fixture)
print("Discord unlink is conditional on view.discordLinked:", "view.discordLinked" in page)
print("character rest name template present:", "restName={`unlink ${c.name}`}" in page)
print("character confirm name template present:", "confirmName={`confirm unlink ${c.name}`}" in page)
names_at_rest = ["unlink Pilot Alt"]
names_armed = ["confirm unlink Pilot Alt"]
for label, names in [("rest", names_at_rest), ("armed", names_armed)]:
matches = [n for n in names if "unlink".casefold() in n.casefold()]
print(f"{label} bare-locator matches: {matches}")
PYRepository: guarzo/authGD
Length of output: 11827
Use the exact character-specific locator at e2e/account.spec.ts:713.
seedMember leaves Discord unlinked, so "unlink" is currently unique. Use page.getByRole("button", { name: "unlink Pilot Alt", exact: true }) to target the intended control if the fixture gains additional unlink actions.
🤖 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 `@e2e/account.spec.ts` around lines 710 - 713, Update the unlink locator in the
walkthrough after opening the Pilot Alt actions drawer to use the exact
character-specific accessible name “unlink Pilot Alt” with exact matching, so it
continues targeting the intended control if additional unlink actions appear.
Comment on lines
+1855
to
+1857
| await expect(manifest(page).locator("tbody tr")).toHaveCount(1); | ||
| await expect(page.getByRole("button", { name: "Solo Pilot actions" })).toHaveCount(0); | ||
| await expect(manifest(page).locator("tr.drawer-row")).toHaveCount(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Line 1855 passes against the empty state.
An account with zero characters renders one <tr> holding the log__empty cell. toHaveCount(1) therefore succeeds whether the solo character row rendered or the seed silently produced nothing. Assert the content that proves the row is the character's.
💚 Proposed fix
- await expect(manifest(page).locator("tbody tr")).toHaveCount(1);
+ const rows = manifest(page).locator("tbody tr");
+ await expect(rows).toHaveCount(1);
+ await expect(rows.first()).toContainText("Solo Pilot");
+ await expect(manifest(page).locator(".log__empty")).toHaveCount(0);
await expect(page.getByRole("button", { name: "Solo Pilot actions" })).toHaveCount(0);
await expect(manifest(page).locator("tr.drawer-row")).toHaveCount(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(manifest(page).locator("tbody tr")).toHaveCount(1); | |
| await expect(page.getByRole("button", { name: "Solo Pilot actions" })).toHaveCount(0); | |
| await expect(manifest(page).locator("tr.drawer-row")).toHaveCount(0); | |
| const rows = manifest(page).locator("tbody tr"); | |
| await expect(rows).toHaveCount(1); | |
| await expect(rows.first()).toContainText("Solo Pilot"); | |
| await expect(manifest(page).locator(".log__empty")).toHaveCount(0); | |
| await expect(page.getByRole("button", { name: "Solo Pilot actions" })).toHaveCount(0); | |
| await expect(manifest(page).locator("tr.drawer-row")).toHaveCount(0); |
🤖 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 `@e2e/account.spec.ts` around lines 1855 - 1857, Replace the broad manifest row
count assertion in the account test with an assertion targeting the solo
character row’s identifying content, so the test fails when only the empty-state
row is rendered. Keep the existing action-button and drawer-row assertions
unchanged.
Source: Path instructions
Comment on lines
+1963
to
+1980
| test("main with no location reading elides nothing", async ({ page, context }) => { | ||
| const acc = await seedMember(db, { | ||
| name: "Main Pilot", | ||
| tier: "alumni", | ||
| alts: ["Alt Pilot"], | ||
| }); | ||
| await markTokensHealthy(acc.id); | ||
| const crew = await db.select().from(character).where(eq(character.accountId, acc.id)); | ||
| // Only the alt is placed; main's `location` stays `{ kind: "none" }`, so | ||
| // `locationKey(mainCharacter.location)` is null and `mainLocationKey !== | ||
| // null` keeps the alt's own line rather than comparing it to nothing. | ||
| const alt = crew.find((c) => c.id !== acc.mainCharacterId)!; | ||
| await placeCrew([alt.id], 30000142, "Home Astrahus"); | ||
| await context.addCookies([await sessionCookieFor(db, acc.id)]); | ||
| await page.goto("/account"); | ||
|
|
||
| await expect(manifest(page).locator(".char__location")).toHaveCount(1); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Line 1979 does not prove which row kept its location.
The test claims main has no location reading and the alt keeps its own. toHaveCount(1) holds equally if main rendered a location and the alt was elided — the inverse of the behaviour under test. Pin the owner and the text.
💚 Proposed fix
- await expect(manifest(page).locator(".char__location")).toHaveCount(1);
+ const located = manifest(page).locator("tbody tr:not(.drawer-row)", {
+ hasText: "Alt Pilot",
+ });
+ await expect(located.locator(".char__location")).toHaveText(
+ "J30000142 — Home Astrahus",
+ );
+ await expect(manifest(page).locator(".char__location")).toHaveCount(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("main with no location reading elides nothing", async ({ page, context }) => { | |
| const acc = await seedMember(db, { | |
| name: "Main Pilot", | |
| tier: "alumni", | |
| alts: ["Alt Pilot"], | |
| }); | |
| await markTokensHealthy(acc.id); | |
| const crew = await db.select().from(character).where(eq(character.accountId, acc.id)); | |
| // Only the alt is placed; main's `location` stays `{ kind: "none" }`, so | |
| // `locationKey(mainCharacter.location)` is null and `mainLocationKey !== | |
| // null` keeps the alt's own line rather than comparing it to nothing. | |
| const alt = crew.find((c) => c.id !== acc.mainCharacterId)!; | |
| await placeCrew([alt.id], 30000142, "Home Astrahus"); | |
| await context.addCookies([await sessionCookieFor(db, acc.id)]); | |
| await page.goto("/account"); | |
| await expect(manifest(page).locator(".char__location")).toHaveCount(1); | |
| }); | |
| test("main with no location reading elides nothing", async ({ page, context }) => { | |
| const acc = await seedMember(db, { | |
| name: "Main Pilot", | |
| tier: "alumni", | |
| alts: ["Alt Pilot"], | |
| }); | |
| await markTokensHealthy(acc.id); | |
| const crew = await db.select().from(character).where(eq(character.accountId, acc.id)); | |
| // Only the alt is placed; main's `location` stays `{ kind: "none" }`, so | |
| // `locationKey(mainCharacter.location)` is null and `mainLocationKey !== | |
| // null` keeps the alt's own line rather than comparing it to nothing. | |
| const alt = crew.find((c) => c.id !== acc.mainCharacterId)!; | |
| await placeCrew([alt.id], 30000142, "Home Astrahus"); | |
| await context.addCookies([await sessionCookieFor(db, acc.id)]); | |
| await page.goto("/account"); | |
| const located = manifest(page).locator("tbody tr:not(.drawer-row)", { | |
| hasText: "Alt Pilot", | |
| }); | |
| await expect(located.locator(".char__location")).toHaveText( | |
| "J30000142 — Home Astrahus", | |
| ); | |
| await expect(manifest(page).locator(".char__location")).toHaveCount(1); | |
| }); |
🤖 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 `@e2e/account.spec.ts` around lines 1963 - 1980, Strengthen the assertion in
the test “main with no location reading elides nothing” by verifying that the
single rendered .char__location row belongs to the alt character and contains
“Home Astrahus”. Retain the count check only if useful, and use the existing
character-specific selectors or manifest helpers to distinguish the alt row from
the main row.
Source: Path instructions
This was referenced Aug 9, 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.
Session 3 of
docs/design-walkthrough.md— the/accountblock. Four findings, all in one commit because 3.3 is downstream of 3.2 and 3.1b's deletion is what 3.1's test re-anchoring needed.3.1 — the health strip no longer stops 287px short of the manifest
.page--narrow > :where(*)(0,1,0) caps direct children at60rem - 2 × --s-5 = 912px. The manifest opts out viafull-measure(0,2,0) and runs the viewport; the head row didn't, so the strip right-aligned to 912px while the table beneath it ran on — a measured 287px shortfall reading as a broken alignment. Fixed withfull-measureon.page__head(page.tsx:249).The e2e test that measured the narrow cap was anchored on
.page__lede, which 3.1b deletes. Re-anchored on.page__meta, and gave.page__metaan explicitmax-width: calc(var(--measure-page-narrow) - 2 * var(--s-5))so the assertion measures a stated intent rather than an inherited side effect of a rule two selectors away.3.1b — the lede is gone
"Membership, characters, and the state authGD is pushing out to standings, the map, and Discord." Deferred out of Session 0 only because it was the test anchor.
.page__ledeitself stays — eight other pages still use it.3.2 — R2 applied:
MAIN/UNLINKmove behind per-row disclosure40 permanent controls across 20 rows, in the two widest columns, for actions an owner takes rarely and destructively. Now one
actionstoggle per row opening a drawer.New client component
src/app/account/character-row.tsxrather than<Disclosure as="row">: that component puts the toggle in the row's first cell (disclosure.tsx:95-103), which on the admin table is the row's identity but here is the portrait. Reusing it would mean either reordering the row or displacing the identity a screen-reader user scans for first. The open/everOpen/hiddenmachinery mirrorsDisclosureexactly.Per R1 the drawer takes the 36px standalone grade, not the 28px in-row grade —
.drawer__controls .btn--quietbuys it back. Its padding matches bare.btnexactly soConfirmSubmit's armed class-swap to.btn.btn--dangerdoesn't resize the control under the pointer.On #108/#111/#112: that constraint bars reveal-on-arm reflow inside a
<td>— the cell widens, the armed button slides out from under the pointer,pointerLeavefires, the control disarms. It does not bar armed controls in a drawer, which is what made R2 implementable here. An armed control can't hide in a closed drawer either: closing blurs the button andonBlurdisarms (confirm-submit.tsx:428-433).A single-character row has neither action (
mainis gated on!isMain,unlinkon more than one character) and renders no toggle and no drawer at all, rather than a control that opens on nothing.3.3 — the reclaimed space
Actions cell 155px → 113px. Total forced horizontal scroll at 320px on a faulted crew 299px → 257px, against a 286px region — under one region width for the first time.
3.4 — location shown only when it differs from the main's
Elided when the row is not the main, the main has a comparable location, and both the text and the offline flag match. An online alt sitting where an offline main sits is not elided — the offline flag is part of the key. No-ops when there is no main or the main has no location line.
Verification
Measured on the pre-amend tree (
cdcad6b):npm run typecheck— cleannpm run lint— cleannpm run format:check— cleannpm test— 83 files, 1322 passednpm run test:e2e— 328 passed, 0 failuresnpm run build— succeeded./scripts/check-node-version.sh— OK (Dockerfile=24 .nvmrc=24 engines='>=24')code-reviewer— cleanmy:polish-core --fixthen applied one fix, comment text only: the.scroller:has(.drawer)docblock claimed "the class is shared with the audit, sync and account pages, and none of those hold a drawer" — true when written, false as of this PR. Rewritten to name both drawer-holding regions while preserving the argument for the:has()narrowing.format:check,typecheckandlintre-run clean after the amend;npm test,test:e2eandbuildwere not re-run, because comment text in a.cssfile cannot affect runtime — flagging that explicitly rather than implying a fresh green.Two
polish-coreitems reported and deliberately not applied:character-row.tsx:102-106—idsits on the<td>, whereDisclosureputs it on the inner<div>. Intentional and commented; a divergence worth a reviewer's eye.e2e/account.spec.ts:1900— one hardcoded[aria-label='Your characters']whereMANIFESTexists. (polish-reviewerclaimed evaluate callbacks can't take outer bindings; that's wrong — the same file already passesMANIFESTas an evaluate arg at lines 1561 and 1797. Left as-is only to keep the amend comment-only.)Test changes worth reading
The fold-count gate had a real correctness bug: hidden drawer rows have all-zero rects, so
bottom <= viewportHeightwas trivially true for them. Ten selectors gained:not(.drawer-row), fourteen gained:not(.drawer-row--actions), and the bounds were re-measured against the new layout (< 308→< 270;FAULTED_FOLD_TARGETS7/6/5 → 9/7/6;.char__locationcount 10 → 1). Four new tests cover the single-character no-toggle case, three-row data/actions/remedy composition, online-alt-vs-offline-main, and the mainless/no-location no-ops.For Session 4
#185 added a "Coordination with Session 3" note: 4.2 applies R2 to the same
UNLINKcontrol on the admin table and should adopt this pattern rather than invent a second one.character-row.tsxis that pattern — note in particular why it is not<Disclosure as="row">, which the admin table can reuse directly since its first column already is the row identity.Summary by CodeRabbit
New Features
Layout Improvements