Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ❌ |
📝 WalkthroughWalkthroughAdds read-only incident-command chatbot support with ICS intents, authorized incident context resolution, board narration, playbook guidance, web endpoints, incident context propagation, and chatbot audit logging. It also updates web shell layout, browser error handling, metadata validation, and generated API documentation. ChangesIncident command assistant
Web shell behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WebClient
participant ChatbotController
participant ChatbotIngressService
participant IncidentCommandActionHandler
participant IncidentContextResolver
participant IncidentBoardNarrator
WebClient->>ChatbotController: Submit incident question and optional call ID
ChatbotController->>ChatbotIngressService: Process WebChat message
ChatbotIngressService->>IncidentCommandActionHandler: Dispatch classified intent
IncidentCommandActionHandler->>IncidentContextResolver: Resolve authorized incident context
IncidentContextResolver-->>IncidentCommandActionHandler: Return IncidentContext
IncidentCommandActionHandler->>IncidentBoardNarrator: Generate incident-board response
IncidentBoardNarrator-->>ChatbotIngressService: Return localized narration
ChatbotIngressService-->>ChatbotController: Return chatbot response
ChatbotController-->>WebClient: Return answer and intent metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost("AskIncident")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<IncidentAssistantAnswerResult>> AskIncident([FromBody] AskIncidentAssistantInput input) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs (1)
57-76: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winResolve the repository through the required service locator.
The new constructor parameter extends the constructor-injected dependency graph. Resolve
IChatbotMessageLogRepositorywithBootstrapper.GetKernel().Resolve<IChatbotMessageLogRepository>()in the constructor instead.As per coding guidelines, use
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/Services/ChatbotIngressService.cs` around lines 57 - 76, Remove the IChatbotMessageLogRepository constructor parameter and stop assigning it directly in ChatbotIngressService. In the constructor, initialize _messageLogRepository by resolving IChatbotMessageLogRepository through Bootstrapper.GetKernel().Resolve<IChatbotMessageLogRepository>(), while leaving the other injected dependencies unchanged.Source: Coding guidelines
🧹 Nitpick comments (9)
Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs (1)
15-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConvert
RoleWordsinternal optional groups to non-capturing groups.
RoleWordsis the outer capturing group referenced by fixed index (m.Groups[3]on line 171,m.Groups[4]on line 175). It embeds roughly 25 additional unnamed capturing groups for its internal optional suffixes (for example(\s+commander)?,(\s+chief)?,(\s+officer)?).These internal groups do not shift the index of the outer
RoleWordsgroup itself (group numbering runs left to right by opening parenthesis, and the outer group opens first). Today's two usages are correctly indexed. But any future edit that adds a capturing group before or insideRoleWordswill silently change downstream group numbers with no runtime error — theroleQueryparameter would then quietly hold the wrong text.Use
(?:...)for the internal optional suffixes soRoleWordsstays the only capturing group in play, removing this fragility for future maintainers.♻️ Proposed fix: make internal RoleWords suffixes non-capturing
private const string RoleWords = - @"(ic|incident\s+commander|deputy(\s+incident)?(\s+commander)?|commander|unified\s+command|safety(\s+officer)?|" + - @"ops(\s+chief)?|operations(\s+section)?(\s+chief)?|planning(\s+section)?(\s+chief)?|logistics(\s+section)?(\s+chief)?|" + - @"finance(\s+admin)?(\s+section)?(\s+chief)?|pio|public\s+information\s+officer|liaison(\s+officer)?|" + - @"staging(\s+area)?\s+manager|resources?\s+unit\s+leader|situation\s+unit\s+leader|documentation\s+unit\s+leader|" + - @"communications\s+unit\s+leader|division\s+supervisor|group\s+supervisor|branch\s+director|" + - @"strike\s+team\s+leader|task\s+force\s+leader|medical\s+unit\s+leader|rehab(\s+officer)?|medical\s+branch\s+director|" + - @"triage(\s+officer)?|treatment(\s+officer)?|transport(\s+officer)?|hazmat\s+group\s+supervisor|decon(\s+officer)?|" + - @"entry\s+team\s+leader|search\s+group\s+supervisor|air\s+operations(\s+branch)?(\s+director)?|" + - @"shelter(\s+mass\s+care)?\s+coordinator|mass\s+care\s+coordinator|damage\s+assessment\s+lead|" + - @"rit|ric|rapid\s+intervention(\s+team|\s+crew)?|accountability\s+officer)"; + @"(ic|incident\s+commander|deputy(?:\s+incident)?(?:\s+commander)?|commander|unified\s+command|safety(?:\s+officer)?|" + + @"ops(?:\s+chief)?|operations(?:\s+section)?(?:\s+chief)?|planning(?:\s+section)?(?:\s+chief)?|logistics(?:\s+section)?(?:\s+chief)?|" + + @"finance(?:\s+admin)?(?:\s+section)?(?:\s+chief)?|pio|public\s+information\s+officer|liaison(?:\s+officer)?|" + + @"staging(?:\s+area)?\s+manager|resources?\s+unit\s+leader|situation\s+unit\s+leader|documentation\s+unit\s+leader|" + + @"communications\s+unit\s+leader|division\s+supervisor|group\s+supervisor|branch\s+director|" + + @"strike\s+team\s+leader|task\s+force\s+leader|medical\s+unit\s+leader|rehab(?:\s+officer)?|medical\s+branch\s+director|" + + @"triage(?:\s+officer)?|treatment(?:\s+officer)?|transport(?:\s+officer)?|hazmat\s+group\s+supervisor|decon(?:\s+officer)?|" + + @"entry\s+team\s+leader|search\s+group\s+supervisor|air\s+operations(?:\s+branch)?(?:\s+director)?|" + + @"shelter(?:\s+mass\s+care)?\s+coordinator|mass\s+care\s+coordinator|damage\s+assessment\s+lead|" + + @"rit|ric|rapid\s+intervention(?:\s+team|\s+crew)?|accountability\s+officer)";🤖 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/KeywordIntentClassifier.cs` around lines 15 - 33, Update the RoleWords regex so every internal optional suffix/group uses non-capturing syntax (?:...) while preserving the outer capturing group. Keep the existing role vocabulary and downstream m.Groups[3]/m.Groups[4] references unchanged, ensuring RoleWords remains the only capture within this pattern.Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs (3)
46-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe constructor now takes 15 dependencies.
The coding guidelines ask to keep injected dependencies small and to resolve dependencies explicitly with the service locator. This change adds two more constructor parameters. Consider resolving
IChatbotIngressServiceandICallsServicethroughBootstrapper.GetKernel().Resolve<T>(), or splitting the incident-assistant endpoints into a separate controller.As per coding guidelines: "Minimize constructor injection; keep the number of injected dependencies small" and "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 `@Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs` around lines 46 - 61, Reduce ChatbotController constructor injection by removing IChatbotIngressService and ICallsService from its parameters and resolving them explicitly via Bootstrapper.GetKernel().Resolve<T>() where the controller initializes or uses those services. Preserve the existing behavior of the incident-assistant endpoints and keep the remaining constructor dependencies unchanged.Source: Coding guidelines
519-522: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse the shared incident context key constant.
ChatbotIngressServicealready mapsPlatformMetadata["incidentCallId"]intoChatbotSession.ContextandIncidentContextResolver.IncidentCallIdContextKeyreads it there. InChatbotController.cs, write metadata withServices.IncidentContextResolver.IncidentCallIdContextKeyinstead of the literal string to avoid the two incident-call hints diverging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs` around lines 519 - 522, Update the metadata assignment in the ChatbotController flow before ProcessMessageAsync to use Services.IncidentContextResolver.IncidentCallIdContextKey instead of the literal "incidentCallId", preserving the existing input.CallId > 0 condition.
499-545: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo change needed. The NLU and conversational fallback LLM calls use bounded per-request timeouts, with NLU defaulting to 10 seconds and chat completion defaulting to 15 seconds. Add explicit
CancellationTokenpropagation fromAskIncidentonly if request cancellation semantics need to take priority over those configured timeouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs` around lines 499 - 545, No code change is required in AskIncident because downstream NLU and conversational fallback calls already use bounded per-request timeouts. Only add CancellationToken propagation through AskIncident and its service calls if request cancellation must take priority over those configured timeouts.Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs (2)
719-725: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ChecklistFordoes not de-duplicate.The summary states the result is "de-duplicated, type-specific wording winning", but the method concatenates both lists without a distinct pass.
KeyRolesForon Line 733 does applyDistinct().♻️ Proposed fix
- return playbook.Checklist.Concat(GeneralPlaybook.Checklist).ToList(); + return playbook.Checklist.Concat(GeneralPlaybook.Checklist) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList();🤖 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/IcsPlaybooks.cs` around lines 719 - 725, Update ChecklistFor to de-duplicate the combined playbook and GeneralPlaybook checklist entries, matching the behavior of KeyRolesFor. Preserve the ordering so type-specific checklist wording remains before general entries and therefore wins when duplicates are removed.
31-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlaybook state is mutable and shared across all requests.
IncidentPlaybookexposes public setters, andAll,General, andGethand out the shared static instances. A caller can mutateChecklist,KeyRoles, orDisplayNameand corrupt the doctrine table for every subsequent request in the process. The coding guidelines ask for immutable data and separation of state from behavior.Consider
init-only properties (or a record) so the table cannot be changed after construction.As per coding guidelines: "Prefer functional patterns and immutable data where appropriate in C#".
Also applies to: 640-648
🤖 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/IcsPlaybooks.cs` around lines 31 - 51, Make IncidentPlaybook immutable after initialization by replacing its public setters with init-only properties (or converting it to an equivalent immutable record), while preserving the existing defaults and read-only collection types. Ensure the shared instances exposed through All, General, and Get cannot have DisplayName, Checklist, KeyRoles, or other doctrine data reassigned by callers.Source: Coding guidelines
Core/Resgrid.Chatbot/Services/IncidentContextResolver.cs (1)
96-104: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCandidate resolution issues two sequential service calls per active command.
For each active command the loop awaits
GetCallByIdAsyncand thenCanUserViewCallAsync. A department running many concurrent commands pays 2N sequential round-trips before the assistant can even ask which incident the commander meant. This path runs on the request thread for the synchronousAskIncidentendpoint.Consider bounding the candidate list before the loop, or batching the call lookup if
ICallsServiceexposes a multi-id read.🤖 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/IncidentContextResolver.cs` around lines 96 - 104, Reduce sequential service calls in candidate resolution by bounding candidates before the loop or using an available multi-ID lookup from ICallsService. Update the flow around GetCallByIdAsync and CanUserViewCallAsync so active commands do not incur unnecessary lookups, while preserving department filtering and authorization checks before adding entries to context.Candidates.Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs (2)
48-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNull handling for
sessionis inconsistent.Each of these methods reads
session?.Cultureand then dereferencessession.DepartmentIda few lines later. Ifsessioncan be null, the second access throws. Ifsessioncannot be null, the null-conditional operator is misleading.Today
IncidentCommandActionHandlernever reaches narration with a null session, becauseIncidentContextResolver.ResolveAsyncreturns an empty context for a null session. Pick one contract and apply it in all narrator methods.Also applies to: 482-486, 528-532, 560-566, 717-724
🤖 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/IncidentBoardNarrator.cs` around lines 48 - 57, Make session handling consistent across all narrator methods, including DescribeStatusAsync and the methods containing the referenced ranges: either enforce a non-null session contract and replace session?.Culture with direct access, or support null sessions by guarding every later session.DepartmentId dereference and defining the appropriate fallback. Apply the same contract uniformly throughout IncidentBoardNarrator.
905-918: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe department profile roster is read two or three times per answer.
BuildResourceNameLookupAsynccallsGetPersonNamesAsyncinternally. Several callers also callGetPersonNamesAsyncdirectly in the same request:
DescribeResourcesAsync(Line 160 and Line 185)DescribeBriefingAsync(Line 565 and Line 567)BuildGroundingSnapshotAsync(Line 778 and Line 779)Each call reaches
IUserProfileService.GetAllProfilesForDepartmentAsync.AskIncidentruns this inline on the request thread.Cache the result for the lifetime of one narration call, for example by passing the already-resolved dictionary into
BuildResourceNameLookupAsync.♻️ Proposed change
- private async Task<Dictionary<string, string>> BuildResourceNameLookupAsync(IncidentContext context, ChatbotSession session) + private async Task<Dictionary<string, string>> BuildResourceNameLookupAsync( + IncidentContext context, + ChatbotSession session, + Dictionary<string, UserProfile> profiles = null) { var lookup = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); - var profiles = await GetPersonNamesAsync(session); + profiles ??= await GetPersonNamesAsync(session);Also applies to: 935-948
🤖 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/IncidentBoardNarrator.cs` around lines 905 - 918, Resolve the department profiles once per narration in AskIncident and reuse that dictionary throughout the request. Update DescribeResourcesAsync, DescribeBriefingAsync, and BuildGroundingSnapshotAsync, including BuildResourceNameLookupAsync, to accept and use the already-resolved profiles instead of calling GetPersonNamesAsync repeatedly. Preserve the existing empty-dictionary fallback and per-request scope.
🤖 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/Services/ChatbotIngressService.cs`:
- Around line 634-650: Update the logging flow before
_messageLogRepository.InsertAsync in ChatbotIngressService to avoid persisting
raw message.Text and exception details by default. Store only a controlled
reason and safe metadata in MessageText and ErrorInfo; if message mining is
required, apply the project’s established redaction or encryption plus access
and retention controls before insertion.
- Line 586: The fallback and pipeline-error paths in ChatbotIngressService
should not block indefinitely on audit persistence. Update the
LogUnhandledMessageAsync call sites and its InsertAsync implementation to use a
bounded cancellation token and database command timeout, or enqueue writes
through an existing durable background mechanism, while preserving the chatbot
response flow.
In `@Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs`:
- Around line 654-671: Update Resolve to select the matching playbook by the
highest keyword-length score rather than returning the first match, while
preserving exact DisplayName matching. Replace plain substring checks with the
existing word-boundary-aware ContainsPhrase helper, and apply that guard to
Infer’s keyword matching as well.
In `@Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs`:
- Around line 1005-1027: Update MatchNode so the contains-based fallback only
returns a node when exactly one lane matches; return null when contains contains
multiple candidates, preserving exact-match behavior and the existing unique
byType fallback.
In `@Core/Resgrid.Chatbot/Services/IncidentContextResolver.cs`:
- Around line 123-133: Track degraded board reads explicitly: in
Core/Resgrid.Chatbot/Services/IncidentContextResolver.cs lines 123-133, set
BoardReadFailed before returning from the board-read catch; in
Core/Resgrid.Chatbot/Interfaces/IIncidentContextResolver.cs lines 38-45, add the
property and exclude it from HasNoCommand; in
Core/Resgrid.Chatbot/Handlers/IncidentCommandActionHandler.cs lines 80-88,
return Incident_BoardUnavailable for this flag before the HasNoCommand branch.
In `@Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js`:
- Around line 14-17: Update the DataTables error handling near the document
error.dt listener: remove the global $.fn.dataTable.ext.errMode = 'none'
suppression and only intercept 401 errors originating from _fnAjaxUpdate when
jqXHR.status === 401, routing them through the existing redirect behavior.
Preserve DataTables’ default error rendering for all other errors and avoid
relying solely on console.warn.
In `@Web/Resgrid.Web/wwwroot/scss/_custom.scss`:
- Around line 37-43: Update the fixed sidebar styles under the desktop media
query in Web/Resgrid.Web/wwwroot/scss/_custom.scss so body.rtls resets left and
positions the sidebar at right: 0; regenerate
Web/Resgrid.Web/wwwroot/css/style.css to include the corresponding compiled
override at lines 9963-9969.
---
Outside diff comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 57-76: Remove the IChatbotMessageLogRepository constructor
parameter and stop assigning it directly in ChatbotIngressService. In the
constructor, initialize _messageLogRepository by resolving
IChatbotMessageLogRepository through
Bootstrapper.GetKernel().Resolve<IChatbotMessageLogRepository>(), while leaving
the other injected dependencies unchanged.
---
Nitpick comments:
In `@Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs`:
- Around line 15-33: Update the RoleWords regex so every internal optional
suffix/group uses non-capturing syntax (?:...) while preserving the outer
capturing group. Keep the existing role vocabulary and downstream
m.Groups[3]/m.Groups[4] references unchanged, ensuring RoleWords remains the
only capture within this pattern.
In `@Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs`:
- Around line 719-725: Update ChecklistFor to de-duplicate the combined playbook
and GeneralPlaybook checklist entries, matching the behavior of KeyRolesFor.
Preserve the ordering so type-specific checklist wording remains before general
entries and therefore wins when duplicates are removed.
- Around line 31-51: Make IncidentPlaybook immutable after initialization by
replacing its public setters with init-only properties (or converting it to an
equivalent immutable record), while preserving the existing defaults and
read-only collection types. Ensure the shared instances exposed through All,
General, and Get cannot have DisplayName, Checklist, KeyRoles, or other doctrine
data reassigned by callers.
In `@Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs`:
- Around line 48-57: Make session handling consistent across all narrator
methods, including DescribeStatusAsync and the methods containing the referenced
ranges: either enforce a non-null session contract and replace session?.Culture
with direct access, or support null sessions by guarding every later
session.DepartmentId dereference and defining the appropriate fallback. Apply
the same contract uniformly throughout IncidentBoardNarrator.
- Around line 905-918: Resolve the department profiles once per narration in
AskIncident and reuse that dictionary throughout the request. Update
DescribeResourcesAsync, DescribeBriefingAsync, and BuildGroundingSnapshotAsync,
including BuildResourceNameLookupAsync, to accept and use the already-resolved
profiles instead of calling GetPersonNamesAsync repeatedly. Preserve the
existing empty-dictionary fallback and per-request scope.
In `@Core/Resgrid.Chatbot/Services/IncidentContextResolver.cs`:
- Around line 96-104: Reduce sequential service calls in candidate resolution by
bounding candidates before the loop or using an available multi-ID lookup from
ICallsService. Update the flow around GetCallByIdAsync and CanUserViewCallAsync
so active commands do not incur unnecessary lookups, while preserving department
filtering and authorization checks before adding entries to context.Candidates.
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs`:
- Around line 46-61: Reduce ChatbotController constructor injection by removing
IChatbotIngressService and ICallsService from its parameters and resolving them
explicitly via Bootstrapper.GetKernel().Resolve<T>() where the controller
initializes or uses those services. Preserve the existing behavior of the
incident-assistant endpoints and keep the remaining constructor dependencies
unchanged.
- Around line 519-522: Update the metadata assignment in the ChatbotController
flow before ProcessMessageAsync to use
Services.IncidentContextResolver.IncidentCallIdContextKey instead of the literal
"incidentCallId", preserving the existing input.CallId > 0 condition.
- Around line 499-545: No code change is required in AskIncident because
downstream NLU and conversational fallback calls already use bounded per-request
timeouts. Only add CancellationToken propagation through AskIncident and its
service calls if request cancellation must take priority over those configured
timeouts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 76ef20d4-9989-41e4-b4f6-8bea881b9e6c
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Chatbot/ChatbotTextResponseResolverTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Chatbot/IncidentCommandIntentClassifierTests.csis excluded by!**/Tests/**
📒 Files selected for processing (29)
Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.csCore/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.csCore/Resgrid.Chatbot/ChatbotModule.csCore/Resgrid.Chatbot/Handlers/IncidentCommandActionHandler.csCore/Resgrid.Chatbot/Interfaces/IIncidentBoardNarrator.csCore/Resgrid.Chatbot/Interfaces/IIncidentContextResolver.csCore/Resgrid.Chatbot/Localization/ChatbotResources.csCore/Resgrid.Chatbot/Models/ChatbotIntent.csCore/Resgrid.Chatbot/Models/ChatbotMessageLog.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Chatbot/Services/ConversationalFallbackService.csCore/Resgrid.Chatbot/Services/IcsPlaybooks.csCore/Resgrid.Chatbot/Services/IncidentBoardNarrator.csCore/Resgrid.Chatbot/Services/IncidentContextResolver.csCore/Resgrid.Chatbot/Services/IncidentRoleVocabulary.csCore/Resgrid.Chatbot/Services/IntentMapper.csCore/Resgrid.Model/ChatbotMessageLog.csCore/Resgrid.Model/Queue/ChatbotMessageQueueItem.csCore/Resgrid.Model/Repositories/IChatbotMessageLogRepository.csRepositories/Resgrid.Repositories.DataRepository/ChatbotMessageLogRepository.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.csWeb/Resgrid.Web.Services/Controllers/v4/ChatbotController.csWeb/Resgrid.Web.Services/Models/v4/Chat/ChatApiModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/wwwroot/css/style.cssWeb/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.jsWeb/Resgrid.Web/wwwroot/scss/_custom.scssWorkers/Resgrid.Workers.Framework/Logic/ChatbotMessageLogic.cs
💤 Files with no reviewable changes (1)
- Core/Resgrid.Chatbot/Models/ChatbotMessageLog.cs
| { | ||
| // The LLM answered but no structured intent could — still a coverage gap | ||
| // worth mining, so it's audited before the reply goes out. | ||
| await LogUnhandledMessageAsync(message, session, intent, Model.ChatbotMessageLog.ReasonFallbackAnswered, processed: true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the audit write before returning the chatbot response.
Lines 586, 599, and 611 await LogUnhandledMessageAsync. That helper awaits InsertAsync with CancellationToken.None. A slow or unavailable audit database can delay or time out the fallback and pipeline-error responses. Use a bounded cancellation token and command timeout, or enqueue the audit to a durable background path.
Also applies to: 599-611, 638-651
🤖 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` at line 586, The
fallback and pipeline-error paths in ChatbotIngressService should not block
indefinitely on audit persistence. Update the LogUnhandledMessageAsync call
sites and its InsertAsync implementation to use a bounded cancellation token and
database command timeout, or enqueue writes through an existing durable
background mechanism, while preserving the chatbot response flow.
| var errorInfo = string.IsNullOrWhiteSpace(error) ? reason : $"{reason}: {error}"; | ||
| if (errorInfo.Length > 500) | ||
| errorInfo = errorInfo.Substring(0, 500); | ||
|
|
||
| await _messageLogRepository.InsertAsync(new Model.ChatbotMessageLog | ||
| { | ||
| Id = Guid.NewGuid().ToString("N"), | ||
| DepartmentId = session?.DepartmentId ?? 0, | ||
| UserId = session?.UserId, | ||
| SessionId = session?.SessionId, | ||
| Platform = (int)message.Platform, | ||
| Direction = "inbound", | ||
| MessageText = message.Text, | ||
| IntentType = intent != null ? (int?)intent.Type : null, | ||
| Processed = processed, | ||
| ErrorInfo = errorInfo, | ||
| Timestamp = DateTime.UtcNow |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not store raw chatbot text and exception messages without privacy controls.
MessageText can contain incident, medical, contact, or operational data. ex.Message can expose backend details. Persist a controlled reason and safe metadata by default. If message mining is required, redact or encrypt the content and apply defined access and retention controls before InsertAsync.
🤖 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 634 -
650, Update the logging flow before _messageLogRepository.InsertAsync in
ChatbotIngressService to avoid persisting raw message.Text and exception details
by default. Store only a controlled reason and safe metadata in MessageText and
ErrorInfo; if message mining is required, apply the project’s established
redaction or encryption plus access and retention controls before insertion.
| $.fn.dataTable.ext.errMode = 'none'; | ||
| $(document).on('error.dt', function (e, settings, techNote, message) { | ||
| if (window.console && console.warn) { console.warn(message); } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -ni -C 4 \
'cdn\.datatables|datatables\.net|jquery\.dataTables|dataTables|error\.dt|dt-error' \
Web/Resgrid.Web \
--glob '*.cshtml' \
--glob '*.html' \
--glob '*.js' \
--glob '*.json'Repository: Resgrid/Core
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed file and locate DataTables assets/scripts without scanning the huge library file.
echo "== reviewed file =="
sed -n '1,120p' Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js
echo
echo "== DataTables asset script tags =="
rg -n 'dataTables.*\.js|jquery\.datatables|dataTables' \
Web/Resgrid.Web --glob '*.cshtml' --glob '*.html' --glob '*.js' --glob '*.json' \
| grep -v 'wwwroot/clib/data-tables/datatables.js' | head -n 200
echo
echo "== library file header/version hints =="
sed -n '1,120p' Web/Resgrid.Web/wwwroot/clib/data-tables/datatables.js
grep -nEi 'DataTables Version|version|dataTables.version|error\.dt|dt-error' Web/Resgrid.Web/wwwroot/clib/data-tables/datatables.js | head -n 80Repository: Resgrid/Core
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== reviewed file lines 1-45 =="
sed -n '1,45p' Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js
echo
echo "== DataTables 1.13.5 core size/first lines =="
wc -l Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js
sed -n '1,120p' Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js
echo
echo "== versions / package files =="
for f in package.json package-lock.json Web/Resgrid.Web/package.json Web/Resgrid.Web/package-lock.json global.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n '"datatables|DataTables|jquery.*dataTables|jQuery|dotnet/|sdk|targetFrameworks|aspnetcore|TargetFramework' "$f" || true
fi
done
echo
echo "== DataTables event/event namespace occurrences in vendored 1.13.5 =="
rg -n '\b(error\.dt|dt-error\.dt|\.on\(["'\''][^"'\'']*error|\.Trigger\(["'\''][^"'\'']*error|event\.namespace|namespace:' Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js | head -n 120Repository: Resgrid/Core
Length of output: 8529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp=/tmp/data-tables-1.13.5-events.$$
mkdir -p "$tmp"
echo "== error-related lines with line numbers in 1.13.5 =="
rg -n "error|Trigger|namespace|DataTable\.event|_fnLog|FnEventFire|fire" Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js | grep -iE 'error|namespace|ajax|Trigger' | head -n 160
echo
echo "== locate _fnBuildAjax/error handling functions =="
rg -n "_fnBuildAjax|_fnAjaxUpdate|_fnLog|fnEventFire|DataTable\.event|Trigger\\(" Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js | head -n 120
echo
echo "== inspect relevant source ranges =="
for range in "5900,6055" "6500,6645" "2360,2385"; do
echo "--- src:${range} ---"
sed -n "${range}p" Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js
done
echo
echo "== event name strings in 1.13.5 =="
rg -n "['\"][^{}]+\\.dt['\"]|namespace" Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js | head -n 200Repository: Resgrid/Core
Length of output: 18079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect exact _fnBuildAjax callbacks =="
sed -n '3925,4055p' Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js
echo
echo "== inspect exact _fnCallbackFire implementation =="
sed -n '6679,6825p' Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js
echo
echo "== inspect DataTable.on/event wrapper =="
sed -n '9560,9680p' Web/Resgrid.Web/wwwroot/clib/data-tables/DataTypes-1.13.5/js/jquery.dataTables.js 2>/dev/null || sed -n '9550,9680p' Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js
echo
echo "== check event source in minified 1.13.5 for dt-error vs error =="
rg -n "dt-error|error.dt|error.*dt|tn['\"]|tn\":|tn\\b" Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.min.js | head -n 80Repository: Resgrid/Core
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact err/log handling implementation =="
sed -n '6642,6689p' Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js
echo
echo "== source search for dt-error event name =="
rg -n "dt-error|\b(error)\.'|\\.error[^\"]+'\\.dt|['\"][0-9|tn]" Web/Resgrid.Web/wwwroot/clib/data-tables/DataTables-1.13.5/js/jquery.dataTables.js | head -n 240Repository: Resgrid/Core
Length of output: 16175
Scope DataTables error handling to the 401 path.
DataTable.ext.errMode = 'none' suppresses all DataTables errors in the vendored DataTables 1.x, and the error.dt handler only writes to the browser console. Let DataTables render its normal error message except when _fnAjaxUpdate calls _fnLog with a 401 response from jqXHR.status === 401; route that case through the existing redirect instead of swallowing it.
🤖 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/wwwroot/js/app/internal/resgrid.user.js` around lines 14 -
17, Update the DataTables error handling near the document error.dt listener:
remove the global $.fn.dataTable.ext.errMode = 'none' suppression and only
intercept 401 errors originating from _fnAjaxUpdate when jqXHR.status === 401,
routing them through the existing redirect behavior. Preserve DataTables’
default error rendering for all other errors and avoid relying solely on
console.warn.
| @media (min-width: 768px) { | ||
| nav.navbar-static-side { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| bottom: 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add an RTL position override for the fixed sidebar.
Line 41 fixes the sidebar to the left edge for all layouts. Existing body.rtls #page-wrapper`` uses a right-side offset, so RTL users get the content offset on the right while the fixed sidebar remains on the left. Add a body.rtls rule that resets `left` and sets `right: 0`, then regenerate `Web/Resgrid.Web/wwwroot/css/style.css`.
Proposed fix
`@media` (min-width: 768px) {
nav.navbar-static-side {
position: fixed;
top: 0;
left: 0;
bottom: 0;
}
+
+ body.rtls nav.navbar-static-side {
+ left: auto;
+ right: 0;
+ }
}📝 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.
| @media (min-width: 768px) { | |
| nav.navbar-static-side { | |
| position: fixed; | |
| top: 0; | |
| left: 0; | |
| bottom: 0; | |
| } | |
| `@media` (min-width: 768px) { | |
| nav.navbar-static-side { | |
| position: fixed; | |
| top: 0; | |
| left: 0; | |
| bottom: 0; | |
| } | |
| body.rtls nav.navbar-static-side { | |
| left: auto; | |
| right: 0; | |
| } | |
| } |
📍 Affects 2 files
Web/Resgrid.Web/wwwroot/scss/_custom.scss#L37-L43(this comment)Web/Resgrid.Web/wwwroot/css/style.css#L9963-L9969
🤖 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/wwwroot/scss/_custom.scss` around lines 37 - 43, Update the
fixed sidebar styles under the desktop media query in
Web/Resgrid.Web/wwwroot/scss/_custom.scss so body.rtls resets left and positions
the sidebar at right: 0; regenerate Web/Resgrid.Web/wwwroot/css/style.css to
include the corresponding compiled override at lines 9963-9969.
| (R(@"^(reply|respond)\s+(yes|no|acknowledge|ack)\s+to\s+(message|msg|#)?\s*#?(\d+)"), | ||
| "respond_to_message", m => P2("response", m.Groups[2].Value, "messageId", m.Groups[4].Value)), | ||
|
|
||
| // === Incident Command (ICS) board questions === |
There was a problem hiding this comment.
Unsafe type casting violates team rule 'Use safe type casting with as operator'. Use the as operator or pattern matching for safe casts and guard null results before usage across KeywordIntentClassifier.cs:168, OpenAiCompatibleNluProvider.cs:91, ChatbotModule.cs:65, ChatbotModule.cs:218, IncidentCommandActionHandler.cs:16, ChatbotResources.cs:1996, IcsPlaybooks.cs:84, IncidentBoardNarrator.cs:395, IncidentBoardNarrator.cs:464, IncidentBoardNarrator.cs:466, IncidentBoardNarrator.cs:598, IncidentBoardNarrator.cs:841, IncidentBoardNarrator.cs:1086, IncidentBoardNarrator.cs:1091, IncidentBoardNarrator.cs:1096, IncidentBoardNarrator.cs:1101, ChatbotMessageLog.cs:53, and IncidentCommandIntentClassifierTests.cs:11.
Prompt for LLM
File Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs:
Line 117:
Unsafe type casting violates team rule 'Use safe type casting with as operator'. Use the `as` operator or pattern matching for safe casts and guard null results before usage across KeywordIntentClassifier.cs:168, OpenAiCompatibleNluProvider.cs:91, ChatbotModule.cs:65, ChatbotModule.cs:218, IncidentCommandActionHandler.cs:16, ChatbotResources.cs:1996, IcsPlaybooks.cs:84, IncidentBoardNarrator.cs:395, IncidentBoardNarrator.cs:464, IncidentBoardNarrator.cs:466, IncidentBoardNarrator.cs:598, IncidentBoardNarrator.cs:841, IncidentBoardNarrator.cs:1086, IncidentBoardNarrator.cs:1091, IncidentBoardNarrator.cs:1096, IncidentBoardNarrator.cs:1101, ChatbotMessageLog.cs:53, and IncidentCommandIntentClassifierTests.cs:11.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// A <c>const</c> (not a static field) so it is available to the <c>_patterns</c> initializer. | ||
| /// </summary> | ||
| private const string RoleWords = | ||
| @"(ic|incident\s+commander|deputy(\s+incident)?(\s+commander)?|commander|unified\s+command|safety(\s+officer)?|" + |
There was a problem hiding this comment.
String concatenation using + violates team rule 'Use Template Literals Instead of String Concatenation'. Replace with template literals to improve readability and reduce errors across KeywordIntentClassifier.cs:24-31, KeywordIntentClassifier.cs:170, KeywordIntentClassifier.cs:174, ConversationalFallbackService.cs:99, IncidentBoardNarrator.cs:172, IncidentBoardNarrator.cs:198, IncidentBoardNarrator.cs:312, IncidentBoardNarrator.cs:461, IncidentBoardNarrator.cs:464, IncidentBoardNarrator.cs:522, IncidentBoardNarrator.cs:549, IncidentBoardNarrator.cs:598, IncidentBoardNarrator.cs:608, IncidentBoardNarrator.cs:623, IncidentBoardNarrator.cs:650-651, IncidentBoardNarrator.cs:705-710, IncidentBoardNarrator.cs:787, IncidentBoardNarrator.cs:872, IncidentBoardNarrator.cs:987, IncidentRoleVocabulary.cs:182-184, and resgrid.user.js:22.
Prompt for LLM
File Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs:
Line 23:
String concatenation using `+` violates team rule 'Use Template Literals Instead of String Concatenation'. Replace with template literals to improve readability and reduce errors across KeywordIntentClassifier.cs:24-31, KeywordIntentClassifier.cs:170, KeywordIntentClassifier.cs:174, ConversationalFallbackService.cs:99, IncidentBoardNarrator.cs:172, IncidentBoardNarrator.cs:198, IncidentBoardNarrator.cs:312, IncidentBoardNarrator.cs:461, IncidentBoardNarrator.cs:464, IncidentBoardNarrator.cs:522, IncidentBoardNarrator.cs:549, IncidentBoardNarrator.cs:598, IncidentBoardNarrator.cs:608, IncidentBoardNarrator.cs:623, IncidentBoardNarrator.cs:650-651, IncidentBoardNarrator.cs:705-710, IncidentBoardNarrator.cs:787, IncidentBoardNarrator.cs:872, IncidentBoardNarrator.cs:987, IncidentRoleVocabulary.cs:182-184, and resgrid.user.js:22.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| "incident_par", null), | ||
|
|
||
| // Span of control — must precede the generic resources patterns ("which lanes are over..."). | ||
| (R(@"^span(\s+of\s+control)?(\s+check)?$"), "incident_span_of_control", null), |
There was a problem hiding this comment.
Magic string "incident_span_of_control" is used as an intent label. Intent names form a finite set known at compile time; bare strings are error-prone and resist rename refactors. Define an enum such as IntentType.IncidentSpanOfControl or a constants class and reference it in every pattern tuple across KeywordIntentClassifier.cs:127, KeywordIntentClassifier.cs:129, KeywordIntentClassifier.cs:131, KeywordIntentClassifier.cs:136-148, KeywordIntentClassifier.cs:151-167, KeywordIntentClassifier.cs:171-175, KeywordIntentClassifier.cs:178-188, KeywordIntentClassifier.cs:192-194, KeywordIntentClassifier.cs:198-209, KeywordIntentClassifier.cs:212-221, IncidentBoardNarrator.cs:891, IncidentBoardNarrator.cs:894, and ChatbotIngressService.cs:645.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs:
Line 134:
Magic string "incident_span_of_control" is used as an intent label. Intent names form a finite set known at compile time; bare strings are error-prone and resist rename refactors. Define an enum such as `IntentType.IncidentSpanOfControl` or a constants class and reference it in every pattern tuple across KeywordIntentClassifier.cs:127, KeywordIntentClassifier.cs:129, KeywordIntentClassifier.cs:131, KeywordIntentClassifier.cs:136-148, KeywordIntentClassifier.cs:151-167, KeywordIntentClassifier.cs:171-175, KeywordIntentClassifier.cs:178-188, KeywordIntentClassifier.cs:192-194, KeywordIntentClassifier.cs:198-209, KeywordIntentClassifier.cs:212-221, IncidentBoardNarrator.cs:891, IncidentBoardNarrator.cs:894, and ChatbotIngressService.cs:645.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| var context = await _contextResolver.ResolveAsync(intent, session, needsAdHoc); | ||
|
|
||
| if (context.IsUnauthorized) |
There was a problem hiding this comment.
NullReferenceException risk: context returned from await _contextResolver.ResolveAsync(...) is dereferenced without a null check. If ResolveAsync returns null, accessing context.IsUnauthorized throws. Add if (context == null) return new ChatbotResponse { Text = ChatbotResources.Get("Incident_Error", culture), Processed = false }; or use pattern matching such as if (context is not { IsUnauthorized: true }).
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Core/Resgrid.Chatbot/Handlers/IncidentCommandActionHandler.cs:
Line 71:
NullReferenceException risk: `context` returned from `await _contextResolver.ResolveAsync(...)` is dereferenced without a null check. If `ResolveAsync` returns null, accessing `context.IsUnauthorized` throws. Add `if (context == null) return new ChatbotResponse { Text = ChatbotResources.Get("Incident_Error", culture), Processed = false };` or use pattern matching such as `if (context is not { IsUnauthorized: true })`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// The incident-type ICS checklist, with the items the board can already prove marked done. | ||
| /// <paramref name="incidentTypeText"/> overrides the type inferred from the call. | ||
| /// </summary> | ||
| Task<string> DescribeChecklistAsync(IncidentContext context, ChatbotSession session, string incidentTypeText); |
There was a problem hiding this comment.
Missing XML returns documentation on async Task<string> method. Async methods must document the resolved value, rejection conditions, and await usage. Add XML returns documentation for the string payload and exception documentation for expected failure cases across IIncidentBoardNarrator.cs:15-60.
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File Core/Resgrid.Chatbot/Interfaces/IIncidentBoardNarrator.cs:
Line 51:
Missing XML returns documentation on async `Task<string>` method. Async methods must document the resolved value, rejection conditions, and await usage. Add XML returns documentation for the string payload and exception documentation for expected failure cases across IIncidentBoardNarrator.cs:15-60.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public class IncidentContext | ||
| { | ||
| /// <summary>The call the command runs on. Null when nothing could be resolved.</summary> | ||
| public Call Call { get; set; } |
There was a problem hiding this comment.
Uninitialized Call property has no explicit default value. Properties must be initialized with sensible defaults to avoid null-reference surprises. Mark as nullable via public Call? Call { get; set; } or initialize via constructor to match sibling properties that already establish explicit defaults across IIncidentContextResolver.cs:18, IcsPlaybooks.cs:35, ChatbotMessageLog.cs:36, and ChatApiModels.cs:1452, ChatApiModels.cs:1483, ChatApiModels.cs:1488, ChatApiModels.cs:1520, ChatApiModels.cs:1525.
Kody rule violation: Initialize properties with default values
Prompt for LLM
File Core/Resgrid.Chatbot/Interfaces/IIncidentContextResolver.cs:
Line 15:
Uninitialized `Call` property has no explicit default value. Properties must be initialized with sensible defaults to avoid null-reference surprises. Mark as nullable via `public Call? Call { get; set; }` or initialize via constructor to match sibling properties that already establish explicit defaults across IIncidentContextResolver.cs:18, IcsPlaybooks.cs:35, ChatbotMessageLog.cs:36, and ChatApiModels.cs:1452, ChatApiModels.cs:1483, ChatApiModels.cs:1488, ChatApiModels.cs:1520, ChatApiModels.cs:1525.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| catch (System.Exception ex) | ||
| { | ||
| Logging.LogException(ex, "Chatbot conversational fallback: incident grounding failed; answering without it."); |
There was a problem hiding this comment.
Incomplete structured logging: the catch-block error log passes only a free-text message and the exception object. Error logs must include the operation name and relevant identifiers (department id, incident/call id, trace id) as structured fields. Add structured parameters such as Logging.LogException(ex, "IncidentGroundingFailed", new { op = "TryBuildIncidentSnapshot", departmentId = session?.DepartmentId, callId = session?.Context?[IncidentContextResolver.IncidentCallIdContextKey] }).
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ConversationalFallbackService.cs:
Line 140:
Incomplete structured logging: the catch-block error log passes only a free-text message and the exception object. Error logs must include the operation name and relevant identifiers (department id, incident/call id, trace id) as structured fields. Add structured parameters such as `Logging.LogException(ex, "IncidentGroundingFailed", new { op = "TryBuildIncidentSnapshot", departmentId = session?.DepartmentId, callId = session?.Context?[IncidentContextResolver.IncidentCallIdContextKey] })`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var maxTokens = Config.ChatbotConfig.CloudNluMaxTokens > 0 ? (int?)Config.ChatbotConfig.CloudNluMaxTokens : null; | ||
| var reply = await _chatCompletionClient.CompleteAsync(departmentId, SystemPrompt, | ||
| new List<ChatCompletionTurn> { new ChatCompletionTurn("user", message.Text.Trim()) }, | ||
| var reply = await _chatCompletionClient.CompleteAsync(departmentId, systemPrompt, |
There was a problem hiding this comment.
Unhandled exception risk: the call to _chatCompletionClient.CompleteAsync, which reaches an external LLM API over the network, is not wrapped in a try/catch. An LLM timeout or HTTP failure would propagate as an unhandled exception. Wrap the CompleteAsync call in try/catch, log the failure with departmentId and a trace id, and return null or a safe fallback response so the ingress pipeline degrades gracefully across IncidentBoardNarrator.cs:103, IncidentBoardNarrator.cs:485, IncidentBoardNarrator.cs:488-489, IncidentBoardNarrator.cs:531, and IncidentBoardNarrator.cs:566.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ConversationalFallbackService.cs:
Line 103:
Unhandled exception risk: the call to `_chatCompletionClient.CompleteAsync`, which reaches an external LLM API over the network, is not wrapped in a try/catch. An LLM timeout or HTTP failure would propagate as an unhandled exception. Wrap the `CompleteAsync` call in try/catch, log the failure with `departmentId` and a trace id, and return null or a safe fallback response so the ingress pipeline degrades gracefully across IncidentBoardNarrator.cs:103, IncidentBoardNarrator.cs:485, IncidentBoardNarrator.cs:488-489, IncidentBoardNarrator.cs:531, and IncidentBoardNarrator.cs:566.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var maxTokens = Config.ChatbotConfig.CloudNluMaxTokens > 0 ? (int?)Config.ChatbotConfig.CloudNluMaxTokens : null; | ||
| var reply = await _chatCompletionClient.CompleteAsync(departmentId, SystemPrompt, | ||
| new List<ChatCompletionTurn> { new ChatCompletionTurn("user", message.Text.Trim()) }, | ||
| var reply = await _chatCompletionClient.CompleteAsync(departmentId, systemPrompt, |
There was a problem hiding this comment.
Unguarded awaited operation: CompleteAsync has no error-handling guard at the call site. Every awaited async operation must be guarded so rejections are never left unhandled. Wrap the await in a try/catch, log the error, and fall back to a null reply so the caller uses the standard fallback.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ConversationalFallbackService.cs:
Line 103:
Unguarded awaited operation: `CompleteAsync` has no error-handling guard at the call site. Every awaited async operation must be guarded so rejections are never left unhandled. Wrap the await in a try/catch, log the error, and fall back to a null reply so the caller uses the standard fallback.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var score = playbook.Keywords | ||
| .Where(keyword => haystack.Contains(keyword)) | ||
| .Select(keyword => keyword.Length) | ||
| .DefaultIfEmpty(0) | ||
| .Max(); |
There was a problem hiding this comment.
Complex four-operation LINQ chain (Where → Select → DefaultIfEmpty → Max) reduces readability. Split into two steps — first filter/project the matching keyword lengths, then compute the score with a simple conditional.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs:
Line 699 to 703:
Complex four-operation LINQ chain (`Where` → `Select` → `DefaultIfEmpty` → `Max`) reduces readability. Split into two steps — first filter/project the matching keyword lengths, then compute the score with a simple conditional.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return ChatbotResources.Get("Incident_LaneNotFound", culture, laneName.Trim(), | ||
| liveNodes.Count == 0 ? ChatbotResources.Get("Incident_NoLanes", culture) : string.Join(", ", liveNodes.Select(n => n.Name))); | ||
|
|
||
| var inLane = liveAssignments.Where(a => string.Equals(a.CommandStructureNodeId, node.CommandStructureNodeId, StringComparison.OrdinalIgnoreCase)).ToList(); |
There was a problem hiding this comment.
Duplicated LINQ predicate: the 'assignments in this lane' filter is copy-pasted verbatim across IncidentBoardNarrator.cs:219, IncidentBoardNarrator.cs:252, IncidentBoardNarrator.cs:412-414, IncidentBoardNarrator.cs:427-428, IncidentBoardNarrator.cs:596, IncidentBoardNarrator.cs:622, IncidentBoardNarrator.cs:665, IncidentBoardNarrator.cs:811, and IncidentBoardNarrator.cs:836. Repeated predicates risk drift and obscure intent; factor into a reusable helper such as AssignmentsForNode(liveAssignments, node).
Kody rule violation: Extract common query logic
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs:
Line 184:
Duplicated LINQ predicate: the 'assignments in this lane' filter is copy-pasted verbatim across IncidentBoardNarrator.cs:219, IncidentBoardNarrator.cs:252, IncidentBoardNarrator.cs:412-414, IncidentBoardNarrator.cs:427-428, IncidentBoardNarrator.cs:596, IncidentBoardNarrator.cs:622, IncidentBoardNarrator.cs:665, IncidentBoardNarrator.cs:811, and IncidentBoardNarrator.cs:836. Repeated predicates risk drift and obscure intent; factor into a reusable helper such as `AssignmentsForNode(liveAssignments, node)`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| [NotMapped] public string TableName => "ChatbotMessageLog"; | ||
| [NotMapped] public string IdName => "Id"; | ||
| [NotMapped] public int IdType => 1; |
There was a problem hiding this comment.
Magic number 1 is returned for IdType without a named constant explaining its meaning. Extract the value into a named constant such as private const int StringIdType = 1; or use an enum so the reader can infer what 1 represents.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Core/Resgrid.Model/ChatbotMessageLog.cs:
Line 58:
Magic number `1` is returned for `IdType` without a named constant explaining its meaning. Extract the value into a named constant such as `private const int StringIdType = 1;` or use an enum so the reader can infer what `1` represents.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var pn = _sqlConfiguration.ParameterNotation; | ||
| var sql = DataConfig.DatabaseType == DatabaseTypes.Postgres | ||
| ? $"SELECT * FROM {_sqlConfiguration.SchemaName}.chatbotmessagelog WHERE timestamp >= {pn}SinceUtc ORDER BY timestamp DESC" | ||
| : $"SELECT * FROM {_sqlConfiguration.SchemaName}.[ChatbotMessageLog] WHERE [Timestamp] >= {pn}SinceUtc ORDER BY [Timestamp] DESC"; |
There was a problem hiding this comment.
Duplicated SQL-construction sequence (ParameterNotation lookup, DatabaseType ternary, table/column selection) repeats the identical pattern from GetUnhandledByDepartmentAsync (lines 40-43). Extract a helper such as string BuildSelectSql(string whereClause, DynamicParametersExtension dp) that centralizes the Postgres/SQL-Server branch and schema/parameter-notation logic, then call it from both methods.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ChatbotMessageLogRepository.cs:
Line 57 to 60:
Duplicated SQL-construction sequence (ParameterNotation lookup, DatabaseType ternary, table/column selection) repeats the identical pattern from `GetUnhandledByDepartmentAsync` (lines 40-43). Extract a helper such as `string BuildSelectSql(string whereClause, DynamicParametersExtension dp)` that centralizes the Postgres/SQL-Server branch and schema/parameter-notation logic, then call it from both methods.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex); | ||
| return BadRequest(new { error = "Unable to answer that right now." }); |
There was a problem hiding this comment.
Incorrect HTTP status code: AskIncident returns HTTP 400 BadRequest for a generic server-side Exception caught in its catch block. A 4xx is reserved for client errors; an unhandled server-side Exception is a server error. Return StatusCode(StatusCodes.Status500InternalServerError, new { error = "Unable to answer that right now." }) to match the server-error semantics used by the sibling IncidentSuggestions endpoint.
Kody rule violation: Use appropriate HTTP status codes
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 543:
Incorrect HTTP status code: `AskIncident` returns HTTP 400 BadRequest for a generic server-side Exception caught in its catch block. A 4xx is reserved for client errors; an unhandled server-side Exception is a server error. Return `StatusCode(StatusCodes.Status500InternalServerError, new { error = "Unable to answer that right now." })` to match the server-error semantics used by the sibling `IncidentSuggestions` endpoint.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <summary> | ||
| /// Questions worth putting in front of the commander for this incident type | ||
| /// </summary> | ||
| public List<string> Questions { get; set; } = new List<string>(); |
There was a problem hiding this comment.
Mutable List<string> exposed publicly on an API response model. API responses should expose read-only views to prevent mutation; change the type to IReadOnlyList<string> or use IReadOnlyList<string> for the getter while keeping an internal mutable backing field.
Kody rule violation: Use IReadOnlyList for immutable collections
Prompt for LLM
File Web/Resgrid.Web.Services/Models/v4/Chat/ChatApiModels.cs:
Line 1530:
Mutable `List<string>` exposed publicly on an API response model. API responses should expose read-only views to prevent mutation; change the type to `IReadOnlyList<string>` or use `IReadOnlyList<string>` for the getter while keeping an internal mutable backing field.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // send the user back to the login page instead of surfacing broken-table errors. | ||
| if ($.fn.dataTable) { | ||
| $.fn.dataTable.ext.errMode = 'none'; | ||
| $(document).on('error.dt', function (e, settings, techNote, message) { |
There was a problem hiding this comment.
Event listener leak: subscribes to the error.dt event on document with no deterministic unsubscribe path. The global handler is never unbound. Capture the handler reference and provide teardown via off('error.dt', handler) on module dispose/unload.
Kody rule violation: Provide error handlers to subscription/listener APIs
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js:
Line 15:
Event listener leak: subscribes to the `error.dt` event on document with no deterministic unsubscribe path. The global handler is never unbound. Capture the handler reference and provide teardown via `off('error.dt', handler)` on module dispose/unload.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // send the user back to the login page instead of surfacing broken-table errors. | ||
| if ($.fn.dataTable) { | ||
| $.fn.dataTable.ext.errMode = 'none'; | ||
| $(document).on('error.dt', function (e, settings, techNote, message) { |
There was a problem hiding this comment.
Anonymous document-level event listener can never be unbound. Listeners must be removed when no longer needed to avoid leaks; use a named handler reference and call $(document).off('error.dt', handler) on teardown.
Kody rule violation: Proper memory management in event listeners
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js:
Line 15:
Anonymous document-level event listener can never be unbound. Listeners must be removed when no longer needed to avoid leaks; use a named handler reference and call `$(document).off('error.dt', handler)` on teardown.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Command-board questions carry the incident the sender has open so "PAR" means "PAR on | ||
| // this board". The ingress copies it onto the session; authorization is re-checked there. | ||
| if (item.IncidentCallId.HasValue && item.IncidentCallId.Value > 0) | ||
| message.PlatformMetadata["incidentCallId"] = item.IncidentCallId.Value; |
There was a problem hiding this comment.
Magic string "incidentCallId" used as a metadata key. Shared keys/claim types must be defined once as a constant so callers and ingress stay in sync. Add a const such as private const string IncidentCallIdMetadataKey = "incidentCallId"; in a centralized constants location and use it here and in the ingress that reads it.
Kody rule violation: Centralize string constants
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/ChatbotMessageLogic.cs:
Line 61:
Magic string "incidentCallId" used as a metadata key. Shared keys/claim types must be defined once as a constant so callers and ingress stay in sync. Add a const such as `private const string IncidentCallIdMetadataKey = "incidentCallId";` in a centralized constants location and use it here and in the ingress that reads it.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
| /// A <c>const</c> (not a static field) so it is available to the <c>_patterns</c> initializer. | ||
| /// </summary> | ||
| private const string RoleWords = | ||
| @"(ic|incident\s+commander|deputy(\s+incident)?(\s+commander)?|commander|unified\s+command|safety(\s+officer)?|" + |
There was a problem hiding this comment.
String concatenation using the + operator violates the team rule requiring template literals. Found across 31 locations including KeywordIntentClassifier.cs:24-31, KeywordIntentClassifier.cs:170, KeywordIntentClassifier.cs:174, ConversationalFallbackService.cs:99, IncidentBoardNarrator.cs:172, IncidentBoardNarrator.cs:198, IncidentBoardNarrator.cs:312, IncidentBoardNarrator.cs:461, IncidentBoardNarrator.cs:464, IncidentBoardNarrator.cs:522, IncidentBoardNarrator.cs:549, IncidentBoardNarrator.cs:598, IncidentBoardNarrator.cs:608, IncidentBoardNarrator.cs:623, IncidentBoardNarrator.cs:650-651, IncidentBoardNarrator.cs:705-710, IncidentBoardNarrator.cs:787, IncidentBoardNarrator.cs:872, IncidentBoardNarrator.cs:987, IncidentRoleVocabulary.cs:182-184, and resgrid.user.js:22. Replace concatenation with template literals to improve readability and reduce error risk.
Kody rule violation: Use Template Literals Instead of String Concatenation
Prompt for LLM
File Core/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.cs:
Line 23:
String concatenation using the + operator violates the team rule requiring template literals. Found across 31 locations including KeywordIntentClassifier.cs:24-31, KeywordIntentClassifier.cs:170, KeywordIntentClassifier.cs:174, ConversationalFallbackService.cs:99, IncidentBoardNarrator.cs:172, IncidentBoardNarrator.cs:198, IncidentBoardNarrator.cs:312, IncidentBoardNarrator.cs:461, IncidentBoardNarrator.cs:464, IncidentBoardNarrator.cs:522, IncidentBoardNarrator.cs:549, IncidentBoardNarrator.cs:598, IncidentBoardNarrator.cs:608, IncidentBoardNarrator.cs:623, IncidentBoardNarrator.cs:650-651, IncidentBoardNarrator.cs:705-710, IncidentBoardNarrator.cs:787, IncidentBoardNarrator.cs:872, IncidentBoardNarrator.cs:987, IncidentRoleVocabulary.cs:182-184, and resgrid.user.js:22. Replace concatenation with template literals to improve readability and reduce error risk.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex, "Failed to write chatbot unhandled-message audit entry."); |
There was a problem hiding this comment.
Generic error log message lacks structured identifiers, making it impossible to correlate failures with the specific request that triggered it. Include structured fields mirroring the pattern used elsewhere in the file: Logging.LogException(ex, $"Failed to write chatbot unhandled-message audit entry (sessionId={session?.SessionId}, messageId={message?.MessageId}, departmentId={session?.DepartmentId})."); or pass them as structured logging parameters.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs:
Line 655:
Generic error log message lacks structured identifiers, making it impossible to correlate failures with the specific request that triggered it. Include structured fields mirroring the pattern used elsewhere in the file: `Logging.LogException(ex, $"Failed to write chatbot unhandled-message audit entry (sessionId={session?.SessionId}, messageId={message?.MessageId}, departmentId={session?.DepartmentId}).");` or pass them as structured logging parameters.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| SessionId = session?.SessionId, | ||
| Platform = (int)message.Platform, | ||
| Direction = "inbound", | ||
| MessageText = message.Text, |
There was a problem hiding this comment.
Raw user message text persisted verbatim into the ChatbotMessageLog table without PII or secret detection. Free-text chatbot input can contain emails, phone numbers, names, tokens, or other sensitive data. Run a PII/secret-detection pass (regex or library) and store a redacted or hashed variant, retaining only what is needed for coverage-gap analysis.
Kody rule violation: Mask PII and secrets in logs
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs:
Line 646:
Raw user message text persisted verbatim into the ChatbotMessageLog table without PII or secret detection. Free-text chatbot input can contain emails, phone numbers, names, tokens, or other sensitive data. Run a PII/secret-detection pass (regex or library) and store a redacted or hashed variant, retaining only what is needed for coverage-gap analysis.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| SessionId = session?.SessionId, | ||
| Platform = (int)message.Platform, | ||
| Direction = "inbound", | ||
| MessageText = message.Text, |
There was a problem hiding this comment.
GDPR Art. 5(1)(c) data-minimization violation: raw PII from user messages is written to the audit table without hashing, minimization, lawful-basis annotation, or purpose tagging. Store a tokenized or hashed representation of the text and attach purpose (e.g., feature_gap_analysis) and lawful_basis metadata to the record.
Kody rule violation: Redact PII in logs and metrics by default
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs:
Line 646:
GDPR Art. 5(1)(c) data-minimization violation: raw PII from user messages is written to the audit table without hashing, minimization, lawful-basis annotation, or purpose tagging. Store a tokenized or hashed representation of the text and attach `purpose` (e.g., feature_gap_analysis) and `lawful_basis` metadata to the record.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| UserId = session?.UserId, | ||
| SessionId = session?.SessionId, | ||
| Platform = (int)message.Platform, | ||
| Direction = "inbound", |
There was a problem hiding this comment.
Magic string "inbound" represents one value of a finite set (inbound/outbound) without a named constant or enum, making valid values undiscoverable and typo-prone. Introduce an enum (e.g., ChatbotMessageDirection.Inbound) or reference a shared constant and assign it instead of the raw string.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs:
Line 645:
Magic string "inbound" represents one value of a finite set (inbound/outbound) without a named constant or enum, making valid values undiscoverable and typo-prone. Introduce an enum (e.g., ChatbotMessageDirection.Inbound) or reference a shared constant and assign it instead of the raw string.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| public IncidentPlaybookType Type { get; set; } | ||
|
|
||
| public string DisplayName { get; set; } |
There was a problem hiding this comment.
Null reference risk: the string auto-property DisplayName lacks a default initializer, so a new IncidentPlaybook() yields a null DisplayName with no constructor enforcing initialization. Add an explicit default: public string DisplayName { get; set; } = string.Empty;, mirroring the Array.Empty<T>() defaults already used on the sibling IReadOnlyList properties.
Kody rule violation: Initialize properties with default values
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs:
Line 35:
Null reference risk: the string auto-property `DisplayName` lacks a default initializer, so a `new IncidentPlaybook()` yields a null `DisplayName` with no constructor enforcing initialization. Add an explicit default: `public string DisplayName { get; set; } = string.Empty;`, mirroring the `Array.Empty<T>()` defaults already used on the sibling `IReadOnlyList` properties.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // The Incident Commander is the command row's own field, not a role assignment. | ||
| if (role.Value == IncidentRoleType.IncidentCommander) | ||
| { | ||
| var commander = ResolveName(names, context.Command.CurrentCommanderUserId); |
There was a problem hiding this comment.
NullReferenceException risk: context.Command is dereferenced without a null check on line 459 (and line 672), yet the same method uses context.Command?.Name on line 455. Add a null check or use context.Command?.CurrentCommanderUserId with a fallback.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs:
Line 442:
NullReferenceException risk: context.Command is dereferenced without a null check on line 459 (and line 672), yet the same method uses `context.Command?.Name` on line 455. Add a null check or use `context.Command?.CurrentCommanderUserId` with a fallback.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| if (command.EstimatedEndOn.HasValue) | ||
| { | ||
| var department = await _departmentsService.GetDepartmentByIdAsync(session.DepartmentId); |
There was a problem hiding this comment.
Unhandled transient DB/network errors: the external service call to _departmentsService.GetDepartmentByIdAsync(session.DepartmentId) executes without a try/catch, so transient failures propagate unhandled. Wrap in try/catch, log with context, and degrade to a fallback (e.g., UTC times).
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs:
Line 103:
Unhandled transient DB/network errors: the external service call to `_departmentsService.GetDepartmentByIdAsync(session.DepartmentId)` executes without a try/catch, so transient failures propagate unhandled. Wrap in try/catch, log with context, and degrade to a fallback (e.g., UTC times).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var board = context.Board; | ||
| var liveAssignments = LiveAssignments(board); | ||
| var liveNodes = LiveNodes(board); | ||
| var resourceNames = await BuildResourceNameLookupAsync(context, session); |
There was a problem hiding this comment.
Redundant database query: DescribeResourcesAsync fetches department personnel profiles twice because BuildResourceNameLookupAsync (line 160) internally calls GetPersonNamesAsync, then the named-lane branch calls GetPersonNamesAsync again at line 185, doubling the GetAllProfilesForDepartmentAsync DB read for that code path. Remove the standalone GetPersonNamesAsync call at line 185 or refactor BuildResourceNameLookupAsync to return the profiles it already fetched so the lane branch can reuse them.
// BuildResourceNameLookupAsync already fetched profiles; reuse that result or have it return
// the names dictionary so the lane branch does not re-query GetAllProfilesForDepartmentAsync.Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs:
Line 160:
Redundant database query: DescribeResourcesAsync fetches department personnel profiles twice because BuildResourceNameLookupAsync (line 160) internally calls GetPersonNamesAsync, then the named-lane branch calls GetPersonNamesAsync again at line 185, doubling the GetAllProfilesForDepartmentAsync DB read for that code path. Remove the standalone GetPersonNamesAsync call at line 185 or refactor BuildResourceNameLookupAsync to return the profiles it already fetched so the lane branch can reuse them.
Suggested Code:
// BuildResourceNameLookupAsync already fetched profiles; reuse that result or have it return
// the names dictionary so the lane branch does not re-query GetAllProfilesForDepartmentAsync.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var names = await GetPersonNamesAsync(session); | ||
| var department = await _departmentsService.GetDepartmentByIdAsync(session.DepartmentId); | ||
| var resourceNames = await BuildResourceNameLookupAsync(context, session); |
There was a problem hiding this comment.
Duplicate roster loading: DescribeBriefingAsync, DescribeResourcesAsync (named-lane branch), and BuildGroundingSnapshotAsync each call GetPersonNamesAsync directly and then call BuildResourceNameLookupAsync, which internally calls GetPersonNamesAsync again, issuing the full GetAllProfilesForDepartmentAsync DB query twice per incident question. Have BuildResourceNameLookupAsync accept the already-fetched profiles as a parameter (or cache the result on the context) so each method loads the roster once.
var names = await GetPersonNamesAsync(session);
...
var resourceNames = await BuildResourceNameLookupAsync(context, session, names);Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs:
Line 565 to 567:
Duplicate roster loading: DescribeBriefingAsync, DescribeResourcesAsync (named-lane branch), and BuildGroundingSnapshotAsync each call GetPersonNamesAsync directly and then call BuildResourceNameLookupAsync, which internally calls GetPersonNamesAsync again, issuing the full GetAllProfilesForDepartmentAsync DB query twice per incident question. Have BuildResourceNameLookupAsync accept the already-fetched profiles as a parameter (or cache the result on the context) so each method loads the roster once.
Suggested Code:
var names = await GetPersonNamesAsync(session);
...
var resourceNames = await BuildResourceNameLookupAsync(context, session, names);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return ChatbotResources.Get("Incident_LaneNotFound", culture, laneName.Trim(), | ||
| liveNodes.Count == 0 ? ChatbotResources.Get("Incident_NoLanes", culture) : string.Join(", ", liveNodes.Select(n => n.Name))); | ||
|
|
||
| var inLane = liveAssignments.Where(a => string.Equals(a.CommandStructureNodeId, node.CommandStructureNodeId, StringComparison.OrdinalIgnoreCase)).ToList(); |
There was a problem hiding this comment.
Duplicated LINQ predicate filtering assignments by node ID at lines 184, 219, 252, 622, and 811 invites drift and is hard to maintain. Extract a reusable method such as AssignmentsForNode(liveAssignments, node) or an Expression<ResourceAssignment, bool>.
Kody rule violation: Extract common query logic
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs:
Line 184:
Duplicated LINQ predicate filtering assignments by node ID at lines 184, 219, 252, 622, and 811 invites drift and is hard to maintain. Extract a reusable method such as `AssignmentsForNode(liveAssignments, node)` or an `Expression<ResourceAssignment, bool>`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var culture = session?.Culture; | ||
| var names = await GetPersonNamesAsync(session); | ||
| var active = (context.Board.Roles ?? new List<IncidentRoleAssignment>()) |
There was a problem hiding this comment.
Duplicated active-roles filter pattern (Board.Roles ?? new List<IncidentRoleAssignment>(), Where Not RemovedOn, ToList) at lines 412, 596, 665, and 836. Extract a method such as LiveRoles(board) that encapsulates the null-coalesce and active filter.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs:
Line 412:
Duplicated active-roles filter pattern (`Board.Roles ?? new List<IncidentRoleAssignment>()`, Where Not RemovedOn, ToList) at lines 412, 596, 665, and 836. Extract a method such as `LiveRoles(board)` that encapsulates the null-coalesce and active filter.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Several in play — surface only the ones the caller may actually see. | ||
| foreach (var command in candidates) | ||
| { | ||
| var call = await _callsService.GetCallByIdAsync(command.CallId); |
There was a problem hiding this comment.
N+1 query: GetCallByIdAsync is called once per command inside a foreach loop, issuing a separate database round-trip per iteration. Collect all CallIds and use a batch query (e.g., GetCallsByIdsAsync) or a single filtered query before the loop.
Kody rule violation: Optimize database queries with JOINs
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentContextResolver.cs:
Line 98:
N+1 query: GetCallByIdAsync is called once per command inside a foreach loop, issuing a separate database round-trip per iteration. Collect all CallIds and use a batch query (e.g., GetCallsByIdsAsync) or a single filtered query before the loop.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (call == null || call.DepartmentId != departmentId) | ||
| continue; | ||
|
|
||
| if (await _authorizationService.CanUserViewCallAsync(session.UserId, call.CallId)) |
There was a problem hiding this comment.
N+1 pattern: sequential per-item authorization checks via _authorizationService.CanUserViewCallAsync inside a foreach loop are slow. Batch authorization checks or pre-filter candidates by permission in a single query.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IncidentContextResolver.cs:
Line 102:
N+1 pattern: sequential per-item authorization checks via `_authorizationService.CanUserViewCallAsync` inside a foreach loop are slow. Batch authorization checks or pre-filter candidates by permission in a single query.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| [NotMapped] public string TableName => "ChatbotMessageLog"; | ||
| [NotMapped] public string IdName => "Id"; | ||
| [NotMapped] public int IdType => 1; |
There was a problem hiding this comment.
Magic number '1' in IdType => 1 lacks a named constant or explanation. Numeric literals with meaning should be extracted into named constants or readonly fields. Define a named constant (e.g., private const int StringIdType = 1;) or use an existing enum and reference it as public int IdType => StringIdType;.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Core/Resgrid.Model/ChatbotMessageLog.cs:
Line 58:
Magic number '1' in `IdType => 1` lacks a named constant or explanation. Numeric literals with meaning should be extracted into named constants or readonly fields. Define a named constant (e.g., `private const int StringIdType = 1;`) or use an existing enum and reference it as `public int IdType => StringIdType;`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var result = await _classifier.ClassifyAsync("checklist for a structure fire"); | ||
|
|
||
| result.Parameters["incidentType"].Should().Be("structure fire"); |
There was a problem hiding this comment.
Unvalidated dictionary access: result.Parameters["incidentType"] is indexed without checking key existence, violating the rule requiring collection bounds validation before indexing. Check result.Parameters.ContainsKey("incidentType") or use TryGetValue before accessing the key.
Kody rule violation: Check query results before accessing indices
Prompt for LLM
File Tests/Resgrid.Tests/Chatbot/IncidentCommandIntentClassifierTests.cs:
Line 145:
Unvalidated dictionary access: result.Parameters["incidentType"] is indexed without checking key existence, violating the rule requiring collection bounds validation before indexing. Check `result.Parameters.ContainsKey("incidentType")` or use `TryGetValue` before accessing the key.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<IncidentAssistantAnswerResult>> AskIncident([FromBody] AskIncidentAssistantInput input) | ||
| { | ||
| if (!await ChatbotChatEnabledAsync()) |
There was a problem hiding this comment.
Unguarded feature-flag check: the awaited ChatbotChatEnabledAsync() call sits outside the try/catch block (try begins at line 507), but it internally awaits feature-toggle and department-config services that could throw without structured error context. Move the feature-flag check inside the try block or wrap it in its own try/catch that logs with context and returns a 500.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 501:
Unguarded feature-flag check: the awaited ChatbotChatEnabledAsync() call sits outside the try/catch block (try begins at line 507), but it internally awaits feature-toggle and department-config services that could throw without structured error context. Move the feature-flag check inside the try block or wrap it in its own try/catch that logs with context and returns a 500.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<IncidentAssistantAnswerResult>> AskIncident([FromBody] AskIncidentAssistantInput input) | ||
| { | ||
| if (!await ChatbotChatEnabledAsync()) |
There was a problem hiding this comment.
Incorrect validation ordering: the feature-flag check on line 501 triggers database queries via _featureToggleService and _departmentConfigService before input validation on line 504. Invalid input (null or empty question) should be rejected before spending resources on DB lookups. Move the input null/empty check above the ChatbotChatEnabledAsync() call.
Kody rule violation: Order validations before database queries
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 501:
Incorrect validation ordering: the feature-flag check on line 501 triggers database queries via _featureToggleService and _departmentConfigService before input validation on line 504. Invalid input (null or empty question) should be rejected before spending resources on DB lookups. Move the input null/empty check above the ChatbotChatEnabledAsync() call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex); | ||
| return BadRequest(new { error = "Unable to answer that right now." }); |
There was a problem hiding this comment.
Incorrect HTTP status: a caught generic Exception returns HTTP 400 BadRequest, but a server-side processing failure is a 5xx server error, not a 4xx client error. Return StatusCode(StatusCodes.Status500InternalServerError, new { error = "An unexpected error occurred." }) to match the pattern in IncidentSuggestions (line 593).
Kody rule violation: Use appropriate HTTP status codes
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 543:
Incorrect HTTP status: a caught generic Exception returns HTTP 400 BadRequest, but a server-side processing failure is a 5xx server error, not a 4xx client error. Return `StatusCode(StatusCodes.Status500InternalServerError, new { error = "An unexpected error occurred." })` to match the pattern in IncidentSuggestions (line 593).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Command-board questions carry the incident the sender has open so "PAR" means "PAR on | ||
| // this board". The ingress copies it onto the session; authorization is re-checked there. | ||
| if (item.IncidentCallId.HasValue && item.IncidentCallId.Value > 0) | ||
| message.PlatformMetadata["incidentCallId"] = item.IncidentCallId.Value; |
There was a problem hiding this comment.
Inline string literal "incidentCallId" used as a metadata key violates the rule requiring shared string literals to be centralized as constants. Declare a shared constant such as public const string IncidentCallIdMetadataKey = "incidentCallId"; in a common metadata-keys class and reference it to keep the contract between the ingress service and producer in one place.
Kody rule violation: Centralize string constants
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/ChatbotMessageLogic.cs:
Line 61:
Inline string literal "incidentCallId" used as a metadata key violates the rule requiring shared string literals to be centralized as constants. Declare a shared constant such as `public const string IncidentCallIdMetadataKey = "incidentCallId";` in a common metadata-keys class and reference it to keep the contract between the ingress service and producer in one place.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs (1)
661-664: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the explicit general playbook.
Resolve("Incident")returnsnullbecause this loop excludesGeneralPlaybook.DescribeChecklistAsyncthen infers a type-specific playbook from the call, despite the explicit general selection. CheckGeneralPlaybook.DisplayNamebefore this loop.Proposed fix
+ if (string.Equals(GeneralPlaybook.DisplayName, needle, StringComparison.OrdinalIgnoreCase)) + return GeneralPlaybook; + foreach (var playbook in Playbooks)🤖 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/IcsPlaybooks.cs` around lines 661 - 664, Update Resolve to check GeneralPlaybook.DisplayName against needle with the same case-insensitive comparison before iterating Playbooks, returning GeneralPlaybook on a match; preserve the existing Playbooks matching logic for all other names.
🧹 Nitpick comments (1)
Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs (1)
31-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftProtect the shared playbook catalog from mutation.
All,Get,Resolve, andInferreturn the staticIncidentPlaybookinstances. Public setters let any caller change guidance for later requests. Make the playbook properties init-only or otherwise immutable before exposing them. Verify that no external assembly mutates or constructsIncidentPlaybookbefore tightening this public contract.As per coding guidelines, “Prefer functional patterns and immutable data where appropriate in C#.”
Also applies to: 639-648
🤖 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/IcsPlaybooks.cs` around lines 31 - 50, Protect the shared IncidentPlaybook catalog by auditing usages of IncidentPlaybook and its catalog methods All, Get, Resolve, and Infer for external construction or mutation. Then replace the public setters on Type, DisplayName, Keywords, Benchmarks, Checklist, KeyRoles, and SuggestedQuestions with init-only or otherwise immutable members, preserving existing catalog initialization and preventing changes after publication.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs`:
- Around line 661-664: Update Resolve to check GeneralPlaybook.DisplayName
against needle with the same case-insensitive comparison before iterating
Playbooks, returning GeneralPlaybook on a match; preserve the existing Playbooks
matching logic for all other names.
---
Nitpick comments:
In `@Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs`:
- Around line 31-50: Protect the shared IncidentPlaybook catalog by auditing
usages of IncidentPlaybook and its catalog methods All, Get, Resolve, and Infer
for external construction or mutation. Then replace the public setters on Type,
DisplayName, Keywords, Benchmarks, Checklist, KeyRoles, and SuggestedQuestions
with init-only or otherwise immutable members, preserving existing catalog
initialization and preventing changes after publication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 613e9b30-9e14-4416-8cc1-ab9236af510a
📒 Files selected for processing (7)
Core/Resgrid.Chatbot/Handlers/IncidentCommandActionHandler.csCore/Resgrid.Chatbot/Interfaces/IIncidentContextResolver.csCore/Resgrid.Chatbot/Services/IcsPlaybooks.csCore/Resgrid.Chatbot/Services/IncidentBoardNarrator.csCore/Resgrid.Chatbot/Services/IncidentContextResolver.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js
🚧 Files skipped from review as they are similar to previous changes (4)
- Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js
- Core/Resgrid.Chatbot/Services/IncidentContextResolver.cs
- Core/Resgrid.Chatbot/Handlers/IncidentCommandActionHandler.cs
- Core/Resgrid.Chatbot/Services/IncidentBoardNarrator.cs
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| var score = playbook.Keywords | ||
| .Where(keyword => ContainsPhrase(needle, keyword)) | ||
| .Select(keyword => keyword.Length) | ||
| .DefaultIfEmpty(0) | ||
| .Max(); |
There was a problem hiding this comment.
Overly long LINQ chain (Where → Select → DefaultIfEmpty → Max) in IcsPlaybooks.cs reduces readability and debuggability per rule [12]. Break it into intermediate steps: compute matching keywords first, then select lengths, then compute the max.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs:
Line 674 to 678:
Overly long LINQ chain (Where → Select → DefaultIfEmpty → Max) in IcsPlaybooks.cs reduces readability and debuggability per rule [12]. Break it into intermediate steps: compute matching keywords first, then select lengths, then compute the max.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Same longest-keyword-wins scoring as Infer, so "vehicle fire" resolves to the vehicle | ||
| // playbook even though a fire playbook appears earlier in the list. | ||
| IncidentPlaybook best = null; |
There was a problem hiding this comment.
Duplicate best-keyword-scoring loop in Resolve copies the foreach-LINQ-update-return sequence from Infer, violating rule [14]. Extract a shared helper like private static IncidentPlaybook FindBestPlaybook(string text) and call it from both Resolve and Infer.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Core/Resgrid.Chatbot/Services/IcsPlaybooks.cs:
Line 669:
Duplicate best-keyword-scoring loop in Resolve copies the foreach-LINQ-update-return sequence from Infer, violating rule [14]. Extract a shared helper like `private static IncidentPlaybook FindBestPlaybook(string text)` and call it from both Resolve and Infer.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if ($.fn.dataTable) { | ||
| $.fn.dataTable.ext.errMode = function (settings, techNote, message) { | ||
| if (settings && settings.jqXHR && settings.jqXHR.status === 401) { | ||
| if (window.console && console.warn) { console.warn(message); } |
There was a problem hiding this comment.
console.warn(message) in resgrid.user.js logs only the raw DataTables message string, violating rule [3]'s structured logging requirement. Log with context such as console.warn('dataTable.error', { techNote, status: settings?.jqXHR?.status, message }); or route through a structured logger.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js:
Line 17:
`console.warn(message)` in resgrid.user.js logs only the raw DataTables message string, violating rule [3]'s structured logging requirement. Log with context such as `console.warn('dataTable.error', { techNote, status: settings?.jqXHR?.status, message });` or route through a structured logger.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Every other DataTables error keeps the library's default alert so real problems surface. | ||
| if ($.fn.dataTable) { | ||
| $.fn.dataTable.ext.errMode = function (settings, techNote, message) { | ||
| if (settings && settings.jqXHR && settings.jqXHR.status === 401) { |
There was a problem hiding this comment.
Magic number 401 representing HTTP Unauthorized in resgrid.user.js hurts readability and invites typos. Define a named constant (e.g., const HTTP_UNAUTHORIZED = 401;) or reuse an existing HttpStatus enum and compare against it.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/resgrid.user.js:
Line 16:
Magic number `401` representing HTTP Unauthorized in resgrid.user.js hurts readability and invites typos. Define a named constant (e.g., `const HTTP_UNAUTHORIZED = 401;`) or reuse an existing HttpStatus enum and compare against it.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
3 participants
Pull Request Description
Add Incident Command (ICS) Assistant to the Chatbot
This PR introduces a major new capability: the Resgrid chatbot can now answer the operational questions an Incident Commander asks while working a command board, and includes several supporting fixes and improvements.
Incident Command Assistant
New ICS question types (13 new intents): The chatbot now understands and answers natural-language questions about an active incident, including:
Key components added:
IncidentContextResolver— resolves which incident a question is about (explicit call reference → client-open incident → department active commands), with authorization checks and ambiguity disambiguationIncidentBoardNarrator— turns the loaded command board into readable, radio-friendly answersIcsPlaybooks— static NIMS/ICS knowledge tables for 12 incident families (structure fire, wildland, MVA, EMS, MCI, HazMat, natural disaster, SAR, technical rescue, water rescue, active threat, general), with benchmarks, checklists, key roles, and suggested questionsIncidentRoleVocabulary— maps radio shorthand ("ops", "PIO", "safety", "RIT") to ICS positions with longest-match-first resolutionLLM grounding: The conversational fallback now grounds free-form questions on a factual snapshot of the incident board when a command board is open, so questions that don't match a structured intent are still answered from real data rather than invented.
New API endpoints:
POST /v4/Chatbot/AskIncident— synchronous question-and-answer for the command board UIGET /v4/Chatbot/IncidentSuggestions— incident-type-tailored suggested questions for the board UIUnhandled Message Auditing
Added
ChatbotMessageLogpersistence to track messages the chatbot couldn't handle with a structured intent (no match, fallback answered/failed, or pipeline error), enabling per-department and system-wide feature-gap analysis.Security Fix: Chat Metadata URL Validation
Fixed GIF and link URL validation to handle the nested envelope format clients actually send (
{"gif":{"url":...}},{"link":{"url":...}}) in addition to the legacy flat format. Previously, reading only the root-level URL meant every real GIF payload bypassed the CDN allowlist entirely. GIF preview URLs are now also validated against the allowlist.Web UI Fixes
Other Changes
ChatbotChatMessageRequestnow accepts an optionalCallIdto carry incident context from the web chatSendChatMessageInput.Bodynow allows empty strings (image messages send empty body as caption is optional)ChatbotMessageQueueItemextended withIncidentCallIdfor incident context through the queue pathSummary by CodeRabbit
New Features
Bug Fixes