Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughAdds a full Incident Command System feature with new command, resource, voice, reporting, role, and mutual-aid models and services; database migrations and repository wiring; v4 API controllers and DTOs; a PAR evaluation worker; and a SqlClient package swap. ChangesIncident Command System
Microsoft.Data.SqlClient Upgrade
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 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}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (25)
Web/Resgrid.Web.Services/Controllers/v4/CommandsController.cs-28-31 (1)
28-31: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
Bootstrapper.GetKernel().Resolve<T>()in the constructor to match project convention.This constructor is using direct constructor injection for
ICommandsService, which conflicts with the repository’s required dependency-resolution pattern.
As per coding guidelines, "UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection".🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/CommandsController.cs` around lines 28 - 31, The CommandsController constructor is using constructor injection for ICommandsService, but the project convention requires using the Service Locator pattern. Remove the ICommandsService parameter from the CommandsController constructor and instead assign _commandsService by calling Bootstrapper.GetKernel().Resolve<ICommandsService>() inside the constructor body. This aligns the dependency resolution with the project's required pattern.Source: Coding guidelines
Web/Resgrid.Web.Services/Controllers/v4/MutualAidController.cs-24-27 (1)
24-27: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign constructor dependency resolution with the project’s required pattern.
IMutualAidServiceis injected directly here, but the project convention for*.csrequires resolving dependencies throughBootstrapper.GetKernel().Resolve<T>()in constructors.
As per coding guidelines, "UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection".🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/MutualAidController.cs` around lines 24 - 27, The MutualAidController constructor is using constructor injection for the IMutualAidService dependency, but the project convention requires using the Service Locator pattern instead. Remove the IMutualAidService parameter from the MutualAidController constructor signature and resolve the dependency directly inside the constructor body using Bootstrapper.GetKernel().Resolve<IMutualAidService>() to assign it to the _mutualAidService field, aligning with the project's required dependency resolution pattern.Source: Coding guidelines
Web/Resgrid.Web.Services/Controllers/v4/MutualAidController.cs-40-41 (1)
40-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize null service output before computing
PageSize.If
GetAssignableResourcesForIncidentAsyncreturnsnull,result.Data.Countwill throw.Null-safe mapping
- result.Data = await _mutualAidService.GetAssignableResourcesForIncidentAsync(DepartmentId); + result.Data = await _mutualAidService.GetAssignableResourcesForIncidentAsync(DepartmentId) ?? new System.Collections.Generic.List<Resgrid.Model.AssignableResource>(); result.PageSize = result.Data.Count;🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/MutualAidController.cs` around lines 40 - 41, In the MutualAidController, the code assigns the result of GetAssignableResourcesForIncidentAsync to result.Data and immediately accesses result.Data.Count without null checking. If GetAssignableResourcesForIncidentAsync returns null, this will throw a NullReferenceException. Add a null check after assigning result.Data to verify it is not null before computing result.PageSize. If result.Data is null, set result.PageSize to 0 or an appropriate default value, otherwise set it to result.Data.Count.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-1034-1062 (1)
1034-1062: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAvoid binding domain entities directly in public API inputs.
These endpoints accept
Resgrid.Model.*types (CommandStructureNode,ResourceAssignment,TacticalObjective,IncidentTimer,IncidentMapAnnotation,IncidentAdHocUnit,IncidentAdHocPersonnel,IncidentRoleAssignment) as request contracts. That leaks internal domain shape to clients and increases over-posting risk. Prefer dedicatedModels.v4.IncidentCommand.*InputDTOs with explicit mapping.Also applies to: 1086-1096, 1116-1117
🤖 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 `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 1034 - 1062, The SaveNode, AssignResource, SaveObjective, StartTimer, and SaveAnnotation methods in IncidentCommandController are accepting domain model entities (CommandStructureNode, ResourceAssignment, TacticalObjective, IncidentTimer, IncidentMapAnnotation) directly as request parameters, which exposes internal domain shapes to clients and increases over-posting risk. Create dedicated Input DTOs in the Models.v4.IncidentCommand namespace for each of these methods (e.g., CommandStructureNodeInput, ResourceAssignmentInput, TacticalObjectiveInput, IncidentTimerInput, IncidentMapAnnotationInput), update the method signatures to accept these DTOs instead of the domain entities, and add explicit mapping logic inside each controller method to convert the Input DTOs to the appropriate domain models before processing. Apply the same pattern to the other domain entity bindings mentioned at lines 1086-1096 and 1116-1117.Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs-166-167 (1)
166-167: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
result.Databefore calling.Countin list endpoints.Line 167 and Line 457 dereference
result.Data.Countwithout null protection. If the service returnsnull, these endpoints will throw and return 500.Suggested patch
- result.Data = await _incidentCommandService.GetAccountabilityForCallAsync(DepartmentId, callId); - result.PageSize = result.Data.Count; + result.Data = await _incidentCommandService.GetAccountabilityForCallAsync(DepartmentId, callId); + result.PageSize = result.Data?.Count ?? 0; ... - result.Data = await _incidentCommandService.GetTimelineForCallAsync(DepartmentId, callId); - result.PageSize = result.Data.Count; + result.Data = await _incidentCommandService.GetTimelineForCallAsync(DepartmentId, callId); + result.PageSize = result.Data?.Count ?? 0;Also applies to: 456-457
🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs` around lines 166 - 167, Add null protection before dereferencing result.Data with the .Count property in the GetAccountabilityForCallAsync service call at line 167, and also apply the same fix at line 457. After assigning the result from the service call, check if result.Data is not null before accessing .Count; if it is null, set result.PageSize to 0 or handle it appropriately to prevent NullReferenceException when the service returns null.Web/Resgrid.Web.Services/Controllers/v4/IncidentResourcesController.cs-70-71 (1)
70-71: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent null dereference when computing
PageSizefrom service lists.Line 71 and Line 130 assume
result.Datais always non-null. A null service response will throw and return 500.Suggested patch
- result.Data = await _incidentResourcesService.GetAdHocUnitsForCallAsync(DepartmentId, callId); - result.PageSize = result.Data.Count; + result.Data = await _incidentResourcesService.GetAdHocUnitsForCallAsync(DepartmentId, callId); + result.PageSize = result.Data?.Count ?? 0; ... - result.Data = await _incidentResourcesService.GetAdHocPersonnelForCallAsync(DepartmentId, callId); - result.PageSize = result.Data.Count; + result.Data = await _incidentResourcesService.GetAdHocPersonnelForCallAsync(DepartmentId, callId); + result.PageSize = result.Data?.Count ?? 0;Also applies to: 129-130
🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentResourcesController.cs` around lines 70 - 71, The GetAdHocUnitsForCallAsync method call may return null, and accessing the Count property on result.Data without null checking will cause a NullReferenceException and return a 500 error. Add a null check before accessing result.Data.Count to assign the PageSize property. This same issue also occurs at lines 129-130 where another service method result is accessed without null validation. Use a null-coalescing operator or conditional check to safely handle null service responses.Web/Resgrid.Web.Services/Controllers/v4/IncidentVoiceController.cs-64-65 (1)
64-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid null dereference in channel list pagination.
Line 65 calls
.Countdirectly onresult.Data; a null service return triggers a 500.Suggested patch
result.Data = await _incidentVoiceService.GetChannelsForCallAsync(DepartmentId, callId); - result.PageSize = result.Data.Count; + result.PageSize = result.Data?.Count ?? 0;🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentVoiceController.cs` around lines 64 - 65, The code on line 65 assigns result.Data from _incidentVoiceService.GetChannelsForCallAsync and immediately calls .Count on it without null checking. If the service returns null, this causes a null dereference exception resulting in a 500 error. Add a null check on result.Data before accessing its Count property, or use a null-coalescing operator to provide a default count of zero when result.Data is null.Web/Resgrid.Web.Services/Controllers/v4/IncidentRolesController.cs-82-83 (1)
82-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNull-protect role list before using
.Count.Line 83 assumes
GetIncidentRolesAsyncnever returns null. If it does, this endpoint fails with 500.Suggested patch
result.Data = await _incidentCommandService.GetIncidentRolesAsync(DepartmentId, callId); - result.PageSize = result.Data.Count; + result.PageSize = result.Data?.Count ?? 0;🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentRolesController.cs` around lines 82 - 83, The code at the GetIncidentRolesAsync call in the IncidentRolesController assumes the return value is never null before accessing .Count on result.Data, which can cause a NullReferenceException if the method returns null. Add a null check on result.Data after the await GetIncidentRolesAsync call and only set result.PageSize with result.Data.Count if result.Data is not null, otherwise set PageSize to 0 or an appropriate default value.Web/Resgrid.Web.Services/Controllers/v4/IncidentVoiceController.cs-77-80 (1)
77-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CloseIncidentChannelsreports success even when operation returns false.Line 79 sets success unconditionally, which can mask failure states and return contradictory payloads (
Data == false,Status == Success).Suggested patch
result.Data = await _incidentVoiceService.CloseIncidentChannelsForCallAsync(DepartmentId, callId, UserId, CancellationToken.None); - result.Status = ResponseHelper.Success; + result.Status = result.Data ? ResponseHelper.Success : ResponseHelper.NotFound; ResponseHelper.PopulateV4ResponseData(result);🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentVoiceController.cs` around lines 77 - 80, The CloseIncidentChannelsForCallAsync method in the IncidentVoiceController is unconditionally setting result.Status to ResponseHelper.Success without checking whether the actual operation succeeded. You need to modify the code to check the boolean value returned from CloseIncidentChannelsForCallAsync before setting the Status property. If the Data value is true, set Status to ResponseHelper.Success, but if it is false, set Status to an appropriate error status using ResponseHelper to reflect the failure condition and prevent contradictory response payloads.Web/Resgrid.Web.Services/Controllers/v4/IncidentReportingController.cs-83-85 (1)
83-85: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle null CSV before UTF-8 encoding.
If
ExportTimelineCsvAsyncreturns null, Line 84 throwsArgumentNullException, causing a 500.Suggested patch
var csv = await _incidentReportingService.ExportTimelineCsvAsync(DepartmentId, callId); + if (string.IsNullOrWhiteSpace(csv)) + return NotFound(); var bytes = Encoding.UTF8.GetBytes(csv); return File(bytes, "text/csv", $"incident-{callId}-timeline.csv");🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentReportingController.cs` around lines 83 - 85, The code does not handle the case where ExportTimelineCsvAsync returns null, which causes ArgumentNullException when null is passed to Encoding.UTF8.GetBytes. Add a null check after the await call to ExportTimelineCsvAsync and before the Encoding.UTF8.GetBytes call. If the csv variable is null, return an appropriate HTTP error response (such as BadRequest or NotFound) instead of proceeding with the encoding and file response.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0077_AddIncidentCommandTablesPg.cs-27-30 (1)
27-30: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce DB uniqueness for one active command per department/call.
Application-level idempotency in
EstablishCommandAsyncis check-then-insert; concurrent requests can still create duplicate active commands because this index is not unique. Add a DB uniqueness constraint (ideally partial on active status) to prevent race-driven duplicate incident command rows.Suggested migration shape
- Create.Index("IX_IncidentCommands_Department_Call".ToLower()) - .OnTable("IncidentCommands".ToLower()) - .OnColumn("DepartmentId".ToLower()).Ascending() - .OnColumn("CallId".ToLower()).Ascending(); + // Keep lookup index if needed, but enforce active-command uniqueness at DB level. + Create.Index("IX_IncidentCommands_Department_Call".ToLower()) + .OnTable("IncidentCommands".ToLower()) + .OnColumn("DepartmentId".ToLower()).Ascending() + .OnColumn("CallId".ToLower()).Ascending(); + + Execute.Sql(@" +CREATE UNIQUE INDEX IF NOT EXISTS ux_incidentcommands_active_dept_call +ON incidentcommands (departmentid, callid) +WHERE status = 0;");And in
Down():Execute.Sql("DROP INDEX IF EXISTS ux_incidentcommands_active_dept_call;");🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0077_AddIncidentCommandTablesPg.cs` around lines 27 - 30, The current index on IncidentCommands table for DepartmentId and CallId is non-unique, allowing concurrent requests to create duplicate active commands. Change the Create.Index call to Create.UniqueIndex to enforce database-level uniqueness. Additionally, add a partial index constraint using a WHERE clause to only enforce uniqueness when the command is in active status, preventing stale inactive commands from blocking new active ones. Update the corresponding Down() method to properly drop this unique index using the appropriate DROP INDEX syntax referencing the index name.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0079_AddIncidentAdHocResourceTablesPg.cs-10-49 (1)
10-49: 🗄️ Data Integrity & Integration | 🟠 MajorReplace culture-sensitive
ToLower()calls with literal lowercase identifiers in migrations.Using
ToLower()on schema identifiers is culture-sensitive—under Turkish and other locales, letters like "I" transform differently (e.g., "I" → "ı" in Turkish), causing the generated table, column, and index names to differ from expectations. This creates schema mismatch risks between development and production environments.Replace all
.ToLower()calls with literal lowercase strings:"IncidentAdHocUnits".ToLower()→"incidentadhocunits".🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0079_AddIncidentAdHocResourceTablesPg.cs` around lines 10 - 49, Replace all culture-sensitive `.ToLower()` method calls with literal lowercase strings in the M0079_AddIncidentAdHocResourceTablesPg migration. For each occurrence, remove the `.ToLower()` call and replace the string with its literal lowercase equivalent—for example, "IncidentAdHocUnits".ToLower() becomes "incidentadhocunits", "DepartmentId".ToLower() becomes "departmentid", and so on. Apply this change to all table names, column names, and index names throughout both the IncidentAdHocUnits and IncidentAdHocPersonnel table creation blocks.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0080_AddIncidentRoleAssignmentTablePg.cs-10-27 (1)
10-27: 🗄️ Data Integrity & Integration | 🟠 MajorUse literal lowercase identifiers instead of
.ToLower()in database migration DDL.
.ToLower()is culture-sensitive and unnecessary for PostgreSQL schema identifiers. PostgreSQL unfolds unquoted identifiers to lowercase, but using explicit literal lowercase strings in migrations is the safest, clearest practice and avoids thread culture dependencies. This pattern affects 57+ migration files across the Pg migrations folder.Suggested fix
- if (!Schema.Table("IncidentRoleAssignments".ToLower()).Exists()) + if (!Schema.Table("incidentroleassignments").Exists()) { - Create.Table("IncidentRoleAssignments".ToLower()) - .WithColumn("IncidentRoleAssignmentId".ToLower()).AsCustom("citext").NotNullable().PrimaryKey() + Create.Table("incidentroleassignments") + .WithColumn("incidentroleassignmentid").AsCustom("citext").NotNullable().PrimaryKey() ... - Create.Index("IX_IncidentRoleAssignments_Department_Call".ToLower()) - .OnTable("IncidentRoleAssignments".ToLower()) + Create.Index("ix_incidentroleassignments_department_call") + .OnTable("incidentroleassignments") ... }🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0080_AddIncidentRoleAssignmentTablePg.cs` around lines 10 - 27, Remove the `.ToLower()` method calls from all string literals in the M0080_AddIncidentRoleAssignmentTablePg migration class and replace them with literal lowercase strings. For each table name, column name, and index name (such as "IncidentRoleAssignments", "IncidentRoleAssignmentId", "IX_IncidentRoleAssignments_Department_Call", etc.), directly write the lowercase version as a string literal instead of calling `.ToLower()` at runtime. This eliminates unnecessary culture-sensitive operations and makes the PostgreSQL schema identifiers explicit and predictable in the migration DDL.Providers/Resgrid.Providers.Migrations/Migrations/M0076_AddCommandDefinitionRoleLaneColumns.cs-10-20 (1)
10-20: 🗄️ Data Integrity & Integration | 🟠 MajorSynchronize Down() with conditional Up() to prevent unsafe rollback.
Lines 10-20 conditionally add columns only if they don't exist, but lines 25-26 unconditionally delete them. If these columns pre-existed before the migration ran,
Up()would skip them yetDown()would remove them—breaking schema reversibility and risking data loss. Use conditional checks inDown()(as seen in M0042) to match the conditionalUp()behavior, or mark the migration as forward-only.Suggested safe pattern (conditional Down)
using FluentMigrator; namespace Resgrid.Providers.Migrations.Migrations { public override void Down() { + if (Schema.Table("CommandDefinitionRoles").Column("LaneType").Exists()) + Delete.Column("LaneType").FromTable("CommandDefinitionRoles"); + + if (Schema.Table("CommandDefinitionRoles").Column("SortOrder").Exists()) + Delete.Column("SortOrder").FromTable("CommandDefinitionRoles"); } }🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0076_AddCommandDefinitionRoleLaneColumns.cs` around lines 10 - 20, The Up() method conditionally adds the LaneType and SortOrder columns to the CommandDefinitionRoles table only if they don't already exist, but the Down() method unconditionally removes them. This breaks migration reversibility because if these columns existed before the migration ran, Up() would skip them but Down() would still delete them. Update the Down() method to include conditional checks matching the Up() method—verify that each column (LaneType and SortOrder) exists on the CommandDefinitionRoles table before attempting to delete it, using the same pattern as the Up() method with Schema.Table().Column().Exists() checks.Providers/Resgrid.Providers.Migrations/Migrations/M0078_AddIncidentScopeToVoiceChannels.cs-10-26 (1)
10-26: 🗄️ Data Integrity & Integration | 🟠 MajorRollback is unsafe when
Up()is conditional.Lines 10-26 use existence checks, but lines 31-33 always drop columns. In environments where columns predate this migration, rollback can remove schema outside this migration's ownership.
Proposed correction
+using System; using FluentMigrator; @@ public override void Down() { - Delete.Column("CallId").FromTable("DepartmentVoiceChannels"); - Delete.Column("IsOnDemand").FromTable("DepartmentVoiceChannels"); - Delete.Column("ClosedOn").FromTable("DepartmentVoiceChannels"); + throw new NotSupportedException( + "M0078 is forward-only: conditional Up() prevents safe ownership-aware rollback."); }🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0078_AddIncidentScopeToVoiceChannels.cs` around lines 10 - 26, The Up method conditionally adds columns to DepartmentVoiceChannels only if they do not already exist, but the Down method (not shown) unconditionally drops these same columns. This creates an unsafe rollback scenario where columns that predate this migration could be removed. Make the Down method mirror the Up method's approach by adding existence checks before dropping each column (CallId, IsOnDemand, and ClosedOn) so that only columns created by this migration are removed during rollback.Providers/Resgrid.Providers.Migrations/Migrations/M0080_AddIncidentRoleAssignmentTable.cs-31-34 (1)
31-34: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
Down()symmetric with conditionalUp()to prevent unintended table deletion.Because
Up()only creates when absent,Down()should also be existence-guarded; otherwise rollback can drop a pre-existing table.Suggested fix
public override void Down() { - Delete.Table("IncidentRoleAssignments"); + if (Schema.Table("IncidentRoleAssignments").Exists()) + Delete.Table("IncidentRoleAssignments"); }🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0080_AddIncidentRoleAssignmentTable.cs` around lines 31 - 34, The Down() method in the M0080_AddIncidentRoleAssignmentTable migration unconditionally deletes the "IncidentRoleAssignments" table, which is asymmetric with the Up() method that likely only creates it conditionally. To fix this, wrap the Delete.Table("IncidentRoleAssignments") call with a conditional check (using IfTableExists or similar existence guard) so that the table is only deleted during rollback if it exists, preventing unintended removal of pre-existing tables that may have been created outside this migration.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0076_AddCommandDefinitionRoleLaneColumnsPg.cs-23-27 (1)
23-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winProtect
Down()column drops with existence checks.
Up()is guarded per column;Down()should mirror that. Current rollback can delete columns that pre-dated this migration.Suggested fix
public override void Down() { - Delete.Column("lanetype").FromTable("commanddefinitionroles"); - Delete.Column("sortorder").FromTable("commanddefinitionroles"); + if (Schema.Table("commanddefinitionroles").Exists() && + Schema.Table("commanddefinitionroles").Column("lanetype").Exists()) + { + Delete.Column("lanetype").FromTable("commanddefinitionroles"); + } + + if (Schema.Table("commanddefinitionroles").Exists() && + Schema.Table("commanddefinitionroles").Column("sortorder").Exists()) + { + Delete.Column("sortorder").FromTable("commanddefinitionroles"); + } }🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0076_AddCommandDefinitionRoleLaneColumnsPg.cs` around lines 23 - 27, The Down() method in the M0076_AddCommandDefinitionRoleLaneColumnsPg migration class is dropping columns without existence checks, which can cause issues during rollback. Add conditional guards around both Delete.Column calls for "lanetype" and "sortorder" from the "commanddefinitionroles" table to match the protection pattern used in the Up() method. This ensures the rollback operation only removes columns that were actually added by this specific migration.Providers/Resgrid.Providers.Migrations/Migrations/M0079_AddIncidentAdHocResourceTables.cs-53-57 (1)
53-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard
Down()table drops to avoid destructive rollback after a no-opUp().
Up()is existence-guarded, butDown()always drops both tables. If these tables existed before this migration, rollback will remove pre-existing data/schema.Suggested fix
public override void Down() { - Delete.Table("IncidentAdHocPersonnel"); - Delete.Table("IncidentAdHocUnits"); + if (Schema.Table("IncidentAdHocPersonnel").Exists()) + Delete.Table("IncidentAdHocPersonnel"); + + if (Schema.Table("IncidentAdHocUnits").Exists()) + Delete.Table("IncidentAdHocUnits"); }🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0079_AddIncidentAdHocResourceTables.cs` around lines 53 - 57, The Down() method in the M0079_AddIncidentAdHocResourceTables migration class unconditionally drops both "IncidentAdHocPersonnel" and "IncidentAdHocUnits" tables without checking if they exist. Since the Up() method is guarded against recreating existing tables, the Down() method should also be guarded to prevent removing pre-existing tables during rollback. Wrap each Delete.Table() call in existence checks using conditional guards (such as IfTableExists) to ensure tables are only dropped if they were created by this migration, not if they existed beforehand.Core/Resgrid.Services/MutualAidService.cs-42-43 (1)
42-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire explicit accepted-state check before exposing linked resources.
The current guard treats
LinkAccepted == falseas eligible, so declined links can still contribute resources. Only explicitly accepted links should pass.Suggested fix
- if (!link.LinkEnabled || link.LinkAccepted == null) + if (!link.LinkEnabled || link.LinkAccepted != true) continue;🤖 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/MutualAidService.cs` around lines 42 - 43, The guard condition in MutualAidService.cs at the link processing logic is insufficiently strict. The current check `if (!link.LinkEnabled || link.LinkAccepted == null)` only filters out null values but allows LinkAccepted when it equals false, which means declined links can still contribute resources. Modify the condition to explicitly require LinkAccepted to equal true, ensuring that both null and false values are caught and declined links are properly excluded before any linked resources are exposed.Core/Resgrid.Services/CheckInTimerService.cs-324-328 (1)
324-328: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t fail persisted check-ins when CQRS enqueue fails.
This path commits the check-in first, then awaits event publication. If enqueue throws, the call fails even though data is already saved, which can trigger duplicate retries and unstable UX.
Suggested fix
record.Timestamp = DateTime.UtcNow; var saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken); // Real-time: a check-in changes the incident accountability/PAR board. - await _coreEventService.IncidentCommandUpdatedAsync(record.DepartmentId, record.CallId); + try + { + await _coreEventService.IncidentCommandUpdatedAsync(saved.DepartmentId, saved.CallId); + } + catch (Exception ex) + { + Framework.Logging.LogException(ex, "Failed to publish IncidentCommandUpdated after check-in save."); + } return saved;As per coding guidelines, use
Resgrid.Framework.Loggingstatic methods (LogException,LogError,LogInfo,LogDebug) for logging throughout the codebase.🤖 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/CheckInTimerService.cs` around lines 324 - 328, The SaveOrUpdateAsync call persists the check-in data to the database, but then awaits the IncidentCommandUpdatedAsync call for CQRS event publication. If this event publishing throws an exception, the entire operation fails despite the data already being saved, causing duplicate retries and unstable behavior. Wrap the _coreEventService.IncidentCommandUpdatedAsync call in a try-catch block that logs any exceptions using Resgrid.Framework.Logging static methods (such as LogException or LogError) without re-throwing the exception, ensuring the method always returns the saved result regardless of whether the event publication succeeds or fails.Source: Coding guidelines
Core/Resgrid.Services/IncidentResourcesService.cs-141-149 (1)
141-149: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce call-scoped validation when assigning personnel to units.
These paths only enforce department ownership, so personnel from another call in the same department can be attached to this incident unit/roster. Validate
CallIdalignment (and active/non-released state) before assigning.Also applies to: 167-173
🤖 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/IncidentResourcesService.cs` around lines 141 - 149, The AssignPersonnelToUnitAsync method only validates department ownership but does not enforce call-scoped validation. After retrieving the personnel via GetByIdAsync and checking that it exists and belongs to the correct department, add additional validation to ensure the personnel's CallId aligns with the incident's CallId and that the personnel and call are in active, non-released states before proceeding with the assignment. Return null if any of these validations fail, similar to the existing department ownership check. Apply the same fix to the second method referenced at lines 167-173.Core/Resgrid.Services/IncidentCommandService.cs-82-100 (1)
82-100: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce atomic establishment for active command creation.
The idempotency check is read-then-write only. Concurrent requests can both pass
GetActiveCommandForCallAsyncand create duplicate active commands for the same call. Add a DB uniqueness guard and handle duplicate-key by reloading the existing command.🤖 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 82 - 100, The race condition exists because concurrent requests can both pass the GetActiveCommandForCallAsync check and create duplicate active commands. Wrap the _incidentCommandRepository.SaveOrUpdateAsync call in a try-catch block to handle database uniqueness constraint violations. When a duplicate key exception is caught (indicating another concurrent request created the command), call GetActiveCommandForCallAsync again to retrieve and return the existing command that was created by the other request. This ensures only one active command is established per call even under concurrent conditions.Core/Resgrid.Services/IncidentCommandService.cs-651-658 (1)
651-658: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate and normalize child
CallIdagainst the parent command.
CommandBelongsToDepartmentAsynconly checks department ownership. Mutation methods then persist caller-suppliedCallId, allowing rows linked to oneIncidentCommandIdbut a differentCallId. That breaks board/report consistency. Load the parent command and enforce/stampDepartmentId+CallIdfrom the parent before save.🤖 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 651 - 658, The CommandBelongsToDepartmentAsync method only validates department ownership but does not validate the CallId against the parent command. This allows mutation methods to persist caller-supplied CallId values that may not match the parent command's CallId, breaking data consistency. Enhance the validation logic by either modifying CommandBelongsToDepartmentAsync to also validate CallId, or creating a new helper method that loads the parent command and returns both the DepartmentId and CallId from it. Then in all mutation methods that persist IncidentCommand data, enforce and stamp the DepartmentId and CallId from the parent command before saving instead of accepting and persisting the caller-supplied CallId values.Core/Resgrid.Services/IncidentCommandService.cs-417-424 (1)
417-424: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate target lane before moving a resource.
MoveResourceAsyncsetsCommandStructureNodeIdwithout verifying the target node exists and belongs to the same incident/call/department. This can corrupt board state by attaching assignments to foreign lanes.Suggested fix
public async Task<ResourceAssignment> MoveResourceAsync(int departmentId, string resourceAssignmentId, string targetNodeId, string userId, CancellationToken cancellationToken = default(CancellationToken)) { var assignment = await _resourceAssignmentRepository.GetByIdAsync(resourceAssignmentId); if (assignment == null || assignment.DepartmentId != departmentId) return null; + + var targetNode = await _commandStructureNodeRepository.GetByIdAsync(targetNodeId); + if (targetNode == null + || targetNode.DepartmentId != assignment.DepartmentId + || targetNode.CallId != assignment.CallId + || !string.Equals(targetNode.IncidentCommandId, assignment.IncidentCommandId, StringComparison.Ordinal)) + return null; assignment.CommandStructureNodeId = targetNodeId; assignment = await _resourceAssignmentRepository.SaveOrUpdateAsync(assignment, cancellationToken);🤖 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 417 - 424, The MoveResourceAsync method assigns a resource to the target node via CommandStructureNodeId without validating that the target node exists or belongs to the same department, which can corrupt the board state. Before setting assignment.CommandStructureNodeId = targetNodeId, retrieve the target node from an appropriate repository and verify it exists and has the matching departmentId. If validation fails, return null to prevent the invalid assignment from being saved.Core/Resgrid.Services/IncidentReportingService.cs-84-98 (1)
84-98: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHarden CSV export against spreadsheet formula injection.
DescriptionandUserIdare exported as cells. Values beginning with=,+,-, or@can execute formulas in spreadsheet clients. Sanitize before writing CSV.Suggested fix
private static string Escape(string value) { if (string.IsNullOrEmpty(value)) return string.Empty; + + // Prevent CSV/spreadsheet formula execution + if ("=+-@".Contains(value[0])) + value = "'" + value; if (value.Contains(",") || value.Contains("\"") || value.Contains("\n")) return "\"" + value.Replace("\"", "\"\"") + "\""; return value; }🤖 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/IncidentReportingService.cs` around lines 84 - 98, The Escape method needs to be hardened against CSV formula injection attacks. Before returning the escaped value, check if the value starts with any of the formula trigger characters (=, +, -, or @), and if so, prepend a single quote character to prevent spreadsheet clients from interpreting it as a formula. This sanitization should occur regardless of whether the value contains special CSV characters that require quoting.
🟡 Minor comments (4)
Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-1070-1070 (1)
1070-1070: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the malformed XML documentation comment in source.
Line 1070 indicates the compiler ignored the type-level XML docs for
IncidentReportingController. This leaves incomplete generated docs and adds warning noise in builds; fix the malformed XML comment in the controller source.🤖 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 `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` at line 1070, Locate the IncidentReportingController class definition in the source code and examine the XML documentation comment (/// lines) directly above it. The XML comment is malformed and causing the compiler to ignore it. Fix any XML syntax errors such as unclosed tags, incorrectly formatted tags, or unescaped special characters in the comment to ensure it is valid XML that the compiler can properly parse and include in the generated documentation.Repositories/Resgrid.Repositories.DataRepository/Resgrid.Repositories.DataRepository.csproj-12-12 (1)
12-12: 🩺 Stability & Availability | 🟡 MinorMicrosoft.Data.SqlClient 6.0.1 is compatible with net9.0, but the version is now out of support.
Both projects target net9.0, which is officially supported by version 6.0.1. However, this version reached end-of-support on February 14, 2026, and is not recommended for new development. Additionally, there are known issues with SNI dependency loading in class libraries when using .NET 9.0 with this version. Consider upgrading to version 6.1.x or 7.0.x, which are currently supported and recommended for .NET 9.0 projects.
🤖 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/Resgrid.Repositories.DataRepository.csproj` at line 12, Update the Microsoft.Data.SqlClient PackageReference in the Resgrid.Repositories.DataRepository.csproj file to a supported version. Change the Version attribute from 6.0.1 to a currently supported version such as 6.1.x or 7.0.x. This will address the end-of-support issue and resolve the known SNI dependency loading problems that can occur with version 6.0.1 when targeting net9.0.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0078_AddIncidentScopeToVoiceChannelsPg.cs-31-33 (1)
31-33: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard column drops in
Down()the same way asUp().
Down()currently assumes all three columns exist; rollback can fail if schema state is drifted/partial. Add existence checks before each delete to keep rollback robust.Suggested fix
public override void Down() { - Delete.Column("callid").FromTable("departmentvoicechannels"); - Delete.Column("isondemand").FromTable("departmentvoicechannels"); - Delete.Column("closedon").FromTable("departmentvoicechannels"); + if (Schema.Table("departmentvoicechannels").Column("callid").Exists()) + Delete.Column("callid").FromTable("departmentvoicechannels"); + + if (Schema.Table("departmentvoicechannels").Column("isondemand").Exists()) + Delete.Column("isondemand").FromTable("departmentvoicechannels"); + + if (Schema.Table("departmentvoicechannels").Column("closedon").Exists()) + Delete.Column("closedon").FromTable("departmentvoicechannels"); }🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0078_AddIncidentScopeToVoiceChannelsPg.cs` around lines 31 - 33, In the Down() method of M0078_AddIncidentScopeToVoiceChannelsPg, the Delete.Column operations for callid, isondemand, and closedon from the departmentvoicechannels table lack existence checks, which can cause rollback failures if the schema state is drifted or partial. Add existence checks (IfExists guard) before each Delete.Column operation to match the defensive approach used in the Up() method, ensuring each column deletion only attempts to drop if the column exists.Core/Resgrid.Services/IncidentVoiceService.cs-109-112 (1)
109-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not require active status when logging channel closure.
WriteLogAsynconly resolves an active command. In the close-command flow, channels are closed after command status is set to closed, so channel-close timeline entries (and board update signal) are skipped on that path.🤖 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/IncidentVoiceService.cs` around lines 109 - 112, The current filter in the GetAllByDepartmentIdAsync query requires the command status to be Active when searching for a command by CallId, but in the close-command flow the status is already set to closed before WriteLogAsync is called, causing channel closure logging to be skipped. Remove the status equality check (x.Status == (int)IncidentCommandStatus.Active) from the FirstOrDefault filter in the command lookup so that commands in any status can be found and allow proper logging of channel closure and board updates.
🧹 Nitpick comments (6)
Web/Resgrid.Web.Services/Controllers/v4/CommandsController.cs (1)
126-177: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate lane/timer invariants before mapping to
CommandDefinitionRole.
SaveCommandcurrently accepts negative values and inverted min/max constraints (for personnel/time), which can persist invalid command-template rules.Suggested validation guard
if (input == null || string.IsNullOrWhiteSpace(input.Name)) return BadRequest(); + + if (input.TimerMinutes < 0) + return BadRequest(); + + if (input.Lanes != null && input.Lanes.Any(l => + l == null || + string.IsNullOrWhiteSpace(l.Name) || + l.MinUnitPersonnel < 0 || l.MaxUnitPersonnel < 0 || + l.MinTimeInRole < 0 || l.MaxTimeInRole < 0 || + l.MaxUnits < 0 || + l.MinUnitPersonnel > l.MaxUnitPersonnel || + l.MinTimeInRole > l.MaxTimeInRole)) + { + return BadRequest(); + }🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/CommandsController.cs` around lines 126 - 177, The SaveCommand method lacks validation for timer values and lane constraints before mapping input data to CommandDefinitionRole objects, allowing invalid rules to persist. Add validation checks after the command object is initialized to ensure Timer and TimerMinutes are non-negative values, and before the foreach loop that processes input.Lanes, validate that each lane's minimum/maximum constraints are valid (such as MinUnitPersonnel not exceeding MaxUnitPersonnel, MinTimeInRole not exceeding MaxTimeInRole, and all numeric values being non-negative). Return a BadRequest response if any invariants are violated.Web/Resgrid.Web.Services/Controllers/v4/IncidentVoiceController.cs (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFollow the required constructor resolution pattern for services.
This controller uses constructor injection, but project guidance requires resolving dependencies via
Bootstrapper.GetKernel().Resolve<T>().
As per coding guidelines, "**/*.cs: UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection."🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentVoiceController.cs` around lines 24 - 27, The IncidentVoiceController constructor uses constructor injection, but the project requires using the Service Locator pattern instead. Remove the IIncidentVoiceService parameter from the IncidentVoiceController constructor and instead resolve the dependency inside the constructor body using Bootstrapper.GetKernel().Resolve<IIncidentVoiceService>(), then assign the resolved instance to the _incidentVoiceService field. This change aligns the code with the project's required dependency resolution guidelines.Source: Coding guidelines
Web/Resgrid.Web.Services/Controllers/v4/IncidentResourcesController.cs (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required Service Locator pattern for controller dependencies.
This constructor uses direct DI parameters instead of resolving through
Bootstrapper.GetKernel().Resolve<T>().
As per coding guidelines, "**/*.cs: UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection."🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentResourcesController.cs` around lines 26 - 29, The IncidentResourcesController constructor is using direct constructor injection with an IIncidentResourcesService parameter instead of following the Service Locator pattern. Refactor the constructor to remove the IIncidentResourcesService parameter and instead assign _incidentResourcesService by calling Bootstrapper.GetKernel().Resolve<IIncidentResourcesService>() within the constructor body, following the required Service Locator pattern guideline for dependency resolution.Source: Coding guidelines
Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repo’s required dependency resolution pattern in controllers.
This controller constructor uses direct constructor injection instead of the required Service Locator pattern for
*.csfiles in this repo.
As per coding guidelines, "**/*.cs: UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection."🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs` around lines 26 - 29, The IncidentCommandController constructor is using direct constructor injection for the IIncidentCommandService parameter, but the repository requires using the Service Locator pattern instead. Remove the constructor parameter for incidentCommandService from the IncidentCommandController constructor and instead initialize the _incidentCommandService field using Bootstrapper.GetKernel().Resolve<IIncidentCommandService>() inside the constructor body, following the established pattern used elsewhere in the codebase.Source: Coding guidelines
Web/Resgrid.Web.Services/Controllers/v4/IncidentReportingController.cs (1)
25-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign constructor dependency resolution with project convention.
This constructor currently uses parameter injection instead of the required Service Locator approach.
As per coding guidelines, "**/*.cs: UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection."🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentReportingController.cs` around lines 25 - 28, The IncidentReportingController constructor uses constructor parameter injection for IIncidentReportingService, but the project convention requires using the Service Locator pattern instead. Remove the incidentReportingService parameter from the constructor signature and inside the constructor body, resolve the dependency using Bootstrapper.GetKernel().Resolve<IIncidentReportingService>() and assign the result to the _incidentReportingService field.Source: Coding guidelines
Web/Resgrid.Web.Services/Controllers/v4/IncidentRolesController.cs (1)
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstructor dependency resolution should follow repo standard.
This uses constructor injection, but this codebase’s
*.csguideline requires explicit Service Locator resolution in constructors.
As per coding guidelines, "**/*.cs: UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection."🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentRolesController.cs` around lines 27 - 30, The IncidentRolesController constructor is using constructor injection, but the codebase guideline requires the Service Locator pattern for dependency resolution. Remove the IIncidentCommandService parameter from the IncidentRolesController constructor and instead use Bootstrapper.GetKernel().Resolve<IIncidentCommandService>() inside the constructor body to explicitly resolve the dependency and assign it to the _incidentCommandService field, following the codebase's standard pattern for all *.cs files.Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Workers/Resgrid.Workers.Console/Tasks/ParEvaluationTask.cs (1)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the failure branch at a higher level.
A failed accountability sweep is logged at
Information(Line 35), so a recurring PAR-evaluation failure on this safety-critical job would be indistinguishable from normal output. UseLogWarning/LogErrorfor the failure branch so it surfaces in alerting.♻️ Suggested change
if (result.Item1) _logger.LogInformation($"ParEvaluation::{result.Item2}"); else - _logger.LogInformation($"ParEvaluation::Failed to sweep accountability. {result.Item2}"); + _logger.LogWarning($"ParEvaluation::Failed to sweep accountability. {result.Item2}");🤖 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 `@Workers/Resgrid.Workers.Console/Tasks/ParEvaluationTask.cs` around lines 32 - 35, The failure path in ParEvaluationTask’s result handling is logged at Information, which hides recurring sweep failures from alerting. Update the else branch in the result.Item1 check to use a higher-severity logger call such as LogWarning or LogError, keeping the existing success path in LogInformation and preserving the same ParEvaluation message context.Workers/Resgrid.Workers.Framework/Logic/ParEvaluationLogic.cs (1)
50-59: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueInner loop ignores cancellation.
The outer department loop honors
cancellationToken(Lines 43-44), but the inner per-call loop does not, so a long sweep of a single high-call-volume department won't abort promptly on shutdown.EvaluateCriticalParAsyncalready receives the token, but the loop itself keeps iterating. Consider re-checkingcancellationToken.IsCancellationRequestedinside the inner loop.♻️ Suggested change
foreach (var call in activeCalls.Where(c => c.CheckInTimersEnabled)) { + if (cancellationToken.IsCancellationRequested) + break; + // EvaluateCriticalParAsync no-ops cheaply when the call has no active incident command, // so we can sweep every check-in-enabled active call without pre-filtering by command. var flagged = await _incidentCommandService.EvaluateCriticalParAsync( department.DepartmentId, call.CallId, cancellationToken);🤖 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 `@Workers/Resgrid.Workers.Framework/Logic/ParEvaluationLogic.cs` around lines 50 - 59, The inner per-call sweep in ParEvaluationLogic’s department loop does not stop promptly when cancellation is requested. Update the foreach over activeCalls in ParEvaluationLogic so it re-checks cancellationToken inside the loop and exits/breaks early when cancellation is signaled, while continuing to pass the token into IncidentCommandService.EvaluateCriticalParAsync.Workers/Resgrid.Workers.Console/Program.cs (1)
424-431: 🚀 Performance & Scalability | 🔵 TrivialEvery-minute, all-department sweep — watch scaling cost.
ParEvaluationLogic.Processenumerates every department viaGetAllAsync()and issues aGetActiveCallsByDepartmentAsyncquery per department on each 1-minute tick (ParEvaluationLogic.cs Lines 34-46). The per-call evaluation short-circuits cheaply, but the per-department DB round-trips grow linearly with tenant count and run unconditionally every minute. Consider scoping the sweep to departments that actually have active, check-in-enabled calls (e.g. a single query for active calls across departments), or widening the interval, to bound load as the install grows.🤖 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 `@Workers/Resgrid.Workers.Console/Program.cs` around lines 424 - 431, Adjust the PAR scheduling in Program so the 1-minute “PAR Evaluation” job doesn’t unconditionally sweep every department on each tick; the issue is the linear per-department load caused by ParEvaluationLogic.Process/GetAllAsync and repeated GetActiveCallsByDepartmentAsync calls. Update the scheduling/selection logic to scope evaluation to departments with active, check-in-enabled calls (for example by driving it from a single active-calls query or another narrower selector), or increase the interval if needed, while keeping the existing PAR Evaluation command flow intact.
🤖 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.
Nitpick comments:
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Around line 424-431: Adjust the PAR scheduling in Program so the 1-minute “PAR
Evaluation” job doesn’t unconditionally sweep every department on each tick; the
issue is the linear per-department load caused by
ParEvaluationLogic.Process/GetAllAsync and repeated
GetActiveCallsByDepartmentAsync calls. Update the scheduling/selection logic to
scope evaluation to departments with active, check-in-enabled calls (for example
by driving it from a single active-calls query or another narrower selector), or
increase the interval if needed, while keeping the existing PAR Evaluation
command flow intact.
In `@Workers/Resgrid.Workers.Console/Tasks/ParEvaluationTask.cs`:
- Around line 32-35: The failure path in ParEvaluationTask’s result handling is
logged at Information, which hides recurring sweep failures from alerting.
Update the else branch in the result.Item1 check to use a higher-severity logger
call such as LogWarning or LogError, keeping the existing success path in
LogInformation and preserving the same ParEvaluation message context.
In `@Workers/Resgrid.Workers.Framework/Logic/ParEvaluationLogic.cs`:
- Around line 50-59: The inner per-call sweep in ParEvaluationLogic’s department
loop does not stop promptly when cancellation is requested. Update the foreach
over activeCalls in ParEvaluationLogic so it re-checks cancellationToken inside
the loop and exits/breaks early when cancellation is signaled, while continuing
to pass the token into IncidentCommandService.EvaluateCriticalParAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79129747-22ec-4f28-93ec-136b5f3f304c
⛔ Files ignored due to path filters (3)
Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentExportTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentVoiceServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (15)
Core/Resgrid.Model/IncidentCommand/IncidentCommandEnums.csCore/Resgrid.Model/Services/IIncidentCommandService.csCore/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/IncidentReportingService.csCore/Resgrid.Services/IncidentVoiceService.csProviders/Resgrid.Providers.Migrations/Migrations/M0077_AddIncidentCommandTables.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0077_AddIncidentCommandTablesPg.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWorkers/Resgrid.Workers.Console/Commands/ParEvaluationCommand.csWorkers/Resgrid.Workers.Console/Program.csWorkers/Resgrid.Workers.Console/Tasks/ParEvaluationTask.csWorkers/Resgrid.Workers.Framework/Logic/ParEvaluationLogic.cs
🚧 Files skipped from review as they are similar to previous changes (10)
- Core/Resgrid.Model/IncidentCommand/IncidentCommandEnums.cs
- Providers/Resgrid.Providers.Migrations/Migrations/M0077_AddIncidentCommandTables.cs
- Core/Resgrid.Model/Services/IIncidentCommandService.cs
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0077_AddIncidentCommandTablesPg.cs
- Core/Resgrid.Services/CheckInTimerService.cs
- Core/Resgrid.Services/IncidentVoiceService.cs
- Web/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.cs
- Core/Resgrid.Services/IncidentReportingService.cs
- Core/Resgrid.Services/IncidentCommandService.cs
- Web/Resgrid.Web.Services/Resgrid.Web.Services.xml
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Summary by CodeRabbit