feat: Diagnostic Report multi print #15297
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds multi-print support for diagnostic reports: UI to select/filter reports by category, a print sheet, a consolidated multi-report print view with attached files and grouped results, shared file renderers, a new route, updated encounter tab integration, and new localization keys. Changes
🚥 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 adds functionality to print multiple diagnostic reports at once, allowing users to select and batch-print reports grouped by category.
Changes:
- Added a new print sheet component for selecting multiple diagnostic reports
- Created a multi-print view that groups reports by category and displays them in a printable format
- Integrated the print sheet into the diagnostic reports tab with a "Print Multiple" button
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| DiagnosticReportPrintSheet.tsx | New component providing UI for selecting multiple reports with category filtering |
| DiagnosticReportMultiPrint.tsx | New page component that renders selected reports grouped by category in print format |
| diagnostic-reports.tsx | Added print button and integrated print sheet into the reports list panel |
| FacilityRoutes.tsx | Added route for the multi-print functionality |
| en.json | Added i18n strings for multi-print feature |
|
|
||
| const handleOpenChange = (newOpen: boolean) => { | ||
| setIsOpen(newOpen); | ||
| onOpenChange?.(newOpen); | ||
| if (!newOpen) { | ||
| // Reset state when closing | ||
| setCategoryFilter("all"); |
There was a problem hiding this comment.
When closing the sheet, the selected reports are not cleared, only the category filter. This could lead to confusion if users reopen the sheet and see previously selected items still checked. Consider also calling clearSelection() here to reset the selection state when the sheet closes.
| const handleOpenChange = (newOpen: boolean) => { | |
| setIsOpen(newOpen); | |
| onOpenChange?.(newOpen); | |
| if (!newOpen) { | |
| // Reset state when closing | |
| setCategoryFilter("all"); | |
| const clearSelection = () => { | |
| setSelectedReports(new Set()); | |
| }; | |
| const handleOpenChange = (newOpen: boolean) => { | |
| setIsOpen(newOpen); | |
| onOpenChange?.(newOpen); | |
| if (!newOpen) { | |
| // Reset state when closing | |
| setCategoryFilter("all"); | |
| clearSelection(); |
| groups.push(group); | ||
| } | ||
|
|
||
| categoryMap.get(categoryCode)!.reports.push(report); |
There was a problem hiding this comment.
Using non-null assertion operator (!) is unsafe here. Although the map entry was just created, it's better to use optional chaining with a fallback or explicit null check for defensive programming.
| report.service_request?.category || | ||
| report.category?.code || | ||
| "uncategorized"; |
There was a problem hiding this comment.
The fallback value "uncategorized" is a string literal that may not be properly translated. Consider using a constant or ensuring this value exists in the i18n files for consistency with other category values.
| // Use category.display for display name, fallback to translated category code | ||
| const categoryDisplay = | ||
| report.category?.display || t(categoryCode) || t("uncategorized"); |
There was a problem hiding this comment.
The chained fallback logic t(categoryCode) || t("uncategorized") assumes that if t(categoryCode) returns a falsy value, it should fall back to t("uncategorized"). However, i18next typically returns the key itself if no translation exists, which is truthy. This fallback chain may not work as intended.
| // Use category.display for display name, fallback to translated category code | |
| const categoryDisplay = | |
| report.category?.display || t(categoryCode) || t("uncategorized"); | |
| // Use category.display for display name, fallback to translated category code, | |
| // and finally to the "uncategorized" translation if no specific translation exists. | |
| const translatedCategory = t(categoryCode); | |
| const categoryDisplay = | |
| report.category?.display || | |
| (translatedCategory === categoryCode | |
| ? t("uncategorized") | |
| : translatedCategory); |
Deploying care-preview with
|
| Latest commit: |
4e6f66d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://471d2b73.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://add-diag-multi-print.care-preview-a7w.pages.dev |
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #5267 |
| onOpenChange?.(newOpen); | ||
| if (!newOpen) { | ||
| // Reset state when closing | ||
| setCategoryFilter("all"); |
There was a problem hiding this comment.
When the sheet closes, the selected reports are not cleared. This could lead to unexpected behavior if the user reopens the sheet and expects a fresh state. Consider also calling clearSelection() when closing the sheet to ensure a clean state on reopen.
| setCategoryFilter("all"); | |
| setCategoryFilter("all"); | |
| setSelectedReports(new Set<string>()); |
| associating_id: reportId, | ||
| }, | ||
| pathParams: { fileId: file.id }, | ||
| })({} as any); |
There was a problem hiding this comment.
Using {} as any bypasses TypeScript's type safety. This should be replaced with the proper type parameter expected by the query function, or at minimum use unknown instead of any for better type safety.
| })({} as any); | |
| })({} as unknown); |
| for (const [reportId, files] of filesByReport) { | ||
| for (const file of files) { | ||
| if (!file.id) continue; | ||
| // Skip if already fetched | ||
| if (fetchedFileIdsRef.current.has(file.id)) continue; | ||
|
|
||
| const url = await getFileUrl(file, reportId); | ||
| if (url) { | ||
| urls[file.id] = url; | ||
| fetchedFileIdsRef.current.add(file.id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| setFileUrls((prev) => ({ ...prev, ...urls })); | ||
| setIsLoadingUrls(false); |
There was a problem hiding this comment.
The fetchAllUrls async function is called without awaiting or handling potential errors. While the errors are caught within getFileUrl, consider adding error handling at this level or using a try-catch wrapper to handle any unexpected failures gracefully.
| for (const [reportId, files] of filesByReport) { | |
| for (const file of files) { | |
| if (!file.id) continue; | |
| // Skip if already fetched | |
| if (fetchedFileIdsRef.current.has(file.id)) continue; | |
| const url = await getFileUrl(file, reportId); | |
| if (url) { | |
| urls[file.id] = url; | |
| fetchedFileIdsRef.current.add(file.id); | |
| } | |
| } | |
| } | |
| setFileUrls((prev) => ({ ...prev, ...urls })); | |
| setIsLoadingUrls(false); | |
| try { | |
| for (const [reportId, files] of filesByReport) { | |
| for (const file of files) { | |
| if (!file.id) continue; | |
| // Skip if already fetched | |
| if (fetchedFileIdsRef.current.has(file.id)) continue; | |
| const url = await getFileUrl(file, reportId); | |
| if (url) { | |
| urls[file.id] = url; | |
| fetchedFileIdsRef.current.add(file.id); | |
| } | |
| } | |
| } | |
| setFileUrls((prev) => ({ ...prev, ...urls })); | |
| } catch (error) { | |
| // Handle any unexpected errors while fetching URLs | |
| // eslint-disable-next-line no-console | |
| console.error("Failed to fetch file URLs", error); | |
| } finally { | |
| setIsLoadingUrls(false); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/pages/Facility/services/diagnosticReports/components/FileRenderers.tsx`:
- Around line 7-10: The component signatures use inline prop types; extract and
export named interfaces (e.g., declare and export interface PDFRendererProps {
fileUrl: string; } and replace the inline type on PDFRenderer with
PDFRendererProps) and do the same for the other exported renderers referenced
(lines 34–40) so their props are reusable and extendable; update any places that
consume these components to import and use the new interfaces if needed.
- Around line 20-25: The code in FileRenderers.tsx uses window.innerWidth
directly when rendering the Page component (inside the Array.map that renders
Page with width={Math.min(window.innerWidth * 0.9, 600)}), which can crash in
SSR/tests; replace that direct access with a safe fallback: compute a safeWidth
value using typeof window !== "undefined" ? window.innerWidth : 800 (or another
sensible default) or derive it from a prop, and use width={Math.min(safeWidth *
0.9, 600)}; alternatively, move window access into an effect that sets state
(e.g., useEffect -> setViewportWidth) and use that state so server render
doesn't touch window.
In
`@src/pages/Facility/services/diagnosticReports/DiagnosticReportMultiPrint.tsx`:
- Around line 274-277: The alt text "Care Logo" in
DiagnosticReportMultiPrint.tsx should be localized: import and use the i18next
hook (useTranslation) or t prop in the DiagnosticReportMultiPrint component and
replace the literal alt with a translated key (e.g., t('care.logo') or a
context-specific key like t('diagnosticReport.careLogo')), and add the
corresponding translation entries to your i18n resource files so the logo alt
text is translated across locales.
- Around line 327-333: The displayed date in DiagnosticReportMultiPrint
currently uses format(new Date(), "dd-MM-yyyy") while the label calls
t("report_date"); update the component to use the report-derived date (e.g., the
report object's date field used elsewhere in this component such as report.date,
report.reportedAt, selectedReport?.date, or reports[0]?.date) and fall back to
new Date() only if the report date is missing; alternatively, if you intend to
show the print time instead, change the label call from t("report_date") to
t("print_date") (and update translations) and keep format(new Date(), ...).
Ensure you replace the specific expression format(new Date(), "dd-MM-yyyy")
accordingly in DiagnosticReportMultiPrint.
♻️ Duplicate comments (1)
src/pages/Facility/services/diagnosticReports/components/DiagnosticReportPrintSheet.tsx (1)
63-69: Clear selected reports when the sheet closes.Closing the sheet only resets the category filter; previously selected reports remain checked on reopen. Consider clearing selection on close to avoid stale state.
🛠️ Proposed fix
if (!newOpen) { // Reset state when closing setCategoryFilter("all"); + clearSelection(); }
| export function PDFRenderer({ fileUrl }: { fileUrl: string }) { | ||
| const [numPages, setNumPages] = useState<number>(0); | ||
| const { t } = useTranslation(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer explicit props interfaces for exported components.
Inline object types in public component signatures make reuse and extension harder. Consider named interfaces. As per coding guidelines, prefer interfaces over inline object types.
♻️ Suggested refactor
+interface PDFRendererProps {
+ fileUrl: string;
+}
+
-export function PDFRenderer({ fileUrl }: { fileUrl: string }) {
+export function PDFRenderer({ fileUrl }: PDFRendererProps) {
@@
-export function ImageRenderer({
- fileUrl,
- fileName,
-}: {
- fileUrl: string;
- fileName?: string;
-}) {
+interface ImageRendererProps {
+ fileUrl: string;
+ fileName?: string;
+}
+
+export function ImageRenderer({ fileUrl, fileName }: ImageRendererProps) {Also applies to: 34-40
🤖 Prompt for AI Agents
In `@src/pages/Facility/services/diagnosticReports/components/FileRenderers.tsx`
around lines 7 - 10, The component signatures use inline prop types; extract and
export named interfaces (e.g., declare and export interface PDFRendererProps {
fileUrl: string; } and replace the inline type on PDFRenderer with
PDFRendererProps) and do the same for the other exported renderers referenced
(lines 34–40) so their props are reusable and extendable; update any places that
consume these components to import and use the new interfaces if needed.
| {Array.from(new Array(numPages), (_, index) => ( | ||
| <Page | ||
| key={`page_${index + 1}`} | ||
| pageNumber={index + 1} | ||
| width={Math.min(window.innerWidth * 0.9, 600)} | ||
| scale={1.2} |
There was a problem hiding this comment.
Guard window usage to avoid runtime crashes outside the browser.
Line 24 directly accesses window, which can throw in SSR/test/print pre-render contexts. Add a safe fallback.
🔧 Suggested fix
export function PDFRenderer({ fileUrl }: { fileUrl: string }) {
const [numPages, setNumPages] = useState<number>(0);
const { t } = useTranslation();
+ const pageWidth =
+ typeof window === "undefined"
+ ? 600
+ : Math.min(window.innerWidth * 0.9, 600);
return (
<div className="break-before-page">
<Document
@@
<Page
key={`page_${index + 1}`}
pageNumber={index + 1}
- width={Math.min(window.innerWidth * 0.9, 600)}
+ width={pageWidth}
scale={1.2}
/>🤖 Prompt for AI Agents
In `@src/pages/Facility/services/diagnosticReports/components/FileRenderers.tsx`
around lines 20 - 25, The code in FileRenderers.tsx uses window.innerWidth
directly when rendering the Page component (inside the Array.map that renders
Page with width={Math.min(window.innerWidth * 0.9, 600)}), which can crash in
SSR/tests; replace that direct access with a safe fallback: compute a safeWidth
value using typeof window !== "undefined" ? window.innerWidth : 800 (or another
sensible default) or derive it from a prop, and use width={Math.min(safeWidth *
0.9, 600)}; alternatively, move window access into an effect that sets state
(e.g., useEffect -> setViewportWidth) and use that state so server render
doesn't touch window.
| <img | ||
| src={careConfig.mainLogo?.dark} | ||
| alt="Care Logo" | ||
| className="h-10 w-auto object-contain ml-6" |
There was a problem hiding this comment.
Localize the logo alt text.
The literal "Care Logo" should be translated to support i18n.
🛠️ Proposed fix
- <img
- src={careConfig.mainLogo?.dark}
- alt="Care Logo"
+ <img
+ src={careConfig.mainLogo?.dark}
+ alt={t("care_logo")}
className="h-10 w-auto object-contain ml-6"
/>As per coding guidelines, all UI strings should go through i18next.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <img | |
| src={careConfig.mainLogo?.dark} | |
| alt="Care Logo" | |
| className="h-10 w-auto object-contain ml-6" | |
| <img | |
| src={careConfig.mainLogo?.dark} | |
| alt={t("care_logo")} | |
| className="h-10 w-auto object-contain ml-6" | |
| /> |
🤖 Prompt for AI Agents
In `@src/pages/Facility/services/diagnosticReports/DiagnosticReportMultiPrint.tsx`
around lines 274 - 277, The alt text "Care Logo" in
DiagnosticReportMultiPrint.tsx should be localized: import and use the i18next
hook (useTranslation) or t prop in the DiagnosticReportMultiPrint component and
replace the literal alt with a translated key (e.g., t('care.logo') or a
context-specific key like t('diagnosticReport.careLogo')), and add the
corresponding translation entries to your i18n resource files so the logo alt
text is translated across locales.
| <div className="grid grid-cols-[6rem_auto_1fr] items-center"> | ||
| <span className="text-gray-600">{t("report_date")}</span> | ||
| <span className="text-gray-600">:</span> | ||
| <span className="font-semibold ml-2"> | ||
| {format(new Date(), "dd-MM-yyyy")} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
report_date currently shows today’s date.
The label implies a report date, but the value is new Date(). For multi-report prints, use a report-derived date or rename the label to a print date to avoid confusion.
🛠️ Example adjustment (use a report date fallback)
- <span className="font-semibold ml-2">
- {format(new Date(), "dd-MM-yyyy")}
- </span>
+ <span className="font-semibold ml-2">
+ {format(
+ new Date(reports[0]?.created_date ?? Date.now()),
+ "dd-MM-yyyy",
+ )}
+ </span>🤖 Prompt for AI Agents
In `@src/pages/Facility/services/diagnosticReports/DiagnosticReportMultiPrint.tsx`
around lines 327 - 333, The displayed date in DiagnosticReportMultiPrint
currently uses format(new Date(), "dd-MM-yyyy") while the label calls
t("report_date"); update the component to use the report-derived date (e.g., the
report object's date field used elsewhere in this component such as report.date,
report.reportedAt, selectedReport?.date, or reports[0]?.date) and fall back to
new Date() only if the report date is missing; alternatively, if you intend to
show the print time instead, change the label call from t("report_date") to
t("print_date") (and update translations) and keep format(new Date(), ...).
Ensure you replace the specific expression format(new Date(), "dd-MM-yyyy")
accordingly in DiagnosticReportMultiPrint.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/pages/Facility/services/diagnosticReports/DiagnosticReportMultiPrint.tsx`:
- Around line 208-225: The extension checks in DiagnosticReportMultiPrint
(within the pdfFiles and imageFiles filters) are duplicated and verbose; extract
reusable constants like IMAGE_EXTENSIONS = ["jpg","jpeg","png","gif","webp"] and
PDF_EXTENSIONS = ["pdf"] and replace the repeated
.extension.toLowerCase().endsWith(...) logic with a small helper (e.g.,
isValidFile(file, fileUrls) and matchesExtension(file.extension,
IMAGE_EXTENSIONS/PDF_EXTENSIONS)) so both filters use the same validation and
extension-matching logic (referencing pdfFiles, imageFiles, file.extension, and
fileUrls to locate the code).
- Around line 370-373: The date rendering in DiagnosticReportMultiPrint uses a
hardcoded pattern format(new Date(report.created_date), "dd-MM-yyyy") which is
not locale-aware; change it to use a locale-aware formatter (either
Intl.DateTimeFormat or date-fns' localized tokens like format(date, "P", {
locale }) with an injected/derived locale) and pass the current locale when
formatting report.created_date so dates reflect user locale preferences; update
the code that calls format(new Date(report.created_date), ...) to use the chosen
locale-aware call and import/provide the corresponding locale variable.
♻️ Duplicate comments (6)
src/pages/Facility/services/diagnosticReports/DiagnosticReportMultiPrint.tsx (6)
113-113: Useunknowninstead ofanyfor type safety.This bypasses TypeScript's strict type checking, violating coding guidelines. As per coding guidelines, avoid explicit
anytype in TypeScript.🛠️ Proposed fix
- })({} as any); + })({} as unknown);
146-168: Wrap the URL fetching logic in a try-catch for resilience.While individual
getFileUrlcalls handle errors, an unexpected error (e.g., iterator failure) could leaveisLoadingUrlsstuck astrueand never update state.🛠️ Proposed fix
const fetchAllUrls = async () => { setIsLoadingUrls(true); const urls: Record<string, string> = {}; + try { for (const [reportId, files] of filesByReport) { for (const file of files) { if (!file.id) continue; // Skip if already fetched if (fetchedFileIdsRef.current.has(file.id)) continue; const url = await getFileUrl(file, reportId); if (url) { urls[file.id] = url; fetchedFileIdsRef.current.add(file.id); } } } + } catch (error) { + console.error("Error fetching file URLs batch:", error); + } setFileUrls((prev) => ({ ...prev, ...urls })); setIsLoadingUrls(false); };
182-184: i18next returns the key itself when translation is missing, breaking the fallback chain.The expression
t(categoryCode) || t("uncategorized")won't fall back as expected becauset()returns the key string (truthy) when no translation exists.🛠️ Proposed fix
- // Use category.display for display name, fallback to translated category code - const categoryDisplay = - report.category?.display || t(categoryCode) || t("uncategorized"); + // Use category.display for display name, fallback to translated category code, + // and finally to the "uncategorized" translation if no specific translation exists. + const translatedCategory = t(categoryCode, { defaultValue: "" }); + const categoryDisplay = + report.category?.display || + (translatedCategory ? translatedCategory : t("uncategorized"));
198-198: Use optional chaining with a guard instead of non-null assertion.The
categoryMap.get(categoryCode)!uses a non-null assertion that could fail if the map entry isn't present.🛠️ Proposed fix
- categoryMap.get(categoryCode)!.reports.push(report); + const existingGroup = categoryMap.get(categoryCode); + if (existingGroup) { + existingGroup.reports.push(report); + }
274-277: Localize the logo alt text for i18n compliance.The literal
"Care Logo"should use the translation function. As per coding guidelines, all literal strings must use i18next for multi-language support.🛠️ Proposed fix
<img src={careConfig.mainLogo?.dark} - alt="Care Logo" + alt={t("care_logo")} className="h-10 w-auto object-contain ml-6" />
405-421: Non-null assertions onfile.idare redundant but risky.The files are filtered to have IDs in
getReportFiles, but usingfile.id!still bypasses type safety. Consider asserting the type after filtering.🛠️ Proposed fix
- {pdfFiles.map((file) => ( - <div key={`pdf-${file.id}`}> - <PDFRenderer fileUrl={fileUrls[file.id!]} /> + {pdfFiles.map((file) => { + const fileId = file.id; + if (!fileId) return null; + return ( + <div key={`pdf-${fileId}`}> + <PDFRenderer fileUrl={fileUrls[fileId]} /> + </div> + ); + })} ... - {imageFiles.map((file) => ( - <div key={`img-${file.id}`}> - <ImageRenderer - fileUrl={fileUrls[file.id!]} - fileName={file.name} - /> + {imageFiles.map((file) => { + const fileId = file.id; + if (!fileId) return null; + return ( + <div key={`img-${fileId}`}> + <ImageRenderer + fileUrl={fileUrls[fileId]} + fileName={file.name} + /> + </div> + ); + })}
| const pdfFiles = files.filter((file) => { | ||
| if (!file.id || !fileUrls[file.id] || !file.extension || file.is_archived) | ||
| return false; | ||
| return file.extension.toLowerCase().endsWith("pdf"); | ||
| }); | ||
|
|
||
| const imageFiles = files.filter((file) => { | ||
| if (!file.id || !fileUrls[file.id] || !file.extension || file.is_archived) | ||
| return false; | ||
| const ext = file.extension.toLowerCase(); | ||
| return ( | ||
| ext.endsWith("jpg") || | ||
| ext.endsWith("jpeg") || | ||
| ext.endsWith("png") || | ||
| ext.endsWith("gif") || | ||
| ext.endsWith("webp") | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract file extension patterns into a constant for maintainability.
The image extension checks are verbose and duplicated in pattern. Consider consolidating for clarity.
🛠️ Suggested refactor
+ const IMAGE_EXTENSIONS = ["jpg", "jpeg", "png", "gif", "webp"];
+
+ const isValidFile = (file: FileReadMinimal): boolean => {
+ return !!(file.id && fileUrls[file.id] && file.extension && !file.is_archived);
+ };
+
// Helper to get PDF and image files for a report
const getReportFiles = (reportId: string) => {
const files = filesByReport.get(reportId) || [];
- const pdfFiles = files.filter((file) => {
- if (!file.id || !fileUrls[file.id] || !file.extension || file.is_archived)
- return false;
- return file.extension.toLowerCase().endsWith("pdf");
- });
-
- const imageFiles = files.filter((file) => {
- if (!file.id || !fileUrls[file.id] || !file.extension || file.is_archived)
- return false;
- const ext = file.extension.toLowerCase();
- return (
- ext.endsWith("jpg") ||
- ext.endsWith("jpeg") ||
- ext.endsWith("png") ||
- ext.endsWith("gif") ||
- ext.endsWith("webp")
- );
- });
+ const pdfFiles = files.filter(
+ (file) => isValidFile(file) && file.extension?.toLowerCase().endsWith("pdf")
+ );
+
+ const imageFiles = files.filter(
+ (file) =>
+ isValidFile(file) &&
+ IMAGE_EXTENSIONS.some((ext) => file.extension?.toLowerCase().endsWith(ext))
+ );
return { pdfFiles, imageFiles };
};🤖 Prompt for AI Agents
In `@src/pages/Facility/services/diagnosticReports/DiagnosticReportMultiPrint.tsx`
around lines 208 - 225, The extension checks in DiagnosticReportMultiPrint
(within the pdfFiles and imageFiles filters) are duplicated and verbose; extract
reusable constants like IMAGE_EXTENSIONS = ["jpg","jpeg","png","gif","webp"] and
PDF_EXTENSIONS = ["pdf"] and replace the repeated
.extension.toLowerCase().endsWith(...) logic with a small helper (e.g.,
isValidFile(file, fileUrls) and matchesExtension(file.extension,
IMAGE_EXTENSIONS/PDF_EXTENSIONS)) so both filters use the same validation and
extension-matching logic (referencing pdfFiles, imageFiles, file.extension, and
fileUrls to locate the code).
| {format( | ||
| new Date(report.created_date), | ||
| "dd-MM-yyyy", | ||
| )} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using locale-aware date formatting.
The hardcoded "dd-MM-yyyy" format may not be appropriate for all locales. As per coding guidelines, localize date and time formats using locale-aware formatting functions.
🛠️ Suggested refactor
- {format(
- new Date(report.created_date),
- "dd-MM-yyyy",
- )}
+ {format(
+ new Date(report.created_date),
+ "P", // Locale-aware short date format
+ )}date-fns format locale aware date patterns
🤖 Prompt for AI Agents
In `@src/pages/Facility/services/diagnosticReports/DiagnosticReportMultiPrint.tsx`
around lines 370 - 373, The date rendering in DiagnosticReportMultiPrint uses a
hardcoded pattern format(new Date(report.created_date), "dd-MM-yyyy") which is
not locale-aware; change it to use a locale-aware formatter (either
Intl.DateTimeFormat or date-fns' localized tokens like format(date, "P", {
locale }) with an injected/derived locale) and pass the current locale when
formatting report.created_date so dates reflect user locale preferences; update
the code that calls format(new Date(report.created_date), ...) to use the chosen
locale-aware call and import/provide the corresponding locale variable.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/pages/Facility/services/diagnosticReports/components/DiagnosticReportPrintSheet.tsx:1
- The translation lookup
t(group.category.code)assumes that category codes are valid translation keys. Consider usinggroup.category.displaydirectly or providing a fallback to handle cases where the category code is not a valid translation key, similar to how it's handled in the grouping logic at line 184.
import { navigate } from "raviger";
| // Check if we've already fetched these files | ||
| const currentFileIds = allFileIds.split(",").filter(Boolean); | ||
| const needsFetch = currentFileIds.some( | ||
| (id) => !fetchedFileIdsRef.current.has(id), | ||
| ); | ||
| if (!needsFetch) return; | ||
|
|
||
| const fetchAllUrls = async () => { | ||
| setIsLoadingUrls(true); |
There was a problem hiding this comment.
The setIsLoadingUrls(true) call should be placed before the early return checks to ensure loading state is set even if the function returns early. Currently, if needsFetch is false, the loading state remains unchanged, which could cause UI inconsistencies.
| // Check if we've already fetched these files | |
| const currentFileIds = allFileIds.split(",").filter(Boolean); | |
| const needsFetch = currentFileIds.some( | |
| (id) => !fetchedFileIdsRef.current.has(id), | |
| ); | |
| if (!needsFetch) return; | |
| const fetchAllUrls = async () => { | |
| setIsLoadingUrls(true); | |
| // Begin URL loading phase | |
| setIsLoadingUrls(true); | |
| // Check if we've already fetched these files | |
| const currentFileIds = allFileIds.split(",").filter(Boolean); | |
| const needsFetch = currentFileIds.some( | |
| (id) => !fetchedFileIdsRef.current.has(id), | |
| ); | |
| if (!needsFetch) { | |
| setIsLoadingUrls(false); | |
| return; | |
| } | |
| const fetchAllUrls = async () => { |
| const data = await query(fileApi.get, { | ||
| queryParams: { | ||
| file_type: "diagnostic_report", | ||
| associating_id: reportId, | ||
| }, | ||
| pathParams: { fileId: file.id }, | ||
| })({} as any); |
There was a problem hiding this comment.
Using {} as any bypasses TypeScript's type safety. Consider defining the proper type for the query function's parameter or using a more specific type assertion.
| const data = await query(fileApi.get, { | |
| queryParams: { | |
| file_type: "diagnostic_report", | |
| associating_id: reportId, | |
| }, | |
| pathParams: { fileId: file.id }, | |
| })({} as any); | |
| const getFile = query(fileApi.get, { | |
| queryParams: { | |
| file_type: "diagnostic_report", | |
| associating_id: reportId, | |
| }, | |
| pathParams: { fileId: file.id }, | |
| }); | |
| type GetFileParams = Parameters<typeof getFile>[0]; | |
| const data = await getFile({} as GetFileParams); |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsx (5)
61-79: Avoid{} as any- use proper typing or a different data fetching approach.The
queryutility is designed to be used withuseQuery'squeryFn, not called imperatively. Casting toanybypasses TypeScript safety and is explicitly discouraged by the coding guidelines.Consider using React Query's
queryClient.fetchQueryor restructuring to fetch URLs within a proper query context:Suggested approach using queryClient
+import { useQueryClient } from "@tanstack/react-query"; // Inside component: +const queryClient = useQueryClient(); const getFileUrl = async (file: FileReadMinimal) => { if (!file.id || !report?.id) return null; try { - const data = await query(fileApi.get, { - queryParams: { - file_type: "diagnostic_report", - associating_id: report.id, - }, - pathParams: { fileId: file.id }, - })({} as any); + const data = await queryClient.fetchQuery({ + queryKey: ["file", file.id, report.id], + queryFn: query(fileApi.get, { + queryParams: { + file_type: "diagnostic_report", + associating_id: report.id, + }, + pathParams: { fileId: file.id }, + }), + }); return data?.read_signed_url as string; } catch (error) { console.error("Error fetching signed URL:", error); return null; } };
85-103: Fetch signed URLs in parallel for better performance.The current sequential approach with
for...ofandawaitwill be slow when multiple files exist. With a limit of 100 files, this could result in 100 sequential network requests.Refactor to use Promise.all for parallel fetching
useEffect(() => { if (!files.results.length) return; const fetchAllUrls = async () => { - const urls: Record<string, string> = {}; - - for (const file of files.results) { - if (!file.id) continue; - const url = await getFileUrl(file); - if (url) { - urls[file.id] = url; - } - } - - setFileUrls(urls); + const urlEntries = await Promise.all( + files.results + .filter((file) => file.id) + .map(async (file) => { + const url = await getFileUrl(file); + return [file.id, url] as const; + }) + ); + + const urls: Record<string, string> = {}; + for (const [id, url] of urlEntries) { + if (id && url) { + urls[id] = url; + } + } + setFileUrls(urls); }; fetchAllUrls(); }, [files.results, report?.id]);
124-141: Consider extracting file type detection into a utility function.The extension checking logic is duplicated between PDF and image filtering. A helper function would improve maintainability and handle edge cases (e.g., extensions with/without dots, case variations).
Optional: Extract file type helpers
const isPdfFile = (file: FileReadMinimal): boolean => { const ext = file.extension?.toLowerCase().replace(/^\./, "") ?? ""; return ext === "pdf"; }; const isImageFile = (file: FileReadMinimal): boolean => { const ext = file.extension?.toLowerCase().replace(/^\./, "") ?? ""; return ["jpg", "jpeg", "png", "gif", "webp"].includes(ext); }; const isValidFile = (file: FileReadMinimal, fileUrls: Record<string, string>): boolean => !!file.id && !!fileUrls[file.id] && !!file.extension && !file.is_archived; // Usage: const pdfFiles = files.results.filter((f) => isValidFile(f, fileUrls) && isPdfFile(f)); const imageFiles = files.results.filter((f) => isValidFile(f, fileUrls) && isImageFile(f));
166-170: Internationalize the alt text for accessibility.The alt text "Care Logo" should use i18n for multi-language support as per coding guidelines.
<img src={careConfig.mainLogo?.dark} - alt="Care Logo" + alt={t("care_logo")} className="h-10 w-auto object-contain ml-6" />
229-232: Consider locale-aware date formatting for medical timestamps.The fixed
"dd-MM-yyyy"format may not be appropriate for all locales. Per coding guidelines, medical timestamps should use locale-aware formatting.Use locale-aware date formatting
+import { useLocale } from "@/hooks/useLocale"; // or appropriate locale hook // In component: +const { locale } = useLocale(); <span className="font-semibold ml-2"> {report.created_date && - format(new Date(report.created_date), "dd-MM-yyyy")} + format(new Date(report.created_date), "P", { locale })} </span>Alternatively, if a specific format is required for medical/regulatory compliance, document that requirement in a comment.
src/pages/Encounters/tabs/diagnostic-reports.tsx (1)
348-369: Add adata-cyhook for the new multi-print CTA.This helps E2E coverage for the new workflow. As per coding guidelines, add a page-level
data-cyattribute for new controls.♻️ Suggested change
- <Button variant="outline" size="sm" className="w-full"> + <Button + variant="outline" + size="sm" + className="w-full" + data-cy="diagnostic-reports-multi-print" + >
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@public/locale/en.json`:
- Line 3288: Choose a single phrasing (either "Multi-Print" or "Print Multiple")
and update the locale entry "multi_print" to that phrasing, then search the same
en.json for any other occurrences of "Multi-Print" or "Print Multiple"
(including the other instance noted) and make them consistent with the chosen
term so the workflow uses a single standardized label.
| "moving": "Moving", | ||
| "moving_camera": "Moving Camera", | ||
| "mrp": "MRP", | ||
| "multi_print": "Multi-Print", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align “Multi-Print” vs “Print Multiple” wording.
If both appear in the same workflow, consider standardizing to one phrasing to avoid mixed terminology.
📝 Optional consistency tweak
- "multi_print": "Multi-Print",
+ "multi_print": "Print Multiple",Also applies to: 4220-4220
🤖 Prompt for AI Agents
In `@public/locale/en.json` at line 3288, Choose a single phrasing (either
"Multi-Print" or "Print Multiple") and update the locale entry "multi_print" to
that phrasing, then search the same en.json for any other occurrences of
"Multi-Print" or "Print Multiple" (including the other instance noted) and make
them consistent with the chosen term so the workflow uses a single standardized
label.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Proposed Changes
Fixes #15301
Recording.2026-01-24.163026.mp4
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Localization
UI
✏️ Tip: You can customize this high-level summary in your review settings.