Fix replies and reactions to CTCP actions#157
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughActionMessage now uses SwipeableMessage, wiring reply and reaction callbacks and new props; MessageItem forwards those props; messageFormatter strips CTCP wrappers; CollapsibleMessage adds a gradient overlay when collapsed; /me sending may use whisper path or prefix draft-reply metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SwipeableMessage
participant ActionMessage
participant MessageActions
participant MessageReactions
User->>SwipeableMessage: tap / swipe / interact
SwipeableMessage->>ActionMessage: forward interaction & props
alt reply flow
ActionMessage->>MessageActions: render reply control
User->>MessageActions: click reply
MessageActions->>ActionMessage: onReplyClick
ActionMessage->>ActionMessage: setReplyTo(message)
else reaction flow
ActionMessage->>MessageReactions: render reactions
User->>MessageReactions: select emoji
MessageReactions->>ActionMessage: onReactionClick(emoji)
alt user already reacted
ActionMessage->>ActionMessage: onReactionUnreact(emoji, message)
else new reaction
ActionMessage->>ActionMessage: onDirectReaction(emoji, message)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/message/MessageItem.tsx (1)
784-803:⚠️ Potential issue | 🟡 MinorDuplicate date separator when
showDateis true.When
showDateis true, aDateSeparatoris rendered at line 788 beforeActionMessage, and thenActionMessageinternally renders its own date divider (lines 68-76 inActionMessage.tsx). This results in two date indicators appearing for ACTION messages.Consider either:
- Removing the
DateSeparatorhere (lines 787-789) sinceActionMessagehandles it internally, OR- Removing the internal date rendering from
ActionMessageand relying on the parent'sDateSeparator🔧 Option 1: Remove external DateSeparator for ACTION messages
// Handle ACTION messages if (message.content.substring(0, 7) === "\u0001ACTION") { return ( - <> - {showDate && ( - <DateSeparator date={new Date(message.timestamp)} theme={theme} /> - )} + <div data-message-id={message.id}> <ActionMessage message={message} showDate={showDate} messageUser={messageUser} onUsernameContextMenu={onUsernameContextMenu} setReplyTo={setReplyTo} onReactClick={onReactClick} onReactionUnreact={onReactionUnreact} onDirectReaction={onDirectReaction} isTouchDevice={isTouchDevice} isNarrowView={isTouchDevice} /> - </> + </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/MessageItem.tsx` around lines 784 - 803, The external DateSeparator rendered in the ACTION branch of MessageItem (the if block checking message.content.substring(0, 7) === "\u0001ACTION") duplicates the date already rendered inside ActionMessage; remove the outer DateSeparator (the JSX rendering DateSeparator before <ActionMessage />) or instead pass showDate={false} into ActionMessage to prevent double rendering—update the MessageItem ACTION branch so only ActionMessage is responsible for rendering the date, referencing the message.content check and the ActionMessage component and its showDate prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/message/MessageItem.tsx`:
- Around line 795-800: The component is passing the wrong prop value: change the
prop assignment isNarrowView={isTouchDevice} to isNarrowView={isNarrowView}
wherever MessageItem renders ActionMessage/MessageActions props (e.g., the block
that sets setReplyTo, onReactClick, onReactionUnreact, onDirectReaction,
isTouchDevice); also update the other similar render block for regular messages
that currently repeats the same mistake so swipe gestures use the viewport-based
isNarrowView value rather than the touch-only isTouchDevice value.
---
Outside diff comments:
In `@src/components/message/MessageItem.tsx`:
- Around line 784-803: The external DateSeparator rendered in the ACTION branch
of MessageItem (the if block checking message.content.substring(0, 7) ===
"\u0001ACTION") duplicates the date already rendered inside ActionMessage;
remove the outer DateSeparator (the JSX rendering DateSeparator before
<ActionMessage />) or instead pass showDate={false} into ActionMessage to
prevent double rendering—update the MessageItem ACTION branch so only
ActionMessage is responsible for rendering the date, referencing the
message.content check and the ActionMessage component and its showDate prop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4faa301f-6e46-4999-9f0e-a04911012794
📒 Files selected for processing (3)
src/components/message/ActionMessage.tsxsrc/components/message/MessageItem.tsxsrc/lib/messageFormatter.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/message/CollapsibleMessage.tsx (1)
83-99: Make the fade's hover state local to this component.
group-hover:to-discord-message-hovercurrently depends on some outer ancestor providing Tailwind'sgroupclass. If that wrapper is missing in any call site, the new overlay keeps the non-hover color and won't match the hovered message background.💡 Suggested tweak
- <div className="relative"> + <div className="relative group">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/CollapsibleMessage.tsx` around lines 83 - 99, The fade overlay's hover style relies on an external Tailwind "group" ancestor, causing mismatches when that wrapper is missing; to localize it, add the "group" class to this component's own wrapper (the div with className="relative" in CollapsibleMessage.tsx) so the existing group-hover:to-discord-message-hover on the overlay will work locally, or alternatively implement a local hover state (isHovered via onMouseEnter/onMouseLeave on the same wrapper) and toggle a class on the overlay based on that state (references: the outer wrapper div with className="relative", the overlay div that contains group-hover:to-discord-message-hover, and props/state names needsCollapsing/isExpanded).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/message/CollapsibleMessage.tsx`:
- Around line 83-99: The fade overlay's hover style relies on an external
Tailwind "group" ancestor, causing mismatches when that wrapper is missing; to
localize it, add the "group" class to this component's own wrapper (the div with
className="relative" in CollapsibleMessage.tsx) so the existing
group-hover:to-discord-message-hover on the overlay will work locally, or
alternatively implement a local hover state (isHovered via
onMouseEnter/onMouseLeave on the same wrapper) and toggle a class on the overlay
based on that state (references: the outer wrapper div with
className="relative", the overlay div that contains
group-hover:to-discord-message-hover, and props/state names
needsCollapsing/isExpanded).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c4c6de2-9ee4-4cbb-a68e-2b2b8536e6ef
📒 Files selected for processing (1)
src/components/message/CollapsibleMessage.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useMessageSending.ts (1)
155-163:⚠️ Potential issue | 🔴 CriticalRoute
/mereplies through whisper transport.When
localReplyTois a whisper, this branch still sendsPRIVMSG ${selectedChannel?.name}. That turns an intended private reply into a public/meaction in the channel. ReusegetWhisperContext(...)here, the same waysendRegularMessagedoes.Suggested fix
} else if (commandName === "me") { const actionMessage = cleanedText.substring(4).trim(); + const whisperContext = getWhisperContext(localReplyTo, currentUser); const target = selectedChannel?.name ?? selectedPrivateChat?.username ?? ""; - if (!target) return; - ircClient.sendRaw( - selectedServerId, - `${localReplyTo?.msgid ? `@+draft/reply=${localReplyTo.msgid} ` : ""}PRIVMSG ${target} :\u0001ACTION ${actionMessage}\u0001`, - ); + if (whisperContext) { + ircClient.sendWhisper( + selectedServerId, + whisperContext.targetUser, + whisperContext.channelContext, + `\u0001ACTION ${actionMessage}\u0001`, + ); + } else { + if (!target) return; + const replyPrefix = localReplyTo?.msgid + ? `@+draft/reply=${localReplyTo.msgid} ` + : ""; + ircClient.sendRaw( + selectedServerId, + `${replyPrefix}PRIVMSG ${target} :\u0001ACTION ${actionMessage}\u0001`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useMessageSending.ts` around lines 155 - 163, The /me branch currently computes target as selectedChannel?.name and always sends a PRIVMSG, which makes whisper replies public; update the branch in useMessageSending (inside the commandName === "me" block) to reuse getWhisperContext(localReplyTo, selectedServerId, selectedChannel, selectedPrivateChat) exactly as sendRegularMessage does, derive the correct target and transport from that context (fall back to selectedChannel?.name or selectedPrivateChat?.username if no whisper context), and send the ACTION to that resolved target (preserving the @+draft/reply= prefix when localReplyTo exists) so whisper replies are routed via the whisper transport instead of the channel. Ensure you reference getWhisperContext, localReplyTo, actionMessage, and ircClient.sendRaw when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/message/ActionMessage.tsx`:
- Around line 117-126: The reactions component is using currentUser?.username
which can be undefined and causes “mine” to never be set; replace that with the
same fallback identity used elsewhere in the send path so reaction ownership is
computed consistently—for example, change the prop passed to MessageReactions
from currentUser?.username to the shared fallback expression or helper used
elsewhere (the same value you use everywhere you render/send messages), ensuring
MessageReactions receives a non-empty username so onReactionUnreact vs
onDirectReaction branches behave correctly.
---
Outside diff comments:
In `@src/hooks/useMessageSending.ts`:
- Around line 155-163: The /me branch currently computes target as
selectedChannel?.name and always sends a PRIVMSG, which makes whisper replies
public; update the branch in useMessageSending (inside the commandName === "me"
block) to reuse getWhisperContext(localReplyTo, selectedServerId,
selectedChannel, selectedPrivateChat) exactly as sendRegularMessage does, derive
the correct target and transport from that context (fall back to
selectedChannel?.name or selectedPrivateChat?.username if no whisper context),
and send the ACTION to that resolved target (preserving the @+draft/reply=
prefix when localReplyTo exists) so whisper replies are routed via the whisper
transport instead of the channel. Ensure you reference getWhisperContext,
localReplyTo, actionMessage, and ircClient.sendRaw when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e0b54b8-1a5a-4c0c-acf8-934f43bcdf58
📒 Files selected for processing (2)
src/components/message/ActionMessage.tsxsrc/hooks/useMessageSending.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/message/ActionMessage.tsx (1)
117-119:⚠️ Potential issue | 🟠 MajorUse a truthy fallback for reaction ownership.
currentUser?.username ?? currentUser?.idstill keeps"". The rest of the send path usescurrentUser.username || currentUser.id; if the username is empty here, existing reactions are never marked as yours and clicks go down the react path instead of unreact.🔧 Suggested fix
- currentUserUsername={currentUser?.username ?? currentUser?.id} + currentUserUsername={currentUser?.username || currentUser?.id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/ActionMessage.tsx` around lines 117 - 119, The prop for MessageReactions is using the nullish coalescing operator which still allows an empty string (currentUser?.username ?? currentUser?.id), causing ownership checks to fail; change the fallback to a truthy fallback like currentUser?.username || currentUser?.id so empty usernames fall back to the id—update the JSX where MessageReactions is rendered (prop currentUserUsername) and ensure any related ownership checks in MessageReactions expect a falsy-empty handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useMessageSending.ts`:
- Around line 160-166: The whispered /me path in useMessageSending.ts calls
ircClient.sendWhisper but that helper only emits draft/channel-context and never
includes the reply ID (localReplyTo.msgid), so whispered action replies arrive
unthreaded; update the call site or ircClient.sendWhisper to include the reply
tag (draft/reply) by passing localReplyTo.msgid (or switch to the helper that
sends both channel-context and reply tags), ensuring sendWhisper (in
src/lib/ircClient.ts) serializes draft/reply along with draft/channel-context
when a localReplyTo exists so replies to whispered ACTION messages preserve
threading.
---
Duplicate comments:
In `@src/components/message/ActionMessage.tsx`:
- Around line 117-119: The prop for MessageReactions is using the nullish
coalescing operator which still allows an empty string (currentUser?.username ??
currentUser?.id), causing ownership checks to fail; change the fallback to a
truthy fallback like currentUser?.username || currentUser?.id so empty usernames
fall back to the id—update the JSX where MessageReactions is rendered (prop
currentUserUsername) and ensure any related ownership checks in MessageReactions
expect a falsy-empty handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efc98c4e-4716-4b78-b2f3-9ef9a040a364
📒 Files selected for processing (2)
src/components/message/ActionMessage.tsxsrc/hooks/useMessageSending.ts
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UX
Bug Fixes