Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds protobuf serialization support for plan-addon models and introduces defensive validation and error handling across services, controllers, and views to handle null/empty input parameters and improve HTTP error diagnostics. ChangesProtobuf Serialization Setup
Defensive Validation & Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/Areas/User/Controllers/CustomMapsController.cs (1)
274-310:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate that the selected layer belongs to the requested map before importing.
Line 280 only checks presence. A crafted request can still submit a
layerIdfrom another map, and Line 300/305/309 will import into that foreign layer.Suggested fix
public async Task<IActionResult> ImportUpload(string mapId, string layerId, IFormFile importFile, CancellationToken cancellationToken) { var map = await _customMapService.GetCustomMapByIdAsync(mapId); if (map == null || map.DepartmentId != DepartmentId) return RedirectToAction("Index"); if (string.IsNullOrWhiteSpace(layerId)) { var model = new CustomMapImportView(); model.Map = map; model.Layers = await _customMapService.GetLayersForMapAsync(mapId); model.Imports = await _customMapService.GetImportsForMapAsync(mapId); model.Message = "Please select a target layer before importing. Create a layer first if none exist."; return View("Import", model); } + + var selectedLayer = await _customMapService.GetLayerByIdAsync(layerId); + if (selectedLayer == null || !string.Equals(selectedLayer.IndoorMapId, mapId, StringComparison.Ordinal)) + { + var model = new CustomMapImportView(); + model.Map = map; + model.Layers = await _customMapService.GetLayersForMapAsync(mapId); + model.Imports = await _customMapService.GetImportsForMapAsync(mapId); + model.Message = "Please select a valid target layer for this map."; + return View("Import", model); + } if (importFile == null || importFile.Length == 0) return RedirectToAction("Import", new { id = mapId });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/CustomMapsController.cs` around lines 274 - 310, In ImportUpload, validate that the provided layerId actually belongs to the map before performing any imports: after loading map (variable map) and before processing importFile, fetch/verify the layer via the service (e.g., _customMapService.GetLayerByIdAsync(layerId) or by checking layerId exists in await _customMapService.GetLayersForMapAsync(mapId)), and if the layer is null or its MapId/CustomMapId doesn't match mapId (or DepartmentId), return RedirectToAction("Import", new { id = mapId }) or show the same “select a target layer” view; ensure this check occurs prior to any calls to _customMapService.ImportGeoJsonAsync, ImportKmlAsync, etc., to prevent importing into an unrelated layer.
🧹 Nitpick comments (2)
Web/Resgrid.Web/Areas/User/Controllers/TemplatesController.cs (1)
73-82: ⚡ Quick winReuse
PopulateDropdownsin GET actions to remove remaining duplication.Now that this helper exists, call it from
New()andEdit(int id)too, so all dropdown construction stays in one place.♻️ Suggested refactor
public async Task<IActionResult> New() { var model = new NewTemplateModel(); model.Template = new CallQuickTemplate(); - - var priorites = await _callsService.GetActiveCallPrioritiesForDepartmentAsync(DepartmentId); - model.CallPriorities = new SelectList(priorites, "DepartmentCallPriorityId", "Name", priorites.FirstOrDefault(x => x.IsDefault)); - - - List<CallType> types = new List<CallType>(); - types.Add(new CallType { CallTypeId = 0, Type = "No Type" }); - types.AddRange(await _callsService.GetCallTypesForDepartmentAsync(DepartmentId)); - model.CallTypes = new SelectList(types, "Type", "Type"); + await PopulateDropdowns(model); return View(model); }public async Task<IActionResult> Edit(int id) { var model = new NewTemplateModel(); model.Template = new CallQuickTemplate(); model.Template = await _templatesService.GetCallQuickTemplateByIdAsync(id); if (model.Template == null || model.Template.DepartmentId != DepartmentId) return Unauthorized(); - - var priorites = await _callsService.GetActiveCallPrioritiesForDepartmentAsync(DepartmentId); - model.CallPriorities = new SelectList(priorites, "DepartmentCallPriorityId", "Name", priorites.FirstOrDefault(x => x.IsDefault)); - - - List<CallType> types = new List<CallType>(); - types.Add(new CallType { CallTypeId = 0, Type = "No Type" }); - types.AddRange(await _callsService.GetCallTypesForDepartmentAsync(DepartmentId)); - model.CallTypes = new SelectList(types, "Type", "Type"); + await PopulateDropdowns(model); return View(model); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/TemplatesController.cs` around lines 73 - 82, The NewTemplateModel dropdown population is duplicated in the GET actions; reuse the existing PopulateDropdowns(NewTemplateModel model) helper by calling it from the New() GET and Edit(int id) GET actions after creating/loading the model and before returning the view. Update TemplatesController methods New() and Edit(int id) to await PopulateDropdowns(model) so CallPriorities and CallTypes are built in one place (refer to PopulateDropdowns and the New and Edit(int id) action methods).Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs (1)
672-673: ⚡ Quick winConsider returning
BadRequest()instead of a silent redirect for malformed POST input.The null-conditional
model?.UserIdcorrectly collapses both a null model and a null/whitespaceUserIdinto one check. However, silently redirecting toIndexon a POST with malformed data hides the error from the caller — especially relevant for any non-browser client that expects HTTP error semantics. A400 Bad Requestwould be more appropriate here.♻️ Suggested change
- if (string.IsNullOrWhiteSpace(model?.UserId)) - return RedirectToAction("Index", "Personnel", new { area = "User" }); + if (model == null || string.IsNullOrWhiteSpace(model.UserId)) + return BadRequest();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs` around lines 672 - 673, The POST handler currently checks if (string.IsNullOrWhiteSpace(model?.UserId)) and performs a RedirectToAction("Index", "Personnel", new { area = "User" }); — change this to return BadRequest() so malformed POST input returns HTTP 400 instead of silently redirecting; update the action method in PersonnelController where the model?.UserId check exists to return BadRequest() (or BadRequest(ModelState) if you prefer) and keep the null/whitespace check logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Model/PlanAddon.cs`:
- Around line 31-32: Update the PlanAddon.GetExternalKey() logic to treat the
new TestExternalId as a fallback when the normal external key is null/empty in
test scenarios: inside the GetExternalKey() method (class PlanAddon) check if
the primary external key is null/empty and if so return TestExternalId (if
non-empty) before returning null; ensure this only acts as a fallback
(preserving the original external id precedence) so production behavior remains
unchanged.
In `@Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs`:
- Line 101: Replace the System.Diagnostics.Debug.WriteLine(errorMsg) call in
NwsWeatherAlertProvider with the Resgrid logging API: call
Resgrid.Framework.Logging.LogError(...) (or LogException(...) if you have an
Exception instance) passing the same error message and any available
exception/context so the error goes through centralized logging; look for the
Debug.WriteLine(errorMsg) usage in the class/method and swap it to
Resgrid.Framework.Logging.LogError(errorMsg) (or LogException(exception,
errorMsg)) accordingly.
- Line 83: The call to response.Content.ReadAsStringAsync() can NRE if
response.Content is null; in the NwsWeatherAlertProvider (where response and
json are used) first check response.Content != null before calling
ReadAsStringAsync(), e.g. set json to string.Empty or null when Content is null
and then proceed with the existing status-specific handling; update any
downstream logic that assumes json non-null (variables: response,
response.Content, json) so null/empty content is handled gracefully.
In `@Web/Resgrid.Web/Areas/User/Controllers/CustomMapsController.cs`:
- Line 286: The hard-coded message assigned to model.Message in
CustomMapsController should be replaced with a localized resource lookup: add a
new resource key (e.g., "PleaseSelectTargetLayer") to the project's resource
file(s) and use the controller's existing localization mechanism (the same
pattern used elsewhere in this controller/page, e.g.,
_localizer["PleaseSelectTargetLayer"] or Resources.PleaseSelectTargetLayer) when
assigning model.Message so the string is retrieved from resources instead of
being hard-coded.
In `@Web/Resgrid.Web/Areas/User/Controllers/TemplatesController.cs`:
- Around line 75-77: The SelectList is being constructed with the selectedValue
set to an object (priorites.FirstOrDefault(x => x.IsDefault)) instead of the
scalar key; change the selectedValue to the default item's
DepartmentCallPriorityId. Locate the call that assigns model.CallPriorities
(using _callsService.GetActiveCallPrioritiesForDepartmentAsync and the priorites
variable) and pass priorites.FirstOrDefault(x =>
x.IsDefault)?.DepartmentCallPriorityId as the fourth parameter to new
SelectList(...) so the selected value matches the "DepartmentCallPriorityId"
value field.
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/CustomMapsController.cs`:
- Around line 274-310: In ImportUpload, validate that the provided layerId
actually belongs to the map before performing any imports: after loading map
(variable map) and before processing importFile, fetch/verify the layer via the
service (e.g., _customMapService.GetLayerByIdAsync(layerId) or by checking
layerId exists in await _customMapService.GetLayersForMapAsync(mapId)), and if
the layer is null or its MapId/CustomMapId doesn't match mapId (or
DepartmentId), return RedirectToAction("Import", new { id = mapId }) or show the
same “select a target layer” view; ensure this check occurs prior to any calls
to _customMapService.ImportGeoJsonAsync, ImportKmlAsync, etc., to prevent
importing into an unrelated layer.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs`:
- Around line 672-673: The POST handler currently checks if
(string.IsNullOrWhiteSpace(model?.UserId)) and performs a
RedirectToAction("Index", "Personnel", new { area = "User" }); — change this to
return BadRequest() so malformed POST input returns HTTP 400 instead of silently
redirecting; update the action method in PersonnelController where the
model?.UserId check exists to return BadRequest() (or BadRequest(ModelState) if
you prefer) and keep the null/whitespace check logic unchanged.
In `@Web/Resgrid.Web/Areas/User/Controllers/TemplatesController.cs`:
- Around line 73-82: The NewTemplateModel dropdown population is duplicated in
the GET actions; reuse the existing PopulateDropdowns(NewTemplateModel model)
helper by calling it from the New() GET and Edit(int id) GET actions after
creating/loading the model and before returning the view. Update
TemplatesController methods New() and Edit(int id) to await
PopulateDropdowns(model) so CallPriorities and CallTypes are built in one place
(refer to PopulateDropdowns and the New and Edit(int id) action methods).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4772444c-a7db-4e4a-a6ba-02335b009492
⛔ Files ignored due to path filters (2)
Core/Resgrid.Localization/Areas/User/CustomMaps/CustomMaps.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomMaps/CustomMaps.en.resxis excluded by!**/*.resx
📒 Files selected for processing (9)
Core/Resgrid.Model/Helpers/SerializerHelper.csCore/Resgrid.Model/PlanAddon.csCore/Resgrid.Services/CustomMapService.csCore/Resgrid.Services/UsersService.csProviders/Resgrid.Providers.Weather/NwsWeatherAlertProvider.csWeb/Resgrid.Web/Areas/User/Controllers/CustomMapsController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/TemplatesController.csWeb/Resgrid.Web/Areas/User/Views/CustomMaps/Import.cshtml
Member
Author
|
Approve |
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
Bug Fixes
Improvements