Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
This comment has been minimized.
This comment has been minimized.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (14)
📒 Files selected for processing (35)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThis PR expands chatbot intent recognition and handlers, adds multi-turn poll and calendar RSVP responses, supports per-session TTLs, filters selected call broadcast recipients, and updates document and certification upload workflows with authorization, sanitization, and safer file handling. ChangesChatbot response expansion
Poll and calendar RSVP responses
Document and broadcast updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Workers/Resgrid.Workers.Framework/Logic/CalendarNotifierLogic.cs (1)
98-136: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap the entity notification path in the worker’s try-catch.
The added helper calls can throw from communication or data access, but this entire branch remains outside the attendee branch’s error handling, allowing
Process()to escape without a failure tuple.As per coding guidelines, worker
Process()methods must wrap processing in try-catch, log viaLogging.LogException(), and return a failure tuple on error.🤖 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 `@Workers/Resgrid.Workers.Framework/Logic/CalendarNotifierLogic.cs` around lines 98 - 136, The entity notification branch in Process(), including SendCalendarReminderAsync and GetAllGroupsForDepartmentAsync calls, must be enclosed in the worker’s try-catch. On any exception, log it through Logging.LogException() and return the established failure tuple instead of allowing Process() to escape; preserve the existing notification and MarkAsNotifiedAsync flow on success.Source: Coding guidelines
🟡 Minor comments (5)
Core/Resgrid.Chatbot/Services/ConversationEngine.cs-292-305 (1)
292-305: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize the custom-state prompt headings.
These branches bypass
ChatbotResources, so every non-English session receives an English heading whenever custom states are configured. Add resource keys and pass the culture-specific heading intoBuildOptionsPrompt.🤖 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 292 - 305, Localize the headings in the SetStatus and SetStaffing branches of the conversation flow by adding corresponding ChatbotResources keys for each prompt and passing the culture-specific values to BuildOptionsPrompt. Preserve the existing custom-state lookup and option handling, and use the session/request culture already established by the surrounding code.Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs-47-47 (1)
47-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the new English response fragments into localized resources.
Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs#L47-L47: localize the disabled-dispatch response.Core/Resgrid.Chatbot/Handlers/RespondToCallHandler.cs#L77-L80: localize the entire negative call-response format.Core/Resgrid.Chatbot/Handlers/StatusActionHandler.cs#L65-L65: localize the “your status” confirmation label.🤖 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/DispatchCallHandler.cs` at line 47, Move the hard-coded English chatbot responses into the existing localized resource system: replace the disabled-dispatch text in DispatchCallHandler, the complete negative call-response format in RespondToCallHandler, and the “your status” confirmation label in StatusActionHandler with resource lookups, adding the corresponding resource entries and preserving each response’s current formatting and behavior.Core/Resgrid.Chatbot/Handlers/HelpActionHandler.cs-139-139 (1)
139-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMention calendar RSVP prompts here too.
Bare
YESorNOcan also resolve a pending calendar RSVP, not only the latest poll.Proposed wording
- + "YES or NO: answers your latest poll\n" + + "YES or NO: answers your latest poll or RSVP prompt\n"🤖 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/HelpActionHandler.cs` at line 139, Update the help text in HelpActionHandler to mention that a bare YES or NO also answers a pending calendar RSVP, alongside the existing latest-poll behavior.Core/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.cs-92-114 (1)
92-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIndicate truncated group, role, and unit results.
These sections silently hide entries beyond
MaxLinesPerSection, unlike personnel. AppendMsg_AndMorewith the omitted count to avoid presenting an incomplete dispatch list as complete.🤖 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/CallDispatchedActionHandler.cs` around lines 92 - 114, Update the group, role, and unit sections in CallDispatchedActionHandler so each calculates omitted entries beyond MaxLinesPerSection and appends ChatbotResources.Get("Msg_AndMore", culture) with that count, matching the existing personnel truncation behavior. Preserve the current entries and section output when no results are omitted.Core/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.cs-43-60 (1)
43-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat filler-only call references as unspecified.
Inputs such as “the call” are sent to
CallReferenceResolver, producingCall_NoMatchinstead of selecting the sole active call. Normalizecall,the call, and similar filler values before resolution, as the responder handler does.🤖 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/CallDispatchedActionHandler.cs` around lines 43 - 60, Normalize filler-only values such as “call” and “the call” to an unspecified reference before the reference check in the call-selection flow. Reuse the responder handler’s existing normalization behavior or helper, then let empty results follow the sole-active-call fallback; keep valid references routed through CallReferenceResolver.ResolveAsync.
🧹 Nitpick comments (2)
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs (1)
52-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s Service Locator pattern for the newly added service dependencies.
Both changes add constructor injection despite the repository-specific dependency-resolution rule.
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs#L52-L69: resolveITextResponseResolverthroughBootstrapper.GetKernel().Resolve<ITextResponseResolver>().Core/Resgrid.Chatbot/Services/ConversationEngine.cs#L16-L20: resolveICustomStateServicethroughBootstrapper.GetKernel().Resolve<ICustomStateService>().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/Services/ChatbotIngressService.cs` around lines 52 - 69, Replace constructor injection for ITextResponseResolver in ChatbotIngressService with Bootstrapper.GetKernel().Resolve<ITextResponseResolver>(), removing the corresponding constructor parameter and assignment. Apply the same change in Core/Resgrid.Chatbot/Services/ConversationEngine.cs at lines 16-20 for ICustomStateService: resolve it through Bootstrapper.GetKernel().Resolve<ICustomStateService>() instead of injecting it.Source: Coding guidelines
Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs (1)
61-77: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndex action logs and user states before iterating.
Lines 69-70 scan both department-wide collections for every user, making this path quadratic. Build dictionaries keyed by
UserIdonce before the loop.🤖 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/AvailabilityActionHandler.cs` around lines 61 - 77, Update the availability flow before the user iteration to build dictionaries keyed by UserId from lastActionLogs and userStates, then replace the per-user FirstOrDefault lookups in the foreach over allUsers with dictionary lookups while preserving null and missing-entry behavior.
🤖 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/Providers/KeywordIntentClassifier.cs`:
- Around line 202-210: Update the positive and negative responder patterns in
the keyword classifier to pass extracted call references through CleanReference
before constructing the callRef parameter. Apply this consistently to the
callRef extraction callbacks in the relevant respond_to_call rules, while
leaving callId handling unchanged.
In `@Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs`:
- Around line 24-45: Replace constructor injection with
Bootstrapper.GetKernel().Resolve<T>() assignments for all dependencies in
Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs lines 24-45,
including all six services; resolve the three handler services explicitly in
Core/Resgrid.Chatbot/Handlers/UnitsAvailableActionHandler.cs lines 22-34;
resolve calls and units services explicitly in
Core/Resgrid.Chatbot/Handlers/MyCallsActionHandler.cs lines 26-33; resolve
message, department, and profile services explicitly in
Core/Resgrid.Chatbot/Handlers/PollCreateHandler.cs lines 28-40; and resolve
message, calendar, and authorization services explicitly in
Core/Resgrid.Chatbot/Services/TextResponseResolver.cs lines 19-29. Remove the
corresponding constructor parameters while preserving the existing private
dependency fields.
In `@Core/Resgrid.Chatbot/Handlers/CallRespondersActionHandler.cs`:
- Around line 71-76: Update the action-log selection in
CallRespondersActionHandler to fetch each resource’s current department state
before filtering responders. Compare those current states against the
call-scoped records and retain only records whose destination is this call,
rather than selecting the latest entry solely from GetActionLogsForCallAsync;
apply the same logic to the corresponding block around the later occurrence.
- Around line 29-50: Replace constructor injection with explicit
Bootstrapper.GetKernel().Resolve<T>() resolution for all six services in
Core/Resgrid.Chatbot/Handlers/CallRespondersActionHandler.cs#L29-L50, all three
services in
Core/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.cs#L22-L34, and all
three services in
Core/Resgrid.Chatbot/Handlers/MyScheduleActionHandler.cs#L24-L36. In
Core/Resgrid.Services/CalendarService.cs#L28-L46, resolve
ITextResponsePromptService explicitly instead of using an optional constructor
parameter; in Core/Resgrid.Services/TextResponsePromptService.cs#L14-L19,
resolve IMessageService through the kernel. Preserve each constructor’s existing
initialization behavior while applying the required Service Locator pattern.
In `@Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs`:
- Around line 26-31: Replace optional constructor service parameters with
explicit Bootstrapper.GetKernel().Resolve<T>() resolution: in
Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs lines 26-31, always resolve
IChatbotDepartmentConfigService; in
Core/Resgrid.Chatbot/Handlers/RespondToCallHandler.cs lines 29-37, always
resolve the custom-state, group, and role services; and in
Core/Resgrid.Chatbot/Handlers/StatusActionHandler.cs lines 18-23, always resolve
the department configuration service. Update each constructor’s field assignment
while preserving the existing handler behavior.
In `@Core/Resgrid.Chatbot/Handlers/MyCallsActionHandler.cs`:
- Around line 116-135: The GetActiveCallsWithDispatchesAsync method must not
truncate active calls with Take(MaxActiveCallsToScan), since older dispatched
calls can be missed. Replace the limited retrieval with a dispatch-filtered
service query, or process all active calls using bounded concurrency, while
preserving the existing forUnits-specific PopulateCallData options and result
behavior.
In `@Core/Resgrid.Chatbot/Handlers/MyScheduleActionHandler.cs`:
- Line 53: Update MyScheduleActionHandler date parsing and formatting to
consistently use the session culture rather than CultureInfo.InvariantCulture or
process culture. Apply this to the dateLabel formatting and the additional
date-related locations at the indicated symbols, ensuring localized weekday
names and date input are accepted while preserving the existing response
behavior.
- Around line 84-99: The calendar retrieval in the schedule handler currently
excludes multi-day events because it requires both endpoints within the window.
Update GetAllCalendarItemsForDepartmentInRangeAsync or the underlying query it
uses to apply overlap semantics, selecting items where Start is before windowEnd
and End is on or after windowStart, while preserving the existing
windowStartUtc/windowEndUtc inputs and attendee/date filtering.
In `@Core/Resgrid.Chatbot/Handlers/PollCreateHandler.cs`:
- Around line 83-99: Carry the department boundary through the poll-response
flow: in Core/Resgrid.Chatbot/Handlers/PollCreateHandler.cs lines 83-99, persist
session.DepartmentId in machine-readable recipient metadata; in
Core/Resgrid.Chatbot/Services/TextResponseResolver.cs lines 31-63, filter
discovered poll recipients to matching departmentId metadata; and in lines
89-109, revalidate that metadata before saving the response.
In `@Core/Resgrid.Chatbot/Handlers/RespondToCallHandler.cs`:
- Around line 121-149: Update the dispatch timestamp aggregation in the handler
so group and role dispatches are evaluated even when a direct dispatch exists;
remove the dispatch-null gating around the group and role checks while
preserving their existing membership and role filtering, then take the maximum
applicable timestamp across direct, group, and role dispatches.
In `@Core/Resgrid.Chatbot/Handlers/StaffingActionHandler.cs`:
- Around line 116-118: Remove the positional fallback in the staffing-level
resolution logic that indexes the active custom levels with staffingId. Treat
staffingId as a UserStateTypes value only, and return no match when the existing
ID/name resolution does not succeed, preserving the active-level filtering for
valid resolution paths.
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 254-255: Update the session flow around GetOrCreateSessionAsync so
the effective department TTL is applied before the session expiry check and
LastActivity refresh. Pass the department-configured TTL into retrieval, or set
it before invoking GetOrCreateSessionAsync, while preserving the existing
session state behavior for valid sessions.
In `@Core/Resgrid.Chatbot/Services/RedisSessionStore.cs`:
- Around line 35-38: Update PruneExpiredAsync in RedisSessionStore to use
ChatbotSession.IsExpired() for each entry when no explicit cutoff is supplied,
instead of applying the global default cutoff. Preserve the explicit-cutoff
behavior when a cutoff is provided, and retain GetTtl for Redis expiration.
In `@Core/Resgrid.Services/CalendarService.cs`:
- Around line 585-598: Use the boolean result from SendCalendarAsync in
CalendarService.SendCalendarPromptAsync and record the RSVP prompt only when
delivery is accepted. Apply the same guard to SendNotificationAsync in
CalendarNotifierLogic, and retain its send result in NotificationBroadcastLogic
before skipping prompt recording when it is false; update all three specified
sites accordingly.
In `@Core/Resgrid.Services/CommunicationService.cs`:
- Around line 580-590: Update the SMS send guards in the notification and
calendar paths around _smsService.SendNotificationAsync to require
MobileNumberVerified.IsContactMethodAllowedForSending() in addition to the
existing profile check. Apply the same verification condition to both affected
paths, preserving the current send and exception-logging behavior for allowed
contacts.
In `@Core/Resgrid.Services/TextResponsePromptService.cs`:
- Around line 29-50: Update the prompt creation flow in
TextResponsePromptService around SaveMessageAsync to reuse or upsert an existing
unexpired CalendarRsvp prompt keyed by calendarItem.CalendarItemId and userId,
using the same metadata. Ensure repeated invitations, updates, reminders, and
worker executions do not create duplicate active prompts, while still creating a
prompt when no matching unexpired record exists.
In `@Workers/Resgrid.Workers.Framework/Logic/NotificationBroadcastLogic.cs`:
- Around line 63-74: Wrap the RSVP enrichment call in the calendar-event branch
of the notification broadcast flow with exception handling so failures from
GetCalendarItemByIdAsync do not abort delivery of the original notification.
Catch the exception, log it through Logging.LogException(), and continue
processing without RSVP text or item enrichment.
---
Outside diff comments:
In `@Workers/Resgrid.Workers.Framework/Logic/CalendarNotifierLogic.cs`:
- Around line 98-136: The entity notification branch in Process(), including
SendCalendarReminderAsync and GetAllGroupsForDepartmentAsync calls, must be
enclosed in the worker’s try-catch. On any exception, log it through
Logging.LogException() and return the established failure tuple instead of
allowing Process() to escape; preserve the existing notification and
MarkAsNotifiedAsync flow on success.
---
Minor comments:
In `@Core/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.cs`:
- Around line 92-114: Update the group, role, and unit sections in
CallDispatchedActionHandler so each calculates omitted entries beyond
MaxLinesPerSection and appends ChatbotResources.Get("Msg_AndMore", culture) with
that count, matching the existing personnel truncation behavior. Preserve the
current entries and section output when no results are omitted.
- Around line 43-60: Normalize filler-only values such as “call” and “the call”
to an unspecified reference before the reference check in the call-selection
flow. Reuse the responder handler’s existing normalization behavior or helper,
then let empty results follow the sole-active-call fallback; keep valid
references routed through CallReferenceResolver.ResolveAsync.
In `@Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs`:
- Line 47: Move the hard-coded English chatbot responses into the existing
localized resource system: replace the disabled-dispatch text in
DispatchCallHandler, the complete negative call-response format in
RespondToCallHandler, and the “your status” confirmation label in
StatusActionHandler with resource lookups, adding the corresponding resource
entries and preserving each response’s current formatting and behavior.
In `@Core/Resgrid.Chatbot/Handlers/HelpActionHandler.cs`:
- Line 139: Update the help text in HelpActionHandler to mention that a bare YES
or NO also answers a pending calendar RSVP, alongside the existing latest-poll
behavior.
In `@Core/Resgrid.Chatbot/Services/ConversationEngine.cs`:
- Around line 292-305: Localize the headings in the SetStatus and SetStaffing
branches of the conversation flow by adding corresponding ChatbotResources keys
for each prompt and passing the culture-specific values to BuildOptionsPrompt.
Preserve the existing custom-state lookup and option handling, and use the
session/request culture already established by the surrounding code.
---
Nitpick comments:
In `@Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs`:
- Around line 61-77: Update the availability flow before the user iteration to
build dictionaries keyed by UserId from lastActionLogs and userStates, then
replace the per-user FirstOrDefault lookups in the foreach over allUsers with
dictionary lookups while preserving null and missing-entry behavior.
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 52-69: Replace constructor injection for ITextResponseResolver in
ChatbotIngressService with
Bootstrapper.GetKernel().Resolve<ITextResponseResolver>(), removing the
corresponding constructor parameter and assignment. Apply the same change in
Core/Resgrid.Chatbot/Services/ConversationEngine.cs at lines 16-20 for
ICustomStateService: resolve it through
Bootstrapper.GetKernel().Resolve<ICustomStateService>() instead of injecting it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b30cace-b05b-4015-9fbb-4661d4b3c1db
⛔ Files ignored due to path filters (4)
Tests/Resgrid.Tests/Chatbot/ChatbotAvailabilityIntentClassifierTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Chatbot/ChatbotHandlerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Chatbot/ChatbotTextResponseResolverTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/CommunicationServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (39)
Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.csCore/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.csCore/Resgrid.Chatbot/ChatbotModule.csCore/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.csCore/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.csCore/Resgrid.Chatbot/Handlers/CallRespondersActionHandler.csCore/Resgrid.Chatbot/Handlers/DispatchCallHandler.csCore/Resgrid.Chatbot/Handlers/HelpActionHandler.csCore/Resgrid.Chatbot/Handlers/MyCallsActionHandler.csCore/Resgrid.Chatbot/Handlers/MyScheduleActionHandler.csCore/Resgrid.Chatbot/Handlers/MyStatusActionHandler.csCore/Resgrid.Chatbot/Handlers/PollCreateHandler.csCore/Resgrid.Chatbot/Handlers/RespondToCallHandler.csCore/Resgrid.Chatbot/Handlers/SetUnitStatusHandler.csCore/Resgrid.Chatbot/Handlers/StaffingActionHandler.csCore/Resgrid.Chatbot/Handlers/StatusActionHandler.csCore/Resgrid.Chatbot/Handlers/UnitsAvailableActionHandler.csCore/Resgrid.Chatbot/Interfaces/IConversationEngine.csCore/Resgrid.Chatbot/Interfaces/ITextResponseResolver.csCore/Resgrid.Chatbot/Localization/ChatbotResources.csCore/Resgrid.Chatbot/Models/ChatbotIntent.csCore/Resgrid.Chatbot/Models/ChatbotSession.csCore/Resgrid.Chatbot/Models/PendingTextResponse.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Chatbot/Services/ConversationEngine.csCore/Resgrid.Chatbot/Services/CustomStateMatcher.csCore/Resgrid.Chatbot/Services/IntentMapper.csCore/Resgrid.Chatbot/Services/RedisSessionStore.csCore/Resgrid.Chatbot/Services/TextResponseResolver.csCore/Resgrid.Model/MessageTypes.csCore/Resgrid.Model/Messages/TextResponsePromptMetadata.csCore/Resgrid.Model/Services/ITextResponsePromptService.csCore/Resgrid.Services/CalendarService.csCore/Resgrid.Services/CommunicationService.csCore/Resgrid.Services/CustomStateService.csCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/TextResponsePromptService.csWorkers/Resgrid.Workers.Framework/Logic/CalendarNotifierLogic.csWorkers/Resgrid.Workers.Framework/Logic/NotificationBroadcastLogic.cs
Comment on lines
+24
to
+45
| private readonly IUsersService _usersService; | ||
| private readonly IActionLogsService _actionLogsService; | ||
| private readonly IUserStateService _userStateService; | ||
| private readonly ICustomStateService _customStateService; | ||
| private readonly IPlatformReportingService _platformReportingService; | ||
| private readonly IAuthorizationService _authorizationService; | ||
|
|
||
| public AvailabilityActionHandler( | ||
| IUsersService usersService, | ||
| IActionLogsService actionLogsService, | ||
| IUserStateService userStateService, | ||
| ICustomStateService customStateService, | ||
| IPlatformReportingService platformReportingService, | ||
| IAuthorizationService authorizationService) | ||
| { | ||
| _usersService = usersService; | ||
| _actionLogsService = actionLogsService; | ||
| _userStateService = userStateService; | ||
| _customStateService = customStateService; | ||
| _platformReportingService = platformReportingService; | ||
| _authorizationService = authorizationService; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use the repository’s required dependency-resolution pattern across the new classes.
Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs#L24-L45: resolve all six services throughBootstrapper.GetKernel().Resolve<T>().Core/Resgrid.Chatbot/Handlers/UnitsAvailableActionHandler.cs#L22-L34: resolve the three handler services explicitly.Core/Resgrid.Chatbot/Handlers/MyCallsActionHandler.cs#L26-L33: resolve calls and units services explicitly.Core/Resgrid.Chatbot/Handlers/PollCreateHandler.cs#L28-L40: resolve message, department, and profile services explicitly.Core/Resgrid.Chatbot/Services/TextResponseResolver.cs#L19-L29: resolve message, calendar, and authorization services explicitly.
As per coding guidelines, use Bootstrapper.GetKernel().Resolve<T>() in constructors rather than constructor injection.
📍 Affects 5 files
Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs#L24-L45(this comment)Core/Resgrid.Chatbot/Handlers/UnitsAvailableActionHandler.cs#L22-L34Core/Resgrid.Chatbot/Handlers/MyCallsActionHandler.cs#L26-L33Core/Resgrid.Chatbot/Handlers/PollCreateHandler.cs#L28-L40Core/Resgrid.Chatbot/Services/TextResponseResolver.cs#L19-L29
🤖 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/AvailabilityActionHandler.cs` around lines 24 -
45, Replace constructor injection with Bootstrapper.GetKernel().Resolve<T>()
assignments for all dependencies in
Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs lines 24-45,
including all six services; resolve the three handler services explicitly in
Core/Resgrid.Chatbot/Handlers/UnitsAvailableActionHandler.cs lines 22-34;
resolve calls and units services explicitly in
Core/Resgrid.Chatbot/Handlers/MyCallsActionHandler.cs lines 26-33; resolve
message, department, and profile services explicitly in
Core/Resgrid.Chatbot/Handlers/PollCreateHandler.cs lines 28-40; and resolve
message, calendar, and authorization services explicitly in
Core/Resgrid.Chatbot/Services/TextResponseResolver.cs lines 19-29. Remove the
corresponding constructor parameters while preserving the existing private
dependency fields.
Source: Coding guidelines
Comment on lines
+29
to
+50
| private readonly ICallsService _callsService; | ||
| private readonly IActionLogsService _actionLogsService; | ||
| private readonly IUnitsService _unitsService; | ||
| private readonly ICustomStateService _customStateService; | ||
| private readonly IUserProfileService _userProfileService; | ||
| private readonly IAuthorizationService _authorizationService; | ||
|
|
||
| public CallRespondersActionHandler( | ||
| ICallsService callsService, | ||
| IActionLogsService actionLogsService, | ||
| IUnitsService unitsService, | ||
| ICustomStateService customStateService, | ||
| IUserProfileService userProfileService, | ||
| IAuthorizationService authorizationService) | ||
| { | ||
| _callsService = callsService; | ||
| _actionLogsService = actionLogsService; | ||
| _unitsService = unitsService; | ||
| _customStateService = customStateService; | ||
| _userProfileService = userProfileService; | ||
| _authorizationService = authorizationService; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use the required Service Locator pattern for the newly added dependencies.
Core/Resgrid.Chatbot/Handlers/CallRespondersActionHandler.cs#L29-L50: resolve the six handler services throughBootstrapper.GetKernel().Core/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.cs#L22-L34: resolve the three handler services through the kernel.Core/Resgrid.Services/CalendarService.cs#L28-L46: resolveITextResponsePromptServiceexplicitly rather than adding an optional parameter.Core/Resgrid.Services/TextResponsePromptService.cs#L14-L19: resolveIMessageServicethrough the kernel.Core/Resgrid.Chatbot/Handlers/MyScheduleActionHandler.cs#L24-L36: resolve the three handler services through the kernel.
As per coding guidelines, “Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection.”
📍 Affects 5 files
Core/Resgrid.Chatbot/Handlers/CallRespondersActionHandler.cs#L29-L50(this comment)Core/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.cs#L22-L34Core/Resgrid.Services/CalendarService.cs#L28-L46Core/Resgrid.Services/TextResponsePromptService.cs#L14-L19Core/Resgrid.Chatbot/Handlers/MyScheduleActionHandler.cs#L24-L36
🤖 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/CallRespondersActionHandler.cs` around lines 29
- 50, Replace constructor injection with explicit
Bootstrapper.GetKernel().Resolve<T>() resolution for all six services in
Core/Resgrid.Chatbot/Handlers/CallRespondersActionHandler.cs#L29-L50, all three
services in
Core/Resgrid.Chatbot/Handlers/CallDispatchedActionHandler.cs#L22-L34, and all
three services in
Core/Resgrid.Chatbot/Handlers/MyScheduleActionHandler.cs#L24-L36. In
Core/Resgrid.Services/CalendarService.cs#L28-L46, resolve
ITextResponsePromptService explicitly instead of using an optional constructor
parameter; in Core/Resgrid.Services/TextResponsePromptService.cs#L14-L19,
resolve IMessageService through the kernel. Preserve each constructor’s existing
initialization behavior while applying the required Service Locator pattern.
Source: Coding guidelines
Comment on lines
+26
to
+31
| public DispatchCallHandler(ICallsService callsService, IAuthorizationService authorizationService, | ||
| IChatbotDepartmentConfigService departmentConfigService = null) | ||
| { | ||
| _callsService = callsService; | ||
| _authorizationService = authorizationService; | ||
| _departmentConfigService = departmentConfigService; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the new services explicitly instead of making them optional. The optional defaults silently disable policy enforcement or response resolution when older construction paths omit them.
Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs#L26-L31: always resolve the department configuration service so dispatch restrictions cannot fail open.Core/Resgrid.Chatbot/Handlers/RespondToCallHandler.cs#L29-L37: always resolve custom-state, group, and role services so eligible dispatches are not omitted.Core/Resgrid.Chatbot/Handlers/StatusActionHandler.cs#L18-L23: always resolve department configuration so required confirmation cannot be bypassed.
As per coding guidelines, “Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection.”
📍 Affects 3 files
Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs#L26-L31(this comment)Core/Resgrid.Chatbot/Handlers/RespondToCallHandler.cs#L29-L37Core/Resgrid.Chatbot/Handlers/StatusActionHandler.cs#L18-L23
🤖 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/DispatchCallHandler.cs` around lines 26 - 31,
Replace optional constructor service parameters with explicit
Bootstrapper.GetKernel().Resolve<T>() resolution: in
Core/Resgrid.Chatbot/Handlers/DispatchCallHandler.cs lines 26-31, always resolve
IChatbotDepartmentConfigService; in
Core/Resgrid.Chatbot/Handlers/RespondToCallHandler.cs lines 29-37, always
resolve the custom-state, group, and role services; and in
Core/Resgrid.Chatbot/Handlers/StatusActionHandler.cs lines 18-23, always resolve
the department configuration service. Update each constructor’s field assignment
while preserving the existing handler behavior.
Source: Coding guidelines
Comment on lines
+585
to
+598
| private async System.Threading.Tasks.Task SendCalendarPromptAsync(CalendarItem calendarItem, string userId, string message, | ||
| string departmentNumber, string title, UserProfile profile, Department department) | ||
| { | ||
| await _communicationService.SendCalendarAsync(userId, calendarItem.DepartmentId, message, | ||
| departmentNumber, title, profile, department); | ||
|
|
||
| if (_textResponsePromptService == null | ||
| || calendarItem.SignupType != (int)CalendarItemSignupTypes.RSVP) | ||
| return; | ||
|
|
||
| try | ||
| { | ||
| await _textResponsePromptService.RecordCalendarRsvpPromptAsync(calendarItem, userId); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist RSVP prompts only after communication accepts delivery.
All three paths ignore the communication method’s boolean result, creating pending responses for filtered recipients.
Core/Resgrid.Services/CalendarService.cs#L585-L598: checkSendCalendarAsyncbefore recording.Workers/Resgrid.Workers.Framework/Logic/CalendarNotifierLogic.cs#L142-L153: checkSendNotificationAsyncbefore recording.Workers/Resgrid.Workers.Framework/Logic/NotificationBroadcastLogic.cs#L87-L92: retain the send result and skip recording when false.
📍 Affects 3 files
Core/Resgrid.Services/CalendarService.cs#L585-L598(this comment)Workers/Resgrid.Workers.Framework/Logic/CalendarNotifierLogic.cs#L142-L153Workers/Resgrid.Workers.Framework/Logic/NotificationBroadcastLogic.cs#L87-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/CalendarService.cs` around lines 585 - 598, Use the
boolean result from SendCalendarAsync in CalendarService.SendCalendarPromptAsync
and record the RSVP prompt only when delivery is accepted. Apply the same guard
to SendNotificationAsync in CalendarNotifierLogic, and retain its send result in
NotificationBroadcastLogic before skipping prompt recording when it is false;
update all three specified sites accordingly.
Resgrid-Bot
reviewed
Jul 17, 2026
| if (state == null) | ||
| return AvailabilityClass.Unknown; | ||
|
|
||
| if (state.State <= 25) |
There was a problem hiding this comment.
Magic number literal 25 acts as a threshold distinguishing built-in UserStateTypes values from custom CustomStateDetailId values across multiple call sites (CallRespondersActionHandler.cs:198,216; StatusActionHandler.cs:121; CustomStateMatcher.cs:23-36) without a named constant. Define a shared constant (e.g., MaxBuiltInStateDetailId = 25) or reference an existing one from the Model layer.
Kody rule violation: Replace magic numbers with named constants
private const int MaxBuiltInStateDetailId = 25;
// ...
if (state.State <= MaxBuiltInStateDetailId)Prompt for LLM
File Core/Resgrid.Chatbot/Handlers/AvailabilityActionHandler.cs:
Line 123:
Violates rule 'Replace magic numbers with named constants': the literal `25` is used as a threshold to distinguish built-in UserStateTypes values from custom CustomStateDetailId values. This number has domain meaning but is not a named constant. The same unguarded literal appears in CallRespondersActionHandler.cs:198,216 and StatusActionHandler.cs:121. Define a shared constant (e.g., `private const int MaxBuiltInStateDetailId = 25;`) or reference an existing one from the Model layer.
Suggested Code:
private const int MaxBuiltInStateDetailId = 25;
// ...
if (state.State <= MaxBuiltInStateDetailId)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 17, 2026
Resgrid-Bot
reviewed
Jul 17, 2026
Comment on lines
+99
to
+111
| foreach (var activeCall in (activeCalls ?? new List<Call>()) | ||
| .OrderByDescending(x => x.LastDispatchedOn ?? x.DispatchOn ?? x.LoggedOn)) | ||
| { | ||
| var call = await _callsService.PopulateCallData(activeCall, | ||
| getDispatches: true, | ||
| getAttachments: false, | ||
| getNotes: false, | ||
| getGroupDispatches: true, | ||
| getUnitDispatches: false, | ||
| getRoleDispatches: true, | ||
| getProtocols: false, | ||
| getReferences: false, | ||
| getContacts: false); |
There was a problem hiding this comment.
N+1 query in ResolveMostRecentDispatchAsync: the method iterates all active calls invoking _callsService.PopulateCallData() per call plus nested _departmentGroupsService.GetAllMembersForGroupAsync() calls with no scan limit, unlike the sibling MyCallsActionHandler.GetActiveCallsWithDispatchesAsync which bounds with .Take(MaxActiveCallsToScan) (25). Add a .Take(N) limit to bound the number of PopulateCallData calls.
Kody rule violation: Detect N+1 style queries and suggest batching
private const int MaxActiveCallsToScan = 25;
// ...
foreach (var activeCall in (activeCalls ?? new List<Call>())
.OrderByDescending(x => x.LastDispatchedOn ?? x.DispatchOn ?? x.LoggedOn)
.Take(MaxActiveCallsToScan))
{
var call = await _callsService.PopulateCallData(activeCall, ...);Prompt for LLM
File Core/Resgrid.Chatbot/Handlers/RespondToCallHandler.cs:
Line 99 to 111:
Violates rule 'Detect N+1 style queries and suggest batching': `ResolveMostRecentDispatchAsync` iterates through ALL active calls calling `_callsService.PopulateCallData()` (a multi-query DB operation) per call, plus `_departmentGroupsService.GetAllMembersForGroupAsync()` inside nested loops, with no scan limit. The sibling `MyCallsActionHandler.GetActiveCallsWithDispatchesAsync` in the same PR correctly limits with `.Take(MaxActiveCallsToScan)` (25), but this method does not. Add a `.Take(N)` limit to bound the number of PopulateCallData calls.
Suggested Code:
private const int MaxActiveCallsToScan = 25;
// ...
foreach (var activeCall in (activeCalls ?? new List<Call>())
.OrderByDescending(x => x.LastDispatchedOn ?? x.DispatchOn ?? x.LoggedOn)
.Take(MaxActiveCallsToScan))
{
var call = await _callsService.PopulateCallData(activeCall, ...);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 17, 2026
Comment on lines
+105
to
+111
| if (assignedState == null || assignedState.IsDeleted | ||
| || assignedState.DepartmentId != departmentId | ||
| || assignedState.Type != (int)CustomStateTypes.Unit) | ||
| return new List<CustomStateDetail>(); | ||
|
|
||
| return assignedState.GetActiveDetails(); | ||
| } |
There was a problem hiding this comment.
Logic gap in GetAllowedStatesAsync: returning an empty list for null or deleted custom state sets causes every status name lookup to fail with a misleading 'Unknown status' response, locking the unit out of chatbot status changes until an admin fixes the configuration. Fall back to GetDefaultUnitStatuses() for the IsDeleted/null cases, matching the CustomStatesId <= 0 branch.
if (assignedState == null || assignedState.IsDeleted)
return _customStateService.GetDefaultUnitStatuses() ?? new List<CustomStateDetail>();
if (assignedState.DepartmentId != departmentId
|| assignedState.Type != (int)CustomStateTypes.Unit)
return new List<CustomStateDetail>();
return assignedState.GetActiveDetails();Prompt for LLM
File Core/Resgrid.Chatbot/Handlers/SetUnitStatusHandler.cs:
Line 105 to 111:
WHAT: GetAllowedStatesAsync returns an empty list when the unit type's assigned custom state set is deleted or null, instead of falling back to default unit statuses. WHY: Every status name the user sends will fail to match against the empty list, producing a misleading 'Unknown status' response and locking the unit out of chatbot status changes until an admin fixes the configuration. HOW: Fall back to GetDefaultUnitStatuses() for the IsDeleted/null cases, matching the CustomStatesId <= 0 branch.
Suggested Code:
if (assignedState == null || assignedState.IsDeleted)
return _customStateService.GetDefaultUnitStatuses() ?? new List<CustomStateDetail>();
if (assignedState.DepartmentId != departmentId
|| assignedState.Type != (int)CustomStateTypes.Unit)
return new List<CustomStateDetail>();
return assignedState.GetActiveDetails();
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 17, 2026
| : (ChatbotConfig.DefaultSessionTimeoutMinutes > 0 ? ChatbotConfig.DefaultSessionTimeoutMinutes : 30)); | ||
|
|
||
| private bool UseRedis() | ||
| { |
There was a problem hiding this comment.
Silent exception swallowing: the bare catch { return false; } skips logging that every other catch block in RedisSessionStore.cs performs via Logging.LogException(ex) (lines 78-82, 102-105, 131-134, 155-158). Log the exception with Logging.LogException(ex) before returning false.
Kody rule violation: Avoid empty catch blocks
try
{
return _cacheProvider.IsConnected();
}
catch (Exception ex)
{
Logging.LogException(ex);
return false;
}Prompt for LLM
File Core/Resgrid.Chatbot/Services/RedisSessionStore.cs:
Line 41:
Bare `catch { return false; }` swallows the exception from `IsConnected()` without logging. Every other catch block in this same file logs with `Logging.LogException(ex)` (lines 78-82, 102-105, 131-134, 155-158). Rule 33 requires logging exceptions with context rather than silently swallowing them.
Suggested Code:
try
{
return _cacheProvider.IsConnected();
}
catch (Exception ex)
{
Logging.LogException(ex);
return false;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
Description
This PR significantly expands the Resgrid chatbot's natural language capabilities with new query intents, improved custom state resolution, and text-based RSVP/poll response handling.
New Chatbot Intents & Handlers
who_available) — Lists personnel whose status and staffing classify as available to respond.units_available) — Lists units currently classified as available/in service.call_responders— Personnel and units currently en route or on scene for a call.call_dispatched— The full dispatch list (personnel, groups, roles, units) for a call.my_calls) / Unit calls (unit_calls) — Active calls the user or a named unit was dispatched to.my_schedule) — User's shifts plus RSVP'd calendar events for a given day.create_poll) — Admin-only yes/no poll broadcast to all department members, with confirmation before sending.Reworked Call Response Flow
BaseType, falling back to built-in ActionTypes.Custom State Resolution Overhaul
CustomStateMatcherservice standardizes state lookup by ID, name,BaseType, and numeric selection across status, staffing, and unit status handlers.Text-Based RSVP & Poll Responses
CalendarRsvpmessage type andTextResponsePromptServicerecord RSVP prompts when calendar notifications are sent.TextResponseResolverresolves bare YES/NO replies against recent unanswered polls and calendar RSVPs, with a numbered disambiguation flow when multiple items are pending.Other Changes
AllowDispatchViaChatbot,RequireConfirmationForStatusChange, andSessionTtlMinutes.Summary by CodeRabbit