Add test plan test cases pagination #4786
Conversation
artem-blazhko
requested review from
AmsterGet and
maria-hambardzumian
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors pagination handling for test plan test cases from local component state to URL-bound Redux-driven pagination. It introduces location-aware saga logic, adds Changes
Sequence DiagramsequenceDiagram
participant User
participant URL as URL/Router
participant Component as Component
participant Redux as Redux Store
participant Saga
participant API
User->>URL: Change pagination (page/size)
activate URL
URL->>Component: Location query updated
deactivate URL
activate Component
Component->>Redux: Read location query (offset, limit)
Component->>Redux: Read page data & loading state
deactivate Component
Component->>Saga: Trigger (via route/action)
activate Saga
Saga->>Saga: Compute pagination params<br/>(query + defaults)
Saga->>Saga: Check location state<br/>changed from previous?
alt State changed
Saga->>API: Fetch test plan/cases<br/>with offset/limit
API-->>Saga: Return paginated data
Saga->>Redux: Dispatch FETCH_SUCCESS<br/>+ page data + loading
else State unchanged
Saga->>Redux: Skip fetch
end
deactivate Saga
activate Redux
Redux->>Redux: Update testPlanTestCases.page<br/>+ isLoadingTestPlanTestCases
Redux-->>Component: New state
deactivate Redux
Component->>Component: Render with new page data
Component->>User: Display paginated results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/test-library #4786 +/- ##
=====================================================
Coverage 72.59% 72.59%
=====================================================
Files 79 79
Lines 905 905
Branches 124 124
=====================================================
Hits 657 657
Misses 224 224
Partials 24 24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
app/src/controllers/pages/typed-selectors.ts (1)
19-36:LocationInfoadditions look good; double‑checktestPlanIdtyping across selectors/hooksExporting
LocationInfoand addingtestPlanId?: number(including inprev.payload) matches howPROJECT_TEST_PLAN_DETAILS_PAGEis dispatched with numeric IDs and gives sagas a typed place to read it from.One thing to verify:
useTestPlanIdcurrently returnsstate.location?.payload?.testPlanId || '', whileuseTestPlanByIdis typed to take astring. WithtestPlanIdnow explicitlynumber, you may want to either:
- Normalize to a string everywhere (e.g., stringify before storing in
location.payload), or- Update
useTestPlanById/ related selectors to accept anumber(orstring | number) consistently.Please run your TS typecheck and align the
testPlanIdtypes acrossLocationInfo,useTestPlanId, andtestPlanByIdSelectorif it reports mismatches.app/src/common/utils/getRouterParams.ts (1)
20-22: Consider reusing existing UserSettings type.There's a
UserSettingsinterface inapp/src/controllers/user/types.ts. If the existing type can be extended or if this local definition diverges intentionally (index signature for dynamic page size keys), consider adding a comment explaining why a local type is needed.app/src/controllers/testPlan/sagas.ts (1)
133-148: Consider adding error notification for test cases fetch failure.Unlike the active test plan error handling (lines 121-131), the test cases fetch failure doesn't show a user notification. Consider adding a
showErrorNotificationdispatch for consistency with the test plans saga error handling (lines 83-87).} catch (error) { yield put(fetchErrorAction(TEST_PLAN_TEST_CASES_NAMESPACE, error)); + yield put( + showErrorNotification({ + messageId: 'testPlanTestCasesLoadingFailed', + }), + ); }app/src/pages/inside/common/testCaseList/useURLBoundPagination.ts (1)
66-72: Consider handling undefined query params in comparison.When
query?.offsetis undefined,Number(undefined)returnsNaN, and any comparison withNaNis alwaystrue. This means the URL update will always trigger on initial navigation even if unnecessary.const setPageNumber = (page: number): void => { const offset = (page - 1) * pageSize; + const currentOffset = query?.offset != null ? Number(query.offset) : undefined; - if (offset !== Number(query?.offset)) { + if (currentOffset === undefined || offset !== currentOffset) { changeUrlParams({ limit: pageSize, offset }); } };This makes the intent clearer and avoids the implicit NaN behavior, though the current implementation does work correctly in practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
app/src/common/urls.js(1 hunks)app/src/common/utils/getRouterParams.ts(1 hunks)app/src/common/utils/index.js(1 hunks)app/src/controllers/pages/typed-selectors.ts(1 hunks)app/src/controllers/testPlan/actionCreators.ts(1 hunks)app/src/controllers/testPlan/constants.ts(1 hunks)app/src/controllers/testPlan/reducer.ts(1 hunks)app/src/controllers/testPlan/sagas.ts(3 hunks)app/src/controllers/testPlan/selectors.ts(2 hunks)app/src/hooks/useTypedSelector.ts(1 hunks)app/src/pages/inside/common/testCaseList/testCaseList.tsx(4 hunks)app/src/pages/inside/common/testCaseList/useURLBoundPagination.ts(1 hunks)app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanDetailsPage.tsx(3 hunks)app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsx(4 hunks)app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/testPlanFolders.tsx(2 hunks)app/src/pages/inside/testPlansPage/testPlanModals/editTestPlanModal/useEditTestPlan.ts(1 hunks)app/src/pages/inside/testPlansPage/testPlansTable/testPlansTable.tsx(3 hunks)app/src/routes/routesMap.js(2 hunks)app/src/types/common.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: allaprischepa
Repo: reportportal/service-ui PR: 4635
File: app/src/controllers/loading/reducer.js:72-89
Timestamp: 2025-10-02T11:38:11.040Z
Learning: In the reportportal/service-ui repository, the backend API uses 1-based page numbering (pages start from 1, not 0). The pagination handling in reducers should treat page numbers as 1-based.
📚 Learning: 2025-10-24T13:55:44.224Z
Learnt from: yelyzavetakhokhlova
Repo: reportportal/service-ui PR: 4674
File: app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanDetailsPage.tsx:164-164
Timestamp: 2025-10-24T13:55:44.224Z
Learning: In app/src/controllers/testPlan/constants.ts, the TestPlanDto type defines executionStatistic as a required field (not optional), containing covered: number and total: number. When testPlan exists, executionStatistic is guaranteed to be present, so testPlan?.executionStatistic.total does not require additional optional chaining on executionStatistic.
Applied to files:
app/src/controllers/testPlan/constants.tsapp/src/controllers/testPlan/reducer.tsapp/src/controllers/testPlan/selectors.tsapp/src/controllers/pages/typed-selectors.tsapp/src/controllers/testPlan/sagas.tsapp/src/controllers/testPlan/actionCreators.ts
📚 Learning: 2025-07-21T12:08:28.654Z
Learnt from: allaprischepa
Repo: reportportal/service-ui PR: 4480
File: app/src/controllers/instance/allUsers/constants.js:27-27
Timestamp: 2025-07-21T12:08:28.654Z
Learning: In the reportportal/service-ui codebase, the allUsers functionality uses 20 items per page as the default pagination size, defined by DEFAULT_LIMITATION = 20 in app/src/controllers/instance/allUsers/constants.js. The previous DEFAULT_PAGE_SIZE constant was not being used at all and was correctly removed as dead code.
Applied to files:
app/src/controllers/testPlan/constants.tsapp/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsxapp/src/pages/inside/common/testCaseList/testCaseList.tsx
📚 Learning: 2025-09-22T11:43:56.813Z
Learnt from: yelyzavetakhokhlova
Repo: reportportal/service-ui PR: 4615
File: app/src/pages/inside/testCaseLibraryPage/createTestCaseModal/useCreateTestCase.ts:22-23
Timestamp: 2025-09-22T11:43:56.813Z
Learning: In the test case creation modal (app/src/pages/inside/testCaseLibraryPage/createTestCaseModal/), testFolderId is intentionally hardcoded to 85 as a temporary measure until folder selection functionality is implemented.
Applied to files:
app/src/controllers/testPlan/selectors.tsapp/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/testPlanFolders.tsxapp/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanDetailsPage.tsxapp/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsxapp/src/pages/inside/common/testCaseList/testCaseList.tsx
📚 Learning: 2025-10-02T11:38:11.040Z
Learnt from: allaprischepa
Repo: reportportal/service-ui PR: 4635
File: app/src/controllers/loading/reducer.js:72-89
Timestamp: 2025-10-02T11:38:11.040Z
Learning: In the reportportal/service-ui repository, the backend API uses 1-based page numbering (pages start from 1, not 0). The pagination handling in reducers should treat page numbers as 1-based.
Applied to files:
app/src/pages/inside/testPlansPage/testPlansTable/testPlansTable.tsxapp/src/pages/inside/common/testCaseList/useURLBoundPagination.tsapp/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsx
📚 Learning: 2025-07-24T11:26:08.671Z
Learnt from: allaprischepa
Repo: reportportal/service-ui PR: 4487
File: app/src/pages/organization/projectTeamPage/projectTeamListTable/projectTeamListTable.jsx:105-105
Timestamp: 2025-07-24T11:26:08.671Z
Learning: In the ReportPortal service-ui project, the react-hooks/exhaustive-deps ESLint rule is strictly enforced, requiring ALL dependencies used within useEffect, useMemo, and useCallback hooks to be included in the dependency array, even stable references like formatMessage from useIntl(). This prevents ESLint errors and maintains code consistency.
Applied to files:
app/src/pages/inside/testPlansPage/testPlansTable/testPlansTable.tsxapp/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsx
📚 Learning: 2025-08-14T12:06:14.147Z
Learnt from: allaprischepa
Repo: reportportal/service-ui PR: 4543
File: app/src/pages/inside/projectSettingsPageContainer/content/analyzerContainer/autoAnalysis/autoAnalysis.jsx:17-17
Timestamp: 2025-08-14T12:06:14.147Z
Learning: In the reportportal/service-ui project, webpack uses ProvidePlugin to automatically provide React as a global variable, so explicit `import React from 'react'` is not needed in .jsx files for JSX to work. The project uses classic JSX runtime with automatic React injection via webpack.
Applied to files:
app/src/pages/inside/testPlansPage/testPlansTable/testPlansTable.tsxapp/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsx
🧬 Code graph analysis (8)
app/src/controllers/testPlan/reducer.ts (1)
app/src/controllers/testPlan/constants.ts (1)
TEST_PLAN_TEST_CASES_NAMESPACE(25-25)
app/src/routes/routesMap.js (4)
app/src/common/utils/getRouterParams.ts (1)
getRouterParams(44-58)app/src/controllers/testPlan/constants.ts (4)
TEST_PLANS_NAMESPACE(22-22)defaultQueryParams(26-30)TEST_PLAN_TEST_CASES_NAMESPACE(25-25)defaultTestPlanTestCasesQueryParams(32-35)app/src/controllers/testPlan/actionCreators.ts (2)
getTestPlansAction(30-33)getTestPlanAction(35-38)app/src/controllers/testPlan/index.ts (2)
getTestPlansAction(17-17)getTestPlanAction(17-17)
app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/testPlanFolders.tsx (1)
app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/index.ts (1)
TestPlanFolders(17-17)
app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanDetailsPage.tsx (2)
app/src/hooks/useTypedSelector.ts (1)
useTestPlanTestCasesLoading(71-72)app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/testPlanFolders.tsx (1)
TestPlanFolders(33-65)
app/src/pages/inside/testPlansPage/testPlanModals/editTestPlanModal/useEditTestPlan.ts (2)
app/src/controllers/testPlan/actionCreators.ts (1)
getTestPlanAction(35-38)app/src/controllers/launch/sagas.js (1)
queryParams(54-54)
app/src/controllers/testPlan/sagas.ts (4)
app/src/controllers/pages/typed-selectors.ts (2)
locationSelector(42-42)LocationInfo(19-36)app/src/controllers/testPlan/constants.ts (1)
defaultTestPlanTestCasesQueryParams(32-35)app/src/types/store.ts (1)
BaseAppState(24-34)app/src/controllers/pages/constants.js (2)
PROJECT_TEST_PLANS_PAGE(73-73)PROJECT_TEST_PLANS_PAGE(73-73)
app/src/pages/inside/testPlansPage/testPlansTable/testPlansTable.tsx (3)
app/src/hooks/useTypedSelector.ts (1)
useProjectDetails(34-42)app/src/pages/inside/common/testCaseList/useURLBoundPagination.ts (1)
useURLBoundPagination(34-88)app/src/controllers/testPlan/constants.ts (1)
TEST_PLANS_NAMESPACE(22-22)
app/src/common/utils/getRouterParams.ts (3)
app/src/controllers/user/types.ts (1)
UserSettings(55-59)app/src/types/common.ts (1)
queryParamsType(33-36)app/src/routes/routesMap.js (6)
getRouterParams(446-450)getRouterParams(460-464)state(225-225)state(409-409)state(445-445)state(458-458)
🔇 Additional comments (28)
app/src/types/common.ts (1)
33-36: Optional offset/limit inqueryParamsTypelooks correctMaking
offset/limitoptional aligns with cases where defaults or URL params may be absent and avoids forcing callers to always provide them. This is compatible with offset-based TMS endpoints and does not conflict with 1‑basedpage.*pagination used elsewhere.
Based on learnings, this keeps the distinction between offset-based and page-based APIs clear.app/src/common/urls.js (1)
382-385: Query‑awaretestPlanTestCasesURL matches existing patternsExtending
testPlanTestCasesto accept an optionalqueryobject and piping it throughgetQueryParamsis consistent with nearby TMS URL helpers (testCases,testPlan,testFolders, etc.) and supports the new pagination flow cleanly. Just keep passing a flat object of serializable primitives (e.g.,offset,limit) and avoid nested objects here.app/src/common/utils/index.js (1)
71-71: Re‑exportinggetRouterParamsis appropriateAdding
getRouterParamsto the common utils barrel keeps route logic and other callers from importing deep paths and matches how other helpers are exposed.app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanDetailsPage.tsx (1)
39-40: Loading state plumbing for test plan test cases looks consistent
useTestPlanTestCasesLoadingis correctly pulled from the store and propagated asisLoadingintoTestPlanFolders, which then forwards it toAllTestCasesPage. This keeps the not‑found redirect logic intact while giving the list its own loading indicator.Also applies to: 68-69, 170-171
app/src/controllers/testPlan/reducer.ts (1)
31-39: NewisLoadingTestPlanTestCasesslice matches existing loading patternHooking
loadingReducer(TEST_PLAN_TEST_CASES_NAMESPACE)intoisLoadingTestPlanTestCasesis consistent with howisLoadingandisLoadingActiveare handled and lines up with the new selector/hook usage. No issues here.app/src/hooks/useTypedSelector.ts (1)
71-72:useTestPlanTestCasesLoadinghook is correctly wiredThe hook follows the same pattern as other loading hooks, targets
isLoadingTestPlanTestCaseson thetestPlanslice, and safely defaults tofalsewhen unset. It integrates cleanly with the new UI usage.app/src/controllers/testPlan/selectors.ts (2)
33-33: LGTM!The new
isLoadingTestPlanTestCasesflag is consistent with the existing optional loading state pattern used byisLoadingandisLoadingActive.
63-65: LGTM!The new selector follows the same pattern as
testPlansPageSelector(line 47), returningundefinedwhen page data is not available.app/src/routes/routesMap.js (3)
138-141: LGTM!Good refactoring to centralize pagination parameter extraction using
getRouterParams. The imports are correctly added to support the new utility function.
446-450: LGTM!The refactored pagination logic using
getRouterParamsimproves consistency and maintainability across routes.
458-466: LGTM!The test plan details page now correctly derives pagination parameters using
getRouterParamswith the appropriate namespace and defaults for test cases, while passingoffsetandlimittogetTestPlanAction.app/src/controllers/testPlan/constants.ts (1)
32-35: LGTM!The new constant provides appropriate default pagination for test cases with a larger page size (50) compared to test plans (20), which makes sense given the different data volumes.
app/src/controllers/testPlan/actionCreators.ts (2)
20-21: Type widening for URL query parameters.The
string | numbertype accommodates URL query params which arrive as strings. Ensure the API layer or saga properly coerces these to numbers before making backend requests.
26-27: LGTM!Consistent type widening for pagination parameters in
GetTestPlanParams.app/src/common/utils/getRouterParams.ts (1)
44-57: LGTM on overall structure.The utility correctly centralizes pagination parameter extraction from URL query, user preferences, and defaults. The priority order (query > savedLimit > default) for
limitis appropriate.app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/testPlanFolders.tsx (2)
29-34: LGTM!Clean implementation of the loading state prop with an appropriate default value. The interface is properly typed and the component signature is clear.
60-60: LGTM!Good change from hardcoded
loading={false}to dynamicloading={isLoading}, enabling proper loading state propagation from parent components.app/src/pages/inside/testPlansPage/testPlansTable/testPlansTable.tsx (2)
57-65: LGTM!Good refactoring to use
useURLBoundPaginationhook. The hook properly handles the conversion between 0-based offsets and 1-based page numbers (as required by the backend API, based on learnings). ThebaseUrlconstruction is correct for the milestones route.
193-194: LGTM!Clean integration with the URL-bound pagination handlers. The
setPageNumberandsetPageSizefunctions from the hook will handle URL updates automatically.app/src/controllers/testPlan/sagas.ts (3)
92-98: LGTM on params derivation.Good use of defaults from
defaultTestPlanTestCasesQueryParamswhen offset/limit are not provided.
101-120: Smart caching optimization for test plan details.The conditional fetch logic avoids redundant API calls when only pagination changes (same test plan, same params). The comparison using
String()coercion handles the string/number type mismatch from URL params correctly.
121-131: Verify error navigation behavior.On active test plan fetch failure, the saga navigates back to
PROJECT_TEST_PLANS_PAGE. This is reasonable UX, but ensure this doesn't cause issues if the error is transient (e.g., network hiccup) where the user might prefer to retry rather than be redirected.app/src/pages/inside/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsx (1)
85-98: LGTM!The conditional rendering logic correctly guards against undefined
pageDataand hides pagination whentotalElementsis zero or falsy. The non-optional access on line 90 is safe within this conditional block.app/src/pages/inside/common/testCaseList/testCaseList.tsx (3)
109-125: LGTM!The selection logic correctly operates on
testCasesdirectly. Since pagination is now handled externally,testCasesalready represents the current page's data, socurrentPageTestCaseIdsaccurately reflects the visible items for select-all functionality.
129-157: LGTM!Table data construction correctly uses
testCases, which now represents the pre-paginated data from the parent component.
212-233: LGTM!The emptiness check correctly references
testCases, maintaining consistent behavior with the refactored data flow.app/src/pages/inside/common/testCaseList/useURLBoundPagination.ts (2)
60-64: LGTM!The URL construction and navigation via
pushis clean and appropriate for redux-first-router integration.
74-78: LGTM!Resetting to
defaultQueryParams.offsetwhen page size changes is the correct UX pattern—it prevents landing on an invalid page after changing the items-per-page setting. The same NaN comparison consideration fromsetPageNumberapplies here, but consistently.
...side/testPlansPage/testPlanDetailsPage/testPlanFolders/allTestCasesPage/allTestCasesPage.tsx
Show resolved
Hide resolved
app/src/pages/inside/testPlansPage/testPlanModals/editTestPlanModal/useEditTestPlan.ts
Outdated
Show resolved
Hide resolved
artem-blazhko
force-pushed
the
test-plan-test-cases-pagination
branch
from
f2124d4 to
494a460
Compare
app/src/pages/inside/common/testCaseList/useURLBoundPagination.ts
Outdated
Show resolved
Hide resolved
artem-blazhko
force-pushed
the
test-plan-test-cases-pagination
branch
from
494a460 to
5ca457a
Compare
|
3 participants
PR Checklist
developfor features/bugfixes, other if mentioned in the task)npm run lint) prior to submission? Enable the git hook on commit in your IDE to run it and format the code automatically.npm run build)?devnpm script)manage:translationsscript?Visuals
2025-12-06_02h13_11.mp4
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.