feat: refactor ReportViewer for Account report#16419
Conversation
Deploying care-preview with
|
| Latest commit: |
60945bd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5c4301f7.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-485.care-preview-a7w.pages.dev |
|
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:
WalkthroughReportViewer generalized to accept ChangesReport Viewer Generalization and Hardening
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR refactors
Confidence Score: 4/5The change is well-scoped and the core routing and prop-threading logic is correct. The two findings are minor: a loose The refactor correctly generalises src/pages/Encounters/ReportViewer.tsx — Important Files Changed
Reviews (1): Last reviewed commit: "feat: refactor ReportViewer for Account ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Refactors the existing encounter-focused ReportViewer so it can also be used to view Account reports from the billing flow, aligning the UI/UX with the follow-up PR feedback for ENG-485.
Changes:
- Generalizes
ReportViewerto use anassociatingIdand configurablereportType, and adds a printing/loading UI state. - Adds an account-report viewer route and wires the Account “Reports” tab to navigate to it.
- Removes the PDF print button from the generic
FilePreviewDialog(print is now handled inReportViewer).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Routers/routes/FacilityRoutes.tsx | Adds a new account report viewer route using ReportViewer. |
| src/Routers/routes/ConsultationRoutes.tsx | Updates encounter report routes to the new ReportViewer prop name. |
| src/pages/Facility/billing/account/AccountShow.tsx | Makes Account “Reports” tab navigate to the new report viewer route. |
| src/pages/Encounters/ReportViewer.tsx | Refactors report querying/generation + adds print loading UX and fetch validation. |
| src/components/Files/ReportSubTab.tsx | Adds getViewUrl to support custom “view report” navigation targets. |
| src/components/Common/FilePreviewDialog.tsx | Removes PDF print button from the generic file preview dialog. |
Comments suppressed due to low confidence (1)
src/pages/Encounters/ReportViewer.tsx:372
- The print iframe is appended without being hidden/styled, so it can show up as a visible blank frame in the page layout on some browsers.
const iframe = document.createElement("iframe");
iframe.src = blobUrl;
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/pages/Encounters/ReportViewer.tsx`:
- Around line 120-131: The queryKey arrays for the report fetching queries (the
one using query(reportApi.listReports, ...) and the polling/"fresh" query) must
include the dynamic reportType so cache entries are unique per report_type;
update the queryKey values (the arrays referenced as queryKey: ["reports",
associatingId, "template", effectiveTSlug]) to include reportType (e.g., add
reportType into the array) for both the main query and the fresh/polling query
so associatingId + effectiveTSlug + reportType produce distinct cache keys.
In `@src/Routers/routes/FacilityRoutes.tsx`:
- Around line 108-117: Replace the hard-coded report type string in the route's
ReportViewer props with the shared enum constant: import ReportType and change
the reportType prop from "account_report" to ReportType.ACCOUNT_REPORT in the
route handler for
"/facility/:facilityId/billing/account/:accountId/reports/:reportId"; this
ensures type-safety and consistency with usages like AccountShow.tsx and avoids
mismatches if the constant changes.
🪄 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: f69fc122-c7e9-4f45-a1a7-217f404e22c0
📒 Files selected for processing (6)
src/Routers/routes/ConsultationRoutes.tsxsrc/Routers/routes/FacilityRoutes.tsxsrc/components/Common/FilePreviewDialog.tsxsrc/components/Files/ReportSubTab.tsxsrc/pages/Encounters/ReportViewer.tsxsrc/pages/Facility/billing/account/AccountShow.tsx
💤 Files with no reviewable changes (1)
- src/components/Common/FilePreviewDialog.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/Encounters/ReportViewer.tsx (1)
120-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
reportTypeis still missing from the query key.
report_typeis a dynamic query param (Line 125), but thequeryKeyon Line 120 doesn't includereportType. Two report types sharing the sameassociatingId/effectiveTSlugwould collide on the same cache entry. The same omission exists in the "fresh" polling key (Lines 204-210).🐛 Proposed fix
- queryKey: ["reports", associatingId, "template", effectiveTSlug], + queryKey: ["reports", associatingId, "template", effectiveTSlug, reportType],And for the fresh query (Lines 204-210):
queryKey: [ "reports", associatingId, "template", effectiveTSlug, + reportType, "fresh", ],🤖 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/Encounters/ReportViewer.tsx` around lines 120 - 132, The queryKey used for fetching reports (the array starting with "reports", associatingId, "template", effectiveTSlug) omits the dynamic reportType, causing cache collisions; update the queryKey to include reportType (same place where queryParams sets report_type) so the key becomes unique per reportType, and make the analogous change to the "fresh" polling key used later (the fresh query/polling key array) to also include reportType; locate symbols queryKey, associatingId, effectiveTSlug, reportType and the query(reportApi.listReports, ...) call to apply the fix.
🤖 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.
Duplicate comments:
In `@src/pages/Encounters/ReportViewer.tsx`:
- Around line 120-132: The queryKey used for fetching reports (the array
starting with "reports", associatingId, "template", effectiveTSlug) omits the
dynamic reportType, causing cache collisions; update the queryKey to include
reportType (same place where queryParams sets report_type) so the key becomes
unique per reportType, and make the analogous change to the "fresh" polling key
used later (the fresh query/polling key array) to also include reportType;
locate symbols queryKey, associatingId, effectiveTSlug, reportType and the
query(reportApi.listReports, ...) call to apply the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 387abe64-7911-45c3-b8cb-e300f4cc2401
📒 Files selected for processing (2)
src/Routers/routes/FacilityRoutes.tsxsrc/pages/Encounters/ReportViewer.tsx
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #9459 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/pages/Encounters/ReportViewer.tsx:372
- The print iframe is appended without being hidden. This can cause a visible blank iframe to appear in the page layout during printing. Hide it like the previous implementation to avoid UI flicker/layout shifts.
const blob = await response.blob();
const blobUrl = window.URL.createObjectURL(blob);
const iframe = document.createElement("iframe");
iframe.src = blobUrl;
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/pages/Facility/billing/account/AccountShow.tsx`:
- Around line 173-183: Replace the raw string literals in the useQuery call with
the typed constants used elsewhere: use ReportType.ACCOUNT_REPORT for
template_type and, if available, the TemplateStatus enum/value for status (e.g.,
TemplateStatus.ACTIVE); update the queryParams passed to
templateApi.listTemplates accordingly and ensure types align with listTemplates'
request typing (adjust generics or casting if needed) so query params remain
consistent and type-safe.
🪄 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: d2b24e73-06a4-4e76-8036-b3dda6e6fd50
📒 Files selected for processing (2)
src/Routers/routes/FacilityRoutes.tsxsrc/pages/Facility/billing/account/AccountShow.tsx
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/Encounters/ReportViewer.tsx (1)
348-348:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd report file extension to download filename in ReportViewer
ReportViewer’shandleDownloadsetsanchor.download = report.name || report.report_type || "report"without usingReportReadList.extension, so downloads may be saved without a proper suffix (e.g., “.pdf”).ReportReadListincludesextension, and the preview download path already builds filenames as${file_state.name}.${file_state.extension}, so updatehandleDownloadto append.${report.extension}(only ifreport.namedoesn’t already include it).🤖 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/Encounters/ReportViewer.tsx` at line 348, In ReportViewer's handleDownload, ensure the downloaded filename includes the report file extension from ReportReadList.extension: when building anchor.download replace the current fallback (report.name || report.report_type || "report") with logic that uses report.name if present but appends `.` + report.extension when the name does not already end with that extension; otherwise use `${report.report_type}.${report.extension}` or `"report.${report.extension}"` as fallbacks, taking care not to duplicate the dot and to handle missing extension gracefully.
🤖 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/Encounters/ReportViewer.tsx`:
- Line 348: In ReportViewer's handleDownload, ensure the downloaded filename
includes the report file extension from ReportReadList.extension: when building
anchor.download replace the current fallback (report.name || report.report_type
|| "report") with logic that uses report.name if present but appends `.` +
report.extension when the name does not already end with that extension;
otherwise use `${report.report_type}.${report.extension}` or
`"report.${report.extension}"` as fallbacks, taking care not to duplicate the
dot and to handle missing extension gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0edc8844-6890-4457-bee0-9251bdcf67f5
📒 Files selected for processing (2)
src/pages/Encounters/ReportViewer.tsxsrc/pages/Facility/billing/account/AccountShow.tsx
|
once its done, mark it for testing @abhimanyurajeesh |
…rtSubTab; update ConsultationRoutes for new report paths
|
@nihal467 Need to test all the report view and generate flow
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
abhimanyurajeesh
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
closing in fav of #16440 |
abhimanyurajeesh
changed the title
Proposed Changes
This is a follow up PR #16418 (review)
This PR matches the requested UI and made few fixes
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Improvements