Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
This comment has been minimized.
This comment has been minimized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds active-channel presence tracking with unit context, integrates unit memberships into channel retrieval, scopes notification suppression to channels, supports server-generated group names, and updates related group, recipient, and Sentry handling. ChangesChat activity and channel behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatPanelElement
participant chatHub
participant ChatHub
participant ChatPresenceService
participant ChatNotificationService
ChatPanelElement->>chatHub: report viewed channel
chatHub->>ChatHub: SetActiveChannel(channelId)
ChatHub->>ChatPresenceService: store active channel
ChatNotificationService->>ChatPresenceService: query channel activity
ChatPresenceService-->>ChatNotificationService: active users and units
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Core/Resgrid.Services/ChatPresenceService.cs (1)
39-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse the required cache-aside API.
These new cache reads use
GetStringAsync,SetStringAsync, andRemoveAsyncdirectly. Route cache retrieval throughICacheProvider.RetrieveAsync<T>()with a local fallback, or document and approve a presence-state exception to this rule.As per coding guidelines, “All caching operations must go through
ICacheProvider.Retrieve<T>()orICacheProvider.RetrieveAsync<T>()using the cache-aside pattern with fallback functions.”Also applies to: 92-162
🤖 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/ChatPresenceService.cs` around lines 39 - 48, The heartbeat logic in ChatPresenceService must use the required cache-aside API instead of direct GetStringAsync, SetStringAsync, or RemoveAsync calls. Update the active-channel and unit-marker handling across the affected methods to retrieve state through ICacheProvider.RetrieveAsync<T>() with local fallback functions, while preserving the existing TTL and presence behavior; do not add a presence exception unless explicitly approved.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 159-176: Validate activeUnitId with CanSendAsUnitAsync(UserId,
activeUnitId.Value, departmentId) before querying or returning unit memberships
in ChatChannelService; skip or reject unauthorized IDs. In ChatController, carry
only the validated active unit through channel/message reads,
CanAccessChannelAsync checks, and member-state operations so unit-only channels
remain accessible only to authorized crew members. Apply the controller change
at Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs lines 125-145 and
the service authorization change at Core/Resgrid.Services/ChatChannelService.cs
lines 159-176.
In `@Core/Resgrid.Services/ChatPresenceService.cs`:
- Around line 39-48: Update TouchAsync and the related active-channel set/remove
paths to store unit activity per user or connection rather than in the shared
GetUnitActiveKey marker. Use atomic owner-aware updates and deletes so one
viewer cannot overwrite, remove, or restore another viewer’s activity, and
derive unit push suppression only after resolving the current owners.
In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chatHub.ts`:
- Around line 371-387: Update setActiveChannel and reportActiveChannel to accept
and retain the acting unit ID alongside the channel ID, then pass that stored
unit ID as the asUnitId argument to CHAT_HUB_METHODS.SetActiveChannel instead of
null. Preserve the existing reconnect reporting behavior and allow the unit ID
to be cleared when no unit is active.
---
Nitpick comments:
In `@Core/Resgrid.Services/ChatPresenceService.cs`:
- Around line 39-48: The heartbeat logic in ChatPresenceService must use the
required cache-aside API instead of direct GetStringAsync, SetStringAsync, or
RemoveAsync calls. Update the active-channel and unit-marker handling across the
affected methods to retrieve state through ICacheProvider.RetrieveAsync<T>()
with local fallback functions, while preserving the existing TTL and presence
behavior; do not add a presence exception unless explicitly approved.
🪄 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: 4707efa0-c94c-40d0-88b4-098a25b441fd
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Services/ChatChannelServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/ChatPresenceServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (22)
Core/Resgrid.Model/Repositories/IChatRepositories.csCore/Resgrid.Model/Services/IChatServices.csCore/Resgrid.Services/ChatChannelService.csCore/Resgrid.Services/ChatMessageService.csCore/Resgrid.Services/ChatNotificationService.csCore/Resgrid.Services/ChatPresenceService.csCore/Resgrid.Services/DepartmentGroupsService.csRepositories/Resgrid.Repositories.DataRepository/ChatRepositories.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web.Eventing/Hubs/ChatHub.csWeb/Resgrid.Web.Services/Controllers/v4/ChatController.csWeb/Resgrid.Web.Services/Models/v4/Chat/ChatApiModels.csWeb/Resgrid.Web.Services/Program.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/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/NewConversationDialog.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chatHub.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/types.tsWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Views/Shared/_UserLayout.cshtml
| // invited to groups) — the unit's operator must see them without a personal member row. | ||
| if (activeUnitId.HasValue) | ||
| { | ||
| var unitMemberships = await _chatChannelMemberRepository.GetActiveByUnitIdAsync(departmentId, activeUnitId.Value); |
There was a problem hiding this comment.
Unhandled repository call: GetActiveByUnitIdAsync can throw transient or permanent errors that should be caught, augmented with context (departmentId, activeUnitId), and mapped to application-level errors. Wrap the call in try/catch, distinguish transient vs non-transient errors, log the operation name and identifiers, and apply retry or fallback as appropriate.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 163:
Unhandled repository call: GetActiveByUnitIdAsync can throw transient or permanent errors that should be caught, augmented with context (departmentId, activeUnitId), and mapped to application-level errors. Wrap the call in try/catch, distinguish transient vs non-transient errors, log the operation name and identifiers, and apply retry or fallback as appropriate.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (await _chatPresenceService.IsUnitActiveInChannelAsync(channel.DepartmentId, unitMember.UnitId.Value, channel.ChatChannelId)) | ||
| continue; |
There was a problem hiding this comment.
N+1 query pattern in the notification loop: the per-unit await of IsUnitActiveInChannelAsync makes one round-trip per unit member, breaking the batch pattern established by the activeUsers HashSet (lines 78–80) and degrading latency as audience size grows. Fetch all active unit IDs for the channel once into a HashSet (via GetUnitsActiveInChannelAsync) alongside the activeUsers fetch, then replace the await with a local set lookup (activeUnits.Contains(unitMember.UnitId.Value)).
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/ChatNotificationService.cs:
Line 124 to 125:
N+1 query pattern in the notification loop: the per-unit await of IsUnitActiveInChannelAsync makes one round-trip per unit member, breaking the batch pattern established by the activeUsers HashSet (lines 78–80) and degrading latency as audience size grows. Fetch all active unit IDs for the channel once into a HashSet (via GetUnitsActiveInChannelAsync) alongside the activeUsers fetch, then replace the await with a local set lookup (activeUnits.Contains(unitMember.UnitId.Value)).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var unitId = ParseUnitId(active); | ||
| if (unitId.HasValue) | ||
| await _cacheProvider.SetStringAsync(GetUnitActiveKey(departmentId, unitId.Value), ParseChannelId(active), GetTtl()); |
There was a problem hiding this comment.
Race condition in TouchAsync (line 48): the shared chatactiveunit:{dept}:{unitId} key is overwritten by each operator's heartbeat, causing the marker to oscillate between channels when multiple operators of the same unit view different channels simultaneously — during the mismatch window IsUnitActiveInChannelAsync returns false and triggers duplicate pushes to the unit device. Key the unit active marker per (unitId, userId) pair instead of per unitId alone, or have GetUsersActiveInChannelAsync check whether any operator of the unit is active in the channel.
Prompt for LLM
File Core/Resgrid.Services/ChatPresenceService.cs:
Line 46 to 48:
Race condition in TouchAsync (line 48): the shared chatactiveunit:{dept}:{unitId} key is overwritten by each operator's heartbeat, causing the marker to oscillate between channels when multiple operators of the same unit view different channels simultaneously — during the mismatch window IsUnitActiveInChannelAsync returns false and triggers duplicate pushes to the unit device. Key the unit active marker per (unitId, userId) pair instead of per unitId alone, or have GetUsersActiveInChannelAsync check whether any operator of the unit is active in the channel.
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); |
There was a problem hiding this comment.
Insufficient error context: Logging.LogException(ex) omits the operation name and identifiers (departmentId, unitId) required for tracing failures per Rule [3]. Pass structured context, e.g., Logging.LogException(ex, new { op = nameof(GetActiveByUnitIdAsync), departmentId, unitId }).
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ChatRepositories.cs:
Line 898:
Insufficient error context: Logging.LogException(ex) omits the operation name and identifiers (departmentId, unitId) required for tracing failures per Rule [3]. Pass structured context, e.g., Logging.LogException(ex, new { op = nameof(GetActiveByUnitIdAsync), departmentId, unitId }).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| if (string.IsNullOrWhiteSpace(channelId)) | ||
| { | ||
| await _chatPresenceService.ClearActiveChannelAsync(departmentId, userId); |
There was a problem hiding this comment.
Unguarded async operation: the awaited ClearActiveChannelAsync will propagate an opaque error to the SignalR client if it throws. Wrap the call in try/catch, log the error with context (departmentId, userId), and either rethrow or return an appropriate error.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web.Eventing/Hubs/ChatHub.cs:
Line 297:
Unguarded async operation: the awaited ClearActiveChannelAsync will propagate an opaque error to the SignalR client if it throws. Wrap the call in try/catch, log the error with context (departmentId, userId), and either rethrow or return an appropriate error.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var userIds = counterparts.Values | ||
| .Where(m => String.IsNullOrWhiteSpace(m.DisplayNameOverride) && !String.IsNullOrWhiteSpace(m.UserId)) | ||
| .Select(m => m.UserId) | ||
| .Distinct() | ||
| .ToList(); |
There was a problem hiding this comment.
Long LINQ chain (Where → Select → Distinct → ToList) in a single expression reduces readability and obscures intermediate results. Break into named intermediate steps — for example, extract counterparts.Values.Where(...) into a membersNeedingLookup variable before projecting and deduplicating.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 1877 to 1881:
Long LINQ chain (Where → Select → Distinct → ToList) in a single expression reduces readability and obscures intermediate results. Break into named intermediate steps — for example, extract counterparts.Values.Where(...) into a membersNeedingLookup variable before projecting and deduplicating.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| result.Data = ConvertChannelResultData(channel, member); | ||
| await ResolveDirectMessageNamesAsync(new List<ChatChannelResultData> { result.Data }); | ||
| result.PageSize = 1; |
There was a problem hiding this comment.
ResolveDirectMessageNamesAsync is called from GetChannel (line 185) and CreateDirectMessage (line 227) without activeUnitId, so the unit's own member row (UserId=null, UnitId=7) bypasses both the UserId check (line 1867) and the activeUnitId check (line 1870, which defaults to null), potentially selecting the unit's DisplayNameOverride (e.g. "Engine 6") as the DM counterpart name instead of the dispatcher's. Thread the activeUnitId parameter through to ResolveDirectMessageNamesAsync so the unit's own row is excluded from counterpart selection.
// Pass the viewer's acting unit so the unit's own row is never selected as the DM counterpart.
result.Data = ConvertChannelResultData(channel, member);
await ResolveDirectMessageNamesAsync(new List<ChatChannelResultData> { result.Data }, activeUnitId);Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 184 to 186:
ResolveDirectMessageNamesAsync is called from GetChannel (line 185) and CreateDirectMessage (line 227) without activeUnitId, so the unit's own member row (UserId=null, UnitId=7) bypasses both the UserId check (line 1867) and the activeUnitId check (line 1870, which defaults to null), potentially selecting the unit's DisplayNameOverride (e.g. "Engine 6") as the DM counterpart name instead of the dispatcher's. Thread the activeUnitId parameter through to ResolveDirectMessageNamesAsync so the unit's own row is excluded from counterpart selection.
Suggested Code:
// Pass the viewer's acting unit so the unit's own row is never selected as the DM counterpart.
result.Data = ConvertChannelResultData(channel, member);
await ResolveDirectMessageNamesAsync(new List<ChatChannelResultData> { result.Data }, activeUnitId);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (this.connection && this.connection.state === HubConnectionState.Connected) { | ||
| this.connection | ||
| .invoke(CHAT_HUB_METHODS.SetActiveChannel, this.activeChannelReported, null) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
Silent error swallowing: the .catch(() => undefined) on the SignalR invoke promise discards all exceptions, making failed SetActiveChannel calls invisible during debugging. Replace with a catch that logs structured context including the operation name and channelId (e.g., logger.error('Failed to report active channel', { channelId: this.activeChannelReported, err })).
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chatHub.ts:
Line 385:
Silent error swallowing: the .catch(() => undefined) on the SignalR invoke promise discards all exceptions, making failed SetActiveChannel calls invisible during debugging. Replace with a catch that logs structured context including the operation name and channelId (e.g., logger.error('Failed to report active channel', { channelId: this.activeChannelReported, err })).
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:
|
| // invited to groups) — the unit's operator must see them without a personal member row. | ||
| // The caller-supplied unit only counts when the user actually crews it; otherwise any | ||
| // department member could list another unit's private channels. | ||
| if (activeUnitId.HasValue && await _chatPermissionService.CanSendAsUnitAsync(userId, activeUnitId.Value, departmentId)) |
There was a problem hiding this comment.
Unhandled exception in await _chatPermissionService.CanSendAsUnitAsync(...) propagates raw when the permission service throws, causing the channel list fetch to fail without context or safe fallback (also at Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:110). Wrap the call in try/catch, log structured context (userId, activeUnitId, departmentId), and default to denying access on failure.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Services/ChatChannelService.cs:
Line 163:
Unhandled exception in `await _chatPermissionService.CanSendAsUnitAsync(...)` propagates raw when the permission service throws, causing the channel list fetch to fail without context or safe fallback (also at `Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:110`). Wrap the call in try/catch, log structured context (`userId`, `activeUnitId`, `departmentId`), and default to denying access on failure.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (unitId <= 0 || string.IsNullOrWhiteSpace(channelId)) | ||
| return false; | ||
|
|
||
| var marker = await _cacheProvider.GetStringAsync(GetUnitActiveKey(departmentId, unitId)); |
There was a problem hiding this comment.
Unwrapped cache read in await _cacheProvider.GetStringAsync(GetUnitActiveKey(departmentId, unitId)) violates Rule [27] — a cache failure throws raw and loses operation context (also at Core/Resgrid.Services/ChatPresenceService.cs:169, 185, 190, 196, 202). Wrap the call in try/catch, log {op:'IsUnitActiveInChannelAsync', departmentId, unitId, channelId}, and return false on failure.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/ChatPresenceService.cs:
Line 161:
Unwrapped cache read in `await _cacheProvider.GetStringAsync(GetUnitActiveKey(departmentId, unitId))` violates Rule [27] — a cache failure throws raw and loses operation context (also at `Core/Resgrid.Services/ChatPresenceService.cs:169, 185, 190, 196, 202`). Wrap the call in try/catch, log `{op:'IsUnitActiveInChannelAsync', departmentId, unitId, channelId}`, and return `false` on failure.
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 several improvements to the chat system, department group performance, and error handling:
Chat Enhancements
Department Groups Performance & Correctness
GetAllGroupsForDepartmentUnlimitedAsyncby resolving parent/child relationships and addresses in-memory from a single query.DepartmentId, preventing cross-department data leakage when the same user belongs to multiple departments.Error Handling
BadHttpRequestException(client-aborted requests) from Sentry to reduce noise.webkit-masked-url://) from both client and server Sentry reporting.Summary by CodeRabbit
New Features
Improvements
Bug Fixes