feat: enhance PDF export with improved styling and error handling #91
Conversation
WalkthroughThis pull request introduces a new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/UI
participant Hook as useExportPDF
participant Generator as generatePDF
participant Library as html2pdf.js
UI->>Hook: Call exportPDF
Hook->>Generator: Initiate PDF generation
Generator->>Library: Generate PDF from HTML content
Library-->>Generator: Return PDF (or error)
alt Success
Generator->>Hook: Deliver generated PDF
Hook->>UI: Return PDF file
else Error
Generator->>Hook: Report error
Hook->>Hook: Log error and check retry count
loop Retry Attempts (if < MAX_RETRIES)
Hook->>Library: Retry PDF generation
end
Hook->>UI: Return final PDF or error after retries
end
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/lib/hooks/use-export-pdf.tsx (5)
4-8: Prefer usingimport typefor type-only imports.Static analysis indicates that some imported symbols (e.g.,
Invoice,InvoiceItem,PaymentData) are used exclusively as types. Converting these imports toimport typecan help reduce bundle size and enhance clarity in type usage.Here is a recommended diff:
-import { Invoice, InvoiceItem } from "@requestnetwork/data-format"; -import { PaymentData } from "../types"; +import type { Invoice, InvoiceItem } from "@requestnetwork/data-format"; +import type { PaymentData } from "../types";🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
[error] 8-8: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
17-20: Adjust configuration constants for maintainability.
TIMEOUT_MSandMAX_RETRIESare magic numbers that might need fine-tuning across environments. Consider exposing them either via config files, environment variables, or top-level constants to make them more discoverable and consistent across the codebase.
21-23: Ensure locale consistency in date formatting.
new Date(date).toLocaleDateString()may behave differently across regions or servers. Consider specifying a locale or format string for more stable, predictable output (for example,'en-GB','en-US', or a configuration-based approach).
34-52: Verify negative or zero price handling.Using
BigIntto compute item totals is robust, but caution is needed if:
unitPrice < discount, making priceAfterDiscount negative.quantityis0.
Handle these edge cases explicitly to avoid surprises in final invoice totals.
68-320: Review robust error handling and confirm cross-browser PDF rendering.
- Potential SSR usage: Since this is a Next.js codebase, ensure
use clientsuffices and calls towindow.location.originwon't unexpectedly break in SSR contexts.- Error-handling logic: You have a retry mechanism; ensure that repeated
await sleep(1000)plus a final throw meets user experience requirements under ephemeral or network-limited conditions.- DOM cleanup: The
finallyblock ensures containers are removed—good practice. Make sure there's no concurrency scenario where multiple exports overlap in the DOM.- Large intangible overhead: The entire HTML string is kept in memory. Consider streaming or partial rendering if the invoice is extremely large.
Overall, the approach is sound; just confirm these advanced scenarios are tested and validated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/logo-1.pngis excluded by!**/*.png
📒 Files selected for processing (2)
package.json(1 hunks)src/lib/hooks/use-export-pdf.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/hooks/use-export-pdf.tsx
[error] 4-4: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
[error] 8-8: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
🔇 Additional comments (3)
src/lib/hooks/use-export-pdf.tsx (2)
25-32: Validate currency details fallback.When
isAddress(currency)orcurrencyManager.fromSymbol(currency)returns undefined, this function currently returnsundefined. Downstream consumers may not handleundefinedgracefully. Consider providing fallback behavior or throwing an error if the currency is unknown.
54-66: Log formatBigIntAmount fallback usage.When an error occurs, you return
"0"and suppress the exception, which might mask data issues. Consider more detailed logging or surfacing warnings to ensure that unexpected failures in large numeric conversions are not silently dismissed.package.json (1)
31-31: Confirm html2pdf.js version and check for known vulnerabilities.Adding
"html2pdf.js": "^0.10.2"improves the flow by removing dynamic script loading, but confirm that this version is stable and has no reported security flaws. Reviewing release notes or security advisories is advisable before finalizing.
aimensahnoun
deleted the
64-update-request-scan-to-use-the-same-html-for-pdf-as-web-components-currently-pdf-is-cut-off-only-top-half-rendered
branch
Resolves #64
Updates:
Example:
Summary by CodeRabbit
New Features
Chores