Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR introduces a new service-oriented architecture for managing call dispatch and release statuses for both personnel (via shift signups) and units. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CallDispatchStatusService
participant DepartmentSettingsService
participant Repository
participant UserActionLogService
participant UnitStateService
Client->>CallDispatchStatusService: ApplyDispatchStatusesAsync(call, groupIds, unitIds)
CallDispatchStatusService->>DepartmentSettingsService: GetUnitCallDispatchStatusToSetAsync(departmentId)
DepartmentSettingsService->>Repository: Query department setting
Repository-->>DepartmentSettingsService: Setting value or null
DepartmentSettingsService-->>CallDispatchStatusService: status (int)
alt Group IDs provided and shift dispatch enabled
CallDispatchStatusService->>Repository: Get shift signups for shift date
Repository-->>CallDispatchStatusService: User IDs from signups
CallDispatchStatusService->>UserActionLogService: Set action log status for each user
UserActionLogService->>Repository: Persist action logs
end
alt Unit IDs provided
CallDispatchStatusService->>UnitStateService: Set unit state for each unit
UnitStateService->>Repository: Persist unit states with call destination metadata
end
CallDispatchStatusService-->>Client: Task complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 9 seconds.Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Workers/Resgrid.Workers.Framework/Logic/CallBroadcast.cs (1)
119-141: Optional refactor: extract a single "dispatch-once" helper.This signup loop is the third near-identical copy of the same shape (see lines 145–164 for groups, 192–214 for unit-assigned personnel, 223–242 for unit-station-group members, 258–277 for roles). Pulling the dedup + profile lookup + try/catch into a small local function would remove duplication and make it harder to drift error handling between branches.
♻️ Suggested helper
async Task DispatchUserOnceAsync(string userId) { if (string.IsNullOrEmpty(userId) || !dispatchedUsers.Add(userId)) return; try { var profile = cqi.Profiles.FirstOrDefault(x => x.UserId == userId); await _communicationService.SendCallAsync( cqi.Call, new CallDispatch { UserId = userId }, cqi.DepartmentTextNumber, cqi.Call.DepartmentId, profile, cqi.Address); } catch (SocketException) { /* swallow, matches existing behavior */ } catch (Exception ex) { Logging.LogException(ex); } }Then the signup branch collapses to:
- foreach (var signup in signups) - { - if (!dispatchedUsers.Contains(signup.UserId)) - { - dispatchedUsers.Add(signup.UserId); - try - { - var profile = cqi.Profiles.FirstOrDefault(x => x.UserId == signup.UserId); - await _communicationService.SendCallAsync(cqi.Call, new CallDispatch() { UserId = signup.UserId }, cqi.DepartmentTextNumber, cqi.Call.DepartmentId, profile, cqi.Address); - } - catch (SocketException sex) - { - } - catch (Exception ex) - { - Logging.LogException(ex); - } - } - } + foreach (var signup in signups) + await DispatchUserOnceAsync(signup.UserId);As per coding guidelines: "Use extension methods appropriately for domain-specific operations" and prefer "composition with interfaces to extend behavior" — extracting the per-user dispatch reduces duplication and isolates the side effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Framework/Logic/CallBroadcast.cs` around lines 119 - 141, Extract the repeated "dispatch-once" logic into a local helper (e.g., DispatchUserOnceAsync) inside CallBroadcast so each branch calls it instead of duplicating code: the helper should accept a userId, check for null/empty and use dispatchedUsers.Add(userId) to ensure single dispatch, perform the profile lookup via cqi.Profiles.FirstOrDefault(x => x.UserId == userId), call _communicationService.SendCallAsync(cqi.Call, new CallDispatch { UserId = userId }, cqi.DepartmentTextNumber, cqi.Call.DepartmentId, profile, cqi.Address), and preserve existing exception handling (swallow SocketException, log other exceptions via Logging.LogException); replace the foreach blocks that currently contain the dedupe/lookup/try-catch with calls to DispatchUserOnceAsync(signup.UserId) (and analogous places for groups/roles/unit members).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Services/CallDispatchStatusService.cs`:
- Around line 145-165: GetShiftDate/GetReferenceDate in
CallDispatchStatusService currently always uses the call's stored timestamps
(LastDispatchedOn/DispatchOn/LoggedOn) which can be stale when UpdateCall
performs incremental group additions; modify the service to accept an optional
effective operation time (DateTime? effectiveTime) and use that as the primary
reference in GetReferenceDate (fall back to LastDispatchedOn/DispatchOn/LoggedOn
and then DateTime.UtcNow if effectiveTime is null), update GetShiftDate to use
the new GetReferenceDate signature, and change callers (e.g.,
DispatchController.UpdateCall) to pass the current operation time when doing
incremental updates so shift resolution uses the operation time instead of the
old dispatch timestamp.
---
Nitpick comments:
In `@Workers/Resgrid.Workers.Framework/Logic/CallBroadcast.cs`:
- Around line 119-141: Extract the repeated "dispatch-once" logic into a local
helper (e.g., DispatchUserOnceAsync) inside CallBroadcast so each branch calls
it instead of duplicating code: the helper should accept a userId, check for
null/empty and use dispatchedUsers.Add(userId) to ensure single dispatch,
perform the profile lookup via cqi.Profiles.FirstOrDefault(x => x.UserId ==
userId), call _communicationService.SendCallAsync(cqi.Call, new CallDispatch {
UserId = userId }, cqi.DepartmentTextNumber, cqi.Call.DepartmentId, profile,
cqi.Address), and preserve existing exception handling (swallow SocketException,
log other exceptions via Logging.LogException); replace the foreach blocks that
currently contain the dedupe/lookup/try-catch with calls to
DispatchUserOnceAsync(signup.UserId) (and analogous places for groups/roles/unit
members).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5aba14e-e1d7-4104-87c3-0732bc426add
⛔ Files ignored due to path filters (2)
Core/Resgrid.Localization/Areas/User/Department/Department.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.resxis excluded by!**/*.resx
📒 Files selected for processing (15)
Core/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/Services/ICallDispatchStatusService.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Services/CallDispatchStatusService.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/ServicesModule.csTests/Resgrid.Tests/Services/CallDispatchStatusServiceTests.csTests/Resgrid.Tests/Services/DepartmentSettingsServiceDispatchStatusTests.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Models/Departments/DispatchSettingsView.csWeb/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtmlWorkers/Resgrid.Workers.Console/Tasks/DispatchScheduledCallsTask.csWorkers/Resgrid.Workers.Framework/Logic/CallBroadcast.cs
Comment on lines
+145
to
+165
| private static DateTime GetShiftDate(Call call, Department department) | ||
| { | ||
| var referenceDate = GetReferenceDate(call); | ||
| var localizedDate = department != null ? TimeConverterHelper.TimeConverter(referenceDate, department) : referenceDate; | ||
|
|
||
| return new DateTime(localizedDate.Year, localizedDate.Month, localizedDate.Day); | ||
| } | ||
|
|
||
| private static DateTime GetReferenceDate(Call call) | ||
| { | ||
| if (call.LastDispatchedOn.HasValue) | ||
| return call.LastDispatchedOn.Value; | ||
|
|
||
| if (call.DispatchOn.HasValue) | ||
| return call.DispatchOn.Value; | ||
|
|
||
| if (call.LoggedOn != default(DateTime)) | ||
| return call.LoggedOn; | ||
|
|
||
| return DateTime.UtcNow; | ||
| } |
Contributor
There was a problem hiding this comment.
Shift resolution uses a stale date for later dispatch changes.
For calls that were already dispatched earlier, GetReferenceDate locks shift lookups to LastDispatchedOn/DispatchOn. DispatchController.UpdateCall now reuses this service when new groups are added later, so a group added after a shift/day boundary can resolve yesterday’s signups instead of the crew currently being dispatched. Pass the effective operation time into the service, or fall back to DateTime.UtcNow for incremental update flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Services/CallDispatchStatusService.cs` around lines 145 - 165,
GetShiftDate/GetReferenceDate in CallDispatchStatusService currently always uses
the call's stored timestamps (LastDispatchedOn/DispatchOn/LoggedOn) which can be
stale when UpdateCall performs incremental group additions; modify the service
to accept an optional effective operation time (DateTime? effectiveTime) and use
that as the primary reference in GetReferenceDate (fall back to
LastDispatchedOn/DispatchOn/LoggedOn and then DateTime.UtcNow if effectiveTime
is null), update GetShiftDate to use the new GetReferenceDate signature, and
change callers (e.g., DispatchController.UpdateCall) to pass the current
operation time when doing incremental updates so shift resolution uses the
operation time instead of the old dispatch timestamp.
Comment on lines
+883
to
+891
| var shouldApplyDispatchStatuses = call.HasBeenDispatched.GetValueOrDefault() || !call.DispatchOn.HasValue || call.DispatchOn.Value <= DateTime.UtcNow; | ||
| if (shouldApplyDispatchStatuses && (cancelledGroupIds.Any() || cancelledUnitIds.Any())) | ||
| { | ||
| await _callDispatchStatusService.ApplyReleaseStatusesAsync(call, cancelledGroupIds, cancelledUnitIds, cancellationToken); | ||
| } | ||
|
|
||
| if (shouldApplyDispatchStatuses && (newGroupIds.Any() || newUnitIds.Any())) | ||
| { | ||
| await _callDispatchStatusService.ApplyDispatchStatusesAsync(call, newGroupIds, newUnitIds, cancellationToken); |
Contributor
There was a problem hiding this comment.
Avoid clearing responders who are still dispatched through another shift group.
This releases everyone resolved from cancelledGroupIds in isolation. If multi-group shift signups are allowed, a responder who belongs to both a removed group and a still-dispatched group will still get the clear status here. Filter out users who remain covered by retained/new group dispatches before calling the release path.
Member
Author
|
Approve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Refactor