Show mark as complete as default for surgical procedure and counselling service requests#15837
Open
NikhilA8606 wants to merge 9 commits intodevelopfrom
Open
Show mark as complete as default for surgical procedure and counselling service requests#15837NikhilA8606 wants to merge 9 commits intodevelopfrom
NikhilA8606 wants to merge 9 commits intodevelopfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReworks the service-request completion flow: eligibility checks, mutation switched to Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modifies the service request detail page to show the "mark as complete" button by default for surgical procedure and counselling service requests, without requiring a finalized diagnostic report. Previously, this button was only shown when a diagnostic report had final status.
Changes:
- Refactored button visibility logic to show "mark as complete" for surgical_procedure and counselling categories regardless of diagnostic report status
- Extracted
isFinalstatus check into a constant for improved code readability - Restructured conditional rendering to separate "mark as complete" and "view report" button visibility
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx (2)
152-162:⚠️ Potential issue | 🟡 MinorAdd error handling for
completeServiceRequestmutation.The mutation has an
onSuccesshandler but noonErrorhandler. If completion fails, users won't receive feedback.🛡️ Proposed fix
const { mutate: completeServiceRequest } = useMutation({ mutationFn: mutate(serviceRequestApi.completeServiceRequest, { pathParams: { facilityId, serviceRequestId }, }), onSuccess: () => { toast.success(t("service_request_completed")); queryClient.invalidateQueries({ queryKey: ["serviceRequest", facilityId, serviceRequestId], }); }, + onError: () => { + toast.error(t("service_request_complete_error")); + }, });Note: Ensure the i18n key
service_request_complete_erroris added to the translation files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around lines 152 - 162, Add an onError handler to the useMutation for completeServiceRequest (the mutation created with mutate(serviceRequestApi.completeServiceRequest, { pathParams: { facilityId, serviceRequestId } })) so failures surface to users: call toast.error(t("service_request_complete_error")) and include the error message (or error object) in a process/log call; optionally keep or adjust queryClient.invalidateQueries behavior to run only on success. Also ensure the i18n key service_request_complete_error exists in translation files.
335-382: 🧹 Nitpick | 🔵 TrivialRemove unnecessary React fragment.
The fragment
<>...</>wrapping lines 335-382 is redundant since the two children (AlertDialog and conditional Button) can be rendered directly as siblings within the parent<div>.♻️ Proposed simplification
<div className="flex items-center gap-2"> - <> - {!disableEdit && ( - <AlertDialog> + {!disableEdit && ( + <AlertDialog> ... - </AlertDialog> - )} - {isFinal && ( - <Button - ... - </Button> - )} - </> + </AlertDialog> + )} + {isFinal && ( + <Button + ... + </Button> + )} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around lines 335 - 382, The surrounding React fragment <>...</> around the AlertDialog/conditional Button block is unnecessary; remove the fragment so the AlertDialog (with AlertDialogTrigger, AlertDialogContent, AlertDialogAction that calls completeServiceRequest) and the conditional Button (guarded by isFinal, using navigate and ShortcutBadge) are rendered directly as siblings inside the parent container, keeping all existing props, handlers and translations intact (preserve disableEdit check, isFinal check, onClick navigate to diagnostic_reports[0].id, and ShortcutBadge usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx`:
- Around line 152-162: Add an onError handler to the useMutation for
completeServiceRequest (the mutation created with
mutate(serviceRequestApi.completeServiceRequest, { pathParams: { facilityId,
serviceRequestId } })) so failures surface to users: call
toast.error(t("service_request_complete_error")) and include the error message
(or error object) in a process/log call; optionally keep or adjust
queryClient.invalidateQueries behavior to run only on success. Also ensure the
i18n key service_request_complete_error exists in translation files.
- Around line 335-382: The surrounding React fragment <>...</> around the
AlertDialog/conditional Button block is unnecessary; remove the fragment so the
AlertDialog (with AlertDialogTrigger, AlertDialogContent, AlertDialogAction that
calls completeServiceRequest) and the conditional Button (guarded by isFinal,
using navigate and ShortcutBadge) are rendered directly as siblings inside the
parent container, keeping all existing props, handlers and translations intact
(preserve disableEdit check, isFinal check, onClick navigate to
diagnostic_reports[0].id, and ShortcutBadge usage).
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #8610 |
NikhilA8606
changed the title
ohc-project-manager
Bot
added
Work in Progress
and removed
needs testing
needs review
labels
Deploying care-preview with
|
| Latest commit: |
1d86450
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d2e0bf45.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://feat-hide-mark-as-complete.care-preview-a7w.pages.dev |
ohc-project-manager
Bot
added
needs testing
needs review
and removed
Work in Progress
labels
Contributor
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 (1)
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx (1)
334-388: 🧹 Nitpick | 🔵 TrivialEmpty container may render for completed surgical/counselling requests.
When
disableEditistrue(request already completed) andisFinalisfalsefor surgical_procedure/counselling categories, the outer condition passes but neither inner button renders, leaving an empty<div className="flex items-center gap-2">.Consider adding an inner guard to avoid rendering the empty container:
♻️ Suggested improvement
- {(!request?.activity_definition?.diagnostic_report_codes || - isFinal || - CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE.includes( - request.category, - )) && ( + {(!request?.activity_definition?.diagnostic_report_codes || + isFinal || + CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE.includes( + request.category, + )) && + (!disableEdit || isFinal) && ( <div className="flex items-center gap-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around lines 334 - 388, The outer container <div className="flex items-center gap-2"> can render empty when disableEdit is true and isFinal is false for categories in CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE; change the render guard so the container is only rendered when at least one inner action will appear (e.g., require (!disableEdit || isFinal) in addition to the existing condition that checks request.activity_definition, isFinal, and CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE) so that when both buttons would be hidden the container is not output; update the conditional around the container (or add an inner guard) referencing disableEdit, isFinal, and request.category/CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE to prevent the empty div.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx`:
- Around line 334-388: The outer container <div className="flex items-center
gap-2"> can render empty when disableEdit is true and isFinal is false for
categories in CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE; change the render guard
so the container is only rendered when at least one inner action will appear
(e.g., require (!disableEdit || isFinal) in addition to the existing condition
that checks request.activity_definition, isFinal, and
CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE) so that when both buttons would be
hidden the container is not output; update the conditional around the container
(or add an inner guard) referencing disableEdit, isFinal, and
request.category/CLASSIFICATIONS_CAN_BE_MARKED_AS_COMPLETE to prevent the empty
div.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx (1)
354-370:⚠️ Potential issue | 🟠 MajorKeep
view_reportindependent from the completion CTA.This button is now gated by
canShowCompleteCta, which includes!disableEdit. That means a completed or otherwise non-editable request loses theview_reportaction even when a final diagnostic report exists.♻️ Proposed fix
- {canShowCompleteCta && ( - <div className="flex items-center gap-2"> - <> - {isFinal && ( - <Button - variant="primary" - className="font-semibold" - onClick={() => - navigate( - `/facility/${facilityId}/patient/${request.encounter.patient.id}/diagnostic_reports/${request.diagnostic_reports[0].id}`, - ) - } - > - {t("view_report")} - <ShortcutBadge actionId="view-report" /> - </Button> - )} - </> - </div> - )} + {isFinal && ( + <div className="flex items-center gap-2"> + <Button + variant="primary" + className="font-semibold" + onClick={() => + navigate( + `/facility/${facilityId}/patient/${request.encounter.patient.id}/diagnostic_reports/${request.diagnostic_reports[0].id}`, + ) + } + > + {t("view_report")} + <ShortcutBadge actionId="view-report" /> + </Button> + </div> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around lines 354 - 370, The view_report Button is currently wrapped by the canShowCompleteCta block (which depends on !disableEdit) so users lose the View Report action when editing is disabled; update rendering so the view_report Button (checking isFinal and request.diagnostic_reports?.length > 0) is rendered independently of canShowCompleteCta: move the Button that uses navigate(`/facility/${facilityId}/patient/${request.encounter.patient.id}/diagnostic_reports/${request.diagnostic_reports[0].id}`) and the ShortcutBadge actionId="view-report" out of the canShowCompleteCta conditional (or duplicate the isFinal check outside it) so it displays whenever a final diagnostic report exists regardless of disableEdit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx`:
- Around line 646-657: Add stable data-cy attributes for E2E tests to the new
completion flow: add data-cy="sr-mark-complete-btn" to the primary Button that
calls setCompletionNote/setIsCompleteDialogOpen (the Button with onClick and
disabled={isCompletingServiceRequest}), add data-cy="sr-completion-textarea" to
the textarea/input bound to completion note (the element using
setCompletionNote/request.note), and add data-cy="sr-complete-confirm" and
data-cy="sr-complete-cancel" to the dialog/sheet confirm and cancel action
Buttons (the buttons that trigger the completion submit and dialog close). Use
those exact attribute names so tests are stable across the sheet/dialog split.
- Around line 331-334: The guard computing canShowCompleteCta incorrectly treats
an empty diagnostic_report_codes array as present; change the condition in
canShowCompleteCta to check for absence or emptiness of
request?.activity_definition?.diagnostic_report_codes (e.g., use length === 0 or
a falsy-or-empty check) so that empty arrays are treated like undefined and the
completion CTA is shown when disableEdit is false and there are no report codes;
update the expression that references diagnostic_report_codes inside the const
canShowCompleteCta accordingly.
---
Outside diff comments:
In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx`:
- Around line 354-370: The view_report Button is currently wrapped by the
canShowCompleteCta block (which depends on !disableEdit) so users lose the View
Report action when editing is disabled; update rendering so the view_report
Button (checking isFinal and request.diagnostic_reports?.length > 0) is rendered
independently of canShowCompleteCta: move the Button that uses
navigate(`/facility/${facilityId}/patient/${request.encounter.patient.id}/diagnostic_reports/${request.diagnostic_reports[0].id}`)
and the ShortcutBadge actionId="view-report" out of the canShowCompleteCta
conditional (or duplicate the isFinal check outside it) so it displays whenever
a final diagnostic report exists regardless of disableEdit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9284ea13-abc0-40e1-9650-ecd11f9e8e45
📒 Files selected for processing (1)
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx
Comment on lines
+646
to
+657
| <Button | ||
| variant="primary" | ||
| className="font-semibold shrink-0" | ||
| onClick={() => { | ||
| setCompletionNote(request.note ?? ""); | ||
| setIsCompleteDialogOpen(true); | ||
| }} | ||
| disabled={isCompletingServiceRequest} | ||
| > | ||
| {t("mark_as_complete")} | ||
| <ShortcutBadge actionId="mark-as-complete" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add stable data-cy hooks to the new completion flow.
This is a new primary workflow, but the trigger, textarea, and confirm/cancel actions do not expose stable selectors. That will make the required E2E coverage brittle, especially across the sheet/dialog split.
As per coding guidelines, "Add appropriate data-cy attributes for E2E tests to page components".
Also applies to: 675-686, 703-714, 747-759
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around
lines 646 - 657, Add stable data-cy attributes for E2E tests to the new
completion flow: add data-cy="sr-mark-complete-btn" to the primary Button that
calls setCompletionNote/setIsCompleteDialogOpen (the Button with onClick and
disabled={isCompletingServiceRequest}), add data-cy="sr-completion-textarea" to
the textarea/input bound to completion note (the element using
setCompletionNote/request.note), and add data-cy="sr-complete-confirm" and
data-cy="sr-complete-cancel" to the dialog/sheet confirm and cancel action
Buttons (the buttons that trigger the completion submit and dialog close). Use
those exact attribute names so tests are stable across the sheet/dialog split.
amjithtitus09
requested changes
Apr 17, 2026
Comment on lines
353
to
360
| <div className="flex items-end gap-2"> | ||
| {(!request?.activity_definition?.diagnostic_report_codes || | ||
| request?.diagnostic_reports?.[0]?.status === | ||
| DiagnosticReportStatus.final) && ( | ||
| {canShowCompleteCta && ( | ||
| <div className="flex items-center gap-2"> | ||
| {request?.diagnostic_reports?.[0]?.status === | ||
| DiagnosticReportStatus.final && ( | ||
| <> | ||
| {!disableEdit && ( | ||
| <AlertDialog> | ||
| <AlertDialogTrigger asChild> | ||
| <Button | ||
| variant="outline" | ||
| className="font-semibold border border-gray-400" | ||
| > | ||
| {t("mark_as_complete")} | ||
| <ShortcutBadge actionId="mark-as-complete" /> | ||
| </Button> | ||
| </AlertDialogTrigger> | ||
| <AlertDialogContent> | ||
| <AlertDialogHeader> | ||
| <AlertDialogTitle> | ||
| {t("confirm_completion")} | ||
| </AlertDialogTitle> | ||
| <AlertDialogDescription> | ||
| {t("service_request_completion_confirmation")} | ||
| </AlertDialogDescription> | ||
| </AlertDialogHeader> | ||
| <AlertDialogFooter> | ||
| <AlertDialogCancel> | ||
| {t("cancel")} | ||
| </AlertDialogCancel> | ||
| <AlertDialogAction | ||
| onClick={() => completeServiceRequest({})} | ||
| > | ||
| {t("confirm")} | ||
| <ShortcutBadge actionId="enter-action" /> | ||
| </AlertDialogAction> | ||
| </AlertDialogFooter> | ||
| </AlertDialogContent> | ||
| </AlertDialog> | ||
| )} | ||
| <> | ||
| {isFinal && ( | ||
| <Button | ||
| variant="primary" | ||
| className="font-semibold" |
amjithtitus09
requested changes
Apr 21, 2026
Member
amjithtitus09
left a comment
amjithtitus09
left a comment
There was a problem hiding this comment.
@NikhilA8606 Check out copilot comments
|
@nihal467 this looks fine |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
| {t("complete_service_request")} | ||
| </p> | ||
| <p className="text-xs text-gray-600"> | ||
| {t("complete_service_request_help_text")} |
There was a problem hiding this comment.
The footer help text (complete_service_request_help_text) tells users to ensure “all invoices, observations, and results are completed” before completing the request, but this bar is also shown for categories that are explicitly allowed to be completed without results (e.g. surgical procedure/counselling). Consider making the help text conditional by request category / presence of diagnostic reports, or rewriting it to be accurate for all cases.
Suggested change
| {t("complete_service_request_help_text")} | |
| {t("complete_service_request_help_text_generic", { | |
| defaultValue: | |
| "Ensure all required invoices, observations, and results are completed before completing the request.", | |
| })} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor