-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(island-ui): Polyfill for PDF viewer #16249
Conversation
WalkthroughThe changes in this pull request primarily focus on the Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16249 +/- ##
=======================================
Coverage 36.92% 36.92%
=======================================
Files 6781 6781
Lines 140009 139996 -13
Branches 39810 39803 -7
=======================================
- Hits 51703 51699 -4
+ Misses 88306 88297 -9 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 101 Total Test Services: 0 Failed, 97 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (6)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (2)
Line range hint
24-30
: Remove unusedIPdfLib
interface to improve maintainabilityThe
IPdfLib
interface is no longer needed since the dynamic import ofreact-pdf
has been removed. Eliminating unused code enhances maintainability and supports effective tree-shaking.Apply this diff to remove the unused interface:
-interface IPdfLib { - default: any - pdfjs: typeof pdfjs - Document: typeof Document - Page: typeof Page - Outline: typeof Outline -}
71-71
: Simplifyloading
prop by passingloadingView
directlyInstead of wrapping
loadingView
in an anonymous function, you can pass it directly to theloading
prop for clarity.Apply this diff to simplify the code:
- loading={() => loadingView()} + loading={loadingView}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (2)
- libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (3 hunks)
- package.json (0 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (1)
65-118
: Code changes improve clarity and remove unnecessary complexityThe refactored component simplifies the rendering logic by eliminating the dynamic import and state management for
pdfLib
. Directly usingDocument
andPage
components enhances readability and efficiency. The error handling and pagination logic are correctly maintained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (1)
Line range hint
1-118
: Improved overall component structure and TypeScript usageThe changes in this file have significantly improved the PdfViewer component:
- Consistent use of TypeScript for defining props and types aligns with our coding guidelines.
- The simplified structure enhances reusability and maintainability.
- The changes successfully address the PR objective of introducing a polyfill for the PDF viewer functionality.
These modifications contribute to better reusability across different NextJS apps and adhere to our TypeScript usage guidelines.
However, I suggest a minor improvement in TypeScript usage:
Consider using more specific types for the error state:
- const [pdfLibError, setPdfLibError] = useState<any>() + const [pdfLibError, setPdfLibError] = useState<Error | null>(null)This change provides more type safety and makes the error handling more explicit.
Overall, great job on improving this component!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (1)
Line range hint
1-16
: Improved imports and PDF.js worker setupThe changes in this section enhance the code in several ways:
- Removal of unused
useEffect
import improves tree-shaking.- Direct imports from 'react-pdf' simplify the code and improve readability.
- Static setup of the PDF.js worker source eliminates the need for dynamic import, potentially improving initial load time.
These modifications align well with our coding guidelines for effective tree-shaking and bundling practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
jest.preset.js (1)
Line range hint
28-36
: Consider updating Jest snapshot formatThere's a TODO comment about updating to the latest Jest snapshot format. While not directly related to the current PR objectives, addressing this could improve the overall testing setup.
Consider updating the Jest snapshot format by removing the
snapshotFormat
property and running tests with the--update-snapshot
flag. This will ensure you're using the latest Jest features and formats.libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (3)
65-75
: LGTM with a suggestion: Improved Document component renderingThe direct rendering of the Document component and streamlined error handling are good improvements. They enhance readability and maintainability, aligning with our guidelines for reusable components.
Consider implementing the suggestion from the previous review for consistent error handling:
- error={errorComponent ?? pdfError} + error={<ErrorComponent message={pdfError} />}This would allow for consistent error styling and easier maintenance of error messages.
76-93
: LGTM with a suggestion: Simplified page rendering logicThe simplified conditional rendering for showing all pages or a single page is a good improvement. It enhances readability and maintainability, contributing to the component's reusability across different NextJS apps.
Consider implementing the suggestion from the previous review for a minor optimization:
- [...Array(numPages)].map((x, page) => ( + Array.from({ length: numPages }, (_, page) => (This change is more idiomatic and slightly more efficient, especially for larger documents.
96-118
: LGTM with a suggestion: Improved pagination structureMoving the pagination outside the Document component and adding className logic for hiding pagination when not needed are excellent improvements. They enhance the component's structure and flexibility, aligning well with our guidelines for reusable and effective component design.
Consider implementing the suggestion from the previous review to optimize performance:
+ const handlePageChange = React.useCallback((page: number) => { + setPageNumber(page); + }, []); // ... in the Pagination component - onClick={() => setPageNumber(page)} + onClick={() => handlePageChange(page)}This memoization can help prevent unnecessary re-renders, especially for larger documents with many pages.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- jest.preset.js (1 hunks)
- libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
jest.preset.js (1)
27-27
: LGTM. Verify polyfill implementation.The addition of
transformIgnorePatterns
to allow transformation of the 'pdfjs-dist' module aligns with the PR objective of introducing a polyfill for the PDF viewer. This change should enable proper handling of the PDF.js library during Jest tests.To ensure the polyfill is working as expected, please verify:
- The PDF viewer functionality in the relevant components.
- Any tests related to PDF viewing pass successfully.
You can use the following script to check for PDF-related tests:
✅ Verification successful
LGTM. Verified the transformation of PDF-related modules and associated tests.
The addition of
transformIgnorePatterns
to include the 'pdfjs-dist' module ensures that Jest properly handles PDF functionalities. The extensive presence of PDF-related tests confirms that the polyfill implementation is comprehensive and aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find and list PDF-related test files echo "PDF-related test files:" fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | rg -i "pdf" echo "\nPDF-related code in test files:" rg -i "pdf" -t ts -t tsx -g "*test*" -g "*spec*"Length of output: 4712
Script:
#!/bin/bash # Description: Find and list PDF-related test files excluding unsupported file types echo "PDF-related test files:" fd -e test.ts -e spec.ts | rg -i "pdf" echo "\nPDF-related code in test files:" rg -i "pdf" -t ts -g "*test*" -g "*spec*"Length of output: 88651
libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (2)
1-3
: LGTM: Improved imports for better tree-shakingThe direct import of react-pdf components (
Document
,Page
,Outline
,pdfjs
) is a good practice. It enhances code clarity and potentially improves tree-shaking, aligning with our guidelines for effective bundling practices.
12-16
: LGTM: Improved pdfjs worker configurationMoving the pdfjs worker configuration to the top level is a good practice. It ensures the worker is set up once, potentially improving performance. The use of
import.meta.url
is a modern approach that works well with module bundlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/shared/types/src/lib/pdfjs-dist.d.ts (1)
1-5
: Consider adding documentation for library consumers.While the implementation is correct, it would be beneficial to add some documentation or examples for library consumers. This aligns with the coding guidelines for shared libraries, which emphasize the importance of documentation for reusable components.
You could add a comment block above the module declaration explaining its purpose and providing a brief example of how to use it. For instance:
/** * Declaration for the PDF.js worker module. * This module provides the worker source URL for PDF.js. * * Example usage: * import workerSrc from 'pdfjs-dist/legacy/build/pdf.worker.min.mjs'; * * // Use workerSrc when initializing PDF.js * pdfjsLib.GlobalWorkerOptions.workerSrc = workerSrc; */ declare module 'pdfjs-dist/legacy/build/pdf.worker.min.mjs' { // ... (existing code) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx (3 hunks)
- libs/shared/types/src/lib/pdfjs-dist.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/island-ui/core/src/lib/PdfViewer/PdfViewer.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/shared/types/src/lib/pdfjs-dist.d.ts (2)
Pattern
libs/shared/**/*
: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
libs/shared/types/src/lib/pdfjs-dist.d.ts (1)
1-5
: LGTM! The module declaration is correctly implemented.The module declaration for 'pdfjs-dist/legacy/build/pdf.worker.min.mjs' is correctly implemented. It properly exports the
workerSrc
constant as a string, which aligns with the PR objectives of introducing a polyfill for the PDF viewer and ensuring legacy support.
What
Polyfill for pdf-viewer ✨
Remove types package, no longer needed. It depends on an older version of pdfjs-dist, which causes an error.
We need the new version to depend on legacy for browser support.
Why
Proper browser support.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores