Make the three admin tables operable at 320px - #38
Merged
Conversation
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR adds responsive sticky tables for admin accounts and audit logs, responsive timestamps for audit and sync data, stable account identities, contextual accessible labels, and extensive Playwright coverage for layout, keyboard access, and fallback states. ChangesAdmin table responsiveness and accessibility
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
guarzo
force-pushed
the
worktree-admin-table-mobile
branch
from
August 3, 2026 23:53
d8726e1 to
ae4f055
Compare
![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/admin.spec.ts`:
- Around line 216-224: Extract the repeated admin-page setup into an openAdmin
helper near the existing test utilities, encapsulating seedDenseWorld,
sessionCookieFor, viewport configuration, and navigation. Update the affected
tests, including the narrow-screen accounts test, to call openAdmin with their
page, context, route, and width, preserving the returned admin value where
needed.
In `@playwright.config.ts`:
- Around line 6-13: Update the E2E URL construction in the helpers module to
reuse the centralized ORIGIN configuration from playwright.config.ts instead of
hardcoding localhost:3111. Ensure navigation URLs continue following the
E2E_PORT override consistently.
In `@src/app/admin/accounts/page.tsx`:
- Around line 271-281: Update the fallback display in the account summary JSX to
render the existing idLabel value instead of reconstructing the account ID
string inline, preserving identical accessible and visible text while leaving
the surrounding firstName and “no main” logic unchanged.
🪄 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: 4bec8bcc-3236-492b-9d98-be8762792b94
📒 Files selected for processing (9)
e2e/admin.spec.tse2e/helpers.tsplaywright.config.tssrc/app/_components/submit.tsxsrc/app/_components/ui.tsxsrc/app/admin/accounts/page.tsxsrc/app/admin/audit/page.tsxsrc/app/admin/sync/page.tsxsrc/app/globals.css
Comment on lines
+216
to
+224
| test(`accounts at ${width}px: the name stays put while the controls are reached`, async ({ | ||
| page, | ||
| context, | ||
| }) => { | ||
| const admin = await seedDenseWorld(); | ||
| await context.addCookies([await sessionCookieFor(db, admin.id)]); | ||
| await page.setViewportSize({ width, height: 720 }); | ||
| await page.goto("/admin/accounts"); | ||
| await page.locator(".scroller").scrollIntoViewIfNeeded(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract the repeated narrow-screen setup.
The four-step sequence seedDenseWorld(), context.addCookies(...), setViewportSize(...), goto(...) repeats in about ten tests. A helper keeps the seeding and cookie contract in one place, so a change to sessionCookieFor or to the seed shape touches one call site.
♻️ Proposed helper
async function openAdmin(
page: import("`@playwright/test`").Page,
context: import("`@playwright/test`").BrowserContext,
path: string,
width: number,
) {
const admin = await seedDenseWorld();
await context.addCookies([await sessionCookieFor(db, admin.id)]);
await page.setViewportSize({ width, height: 720 });
await page.goto(path);
return admin;
}🤖 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/admin.spec.ts` around lines 216 - 224, Extract the repeated admin-page
setup into an openAdmin helper near the existing test utilities, encapsulating
seedDenseWorld, sessionCookieFor, viewport configuration, and navigation. Update
the affected tests, including the narrow-screen accounts test, to call openAdmin
with their page, context, route, and width, preserving the returned admin value
where needed.
guarzo
enabled auto-merge (squash)
August 4, 2026 00:55
guarzo
force-pushed
the
worktree-admin-table-mobile
branch
from
August 4, 2026 01:33
ae4f055 to
83ccae9
Compare
guarzo
changed the title
guarzo
force-pushed
the
worktree-admin-table-mobile
branch
2 times, most recently
from
August 4, 2026 02:16
351a4e4 to
7130342
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/app/admin/accounts/page.tsx`:
- Around line 246-263: Normalize r.mainName by trimming whitespace and use that
normalized value for identity selection and displayed labels, while preserving
the account-id fallback when no valid name exists. Update pinLabel to append
“·no main” based on the absence of the normalized main name, not on firstName,
so every row without a main character is marked.
🪄 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: 22cb325f-1746-428b-9aa6-74a1ef0d1659
📒 Files selected for processing (11)
e2e/admin.spec.tse2e/audit.spec.tse2e/geometry.tse2e/helpers.tse2e/sync.spec.tssrc/app/_components/scroller.tsxsrc/app/_components/submit.tsxsrc/app/admin/accounts/page.tsxsrc/app/admin/audit/page.tsxsrc/app/admin/sync/page.tsxsrc/app/globals.css
guarzo
disabled auto-merge
August 4, 2026 03:01
The prior mobile pass proved there was no horizontal *page* scroll at
320px. It did not prove the admin tables were usable, and they were not:
globals.css had one layout breakpoint and no position:sticky anywhere.
To reach a row's controls you scrolled the region right, which carried
the character name off the left edge — so you pressed a 28px control
that changes someone's tier with nothing on screen saying whose. On a
product whose promise is "derole, don't boot", that is a
wrong-person-deroled bug waiting for a bad night.
Sticky headers on accounts and audit, and a pinned first column on both,
so the identifying value survives the scroll right. The pin needs an
opaque cell, which cannot inherit the translucent row hover; it is
flattened with a measured color-mix that lands within 1/255 of the
neighbouring cells. Not applied to the runs tables: getSyncStatus caps
them at five rows, so they have no vertical scroll range and a sticky
header there could not be tested.
Narrow paths for the two tables that forced the most sideways scroll.
Below 40rem the runs table gives up its 44rem floor and its ISO stamp
for a relative time, and audit does the same: 992px wide with a pin
eating 69% of the region becomes 544px with a pin at 28%, taking forced
scroll at 320px from 706px to 258px. The exact instant stays in a
visually-hidden span in both, so nothing is destroyed to fit. 258px is
the honest floor, not a way station — four legible columns plus a
non-zero Details column exceed a 286px region no matter how it is cut.
Two pre-existing defects surfaced on the way. Audit's Details column
measured 0px at every viewport: table-layout:fixed plus a colgroup
summing 47.75rem left nothing for the one unsized column, so the payload
disclosure was unclickable. And the accounts identity chain used ??,
which walks straight past an empty string, so a blank character name
rendered aria-label="Note for " — a row with no identity at all, in the
column whose entire job is saying whose tier is about to change. Blank
names are now filtered before the pick ("" sorts first and would win it)
and the tier, cryo, auto and note controls all carry the row identity.
Two cascade traps found by reading computed style rather than the
stylesheet: the runs and audit width floors were declared after the
40rem block, so every same-specificity narrow override was silently
void; and audit's colgroup widths were inline styles, which outrank a
media query outright. Both moved.
Every geometry assertion in this change was shown to fail without its
rule before being kept. toBeInViewport is not used for any of them — an
element painted over by a sticky cell still passes it, so occlusion is
measured rect-vs-rect via e2e/geometry.ts. One pre-existing audit
assertion was found to have been silently disabled by this branch's own
visually-hidden span, whose overflow:hidden matched the test's
clipped-cell skip guard; the guard now excludes it.
Superseded by main in the meantime and deliberately not carried over:
an E2E_PORT override for the test harness, which #49 replaced with
per-worktree hashed ports and server-ownership proof.
guarzo
force-pushed
the
worktree-admin-table-mobile
branch
from
August 4, 2026 03:10
7130342 to
4fbd61f
Compare
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
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.
Three admin tables that were readable at 320px but not operable. On
/admin/accounts you scrolled the region right to reach a row's controls, which
took the character's name off the left edge — and then pressed a 28px control
that changes someone's tier with nothing on screen saying whose. On a product
whose promise is "derole, don't boot", that is the wrong-person-deroled bug
waiting to happen, so this is a correctness change, not polish.
What changed
Sticky header (
.log--sticky-head) on all three tables. Sticky needs ascroll range to travel over, so
Scrollergained atallprop capping theregion at
80svh; without itscrollHeight === clientHeightandtop: 0hasnowhere to go.
border-collapsebecomesseparateon these tables: undercollapsethe rules belong to the table box rather than the cell, so a pinnedheader's hairline detaches on the first scroll.
Sticky first column (
.log--sticky-col) on accounts and audit, with anopaque ground, a right hairline for the edge, a corner cell that outranks both
axes, and the translucent row hover flattened via
color-mixso the pinned celltracks its neighbours.
A real narrow path for
.log--runs, which had a hardmin-width: 60rem—672px of forced horizontal scroll at 320px, for a table whose first two columns
are ISO timestamps. Below 40rem it gives up the floor and the ISO stamp for
elapsed time. Audit gets the same treatment: its pinned At column was 69% of a
286px region spent on the least of the row's meaning; as elapsed time it is 28%,
and the table's forced scroll drops from 706px to 258px. The exact instant stays
in the accessibility tree in both cases.
Two interactions worth a reviewer's attention
Table columns are shared. The accounts drawer lives inside the first cell
and its crew group is
flex: 1 1 100%, so opening one row widens column 1 forevery row. Measured at 320px: the first cell takes 279.5px of a 286px region —
98%, i.e. a pinned cell painting over every other column. Below 40rem an open
drawer therefore unpins column 1 (body and header corner; leaving the corner
behind parks an opaque NAME heading over the "sync now" buttons). Scoped to
td:first-child details[open]because audit carries.log--sticky-coltoo andits Details column is a
<details>in column 5 — a bare selector dropped therow's anchor exactly when the row got taller and needed it most.
The edge fades from #57.
.scroller-fade--startsits atleft: 0of theframe — on top of the pinned cell — and claims content is hidden that way when
the pin guarantees it isn't. It is suppressed while the pin is live, and
restored when an open drawer unpins the column and the cue becomes true again.
This is the one change here that wasn't in the original scope; it's a seam
between this branch and #57 that only exists once both land.
Verification
npm run typecheck,npm test(422 passed),npm run lint(0 errors; 5pre-existing
no-img-elementwarnings, none in files this branch touches),npm run format:check, andnpm run test:e2e(52 passed) green twice on thefinal tree.
The geometry assertions compare rectangles rather than using
toBeInViewport:that is an intersection with the viewport rect, and an element painted over
completely by an opaque sticky cell still intersects it — so an assertion built
on it passes with the pinning CSS deleted outright. Every new rule here was
sabotage-checked by deleting it and confirming the test goes red.
Summary by CodeRabbit