Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ✅ |
📝 WalkthroughWalkthroughAdds offline sync bundle/reference endpoints with new department-wide DTOs, service contracts, and aggregation logic. Also adds a per-incident capability filter and applies it to incident command, resource, role, and voice actions, and wires incident command updates through eventing and SignalR. ChangesOffline Incident Sync Endpoints
Per-Incident Capability Authorization Filter
Incident Command Update Event Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/SyncService.cs`:
- Around line 56-88: GetReferenceDataAsync currently does all reference/service
reads serially and bypasses the required department cache-aside pattern. Update
SyncService.GetReferenceDataAsync to accept and pass through a bypassCache flag
where needed, then wrap the full payload build in
ICacheProvider.RetrieveAsync<T>() with a local async fallback that calls the
existing services. Keep the safe Group/Personnel projection logic, but ensure
the cache key is department-scoped so callers can reuse or refresh the reference
bootstrap consistently.
In `@Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs`:
- Line 39: `EstablishCommand` is being blocked by `RequiresIncidentCapability`,
which prevents the bootstrap command context from being created before a
command/role exists. Update `IncidentCommandController.EstablishCommand` to use
the existing `Command_Create` policy, or adjust the capability check in the
filter/service so bootstrap establishment is exempt from
`GetCapabilitiesForUserAsync` until the command is initialized.
In `@Web/Resgrid.Web.Services/Controllers/v4/SyncController.cs`:
- Around line 81-87: The controller-level loop in
SyncController.GetBundleForDepartment is doing N+1 ad-hoc lookups by calling
IncidentResourcesService once per board. Move this aggregation into a single
batched call on IncidentResourcesService, or fold it into
GetBundleForDepartmentAsync so bundle.AdHocUnits and bundle.AdHocPersonnel are
populated in one pass. Keep the existing board/callId mapping logic, but replace
the per-board AddRange fetches with a single service method that returns all
ad-hoc data for the department’s active incidents.
🪄 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: 63fa3c79-b938-4413-8726-c082078e5379
⛔ Files ignored due to path filters (4)
Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/SyncServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/RequiresIncidentCapabilityFilterTests.csis excluded by!**/Tests/**docs/architecture/offline-first-architecture.mdis excluded by!**/*.md
📒 Files selected for processing (15)
Core/Resgrid.Model/IncidentCommand/IncidentCommandBundle.csCore/Resgrid.Model/IncidentCommand/SyncReferenceData.csCore/Resgrid.Model/Services/IIncidentCommandService.csCore/Resgrid.Model/Services/ISyncService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/SyncService.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentResourcesController.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentRolesController.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentVoiceController.csWeb/Resgrid.Web.Services/Controllers/v4/SyncController.csWeb/Resgrid.Web.Services/Filters/RequiresIncidentCapabilityAttribute.csWeb/Resgrid.Web.Services/Models/v4/Sync/SyncModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/SyncService.cs`:
- Around line 33-34: The SyncService reference snapshot is using a hard-coded
5-minute cache lifetime instead of the standard department-data TTL. Update the
caching setup in SyncService by replacing the CacheLength constant with the
shared department cache duration used across the repository, and keep
SyncReferenceData aligned with the repository cache contract. If any part of the
payload needs fresher data, move that volatile subset out rather than shortening
the whole snapshot.
In `@Web/Resgrid.Web.Services/Controllers/v4/SyncController.cs`:
- Around line 107-108: The PageSize calculation in SyncController’s response
only includes Units and Personnel, so the reported metadata is incomplete for
sync payloads. Update the SyncReferenceResult.PageSize assignment to count all
returned collections in the sync reference data, including call types,
priorities, templates, groups, POIs, protocols, timer configs, custom states,
and features, using the same SyncController result-building logic.
- Around line 103-105: The Reference endpoint currently exposes mobile numbers
via SyncService.GetReferenceDataAsync, but SyncController.Reference is only
gated by Command_View. Update the Reference flow to either omit
SyncReferenceData.MobilePhone from the returned payload or conditionally
populate it only when the caller passes the same CanViewPII check used
elsewhere, and make the change in the Reference action and/or the
GetReferenceDataAsync mapping so the PII gate is enforced consistently.
🪄 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: 611c138e-d54e-4f17-b1ae-b83058d4d4da
⛔ Files ignored due to path filters (3)
Tests/Resgrid.Tests/Services/IncidentResourcesServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/SyncServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/RequiresIncidentCapabilityFilterTests.csis excluded by!**/Tests/**
📒 Files selected for processing (8)
Core/Resgrid.Model/IncidentCommand/SyncReferenceData.csCore/Resgrid.Model/Services/IIncidentResourcesService.csCore/Resgrid.Model/Services/ISyncService.csCore/Resgrid.Services/IncidentResourcesService.csCore/Resgrid.Services/SyncService.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Controllers/v4/SyncController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/Resgrid.Model/Services/ISyncService.cs
- Core/Resgrid.Model/IncidentCommand/SyncReferenceData.cs
| private static readonly string CacheKey = "SyncReferenceData_{0}"; | ||
| private static readonly TimeSpan CacheLength = TimeSpan.FromMinutes(5); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Use the standard department-data TTL here.
SyncReferenceData is a department-scoped reference snapshot, but this hard-codes a 5-minute lifetime. That will repopulate the full multi-service fan-out far more often than the repository cache contract expects. If part of this payload truly needs near-live freshness, split that volatile subset out instead of shortening the whole snapshot. As per coding guidelines, "most user/department data is cached for 1 day. When testing or requiring fresh data, use bypassCache: true parameter or call invalidation methods."
♻️ Proposed change
- private static readonly TimeSpan CacheLength = TimeSpan.FromMinutes(5);
+ private static readonly TimeSpan CacheLength = TimeSpan.FromDays(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static readonly string CacheKey = "SyncReferenceData_{0}"; | |
| private static readonly TimeSpan CacheLength = TimeSpan.FromMinutes(5); | |
| private static readonly string CacheKey = "SyncReferenceData_{0}"; | |
| private static readonly TimeSpan CacheLength = TimeSpan.FromDays(1); |
🤖 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/SyncService.cs` around lines 33 - 34, The SyncService
reference snapshot is using a hard-coded 5-minute cache lifetime instead of the
standard department-data TTL. Update the caching setup in SyncService by
replacing the CacheLength constant with the shared department cache duration
used across the repository, and keep SyncReferenceData aligned with the
repository cache contract. If any part of the payload needs fresher data, move
that volatile subset out rather than shortening the whole snapshot.
Source: Coding guidelines
| public async Task<ActionResult<SyncReferenceResult>> Reference(bool bypassCache = false) | ||
| { | ||
| var data = await _syncService.GetReferenceDataAsync(DepartmentId, bypassCache); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Compare the Sync reference endpoint's authorization with personnel/PII-related endpoints
# and confirm where mobile numbers are projected into the offline reference payload.
rg -n -C2 'Authorize\(Policy = .*Command_View|Authorize\(Policy = .*Personnel_View|CanViewPII|MobilePhone' \
Web/Resgrid.Web.Services/Controllers/v4 \
Core/Resgrid.Services \
Core/Resgrid.ModelRepository: Resgrid/Core
Length of output: 19108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether Command_View is used for other endpoints that expose roster-like data,
# and whether there is a distinct policy for personnel PII exposure.
rg -n -C2 'Authorize\(Policy = ResgridResources\.(Command_View|Personnel_View)|CanViewPII|MobileNumber|MobilePhone' \
Web/Resgrid.Web.Services/Controllers/v4 \
Core/Resgrid.Services \
Core/Resgrid.Model \
Core/Resgrid.FrameworkRepository: Resgrid/Core
Length of output: 34377
Gate Reference mobile numbers behind a PII check. SyncService.GetReferenceDataAsync() copies UserProfile.MobileNumber into SyncReferenceData.MobilePhone, but SyncController.Reference() is only protected by Command_View. If command access shouldn’t include phone numbers, strip that field from the payload or apply the same CanViewPII gate used elsewhere.
🤖 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/SyncController.cs` around lines 103 -
105, The Reference endpoint currently exposes mobile numbers via
SyncService.GetReferenceDataAsync, but SyncController.Reference is only gated by
Command_View. Update the Reference flow to either omit
SyncReferenceData.MobilePhone from the returned payload or conditionally
populate it only when the caller passes the same CanViewPII check used
elsewhere, and make the change in the Reference action and/or the
GetReferenceDataAsync mapping so the PII gate is enforced consistently.
| var result = new SyncReferenceResult { Data = data }; | ||
| result.PageSize = data.Units.Count + data.Personnel.Count; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
PageSize only counts two collections.
The reference response also returns call types, priorities, templates, groups, POIs, protocols, timer configs, custom states, and features. Reporting only Units.Count + Personnel.Count makes the standard metadata wrong for every non-trivial payload.
🤖 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/SyncController.cs` around lines 107 -
108, The PageSize calculation in SyncController’s response only includes Units
and Personnel, so the reported metadata is incomplete for sync payloads. Update
the SyncReferenceResult.PageSize assignment to count all returned collections in
the sync reference data, including call types, priorities, templates, groups,
POIs, protocols, timer configs, custom states, and features, using the same
SyncController result-building logic.
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Web/Resgrid.Web.Eventing/Worker.cs (1)
114-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the framework logger instead of
Console.WriteLine.This new handler is bypassing the standard logging pipeline, so these event traces won't be handled consistently with the rest of the service.
As per coding guidelines, use
Resgrid.Framework.Loggingstatic methods (LogException,LogError,LogInfo,LogDebug) for all logging throughout the codebase.Suggested change
- Console.WriteLine($"Processing RabbitMQ IncidentCommandUpdated Event For {departmentId}"); + Resgrid.Framework.Logging.LogInfo($"Processing RabbitMQ IncidentCommandUpdated Event For {departmentId}");🤖 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.Eventing/Worker.cs` around lines 114 - 121, The IncidentCommandUpdated handler is writing trace output directly with Console.WriteLine, bypassing the standard logging pipeline. Replace that call in Worker.IncidentCommandUpdated with the appropriate Resgrid.Framework.Logging static method (for example LogInfo or LogDebug) so event processing is logged consistently with the rest of the service. Keep the existing context in the message, and leave the hub send logic unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/CoreEventService.cs`:
- Around line 15-17: `IncidentCommandUpdatedAsync` now relies on an in-process
send and fire-and-forget publish, which can lose the only client sync trigger
before persistence. Update `CoreEventService` so the event path uses a durable,
awaited handoff before returning—either restore the previous CQRS enqueue or
switch `SendMessage`/the downstream publisher path to a persisted publish
mechanism and await completion in `IncidentCommandUpdatedAsync`.
---
Nitpick comments:
In `@Web/Resgrid.Web.Eventing/Worker.cs`:
- Around line 114-121: The IncidentCommandUpdated handler is writing trace
output directly with Console.WriteLine, bypassing the standard logging pipeline.
Replace that call in Worker.IncidentCommandUpdated with the appropriate
Resgrid.Framework.Logging static method (for example LogInfo or LogDebug) so
event processing is logged consistently with the rest of the service. Keep the
existing context in the message, and leave the hub send logic unchanged.
🪄 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: 5b70a498-0942-4bc3-bd6f-136a5f2488ad
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Services/CoreEventServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (9)
Core/Resgrid.Model/EventingTypes.csCore/Resgrid.Model/Events/IncidentCommandEvents.csCore/Resgrid.Model/Providers/IRabbitInboundEventProvider.csCore/Resgrid.Model/Services/ICoreEventService.csCore/Resgrid.Services/CoreEventService.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.csProviders/Resgrid.Providers.Bus/OutboundEventProvider.csWeb/Resgrid.Web.Eventing/Worker.cs
✅ Files skipped from review due to trivial changes (1)
- Core/Resgrid.Model/Services/ICoreEventService.cs
| public CoreEventService(IEventAggregator eventAggregator) | ||
| { | ||
| _eventAggregator = eventAggregator; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep a durable handoff for IncidentCommandUpdatedAsync.
This now returns before the update is persisted anywhere. SendMessage(...) only dispatches in-process, and the new downstream publisher path is fire-and-forget, so a Rabbit publish failure or process recycle can drop the only sync trigger for connected IC clients.
Either keep the previous CQRS enqueue here or move this event rail to an awaitable/persisted publish path before returning.
Also applies to: 28-39
🤖 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/CoreEventService.cs` around lines 15 - 17,
`IncidentCommandUpdatedAsync` now relies on an in-process send and
fire-and-forget publish, which can lose the only client sync trigger before
persistence. Update `CoreEventService` so the event path uses a durable, awaited
handoff before returning—either restore the previous CQRS enqueue or switch
`SendMessage`/the downstream publisher path to a persisted publish mechanism and
await completion in `IncidentCommandUpdatedAsync`.
| case EventingTypes.IncidentCommandUpdated: | ||
| if (ProcessIncidentCommandUpdated != null) | ||
| await ProcessIncidentCommandUpdated.Invoke(eventingMessage.DepartmentId, eventingMessage.ItemId); | ||
| break; |
There was a problem hiding this comment.
Unhandled exception vulnerability in RabbitInboundEventProvider.cs and Web/Resgrid.Web.Eventing/Worker.cs:114-122 where await ProcessIncidentCommandUpdated.Invoke(...) executes without a try/catch. A thrown callback, such as a SignalR SendAsync failure, propagates unhandled through the async-void consumer.ReceivedAsync handler and risks consumer instability without logging; wrap the invocation in a try/catch block to log exceptions.
Kody rule violation: Handle async operations with proper error handling
case EventingTypes.IncidentCommandUpdated:
if (ProcessIncidentCommandUpdated != null)
{
try
{
await ProcessIncidentCommandUpdated.Invoke(eventingMessage.DepartmentId, eventingMessage.ItemId);
}
catch (Exception ex)
{
Framework.Logging.LogException(ex);
}
}
break;Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundEventProvider.cs:
Line 118 to 121:
Violates rule 'Handle async operations with proper error handling': the newly added 'case EventingTypes.IncidentCommandUpdated' block calls 'await ProcessIncidentCommandUpdated.Invoke(...)' without try/catch. If the invoked callback throws (e.g., SignalR SendAsync failure in Worker.IncidentCommandUpdated), the exception propagates unhandled through the async-void consumer.ReceivedAsync event handler, risking consumer instability with no error logging.
Suggested Code:
case EventingTypes.IncidentCommandUpdated:
if (ProcessIncidentCommandUpdated != null)
{
try
{
await ProcessIncidentCommandUpdated.Invoke(eventingMessage.DepartmentId, eventingMessage.ItemId);
}
catch (Exception ex)
{
Framework.Logging.LogException(ex);
}
}
break;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
Summary
This PR implements the backend infrastructure for the Resgrid IC (Incident Command) app's offline-first shift-start workflow, comprising three major additions: (1) two new aggregate sync endpoints that let a field client pull everything it needs in minimal round-trips, (2) a per-endpoint incident-capability authorization layer, and (3) associated tests and documentation.
1. Shift-start aggregate sync endpoints
GET /api/v4/Sync/Bundle— Returns a render-readyIncidentCommandBoard(lanes, resources, objectives, timers, roles, annotations, and computed accountability/PAR) for every active incident in the caller's department, plus active ad-hoc units and personnel, in a single call. The client uses the returnedServerTimestampMsas the cursor for subsequent incremental/Sync/Changespulls.IncidentCommandService.GetBundleForDepartmentAsyncscans each board table once for the whole department and groups byCallIdin memory — O(tables) instead of the previous O(active incidents × department size) per-incident N+1 pattern.GetCommandBoardAsync, it does not run the write-side PAR sweep (no marker writes or SignalR pushes).?includeAccountability=falselets very busy departments skip the per-incident PAR computation.GET /api/v4/Sync/Reference— Returns the slowly-changing department configuration and a safe personnel roster needed to start and run an incident offline (call types, priorities, command templates, units, groups, POIs, protocols, custom statuses, feature flags).SyncServiceaggregates data from 12 existing services into a singleSyncReferenceDatapayload.ReferencePersonnel,ReferenceGroup) that structurally excludeIdentityUsernavs, password/security fields, andUserProfilecontact-verification secrets.ReferenceCacheEnvelope);?bypassCache=trueforces a fresh read.Batched ad-hoc resources:
IncidentResourcesService.GetActiveAdHocResourcesForDepartmentAsyncreturns all active (non-released) ad-hoc units and personnel scoped to the department's active incidents in one scan per table, replacing the per-incident N+1 lookups.2. Incident-scoped capability authorization
A new
RequiresIncidentCapabilityAttributeaction filter enforces that the calling user holds the requiredIncidentCapabilities(e.g.,ManageStructure,AssignResources,ManageAccountability) for the specific incident targeted by the request — layered on top of the existing broad[Authorize(Policy = Command_*)]claims.callIdroute value,IncidentCommandId(resolved to its Call with department-ownership verification), orCallIdon the bound body.IncidentCommandController,IncidentResourcesController,IncidentRolesController, andIncidentVoiceController.EstablishCommand(it creates the command, so no capabilities exist yet) or to entity-id "second action" verbs where the target Call isn't on the request.3. Tests & documentation
docs/architecture/offline-first-architecture.mdwith the authoritative shift-start manifest (§6.1) and marked the delta and aggregate endpoints as done (§9).Summary by CodeRabbit
New Features
Bug Fixes