fix/dm replies and history#156
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughUpdates across UI, hooks, store, client, and tests: layout fixes in message components, reworked reply UI, reaction popover and anchoring, private-message and /me send behavior adjusted with capability checks, centralized reply resolution in store, IRC client capability API, and new DM multiline tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (1)
tests/store/multilineDedup.test.ts (1)
256-261: Tighten reply-link assertion for stronger regression protection.At Line 260, you assert sender identity; consider also asserting the referenced message identity (
id/msgid) so the test fails if reply linkage points to the wrong message from the same user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/store/multilineDedup.test.ts` around lines 256 - 261, The test currently checks replyMessage is present and the userId is "alice" but doesn't verify which message is being referenced; update the assertion in the same test after accessing stored?.replyMessage (from useStore.getState(), state.messages, dmChannelKey, stored) to also assert the referenced message identity (e.g., stored?.replyMessage?.id or stored?.replyMessage?.msgid) equals the expected original message id (replace with the actual original message id used in the test, e.g., "orig-msg") so the test fails if the reply links to the wrong message even when the sender matches.
🤖 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/store/index.ts`:
- Around line 3675-3679: Guard the use of target before calling
target.toLowerCase(): in the block that checks if
currentUser?.username.toLowerCase() === sender.toLowerCase(), ensure target is
non-null/undefined (and a string) before using target.toLowerCase() — e.g.,
return or skip the DM echo handling when target is falsy, or normalize target
with a safe default (use optional chaining/default '') before passing it into
the server.privateChats?.find comparator; update the lookup that assigns
privateChat (and any other places using target.toLowerCase()) to use the
guarded/normalized value to avoid runtime exceptions.
- Around line 130-139: The reply resolver in resolveReplyMessage only matches
messages by message.msgid, so replies referencing IDs in a message's
multilineMessageIds won't resolve; update the lookup logic (either in
resolveReplyMessage or inside findChannelMessageById) to consider both
message.msgid and any entries in message.multilineMessageIds when comparing to
replyId (i.e., treat a message as a match if replyId === message.msgid OR
replyId is included in message.multilineMessageIds), ensuring the function still
returns the found Message or null as before.
---
Nitpick comments:
In `@tests/store/multilineDedup.test.ts`:
- Around line 256-261: The test currently checks replyMessage is present and the
userId is "alice" but doesn't verify which message is being referenced; update
the assertion in the same test after accessing stored?.replyMessage (from
useStore.getState(), state.messages, dmChannelKey, stored) to also assert the
referenced message identity (e.g., stored?.replyMessage?.id or
stored?.replyMessage?.msgid) equals the expected original message id (replace
with the actual original message id used in the test, e.g., "orig-msg") so the
test fails if the reply links to the wrong message even when the sender matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6a5690b-b1b6-4abc-b2bf-232a19b3e6aa
📒 Files selected for processing (6)
src/components/message/MessageItem.tsxsrc/components/message/MessageReply.tsxsrc/hooks/useMessageSending.tssrc/lib/ircClient.tssrc/store/index.tstests/store/multilineDedup.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/index.ts (1)
3715-3739:⚠️ Potential issue | 🟠 MajorUse
openPrivateChat()for multiline-first DMs.Unlike the single-line DM path on Line 3978, this branch hand-rolls a
PrivateChat. That skips the side effects inopenPrivateChat()—MONITOR, WHO, andCHATHISTORY LATEST—so a DM whose first message is multiline comes up without presence hydration or history backfill.Suggested fix
let privateChat = server.privateChats.find( (chat) => chat.username.toLowerCase() === sender.toLowerCase(), ); if (!privateChat) { - const newPrivateChat = { - id: generateDeterministicId(server.id, sender), - username: sender, - serverId: server.id, - unreadCount: 0, - isMentioned: false, - lastActivity: new Date(), - isPinned: false, - order: undefined, - isOnline: false, - isAway: false, - }; - privateChat = newPrivateChat; - useStore.setState((state) => ({ - servers: state.servers.map((s) => - s.id === server.id - ? { ...s, privateChats: [...s.privateChats, newPrivateChat] } - : s, - ), - })); + useStore.getState().openPrivateChat(server.id, sender); + privateChat = useStore + .getState() + .servers.find((s) => s.id === server.id) + ?.privateChats?.find( + (pc) => pc.username.toLowerCase() === sender.toLowerCase(), + ); + } + + if (!privateChat) { + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 3715 - 3739, This branch creates a PrivateChat object manually (using generateDeterministicId and directly updating servers[].privateChats) which skips important side effects; instead call the existing openPrivateChat(sender, server.id) (or the function signature used elsewhere) when no privateChat is found so MONITOR, WHO and CHATHISTORY LATEST are executed and presence/history are hydrated; remove the ad-hoc newPrivateChat/state update and rely on openPrivateChat to create and insert the chat, then assign privateChat from the result before proceeding.
🤖 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/MessageReply.tsx`:
- Around line 48-50: The dynamic Tailwind class `text-${theme}-text-muted` in
the MessageReply component will be purged; replace it with a deterministic
mapping and conditional lookup: add a theme-to-class map (e.g., themeTextMuted)
and use that map when building the className for the div that renders
plainContent so you pass a static class string (fallback to a safe default like
"text-discord-text-muted"); update the className expression in the MessageReply
JSX (the div containing plainContent) to use the mapped value instead of the
template literal.
In `@src/store/index.ts`:
- Around line 3937-3959: The DM-echo branch that creates newMessage and calls
useStore.getState().addMessage(...) does not add the echoed message's
mtags?.msgid to the dedupe set, so add the echo's msgid into processedMessageIds
(or call the existing function that marks a msgid processed) immediately after
creating/adding newMessage (before returning) to ensure mtags?.msgid is recorded
and the USERMSG dedupe gate (processedMessageIds) is respected for future
replays/duplicate echoes.
- Around line 130-138: resolveReplyMessage currently only looks up replyId
directly via findChannelMessageById, so replies that target per-line multiline
aliases don't resolve; change resolveReplyMessage to, after a failed direct
lookup, consult the multiline alias mapping (the multilineMessageIds structure)
for the server/channel to map the replyId back to the root message id and then
call findChannelMessageById with that root id; specifically, in
resolveReplyMessage use the existing replyId, call
findChannelMessageById(serverId, channelId, replyId) first, and if it returns
null, query multilineMessageIds for the given serverId/channelId to get the
canonical msgid for that alias and retry findChannelMessageById with that
canonical id so multiline-line replies resolve correctly.
---
Outside diff comments:
In `@src/store/index.ts`:
- Around line 3715-3739: This branch creates a PrivateChat object manually
(using generateDeterministicId and directly updating servers[].privateChats)
which skips important side effects; instead call the existing
openPrivateChat(sender, server.id) (or the function signature used elsewhere)
when no privateChat is found so MONITOR, WHO and CHATHISTORY LATEST are executed
and presence/history are hydrated; remove the ad-hoc newPrivateChat/state update
and rely on openPrivateChat to create and insert the chat, then assign
privateChat from the result before proceeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ae7943a-5c42-4fec-8bf3-390d0b5bbe89
📒 Files selected for processing (7)
src/components/message/MessageItem.tsxsrc/components/message/MessageReply.tsxsrc/hooks/useMessageSending.tssrc/hooks/useReactions.tssrc/lib/ircClient.tssrc/store/index.tstests/store/multilineDedup.test.ts
Comment on lines
+48
to
+50
| <div className={`text-xs text-${theme}-text-muted opacity-80 truncate`}> | ||
| {plainContent} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Dynamic Tailwind class won't be purged correctly.
The template literal text-${theme}-text-muted generates dynamic class names at runtime (e.g., text-discord-text-muted). Tailwind's purge/content scanner cannot detect these classes during the build, so they may not be included in the production CSS unless explicitly safelisted.
🔧 Suggested fix: Use conditional classes or a lookup
- <div className={`text-xs text-${theme}-text-muted opacity-80 truncate`}>
+ <div className="text-xs text-discord-text-muted opacity-80 truncate">If multiple themes are needed, consider using a class map:
const themeTextMuted: Record<string, string> = {
discord: "text-discord-text-muted",
// add other themes as needed
};
// ...
<div className={`text-xs ${themeTextMuted[theme] || "text-discord-text-muted"} opacity-80 truncate`}>📝 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.
Suggested change
| <div className={`text-xs text-${theme}-text-muted opacity-80 truncate`}> | |
| {plainContent} | |
| </div> | |
| const themeTextMuted: Record<string, string> = { | |
| discord: "text-discord-text-muted", | |
| // add other themes as needed | |
| }; | |
| // In the render: | |
| <div className={`text-xs ${themeTextMuted[theme] || "text-discord-text-muted"} opacity-80 truncate`}> | |
| {plainContent} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/message/MessageReply.tsx` around lines 48 - 50, The dynamic
Tailwind class `text-${theme}-text-muted` in the MessageReply component will be
purged; replace it with a deterministic mapping and conditional lookup: add a
theme-to-class map (e.g., themeTextMuted) and use that map when building the
className for the div that renders plainContent so you pass a static class
string (fallback to a safe default like "text-discord-text-muted"); update the
className expression in the MessageReply JSX (the div containing plainContent)
to use the mapped value instead of the template literal.
Comment on lines
+130
to
+138
| const resolveReplyMessage = ( | ||
| mtags: Record<string, string> | undefined, | ||
| serverId: string, | ||
| channelId: string, | ||
| ): Message | null => { | ||
| const replyId = mtags?.["+draft/reply"]?.trim() || null; | ||
| return replyId | ||
| ? (findChannelMessageById(serverId, channelId, replyId) ?? null) | ||
| : null; |
Contributor
There was a problem hiding this comment.
Resolve replies against multiline aliases too.
Line 136 now funnels every reply through a lookup that only matches message.msgid. Multiline messages in this file also keep per-line ids in multilineMessageIds, and Line 3296 already treats those ids as valid aliases for later events. If a reply points at one of those ids, the bubble will still resolve to null.
Suggested fix
const resolveReplyMessage = (
mtags: Record<string, string> | undefined,
serverId: string,
channelId: string,
): Message | null => {
const replyId = mtags?.["+draft/reply"]?.trim() || null;
- return replyId
- ? (findChannelMessageById(serverId, channelId, replyId) ?? null)
- : null;
+ if (!replyId) {
+ return null;
+ }
+
+ const messages = getChannelMessages(serverId, channelId);
+ return (
+ messages.find(
+ (message) =>
+ message.msgid === replyId ||
+ message.multilineMessageIds?.includes(replyId),
+ ) ?? null
+ );
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/index.ts` around lines 130 - 138, resolveReplyMessage currently
only looks up replyId directly via findChannelMessageById, so replies that
target per-line multiline aliases don't resolve; change resolveReplyMessage to,
after a failed direct lookup, consult the multiline alias mapping (the
multilineMessageIds structure) for the server/channel to map the replyId back to
the root message id and then call findChannelMessageById with that root id;
specifically, in resolveReplyMessage use the existing replyId, call
findChannelMessageById(serverId, channelId, replyId) first, and if it returns
null, query multilineMessageIds for the given serverId/channelId to get the
canonical msgid for that alias and retry findChannelMessageById with that
canonical id so multiline-line replies resolve correctly.
Comment on lines
+3937
to
+3959
| // Own message echo — store under the DM keyed by `target`, not `sender`. | ||
| if (server && target) { | ||
| const privateChat = server.privateChats?.find( | ||
| (pc) => pc.username.toLowerCase() === target.toLowerCase(), | ||
| ); | ||
| if (privateChat) { | ||
| const newMessage = { | ||
| id: uuidv4(), | ||
| msgid: mtags?.msgid, | ||
| content: message, | ||
| timestamp, | ||
| userId: sender, | ||
| channelId: privateChat.id, | ||
| serverId: server.id, | ||
| type: "message" as const, | ||
| reactions: [], | ||
| replyMessage: resolveReplyMessage(mtags, server.id, privateChat.id), | ||
| mentioned: [], | ||
| tags: mtags, | ||
| }; | ||
| useStore.getState().addMessage(newMessage); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Record the echoed DM msgid before returning.
Line 3957 adds the echoed message, but this branch never updates processedMessageIds. That makes outbound DM echoes the only USERMSG path that bypasses the msgid dedupe gate on Lines 3800-3806, so the same message can still be accepted again during history replay or a duplicate echo.
Suggested fix
if (privateChat) {
const newMessage = {
id: uuidv4(),
msgid: mtags?.msgid,
content: message,
timestamp,
userId: sender,
channelId: privateChat.id,
serverId: server.id,
type: "message" as const,
reactions: [],
replyMessage: resolveReplyMessage(mtags, server.id, privateChat.id),
mentioned: [],
tags: mtags,
};
+ if (mtags?.msgid) {
+ useStore.setState((state) => ({
+ processedMessageIds: new Set([
+ ...state.processedMessageIds,
+ mtags.msgid,
+ ]),
+ }));
+ }
useStore.getState().addMessage(newMessage);
}📝 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.
Suggested change
| // Own message echo — store under the DM keyed by `target`, not `sender`. | |
| if (server && target) { | |
| const privateChat = server.privateChats?.find( | |
| (pc) => pc.username.toLowerCase() === target.toLowerCase(), | |
| ); | |
| if (privateChat) { | |
| const newMessage = { | |
| id: uuidv4(), | |
| msgid: mtags?.msgid, | |
| content: message, | |
| timestamp, | |
| userId: sender, | |
| channelId: privateChat.id, | |
| serverId: server.id, | |
| type: "message" as const, | |
| reactions: [], | |
| replyMessage: resolveReplyMessage(mtags, server.id, privateChat.id), | |
| mentioned: [], | |
| tags: mtags, | |
| }; | |
| useStore.getState().addMessage(newMessage); | |
| } | |
| } | |
| // Own message echo — store under the DM keyed by `target`, not `sender`. | |
| if (server && target) { | |
| const privateChat = server.privateChats?.find( | |
| (pc) => pc.username.toLowerCase() === target.toLowerCase(), | |
| ); | |
| if (privateChat) { | |
| const newMessage = { | |
| id: uuidv4(), | |
| msgid: mtags?.msgid, | |
| content: message, | |
| timestamp, | |
| userId: sender, | |
| channelId: privateChat.id, | |
| serverId: server.id, | |
| type: "message" as const, | |
| reactions: [], | |
| replyMessage: resolveReplyMessage(mtags, server.id, privateChat.id), | |
| mentioned: [], | |
| tags: mtags, | |
| }; | |
| if (mtags?.msgid) { | |
| useStore.setState((state) => ({ | |
| processedMessageIds: new Set([ | |
| ...state.processedMessageIds, | |
| mtags.msgid, | |
| ]), | |
| })); | |
| } | |
| useStore.getState().addMessage(newMessage); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/index.ts` around lines 3937 - 3959, The DM-echo branch that creates
newMessage and calls useStore.getState().addMessage(...) does not add the echoed
message's mtags?.msgid to the dedupe set, so add the echo's msgid into
processedMessageIds (or call the existing function that marks a msgid processed)
immediately after creating/adding newMessage (before returning) to ensure
mtags?.msgid is recorded and the USERMSG dedupe gate (processedMessageIds) is
respected for future replays/duplicate echoes.
ValwareIRC
approved these changes
Mar 10, 2026
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
Bug Fixes
Refactor
Tests