Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughThe change adds department-admin channel access, feature-gated chat routes, retryable channel loading, footer-based chat and assistant panels, provider registrations, fail-closed feature evaluation, and TTS temporary-directory validation. ChangesChat integration
TTS health validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Apps/src/components/chat/useChatBootstrap.ts (1)
38-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep valid channels visible when a refresh fails.
At Line 51, every rejected request sets
channelsLoadFailedtotrue.ChannelListchecks this flag before rendering existing channels. A transient refresh failure therefore replaces valid conversations with the retry view.Overlapping
reloadcalls can also let an older rejection overwrite a newer successful result. Track the latest request and ignore stale outcomes. SetchannelsLoadFailedonly when the latest initial load fails and no valid channel list exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/useChatBootstrap.ts` around lines 38 - 53, Update reload in useChatBootstrap to track and validate the latest request before applying success or failure results, ignoring stale outcomes from overlapping calls. On the latest failure, set channelsLoadFailed only when the initial load has no valid existing channel list; otherwise preserve the current channels and chat state while exposing the retryable error appropriately.
🧹 Nitpick comments (1)
Web/Resgrid.Web/Areas/User/Controllers/ChatController.cs (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the service-locator pattern for the new feature-toggle dependency.
Both controllers add
IFeatureToggleServicethrough constructor injection, which violates the shared C# dependency-resolution rule.
Web/Resgrid.Web/Areas/User/Controllers/ChatController.cs#L13-L18: resolveIFeatureToggleServicewithBootstrapper.GetKernel().Resolve<IFeatureToggleService>().Web/Resgrid.Web/Areas/User/Controllers/ModerationController.cs#L14-L20: resolve the new feature-toggle dependency the same way without extending the constructor contract.As per coding guidelines,
**/*.cs: UseService 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/Areas/User/Controllers/ChatController.cs` around lines 13 - 18, Replace constructor injection of IFeatureToggleService in Web/Resgrid.Web/Areas/User/Controllers/ChatController.cs lines 13-18 with Bootstrapper.GetKernel().Resolve<IFeatureToggleService>(), removing it from the constructor contract. Apply the same change to the new feature-toggle dependency in Web/Resgrid.Web/Areas/User/Controllers/ModerationController.cs lines 14-20, resolving it through the service locator without extending the constructor.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/ChatChannelService.cs`:
- Around line 87-110: The department-admin branch in the channel-list loading
method must not synchronously call EnsureGroupChannelAsync for every group.
Bulk-load existing group channels first, then provision missing defaults through
a separate bounded bulk path with explicit per-group failure handling so one
failure cannot abort the complete administrator result; preserve adding
successfully resolved channels to results.
In `@Web/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.cs`:
- Around line 76-79: Update the exception handler in the TTS dependency health
check to call Resgrid.Framework.Logging.LogException with the caught ex before
adding the validation error. Preserve the existing validationErrors message and
use the logger’s optional extra message only if needed for context.
- Around line 66-78: The TTS health check reports the configured value instead
of the resolved directory being probed. In the health-check method containing
tempRoot and the probe file operations, compute and retain the selected temp
path before the try block, then use that resolved path in validationErrors when
reporting failures, including the Path.GetTempPath() fallback.
- Around line 70-74: Update the probe in TtsDependencyHealthCheck to create a
unique child directory beneath tempRoot, write the probe file inside that
directory, and remove the child directory in a finally block. Keep cleanup
reliable even when file creation or deletion fails, and preserve the existing
probe behavior while exercising the same child-directory operation used by
AudioProcessingService.
In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPanelElement.tsx`:
- Around line 37-45: Update the ChatPanelElement visibility and render guards
around chatReady, hostElement, and the panel’s early return so loadFailed keeps
the footer host visible and allows the failure view with the ChannelList retry
control to render. Continue hiding and returning null only when chat is
confirmed unavailable, while preserving the normal loaded-and-available
behavior.
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/useChatBootstrap.ts`:
- Around line 38-53: Update reload in useChatBootstrap to track and validate the
latest request before applying success or failure results, ignoring stale
outcomes from overlapping calls. On the latest failure, set channelsLoadFailed
only when the initial load has no valid existing channel list; otherwise
preserve the current channels and chat state while exposing the retryable error
appropriately.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/ChatController.cs`:
- Around line 13-18: Replace constructor injection of IFeatureToggleService in
Web/Resgrid.Web/Areas/User/Controllers/ChatController.cs lines 13-18 with
Bootstrapper.GetKernel().Resolve<IFeatureToggleService>(), removing it from the
constructor contract. Apply the same change to the new feature-toggle dependency
in Web/Resgrid.Web/Areas/User/Controllers/ModerationController.cs lines 14-20,
resolving it through the service locator without extending the constructor.
🪄 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: 74ff43c2-939b-4a95-bd22-bacf82af046d
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Common.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.uk.resxis excluded by!**/*.resxTests/Resgrid.Tests/Services/ChatChannelServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (21)
Core/Resgrid.Model/Services/IChatServices.csCore/Resgrid.Services/ChatChannelService.csCore/Resgrid.Services/ChatPermissionService.csWeb/Resgrid.Web.Eventing/Resgrid.Web.Eventing.csprojWeb/Resgrid.Web.Eventing/Startup.csWeb/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.csWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/AssistantPanelElement.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ChannelList.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPageElement.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPanelElement.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chat.cssWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chatStore.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/useChatBootstrap.tsWeb/Resgrid.Web/Areas/User/Apps/src/elements.tsWeb/Resgrid.Web/Areas/User/Controllers/ChatController.csWeb/Resgrid.Web/Areas/User/Controllers/ModerationController.csWeb/Resgrid.Web/Areas/User/Views/Chat/Chatbot.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_Footer.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_Navigation.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_UserLayout.cshtml
💤 Files with no reviewable changes (2)
- Web/Resgrid.Web/Areas/User/Views/Chat/Chatbot.cshtml
- Web/Resgrid.Web/Areas/User/Views/Shared/_UserLayout.cshtml
| var tempRoot = Path.GetFullPath(string.IsNullOrWhiteSpace(_ttsOptions.TempDirectory) | ||
| ? Path.GetTempPath() | ||
| : _ttsOptions.TempDirectory); | ||
|
|
||
| Directory.CreateDirectory(tempRoot); | ||
|
|
||
| var probeFilePath = Path.Combine(tempRoot, $"health-probe-{Guid.NewGuid():N}.tmp"); | ||
| File.WriteAllBytes(probeFilePath, new byte[] { 1 }); | ||
| File.Delete(probeFilePath); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| validationErrors.Add($"The TTS temp directory '{_ttsOptions.TempDirectory}' is not writable: {ex.Message}"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report the path that the probe actually tests.
Lines 66-68 use Path.GetTempPath() when _ttsOptions.TempDirectory is blank. Line 78 reports _ttsOptions.TempDirectory instead. A failed fallback probe can therefore report an empty or misleading path. Store the selected path before the try block and use it in the validation error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.cs` around lines 66 - 78,
The TTS health check reports the configured value instead of the resolved
directory being probed. In the health-check method containing tempRoot and the
probe file operations, compute and retain the selected temp path before the try
block, then use that resolved path in validationErrors when reporting failures,
including the Path.GetTempPath() fallback.
| catch (Exception ex) | ||
| { | ||
| validationErrors.Add($"The TTS temp directory '{_ttsOptions.TempDirectory}' is not writable: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Log the caught exception with the repository logger.
This catch block records only ex.Message. It drops the stack trace and caller metadata needed to diagnose filesystem failures. Call Resgrid.Framework.Logging.LogException(ex, ...) before adding the health validation error.
As per coding guidelines, use Resgrid.Framework.Logging.LogException(Exception ex, string extraMessage = null, string correlationId = null) when catching exceptions.
🤖 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.Tts/Health/TtsDependencyHealthCheck.cs` around lines 76 - 79,
Update the exception handler in the TTS dependency health check to call
Resgrid.Framework.Logging.LogException with the caught ex before adding the
validation error. Preserve the existing validationErrors message and use the
logger’s optional extra message only if needed for context.
Source: Coding guidelines
| if (await _chatPermissionService.IsDepartmentAdminAsync(departmentId, userId)) | ||
| { | ||
| var groupChannel = await EnsureGroupChannelAsync(group); | ||
| if (groupChannel != null) | ||
| results[groupChannel.ChatChannelId] = groupChannel; | ||
| var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId); | ||
| if (allGroups != null) | ||
| { | ||
| foreach (var departmentGroup in allGroups) | ||
| { | ||
| var groupChannel = await EnsureGroupChannelAsync(departmentGroup); | ||
| if (groupChannel != null) | ||
| results[groupChannel.ChatChannelId] = groupChannel; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
N+1 query issue in GetChannelsForUserAsync causes the department-admin branch to await EnsureGroupChannelAsync once per group, resulting in a GetByGroupIdAsync database round-trip for each group. Batch fetch all of the department's existing GroupDefault channels in a single query, similar to the EnsureLaneChannelsAsync pattern, and provision only the missing groups.
// Fetch all existing GroupDefault channels for the department in one query instead of one
// GetByGroupIdAsync round-trip per group (the per-group EnsureGroupChannelAsync N+1).
if (await _chatPermissionService.IsDepartmentAdminAsync(departmentId, userId))
{
var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId);
if (allGroups != null && allGroups.Count > 0)
{
var groupIds = allGroups.Select(g => g.DepartmentGroupId).ToHashSet();
var existingGroupChannels = await _chatChannelRepository.GetByGroupIdsAsync(groupIds);
var byGroupId = (existingGroupChannels ?? Enumerable.Empty<ChatChannel>())
.Where(c => c.ChannelType == (int)ChatChannelType.GroupDefault)
.ToDictionary(c => c.GroupId);
foreach (var departmentGroup in allGroups)
{
if (byGroupId.TryGetValue(departmentGroup.DepartmentGroupId, out var existing) && existing != null)
{
results[existing.ChatChannelId] = existing;
}
else
{
var groupChannel = await EnsureGroupChannelAsync(departmentGroup);
if (groupChannel != null)
results[groupChannel.ChatChannelId] = groupChannel;
}
}
}
}Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 89 to 101:
N+1 query issue in `GetChannelsForUserAsync` causes the department-admin branch to await `EnsureGroupChannelAsync` once per group, resulting in a `GetByGroupIdAsync` database round-trip for each group. Batch fetch all of the department's existing `GroupDefault` channels in a single query, similar to the `EnsureLaneChannelsAsync` pattern, and provision only the missing groups.
Suggested Code:
// Fetch all existing GroupDefault channels for the department in one query instead of one
// GetByGroupIdAsync round-trip per group (the per-group EnsureGroupChannelAsync N+1).
if (await _chatPermissionService.IsDepartmentAdminAsync(departmentId, userId))
{
var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId);
if (allGroups != null && allGroups.Count > 0)
{
var groupIds = allGroups.Select(g => g.DepartmentGroupId).ToHashSet();
var existingGroupChannels = await _chatChannelRepository.GetByGroupIdsAsync(groupIds);
var byGroupId = (existingGroupChannels ?? Enumerable.Empty<ChatChannel>())
.Where(c => c.ChannelType == (int)ChatChannelType.GroupDefault)
.ToDictionary(c => c.GroupId);
foreach (var departmentGroup in allGroups)
{
if (byGroupId.TryGetValue(departmentGroup.DepartmentGroupId, out var existing) && existing != null)
{
results[existing.ChatChannelId] = existing;
}
else
{
var groupChannel = await EnsureGroupChannelAsync(departmentGroup);
if (groupChannel != null)
results[groupChannel.ChatChannelId] = groupChannel;
}
}
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| foreach (var departmentGroup in allGroups) | ||
| { | ||
| var groupChannel = await EnsureGroupChannelAsync(departmentGroup); |
There was a problem hiding this comment.
N+1 call pattern occurs because EnsureGroupChannelAsync is awaited inside a foreach loop, degrading performance as department groups grow. Batch the provisioning using a bulk method for a single round-trip, or use Task.WhenAll over the collection instead of sequential awaits.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 96:
N+1 call pattern occurs because `EnsureGroupChannelAsync` is awaited inside a `foreach` loop, degrading performance as department groups grow. Batch the provisioning using a bulk method for a single round-trip, or use `Task.WhenAll` over the collection instead of sequential awaits.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (await _chatPermissionService.IsDepartmentAdminAsync(departmentId, userId)) | ||
| { | ||
| var groupChannel = await EnsureGroupChannelAsync(group); | ||
| if (groupChannel != null) | ||
| results[groupChannel.ChatChannelId] = groupChannel; | ||
| var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId); | ||
| if (allGroups != null) | ||
| { | ||
| foreach (var departmentGroup in allGroups) | ||
| { | ||
| var groupChannel = await EnsureGroupChannelAsync(departmentGroup); | ||
| if (groupChannel != null) | ||
| results[groupChannel.ChatChannelId] = groupChannel; | ||
| } | ||
| } |
There was a problem hiding this comment.
Query amplification in GetChannelsForUserAsync causes the department-admin branch to loop over all department groups calling EnsureGroupChannelAsync, which issues a separate GetByGroupIdAsync database query per group. Add a batch repository method like GetExistingGroupChannelsByDepartmentAsync to fetch all existing group-default channels in one query, matching them by GroupId in memory and only calling EnsureGroupChannelAsync for missing channels.
// Fetch all existing group-default channels for this department in a single query.
var existingGroupChannels = await _chatChannelRepository.GetAllGroupChannelsByDepartmentAsync(departmentId);
var channelByGroupId = existingGroupChannels?.ToDictionary(c => c.GroupId.GetValueOrDefault()) ?? new Dictionary<int, ChatChannel>();
var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId);
if (allGroups != null)
{
foreach (var departmentGroup in allGroups)
{
if (channelByGroupId.TryGetValue(departmentGroup.DepartmentGroupId, out var existing) && existing != null)
{
results[existing.ChatChannelId] = existing;
continue;
}
// Only provision (query + insert) for groups that don't have a channel yet.
var groupChannel = await EnsureGroupChannelAsync(departmentGroup);
if (groupChannel != null)
results[groupChannel.ChatChannelId] = groupChannel;
}
}Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 89 to 100:
Query amplification in `GetChannelsForUserAsync` causes the department-admin branch to loop over all department groups calling `EnsureGroupChannelAsync`, which issues a separate `GetByGroupIdAsync` database query per group. Add a batch repository method like `GetExistingGroupChannelsByDepartmentAsync` to fetch all existing group-default channels in one query, matching them by `GroupId` in memory and only calling `EnsureGroupChannelAsync` for missing channels.
Suggested Code:
// Fetch all existing group-default channels for this department in a single query.
var existingGroupChannels = await _chatChannelRepository.GetAllGroupChannelsByDepartmentAsync(departmentId);
var channelByGroupId = existingGroupChannels?.ToDictionary(c => c.GroupId.GetValueOrDefault()) ?? new Dictionary<int, ChatChannel>();
var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId);
if (allGroups != null)
{
foreach (var departmentGroup in allGroups)
{
if (channelByGroupId.TryGetValue(departmentGroup.DepartmentGroupId, out var existing) && existing != null)
{
results[existing.ChatChannelId] = existing;
continue;
}
// Only provision (query + insert) for groups that don't have a channel yet.
var groupChannel = await EnsureGroupChannelAsync(departmentGroup);
if (groupChannel != null)
results[groupChannel.ChatChannelId] = groupChannel;
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var groupChannel = await EnsureGroupChannelAsync(group); | ||
| if (groupChannel != null) | ||
| results[groupChannel.ChatChannelId] = groupChannel; |
There was a problem hiding this comment.
Code duplication increases maintenance burden because the await EnsureGroupChannelAsync(group); if (channel != null) results[...] = channel sequence is repeated at lines 96-98 and 107-109. Extract a helper like TryAddGroupChannelAsync and call it from both the admin and non-admin branches.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 107 to 109:
Code duplication increases maintenance burden because the `await EnsureGroupChannelAsync(group); if (channel != null) results[...] = channel` sequence is repeated at lines 96-98 and 107-109. Extract a helper like `TryAddGroupChannelAsync` and call it from both the admin and non-admin branches.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
|
|
||
| private async Task<bool> IsDepartmentAdminAsync(int departmentId, string userId) | ||
| public async Task<bool> IsDepartmentAdminAsync(int departmentId, string userId) |
There was a problem hiding this comment.
Overly broad access modifier widens the API surface unnecessarily by changing IsDepartmentAdminAsync from private to public. Use internal if only another service in the same assembly requires access, or expose it via a higher-level public API.
Kody rule violation: Use private access modifiers for encapsulation
Prompt for LLM
File Core/Resgrid.Services/ChatPermissionService.cs:
Line 685:
Overly broad access modifier widens the API surface unnecessarily by changing `IsDepartmentAdminAsync` from `private` to `public`. Use `internal` if only another service in the same assembly requires access, or expose it via a higher-level public API.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| new DepartmentGroup { DepartmentGroupId = 9, DepartmentId = 1, Name = "Station 1" }, | ||
| new DepartmentGroup { DepartmentGroupId = 10, DepartmentId = 1, Name = "Station 2" } | ||
| }); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetByGroupIdAsync(It.IsAny<int>())).ReturnsAsync((ChatChannel)null); |
There was a problem hiding this comment.
Type casting violation requires using the as operator or pattern matching for safe casts and guarding null results before usage.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Tests/Resgrid.Tests/Services/ChatChannelServiceTests.cs:
Line 554:
Type casting violation requires using the `as` operator or pattern matching for safe casts and guarding null results before usage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| return ( | ||
| <> | ||
| <button type="button" className="rgchat-root rgchat-footerbtn rgchat-footerbtn--assistant" aria-label={label} onClick={() => setOpen(true)}> |
There was a problem hiding this comment.
JSX prop violation causes inline arrow functions to create new functions on every render, negatively impacting performance. Move function definitions outside the render method.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/AssistantPanelElement.tsx:
Line 49:
JSX prop violation causes inline arrow functions to create new functions on every render, negatively impacting performance. Move function definitions outside the render method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Footer button + right-hand slide-out drawer hosting the assistant conversation. The assistant | ||
| // is intentionally not a standalone page: the drawer overlays whatever the user is working on. | ||
| export default function AssistantPanelElement({ hostElement, label = 'Assistant', closeLabel = 'Close' }: AssistantPanelElementProps) { |
There was a problem hiding this comment.
Default export reduces clarity and complicates IDE auto-imports. Change export default function AssistantPanelElement to a named export like export function AssistantPanelElement and update all import sites.
Kody rule violation: Avoid default exports
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/AssistantPanelElement.tsx:
Line 15:
Default export reduces clarity and complicates IDE auto-imports. Change `export default function AssistantPanelElement` to a named export like `export function AssistantPanelElement` and update all import sites.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return ( | ||
| <div className="rgchat-empty" role="alert"> | ||
| <div className="rgchat-empty__icon" aria-hidden="true">⚠️</div> | ||
| <div>Couldn't load conversations.</div> |
There was a problem hiding this comment.
Hardcoded user-facing string violates externalization rules by embedding text directly in the component. Replace the literal with a translation function call, such as t('channelList.loadFailed'), and add the key to the dictionary.
Kody rule violation: Internationalize user-facing text with next-intl or next-i18next
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ChannelList.tsx:
Line 69:
Hardcoded user-facing string violates externalization rules by embedding text directly in the component. Replace the literal with a translation function call, such as `t('channelList.loadFailed')`, and add the key to the dictionary.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| padding: 0 6px; | ||
| .rgchat-footerbtn__badge { | ||
| min-width: 18px; | ||
| height: 18px; |
There was a problem hiding this comment.
Fixed dimension violation requires avoiding fixed pixel heights for containers in favor of min/max constraints or flexible layouts.
Kody rule violation: Prefer responsive over fixed dimensions
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chat.css:
Line 75:
Fixed dimension violation requires avoiding fixed pixel heights for containers in favor of min/max constraints or flexible layouts.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| box-shadow: 0 10px 30px rgba(15, 23, 42, 0.28); | ||
| transition: transform 160ms ease, box-shadow 160ms ease; | ||
| box-shadow: 0 2px 6px rgba(15, 23, 42, 0.18); | ||
| transition: background-color 140ms ease, box-shadow 140ms ease, transform 140ms ease; |
There was a problem hiding this comment.
Render performance degradation occurs because the hover transition on .rgchat-footerbtn animates the non-composited box-shadow property, forcing a paint on every frame. Remove box-shadow 140ms ease from the transition declaration, and if a shadow change is required, simulate it via a pseudo-element transitioning opacity.
Kody rule violation: Avoid expensive animations on non-composited properties
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chat.css:
Line 64:
Render performance degradation occurs because the hover transition on `.rgchat-footerbtn` animates the non-composited `box-shadow` property, forcing a paint on every frame. Remove `box-shadow 140ms ease` from the transition declaration, and if a shadow change is required, simulate it via a pseudo-element transitioning `opacity`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Non-404 failure (network, 500, expired token): surface a retryable error state | ||
| // instead of leaving the skeleton loader up forever. | ||
| setChannelsLoadFailed(true); | ||
| console.error('Failed to load chat channels.', error); |
There was a problem hiding this comment.
Unstructured error logging violates standards by using console.error with a plain message string and raw error. Replace this with a structured logger call that includes the operation name and relevant identifiers, such as logger.error('Failed to load chat channels', { op: 'getChannels', err: error }).
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/useChatBootstrap.ts:
Line 52:
Unstructured error logging violates standards by using `console.error` with a plain message string and raw error. Replace this with a structured logger call that includes the operation name and relevant identifiers, such as `logger.error('Failed to load chat channels', { op: 'getChannels', err: error })`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public IActionResult Index() | ||
| public async Task<IActionResult> Index() | ||
| { | ||
| if (!await ChatEnabledAsync()) |
There was a problem hiding this comment.
Unguarded external call occurs because the ChatEnabledAsync feature-toggle lookup is invoked without a try/catch. Add a try/catch around the await, log with operation and department context, and fall back to a safe default or redirect on failure.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/ChatController.cs:
Line 31:
Unguarded external call occurs because the `ChatEnabledAsync` feature-toggle lookup is invoked without a `try/catch`. Add a `try/catch` around the await, log with operation and department context, and fall back to a safe default or redirect on failure.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| [HttpGet] | ||
| public async Task<IActionResult> Index() | ||
| { | ||
| if (!await _featureToggleService.IsEnabledAsync(Resgrid.Model.FeatureFlagKeys.ChatSystem, DepartmentId)) |
There was a problem hiding this comment.
Unguarded async operation exists because the awaited call to _featureToggleService.IsEnabledAsync(...) is not wrapped in error handling, risking unhandled exceptions from the controller action on service failure. Wrap the call in a try/catch block, log the exception, and fall back to a safe default.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/ModerationController.cs:
Line 25:
Unguarded async operation exists because the awaited call to `_featureToggleService.IsEnabledAsync(...)` is not wrapped in error handling, risking unhandled exceptions from the controller action on service failure. Wrap the call in a `try/catch` block, log the exception, and fall back to a safe default.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| </div> | ||
| <div> | ||
| <strong>Copyright</strong> Resgrid, LLC © @DateTime.Now.Year All Rights Reserved. | ||
| <strong>Copyright</strong> Resgrid, LLC © @DateTime.Now.Year All Rights Reserved. <span class="text-muted">Version @typeof(Startup).Assembly.GetName().Version</span> |
There was a problem hiding this comment.
Timing operation violation occurs because DateTime.Now is affected by daylight savings and system clock adjustments. Use Stopwatch for more accurate timing.
Kody rule violation: Avoid `DateTime.Now` for Timing Operations
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Shared/_Footer.cshtml:
Line 7:
Timing operation violation occurs because `DateTime.Now` is affected by daylight savings and system clock adjustments. Use `Stopwatch` for more accurate timing.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @inject Resgrid.Model.Services.IFeatureToggleService featureToggleService | ||
| @{ | ||
| // Chat.System gates the moderation console link; it is only useful to department or group admins. | ||
| var chatModerationVisible = ClaimsAuthorizationHelper.IsUserDepartmentOrGroupAdmin(0) && |
There was a problem hiding this comment.
Magic number degrades readability because the literal 0 passed to IsUserDepartmentOrGroupAdmin lacks semantic context. Introduce a named constant with the correct semantic name and pass it to the method.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtml:
Line 5:
Magic number degrades readability because the literal `0` passed to `IsUserDepartmentOrGroupAdmin` lacks semantic context. Introduce a named constant with the correct semantic name and pass it to the method.
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Web/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.cs (1)
12-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository dependency-resolution pattern.
The new
ILogger<TtsDependencyHealthCheck>parameter introduces constructor injection. Remove this dependency when using static repository logging. If a logger dependency must remain, resolve it withBootstrapper.GetKernel().Resolve<T>().As per coding guidelines, use Service Locator pattern via
Bootstrapper.GetKernel().Resolve<T>()rather than constructor injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.cs` around lines 12 - 21, Update the TtsDependencyHealthCheck constructor to remove direct ILogger<TtsDependencyHealthCheck> injection and assign the logger through Bootstrapper.GetKernel().Resolve<ILogger<TtsDependencyHealthCheck>>(), following the repository’s service-locator dependency-resolution pattern.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Model/Services/IFeatureToggleService.cs`:
- Around line 19-24: Align the IsEnabledAsync contract with BuildDefault:
document that a value configured in FeatureFlagsConfig.CodeDefaults takes
precedence over the defaultValue argument for known keys, or update the
implementation to honor defaultValue instead. Ensure the documentation and
behavior consistently describe the same precedence.
In `@Core/Resgrid.Services/FeatureToggleService.cs`:
- Around line 67-80: Update PassesPlanGateAsync so Billing or subscription-check
failures return false or propagate the exception instead of returning true.
Ensure IsEnabledAsync’s existing fail-closed handling keeps flags with
MinimumPlanType disabled whenever the plan gate cannot be evaluated.
---
Nitpick comments:
In `@Web/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.cs`:
- Around line 12-21: Update the TtsDependencyHealthCheck constructor to remove
direct ILogger<TtsDependencyHealthCheck> injection and assign the logger through
Bootstrapper.GetKernel().Resolve<ILogger<TtsDependencyHealthCheck>>(), following
the repository’s service-locator dependency-resolution pattern.
🪄 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: 7dc5b33f-2971-4afd-b202-6bb5a440f255
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Services/ChatChannelServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/FeatureToggleServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (5)
Core/Resgrid.Model/Services/IFeatureToggleService.csCore/Resgrid.Services/ChatChannelService.csCore/Resgrid.Services/FeatureToggleService.csWeb/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.csWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPanelElement.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/Resgrid.Services/ChatChannelService.cs
- Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPanelElement.tsx
| /// <summary> | ||
| /// Returns whether a flag is enabled for a department, falling back to defaultValue when the flag | ||
| /// is unknown. Never throws: if evaluation itself fails (flag store unreachable) the error is | ||
| /// logged and the answer is false — gated features fail shut instead of 500ing their callers. | ||
| /// </summary> | ||
| Task<bool> IsEnabledAsync(string key, int departmentId, bool defaultValue = false, IDictionary<string, string> context = null); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the unknown-flag contract with BuildDefault.
When FeatureFlagsConfig.CodeDefaults contains the key, BuildDefault returns that configured value instead of the defaultValue method argument. Update this documentation to describe that precedence, or change the implementation to honor defaultValue.
Proposed documentation correction
- /// Returns whether a flag is enabled for a department, falling back to defaultValue when the flag
- /// is unknown. Never throws: if evaluation itself fails (flag store unreachable) the error is
+ /// Returns whether a flag is enabled for a department, falling back to the configured code
+ /// default when present, otherwise to defaultValue, when the flag is unknown. If evaluation
+ /// itself fails (flag store unreachable), the error is📝 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.
| /// <summary> | |
| /// Returns whether a flag is enabled for a department, falling back to defaultValue when the flag | |
| /// is unknown. Never throws: if evaluation itself fails (flag store unreachable) the error is | |
| /// logged and the answer is false — gated features fail shut instead of 500ing their callers. | |
| /// </summary> | |
| Task<bool> IsEnabledAsync(string key, int departmentId, bool defaultValue = false, IDictionary<string, string> context = null); | |
| /// <summary> | |
| /// Returns whether a flag is enabled for a department, falling back to the configured code | |
| /// default when present, otherwise to defaultValue, when the flag is unknown. If evaluation | |
| /// itself fails (flag store unreachable), the error is | |
| /// logged and the answer is false — gated features fail shut instead of 500ing their callers. | |
| /// </summary> | |
| Task<bool> IsEnabledAsync(string key, int departmentId, bool defaultValue = false, IDictionary<string, string> context = null); |
🤖 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.Model/Services/IFeatureToggleService.cs` around lines 19 - 24,
Align the IsEnabledAsync contract with BuildDefault: document that a value
configured in FeatureFlagsConfig.CodeDefaults takes precedence over the
defaultValue argument for known keys, or update the implementation to honor
defaultValue instead. Ensure the documentation and behavior consistently
describe the same precedence.
| try | ||
| { | ||
| var evaluation = await EvaluateInternalAsync(key, departmentId, context, defaultValue, new HashSet<int>()); | ||
| return evaluation.IsEnabled; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Fail shut: a flag-store outage (cache AND database unreachable) must never 500 every | ||
| // gated page/endpoint — the feature simply reads as disabled until the store recovers. | ||
| // Deliberately ignores defaultValue here: that is the "flag not defined" default, not | ||
| // the "evaluation infrastructure down" answer. | ||
| Logging.LogException(ex, $"FeatureToggle evaluation failed for '{key}' in department {departmentId}; failing shut (disabled)"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep plan-gate failures fail-closed.
IsEnabledAsync catches only exceptions that escape EvaluateInternalAsync. PassesPlanGateAsync catches Billing exceptions and returns true, so a flag with MinimumPlanType can remain enabled during a Billing outage. Return false from that failure path or propagate the exception so the documented fail-closed behavior applies to subscription checks.
Proposed plan-gate failure handling
catch (Exception ex)
{
Logging.LogException(ex, $"FeatureToggle plan gate check failed for department {departmentId}");
- return true;
+ return false;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/FeatureToggleService.cs` around lines 67 - 80, Update
PassesPlanGateAsync so Billing or subscription-check failures return false or
propagate the exception instead of returning true. Ensure IsEnabledAsync’s
existing fail-closed handling keeps flags with MinimumPlanType disabled whenever
the plan gate cannot be evaluated.
| if (group != null) | ||
| // Loaded once: source for the admin group-channel matching here AND the | ||
| // implicit-audience pass further down. | ||
| var allChannels = await _chatChannelRepository.GetAllByDepartmentIdAsync(departmentId, includeArchived); |
There was a problem hiding this comment.
Unhandled exception propagation occurs if GetAllByDepartmentIdAsync rejects. Wrap the repository call in a try/catch block, log the exception with context, and fall back to an empty list on failure.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 90:
Unhandled exception propagation occurs if `GetAllByDepartmentIdAsync` rejects. Wrap the repository call in a try/catch block, log the exception with context, and fall back to an empty list on failure.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var groupChannelsByGroupId = allChannels? | ||
| .Where(c => c.ChannelType == (int)ChatChannelType.GroupDefault && c.GroupId.HasValue) | ||
| .GroupBy(c => c.GroupId.Value) | ||
| .ToDictionary(g => g.Key, g => g.First()); |
There was a problem hiding this comment.
Complex multi-step LINQ chain reduces readability and debuggability. Extract the Where and GroupBy operations into an intermediate variable before building the dictionary.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 101 to 104:
Complex multi-step LINQ chain reduces readability and debuggability. Extract the `Where` and `GroupBy` operations into an intermediate variable before building the dictionary.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // gated page/endpoint — the feature simply reads as disabled until the store recovers. | ||
| // Deliberately ignores defaultValue here: that is the "flag not defined" default, not | ||
| // the "evaluation infrastructure down" answer. | ||
| Logging.LogException(ex, $"FeatureToggle evaluation failed for '{key}' in department {departmentId}; failing shut (disabled)"); |
There was a problem hiding this comment.
Unstructured logging detected in FeatureToggleService.cs and ChatChannelService.cs:119. Pass the operation name, key, departmentId, and error as structured fields instead of an interpolated string so logs remain queryable.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Services/FeatureToggleService.cs:
Line 78:
Unstructured logging detected in `FeatureToggleService.cs` and `ChatChannelService.cs:119`. Pass the operation name, `key`, `departmentId`, and error as structured fields instead of an interpolated string so logs remain queryable.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| new DepartmentGroup { DepartmentGroupId = 10, DepartmentId = 1, Name = "Station 2" } | ||
| }); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetByGroupIdAsync(9)).ThrowsAsync(new InvalidOperationException("db down")); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetByGroupIdAsync(10)).ReturnsAsync((ChatChannel)null); |
There was a problem hiding this comment.
Type casting rule violation detected. Use the as operator or pattern matching for safe casts and guard null results before usage.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Tests/Resgrid.Tests/Services/ChatChannelServiceTests.cs:
Line 600:
Type casting rule violation detected. Use the `as` operator or pattern matching for safe casts and guard null results before usage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (DirectoryNotFoundException) | ||
| { | ||
| // Creation itself failed; nothing to clean up. |
There was a problem hiding this comment.
Silent exception swallowing detected in the DirectoryNotFoundException catch block. Add a debug-level log statement with context or rethrow the exception to preserve diagnostic information during health check failures.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Web/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.cs:
Line 89 to 91:
Silent exception swallowing detected in the `DirectoryNotFoundException` catch block. Add a debug-level log statement with context or rethrow the exception to preserve diagnostic information during health check failures.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Pull Request Description
This PR introduces significant changes to the Chat and AI Assistant user experience, department admin channel visibility, and feature flag enforcement across the Resgrid application.
Chat UI Restructuring
AssistantPanelElement) accessible from any page via a footer button, improving accessibility without navigating away from the current view.Department Admin Channel Access
Feature Flag Enforcement
Localization
Infrastructure Fixes
Tests
Summary by CodeRabbit
New Features
Bug Fixes