-
Notifications
You must be signed in to change notification settings - Fork 29
OCMUI-3883: Add pagination to cluster transfer list #258
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideImplements server-backed, globally managed pagination for the cluster transfer list, wires it into shared view state, and adjusts clusters page header behavior and page titles for the tabbed clusters view. Sequence diagram for refreshed behavior of clusters page header across tabssequenceDiagram
actor User
participant Clusters as Clusters
participant ClustersPageHeader as ClustersPageHeader
participant useFetchClusters as useFetchClusters
participant useFetchClusterTransferDetail as useFetchClusterTransferDetail
User->>Clusters: Navigate to Clusters route
Clusters->>ClustersPageHeader: Render ClustersPageHeader(activeTabKey)
ClustersPageHeader->>useFetchClusters: useFetchClusters(skipClustersLoading, includeTransfers)
ClustersPageHeader->>useFetchClusterTransferDetail: useFetchClusterTransferDetail(params)
Note over User,ClustersPageHeader: User clicks Refresh on List tab
User->>ClustersPageHeader: Click Refresh (activeTabKey = list)
ClustersPageHeader->>useFetchClusters: refetch()
ClustersPageHeader->>useFetchClusterTransferDetail: refetchClusterTransferDetail()
Note over User,ClustersPageHeader: User clicks Refresh on Transfers tab
User->>ClustersPageHeader: Click Refresh (activeTabKey = transfers)
ClustersPageHeader-xuseFetchClusters: refetch() (not called)
ClustersPageHeader->>useFetchClusterTransferDetail: refetchClusterTransferDetail()
Sequence diagram for cluster transfer list pagination and data fetchingsequenceDiagram
actor User
participant ClusterTransferList as ClusterTransferList
participant Pagination as ClusterTransferListTablePagination
participant ViewPaginationRow as ViewPaginationRow
participant GlobalState as GlobalState_viewOptions
participant useFetchClusterTransfer as useFetchClusterTransfer
participant ReactQuery as ReactQuery_useQuery
participant AccountsService as accountsService
participant Backend as AccountsMgmtAPI
User->>ClusterTransferList: Open Transfers tab
ClusterTransferList->>GlobalState: read viewOptions[CLUSTER_TRANSFER_VIEW]
ClusterTransferList->>useFetchClusterTransfer: useFetchClusterTransfer(clusterExternalID, transferID, filter, showPendingTransfer)
useFetchClusterTransfer->>GlobalState: read viewOptions[CLUSTER_TRANSFER_VIEW]
useFetchClusterTransfer->>ReactQuery: useQuery(queryKey including viewOptions)
ReactQuery->>AccountsService: searchClusterTransfers(params)
activate AccountsService
AccountsService->>Backend: GET /cluster_transfers?search=filter&page=currentPage&size=pageSize&orderBy=orderBy
Backend-->>AccountsService: ClusterTransferList(total, items)
deactivate AccountsService
AccountsService-->>ReactQuery: data
ReactQuery-->>useFetchClusterTransfer: data
useFetchClusterTransfer-->>ClusterTransferList: { data, pendingTransfer, isLoading, isError, error }
ClusterTransferList->>Pagination: render ClusterTransferListTablePagination(viewType, viewOptions, variant)
Pagination->>ViewPaginationRow: ViewPaginationRow(viewType, currentPage, pageSize, totalCount, totalPages, variant, isDisabled)
ViewPaginationRow-->>User: Display pagination controls
User->>ViewPaginationRow: Change page or page size
ViewPaginationRow->>GlobalState: dispatch updateViewOptions(viewType, newCurrentPage, newPageSize)
GlobalState-->>useFetchClusterTransfer: viewOptions changed
useFetchClusterTransfer->>ReactQuery: useQuery rerun with new viewOptions in queryKey
ReactQuery->>AccountsService: searchClusterTransfers(params with new page and size)
AccountsService->>Backend: GET /cluster_transfers with updated paging
Backend-->>AccountsService: ClusterTransferList(total, items)
AccountsService-->>ReactQuery: data
ReactQuery-->>useFetchClusterTransfer: data
useFetchClusterTransfer->>GlobalState: dispatch onSetTotal(total, CLUSTER_TRANSFER_VIEW)
GlobalState-->>ClusterTransferList: updated viewOptions.totalCount and totalPages
ClusterTransferList-->>Pagination: re-render with updated totals
Updated class diagram for cluster transfer list, pagination, and related servicesclassDiagram
class ClusterTransferList {
- username: string
- viewOptions: ViewOptions
+ ClusterTransferList(hideRefreshButton: boolean)
}
class ClusterTransferPageHeader {
+ ClusterTransferPageHeader(showSpinner: boolean, isError: boolean, error: Error, refresh: function, hideRefreshButton: boolean, viewOptions: ViewOptions)
}
class ClusterTransferListTablePagination {
+ ClusterTransferListTablePagination(viewType: string, viewOptions: ViewOptions, isDisabled: boolean, variant: string)
}
class ViewPaginationRow {
+ ViewPaginationRow(viewType: string, currentPage: number, pageSize: number, totalCount: number, totalPages: number, variant: string, isDisabled: boolean)
}
class ViewOptions {
+ currentPage: number
+ pageSize: number
+ totalCount: number
+ totalPages: number
+ sorting: SortingOptions
}
class SortingOptions {
+ sortField: string
+ isAscending: boolean
}
class SearchClusterTransfersParams {
+ filter: string
+ page: number
+ size: number
+ orderBy: string
}
class AccountsService {
+ getClusterTransferByExternalID(clusterExternalID: string) ClusterTransferList
+ searchClusterTransfers(params: SearchClusterTransfersParams) ClusterTransferList
}
class useFetchClusterTransfer {
+ useFetchClusterTransfer(clusterExternalID: string, transferID: string, filter: string, showPendingTransfer: boolean) ClusterTransferHookResult
- viewType: string
- viewOptions: ViewOptions
}
class ClusterTransferHookResult {
+ data: any
+ pendingTransfer: any
+ isLoading: boolean
+ isError: boolean
+ error: Error
}
class ViewOptionsReducer {
+ initialState: ViewOptionsState
}
class ViewOptionsState {
+ CLUSTERS_VIEW: ViewOptions
+ ARCHIVED_CLUSTERS_VIEW: ViewOptions
+ CLUSTER_LOGS_VIEW: ViewOptions
+ OVERVIEW_VIEW: ViewOptions
+ OVERVIEW_EXPIRED_TRIALS: ViewOptions
+ CLUSTER_TRANSFER_VIEW: ViewOptions
}
class ClustersPageHeader {
+ ClustersPageHeader(activeTabKey: string)
- activeTabKey: string
- refresh(): void
}
class ClusterList {
+ ClusterList(showTabbedView: boolean)
- PAGE_TITLE: string
}
class Clusters {
+ Clusters()
}
ClusterTransferList --> ClusterTransferPageHeader : uses
ClusterTransferList --> useFetchClusterTransfer : calls
ClusterTransferList --> ClusterTransferListTablePagination : renders top and bottom
ClusterTransferPageHeader --> ClusterTransferListTablePagination : renders top
ClusterTransferListTablePagination --> ViewPaginationRow : wraps
useFetchClusterTransfer --> AccountsService : calls searchClusterTransfers
useFetchClusterTransfer --> ViewOptions : reads
ViewOptionsReducer --> ViewOptionsState : initializes
ClustersPageHeader --> useFetchClusterTransfer : calls refetchClusterTransferDetail
ClustersPageHeader --> ClusterList : coordinates with activeTabKey
Clusters --> ClustersPageHeader : renders with activeTabKey
ClusterList --> ClustersPageHeader : page title depends on showTabbedView
ViewOptionsState --> ViewOptions : CLUSTER_TRANSFER_VIEW uses shared view state
AccountsService --> SearchClusterTransfersParams : consumes
ViewOptions --> SortingOptions : has
useFetchClusterTransfer --> ClusterTransferHookResult : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In
useFetchClusterTransfer, the react-queryqueryKeydoes not include pagination/sorting state (e.g.,viewOptions.currentPage,viewOptions.pageSize,viewOptions.sorting), so changing pages or sort order will not trigger a refetch unless manually forced; consider adding these to the key to keep the data in sync with the controls. - The
useEffectthat dispatchesonSetTotalinuseFetchClusterTransferbails out whendata.data.totalis0, which means the ReduxtotalCountwill never be updated to zero; remove the!data.data.totalcheck (or explicitly check forundefined/null) so that empty result sets correctly update pagination state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useFetchClusterTransfer`, the react-query `queryKey` does not include pagination/sorting state (e.g., `viewOptions.currentPage`, `viewOptions.pageSize`, `viewOptions.sorting`), so changing pages or sort order will not trigger a refetch unless manually forced; consider adding these to the key to keep the data in sync with the controls.
- The `useEffect` that dispatches `onSetTotal` in `useFetchClusterTransfer` bails out when `data.data.total` is `0`, which means the Redux `totalCount` will never be updated to zero; remove the `!data.data.total` check (or explicitly check for `undefined`/`null`) so that empty result sets correctly update pagination state.
## Individual Comments
### Comment 1
<location> `src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransfer.tsx:48-54` </location>
<code_context>
}
- return accountsService.searchClusterTransfers(`id='${transferID}'`);
+
+ return accountsService.searchClusterTransfers({
+ filter: filter || `id='${transferID}'`,
+ page: viewOptions.currentPage || 1,
+ size: viewOptions.pageSize || 20,
+ orderBy: viewOptions.sorting.sortField
+ ? `${viewOptions.sorting.sortField} ${viewOptions.sorting.isAscending ? 'asc' : 'desc'}`
+ : 'updated_at desc',
+ });
},
</code_context>
<issue_to_address>
**issue (bug_risk):** Query key does not include pagination/sort, so page/sort changes won't trigger a refetch or cache separation.
The `queryKey` still only varies by `filter || clusterExternalID || transferID`, but the fetch now also depends on `viewOptions.currentPage`, `viewOptions.pageSize`, and `viewOptions.sorting`. As a result, different pages/sorts can reuse the same cache entry and won’t refetch on change. Please add the relevant pagination/sort values (or a derived subset) to the `queryKey` so React Query caches and invalidates per page/sort combination.
</issue_to_address>
### Comment 2
<location> `src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransfer.tsx:61-73` </location>
<code_context>
});
+ // Update Redux state with total count when data changes
+ useEffect(() => {
+ if (!data?.data || !data.data.total) return;
+ dispatch(onSetTotal(data.data.total, viewType));
+ }, [
+ data?.data,
+ dispatch,
+ viewOptions?.totalCount,
+ viewOptions?.pageSize,
+ viewOptions?.currentPage,
+ viewOptions?.totalPages,
+ viewType,
+ ]);
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Effect dependencies include viewOptions fields that aren't used, and may cause redundant dispatch cycles.
The effect only reads `data?.data`, `dispatch`, and `viewType`, but the dependency array also includes several `viewOptions` fields. If `onSetTotal` updates any of those, the effect will re-run after each dispatch and may dispatch again with the same `total`, potentially causing unnecessary renders or even a render loop if the reducer returns new objects each time. Please limit the dependencies to the values actually used in the effect body (e.g. `[data?.data, dispatch, viewType]`).
```suggestion
// Update Redux state with total count when data changes
useEffect(() => {
if (!data?.data || !data.data.total) return;
dispatch(onSetTotal(data.data.total, viewType));
}, [data?.data, dispatch, viewType]);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransfer.tsx
Outdated
Show resolved
Hide resolved
src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransfer.tsx
Outdated
Show resolved
Hide resolved
src/components/clusters/ClusterTransfer/ClusterTransferList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In
useFetchClusterTransfer, the React QueryqueryKeynow includes the entireviewOptionsobject; consider narrowing this to specific primitive fields (e.g.,currentPage,pageSize,sorting) to avoid unnecessary refetches or subtle bugs from object identity changes. - The pagination controls in
ClusterTransferListTablePaginationare disabled when!viewOptions.totalCount; this means they are unusable until the first successful fetch populatestotalCount, so you may want to base disabling purely on loading/error state instead to avoid the controls appearing inert on initial load.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useFetchClusterTransfer`, the React Query `queryKey` now includes the entire `viewOptions` object; consider narrowing this to specific primitive fields (e.g., `currentPage`, `pageSize`, `sorting`) to avoid unnecessary refetches or subtle bugs from object identity changes.
- The pagination controls in `ClusterTransferListTablePagination` are disabled when `!viewOptions.totalCount`; this means they are unusable until the first successful fetch populates `totalCount`, so you may want to base disabling purely on loading/error state instead to avoid the controls appearing inert on initial load.
## Individual Comments
### Comment 1
<location> `src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransfer.tsx:42` </location>
<code_context>
error,
refresh,
hideRefreshButton,
+ viewOptions,
}: {
showSpinner: boolean;
</code_context>
<issue_to_address>
**suggestion (performance):** Narrow the query key to only the fields that affect the query to avoid unnecessary refetches.
Using the full `viewOptions` in the `queryKey` will trigger refetches when any property changes, including derived response fields like `totalCount` and `totalPages`. This adds unnecessary network calls and cache churn. Consider including only the fields that affect the request, such as `currentPage`, `pageSize`, and the sorting parameters (e.g. `['fetchClusterTransfer', filterOrId, viewOptions.currentPage, viewOptions.pageSize, viewOptions.sorting.sortField, viewOptions.sorting.isAscending]`).
</issue_to_address>
### Comment 2
<location> `src/components/clusters/ClusterTransfer/ClusterTransferList.tsx:337` </location>
<code_context>
+ viewType={viewConstants.CLUSTER_TRANSFER_VIEW}
+ viewOptions={viewOptions}
+ variant="bottom"
+ isDisabled={isLoading || !viewOptions.totalCount}
+ />
+ </CardFooter>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align top and bottom pagination disabling logic and consider centralizing the condition.
The header pagination uses `showSpinner || !viewOptions.totalCount`, while the footer uses `isLoading || !viewOptions.totalCount`. If `showSpinner` and `isLoading` differ, the two pagers can end up with different enabled/disabled states. Consider computing a single `isPaginationDisabled` value in `ClusterTransferList` and passing it to both so their behavior stays consistent.
Suggested implementation:
```typescript
<CardFooter>
<ClusterTransferListTablePagination
viewType={viewConstants.CLUSTER_TRANSFER_VIEW}
viewOptions={viewOptions}
variant="bottom"
isDisabled={isPaginationDisabled}
/>
</CardFooter>
```
To fully implement the centralization:
1. Inside the `ClusterTransferList` component (above the `return`), define a common flag:
```ts
const isPaginationDisabled = showSpinner || !viewOptions.totalCount;
```
(Use whichever loading indicator you intend to be canonical—`showSpinner` per your comment—or adjust the expression as appropriate.)
2. Update the header pagination (the one that currently has `isDisabled={showSpinner || !viewOptions.totalCount}`) to:
```tsx
isDisabled={isPaginationDisabled}
```
These two changes will ensure both top and bottom paginations share one consistent disable condition computed in a single place.
</issue_to_address>
### Comment 3
<location> `src/components/clusters/ClusterTransfer/ClusterTransferListTablePagination.tsx:12` </location>
<code_context>
+ isDisabled?: boolean;
+ variant: 'bottom' | 'top';
+};
+const ClusterTransferListTablePagination = ({
+ viewType,
+ viewOptions,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the wrapper component by spreading `viewOptions` into `ViewPaginationRow` instead of mapping each pagination prop manually.
You can reduce the indirection/duplication here by at least avoiding the explicit field mapping from `viewOptions` to `ViewPaginationRow` props and let the type system keep them in sync.
Instead of manually threading each property:
```tsx
const ClusterTransferListTablePagination = ({
viewType,
viewOptions,
isDisabled,
variant,
}: ClusterTransferListTablePaginationProps) => (
<ViewPaginationRow
viewType={viewType}
currentPage={viewOptions.currentPage}
pageSize={viewOptions.pageSize}
totalCount={viewOptions.totalCount}
totalPages={viewOptions.totalPages}
variant={variant}
isDisabled={isDisabled}
/>
);
```
you can make the wrapper thinner and more future‑proof by spreading `viewOptions`:
```tsx
const ClusterTransferListTablePagination = ({
viewType,
viewOptions,
isDisabled,
variant,
}: ClusterTransferListTablePaginationProps) => (
<ViewPaginationRow
viewType={viewType}
variant={variant}
isDisabled={isDisabled}
{...viewOptions}
/>
);
export default ClusterTransferListTablePagination;
```
This keeps the new component and its semantics intact, but:
- Removes repeated prop wiring (less to read, less to maintain).
- Automatically stays in sync if `ViewOptions` gains/removes pagination fields.
</issue_to_address>
### Comment 4
<location> `src/components/clusters/ClusterTransfer/ClusterTransferList.tsx:60` </location>
<code_context>
error,
refresh,
hideRefreshButton,
+ viewOptions,
}: {
showSpinner: boolean;
</code_context>
<issue_to_address>
**issue (complexity):** Consider keeping pagination logic local to ClusterTransferList instead of passing viewOptions through ClusterTransferPageHeader to avoid unnecessary prop-drilling and coupling.
You can drop the `viewOptions` prop-drilling and keep the same UX by moving the top pagination out of `ClusterTransferPageHeader` and into `ClusterTransferList` alongside the header. That keeps pagination wiring local to the list and removes the extra coupling.
**1. Remove `viewOptions` from `ClusterTransferPageHeader`**
```ts
const ClusterTransferPageHeader = ({
showSpinner,
isError,
error,
refresh,
hideRefreshButton,
}: {
showSpinner: boolean;
isError?: boolean;
error?: Error;
refresh: () => void;
hideRefreshButton?: boolean;
}) => {
// ...unchanged header/toolbar contents, but no pagination here...
};
```
**2. Render header + top pagination together in `ClusterTransferList`**
Instead of letting the header own the pagination, compose them at the list level:
```tsx
const ClusterTransferList = ({ hideRefreshButton }: { hideRefreshButton?: boolean }) => {
const username = useGlobalState((state) => state.userProfile.keycloakProfile.username);
const viewOptions = useGlobalState(
(state) => state.viewOptions[viewConstants.CLUSTER_TRANSFER_VIEW],
);
const { data, isLoading, isError, error } = useFetchClusterTransferDetail({ username });
// ...rest of the component...
return (
<Card>
<CardHeader>
<Flex>
<FlexItem grow={{ default: 'grow' }}>
<ClusterTransferPageHeader
showSpinner={isLoading}
isError={isError}
error={error instanceof Error ? error : undefined}
refresh={refetchClusterTransferDetail}
hideRefreshButton={hideRefreshButton}
/>
</FlexItem>
<FlexItem align={{ default: 'alignRight' }}>
<ClusterTransferListTablePagination
viewType={viewConstants.CLUSTER_TRANSFER_VIEW}
viewOptions={viewOptions}
variant="top"
isDisabled={isLoading || !viewOptions.totalCount}
/>
</FlexItem>
</Flex>
</CardHeader>
{/* existing CardBody stays as-is */}
<CardFooter>
<ClusterTransferListTablePagination
viewType={viewConstants.CLUSTER_TRANSFER_VIEW}
viewOptions={viewOptions}
variant="bottom"
isDisabled={isLoading || !viewOptions.totalCount}
/>
</CardFooter>
</Card>
);
};
```
This:
- Keeps pagination fully controlled by `ClusterTransferList`, where the data lives.
- Removes the `viewOptions` prop from the header and the header → pagination dependency.
- Preserves top and bottom pagination behavior and layout with minimal structural change.
If `ClusterTransferListTablePagination` is itself only a thin wrapper over `ViewPaginationRow`, you can consider inlining `ViewPaginationRow` here later to collapse one more layer, but the above change already eliminates the main prop-drilling chain.
</issue_to_address>
### Comment 5
<location> `src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransfer.tsx:49-58` </location>
<code_context>
const response = await accountsService.searchClusterTransfers({
filter: filter || `id='${transferID}'`,
page: viewOptions.currentPage || 1,
size: viewOptions.pageSize || 20,
orderBy: viewOptions.sorting.sortField
? `${viewOptions.sorting.sortField} ${viewOptions.sorting.isAscending ? 'asc' : 'desc'}`
: 'updated_at desc',
});
return response;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return await accountsService.searchClusterTransfers({
filter: filter || `id='${transferID}'`,
page: viewOptions.currentPage || 1,
size: viewOptions.pageSize || 20,
orderBy: viewOptions.sorting.sortField
? `${viewOptions.sorting.sortField} ${viewOptions.sorting.isAscending ? 'asc' : 'desc'}`
: 'updated_at desc',
});
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary
This has slowly grown to include 3 smaller stories.
First, add pagination to cluster transfer list. Follows same pattern as Access Request pagination on Details->Access Request.
Also fix the title on clusters page (the one in the browser tab). It was saying "Cluster List" instead of "Clusters"
Third, remove the cluster requests links from the Cluster List page. I have left the cluster request link on the empty state of the Dashboard page for now - it will automatically redirect to the new tab page when we update the routes so I am leaving it for now.
Jira
Fixes OCMUI-3883
Fixes OCMUI-3968
Fixes OCMUI-3869
How to Test
Pagination
Title update
Remove Links to request page
Screen Captures
Review process
Please review and follow the PR process.
QE Reviewer
Summary by Sourcery
Add server-backed pagination and shared view state for the cluster transfer list and integrate it with the clusters page layout.
New Features:
Bug Fixes:
Enhancements: