feature: Added services history section in device details page (10828) #10934
Conversation
WalkthroughThe pull request introduces new localization keys in the English JSON for service record-related UI elements. It adds a new component, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as DeviceDetail
participant DHS as DeviceServiceHistory
participant SHEET as ServiceHistorySheet
participant API as deviceApi
U->>D: Open Device Detail Page
D->>DHS: Render service history component with facilityId & deviceId
DHS->>API: Fetch service history list
API-->>DHS: Return service history data
U->>DHS: Click on add/edit service record
DHS->>SHEET: Open service history sheet/modal
SHEET->>API: Submit create/update request
API-->>SHEET: Confirm mutation
SHEET-->>DHS: Trigger refresh of service history list
Suggested labels
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
src/types/device/device.ts (1)
68-72: Consider making the meta field more type-safe.While using
Record<string, any>provides flexibility, it could lead to type safety issues. Consider defining a more specific structure for the meta field if the structure is known.- meta?: Record<string, any>; + meta?: Record<string, string | number | boolean | null>;src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx (1)
193-196: Add character limit and validation for note field.The note textarea should have a maximum length to prevent excessively long entries.
<Textarea placeholder={t("service_notes_enter")} {...field} rows={5} + maxLength={500} />Also, update the form schema:
const formSchema = z.object({ - note: z.string().min(1, { message: "Notes are required" }), + note: z.string().min(1, { message: "Notes are required" }).max(500, { message: "Notes cannot exceed 500 characters" }), serviced_on: z.date({ required_error: "Service date is required" }), });src/types/device/deviceApi.ts (1)
56-82: Consider using type annotations instead of type assertions for better type safety.The service history API methods use type assertions with empty objects (e.g.,
{} as ServiceHistoryResponse). While this works, it's generally better practice to use type annotations for better type safety.- TRes: {} as ServiceHistoryListResponse, + TRes: Type<ServiceHistoryListResponse>(), - TReq: {}, + TReq: Type<void>(), - TRes: {} as ServiceHistoryResponse, + TRes: Type<ServiceHistoryResponse>(), - TReq: {}, + TReq: Type<void>(), - TRes: {} as ServiceHistoryResponse, + TRes: Type<ServiceHistoryResponse>(), - TReq: {} as ServiceHistoryCreateUpdateRequest, + TReq: Type<ServiceHistoryCreateUpdateRequest>(), - TRes: {} as ServiceHistoryResponse, + TRes: Type<ServiceHistoryResponse>(), - TReq: {} as ServiceHistoryCreateUpdateRequest, + TReq: Type<ServiceHistoryCreateUpdateRequest>(),This matches the pattern used in other API methods in this file and provides more consistent type handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/locale/en.json(2 hunks)src/pages/Facility/settings/devices/DeviceDetail.tsx(2 hunks)src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx(1 hunks)src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx(1 hunks)src/types/device/device.ts(1 hunks)src/types/device/deviceApi.ts(2 hunks)
🔇 Additional comments (9)
src/pages/Facility/settings/devices/DeviceDetail.tsx (2)
41-41: Good addition of the DeviceServiceHistory component import.The import is correctly placed with other component imports.
372-372: Properly integrated the DeviceServiceHistory component.The DeviceServiceHistory component is correctly added to the UI with the required props (facilityId and deviceId).
src/types/device/device.ts (2)
53-59: Type definition for ServiceHistoryResponse looks good.The interface properly defines the structure for a service history record with appropriate fields.
61-66: Well-structured pagination interface for service history.The ServiceHistoryListResponse interface follows the standard pagination pattern with count, next, previous, and results fields.
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx (1)
118-118: Good use of fallback for translation.Providing a fallback text in case the translation key is missing is a good practice.
src/types/device/deviceApi.ts (2)
4-11: Good addition of new service history types to import statement.The import statement has been updated to include the new types needed for the service history API functionality. This is a clean approach to organizing the imports.
85-85: Export statement updated correctly.The export statement has been properly updated to export the deviceApi constant, making the API accessible to other modules.
public/locale/en.json (2)
505-509: Good addition of reusable button labels.These button labels ("Cancel", "Save", "Saving...", etc.) are well-structured and can be reused across the application, promoting consistency in the UI.
2191-2198: Service history labels added correctly.The service history-related labels are descriptive and follow the established naming conventions of the project. They adequately cover all the UI needs for the new service history functionality.
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx (3)
48-51: Localize validation error messages.The form schema contains hardcoded error messages which should be internationalized for consistency with the rest of the application.
const formSchema = z.object({ - note: z.string().min(1, { message: "Notes are required" }), - serviced_on: z.date({ required_error: "Service date is required" }), + note: z.string().min(1, { message: t("error_notes_required") }), + serviced_on: z.date({ required_error: t("error_service_date_required") }), });Don't forget to add these new translation keys to your localization files.
180-185: Add date constraints to the Calendar component.The Calendar component currently allows selection of any date, including future dates. Consider adding constraints appropriate for service records (e.g., preventing future dates).
<Calendar mode="single" selected={field.value} onSelect={field.onChange} initialFocus + disabled={(date) => date > new Date()} + fromDate={new Date(2000, 0, 1)} // Set a reasonable minimum date />
140-140: Extract loading state to a more descriptive variable name.The variable name
isPendingis correct but could be more descriptive about what operation is pending.-const isPending = createMutation.isPending || updateMutation.isPending; +const isSubmitting = createMutation.isPending || updateMutation.isPending; // Then update usage: -<Button type="submit" disabled={isPending}> - {isPending +<Button type="submit" disabled={isSubmitting}> + {isSubmitting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx(1 hunks)src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx
🔇 Additional comments (5)
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx (5)
114-116: Improve error handling with user feedback.Similar to the create mutation, the update mutation should provide user feedback on errors.
onError: (error) => { console.error("Failed to update service record:", error); + // Add user-facing error notification + toast({ + title: t("error_title"), + description: t("error_update_service_record"), + variant: "destructive", + }); },
36-38: LGTM - Good use of type imports and API structure.The component properly imports and uses the correct types from device APIs, making it type-safe and maintainable.
73-85: LGTM - Well-implemented form reset logic.The useEffect hook properly handles form reset logic for both new and existing service records, ensuring the form is in the correct state.
126-138: LGTM - Effective submission handler with metadata preservation.The onSubmit function correctly handles both creation and update cases while preserving metadata.
209-226: LGTM - Well-implemented button states with internationalization.The form buttons correctly handle loading states and use translation keys for text, making the UI responsive and properly localized.
src/pages/Facility/settings/devices/components/ServiceHistorySheet.tsx
Outdated
Show resolved
Hide resolved
public/locale/en.json
Outdated
| "button_cancel": "Cancel", | ||
| "button_save": "Save", | ||
| "button_saving": "Saving...", | ||
| "button_update": "Update", | ||
| "button_updating": "Updating...", |
There was a problem hiding this comment.
we already have i18n for this, reuse them instead of creating new ones
src/types/device/deviceApi.ts
Outdated
| path: "/api/v1/facility/{facility_external_id}/device/{device_external_id}/service_history/", | ||
| method: HttpMethod.GET, | ||
| TRes: {} as ServiceHistoryListResponse, | ||
| TReq: {}, |
src/types/device/deviceApi.ts
Outdated
| path: "/api/v1/facility/{facility_external_id}/device/{device_external_id}/service_history/", | ||
| method: HttpMethod.GET, |
There was a problem hiding this comment.
this is javascript; follow naming conventions;
| path: "/api/v1/facility/{facility_external_id}/device/{device_external_id}/service_history/", | |
| method: HttpMethod.GET, | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/", | |
| method: HttpMethod.GET, |
There was a problem hiding this comment.
but i am passsing also like these and in api documentation these names facility_external_id are there
const { data: serviceHistory, isLoading } = useQuery({
queryKey: ["device", facilityId, deviceId, "serviceHistory"],
queryFn: query(deviceApi.serviceHistory.list, {
pathParams: {
facility_external_id: facilityId,
device_external_id: deviceId,
},
}),
There was a problem hiding this comment.
That is on the backend. For path params, it doesn't matter what you call it so long as url path is accurate.
There was a problem hiding this comment.
i am getting 403 forbidden during calling this api can you help me?
src/types/device/device.ts
Outdated
| export interface ServiceHistoryCreateUpdateRequest { | ||
| serviced_on: string; | ||
| note: string; | ||
| meta?: Record<string, any>; | ||
| } |
There was a problem hiding this comment.
| export interface ServiceHistoryCreateUpdateRequest { | |
| serviced_on: string; | |
| note: string; | |
| meta?: Record<string, any>; | |
| } | |
| export interface ServiceHistoryWriteRequest { | |
| serviced_on: string; | |
| note: string; | |
| meta?: Record<string, any>; | |
| } |
| </Table> | ||
| ) : ( | ||
| <div className="text-center py-6 text-muted-foreground"> | ||
| {t("service_records_none", "No service history available")} |
There was a problem hiding this comment.
why pass a second argument? 🤔
| {t("service_records_none", "No service history available")} | |
| {t("service_records_none")} |
There was a problem hiding this comment.
Same here. Make the changes suggested.
There was a problem hiding this comment.
I was actually working on it and made some changes locally. I thought I would push the changes first, then request a review
| <TableRow> | ||
| <TableHead>{t("service_date")}</TableHead> | ||
| <TableHead>{t("service_notes")}</TableHead> | ||
| <TableHead className="text-right">{t("actions")}</TableHead> | ||
| </TableRow> |
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx
Outdated
Show resolved
Hide resolved
src/types/device/deviceApi.ts
Outdated
| path: "/api/v1/facility/{facility_external_id}/device/{device_external_id}/service_history/", | ||
| method: HttpMethod.GET, |
There was a problem hiding this comment.
That is on the backend. For path params, it doesn't matter what you call it so long as url path is accurate.
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx
Outdated
Show resolved
Hide resolved
| </Table> | ||
| ) : ( | ||
| <div className="text-center py-6 text-muted-foreground"> | ||
| {t("service_records_none", "No service history available")} |
There was a problem hiding this comment.
Same here. Make the changes suggested.
|
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: 5
🧹 Nitpick comments (3)
src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx (1)
47-60: Query params should be included in all related invalidation calls.The queryParams are included in the fetch query key but not consistently in invalidation calls. This could lead to stale data remaining in the cache.
// In handleServiceSheetClose() method: queryClient.invalidateQueries({ - queryKey: ["deviceserviceHistory", facilityId, deviceId, qParams], + queryKey: ["deviceserviceHistory", facilityId, deviceId], });This should match the query key in line 49 exactly to properly invalidate the cache.
src/pages/Facility/settings/devices/components/ServiceHistoryForm.tsx (2)
86-87: Implement proper error handling instead of console.log.Using console.error is not appropriate for production code. Implement proper error handling.
- onError: (error) => { - console.error("Failed to create service record:", error); - }, + onError: (error) => { + // Use a toast notification or other UI feedback mechanism + toast({ + title: "Error", + description: "Failed to create service record. Please try again.", + variant: "destructive", + }); + },Apply similar changes to the updateMutation error handler as well.
Also applies to: 106-107
180-186: Add maxLength attribute to the Textarea.The textarea for service notes should have a maxLength attribute to limit input length.
<Textarea placeholder={t("service_notes_enter")} {...field} rows={5} + maxLength={1000} />This helps prevent excessive data submission and provides clear user feedback on input limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/locale/en.json(1 hunks)src/pages/Facility/settings/devices/DeviceDetail.tsx(2 hunks)src/pages/Facility/settings/devices/components/DeviceServiceHistory.tsx(1 hunks)src/pages/Facility/settings/devices/components/ServiceHistoryForm.tsx(1 hunks)src/types/device/device.ts(1 hunks)src/types/device/deviceApi.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Facility/settings/devices/DeviceDetail.tsx
- src/types/device/device.ts
🔇 Additional comments (1)
public/locale/en.json (1)
2195-2202: Well-structured service history localization keys.The added localization keys follow a consistent naming convention and provide all the necessary text for the service history feature.
| <div className="flex w-full items-center justify-center"> | ||
| <div | ||
| className={cn( | ||
| "flex w-full justify-center", | ||
| (serviceHistory?.count ?? 0) > RESULTS_PER_PAGE_LIMIT | ||
| ? "visible" | ||
| : "invisible", | ||
| )} | ||
| > | ||
| <PaginationComponent | ||
| cPage={qParams.page ?? 1} | ||
| defaultPerPage={RESULTS_PER_PAGE_LIMIT} | ||
| data={{ totalCount: serviceHistory?.count ?? 0 }} | ||
| onChange={(page) => setQueryParams({ page })} | ||
| /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Table pagination should be outside of the table body.
The pagination component is currently placed inside the TableBody, which is not semantically correct and could cause rendering issues.
</TableBody>
- <div className="flex w-full items-center justify-center">
- <div
- className={cn(
- "flex w-full justify-center",
- (serviceHistory?.count ?? 0) > RESULTS_PER_PAGE_LIMIT
- ? "visible"
- : "invisible",
- )}
- >
- <PaginationComponent
- cPage={qParams.page ?? 1}
- defaultPerPage={RESULTS_PER_PAGE_LIMIT}
- data={{ totalCount: serviceHistory?.count ?? 0 }}
- onChange={(page) => setQueryParams({ page })}
- />
- </div>
- </div>
</Table>
+ <div className="flex w-full items-center justify-center mt-4">
+ <div
+ className={cn(
+ "flex w-full justify-center",
+ (serviceHistory?.count ?? 0) > RESULTS_PER_PAGE_LIMIT
+ ? "visible"
+ : "invisible",
+ )}
+ >
+ <PaginationComponent
+ cPage={qParams.page ?? 1}
+ defaultPerPage={RESULTS_PER_PAGE_LIMIT}
+ data={{ totalCount: serviceHistory?.count ?? 0 }}
+ onChange={(page) => setQueryParams({ page })}
+ />
+ </div>
+ </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 className="flex w-full items-center justify-center"> | |
| <div | |
| className={cn( | |
| "flex w-full justify-center", | |
| (serviceHistory?.count ?? 0) > RESULTS_PER_PAGE_LIMIT | |
| ? "visible" | |
| : "invisible", | |
| )} | |
| > | |
| <PaginationComponent | |
| cPage={qParams.page ?? 1} | |
| defaultPerPage={RESULTS_PER_PAGE_LIMIT} | |
| data={{ totalCount: serviceHistory?.count ?? 0 }} | |
| onChange={(page) => setQueryParams({ page })} | |
| /> | |
| </div> | |
| </div> | |
| </TableBody> | |
| </Table> | |
| <div className="flex w-full items-center justify-center mt-4"> | |
| <div | |
| className={cn( | |
| "flex w-full justify-center", | |
| (serviceHistory?.count ?? 0) > RESULTS_PER_PAGE_LIMIT | |
| ? "visible" | |
| : "invisible", | |
| )} | |
| > | |
| <PaginationComponent | |
| cPage={qParams.page ?? 1} | |
| defaultPerPage={RESULTS_PER_PAGE_LIMIT} | |
| data={{ totalCount: serviceHistory?.count ?? 0 }} | |
| onChange={(page) => setQueryParams({ page })} | |
| /> | |
| </div> | |
| </div> |
| serviceHistory: { | ||
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/", | ||
| method: HttpMethod.GET, | ||
| TRes: Type<PaginatedResponse<ServiceHistory>>(), | ||
| }, | ||
| retrieveserviceHistory: { | ||
| method: HttpMethod.GET, | ||
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/", | ||
| TRes: Type<ServiceHistory>(), | ||
| }, | ||
| createserviceHistory: { | ||
| method: HttpMethod.POST, | ||
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/", | ||
| TRes: Type<ServiceHistory>(), | ||
| TBody: Type<ServiceHistoryWriteRequest>(), | ||
| }, | ||
| updateserviceHistory: { | ||
| method: HttpMethod.PUT, | ||
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/", | ||
| TRes: Type<ServiceHistory>(), | ||
| TBody: Type<ServiceHistoryWriteRequest>(), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent naming in service history API methods.
The method names for service history should follow consistent camelCase convention with appropriate word separation.
- serviceHistory: {
+ serviceHistory: {
path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/",
method: HttpMethod.GET,
TRes: Type<PaginatedResponse<ServiceHistory>>(),
},
- retrieveserviceHistory: {
+ retrieveServiceHistory: {
method: HttpMethod.GET,
path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/",
TRes: Type<ServiceHistory>(),
},
- createserviceHistory: {
+ createServiceHistory: {
method: HttpMethod.POST,
path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/",
TRes: Type<ServiceHistory>(),
TBody: Type<ServiceHistoryWriteRequest>(),
},
- updateserviceHistory: {
+ updateServiceHistory: {
method: HttpMethod.PUT,
path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/",
TRes: Type<ServiceHistory>(),
TBody: Type<ServiceHistoryWriteRequest>(),
},Make sure to update all references to these methods in other files (ServiceHistoryForm.tsx) as well.
📝 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.
| serviceHistory: { | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/", | |
| method: HttpMethod.GET, | |
| TRes: Type<PaginatedResponse<ServiceHistory>>(), | |
| }, | |
| retrieveserviceHistory: { | |
| method: HttpMethod.GET, | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/", | |
| TRes: Type<ServiceHistory>(), | |
| }, | |
| createserviceHistory: { | |
| method: HttpMethod.POST, | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/", | |
| TRes: Type<ServiceHistory>(), | |
| TBody: Type<ServiceHistoryWriteRequest>(), | |
| }, | |
| updateserviceHistory: { | |
| method: HttpMethod.PUT, | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/", | |
| TRes: Type<ServiceHistory>(), | |
| TBody: Type<ServiceHistoryWriteRequest>(), | |
| }, | |
| serviceHistory: { | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/", | |
| method: HttpMethod.GET, | |
| TRes: Type<PaginatedResponse<ServiceHistory>>(), | |
| }, | |
| retrieveServiceHistory: { | |
| method: HttpMethod.GET, | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/", | |
| TRes: Type<ServiceHistory>(), | |
| }, | |
| createServiceHistory: { | |
| method: HttpMethod.POST, | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/", | |
| TRes: Type<ServiceHistory>(), | |
| TBody: Type<ServiceHistoryWriteRequest>(), | |
| }, | |
| updateServiceHistory: { | |
| method: HttpMethod.PUT, | |
| path: "/api/v1/facility/{facilityId}/device/{deviceId}/service_history/{id}/", | |
| TRes: Type<ServiceHistory>(), | |
| TBody: Type<ServiceHistoryWriteRequest>(), | |
| }, |
| mutationFn: mutate(deviceApi.createserviceHistory, { | ||
| pathParams: { | ||
| facilityId, | ||
| deviceId, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
API method name needs to be updated to maintain consistency.
The service history API method names need to be updated to match the revised names in deviceApi.ts.
- mutationFn: mutate(deviceApi.createserviceHistory, {
+ mutationFn: mutate(deviceApi.createServiceHistory, {
pathParams: {
facilityId,
deviceId,
},
}),📝 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.
| mutationFn: mutate(deviceApi.createserviceHistory, { | |
| pathParams: { | |
| facilityId, | |
| deviceId, | |
| }, | |
| }), | |
| mutationFn: mutate(deviceApi.createServiceHistory, { | |
| pathParams: { | |
| facilityId, | |
| deviceId, | |
| }, | |
| }), |
| const createMutation = useMutation({ | ||
| mutationFn: mutate(deviceApi.createserviceHistory, { | ||
| pathParams: { | ||
| facilityId, | ||
| deviceId, | ||
| }, | ||
| }), | ||
| onError: (error) => { | ||
| console.error("Failed to create service record:", error); | ||
| }, | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: ["deviceserviceHistory", facilityId, deviceId], | ||
| }); | ||
| onOpenChange(false); | ||
| form.reset(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Query key inconsistency in cache invalidation.
The query key used for cache invalidation doesn't match the one used for fetching in DeviceServiceHistory component.
onSuccess: () => {
queryClient.invalidateQueries({
- queryKey: ["deviceserviceHistory", facilityId, deviceId],
+ queryKey: ["deviceserviceHistory", facilityId, deviceId, qParams],
});
onOpenChange(false);
form.reset();
},You need to align this with how the query is defined in DeviceServiceHistory.tsx and also add the qParams (which may require passing them as a prop to this component).
📝 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 createMutation = useMutation({ | |
| mutationFn: mutate(deviceApi.createserviceHistory, { | |
| pathParams: { | |
| facilityId, | |
| deviceId, | |
| }, | |
| }), | |
| onError: (error) => { | |
| console.error("Failed to create service record:", error); | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ | |
| queryKey: ["deviceserviceHistory", facilityId, deviceId], | |
| }); | |
| onOpenChange(false); | |
| form.reset(); | |
| }, | |
| }); | |
| const createMutation = useMutation({ | |
| mutationFn: mutate(deviceApi.createserviceHistory, { | |
| pathParams: { | |
| facilityId, | |
| deviceId, | |
| }, | |
| }), | |
| onError: (error) => { | |
| console.error("Failed to create service record:", error); | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ | |
| queryKey: ["deviceserviceHistory", facilityId, deviceId, qParams], | |
| }); | |
| onOpenChange(false); | |
| form.reset(); | |
| }, | |
| }); |
| const updateMutation = useMutation({ | ||
| mutationFn: mutate(deviceApi.updateserviceHistory, { | ||
| pathParams: { | ||
| facilityId, | ||
| deviceId, | ||
| id: serviceRecord?.id, | ||
| }, | ||
| }), | ||
| onError: (error) => { | ||
| console.error("Failed to update service record:", error); | ||
| }, | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: ["deviceserviceHistory", facilityId, deviceId], | ||
| }); | ||
| onOpenChange(false); | ||
| form.reset(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update the API method name and add query params to invalidation.
Similar to the previous comment, update the method name and query params here.
const updateMutation = useMutation({
- mutationFn: mutate(deviceApi.updateserviceHistory, {
+ mutationFn: mutate(deviceApi.updateServiceHistory, {
pathParams: {
facilityId,
deviceId,
id: serviceRecord?.id,
},
}),
onError: (error) => {
console.error("Failed to update service record:", error);
},
onSuccess: () => {
queryClient.invalidateQueries({
- queryKey: ["deviceserviceHistory", facilityId, deviceId],
+ queryKey: ["deviceserviceHistory", facilityId, deviceId, qParams],
});
onOpenChange(false);
form.reset();
},
});📝 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 updateMutation = useMutation({ | |
| mutationFn: mutate(deviceApi.updateserviceHistory, { | |
| pathParams: { | |
| facilityId, | |
| deviceId, | |
| id: serviceRecord?.id, | |
| }, | |
| }), | |
| onError: (error) => { | |
| console.error("Failed to update service record:", error); | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ | |
| queryKey: ["deviceserviceHistory", facilityId, deviceId], | |
| }); | |
| onOpenChange(false); | |
| form.reset(); | |
| }, | |
| }); | |
| const updateMutation = useMutation({ | |
| mutationFn: mutate(deviceApi.updateServiceHistory, { | |
| pathParams: { | |
| facilityId, | |
| deviceId, | |
| id: serviceRecord?.id, | |
| }, | |
| }), | |
| onError: (error) => { | |
| console.error("Failed to update service record:", error); | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ | |
| queryKey: ["deviceserviceHistory", facilityId, deviceId, qParams], | |
| }); | |
| onOpenChange(false); | |
| form.reset(); | |
| }, | |
| }); |
Proposed Changes
Screen.Recording.2025-03-01.222211.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
DeviceServiceHistory,ServiceHistorySheet, andServiceHistoryForm, enhancing user interaction with service records.