Basic CRUD pages for Devices #10560
Basic CRUD pages for Devices #10560
Conversation
WalkthroughThis change introduces new localization keys in the English locale file and implements several new and updated React components to manage devices and locations. The updates include components for creating, updating, listing, and detailing devices, as well as components to associate locations with devices. Enhanced internationalization is applied in location-related components, and routes are added for device management within facility settings. Additionally, new TypeScript definitions and API endpoints are added for contact points, devices, and location forms. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CreateDevice Component
participant F as DeviceForm Component
participant API as Device API
participant Nav as Navigation (raviger)
U->>C: Navigate to "Create Device" page
C->>F: Render DeviceForm with facility ID
F->>API: Submit form data
API-->>F: Return success response
F->>C: Execute onSuccess callback
C->>Nav: Redirect to Devices List page
sequenceDiagram
participant U as User
participant S as AssociateLocationSheet
participant API as Device API
participant Q as Query Client & Toast
U->>S: Open location association sheet
S->>API: Submit selected location for association
API-->>S: Return confirmation
S->>Q: Invalidate queries and show success toast
S->>U: Close sheet and reset selection
Possibly related PRs
Suggested reviewers
Poem
✨ 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. |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Deploying care-fe with
|
| Latest commit: |
850e8c3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://20b63ba6.care-fe.pages.dev |
| Branch Preview URL: | https://rithviknishad-feat-devices.care-fe.pages.dev |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
rithviknishad/feat/devices
|
| Run status |
|
| Run duration | 07m 02s |
| Commit |
|
| Committer | Rithvik Nishad |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
11
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/components/Location/LocationSearch.tsx (1)
41-48: 🛠️ Refactor suggestionAdd debouncing to the search input.
The search query is triggered on every keystroke which could lead to unnecessary API calls.
const { data: locations } = useQuery({ queryKey: ["locations", facilityId, mode, search], queryFn: query(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { mode, name: search }, }), enabled: facilityId !== "preview", + staleTime: 1000, + refetchOnWindowFocus: false, });
🧹 Nitpick comments (18)
src/pages/Facility/settings/devices/DeviceDetail.tsx (3)
58-66: Consider handling device deletion errors.Currently, there's an
onSuccesscallback but noonErrorhandling in the mutation. If deletion fails, there's no user feedback. Provide anonErrorcallback to show an error message or toast to the user.You might add an
onErroroption:const { mutate: deleteDevice, isPending: isDeleting } = useMutation({ mutationFn: mutate(deviceApi.delete, { pathParams: { facility_id: facilityId, id: deviceId }, }), + onError: (error) => { + // Show error notification/toast + console.error("Device deletion failed:", error); + }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["devices"] }); navigate(`/facility/${facilityId}/settings/devices`); }, });
76-87: Refactor repeated badge color logic if appropriate.
getStatusColorandgetAvailabilityStatusColorshare a similar structure. Consider creating a single utility function or a dictionary-based approach to map status to color classes, consolidating logic for easier maintenance.
143-376: Enhance readability for repeated info fields.In the detailed card sections (lines 185–344), fields are repeated in a pattern that includes a heading, small text, and a value. Extracting these into smaller, reusable sub-components (e.g.,
<DetailField label="" value="" />) can simplify the structure, reduce duplication, and make your codebase more maintainable.Consider advanced date formatting.
For
manufacture_dateandexpiration_date(lines 321–343), consider using libraries like date-fns or dayjs for more consistent, localized date formatting. The currenttoLocaleDateStringmay vary based on browser locale settings and might not align with user preferences in every region.src/pages/Facility/settings/devices/CreateDevice.tsx (1)
14-32: Confirm error handling or feedback when device creation fails.While
DeviceFormlikely handles form validation, there’s no indication of a fallback or retry mechanism in case of creation failure. Consider adding error notifications inDeviceFormor here to inform the user if the device creation fails.src/types/device/deviceApi.ts (1)
42-46: Consider adding validation for location ID.The
associateLocationendpoint accepts a location ID as a string without any validation. Consider adding a more specific type or validation to ensure the location ID is in the correct format.- TBody: Type<{ location: string }>(), + TBody: Type<{ location: string & { readonly brand: 'LocationId' } }>(),src/types/device/device.ts (2)
29-30: Consider using a more specific date type.The
manufacture_dateandexpiration_datefields are typed asstringwith a comment indicating they are datetime fields. Consider using a more specific type to enforce proper datetime format.- manufacture_date?: string; // datetime - expiration_date?: string; // datetime + manufacture_date?: string & { readonly brand: 'ISO8601DateTime' }; + expiration_date?: string & { readonly brand: 'ISO8601DateTime' };
38-38: Remove commented-out code.The commented-out
care_typefield should be removed if it's not being used.- // care_type: string | undefined;src/pages/Facility/settings/devices/UpdateDevice.tsx (1)
42-44: Consider using a more explicit navigation method.Using
window.history.back()might lead to unexpected navigation if the user arrived at this page through a different path. Consider using theLinkcomponent or a more explicit navigation method.- onSuccess={() => { - window.history.back(); - }} + onSuccess={() => { + navigate(`/facility/${facilityId}/settings/devices/${deviceId}`); + }}src/types/common/contactPoint.ts (2)
63-64: Enhance validation error messages.The error messages for pager validation could be more descriptive to help users understand the requirements.
- .min(1, { message: "Required" }) - .max(20, { message: "Pager number too long" }), + .min(1, { message: "Pager number is required" }) + .max(20, { message: "Pager number must be 20 characters or less" }),
31-31: Consider adding country-specific phone number validation.The phone number validation could be enhanced to support country-specific formats and validation rules.
Consider using a library like
libphonenumber-jsfor more robust phone number validation:import { parsePhoneNumber, isValidPhoneNumber } from 'libphonenumber-js' const phoneNumberValidator = z.string().refine( (value) => isValidPhoneNumber(value), { message: "Invalid phone number format" } )Also applies to: 37-37, 55-55
src/pages/Facility/settings/devices/components/AssociateLocationSheet.tsx (2)
55-58: Add type safety to the handleSubmit function.The function should explicitly type the return value for better type safety.
-const handleSubmit = () => { +const handleSubmit = (): void => { if (!selectedLocation) return; associateLocation({ location: selectedLocation.id }); };
60-86: Add loading state feedback in the UI.The component shows loading state in the button but could provide better visual feedback during the mutation.
<SheetContent> + {isPending && ( + <div className="absolute inset-0 bg-white/50 flex items-center justify-center"> + <LoadingSpinner /> + </div> + )} <SheetHeader>src/pages/Facility/settings/devices/components/DeviceCard.tsx (1)
22-33: Refactor status color functions to improve maintainability.The status color functions contain duplicated logic and could be simplified using a lookup object.
+const STATUS_COLORS = { + active: "bg-green-100 text-green-800 hover:bg-green-100/80", + inactive: "bg-gray-100 text-gray-800 hover:bg-gray-100/80", + entered_in_error: "bg-red-100 text-red-800 hover:bg-red-100/80", + available: "bg-green-100 text-green-800 hover:bg-green-100/80", + lost: "bg-yellow-100 text-yellow-800 hover:bg-yellow-100/80", + damaged: "bg-red-100 text-red-800 hover:bg-red-100/80", + destroyed: "bg-red-100 text-red-800 hover:bg-red-100/80", +} as const; + -const getStatusColor = (status: string) => { +const getStatusColor = (status: keyof typeof STATUS_COLORS) => { - switch (status) { - case "active": - return "bg-green-100 text-green-800 hover:bg-green-100/80"; - // ... - } + return STATUS_COLORS[status] || STATUS_COLORS.inactive; };Also applies to: 35-47
src/pages/Facility/settings/devices/DevicesList.tsx (1)
78-78: Fix unused parameter in onChange callback.The onChange callback has an unused parameter that should be removed.
-onChange={(page, _) => setPage(page)} +onChange={(page) => setPage(page)}src/components/Location/LocationSearch.tsx (1)
82-83: Add type safety for translation keys.The translation keys are not type-safe and could lead to runtime errors if the keys don't exist.
Consider using a type-safe translation solution or adding runtime checks for key existence.
Also applies to: 87-87
src/pages/Facility/settings/devices/components/DeviceForm.tsx (3)
89-101: Consider optimistic updates for better UX.The mutation is correctly set up, but consider implementing optimistic updates to improve user experience.
const { mutate: submitForm, isPending } = useMutation({ mutationFn: device?.id ? mutate(deviceApi.update, { pathParams: { facility_id: facilityId, id: device.id }, }) : mutate(deviceApi.create, { pathParams: { facility_id: facilityId }, }), onMutate: async (newDevice) => { // Cancel outgoing refetches await queryClient.cancelQueries({ queryKey: ['devices'] }); // Snapshot previous value const previousDevices = queryClient.getQueryData(['devices']); // Optimistically update queryClient.setQueryData(['devices'], old => [...old, newDevice]); return { previousDevices }; }, onError: (err, newDevice, context) => { // Rollback on error queryClient.setQueryData(['devices'], context.previousDevices); }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['devices'] }); onSuccess?.(); }, });
326-429: Consider extracting contact points to a separate component.The contact points section is quite large and could be extracted into a separate component for better maintainability.
// ContactPoints.tsx interface ContactPointsProps { fields: any[]; form: any; append: () => void; remove: (index: number) => void; } function ContactPoints({ fields, form, append, remove }: ContactPointsProps) { const { t } = useTranslation(); return ( <div className="space-y-2"> {/* ... existing contact points JSX ... */} </div> ); } // Usage in DeviceForm <ContactPoints fields={fields} form={form} append={() => append({ system: ContactPointSystems[0], value: "", use: "work", })} remove={remove} />
431-436: Consider adding form validation feedback.The submit button could provide better feedback about form validation state.
<div className="flex justify-end gap-4"> - <Button type="submit" disabled={isPending}> + <Button + type="submit" + disabled={isPending || !form.formState.isValid} + variant={form.formState.isValid ? "default" : "destructive"} + > {isPending ? t("saving") : t("save")} </Button> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
public/locale/en.json(28 hunks)src/components/Location/LocationSearch.tsx(5 hunks)src/pages/Facility/settings/devices/CreateDevice.tsx(1 hunks)src/pages/Facility/settings/devices/DeviceDetail.tsx(1 hunks)src/pages/Facility/settings/devices/DevicesList.tsx(1 hunks)src/pages/Facility/settings/devices/UpdateDevice.tsx(1 hunks)src/pages/Facility/settings/devices/components/AssociateLocationSheet.tsx(1 hunks)src/pages/Facility/settings/devices/components/DeviceCard.tsx(1 hunks)src/pages/Facility/settings/devices/components/DeviceForm.tsx(1 hunks)src/pages/Facility/settings/layout.tsx(3 hunks)src/pages/Facility/settings/locations/LocationForm.tsx(3 hunks)src/pages/Facility/settings/locations/LocationView.tsx(2 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(2 hunks)src/pages/Facility/settings/organizations/components/CreateFacilityOrganizationSheet.tsx(1 hunks)src/types/common/contactPoint.ts(1 hunks)src/types/device/device.ts(1 hunks)src/types/device/deviceApi.ts(1 hunks)src/types/location/location.ts(2 hunks)vite.config.mts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pages/Facility/settings/organizations/components/CreateFacilityOrganizationSheet.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (15)
src/pages/Facility/settings/devices/CreateDevice.tsx (1)
1-2: Imports appear correct.The usage of
navigateanduseTranslationis consistent with the rest of the codebase. No immediate concerns.src/types/location/location.ts (2)
12-12: Nicely implemented indexed type for LocationForm.Using
(typeof LocationFormOptions)[number]is a concise approach to ensure type consistency with the array values.
43-59: Flexible definition of LocationFormOptions.This approach gives you future-proofing by allowing more entries without additional union definitions. Verify all references to the old label-based location forms are updated accordingly in other files.
Would you like a script to ensure no references to removed constants or functions remain?
src/pages/Facility/settings/layout.tsx (3)
9-12: LGTM! Device management components properly imported.The imports are well-organized and follow the project's import structure.
38-45: LGTM! Device routes are properly configured.The routes are well-structured with consistent parameter passing:
- List view at
/devices- Create view at
/devices/create- Detail view at
/devices/:id- Edit view at
/devices/:id/edit
75-79: LGTM! Device tab integration looks good.The devices tab is properly integrated into the settings menu with correct translation support.
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
86-86:❓ Verification inconclusive
Verify translation keys exist for all location forms.
The change to use translation keys (
location_form__${location.form}) is good, but we should ensure all possible form values have corresponding translation entries.
🏁 Script executed:
#!/bin/bash # Description: Check if all location form types have corresponding translation keys # Get all possible form values from the type definition echo "Checking for translation keys in locale files..." for form in $(rg -o 'LocationFormOptions = \[(.*?)\]' -r '$1' | tr -d ' ' | tr ',' '\n' | tr -d '"'); do echo "Checking translation key: location_form__$form" rg "\"location_form__$form\":" public/locale/ doneLength of output: 196
Translation Keys Verification – Manual Check Recommended
The refactor to use the translation key{t(`location_form__${location.form}`)}instead of a function-based label is in line with our current approach. However, the automated script to verify that every possible location form value has a corresponding translation key produced no detectable output. This makes it unclear whether the locale files definitively include keys for all forms.
- Action Item: Please ensure manually that for every form value defined (e.g., those in your
LocationFormOptions), there exists a matching translation key (e.g.,"location_form__<form_value>": ...) in thepublic/locale/files.src/pages/Facility/settings/locations/LocationView.tsx (1)
157-157: LGTM! Consistent translation key usage.The change maintains consistency with other components by using the same translation key pattern.
src/pages/Facility/settings/locations/LocationForm.tsx (2)
43-43: LGTM! Improved type safety with LocationFormOptions enum.The form schema now correctly uses the LocationFormOptions enum, ensuring type safety and consistency.
211-214: LGTM! Consistent translation approach in form options.The SelectContent implementation:
- Correctly maps over LocationFormOptions array
- Uses consistent translation key pattern
- Maintains type safety with proper key/value handling
vite.config.mts (1)
291-291: Verify security implications of allowing all hosts.Setting
allowedHosts: trueallows requests from any host. This could potentially expose your development server to security risks.Consider using a more restrictive configuration by specifying allowed hosts explicitly:
- allowedHosts: true, + allowedHosts: [ + 'localhost', + '.your-domain.com' // Allows all subdomains of your-domain.com + ],src/pages/Facility/settings/devices/components/DeviceForm.tsx (3)
43-57: LGTM! Well-structured form schema with proper validation.The form schema is well-defined using zod with appropriate validation rules and optional fields.
103-107: LGTM! Proper form reset on device prop change.The effect correctly resets the form when the device prop changes.
109-120: LGTM! Proper date formatting.The effect correctly formats dates using the dateQueryString utility.
public/locale/en.json (1)
746-756: LGTM! Well-structured localization keys.The device-related localization keys follow consistent naming patterns and provide comprehensive coverage for the device management features.
Also applies to: 747-750, 751-756
| const { mutate: associateLocation, isPending } = useMutation({ | ||
| mutationFn: mutate(deviceApi.associateLocation, { | ||
| pathParams: { facility_id: facilityId, id: deviceId }, | ||
| }), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: ["device", facilityId, deviceId], | ||
| }); | ||
| toast.success(t("location_associated_successfully")); | ||
| onOpenChange(false); | ||
| setSelectedLocation(null); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the mutation.
The mutation lacks error handling which could lead to a poor user experience if the API call fails.
Add error handling to the mutation:
const { mutate: associateLocation, isPending } = useMutation({
mutationFn: mutate(deviceApi.associateLocation, {
pathParams: { facility_id: facilityId, id: deviceId },
}),
onSuccess: () => {
queryClient.invalidateQueries({
queryKey: ["device", facilityId, deviceId],
});
toast.success(t("location_associated_successfully"));
onOpenChange(false);
setSelectedLocation(null);
},
+ onError: (error) => {
+ toast.error(t("failed_to_associate_location"));
+ console.error("Failed to associate location:", error);
+ },
});📝 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 { mutate: associateLocation, isPending } = useMutation({ | |
| mutationFn: mutate(deviceApi.associateLocation, { | |
| pathParams: { facility_id: facilityId, id: deviceId }, | |
| }), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ | |
| queryKey: ["device", facilityId, deviceId], | |
| }); | |
| toast.success(t("location_associated_successfully")); | |
| onOpenChange(false); | |
| setSelectedLocation(null); | |
| }, | |
| }); | |
| const { mutate: associateLocation, isPending } = useMutation({ | |
| mutationFn: mutate(deviceApi.associateLocation, { | |
| pathParams: { facility_id: facilityId, id: deviceId }, | |
| }), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ | |
| queryKey: ["device", facilityId, deviceId], | |
| }); | |
| toast.success(t("location_associated_successfully")); | |
| onOpenChange(false); | |
| setSelectedLocation(null); | |
| }, | |
| onError: (error) => { | |
| toast.error(t("failed_to_associate_location")); | |
| console.error("Failed to associate location:", error); | |
| }, | |
| }); |
| <Link href={`/devices/${device.id}`} className="block h-[160px]"> | ||
| <Card className="hover:shadow-md transition-shadow h-full"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA labels for better accessibility.
The link lacks proper ARIA labels for screen readers.
-<Link href={`/devices/${device.id}`} className="block h-[160px]">
+<Link
+ href={`/devices/${device.id}`}
+ className="block h-[160px]"
+ aria-label={t("view_device_details", { name: device.registered_name })}
+>📝 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.
| <Link href={`/devices/${device.id}`} className="block h-[160px]"> | |
| <Card className="hover:shadow-md transition-shadow h-full"> | |
| <Link | |
| href={`/devices/${device.id}`} | |
| className="block h-[160px]" | |
| aria-label={t("view_device_details", { name: device.registered_name })} | |
| > | |
| <Card className="hover:shadow-md transition-shadow h-full"> |
| const { data, isLoading } = useQuery({ | ||
| queryKey: ["devices", facilityId, page, limit], | ||
| queryFn: query.debounced(deviceApi.list, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| offset: (page - 1) * limit, | ||
| limit, | ||
| }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the query.
The query lacks error handling and retry configuration.
const { data, isLoading } = useQuery({
queryKey: ["devices", facilityId, page, limit],
queryFn: query.debounced(deviceApi.list, {
pathParams: { facility_id: facilityId },
queryParams: {
offset: (page - 1) * limit,
limit,
},
}),
+ retry: 2,
+ onError: (error) => {
+ console.error("Failed to fetch devices:", error);
+ toast.error(t("failed_to_fetch_devices"));
+ },
});📝 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, isLoading } = useQuery({ | |
| queryKey: ["devices", facilityId, page, limit], | |
| queryFn: query.debounced(deviceApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| offset: (page - 1) * limit, | |
| limit, | |
| }, | |
| }), | |
| }); | |
| const { data, isLoading } = useQuery({ | |
| queryKey: ["devices", facilityId, page, limit], | |
| queryFn: query.debounced(deviceApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| offset: (page - 1) * limit, | |
| limit, | |
| }, | |
| }), | |
| retry: 2, | |
| onError: (error) => { | |
| console.error("Failed to fetch devices:", error); | |
| toast.error(t("failed_to_fetch_devices")); | |
| }, | |
| }); |
| const formatLocationParent = (location: LocationList) => { | ||
| const parents: string[] = []; | ||
| while (location.parent?.name) { | ||
| parents.push(location.parent?.name); | ||
| location = location.parent; | ||
| } | ||
| return parents.reverse().join(" > "); | ||
| }; |
There was a problem hiding this comment.
Add safeguard against infinite loops in formatLocationParent.
The while loop could potentially enter an infinite loop if there's a circular reference in the location hierarchy.
const formatLocationParent = (location: LocationList) => {
const parents: string[] = [];
+ const visited = new Set<string>();
while (location.parent?.name) {
+ if (visited.has(location.parent.id)) {
+ console.error("Circular reference detected in location hierarchy");
+ break;
+ }
+ visited.add(location.parent.id);
parents.push(location.parent?.name);
location = location.parent;
}
return parents.reverse().join(" > ");
};📝 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 formatLocationParent = (location: LocationList) => { | |
| const parents: string[] = []; | |
| while (location.parent?.name) { | |
| parents.push(location.parent?.name); | |
| location = location.parent; | |
| } | |
| return parents.reverse().join(" > "); | |
| }; | |
| const formatLocationParent = (location: LocationList) => { | |
| const parents: string[] = []; | |
| const visited = new Set<string>(); | |
| while (location.parent?.name) { | |
| if (visited.has(location.parent.id)) { | |
| console.error("Circular reference detected in location hierarchy"); | |
| break; | |
| } | |
| visited.add(location.parent.id); | |
| parents.push(location.parent?.name); | |
| location = location.parent; | |
| } | |
| return parents.reverse().join(" > "); | |
| }; |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Location/LocationSearch.tsx (1)
97-104:⚠️ Potential issueAdd safeguard against infinite loops in formatLocationParent.
The while loop could potentially enter an infinite loop if there's a circular reference in the location hierarchy.
Apply this diff to add circular reference detection:
const formatLocationParent = (location: LocationList) => { const parents: string[] = []; + const visited = new Set<string>(); while (location.parent?.name) { + if (visited.has(location.parent.id)) { + console.error("Circular reference detected in location hierarchy"); + break; + } + visited.add(location.parent.id); parents.push(location.parent?.name); location = location.parent; } return parents.reverse().join(" > "); };
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/LocationForm.tsx (1)
131-134: Improve documentation for mode selection logic.The comment "Mode = instance only for beds" could be more descriptive to explain the business logic behind this decision.
Consider updating the comment to be more descriptive:
- // Mode = instance only for beds + // For bed locations (form='bd'), we use 'instance' mode as each bed needs to be tracked individually. + // For other location types, we use 'kind' mode as they represent categories of spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(27 hunks)src/components/Location/LocationSearch.tsx(5 hunks)src/pages/Facility/settings/locations/LocationForm.tsx(3 hunks)src/pages/Facility/settings/organizations/components/CreateFacilityOrganizationSheet.tsx(1 hunks)src/types/common/contactPoint.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/Facility/settings/organizations/components/CreateFacilityOrganizationSheet.tsx
- src/types/common/contactPoint.ts
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (2)
src/pages/Facility/settings/locations/LocationForm.tsx (2)
31-31:✅ Verification successful
LGTM! Good practice using centralized constants.
Moving from hardcoded enum values to a centralized
LocationFormOptionsconstant improves maintainability and reduces the risk of inconsistencies.Let's verify that all location form options are properly handled:
Also applies to: 43-43
🏁 Script executed:
#!/bin/bash # Description: Verify that all location form options have corresponding translations # Find all location form options echo "Location form options:" rg -l 'LocationFormOptions.*=.*\[.*\]' | xargs cat # Find corresponding translations echo -e "\nTranslations:" rg "location_form__[a-z]{2}" public/locale/en.jsonLength of output: 698
LGTM! Centralized constant usage is confirmed.
The verification script output shows that all location form options are correctly associated with the expected translation keys. Using the centralized
LocationFormOptionsconstant improves maintainability by eliminating hardcoded values, and the corresponding translations inpublic/locale/en.jsonare in place.
211-215:✅ Verification successful
LGTM! Good internationalization practice.
The code properly handles internationalization by using translation keys for location form options. The mapping logic is clean and efficient.
Let's verify the translation key format matches the backend expectations:
🏁 Script executed:
#!/bin/bash # Description: Check if the translation key format matches backend expectations # Search for translation key usage in backend rg -A 2 "location_form__" --glob '!*.{json,tsx,ts}' # Search for any other translation key formats for location forms rg -A 2 "location.*form.*translation" --glob '!*.{json,tsx,ts}'Length of output: 117
Below is a new shell script that searches for the translation key format in JSON files (which are common for backend translations):
Translation Key Verification Pending:
Your code change for internationalization looks good. However, since the initial command produced no output, please verify manually (or re-run the above script) to ensure that the backend translation keys using thelocation_form__${option}format are defined as expected.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-check translation key pattern in translation JSON files (likely storing translations) rg -A 2 "location_form__" --glob "*.{json}"Length of output: 1124
LGTM! Internationalization keys verified.
- The mapping logic in
src/pages/Facility/settings/locations/LocationForm.tsx(lines 211-215) dynamically constructs translation keys (e.g.,location_form__area) that align with the keys defined in the backend (as seen inpublic/locale/en.json).- The re-run of the translation verification script confirms that the backend supports this key format.
| <span>{location.name}</span> | ||
| <span className="text-xs text-gray-500"> | ||
| {t(`location_form__${location.form}`)} | ||
| {" in "} | ||
| {formatLocationParent(location)} | ||
| </span> | ||
| <span className="text-xs text-gray-500 ml-auto"> | ||
| {t(`location_status__${location.status}`)} | ||
| </span> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add error handling for missing translation keys.
The translation keys location_form__${location.form} and location_status__${location.status} might not exist in the translation files. Consider adding fallback values or error handling.
Run this script to check for missing translation keys:
🏁 Script executed:
#!/bin/bash
# Description: Check if all required translation keys exist in the English locale file.
# Get unique location forms and statuses from the codebase
forms=$(rg -o 'location_form__\w+' | sort -u)
statuses=$(rg -o 'location_status__\w+' | sort -u)
# Check if these keys exist in the English locale file
echo "Checking translation keys in locale file..."
for key in $forms $statuses; do
if ! rg -q "\"$key\":" "public/locale/en.json"; then
echo "Missing translation key: $key"
fi
doneLength of output: 5415
Enhance Fallbacks for Missing Translations
The dynamic translation keys (location_form__${location.form} and location_status__${location.status}) currently lack a fallback mechanism. This may lead to unfriendly UI when a translation key is absent. Please implement fallbacks or leverage i18next’s default behavior to ensure a graceful display if the translation is missing. Also, note that the verification script appears to prepend file names to the keys, so double-check that the locale key extraction accurately reflects the keys used in the component.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Deploying care-fe with
|
| Latest commit: |
9e7bd2c
|
| Status: | ⚡️ Build in progress... |
Deploying care-fe with
|
| Latest commit: |
9e7bd2c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://43d6f75d.care-fe.pages.dev |
| Branch Preview URL: | https://rithviknishad-feat-devices.care-fe.pages.dev |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/pages/Facility/settings/devices/components/DeviceForm.tsx (3)
369-446: Consider making contact-point usage configurable.Currently, the
usefield is hardcoded to"work". However, there's feedback in the PR objectives about removing(work)from headings or clarifying it. If you need different contact uses (home, work, mobile, etc.), or if you want to omit literal “work” references, consider making this field user-selectable or removing it if unnecessary.- use: "work", + use: "home", // or remove the field entirely if not required
356-459: Enhance user experience with dynamic placeholders for contact points.The placeholders change based on the contact system, which is excellent. However, consider automatically switching between phone input and text input if the user changes the system type. This approach would avoid confusion if they mistakenly choose "phone" first and then switch to "email" or vice versa.
278-281: Maintain date consistency in placeholders.To align with the overall "dd/mm/yyyy" guidance, consider also placing example placeholders (e.g., "dd/mm/yyyy") in the date fields to reinforce the desired format.
<Input type="date" {...field} - placeholder="" + placeholder="dd/mm/yyyy" />Also applies to: 292-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(27 hunks)src/pages/Facility/settings/devices/DeviceDetail.tsx(1 hunks)src/pages/Facility/settings/devices/components/DeviceForm.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/pages/Facility/settings/devices/components/DeviceForm.tsx (2)
53-56: Ensure follow-through on date format requirement.While the code correctly rejects future manufacture dates, please confirm if a
type="date"input meets the requirement to display the date in "dd/mm/yyyy" format. By default, the HTML date picker typically uses the ISO format (yyyy-mm-dd). If strict "dd/mm/yyyy" formatting is required, consider implementing a custom date picker or using a library to enforce the format.
68-74: Validate optional fields more cautiously.The
.refine()for comparing expiration and manufacture dates works, but ensure that other optional fields (likeidentifierorlot_number) do not trigger unintended validation errors in certain edge cases. It might be worthwhile to confirm that the user cannot accidentally pass empty strings leading to undefined or invalid date parsing.src/pages/Facility/settings/devices/DeviceDetail.tsx (2)
47-75: Verify redirection paths and deletion.When a device is successfully deleted, the code navigates back to
"/facility/${facilityId}/settings/devices". Please confirm that this route is valid. If there's a reported issue of broken redirection to locations/devices, double-check the path or any alternate route definitions.
326-339: Great job enforcing date format.Using
formatDate(device.expiration_date, "dd/MM/yyyy")matches the PR request to show dates in the dd/mm/yyyy format. This is consistent with the requested date format changes.public/locale/en.json (1)
292-297: All new localization keys look consistent.The newly added keys (e.g.,
"add_device","delete_device","device_availability_status_*", and"manufacture_date_cannot_be_in_future") align well with the existing style. The messages are concise and accurately reflect the device management functionality. The only minor grammatical improvement could be changing"manufacture_date_cannot_be_in_future"to “Manufacture date cannot be in the future,” but this is mostly stylistic.Also applies to: 425-429, 452-453, 525-526, 529-530, 709-709, 742-743, 763-767, 768-773, 1010-1011, 1014-1016, 1019-1020, 1025-1025, 1401-1402, 1561-1562, 2080-2080, 2086-2087
| const renderContactInfo = (contact: ContactPoint) => { | ||
| const getContactLink = (system: string, value: string) => { | ||
| switch (system) { | ||
| case "phone": | ||
| case "fax": | ||
| return `tel:${value}`; | ||
| case "email": | ||
| return `mailto:${value}`; | ||
| case "url": | ||
| return value; | ||
| case "sms": | ||
| return `sms:${value}`; | ||
| default: | ||
| return null; | ||
| } | ||
| }; | ||
|
|
||
| const link = getContactLink(contact.system, contact.value); | ||
|
|
||
| return ( | ||
| <div key={`${contact.system}-${contact.value}`} className="space-y-1"> | ||
| <p className="text-sm font-medium text-gray-500">{t(contact.system)}</p> | ||
| {link ? ( | ||
| <a | ||
| href={link} | ||
| className="text-sm text-primary-600 hover:text-primary-700 hover:underline" | ||
| target={contact.system === "url" ? "_blank" : undefined} | ||
| rel={contact.system === "url" ? "noopener noreferrer" : undefined} | ||
| > | ||
| {contact.value} | ||
| </a> | ||
| ) : ( | ||
| <p className="text-sm text-gray-900">{contact.value}</p> | ||
| )} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve "renderContactInfo" resilience.
It’s good to generate different clickable links for phone, email, etc. Consider validating or sanitizing the contact.value for edge cases (e.g., missing "@" in emails or invalid URLs). That can prevent broken links or undesired behavior if the user enters malformed data.
|
Enhancement
|
|
functionally looks good |
|
@rithviknishad 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! 🙌 |
we can consider removing the remove button
no use case as of now, also not present in FHIR spec, we can reconsider this later when needed. |
Required Backends
Proposed Changes
Summary by CodeRabbit
New Features
Localization Enhancements