Repository navigation
Give account and admin records the whole content column (decision 0010) #1273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
bd21650
docs(decisions): record the account RecordTable direction
vojtaholik fb84061
Give account and admin records the whole content column (decision 0010)
vojtaholik a6c7f0f
Stop a short account page from stretching its own section gaps
vojtaholik acce87f
Report a refetch inside the table instead of above it
vojtaholik b8ab08a
Drop the 0010 prototypes
vojtaholik 1d76d18
Address PR review: two real bugs in RecordTable, and a timestamp that…
vojtaholik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # 0010: One RecordTable for account and admin list/detail screens | ||
|
|
||
| - **Status:** accepted | ||
| - **Date:** 2026-08-07 | ||
|
|
||
| ## Context | ||
|
|
||
| Thirteen account and admin screens compose `AccountManagementLayout`: a card | ||
| list in a `minmax(18rem, 22rem)` sidebar beside a card detail pane. Inside | ||
| `AccountManagementShell` the 200px nav rail, the gutters and the `72rem` | ||
| (`layoutMaxWidths.extended`) cap leave an 810px content column, and that column | ||
| is then split 352 + 24 + 434. The detail pane therefore gets 434px on any | ||
| monitor, because widening the browser past 1152px only adds margin. | ||
|
|
||
| At 434px a card detail pane is 376px inside its padding, and | ||
| `<MetadataGrid columns={3}>` divides that into 115px columns. A 36-character | ||
| UUID reaching such a column with `overflow-wrap: anywhere` wraps into five lines | ||
| of broken hex. There are 17 `MetadataGrid` call sites, nine of them at | ||
| `columns={3}`. Four screens hand-roll a table through | ||
| `accountManagementTableCss`. | ||
|
|
||
| Measurements, the rejected alternatives, and a four-page working prototype of | ||
| the accepted shape are in | ||
| [the supporting material](./0010-account-record-table/index.md). | ||
|
|
||
| ## Decision | ||
|
|
||
| Keep `AccountManagementShell` exactly as it is — rail, cap, gutters and both | ||
| breakpoints — and stop splitting the content column. A single `RecordTable` | ||
| component owns the inside of these screens, with a `mode` prop selecting where | ||
| the record renders: | ||
|
|
||
| - `expand` — the record unfolds inside the table beneath its own row, for | ||
| read-only records (packages, activity, email, admin users, admin platform | ||
| feedback) | ||
| - `pane` — the record renders in a pane below the table, for records with an | ||
| editor (secrets, values, memories, MCP servers, integrations, connectors, | ||
| package invocation tokens, jobs) | ||
| - `none` — table only, replacing the four hand-rolled | ||
| `accountManagementTableCss` screens | ||
|
|
||
| Rows stay real anchors built from `createListDetailRoute`, so the selected | ||
| record remains in the URL and the scroll-preserving list navigation from #1270 | ||
| keeps working. `MetadataGrid` drops its `columns` prop for | ||
| `repeat(auto-fit, minmax(14rem, 1fr))`, and identifiers render through an | ||
| `IdValue` primitive as single-line copy targets rather than wrapped prose. | ||
|
|
||
| Community listings and `/account/stars` stay card grids. They are content | ||
| (title, description, star count) rather than uniform records, and | ||
| `/account/stars` never adopted `AccountManagementLayout` in the first place. | ||
| Both still benefit from the `MetadataGrid` change. | ||
|
|
||
| ## Consequences | ||
|
|
||
| The record gets the full 810px content column instead of 434px, metadata columns | ||
| size themselves from available space at every viewport, and the same markup | ||
| collapses to labeled cards below 620px — which is the part that helps phones, | ||
| where no amount of page width would have. `mode` being a prop rather than two | ||
| components keeps one toolbar, one table, one metadata band. | ||
|
|
||
| Each screen pays for column choices: which fields become columns, and a | ||
| `drop: 1 | 2 | 3` priority per column so narrow containers shed the least useful | ||
| ones instead of squeezing all of them. `expand` must not cap the list's height, | ||
| because the record renders inside that scroller and a capped list traps it in a | ||
| nested scroll; `pane` and `none` keep the cap. | ||
|
|
||
| Revisit if a screen needs simultaneous list and record visibility badly enough | ||
| to justify a fourth mode, or if a record grows past what one column can hold — | ||
| at which point a dedicated record route, deliberately not built here, becomes | ||
| the answer. |
188 changes: 188 additions & 0 deletions
188
docs/contributing/decisions/0010-account-record-table/index.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| # 0010 supporting material — account RecordTable | ||
|
|
||
| Working material behind [decision 0010](../0010-account-record-table.md): the | ||
| measurements that forced it, the alternatives that were rejected, the settled | ||
| component API, and the screen-by-screen inventory. Point-in-time by design, like | ||
| the record it supports. | ||
|
|
||
| - [Screen inventory](./page-inventory.md) — every affected screen, its mode, and | ||
| its `MetadataGrid` call site | ||
| - [Prototypes](./prototypes/) — four self-contained pages, openable directly in | ||
| a browser | ||
|
|
||
| ## The measurement that reframed it | ||
|
|
||
| `AccountManagementShell` caps at `72rem`. Inside it the rail and gutters leave | ||
| an 810px content column, and `AccountManagementLayout` splits that column: | ||
|
|
||
| | | list | gap | record | | ||
| | ------------- | ----- | ---- | --------- | | ||
| | Split (today) | 352px | 24px | **434px** | | ||
| | Unsplit | — | — | **810px** | | ||
|
|
||
| 434px is what the browser reports at 1244px wide, and also at 2560px: past | ||
| 1152px every extra pixel becomes margin. Unsplitting the column is worth +87% to | ||
| the record and touches nothing outside `AccountManagementLayout` — more than | ||
| raising the cap to `96rem` would have given (525px at the same viewport), for a | ||
| far smaller blast radius. | ||
|
|
||
| At 434px the pane is 376px inside its padding, so `columns={3}` yields 115px | ||
| columns. A UUID is 36 characters. That is the whole story of the wrapped ids. | ||
|
|
||
| ## Rejected alternatives | ||
|
|
||
| **Raise the cap to `layoutMaxWidths.wide`.** Two tokens, and it does help. But | ||
| the header and footer keep their own `72rem` measure, so the console outgrows | ||
| the brand above it unless all three move together, and it does nothing for | ||
| phones. Kept as a possible follow-up, not a prerequisite. | ||
|
|
||
| **Replace the shell: rail into a horizontal section bar, uncapped width, | ||
| mail-client split.** Prototyped in | ||
| [02-rejected-shell-rewrites.html](./prototypes/02-rejected-shell-rewrites.html). | ||
| It buys the most room, but the rail is not what starves the record — the split | ||
| is — and a 15-item horizontal section bar scrolls sideways on a laptop, which | ||
| trades one problem for another. Rejected in favour of leaving the shell alone. | ||
|
|
||
| **A dedicated record route per screen.** Best density available, and the right | ||
| answer for a record deep-linked from outside the account area. It discards the | ||
| scroll-preserving list navigation from #1270 and turns comparing two records | ||
| into two navigations. Deliberately not built; named in 0010 as the escape hatch | ||
| if a record outgrows one column. | ||
|
|
||
| **Keep side-by-side with a slimmer list.** A 216px name-and-date rail gives the | ||
| record 578px. Better than today, worse than stacking, and it forces the filters | ||
| into a popover that then needs designing. Rejected. | ||
|
|
||
| ## The component API | ||
|
|
||
| Settled by building it against four screens rather than by design: | ||
|
|
||
| ```ts | ||
| RecordTable({ | ||
| mode: 'expand' | 'pane' | 'none', | ||
| columns: [{ key, label, align?, drop?, render? }], | ||
| rows, | ||
| selectedId, | ||
| buildHref, // rows stay real links | ||
| toolbar: [ /* controls */ ], | ||
| renderRecord, // omitted for mode: 'none' | ||
| countLabel, | ||
| }) | ||
| ``` | ||
|
|
||
| Four screens needed 13–15 lines of config each and no per-page CSS. Three things | ||
| that only surfaced from building it: | ||
|
|
||
| 1. **`mode` is a prop, not two components.** The plan was `RecordTable` plus a | ||
| separate list-over-detail layout. Secrets proves they are one component: | ||
| identical toolbar, table, columns and metadata band, differing only in where | ||
| the record renders. | ||
| 2. **Column dropping is per-column priority, not one breakpoint.** Usage has | ||
| five columns that all matter; packages has two that do not. Hence | ||
| `drop: 1 | 2 | 3` on the column, with the card fallback at 620px catching | ||
| whatever is left. | ||
| 3. **`expand` must not cap the list height.** The record renders inside the | ||
| list's scroller, so a `max-height` traps it in a nested scroll. Visible the | ||
| moment admin users unfolded eight fields and a role cluster. `pane` and | ||
| `none` keep the cap, because there it is what keeps the record below | ||
| reachable. | ||
|
|
||
| A control _cluster_ fits an expanded row (admin users carries a role select, an | ||
| add button and two destructive actions). A _form_ does not — that is what `pane` | ||
| exists for. | ||
|
|
||
| ## Implementation order | ||
|
|
||
| Two independent steps. The first stands alone and is the only one that helps | ||
| phones. | ||
|
|
||
| **1. Content layer — landed.** No layout change, so it could land and be judged | ||
| by itself. | ||
|
|
||
| - `MetadataGrid`: `columns` prop gone, replaced by | ||
| `repeat(auto-fit, minmax(min(14rem, 100%), 1fr))` across all 17 call sites. | ||
| The `min()` guards a container narrower than the floor; the old | ||
| `max-width: 860px` single-column override went with it, since auto-fit now | ||
| answers that question from the container rather than the viewport | ||
| - `IdValue`: single line, `text-overflow: ellipsis`, copy button. The whole | ||
| value stays in the DOM — clipping is CSS, never a truncated substring — so | ||
| assistive tech reads the id in full and `user-select: all` makes one click | ||
| select it; `title` is the hover convenience on top of that. Its copy button is | ||
| a new `chip` variant of `CopyTextButton`, named per field so a band of six ids | ||
| does not present six identical "Copy" buttons | ||
| - `TimestampValue`: `white-space: nowrap` and | ||
| `font-variant-numeric: tabular-nums`, and it owns the null fallback | ||
| - `/account/packages` `searchText` moved behind an `accountDisclosureCss` | ||
| disclosure with a `12rem` scroll box | ||
|
|
||
| Ids outside a metadata band were left alone: the inbox address on | ||
| `/account/email`, the connector and endpoint URLs, the TOTP setup key, and the | ||
| reporter/owner ids on `admin/community-reports` — a screen the inventory above | ||
| already defers. | ||
|
|
||
| **2. `RecordTable` across all seventeen screens — landed.** | ||
| `packages/worker/client/routes/record-table.tsx`, with `/account/packages` as | ||
| its first consumer and the other sixteen following. `AccountManagementLayout`, | ||
| `AccountManagementSidebar`, `AccountManagementList`, | ||
| `AccountManagementListItemLink`, `AccountManagementSearchField`, | ||
| `accountManagementListMaxHeight`, and the three `accountManagementTable*Css` | ||
| exports are deleted. | ||
|
|
||
| Both steps keep `createListDetailRoute`, the loader payloads, and the `selected` | ||
| query parameter untouched — this was a rendering change, not a data or routing | ||
| one. | ||
|
|
||
| ### Where the built API differs from the one settled above | ||
|
|
||
| The API in this record was written from the prototype, which held one flat | ||
| array. Two parts of it could not be built as specified: | ||
|
|
||
| - **`columns[].render(row)` became `rows[].cells` keyed by column key.** | ||
| `remix/ui`'s JSX does not carry component generics — neither inference nor an | ||
| explicit `<RecordTable<Pkg>>` type argument reaches the component, so a | ||
| `render` callback would receive `unknown`. Call sites stay fully typed because | ||
| each maps its own typed rows into `cells`. | ||
| - **`renderRecord(row)` became a `record` prop holding the built node.** Every | ||
| one of these screens loads its detail separately from its list — an | ||
| `AccountPackageDetail` is a different payload than an | ||
| `AccountPackageListItem`, not a richer view of the same object. `RecordTable` | ||
| decides where the record goes; the screen decides what it is. | ||
|
|
||
| Two more things the components forced that the prototype did not show: | ||
|
|
||
| - **`ariaLabel` names the region as well as the table.** An empty collection | ||
| renders no `<table>` at all, so on a fresh account the toolbar, count, and | ||
| empty copy sat in an unnamed part of the page. | ||
| - **`onNavigate` takes the row id.** `account/remote-connectors` seeds its | ||
| editor draft from the row being opened, which a no-argument callback cannot | ||
| do. | ||
|
|
||
| ### Screens whose shape changed beyond the swap | ||
|
|
||
| - **`account/integrations`** had a two-level sidebar: OAuth apps as group | ||
| headers with their connections nested underneath. A table has no second level, | ||
| so this is now two tables — apps, then connections with an App column. Each | ||
| keeps its own selection. | ||
| - **`admin/system-email`** made the inbox local part the row link. The subject | ||
| is the primary column now, because the primary column is both the link and the | ||
| card heading below 620px, and "support" is not a useful heading. | ||
| - **`admin/users`** had a hand-rolled entitlements table inside its detail; it | ||
| is a nested `mode="none"` `RecordTable`, which trades a sideways scroll for | ||
| the card fallback. | ||
| - Placeholder panes ("Select a package", "Select a job", …) are gone. They | ||
| existed to fill the empty right half of a split that no longer exists. | ||
|
|
||
| ## Open questions | ||
|
|
||
| - **Sort affordance.** Settled: the toolbar keeps the existing `sort` select. | ||
| Clickable column headers would be more natural in a table but need to stay | ||
| URL-backed to match how `q`, `app` and `sort` already work, and that is a | ||
| separate change. | ||
| - **Expanded-row semantics.** Built as specified: a `colspan` row after the | ||
| selected row, with the name cell's link carrying `aria-expanded` and | ||
| `aria-controls`. The axe suite passes, but it still wants a screen-reader | ||
| pass, which axe cannot stand in for. | ||
| - **`admin/users` role cluster.** Shipped as `expand`. If it feels wrong in use | ||
| it moves to `pane` — still a one-word change. | ||
| - **Whether to also raise the cap** afterwards, moving header and footer in | ||
| step. Independent of everything above, and still open. | ||
114 changes: 114 additions & 0 deletions
114
docs/contributing/decisions/0010-account-record-table/page-inventory.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| # Screen inventory | ||
|
|
||
| Supporting material for [decision 0010](../0010-account-record-table.md). Counts | ||
| taken on `main` at `9a4f3fcc`; line numbers drift, the shapes do not. | ||
|
|
||
| Two independent axes: which screens compose `AccountManagementLayout` (so they | ||
| get a `RecordTable` mode), and which render a `MetadataGrid` (so they get the | ||
| content-layer change whether or not their layout moves). | ||
|
|
||
| ## `mode: 'expand'` — read-only record | ||
|
|
||
| The record unfolds inside the table under its own row. Buttons and short control | ||
| clusters are fine here; forms are not. | ||
|
|
||
| | Screen | Record content | | ||
| | ------------------------- | ---------------------------------------------------------- | | ||
| | `account/packages` | 6 metadata fields, tags, search index | | ||
| | `account/activity` | metadata only | | ||
| | `account/email` | metadata plus action buttons | | ||
| | `admin/users` | 9 metadata fields, a role cluster, two destructive actions | | ||
| | `admin/platform-feedback` | metadata only | | ||
|
|
||
| `admin/users` is the boundary case: the only one here with live controls, and it | ||
| also hand-rolls a table today, so it is a replacement rather than an addition. | ||
| If the role cluster feels wrong in use it moves to `pane`. | ||
|
|
||
| ## `mode: 'pane'` — record has an editor | ||
|
|
||
| The record renders in a pane below the table. Unfolded inside a row these push | ||
| the table apart by a screen or more. | ||
|
|
||
| | Screen | Why | | ||
| | ----------------------------------- | ---------------------------------------------------- | | ||
| | `account/secrets` | 8 editable fields, masked value, destructive actions | | ||
| | `account/values` | name, description, value textarea | | ||
| | `account/jobs` | `editState` form (name, timezone, schedule) | | ||
| | `account/package-invocation-tokens` | two forms, eight textareas | | ||
| | `account/mcp-servers` | connection form | | ||
| | `account/integrations` | provider config form | | ||
| | `account/remote-connectors` | connector form, five inputs | | ||
| | `account/memories` | edit field | | ||
|
|
||
| ## `mode: 'none'` — table only, no selection | ||
|
|
||
| These already hand-roll a table through `accountManagementTableCss`. Adopting | ||
| `RecordTable` removes that primitive rather than adding beside it. | ||
|
|
||
| | Screen | Notes | | ||
| | -------------------- | ------------------------------------------------------ | | ||
| | `account/usage` | numeric columns; wants `align: 'end'` and a meter cell | | ||
| | `admin/codemods` | three tables in one screen | | ||
| | `admin/system-email` | one table | | ||
| | `admin/users` | listed above; its table becomes the `expand` one | | ||
|
|
||
| `admin/invites`, `admin/community-reports` and `admin/roles` are flat lists with | ||
| no table today. They would read better tabulated, but none of them is why this | ||
| work exists — leave them until the primitive has settled. | ||
|
|
||
| ## Out of scope | ||
|
|
||
| `account/stars` and the community listings stay card grids. They render content | ||
| — a title, a description, a star count — not uniform records, and | ||
| `account/stars` never adopted `AccountManagementLayout`. Both still pick up the | ||
| `MetadataGrid` change. | ||
|
|
||
| ## `MetadataGrid` call sites | ||
|
|
||
| All 17 lose their `columns` prop. Nine currently pass `columns={3}`, which is | ||
| the configuration that produces the 115px column; the rest default to 2. The | ||
| definition itself lives at `account-management-components.tsx:808`. | ||
|
|
||
| | File | Line | Today | | ||
| | --------------------------------------- | ---- | ------------- | | ||
| | `account-email.tsx` | 734 | `columns={3}` | | ||
| | `account-packages.tsx` | 588 | `columns={3}` | | ||
| | `account-secrets.tsx` | 1670 | `columns={3}` | | ||
| | `admin-feature-flags.tsx` | 408 | `columns={3}` | | ||
| | `admin-feature-flags.tsx` | 561 | `columns={3}` | | ||
| | `admin-invites.tsx` | 505 | `columns={3}` | | ||
| | `admin-platform-feedback.tsx` | 506 | `columns={3}` | | ||
| | `admin-system-email.tsx` | 276 | `columns={3}` | | ||
| | `admin-users.tsx` | 868 | `columns={3}` | | ||
| | `account-activity.tsx` | 793 | default (2) | | ||
| | `account-billing.tsx` | 455 | default (2) | | ||
| | `account-jobs.tsx` | 839 | default (2) | | ||
| | `account-mcp-servers.tsx` | 583 | default (2) | | ||
| | `account-memories.tsx` | 468 | default (2) | | ||
| | `account-package-invocation-tokens.tsx` | 1715 | default (2) | | ||
| | `account-usage.tsx` | 276 | default (2) | | ||
| | `account-values.tsx` | 691 | default (2) | | ||
|
|
||
| `account-billing.tsx` and `account-usage.tsx` render a `MetadataGrid` without | ||
| composing `AccountManagementLayout` at all, which is why the content-layer step | ||
| is worth landing on its own. | ||
|
|
||
| ## Reproducing these counts | ||
|
|
||
| ```sh | ||
| cd packages/worker/client/routes | ||
|
|
||
| # screens composing the list/detail layout | ||
| grep -l AccountManagementLayout *.tsx | grep -v account-management-components | ||
|
|
||
| # every MetadataGrid call site with its columns value | ||
| for f in *.tsx; do | ||
| grep -n '<MetadataGrid' "$f" | while IFS=: read -r ln _; do | ||
| cols=$(sed -n "${ln},$((ln + 2))p" "$f" | grep -oE 'columns=\{[0-9]\}' | head -1) | ||
| printf '%s:%s %s\n' "$f" "$ln" "${cols:-default}" | ||
| done | ||
| done | ||
|
|
||
| # screens hand-rolling a table | ||
| grep -rl accountManagementTableCss . | grep -v account-management-components | ||
| ``` |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.