fix(service-portal): Change download for pdf docs inbox #16398
Conversation
thordurhhh
added
the
deprecated:automerge
WalkthroughThe changes in this pull request involve updates to the Changes
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (6)
apps/download-service/src/app/modules/documents/document.controller.ts (2)
43-43: LGTM: Method signature updated correctlyThe addition of the
@Query('action') action: stringparameter to thegetPdfmethod is appropriate for implementing the new download behavior. This change aligns with the PR objectives and maintains type safety.Consider adding a default value or making the
actionparameter optional for backward compatibility:@Query('action') action?: string = 'inline'This would ensure that existing calls to the endpoint without the
actionquery parameter continue to work as before.
66-68: LGTM: Content-Disposition header set correctly based on actionThe conditional setting of the
Content-Dispositionheader based on theactionparameter effectively implements the desired download behavior. This change addresses the PR objectives and improves compatibility across different browsers.For improved readability, consider extracting the ternary operation into a separate constant:
const disposition = action === 'download' ? 'attachment' : 'inline'; res.header( 'Content-Disposition', `${disposition}; filename=${rawDocumentDTO.fileName}.pdf` );This minor refactoring would make the code slightly more readable without changing its functionality.
libs/service-portal/documents/src/utils/downloadDocumentV2.ts (1)
40-40: LGTM! Consider adding type annotation forquery.The changes to the
downloadFilefunction effectively implement the new download method for PDF documents as per the PR objectives. The optionalqueryparameter allows for flexible URL construction, maintaining backwards compatibility.Consider adding a type annotation for the
queryparameter to improve code clarity:export const downloadFile = async ( doc: ActiveDocumentType2, userInfo: User, - query?: string, + query?: string | undefined, ) => {Also applies to: 68-69, 72-72
libs/service-portal/documents/src/components/DocumentActionBar/DocumentActionBarV2.tsx (3)
121-139: LGTM! Consider enhancing accessibility.The implementation for downloading HTML documents is well-structured and follows React best practices. It adheres to the coding guidelines for reusability and TypeScript usage.
To improve accessibility, consider adding an
aria-labelto theButtoncomponent:<Button as="span" unfocusable circle icon="download" iconType="outline" size="medium" colorScheme="light" + aria-label={formatMessage(m.download)} />This will provide more context for screen readers when focusing on the button.
🧰 Tools
🪛 Biome
[error] 121-121: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
140-153: LGTM! Consider enhancing consistency.The implementation for downloading PDF documents aligns well with the PR objectives and follows React best practices. It adheres to the coding guidelines and uses the
downloadFilefunction, which should improve browser compatibility as mentioned in the PR summary.For consistency with the HTML download button, consider adding an
aria-labelto this Button as well:<Button circle icon="download" iconType={'outline'} onClick={() => downloadFile(activeDocument, userInfo, 'download') } size="medium" colorScheme="light" + aria-label={formatMessage(m.download)} />This will ensure both download buttons have consistent accessibility features.
121-154: Great job on restructuring! Consider using optional chaining.The restructuring of the download functionality using a ternary operator improves code readability and follows React best practices. The separation of HTML and PDF download handling is clear and maintainable.
Consider using optional chaining as suggested by the static analysis tool:
- {activeDocument && activeDocument.document.value ? ( + {activeDocument?.document.value ? (This change will make the code more concise and potentially prevent runtime errors if
activeDocumentis unexpectedlynullorundefined.🧰 Tools
🪛 Biome
[error] 121-121: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/download-service/src/app/modules/documents/document.controller.ts (3 hunks)
- libs/service-portal/documents/src/components/DocumentActionBar/DocumentActionBarV2.tsx (1 hunks)
- libs/service-portal/documents/src/utils/downloadDocumentV2.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/download-service/src/app/modules/documents/document.controller.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/service-portal/documents/src/components/DocumentActionBar/DocumentActionBarV2.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."
libs/service-portal/documents/src/utils/downloadDocumentV2.ts (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."
🪛 Biome
libs/service-portal/documents/src/components/DocumentActionBar/DocumentActionBarV2.tsx
[error] 121-121: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
apps/download-service/src/app/modules/documents/document.controller.ts (2)
11-19: LGTM: Import statement updated correctlyThe addition of
Queryto the import statement from '@nestjs/common' is appropriate for handling the new query parameter in thegetPdfmethod. This change aligns with NestJS best practices for parameter decoration.
Line range hint
1-77: Summary: Changes effectively implement new download behaviorThe modifications to the
DocumentControllerclass, particularly in thegetPdfmethod, successfully implement the new download behavior for PDF documents. The changes include:
- Adding a new
actionquery parameter to control the download behavior.- Conditionally setting the
Content-Dispositionheader based on theactionparameter.These changes align well with the PR objectives and should improve compatibility across different web browsers. The implementation maintains good TypeScript practices and follows NestJS conventions.
To ensure that these changes don't introduce any unintended side effects, please run the following verification script:
This script will help identify any potential areas in the codebase that might need to be updated to work with the new
actionparameter.libs/service-portal/documents/src/utils/downloadDocumentV2.ts (1)
Line range hint
1-89: Compliance with coding guidelines confirmed.The code in this file adheres to the specified guidelines for
libs/**/*:
- The
downloadFilefunction is exported, promoting reusability across different NextJS apps.- TypeScript is used effectively for defining types and exporting functions.
- No issues with tree-shaking or bundling practices are apparent.
libs/service-portal/documents/src/components/DocumentActionBar/DocumentActionBarV2.tsx (1)
Line range hint
1-154: Overall, excellent work on improving the download functionality!The changes in this file align well with the PR objectives and coding guidelines. The separation of HTML and PDF download handling improves maintainability and browser compatibility. The use of TypeScript and reusable components adheres to the project's standards.
A few minor suggestions have been made to further enhance accessibility and code safety. Great job on this update!
🧰 Tools
🪛 Biome
[error] 121-121: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Datadog ReportBranch report: ✅ 0 Failed, 1 Passed, 0 Skipped, 18.38s Total Time |
What
Change download for pdf docs inbox
Why
Current approach is blocked by some browsers. Use download-service instead when possible.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style