Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughAdds incident needs, objective progress, lane lead data, command detail fields, responder incident views, database migrations, notification fan-out, and v4 API endpoints across the incident command model, service, repository, and web layers. ChangesIncident command workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant IncidentCommandController
participant IncidentCommandService
participant IncidentCommandNotificationService
participant CommunicationService
Client->>IncidentCommandController: update command, objective, or need
IncidentCommandController->>IncidentCommandService: validate and persist change
IncidentCommandService->>IncidentCommandNotificationService: publish command notification
IncidentCommandNotificationService->>CommunicationService: deliver user notification
IncidentCommandService-->>IncidentCommandController: updated command data
IncidentCommandController-->>Client: API result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost("UpdateObjectiveProgress")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Command_Update)] | ||
| public async Task<ActionResult<ICModels.TacticalObjectiveResult>> UpdateObjectiveProgress([FromBody] ICModels.UpdateObjectiveProgressInput input) |
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Command_Update)] | ||
| [RequiresIncidentCapability(IncidentCapabilities.ManageObjectives)] | ||
| public async Task<ActionResult<ICModels.IncidentNeedResult>> SaveNeed([FromBody] IncidentNeed need) |
| [HttpPost("SetNeedStatus")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Command_Update)] | ||
| public async Task<ActionResult<ICModels.IncidentNeedResult>> SetNeedStatus([FromBody] ICModels.SetNeedStatusInput input) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Core/Resgrid.Services/IncidentCommandNotificationService.cs (1)
141-199: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the shared department lookups out of the recipient loop.
SendToUserAsyncfetches the department and text-to-call number for every recipient even though both are constant within a broadcast; resolve them once inBroadcastToIncidentAsyncand pass them through, keeping only the per-user profile lookup inside the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/IncidentCommandNotificationService.cs` around lines 141 - 199, The shared department and text-to-call number lookups currently occur for every user through SendToUserAsync. Resolve them once in BroadcastToIncidentAsync before the recipient loop, then update SendToUserAsync to accept and reuse those resolved values while retaining only the per-user profile lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 477-480: The per-call board and bundle need collections apply
inconsistent cancelled-status filtering. Align GetNeedsForCallAsync and the
GetCommandBoardAsync path with GetBundleForDepartmentAsync so both exclude
IncidentNeedStatus.Cancelled, preserving the existing ordering and other
filters.
---
Nitpick comments:
In `@Core/Resgrid.Services/IncidentCommandNotificationService.cs`:
- Around line 141-199: The shared department and text-to-call number lookups
currently occur for every user through SendToUserAsync. Resolve them once in
BroadcastToIncidentAsync before the recipient loop, then update SendToUserAsync
to accept and reuse those resolved values while retaining only the per-user
profile lookup.
🪄 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: 701fafb0-49ab-4a12-882e-c96d3beb7ceb
⛔ Files ignored due to path filters (3)
Tests/Resgrid.Tests/Services/IncidentCommandContentServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandNeedsAndLeadsTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**
📒 Files selected for processing (22)
Core/Resgrid.Model/Events/IncidentCommandEvents.csCore/Resgrid.Model/IncidentCommand/CommandStructureNode.csCore/Resgrid.Model/IncidentCommand/IncidentCommand.csCore/Resgrid.Model/IncidentCommand/IncidentCommandBoard.csCore/Resgrid.Model/IncidentCommand/IncidentCommandChanges.csCore/Resgrid.Model/IncidentCommand/IncidentCommandEnums.csCore/Resgrid.Model/IncidentCommand/IncidentNeed.csCore/Resgrid.Model/IncidentCommand/IncidentTacticals.csCore/Resgrid.Model/IncidentCommand/ResourceIncidentView.csCore/Resgrid.Model/Repositories/IIncidentCommandRepositories.csCore/Resgrid.Model/Services/IIncidentCommandNotificationService.csCore/Resgrid.Model/Services/IIncidentCommandService.csCore/Resgrid.Services/IncidentCommandNotificationService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/ServicesModule.csProviders/Resgrid.Providers.Migrations/Migrations/M0093_AddIncidentNeedsLaneLeadsAndObjectiveProgress.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0093_AddIncidentNeedsLaneLeadsAndObjectiveProgressPg.csRepositories/Resgrid.Repositories.DataRepository/IncidentCommandRepositories.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
Comment on lines
477
to
+480
| Nodes = nodes[callId].Where(x => x.DeletedOn == null).OrderBy(x => x.SortOrder).ToList(), | ||
| Assignments = assignments[callId].Where(x => x.ReleasedOn == null).ToList(), | ||
| Objectives = objectives[callId].OrderBy(x => x.SortOrder).ToList(), | ||
| Needs = needs[callId].Where(x => x.Status != (int)IncidentNeedStatus.Cancelled).OrderBy(x => x.SortOrder).ToList(), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Board vs. bundle Needs diverge on cancelled needs.
GetBundleForDepartmentAsync filters Status != Cancelled, but GetCommandBoardAsync populates Needs via GetNeedsForCallAsync (Line 1626), which returns cancelled needs too. So the live polled board and the offline bundle return different need sets for the same call, and the comment on Lines 475-476 ("mirror the per-call getter filters exactly") no longer holds. Align them — either add the cancelled filter to GetNeedsForCallAsync/board or drop it from the bundle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/IncidentCommandService.cs` around lines 477 - 480, The
per-call board and bundle need collections apply inconsistent cancelled-status
filtering. Align GetNeedsForCallAsync and the GetCommandBoardAsync path with
GetBundleForDepartmentAsync so both exclude IncidentNeedStatus.Cancelled,
preserving the existing ordering and other filters.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
2 participants
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
New Features
Documentation