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? |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes add per-user modern sound preferences, active inbox filtering, calendar RSVP handling for API and web messages, and delegated personnel check-ins with accountability validation. ChangesModern application sounds
Active inbox filtering
Calendar RSVP messages
Delegated personnel check-ins
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MessagesController
participant ICalendarService
participant MessageRecipient
User->>MessagesController: submit CalendarRsvp response
MessagesController->>ICalendarService: validate item and signup
ICalendarService->>MessageRecipient: save response and read timestamp
MessagesController-->>User: redirect or API response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 (2)
Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs (2)
41-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConstructor injection keeps growing in both
MessagesControllerimplementations. Per coding guidelines, dependencies should be resolved viaBootstrapper.GetKernel().Resolve<T>()in the constructor body rather than accumulating constructor parameters, and the number of injected dependencies should be kept small; each new feature (this PR addsICalendarServiceto both) pushes both controllers further from that.
Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs#L41-L67: this constructor now takes 10 dependencies; consider resolving less-frequently-used ones (e.g.,ICalendarService,IUnitsService) via the service locator pattern instead of adding more constructor parameters.Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs#L34-L49: same pattern, now at 9 dependencies with the addition ofICalendarService.As per coding guidelines, "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve() to resolve dependencies explicitly in constructors, rather than constructor injection" and "Minimize constructor injection; keep the number of injected dependencies small."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs` around lines 41 - 67, Reduce constructor injection in both MessagesController implementations by removing less-frequently-used dependencies, specifically ICalendarService and IUnitsService where applicable, from their constructor parameters and resolving them in the constructor body through Bootstrapper.GetKernel().Resolve<T>(). Apply the corresponding change in Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs lines 41-67 and Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs lines 34-49, while preserving the existing field assignments and behavior.Source: Coding guidelines
450-476: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCalendar RSVP eligibility-check-and-signup sequence is duplicated across the API and Web controllers. Both implement the same steps independently: parse
calendarItemIdfrom the recipient's note viaTextResponsePromptMetadata.TryGetCalendarItemId, load the calendar item, verify department match, verifySignupType == RSVP, callCanUserCheckInToCalendarEventAsync, then call_calendarService.SignupForEvent. Any future fix or business-rule change to this sequence (e.g., an additional eligibility check) has to be made in three places and can silently drift out of sync.
Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs#L450-L476: extract this block into a shared method (e.g., onICalendarServiceor a small static helper) that returns a typed result (success/calendar-item-not-found/unauthorized/invalid-response) for both callers to consume.Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs#L257-L273: reuse the shared validation for computingCanRespondToCalendarRsvp/attendee lookup instead of re-implementing department/signup-type checks inline.Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs#L280-L316: reuse the same shared method for the POST submit path instead of duplicating the validation+SignupForEventcall.🤖 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/MessagesController.cs` around lines 450 - 476, Calendar RSVP eligibility and signup logic is duplicated across three controller sites. Extract the shared parsing, calendar lookup, department and RSVP authorization checks, and signup operation into a reusable typed-result method, then update Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs lines 450-476, Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs lines 257-273, and Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs lines 280-316 to consume it for validation, attendee lookup, CanRespondToCalendarRsvp, and submission handling.
🤖 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/MessageService.cs`:
- Around line 85-89: Update IsActiveInboxMessage to be user-scoped and exclude
messages whose recipient record for the current user has
MessageRecipient.IsDeleted set, while preserving the existing null,
message-level deletion, and expiration checks. Alternatively, enforce the
equivalent recipient-level filter in the inbox repository before calculating
inbox results and unread counts.
- Around line 81-83: Update the unread-count logic around
GetUnreadInboxMessagesByUserIdAsync so it does not materialize all unread
Message entities or duplicate the inbox fetch. Compute the count in the database
using the same active predicates, or reuse the result already fetched by
GetInboxMessagesByUserIdAsync while preserving the existing count behavior.
In `@Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs`:
- Around line 30-45: Update CheckInTimersController to remove
IIncidentCommandService constructor injection and resolve it inside the
constructor using Bootstrapper.GetKernel().Resolve<IIncidentCommandService>(),
while preserving the existing _incidentCommandService assignment and other
injected dependencies.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs`:
- Around line 41-67: Reduce constructor injection in both MessagesController
implementations by removing less-frequently-used dependencies, specifically
ICalendarService and IUnitsService where applicable, from their constructor
parameters and resolving them in the constructor body through
Bootstrapper.GetKernel().Resolve<T>(). Apply the corresponding change in
Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs lines 41-67 and
Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs lines 34-49, while
preserving the existing field assignments and behavior.
- Around line 450-476: Calendar RSVP eligibility and signup logic is duplicated
across three controller sites. Extract the shared parsing, calendar lookup,
department and RSVP authorization checks, and signup operation into a reusable
typed-result method, then update
Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs lines 450-476,
Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs lines 257-273, and
Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs lines 280-316 to
consume it for validation, attendee lookup, CanRespondToCalendarRsvp, and
submission handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13e07686-f466-41cd-97eb-dbd144992421
⛔ Files ignored due to path filters (21)
Core/Resgrid.Localization/Areas/User/Home/EditProfile.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.uk.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Messages/Messages.uk.resxis excluded by!**/*.resxTests/Resgrid.Tests/Services/MessageServiceInboxTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/PushServiceModernApplicationSoundTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/CheckInTimersControllerTests.csis excluded by!**/Tests/**
📒 Files selected for processing (18)
Core/Resgrid.Model/ModernApplicationSoundSettings.csCore/Resgrid.Model/UserProfile.csCore/Resgrid.Services/MessageService.csCore/Resgrid.Services/PushService.csProviders/Resgrid.Providers.Migrations/Migrations/M0100_AddModernApplicationSoundsUserSetting.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0100_AddModernApplicationSoundsUserSettingPg.csWeb/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.csWeb/Resgrid.Web.Services/Controllers/v4/ConfigController.csWeb/Resgrid.Web.Services/Controllers/v4/MessagesController.csWeb/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.csWeb/Resgrid.Web.Services/Models/v4/Configs/GetConfigResult.csWeb/Resgrid.Web.Services/Models/v4/Messages/GetMessagesResult.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Controllers/MessagesController.csWeb/Resgrid.Web/Areas/User/Models/Messages/ViewMessageView.csWeb/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtmlWeb/Resgrid.Web/Areas/User/Views/Messages/ViewMessage.cshtml
| private static bool IsActiveInboxMessage(Message message) | ||
| { | ||
| return message != null | ||
| && !message.IsDeleted | ||
| && (!message.ExpireOn.HasValue || message.ExpireOn.Value > DateTime.UtcNow); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Include recipient-level deletion in the active predicate.
IsActiveInboxMessage only checks Message.IsDeleted, but this service also supports per-user deletion through MessageRecipient.IsDeleted. Because the inbox repository contract is unfiltered, a message deleted only for the current user can still appear in the inbox and unread count. Make this predicate user-scoped and exclude the user’s deleted recipient record, or enforce the equivalent filter in the repository.
🤖 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/MessageService.cs` around lines 85 - 89, Update
IsActiveInboxMessage to be user-scoped and exclude messages whose recipient
record for the current user has MessageRecipient.IsDeleted set, while preserving
the existing null, message-level deletion, and expiration checks. Alternatively,
enforce the equivalent recipient-level filter in the inbox repository before
calculating inbox results and unread counts.
| private readonly IIncidentCommandService _incidentCommandService; | ||
|
|
||
| public CheckInTimersController( | ||
| ICheckInTimerService checkInTimerService, | ||
| ICallsService callsService, | ||
| IDepartmentSettingsService departmentSettingsService, | ||
| IDepartmentsService departmentsService, | ||
| IUserProfileService userProfileService) | ||
| IUserProfileService userProfileService, | ||
| IIncidentCommandService incidentCommandService) | ||
| { | ||
| _checkInTimerService = checkInTimerService; | ||
| _callsService = callsService; | ||
| _departmentSettingsService = departmentSettingsService; | ||
| _departmentsService = departmentsService; | ||
| _userProfileService = userProfileService; | ||
| _incidentCommandService = incidentCommandService; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve IIncidentCommandService through the required Service Locator pattern.
Line 38 adds constructor injection, which conflicts with the repository’s required dependency-resolution approach.
Proposed fix
public CheckInTimersController(
ICheckInTimerService checkInTimerService,
ICallsService callsService,
IDepartmentSettingsService departmentSettingsService,
IDepartmentsService departmentsService,
- IUserProfileService userProfileService,
- IIncidentCommandService incidentCommandService)
+ IUserProfileService userProfileService)
{
_checkInTimerService = checkInTimerService;
_callsService = callsService;
_departmentSettingsService = departmentSettingsService;
_departmentsService = departmentsService;
_userProfileService = userProfileService;
- _incidentCommandService = incidentCommandService;
+ _incidentCommandService = Bootstrapper.GetKernel().Resolve<IIncidentCommandService>();
}As per coding guidelines, use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private readonly IIncidentCommandService _incidentCommandService; | |
| public CheckInTimersController( | |
| ICheckInTimerService checkInTimerService, | |
| ICallsService callsService, | |
| IDepartmentSettingsService departmentSettingsService, | |
| IDepartmentsService departmentsService, | |
| IUserProfileService userProfileService) | |
| IUserProfileService userProfileService, | |
| IIncidentCommandService incidentCommandService) | |
| { | |
| _checkInTimerService = checkInTimerService; | |
| _callsService = callsService; | |
| _departmentSettingsService = departmentSettingsService; | |
| _departmentsService = departmentsService; | |
| _userProfileService = userProfileService; | |
| _incidentCommandService = incidentCommandService; | |
| private readonly IIncidentCommandService _incidentCommandService; | |
| public CheckInTimersController( | |
| ICheckInTimerService checkInTimerService, | |
| ICallsService callsService, | |
| IDepartmentSettingsService departmentSettingsService, | |
| IDepartmentsService departmentsService, | |
| IUserProfileService userProfileService) | |
| { | |
| _checkInTimerService = checkInTimerService; | |
| _callsService = callsService; | |
| _departmentSettingsService = departmentSettingsService; | |
| _departmentsService = departmentsService; | |
| _userProfileService = userProfileService; | |
| _incidentCommandService = Bootstrapper.GetKernel().Resolve<IIncidentCommandService>(); | |
| } |
🤖 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/CheckInTimersController.cs` around
lines 30 - 45, Update CheckInTimersController to remove IIncidentCommandService
constructor injection and resolve it inside the constructor using
Bootstrapper.GetKernel().Resolve<IIncidentCommandService>(), while preserving
the existing _incidentCommandService assignment and other injected dependencies.
Source: Coding guidelines
| public async Task<int> GetUnreadMessagesCountByUserIdAsync(string userId) | ||
| { | ||
| return await _messageRepository.GetUnreadMessageCountAsync(userId); | ||
| var messages = await GetUnreadInboxMessagesByUserIdAsync(userId); | ||
| return messages.Count; | ||
| } |
There was a problem hiding this comment.
Unbounded memory allocation: GetUnreadMessagesCountByUserIdAsync replaces a DB-side COUNT query with GetUnreadInboxMessagesByUserIdAsync, loading ALL inbox Message entities (bodies, recipients, etc.) into memory on every Inbox and ViewMessage page load just to return .Count. Add ExpireOn and IsDeleted predicates to the existing server-side COUNT query (SelectUnreadMessageCountQuery) instead of materializing full entities.
public async Task<int> GetUnreadMessagesCountByUserIdAsync(string userId)
{
return await _messageRepository.GetUnreadMessageCountAsync(userId);
// (update SelectUnreadMessageCountQuery SQL to also exclude IsDeleted and expired messages)Prompt for LLM
File Core/Resgrid.Services/MessageService.cs:
Line 79 to 83:
Unbounded memory allocation: `GetUnreadMessagesCountByUserIdAsync` replaces a DB-side COUNT query with `GetUnreadInboxMessagesByUserIdAsync`, loading ALL inbox `Message` entities (bodies, recipients, etc.) into memory on every Inbox and ViewMessage page load just to return `.Count`. Add `ExpireOn` and `IsDeleted` predicates to the existing server-side COUNT query (`SelectUnreadMessageCountQuery`) instead of materializing full entities.
Suggested Code:
public async Task<int> GetUnreadMessagesCountByUserIdAsync(string userId)
{
return await _messageRepository.GetUnreadMessageCountAsync(userId);
// (update SelectUnreadMessageCountQuery SQL to also exclude IsDeleted and expired messages)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Framework.Logging.LogException(ex); |
There was a problem hiding this comment.
Missing structured logging context: the catch block in GetModernApplicationSoundsEnabledAsync calls Framework.Logging.LogException(ex) with only a bare exception. Include the operation name and relevant identifiers (e.g., departmentId) as structured fields in the log call.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Services/PushService.cs:
Line 269:
Missing structured logging context: the catch block in `GetModernApplicationSoundsEnabledAsync` calls `Framework.Logging.LogException(ex)` with only a bare exception. Include the operation name and relevant identifiers (e.g., `departmentId`) as structured fields in the log call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| public override void Down() | ||
| { | ||
| Delete.Column("EnableModernApplicationSounds".ToLower()).FromTable("UserProfiles".ToLower()); |
There was a problem hiding this comment.
Duplicated string literals: "EnableModernApplicationSounds" and "UserProfiles" are repeated as raw literals across migration methods, requiring synchronized edits on any rename. Define const fields for the table and column names at the class level and reuse them in every method.
Kody rule violation: Centralize string constants
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0100_AddModernApplicationSoundsUserSettingPg.cs:
Line 16:
Duplicated string literals: `"EnableModernApplicationSounds"` and `"UserProfiles"` are repeated as raw literals across migration methods, requiring synchronized edits on any rename. Define `const` fields for the table and column names at the class level and reuse them in every method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| UserId = TargetUserId | ||
| }, CancellationToken.None); | ||
|
|
||
| response.Result.Should().BeOfType<OkObjectResult>(); |
There was a problem hiding this comment.
Blocking async operation: .Result or .Wait() causes deadlocks and prevents efficient asynchronous execution. Use await instead for proper async behavior.
Kody rule violation: Avoid Blocking Calls to Async Methods
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/CheckInTimersControllerTests.cs:
Line 102:
Blocking async operation: `.Result` or `.Wait()` causes deadlocks and prevents efficient asynchronous execution. Use `await` instead for proper async behavior.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| UserId = TargetUserId | ||
| }, CancellationToken.None); | ||
|
|
||
| response.Result.Should().BeOfType<OkObjectResult>(); |
There was a problem hiding this comment.
Blocking async operation: .Result or .Wait() on async methods can cause deadlocks. Use await end-to-end and configure awaits appropriately.
Kody rule violation: Await async operations properly
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/CheckInTimersControllerTests.cs:
Line 102:
Blocking async operation: `.Result` or `.Wait()` on async methods can cause deadlocks. Use `await` end-to-end and configure awaits appropriately.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| response.Result.Should().BeOfType<OkObjectResult>(); | ||
| savedRecord.Should().NotBeNull(); | ||
| savedRecord.UserId.Should().Be(TargetUserId); |
There was a problem hiding this comment.
Potential NullReferenceException: savedRecord is accessed without a null check, which throws if the previous assertion fails or is bypassed. Use a null check or pattern matching (e.g., if (savedRecord != null)).
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/CheckInTimersControllerTests.cs:
Line 104:
Potential `NullReferenceException`: `savedRecord` is accessed without a null check, which throws if the previous assertion fails or is bypassed. Use a null check or pattern matching (e.g., `if (savedRecord != null)`).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .Callback<CheckInRecord, CancellationToken>((record, _) => savedRecord = record) | ||
| .ReturnsAsync(new CheckInRecord { CheckInRecordId = "record-1" }); | ||
|
|
||
| var response = await _controller.PerformCheckIn(new PerformCheckInInput |
There was a problem hiding this comment.
Duplicated code block initializing and passing a PerformCheckInInput object reduces maintainability and risks inconsistent updates. Extract the creation of PerformCheckInInput and the controller call into a helper method (e.g., ExecutePerformCheckIn(CheckInTimerTargetType type)).
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/CheckInTimersControllerTests.cs:
Line 95:
Duplicated code block initializing and passing a `PerformCheckInInput` object reduces maintainability and risks inconsistent updates. Extract the creation of `PerformCheckInInput` and the controller call into a helper method (e.g., `ExecutePerformCheckIn(CheckInTimerTargetType type)`).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| result.Data.AnalyticsHost = ""; | ||
|
|
||
| bool departmentModernApplicationSoundsEnabled = departmentId > 0 | ||
| && await _departmentSettingsService.GetModernNotificationsEnabledAsync(departmentId); |
There was a problem hiding this comment.
Unhandled exception: the awaited call to GetModernNotificationsEnabledAsync is not wrapped in try/catch, so a failure from the department settings service propagates and produces an unhelpful 500 error. Wrap the await in a try/catch, log the error with context (departmentId, operation name), and provide a safe default (e.g., false).
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ConfigController.cs:
Line 118:
Unhandled exception: the awaited call to `GetModernNotificationsEnabledAsync` is not wrapped in try/catch, so a failure from the department settings service propagates and produces an unhelpful 500 error. Wrap the await in a try/catch, log the error with context (`departmentId`, operation name), and provide a safe default (e.g., `false`).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return NotFound(); | ||
|
|
||
| if (calendarItem.DepartmentId != DepartmentId) | ||
| return Unauthorized(); |
There was a problem hiding this comment.
Incorrect HTTP status code: Unauthorized() (401) is returned when a calendar item belongs to a different department, but the user is authenticated and only lacks authorization. Return Forbid() (403) or NotFound() to correctly indicate authorization failure and avoid information leakage.
Kody rule violation: Use appropriate HTTP status codes
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs:
Line 461:
Incorrect HTTP status code: `Unauthorized()` (401) is returned when a calendar item belongs to a different department, but the user is authenticated and only lacks authorization. Return `Forbid()` (403) or `NotFound()` to correctly indicate authorization failure and avoid information leakage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| response.Response = responseInput.Type.ToString(); | ||
| if (message.Type == (int)MessageTypes.CalendarRsvp) | ||
| { | ||
| if ((responseInput.Type != 1 && responseInput.Type != 3) |
There was a problem hiding this comment.
Magic numbers 1 and 3 used to check response types obscure intent and are error-prone during maintenance. Define named constants (e.g., ResponseTypeAttending = 1, ResponseTypeNotAttending = 3) or compare against enum values.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs:
Line 452:
Magic numbers `1` and `3` used to check response types obscure intent and are error-prone during maintenance. Define named constants (e.g., `ResponseTypeAttending = 1`, `ResponseTypeNotAttending = 3`) or compare against enum values.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public int CheckInType { get; set; } | ||
|
|
||
| /// <summary>Optional personnel user id when an incident commander checks in on that person's behalf.</summary> | ||
| public string UserId { get; set; } |
There was a problem hiding this comment.
Uninitialized property: the UserId string auto-property defaults to null, risking downstream NullReferenceExceptions. Initialize it to string.Empty to provide a safe default.
Kody rule violation: Initialize properties with default values
Prompt for LLM
File Web/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.cs:
Line 145:
Uninitialized property: the `UserId` string auto-property defaults to `null`, risking downstream `NullReferenceException`s. Initialize it to `string.Empty` to provide a safe default.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _calendarService.SignupForEvent(calendarItemId, UserId, normalizedResponse, | ||
| attendeeType, cancellationToken); |
There was a problem hiding this comment.
Unhandled exception: the SignupForEvent service call — an external/database write — is not wrapped in try/catch, so connection failures or constraint violations produce a generic 500 with no diagnostic context. Wrap the write in try/catch, log structured error details (messageId, calendarItemId, UserId), and return a user-friendly error result.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs:
Line 309 to 310:
Unhandled exception: the `SignupForEvent` service call — an external/database write — is not wrapped in try/catch, so connection failures or constraint violations produce a generic 500 with no diagnostic context. Wrap the write in try/catch, log structured error details (`messageId`, `calendarItemId`, `UserId`), and return a user-friendly error result.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _calendarService.SignupForEvent(calendarItemId, UserId, normalizedResponse, | ||
| attendeeType, cancellationToken); | ||
| recipient.Response = normalizedResponse; | ||
| recipient.ReadOn = DateTime.UtcNow; | ||
| await _messageService.SaveMessageRecipientAsync(recipient, cancellationToken); |
There was a problem hiding this comment.
Non-atomic writes: SignupForEvent and SaveMessageRecipientAsync modify persisted state without a wrapping transaction, leaving the system inconsistent if the first write succeeds and the second fails. Wrap both writes in a database transaction (BeginTransaction/Commit), issue a single SaveChanges, and roll back on failure.
Kody rule violation: Handle transaction rollbacks properly
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs:
Line 309 to 313:
Non-atomic writes: `SignupForEvent` and `SaveMessageRecipientAsync` modify persisted state without a wrapping transaction, leaving the system inconsistent if the first write succeeds and the second fails. Wrap both writes in a database transaction (`BeginTransaction`/`Commit`), issue a single `SaveChanges`, and roll back on failure.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| || !await _authorizationService.CanUserCheckInToCalendarEventAsync(UserId, calendarItemId)) | ||
| return Unauthorized(); | ||
|
|
||
| var normalizedResponse = attending ? "Yes" : "No"; |
There was a problem hiding this comment.
Hardcoded string literals "Yes" and "No" represent a finite RSVP set but introduce typos, are hard to search for, and duplicate domain meaning. Define an enum (e.g., CalendarRsvpResponse { Yes, No }) or reference an existing response-value constant.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs:
Line 304:
Hardcoded string literals `"Yes"` and `"No"` represent a finite RSVP set but introduce typos, are hard to search for, and duplicate domain meaning. Define an enum (e.g., `CalendarRsvpResponse { Yes, No }`) or reference an existing response-value constant.
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Core/Resgrid.Services/CalendarService.cs (1)
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the mandated dependency-resolution pattern.
These dependencies are added through constructor injection, but the repository guideline requires resolving dependencies with
Bootstrapper.GetKernel().Resolve<T>()instead. Please follow that convention or confirm this service is explicitly exempt.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 `@Core/Resgrid.Services/CalendarService.cs` around lines 30 - 37, Update the CalendarService constructor dependency setup to follow the mandated service-locator pattern: remove the newly injected dependencies from the constructor parameters and resolve them with Bootstrapper.GetKernel().Resolve<T>() inside the constructor. Apply this consistently to the affected repository and unit-of-work dependencies, or document the service’s explicit exemption if constructor injection must remain.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.
Nitpick comments:
In `@Core/Resgrid.Services/CalendarService.cs`:
- Around line 30-37: Update the CalendarService constructor dependency setup to
follow the mandated service-locator pattern: remove the newly injected
dependencies from the constructor parameters and resolve them with
Bootstrapper.GetKernel().Resolve<T>() inside the constructor. Apply this
consistently to the affected repository and unit-of-work dependencies, or
document the service’s explicit exemption if constructor injection must remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f490994a-1999-4736-a71c-2f7b1076b282
⛔ Files ignored due to path filters (4)
Tests/Resgrid.Tests/Services/CalendarServiceCheckInTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/CalendarServiceRsvpTransactionTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/CalendarServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/MessageServiceInboxTests.csis excluded by!**/Tests/**
📒 Files selected for processing (10)
Core/Resgrid.Model/Services/ICalendarService.csCore/Resgrid.Services/CalendarService.csCore/Resgrid.Services/MessageService.csRepositories/Resgrid.Repositories.DataRepository/MessageRepository.csRepositories/Resgrid.Repositories.DataRepository/Queries/Messages/SelectUnreadMessageCountQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web.Services/Controllers/v4/ConfigController.csWeb/Resgrid.Web.Services/Controllers/v4/MessagesController.csWeb/Resgrid.Web/Areas/User/Controllers/MessagesController.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Web/Resgrid.Web.Services/Controllers/v4/ConfigController.cs
- Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs
- Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs
| catch | ||
| { | ||
| _unitOfWork.DiscardChanges(); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
The catch block rethrows without logging context, violating Rule 30 which requires operation name and identifiers before propagation for diagnosability. Change to catch (Exception ex) and log structured fields such as operation name SignupForEventAndUpdateMessageRecipientAsync, calendarEventItemId, and userId before throw;.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Core/Resgrid.Services/CalendarService.cs:
Line 508 to 512:
The `catch` block rethrows without logging context, violating Rule 30 which requires operation name and identifiers before propagation for diagnosability. Change to `catch (Exception ex)` and log structured fields such as operation name `SignupForEventAndUpdateMessageRecipientAsync`, `calendarEventItemId`, and `userId` before `throw;`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @@ -75,6 +81,13 @@ public async Task<int> GetUnreadMessagesCountByUserIdAsync(string userId) | |||
| return await _messageRepository.GetUnreadMessageCountAsync(userId); | |||
There was a problem hiding this comment.
Unguarded awaited repository call violates Rule 1, which requires every awaited async operation to be wrapped in try/catch with explicit rejection handling. Wrap the call in try/catch, log the exception (e.g., logger.LogError(ex, "GetUnreadMessageCountAsync failed")) before throw;, or document caller responsibility for rejection handling.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Services/MessageService.cs:
Line 81:
Unguarded awaited repository call violates Rule 1, which requires every awaited async operation to be wrapped in try/catch with explicit rejection handling. Wrap the call in try/catch, log the exception (e.g., `logger.LogError(ex, "GetUnreadMessageCountAsync failed")`) before `throw;`, or document caller responsibility for rejection handling.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @@ -75,6 +81,13 @@ public async Task<int> GetUnreadMessagesCountByUserIdAsync(string userId) | |||
| return await _messageRepository.GetUnreadMessageCountAsync(userId); | |||
There was a problem hiding this comment.
Unguarded external repository call violates Rule 28, which requires wrapping external calls in try/catch with structured context before mapping to application-level errors. Wrap the call in try/catch, log userId and operation context, and transduce the exception to an application-level error before propagation.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/MessageService.cs:
Line 81:
Unguarded external repository call violates Rule 28, which requires wrapping external calls in try/catch with structured context before mapping to application-level errors. Wrap the call in try/catch, log `userId` and operation context, and transduce the exception to an application-level error before propagation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Logging.LogException(ex, | ||
| $"CalendarRsvp response transaction failed. messageId={messageId}, calendarItemId={calendarItemId}, UserId={UserId}."); |
There was a problem hiding this comment.
String interpolation in the error log bakes identifiers into the message template, defeating structured logging required by SIEM tools for filtering and aggregation. Pass a message template with named placeholders ({MessageId}, {CalendarItemId}, {UserId}) and supply values as separate arguments to Logging.LogException or ILogger.LogError.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs:
Line 317 to 318:
String interpolation in the error log bakes identifiers into the message template, defeating structured logging required by SIEM tools for filtering and aggregation. Pass a message template with named placeholders (`{MessageId}`, `{CalendarItemId}`, `{UserId}`) and supply values as separate arguments to `Logging.LogException` or `ILogger.LogError`.
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}
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
2 participants
Pull Request Description
This PR delivers several bug fixes and feature enhancements (RIC-T39) across notification settings, calendar RSVP messaging, and check-in timer functionality.
User-Level Modern Application Sounds
Introduces a per-user toggle (
EnableModernApplicationSounds) that lets individuals opt into modern notification sounds independently of the department-wide setting. The effective setting is resolved by combining both department and user preferences—if either is enabled, modern sounds are used. The setting is exposed through the API config endpoint, applied to all push notification paths (messages, notifications, chats, calls), and added to the profile edit page with full localization across 9 languages.Calendar RSVP Messages
Adds support for a new
CalendarRsvpmessage type that allows users to respond to calendar event invitations directly from a message. When a user responds (attending/not attending), the system updates both the message recipient record and the corresponding calendar event attendee. This includes API endpoint support, web UI RSVP buttons in the message view, and proper authorization checks. System-generated messages now display "System" as the sender instead of the generating user.Check-In Timer: Commander Check-In on Behalf of Personnel
Enables incident commanders with the
ManageAccountabilitycapability to perform personnel check-ins on behalf of dispatched users. The feature validates that the target user is actually dispatched on the call and restricts this to personnel-type timers only.Message Inbox Fixes
Fixes issues where deleted and expired messages were still appearing in user inboxes and unread counts. The
MessageServicenow filters out deleted and expired messages, and expired messages can no longer be viewed or responded to.Summary by CodeRabbit