Skip to content

Conversation

@zherman0
Copy link
Contributor

@zherman0 zherman0 commented Nov 26, 2025

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

  1. If there are no transfer requests already - Create a ROSA classic cluster - initiate a transfer and then cancel it 11 times or as many as you need to have at least 11 items in the list - this is pretty tedious but if you are in my org there should be old transfers already listed.
  2. Check the transfer list page (openshift/clusters/requests or openshift/cluster-request)
  3. Change the page size to 10 and try the paging functions

Title update

  1. Load the openshift/clusters page. Check that the title on the tab says "Clusters"

Remove Links to request page

  1. In the Cluster List, the "View cluster request" link is on the empty page and on the actions link menu which will show as a link on the tool bar or under the 3 dots if the screen size is small..
  2. Go to the new Clusters page (openshift/clusters) and verify that the empty page has no "View cluster request" link. When there is a cluster, verify there is no "View cluster request" link on the tool bar at the top of the list.
  3. Note: The links will still show on the normal Cluster List page (openshift/cluster-list)

Screen Captures

Before After
Screenshot From 2025-11-26 15-45-35 Screenshot From 2025-11-26 15-35-24
Screenshot From 2025-11-26 17-25-00
Screenshot From 2025-11-26 17-25-16
Before After
Screenshot From 2025-11-26 18-59-19 Screenshot From 2025-11-26 18-58-10

Review process

Please review and follow the PR process.

QE Reviewer

  • Pre-merge testing : Verified change locally in a browser (downloaded and ran code using reviewx tool)
  • Updated/created Polarion test cases which were peer QE reviewed
  • Confirmed 'tc-approved' label was added by dev to the linked JIRA ticket
  • (optional) Updated/created Cypress e2e tests
  • Closed threads I started after the author made changes or added an explanation

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:

  • Introduce paginated cluster transfer list using global view options for page, page size, and sorting.
  • Expose pagination controls in the cluster transfer list header and footer tied to the global view state.

Bug Fixes:

  • Ensure cluster transfer refresh does not unnecessarily refetch the main clusters list when non-list tabs are active.
  • Include all relevant cluster transfers in the detail view by relaxing the previous filtering logic on transfer ownership.

Enhancements:

  • Extend the cluster transfer search API wrapper to accept paging and sorting parameters instead of a simple filter string.
  • Wire the cluster transfer view into the shared viewOptions reducer with appropriate initial state and total-count tracking from query results.

@zherman0 zherman0 marked this pull request as draft November 26, 2025 22:46
@sourcery-ai
Copy link

sourcery-ai bot commented Nov 26, 2025

Reviewer's Guide

Implements 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 tabs

sequenceDiagram
  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()
Loading

Sequence diagram for cluster transfer list pagination and data fetching

sequenceDiagram
  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
Loading

Updated class diagram for cluster transfer list, pagination, and related services

classDiagram
  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
Loading

File-Level Changes

Change Details Files
Add global viewOptions-backed pagination controls to the cluster transfer list header and footer.
  • Introduce ClusterTransferListTablePagination wrapper around ViewPaginationRow for top and bottom pagination controls.
  • Pass CLUSTER_TRANSFER_VIEW viewOptions into ClusterTransferPageHeader and ClusterTransferList to drive pagination state.
  • Disable pagination controls when loading or when no totalCount is present to avoid invalid interactions.
src/components/clusters/ClusterTransfer/ClusterTransferList.tsx
src/components/clusters/ClusterTransfer/ClusterTransferListTablePagination.tsx
src/redux/reducers/viewOptionsReducer.ts
Update cluster transfer fetch logic to use paginated search API with shared view state and track total counts.
  • Extend accountsService.searchClusterTransfers to accept an options object (filter, page, size, orderBy) and set sane defaults.
  • Update useFetchClusterTransfer to include viewOptions in the query key and call the new paginated searchClusterTransfers API when using transfer IDs/filters.
  • On each fetch, dispatch onSetTotal with the server-reported total to keep CLUSTER_TRANSFER_VIEW viewOptions.totalCount in sync.
src/services/accountsService.ts
src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransfer.tsx
src/redux/reducers/viewOptionsReducer.ts
Relax transfer filtering logic so all transfer details are included regardless of previous ownership workaround.
  • Remove conditional that only pushed certain transfers to the details list based on recipient_external_org_id and username.
  • Always push computed transferDetails into clusterTransferDetails, ensuring the list includes all relevant transfers.
src/queries/ClusterDetailsQueries/ClusterTransferOwnership/useFetchClusterTransferDetails.tsx
Make clusters page header tab-aware so refresh behavior depends on the active tab and tests are updated accordingly.
  • Change ClustersPageHeader to accept activeTabKey and only refetch the clusters list when the list tab is active, while always refetching transfer details.
  • Update Clusters component to pass activeTabKey into ClustersPageHeader.
  • Adjust ClustersPageHeader tests to pass activeTabKey and add a new test ensuring the transfers tab refresh does not refetch the clusters list.
src/components/clusters/Clusters/ClustersPageHeader.tsx
src/components/clusters/Clusters/Clusters.tsx
src/components/clusters/Clusters/ClustersPageHeader.test.tsx
Fix the document title for the clusters page when the tabbed clusters view is enabled.
  • Move PAGE_TITLE construction inside ClusterList and make it depend on showTabbedView.
  • Use 'Clusters
Red Hat OpenShift Cluster Manager' when tabbed view is active and the old 'Cluster List

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@zherman0 zherman0 self-assigned this Nov 26, 2025
Copy link

@sourcery-ai sourcery-ai bot left a 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-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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zherman0 zherman0 marked this pull request as ready for review November 27, 2025 00:28
Copy link

@sourcery-ai sourcery-ai bot left a 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 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zherman0 zherman0 requested a review from jmekkatt November 27, 2025 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant