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.
📝 WalkthroughWalkthroughThis PR adds hardware unit tracking across models, persistence, services, RabbitMQ, HTTP APIs, security middleware, administration UI, and queued location processing. It also adds idempotent location writes, capability-path redaction, credential lifecycle management, retry/dead-letter handling, and broader authorization checks. ChangesUnit tracking platform
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.14.0)Web/Resgrid.Web/wwwroot/js/ng/react-elements.cssConfigurationError: Could not find "stylelint-config-sass-guidelines". Do you need to install the package or use the "configBasedir" option? Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingDeviceData>> CreateTracker( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingCredentialProvisionData>> CreateCredential( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingCredentialProvisionData>> RotateCredential( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingCredentialData>> RevokeCredential( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingDeviceData>> DisableTracker( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingDeviceData>> RebindTracker( |
| [ProducesResponseType(typeof(UnitTrackingIngressErrorResponse), StatusCodes.Status422UnprocessableEntity)] | ||
| [ProducesResponseType(StatusCodes.Status429TooManyRequests)] | ||
| [ProducesResponseType(StatusCodes.Status503ServiceUnavailable)] | ||
| public async Task<IActionResult> PostPositions(string unitTrackingDeviceId) |
| [ProducesResponseType(typeof(UnitTrackingIngressErrorResponse), StatusCodes.Status422UnprocessableEntity)] | ||
| [ProducesResponseType(StatusCodes.Status429TooManyRequests)] | ||
| [ProducesResponseType(StatusCodes.Status503ServiceUnavailable)] | ||
| public async Task<IActionResult> PostCapability(string capabilityToken) |
| !ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return NotFound(); | ||
|
|
||
| if (model == null || string.IsNullOrWhiteSpace(model.UnitTrackingDeviceId)) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Services/DepartmentSettingsService.cs (1)
48-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCache invalidated before the write lands — reintroduces stale-cache window.
InvalidateSettingCacheAsyncruns beforeSaveOrUpdateAsyncpersists the change (Line 51 vs. Lines 60/65). A reader hitting the cache miss in that window re-populates it with the pre-update value via the fallback function, and since these settings useLongCacheLength(14 days), the stale value can survive for up to 14 days after the update. Invalidate after the write succeeds instead.🛠️ Proposed fix: invalidate after the write succeeds
public async Task<DepartmentSetting> SaveOrUpdateSettingAsync(int departmentId, string setting, DepartmentSettingTypes type, CancellationToken cancellationToken = default(CancellationToken)) { var savedSetting = await GetSettingByDepartmentIdType(departmentId, type); - await InvalidateSettingCacheAsync(departmentId, type); + DepartmentSetting result; if (savedSetting == null) { DepartmentSetting newSetting = new DepartmentSetting(); newSetting.DepartmentId = departmentId; newSetting.Setting = setting; newSetting.SettingType = (int)type; - return await _departmentSettingsRepository.SaveOrUpdateAsync(newSetting, cancellationToken); + result = await _departmentSettingsRepository.SaveOrUpdateAsync(newSetting, cancellationToken); } else { savedSetting.Setting = setting; - return await _departmentSettingsRepository.SaveOrUpdateAsync(savedSetting, cancellationToken); + result = await _departmentSettingsRepository.SaveOrUpdateAsync(savedSetting, cancellationToken); } - return null; + await InvalidateSettingCacheAsync(departmentId, type); + return result; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/DepartmentSettingsService.cs` around lines 48 - 69, Update SaveOrUpdateSettingAsync so InvalidateSettingCacheAsync runs only after SaveOrUpdateAsync completes successfully for both the new-setting and existing-setting branches. Preserve the saved result, invalidate using departmentId and type after persistence, then return the result; remove the current pre-write invalidation.
🧹 Nitpick comments (21)
Web/Resgrid.Web/Areas/User/Models/UnitTracking/UnitTrackingViewModels.cs (1)
79-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
[EnumDataType]over hardcoded[Range(1,4)]for enum-backed field.Hardcoding the valid range ties validation to the current
UnitTrackingAuthModemember count/numbering; it will silently misvalidate if the enum changes.♻️ Suggested fix
- [Range(1, 4)] + [EnumDataType(typeof(UnitTrackingAuthMode))] public int AuthMode { get; set; } = (int)UnitTrackingAuthMode.Bearer;🤖 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/Areas/User/Models/UnitTracking/UnitTrackingViewModels.cs` around lines 79 - 80, Replace the hardcoded Range validation on AuthMode with EnumDataType targeting UnitTrackingAuthMode, preserving the existing Bearer default and enum-backed validation behavior.Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml (1)
169-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded adapter key literal in view.
"resgrid-json-v1"is duplicated as a magic string here rather than referenced from a shared constant (e.g., onUnitTrackingCatalogProfileor a dedicated constants class), risking silent drift if the key changes 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/Areas/User/Views/UnitTracking/Details.cshtml` at line 169, Replace the hardcoded "resgrid-json-v1" comparison in the Details view with the shared constant exposed by UnitTrackingCatalogProfile or the established adapter-key constants class. Preserve the existing case-insensitive comparison and preview condition while ensuring the value comes from the single canonical definition.Web/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtml (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
data-identifier-requiredattribute is seeded but unused client-side.The profile
<option>carriesdata-identifier-required, butupdateProfileHelp()never reads it, so the UI never surfaces the identifier requirement before submit even though the server enforces it (ValidateProfilein the controller). Consider wiring this up to toggle a "required" hint/marker on the Device Identifier field when the selected profile requires one, or remove the unused attribute.Also applies to: 79-92
🤖 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/Areas/User/Views/UnitTracking/_Editor.cshtml` around lines 19 - 30, The profile option’s data-identifier-required attribute is unused by updateProfileHelp(), so the UI does not reflect the server-side requirement. Update updateProfileHelp() and the Device Identifier field markup to read the selected option’s flag and show or hide an appropriate required hint/marker; preserve the existing behavior when the profile does not require an identifier.Web/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.cs (2)
489-551: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPreview validation duplicates (and may diverge from) the production JSON parser.
ValidatePreviewJsonre-implements structural checks (size/depth limits, duplicate-key handling, positions array shape, per-position field validation) independently of the ingestion-side parser (UnitTrackingJsonPayloadParser, per the PR stack outline). If the two diverge, this preview tool could report success for payloads the real ingestion endpoint would reject, or vice versa, misleading admins testing device setups.Consider extracting/reusing the shared parsing logic (e.g., expose a
TryParse/Validatemethod on the ingress parser) so both paths stay in sync.🤖 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/Areas/User/Controllers/UnitTrackingController.cs` around lines 489 - 551, The ValidatePreviewJson method duplicates ingestion validation; expose and reuse a shared TryParse or Validate entry point from UnitTrackingJsonPayloadParser for preview requests. Move or centralize size, depth, duplicate-key, positions-shape, and coordinate checks there, then have ValidatePreviewJson consume its result and preserve the existing error key and position-count behavior.
609-647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated field-mapping between
BuildDeviceandApplyUpdate.Both methods map the same set of profile/model fields onto a
UnitTrackingDevice; consider a single mapper that both call (withIsEnabledsupplied by the caller) to avoid drift when new fields are added.🤖 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/Areas/User/Controllers/UnitTrackingController.cs` around lines 609 - 647, Consolidate the duplicated field assignments in BuildDevice and ApplyUpdate into one shared UnitTrackingDevice mapper, accepting IsEnabled from each caller so creation remains enabled while updates use model.IsEnabled. Have both methods reuse this mapper and preserve all existing model/profile field mappings.Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs (1)
132-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
[Range(1, 4)]duplicates enum knowledge.
UnitTrackingDevicesController.CreateCredentialalready validates withEnum.IsDefinedplus the profile'sSupportedAuthModes. Hardcoding the upper bound means the nextUnitTrackingAuthModevalue is rejected with a 400 that no one will trace back to this attribute.♻️ Rely on the enum-based validation already in the controller
- [Range(1, 4)] public int AuthMode { get; set; }🤖 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/UnitTracking/UnitTrackingAdminModels.cs` around lines 132 - 133, Remove the hardcoded [Range(1, 4)] validation from the AuthMode property in the unit-tracking admin model, leaving validation to UnitTrackingDevicesController.CreateCredential’s Enum.IsDefined and SupportedAuthModes checks so future enum values are accepted when supported.Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.cs (1)
84-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnsupported
Authorizationscheme short-circuits custom-header credentials.If any
Authorizationheader is present with a scheme other than Bearer/Basic (some trackers or intermediary proxies inject one), Line 101 returnsnulland the CustomHeader credential path is never evaluated, so an otherwise valid custom-header device is rejected. Consider falling through to the custom-header lookup instead of returning early.♻️ Fall through to custom-header lookup
if (authorization.StartsWith("Basic ", StringComparison.OrdinalIgnoreCase)) return ParseBasic(authorization.Substring("Basic ".Length).Trim()); - - return null; }Note: with this change the
MaximumAuthorizationHeaderLengthearly return should also fall through rather than reject outright, or be kept as-is if strict rejection of oversized headers is intended.🤖 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/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.cs` around lines 84 - 102, Update the Authorization parsing branch in the authentication method so unsupported schemes fall through to the existing custom-header credential lookup instead of returning null. Apply the same fall-through behavior to the MaximumAuthorizationHeaderLength check unless strict rejection of oversized headers is explicitly required; preserve the Bearer, Basic, and empty-token handling.Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs (1)
30-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s required dependency-resolution pattern.
This new controller constructor injects four services directly. Use
Bootstrapper.GetKernel().Resolve<T>()explicitly instead, per the repository rule.As per coding guidelines, “Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs` around lines 30 - 40, Update the UnitTrackingIngressController constructor to remove the four service parameters and resolve UnitTrackingHttpAuthenticationService, UnitTrackingJsonPayloadParser, UnitTrackingRateLimiter, and IUnitTrackingIngressService through Bootstrapper.GetKernel().Resolve<T>(), assigning each result to the corresponding fields.Source: Coding guidelines
Core/Resgrid.Model/Services/IUnitTrackingCatalogService.cs (1)
10-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not expose mutable singleton catalog entries.
UnitTrackingCatalogServicereturns its staticProfilesinstances directly, whileUnitTrackingCatalogProfileis initialized through mutable properties. A consumer can alter shared profile state, including selection/auth configuration, for subsequent requests. Return immutable profiles or defensive copies.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 `@Core/Resgrid.Model/Services/IUnitTrackingCatalogService.cs` around lines 10 - 15, Update UnitTrackingCatalogService methods GetProfilesAsync and GetProfileAsync so they never return the shared static Profiles instances directly. Return immutable profile values or defensive copies with independently owned mutable properties, preserving the existing profile data and lookup behavior while preventing consumers from modifying catalog state across requests.Source: Coding guidelines
Core/Resgrid.Services/UnitLocationSourceResolver.cs (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNaming deviates from the service convention for this project.
IUnitLocationSourceResolver/UnitLocationSourceResolverlives inCore/Resgrid.Servicesand is registered like other services, but does not use theI{Name}Service/{Name}Servicepair (e.g.IUnitLocationSourceService). Low priority given the DI/registration churn a rename implies.As per coding guidelines, "Service interface names must follow the pattern
I{Name}Serviceand implementations must be named{Name}Service".🤖 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/UnitLocationSourceResolver.cs` around lines 10 - 17, Rename the resolver symbols to follow the service convention: use IUnitLocationSourceService and UnitLocationSourceService instead of IUnitLocationSourceResolver and UnitLocationSourceResolver. Update the constructor, implemented interface, dependency-injection registration, and all references consistently while preserving behavior.Source: Coding guidelines
Core/Resgrid.Services/UnitTrackingAuthenticationService.cs (2)
231-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated validity predicate.
The credential window check in the
UnitTrackingAuthenticationResultoverload is identical to theUnitTrackingCredentialoverload; delegate to avoid the two drifting.♻️ Proposed refactor
private static bool IsActive(UnitTrackingAuthenticationResult result, DateTime utcNow) { if (result?.Credential == null || !IsEnabled(result.Device)) return false; - return result.Credential.ValidFrom <= utcNow && - !result.Credential.RevokedOn.HasValue && - (!result.Credential.ExpiresOn.HasValue || result.Credential.ExpiresOn > utcNow); + return IsActive(result.Credential, utcNow); }🤖 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/UnitTrackingAuthenticationService.cs` around lines 231 - 247, Update the UnitTrackingAuthenticationResult overload of IsActive to retain its null-result and device-enabled checks, then delegate credential validity evaluation to the existing IsActive(UnitTrackingCredential, DateTime) overload instead of duplicating the validity predicate.
252-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated credential-sanitization/key-normalization helpers across both tracking services.
SanitizeCredentialandNormalizeKeyare copied verbatim into two files; the shared root cause is the absence of one shared helper, so any futureUnitTrackingCredentialfield addition must be mirrored in both copies orSecretHashleaks / data is dropped from whichever copy is missed.
Core/Resgrid.Services/UnitTrackingAuthenticationService.cs#L252-L272: extractSanitizeCredentialandNormalizeKeyinto a single shared internal static helper inCore/Resgrid.Servicesand call it here.Core/Resgrid.Services/UnitTrackingService.cs#L560-L577: delete the localSanitizeCredential(andNormalizeKeyat L707-L708) and call the shared helper 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.Services/UnitTrackingAuthenticationService.cs` around lines 252 - 272, Extract the duplicated SanitizeCredential and NormalizeKey logic into one shared internal static helper in Core/Resgrid.Services, then update Core/Resgrid.Services/UnitTrackingAuthenticationService.cs lines 252-272 to call it. Remove the local SanitizeCredential and NormalizeKey implementations from Core/Resgrid.Services/UnitTrackingService.cs lines 560-577 and 707-708, replacing their usages with the shared helper while preserving the existing sanitization and normalization behavior.Core/Resgrid.Model/Services/IUnitTrackingAuthenticationService.cs (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a
bypassCacheparameter and consistentCancellationTokensurface.
GetEnabledDeviceByEndpointIdAsync,GetEnabledDeviceByProtocolIdentifierAsync, andGetActiveCredentialsForDeviceAsyncare cache-backed inUnitTrackingAuthenticationService, but callers can only get fresh data by calling theInvalidate*methods. AddingbypassCache = falsematches the service convention used elsewhere. Also,InvalidateCredentialAsync/InvalidateDeviceAsyncare the only async members without aCancellationToken.As per coding guidelines, "Service methods should include a
bypassCacheparameter (default:false) to allow callers to skip cache retrieval when fresh data is required".🤖 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/IUnitTrackingAuthenticationService.cs` around lines 24 - 29, Update the cache-backed methods GetEnabledDeviceByEndpointIdAsync, GetEnabledDeviceByProtocolIdentifierAsync, and GetActiveCredentialsForDeviceAsync to accept an optional bypassCache parameter defaulting to false, and propagate it through their implementations and cache lookup logic. Add optional CancellationToken parameters defaulting to default to InvalidateCredentialAsync and InvalidateDeviceAsync, updating implementations and call sites to preserve cancellation support.Source: Coding guidelines
Providers/Resgrid.Providers.Bus/EventAggregator.cs (1)
26-32: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAsync dispatch keys on the static type, unlike the sync path.
typeof(TMessage)resolves at compile time, so a caller holding the event in a base-class/interface/object-typed variable silently matches no listeners and the message is dropped without any signal._hub.Publish(sync path) dispatches on the runtime type, so the two APIs behave differently for the same call shape. Keying onmessage?.GetType()(with a null guard) removes the trap.♻️ Proposed change
public async Task SendMessageAsync<TMessage>(TMessage message) { - if (!_asyncListeners.TryGetValue(typeof(TMessage), out var listeners)) + if (message == null) + throw new ArgumentNullException(nameof(message)); + + if (!_asyncListeners.TryGetValue(message.GetType(), out var listeners)) return; await Task.WhenAll(listeners.Values.Select(listener => listener(message))); }🤖 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.Bus/EventAggregator.cs` around lines 26 - 32, Update SendMessageAsync<TMessage> to resolve the listener key from message.GetType() at runtime, with an appropriate null guard, instead of typeof(TMessage). Preserve the existing no-listener return and asynchronous listener dispatch behavior so async dispatch matches the sync path for base-class, interface, and object-typed references.Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs (1)
252-268: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBatch-wide publish timeout can leave the batch half-published.
publishTimeoutis created once andCancelAfterapplies to the whole loop, so with a large collection the laterBasicPublishAsynccalls can be cancelled after earlier ones were already confirmed. The method then returnsfalse, and the caller has no way to know how many messages landed — a retry re-publishes the entire batch. A per-message timeout (or returning the count/failure index) makes the contract honest.🤖 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.Bus.Rabbit/RabbitOutboundQueueProvider.cs` around lines 252 - 268, Change the publish loop in the outbound batch method so each message gets its own timeout token and timeout window when calling BasicPublishAsync, preventing one shared publishTimeout from expiring across the entire batch. Preserve the existing success return and cancellation behavior while ensuring a timeout applies only to the current message.Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs (3)
727-748: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate header-parsing switch.
This is the same numeric-header parse already inlined in
RetryQueueItem(Lines 759-778), now with clamping added — the two copies have already diverged. Extracting a singleTryGetIntHeader(headers, name)helper and using it from both keeps the semantics aligned.🤖 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.Bus.Rabbit/RabbitInboundQueueProvider.cs` around lines 727 - 748, Replace the duplicated numeric header parsing in GetRetryCount and RetryQueueItem with a shared TryGetIntHeader(headers, name) helper. Move the existing type handling and non-negative/int-range clamping into that helper, then have both callers use it while preserving a zero result for missing or unparseable headers.
663-671: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPermanent and transient failures are handled identically. The worker signals unrecoverable payload problems (missing
UnitId/DepartmentId, absent coordinates) with the same plain exceptions used for transient store/broker failures, and the consumer's catch cannot tell them apart — so a message that can never succeed still consumes the fullUnitLocationMaxRetryAttemptsbudget, and requeues in a tight loop whenever the republish itself fails.
Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs#L663-L671: catch a dedicated non-retryable exception type separately andBasicNackAsync(..., requeue: false)(or publish straight to the dead-letter queue) instead of the retry path.Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs#L15-L28: throw a distinct non-retryable exception type from the validation guards so the consumer can classify them.🤖 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.Bus.Rabbit/RabbitInboundQueueProvider.cs` around lines 663 - 671, The unit-location consumer must distinguish unrecoverable validation failures from retryable exceptions. In Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs lines 663-671, catch the dedicated non-retryable exception separately and acknowledge it with BasicNackAsync using requeue false, while preserving RetryOrDeadLetterUnitLocationAsync for other exceptions; in Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs lines 15-28, update the UnitId, DepartmentId, and coordinate validation guards to throw that distinct non-retryable exception type.
688-716: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse a confirm channel for retry publisher
RetryOrDeadLetterUnitLocationAsynccreates a new channel for every failed delivery, andRabbitConnection.CreateConnectioncurrently creates a new shared connection on failed initialization paths. Under failure bursts this generates one channel open/close per message, increasing broker-handshake load and exhausting connection channel capacity. Keep the retry publisher as a single lazy/channel-recoverable channel instead of per-message churn.🤖 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.Bus.Rabbit/RabbitInboundQueueProvider.cs` around lines 688 - 716, Update RetryOrDeadLetterUnitLocationAsync to reuse a single lazily initialized confirm channel for retry publishing instead of creating and disposing a channel per delivery. Back the channel with a shared connection, recover or recreate it when closed or publishing fails, and preserve the existing publish options, timeout, and retry behavior.Core/Resgrid.Services/UnitsService.cs (2)
545-565: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cancellationTokenis now unused, and the Mongo new-vs-existing check is obscure.Neither
InsertAsync/UpdateAsyncnorSendMessageAsyncreceive the token, so callers passing one (e.g.UnitLocationQueueLogic) get no cancellation. Alsolocation.Id.Timestamp == 0is an indirect way of asking "is this ObjectId unset" —location.Id == ObjectId.Emptystates the intent directly.♻️ Proposed change
else { - if (location.Id.Timestamp == 0) - result = await _unitLocationsMongoRepository.Value.InsertAsync(location); + if (location.Id == ObjectId.Empty) + result = await _unitLocationsMongoRepository.Value.InsertAsync(location, cancellationToken); else - result = await _unitLocationsMongoRepository.Value.UpdateAsync(location); + result = await _unitLocationsMongoRepository.Value.UpdateAsync(location, cancellationToken); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/UnitsService.cs` around lines 545 - 565, Update AddUnitLocationAsync to propagate cancellationToken through the relevant repository and SendMessageAsync calls so caller cancellation is honored, including the UnitLocationQueueLogic path. Replace the Mongo insert/update condition based on location.Id.Timestamp with a direct comparison against ObjectId.Empty, preserving the existing insert behavior for unset IDs and update behavior for existing IDs.
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the overlapping unit-location read/write repositories
SaveUnitLocationAsyncnow uses_unitLocationsMongoRepositoryfor writes, butGetLatestUnitLocationAsyncandGetLatestUnitLocationsAsyncstill rely on_unitLocationRepositoryfor MongoDB reads. Move the read paths under_unitLocationsMongoRepositoryor expose equivalent read methods on that abstraction so the same data source is not split between two abstractions.This also increases
UnitsServiceto 17 constructor parameters; per the C# guidelines, prefer resolving dependencies explicitly viaBootstrapper.GetKernel().Resolve<T>()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 `@Core/Resgrid.Services/UnitsService.cs` at line 25, Consolidate unit-location access in UnitsService by updating GetLatestUnitLocationAsync and GetLatestUnitLocationsAsync to use _unitLocationsMongoRepository, adding equivalent read methods there if needed, while preserving their current behavior. Remove the redundant _unitLocationRepository dependency and resolve any required repository explicitly through Bootstrapper.GetKernel().Resolve<T>() instead of increasing constructor injection.Source: Coding guidelines
Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs (1)
61-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThrowing here fails the caller's persistence path, not just the broadcast.
EventAggregator.SendMessageAsyncpropagates listener exceptions, and the only caller isUnitsService.AddUnitLocationAsync(Line 569), which invokes it after the location row is already written. So a transient topic-publish hiccup makesAddUnitLocationAsyncthrow, which makesUnitLocationQueueLogic.ProcessUnitLocationQueueItemthrow, which drives the whole message back through the retry/dead-letter pipeline and re-persists the location.If that at-least-once behavior is intentional it's workable given idempotent writes, but consider surfacing it distinctly (e.g. log + a dedicated broadcast-retry) so a realtime failure doesn't re-drive persistence and doesn't consume the location message's retry budget.
Also applies to: 613-620
🤖 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.Bus/OutboundEventProvider.cs` at line 61, Update the outbound broadcast handling around the event listeners at the referenced locations so EventAggregator.SendMessageAsync failures are caught and handled separately from the persistence call. Log the broadcast failure and route it through a dedicated broadcast retry mechanism, ensuring UnitsService.AddUnitLocationAsync and UnitLocationQueueLogic.ProcessUnitLocationQueueItem do not rethrow realtime delivery errors or consume the location message retry budget.
🤖 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.Config/ServiceBusConfig.cs`:
- Around line 53-55: Update the UnitLocationQueueV2Name,
UnitLocationRetryQueueV2Name, and UnitLocationDeadQueueV2Name definitions in
ServiceBusConfig to follow the existing `#if` DEBUG queue-name split, applying the
established test suffix or equivalent DEBUG-only isolation while preserving the
current release names.
In `@Core/Resgrid.Framework/SentryTransactionFilter.cs`:
- Around line 64-70: Update the transaction filtering logic around
RedactCapabilityPath so transaction.Name is also passed through the same
redaction before the status check. Preserve the existing Request.Url redaction
and ensure both fields are scrubbed, including 404 transactions whose names
derive from the raw request path.
In `@Core/Resgrid.Services/UnitTrackingAuthenticationService.cs`:
- Around line 39-45: Update the token generation logic around
RandomNumberGenerator and keyPrefix so the stored/displayed prefix is generated
from separate random bytes rather than derived from encodedSecret. Encode the
independent prefix with the same base64url-safe normalization, then prepend it
to the token while preserving the existing rgtrk token format and full secret
entropy.
- Around line 195-205: Update the cached result handling in the credential
retrieval flow before the `cached.Where(...)` call to coalesce a null provider
result to an empty credential collection. Preserve the existing filtering and
list conversion for non-null results.
In `@Core/Resgrid.Services/UnitTrackingService.cs`:
- Around line 135-136: Update the edit flow containing the existing.IsEnabled
and device.IsEnabled check so requested field changes are applied before
handling the disable transition; do not return early to DisableDeviceAsync
before updating DisplayName, keys, identifiers, SourcePriority,
AllowedSourceCidrs, and FirmwareVersion. Preserve the disable behavior after the
updates, and expose the credential-revocation consequence through the existing
caller/UI notification or response mechanism.
- Line 102: Validate device.AllowedSourceCidrs before persisting it in the
device create/update flow, rather than only applying TrimToNull. Parse each
nonblank CIDR entry with TryParse, enforce valid prefix bounds, and require
canonical entries; reject malformed values before saving while preserving the
network policy’s blank-value behavior.
- Around line 681-697: Update IsHttpTokenCharacter to accept only ASCII letters
and digits, while retaining the existing RFC 9110 tchar punctuation set; replace
the Unicode-permissive char.IsLetterOrDigit check so CustomHeader validation in
the surrounding authentication configuration rejects characters such as é and
non-ASCII digits.
- Around line 633-660: Update CopyForRebind to preserve the source device’s
enabled state instead of hardcoding IsEnabled to true, and derive LastStatus
from that state using the same behavior as CreateDeviceAsync. Keep all other
copied properties unchanged.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs`:
- Around line 142-186: Isolate the UnitLocation V2 queue declaration and binding
block from the main setup flow by moving it after the unrelated queue
declarations and/or extracting it into a dedicated helper such as
DeclareUnitLocationV2QueuesAsync. Catch and log failures within that isolated
flow so a PRECONDITION_FAILED from the TTL arguments does not abort declarations
for PersonnelLoactionQueueName, EventingTopic, SecurityRefreshQueueName,
WorkflowQueueName, or ChatbotProcessingQueueName.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs`:
- Around line 188-194: Replace ASCII encoding with UTF-8 in both unit-location
publish paths: SendMessage at
Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs lines
188-194 and SendMessagesWithConfirmation at lines 259-265. Update each message
body encoding while preserving the existing publish behavior.
In `@Providers/Resgrid.Providers.Migrations/Migrations/M0101_AddUnitTracking.cs`:
- Around line 42-48: Replace the separate department and unit foreign keys in
M0101_AddUnitTracking.cs and M0101_AddUnitTrackingPg.cs at lines 42-48 with a
composite foreign key linking UnitTrackingDevices.(DepartmentId, UnitId) to the
matching department-owned unit key (DepartmentId, UnitId), preserving the
existing table and constraint intent in both migrations.
In
`@Repositories/Resgrid.Repositories.DataRepository/UnitTrackingCredentialsRepository.cs`:
- Around line 22-32: Replace constructor-injected collaborator parameters with
explicit Bootstrapper.GetKernel().Resolve<T>() calls in
UnitTrackingCredentialsRepository.cs (lines 22-32) and
UnitTrackingDevicesRepository.cs (lines 22-32), assigning each resolved
dependency to the existing fields. In DocumentDbRepository.cs (lines 13-18),
resolve the Mongo repository through the same service-locator pattern while
preserving its deferred initialization behavior.
In
`@Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsMongoRepository.cs`:
- Around line 57-62: Update EnsureIndexesAsync and the cached _ensureIndexesTask
flow so a failed CreateIndexesAsync operation clears the cached task after its
await fails, allowing the next call to retry index initialization. Preserve the
existing lock and successful-task caching behavior.
In
`@Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingJsonPayloadParser.cs`:
- Around line 232-238: Update ReadBoundedBodyAsync so the chunk allocation no
longer computes maximumBytes + 1; size the chunk using the bounded maximum
directly while retaining the existing total-byte limit enforcement.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitLocationController.cs`:
- Around line 103-104: Update the enqueue flow in EnqueueUnitLocationEventAsync
handling within the controller action so exceptions from queue submission are
caught and mapped to 503 ServiceUnavailable, matching the existing false-result
path. Ensure this handling occurs before the outer catch that returns 400, while
preserving the current response behavior for other failures.
In `@Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs`:
- Around line 98-99: Validate AllowedSourceCidrs at the API model boundary in
the relevant model properties, rejecting malformed CIDR entries such as invalid
prefixes or incomplete addresses instead of relying on
UnitTrackingNetworkPolicy.Contains. Preserve valid CIDR-list input and return a
clear validation error for any invalid entry, while retaining the existing
MaxLength constraint.
In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml`:
- Around line 137-154: Associate the Create Credential labels with their
controls by updating the labels in the authentication mode, custom header, and
basic username groups to use the corresponding asp-for bindings or matching for
attributes. Ensure they target CredentialInput.AuthMode,
CredentialInput.HeaderName, and CredentialInput.BasicUsername, while preserving
the existing label styling and text.
In `@Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs`:
- Around line 24-25: Update the UnitLocationQueueLogic handling so an unknown
IsValidFix value remains null rather than being coerced to true when persisted
to UnitsLocation. Preserve the existing invalid-fix early return, but add the
appropriate operational logging or metric before returning success so dropped
invalid fixes are observable.
- Around line 15-28: Restore try-catch handling around the validation logic in
the relevant method, retain the existing exception propagation for rejected
events, and call Resgrid.Framework.Logging.LogException() in the catch before
rethrowing. Re-add the Resgrid.Framework import required for the static logging
API, while preserving the current validation and retry/nack behavior.
---
Outside diff comments:
In `@Core/Resgrid.Services/DepartmentSettingsService.cs`:
- Around line 48-69: Update SaveOrUpdateSettingAsync so
InvalidateSettingCacheAsync runs only after SaveOrUpdateAsync completes
successfully for both the new-setting and existing-setting branches. Preserve
the saved result, invalidate using departmentId and type after persistence, then
return the result; remove the current pre-write invalidation.
---
Nitpick comments:
In `@Core/Resgrid.Model/Services/IUnitTrackingAuthenticationService.cs`:
- Around line 24-29: Update the cache-backed methods
GetEnabledDeviceByEndpointIdAsync, GetEnabledDeviceByProtocolIdentifierAsync,
and GetActiveCredentialsForDeviceAsync to accept an optional bypassCache
parameter defaulting to false, and propagate it through their implementations
and cache lookup logic. Add optional CancellationToken parameters defaulting to
default to InvalidateCredentialAsync and InvalidateDeviceAsync, updating
implementations and call sites to preserve cancellation support.
In `@Core/Resgrid.Model/Services/IUnitTrackingCatalogService.cs`:
- Around line 10-15: Update UnitTrackingCatalogService methods GetProfilesAsync
and GetProfileAsync so they never return the shared static Profiles instances
directly. Return immutable profile values or defensive copies with independently
owned mutable properties, preserving the existing profile data and lookup
behavior while preventing consumers from modifying catalog state across
requests.
In `@Core/Resgrid.Services/UnitLocationSourceResolver.cs`:
- Around line 10-17: Rename the resolver symbols to follow the service
convention: use IUnitLocationSourceService and UnitLocationSourceService instead
of IUnitLocationSourceResolver and UnitLocationSourceResolver. Update the
constructor, implemented interface, dependency-injection registration, and all
references consistently while preserving behavior.
In `@Core/Resgrid.Services/UnitsService.cs`:
- Around line 545-565: Update AddUnitLocationAsync to propagate
cancellationToken through the relevant repository and SendMessageAsync calls so
caller cancellation is honored, including the UnitLocationQueueLogic path.
Replace the Mongo insert/update condition based on location.Id.Timestamp with a
direct comparison against ObjectId.Empty, preserving the existing insert
behavior for unset IDs and update behavior for existing IDs.
- Line 25: Consolidate unit-location access in UnitsService by updating
GetLatestUnitLocationAsync and GetLatestUnitLocationsAsync to use
_unitLocationsMongoRepository, adding equivalent read methods there if needed,
while preserving their current behavior. Remove the redundant
_unitLocationRepository dependency and resolve any required repository
explicitly through Bootstrapper.GetKernel().Resolve<T>() instead of increasing
constructor injection.
In `@Core/Resgrid.Services/UnitTrackingAuthenticationService.cs`:
- Around line 231-247: Update the UnitTrackingAuthenticationResult overload of
IsActive to retain its null-result and device-enabled checks, then delegate
credential validity evaluation to the existing IsActive(UnitTrackingCredential,
DateTime) overload instead of duplicating the validity predicate.
- Around line 252-272: Extract the duplicated SanitizeCredential and
NormalizeKey logic into one shared internal static helper in
Core/Resgrid.Services, then update
Core/Resgrid.Services/UnitTrackingAuthenticationService.cs lines 252-272 to call
it. Remove the local SanitizeCredential and NormalizeKey implementations from
Core/Resgrid.Services/UnitTrackingService.cs lines 560-577 and 707-708,
replacing their usages with the shared helper while preserving the existing
sanitization and normalization behavior.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs`:
- Around line 727-748: Replace the duplicated numeric header parsing in
GetRetryCount and RetryQueueItem with a shared TryGetIntHeader(headers, name)
helper. Move the existing type handling and non-negative/int-range clamping into
that helper, then have both callers use it while preserving a zero result for
missing or unparseable headers.
- Around line 663-671: The unit-location consumer must distinguish unrecoverable
validation failures from retryable exceptions. In
Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs lines
663-671, catch the dedicated non-retryable exception separately and acknowledge
it with BasicNackAsync using requeue false, while preserving
RetryOrDeadLetterUnitLocationAsync for other exceptions; in
Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs lines 15-28,
update the UnitId, DepartmentId, and coordinate validation guards to throw that
distinct non-retryable exception type.
- Around line 688-716: Update RetryOrDeadLetterUnitLocationAsync to reuse a
single lazily initialized confirm channel for retry publishing instead of
creating and disposing a channel per delivery. Back the channel with a shared
connection, recover or recreate it when closed or publishing fails, and preserve
the existing publish options, timeout, and retry behavior.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs`:
- Around line 252-268: Change the publish loop in the outbound batch method so
each message gets its own timeout token and timeout window when calling
BasicPublishAsync, preventing one shared publishTimeout from expiring across the
entire batch. Preserve the existing success return and cancellation behavior
while ensuring a timeout applies only to the current message.
In `@Providers/Resgrid.Providers.Bus/EventAggregator.cs`:
- Around line 26-32: Update SendMessageAsync<TMessage> to resolve the listener
key from message.GetType() at runtime, with an appropriate null guard, instead
of typeof(TMessage). Preserve the existing no-listener return and asynchronous
listener dispatch behavior so async dispatch matches the sync path for
base-class, interface, and object-typed references.
In `@Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs`:
- Line 61: Update the outbound broadcast handling around the event listeners at
the referenced locations so EventAggregator.SendMessageAsync failures are caught
and handled separately from the persistence call. Log the broadcast failure and
route it through a dedicated broadcast retry mechanism, ensuring
UnitsService.AddUnitLocationAsync and
UnitLocationQueueLogic.ProcessUnitLocationQueueItem do not rethrow realtime
delivery errors or consume the location message retry budget.
In
`@Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.cs`:
- Around line 84-102: Update the Authorization parsing branch in the
authentication method so unsupported schemes fall through to the existing
custom-header credential lookup instead of returning null. Apply the same
fall-through behavior to the MaximumAuthorizationHeaderLength check unless
strict rejection of oversized headers is explicitly required; preserve the
Bearer, Basic, and empty-token handling.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs`:
- Around line 30-40: Update the UnitTrackingIngressController constructor to
remove the four service parameters and resolve
UnitTrackingHttpAuthenticationService, UnitTrackingJsonPayloadParser,
UnitTrackingRateLimiter, and IUnitTrackingIngressService through
Bootstrapper.GetKernel().Resolve<T>(), assigning each result to the
corresponding fields.
In `@Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs`:
- Around line 132-133: Remove the hardcoded [Range(1, 4)] validation from the
AuthMode property in the unit-tracking admin model, leaving validation to
UnitTrackingDevicesController.CreateCredential’s Enum.IsDefined and
SupportedAuthModes checks so future enum values are accepted when supported.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.cs`:
- Around line 489-551: The ValidatePreviewJson method duplicates ingestion
validation; expose and reuse a shared TryParse or Validate entry point from
UnitTrackingJsonPayloadParser for preview requests. Move or centralize size,
depth, duplicate-key, positions-shape, and coordinate checks there, then have
ValidatePreviewJson consume its result and preserve the existing error key and
position-count behavior.
- Around line 609-647: Consolidate the duplicated field assignments in
BuildDevice and ApplyUpdate into one shared UnitTrackingDevice mapper, accepting
IsEnabled from each caller so creation remains enabled while updates use
model.IsEnabled. Have both methods reuse this mapper and preserve all existing
model/profile field mappings.
In `@Web/Resgrid.Web/Areas/User/Models/UnitTracking/UnitTrackingViewModels.cs`:
- Around line 79-80: Replace the hardcoded Range validation on AuthMode with
EnumDataType targeting UnitTrackingAuthMode, preserving the existing Bearer
default and enum-backed validation behavior.
In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtml`:
- Around line 19-30: The profile option’s data-identifier-required attribute is
unused by updateProfileHelp(), so the UI does not reflect the server-side
requirement. Update updateProfileHelp() and the Device Identifier field markup
to read the selected option’s flag and show or hide an appropriate required
hint/marker; preserve the existing behavior when the profile does not require an
identifier.
In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml`:
- Line 169: Replace the hardcoded "resgrid-json-v1" comparison in the Details
view with the shared constant exposed by UnitTrackingCatalogProfile or the
established adapter-key constants class. Preserve the existing case-insensitive
comparison and preview condition while ensuring the value comes from the single
canonical definition.
🪄 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: 8d6e3773-5a91-4618-9a51-1c93ee77db66
⛔ Files ignored due to path filters (26)
Core/Resgrid.Localization/Areas/User/Units/Units.resxis excluded by!**/*.resxTests/Resgrid.Tests/Framework/SentryTransactionFilterTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Framework/UnitLocationEventSerializationTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Providers/EventAggregatorTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Providers/UnitLocationEventProviderTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Repositories/DocumentDbRepositoryTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Resgrid.Tests.csprojis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/DepartmentSettingsServiceUnitTrackingTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitLocationSourceResolverTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingAuthenticationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingCatalogServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingEventIdServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingIdentifierServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingIngressServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingStatusServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/CapabilityPathRedactionMiddlewareTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitLocationControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingDevicesControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingHttpAuthenticationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingIngressControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingJsonPayloadParserTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingNetworkPolicyTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingRateLimiterTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/User/UnitTrackingControllerTests.csis excluded by!**/Tests/**
📒 Files selected for processing (94)
.coderabbit.yamlCore/Resgrid.Config/ServiceBusConfig.csCore/Resgrid.Config/UnitTrackingConfig.csCore/Resgrid.Framework/SentryTransactionFilter.csCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/Events/UnitLocationEvent.csCore/Resgrid.Model/Providers/IEventAggregator.csCore/Resgrid.Model/Providers/IRabbitOutboundQueueProvider.csCore/Resgrid.Model/Providers/IUnitLocationEventProvider.csCore/Resgrid.Model/Repositories/IDocumentDbRepository.csCore/Resgrid.Model/Repositories/IUnitLocationsDocRepository.csCore/Resgrid.Model/Repositories/IUnitLocationsMongoRepository.csCore/Resgrid.Model/Repositories/IUnitTrackingCredentialsRepository.csCore/Resgrid.Model/Repositories/IUnitTrackingDevicesRepository.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Model/Services/IUnitLocationSourceResolver.csCore/Resgrid.Model/Services/IUnitTrackingAuthenticationService.csCore/Resgrid.Model/Services/IUnitTrackingCatalogService.csCore/Resgrid.Model/Services/IUnitTrackingEventIdService.csCore/Resgrid.Model/Services/IUnitTrackingIdentifierService.csCore/Resgrid.Model/Services/IUnitTrackingIngressService.csCore/Resgrid.Model/Services/IUnitTrackingService.csCore/Resgrid.Model/Services/IUnitTrackingStatusService.csCore/Resgrid.Model/Services/IUnitsService.csCore/Resgrid.Model/Tracking/AuthenticatedTrackingSource.csCore/Resgrid.Model/Tracking/CanonicalTrackingPosition.csCore/Resgrid.Model/Tracking/TrackingIngressResult.csCore/Resgrid.Model/Tracking/UnitTrackingCatalogProfile.csCore/Resgrid.Model/UnitLocationWriteResult.csCore/Resgrid.Model/UnitTrackingCredential.csCore/Resgrid.Model/UnitTrackingDevice.csCore/Resgrid.Model/UnitTrackingEnums.csCore/Resgrid.Model/UnitTrackingResults.csCore/Resgrid.Model/UnitsLocation.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/UnitLocationSourceResolver.csCore/Resgrid.Services/UnitTrackingAuthenticationService.csCore/Resgrid.Services/UnitTrackingCacheKeys.csCore/Resgrid.Services/UnitTrackingCatalogService.csCore/Resgrid.Services/UnitTrackingEventIdService.csCore/Resgrid.Services/UnitTrackingIdentifierService.csCore/Resgrid.Services/UnitTrackingIngressService.csCore/Resgrid.Services/UnitTrackingService.csCore/Resgrid.Services/UnitTrackingStatusService.csCore/Resgrid.Services/UnitsService.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.csProviders/Resgrid.Providers.Bus/BusModule.csProviders/Resgrid.Providers.Bus/EventAggregator.csProviders/Resgrid.Providers.Bus/OutboundEventProvider.csProviders/Resgrid.Providers.Bus/UnitLocationEventProvider.csProviders/Resgrid.Providers.Migrations/Migrations/M0101_AddUnitTracking.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.csProviders/Resgrid.Providers.MigrationsPg/Sql/EF0003_PopulateDocDb.sqlRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingCredentialBySecretHashQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingCredentialsByDeviceQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingDeviceByProtocolIdentifierQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingDevicesByUnitQuery.csRepositories/Resgrid.Repositories.DataRepository/UnitTrackingCredentialsRepository.csRepositories/Resgrid.Repositories.DataRepository/UnitTrackingDevicesRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/NoSqlDataModule.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsMongoRepository.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingJsonPayloadParser.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingNetworkPolicy.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingRateLimiter.csWeb/Resgrid.Web.Services/Controllers/v4/UnitLocationController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitTrackingDevicesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.csWeb/Resgrid.Web.Services/Middleware/CapabilityPathRedactionMiddleware.csWeb/Resgrid.Web.Services/Middleware/SentryEventProcessor.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingIngressResponse.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingPositionInput.csWeb/Resgrid.Web.Services/Program.csWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.csWeb/Resgrid.Web/Areas/User/Models/UnitTracking/UnitTrackingViewModels.csWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Credential.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtmlWorkers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs
| public static string UnitLocationQueueV2Name = "unitlocation-v2"; | ||
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry"; | ||
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Isolate the v2 queues in DEBUG builds.
Lines 53-55 are outside the existing #if DEBUG split, so DEBUG and release builds use the same RabbitMQ queue names. Mirror the existing test suffix convention or verify that DEBUG always uses a completely isolated broker.
Proposed fix
+#if DEBUG
+ public static string UnitLocationQueueV2Name = "unitlocation-v2test";
+ public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retrytest";
+ public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.deadtest";
+#else
public static string UnitLocationQueueV2Name = "unitlocation-v2";
public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry";
public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead";
+#endif📝 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 static string UnitLocationQueueV2Name = "unitlocation-v2"; | |
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry"; | |
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead"; | |
| `#if` DEBUG | |
| public static string UnitLocationQueueV2Name = "unitlocation-v2test"; | |
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retrytest"; | |
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.deadtest"; | |
| `#else` | |
| public static string UnitLocationQueueV2Name = "unitlocation-v2"; | |
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry"; | |
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead"; | |
| `#endif` |
🤖 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.Config/ServiceBusConfig.cs` around lines 53 - 55, Update the
UnitLocationQueueV2Name, UnitLocationRetryQueueV2Name, and
UnitLocationDeadQueueV2Name definitions in ServiceBusConfig to follow the
existing `#if` DEBUG queue-name split, applying the established test suffix or
equivalent DEBUG-only isolation while preserving the current release names.
| if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location)) | ||
| return StatusCode(StatusCodes.Status503ServiceUnavailable); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return 503 when enqueueing throws.
A faulted EnqueueUnitLocationEventAsync task reaches the outer catch and becomes 400 BadRequest, so clients will not treat a queue outage as retryable.
Suggested handling
- if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location))
- return StatusCode(StatusCodes.Status503ServiceUnavailable);
+ try
+ {
+ if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location))
+ return StatusCode(StatusCodes.Status503ServiceUnavailable);
+ }
+ catch (Exception ex)
+ {
+ Logging.LogException(ex);
+ return StatusCode(StatusCodes.Status503ServiceUnavailable);
+ }📝 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.
| if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location)) | |
| return StatusCode(StatusCodes.Status503ServiceUnavailable); | |
| try | |
| { | |
| if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location)) | |
| return StatusCode(StatusCodes.Status503ServiceUnavailable); | |
| } | |
| catch (Exception ex) | |
| { | |
| Logging.LogException(ex); | |
| return StatusCode(StatusCodes.Status503ServiceUnavailable); | |
| } |
🤖 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/UnitLocationController.cs` around
lines 103 - 104, Update the enqueue flow in EnqueueUnitLocationEventAsync
handling within the controller action so exceptions from queue submission are
caught and mapped to 503 ServiceUnavailable, matching the existing false-result
path. Ensure this handling occurs before the outer catch that returns 400, while
preserving the current response behavior for other failures.
| public static bool Enabled = false; | ||
| public static bool HttpsIngressEnabled = false; | ||
| public static bool NativeGatewayEnabled = false; | ||
| public static string CredentialPepper = ""; |
There was a problem hiding this comment.
Mutable static fields, such as CredentialPepper, risk accidental runtime modification. Declare compile-time constant values using const or static readonly to ensure immutability and thread safety.
Kody rule violation: Use `readonly` or `const` for Immutable Data
Prompt for LLM
File Core/Resgrid.Config/UnitTrackingConfig.cs:
Line 8:
Mutable static fields, such as `CredentialPepper`, risk accidental runtime modification. Declare compile-time constant values using `const` or `static readonly` to ensure immutability and thread safety.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| public static string RedactCapabilityPath(string value) | ||
| { | ||
| const string prefix = "/api/v4/unit-trackers/c/"; |
There was a problem hiding this comment.
Decentralized string literals like route names violate reuse guidelines and complicate updates when paths change. Move this route-name constant to a centralized shared constants class, such as RouteConstants.UnitTrackerCapabilityPrefix, and reference it throughout the codebase.
Kody rule violation: Centralize string constants
Prompt for LLM
File Core/Resgrid.Framework/SentryTransactionFilter.cs:
Line 82:
Decentralized string literals like route names violate reuse guidelines and complicate updates when paths change. Move this route-name constant to a centralized shared constants class, such as `RouteConstants.UnitTrackerCapabilityPrefix`, and reference it throughout the codebase.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <returns>Returns the current IEventSubscriptionManager to allow for easy fluent additions.</returns> | ||
| Guid AddListener<T>(Action<T> listener); | ||
|
|
||
| Guid AddAsyncListener<T>(Func<T, Task> listener); |
There was a problem hiding this comment.
Unobserved task rejections occur when async listeners provided to AddAsyncListener<T> fault without a defined error-handling path. Add an error handler parameter, such as Action<Exception> onError, to allow deterministic error handling alongside the subscription.
Kody rule violation: Provide error handlers to subscription/listener APIs
Prompt for LLM
File Core/Resgrid.Model/Providers/IEventAggregator.cs:
Line 29:
Unobserved task rejections occur when async listeners provided to `AddAsyncListener<T>` fault without a defined error-handling path. Add an error handler parameter, such as `Action<Exception> onError`, to allow deterministic error handling alongside the subscription.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Task<TrackingIngressResult> AcceptAsync( | ||
| AuthenticatedTrackingSource source, | ||
| IReadOnlyCollection<CanonicalTrackingPosition> positions, | ||
| CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Ambiguous async contract identified on AcceptAsync due to missing XML documentation describing resolution values, rejection conditions, or cancellation semantics. Add XML doc comments to detail the success result shape, faulted states, and OperationCanceledException propagation behavior.
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File Core/Resgrid.Model/Services/IUnitTrackingIngressService.cs:
Line 10 to 13:
Ambiguous async contract identified on `AcceptAsync` due to missing XML documentation describing resolution values, rejection conditions, or cancellation semantics. Add XML doc comments to detail the success result shape, faulted states, and `OperationCanceledException` propagation behavior.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Task<UnitTrackingDevice> GetDeviceByIdAsync(string deviceId, int departmentId); | ||
| Task<List<UnitTrackingDevice>> GetDevicesForDepartmentAsync(int departmentId); | ||
| Task<List<UnitTrackingDevice>> GetDevicesForUnitAsync(int departmentId, int unitId); | ||
| Task<List<UnitTrackingCredential>> GetCredentialsForDeviceAsync(string deviceId, int departmentId); |
There was a problem hiding this comment.
Mutable return types expose APIs to unintended side effects by allowing callers to modify the underlying collection. Change the GetCredentialsForDeviceAsync return type to Task<IReadOnlyList<UnitTrackingCredential>> to enforce immutability.
Kody rule violation: Use IReadOnlyList for immutable collections
Prompt for LLM
File Core/Resgrid.Model/Services/IUnitTrackingService.cs:
Line 13:
Mutable return types expose APIs to unintended side effects by allowing callers to modify the underlying collection. Change the `GetCredentialsForDeviceAsync` return type to `Task<IReadOnlyList<UnitTrackingCredential>>` to enforce immutability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <returns>Task<UnitLocation>.</returns> | ||
| Task<UnitsLocation> AddUnitLocationAsync(UnitsLocation location, int departmentId, | ||
| /// <returns>The idempotent document-store write result.</returns> | ||
| Task<UnitLocationWriteResult> AddUnitLocationAsync(UnitsLocation location, int departmentId, |
There was a problem hiding this comment.
Breaking contract change identified by modifying the AddUnitLocationAsync return type from Task<UnitsLocation> to Task<UnitLocationWriteResult>. Document this modification with an explicit 'BREAKING CHANGE' section detailing migration steps, affected consumers, and a rollout plan.
Kody rule violation: Call out breaking changes explicitly
Prompt for LLM
File Core/Resgrid.Model/Services/IUnitsService.cs:
Line 308:
Breaking contract change identified by modifying the `AddUnitLocationAsync` return type from `Task<UnitsLocation>` to `Task<UnitLocationWriteResult>`. Document this modification with an explicit 'BREAKING CHANGE' section detailing migration steps, affected consumers, and a rollout plan.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public static UnitLocationWriteResult Duplicate(UnitsLocation location) | ||
| { | ||
| return new UnitLocationWriteResult | ||
| { | ||
| Status = UnitLocationWriteStatus.Duplicate, | ||
| Location = location | ||
| }; | ||
| } |
There was a problem hiding this comment.
Duplicate code sequences identified in the Duplicate and Inserted factory methods, which differ only by the Status enum value. Extract a private helper method accepting the status parameter to reduce maintenance burden and consolidate instantiation logic.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Core/Resgrid.Model/UnitLocationWriteResult.cs:
Line 23 to 30:
Duplicate code sequences identified in the `Duplicate` and `Inserted` factory methods, which differ only by the `Status` enum value. Extract a private helper method accepting the status parameter to reduce maintenance burden and consolidate instantiation logic.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| LongCacheLength) | ||
| : await getSetting(); | ||
|
|
||
| return int.TryParse(value, out var seconds) ? Math.Max(1, seconds) : 180; |
There was a problem hiding this comment.
Magic numbers obscure intent and introduce maintenance risks when duplicated, such as 180 appearing as both an integer and a string literal. Extract named constants like DefaultStaleAfterSeconds and MinimumStaleAfterSeconds to clarify intent and centralize default values.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Core/Resgrid.Services/DepartmentSettingsService.cs:
Line 974:
Magic numbers obscure intent and introduce maintenance risks when duplicated, such as `180` appearing as both an integer and a string literal. Extract named constants like `DefaultStaleAfterSeconds` and `MinimumStaleAfterSeconds` to clarify intent and centralize default values.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| public async Task<int> GetHardwareTrackingStaleAfterSecondsAsync(int departmentId, bool bypassCache = false) | ||
| { | ||
| async Task<string> getSetting() |
There was a problem hiding this comment.
Naming convention violation identified on the local function getSetting. Rename it to GetSetting to comply with .NET PascalCase requirements for methods.
Kody rule violation: Use proper naming conventions
Prompt for LLM
File Core/Resgrid.Services/DepartmentSettingsService.cs:
Line 959:
Naming convention violation identified on the local function `getSetting`. Rename it to `GetSetting` to comply with .NET PascalCase requirements for methods.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var latestPerSource = locations | ||
| .Where(location => | ||
| location != null && | ||
| location.DepartmentId == departmentId && | ||
| location.IsValidFix != false) | ||
| .GroupBy(location => new | ||
| { | ||
| location.SourceType, | ||
| SourceId = location.SourceId ?? string.Empty | ||
| }) | ||
| .Select(group => group | ||
| .OrderByDescending(location => location.Timestamp) | ||
| .ThenByDescending(location => location.ReceivedOn ?? location.Timestamp) | ||
| .First()) | ||
| .ToList(); |
There was a problem hiding this comment.
Complex LINQ chains obscure hidden grouping and latest-selection logic, complicating readability, debugging, and testing. Break the 15-line expression into named intermediate steps to materialize the list through discrete filtering, grouping, and selection operations.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Core/Resgrid.Services/UnitLocationSourceResolver.cs:
Line 30 to 44:
Complex LINQ chains obscure hidden grouping and latest-selection logic, complicating readability, debugging, and testing. Break the 15-line expression into named intermediate steps to materialize the list through discrete filtering, grouping, and selection operations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return Invalid(receivedOn, "The tracking binding is not enabled."); | ||
|
|
||
| var device = source.Device; | ||
| var unit = await _unitsService.GetUnitByIdAsync(device.UnitId); |
There was a problem hiding this comment.
Unnecessary database queries occur because GetUnitByIdAsync executes before input validations complete. Move the positions.Count == 0 and batch-limit validations above the database call to allow invalid payloads to return early without a round-trip.
Kody rule violation: Order validations before database queries
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingIngressService.cs:
Line 57:
Unnecessary database queries occur because `GetUnitByIdAsync` executes before input validations complete. Move the `positions.Count == 0` and batch-limit validations above the database call to allow invalid payloads to return early without a round-trip.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var oldCacheIdentity = CopyCacheIdentity(existing); | ||
| var now = DateTime.UtcNow; | ||
|
|
||
| _unitOfWork.CreateOrGetConnection(); |
There was a problem hiding this comment.
Thread starvation risk identified due to synchronous connection-creation calls blocking execution inside an async method. Replace _unitOfWork.CreateOrGetConnection() with its async equivalent, such as await _unitOfWork.CreateOrGetConnectionAsync(), to improve throughput.
Kody rule violation: Use Awaitable Methods in Async Code
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 202:
Thread starvation risk identified due to synchronous connection-creation calls blocking execution inside an async method. Replace `_unitOfWork.CreateOrGetConnection()` with its async equivalent, such as `await _unitOfWork.CreateOrGetConnectionAsync()`, to improve throughput.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _devicesRepository.InsertAsync(replacement, cancellationToken); | ||
| _unitOfWork.CommitChanges(); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Silent transaction failures hinder production diagnostics because the catch block rolls back and rethrows without structured logging. Catch a typed exception, log it with operational context including deviceId and departmentId, and then rethrow.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 218:
Silent transaction failures hinder production diagnostics because the catch block rolls back and rethrows without structured logging. Catch a typed exception, log it with operational context including `deviceId` and `departmentId`, and then rethrow.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _devicesRepository.InsertAsync(replacement, cancellationToken); | ||
| _unitOfWork.CommitChanges(); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Indiscriminate exception handling masks transient database errors like timeouts and deadlocks, preventing retry policies from executing. Catch DbUpdateException to inspect for transient error codes and conditionally retry safe, idempotent operations.
Kody rule violation: Implement proper database error checking
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 218:
Indiscriminate exception handling masks transient database errors like timeouts and deadlocks, preventing retry policies from executing. Catch `DbUpdateException` to inspect for transient error codes and conditionally retry safe, idempotent operations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| _eventAggregator.SendMessage(new AuditEvent | ||
| { | ||
| DepartmentId = departmentId, | ||
| UserId = userId, | ||
| Type = type, | ||
| Before = before == null ? null : JsonConvert.SerializeObject(before), | ||
| After = after == null ? null : JsonConvert.SerializeObject(after), | ||
| Successful = true, | ||
| ServerName = Environment.MachineName | ||
| }); |
There was a problem hiding this comment.
Incomplete audit trail fails security and compliance requirements due to missing tamper-evident fields. Populate timestamp, actor.role, resource.id, trace_id, ip, and user_agent in the AuditEvent to ensure immutable, verifiable logging.
Kody rule violation: Emit tamper-evident audit logs with required fields
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 495 to 504:
Incomplete audit trail fails security and compliance requirements due to missing tamper-evident fields. Populate `timestamp`, `actor.role`, `resource.id`, `trace_id`, `ip`, and `user_agent` in the `AuditEvent` to ensure immutable, verifiable logging.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var credential in credentials.Where(item => !item.RevokedOn.HasValue)) | ||
| { | ||
| credential.RevokedOn = revokedOn; | ||
| await _credentialsRepository.UpdateAsync(credential, cancellationToken); |
There was a problem hiding this comment.
N+1 query performance degradation identified due to issuing database updates per credential inside a foreach loop. Batch the update into a single bulk operation or use ExecuteUpdateAsync to minimize database round-trips.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 472:
N+1 query performance degradation identified due to issuing database updates per credential inside a foreach loop. Batch the update into a single bulk operation or use `ExecuteUpdateAsync` to minimize database round-trips.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var unitLocation = ObjectSerialization.Deserialize<UnitLocationEvent>(message) | ||
| ?? throw new InvalidOperationException("Unit location queue message could not be deserialized."); | ||
|
|
||
| await UnitLocationEventQueueReceived.Invoke(unitLocation); |
There was a problem hiding this comment.
NullReferenceException vulnerability identified where the public UnitLocationEventQueueReceived delegate is invoked without a null check. Capture the delegate into a local variable and verify it is non-null before invoking it to prevent runtime crashes.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs:
Line 660:
NullReferenceException vulnerability identified where the public `UnitLocationEventQueueReceived` delegate is invoked without a null check. Capture the delegate into a local variable and verify it is non-null before invoking it to prevent runtime crashes.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var listeners = _asyncListeners.GetOrAdd( | ||
| typeof(T), | ||
| _ => new ConcurrentDictionary<Guid, Func<object, Task>>()); | ||
| listeners[token] = message => listener((T)message); |
There was a problem hiding this comment.
InvalidCastException risk identified due to a direct cast on an object parameter without type verification. Replace the direct cast with pattern matching or the as operator to ensure safe type evaluation before listener invocation.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Providers/Resgrid.Providers.Bus/EventAggregator.cs:
Line 48:
InvalidCastException risk identified due to a direct cast on an object parameter without type verification. Replace the direct cast with pattern matching or the `as` operator to ensure safe type evaluation before listener invocation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message)) | ||
| throw new InvalidOperationException("Unable to publish the Unit location realtime update."); |
There was a problem hiding this comment.
Transient RabbitMQ realtime-publish failures in unitLocationUpdatedTopicHandler now propagate through UnitLocationQueueLogic.ProcessUnitLocationQueueItem into the queue consumer's retry path, requeuing already-persisted messages and causing duplicate inserts. Swallow or log the publish failure in the handler instead of throwing, or wrap the SendMessageAsync call in AddUnitLocationAsync with a try/catch to prevent realtime side-effects from failing the committed primary persistence operation.
if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message))
Logging.LogError("Unable to publish the Unit location realtime update for department {DepartmentId}.", message.DepartmentId);Prompt for LLM
File Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs:
Line 618 to 619:
Transient RabbitMQ realtime-publish failures in `unitLocationUpdatedTopicHandler` now propagate through `UnitLocationQueueLogic.ProcessUnitLocationQueueItem` into the queue consumer's retry path, requeuing already-persisted messages and causing duplicate inserts. Swallow or log the publish failure in the handler instead of throwing, or wrap the `SendMessageAsync` call in `AddUnitLocationAsync` with a try/catch to prevent realtime side-effects from failing the committed primary persistence operation.
Suggested Code:
if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message))
Logging.LogError("Unable to publish the Unit location realtime update for department {DepartmentId}.", message.DepartmentId);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .WithColumn("headername").AsCustom("citext").Nullable() | ||
| .WithColumn("basicusername").AsCustom("citext").Nullable() | ||
| .WithColumn("keyprefix").AsCustom("citext").NotNullable() | ||
| .WithColumn("secrethash").AsCustom("citext").NotNullable() |
There was a problem hiding this comment.
Case-insensitive text declaration on the secrethash column breaks cryptographic hash comparisons and authentication logic. Use AsCustom("text") instead of AsCustom("citext") to enforce exact byte-level hash equality.
Kody rule violation: Optimize string column types
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.cs:
Line 90:
Case-insensitive text declaration on the `secrethash` column breaks cryptographic hash comparisons and authentication logic. Use `AsCustom("text")` instead of `AsCustom("citext")` to enforce exact byte-level hash equality.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| CREATE UNIQUE INDEX IF NOT EXISTS ux_unitlocations_eventid | ||
| ON public.unitlocations (eventid) | ||
| WHERE eventid IS NOT NULL; |
There was a problem hiding this comment.
Database locking risk identified because creating a unique index without CONCURRENTLY acquires an ACCESS EXCLUSIVE lock on the unitlocations table. Use CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS to prevent blocking reads and writes during index creation on large tables.
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Sql/EF0003_PopulateDocDb.sql:
Line 30 to 32:
Database locking risk identified because creating a unique index without `CONCURRENTLY` acquires an `ACCESS EXCLUSIVE` lock on the `unitlocations` table. Use `CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS` to prevent blocking reads and writes during index creation on large tables.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var query = _queryFactory.GetQuery<SelectUnitTrackingDevicesByUnitQuery>(); | ||
|
|
||
| return await WithConnectionAsync(connection => | ||
| connection.QueryAsync<UnitTrackingDevice>(query, parameters, _unitOfWork.Transaction)); |
There was a problem hiding this comment.
NullReferenceException vulnerability identified by accessing _unitOfWork.Transaction without a null guard inside a Dapper lambda. Capture the transaction into a local variable using _unitOfWork?.Transaction before passing it to the lambda to align with existing null-safe fallback paths.
Kody rule violation: Add null checks before accessing properties
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/UnitTrackingDevicesRepository.cs:
Line 44:
NullReferenceException vulnerability identified by accessing `_unitOfWork.Transaction` without a null guard inside a Dapper lambda. Capture the transaction into a local variable using `_unitOfWork?.Transaction` before passing it to the lambda to align with existing null-safe fallback paths.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <ProjectReference Include="..\..\Providers\Resgrid.Providers.Messaging\Resgrid.Providers.Messaging.csproj" /> | ||
| <ProjectReference Include="..\..\Repositories\Resgrid.Repositories.DataRepository\Resgrid.Repositories.DataRepository.csproj" /> | ||
| <ProjectReference Include="..\..\Repositories\Resgrid.Repositories.NoSqlRepository\Resgrid.Repositories.NoSqlRepository.csproj" /> | ||
| <ProjectReference Include="..\..\Web\Resgrid.Web\Resgrid.Web.csproj" /> |
There was a problem hiding this comment.
Architecture boundary violation identified by coupling the general test project directly to the presentation layer (Resgrid.Web). Move the tested logic into a service or domain object, or relocate web-layer integration tests to a dedicated assembly to maintain proper separation of concerns.
Kody rule violation: Enforce architecture boundaries and layering rules
Prompt for LLM
File Tests/Resgrid.Tests/Resgrid.Tests.csproj:
Line 58:
Architecture boundary violation identified by coupling the general test project directly to the presentation layer (`Resgrid.Web`). Move the tested logic into a service or domain object, or relocate web-layer integration tests to a dedicated assembly to maintain proper separation of concerns.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Assert | ||
| profiles.Select(profile => profile.Key) | ||
| .Should().BeEquivalentTo("generic-https", "traccar-forwarder"); |
There was a problem hiding this comment.
Inline magic strings representing a closed set of profile keys are brittle and resist refactoring. Define a static class or enum, such as UnitTrackingProfileKeys, to reference named members instead of raw literals.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Tests/Resgrid.Tests/Services/UnitTrackingCatalogServiceTests.cs:
Line 24:
Inline magic strings representing a closed set of profile keys are brittle and resist refactoring. Define a static class or enum, such as `UnitTrackingProfileKeys`, to reference named members instead of raw literals.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (authentication.Status == UnitTrackingHttpAuthenticationStatus.NotFound) | ||
| return UnknownEndpointResponse(); | ||
| if (authentication.Status != UnitTrackingHttpAuthenticationStatus.Authenticated) | ||
| return Unauthorized(); | ||
|
|
||
| return await AcceptAsync(authentication.Source); |
There was a problem hiding this comment.
Unthrottled failed-authentication requests against known device endpoints allow attackers to drive unbounded DB queries via GetBySecretHashAsync and brute-force credentials, because PostPositions returns Unauthorized() at line 78 without consuming any rate-limit counter. Apply a rate-limit check, such as CheckUnknownEndpoint keyed on RemoteIpAddress or CheckRequest keyed on deviceId, before evaluating the authentication outcome to mirror the PostCapability path.
if (authentication.Status == UnitTrackingHttpAuthenticationStatus.NotFound)
return UnknownEndpointResponse();
if (authentication.Status != UnitTrackingHttpAuthenticationStatus.Authenticated)
{
var failedLimit = _rateLimiter.CheckUnknownEndpoint(
HttpContext.Connection.RemoteIpAddress?.ToString());
return failedLimit.Allowed ? Unauthorized() : RateLimited(failedLimit);
}
return await AcceptAsync(authentication.Source);Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs:
Line 75 to 80:
Unthrottled failed-authentication requests against known device endpoints allow attackers to drive unbounded DB queries via `GetBySecretHashAsync` and brute-force credentials, because `PostPositions` returns `Unauthorized()` at line 78 without consuming any rate-limit counter. Apply a rate-limit check, such as `CheckUnknownEndpoint` keyed on `RemoteIpAddress` or `CheckRequest` keyed on `deviceId`, before evaluating the authentication outcome to mirror the `PostCapability` path.
Suggested Code:
if (authentication.Status == UnitTrackingHttpAuthenticationStatus.NotFound)
return UnknownEndpointResponse();
if (authentication.Status != UnitTrackingHttpAuthenticationStatus.Authenticated)
{
var failedLimit = _rateLimiter.CheckUnknownEndpoint(
HttpContext.Connection.RemoteIpAddress?.ToString());
return failedLimit.Allowed ? Unauthorized() : RateLimited(failedLimit);
}
return await AcceptAsync(authentication.Source);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var parsed = await _payloadParser.ParseAsync( | ||
| Request, | ||
| receivedOn, | ||
| HttpContext.RequestAborted); |
There was a problem hiding this comment.
Unhandled I/O exceptions from _payloadParser.ParseAsync bubble up to ASP.NET's default handler, producing contextless 500 errors unlike other awaited calls in the controller. Wrap the call in a try/catch block, rethrowing OperationCanceledException and returning a contextual 503 response for general exceptions.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs:
Line 140 to 143:
Unhandled I/O exceptions from `_payloadParser.ParseAsync` bubble up to ASP.NET's default handler, producing contextless 500 errors unlike other awaited calls in the controller. Wrap the call in a try/catch block, rethrowing `OperationCanceledException` and returning a contextual 503 response for general exceptions.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var parsed = await _payloadParser.ParseAsync( | ||
| Request, | ||
| receivedOn, | ||
| HttpContext.RequestAborted); |
There was a problem hiding this comment.
Unhandled exceptions from external I/O operations in ParseAsync yield raw 500 errors without operational context. Wrap the call in a try/catch block to map exceptions to appropriate HTTP status codes, such as 422 for parse failures and 503 for infrastructure errors, while including relevant diagnostic context.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs:
Line 140 to 143:
Unhandled exceptions from external I/O operations in `ParseAsync` yield raw 500 errors without operational context. Wrap the call in a try/catch block to map exceptions to appropriate HTTP status codes, such as 422 for parse failures and 503 for infrastructure errors, while including relevant diagnostic context.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| private IActionResult Unavailable(Exception exception, string message) | ||
| { | ||
| Logging.LogException(exception, message); |
There was a problem hiding this comment.
Unstructured error logs lacking operation names, device IDs, and trace IDs prevent effective correlation and diagnostics across services. Include relevant identifiers and context as structured parameters alongside the exception in Logging.LogException calls.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs:
Line 227:
Unstructured error logs lacking operation names, device IDs, and trace IDs prevent effective correlation and diagnostics across services. Include relevant identifiers and context as structured parameters alongside the exception in `Logging.LogException` calls.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| function updateProfileHelp() { | ||
| const option = select.options[select.selectedIndex]; | ||
| summary.textContent = option && option.dataset.summary | ||
| ? option.dataset.summary + " " + (option.dataset.retry || "") |
There was a problem hiding this comment.
Error-prone string concatenation utilized for building profile help text. Use template literals to improve readability and safely interpolate dataset properties.
Kody rule violation: Use Template Literals Instead of String Concatenation
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtml:
Line 86:
Error-prone string concatenation utilized for building profile help text. Use template literals to improve readability and safely interpolate dataset properties.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| var currentUserId = ClaimsAuthorizationHelper.GetUserId(); | ||
| var isDepartmentAdmin = ClaimsAuthorizationHelper.IsUserDepartmentAdmin(); | ||
|
|
||
| if (type == null || type.Value == (int)ImageTypes.Avatar) |
| var currentUserId = ClaimsAuthorizationHelper.GetUserId(); | ||
| var isDepartmentAdmin = ClaimsAuthorizationHelper.IsUserDepartmentAdmin(); | ||
|
|
||
| if (type == null || type.Value == (int)ImageTypes.Avatar) |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs (1)
613-620: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPublish failures that throw (rather than return
false) can still bubble into the retry path.The fix logs instead of throwing when
UnitLocationUpdatedChangedreturnsfalse, addressing part of the earlier duplicate-insert concern. But if the call throws (e.g. a transient RabbitMQ I/O error) rather than returningfalse, the exception isn't caught here, and since noonErrorcallback is passed toAddAsyncListener(line 61), it propagates back into whatever awaits this handler — recreating the original risk of requeuing an already-persisted location.unitStatusHandlerin this same file (lines 83-90) already wraps its downstream call in try/catch for exactly this reason.🔧 Wrap the publish call defensively
public Func<UnitLocationUpdatedEvent, Task> unitLocationUpdatedTopicHandler = async delegate (UnitLocationUpdatedEvent message) { if (_rabbitTopicProvider == null) _rabbitTopicProvider = new RabbitTopicProvider(); - if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message)) - Framework.Logging.LogError($"Unable to publish the Unit location realtime update for department {message.DepartmentId}."); + try + { + if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message)) + Framework.Logging.LogError($"Unable to publish the Unit location realtime update for department {message.DepartmentId}."); + } + catch (Exception ex) + { + Framework.Logging.LogException(ex, $"Unable to publish the Unit location realtime update for department {message.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 `@Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs` around lines 613 - 620, Wrap the await of _rabbitTopicProvider.UnitLocationUpdatedChanged in unitLocationUpdatedTopicHandler with try/catch, logging thrown publish failures through Framework.Logging.LogError and preventing them from propagating into the retry path. Preserve the existing false-result logging behavior.Core/Resgrid.Services/UnitTrackingService.cs (1)
288-357: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThread-starvation and silent-catch fixes weren't applied to
RotateCredentialAsync/PersistDisabledOrDeletedDeviceAsync.
RebindDeviceAsyncin this same diff now awaitsCreateOrGetConnectionAsyncand logs the exception in its catch block (Logging.LogException(ex, ...)), butRotateCredentialAsync(line 327) andPersistDisabledOrDeletedDeviceAsync(line 423) still call the blocking_unitOfWork.CreateOrGetConnection()inside anasyncmethod, and bothcatchblocks (335-339, 435-439) discard/rethrow with no logging at all — the same thread-starvation and silent-failure risk previously flagged, now fixed in one of three sites but not the other two.🔧 Align with the RebindDeviceAsync pattern
- _unitOfWork.CreateOrGetConnection(); + await _unitOfWork.CreateOrGetConnectionAsync(cancellationToken); try { ... } - catch + catch (Exception ex) { _unitOfWork.DiscardChanges(); + Logging.LogException(ex, $"... deviceId ..., departmentId {departmentId} ..."); throw; }Also applies to: 411-450
🤖 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/UnitTrackingService.cs` around lines 288 - 357, Update RotateCredentialAsync and PersistDisabledOrDeletedDeviceAsync to follow the RebindDeviceAsync pattern: await _unitOfWork.CreateOrGetConnectionAsync instead of calling the blocking connection method, and capture each exception in the catch block to log it via Logging.LogException(ex, ...), while retaining discard-and-rethrow behavior.Web/Resgrid.Web/Areas/User/Controllers/OrdersController.cs (1)
283-310: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce mutual-aid eligibility for all fulfillment paths.
FillandFillItemonly require authentication. UnlikeView, they never verify that the requested order/item belongs to the current department or is inGetOpenAvailableOrdersAsync(DepartmentId). An authenticated user can submit fills for private foreign-department items by supplying their IDs. Apply the same access check before renderingFilland beforeCreateFillAsync.🤖 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/Areas/User/Controllers/OrdersController.cs` around lines 283 - 310, Update OrdersController.Fill and FillItem to validate that the requested order/item belongs to the current department and is included in GetOpenAvailableOrdersAsync(DepartmentId) before rendering. Apply the same eligibility check in the POST fulfillment flow immediately before CreateFillAsync, rejecting unauthorized or unavailable resources while preserving the existing valid fulfillment behavior.
🧹 Nitpick comments (1)
Repositories/Resgrid.Repositories.DataRepository/Transactions/UnitOfWork.cs (1)
43-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAsync version fixes a semaphore leak the sync version still has.
CreateOrGetConnectionAsyncproperly wraps connection setup in try/finally so_semaphoreis always released. The syncCreateOrGetConnection(lines 26-41) has no such guard — ifConnection.Open()orBeginTransaction()throws,_semaphore.Release()is skipped and the semaphore permanently deadlocks all future callers on thisUnitOfWorkinstance.♻️ Apply the same try/finally to the sync path
public DbConnection CreateOrGetConnection() { _semaphore.Wait(); - - if (Connection == null) - { - Connection = _connectionProvider.Create(); - Connection.Open(); - - Transaction = Connection.BeginTransaction(); - } - - _semaphore.Release(); - - return Connection; + try + { + if (Connection == null) + { + Connection = _connectionProvider.Create(); + Connection.Open(); + + Transaction = Connection.BeginTransaction(); + } + + return Connection; + } + finally + { + _semaphore.Release(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/Transactions/UnitOfWork.cs` around lines 43 - 64, Update the synchronous CreateOrGetConnection method to wrap its connection initialization and return logic in a try/finally, releasing _semaphore in the finally block. Preserve the existing behavior while ensuring failures from Connection.Open or BeginTransaction cannot leave the semaphore held.
🤖 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 `@Repositories/Resgrid.Repositories.NoSqlRepository/MapLayersDocRepository.cs`:
- Around line 40-50: Update the lookup flow around mapLayersData in the
repository method so it checks whether the oid query contains a first result
before returning it. Only return the result when present; otherwise continue
through the existing numericId parsing and fallback QueryAsync lookup,
preserving the null return for invalid IDs.
In
`@Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/TraccarJsonPayloadAdapter.cs`:
- Around line 103-111: Update the coordinate validation in
TraccarJsonPayloadAdapter so present latitude values must be within [-90, 90]
and longitude values within [-180, 180]. Preserve the existing required-field
errors for missing values, and add validation errors for out-of-range
coordinates before constructing the canonical position.
In `@Web/Resgrid.Web.Services/Controllers/v4/AvatarsController.cs`:
- Around line 154-159: Update Crop to validate the request model and
model.imgUrl before constructing a Uri or entering the crop flow. Use
Uri.TryCreate for parsing, and return BadRequest() when the model is null,
imgUrl is missing, or the URI is malformed; preserve the existing originalId
extraction for valid input.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs`:
- Line 39: Replace the constructor-injected IDepartmentsService dependency in
UnitStatusController with
Bootstrapper.GetKernel().Resolve<IDepartmentsService>() inside the constructor,
and remove the corresponding field and parameter. Apply the same service-locator
resolution to the dependencies introduced at the referenced constructor
locations, preserving their existing usage.
In `@Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs`:
- Around line 247-258: State-changing endpoints are exposed as GET actions
without CSRF protection. In
Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs lines 247-258,
update NotificationsController.Delete to accept POST requests and add
ValidateAntiForgeryToken; submit it through an antiforgery-protected form. In
Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs lines 569-583 and
587-601, apply the same changes to the training deletion and reset actions,
respectively, including updating their callers to use protected POST forms.
In `@Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs`:
- Around line 550-557: Move the internal report credential from the URL query
parameter to a dedicated internal request header: in
ReportsController.InternalRunReport, read and validate the header value using
the existing fixed-time comparison, and in ReportDeliveryLogic send
InternalReportsToken through that header instead of AddParameter. Apply the
corresponding changes in
Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs lines 550-557 and
Workers/Resgrid.Workers.Framework/Logic/ReportDeliveryLogic.cs lines 41-44.
---
Outside diff comments:
In `@Core/Resgrid.Services/UnitTrackingService.cs`:
- Around line 288-357: Update RotateCredentialAsync and
PersistDisabledOrDeletedDeviceAsync to follow the RebindDeviceAsync pattern:
await _unitOfWork.CreateOrGetConnectionAsync instead of calling the blocking
connection method, and capture each exception in the catch block to log it via
Logging.LogException(ex, ...), while retaining discard-and-rethrow behavior.
In `@Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs`:
- Around line 613-620: Wrap the await of
_rabbitTopicProvider.UnitLocationUpdatedChanged in
unitLocationUpdatedTopicHandler with try/catch, logging thrown publish failures
through Framework.Logging.LogError and preventing them from propagating into the
retry path. Preserve the existing false-result logging behavior.
In `@Web/Resgrid.Web/Areas/User/Controllers/OrdersController.cs`:
- Around line 283-310: Update OrdersController.Fill and FillItem to validate
that the requested order/item belongs to the current department and is included
in GetOpenAvailableOrdersAsync(DepartmentId) before rendering. Apply the same
eligibility check in the POST fulfillment flow immediately before
CreateFillAsync, rejecting unauthorized or unavailable resources while
preserving the existing valid fulfillment behavior.
---
Nitpick comments:
In `@Repositories/Resgrid.Repositories.DataRepository/Transactions/UnitOfWork.cs`:
- Around line 43-64: Update the synchronous CreateOrGetConnection method to wrap
its connection initialization and return logic in a try/finally, releasing
_semaphore in the finally block. Preserve the existing behavior while ensuring
failures from Connection.Open or BeginTransaction cannot leave the semaphore
held.
🪄 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: d66858d1-9744-4603-a071-91b12727292f
⛔ Files ignored due to path filters (14)
Providers/Resgrid.Providers.Tracking/NOTICE.mdis excluded by!**/*.mdProviders/Resgrid.Providers.Tracking/PROVENANCE.mdis excluded by!**/*.mdTests/Resgrid.Tests/Data/UnitTracking/Fixtures/traccar/v6.14.5/README.mdis excluded by!**/Tests/**,!**/*.mdTests/Resgrid.Tests/Data/UnitTracking/Fixtures/traccar/v6.14.5/sinotrack-st901-h02-position.jsonis excluded by!**/Tests/**Tests/Resgrid.Tests/Framework/SentryTransactionFilterTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Mocks/MockUnitOfWork.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Resgrid.Tests.csprojis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingAuthenticationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingCatalogServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingIngressControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingJsonPayloadParserTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/User/UnitTrackingControllerTests.csis excluded by!**/Tests/**Web/Resgrid.Web/Areas/User/Apps/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
Core/Resgrid.Config/SecurityConfig.csCore/Resgrid.Config/ServiceBusConfig.csCore/Resgrid.Framework/SentryTransactionFilter.csCore/Resgrid.Model/Providers/IEventAggregator.csCore/Resgrid.Model/Repositories/IUnitTrackingCredentialsRepository.csCore/Resgrid.Model/Repositories/Queries/IUnitOfWork.csCore/Resgrid.Model/UnitLocationWriteResult.csCore/Resgrid.Services/UnitTrackingAuthenticationService.csCore/Resgrid.Services/UnitTrackingCatalogService.csCore/Resgrid.Services/UnitTrackingIngressService.csCore/Resgrid.Services/UnitTrackingService.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.csProviders/Resgrid.Providers.Bus/EventAggregator.csProviders/Resgrid.Providers.Bus/OutboundEventProvider.csProviders/Resgrid.Providers.Migrations/Migrations/M0101_AddUnitTracking.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/RevokeUnitTrackingCredentialsByDeviceQuery.csRepositories/Resgrid.Repositories.DataRepository/Transactions/UnitOfWork.csRepositories/Resgrid.Repositories.DataRepository/UnitTrackingCredentialsRepository.csRepositories/Resgrid.Repositories.DataRepository/UnitTrackingDevicesRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/MapLayersDocRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsMongoRepository.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/TraccarJsonPayloadAdapter.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingJsonPayloadParser.csWeb/Resgrid.Web.Services/Controllers/v4/AvatarsController.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelLocationController.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStaffingController.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitStatusController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/AllowedSourceCidrsAttribute.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/CalendarController.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Controllers/InventoryController.csWeb/Resgrid.Web/Areas/User/Controllers/MappingController.csWeb/Resgrid.Web/Areas/User/Controllers/MessagesController.csWeb/Resgrid.Web/Areas/User/Controllers/NotificationsController.csWeb/Resgrid.Web/Areas/User/Controllers/OrdersController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/ProfileController.csWeb/Resgrid.Web/Areas/User/Controllers/ProtocolsController.csWeb/Resgrid.Web/Areas/User/Controllers/ReportsController.csWeb/Resgrid.Web/Areas/User/Controllers/TemplatesController.csWeb/Resgrid.Web/Areas/User/Controllers/TrainingsController.csWeb/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.csWeb/Resgrid.Web/Areas/User/Controllers/WorkshiftsController.csWeb/Resgrid.Web/Areas/User/Views/Dispatch/CallExportEx.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtmlWeb/Resgrid.Web/wwwroot/js/ng/react-elements.cssWeb/Resgrid.Web/wwwroot/js/ng/react-elements.jsWorkers/Resgrid.Workers.Framework/Logic/ReportDeliveryLogic.csWorkers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs
🚧 Files skipped from review as they are similar to previous changes (21)
- Core/Resgrid.Model/Repositories/IUnitTrackingCredentialsRepository.cs
- Core/Resgrid.Model/UnitLocationWriteResult.cs
- Core/Resgrid.Model/Providers/IEventAggregator.cs
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.cs
- Core/Resgrid.Services/UnitTrackingCatalogService.cs
- Repositories/Resgrid.Repositories.DataRepository/UnitTrackingDevicesRepository.cs
- Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsMongoRepository.cs
- Providers/Resgrid.Providers.Migrations/Migrations/M0101_AddUnitTracking.cs
- Core/Resgrid.Framework/SentryTransactionFilter.cs
- Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingJsonPayloadParser.cs
- Providers/Resgrid.Providers.Bus/EventAggregator.cs
- Web/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtml
- Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs
- Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml
- Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs
- Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs
- Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs
- Core/Resgrid.Services/UnitTrackingIngressService.cs
- Core/Resgrid.Services/UnitTrackingAuthenticationService.cs
- Web/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.cs
- Repositories/Resgrid.Repositories.DataRepository/UnitTrackingCredentialsRepository.cs
| if (mapLayersData != null) | ||
| return mapLayersData.FirstOrDefault(); | ||
| else | ||
| { | ||
| var mapLayersData2 = await connection.QueryAsync<MapLayer>($"SELECT data FROM public.maplayers ul WHERE ul.id = {id};"); | ||
| if (!int.TryParse(id, out var numericId)) | ||
| return null; | ||
|
|
||
| var mapLayersData2 = await connection.QueryAsync<MapLayer>("SELECT data FROM public.maplayers ul WHERE ul.id = @numericId;", new { numericId }); | ||
|
|
||
| if (mapLayersData2 != null) | ||
| return mapLayersData2.FirstOrDefault(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For Dapper version 2.1.66, does QueryAsync<T>() return an empty enumerable rather than null when a query has no rows?
💡 Result:
Yes, in Dapper version 2.1.66, QueryAsync returns an empty IEnumerable when a query returns no rows, rather than null [1]. When a database query executes successfully but produces no result rows, Dapper returns an empty collection [1]. This is standard behavior for the library's Query methods. If your code expects a single object or null, using QueryAsync directly may lead to issues where you expect a null but receive an empty collection instead [1][2]. To handle scenarios where you expect a single result or null, it is recommended to use methods specifically designed for those patterns, such as QueryFirstOrDefaultAsync or QuerySingleOrDefaultAsync, which are designed to return the default value for the type (null for reference types) when no rows are found [1][2].
Citations:
- 1: https://stackoverflow.com/questions/57417854/dapper-query-multiple-blank-result-set
- 2: No exception, but select count(1) returns empty resultset DapperLib/Dapper#995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file =="
fd -a 'MapLayersDocRepository.cs' . || true
echo "== File excerpt =="
file="$(fd 'MapLayersDocRepository.cs' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
cat -n "$file" | sed -n '1,90p'
fi
echo "== Related usages/tests mention MapLayersDocRepository or MapLayerDocument == "
rg -n "MapLayersDocRepository|MapLayerDocument|GetByIdAsync|Select data from public.maplayers|public.maplayers" -S .Repository: Resgrid/Core
Length of output: 50368
Allow the numeric-ID fallback after an empty oid query.
Dapper.QueryAsync<T>() returns an empty IEnumerable<MapLayer> when no rows are found, so the mapLayersData != null branch returns null immediately and the numeric-ID lookup is unreachable. Check the first result explicitly before returning.
Proposed fix
- if (mapLayersData != null)
- return mapLayersData.FirstOrDefault();
- else
- ...
+ var mapLayer = mapLayersData.FirstOrDefault();
+ if (mapLayer != null)
+ return mapLayer;
+
+ if (!int.TryParse(id, out var numericId))
+ return null;
+
+ var mapLayersData2 = await connection.QueryAsync<MapLayer>("SELECT data FROM public.maplayers ul WHERE ul.id = `@numericId`;", new { numericId });
+ return mapLayersData2.FirstOrDefault();📝 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.
| if (mapLayersData != null) | |
| return mapLayersData.FirstOrDefault(); | |
| else | |
| { | |
| var mapLayersData2 = await connection.QueryAsync<MapLayer>($"SELECT data FROM public.maplayers ul WHERE ul.id = {id};"); | |
| if (!int.TryParse(id, out var numericId)) | |
| return null; | |
| var mapLayersData2 = await connection.QueryAsync<MapLayer>("SELECT data FROM public.maplayers ul WHERE ul.id = @numericId;", new { numericId }); | |
| if (mapLayersData2 != null) | |
| return mapLayersData2.FirstOrDefault(); | |
| var mapLayer = mapLayersData.FirstOrDefault(); | |
| if (mapLayer != null) | |
| return mapLayer; | |
| if (!int.TryParse(id, out var numericId)) | |
| return null; | |
| var mapLayersData2 = await connection.QueryAsync<MapLayer>("SELECT data FROM public.maplayers ul WHERE ul.id = `@numericId`;", new { numericId }); | |
| return mapLayersData2.FirstOrDefault(); |
🤖 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/MapLayersDocRepository.cs`
around lines 40 - 50, Update the lookup flow around mapLayersData in the
repository method so it checks whether the oid query contains a first result
before returning it. Only return the result when present; otherwise continue
through the existing numericId parsing and fallback QueryAsync lookup,
preserving the null return for invalid IDs.
| if (!input.Position.Latitude.HasValue) | ||
| errors.Add("position.latitude is required."); | ||
| if (!input.Position.Longitude.HasValue) | ||
| errors.Add("position.longitude is required."); | ||
| if (!FirstTimestamp( | ||
| input.Position.FixTime, | ||
| input.Position.DeviceTime, | ||
| input.Position.ServerTime).HasValue) | ||
| errors.Add("A valid position fixTime, deviceTime, or serverTime is required."); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject out-of-range coordinates.
Latitude and longitude are only checked for presence, so values such as 91 or 181 produce a successful canonical position. Validate latitude is within [-90, 90] and longitude within [-180, 180] before accepting the 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/ApplicationCore/UnitTracking/TraccarJsonPayloadAdapter.cs`
around lines 103 - 111, Update the coordinate validation in
TraccarJsonPayloadAdapter so present latitude values must be within [-90, 90]
and longitude values within [-180, 180]. Preserve the existing required-field
errors for missing values, and add validation errors for out-of-range
coordinates before constructing the canonical position.
| public async Task<ActionResult> Crop([FromBody] CropRequest model) | ||
| { | ||
| // extract original image ID and generate a new filename for the cropped result | ||
| var originalUri = new Uri(model.imgUrl); | ||
| var originalId = originalUri.Query.Replace("?id=", ""); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate imgUrl before constructing Uri.
A null or malformed model.imgUrl throws before the try block, producing an unhandled 500. Validate the model and use Uri.TryCreate, returning BadRequest() for invalid input.
🤖 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/AvatarsController.cs` around lines
154 - 159, Update Crop to validate the request model and model.imgUrl before
constructing a Uri or entering the crop flow. Use Uri.TryCreate for parsing, and
return BadRequest() when the model is null, imgUrl is missing, or the URI is
malformed; preserve the existing originalId extraction for valid input.
| private readonly IActionLogsService _actionLogsService; | ||
| private readonly IMappingService _mappingService; | ||
| private readonly IIncidentCommandService _incidentCommandService; | ||
| private readonly IDepartmentsService _departmentsService; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve the new dependency through Bootstrapper.
This adds constructor injection for IDepartmentsService, contrary to the repository requirement. As per coding guidelines, use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection.
Also applies to: 48-49, 59-59
🤖 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/UnitStatusController.cs` at line 39,
Replace the constructor-injected IDepartmentsService dependency in
UnitStatusController with
Bootstrapper.GetKernel().Resolve<IDepartmentsService>() inside the constructor,
and remove the corresponding field and parameter. Apply the same service-locator
resolution to the dependencies introduced at the referenced constructor
locations, preserving their existing usage.
Source: Coding guidelines
| [Authorize] | ||
| public async Task<IActionResult> Delete(int notificationId, CancellationToken cancellationToken) | ||
| { | ||
| if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return Unauthorized(); | ||
|
|
||
| var notifications = await _notificationService.GetNotificationsByDepartmentAsync(DepartmentId); | ||
|
|
||
| if (notifications == null || !notifications.Any(x => x.DepartmentNotificationId == notificationId)) | ||
| return Unauthorized(); | ||
|
|
||
| await _notificationService.DeleteDepartmentNotificationByIdAsync(notificationId, cancellationToken); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
State-changing GET endpoints permit CSRF. These authenticated actions mutate data without requiring an antiforgery token; move each to POST and submit it from an antiforgery-protected form.
Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs#L247-L258: make notification deletion POST-only and add[ValidateAntiForgeryToken].Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs#L569-L583: make training deletion POST-only and add[ValidateAntiForgeryToken].Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs#L587-L601: make training reset POST-only and add[ValidateAntiForgeryToken].
📍 Affects 2 files
Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs#L247-L258(this comment)Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs#L569-L583Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs#L587-L601
🤖 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/Areas/User/Controllers/NotificationsController.cs` around
lines 247 - 258, State-changing endpoints are exposed as GET actions without
CSRF protection. In
Web/Resgrid.Web/Areas/User/Controllers/NotificationsController.cs lines 247-258,
update NotificationsController.Delete to accept POST requests and add
ValidateAntiForgeryToken; submit it through an antiforgery-protected form. In
Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs lines 569-583 and
587-601, apply the same changes to the training deletion and reset actions,
respectively, including updating their callers to use protected POST forms.
| public async Task<IActionResult> InternalRunReport(int type, int departmentId, string key) | ||
| { | ||
| var expectedKey = Config.SecurityConfig.InternalReportsToken; | ||
|
|
||
| if (String.IsNullOrWhiteSpace(expectedKey) || String.IsNullOrWhiteSpace(key) || | ||
| !System.Security.Cryptography.CryptographicOperations.FixedTimeEquals( | ||
| Encoding.UTF8.GetBytes(key), Encoding.UTF8.GetBytes(expectedKey))) | ||
| return Unauthorized(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep InternalReportsToken out of request URLs.
The worker serializes this long-lived credential into a GET query string, and the anonymous endpoint accepts it there. URLs are commonly retained in proxy/access logs and telemetry, exposing a credential that grants report access. Move the token to an internal request header and bind/validate that header instead.
Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs#L550-L557: read the internal credential from a request header rather than thekeyquery parameter.Workers/Resgrid.Workers.Framework/Logic/ReportDeliveryLogic.cs#L41-L44: sendInternalReportsTokenas that request header rather thanAddParameter.
📍 Affects 2 files
Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs#L550-L557(this comment)Workers/Resgrid.Workers.Framework/Logic/ReportDeliveryLogic.cs#L41-L44
🤖 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/Areas/User/Controllers/ReportsController.cs` around lines 550
- 557, Move the internal report credential from the URL query parameter to a
dedicated internal request header: in ReportsController.InternalRunReport, read
and validate the header value using the existing fixed-time comparison, and in
ReportDeliveryLogic send InternalReportsToken through that header instead of
AddParameter. Apply the corresponding changes in
Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs lines 550-557 and
Workers/Resgrid.Workers.Framework/Logic/ReportDeliveryLogic.cs lines 41-44.
| public static string PaymentQueueName = "paymenttest"; | ||
| public static string AuditQueueName = "audittest"; | ||
| public static string UnitLoactionQueueName = "unitlocationtest"; | ||
| public static string UnitLocationQueueV2Name = "unitlocation-v2test"; |
There was a problem hiding this comment.
Implicit immutability violation in UnitLocationQueueV2Name where a compile-time string literal is declared as public static string. Rule [115] requires immutable compile-time constants to be marked const. Change the declaration to public const string.
Kody rule violation: Use `readonly` or `const` for Immutable Data
Prompt for LLM
File Core/Resgrid.Config/ServiceBusConfig.cs:
Line 18:
Implicit immutability violation in `UnitLocationQueueV2Name` where a compile-time string literal is declared as `public static string`. Rule [115] requires immutable compile-time constants to be marked `const`. Change the declaration to `public const string`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (transaction.Request != null) | ||
| transaction.Request.Url = RedactCapabilityPath(transaction.Request.Url); | ||
|
|
||
| ((IEventLike)transaction).TransactionName = RedactCapabilityPath(transaction.Name); |
There was a problem hiding this comment.
Unhandled InvalidCastException risk caused by a direct cast to (IEventLike)transaction without a safety guard. Use pattern matching, such as if (transaction is IEventLike eventLike), to safely guard against null or incompatible types.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Core/Resgrid.Framework/SentryTransactionFilter.cs:
Line 70:
Unhandled `InvalidCastException` risk caused by a direct cast to `(IEventLike)transaction` without a safety guard. Use pattern matching, such as `if (transaction is IEventLike eventLike)`, to safely guard against null or incompatible types.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return Invalid(receivedOn, "The position batch exceeds the configured limit."); | ||
| } | ||
|
|
||
| var unit = await _unitsService.GetUnitByIdAsync(device.UnitId); |
There was a problem hiding this comment.
Unhandled exception risk in _unitsService.GetUnitByIdAsync caused by invoking a database call without a try/catch block. Rule 28 requires wrapping external calls to provide failure context and map exceptions to application-level errors. Enclose the call in a try/catch block, log with context, and return a descriptive error result.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingIngressService.cs:
Line 70:
Unhandled exception risk in `_unitsService.GetUnitByIdAsync` caused by invoking a database call without a `try/catch` block. Rule 28 requires wrapping external calls to provide failure context and map exceptions to application-level errors. Enclose the call in a `try/catch` block, log with context, and return a descriptive error result.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return _hub.Subscribe<T>(listener); | ||
| } | ||
|
|
||
| public Guid AddAsyncListener<T>(Func<T, Task> listener, Action<Exception> onError = null) |
There was a problem hiding this comment.
Unobserved async exception vulnerability in AddAsyncListener caused by the onError parameter defaulting to null. Rule [4] mandates providing an error handler for listener API subscriptions. Remove the default value to make onError required, or supply a built-in default handler that logs exceptions.
Kody rule violation: Provide error handlers to subscription/listener APIs
Prompt for LLM
File Providers/Resgrid.Providers.Bus/EventAggregator.cs:
Line 39:
Unobserved async exception vulnerability in `AddAsyncListener` caused by the `onError` parameter defaulting to `null`. Rule [4] mandates providing an error handler for listener API subscriptions. Remove the default value to make `onError` required, or supply a built-in default handler that logs exceptions.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| _rabbitTopicProvider.UnitLocationUpdatedChanged(message); | ||
| if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message)) | ||
| Framework.Logging.LogError($"Unable to publish the Unit location realtime update for department {message.DepartmentId}."); |
There was a problem hiding this comment.
Unstructured logging violation in Framework.Logging.LogError caused by embedding the operation name and departmentId inside a string-interpolated message. Rule [3] requires passing relevant identifiers as structured fields. Pass structured parameters alongside the message to enable search and correlation.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs:
Line 619:
Unstructured logging violation in `Framework.Logging.LogError` caused by embedding the operation name and `departmentId` inside a string-interpolated message. Rule [3] requires passing relevant identifiers as structured fields. Pass structured parameters alongside the message to enable search and correlation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (!Schema.Table("units").Constraint("uq_units_departmentid_unitid").Exists()) | ||
| { | ||
| Create.UniqueConstraint("uq_units_departmentid_unitid") | ||
| .OnTable("units") | ||
| .Columns("departmentid", "unitid"); | ||
| } |
There was a problem hiding this comment.
Database lock contention and migration failure risk identified in Create.UniqueConstraint on the units table. PostgreSQL ALTER TABLE ... ADD CONSTRAINT ... UNIQUE (...) acquires a SHARE ROW EXCLUSIVE lock, blocks writes, and fails if duplicates exist. Pre-validate duplicates and use CREATE UNIQUE INDEX CONCURRENTLY to enforce uniqueness online.
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.cs:
Line 42 to 47:
Database lock contention and migration failure risk identified in `Create.UniqueConstraint` on the `units` table. PostgreSQL `ALTER TABLE ... ADD CONSTRAINT ... UNIQUE (...)` acquires a `SHARE ROW EXCLUSIVE` lock, blocks writes, and fails if duplicates exist. Pre-validate duplicates and use `CREATE UNIQUE INDEX CONCURRENTLY` to enforce uniqueness online.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Connection = _connectionProvider.Create(); | ||
| await Connection.OpenAsync(cancellationToken); | ||
|
|
||
| Transaction = Connection.BeginTransaction(); |
There was a problem hiding this comment.
Thread blocking violation in async method caused by the synchronous Connection.BeginTransaction() call. Rule [26] requires using awaitable methods to prevent blocking threads on database commands. Replace the call with await Connection.BeginTransactionAsync(cancellationToken).
Kody rule violation: Use Awaitable Methods in Async Code
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/Transactions/UnitOfWork.cs:
Line 54:
Thread blocking violation in async method caused by the synchronous `Connection.BeginTransaction()` call. Rule [26] requires using awaitable methods to prevent blocking threads on database commands. Replace the call with `await Connection.BeginTransactionAsync(cancellationToken)`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var query = _queryFactory.GetUpdateQuery<RevokeUnitTrackingCredentialsByDeviceQuery, UnitTrackingCredential>(null); | ||
|
|
||
| return await WithConnectionAsync(connection => | ||
| connection.ExecuteAsync(query, parameters, _unitOfWork.Transaction)); |
There was a problem hiding this comment.
Null reference exception risk caused by dereferencing _unitOfWork.Transaction without a null check. The _unitOfWork object can be null, as demonstrated by the null-conditional operator used on Connection. Use a null-conditional operator, such as _unitOfWork?.Transaction, to safely access the property.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/UnitTrackingCredentialsRepository.cs:
Line 83:
Null reference exception risk caused by dereferencing `_unitOfWork.Transaction` without a null check. The `_unitOfWork` object can be null, as demonstrated by the null-conditional operator used on `Connection`. Use a null-conditional operator, such as `_unitOfWork?.Transaction`, to safely access the property.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const string token = "rgtrk_prefix12_super-secret-capability"; | ||
| var transaction = new SentryTransaction( | ||
| $"POST /api/v4/unit-trackers/c/{token}", | ||
| "http.server") |
There was a problem hiding this comment.
Code duplication identified in Filter_404WithCapabilityPathOnlyInTransactionName_RedactsName where the token constant and SentryTransaction construction are repeated. Duplicated test arrange logic increases maintenance risks. Extract a helper method like CreateCapabilityTransaction(token, status) or hoist the token to a class-level constant.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Tests/Resgrid.Tests/Framework/SentryTransactionFilterTests.cs:
Line 102 to 105:
Code duplication identified in `Filter_404WithCapabilityPathOnlyInTransactionName_RedactsName` where the token constant and `SentryTransaction` construction are repeated. Duplicated test arrange logic increases maintenance risks. Extract a helper method like `CreateCapabilityTransaction(token, status)` or hoist the token to a class-level constant.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .IsSelectable.Should().BeTrue(); | ||
| profiles.Single(profile => profile.Key == "traccar-forwarder") | ||
| .IsSelectable.Should().BeFalse(); | ||
| profiles.Single(profile => profile.Key == "traccar-forwarder") |
There was a problem hiding this comment.
LINQ predicate duplication violates Rule [12] where profile => profile.Key == "traccar-forwarder" is repeated across multiple assertions. Repeated lookups bloat tests and risk divergence. Resolve the lookup once into a local variable, such as var traccar = profiles.Single(...), and assert against that reference.
Kody rule violation: Extract common query logic
Prompt for LLM
File Tests/Resgrid.Tests/Services/UnitTrackingCatalogServiceTests.cs:
Line 33:
LINQ predicate duplication violates Rule [12] where `profile => profile.Key == "traccar-forwarder"` is repeated across multiple assertions. Repeated lookups bloat tests and risk divergence. Resolve the lookup once into a local variable, such as `var traccar = profiles.Single(...)`, and assert against that reference.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| if (ModelState.IsValid) | ||
| { | ||
| if (item.CalendarItemId != 0 && !await _authorizationService.CanUserModifyCalendarEntryAsync(UserId, item.CalendarItemId)) |
There was a problem hiding this comment.
Magic number anti-pattern identified in item.CalendarItemId where the literal 0 acts as a sentinel for unpersisted items. Extract a named constant such as NewItemId or use item.CalendarItemId == default to clarify intent.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs:
Line 316:
Magic number anti-pattern identified in `item.CalendarItemId` where the literal `0` acts as a sentinel for unpersisted items. Extract a named constant such as `NewItemId` or use `item.CalendarItemId == default` to clarify intent.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return File(callAttachment.Data, "image/jpeg"); | ||
| } | ||
|
|
||
| private static bool IsValidCallImageToken(int callId, int attachmentId, string token) |
There was a problem hiding this comment.
Permanent token exposure risk in anonymous CallExportEx access caused by the GetCallImage capability token lacking an expiration or nonce. Leaked page URLs grant indefinite attachment retrieval. Include a short-lived expiry, such as {expEpochSeconds}, in the encrypted payload and reject expired tokens in IsValidCallImageToken.
var decrypted = SymmetricEncryption.Decrypt(decoded, Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase);
var parts = decrypted.Split('|');
if (parts.Length != 3 ||
!String.Equals(parts[0], callId.ToString(), StringComparison.Ordinal) ||
!String.Equals(parts[1], attachmentId.ToString(), StringComparison.Ordinal) ||
!long.TryParse(parts[2], out var exp) ||
exp < DateTimeOffset.UtcNow.ToUnixTimeSeconds())
return false;
return true;Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs:
Line 2720:
Permanent token exposure risk in anonymous `CallExportEx` access caused by the `GetCallImage` capability token lacking an expiration or nonce. Leaked page URLs grant indefinite attachment retrieval. Include a short-lived expiry, such as `{expEpochSeconds}`, in the encrypted payload and reject expired tokens in `IsValidCallImageToken`.
Suggested Code:
var decrypted = SymmetricEncryption.Decrypt(decoded, Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase);
var parts = decrypted.Split('|');
if (parts.Length != 3 ||
!String.Equals(parts[0], callId.ToString(), StringComparison.Ordinal) ||
!String.Equals(parts[1], attachmentId.ToString(), StringComparison.Ordinal) ||
!long.TryParse(parts[2], out var exp) ||
exp < DateTimeOffset.UtcNow.ToUnixTimeSeconds())
return false;
return true;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch | ||
| { | ||
| return false; |
There was a problem hiding this comment.
Silent exception swallowing in IsValidCallImageToken violates Rule 30, risking hidden configuration errors and crypto failures. Catch the exception into a variable and log it with structured context such as callId and attachmentId before returning false.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs:
Line 2732 to 2734:
Silent exception swallowing in `IsValidCallImageToken` violates Rule 30, risking hidden configuration errors and crypto failures. Catch the exception into a variable and log it with structured context such as `callId` and `attachmentId` before returning `false`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return File(callAttachment.Data, "image/jpeg"); | ||
| } | ||
|
|
||
| private static bool IsValidCallImageToken(int callId, int attachmentId, string token) |
There was a problem hiding this comment.
Permanent access vulnerability identified in the GetCallImage capability token caused by an absent expiration timestamp in the Base64(Encrypt("{callId}|{attachmentId}")) payload. Include an expiration timestamp, such as {expiryEpoch}, in the encrypted payload and reject expired tokens in IsValidCallImageToken.
var decrypted = SymmetricEncryption.Decrypt(decoded, Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase);
var parts = decrypted.Split('|');
if (parts.Length != 3) return false;
if (!long.TryParse(parts[0], out var tokenCallId) || tokenCallId != callId) return false;
if (!int.TryParse(parts[1], out var tokenAttachmentId) || tokenAttachmentId != attachmentId) return false;
if (!long.TryParse(parts[2], out var expiryEpoch) || expiryEpoch < DateTimeOffset.UtcNow.ToUnixTimeSeconds()) return false;
return true;Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs:
Line 2720:
Permanent access vulnerability identified in the `GetCallImage` capability token caused by an absent expiration timestamp in the `Base64(Encrypt("{callId}|{attachmentId}"))` payload. Include an expiration timestamp, such as `{expiryEpoch}`, in the encrypted payload and reject expired tokens in `IsValidCallImageToken`.
Suggested Code:
var decrypted = SymmetricEncryption.Decrypt(decoded, Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase);
var parts = decrypted.Split('|');
if (parts.Length != 3) return false;
if (!long.TryParse(parts[0], out var tokenCallId) || tokenCallId != callId) return false;
if (!int.TryParse(parts[1], out var tokenAttachmentId) || tokenAttachmentId != attachmentId) return false;
if (!long.TryParse(parts[2], out var expiryEpoch) || expiryEpoch < DateTimeOffset.UtcNow.ToUnixTimeSeconds()) return false;
return true;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [Authorize(Policy = ResgridResources.Department_View)] | ||
| public async Task<IActionResult> ResetGroupToStandingBy(int groupId) | ||
| { | ||
| var group = await _departmentGroupsService.GetGroupByIdAsync(groupId); |
There was a problem hiding this comment.
Unnecessary database query executed in GetGroupByIdAsync prior to the authorization validation on line 1067. The ClaimsAuthorizationHelper check does not depend on the query result and should evaluate first to fail fast. Move the authorization check before the GetGroupByIdAsync call.
Kody rule violation: Order validations before database queries
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:
Line 1062:
Unnecessary database query executed in `GetGroupByIdAsync` prior to the authorization validation on line 1067. The `ClaimsAuthorizationHelper` check does not depend on the query result and should evaluate first to fail fast. Move the authorization check before the `GetGroupByIdAsync` call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public async Task<IActionResult> ResetPasswordForUser(string userId) | ||
| { | ||
| if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return Unauthorized(); |
There was a problem hiding this comment.
Incorrect HTTP status code mapping identified in IsUserDepartmentAdmin() check where Unauthorized() returns HTTP 401 for authenticated users lacking permissions. The semantically correct status for an authorization failure is 403 Forbidden. Replace return Unauthorized() with return Forbid() to accurately reflect the permissions denial.
Kody rule violation: Use appropriate HTTP status codes
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs:
Line 955:
Incorrect HTTP status code mapping identified in `IsUserDepartmentAdmin()` check where `Unauthorized()` returns HTTP 401 for authenticated users lacking permissions. The semantically correct status for an authorization failure is 403 Forbidden. Replace `return Unauthorized()` with `return Forbid()` to accurately reflect the permissions denial.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [Authorize(Policy = ResgridResources.Protocol_View)] | ||
| public async Task<IActionResult> GetProtocol(int id) | ||
| { | ||
| if (!await _authorizationService.CanUserViewProtocolAsync(UserId, id)) |
There was a problem hiding this comment.
Unhandled exception risk in _authorizationService.CanUserViewProtocolAsync caused by an awaited operation lacking a try/catch block. Rule [1] requires guarding awaited operations to handle exceptions like database connectivity failures. Wrap the await in a try/catch block, log the error, and return a StatusCode(500) response.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/ProtocolsController.cs:
Line 212:
Unhandled exception risk in `_authorizationService.CanUserViewProtocolAsync` caused by an awaited operation lacking a `try/catch` block. Rule [1] requires guarding awaited operations to handle exceptions like database connectivity failures. Wrap the `await` in a `try/catch` block, log the error, and return a `StatusCode(500)` response.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| </tbody> | ||
| </table> | ||
| <img style="max-width:600px;" src="@Url.Action("GetCallImage", "Dispatch", new { Area = "User", callId = img.CallId, attachmentId = img.CallAttachmentId })" /> | ||
| <img style="max-width:600px;" src="@Url.Action("GetCallImage", "Dispatch", new { Area = "User", callId = img.CallId, attachmentId = img.CallAttachmentId, token = Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes(Resgrid.Framework.SymmetricEncryption.Encrypt($"{img.CallId}|{img.CallAttachmentId}", Resgrid.Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase))) })" /> |
There was a problem hiding this comment.
Accessibility and performance violation caused by the use of a plain <img> tag. Utilize the Next.js Image component with explicit width, height, and meaningful alt text for app assets.
Kody rule violation: Use next/image with explicit dimensions and alt
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Dispatch/CallExportEx.cshtml:
Line 548:
Accessibility and performance violation caused by the use of a plain `<img>` tag. Utilize the Next.js Image component with explicit width, height, and meaningful alt text for app assets.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| </tbody> | ||
| </table> | ||
| <img style="max-width:600px;" src="@Url.Action("GetCallImage", "Dispatch", new { Area = "User", callId = img.CallId, attachmentId = img.CallAttachmentId })" /> | ||
| <img style="max-width:600px;" src="@Url.Action("GetCallImage", "Dispatch", new { Area = "User", callId = img.CallId, attachmentId = img.CallAttachmentId, token = Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes(Resgrid.Framework.SymmetricEncryption.Encrypt($"{img.CallId}|{img.CallAttachmentId}", Resgrid.Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase))) })" /> |
There was a problem hiding this comment.
Fat UI anti-pattern identified in the Razor view caused by embedding complex cryptographic token generation logic inline. Rule 24 requires moving domain logic, such as SymmetricEncryption.Encrypt and Base64 encoding, to services or helpers. Create a helper method like LinkTokenService.Generate(img.CallId, img.CallAttachmentId) and reference it from the view.
Kody rule violation: Separate UI logic from business logic
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Dispatch/CallExportEx.cshtml:
Line 548:
Fat UI anti-pattern identified in the Razor view caused by embedding complex cryptographic token generation logic inline. Rule 24 requires moving domain logic, such as `SymmetricEncryption.Encrypt` and Base64 encoding, to services or helpers. Create a helper method like `LinkTokenService.Generate(img.CallId, img.CallAttachmentId)` and reference it from the view.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var request = new RestRequest("User/Reports/InternalRunReport", Method.Get); | ||
| request.AddParameter("type", item.ScheduledTask.Data); | ||
| request.AddParameter("departmentId", item.Department.DepartmentId); | ||
| request.AddParameter("key", Config.SecurityConfig.InternalReportsToken); |
There was a problem hiding this comment.
Credential exposure vulnerability caused by appending the internal reports token to the URL query string via AddParameter. Rule [44] prohibits secrets in query strings as they are captured in server and proxy logs. Transmit the credential in an HTTP header instead, using request.AddHeader("Authorization", $"Bearer {Config.SecurityConfig.InternalReportsToken}").
Kody rule violation: Mask PII and secrets in logs
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/ReportDeliveryLogic.cs:
Line 44:
Credential exposure vulnerability caused by appending the internal reports token to the URL query string via `AddParameter`. Rule [44] prohibits secrets in query strings as they are captured in server and proxy logs. Transmit the credential in an HTTP header instead, using `request.AddHeader("Authorization", $"Bearer {Config.SecurityConfig.InternalReportsToken}")`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var request = new RestRequest("User/Reports/InternalRunReport", Method.Get); | ||
| request.AddParameter("type", item.ScheduledTask.Data); | ||
| request.AddParameter("departmentId", item.Department.DepartmentId); | ||
| var request = new RestRequest("User/Reports/InternalRunReport", Method.Get); |
There was a problem hiding this comment.
Hardcoded route path in RestRequest violates Rule [7] requiring centralized route management. Define a constant for "User/Reports/InternalRunReport" in a shared constants class and reference it in the RestRequest instantiation.
Kody rule violation: Centralize string constants
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/ReportDeliveryLogic.cs:
Line 41:
Hardcoded route path in `RestRequest` violates Rule [7] requiring centralized route management. Define a constant for `"User/Reports/InternalRunReport"` in a shared constants class and reference it in the `RestRequest` instantiation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
3 participants
Description
This PR implements comprehensive hardware GPS unit tracking support (RG-T127), enabling departments to bind physical hardware trackers or forwarding services to Units and receive live position data through authenticated HTTPS endpoints.
Key additions
Device & credential management
UnitTrackingDevicesandUnitTrackingCredentialsdata models with full database migrations (SQL Server and PostgreSQL)no-storecaching headers; stored secrets are never returned by APIsPosition ingestion pipeline
/api/v4/unit-trackers/{id}/positionsand capability-path variant) accepting single or batched JSON position payloadsLocation resolution and status
UnitLocationSourceResolverthat selects the freshest, highest-priority location across hardware and mobile-app sources, with configurable staleness thresholds and mobile fallback behaviorUnitTrackingStatusServicecomputing effective device status (Online, Stale, Error, Disabled, NeverSeen)UnitsLocationandUnitLocationEventdocuments with source metadata and rich telemetry (satellites, HDOP, battery, ignition, alarm codes, etc.)Web UI and API
User/UnitTracking) with views for listing, creating, editing, and managing tracker bindings, credentials, and a JSON preview tool (non-production only)UnitTrackingDevicesController) for programmatic lifecycle management with authorization scoped to Unit view/update permissionsSecurity and observability
Infrastructure improvements
Inserted/Duplicatestatus to suppress duplicate realtime eventsSummary by CodeRabbit
New Features
Bug Fixes