Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 Walkthrough<review_stack_artifact_start> Introduces the complete Phase 3 chatbot feature: NLU providers and extractor, core models/interfaces/services (ingress, routing, sessions, conversation, rate limiting), action handlers, platform adapters and outbound delivery, data models/repos/migrations, web controllers/UI, tooling tweaks, and tests.All changed ranges from the PR presented in a single checkpoint for deterministic hydration.range_cea92a553784 range_a7812231a15e range_ff12e8e41248 range_7bb2779c5a84 range_8ba4895a5d90 range_ca25ab40dcb0 range_f0f48c9b5dd6 range_404513cece05 range_074cb145b4cf range_7d7ab260785f range_7cb5fad29cac range_eccb2f7592f9 range_ff6d6de60fa3 range_d13515805320 range_f7b0e6a8766a range_88908687da55 range_ac29ca13cdbb range_b080fe788603 range_27cabff46adf range_3fd15092b49f range_a06035563732 range_405c7d217774 range_f1d4f78c13f8 range_524910f23da3 range_f88ba8eb7381 range_afa79fca2cc5 range_afa79fca2cc5 range_955c2f169921 range_2bb56a931e52 range_be80b58311d2 range_2359fd42dbdb range_b82f59e013bf range_04cbfe54e9a2 range_0bfb93697748 range_10019fdf6350 range_c687484bb66c range_240e3ce63124 range_9a4df13e0f24 range_3691ccde6db7 range_09f8a8a588d3 range_42e21c0d91cd range_5d5a6e386427 range_a6b46fa323be range_1e703b1ccf69 range_4e78fa5bd735 range_ced711d671d0 range_05baa39373c0 range_950e309e46b6 range_6ba8bd1035a4 range_07e6415c50f8 range_4a3047c2926f range_95abb998371a range_2d3be0b6a900 range_42f7315568a4 range_5fb33b55272e range_5ddfe237342d range_db6eb6ad59b6 range_5543cae7dca3 range_29cdd049fb55 range_425637659863 range_9e726817a9f6 range_ca84606132c4 range_cb49b9c4ef68 range_34c624e3f36e range_bb104d9c4466 range_b2d3d0dcd472 range_da1f03d8d11c range_ccf7f4eee7fc range_00fb5cce12fa range_46c7c678ff99 range_dde6c565e7d1 range_9ac96ba7df77 range_0f05fcfe5e09 range_a6b3590c127a range_1fd05b553685 range_5e0640279305 range_56dc8985ec79 range_0c4c3dc33f76 range_ad2383c73ad1 range_29880d8c6287 range_f8bbbcf3d821 range_48ad7674fcdc range_384eafb271fa range_22cb89d47c9f range_2336fbb58d73 range_cba6628afa45 range_891996abf255 range_ed748e1a7189 range_845abdde837d range_893b9851376e range_0a4a2e8622a2 range_b93d80948124 range_be49bf528fd9 range_d065f76e1c2c range_d7397ac6ec52 range_65e630d4c967 range_3c056cb08add range_90db16e49957 range_4478408f42d2 range_a2a794e95181 range_3938482cd43c range_9c60b3bb6c17 range_c51e0fb99fc9 range_96f57e5cddea range_11273ea75dcb range_6f1edcba2756 range_fe7eb06b70fe range_3784fd92d969 range_e647466f9181 range_76ba38b24f71 range_ea85b1d7e62e range_86c831440d50 range_7093fbffd508 range_83a8b21f1827 range_258d62bfc13d range_74061b53bc14 range_9583e96f8f01 range_02d9abdc3922 range_b9447cc1a11d range_93c700dd44f5 range_e8ee002692bc range_cddbcc09d9c3 range_c33baf93da8a range_3454aca715eb range_6312ccd2229f range_47ee2aac925b range_3425b5c8ada8 range_c09cb6846754 range_465e002c5070 range_ac3fdce9764c range_5c08b85b6bce range_4f1969434620 range_b9c7d4d961c6 range_300994fb29ce range_a254bd8721a4 range_9b375aa03f2d range_2620c9bc83bc range_a2350759054b range_11273ea75dcb range_9b949c33d40b range_0fb19d53ba33 range_22a1b843a9ff range_f2c204a91c22 range_9fed7eecbbb0 range_6c51ca13cec3 range_08763b08b89f range_ff4a4da49016 range_727bc519692d range_280ff4fe5f45 range_3cccbc2350ef range_af0d23dec714 range_6e406cb568e5 range_bc5b26b44354 range_7ded804e7027 range_ce6437ac18a9 range_087c276a3a10 range_75462035fd6c range_d4003f6cba66 range_cfb6784347c8 range_5609b8730f23 range_a10efe4c1a4d range_60729aed6135 range_e91c99ee0c65 range_0ab19ea97143 range_501864285e4c range_87d804216f3a range_c500bf3be74e range_56ef6cd68189 range_0ba72a669bea range_069fbaa00372 range_abc7bd43eb57 range_43603335f5b0 range_a64e1dba8aab range_47a8623e3ea7 range_21f147357762 range_6c7451ab7271 range_d6926ea057b7 range_ba97d600b402 range_99ca1902fb11 range_5fb05548c634 range_8363c54178e7 range_6312ccd2229f range_ce5a622cb53e range_a4356ac9d559 range_03fe3d0b18c1 range_c1c15499ddb1 range_a2b9c921ad65 range_ed063e146fdd range_c6adffe41dca range_018c7402f8c1 range_353bc49ab935 range_e9856291d29d range_0268715a3fef range_4c2b1d9ab592 range_20004e88563f range_508e93089467 range_d0a49eab9858 range_5b8c50ab2318 range_0443caadebfc range_9e6160d94340 range_b76ab0b4cc92 range_1853ac62610a range_c76be6d258c1 range_24c01b4ea70f range_ea6e901b8d32 range_bb9d093169fa range_1de7eb757cce range_a06b2ddcbaba range_f0f4fba4f4f3 range_c132a1e16f7e range_f5bf26210331 range_d128991ac474 range_ad2be7a2b2a0 range_73654e116347 range_5b25ce8298a8 range_b4580eeb80d7 range_534e6929f338 range_d78e3f9e514b range_ad810f6ebc75 range_13b18bb3447a range_323d89f075a6 range_08c7e5afde97 range_66294f3a0b6a range_68d3a503f8d2 range_cb40ad53aebf range_a35a2bb64660 range_d44310c3a6d6 range_112aff430651 range_6b0bb07dd881 range_4f94235af8be range_2eff3da93709 range_7471abf2aaa0 range_fec1cd6caa66 range_857e5727a165 range_918261b55777✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost("Webhook")] | ||
| [Consumes("application/json")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> Webhook() |
| /// </summary> | ||
| [HttpPost("GenerateLinkingCode")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> GenerateLinkingCode() |
| /// </summary> | ||
| [HttpPost("UnlinkAccount")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> UnlinkAccount([FromBody] UnlinkRequest request) |
| /// </summary> | ||
| [HttpPost("OAuthComplete")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> OAuthComplete([FromBody] OAuthCompleteRequest request) |
| { | ||
| try | ||
| { | ||
| if (request == null) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/settings.local.json (1)
35-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop committing user-local Claude config; remove tracked
.claude/settings.local.jsonand use a template.
.claude/settings.local.jsonhardcodes developer-specific paths in the MCP hooks (e.g.,/Users/shawn/.graperoot-pro/...and/Volumes/USBSSD/...in thePreToolUse/PostToolUsecommands around lines 41 and 52). Even though.gitignorealready includes.claude/settings.local.json, the file is currently tracked, so other contributors/CI will still pick up these broken paths.
- Untrack it (
git rm --cached .claude/settings.local.json) and rely on.gitignore- Commit a
.claude/settings.local.json.examplewith placeholders (or environment variables/relative paths) for local customization🤖 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 @.claude/settings.local.json around lines 35 - 56, The tracked .claude/settings.local.json contains developer-local MCP hooks (PreToolUse/PostToolUse) that hardcode DG_DATA_DIR and absolute paths to graph_gate.py/graph_sync.py; untrack this file with git rm --cached .claude/settings.local.json and commit that change, then add a new .claude/settings.local.json.example that preserves the PreToolUse and PostToolUse keys but replaces the hardcoded DG_DATA_DIR and the absolute python/script paths with placeholders or environment-variable-based values (e.g., ${DG_DATA_DIR}, ${VENV_PYTHON}, ${GRAPH_SCRIPT}) and a short comment explaining how to customize locally, ensuring .gitignore continues to ignore the real settings.local.json.
🧹 Nitpick comments (18)
Core/Resgrid.Chatbot/Services/ChatbotUserSearchService.cs (1)
67-80: ⚡ Quick winRefine search-correctness concern: multi-token matching relies on computed
NamefromFirstName/LastName
UserGroupRole.Nameis not an independently populated field—it’s computed as"{FirstName} {LastName}". For multi-token queries like"john smith",MatchesQuerycan only match viau.Name(sinceFirstName/LastNameContainswon’t match the space-separated full string).ResolveSingleAsync’s exact-match path also compares againstm.FullName(which comes fromu.Name), so missing/blank/oddly-formattedFirstNameorLastNamewill lead to no match.🤖 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.Chatbot/Services/ChatbotUserSearchService.cs` around lines 67 - 80, MatchesQuery and the produced ChatbotUserMatch.FullName rely on UserGroupRole.Name which is just FirstName + " " + LastName and can be missing/misformatted, causing multi-token queries (e.g., "john smith") to fail; update MatchesQuery to build/compare against an explicit combined full name from u.FirstName and u.LastName (and/or split the incoming lowerQuery into tokens and ensure each token matches FirstName, LastName, or the constructed full name) and update ToMatch to set FullName from the same constructed string (not u.Name) so ResolveSingleAsync exact-match logic uses a reliable FullName source; make changes in MatchesQuery, ToMatch, and any code paths in ResolveSingleAsync that compare against m.FullName.Core/Resgrid.Chatbot/Interfaces/IIntentMapper.cs (1)
4-9: 💤 Low valueNamespace inconsistent with sibling interfaces.
This interface lives in the
Interfaces/folder but is declared underResgrid.Chatbot.Services, whereas the other new chatbot interfaces (IEntityExtractor,IChatbotIntentRouter) useResgrid.Chatbot.Interfaces. Consider aligning for discoverability.🤖 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.Chatbot/Interfaces/IIntentMapper.cs` around lines 4 - 9, The IIntentMapper interface is declared in the Resgrid.Chatbot.Services namespace but should follow its sibling interfaces' namespace for consistency; change the namespace declaration for IIntentMapper to Resgrid.Chatbot.Interfaces so it aligns with IEntityExtractor and IChatbotIntentRouter. Ensure the file still exposes the same members (IIntentMapper, MapToIntent, NLUResult, ChatbotIntent) and update any using/imports or references that relied on the old Resgrid.Chatbot.Services namespace.Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs (1)
29-57: ⚖️ Poor tradeoffConstructor injection conflicts with the project's dependency-resolution convention.
This service takes 13 injected dependencies via the constructor. The repository convention is to resolve dependencies explicitly with the Service Locator (
Bootstrapper.GetKernel().Resolve<T>()) and to keep constructor injection minimal. The large dependency count is also a cohesion smell worth splitting (identity/auth gating vs. session/dialog orchestration).As per coding guidelines: "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection" and "Minimize constructor injection; keep the number of injected dependencies small".🤖 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.Chatbot/Services/ChatbotIngressService.cs` around lines 29 - 57, The ChatbotIngressService constructor currently injects 13 dependencies (_userIdentityService, _sessionManager, _intentRouter, _actionHandlers, _conversationEngine, _templateRenderer, _userProfileService, _departmentsService, _departmentSettingsService, _limitsService, _authorizationService, _departmentConfigService, _rateLimiter), which violates the repo convention and indicates low cohesion; refactor the constructor to only accept the minimal, truly required collaborators (e.g., keep critical ones like IChatbotUserIdentityService or IChatbotSessionManager if necessary) and resolve the rest inside the class via the service locator (Bootstrapper.GetKernel().Resolve<T>()), or split responsibilities into separate services (e.g., an Auth/Identity helper and a Dialog/Orchestration helper) so ChatbotIngressService no longer requires all 13 injected dependencies—update field initialization to use Resolve<T>() for the moved dependencies and adjust usages to the new helpers or resolved instances.Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs (1)
107-110: ⚡ Quick winConsider using Service Locator pattern instead of constructor injection.
The codebase guideline specifies: "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection." This constructor currently uses constructor injection.As per coding guidelines: Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, 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 `@Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs` around lines 107 - 110, The constructor for OpenAiCompatibleNluProvider currently uses constructor injection for IChatbotDepartmentConfigService; replace this with the Service Locator pattern by removing the ctor parameter and resolving the dependency inside the constructor with Bootstrapper.GetKernel().Resolve<IChatbotDepartmentConfigService>(), assigning the result to the existing _configService field so the provider uses the resolved instance instead of an injected one.Providers/Resgrid.Providers.Chatbot/Services/ChatbotOutboundService.cs (1)
25-33: ⚡ Quick winConsider using Service Locator pattern instead of constructor injection.
The codebase guideline specifies: "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection." This constructor currently uses constructor injection for three dependencies.As per coding guidelines: Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, 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 `@Providers/Resgrid.Providers.Chatbot/Services/ChatbotOutboundService.cs` around lines 25 - 33, The constructor for ChatbotOutboundService currently uses constructor injection for IChatbotUserIdentityService, IChatbotAdapterRegistry, and IChatbotDepartmentConfigService; change it to use the Service Locator pattern by removing those parameters and resolving each dependency inside the constructor via Bootstrapper.GetKernel().Resolve<T>() (resolve IChatbotUserIdentityService into _identityService, IChatbotAdapterRegistry into _adapterRegistry, and IChatbotDepartmentConfigService into _departmentConfigService) so the class adheres to the guideline of using Bootstrapper.GetKernel().Resolve<T>() for dependency resolution.Core/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs (1)
88-99: 💤 Low valueOptional: per-user N+1 async calls.
For each of up to 15 users this awaits
GetCustomPersonnelStatusAsyncandGetCustomPersonnelStaffingAsyncsequentially. If these aren't cached, consider fetching custom states once for the department and resolving in-memory, or at least parallelizing. Bounded at 15 so impact is limited.🤖 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.Chatbot/Handlers/PersonnelActionHandler.cs` around lines 88 - 99, The loop in PersonnelActionHandler.cs makes per-user sequential awaits to GetCustomPersonnelStatusAsync and GetCustomPersonnelStaffingAsync, causing N+1 async calls; instead either (a) prefetch all custom states/staffings for the department once (use _customStateService to load department-wide lists and then resolve status/staffing by matching lastActionLogs and userStates in-memory using UserId) or (b) parallelize the per-user calls by creating Tasks for GetCustomPersonnelStatusAsync/ GetCustomPersonnelStaffingAsync for each user and awaiting Task.WhenAll before building the output; reference the userList loop, lastActionLogs, userStates, GetCustomPersonnelStatusAsync and GetCustomPersonnelStaffingAsync when applying the change.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0068_ChatbotTablesPg.cs (1)
55-76: Consider a retention/cleanup policy forChatbotMessageLog.
MessageTextpersists raw user-entered content indefinitely, which can include PII. There's no TTL/cleanup defined in this migration. Consider a scheduled purge or retention window to limit privacy exposure.🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0068_ChatbotTablesPg.cs` around lines 55 - 76, The ChatbotMessageLog table stores raw MessageText indefinitely (privacy risk); add a retention mechanism by modifying the migration that creates ChatbotMessageLog to include a nullable ExpireAt (or RetentionUntil) DateTime column and an index on it, and implement a scheduled cleanup job (or DB-side policy) that deletes rows where COALESCE(ExpireAt, Timestamp) is older than your retention window; update the Create.Table call for "ChatbotMessageLog" and reference the MessageText and Timestamp columns in the cleanup logic so old user-entered content is purged automatically.Providers/Resgrid.Providers.Migrations/Migrations/M0068_ChatbotTables.cs (1)
27-30: Consider enforcing uniqueness for platform identity lookups.
GetIdentityAsync(platform, platformUserId)resolves a single identity, butIX_ChatbotUserIdentities_Platform_PlatformUserIdis non-unique, so duplicate active rows for the same(Platform, PlatformUserId)would make the lookup ambiguous. BecauseIsActivesoft-deletes are in play, a plain unique index would break re-linking — a filtered unique index on active rows (e.g.,WHERE IsActive = 1) would be the safer enforcement.🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0068_ChatbotTables.cs` around lines 27 - 30, The non-unique index IX_ChatbotUserIdentities_Platform_PlatformUserId on ChatbotUserIdentities allows duplicate (Platform, PlatformUserId) rows and makes GetIdentityAsync ambiguous; change the migration to create a unique filtered index on (Platform, PlatformUserId) that only applies to active records (e.g., WHERE IsActive = 1) so uniqueness is enforced for active identities while allowing soft-deleted rows to coexist.Core/Resgrid.Chatbot/Handlers/CallsActionHandler.cs (1)
16-31: ⚡ Quick winDrop unused injected dependencies.
_customStateServiceand_userProfileServiceare injected but never referenced inHandleAsync. Removing them simplifies construction and resolution.As per coding guidelines: "Minimize constructor injection; keep the number of injected dependencies small".♻️ Proposed cleanup
public class CallsActionHandler : IChatbotActionHandler { private readonly ICallsService _callsService; private readonly IDepartmentsService _departmentsService; - private readonly ICustomStateService _customStateService; - private readonly IUserProfileService _userProfileService; public CallsActionHandler( ICallsService callsService, - IDepartmentsService departmentsService, - ICustomStateService customStateService, - IUserProfileService userProfileService) + IDepartmentsService departmentsService) { _callsService = callsService; _departmentsService = departmentsService; - _customStateService = customStateService; - _userProfileService = userProfileService; }🤖 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.Chatbot/Handlers/CallsActionHandler.cs` around lines 16 - 31, Remove the unused injected dependencies _customStateService and _userProfileService from the CallsActionHandler: delete their private fields, remove the corresponding constructor parameters (customStateService, userProfileService) and assignments in the CallsActionHandler constructor, and update any DI registration if present; ensure HandleAsync and the class compile with only _callsService and _departmentsService injected.Tests/Resgrid.Tests/Chatbot/ChatbotDeptConfigAndSessionTests.cs (1)
23-30: 💤 Low valueGlobal flag mutation isn't restored.
CacheMock()flips the process-wideSystemBehaviorConfig.CacheEnabledtofalseand never restores it. If any other fixture relies on the default value, this can introduce order-dependent flakiness. Consider saving/restoring it in[SetUp]/[TearDown](or[OneTimeTearDown]).🤖 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 `@Tests/Resgrid.Tests/Chatbot/ChatbotDeptConfigAndSessionTests.cs` around lines 23 - 30, The CacheMock method mutates the global Resgrid.Config.SystemBehaviorConfig.CacheEnabled flag and never restores it, so update the ChatbotDeptConfigAndSessionTests fixture to preserve and restore that flag: capture the original value in a test fixture setup (e.g., OneTimeSetUp or SetUp) into a private field, leave CacheMock to set CacheEnabled = false if necessary, and then restore the saved value in the matching teardown (OneTimeTearDown or TearDown); alternatively change CacheMock to avoid touching SystemBehaviorConfig at all and instead mock configuration behavior locally—reference CacheMock and the ChatbotDeptConfigAndSessionTests class when making the change.Core/Resgrid.Chatbot/Services/OAuthLinkingService.cs (1)
251-259: ⚡ Quick winPotential disposal ordering issue in PostFormAsync.
Lines 254 and 258: The
using var responsedisposes theHttpResponseMessagebefore the return statement completes. WhileReadAsStringAsync()typically buffers the content and this code may work in practice, the disposal occurs before the async read completes, which can be fragile and implementation-dependent.♻️ Ensure content is read before disposal
private static async Task<JsonDocument> PostFormAsync(string url, Dictionary<string, string> form) { using var content = new FormUrlEncodedContent(form); using var response = await _http.PostAsync(url, content); if (!response.IsSuccessStatusCode) return null; - return JsonDocument.Parse(await response.Content.ReadAsStringAsync()); + var json = await response.Content.ReadAsStringAsync(); + return JsonDocument.Parse(json); }This ensures the content is fully read into memory before the response is disposed.
🤖 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.Chatbot/Services/OAuthLinkingService.cs` around lines 251 - 259, PostFormAsync disposes the HttpResponseMessage (response) before the async read completes which can be fragile; to fix, read the response content into a string (await response.Content.ReadAsStringAsync()) while the response is still in scope, then dispose response and parse the JSON from the buffered string; modify PostFormAsync to capture the content string before returning JsonDocument.Parse, keeping usage of FormUrlEncodedContent and _http.PostAsync but ensuring response is not disposed prior to completing the content read.Core/Resgrid.Chatbot/Services/ChatbotTemplateRenderer.cs (1)
197-258: ⚡ Quick winConvert public fields to properties in template model classes.
Lines 199-201, 206, 213, 218, 224-225, 232, 239, 246, 251, and 256 declare template model classes and nested entry classes using public fields instead of properties. Modern C# best practice is to use properties (at minimum auto-properties), which provide better encapsulation, support data binding, and allow future addition of validation or change notification.
♻️ Convert fields to auto-properties
public class CallsListModel { - public string DepartmentName; - public CallEntry[] Calls; - public class CallEntry { public string Number; public string Name; public string Nature; public string Address; } + public string DepartmentName { get; set; } + public CallEntry[] Calls { get; set; } + public class CallEntry + { + public string Number { get; set; } + public string Name { get; set; } + public string Nature { get; set; } + public string Address { get; set; } + } } public class CallDetailModel { - public string Number; public string Name; public string Nature; public string Address; public string Priority; public string State; + public string Number { get; set; } + public string Name { get; set; } + public string Nature { get; set; } + public string Address { get; set; } + public string Priority { get; set; } + public string State { get; set; } }Apply the same pattern to the remaining model classes (UnitsListModel, StatusResponseModel, MessagesListModel, CalendarListModel, ShiftsListModel, PersonnelListModel, HelpModel, ErrorModel).
As per coding guidelines, "Use modern C# features appropriately."
🤖 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.Chatbot/Services/ChatbotTemplateRenderer.cs` around lines 197 - 258, Convert all public fields in the template model classes to public auto-properties: replace field declarations in CallsListModel (DepartmentName, Calls and nested CallEntry fields Number, Name, Nature, Address), CallDetailModel (Number, Name, Nature, Address, Priority, State), UnitsListModel (DepartmentName, Units and nested UnitEntry fields Name, Status), StatusResponseModel (StatusName, StaffingLevel), MessagesListModel (UnreadCount, Messages and nested MessageEntry fields Id, Subject, From, Date), CalendarListModel (DepartmentName, Events and nested EventEntry fields Start, Title, Type), ShiftsListModel (DepartmentName, Shifts and nested ShiftEntry fields Start, End, Name, Station), PersonnelListModel (DepartmentName, Personnel and nested PersonnelEntry fields Name, Status, Staffing, Destination), HelpModel (HelpText), and ErrorModel (Message) into auto-properties (e.g., public string DepartmentName { get; set; }) to follow modern C# conventions and enable future extensibility.Core/Resgrid.Chatbot/Interfaces/IChatbotTemplateRenderer.cs (1)
8-9: ⚡ Quick winConsider using generics instead of
objectfor the model parameter.Both methods use
object model, which loses compile-time type safety and increases the risk of runtime errors if incorrect types are passed. Using a generic type parameter would provide better type checking and align with functional programming principles.♻️ Proposed refactor using generics
public interface IChatbotTemplateRenderer { - string Render(string templateName, object model, ChatbotPlatform platform); - Task<ChatbotResponse> RenderResponseAsync(string templateName, object model, ChatbotPlatform platform, ChatbotIntent intent); + string Render<TModel>(string templateName, TModel model, ChatbotPlatform platform); + Task<ChatbotResponse> RenderResponseAsync<TModel>(string templateName, TModel model, ChatbotPlatform platform, ChatbotIntent intent); }As per coding guidelines, prefer functional patterns and use modern C# features appropriately; generics provide stronger type guarantees than
object.🤖 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.Chatbot/Interfaces/IChatbotTemplateRenderer.cs` around lines 8 - 9, The interface IChatbotTemplateRenderer currently uses object for the model parameter in Render and RenderResponseAsync which sacrifices compile-time type safety; change both method signatures to be generic (e.g., Render<TModel>(string templateName, TModel model, ChatbotPlatform platform) and RenderResponseAsync<TModel>(string templateName, TModel model, ChatbotPlatform platform, ChatbotIntent intent)), update all implementing classes and callers to supply the concrete TModel type, and ensure any reflection/serialization logic inside those implementations still handles the generic type (or add type constraints if needed).Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs (1)
28-49: 💤 Low valueGET
Indexlacks the error handling the POST has.
GetConfigAsynccan hit cache/DB/decryption paths; an exception here surfaces an unhandled error page, unlike the POST which catches and logs. Consider wrapping consistently if config load failures are plausible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs` around lines 28 - 49, The Index GET currently calls _chatbotConfigService.GetConfigAsync(DepartmentId) without error handling which can surface unhandled exceptions; wrap the await and model mapping inside a try/catch in the Index() action, catch Exception (ex), log the exception using the existing logger (e.g. _logger.LogError(ex, "...")) and then populate a safe default ChatbotSettingsModel (same fields as now with null/false defaults) and return View(model) while adding a user-visible error (ModelState.AddModelError or TempData) so the page renders instead of throwing; reference Index(), _chatbotConfigService.GetConfigAsync, ChatbotSettingsModel and DepartmentId when making the change.Core/Resgrid.Chatbot.NLU/Services/EntityExtractor.cs (1)
44-44: 💤 Low valueRedundant condition.
"units"contains the substring"unit", sointentName?.Contains("unit") == truealready covers the second clause; theContains("units")check is dead.♻️ Simplify
- if (intentName?.Contains("unit") == true || intentName?.Contains("units") == true) + if (intentName?.Contains("unit") == true)🤖 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.Chatbot.NLU/Services/EntityExtractor.cs` at line 44, The if condition in EntityExtractor.cs currently checks intentName?.Contains("unit") == true || intentName?.Contains("units") == true which is redundant because "units" contains "unit"; simplify the condition to only check intentName?.Contains("unit") == true (or intentName?.Contains("unit", StringComparison.OrdinalIgnoreCase) if case-insensitive matching is needed) to remove the dead Contains("units") clause and keep the logic intact; update the conditional where intentName is evaluated in EntityExtractor.cs accordingly.Core/Resgrid.Chatbot/Handlers/UnitsActionHandler.cs (1)
44-50: 💤 Low valueUnit list is silently truncated at 15.
When a department has more than 15 units the extra ones are dropped without any indication to the user. Consider appending a localized "showing 15 of N" hint so users know the list is incomplete.
♻️ Optional hint for truncated lists
var unitList = unitStatuses.Take(15).ToList(); foreach (var unitState in unitList) { var status = await _customStateService.GetCustomUnitStateAsync(unitState); var statusText = status?.ButtonText ?? ChatbotResources.Get("Personnel_Unknown", culture); sb.AppendLine(ChatbotResources.Get("Units_Line", culture, unitState.Unit?.Name, statusText)); } + + if (unitStatuses.Count() > 15) + sb.AppendLine(ChatbotResources.Get("Units_More", culture, unitStatuses.Count() - 15));🤖 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.Chatbot/Handlers/UnitsActionHandler.cs` around lines 44 - 50, The code truncates unitStatuses to 15 via var unitList = unitStatuses.Take(15).ToList() without informing the user; update UnitsActionHandler (around unitList and the loop using _customStateService.GetCustomUnitStateAsync and sb.AppendLine) to compute the total count (e.g., total = unitStatuses.Count()), only take the first 15 for display, and if total > 15 append a localized hint line (use ChatbotResources.Get with a new or existing key like "Units_Truncated" and culture) such as "showing 15 of N" so users know the list is incomplete; ensure the hint is only added when total > unitList.Count().Providers/Resgrid.Providers.Chatbot/Adapters/DiscordBotAdapter.cs (1)
108-112: 💤 Low valueMove
IChatbotPlatformAdapterConfigto its own file.This interface is platform-agnostic (it resolves token/bot id for any
ChatbotPlatform) yet lives inside the Discord-specific adapter file, which hurts discoverability. Consider relocating it to theInterfacesfolder alongside the other adapter contracts.🤖 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 `@Providers/Resgrid.Providers.Chatbot/Adapters/DiscordBotAdapter.cs` around lines 108 - 112, The IChatbotPlatformAdapterConfig interface is platform-agnostic and should be extracted from DiscordBotAdapter.cs into its own file named IChatbotPlatformAdapterConfig.cs inside the Interfaces folder; create the new file with the same public interface declaration (string GetToken(ChatbotPlatform platform); string GetBotId(ChatbotPlatform platform);), update namespaces/usings in the new file to match the other adapter contracts, remove the interface declaration from DiscordBotAdapter.cs, and update any files (including DiscordBotAdapter and any consumers) to reference the moved interface's namespace; rebuild to ensure no missing references and run tests.Core/Resgrid.Chatbot/Services/ConversationEngine.cs (1)
46-106: ⚖️ Poor tradeoffDuplicated state machine.
HandleContinuationAsyncre-implements the sameAwaitingParameter/AwaitingConfirmation/AwaitingAuthtransitions already handled byHandleParameterResponse/HandleConfirmationResponse/HandleAuthResponse, just returningChatbotResponseinstead ofConversationResult. Two divergent copies of this logic are easy to drift (the fake-send bug already exists in both). Consider consolidating to a single source of truth.🤖 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.Chatbot/Services/ConversationEngine.cs` around lines 46 - 106, HandleContinuationAsync duplicates the AwaitingParameter/AwaitingConfirmation/AwaitingAuth state handling already implemented in HandleParameterResponse/HandleConfirmationResponse/HandleAuthResponse causing drift; refactor HandleContinuationAsync to delegate to those existing methods (passing ChatbotMessage and ChatbotSession) and map their ConversationResult into a ChatbotResponse (or update the called methods to return/produce a ChatbotResponse via a small adapter), ensuring you preserve session updates on ChatbotSession and reference symbols: HandleContinuationAsync, HandleParameterResponse, HandleConfirmationResponse, HandleAuthResponse, ConversationResult, ChatbotResponse, ChatbotSession, ChatbotMessage; remove the inline switch logic so there is a single source of truth for each state transition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Chatbot.NLU/Services/EntityExtractor.cs`:
- Around line 110-124: The time regex in ExtractTimes currently allows a lone
1–2 digit number because both the minutes group and the am/pm group are
optional; update the pattern assigned to timePattern inside ExtractTimes so it
requires at least minutes or an am/pm suffix (e.g. change the groups so the
digits are followed by either :mm or an am/pm token using an alternation like
(?::\d{2}|(?:\s*(?:am|pm|AM|PM))) ), then reuse the updated timePattern and
match.Value/NormalizedValue as before to add the ChatbotEntity; keep the entity
creation in ExtractTimes unchanged apart from using the new stricter
timePattern.
In `@Core/Resgrid.Chatbot/Handlers/CallDetailActionHandler.cs`:
- Around line 19-24: Replace constructor dependency injection in
CallDetailActionHandler: remove the injected parameters from the
CallDetailActionHandler constructor and instead resolve the dependencies inside
the constructor using the Service Locator
(Bootstrapper.GetKernel().Resolve<T>()), assigning the resolved instances to
_callsService, _departmentsService, and _authorizationService so the class uses
service-locator resolution rather than constructor injection.
In `@Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs`:
- Around line 60-73: The code currently sets Priority to 0 when no priorities
are returned and uses a magic number for CallSource; update the handler to
explicitly validate the result of
_callsService.GetActiveCallPrioritiesForDepartmentAsync(session.DepartmentId)
and handle empty/null lists (either select a defined fallback priority id, fetch
a default via the service, or log/return an error instead of silently using 0)
before constructing the Call object, and replace the literal CallSource = 0 with
a named enum or constant (e.g., use an existing CallSourceType enum or introduce
CallSourceType.Chatbot and assign CallSource = (int)CallSourceType.Chatbot) so
Priority and CallSource are always valid and self-documenting.
In `@Core/Resgrid.Chatbot/Handlers/MessageDeleteHandler.cs`:
- Around line 19-22: The MessageDeleteHandler constructor currently takes
IMessageService via constructor injection; change it to use the repository's
Service Locator pattern by replacing the parameterized ctor with a parameterless
constructor that assigns _messageService =
Bootstrapper.GetKernel().Resolve<IMessageService>(); (keep the _messageService
field name), and update any callsites or tests that instantiate
MessageDeleteHandler to use the parameterless ctor. Ensure the class name
MessageDeleteHandler and the field _messageService are used exactly as in the
diff.
In `@Core/Resgrid.Chatbot/Handlers/MyStatusActionHandler.cs`:
- Around line 19-31: The constructor of MyStatusActionHandler currently uses
constructor injection for IActionLogsService, IUserStateService,
ICustomStateService, IUserProfileService, and IDepartmentsService; change it to
use the Service Locator pattern by calling Bootstrapper.GetKernel().Resolve<T>()
inside the MyStatusActionHandler constructor to obtain each dependency and
assign them to the corresponding fields (_actionLogsService, _userStateService,
_customStateService, _userProfileService, _departmentsService) instead of
accepting them as parameters, and remove those parameters from the ctor
signature so resolution is performed explicitly within the constructor.
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 204-238: The YES-confirmation branch mutates session.State and
session.PendingIntent before locating a matching handler, causing silent drop if
no handler is found; change the flow in ChatbotIngressService so you first find
confirmHandler = _actionHandlers.FirstOrDefault(h => h.CanHandle(pendingType))
and if confirmHandler is null return an explicit response (e.g., "Unable to
complete confirmation — please try again or contact support.") without changing
session.State or clearing PendingIntent; only after confirmHandler is non-null
call confirmHandler.HandleAsync(...), set confirmResponse.Intent, then clear
session.Context, set session.State = ChatbotDialogState.Idle,
session.PendingIntent = null and SaveSessionAsync(session).
- Line 105: You set identity.LastUsedAt = DateTime.UtcNow in
ChatbotIngressService.ProcessMessageAsync but never persist it; update the flow
to persist the change by invoking the existing persistence path (call
LinkUserAsync or otherwise call into _identityRepository.UpdateAsync/InsertAsync
via the _userIdentityService) after setting LastUsedAt so the new timestamp is
stored (refer to ChatbotIngressService.ProcessMessageAsync, identity.LastUsedAt,
LinkUserAsync, _userIdentityService, and
_identityRepository.UpdateAsync/InsertAsync).
In `@Core/Resgrid.Chatbot/Services/ChatbotUserIdentityService.cs`:
- Around line 34-42: GetIdentityByPhoneAsync currently calls
_identityRepository.GetByPlatformUserIdAsync(clean) which can return identities
from other platforms; change it to query by platform+user instead. Replace the
GetByPlatformUserIdAsync(clean) call in GetIdentityByPhoneAsync with a call to
_identityRepository.GetByPlatformAndUserAsync(smsPlatformId, clean) (or add/use
an SMS-scoped lookup) after calling CleanPhone(phoneNumber), and return ToModel
on that result so phone lookups are limited to the SMS platform.
In `@Core/Resgrid.Chatbot/Services/ConversationEngine.cs`:
- Around line 65-102: Several responses and confirmation parsing in
ConversationEngine are hardcoded English; update them to use
ChatbotResources.Get with session.Culture and make confirmation parsing
locale-aware. Replace the literal texts returned in the AwaitingConfirmation
case ("Confirmed.", "Cancelled.", "Please reply YES to confirm or NO to
cancel.") and the AwaitingAuth and default/parameter branches ("Processing your
linking code...", "Received. Processing your request...", prompts built by
BuildParameterPrompt) with resource lookups via ChatbotResources.Get(...,
session.Culture) using new/descriptive resource keys; for the
affirmative/negative checks around ChatbotDialogState.AwaitingConfirmation, use
a locale-aware helper (e.g., IsAffirmative/IsNegative taking message.Text and
session.Culture) rather than hardcoded "YES"/"NO"/"CANCEL" strings so
non-English locales parse confirmations correctly.
- Around line 196-208: The SendMessage flow currently returns a fake "Message
sent…" success in ConversationEngine.HandleParameterResponse and
HandleContinuationAsync even though ChatbotIngressService doesn't route
multi-turn intents through ProcessAsync; fix by removing the hardcoded success
branch for ChatbotIntentType.SendMessage (the missing-"body" case in
HandleParameterResponse) and instead route the collected "body" into the actual
MessageSendHandler (invoke the same handler used for single-turn sends) so the
message is truly sent and the real result is returned; also ensure
IsMultiTurnIntent(ChatbotIntentType.SendMessage) and NeedsParameterCollection
behavior are consistent (make SendMessage a multi-turn intent or adjust
branching so continuation paths hit the MessageSendHandler), and replace all
hardcoded English strings (e.g., "Message sent to …", parameter prompts,
fallback "I didn't understand…") with localized lookups via session.Culture and
ChatbotResources so user-facing text is localized.
In `@Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs`:
- Around line 247-251: The IncrementAsync implementation in
AzureRedisCacheProvider currently calls StringIncrementAsync then KeyExpireAsync
and returns 0 on exception, which can leave keys without TTL; change
IncrementAsync to perform an atomic INCR+EXPIRE using a Redis Lua script (or the
server-side EVAL) so the increment and setting of expiration happen in one
operation, reference the IncrementAsync method in AzureRedisCacheProvider and
replace the separate StringIncrementAsync/KeyExpireAsync calls with a single
EVAL that increments the key and sets expiration only when the key was newly
created; ensure the method returns the true counter value (not 0 on expire
failure) and surface/log errors instead of masking state to preserve
ICacheProvider semantics.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0070_ChatbotDepartmentConfigColumns.cs`:
- Line 13: The migration adds the LlmApiKey column as plaintext; change the
persistence to follow existing secret handling by storing an encrypted value
instead: update M0070_ChatbotDepartmentConfigColumns (the AddColumn("LlmApiKey")
definition) to match the pattern used by other secret columns (e.g., same column
name suffix/metadata or use the same encryption/storage helper), and ensure the
application layer (where ChatbotDepartmentConfig is written/read) uses the same
Encrypt/Decrypt or DataProtector helper used elsewhere to encrypt before saving
and decrypt after reading; review other migrations and the encryption helper you
find and apply the identical approach for LlmApiKey so secrets are encrypted at
rest.
In `@Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs`:
- Around line 91-99: The formattedResponse produced by
telegramAdapter.FormatOutboundResponseAsync(...) is never sent; after obtaining
formattedResponse, call the adapter's send method (e.g.,
telegramAdapter.SendOutboundAsync or SendMessageAsync) or perform the HTTP POST
to Telegram using the adapter's HTTP client with the formatted payload, and
await that call before returning Ok; if the adapter lacks a send method, update
FormatOutboundResponseAsync (or add a new SendOutboundAsync on telegramAdapter)
to both format and transmit the message so that the outgoing message created in
FormatOutboundResponseAsync is actually delivered to Telegram instead of only
returning Ok("ok").
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 8284-8287: Update the XML summary for the protocol result wrappers
so they describe protocol retrieval responses instead of “populate the New Call
form”: change the <summary> for
T:Resgrid.Web.Services.Models.v4.Protocols.GetAllProtocolsResult to something
like “Contains the list of protocols returned by the API” (or similar accurate
wording), and make the analogous change to the single-protocol result wrapper in
the same Resgrid.Web.Services.Models.v4.Protocols namespace so both
documentation entries reflect protocol retrieval rather than New Call form
population.
- Around line 8320-8322: Update the incorrect summary text in the XML comment
where the <summary> element currently reads "This this protocol disabled" and
change it to a grammatically correct form such as "Is this protocol disabled"
(preserve surrounding XML structure and capitalization); locate the <summary>
element in Resgrid.Web.Services.xml associated with the protocol description and
replace the text only, leaving other tags unchanged.
---
Outside diff comments:
In @.claude/settings.local.json:
- Around line 35-56: The tracked .claude/settings.local.json contains
developer-local MCP hooks (PreToolUse/PostToolUse) that hardcode DG_DATA_DIR and
absolute paths to graph_gate.py/graph_sync.py; untrack this file with git rm
--cached .claude/settings.local.json and commit that change, then add a new
.claude/settings.local.json.example that preserves the PreToolUse and
PostToolUse keys but replaces the hardcoded DG_DATA_DIR and the absolute
python/script paths with placeholders or environment-variable-based values
(e.g., ${DG_DATA_DIR}, ${VENV_PYTHON}, ${GRAPH_SCRIPT}) and a short comment
explaining how to customize locally, ensuring .gitignore continues to ignore the
real settings.local.json.
---
Nitpick comments:
In `@Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs`:
- Around line 107-110: The constructor for OpenAiCompatibleNluProvider currently
uses constructor injection for IChatbotDepartmentConfigService; replace this
with the Service Locator pattern by removing the ctor parameter and resolving
the dependency inside the constructor with
Bootstrapper.GetKernel().Resolve<IChatbotDepartmentConfigService>(), assigning
the result to the existing _configService field so the provider uses the
resolved instance instead of an injected one.
In `@Core/Resgrid.Chatbot.NLU/Services/EntityExtractor.cs`:
- Line 44: The if condition in EntityExtractor.cs currently checks
intentName?.Contains("unit") == true || intentName?.Contains("units") == true
which is redundant because "units" contains "unit"; simplify the condition to
only check intentName?.Contains("unit") == true (or intentName?.Contains("unit",
StringComparison.OrdinalIgnoreCase) if case-insensitive matching is needed) to
remove the dead Contains("units") clause and keep the logic intact; update the
conditional where intentName is evaluated in EntityExtractor.cs accordingly.
In `@Core/Resgrid.Chatbot/Handlers/CallsActionHandler.cs`:
- Around line 16-31: Remove the unused injected dependencies _customStateService
and _userProfileService from the CallsActionHandler: delete their private
fields, remove the corresponding constructor parameters (customStateService,
userProfileService) and assignments in the CallsActionHandler constructor, and
update any DI registration if present; ensure HandleAsync and the class compile
with only _callsService and _departmentsService injected.
In `@Core/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs`:
- Around line 88-99: The loop in PersonnelActionHandler.cs makes per-user
sequential awaits to GetCustomPersonnelStatusAsync and
GetCustomPersonnelStaffingAsync, causing N+1 async calls; instead either (a)
prefetch all custom states/staffings for the department once (use
_customStateService to load department-wide lists and then resolve
status/staffing by matching lastActionLogs and userStates in-memory using
UserId) or (b) parallelize the per-user calls by creating Tasks for
GetCustomPersonnelStatusAsync/ GetCustomPersonnelStaffingAsync for each user and
awaiting Task.WhenAll before building the output; reference the userList loop,
lastActionLogs, userStates, GetCustomPersonnelStatusAsync and
GetCustomPersonnelStaffingAsync when applying the change.
In `@Core/Resgrid.Chatbot/Handlers/UnitsActionHandler.cs`:
- Around line 44-50: The code truncates unitStatuses to 15 via var unitList =
unitStatuses.Take(15).ToList() without informing the user; update
UnitsActionHandler (around unitList and the loop using
_customStateService.GetCustomUnitStateAsync and sb.AppendLine) to compute the
total count (e.g., total = unitStatuses.Count()), only take the first 15 for
display, and if total > 15 append a localized hint line (use
ChatbotResources.Get with a new or existing key like "Units_Truncated" and
culture) such as "showing 15 of N" so users know the list is incomplete; ensure
the hint is only added when total > unitList.Count().
In `@Core/Resgrid.Chatbot/Interfaces/IChatbotTemplateRenderer.cs`:
- Around line 8-9: The interface IChatbotTemplateRenderer currently uses object
for the model parameter in Render and RenderResponseAsync which sacrifices
compile-time type safety; change both method signatures to be generic (e.g.,
Render<TModel>(string templateName, TModel model, ChatbotPlatform platform) and
RenderResponseAsync<TModel>(string templateName, TModel model, ChatbotPlatform
platform, ChatbotIntent intent)), update all implementing classes and callers to
supply the concrete TModel type, and ensure any reflection/serialization logic
inside those implementations still handles the generic type (or add type
constraints if needed).
In `@Core/Resgrid.Chatbot/Interfaces/IIntentMapper.cs`:
- Around line 4-9: The IIntentMapper interface is declared in the
Resgrid.Chatbot.Services namespace but should follow its sibling interfaces'
namespace for consistency; change the namespace declaration for IIntentMapper to
Resgrid.Chatbot.Interfaces so it aligns with IEntityExtractor and
IChatbotIntentRouter. Ensure the file still exposes the same members
(IIntentMapper, MapToIntent, NLUResult, ChatbotIntent) and update any
using/imports or references that relied on the old Resgrid.Chatbot.Services
namespace.
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 29-57: The ChatbotIngressService constructor currently injects 13
dependencies (_userIdentityService, _sessionManager, _intentRouter,
_actionHandlers, _conversationEngine, _templateRenderer, _userProfileService,
_departmentsService, _departmentSettingsService, _limitsService,
_authorizationService, _departmentConfigService, _rateLimiter), which violates
the repo convention and indicates low cohesion; refactor the constructor to only
accept the minimal, truly required collaborators (e.g., keep critical ones like
IChatbotUserIdentityService or IChatbotSessionManager if necessary) and resolve
the rest inside the class via the service locator
(Bootstrapper.GetKernel().Resolve<T>()), or split responsibilities into separate
services (e.g., an Auth/Identity helper and a Dialog/Orchestration helper) so
ChatbotIngressService no longer requires all 13 injected dependencies—update
field initialization to use Resolve<T>() for the moved dependencies and adjust
usages to the new helpers or resolved instances.
In `@Core/Resgrid.Chatbot/Services/ChatbotTemplateRenderer.cs`:
- Around line 197-258: Convert all public fields in the template model classes
to public auto-properties: replace field declarations in CallsListModel
(DepartmentName, Calls and nested CallEntry fields Number, Name, Nature,
Address), CallDetailModel (Number, Name, Nature, Address, Priority, State),
UnitsListModel (DepartmentName, Units and nested UnitEntry fields Name, Status),
StatusResponseModel (StatusName, StaffingLevel), MessagesListModel (UnreadCount,
Messages and nested MessageEntry fields Id, Subject, From, Date),
CalendarListModel (DepartmentName, Events and nested EventEntry fields Start,
Title, Type), ShiftsListModel (DepartmentName, Shifts and nested ShiftEntry
fields Start, End, Name, Station), PersonnelListModel (DepartmentName, Personnel
and nested PersonnelEntry fields Name, Status, Staffing, Destination), HelpModel
(HelpText), and ErrorModel (Message) into auto-properties (e.g., public string
DepartmentName { get; set; }) to follow modern C# conventions and enable future
extensibility.
In `@Core/Resgrid.Chatbot/Services/ChatbotUserSearchService.cs`:
- Around line 67-80: MatchesQuery and the produced ChatbotUserMatch.FullName
rely on UserGroupRole.Name which is just FirstName + " " + LastName and can be
missing/misformatted, causing multi-token queries (e.g., "john smith") to fail;
update MatchesQuery to build/compare against an explicit combined full name from
u.FirstName and u.LastName (and/or split the incoming lowerQuery into tokens and
ensure each token matches FirstName, LastName, or the constructed full name) and
update ToMatch to set FullName from the same constructed string (not u.Name) so
ResolveSingleAsync exact-match logic uses a reliable FullName source; make
changes in MatchesQuery, ToMatch, and any code paths in ResolveSingleAsync that
compare against m.FullName.
In `@Core/Resgrid.Chatbot/Services/ConversationEngine.cs`:
- Around line 46-106: HandleContinuationAsync duplicates the
AwaitingParameter/AwaitingConfirmation/AwaitingAuth state handling already
implemented in
HandleParameterResponse/HandleConfirmationResponse/HandleAuthResponse causing
drift; refactor HandleContinuationAsync to delegate to those existing methods
(passing ChatbotMessage and ChatbotSession) and map their ConversationResult
into a ChatbotResponse (or update the called methods to return/produce a
ChatbotResponse via a small adapter), ensuring you preserve session updates on
ChatbotSession and reference symbols: HandleContinuationAsync,
HandleParameterResponse, HandleConfirmationResponse, HandleAuthResponse,
ConversationResult, ChatbotResponse, ChatbotSession, ChatbotMessage; remove the
inline switch logic so there is a single source of truth for each state
transition.
In `@Core/Resgrid.Chatbot/Services/OAuthLinkingService.cs`:
- Around line 251-259: PostFormAsync disposes the HttpResponseMessage (response)
before the async read completes which can be fragile; to fix, read the response
content into a string (await response.Content.ReadAsStringAsync()) while the
response is still in scope, then dispose response and parse the JSON from the
buffered string; modify PostFormAsync to capture the content string before
returning JsonDocument.Parse, keeping usage of FormUrlEncodedContent and
_http.PostAsync but ensuring response is not disposed prior to completing the
content read.
In `@Providers/Resgrid.Providers.Chatbot/Adapters/DiscordBotAdapter.cs`:
- Around line 108-112: The IChatbotPlatformAdapterConfig interface is
platform-agnostic and should be extracted from DiscordBotAdapter.cs into its own
file named IChatbotPlatformAdapterConfig.cs inside the Interfaces folder; create
the new file with the same public interface declaration (string
GetToken(ChatbotPlatform platform); string GetBotId(ChatbotPlatform platform);),
update namespaces/usings in the new file to match the other adapter contracts,
remove the interface declaration from DiscordBotAdapter.cs, and update any files
(including DiscordBotAdapter and any consumers) to reference the moved
interface's namespace; rebuild to ensure no missing references and run tests.
In `@Providers/Resgrid.Providers.Chatbot/Services/ChatbotOutboundService.cs`:
- Around line 25-33: The constructor for ChatbotOutboundService currently uses
constructor injection for IChatbotUserIdentityService, IChatbotAdapterRegistry,
and IChatbotDepartmentConfigService; change it to use the Service Locator
pattern by removing those parameters and resolving each dependency inside the
constructor via Bootstrapper.GetKernel().Resolve<T>() (resolve
IChatbotUserIdentityService into _identityService, IChatbotAdapterRegistry into
_adapterRegistry, and IChatbotDepartmentConfigService into
_departmentConfigService) so the class adheres to the guideline of using
Bootstrapper.GetKernel().Resolve<T>() for dependency resolution.
In `@Providers/Resgrid.Providers.Migrations/Migrations/M0068_ChatbotTables.cs`:
- Around line 27-30: The non-unique index
IX_ChatbotUserIdentities_Platform_PlatformUserId on ChatbotUserIdentities allows
duplicate (Platform, PlatformUserId) rows and makes GetIdentityAsync ambiguous;
change the migration to create a unique filtered index on (Platform,
PlatformUserId) that only applies to active records (e.g., WHERE IsActive = 1)
so uniqueness is enforced for active identities while allowing soft-deleted rows
to coexist.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0068_ChatbotTablesPg.cs`:
- Around line 55-76: The ChatbotMessageLog table stores raw MessageText
indefinitely (privacy risk); add a retention mechanism by modifying the
migration that creates ChatbotMessageLog to include a nullable ExpireAt (or
RetentionUntil) DateTime column and an index on it, and implement a scheduled
cleanup job (or DB-side policy) that deletes rows where COALESCE(ExpireAt,
Timestamp) is older than your retention window; update the Create.Table call for
"ChatbotMessageLog" and reference the MessageText and Timestamp columns in the
cleanup logic so old user-entered content is purged automatically.
In `@Tests/Resgrid.Tests/Chatbot/ChatbotDeptConfigAndSessionTests.cs`:
- Around line 23-30: The CacheMock method mutates the global
Resgrid.Config.SystemBehaviorConfig.CacheEnabled flag and never restores it, so
update the ChatbotDeptConfigAndSessionTests fixture to preserve and restore that
flag: capture the original value in a test fixture setup (e.g., OneTimeSetUp or
SetUp) into a private field, leave CacheMock to set CacheEnabled = false if
necessary, and then restore the saved value in the matching teardown
(OneTimeTearDown or TearDown); alternatively change CacheMock to avoid touching
SystemBehaviorConfig at all and instead mock configuration behavior
locally—reference CacheMock and the ChatbotDeptConfigAndSessionTests class when
making the change.
In `@Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs`:
- Around line 28-49: The Index GET currently calls
_chatbotConfigService.GetConfigAsync(DepartmentId) without error handling which
can surface unhandled exceptions; wrap the await and model mapping inside a
try/catch in the Index() action, catch Exception (ex), log the exception using
the existing logger (e.g. _logger.LogError(ex, "...")) and then populate a safe
default ChatbotSettingsModel (same fields as now with null/false defaults) and
return View(model) while adding a user-visible error (ModelState.AddModelError
or TempData) so the page renders instead of throwing; reference Index(),
_chatbotConfigService.GetConfigAsync, ChatbotSettingsModel and DepartmentId when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+19
to
+24
| public CallDetailActionHandler(ICallsService callsService, IDepartmentsService departmentsService, IAuthorizationService authorizationService) | ||
| { | ||
| _callsService = callsService; | ||
| _departmentsService = departmentsService; | ||
| _authorizationService = authorizationService; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Service Locator resolution instead of constructor injection here.
Lines 19-24 currently use constructor injection, which conflicts with the repository convention for dependency resolution in this codebase.
♻️ Suggested change
- public CallDetailActionHandler(ICallsService callsService, IDepartmentsService departmentsService, IAuthorizationService authorizationService)
- {
- _callsService = callsService;
- _departmentsService = departmentsService;
- _authorizationService = authorizationService;
- }
+ public CallDetailActionHandler()
+ {
+ _callsService = Bootstrapper.GetKernel().Resolve<ICallsService>();
+ _departmentsService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>();
+ _authorizationService = Bootstrapper.GetKernel().Resolve<IAuthorizationService>();
+ }As per coding guidelines "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public CallDetailActionHandler(ICallsService callsService, IDepartmentsService departmentsService, IAuthorizationService authorizationService) | |
| { | |
| _callsService = callsService; | |
| _departmentsService = departmentsService; | |
| _authorizationService = authorizationService; | |
| } | |
| public CallDetailActionHandler() | |
| { | |
| _callsService = Bootstrapper.GetKernel().Resolve<ICallsService>(); | |
| _departmentsService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>(); | |
| _authorizationService = Bootstrapper.GetKernel().Resolve<IAuthorizationService>(); | |
| } |
🤖 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.Chatbot/Handlers/CallDetailActionHandler.cs` around lines 19 -
24, Replace constructor dependency injection in CallDetailActionHandler: remove
the injected parameters from the CallDetailActionHandler constructor and instead
resolve the dependencies inside the constructor using the Service Locator
(Bootstrapper.GetKernel().Resolve<T>()), assigning the resolved instances to
_callsService, _departmentsService, and _authorizationService so the class uses
service-locator resolution rather than constructor injection.
Comment on lines
+19
to
+22
| public MessageDeleteHandler(IMessageService messageService) | ||
| { | ||
| _messageService = messageService; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the mandated constructor resolution pattern.
Lines 19-22 use constructor injection, which conflicts with the required Service Locator constructor pattern in this repository.
As per coding guidelines "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, 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 `@Core/Resgrid.Chatbot/Handlers/MessageDeleteHandler.cs` around lines 19 - 22,
The MessageDeleteHandler constructor currently takes IMessageService via
constructor injection; change it to use the repository's Service Locator pattern
by replacing the parameterized ctor with a parameterless constructor that
assigns _messageService = Bootstrapper.GetKernel().Resolve<IMessageService>();
(keep the _messageService field name), and update any callsites or tests that
instantiate MessageDeleteHandler to use the parameterless ctor. Ensure the class
name MessageDeleteHandler and the field _messageService are used exactly as in
the diff.
Comment on lines
+19
to
+31
| public MyStatusActionHandler( | ||
| IActionLogsService actionLogsService, | ||
| IUserStateService userStateService, | ||
| ICustomStateService customStateService, | ||
| IUserProfileService userProfileService, | ||
| IDepartmentsService departmentsService) | ||
| { | ||
| _actionLogsService = actionLogsService; | ||
| _userStateService = userStateService; | ||
| _customStateService = customStateService; | ||
| _userProfileService = userProfileService; | ||
| _departmentsService = departmentsService; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Switch constructor dependency resolution to Service Locator.
Lines 19-31 use constructor injection, but this layer’s convention requires resolving dependencies via Bootstrapper.GetKernel().Resolve<T>() in constructors.
As per coding guidelines "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, 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 `@Core/Resgrid.Chatbot/Handlers/MyStatusActionHandler.cs` around lines 19 - 31,
The constructor of MyStatusActionHandler currently uses constructor injection
for IActionLogsService, IUserStateService, ICustomStateService,
IUserProfileService, and IDepartmentsService; change it to use the Service
Locator pattern by calling Bootstrapper.GetKernel().Resolve<T>() inside the
MyStatusActionHandler constructor to obtain each dependency and assign them to
the corresponding fields (_actionLogsService, _userStateService,
_customStateService, _userProfileService, _departmentsService) instead of
accepting them as parameters, and remove those parameters from the ctor
signature so resolution is performed explicitly within the constructor.
| // Per-department LLM/AI override + rate limits + linking/notification preferences. | ||
| Alter.Table("ChatbotDepartmentConfigs") | ||
| .AddColumn("LlmApiEndpoint").AsString(500).Nullable() | ||
| .AddColumn("LlmApiKey").AsString(1000).Nullable() |
Contributor
There was a problem hiding this comment.
Confirm LlmApiKey is encrypted at rest rather than stored as plaintext.
This column persists a third-party LLM API secret. Storing credentials in plaintext is a security posture gap; if the rest of the codebase encrypts secret columns (or stores them in a secrets vault), this should follow the same pattern, and the application layer should encrypt/decrypt around it.
Verify how other secret/key columns are persisted in existing migrations and whether an encryption helper is applied:
#!/bin/bash
# Look for existing secret/api-key/token columns in migrations to see if encryption is applied
rg -nP --type=cs -C2 '\.AddColumn\("[^"]*(Key|Secret|Token|Password|ApiKey)[^"]*"\)' -g '**/Migrations/**'
# Look for any encryption/decryption helpers used around stored secrets
rg -nP --type=cs -C2 '\b(Encrypt|Decrypt|Protect|Unprotect|DataProtector)\b' -g '!**/Migrations/**' | head -50🤖 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
`@Providers/Resgrid.Providers.Migrations/Migrations/M0070_ChatbotDepartmentConfigColumns.cs`
at line 13, The migration adds the LlmApiKey column as plaintext; change the
persistence to follow existing secret handling by storing an encrypted value
instead: update M0070_ChatbotDepartmentConfigColumns (the AddColumn("LlmApiKey")
definition) to match the pattern used by other secret columns (e.g., same column
name suffix/metadata or use the same encryption/storage helper), and ensure the
application layer (where ChatbotDepartmentConfig is written/read) uses the same
Encrypt/Decrypt or DataProtector helper used elsewhere to encrypt before saving
and decrypt after reading; review other migrations and the encryption helper you
find and apply the identical approach for LlmApiKey so secrets are encrypted at
rest.
Comment on lines
+91
to
+99
| var chatbotMessage = await telegramAdapter.ParseInboundMessageAsync(parameters); | ||
|
|
||
| // Process through the chatbot pipeline | ||
| var response = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage); | ||
|
|
||
| // Format and send response via Telegram adapter | ||
| var formattedResponse = await telegramAdapter.FormatOutboundResponseAsync(response); | ||
|
|
||
| return Ok("ok"); |
Contributor
There was a problem hiding this comment.
Formatted response is never sent to Telegram.
Line 97 generates formattedResponse but it's never delivered. The Telegram Bot API requires an explicit HTTP POST to send messages; simply formatting the response and returning Ok("ok") to the webhook doesn't send anything to the user.
🔧 Proposed fix to send the response
After line 97, add a send operation. The exact implementation depends on whether telegramAdapter has a send method or if you need to call the Telegram API directly:
// Format and send response via Telegram adapter
var formattedResponse = await telegramAdapter.FormatOutboundResponseAsync(response);
+
+// Send the response back to Telegram
+await telegramAdapter.SendMessageAsync(formattedResponse);
return Ok("ok");Alternatively, if the adapter doesn't have a send method, verify whether FormatOutboundResponseAsync is intended to both format AND send, and update its implementation accordingly.
🤖 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/ChatbotTelegramController.cs` around
lines 91 - 99, The formattedResponse produced by
telegramAdapter.FormatOutboundResponseAsync(...) is never sent; after obtaining
formattedResponse, call the adapter's send method (e.g.,
telegramAdapter.SendOutboundAsync or SendMessageAsync) or perform the HTTP POST
to Telegram using the adapter's HTTP client with the formatted payload, and
await that call before returning Ok; if the adapter lacks a send method, update
FormatOutboundResponseAsync (or add a new SendOutboundAsync on telegramAdapter)
to both format and transmit the message so that the outgoing message created in
FormatOutboundResponseAsync is actually delivered to Telegram instead of only
returning Ok("ok").
Comment on lines
+8284
to
8287
| <member name="T:Resgrid.Web.Services.Models.v4.Protocols.GetAllProtocolsResult"> | ||
| <summary> | ||
| The result of getting all personnel filters for the system | ||
| Result containing all the data required to populate the New Call form | ||
| </summary> |
Contributor
There was a problem hiding this comment.
Protocol result summaries appear copy-pasted from unrelated endpoint
Both summaries say “populate the New Call form,” which is misleading for protocol result wrappers. Update the text to describe protocol retrieval responses.
Also applies to: 8379-8382
🤖 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/Resgrid.Web.Services.xml` around lines 8284 - 8287,
Update the XML summary for the protocol result wrappers so they describe
protocol retrieval responses instead of “populate the New Call form”: change the
<summary> for T:Resgrid.Web.Services.Models.v4.Protocols.GetAllProtocolsResult
to something like “Contains the list of protocols returned by the API” (or
similar accurate wording), and make the analogous change to the single-protocol
result wrapper in the same Resgrid.Web.Services.Models.v4.Protocols namespace so
both documentation entries reflect protocol retrieval rather than New Call form
population.
Comment on lines
8320
to
8322
| <summary> | ||
| Response Data | ||
| This this protocol disabled | ||
| </summary> |
Contributor
There was a problem hiding this comment.
Fix typo in protocol description
The summary says “This this protocol disabled”. Please change to “Is this protocol disabled” (or equivalent).
🤖 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/Resgrid.Web.Services.xml` around lines 8320 - 8322,
Update the incorrect summary text in the XML comment where the <summary> element
currently reads "This this protocol disabled" and change it to a grammatically
correct form such as "Is this protocol disabled" (preserve surrounding XML
structure and capitalization); locate the <summary> element in
Resgrid.Web.Services.xml associated with the protocol description and replace
the text only, leaving other tags unchanged.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Core/Resgrid.Chatbot/Services/ConversationEngine.cs (1)
207-243:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
HandleConfirmationResponsestill hardcodes English strings and English-only YES/NO parsing.Localization was applied to
HandleContinuationAsyncbut theProcessAsyncconfirmation path was left unchanged:"Confirmed.","Cancelled.","Please reply YES to confirm or NO to cancel."and the literalYES/Y/CONFIRM/OK/NO/N/CANCELchecks bypasssession.Culture. Non-English users get untranslated responses and cannot confirm/cancel. Route these throughChatbotResources.Get(...)andChatbotResources.IsAffirmative/IsNegativefor parity with the continuation path.🌐 Proposed alignment with the localized continuation path
private Task<ConversationResult> HandleConfirmationResponse(ChatbotMessage message, ChatbotSession session, ChatbotIntent intent) { - var response = message.Text?.Trim().ToUpperInvariant(); - - if (response == "YES" || response == "Y" || response == "CONFIRM" || response == "OK") + if (ChatbotResources.IsAffirmative(message.Text, session.Culture)) { session.State = ChatbotDialogState.Completed; return Task.FromResult(new ConversationResult { - Response = new ChatbotResponse { Text = "Confirmed.", Processed = true }, + Response = new ChatbotResponse { Text = ChatbotResources.Get("Conv_Confirmed", session.Culture), Processed = true }, IsComplete = true, NeedsFollowUp = false, NextState = ChatbotDialogState.Idle }); } - if (response == "NO" || response == "N" || response == "CANCEL") + if (ChatbotResources.IsNegative(message.Text, session.Culture)) { session.Reset(); return Task.FromResult(new ConversationResult { - Response = new ChatbotResponse { Text = "Cancelled.", Processed = true }, + Response = new ChatbotResponse { Text = ChatbotResources.Get("Conv_Cancelled", session.Culture), Processed = true }, IsComplete = true, NeedsFollowUp = false, NextState = ChatbotDialogState.Idle }); } // Unrecognized - ask again return Task.FromResult(new ConversationResult { - Response = new ChatbotResponse { Text = "Please reply YES to confirm or NO to cancel.", Processed = true }, + Response = new ChatbotResponse { Text = ChatbotResources.Get("Conv_ConfirmPrompt", session.Culture), Processed = true }, IsComplete = false, NeedsFollowUp = true, NextState = ChatbotDialogState.AwaitingConfirmation }); }🤖 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.Chatbot/Services/ConversationEngine.cs` around lines 207 - 243, HandleConfirmationResponse currently hardcodes English prompts and parses only English YES/NO; update it to use session.Culture-aware helpers: replace literal checks for "YES"/"NO"/"Y"/etc. with ChatbotResources.IsAffirmative(message.Text, session.Culture) and ChatbotResources.IsNegative(message.Text, session.Culture), and replace hardcoded response texts ("Confirmed.", "Cancelled.", "Please reply YES...") with ChatbotResources.Get(key, session.Culture) (use appropriate resource keys consistent with other methods). Preserve existing behavior for setting session.State, calling session.Reset(), and returning ConversationResult/ChatbotResponse/ChatbotDialogState values (Completed/Idle/AwaitingConfirmation) but ensure all user-facing strings and parsing honor session.Culture.
🤖 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.
Duplicate comments:
In `@Core/Resgrid.Chatbot/Services/ConversationEngine.cs`:
- Around line 207-243: HandleConfirmationResponse currently hardcodes English
prompts and parses only English YES/NO; update it to use session.Culture-aware
helpers: replace literal checks for "YES"/"NO"/"Y"/etc. with
ChatbotResources.IsAffirmative(message.Text, session.Culture) and
ChatbotResources.IsNegative(message.Text, session.Culture), and replace
hardcoded response texts ("Confirmed.", "Cancelled.", "Please reply YES...")
with ChatbotResources.Get(key, session.Culture) (use appropriate resource keys
consistent with other methods). Preserve existing behavior for setting
session.State, calling session.Reset(), and returning
ConversationResult/ChatbotResponse/ChatbotDialogState values
(Completed/Idle/AwaitingConfirmation) but ensure all user-facing strings and
parsing honor session.Culture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3e004ae-f874-4bb9-9f40-d6f38c69daa3
📒 Files selected for processing (8)
Core/Resgrid.Chatbot.NLU/Services/EntityExtractor.csCore/Resgrid.Chatbot/Handlers/DispatchCallHandler.csCore/Resgrid.Chatbot/Localization/ChatbotResources.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Chatbot/Services/ChatbotUserIdentityService.csCore/Resgrid.Chatbot/Services/ConversationEngine.csCore/Resgrid.Model/CallSources.csProviders/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Core/Resgrid.Chatbot/Localization/ChatbotResources.cs
- Core/Resgrid.Chatbot/Services/ChatbotUserIdentityService.cs
- Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs
- Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Configuration