General enhancements, optimizations, bug fixes#90
Conversation
- Preserve scroll position when user scrolls up to read history - Auto-scroll only when user is already at bottom - Add wasAtBottomRef to track scroll state before new messages arrive - Fix timing issue where scroll position was checked after DOM updates
|
Caution Review failedThe pull request is closed. WalkthroughAdds per-server buffered LIST handling with ELIST/filters and lazy loading, detects UnrealIRCd via RPL_YOURHOST, extends IRC client and store APIs, refactors message rendering with collapsible messages and enhanced markdown (hljs, copy UI), refines link security, adds OPER credentials in settings, and adjusts ChatArea auto-scroll behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ScrollHandler
participant ChatArea
participant MessageService
User->>ScrollHandler: scroll
ScrollHandler->>ChatArea: set wasAtBottomRef(atBottom)
MessageService->>ChatArea: deliver new messages
ChatArea->>ChatArea: read wasAtBottomRef
alt wasAtBottomRef == true
ChatArea->>ChatArea: scrollToBottom()
else
ChatArea->>ChatArea: append messages (no auto-scroll)
end
Note right of ChatArea: switching channels resets wasAtBottomRef -> true
sequenceDiagram
participant User
participant ChannelListModal
participant Store
participant IRCClient
User->>ChannelListModal: open modal(server)
ChannelListModal->>Store: read channelListFilters[server]
User->>ChannelListModal: apply filters
ChannelListModal->>Store: updateChannelListFilters(server, filters)
ChannelListModal->>IRCClient: listChannels(server, elist, filters)
IRCClient->>Store: emit LIST_CHANNEL (buffered)
IRCClient->>Store: emit LIST_END
Store->>Store: commit channelListBuffer -> channelList
Store->>ChannelListModal: channelList updated
ChannelListModal->>User: render filtered/lazy-loaded channel rows
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/layout/ChannelList.tsx(1 hunks)src/components/ui/UserSettings.tsx(14 hunks)tests/App.test.tsx(6 hunks)tests/components/UserSettings.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/App.test.tsx (1)
src/lib/ircClient.ts (1)
ircClient(2643-2643)
src/components/ui/UserSettings.tsx (3)
src/store/index.ts (1)
loadSavedServers(108-110)src/lib/ircClient.ts (1)
sendRaw(763-776)src/types/index.ts (1)
ServerConfig(48-64)
| // Save oper settings to server config if changed | ||
| if (currentServer) { | ||
| const serverConfigUpdates: Partial<ServerConfig> = {}; | ||
| if (operName !== originalValues.operName) { | ||
| serverConfigUpdates.operUsername = operName || undefined; | ||
| } | ||
| if (operPassword !== originalValues.operPassword) { | ||
| serverConfigUpdates.operPassword = operPassword || undefined; | ||
| } | ||
| if (operOnConnect !== originalValues.operOnConnect) { | ||
| serverConfigUpdates.operOnConnect = operOnConnect; | ||
| } | ||
| if (Object.keys(serverConfigUpdates).length > 0) { | ||
| updateServer(currentServer.id, serverConfigUpdates); | ||
| } | ||
| } |
There was a problem hiding this comment.
“Forget Credentials” leaves the oper password behind
Because operPassword is initialized to an empty string and originalValues.operPassword is captured as the same empty string, the comparison in handleSaveAll never detects a change when we wipe the field (either manually or via Forget Credentials). As a result, updateServer never receives operPassword: undefined, so the previously stored value remains in localStorage. Reproduce: save an oper password once, reopen settings, hit “Forget Credentials”, save—inspect the saved config and the password is still there. We need to mark the field as dirty (or track the stored value separately) and always propagate the clear operation so the credential actually gets removed.
- if (operPassword !== originalValues.operPassword) {
- serverConfigUpdates.operPassword = operPassword || undefined;
- }
+ if (operPasswordDirty) {
+ serverConfigUpdates.operPassword = operPassword || undefined;
+ }(Remember to add an operPasswordDirty flag that flips on any input change or when “Forget Credentials” is pressed, and include it in hasUnsavedChanges.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/UserSettings.tsx around lines 932-947, the oper password
clear action is not propagated because operPassword is initialized to empty
string and the save logic only compares values; add an operPasswordDirty boolean
state initialized false, set operPasswordDirty = true whenever the oper password
input changes or when “Forget Credentials” is pressed, include operPasswordDirty
in hasUnsavedChanges, and change the save logic to treat operPassword as updated
when operPasswordDirty is true (i.e., set serverConfigUpdates.operPassword =
operPassword || undefined when operPasswordDirty or when operPassword !==
originalValues.operPassword), then pass that to updateServer so clearing the
field writes operPassword: undefined to storage.
- Default sort channels by user count (highest first) - Display user count as purple badge matching selected server tab color - Click overlay to close modal - Make channels list scrollable while keeping filter/sort options fixed - Fix modal layout structure for proper scrolling behavior
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/ChannelListModal.tsx(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint and Tests
src/components/ui/ChannelListModal.tsx
[error] 165-165: File content differs from formatting output. ChannelListModal.tsx needs formatting. Run the formatter to fix.
🔇 Additional comments (1)
src/components/ui/ChannelListModal.tsx (1)
207-208: LGTM! Proper flexbox scroll implementation.The combination of
flex-1,min-h-0, andoverflow-y-autocorrectly enables scrolling within the flex container. Themin-h-0is essential to allow the flex child to shrink below its content size, preventing layout issues when the content overflows.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/ui/ChannelListModal.tsx (2)
204-208: Trailing whitespace issue resolved.The earlier pipeline failure is fixed here.
204-211: Add role/aria and ESC-to-close for keyboard accessibility.Modal supports click-to-close, but lacks ESC handling and dialog semantics.
Apply this diff for ARIA; then add the keydown handler shown below:
- <div - className="bg-discord-dark-200 rounded-lg w-full max-w-2xl p-5 max-h-[90vh] flex flex-col" - onClick={(e) => e.stopPropagation()} - > + <div + className="bg-discord-dark-200 rounded-lg w-full max-w-2xl p-5 max-h-[90vh] flex flex-col" + onClick={(e) => e.stopPropagation()} + role="dialog" + aria-modal="true" + aria-labelledby="channel-list-title" + tabIndex={-1} + >Add this effect in the component to close on Escape:
useEffect(() => { const onKeyDown = (e: KeyboardEvent) => { if (e.key === "Escape") { e.stopPropagation(); toggleChannelListModal(false); } }; window.addEventListener("keydown", onKeyDown); return () => window.removeEventListener("keydown", onKeyDown); }, [toggleChannelListModal]);
🧹 Nitpick comments (9)
src/protocol/isupport.ts (1)
77-88: Normalize ELIST value when storingStore ELIST as uppercase to simplify later checks and avoid case drift.
Apply:
- if (key === "ELIST") { - useStore.setState((state) => { + if (key === "ELIST") { + const elist = (value || "").toUpperCase(); + useStore.setState((state) => { const updatedServers = state.servers.map((server: Server) => { if (server.id === serverId) { - return { ...server, elist: value }; + return { ...server, elist }; } return server; }); return { servers: updatedServers }; }); return; }tests/components/ChannelListModal.test.tsx (1)
119-120: Consider asserting close interactionOptional: verify toggleChannelListModal (or equivalent) is called when clicking the Close control to improve coverage.
src/lib/ircUtils.tsx (1)
515-522: Minor simplification possibleReturn a boolean cast instead of strict equality for clarity.
-export function isUnrealIRCd(server: Server): boolean { - return server.isUnrealIRCd === true; -} +export function isUnrealIRCd(server: Server): boolean { + return !!server.isUnrealIRCd; +}src/lib/ircClient.ts (1)
1665-1679: Make RPL_YOURHOST parsing more robustRelax wording/case to avoid missing variants (e.g., “running vX”, capitalization differences).
- } else if (command === "002") { + } else if (command === "002") { // RPL_YOURHOST - Your host is <servername>, running version <version> - const message = parv.slice(1).join(" "); - // Parse the message: "Your host is <servername>, running version <version>" - const match = message.match( - /Your host is ([^,]+), running version (.+)/, - ); + const message = parv.slice(1).join(" "); + // Be tolerant of variations and case + const re = /your host is ([^,]+), running (?:version\s*)?(.+)/i; + const match = re.exec(message); if (match) { const serverName = match[1]; const version = match[2]; this.triggerEvent("RPL_YOURHOST", { serverId, serverName, version, }); } }src/components/ui/ChannelListModal.tsx (1)
192-201: Ensure new list items are observed after filter/sort changes.Currently observed once; new DOM nodes after re-render may not be observed, so metadata fetches can be missed.
Apply this diff to observe/unobserve in the ref callback:
const setChannelRef = ( channelName: string, element: HTMLDivElement | null, ) => { - if (element) { - channelRefs.current.set(channelName, element); - } else { - channelRefs.current.delete(channelName); - } + if (element) { + channelRefs.current.set(channelName, element); + observerRef.current?.observe(element); + } else { + const prev = channelRefs.current.get(channelName); + if (prev) observerRef.current?.unobserve(prev); + channelRefs.current.delete(channelName); + } };src/store/index.ts (4)
595-607: Avoid clearing existing list while (re)listing to reduce flicker.You wipe channelList before LIST completes, causing brief empty state. Keep current list, just set listingInProgress and clear the buffer; replace on LIST_END.
Apply this diff:
- // Clear the channel list and buffer before starting a new list - set((state) => ({ - channelList: { - ...state.channelList, - [serverId]: [], - }, - channelListBuffer: { + // Prepare for new list: clear buffer, keep current list until LIST_END + set((state) => ({ + channelListBuffer: { ...state.channelListBuffer, [serverId]: [], }, listingInProgress: { ...state.listingInProgress, [serverId]: true, }, }));Also applies to: 1262-1291
1293-1300: Persist filters optionally.Consider persisting channelListFilters to localStorage per server to survive reloads.
1319-1327: Make duplicate check timestamp-safe.Strict equality on timestamps can miss dupes (Date vs string). Compare epoch millis.
- (existingMessage.content === message.content && - existingMessage.timestamp === message.timestamp && - existingMessage.userId === message.userId) + (existingMessage.content === message.content && + new Date(existingMessage.timestamp).getTime() === + new Date(message.timestamp).getTime() && + existingMessage.userId === message.userId)
5759-5765: LIST buffering is correct; consider chunked commits for huge lists.For servers with tens of thousands of channels, a single giant array push and final swap can stutter. Optionally commit in batches (e.g., every N entries or via rAF throttle).
Also applies to: 5771-5781
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/ui/ChannelListModal.tsx(5 hunks)src/lib/ircClient.ts(4 hunks)src/lib/ircUtils.tsx(2 hunks)src/protocol/isupport.ts(1 hunks)src/store/index.ts(8 hunks)src/types/index.ts(1 hunks)tests/components/ChannelListModal.test.tsx(4 hunks)tests/components/ChannelSettingsModal.test.tsx(1 hunks)tests/components/LinkSecurityWarningModal.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/protocol/isupport.ts (1)
src/types/index.ts (1)
Server(25-48)
src/components/ui/ChannelListModal.tsx (2)
src/lib/ircClient.ts (1)
listChannels(1018-1085)src/lib/ircUtils.tsx (2)
getChannelAvatarUrl(454-469)getChannelDisplayName(439-449)
src/store/index.ts (2)
src/lib/ircClient.ts (1)
ircClient(2728-2728)src/lib/ircUtils.tsx (1)
isUnrealIRCd(520-522)
src/lib/ircClient.ts (1)
src/types/index.ts (1)
BaseIRCEvent(206-208)
src/lib/ircUtils.tsx (1)
src/types/index.ts (1)
Server(25-48)
🔇 Additional comments (13)
tests/components/ChannelSettingsModal.test.tsx (1)
76-78: State shape alignment looks goodAdding channelListBuffer and channelListFilters keeps tests in sync with store. No issues.
src/types/index.ts (1)
46-47: Type additions are appropriateServer gains isUnrealIRCd and elist; consistent with protocol/util changes.
tests/components/LinkSecurityWarningModal.test.tsx (1)
119-121: Mock state updated correctlychannelListBuffer and channelListFilters additions keep tests compatible with store shape.
tests/components/ChannelListModal.test.tsx (2)
28-34: Store mock updates align with new filtering/buffering flowchannelListBuffer/channelListFilters and updateChannelListFilters mock are appropriate for the new flow.
Also applies to: 43-44
65-67: Updated expectations for numeric counts make senseAsserting raw numbers (“10”, “20”, “5”) matches the new UI.
src/lib/ircUtils.tsx (1)
4-4: Import change is fineType import extended to include Server; consistent with new helper usage.
src/lib/ircClient.ts (3)
195-198: Event map extension LGTMRPL_YOURHOST event definition is clear and consistent.
1826-1831: Timestamp selection for multiline batches LGTMTaking the earliest timestamp (or batch time) is sensible for ordering.
1018-1085: ELIST parameter building: syntax verified as correctThe syntax you've implemented matches the IRC standard across common daemons (UnrealIRCd, DALnet, Bahamut, ISUPPORT ELIST draft): user-count filters as
>n/<n(U), creation/topic time asC>n/C<nandT>n/T<n(C, T), and masks asmask/!mask(M, N). No changes needed.Regarding allowing 0 values: if meaningful for your servers (e.g., channels modified 0 minutes ago, or exactly 0 users), consider relaxing the
> 0checks to>= 0. Otherwise, the current guards are fine.src/components/ui/ChannelListModal.tsx (3)
421-426: Loading state gating looks good.Correctly hides “No channels found” while listing is in progress.
152-166: State sync with store filters works.Values hydrate from per‑server filters as expected.
167-183: Filters application flow LGTM.Commits to store and triggers list refresh with normalized values.
src/store/index.ts (1)
468-485: AppState additions for channelListBuffer and channelListFilters look solid.Clear per‑server modeling; aligns with LIST buffering and UI filters.
| ircClient.on("RPL_YOURHOST", ({ serverId, serverName, version }) => { | ||
| // Check if the server is running UnrealIRCd | ||
| const isUnrealIRCd = version.includes("UnrealIRCd"); | ||
|
|
||
| // Update the server with the UnrealIRCd information | ||
| useStore.setState((state) => ({ | ||
| servers: state.servers.map((server) => | ||
| server.id === serverId ? { ...server, isUnrealIRCd } : server, | ||
| ), | ||
| })); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Guard version, avoid name shadowing, and reuse util if available.
version may be undefined; also local const name matches util fn name.
-ircClient.on("RPL_YOURHOST", ({ serverId, serverName, version }) => {
- // Check if the server is running UnrealIRCd
- const isUnrealIRCd = version.includes("UnrealIRCd");
+ircClient.on("RPL_YOURHOST", ({ serverId, serverName, version }) => {
+ // Detect UnrealIRCd
+ const isUnreal = typeof version === "string" && version.includes("UnrealIRCd");
// Update the server with the UnrealIRCd information
useStore.setState((state) => ({
servers: state.servers.map((server) =>
- server.id === serverId ? { ...server, isUnrealIRCd } : server,
+ server.id === serverId ? { ...server, isUnrealIRCd: isUnreal } : server,
),
}));
});📝 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.
| ircClient.on("RPL_YOURHOST", ({ serverId, serverName, version }) => { | |
| // Check if the server is running UnrealIRCd | |
| const isUnrealIRCd = version.includes("UnrealIRCd"); | |
| // Update the server with the UnrealIRCd information | |
| useStore.setState((state) => ({ | |
| servers: state.servers.map((server) => | |
| server.id === serverId ? { ...server, isUnrealIRCd } : server, | |
| ), | |
| })); | |
| }); | |
| ircClient.on("RPL_YOURHOST", ({ serverId, serverName, version }) => { | |
| // Detect UnrealIRCd | |
| const isUnreal = typeof version === "string" && version.includes("UnrealIRCd"); | |
| // Update the server with the UnrealIRCd information | |
| useStore.setState((state) => ({ | |
| servers: state.servers.map((server) => | |
| server.id === serverId ? { ...server, isUnrealIRCd: isUnreal } : server, | |
| ), | |
| })); | |
| }); |
🤖 Prompt for AI Agents
In src/store/index.ts around lines 5062 to 5073, the handler uses version
without guarding against undefined and shadows a util name; change the callback
to first obtain the version safely (e.g., const ver = version ?? ""), or better,
call an existing util isUnrealIRCd(ver) if available, and rename the local
boolean to avoid name shadowing (e.g., const isUnreal = typeof ver === "string"
&& ver.includes("UnrealIRCd") or const isUnreal = isUnrealIRCd(ver)); then
update useStore.setState exactly as before but using the new local name
(isUnreal) so includes is only called on a string and the util is
reused/variable name collision is avoided.
…anced filtering using ELIST ISUPPORT
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/ChannelListModal.tsx (1)
158-172: IntersectionObserver never observes new items; metadata prefetch won’t run.The “Observe channel elements” effect runs once with an empty map. New items aren’t observed. Observe/unobserve in setChannelRef and set IO root to the scroll container.
Apply this diff:
@@ - // Setup IntersectionObserver - useEffect(() => { - observerRef.current = new IntersectionObserver( + // Setup IntersectionObserver (use scroll container as root when available) + useEffect(() => { + const root = scrollContainerRef.current ?? null; + observerRef.current = new IntersectionObserver( (entries) => { const visibleChannels = entries .filter((entry) => entry.isIntersecting) .map((entry) => entry.target.getAttribute("data-channel")) .filter((ch): ch is string => ch !== null); if (visibleChannels.length > 0) { fetchMetadataForChannels(visibleChannels); } }, { - root: null, + root, rootMargin: "100px", // Start loading slightly before they come into view threshold: 0.1, }, ); return () => { if (observerRef.current) { observerRef.current.disconnect(); } }; - }, [fetchMetadataForChannels]); + }, [fetchMetadataForChannels]); @@ - // Observe channel elements - useEffect(() => { - if (!observerRef.current) return; - - channelRefs.current.forEach((element) => { - observerRef.current?.observe(element); - }); - - return () => { - if (observerRef.current) { - channelRefs.current.forEach((element) => { - observerRef.current?.unobserve(element); - }); - } - }; - }, []); + // (Removed) Observe-once effect; observation handled in setChannelRef @@ const setChannelRef = ( channelName: string, element: HTMLDivElement | null, ) => { - if (element) { - channelRefs.current.set(channelName, element); - } else { - channelRefs.current.delete(channelName); - } + if (element) { + channelRefs.current.set(channelName, element); + observerRef.current?.observe(element); + } else { + const prev = channelRefs.current.get(channelName); + if (prev) observerRef.current?.unobserve(prev); + channelRefs.current.delete(channelName); + } };Also applies to: 259-268, 130-155
♻️ Duplicate comments (1)
src/components/ui/ChannelListModal.tsx (1)
270-299: Add ESC-to-close for keyboard accessibility (previously requested).Modal lacks ESC handling. Add a keydown listener to close on Escape.
Apply this diff:
@@ const { @@ } = useStore(); @@ + useEffect(() => { + const onKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape") { + toggleChannelListModal(false); + } + }; + window.addEventListener("keydown", onKeyDown); + return () => window.removeEventListener("keydown", onKeyDown); + }, [toggleChannelListModal]); @@ - <div + <div + role="dialog" + aria-modal="true" + aria-label="Channel list" className="fixed inset-0 bg-black bg-opacity-70 flex items-center justify-center z-50" onClick={() => toggleChannelListModal(false)} >As per previous review.
🧹 Nitpick comments (2)
src/components/ui/ChannelListModal.tsx (2)
542-553: Lazy-load avatars to reduce network/paint cost.Add loading="lazy" and decoding="async" to the img.
Apply this diff:
- <img + <img + loading="lazy" + decoding="async" src={avatarUrl} alt={channel.channel} className="w-8 h-8 rounded-full object-cover"
195-224: Optional: clear timeout on unmount to avoid stray state updates.Minor polish: store the setTimeout id and clear it in the effect cleanup.
- setTimeout(() => { + const id = window.setTimeout(() => { setDisplayedChannelsCount((prev) => Math.min(prev + 50, filteredChannels.length), ); setLoadingMore(false); }, 200); // Small delay for smooth UX + return () => window.clearTimeout(id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/ChannelListModal.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/ChannelListModal.tsx (2)
src/lib/ircClient.ts (1)
listChannels(1018-1085)src/lib/ircUtils.tsx (2)
getChannelAvatarUrl(454-469)getChannelDisplayName(439-449)
🔇 Additional comments (1)
src/components/ui/ChannelListModal.tsx (1)
248-250: No signature mismatch—review comment is incorrect.The component calls
listChannelsfrom the store wrapper (not directly fromircClient). The store's signature is(serverId, filters?), which matches your calls at lines 176 and 249. The store internally extractselistfrom the server configuration and correctly forwards(serverId, elist, filterSettings)toircClient.listChannels, so parameter order is correct and no mis-routing occurs.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/ircUtils.tsx (2)
264-309: Consider adding language validation for security.The code block renderer properly handles hljs integration with fallbacks, but consider validating the
langparameter to prevent potential injection if it's user-controlled. Whilehljs.getLanguage()provides some validation, an explicit allowlist of supported languages would add defense-in-depth.Example implementation:
// At module level const SUPPORTED_LANGUAGES = new Set([ 'javascript', 'typescript', 'python', 'java', 'cpp', 'c', 'go', 'rust', 'ruby', 'php', 'swift', 'kotlin', 'scala', 'html', 'css', 'json', 'xml', 'yaml', 'markdown', 'sql' // Add other commonly used languages ]); // In renderer.code renderer.code = ({ text, lang, escaped }) => { const trimmedText = text.trimEnd(); let highlightedCode = trimmedText; let language = lang; // Only attempt highlighting if language is in the allowlist if (lang && SUPPORTED_LANGUAGES.has(lang) && hljs.getLanguage(lang)) { // ... rest of highlighting logic } // ... };
346-358: Consider using crypto.randomUUID() for IDs.The current ID generation using
Math.random().toString(36).substr(2, 9)works but has potential collision risks. Consider usingcrypto.randomUUID()for cryptographically secure unique IDs, or at leastcrypto.getRandomValues()for better entropy.Example:
// For code block IDs const codeId = `code-${crypto.randomUUID()}`; // For inline code IDs (in codespan renderer) const codeId = `inline-code-${crypto.randomUUID()}`;This eliminates collision risks and provides better uniqueness guarantees, especially if multiple code blocks are rendered simultaneously.
📜 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 (6)
package.json(1 hunks)src/components/ui/LinkWrapper.tsx(1 hunks)src/index.css(8 hunks)src/lib/ircUtils.tsx(6 hunks)tests/components/ChannelListModal.test.tsx(5 hunks)tests/lib/messageFormatter.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/lib/messageFormatter.test.ts (1)
src/lib/ircUtils.tsx (1)
renderMarkdown(194-415)
src/lib/ircUtils.tsx (1)
src/types/index.ts (1)
Server(25-48)
🪛 ast-grep (0.39.6)
src/lib/ircUtils.tsx
[warning] 364-364: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🔇 Additional comments (13)
tests/components/ChannelListModal.test.tsx (4)
28-43: LGTM! Mock store properly extended for buffering and filtering.The test mock correctly includes the new per-server
channelListBufferandchannelListFiltersstate, plus theupdateChannelListFiltersaction. These additions align with the store changes for buffered LIST results handling.
56-56: LGTM! UI text updated to match component changes.The header text expectation correctly reflects the new channel list modal header format.
65-67: LGTM! User count display expectations updated.Test expectations correctly updated to match the simplified user count display format (numeric values only).
119-119: LGTM! Close button selector improved.Using
getByLabelText("Close")is more accessible and semantic than the previous generic role-based selector.tests/lib/messageFormatter.test.ts (1)
343-390: LGTM! Comprehensive test coverage for new markdown features.These test cases effectively validate:
- Strikethrough rendering with
~~text~~syntax- Prevention of single-line tilde syntax from being treated as code blocks
- Multi-line tilde-fenced code block rendering
- Syntax highlighting integration via highlight.js
- Copy button functionality in code blocks
The tests align with the markdown rendering enhancements in
src/lib/ircUtils.tsxand provide good coverage of the new features.src/index.css (3)
5-10: LGTM! Proper highlight.js stylesheet integration.The GitHub theme import with transparent background override ensures syntax highlighting works correctly without interfering with the custom code block styling. This aligns with the highlight.js integration in
src/lib/ircUtils.tsx.
249-318: LGTM! Well-designed copy button UI.The code block copy button implementation includes:
- Proper positioning and styling for both regular and inline code blocks
- Smooth hover transitions with opacity changes
- Comprehensive dark mode support
- Consistent sizing and spacing
The inline code copy button's hover-triggered appearance (line 353-355) is a nice UX touch that keeps the UI clean when not needed.
488-579: LGTM! Comprehensive dark mode syntax highlighting.The dark mode hljs class mappings cover all major token types (keywords, strings, comments, operators, etc.) with appropriate colors from the GitHub dark theme palette. This ensures consistent and readable syntax highlighting in dark mode.
src/lib/ircUtils.tsx (4)
252-261: LGTM! Inline code copy button implementation.The custom
codespanrenderer adds copy functionality to inline code blocks with a unique ID and hidden-until-hover button. The implementation is clean and follows the same pattern as the block-level copy buttons.
318-325: LGTM! Single-line tilde syntax properly escaped.The regex correctly identifies and escapes single-line tilde-fenced code (e.g.,
~~~lang code~~~) to prevent it from being rendered as a code block. This aligns with the test case intests/lib/messageFormatter.test.ts(lines 351-357).
366-412: LGTM! Robust copy button click handler.The click handler implementation includes:
- Proper event delegation for both block and inline copy buttons
- Success and error feedback with visual indicators (checkmark/X)
- Automatic reset after 2 seconds
- Dynamic sizing based on button type
- Error logging for debugging
The implementation is production-ready and provides good UX.
669-676: LGTM! Simple and clear server type check.The
isUnrealIRCdutility function provides a clean API for checking server type. The implementation correctly checks the optionalisUnrealIRCdboolean property with strict equality.src/components/ui/LinkWrapper.tsx (1)
29-41: LGTM! Proper DOM traversal for external link detection.The updated click handler correctly traverses up the DOM tree to detect the
external-link-securityclass on the target element or any of its ancestors. This is necessary because click events may originate from child elements (e.g., images, SVG icons, or text nodes) within the link.The implementation:
- Uses a
whileloop with proper null checking- Breaks early once a matching element is found (efficient)
- Correctly reads the
hreffrom the matched element rather than the click targetThis aligns with the markdown rendering changes in
src/lib/ircUtils.tsxwhere external links are assigned theexternal-link-securityclass.
ValwareIRC
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/index.css (2)
175-177: Consider the impact of negative margins and !important overrides.The negative margins (
-0.0625rem,-0.125rem) and multiple!importantdeclarations tighten spacing for a compact chat UI but may cause:
- Content overlap with certain markdown patterns
- Difficulty overriding styles in specific contexts
- Maintenance challenges when adjusting spacing in the future
Ensure these spacing adjustments have been tested with various markdown patterns (nested lists, complex structures, etc.) to avoid visual glitches.
Also applies to: 415-457
342-394: Inline code copy button may misalign with dynamic font sizes.The inline code container adjusts padding on hover (line 353:
padding-right: 2.125rem), but the copy button uses absolute positioning with fixedright: 0.25rem. This could cause misalignment with:
- Different font sizes/zoom levels
- Custom font families with varying metrics
- RTL layouts
Consider testing at different zoom levels and with various fonts to ensure the copy button remains properly positioned.
src/components/message/MessageItem.tsx (1)
562-564: Consider memoizing collapsibleContent to avoid recreation on every render.The
collapsibleContentelement is recreated on every render of MessageItem. SincehtmlContentis already computed and MessageItem is wrapped inReact.memo, consider usinguseMemoto avoid unnecessary CollapsibleMessage component recreation.- // Create collapsible content wrapper - const collapsibleContent = <CollapsibleMessage content={htmlContent} />; + // Create collapsible content wrapper + const collapsibleContent = useMemo( + () => <CollapsibleMessage content={htmlContent} />, + [htmlContent] + );Note: You'll need to import
useMemofrom React (it's not currently imported, thoughuseCallbackis on line 3).src/components/message/CollapsibleMessage.tsx (1)
63-80: Add ARIA attributes for better accessibility.The toggle button lacks
aria-expandedandaria-controlsattributes, which are important for screen reader users to understand the collapsible state.Apply this diff:
<button onClick={toggleExpanded} + aria-expanded={isExpanded} + aria-label={isExpanded ? "Show less content" : "Show more content"} className="ml-1 text-blue-500 hover:text-blue-600 text-xs font-medium cursor-pointer border border-blue-500 rounded-full py-0 px-1" style={{ textDecoration: "none" }} >This improves accessibility by explicitly communicating the button's purpose and state to assistive technologies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/message/CollapsibleMessage.tsx(1 hunks)src/components/message/MessageItem.tsx(3 hunks)src/components/message/index.ts(1 hunks)src/index.css(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/message/MessageItem.tsx (2)
src/components/message/CollapsibleMessage.tsx (1)
CollapsibleMessage(9-84)src/components/message/index.ts (1)
CollapsibleMessage(4-4)
src/components/message/CollapsibleMessage.tsx (1)
src/components/message/index.ts (1)
CollapsibleMessage(4-4)
🔇 Additional comments (9)
src/index.css (3)
5-13: LGTM: Collapsible message styles with good fallback.The line-clamp implementation correctly uses both webkit-prefixed and standard properties for broader compatibility, with a max-height fallback for unsupported browsers.
271-340: LGTM: Code block copy button implementation.The copy button styling is well-structured with proper hover states and dark mode support. The
position: staticfor.copy-buttonis appropriate for the header layout context.
510-601: LGTM: Comprehensive dark mode syntax highlighting.The syntax highlighting color mappings are thorough and cover all major token types with appropriate semantic colors.
src/components/message/index.ts (1)
4-4: LGTM: Clean export addition.The CollapsibleMessage export is properly added and maintains alphabetical ordering.
src/components/message/MessageItem.tsx (2)
16-16: LGTM: Clean import of CollapsibleMessage.
876-886: No virtualization currently implemented; performance concern is valid but may be overstated.The codebase has no virtualization (react-window, react-virtualized) or message windowing in place. Every text message instantiates CollapsibleMessage, which adds 5 state variables per instance and runs
useLayoutEffectto measure content height on mount. While the layout effect dependency array[maxLines]means repeated measurements are avoided, having 1000+ message nodes in the DOM with per-message state could still impact memory usage and performance.However, the impact is primarily from DOM size and state overhead rather than repeated layout thrashing. The component already uses React.memo and optimization patterns (useCallback, useMemo).
Suggested approach:
- Benchmark with realistic message volumes before optimizing
- If performance degrades, consider message list virtualization over selective wrapping (virtualization is simpler than detecting "likely long" messages)
- IntersectionObserver is already used in ChannelListModal; similar pattern could apply to messages if needed
src/components/message/CollapsibleMessage.tsx (3)
4-12: LGTM: Well-defined component interface.The props interface is clean and the default
maxLinesof 3 is appropriate for chat messages.
13-18: LGTM: State initialization is appropriate.
46-83: LGTM: Render logic is well-structured with appropriate conditional styling.The maxHeight transitions and arrow animation are correctly implemented. The combination of CSS classes and inline styles provides smooth expand/collapse animations.
| useLayoutEffect(() => { | ||
| if (!contentRef.current) return; | ||
|
|
||
| // Measure the actual rendered content height | ||
| const element = contentRef.current; | ||
| const computedStyle = window.getComputedStyle(element); | ||
| const lineHeight = Number.parseFloat(computedStyle.lineHeight) || 16; // fallback to 16px | ||
| const maxHeight = lineHeight * maxLines; | ||
|
|
||
| // Get the full content height | ||
| const fullHeight = element.scrollHeight; | ||
| setContentHeight(fullHeight); | ||
|
|
||
| // Check if content overflows the max height | ||
| setNeedsCollapsing(fullHeight > maxHeight); | ||
| }, [maxLines]); |
There was a problem hiding this comment.
Add content to the dependency array to handle dynamic content updates.
The useLayoutEffect only depends on maxLines, but if the content prop changes (e.g., message editing, dynamic updates), the height measurement and needsCollapsing flag won't update. This could cause:
- Stale collapse state when content changes
- Incorrect "Show more" button visibility
- Wrong expanded height
Apply this diff:
- }, [maxLines]);
+ }, [maxLines, content]);This ensures the component remeasures when content changes. Note that content is a ReactNode which may not have stable reference equality, so consider the performance implications if parent components frequently recreate the content prop.
🤖 Prompt for AI Agents
In src/components/message/CollapsibleMessage.tsx around lines 20 to 35, the
useLayoutEffect currently only depends on maxLines so it won't remeasure when
the message content changes; update the effect dependency array to include the
content prop (i.e., [maxLines, content]) so the effect remeasures and updates
contentHeight and needsCollapsing whenever content changes, and ensure any lint
warnings are addressed (if content is a ReactNode with unstable identity,
consider memoizing the content upstream or stableizing the prop to avoid
unnecessary re-renders).
…e multiline cutoff point
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.css (1)
426-440: Resolve conflicting and over-specified padding rules in lists.Lines 426–440 define list styling with conflicting rules: line 431 sets
padding: 0 !important, but line 439 addspadding-left: 0.5remwithout!important. The second rule will not override the first due to specificity. Additionally, the extensive!importantflags suggest potential CSS conflicts; these should be resolved through proper specificity rather than broad!importantusage.Consolidate list padding into a single rule without
!important:.markdown-content ul, .markdown-content ol { list-style-type: disc; list-style-position: inside; - margin: 0 !important; - padding: 0 !important; + margin: 0; + padding: 0 0 0 0.5rem; margin-bottom: 0.125rem !important; margin-top: -0.125rem !important; line-height: 1.0 !important; display: block; box-sizing: border-box; height: auto !important; min-height: auto !important; - padding-left: 0.5rem; }
🧹 Nitpick comments (1)
src/index.css (1)
267-267: Dark mode inline code background is overly harsh.Line 267 sets the dark mode code background to pure
black, which creates high contrast and appears harsher than the surroundingpreblock background (rgb(31 41 55)on line 280). For visual consistency and readability, consider using a softer color likergb(31 41 55)orrgb(20 24 32).Apply this adjustment for softer dark mode appearance:
-.dark .markdown-content code { - background-color: black; -} +.dark .markdown-content code { + background-color: rgb(31 41 55); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/message/CollapsibleMessage.tsx(1 hunks)src/index.css(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/message/CollapsibleMessage.tsx
🔇 Additional comments (4)
src/index.css (4)
321-321: Verify copy button positioning strategy.Line 321 marks
.copy-buttonwithposition: static, which prevents absolute positioning within a relative container. For UI elements that need precise positioning (like copy buttons), this typically should beposition: relativeorposition: absolutedepending on the layout context.Verify the intended behavior: Is
position: staticcorrect, or should this beposition: relativeorposition: absoluteto allow proper positioning relative to its container?
260-260: Clarify non-standard code padding value.Line 260 uses
padding: 0.25rem 0.45rem, where the horizontal padding of 0.45rem is unusual compared to standard Tailwind/CSS increment patterns (0.25, 0.5, 0.75, 1rem). Was this intentional, or should it be0.5remfor consistency?
192-194: Clarify intent of negative margin on markdown content.Line 193 applies a very small negative margin (
margin-bottom: -0.0625rem) to markdown content elements. This unusual value is not immediately clear in purpose—is it compensating for an upstream style, or is there a specific design intent? Document the rationale or consider removing if it's a legacy adjustment.Confirm whether the
-0.0625remmargin is intentional and, if so, add an inline comment explaining the spacing adjustment.
527-618: Dark mode syntax highlighting additions look well‑considered.The comprehensive GitHub-themed syntax highlighting colors (lines 527–618) are well-structured and use appropriate semantic color assignments for different syntax elements. The color palette is consistent and readable.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/ChannelListModal.tsx (1)
159-173: Observer won't track new channel elements due to empty dependency array.The effect only runs once on mount because of the empty
[]dependency array. WhendisplayedChannelsCountincreases orfilteredChannelschanges, new channel elements are rendered but never observed for metadata fetching. Visible channels beyond the initial 50 won't trigger metadata requests.Apply this diff to re-run observation when the displayed channels change:
}, [fetchMetadataForChannels]); // Observe channel elements useEffect(() => { if (!observerRef.current) return; channelRefs.current.forEach((element) => { observerRef.current?.observe(element); }); return () => { if (observerRef.current) { channelRefs.current.forEach((element) => { observerRef.current?.unobserve(element); }); } }; - }, []); + }, [displayedChannelsCount, filteredChannels.length]);
♻️ Duplicate comments (1)
src/components/message/CollapsibleMessage.tsx (1)
31-46: Still missingcontentin the dependency array.The previous review comment about adding
contentto the dependency array has not been addressed. When thecontentprop changes, the component won't remeasure the height, potentially causing incorrect collapse state and wrong "Show more" button visibility.Apply this diff:
- }, [maxLines]); + }, [maxLines, content]);Note: Since
contentis a ReactNode, consider the performance implications if the parent frequently recreates this prop.
🧹 Nitpick comments (4)
src/components/message/CollapsibleMessage.tsx (1)
66-107: Consider defensive handling for nullcontentHeight.In the inline style at lines 72-76, when
isExpandedis true, the code uses${contentHeight}px. IfcontentHeightis null, this would render invalid CSS ("nullpx").While the logical flow suggests this shouldn't happen (button only renders when
needsCollapsingis true, which impliescontentHeightis set), defensive coding would be safer.Apply this diff for more defensive rendering:
maxHeight: isExpanded - ? `${contentHeight}px` + ? contentHeight ? `${contentHeight}px` : "none" : needsCollapsing ? "4.5em" : "none",src/index.css (1)
426-491: Heavy use of!importantflags indicates potential specificity issues.The list styling rules (lines 426-491) use many
!importantdeclarations (7 in the ul/ol rule alone). This typically indicates:
- Specificity conflicts with other styles
- Difficulty in maintaining or overriding these styles later
- Possible architectural issues in CSS organization
Consider refactoring to reduce
!importantusage by:
- Increasing selector specificity naturally (e.g.,
.markdown-content ul.markdown-list)- Reviewing conflicting styles and resolving at their source
- Using CSS cascade order more effectively
src/components/ui/ChannelListModal.tsx (2)
52-71: Consider simplifying the dual state-and-ref pattern.The custom setters that update both state and refs work correctly but add complexity. Modern React patterns typically prefer:
- Using
useCallbackwith proper dependencies and accepting re-creation of listeners, or- Using functional state updates in event handlers to access latest state without closures
This pattern is functional but may confuse future maintainers.
540-561: Prefer state-based image fallback over direct DOM manipulation.The
onErrorhandler directly manipulates the DOM by hiding the<img>and showing the next sibling. This is fragile and non-idiomatic for React. Use a state variable to track image load failures instead.Consider this pattern:
const [imageLoadError, setImageLoadError] = useState(false); // In render: {avatarUrl && !imageLoadError ? ( <img src={avatarUrl} alt={channel.channel} className="w-8 h-8 rounded-full object-cover" onError={() => setImageLoadError(true)} /> ) : ( <span className="text-gray-400 text-xl font-bold">#</span> )}Note: You'll need to track image errors per channel, perhaps in a Map or as part of the metadata cache.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/components/message/CollapsibleMessage.tsx(1 hunks)src/components/ui/ChannelListModal.tsx(4 hunks)src/index.css(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/message/CollapsibleMessage.tsx (1)
src/components/message/index.ts (1)
CollapsibleMessage(4-4)
src/components/ui/ChannelListModal.tsx (2)
src/lib/ircClient.ts (1)
listChannels(1018-1085)src/lib/ircUtils.tsx (2)
getChannelAvatarUrl(608-623)getChannelDisplayName(593-603)
🔇 Additional comments (7)
src/components/message/CollapsibleMessage.tsx (2)
22-29: Good addition: timeout cleanup on unmount.This properly addresses the previous review feedback about cleaning up the animation timeout when the component unmounts.
48-64: Timeout management properly implemented.The timeout is now correctly stored in
animationTimeoutRef, cleared before setting new timeouts, and the ref is properly cleaned up on unmount. This addresses the previous review feedback.src/index.css (3)
192-194: Verify the negative margin doesn't cause layout issues.The negative
margin-bottom: -0.0625remcould cause overlapping elements or unexpected spacing behavior. While this might be intentional to tighten spacing between markdown blocks, it's fragile and could interact poorly with other spacing rules.Test with various markdown content (headings, paragraphs, lists, code blocks) to ensure the negative margin doesn't create visual artifacts or overlapping elements.
288-410: Well-structured code block and copy button styling.The code block container, header, and copy button styles are comprehensive and include proper hover states, dark mode variants, and transition effects. The inline code copy button pattern with opacity transitions on hover is a good UX choice.
511-617: Comprehensive table and syntax highlighting styles.The table styling is clean and includes dark mode variants. The syntax highlighting color scheme for hljs is comprehensive and follows a GitHub-style dark theme with proper color assignments for all token types.
src/components/ui/ChannelListModal.tsx (2)
22-23: Good fix for the ELIST safety issue.Normalizing
elistto a safe string upfront prevents the runtime errors from unsafe optional chaining that were flagged in previous reviews.
196-224: Scroll listener logic is complex but appears correct.The disabled lint warning and ref-based state access pattern correctly avoid recreating the scroll listener on every render while preventing stale closures. The listener recreates only when
filteredChannels.lengthchanges, which is the intended behavior.
| const applyFilters = () => { | ||
| if (!selectedServerId) return; | ||
|
|
||
| const filters = { | ||
| minUsers: minUsers > 0 ? minUsers : undefined, | ||
| maxUsers: maxUsers > 0 ? maxUsers : undefined, | ||
| minCreationTime: minCreationTime > 0 ? minCreationTime : undefined, | ||
| maxCreationTime: maxCreationTime > 0 ? maxCreationTime : undefined, | ||
| minTopicTime: minTopicTime > 0 ? minTopicTime : undefined, | ||
| maxTopicTime: maxTopicTime > 0 ? maxTopicTime : undefined, | ||
| mask: mask.trim() || undefined, | ||
| notMask: notMask.trim() || undefined, | ||
| }; | ||
|
|
||
| updateChannelListFilters(selectedServerId, filters); | ||
| listChannels(selectedServerId, filters); | ||
| }; |
There was a problem hiding this comment.
Critical: Missing elist parameter in listChannels call.
The listChannels function signature expects (serverId, elist?, filters?) but Line 250 calls listChannels(selectedServerId, filters), omitting the elist parameter. This causes the filters object to be misinterpreted as the elist string, breaking server-side filtering.
Apply this diff to pass the correct parameters:
updateChannelListFilters(selectedServerId, filters);
- listChannels(selectedServerId, filters);
+ listChannels(selectedServerId, elist, filters);
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/ChannelListModal.tsx around lines 235-251, the call
listChannels(selectedServerId, filters) omits the required elist parameter so
the filters object is being passed as elist; change the call to pass the elist
as the second argument and filters as the third (e.g.
listChannels(selectedServerId, elist, filters)), ensuring you reference the
existing elist state/variable (or pass undefined explicitly if none) so
server-side filtering receives the correct arguments.
| return ( | ||
| <div className="fixed inset-0 bg-black bg-opacity-70 flex items-center justify-center z-50"> | ||
| <div className="bg-discord-dark-200 rounded-lg w-full max-w-2xl p-5 max-h-[90vh] overflow-y-auto"> | ||
| <div className="flex justify-between items-center mb-4"> | ||
| <div | ||
| className="fixed inset-0 bg-black bg-opacity-70 flex items-center justify-center z-50" | ||
| onClick={() => toggleChannelListModal(false)} | ||
| > | ||
| <div | ||
| className="bg-discord-dark-200 rounded-lg w-full max-w-2xl p-5 max-h-[90vh] flex flex-col" | ||
| onClick={(e) => e.stopPropagation()} | ||
| > |
There was a problem hiding this comment.
Add ESC key handler for keyboard accessibility.
This was flagged in previous reviews but not yet addressed. The modal only supports mouse-based dismissal (overlay click and close button). As per the past review comment, other modals in the codebase (UserContextMenu, LinkSecurityWarningModal, etc.) implement Escape key handling for accessibility.
Add this effect after the other useEffect hooks (around line 233):
// ESC key handler to close modal
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if (e.key === "Escape") {
toggleChannelListModal(false);
}
};
window.addEventListener("keydown", handleKeyDown);
return () => window.removeEventListener("keydown", handleKeyDown);
}, [toggleChannelListModal]);🤖 Prompt for AI Agents
In src/components/ui/ChannelListModal.tsx around lines 271-279 (add after the
other useEffect hooks near line 233), the modal lacks an Escape key handler for
keyboard accessibility; add a useEffect that registers a window keydown listener
which calls toggleChannelListModal(false) when e.key === "Escape", ensure you
remove the listener in the cleanup return, and include toggleChannelListModal in
the effect dependency array so the handler updates correctly.
There was a problem hiding this comment.
Indeed esc key not working for me in any modal @ValwareIRC
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| .dark .markdown-content code { | ||
| background-color: rgb(55 65 81); | ||
| background-color: rgb(31 41 55);; |
There was a problem hiding this comment.
Fix CSS syntax error: double semicolon.
Line 267 has a double semicolon at the end of the property value.
Apply this diff:
-.dark .markdown-content code {
- background-color: rgb(31 41 55);;
-}
+.dark .markdown-content code {
+ background-color: rgb(31 41 55);
+}📝 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.
| background-color: rgb(31 41 55);; | |
| background-color: rgb(31 41 55); |
🤖 Prompt for AI Agents
In src/index.css around line 267, there's a CSS syntax error: the
background-color declaration ends with a double semicolon ("background-color:
rgb(31 41 55);;"). Remove the extra semicolon so the property ends with a single
semicolon ("background-color: rgb(31 41 55);") to fix the syntax.
|
|
||
| .dark .markdown-content pre { | ||
| background-color: rgb(55 65 81); | ||
| background-color: rgb(31 41 55);; |
There was a problem hiding this comment.
Fix CSS syntax error: double semicolon.
Line 280 has a double semicolon at the end of the property value.
Apply this diff:
-.dark .markdown-content pre {
- background-color: rgb(31 41 55);;
-}
+.dark .markdown-content pre {
+ background-color: rgb(31 41 55);
+}📝 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.
| background-color: rgb(31 41 55);; | |
| .dark .markdown-content pre { | |
| background-color: rgb(31 41 55); | |
| } |
🤖 Prompt for AI Agents
In src/index.css around line 280, there's a CSS syntax error: the property
"background-color: rgb(31 41 55);;" contains a double semicolon; remove the
extra semicolon so the declaration ends with a single semicolon
("background-color: rgb(31 41 55);") to fix the syntax.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/layout/ChatHeader.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/layout/ChatHeader.tsx (2)
src/store/index.ts (1)
loadSavedMetadata(113-115)src/types/index.ts (2)
PrivateChat(90-106)User(1-17)
🔇 Additional comments (2)
src/components/layout/ChatHeader.tsx (2)
7-7: LGTM!The
FaCheckCircleimport is correctly added and used for rendering verification badges.
449-500: The badge rendering asymmetry is intentional and correct.The search results confirm this is deliberate UX design, consistently implemented across the codebase:
- The test case
MetadataDisplay.test.tsxexpects Alice Wonderland to display with username badge "alice" below it- Comments in
ChatHeader.tsx,ChannelList.tsx, andMemberList.tsxall document the pattern: "Only show verified badge if NO display-name"- When a display-name exists, the verified badge moves from the title to the username badge context to reduce visual clutter
- When no display-name exists, the verified badge appears next to the username in the title
This asymmetry is not a bug—it's consistent badge placement strategy across all components that avoids cluttering the primary display when a display-name is available.
| // Helper function to get user metadata | ||
| const getUserMetadata = (username: string) => { | ||
| if (!selectedServerId) return null; | ||
|
|
||
| // First check localStorage for saved metadata | ||
| const savedMetadata = loadSavedMetadata(); | ||
| const serverMetadata = savedMetadata[selectedServerId]; | ||
| if (serverMetadata?.[username]) { | ||
| return serverMetadata[username]; | ||
| } | ||
|
|
||
| // If not in localStorage, check if user is in any shared channels | ||
| const server = servers.find((s) => s.id === selectedServerId); | ||
| if (!server) return null; | ||
|
|
||
| // Search through all channels for this user | ||
| for (const channel of server.channels) { | ||
| const user = channel.users.find( | ||
| (u) => u.username.toLowerCase() === username.toLowerCase(), | ||
| ); | ||
| if (user?.metadata && Object.keys(user.metadata).length > 0) { | ||
| return user.metadata; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate code duplication and improve performance.
This function duplicates the logic in the privateChatUserMetadata useMemo (lines 90-117). Additionally, since it's called during render (lines 450, 472) without memoization, it recalculates on every render.
Consider refactoring to a single memoized implementation:
+ // Get user metadata - memoized lookup for any username
+ const getUserMetadata = useMemo(() => {
+ return (username: string) => {
+ if (!selectedServerId) return null;
+
+ // First check localStorage for saved metadata
+ const savedMetadata = loadSavedMetadata();
+ const serverMetadata = savedMetadata[selectedServerId];
+ if (serverMetadata?.[username]) {
+ return serverMetadata[username];
+ }
+
+ // If not in localStorage, check if user is in any shared channels
+ const server = servers.find((s) => s.id === selectedServerId);
+ if (!server) return null;
+
+ // Search through all channels for this user
+ for (const channel of server.channels) {
+ const user = channel.users.find(
+ (u) => u.username.toLowerCase() === username.toLowerCase(),
+ );
+ if (user?.metadata && Object.keys(user.metadata).length > 0) {
+ return user.metadata;
+ }
+ }
+
+ return null;
+ };
+ }, [selectedServerId, servers]);
+
- // Get private chat user metadata - first check localStorage, then check shared channels
- const privateChatUserMetadata = useMemo(() => {
- if (!selectedPrivateChat || !selectedServerId) return null;
-
- // First check localStorage for saved metadata
- const savedMetadata = loadSavedMetadata();
- const serverMetadata = savedMetadata[selectedServerId];
- if (serverMetadata?.[selectedPrivateChat.username]) {
- return serverMetadata[selectedPrivateChat.username];
- }
-
- // If not in localStorage, check if user is in any shared channels
- const server = servers.find((s) => s.id === selectedServerId);
- if (!server) return null;
-
- // Search through all channels for this user
- for (const channel of server.channels) {
- const user = channel.users.find(
- (u) =>
- u.username.toLowerCase() ===
- selectedPrivateChat.username.toLowerCase(),
- );
- if (user?.metadata && Object.keys(user.metadata).length > 0) {
- return user.metadata;
- }
- }
-
- return null;
- }, [selectedPrivateChat, selectedServerId, servers]);
+ const privateChatUserMetadata = selectedPrivateChat
+ ? getUserMetadata(selectedPrivateChat.username)
+ : null;
-
- // Helper function to get user metadata
- const getUserMetadata = (username: string) => {
- if (!selectedServerId) return null;
-
- // First check localStorage for saved metadata
- const savedMetadata = loadSavedMetadata();
- const serverMetadata = savedMetadata[selectedServerId];
- if (serverMetadata?.[username]) {
- return serverMetadata[username];
- }
-
- // If not in localStorage, check if user is in any shared channels
- const server = servers.find((s) => s.id === selectedServerId);
- if (!server) return null;
-
- // Search through all channels for this user
- for (const channel of server.channels) {
- const user = channel.users.find(
- (u) => u.username.toLowerCase() === username.toLowerCase(),
- );
- if (user?.metadata && Object.keys(user.metadata).length > 0) {
- return user.metadata;
- }
- }
-
- return null;
- };Committable suggestion skipped: line range outside the PR's diff.
| // Helper function to get full user object from shared channels | ||
| const getUserFromChannels = (username: string) => { | ||
| const server = servers.find((s) => s.id === selectedServerId); | ||
| if (!server) return null; | ||
|
|
||
| // Search through all channels for this user | ||
| for (const channel of server.channels) { | ||
| const user = channel.users.find( | ||
| (u) => u.username.toLowerCase() === username.toLowerCase(), | ||
| ); | ||
| if (user) { | ||
| return user; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Memoize to avoid repeated channel scans on every render.
This function is called during render (lines 454, 476) and performs an O(n×m) search through all channels and users. Without memoization, it recalculates on every render.
Apply this diff to memoize the function:
- // Helper function to get full user object from shared channels
- const getUserFromChannels = (username: string) => {
- const server = servers.find((s) => s.id === selectedServerId);
- if (!server) return null;
-
- // Search through all channels for this user
- for (const channel of server.channels) {
- const user = channel.users.find(
- (u) => u.username.toLowerCase() === username.toLowerCase(),
- );
- if (user) {
- return user;
- }
- }
-
- return null;
- };
+ // Helper function to get full user object from shared channels
+ const getUserFromChannels = useMemo(() => {
+ return (username: string) => {
+ const server = servers.find((s) => s.id === selectedServerId);
+ if (!server) return null;
+
+ // Search through all channels for this user
+ for (const channel of server.channels) {
+ const user = channel.users.find(
+ (u) => u.username.toLowerCase() === username.toLowerCase(),
+ );
+ if (user) {
+ return user;
+ }
+ }
+
+ return null;
+ };
+ }, [selectedServerId, servers]);📝 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.
| // Helper function to get full user object from shared channels | |
| const getUserFromChannels = (username: string) => { | |
| const server = servers.find((s) => s.id === selectedServerId); | |
| if (!server) return null; | |
| // Search through all channels for this user | |
| for (const channel of server.channels) { | |
| const user = channel.users.find( | |
| (u) => u.username.toLowerCase() === username.toLowerCase(), | |
| ); | |
| if (user) { | |
| return user; | |
| } | |
| } | |
| return null; | |
| }; | |
| // Helper function to get full user object from shared channels | |
| const getUserFromChannels = useMemo(() => { | |
| return (username: string) => { | |
| const server = servers.find((s) => s.id === selectedServerId); | |
| if (!server) return null; | |
| // Search through all channels for this user | |
| for (const channel of server.channels) { | |
| const user = channel.users.find( | |
| (u) => u.username.toLowerCase() === username.toLowerCase(), | |
| ); | |
| if (user) { | |
| return user; | |
| } | |
| } | |
| return null; | |
| }; | |
| }, [selectedServerId, servers]); |
🤖 Prompt for AI Agents
In src/components/layout/ChatHeader.tsx around lines 147 to 163, the
getUserFromChannels helper does an O(n×m) scan on every render; replace it with
a memoized lookup: use React.useMemo to build a Map (key =
username.toLowerCase(), value = user) for the currently selected server
(dependencies: servers and selectedServerId) and then expose getUserFromChannels
as a small wrapper that does a constant-time map.get(username.toLowerCase());
ensure useMemo is imported from React and update dependencies so the map is
rebuilt only when servers or selectedServerId change.
| // Helper function to render verification and bot badges | ||
| const renderUserBadges = ( | ||
| username: string, | ||
| privateChat: PrivateChat | undefined, | ||
| user: User | null, | ||
| showVerified = true, | ||
| ) => { | ||
| // Get account and bot info from privateChat first, fall back to channel user | ||
| const account = privateChat?.account || user?.account; | ||
| const isBot = | ||
| privateChat?.isBot || | ||
| user?.isBot || | ||
| user?.metadata?.bot?.value === "true"; | ||
| const isIrcOp = user?.isIrcOp || false; | ||
|
|
||
| const isVerified = | ||
| showVerified && | ||
| account && | ||
| account !== "0" && | ||
| username.toLowerCase() === account.toLowerCase(); | ||
|
|
||
| if (!isVerified && !isBot && !isIrcOp) return null; | ||
|
|
||
| return ( | ||
| <> | ||
| {isVerified && ( | ||
| <FaCheckCircle | ||
| className="inline ml-0.5 text-green-500" | ||
| style={{ fontSize: "0.75em", verticalAlign: "baseline" }} | ||
| title="Verified account" | ||
| /> | ||
| )} | ||
| {isBot && ( | ||
| <span | ||
| className="inline ml-0.5" | ||
| style={{ fontSize: "0.9em" }} | ||
| title="Bot" | ||
| > | ||
| 🤖 | ||
| </span> | ||
| )} | ||
| {isIrcOp && ( | ||
| <span | ||
| className="inline ml-0.5" | ||
| style={{ fontSize: "0.9em" }} | ||
| title="IRC Operator" | ||
| > | ||
| 🔑 | ||
| </span> | ||
| )} | ||
| </> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Inconsistent handling of IRC operator status.
Line 178 only checks user?.isIrcOp, unlike account (line 173) and isBot (lines 174-177) which check both privateChat and user. The PrivateChat interface includes an isIrcOp field, so it should be checked first.
Apply this diff to maintain consistency:
- const isIrcOp = user?.isIrcOp || false;
+ const isIrcOp = privateChat?.isIrcOp || user?.isIrcOp || false;📝 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.
| // Helper function to render verification and bot badges | |
| const renderUserBadges = ( | |
| username: string, | |
| privateChat: PrivateChat | undefined, | |
| user: User | null, | |
| showVerified = true, | |
| ) => { | |
| // Get account and bot info from privateChat first, fall back to channel user | |
| const account = privateChat?.account || user?.account; | |
| const isBot = | |
| privateChat?.isBot || | |
| user?.isBot || | |
| user?.metadata?.bot?.value === "true"; | |
| const isIrcOp = user?.isIrcOp || false; | |
| const isVerified = | |
| showVerified && | |
| account && | |
| account !== "0" && | |
| username.toLowerCase() === account.toLowerCase(); | |
| if (!isVerified && !isBot && !isIrcOp) return null; | |
| return ( | |
| <> | |
| {isVerified && ( | |
| <FaCheckCircle | |
| className="inline ml-0.5 text-green-500" | |
| style={{ fontSize: "0.75em", verticalAlign: "baseline" }} | |
| title="Verified account" | |
| /> | |
| )} | |
| {isBot && ( | |
| <span | |
| className="inline ml-0.5" | |
| style={{ fontSize: "0.9em" }} | |
| title="Bot" | |
| > | |
| 🤖 | |
| </span> | |
| )} | |
| {isIrcOp && ( | |
| <span | |
| className="inline ml-0.5" | |
| style={{ fontSize: "0.9em" }} | |
| title="IRC Operator" | |
| > | |
| 🔑 | |
| </span> | |
| )} | |
| </> | |
| ); | |
| }; | |
| // Helper function to render verification and bot badges | |
| const renderUserBadges = ( | |
| username: string, | |
| privateChat: PrivateChat | undefined, | |
| user: User | null, | |
| showVerified = true, | |
| ) => { | |
| // Get account and bot info from privateChat first, fall back to channel user | |
| const account = privateChat?.account || user?.account; | |
| const isBot = | |
| privateChat?.isBot || | |
| user?.isBot || | |
| user?.metadata?.bot?.value === "true"; | |
| const isIrcOp = privateChat?.isIrcOp || user?.isIrcOp || false; | |
| const isVerified = | |
| showVerified && | |
| account && | |
| account !== "0" && | |
| username.toLowerCase() === account.toLowerCase(); | |
| if (!isVerified && !isBot && !isIrcOp) return null; | |
| return ( | |
| <> | |
| {isVerified && ( | |
| <FaCheckCircle | |
| className="inline ml-0.5 text-green-500" | |
| style={{ fontSize: "0.75em", verticalAlign: "baseline" }} | |
| title="Verified account" | |
| /> | |
| )} | |
| {isBot && ( | |
| <span | |
| className="inline ml-0.5" | |
| style={{ fontSize: "0.9em" }} | |
| title="Bot" | |
| > | |
| 🤖 | |
| </span> | |
| )} | |
| {isIrcOp && ( | |
| <span | |
| className="inline ml-0.5" | |
| style={{ fontSize: "0.9em" }} | |
| title="IRC Operator" | |
| > | |
| 🔑 | |
| </span> | |
| )} | |
| </> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/components/layout/ChatHeader.tsx around lines 165 to 217, the isIrcOp
check currently only reads user?.isIrcOp; change it to read privateChat?.isIrcOp
|| user?.isIrcOp (consistent with account and isBot lookups) so IRC operator
status is derived from the privateChat first and falls back to the user; update
the isIrcOp variable assignment accordingly.
I think it is hard to understand that this signifies numbers of users, I thought was mentions or unread messages (the number in the right). Maybe could be a number over an icon?
|
|
regression on URL highlights not working anymore |
|
The scrolling keeping doesn't work for me in chrome and I dont see the arrow down button that used to happen, but this is gone since long ago. Also in anonymous mode so pretty weird. |
|
classic highlight.js issues with symbols, or maybe is just the tag escapting idk 
|
There was a problem hiding this comment.
neat that you tested!
| }, [filteredChannels.length, setDisplayedChannelsCount, setLoadingMore]); // Only depend on filteredChannels.length to avoid recreating listener too often | ||
|
|
||
| // Reset displayed count when filtered channels change | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
I think you can update react and use useEffectEvent for this now. https://react.dev/reference/react/useEffectEvent
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/ircUtils.tsx (1)
236-250: Escapehrefbefore injecting into HTML.
hrefcan be attacker-controlled (e.g.[label](http://example.com" onmouseover="alert(1))) and we interpolate it directly into the anchor attribute. Because we don’t HTML-escape quotes after sanitizing the protocol, an attacker can break out of the attribute and run script even though we removedjavascript:. Please HTML-escape the value (including quotes) and guard against falsyhrefbefore calling.replace. (docs.devnetexperttraining.com)- renderer.link = ({ href, title, text }) => { - // Sanitize URL to prevent javascript: and other malicious protocols - const sanitizedHref = href.replace(/^(javascript|data|vbscript):/i, "#"); + const escapeHtmlAttr = (value: string) => + value + .replace(/&/g, "&") + .replace(/"/g, """) + .replace(/'/g, "'"); + + renderer.link = ({ href, title, text }) => { + if (!href) { + return text ?? ""; + } + + // Sanitize URL to prevent javascript: and other malicious protocols + const sanitizedHref = href.replace(/^(javascript|data|vbscript):/i, "#"); + const escapedHref = escapeHtmlAttr(sanitizedHref); @@ - return `<a href="${sanitizedHref}"${titleAttr} target="_blank" rel="noopener noreferrer" class="${linkClass}">${text}</a>`; + return `<a href="${escapedHref}"${titleAttr} target="_blank" rel="noopener noreferrer" class="${linkClass}">${text}</a>`; };src/components/layout/ChannelList.tsx (1)
1279-1348: Verify nested click handler intent.The settings button at line 1344 and its parent container at line 1279 both call
toggleUserProfileModal(true). This creates nested click handlers with identical behavior. If intentional (providing both a large clickable area and an explicit button), this is fine. Otherwise, consider removing one handler to simplify the interaction model.
♻️ Duplicate comments (1)
src/store/index.ts (1)
5065-5073: Guardversionbefore calling.includes().This handler still calls
version.includes(...)without confirmingversionis a string, so the same TypeError reported earlier will reappear if the server omits the field. Please add the guard you already discussed.- const isUnrealIRCd = version.includes("UnrealIRCd"); + const ver = typeof version === "string" ? version : ""; + const isUnrealIRCd = ver.includes("UnrealIRCd");
🧹 Nitpick comments (1)
src/components/message/MessageItem.tsx (1)
573-611: Consider consolidating duplicate multiline logic.The multiline message handling logic appears twice: first in
getMessageContent()(lines 574-589) to aggregate content, and again at lines 600-611 for early return. Consider extracting the "is first message in group" check into a helper function to reduce duplication and improve maintainability.Example consolidation:
const isFirstInMultilineGroup = () => { if (!message.multilineMessageIds || message.multilineMessageIds.length === 0) { return true; } const multilineMessages = channelMessages.filter((m) => message.multilineMessageIds?.includes(m.id), ); multilineMessages.sort( (a, b) => new Date(a.timestamp).getTime() - new Date(b.timestamp).getTime(), ); return multilineMessages[0]?.id === message.id; }; const getMessageContent = () => { if (message.multilineMessageIds && message.multilineMessageIds.length > 0) { const multilineMessages = channelMessages.filter((m) => message.multilineMessageIds?.includes(m.id), ); multilineMessages.sort( (a, b) => new Date(a.timestamp).getTime() - new Date(b.timestamp).getTime(), ); if (!isFirstInMultilineGroup()) { return ""; } return multilineMessages.map((m) => m.content).join("\n"); } return message.content; }; // Early return simplified if (!isFirstInMultilineGroup()) { return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/layout/ChannelList.tsx(2 hunks)src/components/layout/MemberList.tsx(1 hunks)src/components/message/MessageItem.tsx(2 hunks)src/components/message/MessageReply.tsx(1 hunks)src/components/message/SystemMessage.tsx(1 hunks)src/components/message/WhisperMessage.tsx(1 hunks)src/components/ui/StandardReplyNotification.tsx(1 hunks)src/lib/ircUtils.tsx(9 hunks)src/store/index.ts(12 hunks)tests/lib/messageFormatter.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/lib/messageFormatter.test.ts
🧰 Additional context used
🧬 Code graph analysis (9)
src/components/message/WhisperMessage.tsx (1)
src/lib/ircUtils.tsx (1)
processMarkdownInText(417-451)
src/components/layout/MemberList.tsx (1)
src/lib/ircUtils.tsx (1)
processMarkdownInText(417-451)
src/components/message/SystemMessage.tsx (1)
src/lib/ircUtils.tsx (1)
processMarkdownInText(417-451)
src/components/message/MessageReply.tsx (1)
src/lib/ircUtils.tsx (1)
processMarkdownInText(417-451)
src/components/message/MessageItem.tsx (5)
src/store/index.ts (1)
loadSavedMetadata(113-115)src/types/index.ts (1)
User(1-17)src/lib/ircUtils.tsx (1)
processMarkdownInText(417-451)src/components/message/CollapsibleMessage.tsx (1)
CollapsibleMessage(9-108)src/components/message/MessageReply.tsx (1)
MessageReply(14-51)
src/components/ui/StandardReplyNotification.tsx (1)
src/lib/ircUtils.tsx (1)
processMarkdownInText(417-451)
src/store/index.ts (2)
src/lib/ircClient.ts (1)
ircClient(2728-2728)src/lib/ircUtils.tsx (1)
isUnrealIRCd(774-776)
src/lib/ircUtils.tsx (1)
src/types/index.ts (1)
Server(25-48)
src/components/layout/ChannelList.tsx (1)
src/lib/ircUtils.tsx (1)
processMarkdownInText(417-451)
🪛 ast-grep (0.39.6)
src/lib/ircUtils.tsx
[warning] 364-364: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🔇 Additional comments (10)
src/components/layout/ChannelList.tsx (1)
1077-1082: LGTM! Enhanced markdown processing with unique key.The updated
processMarkdownInTextcall correctly passes explicit parameters and a unique key derived from the private chat ID, ensuring stable rendering and proper React reconciliation.src/components/message/SystemMessage.tsx (1)
22-27: LGTM! Consistent markdown processing with fallback.The updated call includes a proper fallback chain for the key prefix (
message.id || message.msgid || "system"), ensuring stable keys even when message identifiers are missing.src/components/layout/MemberList.tsx (2)
271-276: LGTM! Unique key for realname rendering.The key prefix
member-${user.id}-realnameensures each user's realname has a unique, stable identifier for React rendering.
284-289: LGTM! Unique key for status rendering.The key prefix
member-${user.id}-statusappropriately distinguishes status rendering from realname rendering while maintaining per-user uniqueness.src/components/ui/StandardReplyNotification.tsx (1)
71-76: LGTM! Composite key ensures uniqueness.The key prefix combines command, code, and timestamp to create a unique identifier for each standard reply notification. Since the timestamp is passed as a prop and derived from the message, it remains stable across re-renders.
src/components/message/MessageReply.tsx (1)
42-47: LGTM! Reply-specific key with fallback.The key prefix
reply-${replyMessage.id || replyMessage.msgid || "unknown"}appropriately namespaces reply content rendering and includes a fallback for edge cases.src/components/message/WhisperMessage.tsx (1)
58-63: LGTM! Whisper-specific key with fallback.The key derivation follows the established pattern with an appropriate "whisper" fallback for cases where message identifiers are unavailable.
src/components/message/MessageItem.tsx (3)
352-395: Helper functions for metadata retrieval.The
getUserMetadataandgetUserFromChannelshelpers useuseStore.getState()for non-reactive state access. This is appropriate here since these helpers are called within reactive contexts where updates are already tracked (e.g., within selectors or effects).
397-520: Refactored user resolution with reactive selectors.The refactoring introduces clear separation between user key derivation (lines 422-464) and user data resolution (lines 466-520), with proper reactive tracking via
useStoreselectors. ThepmUserMetadataselector includesmetadataChangeCounterto ensure reactivity for metadata updates.
614-622: LGTM! CollapsibleMessage integration.The integration of
CollapsibleMessagewrapping the processed markdown content is clean and follows a clear separation of concerns. ThehtmlContentprocessing and wrapping inCollapsibleMessageprovides the collapsible behavior mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lib/ircUtils.tsx (4)
263-263: Use crypto.randomUUID() or a counter for code IDs.
Math.random().toString(36).substr(2, 9)can produce collisions (low probability but nonzero), which would break copy-to-clipboard targeting if two code blocks receive the same ID on a single page render.Apply this diff to use a counter-based ID generator:
+let codeIdCounter = 0; + renderer.codespan = ({ text }) => { // Decode HTML entities back to characters for inline code const decodedText = text .replace(/</g, "<") .replace(/>/g, ">") .replace(/&/g, "&") .replace(/"/g, '"') .replace(/'/g, "'") .replace(/'/g, "'"); - const codeId = `inline-code-${Math.random().toString(36).substr(2, 9)}`; + const codeId = `inline-code-${codeIdCounter++}`; return `<span class="inline-code-container"><code id="${codeId}" class="inline-code">${decodedText}</code><button class="inline-copy-button" data-code-id="${codeId}" title="Copy to clipboard">
272-322: Extract entity decoding into a shared helper.The HTML entity decoding logic (lines 278-284) is duplicated from the codespan renderer (lines 255-261). Extract this into a helper function to follow the DRY principle and ensure consistency if the entity list needs updating.
Apply this diff:
+// Helper to decode HTML entities in code content +function decodeCodeEntities(text: string): string { + return text + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/&/g, "&") + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/'/g, "'"); +} + renderer.codespan = ({ text }) => { - const decodedText = text - .replace(/</g, "<") - .replace(/>/g, ">") - .replace(/&/g, "&") - .replace(/"/g, '"') - .replace(/'/g, "'") - .replace(/'/g, "'"); + const decodedText = decodeCodeEntities(text); // ... rest of codespan }; renderer.code = ({ text, lang }) => { const trimmedText = text.trimEnd(); - const decodedText = trimmedText - .replace(/</g, "<") - .replace(/>/g, ">") - .replace(/&/g, "&") - .replace(/"/g, '"') - .replace(/'/g, "'") - .replace(/'/g, "'"); + const decodedText = decodeCodeEntities(trimmedText); // ... rest of code };
379-425: Add feature detection for Clipboard API.The Clipboard API (
navigator.clipboard) is only available in secure contexts (HTTPS or localhost). If the application is accessed over HTTP, the copy functionality will silently fail with a console error. Consider adding feature detection and gracefully degrading to a fallback (e.g., showing the code text for manual copy, or a message that HTTPS is required).Apply this diff to add a check:
onClick={(e) => { const target = e.target as HTMLElement; const button = (target.closest(".copy-button") as HTMLButtonElement) || (target.closest(".inline-copy-button") as HTMLButtonElement); if (button) { const codeId = button.getAttribute("data-code-id"); if (codeId) { const codeElement = document.getElementById(codeId); if (codeElement) { const textToCopy = codeElement.textContent || ""; + if (!navigator.clipboard) { + console.warn("Clipboard API not available. Copy feature disabled."); + return; + } navigator.clipboard .writeText(textToCopy)
782-789: Consider whether this helper is necessary.The function simply returns
server.isUnrealIRCd === true. While it provides documentation and a stable API, you could inline this check at call sites (if (server.isUnrealIRCd)) unless you anticipate more complex detection logic in the future.If you prefer to keep a function for future extensibility (e.g., fallback detection via other server properties), this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/ircUtils.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/ircUtils.tsx (1)
src/types/index.ts (1)
Server(25-48)
🪛 ast-grep (0.39.6)
src/lib/ircUtils.tsx
[warning] 377-377: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🔇 Additional comments (6)
src/lib/ircUtils.tsx (6)
221-232: LGTM: Image renderer enhancement.The clickable image wrapper and external-link-security class improve UX and enable consistent link security handling.
332-338: Clarify the single-line tilde fence escaping logic.The regex
/^~~~.*~~~$/gmescapes single-line tilde blocks (e.g.,~~~js code~~~) so they render as literal text. However, multi-line tilde fences (~~~\ncode\n~~~) are still parsed as code blocks (pattern on line 444). This creates inconsistent behavior:
- Single-line:
~~~code~~~→ renders as text~~~code~~~- Multi-line:
~~~\ncode\n~~~→ renders as code blockIs this intentional? If single-line tilde blocks should render as code, remove the escaping. If they should render as text, document why they differ from multi-line blocks.
430-464: LGTM: Markdown detection enhanced.The addition of
keyPrefixfor unique React keys and the tilde fence patterns for code blocks and strikethrough improve the markdown detection logic.
1-1: No action required—highlight.js version is correctly specified.The package.json correctly specifies
^11.11.1, which matches the latest stable version (as of Dec 2024). The dependency is up-to-date and properly configured.
194-428: Sanitization strategy is sound and comprehensive for dangerouslySetInnerHTML usage.The approach is secure:
- HTML tags are escaped before parsing (line 341:
<→<,>→>)- URLs are sanitized to remove dangerous protocols (lines 215, 238)
markedis configured with safe options- Code content is decoded for display but remains within
<code>elements where script execution is preventedNo additional changes required.
252-270: Now let me check if there's an interaction issue with highlight.js:Based on my verification, I can now provide the rewritten review comment:
Fix HTML entity decoding order in code renderers to follow best practices.
The entity decoding order violates standard security practices. When escaping/unescaping HTML entities, the ampersand (&) must be decoded last. Currently,
&is decoded in the middle (after<and>), which is fragile and non-standard.Additionally, the entity decoder handles only six entities (
<,>,&,",','). While this covers what marked produces by default, marked encodes quotes, ampersands, and other characters into HTML entities. Consider using thehtml-entitiespackage (already in dependencies) instead of manual decoding.Recommended fixes:
- For inline code (line 255-261): Reorder decoding to handle
&last- For both renderers: Replace manual entity decoding with the
html-entitieslibrary'sdecode()function to handle all entities correctly- Consider whether pre-escaping on line 334 is necessary if marked already handles escaping
| // Helper function to detect and render URLs in text | ||
| function processUrlsInText( | ||
| text: string, | ||
| style?: React.CSSProperties, | ||
| keyPrefix = "", | ||
| elementIndexRef?: { current: number }, | ||
| ): React.ReactNode[] { | ||
| // URL regex pattern - matches http://, https://, and www. URLs | ||
| const urlRegex = | ||
| /(https?:\/\/[^\s<>"{}|\\^`[\]]+|www\.[^\s<>"{}|\\^`[\]]+)/gi; | ||
|
|
||
| const parts: React.ReactNode[] = []; | ||
| let lastIndex = 0; | ||
| let elementIndex = elementIndexRef ? elementIndexRef.current : 0; | ||
| let match: RegExpExecArray | null = urlRegex.exec(text); | ||
|
|
||
| while (match !== null) { | ||
| // Add text before the URL | ||
| if (match.index > lastIndex) { | ||
| parts.push( | ||
| <span key={`${keyPrefix}text-${elementIndex++}`} style={style}> | ||
| {text.slice(lastIndex, match.index)} | ||
| </span>, | ||
| ); | ||
| } | ||
|
|
||
| const url = match[0]; | ||
| // Ensure URL has protocol | ||
| const fullUrl = url.startsWith("http") ? url : `https://${url}`; | ||
|
|
||
| // Truncate long URLs for display | ||
| const displayText = url.length > 50 ? `${url.slice(0, 47)}...` : url; | ||
|
|
||
| parts.push( | ||
| <a | ||
| key={`${keyPrefix}url-${elementIndex++}`} | ||
| href={fullUrl} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="text-blue-500 hover:text-blue-700 underline" | ||
| style={style} | ||
| title={url} | ||
| > | ||
| {displayText} | ||
| </a>, | ||
| ); | ||
|
|
||
| lastIndex = match.index + match[0].length; | ||
| match = urlRegex.exec(text); | ||
| } | ||
|
|
||
| // Add remaining text | ||
| if (lastIndex < text.length) { | ||
| parts.push( | ||
| <span key={`${keyPrefix}text-${elementIndex++}`} style={style}> | ||
| {text.slice(lastIndex)} | ||
| </span>, | ||
| ); | ||
| } | ||
|
|
||
| // Update the shared elementIndex if provided | ||
| if (elementIndexRef) { | ||
| elementIndexRef.current = elementIndex; | ||
| } | ||
|
|
||
| return parts.length > 0 | ||
| ? parts | ||
| : [ | ||
| <span | ||
| key={`${keyPrefix}text-${elementIndexRef ? elementIndexRef.current++ : 0}`} | ||
| style={style} | ||
| > | ||
| {text} | ||
| </span>, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Verify the URL regex and address user-reported URL highlighting regression.
A user reported that "URL highlights/styling no longer work as expected" (PR comments). The URL regex on line 611 may have escaping issues:
/(https?:\/\/[^\s<>"{}|\\^`[\]]+|www\.[^\s<>"{}|\\^`[\]]+)/giThe character class [^\s<>"{}|\\^[]]might not correctly escape the closing]`. Depending on the JavaScript regex parser, this could:
- Fail to match URLs with certain characters
- Include unintended characters in matches
- Cause unexpected truncation of URLs
Additionally, the regex doesn't handle trailing punctuation (e.g., "Check https://example.com." includes the period in the URL).
Run the following script to test the regex behavior:
#!/bin/bash
# Test URL regex matching behavior
node -e "
const urlRegex = /(https?:\/\/[^\s<>\"{}|\\\\^\`[\]]+|www\.[^\s<>\"{}|\\\\^\`[\]]+)/gi;
const tests = [
'Check out https://example.com.',
'Visit https://example.com/path?query=1',
'See https://example.com/page[1]',
'Link: www.example.com',
'Multiple: https://a.com and https://b.com'
];
tests.forEach(test => {
const matches = test.match(urlRegex);
console.log('Input:', test);
console.log('Matches:', matches);
console.log('---');
});
"Consider using a well-tested URL regex library or improving the pattern:
- const urlRegex =
- /(https?:\/\/[^\s<>"{}|\\^`[\]]+|www\.[^\s<>"{}|\\^`[\]]+)/gi;
+ // Improved regex that properly escapes brackets and handles trailing punctuation
+ const urlRegex =
+ /(https?:\/\/[^\s<>"{}|\\^`\[\]]+[\w/]|www\.[^\s<>"{}|\\^`\[\]]+[\w/])/gi;🤖 Prompt for AI Agents
In src/lib/ircUtils.tsx around lines 602–677, the URL regex has
incorrect/insufficient escaping for the closing bracket/backtick/backslash class
and also permits trailing punctuation (e.g., trailing period/comma) to be
captured; update the regex so the character class correctly escapes the closing
square bracket and backtick (for example ensure the class is something like
[^\s<>"{}|\\^`\[\]] or equivalent valid JS escape order) and either add a
lookahead to exclude terminal punctuation or post-process each regex match to
strip trailing punctuation characters (.,:;!?)]), then re-run the provided tests
and add a unit test covering cases like URL followed by punctuation, bracketed
URLs, query strings, and multiple URLs in one string to verify highlighting is
restored.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/ChannelListModal.tsx (1)
271-619: Major: ESC key handler still missing despite previous fix.Although a previous review (marked as addressed in commit ec1d37a) flagged the missing ESC key handler for keyboard accessibility, the current code shows no
useEffectwith a keydown listener. matheusfillipe confirms "Indeed esc key not working for me in any modal". Other modals in the codebase (UserContextMenu, LinkSecurityWarningModal, etc.) implement this pattern.Add this effect after the existing
useEffecthooks (around line 233):// ESC key handler to close modal useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { if (e.key === "Escape") { toggleChannelListModal(false); } }; window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); }, [toggleChannelListModal]);Based on learnings
♻️ Duplicate comments (1)
src/components/ui/ChannelListModal.tsx (1)
235-251: Critical: Missingelistparameter breaks server-side filtering.Line 250 calls
listChannels(selectedServerId, filters), but the signature islistChannels(serverId, elist?, filters?). Thefiltersobject is being passed as theelistparameter, breaking server-side filtering. This issue was flagged in previous reviews but remains unaddressed.Apply this diff:
updateChannelListFilters(selectedServerId, filters); - listChannels(selectedServerId, filters); + listChannels(selectedServerId, elist, filters); };
🧹 Nitpick comments (1)
src/components/ui/ChannelListModal.tsx (1)
52-71: Consider migrating touseEffectEventwhen stable.The custom setter pattern (updating both state and ref) avoids stale closures in the scroll handler, but React's upcoming
useEffectEventhook would eliminate this workaround. As matheusfillipe noted at line 227, you can now useuseEffectEventif you've updated to a React version that supports it.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/ChannelListModal.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/ChannelListModal.tsx (2)
src/lib/ircClient.ts (1)
listChannels(1018-1085)src/lib/ircUtils.tsx (2)
getChannelAvatarUrl(721-736)getChannelDisplayName(706-716)
🔇 Additional comments (5)
src/components/ui/ChannelListModal.tsx (5)
14-17: LGTM: Store integration and safe elist handling.The new store properties enable per-server filtering state, and line 23 safely normalizes
elistto prevent runtime errors from undefined optional chaining (as flagged in previous reviews).Also applies to: 23-23
196-224: LGTM: Lazy loading implementation.The scroll-based lazy loading correctly uses refs to avoid recreating the event listener on every render. The eslint-disable is intentional and appropriate here. The 50-item incremental loading with a 200ms delay provides smooth UX.
320-495: LGTM: Advanced filters UI respects server capabilities.The filter panel correctly conditionally renders based on ELIST capability flags (U, C, T, M, N) and provides a clear fallback message when the server doesn't support advanced filtering. Once the critical issue at line 250 is fixed, this will integrate properly with server-side filtering.
522-599: LGTM: Metadata-driven channel rendering.The channel item rendering correctly integrates metadata-driven avatars and display names with appropriate fallbacks. The avatar error handling and conditional badge display (showing original channel name when display name differs) enhance the UX.
290-290: LGTM: Accessibility improvement.The
aria-label="Close"attribute enhances screen reader accessibility for the close button.
* Fix scroll position preservation in chat - Preserve scroll position when user scrolls up to read history - Auto-scroll only when user is already at bottom - Add wasAtBottomRef to track scroll state before new messages arrive - Fix timing issue where scroll position was checked after DOM updates * lint * UI sidebar width fix in User Settings modal mobile view * Fix a test * Fix tests some more * Fix tests some more * feat: enhance channel list modal with user count sorting and improved UX - Default sort channels by user count (highest first) - Display user count as purple badge matching selected server tab color - Click overlay to close modal - Make channels list scrollable while keeping filter/sort options fixed - Fix modal layout structure for proper scrolling behavior * /LIST improvements * Implement lazy-loading on channels lists in chunks of 50, dynamic advanced filtering using ELIST ISUPPORT * Make markdown rendering more beautiful and practical * Collapsible/Expandable multiline messages * Center see more button * add a hs divider around the "show more button" to be clearer about the multiline cutoff point * Improvements on package.json and regarding ELIST possibility to not exist * deduplicate +typing=done tags * Show display-names in PM windows in the ChatHeader * Fix metadata sync issue, bug added by me earlier * Fix > and < rendering in markdown codeblocks * Fix channel listing headers, be clear about channel name and users property * Be more specific that the number in the channels list is users
2 participants
This PR fixes the scroll position issue in chat windows where new messages would always scroll to the bottom, even when users had scrolled up to read previous messages.
Changes
Testing
Fixes the issue where scrolling up in a channel would be interrupted by new messages automatically scrolling to bottom.
Summary by CodeRabbit
New Features
Bug Fixes
UI
Tests