feat/emoji improvements and fixes#65
Conversation
And click again to just add
WalkthroughAdds a suite of message UI components, a centralized MessageItem, IRC URL parsing and message-formatting libraries, enhanced link handling, portal-based emoji picker integration, updated IRC event typings, unit tests, and a new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant MI as MessageItem
participant CA as ChatArea
participant IRC as IRC Client
rect rgba(230,245,255,0.6)
note over U,MI: Direct reaction flow (new)
U->>MI: Click react / choose emoji
MI-->>CA: onDirectReaction(emoji, message)
CA->>CA: Build draft reaction (TAGMSG payload)
CA-->>IRC: Send reaction command
IRC-->>CA: Event / ack with updated reactions
CA-->>MI: Re-render message reactions
end
sequenceDiagram
autonumber
participant U as User
participant LW as EnhancedLinkWrapper
participant CA as ChatArea
participant PARSE as parseIrcUrl
rect rgba(240,255,240,0.6)
note over U,PARSE: IRC link handling (click -> parse -> prefill)
U->>LW: Click irc://... link
LW-->>CA: onIrcLinkClick(url)
CA->>PARSE: parseIrcUrl(url, username)
PARSE-->>CA: { host, port, scheme, channels, nick, password }
CA->>CA: Prefill connection/join flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
matheusfillipe
force-pushed
the
feat/emoji-improvements-and-fixes
branch
from
77f68de to
5138fd5
Compare
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/layout/ChatArea.tsx (1)
333-351: Fix private DM echo to preserve IRC formattingWhen we fall back to locally echoing a private message (because the server doesn’t echo it), we currently store
messageText. That drops all the IRC control codes we just added, so the sender sees a plain message instead of the colored/styled version they actually transmitted. Channel sends still render correctly because the server echoes the formatted payload, but DMs lose the formatting entirely. Please store the formatted string (or otherwise retain the formatting metadata) when adding the message to the store.- const outgoingMessage = { + const outgoingMessage = { id: uuidv4(), - content: messageText, + content: formattedText,
🧹 Nitpick comments (9)
src/components/ui/ReactionModal.tsx (1)
30-48: Add modal semantics for assistive techRight now the portal renders a plain div, so screen readers won’t announce it as a dialog. Please add
role="dialog"andaria-modal="true"(plus an accessible name) so assistive tech recognises the overlay as a modal surface.- <div - className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50" - onClick={handleBackdropClick} - > + <div + className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50" + role="dialog" + aria-modal="true" + aria-label="Emoji picker" + onClick={handleBackdropClick} + >src/components/message/MessageAvatar.tsx (1)
33-36: Make the avatar focusable/keyboard-activable.Use a button for clickability; adds built-in keyboard support and semantics.
- <div - className={`mr-4 ${isClickable ? "cursor-pointer" : ""}`} - onClick={onClick} - > + <button + type="button" + aria-label={`Open profile for ${username}`} + className={`mr-4 ${isClickable ? "cursor-pointer" : ""} bg-transparent p-0`} + onClick={onClick} + disabled={!isClickable || !onClick} + > @@ - </div> + </button>Also applies to: 70-70
src/components/message/MessageHeader.tsx (3)
35-46: Clickable name should be a button for accessibility.Swap span for button when interactive; keep span otherwise.
- <span - className={`font-bold text-white ${isClickable ? "cursor-pointer" : ""}`} - style={getColorStyle(userColor)} - onClick={onClick} - > - {isSystem ? "System" : displayName || username} - {displayName && ( - <span className="ml-2 text-xs bg-discord-dark-600 px-1 py-0.5 rounded"> - {username} - </span> - )} - </span> + {isClickable ? ( + <button + type="button" + className="font-bold text-white cursor-pointer bg-transparent p-0" + style={getColorStyle(userColor)} + onClick={onClick} + aria-label={`Open profile for ${displayName || username}`} + > + {isSystem ? "System" : displayName || username} + {displayName && ( + <span className="ml-2 text-xs bg-discord-dark-600 px-1 py-0.5 rounded"> + {username} + </span> + )} + </button> + ) : ( + <span className="font-bold text-white" style={getColorStyle(userColor)}> + {isSystem ? "System" : displayName || username} + {displayName && ( + <span className="ml-2 text-xs bg-discord-dark-600 px-1 py-0.5 rounded"> + {username} + </span> + )} + </span> + )}
26-31: Locale: avoid hardcoding "en-US".Use the user’s locale (undefined) or inject one from context.
- return new Intl.DateTimeFormat("en-US", { + return new Intl.DateTimeFormat(undefined, { hour: "2-digit", minute: "2-digit", }).format(date);
23-25: Deduplicate username parsing.
userId.split("-")[0]repeats here and in MessageAvatar. Consider a small util (e.g., getUsernameFromUserId) for consistency.src/components/message/MessageActions.tsx (2)
18-23: Add button semantics and labels.Set type and aria for accessibility and prevent accidental form submits.
- <button - className="bg-discord-dark-300 hover:bg-discord-dark-200 text-white px-2 py-1 rounded text-xs" - onClick={onReplyClick} - > + <button + type="button" + title="Reply" + aria-label="Reply" + className="bg-discord-dark-300 hover:bg-discord-dark-200 text-white px-2 py-1 rounded text-xs" + onClick={onReplyClick} + > <FaReply /> </button> @@ - <button - className="bg-discord-dark-300 hover:bg-discord-dark-200 text-white px-2 py-1 rounded text-xs" - onClick={(e) => onReactClick(e.currentTarget)} - > + <button + type="button" + title="Add reaction" + aria-label="Add reaction" + className="bg-discord-dark-300 hover:bg-discord-dark-200 text-white px-2 py-1 rounded text-xs" + onClick={(e) => onReactClick(e.currentTarget)} + > <FaGrinAlt /> </button>Also applies to: 25-31
5-9: Tighten the element type.
onReactClickreceives an HTML button. PreferHTMLElementfor downstream positioning APIs.- onReactClick: (buttonElement: Element) => void; + onReactClick: (buttonElement: HTMLElement) => void;tests/lib/ircUrlParser.test.ts (1)
164-166: Empty channel normalization check.Normalizing "" to "#" may be unexpected. Confirm this is desired UX; alternatively, return empty or filter empties earlier.
tests/lib/messageFormatter.test.ts (1)
1-311: Approve code;ircColorsmapping confirmed.
Optional:
- Add a test for a two-digit color index (e.g., index 10 → “\x0310”).
- Add a test covering the reset code interaction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
package.json(1 hunks)src/components/layout/ChatArea.tsx(10 hunks)src/components/message/ActionMessage.tsx(1 hunks)src/components/message/DateSeparator.tsx(1 hunks)src/components/message/MessageActions.tsx(1 hunks)src/components/message/MessageAvatar.tsx(1 hunks)src/components/message/MessageHeader.tsx(1 hunks)src/components/message/MessageItem.tsx(1 hunks)src/components/message/MessageReactions.tsx(1 hunks)src/components/message/MessageReply.tsx(1 hunks)src/components/message/SystemMessage.tsx(1 hunks)src/components/message/index.ts(1 hunks)src/components/ui/ColorPicker.tsx(1 hunks)src/components/ui/LinkWrapper.tsx(1 hunks)src/components/ui/ReactionModal.tsx(1 hunks)src/lib/ircClient.ts(1 hunks)src/lib/ircUrlParser.ts(1 hunks)src/lib/messageFormatter.ts(1 hunks)src/types/index.ts(2 hunks)tests/lib/ircUrlParser.test.ts(1 hunks)tests/lib/messageFormatter.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
src/components/message/SystemMessage.tsx (3)
src/types/index.ts (1)
MessageType(80-80)src/lib/ircUtils.tsx (1)
mircToHtml(167-279)src/components/ui/LinkWrapper.tsx (1)
EnhancedLinkWrapper(9-85)
src/components/message/MessageActions.tsx (2)
src/types/index.ts (1)
MessageType(80-80)src/components/message/index.ts (1)
MessageActions(3-3)
src/components/message/MessageAvatar.tsx (1)
src/components/message/index.ts (1)
MessageAvatar(4-4)
src/components/message/MessageHeader.tsx (1)
src/lib/ircUtils.tsx (1)
getColorStyle(282-302)
src/components/message/ActionMessage.tsx (2)
src/types/index.ts (1)
User(1-10)src/components/message/MessageAvatar.tsx (1)
MessageAvatar(13-72)
src/components/message/MessageItem.tsx (11)
src/types/index.ts (2)
MessageType(80-80)User(1-10)src/lib/ircUtils.tsx (1)
mircToHtml(167-279)src/components/message/SystemMessage.tsx (1)
SystemMessage(11-37)src/components/message/DateSeparator.tsx (1)
DateSeparator(8-27)src/components/message/ActionMessage.tsx (1)
ActionMessage(17-84)src/components/message/MessageAvatar.tsx (1)
MessageAvatar(13-72)src/components/message/MessageHeader.tsx (1)
MessageHeader(14-52)src/components/message/MessageReply.tsx (1)
MessageReply(13-37)src/components/ui/LinkWrapper.tsx (1)
EnhancedLinkWrapper(9-85)src/components/message/MessageReactions.tsx (1)
MessageReactions(16-82)src/components/message/MessageActions.tsx (1)
MessageActions(11-34)
src/components/message/MessageReactions.tsx (2)
src/types/index.ts (1)
MessageType(80-80)src/components/message/index.ts (1)
MessageReactions(6-6)
src/components/ui/ColorPicker.tsx (1)
src/lib/messageFormatter.ts (1)
FormattingType(8-14)
src/lib/messageFormatter.ts (1)
src/lib/ircUtils.tsx (1)
ircColors(64-165)
src/components/message/MessageReply.tsx (3)
src/types/index.ts (1)
MessageType(80-80)src/components/ui/LinkWrapper.tsx (1)
EnhancedLinkWrapper(9-85)src/lib/ircUtils.tsx (1)
mircToHtml(167-279)
tests/lib/messageFormatter.test.ts (1)
src/lib/messageFormatter.ts (8)
getIrcColorCode(47-56)applyIrcFormatting(71-86)MessageFormatting(16-19)formatMessageForIrc(104-120)getPreviewStyles(128-144)FormattingType(8-14)isValidFormattingType(152-164)IRC_FORMATTING_CODES(24-33)
src/components/layout/ChatArea.tsx (2)
src/lib/messageFormatter.ts (4)
FormattingType(8-14)isValidFormattingType(152-164)formatMessageForIrc(104-120)getPreviewStyles(128-144)src/lib/ircUrlParser.ts (1)
parseIrcUrl(35-76)
src/components/message/DateSeparator.tsx (1)
src/components/message/index.ts (1)
DateSeparator(2-2)
tests/lib/ircUrlParser.test.ts (1)
src/lib/ircUrlParser.ts (5)
parseIrcUrl(35-76)normalizeChannelName(85-92)isValidIrcUrl(100-109)ParsedIrcUrl(6-13)constructIrcUrl(117-145)
src/lib/ircClient.ts (1)
src/types/index.ts (7)
BaseIRCEvent(118-120)EventWithTags(123-125)BaseUserActionEvent(147-149)BaseMessageEvent(140-144)User(1-10)MetadataValueEvent(134-137)BaseMetadataEvent(128-131)
🔇 Additional comments (6)
src/components/ui/ColorPicker.tsx (1)
7-11: Typed formatting contract looks solidSwitching the picker callbacks and state to
FormattingType[]keeps the UI in lockstep with the shared formatter union, preventing accidental drift in callers. Nicely done.src/components/message/SystemMessage.tsx (1)
11-34: Clean integration with the shared formatterLeaning on
mircToHtmlplusEnhancedLinkWrapperkeeps system messages visually consistent and preserves IRC link handling without duplicating logic. The timestamp formatter is a nice touch too.src/components/message/DateSeparator.tsx (1)
21-24: Confirm Tailwind safelist for theme-bound classesCould you double-check that combinations like
text-${theme}-text-mutedandborder-${theme}-dark-400are safelisted (or otherwise guaranteed to be emitted) by Tailwind? Without a static reference, they may be purged in production builds. If we already cover them elsewhere, all good—just want to ensure we don’t lose the divider styling.src/components/message/MessageReply.tsx (1)
21-34: Reply preview ties into shared formatting nicelyRouting the quoted content through
mircToHtmlandEnhancedLinkWrapperkeeps replies visually consistent and preserves IRC link handling without extra plumbing. Looks solid.tests/lib/ircUrlParser.test.ts (1)
26-31: Tests intentionally use ports 8000 (plain) and 443 (secure) to match ObsidianIRC’s websocket-based connections, not the traditional 6667/6697—no change needed.Likely an incorrect or invalid review comment.
src/components/message/MessageActions.tsx (1)
17-17: No action needed—groupwrapper is present. The<MessageActions>component is nested inside a<div className={\… group relative`}>on line 136, so thegroup-hover:opacity-100` rule will apply as intended.
| {showDate && ( | ||
| <div className="flex items-center text-xs text-discord-text-muted mb-2"> | ||
| <div className="flex-grow border-t border-discord-dark-400" /> | ||
| <div className="px-2">{formatDate(new Date(message.timestamp))}</div> | ||
| <div className="flex-grow border-t border-discord-dark-400" /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Avoid rendering the date header twice for ACTION messages.
MessageItem already renders a DateSeparator before instantiating ActionMessage, so this additional in-component date banner produces duplicate date rows for every /me message that lands on a new day. Please drop the local date rendering (and the showDate prop) so the shared DateSeparator remains the single source of truth. Remember to remove the prop at the call site in MessageItem to keep the types aligned.
interface ActionMessageProps {
message: MessageType;
- showDate: boolean;
messageUser?: User;
onUsernameContextMenu: (
e: React.MouseEvent,
@@
-export const ActionMessage: React.FC<ActionMessageProps> = ({
- message,
- showDate,
+export const ActionMessage: React.FC<ActionMessageProps> = ({
+ message,
messageUser,
onUsernameContextMenu,
}) => {
@@
- const formatDate = (date: Date) => {
- return new Intl.DateTimeFormat("en-US", {
- month: "long",
- day: "numeric",
- year: "numeric",
- }).format(date);
- };
-
@@
- {showDate && (
- <div className="flex items-center text-xs text-discord-text-muted mb-2">
- <div className="flex-grow border-t border-discord-dark-400" />
- <div className="px-2">{formatDate(new Date(message.timestamp))}</div>
- <div className="flex-grow border-t border-discord-dark-400" />
- </div>
- )}📝 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.
| {showDate && ( | |
| <div className="flex items-center text-xs text-discord-text-muted mb-2"> | |
| <div className="flex-grow border-t border-discord-dark-400" /> | |
| <div className="px-2">{formatDate(new Date(message.timestamp))}</div> | |
| <div className="flex-grow border-t border-discord-dark-400" /> | |
| </div> | |
| )} | |
| interface ActionMessageProps { | |
| message: MessageType; | |
| messageUser?: User; | |
| onUsernameContextMenu: ( | |
| e: React.MouseEvent, | |
| userId: string | |
| ) => void; | |
| } | |
| export const ActionMessage: React.FC<ActionMessageProps> = ({ | |
| message, | |
| messageUser, | |
| onUsernameContextMenu, | |
| }) => { | |
| return ( | |
| <> | |
| {/* Local date banner removed to defer to MessageItem’s DateSeparator */} | |
| {/* ...existing action-message rendering... */} | |
| </> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/components/message/ActionMessage.tsx around lines 43 to 49, remove the
in-component date banner and the showDate prop: delete the conditional block
that renders the date header, remove showDate from the component props/interface
and all references inside ActionMessage, and update any related imports (e.g.,
formatDate) if they become unused. Also update the call site in MessageItem to
stop passing showDate to ActionMessage and remove the prop from MessageItem's
ActionMessage invocation so the types remain aligned.
| import type React from "react"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Import useState (needed for safe image fallback).
Add the runtime hook import; the current code only imports React types.
-import type React from "react";
+import type React from "react";
+import { useState } from "react";📝 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.
| import type React from "react"; | |
| import type React from "react"; | |
| import { useState } from "react"; |
🤖 Prompt for AI Agents
In src/components/message/MessageAvatar.tsx around lines 1-2, the file only
imports React types but needs the runtime hook useState for the safe image
fallback; update the import to include the useState hook (so the component can
call useState at runtime) and ensure you keep any existing type imports intact.
| showHeader, | ||
| onClick, | ||
| isClickable = false, | ||
| }) => { |
There was a problem hiding this comment.
Avoid DOM mutations in onError; use state fallback to preserve children.
parent.textContent = ... nukes the status badge and fights React reconciliation. Use an imgError flag and conditional render instead.
export const MessageAvatar: React.FC<MessageAvatarProps> = ({
@@
}) => {
- const username = userId.split("-")[0];
+ const [imgError, setImgError] = useState(false);
+ const username = userId.split("-")[0];
@@
- {avatarUrl ? (
+ {avatarUrl && !imgError ? (
<img
src={avatarUrl}
alt={username}
className="w-8 h-8 rounded-full object-cover"
- onError={(e) => {
- // Fallback to initial if image fails to load
- e.currentTarget.style.display = "none";
- const parent = e.currentTarget.parentElement;
- if (parent) {
- parent.textContent = username.charAt(0).toUpperCase();
- }
- }}
+ onError={() => setImgError(true)}
/>
) : (
- username.charAt(0).toUpperCase()
+ <span aria-hidden="true">{username.charAt(0).toUpperCase()}</span>
)}Also applies to: 41-56
🤖 Prompt for AI Agents
In src/components/message/MessageAvatar.tsx around lines 21 (and also affecting
lines 41-56), the current onError handler mutates the DOM via parent.textContent
which removes the status badge and breaks React reconciliation; replace this
with a React state-based fallback: add a useState boolean (e.g. imgError), set
imgError = true inside the image onError callback, and change the render logic
to conditionally render the fallback content (initials/text) when imgError is
true instead of touching parent.textContent — ensure the status badge and other
children are kept as part of the component output so React manages updates
correctly.
| onClick={onClick} | ||
| > | ||
| <div | ||
| className={`w-8 h-8 rounded-full bg-${theme}-dark-400 flex items-center justify-center text-white relative`} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Dynamic Tailwind class may be purged.
bg-${theme}-dark-400 won’t be seen by Tailwind’s scanner. Either map themes to explicit classes or safelist the patterns.
- Option A: map theme -> class (preferred).
- Option B: add safelist (e.g.,
/^bg-(discord|…)-dark-400$/) in tailwind.config.
🏁 Script executed:
#!/bin/bash
# Verify safelist for dynamic classes
fd -a 'tailwind.config.*' -t f | xargs -I{} rg -n 'safelist|safelistPatterns' {}Length of output: 0
Ensure dynamic Tailwind classes aren’t purged
Dynamic bg-${theme}-dark-400 won’t be detected by Tailwind’s scanner—either implement an explicit theme→class mapping (preferred) or add a safelist (e.g. /^bg-(discord|…)-dark-400$/) in your tailwind.config.
🤖 Prompt for AI Agents
In src/components/message/MessageAvatar.tsx around line 38, the dynamic Tailwind
class `bg-${theme}-dark-400` will be purged by Tailwind; replace the runtime
template string with an explicit mapping from theme keys to their full class
names (e.g. const bgClassMap = { discord: 'bg-discord-dark-400', ... } and use
bgClassMap[theme] || default) so the scanner can see concrete class names, and
ensure a sensible fallback theme; alternatively (if mapping is not feasible) add
a safelist entry in tailwind.config.js that matches these background classes
(for example a regex like /^bg-(discord|...)-dark-400$/) so Tailwind won’t purge
them.
| <span className={`ml-2 text-xs text-${theme}-text-muted`}> | ||
| {formatTime(timestamp)} | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Dynamic Tailwind class may be purged.
text-${theme}-text-muted needs explicit enumeration or a safelist.
🏁 Script executed:
#!/bin/bash
# Check for Tailwind safelist covering text-*-text-muted
fd -a 'tailwind.config.*' -t f | xargs -I{} rg -n 'safelist.*text-.*-text-muted|safelistPatterns' {}Length of output: 0
Please verify whether any safelist or whitelist entries exist in your Tailwind configuration:
🏁 Script executed:
#!/bin/bash
# Search for safelist or whitelist in Tailwind config files
fd -a 'tailwind.config.*' -t f | xargs -I{} rg -n -C2 -e 'safelist' -e 'whitelist' {}Length of output: 0
Safelist dynamic Tailwind class
The dynamic class text-${theme}-text-muted won’t be picked up by PurgeCSS; add a safelist entry or explicitly list all variants (e.g. 'text-light-text-muted', 'text-dark-text-muted') in your tailwind.config.js.
🤖 Prompt for AI Agents
In src/components/message/MessageHeader.tsx around lines 47-49, the dynamic
Tailwind class `text-${theme}-text-muted` will be removed by PurgeCSS; replace
the dynamic interpolation with an explicit mapping (e.g. compute className via a
conditional or object map that returns 'text-light-text-muted' or
'text-dark-text-muted' based on theme) or add both variants to the
tailwind.config.js safelist (e.g. 'text-light-text-muted' and
'text-dark-text-muted') so PurgeCSS preserves them.
| reactions={message.reactions} | ||
| currentUserUsername={currentUser?.username} | ||
| onReactionClick={handleReactionClick} | ||
| /> |
There was a problem hiding this comment.
Pass the current user’s ID into MessageReactions.
After fixing MessageReactions to compare IDs, we need to forward the stored identifier here as well. Keeping the username breaks the unreact flow and will keep the bug alive.
- currentUserUsername={currentUser?.username}
+ currentUserId={currentUser?.id}📝 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.
| reactions={message.reactions} | |
| currentUserUsername={currentUser?.username} | |
| onReactionClick={handleReactionClick} | |
| /> | |
| reactions={message.reactions} | |
| currentUserId={currentUser?.id} | |
| onReactionClick={handleReactionClick} | |
| /> |
🤖 Prompt for AI Agents
In src/components/message/MessageItem.tsx around lines 181 to 184, the
MessageReactions component is being passed currentUser?.username but
MessageReactions now compares user IDs; update the prop to forward the stored
identifier instead (e.g., pass currentUser?.id as currentUserId or the prop name
MessageReactions expects). Ensure the prop name matches MessageReactions' prop
interface and that you pass the nullable id (currentUser?.id) so the component
can correctly toggle/unreact by ID.
| reactions: MessageType["reactions"]; | ||
| currentUserUsername?: string; | ||
| onReactionClick: (emoji: string, currentUserReacted: boolean) => void; | ||
| } | ||
|
|
||
| export const MessageReactions: React.FC<MessageReactionsProps> = ({ | ||
| reactions, | ||
| currentUserUsername, | ||
| onReactionClick, | ||
| }) => { | ||
| if (!reactions || reactions.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| // Group reactions by emoji | ||
| const groupedReactions = reactions.reduce( | ||
| ( | ||
| acc: Record<string, ReactionData>, | ||
| reaction: { emoji: string; userId: string }, | ||
| ) => { | ||
| if (!acc[reaction.emoji]) { | ||
| acc[reaction.emoji] = { | ||
| count: 0, | ||
| users: [], | ||
| currentUserReacted: false, | ||
| }; | ||
| } | ||
| acc[reaction.emoji].count++; | ||
| acc[reaction.emoji].users.push(reaction.userId); | ||
| // Check if current user reacted with this emoji | ||
| if (reaction.userId === currentUserUsername) { | ||
| acc[reaction.emoji].currentUserReacted = true; | ||
| } |
There was a problem hiding this comment.
Use the stored reaction user ID to detect current-user reactions.
reaction.userId already contains the server-qualified identifier stored on each reaction, but this component compares it against currentUserUsername, which receives currentUser?.username. The comparison never succeeds, so the UI never marks the current user as having reacted—preventing the unreact flow and hiding the removal button. Compare against the same ID that was stored in the reaction payload (and update the prop name accordingly), then have the caller pass currentUser?.id.
interface MessageReactionsProps {
reactions: MessageType["reactions"];
- currentUserUsername?: string;
+ currentUserId?: string;
onReactionClick: (emoji: string, currentUserReacted: boolean) => void;
}
export const MessageReactions: React.FC<MessageReactionsProps> = ({
reactions,
- currentUserUsername,
+ currentUserId,
onReactionClick,
}) => {
@@
- if (reaction.userId === currentUserUsername) {
+ if (reaction.userId === currentUserId) {
acc[reaction.emoji].currentUserReacted = true;
}📝 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.
| reactions: MessageType["reactions"]; | |
| currentUserUsername?: string; | |
| onReactionClick: (emoji: string, currentUserReacted: boolean) => void; | |
| } | |
| export const MessageReactions: React.FC<MessageReactionsProps> = ({ | |
| reactions, | |
| currentUserUsername, | |
| onReactionClick, | |
| }) => { | |
| if (!reactions || reactions.length === 0) { | |
| return null; | |
| } | |
| // Group reactions by emoji | |
| const groupedReactions = reactions.reduce( | |
| ( | |
| acc: Record<string, ReactionData>, | |
| reaction: { emoji: string; userId: string }, | |
| ) => { | |
| if (!acc[reaction.emoji]) { | |
| acc[reaction.emoji] = { | |
| count: 0, | |
| users: [], | |
| currentUserReacted: false, | |
| }; | |
| } | |
| acc[reaction.emoji].count++; | |
| acc[reaction.emoji].users.push(reaction.userId); | |
| // Check if current user reacted with this emoji | |
| if (reaction.userId === currentUserUsername) { | |
| acc[reaction.emoji].currentUserReacted = true; | |
| } | |
| interface MessageReactionsProps { | |
| reactions: MessageType["reactions"]; | |
| currentUserId?: string; | |
| onReactionClick: (emoji: string, currentUserReacted: boolean) => void; | |
| } | |
| export const MessageReactions: React.FC<MessageReactionsProps> = ({ | |
| reactions, | |
| currentUserId, | |
| onReactionClick, | |
| }) => { | |
| if (!reactions || reactions.length === 0) { | |
| return null; | |
| } | |
| // Group reactions by emoji | |
| const groupedReactions = reactions.reduce( | |
| ( | |
| acc: Record<string, ReactionData>, | |
| reaction: { emoji: string; userId: string }, | |
| ) => { | |
| if (!acc[reaction.emoji]) { | |
| acc[reaction.emoji] = { | |
| count: 0, | |
| users: [], | |
| currentUserReacted: false, | |
| }; | |
| } | |
| acc[reaction.emoji].count++; | |
| acc[reaction.emoji].users.push(reaction.userId); | |
| // Check if current user reacted with this emoji | |
| if (reaction.userId === currentUserId) { | |
| acc[reaction.emoji].currentUserReacted = true; | |
| } | |
| // ...rest of reducer... |
🤖 Prompt for AI Agents
In src/components/message/MessageReactions.tsx around lines 11 to 43, the
component is comparing reaction.userId to currentUserUsername (which is
currentUser?.username) so current-user reactions never match; rename the prop to
currentUserId (and update its type to string | undefined), compare
reaction.userId === currentUserId when setting currentUserReacted, and ensure
callers pass currentUser?.id (not username); also keep the optional handling
(undefined) so the check is safe.
| <a | ||
| href={matches[index]} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="text-discord-text-link underline hover:text-blue-700" | ||
| onClick={(e) => { | ||
| if ( | ||
| (matches[index].startsWith("ircs://") || | ||
| matches[index].startsWith("irc://")) && | ||
| onIrcLinkClick | ||
| ) { | ||
| e.preventDefault(); | ||
| onIrcLinkClick(matches[index]); | ||
| } | ||
| }} | ||
| > | ||
| {matches[index]} | ||
| </a> | ||
| </Fragment> |
There was a problem hiding this comment.
Handle trailing punctuation when linkifying text
On Line 31, the regex match still includes trailing punctuation (e.g., ) or .) so the generated <a> gets href="https://example.com)". That breaks very common patterns like See (https://example.com) because the browser requests the malformed URL. Please strip the punctuation from the link target (and hand the cleaned URL to onIrcLinkClick) while rendering the punctuation back as plain text so the original message formatting is preserved.
Apply this diff to sanitize the URLs and keep the punctuation outside the anchor:
- const fragmentKey = `fragment-${matches[index]}-${index}`;
+ const rawMatch = matches[index];
+ const trimmedMatch = rawMatch.replace(/[),.;:]+$/, "");
+ const trailingPunctuation = rawMatch.slice(trimmedMatch.length);
+ const fragmentKey = `fragment-${trimmedMatch}-${index}`;
+ const trailingKey = `trailing-${trailingPunctuation}-${index}`;
return (
<Fragment key={fragmentKey}>
{textPart}
<a
- href={matches[index]}
+ href={trimmedMatch}
target="_blank"
rel="noopener noreferrer"
className="text-discord-text-link underline hover:text-blue-700"
onClick={(e) => {
if (
- (matches[index].startsWith("ircs://") ||
- matches[index].startsWith("irc://")) &&
+ (trimmedMatch.startsWith("ircs://") ||
+ trimmedMatch.startsWith("irc://")) &&
onIrcLinkClick
) {
e.preventDefault();
- onIrcLinkClick(matches[index]);
+ onIrcLinkClick(trimmedMatch);
}
}}
>
- {matches[index]}
+ {trimmedMatch}
</a>
+ {trailingPunctuation ? (
+ <span key={trailingKey}>{trailingPunctuation}</span>
+ ) : null}
</Fragment>
);📝 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.
| <a | |
| href={matches[index]} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="text-discord-text-link underline hover:text-blue-700" | |
| onClick={(e) => { | |
| if ( | |
| (matches[index].startsWith("ircs://") || | |
| matches[index].startsWith("irc://")) && | |
| onIrcLinkClick | |
| ) { | |
| e.preventDefault(); | |
| onIrcLinkClick(matches[index]); | |
| } | |
| }} | |
| > | |
| {matches[index]} | |
| </a> | |
| </Fragment> | |
| const rawMatch = matches[index]; | |
| const trimmedMatch = rawMatch.replace(/[),.;:]+$/, ""); | |
| const trailingPunctuation = rawMatch.slice(trimmedMatch.length); | |
| const fragmentKey = `fragment-${trimmedMatch}-${index}`; | |
| const trailingKey = `trailing-${trailingPunctuation}-${index}`; | |
| return ( | |
| <Fragment key={fragmentKey}> | |
| {textPart} | |
| <a | |
| href={trimmedMatch} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="text-discord-text-link underline hover:text-blue-700" | |
| onClick={(e) => { | |
| if ( | |
| (trimmedMatch.startsWith("ircs://") || | |
| trimmedMatch.startsWith("irc://")) && | |
| onIrcLinkClick | |
| ) { | |
| e.preventDefault(); | |
| onIrcLinkClick(trimmedMatch); | |
| } | |
| }} | |
| > | |
| {trimmedMatch} | |
| </a> | |
| {trailingPunctuation ? ( | |
| <span key={trailingKey}>{trailingPunctuation}</span> | |
| ) : null} | |
| </Fragment> | |
| ); |
| const port = urlObj.port | ||
| ? Number.parseInt(urlObj.port, 10) | ||
| : scheme === "ircs" | ||
| ? 443 | ||
| : 8000; |
There was a problem hiding this comment.
Fix default IRC port selection
Line 44 hard-codes 443/8000. IRC clients expect 6697 for TLS (ircs://) and 6667 for plain (irc://). Defaulting to 443/8000 will fail to connect on most networks when the URL omits an explicit port, effectively breaking the feature for the common case.
Use the canonical defaults instead:
- const port = urlObj.port
- ? Number.parseInt(urlObj.port, 10)
- : scheme === "ircs"
- ? 443
- : 8000;
+ const port = urlObj.port
+ ? Number.parseInt(urlObj.port, 10)
+ : scheme === "ircs"
+ ? 6697
+ : 6667;📝 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.
| const port = urlObj.port | |
| ? Number.parseInt(urlObj.port, 10) | |
| : scheme === "ircs" | |
| ? 443 | |
| : 8000; | |
| const port = urlObj.port | |
| ? Number.parseInt(urlObj.port, 10) | |
| : scheme === "ircs" | |
| ? 6697 | |
| : 6667; |
🤖 Prompt for AI Agents
In src/lib/ircUrlParser.ts around lines 44 to 48, the code currently defaults
ports to 443/8000 for ircs/irc when urlObj.port is missing; change the defaults
to the canonical IRC ports: use 6697 for "ircs" (TLS) and 6667 for plain "irc".
Replace the hard-coded 443/8000 fallback logic so it checks scheme === "ircs" ?
6697 : 6667 and still parses an explicit urlObj.port into a Number with base 10
when present.
| import { ircColors } from "./ircUtils"; | ||
|
|
||
| export type FormattingType = | ||
| | "bold" | ||
| | "italic" | ||
| | "underline" | ||
| | "strikethrough" | ||
| | "reverse" | ||
| | "monospace"; | ||
|
|
||
| export interface MessageFormatting { | ||
| color?: string; | ||
| formatting: FormattingType[]; | ||
| } | ||
|
|
||
| /** | ||
| * IRC formatting control codes | ||
| */ | ||
| export const IRC_FORMATTING_CODES = { | ||
| bold: "\x02", | ||
| italic: "\x1D", | ||
| underline: "\x1F", | ||
| strikethrough: "\x1E", | ||
| reverse: "\x16", | ||
| monospace: "\x11", | ||
| color: "\x03", | ||
| reset: "\x0F", | ||
| } as const; | ||
|
|
||
| /** | ||
| * Gets the IRC color code for a given color name | ||
| * | ||
| * @param color - The color name or 'inherit' for no color | ||
| * @returns IRC color code string (e.g., '\x0301' for black) | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * getIrcColorCode('red') // Returns '\x0304' | ||
| * getIrcColorCode('inherit') // Returns '' | ||
| * ``` | ||
| */ | ||
| export function getIrcColorCode(color: string): string { | ||
| if (color === "inherit") return ""; | ||
|
|
||
| const index = ircColors.indexOf(color); | ||
| if (index === -1 || index === 99) return ""; | ||
|
|
||
| // Format as two-digit number (e.g., 01, 02, 10) | ||
| const formattedIndex = index < 10 ? `0${index}` : `${index}`; | ||
| return `${IRC_FORMATTING_CODES.color}${formattedIndex}`; | ||
| } | ||
|
|
||
| /** | ||
| * Applies IRC formatting codes to text | ||
| * | ||
| * @param text - The text to format | ||
| * @param formatting - Array of formatting types to apply | ||
| * @returns Text wrapped with IRC formatting codes | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * applyIrcFormatting('Hello', ['bold', 'italic']) | ||
| * // Returns '\x02\x1DHello\x1D\x02' | ||
| * ``` | ||
| */ | ||
| export function applyIrcFormatting( | ||
| text: string, | ||
| formatting: FormattingType[], | ||
| ): string { | ||
| let formattedText = text; | ||
|
|
||
| // Apply each formatting type | ||
| for (const format of formatting) { | ||
| const code = IRC_FORMATTING_CODES[format]; | ||
| if (code) { | ||
| formattedText = `${code}${formattedText}${code}`; | ||
| } | ||
| } | ||
|
|
||
| return formattedText; | ||
| } | ||
|
|
||
| /** | ||
| * Formats a message with color and styling for IRC transmission | ||
| * | ||
| * @param text - The message text | ||
| * @param options - Formatting options (color and styling) | ||
| * @returns Formatted message ready for IRC transmission | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * formatMessageForIrc('Hello world', { | ||
| * color: 'red', | ||
| * formatting: ['bold', 'underline'] | ||
| * }) | ||
| * // Returns '\x0304\x02\x1FHello world\x1F\x02' | ||
| * ``` | ||
| */ | ||
| export function formatMessageForIrc( | ||
| text: string, | ||
| options: MessageFormatting, | ||
| ): string { | ||
| const { color, formatting } = options; | ||
|
|
||
| // Apply formatting codes | ||
| let formattedText = applyIrcFormatting(text, formatting); | ||
|
|
||
| // Prepend color code if specified | ||
| const colorCode = color ? getIrcColorCode(color) : ""; | ||
| if (colorCode) { | ||
| formattedText = `${colorCode}${formattedText}`; | ||
| } | ||
|
|
||
| return formattedText; | ||
| } | ||
|
|
||
| /** | ||
| * Gets CSS styles for preview based on formatting options | ||
| * | ||
| * @param options - Formatting options | ||
| * @returns CSS style object | ||
| */ | ||
| export function getPreviewStyles( | ||
| options: MessageFormatting, | ||
| ): React.CSSProperties { | ||
| const { color, formatting } = options; | ||
|
|
||
| return { | ||
| color: color && color !== "inherit" ? color : "inherit", | ||
| fontWeight: formatting.includes("bold") ? "bold" : "normal", | ||
| fontStyle: formatting.includes("italic") ? "italic" : "normal", | ||
| textDecoration: formatting.includes("underline") | ||
| ? "underline" | ||
| : formatting.includes("strikethrough") | ||
| ? "line-through" | ||
| : "none", | ||
| fontFamily: formatting.includes("monospace") ? "monospace" : "inherit", | ||
| }; |
There was a problem hiding this comment.
Import React types before using CSSProperties
This module already has ES imports, so TypeScript treats it as a module. Referencing React.CSSProperties without importing React (or the specific type) causes a compile-time error (Cannot find namespace 'React'). Bring in the type explicitly and reference it directly to keep the build green.
-import { ircColors } from "./ircUtils";
+import type { CSSProperties } from "react";
+import { ircColors } from "./ircUtils";
@@
-export function getPreviewStyles(
- options: MessageFormatting,
-): React.CSSProperties {
+export function getPreviewStyles(
+ options: MessageFormatting,
+): CSSProperties {📝 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.
| import { ircColors } from "./ircUtils"; | |
| export type FormattingType = | |
| | "bold" | |
| | "italic" | |
| | "underline" | |
| | "strikethrough" | |
| | "reverse" | |
| | "monospace"; | |
| export interface MessageFormatting { | |
| color?: string; | |
| formatting: FormattingType[]; | |
| } | |
| /** | |
| * IRC formatting control codes | |
| */ | |
| export const IRC_FORMATTING_CODES = { | |
| bold: "\x02", | |
| italic: "\x1D", | |
| underline: "\x1F", | |
| strikethrough: "\x1E", | |
| reverse: "\x16", | |
| monospace: "\x11", | |
| color: "\x03", | |
| reset: "\x0F", | |
| } as const; | |
| /** | |
| * Gets the IRC color code for a given color name | |
| * | |
| * @param color - The color name or 'inherit' for no color | |
| * @returns IRC color code string (e.g., '\x0301' for black) | |
| * | |
| * @example | |
| * ```typescript | |
| * getIrcColorCode('red') // Returns '\x0304' | |
| * getIrcColorCode('inherit') // Returns '' | |
| * ``` | |
| */ | |
| export function getIrcColorCode(color: string): string { | |
| if (color === "inherit") return ""; | |
| const index = ircColors.indexOf(color); | |
| if (index === -1 || index === 99) return ""; | |
| // Format as two-digit number (e.g., 01, 02, 10) | |
| const formattedIndex = index < 10 ? `0${index}` : `${index}`; | |
| return `${IRC_FORMATTING_CODES.color}${formattedIndex}`; | |
| } | |
| /** | |
| * Applies IRC formatting codes to text | |
| * | |
| * @param text - The text to format | |
| * @param formatting - Array of formatting types to apply | |
| * @returns Text wrapped with IRC formatting codes | |
| * | |
| * @example | |
| * ```typescript | |
| * applyIrcFormatting('Hello', ['bold', 'italic']) | |
| * // Returns '\x02\x1DHello\x1D\x02' | |
| * ``` | |
| */ | |
| export function applyIrcFormatting( | |
| text: string, | |
| formatting: FormattingType[], | |
| ): string { | |
| let formattedText = text; | |
| // Apply each formatting type | |
| for (const format of formatting) { | |
| const code = IRC_FORMATTING_CODES[format]; | |
| if (code) { | |
| formattedText = `${code}${formattedText}${code}`; | |
| } | |
| } | |
| return formattedText; | |
| } | |
| /** | |
| * Formats a message with color and styling for IRC transmission | |
| * | |
| * @param text - The message text | |
| * @param options - Formatting options (color and styling) | |
| * @returns Formatted message ready for IRC transmission | |
| * | |
| * @example | |
| * ```typescript | |
| * formatMessageForIrc('Hello world', { | |
| * color: 'red', | |
| * formatting: ['bold', 'underline'] | |
| * }) | |
| * // Returns '\x0304\x02\x1FHello world\x1F\x02' | |
| * ``` | |
| */ | |
| export function formatMessageForIrc( | |
| text: string, | |
| options: MessageFormatting, | |
| ): string { | |
| const { color, formatting } = options; | |
| // Apply formatting codes | |
| let formattedText = applyIrcFormatting(text, formatting); | |
| // Prepend color code if specified | |
| const colorCode = color ? getIrcColorCode(color) : ""; | |
| if (colorCode) { | |
| formattedText = `${colorCode}${formattedText}`; | |
| } | |
| return formattedText; | |
| } | |
| /** | |
| * Gets CSS styles for preview based on formatting options | |
| * | |
| * @param options - Formatting options | |
| * @returns CSS style object | |
| */ | |
| export function getPreviewStyles( | |
| options: MessageFormatting, | |
| ): React.CSSProperties { | |
| const { color, formatting } = options; | |
| return { | |
| color: color && color !== "inherit" ? color : "inherit", | |
| fontWeight: formatting.includes("bold") ? "bold" : "normal", | |
| fontStyle: formatting.includes("italic") ? "italic" : "normal", | |
| textDecoration: formatting.includes("underline") | |
| ? "underline" | |
| : formatting.includes("strikethrough") | |
| ? "line-through" | |
| : "none", | |
| fontFamily: formatting.includes("monospace") ? "monospace" : "inherit", | |
| }; | |
| // Add at the top of the file, before any React types are used | |
| import type { CSSProperties } from "react"; | |
| import { ircColors } from "./ircUtils"; | |
| // …later in the file… | |
| export function getPreviewStyles( | |
| options: MessageFormatting, | |
| ): CSSProperties { | |
| // function body remains the same | |
| const { color, formatting } = options; | |
| return { | |
| color: color && color !== "inherit" ? color : "inherit", | |
| fontWeight: formatting.includes("bold") ? "bold" : "normal", | |
| fontStyle: formatting.includes("italic") ? "italic" : "normal", | |
| textDecoration: formatting.includes("underline") | |
| ? "underline" | |
| : formatting.includes("strikethrough") | |
| ? "line-through" | |
| : "none", | |
| fontFamily: formatting.includes("monospace") ? "monospace" : "inherit", | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/lib/messageFormatter.ts around lines 6 to 143, the code references
React.CSSProperties without importing React types which causes a TypeScript
compile error; add a type-only import at the top (e.g., import type {
CSSProperties } from 'react') and then change the function signature to use
CSSProperties instead of React.CSSProperties so the file compiles cleanly.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/ui/ColorPicker.tsx (1)
27-31: Avoid random keys; causes remounting and perf issuesRandom keys force React to recreate elements every render, losing state and hurting performance. Use a stable key.
- const keyToken = `${color}-${index}-${Math.floor(Math.random() * 9999)}`; + const keyToken = `${color}-${index}`; return ( <button key={keyToken}src/components/layout/ChatArea.tsx (1)
332-351: Preserve formatting in locally echoed private messagesFor DMs we enqueue
messageText, but the server receives the formatted payload (formattedText). Because the store copy drops the control codes, our own DM immediately renders without color or styling. Please push the same formatted string you just sent so the local echo matches what the remote side sees.Apply this diff:
- const outgoingMessage = { + const outgoingMessage = { id: uuidv4(), - content: messageText, + content: formattedText,src/lib/ircClient.ts (4)
295-306: Guard against blank/partial lines to prevent parser crashesDirectly indexing
l[i][0]can throw on empty tokens. Add early-continue and token checks.Apply this diff:
- let mtags: Record<string, string> | undefined; - let source: string; - const parv = []; + let mtags: Record<string, string> | undefined; + let source: string; + const parv: string[] = []; let i = 0; let l: string[]; line = line.trim(); + if (!line) continue; l = line.split(" ") ?? line; - if (l[i][0] === "@") { + if (l[i]?.startsWith("@")) { mtags = parseMessageTags(l[i]); i++; } // Determine the source. if none, spoof as host server - if (l[i][0] !== ":") { + if (!l[i]?.startsWith(":")) {Also applies to: 311-318
335-339: Fix RPL 001 nickname parsing
nicknameshould beparv[0], not the joined rest of the welcome message.- const nickname = parv.join(" "); + const nickname = parv[0]; this.triggerEvent("ready", { serverId, serverName, nickname });
360-364: Normalize QUIT/PART reasons (strip leading colon)Current reasons may include a leading “:”. Normalize for consistency with KICK/PRIVMSG handling.
- const reason = parv.join(" "); + const reason = parv.join(" ").replace(/^:/, ""); this.triggerEvent("QUIT", { serverId, username, reason });- const reason = parv.join(" ").trim(); + const reason = parv.join(" ").trim().replace(/^:/, ""); this.triggerEvent("PART", {Also applies to: 371-378
168-171: Redact METADATA values in logs (PII risk)Logging full METADATA commands/values can leak sensitive data. Redact payloads.
- if (command.startsWith("METADATA")) { - console.log(`[IRC] Sending: ${command}`); - } + if (command.startsWith("METADATA")) { + const redacted = command.replace(/:(.*)$/, ":<redacted>"); + console.log(`[IRC] Sending: ${redacted}`); + }- console.log(`[IRC] Sending metadata SET command: ${command}`); + console.log( + `[IRC] Sending metadata SET command: ${command.replace(/:(.*)$/, ":<redacted>")}`, + );- console.log( - `[IRC] Received METADATA: target=${target}, key=${key}, visibility=${visibility}, value=${value}`, - ); + console.log( + `[IRC] Received METADATA: target=${target}, key=${key}, visibility=${visibility}, value=<redacted>`, + );Also applies to: 244-246, 524-526
🧹 Nitpick comments (24)
src/components/message/MessageActions.tsx (1)
5-9: Tighten callback type to HTMLButtonElement.onReactClick currently accepts Element, which loses useful typing (e.g., getBoundingClientRect). Use HTMLButtonElement for safer call sites.
interface MessageActionsProps { message: MessageType; onReplyClick: () => void; - onReactClick: (buttonElement: Element) => void; + onReactClick: (buttonElement: HTMLButtonElement) => void; }src/components/message/SystemMessage.tsx (1)
15-20: Time format: make 24‑hour explicit (or derive from user locale).Current Intl defaults to 12h in en‑US and may include AM/PM. If you intend HH:MM, set hour12: false (or use navigator.language).
- const formatTime = (date: Date) => { - return new Intl.DateTimeFormat("en-US", { + const formatTime = (date: Date) => { + return new Intl.DateTimeFormat("en-US", { hour: "2-digit", minute: "2-digit", + hour12: false, }).format(date); - }; + };src/components/message/DateSeparator.tsx (1)
12-18: Locale: consider using user locale for date.If you want system locale, swap "en-US" for navigator.language (with a fallback) to match user expectations.
- return new Intl.DateTimeFormat("en-US", { + return new Intl.DateTimeFormat(typeof navigator !== "undefined" ? navigator.language : "en-US", {src/components/message/MessageReply.tsx (3)
27-29: Clickable span should be a button for a11y and keyboard.Spans aren’t focusable by default. Use a with type, aria‑label, and keyboard semantics.
- <strong> - <span className="cursor-pointer" onClick={onUsernameClick}> - {replyUsername} - </span> - : - </strong>{" "} + <strong> + <button + type="button" + className="cursor-pointer underline hover:no-underline" + aria-label={`View profile of ${replyUsername}`} + onClick={onUsernameClick} + > + {replyUsername} + </button> + : + </strong>{" "}
21-35: Optional: clamp reply preview to one line.If this is meant to be a single‑line preview, add truncation utilities.
- <div - className={`bg-${theme}-dark-200 rounded text-sm text-${theme}-text-muted mb-2 pl-1 pr-2`} - > + <div + className={`… mb-2 pl-1 pr-2 whitespace-nowrap overflow-hidden text-ellipsis`} + >
25-31: i18n: hard‑coded “Replying to”.Extract to a message catalog for localization.
package.json (1)
33-33: Consider lazy-loading emoji-picker-react client-side only
emoji-picker-react v4.13.3 injects its styles at runtime (no CSS import needed) and supports React 18 but isn’t SSR-safe. Dynamically import it in the Reaction modal (e.g. React.lazy on interaction or next/dynamic with { ssr: false }) to keep your initial bundle small.src/components/ui/ReactionModal.tsx (1)
29-33: Add basic a11y and SSR safety for the portalAdd role/aria, Escape-to-close, and guard document for non-browser envs.
- return createPortal( - <div - className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50" - onClick={handleBackdropClick} - > + const portalTarget = typeof document !== "undefined" ? document.body : null; + if (!portalTarget) return null; + + const onKeyDown = (e: React.KeyboardEvent) => { + if (e.key === "Escape") onClose(); + }; + + return createPortal( + <div + role="dialog" + aria-modal="true" + aria-label="Emoji picker" + className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50" + onClick={handleBackdropClick} + onKeyDown={onKeyDown} + tabIndex={-1} + > <div className="bg-discord-dark-400 rounded-lg shadow-lg border border-discord-dark-300 max-w-sm w-full mx-4 max-h-[90vh] overflow-auto"> <div className="p-2"> <EmojiPicker onEmojiClick={handleEmojiSelect} theme={Theme.DARK} width="100%" height={350} searchPlaceholder="Search emojis..." previewConfig={{ showPreview: false, }} skinTonesDisabled={false} lazyLoadEmojis={true} /> </div> <div className="p-2 border-t border-discord-dark-300"> <button onClick={onClose} className="text-sm text-discord-text-muted hover:text-white w-full text-center py-1" > Cancel </button> </div> </div> - </div>, - document.body, + </div>, + portalTarget, );Also applies to: 35-41, 49-55, 58-60
src/components/ui/ColorPicker.tsx (1)
93-103: Optional: include the “reverse” formatting toggle to match the type unionFormattingType includes "reverse" but UI lacks a control. Add if supported.
<button className={`w-6 h-6 rounded border-2 flex items-center justify-center ${ selectedFormatting.includes("strikethrough") ? "bg-discord-dark-200 text-white border-purple-500" : "bg-discord-dark-400 text-discord-text-muted hover:bg-discord-dark-300 border-gray-700" }`} onClick={() => toggleFormatting("strikethrough")} > <span className="line-through">S</span> </button> + <button + className={`w-6 h-6 rounded border-2 flex items-center justify-center ${ + selectedFormatting.includes("reverse") + ? "bg-discord-dark-200 text-white border-purple-500" + : "bg-discord-dark-400 text-discord-text-muted hover:bg-discord-dark-300 border-gray-700" + }`} + onClick={() => toggleFormatting("reverse")} + aria-label="Reverse" + title="Reverse" + > + <span className="text-xs">R</span> + </button>src/types/index.ts (1)
117-150: Polish event typings: make mtags optional, consider timestamp shape.
- Prefer
mtags?: Record<string, string>so the property itself is optional rather than present-but-undefined.- If events cross the wire,
timestamp: Datecan be awkward. ConsiderDate | number(or normalize at parse boundaries) to avoid runtime mismatches.Suggested diff:
-export interface EventWithTags extends BaseIRCEvent { - mtags: Record<string, string> | undefined; -} +export interface EventWithTags extends BaseIRCEvent { + mtags?: Record<string, string>; +} -export interface BaseMessageEvent extends EventWithTags { +export interface BaseMessageEvent extends EventWithTags { sender: string; message: string; - timestamp: Date; + timestamp: Date | number; }Also consider constraining
visibilityto a union type shared withUser/Server/Channel.metadatafor consistency. I can follow up with a small PR to DRY the metadata/visibility types.src/components/message/MessageAvatar.tsx (2)
33-36: Add keyboard/accessibility affordances when clickable.Expose button semantics and keyboard activation.
- <div - className={`mr-4 ${isClickable ? "cursor-pointer" : ""}`} - onClick={onClick} - > + <div + className={`mr-4 ${isClickable ? "cursor-pointer" : ""}`} + onClick={onClick} + role={isClickable ? "button" : undefined} + tabIndex={isClickable ? 0 : undefined} + onKeyDown={(e) => { + if (!isClickable || !onClick) return; + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + onClick((e as unknown) as React.MouseEvent); + } + }} + aria-label={isClickable ? `Open profile for ${username}` : undefined} + >
22-22: Don’t derive username from userId; pass it or render a safer fallback.
userId.split("-")[0]is brittle. Prefer an explicitusername/displayNameprop; fall back to first char of that, then to?.Example:
-interface MessageAvatarProps { - userId: string; +interface MessageAvatarProps { + userId: string; + username?: string; ... } ... -export const MessageAvatar: React.FC<MessageAvatarProps> = ({ - userId, +export const MessageAvatar: React.FC<MessageAvatarProps> = ({ + userId, + username: usernameProp, ... }) => { - const username = userId.split("-")[0]; + const username = usernameProp ?? userId ?? ""; ... - username.charAt(0).toUpperCase() + (username.charAt(0) || "?").toUpperCase()Also applies to: 55-56
src/components/ui/LinkWrapper.tsx (3)
13-15: Tighten regex and update comment.
- The comment mentions only HTTP(S) but the pattern also matches IRC(S).
- Current pattern may include trailing punctuation. Use a lookahead to stop before punctuation/terminators.
- // Regular expression to detect HTTP and HTTPS links - const urlRegex = /\b(?:https?|irc|ircs):\/\/[^\s<>"']+/gi; + // Detect http, https, irc, ircs links; avoid trailing punctuation + const urlRegex = /\b(?:https?|irc|ircs):\/\/[^\s<>"']+?(?=[\s<>"']|[.,!?;:)]|$)/gi;
31-49: Minor hardening of anchors.
- Consider adding
nofollow ugctorelto signal user-generated content.- Optional: shorten displayed URL text while keeping full href to avoid overflow.
- rel="noopener noreferrer" + rel="noopener noreferrer nofollow ugc"
64-71: Skip linkifying inside code/pre or contentEditable.Pre/code blocks often should remain untouched; also avoid interfering with editable regions.
- // Skip already-linkified anchors to avoid nested <a> - if ((child as React.ReactElement).type === "a") { + // Skip anchors, code/pre, and editable content + const el = child as React.ReactElement<any>; + const tag = typeof el.type === "string" ? el.type : ""; + if (tag === "a" || tag === "code" || tag === "pre" || el.props?.contentEditable) { return child; }src/components/message/MessageReactions.tsx (1)
31-43: Avoid duplicate user listings and ensure stable orderingIf a user can’t react more than once with the same emoji, store unique users to prevent duplication in the tooltip, and render pills in a stable, meaningful order (e.g., by count desc, then emoji).
- acc[reaction.emoji].count++; - acc[reaction.emoji].users.push(reaction.userId); + const data = acc[reaction.emoji]; + data.count++; + if (!data.users.includes(reaction.userId)) data.users.push(reaction.userId);And before rendering:
- {Object.entries(groupedReactions).map(([emoji, data]) => { + {Object.entries(groupedReactions) + .sort(([, a], [, b]) => b.count - a.count || 0) + .map(([emoji, data]) => {Also applies to: 57-64
tests/lib/messageFormatter.test.ts (1)
19-37: Solid coverage; consider a couple of edge cases for resilienceLooks good. Optional adds:
- Assert that unknown colors (index 99 sentinel) and mixed-case hex are handled as empty codes.
- Add a test for reverse combined with others (order-sensitive) and that reset isn’t implicitly injected.
- Negative test for invalid sequences in applyIrcFormatting (e.g., duplicate entries) to ensure deterministic wrapping.
Also applies to: 70-75, 272-283
src/lib/ircUrlParser.ts (2)
50-63: Decode, normalize, then consider de-duping and trimmingMinor: trim whitespace around channel tokens and de-duplicate to avoid accidental repeats.
- const channels = rawChannelStr - .split(",") - .filter(Boolean) - .map((c) => decodeURIComponent(c)) - .map((c) => normalizeChannelName(c)); + const seen = new Set<string>(); + const channels = rawChannelStr + .split(",") + .map((c) => c.trim()) + .filter(Boolean) + .map((c) => normalizeChannelName(decodeURIComponent(c))) + .filter((c) => (seen.has(c) ? false : (seen.add(c), true)));
100-106: Validation could also guard against invalid portsOptional: when a port is present, ensure it parses to 1–65535.
- return ( - ["irc:", "ircs:"].includes(urlObj.protocol) && urlObj.hostname !== "" - ); + const hasProto = ["irc:", "ircs:"].includes(urlObj.protocol); + const hasHost = urlObj.hostname !== ""; + const port = urlObj.port ? Number.parseInt(urlObj.port, 10) : undefined; + const portOk = port === undefined || (port >= 1 && port <= 65535); + return hasProto && hasHost && portOk;src/lib/ircClient.ts (5)
154-162: Safer disconnect: check readyState before QUIT and always cleanupdisconnect(serverId: string): void { const socket = this.sockets.get(serverId); if (socket) { - socket.send("QUIT :ObsidianIRC - Bringing IRC into the future"); - socket.close(); - this.sockets.delete(serverId); + try { + if (socket.readyState === WebSocket.OPEN) { + socket.send("QUIT :ObsidianIRC - Bringing IRC into the future"); + } + } finally { + socket.close(); + this.sockets.delete(serverId); + } } this.servers.delete(serverId); }
643-655: Deduplicate requested capabilities
message-tagsappears twice; remove the duplicate.const ourCaps = [ "multi-prefix", "message-tags", "server-time", "echo-message", - "message-tags", "userhost-in-names", "draft/chathistory", "draft/extended-isupport", "sasl", "draft/metadata-2", ];
143-151: Avoid O(n) socket→serverId lookup on every messageYou scan all sockets to find
serverIdper frame. Store it instead:
- Option A: move
socket.onmessageinsideonopenand close overserver.id.- Option B: keep a
socketToServerId: WeakMap<WebSocket, string>.This reduces per-message overhead and removes equality pitfalls across WebSocket instances.
Would you like a small diff applying Option A?
41-45: TAGMSG target can be a user or a channel; avoidchannelNamenamingUse a neutral
targetfield in the event payload/type to prevent consumer assumptions. This is a naming-only change but avoids downstream bugs.If you want, I can generate the minimal type + callsite rename diff across this file.
Also applies to: 422-430
497-505: Unify CAP event naming and drop non-standard ISUPPORT request
- Use a single CAP acknowledgment event name (either
"CAP ACK"or"CAP_ACKNOWLEDGED") and remove the other.- Remove
this.sendRaw(serverId, "ISUPPORT"); RPL_005 (ISUPPORT) is advertised by servers only.Also applies to lines 671–673.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
package.json(1 hunks)src/components/layout/ChatArea.tsx(10 hunks)src/components/message/ActionMessage.tsx(1 hunks)src/components/message/DateSeparator.tsx(1 hunks)src/components/message/MessageActions.tsx(1 hunks)src/components/message/MessageAvatar.tsx(1 hunks)src/components/message/MessageHeader.tsx(1 hunks)src/components/message/MessageItem.tsx(1 hunks)src/components/message/MessageReactions.tsx(1 hunks)src/components/message/MessageReply.tsx(1 hunks)src/components/message/SystemMessage.tsx(1 hunks)src/components/message/index.ts(1 hunks)src/components/ui/ColorPicker.tsx(1 hunks)src/components/ui/LinkWrapper.tsx(1 hunks)src/components/ui/ReactionModal.tsx(1 hunks)src/lib/ircClient.ts(1 hunks)src/lib/ircUrlParser.ts(1 hunks)src/lib/messageFormatter.ts(1 hunks)src/types/index.ts(2 hunks)tests/lib/ircUrlParser.test.ts(1 hunks)tests/lib/messageFormatter.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
src/components/message/DateSeparator.tsx (1)
src/components/message/index.ts (1)
DateSeparator(2-2)
src/components/message/MessageActions.tsx (2)
src/types/index.ts (1)
MessageType(80-80)src/components/message/index.ts (1)
MessageActions(3-3)
src/components/ui/ColorPicker.tsx (1)
src/lib/messageFormatter.ts (1)
FormattingType(8-14)
src/components/message/MessageReactions.tsx (2)
src/types/index.ts (1)
MessageType(80-80)src/components/message/index.ts (1)
MessageReactions(6-6)
src/components/message/MessageItem.tsx (11)
src/types/index.ts (2)
MessageType(80-80)User(1-10)src/lib/ircUtils.tsx (1)
mircToHtml(167-279)src/components/message/SystemMessage.tsx (1)
SystemMessage(11-37)src/components/message/DateSeparator.tsx (1)
DateSeparator(8-27)src/components/message/ActionMessage.tsx (1)
ActionMessage(17-84)src/components/message/MessageAvatar.tsx (1)
MessageAvatar(13-72)src/components/message/MessageHeader.tsx (1)
MessageHeader(14-52)src/components/message/MessageReply.tsx (1)
MessageReply(13-37)src/components/ui/LinkWrapper.tsx (1)
EnhancedLinkWrapper(9-85)src/components/message/MessageReactions.tsx (1)
MessageReactions(16-82)src/components/message/MessageActions.tsx (1)
MessageActions(11-34)
src/lib/messageFormatter.ts (1)
src/lib/ircUtils.tsx (1)
ircColors(64-165)
src/components/layout/ChatArea.tsx (2)
src/lib/messageFormatter.ts (4)
FormattingType(8-14)isValidFormattingType(152-164)formatMessageForIrc(104-120)getPreviewStyles(128-144)src/lib/ircUrlParser.ts (1)
parseIrcUrl(35-76)
src/lib/ircClient.ts (1)
src/types/index.ts (7)
BaseIRCEvent(118-120)EventWithTags(123-125)BaseUserActionEvent(147-149)BaseMessageEvent(140-144)User(1-10)MetadataValueEvent(134-137)BaseMetadataEvent(128-131)
src/components/message/SystemMessage.tsx (3)
src/types/index.ts (1)
MessageType(80-80)src/lib/ircUtils.tsx (1)
mircToHtml(167-279)src/components/ui/LinkWrapper.tsx (1)
EnhancedLinkWrapper(9-85)
src/components/message/MessageHeader.tsx (2)
src/components/message/index.ts (1)
MessageHeader(5-5)src/lib/ircUtils.tsx (1)
getColorStyle(282-302)
src/components/message/MessageReply.tsx (3)
src/types/index.ts (1)
MessageType(80-80)src/components/ui/LinkWrapper.tsx (1)
EnhancedLinkWrapper(9-85)src/lib/ircUtils.tsx (1)
mircToHtml(167-279)
tests/lib/ircUrlParser.test.ts (1)
src/lib/ircUrlParser.ts (5)
parseIrcUrl(35-76)normalizeChannelName(85-92)isValidIrcUrl(100-109)ParsedIrcUrl(6-13)constructIrcUrl(117-145)
src/components/message/ActionMessage.tsx (2)
src/types/index.ts (2)
MessageType(80-80)User(1-10)src/components/message/MessageAvatar.tsx (1)
MessageAvatar(13-72)
src/components/message/MessageAvatar.tsx (1)
src/components/message/index.ts (1)
MessageAvatar(4-4)
tests/lib/messageFormatter.test.ts (1)
src/lib/messageFormatter.ts (8)
getIrcColorCode(47-56)applyIrcFormatting(71-86)MessageFormatting(16-19)formatMessageForIrc(104-120)getPreviewStyles(128-144)FormattingType(8-14)isValidFormattingType(152-164)IRC_FORMATTING_CODES(24-33)
🔇 Additional comments (8)
src/components/message/MessageReply.tsx (1)
19-19: Use a dedicated username field or enforce userId format
SplittinguserIdon'-'to derive a username is fragile if hyphens are allowed in usernames; either introduce a separateusername/displayNamefield or ensureuserIdis strictly formatted (e.g.username-<uuid>) at creation.src/components/message/SystemMessage.tsx (1)
22-33: No timestamp unit issues –timestampis aDateMessageType.timestamp is declared asDateinsrc/types/index.ts, sonew Date(message.timestamp)handles it correctly and no ms-vs-s normalization is needed.Likely an incorrect or invalid review comment.
src/components/message/MessageActions.tsx (1)
17-31: Accessibility enhancements verified; resolving
Parent wrapper already includesgroupandrelative. Both buttons now havetype="button", meaningfularia-labelandtitleattributes.tests/lib/ircUrlParser.test.ts (2)
10-141: Parse cases look solid (defaults, channels, params, sanitization)Good coverage of default ports, channel sources, URL encoding, and params.
188-229: Construction and round‑trip tests are clear and robustRound‑trip assertions improve confidence in parser/constructor symmetry.
Also applies to: 231-287
src/types/index.ts (1)
79-81: Back-compat alias looks good.Type alias preserves external API while allowing internal refactors. No concerns.
src/components/message/index.ts (1)
1-8: Barrel exports LGTM.Optional: if
MessageItemexists in this folder (used elsewhere in the PR), consider exporting it here for a single import surface.src/lib/ircUrlParser.ts (1)
44-49: Incorrect default ports for IRC/IRCSStandard defaults are 6667 for irc and 6697 for ircs. Using 8000/443 will break many links.
- const port = urlObj.port - ? Number.parseInt(urlObj.port, 10) - : scheme === "ircs" - ? 443 - : 8000; + const port = urlObj.port + ? Number.parseInt(urlObj.port, 10) + : scheme === "ircs" + ? 6697 + : 6667;⛔ Skipped due to learnings
Learnt from: ValwareIRC PR: ObsidianIRC/ObsidianIRC#62 File: src/components/layout/ChatArea.tsx:0-0 Timestamp: 2025-09-28T19:37:34.200Z Learning: ObsidianIRC uses websockets for IRC connections, not traditional IRC protocol. Default ports are 443 for secure connections (ircs://) and 8000 for plain connections (irc://), not the standard IRC ports 6667/6697.
| avatarUrl={messageUser?.metadata?.avatar?.value} | ||
| userStatus={messageUser?.metadata?.status?.value} | ||
| theme="discord" | ||
| showHeader={true} | ||
| onClick={(e) => { | ||
| onUsernameContextMenu( | ||
| e, | ||
| username, | ||
| message.serverId, | ||
| e.currentTarget, | ||
| ); | ||
| }} | ||
| isClickable={true} | ||
| /> |
There was a problem hiding this comment.
Fragile ACTION parsing; can cut content and drop last char
Hard‑coding substring(7, length - 1) assumes CTCP format and always strips the last char. Parse robustly and add spacing. Also fall back to top‑level avatar if metadata is absent.
- <MessageAvatar
+ <MessageAvatar
userId={message.userId}
- avatarUrl={messageUser?.metadata?.avatar?.value}
+ avatarUrl={messageUser?.metadata?.avatar?.value ?? messageUser?.avatar}
userStatus={messageUser?.metadata?.status?.value}
theme="discord"
showHeader={true}
onClick={(e) => {
onUsernameContextMenu(
e,
username,
message.serverId,
e.currentTarget,
);
}}
isClickable={true}
/>
<div className="flex-1 text-white">
<div className="flex items-center">
<span className="ml-2 text-xs text-discord-text-muted">
{formatTime(new Date(message.timestamp))}
</span>
</div>
- <span className="italic text-white">
- {message.userId === "system"
- ? "System"
- : (displayName || username) +
- (displayName ? ` (${username})` : "") +
- message.content.substring(7, message.content.length - 1)}
- </span>
+ {(() => {
+ const extractAction = (input: string): string => {
+ const m = input.match(/^\u0001ACTION\s([\s\S]*?)\u0001$/);
+ if (m) return m[1];
+ if (input.startsWith("/me ")) return input.slice(4);
+ return input;
+ };
+ const actor =
+ message.userId === "system"
+ ? "System"
+ : (displayName || username) + (displayName ? ` (${username})` : "");
+ const actionText = extractAction(message.content);
+ return <span className="italic text-white">{`${actor} ${actionText}`}</span>;
+ })()}
</div>Also applies to: 73-79
🤖 Prompt for AI Agents
In src/components/message/ActionMessage.tsx around lines 53-66 (and likewise
update lines 73-79), the code currently uses a brittle substring(7, length - 1)
to extract CTCP ACTION text and always strips the last character and avatar only
from metadata; update parsing to detect CTCP robustly by checking for the CTCP
delimiter (startsWith "\x01ACTION " and endsWith "\x01"), extract the inner
content via a safe slice between those delimiters (or fall back to the entire
message text if it does not match CTCP), trim the result and ensure a space is
preserved around the action text when rendering; also when resolving avatarUrl
fall back to top-level messageUser.avatar or a sensible default if
metadata?.avatar?.value is absent so missing metadata does not drop the avatar.
| <div className={`flex items-center text-xs text-${theme}-text-muted mb-2`}> | ||
| <div className={`flex-grow border-t border-${theme}-dark-400`} /> | ||
| <div className="px-2">{formatDate(date)}</div> | ||
| <div className={`flex-grow border-t border-${theme}-dark-400`} /> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
Tailwind JIT cannot see dynamic class names; these may purge in prod.
text-${theme}-text-muted and border-${theme}-dark-400 likely won’t be generated unless safelisted. Prefer a static map or CSS variables.
Option A (map classes):
-export const DateSeparator: React.FC<DateSeparatorProps> = ({
- date,
- theme,
-}) => {
+const themeStyles = {
+ discord: { text: "text-discord-text-muted", border: "border-discord-dark-400" },
+ // add other themes here
+} as const;
+
+export const DateSeparator: React.FC<DateSeparatorProps> = ({ date, theme }) => {
+ const styles = themeStyles[theme] ?? themeStyles.discord;
const formatDate = (date: Date) => {
return new Intl.DateTimeFormat("en-US", {
month: "long",
day: "numeric",
year: "numeric",
}).format(date);
};
return (
- <div className={`flex items-center text-xs text-${theme}-text-muted mb-2`}>
- <div className={`flex-grow border-t border-${theme}-dark-400`} />
+ <div className={`flex items-center text-xs ${styles.text} mb-2`}>
+ <div className={`flex-grow border-t ${styles.border}`} />
<div className="px-2">{formatDate(date)}</div>
- <div className={`flex-grow border-t border-${theme}-dark-400`} />
+ <div className={`flex-grow border-t ${styles.border}`} />
</div>
);
};Option B (ensure safelist):
Add concrete class names to tailwind.config.js safelist.
🏁 Script executed:
#!/bin/bash
# Verify safelist exists and includes the needed theme classes
fd tailwind.config -a | xargs -I{} rg -n 'safelist' {}Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate Tailwind config files (JS, CJS, TS)
fd --max-depth 3 --type f 'tailwind.config.*'
# Check for a `safelist` key in those config files
fd --max-depth 3 --type f 'tailwind.config.*' | xargs -r rg -n 'safelist'Length of output: 18
🏁 Script executed:
#!/bin/bash
# Print first 200 lines of Tailwind config to check for safelist
sed -n '1,200p' tailwind.config.jsLength of output: 4778
Prevent Tailwind JIT from purging dynamic theme classes
- Tailwind JIT won’t generate
text-${theme}-text-mutedorborder-${theme}-dark-400at build time—these will be purged. Choose one:- Static map: use a
themeStylesobject and inject${styles.text}/${styles.border}(see diff). - Safelist: add the concrete class names to the
safelistarray intailwind.config.js.
- Static map: use a
export const DateSeparator: React.FC<DateSeparatorProps> = ({ date, theme }) => {
+ const themeStyles = {
+ discord: { text: "text-discord-text-muted", border: "border-discord-dark-400" },
+ // add other themes here
+ } as const;
+ const styles = themeStyles[theme] ?? themeStyles.discord;
return (
- <div className={`flex items-center text-xs text-${theme}-text-muted mb-2`}>
+ <div className={`flex items-center text-xs ${styles.text} mb-2`}>
<div className={`flex-grow border-t ${styles.border}`} />
<div className="px-2">{formatDate(date)}</div>
<div className={`flex-grow border-t ${styles.border}`} />
</div>
);
};📝 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.
| <div className={`flex items-center text-xs text-${theme}-text-muted mb-2`}> | |
| <div className={`flex-grow border-t border-${theme}-dark-400`} /> | |
| <div className="px-2">{formatDate(date)}</div> | |
| <div className={`flex-grow border-t border-${theme}-dark-400`} /> | |
| </div> | |
| export const DateSeparator: React.FC<DateSeparatorProps> = ({ date, theme }) => { | |
| const themeStyles = { | |
| discord: { | |
| text: "text-discord-text-muted", | |
| border: "border-discord-dark-400", | |
| }, | |
| // add other themes here, e.g.: | |
| // light: { text: "text-light-text-muted", border: "border-light-dark-400" }, | |
| } as const; | |
| const styles = themeStyles[theme] ?? themeStyles.discord; | |
| return ( | |
| <div className={`flex items-center text-xs ${styles.text} mb-2`}> | |
| <div className={`flex-grow border-t ${styles.border}`} /> | |
| <div className="px-2">{formatDate(date)}</div> | |
| <div className={`flex-grow border-t ${styles.border}`} /> | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/components/message/DateSeparator.tsx around lines 21-25, dynamic Tailwind
class names like `text-${theme}-text-muted` and `border-${theme}-dark-400` are
being generated at runtime and will be purged by JIT; replace them with static
class names by either (A) creating a themeStyles mapping (e.g. const themeStyles
= { light: { text: 'text-light-text-muted', border: 'border-light-dark-400' },
dark: { ... } }) and use themeStyles[theme].text and themeStyles[theme].border
in the JSX, making sure the map is defined/imported in the file, or (B) add all
concrete class names (e.g. text-light-text-muted, text-dark-text-muted,
border-light-dark-400, border-dark-dark-400) to the safelist array in
tailwind.config.js so Tailwind generates them; pick one approach and update the
JSX accordingly.
| onClick={onClick} | ||
| > | ||
| <div | ||
| className={`w-8 h-8 rounded-full bg-${theme}-dark-400 flex items-center justify-center text-white relative`} |
There was a problem hiding this comment.
Dynamic Tailwind class likely purged; map theme to concrete classes.
bg-${theme}-dark-400 won’t be seen by Tailwind’s scanner. Resolve to a fixed class string.
Example:
- className={`w-8 h-8 rounded-full bg-${theme}-dark-400 flex items-center justify-center text-white relative`}
+ className={`w-8 h-8 rounded-full ${bgClass} flex items-center justify-center text-white relative`}Add inside component:
const themeBg: Record<string, string> = {
discord: "bg-discord-dark-400",
light: "bg-gray-400",
dark: "bg-gray-700",
};
const bgClass = themeBg[theme] ?? "bg-gray-500";🤖 Prompt for AI Agents
In src/components/message/MessageAvatar.tsx around line 38, the Tailwind class
bg-${theme}-dark-400 is dynamic and will be purged; replace it by mapping theme
values to concrete class strings (e.g. a Record<string,string> mapping for known
themes with a fallback) and use the resolved bgClass in the className template
instead of the dynamic expression; ensure the mapping covers expected theme keys
and provides a sensible default.
| onError={(e) => { | ||
| // Fallback to initial if image fails to load | ||
| e.currentTarget.style.display = "none"; | ||
| const parent = e.currentTarget.parentElement; | ||
| if (parent) { | ||
| parent.textContent = username.charAt(0).toUpperCase(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Avoid DOM mutation in onError; render fallback via state.
Directly setting style.display and parent.textContent breaks React’s DOM contract and may clobber children (e.g., status badge). Use local state to render initials on error.
Minimal change:
- onError={(e) => {
- // Fallback to initial if image fails to load
- e.currentTarget.style.display = "none";
- const parent = e.currentTarget.parentElement;
- if (parent) {
- parent.textContent = username.charAt(0).toUpperCase();
- }
- }}
+ onError={() => setImgFailed(true)}Additions required outside this hunk:
+import { useState } from "react";
export const MessageAvatar: React.FC<MessageAvatarProps> = ({ ... }) => {
+ const [imgFailed, setImgFailed] = useState(false);
const username = userId.split("-")[0];
...
- {avatarUrl ? (
+ {avatarUrl && !imgFailed ? (
<img ... />
) : (
username.charAt(0).toUpperCase()
)}📝 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.
| onError={(e) => { | |
| // Fallback to initial if image fails to load | |
| e.currentTarget.style.display = "none"; | |
| const parent = e.currentTarget.parentElement; | |
| if (parent) { | |
| parent.textContent = username.charAt(0).toUpperCase(); | |
| } | |
| }} | |
| // src/components/message/MessageAvatar.tsx | |
| import { useState } from "react"; | |
| export const MessageAvatar: React.FC<MessageAvatarProps> = ({ userId, avatarUrl, ...props }) => { | |
| /* Track whether the image has failed to load */ | |
| const [imgFailed, setImgFailed] = useState(false); | |
| const username = userId.split("-")[0]; | |
| return ( | |
| <div {...props}> | |
| {avatarUrl && !imgFailed ? ( | |
| <img | |
| src={avatarUrl} | |
| alt={`${username}'s avatar`} | |
| onError={() => setImgFailed(true)} | |
| /> | |
| ) : ( | |
| /* Fallback to the user's initial */ | |
| username.charAt(0).toUpperCase() | |
| )} | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/components/message/MessageAvatar.tsx around lines 45 to 52, the onError
handler mutates the DOM (setting style.display and parent.textContent); change
this to use React state: add a local boolean state like imageErrored (using
useState) near the component top, update onError to setImageErrored(true) (not
touch DOM), and change the JSX to conditionally render the <img> when
!imageErrored and render the username initial when imageErrored (ensuring any
status badge or children remain rendered as separate elements so they aren't
clobbered).
| <span className={`ml-2 text-xs text-${theme}-text-muted`}> | ||
| {formatTime(timestamp)} | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid dynamic Tailwind classes; they won’t be generated in JIT builds
Using text-${theme}-text-muted will purge the class at build time. Map theme to concrete classes or use CSS variables.
Apply one of the following:
Option A: Map theme to class
- <span className={`ml-2 text-xs text-${theme}-text-muted`}>
+ {(() => {
+ const themeTextMuted: Record<string, string> = {
+ discord: "text-discord-text-muted",
+ // add more themes here
+ };
+ const muted = themeTextMuted[theme] ?? "text-discord-text-muted";
+ return <span className={`ml-2 text-xs ${muted}`}>{formatTime(timestamp)}</span>;
+ })()}
- {formatTime(timestamp)}
- </span>Option B: Use a CSS variable
- <span className={`ml-2 text-xs text-${theme}-text-muted`}>
+ <span className="ml-2 text-xs" style={{ color: "var(--text-muted)" }}>
{formatTime(timestamp)}
</span>📝 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.
| <span className={`ml-2 text-xs text-${theme}-text-muted`}> | |
| {formatTime(timestamp)} | |
| </span> | |
| {(() => { | |
| const themeTextMuted: Record<string, string> = { | |
| discord: "text-discord-text-muted", | |
| // add more themes here | |
| }; | |
| const muted = themeTextMuted[theme] ?? "text-discord-text-muted"; | |
| return <span className={`ml-2 text-xs ${muted}`}>{formatTime(timestamp)}</span>; | |
| })()} |
| <span className={`ml-2 text-xs text-${theme}-text-muted`}> | |
| {formatTime(timestamp)} | |
| </span> | |
| <span className="ml-2 text-xs" style={{ color: "var(--text-muted)" }}> | |
| {formatTime(timestamp)} | |
| </span> |
🤖 Prompt for AI Agents
In src/components/message/MessageHeader.tsx around lines 47-49 the component
uses a dynamic Tailwind class text-${theme}-text-muted which will be purged by
JIT; replace the dynamic class with a deterministic solution: either (A) map the
theme value to explicit Tailwind classes (e.g. const themeClass = theme ===
'dark' ? 'text-dark-text-muted' : 'text-light-text-muted' and use that variable
in the className) or (B) switch to a CSS variable for the color (define
--message-text-muted in your theme classes and use a static Tailwind class that
reads the variable). Implement one of these options so the generated Tailwind
class is present at build time.
|
|
||
| return ( | ||
| <div | ||
| className={`bg-${theme}-dark-200 rounded text-sm text-${theme}-text-muted mb-2 pl-1 pr-2`} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Tailwind dynamic classes risk purge; mirror DateSeparator fix.
bg-${theme}-dark-200 and text-${theme}-text-muted should be mapped or safelisted, otherwise styles can disappear in prod.
- <div
- className={`bg-${theme}-dark-200 rounded text-sm text-${theme}-text-muted mb-2 pl-1 pr-2`}
- >
+ <div
+ className={`${(themeStyles[theme] ?? themeStyles.discord).bg} rounded text-sm ${(themeStyles[theme] ?? themeStyles.discord).text} mb-2 pl-1 pr-2`}
+ >And extend the themeStyles map as in DateSeparator:
const themeStyles = {
discord: {
bg: "bg-discord-dark-200",
text: "text-discord-text-muted",
},
// ...
} as const;🏁 Script executed:
#!/bin/bash
# Also search repo for other occurrences of `-${theme}-`
rg -nP --type=tsx '\-\$\{theme\}\-'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for any dynamic theme interpolation in TSX components
rg -nP '\$\{theme\}' -g '*.tsx'Length of output: 889
🏁 Script executed:
#!/bin/bash
# Search for themeStyles definitions across TSX files
rg -n "themeStyles" -g '*.tsx'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Show DateSeparator.tsx contents to find themeStyles map
sed -n '1,200p' src/components/message/DateSeparator.tsxLength of output: 669
Centralize dynamic theme classes into a shared themeStyles map
Dynamic Tailwind classes like bg-${theme}-dark-200, text-${theme}-text-muted, hover:bg-${theme}-message-hover, etc., will be purged in production unless explicitly referenced. Define a single themeStyles constant (e.g. in src/themeStyles.ts) mapping each theme to its class names (bg, text, border, hover, messageHover, …) and replace all ${theme} interpolations:
- In MessageReply.tsx: use
themeStyles[theme].bgand.text - In MessageItem.tsx: use
themeStyles[theme].messageHoverinstead ofhover:bg-${theme}-message-hover - In MessageHeader.tsx, MessageAvatar.tsx, DateSeparator.tsx: replace
text-${theme}-text-muted,border-${theme}-dark-400, etc., with mapped values.
Alternatively, add these dynamic patterns to safelist in tailwind.config.js.
🤖 Prompt for AI Agents
In src/components/message/MessageReply.tsx around line 23, dynamic Tailwind
classes like `bg-${theme}-dark-200` and `text-${theme}-text-muted` will be
purged in production; create a new src/themeStyles.ts that exports a themeStyles
map (keys = theme names) containing explicit class strings for bg, text, border,
hover, messageHover, etc., then import themeStyles in MessageReply.tsx and
replace the interpolated classes with themeStyles[theme].bg and
themeStyles[theme].text; also update MessageItem.tsx to use
themeStyles[theme].messageHover, and replace occurrences in MessageHeader.tsx,
MessageAvatar.tsx, DateSeparator.tsx with the mapped values; alternatively add
the dynamic patterns to tailwind.config.js safelist if you prefer not to
centralize.
| <div className="bg-discord-dark-400 rounded-lg shadow-lg border border-discord-dark-300 max-w-sm w-full mx-4 max-h-[90vh] overflow-hidden"> | ||
| <div className="p-2"> | ||
| <EmojiPicker | ||
| onEmojiClick={handleEmojiSelect} | ||
| theme={Theme.DARK} | ||
| width="100%" | ||
| height={400} | ||
| searchPlaceholder="Search emojis..." |
There was a problem hiding this comment.
Modal content can be clipped on small screens
Fixed height 400 plus overflow-hidden can crop the picker on short viewports. Allow scrolling.
- <div className="bg-discord-dark-400 rounded-lg shadow-lg border border-discord-dark-300 max-w-sm w-full mx-4 max-h-[90vh] overflow-hidden">
+ <div className="bg-discord-dark-400 rounded-lg shadow-lg border border-discord-dark-300 max-w-sm w-full mx-4 max-h-[90vh] overflow-auto">
<div className="p-2">
- <EmojiPicker
+ <EmojiPicker
onEmojiClick={handleEmojiSelect}
theme={Theme.DARK}
- width="100%"
- height={400}
+ width="100%"
+ height={350}Also applies to: 49-55
🤖 Prompt for AI Agents
In src/components/ui/ReactionModal.tsx around lines 34-41 (and similarly lines
49-55), the EmojiPicker is given a fixed height (400) and its container uses
overflow-hidden which can clip the picker on small viewports; change the layout
to allow vertical scrolling and responsive height by removing the fixed height
prop, replacing it with a max-height (e.g., max-h based on viewport like
max-h-[70vh] or pass a responsive height prop), and make the container use
overflow-auto (or allow the picker's internal scroll) instead of overflow-hidden
so the picker can scroll on short screens; ensure the modal still respects
max-width and rounded/shadow styles.
| if (details.channels.length > 0) { | ||
| // Remove # prefixes from channels for clean URL construction | ||
| const cleanChannels = details.channels | ||
| .map((channel) => | ||
| channel.startsWith("#") ? channel.substring(1) : channel, | ||
| ) | ||
| .join(","); | ||
| url += `/${cleanChannels}`; | ||
| } else { |
There was a problem hiding this comment.
Channel construction can produce invalid URLs for channels like “##c++”
Placing raw “#” in the path starts a fragment. Removing only one “#” still leaves another. Percent‑encode channel names (don’t strip “#”) to ensure validity and round‑trip with parseIrcUrl (which decodes).
- if (details.channels.length > 0) {
- // Remove # prefixes from channels for clean URL construction
- const cleanChannels = details.channels
- .map((channel) =>
- channel.startsWith("#") ? channel.substring(1) : channel,
- )
- .join(",");
- url += `/${cleanChannels}`;
+ if (details.channels.length > 0) {
+ const encoded = details.channels
+ .map((channel) => encodeURIComponent(channel))
+ .join(",");
+ url += `/${encoded}`;Alternative: emit channels in the hash fragment (widely used scheme), e.g., url += /#${details.channels.join(",")}; in that case, adjust parse precedence accordingly.
📝 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.
| if (details.channels.length > 0) { | |
| // Remove # prefixes from channels for clean URL construction | |
| const cleanChannels = details.channels | |
| .map((channel) => | |
| channel.startsWith("#") ? channel.substring(1) : channel, | |
| ) | |
| .join(","); | |
| url += `/${cleanChannels}`; | |
| } else { | |
| if (details.channels.length > 0) { | |
| const encoded = details.channels | |
| .map((channel) => encodeURIComponent(channel)) | |
| .join(","); | |
| url += `/${encoded}`; | |
| } else { |
| // Apply each formatting type | ||
| for (const format of formatting) { | ||
| const code = IRC_FORMATTING_CODES[format]; | ||
| if (code) { | ||
| formattedText = `${code}${formattedText}${code}`; | ||
| } | ||
| } | ||
|
|
||
| return formattedText; |
There was a problem hiding this comment.
Fix nesting order when applying multiple IRC styles
Wrapping the string on every iteration means the last format in the array is emitted first (italic wraps bold instead of the other way around), so applyIrcFormatting("Hi", ["bold", "italic"]) currently yields \x1D\x02Hi\x02\x1D, contradicting the documented \x02\x1DHi\x1D\x02. The toggles still happen, but the ordering is now inconsistent with expectations/tests and breaks deterministic round-tripping. Please build the prefix/suffix once so the formats appear in the order supplied.
Apply this diff:
- let formattedText = text;
-
- // Apply each formatting type
- for (const format of formatting) {
- const code = IRC_FORMATTING_CODES[format];
- if (code) {
- formattedText = `${code}${formattedText}${code}`;
- }
- }
-
- return formattedText;
+ const prefix: string[] = [];
+ const suffix: string[] = [];
+
+ for (const format of formatting) {
+ const code = IRC_FORMATTING_CODES[format];
+ if (code) {
+ prefix.push(code);
+ suffix.unshift(code);
+ }
+ }
+
+ return `${prefix.join("")}${text}${suffix.join("")}`;📝 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.
| // Apply each formatting type | |
| for (const format of formatting) { | |
| const code = IRC_FORMATTING_CODES[format]; | |
| if (code) { | |
| formattedText = `${code}${formattedText}${code}`; | |
| } | |
| } | |
| return formattedText; | |
| const prefix: string[] = []; | |
| const suffix: string[] = []; | |
| for (const format of formatting) { | |
| const code = IRC_FORMATTING_CODES[format]; | |
| if (code) { | |
| prefix.push(code); | |
| suffix.unshift(code); | |
| } | |
| } | |
| return `${prefix.join("")}${text}${suffix.join("")}`; |
🤖 Prompt for AI Agents
In src/lib/messageFormatter.ts around lines 77 to 85, the current loop wraps the
string on each iteration causing formats to be applied in reverse order;
instead, construct a single prefix by concatenating the formatting codes in
order and a single suffix by concatenating the same codes in reverse order, then
return prefix + originalText + suffix so formats are emitted in the supplied
order and round-tripping remains deterministic.
| return { | ||
| color: color && color !== "inherit" ? color : "inherit", | ||
| fontWeight: formatting.includes("bold") ? "bold" : "normal", | ||
| fontStyle: formatting.includes("italic") ? "italic" : "normal", | ||
| textDecoration: formatting.includes("underline") | ||
| ? "underline" | ||
| : formatting.includes("strikethrough") | ||
| ? "line-through" | ||
| : "none", | ||
| fontFamily: formatting.includes("monospace") ? "monospace" : "inherit", | ||
| }; |
There was a problem hiding this comment.
Preview should honor underline and strikethrough together
When both toggles are active the ternary collapses to "line-through", hiding the underline preview even though the outgoing IRC string contains both. Please collect the active decorations and join them so the preview mirrors what will be sent.
Apply this diff:
- textDecoration: formatting.includes("underline")
- ? "underline"
- : formatting.includes("strikethrough")
- ? "line-through"
- : "none",
+ textDecoration: (() => {
+ const decorations: string[] = [];
+ if (formatting.includes("underline")) decorations.push("underline");
+ if (formatting.includes("strikethrough"))
+ decorations.push("line-through");
+ return decorations.length ? decorations.join(" ") : "none";
+ })(),📝 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.
| return { | |
| color: color && color !== "inherit" ? color : "inherit", | |
| fontWeight: formatting.includes("bold") ? "bold" : "normal", | |
| fontStyle: formatting.includes("italic") ? "italic" : "normal", | |
| textDecoration: formatting.includes("underline") | |
| ? "underline" | |
| : formatting.includes("strikethrough") | |
| ? "line-through" | |
| : "none", | |
| fontFamily: formatting.includes("monospace") ? "monospace" : "inherit", | |
| }; | |
| return { | |
| color: color && color !== "inherit" ? color : "inherit", | |
| fontWeight: formatting.includes("bold") ? "bold" : "normal", | |
| fontStyle: formatting.includes("italic") ? "italic" : "normal", | |
| textDecoration: (() => { | |
| const decorations: string[] = []; | |
| if (formatting.includes("underline")) decorations.push("underline"); | |
| if (formatting.includes("strikethrough")) | |
| decorations.push("line-through"); | |
| return decorations.length ? decorations.join(" ") : "none"; | |
| })(), | |
| fontFamily: formatting.includes("monospace") ? "monospace" : "inherit", | |
| }; |
🤖 Prompt for AI Agents
In src/lib/messageFormatter.ts around lines 133 to 143, the textDecoration
ternary only allows either "underline" or "line-through", so when both underline
and strikethrough are active the preview shows only strikethrough; change it to
collect active decorations: build an array, push "underline" if
formatting.includes("underline"), push "line-through" if
formatting.includes("strikethrough"), then set textDecoration to the joined
array (or "none" if empty) so both decorations are shown together.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/layout/ChatArea.tsx (2)
703-713: Consider checking for existing reactions before adding.The
handleDirectReactionfunction always sends a+draft/reactmessage without checking if the user has already reacted with that emoji. This differs fromhandleReactionSelect(lines 675-701), which toggles between react and unreact based on existing reactions.If the IRC server doesn't handle duplicate reactions, consider adding a check similar to the one in
handleReactionSelect:const handleDirectReaction = (emoji: string, message: MessageType) => { if (message.msgid && selectedServerId) { const server = servers.find((s) => s.id === selectedServerId); const channel = server?.channels.find((c) => c.id === message.channelId); if (server && channel) { - // Send react message directly - const tagMsg = `@+draft/react=${emoji};+draft/reply=${message.msgid} TAGMSG ${channel.name}`; - ircClient.sendRaw(server.id, tagMsg); + // Check if user has already reacted with this emoji + const existingReaction = message.reactions.find( + (r) => r.emoji === emoji && r.userId === currentUser?.username, + ); + + if (existingReaction) { + // Send unreact message + const tagMsg = `@+draft/unreact=${emoji};+draft/reply=${message.msgid} TAGMSG ${channel.name}`; + ircClient.sendRaw(server.id, tagMsg); + } else { + // Send react message + const tagMsg = `@+draft/react=${emoji};+draft/reply=${message.msgid} TAGMSG ${channel.name}`; + ircClient.sendRaw(server.id, tagMsg); + } } } };Verify whether the IRC server automatically deduplicates reactions or if client-side checking is required.
987-1019: LGTM! Well-implemented portal-based emoji picker.The portal-based approach properly isolates the picker from layout constraints, and the configuration choices (lazy loading, dark theme, skin tone support) align with best practices for the emoji-picker-react library.
Consider adding error boundary handling around the EmojiPicker. While the library includes a built-in error boundary, you could enhance the user experience by catching render failures:
{isEmojiSelectorOpen && createPortal( <div className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50" onClick={handleEmojiModalBackdropClick} > <div className="bg-discord-dark-400 rounded-lg shadow-lg border border-discord-dark-300 max-w-sm w-full mx-4 max-h-[90vh] overflow-hidden"> <div className="p-2"> <ErrorBoundary fallback={<div className="text-discord-text-muted p-4">Failed to load emoji picker</div>}> <EmojiPicker onEmojiClick={handleEmojiSelect} theme={Theme.DARK} width="100%" height={400} searchPlaceholder="Search emojis..." previewConfig={{ showPreview: false, }} skinTonesDisabled={false} lazyLoadEmojis={true} /> </ErrorBoundary> </div> {/* ... Cancel button ... */} </div> </div>, document.body, )}Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/layout/ChatArea.tsx(11 hunks)src/components/ui/EmojiSelector.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/EmojiSelector.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/layout/ChatArea.tsx (3)
src/lib/messageFormatter.ts (4)
FormattingType(8-14)isValidFormattingType(152-164)formatMessageForIrc(104-120)getPreviewStyles(128-144)src/lib/ircUrlParser.ts (1)
parseIrcUrl(35-76)src/types/index.ts (1)
MessageType(80-80)
🔇 Additional comments (11)
src/components/layout/ChatArea.tsx (11)
3-34: LGTM! Imports align with the new modular architecture.The new imports support the centralized message formatting, IRC URL parsing, and portal-based emoji picker integration as described in the PR objectives.
78-80: LGTM! Type-safe formatting state.Using
FormattingType[]provides better type safety compared to the previousstring[]approach and aligns with the centralized formatting API.
128-138: LGTM! Proper use of centralized IRC URL parsing.The refactoring correctly leverages the new
parseIrcUrlutility and handles the default nickname fallback appropriately.
153-159: LGTM! Proper validation of persisted formatting preferences.Using
isValidFormattingTypeto filter the parsed array ensures that only valid formatting types are restored from localStorage, preventing potential issues with corrupted or outdated data.
292-296: LGTM! Correct use of centralized message formatting.The integration with
formatMessageForIrcproperly applies color and formatting options before sending the message to the IRC server.
736-739: LGTM! Correct emoji picker callback signature.The handler properly uses the
EmojiClickDatatype from emoji-picker-react and accesses the emoji via theemojiproperty as expected by the library's API.
741-745: LGTM! Standard backdrop click handling.The backdrop click handler correctly checks for direct target matching to avoid closing the picker when clicking inside it.
747-751: LGTM! Type-safe color selection handler.The updated signature using
FormattingType[]provides better type safety and aligns with the centralized formatting types.
753-759: LGTM! Type-safe formatting toggle.Using
FormattingTypeensures only valid formatting options can be toggled, preventing runtime errors from invalid format strings.
892-892: LGTM! Proper callback wiring for direct reactions.The
onDirectReactionprop correctly passes thehandleDirectReactioncallback to enable quick emoji reactions from the message UI. Note that the effectiveness depends on the duplicate reaction handling discussed in the earlier comment on lines 703-713.
957-960: LGTM! Live formatting preview in input field.Using
getPreviewStylesprovides users with a visual preview of their message formatting before sending, which is an excellent UX improvement.
* Improved emoji dialog And click again to just add * DRY types in ircclient * Split and refactor chat area a little * lint fix * Use same emoji picker everywhere
2 participants
Summary by CodeRabbit