Replaced the older components with ShadCN components in the ResourceCreate Page #10367
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes include adding two new localization entries in the English locale file, removing the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as ResourceCreate Form
participant A as Autocomplete Component
participant F as Facility API
U->>R: Interact with facility field
R->>A: Render Autocomplete with query parameters
U->>A: Type search term
A->>F: Query facilities using search term
F-->>A: Return facility options
A->>U: Display suggestions
U->>A: Select a facility
A->>R: Trigger onChange with selected facility
R-->>U: Update form state
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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: 1
🧹 Nitpick comments (1)
src/components/Resource/ResourceCreate.tsx (1)
112-115: Consider memoizing facilityOptions for better performance.The options array is recreated on every render. Consider using useMemo to optimize performance:
- const facilityOptions = facilitiesResponse?.results.map((facility) => ({ + const facilityOptions = useMemo(() => facilitiesResponse?.results.map((facility) => ({ label: facility.name, value: facility.id, - })); + })), [facilitiesResponse?.results]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Common/FacilitySelect.tsx(0 hunks)src/components/Resource/ResourceCreate.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- src/components/Common/FacilitySelect.tsx
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (3)
src/components/Resource/ResourceCreate.tsx (3)
14-14: LGTM! Clean import organization for new dependencies.The new imports are well-organized and properly support the migration to ShadCN components.
Also applies to: 49-52
97-110: LGTM! Well-structured query with proper typing and debouncing.The facility query implementation is clean and includes proper error handling through the query hook.
131-165: LGTM! Form submission logic is robust and well-typed.The form submission logic maintains good error handling and type safety while integrating well with the new Autocomplete component.
| <Autocomplete | ||
| options={facilityOptions ?? []} | ||
| value={field.value?.id ?? ""} | ||
| placeholder={t("start_typing_to_search")} | ||
| onChange={(value) => { | ||
| const facility = | ||
| facilitiesResponse?.results.find( | ||
| (f) => f.id === value, | ||
| ) ?? null; | ||
| form.setValue("assigned_facility", facility); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for facility selection.
The facility lookup in onChange could be optimized and should include error handling:
<Autocomplete
options={facilityOptions ?? []}
value={field.value?.id ?? ""}
placeholder={t("start_typing_to_search")}
onChange={(value) => {
+ if (!value) {
+ form.setValue("assigned_facility", null);
+ return;
+ }
const facility =
facilitiesResponse?.results.find(
(f) => f.id === value,
) ?? null;
+ if (!facility) {
+ console.error("Selected facility not found in results");
+ return;
+ }
form.setValue("assigned_facility", facility);
}}
/>📝 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.
| <Autocomplete | |
| options={facilityOptions ?? []} | |
| value={field.value?.id ?? ""} | |
| placeholder={t("start_typing_to_search")} | |
| onChange={(value) => { | |
| const facility = | |
| facilitiesResponse?.results.find( | |
| (f) => f.id === value, | |
| ) ?? null; | |
| form.setValue("assigned_facility", facility); | |
| }} | |
| /> | |
| <Autocomplete | |
| options={facilityOptions ?? []} | |
| value={field.value?.id ?? ""} | |
| placeholder={t("start_typing_to_search")} | |
| onChange={(value) => { | |
| if (!value) { | |
| form.setValue("assigned_facility", null); | |
| return; | |
| } | |
| const facility = | |
| facilitiesResponse?.results.find( | |
| (f) => f.id === value, | |
| ) ?? null; | |
| if (!facility) { | |
| console.error("Selected facility not found in results"); | |
| return; | |
| } | |
| form.setValue("assigned_facility", facility); | |
| }} | |
| /> |
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 (2)
public/locale/en.json(2 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceDetailsUpdate.tsx (1)
Learnt from: AdityaJ2305
PR: ohcnetwork/care_fe#10345
File: src/components/Resource/ResourceDetailsUpdate.tsx:132-145
Timestamp: 2025-01-31T22:13:06.153Z
Learning: The migration from useTanStackQueryInstead to useQuery in React components requires:
1. Updating types to use UseQueryResult instead of custom hook return type
2. Replacing loading with isLoading property
3. Using queryKey array for cache management
4. Using queryFn with query utility function
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/components/Resource/ResourceDetailsUpdate.tsx (1)
152-170: LGTM! Well-implemented facility data fetching.The implementation follows React Query best practices with:
- Proper cache key management including query params
- Debounced query for performance
- Type-safe response handling
- Clean mapping to Autocomplete options format
| <div> | ||
| <FieldLabel> | ||
| What facility would you like to assign the request to | ||
| </FieldLabel> | ||
| <FacilitySelect | ||
| multiple={false} | ||
| name="assigned_facility" | ||
| selected={state.form.assigned_facility} | ||
| setSelected={(obj) => setFacility(obj, "assigned_facility")} | ||
| errors={state.errors.assigned_facility} | ||
| <Label className="text-gray-700 -mt-3 mb-3"> | ||
| {t("facility_assign_request")} | ||
| </Label> | ||
| <Autocomplete | ||
| options={facilityOptions ?? []} | ||
| placeholder={t("start_typing_to_search")} | ||
| value={state.form.assigned_facility} | ||
| onChange={(selected) => | ||
| setFacility(selected, "assigned_facility") | ||
| } | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for facility selection.
The previous FacilitySelect component likely included error display functionality that needs to be maintained for form validation.
Add error handling to match the pattern used in other form fields:
<div>
<Label className="text-gray-700 -mt-3 mb-3">
{t("facility_assign_request")}
</Label>
<Autocomplete
options={facilityOptions ?? []}
placeholder={t("start_typing_to_search")}
value={state.form.assigned_facility}
onChange={(selected) =>
setFacility(selected, "assigned_facility")
}
+ error={state.errors.assigned_facility}
/>
+ {state.errors.assigned_facility && (
+ <p className="text-red-500 text-sm mt-2">
+ {state.errors.assigned_facility}
+ </p>
+ )}
</div>📝 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.
| <div> | |
| <FieldLabel> | |
| What facility would you like to assign the request to | |
| </FieldLabel> | |
| <FacilitySelect | |
| multiple={false} | |
| name="assigned_facility" | |
| selected={state.form.assigned_facility} | |
| setSelected={(obj) => setFacility(obj, "assigned_facility")} | |
| errors={state.errors.assigned_facility} | |
| <Label className="text-gray-700 -mt-3 mb-3"> | |
| {t("facility_assign_request")} | |
| </Label> | |
| <Autocomplete | |
| options={facilityOptions ?? []} | |
| placeholder={t("start_typing_to_search")} | |
| value={state.form.assigned_facility} | |
| onChange={(selected) => | |
| setFacility(selected, "assigned_facility") | |
| } | |
| /> | |
| </div> | |
| <div> | |
| <Label className="text-gray-700 -mt-3 mb-3"> | |
| {t("facility_assign_request")} | |
| </Label> | |
| <Autocomplete | |
| options={facilityOptions ?? []} | |
| placeholder={t("start_typing_to_search")} | |
| value={state.form.assigned_facility} | |
| onChange={(selected) => | |
| setFacility(selected, "assigned_facility") | |
| } | |
| error={state.errors.assigned_facility} | |
| /> | |
| {state.errors.assigned_facility && ( | |
| <p className="text-red-500 text-sm mt-2"> | |
| {state.errors.assigned_facility} | |
| </p> | |
| )} | |
| </div> |
| @@ -1,20 +1,20 @@ | |||
| import { useQuery } from "@tanstack/react-query"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Migrate Query Hooks for Uniformity
The ResourceDetailsUpdate.tsx file still calls the legacy useTanStackQueryInstead (see line 32), even though it now imports useQuery from "@tanstack/react-query". Based on our repository-wide migration efforts (see PR #10345), all data-fetching logic should consistently use useQuery.
- Replace all
useTanStackQueryInsteadusages withuseQueryin ResourceDetailsUpdate.tsx. - Ensure that legacy wrappers are eventually removed or fully migrated for consistency.
🔗 Analysis chain
Consider migrating all queries to useQuery for consistency.
The file uses both useTanStackQueryInstead and useQuery hooks. Based on the retrieved learnings from PR #10345, we should consistently use useQuery across the codebase.
Let's verify other query usages in the codebase:
Also applies to: 32-32
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of both hooks to assess migration status
echo "Searching for useTanStackQueryInstead usage..."
rg "useTanStackQueryInstead"
echo -e "\nSearching for @tanstack/react-query useQuery usage..."
rg "useQuery.*from.*@tanstack/react-query"
Length of output: 14985
|
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.
|
NikhilA8606
changed the title
…k#10048/Upgrading-old-UI-components-in-ResourceCreate-with-Shadcn-component
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Resource/ResourceCreate.tsx (1)
133-147: Add error handling and loading state for facilities query.While the implementation is good, it could benefit from error handling and loading states.
const { data: facilities } = useQuery({ queryKey: ["facilities", qParams], queryFn: query.debounced(facilityApi.getAllFacilities, { queryParams: { name: qParams.name, facility_type: qParams.facility_type, organization: qParams.organization, }, }), + onError: (error) => { + toast.error(t("error_loading_facilities")); + console.error("Error loading facilities:", error); + } }); const facilityOptions = facilities?.results.map((facility) => ({ label: facility.name, value: facility.id, })); + + const isLoadingFacilities = !facilities && qParams.name;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(2 hunks)src/components/Form/AutoCompleteAsync.tsx(0 hunks)src/components/Resource/ResourceCreate.tsx(6 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(4 hunks)src/hooks/useDebounce.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/hooks/useDebounce.ts
- src/components/Form/AutoCompleteAsync.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceDetailsUpdate.tsx (1)
Learnt from: AdityaJ2305
PR: ohcnetwork/care_fe#10345
File: src/components/Resource/ResourceDetailsUpdate.tsx:132-145
Timestamp: 2025-01-31T22:13:06.153Z
Learning: The migration from useTanStackQueryInstead to useQuery in React components requires:
1. Updating types to use UseQueryResult instead of custom hook return type
2. Replacing loading with isLoading property
3. Using queryKey array for cache management
4. Using queryFn with query utility function
🔇 Additional comments (4)
src/components/Resource/ResourceCreate.tsx (2)
13-13: LGTM! Clean imports for ShadCN components.The new imports align with the PR objectives of migrating to ShadCN components.
Also applies to: 25-25, 50-50
204-219: Add error handling for facility selection.The facility lookup in onChange could be optimized and should include error handling.
src/components/Resource/ResourceDetailsUpdate.tsx (2)
1-1: Migrate all query hooks to useQuery for consistency.Based on the retrieved learnings from PR #10345, we should consistently use
useQueryacross the codebase.Also applies to: 32-32
244-256: Add error handling for facility selection.The Autocomplete component should display validation errors to maintain consistency with other form fields.
| const { data: facilitiesResponse } = useQuery({ | ||
| queryKey: ["facilities", qParams], | ||
| queryFn: query.debounced(facilityApi.getAllFacilities, { | ||
| queryParams: { | ||
| name: qParams.name, | ||
| facility_type: qParams.facility_type, | ||
| organization: qParams.organization, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const facilityOptions = facilitiesResponse?.results.map((facility) => ({ | ||
| label: facility.name, | ||
| value: facility.id, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for facility data fetching.
The facility query should handle error cases to prevent undefined behavior when the API call fails.
- const { data: facilitiesResponse } = useQuery({
+ const { data: facilitiesResponse, error: facilitiesError } = useQuery({
queryKey: ["facilities", qParams],
queryFn: query.debounced(facilityApi.getAllFacilities, {
queryParams: {
name: qParams.name,
facility_type: qParams.facility_type,
organization: qParams.organization,
},
}),
});
+ const facilityOptions = facilitiesError
+ ? []
+ : facilitiesResponse?.results.map((facility) => ({
label: facility.name,
value: facility.id,
}));📝 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 { data: facilitiesResponse } = useQuery({ | |
| queryKey: ["facilities", qParams], | |
| queryFn: query.debounced(facilityApi.getAllFacilities, { | |
| queryParams: { | |
| name: qParams.name, | |
| facility_type: qParams.facility_type, | |
| organization: qParams.organization, | |
| }, | |
| }), | |
| }); | |
| const facilityOptions = facilitiesResponse?.results.map((facility) => ({ | |
| label: facility.name, | |
| value: facility.id, | |
| })); | |
| const { data: facilitiesResponse, error: facilitiesError } = useQuery({ | |
| queryKey: ["facilities", qParams], | |
| queryFn: query.debounced(facilityApi.getAllFacilities, { | |
| queryParams: { | |
| name: qParams.name, | |
| facility_type: qParams.facility_type, | |
| organization: qParams.organization, | |
| }, | |
| }), | |
| }); | |
| const facilityOptions = facilitiesError | |
| ? [] | |
| : facilitiesResponse?.results.map((facility) => ({ | |
| label: facility.name, | |
| value: facility.id, | |
| })); |
| <FormLabel required> | ||
| {t("facility_for_care_support")} | ||
| </FormLabel> | ||
| <Label className="text-gray-700 -mt-3 mb-3"> | ||
| {t("facility_assign_request")} | ||
| </Label> |
There was a problem hiding this comment.
As usual i converted label to shadcn Label
There was a problem hiding this comment.
FormLabel is the one's that's to be used when using useForm
| @@ -58,6 +60,7 @@ export default function ResourceCreate(props: ResourceProps) { | |||
| const { t } = useTranslation(); | |||
| const [{ related_patient }] = useQueryParams(); | |||
| const authUser = useAuthUser(); | |||
| const [qParams, _] = useQueryParams(); | |||
There was a problem hiding this comment.
why use query params here?
| queryParams: { | ||
| name: qParams.name, | ||
| facility_type: qParams.facility_type, | ||
| organization: qParams.organization, | ||
| }, |
rithviknishad
added
invalid
rithviknishad
added
needs testing
needs review
and removed
invalid
rithviknishad
requested review from
AdityaJ2305 and
Jacobjeevan
and removed request for
AdityaJ2305
| @@ -183,14 +201,21 @@ export default function ResourceCreate(props: ResourceProps) { | |||
| render={({ field }) => ( | |||
| <FormItem> | |||
| <FormLabel required> | |||
| {t("facility_for_care_support")} | |||
| {t("facility_assign_request")} | |||
There was a problem hiding this comment.
| {t("facility_assign_request")} | |
| {t("facility_for_care_support")} |
Let's switch this back as it was a product request.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
…k#10048/Upgrading-old-UI-components-in-ResourceCreate-with-Shadcn-component
rithviknishad
added
tested
and removed
needs testing
merge conflict
|
@NikhilA8606 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! 🙌 |
Migrated all older components in the ResourceCreate page using Shadcn component
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
FacilitySelectandAutoCompleteAsynccomponents, as well as theuseDebouncehook.