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 225 billable files and costs up to $56.25. View limit details
Comment |
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}
|
|
||
| private static string EncodeAngleBrackets(string text) => text.Replace("<", "<").Replace(">", ">"); | ||
|
|
||
| private static readonly Regex HtmlTagToken = new Regex(@"<!--|<!doctype|<(/?)([a-zA-Z][\w:.-]*)(\s[^<>]*)?/?>", |
There was a problem hiding this comment.
Regex processing without a timeout can allow crafted HTML input to consume excessive CPU and cause a denial-of-service condition. Configure a timeout for HtmlTagToken and the other Regex instances at Core/Resgrid.Framework/StringHelpers.cs:191, 194, and 316, Core/Resgrid.Model/CallNumbering.cs:258, and the listed test files.
Kody rule violation: Specify Timeout for Regular Expressions
Prompt for LLM
File Core/Resgrid.Framework/StringHelpers.cs:
Line 174:
Regex processing without a timeout can allow crafted HTML input to consume excessive CPU and cause a denial-of-service condition. Configure a timeout for HtmlTagToken and the other Regex instances at Core/Resgrid.Framework/StringHelpers.cs:191, 194, and 316, Core/Resgrid.Model/CallNumbering.cs:258, and the listed test files.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var cleanHtml = SanitizeHtml(source); | ||
| if (cleanHtml == null) | ||
| { | ||
| Logging.LogError("Following string could not be stripped of Word HTML: " + source); |
There was a problem hiding this comment.
Logging.LogError concatenates source into the message, potentially exposing sensitive input and producing unstructured logs. Emit a structured error containing the operation name and a non-sensitive identifier, including the occurrences in Web/Resgrid.Web/Areas/User/Controllers/DisclosuresController.cs:78 and Core/Resgrid.Framework/StringHelpers.cs:121 and 145.
Kody rule violation: Include error context in structured logs
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", sourceId = source.GetHashCode() });Prompt for LLM
File Core/Resgrid.Framework/StringHelpers.cs:
Line 85:
Logging.LogError concatenates source into the message, potentially exposing sensitive input and producing unstructured logs. Emit a structured error containing the operation name and a non-sensitive identifier, including the occurrences in Web/Resgrid.Web/Areas/User/Controllers/DisclosuresController.cs:78 and Core/Resgrid.Framework/StringHelpers.cs:121 and 145.
Suggested Code:
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", sourceId = source.GetHashCode() });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var cleanHtml = SanitizeHtml(source); | ||
| if (cleanHtml == null) | ||
| { | ||
| Logging.LogError("Following string could not be stripped of Word HTML: " + source); |
There was a problem hiding this comment.
Logging.LogError writes the raw source text, which may contain personal data or secrets. Log only a consistent hash or another non-sensitive identifier.
Kody rule violation: Mask PII and secrets in logs
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", sourceHash = Hash(source) });Prompt for LLM
File Core/Resgrid.Framework/StringHelpers.cs:
Line 85:
Logging.LogError writes the raw source text, which may contain personal data or secrets. Log only a consistent hash or another non-sensitive identifier.
Suggested Code:
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", sourceHash = Hash(source) });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var cleanHtml = SanitizeHtml(source); | ||
| if (cleanHtml == null) | ||
| { | ||
| Logging.LogError("Following string could not be stripped of Word HTML: " + source); |
There was a problem hiding this comment.
Logging.LogError writes rich text that may contain PHI, creating a sensitive-data exposure in application logs. Remove the raw source and retain only non-identifying troubleshooting metadata such as traceId.
Kody rule violation: Do not log PHI; mask and drop sensitive fields
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", traceId });Prompt for LLM
File Core/Resgrid.Framework/StringHelpers.cs:
Line 85:
Logging.LogError writes rich text that may contain PHI, creating a sensitive-data exposure in application logs. Remove the raw source and retain only non-identifying troubleshooting metadata such as traceId.
Suggested Code:
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", traceId });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var cleanHtml = SanitizeHtml(source); | ||
| if (cleanHtml == null) | ||
| { | ||
| Logging.LogError("Following string could not be stripped of Word HTML: " + source); |
There was a problem hiding this comment.
Logging.LogError writes the raw source without the diagnostic purpose or lawful-basis metadata required for sensitive-data processing. Redact or hash source and attach purpose="diagnostics" and lawful_basis="legitimate_interest" metadata.
Kody rule violation: Redact PII in logs and metrics by default
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", sourceHash = Hash(source), gdpr = new { purpose = "diagnostics", lawful_basis = "legitimate_interest" } });Prompt for LLM
File Core/Resgrid.Framework/StringHelpers.cs:
Line 85:
Logging.LogError writes the raw source without the diagnostic purpose or lawful-basis metadata required for sensitive-data processing. Redact or hash source and attach purpose="diagnostics" and lawful_basis="legitimate_interest" metadata.
Suggested Code:
Logging.LogError("HTML sanitization failed", new { operation = "SanitizeHtmlInString", sourceHash = Hash(source), gdpr = new { purpose = "diagnostics", lawful_basis = "legitimate_interest" } });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| private static int Group(Match match, string name) | ||
| { | ||
| return match.Groups[name].Success ? int.Parse(match.Groups[name].Value, NumberStyles.None, CultureInfo.InvariantCulture) : 0; |
There was a problem hiding this comment.
int.Parse on user or I/O input can throw a FormatException when the capture value is invalid. Use a TryParse-style API with NumberStyles.None and CultureInfo.InvariantCulture, and handle parsing failure explicitly.
Kody rule violation: Use TryParse for string conversions
Prompt for LLM
File Core/Resgrid.Model/CallNumbering.cs:
Line 301:
int.Parse on user or I/O input can throw a FormatException when the capture value is invalid. Use a TryParse-style API with NumberStyles.None and CultureInfo.InvariantCulture, and handle parsing failure explicitly.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Task<IEnumerable<Bid>> GetForDepartmentAsync(int departmentId, int? status, int skip, int take); | ||
| Task<int> CountForDepartmentAsync(int departmentId, int? status); | ||
| /// <summary>Newest number first; <paramref name="number"/> (optional) is part of the bid number as issued (IdentifierSearchKeys.ContainsParts).</summary> | ||
| Task<IEnumerable<Bid>> GetForDepartmentAsync(int departmentId, int? status, string number, int skip, int take); |
There was a problem hiding this comment.
Adding the required number parameter to IContractorRepositories.GetForDepartmentAsync changes the interface contract and may break existing implementations and callers. Add a BREAKING CHANGE section documenting affected consumers and migration steps for supplying the new number argument.
Kody rule violation: Call out breaking changes explicitly
Task<IEnumerable<Bid>> GetForDepartmentAsync(int departmentId, int? status, string number, int skip, int take);Prompt for LLM
File Core/Resgrid.Model/Repositories/IContractorRepositories.cs:
Line 65:
Adding the required number parameter to IContractorRepositories.GetForDepartmentAsync changes the interface contract and may break existing implementations and callers. Add a BREAKING CHANGE section documenting affected consumers and migration steps for supplying the new number argument.
Suggested Code:
Task<IEnumerable<Bid>> GetForDepartmentAsync(int departmentId, int? status, string number, int skip, int take);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public async Task<Inventory> SaveInventoryAsync(Inventory inventory, CancellationToken cancellationToken = default(CancellationToken)) | ||
| { | ||
| await GuardLegacyWriteAsync(inventory.DepartmentId); | ||
| inventory.Note = StringHelpers.SanitizeHtmlForStorage(inventory.Note); |
There was a problem hiding this comment.
Calling synchronous StringHelpers.SanitizeHtmlForStorage inside an async method can perform CPU-bound or blocking work on the request thread and reduce asynchronous scalability. Provide and await StringHelpers.SanitizeHtmlForStorageAsync, or explicitly move the CPU-bound operation off the request thread.
Kody rule violation: Use Awaitable Methods in Async Code
inventory.Note = await StringHelpers.SanitizeHtmlForStorageAsync(inventory.Note);Prompt for LLM
File Core/Resgrid.Services/InventoryService.cs:
Line 60:
Calling synchronous StringHelpers.SanitizeHtmlForStorage inside an async method can perform CPU-bound or blocking work on the request thread and reduce asynchronous scalability. Provide and await StringHelpers.SanitizeHtmlForStorageAsync, or explicitly move the CPU-bound operation off the request thread.
Suggested Code:
inventory.Note = await StringHelpers.SanitizeHtmlForStorageAsync(inventory.Note);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public static NumberingYearWindow YearWindow(RecordsNumberingConfig config, int year, string timeZone) | ||
| { | ||
| var start = (config ?? new RecordsNumberingConfig()).YearStart(); | ||
| timeZone = KnownZone(timeZone); | ||
| return new NumberingYearWindow(year, LocalToUtc(start.StartOfYear(year), timeZone), LocalToUtc(start.StartOfYear(year + 1), timeZone)); |
There was a problem hiding this comment.
YearWindow constructs the next boundary from year + 1 without validating the requested year, so public records search controllers passing user-supplied int year values such as 9999 or 0 can trigger DateTime/numbering-year range exceptions and return server errors instead of controlled empty or invalid-request responses. Validate year before constructing the window, requiring 1 <= year < 9999 or rejecting the request at the controller/service boundary with a controlled validation result.
if (year < 1 || year >= DateTime.MaxValue.Year) throw new ArgumentOutOfRangeException(nameof(year));
return new NumberingYearWindow(year, LocalToUtc(start.StartOfYear(year), timeZone), LocalToUtc(start.StartOfYear(year + 1), timeZone));Prompt for LLM
File Core/Resgrid.Services/Records/RecordsNumberingService.cs:
Line 58 to 62:
YearWindow constructs the next boundary from year + 1 without validating the requested year, so public records search controllers passing user-supplied int year values such as 9999 or 0 can trigger DateTime/numbering-year range exceptions and return server errors instead of controlled empty or invalid-request responses. Validate year before constructing the window, requiring 1 <= year < 9999 or rejecting the request at the controller/service boundary with a controlled validation result.
Suggested Code:
if (year < 1 || year >= DateTime.MaxValue.Year) throw new ArgumentOutOfRangeException(nameof(year));
return new NumberingYearWindow(year, LocalToUtc(start.StartOfYear(year), timeZone), LocalToUtc(start.StartOfYear(year + 1), timeZone));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| text = System.Net.WebUtility.HtmlDecode(text); | ||
| return Whitespace.Replace(text, " ").Trim(); | ||
| var text = StringHelpers.HtmlToPlainText(html); | ||
| return text.Length == 0 ? null : text; |
There was a problem hiding this comment.
Accessing text.Length before checking for null can throw a NullReferenceException when the helper returns null. Use string.IsNullOrEmpty or an equivalent null-safe check so Strip handles empty and null results safely.
Kody rule violation: Add null checks before accessing properties
return string.IsNullOrEmpty(text) ? null : text;Prompt for LLM
File Core/Resgrid.Services/Search/SearchProjectionService.cs:
Line 1115:
Accessing text.Length before checking for null can throw a NullReferenceException when the helper returns null. Use string.IsNullOrEmpty or an equivalent null-safe check so Strip handles empty and null results safely.
Suggested Code:
return string.IsNullOrEmpty(text) ? null : text;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| return QueryAsync<int>( | ||
| $"SELECT DISTINCT {YearOf(Col("CreatedOn"))} AS Y FROM {Tbl("RmsIncidentReports")} WHERE {Col("DepartmentId")} = {P}DepartmentId AND {Col("DeletedOn")} IS NULL ORDER BY Y DESC", | ||
| $"SELECT DISTINCT {YearOf($"COALESCE({Col("CallCreatedOn")}, {Col("CreatedOn")})")} AS Y FROM {Tbl("RmsIncidentReports")} WHERE {Col("DepartmentId")} = {P}DepartmentId AND {Col("DeletedOn")} IS NULL ORDER BY Y DESC", |
There was a problem hiding this comment.
The QueryAsync call in Repositories/Resgrid.Repositories.DataRepository/RmsIncidentRepositories.cs:199 propagates database failures without department context. Wrap the query in try/catch and rethrow a DataException that identifies the department while preserving the original exception.
Kody rule violation: Add try-catch blocks for external calls
try
{
return await QueryAsync<int>(
$"SELECT DISTINCT {YearOf($"COALESCE({Col("CallCreatedOn")}, {Col("CreatedOn")})")} AS Y FROM {Tbl("RmsIncidentReports")} WHERE {Col("DepartmentId")} = {P}DepartmentId AND {Col("DeletedOn")} IS NULL ORDER BY Y DESC",
new { DepartmentId = departmentId });
}
catch (Exception ex)
{
throw new DataException($"Failed to retrieve numbering years for department {departmentId}.", ex);
}Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/RmsIncidentRepositories.cs:
Line 193:
The QueryAsync call in Repositories/Resgrid.Repositories.DataRepository/RmsIncidentRepositories.cs:199 propagates database failures without department context. Wrap the query in try/catch and rethrow a DataException that identifies the department while preserving the original exception.
Suggested Code:
try
{
return await QueryAsync<int>(
$"SELECT DISTINCT {YearOf($"COALESCE({Col("CallCreatedOn")}, {Col("CreatedOn")})")} AS Y FROM {Tbl("RmsIncidentReports")} WHERE {Col("DepartmentId")} = {P}DepartmentId AND {Col("DeletedOn")} IS NULL ORDER BY Y DESC",
new { DepartmentId = departmentId });
}
catch (Exception ex)
{
throw new DataException($"Failed to retrieve numbering years for department {departmentId}.", ex);
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| result.Should().Be("<p>Fire at Main</p>"); | ||
| } | ||
|
|
||
| [TestCase("<p onclick=\"alert(1)\">a<script>alert(2)</script>b</p><img src=\"x\" onerror=\"alert(3)\"><iframe src=\"https://evil.example\">i</iframe>")] |
There was a problem hiding this comment.
The test input contains plain markup, which violates the project rule requiring Next.js Image with explicit dimensions and meaningful alt text for app assets. Replace the plain image markup in the listed test and JavaScript locations with the approved image component or test fixture.
Kody rule violation: Use next/image with explicit dimensions and alt
Prompt for LLM
File Tests/Resgrid.Tests/Framework/StringHelperTests.cs:
Line 211:
The test input contains plain <img> markup, which violates the project rule requiring Next.js Image with explicit dimensions and meaningful alt text for app assets. Replace the plain image markup in the listed test and JavaScript locations with the approved image component or test fixture.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
|
|
||
| [TestCase("<x-el autofocus onfocus=\"alert(1)\">hi</x-el><p>ok</p>")] | ||
| [TestCase("<unknown tabindex=1 onfocus=alert(1)>hi</unknown> ok")] |
There was a problem hiding this comment.
The test input uses tabindex=1, and positive tabIndex values disrupt the natural keyboard navigation order. Use tabindex=0 or tabindex=-1 when testing focus behavior.
Kody rule violation: Use appropriate tabIndex values
Prompt for LLM
File Tests/Resgrid.Tests/Framework/StringHelperTests.cs:
Line 254:
The test input uses tabindex=1, and positive tabIndex values disrupt the natural keyboard navigation order. Use tabindex=0 or tabindex=-1 when testing focus behavior.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| DataConfig.DatabaseType = type; | ||
| _database = Prefix + Guid.NewGuid().ToString("N"); | ||
| await using (var master = Connect(_master)) | ||
| await master.ExecuteAsync("CREATE DATABASE " + _database); |
There was a problem hiding this comment.
The CREATE DATABASE statement concatenates unsanitized _database input, allowing SQL injection if the database name is attacker-controlled. Use a safe identifier-quoting mechanism or a validated allowlist before executing the query at Tests/Resgrid.Tests/Repositories/NumberFilterDatabaseTests.cs:136-137.
Kody rule violation: Prevent SQL Injection in Queries
Prompt for LLM
File Tests/Resgrid.Tests/Repositories/NumberFilterDatabaseTests.cs:
Line 65:
The CREATE DATABASE statement concatenates unsanitized _database input, allowing SQL injection if the database name is attacker-controlled. Use a safe identifier-quoting mechanism or a validated allowlist before executing the query at Tests/Resgrid.Tests/Repositories/NumberFilterDatabaseTests.cs:136-137.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| public Task<RmsInspection> GetByIdForDepartmentAsync(int departmentId, string inspectionId) => Task.FromResult(Rows.FirstOrDefault(i => i.DepartmentId == departmentId && i.RmsInspectionId == inspectionId)); | ||
| private IEnumerable<RmsInspection> Filter(int departmentId, RmsInspectionQuery q) => Live(departmentId).Where(i => (q.OccupancyId == null || i.RmsOccupancyId == q.OccupancyId) && (q.ProgramId == null || i.RmsInspectionProgramId == q.ProgramId) && (q.States == null || q.States.Count == 0 || q.States.Contains(i.State)) && (q.ScheduledBefore == null || i.ScheduledOn <= q.ScheduledBefore)); | ||
| private IEnumerable<RmsInspection> Filter(int departmentId, RmsInspectionQuery q) => Live(departmentId).Where(i => (q.OccupancyId == null || i.RmsOccupancyId == q.OccupancyId) && (q.ProgramId == null || i.RmsInspectionProgramId == q.ProgramId) && (q.States == null || q.States.Count == 0 || q.States.Contains(i.State)) && (q.ScheduledBefore == null || i.ScheduledOn <= q.ScheduledBefore) && (q.Number == null || IdentifierSearchKeys.ContainsParts(i.InspectionNumber, q.Number))); |
There was a problem hiding this comment.
The Filter method in Tests/Resgrid.Tests/Rms/RmsPreventionFakes.cs:202 combines occupancy, program, state, scheduling, and number predicates in one LINQ expression, making individual filtering rules difficult to review and maintain. Split the expression into named intermediate predicates or query stages.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Tests/Resgrid.Tests/Rms/RmsPreventionFakes.cs:
Line 133:
The Filter method in Tests/Resgrid.Tests/Rms/RmsPreventionFakes.cs:202 combines occupancy, program, state, scheduling, and number predicates in one LINQ expression, making individual filtering rules difficult to review and maintain. Split the expression into named intermediate predicates or query stages.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| private static string WebRoot() | ||
| { | ||
| var directory = new DirectoryInfo(TestContext.CurrentContext.TestDirectory); |
There was a problem hiding this comment.
The DirectoryInfo instance in Tests/Resgrid.Tests/Security/StoredHtmlRenderingTests.cs is not disposed deterministically. Dispose it with using or another explicit cleanup path.
Kody rule violation: Use using statements for disposable resources
using (DirectoryInfo directory = new DirectoryInfo(TestContext.CurrentContext.TestDirectory))Prompt for LLM
File Tests/Resgrid.Tests/Security/StoredHtmlRenderingTests.cs:
Line 45:
The DirectoryInfo instance in Tests/Resgrid.Tests/Security/StoredHtmlRenderingTests.cs is not disposed deterministically. Dispose it with using or another explicit cleanup path.
Suggested Code:
using (DirectoryInfo directory = new DirectoryInfo(TestContext.CurrentContext.TestDirectory))
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (!await EnabledAsync()) return Failed<BidsResult>("contractor_billing_disabled", StatusCodes.Status403Forbidden); | ||
| var bids = string.IsNullOrWhiteSpace(contactId) | ||
| ? await _bids.GetBidsForDepartmentAsync(DepartmentId, status.HasValue && Enum.IsDefined(typeof(BidStatuses), status.Value) ? (BidStatuses?)status.Value : null, skip, take) | ||
| ? await _bids.GetBidsForDepartmentAsync(DepartmentId, status.HasValue && Enum.IsDefined(typeof(BidStatuses), status.Value) ? (BidStatuses?)status.Value : null, number, skip, take) |
There was a problem hiding this comment.
The awaited _bids.GetBidsForDepartmentAsync operation can propagate task failures without controller context, leaving promise or task rejections unhandled. Guard the operation with try/catch and handle or propagate failures appropriately, using ConfigureAwait(false) where the library context does not need to be resumed.
Kody rule violation: Handle async operations with proper error handling
? await _bids.GetBidsForDepartmentAsync(DepartmentId, status.HasValue && Enum.IsDefined(typeof(BidStatuses), status.Value) ? (BidStatuses?)status.Value : null, number, skip, take).ConfigureAwait(false)Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/BidsController.cs:
Line 83:
The awaited _bids.GetBidsForDepartmentAsync operation can propagate task failures without controller context, leaving promise or task rejections unhandled. Guard the operation with try/catch and handle or propagate failures appropriately, using ConfigureAwait(false) where the library context does not need to be resumed.
Suggested Code:
? await _bids.GetBidsForDepartmentAsync(DepartmentId, status.HasValue && Enum.IsDefined(typeof(BidStatuses), status.Value) ? (BidStatuses?)status.Value : null, number, skip, take).ConfigureAwait(false)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| var states = state.HasValue ? new List<RmsDisclosureState> { (RmsDisclosureState)state.Value } : null; | ||
| var requests = await _disclosures.QueryAsync(DepartmentId, UserId, states, Math.Max(0, skip), Math.Max(1, Math.Min(RecordsController.MaxPageSize, take))); | ||
| var requests = await _disclosures.QueryAsync(DepartmentId, UserId, states, Math.Max(0, skip), Math.Max(1, Math.Min(RecordsController.MaxPageSize, take)), number); |
There was a problem hiding this comment.
The new number parameter reaches the database-backed QueryAsync call without length validation, allowing oversized input to consume resources or violate downstream constraints. Validate and sanitize number in Web/Resgrid.Web.Services/Controllers/v4/DisclosuresController.cs and return a client error when it exceeds MaxDisclosureNumberLength.
Kody rule violation: Order validations before database queries
if (number != null && number.Length > MaxDisclosureNumberLength)
return BadRequest();
var requests = await _disclosures.QueryAsync(DepartmentId, UserId, states, Math.Max(0, skip), Math.Max(1, Math.Min(RecordsController.MaxPageSize, take)), number);Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/DisclosuresController.cs:
Line 58:
The new number parameter reaches the database-backed QueryAsync call without length validation, allowing oversized input to consume resources or violate downstream constraints. Validate and sanitize number in Web/Resgrid.Web.Services/Controllers/v4/DisclosuresController.cs and return a client error when it exceeds MaxDisclosureNumberLength.
Suggested Code:
if (number != null && number.Length > MaxDisclosureNumberLength)
return BadRequest();
var requests = await _disclosures.QueryAsync(DepartmentId, UserId, states, Math.Max(0, skip), Math.Max(1, Math.Min(RecordsController.MaxPageSize, take)), number);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <a id="newCallButton" title="@localizer["NewCallHelp"]" class="btn btn-success" href="@Url.Action("NewCall", "Dispatch", new { area = "User" })">@localizer["NewCall"]</a> | ||
| } | ||
| </div> | ||
| <div class="btn-group top-page-buttons buttons-actions" style="float:right;padding-right:15px;"> |
There was a problem hiding this comment.
The inline style in Web/Resgrid.Web/Areas/User/Views/Dispatch/Dashboard.cshtml hard-codes layout styling in the view and bypasses scoped styling conventions. Move the styling to a scoped stylesheet or component-scoped class.
Kody rule violation: Use component-scoped styling
<div class="btn-group top-page-buttons buttons-actions">Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Dispatch/Dashboard.cshtml:
Line 45:
The inline style in Web/Resgrid.Web/Areas/User/Views/Dispatch/Dashboard.cshtml hard-codes layout styling in the view and bypasses scoped styling conventions. Move the styling to a scoped stylesheet or component-scoped class.
Suggested Code:
<div class="btn-group top-page-buttons buttons-actions">
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var PERSONNEL_ETA_COLUMN = 4; | ||
| var PERSONNEL_BOARD_COLUMNS = [1, 4, 5]; | ||
| var UNIT_ETA_COLUMN = 3; | ||
| var UNIT_BOARD_COLUMNS = [1, 3, 4, 5, 6]; |
There was a problem hiding this comment.
PERSONNEL_ETA_COLUMN, PERSONNEL_BOARD_COLUMNS, UNIT_ETA_COLUMN, and UNIT_BOARD_COLUMNS are compile-time bindings that are never reassigned. Declare them with const in Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.nearestboard.js.
Kody rule violation: Use `readonly` or `const` for Immutable Data
const PERSONNEL_ETA_COLUMN = 4;
const PERSONNEL_BOARD_COLUMNS = [1, 4, 5];
const UNIT_ETA_COLUMN = 3;
const UNIT_BOARD_COLUMNS = [1, 3, 4, 5, 6];Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.nearestboard.js:
Line 42 to 45:
PERSONNEL_ETA_COLUMN, PERSONNEL_BOARD_COLUMNS, UNIT_ETA_COLUMN, and UNIT_BOARD_COLUMNS are compile-time bindings that are never reassigned. Declare them with const in Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.nearestboard.js.
Suggested Code:
const PERSONNEL_ETA_COLUMN = 4;
const PERSONNEL_BOARD_COLUMNS = [1, 4, 5];
const UNIT_ETA_COLUMN = 3;
const UNIT_BOARD_COLUMNS = [1, 3, 4, 5, 6];
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| var html = ''; | ||
| if (row.TypeIconUrl) { | ||
| html += '<img src="' + escapeAttr(row.TypeIconUrl) + '" alt="" aria-hidden="true" class="rg-type-icon" style="width:20px;height:20px;object-fit:contain;vertical-align:middle;margin-right:4px;" />'; |
There was a problem hiding this comment.
The generated image in Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.nearestboard.js lacks lazy loading, asynchronous decoding, and intrinsic dimensions, which can delay rendering and contribute to layout shifts. Add loading="lazy", decoding="async", and explicit width="20" and height="20" attributes.
Kody rule violation: Serve responsive images with modern formats and lazy-load
html += '<img src="' + escapeAttr(row.TypeIconUrl) + '" alt="" aria-hidden="true" loading="lazy" decoding="async" width="20" height="20" class="rg-type-icon" style="object-fit:contain;vertical-align:middle;margin-right:4px;" />';Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.nearestboard.js:
Line 284:
The generated image in Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.nearestboard.js lacks lazy loading, asynchronous decoding, and intrinsic dimensions, which can delay rendering and contribute to layout shifts. Add loading="lazy", decoding="async", and explicit width="20" and height="20" attributes.
Suggested Code:
html += '<img src="' + escapeAttr(row.TypeIconUrl) + '" alt="" aria-hidden="true" loading="lazy" decoding="async" width="20" height="20" class="rg-type-icon" style="object-fit:contain;vertical-align:middle;margin-right:4px;" />';
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| if (r === 0) { | ||
| questionHtml = questionHtml + `<option selected="selected" value="${answer.Weight}">${answer.Answer}</option>`; | ||
| questionHtml = questionHtml + `<option selected="selected" value="${answer.Weight}">${resgrid.escapeHtml(answer.Answer)}</option>`; |
There was a problem hiding this comment.
answer.Weight is interpolated directly into an HTML value attribute, so backend or user-controlled content can inject markup or malformed attribute data. Validate or escape answer.Weight before interpolation in Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js:569 and the listed occurrences.
Kody rule violation: Always sanitize user inputs
questionHtml = questionHtml + `<option selected="selected" value="${resgrid.escapeHtml(answer.Weight)}">${resgrid.escapeHtml(answer.Answer)}</option>`;Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js:
Line 276:
answer.Weight is interpolated directly into an HTML value attribute, so backend or user-controlled content can inject markup or malformed attribute data. Validate or escape answer.Weight before interpolation in Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js:569 and the listed occurrences.
Suggested Code:
questionHtml = questionHtml + `<option selected="selected" value="${resgrid.escapeHtml(answer.Weight)}">${resgrid.escapeHtml(answer.Answer)}</option>`;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var tr = '<tr>'; | ||
| tr += '<td>' + value.Name + " on " + value.Computer.Name + '</td>'; | ||
| tr += `<td><a class="btn btn-xs btn-primary" onclick='resgrid.groups.editgroup.selectPrinter("${value.Id}","${value.Name.replace(/\//g, "-").replace(/ /g, "_")}");' data-dismiss="modal">Select this Printer</a></td>`; | ||
| tr += '<td>' + resgrid.escapeHtml(value.Name) + " on " + resgrid.escapeHtml(value.Computer.Name) + '</td>'; |
There was a problem hiding this comment.
The expression dereferences value.Name and value.Computer.Name without verifying value or Computer, causing NullReferenceException-style failures for incomplete results. Use null-safe access for both properties in Web/Resgrid.Web/wwwroot/js/app/internal/groups/resgrid.groups.editgroup.js and the listed occurrences.
Kody rule violation: Add null checks to prevent NullReferenceException
tr += '<td>' + resgrid.escapeHtml(value?.Name ?? '') + " on " + resgrid.escapeHtml(value?.Computer?.Name ?? '') + '</td>';Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/groups/resgrid.groups.editgroup.js:
Line 81:
The expression dereferences value.Name and value.Computer.Name without verifying value or Computer, causing NullReferenceException-style failures for incomplete results. Use null-safe access for both properties in Web/Resgrid.Web/wwwroot/js/app/internal/groups/resgrid.groups.editgroup.js and the listed occurrences.
Suggested Code:
tr += '<td>' + resgrid.escapeHtml(value?.Name ?? '') + " on " + resgrid.escapeHtml(value?.Computer?.Name ?? '') + '</td>';
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:
|
| return seats | ||
| .Where(r => unitNames.ContainsKey(r.UnitId)) | ||
| .GroupBy(r => r.UserId, StringComparer.OrdinalIgnoreCase) | ||
| .ToDictionary(g => g.Key, | ||
| g => String.Join(", ", g.Select(r => unitNames[r.UnitId]).Distinct(StringComparer.OrdinalIgnoreCase).OrderBy(n => n, StringComparer.OrdinalIgnoreCase)), | ||
| StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The multi-stage LINQ expression in PersonnelMapLabel.cs combines filtering, grouping, and projection in a single statement, making each transformation harder to understand and maintain. Assign the filtered and grouped results to named intermediate queries before building the dictionary.
Kody rule violation: Limit Lengthy LINQ Chains
var namedSeats = seats.Where(r => unitNames.ContainsKey(r.UnitId));
var groupedSeats = namedSeats.GroupBy(r => r.UserId, StringComparer.OrdinalIgnoreCase);
return groupedSeats.ToDictionary(g => g.Key,
g => String.Join(", ", g.Select(r => unitNames[r.UnitId]).Distinct(StringComparer.OrdinalIgnoreCase).OrderBy(n => n, StringComparer.OrdinalIgnoreCase)),
StringComparer.OrdinalIgnoreCase);Prompt for LLM
File Core/Resgrid.Model/PersonnelMapLabel.cs:
Line 37 to 42:
The multi-stage LINQ expression in `PersonnelMapLabel.cs` combines filtering, grouping, and projection in a single statement, making each transformation harder to understand and maintain. Assign the filtered and grouped results to named intermediate queries before building the dictionary.
Suggested Code:
var namedSeats = seats.Where(r => unitNames.ContainsKey(r.UnitId));
var groupedSeats = namedSeats.GroupBy(r => r.UserId, StringComparer.OrdinalIgnoreCase);
return groupedSeats.ToDictionary(g => g.Key,
g => String.Join(", ", g.Select(r => unitNames[r.UnitId]).Distinct(StringComparer.OrdinalIgnoreCase).OrderBy(n => n, StringComparer.OrdinalIgnoreCase)),
StringComparer.OrdinalIgnoreCase);
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.UnitStatusAlertUpdated, TimeStamp = DateTime.UtcNow, DepartmentId = departmentId, ItemId = unitId.ToString() | ||
| }.SerializeJson()); | ||
|
|
||
| public Task<bool> UnitActiveRolesUpdated(int departmentId, int unitId) => SendMessage(Topics.EventingTopic, new EventingMessage |
There was a problem hiding this comment.
The UnitActiveRolesUpdated method returns the SendMessage task directly, so RabbitMQ failures are not wrapped with operation and identifier context. Await SendMessage inside an async try/catch and rethrow an InvalidOperationException containing the department and unit identifiers.
Kody rule violation: Add try-catch blocks for external calls
public async Task<bool> UnitActiveRolesUpdated(int departmentId, int unitId)
{
try
{
return await SendMessage(Topics.EventingTopic, new EventingMessage
{
Id = Guid.NewGuid(), Type = (int)EventingTypes.UnitActiveRolesUpdated, TimeStamp = DateTime.UtcNow, DepartmentId = departmentId, ItemId = unitId.ToString()
}.SerializeJson());
}
catch (Exception exception)
{
throw new InvalidOperationException($"Failed to publish UnitActiveRolesUpdated for department {departmentId}, unit {unitId}.", exception);
}
}Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs:
Line 94:
The `UnitActiveRolesUpdated` method returns the `SendMessage` task directly, so RabbitMQ failures are not wrapped with operation and identifier context. Await `SendMessage` inside an async `try/catch` and rethrow an `InvalidOperationException` containing the department and unit identifiers.
Suggested Code:
public async Task<bool> UnitActiveRolesUpdated(int departmentId, int unitId)
{
try
{
return await SendMessage(Topics.EventingTopic, new EventingMessage
{
Id = Guid.NewGuid(), Type = (int)EventingTypes.UnitActiveRolesUpdated, TimeStamp = DateTime.UtcNow, DepartmentId = departmentId, ItemId = unitId.ToString()
}.SerializeJson());
}
catch (Exception exception)
{
throw new InvalidOperationException($"Failed to publish UnitActiveRolesUpdated for department {departmentId}, unit {unitId}.", exception);
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| private async Task<Dictionary<string, string>> PersonnelTitles() | ||
| { | ||
| var result = (GetMapDataResult)(await Build().GetMapDataAndMarkers()).Result.Should().BeOfType<OkObjectResult>().Which.Value; |
There was a problem hiding this comment.
Blocking access to .Result in MappingControllerPersonnelLabelTests.cs and Tests/Resgrid.Tests/Web/Services/MappingControllerUnitMapTests.cs:130 can deadlock asynchronous execution and prevents proper task propagation. Replace the blocking access with await throughout the call chain.
Kody rule violation: Avoid Blocking Calls to Async Methods
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/MappingControllerPersonnelLabelTests.cs:
Line 118:
Blocking access to `.Result` in `MappingControllerPersonnelLabelTests.cs` and `Tests/Resgrid.Tests/Web/Services/MappingControllerUnitMapTests.cs:130` can deadlock asynchronous execution and prevents proper task propagation. Replace the blocking access with `await` throughout the call chain.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| private async Task<Dictionary<string, string>> PersonnelTitles() | ||
| { | ||
| var result = (GetMapDataResult)(await Build().GetMapDataAndMarkers()).Result.Should().BeOfType<OkObjectResult>().Which.Value; |
There was a problem hiding this comment.
Blocking access to .Result in MappingControllerPersonnelLabelTests.cs and Tests/Resgrid.Tests/Web/Services/MappingControllerUnitMapTests.cs:130 violates the async execution rule and can cause deadlocks. Await the operation end-to-end instead of using .Result or .Wait().
Kody rule violation: Await async operations properly
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/MappingControllerPersonnelLabelTests.cs:
Line 118:
Blocking access to `.Result` in `MappingControllerPersonnelLabelTests.cs` and `Tests/Resgrid.Tests/Web/Services/MappingControllerUnitMapTests.cs:130` violates the async execution rule and can cause deadlocks. Await the operation end-to-end instead of using `.Result` or `.Wait()`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| _rabbitInboundEventProvider.RegisterForSessionEvents(SessionEventReceived); | ||
| _rabbitInboundEventProvider.RegisterForUnitStatusAlertEvents((departmentId, id) => _eventingHub.Clients.Group(departmentId.ToString()).SendAsync("unitStatusAlertUpdated", id)); | ||
| // Carries only the unit id; clients reload the crew through the REST endpoints, which apply the viewer's visibility. | ||
| _rabbitInboundEventProvider.RegisterForUnitActiveRolesEvents((departmentId, id) => _eventingHub.Clients.Group(departmentId.ToString()).SendAsync("unitActiveRolesUpdated", id)); |
There was a problem hiding this comment.
The SendAsync task created in the listener callback is neither awaited nor caught, so publishing failures can become unhandled rejections. Make the callback async, await SendAsync inside try/catch, and log the department and unit identifiers with the failure.
Kody rule violation: Handle async operations with proper error handling
_rabbitInboundEventProvider.RegisterForUnitActiveRolesEvents(async (departmentId, id) =>
{
try
{
await _eventingHub.Clients.Group(departmentId.ToString()).SendAsync(UnitActiveRolesUpdatedEvent, id);
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to publish unit active roles update for department {DepartmentId} and unit {UnitId}", departmentId, id);
}
});Prompt for LLM
File Web/Resgrid.Web.Eventing/Worker.cs:
Line 66:
The `SendAsync` task created in the listener callback is neither awaited nor caught, so publishing failures can become unhandled rejections. Make the callback async, await `SendAsync` inside `try/catch`, and log the department and unit identifiers with the failure.
Suggested Code:
_rabbitInboundEventProvider.RegisterForUnitActiveRolesEvents(async (departmentId, id) =>
{
try
{
await _eventingHub.Clients.Group(departmentId.ToString()).SendAsync(UnitActiveRolesUpdatedEvent, id);
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to publish unit active roles update for department {DepartmentId} and unit {UnitId}", departmentId, id);
}
});
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| _rabbitInboundEventProvider.RegisterForSessionEvents(SessionEventReceived); | ||
| _rabbitInboundEventProvider.RegisterForUnitStatusAlertEvents((departmentId, id) => _eventingHub.Clients.Group(departmentId.ToString()).SendAsync("unitStatusAlertUpdated", id)); | ||
| // Carries only the unit id; clients reload the crew through the REST endpoints, which apply the viewer's visibility. | ||
| _rabbitInboundEventProvider.RegisterForUnitActiveRolesEvents((departmentId, id) => _eventingHub.Clients.Group(departmentId.ToString()).SendAsync("unitActiveRolesUpdated", id)); |
There was a problem hiding this comment.
The listener registration in Web/Resgrid.Web.Eventing/Worker.cs lacks explicit callback error handling and a deterministic unsubscribe or cleanup path during worker teardown. Register the async callback with try/catch logging and retain the provider's registration handle for disposal when the worker stops.
Kody rule violation: Provide error handlers to subscription/listener APIs
_rabbitInboundEventProvider.RegisterForUnitActiveRolesEvents(async (departmentId, id) =>
{
try
{
await _eventingHub.Clients.Group(departmentId.ToString()).SendAsync(UnitActiveRolesUpdatedEvent, id);
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to publish unit active roles update for department {DepartmentId} and unit {UnitId}", departmentId, id);
}
});
// Retain and dispose the registration when the worker stops.Prompt for LLM
File Web/Resgrid.Web.Eventing/Worker.cs:
Line 66:
The listener registration in `Web/Resgrid.Web.Eventing/Worker.cs` lacks explicit callback error handling and a deterministic unsubscribe or cleanup path during worker teardown. Register the async callback with `try/catch` logging and retain the provider's registration handle for disposal when the worker stops.
Suggested Code:
_rabbitInboundEventProvider.RegisterForUnitActiveRolesEvents(async (departmentId, id) =>
{
try
{
await _eventingHub.Clients.Group(departmentId.ToString()).SendAsync(UnitActiveRolesUpdatedEvent, id);
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to publish unit active roles update for department {DepartmentId} and unit {UnitId}", departmentId, id);
}
});
// Retain and dispose the registration when the worker stops.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var coordinatesArr = coordinates.Split(char.Parse(",")); | ||
| if (double.TryParse(coordinatesArr[0], out newLat) && double.TryParse(coordinatesArr[1], out newLon)) | ||
| { | ||
| centerLat = newLat; | ||
| centerLon = newLon; | ||
| } |
There was a problem hiding this comment.
Culture-sensitive parsing in the shared map-center helper can misinterpret invariant geocoder output such as 39.5,-119.8 on comma-decimal servers, producing an incorrect or invalid center instead of the fallback, while malformed nonempty output can throw IndexOutOfRangeException. Validate that the split result has two components, parse both values with CultureInfo.InvariantCulture, and accept them only within latitude/longitude bounds.
var coordinatesArr = coordinates.Split(',');
if (coordinatesArr.Length == 2 &&
double.TryParse(coordinatesArr[0], NumberStyles.Float, CultureInfo.InvariantCulture, out var newLat) &&
double.TryParse(coordinatesArr[1], NumberStyles.Float, CultureInfo.InvariantCulture, out var newLon) &&
newLat >= -90 && newLat <= 90 && newLon >= -180 && newLon <= 180)
{
centerLat = newLat;
centerLon = newLon;
}Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs:
Line 510 to 515:
Culture-sensitive parsing in the shared map-center helper can misinterpret invariant geocoder output such as `39.5,-119.8` on comma-decimal servers, producing an incorrect or invalid center instead of the fallback, while malformed nonempty output can throw `IndexOutOfRangeException`. Validate that the split result has two components, parse both values with `CultureInfo.InvariantCulture`, and accept them only within latitude/longitude bounds.
Suggested Code:
var coordinatesArr = coordinates.Split(',');
if (coordinatesArr.Length == 2 &&
double.TryParse(coordinatesArr[0], NumberStyles.Float, CultureInfo.InvariantCulture, out var newLat) &&
double.TryParse(coordinatesArr[1], NumberStyles.Float, CultureInfo.InvariantCulture, out var newLon) &&
newLat >= -90 && newLat <= 90 && newLon >= -180 && newLon <= 180)
{
centerLat = newLat;
centerLon = newLon;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| if (!String.IsNullOrWhiteSpace(gpsCoordinates)) | ||
| { | ||
| string[] coordinates = gpsCoordinates.Split(char.Parse(",")); |
There was a problem hiding this comment.
Using char.Parse(",") for gpsCoordinates.Split in Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs and at lines 490 and 510 performs unnecessary parsing on input. Use a direct delimiter or a TryParse-based approach and validate the coordinate format and culture.
Kody rule violation: Use TryParse for string conversions
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs:
Line 467:
Using `char.Parse(",")` for `gpsCoordinates.Split` in `Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs` and at lines 490 and 510 performs unnecessary parsing on input. Use a direct delimiter or a TryParse-based approach and validate the coordinate format and culture.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return (info, AsUtc(state.Timestamp)); | ||
| } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
The empty catch { } at Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs:615 silently swallows exceptions, obscuring failures and leaving the operation undefined. Log the exception with context and either rethrow it or handle it explicitly.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs:
Line 574:
The empty `catch { }` at `Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs:615` silently swallows exceptions, obscuring failures and leaving the operation undefined. Log the exception 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.
| RoleName = String.Join(", ", member.Select(x => x.Role.Name)) | ||
| }; | ||
|
|
||
| if (await _authorizationService.CanUserViewPersonLocationViaMatrixAsync(person.UserId, UserId, DepartmentId)) |
There was a problem hiding this comment.
Calling CanUserViewPersonLocationViaMatrixAsync inside the crew loop issues one authorization or network request per person, increasing latency and load. Batch the checks or use a bulk authorization API before iterating over the crew.
Kody rule violation: Detect N+1 style queries and suggest batching
var personIds = ...;
var visibilityByPerson = await Task.WhenAll(personIds.Select(async personId => ...));Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs:
Line 432:
Calling `CanUserViewPersonLocationViaMatrixAsync` inside the crew loop issues one authorization or network request per person, increasing latency and load. Batch the checks or use a bulk authorization API before iterating over the crew.
Suggested Code:
var personIds = ...;
var visibilityByPerson = await Task.WhenAll(personIds.Select(async personId => ...));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <div className="rg-map__toolbar"> | ||
| <div className="rg-map__toolbar-section"> | ||
| <label className="rg-map__checkbox"> | ||
| <input type="checkbox" checked={keepInView} onChange={(event) => setKeepInView(event.target.checked)} /> |
There was a problem hiding this comment.
The inline onChange arrow function in UnitMapElement.tsx:415 creates a new function on every render, increasing component re-render overhead. Move the handler outside the JSX props.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/unitMap/UnitMapElement.tsx:
Line 410:
The inline `onChange` arrow function in `UnitMapElement.tsx:415` creates a new function on every render, increasing component re-render overhead. Move the handler outside the JSX props.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| connection = established; | ||
| onConnected(attempt); | ||
| } catch (connectionError) { | ||
| console.error(`Unable to connect to realtime ${label}.`, connectionError); |
There was a problem hiding this comment.
The realtime connection error log interpolates the connection label and passes the raw error positionally, preventing consistent structured querying. Log the operation name, connection label, and error as structured fields.
Kody rule violation: Include error context in structured logs
logger.error('Realtime connection failed', { operation: 'connect', label, error: connectionError });Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/unitMap/UnitMapElement.tsx:
Line 119:
The realtime connection error log interpolates the connection label and passes the raw error positionally, preventing consistent structured querying. Log the operation name, connection label, and error as structured fields.
Suggested Code:
logger.error('Realtime connection failed', { operation: 'connect', label, error: connectionError });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @Html.Raw("<td>") | ||
|
|
||
| @Html.Raw($"<a type='button' href='{@Url.Action("ViewEvents", "Units", new { area = "User" })}?unitId={u.UnitId}' class='btn btn-xs btn-primary'>{Html.Encode(localizer["ViewEvents"].Value)}</a> <a type='button' href='{@Url.Action("ViewLogs", "Units", new { area = "User" })}?unitId={u.UnitId}' class='btn btn-xs btn-info'>{Html.Encode(localizer["Logs"].Value)}</a> ") | ||
| @Html.Raw($"<a type='button' href='{@Url.Action("ViewEvents", "Units", new { area = "User" })}?unitId={u.UnitId}' class='btn btn-xs btn-primary'>{Html.Encode(localizer["ViewEvents"].Value)}</a> <a type='button' href='{@Url.Action("ViewLogs", "Units", new { area = "User" })}?unitId={u.UnitId}' class='btn btn-xs btn-info'>{Html.Encode(localizer["Logs"].Value)}</a> <a type='button' href='{@Url.Action("ViewMap", "Units", new { area = "User" })}?unitId={u.UnitId}' class='btn btn-xs btn-success'>{Html.Encode(localizer["UnitMap"].Value)}</a> ") |
There was a problem hiding this comment.
Embedding u.UnitId in an Html.Raw-generated href bypasses Razor's normal HTML encoding and can produce an unsafe URL. Pass unitId through Url.Action route values and render the anchor normally.
Kody rule violation: Always sanitize user inputs
<a type="button" href="@Url.Action("ViewMap", "Units", new { area = "User", unitId = u.UnitId })" class="btn btn-xs btn-success">@Html.Encode(localizer["UnitMap"].Value)</a> Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Units/Index.cshtml:
Line 456:
Embedding `u.UnitId` in an `Html.Raw`-generated href bypasses Razor's normal HTML encoding and can produce an unsafe URL. Pass `unitId` through `Url.Action` route values and render the anchor normally.
Suggested Code:
<a type="button" href="@Url.Action("ViewMap", "Units", new { area = "User", unitId = u.UnitId })" class="btn btn-xs btn-success">@Html.Encode(localizer["UnitMap"].Value)</a>
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <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> |
There was a problem hiding this comment.
Dereferencing Unit and mapConfig during ViewMap.cshtml rendering can throw NullReferenceException when either value is missing. Guard both objects and provide safe defaults for map properties.
Kody rule violation: Add null checks to prevent NullReferenceException
<rg-unit-map unitid="@(Model.Unit?.UnitId)" departmentid="@ClaimsAuthorizationHelper.GetDepartmentId()" mapheight="650px" strings="@System.Text.Json.JsonSerializer.Serialize(unitMapStrings)" leafletosmurl="@(mapConfig?.TileUrl ?? string.Empty)" mapattribution="@(mapConfig?.Attribution ?? string.Empty)" mapprovider="@(mapConfig?.MapProvider ?? string.Empty)" mapstyleurl="@(mapConfig?.StyleUrl ?? string.Empty)" mapaccesstoken="@(mapConfig?.AccessToken ?? string.Empty)"></rg-unit-map>
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Units/ViewMap.cshtml:
Line 48:
Dereferencing `Unit` and `mapConfig` during `ViewMap.cshtml` rendering can throw `NullReferenceException` when either value is missing. Guard both objects and provide safe defaults for map properties.
Suggested Code:
<rg-unit-map unitid="@(Model.Unit?.UnitId)" departmentid="@ClaimsAuthorizationHelper.GetDepartmentId()" mapheight="650px" strings="@System.Text.Json.JsonSerializer.Serialize(unitMapStrings)" leafletosmurl="@(mapConfig?.TileUrl ?? string.Empty)" mapattribution="@(mapConfig?.Attribution ?? string.Empty)" mapprovider="@(mapConfig?.MapProvider ?? string.Empty)" mapstyleurl="@(mapConfig?.StyleUrl ?? string.Empty)" mapaccesstoken="@(mapConfig?.AccessToken ?? string.Empty)"></rg-unit-map>
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <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> |
There was a problem hiding this comment.
Direct property access on Unit and mapConfig in ViewMap.cshtml can throw NullReferenceException when either object or its map settings are missing. Use null-safe access and provide safe defaults for all map properties.
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 ?? string.Empty)" mapattribution="@(mapConfig?.Attribution ?? string.Empty)" mapprovider="@(mapConfig?.MapProvider ?? string.Empty)" mapstyleurl="@(mapConfig?.StyleUrl ?? string.Empty)" mapaccesstoken="@(mapConfig?.AccessToken ?? string.Empty)"></rg-unit-map>
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Units/ViewMap.cshtml:
Line 48:
Direct property access on `Unit` and `mapConfig` in `ViewMap.cshtml` can throw `NullReferenceException` when either object or its map settings are missing. Use null-safe access and provide safe defaults for all map properties.
Suggested Code:
<rg-unit-map unitid="@(Model.Unit?.UnitId)" departmentid="@ClaimsAuthorizationHelper.GetDepartmentId()" mapheight="650px" strings="@System.Text.Json.JsonSerializer.Serialize(unitMapStrings)" leafletosmurl="@(mapConfig?.TileUrl ?? string.Empty)" mapattribution="@(mapConfig?.Attribution ?? string.Empty)" mapprovider="@(mapConfig?.MapProvider ?? string.Empty)" mapstyleurl="@(mapConfig?.StyleUrl ?? string.Empty)" mapaccesstoken="@(mapConfig?.AccessToken ?? string.Empty)"></rg-unit-map>
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
This pull request delivers the RG-T148 search, HTML safety, custom numbering, and dispatch updates.
HTML safety and rich-text handling
Html.Rawusage for stored content with sanitized rendering or normal Razor encoding.Custom number searching
Number ordering and numbering-year behavior
Dispatch updates
Inventory report presentation
Validation and regression coverage