#1588 - Download Report Functionality using existing components #1743
Conversation
Merging download report functionality into develop
Used existing component to download report
WalkthroughThe pull request introduces a new dependency for PDF generation and implements a comprehensive reporting feature. A new React component, ReportDownloadButton, is added to allow users to select a report format (HTML or PDF) and trigger downloads. The MonitorStatusHeader component is updated to accept children and now imports the download button. In addition, new report-related utilities—including a download function, a ProductReport component, associated CSS styles, and utility functions for calculating monitoring statistics—have been implemented. A new date formatting function is also added in the time utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as ReportDownloadButton
participant N as NetworkService
participant D as downloadReport
participant H as html2pdf
U->>R: Select report format (HTML/PDF) and click "Download"
R->>N: Fetch monitor statistics (getStatsByMonitorId)
N-->>R: Return monitor data
R->>D: Invoke downloadReport(monitorData, format, certExpiry)
alt HTML Format
D-->>U: Trigger HTML file download via Blob and anchor element
else PDF Format
D->>H: Convert HTML content to PDF
H-->>U: Trigger PDF download
end
D->>R: Clean up and resolve promise
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 Core Changes
- Primary purpose and scope: Add functionality to download reports in PDF/HTML format using existing components.
- Key components modified:
ReportDownloadButton: New component for downloading reports.MonitorStatusHeader: Integration point for the new download button.useReportFetch: Custom hook for fetching report data.downloadReport: Utility function for generating and downloading reports.ProductReport: Component for rendering the report content.
- Cross-component impacts: Integration with existing monitoring and UI components.
- Business value alignment: Enhances user experience by providing downloadable reports, which is valuable for monitoring and analysis.
2. Deep Technical Analysis
2.1 Code Logic Analysis
Client/src/Components/MonitorStatusHeader/ReportDownloadButton/index.jsx - ReportDownloadButton
- Submitted PR Code:
import { useState } from "react";
import { Box, Button } from "@mui/material";
import { useTheme } from '@mui/material/styles';
import FileDownloadIcon from '@mui/icons-material/FileDownload';
import Select from "../../Inputs/Select";
import { downloadReport } from "../../../Utils/Report/downloadReport";
import PropTypes from 'prop-types';
const options = [
{ _id: "html", name: "HTML Report" },
{ _id: "pdf", name: "Pdf Report" },
];
const ReportDownloadButton = ({ shouldRender, monitor, certificateExpiry }) => {
const [downloadFormat, setDownloadFormat] = useState(options[1]._id);
const theme = useTheme();
if (!shouldRender) return null;
if(!monitor) return null;
const handleDownload = async () => {
try {
console.log(monitor);
await downloadReport({
monitorData: monitor,
downloadFormat,
certificateExpiry,
});
} finally {
setDownloadFormat(options[1]._id);
}
};
return (
<Box alignSelf="flex-end" display="flex" alignItems="center">
<Select
id="report-format-select"
value={downloadFormat}
onChange={(e) => setDownloadFormat(e.target.value)}
items={options}
/>
<Button
variant="contained"
color="secondary"
onClick={handleDownload}
disabled={!downloadFormat}
sx={{
px: theme.spacing(5),
"& svg": {
mr: theme.spacing(3),
"& path": {
/* Should always be contrastText for the button color */
stroke: theme.palette.secondary.contrastText,
},
},
}}
>
<FileDownloadIcon />
</Button>
</Box>
);
};
ReportDownloadButton.propTypes = {
shouldRender: PropTypes.bool,
monitor: PropTypes.object,
certificateExpiry: PropTypes.string,
};
export default ReportDownloadButton;- Analysis:
- Current logic and potential issues:
- The component correctly handles the rendering of the download button and the selection of the report format.
- The
handleDownloadfunction correctly calls thedownloadReportutility. - The component ensures that the button is disabled if no format is selected.
- Edge cases and error handling:
- The component handles the case where
shouldRenderormonitorisnull. - The
finallyblock resets the download format, which is a good practice.
- The component handles the case where
- Cross-component impact :
- The component integrates well with the
MonitorStatusHeaderand uses existing utility functions.
- The component integrates well with the
- Business logic considerations :
- The component aligns with the business requirement of providing downloadable reports.
- Current logic and potential issues:
Client/src/Utils/Report/downloadReport.jsx - downloadReport
- Submitted PR Code:
import { createRoot } from "react-dom/client";
import { ProductReport } from "./report";
import html2pdf from "html2pdf.js";
export const downloadReport = ({ monitorData, downloadFormat, certificateExpiry }) => {
const tempDiv = document.createElement("div");
tempDiv.style.position = "absolute";
tempDiv.style.left = "-9999px";
document.body.appendChild(tempDiv);
return new Promise((resolve) => {
const reportRoot = createRoot(tempDiv);
reportRoot.render(
<ProductReport
monitorData={monitorData}
certificateExpiry={certificateExpiry}
/>
);
setTimeout(() => {
try {
const htmlContent = `
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Product Report</title>
</head>
<body>
${tempDiv.innerHTML}
</body>
</html>`;
if (downloadFormat === "html") {
const blob = new Blob([htmlContent], { type: "text/html" });
const url = URL.createObjectURL(blob);
const a = document.createElement("a");
a.href = url;
a.download = `${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.html`;
document.body.appendChild(a);
a.click();
document.body.removeChild(a);
URL.revokeObjectURL(url);
} else if (downloadFormat === "pdf") {
html2pdf().from(htmlContent).save(`${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.pdf`);
}
} finally {
reportRoot.unmount();
document.body.removeChild(tempDiv);
resolve();
}
}, 100);
});
};- Analysis:
- Current logic and potential issues:
- The function creates a temporary div to render the report content.
- It uses
html2pdfto generate the PDF and handles the HTML download using a blob. - The
setTimeoutensures that styles are applied before generating the report.
- Edge cases and error handling:
- The function handles both HTML and PDF formats.
- The
finallyblock ensures that the temporary div is removed and the root is unmounted.
- Cross-component impact :
- The function integrates well with the
ProductReportcomponent.
- The function integrates well with the
- Business logic considerations :
- The function aligns with the business requirement of providing downloadable reports in both HTML and PDF formats.
- Current logic and potential issues:
2.2 Implementation Quality
-
Code Structure:
- Organization and modularity:
- The code is well-organized into separate components and utility functions.
- The use of custom hooks (
useReportFetch) and utility functions (downloadReport) enhances modularity.
- Design pattern adherence:
- The code follows React best practices, including the use of hooks and functional components.
- Reusability aspects:
- The
downloadReportfunction andProductReportcomponent can be reused for other report generation tasks.
- The
- Maintainability factors:
- The code is maintainable due to its modular structure and clear separation of concerns.
- Organization and modularity:
-
Error Handling:
- Exception scenarios coverage:
- The code handles exceptions in network requests and ensures that resources are cleaned up in
finallyblocks.
- The code handles exceptions in network requests and ensures that resources are cleaned up in
- Recovery mechanisms:
- The code includes recovery mechanisms such as resetting the download format and removing temporary elements.
- Logging and monitoring:
- The code includes basic logging (
console.log), but more advanced logging and monitoring could be added.
- The code includes basic logging (
- User experience impact:
- The code ensures a good user experience by handling edge cases and providing feedback through the UI.
- Exception scenarios coverage:
-
Performance Considerations:
- Resource utilization:
- The code efficiently utilizes resources by creating temporary elements and cleaning them up.
- Scalability aspects:
- The code is scalable due to its modular structure and use of hooks.
- Bottleneck analysis:
- Potential bottlenecks include the rendering of large reports, which could be optimized further.
- Optimization opportunities:
- The
setTimeoutdelay could be adjusted based on the complexity of the report to improve performance.
- The
- Resource utilization:
3. Risk Assessment
3.1 Critical Issues
🔴 P0 (Must Fix):
- Issue: Potential security risk with
html2pdf - Impact:
- Technical implications: The use of
html2pdfcould introduce security risks, such as XSS attacks, if not properly sanitized. - Business consequences: Compromised user data and loss of trust.
- User experience effects: Potential security breaches affecting user confidence.
- Technical implications: The use of
- Resolution:
- Specific code changes: Ensure that all input data is properly sanitized before rendering.
- Configuration updates: Consider using a more secure library or adding sanitization steps.
- Testing requirements: Conduct thorough security testing, including penetration testing.
3.2 Important Improvements
🟡 P1 (Should Fix):
- Issue: Lack of comprehensive error handling in
downloadReport - Current Impact:
- Performance implications: Errors during report generation are not handled gracefully.
- Maintenance overhead: Difficult to debug and maintain without proper error handling.
- Future scalability: May lead to unexpected behaviors as the application scales.
- Suggested Solution:
- Implementation approach: Add comprehensive error handling and logging in the
downloadReportfunction. - Migration strategy: Gradually introduce error handling and logging in critical sections.
- Testing considerations: Write unit tests to cover various error scenarios.
- Implementation approach: Add comprehensive error handling and logging in the
3.3 Minor Suggestions
🟢 P2 (Consider):
- Area: Code documentation
- Improvement Opportunity:
- Code quality enhancement: Add detailed comments and documentation to explain the purpose and functionality of each component and function.
- Best practice alignment: Follow best practices for code documentation to improve maintainability.
- Documentation updates: Ensure that all new components and functions are well-documented.
4. Requirements Analysis
4.1 Functional Coverage
- Requirements mapping:
- Implemented features: Download report functionality in PDF/HTML format.
- Missing elements: None identified.
- Edge cases handling: The code handles edge cases such as null values and format selection.
- Business Logic:
- Use case coverage: The code covers the use case of downloading reports with necessary stats.
- Business rule implementation: The business rules for report generation are correctly implemented.
- Data flow correctness: The data flow from fetching report data to generating the report is correct.
4.2 Non-functional Aspects
- Performance metrics: The code performs well for generating reports, but further optimization could be considered.
- Security considerations: Potential security risks with
html2pdfneed to be addressed. - Scalability factors: The code is scalable due to its modular structure.
- Maintainability aspects: The code is maintainable due to its clear structure and separation of concerns.
5. Testing Strategy
- Test Coverage:
- Unit test requirements: Write unit tests for the
ReportDownloadButtoncomponent,downloadReportfunction, anduseReportFetchhook. - Integration test scenarios: Test the integration of the download button with the
MonitorStatusHeadercomponent. - Edge case validation: Validate edge cases such as null values and format selection.
- Unit test requirements: Write unit tests for the
- Quality Metrics:
- Current coverage: The code includes basic testing and self-review.
- Critical paths: Ensure that all critical paths, such as report generation and download, are thoroughly tested.
- Performance benchmarks: Conduct performance benchmarks to ensure that report generation is efficient.
6. Final Assessment
6.1 Key Action Items
-
Critical Changes (P0):
- Address potential security risks with
html2pdf.
- Address potential security risks with
-
Important Improvements (P1):
- Add comprehensive error handling and logging in the
downloadReportfunction.
- Add comprehensive error handling and logging in the
-
Suggested Enhancements (P2):
- Improve code documentation and comments.
6.2 Overall Evaluation
- Technical assessment: The code is technically sound but needs improvements in security and error handling.
- Business impact: The new functionality aligns well with business requirements and enhances user experience.
- Risk evaluation: There are critical security risks that need to be addressed.
- Implementation quality: The code is well-implemented but can be further optimized and documented.
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
There was a problem hiding this comment.
Hi @DMJain , thanks for contributing to the project!
There are some minor changes that can be made to reduce some front end complexity, but my main concern with this PR is that the calculation of stats for the report are done on the frontend.
This is something that should be handled on the backend, preferrably at the database layer. MongoDBs aggregation pipeline should be sufficient to build whatever data you need.
The button also looks a bit odd in dark mode:
@gorkem-bwl can you provide guidance for the design aspect for us? Thank you!
| document.body.appendChild(tempDiv); | ||
|
|
||
| // Create a promise to handle the rendering and download | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
Any particular reason to not use async/await pattern?
There was a problem hiding this comment.
no particular reason. async/await can be used but using Promises helped me using setTimeout and render of element properly
| ); | ||
|
|
||
| // Wait for styles to be applied | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Can you explain why a timeout is needed here? I see in your comment you need to wait for styles to be applied, but why is that?
There was a problem hiding this comment.
I tried without setTimeout and frontend calculation, for some reason blank html/pdf report were getting download. not able to figure out why and waiting even 50ms was helping. so kept setTimeout
There was a problem hiding this comment.
I found the reason. setTimeout is used because of how react renders document and how browser engine renders the DOM. it's not instantaneous and synchronous. using setTimeout with 0ms also works as it let react and browser engine to complete these asynchronous task before executing our download function.
'//Wait for Styles to be applied' is wrong comment and I will remove it. I was trying to use theme previously and it was not working without setTimeOut. so i thought react is taking time to apply the styles. it's mistake from my side. but we still need setTimeout.
![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
♻️ Duplicate comments (1)
Client/src/Pages/Uptime/Details/index.jsx (1)
73-77:⚠️ Potential issueMove report data fetching to on-demand.
The report data is being fetched immediately when the component mounts, even if the user never downloads a report. This is inefficient.
Consider moving the data fetching to the download button click handler:
- const [reportData, reportIsLoading, reportNetworkError] = useReportFetch({ - authToken, - monitorId, - dateRange, - }); + const fetchReportData = async () => { + const [data, , error] = await useReportFetch({ + authToken, + monitorId, + dateRange, + }); + if (error) { + // Handle error + return; + } + // Proceed with download + };
🧹 Nitpick comments (9)
Client/src/Pages/Uptime/Details/Hooks/useReportFetch.jsx (1)
22-25: Enhance error handling with specific error typesThe current error handling is generic. Consider handling specific error types (e.g., network errors, authentication errors) differently.
} catch (error) { + if (error.response?.status === 401) { + createToast({ body: "Authentication failed. Please log in again." }); + } else if (error.response?.status === 404) { + createToast({ body: "Report data not found." }); + } else { setReportNetworkError(true); createToast({ body: error.message }); + } }Client/src/Utils/Report/reportUtils.js (2)
1-14: Optimize downtime calculation with reduceThe downtime calculation can be more concise using reduce.
export const calculateDowntimeCount = (checks) => { if (!checks || checks.length < 2) return 0; - let downtimeCount = 0; - for (let i = 0; i < checks.length - 1; i++) { - const currentCheck = checks[i]; - const nextCheck = checks[i + 1]; - if (currentCheck.status === true && nextCheck.status === false) { - downtimeCount++; - } - } - return downtimeCount; + return checks.slice(0, -1).reduce((count, check, i) => + count + (check.status && !checks[i + 1].status ? 1 : 0), 0); };
16-33: Consider using Array.reduce for ping stats calculationThe ping stats calculation can be simplified using a single reduce operation.
export const calculatePingStats = (checks) => { if (!checks || checks.length === 0) return { min: 0, max: 0 }; - let min = Number.MAX_SAFE_INTEGER; - let max = 0; - checks.forEach((check) => { - if (check.originalResponseTime) { - min = Math.min(min, check.originalResponseTime); - max = Math.max(max, check.originalResponseTime); - } - }); + const stats = checks.reduce((acc, check) => { + if (check.originalResponseTime) { + return { + min: Math.min(acc.min, check.originalResponseTime), + max: Math.max(acc.max, check.originalResponseTime) + }; + } + return acc; + }, { min: Number.MAX_SAFE_INTEGER, max: 0 }); return { - min: min === Number.MAX_SAFE_INTEGER ? 0 : min, - max: max, + min: stats.min === Number.MAX_SAFE_INTEGER ? 0 : stats.min, + max: stats.max }; };Client/src/Components/MonitorStatusHeader/ReportDownloadButton/index.jsx (1)
9-12: Use constants for default format selectionInstead of using array index for default format, use a named constant.
+const DEFAULT_FORMAT = "pdf"; const options = [ { _id: "html", name: "HTML Report" }, { _id: "pdf", name: "Pdf Report" }, ]; -const [downloadFormat, setDownloadFormat] = useState(options[1]._id); +const [downloadFormat, setDownloadFormat] = useState(DEFAULT_FORMAT);Also applies to: 15-15
Client/src/Utils/Report/downloadReport.jsx (2)
5-10: Consider using React portals instead of direct DOM manipulation.While the temporary div approach works, React portals provide a more idiomatic way to render elements outside the normal DOM hierarchy.
- const tempDiv = document.createElement("div"); - tempDiv.style.position = "absolute"; - tempDiv.style.left = "-9999px"; - document.body.appendChild(tempDiv); + const portalContainer = document.createElement("div"); + portalContainer.style.position = "absolute"; + portalContainer.style.left = "-9999px"; + document.body.appendChild(portalContainer); + const portal = createPortal( + <ProductReport + monitorData={monitorData} + certificateExpiry={certificateExpiry} + />, + portalContainer + );
23-24: Optimize setTimeout delay.Based on previous discussions, a 0ms timeout is sufficient for the React rendering cycle. The current 100ms delay is unnecessary.
- setTimeout(() => { + // Allow React rendering cycle to complete + setTimeout(() => {Client/src/Utils/Report/report.jsx (2)
59-73: Extract ping formatting logic into a utility function.The ping formatting logic is duplicated across minimum and maximum ping displays. Consider extracting this into a reusable utility function.
+const formatPing = (value) => { + return value > 2000 ? `${(value / 1000).toFixed(2)}s` : `${value}ms`; +}; // Then use it like: -{pingStats.min > 2000 - ? (pingStats.min / 1000).toFixed(2) + "s" - : pingStats.min + "ms"} +{formatPing(pingStats.min)}
120-123: Enhance PropTypes validation for monitorData.The current PropTypes validation for monitorData is too generic. Consider defining a more specific shape to catch potential issues early.
ProductReport.propTypes = { - monitorData: PropTypes.object.isRequired, + monitorData: PropTypes.shape({ + name: PropTypes.string.isRequired, + url: PropTypes.string.isRequired, + createdAt: PropTypes.string.isRequired, + checks: PropTypes.array.isRequired, + periodUptime: PropTypes.number.isRequired, + periodAvgResponseTime: PropTypes.number.isRequired, + periodIncidents: PropTypes.number.isRequired, + periodTotalChecks: PropTypes.number.isRequired, + interval: PropTypes.number.isRequired, + isActive: PropTypes.bool.isRequired, + uptimeDuration: PropTypes.number.isRequired + }).isRequired, certificateExpiry: PropTypes.string.isRequired, };Client/src/Utils/Report/reportStyles.css (1)
1-20: Let's address the elephant in the room about CSS usage.I noticed the past discussion about using MUI theme instead of CSS. While the current implementation works, we should consider migrating to MUI styled components for consistent theming. However, I understand the challenge with PDF generation outside ThemeProvider.
Here's a potential solution:
- Create a separate theme instance for PDF generation
- Use MUI's createTheme and ThemeProvider specifically for the report component
- Export the styles as styled components
Would you like me to help create a proof of concept for this approach?
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-10: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
Client/package.json(1 hunks)Client/src/Components/MonitorStatusHeader/ReportDownloadButton/index.jsx(1 hunks)Client/src/Components/MonitorStatusHeader/index.jsx(3 hunks)Client/src/Pages/Uptime/Details/Hooks/useReportFetch.jsx(1 hunks)Client/src/Pages/Uptime/Details/index.jsx(4 hunks)Client/src/Utils/Report/downloadReport.jsx(1 hunks)Client/src/Utils/Report/report.jsx(1 hunks)Client/src/Utils/Report/reportStyles.css(1 hunks)Client/src/Utils/Report/reportUtils.js(1 hunks)Client/src/Utils/timeUtils.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
Client/src/Utils/Report/reportStyles.css
[error] 8-10: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (2)
Client/src/Pages/Uptime/Details/Hooks/useReportFetch.jsx (1)
21-21: Use undefined as fallback value instead of empty objectBased on the team's convention from past reviews, we should use
undefinedas the fallback value for empty data.-setReportData(res?.data?.data ?? {}); +setReportData(res?.data?.data ?? undefined);Client/src/Components/MonitorStatusHeader/index.jsx (1)
12-58: 🛠️ Refactor suggestionSplit component into variants as suggested
The component is becoming too complex with multiple responsibilities. As suggested in past reviews, let's split it into variants.
Create separate variants:
// HeaderBase.jsx export const HeaderBase = ({ monitor, theme, statusColor, statusMsg }) => ( <Stack direction="row" justifyContent="space-between"> {/* Base header content */} </Stack> ); // HeaderWithConfig.jsx export const HeaderWithConfig = (props) => ( <HeaderBase {...props}> <ConfigButton {...props} /> </HeaderBase> ); // HeaderWithReport.jsx export const HeaderWithReport = (props) => ( <HeaderBase {...props}> <Stack direction="row" gap={theme.spacing(2)}> <ConfigButton {...props} /> <ReportDownloadButton {...props} /> </Stack> </HeaderBase> );
| } | ||
| }; | ||
| fetchMonitors(); | ||
| }, [authToken, dateRange, monitorId, navigate]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused dependency from useEffect
The navigate function is included in the dependency array but is not used within the effect.
-}, [authToken, dateRange, monitorId, navigate]);
+}, [authToken, dateRange, monitorId]);📝 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.
| }, [authToken, dateRange, monitorId, navigate]); | |
| }, [authToken, dateRange, monitorId]); |
|
|
||
| const handleDownload = async () => { | ||
| try { | ||
| console.log(monitor); |
There was a problem hiding this comment.
Remove console.log statement
Debug statement should be removed before production.
-console.log(monitor);📝 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.
| console.log(monitor); | |
| // (The console.log statement has been removed as per the review comment) |
| const handleDownload = async () => { | ||
| try { | ||
| console.log(monitor); | ||
| await downloadReport({ | ||
| monitorData: monitor, | ||
| downloadFormat, | ||
| certificateExpiry, | ||
| }); | ||
| } finally { | ||
| setDownloadFormat(options[1]._id); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling to download function
The download function should handle errors and show appropriate messages to users.
const handleDownload = async () => {
try {
- console.log(monitor);
await downloadReport({
monitorData: monitor,
downloadFormat,
certificateExpiry,
});
+ } catch (error) {
+ createToast({
+ body: "Failed to download report. Please try again.",
+ type: "error"
+ });
} finally {
setDownloadFormat(options[1]._id);
}
};📝 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.
| const handleDownload = async () => { | |
| try { | |
| console.log(monitor); | |
| await downloadReport({ | |
| monitorData: monitor, | |
| downloadFormat, | |
| certificateExpiry, | |
| }); | |
| } finally { | |
| setDownloadFormat(options[1]._id); | |
| } | |
| }; | |
| const handleDownload = async () => { | |
| try { | |
| await downloadReport({ | |
| monitorData: monitor, | |
| downloadFormat, | |
| certificateExpiry, | |
| }); | |
| } catch (error) { | |
| createToast({ | |
| body: "Failed to download report. Please try again.", | |
| type: "error" | |
| }); | |
| } finally { | |
| setDownloadFormat(options[1]._id); | |
| } | |
| }; |
| if (downloadFormat === "html") { | ||
| // Create and trigger HTML download | ||
| const blob = new Blob([htmlContent], { type: "text/html" }); | ||
| const url = URL.createObjectURL(blob); | ||
| const a = document.createElement("a"); | ||
| a.href = url; | ||
| a.download = `${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.html`; | ||
| document.body.appendChild(a); | ||
| a.click(); | ||
|
|
||
| // Cleanup | ||
| document.body.removeChild(a); | ||
| URL.revokeObjectURL(url); | ||
| } else if (downloadFormat === "pdf") { | ||
| // Create and trigger PDF download | ||
| html2pdf().from(htmlContent).save(`${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.pdf`); | ||
| } |
There was a problem hiding this comment.
Add error handling for PDF conversion and ensure proper cleanup.
The current implementation lacks error handling for PDF conversion and might leak memory if URL.revokeObjectURL isn't called in case of errors.
} else if (downloadFormat === "pdf") {
- html2pdf().from(htmlContent).save(`${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.pdf`);
+ try {
+ await html2pdf().from(htmlContent).save(`${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.pdf`);
+ } catch (error) {
+ console.error('PDF generation failed:', error);
+ throw new Error('Failed to generate PDF report');
+ }
}📝 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.
| if (downloadFormat === "html") { | |
| // Create and trigger HTML download | |
| const blob = new Blob([htmlContent], { type: "text/html" }); | |
| const url = URL.createObjectURL(blob); | |
| const a = document.createElement("a"); | |
| a.href = url; | |
| a.download = `${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.html`; | |
| document.body.appendChild(a); | |
| a.click(); | |
| // Cleanup | |
| document.body.removeChild(a); | |
| URL.revokeObjectURL(url); | |
| } else if (downloadFormat === "pdf") { | |
| // Create and trigger PDF download | |
| html2pdf().from(htmlContent).save(`${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.pdf`); | |
| } | |
| if (downloadFormat === "html") { | |
| // Create and trigger HTML download | |
| const blob = new Blob([htmlContent], { type: "text/html" }); | |
| const url = URL.createObjectURL(blob); | |
| const a = document.createElement("a"); | |
| a.href = url; | |
| a.download = `${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.html`; | |
| document.body.appendChild(a); | |
| a.click(); | |
| // Cleanup | |
| document.body.removeChild(a); | |
| URL.revokeObjectURL(url); | |
| } else if (downloadFormat === "pdf") { | |
| // Create and trigger PDF download | |
| try { | |
| await html2pdf().from(htmlContent).save(`${monitorData.name}-monitor-report-${new Date().toISOString().split("T")[0]}.pdf`); | |
| } catch (error) { | |
| console.error('PDF generation failed:', error); | |
| throw new Error('Failed to generate PDF report'); | |
| } | |
| } |
| @@ -25,6 +25,7 @@ | |||
| "axios": "^1.7.4", | |||
| "dayjs": "1.11.13", | |||
| "flag-icons": "7.3.2", | |||
| "html2pdf.js": "^0.10.2", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Yo, let's verify this package version, eh?
The package version uses a caret (^), which could lead to compatibility issues down the road. Let's make sure we're using the most stable version and check for any security concerns.
🌐 Web query:
What is the latest stable version of html2pdf.js and are there any known vulnerabilities?
💡 Result:
The latest stable version of html2pdf.js (the JavaScript library) is v0.9.3, as newer versions (v0.10.x) have reported unresolved issues[3][4]. However, npm lists v0.10.2 as technically the most recent release[3][10].
Key Security Notes:
-
JS Library Vulnerabilities:
- No direct CVEs are reported for the JavaScript
html2pdf.jspackage in the provided sources[6]. - A warning exists for versions ≥0.10 due to unspecified bugs[3][4].
- No direct CVEs are reported for the JavaScript
-
PHP Library Confusion:
- A separate PHP library (
spipu/html2pdf) had critical vulnerabilities:- CVE-2021-45394: SSRF/deserialization flaws (fixed in v5.2.4)[2][9].
- CVE-2023-39062: XSS in forms.php (fixed in v5.2.8)[5][7][8].
- A separate PHP library (
Recommendations:
- For JavaScript: Use v0.9.3 until v0.10.x issues are resolved[3][4].
- For PHP: Ensure
spipu/html2pdfis updated to ≥v5.2.8[5][7][8].
Citations:
- 1: https://www.cdnpkg.com/html2pdf.js/
- 2: https://www.synacktiv.com/sites/default/files/2022-01/html2pdf_ssrf_deserialization.pdf
- 3: https://www.npmjs.com/package/html2pdf.js/v/0.10.2
- 4: https://ekoopmans.github.io/html2pdf.js/
- 5: https://vulert.com/vuln-db/CVE-2023-39062
- 6: https://security.snyk.io/package/npm/html2pdf-fix-jspdf
- 7: https://github.com/spipu/html2pdf/releases
- 8: https://www.cvedetails.com/cve/CVE-2023-39062/
- 9: https://nvd.nist.gov/vuln/detail/CVE-2021-45394
- 10: https://www.npmjs.com/package/html2pdf.js/v/0.9.0
Yo, heads up on the html2pdf.js dependency!
- The package is set to "^0.10.2", which, although the most recent release per npm, isn’t actually the most stable version.
- External research suggests that the stable version is v0.9.3, because releases in the 0.10.x series have unresolved issues—even though there aren’t any direct CVE-reported vulnerabilities.
- To avoid potential compatibility issues down the road, consider pinning the dependency to v0.9.3 rather than relying on the caret-led upgrade to unstable versions.
| hr{ | ||
| border: 0; | ||
| border-top: 1px solid #ddd; | ||
| width: 700px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded values need to go, eh?
The hr styles contain hard-coded values for width and color. Let's make it more responsive and theme-consistent.
-hr{
- border: 0;
- border-top: 1px solid #ddd;
- width: 700px;
-}
+import { styled } from '@mui/material/styles';
+
+export const Divider = styled('hr')(({ theme }) => ({
+ border: 0,
+ borderTop: `1px solid ${theme.palette.divider}`,
+ width: '100%',
+ maxWidth: '700px',
+ margin: theme.spacing(2, 0),
+}));Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Client/src/Components/MonitorStatusHeader/ReportDownloadButton/index.jsx (2)
11-14: Yo dawg, let's move these options to a constants file!These report format options could be reused elsewhere in the app. Consider moving them to a dedicated constants file like
src/Constants/reportFormats.js.
45-68: Let's make this UI accessible like mom's recipe book!The UI looks clean, but could use some accessibility improvements:
- Add aria-label to the download button
- Add tooltip for the download icon
<Button variant="contained" color="secondary" onClick={handleDownload} disabled={!downloadFormat} + aria-label={`Download ${downloadFormat} report`} sx={{ px: theme.spacing(5), "& svg": { mr: theme.spacing(3), "& path": { stroke: theme.palette.secondary.contrastText, }, }, }} > - <FileDownloadIcon /> + <FileDownloadIcon titleAccess="Download Report" /> </Button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Client/src/Components/MonitorStatusHeader/ReportDownloadButton/index.jsx(1 hunks)Client/src/Components/MonitorStatusHeader/index.jsx(2 hunks)Client/src/Pages/Uptime/Details/index.jsx(2 hunks)Client/src/Utils/Report/downloadReport.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Client/src/Pages/Uptime/Details/index.jsx
- Client/src/Utils/Report/downloadReport.jsx
- Client/src/Components/MonitorStatusHeader/index.jsx
🔇 Additional comments (3)
Client/src/Components/MonitorStatusHeader/ReportDownloadButton/index.jsx (3)
16-20: Mom's spaghetti approves this state management!Clean implementation of component props and state management. Good choice setting PDF as the default format.
21-37: Catch those errors like mom's spaghetti!The download handler needs error handling to provide feedback to users when downloads fail.
73-77: These PropTypes are as solid as mom's cooking!Well-defined prop types with clear required vs optional distinctions.
| await downloadReport({ | ||
| monitorData: monitor?.data?.data, | ||
| downloadFormat, | ||
| certificateExpiry, | ||
| }); |
There was a problem hiding this comment.
Validate that monitor data before it makes you weak in the knees!
The optional chaining on monitor?.data?.data suggests potential null values. Add validation before passing to downloadReport.
- await downloadReport({
- monitorData: monitor?.data?.data,
- downloadFormat,
- certificateExpiry,
- });
+ const monitorData = monitor?.data?.data;
+ if (!monitorData) {
+ throw new Error('Monitor data not available');
+ }
+ await downloadReport({
+ monitorData,
+ downloadFormat,
+ certificateExpiry,
+ });📝 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.
| await downloadReport({ | |
| monitorData: monitor?.data?.data, | |
| downloadFormat, | |
| certificateExpiry, | |
| }); | |
| const monitorData = monitor?.data?.data; | |
| if (!monitorData) { | |
| throw new Error('Monitor data not available'); | |
| } | |
| await downloadReport({ | |
| monitorData, | |
| downloadFormat, | |
| certificateExpiry, | |
| }); |
|
@ajhollid can you verify all the changes made to the PR. |
|
Hi @DMJain , After further reviewing this PR I believe that report generation is best done entirely on the backend, I can't think of any good reason to build reports on the front end of the application. This is a data intense application and we do not want to bog down the front end doing work that should be done on the backend. Aside from that, there are many hardcoded absolute dimension values used for styling. Using hardcoded values, especially absolute dimenisons, is not compatible with our project policy. I appreciate your efforts here, if you'd like to continute working on this one we can open a new issue/PR and plan out how this should be implemented. I'm happy to go over that with you if you like! |
|
@ajhollid @gorkem-bwl I would like to continue working on this feature effectively. Let's do it correctly. Moving forward, what approach do you suggest we take: creating a new API endpoint for report data or modifying the current one? For front end, another approach I considered was to include the report component within the Detail page but not display it directly. {display: none} This would allow the report component to render within the ThemeProvider and inherit the app-wide theme. There are lots of ways to achieve this functionality. can we connect and discuss the approach so that we can have a clear direction moving forward? |
Hey @DMJain ,glad you're on board for this! Let's create a new endpoint, endpoints are free 😂 Let's set it up at /api/v1/reports/<whatever_you_need> You can create a router, controller, and db module for this purpose, please follow the general patten of the other controllers / modules / routers. You will probably need a service to generate the reports, so you can have a look at how other services are set up and registered on the backend. If you can open up a draft PR and push to it incrementally we can follow along with your progress. Please try to keep library use to the minimum necessary and check dependencies to see if we have what you need there already. Thanks again for your contributions! Feel free to post any time you'd like to consult. |
Describe your changes
Added new functionality Download report in PDF/HTML format using existing components
This functionality will allow user to download the report in pdf or html format with necessary stats for it's monitor.
Issue number
#1588
Mention the issue number(s) this PR addresses (e.g., #123).
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
PDF, Images and Video of the Feature.
healthCheck Personal Portfolio-monitor-report-2025-02-11 (5).pdf
Recording.2025-02-12.003932.mp4