fix(account): give the drawer's unlink an edge, and hold its cost until it's armed - #192
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 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 (4)
Comment |
… with nothing in it (#199) * feat(account): let escape back out of the drawer, and drop the column with nothing in it A critique pass over the crew manifest's actions drawer raised eleven claims. Five were retired by measuring `/account` instead of reasoning from the admin pages it resembles, and one was dropped by the owner. Six survived. Escape now closes an open drawer and returns focus to the toggle. It layers through `e.defaultPrevented`: `ConfirmSubmit`'s own handler calls `preventDefault()` without `stopPropagation()`, so the first press disarms an armed unlink and the second closes — collapsing both into one press would take the panel away mid-confirmation. Bound to the toggle as well as the panel, because opening moves no focus. The refocus is not a nicety: closing sets `hidden` on the row holding the focused element, so without it focus is stranded in a hidden subtree, which is worse than having no Escape at all. ACTIONS is now an exception column like STATUS. A single-character account's row has neither `main` nor `unlink`, so the header and `<col>` were reporting a column that is empty in every row (walkthrough ruling R4). The predicate is the per-row one hoisted — `!isMain || characters.length > 1`, NOT `length > 1`: `applyNoMainRule` clears `mainCharacterId` when a member unlinks their main, so a lone survivor keeps a live `make main` and must keep its column. Five touch points, including `CharacterRow`'s own `<td>` — nothing throws when body cells outnumber header cells. `make main` now says the sync it queued. `setMainCharacter` ends in `enqueueSync`, and the membership job derives the account's tier from the MAIN character's alliance, so the press can change the account's tier and always re-pushes its Discord roles — none of it visible on this render. Said in the confirmation rather than as a `ConfirmCost`, following `"wake"`: this is one press with no armed state to reveal prose on, and permanent panel prose is what that panel's own reveal rationale argues against. Three comments were wrong and are corrected. `confirm-notice.tsx` claimed a redirect resets every `useState` below it; its own caller list is the counter-example. The manifest's rows are keyed, so React moves the existing row instance rather than remounting it, and the `useId` its open state is matched on is stable for an instance's lifetime — the drawer measurably survives, and on `setMainAction` follows the character to its new position as main. The rule is empirical in both directions now. `page.tsx` and `globals.css` both stated `hasActions` as `characters.length > 1`. DESIGN.md said `.btn--quiet` carries no size; it carries the 28px base, which `.manifest-panel__controls`' 36px then beats on specificity and source order. The retired claims are the reason this diff is smaller than the critique. The P1 — an arm outliving its drawer — does not reproduce on any of the four close paths, mouse or touch; `ConfirmSubmit`'s `onBlur` covers it, and a drafted `useConfirmArm` export was reverted rather than shipped unused onto a component two admin pages also mount. A parameterized e2e guard pins the outcome at the drawer level instead, since the guarantee lives two components away. One test was passing vacuously: `.visually-hidden` is a 1px `clip-path: inset(50%)` box, which still has a non-empty bounding box, so `toBeVisible()` never caught #192's switch to reveal-on-arm. It now asserts the class. * test(account): the lone-main seed now has three header cells, not four `showActionsColumn` elides the ACTIONS column when no row can act, and `seedMember` gives exactly one character, who is the main: no `make main` (gated on `!isMain`), no `unlink` (gated on crew size). This test asserted the pre-elision count and was the one spec the local run before the PR did not cover. Renamed to say what it now checks, and paired the count with an explicit `Actions` columnheader `toHaveCount(0)`, so a future column arriving under a different name can't quietly restore the four.
1 participant
The owner's production screenshot of
/account: every open actions drawer is one faint word over a two-line paragraph about an action nobody is taking, and on the main character's row — wheremake mainis absent — UNLINK is the only thing in the panel, borderless--ink-fainttext on--hull, reading as a column label rather than a control.Two changes, both scoped to the manifest's actions panel.
UNLINK gets a resting edge.
.manifest-panel__controls .btn--quiet { border-color: var(--rule-strong) }— the same resting-affordance buy-back.inline-edit .btn--quietalready documents (walkthrough finding 1.1), for the same reason. Colour is untouched: UNLINK stays--ink-faintat rest and still takes--signal-badon row hover and focus, so it remains the quiet destructive grade PRODUCT.md principle 4 asks for. It is now a quiet control that looks like a control.The cost sentence waits for the arm.
ConfirmCostgoes fromvisibility="visible"back to the default"reveal". Round 3 ruled"reveal"out on the grounds that this sits in a<td>and #108/#111/#112 found that revealing inside a cell widens it, slides the armed button out from under a stationary pointer, and disarms it onpointerLeave. That failure is horizontal growth, and it does not reach this panel:.manifest-panel__controls .confirm-costalready setsflex-basis: 100%inside analign-items: flex-startrow, so the reveal claims a fresh line and the panel grows downward from a fixed top edge.Measured, not assumed.
arming unlink inside the actions drawer does not move the buttonruns at 1280px, 800px and 390px and asserts the button's whole box is byte-identical across the arm, that the armed control is still armed afterward (a Playwright click leaves the pointer where it pressed, so a button that moved would have disarmed itself before the assertion runs), and that the panel grew — taller only, samey, same width.Three widths because #112's disarm only appeared between roughly 641px and 851px. Mutation-tested by deleting
flex-basis: 100%: the 1280px case fails (panel height unchanged — the sentence sits beside the buttons), 800px and 390px still pass because the panel wraps on its own there. A single-width gate is how this shipped the first time."hidden"is still wrong for the reason round 3 gave: it would leave the sighted member who opened the panel never reading the cost at all (R4). Revealing on arm shows it to everyone about to need it.Comments corrected where this diff invalidated them: the
visibility="visible"justification at the call site, the.confirm-costrule's "the line it claims is permanent" paragraph, andConfirmCost's own docblock, which listed the character unlink as a"visible"caller and stated flatly that revealing on arm is never a good idea in a table.Verification —
npx playwright test e2e/account.spec.ts68 passed ·npm test83 files / 1324 passed ·npm run test:e2e353 passed ·npm run typecheckclean ·npm run format:checkclean ·npm run buildclean.