Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughAdds a native tracker gateway with Queclink, GT06, and Teltonika protocol handling, validated tracking catalogs, TCP/UDP listeners, monitoring, Docker deployment, and unit-location retention. It also adds IC-specific push routing and an Incident Command messaging API. ChangesTracking platform
Incident Command messaging
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (15)
Workers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs (1)
109-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent early-exit on
maximumRowscap.When the global
totalDeletedbudget is hit, the loop breaks without any log/metric, silently skipping the remaining departments (processed in fixed ascendingdepartmentIdorder) for that run. Over time this could starve higher-ID departments of retention processing without any operator visibility.📋 Suggested fix
cancellationToken .ThrowIfCancellationRequested(); if (totalDeleted >= maximumRows) + { + Logging.LogInfo( + $"Unit tracking retention: reached max rows-per-run cap ({maximumRows}); {departmentIds.Count - processedDepartments} department(s) deferred to next run."); break; + }🤖 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/UnitTrackingRetentionLogic.cs` around lines 109 - 115, Update the totalDeleted versus maximumRows guard in the departmentIds loop to emit an appropriate warning log or metric before breaking, including that the global retention cap was reached and remaining departments were skipped. Preserve the existing break behavior and ensure the notification is emitted once when the cap causes the early exit.Docker/docker-compose.yml (1)
163-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray trailing whitespace on blank line.
Line 163 is a blank line with trailing spaces between the
elkandmongodbblocks; harmless but likely to trip YAML lint/whitespace checks.🤖 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 `@Docker/docker-compose.yml` at line 163, Remove the trailing spaces from the blank line separating the elk and mongodb service blocks in the Docker Compose configuration, leaving the blank line otherwise unchanged.Workers/Resgrid.TrackerGateway/Health/TrackingGatewayMetrics.cs (1)
92-108: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGauge decrement is racily clamped, which can slightly understate current connections under concurrency.
ConnectionCompleteddoesMath.Max(0, current - 1)insideAddOrUpdate, so if two completions race for the same key, one decrement can be "lost" relative toConnectionStarted's increments (though it will self-correct as further starts/completions occur). Given this is best-effort observability data rather than a correctness-critical counter, this is a minor, self-recovering drift and not worth blocking on.🤖 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.TrackerGateway/Health/TrackingGatewayMetrics.cs` around lines 92 - 108, Update the current-connection gauge decrement in ConnectionCompleted to avoid the racily clamped Math.Max(0, current - 1) AddOrUpdate behavior. Use an atomic decrement approach consistent with ConnectionStarted, while preserving the non-negative gauge requirement and existing outcome increment..github/workflows/dotnet.yml (1)
150-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
\bPR\b→Releasereplacement may mangle unrelated text.This regex replaces every standalone occurrence of "PR" in the PR body (not just the heading phrases handled above), which could produce awkward text if "PR" appears elsewhere in the description (e.g., "See PR
#123for context" → "See Release#123for context"). This is also unrelated to the tracker-gateway feature this PR introduces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/dotnet.yml at line 150, The replacement in the PR-body formatting step should not globally convert standalone “PR” occurrences. Remove the broad `.replace(/\bPR\b/g, 'Release')` call and preserve only the targeted heading replacements already handled in the surrounding workflow logic.Workers/Resgrid.TrackerGateway/Dockerfile (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ARG BUILD_VERSION=3.5.0default looks stale for this project.The CI workflow always supplies
BUILD_VERSION=4.${{ github.run_number }}.0, so this default is effectively dead, but it's inconsistent with the versioning scheme and reads like it was copy-pasted from another Dockerfile.🤖 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.TrackerGateway/Dockerfile` at line 3, Update the BUILD_VERSION default argument in the Dockerfile to match this project's 4.x versioning scheme instead of the stale 3.5.0 value, while preserving the CI-provided BUILD_VERSION override.Providers/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs (2)
314-319: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHard-coded catalog profile key.
"jimi-vl103m"duplicates a key owned byunit-tracking-catalog.json. Promote it to a shared constant (next toTrackingProtocolKeys) so a catalog rename doesn't silently change alarm mapping.🤖 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.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs` around lines 314 - 319, Replace the hard-coded "jimi-vl103m" comparison in Gt06ProtocolSession with a shared constant declared alongside TrackingProtocolKeys, and use that constant in the MapAlarm call so the catalog profile key has a single source of truth.
501-525: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer explicit validation over exception-driven parsing.
TryReadTimestamprelies onDateTimethrowing for attacker-controlled bytes; malformed frames at line rate make this a needless allocation/throw hot path.DateTime.TryParse-style range checks (ornew DateTime(...)guarded byDateTime.DaysInMonth) avoid it.🤖 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.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs` around lines 501 - 525, Update TryReadTimestamp to validate the decoded month and day ranges explicitly before constructing DateTime, using bounds checks and DateTime.DaysInMonth as needed; remove the exception-driven catch path while preserving false for malformed six-byte timestamps and true with UTC output for valid values.Core/Resgrid.Model/Tracking/UnitTrackingCatalog.cs (1)
25-47: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider precomputed case-insensitive dictionaries for lookups.
GetProfile/GetIoMapdo a linear scan per call. If these are hit on ingress paths (per device connect/message), buildDictionary<string, ...>(StringComparer.OrdinalIgnoreCase)once inside the lazy document instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Model/Tracking/UnitTrackingCatalog.cs` around lines 25 - 47, Replace the per-call linear scans in GetProfile and GetIoMap with case-insensitive dictionaries initialized once within the existing lazy catalog/document initialization. Normalize lookup keys consistently, preserve null/whitespace handling, and retrieve profiles and I/O maps by key using Dictionary<string, ...>(StringComparer.OrdinalIgnoreCase).Core/Resgrid.Services/UnitTrackingIngressService.cs (1)
137-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared device/tenant/identifier validation.
The device-enabled check, unit/tenant-mismatch check, and identifier-match check in
AcceptHeartbeatAsync(Lines 148-183) duplicate the equivalent blocks already inAcceptAsync(Lines 53-81). Consider extracting a shared private helper (e.g.,ValidateBindingAsync(source, receivedOn, cancellationToken)returning either the validated device/unit or anInvalidresult) so both entry points stay in sync as validation rules evolve.🤖 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/UnitTrackingIngressService.cs` around lines 137 - 196, The validation flow in AcceptHeartbeatAsync duplicates the device-enabled, unit/tenant, and identifier checks in AcceptAsync. Extract these checks into a shared private helper such as ValidateBindingAsync, returning the validated binding context or the appropriate Invalid result, then update both entry points to use it while preserving their existing status-update and response behavior.Providers/Resgrid.Providers.Tracking/Protocols/Queclink/QueclinkProtocolSession.cs (1)
87-669: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for this new parser.
This is a complex, hand-rolled binary/ASCII protocol decoder handling untrusted network input (framing, header parsing, multi-location extraction, heuristic field scans). No test file is included in this diff. Given the correctness risk of protocol-specific offset math and the difficulty of manually verifying it, consider adding unit tests using known-good/malformed Queclink sample frames (RESP/BUFF single- and multi-location, ACK:GTHBD heartbeat, oversized/malformed frames).
🤖 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.Tracking/Protocols/Queclink/QueclinkProtocolSession.cs` around lines 87 - 669, Add focused unit tests covering the QueclinkProtocolSession.Parse and BuildResponse flows with known-good and malformed frames: RESP/BUFF single- and multi-location messages, ACK:GTHBD heartbeat acknowledgements, oversized frames, and invalid framing or fields. Assert statuses, consumed data, parsed positions, identifiers, acknowledgement tokens, and rejection reason codes to exercise the offset and validation logic.Workers/Resgrid.TrackerGateway/Hosting/TrackingGatewaySettings.cs (1)
33-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
IsEnabled(transport)ignores the protocol-levelEnabledflag.Today the only caller (
TrackingListenerPlanBuilder) pre-filters onprotocol.Enabled, so behavior is correct — but the name implies a full enablement check and a future caller would silently bind a listener for a disabled protocol. FoldingEnabledin makes the helper self-contained.♻️ Suggested tightening
public bool IsEnabled(TrackingSocketTransport transport) { + if (!Enabled) + return false; + switch (transport)🤖 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.TrackerGateway/Hosting/TrackingGatewaySettings.cs` around lines 33 - 44, Update IsEnabled(TrackingSocketTransport) to require the protocol-level Enabled flag in addition to the transport-specific TcpEnabled or UdpEnabled setting. Preserve the existing false result for unsupported transports and ensure every supported transport returns false when Enabled is false.Providers/Resgrid.Providers.Tracking/Protocols/TrackingProtocolContract.cs (1)
49-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
init-only setters for these contract DTOs.
TrackingSessionContext,ProtocolMessage,ProtocolParseResult, andTrackingAcceptanceare all created via object initializers in the protocol modules and the gateway handler, so switchingset→initcosts nothing and prevents post-parse mutation of the contract surface. NoteTrackingTransportSessionHandlerdoes mutatemessage.ProtocolData = nullafter enrichment, so that one property would need to stay settable (or move to an explicitClearProtocolData()).As per coding guidelines, "Prefer functional patterns and immutable data where appropriate in C#".
🤖 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.Tracking/Protocols/TrackingProtocolContract.cs` around lines 49 - 84, Update the contract DTO properties in TrackingSessionContext, ProtocolMessage, ProtocolParseResult, and TrackingAcceptance from set-only mutation to init-only setters so they cannot be modified after object initialization. Preserve ProtocolMessage.ProtocolData as settable because TrackingTransportSessionHandler clears it after enrichment; leave the existing default initialization for Positions intact.Source: Coding guidelines
Workers/Resgrid.TrackerGateway/Sessions/TrackingTransportSessionHandler.cs (1)
597-631: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDevice lookup runs on every frame of a TCP session.
ResolveSourceAsyncis invoked per message, so a device sending positions every few seconds re-hitsGetEnabledDeviceByProtocolIdentifierAsyncfor each frame, plus once per heartbeat. Oncestate.Sourceis established andmessage.ExternalIdentifiermatchesstate.Source.ReportedDeviceIdentifier, the lookup can be skipped (the identifier-changed and CIDR checks still hold from the initial resolution). If the lookup is only backed by the DB rather thanICacheProvider, this is a hot-path amplifier at connection scale.Note this only applies to the TCP path (
useGeneration: true); UDP resolves once per datagram by design.🤖 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.TrackerGateway/Sessions/TrackingTransportSessionHandler.cs` around lines 597 - 631, Update ResolveSourceAsync’s TCP path (useGeneration: true) to reuse the established state.Source when message.ExternalIdentifier is absent or matches state.Source.ReportedDeviceIdentifier, skipping GetEnabledDeviceByProtocolIdentifierAsync for those frames while preserving the existing identifier-changed and CIDR validation. Keep the current lookup behavior for initial resolution or changed identifiers, and leave UDP datagram resolution unchanged.Workers/Resgrid.TrackerGateway/Listeners/UdpTrackingListener.cs (1)
244-264: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
SendToAsyncon the shared_listenerSocketcan race with shutdown disposal.
StopAsyncdisposes_listenerSocketin itsfinallywhile forced-shutdown datagrams are still running, so this send can hitObjectDisposedException, which the generic handler below logs as a warning ("datagram failed") on a normal timeout shutdown. Capturing the socket and treatingObjectDisposedExceptionas a cancellation-style outcome keeps shutdown logs clean.🤖 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.TrackerGateway/Listeners/UdpTrackingListener.cs` around lines 244 - 264, The UDP response send in the listener’s datagram handling flow can race with StopAsync disposal. Capture the current _listenerSocket before awaiting SendToAsync and treat ObjectDisposedException from that send as a cancellation-style shutdown outcome, while preserving normal send behavior and avoiding the generic “datagram failed” warning.Workers/Resgrid.TrackerGateway/Hosting/TrackingListenerPlanBuilder.cs (1)
97-107: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
SingleOrDefaultthrows an opaqueInvalidOperationExceptionon duplicate module keys.Two modules registered with the same protocol key (case-insensitive) escape the accumulated-error path and surface as "Sequence contains more than one matching element" at startup instead of a
TrackingGatewayConfigurationException. Collect the matches instead so duplicates become a normal validation error.♻️ Suggested handling
- var module = moduleRegistry.Modules.SingleOrDefault( - candidate => string.Equals( - candidate.ProtocolKey.Trim(), - protocol.ProtocolKey, - StringComparison.OrdinalIgnoreCase)); - if (module == null) + var matches = moduleRegistry.Modules + .Where(candidate => string.Equals( + candidate.ProtocolKey?.Trim(), + protocol.ProtocolKey, + StringComparison.OrdinalIgnoreCase)) + .ToList(); + if (matches.Count > 1) + { + errors.Add( + $"More than one tracking protocol module is registered for '{protocol.ProtocolKey}'."); + continue; + } + + var module = matches.FirstOrDefault(); + if (module == null) { errors.Add( $"No tracking protocol module is registered for '{protocol.ProtocolKey}'."); 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 `@Workers/Resgrid.TrackerGateway/Hosting/TrackingListenerPlanBuilder.cs` around lines 97 - 107, Update the module lookup in the tracking listener plan-building flow around moduleRegistry.Modules to collect all case-insensitive ProtocolKey matches instead of using SingleOrDefault. Treat zero matches as the existing missing-module validation error, exactly one match as the selected module, and duplicates as an accumulated configuration validation error that ultimately produces TrackingGatewayConfigurationException.
🤖 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 `@Providers/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs`:
- Around line 412-429: The NormalizeBattery and NormalizeSignal methods
incorrectly infer encoding from the observed magnitude; replace these thresholds
with profile/model-based decoding using the catalog decoderVariant. Route each
value through the variant’s appropriate level-to-percentage or native-percentage
interpretation, preserving null handling for invalid values and ensuring true
low percentages remain unchanged.
In
`@Providers/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaCodec8ProtocolSession.cs`:
- Around line 356-358: Update the altitude parsing in the Teltonika codec
session to use a signed 16-bit read instead of TryReadUInt16, storing the result
in altitude as short so below-sea-level values remain negative before assignment
to AltitudeMeters.
In
`@Providers/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaIoMapper.cs`:
- Around line 110-112: Update the raw-value range check in TeltonikaIoMapper so
nullable MinimumRawValue and MaximumRawValue are only compared when present;
treat an omitted bound as unchecked while preserving rejection for values
outside any specified bound.
In
`@Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationRetentionRepository.cs`:
- Around line 9-29: Update UnitLocationRetentionRepository to use a
parameterless constructor that resolves IUnitLocationsDocRepository and
IUnitLocationsMongoRepository through Bootstrapper.GetKernel().Resolve<T>(),
matching the UnitTrackingRetentionLogic pattern. Retain a secondary constructor
accepting the Lazy dependencies for testability, including the existing null
validation.
In `@Workers/Resgrid.TrackerGateway/Listeners/TcpTrackingListener.cs`:
- Around line 153-204: Wrap the per-connection setup in AcceptLoopAsync,
including NoDelay, RemoteEndPoint access, admission handling, and
ProcessConnectionAsync startup, so peer-reset SocketException or
ObjectDisposedException cannot terminate the accept loop. Add a non-cancellation
SocketException catch around AcceptAsync that logs the transient accept failure
and continues accepting; preserve the existing cancellation returns and ensure
any acquired admission lease or socket is released when setup fails.
In `@Workers/Resgrid.TrackerGateway/Listeners/TrackingListenerSupervisor.cs`:
- Around line 112-124: Update the listener shutdown loop in
TrackingListenerSupervisor.StopAsync to isolate each listeners[index].StopAsync
failure so one exception cannot prevent remaining listeners from being stopped.
Handle errors per listener while preserving readiness marking for successfully
stopped listeners, then always execute the existing base.StopAsync call in the
finally block.
In `@Workers/Resgrid.TrackerGateway/Listeners/UdpTrackingListener.cs`:
- Around line 151-193: Update the ReceiveFromAsync exception handling in the UDP
listener loop to catch non-fatal SocketException instances, including
SocketError.ConnectionReset, log the event, and continue receiving datagrams so
_completion is not faulted. Preserve the existing cancellation and MessageSize
handling, and optionally disable SIO_UDP_CONNRESET during socket binding if that
configuration path is already available.
In `@Workers/Resgrid.Workers.Console/Tasks/UnitTrackingRetentionTask.cs`:
- Around line 41-59: Update the exception handling around the task’s Process
flow to treat OperationCanceledException as normal cancellation: do not log it
as a failure, and rethrow it unchanged. For other exceptions, remove the
duplicate Logging.LogException/error logging in this catch block and propagate
the original exception without wrapping it in a new InvalidOperationException.
In `@Workers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs`:
- Around line 89-121: Update the department iteration in the retention sweep to
avoid forcing fresh settings for every department: call
GetHardwareTrackingLocationRetentionDaysAsync with the default cached behavior
instead of bypassCache: true. Preserve the existing cancellation, maximumRows
limit, and retention-processing flow.
---
Nitpick comments:
In @.github/workflows/dotnet.yml:
- Line 150: The replacement in the PR-body formatting step should not globally
convert standalone “PR” occurrences. Remove the broad `.replace(/\bPR\b/g,
'Release')` call and preserve only the targeted heading replacements already
handled in the surrounding workflow logic.
In `@Core/Resgrid.Model/Tracking/UnitTrackingCatalog.cs`:
- Around line 25-47: Replace the per-call linear scans in GetProfile and
GetIoMap with case-insensitive dictionaries initialized once within the existing
lazy catalog/document initialization. Normalize lookup keys consistently,
preserve null/whitespace handling, and retrieve profiles and I/O maps by key
using Dictionary<string, ...>(StringComparer.OrdinalIgnoreCase).
In `@Core/Resgrid.Services/UnitTrackingIngressService.cs`:
- Around line 137-196: The validation flow in AcceptHeartbeatAsync duplicates
the device-enabled, unit/tenant, and identifier checks in AcceptAsync. Extract
these checks into a shared private helper such as ValidateBindingAsync,
returning the validated binding context or the appropriate Invalid result, then
update both entry points to use it while preserving their existing status-update
and response behavior.
In `@Docker/docker-compose.yml`:
- Line 163: Remove the trailing spaces from the blank line separating the elk
and mongodb service blocks in the Docker Compose configuration, leaving the
blank line otherwise unchanged.
In `@Providers/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs`:
- Around line 314-319: Replace the hard-coded "jimi-vl103m" comparison in
Gt06ProtocolSession with a shared constant declared alongside
TrackingProtocolKeys, and use that constant in the MapAlarm call so the catalog
profile key has a single source of truth.
- Around line 501-525: Update TryReadTimestamp to validate the decoded month and
day ranges explicitly before constructing DateTime, using bounds checks and
DateTime.DaysInMonth as needed; remove the exception-driven catch path while
preserving false for malformed six-byte timestamps and true with UTC output for
valid values.
In
`@Providers/Resgrid.Providers.Tracking/Protocols/Queclink/QueclinkProtocolSession.cs`:
- Around line 87-669: Add focused unit tests covering the
QueclinkProtocolSession.Parse and BuildResponse flows with known-good and
malformed frames: RESP/BUFF single- and multi-location messages, ACK:GTHBD
heartbeat acknowledgements, oversized frames, and invalid framing or fields.
Assert statuses, consumed data, parsed positions, identifiers, acknowledgement
tokens, and rejection reason codes to exercise the offset and validation logic.
In `@Providers/Resgrid.Providers.Tracking/Protocols/TrackingProtocolContract.cs`:
- Around line 49-84: Update the contract DTO properties in
TrackingSessionContext, ProtocolMessage, ProtocolParseResult, and
TrackingAcceptance from set-only mutation to init-only setters so they cannot be
modified after object initialization. Preserve ProtocolMessage.ProtocolData as
settable because TrackingTransportSessionHandler clears it after enrichment;
leave the existing default initialization for Positions intact.
In `@Workers/Resgrid.TrackerGateway/Dockerfile`:
- Line 3: Update the BUILD_VERSION default argument in the Dockerfile to match
this project's 4.x versioning scheme instead of the stale 3.5.0 value, while
preserving the CI-provided BUILD_VERSION override.
In `@Workers/Resgrid.TrackerGateway/Health/TrackingGatewayMetrics.cs`:
- Around line 92-108: Update the current-connection gauge decrement in
ConnectionCompleted to avoid the racily clamped Math.Max(0, current - 1)
AddOrUpdate behavior. Use an atomic decrement approach consistent with
ConnectionStarted, while preserving the non-negative gauge requirement and
existing outcome increment.
In `@Workers/Resgrid.TrackerGateway/Hosting/TrackingGatewaySettings.cs`:
- Around line 33-44: Update IsEnabled(TrackingSocketTransport) to require the
protocol-level Enabled flag in addition to the transport-specific TcpEnabled or
UdpEnabled setting. Preserve the existing false result for unsupported
transports and ensure every supported transport returns false when Enabled is
false.
In `@Workers/Resgrid.TrackerGateway/Hosting/TrackingListenerPlanBuilder.cs`:
- Around line 97-107: Update the module lookup in the tracking listener
plan-building flow around moduleRegistry.Modules to collect all case-insensitive
ProtocolKey matches instead of using SingleOrDefault. Treat zero matches as the
existing missing-module validation error, exactly one match as the selected
module, and duplicates as an accumulated configuration validation error that
ultimately produces TrackingGatewayConfigurationException.
In `@Workers/Resgrid.TrackerGateway/Listeners/UdpTrackingListener.cs`:
- Around line 244-264: The UDP response send in the listener’s datagram handling
flow can race with StopAsync disposal. Capture the current _listenerSocket
before awaiting SendToAsync and treat ObjectDisposedException from that send as
a cancellation-style shutdown outcome, while preserving normal send behavior and
avoiding the generic “datagram failed” warning.
In `@Workers/Resgrid.TrackerGateway/Sessions/TrackingTransportSessionHandler.cs`:
- Around line 597-631: Update ResolveSourceAsync’s TCP path (useGeneration:
true) to reuse the established state.Source when message.ExternalIdentifier is
absent or matches state.Source.ReportedDeviceIdentifier, skipping
GetEnabledDeviceByProtocolIdentifierAsync for those frames while preserving the
existing identifier-changed and CIDR validation. Keep the current lookup
behavior for initial resolution or changed identifiers, and leave UDP datagram
resolution unchanged.
In `@Workers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs`:
- Around line 109-115: Update the totalDeleted versus maximumRows guard in the
departmentIds loop to emit an appropriate warning log or metric before breaking,
including that the global retention cap was reached and remaining departments
were skipped. Preserve the existing break behavior and ensure the notification
is emitted once when the cap causes the early exit.
🪄 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 Plus
Run ID: e89b5ea7-440b-4a4c-8fc4-326391a5bdaf
⛔ Files ignored due to path filters (44)
Providers/Resgrid.Providers.Tracking/NOTICE.mdis excluded by!**/*.mdProviders/Resgrid.Providers.Tracking/PROVENANCE.mdis excluded by!**/*.mdTests/Resgrid.Tests/Repositories/UnitLocationRetentionRepositoryTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Repositories/UnitLocationRetentionStoreIntegrationTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingCatalogServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingIngressServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Workers/UnitTrackingRetentionLogicTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Gt06/README.mdis excluded by!**/Tests/**,!**/*.mdTests/Resgrid.Tracking.Tests/Data/Gt06/heartbeat.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Gt06/location-jm-vl03-a0.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Gt06/location-standard.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Gt06/login.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Queclink/README.mdis excluded by!**/Tests/**,!**/*.mdTests/Resgrid.Tracking.Tests/Data/Queclink/gteri-buffered.txtis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Queclink/gtfri-live.txtis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Queclink/gtign-live.txtis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Queclink/heartbeat.txtis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Teltonika/README.mdis excluded by!**/Tests/**,!**/*.mdTests/Resgrid.Tracking.Tests/Data/Teltonika/codec8-io-location.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Teltonika/codec8-location.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Teltonika/codec8-udp-location.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Teltonika/codec8e-location.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Data/Teltonika/codec8e-udp-location.hexis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Health/TrackingGatewayHealthCheckTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Hosting/TrackingListenerPlanBuilderTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Listeners/TrackingConnectionAdmissionTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Listeners/TrackingEndpointMaskerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Listeners/TrackingListenerSupervisorTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Listeners/TrackingSocketListenerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/Gt06ProtocolModuleTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/QueclinkProtocolModuleTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/TeltonikaCodec8ProtocolModuleTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/TeltonikaCodec8UdpProtocolSessionTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/TrackingProtocolCatalogValidatorTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/TrackingProtocolContractTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/TrackingProtocolParserFuzzTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Resgrid.Tracking.Tests.csprojis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Sessions/TrackingSessionGenerationRegistryTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Sessions/TrackingTransportSessionHandlerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Sessions/UnitTrackingSourceNetworkPolicyTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Tools/Gt06/Gt06TcpSimulator.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Tools/Queclink/QueclinkTcpSimulator.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Tools/Teltonika/TeltonikaTcpSimulator.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Tools/Teltonika/TeltonikaUdpSimulator.csis excluded by!**/Tests/**
📒 Files selected for processing (73)
.github/workflows/dotnet.ymlCore/Resgrid.Config/UnitTrackingConfig.csCore/Resgrid.Model/Repositories/IUnitLocationRetentionRepository.csCore/Resgrid.Model/Repositories/IUnitLocationsDocRepository.csCore/Resgrid.Model/Repositories/IUnitLocationsMongoRepository.csCore/Resgrid.Model/Resgrid.Model.csprojCore/Resgrid.Model/Services/IUnitTrackingIngressService.csCore/Resgrid.Model/Tracking/Catalog/unit-tracking-catalog.jsonCore/Resgrid.Model/Tracking/UnitTrackingCatalog.csCore/Resgrid.Model/Tracking/UnitTrackingCatalogProfile.csCore/Resgrid.Model/Tracking/UnitTrackingIoMap.csCore/Resgrid.Model/Tracking/UnitTrackingSourceNetworkPolicy.csCore/Resgrid.Services/UnitTrackingCatalogService.csCore/Resgrid.Services/UnitTrackingIngressService.csDocker/docker-compose.ymlDocker/monitoring/grafana/resgrid-tracker-gateway.jsonDocker/monitoring/tracker-gateway-alerts.ymlProviders/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06Crc16.csProviders/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolModule.csProviders/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.csProviders/Resgrid.Providers.Tracking/Protocols/ITrackingProtocolModuleRegistry.csProviders/Resgrid.Providers.Tracking/Protocols/Queclink/QueclinkProtocolModule.csProviders/Resgrid.Providers.Tracking/Protocols/Queclink/QueclinkProtocolSession.csProviders/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaCodec8ProtocolModule.csProviders/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaCodec8ProtocolSession.csProviders/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaCodec8UdpProtocolSession.csProviders/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaCrc16.csProviders/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaIoMapper.csProviders/Resgrid.Providers.Tracking/Protocols/TrackingProtocolCatalogValidator.csProviders/Resgrid.Providers.Tracking/Protocols/TrackingProtocolContract.csProviders/Resgrid.Providers.Tracking/Protocols/TrackingProtocolKeys.csProviders/Resgrid.Providers.Tracking/Protocols/TrackingProtocolModuleRegistry.csProviders/Resgrid.Providers.Tracking/Resgrid.Providers.Tracking.csprojProviders/Resgrid.Providers.Tracking/TrackingProviderModule.csRepositories/Resgrid.Repositories.NoSqlRepository/NoSqlDataModule.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationRetentionRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsMongoRepository.csResgrid.slnWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingNetworkPolicy.csWeb/Resgrid.Web.Services/Controllers/v4/UnitTrackingDevicesController.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.csWeb/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtmlWorkers/Resgrid.TrackerGateway/Bootstrapper.csWorkers/Resgrid.TrackerGateway/DockerfileWorkers/Resgrid.TrackerGateway/Health/TrackingGatewayLivenessHealthCheck.csWorkers/Resgrid.TrackerGateway/Health/TrackingGatewayMetrics.csWorkers/Resgrid.TrackerGateway/Health/TrackingGatewayMetricsWriter.csWorkers/Resgrid.TrackerGateway/Health/TrackingGatewayReadinessHealthCheck.csWorkers/Resgrid.TrackerGateway/Health/TrackingGatewayReadinessState.csWorkers/Resgrid.TrackerGateway/Hosting/TrackingGatewayConfigurationException.csWorkers/Resgrid.TrackerGateway/Hosting/TrackingGatewaySettings.csWorkers/Resgrid.TrackerGateway/Hosting/TrackingListenerDefinition.csWorkers/Resgrid.TrackerGateway/Hosting/TrackingListenerPlan.csWorkers/Resgrid.TrackerGateway/Hosting/TrackingListenerPlanBuilder.csWorkers/Resgrid.TrackerGateway/Listeners/ITrackingListener.csWorkers/Resgrid.TrackerGateway/Listeners/ITrackingListenerFactory.csWorkers/Resgrid.TrackerGateway/Listeners/TcpTrackingListener.csWorkers/Resgrid.TrackerGateway/Listeners/TrackingConnectionAdmission.csWorkers/Resgrid.TrackerGateway/Listeners/TrackingEndpointMasker.csWorkers/Resgrid.TrackerGateway/Listeners/TrackingListenerSupervisor.csWorkers/Resgrid.TrackerGateway/Listeners/TrackingSocketListenerFactory.csWorkers/Resgrid.TrackerGateway/Listeners/UdpTrackingListener.csWorkers/Resgrid.TrackerGateway/Listeners/UnavailableTrackingListenerFactory.csWorkers/Resgrid.TrackerGateway/Program.csWorkers/Resgrid.TrackerGateway/Resgrid.TrackerGateway.csprojWorkers/Resgrid.TrackerGateway/Sessions/ITrackingTransportSessionHandler.csWorkers/Resgrid.TrackerGateway/Sessions/TrackingSessionGenerationRegistry.csWorkers/Resgrid.TrackerGateway/Sessions/TrackingTransportSessionHandler.csWorkers/Resgrid.Workers.Console/Commands/UnitTrackingRetentionCommand.csWorkers/Resgrid.Workers.Console/Program.csWorkers/Resgrid.Workers.Console/Tasks/UnitTrackingRetentionTask.csWorkers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs
| public sealed class UnitLocationRetentionRepository : | ||
| IUnitLocationRetentionRepository | ||
| { | ||
| private readonly Lazy<IUnitLocationsDocRepository> | ||
| _postgresRepository; | ||
| private readonly Lazy<IUnitLocationsMongoRepository> | ||
| _mongoRepository; | ||
|
|
||
| public UnitLocationRetentionRepository( | ||
| Lazy<IUnitLocationsDocRepository> postgresRepository, | ||
| Lazy<IUnitLocationsMongoRepository> mongoRepository) | ||
| { | ||
| _postgresRepository = | ||
| postgresRepository ?? | ||
| throw new ArgumentNullException( | ||
| nameof(postgresRepository)); | ||
| _mongoRepository = | ||
| mongoRepository ?? | ||
| throw new ArgumentNullException( | ||
| nameof(mongoRepository)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Constructor injection instead of Service Locator pattern.
This repository takes Lazy<IUnitLocationsDocRepository>/Lazy<IUnitLocationsMongoRepository> via constructor injection. The repo's coding guideline for **/*.cs explicitly directs resolving dependencies via Bootstrapper.GetKernel().Resolve<T>() in constructors rather than constructor injection. UnitTrackingRetentionLogic (added in this same PR) follows the expected pattern: a parameterless ctor resolving via the service locator, delegating to a secondary testable ctor. Consider mirroring that here for consistency.
♻️ Suggested pattern (mirrors UnitTrackingRetentionLogic)
public UnitLocationRetentionRepository(
+ ) : this(
+ new Lazy<IUnitLocationsDocRepository>(
+ () => Bootstrapper.GetKernel().Resolve<IUnitLocationsDocRepository>()),
+ new Lazy<IUnitLocationsMongoRepository>(
+ () => Bootstrapper.GetKernel().Resolve<IUnitLocationsMongoRepository>()))
+ {
+ }
+
+ public UnitLocationRetentionRepository(
Lazy<IUnitLocationsDocRepository> postgresRepository,
Lazy<IUnitLocationsMongoRepository> mongoRepository)📝 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.
| public sealed class UnitLocationRetentionRepository : | |
| IUnitLocationRetentionRepository | |
| { | |
| private readonly Lazy<IUnitLocationsDocRepository> | |
| _postgresRepository; | |
| private readonly Lazy<IUnitLocationsMongoRepository> | |
| _mongoRepository; | |
| public UnitLocationRetentionRepository( | |
| Lazy<IUnitLocationsDocRepository> postgresRepository, | |
| Lazy<IUnitLocationsMongoRepository> mongoRepository) | |
| { | |
| _postgresRepository = | |
| postgresRepository ?? | |
| throw new ArgumentNullException( | |
| nameof(postgresRepository)); | |
| _mongoRepository = | |
| mongoRepository ?? | |
| throw new ArgumentNullException( | |
| nameof(mongoRepository)); | |
| } | |
| public sealed class UnitLocationRetentionRepository : | |
| IUnitLocationRetentionRepository | |
| { | |
| private readonly Lazy<IUnitLocationsDocRepository> | |
| _postgresRepository; | |
| private readonly Lazy<IUnitLocationsMongoRepository> | |
| _mongoRepository; | |
| public UnitLocationRetentionRepository( | |
| ) : this( | |
| new Lazy<IUnitLocationsDocRepository>( | |
| () => Bootstrapper.GetKernel().Resolve<IUnitLocationsDocRepository>()), | |
| new Lazy<IUnitLocationsMongoRepository>( | |
| () => Bootstrapper.GetKernel().Resolve<IUnitLocationsMongoRepository>())) | |
| { | |
| } | |
| public UnitLocationRetentionRepository( | |
| Lazy<IUnitLocationsDocRepository> postgresRepository, | |
| Lazy<IUnitLocationsMongoRepository> mongoRepository) |
🤖 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.NoSqlRepository/UnitLocationRetentionRepository.cs`
around lines 9 - 29, Update UnitLocationRetentionRepository to use a
parameterless constructor that resolves IUnitLocationsDocRepository and
IUnitLocationsMongoRepository through Bootstrapper.GetKernel().Resolve<T>(),
matching the UnitTrackingRetentionLogic pattern. Retain a secondary constructor
accepting the Lazy dependencies for testability, including the existing null
validation.
Source: Coding guidelines
| try | ||
| { | ||
| var listeners = GetListenerSnapshot(); | ||
| for (var index = listeners.Count - 1; index >= 0; index--) | ||
| { | ||
| await listeners[index].StopAsync(shutdownCancellation.Token); | ||
| _readiness.MarkStopped(listeners[index].Definition); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| await base.StopAsync(shutdownCancellation.Token); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
One failing StopAsync aborts shutdown of the remaining listeners.
The loop is sequential with no per-listener error handling, so an exception from any StopAsync leaves the earlier-indexed listeners bound with live sessions until process exit. Isolate each stop.
🛡️ Proposed fix
for (var index = listeners.Count - 1; index >= 0; index--)
{
- await listeners[index].StopAsync(shutdownCancellation.Token);
- _readiness.MarkStopped(listeners[index].Definition);
+ try
+ {
+ await listeners[index].StopAsync(shutdownCancellation.Token);
+ }
+ catch (Exception ex)
+ {
+ _logger.LogError(
+ ex,
+ "Tracking listener failed to stop cleanly: {Definition}.",
+ listeners[index].Definition);
+ }
+ finally
+ {
+ _readiness.MarkStopped(listeners[index].Definition);
+ }
}🤖 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.TrackerGateway/Listeners/TrackingListenerSupervisor.cs`
around lines 112 - 124, Update the listener shutdown loop in
TrackingListenerSupervisor.StopAsync to isolate each listeners[index].StopAsync
failure so one exception cannot prevent remaining listeners from being stopped.
Handle errors per listener while preserving readiness marking for successfully
stopped listeners, then always execute the existing base.StopAsync call in the
finally block.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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.Model/PushUri.cs`:
- Around line 54-60: Add the "Source" property name to PushUri.IgnoredProperties
while leaving the existing [NotMapped] and [ProtoMember(10)] attributes
unchanged, so generic persistence excludes this transient field.
In `@Core/Resgrid.Model/Services/IIncidentCommandNotificationService.cs`:
- Around line 34-40: Update SendMessageToCommandAsync in
IncidentCommandNotificationService to check cancellationToken before each
recipient send and forward it to every cancellable downstream notification
operation. Preserve the existing recipient fan-out and message behavior while
ensuring cancellation interrupts further sends.
- Around line 34-40: Update SendMessageToCommandAsync so ResolveDisplayNameAsync
runs within the best-effort error boundary, catching and logging resolution
failures before falling back to the sender’s default name; ensure the method
continues processing recipients and does not propagate the exception to
IncidentCommandController.SendMessageToCommand.
In `@Core/Resgrid.Model/Services/IPushService.cs`:
- Line 15: Rename the IPushService.PushICNotification contract to
PushICNotificationAsync, rename the corresponding PushService implementation,
and update the IC routing call in CommunicationService to use the new method
name. Apply these changes at Core/Resgrid.Model/Services/IPushService.cs:15,
Core/Resgrid.Services/PushService.cs:169-192, and
Core/Resgrid.Services/CommunicationService.cs:618-621.
In `@Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs`:
- Around line 31-37: Update the IncidentCommandController constructor to remove
IIncidentCommandNotificationService constructor injection and assign
_incidentCommandNotificationService by resolving it through
Bootstrapper.GetKernel().Resolve<IIncidentCommandNotificationService>().
Preserve the existing incidentCommandService injection and field initialization.
- Around line 275-279: Update IncidentCommandController.cs lines 275-279 to
label the returned count as targeted/attempted recipients rather than delivered
recipients; retain targets.Count unless tracked send outcomes are already
available. Update IncidentCommandModels.cs lines 169-174 XML documentation to
describe the count as targeted or attempted recipients, not confirmed
deliveries.
- Around line 239-242: Update SendMessageToCommand authorization to require the
write-level command policy instead of Command_View, and include the appropriate
incident capability policy used for sending commander notifications when
applicable. Preserve the existing endpoint behavior while preventing read-only
users from invoking it.
In `@Web/Resgrid.Web.Services/Controllers/v4/SecurityController.cs`:
- Around line 111-112: Remove the CreateICUserSubscriber call from
GetCurrentUsersRights so rights reads remain free of Novu network provisioning
and do not create subscribers for unregistered IC users. Move IC subscriber
creation to the IC registration/onboarding flow, or replace it with a separately
queued ensure operation that is idempotent and uses bounded retries.
In `@Web/Resgrid.Web.Services/Models/v4/Device/PushRegistrationInput.cs`:
- Around line 32-36: Validate and canonicalize the Source property in
PushRegistrationInput so only null/empty or the supported “IC” value is
accepted, normalizing valid values to the canonical casing before
DevicesController forwards them into push-routing events. Reject all other
strings at input validation rather than allowing them to serialize or route.
🪄 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 Plus
Run ID: fcd0f80e-26f3-4a8f-9607-73a1f68e3fd4
📒 Files selected for processing (15)
Core/Resgrid.Config/ChatConfig.csCore/Resgrid.Model/Providers/Models/INovuProvider.csCore/Resgrid.Model/PushUri.csCore/Resgrid.Model/Services/ICommunicationService.csCore/Resgrid.Model/Services/IIncidentCommandNotificationService.csCore/Resgrid.Model/Services/IPushService.csCore/Resgrid.Services/CommunicationService.csCore/Resgrid.Services/IncidentCommandNotificationService.csCore/Resgrid.Services/PushService.csProviders/Resgrid.Providers.Messaging/NovuProvider.csWeb/Resgrid.Web.Services/Controllers/v4/DevicesController.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Controllers/v4/SecurityController.csWeb/Resgrid.Web.Services/Models/v4/Device/PushRegistrationInput.csWeb/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.cs
| /// <summary> | ||
| /// Source app of the registration (e.g. "IC" for the Incident Command app). Routes the Novu push | ||
| /// credential update to the app-specific subscriber; null/empty means the default Responder app. | ||
| /// </summary> | ||
| [NotMapped] | ||
| [ProtoMember(10)] | ||
| public string Source { get; set; } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 '\bIgnoredProperties\b|\bPushUri\b' --glob '*.cs'Repository: Resgrid/Core
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files matching PushUri.cs/IgnoredProperties:\n'
git ls-files | rg '(^|/)PushUri\.cs$|IgnoredProperties|Repository|Persistence|Migration|EntityConfiguration|DbContext|DbModelBuilder|MapIgnore|Ignore|NotMapped' || true
printf '\nAll files with Source in PushUri context:\n'
rg -n 'class PushUri|Source\s*\{|IgnoredProperties|NotMapped|ProtoMember|Uuid|DepartmentId' --glob '*.cs' || trueRepository: Resgrid/Core
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'PushUri.cs:\n'
cat -n Core/Resgrid.Model/PushUri.cs
printf '\nPushUriRepository.cs:\n'
cat -n Repositories/Resgrid.Repositories.DataRepository/PushUriRepository.cs
printf '\nPushUri repository interface:\n'
cat -n Core/Resgrid.Model/Repositories/IPushUriRepository.cs
printf '\nFocused IgnoredProperties matches:\n'
rg -n '\bIgnoredProperties\b' --glob '*.cs' || true
printf '\nRepository reflection helper around property handling:\n'
cat -n Repositories/Resgrid.Repositories.DataRepository/Extensions/ReflectionExtensions.csRepository: Resgrid/Core
Length of output: 4319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'All IgnoredProperties matches excluding result log:\n'
rg -n '\bIgnoredProperties\b' --glob '*.cs' || true
printf '\nRelevant generic repository/mapping code searches:\n'
rg -n '\bNotMapped\b|\bDatabaseGenerated\b|\bMapIgnore\b|\bIgnore\b|\bDapper\b|\bQuery\*' --glob '*.cs' \
| rg '(Repositories/Resgrid\.Repositories\.DataRepository/Core\.Resgrid\.Model/.*/.*\.cs$|Repositories/Resgrid\.Repositories\.DataRepository.*\.cs$)' \
| head -n 200 || true
printf '\nRepository base implementation files:\n'
git ls-files 'Repositories/Resgrid.Repositories.DataRepository/*.cs' 'Core/Resgrid.Model/*.cs' \
| rg 'Base|Generic|Repository|Dapper|Mapper|Reflection' \
| head -n 100Repository: Resgrid/Core
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo DataRepository file count:\n'
git ls-files 'Repositories/Resgrid.Repositories.DataRepository/*.cs' | wc -l
printf '\nGeneric repository/builder symbols:\n'
rg -n 'class .*Repository|interface .*Repository|Build|Ignore|NotMapped|Insert|Update|InsertDynamic|UpdateDynamic|Dapper|QueryMulti' \
--glob '*.cs' Repositories/Resgrid.Repositories.DataRepository/Core/Resgrid.Model \
| head -n 300 || true
printf '\nRepository-specific symbol search using tracked files only:\n'
for f in $(git ls-files 'Repositories/Resgrid.Repositories.DataRepository/*.cs' 'Repositories/Resgrid.Repositories.DataRepository/**/*.cs' | rg 'Base|Generic|Mapping|Reflection|Query|Insert|Update'); do
echo "--- $f"
rg -n 'IgnoredProperties|Mapping|NotMapped|Build|Ignore|Dapper|QueryMulti|InsertDynamic|UpdateDynamic|Parameters\(' "$f" | head -n 50 || true
done | head -n 300 || trueRepository: Resgrid/Core
Length of output: 29062
Add Source to PushUri.IgnoredProperties.
Source is [NotMapped] and transient, but the generic insert/update helpers use IgnoredProperties to drop IEntity transient fields. Add "Source" to keep generic persistence in sync while retaining its protobuf field.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Model/PushUri.cs` around lines 54 - 60, Add the "Source"
property name to PushUri.IgnoredProperties while leaving the existing
[NotMapped] and [ProtoMember(10)] attributes unchanged, so generic persistence
excludes this transient field.
|
|
||
| /// <summary> | ||
| /// Sends a free-form message from a department member directly to specific command users (e.g. the | ||
| /// incident commander and deputies). The sender's display name is prepended to the body so recipients | ||
| /// know who it came from on every channel. | ||
| /// </summary> | ||
| Task SendMessageToCommandAsync(int departmentId, string senderUserId, string title, string body, IEnumerable<string> recipientUserIds, CancellationToken cancellationToken = default(CancellationToken)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Honor the new cancellation token.
The implementation in Core/Resgrid.Services/IncidentCommandNotificationService.cs Lines 117-137 never checks or forwards cancellationToken, so callers cannot cancel a potentially long recipient fan-out. Check cancellation before each send and pass the token to cancellable downstream operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Model/Services/IIncidentCommandNotificationService.cs` around
lines 34 - 40, Update SendMessageToCommandAsync in
IncidentCommandNotificationService to check cancellationToken before each
recipient send and forward it to every cancellable downstream notification
operation. Preserve the existing recipient fan-out and message behavior while
ensuring cancellation interrupts further sends.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep sender resolution inside the best-effort error boundary.
In Core/Resgrid.Services/IncidentCommandNotificationService.cs Lines 117-137, ResolveDisplayNameAsync runs before the per-recipient try/catch. If it throws, SendMessageToCommandAsync propagates the exception and IncidentCommandController.SendMessageToCommand can fail, contradicting this interface’s best-effort contract. Catch/log that failure and use the fallback sender name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Model/Services/IIncidentCommandNotificationService.cs` around
lines 34 - 40, Update SendMessageToCommandAsync so ResolveDisplayNameAsync runs
within the best-effort error boundary, catching and logging resolution failures
before falling back to the sender’s default name; ensure the method continues
processing recipients and does not propagate the exception to
IncidentCommandController.SendMessageToCommand.
| Task<bool> UnRegister(PushUri pushUri); | ||
| void UnRegisterNotificationOnly(PushUri pushUri); | ||
| Task<bool> PushNotification(StandardPushMessage message, string userId, UserProfile profile = null); | ||
| Task<bool> PushICNotification(StandardPushMessage message, string userId, UserProfile profile = null); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required Async suffix for the new asynchronous push API.
Core/Resgrid.Model/Services/IPushService.cs#L15-L15: renamePushICNotificationtoPushICNotificationAsync.Core/Resgrid.Services/PushService.cs#L169-L192: rename the implementation accordingly.Core/Resgrid.Services/CommunicationService.cs#L618-L621: update the IC routing call to the renamed method.
As per coding guidelines, service methods must be async and follow the {Verb}{Entity}{Filter}Async naming pattern.
📍 Affects 3 files
Core/Resgrid.Model/Services/IPushService.cs#L15-L15(this comment)Core/Resgrid.Services/PushService.cs#L169-L192Core/Resgrid.Services/CommunicationService.cs#L618-L621
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Model/Services/IPushService.cs` at line 15, Rename the
IPushService.PushICNotification contract to PushICNotificationAsync, rename the
corresponding PushService implementation, and update the IC routing call in
CommunicationService to use the new method name. Apply these changes at
Core/Resgrid.Model/Services/IPushService.cs:15,
Core/Resgrid.Services/PushService.cs:169-192, and
Core/Resgrid.Services/CommunicationService.cs:618-621.
Source: Coding guidelines
| private readonly IIncidentCommandNotificationService _incidentCommandNotificationService; | ||
|
|
||
| public IncidentCommandController(IIncidentCommandService incidentCommandService) | ||
| public IncidentCommandController(IIncidentCommandService incidentCommandService, | ||
| IIncidentCommandNotificationService incidentCommandNotificationService) | ||
| { | ||
| _incidentCommandService = incidentCommandService; | ||
| _incidentCommandNotificationService = incidentCommandNotificationService; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve the new dependency through the configured service locator.
Do not add IIncidentCommandNotificationService as constructor injection; resolve it explicitly in the constructor with Bootstrapper.GetKernel().Resolve<IIncidentCommandNotificationService>().
As per coding guidelines, “Use Service Locator pattern via Bootstrapper.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 31 - 37, Update the IncidentCommandController constructor to remove
IIncidentCommandNotificationService constructor injection and assign
_incidentCommandNotificationService by resolving it through
Bootstrapper.GetKernel().Resolve<IIncidentCommandNotificationService>().
Preserve the existing incidentCommandService injection and field initialization.
Source: Coding guidelines
| result.Data = targets.Count; | ||
| result.PageSize = 1; | ||
| result.Status = ResponseHelper.Success; | ||
| ResponseHelper.PopulateV4ResponseData(result); | ||
| return result; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report these as targeted recipients, not delivered recipients.
Fan-out failures are intentionally swallowed in Core/Resgrid.Services/IncidentCommandNotificationService.cs, but the endpoint returns targets.Count; this is an attempted-recipient count, not delivery confirmation.
Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs#L275-L279: retain the count only as the number of targeted recipients, or return tracked send outcomes.Web/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.cs#L169-L174: change the XML documentation to describe targeted/attempted recipients.
📍 Affects 2 files
Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs#L275-L279(this comment)Web/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.cs#L169-L174
🤖 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 275 - 279, Update IncidentCommandController.cs lines 275-279 to label the
returned count as targeted/attempted recipients rather than delivered
recipients; retain targets.Count unless tracked send outcomes are already
available. Update IncidentCommandModels.cs lines 169-174 XML documentation to
describe the count as targeted or attempted recipients, not confirmed
deliveries.
|
|
||
| /// <summary> | ||
| /// Source app of the registration (e.g. "IC" for the Incident Command app). Null/empty means the default Responder app. | ||
| /// </summary> | ||
| public string Source { get; set; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate and canonicalize Source.
This field accepts any string, while the documented contract only supports "IC" or null/empty. Since DevicesController forwards it into push-routing events, reject unsupported values and normalize casing before serialization to prevent ambiguous routing behavior.
🤖 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/Models/v4/Device/PushRegistrationInput.cs` around
lines 32 - 36, Validate and canonicalize the Source property in
PushRegistrationInput so only null/empty or the supported “IC” value is
accepted, normalizing valid values to the canonical casing before
DevicesController forwards them into push-routing events. Reject all other
strings at input validation rather than allowing them to serialize or route.
| receivedOn, | ||
| "The tracking binding is not enabled."); | ||
|
|
||
| var device = source.Device; |
There was a problem hiding this comment.
Null pointer dereference risk exists where source.Device is accessed without a null check, despite line 148 using source?.Device. Add an explicit null check or consistently use the null-conditional operator before accessing .Device.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingIngressService.cs:
Line 153:
Null pointer dereference risk exists where `source.Device` is accessed without a null check, despite line 148 using `source?.Device`. Add an explicit null check or consistently use the null-conditional operator before accessing `.Device`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| tracker-gateway: | ||
| profiles: ["tracking"] | ||
| image: "resgridllc/resgridtrackergateway:0.6.70" |
There was a problem hiding this comment.
Mutable image tag violates the team rule requiring immutable digests for production workloads. Pin the container image using the repo@sha256:... digest format instead of the mutable tag.
Kody rule violation: Pin container images by digest in Helm/K8s
Prompt for LLM
File Docker/docker-compose.yml:
Line 86:
Mutable image tag violates the team rule requiring immutable digests for production workloads. Pin the container image using the `repo@sha256:...` digest format instead of the mutable tag.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| if (string.Equals( | ||
| transport, | ||
| "Tcp", |
There was a problem hiding this comment.
Magic string literal "Tcp" defeats type safety and is error-prone when duplicated across the codebase. Define a constant, such as private const string TcpTransport = "Tcp", or parse the string into the existing TrackingSocketTransport enum before comparison.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Providers/Resgrid.Providers.Tracking/Protocols/TrackingProtocolCatalogValidator.cs:
Line 52:
Magic string literal `"Tcp"` defeats type safety and is error-prone when duplicated across the codebase. Define a constant, such as `private const string TcpTransport = "Tcp"`, or parse the string into the existing `TrackingSocketTransport` enum before comparison.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| private static RetentionFixture CreateFixture() | ||
| { | ||
| var eventPrefix = | ||
| "retention-integration-" + |
There was a problem hiding this comment.
String concatenation using the + operator is less readable and error-prone. Replace the concatenation with C# string interpolation, such as $"retention-integration-{Guid.NewGuid():N}".
Kody rule violation: Use Template Literals Instead of String Concatenation
Prompt for LLM
File Tests/Resgrid.Tests/Repositories/UnitLocationRetentionStoreIntegrationTests.cs:
Line 173:
String concatenation using the `+` operator is less readable and error-prone. Replace the concatenation with C# string interpolation, such as `$"retention-integration-{Guid.NewGuid():N}"`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| using var client = new UdpClient( | ||
| new IPEndPoint(IPAddress.Loopback, 0)); | ||
| return ((IPEndPoint)client.Client.LocalEndPoint).Port; |
There was a problem hiding this comment.
Direct type cast risks an InvalidCastException during endpoint resolution. Use the as operator or pattern matching, such as if (client.Client.LocalEndPoint is IPEndPoint ep), for safe casting.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Tests/Resgrid.Tracking.Tests/Listeners/TrackingSocketListenerTests.cs:
Line 278:
Direct type cast risks an `InvalidCastException` during endpoint resolution. Use the `as` operator or pattern matching, such as `if (client.Client.LocalEndPoint is IPEndPoint ep)`, for safe casting.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| "Gt06", | ||
| fileName); | ||
| return Convert.FromHexString( | ||
| File.ReadAllText(path).Trim()); |
There was a problem hiding this comment.
Unguarded external I/O call File.ReadAllText produces opaque test failures when throwing IOException, FileNotFoundException, or UnauthorizedAccessException. Wrap the call in a try/catch block to provide fixture path context in the rethrown InvalidOperationException.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Tests/Resgrid.Tracking.Tests/Protocols/Gt06ProtocolModuleTests.cs:
Line 391:
Unguarded external I/O call `File.ReadAllText` produces opaque test failures when throwing `IOException`, `FileNotFoundException`, or `UnauthorizedAccessException`. Wrap the call in a try/catch block to provide fixture path context in the rethrown `InvalidOperationException`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| private static byte[] TeltonikaFixture( | ||
| string fileName) | ||
| { | ||
| var path = Path.Combine( |
There was a problem hiding this comment.
Code duplication exists across fixture methods sharing identical Path.Combine and file conversion sequences. Extract the logic into a shared utility function like private static byte[] LoadFixture(string protocolFolder, string fileName, Func<string, byte[]> converter).
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Tests/Resgrid.Tracking.Tests/Sessions/TrackingTransportSessionHandlerTests.cs:
Line 1232:
Code duplication exists across fixture methods sharing identical `Path.Combine` and file conversion sequences. Extract the logic into a shared utility function like `private static byte[] LoadFixture(string protocolFolder, string fileName, Func<string, byte[]> converter)`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| "gauge"); | ||
| AppendSamples( | ||
| builder, | ||
| "resgrid_tracking_connections_current", |
There was a problem hiding this comment.
Duplicated hardcoded metric name "resgrid_tracking_connections_current" in AppendMetricHeader (line 224) and AppendSamples risks silent inconsistencies in Prometheus output if a typo occurs. Define private const string fields for metric names and reference them in both calls.
Kody rule violation: Centralize string constants
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Health/TrackingGatewayMetrics.cs:
Line 229:
Duplicated hardcoded metric name `"resgrid_tracking_connections_current"` in `AppendMetricHeader` (line 224) and `AppendSamples` risks silent inconsistencies in Prometheus output if a typo occurs. Define `private const string` fields for metric names and reference them in both calls.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
|
|
||
| public double[] Buckets { get; } | ||
| public long[] BucketCounts { get; } |
There was a problem hiding this comment.
Mutable array property BucketCounts allows callers to corrupt the intended immutable point-in-time histogram snapshot. Return IReadOnlyList<long> instead of long[] or wrap the array in a ReadOnlyCollection.
Kody rule violation: Use IReadOnlyList for immutable collections
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Health/TrackingGatewayMetrics.cs:
Line 689:
Mutable array property `BucketCounts` allows callers to corrupt the intended immutable point-in-time histogram snapshot. Return `IReadOnlyList<long>` instead of `long[]` or wrap the array in a `ReadOnlyCollection`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (port >= 1 && port <= 65535) | ||
| return true; |
There was a problem hiding this comment.
Magic number literals 1 and 65535 reduce readability and risk typos when validating TCP/UDP port ranges, notably where line 193 repeats them. Declare private const int MinimumPortNumber = 1; and private const int MaximumPortNumber = 65535; and reference them in both the condition and error message.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Hosting/TrackingListenerPlanBuilder.cs:
Line 190 to 191:
Magic number literals `1` and `65535` reduce readability and risk typos when validating TCP/UDP port ranges, notably where line 193 repeats them. Declare `private const int MinimumPortNumber = 1;` and `private const int MaximumPortNumber = 65535;` and reference them in both the condition and error message.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost("SendMessageToCommand")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Command_View)] | ||
| public async Task<ActionResult<ICModels.SendMessageToCommandResult>> SendMessageToCommand([FromBody] ICModels.SendMessageToCommandInput input) |
| var listeners = GetListenerSnapshot(); | ||
| for (var index = listeners.Count - 1; index >= 0; index--) | ||
| { | ||
| await listeners[index].StopAsync(shutdownCancellation.Token); |
There was a problem hiding this comment.
Unguarded awaited StopAsync call propagates unhandled exceptions, preventing remaining listeners from stopping. Wrap each listener stop in its own try/catch block to handle failures individually.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Listeners/TrackingListenerSupervisor.cs:
Line 117:
Unguarded awaited `StopAsync` call propagates unhandled exceptions, preventing remaining listeners from stopping. Wrap each listener stop in its own try/catch block to handle failures individually.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (Exception ex) | ||
| { | ||
| _readiness.MarkFailed(); | ||
| _logger.LogError(ex, "Tracker gateway listener supervisor failed."); |
There was a problem hiding this comment.
Unstructured error log message prevents effective diagnostics by omitting operation names and relevant identifiers. Include structured properties such as {Operation} and {ListenerCount} in the _logger.LogError call.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Listeners/TrackingListenerSupervisor.cs:
Line 92:
Unstructured error log message prevents effective diagnostics by omitting operation names and relevant identifiers. Include structured properties such as `{Operation}` and `{ListenerCount}` in the `_logger.LogError` call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (ObjectDisposedException) | ||
| { | ||
| } |
There was a problem hiding this comment.
Silently swallowed ObjectDisposedException violates error handling rules and obscures expected race conditions when cancelling a disposed CancellationTokenSource. Add a debug-level log statement or an explicit comment documenting the suppression intent.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Sessions/TrackingSessionGenerationRegistry.cs:
Line 61 to 63:
Silently swallowed `ObjectDisposedException` violates error handling rules and obscures expected race conditions when cancelling a disposed `CancellationTokenSource`. Add a debug-level log statement or an explicit comment documenting the suppression intent.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public Dictionary<string, string> Metadata | ||
| { get; set; } |
There was a problem hiding this comment.
Uninitialized reference type property Metadata causes a NullReferenceException when callers iterate or read it. Initialize the Dictionary<string, string> inline or assign it in the constructor to enforce sensible defaults.
Kody rule violation: Initialize properties with default values
Prompt for LLM
File Workers/Resgrid.Workers.Console/Commands/UnitTrackingRetentionCommand.cs:
Line 17 to 18:
Uninitialized reference type property `Metadata` causes a `NullReferenceException` when callers iterate or read it. Initialize the `Dictionary<string, string>` inline or assign it in the constructor to enforce sensible defaults.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| new UnitTrackingRetentionLogic(); | ||
| var result = await logic.Process( | ||
| cancellationToken); | ||
| if (!result.Item1) |
There was a problem hiding this comment.
Unnamed tuple members Item1 and Item2 lack semantic meaning and violate structuring rules. Define a named result class, such as ProcessResult { bool Success; string Message; }, or use named tuples to access result.Success and result.Message.
Kody rule violation: Prefer Named Classes Over Tuples
Prompt for LLM
File Workers/Resgrid.Workers.Console/Tasks/UnitTrackingRetentionTask.cs:
Line 41:
Unnamed tuple members `Item1` and `Item2` lack semantic meaning and violate structuring rules. Define a named result class, such as `ProcessResult { bool Success; string Message; }`, or use named tuples to access `result.Success` and `result.Message`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var departmentIds = departments? | ||
| .Where( | ||
| department => | ||
| department != null && | ||
| department.DepartmentId > 0) | ||
| .Select( | ||
| department => | ||
| department.DepartmentId) | ||
| .Distinct() | ||
| .OrderBy( | ||
| departmentId => | ||
| departmentId) | ||
| .ToList() ?? |
There was a problem hiding this comment.
Excessive LINQ method chaining reduces readability and debuggability. Break the sequence into intermediate variables using Where, Select, Distinct, OrderBy, and ToList operations.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs:
Line 92 to 104:
Excessive LINQ method chaining reduces readability and debuggability. Break the sequence into intermediate variables using `Where`, `Select`, `Distinct`, `OrderBy`, and `ToList` operations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _departmentSettingsService | ||
| .GetHardwareTrackingLocationRetentionDaysAsync( | ||
| departmentId, | ||
| bypassCache: true) | ||
| .WaitAsync(cancellationToken); |
There was a problem hiding this comment.
N+1 query pattern occurs inside the loop on line 109, triggering a separate GetHardwareTrackingLocationRetentionDaysAsync lookup per department and degrading performance. Batch-fetch all department retention-day settings in a single query or via Task.WhenAll before processing deletions.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs:
Line 117 to 121:
N+1 query pattern occurs inside the loop on line 109, triggering a separate `GetHardwareTrackingLocationRetentionDaysAsync` lookup per department and degrading performance. Batch-fetch all department retention-day settings in a single query or via `Task.WhenAll` before processing deletions.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ❌ |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Workers/Resgrid.Workers.Console/Tasks/UnitTrackingRetentionTask.cs (1)
15-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required static logging API.
Line 45 logs through injected
ILogger. Remove_logger/constructor injection and useResgrid.Framework.Logging.LogInfo(result.Item2)instead. As per coding guidelines, “UseResgrid.Framework.Loggingstatic methods … for all logging throughout the codebase” and “useService Locator… rather than constructor injection.”Also applies to: 45-47
🤖 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/UnitTrackingRetentionTask.cs` around lines 15 - 20, Update UnitTrackingRetentionTask to remove the injected ILogger field and constructor parameter, then replace the logging call with Resgrid.Framework.Logging.LogInfo(result.Item2). Keep the task’s existing behavior and use the static logging API rather than constructor-based logging.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.
Nitpick comments:
In `@Workers/Resgrid.Workers.Console/Tasks/UnitTrackingRetentionTask.cs`:
- Around line 15-20: Update UnitTrackingRetentionTask to remove the injected
ILogger field and constructor parameter, then replace the logging call with
Resgrid.Framework.Logging.LogInfo(result.Item2). Keep the task’s existing
behavior and use the static logging API rather than constructor-based logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85b82e43-8080-48d9-ab7c-10da4850595f
⛔ Files ignored due to path filters (3)
Tests/Resgrid.Tests/Workers/UnitTrackingRetentionLogicTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/Gt06ProtocolModuleTests.csis excluded by!**/Tests/**Tests/Resgrid.Tracking.Tests/Protocols/TeltonikaCodec8ProtocolModuleTests.csis excluded by!**/Tests/**
📒 Files selected for processing (18)
Core/Resgrid.Services/IncidentCommandNotificationService.csCore/Resgrid.Services/PushService.csDocker/docker-compose.ymlProviders/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.csProviders/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaCodec8ProtocolSession.csProviders/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaIoMapper.csWeb/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.csWeb/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWorkers/Resgrid.TrackerGateway/DockerfileWorkers/Resgrid.TrackerGateway/Health/TrackingGatewayMetrics.csWorkers/Resgrid.TrackerGateway/Listeners/TcpTrackingListener.csWorkers/Resgrid.TrackerGateway/Listeners/TrackingListenerSupervisor.csWorkers/Resgrid.TrackerGateway/Listeners/UdpTrackingListener.csWorkers/Resgrid.TrackerGateway/Sessions/TrackingSessionGenerationRegistry.csWorkers/Resgrid.TrackerGateway/localxpose-entrypoint.shWorkers/Resgrid.Workers.Console/Tasks/UnitTrackingRetentionTask.csWorkers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs
🚧 Files skipped from review as they are similar to previous changes (14)
- Web/Resgrid.Web.Services/Models/v4/IncidentCommand/IncidentCommandModels.cs
- Core/Resgrid.Services/PushService.cs
- Workers/Resgrid.TrackerGateway/Health/TrackingGatewayMetrics.cs
- Providers/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaIoMapper.cs
- Workers/Resgrid.TrackerGateway/Listeners/TrackingListenerSupervisor.cs
- Docker/docker-compose.yml
- Core/Resgrid.Services/IncidentCommandNotificationService.cs
- Workers/Resgrid.TrackerGateway/Sessions/TrackingSessionGenerationRegistry.cs
- Workers/Resgrid.Workers.Framework/Logic/UnitTrackingRetentionLogic.cs
- Web/Resgrid.Web.Services/Controllers/v4/IncidentCommandController.cs
- Workers/Resgrid.TrackerGateway/Listeners/UdpTrackingListener.cs
- Workers/Resgrid.TrackerGateway/Listeners/TcpTrackingListener.cs
- Providers/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs
- Providers/Resgrid.Providers.Tracking/Protocols/Teltonika/TeltonikaCodec8ProtocolSession.cs
| } | ||
| catch (Exception ex) | ||
| { | ||
| Resgrid.Framework.Logging.LogException(ex); |
There was a problem hiding this comment.
Missing operation context. The catch block logs only the raw exception, violating Rule [3] which requires structured fields like pushUri.UserId and pushUri.DepartmentId for production traceability. Pass these operation details alongside the exception to Resgrid.Framework.Logging.LogException.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Services/PushService.cs:
Line 78:
Missing operation context. The catch block logs only the raw exception, violating Rule [3] which requires structured fields like `pushUri.UserId` and `pushUri.DepartmentId` for production traceability. Pass these operation details alongside the exception to `Resgrid.Framework.Logging.LogException`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| var levelEncoded = string.Equals( | ||
| profile.DecoderVariant, | ||
| "gt06-vl103-bounded", |
There was a problem hiding this comment.
Error-prone string literal. Scattered raw strings like "gt06-vl103-bounded" lack a single source of truth and risk silent behavior changes from typos. Define a named constant such as private const string VariantVl103Bounded = "gt06-vl103-bounded"; in a centralized constants class.
Kody rule violation: Centralize string constants
Prompt for LLM
File Providers/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs:
Line 338:
Error-prone string literal. Scattered raw strings like `"gt06-vl103-bounded"` lack a single source of truth and risk silent behavior changes from typos. Define a named constant such as `private const string VariantVl103Bounded = "gt06-vl103-bounded";` in a centralized constants class.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| var levelEncoded = string.Equals( | ||
| profile.DecoderVariant, | ||
| "gt06-vl103-bounded", |
There was a problem hiding this comment.
Implicit enumerated values. The string literal "gt06-vl103-bounded" represents a finite set of variants but risks typos without compiler validation. Replace it with an enum such as enum Gt06DecoderVariant { Vl103Bounded, JmVl03A0Bounded, ... } to explicitly define valid memberships.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Providers/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs:
Line 338:
Implicit enumerated values. The string literal `"gt06-vl103-bounded"` represents a finite set of variants but risks typos without compiler validation. Replace it with an enum such as `enum Gt06DecoderVariant { Vl103Bounded, JmVl03A0Bounded, ... }` to explicitly define valid memberships.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| string.Equals( | ||
| profile.DecoderVariant, | ||
| "gt06-jm-vl03-a0-bounded", | ||
| StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Duplicated structural pattern. The string.Equals(profile.DecoderVariant, ..., StringComparison.OrdinalIgnoreCase) expression repeats five times, requiring multiple call site updates when modifying variants. Extract a helper method like private static bool IsVariant(string variant, params string[] known) using a case-insensitive HashSet to declare variant sets once.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Providers/Resgrid.Providers.Tracking/Protocols/Gt06/Gt06ProtocolSession.cs:
Line 340 to 343:
Duplicated structural pattern. The `string.Equals(profile.DecoderVariant, ..., StringComparison.OrdinalIgnoreCase)` expression repeats five times, requiring multiple call site updates when modifying variants. Extract a helper method like `private static bool IsVariant(string variant, params string[] known)` using a case-insensitive `HashSet` to declare variant sets once.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| try | ||
| { | ||
| socket.IOControl( | ||
| -1744830452, |
There was a problem hiding this comment.
Opaque numeric literal. The raw value -1744830452 passed to IOControl represents the Windows SIO_UDP_CONNRESET control code but obscures maintenance intent. Extract a named constant such as private const int SIO_UDP_CONNRESET = -1744830452; to clarify its usage.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Listeners/UdpTrackingListener.cs:
Line 253:
Opaque numeric literal. The raw value `-1744830452` passed to `IOControl` represents the Windows `SIO_UDP_CONNRESET` control code but obscures maintenance intent. Extract a named constant such as `private const int SIO_UDP_CONNRESET = -1744830452;` to clarify its usage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Expected race: the displaced session's CTS was already disposed by its own | ||
| // teardown after it was replaced in the dictionary; nothing left to cancel. |
There was a problem hiding this comment.
Swallowed exception. The ObjectDisposedException catch block lacks logging or explicit handling, violating Rule [30] which mandates recording caught exceptions with context. Add a structured log statement such as _logger.LogDebug("CTS already disposed during Activate displacement; no cancellation needed", new { sessionId }); for diagnostics.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Workers/Resgrid.TrackerGateway/Sessions/TrackingSessionGenerationRegistry.cs:
Line 63 to 64:
Swallowed exception. The `ObjectDisposedException` catch block lacks logging or explicit handling, violating Rule [30] which mandates recording caught exceptions with context. Add a structured log statement such as `_logger.LogDebug("CTS already disposed during Activate displacement; no cancellation needed", new { sessionId });` for diagnostics.
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
Description
This PR introduces native TCP/UDP hardware GPS tracker gateway support to Resgrid, enabling direct ingestion of position data from physical tracking devices over socket-based protocols.
New Tracker Gateway Service
A new standalone worker service (
Resgrid.TrackerGateway) that:Hardware Tracker Protocol Decoders
Three bounded protocol modules adapted from public documentation and pinned Traccar reference decoders:
+RESP/+BUFFposition, status, ignition, and alarm reports plus heartbeat acknowledgementAll protocol profiles remain in
Candidatecertification status with no certified transports.Unit Tracking Catalog
Migrated device profiles from hardcoded C# to a validated embedded JSON catalog, adding 21 profiles across Teltonika (Wave 1: FMC920, FMM920, FMC130, FMM130, FMC003), Queclink (GV57MG, GV350MG, GV500MA), and Jimi (VL103M, JM-VL03), plus additional Wave 2 and WP11 family placeholders. Profiles now include decoder variant, supported/certified transports, protocol document version, and optional I/O map references.
Location Data Retention
Added a configurable retention worker that deletes expired hardware tracker location records on a daily schedule, honoring per-department retention day settings. Implemented for both PostgreSQL and MongoDB document stores with batched, bounded deletion.
Other Changes
AcceptHeartbeatAsync) to the tracking ingress service for tracker keep-alive messagesSummary by CodeRabbit
New Features
Bug Fixes