Repository navigation
feat(ui): table refresh — column picker, density, page sizes, toolbar sort, phone cards - #2259
Conversation
Column picker writing ?columns=, resolved on the server so to-many counts are read for every shown column; comfortable/compact density remembered in this browser and SSR-safe; 25/50/100 page sizes; a toolbar sort control beside header sorting; labelled cards below md in place of the sticky Actions column. Adds a DropdownMenu primitive on the existing Radix dependency. Implements #2254. Part of #2246. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 9804411 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES (posted as a COMMENT review because the reviewing identity authored this PR)
Reviewed at cb01ac0. Ten findings are posted as inline comments. Ordered by severity:
Must fix
packages/ui/src/lib/density.ts:52: Compact density does nothing when localStorage is unavailable (Safari private mode, blocked site data).storeDensityswallows the error and notifies listeners.readStoredDensitythen re-reads the empty storage and returnscomfortable. The comment that the choice "still applies" is false. Keep an in-memory value as the snapshot fallback.packages/ui/src/components/ListViewClient.tsx:300:toggleColumnbuilds the next set fromdisplayColumns, which is the previous server render. The menu stays open (preventDefault), so two quick toggles drop the first one. Build from a local pending set, or from the current URL.packages/ui/src/components/ListView.tsx:195:availableColumnsis every key ofdisplayFields. The picker and?columns=therefore bring back fields that were removed from the defaults because a read rule always leaves them empty (for example the AuthAccount credential fields withdefaultColumn: false). Column choice should not be able to bring those back.
Should fix
4. packages/ui/src/components/AdminUI.tsx:176: a repeated ?columns=a&columns=b (string[]) is silently ignored, and the view falls back to the default columns.
5. packages/ui/src/components/ListViewClient.tsx:435: choosing the active sort field or direction again navigates and resets to page 1.
Cleanups
6. density.ts:48 copies theme-mode.ts. Extract a shared stored-choice helper, which also means the fix in item 1 is made once.
7. ListViewClient.tsx:91: the CARD_* classes belong in lib/tableClasses.ts, so other tables can use them.
8. ListViewClient.tsx:88: DEFAULT_PAGE_SIZE/max size duplicate the defaults in ListView and AdminUI.
9. ListView.tsx:192: the comment about the fallback "used when no explicit columns" is now stale. The empty catch comments in density.ts ("Mirror of the add above.") also fail the repo comment rule.
10. ListViewClient.tsx:311: chooseDensity is just another name for storeDensity.
Checked and fine
?columns=resolution. Unknown names are filtered withObject.hasOwn(displayFields, …), so nothing leaks and nothing throws. To-many counts are computed fromresolvedColumns, so an added relationship column gets its count. Field Visibility still applies, because values come through the secured read.- SSR safety. Density uses
useSyncExternalStorewith a server snapshot ofcomfortable, and storage access is wrapped in try/catch, so there is no hydration mismatch and no throw. - Props passed to the client component (
columns,availableColumns,columnsChosen) are plain string arrays and a boolean, so they serialise. - I found no
anyor casts in the new source. - Minor accessibility point on mobile cards: with
display:blockon the table parts, screen readers lose table semantics. Considerrole="list"/listitemon the card branch, or labelleddt/ddpairs.
Generated by Claude Code
- Share one stored-choice helper between density and the theme choice, and keep a choice in memory when the browser refuses to store it. - Toggle picker columns from the pending selection, so two quick ticks both land. - Read a repeated ?columns= as one selection. - Leave the page alone when the toolbar sort re-picks the active field or direction. - Move the phone-card classes into tableClasses, share the page-size default and cap, call storeDensity directly, and drop a stale comment. - Point the sticky-actions browser test at bg-card, the surface its remaining consumer uses: the list view no longer emits max-md:before:bg-background. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
Coverage Report for Core Package Coverage (./packages/core)
File CoverageNo changed files found. |
Coverage Report for UI Package Coverage (./packages/ui)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES. I'm posting this as a COMMENT review because GitHub won't let this identity request changes on its own PR. This is a re-review at 8c45016.
Previous round: I checked each reply against the current code, and every claimed fix holds. createStoredChoice has an in-memory fallback, and the picker keeps its pending selection. A repeated ?columns= is flattened. Re-picking the active sort field or direction returns early. The card classes moved to tableClasses.ts, and the page-size constants are shared. The stale comment and the chooseDensity alias are both gone. I resolved all of those threads. The deferral on read-denied columns is sound, since classifyRowIndependentRead has no public core export, so I resolved it as deferred. Please link the follow-up issue.
Shared stored-choice helper (theme): I found no regression. The theme module's exports and themeInitScript are unchanged. ThemeToggle still uses getServerChoice for SSR. On the server, read catches the missing localStorage and returns the fallback, and store is only ever called from a click handler, so the module-level unpersisted can't leak between requests. A refused write now applies for the page, as the changeset says. One edge case is acceptable: after a refused write in this tab, a successful write in another tab is ignored here until this tab stores again.
Blocking (new in this PR):
ListView.tsx:195:?columns=accepts only names indisplayFields, butinitialColumns(passed through unchanged before this PR) can name something outside it, such asid. The first picker toggle silently drops that column, and nothing in the picker can bring it back.pendingColumnsthen never matches the server's columns, so the picker drifts (the related finding atListViewClient.tsx:293). Accept the configuredcolumnsnames in the allow-list and inavailableColumns, or reconcilependingColumnswithout requiring an exact match.ListViewClient.tsx:471:ListViewClientis a public export, and it now always renders a Columns menu. That menu only works when the parent reads?columns=back, which onlyAdminUI→ListViewdoes. Render the picker only whenavailableColumnsis supplied, or add an opt-out prop.
Non-blocking: the card title's order on phones (:696), and re-checking a default column, which pins the default set in the URL (:299). Also: use computeDefaultColumns instead of re-implementing it (ListView.tsx:198), the duplicated segmented-control markup (:515), and the now-inaccurate comment "resolved the same way it resolves them" (ListView.tsx:186).
I left the dropdown-menu.tsx slot finding out on purpose: that file must stay byte-identical with the other open PR.
Generated by Claude Code
- Accept configured initialColumns names outside the fields in ?columns= and the picker - Tie the picker's pending selection to the confirmed columns it was built over - Render the Columns menu only when availableColumns is supplied - Drop ?columns= when a pick lands back on the default set (new defaultColumns prop) - Put the title cell first on phone cards - Reuse computeDefaultColumns, share one segmented control, fix a stale comment - Narrow the dropdown-menu slot wording in docs and changeset Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
|
Deferred findings from review are tracked as follow-ups:
Generated by Claude Code |
| over: string | ||
| columns: string[] | ||
| } | null>(null) | ||
| const pending = pendingColumns?.over === confirmedColumns ? pendingColumns.columns : null |
There was a problem hiding this comment.
Stale pending column set after browser Back/Forward. pendingColumns is never cleared once its navigation lands; it only goes dormant while confirmedColumns differs from over. AdminUI's Suspense is unkeyed and router.push runs in a transition, so this client component (and its state) survives navigation. Scenario: default set A, check column X → pending { over: A, columns: A+X }, URL becomes A+X. Press Back → server renders A again, pending.over === confirmedColumns matches, so the Columns menu shows X checked and offers "Reset columns" while the table shows A; the next toggle is computed from A+X instead of A. Clear the pending entry once the confirmed set changes (e.g. store the target set and drop it when confirmedColumns equals or moves past it), or use React.useOptimistic inside the push transition, which reverts automatically.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 9804411. The pending entry still holds the set it was built over plus the target set. It is now cleared, not just ignored, as soon as the confirmed columns differ from the set it was built over. That covers both landing on the target and moving on to anything else. So after Back, the menu shows what the table shows, and the next toggle builds from that. The two existing tests (keeps both of two columns ticked before navigation lands and follows the columns the server confirms when it drops a picked name) still pass. New test: shows what the table shows after Back undoes a pick. It ticks Views, confirms title,status,views, then goes back to title,status. It asserts Views is unchecked, Reset is gone, and ticking Author pushes columns=title,status,author. I confirmed it fails without the clear.
Generated by Claude Code
| relationshipRefs={relationshipRefs} | ||
| columns={columns} | ||
| columns={resolvedColumns} | ||
| availableColumns={availableColumns} |
There was a problem hiding this comment.
Columns picker (and the new Rows-per-page control) render for every ListView consumer, not only pages that read the params back. ListView is a public export and always passes availableColumns, so a host rendering <ListView> directly (without AdminUI) gets a Columns menu that writes ?columns= and a 25/50/100 control that writes ?pageSize=, neither of which that host passes back as visibleColumns/pageSize. Clicking them navigates and nothing changes. The docs/props say the menu "renders only when availableColumns is supplied, since it works only on a page that reads ?columns= back", but ListView supplies it unconditionally. Gate it on visibleColumns !== undefined (or an explicit opt-in prop set by AdminUI).
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 9804411 with an explicit opt-in prop. visibleColumns !== undefined can't work here, because AdminUI leaves it undefined on every default page (no ?columns=), and there is no equivalent for page size. ListView has a new urlControls prop, which says the page reads ?columns= and ?pageSize= back, and AdminUI sets it. Only when it is set does ListView pass availableColumns and the new ListViewClient pageSizeControl flag. A bare ListView gets neither control. Docs and changeset updated.
Tests:
ListView ?columns= resolution > offers no column picker or size control to a host that does not read the params back(bareListView:availableColumnsundefined,pageSizeControlfalse)offers the column picker and the size control on an AdminUI list pageinAdminUIListView.test.tsx, which renders theListViewelement thatAdminUIroutes to over a Test context and checks that both are presentpage sizes > offers no size control unless the page reads ?pageSize= back
Generated by Claude Code
| const listeners = new Set<() => void>() | ||
|
|
||
| const read = (): T => { | ||
| if (unpersisted !== undefined) return unpersisted |
There was a problem hiding this comment.
After one failed write, cross-tab updates are ignored for the life of the page. Once store() throws, unpersisted is set and read() returns it unconditionally; a later storage event from another tab (where the write succeeded) triggers the listener but read() still returns the in-memory value. This also changes readStoredChoice/subscribeThemeChoice (public theme API) to ignore other tabs in that state. Clear unpersisted when a storage event for key arrives.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 9804411. subscribe now wraps the listener. A storage event for this key, or a null key from localStorage.clear() in another tab, clears unpersisted before notifying. So a write that succeeds in another tab takes effect here, and the theme API gets the same behaviour. New test: density > takes a choice another tab stored after this one failed to store its own. Compact is applied with setItem throwing, then another tab stores comfortable and dispatches the event, and the table goes back to comfortable. I confirmed it fails without the clear.
Generated by Claude Code
|
|
||
| const toggleColumn = (column: string, shown: boolean) => { | ||
| const next = shown | ||
| ? [...pickedColumns.filter((name) => name !== column), column] |
There was a problem hiding this comment.
Re-checking a column moves it to the end. toggleColumn appends a newly shown column, so unchecking and re-checking title produces ?columns=status,views,title and the label column jumps to the last position (unless the set happens to equal the default, which then snaps back to default order). The picker lists columns in availableColumns order; inserting at the column's position in availableColumns (or the current order) would keep layout stable.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 9804411. A re-checked column is now inserted before the first picked column that comes after it in availableColumns order. The rest keep their current order, and a column with nothing after it still goes last. New test: column picker > puts a re-checked column back at its place in the picker order. With status,views shown, ticking Title pushes columns=title,status,views. I confirmed it fails when the column is appended.
Generated by Claude Code
| > | ||
| Next | ||
| </Button> | ||
| {total > 0 && ( |
There was a problem hiding this comment.
Rows-per-page control shows in states where it does nothing useful. It now renders whenever total > 0, even when every row fits on the smallest page (e.g. total = 3), and with an arbitrary ?pageSize=37 none of the segments is pressed, so the control shows no current state. Consider hiding it when total <= 25 and reflecting non-preset sizes.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 9804411. The rows-per-page control is hidden when total <= 25. When ?pageSize= is not 25, 50 or 100, the actual size is added as its own segment, in numeric order and pressed, so the control always shows the current state. Docs and changeset updated. Tests in page sizes:
hides the size control when every row fits on the smallest page(total 25)shows the size control on a single page of more than 25 results(total 40; replaces the old total-2 test)shows a size outside the presets as the pressed segment, in order(?pageSize=37gives 25 / 37 / 50 / 100, with 37 pressed)
Generated by Claude Code
|
|
||
| const row = screen.getByText('First Post').closest('tr') | ||
| expect(row).toHaveClass('max-md:grid') | ||
| const statusCell = within(row as HTMLElement) |
There was a problem hiding this comment.
Type cast in new test code. Root CLAUDE.md: "Avoid the use of the any type, and do not use type casting." This adds within(row as HTMLElement). Narrow with if (!(row instanceof HTMLElement)) throw … or use a query that returns HTMLElement (e.g. screen.getByRole('row', …)).
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 9804411. The test now gets the row with screen.getByRole('row', { name: /First Post/ }), which returns HTMLElement, so there is no cast.
Generated by Claude Code
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES. I'm posting this as a COMMENT because GitHub won't let this account request changes on its own PR. One small fix is needed. Apart from it, the PR is sound.
Round-2 threads. I checked all eight replies against 635afe5 and resolved them:
- configured columns outside
displayFieldsare now pickable - the picker is gated on
availableColumns - the pending selection tracks the confirmed set it was built over (
over) - the title cell has
max-md:order-first - a pick that lands on the default set drops
?columns= ListViewusescomputeDefaultColumns- a shared
SegmentedControlreplaces the two button groups - the stale comment is gone
Blocking
packages/ui/tests/components/ListViewClient.test.tsx:52:within(row as HTMLElement)is a type cast, which the root CLAUDE.md does not allow ("do not use type casting"). Narrow it withif (!(row instanceof HTMLElement)) throw new Error('row not found').
Non-blocking. These are in the inline threads, and they can be fixed here or in a follow-up.
ListViewClient.tsx:353: the pending column set is never cleared. If this component's state survives a Back navigation, the stale set becomes active again. Please confirm with one test that rerenderscolumnsA → A+X → A after a toggle. If it reproduces, drop the pending set once the confirmed set changes.ListView.tsx:391:ListViewalways offers the Columns and page-size controls, which is broader than the docs' "only when supplied" wording. Hosts already have to read back?page/?sort/?search, so this follows an existing pattern. Narrowing the docs would be enough.storedChoice.ts:16: after one failed write,unpersistedhides updates from other tabs. This is an edge case.ListViewClient.tsx:363: a column you re-check moves to the end of the list.ListViewClient.tsx:320: the column choice is lost on links that leave the list. This is a design choice.ListViewClient.tsx:784: the Rows-per-page control shows whentotalis small, and it shows no selected button for a size outside 25/50/100.
I withdrew and resolved the phone-card accessibility thread because #2261 already tracks it. I did not ask for any change to dropdown-menu.tsx.
Generated by Claude Code
- ListView offers the Columns menu and rows-per-page control only under a new urlControls prop, which AdminUI sets - clear the pending column set once the confirmed columns move, so Back shows what the table shows - a storage event for the key clears the unpersisted stored choice - a re-checked column returns to its place in the picker order - hide rows-per-page at 25 rows or fewer, and show a non-preset size - drop a test cast Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
Brings in the table refresh (#2259): column picker, density, page sizes, toolbar sort and the phone-card layout. The Row actions trigger now sits in the card's pinned actions cell in place of the old Edit link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
Implements #2254
Part of #2246
Summary
DropdownMenuprimitive (packages/ui/src/primitives/dropdown-menu.tsx) on the already-declared@radix-ui/react-dropdown-menu, exported from@opensaas/stack-ui/primitives, every part carrying adropdown-menu-*data-slot.?columns=:AdminUIparses the comma list and passes it toListViewas the newvisibleColumnsprop.ListViewresolves it on the server against the list's displayable fields. It keeps URL order, de-duplicates, ignores unknown names, and falls back toinitialColumnsor the defaults when no valid name is left. Because the server builds theinclude(..., rows => rows.count())set from the resolved columns, a to-many you add through the picker is counted through the secured reducer, so its count is correct. A to-many that is not shown is still not reduced. Field Visibility still applies: a read-denied column shows up with no value.ListViewClientgets the resolvedcolumns, plusavailableColumnsandcolumnsChosen.?columns=, the last column can't be unchecked, and "Reset columns" drops the param. A chosen set is carried through paging, sorting and filtering. The default set never appears in the URL.localStorageunderopensaas-list-density(src/lib/density.ts, the same pattern astheme-mode.ts) and read throughuseSyncExternalStorewith a comfortable server snapshot, so SSR is safe. The table wrapper carriesdata-density.?pageSize=are unchanged.md: rows become labelled grid cards (data-labelwithbefore:content-[attr(data-label)]), the label column spans the card, and checkbox and Edit are pinned in the corners. The table container stops scrolling sideways, and this replaces the list view's sticky Actions column. Onlymax-md:classes the prebuilt-stylesheet test can scan are used.docs/content/reference/ui.md). Changesets:@opensaas/stack-uiminor, plus the requiredcreate-opensaas-apppatch.Tests
tests/components/ListView.test.tsx: new?columns=block against a real Test context. It covers chosen order, the count for an added to-many (drafts) being correct, unknown names ignored with fallback to config or defaults, Field Visibility on a chosen read-denied column, and sortability of chosen columns.tests/components/AdminUIListView.test.tsx:?columns=is parsed intovisibleColumns.tests/components/ListViewTableControls.test.tsx(new): column picker URLs, reset, carry-through, toolbar sort, density persistence and the SSR snapshot, page sizes, selection rules.tests/components/ListViewClient.test.tsx: the sticky-actions assertions are replaced by card-layout assertions.pnpm build,pnpm --filter @opensaas/stack-ui typecheckand the full UI suite pass (1103 tests).pnpm lintreports 0 errors; its one warning is an existing one inpackages/core/src/secured/include.ts.Notes for reviewers
DropdownMenuprimitive. This issue's acceptance criteria require it here, so whichever PR merges second should keep the one inprimitives/dropdown-menu.tsx.ListViewis where that belongs.Generated by Claude Code