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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds moderation workflows across storage, services, APIs, controllers, and chat interfaces. It also adds localization, moderation state, legacy-data migrations, notification handling, validation changes, and supporting database updates. ChangesModeration platform
Supporting behavior updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Reporter
participant ModerationController
participant ModerationService
participant ModerationRepository
participant Moderator
Reporter->>ModerationController: submit moderation report
ModerationController->>ModerationService: flag content
ModerationService->>ModerationRepository: store request and report
ModerationRepository-->>ModerationService: return moderation data
ModerationService-->>Reporter: return report status
Moderator->>ModerationController: search or complete request
ModerationController->>ModerationService: authorize and complete
ModerationService->>ModerationRepository: store action and status
ModerationService-->>Moderator: return completed request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 1
🤖 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/NotificationService.cs`:
- Around line 507-511: Update the single-select comparisons in the notification
matching logic around the existing before/current data branches to test
normalized values for exact equality with "-1", not substring containment. Apply
this consistently to the early wildcard match and both beforeAny/currentAny
assignments, including the corresponding branches around the additional affected
locations, so values such as "-10" are not treated as the wildcard.
🪄 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: 9c388c78-29fd-42a9-9a14-01652d4b0775
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Services/NotificationServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (2)
Core/Resgrid.Services/NotificationService.csWeb/Resgrid.Web/wwwroot/js/app/internal/notifications/resgrid.notifications.addNotification.js
|
|
||
| if ((currentAny || currentState.State == int.Parse(setting.CurrentData)) && | ||
| (beforeAny || beforeState.State == int.Parse(setting.BeforeData))) | ||
| if ((currentAny || currentState.State == int.Parse(currentData)) && |
There was a problem hiding this comment.
Unsafe string conversion: int.Parse(currentData) is used without TryParse validation for user/IO input. Prefer int.TryParse and validate culture/format where applicable.
Kody rule violation: Use TryParse for string conversions
Prompt for LLM
File Core/Resgrid.Services/NotificationService.cs:
Line 521:
Unsafe string conversion: `int.Parse(currentData)` is used without `TryParse` validation for user/IO input. Prefer `int.TryParse` and validate culture/format where applicable.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (setting.BeforeData.Contains("-1") && setting.CurrentData.Contains("-1")) | ||
| // Empty Before/Current data means "Any": the post-Telerik UI posts "" for the | ||
| // default Any option, so settings saved that way must still match every change. | ||
| var beforeData = String.IsNullOrWhiteSpace(setting.BeforeData) ? "-1" : setting.BeforeData; |
There was a problem hiding this comment.
Magic string literal "-1" is scattered across NotificationService.cs (lines 505–565) and resgrid.notifications.addNotification.js (lines 132–146) as a sentinel meaning "Any" without a named constant. Define a class-level constant private const string AnySelection = "-1"; and replace all inline occurrences.
Kody rule violation: Centralize string constants
Prompt for LLM
File Core/Resgrid.Services/NotificationService.cs:
Line 504:
Magic string literal `"-1"` is scattered across `NotificationService.cs` (lines 505–565) and `resgrid.notifications.addNotification.js` (lines 132–146) as a sentinel meaning "Any" without a named constant. Define a class-level constant `private const string AnySelection = "-1";` and replace all inline occurrences.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (setting.BeforeData.Contains("-1") && setting.CurrentData.Contains("-1")) | ||
| // Empty Before/Current data means "Any": the post-Telerik UI posts "" for the | ||
| // default Any option, so settings saved that way must still match every change. | ||
| var beforeData = String.IsNullOrWhiteSpace(setting.BeforeData) ? "-1" : setting.BeforeData; |
There was a problem hiding this comment.
Triplicated logic: the normalization-plus-state-comparison sequence is duplicated verbatim across the UnitStatusChanged, PersonnelStaffingChanged, and PersonnelStatusChanged cases. Extract a single generic helper such as ValidateStateChangeAsync<TState>(setting, Func<int, Task<TState>> getCurrent, Func<TState, Task<TState>> getBefore, Func<TState,int> stateSelector) and call it from each case.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Core/Resgrid.Services/NotificationService.cs:
Line 504:
Triplicated logic: the normalization-plus-state-comparison sequence is duplicated verbatim across the `UnitStatusChanged`, `PersonnelStaffingChanged`, and `PersonnelStatusChanged` cases. Extract a single generic helper such as `ValidateStateChangeAsync<TState>(setting, Func<int, Task<TState>> getCurrent, Func<TState, Task<TState>> getBefore, Func<TState,int> stateSelector)` and call it from each case.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| DepartmentId = 1, | ||
| MessageId = "123456", | ||
| Data = new NotificationItem() { StateId = 3, DepartmentId = 1, PreviousStateId = 2 }.SerializeProto(), |
There was a problem hiding this comment.
Magic numbers StateId=3 and PreviousStateId=2 lack self-documenting domain meaning. Use enum casts such as StateId=(int)UnitStateTypes.Responding, consistent with existing BeforeData casts in the same test.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Tests/Resgrid.Tests/Services/NotificationServiceTests.cs:
Line 771:
Magic numbers `StateId=3` and `PreviousStateId=2` lack self-documenting domain meaning. Use enum casts such as `StateId=(int)UnitStateTypes.Responding`, consistent with existing `BeforeData` casts in the same test.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| $('#beforeStateControl').empty().append('<select id="Notification_BeforeData" name="Notification.BeforeData" style="width:100%"></select>'); | ||
| $('#currentStateControl').empty().append('<select id="Notification_CurrentData" name="Notification.CurrentData" style="width:100%"></select>'); | ||
| var url = resgrid.absoluteBaseUrl + '/User/CustomStatuses/GetUnitStatusesLevelsForDepartmentCombined?includeAny=True'; | ||
| var url = resgrid.absoluteBaseUrl + '/User/CustomStatuses/GetUnitStatusesLevelsForDepartmentCombined?includeAny=False'; |
There was a problem hiding this comment.
String concatenation using + violates the team template literals rule. Replace with a template literal to improve readability and reduce error-proneness.
Kody rule violation: Use Template Literals Instead of String Concatenation
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/notifications/resgrid.notifications.addNotification.js:
Line 132:
String concatenation using `+` violates the team template literals rule. Replace with a template literal to improve readability and reduce error-proneness.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var $sel = $(selector).empty().append('<option value="">-- Any --</option>'); | ||
| // "Any" must post "-1", not "" — the notification engine treats the value as a | ||
| // state id and an empty string used to make the setting never match. | ||
| var $sel = $(selector).empty().append('<option value="-1">-- Any --</option>'); |
There was a problem hiding this comment.
var declaration of $sel violates Rule [37] and risks function-scoping pitfalls. Use const since $sel is never reassigned.
Kody rule violation: Always use const and let
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/notifications/resgrid.notifications.addNotification.js:
Line 119:
`var` declaration of `$sel` violates Rule [37] and risks function-scoping pitfalls. Use `const` since `$sel` is never reassigned.
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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public async Task<ActionResult<GetCallResult>> GetCall(string callId, [FromQuery] string departmentId = null) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(callId)) | ||
| if (!int.TryParse(callId, NumberStyles.Integer, CultureInfo.InvariantCulture, out int parsedCallId)) |
| var result = new EditCallResult(); | ||
|
|
||
| var canDoOperation = await _authorizationService.CanUserEditCallAsync(UserId, int.Parse(editCallInput.Id)); | ||
| if (editCallInput == null || !ModelState.IsValid || |
|
|
||
| var canDoOperation = await _authorizationService.CanUserEditCallAsync(UserId, int.Parse(editCallInput.Id)); | ||
| if (editCallInput == null || !ModelState.IsValid || | ||
| !int.TryParse(editCallInput.Id, NumberStyles.Integer, CultureInfo.InvariantCulture, out int callId)) |
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status401Unauthorized)] | ||
| public async Task<ActionResult<ModerationActionResult>> Flag([FromBody] FlagModerationInput input, |
| public async Task<ActionResult<ModerationActionResult>> Flag([FromBody] FlagModerationInput input, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| if (!ModelState.IsValid || input == null) |
|
|
||
| /// <summary>Completes a scoped request with no action or by removing the live content.</summary> | ||
| [HttpPost("Complete")] | ||
| public async Task<ActionResult<ModerationActionResult>> Complete(string requestId, |
| public async Task<ActionResult<ModerationActionResult>> Complete(string requestId, | ||
| [FromBody] CompleteModerationInput input, CancellationToken cancellationToken) | ||
| { | ||
| if (!ModelState.IsValid || input == null || string.IsNullOrWhiteSpace(requestId)) |
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs (1)
1461-1486: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
FlagMessagedoes not handle the exceptionsFlagAsyncthrows.
ModerationService.FlagAsyncthrows in cases this endpoint can reach:
InvalidOperationExceptionwhen the chat message is deleted.CheckMessageChannelAccessAsyncdoes not inspectDeletedOn, so flagging a tombstoned message reachesLoadEvidenceAsyncand throws.ArgumentOutOfRangeExceptionwheninput.Reasonis outside theModerationReasonrange.FlagMessageInput.Reasonis a plainint.UnauthorizedAccessExceptionfromLoadEvidenceAsync.None are caught, so each returns 500.
ModerationController.Flagcatches all three and maps them to 400 or 401. Mirror that handling here.The cast
(ModerationReason)input.Reasonalso couples two independently declared enums. The values align today. Map them explicitly so a future change to either enum fails at compile time instead of silently mislabelling a report.🛠️ Proposed fix
var result = new ChatActionResult(); - var flag = await _moderationService.FlagAsync(DepartmentId, UserId, - ModerationItemType.ChatMessage, messageId, (ModerationReason)input.Reason, input.Note, - BuildModerationContext("Reporter"), cancellationToken); + ModerationReport flag; + + try + { + flag = await _moderationService.FlagAsync(DepartmentId, UserId, + ModerationItemType.ChatMessage, messageId, (ModerationReason)input.Reason, input.Note, + BuildModerationContext("Reporter"), cancellationToken); + } + catch (UnauthorizedAccessException) + { + return Unauthorized(); + } + catch (ArgumentException ex) + { + return BadRequest(ex.Message); + } + catch (InvalidOperationException ex) + { + return BadRequest(ex.Message); + } result.Success = flag != null;
ArgumentOutOfRangeExceptionderives fromArgumentException, so theArgumentExceptioncatch covers the invalid-reason case.🤖 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/ChatController.cs` around lines 1461 - 1486, Update FlagMessage to explicitly map input.Reason to the corresponding ModerationReason value instead of directly casting between enums, and wrap FlagAsync in exception handling matching ModerationController.Flag: map ArgumentException and InvalidOperationException to BadRequest, UnauthorizedAccessException to Unauthorized, and preserve the existing success response for successful flags.Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chatStore.ts (1)
341-345: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCopy
IsModeratedfor deleted thread replies.The channel-message branch copies moderation state from
HubDeletedPayload, but the thread-reply branch does not. A moderator-deleted reply remainsIsModerated: falsein local state. Apply the samepayload.IsModerated ?? payload.DeletedByModeratorvalue in this branch.🤖 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/chatStore.ts` around lines 341 - 345, Update the thread-reply handling in the loop over state.threadMessagesByRoot to include IsModerated from payload.IsModerated ?? payload.DeletedByModerator when calling upsertThreadMessage, alongside DeletedOn and Body. Preserve the existing reply lookup and early return behavior.Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs (1)
37-56: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the required dependency-resolution pattern.
These changes add constructor injection for new dependencies. Resolve the dependencies with
Bootstrapper.GetKernel().Resolve<T>()in each constructor instead.
Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs#L37-L56: ResolveIModerationServiceand the moderation localizer through the required service locator.Web/Resgrid.Web/Areas/User/Controllers/ModerationController.cs#L13-L18: ResolveIDepartmentGroupsServicethrough the required service locator.As per coding guidelines, use
Bootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors rather than constructor injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs` around lines 37 - 56, Replace constructor injection with explicit service-locator resolution in MessagesController: remove the IModerationService and moderation localizer parameters and initialize both fields via Bootstrapper.GetKernel().Resolve<T>(). In Web/Resgrid.Web/Areas/User/Controllers/ModerationController.cs lines 13-18, resolve IDepartmentGroupsService through Bootstrapper.GetKernel().Resolve<T>() instead of injecting it; update each constructor accordingly.Source: Coding guidelines
🧹 Nitpick comments (8)
Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs (1)
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
IChatModerationServicedependency fromChatController.
_chatModerationServiceand its constructor parameter are no longer referenced byChatController; removing them reduces unnecessary injected dependencies and the unused registration can be cleaned up separately if this is its only remaining direct dependency.🤖 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/ChatController.cs` around lines 45 - 46, Remove the unused IChatModerationService field and its constructor parameter from ChatController, and update constructor assignments and calls accordingly while preserving the existing IModerationService dependency.Source: Coding guidelines
Web/Resgrid.Web.Services/Models/v4/Moderation/ModerationApiModels.cs (1)
78-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive moderation ranges from the enums.
FlagAsyncandCompleteRequestAsyncuseEnum.IsDefined/explicit checks forModerationItemType,ModerationReason, and the acceptedModerationDispositionvalues, while the API input uses literalRangeconstraints. If a future enum value is added, validation can block it at the controller before the service rejects it. Bind these properties to their enum types or validate withEnum.IsDefinedso the enum remains the source of truth.Note limits are not an issue here: both migration definitions use
text/int.MaxValue.🤖 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/Models/v4/Moderation/ModerationApiModels.cs` around lines 78 - 101, Replace the literal Range constraints on FlagModerationInput.ItemType, FlagModerationInput.Reason, and CompleteModerationInput.Disposition with enum-based validation using their corresponding moderation enums, preferably by changing the properties to those enum types or applying Enum.IsDefined validation. Preserve the existing note and ItemId validation.Core/Resgrid.Services/ChatMessageService.cs (1)
316-324: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove the duplicate moderator flag from the deletion event payload.
DeletedByModeratornow carries the same rawasModeratorvalue asIsModerated, so moderators deleting their own messages publish both fields as true. The chat client derives the display state fromIsModerated, so keep that field and removeDeletedByModeratorfrom the event and type.🤖 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/ChatMessageService.cs` around lines 316 - 324, Update the deletion event payload in the message deletion flow around PublishEvent to remove DeletedByModerator = asModerator while retaining message.IsModerated. Remove the corresponding DeletedByModerator property from the event payload type and update any affected consumers to use IsModerated only.Web/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/ModerationRequestsTable.tsx (2)
246-246: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConfirm the destructive action before it runs.
The
RemoveContentbutton callscomplete(request, 2)on the first click. The moderation service removes the live content for that disposition, and the table offers no undo. Add a confirmation step, so a mis-click does not delete a message, a call note, or a call image.🤖 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/moderation/ModerationRequestsTable.tsx` at line 246, Update the RemoveContent button in ModerationRequestsTable so it asks the user for confirmation before invoking complete(request, 2). Only call complete after confirmation is accepted; preserve the existing isBusy disabled state and behavior for other moderation actions.
71-75: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndex the personnel list once.
personNameruns a linearpeople.findfor every content author, every report, and every audit action. With a page of 100 requests and a large personnel roster, the render performs thousands of scans. Build aMaponce and read from it.♻️ Proposed refactor
+ const peopleById = useMemo( + () => new Map(people.map((item) => [item.userId, item.name])), + [people], + ); + const personName = useCallback((userId?: string | null) => { if (!userId) return moderationText('SystemOrUnknown'); - const person = people.find((item) => item.userId === userId); - return person ? `${person.name} (${userId})` : userId; - }, [people]); + const name = peopleById.get(userId); + return name ? `${name} (${userId})` : userId; + }, [peopleById]);🤖 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/moderation/ModerationRequestsTable.tsx` around lines 71 - 75, Update the personnel lookup used by personName to build a Map keyed by userId once per people change, then read entries from that Map instead of calling people.find for each author, report, or audit action. Preserve the existing SystemOrUnknown fallback and display formatting for found and missing users.Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs (2)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op ternary.
Both branches return
"r", and every generated statement already hard-codes theralias. Replace the variable with the literal at line 153.♻️ Proposed cleanup
- var requestAlias = postgres ? "r" : "r"; var filters = new List<string>();Then use the literal alias in the PostgreSQL statement:
-FROM {_sqlConfiguration.SchemaName}.moderationrequests {requestAlias} +FROM {_sqlConfiguration.SchemaName}.moderationrequests r🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs` at line 69, Remove the no-op requestAlias assignment in the moderation repository and replace its usage in the PostgreSQL statement around the generated query with the literal “r” alias. Preserve the existing SQL behavior and remove the now-unused variable.
180-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
_unitOfWorknull handling consistent.Line 185 tests
_unitOfWork?.Connection, which states that_unitOfWorkcan be null. Line 183 then reads_unitOfWork.Transactioninside the delegate, and line 192 calls_unitOfWork.CreateOrGetConnection(). If_unitOfWorkwere ever null, the delegate throws aNullReferenceExceptionwhen it runs, not at line 185. The same mix exists at lines 244/246 and 298/300.The constructors require the dependency, so remove the null-conditional operator, or guard the whole method.
Also applies to: 243-253, 297-307
🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs` around lines 180 - 193, Make null handling consistent in QueryAsync and the corresponding methods around the later query blocks: since constructors require _unitOfWork, remove the null-conditional checks and use _unitOfWork.Connection directly, or consistently guard the entire method before accessing _unitOfWork.Transaction and CreateOrGetConnection(). Apply the same correction to all three affected query methods.Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chatFormat.ts (1)
192-192: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the formatter out of the function.
formatRelativeDayruns once per rendered message and once per moderation or export table row. Each call with a one-day-old date constructs a newIntl.RelativeTimeFormat. Create the formatter once at module scope and reuse it.Note that the output is now lowercase in English, for example "yesterday" instead of the previous "Yesterday". Confirm that reads correctly in the cells that show it.
♻️ Proposed change
+const relativeDayFormatter = new Intl.RelativeTimeFormat(undefined, { numeric: 'auto' }); + export function formatRelativeDay(iso: string | null | undefined): string {if (diffDays === 1) { - return new Intl.RelativeTimeFormat(undefined, { numeric: 'auto' }).format(-1, 'day'); + return relativeDayFormatter.format(-1, 'day'); }🤖 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/chatFormat.ts` at line 192, Hoist the Intl.RelativeTimeFormat instance used by formatRelativeDay to module scope and reuse it for each one-day-old date instead of constructing it per call. Preserve the relative-day output, and verify the resulting lowercase English text such as “yesterday” reads correctly in message, moderation, and export table cells.
🤖 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/Repositories/IChatRepositories.cs`:
- Around line 147-148: Update the delete flow that calls TombstoneAsync so it
derives one effective moderator/sender flag and reuses it for the TombstoneAsync
isModerated argument, message.IsModerated, and delete-event audit type;
alternatively block moderator self-deletion consistently. Ensure persisted
tombstone state and audit history reflect the same actor classification.
In `@Core/Resgrid.Services/ChatMessageService.cs`:
- Around line 303-313: Make the edit-history type and IsModerated assignment in
DeleteMessageAsync use the same moderator-deletion condition, including the
isSender case consistently. Derive both from one shared condition so
moderator-authored deletions cannot produce SenderDelete while setting
IsModerated to true.
In `@Core/Resgrid.Services/ModerationService.cs`:
- Around line 519-542: The HydrateAsync loop performs per-request report and
action queries, causing excessive sequential database round trips. Add batch
repository lookup methods accepting the collected ModerationRequestId values,
call each once, group the returned reports and actions by request ID in memory,
and use those groups while preserving ApplyGroupScope and null-scope behavior.
- Around line 338-343: Update LoadEvidenceAsync to retain and load every
attachment returned by GetMetadataByMessageIdsAsync instead of selecting only
FirstOrDefault, and pass the complete attachment collection through the evidence
model so RemoveLiveContentAsync preserves all attachments in the audit trail. If
the surrounding API cannot support multiple attachments, document the enforced
single-attachment limitation in LoadEvidenceAsync instead.
- Around line 272-282: Make CompleteRequestAsync persist content removal and the
moderation request status transition atomically, so RemoveLiveContentAsync
cannot leave destructive changes committed when
_moderationRequestRepository.UpdateAsync fails. Use the existing
transaction/unit-of-work mechanism around both operations; preserve the current
failure behavior and only finalize the transaction after the removal and request
update succeed.
- Around line 110-124: Update both insert exception handlers in the moderation
request flow, including the blocks around InsertAsync and the reporter lookup,
to catch OperationCanceledException separately and rethrow it so cancellation
propagates. Restrict the general handler to non-cancellation exceptions, log
each swallowed insert failure with Resgrid.Framework.Logging.LogException, then
retain the existing concurrent-row recovery and rethrow behavior. Add the
required Resgrid.Framework import.
- Around line 654-689: Update NotifyReportersAsync to verify the result returned
by SaveMessageAsync before passing it to SendMessageAsync; skip sending when the
saved message is null so notification failure cannot throw. Move the
per-recipient profile lookup, message persistence, and send/enqueue work out of
CompleteRequestAsync’s synchronous completion path by dispatching the
notification fan-out through the existing background queue mechanism, while
preserving recipient filtering and cancellation behavior.
In `@Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs`:
- Around line 177-179: Replace manual metadata JSON string concatenation in the
migration’s notification metadata construction, including the logic around
n.Source, n.Latitude, n.Longitude, and the lines handling names, with SQL
Server’s JSON API such as FOR JSON or JSON_OBJECT. Ensure all values are
serialized with valid JSON escaping and numeric formatting, while preserving the
existing metadata fields and fallback values.
- Around line 118-127: Update M0112_AddModeration.cs at
Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs:118-127
and M0112_AddModerationPg.cs at
Providers/Resgrid.Providers.MigrationsPg/Migrations/M0112_AddModerationPg.cs:118-127
by guarding each foreign-key creation with a constraint-existence check, and
invoke ImportLegacyFlags() only when ModerationRequests/moderationrequests
contains no rows. Apply the equivalent checks and condition in both migration
implementations.
In `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs`:
- Around line 41-45: Update the SQL built by GetByItemAsync to replace SELECT *
with the same explicit non-blob moderation request column list used by
SearchAsync and ModerationActionRepository.GetByRequestAsync. Exclude
OriginalContent and any other evidence blob columns while preserving the
existing PostgreSQL and non-PostgreSQL table and filter syntax.
In `@Web/Resgrid.Web.Services/Controllers/v4/ModerationController.cs`:
- Around line 190-206: Update DownloadEvidence to accept a CancellationToken and
pass it through to the moderation service calls, matching Flag and Complete.
Handle UnauthorizedAccessException from RecordEvidenceAccessAsync by returning
Unauthorized instead of allowing a 500, while preserving the existing
no-disclosure behavior. Also revise the method summary to describe general
evidence or retained content rather than only image evidence.
In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPageElement.tsx`:
- Around line 59-65: Update openFlag to ignore stale getMyModerationRequest
responses when the selected ChatMessageId changes, using a request token or
matching the response to the current target before calling setFlagStatus. Apply
the same guard to success and failure handlers so an earlier lookup cannot alter
the second message’s dialog state.
In
`@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/ModerationRequestsTable.tsx`:
- Line 208: Update the conditional rendering for request.CallId and
report.ReporterGroupId to explicitly check that each value is neither null nor
undefined, preventing a numeric zero from rendering as text while preserving
rendering for valid zero and nonzero identifiers.
- Around line 77-86: Update ModerationRequestsTable’s pagination flow so
moderators can access results beyond the hard-coded first 100 records: add page
state and previous/next controls that update search.page, while keeping pageSize
within the repository’s 200-row cap. Reuse the existing ActionsTab pagination
behavior and ensure controls reflect whether another page is available based on
the returned result count.
- Line 270: Replace the dynamic moderationText key construction for
action.ActorRole in ModerationRequestsTable with an explicit
role-to-localization-key map and a defined fallback for unmapped roles,
following the existing ITEM_LABEL_KEYS and ACTION_LABEL_KEYS pattern. Preserve
the UnknownRole behavior when ActorRole is absent and ensure persisted values
such as LegacyImport do not render as raw localization keys.
In `@Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs`:
- Line 74: Remove the IModerationService parameter from the DispatchController
constructor and initialize the existing _moderationService field by resolving
IModerationService through
Bootstrapper.GetKernel().Resolve<IModerationService>() inside the constructor.
Update all affected constructor call sites while preserving the controller’s
existing behavior.
- Around line 1792-1793: Update the call-note handling in DispatchController to
load the current reporter’s moderation requests once before the note loop,
instead of awaiting GetReporterRequestAsync for each note. Build a set of
flagged CallNote item IDs from that result, then assign note.IsFlagged by
checking callNote.CallNoteId against the set while preserving the existing
department, user, and moderation item-type filters.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs`:
- Around line 1461-1486: Update FlagMessage to explicitly map input.Reason to
the corresponding ModerationReason value instead of directly casting between
enums, and wrap FlagAsync in exception handling matching
ModerationController.Flag: map ArgumentException and InvalidOperationException
to BadRequest, UnauthorizedAccessException to Unauthorized, and preserve the
existing success response for successful flags.
In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chatStore.ts`:
- Around line 341-345: Update the thread-reply handling in the loop over
state.threadMessagesByRoot to include IsModerated from payload.IsModerated ??
payload.DeletedByModerator when calling upsertThreadMessage, alongside DeletedOn
and Body. Preserve the existing reply lookup and early return behavior.
In `@Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs`:
- Around line 37-56: Replace constructor injection with explicit service-locator
resolution in MessagesController: remove the IModerationService and moderation
localizer parameters and initialize both fields via
Bootstrapper.GetKernel().Resolve<T>(). In
Web/Resgrid.Web/Areas/User/Controllers/ModerationController.cs lines 13-18,
resolve IDepartmentGroupsService through Bootstrapper.GetKernel().Resolve<T>()
instead of injecting it; update each constructor accordingly.
---
Nitpick comments:
In `@Core/Resgrid.Services/ChatMessageService.cs`:
- Around line 316-324: Update the deletion event payload in the message deletion
flow around PublishEvent to remove DeletedByModerator = asModerator while
retaining message.IsModerated. Remove the corresponding DeletedByModerator
property from the event payload type and update any affected consumers to use
IsModerated only.
In `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs`:
- Line 69: Remove the no-op requestAlias assignment in the moderation repository
and replace its usage in the PostgreSQL statement around the generated query
with the literal “r” alias. Preserve the existing SQL behavior and remove the
now-unused variable.
- Around line 180-193: Make null handling consistent in QueryAsync and the
corresponding methods around the later query blocks: since constructors require
_unitOfWork, remove the null-conditional checks and use _unitOfWork.Connection
directly, or consistently guard the entire method before accessing
_unitOfWork.Transaction and CreateOrGetConnection(). Apply the same correction
to all three affected query methods.
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs`:
- Around line 45-46: Remove the unused IChatModerationService field and its
constructor parameter from ChatController, and update constructor assignments
and calls accordingly while preserving the existing IModerationService
dependency.
In `@Web/Resgrid.Web.Services/Models/v4/Moderation/ModerationApiModels.cs`:
- Around line 78-101: Replace the literal Range constraints on
FlagModerationInput.ItemType, FlagModerationInput.Reason, and
CompleteModerationInput.Disposition with enum-based validation using their
corresponding moderation enums, preferably by changing the properties to those
enum types or applying Enum.IsDefined validation. Preserve the existing note and
ItemId validation.
In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chatFormat.ts`:
- Line 192: Hoist the Intl.RelativeTimeFormat instance used by formatRelativeDay
to module scope and reuse it for each one-day-old date instead of constructing
it per call. Preserve the relative-day output, and verify the resulting
lowercase English text such as “yesterday” reads correctly in message,
moderation, and export table cells.
In
`@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/ModerationRequestsTable.tsx`:
- Line 246: Update the RemoveContent button in ModerationRequestsTable so it
asks the user for confirmation before invoking complete(request, 2). Only call
complete after confirmation is accepted; preserve the existing isBusy disabled
state and behavior for other moderation actions.
- Around line 71-75: Update the personnel lookup used by personName to build a
Map keyed by userId once per people change, then read entries from that Map
instead of calling people.find for each author, report, or audit action.
Preserve the existing SystemOrUnknown fallback and display formatting for found
and missing users.
🪄 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: 5f4c2e9b-e2eb-408b-8004-393cdc24a872
⛔ Files ignored due to path filters (36)
Core/Resgrid.Localization/Areas/User/Dispatch/Call.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.uk.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Moderation/Moderation.uk.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.en.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/Chatbot/ChatbotDeptConfigAndSessionTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Localization/ModerationLocalizationTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Models/FormAutomationTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/MessageServiceInboxTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/ModerationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/NotificationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/CallsControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/MessagesControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/User/SubscriptionControllerTests.csis excluded by!**/Tests/**
📒 Files selected for processing (69)
Core/Resgrid.Localization/Areas/User/Moderation/Moderation.csCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/Chat/ChatMessage.csCore/Resgrid.Model/ChatbotDepartmentConfig.csCore/Resgrid.Model/FormAutomation.csCore/Resgrid.Model/Message.csCore/Resgrid.Model/Moderation/Moderation.csCore/Resgrid.Model/Repositories/IChatRepositories.csCore/Resgrid.Model/Repositories/IModerationRepositories.csCore/Resgrid.Model/Services/IModerationService.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/ChatMessageService.csCore/Resgrid.Services/MessageService.csCore/Resgrid.Services/ModerationService.csCore/Resgrid.Services/NotificationService.csCore/Resgrid.Services/Resgrid.Services.csprojCore/Resgrid.Services/ServicesModule.csProviders/Resgrid.Providers.Migrations/Migrations/M0111_CascadeCommunicationTestDeletes.csProviders/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0111_CascadeCommunicationTestDeletesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0112_AddModerationPg.csRepositories/Resgrid.Repositories.DataRepository/ChatRepositories.csRepositories/Resgrid.Repositories.DataRepository/ModerationRepositories.csRepositories/Resgrid.Repositories.DataRepository/Modules/ApiDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/NonWebDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/ChatController.csWeb/Resgrid.Web.Services/Controllers/v4/MessagesController.csWeb/Resgrid.Web.Services/Controllers/v4/ModerationController.csWeb/Resgrid.Web.Services/Models/v4/Calls/EditCallInput.csWeb/Resgrid.Web.Services/Models/v4/Chat/ChatApiModels.csWeb/Resgrid.Web.Services/Models/v4/Moderation/ModerationApiModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatModerationElement.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPageElement.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/FlagDialog.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/MessageBubble.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chat.cssWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chatFormat.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chatStore.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/ActionsTab.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/ExportsTab.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/FlagsTab.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/ModerationRequestsTable.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/ReportsTab.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderation/SettingsTab.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderationApi.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/moderationI18n.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/types.tsWeb/Resgrid.Web/Areas/User/Apps/src/elements.tsWeb/Resgrid.Web/Areas/User/Controllers/ChatController.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Controllers/MessagesController.csWeb/Resgrid.Web/Areas/User/Controllers/ModerationController.csWeb/Resgrid.Web/Areas/User/Controllers/SubscriptionController.csWeb/Resgrid.Web/Areas/User/Models/Dispatch/FlagCallImageView.csWeb/Resgrid.Web/Areas/User/Models/Dispatch/FlagCallNoteView.csWeb/Resgrid.Web/Areas/User/Models/Messages/ViewMessageView.csWeb/Resgrid.Web/Areas/User/Views/Chat/Moderation.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/FlagCallImage.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/FlagCallNote.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Messages/ViewMessage.cshtmlWeb/Resgrid.Web/Areas/User/Views/Moderation/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_Navigation.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_UserLayout.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/templates/resgrid.templates.newtemplate.js
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/Resgrid.Services/NotificationService.cs
| ChatAttachment attachment = null; | ||
| var attachmentMetadata = await _chatAttachmentRepository.GetMetadataByMessageIdsAsync(new[] { itemId }); | ||
| var firstAttachment = attachmentMetadata?.FirstOrDefault(); | ||
| if (firstAttachment != null) | ||
| attachment = await _chatAttachmentRepository.GetByIdAsync(firstAttachment.ChatAttachmentId); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Only the first attachment is captured as evidence.
LoadEvidenceAsync reads the attachment metadata list for the chat message and keeps FirstOrDefault(). If the reported message carries more than one attachment, the remaining attachments are never captured. RemoveLiveContentAsync still tombstones the whole message, so the uncaptured attachments are lost from the audit trail.
Capture every attachment, or document the single-attachment limit in the method comment.
🤖 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/ModerationService.cs` around lines 338 - 343, Update
LoadEvidenceAsync to retain and load every attachment returned by
GetMetadataByMessageIdsAsync instead of selecting only FirstOrDefault, and pass
the complete attachment collection through the evidence model so
RemoveLiveContentAsync preserves all attachments in the audit trail. If the
surrounding API cannot support multiple attachments, document the enforced
single-attachment limitation in LoadEvidenceAsync instead.
| <strong>{moderationText(ACTION_LABEL_KEYS[action.ActionType] ?? 'Action')}</strong> {moderationText('By')} {personName(action.PerformedByUserId)} | ||
| <div>{action.Note || moderationText('NoNote')}</div> | ||
| <div className="rgchat-convo__sub"> | ||
| {formatTimestamp(action.PerformedOn)} · {action.ActorRole ? moderationText(`Actor${action.ActorRole}`) : moderationText('UnknownRole')} · {action.IpAddress || moderationText('NoIp')} · {moderationText('TraceFormat', action.TraceId || moderationText('NotAvailable'))} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not build a localization key from stored server data.
moderationText(Actor${action.ActorRole}) derives the key from the persisted ActorRole string. moderationText returns the key itself when the entry is missing, so an unmapped role renders a raw identifier. The SQL Server and PostgreSQL migrations both write ActorRole = 'LegacyImport' for every imported action, which produces the key ActorLegacyImport.
Use an explicit map with a fallback, like ITEM_LABEL_KEYS and ACTION_LABEL_KEYS do for the numeric enums.
🤖 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/moderation/ModerationRequestsTable.tsx`
at line 270, Replace the dynamic moderationText key construction for
action.ActorRole in ModerationRequestsTable with an explicit
role-to-localization-key map and a defined fallback for unmapped roles,
following the existing ITEM_LABEL_KEYS and ACTION_LABEL_KEYS pattern. Preserve
the UnknownRole behavior when ActorRole is absent and ensure persisted values
such as LegacyImport do not render as raw localization keys.
| private readonly ICheckInTimerService _checkInTimerService; | ||
| private readonly IWeatherAlertService _weatherAlertService; | ||
| private readonly ICallDispatchStatusService _callDispatchStatusService; | ||
| private readonly IModerationService _moderationService; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve IModerationService through the required service locator.
Line 87 adds constructor injection for IModerationService. Remove this parameter. Resolve the service in the constructor with Bootstrapper.GetKernel().Resolve<IModerationService>().
As per coding guidelines, use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection.
Also applies to: 87-87, 118-118
🤖 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/DispatchController.cs` at line 74,
Remove the IModerationService parameter from the DispatchController constructor
and initialize the existing _moderationService field by resolving
IModerationService through
Bootstrapper.GetKernel().Resolve<IModerationService>() inside the constructor.
Update all affected constructor call sites while preserving the controller’s
existing behavior.
Source: Coding guidelines
|
|
||
| return resourceSet.Cast<DictionaryEntry>() | ||
| .Where(x => x.Key is string && x.Value is string) | ||
| .ToDictionary(x => (string)x.Key, x => (string)x.Value!); |
There was a problem hiding this comment.
Unsafe type casting violates team rule. 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 Core/Resgrid.Localization/Areas/User/Moderation/Moderation.cs:
Line 49:
Unsafe type casting violates team rule. 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.
| { | ||
| var cultureInfo = GetSupportedCulture(culture); | ||
| var value = ResourceManager.GetString(key, cultureInfo) | ||
| ?? ResourceManager.GetString(key, CultureInfo.GetCultureInfo("en")) |
There was a problem hiding this comment.
Magic string "en" for the default culture is repeated across multiple locations, risking inconsistency during changes. Define a private constant like private const string DefaultCulture = "en"; in ModerationResources and reference it.
Kody rule violation: Centralize string constants
Prompt for LLM
File Core/Resgrid.Localization/Areas/User/Moderation/Moderation.cs:
Line 28:
Magic string `"en"` for the default culture is repeated across multiple locations, risking inconsistency during changes. Define a private constant like `private const string DefaultCulture = "en";` in `ModerationResources` and reference it.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } | ||
|
|
||
| private async Task<List<ModerationRequest>> HydrateAsync(IEnumerable<ModerationRequest> requests, |
There was a problem hiding this comment.
Query amplification in HydrateAsync issues two sequential database round-trips (GetByRequestAsync for reports and actions) per request, causing up to 400 queries on a 200-item page. Batch-load reports and actions using WHERE ModerationRequestId IN (...) and group them in memory.
var requestIds = result.Select(x => x.ModerationRequestId).ToList();
var allReports = await _moderationReportRepository.GetByRequestIdsAsync(requestIds);
var allActions = await _moderationActionRepository.GetByRequestIdsAsync(requestIds);
var reportsByRequest = allReports.ToLookup(x => x.ModerationRequestId);
var actionsByRequest = allActions.ToLookup(x => x.ModerationRequestId);
foreach (var request in result)
{
var reports = reportsByRequest[request.ModerationRequestId].ToList();
var actions = actionsByRequest[request.ModerationRequestId].ToList();
if (visibleGroupIds == null)
{
request.Reports = reports;
request.Actions = actions;
}
else
{
ApplyGroupScope(request, reports, actions, visibleGroupIds, viewerUserId);
}
}Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 519:
Query amplification in `HydrateAsync` issues two sequential database round-trips (`GetByRequestAsync` for reports and actions) per request, causing up to 400 queries on a 200-item page. Batch-load reports and actions using `WHERE ModerationRequestId IN (...)` and group them in memory.
Suggested Code:
var requestIds = result.Select(x => x.ModerationRequestId).ToList();
var allReports = await _moderationReportRepository.GetByRequestIdsAsync(requestIds);
var allActions = await _moderationActionRepository.GetByRequestIdsAsync(requestIds);
var reportsByRequest = allReports.ToLookup(x => x.ModerationRequestId);
var actionsByRequest = allActions.ToLookup(x => x.ModerationRequestId);
foreach (var request in result)
{
var reports = reportsByRequest[request.ModerationRequestId].ToList();
var actions = actionsByRequest[request.ModerationRequestId].ToList();
if (visibleGroupIds == null)
{
request.Reports = reports;
request.Actions = actions;
}
else
{
ApplyGroupScope(request, reports, actions, visibleGroupIds, viewerUserId);
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } | ||
|
|
||
| private async Task<List<ModerationRequest>> HydrateAsync(IEnumerable<ModerationRequest> requests, |
There was a problem hiding this comment.
O(N) query amplification in HydrateAsync issues two database round-trips (GetByRequestAsync for reports and actions) per ModerationRequest, causing up to 400 sequential queries on a 200-item page. Replace the loop with two batch queries keyed on request IDs (WHERE ModerationRequestId IN @ids) and group the results client-side.
// Batch-load all reports and actions for the page in two queries, then group client-side:
// var ids = result.Select(r => r.ModerationRequestId).ToList();
// var allReports = (await _moderationReportRepository.GetByRequestIdsAsync(ids))
// .GroupBy(r => r.ModerationRequestId).ToDictionary(g => g.Key, g => g.ToList());
// var allActions = (await _moderationActionRepository.GetByRequestIdsAsync(ids))
// .GroupBy(a => a.ModerationRequestId).ToDictionary(g => g.Key, g => g.ToList());
// then iterate result and pull reports/actions from the dictionaries (falling back to empty lists).Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 519:
O(N) query amplification in `HydrateAsync` issues two database round-trips (`GetByRequestAsync` for reports and actions) per `ModerationRequest`, causing up to 400 sequential queries on a 200-item page. Replace the loop with two batch queries keyed on request IDs (`WHERE ModerationRequestId IN @ids`) and group the results client-side.
Suggested Code:
// Batch-load all reports and actions for the page in two queries, then group client-side:
// var ids = result.Select(r => r.ModerationRequestId).ToList();
// var allReports = (await _moderationReportRepository.GetByRequestIdsAsync(ids))
// .GroupBy(r => r.ModerationRequestId).ToDictionary(g => g.Key, g => g.ToList());
// var allActions = (await _moderationActionRepository.GetByRequestIdsAsync(ids))
// .GroupBy(a => a.ModerationRequestId).ToDictionary(g => g.Key, g => g.ToList());
// then iterate result and pull reports/actions from the dictionaries (falling back to empty lists).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| request.CompletedOn = null; | ||
| request.AdminNote = null; | ||
| request.ModifiedOn = DateTime.UtcNow; | ||
| await _moderationRequestRepository.UpdateAsync(request, cancellationToken); |
There was a problem hiding this comment.
Database inconsistency risk arises when the request-reopen block performs three separate writes (UpdateAsync, InsertAsync, SaveAuditLogAsync) without a wrapping transaction. Wrap the entire sequence in a transaction or unit-of-work to ensure atomic commits.
Kody rule violation: Handle transaction rollbacks properly
Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 141:
Database inconsistency risk arises when the request-reopen block performs three separate writes (`UpdateAsync`, `InsertAsync`, `SaveAuditLogAsync`) without a wrapping transaction. Wrap the entire sequence in a transaction or unit-of-work to ensure atomic commits.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .WithColumn("ContentAuthorUserId").AsString(450).Nullable() | ||
| .WithColumn("ContentAuthorUnitId").AsInt32().Nullable() | ||
| .WithColumn("ContentCreatedOn").AsDateTime2().Nullable() | ||
| .WithColumn("OriginalSubject").AsString(int.MaxValue).Nullable() |
There was a problem hiding this comment.
Inefficient row storage occurs because OriginalSubject uses NVARCHAR(MAX), preventing SQL Server optimization for bounded subjects. Use a bounded length like AsString(512).
Kody rule violation: Optimize string column types
Prompt for LLM
File Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs:
Line 28:
Inefficient row storage occurs because `OriginalSubject` uses `NVARCHAR(MAX)`, preventing SQL Server optimization for bounded subjects. Use a bounded length like `AsString(512)`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Create.ForeignKey("fk_communicationtestruns_communicationtests") | ||
| .FromTable("communicationtestruns").ForeignColumn("communicationtestid") | ||
| .ToTable("communicationtests").PrimaryColumn("communicationtestid") | ||
| .OnDelete(Rule.Cascade); |
There was a problem hiding this comment.
Locking and downtime risk occurs when adding a new FK constraint, as it takes ACCESS EXCLUSIVE locks while validating existing rows. Use the PostgreSQL online pattern by adding the constraint NOT VALID first, then executing VALIDATE CONSTRAINT in a later transaction.
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0111_CascadeCommunicationTestDeletesPg.cs:
Line 16 to 19:
Locking and downtime risk occurs when adding a new FK constraint, as it takes `ACCESS EXCLUSIVE` locks while validating existing rows. Use the PostgreSQL online pattern by adding the constraint `NOT VALID` first, then executing `VALIDATE CONSTRAINT` in a later transaction.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| MIN(m.senderuserid::text), MIN(m.senderunitid), MIN(m.senton), | ||
| COALESCE(MIN(m.body), (SELECT e.priorbody FROM chatmessageedits e | ||
| WHERE e.chatmessageid = f.chatmessageid ORDER BY e.editedon DESC LIMIT 1)), | ||
| (SELECT ca.filename FROM chatattachments ca |
There was a problem hiding this comment.
Tripled per-row I/O occurs due to three separate correlated subqueries to chatattachments for the same chatmessageid. Replace them with a single LEFT JOIN LATERAL to select the required fields in one pass.
Kody rule violation: Optimize database queries with JOINs
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0112_AddModerationPg.cs:
Line 141:
Tripled per-row I/O occurs due to three separate correlated subqueries to `chatattachments` for the same `chatmessageid`. Replace them with a single `LEFT JOIN LATERAL` to select the required fields in one pass.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } | ||
|
|
||
| var extra = filters.Count > 0 ? " AND " + string.Join(" AND ", filters) : string.Empty; |
There was a problem hiding this comment.
Error-prone string concatenation using + violates team rule. Use template literals to improve readability.
Kody rule violation: Use Template Literals Instead of String Concatenation
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs:
Line 144:
Error-prone string concatenation using `+` violates team rule. Use template literals to improve readability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| foreach (var pair in english) | ||
| { | ||
| var expected = Regex.Matches(pair.Value, @"\{\d+\}").Select(x => x.Value).OrderBy(x => x); |
There was a problem hiding this comment.
Regular expression denial of service (ReDoS) vulnerability violates team rule. Define a timeout when using regex on untrusted input to prevent DoS attacks.
Kody rule violation: Specify Timeout for Regular Expressions
Prompt for LLM
File Tests/Resgrid.Tests/Localization/ModerationLocalizationTests.cs:
Line 118:
Regular expression denial of service (ReDoS) vulnerability violates team rule. Define a timeout when using regex on untrusted input to prevent DoS attacks.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| foreach (var pair in english) | ||
| { | ||
| var expected = Regex.Matches(pair.Value, @"\{\d+\}").Select(x => x.Value).OrderBy(x => x); |
There was a problem hiding this comment.
Wasted CPU cycles occur because Regex.Matches is called with a constant pattern inside a foreach loop, forcing recompilation on every iteration. Declare a private static readonly Regex with RegexOptions.Compiled at the class level and reuse it.
Kody rule violation: Cache expensive operations outside loops
Prompt for LLM
File Tests/Resgrid.Tests/Localization/ModerationLocalizationTests.cs:
Line 118:
Wasted CPU cycles occur because `Regex.Matches` is called with a constant pattern inside a `foreach` loop, forcing recompilation on every iteration. Declare a `private static readonly Regex` with `RegexOptions.Compiled` at the class level and reuse it.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .GetColumns(new SqlServerConfiguration(), ignoreProperties: automation.IgnoredProperties) | ||
| .ToList(); | ||
|
|
||
| automation.IdType.Should().Be(1); |
There was a problem hiding this comment.
Opaque magic number 1 represents an IdType value without a named constant, making the test brittle and hard to read. Reference the named enum or constant, such as automation.IdType.Should().Be((int)IdType.String), for self-documenting assertions.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Tests/Resgrid.Tests/Models/FormAutomationTests.cs:
Line 28:
Opaque magic number `1` represents an `IdType` value without a named constant, making the test brittle and hard to read. Reference the named enum or constant, such as `automation.IdType.Should().Be((int)IdType.String)`, for self-documenting assertions.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var response = await _controller.GetCall(callId); | ||
|
|
||
| response.Result.Should().BeOfType<BadRequestResult>(); |
There was a problem hiding this comment.
Blocking async methods with .Result or .Wait() can cause deadlocks and violates team rule. Use await instead for proper asynchronous execution.
Kody rule violation: Avoid Blocking Calls to Async Methods
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/CallsControllerTests.cs:
Line 63:
Blocking async methods with `.Result` or `.Wait()` can cause deadlocks and violates team rule. Use `await` instead for proper asynchronous execution.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var response = await _controller.GetCall(callId); | ||
|
|
||
| response.Result.Should().BeOfType<BadRequestResult>(); |
There was a problem hiding this comment.
Blocking async operation violates team rule. Await Tasks instead of blocking with .Result or .Wait(), and prefer async/await end-to-end.
Kody rule violation: Await async operations properly
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/CallsControllerTests.cs:
Line 63:
Blocking async operation violates team rule. Await Tasks instead of blocking with `.Result` or `.Wait()`, and prefer `async/await` end-to-end.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var message = await _chatMessageService.GetMessageByIdAsync(attachment.ChatMessageId); | ||
| if (message == null || message.DeletedOn.HasValue) | ||
| return NotFound(); |
There was a problem hiding this comment.
Duplicated domain logic for the deleted-message check violates the DRY principle across GetAttachment and GetAttachmentThumbnail. Extract a helper method like EnsureMessageNotDeletedAsync(ChatMessageId) to handle the rule in one location.
Kody rule violation: Extract duplicated business logic
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 1282 to 1284:
Duplicated domain logic for the deleted-message check violates the DRY principle across `GetAttachment` and `GetAttachmentThumbnail`. Extract a helper method like `EnsureMessageNotDeletedAsync(ChatMessageId)` to handle the rule in one location.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| ContentCreatedOn, OriginalText, OriginalMetadataJson, Status, Disposition, CreatedOn, ModifiedOn) | ||
| SELECT CONCAT('callnote-', n.CallNoteId), c.DepartmentId, 2, CONVERT(varchar(32), n.CallNoteId), | ||
| n.CallId, n.UserId, n.Timestamp, n.Note, | ||
| (SELECT n.Source AS [source], n.Latitude AS [latitude], n.Longitude AS [longitude] | ||
| FOR JSON PATH, INCLUDE_NULL_VALUES, WITHOUT_ARRAY_WRAPPER), | ||
| 0, 0, COALESCE(n.FlaggedOn, n.Timestamp), COALESCE(n.FlaggedOn, n.Timestamp) | ||
| FROM CallNotes n | ||
| INNER JOIN Calls c ON c.CallId = n.CallId |
There was a problem hiding this comment.
Unbatched INSERT...SELECT operations in ImportLegacyFlags cause extended lock contention and transaction log bloat on production-scale data. Implement keyset batching using ranges like TOP @batch WHERE CallNoteId > @last, commit per batch, and ensure migration idempotency.
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Prompt for LLM
File Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs:
Line 191 to 198:
Unbatched `INSERT...SELECT` operations in `ImportLegacyFlags` cause extended lock contention and transaction log bloat on production-scale data. Implement keyset batching using ranges like `TOP @batch WHERE CallNoteId > @last`, commit per batch, and ensure migration idempotency.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| ? $@"SELECT moderationrequestid, departmentid, itemtype, itemid, callid, | ||
| chatchannelid, contentauthoruserid, contentauthorunitid, contentcreatedon, | ||
| originalsubject, originaltext, originalfilename, originalcontenttype, | ||
| originalmetadatajson, status, disposition, createdon, modifiedon, | ||
| completedbyuserid, completedon, adminnote | ||
| FROM {_sqlConfiguration.SchemaName}.moderationrequests WHERE departmentid = {notation}DepartmentId AND itemtype = {notation}ItemType AND itemid = {notation}ItemId" | ||
| : $@"SELECT [ModerationRequestId], [DepartmentId], [ItemType], [ItemId], [CallId], | ||
| [ChatChannelId], [ContentAuthorUserId], [ContentAuthorUnitId], [ContentCreatedOn], | ||
| [OriginalSubject], [OriginalText], [OriginalFileName], [OriginalContentType], | ||
| [OriginalMetadataJson], [Status], [Disposition], [CreatedOn], [ModifiedOn], | ||
| [CompletedByUserId], [CompletedOn], [AdminNote] | ||
| FROM {_sqlConfiguration.SchemaName}.[ModerationRequests] WHERE [DepartmentId] = {notation}DepartmentId AND [ItemType] = {notation}ItemType AND [ItemId] = {notation}ItemId"; |
There was a problem hiding this comment.
Omitting the originalcontent column in GetByItemAsync permanently nulls out retained binary evidence during updates, as Dapper repositories write all mapped columns without change tracking. Include originalcontent, evidencetext, evidencecontent, and evidencemetadatajson in the GetByItemAsync query to prevent overwriting unloaded fields.
var sql = DataConfig.DatabaseType == DatabaseTypes.Postgres
? $@"SELECT moderationrequestid, departmentid, itemtype, itemid, callid,
chatchannelid, contentauthoruserid, contentauthorunitid, contentcreatedon,
originalsubject, originaltext, originalfilename, originalcontenttype,
originalmetadatajson, originalcontent, status, disposition, createdon, modifiedon,
completedbyuserid, completedon, adminnote
FROM {_sqlConfiguration.SchemaName}.moderationrequests WHERE departmentid = {notation}DepartmentId AND itemtype = {notation}ItemType AND itemid = {notation}ItemId"Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs:
Line 42 to 53:
Omitting the `originalcontent` column in `GetByItemAsync` permanently nulls out retained binary evidence during updates, as Dapper repositories write all mapped columns without change tracking. Include `originalcontent`, `evidencetext`, `evidencecontent`, and `evidencemetadatajson` in the `GetByItemAsync` query to prevent overwriting unloaded fields.
Suggested Code:
var sql = DataConfig.DatabaseType == DatabaseTypes.Postgres
? $@"SELECT moderationrequestid, departmentid, itemtype, itemid, callid,
chatchannelid, contentauthoruserid, contentauthorunitid, contentcreatedon,
originalsubject, originaltext, originalfilename, originalcontenttype,
originalmetadatajson, originalcontent, status, disposition, createdon, modifiedon,
completedbyuserid, completedon, adminnote
FROM {_sqlConfiguration.SchemaName}.moderationrequests WHERE departmentid = {notation}DepartmentId AND itemtype = {notation}ItemType AND itemid = {notation}ItemId"
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 logging context occurs when Logging.LogException(ex) omits operation names and relevant identifiers. Pass structured context, such as Operation = nameof(GetByItemsAndReporterAsync), DepartmentId, ItemType, and ReporterUserId, alongside the exception to improve traceability.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs:
Line 105:
Insufficient logging context occurs when `Logging.LogException(ex)` omits operation names and relevant identifiers. Pass structured context, such as `Operation = nameof(GetByItemsAndReporterAsync)`, `DepartmentId`, `ItemType`, and `ReporterUserId`, alongside the exception to improve traceability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| : $"SELECT * FROM {_sqlConfiguration.SchemaName}.[ModerationReports] WHERE [ModerationRequestId] IN {notation}Ids ORDER BY [ModerationRequestId], [ReportedOn]"; | ||
|
|
||
| var select = new Func<DbConnection, Task<IEnumerable<ModerationReport>>>(connection => | ||
| connection.QueryAsync<ModerationReport>(sql, new { Ids = ids }, _unitOfWork.Transaction)); |
There was a problem hiding this comment.
Null pointer dereference identified when _unitOfWork.Transaction is accessed without a null guard, despite treating _unitOfWork as potentially null elsewhere. Use the null-conditional operator (_unitOfWork?.Transaction) inside the lambda or add an explicit null check before delegate invocation to prevent a runtime NullReferenceException.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs:
Line 292:
Null pointer dereference identified when `_unitOfWork.Transaction` is accessed without a null guard, despite treating `_unitOfWork` as potentially null elsewhere. Use the null-conditional operator (`_unitOfWork?.Transaction`) inside the lambda or add an explicit null check before delegate invocation to prevent a runtime `NullReferenceException`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var select = new Func<DbConnection, Task<IEnumerable<ModerationAction>>>(connection => | ||
| connection.QueryAsync<ModerationAction>(sql, new { Ids = ids }, _unitOfWork.Transaction)); | ||
|
|
||
| if (_unitOfWork?.Connection == null) | ||
| { | ||
| using var connection = _connectionProvider.Create(); | ||
| await connection.OpenAsync(); | ||
| return await select(connection); | ||
| } | ||
|
|
||
| return await select(_unitOfWork.CreateOrGetConnection()); |
There was a problem hiding this comment.
Code duplication identified in the connection-creation and query-execution block (lines 425-435), mirroring logic in GetByRequestIdsAsync (lines 291-301). Extract a generic helper like ExecuteWithConnectionsAsync<T>(string sql, object param) to encapsulate connection creation and the _unitOfWork fallback logic.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs:
Line 425 to 435:
Code duplication identified in the connection-creation and query-execution block (lines 425-435), mirroring logic in `GetByRequestIdsAsync` (lines 291-301). Extract a generic helper like `ExecuteWithConnectionsAsync<T>(string sql, object param)` to encapsulate connection creation and the `_unitOfWork` fallback logic.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .catch(() => { | ||
| if (requestToken === flagRequestToken.current) { | ||
| setFlagStatus(null); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Silent exception swallowing identified where the catch handler fails to capture or log rejection contexts, obscuring debugging failures. Capture the error parameter, log structured context such as console.error('getMyModerationRequest failed', { chatMessageId: message.ChatMessageId, err }), or rethrow the exception.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ChatPageElement.tsx:
Line 70 to 74:
Silent exception swallowing identified where the `catch` handler fails to capture or log rejection contexts, obscuring debugging failures. Capture the error parameter, log structured context such as `console.error('getMyModerationRequest failed', { chatMessageId: message.ChatMessageId, err })`, or rethrow the exception.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <div className="rgchat-mod__filters"> | ||
| <label> | ||
| <span>{moderationText('Status')}</span> | ||
| <select className="rgchat-input" value={status} onChange={(event) => { setStatus(Number(event.target.value)); setPage(1); }}> |
There was a problem hiding this comment.
Performance degradation identified due to inline arrow functions in JSX props creating new function instances on every render cycle. Extract these function definitions outside the render method to prevent unnecessary memory allocations.
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/moderation/ModerationRequestsTable.tsx:
Line 151:
Performance degradation identified due to inline arrow functions in JSX props creating new function instances on every render cycle. Extract these function definitions outside the render method to prevent unnecessary memory allocations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (ni?.Type == (int)EventTypes.ModerationRequestCompleted) | ||
| { | ||
| var moderationService = Bootstrapper.GetKernel().Resolve<IModerationService>(); | ||
| await moderationService.NotifyReportersAsync(ni.Value, cancellationToken); |
There was a problem hiding this comment.
Unguarded async exception propagation risks system instability when awaiting NotifyReportersAsync without error handling. Wrap the awaited call in a try/catch block and log the exception context to handle moderation service rejections safely.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/NotificationBroadcastLogic.cs:
Line 23:
Unguarded async exception propagation risks system instability when awaiting `NotifyReportersAsync` without error handling. Wrap the awaited call in a `try/catch` block and log the exception context to handle moderation service rejections safely.
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.
Actionable comments posted: 2
🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs`:
- Around line 190-195: Update the moderation request aggregation around the
computed Status and the CompletedByUserId/CompletedOn expressions so both
completion columns are populated only when the aggregated status is not Open
(Status = 0); otherwise return NULL. Preserve the existing MAX-based values for
non-open requests and keep the surrounding aggregation unchanged.
- Around line 144-159: Fix the import guard in the migration’s Execute callback
so it checks for the presence of an existing LegacyImport action, rather than
any moderation request lacking one, before calling ImportLegacyFlags. Preserve
rerun safety through ImportLegacyFlags’ existing NOT EXISTS guards; if
partial-import resumption is required, remove the outer guard and always invoke
ImportLegacyFlags.
🪄 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: 059bec34-1855-473b-9b6d-1fd44f4109a0
⛔ Files ignored due to path filters (2)
Tests/Resgrid.Tests/Services/ModerationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/ModerationControllerTests.csis excluded by!**/Tests/**
📒 Files selected for processing (5)
Core/Resgrid.Model/Moderation/Moderation.csCore/Resgrid.Services/ModerationService.csProviders/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.csRepositories/Resgrid.Repositories.DataRepository/ModerationRepositories.csWeb/Resgrid.Web.Services/Controllers/v4/ModerationController.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Core/Resgrid.Services/ModerationService.cs
- Core/Resgrid.Model/Moderation/Moderation.cs
- Web/Resgrid.Web.Services/Controllers/v4/ModerationController.cs
- Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs
| Execute.WithConnection((connection, _) => | ||
| { | ||
| using var command = connection.CreateCommand(); | ||
| command.CommandText = @"SELECT TOP (1) 1 | ||
| FROM ModerationRequests r | ||
| WHERE NOT EXISTS | ||
| ( | ||
| SELECT 1 | ||
| FROM ModerationActions a | ||
| WHERE a.ModerationRequestId = r.ModerationRequestId | ||
| AND a.ActorRole = 'LegacyImport' | ||
| );"; | ||
|
|
||
| if (command.ExecuteScalar() == null) | ||
| ImportLegacyFlags(connection); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The import guard is inverted and can skip the legacy import permanently.
The probe returns a row when a ModerationRequests row exists that has no LegacyImport action. ImportLegacyFlags runs only when the probe returns nothing. Two consequences follow:
- If any moderation request was created by the application (no
LegacyImportaction), the probe returns 1 and the whole legacy import is skipped. - If a previous run committed some batches and then failed, the remaining legacy rows are never imported, because the imported requests now coexist with rows that satisfy the probe.
The intent is "no legacy import has run yet". Test for the presence of a LegacyImport action instead. Because every insert in ImportLegacyFlags is guarded by a NOT EXISTS check on the target key, a rerun is safe.
🐛 Proposed fix for the import guard
Execute.WithConnection((connection, _) =>
{
using var command = connection.CreateCommand();
command.CommandText = @"SELECT TOP (1) 1
-FROM ModerationRequests r
-WHERE NOT EXISTS
-(
- SELECT 1
- FROM ModerationActions a
- WHERE a.ModerationRequestId = r.ModerationRequestId
- AND a.ActorRole = 'LegacyImport'
-);";
+FROM ModerationActions a
+WHERE a.ActorRole = 'LegacyImport';";
if (command.ExecuteScalar() == null)
ImportLegacyFlags(connection);
});Note: this still skips a resumed import after a partial run. If resumption matters, drop the guard and rely on the NOT EXISTS checks inside ImportLegacyFlags.
🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs`
around lines 144 - 159, Fix the import guard in the migration’s Execute callback
so it checks for the presence of an existing LegacyImport action, rather than
any moderation request lacking one, before calling ImportLegacyFlags. Preserve
rerun safety through ImportLegacyFlags’ existing NOT EXISTS guards; if
partial-import resumption is required, remove the outer guard and always invoke
ImportLegacyFlags.
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN 0 ELSE 1 END AS Status, | ||
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN 0 | ||
| WHEN SUM(CASE WHEN f.Status = 3 THEN 1 ELSE 0 END) > 0 THEN 2 ELSE 1 END AS Disposition, | ||
| MIN(f.FlaggedOn) AS CreatedOn, MAX(COALESCE(f.ReviewedOn, f.FlaggedOn)) AS ModifiedOn, | ||
| MAX(f.ReviewedByUserId) AS CompletedByUserId, MAX(f.ReviewedOn) AS CompletedOn, | ||
| MAX(f.ResolutionNote) AS AdminNote |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Open imported requests can carry completion values.
Status becomes 0 when any flag has Status = 0, but CompletedByUserId and CompletedOn are still taken with MAX across all flags of the message. A message with one open flag and one reviewed flag produces an Open request that has a completion user and a completion timestamp. ModerationRequestRepository reads both columns, so consumers see a completed-looking open request.
Set the completion columns only when the computed status is not Open.
🐛 Proposed fix for the completion columns
- MAX(f.ReviewedByUserId) AS CompletedByUserId, MAX(f.ReviewedOn) AS CompletedOn,
+ CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN NULL
+ ELSE MAX(f.ReviewedByUserId) END AS CompletedByUserId,
+ CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN NULL
+ ELSE MAX(f.ReviewedOn) END AS CompletedOn,
MAX(f.ResolutionNote) AS AdminNote📝 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.
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN 0 ELSE 1 END AS Status, | |
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN 0 | |
| WHEN SUM(CASE WHEN f.Status = 3 THEN 1 ELSE 0 END) > 0 THEN 2 ELSE 1 END AS Disposition, | |
| MIN(f.FlaggedOn) AS CreatedOn, MAX(COALESCE(f.ReviewedOn, f.FlaggedOn)) AS ModifiedOn, | |
| MAX(f.ReviewedByUserId) AS CompletedByUserId, MAX(f.ReviewedOn) AS CompletedOn, | |
| MAX(f.ResolutionNote) AS AdminNote | |
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN 0 ELSE 1 END AS Status, | |
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN 0 | |
| WHEN SUM(CASE WHEN f.Status = 3 THEN 1 ELSE 0 END) > 0 THEN 2 ELSE 1 END AS Disposition, | |
| MIN(f.FlaggedOn) AS CreatedOn, MAX(COALESCE(f.ReviewedOn, f.FlaggedOn)) AS ModifiedOn, | |
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN NULL | |
| ELSE MAX(f.ReviewedByUserId) END AS CompletedByUserId, | |
| CASE WHEN SUM(CASE WHEN f.Status = 0 THEN 1 ELSE 0 END) > 0 THEN NULL | |
| ELSE MAX(f.ReviewedOn) END AS CompletedOn, | |
| MAX(f.ResolutionNote) AS AdminNote |
🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs`
around lines 190 - 195, Update the moderation request aggregation around the
computed Status and the CompletedByUserId/CompletedOn expressions so both
completion columns are populated only when the aggregated status is not Open
(Status = 0); otherwise return NULL. Preserve the existing MAX-based values for
non-open requests and keep the surrounding aggregation unchanged.
| await RecordActionAsync(request, ModerationActionType.ReportersNotified, null, null, | ||
| request.Status, request.Status, null, null, cancellationToken); | ||
| await NotifyReportersAsync(request, reports, (ModerationDisposition)request.Disposition, | ||
| request.AdminNote, cancellationToken); |
There was a problem hiding this comment.
At-most-once delivery semantics occur because RecordActionAsync (line 700) auto-commits the ReportersNotified guard via RepositoryBase.InsertAsync (lines 192-199) before the inner send loop (lines 761-787) executes, permanently short-circuiting future retry attempts at lines 378-380 upon partial failure. Move the RecordActionAsync call below line 386 so the guard only suppresses genuinely completed deliveries after NotifyReportersAsync succeeds.
var reports = (await _moderationReportRepository.GetByRequestAsync(moderationRequestId))?.ToList()
?? new List<ModerationReport>();
await NotifyReportersAsync(request, reports, (ModerationDisposition)request.Disposition,
request.AdminNote, cancellationToken);
await RecordActionAsync(request, ModerationActionType.ReportersNotified, null, null,
request.Status, request.Status, null, null, cancellationToken);Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 384 to 387:
At-most-once delivery semantics occur because RecordActionAsync (line 700) auto-commits the ReportersNotified guard via RepositoryBase.InsertAsync (lines 192-199) before the inner send loop (lines 761-787) executes, permanently short-circuiting future retry attempts at lines 378-380 upon partial failure. Move the RecordActionAsync call below line 386 so the guard only suppresses genuinely completed deliveries after NotifyReportersAsync succeeds.
Suggested Code:
var reports = (await _moderationReportRepository.GetByRequestAsync(moderationRequestId))?.ToList()
?? new List<ModerationReport>();
await NotifyReportersAsync(request, reports, (ModerationDisposition)request.Disposition,
request.AdminNote, cancellationToken);
await RecordActionAsync(request, ModerationActionType.ReportersNotified, null, null,
request.Status, request.Status, null, null, cancellationToken);
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); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Race condition recovery loss in FlagAsync causes unhandled 500 exceptions during concurrent flag submissions for the same (ModerationRequestId, ReportedByUserId) because the catch handler throws unconditionally when concurrent requests race past the check at line 142 and trigger the unique index UX_ModerationReports_Request_Reporter. Restore the symmetric request-level recovery by re-querying GetByRequestAndReporterAsync and returning the concurrent report when found, re-throwing only when none exists.
catch (Exception ex)
{
Logging.LogException(ex);
var concurrent = await _moderationReportRepository.GetByRequestAndReporterAsync(
request.ModerationRequestId, reportedByUserId);
if (concurrent == null)
throw;
return concurrent;
}Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 201 to 205:
Race condition recovery loss in FlagAsync causes unhandled 500 exceptions during concurrent flag submissions for the same (ModerationRequestId, ReportedByUserId) because the catch handler throws unconditionally when concurrent requests race past the check at line 142 and trigger the unique index UX_ModerationReports_Request_Reporter. Restore the symmetric request-level recovery by re-querying GetByRequestAndReporterAsync and returning the concurrent report when found, re-throwing only when none exists.
Suggested Code:
catch (Exception ex)
{
Logging.LogException(ex);
var concurrent = await _moderationReportRepository.GetByRequestAndReporterAsync(
request.ModerationRequestId, reportedByUserId);
if (concurrent == null)
throw;
return concurrent;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return existingReport; | ||
|
|
||
| var reopenRequest = request.Status == (int)ModerationRequestStatus.Completed; | ||
| await _unitOfWork.CreateOrGetConnectionAsync(cancellationToken); |
There was a problem hiding this comment.
Unhandled exception propagation occurs because the awaited _unitOfWork.CreateOrGetConnectionAsync call sits outside the try/catch block starting at line 150, preventing unit of work cleanup and error handling context. Wrap this await in its own try/catch or move it inside the existing try block, ensuring the catch path safely handles a null or unestablished connection.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 148:
Unhandled exception propagation occurs because the awaited _unitOfWork.CreateOrGetConnectionAsync call sits outside the try/catch block starting at line 150, preventing unit of work cleanup and error handling context. Wrap this await in its own try/catch or move it inside the existing try block, ensuring the catch path safely handles a null or unestablished connection.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return existingReport; | ||
|
|
||
| var reopenRequest = request.Status == (int)ModerationRequestStatus.Completed; | ||
| await _unitOfWork.CreateOrGetConnectionAsync(cancellationToken); |
There was a problem hiding this comment.
Missing error mapping occurs because the external _unitOfWork.CreateOrGetConnectionAsync database connection call lacks a try/catch for network or configuration failures. Wrap the call in a try/catch to log the failure with operation and department context, and map the exception to an application-level error.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 148:
Missing error mapping occurs because the external _unitOfWork.CreateOrGetConnectionAsync database connection call lacks a try/catch for network or configuration failures. Wrap the call in a try/catch to log the failure with operation and department context, and map the exception to an application-level error.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| SELECT 1 | ||
| FROM ModerationActions a | ||
| WHERE a.ModerationRequestId = r.ModerationRequestId | ||
| AND a.ActorRole = 'LegacyImport' |
There was a problem hiding this comment.
String literal duplication of 'LegacyImport' across multiple SQL commands risks inconsistency between the existence-check query and INSERT statements if the actor-role identifier changes. Define a private const string at the class level, such as LegacyImportActorRole = "LegacyImport", and interpolate or parameterize it into each SQL command text.
Kody rule violation: Centralize string constants
Prompt for LLM
File Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs:
Line 154:
String literal duplication of 'LegacyImport' across multiple SQL commands risks inconsistency between the existence-check query and INSERT statements if the actor-role identifier changes. Define a private const string at the class level, such as `LegacyImportActorRole = "LegacyImport"`, and interpolate or parameterize it into each SQL command text.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| SELECT 1 | ||
| FROM ModerationActions a | ||
| WHERE a.ModerationRequestId = r.ModerationRequestId | ||
| AND a.ActorRole = 'LegacyImport' |
There was a problem hiding this comment.
Magic string comparison against 'LegacyImport' obscures the finite set of valid ActorRole values and introduces fragility. Define an ActorRole enum or constants class and interpolate its member, such as ActorRoles.LegacyImport, into the SQL command text.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Providers/Resgrid.Providers.Migrations/Migrations/M0112_AddModeration.cs:
Line 154:
Magic string comparison against 'LegacyImport' obscures the finite set of valid ActorRole values and introduces fragility. Define an ActorRole enum or constants class and interpolate its member, such as `ActorRoles.LegacyImport`, into the SQL command text.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| Data = requests.Select(x => ConvertRequest(x, true)).ToList(), | ||
| Page = Math.Max(page, 1), | ||
| PageSize = Math.Min(Math.Max(pageSize, 1), 200), |
There was a problem hiding this comment.
Magic number usage of the literal 200 to cap the page size obscures the limit's intent and hinders discoverability. Extract this value to a named constant like private const int MaxPageSize = 200; and reference it within the Math.Min expression.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ModerationController.cs:
Line 123:
Magic number usage of the literal 200 to cap the page size obscures the limit's intent and hinders discoverability. Extract this value to a named constant like `private const int MaxPageSize = 200;` and reference it within the Math.Min expression.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs (3)
204-224: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a unique tie-breaker to the paginated ordering.
SearchAsyncuses offset pagination and orders only byModifiedOn. Rows with equal timestamps have no deterministic order. Consecutive page requests can repeat or omit requests.Add
ModerationRequestId DESCafterModifiedOn DESCin both database-specific queries.Suggested ordering change
-ORDER BY r.modifiedon DESC +ORDER BY r.modifiedon DESC, r.moderationrequestid DESC -ORDER BY r.[ModifiedOn] DESC +ORDER BY r.[ModifiedOn] DESC, r.[ModerationRequestId] DESC🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs` around lines 204 - 224, Add ModerationRequestId DESC as a secondary ordering criterion after ModifiedOn DESC in both database-specific SQL query branches of SearchAsync, preserving the existing pagination and primary timestamp ordering.
113-122: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBound
criteria.Pagebefore computingOffset.
criteria.Pagehas only a lower bound. Line 122 performsintmultiplication. A large page can wrapOffsetto a negative value and fail the SQL query. A large valid offset can also force an expensive scan.Apply a product-defined maximum page or offset. Compute the offset with checked
longarithmetic before adding it toparameters.🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs` around lines 113 - 122, Update the pagination setup in the moderation repository method around criteria.Page and the Offset parameter: enforce the product-defined maximum page or offset, then compute the offset using checked long arithmetic before passing it to parameters.Add. Preserve the existing lower-bound page behavior and page-size limits while preventing integer overflow and excessively expensive scans.
238-249: 🩺 Stability & Availability | 🟠 MajorSelect the connection and transaction from the same branch.
At Line 239 and Lines 395 and 435, the lambda reads
_unitOfWork.Transactionbefore the fallback checks_unitOfWork?.Connection. If_unitOfWorkis null, the method throws before opening the standalone connection.At Lines 299 and 339, the code can pass a unit-of-work transaction to a newly created connection when the fallback is selected. Pass a transaction only when the selected connection belongs to the unit of work. Otherwise, pass
null.#!/usr/bin/env bash set -euo pipefail rg -n -C 8 'interface IUnitOfWork|class .*UnitOfWork|Transaction|CreateOrGetConnection|Connection' --glob '*.cs' rg -n -C 8 'GetByRequestAndReporterAsync|GetByRequestIdsAsync|ModerationRequestRepository|ModerationActionRepository' --glob '*.cs'Also applies to: 298-308, 339-350, 394-404, 434-444
🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs` around lines 238 - 249, Update the moderation query methods at the affected `select` blocks to choose the connection and transaction together: use the unit-of-work transaction only when `_unitOfWork?.Connection` is selected, and pass null for standalone connections created by `_connectionProvider.Create()`. Ensure `_unitOfWork.Transaction` is not accessed when `_unitOfWork` is null, including the methods around `GetByRequestAndReporterAsync` and `GetByRequestIdsAsync`.
🧹 Nitpick comments (1)
Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse the required service-locator resolution pattern.
These constructors inject
IConnectionProvider,SqlConfiguration,IUnitOfWork, andIQueryFactory. Resolve these dependencies throughBootstrapper.GetKernel().Resolve<T>()in each repository constructor instead. Use the same resolved instances for the base constructor and repository fields.As per coding guidelines, use the Service Locator pattern via
Bootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.Also applies to: 258-265, 366-373
🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs` around lines 23 - 30, Update the constructors for the moderation repositories, including the ones around the referenced additional locations, to resolve IConnectionProvider, SqlConfiguration, IUnitOfWork, and IQueryFactory through Bootstrapper.GetKernel().Resolve<T>() instead of accepting them as parameters. Reuse those resolved instances for the base constructor invocation and repository fields, preserving the existing initialization behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs`:
- Around line 204-224: Add ModerationRequestId DESC as a secondary ordering
criterion after ModifiedOn DESC in both database-specific SQL query branches of
SearchAsync, preserving the existing pagination and primary timestamp ordering.
- Around line 113-122: Update the pagination setup in the moderation repository
method around criteria.Page and the Offset parameter: enforce the
product-defined maximum page or offset, then compute the offset using checked
long arithmetic before passing it to parameters.Add. Preserve the existing
lower-bound page behavior and page-size limits while preventing integer overflow
and excessively expensive scans.
- Around line 238-249: Update the moderation query methods at the affected
`select` blocks to choose the connection and transaction together: use the
unit-of-work transaction only when `_unitOfWork?.Connection` is selected, and
pass null for standalone connections created by `_connectionProvider.Create()`.
Ensure `_unitOfWork.Transaction` is not accessed when `_unitOfWork` is null,
including the methods around `GetByRequestAndReporterAsync` and
`GetByRequestIdsAsync`.
---
Nitpick comments:
In `@Repositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs`:
- Around line 23-30: Update the constructors for the moderation repositories,
including the ones around the referenced additional locations, to resolve
IConnectionProvider, SqlConfiguration, IUnitOfWork, and IQueryFactory through
Bootstrapper.GetKernel().Resolve<T>() instead of accepting them as parameters.
Reuse those resolved instances for the base constructor invocation and
repository fields, preserving the existing initialization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f1bb1bf9-bfc3-4eca-ba52-dd2baa9e13fc
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Services/ModerationServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (3)
Core/Resgrid.Model/Repositories/IModerationRepositories.csCore/Resgrid.Services/ModerationService.csRepositories/Resgrid.Repositories.DataRepository/ModerationRepositories.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/Resgrid.Model/Repositories/IModerationRepositories.cs
- Core/Resgrid.Services/ModerationService.cs
| await NotifyReportersAsync(request, reports, (ModerationDisposition)request.Disposition, | ||
| request.AdminNote, cancellationToken); | ||
| await RecordActionAsync(request, ModerationActionType.ReportersNotified, null, null, | ||
| request.Status, request.Status, null, null, cancellationToken); |
There was a problem hiding this comment.
Partial send failure in NotifyReportersAsync leaves the aggregate ReportersNotified guard unset, causing retries to duplicate notifications to already-notified recipients. Track per-recipient notification state (e.g., persist a per-recipient ModerationAction marker before sending) so retries skip already-notified recipients.
// Inner NotifyReportersAsync should record a per-recipient marker before/at send time
// (or skip recipients already marked) so retries do not duplicate notifications;
// then record the aggregate ReportersNotified guard only after all recipients succeed.Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 390 to 393:
Partial send failure in NotifyReportersAsync leaves the aggregate ReportersNotified guard unset, causing retries to duplicate notifications to already-notified recipients. Track per-recipient notification state (e.g., persist a per-recipient ModerationAction marker before sending) so retries skip already-notified recipients.
Suggested Code:
// Inner NotifyReportersAsync should record a per-recipient marker before/at send time
// (or skip recipients already marked) so retries do not duplicate notifications;
// then record the aggregate ReportersNotified guard only after all recipients succeed.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var concurrent = await _moderationReportRepository.GetByRequestAndReporterAsync( | ||
| request.ModerationRequestId, reportedByUserId, useUnitOfWork: false); |
There was a problem hiding this comment.
Unhandled exception in catch block: GetByRequestAndReporterAsync at line 201 (and lines 392-393) can fail on DB timeout or connection loss, discarding the original exception context and propagating an opaque error. Wrap the recovery query in a nested try/catch, log the secondary error with context, and rethrow the original exception.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 204 to 205:
Unhandled exception in catch block: GetByRequestAndReporterAsync at line 201 (and lines 392-393) can fail on DB timeout or connection loss, discarding the original exception context and propagating an opaque error. Wrap the recovery query in a nested try/catch, log the secondary error with context, and rethrow the original exception.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var concurrent = await _moderationReportRepository.GetByRequestAndReporterAsync( | ||
| request.ModerationRequestId, reportedByUserId, useUnitOfWork: false); |
There was a problem hiding this comment.
Unhandled external call in catch block: GetByRequestAndReporterAsync can fail independently, discarding the original exception context and producing an opaque error for callers. Wrap the call in try/catch with structured context (requestId, reporterId) and either rethrow the original exception or map the secondary failure to an application-level error.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/ModerationService.cs:
Line 204 to 205:
Unhandled external call in catch block: GetByRequestAndReporterAsync can fail independently, discarding the original exception context and producing an opaque error for callers. Wrap the call in try/catch with structured context (requestId, reporterId) and either rethrow the original exception or map the secondary failure to an application-level error.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| SetupMessageEvidence(); | ||
| _requests.Setup(x => x.GetByItemAsync(7, (int)ModerationItemType.Message, "42")).ReturnsAsync(request); | ||
| _reports.Setup(x => x.GetByRequestAndReporterAsync(request.ModerationRequestId, "reporter")) | ||
| .ReturnsAsync((ModerationReport)null); |
There was a problem hiding this comment.
Unsafe cast risk: 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/ModerationServiceTests.cs:
Line 194:
Unsafe cast risk: 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.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
3 participants
PR Description: RG-T129 Notification System Bug Fixes
Summary
This PR fixes several bugs in the notification system that prevented notifications from firing correctly and caused runtime errors under certain conditions.
Changes
Bug Fix: Incorrect Event Type in Group Lookup
In
GetGroupForEventAsync, the code block that looks up personnel staffing data was incorrectly matched againstPersonnelStatusChangedinstead ofPersonnelStaffingChanged. This meant group-based notifications for staffing changes would never resolve the correct department group.Bug Fix: Empty BeforeData/CurrentData Causing Notifications to Never Fire
The notification validation logic previously returned
falsewheneverBeforeDataorCurrentDatawas null or empty. Since the UI's "Any" option was posting an empty string, notifications saved with default "Any" settings would never trigger. The validation now treats empty/null values as"-1"(the system's "Any" sentinel), allowing these notifications to process as intended.Bug Fix: NullReferenceException When No Previous State Exists
For
UnitStatusChanged,PersonnelStaffingChanged, andPersonnelStatusChangedevents, when a "before" state was required but no prior state existed (e.g., the very first state change), the code would throw a null reference exception. Null checks were added so the notification is safely skipped (returnsfalse) instead of crashing. A missing null check oncurrentStatewas also added forPersonnelStatusChanged.Bug Fix: UI Dropdown Posting Incorrect "Any" Value
The client-side dropdown initialization was changed to post
"-1"for the "Any" option instead of an empty string, aligning with the notification engine's expected value format. The API calls for populating these dropdowns were updated to stop requesting an "Any" entry from the server (includeAny=False), since it is now provided client-side.Test Coverage
New unit tests were added covering:
Summary by CodeRabbit
New Features
Bug Fixes