Conversation
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:
|
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughThe pull request updates chatbot intent matching and chat restrictions, prevents duplicate reactions, adds database column repair migrations, changes reCAPTCHA verification handling, validates language selection, and adjusts chat UI behavior. ChangesChat and assistant behavior
Category column migrations
Web request validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Core/Resgrid.Services/ChatMessageService.cs (1)
346-358: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: narrow the duplicate-reaction check.
The duplicate check loads every reaction on the message just to test for one emoji/user match. For messages with many reactions, a targeted existence query (for example
ExistsAsync(chatMessageId, participantType, userId, unitId, emoji)) would avoid loading the full reaction set on everyAddReactionAsynccall.Reaction counts per message are usually small, so this is a minor concern. Consider it only if the repository already exposes (or can cheaply expose) a narrower lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/ChatMessageService.cs` around lines 346 - 358, The duplicate check in AddReactionAsync currently loads all reactions through GetByMessageIdsAsync before matching one reaction. If the repository supports or can cheaply add a targeted existence lookup, update this check to query by chatMessageId, participant identity, unitId, and emoji while preserving the existing true-return behavior; otherwise leave the current implementation 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 `@Web/Resgrid.Web/Attributes/GoogleReCaptchaValidationAttribute.cs`:
- Around line 43-49: Replace the synchronous verification in
GoogleReCaptchaValidationAttribute.IsValid with an asynchronous action filter or
equivalent request component that awaits both the reCAPTCHA POST and
response-body read without blocking request threads. Preserve the existing
validation outcome for non-OK responses and verification failures, and add those
failures to ModelState before the action executes.
- Around line 49-50: Update the JSON handling in
GoogleReCaptchaValidationAttribute so JObject.Parse failures for malformed or
non-object responses are caught separately from transport errors. Log the
parsing exception through Framework.Logging.LogException and return the existing
retryable validation result, preserving the current behavior for valid object
responses.
---
Nitpick comments:
In `@Core/Resgrid.Services/ChatMessageService.cs`:
- Around line 346-358: The duplicate check in AddReactionAsync currently loads
all reactions through GetByMessageIdsAsync before matching one reaction. If the
repository supports or can cheaply add a targeted existence lookup, update this
check to query by chatMessageId, participant identity, unitId, and emoji while
preserving the existing true-return behavior; otherwise leave the current
implementation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1d315c1-04a4-4142-bb8f-defd530abaec
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Chatbot/ChatbotAvailabilityIntentClassifierTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/ChatMessageServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (15)
Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.csCore/Resgrid.Services/ChatMessageService.csProviders/Resgrid.Providers.Migrations/Migrations/M0113_FixNotesDocumentsCategoryColumn.csProviders/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sqlProviders/Resgrid.Providers.MigrationsPg/Migrations/M0113_FixNotesDocumentsCategoryColumnPg.csProviders/Resgrid.Providers.MigrationsPg/Sql/M0001_InitialMigration.sqlWeb/Resgrid.Web.Services/Controllers/v4/ChatController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPanelElement.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ConversationView.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/Composer.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/MessageBubble.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chat.cssWeb/Resgrid.Web/Attributes/GoogleReCaptchaValidationAttribute.csWeb/Resgrid.Web/Controllers/AccountController.cs
| var httpResponse = _httpClient.PostAsync("https://www.google.com/recaptcha/api/siteverify", content).GetAwaiter().GetResult(); | ||
| if (httpResponse.StatusCode != HttpStatusCode.OK) | ||
| { | ||
| return errorResult.Value; | ||
| } | ||
|
|
||
| return ValidationResult.Success; | ||
| String jsonResponse = httpResponse.Content.ReadAsStringAsync().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate existing async request-validation components and all reCAPTCHA uses.
rg -n -C 3 --glob '*.cs' \
'GoogleReCaptchaValidationAttribute|IAsync(Action|Authorization|Resource)Filter|ModelState\.AddModelError' \
WebRepository: Resgrid/Core
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reCAPTCHA attribute and focused usages without dumping the entire Web/Controllers match list.
printf -- '--- GoogleReCaptchaValidationAttribute.cs ---\n'
cat -n Web/Resgrid.Web/Attributes/GoogleReCaptchaValidationAttribute.cs
printf -- '\n--- GoogleReCaptchaValidationAttribute references ---\n'
rg -n --glob '*.cshtml' --glob '*.cs' --glob '*.razor' 'GoogleReCaptchaValidation|g-recaptcha|recaptcha' Web/Resgrid.Web
printf -- '\n--- AccountController POST snippets around register actions ---\n'
rg -n -A 80 -B 10 'Register|RegisterModel|GoogleReCaptcha|g-recaptcha' Web/Resgrid.Web/Controllers/AccountController.csRepository: Resgrid/Core
Length of output: 10069
Move reCAPTCHA verification out of synchronous model validation.
IsValid() blocks the request thread for both the POST and response-body read, up to the 10-second timeout. Replace the synchronous ValidationAttribute path with an async filter or equivalent request component so concurrent validation calls do not block request threads, and add failures to ModelState.
🤖 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/Attributes/GoogleReCaptchaValidationAttribute.cs` around
lines 43 - 49, Replace the synchronous verification in
GoogleReCaptchaValidationAttribute.IsValid with an asynchronous action filter or
equivalent request component that awaits both the reCAPTCHA POST and
response-body read without blocking request threads. Preserve the existing
validation outcome for non-OK responses and verification failures, and add those
failures to ModelState before the action executes.
| String jsonResponse = httpResponse.Content.ReadAsStringAsync().GetAwaiter().GetResult(); | ||
| dynamic jsonData = JObject.Parse(jsonResponse); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm the installed Newtonsoft.Json reference and inspect parsing exception handling.
rg -n -C 3 --glob '*.csproj' --glob 'packages.config' --glob '*.cs' \
'Newtonsoft\.Json|JObject\.Parse|JsonException|JsonReaderException' \
WebRepository: Resgrid/Core
Length of output: 43035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the validation attribute around the parsing and exception-handling logic.
sed -n '1,140p' Web/Resgrid.Web/Attributes/GoogleReCaptchaValidationAttribute.cs | cat -n
# Search for the exact method/attribute to ensure context is complete.
rg -n "class GoogleReCaptchaValidationAttribute|ValidationAttribute|LogException|TryVerify|OnAuthorization" Web/Resgrid.Web/Attributes/GoogleReCaptchaValidationAttribute.csRepository: Resgrid/Core
Length of output: 3195
🌐 Web query:
Newtonsoft.Json 13.0.3 JObject.Parse JsonException documentation
💡 Result:
In Newtonsoft.Json version 13.0.3, the JObject.Parse method is designed to parse a JSON string into a JObject [1][2]. If the provided string is not valid JSON or does not represent a JSON object (e.g., if it represents a JSON array or a primitive), the method throws a JsonReaderException [1][3][4]. JsonReaderException is a subclass of JsonException [5][6]. This exception typically includes detailed information about the error, such as the path in the JSON string where the error occurred, the line number, and the position [5]. Common scenarios that trigger this exception include: - Passing invalid JSON syntax [1][4]. - Passing a root JSON element that is not an object, such as a JSON array ("[]") [3]. In such cases, if you expect an array, JArray.Parse should be used instead; if the JSON structure is unknown, JToken.Parse is the appropriate method [3]. When handling parsing errors, it is standard practice to catch JsonReaderException specifically to handle malformed input, rather than catching the base JsonException or the generic System.Exception, which may obscure other issues [7][5][6].
Citations:
- 1: https://www.newtonsoft.com/json/help/html/M_Newtonsoft_Json_Linq_JObject_Parse.htm
- 2: https://relativityapiexplorer.azurewebsites.net/packages/Newtonsoft.Json/13.0.3/lib/net20/Newtonsoft.Json.dll/Newtonsoft.Json.Linq/JObject
- 3: Passing "[]" to JObject.Parse Throws Exception JamesNK/Newtonsoft.Json#1824
- 4: https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Linq/JObject.cs
- 5: https://www.newtonsoft.com/json/help/html/t_newtonsoft_json_jsonreaderexception.htm
- 6: https://www.newtonsoft.com/json/help/html/t_newtonsoft_json_jsonexception.htm
- 7: https://stackoverflow.com/questions/29830198/newtonsoft-jobject-parse-throws-base-exception-how-to-handle
Fail closed when Google returns a non-object reCAPTCHA response.
JObject.Parse(jsonResponse) throws for malformed JSON and non-object status-200 payloads, but the filter at line 58 only catches transport failures. Catch the JSON parsing exception, log it with Framework.Logging.LogException, and return the same retryable validation result instead of 500-ing the page.
🤖 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/Attributes/GoogleReCaptchaValidationAttribute.cs` around
lines 49 - 50, Update the JSON handling in GoogleReCaptchaValidationAttribute so
JObject.Parse failures for malformed or non-object responses are caught
separately from transport errors. Log the parsing exception through
Framework.Logging.LogException and return the existing retryable validation
result, preserving the current behavior for valid object responses.
| if (supported != null) | ||
| { | ||
| Response.Cookies.Append(CookieRequestCultureProvider.DefaultCookieName, CookieRequestCultureProvider.MakeCookieValue(new RequestCulture(culture)), new CookieOptions { Expires = DateTime.UtcNow.AddYears(1) }); | ||
| Response.Cookies.Append(CookieRequestCultureProvider.DefaultCookieName, CookieRequestCultureProvider.MakeCookieValue(new RequestCulture(supported)), new CookieOptions { Expires = DateTime.UtcNow.AddYears(1) }); |
| // Upcoming-calendar phrasings: "when is the next event?", "what is upcoming in the | ||
| // calendar?", "upcoming events", "what's coming up", "next events". | ||
| (R(@"^when('?s|\s+is)\s+(the\s+)?next\s+(event|meeting|training|class)(s|es)?$"), | ||
| "list_calendar", null), |
There was a problem hiding this comment.
Hardcoded string literal "list_calendar" represents a finite intent set prone to typos and poor discoverability. Define an enum (e.g., IntentTypes.ListCalendar) or a constants class and reference it in place of the literal.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs:
Line 179:
Hardcoded string literal `"list_calendar"` represents a finite intent set prone to typos and poor discoverability. Define an enum (e.g., `IntentTypes.ListCalendar`) or a constants class and reference it in place of the literal.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Upcoming-calendar phrasings: "when is the next event?", "what is upcoming in the | ||
| // calendar?", "upcoming events", "what's coming up", "next events". | ||
| (R(@"^when('?s|\s+is)\s+(the\s+)?next\s+(event|meeting|training|class)(s|es)?$"), | ||
| "list_calendar", null), |
There was a problem hiding this comment.
Shared string literal "list_calendar" repeats across multiple entries in KeywordIntentClassifier.cs:181 and KeywordIntentClassifier.cs:183, risking inconsistency. Define a constant (e.g., const string ListCalendarIntent = "list_calendar") in a shared intents class and reference it here.
Kody rule violation: Centralize string constants
Prompt for LLM
File Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs:
Line 179:
Shared string literal `"list_calendar"` repeats across multiple entries in `KeywordIntentClassifier.cs:181` and `KeywordIntentClassifier.cs:183`, risking inconsistency. Define a constant (e.g., `const string ListCalendarIntent = "list_calendar"`) in a shared intents class and reference it here.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // reaches the database (RepositoryBase logs every insert exception, so relying on the | ||
| // unique-violation catch below alone floods the error log). The catch still covers the | ||
| // genuine concurrent race two requests can win simultaneously. | ||
| var existingReactions = await _chatMessageReactionRepository.GetByMessageIdsAsync(new[] { chatMessageId }); |
There was a problem hiding this comment.
Unguarded await: GetByMessageIdsAsync sits before the try block at line 359, so transient failures (timeouts, deadlocks) propagate unhandled. Move the read inside the try block or wrap it in its own try/catch that logs context (messageId, emoji) and returns a safe default.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Services/ChatMessageService.cs:
Line 350:
Unguarded await: `GetByMessageIdsAsync` sits before the try block at line 359, so transient failures (timeouts, deadlocks) propagate unhandled. Move the read inside the try block or wrap it in its own try/catch that logs context (`messageId`, `emoji`) and returns a safe default.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // reaches the database (RepositoryBase logs every insert exception, so relying on the | ||
| // unique-violation catch below alone floods the error log). The catch still covers the | ||
| // genuine concurrent race two requests can win simultaneously. | ||
| var existingReactions = await _chatMessageReactionRepository.GetByMessageIdsAsync(new[] { chatMessageId }); |
There was a problem hiding this comment.
Unguarded external DB call: GetByMessageIdsAsync is not wrapped in a try/catch with context, violating the requirement that network/DB/external calls include structured context and map errors to application-level errors. Wrap the repository read in try/catch, log with structured context (chatMessageId, emoji, unitId, userId), and either fall through to the insert path or return a deterministic result on failure.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/ChatMessageService.cs:
Line 350:
Unguarded external DB call: `GetByMessageIdsAsync` is not wrapped in a try/catch with context, violating the requirement that network/DB/external calls include structured context and map errors to application-level errors. Wrap the repository read in try/catch, log with structured context (`chatMessageId`, `emoji`, `unitId`, `userId`), and either fall through to the insert path or return a deterministic result on failure.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| accept="image/*" | ||
| className="rgchat-fileinput" | ||
| style={{ display: 'none' }} | ||
| onChange={(event) => void handleFile(event.target.files?.[0])} |
There was a problem hiding this comment.
Inline arrow function in the onChange JSX prop creates a new function on every render, impacting performance. Move the function definition outside the render method.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/Composer.tsx:
Line 322:
Inline arrow function in the `onChange` JSX prop creates a new function on every render, impacting performance. Move the function definition outside the render method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| // Transient network/DNS failure reaching Google: fail closed with a retryable | ||
| // validation message instead of letting the exception 500 the register page. | ||
| Framework.Logging.LogException(ex); |
There was a problem hiding this comment.
Missing structured context: the error log only passes the exception object with no operation name or relevant identifiers. Pass structured context, e.g., Framework.Logging.LogException(ex, new { op = "recaptcha.siteverify", member = validationContext.MemberName }), or use an overload that accepts a message and identifiers.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Web/Resgrid.Web/Attributes/GoogleReCaptchaValidationAttribute.cs:
Line 62:
Missing structured context: the error log only passes the exception object with no operation name or relevant identifiers. Pass structured context, e.g., `Framework.Logging.LogException(ex, new { op = "recaptcha.siteverify", member = validationContext.MemberName })`, or use an overload that accepts a message and identifiers.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| return ValidationResult.Success; | ||
| String jsonResponse = httpResponse.Content.ReadAsStringAsync().GetAwaiter().GetResult(); | ||
| dynamic jsonData = JObject.Parse(jsonResponse); |
There was a problem hiding this comment.
Untrusted JSON: JObject.Parse(Response) parses Google's reCAPTCHA endpoint response without validating its shape, and the surrounding catch filter (HttpRequestException/TaskCanceledException/OperationCanceledException) will not catch a JsonReaderException. Parse inside a try/catch that also handles Newtonsoft.Json.JsonException, then assert required fields (e.g., Data.success is JToken) before using them, returning the retryable ValidationResult on malformed input.
Kody rule violation: Always validate JSON parsing
Prompt for LLM
File Web/Resgrid.Web/Attributes/GoogleReCaptchaValidationAttribute.cs:
Line 50:
Untrusted JSON: `JObject.Parse(Response)` parses Google's reCAPTCHA endpoint response without validating its shape, and the surrounding catch filter (`HttpRequestException`/`TaskCanceledException`/`OperationCanceledException`) will not catch a `JsonReaderException`. Parse inside a try/catch that also handles `Newtonsoft.Json.JsonException`, then assert required fields (e.g., `Data.success` is `JToken`) before using them, returning the retryable `ValidationResult` on malformed input.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
PR Description (RG-T117 Fixes)
This PR addresses multiple bugs and improvements across the Resgrid platform:
Chat & Assistant
Database
Cateryinstead ofCategoryin the Notes and Documents tables, causing every save to fail. Migration 113 renames the column (for both SQL Server and PostgreSQL), guarded so already-correct databases are untouched.Security & Reliability
HttpClientinstantiation (socket exhaustion under load) to a shared static client with a 10-second timeout, changed from GET to POST to keep the secret out of URLs/logs, and added graceful failure handling for transient network errors.SetLanguageendpoint now whitelists culture values against supported locales, preventing scanner garbage (SQL fragments, paths, etc.) from triggering unhandled exceptions and 500 errors.