fix(ui): rebuild the crew drawer, and give failed actions somewhere to land - #59
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 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 (15)
📝 WalkthroughWalkthroughThe PR adds shared two-step confirmation and table-row disclosure components. Account and admin actions now handle stale or unauthorized operations with redirects. Admin tables show expanded account state. Audit, sync, login, and error pages receive timestamp, accessibility, and error-state updates. ChangesAccount and administration updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant AccountPage
participant ConfirmSubmit
participant ServerAction
participant Database
Operator->>AccountPage: click destructive action
AccountPage->>ConfirmSubmit: arm control
Operator->>ConfirmSubmit: click confirmation
ConfirmSubmit->>ServerAction: submit form
ServerAction->>Database: update account or character
Database-->>ServerAction: success or stale state
ServerAction-->>AccountPage: redirect or revalidated view
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 155-173: In the unlink row stability assertion, validate that both
boundingBox results from unlink and confirm are non-null before comparing their
widths. Keep the existing width equality check, but ensure it only runs after
the boxes have been asserted to exist.
In `@src/app/_components/confirm-submit.tsx`:
- Around line 122-149: Update the unarmed branch of the click handler in the
confirm control to focus the clicked button after calling ctx.arm(id), ensuring
pointer users can subsequently dismiss it with Escape. Preserve the existing
preventDefault behavior and armed-branch disarm logic.
In `@src/app/admin/accounts/page.tsx`:
- Around line 52-55: Update the table’s colgroup span to use COLUMN_COUNT - 1
instead of the hard-coded 9, keeping the fit-column calculation synchronized
with the centralized column count.
In `@src/app/login/page.tsx`:
- Around line 7-18: Update LoginPage’s error-message lookup to verify error is
an own key of ERRORS before reading ERRORS[error]. Treat inherited keys such as
toString, constructor, and __proto__ as absent so the alert receives no message
for unsupported values.
🪄 Autofix (Beta)
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: 8c8f92e6-d3c5-45d1-b6b2-17b71e3e5c68
📒 Files selected for processing (15)
e2e/account.spec.tse2e/admin.spec.tssrc/app/_components/confirm-submit.tsxsrc/app/_components/row-disclosure.tsxsrc/app/_components/unlink-button.tsxsrc/app/_components/utc-time.tssrc/app/account/actions.tssrc/app/account/page.tsxsrc/app/admin/accounts/actions.tssrc/app/admin/accounts/page.tsxsrc/app/admin/audit/page.tsxsrc/app/admin/sync/page.tsxsrc/app/error.tsxsrc/app/globals.csssrc/app/login/page.tsx
💤 Files with no reviewable changes (1)
- src/app/_components/unlink-button.tsx
Comment on lines
+155
to
+173
| const restBox = await unlink.boundingBox(); | ||
|
|
||
| // A server action is a POST to the current route. Counting them is the only | ||
| // assertion that actually proves the first click never reached the server — | ||
| // "the row is still visible" would also pass in the window before an | ||
| // in-flight unlink came back and re-rendered without it. | ||
| let posts = 0; | ||
| page.on("request", (r) => { | ||
| if (r.method() === "POST") posts += 1; | ||
| }); | ||
|
|
||
| await unlink.click(); | ||
| const confirm = altRow.getByRole("button", { name: /^confirm unlink/ }); | ||
| await expect(confirm).toBeVisible(); | ||
| expect(posts).toBe(0); | ||
|
|
||
| // The label swap alone must not jitter the row. | ||
| const armedBox = await confirm.boundingBox(); | ||
| expect(armedBox?.width).toBe(restBox?.width); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the bounding boxes exist, or the width check can pass vacuously.
boundingBox() returns null for a hidden element. If both calls return null, expect(armedBox?.width).toBe(restBox?.width) compares undefined to undefined and passes without measuring anything. Assert the boxes first.
♻️ Proposed fix
- const restBox = await unlink.boundingBox();
+ const restBox = await unlink.boundingBox();
+ expect(restBox).not.toBeNull();
@@
const armedBox = await confirm.boundingBox();
- expect(armedBox?.width).toBe(restBox?.width);
+ expect(armedBox).not.toBeNull();
+ expect(armedBox!.width).toBe(restBox!.width);📝 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
| const restBox = await unlink.boundingBox(); | |
| // A server action is a POST to the current route. Counting them is the only | |
| // assertion that actually proves the first click never reached the server — | |
| // "the row is still visible" would also pass in the window before an | |
| // in-flight unlink came back and re-rendered without it. | |
| let posts = 0; | |
| page.on("request", (r) => { | |
| if (r.method() === "POST") posts += 1; | |
| }); | |
| await unlink.click(); | |
| const confirm = altRow.getByRole("button", { name: /^confirm unlink/ }); | |
| await expect(confirm).toBeVisible(); | |
| expect(posts).toBe(0); | |
| // The label swap alone must not jitter the row. | |
| const armedBox = await confirm.boundingBox(); | |
| expect(armedBox?.width).toBe(restBox?.width); | |
| const restBox = await unlink.boundingBox(); | |
| expect(restBox).not.toBeNull(); | |
| // A server action is a POST to the current route. Counting them is the only | |
| // assertion that actually proves the first click never reached the server — | |
| // "the row is still visible" would also pass in the window before an | |
| // in-flight unlink came back and re-rendered without it. | |
| let posts = 0; | |
| page.on("request", (r) => { | |
| if (r.method() === "POST") posts += 1; | |
| }); | |
| await unlink.click(); | |
| const confirm = altRow.getByRole("button", { name: /^confirm unlink/ }); | |
| await expect(confirm).toBeVisible(); | |
| expect(posts).toBe(0); | |
| // The label swap alone must not jitter the row. | |
| const armedBox = await confirm.boundingBox(); | |
| expect(armedBox).not.toBeNull(); | |
| expect(armedBox!.width).toBe(restBox!.width); |
🤖 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 155 - 173, In the unlink row stability
assertion, validate that both boundingBox results from unlink and confirm are
non-null before comparing their widths. Keep the existing width equality check,
but ensure it only runs after the boxes have been asserted to exist.
Comment on lines
+122
to
+149
| onClick={(e) => { | ||
| if (!armed) { | ||
| // The first click arms rather than fires: never let it reach the | ||
| // server. | ||
| e.preventDefault(); | ||
| ctx.arm(id); | ||
| } else { | ||
| // Let the click proceed as an ordinary submit. Disarming here is | ||
| // just tidy-up for the (rare) case the action doesn't navigate or | ||
| // revalidate this control away. | ||
| ctx.disarm(); | ||
| } | ||
| }} | ||
| onBlur={() => { | ||
| // Tabbing or clicking away is as clear a "not that one" as Escape, and | ||
| // it means an armed control never outlives the member's attention on | ||
| // it. Guarded on `armed` so a blur from a different row's button can't | ||
| // disarm whatever the scope handed the arm to next. | ||
| if (armed) ctx.disarm(); | ||
| }} | ||
| onKeyDown={(e) => { | ||
| // A member who armed the wrong row must not have to reload to get out | ||
| // of it. | ||
| if (armed && e.key === "Escape") { | ||
| e.preventDefault(); | ||
| ctx.disarm(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Focus the control when it arms so Escape can reach it.
onKeyDown only fires while the button has DOM focus. Safari and Firefox on macOS do not focus a <button> on mouse click by default, so a pointer user who arms the control never gives it focus. Escape then does nothing, and the only exit is the 4-second revert. Move focus explicitly when arming.
Note that the e2e coverage in e2e/admin.spec.ts and e2e/account.spec.ts uses confirm.press("Escape"), which focuses the element first, so the tests cannot detect this gap.
🐛 Proposed fix to focus on arm
if (!armed) {
// The first click arms rather than fires: never let it reach the
// server.
e.preventDefault();
+ // Focus explicitly: Safari and Firefox do not focus a button on
+ // click, and `onKeyDown` (Escape) needs focus to fire.
+ e.currentTarget.focus();
ctx.arm(id);
} else {📝 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
| onClick={(e) => { | |
| if (!armed) { | |
| // The first click arms rather than fires: never let it reach the | |
| // server. | |
| e.preventDefault(); | |
| ctx.arm(id); | |
| } else { | |
| // Let the click proceed as an ordinary submit. Disarming here is | |
| // just tidy-up for the (rare) case the action doesn't navigate or | |
| // revalidate this control away. | |
| ctx.disarm(); | |
| } | |
| }} | |
| onBlur={() => { | |
| // Tabbing or clicking away is as clear a "not that one" as Escape, and | |
| // it means an armed control never outlives the member's attention on | |
| // it. Guarded on `armed` so a blur from a different row's button can't | |
| // disarm whatever the scope handed the arm to next. | |
| if (armed) ctx.disarm(); | |
| }} | |
| onKeyDown={(e) => { | |
| // A member who armed the wrong row must not have to reload to get out | |
| // of it. | |
| if (armed && e.key === "Escape") { | |
| e.preventDefault(); | |
| ctx.disarm(); | |
| } | |
| }} | |
| onClick={(e) => { | |
| if (!armed) { | |
| // The first click arms rather than fires: never let it reach the | |
| // server. | |
| e.preventDefault(); | |
| // Focus explicitly: Safari and Firefox do not focus a button on | |
| // click, and `onKeyDown` (Escape) needs focus to fire. | |
| e.currentTarget.focus(); | |
| ctx.arm(id); | |
| } else { | |
| // Let the click proceed as an ordinary submit. Disarming here is | |
| // just tidy-up for the (rare) case the action doesn't navigate or | |
| // revalidate this control away. | |
| ctx.disarm(); | |
| } | |
| }} | |
| onBlur={() => { | |
| // Tabbing or clicking away is as clear a "not that one" as Escape, and | |
| // it means an armed control never outlives the member's attention on | |
| // it. Guarded on `armed` so a blur from a different row's button can't | |
| // disarm whatever the scope handed the arm to next. | |
| if (armed) ctx.disarm(); | |
| }} | |
| onKeyDown={(e) => { | |
| // A member who armed the wrong row must not have to reload to get out | |
| // of it. | |
| if (armed && e.key === "Escape") { | |
| e.preventDefault(); | |
| ctx.disarm(); | |
| } | |
| }} |
🤖 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/_components/confirm-submit.tsx` around lines 122 - 149, Update the
unarmed branch of the click handler in the confirm control to focus the clicked
button after calling ctx.arm(id), ensuring pointer users can subsequently
dismiss it with Escape. Preserve the existing preventDefault behavior and
armed-branch disarm logic.
Comment on lines
+7
to
+18
| const ERRORS: Record<string, string> = { | ||
| oauth_denied: "EVE login was cancelled. Try again when ready.", | ||
| session_expired: "Your session ended. Sign in again to pick up where you left off.", | ||
| }; | ||
|
|
||
| export default async function LoginPage({ | ||
| searchParams, | ||
| }: { | ||
| searchParams: Promise<{ error?: string }>; | ||
| }) { | ||
| const { error } = await searchParams; | ||
| const message = error ? ERRORS[error] : undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i 'page.tsx|package.json|next' . | head -80
printf '%s\n' '--- login page ---'
cat -n src/app/login/page.tsx
printf '%s\n' '--- relevant usages ---'
rg -n -C 4 'ERRORS|searchParams|message|alert' src/app/login
printf '%s\n' '--- Next.js documentation candidates ---'
if [ -d node_modules/next/dist/docs ]; then
find node_modules/next/dist/docs -maxdepth 3 -type f | sort | head -120
else
printf '%s\n' 'node_modules/next/dist/docs is absent'
fiRepository: guarzo/authGD
Length of output: 11896
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package versions ---'
cat package.json
printf '%s\n' '--- Next.js searchParams guidance ---'
rg -n -C 5 'searchParams.*Promise|searchParams.*query|PageProps|query parameters' node_modules/next/dist/docs/01-app node_modules/next/dist/docs/01-app/03-api-reference | head -160
printf '%s\n' '--- runtime lookup probe ---'
node - <<'JS'
const ERRORS = {
oauth_denied: "EVE login was cancelled. Try again when ready.",
session_expired: "Your session ended. Sign in again to pick up where you left off.",
};
for (const error of ["oauth_denied", "unknown", "toString", "constructor", "__proto__"]) {
const direct = error ? ERRORS[error] : undefined;
const guarded =
error && Object.prototype.hasOwnProperty.call(ERRORS, error)
? ERRORS[error]
: undefined;
console.log(JSON.stringify({
error,
directType: typeof direct,
directTruthy: Boolean(direct),
guarded,
}));
}
JSRepository: guarzo/authGD
Length of output: 20134
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
const React = require("react");
const { renderToStaticMarkup } = require("react-dom/server");
const ERRORS = {
oauth_denied: "EVE login was cancelled. Try again when ready.",
session_expired: "Your session ended. Sign in again to pick up where you left off.",
};
for (const error of ["toString", "constructor", "__proto__"]) {
const message = ERRORS[error];
try {
const html = renderToStaticMarkup(
React.createElement(
"p",
{ role: "alert" },
message,
),
);
console.log(JSON.stringify({ error, result: "rendered", html }));
} catch (cause) {
console.log(JSON.stringify({
error,
result: "threw",
error: String(cause),
}));
}
}
JSRepository: guarzo/authGD
Length of output: 836
Reject inherited error keys before rendering the alert.
At line 18, ERRORS[error] treats toString and constructor as truthy messages, which renders an empty alert and emits a React warning. __proto__ can fail server rendering. Use an own-property check before reading ERRORS.
🤖 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/login/page.tsx` around lines 7 - 18, Update LoginPage’s error-message
lookup to verify error is an own key of ERRORS before reading ERRORS[error].
Treat inherited keys such as toString, constructor, and __proto__ as absent so
the alert receives no message for unsupported values.
…o land Answers a design critique of the admin and member pages. Three priorities, in the order they were agreed, plus the cheap consistency wins. The crew drawer was the worst of it: the nested table fought the row it lived in, and the controls sat below the data they acted on. It is now two sibling rows joined by a colSpan, with the controls above the crew table. The drawer row uses `hidden` rather than unmounting, so an admin's half-typed note survives a toggle. Server actions that threw on recoverable conditions now redirect with an error code the destination page renders — a stale character, a mid-flight demotion, a last-admin demotion, an expired session. What still throws is a genuine bug, and `error.tsx` catches it: digest only, never the message, which can carry a raw DB string. REVOKE, UNLINK and FREEZE arm on the first click and act on the second. One `ConfirmArmScope` per table holds a single armed id, so arming one control disarms any other; Escape, blur, and a four-second timer all revert it. Button width is reserved for the longer label up front — the swap was jittering the row, caught by an e2e geometry assertion rather than by eye. This supersedes the UNLINK-only confirm added in #57. Its blur and revert behaviours are folded into ConfirmSubmit, which adds the shared arm scope, Escape, and per-target accessible names, so unlink-button.tsx is deleted rather than left as a second mechanism. Also: "Your account" on both navs, since "Account" and the admin roster's "Accounts" read as one destination; gold off the audit FILTER, which is not the primary action on that page; scope="col" on the sortable headers; pendingLabel on the side-effecting drawer controls, but deliberately not on the tier buttons, where the label is the value being set; a render stamp on both admin lists, since force-dynamic means the page is only as fresh as the last load; and the closing illustration aligned left at a smaller size, so the page ends on the vertical it started on.
…umns Three places depended on the column count — the header row, the empty-state colSpan, and the colgroup span — and only two of them were derived. The third was a literal 9 that would have gone silently wrong the next time someone added a column. FIXED_COLUMNS now names the non-sortable columns in render order; the header maps over it and COLUMN_COUNT falls out of its length. Adding a column is one edit.
guarzo
force-pushed
the
worktree-design-critique-fixes
branch
from
August 4, 2026 03:53
f44e655 to
7796288
Compare
This was referenced Aug 4, 2026
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
…62) PR #59 put `restName="revoke admin for <identity>"` on REVOKE while resolving a rebase conflict, which was outside its scope. It was the right name — a row-per-account table makes the bare word "revoke" identical on every row, and `ConfirmSubmit`'s docblock already argues the case — but it was reached by noticing one gap next to a labelled neighbour, not by looking at the page. Looking at the page: REVOKE was the most visible gap, not the only one. Every control inside the drawer named its row already — tier, auto, wake, freeze, and the note field — and every control in the row itself did not. `grant`, `sync now` and `save note` were all bare, and nothing about them is less anonymous out of their row than "freeze" was. So REVOKE stays and the other three join it, which is the sweep rather than the accident. Each name leads with the visible label, so speech input still reaches the control by the word written on it (WCAG 2.5.3). One consequence worth stating: "save note for X" contains the note field's own "Note for X" as a substring, and `getByLabel` matches on a substring, so six existing assertions were quietly counting the pair. They are `exact` now. The nesting is fine for a screen reader — the two are a field and a button with different roles, the same way "freeze X" and "confirm freeze X" already coexist — but it is not fine for an assertion that means to be about the field. The test covering this grew from tier-and-cryo to all four swept controls and is renamed to say so. It fails closed: with an aria-label dropped, the exact-name lookup finds the bare visible text instead and the count assertion fails.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
Review caught the doc comment asserting "97px closed against 281.5px open". Neither number appears anywhere in the repo. The figure actually on record — kept by #59 as a tombstone at globals.css:1913 for the rule that used to unpin column 1 — is 279.5px of a 286px region. Cite that instead; the repo convention is that a comment's measured figures are reproducible. Also softens the vacuity guard's comment. It rules out a drawer that renders nothing, which is worth keeping, but it is not evidence of how much width the drawer's content demands: a cell spanning every column is wider than any single column by table structure alone.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
…mn (#66) * test(admin): assert an open drawer never widens the shared first column PR #59 moved the accounts row drawer out of the pinned name cell into its own full-width <tr>, and tested that the pin survives an open drawer and that the drawer's own cell is not itself pinned. Neither of those states the property the move existed for. Table columns are shared, so the drawer's `flex: 1 1 100%` crew group set a min-content width for column 1 on every row while it lived in the name cell: 97px closed against 281.5px open at 320px, which is how the pinned cell came to cover 98% of the scroll region. "An open row drawer keeps the pin" cannot catch a regression here — a pinned cell is wholly on screen at either width, so it passes just as happily at 281.5px. clearOfPin notices only indirectly, once a control at the far right happens to fall under the widened pin. The row measured is deliberately not the row being opened: "opening one row widens the column for every row" is the actual failure, and a neighbour's name cell is where it shows. Falsified by re-rendering the drawer inside the name cell, which fails it at 435.45px against 97px. * test(admin): cite the recorded widening figure, not an unreproduced one Review caught the doc comment asserting "97px closed against 281.5px open". Neither number appears anywhere in the repo. The figure actually on record — kept by #59 as a tombstone at globals.css:1913 for the rule that used to unpin column 1 — is 279.5px of a 286px region. Cite that instead; the repo convention is that a comment's measured figures are reproducible. Also softens the vacuity guard's comment. It rules out a drawer that renders nothing, which is worth keeping, but it is not evidence of how much width the drawer's content demands: a cell spanning every column is wider than any single column by table structure alone.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
…#61) PR #38 released the sticky first column whenever a row drawer was open: the drawer lived inside the name cell, its crew group is `flex: 1 1 100%`, so opening one row widened column 1 for the whole table until the pinned cell took 279.5px of a 286px region — 98% — and painted over every other column. PR #59 rebuilt the drawer as its own full-width `<tr>` and deleted the release rules on the grounds that the coupling was gone. That was reasoning from the old rationale; nothing measured the new DOM. Measured now, at 320px, region 286px: the pinned cell is 97px — 34% of the region — and the same figure whether the drawer is open or closed. The drawer costs it nothing either way, being a row of its own: across the whole scroll range no drawer control has any of its area under a pinned cell, including at the offsets where the two share an x-band. Keeping the pin on is right, so no rule changes here. What does change is that the test now asserts the two facts the decision rests on rather than only the CSS that follows from them. `covered` for a drawer control was the missing one: `position: static` on the drawer's colSpan cell says the drawer is not itself pinned, not that nothing paints over it. `clearOfPin` cannot answer that — it compares x-extents, and a drawer control scrolled off the region's left edge has the same x-relationship to the pin as one buried under it — so `coveredByPin` intersects areas instead, and returns `xOverlap`/`inRegion` beside the result so a caller can show the offset it picked was one where the pin could have painted over the control at all. Its pin query is scoped to the region's own table. `.log--sticky-col` is a descendant selector, so the crew table nested in a drawer row picks the rule up too and its first column is sticky as well — within its own scroller, which is harmless, but it is not the pin under measurement and counting it would let `covered: 0` be true for the wrong reason.
This was referenced Aug 4, 2026
Merged
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.
Answers a design critique of the admin and member pages. Three priorities in the order they were agreed, plus the cheap consistency wins.
The crew drawer
The nested table fought the row it lived in, and the controls sat below the data they acted on. It is now two sibling
<tr>s joined by acolSpan, controls above the crew table. The drawer row useshiddenrather than unmounting, so an admin's half-typed note survives a toggle.Failed actions had nowhere to land
Server actions that threw on recoverable conditions now redirect with an error code the destination page renders — a stale character, a mid-flight demotion, a last-admin demotion, an expired session. Every code has a message at the other end; there is no silent redirect.
What still throws is a genuine bug, and the new
error.tsxcatches it. It renderserror.digestand nevererror.message, which can carry a raw DB string.The authority for unlink ownership is unchanged: the page-level check is advisory, and
unlinkCharacter'sexpectedAccountIdgate atsrc/services/accounts.ts:298remains the real one.requireAdminAction()still runs unconditionally in every admin action.Inline confirm on destructive actions
REVOKE, UNLINK and FREEZE arm on the first click and act on the second. One
ConfirmArmScopeper table holds a single armed id, so arming one control disarms any other. Escape, blur, and a four-second timer all revert it. Nowindow.confirm(), no modal.Button width is reserved for the longer label up front — the swap was jittering the row (65.28px → 73.17px), caught by an e2e geometry assertion rather than by eye.
chis sized off the "0" glyph and undercountsletter-spacingcompounding across "confirm".This supersedes the UNLINK-only confirm added in #57, and deletes
unlink-button.tsx. Its blur and revert behaviours are folded intoConfirmSubmit, which adds the shared arm scope, Escape, and per-target accessible names. Running two confirm mechanisms seemed worse than removing one — but it is a deletion of someone else's file, so it's the call in here most worth a second opinion.Consistency
scope="col"on the sortable headers.pendingLabelon the side-effecting drawer controls, but deliberately not on the tier buttons: the label is the value being set, and swapping it for "setting…" erases which of the three was pressed at the moment the admin is checking.force-dynamicmeans the page is only as fresh as the last load. On the audit page this makes the aside always-present where it used to vanish when there was nothing to caveat — intended, since a clean table looks equally trustworthy whether it rendered ten seconds or ten hours ago.Dropped and deferred
Three items from the critique were dropped once the source disagreed with them: the conditional DETAILS column, separating revoke from sync-now, and unlink red at rest.
Character location is deferred — it needs
esi-location.read_location.v1, which means re-auth from every existing member. Audit TARGET name resolution and LAST LOGIN emptiness are noted for a separate pass.Verification
npm run typecheck— cleannpm run lint— 0 errors, 5 pre-existing<img>warningsnpm run format:check— cleannpm test— 422 passed (52 files)npm run test:e2e— 33 passedNot run: any visual check of the rebuilt drawer. The e2e suite asserts structure and geometry, not that it looks right.
One caveat on the e2e result — earlier runs failed 9/16/19/22 times, a different set each time, before three consecutive green ones. Cause was a concurrent
playwright testfrom another checkout; every failing test passed in isolation and it cleared when that run exited. The per-worktree harness isolates port and database, not CPU.Reviewer focus
unlink-button.tsxdeletion.actions.tsfiles.COLUMN_COUNT = SORTS.length + 6— correct today, hand-maintained. A seventh fixed column silently desyncs twocolSpansites.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Accessibility & Style