fix(my-pages): Law and Order - error handling #16506
Conversation
WalkthroughThe pull request introduces enhancements 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
|
disaerna
changed the title
disaerna
added
the
deprecated:automerge
![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 (3)
libs/api/domains/law-and-order/src/lib/law-and-order.resolver.ts (1)
98-103: Consider adding a comment for theauditAndHandlemethodWhile the changes adhere to the coding guidelines for files in the
libs/**/*path, it would be beneficial to add a brief comment explaining the purpose and functionality of theauditAndHandlemethod. This would enhance code readability and maintainability, especially for developers who might be new to the project.For example:
// Wraps the service call with error handling and auditing capabilities return this.auditAndHandle( 'getLawyers', 'lawyers', this.lawAndOrderService.getLawyers(user, locale), user, )libs/service-portal/law-and-order/src/components/DefenderChoices/DefenderChoices.tsx (2)
Line range hint
189-204: Approve changes with a minor suggestion for consistencyThe addition of the error check in the conditional rendering of the submit button for non-popup scenarios is a good improvement. It enhances the user experience by preventing form submission when there's an error, which aligns with the PR's objective of improving error handling.
For consistency with the popup scenario, consider extracting the button content into a separate component or variable. This would reduce code duplication and improve maintainability. For example:
const submitButtonContent = postActionLoading ? ( <LoadingDots /> ) : ( formatMessage(messages.confirm) ); // Then use it in both popup and non-popup scenarios <Button type="submit" size="small" disabled={methods.formState.isSubmitting || postActionLoading} > {submitButtonContent} </Button>
Line range hint
204-235: Approve changes with a suggestion for accessibility improvementThe addition of the error check in the conditional rendering of the submit and cancel buttons for popup scenarios is a good improvement. It enhances the user experience by preventing form submission and cancellation when there's an error, which aligns with the PR's objective of improving error handling.
The use of Box components for layout promotes reusability across different NextJS apps, adhering to the coding guidelines.
To improve accessibility, consider adding aria labels to the buttons. This will help screen readers provide more context to users. For example:
<Button type="button" size="small" variant="ghost" onClick={() => popUp.setPopUp(false)} aria-label={formatMessage(messages.cancelAriaLabel)} // Add this line > {formatMessage(messages.cancel)} </Button> <Button type="submit" size="small" disabled={methods.formState.isSubmitting || postActionLoading} aria-label={formatMessage(messages.confirmAriaLabel)} // Add this line > {postActionLoading ? <LoadingDots /> : formatMessage(messages.confirm)} </Button>Don't forget to add the corresponding messages to your localization file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/api/domains/law-and-order/src/lib/law-and-order.resolver.ts (1 hunks)
- libs/service-portal/law-and-order/src/components/DefenderChoices/DefenderChoices.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/api/domains/law-and-order/src/lib/law-and-order.resolver.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."
libs/service-portal/law-and-order/src/components/DefenderChoices/DefenderChoices.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/api/domains/law-and-order/src/lib/law-and-order.resolver.ts (1)
98-103: Excellent improvement in error handling and auditing!The changes to the
getLawyersmethod align it with other query methods in the class by utilizing theauditAndHandlemethod. This enhancement improves error handling and adds auditing capabilities, which is a positive step towards more robust and maintainable code.The implementation adheres to TypeScript usage and doesn't affect the reusability of the component across different NextJS apps. Good job on maintaining consistency and improving the overall quality of the code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16506 +/- ##
==========================================
- Coverage 36.76% 36.75% -0.01%
==========================================
Files 6851 6850 -1
Lines 142120 141935 -185
Branches 40542 40471 -71
==========================================
- Hits 52250 52170 -80
+ Misses 89870 89765 -105
Flags with carried forward coverage won't be shown. Click here to find out more. see 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/clients/documents-v2/src/lib/documentsClientV2.service.ts (1)
101-106: LGTM! Consider minor improvements to logging.The added info log complements the existing warning log well and improves observability.
Some minor suggestions to enhance the logging:
- Consider a more descriptive message: 'Successfully retrieved document content for findDocumentById'
- Consider standardizing the 'No provider available' fallback across all logs
- Consider defining LOG_CATEGORY as a const enum for better type safety
- this.logger.info('Document content available for findDocumentById', { + this.logger.info('Successfully retrieved document content for findDocumentById', { category: LOG_CATEGORY, documentId, documentProvider: document?.senderName ?? 'No provider available', })libs/api/domains/documents/src/lib/documentV2.service.ts (2)
131-136: Enhance urgent document logging with additional contextWhile the current logging is good, consider adding more context to aid in debugging and monitoring:
if (document.urgent) this.logger.info('Urgent document fetched', { documentId: documentId, includeDocument, + category: LOG_CATEGORY, + senderName: document.senderName, + documentType: document.fileType })
493-493: Optimize action mapper success loggingThe success logging is good for traceability, but consider optimizing the logged data:
- this.logger.info('Actions mapped successfully', { actions: mapped }) + this.logger.info('Actions mapped successfully', { + category: LOG_CATEGORY, + id, + actionCount: mapped.length, + actionTypes: mapped.map(action => action.type) + })This change:
- Adds consistent category logging
- Includes document ID for context
- Reduces log verbosity while maintaining useful information
Also applies to: 520-521
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/api/domains/documents/src/lib/documentV2.service.ts (3 hunks)
- libs/clients/documents-v2/src/lib/documentsClientV2.service.ts (1 hunks)
- libs/service-portal/law-and-order/src/screens/Subpoena/Subpoena.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/api/domains/documents/src/lib/documentV2.service.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."
libs/clients/documents-v2/src/lib/documentsClientV2.service.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."
libs/service-portal/law-and-order/src/screens/Subpoena/Subpoena.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/service-portal/law-and-order/src/screens/Subpoena/Subpoena.tsx (1)
Line range hint
1-190: Implementation follows best practices and guidelines.The code demonstrates good adherence to the coding guidelines for library code:
- Properly uses TypeScript for type safety
- Leverages reusable components from shared libraries
- Follows effective tree-shaking practices with specific imports
- Maintains component reusability across NextJS apps
libs/service-portal/law-and-order/src/screens/Subpoena/Subpoena.tsx
Outdated
Show resolved
Hide resolved
thorkellmani
removed
the
deprecated:automerge
libs/service-portal/law-and-order/src/screens/Subpoena/Subpoena.tsx
Outdated
Show resolved
Hide resolved
disaerna
added
the
deprecated:automerge
* fix: remove button if error * feat: audit and log error if lawyer list fails * feat: detailed logs * fix: remove noice * revert message prop --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix(clients): prod path (#16554) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(my-pages): Law and Order - error handling (#16506) * fix: remove button if error * feat: audit and log error if lawyer list fails * feat: detailed logs * fix: remove noice * revert message prop --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(service-portal): Filtering mailbox documents (#16219) * fix: health delegation bug and move logic to service * feat: hide law and order and health data if certain delegation * console * fix: add correct id * remove birthday use (v1 is deprecated) --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(my-pages): display error message on urgent files on dashboard (#16572) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
My pages
What
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes