FLPATH-4285 | [DCM] Server-side pagination not implemented - #4155
FLPATH-4285 | [DCM] Server-side pagination not implemented#4155asmasarw wants to merge 4 commits into
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
PR Summary by QodoAdd server-side cursor pagination across all DCM tabs
AI Description
Diagram
High-Level Assessment
Files changed (34)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4155 +/- ##
==========================================
+ Coverage 58.06% 58.19% +0.13%
==========================================
Files 2411 2415 +4
Lines 96367 96541 +174
Branches 26856 26895 +39
==========================================
+ Hits 55953 56181 +228
+ Misses 40215 40130 -85
- Partials 199 230 +31
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code Review by Qodo
1.
|
rhdh-qodo-merge
Bot
added
documentation
f14477d to
9c0b53d
Compare
asmasarw
changed the title
asmasarw
changed the title
3c26133 to
d61c0d0
Compare
mareklibra
left a comment
mareklibra
left a comment
There was a problem hiding this comment.
-
No
usePaginatedCrudTabunit tests, nobuildPaginationQuerytests, no search+resetCursorregression, andProvidersClient.teststill only asserts unpaginatedGET /providers. Worth adding at least query-param client coverage and the search/Next regression above. -
Cursor controls default to
[5, 15, 25]while the Table pager uses[5, 10, 25]. Align them unless 15 is intentional. -
Providers/Catalog loadFns re-fetch service-types/catalog-items (capped at 25) on every Next/Prev. Load dropdown options once (separate effect) to avoid redundant traffic.
| fetch(); | ||
| }, [fetch]); | ||
|
|
||
| const resetCursor = useCallback(() => { |
There was a problem hiding this comment.
resetCursor() clears nextToken without a refetch, and Service Types / Resources call it on every search change. After any search interaction, Next is permanently disabled until remount/page-size change (even after clearing the query) because the token is never restored.
Please either:
- Remove these
resetCursor()-on-search effects (keep Prev/Next tied to the loaded page), or - Call
resetToFirstPage()so search resets to page 1 with a real reload.
Also add a regression test: load page 1 with nextPageToken, change search, clear search, assert Next is still enabled / token preserved or page 1 re-fetched.
|
|
||
| // When the search changes, reset cursor navigation state (no API call | ||
| // needed — search filters the current page client-side). | ||
| const handleSearchChange = useCallback( |
There was a problem hiding this comment.
It clears currentTokenRef + token stack but leaves nextPageToken and the displayed page data as-is. Comments say this “resets to page 1”, but the user can still be viewing page N rows with Next enabled. Prefer not touching cursor state for client-side filter, or call a real resetToFirstPage + reload.
| items: r.providers ?? [], | ||
| nextPageToken: r.next_page_token, | ||
| })), | ||
| catalogApi |
There was a problem hiding this comment.
Capping form dropdowns at max_page_size: 25 does not “prevent incomplete lists” (changeset wording). API default is already 100; 25 truncates sooner with no “load more” / typeahead. Prefer 100, full pagination of options, or an explicit incomplete-list indicator.
| const optsRef = useRef(options); | ||
| optsRef.current = options; | ||
|
|
||
| const crud = useCrudTab<T, F>({ |
There was a problem hiding this comment.
usePaginatedCrudTab calls usePersistedPageSize(storageKey) and then spreads ...options (including storageKey) into useCrudTab, which calls usePersistedPageSize again on the same key. Two React states back the same localStorage entry; only the outer one is updated on page-size change. Destructure storageKey out before spreading into useCrudTab.
There was a problem hiding this comment.
items.length === 0 always renders the illustration empty-state and drops cursor controls. After deleting the last row on page 2+, or an empty cursor page with hasPrev, the user cannot go Previous. In cursor mode, treat “empty current page + hasPrev” as an empty table with pagination (or auto-goPrev), not the global empty state.
| } | ||
|
|
||
| /** Props that can be passed directly to {@link DcmCrudTabLayout}'s `cursorPagination`. */ | ||
| export interface CursorPaginationProps { |
There was a problem hiding this comment.
Three near-identical pagination prop types will drift. Export one from CursorPaginationControls.tsx and reuse it.
| }, | ||
| })); | ||
|
|
||
| export type CursorPaginationControlsProps = Readonly<{ |
There was a problem hiding this comment.
Three near-identical pagination prop types will drift. Export one from CursorPaginationControls.tsx and reuse it.
| ); | ||
| } | ||
|
|
||
| export type CursorPaginationProps = Readonly<{ |
There was a problem hiding this comment.
Three near-identical pagination prop types will drift. Export one from CursorPaginationControls.tsx and reuse it.
|
|
||
| Previously only Service Types, Catalog Items, and Catalog Item Instances fetched data page-by-page from the backend. Providers, Policies, and Resources loaded everything in a single call and silently lost records beyond the first page. | ||
|
|
||
| - **Providers** and **Policies** — converted to the `useCrudTab` + manual token-stack pattern (same as Catalog Items). Next / Previous buttons appear below the table; search resets to page 1 client-side without an extra round-trip. |
There was a problem hiding this comment.
Changeset still says Providers/Policies use a “manual token-stack around useCrudTab”, but the code uses usePaginatedCrudTab. Please align the changeset with the final design.
mareklibra
left a comment
mareklibra
left a comment
There was a problem hiding this comment.
Adding new comments. Something is left from the last time.
| nextPageToken: r.next_page_token, | ||
| })), | ||
| catalogApi | ||
| .listCatalogItems({ max_page_size: 25 }) |
There was a problem hiding this comment.
Catalog Items / Providers were moved to a one-shot mount fetch with
max_page_size: 100, but Catalog Item Instances still does
listCatalogItems({ max_page_size: 25 }) inside loadFn via
Promise.all — so every Next/Prev re-hits catalog-items and the create
dropdown silently truncates after 25.
Please mirror Providers/Catalog Items: load catalog items once in a
useEffect with max_page_size: 100 (or paginate/typeahead), and keep
loadFn to instances only.
There was a problem hiding this comment.
Sort of:
useEffect(() => {
catalogApi
.listCatalogItems({ max_page_size: 100 })
.then(r => setCatalogItems(r.results ?? []))
.catch(() => {});
}, [catalogApi]);
const crud = usePaginatedCrudTab({
loadFn: ({ pageToken, pageSize: ps }) =>
catalogApi
.listCatalogItemInstances({ page_token: pageToken, max_page_size: ps })
.then(r => ({
items: r.results ?? [],
nextPageToken: r.next_page_token,
})),
// ...
});
There was a problem hiding this comment.
Also update the changeset line that only mentions Providers/Catalog Items dropdowns.
| * ... | ||
| * /> | ||
| */ | ||
| export function usePaginatedCrudTab<T, F extends Record<string, unknown>>( |
There was a problem hiding this comment.
usePaginatedCrudTab is the shared cursor stack for four CRUD tabs, but
there’s still no dedicated unit test file. Tab tests help, but they won’t
catch hook regressions (token stack / page-size reset / storageKey
stripping) as cheaply.
Please add usePaginatedCrudTab.test.ts covering: initial load params,
goNext/goPrev token passing, handlePageSizeChange → page 1 reload, and
search leaving cursor/next token untouched.
| catalogApi | ||
| .listServiceTypes({ max_page_size: 100 }) | ||
| .then(r => setServiceTypes(r.results ?? [])) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
.catch(() => {}) on the mount-time service-types fetch leaves create/edit
forms with an empty dropdown and no error. At least surface a non-blocking
alert or log via the app logger so failures aren’t silent.
| * Backstage Table's built-in pager. The table is rendered with `paging: | ||
| * false` and {@link CursorPaginationControls} is shown below it. | ||
| */ | ||
| cursorPagination?: { |
There was a problem hiding this comment.
.catch(() => {}) on the mount-time service-types fetch leaves create/edit
forms with an empty dropdown and no error. At least surface a non-blocking
alert or log via the app logger so failures aren’t silent.
| /> | ||
| {cursorPagination ? ( | ||
| <CursorPaginatedTable<T> | ||
| data={filtered} |
There was a problem hiding this comment.
In cursor mode the table now uses data={filtered} (current server page).
The card title still uses filtered.length, so the number is page-local,
not a dataset total. Consider dropping the count, labeling it as “showing”,
or only showing it when a real total exists.
|
2 participants
Summary
Add server-side cursor pagination to all DCM tabs.
Previously only Service Types, Catalog Items, and Instances fetched data page-by-page. Providers, Policies, and Resources loaded everything in a single call, silently dropping records beyond the first page.
What changed
usePaginatedFetch(read-only layout preserved).dcm-common— extractedbuildPaginationQueryutility;listProvidersandlistPoliciesnow accept optionalPaginationParams;next_page_tokenmade optional on list response types to match real backend behaviour.max_page_size: 25.ProvidersTabContent.test.tsxandResourcesTabContent.test.tsx; extendedPoliciesTabContent.test.tsxwith cursor navigation tests.common.previousPage/common.nextPageto all locale files.