Navigate list/detail sidebars through real links that preserve scroll - #1270
Merged
Merged
Conversation
The router already intercepts anchors carrying data-prevent-scroll-reset; render account/admin list-detail sidebar entries as anchors with that attribute (AccountManagementListItemButton -> AccountManagementListItemLink) so selecting a record keeps the page's scroll position and gains real link semantics (open in new tab, hover intent prefetch). Pre-navigation state resets move to an onNavigate callback gated on the exported shouldRouterHandleClick so modified clicks leave the current pane alone. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR replaces account-management button navigation with router-aware links. It adds shared link behavior for hrefs, disabled states, scroll preservation, and navigation callbacks. The admin RBAC E2E test now locates user entries as links. ChangesAccount navigation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kody-bot
marked this pull request as ready for review
August 7, 2026 04:47
Contributor
|
🔎 Preview deployed: https://kody-pr-1270.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/worker/client/routes/account-package-invocation-tokens.tsx`:
- Around line 1069-1076: Update the AccountManagementListItemLink handlers to
stop calling selectToken(token) during navigation, since it initializes editor
state before the detail route commits. Make onNavigate perform only local
cleanup, and move editor initialization into applyPayload so it runs after the
committed detail route supplies the correct token data; apply the same change to
the corresponding handler around the second referenced link.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 45c04f7d-2198-43f1-ac1d-0480a1990830
📒 Files selected for processing (16)
e2e/admin-rbac.spec.tspackages/worker/client/client-router.tsxpackages/worker/client/routes/account-activity.tsxpackages/worker/client/routes/account-email.tsxpackages/worker/client/routes/account-integrations.tsxpackages/worker/client/routes/account-jobs.tsxpackages/worker/client/routes/account-management-components.tsxpackages/worker/client/routes/account-mcp-servers.tsxpackages/worker/client/routes/account-memories.tsxpackages/worker/client/routes/account-package-invocation-tokens.tsxpackages/worker/client/routes/account-packages.tsxpackages/worker/client/routes/account-remote-connectors.tsxpackages/worker/client/routes/account-secrets.tsxpackages/worker/client/routes/account-values.tsxpackages/worker/client/routes/admin-platform-feedback.tsxpackages/worker/client/routes/admin-users.tsx
Selecting a token in the sidebar now only clears local confirmation and message state; applyPayload initializes the editor from the committed payload. Seeding pre-commit left a window where the URL (which updateSelectedToken targets) still pointed at the previous token while the editor held the new token's fields, so a save could cross-write. Addresses CodeRabbit review on #1270. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
pushed a commit
that referenced
this pull request
Aug 7, 2026
…0) (#1273) * docs(decisions): record the account RecordTable direction Thirteen account and admin screens split an 810px content column into a 352px list and a 434px record, so `<MetadataGrid columns={3}>` gets 115px columns and a 36-character UUID wraps into five lines. The cap means the record is 434px at 1244px wide and also at 2560px. Records the decision to keep `AccountManagementShell` untouched and stop splitting the column instead: one `RecordTable` with `mode` selecting where the record renders (expand / pane / none), rows staying real links so #1270's scroll-preserving navigation survives. Supporting material carries the measurements, the four rejected alternatives, the screen inventory with all 17 `MetadataGrid` call sites, and four runnable prototypes. The prototypes load the repo's own fonts by relative path rather than embedding them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0134SBHSA9QGtS4E3uTiaCdf * Give account and admin records the whole content column (decision 0010) Seventeen account and admin screens split an 810px content column into a ~370px list and a ~434px record. A `columns={3}` metadata band divided that 434px into 115px columns, so a UUID wrapped over five lines and package names truncated at fifteen characters — while half the page sat empty. Both steps of decision 0010: Content layer. `MetadataGrid` sizes from the room it has (`repeat(auto-fit, minmax(min(14rem, 100%), 1fr))`) instead of a declared count, and the `columns` prop is gone. New `IdValue` clips an id to one line in CSS while keeping the whole value in the DOM for assistive tech and selection, with a per-field-named copy button (a new `chip` variant of `CopyTextButton`). New `TimestampValue` owns nowrap, tabular figures, and the null fallback. `RecordTable`. One primitive for all seventeen screens: `expand` unfolds the record under its own row, `pane` puts it below the table, `none` is a table with no selection. Columns drop by priority through container queries, then the same `<table>` becomes cards below 620px — these tables live inside a 200px-railed shell, so the viewport says very little about the room they have. Rows stay real anchors, so `createListDetailRoute`, the `selected` param, and the scroll-preserving navigation from #1270 are untouched. `AccountManagementLayout`, its sidebar and list primitives, and the three `accountManagementTable*Css` exports are deleted. At 1244px `/account/packages` goes 1498px to 1159px tall, and the metadata band finally gets three real columns. At 390px it goes 2976px to 2533px, as cards. Also scopes the Sentry sourcemap inject to build output. It was rewriting the tracked `packages/worker/public/theme-init.js` in place, which raced the concurrent `format:check` leg of `npm run validate`. Where the built API departs from the one 0010 settled — component generics do not survive `remix/ui`'s JSX, and every screen loads its record separately from its list — is recorded in the decision index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Stop a short account page from stretching its own section gaps Two things the empty `/account/packages` view exposed. The shell reserves `min-height: 40rem` because the nav rail is absolutely positioned and contributes no height, so a short page would let the rail spill over the footer. But `align-content` defaults to `stretch`, which hands that reserved height to the auto-sized rows: with `align-items: start` pinning each section to the top of its now-tall track, a page shorter than the floor grew a gap between every section. On an empty packages list that was 146px of nothing between the description and the table; it is 44px — one section gap — now, and the reserved height sits below the last section where it belongs. This was always true of the shell; the split layout was simply never short enough to have slack to distribute. The search field also sliced its placeholder through a letter when it shrank to share a toolbar row. `text-overflow: ellipsis` fixes it, but has to sit on the input rather than on `::placeholder`, which does not accept that property. It covers an overlong value too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Report a refetch inside the table instead of above it Typing in a search field that refetches flashed a bare, unpadded "Loading packages…" line between the page description and the table, shoving everything below it down and back on every keystroke — and the count slot vanished at the same time, which resized the search field the reader was typing into. `RecordTable` takes a `busy` prop now. It sets `aria-busy` on the region and dims the count in place; the announcement rides in a visually hidden polite live region, so the whole thing costs no layout. Measured across a held-open refetch, the search field and the table both stay at exactly the same size and position. The page-level line is kept for the first load, where nothing else is on screen yet and it reflows nothing. The guard is "has a load ever completed", not "is there data" — on an empty account those are not the same thing, and the second one treats every refetch as a first load. Only the five screens whose status actually flips to `loading` on a refetch are wired up. The other nine hold their state through a load latch and never flashed; their line is the only thing marking a first load, so it stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop the 0010 prototypes Four self-contained HTML pages, ~7.9k lines, that existed to explore the shape before it was built. The shape is built; the record keeps the measurements, the rejected alternatives, and the reasoning. Leaving the pages in the repo commits us to maintaining a second, diverging copy of the account UI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address PR review: two real bugs in RecordTable, and a timestamp that lies `aria-expanded` was derived from selection alone, but the record row only renders when a record exists. A selected row whose detail was still loading (or 404'd) reported an expanded region and an `aria-controls` id that was not in the DOM. Both now follow the row that actually renders. Worse: `expand` mode dropped the record entirely when the selected row was not in the current window. A deep link, a filter change, or paging past the selection left a loaded detail with nothing to unfold under, and it silently vanished. It falls back to a pane below the table now — one change in the primitive, so all five expand screens get it. Two `updated` cells formatted a payload timestamp with `new Date(value).toLocaleDateString()`. `formatTimestamp` exists because these arrive as `YYYY-MM-DD HH:MM:SS`, which Safari rejects outright and which every engine reads as local time rather than UTC. Chrome tolerated it, which is why the screenshots looked right. Both go through a new `formatTimestampDate` that shares the normalization. Also from review: - The per-cell `Nch` clamps could overflow a card narrower than the clamp. They are one exported `recordCellClamp(ch)` now, using `min(Nch, 100%)` — the same guard `MetadataGrid` already uses — which also removes fourteen near-identical copies. - `CopyTextButton`'s `ariaLabel` overrode the label text, so a named button kept reading "Copy package id" after it had copied. The result rides in a `role="status"` region instead. - `Created` and `Updated` on a package token fell back to "Never", which cannot be true of a stored token. They take the default em dash. - `/account/secrets` showed "No secrets yet" during its first load. - The ADR quoted the `MetadataGrid` template without the `min()` guard. Not taken: debouncing the search inputs. Real, but `AccountManagementSearchField` did not debounce either, so it is pre-existing rather than introduced here, and changing when the URL updates deserves its own change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 7, 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
3 participants
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.
Intent
Selecting a record in the account/admin list/detail views should not scroll the page back to the top. The client router already supports React Router-style scroll preservation via
data-prevent-scroll-reseton intercepted anchors (seedocs/contributing/architecture/request-lifecycle.md); the list/detail sidebars just never used it — their entries were<button>s callingnavigate()programmatically.Summary
AccountManagementListItemButtonis nowAccountManagementListItemLink: a real<a>carryingdata-prevent-scroll-reset, so selecting a record keeps the scroll position and gains link semantics for free (middle/cmd-click opens in a new tab, hover warms the destination loader via intent prefetch). Whiledisabled, thehrefis dropped so clicks mid-mutation do nothing.onNavigatecallback gated on the newly exportedshouldRouterHandleClick, so modified clicks that open a new tab leave the current pane untouched.e2e/admin-rbac.spec.tsselectors updated fromgetByRole('button', ...)togetByRole('link', ...)for the users list.Testing
npm run validategreen locally (typecheck, lint, format, 554 unit test files / 1902 tests, Playwright e2e suite including the updatedadmin-rbacspec)./account/secretstowindow.scrollY ≈ 439, clicked through several sidebar entries — detail pane swaps while the scroll position stays put (previously it jumped to the top).System changes
System recap — composes existing primitives (low risk)
Mode: recap · Base:
main@bc4dbfb2· Head:08bbbba3Classification: composes — no primitives added or changed; this PR wires the router's existing
data-prevent-scroll-resethandling into the app-ui list/detail views.Primitives touched
app-uiSystem map
List/detail sidebar clicks now flow through router-intercepted anchors instead of programmatic
navigate()calls, reusing the router's existing scroll restoration.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
<button>+navigate(href)<a href data-prevent-scroll-reset>disabledattributehrefdropped +aria-disabledSummary by CodeRabbit
Improvements
Tests