Added debouncing for APIs #9801
Conversation
from develop
merge fromdevelop branch
partially revert c9343d5 to fix investigation issues (ohcnetwork#9026)
WalkthroughThis pull request introduces a debouncing mechanism for search functionality across multiple components in the application. The changes focus on optimizing API calls by implementing a 500-millisecond delay when users type in search inputs. Additionally, the PR enhances internationalization support by adding translation hooks and updating locale files with new translatable strings for search-related messages and placeholders. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/FacilityOrganization/FacilityOrganizationView.tsx (1)
32-34: Adopt consistent naming.
debounceQueryis intuitive, but ensure that references to this function consistently denote debouncing logic across all components to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.env(1 hunks)src/pages/FacilityOrganization/FacilityOrganizationView.tsx(4 hunks)src/pages/Organization/OrganizationFacilities.tsx(3 hunks)src/pages/Organization/OrganizationView.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (18)
src/pages/FacilityOrganization/FacilityOrganizationView.tsx (5)
3-3: Well-structured import usage.Bringing in
useEffectanduseStatehere is necessary for managing side effects and state in your debouncing setup. This is clear and follows React best practices.
14-15: Good choice of a custom hook for debouncing.Importing
useDebounceindicates that you've encapsulated the debounce logic neatly. This helps keep the component code clean and reusable.
30-30: Ensure proper initialization value for debounced query.Initializing
debouncedQueryto an empty string is appropriate for controlling the initial search state. Just confirm that any dependent functionalities handle the empty string gracefully.
36-38: UseEffect dependencies and cleanup.Relying on
searchQueryin the effect ensures the debouncing triggers only when the user updates the search input, which is correct. However, consider if you need a cleanup or early-cancel mechanism if an API request is in-flight.Do you want me to propose a diff that handles cancellation of stale promises when the searchQuery changes?
47-47: Perfect alignment between queryKey and query parameters.Substituting
debouncedQueryensures fewer network calls while typing. Great job ensuring both thequeryKeyand thequeryParamsshare the same value.Also applies to: 55-55
src/pages/Organization/OrganizationView.tsx (6)
3-3: Consistent use of React hooks.Similar to the first file, importing
useEffectanduseStateis essential for implementing debouncing.
14-15: Reusable debouncing solution.Reusing
useDebounceacross different components encourages consistency and maintainability.
31-32: Initial debounced state clarity.Initializing
debouncedQueryto an empty string keeps your logic straightforward.
33-36: Naming alignment with prior components.Using
limitanddebounceQueryconsistently aligns with the prior file’s pattern. Keep an eye on potential naming collisions across the codebase, but here it looks organized.
37-39: Side effect dependencies.Effect depends on
searchQuery. Consider adding a teardown or cancellation to avoid race conditions if the user types very quickly, especially if the API queries are large.
41-41: Search performance improvement.Replacing direct references to
searchQuerywithdebouncedQueryis exactly what debouncing aims to do. This is an effective optimization.Also applies to: 47-47
src/pages/Organization/OrganizationFacilities.tsx (7)
3-3: Importing React hooks is consistent.The addition of
useEffect,useStatealigns with the rest of the codebase for state and side-effect management.
13-13: Clean separation of concerns by placing the custom debounce hook in "@/hooks/useDebounce".Keeping custom hooks in a dedicated location improve discoverability and maintainability.
35-36: Debounced parameters for filtering.Storing a separate
debouncedParamsstate is a good approach, ensuring a stable, debounced version ofqParams.
37-40: Well-structured debounce usage.You use
useDebounceto batch user input changes, then update the debounced state in one place. This pattern helps limit network calls.
41-44: JSON.stringify usage in dependencies.Serializing
qParamsbefore comparing changes can help detect deep object diffs. Just be mindful of performance or possible ordering differences—though in practice, it’s commonly used.
46-46: Efficient query key.Including both
idanddebouncedParamsclarifies caching scope. This is helpful for React Query to handle partial data.
49-53: Debouncing page, offset, and name achieves a uniform approach.Great to see consistent usage of debounced parameters for pagination and name-based filtering. This helps ensure consistent user experiences across the app.
is it okay |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/FacilityOrganization/FacilityOrganizationView.tsx(4 hunks)src/pages/Organization/OrganizationFacilities.tsx(3 hunks)src/pages/Organization/OrganizationView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/FacilityOrganization/FacilityOrganizationView.tsx
🔇 Additional comments (5)
src/pages/Organization/OrganizationView.tsx (4)
3-3: LGTM! Clean import organization.The imports are well-organized, separating React core imports, custom hooks, and components.
Also applies to: 14-15
36-38: LGTM! Proper debouncing implementation.The useEffect hook correctly updates debouncedParams when searchQuery changes, with a reasonable 500ms delay.
31-34: Consider adding error boundaries for the debounced state.While the debouncing implementation looks good, consider adding error handling for edge cases where the debounced state might fail to update.
43-43: Verify the impact of undefined name parameter.The code correctly handles empty search by setting name to undefined, but let's verify how the API handles this case.
Also applies to: 49-49
✅ Verification successful
The undefined name parameter is safely handled by React Query
The code uses React Query which automatically handles undefined parameters by omitting them from the request URL. When the name parameter is undefined, it will be excluded from the API request, making this implementation safe and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API parameter validation ast-grep --pattern 'name: $_ || undefined'Length of output: 44
Script:
#!/bin/bash # Search for organization children endpoint and parameter handling ast-grep --pattern 'router.$_("/organizations/$_/children"' # Also search for any file containing organization children route rg -l "organizations/.+/children" --type ts # Look for parameter validation related to organization children rg "name.*\?" --type ts -A 3 -B 3Length of output: 40381
Script:
#!/bin/bash # Search for organization children API endpoint implementation rg -l "api.*organizations.*children" --type ts # Look for route handlers or controllers fd -e ts -e js controller fd -e ts -e js route # Search for organization children query implementation ast-grep --pattern 'router.$_("/organizations/$_/children", async $_'Length of output: 171
src/pages/Organization/OrganizationFacilities.tsx (1)
13-14: Verify the implementation of useDebouncedState hook.Let's check the implementation of the custom hook to ensure it handles cleanup and edge cases properly.
✅ Verification successful
The useDebouncedState implementation is well-implemented and safe to use
The hook properly handles cleanup, maintains type safety, and correctly manages debounced state updates through a well-implemented useDebounce hook that includes proper timeout cleanup and edge case handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of useDebouncedState rg -A 10 "export.*useDebouncedState" --type ts --type tsxLength of output: 87
Script:
#!/bin/bash # First, find the hook file fd "useDebouncedState\.(ts|tsx)" # Then examine its content rg -A 15 "export.*useDebouncedState"Length of output: 478
Script:
#!/bin/bash # Find and examine useDebounce implementation fd "useDebounce\.(ts|tsx)" # Look for useDebounce implementation in case it's in a different file rg -A 20 "export.*useDebounce"Length of output: 1528
| const { data: facilities, isLoading } = useQuery({ | ||
| queryKey: ["organizationFacilities", id, qParams], | ||
| queryKey: ["organizationFacilities", id, debouncedParams], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update query parameters to match the debouncing split.
If implementing the suggested split of debounced parameters, update the query parameters accordingly.
queryKey: ["organizationFacilities", id, debouncedParams],
queryFn: query(routes.facility.list, {
queryParams: {
- page: debouncedParams.page,
+ page: qParams.page,
limit: resultsPerPage,
- offset: (debouncedParams.page - 1) * resultsPerPage,
+ offset: (qParams.page - 1) * resultsPerPage,
organization: id,
- name: debouncedParams.name,
+ name: debouncedSearch || undefined,
...advancedFilter.filter,
},
}),Also applies to: 45-49
| const [debouncedParams, setDebouncedParams] = useDebouncedState(qParams, 500); | ||
|
|
||
| useEffect(() => { | ||
| setDebouncedParams(qParams); | ||
| }, [qParams]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider debouncing the entire qParams object.
While debouncing works well for search queries, debouncing pagination parameters might lead to a degraded user experience. Consider splitting the debouncing logic to only apply to the search parameters.
-const [debouncedParams, setDebouncedParams] = useDebouncedState(qParams, 500);
+const [debouncedSearch, setDebouncedSearch] = useDebouncedState(qParams.name, 500);
useEffect(() => {
- setDebouncedParams(qParams);
+ setDebouncedSearch(qParams.name);
}, [qParams.name]);Committable suggestion skipped: line range outside the PR's diff.
@bodhish once told me to make it only for |
No, see how other debouncing works in care. For example: ValueSetSelect. |
Refer translation guidelines in README. And add translations for entire files that you've changed. |
i0am0arunava
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Organization/OrganizationView.tsx (2)
39-45: Consider adding error handling for the queryWhile the debounced query integration looks good, consider adding error handling to improve user experience when API calls fail.
const { data: children, isLoading } = useQuery({ queryKey: ["organization", id, "children", page, limit, debouncedParams], + onError: (error) => { + // Handle error appropriately + console.error('Failed to fetch organizations:', error); + }, queryFn: query(organizationApi.list, { queryParams: { parent: id,
Line range hint
31-67: Well-architected debouncing implementationThe overall implementation demonstrates good architectural decisions:
- Clear separation between immediate and debounced states
- Proper integration with React Query for data fetching
- Smooth user experience with loading states
- Consistent with the team's discussed approach in the PR comments
Consider documenting this pattern in the project guidelines as it could be useful for other similar implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/FacilityOrganization/FacilityOrganizationView.tsx(4 hunks)src/pages/Organization/OrganizationFacilities.tsx(4 hunks)src/pages/Organization/OrganizationView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/FacilityOrganization/FacilityOrganizationView.tsx
- src/pages/Organization/OrganizationFacilities.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (3)
src/pages/Organization/OrganizationView.tsx (3)
31-34: Well-implemented debouncing setup!The 500ms delay is a good balance between responsiveness and performance optimization. The separation of immediate and debounced states allows for a smooth user experience while reducing API calls.
67-67: Good implementation of the search handler!The handler correctly updates both immediate and debounced states while maintaining proper pagination behavior.
14-14: Verify the implementation of useDebouncedState hookLet's ensure the custom hook follows React's conventions and handles cleanup properly.
✅ Verification successful
Implementation verified - hooks are well-implemented and safe
The
useDebouncedStatehook and its dependencyuseDebounceare properly implemented with:
- Correct cleanup handling
- Proper TypeScript typing
- React hooks best practices
- Memory leak prevention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of useDebouncedState hook ast-grep --pattern 'export default function useDebouncedState($_) { $$$ }'Length of output: 78
Script:
#!/bin/bash # Find the hook file fd useDebouncedState.ts -t f fd useDebouncedState.tsx -t f # If found, display its contents fd useDebouncedState -t f -x cat {}Length of output: 675
Script:
#!/bin/bash # Check the implementation of useDebounce hook fd useDebounce.ts -t f fd useDebounce.tsx -t f fd useDebounce -t f -x cat {}Length of output: 1333
| @@ -25,16 +27,22 @@ interface Props { | |||
| export default function OrganizationView({ id, navOrganizationId }: Props) { | |||
| const [page, setPage] = useState(1); | |||
| const [searchQuery, setSearchQuery] = useState(""); | |||
|
|
|||
| const [debouncedParams, setDebouncedParams] = useDebouncedState( | |||
There was a problem hiding this comment.
nope. i don't see any purpose for having two states for debouncing 1 variable
| @@ -25,16 +27,22 @@ interface Props { | |||
| export default function OrganizationView({ id, navOrganizationId }: Props) { | |||
| const [page, setPage] = useState(1); | |||
| const [searchQuery, setSearchQuery] = useState(""); | |||
|
|
|||
| const [debouncedParams, setDebouncedParams] = useDebouncedState( | |||
There was a problem hiding this comment.
nope. i don't see any purpose for having two states for debouncing 1 variable
| @@ -25,16 +27,22 @@ interface Props { | |||
| export default function OrganizationView({ id, navOrganizationId }: Props) { | |||
| const [page, setPage] = useState(1); | |||
| const [searchQuery, setSearchQuery] = useState(""); | |||
There was a problem hiding this comment.
this should be debounced instead
There was a problem hiding this comment.
this should be debounced instead
i tried this one but problem is If searchQuery is debounced directly, the input's value={searchQuery} value will lag because the state only updates after the debounce delay
const [searchQuery, setSearchQuery] = useDebouncedState("", 500);
const limit = 12; // 3x4 grid
const { data: children, isLoading } = useQuery({
queryKey: ["organization", id, "children", page, limit, searchQuery],
queryFn: query(organizationApi.list, {
queryParams: {
parent: id,
offset: (page - 1) * limit,
limit,
name: searchQuery || undefined,
},
}),
});
return (
<Input
placeholder="Search by name..."
value={searchQuery}
onChange={(e) => {
setSearchQuery(e.target.value);
setPage(1); // Reset to first page on search
}}
className="w-full"
/>
);
To fix this, we need two states but for distinct purposes:
A searchQuery state to handle the immediate user input.
A debounced state (debouncedParams) for triggering the query.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/Organization/OrganizationView.tsx (1)
32-35: Consider consolidating state managementThe current implementation maintains two separate states (
searchQueryanddebouncedParams) for handling the search input. While this works, it could be simplified by using only the debounced state and managing the immediate UI feedback through controlled components.-const [searchQuery, setSearchQuery] = useState(""); -const [debouncedParams, setDebouncedParams] = useDebouncedState( - searchQuery, - 500, -); +const [searchQuery, setSearchQuery] = useDebouncedState("", 500);src/pages/Organization/OrganizationFacilities.tsx (1)
55-55: Remove debug console.log statementDebug logging should be removed before merging to production.
-console.log("ddd", debouncedParams);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/FacilityOrganization/FacilityOrganizationView.tsx(6 hunks)src/pages/Organization/OrganizationFacilities.tsx(7 hunks)src/pages/Organization/OrganizationView.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/FacilityOrganization/FacilityOrganizationView.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/pages/Organization/OrganizationView.tsx (3)
39-46: LGTM! Effective use of debounced parameters in the query configuration.The implementation correctly uses debounced parameters in both the query key and query parameters, ensuring that API calls are triggered only after the debounce delay.
62-68: LGTM! Well-implemented search input with translations and proper state management.The implementation correctly handles:
- Translated placeholder text
- Immediate UI feedback
- Page reset on search
- Debounced API calls
140-141: LGTM! Proper implementation of translated messages.The implementation correctly handles both cases with appropriate translations:
- When search yields no results
- When there are no sub-organizations
src/pages/Organization/OrganizationFacilities.tsx (2)
67-67: LGTM! Comprehensive implementation of translationsThe implementation correctly uses translations for:
- Search placeholder
- No results message
- Action buttons
Also applies to: 110-110, 153-153
38-46: 🛠️ Refactor suggestionSeparate debouncing for search and pagination
Currently, all query parameters including pagination are debounced. This could lead to a degraded user experience as pagination changes should be immediate.
Consider splitting the debouncing logic:
-const [debouncedParams, setDebouncedParams] = useDebouncedState(qParams, 500); +const [debouncedSearch, setDebouncedSearch] = useDebouncedState(qParams.name, 500); const { data: facilities, isLoading } = useQuery({ - queryKey: ["organizationFacilities", id, debouncedParams], + queryKey: ["organizationFacilities", id, qParams.page, debouncedSearch], queryFn: query(routes.facility.list, { queryParams: { - page: debouncedParams.page, + page: qParams.page, limit: resultsPerPage, - offset: (debouncedParams.page - 1) * resultsPerPage, + offset: (qParams.page - 1) * resultsPerPage, organization: id, - name: debouncedParams.name, + name: debouncedSearch || undefined, ...advancedFilter.filter, }, }),Likely invalid or redundant comment.
| onChange={(e) => { | ||
| updateQuery({ | ||
| name: e.target.value, | ||
| page: 1, | ||
| }) | ||
| } | ||
| }); | ||
| setDebouncedParams({ ...qParams, name: e.target.value, page: 1 }); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update onChange handler to match separated debouncing
The onChange handler should be updated to match the separated debouncing logic for search and pagination.
onChange={(e) => {
updateQuery({
name: e.target.value,
page: 1,
});
- setDebouncedParams({ ...qParams, name: e.target.value, page: 1 });
+ setDebouncedSearch(e.target.value);
}}📝 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.
| onChange={(e) => { | |
| updateQuery({ | |
| name: e.target.value, | |
| page: 1, | |
| }) | |
| } | |
| }); | |
| setDebouncedParams({ ...qParams, name: e.target.value, page: 1 }); | |
| }} | |
| onChange={(e) => { | |
| updateQuery({ | |
| name: e.target.value, | |
| page: 1, | |
| }); | |
| setDebouncedSearch(e.target.value); | |
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update onChange handler to match separated debouncing
The onChange handler should be updated to match the separated debouncing logic for search and pagination.
onChange={(e) => { updateQuery({ name: e.target.value, page: 1, }); - setDebouncedParams({ ...qParams, name: e.target.value, page: 1 }); + setDebouncedSearch(e.target.value); }}📝 Committable suggestion
no ,can not do this , this will return type error Argument of type 'string' is not assignable to parameter of type 'SetStateAction<QueryParam>'.ts(2345)
There was a problem hiding this comment.
@i0am0arunava You're absolutely right. I apologize for the incorrect suggestion. Your current implementation using setDebouncedParams with the full query params object is the correct approach as it maintains proper TypeScript type safety. Please keep the existing implementation.
:thinking_face:
⟪ ⬚⬚⬚⬚⬚⬚⬚ ⟫
⟪ ⬚🐰⬚⬚⬚⬚⬚ ⟫
⟪ ⬚⬚⬚⬚⬚⬚⬚ ⟫
✏️ Learnings added
Learnt from: i0am0arunava
PR: ohcnetwork/care_fe#9801
File: src/pages/Organization/OrganizationFacilities.tsx:69-75
Timestamp: 2025-01-09T08:56:20.006Z
Learning: When using useDebouncedState with query parameters in TypeScript, ensure the debounced state maintains the same type as the original state (QueryParam). Avoid splitting into separate string states as it breaks type safety.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
-
Not a fan of how debouncing is achieved here by introducing a new state and all... See Adds
query.debouncedand updated queries that require debouncing #9865 on how it's achieved. -
this does not cover most of the places where debouncing is required. you could've tried a regex search to somewhat get a larger collection of queries that require debouncing
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization