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 PR queues chatbot-channel messages, validates department chat availability, supports wrapped and differently cased channel responses, and changes user-facing terminology from “chatbot” to “assistant.” ChangesChatbot assistant flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChatController
participant IQueueService
participant SenderDevices
ChatController->>IQueueService: Enqueue WebChat chatbot message
IQueueService-->>ChatController: Return enqueue result
ChatController->>SenderDevices: Publish chatbot typing event
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs (1)
68-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
IQueueServicewith the required service locator.The added constructor parameter uses constructor injection. Resolve
IQueueServicewithBootstrapper.GetKernel().Resolve<IQueueService>()in the constructor, then remove the parameter.As per coding guidelines, use
Bootstrapper.GetKernel().Resolve<T>()rather than constructor injection.🤖 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.Services/Controllers/v4/ChatController.cs` around lines 68 - 83, The ChatController constructor currently injects IQueueService directly; remove the queueService parameter and initialize _queueService by resolving IQueueService through Bootstrapper.GetKernel().Resolve<IQueueService>() within the constructor, while leaving the existing field assignment and other dependencies unchanged.Source: Coding guidelines
🤖 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.Services/Controllers/v4/ChatController.cs`:
- Around line 829-846: The chat endpoint must handle the boolean result from
EnqueueChatbotMessageAsync before emitting ChatbotTyping or returning Created.
Update the surrounding method to capture the enqueue result, return the same
failure response used by ChatbotController.SendChatMessage when it is false, and
only send the typing event and success response after queueing succeeds.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs`:
- Around line 68-83: The ChatController constructor currently injects
IQueueService directly; remove the queueService parameter and initialize
_queueService by resolving IQueueService through
Bootstrapper.GetKernel().Resolve<IQueueService>() within the constructor, while
leaving the existing field assignment and other dependencies 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: fb4b76fe-bf32-4e18-ac07-45aff8ba03d9
⛔ Files ignored due to path filters (19)
Core/Resgrid.Localization/Areas/User/Department/Department.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.uk.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (9)
Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.csCore/Resgrid.Chatbot/Handlers/HelpActionHandler.csCore/Resgrid.Chatbot/Handlers/MessageSendHandler.csCore/Resgrid.Chatbot/Handlers/PollCreateHandler.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Chatbot/Services/CodeLinkingService.csWeb/Resgrid.Web.Services/Controllers/v4/ChatController.csWeb/Resgrid.Web.Services/Controllers/v4/ChatbotController.csWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chatApi.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs (1)
54-54: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winResolve
IQueueServicethrough the required Service Locator.Line 69 adds constructor injection for
IQueueService. Remove this parameter. Resolve the dependency in the constructor withBootstrapper.GetKernel().Resolve<IQueueService>().As per coding guidelines, use “
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection”.Proposed fix
IFeatureToggleService featureToggleService, IAuthorizationService authorizationService, ICacheProvider cacheProvider, - IEventAggregator eventAggregator, - IQueueService queueService) + IEventAggregator eventAggregator) @@ _cacheProvider = cacheProvider; _eventAggregator = eventAggregator; - _queueService = queueService; + _queueService = Bootstrapper.GetKernel().Resolve<IQueueService>();Also applies to: 68-83
🤖 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.Services/Controllers/v4/ChatController.cs` at line 54, Update the ChatController constructor to remove the injected IQueueService parameter and initialize the existing _queueService field by resolving IQueueService through Bootstrapper.GetKernel().Resolve<IQueueService>(). Preserve the field and all other constructor dependencies unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs`:
- Line 54: Update the ChatController constructor to remove the injected
IQueueService parameter and initialize the existing _queueService field by
resolving IQueueService through
Bootstrapper.GetKernel().Resolve<IQueueService>(). Preserve the field and all
other constructor dependencies unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e00a484-189c-4d5f-a9f8-ab819daabbdb
📒 Files selected for processing (1)
Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs
| var queued = await _queueService.EnqueueChatbotMessageAsync(new Resgrid.Model.Queue.ChatbotMessageQueueItem | ||
| { | ||
| DepartmentId = DepartmentId, | ||
| From = UserId, | ||
| Body = message.Body, | ||
| MessageId = message.ChatMessageId, | ||
| Platform = (int)Resgrid.Chatbot.Models.ChatbotPlatform.WebChat | ||
| }); |
There was a problem hiding this comment.
Unguarded external I/O call in _queueService.EnqueueChatbotMessageAsync violates Rule [27], which requires external calls to be wrapped with error context and mapped to application-level errors. Wrap the enqueue in try/catch with structured logging context (channelId, DepartmentId, UserId), deciding whether the chatbot pipeline is best-effort or should surface a typed application error. Also found in ChatbotController.cs:478.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 829 to 836:
Unguarded external I/O call in _queueService.EnqueueChatbotMessageAsync violates Rule [27], which requires external calls to be wrapped with error context and mapped to application-level errors. Wrap the enqueue in try/catch with structured logging context (channelId, DepartmentId, UserId), deciding whether the chatbot pipeline is best-effort or should surface a typed application error. Also found in ChatbotController.cs:478.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // The assistant channel is also reachable from the regular chat page, so sends that arrive | ||
| // through this generic endpoint must still feed the chatbot pipeline (the dedicated | ||
| // ChatbotController.SendChatMessage does the same for the assistant panel). | ||
| var channel = await _chatChannelService.GetChannelByIdAsync(channelId); |
There was a problem hiding this comment.
Unhandled exception risk in the chatbot-enqueue block (lines 826-847) which sits outside the try-catch handling SendMessageAsync errors, allowing exceptions from EnqueueChatbotMessageAsync or _eventAggregator.SendMessage to propagate unhandled after the message is already persisted. Wrap the enqueue block in a try-catch that logs the exception and continues returning the already-saved message, matching the pattern in ChatbotController.SendChatMessage (lines 365-413).
if (message == null)
return BadRequest();
// The assistant channel is also reachable from the regular chat page, so sends that arrive
// through this generic endpoint must still feed the chatbot pipeline (the dedicated
// ChatbotController.SendChatMessage does the same for the assistant panel).
try
{
var channel = await _chatChannelService.GetChannelByIdAsync(channelId);
if (channel != null && channel.ChannelType == (int)ChatChannelType.Chatbot && !String.IsNullOrWhiteSpace(message.Body))
{
await _queueService.EnqueueChatbotMessageAsync(new Resgrid.Model.Queue.ChatbotMessageQueueItem
{
DepartmentId = DepartmentId,
From = UserId,
Body = message.Body,
MessageId = message.ChatMessageId,
Platform = (int)Resgrid.Chatbot.Models.ChatbotPlatform.WebChat
});
// Typing indicator to the user's devices while the worker runs the pipeline.
_eventAggregator.SendMessage<ChatEventRaised>(new ChatEventRaised
{
DepartmentId = DepartmentId,
ChatChannelId = channel.ChatChannelId,
Kind = ChatEventKinds.ChatbotTyping,
TargetUserId = UserId,
PayloadJson = Newtonsoft.Json.JsonConvert.SerializeObject(new { channel.ChatChannelId, IsTyping = true })
});
}
}
catch (Exception ex)
{
Framework.Logging.LogException(ex, "Failed to enqueue chatbot message from generic SendMessage.");
}Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 826:
Unhandled exception risk in the chatbot-enqueue block (lines 826-847) which sits outside the try-catch handling SendMessageAsync errors, allowing exceptions from EnqueueChatbotMessageAsync or _eventAggregator.SendMessage to propagate unhandled after the message is already persisted. Wrap the enqueue block in a try-catch that logs the exception and continues returning the already-saved message, matching the pattern in ChatbotController.SendChatMessage (lines 365-413).
Suggested Code:
if (message == null)
return BadRequest();
// The assistant channel is also reachable from the regular chat page, so sends that arrive
// through this generic endpoint must still feed the chatbot pipeline (the dedicated
// ChatbotController.SendChatMessage does the same for the assistant panel).
try
{
var channel = await _chatChannelService.GetChannelByIdAsync(channelId);
if (channel != null && channel.ChannelType == (int)ChatChannelType.Chatbot && !String.IsNullOrWhiteSpace(message.Body))
{
await _queueService.EnqueueChatbotMessageAsync(new Resgrid.Model.Queue.ChatbotMessageQueueItem
{
DepartmentId = DepartmentId,
From = UserId,
Body = message.Body,
MessageId = message.ChatMessageId,
Platform = (int)Resgrid.Chatbot.Models.ChatbotPlatform.WebChat
});
// Typing indicator to the user's devices while the worker runs the pipeline.
_eventAggregator.SendMessage<ChatEventRaised>(new ChatEventRaised
{
DepartmentId = DepartmentId,
ChatChannelId = channel.ChatChannelId,
Kind = ChatEventKinds.ChatbotTyping,
TargetUserId = UserId,
PayloadJson = Newtonsoft.Json.JsonConvert.SerializeObject(new { channel.ChatChannelId, IsTyping = true })
});
}
}
catch (Exception ex)
{
Framework.Logging.LogException(ex, "Failed to enqueue chatbot message from generic SendMessage.");
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Honor the admin toggle from Department Settings > Assistant. Matching the ingress | ||
| // pipeline's semantics, a missing config row means enabled. | ||
| var config = await _departmentConfigService.GetConfigAsync(DepartmentId); |
There was a problem hiding this comment.
Unguarded awaited async operation in _departmentConfigService.GetConfigAsync violates Rule [1], which requires every awaited async operation to be wrapped in try/catch. Wrap the await in a try/catch block that logs with DepartmentId and operation context, returning a safe default (e.g., false) or rethrowing as appropriate.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 478:
Unguarded awaited async operation in _departmentConfigService.GetConfigAsync violates Rule [1], which requires every awaited async operation to be wrapped in try/catch. Wrap the await in a try/catch block that logs with DepartmentId and operation context, returning a safe default (e.g., false) or rethrowing as appropriate.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
PR Description
This pull request addresses several fixes and improvements for the chatbot feature (now referred to as "Assistant"):
1. Rebranding "Chatbot" to "Assistant"
Updates all user-facing text strings across the chatbot handlers, ingress services, and localization files (English, Arabic, German, Spanish, French, Italian, Polish, Swedish, Ukrainian) to replace the term "Chatbot" with "Assistant." This includes error messages, help text, system messages, department settings labels, and profile security PIN descriptions.
2. Fix: Assistant channel messages from regular chat now trigger the chatbot pipeline
When a user sends a message to the assistant (Chatbot-type) channel through the generic chat
SendMessageendpoint (e.g., from the regular chat page), the message is now also enqueued to the chatbot processing pipeline. Previously, only the dedicatedChatbotControllerendpoint fed the pipeline, meaning messages sent from the standard chat UI to the assistant channel were not processed. A typing indicator is also sent to the user's devices while the pipeline runs.3. Fix: Assistant chat now respects the department-level Assistant enable/disable toggle
The
ChatbotChatEnabledAsynccheck inChatbotControllernow additionally evaluates theDepartmentConfig.IsEnabledsetting (from Department Settings > Assistant). Previously, it only checked the chat-specificChatbotEnabledsetting, allowing the assistant to remain accessible even when an administrator had disabled it at the department level.4. Fix: Proper handling of v4 API envelope in chat API client
The
getChatbotChannelfunction in the frontend chat API now correctly unwraps the response payload nested under theDataproperty of the v4 API envelope, while maintaining backward compatibility with flat payloads. This fixes issues where the assistant channel could not be properly resolved from the API response.