chore(web): Prevent 500 error in case of failure when fetching PowerBi embed props #16723
Conversation
WalkthroughThe pull request modifies 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: 1
🧹 Outside diff range and nitpick comments (1)
libs/cms/src/lib/powerbi.service.ts (1)
40-62: Consider implementing more robust error handling strategiesThe current implementation could benefit from:
- Distinguishing between different types of failures (auth failures vs. API failures)
- Implementing retry mechanisms for transient failures
- Providing more detailed error information to the caller
Consider implementing a custom error class:
class PowerBiServiceError extends Error { constructor( message: string, public readonly code: 'AUTH_FAILED' | 'REPORT_NOT_FOUND' | 'EMBED_FAILED', public readonly cause?: Error ) { super(message); this.name = 'PowerBiServiceError'; } }This would allow for more specific error handling upstream while maintaining the current null return for backward compatibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/cms/src/lib/powerbi.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/cms/src/lib/powerbi.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."
🔇 Additional comments (2)
libs/cms/src/lib/powerbi.service.ts (2)
11-11: LGTM: Logger integration follows best practices
The addition of the Logger service through dependency injection is well-implemented and follows the established patterns for service instrumentation.
Also applies to: 23-23
40-62: LGTM: Changes maintain service reusability
The error handling improvements make the service more robust while maintaining:
- Clear TypeScript types
- Consistent return types
- Compatibility with existing consumers
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16723 +/- ##
==========================================
- Coverage 36.51% 36.50% -0.01%
==========================================
Files 6890 6890
Lines 143803 143833 +30
Branches 40970 40984 +14
==========================================
Hits 52511 52511
- Misses 91292 91322 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 18 Total Test Services: 0 Failed, 17 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/cms/src/lib/powerbi.service.ts (1)
56-59: Consider adding error status to the response type.Instead of returning null, consider enhancing the response type to include error status. This would help the frontend display appropriate feedback to users.
export interface GetPowerBiEmbedPropsFromServerResponse { accessToken: string; embedUrl: string; + status: 'success' | 'error'; + errorMessage?: string; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/cms/src/lib/powerbi.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/cms/src/lib/powerbi.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."
🔇 Additional comments (2)
libs/cms/src/lib/powerbi.service.ts (2)
11-11: LGTM: Logger integration looks good!
The Logger integration follows proper dependency injection patterns and TypeScript practices.
Also applies to: 23-23
40-62: Verify complete error handling coverage.
While the try-catch successfully prevents 500 errors, let's verify that all async operations are properly covered.
Prevent 500 error in case of failure when fetching PowerBi embed props
What
Screenshots / Gifs
Before
After
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes