Repository navigation
feat(organization): sort, filter and page the org users table - #1832
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces client-side filtering and pagination to the SimpleBrowseDataTable component, along with comprehensive unit tests and updates to the organization users table to leverage these features. To support this, a new studioClientTableFeatures set is defined to handle client-side row models without impacting other tables. Feedback on these changes suggests moving TablePagination to a shared directory to maintain clean architecture dependencies, and replacing useLayoutEffect with a render-phase state adjustment to avoid SSR warnings.
|
|
||
| import { Input } from '@/components/ui/input'; | ||
| import { Table, TableBody, TableCell, TableHeader, TableHeadSortable, TableRow } from '@/components/ui/table'; | ||
| import { PAGE_SIZE_OPTIONS, TablePagination } from '@/features/instance/databases/components/TablePagination'; |
There was a problem hiding this comment.
Importing TablePagination from a feature-specific directory (@/features/instance/databases/components/) into a shared component (@/components/SimpleBrowseDataTable.tsx) violates the clean architecture principle of unidirectional dependencies. Shared components should be self-contained and not depend on feature-specific code.
Consider moving TablePagination.tsx to src/components/ and updating the import.
| import { PAGE_SIZE_OPTIONS, TablePagination } from '@/features/instance/databases/components/TablePagination'; | |
| import { PAGE_SIZE_OPTIONS, TablePagination } from '@/components/TablePagination'; |
There was a problem hiding this comment.
Declined for this PR. The repo doesn't enforce a components → features boundary: .oxlintrc.json has no import restriction, and src/components already imports from @/features in several places on stage, including Navbar.tsx:13-14, AccountMenu.tsx:16-17, NotificationBell.tsx:5-7 and OrganizationSwitcher.tsx:10-11.
TablePagination has no feature dependency of its own. It imports only @/components, @/lib, lucide-react and React, so moving it would be a pure relocation. But the move would also touch TableView.tsx and TablePagination.test.tsx and ripple through the two PRs stacked on this one. That makes it a better fit for a separate refactor.
Leaving this open for a human to decide.
🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
| import { ColumnDef, Row, studioClientTableFeatures } from '@/lib/table'; | ||
| import { flexRender, PaginationState, RowData, SortingState, useTable } from '@tanstack/react-table'; | ||
| import { SearchIcon } from 'lucide-react'; | ||
| import React, { useLayoutEffect, useState } from 'react'; |
There was a problem hiding this comment.
Since useLayoutEffect is being removed to avoid SSR warnings, we can remove it from the React imports as well.
| import React, { useLayoutEffect, useState } from 'react'; | |
| import React, { useState } from 'react'; |
There was a problem hiding this comment.
Declined: useLayoutEffect stays because the clamp effect stays. The reason is in the thread on line 79: there's no SSR here, and the effect runs before paint.
🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
| useLayoutEffect(() => { | ||
| if (pagination.pageIndex > lastPageIndex) { | ||
| setPagination((current) => ({ ...current, pageIndex: lastPageIndex })); | ||
| } | ||
| }, [pagination.pageIndex, lastPageIndex]); |
There was a problem hiding this comment.
Using useLayoutEffect in a client component ('use client') will trigger SSR warnings during pre-rendering in frameworks like Next.js (e.g., "useLayoutEffect does nothing on the server...").
Instead of an effect, we can safely adjust the state during the render phase. When React detects a state update during render, it immediately aborts the current render and restarts it with the updated state, avoiding both SSR warnings and layout flashes.
if (pagination.pageIndex > lastPageIndex) {
setPagination((current) => ({ ...current, pageIndex: lastPageIndex }));
}
There was a problem hiding this comment.
Declined: the warning this guards against can't fire here. Studio is a client-only Vite SPA. Its one React root is createRoot in src/main.tsx:28, with no renderToString or hydrateRoot anywhere, so React never pre-renders this component on a server. The 'use client' line is a leftover directive and does nothing under Vite.
The layout effect also runs before paint, so the out-of-range page is never visible. moves to the new last page when the data shrinks out from under the current one (SimpleBrowseDataTable.test.tsx:182) pins the outcome.
A guarded render-phase update would also work, and it would save one commit of the empty page. Neither approach changes what the user sees, so I've kept the reviewed version. Leaving this open in case a human prefers the render-phase form.
🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
SimpleBrowseDataTable gains two opt-in props: `filter` (a search box driving TanStack's global filter) and `paginated` (client-side pages of 20, reusing the browse table's TablePagination, shown only once the rows outnumber one page). The org users table turns both on and makes every data column sortable, with Roles now an accessor column so it can be sorted and searched. A re-sort or a new filter returns to the first page, but new `data` keeps the current one (clamped to the last page if the list shrank): these lists poll every 10s, and TanStack's automatic reset would send the user back to page 1 whenever any record in the payload changed. The filtered and paginated row models live in a separate `studioClientTableFeatures` object rather than the shared feature set: the paginated row model defaults to a 10-row page, so registered globally it would truncate the browse, logs and every other table that did not remember `manualPagination: true`. Only the two features' APIs and state join the shared set, so every table keeps the one table type, and a SimpleBrowseDataTable that opts into neither prop bypasses both row models (`manualFiltering` / `manualPagination`). Refs #1264 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
28146a1 to
85ccde4
Compare
Opts the Config > Users table into SimpleBrowseDataTable's `filter` and `paginated` props and makes every column sortable. Role and Created become accessor columns so they can be sorted (and Role searched); the Created and Updated timestamps are kept out of the filter, since their digits would otherwise match almost anything typed. Refs #1268 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opts the organization Roles table into SimpleBrowseDataTable's `filter` and `paginated` props and makes every column sortable. ID and Users Assigned become accessor columns so they can be sorted (by id, and by how many users hold the role); the filter matches the role name only, since the opaque ids and the user counts would match many short queries. Refs #1269 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
⊙ Problem
The organization Users page shows every member in one table with no way to sort it (every column sets
enableSorting: false), search it, or page through it (#1264). The table is the sharedSimpleBrowseDataTable, which backs ten small, fully-loaded lists; #1268 (instance users) and #1269 (org roles) ask for the same three things next.💡 Solution
SimpleBrowseDataTablegains two opt-in props, and the org users table turns both on:filter={{ label, placeholder }}adds a search box. It drives TanStack's global filter: case-insensitive substring match across every string or number column, except a column that setsenableGlobalFilter: false.paginatedpages the rows 20 at a time with the browse table's existingTablePagination. The pager only appears once the rows outnumber the smallest page (20), so a short list looks as it does today.datakeeps the current page, clamped to the last page if the list shrank. TanStack's default resets to page 1 on every newdataarray, and this list polls every 10s (getOrganizationRolesQueryOptions) while central manager rewrites a member'slastAccessedAton everyGET /User/current(central-managersrc/resources/user/User.js:149), so the default would keep bouncing an admin off page 2.On the org users table every data column is now sortable. Roles becomes an accessor column (the same alphabetical join as before) so it can be sorted and searched, and User Id is left out of the search since an opaque id matches many short queries. The placeholder says what is searched: "Filter by email, name, role or status".
A table that opts into neither prop renders exactly what it did: no search box, no pager, and
manualFiltering/manualPaginationkeep both new row models from running.⚖️ Alternatives
studioTableFeaturesand opt out per table (themanualSorting: truepattern AGENTS.md describes): rejected. Unlike sorting, the paginated row model is never a no-op (its default state is a 10-row page), so it would truncate the browse table (20–250 row server pages), the logs table,DataTableand the seven non-optingSimpleBrowseDataTablelists unless every one of them rememberedmanualPagination: true.better-alternative-exists: also bypass the filtered row model withmanualFiltering: !filterfor non-opting tables (adopted). Its claim thatfilterFns: { includesString }must be registered for the filter to work was overruled: the'auto'global filter resolvesfilterFn_includesStringdirectly (table-coreglobal-filtering/globalFilteringFeature.utils.js,table_getGlobalAutoFilterFn), and the tests filter through the real row model with no registry.ClustersListfilters on every keystroke the same way; the bundle cost of importingTablePagination(codex, then withdrawn by both legs in round 2), since it is already in the eager entry chunk throughsrc/features/instance/routes.ts→ databases routes →TableView.🔧 Changes
src/lib/table.ts:globalFilteringFeatureandrowPaginationFeaturejoin the shared feature set (APIs and state only, imported here), and the newstudioClientTableFeaturesadds their row models for tables that page and filter their own data.src/components/SimpleBrowseDataTable.tsx: thefilterandpaginatedprops; sorting, filter and page state held in React state so a sort or filter change can return to page 1 whileautoResetPageIndex: falsekeeps a refresh from doing so; a layout effect that clamps the page when the rows shrink; the search box; and the pager.src/features/instance/databases/components/TablePagination.tsx: exportsPAGE_SIZE_OPTIONS, so the shared table's page size and pager threshold come from the pager's own smallest option.src/features/organization/users/constants/tableDefinition.tsx: sortable columns, Roles as an accessor column, User Id excluded from the search.src/features/organization/users/index.tsx: opts the table intofilterandpaginated.✅ Verification
End-to-end route: component integration in jsdom with TanStack's real row models, the real
TablePaginationand the real org users column definitions. Not browser-verified, since the only authenticated dev origin (port 5173) was shared with other sessions. The authed Playwright spec (e2e/tests/org-users.authed.spec.ts) still asserts the Email/Roles/Status headers, which remain, but it was not run (no test account here) and it does not exercise the new controls.src/components/SimpleBrowseDataTable.test.tsx: a table that does not opt in renders all 45 rows with no search box or pager; case-insensitive filtering that skips anenableGlobalFilter: falsecolumn and shows "No results."; 20-row pages; no pager at exactly 20 rows; sort-then-page ordering; re-sort and re-filter return to page 1 and count only matching rows (no console errors); a refresh keeps the page; shrinking data moves to the new last page.src/features/organization/users/constants/tableDefinition.test.tsx: filtering by name, role and status but not id; sorting by Roles; the roles text is alphabetical without reordering the user's ownrolesarray.manualPagination: !paginated; forcingmanualFiltering: true; re-enabling the User Id column in the search;autoResetPageIndex: true; removing the page reset from sort, and from filter; disabling the clamp.tsc -bexit 0;oxlintexit 0;dprint checkexit 0; commitlint passed.?? []for sorting, whyautoResetPageIndexis off, why the tests await TanStack's microtask).Stacked follow-ups: #1835 (instance users, #1268) is based on this branch, and #1837 (org roles, #1269) on that one.
Closes #1264
🤖 Generated with Claude Code; posted via @dawsontoth.
Related PRs: #1793 independent (merged; it removed SimpleBrowseDataTable's unused, never-read pagination props, and this adds paging the component holds itself)
Complexity: medium
Review-Coverage: authored=claude; ran=codex,gemini; blocked=cursor-composer(no-receipt),domain(auth),cursor-muse(no-receipt); declined=cursor-grok,cursor-kimi; rounds=2; full=1 @ 28146a1
Review-Attention: read ~5m (raised: degraded review) @ 28146a1