fix: add try-catch block in archive controller #379
Conversation
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant Controller as ArchivedNotificationsController
participant Service as ArchivedNotificationsService
participant Logger as Logger
Controller->>Logger: Log start of archiving process
Controller->>Service: Call archiveCompletedNotificationsCron()
alt Successful archiving
Service-->>Controller: Return success
Controller->>Logger: Log successful completion
else Error occurs
Service-->>Controller: Throw error
alt Is HttpException
Controller->>Controller: Re-throw HttpException
else Other error
Controller->>Logger: Log error details
Controller->>Controller: Throw error
end
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (3)
apps/api/src/modules/archived-notifications/archived-notifications.controller.ts (3)
14-14: Use appropriate log level for operation startConsider using
loglevel instead ofdebugfor operation start, as this is a significant business operation rather than debug information.- this.logger.debug('Archiving completed notifications...'); + this.logger.log('Starting to archive completed notifications...');
22-23: Enhance error logging with more contextThe error logging could be more specific and structured:
- Include operation name in error message
- Use error.stack directly instead of JSON.stringify
- this.logger.error('Error while archiving notifications'); - this.logger.error(JSON.stringify(error, ['message', 'stack'], 2)); + this.logger.error( + `Failed to archive completed notifications: ${error.message}`, + error.stack + );
13-25: Consider adding request correlation ID for better traceabilityTo improve error tracking across the application, consider adding a correlation ID to your logs. This will help trace the entire request flow, especially in a containerized environment.
Example implementation:
@Injectable() export class CorrelationIdMiddleware implements NestMiddleware { use(req: Request, res: Response, next: Function) { req['correlationId'] = uuid(); next(); } }Then update your logging:
try { const correlationId = request['correlationId']; this.logger.log('Starting to archive completed notifications...', { correlationId }); // ... existing code ... } catch (error) { // ... existing code ... this.logger.error( `Failed to archive completed notifications: ${error.message}`, error.stack, { correlationId: request['correlationId'] } ); throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/modules/archived-notifications/archived-notifications.controller.ts(1 hunks)
🔇 Additional comments (1)
apps/api/src/modules/archived-notifications/archived-notifications.controller.ts (1)
13-25: LGTM! The try-catch implementation meets the PR objectiveThe implementation successfully prevents the Docker container from stopping unexpectedly by:
- Properly catching and handling errors
- Distinguishing between HTTP and other exceptions
- Ensuring detailed error logging before re-throwing
apps/api/src/modules/archived-notifications/archived-notifications.controller.ts
Show resolved
Hide resolved
apps/api/src/modules/archived-notifications/archived-notifications.controller.ts
Show resolved
Hide resolved
xixas
changed the title
xixas
changed the title
API PR Checklist
Pre-requisites
.env.examplefile with the required values as applicable.PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
This Pr add a try catch block in the archieve controller to avoid docker container from stopping
Related changes:
Screenshots:
Even with the above error in the logs container is running fine , without the try-catch docker container was stopping
Query request and response:
N/A
Documentation changes:
N/A
Test suite/unit testing output:
N/A
Pending actions:
N/A
Additional notes:
N/A
Summary by CodeRabbit