Adds support for query.paginated
#10529
Conversation
WalkthroughThis pull request introduces a new function called Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueryModule as query.paginated
participant API
User->>QueryModule: Call query.paginated(route, options)
loop While more pages exist AND maxPages not reached
QueryModule->>API: Fetch current page data
API-->>QueryModule: Return paginated response
QueryModule->>QueryModule: Append items to results
end
QueryModule->>User: Return aggregated results
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (4)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Deploying care-fe with
|
| Latest commit: |
8c9875d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://84c8a42c.care-fe.pages.dev |
| Branch Preview URL: | https://rithviknishad-feat-use-pagin.care-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Utils/request/use-paginated-query.ts (1)
27-27: Consider using type guard instead of type assertion.Replace the type assertion with a type guard to ensure runtime type safety:
- return response as PaginatedResponse<Route["TRes"]>; + if (isPaginatedResponse(response)) { + return response; + } + throw new Error("Expected paginated response");Add this type guard function:
function isPaginatedResponse<T>(response: unknown): response is PaginatedResponse<T> { return ( typeof response === "object" && response !== null && "count" in response && "results" in response && Array.isArray((response as PaginatedResponse<T>).results) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Utils/request/use-paginated-query.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint Code Base
src/Utils/request/use-paginated-query.ts
[warning] 1-1: File is unimported. Consider importing or removing it.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/Utils/request/use-paginated-query.ts (4)
13-15: LGTM! Well-typed generic hook signature.The function signature is well-designed with proper generic type constraints and follows React hook naming conventions.
42-46: LGTM! Well-structured return value.The return object provides a good balance of convenience (flattened data) and flexibility (paginated results) while maintaining type safety.
1-12: Address the pipeline warning about unimported file.The linter indicates that this file is not imported anywhere in the codebase. Ensure that this hook is properly imported and utilized where needed.
Let's verify the file usage:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for any imports of use-paginated-query rg -l "use-paginated-query"Length of output: 27
Pipeline Warning: "use-paginated-query" Not Imported
Based on the search results—which produced no output—the file "src/Utils/request/use-paginated-query.ts" does not appear to be imported anywhere in the codebase. This aligns with the linter's warning. Please manually verify if the hook is meant to be used (perhaps via dynamic or indirect imports) or if it should be added to where necessary.
🧰 Tools
🪛 GitHub Actions: Lint Code Base
[warning] 1-1: File is unimported. Consider importing or removing it.
23-23: Verify the RESULTS_PER_PAGE_LIMIT constant value.Ensure that the constant value aligns with the API's pagination limits.
Let's check the constant definition:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for RESULTS_PER_PAGE_LIMIT definition rg "RESULTS_PER_PAGE_LIMIT.*=.*" -A 2Length of output: 161
RESULTS_PER_PAGE_LIMIT is set to 14
- The constant is defined in src/common/constants.tsx as follows:
export const RESULTS_PER_PAGE_LIMIT = 14;- Provided that the API's pagination limit is expected to be 14 results per page, this constant aligns with the API constraints.
| useEffect(() => { | ||
| if (hasNextPage && fetchNextPage) { | ||
| fetchNextPage(); | ||
| } | ||
| }, [hasNextPage, fetchNextPage, data]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding safeguards and optimizing dependencies.
The current implementation has two potential issues:
- No safeguards against infinite loops if the API response is malformed
- Unnecessary
datadependency might cause extra re-renders
Consider this improved implementation:
useEffect(() => {
+ const MAX_PAGES = 1000; // Safeguard against infinite loops
+ const currentPages = data?.pages.length ?? 0;
- if (hasNextPage && fetchNextPage) {
+ if (hasNextPage && fetchNextPage && currentPages < MAX_PAGES) {
fetchNextPage();
}
- }, [hasNextPage, fetchNextPage, data]);
+ }, [hasNextPage, fetchNextPage, data?.pages.length]);📝 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.
| useEffect(() => { | |
| if (hasNextPage && fetchNextPage) { | |
| fetchNextPage(); | |
| } | |
| }, [hasNextPage, fetchNextPage, data]); | |
| useEffect(() => { | |
| const MAX_PAGES = 1000; // Safeguard against infinite loops | |
| const currentPages = data?.pages.length ?? 0; | |
| if (hasNextPage && fetchNextPage && currentPages < MAX_PAGES) { | |
| fetchNextPage(); | |
| } | |
| }, [hasNextPage, fetchNextPage, data?.pages.length]); |
CARE
|
||||||||||||||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
rithviknishad/feat/use-paginated-query
|
| Run status |
|
| Run duration | 06m 24s |
| Commit |
|
| Committer | Rithvik Nishad |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
2
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
9
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
facility_spec/facility_creation.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Facility Management > Create a new facility using the admin role and verify validation errors |
Test Replay
Screenshots
|
|
users_spec/user_creation.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| User Creation > should create a new user successfully |
Test Replay
Screenshots
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Utils/request/query.ts(2 hunks)src/Utils/request/types.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/Utils/request/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/Utils/request/query.ts (4)
4-9: LGTM!The imports are well-organized and include all necessary types for the pagination functionality.
122-141: Documentation looks good!The documentation is clear and includes a helpful example of usage with
useQuery.
174-174: LGTM!The extension follows the same pattern as
query.debounced, maintaining consistency in the codebase.
1-175: Verify documentation and type usage.Please ensure:
- The documentation at https://docs.ohc.network is updated to reflect the new
usePaginatedQueryfunctionality.- All components using
PaginatedResponsetype are updated to handle the removednextandpreviousproperties.Run the following script to find components that might need updates:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Find components using PaginatedResponse type rg -l "PaginatedResponse" src/Length of output: 1143
I'll verify that the PaginatedResponse type in the code no longer includes the removed next/previous properties. Additionally, I'll check that any components using PaginatedResponse across the codebase have been updated accordingly.
Let's begin by inspecting the PaginatedResponse definition in "src/Utils/request/types.ts" for any remaining usage of "next" or "previous".
I'll run a script to search for "next:" or "previous:" in that file.
🏁 Script executed:
#!/bin/bash # Search for any occurrences of "next:" or "previous:" in the PaginatedResponse type definition. rg -n "interface PaginatedResponse" -n src/Utils/request/types.ts rg -n "next\s*:" src/Utils/request/types.ts rg -n "previous\s*:" src/Utils/request/types.tsLength of output: 210
Documentation & PaginatedResponse Usage Verification
- Confirmed the PaginatedResponse interface (in src/Utils/request/types.ts) no longer includes next or previous properties.
- Verified that components referencing PaginatedResponse (as identified by the rg search in several files) are now expected to handle only count and results.
- Please ensure the external documentation at https://docs.ohc.network is updated accordingly to reflect the new usePaginatedQuery functionality.
src/Utils/request/query.ts
Outdated
| const paginatedQuery = < | ||
| Route extends ApiRoute<PaginatedResponse<unknown>, unknown>, | ||
| >( | ||
| route: Route, | ||
| options?: ApiCallOptions<Route> & { maxPages?: number }, | ||
| ) => { | ||
| return async ({ signal }: { signal: AbortSignal }) => { | ||
| const items: Route["TRes"]["results"] = []; | ||
| let hasNextPage = true; | ||
| let page = 0; | ||
|
|
||
| while (hasNextPage) { | ||
| const res = await query(route, { ...options })({ signal }); | ||
| items.push(...res.results); | ||
|
|
||
| if (options?.maxPages && page >= options.maxPages) { | ||
| hasNextPage = false; | ||
| } | ||
|
|
||
| if (items.length >= res.count) { | ||
| hasNextPage = false; | ||
| } | ||
|
|
||
| page++; | ||
| } | ||
|
|
||
| return { | ||
| count: items.length, | ||
| results: items, | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and progress tracking.
The pagination implementation could be improved in several areas:
- Add error handling for failed requests within the loop
- Manage offset in query parameters for each page
- Consider memory implications for large datasets
- Add progress tracking for UI feedback
Here's a suggested implementation:
const paginatedQuery = <
Route extends ApiRoute<PaginatedResponse<unknown>, unknown>,
>(
route: Route,
options?: ApiCallOptions<Route> & { maxPages?: number },
) => {
return async ({ signal }: { signal: AbortSignal }) => {
const items: Route["TRes"]["results"] = [];
let hasNextPage = true;
let page = 0;
+ let offset = options?.queryParams?.offset ?? 0;
+ const limit = options?.queryParams?.limit ?? 10;
while (hasNextPage) {
- const res = await query(route, { ...options })({ signal });
+ try {
+ const res = await query(route, {
+ ...options,
+ queryParams: {
+ ...options?.queryParams,
+ offset: offset + (page * limit),
+ },
+ })({ signal });
+
+ items.push(...res.results);
- items.push(...res.results);
+ if (options?.maxPages && page >= options.maxPages - 1) {
+ hasNextPage = false;
+ }
- if (options?.maxPages && page >= options.maxPages) {
- hasNextPage = false;
- }
+ if (items.length >= res.count) {
+ hasNextPage = false;
+ }
- if (items.length >= res.count) {
- hasNextPage = false;
+ page++;
+ } catch (error) {
+ // Rethrow abort errors
+ if (error instanceof DOMException && error.name === 'AbortError') {
+ throw error;
+ }
+ // Handle other errors
+ throw new Error(`Failed to fetch page ${page + 1}: ${error.message}`);
}
-
- page++;
}
return {
count: items.length,
results: items,
+ // Add metadata for UI feedback
+ pagination: {
+ totalPages: page,
+ currentPage: page,
+ },
};
};
};📝 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 paginatedQuery = < | |
| Route extends ApiRoute<PaginatedResponse<unknown>, unknown>, | |
| >( | |
| route: Route, | |
| options?: ApiCallOptions<Route> & { maxPages?: number }, | |
| ) => { | |
| return async ({ signal }: { signal: AbortSignal }) => { | |
| const items: Route["TRes"]["results"] = []; | |
| let hasNextPage = true; | |
| let page = 0; | |
| while (hasNextPage) { | |
| const res = await query(route, { ...options })({ signal }); | |
| items.push(...res.results); | |
| if (options?.maxPages && page >= options.maxPages) { | |
| hasNextPage = false; | |
| } | |
| if (items.length >= res.count) { | |
| hasNextPage = false; | |
| } | |
| page++; | |
| } | |
| return { | |
| count: items.length, | |
| results: items, | |
| }; | |
| }; | |
| }; | |
| const paginatedQuery = < | |
| Route extends ApiRoute<PaginatedResponse<unknown>, unknown>, | |
| >( | |
| route: Route, | |
| options?: ApiCallOptions<Route> & { maxPages?: number }, | |
| ) => { | |
| return async ({ signal }: { signal: AbortSignal }) => { | |
| const items: Route["TRes"]["results"] = []; | |
| let hasNextPage = true; | |
| let page = 0; | |
| let offset = options?.queryParams?.offset ?? 0; | |
| const limit = options?.queryParams?.limit ?? 10; | |
| while (hasNextPage) { | |
| try { | |
| const res = await query(route, { | |
| ...options, | |
| queryParams: { | |
| ...options?.queryParams, | |
| offset: offset + (page * limit), | |
| }, | |
| })({ signal }); | |
| items.push(...res.results); | |
| if (options?.maxPages && page >= options.maxPages - 1) { | |
| hasNextPage = false; | |
| } | |
| if (items.length >= res.count) { | |
| hasNextPage = false; | |
| } | |
| page++; | |
| } catch (error) { | |
| // Rethrow abort errors | |
| if (error instanceof DOMException && error.name === 'AbortError') { | |
| throw error; | |
| } | |
| // Handle other errors | |
| throw new Error(`Failed to fetch page ${page + 1}: ${error.message}`); | |
| } | |
| } | |
| return { | |
| count: items.length, | |
| results: items, | |
| // Add metadata for UI feedback | |
| pagination: { | |
| totalPages: page, | |
| currentPage: page, | |
| }, | |
| }; | |
| }; | |
| }; |
rithviknishad
changed the title
usePaginatedQueryquery.paginated
|
@rithviknishad @amjithtitus09 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
query.paginated@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Refactor