Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change limits GIF requests to Giphy, adds configurable safe ratings, enforces chatbot conversation rules, and adds urgent-message acknowledgment retrieval and display. ChangesGiphy provider configuration
Chatbot restrictions and acknowledgments
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatMessageService
participant ChatHub
participant ChatStore
participant AckStatus
participant ChatController
ChatMessageService->>ChatHub: publish AckRequired with SenderUserId
ChatHub->>ChatStore: add pending acknowledgment when sender differs
ChatStore->>AckStatus: update message acknowledgment revision
AckStatus->>ChatController: request acknowledgments
ChatController-->>AckStatus: return acknowledgment status and display names
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs (1)
444-473: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIsolate the confirmation message failure from the session-reset result.
After
EndSessionAsyncsucceeds,EnsureChatbotChannelAsync/SendBotMessageAsyncare handled by the samecatchthat returns"Unable to reset the chat session.". This reports a reset failure when only the confirmation message failed. Run the confirmation message in its own try/catch, log failures withLogging.LogException, and still returnSuccess = trueif the session reset completed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs` around lines 444 - 473, Separate the confirmation-message flow around EnsureChatbotChannelAsync and SendBotMessageAsync from the outer reset try/catch by giving it its own try/catch. Log confirmation failures with Logging.LogException, but allow the method to continue returning the existing Success = true ChatbotSessionResetResult after EndSessionAsync succeeds; reserve the existing error response for session-reset failures.
🧹 Nitpick comments (2)
Core/Resgrid.Config/ChatConfig.cs (1)
27-32: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSet GIF search defaults to avoid accidental feature loss
GetGifsonly uses the provider whenChatConfig.GifProvideris"giphy"andGiphyApiKeyis non-empty. DefaultGifProviderto"giphy"or add release notes that deployments must set both values; otherwise GIF search remains off for blank configuration.🤖 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.Config/ChatConfig.cs` around lines 27 - 32, Update ChatConfig.GifProvider to default to "giphy" so GIF search is enabled when GiphyApiKey is configured, while retaining the existing empty-key requirement and supported-provider behavior.Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs (1)
976-978: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant message/channel lookups in
AddReactionandRemoveReaction.
IsChatbotMessageChannelAsyncre-fetches the message via_chatMessageService.GetMessageByIdAsyncand the channel via_chatChannelService.GetChannelByIdAsync. InAddReaction(Line 976) andRemoveReaction(Line 1010), this call runs immediately afterCheckMessageChannelAccessAsync, which already fetched the same message and the same channel. Each request now performs 4 lookups instead of 2.Pass the channel already resolved by
CheckMessageChannelAccessAsyncinto the chatbot check instead of re-fetching it. For example, changeCheckMessageChannelAccessAsyncto return the resolved channel (or add an overload ofIsChatbotMessageChannelAsyncthat accepts aChatChannel), and reuse it at both call sites.♻️ Proposed refactor sketch
- private async Task<bool> IsChatbotMessageChannelAsync(string messageId) - { - var message = await _chatMessageService.GetMessageByIdAsync(messageId); - if (message == null) - return false; - - var channel = await _chatChannelService.GetChannelByIdAsync(message.ChatChannelId); - return channel != null && channel.ChannelType == (int)ChatChannelType.Chatbot; - } + private static bool IsChatbotChannel(ChatChannel channel) + { + return channel != null && channel.ChannelType == (int)ChatChannelType.Chatbot; + }Then have
CheckMessageChannelAccessAsyncreturn the resolvedChatChannel(via anoutparameter or a small result type) soAddReaction/RemoveReactioncan callIsChatbotChannel(channel)without a second round trip.DeleteMessage, which has no prior lookup, can keep calling a lookup-based helper.Also applies to: 1010-1012, 1590-1603
🤖 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 976 - 978, Refactor AddReaction and RemoveReaction to reuse the message/channel resolved by CheckMessageChannelAccessAsync when determining whether the conversation is chatbot-owned, avoiding duplicate lookups. Update the access-check result or add a channel-based IsChatbotMessageChannelAsync overload, and use it at both reaction call sites while preserving DeleteMessage’s lookup-based path.
🤖 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 `@Web/Resgrid.Web.Services/Models/v4/Chat/ChatApiModels.cs`:
- Around line 745-748: Update the ChatController GetAcks response mapper to
resolve each acknowledgment’s user profile and assign the profile’s display name
to ChatAckResultData.DisplayName when constructing the result. Preserve the
existing UserId mapping and ensure the populated name is returned for each
acknowledgment.
In `@Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ConversationView.tsx`:
- Line 321: Update the onRetrySend prop in ConversationView to use the existing
isBot flag instead of checking variant === 'bot', disabling retries for chatbot
channels regardless of how they were opened while preserving the current
handleRetry behavior for non-bot conversations.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs`:
- Around line 444-473: Separate the confirmation-message flow around
EnsureChatbotChannelAsync and SendBotMessageAsync from the outer reset try/catch
by giving it its own try/catch. Log confirmation failures with
Logging.LogException, but allow the method to continue returning the existing
Success = true ChatbotSessionResetResult after EndSessionAsync succeeds; reserve
the existing error response for session-reset failures.
---
Nitpick comments:
In `@Core/Resgrid.Config/ChatConfig.cs`:
- Around line 27-32: Update ChatConfig.GifProvider to default to "giphy" so GIF
search is enabled when GiphyApiKey is configured, while retaining the existing
empty-key requirement and supported-provider behavior.
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs`:
- Around line 976-978: Refactor AddReaction and RemoveReaction to reuse the
message/channel resolved by CheckMessageChannelAccessAsync when determining
whether the conversation is chatbot-owned, avoiding duplicate lookups. Update
the access-check result or add a channel-based IsChatbotMessageChannelAsync
overload, and use it at both reaction call sites while preserving
DeleteMessage’s lookup-based path.
🪄 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: 6ba06817-660c-4cf0-add8-09de04463088
📒 Files selected for processing (17)
Core/Resgrid.Config/ChatConfig.csCore/Resgrid.Model/Providers/IGifProvider.csCore/Resgrid.Services/ChatMessageService.csProviders/Resgrid.Providers.Messaging/GifProvider.csWeb/Resgrid.Web.Services/Controllers/v4/ChatController.csWeb/Resgrid.Web.Services/Controllers/v4/ChatbotController.csWeb/Resgrid.Web.Services/Models/v4/Chat/ChatApiModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/ConversationView.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/AckStatus.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/Composer.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/chatApi.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chatHub.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/chatStore.tsWeb/Resgrid.Web/Areas/User/Apps/src/components/chat/types.ts
| /// <summary> | ||
| /// Display name of the user the acknowledgment is required from (populated by GetAcks) | ||
| /// </summary> | ||
| public string DisplayName { get; set; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Populate DisplayName in the GetAcks response mapper.
ChatController, Lines 1815-1824, creates ChatAckResultData without assigning DisplayName. The client then falls back to UserId, so acknowledgment status does not show recipient names. Resolve each acknowledgment user profile and assign DisplayName in that mapper.
🤖 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/Chat/ChatApiModels.cs` around lines 745 -
748, Update the ChatController GetAcks response mapper to resolve each
acknowledgment’s user profile and assign the profile’s display name to
ChatAckResultData.DisplayName when constructing the result. Preserve the
existing UserId mapping and ensure the populated name is returned for each
acknowledgment.
| onFlag={variant === 'bot' ? undefined : props.onFlag} | ||
| onFlag={props.onFlag} | ||
| onOpenImage={handleOpenImage} | ||
| onRetrySend={variant === 'bot' ? undefined : handleRetry} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
onRetrySend still uses the narrower variant === 'bot' check.
Lines 314-318 gate onReact, onOpenThread, and onDelete on isBot, which also covers a chatbot channel opened through the regular chat page (variant='default'), per the comment at lines 58-61. Line 321 still checks variant === 'bot' only, so retrying a failed message stays enabled in that same scenario, contradicting the stated restriction.
Use isBot here for consistency.
🐛 Proposed fix
- onRetrySend={variant === 'bot' ? undefined : handleRetry}
+ onRetrySend={isBot ? undefined : handleRetry}📝 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.
| onRetrySend={variant === 'bot' ? undefined : handleRetry} | |
| onRetrySend={isBot ? undefined : handleRetry} |
🤖 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/ConversationView.tsx` at
line 321, Update the onRetrySend prop in ConversationView to use the existing
isBot flag instead of checking variant === 'bot', disabling retries for chatbot
channels regardless of how they were opened while preserving the current
handleRetry behavior for non-bot conversations.
| if (await IsChatbotMessageChannelAsync(messageId)) | ||
| return BadRequest("Messages can't be deleted in assistant conversations."); |
There was a problem hiding this comment.
Information disclosure vulnerability in DeleteMessage: IsChatbotMessageChannelAsync is called before any department or ownership validation, returning a distinguishable BadRequest ("Messages can't be deleted in assistant conversations.") for chatbot channels versus a generic Failure for non-chatbot — allowing any authenticated user to probe arbitrary message IDs and learn which belong to assistant conversations. Add CheckMessageChannelAccessAsync before the chatbot-type guard so cross-department messages return a uniform NotFound.
var accessCheck = await CheckMessageChannelAccessAsync(messageId);
if (accessCheck != null)
return accessCheck;
if (await IsChatbotMessageChannelAsync(messageId))
return BadRequest("Messages can't be deleted in assistant conversations.");Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 932 to 933:
Information disclosure vulnerability in DeleteMessage: IsChatbotMessageChannelAsync is called before any department or ownership validation, returning a distinguishable BadRequest ("Messages can't be deleted in assistant conversations.") for chatbot channels versus a generic Failure for non-chatbot — allowing any authenticated user to probe arbitrary message IDs and learn which belong to assistant conversations. Add CheckMessageChannelAccessAsync before the chatbot-type guard so cross-department messages return a uniform NotFound.
Suggested Code:
var accessCheck = await CheckMessageChannelAccessAsync(messageId);
if (accessCheck != null)
return accessCheck;
if (await IsChatbotMessageChannelAsync(messageId))
return BadRequest("Messages can't be deleted in assistant conversations.");
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Assistant conversations are plain text only: no threads, no attachments/GIFs, no urgent | ||
| // priority. Enforced here so every client (web and mobile) gets the same behavior. | ||
| var channel = await _chatChannelService.GetChannelByIdAsync(channelId); |
There was a problem hiding this comment.
Unguarded external call: GetChannelByIdAsync is not wrapped in try/catch, violating Rule [27]. Wrap the call in try/catch, log with channelId context, and return an appropriate error ActionResult.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 778:
Unguarded external call: GetChannelByIdAsync is not wrapped in try/catch, violating Rule [27]. Wrap the call in try/catch, log with channelId context, and return an appropriate error ActionResult.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // priority. Enforced here so every client (web and mobile) gets the same behavior. | ||
| var channel = await _chatChannelService.GetChannelByIdAsync(channelId); | ||
| var isChatbotChannel = channel != null && channel.ChannelType == (int)ChatChannelType.Chatbot; | ||
| if (isChatbotChannel && ((ChatMessageType)input.MessageType != ChatMessageType.Text || !String.IsNullOrWhiteSpace(input.ThreadRootMessageId))) |
There was a problem hiding this comment.
Unsafe type casting: (ChatMessageType)input.MessageType uses a direct cast instead of the as operator or pattern matching. Replace with safe casting and guard null results before usage.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatController.cs:
Line 780:
Unsafe type casting: `(ChatMessageType)input.MessageType` uses a direct cast instead of the `as` operator or pattern matching. Replace with safe casting 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 channel = await _chatChannelService.EnsureChatbotChannelAsync(DepartmentId, UserId); | ||
| if (channel != null) | ||
| await _chatMessageService.SendBotMessageAsync(channel.ChatChannelId, | ||
| DepartmentId.ToString(System.Globalization.CultureInfo.InvariantCulture), | ||
| "Starting a new conversation — your previous context has been cleared.", "Resgrid Assistant"); |
There was a problem hiding this comment.
State inconsistency in the reset flow: SendBotMessageAsync runs inside the same try/catch as EndSessionAsync, so a transient failure in the confirmation send returns BadRequest even though the session was already cleared — contradicting the persisted state and causing a duplicate 'Starting a new conversation' bot message on retry. Isolate the confirmation send in its own best-effort try/catch that logs but does not fail the reset.
if (session != null)
await _chatbotSessionManager.EndSessionAsync(session.SessionId);
// Confirmation is best-effort: a failure here must not turn a successful reset into a failure.
try
{
var channel = await _chatChannelService.EnsureChatbotChannelAsync(DepartmentId, UserId);
if (channel != null)
await _chatMessageService.SendBotMessageAsync(channel.ChatChannelId,
DepartmentId.ToString(System.Globalization.CultureInfo.InvariantCulture),
"Starting a new conversation — your previous context has been cleared.", "Resgrid Assistant");
}
catch (Exception ex)
{
Logging.LogException(ex);
}
var result = new ChatbotSessionResetResult { Success = true, ... };Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 452 to 456:
State inconsistency in the reset flow: SendBotMessageAsync runs inside the same try/catch as EndSessionAsync, so a transient failure in the confirmation send returns BadRequest even though the session was already cleared — contradicting the persisted state and causing a duplicate 'Starting a new conversation' bot message on retry. Isolate the confirmation send in its own best-effort try/catch that logs but does not fail the reset.
Suggested Code:
if (session != null)
await _chatbotSessionManager.EndSessionAsync(session.SessionId);
// Confirmation is best-effort: a failure here must not turn a successful reset into a failure.
try
{
var channel = await _chatChannelService.EnsureChatbotChannelAsync(DepartmentId, UserId);
if (channel != null)
await _chatMessageService.SendBotMessageAsync(channel.ChatChannelId,
DepartmentId.ToString(System.Globalization.CultureInfo.InvariantCulture),
"Starting a new conversation — your previous context has been cleared.", "Resgrid Assistant");
}
catch (Exception ex)
{
Logging.LogException(ex);
}
var result = new ChatbotSessionResetResult { Success = true, ... };
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (channel != null) | ||
| await _chatMessageService.SendBotMessageAsync(channel.ChatChannelId, | ||
| DepartmentId.ToString(System.Globalization.CultureInfo.InvariantCulture), | ||
| "Starting a new conversation — your previous context has been cleared.", "Resgrid Assistant"); |
There was a problem hiding this comment.
Hardcoded bot identity: the display name "Resgrid Assistant" and the confirmation message are inlined, risking drift across chatbot message paths when the brand or name changes. Extract the bot name to a shared constant (e.g., ChatbotConstants.AssistantName) and centralize reusable message templates.
Kody rule violation: Centralize string constants
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 456:
Hardcoded bot identity: the display name "Resgrid Assistant" and the confirmation message are inlined, risking drift across chatbot message paths when the brand or name changes. Extract the bot name to a shared constant (e.g., ChatbotConstants.AssistantName) and centralize reusable message templates.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <summary> | ||
| /// Display name of the user the acknowledgment is required from (populated by GetAcks) | ||
| /// </summary> | ||
| public string DisplayName { get; set; } |
There was a problem hiding this comment.
Uninitialized auto-property: DisplayName has no default value, risking null references per Rule [31]. Initialize it with = string.Empty.
Kody rule violation: Initialize properties with default values
Prompt for LLM
File Web/Resgrid.Web.Services/Models/v4/Chat/ChatApiModels.cs:
Line 748:
Uninitialized auto-property: `DisplayName` has no default value, risking null references per Rule [31]. Initialize it with `= string.Empty`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // variant='bot'; the chat page renders the same channel with the default variant): text only — | ||
| // no emoji picker, GIFs, images, urgent priority, reactions, threads or deletes. Pin, flag and | ||
| // editing your own messages stay available. | ||
| const isBot = variant === 'bot' || channel.ChannelType === ChatChannelType.Chatbot; |
There was a problem hiding this comment.
String literal 'bot' represents a value from a finite set of UI variants but is used inline, reducing type safety and inviting typo-related bugs per Rule [8]. Replace it with a named constant or TypeScript string-literal union (e.g., const ChatVariants = { Bot: 'bot', Default: 'default' } as const).
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ConversationView.tsx:
Line 62:
String literal `'bot'` represents a value from a finite set of UI variants but is used inline, reducing type safety and inviting typo-related bugs per Rule [8]. Replace it with a named constant or TypeScript string-literal union (e.g., `const ChatVariants = { Bot: 'bot', Default: 'default' } as const`).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| highlighted={message.ChatMessageId === highlightMessageId} | ||
| onReact={handleReact} | ||
| onOpenThread={variant === 'bot' ? undefined : props.onOpenThread} | ||
| showAckStatus={message.Priority === 1 && (message.SenderUserId === currentUserId || !!canModerate)} |
There was a problem hiding this comment.
Magic number: the literal 1 for message priority is not self-documenting. Extract it into a named constant such as PRIORITY_URGENT or reference an existing priority enum per Rule [9].
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ConversationView.tsx:
Line 313:
Magic number: the literal `1` for message priority is not self-documenting. Extract it into a named constant such as `PRIORITY_URGENT` or reference an existing priority enum per Rule [9].
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| allowGifs={!isBot} | ||
| allowImages={!isBot} | ||
| allowEmoji={!isBot} | ||
| placeholder={isBot ? 'Ask the assistant…' : 'Write a message…'} |
There was a problem hiding this comment.
Hardcoded user-facing strings 'Ask the assistant…' and 'Write a message…' bypass i18n, violating Rule [113]. Move these strings into the i18n dictionary and reference them via translation function calls.
Kody rule violation: Internationalize user-facing text with next-intl or next-i18next
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/ConversationView.tsx:
Line 344:
Hardcoded user-facing strings 'Ask the assistant…' and 'Write a message…' bypass i18n, violating Rule [113]. Move these strings into the i18n dictionary and reference them via translation function calls.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <button | ||
| type="button" | ||
| className={`rgchat-ackstatus__summary${allAcked ? ' rgchat-ackstatus__summary--done' : ''}`} | ||
| onClick={() => setOpen((value) => !value)} |
There was a problem hiding this comment.
Inline arrow function in the onClick JSX prop creates a new function on every render, degrading performance. Move the handler definition outside the render scope.
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/atoms/AckStatus.tsx:
Line 56:
Inline arrow function in the onClick JSX prop creates a new function on every render, degrading performance. Move the handler definition outside the render scope.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .catch(() => { | ||
| if (active) { | ||
| setAcks(null); | ||
| } |
There was a problem hiding this comment.
Silent exception swallowing: the .catch handler for the getAcks call discards rejections without logging, violating Rule 28. Capture the error, log with structured context (messageId, revision), and handle explicitly (e.g., setAcks(null) or surface a user-facing error state).
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/AckStatus.tsx:
Line 33 to 36:
Silent exception swallowing: the `.catch` handler for the `getAcks` call discards rejections without logging, violating Rule 28. Capture the error, log with structured context (messageId, revision), and handle explicitly (e.g., setAcks(null) or surface a user-facing error state).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| * or a moderator — GetAcks returns 401 for anyone else. Live-refreshes off the per-message | ||
| * ack revision the hub bumps on each chatReceiptUpdated ack event. | ||
| */ | ||
| export default function AckStatus({ messageId }: AckStatusProps) { |
There was a problem hiding this comment.
Default export reduces refactor clarity and can cause rename/import mismatches per Rule 70. Change to a named export (export function AckStatus) and update import sites accordingly.
Kody rule violation: Avoid default exports
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/AckStatus.tsx:
Line 20:
Default export reduces refactor clarity and can cause rename/import mismatches per Rule 70. Change to a named export (`export function AckStatus`) and update import sites accordingly.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const acked = acks.filter((ack) => !!ack.AcknowledgedOn); | ||
| const pending = acks.filter((ack) => !ack.AcknowledgedOn); |
There was a problem hiding this comment.
Double iteration: the acks array is traversed twice with complementary .filter() calls. Partition in a single pass using reduce to populate { acked, pending } in one traversal per Rule [98].
Kody rule violation: Optimize chained array operations
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/atoms/AckStatus.tsx:
Line 47 to 48:
Double iteration: the `acks` array is traversed twice with complementary `.filter()` calls. Partition in a single pass using `reduce` to populate `{ acked, pending }` in one traversal per Rule [98].
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| /** Sender/moderator only — the server returns 401 for everyone else. */ | ||
| export async function getAcks(messageId: string): Promise<ChatAckDto[]> { | ||
| const result = await getJson<ApiListResult<ChatAckDto>>('api/v4/Chat/GetAcks', { messageId }); |
There was a problem hiding this comment.
Unguarded async operation: the awaited getJson(...) call lacks try/catch handling, violating Rule [1]. Wrap the await in try/catch, log with context (messageId, operation name), and rethrow or return a safe default.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Apps/src/components/chat/chatApi.ts:
Line 293:
Unguarded async operation: the awaited `getJson(...)` call lacks try/catch handling, violating Rule [1]. Wrap the await in try/catch, log with context (messageId, operation name), and rethrow or return a safe default.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Summary
This PR delivers a set of chatbot and chat-related fixes and enhancements, organized around three main themes:
Assistant (Chatbot) Conversation Restrictions
Urgent Message Acknowledgments
AckRequiredhub event now includesSenderUserId, so clients correctly skip the acknowledgment banner for the sender's own urgent message.GetAcksendpoint now returns each user's display name alongside their acknowledgment data.GIF Provider Changes
GifRatingconfig (defaulting to "g") controls the Giphy content-rating cap. Only "g", "pg", and "pg-13" are permitted; any other value falls back to the workplace-safe "g" default.Summary by CodeRabbit
New Features
Bug Fixes