refactor: add useListPage hook for standardized list management#381
Conversation
…across table components and update document source provider types
📝 WalkthroughWalkthroughThis PR introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/platform/types/documents.ts (1)
21-32: 🧹 Nitpick | 🔵 TrivialConsider adding a JSDoc comment to
isDirectlySelected.The other new fields (
ragIndexedAt,ragError,teamTags,createdBy,createdByName) all have JSDoc comments explaining their purpose.isDirectlySelectedis the only one missing a description — a brief comment clarifying what "directly selected" means (e.g., explicitly selected for sync vs. included via parent folder) would help future readers.📝 Suggested documentation
+ /** Whether this document was explicitly selected (vs. included via parent folder) */ isDirectlySelected?: boolean;services/platform/app/features/documents/components/documents-client.tsx (1)
204-215: 🧹 Nitpick | 🔵 Trivial
handleDocumentClickandhandleRowClickhave overlapping logic.Both handlers dispatch folder navigation and file preview. The
stopPropagation()in the column button prevents double-firing, which is correct. The duplication is minor here, but if more row-interaction behavior is added later, consider unifying the dispatch logic into a single helper.
🤖 Fix all issues with AI agents
In `@services/platform/app/features/automations/executions/executions-client.tsx`:
- Around line 55-59: The destructured binding variables: _variables inside the
useMemo return is unused—remove the unused destructuring to clean up dead code;
update the useMemo destructure so it only extracts metadata and parsedVariables
(or any other actually used symbols) and remove the _variables binding and any
related unused local identifier to avoid linter warnings (locate the useMemo
block where metadata, parsedVariables, variables: _variables are declared).
In `@services/platform/app/features/documents/components/documents-client.tsx`:
- Around line 147-151: getRowClassName currently returns 'cursor-pointer' only
for rows where row.original.type === 'folder', but handleRowClick navigates for
both folders and files so file rows are clickable without cursor feedback;
either add 'cursor-pointer' for file rows as well by returning the class for
both types in getRowClassName (reference getRowClassName and Row<DocumentItem>)
or, if the omission was intentional, add a short inline comment next to
getRowClassName explaining that files are intentionally not styled to encourage
use of the name-column button; ensure the same change or comment is applied to
the other instance noted around lines 182-191.
- Around line 109-137: The nested ternary for dataSource.data is redundant;
replace the expression with a simple check on documentsResult (e.g., data:
documentsResult === undefined ? undefined : allDocuments) inside the useListPage
call so dataSource uses either undefined when documentsResult is missing or
allDocuments otherwise; update the dataSource block in the useListPage
invocation (referencing useListPage, dataSource, allDocuments, and
documentsResult) accordingly.
In `@services/platform/app/hooks/use-list-page.ts`:
- Around line 12-18: The PaginatedDataSource.isLoading property is declared but
unused inside the hook; update the PaginatedDataSource interface in
use-list-page.ts to either remove isLoading or add a clarifying comment that it
is a pass-through from the Convex SDK and kept for consumers who spread
usePaginatedQuery results; locate the interface by the symbol
PaginatedDataSource and the hook logic where dataSource.status and
usePaginatedQuery are referenced (the computed isLoading is derived from
dataSource.status) and make the change so the interface matches actual usage and
intent.
- Around line 208-219: The handleLoadMore callback and the hasMore derivation
recreate every render because they depend on the unstable dataSource object;
extract the specific properties you need (dataSource.type, dataSource.status and
the loadMore function) into stable refs or memoized values and use those in
handleLoadMore and hasMore instead of the whole dataSource. Concretely: create a
ref for loadMore (e.g., loadMoreRef.current = dataSource.loadMore) updated in an
effect, memoize type/status (or read them once into local constants via
useMemo), then update the useCallback for handleLoadMore to depend only on
pageSize, displayCount/processed.length and the stable refs/memos (and similarly
change the hasMore logic to reference the memoized type/status) so
handleLoadMore is not recreated every render.
- Around line 247-265: The controlled filter configs returned by the useMemo
(filterConfigs) branch for isControlledFilters(filters) are not wrapped to call
resetDisplayCount(), causing pagination not to reset when a controlled filter
changes; update the code that returns filters.configs so each config's onChange
is wrapped to first call the original onChange (if any) and then call
resetDisplayCount(), mirroring how controlled search onChange is wrapped, and
ensure you reference the same resetDisplayCount, isControlledFilters,
filters.configs, and the filter config's onChange to implement the wrapper.
- Around line 160-187: The useMemo computing processed reuses the unstable
search object in its deps which forces recomputation on every render; instead,
derive and store the stable primitives needed from search (e.g., a
memoized/normalized searchFields array and any search mode/placeholder flags)
near the top of the hook (useMemo/useRef) and use those stable values in the
processed useMemo deps; do the same for filterConfigs (derive a stable list of
filter field names or a hash) and replace search / filters in the dependency
arrays with those stable primitives so filterByTextSearch, isManagedSearch, and
filterByFields are only re-run when the actual config values change.
| const { | ||
| metadata, | ||
| parsedVariables, | ||
| variables: _variables, | ||
| } = useMemo(() => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: _variables is destructured but unused.
The variables: _variables destructuring suggests dead code. If variables is no longer needed from this memo, the destructured binding can be removed entirely.
Simplify destructuring
- const {
- metadata,
- parsedVariables,
- variables: _variables,
- } = useMemo(() => {
+ const { metadata, parsedVariables } = useMemo(() => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { | |
| metadata, | |
| parsedVariables, | |
| variables: _variables, | |
| } = useMemo(() => { | |
| const { metadata, parsedVariables } = useMemo(() => { |
🤖 Prompt for AI Agents
In `@services/platform/app/features/automations/executions/executions-client.tsx`
around lines 55 - 59, The destructured binding variables: _variables inside the
useMemo return is unused—remove the unused destructuring to clean up dead code;
update the useMemo destructure so it only extracts metadata and parsedVariables
(or any other actually used symbols) and remove the _variables binding and any
related unused local identifier to avoid linter warnings (locate the useMemo
block where metadata, parsedVariables, variables: _variables are declared).
| const list = useListPage({ | ||
| dataSource: { | ||
| type: 'query', | ||
| data: | ||
| allDocuments.length > 0 | ||
| ? allDocuments | ||
| : documentsResult === undefined | ||
| ? undefined | ||
| : allDocuments, | ||
| }, | ||
| pageSize: PAGE_SIZE, | ||
| search: { | ||
| value: query, | ||
| onChange: (value: string) => { | ||
| setQuery(value); | ||
| navigate({ | ||
| to: '/dashboard/$id/documents', | ||
| params: { id: organizationId }, | ||
| search: { | ||
| query: value.trim() || undefined, | ||
| folderPath: currentFolderPath, | ||
| doc: docId, | ||
| }, | ||
| }); | ||
| }, | ||
| placeholder: tDocuments('searchPlaceholder'), | ||
| }, | ||
| getRowId: (row) => row.id, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify the data source ternary.
The nested ternary at lines 112–117 is harder to read than necessary. Since allDocuments is derived from documentsResult?.page ?? [], when documentsResult is undefined, allDocuments is always [] (length 0). The outer allDocuments.length > 0 branch is therefore redundant — whenever it's true, documentsResult is guaranteed to be defined, so the simpler form produces the same result.
♻️ Proposed simplification
const list = useListPage({
dataSource: {
type: 'query',
- data:
- allDocuments.length > 0
- ? allDocuments
- : documentsResult === undefined
- ? undefined
- : allDocuments,
+ data: documentsResult === undefined ? undefined : allDocuments,
},
pageSize: PAGE_SIZE,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const list = useListPage({ | |
| dataSource: { | |
| type: 'query', | |
| data: | |
| allDocuments.length > 0 | |
| ? allDocuments | |
| : documentsResult === undefined | |
| ? undefined | |
| : allDocuments, | |
| }, | |
| pageSize: PAGE_SIZE, | |
| search: { | |
| value: query, | |
| onChange: (value: string) => { | |
| setQuery(value); | |
| navigate({ | |
| to: '/dashboard/$id/documents', | |
| params: { id: organizationId }, | |
| search: { | |
| query: value.trim() || undefined, | |
| folderPath: currentFolderPath, | |
| doc: docId, | |
| }, | |
| }); | |
| }, | |
| placeholder: tDocuments('searchPlaceholder'), | |
| }, | |
| getRowId: (row) => row.id, | |
| }); | |
| const list = useListPage({ | |
| dataSource: { | |
| type: 'query', | |
| data: documentsResult === undefined ? undefined : allDocuments, | |
| }, | |
| pageSize: PAGE_SIZE, | |
| search: { | |
| value: query, | |
| onChange: (value: string) => { | |
| setQuery(value); | |
| navigate({ | |
| to: '/dashboard/$id/documents', | |
| params: { id: organizationId }, | |
| search: { | |
| query: value.trim() || undefined, | |
| folderPath: currentFolderPath, | |
| doc: docId, | |
| }, | |
| }); | |
| }, | |
| placeholder: tDocuments('searchPlaceholder'), | |
| }, | |
| getRowId: (row) => row.id, | |
| }); |
🤖 Prompt for AI Agents
In `@services/platform/app/features/documents/components/documents-client.tsx`
around lines 109 - 137, The nested ternary for dataSource.data is redundant;
replace the expression with a simple check on documentsResult (e.g., data:
documentsResult === undefined ? undefined : allDocuments) inside the useListPage
call so dataSource uses either undefined when documentsResult is missing or
allDocuments otherwise; update the dataSource block in the useListPage
invocation (referencing useListPage, dataSource, allDocuments, and
documentsResult) accordingly.
| const getRowClassName = useCallback( | ||
| (row: Row<DocumentItem>) => | ||
| row.original.type === 'folder' ? 'cursor-pointer' : '', | ||
| [], | ||
| ); |
There was a problem hiding this comment.
File rows are clickable but lack cursor-pointer.
handleRowClick navigates on both folder and file rows, but getRowClassName only applies cursor-pointer to folders. This means clicking anywhere on a file row opens the preview, yet the cursor doesn't signal clickability. If this is intentional (to steer users toward the name-column button for files), consider adding a brief comment. Otherwise, extend the style to files as well:
🖱️ Proposed fix
const getRowClassName = useCallback(
(row: Row<DocumentItem>) =>
- row.original.type === 'folder' ? 'cursor-pointer' : '',
+ row.original.type === 'folder' || row.original.type === 'file'
+ ? 'cursor-pointer'
+ : '',
[],
);Also applies to: 182-191
🤖 Prompt for AI Agents
In `@services/platform/app/features/documents/components/documents-client.tsx`
around lines 147 - 151, getRowClassName currently returns 'cursor-pointer' only
for rows where row.original.type === 'folder', but handleRowClick navigates for
both folders and files so file rows are clickable without cursor feedback;
either add 'cursor-pointer' for file rows as well by returning the class for
both types in getRowClassName (reference getRowClassName and Row<DocumentItem>)
or, if the omission was intentional, add a short inline comment next to
getRowClassName explaining that files are intentionally not styled to encourage
use of the name-column button; ensure the same change or comment is applied to
the other instance noted around lines 182-191.
| interface PaginatedDataSource<TData> { | ||
| type: 'paginated'; | ||
| results: TData[] | undefined; | ||
| status: 'LoadingFirstPage' | 'CanLoadMore' | 'LoadingMore' | 'Exhausted'; | ||
| loadMore: (numItems: number) => void; | ||
| isLoading: boolean; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
PaginatedDataSource.isLoading is declared but never read by the hook.
The hook computes isLoading from dataSource.status (Line 136-137) for paginated sources. The isLoading field in the interface is present only because consumers spread usePaginatedQuery results into it. Consider removing it or adding a comment that it's pass-through from the Convex SDK.
🤖 Prompt for AI Agents
In `@services/platform/app/hooks/use-list-page.ts` around lines 12 - 18, The
PaginatedDataSource.isLoading property is declared but unused inside the hook;
update the PaginatedDataSource interface in use-list-page.ts to either remove
isLoading or add a clarifying comment that it is a pass-through from the Convex
SDK and kept for consumers who spread usePaginatedQuery results; locate the
interface by the symbol PaginatedDataSource and the hook logic where
dataSource.status and usePaginatedQuery are referenced (the computed isLoading
is derived from dataSource.status) and make the change so the interface matches
actual usage and intent.
| const processed = useMemo(() => { | ||
| let data = [...rawData]; | ||
|
|
||
| // Apply managed text search | ||
| if (search && isManagedSearch<TData>(search) && searchValue) { | ||
| data = filterByTextSearch( | ||
| data, | ||
| searchValue, | ||
| search.fields as (keyof TData)[], | ||
| ); | ||
| } | ||
|
|
||
| // Apply managed field filters | ||
| if (filterValues) { | ||
| const activeFilters = Object.entries(filterValues) | ||
| .filter(([, values]) => values.length > 0) | ||
| .map(([field, values]) => ({ | ||
| field: field as keyof TData, | ||
| values: new Set(values), | ||
| })); | ||
|
|
||
| if (activeFilters.length > 0) { | ||
| data = filterByFields(data, activeFilters); | ||
| } | ||
| } | ||
|
|
||
| return data; | ||
| }, [rawData, searchValue, filterValues, search]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unstable search reference in useMemo deps defeats memoization.
search (and filters in other memos) are typically inline object literals from consumers, so they are new references on every render. Including search in the dependency array of processed means the array is re-copied and re-filtered on every render, even when the actual search config hasn't changed. The same pattern affects searchConfig (Line 222) and filterConfigs (Line 247).
Consider extracting the stable primitives needed (e.g., field names, placeholder) from the options at the top of the hook and using those as deps instead. For example:
Example: stabilize the fields reference for managed search
+ // Stabilize search fields reference
+ const searchFieldsKey =
+ search && isManagedSearch(search) ? search.fields.join(',') : '';
+
const processed = useMemo(() => {
let data = [...rawData];
if (search && isManagedSearch<TData>(search) && searchValue) {
data = filterByTextSearch(
data,
searchValue,
search.fields as (keyof TData)[],
);
}
...
return data;
- }, [rawData, searchValue, filterValues, search]);
+ }, [rawData, searchValue, filterValues, searchFieldsKey]);Alternatively, document that consumers should memoize the search / filters options they pass to the hook.
🤖 Prompt for AI Agents
In `@services/platform/app/hooks/use-list-page.ts` around lines 160 - 187, The
useMemo computing processed reuses the unstable search object in its deps which
forces recomputation on every render; instead, derive and store the stable
primitives needed from search (e.g., a memoized/normalized searchFields array
and any search mode/placeholder flags) near the top of the hook (useMemo/useRef)
and use those stable values in the processed useMemo deps; do the same for
filterConfigs (derive a stable list of filter field names or a hash) and replace
search / filters in the dependency arrays with those stable primitives so
filterByTextSearch, isManagedSearch, and filterByFields are only re-run when the
actual config values change.
| // 9. handleLoadMore | ||
| const handleLoadMore = useCallback(() => { | ||
| if (dataSource.type === 'paginated') { | ||
| if ( | ||
| displayCount >= processed.length && | ||
| dataSource.status === 'CanLoadMore' | ||
| ) { | ||
| dataSource.loadMore(pageSize); | ||
| } | ||
| } | ||
| setDisplayCount((prev) => prev + pageSize); | ||
| }, [dataSource, displayCount, processed.length, pageSize]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
handleLoadMore dependency on dataSource causes recreation every render.
The dataSource object passed by consumers (e.g., { type: 'paginated', ...paginatedResult }) is a new reference each render. Since it's in the useCallback deps, handleLoadMore is recreated on every render, making the useCallback wrapper ineffective. The same applies to the hasMore derivation at Lines 196-201 which references dataSource directly.
For handleLoadMore, consider extracting only the needed fields (dataSource.type, dataSource.status, dataSource.loadMore) into stable references or using useRef for the loadMore function to avoid unnecessary recreations.
🤖 Prompt for AI Agents
In `@services/platform/app/hooks/use-list-page.ts` around lines 208 - 219, The
handleLoadMore callback and the hasMore derivation recreate every render because
they depend on the unstable dataSource object; extract the specific properties
you need (dataSource.type, dataSource.status and the loadMore function) into
stable refs or memoized values and use those in handleLoadMore and hasMore
instead of the whole dataSource. Concretely: create a ref for loadMore (e.g.,
loadMoreRef.current = dataSource.loadMore) updated in an effect, memoize
type/status (or read them once into local constants via useMemo), then update
the useCallback for handleLoadMore to depend only on pageSize,
displayCount/processed.length and the stable refs/memos (and similarly change
the hasMore logic to reference the memoized type/status) so handleLoadMore is
not recreated every render.
| const filterConfigs = useMemo((): FilterConfig[] | undefined => { | ||
| if (!filters) return undefined; | ||
|
|
||
| if (isControlledFilters(filters)) { | ||
| return filters.configs; | ||
| } | ||
|
|
||
| return filters.definitions.map((def) => ({ | ||
| key: def.key, | ||
| title: def.title, | ||
| options: def.options, | ||
| grid: def.grid, | ||
| selectedValues: managedFilterStates[def.key] ?? [], | ||
| onChange: (values: string[]) => { | ||
| setManagedFilterStates((prev) => ({ ...prev, [def.key]: values })); | ||
| resetDisplayCount(); | ||
| }, | ||
| })); | ||
| }, [filters, managedFilterStates, resetDisplayCount]); |
There was a problem hiding this comment.
Controlled filter configs are not wrapped to reset displayCount, unlike controlled search.
Controlled search onChange is wrapped (Line 238-241) to call resetDisplayCount(), but controlled filter configs are returned as-is (Line 251). This means after a user has scrolled and increased displayCount, changing a controlled filter won't reset pagination to the first page, creating a UX inconsistency between search and filter interactions.
Proposed fix: wrap controlled filter onChange handlers
if (isControlledFilters(filters)) {
- return filters.configs;
+ return filters.configs.map((config) => ({
+ ...config,
+ onChange: (values: string[]) => {
+ config.onChange(values);
+ resetDisplayCount();
+ },
+ }));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const filterConfigs = useMemo((): FilterConfig[] | undefined => { | |
| if (!filters) return undefined; | |
| if (isControlledFilters(filters)) { | |
| return filters.configs; | |
| } | |
| return filters.definitions.map((def) => ({ | |
| key: def.key, | |
| title: def.title, | |
| options: def.options, | |
| grid: def.grid, | |
| selectedValues: managedFilterStates[def.key] ?? [], | |
| onChange: (values: string[]) => { | |
| setManagedFilterStates((prev) => ({ ...prev, [def.key]: values })); | |
| resetDisplayCount(); | |
| }, | |
| })); | |
| }, [filters, managedFilterStates, resetDisplayCount]); | |
| const filterConfigs = useMemo((): FilterConfig[] | undefined => { | |
| if (!filters) return undefined; | |
| if (isControlledFilters(filters)) { | |
| return filters.configs.map((config) => ({ | |
| ...config, | |
| onChange: (values: string[]) => { | |
| config.onChange(values); | |
| resetDisplayCount(); | |
| }, | |
| })); | |
| } | |
| return filters.definitions.map((def) => ({ | |
| key: def.key, | |
| title: def.title, | |
| options: def.options, | |
| grid: def.grid, | |
| selectedValues: managedFilterStates[def.key] ?? [], | |
| onChange: (values: string[]) => { | |
| setManagedFilterStates((prev) => ({ ...prev, [def.key]: values })); | |
| resetDisplayCount(); | |
| }, | |
| })); | |
| }, [filters, managedFilterStates, resetDisplayCount]); |
🤖 Prompt for AI Agents
In `@services/platform/app/hooks/use-list-page.ts` around lines 247 - 265, The
controlled filter configs returned by the useMemo (filterConfigs) branch for
isControlledFilters(filters) are not wrapped to call resetDisplayCount(),
causing pagination not to reset when a controlled filter changes; update the
code that returns filters.configs so each config's onChange is wrapped to first
call the original onChange (if any) and then call resetDisplayCount(), mirroring
how controlled search onChange is wrapped, and ensure you reference the same
resetDisplayCount, isControlledFilters, filters.configs, and the filter config's
onChange to implement the wrapper.
1 participant
Summary
useListPagehook that centralizes data source handling, client-side search/filtering, pagination, and infinite scroll logic into a single reusable abstractiononRowClickandrowClassNameprops toDataTablefor row-level interaction supportTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements