Repository navigation
Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Warning Review limit reached
This review includes 191 billable files and costs up to $47.75. View limit details
|
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult> RecordDeviation([FromBody] RecordRouteDeviationInput input) |
| [Authorize(Policy = ResgridResources.Route_Update)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<GetRouteDirectionsResult>> GenerateRoute(string routePlanId) |
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<GetRouteDirectionsResult>> ApplyStopOrder(string routePlanId, [FromBody] ApplyStopOrderInput input) |
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<GetRouteResult>> GetRoute([FromBody] GetRouteInput input, CancellationToken cancellationToken) |
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_View)] | ||
| public async Task<ActionResult<ReportNavigationProgressResult>> ReportNavigationProgress([FromBody] ReportNavigationProgressInput input) |
| var night = string.Equals(variant, "night", StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| var departmentId = await ResolveMapResourceDepartmentAsync(MapResourceUrls.StyleScope(surface, night)); | ||
| if (departmentId == null) |
| [FromQuery] int? style, System.Threading.CancellationToken cancellationToken) | ||
| { | ||
| var departmentId = await ResolveMapResourceDepartmentAsync(MapResourceUrls.StaticMapScope); | ||
| if (departmentId == null) |
| public async Task<IActionResult> GetCustomMapMercatorTile(string layerId, int z, int x, int y) | ||
| { | ||
| var departmentId = await ResolveMapResourceDepartmentAsync(MapResourceUrls.CustomMapScope(layerId)); | ||
| if (departmentId == null) |
| [HttpPost("EstimateResponseArea")] | ||
| [Authorize(Policy = ResgridResources.Department_View)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<EstimateResponseAreaResult>> EstimateResponseArea([FromBody] EstimateResponseAreaInput input, CancellationToken cancellationToken) |
| [HttpPost("SignMapResourceUrls")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<SignMapResourceUrlsResult>> SignMapResourceUrls([FromBody] SignMapResourceUrlsInput input) |
| private static GeoJsonPolygonCoordinates<GeoJson2DGeographicCoordinates> ToPolygonCoordinates(GeoJSON.Net.Geometry.Polygon polygon) | ||
| { | ||
| var rings = polygon.Coordinates | ||
| .Select(ring => new GeoJsonLinearRingCoordinates<GeoJson2DGeographicCoordinates>(ring.Coordinates.Select(x => new GeoJson2DGeographicCoordinates(x.Longitude, x.Latitude)))) | ||
| .ToList(); | ||
|
|
||
| return new GeoJsonPolygonCoordinates<GeoJson2DGeographicCoordinates>(rings[0], rings.Skip(1)); | ||
| } |
There was a problem hiding this comment.
The polygon conversion assumes every parsed polygon contains at least one ring and indexes rings[0] without validation, so an empty or malformed persisted polygon throws ArgumentOutOfRangeException during layer hydration and aborts conversion of the entire map layer collection. Validate polygon, its coordinates, and the exterior ring before indexing, and leave the feature unset when the geometry is invalid.
if (polygon?.Coordinates == null || polygon.Coordinates.Count == 0 || polygon.Coordinates[0]?.Coordinates == null)
return null;
var rings = polygon.Coordinates.Select(...).ToList();
return new GeoJsonPolygonCoordinates<GeoJson2DGeographicCoordinates>(rings[0], rings.Skip(1));Prompt for LLM
File Core/Resgrid.Model/MapLayerData.cs:
Line 158 to 165:
The polygon conversion assumes every parsed polygon contains at least one ring and indexes `rings[0]` without validation, so an empty or malformed persisted polygon throws `ArgumentOutOfRangeException` during layer hydration and aborts conversion of the entire map layer collection. Validate `polygon`, its coordinates, and the exterior ring before indexing, and leave the feature unset when the geometry is invalid.
Suggested Code:
if (polygon?.Coordinates == null || polygon.Coordinates.Count == 0 || polygon.Coordinates[0]?.Coordinates == null)
return null;
var rings = polygon.Coordinates.Select(...).ToList();
return new GeoJsonPolygonCoordinates<GeoJson2DGeographicCoordinates>(rings[0], rings.Skip(1));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| Size = 1, | ||
| SlidingExpiration = TimeSpan.FromMinutes(30), | ||
| PostEvictionCallbacks = { new PostEvictionCallbackRegistration { EvictionCallback = (_, value, _, _) => (value as IDisposable)?.Dispose() } } |
There was a problem hiding this comment.
The inline EvictionCallback lambda in Core/Resgrid.Services/CustomMapMercatorTileRenderer.cs and the listed rgMaps.ts locations creates a new delegate for each construction and violates the team's restriction on inline callbacks in UI-related code. Move the callback to a named method or reusable delegate to avoid repeated allocations and improve maintainability.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File Core/Resgrid.Services/CustomMapMercatorTileRenderer.cs:
Line 133:
The inline `EvictionCallback` lambda in `Core/Resgrid.Services/CustomMapMercatorTileRenderer.cs` and the listed `rgMaps.ts` locations creates a new delegate for each construction and violates the team's restriction on inline callbacks in UI-related code. Move the callback to a named method or reusable delegate to avoid repeated allocations and improve maintainability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| private static string MapStyleNightCacheKey = "DSetMapStyleNight_{0}"; | ||
| private static string CallNumberingConfigCacheKey = "DSetCallNumbering_{0}"; | ||
| private static string DocumentNumberingConfigCacheKey = "DSetDocumentNumbering_{0}"; | ||
| private static string MappingNavigationConfigCacheKey = "DSetMappingNavigation_{0}"; |
There was a problem hiding this comment.
MappingNavigationConfigCacheKey in Core/Resgrid.Services/DepartmentSettingsService.cs is immutable but not marked readonly, allowing accidental reassignment. Mark it static readonly, or declare it const where appropriate.
Kody rule violation: Use `readonly` or `const` for Immutable Data
private static readonly string MappingNavigationConfigCacheKey = "DSetMappingNavigation_{0}";Prompt for LLM
File Core/Resgrid.Services/DepartmentSettingsService.cs:
Line 44:
`MappingNavigationConfigCacheKey` in `Core/Resgrid.Services/DepartmentSettingsService.cs` is immutable but not marked `readonly`, allowing accidental reassignment. Mark it `static readonly`, or declare it `const` where appropriate.
Suggested Code:
private static readonly string MappingNavigationConfigCacheKey = "DSetMappingNavigation_{0}";
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // The cached route may be older than the log; subtract the time the responder has already travelled. | ||
| var timeDiff = route.ComputedOn - log.Timestamp; | ||
| var time = route.DurationSeconds - timeDiff.Seconds; |
There was a problem hiding this comment.
Personnel ETA adjustment uses timeDiff.Seconds, which returns only the seconds component of the elapsed TimeSpan, so log ages of one minute or more subtract at most 59 seconds and leave returned ETAs materially too high, producing incorrect dispatch ranking. Use timeDiff.TotalSeconds.
var timeDiff = route.ComputedOn - log.Timestamp;
var time = route.DurationSeconds - timeDiff.TotalSeconds;Prompt for LLM
File Core/Resgrid.Services/GeoService.cs:
Line 84 to 86:
Personnel ETA adjustment uses `timeDiff.Seconds`, which returns only the seconds component of the elapsed `TimeSpan`, so log ages of one minute or more subtract at most 59 seconds and leave returned ETAs materially too high, producing incorrect dispatch ranking. Use `timeDiff.TotalSeconds`.
Suggested Code:
var timeDiff = route.ComputedOn - log.Timestamp;
var time = route.DurationSeconds - timeDiff.TotalSeconds;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| private static byte[] Key() | ||
| { | ||
| // Derived, so a map URL signature can never be replayed as any other signed value made with the passphrase. | ||
| return SHA256.HashData(Encoding.UTF8.GetBytes("resgrid-map-resources|" + (Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase ?? string.Empty))); | ||
| } |
There was a problem hiding this comment.
New map-resource signatures derive their HMAC key from an empty passphrase when ExternalLinkUrlParamPassphrase is unset, allowing anyone who knows the public signing formula to forge valid department, scope, and expiry signatures and request signed map resources without authorization. Fail closed when the passphrase is missing, refusing to issue or validate signed URLs instead of hashing an empty secret.
var passphrase = Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase;
if (string.IsNullOrWhiteSpace(passphrase))
throw new InvalidOperationException("ExternalLinkUrlParamPassphrase is required for map-resource signatures.");
return SHA256.HashData(Encoding.UTF8.GetBytes("resgrid-map-resources|" + passphrase));Prompt for LLM
File Core/Resgrid.Services/MapResourceUrls.cs:
Line 76 to 80:
New map-resource signatures derive their HMAC key from an empty passphrase when `ExternalLinkUrlParamPassphrase` is unset, allowing anyone who knows the public signing formula to forge valid department, scope, and expiry signatures and request signed map resources without authorization. Fail closed when the passphrase is missing, refusing to issue or validate signed URLs instead of hashing an empty secret.
Suggested Code:
var passphrase = Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase;
if (string.IsNullOrWhiteSpace(passphrase))
throw new InvalidOperationException("ExternalLinkUrlParamPassphrase is required for map-resource signatures.");
return SHA256.HashData(Encoding.UTF8.GetBytes("resgrid-map-resources|" + passphrase));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex, $"Map style for department {departmentId} ({appKey}) could not be built from {(info.TileProvider == MappingConfig.MapboxMapProvider ? "Mapbox" : "MappingConfig.SelfHostedStyleUrlDay/Night or SelfHostedStyleCatalog")}; serving the raster style."); |
There was a problem hiding this comment.
MapStyleService logs the raw appKey, exposing a potentially sensitive identifier in application logs. Redact or hash appKey and retain only non-sensitive structured identifiers such as departmentId and the provider.
Kody rule violation: Mask PII and secrets in logs
Logging.LogException(ex, "Map style build failed; serving raster style.", new { operation = "BuildMapStyle", departmentId, provider = info.TileProvider });Prompt for LLM
File Core/Resgrid.Services/MapStyleService.cs:
Line 105:
`MapStyleService` logs the raw `appKey`, exposing a potentially sensitive identifier in application logs. Redact or hash `appKey` and retain only non-sensitive structured identifiers such as `departmentId` and the provider.
Suggested Code:
Logging.LogException(ex, "Map style build failed; serving raster style.", new { operation = "BuildMapStyle", departmentId, provider = info.TileProvider });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| private const string MapboxApi = "https://api.mapbox.com"; | ||
|
|
||
| private static readonly Regex StyleUrl = new Regex(@"^mapbox://styles/([^/?#]+)/([^/?#]+)/?$", RegexOptions.IgnoreCase | RegexOptions.Compiled); |
There was a problem hiding this comment.
The regular expressions in Core/Resgrid.Services/MapboxStyleTransformer.cs, Providers/Resgrid.Providers.Geo/Routing/GoogleRoutingProvider.cs, and Tests/Resgrid.Tests/Web/AccountPageScriptsTests.cs do not specify a timeout, allowing processing of untrusted input to consume excessive resources in a denial-of-service attack. Define an appropriate regex timeout for each expression.
Kody rule violation: Specify Timeout for Regular Expressions
Prompt for LLM
File Core/Resgrid.Services/MapboxStyleTransformer.cs:
Line 20:
The regular expressions in `Core/Resgrid.Services/MapboxStyleTransformer.cs`, `Providers/Resgrid.Providers.Geo/Routing/GoogleRoutingProvider.cs`, and `Tests/Resgrid.Tests/Web/AccountPageScriptsTests.cs` do not specify a timeout, allowing processing of untrusted input to consume excessive resources in a denial-of-service attack. Define an appropriate regex timeout for each expression.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var sized = icons.Select(icon => | ||
| { | ||
| var scale = Math.Min(1.0, 48.0 / Math.Max(icon.Image.Width, icon.Image.Height)) * ratio; | ||
| var width = Math.Max(1, (int)Math.Round(icon.Image.Width * scale)); | ||
| var height = Math.Max(1, (int)Math.Round(icon.Image.Height * scale)); | ||
| return (icon.Name, icon.Image, Width: width, Height: height); | ||
| }).OrderByDescending(i => i.Height).ThenBy(i => i.Name, StringComparer.Ordinal).ToList(); |
There was a problem hiding this comment.
The DepartmentSpriteBuilder projection combines icon sizing, sorting, and materialization in one expression, obscuring the separate processing stages and making maintenance harder. Break the pipeline into named steps or helper methods such as CreateSizedIcon and an ordered materialization step.
Kody rule violation: Limit Lengthy LINQ Chains
var sizedIcons = icons.Select(CreateSizedIcon);
var orderedIcons = sizedIcons.OrderByDescending(i => i.Height).ThenBy(i => i.Name, StringComparer.Ordinal).ToList();Prompt for LLM
File Core/Resgrid.Services/Offline/DepartmentSpriteBuilder.cs:
Line 65 to 71:
The `DepartmentSpriteBuilder` projection combines icon sizing, sorting, and materialization in one expression, obscuring the separate processing stages and making maintenance harder. Break the pipeline into named steps or helper methods such as `CreateSizedIcon` and an ordered materialization step.
Suggested Code:
var sizedIcons = icons.Select(CreateSizedIcon);
var orderedIcons = sizedIcons.OrderByDescending(i => i.Height).ThenBy(i => i.Name, StringComparer.Ordinal).ToList();
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| else | ||
| { | ||
| var cell = pack.PackKey.Split(':')[1].Substring(1).Split('/'); | ||
| ranges = Pmtiles.CellRanges(int.Parse(cell[0], CultureInfo.InvariantCulture), ulong.Parse(cell[1], CultureInfo.InvariantCulture), |
There was a problem hiding this comment.
OfflineMapPackService.cs:992 and Providers/Resgrid.Providers.Geo/Offline/PmtilesArchive.cs:456 use int.Parse and ulong.Parse on cell input, so malformed user or I/O data can throw instead of being handled. Use TryParse with CultureInfo.InvariantCulture and validate the parsed values before calling Pmtiles.CellRanges.
Kody rule violation: Use TryParse for string conversions
Prompt for LLM
File Core/Resgrid.Services/OfflineMapPackService.cs:
Line 991:
`OfflineMapPackService.cs:992` and `Providers/Resgrid.Providers.Geo/Offline/PmtilesArchive.cs:456` use `int.Parse` and `ulong.Parse` on cell input, so malformed user or I/O data can throw instead of being handled. Use `TryParse` with `CultureInfo.InvariantCulture` and validate the parsed values before calling `Pmtiles.CellRanges`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public async Task<bool> TryAcquireAsync(PublicFormLimit form, string ipAddress, CancellationToken cancellationToken = default) | ||
| { | ||
| if (await IsLimitedAsync(form, ipAddress, cancellationToken)) | ||
| return false; | ||
|
|
||
| await RecordAsync(form, ipAddress, cancellationToken); | ||
| return true; |
There was a problem hiding this comment.
TryAcquireAsync performs the limit check and counter increment as separate operations, allowing concurrent requests to observe no refusal marker before any request records the cap-reaching increment and thereby exceed the configured per-address form limit. Use one atomic cache operation or script that increments and refuses based on the resulting count, or acquire a distributed lock around the check and increment.
// Atomically increment the fixed-window counter and return false when the resulting count exceeds the cap.
return await _cacheProvider.TryIncrementWithinLimitAsync(Key(form, ipAddress, "count"), LimitOf(form).Cap, LimitOf(form).Window, cancellationToken);Prompt for LLM
File Core/Resgrid.Services/PublicFormRateLimiter.cs:
Line 41 to 47:
`TryAcquireAsync` performs the limit check and counter increment as separate operations, allowing concurrent requests to observe no refusal marker before any request records the cap-reaching increment and thereby exceed the configured per-address form limit. Use one atomic cache operation or script that increments and refuses based on the resulting count, or acquire a distributed lock around the check and increment.
Suggested Code:
// Atomically increment the fixed-window counter and return false when the resulting count exceeds the cap.
return await _cacheProvider.TryIncrementWithinLimitAsync(Key(form, ipAddress, "count"), LimitOf(form).Cap, LimitOf(form).Window, cancellationToken);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var paused = await GetActiveOrPausedInstanceForUnitAsync(plan.DepartmentId, unitId); | ||
| if (paused != null) | ||
| throw new InvalidOperationException("Unit already has a paused route instance."); | ||
|
|
||
| var stops = await GetRouteStopsForPlanAsync(routePlanId); | ||
| var startedOn = DateTime.UtcNow; | ||
|
|
||
| var instance = new RouteInstance | ||
| { |
There was a problem hiding this comment.
StartRouteAsync checks for an active or paused route and then inserts a new instance without an atomic uniqueness guard, so two concurrent starts for the same unit can both pass the check and create simultaneous route instances, violating the one-active-route invariant. Enforce a database uniqueness constraint or atomic insert for active unit instances and handle the conflict, or serialize the check-and-create operation.
// Make the active-instance check and insert one atomic repository operation backed by a unique constraint.
var instance = await _routeInstancesRepository.TryStartIfNoActiveInstanceAsync(plan, unitId, startedByUserId, cancellationToken);
if (instance == null)
throw new InvalidOperationException("Unit already has an active route instance.");Prompt for LLM
File Core/Resgrid.Services/RouteService.cs:
Line 238 to 246:
`StartRouteAsync` checks for an active or paused route and then inserts a new instance without an atomic uniqueness guard, so two concurrent starts for the same unit can both pass the check and create simultaneous route instances, violating the one-active-route invariant. Enforce a database uniqueness constraint or atomic insert for active unit instances and handle the conflict, or serialize the check-and-create operation.
Suggested Code:
// Make the active-instance check and insert one atomic repository operation backed by a unique constraint.
var instance = await _routeInstancesRepository.TryStartIfNoActiveInstanceAsync(plan, unitId, startedByUserId, cancellationToken);
if (instance == null)
throw new InvalidOperationException("Unit already has an active route instance.");
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _routeInstanceStopsRepository.SaveOrUpdateAsync(new RouteInstanceStop | ||
| { | ||
| RouteInstanceId = instance.RouteInstanceId, | ||
| RouteStopId = stop.RouteStopId, | ||
| StopOrder = stop.StopOrder, | ||
| Status = 0, // Pending | ||
| AddedOn = DateTime.UtcNow | ||
| }, cancellationToken); | ||
|
|
||
| instance.StopsTotal += 1; | ||
| await _routeInstancesRepository.SaveOrUpdateAsync(instance, cancellationToken); |
There was a problem hiding this comment.
RouteService writes the instance stop and route instance in separate operations, so a failure between them can leave inconsistent route state. Wrap both writes in a transaction, commit only after both succeed, and roll back and rethrow on failure.
Kody rule violation: Handle transaction rollbacks properly
await using var transaction = await _routeInstancesRepository.BeginTransactionAsync(cancellationToken);
try
{
await _routeInstanceStopsRepository.SaveOrUpdateAsync(new RouteInstanceStop { ... }, cancellationToken);
await _routeInstancesRepository.SaveOrUpdateAsync(instance, cancellationToken);
await transaction.CommitAsync(cancellationToken);
}
catch
{
await transaction.RollbackAsync(cancellationToken);
throw;
}Prompt for LLM
File Core/Resgrid.Services/RouteService.cs:
Line 114 to 124:
`RouteService` writes the instance stop and route instance in separate operations, so a failure between them can leave inconsistent route state. Wrap both writes in a transaction, commit only after both succeed, and roll back and rethrow on failure.
Suggested Code:
await using var transaction = await _routeInstancesRepository.BeginTransactionAsync(cancellationToken);
try
{
await _routeInstanceStopsRepository.SaveOrUpdateAsync(new RouteInstanceStop { ... }, cancellationToken);
await _routeInstancesRepository.SaveOrUpdateAsync(instance, cancellationToken);
await transaction.CommitAsync(cancellationToken);
}
catch
{
await transaction.RollbackAsync(cancellationToken);
throw;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| private async Task AddToIndexAsync(int departmentId, int unitId) | ||
| { | ||
| var unitIds = await ReadIndexAsync(departmentId); | ||
| if (unitIds.Contains(unitId)) | ||
| { | ||
| // Keep the index alive as long as anyone navigates. | ||
| await WriteIndexAsync(departmentId, unitIds); | ||
| return; | ||
| } | ||
|
|
||
| unitIds.Add(unitId); | ||
| await WriteIndexAsync(departmentId, unitIds); |
There was a problem hiding this comment.
AddToIndexAsync reads, modifies, and rewrites the department navigation-unit index without an atomic update, so simultaneous reports from different units can read the same old index and the last writer can erase the other unit, causing GetActiveAsync to omit a currently navigating unit. Update the index with an atomic set-add operation or serialize the read-modify-write per department.
await _cacheProvider.AddToSetAsync(string.Format(IndexKey, departmentId), unitId, IndexLifetime);Prompt for LLM
File Core/Resgrid.Services/UnitNavigationStateService.cs:
Line 144 to 155:
`AddToIndexAsync` reads, modifies, and rewrites the department navigation-unit index without an atomic update, so simultaneous reports from different units can read the same old index and the last writer can erase the other unit, causing `GetActiveAsync` to omit a currently navigating unit. Update the index with an atomic set-add operation or serialize the read-modify-write per department.
Suggested Code:
await _cacheProvider.AddToSetAsync(string.Format(IndexKey, departmentId), unitId, IndexLifetime);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| foreach (var unitId in unitIds) | ||
| { | ||
| var state = await GetForUnitAsync(departmentId, unitId); |
There was a problem hiding this comment.
UnitNavigationStateService retrieves state with one cache query per unit, creating unnecessary latency and cache load as the unit count grows. Batch the retrieval with GetForUnitsAsync(departmentId, unitIds).
Kody rule violation: Optimize database queries with JOINs
var states = await GetForUnitsAsync(departmentId, unitIds);Prompt for LLM
File Core/Resgrid.Services/UnitNavigationStateService.cs:
Line 80:
`UnitNavigationStateService` retrieves state with one cache query per unit, creating unnecessary latency and cache load as the unit count grows. Batch the retrieval with `GetForUnitsAsync(departmentId, unitIds)`.
Suggested Code:
var states = await GetForUnitsAsync(departmentId, unitIds);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Id = Guid.NewGuid(), Type = (int)EventingTypes.UnitActiveRolesUpdated, TimeStamp = DateTime.UtcNow, DepartmentId = departmentId, ItemId = unitId.ToString() | ||
| }.SerializeJson()); | ||
|
|
||
| public Task<bool> UnitNavigationUpdated(int departmentId, int unitId, string payload) => SendMessage(Topics.EventingTopic, new EventingMessage |
There was a problem hiding this comment.
Providers/Resgrid.Providers.Bus.Rabbit.RabbitTopicProvider.UnitNavigationUpdated returns the external SendMessage task without contextual error handling, so message-bus failures propagate without identifying the operation or provider. Wrap the call in try/catch, await it inside the try block, and add contextual handling or rethrow an application-level exception.
Kody rule violation: Add try-catch blocks for external calls
public async Task<bool> UnitNavigationUpdated(int departmentId, int unitId, string payload)
{
try
{
return await SendMessage(Topics.EventingTopic, new EventingMessagePrompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs:
Line 99:
`Providers/Resgrid.Providers.Bus.Rabbit.RabbitTopicProvider.UnitNavigationUpdated` returns the external `SendMessage` task without contextual error handling, so message-bus failures propagate without identifying the operation or provider. Wrap the call in `try/catch`, await it inside the `try` block, and add contextual handling or rethrow an application-level exception.
Suggested Code:
public async Task<bool> UnitNavigationUpdated(int departmentId, int unitId, string payload)
{
try
{
return await SendMessage(Topics.EventingTopic, new EventingMessage
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .Distinct(); | ||
|
|
||
| return names | ||
| .Select(name => _geocoders.FirstOrDefault(g => g.Name == name)) |
There was a problem hiding this comment.
The FirstOrDefault call in Providers/Resgrid.Providers.Geo/GeoLocationProvider.cs can return a null provider from an empty or injected provider collection, making the subsequent g.Name access unsafe. Use a null check or optional access before dereferencing g.Name.
Kody rule violation: Add null checks to prevent NullReferenceException
.Select(name => _geocoders.FirstOrDefault(g => g?.Name == name))Prompt for LLM
File Providers/Resgrid.Providers.Geo/GeoLocationProvider.cs:
Line 63:
The `FirstOrDefault` call in `Providers/Resgrid.Providers.Geo/GeoLocationProvider.cs` can return a null provider from an empty or injected provider collection, making the subsequent `g.Name` access unsafe. Use a null check or optional access before dereferencing `g.Name`.
Suggested Code:
.Select(name => _geocoders.FirstOrDefault(g => g?.Name == name))
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| private static CancellationToken GeocodeTimeout() | ||
| { | ||
| return new CancellationTokenSource(TimeSpan.FromMilliseconds(Math.Max(1000, Config.MappingConfig.RoutingTimeoutMs))).Token; |
There was a problem hiding this comment.
GeoLocationProvider returns a token from a CancellationTokenSource that is never disposed, leaking the underlying timer resource. Dispose the CancellationTokenSource deterministically with using or await using.
Kody rule violation: Use using statements for disposable resources
using var timeoutSource = new CancellationTokenSource(TimeSpan.FromMilliseconds(Math.Max(MinimumTimeoutMilliseconds, Config.MappingConfig.RoutingTimeoutMs)));
return timeoutSource.Token;Prompt for LLM
File Providers/Resgrid.Providers.Geo/GeoLocationProvider.cs:
Line 70:
`GeoLocationProvider` returns a token from a `CancellationTokenSource` that is never disposed, leaking the underlying timer resource. Dispose the `CancellationTokenSource` deterministically with `using` or `await using`.
Suggested Code:
using var timeoutSource = new CancellationTokenSource(TimeSpan.FromMilliseconds(Math.Max(MinimumTimeoutMilliseconds, Config.MappingConfig.RoutingTimeoutMs)));
return timeoutSource.Token;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| ServiceURL = MappingConfig.OfflinePackS3Endpoint, | ||
| ForcePathStyle = true, | ||
| UseHttp = !MappingConfig.OfflinePackS3UseSsl, |
There was a problem hiding this comment.
OfflineMapPackStores derives UseHttp from MappingConfig.OfflinePackS3UseSsl, allowing plaintext HTTP for external storage endpoints and exposing credentials or map data to interception. Require TLS by disabling plaintext HTTP and validate the endpoint configuration.
Kody rule violation: Enforce TLS 1.2+ and HSTS on all external endpoints
UseHttp = false,Prompt for LLM
File Providers/Resgrid.Providers.Geo/Offline/OfflineMapPackStores.cs:
Line 214:
`OfflineMapPackStores` derives `UseHttp` from `MappingConfig.OfflinePackS3UseSsl`, allowing plaintext HTTP for external storage endpoints and exposing credentials or map data to interception. Require TLS by disabling plaintext HTTP and validate the endpoint configuration.
Suggested Code:
UseHttp = false,
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _lock.WaitAsync(cancellationToken); | ||
| try | ||
| { | ||
| _stream.Seek(offset, SeekOrigin.Begin); |
There was a problem hiding this comment.
Providers/Resgrid.Providers.Geo/Offline/PmtilesArchive.cs and the listed CustomMapMercatorTileRenderer.cs locations call synchronous _stream.Seek from an asynchronous read path, which may be incompatible with the target stream API and can block execution. Use an asynchronous positioning strategy supported by the stream API, or isolate synchronous positioning from the async read path.
Kody rule violation: Use Awaitable Methods in Async Code
Prompt for LLM
File Providers/Resgrid.Providers.Geo/Offline/PmtilesArchive.cs:
Line 83:
`Providers/Resgrid.Providers.Geo/Offline/PmtilesArchive.cs` and the listed `CustomMapMercatorTileRenderer.cs` locations call synchronous `_stream.Seek` from an asynchronous read path, which may be incompatible with the target stream API and can block execution. Use an asynchronous positioning strategy supported by the stream API, or isolate synchronous positioning from the async read path.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var route = await GetLegacyRouteAsync(new RouteRequest | ||
| { | ||
| Waypoints = new List<RoutingWaypoint> { request.Origins[i], request.Destinations[j] }, | ||
| Profile = request.Profile, | ||
| Avoid = request.Avoid | ||
| }, false); |
There was a problem hiding this comment.
GoogleRoutingProvider issues one GetLegacyRouteAsync network request for every origin-destination pair sequentially, creating an N+1 latency and throughput bottleneck. Batch the requests with an aggregate API or a bounded Task.WhenAll.
Kody rule violation: Detect N+1 style queries and suggest batching
var routes = await Task.WhenAll(request.Origins.SelectMany(origin => request.Destinations.Select(destination => GetLegacyRouteAsync(new RouteRequest { Waypoints = new List<RoutingWaypoint> { origin, destination }, Profile = request.Profile, Avoid = request.Avoid }, false))));Prompt for LLM
File Providers/Resgrid.Providers.Geo/Routing/GoogleRoutingProvider.cs:
Line 75 to 80:
`GoogleRoutingProvider` issues one `GetLegacyRouteAsync` network request for every origin-destination pair sequentially, creating an N+1 latency and throughput bottleneck. Batch the requests with an aggregate API or a bounded `Task.WhenAll`.
Suggested Code:
var routes = await Task.WhenAll(request.Origins.SelectMany(origin => request.Destinations.Select(destination => GetLegacyRouteAsync(new RouteRequest { Waypoints = new List<RoutingWaypoint> { origin, destination }, Profile = request.Profile, Avoid = request.Avoid }, false))));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| catch (Exception ex) | ||
| { | ||
| result.Error = ex.Message; |
There was a problem hiding this comment.
GoogleRoutingProvider assigns only ex.Message to result.Error, discarding the operation, provider, request identifier, and exception details needed to diagnose failures. Record the failure through a structured logger with those fields and return a non-sensitive application-level error.
Kody rule violation: Include error context in structured logs
logger.Error("Routes API request failed", new { operation = "computeRoutes", provider = Name, error = ex });
result.Error = "Routes API request failed";Prompt for LLM
File Providers/Resgrid.Providers.Geo/Routing/GoogleRoutingProvider.cs:
Line 329:
`GoogleRoutingProvider` assigns only `ex.Message` to `result.Error`, discarding the operation, provider, request identifier, and exception details needed to diagnose failures. Record the failure through a structured logger with those fields and return a non-sensitive application-level error.
Suggested Code:
logger.Error("Routes API request failed", new { operation = "computeRoutes", provider = Name, error = ex });
result.Error = "Routes API request failed";
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
|
|
||
| b = encoded[index++] - 63; | ||
| result |= (b & 0x1f) << shift; |
There was a problem hiding this comment.
PolylineCodec combines an unchecked shifted value into result, so malformed input can overflow the integer or use an invalid shift range. Validate the shift range and use checked arithmetic when applying (b & ValueMask) << shift.
Kody rule violation: Prevent Numeric Overflow in Calculations
result = checked(result | ((b & ValueMask) << shift));Prompt for LLM
File Providers/Resgrid.Providers.Geo/Routing/PolylineCodec.cs:
Line 87:
`PolylineCodec` combines an unchecked shifted value into `result`, so malformed input can overflow the integer or use an invalid shift range. Validate the shift range and use checked arithmetic when applying `(b & ValueMask) << shift`.
Suggested Code:
result = checked(result | ((b & ValueMask) << shift));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| Execute.Sql( | ||
| "IF NOT EXISTS (SELECT 1 FROM [FeatureFlags] WHERE [FlagKey] = '" + flag.Key + "') " + | ||
| "INSERT INTO [FeatureFlags] ([FlagKey], [Name], [Description], [Category], [IsEnabledGlobally]) " + |
There was a problem hiding this comment.
The SQL statements in Providers/Resgrid.Providers.Migrations/Migrations/M0308_AddMapLibreRoutingFoundation.cs and Providers/Resgrid.Providers.MigrationsPg/Migrations/M0308_AddMapLibreRoutingFoundationPg.cs use unsanitized values, creating SQL injection risk if any query input becomes externally controlled. Use parameterized queries for the affected statements.
Kody rule violation: Prevent SQL Injection in Queries
Prompt for LLM
File Providers/Resgrid.Providers.Migrations/Migrations/M0308_AddMapLibreRoutingFoundation.cs:
Line 82:
The SQL statements in `Providers/Resgrid.Providers.Migrations/Migrations/M0308_AddMapLibreRoutingFoundation.cs` and `Providers/Resgrid.Providers.MigrationsPg/Migrations/M0308_AddMapLibreRoutingFoundationPg.cs` use unsanitized values, creating SQL injection risk if any query input becomes externally controlled. Use parameterized queries for the affected statements.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .WithColumn("attempts").AsInt32().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("error").AsCustom("text").Nullable(); | ||
|
|
||
| Execute.Sql("CREATE UNIQUE INDEX IF NOT EXISTS ix_offlinemappacks_packkey ON offlinemappacks (packkey);"); |
There was a problem hiding this comment.
M0309_AddOfflineMapPacksPg.cs creates the unique offlinemappacks(packkey) index with a regular CREATE INDEX, which can lock the table and cause downtime during migration. Use an online, non-blocking strategy such as PostgreSQL CONCURRENTLY and document the rollback plan.
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Execute.Sql("CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS ix_offlinemappacks_packkey ON offlinemappacks (packkey);");Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0309_AddOfflineMapPacksPg.cs:
Line 35:
`M0309_AddOfflineMapPacksPg.cs` creates the unique `offlinemappacks(packkey)` index with a regular `CREATE INDEX`, which can lock the table and cause downtime during migration. Use an online, non-blocking strategy such as PostgreSQL `CONCURRENTLY` and document the rollback plan.
Suggested Code:
Execute.Sql("CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS ix_offlinemappacks_packkey ON offlinemappacks (packkey);");
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// </summary> | ||
| [HttpPut("UpdateRoutePlan")] | ||
| [Authorize(Policy = ResgridResources.Route_Update)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<SaveRoutePlanResult>> UpdateRoutePlan([FromBody] UpdateRoutePlanInput input) | ||
| { | ||
| var plan = await _routeService.GetRoutePlanByIdAsync(input.RoutePlanId); |
There was a problem hiding this comment.
RoutesController queries _routeService.GetRoutePlanByIdAsync before validating input and input.RoutePlanId, allowing null or blank identifiers to reach the database or service layer. Reject a null request or whitespace-only RoutePlanId with BadRequest() before issuing the query.
Kody rule violation: Order validations before database queries
if (input == null || string.IsNullOrWhiteSpace(input.RoutePlanId)) return BadRequest();
var plan = await _routeService.GetRoutePlanByIdAsync(input.RoutePlanId);Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs:
Line 307:
`RoutesController` queries `_routeService.GetRoutePlanByIdAsync` before validating `input` and `input.RoutePlanId`, allowing null or blank identifiers to reach the database or service layer. Reject a null request or whitespace-only `RoutePlanId` with `BadRequest()` before issuing the query.
Suggested Code:
if (input == null || string.IsNullOrWhiteSpace(input.RoutePlanId)) return BadRequest();
var plan = await _routeService.GetRoutePlanByIdAsync(input.RoutePlanId);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [HttpPost("ReportNavigationProgress")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_View)] | ||
| public async Task<ActionResult<ReportNavigationProgressResult>> ReportNavigationProgress([FromBody] ReportNavigationProgressInput input) | ||
| { | ||
| if (input == null || !Enum.IsDefined(typeof(UnitNavigationStates), input.State) || | ||
| input.RemainingSeconds < 0 || input.RemainingSeconds > MaxNavigationSeconds || input.RemainingMeters < 0 || | ||
| input.CallId.HasValue == !string.IsNullOrWhiteSpace(input.RouteInstanceId)) | ||
| return BadRequest(); | ||
|
|
||
| var unit = await _unitsService.GetUnitByIdAsync(input.UnitId); | ||
| if (unit == null || unit.DepartmentId != DepartmentId || !await _authorizationService.CanUserViewUnitAsync(UserId, unit.UnitId)) | ||
| return NotFound(); |
There was a problem hiding this comment.
ReportNavigationProgress permits any user with Unit_View and unit-view visibility to write navigation state for that unit, allowing a non-unit viewer to submit forged ETA, remaining distance, state, and provider values that control dispatch and Big Board navigation status. Require a unit-owned reporting credential or session, or route/unit update authorization, and verify that the reporter is the unit's authenticated principal before calling ReportAsync.
[Authorize(Policy = ResgridResources.Unit_Update)]
...
if (unit == null || unit.DepartmentId != DepartmentId || !await _authorizationService.CanUserModifyUnitAsync(UserId, unit.UnitId))
return NotFound();
// Additionally require the authenticated unit/app session to match unit.UnitId before persisting progress.Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/RoutingController.cs:
Line 212 to 226:
`ReportNavigationProgress` permits any user with `Unit_View` and unit-view visibility to write navigation state for that unit, allowing a non-unit viewer to submit forged ETA, remaining distance, state, and provider values that control dispatch and Big Board navigation status. Require a unit-owned reporting credential or session, or route/unit update authorization, and verify that the reporter is the unit's authenticated principal before calling `ReportAsync`.
Suggested Code:
[Authorize(Policy = ResgridResources.Unit_Update)]
...
if (unit == null || unit.DepartmentId != DepartmentId || !await _authorizationService.CanUserModifyUnitAsync(UserId, unit.UnitId))
return NotFound();
// Additionally require the authenticated unit/app session to match unit.UnitId before persisting progress.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Live traffic toggle on MapLibre maps that ask for one (§11.1); the URL and label come from _MapKitConfig. | ||
| const kitConfig = window.resgridMapConfig; | ||
| if (library.name === 'MapLibre' && (resolvedMapConfig.traffic === 'on' || resolvedMapConfig.traffic === 'off') && kitConfig?.trafficUrl) { | ||
| map.once('load', () => { |
There was a problem hiding this comment.
The map.once('load', () => { listener in Web/Resgrid.Web/Areas/User/Apps/src/components/map/MapboxMapView.tsx does not handle map errors, explicitly clean up its lifecycle, or handle failures from the callback's asynchronous work. Use a named handleTrafficLoad callback with cancellation checks and a promise catch handler, register a handleMapError listener, and remove listeners during effect cleanup.
Kody rule violation: Provide error handlers to subscription/listener APIs
const handleTrafficLoad = () => {
if (cancelled) return;
void import('../../maps/rgTraffic')
.then(/* ... */)
.catch(/* ... */);
};
map.once('load', handleTrafficLoad);
map.once('error', handleMapError);Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/map/MapboxMapView.tsx:
Line 293:
The `map.once('load', () => {` listener in `Web/Resgrid.Web/Areas/User/Apps/src/components/map/MapboxMapView.tsx` does not handle map errors, explicitly clean up its lifecycle, or handle failures from the callback's asynchronous work. Use a named `handleTrafficLoad` callback with cancellation checks and a promise catch handler, register a `handleMapError` listener, and remove listeners during effect cleanup.
Suggested Code:
const handleTrafficLoad = () => {
if (cancelled) return;
void import('../../maps/rgTraffic')
.then(/* ... */)
.catch(/* ... */);
};
map.once('load', handleTrafficLoad);
map.once('error', handleMapError);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const useMapLibreRenderer = useMemo(() => isMapLibreRendererEnabled(resolvedMapConfig), [resolvedMapConfig]); | ||
| const useMapboxRenderer = useMemo(() => !useMapLibreRenderer && isMapboxRendererEnabled(resolvedMapConfig), [resolvedMapConfig, useMapLibreRenderer]); | ||
| const hasMapSource = useMapboxRenderer || resolvedMapConfig.tileUrl.length > 0; |
There was a problem hiding this comment.
The unit map's hasMapSource gate omits useMapLibreRenderer even though the preceding branch selects MapLibre when it is enabled, so a department configured with only a signed MapLibre style has hasMapSource === false and the existing no-source guard prevents the selected map from rendering. Include useMapLibreRenderer in the gate, matching the main MapElement implementation.
const useMapLibreRenderer = useMemo(() => isMapLibreRendererEnabled(resolvedMapConfig), [resolvedMapConfig]);
const useMapboxRenderer = useMemo(() => !useMapLibreRenderer && isMapboxRendererEnabled(resolvedMapConfig), [resolvedMapConfig, useMapLibreRenderer]);
const hasMapSource = useMapLibreRenderer || useMapboxRenderer || resolvedMapConfig.tileUrl.length > 0;Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/unitMap/UnitMapElement.tsx:
Line 182 to 184:
The unit map's `hasMapSource` gate omits `useMapLibreRenderer` even though the preceding branch selects MapLibre when it is enabled, so a department configured with only a signed MapLibre style has `hasMapSource === false` and the existing no-source guard prevents the selected map from rendering. Include `useMapLibreRenderer` in the gate, matching the main `MapElement` implementation.
Suggested Code:
const useMapLibreRenderer = useMemo(() => isMapLibreRendererEnabled(resolvedMapConfig), [resolvedMapConfig]);
const useMapboxRenderer = useMemo(() => !useMapLibreRenderer && isMapboxRendererEnabled(resolvedMapConfig), [resolvedMapConfig, useMapLibreRenderer]);
const hasMapSource = useMapLibreRenderer || useMapboxRenderer || resolvedMapConfig.tileUrl.length > 0;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| * MapLibre map loads the library through here so the URL is set before the first map is created. | ||
| */ | ||
| export function loadMapLibre(): Promise<MapLibreModule> { | ||
| loading ??= import('maplibre-gl').then((maplibre) => { |
There was a problem hiding this comment.
The cached dynamic-import promise in Web/Resgrid.Web/Areas/User/Apps/src/maps/maplibreLoader.ts remains rejected after module loading fails, causing subsequent callers to reuse the failed promise. Attach a catch handler that resets loading before rethrowing the error.
Kody rule violation: Handle async operations with proper error handling
loading ??= import('maplibre-gl').then((maplibre) => {
maplibre.setWorkerUrl(workerUrl);
return maplibre;
}).catch((error) => {
loading = null;
throw error;
});Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/maps/maplibreLoader.ts:
Line 14:
The cached dynamic-import promise in `Web/Resgrid.Web/Areas/User/Apps/src/maps/maplibreLoader.ts` remains rejected after module loading fails, causing subsequent callers to reuse the failed promise. Attach a catch handler that resets `loading` before rethrowing the error.
Suggested Code:
loading ??= import('maplibre-gl').then((maplibre) => {
maplibre.setWorkerUrl(workerUrl);
return maplibre;
}).catch((error) => {
loading = null;
throw error;
});
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| const style = document.createElement('style'); | ||
| style.textContent = | ||
| '.maplibregl-ctrl-group button.active{background-color:#dbe9f7;color:#1c6fb5}' + |
There was a problem hiding this comment.
The .maplibregl-ctrl-group button.active selector in rgControlStyles.ts applies globally, allowing one map's control styling to affect unrelated maps and components. Scope the selector beneath the owning map container or use a CSS module or class-based strategy.
Kody rule violation: Use component-scoped styling
'.map-container .maplibregl-ctrl-group button.active{background-color:#dbe9f7;color:#1c6fb5}' +Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/maps/rgControlStyles.ts:
Line 12:
The `.maplibregl-ctrl-group button.active` selector in `rgControlStyles.ts` applies globally, allowing one map's control styling to affect unrelated maps and components. Scope the selector beneath the owning map container or use a CSS module or class-based strategy.
Suggested Code:
'.map-container .maplibregl-ctrl-group button.active{background-color:#dbe9f7;color:#1c6fb5}' +
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const image = document.createElement('img'); | ||
| image.src = marker.iconUrl; | ||
| image.alt = ''; |
There was a problem hiding this comment.
rgMaps.ts creates marker images without explicit dimensions or asynchronous loading hints, increasing layout shifts and image loading cost. Set the image width and height, enable lazy loading, and use asynchronous decoding.
Kody rule violation: Serve responsive images with modern formats and lazy-load
const image = document.createElement('img');
image.src = marker.iconUrl;
image.width = size[0];
image.height = size[1];
image.loading = 'lazy';
image.decoding = 'async';
image.alt = '';Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/maps/rgMaps.ts:
Line 506 to 508:
`rgMaps.ts` creates marker images without explicit dimensions or asynchronous loading hints, increasing layout shifts and image loading cost. Set the image width and height, enable lazy loading, and use asynchronous decoding.
Suggested Code:
const image = document.createElement('img');
image.src = marker.iconUrl;
image.width = size[0];
image.height = size[1];
image.loading = 'lazy';
image.decoding = 'async';
image.alt = '';
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @@ -45,7 +45,8 @@ | |||
| <div class="wrapper wrapper-content"> | |||
| <div class="ibox float-e-margins"> | |||
| <div class="ibox-content"> | |||
| <rg-unit-map unitid="@Model.Unit.UnitId" departmentid="@ClaimsAuthorizationHelper.GetDepartmentId()" mapheight="650px" strings="@System.Text.Json.JsonSerializer.Serialize(unitMapStrings)" leafletosmurl="@mapConfig.TileUrl" mapattribution="@mapConfig.Attribution" mapprovider="@mapConfig.MapProvider" mapstyleurl="@mapConfig.StyleUrl" mapaccesstoken="@mapConfig.AccessToken"></rg-unit-map> | |||
| <rg-unit-map unitid="@Model.Unit.UnitId" departmentid="@ClaimsAuthorizationHelper.GetDepartmentId()" mapheight="650px" strings="@System.Text.Json.JsonSerializer.Serialize(unitMapStrings)" leafletosmurl="@mapConfig.TileUrl" mapattribution="@mapConfig.Attribution" mapprovider="@mapConfig.MapProvider" mapstyleurl="@mapConfig.StyleUrl" mapaccesstoken="@mapConfig.AccessToken" maprenderer="@SettingsHelper.GetMapLibreWebConfig().Renderer" maplibrestyleurl="@SettingsHelper.GetMapLibreWebConfig().StyleUrlDay" traffic="off"></rg-unit-map> | |||
There was a problem hiding this comment.
Web/Resgrid.Web/Areas/User/Views/Units/ViewMap.cshtml dereferences Model.Unit, mapConfig, and SettingsHelper.GetMapLibreWebConfig() without null guards, so missing configuration can fail view rendering. Assign the map configuration and MapLibre configuration to local values before rendering, then use optional access and sensible defaults.
Kody rule violation: Add null checks before accessing properties
<rg-unit-map unitid="@Model.Unit?.UnitId" departmentid="@ClaimsAuthorizationHelper.GetDepartmentId()" mapheight="650px" strings="@System.Text.Json.JsonSerializer.Serialize(unitMapStrings)" leafletosmurl="@mapConfig?.TileUrl" mapattribution="@mapConfig?.Attribution" mapprovider="@mapConfig?.MapProvider" mapstyleurl="@mapConfig?.StyleUrl" mapaccesstoken="@mapConfig?.AccessToken" maprenderer="@mapLibreConfig?.Renderer" maplibrestyleurl="@mapLibreConfig?.StyleUrlDay" traffic="@TrafficMode.Off"></rg-unit-map>Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Units/ViewMap.cshtml:
Line 48:
`Web/Resgrid.Web/Areas/User/Views/Units/ViewMap.cshtml` dereferences `Model.Unit`, `mapConfig`, and `SettingsHelper.GetMapLibreWebConfig()` without null guards, so missing configuration can fail view rendering. Assign the map configuration and MapLibre configuration to local values before rendering, then use optional access and sensible defaults.
Suggested Code:
<rg-unit-map unitid="@Model.Unit?.UnitId" departmentid="@ClaimsAuthorizationHelper.GetDepartmentId()" mapheight="650px" strings="@System.Text.Json.JsonSerializer.Serialize(unitMapStrings)" leafletosmurl="@mapConfig?.TileUrl" mapattribution="@mapConfig?.Attribution" mapprovider="@mapConfig?.MapProvider" mapstyleurl="@mapConfig?.StyleUrl" mapaccesstoken="@mapConfig?.AccessToken" maprenderer="@mapLibreConfig?.Renderer" maplibrestyleurl="@mapLibreConfig?.StyleUrlDay" traffic="@TrafficMode.Off"></rg-unit-map>
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| function startGeoman(initial) { | ||
| // Circles, circle markers and text come back as what they were (they used to reload as plain markers). | ||
| if (initial) { | ||
| L.geoJSON(initial, { |
There was a problem hiding this comment.
Existing Leaflet shapes loaded with L.geoJSON(...).addTo(leafletMap) are excluded because collect() serializes only leafletMap.pm.getGeomanLayers(), so saving an unchanged layer or one whose shapes are not registered by Geoman can replace the stored FeatureCollection with an empty or partial collection. Retain the L.geoJSON layer group and include its child layers in collection, or collect the loaded GeoJSON layers together with newly created Geoman layers.
var loadedLayers = [];
if (initial) {
var loaded = L.geoJSON(initial, {
style: function () { return { color: layerColor() }; },
pointToLayer: function (feature, latlng) {
var p = feature.properties || {};
if (p.shape === 'Circle' && Number(p.radius) > 0) return L.circle(latlng, { radius: Number(p.radius), color: layerColor() });
if (p.shape === 'CircleMarker') return L.circleMarker(latlng, { color: layerColor() });
if (p.shape === 'Text' && p.text) return L.marker(latlng, { textMarker: true, text: p.text });
return L.marker(latlng);
}
}).addTo(leafletMap);
loaded.eachLayer(function (layer) { loadedLayers.push(layer); });
}
...
if (leafletMap) {
var layers = loadedLayers.concat(leafletMap.pm.getGeomanLayers());
return layers.map(function (layer) {Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.layereditor.js:
Line 90:
Existing Leaflet shapes loaded with `L.geoJSON(...).addTo(leafletMap)` are excluded because `collect()` serializes only `leafletMap.pm.getGeomanLayers()`, so saving an unchanged layer or one whose shapes are not registered by Geoman can replace the stored FeatureCollection with an empty or partial collection. Retain the `L.geoJSON` layer group and include its child layers in collection, or collect the loaded GeoJSON layers together with newly created Geoman layers.
Suggested Code:
var loadedLayers = [];
if (initial) {
var loaded = L.geoJSON(initial, {
style: function () { return { color: layerColor() }; },
pointToLayer: function (feature, latlng) {
var p = feature.properties || {};
if (p.shape === 'Circle' && Number(p.radius) > 0) return L.circle(latlng, { radius: Number(p.radius), color: layerColor() });
if (p.shape === 'CircleMarker') return L.circleMarker(latlng, { color: layerColor() });
if (p.shape === 'Text' && p.text) return L.marker(latlng, { textMarker: true, text: p.text });
return L.marker(latlng);
}
}).addTo(leafletMap);
loaded.eachLayer(function (layer) { loadedLayers.push(layer); });
}
...
if (leafletMap) {
var layers = loadedLayers.concat(leafletMap.pm.getGeomanLayers());
return layers.map(function (layer) {
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| iconAnchor: [12, 24] | ||
| }); | ||
| iconAnchor: [12, 24], | ||
| title: poi.Title || '', |
There was a problem hiding this comment.
resgrid.mapping.viewType.js passes the raw poi.Title into marker options even though the title is later rendered as user-facing map content, creating an injection risk. Sanitize the POI title with resgrid.escapeHtml before assigning it.
Kody rule violation: Always sanitize user inputs
title: resgrid.escapeHtml(poi?.Title ?? ''),Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.viewType.js:
Line 77:
`resgrid.mapping.viewType.js` passes the raw `poi.Title` into marker options even though the title is later rendered as user-facing map content, creating an injection risk. Sanitize the POI title with `resgrid.escapeHtml` before assigning it.
Suggested Code:
title: resgrid.escapeHtml(poi?.Title ?? ''),
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| editStops = keys.map(function (key) { | ||
| return editStops.filter(function (s) { return s.id === key; })[0]; | ||
| }).filter(Boolean); |
There was a problem hiding this comment.
The edit-page reorder callback compares numeric stop IDs with the editor's string keys using strict equality, so every reordered lookup returns undefined and the subsequent debounced save posts an empty stopIds list, deleting the route's stop order from the UI and state. Normalize both sides before comparing, as the new-page callback already does.
editStops = keys.map(function (key) {
return editStops.filter(function (s) { return String(s.id) === String(key); })[0];
}).filter(Boolean);Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js:
Line 483 to 485:
The edit-page reorder callback compares numeric stop IDs with the editor's string keys using strict equality, so every reordered lookup returns undefined and the subsequent debounced save posts an empty `stopIds` list, deleting the route's stop order from the UI and state. Normalize both sides before comparing, as the new-page callback already does.
Suggested Code:
editStops = keys.map(function (key) {
return editStops.filter(function (s) { return String(s.id) === String(key); })[0];
}).filter(Boolean);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (bounds) { | ||
| points.push(bounds[0], bounds[1]); | ||
| } | ||
| } catch (e) { } |
There was a problem hiding this comment.
The empty catch block in Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.instancedetail.js silently swallows exceptions, hiding failures and preventing explicit recovery. Log the error with context and either rethrow it or handle it explicitly.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.instancedetail.js:
Line 31:
The empty catch block in `Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.instancedetail.js` silently swallows exceptions, hiding failures and preventing explicit recovery. Log the error with context and either rethrow it or handle it explicitly.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Disabled after this turn of the event loop: a button disabled while the browser builds the post would drop the clicked | ||
| // button's own name and value (and its formaction) from it. The SUBMITTING mark already refuses a second submit meanwhile. | ||
| window.setTimeout(function () { |
There was a problem hiding this comment.
resgrid.submit-once.js and the listed callers schedule a timeout without retaining its handle, so the deferred callback can run after the form is released or its owning page is torn down. Store the handle and clear it during release and cleanup.
Kody rule violation: Clear timers on teardown/unmount
form.submitOnceLockTimer = window.setTimeout(function () {Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/public/resgrid.submit-once.js:
Line 69:
`resgrid.submit-once.js` and the listed callers schedule a timeout without retaining its handle, so the deferred callback can run after the form is released or its owning page is torn down. Store the handle and clear it during release and cleanup.
Suggested Code:
form.submitOnceLockTimer = window.setTimeout(function () {
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:
|
| // Without the passphrase the key is a public constant and anyone could sign; refuse every signed URL instead | ||
| // (bearer callers are unaffected). Same rule as the invoice payment links. | ||
| if (string.IsNullOrWhiteSpace(Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase)) | ||
| return false; |
There was a problem hiding this comment.
Signed-resource validation rejects every URL when ExternalLinkUrlParamPassphrase is empty, while SignQuery and Sign continue generating signed URLs without validating that configuration; the default empty value allows MapStyleService, StaticMapService, ConfigController, and MappingController to return anonymous style, static-map, offline, and custom-map URLs that resource endpoints reject. Make signed URL generation fail or return no URL when the passphrase is unset and have callers fall back to bearer-only resources, or require and validate the passphrase before advertising signed resource URLs.
if (string.IsNullOrWhiteSpace(Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase))
return false;
// SignQuery/Sign callers must also avoid returning signed URLs when this configuration is unset.Prompt for LLM
File Core/Resgrid.Services/MapResourceUrls.cs:
Line 51 to 54:
Signed-resource validation rejects every URL when ExternalLinkUrlParamPassphrase is empty, while SignQuery and Sign continue generating signed URLs without validating that configuration; the default empty value allows MapStyleService, StaticMapService, ConfigController, and MappingController to return anonymous style, static-map, offline, and custom-map URLs that resource endpoints reject. Make signed URL generation fail or return no URL when the passphrase is unset and have callers fall back to bearer-only resources, or require and validate the passphrase before advertising signed resource URLs.
Suggested Code:
if (string.IsNullOrWhiteSpace(Config.SystemBehaviorConfig.ExternalLinkUrlParamPassphrase))
return false;
// SignQuery/Sign callers must also avoid returning signed URLs when this configuration is unset.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Decided on the use's own count, not only the marker: concurrent posts that all read no marker each get a distinct | ||
| // count from the atomic increment, so the ones past the cap are refused before the marker is visible to them. | ||
| return await CountAsync(form, ipAddress) <= LimitOf(form).Cap; |
There was a problem hiding this comment.
Unhandled CountAsync rejections in Core/Resgrid.Services/PublicFormRateLimiter.cs:69, Tests/Resgrid.Tests/Security/PublicFormRateLimitTests.cs:122, Tests/Resgrid.Tests/Services/Routing/PersonnelEtaTests.cs:44, Tests/Resgrid.Tests/Services/Routing/PersonnelEtaTests.cs:55, and Tests/Resgrid.Tests/Services/Routing/PersonnelEtaTests.cs:65 lack operation context in the logs. Wrap the awaited CountAsync call in try/catch, log failures with Logging.LogException and the form and IP address, then rethrow the exception.
Kody rule violation: Handle async operations with proper error handling
try
{
return await CountAsync(form, ipAddress) <= LimitOf(form).Cap;
}
catch (Exception ex)
{
Logging.LogException(ex, $"Could not acquire the rate-limit count for form {form} and IP address {ipAddress}.");
throw;
}Prompt for LLM
File Core/Resgrid.Services/PublicFormRateLimiter.cs:
Line 48:
Unhandled CountAsync rejections in Core/Resgrid.Services/PublicFormRateLimiter.cs:69, Tests/Resgrid.Tests/Security/PublicFormRateLimitTests.cs:122, Tests/Resgrid.Tests/Services/Routing/PersonnelEtaTests.cs:44, Tests/Resgrid.Tests/Services/Routing/PersonnelEtaTests.cs:55, and Tests/Resgrid.Tests/Services/Routing/PersonnelEtaTests.cs:65 lack operation context in the logs. Wrap the awaited CountAsync call in try/catch, log failures with Logging.LogException and the form and IP address, then rethrow the exception.
Suggested Code:
try
{
return await CountAsync(form, ipAddress) <= LimitOf(form).Cap;
}
catch (Exception ex)
{
Logging.LogException(ex, $"Could not acquire the rate-limit count for form {form} and IP address {ipAddress}.");
throw;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }), | ||
| ) | ||
| // The toggle is optional: the map stays usable without it, so a failed chunk load is logged, not raised. | ||
| .catch((trafficError: unknown) => console.error('Failed to add the live traffic toggle', trafficError)); |
There was a problem hiding this comment.
Unstructured console.error reporting in Web/Resgrid.Web/Areas/User/Apps/src/components/map/MapboxMapView.tsx and Web/Resgrid.Web/Areas/User/Apps/src/maps/rgMaps.ts:382 omits structured operation context for live traffic toggle failures. Emit the error through the structured logger with operation: 'addTrafficToggle' and the trafficError value in the err field.
Kody rule violation: Include error context in structured logs
.catch((trafficError: unknown) => logger.error('Live traffic toggle failed', { operation: 'addTrafficToggle', err: trafficError }));Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/map/MapboxMapView.tsx:
Line 307:
Unstructured console.error reporting in Web/Resgrid.Web/Areas/User/Apps/src/components/map/MapboxMapView.tsx and Web/Resgrid.Web/Areas/User/Apps/src/maps/rgMaps.ts:382 omits structured operation context for live traffic toggle failures. Emit the error through the structured logger with operation: 'addTrafficToggle' and the trafficError value in the err field.
Suggested Code:
.catch((trafficError: unknown) => logger.error('Live traffic toggle failed', { operation: 'addTrafficToggle', err: trafficError }));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary
Added configurable MapLibre and self-hosted mapping support alongside existing Mapbox/Leaflet behavior, including:
Added server-side routing infrastructure with configurable Valhalla, Mapbox, Google, OSRM, and straight-line estimate providers:
Expanded route planning and navigation:
Added offline map packs:
Added vehicle-size and weight configuration to unit types and individual units, with localized metric/imperial input and inherited defaults.
Added account security protections:
Corrected map-layer geometry persistence so polygon holes and multipolygons round-trip correctly, while unsupported geometries are omitted safely.
Added and expanded localized UI text for navigation, offline maps, routing, vehicle limits, map drawing tools, traffic, and route planning across supported languages.
Added migrations, repositories, service registrations, API models, web controls, and comprehensive tests covering routing, offline packs, MapLibre resources, geocoding, security throttling, route lifecycle behavior, vehicle profiles, map layers, and navigation state.