Skip to content

[ENG-736] Fix Diagnostic Report print in Org level - #16556

Open
NikhilA8606 wants to merge 1 commit into
developfrom
ENG-736-fix-diagnostic-report-print-in-org-level
Open

[ENG-736] Fix Diagnostic Report print in Org level#16556
NikhilA8606 wants to merge 1 commit into
developfrom
ENG-736-fix-diagnostic-report-print-in-org-level

Conversation

@NikhilA8606

@NikhilA8606 NikhilA8606 commented Jul 13, 2026

Copy link
Copy Markdown
Member

Proposed Changes

Fix: https://openhealthcarenetwork.atlassian.net/browse/ENG-736

Screen.Recording.2026-07-13.at.7.59.23.PM.mov

Tagging: @ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate the bug or test the new feature.
  • Update product documentation.
  • Ensure that UI text is placed in I18n files.
  • Prepare a screenshot or demo video for the changelog entry and attach it to the issue.
  • Request peer reviews.
  • Complete QA on mobile devices.
  • Complete QA on desktop devices.
  • Add or update Playwright tests for related changes

Summary by CodeRabbit

  • Bug Fixes
    • Improved diagnostic report printing by retrieving facility details without interrupting the print preview experience.
    • Preserved existing report loading, PDF/image rendering, and layout behavior.

@NikhilA8606
NikhilA8606 requested review from a team and Copilot July 13, 2026 14:30
@NikhilA8606

NikhilA8606 commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

ENG-736

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c1402889-2427-4339-9327-c54612917e2f

📥 Commits

Reviewing files that changed from the base of the PR and between 8160376 and d51944c.

📒 Files selected for processing (1)
  • src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx
 ___________________________________________________________________
< That's not technical debt - that's technical *predatory lending*. >
 -------------------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ENG-736-fix-diagnostic-report-print-in-org-level

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes the diagnostic report print page crashing when accessed from an organization-level route by swapping useCurrentFacility (which throws when the URL path isn't under /facility/:id/) for useCurrentFacilitySilently (which returns undefined instead of throwing).

  • The one-line change lets facility be undefined on org-level routes, so PrintPreview renders without a facility context rather than crashing.
  • useCurrentFacilitySilently is a pre-existing utility that wraps useCurrentFacility in a try-catch; the try-catch pattern causes useQuery to be conditionally skipped (not called) on non-facility paths, which is technically a Rules-of-Hooks violation — though benign for a print page that never switches route contexts during its lifetime.

Confidence Score: 4/5

Safe to merge — the change is a targeted one-liner that unblocks printing on org-level routes with no risk to facility-level flows.

The change is minimal and correct for the bug being fixed. The only concern is that the underlying useCurrentFacilitySilently utility uses a try-catch around hook calls, which is a pre-existing pattern not introduced here; the print page won't remount with a different path type, so no runtime breakage is expected.

src/pages/Facility/utils/useCurrentFacility.tsx warrants a follow-up to remove the try-catch hook pattern in favour of an enabled guard.

Important Files Changed

Filename Overview
src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx Switches from useCurrentFacility (throws on non-facility routes) to useCurrentFacilitySilently (returns undefined gracefully) to fix the diagnostic report print view at org level. The underlying silent hook carries a pre-existing Rules-of-Hooks concern.

Reviews (1): Last reviewed commit: "ENG-736 Fix Diagnostic Report print in O..." | Re-trigger Greptile

}) {
const { t } = useTranslation();
const { facility } = useCurrentFacility();
const { facility } = useCurrentFacilitySilently();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Hooks called inside try-catch in the underlying utility

useCurrentFacilitySilently wraps useCurrentFacility in a try-catch. Inside useCurrentFacility, useFullPath() runs first, then extractFacilityId throws synchronously before useQuery is reached. This means on a non-facility path only one hook fires, while on a facility path two fire — a conditional hook-count difference that violates React's Rules of Hooks. In practice this is stable here because a print page stays on one path type for its lifetime, but if the component were ever remounted on a different path type React would throw a "Rendered more hooks than during the previous render" error. A safer long-term fix would be to make extractFacilityId return undefined instead of throwing, then pass enabled: !!facilityId to useQuery, avoiding the try-catch entirely.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a crash when rendering/printing a Diagnostic Report from organization-level routes by avoiding the strict “must be within a facility route” constraint when reading the current facility context.

Changes:

  • Switched from useCurrentFacility() to useCurrentFacilitySilently() in the Diagnostic Report print page to prevent route-context errors at org level.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

🎭 Playwright Test Results

Status: ✅ Passed
Test Shards: 3

Metric Count
Total Tests 331
✅ Passed 331
❌ Failed 0
⏭️ Skipped 0

📊 Detailed results are available in the playwright-final-report artifact.

Run: #10166

@Jacobjeevan Jacobjeevan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this does fix the issue, we should also add facility information in the print, but this needs to be a discussion at product level.

DiagnosticReport model does include facility, but currently the viewset doesn't return it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants