Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds command-lane requirement persistence, hydration, reconciliation, enforcement, warnings, API exposure, and web editing support. It also adds weather-alert push metadata, removes command-role certification mappings, updates migration safeguards, and normalizes release-note headings. ChangesCommand board lane requirements
Weather alert notifications
Migration and release safeguards
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant APIClient
participant IncidentCommandController
participant IncidentCommandService
participant CommandsService
participant ResourceAssignment
APIClient->>IncidentCommandController: assign or move resource
IncidentCommandController->>IncidentCommandService: submit assignment change
IncidentCommandService->>CommandsService: retrieve lane requirements
IncidentCommandService->>IncidentCommandService: evaluate unit and personnel roles
IncidentCommandService->>ResourceAssignment: persist warning or reject operation
IncidentCommandService-->>IncidentCommandController: return assignment or failure
IncidentCommandController-->>APIClient: return data and message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
Core/Resgrid.Services/CommunicationService.cs (1)
96-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse string interpolation for improved readability.
As per coding guidelines, prefer modern C# features appropriately. Using string interpolation instead of
string.Formatand string concatenation makes the code more concise and readable.🛠️ Proposed refactor
else { if (message.SystemGenerated) spm.SubTitle = "Msg from System"; else - spm.SubTitle = string.Format("Msg from {0}", sendersName); + spm.SubTitle = $"Msg from {sendersName}"; - spm.Title = "Msg:" + message.Subject.Truncate(200); + spm.Title = $"Msg:{message.Subject.Truncate(200)}"; }🤖 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/CommunicationService.cs` around lines 96 - 115, Update the non-weather-alert title and sender subtitle assignments in the communication message formatting block to use C# string interpolation instead of string.Format and string concatenation, while preserving the existing output text and truncation behavior.Source: Coding guidelines
Core/Resgrid.Model/IncidentCommand/CommandRequirementsNotMetException.cs (1)
10-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd standard exception constructors.
Only a
(string message)constructor is provided. Per .NET conventions (CA1032), custom exceptions should also expose parameterless and(string, Exception)constructors for consistency with the baseExceptionAPI and to support re-throwing with an inner exception.♻️ Suggested additional constructors
public class CommandRequirementsNotMetException : Exception { + public CommandRequirementsNotMetException() + { + } + public CommandRequirementsNotMetException(string message) : base(message) { } + + public CommandRequirementsNotMetException(string message, Exception innerException) : base(message, innerException) + { + } }🤖 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.Model/IncidentCommand/CommandRequirementsNotMetException.cs` around lines 10 - 15, Update CommandRequirementsNotMetException to expose parameterless and (string message, Exception innerException) constructors in addition to the existing message constructor, delegating each to the corresponding Exception base constructor.Core/Resgrid.Services/IncidentCommandService.cs (2)
721-784: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRequirement evaluation logic looks correct; note the per-call
GetUnitTypesForDepartmentAsyncfetch.Unit-type validation re-fetches all department unit types on every
RealUnitassign/move to mapunit.Type(name) back to an id. This is a full department-scoped read on a per-assignment hot path (command board). Given board interactions can be frequent, consider whether this list should be cached (per repo caching guidelines) at theIUnitsServicelayer if not already.🤖 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 721 - 784, The per-assignment unit-type lookup in EvaluateNodeRequirementsAsync repeatedly calls GetUnitTypesForDepartmentAsync, creating unnecessary department-scoped reads on the command-board hot path. Reuse an existing IUnitsService caching mechanism or add repository-standard caching at that service layer for department unit types, while preserving the current name-to-UnitTypeId matching behavior.Source: Coding guidelines
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTwo more constructor-injected dependencies added to an already long constructor.
Per coding guidelines, dependencies should be resolved via
Bootstrapper.GetKernel().Resolve<T>()(Service Locator) rather than constructor injection, and constructor injection should be minimized. This class already uses constructor injection exclusively for ~15 existing dependencies; these two new ones follow that established (if guideline-inconsistent) file-wide convention. Not asking for a rewrite here, but flagging per guidelines for awareness — retrofitting this file's DI style is out of scope for this change.Also applies to: 55-57, 74-75
🤖 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 37 - 38, No change is required for this comment: retain _unitsService and _personnelRolesService as constructor-injected dependencies, consistent with the existing IncidentCommandService pattern. Do not retrofit the class to use Bootstrapper.GetKernel().Resolve<T>() as part of this change.Source: Coding guidelines
Core/Resgrid.Services/CommandsService.cs (2)
116-129: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftNew read paths bypass
ICacheProvider.
HydrateAssignmentsAsyncandGetRoleWithRequirementsAsync(and the callers that route through them) read role/unit-type/personnel-role data directly from repositories on every call, with noICacheProvider.RetrieveAsync<T>()cache-aside wrapper and nobypassCacheparameter. As per coding guidelines, "All caching operations must go throughICacheProvider.Retrieve<T>()orICacheProvider.RetrieveAsync<T>()using the cache-aside pattern with fallback functions" and "Service methods should include abypassCacheparameter (default:false)" forCore/Resgrid.Services/**/*.cs. IfCommandsServicenever cached this data before this PR, this may be an acceptable pre-existing gap rather than a regression — flagging for awareness given these are new read methods on a likely hot path (e.g. resolving the command board for a call).Also applies to: 143-166
🤖 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/CommandsService.cs` around lines 116 - 129, Update GetRoleWithRequirementsAsync and HydrateAssignmentsAsync, plus their relevant callers, to use ICacheProvider.RetrieveAsync<T>() with repository fallback functions for role, unit-type, and personnel-role reads. Add an optional bypassCache parameter defaulting to false and propagate it through the call chain, preserving the current hydrated results and null/empty collection behavior.Source: Coding guidelines
29-44: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPer-command hydration loop N+1s across a department's command boards.
GetAllCommandsForDepartmentAsynccallsHydrateAssignmentsAsynconce per command definition, and each call issues 3 queries (roles, unit types, personnel roles) scoped to that single command. For a department with many command boards, this isO(3n)round-trips instead of a single batched fetch across the whole department (similar to howHydrateAssignmentsAsyncalready batches per-command instead of per-role). Consider adding department-scoped bulk queries for unit types/personnel roles, mirroring the by-command-id queries added in this PR.🤖 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/CommandsService.cs` around lines 29 - 44, The per-command HydrateAssignmentsAsync loop in GetAllCommandsForDepartmentAsync causes three database round-trips per command. Add department-scoped bulk queries for roles, unit types, and personnel roles, then hydrate all returned command definitions from those shared results while preserving each command’s assignments; follow the existing by-command-id query patterns and avoid invoking HydrateAssignmentsAsync once per command.
🤖 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 @.github/workflows/dotnet.yml:
- Line 146: Update the replacement in the workflow’s PR-body transformation to
match only the Markdown section heading “Pull Request Description,” including
its heading syntax, rather than every case-insensitive occurrence. Preserve
prose and code-block content while still renaming the intended heading to
“Summary.”
In `@Core/Resgrid.Model/Services/ICommandsService.cs`:
- Around line 39-46: Update ICommandsService.GetRoleWithRequirementsAsync to
accept an optional bool bypassCache parameter defaulting to false, and propagate
this parameter through its implementation and callers, especially
IncidentCommandService.EvaluateNodeRequirementsAsync, so live requirement
evaluation can request fresh data when needed.
In `@Core/Resgrid.Services/CommandsService.cs`:
- Around line 46-88: Wrap the full reconciliation flows in Save and DeleteAsync,
including CommandDefinitionRole and requirement updates, in a shared IUnitOfWork
transaction with commit on success and rollback on failure. Ensure all
repository operations use that same unit-of-work boundary, and preserve the
existing null-versus-empty assignment behavior while preventing concurrent saves
from reconciling against an unprotected existing snapshot.
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 638-656: The node validation in MoveResourceAsync must also
require node.CallId to match the assignment’s CallId before evaluating
requirements or persisting CommandStructureNodeId. Update the existing node !=
null and DepartmentId condition to include the CallId match, preserving
rejection/prevention of foreign-incident lanes.
In `@Providers/Resgrid.Providers.Migrations/Sql/EF0001_PopulateOIDCDb.sql`:
- Around line 14-20: Update the guard around the EF0001 migration-history insert
in Providers/Resgrid.Providers.Migrations/Sql/EF0001_PopulateOIDCDb.sql (lines
14-20) to verify every table or object created by CreateOpenIddictModels, not
just AspNetRoles, before stamping the migration. Apply the same complete-schema
validation in
Providers/Resgrid.Providers.Migrations/Sql/EF0003_MigrateOIDCDbToV5.sql (lines
6-10): when Type is absent, require all V5 columns before inserting the
migration-history row.
In
`@Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs`:
- Around line 41-48: The three repository constructors in
Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs
at lines 41-48, 106-112, and 133-139—CommandDefinitionRoleRepository,
CommandDefinitionRoleUnitTypeRepository, and
CommandDefinitionRolePersonnelRoleRepository—must stop accepting injected
dependencies and resolve each dependency required by the base constructor
through Bootstrapper.GetKernel().Resolve<T>(). Preserve the existing
base-constructor dependency order and repository initialization behavior at all
three sites.
- Around line 69-71: Update the QueryAsync delegate to access the transaction
through the nullable unitOfWork parameter, matching the existing
unitOfWork?.Connection null check and preventing a NullReferenceException when
unitOfWork is absent.
In `@Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs`:
- Around line 264-277: Add Resgrid.Framework.Logging.LogException(ex) at the
start of both CommandRequirementsNotMetException catch blocks in AssignResource
and MoveResource within IncidentCommandController.cs:264-277 and
IncidentCommandController.cs:304-317, before setting result.Status. No other
response behavior should change.
In `@Web/Resgrid.Web/Areas/User/Controllers/CommandController.cs`:
- Around line 156-166: Change the CommandController.Delete action from HttpGet
to HttpPost and add ValidateAntiForgeryToken, preserving its existing
authorization, ownership check, deletion, and redirect behavior. Update the
corresponding delete control in Index.cshtml to submit a POST form carrying the
anti-forgery token instead of navigating through an anchor link.
- Around line 60-87: The New and Edit POST actions in CommandController must
validate the posted SelectedType against the current department’s call-type
collection before assigning CallTypeId. Reuse the department call-type data
loaded by PopulateSupportingDataAsync or the established source, accept only a
valid department-associated value, and otherwise assign null or reject
validation; apply this change at both CommandController.cs ranges 60-87 (New)
and 107-137 (Edit).
---
Nitpick comments:
In `@Core/Resgrid.Model/IncidentCommand/CommandRequirementsNotMetException.cs`:
- Around line 10-15: Update CommandRequirementsNotMetException to expose
parameterless and (string message, Exception innerException) constructors in
addition to the existing message constructor, delegating each to the
corresponding Exception base constructor.
In `@Core/Resgrid.Services/CommandsService.cs`:
- Around line 116-129: Update GetRoleWithRequirementsAsync and
HydrateAssignmentsAsync, plus their relevant callers, to use
ICacheProvider.RetrieveAsync<T>() with repository fallback functions for role,
unit-type, and personnel-role reads. Add an optional bypassCache parameter
defaulting to false and propagate it through the call chain, preserving the
current hydrated results and null/empty collection behavior.
- Around line 29-44: The per-command HydrateAssignmentsAsync loop in
GetAllCommandsForDepartmentAsync causes three database round-trips per command.
Add department-scoped bulk queries for roles, unit types, and personnel roles,
then hydrate all returned command definitions from those shared results while
preserving each command’s assignments; follow the existing by-command-id query
patterns and avoid invoking HydrateAssignmentsAsync once per command.
In `@Core/Resgrid.Services/CommunicationService.cs`:
- Around line 96-115: Update the non-weather-alert title and sender subtitle
assignments in the communication message formatting block to use C# string
interpolation instead of string.Format and string concatenation, while
preserving the existing output text and truncation behavior.
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 721-784: The per-assignment unit-type lookup in
EvaluateNodeRequirementsAsync repeatedly calls GetUnitTypesForDepartmentAsync,
creating unnecessary department-scoped reads on the command-board hot path.
Reuse an existing IUnitsService caching mechanism or add repository-standard
caching at that service layer for department unit types, while preserving the
current name-to-UnitTypeId matching behavior.
- Around line 37-38: No change is required for this comment: retain
_unitsService and _personnelRolesService as constructor-injected dependencies,
consistent with the existing IncidentCommandService pattern. Do not retrofit the
class to use Bootstrapper.GetKernel().Resolve<T>() as part of this change.
🪄 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: 400aba94-086c-466a-b8da-4dd475f3209c
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Services/CommandsServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**
📒 Files selected for processing (46)
.github/workflows/dotnet.ymlCore/Resgrid.Model/CommandDefinitionRole.csCore/Resgrid.Model/CommandDefinitionRoleCert.csCore/Resgrid.Model/IncidentCommand/CommandRequirementsNotMetException.csCore/Resgrid.Model/IncidentCommand/CommandStructureNode.csCore/Resgrid.Model/Message.csCore/Resgrid.Model/MessageTypes.csCore/Resgrid.Model/Repositories/ICommandDefinitionRepository.csCore/Resgrid.Model/Services/ICommandsService.csCore/Resgrid.Services/CommandsService.csCore/Resgrid.Services/CommunicationService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/WeatherAlertService.csProviders/Resgrid.Providers.Migrations/Migrations/M0087_RemoveCommandDefinitionRoleCerts.csProviders/Resgrid.Providers.Migrations/Migrations/M0088_AddResourceAssignmentRequirementsWarning.csProviders/Resgrid.Providers.Migrations/Sql/EF0001_PopulateOIDCDb.sqlProviders/Resgrid.Providers.Migrations/Sql/EF0003_MigrateOIDCDbToV5.sqlProviders/Resgrid.Providers.MigrationsPg/Migrations/M0087_RemoveCommandDefinitionRoleCertsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0088_AddResourceAssignmentRequirementsWarningPg.csRepositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.csRepositories/Resgrid.Repositories.DataRepository/Configs/SqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Modules/ApiDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/NonWebDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.csRepositories/Resgrid.Repositories.DataRepository/Queries/Commands/SelectCmdRolePersonnelRolesByCommandIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Commands/SelectCmdRolePersonnelRolesByRoleIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Commands/SelectCmdRoleUnitTypesByCommandIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Commands/SelectCmdRoleUnitTypesByRoleIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Commands/SelectCommandDefinitionRolesByCommandIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web.Services/Controllers/v4/CommandsController.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Models/v4/Commands/CommandModels.csWeb/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.csWeb/Resgrid.Web/Areas/User/Controllers/CommandController.csWeb/Resgrid.Web/Areas/User/Models/Command/CommandIndexView.csWeb/Resgrid.Web/Areas/User/Models/Command/NewCommandView.csWeb/Resgrid.Web/Areas/User/Views/Command/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Command/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Command/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/Command/View.cshtmlWeb/Resgrid.Web/Areas/User/Views/Command/_AssignmentModal.cshtmlWeb/Resgrid.Web/Areas/User/Views/Command/_CommandForm.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/command/resgrid.commands.newcommand.js
💤 Files with no reviewable changes (1)
- Core/Resgrid.Model/CommandDefinitionRoleCert.cs
| if (pr) { | ||
| const body = (pr.body || '') | ||
| .replace(/##\s*Summary by CodeRabbit[\s\S]*/i, '') | ||
| .replace(/Pull Request Description/gi, 'Summary') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the replacement to the section heading.
Line 146 rewrites every occurrence in the PR body, including prose and code blocks. Match only the intended Markdown heading so release-note content is preserved.
🤖 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 @.github/workflows/dotnet.yml at line 146, Update the replacement in the
workflow’s PR-body transformation to match only the Markdown section heading
“Pull Request Description,” including its heading syntax, rather than every
case-insensitive occurrence. Preserve prose and code-block content while still
renaming the intended heading to “Summary.”
| /// <summary> | ||
| /// Gets a single lane (role) with its requirement sets (RequiredUnitTypes/RequiredRoles) hydrated. | ||
| /// Used to enforce lane requirements when assigning resources on the live command board. | ||
| /// </summary> | ||
| /// <param name="commandDefinitionRoleId">The lane (command definition role) identifier.</param> | ||
| /// <returns>Task<CommandDefinitionRole>.</returns> | ||
| Task<CommandDefinitionRole> GetRoleWithRequirementsAsync(int commandDefinitionRoleId); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a bypassCache parameter to GetRoleWithRequirementsAsync.
Per coding guidelines, service methods should expose a bypassCache (default false) parameter so callers can force fresh data. This method drives live enforcement/warning decisions on every resource assign/move (IncidentCommandService.EvaluateNodeRequirementsAsync); without a bypass option, a lane-requirement edit could be enforced against stale cached data until the cache expires. As per coding guidelines: "Service methods should include a bypassCache parameter (default: false) to allow callers to skip cache retrieval when fresh data is required."
♻️ Suggested signature change
- Task<CommandDefinitionRole> GetRoleWithRequirementsAsync(int commandDefinitionRoleId);
+ Task<CommandDefinitionRole> GetRoleWithRequirementsAsync(int commandDefinitionRoleId, bool bypassCache = false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Gets a single lane (role) with its requirement sets (RequiredUnitTypes/RequiredRoles) hydrated. | |
| /// Used to enforce lane requirements when assigning resources on the live command board. | |
| /// </summary> | |
| /// <param name="commandDefinitionRoleId">The lane (command definition role) identifier.</param> | |
| /// <returns>Task<CommandDefinitionRole>.</returns> | |
| Task<CommandDefinitionRole> GetRoleWithRequirementsAsync(int commandDefinitionRoleId); | |
| /// <summary> | |
| /// Gets a single lane (role) with its requirement sets (RequiredUnitTypes/RequiredRoles) hydrated. | |
| /// Used to enforce lane requirements when assigning resources on the live command board. | |
| /// </summary> | |
| /// <param name="commandDefinitionRoleId">The lane (command definition role) identifier.</param> | |
| /// <returns>Task<CommandDefinitionRole>.</returns> | |
| Task<CommandDefinitionRole> GetRoleWithRequirementsAsync(int commandDefinitionRoleId, bool bypassCache = false); |
🤖 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.Model/Services/ICommandsService.cs` around lines 39 - 46, Update
ICommandsService.GetRoleWithRequirementsAsync to accept an optional bool
bypassCache parameter defaulting to false, and propagate this parameter through
its implementation and callers, especially
IncidentCommandService.EvaluateNodeRequirementsAsync, so live requirement
evaluation can request fresh data when needed.
Source: Coding guidelines
| public CommandDefinitionRoleRepository(IConnectionProvider connectionProvider, SqlConfiguration sqlConfiguration, IUnitOfWork unitOfWork, IQueryFactory queryFactory) | ||
| : base(connectionProvider, sqlConfiguration, unitOfWork, queryFactory) | ||
| { | ||
| _connectionProvider = connectionProvider; | ||
| _sqlConfiguration = sqlConfiguration; | ||
| _queryFactory = queryFactory; | ||
| _unitOfWork = unitOfWork; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use Service Locator pattern for dependencies.
As per coding guidelines, use the Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection. Update these new repository implementations to comply with the guideline by resolving the arguments injected into the base class.
Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs#L41-L48: Refactor theCommandDefinitionRoleRepositoryconstructor to resolve dependencies via theBootstrapper.Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs#L106-L112: Refactor theCommandDefinitionRoleUnitTypeRepositoryconstructor to resolve dependencies via theBootstrapper.Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs#L133-L139: Refactor theCommandDefinitionRolePersonnelRoleRepositoryconstructor to resolve dependencies via theBootstrapper.
📍 Affects 1 file
Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs#L41-L48(this comment)Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs#L106-L112Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs#L133-L139
🤖 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
`@Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs`
around lines 41 - 48, The three repository constructors in
Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs
at lines 41-48, 106-112, and 133-139—CommandDefinitionRoleRepository,
CommandDefinitionRoleUnitTypeRepository, and
CommandDefinitionRolePersonnelRoleRepository—must stop accepting injected
dependencies and resolve each dependency required by the base constructor
through Bootstrapper.GetKernel().Resolve<T>(). Preserve the existing
base-constructor dependency order and repository initialization behavior at all
three sites.
Source: Coding guidelines
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ✅ |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Services/IncidentCommandService.cs (1)
647-657: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winReject assignments targeting invalid or foreign-incident lanes.
The current logic only skips the requirement evaluation if a targeted lane is invalid or belongs to another call. It leaves the foreign
CommandStructureNodeIdintact on theassignmentobject, allowing it to be persisted later in the method. This introduces cross-incident data corruption by linking a resource assignment to a lane that doesn't belong to the incident.If an invalid or foreign lane is explicitly targeted, the assignment must be completely rejected by returning
null(mirroring the safe behavior already implemented inMoveResourceAsync).🐛 Proposed fix
var node = await _commandStructureNodeRepository.GetByIdAsync(assignment.CommandStructureNodeId); - if (node != null && node.DepartmentId == assignment.DepartmentId && node.CallId == assignment.CallId) - { - var (violation, enforced) = await EvaluateNodeRequirementsAsync(node, assignment.DepartmentId, assignment.ResourceKind, assignment.ResourceId); - if (violation != null && enforced) - throw new CommandRequirementsNotMetException(violation); - - assignment.RequirementsWarning = violation != null; - assignment.RequirementsWarningMessage = violation; - } + if (node == null || node.DepartmentId != assignment.DepartmentId || node.CallId != assignment.CallId) + return null; + + var (violation, enforced) = await EvaluateNodeRequirementsAsync(node, assignment.DepartmentId, assignment.ResourceKind, assignment.ResourceId); + if (violation != null && enforced) + throw new CommandRequirementsNotMetException(violation); + + assignment.RequirementsWarning = violation != null; + assignment.RequirementsWarningMessage = violation;🤖 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 647 - 657, Update the assignment flow around _commandStructureNodeRepository.GetByIdAsync and the node validation condition to return null immediately when an explicitly targeted node is missing or its DepartmentId or CallId does not match the assignment. Preserve the existing requirement evaluation for valid nodes, including violation handling and warning fields, and mirror the rejection behavior used by MoveResourceAsync.
🧹 Nitpick comments (1)
Core/Resgrid.Services/IncidentCommandService.cs (1)
55-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve newly added dependencies using the Service Locator pattern.
The constructor adds
IUnitsServiceandIPersonnelRolesServicevia parameter injection. As per coding guidelines, dependencies should be resolved explicitly usingBootstrapper.GetKernel().Resolve<T>()in the constructor to minimize the number of injected parameters.♻️ Proposed refactor
- ICoreEventService coreEventService, - IUnitsService unitsService, - IPersonnelRolesService personnelRolesService) + ICoreEventService coreEventService) { _incidentCommandRepository = incidentCommandRepository; _commandStructureNodeRepository = commandStructureNodeRepository; _resourceAssignmentRepository = resourceAssignmentRepository; _tacticalObjectiveRepository = tacticalObjectiveRepository; _incidentTimerRepository = incidentTimerRepository; _incidentMapAnnotationRepository = incidentMapAnnotationRepository; _commandLogEntryRepository = commandLogEntryRepository; _commandTransferRepository = commandTransferRepository; _commandsService = commandsService; _callsService = callsService; _checkInTimerService = checkInTimerService; _incidentVoiceService = incidentVoiceService; _incidentRoleAssignmentRepository = incidentRoleAssignmentRepository; _eventAggregator = eventAggregator; _coreEventService = coreEventService; - _unitsService = unitsService; - _personnelRolesService = personnelRolesService; + _unitsService = Bootstrapper.GetKernel().Resolve<IUnitsService>(); + _personnelRolesService = Bootstrapper.GetKernel().Resolve<IPersonnelRolesService>(); }🤖 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 55 - 76, Replace the newly injected IUnitsService and IPersonnelRolesService constructor parameters with explicit Service Locator resolution inside the constructor. Assign both fields using Bootstrapper.GetKernel().Resolve<T>(), and leave the existing dependency injection and field assignments unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 647-657: Update the assignment flow around
_commandStructureNodeRepository.GetByIdAsync and the node validation condition
to return null immediately when an explicitly targeted node is missing or its
DepartmentId or CallId does not match the assignment. Preserve the existing
requirement evaluation for valid nodes, including violation handling and warning
fields, and mirror the rejection behavior used by MoveResourceAsync.
---
Nitpick comments:
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 55-76: Replace the newly injected IUnitsService and
IPersonnelRolesService constructor parameters with explicit Service Locator
resolution inside the constructor. Assign both fields using
Bootstrapper.GetKernel().Resolve<T>(), and leave the existing dependency
injection and field assignments unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff978b7f-1a66-44b1-af40-c33fc25ea58e
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Services/CommandsServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**
📒 Files selected for processing (8)
Core/Resgrid.Services/CommandsService.csCore/Resgrid.Services/IncidentCommandService.csProviders/Resgrid.Providers.Migrations/Sql/EF0001_PopulateOIDCDb.sqlProviders/Resgrid.Providers.Migrations/Sql/EF0003_MigrateOIDCDbToV5.sqlRepositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web/Areas/User/Controllers/CommandController.csWeb/Resgrid.Web/Areas/User/Views/Command/Index.cshtml
🚧 Files skipped from review as they are similar to previous changes (7)
- Web/Resgrid.Web/Areas/User/Views/Command/Index.cshtml
- Providers/Resgrid.Providers.Migrations/Sql/EF0003_MigrateOIDCDbToV5.sql
- Providers/Resgrid.Providers.Migrations/Sql/EF0001_PopulateOIDCDb.sql
- Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs
- Repositories/Resgrid.Repositories.DataRepository/CommandDefinitionRepository.cs
- Core/Resgrid.Services/CommandsService.cs
- Web/Resgrid.Web/Areas/User/Controllers/CommandController.cs
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| foreach (var command in list) | ||
| await HydrateAssignmentsAsync(command); |
There was a problem hiding this comment.
N+1 query in GetAllCommandsForDepartmentAsync: the loop calls HydrateAssignmentsAsync per command, and each call issues up to 3 separate DB queries (GetRolesByCommandDefinitionIdAsync, GetUnitTypesByCommandDefinitionIdAsync, GetPersonnelRolesByCommandDefinitionIdAsync), producing 3N+1 queries for N commands. Batch-load all roles, unit types, and personnel roles by command IDs in 3 fixed queries and distribute them in memory to reduce the query count to 4 regardless of command count.
Kody rule violation: Detect N+1 style queries and suggest batching
// Batch-load all child data for the department in 3 queries (not 3N):
var list = items.ToList();
var commandIds = list.Select(c => c.CommandDefinitionId).ToList();
var allRoles = (await _commandDefinitionRoleRepository.GetRolesByCommandDefinitionIdsAsync(commandIds))?.ToList() ?? new List<CommandDefinitionRole>();
var allUnitTypes = (await _commandDefinitionRoleUnitTypeRepository.GetUnitTypesByCommandDefinitionIdsAsync(commandIds))?.ToList() ?? new List<CommandDefinitionRoleUnitType>();
var allPersonnelRoles = (await _commandDefinitionRolePersonnelRoleRepository.GetPersonnelRolesByCommandDefinitionIdsAsync(commandIds))?.ToList() ?? new List<CommandDefinitionRolePersonnelRole>();
foreach (var command in list)
{
command.Assignments = allRoles.Where(r => r.CommandDefinitionId == command.CommandDefinitionId).OrderBy(r => r.SortOrder).ToList();
foreach (var role in command.Assignments)
{
role.RequiredUnitTypes = allUnitTypes.Where(u => u.CommandDefinitionRoleId == role.CommandDefinitionRoleId).ToList();
role.RequiredRoles = allPersonnelRoles.Where(p => p.CommandDefinitionRoleId == role.CommandDefinitionRoleId).ToList();
}
}
return list;Prompt for LLM
File Core/Resgrid.Services/CommandsService.cs:
Line 41 to 42:
Violates rule 'Detect N+1 style queries and suggest batching': GetAllCommandsForDepartmentAsync iterates over every command definition calling await HydrateAssignmentsAsync(command) in a loop. HydrateAssignmentsAsync (line 177) issues up to 3 separate DB queries per command (GetRolesByCommandDefinitionIdAsync, GetUnitTypesByCommandDefinitionIdAsync, GetPersonnelRolesByCommandDefinitionIdAsync). For N commands this results in up to 3N+1 queries. This method is invoked from SyncService (delta sync for offline clients), the web Index page, and the v4 API list endpoint, making the N+1 impact significant on sync cycles. The fix is to batch: load all roles for all command IDs in a single query, then all unit types and personnel roles for those roles, and distribute them in memory — reducing 3N+1 queries to 4 regardless of command count.
Suggested Code:
// Batch-load all child data for the department in 3 queries (not 3N):
var list = items.ToList();
var commandIds = list.Select(c => c.CommandDefinitionId).ToList();
var allRoles = (await _commandDefinitionRoleRepository.GetRolesByCommandDefinitionIdsAsync(commandIds))?.ToList() ?? new List<CommandDefinitionRole>();
var allUnitTypes = (await _commandDefinitionRoleUnitTypeRepository.GetUnitTypesByCommandDefinitionIdsAsync(commandIds))?.ToList() ?? new List<CommandDefinitionRoleUnitType>();
var allPersonnelRoles = (await _commandDefinitionRolePersonnelRoleRepository.GetPersonnelRolesByCommandDefinitionIdsAsync(commandIds))?.ToList() ?? new List<CommandDefinitionRolePersonnelRole>();
foreach (var command in list)
{
command.Assignments = allRoles.Where(r => r.CommandDefinitionId == command.CommandDefinitionId).OrderBy(r => r.SortOrder).ToList();
foreach (var role in command.Assignments)
{
role.RequiredUnitTypes = allUnitTypes.Where(u => u.CommandDefinitionRoleId == role.CommandDefinitionRoleId).ToList();
role.RequiredRoles = allPersonnelRoles.Where(p => p.CommandDefinitionRoleId == role.CommandDefinitionRoleId).ToList();
}
}
return list;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (CommandRequirementsNotMetException ex) | ||
| { | ||
| Resgrid.Framework.Logging.LogException(ex); | ||
| // The target lane forces unit-type/personnel-role requirements the resource doesn't meet. |
There was a problem hiding this comment.
Missing error context in structured logs: the catch blocks at lines 272 and 313 call Resgrid.Framework.Logging.LogException(ex) without the extraMessage parameter, dropping identifiers like DepartmentId, assignment.IncidentCommandId, assignment.CommandStructureNodeId, input.ResourceAssignmentId, and input.TargetNodeId needed to diagnose which department, incident, or lane triggered the CommandRequirementsNotMetException.
Kody rule violation: Include error context in structured logs
catch (CommandRequirementsNotMetException ex)
{
Resgrid.Framework.Logging.LogException(ex, $"AssignResource failed: Dept={DepartmentId}, Cmd={assignment.IncidentCommandId}, Node={assignment.CommandStructureNodeId}, Resource={assignment.ResourceId}");Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs:
Line 270 to 273:
Violates rule 'Include error context in structured logs': both new catch blocks call Resgrid.Framework.Logging.LogException(ex) with no extraMessage. The LogException method (Logging.cs:65) accepts an extraMessage parameter for structured context, and other controllers in the codebase use it (e.g., InboxController passes message IDs). The catch blocks at lines 272 and 313 omit relevant identifiers such as DepartmentId, assignment.IncidentCommandId, assignment.CommandStructureNodeId (line 272), and input.ResourceAssignmentId, input.TargetNodeId (line 313) that are needed to diagnose which department, incident, or lane triggered the requirement rejection.
Suggested Code:
catch (CommandRequirementsNotMetException ex)
{
Resgrid.Framework.Logging.LogException(ex, $"AssignResource failed: Dept={DepartmentId}, Cmd={assignment.IncidentCommandId}, Node={assignment.CommandStructureNodeId}, Resource={assignment.ResourceId}");
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }); | ||
|
|
||
| function addAssignment() { | ||
| var name = $.trim($('#assignment-name').val()); |
There was a problem hiding this comment.
Inconsistent variable declaration in addAssignment(): 10 local variables (name, index, description, laneType, laneTypeName, row, nameCell, laneCell, descriptionCell, actionCell) use var while the same function body already declares forceRequirements, unitTypeIds, unitTypeNames, roleIds, roleNames, and requirementsCell with const. None of the var-declared variables are reassigned, so they should all use const.
Kody rule violation: Always use const and let
const name = $.trim($('#assignment-name').val());
...
const index = newcommand.assignmentCount++;
const description = $('#description-text').val();
const laneType = $('#assignment-lanetype').val() || '0';
const laneTypeName = $('#assignment-lanetype option:selected').text();
...
const row = $('<tr></tr>');
const nameCell = $('<td style="max-width: 215px;"></td>').text(name);
...
const laneCell = $('<td></td>').text(laneTypeName);
...
const descriptionCell = $('<td></td>').text(description);
...
const actionCell = $('<td style="text-align:center;"></td>');Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/command/resgrid.commands.newcommand.js:
Line 28:
Violates rule 'Always use const and let': the rewritten addAssignment() function declares 10 local variables with var (name, index, description, laneType, laneTypeName, row, nameCell, laneCell, descriptionCell, actionCell). None are reassigned, so they should use const (or let for row which is captured in a closure but never reassigned). The same function already uses const for forceRequirements, unitTypeIds, unitTypeNames, roleIds, roleNames, and requirementsCell, making the var usage inconsistent within the same function body.
Suggested Code:
const name = $.trim($('#assignment-name').val());
...
const index = newcommand.assignmentCount++;
const description = $('#description-text').val();
const laneType = $('#assignment-lanetype').val() || '0';
const laneTypeName = $('#assignment-lanetype option:selected').text();
...
const row = $('<tr></tr>');
const nameCell = $('<td style="max-width: 215px;"></td>').text(name);
...
const laneCell = $('<td></td>').text(laneTypeName);
...
const descriptionCell = $('<td></td>').text(description);
...
const actionCell = $('<td style="text-align:center;"></td>');
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
Pull Request Description
RG-T125: IC Bug Fixes and Weather Alert Fixes
This pull request delivers a comprehensive overhaul of the Command Board template system for Incident Command, fixes weather alert notifications, and hardens several OIDC database migration paths.
1. Command Board Templates — Full Lifecycle & Persistence
Previously, command board definitions stored their lanes (Divisions, Groups, Staging, etc.) but the lanes' required unit types and required personnel roles were never reliably persisted, hydrated, or reconciled. This PR implements:
nulllane collection leaves existing lanes untouched; an empty collection clears them.GetAllCommandsForDepartmentAsync,GetCommandByIdAsync, andGetCommandForCallTypeAsyncnow return lanes with their requirement sets populated and ordered bySortOrder.2. Lane Requirement Enforcement (Forced vs. Advisory)
When a resource is assigned to or moved into a lane whose template role defines requirements:
ForceRequirements = true): the assignment/move is rejected withCommandRequirementsNotMetExceptionif an own-department unit doesn't match a required unit type, or personnel don't hold a required role. The v4 API surfaces the reason as a failure message.ForceRequirements = false): the assignment succeeds, but the violation is stamped on the assignment (RequirementsWarning,RequirementsWarningMessage) so the IC app can render a warning indicator on the resource chip — including via offline delta sync.M0088) for the two newResourceAssignmentscolumns.3. Auto-Apply Default Board on Command Establishment
When command is established on a call, the system now auto-resolves the department's template for that call's type (falling back to the "Any Call Type" board), seeds the board lanes in template order, and starts the template's repeating benchmark timer (if configured). Previously, lane seeding only happened when an explicit
commandDefinitionIdwas passed and lane data happened to be loaded.4. Removal of Certification Requirements (Cleanup)
The
CommandDefinitionRoleCert/RequiredCertsconcept — never functional — is removed. The model property is deleted, the orphanedCommandDefinitionRoleCertstable is dropped via migration (M0087), and the UI no longer collects certifications.5. Weather Alert Notification Fixes
MessageTypes.WeatherAlert = 3."Msg:"prefix, and a custom push subtitle (the alert headline) instead of"Msg from System".Message.PushSubTitleso the weather service can supply the headline.6. OIDC SQL Migration Hardening
The EF OIDC bootstrap and V5-migration SQL scripts now correctly detect databases that are already V5-shaped (created via EF
EnsureCreated) and stamp the migration history row, rather than attempting aType → ClientTyperename that fails when theTypecolumn doesn't exist. Each guard now requires all expected columns/tables to be present before stamping, preventing false-positive skips on partial schemas.7. Command Boards Web UI (Admin)
The legacy, half-functional New/Edit/View pages for command definitions are replaced with a working Command Boards management UI:
POSTform with anti-forgery._CommandFormpartial; the assignment modal uses real multi-select pickers populated from the department's unit types and personnel roles (replacing the previous AJAX text inputs that pointed at non-existent endpoints). Lane type, description, requirements, and "Force Requirements" are all captured. The View page renders a read-only summary of lanes and their requirements.ParseAssignmentsFromForm) assignsSortOrderfrom row order and normalizes the posted CSV id lists.8. API v4 Updates
CommandsController.SaveCommandnow persistsRequiredUnitTypesandRequiredPersonnelRolesper lane, andConvertCommandDatareturns them.IncidentCommandControllerassign/move endpoints catchCommandRequirementsNotMetException(returning a failure with the reason) and surface advisory warnings via the responseMessage.9. CI / Release Notes
The release-notes extraction step now also normalizes
Pull Request Descriptionheadings toSummary.10. Tests
Added comprehensive unit tests covering: lane hydration, call-type resolution with "Any Call Type" fallback, Save reconcile (insert/update/delete, foreign-id protection, null handling, requirement replacement), cascade delete,
GetRoleWithRequirementsAsync, and the full lane-requirement enforcement matrix (forced reject, advisory warning stamp/clear, mutual-aid exemption, foreign-call-lane exemption, move rejection).Migrations:
M0087(dropCommandDefinitionRoleCerts) andM0088(addRequirementsWarning/RequirementsWarningMessagetoResourceAssignments), shipped for both SQL Server and PostgreSQL.