feat/bugfixes and refactor#164
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors IRC subsystem into a modular IRCClient with a dispatch/handler layer, wires many IRC events into new Zustand store handlers, consolidates media preview settings and probing, adds mobile haptics and platform helpers, and updates many message/UI components and hooks. Documentation and dependency updates included. Changes
Sequence Diagram(s)sequenceDiagram
participant IRC as IRC Server
participant WS as WebSocket
participant Client as IRCClient
participant Dispatch as IRC_DISPATCH
participant Handler as Handler
participant Store as ZustandStore
IRC->>WS: raw IRC lines (CRLF)
WS->>Client: onmessage(raw)
Client->>Client: parse tags, source, parv, trailing
Client->>Dispatch: lookup command
Dispatch->>Handler: call handler(ctx, serverId, source, parv, mtags, trailing)
Handler->>Client: ctx.triggerEvent(eventName, payload)
Client->>Store: registered listener invoked
Store->>Store: store.setState(updater) / UI updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
matheusfillipe
changed the base branch from
main
to
feat/add-more-media-providers-and-previews
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
src/components/message/MessageReply.tsx-42-44 (1)
42-44:⚠️ Potential issue | 🟠 MajorNon-reactive store access may cause stale data.
useStore.getState().servers.find(...)reads the store imperatively without subscribing to changes. If the server'sfilehostis updated after mount, this component won't re-render, potentially showing or hiding the thumbnail incorrectly.Use a selector to subscribe reactively:
Proposed fix
const { showSafeMedia, showExternalContent } = useStore( (state) => state.globalSettings, ); - const server = replyMessage.serverId - ? useStore.getState().servers.find((s) => s.id === replyMessage.serverId) - : null; + const server = useStore((state) => + replyMessage.serverId + ? state.servers.find((s) => s.id === replyMessage.serverId) + : null, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/MessageReply.tsx` around lines 42 - 44, The code reads the store imperatively via useStore.getState().servers.find(...) which doesn't subscribe to updates; change to a reactive selector so the component re-renders when the server changes — replace the imperative access with a useStore selector like useStore(state => state.servers.find(s => s.id === replyMessage.serverId)) (or a memoized selector that depends on replyMessage.serverId) so the server variable in MessageReply.tsx updates when server.filehost or other server fields change.src/components/ui/EmojiPickerInline.tsx-18-32 (1)
18-32:⚠️ Potential issue | 🟠 MajorBlock outside wheel events while the inline picker is open.
This picker closes on outside click, but it doesn't install the capture-phase wheel trap used by the other reaction pickers. Wheeling over the chat while the picker is open can still reach
useScrollToBottomand flip the channel into the "scrolled up" state even though the list never moved.💡 Suggested fix
useEffect(() => { if (!isOpen) return; const onMouseDown = (e: MouseEvent) => { if (ref.current && !ref.current.contains(e.target as Node)) onClose(); }; const onKeyDown = (e: KeyboardEvent) => { if (e.key === "Escape") onClose(); }; + const onWheel = (e: WheelEvent) => { + if (ref.current?.contains(e.target as Node)) return; + e.preventDefault(); + e.stopPropagation(); + }; document.addEventListener("mousedown", onMouseDown); document.addEventListener("keydown", onKeyDown); + document.addEventListener("wheel", onWheel, { + passive: false, + capture: true, + }); return () => { document.removeEventListener("mousedown", onMouseDown); document.removeEventListener("keydown", onKeyDown); + document.removeEventListener("wheel", onWheel, { capture: true }); }; }, [isOpen, onClose]);Based on learnings, while the emoji picker is open, all chat scrolling is deliberately blocked to prevent
useScrollToBottomfrom incorrectly marking the chat as scrolled up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/EmojiPickerInline.tsx` around lines 18 - 32, The emoji picker useEffect (in EmojiPickerInline.tsx) currently closes on outside click but doesn't block wheel events; update that useEffect to, when isOpen is true, add a 'wheel' event listener in the capture phase that calls e.preventDefault() and e.stopPropagation() (use { capture: true, passive: false }) to block scrolling from reaching useScrollToBottom, and remove that listener in the cleanup alongside the existing mousedown/keydown handlers; reference the existing ref, isOpen and onClose variables and ensure the wheel handler only runs while the picker is open.src/components/ui/ReactionPopover.tsx-39-48 (1)
39-48:⚠️ Potential issue | 🟠 Major
placement="left"is offset by only half the picker width.Subtracting
PICKER_W / 2leaves half of the popover inside the container, which contradicts the "fully outside the container" placement this branch is trying to provide.💡 Suggested fix
const left = placement === "left" ? Math.max( MARGIN, Math.min( - (containerLeft ?? anchorRect.left) - PICKER_W / 2, + (containerLeft ?? anchorRect.left) - PICKER_W, vw - PICKER_W - MARGIN, ), ) : Math.min(Math.max(MARGIN, anchorRect.left), vw - PICKER_W - MARGIN);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ReactionPopover.tsx` around lines 39 - 48, The left calculation for placement === "left" incorrectly subtracts PICKER_W / 2, leaving half the popover inside the container; update the logic in the left expression (the branch that checks placement === "left", which uses containerLeft, anchorRect.left, PICKER_W and vw) to subtract the full PICKER_W (or otherwise compute an x position that places the right edge of the popover flush with the container/anchor) before clamping with Math.max/Math.min so the popover is fully outside the container.src/hooks/useScrollToBottom.ts-66-95 (1)
66-95:⚠️ Potential issue | 🟠 MajorKeep
isScrolledUpaligned with the suppression state.
handleWheel()forceswasAtBottomRef.current = false, but bothcheckIfScrolledToBottom()and the observer still callsetIsScrolledUp(...)before the suppression guard. A small upward wheel that stays within the tolerance window can therefore hide the "scroll to bottom" affordance while auto-scroll is already disabled for incoming messages.💡 Suggested fix
const checkIfScrolledToBottom = () => { const atBottom = isScrolledToBottom(container, tolerance); - setIsScrolledUp(!atBottom); if (wheelUpActive) { + setIsScrolledUp(true); // User is still mid-gesture. Only cancel suppression once they've // genuinely scrolled past the tolerance zone. if (!atBottom) { clearWheelUp(); wasAtBottomRef.current = false; } // If atBottom while suppressed: keep wasAtBottomRef false so the next // message doesn't trigger auto-scroll back down. } else { + setIsScrolledUp(!atBottom); wasAtBottomRef.current = atBottom; } }; @@ (entries) => { const isVisible = entries[0].isIntersecting; - setIsScrolledUp(!isVisible); if (wheelUpActive) { + setIsScrolledUp(true); if (!isVisible) { clearWheelUp(); wasAtBottomRef.current = false; } } else { + setIsScrolledUp(!isVisible); wasAtBottomRef.current = isVisible; } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScrollToBottom.ts` around lines 66 - 95, The issue is that checkIfScrolledToBottom and the IntersectionObserver callback call setIsScrolledUp before honoring the wheel-up suppression state, which can hide the "scroll to bottom" affordance while auto-scroll is suppressed by handleWheel; update both checkIfScrolledToBottom and the observer callback so they check wheelUpActive first and only call setIsScrolledUp with the computed visibility/atBottom when not suppressed (or else set it to the suppressed-consistent value using wasAtBottomRef.current), and ensure they still call clearWheelUp() and update wasAtBottomRef.current appropriately when moving out of the tolerance/visibility zone (use the same suppression-aware logic in both checkIfScrolledToBottom and the IntersectionObserver callback to keep state aligned with handleWheel, wasAtBottomRef, wheelUpActive, clearWheelUp, and setIsScrolledUp).src/hooks/useAutoFocusTyping.ts-3-17 (1)
3-17:⚠️ Potential issue | 🟠 MajorTreat printable keys as the trigger, not just
[A-Z0-9].The current regex drops the first character for punctuation and non-ASCII printable keys, so "focus on type" fails for common starts like
.and for users on non-English keyboard layouts.💡 Suggested fix
-const TYPING_RE = /^[a-zA-Z0-9]$/; +const isPlainPrintableKey = (e: KeyboardEvent) => + e.key.length === 1 && + !e.ctrlKey && + !e.metaKey && + !e.altKey && + !e.isComposing; @@ const handler = (e: KeyboardEvent) => { - // Only plain alphanumeric — skip Ctrl/Cmd/Alt shortcuts - if (!TYPING_RE.test(e.key)) return; - if (e.ctrlKey || e.metaKey || e.altKey) return; + if (!isPlainPrintableKey(e)) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useAutoFocusTyping.ts` around lines 3 - 17, The handler in useAutoFocusTyping currently only tests TYPING_RE (/^[a-zA-Z0-9]$/) which excludes punctuation and non‑ASCII printable characters; replace that check with a printable-key test (e.g. treat a key as a trigger when e.key.length === 1 so single-character keys—including punctuation and non‑ASCII—count) and remove or update the TYPING_RE constant; keep the existing guards for e.ctrlKey/e.metaKey/e.altKey and use isBlockedRef/current and inputRef logic unchanged in the handler.src-tauri/plugins/share-sheet/ios/Sources/ShareSheet/ShareSheet.swift-10-14 (1)
10-14:⚠️ Potential issue | 🟠 MajorPresent the share sheet from the topmost presented view controller.
When another modal or sheet is already displayed,
rootVC.present(...)fails becauserootVCalready has a non-nilpresentedViewController. Walk thepresentedViewControllerchain to find the topmost presenter and callpresent()on it.💡 Suggested fix
guard let scene = UIApplication.shared.connectedScenes .first(where: { $0.activationState == .foregroundActive }) as? UIWindowScene, let rootVC = scene.windows.first(where: { $0.isKeyWindow })?.rootViewController else { return } + var presenter = rootVC + while let presented = presenter.presentedViewController { + presenter = presented + } + let activityVC = UIActivityViewController( activityItems: [fileURL], applicationActivities: nil ) // iPad requires a popover anchor, otherwise it crashes if let popover = activityVC.popoverPresentationController { - popover.sourceView = rootVC.view + popover.sourceView = presenter.view popover.sourceRect = CGRect( - x: rootVC.view.bounds.midX, - y: rootVC.view.bounds.midY, + x: presenter.view.bounds.midX, + y: presenter.view.bounds.midY, width: 0, height: 0 ) popover.permittedArrowDirections = [] } - rootVC.present(activityVC, animated: true) + presenter.present(activityVC, animated: true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/ios/Sources/ShareSheet/ShareSheet.swift` around lines 10 - 14, The current code in ShareSheet.swift fetches scene and rootVC but always calls rootVC.present(...), which fails when rootVC.presentedViewController is non-nil; update the presentation logic to walk the presentedViewController chain (e.g., start from rootVC and while let next = current.presentedViewController { current = next }) to find the topmost view controller, then call present(...) on that topmost presenter (keep the existing scene/window lookup using UIApplication.shared.connectedScenes and the rootViewController retrieval).src/store/handlers/batches.ts-127-174 (1)
127-174:⚠️ Potential issue | 🟠 MajorDuplicate
BATCH_STARTevent listeners may cause issues.There are two separate
ircClient.on("BATCH_START", ...)registrations (lines 128 and 156). Both will fire for everyBATCH_STARTevent, potentially causing duplicate state updates and race conditions betweenmetadataBatchesandactiveBatches.Consider consolidating into a single listener that handles both maps.
🐛 Proposed consolidation
- ircClient.on("BATCH_START", ({ serverId, batchId, type }) => { - const state = store.getState(); - - if (!state.metadataBatches[batchId]) { - store.setState((state) => ({ - metadataBatches: { - ...state.metadataBatches, - [batchId]: { type, messages: [] }, - }, - })); - } - }); - - ircClient.on("BATCH_END", ({ serverId, batchId }) => { - // End a batch - process all messages in the batch - store.setState((state) => { - const batch = state.metadataBatches[batchId]; - if (batch) { - // Process batch messages (they should have been collected during the batch) - // For metadata batches, the individual METADATA_KEYVALUE events should have updated the state - } - const { [batchId]: _, ...remainingBatches } = state.metadataBatches; - return { - metadataBatches: remainingBatches, - }; - }); - }); - - ircClient.on("BATCH_START", ({ serverId, batchId, type, parameters }) => { + ircClient.on("BATCH_START", ({ serverId, batchId, type, parameters }) => { + // Update metadataBatches + const state = store.getState(); + if (!state.metadataBatches[batchId]) { + store.setState((s) => ({ + metadataBatches: { + ...s.metadataBatches, + [batchId]: { type, messages: [] }, + }, + })); + } + + // Update activeBatches store.setState((state) => { const serverBatches = state.activeBatches[serverId] || {}; return { activeBatches: { ...state.activeBatches, [serverId]: { ...serverBatches, [batchId]: { type, parameters: parameters || [], events: [], startTime: new Date(), }, }, }, }; }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/batches.ts` around lines 127 - 174, The file registers two ircClient.on("BATCH_START", ...) handlers which causes duplicate updates; consolidate them inside registerBatchHandlers into a single BATCH_START listener that updates both metadataBatches and activeBatches atomically: read state via store.getState or in store.setState callback, create/merge metadataBatches[batchId] = { type, messages: [] } if missing, and also set activeBatches[serverId][batchId] = { type, parameters: parameters || [], events: [], startTime: new Date() } in the same setState call to avoid races; remove the duplicate BATCH_START handler so only one listener (inside registerBatchHandlers) manages both metadataBatches and activeBatches.src/store/handlers/channels.ts-111-131 (1)
111-131:⚠️ Potential issue | 🟠 MajorDirect state mutation in ban/invite/exception handlers.
When
ch.bans(orinvites/exceptions) exists,const bans = ch.bans || []returns a reference to the existing array. Callingbans.push(...)mutates the store state directly, which can cause React to miss updates and lead to subtle bugs.Proposed fix for RPL_BANLIST (apply similar pattern to INVITELIST and EXCEPTLIST)
if (ch.name === channel) { - const bans = ch.bans || []; + const bans = [...(ch.bans || [])]; // Add the ban if it doesn't already exist - if (!bans.some((ban) => ban.mask === mask)) { + if (!ch.bans?.some((ban) => ban.mask === mask)) { bans.push({ mask, setter, timestamp });Also applies to: 150-170, 189-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/channels.ts` around lines 111 - 131, The handler inside store.setState mutates existing arrays by doing const bans = ch.bans || [] and then bans.push(...), which mutates state; change it to create a new bans array when updating (e.g., const bans = ch.bans ? [...ch.bans, newBan] : [newBan]) and return a new channel object so updatedServers mapping stays immutable; apply the same immutable pattern for INVITELIST and EXCEPTLIST handlers (locations referenced by ch.bans, ch.invites, ch.exceptions within the updatedServers/server.channels mapping and the RPL_BANLIST handling code).src/store/handlers/channels.ts-47-47 (1)
47-47:⚠️ Potential issue | 🟠 MajorMode check doesn't distinguish
+ofrom-o.
modestring.includes("o")is true for both+o(granting operator) and-o(revoking operator). When a user's operator status is revoked,isIrcOpwould still be set totrue.Proposed fix
- const isIrcOp = modestring.includes("o"); + // Check if +o is being set (not -o being removed) + const isAddingOp = /\+[^-]*o/.test(modestring); + const isRemovingOp = /-[^+]*o/.test(modestring); + const isIrcOp = isAddingOp ? true : isRemovingOp ? false : currentUser.isIrcOp;Note: Mode strings can be complex (e.g.,
+o-v). A more robust solution would parse the mode string character by character, tracking the current polarity.Also applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/channels.ts` at line 47, The check using modestring.includes("o") in the handler (the isIrcOp assignment) fails to distinguish +o from -o; update the logic in the channel handler where isIrcOp is computed to parse the modestring sequentially, track the current sign/polarity (+ or -) and only set isIrcOp true when an 'o' is encountered while the polarity is '+' (and unset/leave false when encountered with '-'); apply the same fix to the other occurrence referenced (the other isIrcOp/modestring usage around the second occurrence noted).src/store/handlers/auth.ts-44-52 (1)
44-52:⚠️ Potential issue | 🟠 MajorSASL AUTHENTICATE sent even when server config not found.
If no saved server matches
serverId, the loop completes without returning, and line 51 sendsAUTHENTICATE PLAINunconditionally. This could cause authentication attempts for servers that have no SASL configuration.Proposed fix
if (key === "sasl") { const servers = storage.servers.load(); + let saslEnabled = false; for (const serv of servers) { if (serv.id !== serverId) continue; - - if (!serv.saslEnabled) return; + saslEnabled = serv.saslEnabled ?? false; + break; + } + if (saslEnabled) { + ircClient.sendRaw(serverId, "AUTHENTICATE PLAIN"); } - ircClient.sendRaw(serverId, "AUTHENTICATE PLAIN"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/auth.ts` around lines 44 - 52, The code sends AUTHENTICATE unconditionally if no stored server matches serverId; update the block that checks key === "sasl" to only call ircClient.sendRaw(serverId, "AUTHENTICATE PLAIN") when a matching server with serv.id === serverId is found and serv.saslEnabled is true (e.g., iterate storage.servers.load(), set a found flag or return immediately when you find a matching server with saslEnabled, and avoid sending if no match exists). Ensure you reference the existing symbols storage.servers.load(), serv.id, serv.saslEnabled, and ircClient.sendRaw(serverId, "AUTHENTICATE PLAIN") when making the change.src/components/ui/MediaViewerModal.tsx-1158-1163 (1)
1158-1163:⚠️ Potential issue | 🟠 MajorSame
react-playerprop issue — useurlinstead ofsrc.Consistent with the other files,
ReactPlayerexpects theurlprop.🐛 Proposed fix
<ReactPlayer - src={currentUrl} + url={currentUrl} width="100%" height="100%" controls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/MediaViewerModal.tsx` around lines 1158 - 1163, In MediaViewerModal, the ReactPlayer usage is passing the video via the wrong prop; replace the src prop with url so ReactPlayer receives the media correctly (update the JSX in MediaViewerModal where ReactPlayer is rendered, keep the same width, height and controls props and pass currentUrl via url={currentUrl} to the ReactPlayer component).src/components/message/MediaPreview.tsx-795-798 (1)
795-798:⚠️ Potential issue | 🟠 MajorSame
react-playerprop issue — useurlinstead ofsrc.This is the same issue as in
MiniMediaPlayer.tsx. TheReactPlayercomponent expectsurlprop, notsrc.🐛 Proposed fix
<ReactPlayer key={playerKey} - src={url} + url={url} width="100%" height="100%"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/MediaPreview.tsx` around lines 795 - 798, The ReactPlayer in MediaPreview.tsx is using the wrong prop name; replace the incorrect src={url} prop on the ReactPlayer element (keyed by playerKey) with url={url} so the component receives the media URL correctly, and mirror the same change you made in MiniMediaPlayer.tsx to keep behavior consistent.src/lib/irc/handlers/auth.ts-21-24 (1)
21-24:⚠️ Potential issue | 🟠 MajorTreat the trailing auth description as
message, not always astarget.When the optional context is absent,
parv[2]is already the server description. The current split emits it astarget, leavesmessageempty, andsubstring(1)also truncates the first real character when the trailing param is already normalized.Proposed fix
- const target = parv[2] || undefined; - const message = parv.slice(3).join(" ").substring(1); + const hasTarget = parv.length > 3; + const target = hasTarget ? parv[2] : undefined; + const rawMessage = parv.slice(hasTarget ? 3 : 2).join(" "); + const message = rawMessage.startsWith(":") + ? rawMessage.slice(1) + : rawMessage;Also applies to: 42-45, 63-66, 84-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/handlers/auth.ts` around lines 21 - 24, The parsing currently assumes parv[2] is always a target and parv.slice(3) is the message, which breaks when the optional target is omitted and the server description comes as a single trailing param (prefixed with ':'); update the logic that builds target and message so that if parv[2] starts with ':' (or parv.length <= 3 and the trailing param is present) you set target = undefined and set message = parv.slice(2).join(" ").substring(1), otherwise set target = parv[2] and message = parv.slice(3).join(" ").substring(1). Apply the same fix to the analogous parsing blocks referenced in your comment (the blocks around the other occurrences).src/lib/irc/handlers/metadata.ts-95-99 (1)
95-99:⚠️ Potential issue | 🟠 Major
METADATA_SUB*loses protocol context.These handlers parse the target and then drop it, and
handleMetadataSubsalso startskeysat index1, so the visibility token gets emitted as a key. That makes subscription acknowledgements ambiguous once more than one target is in play.Also applies to: 109-113, 123-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/handlers/metadata.ts` around lines 95 - 99, The METADATA_SUB* handlers drop the parsed target and mis-slice keys (starting at index 1) so the visibility token becomes a key and protocol context is lost; update the handlers (e.g., the block using _target and keys and the functions handleMetadataSub / handleMetadataSubs) to include the parsed target in the emitted event (add target/_target to the METADATA_SUBOK payload) and fix the keys slice to start after the target and visibility token (use parv.slice(2) instead of parv.slice(1), still stripping a leading ":" from the last entry) so only actual metadata keys are emitted.src/lib/irc/handlers/messages.ts-11-12 (1)
11-12:⚠️ Potential issue | 🟠 MajorDon't hard-code
#as the only channel prefix.
PRIVMSG,NOTICE, and multiline batches will misclassify&,+,!, and server-advertisedCHANTYPESas user targets.Also applies to: 77-78, 215-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/handlers/messages.ts` around lines 11 - 12, The code incorrectly treats channels by checking target.startsWith("#"); update the channel-detection logic (used where the variable target and boolean isChannel are set, and in PRIVMSG/NOTICE and multiline batch handling) to test the first character against the server-advertised channel prefix set (CHANTYPES) or a shared CHANNEL_PREFIXES constant instead of a hard-coded "#"; if the codebase exposes server properties (e.g., serverProps.chantypes, connection.chantypes, or similar), use that list (falling back to the standard "&#+!" set if absent) to determine isChannel, and replace every occurrence of target.startsWith("#") with this new check.src/lib/irc/handlers/whois.ts-125-127 (1)
125-127:⚠️ Potential issue | 🟠 Major
WHOIS_BOTis shifted one parameter left.Every other WHOIS numeric in this file treats
parv[0]as the requester. This handler uses it as the subject nick, so the emittednick/target/messagetuple is off by one.Proposed fix
- const nick = parv[0]; - const target = parv[1]; - const message = parv.slice(2).join(" "); + const nick = parv[1]; + const target = parv[2]; + const message = parv.slice(3).join(" ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/handlers/whois.ts` around lines 125 - 127, The WHOIS handler is incorrectly indexing parv so WHOIS_BOT (the requester) is treated as the subject; change the parameter offsets so parv[0] is the requester and the subject fields shift right: set requester = parv[0], nick = parv[1], target = parv[2], and message = parv.slice(3).join(" "), and update any subsequent code that uses nick/target/message in this handler to match the new indices.src/lib/irc/handlers/messages.ts-195-205 (1)
195-205:⚠️ Potential issue | 🟠 MajorUse the previous line's concat flag when stitching multiline batches.
The separator between line N-1 and line N is determined by the previous message's
draft/multiline-concattag. ReadingconcatFlags[index]inserts newlines where the batch requested concatenation.Proposed fix
- const wasConcat = batch.concatFlags?.[index]; + const wasConcat = batch.concatFlags?.[index - 1];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/handlers/messages.ts` around lines 195 - 205, The concatenation logic currently checks batch.concatFlags?.[index] for the separator before message at position index; change it to use the previous message's flag (batch.concatFlags?.[index - 1]) when index > 0 so the separator between line N-1 and N follows the prior message's draft/multiline-concat tag; update the variable (e.g., rename wasConcat to prevWasConcat or similar) and adjust the conditional inside the batch.messages.forEach callback that builds combinedMessage to consult batch.concatFlags[index - 1] for index > 0.src/lib/irc/handlers/metadata.ts-152-180 (1)
152-180:⚠️ Potential issue | 🟠 MajorDon't discard the server's metadata failure text.
_errorMessageis parsed here and then never emitted. For non-RATE_LIMITEDfailures, that text is usually the only actionable detail the server sends.Proposed fix
- let _errorMessage = ""; + let message: string | undefined; @@ - _errorMessage = lastParam; + message = lastParam.startsWith(":") + ? lastParam.substring(1) + : lastParam; @@ ctx.triggerEvent("METADATA_FAIL", { serverId, subcommand, code, target, key, retryAfter, + message, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/handlers/metadata.ts` around lines 152 - 180, The parsed failure text stored in _errorMessage is never passed on — update the METADATA_FAIL emission (ctx.triggerEvent("METADATA_FAIL", {...})) to include that parsed message (e.g., add errorMessage: _errorMessage or message: _errorMessage) so callers receive the server's failure text; ensure the parsing logic that sets _errorMessage (the lastParam check) remains unchanged and that retryAfter is still only set for code === "RATE_LIMITED".src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift-93-114 (1)
93-114:⚠️ Potential issue | 🟠 MajorReplace the force cast with a proper
[String: Any?]dictionary.This code builds an
NSMutableDictionaryand force-casts it toJsonObject(which is[String: Any?]). The force cast will trap at runtime if the cast fails—particularly risky in an error path where the failure would mask the original error. Additionally, flatteningdatainto the top-level payload merges error metadata with user data, changing the error payload shape. Build the response dictionary directly as[String: Any?]instead.var payload: [String: Any?] = ["message": message] if let code = code { payload["code"] = code } if let error = error { payload["error"] = error.localizedDescription } if let data = data { switch data { case .dictionary(let dict): for entry in dict { payload[entry.key] = entry.value } } } sendResponse(self.error, serialize(.dictionary(payload)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift` around lines 93 - 114, Replace the NSMutableDictionary and the force-cast to JsonObject by building the payload as a Swift dictionary var payload: [String: Any?] = ["message": message], set payload["code"] and payload["error"] when present, and when handling data (case .dictionary(let dict)) avoid flattening into top-level keys — put the user data under a distinct key like payload["data"] = dict (or otherwise convert dict to [String: Any?]) to preserve error payload shape; finally call sendResponse(self.error, serialize(.dictionary(payload))) without any forced casts to ensure type-safety.src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift-62-101 (1)
62-101:⚠️ Potential issue | 🟠 MajorDispatch plugin commands to the main thread to support UIKit/WebKit operations.
Plugin selectors are currently dispatched to
ipcDispatchQueue(a background queue), but plugins frequently need to present UI or interact withWKWebView, both of which require main-thread execution. The ShareSheet plugin already works around this by manually dispatching toDispatchQueue.main, but this shouldn't be necessary—the framework should handle main-thread dispatch for UI operations, or plugins should declare their threading requirements explicitly. Without a solution, plugin developers must remember to manually dispatch back to main or face crashes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift` around lines 62 - 101, The plugin command dispatch currently runs on ipcDispatchQueue (background) which causes UI/WebKit work to crash; change the dispatch so the selectors (selectorWithCompletionHandler, selectorWithThrows, and selector) are invoked on the main thread by using DispatchQueue.main.async for the block that performs method lookups and invocations (the code that currently begins with ipcDispatchQueue.async and contains the calls to plugin.instance.method(for:), unsafeBitCast/perform, and invoke.reject). Ensure the completion block and the NSError-handling path still execute on the main thread so plugins like ShareSheet no longer need to manually dispatch to DispatchQueue.main.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6108f464-92d1-487c-ba2c-ff772a6ce7d3
⛔ Files ignored due to path filters (5)
package-lock.jsonis excluded by!**/package-lock.jsonsrc-tauri/Cargo.lockis excluded by!**/*.locksrc-tauri/gen/apple/ObsidianIRC_iOS/Info.plistis excluded by!**/gen/**tests/fixtures/sample.mp3is excluded by!**/*.mp3tests/fixtures/sample.mp4is excluded by!**/*.mp4
📒 Files selected for processing (102)
.gitattributesAGENTS.mdpackage.jsonpublic/pdf.worker.min.mjssrc-tauri/Cargo.tomlsrc-tauri/plugins/share-sheet/.tauri/tauri-api/.gitignoresrc-tauri/plugins/share-sheet/.tauri/tauri-api/Package.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/README.mdsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Channel.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/JSTypes.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/JsonValue.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Logger.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Plugin/Plugin.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/UiUtils.swiftsrc-tauri/plugins/share-sheet/Cargo.tomlsrc-tauri/plugins/share-sheet/build.rssrc-tauri/plugins/share-sheet/ios/Package.resolvedsrc-tauri/plugins/share-sheet/ios/Package.swiftsrc-tauri/plugins/share-sheet/ios/Sources/ShareSheet/ShareSheet.swiftsrc-tauri/plugins/share-sheet/src/lib.rssrc-tauri/src/lib.rssrc/App.tsxsrc/components/layout/AppLayout.tsxsrc/components/layout/ChannelList.tsxsrc/components/layout/ChannelMessageList.tsxsrc/components/layout/ChatArea.tsxsrc/components/layout/MemberList.tsxsrc/components/message/CollapsibleMessage.tsxsrc/components/message/MediaPreview.tsxsrc/components/message/MessageActions.tsxsrc/components/message/MessageAvatar.tsxsrc/components/message/MessageItem.tsxsrc/components/message/MessageReactions.tsxsrc/components/message/MessageReply.tsxsrc/components/message/ReactionsWithActions.tsxsrc/components/message/SwipeableMessage.tsxsrc/components/mobile/MessageBottomSheet.tsxsrc/components/ui/AppEmojiPicker.tsxsrc/components/ui/AutocompleteDropdown.tsxsrc/components/ui/EmojiPickerInline.tsxsrc/components/ui/EmojiPickerModal.tsxsrc/components/ui/ExternalLinkWarningModal.tsxsrc/components/ui/GifSelector.tsxsrc/components/ui/InputToolbar.tsxsrc/components/ui/MediaCommentsSidebar.tsxsrc/components/ui/MediaViewerModal.tsxsrc/components/ui/MiniMediaPlayer.tsxsrc/components/ui/ReactionModal.tsxsrc/components/ui/ReactionPopover.tsxsrc/components/ui/TextInput.tsxsrc/hooks/useAutoFocusTyping.tssrc/hooks/useJoinAndSelectChannel.tssrc/hooks/useScrollToBottom.tssrc/index.csssrc/lib/audioManager.tssrc/lib/imageUtils.tssrc/lib/irc/IRCClient.tssrc/lib/irc/IRCClientContext.tssrc/lib/irc/handlers/auth.tssrc/lib/irc/handlers/channels.tssrc/lib/irc/handlers/connection.tssrc/lib/irc/handlers/index.tssrc/lib/irc/handlers/messages.tssrc/lib/irc/handlers/metadata.tssrc/lib/irc/handlers/monitoring.tssrc/lib/irc/handlers/users.tssrc/lib/irc/handlers/whois.tssrc/lib/irc/utils.tssrc/lib/ircClient.tssrc/lib/mediaProbe.tssrc/lib/mediaUtils.tssrc/lib/modal/components/Button.tsxsrc/lib/settings/definitions/allSettings.tssrc/lib/videoPositionCache.tssrc/main.tsxsrc/store/handlers/auth.tssrc/store/handlers/batches.tssrc/store/handlers/channels.tssrc/store/handlers/connection.tssrc/store/handlers/index.tssrc/store/handlers/messages.tssrc/store/handlers/metadata.tssrc/store/handlers/users.tssrc/store/handlers/whois.tssrc/store/helpers.tssrc/store/index.tssrc/store/localStorage.tssrc/store/types.tstests/components/ChannelListModal.test.tsxtests/components/MediaCommentsSidebar.test.tsxtests/components/MediaPreview.test.tsxtests/components/MediaViewerModal.test.tsxtests/components/MessageItem.mediaCollapse.test.tstests/components/MetadataDisplay.test.tsxtests/components/MiniMediaPlayer.test.tsxtests/fixtures/uiState.tstests/lib/mediaProbe.test.tstests/lib/mediaUtils.test.tstests/lib/videoPositionCache.test.tsvite.config.ts
…o feat/bugfixes-and-refactor
|
Automated deployment preview for the PR in the Cloudflare Pages. |
- Narrow ChannelMessageList selector to per-channel key (was subscribing
to full messages map, causing re-render on every incoming message)
- Wrap MessageItem in memo + useMemo markdown processing
- Lazy-load highlight.js (969KB deferred from initial bundle)
- Replace opacity:0 with visibility:hidden on message action bars
- Split vendor chunks (react, markdown, zustand) for better caching
- Fix reactions and replies lost during chathistory batch playback
- Inline MdAddReaction/RiReplyFill SVGs to drop react-icons sub-packages
matheusfillipe
changed the base branch from
feat/add-more-media-providers-and-previews
to
main
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/message/CollapsibleMessage.tsx (1)
65-77:⚠️ Potential issue | 🟡 MinorCancel the queued RAF when the effect reruns or unmounts.
measure()is called both directly and byResizeObservercallbacks, so multiplerequestAnimationFramecalls can queue without cancellation. When the effect reruns (due tomaxLinesoronNeedsCollapsingchanges), pending RAFs from the previous cycle will still execute. Additionally, a stale callback from an earlier hidden measurement can flipallowTransitionRefback totrueafter the component unmounts, defeating the transition gating this change adds.Store the RAF ID and cancel it before queuing a new one, and cancel any pending frame in the cleanup function:
♻️ Suggested fix
const measuredContentRef = useRef<HTMLDivElement>(null); // Stays false through the initial measurement render so the first collapse is instant. // Flipped to true via RAF after that render commits; user expand/collapse animates after. const allowTransitionRef = useRef(false); + const transitionFrameRef = useRef<number | null>(null); useLayoutEffect(() => { if (!contentRef.current || !measuredContentRef.current) return; @@ - requestAnimationFrame(() => { + if (transitionFrameRef.current !== null) { + cancelAnimationFrame(transitionFrameRef.current); + } + transitionFrameRef.current = requestAnimationFrame(() => { allowTransitionRef.current = true; + transitionFrameRef.current = null; }); }; @@ - return () => resizeObserver.disconnect(); + return () => { + resizeObserver.disconnect(); + if (transitionFrameRef.current !== null) { + cancelAnimationFrame(transitionFrameRef.current); + transitionFrameRef.current = null; + } + }; }, [maxLines, onNeedsCollapsing]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/CollapsibleMessage.tsx` around lines 65 - 77, The effect schedules requestAnimationFrame inside measure causing stray frames to run; change measure to store the RAF id (e.g., rafIdRef or local rafId) and call cancelAnimationFrame on any existing id before queuing a new requestAnimationFrame, and also cancelAnimationFrame in the effect cleanup alongside resizeObserver.disconnect(); update references in measure, allowTransitionRef, and the ResizeObserver cleanup so no stale RAF can flip allowTransitionRef.current after reruns or unmounts.src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift (1)
41-45:⚠️ Potential issue | 🟠 MajorMark the handle as loaded after deferred initialization.
onWebviewCreated(_:)now callsload(webview:)but never flipshandle.loadedtotrue. That makes every later webview creation re-run plugin setup and can duplicate observers, handlers, or other side effects.Possible fix
func onWebviewCreated(_ webview: WKWebView) { for (_, handle) in plugins { if !handle.loaded { handle.instance.load(webview: webview) + handle.loaded = true } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift` around lines 41 - 45, onWebviewCreated(_:) calls each plugin handle's load(webview:) but never updates the handle.loaded flag, causing repeated initialization; after successfully calling handle.instance.load(webview: webview) set handle.loaded = true on that handle (and only after successful initialization) so subsequent onWebviewCreated(_:) calls skip already-loaded plugins; update the loop that iterates plugins to flip handle.loaded to true immediately after load(webview:) (or inside a success path) to prevent duplicate observers/handlers.src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift (2)
47-52:⚠️ Potential issue | 🟠 MajorJSON-escape error strings before sending them.
"\"\(error)\""is not valid JSON encoding for arbitrary error text. If the message contains quotes, backslashes, or newlines, the bridge sends malformed JSON back to JS. Encode the string throughJSONEncoderor your existing JSON value serializer instead of manual interpolation.Also applies to: line 72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift` around lines 47 - 52, The current serialize(_ data: JsonValue) implementation returns error text via manual string interpolation ("\"\(error)\""), which can produce invalid JSON; replace that interpolation with a proper JSON-escaped string by encoding the error through your JSON serializer (e.g., create a JsonValue string from error.localizedDescription or use JSONEncoder to encode the error string) and then call jsonRepresentation() so the result is valid JSON; make the same change to the other serialize implementation that returns error strings (the duplicate/error-path serialize) so both use the JSON-safe path instead of manual interpolation.
87-100:⚠️ Potential issue | 🔴 CriticalConvert Error to JSON-serializable format before assignment.
The
Errorobject assigned at line 88 breaks serialization:JSONSerialization.isValidJSONObject()rejects dictionaries containingErrorinstances since they are not JSON-compatible types. WhenjsonRepresentation()is called at line 100, the serialization fails and error details are lost. Convert the error to a string or structured object (e.g.,error.localizedDescription) before adding it to the payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift` around lines 87 - 100, The payload currently stores the raw Error instance which breaks JSON serialization; update the block that assigns the error (the local variable error used before payload["error"]) to convert it to a JSON-serializable value (for example error.localizedDescription or a small dictionary like ["message": error.localizedDescription, "nsError": (error as NSError).domain] or include code: (error as NSError).code) before inserting into payload, so sendResponse(self.error, serialize(.dictionary(payload as! JsonObject))) receives only JSON-compatible types; modify the Error assignment logic accordingly in the same scope where payload is populated and before serialize/sendResponse is called.src/lib/mediaUtils.ts (1)
286-291:⚠️ Potential issue | 🟠 MajorKeep
showTrustedSourcesMediaaligned withdetectTrustedDomainType().This branch still treats any hostname in
TRUSTED_EMBED_DOMAINSas showable, even whendetectTrustedDomainType()now rejects the URL because the path is not actually embeddable. That re-opens the trusted-media path for URLs like non-video YouTube/Vimeo pages and can trigger probe/render work you just fenced off.Suggested fix
if (settings.showTrustedSourcesMedia) { - const hostname = extractHostname(url); - if (hostname === null) return false; - for (const domain of Object.keys(TRUSTED_EMBED_DOMAINS)) { - if (hostname === domain || hostname.endsWith(`.${domain}`)) return true; - } + return detectTrustedDomainType(url) !== null; }As per coding guidelines
src/lib/media*.ts: Only probe media URLs from trusted origins (server-accepted filehosts, known-safe embeddable services, or if user explicitly enabled 'show all external content').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mediaUtils.ts` around lines 286 - 291, The branch gated by settings.showTrustedSourcesMedia currently allows any hostname listed in TRUSTED_EMBED_DOMAINS without consulting the more strict detection logic; update the logic in the block that uses extractHostname(url) so it also calls detectTrustedDomainType(url) (or the same internal check used by detectTrustedDomainType) and only returns true when that function indicates an embeddable/trusted domain type (e.g., "embed" or the specific allowed enum), otherwise return false so non-embeddable pages (like generic YouTube/Vimeo pages) are not treated as probeable trusted media.
♻️ Duplicate comments (1)
src/lib/irc/IRCClient.ts (1)
885-893:⚠️ Potential issue | 🔴 CriticalClear the previous pong timeout before scheduling a new one.
A stale timeout from the prior ping can still close the socket even after a newer ping is in flight. That makes the keepalive loop disconnect early under packet loss.
Suggested fix
this.sendRaw(serverId, `PING :${timestamp}`); // Set a timeout for pong response (10 seconds) + const oldPongTimeout = this.pongTimeouts.get(serverId); + if (oldPongTimeout) { + clearTimeout(oldPongTimeout); + } const pongTimeout = setTimeout(() => { console.warn( `WebSocket ping timeout for server ${serverId}, closing connection`, ); socket.close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/IRCClient.ts` around lines 885 - 893, Existing pong timeouts must be cleared before scheduling a new one to avoid stale timers closing the socket; in the IRCClient class where you create the pong timeout (the code using this.pongTimeouts.set(serverId, pongTimeout)), first check this.pongTimeouts.get(serverId) and clearTimeout on it (and delete the map entry) before calling setTimeout, and also ensure you remove/clear the timeout when you close the socket so no leftover timer can later call socket.close().
🟡 Minor comments (4)
src/lib/eventGrouping.ts-28-48 (1)
28-48:⚠️ Potential issue | 🟡 MinorDead code:
return "quit"on line 47 is unreachable.When reaching line 37,
lastmust be"join"(since"quit"and"part"return earlier). Sincelast === "join",joinCountwill always be ≥ 1, so line 45 will always return. Line 47 can never execute.♻️ Suggested fix
const joinCount = types.filter((t) => t === "join").length; if (joinCount > 0) return formatCount("joined", joinCount); - - return "quit"; + + // Unreachable, but satisfies TypeScript + return ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/eventGrouping.ts` around lines 28 - 48, The final unconditional return "quit" in computeUserSummary is unreachable; remove it and replace with an explicit unreachable assertion to make intent clear: in computeUserSummary (use the function name to find it) delete the last line returning "quit" and instead throw a clear error like throw new Error("computeUserSummary: unreachable") (or use a language-specific unreachable/assert helper) so any future control-flow changes fail loudly.src/components/layout/ChatHeader.tsx-297-306 (1)
297-306:⚠️ Potential issue | 🟡 MinorMedia overflow menu item won't work for private chats.
The overflow menu's "Media" item calls
openMediaExplorer(selectedServerId, selectedChannelId), but for private chatsselectedChannelIdisnull. The inline button for private chats (lines 812-814) correctly usesselectedPrivateChat?.id, but the overflow menu item doesn't.🔧 Suggested fix
{ label: "Media", icon: <FaFilm />, onClick: () => { - if (selectedServerId && selectedChannelId) { - openMediaExplorer(selectedServerId, selectedChannelId); + const chatId = selectedChannelId ?? selectedPrivateChat?.id ?? null; + if (selectedServerId && chatId) { + openMediaExplorer(selectedServerId, chatId); } }, show: hasMedia, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/ChatHeader.tsx` around lines 297 - 306, The overflow menu "Media" item in ChatHeader.tsx uses openMediaExplorer(selectedServerId, selectedChannelId) which fails for private chats because selectedChannelId is null; update the onClick handler to pass selectedPrivateChat?.id when selectedChannelId is null (e.g., compute channelId = selectedChannelId ?? selectedPrivateChat?.id) and call openMediaExplorer(selectedServerId, channelId) so the menu matches the inline private-chat button behavior (references: openMediaExplorer, selectedServerId, selectedChannelId, selectedPrivateChat, hasMedia).src/components/ui/TopicMediaStrip.tsx-163-172 (1)
163-172:⚠️ Potential issue | 🟡 MinorVideo thumbnail rendering won't work as expected.
Using
<img src={url}>whereurlis a video file (e.g.,.mp4) won't display anything meaningful — browsers don't render video files in<img>tags. Consider using a generic video icon placeholder instead, or extract a thumbnail frame using a<video>element withposter.🔧 Suggested fix using placeholder icon
if (type === "video") { return ( - <div className="relative w-6 h-6 rounded bg-discord-dark-200 overflow-hidden"> - <img src={url} alt="" className="w-6 h-6 object-cover" loading="lazy" /> + <div className="relative w-6 h-6 rounded bg-discord-dark-200 overflow-hidden flex items-center justify-center"> <div className="absolute inset-0 flex items-center justify-center bg-black/50"> <FaPlay className="text-white" style={{ fontSize: 8 }} /> </div> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/TopicMediaStrip.tsx` around lines 163 - 172, In TopicMediaStrip when handling type === "video" you are using <img src={url}> which won't render video files; update the branch in the TopicMediaStrip component to either render a generic video thumbnail/icon (e.g., replace the <img src={url}> with a static poster/placeholder image or SVG) or use a <video> element with a poster attribute to show an extracted thumbnail (and keep the FaPlay overlay); ensure you reference the same url variable for the poster/source or swap to a known placeholder asset so the thumbnail is visible for .mp4/.mov inputs.src/lib/mediaUtils.ts-296-304 (1)
296-304:⚠️ Potential issue | 🟡 MinorUnknown image URLs are treated as opaque now.
The docstring says unknown extensions should default to
true, but the implementation only returnstruefor an allowlist. Extensionless/CDN URLs will lose the transparency grid even when the decoded asset actually has alpha.Suggested fix
export function imageCanHaveTransparency(url: string): boolean { const path = url.split("?")[0].split("#")[0].toLowerCase(); - return /\.(png|gif|webp|avif|svg)$/.test(path); + return !/\.(jpe?g|pdf)$/.test(path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mediaUtils.ts` around lines 296 - 304, The function imageCanHaveTransparency currently allowlists only known transparent extensions and treats unknown/extensionless URLs as opaque; change it to return false only for definitively opaque formats (e.g., .jpg, .jpeg, .jfif, .pdf) and return true for everything else. In the imageCanHaveTransparency function, keep the existing path normalization (strip query/hash and toLowerCase), then test for an explicit opaque-extension regex (match /\.(jpe?g|jfif|pdf)$/) and return false when that matches; otherwise return true so extensionless/CDN URLs and unknown extensions preserve the transparency grid as documented.
🧹 Nitpick comments (13)
src/components/message/CollapsedEventMessage.tsx (2)
32-36: Minor:formatTimecreates a newIntl.DateTimeFormaton every call.Consider moving the formatter outside the component to avoid repeated instantiation:
♻️ Suggested refactor
+const timeFormatter = new Intl.DateTimeFormat("en-US", { + hour: "2-digit", + minute: "2-digit", +}); + export const CollapsedEventMessage: React.FC<CollapsedEventMessageProps> = ({ // ... }) => { // ... - const formatTime = (date: Date) => - new Intl.DateTimeFormat("en-US", { - hour: "2-digit", - minute: "2-digit", - }).format(date); + const formatTime = (date: Date) => timeFormatter.format(date);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/CollapsedEventMessage.tsx` around lines 32 - 36, formatTime currently constructs a new Intl.DateTimeFormat on every call which is wasteful; move the formatter out of the component scope as a module-level constant (e.g., const timeFormatter = new Intl.DateTimeFormat("en-US", { hour: "2-digit", minute: "2-digit" });) and update formatTime to call timeFormatter.format(date) so the formatter is reused across renders; adjust references to formatTime in CollapsedEventMessage (and any exports) accordingly.
119-121: Redundant.map((u) => u)call.The map operation returns each element unchanged, which has no effect.
♻️ Suggested fix
const summary = eventGroup.usernames - ? uniqueUsernames.map((u) => u).join(", ") + ? uniqueUsernames.join(", ") : "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/CollapsedEventMessage.tsx` around lines 119 - 121, The current computation of summary performs a no-op map over uniqueUsernames; update the assignment that builds summary (the variable named summary in CollapsedEventMessage) to remove the redundant uniqueUsernames.map((u) => u) and directly call uniqueUsernames.join(", ") when eventGroup.usernames is truthy, keeping the conditional on eventGroup.usernames unchanged.src/hooks/useAutoFocusTyping.ts (2)
14-15: Reword this comment to the actual reason.Lines 14-15 partly restate what
insertText()does, and the React sync is really driven by the dispatchedinputevent, not just the setter. A single why-focused comment would be clearer.✏️ Suggested wording
-// Insert text into a React-controlled input/textarea at the current cursor position. -// Using the native prototype setter triggers React's synthetic onChange. +// Use the native value setter and dispatch `input` so React-controlled chat inputs observe the paste.Based on learnings, "Comments should explain why, not what — omit comments entirely if code is readable; keep to one line and write in project context".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useAutoFocusTyping.ts` around lines 14 - 15, Replace the two-line comment above insertText() with a single why-focused line explaining that we use the native value setter and dispatch a native 'input' event so React's synthetic onChange detects the programmatic value change (React doesn't observe direct DOM value assignments). Mention insertText() and the dispatched 'input' event by name so future readers know the mechanism being relied upon.
16-35: SafeguardsetSelectionRange()for potential non-text input types.Line 34 calls
setSelectionRange(), which only works on text-capable inputs. While current call sites (MediaCommentsSidebar and ChatArea) both useHTMLTextAreaElement, the function signature accepts anyHTMLInputElement. If this helper is reused with non-text input types (e.g.,type="number"), the call will throw. Consider adding a guard to safely handle such cases:const canRestoreSelection = input instanceof HTMLTextAreaElement || ["text", "search", "url", "tel", "password"].includes(input.type); // ... later ... if (canRestoreSelection) { input.setSelectionRange(start + text.length, start + text.length); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useAutoFocusTyping.ts` around lines 16 - 35, The helper insertText accepts HTMLInputElement but unconditionally calls input.setSelectionRange which can throw for non-text input types; update insertText to detect if the element supports text selection (e.g., input instanceof HTMLTextAreaElement OR input.type in ["text","search","url","tel","password"]) before calling setSelectionRange, and only restore the selection when that guard allows it; retain the existing value insertion and input event dispatch behavior for all inputs.src/components/ui/TopicModal.tsx (1)
33-36: Consider adding feedback for topic update failures.The
handleSavefunction callsircClient.setTopic()and immediately closes the modal without awaiting success or handling potential errors. If the server rejects the topic change (e.g., due to permissions or rate limiting), the user won't know.If
setTopicreturns a promise or the IRC client emits error events, consider providing user feedback on failure rather than optimistically closing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/TopicModal.tsx` around lines 33 - 36, handleSave currently calls ircClient.setTopic(serverId, channel.name, editedTopic) and immediately calls onClose without awaiting success or handling errors; change handleSave to await the setTopic promise (or subscribe to the client's error event) and catch failures, display a user-visible error (toast or inline error state tied to editedTopic/modal) and only call onClose on success, referencing handleSave, ircClient.setTopic, onClose, editedTopic, serverId and channel.name to locate the change.src/components/layout/ChatHeader.tsx (1)
255-263: ThehasMediacheck is a rough heuristic.The check
m.content.includes("http")will match any message containing "http" anywhere, including false positives like text mentioning the protocol without being a URL. This seems intentional for performance (avoiding full URL parsing), but worth documenting.Consider adding a brief comment explaining this is an intentional approximation for performance, or use a slightly more precise regex like
/https?:\/\//if false positives become problematic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/ChatHeader.tsx` around lines 255 - 263, The heuristic using m.content.includes("http") and selectedChannel?.topic?.includes("http") in hasMediaInMessages/hasMedia is intentionally simple and can yield false positives; either add a concise inline comment near the hasMediaInMessages and hasMedia definitions explaining this is a performance-driven approximation, or replace the checks with a slightly stricter pattern (e.g., test against /https?:\/\//) to reduce false positives—update references to hasMediaInMessages, hasMedia, and the m.content.includes("http") usage accordingly.src/components/ui/TopicMediaStrip.tsx (1)
58-82: Consider limiting concurrent media probes.The effect fires
probeMediaUrl()for all unprobed candidates in parallel. If a topic contains many URLs, this could result in a burst of network requests. Consider adding a concurrency limit or processing sequentially.This is a minor concern since topics typically contain few URLs, but worth considering if topics with many links are common.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/TopicMediaStrip.tsx` around lines 58 - 82, The effect that iterates over candidates calls probeMediaUrl for every unprobed entry in parallel, which can spike network requests; modify the useEffect logic to process unprobed entries with a concurrency limit (or sequentially) by batching or using a simple queue/worker pattern: when iterating candidates in the effect, collect URLs that need probing (where getCachedProbeResult returns undefined) and then run probeMediaUrl for those URLs with a limiter (e.g., Promise.all with chunks or a small worker loop) rather than firing all promises at once; ensure you still honor the cancelled flag and update setResolvedTypes exactly as currently done when each probe finishes, and reference probeMediaUrl, getCachedProbeResult, setResolvedTypes, and candidates when locating the change.src/components/ui/UserSettings.tsx (1)
748-749: Consider clamping the level value for safety.If
settings.mediaVisibilityLevelis corrupted or out of expected range (e.g., from a future migration), accessingLEVELS[level]could be undefined. A simple clamp ensures robustness:🛡️ Suggested defensive clamp
- const level = (settings.mediaVisibilityLevel as number | undefined) ?? 1; - const current = LEVELS[level as 0 | 1 | 2 | 3]; + const rawLevel = (settings.mediaVisibilityLevel as number | undefined) ?? 1; + const level = Math.max(0, Math.min(3, rawLevel)); + const current = LEVELS[level];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/UserSettings.tsx` around lines 748 - 749, The code uses settings.mediaVisibilityLevel to index LEVELS via const level and const current, which can be out-of-range; clamp the computed level to the valid index range (0..3) before indexing LEVELS (e.g., compute a safeLevel = Math.max(0, Math.min(3, Number(settings.mediaVisibilityLevel) || 1)) or similar) and then use LEVELS[safeLevel] for current so undefined is never returned if the stored value is corrupted or from a future migration.AGENTS.md (2)
111-119: Add language specifier to test layout code block.Same issue as above — add
textorplaintextspecifier.📝 Proposed fix
-``` +```text tests/ hooks/ # React hook tests (renderHook + act)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 111 - 119, The fenced code block showing the test directory layout is missing a language specifier; update the opening triple-backtick for that block (the block that begins with the "tests/" listing) to include a plain text specifier (e.g., change "```" to "```text" or "```plaintext") so the snippet is rendered as plain text; ensure only the opening fence is modified and the rest of the "tests/ ... setup.ts" lines remain unchanged.
24-56: Add language specifier to fenced code block.The project layout code block lacks a language specifier. Use
textorplaintextfor directory structures to satisfy markdown linting.📝 Proposed fix
-``` +```text src/ components/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 24 - 56, The fenced directory-structure code block in AGENTS.md is missing a language specifier; update the opening triple-backticks for the shown snippet to include a plaintext language (e.g., ```text) so Markdown linting passes and the directory tree is rendered correctly.src/store/handlers/metadata.ts (1)
236-240: Remove empty conditional block (dead code).Lines 239-240 contain an empty
ifblock that serves no purpose.🧹 Proposed fix
const updatedUsers = channel.users.map((user) => { if (user.username === resolvedTarget) { - const existingValue = user.metadata?.[key]?.value; - const existingVisibility = user.metadata?.[key]?.visibility; - - const userInChannel = channel.users.find( - (u) => u.username === resolvedTarget, - ); - if (userInChannel) { - } - - const updatedUsers = channel.users.map((user) => { - if (user.username === resolvedTarget) { const existingValue = user.metadata?.[key]?.value; const existingVisibility = user.metadata?.[key]?.visibility;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/metadata.ts` around lines 236 - 240, Remove the dead empty conditional: the if (userInChannel) { } block after const userInChannel = channel.users.find((u) => u.username === resolvedTarget) is unused and should be deleted (or replaced with needed logic if there was intended behavior). Locate the userInChannel check in the metadata handler and either remove the entire empty if block or implement the intended handling for userInChannel (e.g., perform the original action on userInChannel) so there are no empty conditionals left.src/components/layout/ChatArea.tsx (1)
1674-1692: Use cachedisNativeMobileRefinstead of callingisTauriMobile()directly.Line 1675 calls
isTauriMobile()directly, but line 185 already caches this value inisNativeMobileRefto avoid adding it to callback deps. For consistency and to avoid redundant calls, use the cached ref here.♻️ Proposed fix
useEffect(() => { - if (isTauriMobile()) return; + if (isNativeMobileRef.current) return; // Don't steal focus if any modal is open🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/ChatArea.tsx` around lines 1674 - 1692, The effect currently calls isTauriMobile() directly; replace that call with the cached ref check using isNativeMobileRef.current to avoid redundant calls and extra deps. In the useEffect that focuses inputRef (the effect referencing selectedChannelId, selectedPrivateChatId, isSettingsModalOpen, userProfileModalOpen, isAddServerModalOpen, isChannelListModalOpen), change the early return from checking isTauriMobile() to checking isNativeMobileRef.current so the hook remains stable and uses the cached value.src/lib/settings/definitions/allSettings.ts (1)
391-417: Type mismatch:togglewith numeric default.The
type: "toggle"combined withdefaultValue: 1(a number) is semantically inconsistent. The comment explains this entry exists for search discoverability while the actual UI is a custom slider, but this could confuse future maintainers. Consider using a more descriptive type like"custom"or"slider"to better reflect the actual rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/settings/definitions/allSettings.ts` around lines 391 - 417, The mediaSettings entry for id "media.visibilityLevel" / key "mediaVisibilityLevel" uses type: "toggle" but has a numeric defaultValue (1); change the type to a more accurate value (e.g., "slider" or "custom") in the mediaSettings array so the setting's type matches its numeric semantics and update any consumers/schema that validate SettingDefinition.type (e.g., UI renderer or validation code that reads SettingDefinition) to handle "slider"/"custom" for this setting; keep the existing defaultValue and searchKeywords intact to preserve discoverability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/Cargo.toml`:
- Line 50: Restore the tokio feature on the rfd dependency in Cargo.toml so
async file dialogs keep working: change the existing rfd = "0.16" entry to use
the feature-enabled form (rfd with features = ["tokio"]) so the crate builds
with tokio-backed async support; update the rfd dependency line in Cargo.toml
accordingly.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`:
- Around line 33-37: The getArgs() function currently force-unwraps the UTF-8
Data and force-casts the JSON result to NSDictionary causing crashes; change it
to validate each step and throw errors instead: convert self.data to Data using
a guard/if-let and throw a parsing error if that fails, call JSONSerialization
and verify the result is a dictionary (e.g., check if it can be bridged to
NSDictionary) and throw a type error if not, and call
JSTypes.coerceDictionaryToJSObject only after successful validation and
propagate/throw any error instead of force-unwrapping the result; reference
getArgs(), JSONSerialization.jsonObject(...), and
JSTypes.coerceDictionaryToJSObject when making these checks and error throws.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift`:
- Around line 21-22: The PluginManager singleton currently holds a strong
reference to the view controller via the public var viewController:
UIViewController?, causing a retain cycle/leak; change the declaration on
PluginManager (the shared instance) to use a weak reference (weak var
viewController: UIViewController?) so the view controller can be deallocated
when dismissed, and ensure any assignments to viewController still use optional
semantics.
- Around line 79-90: The NSError is being passed as an OpaquePointer which loses
Objective-C autorelease semantics and forces the unsafe passRetained leak;
instead, change the invoked function pointer type to accept an
AutoreleasingUnsafeMutablePointer<NSError?> and call it with such a pointer.
Concretely, inside the block that currently uses withUnsafeMutablePointer(to:
&error) { ... OpaquePointer($0) }, construct an
AutoreleasingUnsafeMutablePointer<NSError?>(errorPtr) and unsafeBitCast
methodIMP to a function type (Any?, Selector, Invoke,
AutoreleasingUnsafeMutablePointer<NSError?>) -> Void, then call it with
plugin.instance, selectorWithThrows, invoke, and that autoreleasing pointer;
finally remove the Unmanaged.passRetained(error) leak and keep the existing if
let error { invoke.reject("\(error)") } handling.
In `@src/components/message/CollapsibleMessage.tsx`:
- Around line 48-54: The effect in useLayoutEffect schedules
requestAnimationFrame to set allowTransitionRef.current = true but never cancels
it and the zero-height guard incorrectly treats legitimately empty content as
display:none; fix by storing the RAF id when calling requestAnimationFrame and
call cancelAnimationFrame(id) in the effect cleanup (and ensure the
ResizeObserver is disconnected there too), and replace the dual
clientHeight/scrollHeight check with a more precise hidden-layout test (e.g.,
check getComputedStyle(element).display === 'none' or element.offsetParent ===
null) or, if you must keep the existing numeric check, reset needsCollapsing and
contentHeight to their empty-state values before returning so the fade
overlay/button are cleared; update references in the effect to
allowTransitionRef, measuredContent, element, needsCollapsing, and contentHeight
accordingly.
In `@src/components/message/MediaPreview.tsx`:
- Around line 400-405: The effect currently calls
useStore.getState().stopActiveMedia() whenever loadError changes, which can stop
unrelated playback; update the effect to first verify this preview is the active
media before stopping global playback — e.g., read the store's active media
identifier or "isActive" flag (via useStore.getState().activeMediaId or a
provided selector) and only call stopActiveMedia() when that identifier/flag
matches this preview's id/current instance; keep the useEffect and dependency on
loadError but add the guard so only the failed, currently-active preview kills
the mini-player.
In `@src/lib/irc/IRCClient.ts`:
- Around line 522-539: The normalization for ws(s):// in IRCClient.ts is
dropping path and query by extracting only hostname and port; update the parsing
in the block that checks host.startsWith("wss://") / host.startsWith("ws://") to
use the URL constructor (new URL(host)) to extract hostname, port, pathname and
search, set actualHost to url.hostname and actualPort to url.port || port, and
preserve url.pathname + url.search when constructing the final socket endpoint
used by createSocket (so the final URL includes path/query and correct
protocol/port).
- Around line 637-663: The onclose handler treats all closes as unexpected and
always starts a reconnect via startReconnection; modify logic to skip
reconnection when the close was initiated by disconnect(): add/read an explicit
flag on the Server object (e.g., server.isDisconnecting or
server.desiredConnectionState = "disconnecting") that disconnect() sets before
closing the socket, and have socket.onclose check that flag (or the
desiredConnectionState) and clear it instead of calling startReconnection; keep
the existing cleanup (stopWebSocketPing, sockets.delete,
pendingConnections.delete, triggerEvent) but only call startReconnection when
the close was not intentional.
- Around line 1009-1019: The code currently calls sendMultilineMessage whenever
content contains '\n' (in sendMessage), which can emit BATCH traffic even if the
server never negotiated draft/multiline; change sendMessage to first query the
negotiated capability (use the existing capability helper or store lookup, e.g.,
supportsMultiline(serverId) or the capability-check helper used elsewhere) and
only call sendMultilineMessage when the server has ACKed draft/multiline;
otherwise split the content by '\n' and send each line via sendRaw(`PRIVMSG
${channel.name} :${line}`) to gracefully degrade to plain PRIVMSG lines.
In `@src/lib/useMediaController.ts`:
- Around line 95-102: The cleanup effect currently only compares url before
calling useStore.getState().stopActiveMedia(), which can stop the wrong player
when identical URLs exist; change the predicate inside the returned cleanup to
match the full active-media identity (url, type, msgid) — i.e., reuse the same
identity check used for isActive (compare url, type and msgid against
useStore.getState().ui.activeMedia) and only call stopActiveMedia() when all
three fields match; keep this logic inside the useEffect that depends on [url,
type, msgid, stopOnUnmount].
In `@src/store/handlers/auth.ts`:
- Around line 180-191: The CAP handling branch only sets preventCapEnd when
savedServer.saslAccountName exists, but AUTHENTICATE will use serv.nickname when
saslAccountName is empty so nickname-based SASL still needs CAP END held back;
update the check in the caps handling (the block that reads
storage.servers.load() and inspects savedServer) to set preventCapEnd = true
whenever savedServer.saslEnabled is true and a saslPassword is present (i.e.,
require saslPassword rather than saslAccountName), so that nickname-based SASL
(AUTHENTICATE using serv.nickname) prevents CAP END and delays userOnConnect()
until auth completes.
In `@src/store/handlers/batches.ts`:
- Around line 271-380: The chathistory branch only looks up a channel by
comparing batch.parameters[0] to server.channels names, so batches for
private/pinned PMs (where the target is a username) never merge and buffered
items (chathistoryBuffers, reactionBuffers) are left dangling; update the logic
in the chathistory handler to also resolve a PM target: after the existing
channel lookup (using server.channels.find by name), if no channel is found try
to locate the private/direct channel for that username (e.g., check channel.type
=== 'pm' or channel.participants/includes the username, or
channel.otherUser/username fields depending on your channel shape), then proceed
to compute key, merge pending/newMessages, apply pendingReactions, update
processedMessageIds and update that specific channel's
hasMoreHistory/isLoadingHistory in servers.map exactly like the existing code
uses for the public channel; ensure you still delete
chathistoryBuffers.get(batchId) and reactionBuffers.get(batchId) when resolved
so buffered data is flushed.
In `@src/store/handlers/channels.ts`:
- Around line 121-125: The handler is mutating existing arrays (e.g., ch.bans,
ch.invites, ch.exceptions) by reusing them and calling push; change it to
produce new arrays immutably: when reading ch.bans (or ch.invites/ch.exceptions)
create a new array with spread/concat and conditionally include the new entry
instead of push, then return a new channel object ({ ...ch, bans: newBans }) so
store.setState() never mutates the original state; update the same pattern in
the other branches referenced (the blocks around the ch.invites and
ch.exceptions uses) to ensure all additions use immutable copies.
- Around line 393-410: The metadata fetch loop is reading channelData from
state.servers so it misses newly inserted users created in the updatedServers
tree (e.g., newUsers from your NAMES handling), causing metadataList(serverId,
user.username) not to be called for them; update the logic to iterate the
channel users from the updatedServers structure you built (use the same
updatedServers/updatedServer/updatedChannel or newUsers collection produced
during NAMES processing) instead of state.servers so that for each user where
user.metadata is empty you call store.getState().metadataList(serverId,
user.username); keep the existing serverSupportsMetadata(serverId) check and
preserve the hasMetadata test.
In `@src/store/handlers/connection.ts`:
- Around line 107-116: The current handler is mutating the existing store object
by doing const updatedChannelMetadata = channel.metadata || {} and then
Object.assign(updatedChannelMetadata, channelMetadata) inside the setState
callback; instead clone channel.metadata first and merge into a new object so
you don't mutate state—e.g., create a new object for updatedChannelMetadata from
channel.metadata (or empty) and then merge channelMetadata into that (using
object spread or Object.assign into a fresh object) before returning it in the
updated channel; ensure this change is applied where channelMetadata,
updatedChannelMetadata, Object.assign and the setState callback are used so the
original state object is never mutated.
In `@src/store/handlers/messages.ts`:
- Around line 88-104: The own-message check uses strict equality between
response.sender and currentServerUser?.username which is wrong for IRC nicks
(case-insensitive); create a helper like isSameNick(a,b) that normalizes both
nicks to a canonical form (lowercase or proper IRC casemapping) and use it in
place of the direct comparison where you compute isOwnMessage and in the other
repeated occurrences in this file; update the isOwnMessage assignment and any
later comparisons before calling checkForMention and extractMentions to call
isSameNick(response.sender, currentServerUser?.username).
- Around line 358-584: MULTILINE_MESSAGE handling currently only adds the
message and plays sounds, but it omits mention detection and unread bumping so
highlights/DM badges break; update the MULTILINE_MESSAGE code paths (the branch
that builds newMessage, the idsToTrack/processedMessageIds update, and the
post-addMessage logic) to run the same mention calculation and unread/mention
bumping that CHANMSG/USERMSG use: detect mentions for newMessage.mentioned,
increment channel.unreadCount or privateChat.unreadCount and set isMentioned
when appropriate, and ensure shouldPlayNotificationSound/playNotificationSound
are invoked for non-historical messages after computing mentions/unread (reuse
resolveReplyMessage, shouldPlayNotificationSound, playNotificationSound and the
server/privateChat update patterns already used elsewhere in this file).
- Around line 424-442: The handler in messages.ts writes multiline messages
directly to the store via store.getState().addMessage(newMessage) and only
suppresses sound afterwards, which bypasses the chathistory batching used by
CHANMSG/TAGMSG and can surface messages early; change the flow so that when
mtags?.batch (isHistoricalMessage) is true the multiline message is routed into
the same batch buffer used by the CHANMSG/TAGMSG path (instead of calling
addMessage directly), preserving the CHATHISTORY_LOADING behavior and ensuring
no notification/unread counting; locate the logic around addMessage,
isHistoricalMessage, and mtags and replace the direct store.addMessage call with
a call into the existing batch buffer enqueue logic (or factor out a shared
helper) so historical multiline messages follow the same buffering and playback
ordering as single-line chathistory messages.
---
Outside diff comments:
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`:
- Around line 47-52: The current serialize(_ data: JsonValue) implementation
returns error text via manual string interpolation ("\"\(error)\""), which can
produce invalid JSON; replace that interpolation with a proper JSON-escaped
string by encoding the error through your JSON serializer (e.g., create a
JsonValue string from error.localizedDescription or use JSONEncoder to encode
the error string) and then call jsonRepresentation() so the result is valid
JSON; make the same change to the other serialize implementation that returns
error strings (the duplicate/error-path serialize) so both use the JSON-safe
path instead of manual interpolation.
- Around line 87-100: The payload currently stores the raw Error instance which
breaks JSON serialization; update the block that assigns the error (the local
variable error used before payload["error"]) to convert it to a
JSON-serializable value (for example error.localizedDescription or a small
dictionary like ["message": error.localizedDescription, "nsError": (error as
NSError).domain] or include code: (error as NSError).code) before inserting into
payload, so sendResponse(self.error, serialize(.dictionary(payload as!
JsonObject))) receives only JSON-compatible types; modify the Error assignment
logic accordingly in the same scope where payload is populated and before
serialize/sendResponse is called.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift`:
- Around line 41-45: onWebviewCreated(_:) calls each plugin handle's
load(webview:) but never updates the handle.loaded flag, causing repeated
initialization; after successfully calling handle.instance.load(webview:
webview) set handle.loaded = true on that handle (and only after successful
initialization) so subsequent onWebviewCreated(_:) calls skip already-loaded
plugins; update the loop that iterates plugins to flip handle.loaded to true
immediately after load(webview:) (or inside a success path) to prevent duplicate
observers/handlers.
In `@src/components/message/CollapsibleMessage.tsx`:
- Around line 65-77: The effect schedules requestAnimationFrame inside measure
causing stray frames to run; change measure to store the RAF id (e.g., rafIdRef
or local rafId) and call cancelAnimationFrame on any existing id before queuing
a new requestAnimationFrame, and also cancelAnimationFrame in the effect cleanup
alongside resizeObserver.disconnect(); update references in measure,
allowTransitionRef, and the ResizeObserver cleanup so no stale RAF can flip
allowTransitionRef.current after reruns or unmounts.
In `@src/lib/mediaUtils.ts`:
- Around line 286-291: The branch gated by settings.showTrustedSourcesMedia
currently allows any hostname listed in TRUSTED_EMBED_DOMAINS without consulting
the more strict detection logic; update the logic in the block that uses
extractHostname(url) so it also calls detectTrustedDomainType(url) (or the same
internal check used by detectTrustedDomainType) and only returns true when that
function indicates an embeddable/trusted domain type (e.g., "embed" or the
specific allowed enum), otherwise return false so non-embeddable pages (like
generic YouTube/Vimeo pages) are not treated as probeable trusted media.
---
Minor comments:
In `@src/components/layout/ChatHeader.tsx`:
- Around line 297-306: The overflow menu "Media" item in ChatHeader.tsx uses
openMediaExplorer(selectedServerId, selectedChannelId) which fails for private
chats because selectedChannelId is null; update the onClick handler to pass
selectedPrivateChat?.id when selectedChannelId is null (e.g., compute channelId
= selectedChannelId ?? selectedPrivateChat?.id) and call
openMediaExplorer(selectedServerId, channelId) so the menu matches the inline
private-chat button behavior (references: openMediaExplorer, selectedServerId,
selectedChannelId, selectedPrivateChat, hasMedia).
In `@src/components/ui/TopicMediaStrip.tsx`:
- Around line 163-172: In TopicMediaStrip when handling type === "video" you are
using <img src={url}> which won't render video files; update the branch in the
TopicMediaStrip component to either render a generic video thumbnail/icon (e.g.,
replace the <img src={url}> with a static poster/placeholder image or SVG) or
use a <video> element with a poster attribute to show an extracted thumbnail
(and keep the FaPlay overlay); ensure you reference the same url variable for
the poster/source or swap to a known placeholder asset so the thumbnail is
visible for .mp4/.mov inputs.
In `@src/lib/eventGrouping.ts`:
- Around line 28-48: The final unconditional return "quit" in computeUserSummary
is unreachable; remove it and replace with an explicit unreachable assertion to
make intent clear: in computeUserSummary (use the function name to find it)
delete the last line returning "quit" and instead throw a clear error like throw
new Error("computeUserSummary: unreachable") (or use a language-specific
unreachable/assert helper) so any future control-flow changes fail loudly.
In `@src/lib/mediaUtils.ts`:
- Around line 296-304: The function imageCanHaveTransparency currently
allowlists only known transparent extensions and treats unknown/extensionless
URLs as opaque; change it to return false only for definitively opaque formats
(e.g., .jpg, .jpeg, .jfif, .pdf) and return true for everything else. In the
imageCanHaveTransparency function, keep the existing path normalization (strip
query/hash and toLowerCase), then test for an explicit opaque-extension regex
(match /\.(jpe?g|jfif|pdf)$/) and return false when that matches; otherwise
return true so extensionless/CDN URLs and unknown extensions preserve the
transparency grid as documented.
---
Duplicate comments:
In `@src/lib/irc/IRCClient.ts`:
- Around line 885-893: Existing pong timeouts must be cleared before scheduling
a new one to avoid stale timers closing the socket; in the IRCClient class where
you create the pong timeout (the code using this.pongTimeouts.set(serverId,
pongTimeout)), first check this.pongTimeouts.get(serverId) and clearTimeout on
it (and delete the map entry) before calling setTimeout, and also ensure you
remove/clear the timeout when you close the socket so no leftover timer can
later call socket.close().
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 111-119: The fenced code block showing the test directory layout
is missing a language specifier; update the opening triple-backtick for that
block (the block that begins with the "tests/" listing) to include a plain text
specifier (e.g., change "```" to "```text" or "```plaintext") so the snippet is
rendered as plain text; ensure only the opening fence is modified and the rest
of the "tests/ ... setup.ts" lines remain unchanged.
- Around line 24-56: The fenced directory-structure code block in AGENTS.md is
missing a language specifier; update the opening triple-backticks for the shown
snippet to include a plaintext language (e.g., ```text) so Markdown linting
passes and the directory tree is rendered correctly.
In `@src/components/layout/ChatArea.tsx`:
- Around line 1674-1692: The effect currently calls isTauriMobile() directly;
replace that call with the cached ref check using isNativeMobileRef.current to
avoid redundant calls and extra deps. In the useEffect that focuses inputRef
(the effect referencing selectedChannelId, selectedPrivateChatId,
isSettingsModalOpen, userProfileModalOpen, isAddServerModalOpen,
isChannelListModalOpen), change the early return from checking isTauriMobile()
to checking isNativeMobileRef.current so the hook remains stable and uses the
cached value.
In `@src/components/layout/ChatHeader.tsx`:
- Around line 255-263: The heuristic using m.content.includes("http") and
selectedChannel?.topic?.includes("http") in hasMediaInMessages/hasMedia is
intentionally simple and can yield false positives; either add a concise inline
comment near the hasMediaInMessages and hasMedia definitions explaining this is
a performance-driven approximation, or replace the checks with a slightly
stricter pattern (e.g., test against /https?:\/\//) to reduce false
positives—update references to hasMediaInMessages, hasMedia, and the
m.content.includes("http") usage accordingly.
In `@src/components/message/CollapsedEventMessage.tsx`:
- Around line 32-36: formatTime currently constructs a new Intl.DateTimeFormat
on every call which is wasteful; move the formatter out of the component scope
as a module-level constant (e.g., const timeFormatter = new
Intl.DateTimeFormat("en-US", { hour: "2-digit", minute: "2-digit" });) and
update formatTime to call timeFormatter.format(date) so the formatter is reused
across renders; adjust references to formatTime in CollapsedEventMessage (and
any exports) accordingly.
- Around line 119-121: The current computation of summary performs a no-op map
over uniqueUsernames; update the assignment that builds summary (the variable
named summary in CollapsedEventMessage) to remove the redundant
uniqueUsernames.map((u) => u) and directly call uniqueUsernames.join(", ") when
eventGroup.usernames is truthy, keeping the conditional on eventGroup.usernames
unchanged.
In `@src/components/ui/TopicMediaStrip.tsx`:
- Around line 58-82: The effect that iterates over candidates calls
probeMediaUrl for every unprobed entry in parallel, which can spike network
requests; modify the useEffect logic to process unprobed entries with a
concurrency limit (or sequentially) by batching or using a simple queue/worker
pattern: when iterating candidates in the effect, collect URLs that need probing
(where getCachedProbeResult returns undefined) and then run probeMediaUrl for
those URLs with a limiter (e.g., Promise.all with chunks or a small worker loop)
rather than firing all promises at once; ensure you still honor the cancelled
flag and update setResolvedTypes exactly as currently done when each probe
finishes, and reference probeMediaUrl, getCachedProbeResult, setResolvedTypes,
and candidates when locating the change.
In `@src/components/ui/TopicModal.tsx`:
- Around line 33-36: handleSave currently calls ircClient.setTopic(serverId,
channel.name, editedTopic) and immediately calls onClose without awaiting
success or handling errors; change handleSave to await the setTopic promise (or
subscribe to the client's error event) and catch failures, display a
user-visible error (toast or inline error state tied to editedTopic/modal) and
only call onClose on success, referencing handleSave, ircClient.setTopic,
onClose, editedTopic, serverId and channel.name to locate the change.
In `@src/components/ui/UserSettings.tsx`:
- Around line 748-749: The code uses settings.mediaVisibilityLevel to index
LEVELS via const level and const current, which can be out-of-range; clamp the
computed level to the valid index range (0..3) before indexing LEVELS (e.g.,
compute a safeLevel = Math.max(0, Math.min(3,
Number(settings.mediaVisibilityLevel) || 1)) or similar) and then use
LEVELS[safeLevel] for current so undefined is never returned if the stored value
is corrupted or from a future migration.
In `@src/hooks/useAutoFocusTyping.ts`:
- Around line 14-15: Replace the two-line comment above insertText() with a
single why-focused line explaining that we use the native value setter and
dispatch a native 'input' event so React's synthetic onChange detects the
programmatic value change (React doesn't observe direct DOM value assignments).
Mention insertText() and the dispatched 'input' event by name so future readers
know the mechanism being relied upon.
- Around line 16-35: The helper insertText accepts HTMLInputElement but
unconditionally calls input.setSelectionRange which can throw for non-text input
types; update insertText to detect if the element supports text selection (e.g.,
input instanceof HTMLTextAreaElement OR input.type in
["text","search","url","tel","password"]) before calling setSelectionRange, and
only restore the selection when that guard allows it; retain the existing value
insertion and input event dispatch behavior for all inputs.
In `@src/lib/settings/definitions/allSettings.ts`:
- Around line 391-417: The mediaSettings entry for id "media.visibilityLevel" /
key "mediaVisibilityLevel" uses type: "toggle" but has a numeric defaultValue
(1); change the type to a more accurate value (e.g., "slider" or "custom") in
the mediaSettings array so the setting's type matches its numeric semantics and
update any consumers/schema that validate SettingDefinition.type (e.g., UI
renderer or validation code that reads SettingDefinition) to handle
"slider"/"custom" for this setting; keep the existing defaultValue and
searchKeywords intact to preserve discoverability.
In `@src/store/handlers/metadata.ts`:
- Around line 236-240: Remove the dead empty conditional: the if (userInChannel)
{ } block after const userInChannel = channel.users.find((u) => u.username ===
resolvedTarget) is unused and should be deleted (or replaced with needed logic
if there was intended behavior). Locate the userInChannel check in the metadata
handler and either remove the entire empty if block or implement the intended
handling for userInChannel (e.g., perform the original action on userInChannel)
so there are no empty conditionals left.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+48
to
+54
| // Skip when display:none collapses all dimensions — would corrupt state. | ||
| // Both must be 0: jsdom always has clientHeight=0, so the dual condition | ||
| // ensures we only bail in genuine display:none (scrollHeight also collapses). | ||
| if (element.clientHeight === 0 && measuredContent.scrollHeight === 0) { | ||
| allowTransitionRef.current = false; | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/message/CollapsibleMessage.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 6846
Missing RAF cleanup and zero-height guard causes state inconsistencies.
Two issues in the useLayoutEffect:
-
Uncanceled RAF callback (lines 65-67): The
requestAnimationFrameschedulingallowTransitionRef.current = trueis never canceled in the cleanup. WhenResizeObserverfires multiple times, multiple RAF callbacks queue and can execute out of order or after unmount. Cancel this in the cleanup function per the coding guideline: "Always cancel nestedrequestAnimationFramecalls in effect cleanup to avoid post-unmountsetStateerrors." -
Zero-height guard matches empty content (lines 51-54): The condition
element.clientHeight === 0 && measuredContent.scrollHeight === 0also matches genuinely empty content, not justdisplay:none. When content becomes legitimately empty, the guard returns early and skips state updates, leavingneedsCollapsingandcontentHeightstale. The fade overlay and button can then remain visible despite no content. Either reset collapse state before returning or check for a hidden-layout flag that does not also match empty content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/message/CollapsibleMessage.tsx` around lines 48 - 54, The
effect in useLayoutEffect schedules requestAnimationFrame to set
allowTransitionRef.current = true but never cancels it and the zero-height guard
incorrectly treats legitimately empty content as display:none; fix by storing
the RAF id when calling requestAnimationFrame and call cancelAnimationFrame(id)
in the effect cleanup (and ensure the ResizeObserver is disconnected there too),
and replace the dual clientHeight/scrollHeight check with a more precise
hidden-layout test (e.g., check getComputedStyle(element).display === 'none' or
element.offsetParent === null) or, if you must keep the existing numeric check,
reset needsCollapsing and contentHeight to their empty-state values before
returning so the fade overlay/button are cleared; update references in the
effect to allowTransitionRef, measuredContent, element, needsCollapsing, and
contentHeight accordingly.
Comment on lines
+358
to
+584
| if (channel) { | ||
| const channelKey = `${server.id}-${channel.id}`; | ||
| const replyMessage = resolveReplyMessage( | ||
| mtags, | ||
| server.id, | ||
| channel.id, | ||
| store.getState().messages[channelKey] || [], | ||
| ); | ||
|
|
||
| const newMessage = { | ||
| id: uuidv4(), | ||
| msgid: mtags?.msgid, | ||
| multilineMessageIds: messageIds, // Store all message IDs for redaction | ||
| content: message, // Use the properly combined message from IRC client | ||
| timestamp, | ||
| userId: sender, | ||
| channelId: channel.id, | ||
| serverId: server.id, | ||
| type: "message" as const, | ||
| reactions: [], | ||
| replyMessage: replyMessage, | ||
| mentioned: [], // Add logic for mentions if needed | ||
| tags: mtags, | ||
| }; | ||
|
|
||
| // If message has bot tag, mark user as bot | ||
| if (mtags?.bot !== undefined) { | ||
| store.setState((state) => { | ||
| const updatedServers = state.servers.map((s) => { | ||
| if (s.id === server.id) { | ||
| const updatedChannels = s.channels.map((channel) => { | ||
| const updatedUsers = channel.users.map((user) => { | ||
| if (user.username === sender) { | ||
| return { | ||
| ...user, | ||
| isBot: true, | ||
| }; | ||
| } | ||
| return user; | ||
| }); | ||
| return { ...channel, users: updatedUsers }; | ||
| }); | ||
| return { ...s, channels: updatedChannels }; | ||
| } | ||
| return s; | ||
| }); | ||
| return { servers: updatedServers }; | ||
| }); | ||
| } | ||
|
|
||
| // Mark these message IDs as processed to prevent duplicates | ||
| const idsToTrack = | ||
| messageIds && messageIds.length > 0 | ||
| ? messageIds | ||
| : mtags?.msgid | ||
| ? [mtags.msgid] | ||
| : []; | ||
| if (idsToTrack.length > 0) { | ||
| store.setState((state) => ({ | ||
| processedMessageIds: new Set([ | ||
| ...state.processedMessageIds, | ||
| ...idsToTrack, | ||
| ]), | ||
| })); | ||
| } | ||
|
|
||
| store.getState().addMessage(newMessage); | ||
|
|
||
| // Play notification sound if appropriate (but not for historical messages) | ||
| // Don't count unread/mentions for historical messages (batch tag indicates chathistory playback) | ||
| const isHistoricalMessage = mtags?.batch !== undefined; | ||
|
|
||
| if (!isHistoricalMessage) { | ||
| const state = store.getState(); | ||
| const serverCurrentUser = ircClient.getCurrentUser(response.serverId); | ||
| if ( | ||
| shouldPlayNotificationSound( | ||
| newMessage, | ||
| serverCurrentUser, | ||
| state.globalSettings, | ||
| ) | ||
| ) { | ||
| playNotificationSound(state.globalSettings); | ||
| } | ||
| } | ||
|
|
||
| // Remove any typing users from the state | ||
| store.setState((state) => { | ||
| const key = `${server.id}-${channel.id}`; | ||
| const currentUsers = state.typingUsers[key] || []; | ||
| return { | ||
| typingUsers: { | ||
| ...state.typingUsers, | ||
| [key]: currentUsers.filter((u) => u.username !== sender), | ||
| }, | ||
| }; | ||
| }); | ||
| } else if (!channelName) { | ||
| const currentUser = ircClient.getCurrentUser(response.serverId); | ||
|
|
||
| if (currentUser?.username.toLowerCase() === sender.toLowerCase()) { | ||
| // Own message echo — store under DM keyed by `target`, same as USERMSG echo handler. | ||
| if (!target) return; | ||
| const privateChat = server.privateChats?.find( | ||
| (pc) => pc.username.toLowerCase() === target.toLowerCase(), | ||
| ); | ||
| if (privateChat) { | ||
| const privateChatKey = `${server.id}-${privateChat.id}`; | ||
| const newMessage = { | ||
| id: uuidv4(), | ||
| msgid: mtags?.msgid, | ||
| multilineMessageIds: messageIds, | ||
| content: message, | ||
| timestamp, | ||
| userId: sender, | ||
| channelId: privateChat.id, | ||
| serverId: server.id, | ||
| type: "message" as const, | ||
| reactions: [], | ||
| replyMessage: resolveReplyMessage( | ||
| mtags, | ||
| server.id, | ||
| privateChat.id, | ||
| store.getState().messages[privateChatKey] || [], | ||
| ), | ||
| mentioned: [], | ||
| tags: mtags, | ||
| }; | ||
| const idsToTrack = | ||
| messageIds?.length > 0 | ||
| ? messageIds | ||
| : mtags?.msgid | ||
| ? [mtags.msgid] | ||
| : []; | ||
| if (idsToTrack.length > 0) { | ||
| store.setState((state) => ({ | ||
| processedMessageIds: new Set([ | ||
| ...state.processedMessageIds, | ||
| ...idsToTrack, | ||
| ]), | ||
| })); | ||
| } | ||
| store.getState().addMessage(newMessage); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Incoming DM from another user | ||
| let privateChat = server.privateChats.find( | ||
| (chat) => chat.username.toLowerCase() === sender.toLowerCase(), | ||
| ); | ||
| if (!privateChat) { | ||
| const newPrivateChat = { | ||
| id: generateDeterministicId(server.id, sender), | ||
| username: sender, | ||
| serverId: server.id, | ||
| unreadCount: 0, | ||
| isMentioned: false, | ||
| lastActivity: new Date(), | ||
| isPinned: false, | ||
| order: undefined, | ||
| isOnline: false, | ||
| isAway: false, | ||
| }; | ||
| privateChat = newPrivateChat; | ||
| store.setState((state) => ({ | ||
| servers: state.servers.map((s) => | ||
| s.id === server.id | ||
| ? { ...s, privateChats: [...s.privateChats, newPrivateChat] } | ||
| : s, | ||
| ), | ||
| })); | ||
| } | ||
|
|
||
| const privateChatKey = `${server.id}-${privateChat.id}`; | ||
| const newMessage = { | ||
| id: uuidv4(), | ||
| msgid: mtags?.msgid, | ||
| multilineMessageIds: messageIds, | ||
| content: message, | ||
| timestamp, | ||
| userId: sender, | ||
| channelId: privateChat.id, | ||
| serverId: server.id, | ||
| type: "message" as const, | ||
| reactions: [], | ||
| replyMessage: resolveReplyMessage( | ||
| mtags, | ||
| server.id, | ||
| privateChat.id, | ||
| store.getState().messages[privateChatKey] || [], | ||
| ), | ||
| mentioned: [], | ||
| tags: mtags, | ||
| }; | ||
|
|
||
| const idsToTrack = | ||
| messageIds?.length > 0 | ||
| ? messageIds | ||
| : mtags?.msgid | ||
| ? [mtags.msgid] | ||
| : []; | ||
| if (idsToTrack.length > 0) { | ||
| store.setState((state) => ({ | ||
| processedMessageIds: new Set([ | ||
| ...state.processedMessageIds, | ||
| ...idsToTrack, | ||
| ]), | ||
| })); | ||
| } | ||
|
|
||
| store.getState().addMessage(newMessage); | ||
|
|
||
| const isHistoricalMessage = mtags?.batch !== undefined; | ||
| if (!isHistoricalMessage) { | ||
| const state = store.getState(); | ||
| const serverCurrentUser = ircClient.getCurrentUser(response.serverId); | ||
| if ( | ||
| shouldPlayNotificationSound( | ||
| newMessage, | ||
| serverCurrentUser, | ||
| state.globalSettings, | ||
| ) | ||
| ) { | ||
| playNotificationSound(state.globalSettings); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
MULTILINE_MESSAGE skips the unread/mention pipeline.
Unlike CHANMSG and USERMSG, the multiline handler never calculates mentions, bumps channel/DM unread state, or raises browser notifications. On servers with draft/multiline, highlights and private-message badges will silently stop working for multiline messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/messages.ts` around lines 358 - 584, MULTILINE_MESSAGE
handling currently only adds the message and plays sounds, but it omits mention
detection and unread bumping so highlights/DM badges break; update the
MULTILINE_MESSAGE code paths (the branch that builds newMessage, the
idsToTrack/processedMessageIds update, and the post-addMessage logic) to run the
same mention calculation and unread/mention bumping that CHANMSG/USERMSG use:
detect mentions for newMessage.mentioned, increment channel.unreadCount or
privateChat.unreadCount and set isMentioned when appropriate, and ensure
shouldPlayNotificationSound/playNotificationSound are invoked for non-historical
messages after computing mentions/unread (reuse resolveReplyMessage,
shouldPlayNotificationSound, playNotificationSound and the server/privateChat
update patterns already used elsewhere in this file).
Comment on lines
+424
to
+442
| store.getState().addMessage(newMessage); | ||
|
|
||
| // Play notification sound if appropriate (but not for historical messages) | ||
| // Don't count unread/mentions for historical messages (batch tag indicates chathistory playback) | ||
| const isHistoricalMessage = mtags?.batch !== undefined; | ||
|
|
||
| if (!isHistoricalMessage) { | ||
| const state = store.getState(); | ||
| const serverCurrentUser = ircClient.getCurrentUser(response.serverId); | ||
| if ( | ||
| shouldPlayNotificationSound( | ||
| newMessage, | ||
| serverCurrentUser, | ||
| state.globalSettings, | ||
| ) | ||
| ) { | ||
| playNotificationSound(state.globalSettings); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Historical multiline messages should be buffered like other chathistory traffic.
CHANMSG and TAGMSG short-circuit chathistory batches into the batch buffers, but this handler writes multiline messages straight to the store and only suppresses sound afterwards. During load-more, that can surface messages early/out of order and break the CHATHISTORY_LOADING flow that the single-line path relies on.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/messages.ts` around lines 424 - 442, The handler in
messages.ts writes multiline messages directly to the store via
store.getState().addMessage(newMessage) and only suppresses sound afterwards,
which bypasses the chathistory batching used by CHANMSG/TAGMSG and can surface
messages early; change the flow so that when mtags?.batch (isHistoricalMessage)
is true the multiline message is routed into the same batch buffer used by the
CHANMSG/TAGMSG path (instead of calling addMessage directly), preserving the
CHATHISTORY_LOADING behavior and ensuring no notification/unread counting;
locate the logic around addMessage, isHistoricalMessage, and mtags and replace
the direct store.addMessage call with a call into the existing batch buffer
enqueue logic (or factor out a shared helper) so historical multiline messages
follow the same buffering and playback ordering as single-line chathistory
messages.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift (1)
41-47:⚠️ Potential issue | 🔴 CriticalMissing
handle.loaded = trueafter loading plugin causes repeated re-initialization.Plugins registered without an initial webview (via
load(name:plugin:config:webview:)withwebview: nil) will have theirload(webview:)method called on every subsequentonWebviewCreatedevent. Theloadedflag is never set totrueafter the first load in this path.This can cause unintended side effects if
load(webview:)performs one-time setup like registering observers or allocating resources.🐛 Proposed fix
func onWebviewCreated(_ webview: WKWebView) { for (_, handle) in plugins { if !handle.loaded { handle.instance.load(webview: webview) + handle.loaded = true } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift` around lines 41 - 47, The onWebviewCreated loop currently calls handle.instance.load(webview:) for plugins where handle.loaded is false but never sets handle.loaded = true, causing repeated re-initialization; update the onWebviewCreated(_ webview: WKWebView) implementation to set handle.loaded = true immediately after successfully calling handle.instance.load(webview: webview) (for the same plugin handle and plugins collection) so that subsequent calls skip already-loaded plugins.src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift (1)
52-57:⚠️ Potential issue | 🟠 MajorForce unwrap in
parseArgs()is inconsistent withgetArgs()and can crash.
getArgs()properly guards against UTF-8 encoding failure and throws an error, butparseArgs()uses force unwrap on the same operation. This can crash ifself.datacontains invalid UTF-8 sequences.🐛 Proposed fix for consistent error handling
public func parseArgs<T: Decodable>(_ type: T.Type) throws -> T { - let jsonData = self.data.data(using: .utf8)! + guard let jsonData = self.data.data(using: .utf8) else { + throw NSError(domain: "Tauri.Invoke", code: 1, userInfo: [ + NSLocalizedDescriptionKey: "Invoke payload could not be encoded as UTF-8", + ]) + } let decoder = JSONDecoder() decoder.userInfo[channelDataKey] = sendChannelData return try decoder.decode(type, from: jsonData) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift` around lines 52 - 57, parseArgs(_:)'s force unwrap of self.data.data(using: .utf8) can crash on invalid UTF-8; change it to mirror getArgs() by safely unwrapping the UTF-8 conversion (use guard let or if let) and throw the same error type/getArgs() uses when conversion fails, then proceed to create JSONDecoder, set decoder.userInfo[channelDataKey] = sendChannelData and decode; this ensures parseArgs(_:), like getArgs(), returns a thrown error instead of crashing when self.data isn't valid UTF-8.
♻️ Duplicate comments (1)
src/lib/irc/IRCClient.ts (1)
650-676:⚠️ Potential issue | 🟠 MajorIntentional disconnects will still trigger reconnection.
The
disconnect()method (lines 712-740) clears reconnection state and stops pings, but theonclosehandler here still callsstartReconnection()when!wasReconnecting. Sincedisconnect()setsconnectionStateto"disconnected"(not"reconnecting"),wasReconnectingwill befalse, and reconnection will begin.The previous review suggested tracking intentional closes with a flag — this appears unaddressed.
Suggested fix
+ private intentionalCloses = new Set<string>(); + disconnect(serverId: string, quitMessage?: string): void { const socket = this.sockets.get(serverId); if (socket) { + this.intentionalCloses.add(serverId); const message = quitMessage || "ObsidianIRC - Bringing IRC to the future"; socket.send(`QUIT :${message}`); @@ socket.onclose = () => { if (!this.servers.has(server.id)) { return; } + const intentional = this.intentionalCloses.delete(server.id); this.stopWebSocketPing(server.id); this.sockets.delete(server.id); server.isConnected = false; const wasReconnecting = server.connectionState === "reconnecting"; server.connectionState = "disconnected"; // ... - if (!wasReconnecting) { + if (!wasReconnecting && !intentional) { this.startReconnection(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/IRCClient.ts` around lines 650 - 676, The onclose handler is starting reconnection even for intentional disconnects; add and use an "intentionalDisconnect" flag on the Server object (or similar) so disconnect() sets server.intentionalDisconnect = true and the onclose callback checks this flag and skips calling startReconnection when true; ensure you clear server.intentionalDisconnect (set to false) before any legitimate reconnection attempt (e.g., at the start of startReconnection or when a new manual connect is initiated) so normal reconnect logic (in startReconnection, stopWebSocketPing, pendingConnections, sockets manipulations) remains unchanged.
🧹 Nitpick comments (7)
src/store/handlers/batches.ts (3)
165-176: Consider consolidating duplicateBATCH_STARThandlers.There are two separate handlers for
BATCH_START— one updatingmetadataBatches(lines 165-176) and another updatingactiveBatches(lines 193-211). While this works, consolidating into a single handler would reduce event dispatch overhead and make the initialization logic clearer.Also applies to: 193-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/batches.ts` around lines 165 - 176, Consolidate the two BATCH_START handlers into a single ircClient.on("BATCH_START", ...) listener that initializes both metadataBatches and activeBatches in one atomic store.setState call: check/initialize state.metadataBatches[batchId] with { type, messages: [] } and state.activeBatches[batchId] with { serverId, batchId, type, startTime?: Date|number, messages?: [] } (or whatever fields the existing activeBatches handler sets), and remove the duplicate handler; use the existing store.getState/store.setState flow to merge both updates to avoid dispatch duplication and preserve existing initialization logic.
78-111: MultiplesetStatecalls inside forEach may cause performance issues and race conditions.
processBatchedNetsplitcallsstore.setStateonce per QUIT event inside theforEachloop (lines 82-104), and then iteratesaffectedChannelsto add messages (lines 108-111). EachsetStatetriggers a new state snapshot, soaffectedChannelsaccumulated in one call isn't visible to the next iteration's state.Consider batching all user removals into a single
setStatecall.Suggested approach
- // Process each quit event to remove users and track affected channels - quitEvents.forEach((event) => { - const { username } = event.data; - - // Find which channels this user was in and remove them - store.setState((state) => { - const updatedServers = state.servers.map((server) => { - // ... - }); - return { servers: updatedServers }; - }); - }); + // Collect all usernames to remove + const usersToRemove = new Set(quitEvents.map((e) => e.data.username)); + + // Single setState to remove all users and track affected channels + store.setState((state) => { + const updatedServers = state.servers.map((server) => { + if (server.id !== serverId) return server; + const updatedChannels = server.channels.map((channel) => { + const hadUser = channel.users.some((u) => usersToRemove.has(u.username)); + if (hadUser) affectedChannels.add(channel.id); + return { + ...channel, + users: channel.users.filter((u) => !usersToRemove.has(u.username)), + }; + }); + return { ...server, channels: updatedChannels }; + }); + return { servers: updatedServers }; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/batches.ts` around lines 78 - 111, The loop currently calls store.setState for every quit event causing performance/race issues; instead, gather all quitting usernames from quitEvents into a Set (e.g., quittingUsernames), compute a single updated servers array by mapping state.servers and, for each server/channel, filter out users whose username is in quittingUsernames while collecting affectedChannels (the existing affectedChannels Set), then call store.setState once to replace servers with that updatedServers; after that single setState, iterate affectedChannels to create and add the netsplit messages with store.getState().addMessage as before.
222-224: Use optional chaining as suggested by linter.The static analysis tool flagged this for simplification.
Suggested fix
- if (!serverBatches || !serverBatches[batchId]) { + if (!serverBatches?.[batchId]) { return state; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/batches.ts` around lines 222 - 224, The conditional checking for missing batch should be simplified to use optional chaining: replace the manual null/undefined and property check for serverBatches and serverBatches[batchId] with a single optional-chained expression (e.g., serverBatches?.[batchId]) in the same block that currently returns state; update the if condition so it reads like "if (!serverBatches?.[batchId]) { return state; }" referencing the existing variables serverBatches, batchId, and state in this handler.src/store/handlers/connection.ts (2)
595-605: Fixed 1-second wait may not align with actual metadata response timing.The function waits a hardcoded 1000ms before resolving, regardless of whether metadata responses have arrived. This is a reasonable heuristic for IRC's async nature, but consider documenting this timeout choice or making it configurable if metadata-heavy servers prove problematic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/connection.ts` around lines 595 - 605, The hardcoded 1000ms delay before resolving metadata fetch (where setTimeout toggles metadataFetchInProgress for [serverId] and calls resolve) can miss actual metadata arrival; make this wait configurable or replace it with an event-driven wait: expose a configurable timeout option (e.g., metadataFetchTimeout) used instead of the literal 1000, or subscribe to the METADATA_KEYVALUE handler to resolve when the expected metadata keys arrive (and still fallback to the configurable timeout as a safety); update the logic surrounding metadataFetchInProgress, store.setState, and the resolve call to use the new configuration or event completion instead of the fixed delay.
496-552: MultiplesetTimeoutdelays may race under heavy load.The chathistory requests fire at 50ms (line 496) and WHO requests at 100ms (line 503). Under slow network conditions or high server load, these timings may not guarantee the intended ordering. Consider using a single timer or chaining promises for more deterministic sequencing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/connection.ts` around lines 496 - 552, The two separate setTimeout blocks using sortedPinnedChats and ircClient.sendRaw (one sending CHATHISTORY and the other sending WHO) can race; replace them with a single deterministic sequence: schedule one handler (e.g., a single setTimeout) that first iterates sortedPinnedChats to send CHATHISTORY (via ircClient.sendRaw) and then, after that loop completes, performs the server lookup (store.getState, find server by id) and the per-user channel checks (server.channels and channel.users) to decide whether to call ircClient.sendRaw(`WHO ...`) for each user; alternatively, chain the operations with promises/await so CHATHISTORY sends finish before WHO requests are issued, referencing sortedPinnedChats, ircClient.sendRaw, store.getState, and server.channels to locate the code to change.src/lib/irc/IRCClient.ts (1)
630-636: Connection state set to "connected" before CAP negotiation completes.The server is marked as
isConnected: trueandconnectionState: "connected"immediately when the WebSocket opens (line 631-636), but CAP/SASL negotiation hasn't finished yet. This means the UI might show "connected" before authentication succeeds or fails.Consider whether a separate "negotiating" state would provide better UX feedback, or document that "connected" means "socket open" rather than "fully authenticated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/IRCClient.ts` around lines 630 - 636, The code currently sets server.isConnected = true and server.connectionState = "connected" in the WebSocket open handler and fires triggerEvent("connectionStateChange", { serverId: server.id, connectionState: "connected" }) before CAP/SASL negotiation completes; change this to set a transient state (e.g., connectionState = "negotiating" or "socket_open" and server.isConnected = false) in the WebSocket open handler, emit that state via triggerEvent, and only set server.isConnected = true and connectionState = "connected" (and emit the final connectionStateChange) from the CAP/SASL negotiation completion path (the code that currently handles CAP end / SASL success/failure), so the UI reflects authentication/negotiation status properly.src/store/handlers/channels.ts (1)
433-446: Use optional chaining as suggested by linter.The static analysis tool flagged line 435 for simplification.
Suggested fix
if ( hasOperatorStatus && - (!currentUser.modes || !currentUser.modes.includes("o")) + !currentUser.modes?.includes("o") ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/channels.ts` around lines 433 - 446, Replace the explicit null/undefined check plus includes call with optional chaining: change the condition that currently tests hasOperatorStatus && (!currentUser.modes || !currentUser.modes.includes("o")) to use !currentUser.modes?.includes("o") (keeping the hasOperatorStatus part), so the block that updates currentUser.modes (and returns servers: updatedServers, currentUser: {...}) triggers when the user lacks the "o" mode without redundant checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`:
- Around line 99-101: The payload dictionary is storing the Error instance
directly (payload["error"] = error), which can break JSON serialization in
serialize(.dictionary(payload as! JsonObject)); change this to store a string
representation instead (e.g., use error.localizedDescription or
String(describing: error)) so payload["error"] contains a JSON-compatible
string; update the code in Invoke.swift where the payload is constructed to use
that string conversion before calling serialize.
In `@src/components/message/MediaPreview.tsx`:
- Around line 931-934: The youtube config passed to the react-player in
MediaPreview is using the wrong structure; update the config object used in the
component (the config prop where youtube and vimeo are set) to nest disablekb
under playerVars (i.e., change youtube: { disablekb: 1 } to youtube: {
playerVars: { disablekb: 1 } }) while leaving vimeo unchanged so the YouTube
IFrame API params are applied correctly by react-player v3.
---
Outside diff comments:
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`:
- Around line 52-57: parseArgs(_:)'s force unwrap of self.data.data(using:
.utf8) can crash on invalid UTF-8; change it to mirror getArgs() by safely
unwrapping the UTF-8 conversion (use guard let or if let) and throw the same
error type/getArgs() uses when conversion fails, then proceed to create
JSONDecoder, set decoder.userInfo[channelDataKey] = sendChannelData and decode;
this ensures parseArgs(_:), like getArgs(), returns a thrown error instead of
crashing when self.data isn't valid UTF-8.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift`:
- Around line 41-47: The onWebviewCreated loop currently calls
handle.instance.load(webview:) for plugins where handle.loaded is false but
never sets handle.loaded = true, causing repeated re-initialization; update the
onWebviewCreated(_ webview: WKWebView) implementation to set handle.loaded =
true immediately after successfully calling handle.instance.load(webview:
webview) (for the same plugin handle and plugins collection) so that subsequent
calls skip already-loaded plugins.
---
Duplicate comments:
In `@src/lib/irc/IRCClient.ts`:
- Around line 650-676: The onclose handler is starting reconnection even for
intentional disconnects; add and use an "intentionalDisconnect" flag on the
Server object (or similar) so disconnect() sets server.intentionalDisconnect =
true and the onclose callback checks this flag and skips calling
startReconnection when true; ensure you clear server.intentionalDisconnect (set
to false) before any legitimate reconnection attempt (e.g., at the start of
startReconnection or when a new manual connect is initiated) so normal reconnect
logic (in startReconnection, stopWebSocketPing, pendingConnections, sockets
manipulations) remains unchanged.
---
Nitpick comments:
In `@src/lib/irc/IRCClient.ts`:
- Around line 630-636: The code currently sets server.isConnected = true and
server.connectionState = "connected" in the WebSocket open handler and fires
triggerEvent("connectionStateChange", { serverId: server.id, connectionState:
"connected" }) before CAP/SASL negotiation completes; change this to set a
transient state (e.g., connectionState = "negotiating" or "socket_open" and
server.isConnected = false) in the WebSocket open handler, emit that state via
triggerEvent, and only set server.isConnected = true and connectionState =
"connected" (and emit the final connectionStateChange) from the CAP/SASL
negotiation completion path (the code that currently handles CAP end / SASL
success/failure), so the UI reflects authentication/negotiation status properly.
In `@src/store/handlers/batches.ts`:
- Around line 165-176: Consolidate the two BATCH_START handlers into a single
ircClient.on("BATCH_START", ...) listener that initializes both metadataBatches
and activeBatches in one atomic store.setState call: check/initialize
state.metadataBatches[batchId] with { type, messages: [] } and
state.activeBatches[batchId] with { serverId, batchId, type, startTime?:
Date|number, messages?: [] } (or whatever fields the existing activeBatches
handler sets), and remove the duplicate handler; use the existing
store.getState/store.setState flow to merge both updates to avoid dispatch
duplication and preserve existing initialization logic.
- Around line 78-111: The loop currently calls store.setState for every quit
event causing performance/race issues; instead, gather all quitting usernames
from quitEvents into a Set (e.g., quittingUsernames), compute a single updated
servers array by mapping state.servers and, for each server/channel, filter out
users whose username is in quittingUsernames while collecting affectedChannels
(the existing affectedChannels Set), then call store.setState once to replace
servers with that updatedServers; after that single setState, iterate
affectedChannels to create and add the netsplit messages with
store.getState().addMessage as before.
- Around line 222-224: The conditional checking for missing batch should be
simplified to use optional chaining: replace the manual null/undefined and
property check for serverBatches and serverBatches[batchId] with a single
optional-chained expression (e.g., serverBatches?.[batchId]) in the same block
that currently returns state; update the if condition so it reads like "if
(!serverBatches?.[batchId]) { return state; }" referencing the existing
variables serverBatches, batchId, and state in this handler.
In `@src/store/handlers/channels.ts`:
- Around line 433-446: Replace the explicit null/undefined check plus includes
call with optional chaining: change the condition that currently tests
hasOperatorStatus && (!currentUser.modes || !currentUser.modes.includes("o")) to
use !currentUser.modes?.includes("o") (keeping the hasOperatorStatus part), so
the block that updates currentUser.modes (and returns servers: updatedServers,
currentUser: {...}) triggers when the user lacks the "o" mode without redundant
checks.
In `@src/store/handlers/connection.ts`:
- Around line 595-605: The hardcoded 1000ms delay before resolving metadata
fetch (where setTimeout toggles metadataFetchInProgress for [serverId] and calls
resolve) can miss actual metadata arrival; make this wait configurable or
replace it with an event-driven wait: expose a configurable timeout option
(e.g., metadataFetchTimeout) used instead of the literal 1000, or subscribe to
the METADATA_KEYVALUE handler to resolve when the expected metadata keys arrive
(and still fallback to the configurable timeout as a safety); update the logic
surrounding metadataFetchInProgress, store.setState, and the resolve call to use
the new configuration or event completion instead of the fixed delay.
- Around line 496-552: The two separate setTimeout blocks using
sortedPinnedChats and ircClient.sendRaw (one sending CHATHISTORY and the other
sending WHO) can race; replace them with a single deterministic sequence:
schedule one handler (e.g., a single setTimeout) that first iterates
sortedPinnedChats to send CHATHISTORY (via ircClient.sendRaw) and then, after
that loop completes, performs the server lookup (store.getState, find server by
id) and the per-user channel checks (server.channels and channel.users) to
decide whether to call ircClient.sendRaw(`WHO ...`) for each user;
alternatively, chain the operations with promises/await so CHATHISTORY sends
finish before WHO requests are issued, referencing sortedPinnedChats,
ircClient.sendRaw, store.getState, and server.channels to locate the code to
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b114ae66-f70c-429e-8dbb-00809324b6b4
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
src-tauri/Cargo.tomlsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swiftsrc/components/message/CollapsibleMessage.tsxsrc/components/message/MediaPreview.tsxsrc/lib/irc/IRCClient.tssrc/lib/useMediaController.tssrc/store/handlers/auth.tssrc/store/handlers/batches.tssrc/store/handlers/channels.tssrc/store/handlers/connection.tssrc/store/handlers/messages.ts
✅ Files skipped from review due to trivial changes (1)
- src/store/handlers/messages.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/message/CollapsibleMessage.tsx
- src/lib/useMediaController.ts
- src/store/handlers/auth.ts
Comment on lines
99
to
101
| if let error = error { | ||
| payload["error"] = error.localizedDescription | ||
| payload["error"] = error | ||
| } |
Contributor
There was a problem hiding this comment.
Storing Error object directly may cause serialization issues.
The Error protocol doesn't conform to JSON-serializable types. When serialize(.dictionary(payload as! JsonObject)) is called on line 112, the Error object may not serialize correctly, potentially producing unexpected output or failing silently.
Consider using error.localizedDescription or String(describing: error) to ensure a JSON-compatible string representation.
🐛 Proposed fix
if let error = error {
- payload["error"] = error
+ payload["error"] = "\(error)"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let error = error { | |
| payload["error"] = error.localizedDescription | |
| payload["error"] = error | |
| } | |
| if let error = error { | |
| payload["error"] = "\(error)" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`
around lines 99 - 101, The payload dictionary is storing the Error instance
directly (payload["error"] = error), which can break JSON serialization in
serialize(.dictionary(payload as! JsonObject)); change this to store a string
representation instead (e.g., use error.localizedDescription or
String(describing: error)) so payload["error"] contains a JSON-compatible
string; update the code in Invoke.swift where the payload is constructed to use
that string conversion before calling serialize.
Comment on lines
+931
to
+934
| config={{ | ||
| youtube: { disablekb: 1 }, | ||
| vimeo: { keyboard: false }, | ||
| }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
react-player v3 config youtube disablekb
💡 Result:
To disable YouTube keyboard shortcuts in react-player v3, pass YouTube IFrame playerVars.disablekb: 1 via config:
import ReactPlayer from "react-player/youtube";
<ReactPlayer
src="https://www.youtube.com/watch?v=VIDEO_ID"
config={{
youtube: {
playerVars: { disablekb: 1 }
}
}}
/>Notes:
- In v3,
urlwas renamed tosrc(and legacy per-player config props likeyoutubeConfigwere removed in favor ofconfig). [1] - In the YouTube IFrame API,
disablekb=1makes the player not respond to keyboard controls. [2]
Sources: [1] [2]
🌐 Web query:
react-player v3 vimeo config keyboard false
💡 Result:
To disable Vimeo keyboard shortcuts in react-player v3, pass Vimeo’s keyboard embed parameter via the config.vimeo object:
import ReactPlayer from "react-player";
<ReactPlayer
src="https://vimeo.com/76979871"
controls
config={{
vimeo: {
keyboard: false, // or 0
},
}}
/>Vimeo’s keyboard parameter “allows for keyboard input to trigger player events”; when false, the player ignores keyboard input (tabbing is still supported). [1] ReactPlayer v3 forwards provider-specific options under config.vimeo (Vimeo “Embed Options”). [2]
If you’re on react-player v2, the equivalent was typically:
config={{ vimeo: { playerOptions: { keyboard: false } } }}Sources:
[1] Vimeo help: “About Player Parameters” (keyboard parameter) https://help.vimeo.com/hc/en-us/articles/12426260232977-About-Player-Parameters
[2] ReactPlayer docs: API Reference (config.vimeo embed options) https://thedocs.io/react-player/api_reference/
Fix YouTube config structure for react-player v3. The youtube config must nest disablekb under playerVars: use youtube: { playerVars: { disablekb: 1 } } instead of youtube: { disablekb: 1 }. React-player v3 requires this wrapping for YouTube IFrame API parameters. The Vimeo config is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/message/MediaPreview.tsx` around lines 931 - 934, The youtube
config passed to the react-player in MediaPreview is using the wrong structure;
update the config object used in the component (the config prop where youtube
and vimeo are set) to nest disablekb under playerVars (i.e., change youtube: {
disablekb: 1 } to youtube: { playerVars: { disablekb: 1 } }) while leaving vimeo
unchanged so the YouTube IFrame API params are applied correctly by react-player
v3.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/store/handlers/messages.ts (2)
369-586:⚠️ Potential issue | 🟠 Major
MULTILINE_MESSAGEstill bypasses the normal unread / mention / history pipeline.This handler still hardcodes
mentioned: [], never mirrors the channel unread/mention update path, never bumps DM unread state or browser notifications, and sends historical multiline messages straight throughaddMessageinstead of the chathistory buffer. Highlights, PM badges, and history ordering will still diverge from the single-line handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/messages.ts` around lines 369 - 586, The MULTILINE_MESSAGE handler builds newMessage with static mentioned: [] and pushes messages directly via store.getState().addMessage and processedMessageIds, bypassing the same unread/mention, DM badge, notification, and chathistory buffering used by single-line messages; update the MULTILINE_MESSAGE flow (the newMessage creation and subsequent logic around processedMessageIds, addMessage, and notification playback) to mirror the single-line handlers: compute mentions (populate mentioned from mtags or existing mention-parsing logic), update channel or privateChat unreadCount and isMentioned flags the same way the single-line path does, route historical/batch messages into the chathistory buffer rather than calling addMessage immediately when mtags.batch is present, and reuse shouldPlayNotificationSound/playNotificationSound and resolveReplyMessage logic so notifications, badge increments, and ordering match single-line messages (touch symbols: newMessage, processedMessageIds set update, addMessage, shouldPlayNotificationSound, playNotificationSound, resolveReplyMessage, and the privateChats unreadCount update).
141-145:⚠️ Potential issue | 🟠 MajorNormalize self-nick checks everywhere, not just the first one.
Several branches still fall back to raw
===/!==comparisons against the current nick. If the server echoes a different nick casing/casemapping, your own message/typing/reaction can still be treated as foreign and bump unread state or bypass the optimistic-echo guard. Please route all of these through the same normalized helper you already started using above.Also applies to: 717-717, 738-739, 1274-1274, 1410-1410, 1589-1591
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/messages.ts` around lines 141 - 145, The check comparing response.sender to currentServerUser?.username (inside the isActiveChannel/isHistoricalMessage branch) must use the same normalized self-nick helper you already introduced instead of raw ===/!==; replace direct comparisons of response.sender and any other raw self-nick checks in this file with that normalizing function so message/typing/reaction logic (e.g., the branch guarded by isActiveChannel, isHistoricalMessage and other spots that reference response.sender/currentServerUser?.username) consistently treats casemapping variations as self. Also audit and update all other occurrences of direct self-nick equality checks in this module (the remaining branches that compare response.sender to currentServerUser?.username) to use the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/store/handlers/messages.ts`:
- Around line 618-633: The server-notices and the other branches that construct
and call store.getState().addMessage (e.g., the server-notice USERMSG path
creating newMessage with targetChannelId, the own-DM echo path, and the PM
USERNOTICE path) are missing the step that records the incoming IRC message id;
update each branch to push mtags.msgid (or mtags?.msgid) into
processedMessageIds on successful processing, matching the top-of-handler replay
guard semantics (i.e., after constructing the Message and before/after calling
addMessage ensure processedMessageIds includes mtags.msgid so duplicate
deliveries are deduplicated).
- Around line 223-285: The CHANMSG fast path in the store.setState block has
diverged from addMessage: it omits the per-channel history cap and its duplicate
detection compares timestamp identity instead of numeric value, risking
unbounded growth and duplicate inserts; fix by delegating to the existing
addMessage helper (or extracting the shared insertion routine) from this CHANMSG
handler so it enforces the same per-channel cap, uses numeric timestamp
comparison (via getTime()) when checking duplicates, and merges
processedMessageIds the same way addMessage does; locate the CHANMSG fast path
using the channelKey/typingKey variables in messages.ts and either call
addMessage(...) or move the cap/duplicate/processedMessageIds logic into a
shared function and invoke it here.
---
Duplicate comments:
In `@src/store/handlers/messages.ts`:
- Around line 369-586: The MULTILINE_MESSAGE handler builds newMessage with
static mentioned: [] and pushes messages directly via
store.getState().addMessage and processedMessageIds, bypassing the same
unread/mention, DM badge, notification, and chathistory buffering used by
single-line messages; update the MULTILINE_MESSAGE flow (the newMessage creation
and subsequent logic around processedMessageIds, addMessage, and notification
playback) to mirror the single-line handlers: compute mentions (populate
mentioned from mtags or existing mention-parsing logic), update channel or
privateChat unreadCount and isMentioned flags the same way the single-line path
does, route historical/batch messages into the chathistory buffer rather than
calling addMessage immediately when mtags.batch is present, and reuse
shouldPlayNotificationSound/playNotificationSound and resolveReplyMessage logic
so notifications, badge increments, and ordering match single-line messages
(touch symbols: newMessage, processedMessageIds set update, addMessage,
shouldPlayNotificationSound, playNotificationSound, resolveReplyMessage, and the
privateChats unreadCount update).
- Around line 141-145: The check comparing response.sender to
currentServerUser?.username (inside the isActiveChannel/isHistoricalMessage
branch) must use the same normalized self-nick helper you already introduced
instead of raw ===/!==; replace direct comparisons of response.sender and any
other raw self-nick checks in this file with that normalizing function so
message/typing/reaction logic (e.g., the branch guarded by isActiveChannel,
isHistoricalMessage and other spots that reference
response.sender/currentServerUser?.username) consistently treats casemapping
variations as self. Also audit and update all other occurrences of direct
self-nick equality checks in this module (the remaining branches that compare
response.sender to currentServerUser?.username) to use the helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44222819-ea95-4caa-a183-14734324e575
📒 Files selected for processing (1)
src/store/handlers/messages.ts
Comment on lines
+223
to
+285
| // Combine message addition, message ID tracking, and typing user removal into single state update | ||
| store.setState((state) => { | ||
| const channelKey = `${newMessage.serverId}-${newMessage.channelId}`; | ||
| const currentMessages = state.messages[channelKey] || []; | ||
|
|
||
| // Check for duplicate messages | ||
| const isDuplicate = currentMessages.some((existingMessage) => { | ||
| return ( | ||
| existingMessage.id === newMessage.id || | ||
| (existingMessage.content === newMessage.content && | ||
| existingMessage.timestamp === newMessage.timestamp && | ||
| existingMessage.userId === newMessage.userId) | ||
| ); | ||
| }); | ||
|
|
||
| if (isDuplicate) { | ||
| return state; | ||
| } | ||
|
|
||
| // Add message and sort chronologically | ||
| const updatedMessages = [...currentMessages, newMessage].sort( | ||
| (a, b) => { | ||
| const timeA = | ||
| a.timestamp instanceof Date | ||
| ? a.timestamp.getTime() | ||
| : new Date(a.timestamp).getTime(); | ||
| const timeB = | ||
| b.timestamp instanceof Date | ||
| ? b.timestamp.getTime() | ||
| : new Date(b.timestamp).getTime(); | ||
| return timeA - timeB; | ||
| }, | ||
| ); | ||
|
|
||
| // Remove typing user | ||
| const typingKey = `${server.id}-${channel.id}`; | ||
| const currentTypingUsers = state.typingUsers[typingKey] || []; | ||
| const updatedTypingUsers = currentTypingUsers.filter( | ||
| (u) => u.username !== response.sender, | ||
| ); | ||
|
|
||
| // Build combined state update | ||
| const newState: Partial<AppState> = { | ||
| messages: { | ||
| ...state.messages, | ||
| [channelKey]: updatedMessages, | ||
| }, | ||
| typingUsers: { | ||
| ...state.typingUsers, | ||
| [typingKey]: updatedTypingUsers, | ||
| }, | ||
| }; | ||
|
|
||
| // Add processed message ID if present | ||
| if (mtags?.msgid) { | ||
| newState.processedMessageIds = new Set([ | ||
| ...state.processedMessageIds, | ||
| mtags.msgid, | ||
| ]); | ||
| } | ||
|
|
||
| return newState; | ||
| }); |
Contributor
There was a problem hiding this comment.
This CHANMSG fast path has drifted from addMessage.
Unlike src/store/index.ts:1465-1510, this inline insertion path no longer caps per-channel history and its duplicate check compares timestamp by identity instead of by time value. On busy channels this can both grow memory unbounded and admit duplicate untagged messages. Reuse addMessage or extract the shared insertion logic so both paths keep the same semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/messages.ts` around lines 223 - 285, The CHANMSG fast path
in the store.setState block has diverged from addMessage: it omits the
per-channel history cap and its duplicate detection compares timestamp identity
instead of numeric value, risking unbounded growth and duplicate inserts; fix by
delegating to the existing addMessage helper (or extracting the shared insertion
routine) from this CHANMSG handler so it enforces the same per-channel cap, uses
numeric timestamp comparison (via getTime()) when checking duplicates, and
merges processedMessageIds the same way addMessage does; locate the CHANMSG fast
path using the channelKey/typingKey variables in messages.ts and either call
addMessage(...) or move the cap/duplicate/processedMessageIds logic into a
shared function and invoke it here.
Comment on lines
+618
to
+633
| const targetChannelId = "server-notices"; | ||
| const newMessage: Message = { | ||
| id: uuidv4(), | ||
| type: "notice", | ||
| content: message, | ||
| timestamp: timestamp, | ||
| userId: sender, | ||
| channelId: targetChannelId, | ||
| serverId: server.id, | ||
| reactions: [], | ||
| replyMessage: null, | ||
| mentioned: [], | ||
| tags: mtags, | ||
| }; | ||
|
|
||
| store.getState().addMessage(newMessage); |
Contributor
There was a problem hiding this comment.
These branches add messages without recording the processed msgid.
The top-of-handler replay guards only work if every successful add also inserts mtags.msgid into processedMessageIds. The server-notice USERMSG path, the own-DM echo path, and the PM USERNOTICE path currently skip that step, so a second delivery of the same IRC message can slip through these branches even though the other handlers are deduped by msgid.
Also applies to: 747-766, 1295-1311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/messages.ts` around lines 618 - 633, The server-notices
and the other branches that construct and call store.getState().addMessage
(e.g., the server-notice USERMSG path creating newMessage with targetChannelId,
the own-DM echo path, and the PM USERNOTICE path) are missing the step that
records the incoming IRC message id; update each branch to push mtags.msgid (or
mtags?.msgid) into processedMessageIds on successful processing, matching the
top-of-handler replay guard semantics (i.e., after constructing the Message and
before/after calling addMessage ensure processedMessageIds includes mtags.msgid
so duplicate deliveries are deduplicated).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/lib/irc/IRCClient.ts (1)
651-677:⚠️ Potential issue | 🔴 CriticalIntentional disconnects still auto-reconnect.
disconnect()closes the socket, butsocket.onclosestill treats that close as unexpected whenever the server remains registered. A user-requested disconnect therefore falls straight back intostartReconnection().Also applies to: 713-741
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/irc/IRCClient.ts` around lines 651 - 677, The onclose handler treats every close as unexpected and calls startReconnection even for user-initiated disconnects; update the disconnect() implementation to set an explicit state or flag (e.g., server.connectionState = "disconnecting" or server.isDisconnecting = true) and then modify the socket.onclose block in IRCClient (the onclose callback that currently checks this.servers.has and wasReconnecting) to check that flag/state and skip calling startReconnection when the close was intentional; also ensure the flag/state is cleared or set to "disconnected" after cleanup so subsequent reconnect logic works as before (affects socket.onclose and disconnect() and references startReconnection, server.connectionState, and socket.onclose).src/store/handlers/messages.ts (4)
618-633:⚠️ Potential issue | 🟡 MinorRecord
mtags.msgidin every successful add path.These branches still add messages without updating
processedMessageIds, so the replay guards at the top of the handlers cannot suppress a second delivery of the same server notice, own-DM echo, or PM notice.Also applies to: 747-768, 1297-1312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/messages.ts` around lines 618 - 633, The addMessage branches that create server notices/own-DM echoes/PM notices (where targetChannelId, newMessage and store.getState().addMessage(...) are used) must also record the incoming message id so replay guards work; after a successful addMessage call, append the source id from mtags.msgid into the processedMessageIds set/registry (the same structure used at the top of the handlers for duplicate suppression) or call the existing helper that registers processed IDs. Ensure this is done in every successful-add path (including the branches around targetChannelId/newMessage and the other locations mentioned) so mtags.msgid is consistently marked processed.
141-145:⚠️ Potential issue | 🟠 MajorNormalize nick comparisons everywhere you detect “self”.
These branches still rely on raw
===/!==checks even though IRC nicks are case-insensitive. A case-only echo will still be treated as foreign here, which can create unread counts, misroute PM echoes, show your own typing state, or reapply your own reaction echo. Reuse the same normalized nick helper consistently instead of direct string equality.Also applies to: 718-718, 739-740, 1274-1276, 1410-1411, 1587-1593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/messages.ts` around lines 141 - 145, The conditional treating own messages as foreign uses raw string equality (response.sender !== currentServerUser?.username) which fails for case-only IRC nick differences; replace direct comparisons with the project's normalized nick helper (e.g., normalizeNick(response.sender) !== normalizeNick(currentServerUser?.username)) and apply the same change for all occurrences mentioned (around response.sender, currentServerUser?.username checks at the ranges noted) so every self-detection (isActiveChannel/ isHistoricalMessage branches, PM echo handling, typing state, reaction echo logic) uses the normalized comparison consistently.
369-586:⚠️ Potential issue | 🟠 Major
MULTILINE_MESSAGEstill bypasses the normal unread/history pipeline.
mentionedis hard-coded to[], the channel/DM unread state and browser notifications are never updated, and historical multiline messages still go throughaddMessage()instead of the chathistory buffer. That leaves multiline highlights/badges inconsistent withCHANMSG/USERMSGand can surface load-more messages early/out of order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/messages.ts` around lines 369 - 586, The MULTILINE_MESSAGE handling creates newMessage with mentioned: [] and calls store.getState().addMessage() directly, skipping the unread/mention/notification and chat-history buffering pipeline; fix it by populating mentioned the same way as CHANMSG/USERMSG (re-using the mention detection logic), mark/update channel/DM unreadCount and isMentioned flags and trigger browser/notification logic the same way those handlers do (reuse shouldPlayNotificationSound/playNotificationSound flow), and for historical multiline (mtags?.batch) push messages into the chathistory buffer instead of calling addMessage() so batch playback ordering and load-more behavior remains consistent; ensure processedMessageIds handling and replyMessage resolution (resolveReplyMessage) remain identical to the other message handlers.
223-285:⚠️ Potential issue | 🟠 MajorThis inline
CHANMSGinsert path still diverges fromaddMessage().The duplicate check at Line 233 compares
timestampby identity instead of time value, and this branch still skips the shared per-channel history cap. Busy channels can still admit duplicate untagged messages and grow past the intended limit. Delegate toaddMessage()or extract the shared insertion routine so both paths keep the same semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/messages.ts` around lines 223 - 285, The CHANMSG inline insert diverges from addMessage(): fix by delegating to the shared insertion routine (or extracting one) so this path uses the same duplicate-check, timestamp comparison, and per-channel history cap as addMessage(); specifically, replace the custom duplicate logic and manual sorting in the CHANMSG handler with a call to addMessage(newMessage, {mtags, server, channel}) or move the insertion logic into a shared function (e.g., insertMessageIntoChannel(state, channelKey, message)) and call it from both addMessage() and this store.setState handler; also ensure timestamp comparisons use numeric time (e.g., timestamp.getTime()) when implementing the shared routine and preserve processedMessageIds and typingUsers updates in the same way addMessage() does.
🧹 Nitpick comments (1)
src/store/handlers/messages.ts (1)
994-1058: Extract the UnrealIRCd JSON-log parser into a helper.The fallback parsing ladder is duplicated almost verbatim in both notice handlers. Any future fix now has to land twice in one already-large file, which is an easy place for behavior drift.
Also applies to: 1150-1218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/handlers/messages.ts` around lines 994 - 1058, The JSON-log parsing ladder duplicated in the notice handlers should be extracted into a single helper (e.g., create a function parseUnrealJsonLog(jsonString: string): Record<string, unknown> | null) that encapsulates the try/cleanup/aggressive cleanup/partial-extract logic; replace both duplicated blocks that read mtags["unrealircd.org/json-log"] and set jsonLogData with a call to jsonLogData = parseUnrealJsonLog(mtags["unrealircd.org/json-log"]); ensure the helper logs the same console.error/console.log messages and returns null on failure so existing callers (the variables jsonLogData and isJsonLog checks) keep the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/MediaViewerModal.tsx`:
- Around line 562-580: The topicEntries creation is prematurely dropping entries
with type === null so they never reach the later bulk probing; modify the
pipeline around extractMediaFromText/canShowMedia/getCachedProbeResult so that
topicEntries retains entries even when type is null (i.e. remove or relocate the
final .filter((e) => e.type !== null)) and keep the same shape (url, type, msg,
entryKey, isTopicEntry) so the existing bulk probe logic can operate on topic
entries the same way it does for message entries; ensure you still call
canShowMedia(e.url, mediaSettings, filehost) and populate cached type via
getCachedProbeResult as currently done.
In `@src/hooks/useMessageSending.ts`:
- Line 173: Whisper sends are losing reply metadata because getWhisperContext
routes messages through ircClient.sendWhisper which only adds channel-context
tags; update send paths to attach the same +reply and +draft/reply tags when
localReplyTo is present. Specifically, change ircClient.sendWhisper (and any
helper overloads it uses) to accept and forward arbitrary tags (or add
+reply/+draft/reply inside sendWhisper when a localReplyTo/msgid is provided),
and update callers in useMessageSending (where getWhisperContext is used) to
pass localReplyTo.msgid so whisper replies include the reply/thread metadata
just like the PRIVMSG/BATCH paths. Ensure getWhisperContext,
ircClient.sendWhisper, and the localReplyTo usage all propagate the tags
consistently.
In `@src/lib/irc/IRCClient.ts`:
- Around line 1086-1093: When sending fragments produced by splitLongLine in the
loop inside the send block that calls sendRaw with `@batch=${batchId} PRIVMSG
${target} :${splitLine}`, preserve logical lines by tagging continuation
fragments with the IRCv3 multiline concat capability: send the first fragment
unchanged and for each subsequent `splitLine` include the draft/multiline-concat
tag (e.g. add `+draft/multiline-concat` with an appropriate sequence or
continuation marker) so receivers reassemble the original logical line; update
the loop that uses splitLongLine, sendRaw, serverId, batchId, PRIVMSG and target
to add that tag on all fragments after the first.
In `@src/store/handlers/batches.ts`:
- Around line 7-18: The batch buffers (chathistoryBuffers) and related
collections (e.g., metadataBatches) are currently keyed only by batchId which
can collide across servers; change their shape to be namespaced by
serverId—either by making them nested maps (Map<serverId, Map<batchId, ...>>) or
by using a composite key like `${serverId}:${batchId}`. Update
bufferChathistoryMessage to accept a serverId parameter and use the new
namespaced key/lookup, and apply the same change to any functions that
read/write metadataBatches and the other batch-related maps referenced around
the file (lines ~27-41 and ~168-188) so all accesses consistently include
serverId.
In `@src/store/handlers/messages.ts`:
- Around line 747-926: When handling USERMSG/USERNOTICE for private chats,
detect historical messages via mtags.batch and route them through
bufferChathistoryMessage(...) instead of immediately calling addMessage();
specifically, before side-effects like playNotificationSound(...),
store.getState().addMessage(...), updating typing users, and updating private
chat lastActivity/unread/isMentioned, if mtags?.batch is present call
bufferChathistoryMessage(server.id, privateChat.id, newMessage, mtags) (using
the same signature/behavior as the channel path) and return early; apply this
change in both USERMSG/USERNOTICE private-chat branches (the block that builds
newMessage, calls addMessage, updates bot flags, processedMessageIds,
typingUsers, and updates privateChats) so historical PMs are buffered and not
played/marked/merged prematurely.
---
Duplicate comments:
In `@src/lib/irc/IRCClient.ts`:
- Around line 651-677: The onclose handler treats every close as unexpected and
calls startReconnection even for user-initiated disconnects; update the
disconnect() implementation to set an explicit state or flag (e.g.,
server.connectionState = "disconnecting" or server.isDisconnecting = true) and
then modify the socket.onclose block in IRCClient (the onclose callback that
currently checks this.servers.has and wasReconnecting) to check that flag/state
and skip calling startReconnection when the close was intentional; also ensure
the flag/state is cleared or set to "disconnected" after cleanup so subsequent
reconnect logic works as before (affects socket.onclose and disconnect() and
references startReconnection, server.connectionState, and socket.onclose).
In `@src/store/handlers/messages.ts`:
- Around line 618-633: The addMessage branches that create server notices/own-DM
echoes/PM notices (where targetChannelId, newMessage and
store.getState().addMessage(...) are used) must also record the incoming message
id so replay guards work; after a successful addMessage call, append the source
id from mtags.msgid into the processedMessageIds set/registry (the same
structure used at the top of the handlers for duplicate suppression) or call the
existing helper that registers processed IDs. Ensure this is done in every
successful-add path (including the branches around targetChannelId/newMessage
and the other locations mentioned) so mtags.msgid is consistently marked
processed.
- Around line 141-145: The conditional treating own messages as foreign uses raw
string equality (response.sender !== currentServerUser?.username) which fails
for case-only IRC nick differences; replace direct comparisons with the
project's normalized nick helper (e.g., normalizeNick(response.sender) !==
normalizeNick(currentServerUser?.username)) and apply the same change for all
occurrences mentioned (around response.sender, currentServerUser?.username
checks at the ranges noted) so every self-detection (isActiveChannel/
isHistoricalMessage branches, PM echo handling, typing state, reaction echo
logic) uses the normalized comparison consistently.
- Around line 369-586: The MULTILINE_MESSAGE handling creates newMessage with
mentioned: [] and calls store.getState().addMessage() directly, skipping the
unread/mention/notification and chat-history buffering pipeline; fix it by
populating mentioned the same way as CHANMSG/USERMSG (re-using the mention
detection logic), mark/update channel/DM unreadCount and isMentioned flags and
trigger browser/notification logic the same way those handlers do (reuse
shouldPlayNotificationSound/playNotificationSound flow), and for historical
multiline (mtags?.batch) push messages into the chathistory buffer instead of
calling addMessage() so batch playback ordering and load-more behavior remains
consistent; ensure processedMessageIds handling and replyMessage resolution
(resolveReplyMessage) remain identical to the other message handlers.
- Around line 223-285: The CHANMSG inline insert diverges from addMessage(): fix
by delegating to the shared insertion routine (or extracting one) so this path
uses the same duplicate-check, timestamp comparison, and per-channel history cap
as addMessage(); specifically, replace the custom duplicate logic and manual
sorting in the CHANMSG handler with a call to addMessage(newMessage, {mtags,
server, channel}) or move the insertion logic into a shared function (e.g.,
insertMessageIntoChannel(state, channelKey, message)) and call it from both
addMessage() and this store.setState handler; also ensure timestamp comparisons
use numeric time (e.g., timestamp.getTime()) when implementing the shared
routine and preserve processedMessageIds and typingUsers updates in the same way
addMessage() does.
---
Nitpick comments:
In `@src/store/handlers/messages.ts`:
- Around line 994-1058: The JSON-log parsing ladder duplicated in the notice
handlers should be extracted into a single helper (e.g., create a function
parseUnrealJsonLog(jsonString: string): Record<string, unknown> | null) that
encapsulates the try/cleanup/aggressive cleanup/partial-extract logic; replace
both duplicated blocks that read mtags["unrealircd.org/json-log"] and set
jsonLogData with a call to jsonLogData =
parseUnrealJsonLog(mtags["unrealircd.org/json-log"]); ensure the helper logs the
same console.error/console.log messages and returns null on failure so existing
callers (the variables jsonLogData and isJsonLog checks) keep the same behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 792dd738-dfd8-44a7-8130-1c50c699e829
📒 Files selected for processing (10)
src/components/message/MessageItem.tsxsrc/components/ui/MediaCommentsSidebar.tsxsrc/components/ui/MediaViewerModal.tsxsrc/hooks/useMessageSending.tssrc/hooks/useReactions.tssrc/lib/irc/IRCClient.tssrc/store/handlers/batches.tssrc/store/handlers/messages.tssrc/store/helpers.tssrc/store/index.ts
Comment on lines
+562
to
+580
| const topicEntries = channel?.topic | ||
| ? extractMediaFromText(channel.topic) | ||
| .filter((e) => canShowMedia(e.url, mediaSettings, filehost)) | ||
| .map((e, i) => { | ||
| let type = e.type; | ||
| if (type === null) { | ||
| const cached = getCachedProbeResult(e.url); | ||
| if (cached != null && !cached.skipped) type = cached.type; | ||
| } | ||
| return { | ||
| url: e.url, | ||
| type, | ||
| msg: null as Message | null, | ||
| entryKey: `topic:${i}`, | ||
| isTopicEntry: true, | ||
| }; | ||
| }) | ||
| .filter((e) => e.type !== null) | ||
| : []; |
Contributor
There was a problem hiding this comment.
Unknown-type topic media never reaches the probe path.
topicEntries drops every extractMediaFromText() result with type === null, but the bulk probing logic only runs on entries that survive this stage. Extensionless/filehost URLs in channel topics will therefore never appear, even though the message-entry path already keeps those URLs around long enough to probe them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/MediaViewerModal.tsx` around lines 562 - 580, The
topicEntries creation is prematurely dropping entries with type === null so they
never reach the later bulk probing; modify the pipeline around
extractMediaFromText/canShowMedia/getCachedProbeResult so that topicEntries
retains entries even when type is null (i.e. remove or relocate the final
.filter((e) => e.type !== null)) and keep the same shape (url, type, msg,
entryKey, isTopicEntry) so the existing bulk probe logic can operate on topic
entries the same way it does for message entries; ensure you still call
canShowMedia(e.url, mediaSettings, filehost) and populate cached type via
getCachedProbeResult as currently done.
Comment on lines
+1086
to
+1093
| for (const line of lines) { | ||
| const splitLines = this.splitLongLine(line); | ||
| for (const splitLine of splitLines) { | ||
| this.sendRaw( | ||
| serverId, | ||
| `@batch=${batchId} PRIVMSG ${target} :${splitLine}`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve logical lines when splitLongLine() fragments a multiline item.
After the first chunk, these sends need draft/multiline-concat. Without it, one long line is reassembled as several visible lines on the receiver.
Suggested fix
- for (const splitLine of splitLines) {
- this.sendRaw(
- serverId,
- `@batch=${batchId} PRIVMSG ${target} :${splitLine}`,
- );
- }
+ splitLines.forEach((splitLine, index) => {
+ this.sendRaw(
+ serverId,
+ index === 0
+ ? `@batch=${batchId} PRIVMSG ${target} :${splitLine}`
+ : `@batch=${batchId};draft/multiline-concat PRIVMSG ${target} :${splitLine}`,
+ );
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const line of lines) { | |
| const splitLines = this.splitLongLine(line); | |
| for (const splitLine of splitLines) { | |
| this.sendRaw( | |
| serverId, | |
| `@batch=${batchId} PRIVMSG ${target} :${splitLine}`, | |
| ); | |
| } | |
| for (const line of lines) { | |
| const splitLines = this.splitLongLine(line); | |
| splitLines.forEach((splitLine, index) => { | |
| this.sendRaw( | |
| serverId, | |
| index === 0 | |
| ? `@batch=${batchId} PRIVMSG ${target} :${splitLine}` | |
| : `@batch=${batchId};draft/multiline-concat PRIVMSG ${target} :${splitLine}`, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/irc/IRCClient.ts` around lines 1086 - 1093, When sending fragments
produced by splitLongLine in the loop inside the send block that calls sendRaw
with `@batch=${batchId} PRIVMSG ${target} :${splitLine}`, preserve logical lines
by tagging continuation fragments with the IRCv3 multiline concat capability:
send the first fragment unchanged and for each subsequent `splitLine` include
the draft/multiline-concat tag (e.g. add `+draft/multiline-concat` with an
appropriate sequence or continuation marker) so receivers reassemble the
original logical line; update the loop that uses splitLongLine, sendRaw,
serverId, batchId, PRIVMSG and target to add that tag on all fragments after the
first.
Comment on lines
+7
to
+18
| // Chathistory messages are buffered here (outside the store) to avoid one | ||
| // store.setState per incoming PRIVMSG. Only flushed at BATCH_END. | ||
| export const chathistoryBuffers = new Map<string, Message[]>(); | ||
|
|
||
| export function bufferChathistoryMessage(batchId: string, msg: Message): void { | ||
| let buf = chathistoryBuffers.get(batchId); | ||
| if (!buf) { | ||
| buf = []; | ||
| chathistoryBuffers.set(batchId, buf); | ||
| } | ||
| buf.push(msg); | ||
| } |
Contributor
There was a problem hiding this comment.
Namespace batch-local storage by serverId.
IRC batch ids are only meaningful within a single connection, but these buffers and metadataBatches are keyed by bare batchId. If two servers reuse the same id, their buffered messages, reactions, or metadata state will alias and corrupt each other. Use a composite key like ${serverId}:${batchId} or a nested per-server map consistently here.
Also applies to: 27-41, 168-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/batches.ts` around lines 7 - 18, The batch buffers
(chathistoryBuffers) and related collections (e.g., metadataBatches) are
currently keyed only by batchId which can collide across servers; change their
shape to be namespaced by serverId—either by making them nested maps
(Map<serverId, Map<batchId, ...>>) or by using a composite key like
`${serverId}:${batchId}`. Update bufferChathistoryMessage to accept a serverId
parameter and use the new namespaced key/lookup, and apply the same change to
any functions that read/write metadataBatches and the other batch-related maps
referenced around the file (lines ~27-41 and ~168-188) so all accesses
consistently include serverId.
Comment on lines
+747
to
+926
| const privateChatKey = `${server.id}-${privateChat.id}`; | ||
| const newMessage = { | ||
| id: uuidv4(), | ||
| msgid: mtags?.msgid, | ||
| content: message, | ||
| timestamp, | ||
| userId: sender, | ||
| channelId: privateChat.id, | ||
| serverId: server.id, | ||
| type: "message" as const, | ||
| reactions: [], | ||
| replyMessage: resolveReplyMessage( | ||
| mtags, | ||
| server.id, | ||
| privateChat.id, | ||
| store.getState().messages[privateChatKey] || [], | ||
| ), | ||
| mentioned: [], | ||
| tags: mtags, | ||
| }; | ||
| store.getState().addMessage(newMessage); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (server) { | ||
| // Find or create private chat (IRC nicks are case-insensitive) | ||
| let privateChat = server.privateChats?.find( | ||
| (pc) => pc.username.toLowerCase() === sender.toLowerCase(), | ||
| ); | ||
|
|
||
| if (!privateChat) { | ||
| // Auto-create private chat when receiving a message | ||
| store.getState().openPrivateChat(server.id, sender); | ||
| // Get the newly created private chat | ||
| privateChat = store | ||
| .getState() | ||
| .servers.find((s) => s.id === server.id) | ||
| ?.privateChats?.find( | ||
| (pc) => pc.username.toLowerCase() === sender.toLowerCase(), | ||
| ); | ||
| } | ||
|
|
||
| if (privateChat) { | ||
| const privateChatKey = `${server.id}-${privateChat.id}`; | ||
| const newMessage = { | ||
| id: uuidv4(), | ||
| msgid: mtags?.msgid, | ||
| content: message, | ||
| timestamp, | ||
| userId: sender, | ||
| channelId: privateChat.id, | ||
| serverId: server.id, | ||
| type: "message" as const, | ||
| reactions: [], | ||
| replyMessage: resolveReplyMessage( | ||
| mtags, | ||
| server.id, | ||
| privateChat.id, | ||
| store.getState().messages[privateChatKey] || [], | ||
| ), | ||
| mentioned: [], | ||
| tags: mtags, | ||
| }; | ||
|
|
||
| // If message has bot tag, mark user as bot | ||
| if (mtags?.bot !== undefined) { | ||
| store.setState((state) => { | ||
| const updatedServers = state.servers.map((s) => { | ||
| if (s.id === server.id) { | ||
| const updatedChannels = s.channels.map((channel) => { | ||
| const updatedUsers = channel.users.map((user) => { | ||
| if (user.username === sender) { | ||
| return { | ||
| ...user, | ||
| isBot: true, // Set bot flag from message tags | ||
| metadata: { | ||
| ...user.metadata, | ||
| bot: { value: "true", visibility: "public" }, | ||
| }, | ||
| }; | ||
| } | ||
| return user; | ||
| }); | ||
| return { ...channel, users: updatedUsers }; | ||
| }); | ||
| return { ...s, channels: updatedChannels }; | ||
| } | ||
| return s; | ||
| }); | ||
| return { servers: updatedServers }; | ||
| }); | ||
| } | ||
|
|
||
| // Mark this message ID as processed to prevent duplicates | ||
| if (mtags?.msgid) { | ||
| store.setState((state) => ({ | ||
| processedMessageIds: new Set([ | ||
| ...state.processedMessageIds, | ||
| mtags.msgid, | ||
| ]), | ||
| })); | ||
| } | ||
|
|
||
| // If the stored username casing differs from the server-sent nick, correct it now. | ||
| if (privateChat.username !== sender) { | ||
| store.setState((state) => ({ | ||
| servers: state.servers.map((s) => { | ||
| if (s.id !== server.id) return s; | ||
| return { | ||
| ...s, | ||
| privateChats: s.privateChats?.map((pc) => | ||
| pc.id === privateChat.id ? { ...pc, username: sender } : pc, | ||
| ), | ||
| }; | ||
| }), | ||
| })); | ||
| } | ||
|
|
||
| store.getState().addMessage(newMessage); | ||
|
|
||
| // Remove any typing users from the state | ||
| store.setState((state) => { | ||
| const key = `${server.id}-${privateChat.id}`; | ||
| const currentUsers = state.typingUsers[key] || []; | ||
| return { | ||
| typingUsers: { | ||
| ...state.typingUsers, | ||
| [key]: currentUsers.filter((u) => u.username !== sender), | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| // Update private chat's last activity and unread count | ||
| // Don't count unread/mentions for historical messages (batch tag indicates chathistory playback) | ||
| const isHistoricalMessage = mtags?.batch !== undefined; | ||
|
|
||
| // Play notification sound if appropriate (but not for historical messages) | ||
| if (!isHistoricalMessage) { | ||
| const state = store.getState(); | ||
| const serverCurrentUser = ircClient.getCurrentUser(response.serverId); | ||
| if ( | ||
| shouldPlayNotificationSound( | ||
| newMessage, | ||
| serverCurrentUser, | ||
| state.globalSettings, | ||
| ) | ||
| ) { | ||
| playNotificationSound(state.globalSettings); | ||
| } | ||
| } | ||
|
|
||
| store.setState((state) => { | ||
| const updatedServers = state.servers.map((s) => { | ||
| if (s.id === response.serverId) { | ||
| const updatedPrivateChats = | ||
| s.privateChats?.map((pc) => { | ||
| if (pc.id === privateChat.id) { | ||
| const isActive = | ||
| getCurrentSelection(state).selectedPrivateChatId === | ||
| pc.id; | ||
| return { | ||
| ...pc, | ||
| lastActivity: new Date(), | ||
| unreadCount: | ||
| isActive || isHistoricalMessage | ||
| ? 0 | ||
| : pc.unreadCount + 1, | ||
| isMentioned: !isHistoricalMessage && true, // All PMs are considered mentions (except historical) | ||
| }; | ||
| } | ||
| return pc; | ||
| }) || []; | ||
| return { ...s, privateChats: updatedPrivateChats }; | ||
| } | ||
| return s; | ||
| }); | ||
| return { servers: updatedServers }; | ||
| }); |
Contributor
There was a problem hiding this comment.
Historical PM traffic is not using the new chathistory buffers.
src/store/handlers/batches.ts now has a private-chat flush path, but these USERMSG/USERNOTICE branches still call addMessage() even when mtags.batch is present. PM history will render live, update activity state immediately, and leave the batch flush with nothing to merge or re-resolve. Route historical PMs through bufferChathistoryMessage() the same way the channel path does.
Also applies to: 1296-1355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/messages.ts` around lines 747 - 926, When handling
USERMSG/USERNOTICE for private chats, detect historical messages via mtags.batch
and route them through bufferChathistoryMessage(...) instead of immediately
calling addMessage(); specifically, before side-effects like
playNotificationSound(...), store.getState().addMessage(...), updating typing
users, and updating private chat lastActivity/unread/isMentioned, if
mtags?.batch is present call bufferChathistoryMessage(server.id, privateChat.id,
newMessage, mtags) (using the same signature/behavior as the channel path) and
return early; apply this change in both USERMSG/USERNOTICE private-chat branches
(the block that builds newMessage, calls addMessage, updates bot flags,
processedMessageIds, typingUsers, and updates privateChats) so historical PMs
are buffered and not played/marked/merged prematurely.
ValwareIRC
approved these changes
Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements