[ENG-581] Optimize complete service request logic by remove duplication of mutation#16462
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Walkthrough
ChangesCompletionNoteContent self-containment refactor
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Deploying care-preview with
|
| Latest commit: |
b3add71
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d29b9a50.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-581-optimize-complete-se.care-preview-a7w.pages.dev |
Greptile SummaryThis PR consolidates the
Confidence Score: 4/5Safe to merge with low risk; the mutation itself still completes correctly, but a UX regression was introduced around dialog dismissal during submission. The consolidation is sound and the mutation logic is unchanged. The one regression is that the backdrop-click / Escape-key lock that prevented users from accidentally closing the dialog mid-submission is no longer in effect, since src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx — specifically the Important Files Changed
Reviews (1): Last reviewed commit: "ENG-581 Optimize complete service reques..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR refactors the “mark as complete” flow in the Service Request details page by consolidating the “complete service request” mutation into CompletionNoteContent, removing the previously duplicated parent-level mutation/state.
Changes:
- Removed parent-managed completion note state and completion mutation from
ServiceRequestShow. - Moved completion mutation and note state into
CompletionNoteContent, passingfacilityId,serviceRequestId, andserviceRequestas inputs. - Simplified Dialog/Sheet open/close handling by wiring
onOpenChangedirectly tosetIsCompleteDialogOpen.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx (3)
707-745: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocument this medical data update path.
This mutation completes a service request and persists the completion note/location IDs; add a short data-flow and validation-boundary note next to the mutation/payload.
As per coding guidelines,
src/pages/**/*.{ts,tsx}page components must document medical data flows and validation requirements.Suggested documentation
+ /** + * Medical data flow: + * - completes the service request via `serviceRequestApi.updateServiceRequest` + * - persists a trimmed completion note, or `null` when empty + * - preserves current service-request location IDs from the loaded request + * + * Validation boundary: this path relies on the update API/backend to validate + * allowed status transitions and location membership. + */ const { mutate: completeServiceRequest, isPending: isCompletingServiceRequest, } = useMutation({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around lines 707 - 745, Add documentation above the useMutation hook for completeServiceRequest that explains the medical data flow and validation boundaries. Include a comment that describes what medical data is being persisted (the completion note and location IDs), the validation that occurs (such as the note being trimmed), and the data-flow path from the form input through the mutation to the backend update. This follows the coding guideline that page components in src/pages/**/*.{ts,tsx} must document medical data flows and validation requirements.Source: Coding guidelines
688-762: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract
CompletionNoteContentinto its own component file.Now that this component owns state, mutation, and side effects, move it to
components/CompletionNoteContent.tsxwith a default export. While extracting, prefer a singleonClosecallback instead of passing bothonCancelandsetIsCompleteDialogOpen.As per coding guidelines, TSX React components should prefer one component per file, default exports, and
ComponentName.tsxnaming.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around lines 688 - 762, Extract the CompletionNoteContent component from ServiceRequestShow.tsx into a new file at components/CompletionNoteContent.tsx as a default export. Update the CompletionNoteContentProps interface to replace the two separate callbacks onCancel and setIsCompleteDialogOpen with a single onClose callback. Update all usages of these two callbacks within the component to call onClose instead. This consolidation simplifies the component's interface and follows the one-component-per-file convention with default exports as per the coding guidelines.Source: Coding guidelines
640-663:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock Sheet/Dialog closure while completion is pending.
onOpenChange={setIsCompleteDialogOpen}lets outside-click/Escape/close events dismiss the completion surface while the child-owned mutation is still in flight. Since the pending state now lives inCompletionNoteContent, propagate that state upward or move the open wrapper into the child so the close guard still applies.Suggested direction
const [isCompleteDialogOpen, setIsCompleteDialogOpen] = useState(false); + const [isCompletingServiceRequest, setIsCompletingServiceRequest] = + useState(false); ... <Sheet open={isCompleteDialogOpen} - onOpenChange={setIsCompleteDialogOpen} + onOpenChange={(open) => { + if (!isCompletingServiceRequest) setIsCompleteDialogOpen(open); + }} > ... <CompletionNoteContent facilityId={facilityId} serviceRequestId={serviceRequestId} setIsCompleteDialogOpen={setIsCompleteDialogOpen} + onPendingChange={setIsCompletingServiceRequest} onCancel={() => setIsCompleteDialogOpen(false)} serviceRequest={request} /> ... <Dialog open={isCompleteDialogOpen} - onOpenChange={setIsCompleteDialogOpen} + onOpenChange={(open) => { + if (!isCompletingServiceRequest) setIsCompleteDialogOpen(open); + }} >interface CompletionNoteContentProps { facilityId: string; serviceRequestId: string; onCancel: () => void; setIsCompleteDialogOpen: (open: boolean) => void; + onPendingChange: (isPending: boolean) => void; serviceRequest: ServiceRequestReadSpec; } const CompletionNoteContent = ({ facilityId, serviceRequestId, onCancel, setIsCompleteDialogOpen, + onPendingChange, serviceRequest, }: CompletionNoteContentProps) => { ... } = useMutation({ mutationFn: mutate(serviceRequestApi.updateServiceRequest, { pathParams: { facilityId, serviceRequestId }, }), + onMutate: () => { + onPendingChange(true); + }, onSuccess: () => { toast.success(t("service_request_completed")); setIsCompleteDialogOpen(false); queryClient.invalidateQueries({ queryKey: ["serviceRequest", facilityId, serviceRequestId], }); }, + onSettled: () => { + onPendingChange(false); + }, });Also applies to: 707-721
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around lines 640 - 663, The Sheet and Dialog components allow external close events (outside-click, Escape key, close button) to dismiss the completion surface via onOpenChange={setIsCompleteDialogOpen}, even while the completion mutation is pending inside CompletionNoteContent. To fix this, retrieve the pending state from the CompletionNoteContent child component (where the mutation lives) and propagate it upward to the parent. Then modify the onOpenChange handler to conditionally prevent the dialog from closing by wrapping setIsCompleteDialogOpen with logic that checks the pending state - if the mutation is in flight, ignore the close request. This same fix should be applied to both the Sheet component (around line 640) and the Dialog component (around line 707).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx`:
- Around line 707-745: Add documentation above the useMutation hook for
completeServiceRequest that explains the medical data flow and validation
boundaries. Include a comment that describes what medical data is being
persisted (the completion note and location IDs), the validation that occurs
(such as the note being trimmed), and the data-flow path from the form input
through the mutation to the backend update. This follows the coding guideline
that page components in src/pages/**/*.{ts,tsx} must document medical data flows
and validation requirements.
- Around line 688-762: Extract the CompletionNoteContent component from
ServiceRequestShow.tsx into a new file at components/CompletionNoteContent.tsx
as a default export. Update the CompletionNoteContentProps interface to replace
the two separate callbacks onCancel and setIsCompleteDialogOpen with a single
onClose callback. Update all usages of these two callbacks within the component
to call onClose instead. This consolidation simplifies the component's interface
and follows the one-component-per-file convention with default exports as per
the coding guidelines.
- Around line 640-663: The Sheet and Dialog components allow external close
events (outside-click, Escape key, close button) to dismiss the completion
surface via onOpenChange={setIsCompleteDialogOpen}, even while the completion
mutation is pending inside CompletionNoteContent. To fix this, retrieve the
pending state from the CompletionNoteContent child component (where the mutation
lives) and propagate it upward to the parent. Then modify the onOpenChange
handler to conditionally prevent the dialog from closing by wrapping
setIsCompleteDialogOpen with logic that checks the pending state - if the
mutation is in flight, ignore the close request. This same fix should be applied
to both the Sheet component (around line 640) and the Dialog component (around
line 707).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41c2daaa-2382-4c8b-ac66-90fe0a05a767
📒 Files selected for processing (1)
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #9592 |
nihal467
left a comment
There was a problem hiding this comment.
resolve all the AI review comments
The "complete service request" API was being defined in two places once in ServiceRequestShow and again inside CompletionNoteContent but only the child's copy was actually used. Removed the duplicate mutation from the parent and kept a single definition inside CompletionNoteContent
ENG-581
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit