Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughIncident command support expands across reopening, metadata and location updates, objective close-out outcomes, entity-based needs, tactical maps, audit storage, synchronization, and v4 API endpoints. ChangesIncident command expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| public async Task<IncidentMap> SaveIncidentMapAsync(IncidentMap map, string userId, CancellationToken cancellationToken = default(CancellationToken)) | ||
| { | ||
| if (map == null || string.IsNullOrWhiteSpace(map.IncidentCommandId) || string.IsNullOrWhiteSpace(map.Name)) |
| try | ||
| { | ||
| var entities = input.Entities.Select(e => new IncidentNeedEntity { EntityKind = e.EntityKind, EntityId = e.EntityId }).ToList(); | ||
| var need = await _incidentCommandService.RequestNeedEntitiesAsync(DepartmentId, input.IncidentCommandId, input.Name, input.Description, entities, UserId, CancellationToken.None); |
| map.DepartmentId = DepartmentId; | ||
|
|
||
| try | ||
| { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs (1)
850-858: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
CompleteObjectiveaccepting no body as intended.The endpoint defaults
inputtonull, but the current ASP.NET Core configuration still requires a non-empty body for[FromBody]parameters, so clients sending no body can now get400 "A non-empty request body is required."Use an optional binding strategy or the configured empty-body opt-in so this backward-compatible call path still works.🤖 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 850 - 858, Update CompleteObjective’s [FromBody] binding so an omitted request body is accepted, using the project’s configured empty-body opt-in or established optional binding approach. Preserve the existing null handling and TacticalObjectiveOutcome.NotSet behavior when input is absent.
🧹 Nitpick comments (1)
Core/Resgrid.Services/IncidentCommandService.cs (1)
2446-2489: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid the redundant second write on map create. On the create path
UpsertOwnedAsyncinserts the row withCreatedByUserId = nullandCreatedOn = default(DateTime), then this method backfills those fields and issues a secondSaveOrUpdateAsync. Stamping the audit fields onmapbefore the upsert (letting thepreservecallback keep the stored values on the update path) removes the extra round-trip and the transient min-valueCreatedOn.♻️ Proposed refactor
map.CallId = command.CallId; map.Name = TrimToLength(map.Name, 500); map.Description = TrimToLength(map.Description, 2000); + map.CreatedByUserId = userId; + map.CreatedOn = DateTime.UtcNow; var (saved, isNew, rejected) = await UpsertOwnedAsync(_incidentMapRepository, map, map.DepartmentId, e => e.DepartmentId, (stored, incoming) => { incoming.CreatedByUserId = stored.CreatedByUserId; incoming.CreatedOn = stored.CreatedOn; incoming.DeletedOn = stored.DeletedOn; incoming.UpdatedByUserId = userId; incoming.UpdatedOn = DateTime.UtcNow; }, cancellationToken); if (rejected) return null; map = saved; - - if (isNew) - { - map.CreatedByUserId = userId; - map.CreatedOn = DateTime.UtcNow; - map = await _incidentMapRepository.SaveOrUpdateAsync(Touch(map), 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 2446 - 2489, Update SaveIncidentMapAsync to assign CreatedByUserId, CreatedOn, UpdatedByUserId, and UpdatedOn on map before calling UpsertOwnedAsync, using userId and DateTime.UtcNow. Remove the isNew-only SaveOrUpdateAsync block, while preserving the existing preserve callback so updates retain stored creation fields.
🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs`:
- Around line 946-976: In RequestNeedEntities, validate every input EntityKind
with Enum.IsDefined(typeof(NeedEntityKind), e.EntityKind) before constructing
IncidentNeedEntity objects. Reject the request with BadRequest when any kind is
undefined, while preserving the existing entity mapping and service call for
valid values.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs`:
- Around line 285-299: Update the RecordNeedEntityStatusAsync call in
ProcessSetUnitState to pass the existing cancellationToken argument, matching
the equivalent SavePersonStatus call while preserving the current best-effort
exception handling.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs`:
- Around line 850-858: Update CompleteObjective’s [FromBody] binding so an
omitted request body is accepted, using the project’s configured empty-body
opt-in or established optional binding approach. Preserve the existing null
handling and TacticalObjectiveOutcome.NotSet behavior when input is absent.
---
Nitpick comments:
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 2446-2489: Update SaveIncidentMapAsync to assign CreatedByUserId,
CreatedOn, UpdatedByUserId, and UpdatedOn on map before calling
UpsertOwnedAsync, using userId and DateTime.UtcNow. Remove the isNew-only
SaveOrUpdateAsync block, while preserving the existing preserve callback so
updates retain stored creation fields.
🪄 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: 4c290382-decc-405b-9891-12740e8fda5b
⛔ Files ignored due to path filters (4)
Tests/Resgrid.Tests/Services/IncidentCommandContentServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandInfoAndReopenTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandNeedsAndLeadsTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**
📒 Files selected for processing (33)
Core/Resgrid.Model/Events/IncidentCommandEvents.csCore/Resgrid.Model/IncidentCommand/CommandStructureNode.csCore/Resgrid.Model/IncidentCommand/IncidentCommand.csCore/Resgrid.Model/IncidentCommand/IncidentCommandBoard.csCore/Resgrid.Model/IncidentCommand/IncidentCommandChanges.csCore/Resgrid.Model/IncidentCommand/IncidentCommandEnums.csCore/Resgrid.Model/IncidentCommand/IncidentCommandSummary.csCore/Resgrid.Model/IncidentCommand/IncidentMap.csCore/Resgrid.Model/IncidentCommand/IncidentNeed.csCore/Resgrid.Model/IncidentCommand/IncidentNeedEntity.csCore/Resgrid.Model/IncidentCommand/IncidentTacticals.csCore/Resgrid.Model/Repositories/IIncidentCommandRepositories.csCore/Resgrid.Model/Services/IIncidentCommandService.csCore/Resgrid.Services/IncidentCommandService.csProviders/Resgrid.Providers.Migrations/Migrations/M0094_AddIncidentCommandNameAndLocations.csProviders/Resgrid.Providers.Migrations/Migrations/M0095_AddIncidentNeedUpdates.csProviders/Resgrid.Providers.Migrations/Migrations/M0096_AddObjectiveOutcome.csProviders/Resgrid.Providers.Migrations/Migrations/M0097_AddIncidentMapView.csProviders/Resgrid.Providers.Migrations/Migrations/M0098_AddIncidentMaps.csProviders/Resgrid.Providers.Migrations/Migrations/M0099_AddIncidentNeedEntities.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0094_AddIncidentCommandNameAndLocationsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0095_AddIncidentNeedUpdatesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0096_AddObjectiveOutcomePg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0097_AddIncidentMapViewPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0098_AddIncidentMapsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0099_AddIncidentNeedEntitiesPg.csRepositories/Resgrid.Repositories.DataRepository/IncidentCommandRepositories.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitStatusController.csWeb/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
| public List<IncidentMapAnnotation> Annotations { get; set; } = new List<IncidentMapAnnotation>(); | ||
|
|
||
| /// <summary>Named tactical maps for the incident (the main map lives on the Command itself).</summary> | ||
| public List<IncidentMap> Maps { get; set; } = new List<IncidentMap>(); |
There was a problem hiding this comment.
Mutable collection exposure: the Maps property publicly exposes a mutable List<IncidentMap>, allowing callers to alter the underlying collection unexpectedly. Expose the collection as IReadOnlyList<IncidentMap> while keeping the internal mutable list for initialization.
Kody rule violation: Use IReadOnlyList for immutable collections
Prompt for LLM
File Core/Resgrid.Model/IncidentCommand/IncidentCommandBoard.cs:
Line 26:
Mutable collection exposure: the `Maps` property publicly exposes a mutable `List<IncidentMap>`, allowing callers to alter the underlying collection unexpectedly. Expose the collection as `IReadOnlyList<IncidentMap>` while keeping the internal mutable list for initialization.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public int AnnotationType { get; set; } | ||
|
|
||
| /// <summary>The named <see cref="IncidentMap"/> this markup belongs to; null = the incident's main map.</summary> | ||
| public string IncidentMapId { get; set; } |
There was a problem hiding this comment.
Null-initialized string property: IncidentMapId is declared without an initializer, defaulting to null. Null strings invite null-reference exceptions and ambiguous state. Initialize the property with string.Empty or make it explicitly nullable (string?) with documented sentinel semantics.
Kody rule violation: Initialize properties with default values
Prompt for LLM
File Core/Resgrid.Model/IncidentCommand/IncidentTacticals.cs:
Line 152:
Null-initialized string property: `IncidentMapId` is declared without an initializer, defaulting to null. Null strings invite null-reference exceptions and ambiguous state. Initialize the property with `string.Empty` or make it explicitly nullable (`string?`) with documented sentinel semantics.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var command in selected) | ||
| { | ||
| Call call = null; | ||
| try | ||
| { | ||
| call = await _callsService.GetCallByIdAsync(command.CallId); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Resgrid.Framework.Logging.LogException(ex); | ||
| } |
There was a problem hiding this comment.
N+1 query: GetCallByIdAsync is called individually inside a foreach loop over every selected command, triggering one database round-trip per incident. Batch-load the calls once before the loop via GetCallsByIdsAsync or a preloaded dictionary to eliminate the N+1 pattern on every GetCommandList request.
var callIds = selected.Select(c => c.CallId).Distinct().ToList();
var callsById = new Dictionary<int, Call>();
foreach (var callId in callIds)
{
try { var c = await _callsService.GetCallByIdAsync(callId); if (c != null) callsById[callId] = c; }
catch (Exception ex) { Resgrid.Framework.Logging.LogException(ex); }
}
// then inside the loop: callsById.TryGetValue(command.CallId, out var call);Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 802 to 812:
N+1 query: `GetCallByIdAsync` is called individually inside a `foreach` loop over every selected command, triggering one database round-trip per incident. Batch-load the calls once before the loop via `GetCallsByIdsAsync` or a preloaded dictionary to eliminate the N+1 pattern on every `GetCommandList` request.
Suggested Code:
var callIds = selected.Select(c => c.CallId).Distinct().ToList();
var callsById = new Dictionary<int, Call>();
foreach (var callId in callIds)
{
try { var c = await _callsService.GetCallByIdAsync(callId); if (c != null) callsById[callId] = c; }
catch (Exception ex) { Resgrid.Framework.Logging.LogException(ex); }
}
// then inside the loop: callsById.TryGetValue(command.CallId, out var call);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var command = await GetActiveCommandForCallAsync(departmentId, callId); | ||
| if (command == null) | ||
| return; | ||
|
|
||
| var all = await _incidentNeedEntityRepository.GetAllByDepartmentIdAsync(departmentId); | ||
| var callEntities = (all ?? Enumerable.Empty<IncidentNeedEntity>()).Where(x => x.CallId == callId).ToList(); | ||
| if (callEntities.Count == 0) | ||
| return; |
There was a problem hiding this comment.
Hot-path overhead: RecordNeedEntityStatusAsync unconditionally issues two database round-trips (GetActiveCommandForCallAsync + GetAllByDepartmentIdAsync) on every personnel and unit status save before the early return when no entity needs exist. Add a fast-exit guard that checks whether the department has any IncidentNeedEntity rows (or cache a per-call flag) before the full entity-load and match logic.
Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 2106 to 2113:
Hot-path overhead: `RecordNeedEntityStatusAsync` unconditionally issues two database round-trips (`GetActiveCommandForCallAsync` + `GetAllByDepartmentIdAsync`) on every personnel and unit status save before the early return when no entity needs exist. Add a fast-exit guard that checks whether the department has any `IncidentNeedEntity` rows (or cache a per-call flag) before the full entity-load and match logic.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var command in selected) | ||
| { | ||
| Call call = null; | ||
| try | ||
| { | ||
| call = await _callsService.GetCallByIdAsync(command.CallId); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Resgrid.Framework.Logging.LogException(ex); | ||
| } |
There was a problem hiding this comment.
N+1 query: GetCallByIdAsync is called individually inside a foreach loop over every selected command, causing one database round-trip per command. With includeClosed=true, a department with many historical commands triggers O(N) sequential network round-trips. Batch-fetch all needed calls via GetCallsByIdsAsync before the loop and look them up from a dictionary.
var callIds = selected.Select(c => c.CallId).Distinct().ToList();
var callsById = new Dictionary<int, Call>();
foreach (var callId in callIds)
{
try { var c = await _callsService.GetCallByIdAsync(callId); if (c != null) callsById[callId] = c; }
catch (Exception ex) { Resgrid.Framework.Logging.LogException(ex); }
}
foreach (var command in selected)
{
callsById.TryGetValue(command.CallId, out var call);Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 802 to 812:
N+1 query: `GetCallByIdAsync` is called individually inside a `foreach` loop over every selected command, causing one database round-trip per command. With `includeClosed=true`, a department with many historical commands triggers O(N) sequential network round-trips. Batch-fetch all needed calls via `GetCallsByIdsAsync` before the loop and look them up from a dictionary.
Suggested Code:
var callIds = selected.Select(c => c.CallId).Distinct().ToList();
var callsById = new Dictionary<int, Call>();
foreach (var callId in callIds)
{
try { var c = await _callsService.GetCallByIdAsync(callId); if (c != null) callsById[callId] = c; }
catch (Exception ex) { Resgrid.Framework.Logging.LogException(ex); }
}
foreach (var command in selected)
{
callsById.TryGetValue(command.CallId, out var call);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var all = await _incidentNeedEntityRepository.GetAllByDepartmentIdAsync(departmentId); | ||
| var callEntities = (all ?? Enumerable.Empty<IncidentNeedEntity>()).Where(x => x.CallId == callId).ToList(); | ||
| if (callEntities.Count == 0) | ||
| return; |
There was a problem hiding this comment.
Unbounded hot-path overhead: RecordNeedEntityStatusAsync loads ALL department need-entity rows via GetAllByDepartmentIdAsync before filtering to one call in memory on every personnel and unit status save (PersonnelStatusesController line 174, UnitStatusController line 292). Loading an entire department table on each status save adds overhead proportional to the department's total historical entity-need count. Filter at the data layer by callId before materializing results.
Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 2110 to 2113:
Unbounded hot-path overhead: `RecordNeedEntityStatusAsync` loads ALL department need-entity rows via `GetAllByDepartmentIdAsync` before filtering to one call in memory on every personnel and unit status save (`PersonnelStatusesController` line 174, `UnitStatusController` line 292). Loading an entire department table on each status save adds overhead proportional to the department's total historical entity-need count. Filter at the data layer by `callId` before materializing results.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Call call = null; | ||
| try | ||
| { | ||
| call = await _callsService.GetCallByIdAsync(command.CallId); |
There was a problem hiding this comment.
N+1 query: GetCallByIdAsync is called per command inside a foreach loop, producing N database round-trips for N commands. Batch-load all call IDs before the loop via GetCallsByIdsAsync(selected.Select(c => c.CallId).Distinct()) and build a dictionary for lookup inside the loop.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 807:
N+1 query: `GetCallByIdAsync` is called per command inside a `foreach` loop, producing N database round-trips for N commands. Batch-load all call IDs before the loop via `GetCallsByIdsAsync(selected.Select(c => c.CallId).Distinct())` and build a dictionary for lookup inside the loop.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Call call = null; | ||
| try | ||
| { | ||
| call = await _callsService.GetCallByIdAsync(command.CallId); |
There was a problem hiding this comment.
N+1 query: each command's call is fetched individually inside the summary loop instead of via a single JOIN or batch fetch. Eager-load calls via a batch query or a join between commands and calls before iterating.
Kody rule violation: Optimize database queries with JOINs
Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 807:
N+1 query: each command's call is fetched individually inside the summary loop instead of via a single JOIN or batch fetch. Eager-load calls via a batch query or a join between commands and calls before iterating.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| map.Name = TrimToLength(map.Name, 500); | ||
| map.Description = TrimToLength(map.Description, 2000); | ||
|
|
||
| var (saved, isNew, rejected) = await UpsertOwnedAsync(_incidentMapRepository, map, map.DepartmentId, |
There was a problem hiding this comment.
Audit corruption: SaveIncidentMapAsync inserts the map via UpsertOwnedAsync before setting CreatedByUserId and CreatedOn, persisting a row with untrusted/default audit values that a second SaveOrUpdateAsync immediately overwrites. Set CreatedByUserId and CreatedOn on the map object before calling UpsertOwnedAsync, eliminating the redundant double-write.
if (string.IsNullOrWhiteSpace(map.IncidentMapId))
map.IncidentMapId = Guid.NewGuid().ToString();
map.CreatedByUserId = userId;
map.CreatedOn = DateTime.UtcNow;
var (saved, isNew, rejected) = await UpsertOwnedAsync(_incidentMapRepository, map, map.DepartmentId, ...Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 2463:
Audit corruption: `SaveIncidentMapAsync` inserts the map via `UpsertOwnedAsync` before setting `CreatedByUserId` and `CreatedOn`, persisting a row with untrusted/default audit values that a second `SaveOrUpdateAsync` immediately overwrites. Set `CreatedByUserId` and `CreatedOn` on the map object before calling `UpsertOwnedAsync`, eliminating the redundant double-write.
Suggested Code:
if (string.IsNullOrWhiteSpace(map.IncidentMapId))
map.IncidentMapId = Guid.NewGuid().ToString();
map.CreatedByUserId = userId;
map.CreatedOn = DateTime.UtcNow;
var (saved, isNew, rejected) = await UpsertOwnedAsync(_incidentMapRepository, map, map.DepartmentId, ...
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| map.Name = TrimToLength(map.Name, 500); | ||
| map.Description = TrimToLength(map.Description, 2000); | ||
|
|
||
| var (saved, isNew, rejected) = await UpsertOwnedAsync(_incidentMapRepository, map, map.DepartmentId, |
There was a problem hiding this comment.
Audit corruption: SaveIncidentMapAsync calls UpsertOwnedAsync (which inserts via InsertAsync) before setting CreatedByUserId and CreatedOn, then immediately calls SaveOrUpdateAsync a second time to stamp them. If the second save fails due to a network error or timeout, the audit trail is permanently corrupted with null CreatedByUserId and default(DateTime) CreatedOn. Set map.CreatedByUserId and map.CreatedOn before calling UpsertOwnedAsync, matching how annotation and node saves set audit fields before upsert.
if (string.IsNullOrWhiteSpace(map.IncidentMapId))
{
map.CreatedByUserId = userId;
map.CreatedOn = DateTime.UtcNow;
}
var (saved, isNew, rejected) = await UpsertOwnedAsync(_incidentMapRepository, map, map.DepartmentId,
e => e.DepartmentId, (stored, incoming) =>
{
incoming.CreatedByUserId = stored.CreatedByUserId;
incoming.CreatedOn = stored.CreatedOn;
incoming.DeletedOn = stored.DeletedOn;
incoming.UpdatedByUserId = userId;
incoming.UpdatedOn = DateTime.UtcNow;
}, cancellationToken);
if (rejected)
return null;
map = saved;Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 2463:
Audit corruption: `SaveIncidentMapAsync` calls `UpsertOwnedAsync` (which inserts via `InsertAsync`) before setting `CreatedByUserId` and `CreatedOn`, then immediately calls `SaveOrUpdateAsync` a second time to stamp them. If the second save fails due to a network error or timeout, the audit trail is permanently corrupted with null `CreatedByUserId` and `default(DateTime)` `CreatedOn`. Set `map.CreatedByUserId` and `map.CreatedOn` before calling `UpsertOwnedAsync`, matching how annotation and node saves set audit fields before upsert.
Suggested Code:
if (string.IsNullOrWhiteSpace(map.IncidentMapId))
{
map.CreatedByUserId = userId;
map.CreatedOn = DateTime.UtcNow;
}
var (saved, isNew, rejected) = await UpsertOwnedAsync(_incidentMapRepository, map, map.DepartmentId,
e => e.DepartmentId, (stored, incoming) =>
{
incoming.CreatedByUserId = stored.CreatedByUserId;
incoming.CreatedOn = stored.CreatedOn;
incoming.DeletedOn = stored.DeletedOn;
incoming.UpdatedByUserId = userId;
incoming.UpdatedOn = DateTime.UtcNow;
}, cancellationToken);
if (rejected)
return null;
map = saved;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var column in new[] { "StagingLatitude", "StagingLongitude", "RehabLatitude", "RehabLongitude" }) | ||
| { | ||
| if (!Schema.Table("IncidentCommands").Column(column).Exists()) | ||
| Alter.Table("IncidentCommands").AddColumn(column).AsString(int.MaxValue).Nullable(); |
There was a problem hiding this comment.
Oversized column type: four coordinate columns (StagingLatitude, StagingLongitude, RehabLatitude, RehabLongitude) are created with AsString(int.MaxValue) (NVARCHAR(MAX)), but latitude and longitude values are short bounded strings at most ~20 characters. NVARCHAR(MAX) prevents efficient page-row storage, harms index/seek performance, and bloats storage. Use a bounded length such as AsString(50) or store coordinates in a numeric type like AsDouble with appropriate precision.
Kody rule violation: Optimize string column types
Prompt for LLM
File Providers/Resgrid.Providers.Migrations/Migrations/M0094_AddIncidentCommandNameAndLocations.cs:
Line 33:
Oversized column type: four coordinate columns (`StagingLatitude`, `StagingLongitude`, `RehabLatitude`, `RehabLongitude`) are created with `AsString(int.MaxValue)` (NVARCHAR(MAX)), but latitude and longitude values are short bounded strings at most ~20 characters. NVARCHAR(MAX) prevents efficient page-row storage, harms index/seek performance, and bloats storage. Use a bounded length such as `AsString(50)` or store coordinates in a numeric type like `AsDouble` with appropriate precision.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var column in new[] | ||
| { | ||
| "name", "commandpostlocationtext", | ||
| "staginglocationtext", "staginglatitude", "staginglongitude", | ||
| "rehablocationtext", "rehablatitude", "rehablongitude" | ||
| }) |
There was a problem hiding this comment.
Duplicated array: the column-names array is defined identically in both Up() (lines 18–23) and Down() (lines 35–40). Extract the array to a private static readonly field such as private static readonly string[] IncidentCommandColumns = { … }; and reference it in both methods.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0094_AddIncidentCommandNameAndLocationsPg.cs:
Line 35 to 40:
Duplicated array: the column-names array is defined identically in both `Up()` (lines 18–23) and `Down()` (lines 35–40). Extract the array to a `private static readonly` field such as `private static readonly string[] IncidentCommandColumns = { … };` and reference it in both methods.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| public override void Up() | ||
| { | ||
| if (!Schema.Table("tacticalobjectives").Exists()) |
There was a problem hiding this comment.
Duplicated string literal: the table name "tacticalobjectives" is repeated 8 times as an inline string across the migration. A typo in any instance would cause the existence check to silently evaluate false and skip a migration step. Define a class-level constant such as private const string TableName = "tacticalobjectives"; and reference it everywhere.
Kody rule violation: Centralize string constants
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0096_AddObjectiveOutcomePg.cs:
Line 14:
Duplicated string literal: the table name `"tacticalobjectives"` is repeated 8 times as an inline string across the migration. A typo in any instance would cause the existence check to silently evaluate false and skip a migration step. Define a class-level constant such as `private const string TableName = "tacticalobjectives";` and reference it everywhere.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .WithColumn("incidentcommandid").AsCustom("citext").NotNullable() | ||
| .WithColumn("departmentid").AsInt32().NotNullable() | ||
| .WithColumn("callid").AsInt32().NotNullable() | ||
| .WithColumn("entitykind").AsInt32().NotNullable().WithDefaultValue(0) |
There was a problem hiding this comment.
Unexplained magic number: the default value 0 for the entitykind column does not document which EntityKind it represents. Define a named constant or cast the domain enum, e.g. const int DefaultEntityKind = (int)EntityKind.Unknown;, so the migration's intent is self-documenting and stays in sync with the domain enum.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0099_AddIncidentNeedEntitiesPg.cs:
Line 23:
Unexplained magic number: the default value `0` for the `entitykind` column does not document which `EntityKind` it represents. Define a named constant or cast the domain enum, e.g. `const int DefaultEntityKind = (int)EntityKind.Unknown;`, so the migration's intent is self-documenting and stays in sync with the domain enum.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [HttpDelete("DeleteIncidentMap/{incidentMapId}")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Command_Update)] | ||
| public async Task<ActionResult<ICModels.IncidentCommandActionResult>> DeleteIncidentMap(string incidentMapId) |
There was a problem hiding this comment.
Missing authorization attribute: DeleteIncidentMap omits [RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)] that its sibling endpoints SaveIncidentMap and UpdateMapView both carry. A user holding the department-level Command_Update claim but lacking the ManageAnnotations capability can delete incident maps, bypassing the incident-scoped authorization check enforced on the create/update path. Add the attribute to DeleteIncidentMap.
[HttpDelete("DeleteIncidentMap/{incidentMapId}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[Authorize(Policy = ResgridResources.Command_Update)]
[RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)]
public async Task<ActionResult<ICModels.IncidentCommandActionResult>> DeleteIncidentMap(string incidentMapId)Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs:
Line 1142 to 1145:
Missing authorization attribute: `DeleteIncidentMap` omits `[RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)]` that its sibling endpoints `SaveIncidentMap` and `UpdateMapView` both carry. A user holding the department-level `Command_Update` claim but lacking the `ManageAnnotations` capability can delete incident maps, bypassing the incident-scoped authorization check enforced on the create/update path. Add the attribute to `DeleteIncidentMap`.
Suggested Code:
[HttpDelete("DeleteIncidentMap/{incidentMapId}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[Authorize(Policy = ResgridResources.Command_Update)]
[RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)]
public async Task<ActionResult<ICModels.IncidentCommandActionResult>> DeleteIncidentMap(string incidentMapId)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [HttpDelete("DeleteIncidentMap/{incidentMapId}")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Command_Update)] | ||
| public async Task<ActionResult<ICModels.IncidentCommandActionResult>> DeleteIncidentMap(string incidentMapId) |
There was a problem hiding this comment.
Authorization bypass: DeleteIncidentMap lacks the [RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)] attribute that its sibling endpoints SaveIncidentMap (line 1115) and UpdateMapView (line 1172) both enforce. Any department member with the coarse Command_Update permission can delete incident maps without the fine-grained ManageAnnotations capability check. Add the attribute to the DeleteIncidentMap action.
[HttpDelete("DeleteIncidentMap/{incidentMapId}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[Authorize(Policy = ResgridResources.Command_Update)]
[RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)]
public async Task<ActionResult<ICModels.IncidentCommandActionResult>> DeleteIncidentMap(string incidentMapId)Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs:
Line 1142 to 1145:
Authorization bypass: `DeleteIncidentMap` lacks the `[RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)]` attribute that its sibling endpoints `SaveIncidentMap` (line 1115) and `UpdateMapView` (line 1172) both enforce. Any department member with the coarse `Command_Update` permission can delete incident maps without the fine-grained `ManageAnnotations` capability check. Add the attribute to the `DeleteIncidentMap` action.
Suggested Code:
[HttpDelete("DeleteIncidentMap/{incidentMapId}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[Authorize(Policy = ResgridResources.Command_Update)]
[RequiresIncidentCapability(IncidentCapabilities.ManageAnnotations)]
public async Task<ActionResult<ICModels.IncidentCommandActionResult>> DeleteIncidentMap(string incidentMapId)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public async Task<ActionResult<ICModels.IncidentCommandBoardResult>> GetCommandBoardById(string incidentCommandId) | ||
| { | ||
| var result = new ICModels.IncidentCommandBoardResult(); | ||
| var board = await _incidentCommandService.GetCommandBoardByIdAsync(DepartmentId, incidentCommandId); |
There was a problem hiding this comment.
Missing input validation: GetCommandBoardById issues the service/DB query without first validating the incidentCommandId route value. Empty or whitespace IDs should be rejected up-front to avoid pointless data-layer calls and ambiguous null results. Add if (string.IsNullOrWhiteSpace(incidentCommandId)) return BadRequest(); before the await, matching the validation pattern used in the mutating actions.
Kody rule violation: Order validations before database queries
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs:
Line 164:
Missing input validation: `GetCommandBoardById` issues the service/DB query without first validating the `incidentCommandId` route value. Empty or whitespace IDs should be rejected up-front to avoid pointless data-layer calls and ambiguous null results. Add `if (string.IsNullOrWhiteSpace(incidentCommandId)) return BadRequest();` before the await, matching the validation pattern used in the mutating actions.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| var result = new ICModels.IncidentNeedResult(); | ||
| var need = await _incidentCommandService.SetNeedStatusAsync(DepartmentId, input.IncidentNeedId, (IncidentNeedStatus)input.Status, input.QuantityFulfilled, UserId, CancellationToken.None); | ||
| var need = await _incidentCommandService.SetNeedStatusAsync(DepartmentId, input.IncidentNeedId, (IncidentNeedStatus)input.Status, input.QuantityFulfilled, UserId, input.Note, CancellationToken.None); |
There was a problem hiding this comment.
Unvalidated enum cast: (IncidentNeedStatus)input.Status directly casts user-supplied input without verification. An out-of-range integer silently becomes an invalid enum value, risking downstream errors. Validate before casting, e.g. guard with Enum.IsDefined(typeof(IncidentNeedStatus), input.Status) or return BadRequest when the value is undefined, matching the pattern used at line 857.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs:
Line 918:
Unvalidated enum cast: `(IncidentNeedStatus)input.Status` directly casts user-supplied input without verification. An out-of-range integer silently becomes an invalid enum value, risking downstream errors. Validate before casting, e.g. guard with `Enum.IsDefined(typeof(IncidentNeedStatus), input.Status)` or return `BadRequest` when the value is undefined, matching the pattern used at line 857.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Resgrid.Framework.Logging.LogException(ex); |
There was a problem hiding this comment.
Contextless error logging: the catch block logs only the raw exception without any operation name or relevant identifiers such as department id, destination id, or user id. Pass structured context into the log call, e.g. Resgrid.Framework.Logging.LogException(ex, $"RecordNeedEntityStatusAsync failed for Department {DepartmentId}, Destination {destinationId}, User {input.UserId}").
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs:
Line 178:
Contextless error logging: the catch block logs only the raw exception without any operation name or relevant identifiers such as department id, destination id, or user id. Pass structured context into the log call, e.g. `Resgrid.Framework.Logging.LogException(ex, $"RecordNeedEntityStatusAsync failed for Department {DepartmentId}, Destination {destinationId}, User {input.UserId}")`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
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:
|
| foreach (var callId in selected.Select(x => x.CallId).Distinct()) | ||
| { | ||
| try | ||
| { | ||
| var loadedCall = await _callsService.GetCallByIdAsync(callId); |
There was a problem hiding this comment.
N+1 query pattern occurs as the foreach loop issues a separate database round-trip via GetCallByIdAsync for each distinct CallId. Add a batched repository method such as GetCallsByIdsAsync(IEnumerable<int> ids) using WHERE Id IN (...) to load all calls in a single query.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 803 to 807:
N+1 query pattern occurs as the `foreach` loop issues a separate database round-trip via `GetCallByIdAsync` for each distinct `CallId`. Add a batched repository method such as `GetCallsByIdsAsync(IEnumerable<int> ids)` using `WHERE Id IN (...)` to load all calls in a single query.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var callId in selected.Select(x => x.CallId).Distinct()) | ||
| { | ||
| try | ||
| { | ||
| var loadedCall = await _callsService.GetCallByIdAsync(callId); |
There was a problem hiding this comment.
N+1 query inefficiency results from loading calls individually via GetCallByIdAsync inside the loop, creating excessive database round-trips. Implement a batched API like GetCallsByIdsAsync backed by WHERE Id IN @ids or join the Call rows up front when querying selected.
Kody rule violation: Optimize database queries with JOINs
Prompt for LLM
File Core/Resgrid.Services/IncidentCommandService.cs:
Line 803 to 807:
N+1 query inefficiency results from loading calls individually via `GetCallByIdAsync` inside the loop, creating excessive database round-trips. Implement a batched API like `GetCallsByIdsAsync` backed by `WHERE Id IN @ids` or join the `Call` rows up front when querying `selected`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| try | ||
| { | ||
| var stateLabel = Enum.IsDefined(typeof(UnitStateTypes), state.State) ? ((UnitStateTypes)state.State).ToString() : $"status {state.State}"; | ||
| await _incidentCommandService.RecordNeedEntityStatusAsync(DepartmentId, state.DestinationId.Value, NeedEntityKind.Unit, state.UnitId.ToString(), stateLabel, UserId, cancellationToken); |
There was a problem hiding this comment.
InvalidOperationException risk arises as state.DestinationId.Value is accessed without verifying the underlying nullable type has a value. Guard with a HasValue check, such as if (!state.DestinationId.HasValue) return BadRequest(...);, before calling RecordNeedEntityStatusAsync.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs:
Line 292:
InvalidOperationException risk arises as `state.DestinationId.Value` is accessed without verifying the underlying nullable type has a value. Guard with a `HasValue` check, such as `if (!state.DestinationId.HasValue) return BadRequest(...);`, before calling `RecordNeedEntityStatusAsync`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
3 participants
PR Description
This PR delivers a substantial set of Incident Command (IC) feature enhancements, data model extensions, and auditability improvements across the service layer, data layer, and API controllers.
Key Changes
Command Lifecycle
Incident Information & Locations
Incident List Summaries
Entity-Category Needs
Need Fulfillment Audit Trail
IncidentNeedUpdates) capturing every status and fill-quantity change (including reductions) with an optional note, author, and timestamp. New endpoint exposes the audit history per need.Tactical Objective Close-Out
Incident Maps
Timeline Attribution
Database & Tests
IncidentNeedUpdates,IncidentMaps,IncidentNeedEntities).Summary by CodeRabbit