fix(#16066): improve type safety for query params in request utilities#16326
fix(#16066): improve type safety for query params in request utilities#16326rbalogic wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a TQuery generic to ApiRoute and uses it to type queryParams across request utilities; widens request/mutate generics; strengthens paginated query typing and runtime paging; updates API specs to declare TQuery; narrows a component prop; README and JSDoc updated. ChangesQuery Parameter Type Safety System
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/request/query.ts (1)
158-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't discard caller-supplied pagination cursors.
The new merge order makes
query.paginated(..., { queryParams: { offset, limit } })always start from0andpageSize, even though those fields are still part of the public options type. Any caller trying to resume from a non-zero offset will now fetch the wrong slice. Either seed the loop from the incomingoffset/limit, or remove those fields from the exposed options.Possible fix
- const pageSize = options?.pageSize ?? RESULTS_PER_PAGE_LIMIT; + const pageSize = + options?.pageSize ?? + options?.queryParams?.limit ?? + RESULTS_PER_PAGE_LIMIT; + const startOffset = options?.queryParams?.offset ?? 0; @@ ...options, queryParams: { ...options?.queryParams, limit: pageSize, - offset: page * pageSize, + offset: startOffset + page * pageSize, } as RouteQueryParams<Route> & { limit?: number; offset?: number; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/request/query.ts` around lines 158 - 189, paginatedQuery currently overwrites caller-supplied offset/limit; preserve them by seeding the loop from options.queryParams: compute a startingOffset = options?.queryParams?.offset ?? 0 and set pageSize = options?.pageSize ?? options?.queryParams?.limit ?? RESULTS_PER_PAGE_LIMIT, initialize page = Math.floor(startingOffset / pageSize) (or 0) and then pass offset = startingOffset + page * pageSize and limit = pageSize into the per-iteration query call so callers can resume from a non-zero offset while pageSize still defaults to RESULTS_PER_PAGE_LIMIT when neither option is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Utils/request/query.ts`:
- Around line 158-189: paginatedQuery currently overwrites caller-supplied
offset/limit; preserve them by seeding the loop from options.queryParams:
compute a startingOffset = options?.queryParams?.offset ?? 0 and set pageSize =
options?.pageSize ?? options?.queryParams?.limit ?? RESULTS_PER_PAGE_LIMIT,
initialize page = Math.floor(startingOffset / pageSize) (or 0) and then pass
offset = startingOffset + page * pageSize and limit = pageSize into the
per-iteration query call so callers can resume from a non-zero offset while
pageSize still defaults to RESULTS_PER_PAGE_LIMIT when neither option is
provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b04e2b9-095d-42f6-89e4-a313703d3f07
📒 Files selected for processing (10)
src/Utils/request/README.mdsrc/Utils/request/mutate.tssrc/Utils/request/query.tssrc/Utils/request/types.tssrc/components/Questionnaire/ManageResponseTemplatesSheet.tsxsrc/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsxsrc/types/emr/medicationRequest/medicationRequestApi.tssrc/types/inventory/supplyDelivery/supplyDeliveryApi.tssrc/types/inventory/supplyRequest/supplyRequestApi.tssrc/types/questionnaire/questionnaireResponseTemplateApi.ts
|
I have addressed the feedback provided by CodeRabbit regarding docstring coverage, pagination logic, and the scope of changes. 1. Fixed Pagination Cursor Handling (✅ Resolved)As identified in the detailed review of
2. Improved Docstring Coverage (✅ Fixed)I have added comprehensive TSDoc comments to the core request utility layer to meet the 80% threshold:
3. Clarification on "Out of Scope" ChangesThe changes flagged as out-of-scope are essential dependencies for the
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/request/query.ts (1)
168-199:⚠️ Potential issue | 🟠 Major
paginatedQueryshould enforce pagination parameters in the route's TQuery constraint.The function currently accepts any route returning
PaginatedResponseregardless of whether itsTQueryincludeslimitandoffset. Inside the loop (line ~197), it casts the constructed query params to bypass the type check, which sidesteps the contract fromtypes.ts. This allows a route with a restrictive customTQuery(e.g.,{ request_order: string }) to compile withpaginatedQuery, even though pagination params aren't declared.While current usage is safe (routes with custom
TQueryare never used withpaginatedQuery), the type system should prevent this at compile time.Suggested fix
import { ApiCallOptions, ApiRoute, HTTPError, PaginatedResponse, + QueryParams, RouteQueryParams, } from "@/Utils/request/types"; + +interface PaginationQueryParams extends QueryParams { + limit?: number; + offset?: number; +} @@ const paginatedQuery = < - Route extends ApiRoute<PaginatedResponse<unknown>, unknown, unknown>, + Route extends ApiRoute< + PaginatedResponse<unknown>, + unknown, + PaginationQueryParams + >, >( @@ queryParams: { ...options?.queryParams, limit: pageSize, offset: page * pageSize, - } as RouteQueryParams<Route> & { - limit?: number; - offset?: number; - }, + }, })({ signal });Per coding guidelines: "Always provide proper TypeScript types for parameters and return values in utility functions" and "Use generic types where appropriate for reusability in utility functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/request/query.ts` around lines 168 - 199, paginatedQuery currently accepts any ApiRoute that returns PaginatedResponse but bypasses TQuery typing by casting query params; change the generic so the Route's TQuery is constrained to include optional pagination fields and stop using the cast. Specifically, introduce a generic for the route query (e.g., Q extends RouteQueryParams<Route> & { limit?: number; offset?: number } or directly constrain Route as Route extends ApiRoute<PaginatedResponse<unknown>, Record<string, unknown> & { limit?: number; offset?: number }, unknown>), update the paginatedQuery signature to use that constrained Route type, and then remove the "as RouteQueryParams..." cast when passing queryParams to query(route, {...}) so the compiler enforces routes declare limit/offset. Reference symbols: paginatedQuery, ApiRoute, RouteQueryParams, TQuery/TRes, and query.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Utils/request/types.ts`:
- Around line 24-40: The ApiRoute generic should constrain TQuery so routes
cannot widen to a plain QueryParams; update the ApiRoute declaration so the
TQuery generic is declared as TQuery extends QueryParams = QueryParams and
change the defaultQueryParams property to be typed as TQuery
(defaultQueryParams?: TQuery) so any provided defaults must conform to the
route's declared query shape; update any related uses of ApiRoute/TQuery to
respect the new constraint.
---
Outside diff comments:
In `@src/Utils/request/query.ts`:
- Around line 168-199: paginatedQuery currently accepts any ApiRoute that
returns PaginatedResponse but bypasses TQuery typing by casting query params;
change the generic so the Route's TQuery is constrained to include optional
pagination fields and stop using the cast. Specifically, introduce a generic for
the route query (e.g., Q extends RouteQueryParams<Route> & { limit?: number;
offset?: number } or directly constrain Route as Route extends
ApiRoute<PaginatedResponse<unknown>, Record<string, unknown> & { limit?: number;
offset?: number }, unknown>), update the paginatedQuery signature to use that
constrained Route type, and then remove the "as RouteQueryParams..." cast when
passing queryParams to query(route, {...}) so the compiler enforces routes
declare limit/offset. Reference symbols: paginatedQuery, ApiRoute,
RouteQueryParams, TQuery/TRes, and query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8557dc3d-12cb-4881-8e86-7de8e3301227
📒 Files selected for processing (2)
src/Utils/request/query.tssrc/Utils/request/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Utils/request/query.ts`:
- Around line 193-197: The type for the options parameter can be simplified by
removing the redundant Omit and explicit re-declaration of queryParams: replace
the current "Omit<ApiCallOptions<Route>, 'queryParams'> & { queryParams?:
RouteQueryParams<Route>; pageSize?: number; maxPages?: number }" with
"ApiCallOptions<Route> & { pageSize?: number; maxPages?: number }" so you keep
ApiCallOptions and its existing queryParams typing (RouteQueryParams<Route>)
while adding pageSize and maxPages; update the type where the parameter named
options is declared to reference ApiCallOptions<Route> & { pageSize?: number;
maxPages?: number } (look for the options type definition around the function
that uses ApiCallOptions and RouteQueryParams).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1eb885c7-dc9f-4472-8dc3-68794eb36e68
📒 Files selected for processing (2)
src/Utils/request/query.tssrc/Utils/request/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Utils/request/query.ts`:
- Around line 203-207: In query.paginated, validate and normalize pagination
inputs before the paging loop: ensure pageSize (computed from options?.pageSize
or options?.queryParams?.limit or RESULTS_PER_PAGE_LIMIT) is a positive integer
(if <= 0 either throw a clear error or default it to RESULTS_PER_PAGE_LIMIT) and
ensure startOffset (from options?.queryParams?.offset) is >= 0 (coerce negatives
to 0 or throw). Use the validated/normalized pageSize and startOffset variables
in the loop (the code that advances offset by pageSize) so the offset always
progresses and invalid values cannot cause an infinite loop or invalid API
requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cac2731-6106-4712-bec9-d02f9e43763b
📒 Files selected for processing (1)
src/Utils/request/query.ts
🚀 Preview Deployment Ready!🔗 Preview URL: https://pr-16326.care-preview-a7w.pages.dev 📱 Mobile Access: This preview will be automatically updated when you push new commits to this PR. |
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #8756 |
|
@nihal467 this looks fine |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @gigincg, @nihal467, @bodhish, and @rithviknishad! Just following up on this PR when you have a moment. As a first-time contributor to the project, I'd love to get your feedback and make any necessary adjustments to get this merged. Thank you for your time and for maintaining this project! 🙌 |
Proposed Changes
Fixes #16066
TQuerythrough the shared request utilities.callApi,query,query.debounced, andquery.paginatedsoqueryParamspreserve the API definition’s typed query contract.TQuery, and replaceTReqwithTBodyinquestionnaireResponseTemplateApi.Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation