[ENG-500] Support for creating multiple diagnostic reports in a service request - #16455
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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:
WalkthroughUpdates diagnostic report creation and review to operate per report entry, with status-gated editing, per-report queries and mutations, upload input IDs, and new locale strings for report creation, approval, and review states. ChangesDiagnostic Report Multi-Report Refactor
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the Service Request diagnostic report workflow to support multiple diagnostic reports per request, updating both the entry form and the review UI to render and operate on each report independently.
Changes:
- Render diagnostic report entry/review UI as a list of per-report components instead of only using the latest report.
- Add UI for creating additional diagnostic reports (with optional report-type selection based on
activityDefinition.diagnostic_report_codes). - Update/enhance i18n strings to support the new UI text.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/pages/Facility/services/serviceRequests/components/DiagnosticReportReview.tsx |
Refactors review UI to render one collapsible review panel per diagnostic report and adjusts queries/actions per report. |
src/pages/Facility/services/serviceRequests/components/DiagnosticReportForm.tsx |
Refactors entry UI to render one collapsible form per report and adds UI to create additional reports (with report-type selection). |
public/locale/en.json |
Adds/updates translation strings for the multi-report workflow UI. |
Comments suppressed due to low confidence (1)
src/pages/Facility/services/serviceRequests/components/DiagnosticReportForm.tsx:1240
- With multiple diagnostic reports rendered, reusing
id="conclusion"/htmlFor="conclusion"creates duplicate IDs and breaks label association for assistive tech. Make the conclusion field IDs unique per report.
<Label
htmlFor="conclusion"
className="text-base font-semibold text-gray-950"
>
{t("conclusion")}
Greptile SummaryThis PR extends the diagnostic report workflow to support multiple concurrent reports per service request, each with its own edit card, approval flow, and print route. The old single-report form is decomposed into a
Confidence Score: 4/5Safe to merge with a minor fix: the conclusion textarea in DiagnosticReportItem uses a hardcoded id that duplicates across cards in multi-report mode. The multi-report refactor is well-structured. Per-report element ids were correctly scoped for file inputs and the review form conclusion field, but the edit form conclusion textarea still uses id=conclusion for every card. This only manifests when two or more preliminary reports are open simultaneously, causing the label to target the wrong textarea. Everything else including routing, query invalidation, state isolation, and i18n looks correct. Files Needing Attention: src/pages/Facility/services/serviceRequests/components/DiagnosticReportForm.tsx — conclusion textarea id needs to be scoped per-report the same way DiagnosticReportReview already does it.
|
| Filename | Overview |
|---|---|
| src/pages/Facility/services/serviceRequests/components/DiagnosticReportForm.tsx | Major refactor splitting the old single-report form into DiagnosticReportItem (per-report card with its own state/queries) and a new CreateDiagnosticReportForm. File upload ids are now scoped per-report, but the conclusion textarea still uses a hardcoded id that will duplicate when multiple preliminary reports are open. |
| src/pages/Facility/services/serviceRequests/components/DiagnosticReportReview.tsx | Refactored into a container + per-item DiagnosticReportReviewItem; conclusion correctly initialised from report prop and scoped ids used. |
| src/pages/Facility/services/diagnosticReports/DiagnosticReportPreview.tsx | New component extracted from DiagnosticReportPrint; handles multi-report print preview with per-item file URL fetching. |
| src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx | Simplified to delegate rendering to DiagnosticReportPreview; now supports both single-report and all-reports print routes. |
| src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx | Updated for multiple diagnostic reports: hasFinalizedReport/hasPendingReports completion gating and new all-reports print route. |
| src/Routers/routes/FacilityRoutes.tsx | Old per-report print route replaced with two new service_request-scoped routes. |
| src/hooks/useFileUpload.tsx | Added optional inputId prop to allow unique file input ids per card. |
| src/components/Files/FileUploadDialog.tsx | Added instanceId prop to prefix all internal element ids, fixing duplicate-id collisions. |
| public/locale/en.json | Added locale keys for new multi-report workflow; removed unused result_review and failed_to_update_conclusion keys. |
Reviews (24): Last reviewed commit: "used DR api instead of getting it from ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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/components/DiagnosticReportForm.tsx (1)
369-371:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBug:
toast.successused in error handler should betoast.error.The
onErrorcallback incorrectly usestoast.successto display the failure message, which will show a success toast for an error condition.onError: () => { - toast.success(t("failed_to_update_conclusion")); + toast.error(t("failed_to_update_conclusion")); },🤖 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/components/DiagnosticReportForm.tsx` around lines 369 - 371, In the onError callback within DiagnosticReportForm component, replace the toast.success call with toast.error to properly display an error notification when the conclusion update fails. This ensures that error conditions show error toasts rather than success toasts to users.
🤖 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.
Inline comments:
In `@public/locale/en.json`:
- Line 5071: Fix the spelling error in the review_test_results string value
where "Reult" is misspelled. Change it to "Result" so the user-facing string
reads "Review Test Result" instead of "Review Test Reult".
- Line 5537: The select_report_type_to_create string in the locale file is
grammatically awkward and confusing due to the repetition and unclear phrasing.
Rewrite this user-facing instruction to be clear and grammatically correct. The
new text should clearly tell users to select a diagnostic report type and then
create a report, without awkward quotation marks or repeated words, using simple
and direct language that guides the user action.
In
`@src/pages/Facility/services/serviceRequests/components/DiagnosticReportForm.tsx`:
- Around line 158-162: Replace all hardcoded English toast message strings with
translation keys using the t() function in DiagnosticReportForm.tsx. At lines
158-162 in the onError handler within the mutation, replace the hardcoded
failure message string with a call to t() using the key
"failed_to_create_diagnostic_report" and pass the error message as a parameter.
At lines 338-351, replace the hardcoded success message "Test results saved
successfully" with t("test_results_saved_successfully") and replace the
hardcoded failure message string in the error handler with a call to t() using
the key "failed_to_save_test_results" and pass the error message as a parameter.
For both locations, use t("unknown_error") as a fallback when the error message
is unavailable, consistent with i18n requirements.
- Around line 733-747: The upsertObservations and updateDiagnosticReport
mutations are executed sequentially without coordination, causing them to run in
parallel. If upsertObservations fails, updateDiagnosticReport will still
execute, leaving the diagnostic report in an inconsistent state. Fix this by
ensuring updateDiagnosticReport only executes after upsertObservations succeeds.
Either use mutateAsync with await to chain the mutations, or move the
updateDiagnosticReport call into the onSuccess callback of the
upsertObservations mutation. This ensures data consistency by preventing
updateDiagnosticReport from executing if the observations upsert operation
fails.
- Around line 1306-1312: The variables hasCollectedSpecimens and
isMultipleDiagnosticReport are being computed identically in both the parent
component and the DiagnosticReportForm child component. To follow DRY principles
and ensure consistency, remove the duplicate computations from
DiagnosticReportForm (the const declarations for hasCollectedSpecimens based on
activityDefinition.specimen_requirements and specimens, and
isMultipleDiagnosticReport based on activityDefinition.diagnostic_report_codes).
Instead, compute these values once in the parent component and pass them as
props to DiagnosticReportForm. Update the DiagnosticReportForm component
signature to accept these two values as props and use the passed-in values
instead of computing them locally.
In
`@src/pages/Facility/services/serviceRequests/components/DiagnosticReportReview.tsx`:
- Around line 206-208: The Badge component and status-driven actions (render
gates for "view report" and approve actions) in DiagnosticReportReview.tsx are
using report.status from the stale list data instead of the fresher reportDetail
object. Replace report.status with the status from reportDetail at lines 206-208
and also at lines 294-331 where similar status checks are used for conditional
rendering and actions, ensuring all UI elements reflect the fetched detail data
consistently rather than potentially stale source data.
- Around line 163-167: The empty-state check for observations in the condition
block does not filter out ENTERED_IN_ERROR observations, while the table that
renders those observations applies this filter. This causes reports containing
only entered-in-error observations to display as non-empty with blank results.
Update the observation length check in the condition to filter out any
observations with ENTERED_IN_ERROR status before evaluating the length, matching
the filtering logic applied to the table rendering below. Apply the same
filtering logic at all affected locations where the raw observation length is
evaluated to determine visibility.
- Around line 102-106: The conclusion input field at line 267 (and related area
at lines 266-268) uses a fallback value logic that re-injects the server value
when the user clears the field, preventing empty edits from being representable
in UI state. Remove the fallback to reportDetail.conclusion in the value prop of
the input field—change the value binding from value={conclusion ||
reportDetail.conclusion || ""} to value={conclusion || ""} so that when users
clear the conclusion field, the empty state persists without being overwritten
by the server value.
- Around line 144-147: The toast.error call in the onError handler of
DiagnosticReportReview.tsx is using hardcoded English strings instead of i18next
translation keys. Add two new translation keys to public/locale/en.json: one for
the main error message template that accepts an error parameter (like
"diagnostic_report_approve_failed"), and one for the fallback unknown error text
(like "unknown_error"). Then update the toast.error call in the onError handler
to use i18next.t() to retrieve the localized message, passing the error text as
a parameter to the translation template.
---
Outside diff comments:
In
`@src/pages/Facility/services/serviceRequests/components/DiagnosticReportForm.tsx`:
- Around line 369-371: In the onError callback within DiagnosticReportForm
component, replace the toast.success call with toast.error to properly display
an error notification when the conclusion update fails. This ensures that error
conditions show error toasts rather than success toasts to users.
🪄 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: bf6bbacd-e990-42e4-a76b-8cf598ade0b9
📒 Files selected for processing (3)
public/locale/en.jsonsrc/pages/Facility/services/serviceRequests/components/DiagnosticReportForm.tsxsrc/pages/Facility/services/serviceRequests/components/DiagnosticReportReview.tsx
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10565 |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Deploying care-preview with
|
| Latest commit: |
cfcaa11
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://faac5c4d.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-500-support-for-creating.care-preview-a7w.pages.dev |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
yash-learner
left a comment
yash-learner
left a comment
There was a problem hiding this comment.
File upload is broken in ServiceRequest page can you fix it
yash-learner
left a comment
yash-learner
left a comment
There was a problem hiding this comment.
@aravindm4 mentioned we should only allow one diagnostic report per code
| <div className="mt-8 border-t border-gray-300 pt-6"> | ||
| {/* Report header with per-report details */} | ||
| <div className="break-inside-avoid"> | ||
| <h2 className="text-lg font-semibold mb-3">{report.code?.display}</h2> |
There was a problem hiding this comment.
Do you think, is it good to show the report status as well in the print ? -> Preliminary or Final
if we mark a service request as entered in error/cancelled state we are not showing that in the timeline if you think if it already present on develop and if you're planning to fix in this PR, file a separate Jira ticket |
I think |
yash-learner
left a comment
yash-learner
left a comment
There was a problem hiding this comment.
We do not have this button in new design, it is below
and also this button shows up even when all reports not final
On the encounter diagnostic report path -> The diagnostic reports of a service request does not show actual diagnostic report name/code it just showing test results
it is not present in develop |
I discussed this with @gauritejusa, and we decided to keep the View All Reports button only at the top. It doesn't need to be shown at the bottom as well. We also decided that the button should be displayed even when there is only a single report. This was discussed and confirmed with the product team. |
This is the existing behavior in develop as well and is not related to this PR. |
Because previously we had only one diagnostic report per Service Request and now we support multiple so I think it is good to show because without that how would some distinguish which report is which ? check with Product |
But in develop we do not have latest badge right now we have |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
tests/facility/patient/encounter/serviceRequests/ServiceRequestCreate.spec.ts:182
- Using
await reportTypeSelect.count()to decide whether to expand the card can be flaky becauselocator.count()doesn't auto-wait for the dropdown to appear. If the dropdown is still rendering, the count can momentarily be 0 and the extra click may collapse the card, causing intermittent failures. Prefer attempting the click and only expanding on timeout/failure.
// reveal the report-type dropdown before interacting with it.
if ((await reportTypeSelect.count()) === 0) {
await page.getByText("Test Results Entry").click();
}
await reportTypeSelect.click();
src/pages/Encounters/tabs/diagnostic-reports.tsx:174
buildEncounterUrl(...)falls back to the organization route whenfacilityIdis undefined, but the new print route is only registered under facility routes. In org-context encounter pages this would navigate to/organization/organizationId/.../service_request/.../diagnostic_report/.../print, which has no matching route. Use the report's encounter facility id as a fallback so printing works from both contexts.
facilityId,
src/pages/Facility/services/serviceRequests/components/DiagnosticReportReview.tsx:173
- This effect always overwrites the local
conclusionstate when the full report finishes loading. If a user starts typing before the detail query resolves, their input can be reset back to the server value (often empty) mid-edit. Only sync from the server when the user hasn’t changed the field yet (e.g., when the local value is still empty or equals the initial report conclusion).
useEffect(() => {
setConclusion(reportDetail?.conclusion || "");
}, [reportDetail?.conclusion]);
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx:633
- The new behavior disables “Mark as complete” while any diagnostic reports are still pending final review. There isn’t Playwright coverage validating that (a) multiple reports can be created and (b) the completion CTA stays disabled until all reports are finalized. Adding an E2E spec (or extending the existing ServiceRequestCreate spec) would prevent regressions in this workflow.
disabled={isCompletingServiceRequest || hasPendingReports}
As discussed, we can keep the existing behavior from develop. |
nice finding. https://openhealthcarenetwork.atlassian.net/browse/ENG-500?focusedCommentId=12420 |
rithviknishad
left a comment
rithviknishad
left a comment
There was a problem hiding this comment.
simply use service_request filter instead to fetch required diagnostic reports?
I intentionally avoided that API because, for superusers, it fetches all patient records without any filtering. As we discussed, I'm avoiding that use case for now and using the retrieve API instead, since this is the existing behavior across CARE. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
src/pages/Facility/services/serviceRequests/components/DiagnosticReportReview.tsx:173
- This effect resets
conclusionwhenever the full report loads. If a user starts typing before the detail query resolves, their input can be overwritten. It’s safer to only sync from the fetched report when the user hasn’t edited the field yet.
useEffect(() => {
setConclusion(reportDetail?.conclusion || "");
}, [reportDetail?.conclusion]);
tests/facility/patient/encounter/serviceRequests/ServiceRequestCreate.spec.ts:182
- This spec updates interactions for the report-type dropdown, but it still only validates creating a single diagnostic report. Since this PR adds support for multiple diagnostic reports per service request (including completion being blocked when reports are pending), add Playwright coverage for creating at least two reports and verifying the expected review/print/complete behavior.
const reportTypeSelect = page
.getByRole("combobox")
.filter({ hasText: "Select Diagnostic Report Type" });
// The "Test Results Entry" card is collapsed by default; expand it to
// reveal the report-type dropdown before interacting with it.
if ((await reportTypeSelect.count()) === 0) {
await page.getByText("Test Results Entry").click();
}
await reportTypeSelect.click();
src/pages/Facility/services/serviceRequests/components/WorkflowProgress.tsx:205
WorkflowProgresscurrently pushes two timeline events per diagnostic report (the status event in the first loop and a separate "created" event in the second). For preliminary reports both events usecreated_date, which produces duplicate entries with the same timestamp and title/meaning. Consider only adding the extra "created" entry for finalized reports (or otherwise de-duplicating).
// Add diagnostic report events
request.diagnostic_reports?.forEach((report: DiagnosticReportRead) => {
events.push({
title: t("diagnostic_report_created"),
description: t("diagnostic_report_created_description", {
src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx:50
DiagnosticReportPrintcan briefly render "service_request_not_found" while the service request query is still loading, because only the diagnostic report queries are included in the loading gate. Include the service request query's loading state in the loading check so we don't show a not-found state prematurely.
const { data: request } = useQuery({
queryKey: ["serviceRequest", facilityId, serviceRequestId],
queryFn: query(serviceRequestApi.retrieveServiceRequest, {
pathParams: {
facilityId: facilityId,
src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx:72
- When there are no finalized diagnostic reports for the service request,
diagnosticReportsbecomes an empty array (truthy) and the component renders an empty preview instead of the "no_diagnostic_reports_found" state. Treat an empty array as "not found" here.
if (!diagnosticReports) {
return <div>{t("no_diagnostic_reports_found")}</div>;
}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/pages/Facility/services/serviceRequests/components/DiagnosticReportReview.tsx:173
- This effect will overwrite the user’s in-progress edits to the conclusion when
fullReportfinishes loading (becausereportDetail.conclusionchanges), causing typed text to disappear. The conclusion should only be initialized from fetched data when the user hasn’t modified it yet.
useEffect(() => {
setConclusion(reportDetail?.conclusion || "");
}, [reportDetail?.conclusion]);
tests/facility/patient/encounter/serviceRequests/ServiceRequestCreate.spec.ts:184
- This PR adds a new user-facing workflow for creating multiple diagnostic reports, but this spec still only exercises a single-report creation path. Adding an E2E assertion that the “Another Diagnostic Report” flow works (create a second report, verify both report cards appear, and that completion is blocked until all are final) would prevent regressions.
const reportTypeSelect = page
.getByRole("combobox")
.filter({ hasText: "Select Diagnostic Report Type" });
// The "Test Results Entry" card is collapsed by default; expand it to
// reveal the report-type dropdown before interacting with it.
if ((await reportTypeSelect.count()) === 0) {
await page.getByText("Test Results Entry").click();
}
await reportTypeSelect.click();
await page.getByRole("option").first().click();
await page.getByRole("button", { name: "Create Report" }).click();
src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx:54
serviceRequestis fetched viauseQuery, but its loading state isn’t included in the early-loading guard. This can briefly renderservice_request_not_foundwhile the request query is still in-flight. Also, thediagnosticReportsguard only checks forundefined; if the list query returns an empty array, the UI will render an empty preview instead ofno_diagnostic_reports_found.
const { data: request } = useQuery({
queryKey: ["serviceRequest", facilityId, serviceRequestId],
queryFn: query(serviceRequestApi.retrieveServiceRequest, {
pathParams: {
facilityId: facilityId,
serviceRequestId: serviceRequestId,
},
}),
});
nihal467
left a comment
nihal467
left a comment
There was a problem hiding this comment.
- the requested by data is missing in the view full report (Cumulative Report) in the SR, can you verify what should be shown here
| </div> | ||
| <DiagnosticReportPreview | ||
| diagnosticReports={diagnosticReports} | ||
| isLoading={isLoadingReport || isLoadingReports} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/facility/patient/encounter/serviceRequests/ServiceRequestCreate.spec.ts:182
- After expanding the "Test Results Entry" card, the test clicks the report-type combobox immediately. This can be flaky if the dropdown renders asynchronously (or if multiple comboboxes match). Add an assertion that the locator exists before clicking, and click a single element explicitly.
if ((await reportTypeSelect.count()) === 0) {
await page.getByText("Test Results Entry").click();
}
await reportTypeSelect.click();
src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx:55
- The print view fetches full details for every diagnostic report returned by
listDiagnosticReports, but later only printsfinalreports. Filtering tofinalbeforeuseQueries()avoids unnecessary network requests when there are preliminary/in-progress reports.
const { allDiagnosticReports, isLoading: isLoadingReports } = useQueries({
queries:
diagnosticReportResults?.map((report) => ({
queryKey: ["diagnosticReport", report.id, patientId, facilityId],
queryFn: query(diagnosticReportApi.retrieveDiagnosticReport, {
src/pages/Facility/services/serviceRequests/components/WorkflowProgress.tsx:205
- This adds a second diagnostic-report timeline entry for every report (“created” + “in progress/approved”). For non-final reports, both entries use
created_date, so the timeline will show near-duplicate items with identical timestamps. Consider only adding the “created” event once a report is finalized (or otherwise ensure timestamps differ).
// Add diagnostic report events
request.diagnostic_reports?.forEach((report: DiagnosticReportRead) => {
events.push({
title: t("diagnostic_report_created"),
description: t("diagnostic_report_created_description", {
src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx:98
diagnosticReportsis always an array here, soif (!diagnosticReports)never triggers. This means an empty result set (or an invaliddiagnosticReportIdwherefullReportis undefined) will fall through and render the preview with zero reports. Check fordiagnosticReports.length === 0, and add an explicit guard fordiagnosticReportId && !fullReportto show a not-found state.
if (!diagnosticReports) {
return <div>{t("no_diagnostic_reports_found")}</div>;
}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The feature direction is reasonable — multiple diagnostic reports per service request is a legitimate need, and most of the refactoring (splitting DiagnosticReportForm into DiagnosticReportItem, extracting DiagnosticReportPreview, per-report file upload IDs) is sound. The useQueries pattern in DiagnosticReportPrint.tsx is the right approach.
However, there are a few real bugs and code quality issues that need addressing before merge:
{} as anyinDiagnosticReportPreview.tsx— calling a React Query factory imperatively with a fake context. This bypasses the type system and is fragile. Should useuseQueriesor direct API calls.- Missing
getFileUrlinuseEffectdeps (DiagnosticReportPreview.tsx:251) — stale closure waiting to cause subtle bugs. - Dead code guard (
DiagnosticReportPrint.tsx:96) —if (!diagnosticReports)is always false. - Silent empty print when all reports are
preliminaryin the all-reports print route — this needs either documentation or a user-facing message. const { data: data }— proofreading issue.eslint-disablesilencing missing deps — either justify it with a comment or fix the actual issue.
The window.innerWidth issue in PDFRenderer is a minor annoyance for a print-only component, but test environments will give you 0px pages.
Fix the {} as any and the dead code at minimum. The rest are code quality issues that'll cause head-scratching later.
Generated by Grumpy PR Reviewer for issue #16455 · 100.7 AIC · ⌖ 6.35 AIC · ⊞ 6.3K
| } catch (error) { | ||
| console.error("Error fetching signed URL:", error); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Oh wonderful, {} as any. The gold standard of "I gave up on types." query() returns a function that expects a QueryFunctionContext, not an empty object cast to oblivion. This is calling the query factory imperatively outside of React Query — that's the actual problem. Use fetch or the API client directly for imperative calls, or restructure this to use useQueries for each file like the DiagnosticReportPrint.tsx does. Don't paper over broken abstractions with as any.
| return ( | ||
| <div className="p-6"> | ||
| <div className="text-center text-gray-500"> | ||
| {t("diagnostic_report_not_found")} |
There was a problem hiding this comment.
getFileUrl is defined in component scope and closes over report?.id, but it's not in the useEffect dependency array. ESLint is screaming at you and you chose to not listen. Either move getFileUrl inside the effect, wrap it in useCallback and add it to deps, or — better — refactor to useQueries like the rest of this PR does. Stale closures in effects cause bugs that are a joy to debug at 2am.
| const { facility } = useCurrentFacility(); | ||
|
|
||
| const { data: report, isLoading } = useQuery({ | ||
| const { data: data } = useQuery({ |
There was a problem hiding this comment.
const { data: data } — really? JavaScript has had shorthand property names since ES6. This is const { data }. The redundant alias tells me this code wasn't proofread before submission.
| ext.endsWith("webp") | ||
| ); | ||
| }); | ||
| if (!diagnosticReports) { |
There was a problem hiding this comment.
diagnosticReports is either [fullReport] or allDiagnosticReports.filter(...) — it is always an array, never null or undefined. This check if (!diagnosticReports) will never be true. Dead code. The error message will never display. If you want to handle the empty case, check diagnosticReports.length === 0.
| const diagnosticReports = fullReport | ||
| ? [fullReport] | ||
| : allDiagnosticReports.filter( | ||
| (report) => report.status === DiagnosticReportStatus.final, |
There was a problem hiding this comment.
When printing "all reports" via the no-diagnosticReportId route, this silently filters to only final status reports. A user navigating to the all-reports print URL will get a blank print if all reports are preliminary. There's no error, no loading state — just nothing. Is this intentional design? If so, document it. If not, fix it.
| if (!openUploadDialog) { | ||
| fileUpload.clearFiles(); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Ah yes, the classic // eslint-disable-next-line react-hooks/exhaustive-deps. The lint rule is there for a reason. fileUpload.clearFiles is a function from a hook — if it's not stable (not wrapped in useCallback), omitting it from deps can cause stale references. Either ensure clearFiles is stable and document why, or add it to the deps. Don't just silence the linter and move on.
| renderAnnotationLayer={false} | ||
| /> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
window.innerWidth captured during render is not reactive — resize the window and the PDF page width stays wrong. For a print view this matters less, but it also breaks in test environments where window.innerWidth is 0, making every PDF page 0px wide. At minimum, capture this in a useEffect or useMemo with a resize listener, or use a CSS-based approach.
Proposed Changes
ENG-500
Figma link: https://www.figma.com/design/Z93EYKSa1MdBmXndsMBJSQ/Care---Master?node-id=40865-19149&t=94gI7vbM9r8kx6hY-1
Screen record:
Screen.Recording.2026-06-16.at.11.31.07.AM.mov
Merge Checklist
Summary by CodeRabbit
Release Notes
{{error}}details).