fix(auth-admin): Move sleep logic from messgeHandler to worker service #16712
Conversation
WalkthroughThe pull request introduces modifications to the notification system, specifically enhancing the handling of operational hours for the worker service. A new configuration property, 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
|
Datadog ReportAll test runs ❌ 5 Total Test Services: 1 Failed, 4 Passed Test Services
❌ Failed Tests (11)
🔻 Code Coverage Decreases vs Default Branch (1)
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16712 +/- ##
=======================================
Coverage 36.44% 36.44%
=======================================
Files 6852 6855 +3
Lines 143541 143636 +95
Branches 40966 40992 +26
=======================================
+ Hits 52307 52354 +47
- Misses 91234 91282 +48
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 38 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
libs/message-queue/src/lib/nest/types.ts (1)
21-22: Enhance the property documentation.The comment could be more descriptive about when and why the worker sleeps. Consider expanding it to clarify the working hours concept and its impact.
- // if true, the worker will sleep during the night + // if true, the worker will pause processing during non-working hours to optimize + // resource usage. Working hours are typically defined as Monday-Friday, 9 AM to 5 PMlibs/message-queue/src/lib/nest/utils.ts (1)
24-31: Add documentation and handle edge casesThe time calculation is complex and would benefit from:
- JSDoc documentation explaining the logic
- Edge case handling
- Explicit timezone handling
Consider these improvements:
+/** + * Calculates the duration in milliseconds until the next working day at WORK_STARTING_HOUR + * + * @returns number Milliseconds until next working hour + * @throws {Error} If the calculation results in negative duration + */ export const calculateSleepDurationUntilMorning = (): number => { const now = new Date() const currentHour = now.getHours() const currentMinutes = now.getMinutes() const currentSeconds = now.getSeconds() const sleepHours = (24 - currentHour + WORK_STARTING_HOUR) % 24 - return (sleepHours * 3600 - currentMinutes * 60 - currentSeconds) * 1000 + const duration = (sleepHours * 3600 - currentMinutes * 60 - currentSeconds) * 1000 + + if (duration < 0) { + throw new Error('Calculated sleep duration is negative') + } + + return duration }libs/message-queue/src/lib/nest/utils.spec.ts (1)
20-28: Enhance test coverage for edge cases and documentation.While the current tests cover basic scenarios, consider:
- Adding edge cases for exactly 8:00 and 22:00
- Testing weekend behavior if working hours differ
- Using consistent comment formatting (e.g., "// 7:00 AM" instead of mixing styles)
describe('isOutsideWorkingHours', () => { it('returns true if the current hour is outside working hours', () => { - expect(isOutsideWorkingHours(new Date('2023-01-01T07:00:00'))).toBe(true) // 7 AM - expect(isOutsideWorkingHours(new Date('2023-01-01T23:00:00'))).toBe(true) // 11 PM - expect(isOutsideWorkingHours(new Date('2023-01-01T00:00:00'))).toBe(true) // Midnight - expect(isOutsideWorkingHours(new Date('2023-01-01T08:00:00'))).toBe(false) // 8 AM - expect(isOutsideWorkingHours(new Date('2023-01-01T22:00:00'))).toBe(false) // 10 PM + // Before working hours + expect(isOutsideWorkingHours(new Date('2023-01-01T07:00:00'))).toBe(true) // 7:00 AM + // Start of working hours + expect(isOutsideWorkingHours(new Date('2023-01-01T08:00:00'))).toBe(false) // 8:00 AM + // During working hours + expect(isOutsideWorkingHours(new Date('2023-01-01T15:00:00'))).toBe(false) // 3:00 PM + // End of working hours + expect(isOutsideWorkingHours(new Date('2023-01-01T22:00:00'))).toBe(false) // 10:00 PM + // After working hours + expect(isOutsideWorkingHours(new Date('2023-01-01T23:00:00'))).toBe(true) // 11:00 PM + // Midnight + expect(isOutsideWorkingHours(new Date('2023-01-01T00:00:00'))).toBe(true) // 12:00 AM + // Weekend (if applicable) + expect(isOutsideWorkingHours(new Date('2023-01-07T12:00:00'))).toBe(true) // Saturday noon }) })apps/services/user-notification/src/app/modules/notifications/notifications.module.ts (1)
39-39: Consider making the sleep configuration more flexible.Instead of hardcoding
shouldSleepOutsideWorkingHours: true, consider making it configurable through environment variables. This would allow different behavior in different environments (e.g., development vs. production).- shouldSleepOutsideWorkingHours: true, + shouldSleepOutsideWorkingHours: environment.shouldSleepOutsideWorkingHours ?? true,libs/message-queue/src/lib/nest/worker.service.ts (1)
124-124: Review necessity of 'Sleeping' status in shutdown logic.If the worker's status remains
Runningduring sleep, it's unnecessary to includeStatus.Sleepingin the shutdown condition. Adjusting the condition improves code clarity.Apply this diff to refine the
onModuleDestroy()method:- if (this.status === Status.Running || this.status === Status.Sleeping) { + if (this.status === Status.Running) {This change ensures that the shutdown logic triggers appropriately based on the worker's actual operational status.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/services/user-notification/src/app/modules/notifications/notifications.module.ts(1 hunks)apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts(0 hunks)libs/message-queue/src/lib/nest/types.ts(1 hunks)libs/message-queue/src/lib/nest/utils.spec.ts(2 hunks)libs/message-queue/src/lib/nest/utils.ts(2 hunks)libs/message-queue/src/lib/nest/worker.service.ts(5 hunks)
💤 Files with no reviewable changes (1)
- apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/services/user-notification/src/app/modules/notifications/notifications.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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/message-queue/src/lib/nest/types.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/message-queue/src/lib/nest/utils.spec.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/message-queue/src/lib/nest/utils.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/message-queue/src/lib/nest/worker.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 (4)
libs/message-queue/src/lib/nest/types.ts (1)
21-22: Verify the property usage across the codebase.
The type definition looks good and aligns with the PR objective of moving sleep logic to the worker service. Let's verify its implementation.
✅ Verification successful
Property shouldSleepOutsideWorkingHours is correctly implemented and used
The verification confirms proper implementation:
- Used in
WorkerServiceto control sleep behavior during non-working hours - Correctly configured in
notifications.module.tswithshouldSleepOutsideWorkingHours: true - Implementation includes appropriate logging and status management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of shouldSleepOutsideWorkingHours property
# Expected: Should find usage in QueueModule.register and WorkerService
# Test 1: Check QueueModule.register usage
echo "Checking QueueModule.register usage:"
rg -A 5 "QueueModule\.register"
# Test 2: Check WorkerService implementation
echo "Checking WorkerService implementation:"
rg -A 5 "shouldSleepOutsideWorkingHours"
Length of output: 3749
apps/services/user-notification/src/app/modules/notifications/notifications.module.ts (1)
Line range hint 1-73: LGTM! Module structure follows NestJS best practices.
The module maintains proper separation of concerns with clear dependency injection patterns. The queue configuration change aligns well with moving the sleep logic to the worker service.
libs/message-queue/src/lib/nest/worker.service.ts (2)
7-7: Imports are appropriate and clear.
The addition of clamp, isOutsideWorkingHours, and sleepUntilMorning from ./utils is correct and enhances the functionality of the worker service.
23-23: Integration of 'Sleeping' status into the worker lifecycle.
Adding Sleeping to the Status enum appropriately reflects the new state when the worker is paused during non-working hours.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…s into fix/queue-sleep-logic
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
libs/message-queue/src/lib/nest/utils.spec.ts (1)
23-32: Add edge case tests for working hours boundaries.While the basic scenarios are covered, consider adding test cases for edge cases:
- Exactly at work start time (8:00 AM)
- One minute before work start time (7:59 AM)
- Exactly at work end time (4:00 PM)
- One minute after work end time (4:01 PM)
describe('isOutsideWorkingHours', () => { // ... existing tests ... it('handles work start boundary correctly', () => { jest.useFakeTimers({ now: new Date(2021, 1, 1, 8, 0, 0) }) // 8:00 AM expect(isOutsideWorkingHours()).toBe(false) jest.useFakeTimers({ now: new Date(2021, 1, 1, 7, 59, 0) }) // 7:59 AM expect(isOutsideWorkingHours()).toBe(true) }) it('handles work end boundary correctly', () => { jest.useFakeTimers({ now: new Date(2021, 1, 1, 16, 0, 0) }) // 4:00 PM expect(isOutsideWorkingHours()).toBe(false) jest.useFakeTimers({ now: new Date(2021, 1, 1, 16, 1, 0) }) // 4:01 PM expect(isOutsideWorkingHours()).toBe(true) }) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.spec.ts(1 hunks)libs/message-queue/src/lib/nest/utils.spec.ts(2 hunks)libs/message-queue/src/lib/nest/worker.service.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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/message-queue/src/lib/nest/utils.spec.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/message-queue/src/lib/nest/worker.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 (6)
libs/message-queue/src/lib/nest/utils.spec.ts (1)
1-5: Add test coverage for sleepUntilMorning function.
The sleepUntilMorning function is not imported or tested, which could lead to undetected issues in the worker service's sleep logic.
libs/message-queue/src/lib/nest/worker.service.ts (3)
7-7: LGTM: Clean import of sleep-related utilities.
The new imports align well with the PR's objective of moving sleep logic to the worker service.
124-124: LGTM: Proper cleanup handling for sleeping state.
The onModuleDestroy method correctly handles the new Sleeping status, ensuring proper cleanup of active jobs before stopping.
64-72: LGTM: Well-structured sleep implementation with good logging.
The method effectively encapsulates the sleep logic with appropriate logging for both sleep and wake events. However, note that this implementation is affected by the status-related issue mentioned earlier.
Let's verify the sleep configuration usage:
✅ Verification successful
Sleep implementation is well-structured with proper configuration and utility functions
The implementation is well-designed with:
- Configuration properly defined as an optional boolean in the worker config
- Working hours utility functions properly implemented (8 AM - 11 PM)
- Sleep logic encapsulated with appropriate status changes and logging
- Used in the user-notification service with
shouldSleepOutsideWorkingHours: true
The original review comment's approval was correct, and the implementation is verified to be working as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for shouldSleepOutsideWorkingHours configuration
rg "shouldSleepOutsideWorkingHours.*=.*true" -A 2
Length of output: 51
Script:
#!/bin/bash
# Let's search for any usage of shouldSleepOutsideWorkingHours without assuming its value
rg "shouldSleepOutsideWorkingHours" -A 2
Length of output: 936
Script:
#!/bin/bash
# Let's also check the configuration interface/type definition
ast-grep --pattern 'interface $_Config {
$$$
shouldSleepOutsideWorkingHours$_
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Check for isOutsideWorkingHours and sleepUntilMorning implementations
rg "function (isOutsideWorkingHours|sleepUntilMorning)" -A 5
Length of output: 62
Script:
#!/bin/bash
# Let's check for the utility functions in a different way
rg "isOutsideWorkingHours|sleepUntilMorning" -A 3
# Also check for any imports of these functions
rg "import.*{.*(?:isOutsideWorkingHours|sleepUntilMorning).*}"
Length of output: 3146
Script:
#!/bin/bash
# Let's look at the configuration type definition with a different pattern
rg "interface.*Config" -A 10
Length of output: 44360
Script:
#!/bin/bash
# Let's check the implementation of isOutsideWorkingHours and sleepUntilMorning in utils.ts
rg -p "export const (isOutsideWorkingHours|sleepUntilMorning)" -A 10 libs/message-queue/src/lib/nest/utils.ts
Length of output: 697
Script:
#!/bin/bash
# Let's also check the WorkerConfig interface definition
rg -p "interface.*Config.*{" -A 10 libs/message-queue/src/lib/nest/types.ts
Length of output: 181
Script:
#!/bin/bash
# Let's check the Config interface with a simpler pattern
rg "interface.*Config" -A 5 libs/message-queue/src/lib/nest/types.ts
# Also check for any constants related to working hours
rg "WORK_(STARTING|ENDING)_HOUR" libs/message-queue/src/lib/nest/utils.ts
Length of output: 456
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.spec.ts (2)
239-239: LGTM! Good defensive programming practice.
The addition of optional chaining (?.) is a good defensive programming practice to handle cases where recipientMessage might be undefined, preventing potential runtime errors.
Line range hint 1-673: Comprehensive test coverage with robust test cases.
The test suite demonstrates excellent coverage of the NotificationsWorkerService functionality:
- Proper setup and teardown of test environment
- Comprehensive mocking of dependencies
- Clear test cases covering various scenarios including:
- Email and push notification delivery
- Working hours validation
- Feature flag handling
- User profile validation
- Delegation logic
- Company vs individual user handling
This aligns well with NestJS testing best practices and provides good confidence in the service's behavior.
magnearun
added
the
deprecated:automerge
#16712) * move sleep logic from messgeHandler to worker service * chore: nx format:write update dirty files * small refactor * chore: nx format:write update dirty files * Update libs/message-queue/src/lib/nest/utils.spec.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix message-queue tests * chore: nx format:write update dirty files * move sleep check * change placement of sleep check back * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Handle sleep logic in worker instead of messageHandler
Why
Better separation of concerns and
when the worker manages sleep, it doesn’t pull new messages during non-working hours, which optimizes resource usage and reduces unnecessary processing.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
WorkerServiceto manage operations based on working hours, allowing for better control and efficiency.NotificationsWorkerServiceto simplify notification processing by removing time-based checks.Tests
These updates streamline notification processing and improve overall system responsiveness.