Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughAdds mapping and routing features: new mapping endpoints and GeoJSON outputs, expanded route APIs (instance/stop lookups, notes update, directions, contacts, active/scheduled instances), view/UI changes for routes (start, directions, archived), client-side route/location picker scripts, new response/input models, and a documentation file ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [Authorize(Policy = ResgridResources.Route_Update)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult> UpdateStopNotes([FromBody] UpdateStopNotesInput input) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
In general, to fix missing CSRF validation in ASP.NET Core MVC, you should ensure that any state-changing HTTP methods (especially POST) that can be invoked from a browser with cookie-based auth are protected by the anti-forgery system. This is typically done by adding [ValidateAntiForgeryToken] (or [AutoValidateAntiforgeryToken] globally) to such actions, and ensuring clients send the token (via form fields or headers) with each request.
For this specific method, the minimal, targeted fix is to decorate the UpdateStopNotes action with the anti-forgery validation attribute. Since this controller already uses other ASP.NET Core MVC attributes from Microsoft.AspNetCore.Mvc, we can safely add [ValidateAntiForgeryToken] above UpdateStopNotes without changing its behavior otherwise. No new imports are required because ValidateAntiForgeryTokenAttribute lives in Microsoft.AspNetCore.Mvc, which is already imported on line 3. The concrete change is in Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs: between the existing attributes ([HttpPost("UpdateStopNotes")], [Authorize(...)], [ProducesResponseType(...)]) and the method signature on line 842, add [ValidateAntiForgeryToken]. This preserves existing authorization and routing behavior while adding CSRF protection.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs
| @@ -839,6 +839,7 @@ | ||
| [Authorize(Policy = ResgridResources.Route_Update)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult> UpdateStopNotes([FromBody] UpdateStopNotesInput input) | ||
| { | ||
| var instanceStop = await _routeService.GetInstanceStopByIdAsync(input.RouteInstanceStopId); |
Copilot is powered by AI and may make mistakes. Always verify output.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
682-688:⚠️ Potential issue | 🟡 MinorDocument the added second argument.
SearchIndoorLocationsnow exposes two string inputs, but onlytermis described here. Please add the missing<param>in the source XML comment so the generated API docs do not still read like the old one-parameter contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 682 - 688, The XML docs for Resgrid.Web.Services.Controllers.v4.MappingController.SearchIndoorLocations(System.String,System.String) only document the first parameter (`term`); update the source XML comment for the SearchIndoorLocations method to add a second <param> element that matches the second parameter's name in the method signature and briefly describes its purpose (e.g., building or context identifier used to scope the search) so generated API docs reflect the two-argument contract.
🧹 Nitpick comments (12)
Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs (3)
1288-1319: Consider filtering out features with invalid geometry instead of including them with null.When
JToken.Parsefails, the feature is still included withgeometry: null. Some GeoJSON consumers may reject or mishandle features without valid geometry.♻️ Proposed refinement
private static string BuildZonesGeoJson(List<Model.IndoorMapZone> zones) { var features = zones .Where(z => !z.IsDeleted && !string.IsNullOrWhiteSpace(z.GeoGeometry)) - .Select(z => + .Select(z => { - JToken geometry; - try { geometry = JToken.Parse(z.GeoGeometry); } - catch { geometry = JValue.CreateNull(); } - - return new + try { - type = "Feature", - id = z.IndoorMapZoneId, - geometry, - properties = new + var geometry = JToken.Parse(z.GeoGeometry); + return new { - id = z.IndoorMapZoneId, - name = z.Name, - description = z.Description, - zoneType = z.ZoneType, - color = z.Color, - isSearchable = z.IsSearchable, - isDispatchable = z.IsDispatchable, - metadata = z.Metadata - } - }; + type = "Feature", + id = z.IndoorMapZoneId, + geometry, + properties = new + { + id = z.IndoorMapZoneId, + name = z.Name, + description = z.Description, + zoneType = z.ZoneType, + color = z.Color, + isSearchable = z.IsSearchable, + isDispatchable = z.IsDispatchable, + metadata = z.Metadata + } + }; + } + catch + { + return null; + } }) + .Where(f => f != null) .ToList(); return JsonConvert.SerializeObject(new { type = "FeatureCollection", features }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs` around lines 1288 - 1319, In BuildZonesGeoJson, don't include zones whose GeoGeometry fails to parse into a valid JToken; currently the code sets geometry = JValue.CreateNull() and still returns a feature. Change the selection so that after attempting to parse z.GeoGeometry into the local variable geometry (or using a TryParse helper) you skip/ filter out that zone when geometry is null/invalid (e.g., only return the anonymous feature when geometry is non-null), ensuring features list only contains entries with valid geometry for IndoorMapZone entries.
1161-1187: Same N+1 query pattern applies here for custom regions.The same batching optimization suggested above for indoor zones should be applied to custom region lookups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs` around lines 1161 - 1187, The loop in MappingController that calls _customMapService.GetLayerByIdAsync and GetCustomMapByIdAsync per region causes an N+1 query pattern; change SearchRegionsAsync handling to batch-load related layers and maps: after getting regions from SearchRegionsAsync(DepartmentId, term) collect distinct IndoorMapFloorId and IndoorMapId values, call new or existing bulk methods (e.g., _customMapService.GetLayersByIdsAsync(IEnumerable<long> ids) and _customMapService.GetCustomMapsByIdsAsync(IEnumerable<long> ids)) to fetch all layers and maps in two queries, build dictionaries keyed by id, then iterate regions and look up layer/map from those dictionaries when creating MapFeatureResultData before calling result.Data.Add.
1133-1159: Consider batching related entity lookups to avoid N+1 queries.Each zone triggers two sequential database calls (
GetFloorByIdAsyncthenGetIndoorMapByIdAsync). With many search results, this pattern scales poorly. Consider pre-fetching all relevant floors and maps in batches before the loop.♻️ Example approach
if (type == "all" || type == "indoor") { var zones = await _indoorMapService.SearchZonesAsync(DepartmentId, term); if (zones != null) { + // Batch-fetch all floors and maps upfront + var floorIds = zones.Select(z => z.IndoorMapFloorId).Distinct().ToList(); + var floors = await _indoorMapService.GetFloorsByIdsAsync(floorIds); + var floorLookup = floors.ToDictionary(f => f.IndoorMapFloorId); + + var mapIds = floors.Select(f => f.IndoorMapId).Distinct().ToList(); + var maps = await _indoorMapService.GetIndoorMapsByIdsAsync(mapIds); + var mapLookup = maps.ToDictionary(m => m.IndoorMapId); + foreach (var z in zones) { - var floor = await _indoorMapService.GetFloorByIdAsync(z.IndoorMapFloorId); - var map = floor != null ? await _indoorMapService.GetIndoorMapByIdAsync(floor.IndoorMapId) : null; + floorLookup.TryGetValue(z.IndoorMapFloorId, out var floor); + var map = floor != null && mapLookup.TryGetValue(floor.IndoorMapId, out var m) ? m : null;This would require adding batch-fetch methods to the service interfaces if they don't already exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs` around lines 1133 - 1159, The loop in MappingController (using _indoorMapService.SearchZonesAsync -> GetFloorByIdAsync -> GetIndoorMapByIdAsync) causes N+1 queries; change the logic to batch-fetch floors and maps: after calling SearchZonesAsync collect distinct IndoorMapFloorId values, call a new service method like GetFloorsByIdsAsync(ids) to load all Floor objects, collect distinct IndoorMapId values from those floors and call GetIndoorMapsByIdsAsync(mapIds) to load all Map objects, build dictionary lookups for floors and maps, then iterate zones and populate MapFeatureResultData from those dictionaries (no per-zone awaits). If the service interface lacks batch methods, add GetFloorsByIdsAsync and GetIndoorMapsByIdsAsync to the IIndoorMapService and implement them in the service to return the sets required.Web/Resgrid.Web/Areas/User/Views/Routes/Directions.cshtml (1)
128-143: Consider caching repeatedModel.Stops.Any()calls.The same
Model.Stops.Any(...)predicates are evaluated twice each—once for the header and once per row. For larger datasets, consider caching these booleans at the top of the table section.@{ bool hasPlannedArrival = Model.Stops.Any(s => s.PlannedArrivalTime.HasValue); bool hasDwellMinutes = Model.Stops.Any(s => s.EstimatedDwellMinutes.HasValue); bool hasContact = Model.Stops.Any(s => !string.IsNullOrEmpty(s.ContactName)); bool hasNotes = Model.Stops.Any(s => !string.IsNullOrEmpty(s.Notes)); }Then reference these variables in the header and body conditionals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Routes/Directions.cshtml` around lines 128 - 143, Cache the repeated Model.Stops.Any(...) predicates into local booleans before rendering the table (e.g., hasPlannedArrival, hasDwellMinutes, hasContact, hasNotes) and replace all occurrences of Model.Stops.Any(...) in both the header and per-row conditionals with these cached variables so the predicates are evaluated once instead of repeatedly; update the header checks (currently using Model.Stops.Any in the Directions.cshtml view) and the corresponding row rendering logic to reference the new boolean variables.Web/Resgrid.Web/Areas/User/Views/Routes/ArchivedView.cshtml (1)
41-42: Handle nullableGeofenceRadiusMeters.If
GeofenceRadiusMetersis nullable, displaying it directly could show nothing meaningful. Consider adding a conditional or default value.<dt>@localizer["GeofenceLabel"]</dt> - <dd>@Model.Plan.GeofenceRadiusMeters m</dd> + <dd>@(Model.Plan.GeofenceRadiusMeters?.ToString() ?? "100") m</dd>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Routes/ArchivedView.cshtml` around lines 41 - 42, Model.Plan.GeofenceRadiusMeters may be nullable so update the view to handle nulls: when rendering the GeofenceRadiusMeters (reference Model.Plan.GeofenceRadiusMeters) check for HasValue (or null) and render a sensible default or placeholder (e.g., "N/A" or "0 m") and append the "m" unit only when a value exists; ensure the conditional is applied in the ArchivedView.cshtml where the <dd> currently outputs Model.Plan.GeofenceRadiusMeters.Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.directions.js (2)
153-159: Avoid parsing GeoJSON twice for styling and bounds.
L.geoJSON(route.geometry)is called twice—once to add the styled layer and once just to get bounds. Reuse the layer object:// Draw route polyline if (route.geometry) { - L.geoJSON(route.geometry, { + var routeLayer = L.geoJSON(route.geometry, { style: { color: routeColor || '#1c84c6', weight: 5, opacity: 0.75 } - }).addTo(map); - map.fitBounds(L.geoJSON(route.geometry).getBounds(), { padding: [50, 50] }); + }); + routeLayer.addTo(map); + map.fitBounds(routeLayer.getBounds(), { padding: [50, 50] }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.directions.js` around lines 153 - 159, The code parses the same GeoJSON twice (L.geoJSON(route.geometry))—create the geoJSON layer once (e.g., assign to a variable like routeLayer from L.geoJSON(route.geometry, { style: ... })), add that layer to the map with routeLayer.addTo(map) and then call map.fitBounds(routeLayer.getBounds(), { padding: [50,50] }) so you reuse the layer for both styling and bounds calculations instead of parsing twice.
200-208: Fragile string check for existing header.Using
indexOf('stop-num">1<')to detect if the start header exists is brittle—it relies on exact HTML structure. Consider tracking header insertion with a boolean flag instead.var hasStartHeader = false; // ... in the legs.forEach: if (legIndex === 0) { hasStartHeader = true; // ... insert header } // ... after loop: if (!hasStartHeader && validStops.length > 0) { // prepend start header }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.directions.js` around lines 200 - 208, The code uses a brittle string check html.indexOf('stop-num">1<') to detect an inserted start header; replace this with a boolean flag: introduce hasStartHeader (default false), set hasStartHeader = true at the point where you insert the header in the legs.forEach (e.g., where legIndex === 0 or where you build the header for the first leg), then change the final conditional to if (!hasStartHeader && validStops.length > 0) to prepend the start header using startStop and escapeHtml; remove the indexOf('stop-num">1<') check entirely and rely on hasStartHeader and existing variables html, validStops, startStop.Web/Resgrid.Web/Areas/User/Models/Routes/RouteViewModels.cs (1)
17-26: Consider consolidatingArchivedRouteIndexViewwithRouteIndexView.Both classes have identical structure (
PlansandStopCounts). Consider using a single class or base class if the only difference is how they're populated in the controller.// Alternative: single class with an IsArchived flag or just reuse RouteIndexView public class RouteListView : BaseUserModel { public List<RoutePlan> Plans { get; set; } public Dictionary<string, int> StopCounts { get; set; } public bool ShowArchived { get; set; } // ... }This is a minor suggestion for reducing duplication; the current approach is functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Models/Routes/RouteViewModels.cs` around lines 17 - 26, ArchivedRouteIndexView duplicates the structure of RouteIndexView (both define Plans and StopCounts); remove the duplicate by consolidating into a single view model (e.g., reuse RouteIndexView or create RouteListView) and add a distinguishing property such as IsArchived or ShowArchived to indicate archived vs active data. Update usages in controllers and views that reference ArchivedRouteIndexView to use the consolidated model (or RouteIndexView) and set the new flag where appropriate; ensure constructors/properties in the consolidated class initialize Plans and StopCounts as the originals did.Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs (1)
39-45: N+1 query pattern when fetching stop counts.Each plan triggers a separate
GetRouteStopsForPlanAsynccall. For departments with many routes, this could impact performance. Consider batching or adding a dedicated method to fetch counts.// Option 1: Add a service method that returns counts for multiple plans var stopCounts = await _routeService.GetStopCountsForPlansAsync(model.Plans.Select(p => p.RoutePlanId)); // Option 2: Accept current pattern for now if plan counts are typically lowThis is a recommended improvement for scalability but may be acceptable for typical use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs` around lines 39 - 45, The current loop in RoutesController uses _routeService.GetRouteStopsForPlanAsync per plan causing an N+1 query; replace this with a batched counts call by adding a new service method like GetStopCountsForPlansAsync that accepts the list of RoutePlanId (from model.Plans.Select(p => p.RoutePlanId)) and returns a dictionary of RoutePlanId -> count, then populate model.StopCounts from that result instead of calling GetRouteStopsForPlanAsync repeatedly; update RoutesController to call GetRoutePlansForDepartmentAsync, then call the new GetStopCountsForPlansAsync and assign counts to model.StopCounts.Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs (3)
703-712: N+1 query pattern when loading contacts.Each stop with a
ContactIdtriggers a separateGetContactByIdAsynccall. Consider batch-loading contacts:var contactIds = stops.Where(s => !string.IsNullOrWhiteSpace(s.ContactId)) .Select(s => s.ContactId).Distinct().ToList(); var contacts = await _contactsService.GetContactsByIdsAsync(contactIds); // Add batch method var contactLookup = contacts.Where(c => c.DepartmentId == DepartmentId) .ToDictionary(c => c.ContactId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs` around lines 703 - 712, The loop causes N+1 calls by calling _contactsService.GetContactByIdAsync for each stop; instead, collect distinct ContactIds from stops (where ContactId not null/whitespace), add a new batch method on _contactsService like GetContactsByIdsAsync(IEnumerable<string> ids), await that once, filter contacts by DepartmentId, create a lookup (by ContactId) and then iterate stops to MapContactToResult using the lookup and add to result.Data (replacing seenContactIds and per-stop GetContactByIdAsync calls); keep MapContactToResult and result.Data.Add usage unchanged.
784-801: Clarify stop status values in filter.The filter
w.Status == 0 || w.Status == 1appears to target stop-level statuses (Pending=0, CheckedIn=1), not theRouteInstanceStatusenum values. TheRouteInstanceStatusenum defines values for route instances (Scheduled=0, InProgress=1, etc.), not individual stops.Consider adding a comment or using named constants to clarify the stop status semantics:
// Stop status: 0 = Pending, 1 = CheckedIn, 2 = CheckedOut, 3 = Skipped .Where(w => w.Status == 0 || w.Status == 1) // Pending or CheckedIn onlyBetter yet, introduce an enum for
RouteInstanceStopStatusif one doesn't exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs` around lines 784 - 801, The filter using magic numbers in the waypoints projection (variable waypoints, RouteDirectionWaypointData.Status) is ambiguous—replace the hard-coded 0/1 with named values by introducing or using a RouteInstanceStopStatus enum (e.g., Pending = 0, CheckedIn = 1) or declared constants and update the LINQ to .Where(w => w.Status == RouteInstanceStopStatus.Pending || w.Status == RouteInstanceStopStatus.CheckedIn); also add a short inline comment explaining the stop status mapping and ensure the Status assignment when building RouteDirectionWaypointData uses the same enum/typed value (look at planStops, instanceStopLookup and RouteDirectionWaypointData for where to adjust).
856-905: N+1 query pattern inGetActiveRouteInstances.Each active instance triggers three additional queries:
GetRoutePlanByIdAsync,GetInstanceStopsAsync, and potentiallyGetRouteStopByIdAsync. For departments with many active routes, this could significantly impact performance.Consider batch-loading plans and stops:
// Load all needed plans in one query var planIds = active.Select(i => i.RoutePlanId).Distinct(); var plans = await _routeService.GetRoutePlansByIdsAsync(planIds); // Add this method var planLookup = plans.ToDictionary(p => p.RoutePlanId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs` around lines 856 - 905, GetActiveRouteInstances has an N+1 query problem because each active instance calls GetRoutePlanByIdAsync, GetInstanceStopsAsync and sometimes GetRouteStopByIdAsync; fix by batch-loading related data: collect distinct RoutePlanId values and call a new service method GetRoutePlansByIdsAsync to get all plans and build a lookup by RoutePlanId, collect all RouteInstanceId values and call a new GetInstanceStopsByInstanceIdsAsync to return stops grouped by RouteInstanceId (and if needed collect RouteStopId values and add GetRouteStopsByIdsAsync to resolve names), then iterate active instances using these in-memory lookups instead of per-instance service calls (update GetActiveRouteInstances to use the lookups and remove the per-instance awaits).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 94: The document uses mixed English variants; normalize to UK English to
match existing words like “minimise” and “organisation” by replacing “summarize”
on line 94 with the UK spelling “summarise” and scan nearby text for any other
US spellings to convert (e.g., "organize" -> "organise") so the CLAUDE.md uses
one consistent dialect.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 6136-6138: The XML doc for the FeatureType property
(Resgrid.Web.Services.Models.v4.Mapping.MapFeatureResultData.FeatureType) is
ambiguous: update the <summary> to explicitly state the exact string values
clients should expect and any mapping from the search filter terms (e.g., if
search filter uses "indoor" map to "indoor_zone", and "custom" maps to
"custom_region"), or normalize both docs to the same tokens; ensure the summary
lists both the canonical values (e.g., "indoor_zone" and "custom_region") and
the equivalent filter terms (e.g., "indoor" -> "indoor_zone", "custom" ->
"custom_region") so clients can compare the correct strings.
- Around line 727-733: Update the XML docs for the method
M:Resgrid.Web.Services.Controllers.v4.MappingController.SearchCustomMapRegions(System.String,System.String)
to include a second <param> element describing the new second string parameter
(the additional filter/input) — add the exact parameter name used in the method
signature and a concise description of what values it expects and how it alters
the search behavior so consumers can discover the extra filter.
- Around line 1117-1158: The XML docs are missing <param> entries for the listed
route endpoints; add <param> tags that match each method's signature and
describe each parameter (e.g., for GetStopsForInstance(string) and
GetStopContact(string) add a <param name="..."> describing the route/stop
identifier; for GetRouteContacts(string) and GetDirections(string) add a <param>
for the route/plan id; for GetInstanceDirections(string, Nullable<decimal>,
Nullable<decimal>) add three <param> tags: the instance id (string) and the
optional originLatitude and originLongitude (nullable decimals) with notes that
they are query parameters used to route from the unit's current position; for
UpdateStopNotes(Resgrid.Web.Services.Models.v4.Routes.UpdateStopNotesInput) add
a <param name="input"> describing the input payload. Ensure the <param> names
exactly match the method parameter names in the signatures for methods
GetStopsForInstance, GetStopContact, GetRouteContacts, GetDirections,
GetInstanceDirections, and UpdateStopNotes.
In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs`:
- Around line 373-386: Wrap the call to _routeService.StartRouteAsync in
StartRoute(RouteStartView, CancellationToken) with a try/catch that handles
InvalidOperationException and ArgumentException: catch InvalidOperationException
(unit already has active route) and add a user-friendly error
(ModelState.AddModelError or TempData["Error"]) then return
RedirectToAction("View", new { id = model.RoutePlanId }) or back to the form;
catch ArgumentException (plan not found) and redirect to Index or show an error
similarly; rethrow or let other exceptions bubble. Ensure you reference
StartRoute, StartRouteAsync, RouteStartView, and InstanceDetail when
implementing the error handling so the redirects and messages go to the correct
views.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/StartRoute.cshtml`:
- Around line 4-5: The view sets hardcoded English strings (e.g., ViewBag.Title,
breadcrumb text, labels, help text, and button text) despite injecting the
localizer; replace each hardcoded string in StartRoute.cshtml with localized
resources using the injected localizer (e.g., ViewBag.Title = localizer["Start
Route"]; and use `@localizer`["..."] for breadcrumb, labels, help text, and action
buttons) so every user-facing string in the page is localized; ensure keys are
meaningful and consistent with other routes views and update resource files
accordingly.
In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js`:
- Around line 114-115: The coordinate truthiness checks (e.g., "if (initialLat
&& initialLng) { setStartLocation(...); }") mistakenly treat 0 as missing;
change them to explicit null/undefined checks (e.g., use "!= null" or "!==
undefined") so zero latitude/longitude are accepted, and apply the same change
for all other occurrences referenced (lines around 147-148, 173-174, 182-183,
202-203, 217-218, 239-240, 254-255) where variables like initialLat, initialLng,
startLat, startLng, and any geocode/W3W latitude/longitude are validated before
calling setStartLocation or related functions.
- Around line 169-188: updateLocationPickerVisibility currently only hides/shows
the custom picker groups, but when the user switches to "Use station" the hidden
inputs (Plan.StartLatitude, Plan.StartLongitude, Plan.EndLatitude,
Plan.EndLongitude) remain enabled and stale values get saved; update the
function so that when $('#chkUseStationAsStart').prop('checked') is true you
clear and disable the StartLatitude/StartLongitude inputs (set value to empty
and add disabled) and when unchecked you enable them and call initStartPickerMap
as before, and similarly for $('#chkUseStationAsEnd') with
EndLatitude/EndLongitude; reference updateLocationPickerVisibility plus the
hidden form field names Plan.StartLatitude, Plan.StartLongitude,
Plan.EndLatitude, Plan.EndLongitude and ensure enabling/disabling happens in the
same branches that currently show/hide the `#startLocationGroup` and
`#endLocationGroup`.
In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js`:
- Around line 97-108: The coordinate checks currently use truthiness (e.g., in
initStartPickerMap's "if (initialLat && initialLng)") which treats 0 as missing;
update those to explicit numeric checks: use Number.isFinite(initialLat) &&
Number.isFinite(initialLng) (or check !== null/undefined) before calling
setStartLocation; apply the same change to the equivalent checks in
initEndPickerMap and any setStartLocation/setEndLocation or lookup logic
referenced around the other ranges so 0 coordinates are accepted.
- Around line 163-176: The form still posts stale coordinates because
updateLocationPickerVisibility only hides the picker groups; when a "Use
station" checkbox is checked you must disable and clear the corresponding hidden
inputs so they are not submitted. Update updateLocationPickerVisibility to, for
start and end respectively, when $('#chkUseStationAsStart') or
$('#chkUseStationAsEnd') is checked: disable (prop('disabled', true)) and clear
(.val('')) the Plan.StartLatitude / Plan.StartLongitude and Plan.EndLatitude /
Plan.EndLongitude inputs (IDs typically Plan_StartLatitude, Plan_StartLongitude,
Plan_EndLatitude, Plan_EndLongitude); when unchecked re-enable (prop('disabled',
false)), clear if needed, and call initStartPickerMap()/initEndPickerMap() as
before.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 682-688: The XML docs for
Resgrid.Web.Services.Controllers.v4.MappingController.SearchIndoorLocations(System.String,System.String)
only document the first parameter (`term`); update the source XML comment for
the SearchIndoorLocations method to add a second <param> element that matches
the second parameter's name in the method signature and briefly describes its
purpose (e.g., building or context identifier used to scope the search) so
generated API docs reflect the two-argument contract.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs`:
- Around line 1288-1319: In BuildZonesGeoJson, don't include zones whose
GeoGeometry fails to parse into a valid JToken; currently the code sets geometry
= JValue.CreateNull() and still returns a feature. Change the selection so that
after attempting to parse z.GeoGeometry into the local variable geometry (or
using a TryParse helper) you skip/ filter out that zone when geometry is
null/invalid (e.g., only return the anonymous feature when geometry is
non-null), ensuring features list only contains entries with valid geometry for
IndoorMapZone entries.
- Around line 1161-1187: The loop in MappingController that calls
_customMapService.GetLayerByIdAsync and GetCustomMapByIdAsync per region causes
an N+1 query pattern; change SearchRegionsAsync handling to batch-load related
layers and maps: after getting regions from SearchRegionsAsync(DepartmentId,
term) collect distinct IndoorMapFloorId and IndoorMapId values, call new or
existing bulk methods (e.g.,
_customMapService.GetLayersByIdsAsync(IEnumerable<long> ids) and
_customMapService.GetCustomMapsByIdsAsync(IEnumerable<long> ids)) to fetch all
layers and maps in two queries, build dictionaries keyed by id, then iterate
regions and look up layer/map from those dictionaries when creating
MapFeatureResultData before calling result.Data.Add.
- Around line 1133-1159: The loop in MappingController (using
_indoorMapService.SearchZonesAsync -> GetFloorByIdAsync ->
GetIndoorMapByIdAsync) causes N+1 queries; change the logic to batch-fetch
floors and maps: after calling SearchZonesAsync collect distinct
IndoorMapFloorId values, call a new service method like GetFloorsByIdsAsync(ids)
to load all Floor objects, collect distinct IndoorMapId values from those floors
and call GetIndoorMapsByIdsAsync(mapIds) to load all Map objects, build
dictionary lookups for floors and maps, then iterate zones and populate
MapFeatureResultData from those dictionaries (no per-zone awaits). If the
service interface lacks batch methods, add GetFloorsByIdsAsync and
GetIndoorMapsByIdsAsync to the IIndoorMapService and implement them in the
service to return the sets required.
In `@Web/Resgrid.Web.Services/Controllers/v4/RoutesController.cs`:
- Around line 703-712: The loop causes N+1 calls by calling
_contactsService.GetContactByIdAsync for each stop; instead, collect distinct
ContactIds from stops (where ContactId not null/whitespace), add a new batch
method on _contactsService like GetContactsByIdsAsync(IEnumerable<string> ids),
await that once, filter contacts by DepartmentId, create a lookup (by ContactId)
and then iterate stops to MapContactToResult using the lookup and add to
result.Data (replacing seenContactIds and per-stop GetContactByIdAsync calls);
keep MapContactToResult and result.Data.Add usage unchanged.
- Around line 784-801: The filter using magic numbers in the waypoints
projection (variable waypoints, RouteDirectionWaypointData.Status) is
ambiguous—replace the hard-coded 0/1 with named values by introducing or using a
RouteInstanceStopStatus enum (e.g., Pending = 0, CheckedIn = 1) or declared
constants and update the LINQ to .Where(w => w.Status ==
RouteInstanceStopStatus.Pending || w.Status ==
RouteInstanceStopStatus.CheckedIn); also add a short inline comment explaining
the stop status mapping and ensure the Status assignment when building
RouteDirectionWaypointData uses the same enum/typed value (look at planStops,
instanceStopLookup and RouteDirectionWaypointData for where to adjust).
- Around line 856-905: GetActiveRouteInstances has an N+1 query problem because
each active instance calls GetRoutePlanByIdAsync, GetInstanceStopsAsync and
sometimes GetRouteStopByIdAsync; fix by batch-loading related data: collect
distinct RoutePlanId values and call a new service method
GetRoutePlansByIdsAsync to get all plans and build a lookup by RoutePlanId,
collect all RouteInstanceId values and call a new
GetInstanceStopsByInstanceIdsAsync to return stops grouped by RouteInstanceId
(and if needed collect RouteStopId values and add GetRouteStopsByIdsAsync to
resolve names), then iterate active instances using these in-memory lookups
instead of per-instance service calls (update GetActiveRouteInstances to use the
lookups and remove the per-instance awaits).
In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs`:
- Around line 39-45: The current loop in RoutesController uses
_routeService.GetRouteStopsForPlanAsync per plan causing an N+1 query; replace
this with a batched counts call by adding a new service method like
GetStopCountsForPlansAsync that accepts the list of RoutePlanId (from
model.Plans.Select(p => p.RoutePlanId)) and returns a dictionary of RoutePlanId
-> count, then populate model.StopCounts from that result instead of calling
GetRouteStopsForPlanAsync repeatedly; update RoutesController to call
GetRoutePlansForDepartmentAsync, then call the new GetStopCountsForPlansAsync
and assign counts to model.StopCounts.
In `@Web/Resgrid.Web/Areas/User/Models/Routes/RouteViewModels.cs`:
- Around line 17-26: ArchivedRouteIndexView duplicates the structure of
RouteIndexView (both define Plans and StopCounts); remove the duplicate by
consolidating into a single view model (e.g., reuse RouteIndexView or create
RouteListView) and add a distinguishing property such as IsArchived or
ShowArchived to indicate archived vs active data. Update usages in controllers
and views that reference ArchivedRouteIndexView to use the consolidated model
(or RouteIndexView) and set the new flag where appropriate; ensure
constructors/properties in the consolidated class initialize Plans and
StopCounts as the originals did.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/ArchivedView.cshtml`:
- Around line 41-42: Model.Plan.GeofenceRadiusMeters may be nullable so update
the view to handle nulls: when rendering the GeofenceRadiusMeters (reference
Model.Plan.GeofenceRadiusMeters) check for HasValue (or null) and render a
sensible default or placeholder (e.g., "N/A" or "0 m") and append the "m" unit
only when a value exists; ensure the conditional is applied in the
ArchivedView.cshtml where the <dd> currently outputs
Model.Plan.GeofenceRadiusMeters.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/Directions.cshtml`:
- Around line 128-143: Cache the repeated Model.Stops.Any(...) predicates into
local booleans before rendering the table (e.g., hasPlannedArrival,
hasDwellMinutes, hasContact, hasNotes) and replace all occurrences of
Model.Stops.Any(...) in both the header and per-row conditionals with these
cached variables so the predicates are evaluated once instead of repeatedly;
update the header checks (currently using Model.Stops.Any in the
Directions.cshtml view) and the corresponding row rendering logic to reference
the new boolean variables.
In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.directions.js`:
- Around line 153-159: The code parses the same GeoJSON twice
(L.geoJSON(route.geometry))—create the geoJSON layer once (e.g., assign to a
variable like routeLayer from L.geoJSON(route.geometry, { style: ... })), add
that layer to the map with routeLayer.addTo(map) and then call
map.fitBounds(routeLayer.getBounds(), { padding: [50,50] }) so you reuse the
layer for both styling and bounds calculations instead of parsing twice.
- Around line 200-208: The code uses a brittle string check
html.indexOf('stop-num">1<') to detect an inserted start header; replace this
with a boolean flag: introduce hasStartHeader (default false), set
hasStartHeader = true at the point where you insert the header in the
legs.forEach (e.g., where legIndex === 0 or where you build the header for the
first leg), then change the final conditional to if (!hasStartHeader &&
validStops.length > 0) to prepend the start header using startStop and
escapeHtml; remove the indexOf('stop-num">1<') check entirely and rely on
hasStartHeader and existing variables html, validStops, startStop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9d5f446-a9cd-40bc-aff5-9f31c3a37f96
⛔ Files ignored due to path filters (1)
Core/Resgrid.Localization/Areas/User/Routes/Routes.en.resxis excluded by!**/*.resx
📒 Files selected for processing (26)
CLAUDE.mdCore/Resgrid.Model/Services/IRouteService.csCore/Resgrid.Services/RouteService.csWeb/Resgrid.Web.Services/Controllers/v4/MappingController.csWeb/Resgrid.Web.Services/Controllers/v4/RoutesController.csWeb/Resgrid.Web.Services/Models/v4/Mapping/GetAllActiveLayersResult.csWeb/Resgrid.Web.Services/Models/v4/Mapping/GetCustomMapsResult.csWeb/Resgrid.Web.Services/Models/v4/Mapping/GetIndoorMapsResult.csWeb/Resgrid.Web.Services/Models/v4/Mapping/SearchAllMapFeaturesResult.csWeb/Resgrid.Web.Services/Models/v4/Routes/RouteInputModels.csWeb/Resgrid.Web.Services/Models/v4/Routes/RouteResultModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/RoutesController.csWeb/Resgrid.Web/Areas/User/Models/Routes/RouteViewModels.csWeb/Resgrid.Web/Areas/User/Views/Routes/ArchivedRoutes.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/ArchivedView.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/Directions.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/StartRoute.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/View.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.directions.jsWeb/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.jsWeb/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.jsWeb/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.view.js
| - **Key Decisions**: bullet list, max 3 items | ||
| - **Next Steps**: bullet list, max 3 items | ||
|
|
||
| Keep `CONTEXT.md` under 20 lines total. Do NOT summarize the full conversation — only what's needed to resume next session. |
Contributor
There was a problem hiding this comment.
Use one English variant consistently in this document.
Line 94 uses “summarize” while other sections use UK spelling (“minimise”, “organisation”). Please normalize to one dialect for consistency.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~94-~94: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ...ONTEXT.md` under 20 lines total. Do NOT summarize the full conversation — only what's nee...
(EN_WORD_COHERENCY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 94, The document uses mixed English variants; normalize to
UK English to match existing words like “minimise” and “organisation” by
replacing “summarize” on line 94 with the UK spelling “summarise” and scan
nearby text for any other US spellings to convert (e.g., "organize" ->
"organise") so the CLAUDE.md uses one consistent dialect.
Comment on lines
+727
to
733
| <member name="M:Resgrid.Web.Services.Controllers.v4.MappingController.SearchCustomMapRegions(System.String,System.String)"> | ||
| <summary> | ||
| Searches for custom map regions matching the specified term. | ||
| </summary> | ||
| <param name="term">Search term</param> | ||
| <returns>SearchCustomMapRegionsResult object</returns> | ||
| </member> |
Contributor
There was a problem hiding this comment.
Document the new filter/input on SearchCustomMapRegions.
This signature also grew to two string parameters, but the XML still only explains term. Add the second <param> entry so consumers can discover the extra filter behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 727 - 733,
Update the XML docs for the method
M:Resgrid.Web.Services.Controllers.v4.MappingController.SearchCustomMapRegions(System.String,System.String)
to include a second <param> element describing the new second string parameter
(the additional filter/input) — add the exact parameter name used in the method
signature and a concise description of what values it expects and how it alters
the search behavior so consumers can discover the extra filter.
Comment on lines
+1117
to
+1158
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.GetStopsForInstance(System.String)"> | ||
| <summary> | ||
| Gets all stops for an active route instance, merging plan data with execution state | ||
| </summary> | ||
| </member> | ||
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.GetStopContact(System.String)"> | ||
| <summary> | ||
| Gets the full contact record for a route stop's assigned contact | ||
| </summary> | ||
| </member> | ||
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.GetRouteContacts(System.String)"> | ||
| <summary> | ||
| Gets all unique contacts attached to stops on a route plan | ||
| </summary> | ||
| </member> | ||
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.GetDirections(System.String)"> | ||
| <summary> | ||
| Gets the stored route geometry and waypoints for a route plan | ||
| </summary> | ||
| </member> | ||
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.GetInstanceDirections(System.String,System.Nullable{System.Decimal},System.Nullable{System.Decimal})"> | ||
| <summary> | ||
| Gets the route geometry and remaining waypoints for an active route instance. | ||
| Pass the unit's current latitude and longitude as query parameters to route from | ||
| the current position rather than the first remaining stop. | ||
| </summary> | ||
| </member> | ||
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.UpdateStopNotes(Resgrid.Web.Services.Models.v4.Routes.UpdateStopNotesInput)"> | ||
| <summary> | ||
| Updates notes on an active route instance stop | ||
| </summary> | ||
| </member> | ||
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.GetActiveRouteInstances"> | ||
| <summary> | ||
| Gets all active route instances across the department with progress details | ||
| </summary> | ||
| </member> | ||
| <member name="M:Resgrid.Web.Services.Controllers.v4.RoutesController.GetScheduledRoutes"> | ||
| <summary> | ||
| Gets all active schedules for the department's route plans | ||
| </summary> | ||
| </member> |
Contributor
There was a problem hiding this comment.
Add <param> docs for these route endpoints.
Several new public methods take route/stop identifiers or origin coordinates, but none of that is surfaced via parameter tags. GetInstanceDirections is the sharpest example because the summary references query parameters without exposing their names in generated API docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 1117 - 1158,
The XML docs are missing <param> entries for the listed route endpoints; add
<param> tags that match each method's signature and describe each parameter
(e.g., for GetStopsForInstance(string) and GetStopContact(string) add a <param
name="..."> describing the route/stop identifier; for GetRouteContacts(string)
and GetDirections(string) add a <param> for the route/plan id; for
GetInstanceDirections(string, Nullable<decimal>, Nullable<decimal>) add three
<param> tags: the instance id (string) and the optional originLatitude and
originLongitude (nullable decimals) with notes that they are query parameters
used to route from the unit's current position; for
UpdateStopNotes(Resgrid.Web.Services.Models.v4.Routes.UpdateStopNotesInput) add
a <param name="input"> describing the input payload. Ensure the <param> names
exactly match the method parameter names in the signatures for methods
GetStopsForInstance, GetStopContact, GetRouteContacts, GetDirections,
GetInstanceDirections, and UpdateStopNotes.
Comment on lines
+6136
to
+6138
| <member name="P:Resgrid.Web.Services.Models.v4.Mapping.MapFeatureResultData.FeatureType"> | ||
| <summary>"indoor_zone" or "custom_region"</summary> | ||
| </member> |
Contributor
There was a problem hiding this comment.
Clarify the FeatureType value set.
The search filter docs use indoor / custom, while this property documents indoor_zone / custom_region. If both are intentional, spell out the mapping; otherwise clients will likely compare against the wrong strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 6136 - 6138,
The XML doc for the FeatureType property
(Resgrid.Web.Services.Models.v4.Mapping.MapFeatureResultData.FeatureType) is
ambiguous: update the <summary> to explicitly state the exact string values
clients should expect and any mapping from the search filter terms (e.g., if
search filter uses "indoor" map to "indoor_zone", and "custom" maps to
"custom_region"), or normalize both docs to the same tokens; ensure the summary
lists both the canonical values (e.g., "indoor_zone" and "custom_region") and
the equivalent filter terms (e.g., "indoor" -> "indoor_zone", "custom" ->
"custom_region") so clients can compare the correct strings.
Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js (2)
367-367:⚠️ Potential issue | 🟡 MinorInconsistent coordinate validation in stop geocoding.
The start/end handlers correctly use
!= nullchecks (e.g., line 210), but the stop geocode handler still uses truthiness which treats0as falsy. This is inconsistent and will fail for coordinates on the equator or prime meridian.💡 Suggested fix
- if (result && result.Data && result.Data.Latitude && result.Data.Longitude) { + if (result && result.Data && result.Data.Latitude != null && result.Data.Longitude != null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js` at line 367, The stop geocode handler's coordinate check uses truthiness (if (result && result.Data && result.Data.Latitude && result.Data.Longitude)) which treats 0 as falsy; update this conditional to use explicit null/undefined checks (e.g., result.Data.Latitude != null && result.Data.Longitude != null) just like the start/end handlers so coordinates of 0 are accepted—locate the condition inside the stop geocode handler (the if referencing result.Data.Latitude/Longitude) and replace the truthy checks with != null checks.
390-390:⚠️ Potential issue | 🟡 MinorSame truthiness issue in stop What3Words handler.
Apply the same fix for consistency with the start/end handlers.
💡 Suggested fix
- if (data && data.Latitude && data.Longitude) { + if (data && data.Latitude != null && data.Longitude != null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js` at line 390, The stop What3Words handler uses a truthy check "if (data && data.Latitude && data.Longitude)" which fails for valid zero values; update the condition in the stop What3Words handler to mirror the start/end handlers by explicitly checking for null/undefined for Latitude and Longitude (e.g., check that data exists and Latitude !== null/undefined and Longitude !== null/undefined) so zero coordinates are accepted and behavior is consistent across handlers.
🧹 Nitpick comments (2)
Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs (1)
41-45: Consider batching stop count queries to avoid N+1 pattern.Each iteration calls
GetRouteStopsForPlanAsync, resulting in N+1 database queries where N is the number of plans. For departments with many route plans, this could degrade performance.Consider adding a service method like
GetStopCountsForPlansAsync(IEnumerable<string> planIds)that retrieves all counts in a single query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs` around lines 41 - 45, The loop over model.Plans causes an N+1 query because each iteration calls _routeService.GetRouteStopsForPlanAsync(plan.RoutePlanId); change this to a single batched call: add a new service method GetStopCountsForPlansAsync(IEnumerable<string> planIds) on _routeService that returns a dictionary/lookup of RoutePlanId -> stop count, call it once with model.Plans.Select(p => p.RoutePlanId) and populate model.StopCounts from the returned map instead of calling GetRouteStopsForPlanAsync per plan.Web/Resgrid.Web/Areas/User/Views/Routes/StartRoute.cshtml (1)
39-51: Consider usingasp-fortag helper for the select element.Using
name="SelectedUnitId"works, but theasp-fortag helper provides automatic client-side validation attributes and better integration with model binding.♻️ Proposed refactor using tag helper
- <div class="form-group @(ViewData.ModelState["SelectedUnitId"]?.Errors.Count > 0 ? "has-error" : "")"> + <div class="form-group @(ViewData.ModelState[nameof(Model.SelectedUnitId)]?.Errors.Count > 0 ? "has-error" : "")"> <label class="col-sm-3 control-label">@localizer["AssignUnit"]</label> <div class="col-sm-9"> - <select name="SelectedUnitId" class="form-control" required> + <select asp-for="SelectedUnitId" class="form-control" required> <option value="">@localizer["SelectUnitPlaceholder"]</option> `@foreach` (var unit in Model.Units) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Routes/StartRoute.cshtml` around lines 39 - 51, Replace the manual select markup for SelectedUnitId with the ASP.NET tag helper approach: change the select to use asp-for="SelectedUnitId" and supply its items via asp-items bound to a SelectList constructed from Model.Units (value = UnitId, text = Name) so model binding and client-side validation attributes are emitted; keep the empty placeholder option by providing an option label on the SelectList or adding a blank option, and also include an asp-validation-for="SelectedUnitId" span to show validation messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js`:
- Line 367: The stop geocode handler's coordinate check uses truthiness (if
(result && result.Data && result.Data.Latitude && result.Data.Longitude)) which
treats 0 as falsy; update this conditional to use explicit null/undefined checks
(e.g., result.Data.Latitude != null && result.Data.Longitude != null) just like
the start/end handlers so coordinates of 0 are accepted—locate the condition
inside the stop geocode handler (the if referencing
result.Data.Latitude/Longitude) and replace the truthy checks with != null
checks.
- Line 390: The stop What3Words handler uses a truthy check "if (data &&
data.Latitude && data.Longitude)" which fails for valid zero values; update the
condition in the stop What3Words handler to mirror the start/end handlers by
explicitly checking for null/undefined for Latitude and Longitude (e.g., check
that data exists and Latitude !== null/undefined and Longitude !==
null/undefined) so zero coordinates are accepted and behavior is consistent
across handlers.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs`:
- Around line 41-45: The loop over model.Plans causes an N+1 query because each
iteration calls _routeService.GetRouteStopsForPlanAsync(plan.RoutePlanId);
change this to a single batched call: add a new service method
GetStopCountsForPlansAsync(IEnumerable<string> planIds) on _routeService that
returns a dictionary/lookup of RoutePlanId -> stop count, call it once with
model.Plans.Select(p => p.RoutePlanId) and populate model.StopCounts from the
returned map instead of calling GetRouteStopsForPlanAsync per plan.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/StartRoute.cshtml`:
- Around line 39-51: Replace the manual select markup for SelectedUnitId with
the ASP.NET tag helper approach: change the select to use
asp-for="SelectedUnitId" and supply its items via asp-items bound to a
SelectList constructed from Model.Units (value = UnitId, text = Name) so model
binding and client-side validation attributes are emitted; keep the empty
placeholder option by providing an option label on the SelectList or adding a
blank option, and also include an asp-validation-for="SelectedUnitId" span to
show validation messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8655402e-76a4-4dd9-b1bd-e7f7e4b7140a
⛔ Files ignored due to path filters (1)
Core/Resgrid.Localization/Areas/User/Routes/Routes.en.resxis excluded by!**/*.resx
📒 Files selected for processing (4)
Web/Resgrid.Web/Areas/User/Controllers/RoutesController.csWeb/Resgrid.Web/Areas/User/Views/Routes/StartRoute.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.jsWeb/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js
🚧 Files skipped from review as they are similar to previous changes (1)
- Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation