Vastly improve Channel Settings modal#94
Merged
Conversation
- Preserve scroll position when user scrolls up to read history - Auto-scroll only when user is already at bottom - Add wasAtBottomRef to track scroll state before new messages arrive - Fix timing issue where scroll position was checked after DOM updates
- Default sort channels by user count (highest first) - Display user count as purple badge matching selected server tab color - Click overlay to close modal - Make channels list scrollable while keeping filter/sort options fixed - Fix modal layout structure for proper scrolling behavior
…anced filtering using ELIST ISUPPORT
…e multiline cutoff point
…o fix/general-bug-fixes-and-improvements
Contributor
📝 WalkthroughWalkthroughChannelSettingsModal was expanded (props, UI, mobile two‑pane) and integrated with a new FloodSettingsModal; CHANMODES support and a new RPL_CHANNELMODEIS (324) event were added, and mode parsing/generation, store handling, and UI sync for channel modes were implemented. Changes
Sequence DiagramsequenceDiagram
participant User
participant ChannelSettingsModal
participant FloodSettingsModal
participant IRC
participant Store
User->>ChannelSettingsModal: open(serverId, channelName)
ChannelSettingsModal->>IRC: request channel mode / metadata
IRC-->>ChannelSettingsModal: RPL_CHANNELMODEIS (324) with modestring + args
ChannelSettingsModal->>ChannelSettingsModal: parseCurrentChannelModes(modestring, CHANMODES)
ChannelSettingsModal->>Store: read/write channel mode state
User->>ChannelSettingsModal: open Flood settings
ChannelSettingsModal->>FloodSettingsModal: open(initialFloodParams)
FloodSettingsModal-->>ChannelSettingsModal: onSave(floodProfile, floodParams)
User->>ChannelSettingsModal: apply changes
ChannelSettingsModal->>IRC: send MODE command(s)
IRC->>Store: MODE / RPL_CHANNELMODEIS events
Store-->>ChannelSettingsModal: updated channel state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/store/index.ts (1)
4835-4901: User MODE parsing overwrites state and miscomputes isIrcOp on “-o”.You’re storing the raw delta (e.g., “+o” or “-o”) into user.modes and using modestring.includes("o"), which falsely sets isIrcOp true even on “-o”. Apply deltas to the prior mode set and derive isIrcOp from the resulting set.
Apply this diff:
ircClient.on("MODE", ({ serverId, sender, target, modestring, modeargs }) => { // Handle user mode responses (channel modes are handled by the protocol handler) if (!target.startsWith("#")) { // This is a user mode change useStore.setState((state) => { + const applyUserModeDelta = (prev: string | undefined, delta: string) => { + const set = new Set((prev || "").replace(/[+-]/g, "").split("").filter(Boolean)); + let action: "+" | "-" = "+"; + for (const ch of delta) { + if (ch === "+" || ch === "-") { + action = ch as "+" | "-"; + continue; + } + if (action === "+") set.add(ch); + else set.delete(ch); + } + // Keep as letters only (no +/-), sorted for stability + return Array.from(set).sort().join(""); + }; + // Check if this is the current user const currentUser = state.currentUser; if ( currentUser && currentUser.username.toLowerCase() === target.toLowerCase() ) { - // Check if this is an IRC operator mode change - const isIrcOp = modestring.includes("o"); - // Update current user's modes and IRC operator status + const nextModes = applyUserModeDelta(currentUser.modes, modestring); + const isIrcOp = nextModes.includes("o"); return { currentUser: { ...currentUser, - modes: modestring, + modes: nextModes, isIrcOp: isIrcOp, }, }; } // If no currentUser in store, check if this MODE is for the IRC current user const ircCurrentUser = ircClient.getCurrentUser(serverId); if ( !currentUser && ircCurrentUser && ircCurrentUser.username.toLowerCase() === target.toLowerCase() ) { - // Check if this is an IRC operator mode change - const isIrcOp = modestring.includes("o"); - // Set the current user with modes and IRC operator status + const nextModes = applyUserModeDelta(ircCurrentUser.modes, modestring); + const isIrcOp = nextModes.includes("o"); return { currentUser: { ...ircCurrentUser, - modes: modestring, + modes: nextModes, isIrcOp: isIrcOp, }, }; } // Update user in server users list const updatedServers = state.servers.map((server) => { if (server.id === serverId) { const updatedUsers = server.users.map((user) => { if (user.username.toLowerCase() === target.toLowerCase()) { console.log( "Updated user", user.username, "modes to", modestring, ); return { ...user, - modes: modestring, + modes: applyUserModeDelta(user.modes, modestring), }; } return user; }); return { ...server, users: updatedUsers }; } return server; }); return { servers: updatedServers }; }); } });src/components/ui/ChannelSettingsModal.tsx (1)
370-409: Implement timeout cleanup to prevent memory leaks from multiple nested setTimeout callsThe code has six
setTimeoutinstances acrossfetchChannelModes,addMode,removeMode, andsaveEditthat are never tracked or cleared. When the modal closes, these pending timeouts attemptsetStatecalls after unmount, causing memory leaks and potential race conditions.Track all timeouts with a ref and clear them in a
useEffectcleanup:+ const timeoutsRef = useRef<number[]>([]); + const addTimeout = (fn: () => void, ms: number) => { + const id = window.setTimeout(fn, ms); + timeoutsRef.current.push(id); + return id; + }; + useEffect(() => { + return () => { + timeoutsRef.current.forEach(clearTimeout); + timeoutsRef.current = []; + }; + }, []); ... - setTimeout(() => { + addTimeout(() => { ... - }, 1000); + }, 1000);Apply this wrapper to all six instances: lines 385, 417, 424, 449, 456, and 512.
🧹 Nitpick comments (5)
src/protocol/mode.ts (2)
274-291: Add fallback param logic to parseCurrentChannelModes when CHANMODES is missing.Without chanmodes, treat beI as always-param; k/l param only when setting; ovhqa param-for-user-targets is irrelevant here (we already filter them), so only beI/kl matter.
Apply this diff:
- let hasParam = false; - if (groupA.includes(mode) || groupB.includes(mode)) { - hasParam = true; - } else if (groupC.includes(mode)) { - hasParam = currentAction === "+"; - } + let hasParam = false; + if (chanmodes) { + if (groupA.includes(mode) || groupB.includes(mode)) { + hasParam = true; + } else if (groupC.includes(mode)) { + hasParam = currentAction === "+"; + } + } else { + if ("beI".includes(mode)) hasParam = true; + else if ("kl".includes(mode)) hasParam = currentAction === "+"; + }
316-323: Mirror fallback in generateModestringAndArgs.So we consistently reconstruct modes when chanmodes is unavailable.
Apply this diff:
- let hasParam = false; - if (groupA.includes(mode) || groupB.includes(mode)) { - hasParam = true; - } else if (groupC.includes(mode)) { - hasParam = true; // If it's set, it has a param - } + let hasParam = false; + if (chanmodes) { + if (groupA.includes(mode) || groupB.includes(mode)) { + hasParam = true; + } else if (groupC.includes(mode)) { + hasParam = true; // If it's set, it has a param + } + } else { + if ("beI".includes(mode)) hasParam = true; + else if ("kl".includes(mode)) hasParam = true; + }src/components/ui/FloodSettingsModal.tsx (1)
210-225: Add dialog semantics and Esc-to-close for accessibility.Provide role/aria, label, and keyboard handling; set initial focus inside the modal.
Apply this diff:
- return createPortal( - <div className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center p-4 z-50"> + return createPortal( + <div + className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center p-4 z-50" + role="dialog" + aria-modal="true" + aria-labelledby="flood-settings-title" + onKeyDown={(e) => { + if (e.key === "Escape") onClose(); + }} + > <div className={`bg-discord-dark-400 rounded-lg shadow-xl max-h-[90vh] flex flex-col ${isMobile ? "w-full max-w-md" : "w-full max-w-2xl"}`} > {/* Header */} <div className="flex items-center justify-between p-6 border-b border-discord-dark-500 flex-shrink-0"> - <h2 className="text-xl font-semibold text-white"> + <h2 id="flood-settings-title" className="text-xl font-semibold text-white"> Flood Protection Settings </h2>Optionally, focus the container on open:
const scrollContainerRef = useRef<HTMLDivElement>(null); + const modalRef = useRef<HTMLDivElement>(null); + useEffect(() => { + if (isOpen) modalRef.current?.focus(); + }, [isOpen]); - <div + <div + ref={modalRef} + tabIndex={-1} className={`bg-discord-dark-400 ...`}Also applies to: 216-219
src/components/ui/ChannelSettingsModal.tsx (2)
968-973: Add accessible label to the Close buttonIcon-only control needs an aria-label for screen readers.
- <button - onClick={onClose} - className="text-discord-text-muted hover:text-white" - > + <button + onClick={onClose} + aria-label="Close channel settings" + className="text-discord-text-muted hover:text-white" + > <FaTimes /> </button>
932-955: Sidebar buttons: provide labels when text is visually hidden on mobileWhen isMobile hides text, add aria-label so buttons remain accessible.
- <button + <button key={category.id} onClick={() => setActiveTab(category.id)} + aria-label={isMobile ? category.name : undefined} className={`flex items-center ${isMobile ? "justify-center px-2" : "w/full px-3 text-left"} py-2 mb-1 rounded transition-colors overflow-hidden min-w-0 ${ activeTab === category.id ? "bg-discord-primary text-white" : "text-discord-text-muted hover:text-white hover:bg-discord-dark-400" }`} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/ui/ChannelSettingsModal.tsx(13 hunks)src/components/ui/FloodSettingsModal.tsx(1 hunks)src/lib/ircClient.ts(2 hunks)src/protocol/isupport.ts(1 hunks)src/protocol/mode.ts(5 hunks)src/store/index.ts(2 hunks)src/types/index.ts(1 hunks)tests/components/ChannelSettingsModal.test.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/components/ui/FloodSettingsModal.tsx (1)
src/hooks/useMediaQuery.ts (1)
useMediaQuery(3-15)
src/lib/ircClient.ts (1)
src/types/index.ts (1)
BaseIRCEvent(207-209)
src/protocol/isupport.ts (1)
src/types/index.ts (1)
Server(25-49)
src/components/ui/ChannelSettingsModal.tsx (1)
src/hooks/useMediaQuery.ts (1)
useMediaQuery(3-15)
src/protocol/mode.ts (2)
src/types/index.ts (2)
Channel(69-89)Message(114-166)src/store/index.ts (1)
AppState(445-712)
src/store/index.ts (1)
src/lib/ircClient.ts (1)
ircClient(2744-2744)
🔇 Additional comments (7)
src/types/index.ts (1)
42-42: LGTM! Clean addition to the Server interface.The
chanmodesproperty follows the established pattern for other ISUPPORT-related server properties (likeprefix,botMode,elist). The type and comment are clear.src/protocol/isupport.ts (1)
51-62: LGTM! Consistent with existing ISUPPORT handlers.The CHANMODES handler follows the exact same pattern as the PREFIX, NETWORK, and other ISUPPORT handlers in this file. The implementation correctly updates the server's
chanmodesproperty when the matching ISUPPORT key is received.src/lib/ircClient.ts (2)
53-57: LGTM! Well-defined event type.The RPL_CHANNELMODEIS event follows the pattern of other IRC numeric response events, with appropriate fields for channel mode information.
1685-1695: LGTM! Correct parsing of RPL_CHANNELMODEIS.The numeric 324 handler correctly parses the IRC protocol format:
parv[1]→ channel nameparv[2] || ""→ mode string with safe defaultparv.slice(3)→ mode arguments (empty array if none)This matches the IRC protocol specification for RPL_CHANNELMODEIS and follows the parsing pattern used by other numeric handlers in this file.
tests/components/ChannelSettingsModal.test.tsx (1)
158-306: LGTM! Tests updated to match the component's UI refresh.The test updates reflect the modal's new structure:
- Title no longer includes the channel name (line 158, 306)
- Tab queries now use the navigation element and text content for flexibility (lines 164-169, 184-193)
- Active tab detection relies on the
bg-discord-primaryclass (lines 174-179, 191-192)- Explicit tab activation before assertions (lines 204-211)
These changes make the tests more resilient to UI variations while still verifying core functionality.
src/store/index.ts (1)
4904-4927: LGTM: channel mode snapshot from 324 is persisted.Saving modestring and modeargs on RPL_CHANNELMODEIS aligns the store with server truth and unblocks CHANMODES-aware parsing downstream.
src/protocol/mode.ts (1)
480-508: LGTM: compact grouping of MODE notifications improves readability.Grouping by +/- and rendering “+nt -k key” style is a nice UX improvement.
Contributor
matheusfillipe
left a comment
matheusfillipe
left a comment
There was a problem hiding this comment.
seriously consider refactoring all magic strings like "K" "M" "i" etc used everywhere in this PR to be explicitely named enums/variables something unified and referable by LSP.
|
|
||
| const parsedModes: { [key: string]: string | null } = {}; | ||
| let argIndex = 0; | ||
| let currentAction: "+" | "-" = "+"; |
Contributor
There was a problem hiding this comment.
Geee why not use actual names for things or an enum
Comment on lines
+331
to
+345
| setInviteOnly("i" in parsedModes); | ||
| setModerated("m" in parsedModes); | ||
| setSecret("s" in parsedModes); | ||
| setProtectedTopic("t" in parsedModes); | ||
| setNoExternalMessages("n" in parsedModes); | ||
|
|
||
| // Set parameterized modes | ||
| setChannelKey("k" in parsedModes ? parsedModes.k || "" : ""); | ||
| setClientLimit( | ||
| "l" in parsedModes | ||
| ? parsedModes.l | ||
| ? Number.parseInt(parsedModes.l, 10) | ||
| : null | ||
| : null, | ||
| ); |
Contributor
There was a problem hiding this comment.
should really use enums or other names instead of single letter strings. This only makes sense for full IRC nerds. There could be another function to translate it where it is shown.
Comment on lines
+130
to
+167
| const getTypeDescription = (type: FloodRule["type"]): string => { | ||
| const descriptions = { | ||
| c: "CTCP", | ||
| j: "Join", | ||
| k: "Knock", | ||
| m: "Messages (channel-wide)", | ||
| n: "Nickchange", | ||
| t: "Text (per-user)", | ||
| r: "Repeat", | ||
| }; | ||
| return descriptions[type]; | ||
| }; | ||
|
|
||
| const getDefaultAction = (type: FloodRule["type"]): string => { | ||
| const defaults = { | ||
| c: "C", | ||
| j: "i", | ||
| k: "K", | ||
| m: "m", | ||
| n: "N", | ||
| t: "kick", | ||
| r: "kick", | ||
| }; | ||
| return defaults[type]; | ||
| }; | ||
|
|
||
| const getAvailableActions = (type: FloodRule["type"]): string[] => { | ||
| const actions = { | ||
| c: ["C", "m", "M"], | ||
| j: ["i", "R"], | ||
| k: ["K"], | ||
| m: ["m", "M"], | ||
| n: ["N"], | ||
| t: ["kick", "b", "d"], | ||
| r: ["kick", "d", "b"], | ||
| }; | ||
| return actions[type]; | ||
| }; |
Contributor
There was a problem hiding this comment.
Sure the strings can be statically typed but I cannot understand what this means just by looking at the code. This could be probably better structured as an array objects or some better way with interfaces instead of matching object keys in repeated jsons...
…pport Resolves merge conflicts by keeping HEAD's ChannelSettingsModal (sidebar navigation, portal rendering, FloodSettingsModal integration, mobile-aware layout) which supersedes the PR's simpler flat-tab version. Also fixes pre-existing noArrayIndexKey linting issue from ChannelSettingsModal key prop by removing index from the composite key.
…-and-improvements # Conflicts: # src/components/ui/ChannelSettingsModal.tsx # src/index.css # tests/components/ChannelSettingsModal.test.tsx
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes and hardens UI modal behavior (focus, keyboard/touch interactions, consistent styling) while expanding Tauri/mobile support (deep links, native URL opening) and improving navigation/UX patterns across the app.
Changes:
- Introduces a shared modal library (BaseModal, focus trap, modal context/hooks) and migrates multiple modals to it.
- Improves mobile interactions (swipe navigation, bottom sheets, long press) and message actions/reactions UI.
- Adds Tauri deep-linking / external URL opening helpers and updates Android/iOS/Tauri configs to support them.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/platformUtils.ts | Adds Tauri platform detection helpers. |
| src/lib/openUrl.ts | Adds cross-platform external URL opener (Tauri vs web). |
| src/lib/modal/utils/focusTrap.ts | Adds focus trap utility + React hook for modals. |
| src/lib/modal/index.ts | Exposes modal library public exports. |
| src/lib/modal/hooks/useModal.ts | Adds hooks for single/multiple modal state management. |
| src/lib/modal/context/ModalProvider.tsx | Adds modal stack/context provider and helpers. |
| src/lib/modal/components/index.ts | Exports shared modal UI components. |
| src/lib/modal/components/ModalFooter.tsx | Adds a shared modal footer layout component. |
| src/lib/modal/components/ModalBody.tsx | Adds a shared modal body (scrollable) component. |
| src/lib/modal/components/Input.tsx | Adds shared labeled input wrapper for modals. |
| src/lib/modal/components/Button.tsx | Adds shared button component for modals. |
| src/lib/modal/BaseModal.tsx | Adds a responsive BaseModal with focus/esc/click-outside handling. |
| src/lib/messageFormatter.ts | Improves formatting stripping to unwrap CTCP wrappers. |
| src/lib/ircUrlParser.ts | Replaces URL() parsing with manual parsing for Android compatibility. |
| src/lib/fuzzySearch.ts | Adds a fuzzy matching helper (scoring-based). |
| src/lib/eventGrouping.ts | Changes event grouping username derivation logic. |
| src/hooks/useSwipeNavigation.ts | Adds swipe-based page navigation hook. |
| src/hooks/useScrollToBottom.ts | Adds resilient “stick to bottom” tracking + WKWebView fix. |
| src/hooks/useReactions.ts | Fixes stale reaction state handling and DM lookup changes. |
| src/hooks/useModalBehavior.ts | Adds reusable escape/backdrop click behavior for modals. |
| src/hooks/useMediaQuery.ts | Adds debounced matchMedia hook and initializes from matchMedia(). |
| src/hooks/useLongPress.ts | Adds generic long-press detection hook for mobile. |
| src/hooks/useJoinAndSelectChannel.ts | Adds helper to join a channel then auto-select once present. |
| src/hooks/useDragReorder.ts | Adds pointer-based drag-to-reorder hook with rect caching. |
| src/hooks/useConnectionResilience.ts | Adds wake/online reconnect resilience (Tauri focus + sleep detection). |
| src/hooks/useChannelTabSwitching.ts | Adds Ctrl+Tab MRU switching for channels/PMs (Tauri only). |
| src/components/ui/settings/SettingRenderer.tsx | Adds a generic settings field renderer + wrapper field layout. |
| src/components/ui/settings/IgnoreListField.tsx | Adds custom ignore list editor setting component. |
| src/components/ui/settings/CustomMentionsField.tsx | Adds custom mentions editor setting component. |
| src/components/ui/UserProfileModal.tsx | Adds back-navigation, away controls, and uses openExternalUrl. |
| src/components/ui/UserContextMenu.tsx | Ports context menu to portal + backdrop and adds mobile-close behavior. |
| src/components/ui/TopicModal.tsx | Adds a topic view/edit modal using the shared modal library. |
| src/components/ui/TextInput.tsx | Adds mobile “Done” behavior + improved scroll-into-view handling. |
| src/components/ui/ScrollToBottomButton.tsx | Prevents focus stealing on mobile and restyles the button. |
| src/components/ui/ReplyBadge.tsx | Removes ReplyBadge component. |
| src/components/ui/ReactionPopover.tsx | Adds portal-based anchored emoji picker popover. |
| src/components/ui/ReactionModal.tsx | Switches to shared AppEmojiPicker and adjusts modal styling/text. |
| src/components/ui/QuickActions/uiActionConfig.tsx | Adds UI actions configuration for Quick Actions. |
| src/components/ui/QuickActions/types.ts | Adds types for Quick Actions results and UI actions. |
| src/components/ui/QuickActions/context.ts | Adds builder to derive Quick Actions context from store state. |
| src/components/ui/ModerationModal.tsx | Migrates moderation modal to BaseModal + shared components. |
| src/components/ui/LoadingOverlay.tsx | Adds a global loading overlay component. |
| src/components/ui/LinkWrapper.tsx | Improves link interception + uses openExternalUrl and regex factory. |
| src/components/ui/LinkSecurityWarningModal.tsx | Removes localhost warning logic and migrates container styling class. |
| src/components/ui/InviteUserModal.tsx | Migrates invite modal to BaseModal + shared components. |
| src/components/ui/InputToolbar.tsx | Adds optional mobile send button and visibility controls. |
| src/components/ui/ImagePreviewModal.tsx | Migrates image preview modal to BaseModal + shared components. |
| src/components/ui/HomeScreen.tsx | Updates discover flow; filters by platform; uses TextInput. |
| src/components/ui/HeaderOverflowMenu.tsx | Adds portal-based overflow menu anchored to an element. |
| src/components/ui/GifSelector.tsx | Switches search input to TextInput. |
| src/components/ui/ExternalLinkWarningModal.tsx | Migrates link warning modal to BaseModal + shared components. |
| src/components/ui/EmojiPickerModal.tsx | Switches to shared AppEmojiPicker and modal styling class. |
| src/components/ui/EmojiAutocompleteDropdown.tsx | Fixes unstable key usage for emoji list rendering. |
| src/components/ui/EditServerModal.tsx | Switches various inputs to TextInput and tags container class. |
| src/components/ui/ColorPicker.tsx | Adds narrow-view modal variant and refactors color/format grids. |
| src/components/ui/ChannelRenameModal.tsx | Migrates rename modal to BaseModal + shared components. |
| src/components/ui/BlankPage.tsx | Adds “Server Channels” button to open channel list modal. |
| src/components/ui/AvatarUpload.tsx | Switches fallback URL input to TextInput. |
| src/components/ui/AutocompleteDropdown.tsx | Adds narrow-view modal selector for @-triggered member picker. |
| src/components/ui/AppEmojiPicker.tsx | Adds shared emoji picker wrapper with mobile focus suppression. |
| src/components/ui/AddPrivateChatModal.tsx | Migrates to BaseModal + adds free-text DM option. |
| src/components/mobile/TouchableContextMenu.tsx | Avoids long-press when no items and cleans up naming/handlers. |
| src/components/mobile/ServerBottomSheet.tsx | Adds server actions bottom sheet for mobile. |
| src/components/mobile/MessageBottomSheet.tsx | Adds message actions bottom sheet for mobile. |
| src/components/mobile/BottomSheet.tsx | Adds reusable portal-based bottom sheet component. |
| src/components/message/index.ts | Exports ReactionsWithActions helper. |
| src/components/message/WhisperMessage.tsx | Switches to ReactionsWithActions and updates username derivation. |
| src/components/message/SwipeableMessage.tsx | Adds swipe-to-reply/delete + long-press bottom sheet wrapper. |
| src/components/message/ServerNoticesPopup.tsx | Fixes key usage for server notice messages. |
| src/components/message/ReactionsWithActions.tsx | Adds combined reactions + inline actions layout logic. |
| src/components/message/MessageReply.tsx | Redesigns reply preview UI and adds optional close button. |
| src/components/message/MessageReactions.tsx | Improves reaction pill UI and adds “add reaction” affordance. |
| src/components/message/MessageHeader.tsx | Updates username derivation and disables text selection. |
| src/components/message/MessageAvatar.tsx | Updates username derivation and disables text selection. |
| src/components/message/MessageActions.tsx | Restyles actions, changes icons, adds inline layout support. |
| src/components/message/LinkPreview.tsx | Improves URL detection (strip formatting) and uses openExternalUrl. |
| src/components/message/JsonLogMessage.tsx | Stabilizes list rendering keys for structured array values. |
| src/components/message/InviteMessage.tsx | Updates username derivation logic. |
| src/components/message/EventMessage.tsx | Updates username derivation and timestamp visibility/layout. |
| src/components/message/DateSeparator.tsx | Disables text selection on date separators. |
| src/components/message/CollapsibleMessage.tsx | Refactors collapsible logic and adds scroll anchoring on expand. |
| src/components/message/CollapsedEventMessage.tsx | Adjusts spacing and timestamp visibility. |
| src/components/message/ActionMessage.tsx | Adds swipe wrapper + reply preview + reactions/actions integration. |
| src/components/layout/ResizableSidebar.tsx | Adds initial width + width change callback and refines handle UX. |
| src/components/layout/MemberList.tsx | Fixes stale store usage for newly-created DMs and navigation. |
| src/App.tsx | Adds routing, deep links, quick actions, resilience hooks, overlays. |
| src-tauri/tauri.ios.conf.json | Adds iOS keyboard accessory config. |
| src-tauri/tauri.conf.json | Adds deep-link plugin config for desktop/mobile. |
| src-tauri/tauri.android.conf.json | Adds Android devUrl config for emulator dev. |
| src-tauri/src/lib.rs | Registers Tauri plugins and deep-link runtime registration and socket state. |
| src-tauri/gen/apple/project.yml | Adds URL schemes (irc/ircs) for iOS project generation. |
| src-tauri/gen/apple/ObsidianIRC_iOS/Info.plist | Updates app version and URL types entries. |
| src-tauri/gen/apple/ObsidianIRC.xcodeproj/project.pbxproj | Updates iOS deployment target and product name quoting. |
| src-tauri/gen/android/app/src/main/AndroidManifest.xml | Adds IRC deep-link intent filters. |
| src-tauri/gen/android/app/build.gradle.kts | Bumps Android compileSdk/targetSdk. |
| src-tauri/capabilities/default.json | Fixes permission list (adds opener/deep-link). |
| src-tauri/Cargo.toml | Adds deep-link/opener + TLS deps and platform-specific TLS selection. |
| scripts/setup-dev-certs.sh | Adds mkcert bootstrap script for dev TLS cert generation. |
| scripts/ios-dev.sh | Adds helper script for iOS devUrl configuration. |
| public/.well-known/assetlinks.json | Adds Android Asset Links for URL handling verification. |
| package.json | Bumps version and adds mobile build/dev scripts + dependencies updates. |
| index.html | Adds theme-color meta for better mobile browser theming. |
| docker/ergo.yaml | Enables TLS listeners for ircs/wss in local testing server. |
| compose.yaml | Adds cert generation service and mounts certs for TLS listeners. |
| biome.json | Updates Biome schema to a newer version. |
| README.md | Adds Google Play badge and removes websockets-only note. |
| CONTRIBUTING.md | Updates local test stack setup and TLS instructions. |
| ARCHITECTURE.md | Documents Biome dep suppression conventions and Zustand action stability. |
| .github/workflows/github_pages.yaml | Adds CNAME output during GitHub Pages publish. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
15
to
18
Comment on lines
7
to
11
Comment on lines
223
to
245
Comment on lines
47
to
55
Comment on lines
45
to
60
Comment on lines
56
to
68
Comment on lines
19
to
27
Comment on lines
44
to
46
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/ui/FloodSettingsModal.tsx (1)
41-43:⚠️ Potential issue | 🔴 CriticalRegex still allows "e" flood type which is invalid.
The character class
[cjkemntr]includes "e", butFloodRule["type"]only permits"c" | "j" | "k" | "m" | "n" | "t" | "r". If input contains "e", the cast at line 66 produces an invalid type that will breakgetTypeDescription,getDefaultAction, andgetAvailableActionslookups.🐛 Proposed fix
- const match = ruleStr.match(/^(\d+)([cjkemntr])(?:#([^#]+))?/); + const match = ruleStr.match(/^(\d+)([cjkmntr])(?:#([^#]+))?/);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/FloodSettingsModal.tsx` around lines 41 - 43, The regex in parseFloodRule allows an invalid "e" flood type causing FloodRule["type"] casts to be incorrect and breaking lookups in getTypeDescription, getDefaultAction, and getAvailableActions; update the match to only accept the permitted characters (remove "e" from the character class) or alternatively validate the captured type against the allowed union before casting and return null on invalid input so FloodRule["type"] is always one of "c"|"j"|"k"|"m"|"n"|"t"|"r".src/components/ui/ChannelSettingsModal.tsx (1)
341-342:⚠️ Potential issue | 🟠 MajororiginalModesRef is overwritten on every mode reload, breaking change detection.
This assignment runs on every call to
loadCurrentChannelModes, including when MODE events arrive. If a user is editing settings and a MODE event triggers a reload, their baseline snapshot changes and change detection becomes unreliable.Snapshot once when the modal opens, then only update after a successful apply:
🛡️ Proposed fix
// Store original modes for comparison - originalModesRef.current = parsedModes; + // Only snapshot on first load (empty ref) to keep baseline stable during edits + if (Object.keys(originalModesRef.current).length === 0) { + originalModesRef.current = parsedModes; + }Then reset the ref when the modal closes (in the existing cleanup effect at lines 203-208):
useEffect(() => { if (!isOpen) { hasFetchedRef.current = false; + originalModesRef.current = {}; } }, [isOpen]);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ChannelSettingsModal.tsx` around lines 341 - 342, originalModesRef is being overwritten on every loadCurrentChannelModes call (including MODE events), breaking change detection; modify logic so originalModesRef.current is set only once when the modal opens (e.g., in the code path that runs on modal open or initial load) and do not update it during subsequent reloads triggered by MODE events, then update originalModesRef.current only after a successful apply operation (the code path handling apply/save), and finally clear/reset originalModesRef.current in the modal cleanup effect (the existing cleanup for modal close) instead of on each loadCurrentChannelModes invocation.
🧹 Nitpick comments (3)
src/components/ui/FloodSettingsModal.tsx (2)
140-177: Consider consolidating type mappings into a single data structure.The three separate lookup objects (
descriptions,defaults,actions) duplicate the flood type keys. A single typed object would improve maintainability:♻️ Suggested refactor
const FLOOD_TYPE_CONFIG = { c: { description: "CTCP", defaultAction: "C", actions: ["C", "m", "M"] }, j: { description: "Join", defaultAction: "i", actions: ["i", "R"] }, k: { description: "Knock", defaultAction: "K", actions: ["K"] }, m: { description: "Messages (channel-wide)", defaultAction: "m", actions: ["m", "M"] }, n: { description: "Nickchange", defaultAction: "N", actions: ["N"] }, t: { description: "Text (per-user)", defaultAction: "kick", actions: ["kick", "b", "d"] }, r: { description: "Repeat", defaultAction: "kick", actions: ["kick", "d", "b"] }, } as const satisfies Record<FloodRule["type"], { description: string; defaultAction: string; actions: string[] }>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/FloodSettingsModal.tsx` around lines 140 - 177, Consolidate the three separate lookup objects by introducing a single typed config object (e.g., FLOOD_TYPE_CONFIG) and refactor getTypeDescription, getDefaultAction, and getAvailableActions to read from that config; specifically, create a Record keyed by FloodRule["type"] with { description, defaultAction, actions } entries for each type (c, j, k, m, n, t, r) and update getTypeDescription(type), getDefaultAction(type), and getAvailableActions(type) to return FLOOD_TYPE_CONFIG[type].description / .defaultAction / .actions respectively so the keys are defined in one place and typings are preserved.
379-400: Minor: Option labels have redundant descriptions.Options like
"CTCP ({getTypeDescription("c")})"render as "CTCP (CTCP)". The inline descriptions duplicate the short form. Consider using just the type code and description, e.g.,"c - CTCP".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/FloodSettingsModal.tsx` around lines 379 - 400, The option labels currently render duplicated text because they show the full name and also call getTypeDescription (e.g., "CTCP ({getTypeDescription('c')})"). Update each <option> in the FloodSettingsModal select so the visible label uses the short code plus the description (for example: "c - CTCP" or by composing `${'c'} - ${getTypeDescription('c')}`) instead of rendering the full name and getTypeDescription together; change all options that call getTypeDescription("c"/"j"/"k"/"m"/"n"/"t"/"r") to this single-code-and-description format to eliminate the redundancy.src/components/ui/ChannelSettingsModal.tsx (1)
27-32: Type mismatch with FloodSettingsModal's FloodRule.This
FloodRulelacks theidproperty thatFloodSettingsModalincludes. While structural typing makes this work at runtime (extra properties are ignored), having two differentFloodRuledefinitions creates maintenance confusion.Consider either:
- Exporting
FloodRulefromFloodSettingsModaland importing it here- Using
Omit<FloodRule, 'id'>explicitly if the id is intentionally excluded🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ChannelSettingsModal.tsx` around lines 27 - 32, The local FloodRule interface here conflicts with the FloodSettingsModal definition (missing id); replace the duplicate by importing the canonical type from FloodSettingsModal (export its FloodRule there) or, if id is intentionally excluded, change this declaration to use Omit<FloodRuleFromModal,'id'>; update references in this file to use the imported/omitted type so both modules share a single authoritative FloodRule definition.
🤖 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/ChannelSettingsModal.tsx`:
- Around line 920-925: The useEffect that calls loadCurrentChannelModes() with
dependencies [isOpen, channel, loadCurrentChannelModes] is duplicated (the other
identical effect exists at lines 909-918); remove the redundant useEffect block
so loadCurrentChannelModes is only invoked once when isOpen, channel, or the
loadCurrentChannelModes function changes, keeping the original effect that
correctly handles mode loading.
In `@src/store/index.ts`:
- Around line 5668-5669: The target classification uses a brittle check
target.startsWith("#") which misclassifies channels that use other valid
prefixes; replace this with a channel-validation call (e.g. call
normalizeChannelName from ircUrlParser or create a small isChannelTarget helper
that applies the same prefix rules) and use that helper in the MODE handling
path (and mirror the same change where similar checks exist, e.g. in the
protocol MODE handler and irc client). Ensure the new check returns true for any
valid channel prefix (#, &, +, !) and false for user nicknames so channel MODE
events are routed to the channel-mode branch.
---
Duplicate comments:
In `@src/components/ui/ChannelSettingsModal.tsx`:
- Around line 341-342: originalModesRef is being overwritten on every
loadCurrentChannelModes call (including MODE events), breaking change detection;
modify logic so originalModesRef.current is set only once when the modal opens
(e.g., in the code path that runs on modal open or initial load) and do not
update it during subsequent reloads triggered by MODE events, then update
originalModesRef.current only after a successful apply operation (the code path
handling apply/save), and finally clear/reset originalModesRef.current in the
modal cleanup effect (the existing cleanup for modal close) instead of on each
loadCurrentChannelModes invocation.
In `@src/components/ui/FloodSettingsModal.tsx`:
- Around line 41-43: The regex in parseFloodRule allows an invalid "e" flood
type causing FloodRule["type"] casts to be incorrect and breaking lookups in
getTypeDescription, getDefaultAction, and getAvailableActions; update the match
to only accept the permitted characters (remove "e" from the character class) or
alternatively validate the captured type against the allowed union before
casting and return null on invalid input so FloodRule["type"] is always one of
"c"|"j"|"k"|"m"|"n"|"t"|"r".
---
Nitpick comments:
In `@src/components/ui/ChannelSettingsModal.tsx`:
- Around line 27-32: The local FloodRule interface here conflicts with the
FloodSettingsModal definition (missing id); replace the duplicate by importing
the canonical type from FloodSettingsModal (export its FloodRule there) or, if
id is intentionally excluded, change this declaration to use
Omit<FloodRuleFromModal,'id'>; update references in this file to use the
imported/omitted type so both modules share a single authoritative FloodRule
definition.
In `@src/components/ui/FloodSettingsModal.tsx`:
- Around line 140-177: Consolidate the three separate lookup objects by
introducing a single typed config object (e.g., FLOOD_TYPE_CONFIG) and refactor
getTypeDescription, getDefaultAction, and getAvailableActions to read from that
config; specifically, create a Record keyed by FloodRule["type"] with {
description, defaultAction, actions } entries for each type (c, j, k, m, n, t,
r) and update getTypeDescription(type), getDefaultAction(type), and
getAvailableActions(type) to return FLOOD_TYPE_CONFIG[type].description /
.defaultAction / .actions respectively so the keys are defined in one place and
typings are preserved.
- Around line 379-400: The option labels currently render duplicated text
because they show the full name and also call getTypeDescription (e.g., "CTCP
({getTypeDescription('c')})"). Update each <option> in the FloodSettingsModal
select so the visible label uses the short code plus the description (for
example: "c - CTCP" or by composing `${'c'} - ${getTypeDescription('c')}`)
instead of rendering the full name and getTypeDescription together; change all
options that call getTypeDescription("c"/"j"/"k"/"m"/"n"/"t"/"r") to this
single-code-and-description format to eliminate the redundancy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9be42559-d9fa-44cc-832e-a4f45a8bb72f
📒 Files selected for processing (7)
src/components/ui/ChannelSettingsModal.tsxsrc/components/ui/FloodSettingsModal.tsxsrc/index.csssrc/lib/ircClient.tssrc/protocol/isupport.tssrc/store/index.tssrc/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/ircClient.ts
- src/protocol/isupport.ts
Comment on lines
+5668
to
+5669
| // Handle user mode responses (channel modes are handled by the protocol handler) | ||
| if (!target.startsWith("#")) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify MODE target classification and related channel-prefix handling paths.
rg -n --type=ts -C2 'target\.startsWith\("#"\)|ircClient\.on\("MODE"|CHANTYPES|chanmodes' srcRepository: ObsidianIRC/ObsidianIRC
Length of output: 6873
🏁 Script executed:
rg -n --type=ts 'CHANTYPES|chantypes' srcRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for ISUPPORT parsing to see which parameters are handled
rg -n --type=ts 'ISUPPORT' src | head -40Repository: ObsidianIRC/ObsidianIRC
Length of output: 686
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or server config about supported channel types
fd -e ts -e js src | xargs grep -l "channel.*prefix\|&\|~" | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
cat -n src/protocol/isupport.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 3637
🏁 Script executed:
#!/bin/bash
# Search for where channel target type is determined or used
rg -n --type=ts 'isChannel|startsWith' src/store/index.ts | grep -A2 -B2 "5669\|6983"Repository: ObsidianIRC/ObsidianIRC
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Check if there's any reference to non-# channels or alternative prefixes in comments/docs
rg -n --type=ts 'channel.*prefix|&\|!\|~|RFC.*2811' srcRepository: ObsidianIRC/ObsidianIRC
Length of output: 846
🏁 Script executed:
sed -n '102,115p' src/lib/ircUrlParser.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Check what "valid IRC channel prefix" means in context
sed -n '100,120p' src/lib/ircUrlParser.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 692
🏁 Script executed:
#!/bin/bash
# Search for the actual implementation of channel prefix validation
rg -n --type=ts 'normalizeChannelName|valid.*channel.*prefix' -A5 src/lib/ircUrlParser.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 726
🏁 Script executed:
#!/bin/bash
# Find all places using startsWith("#") that are related to channel detection
rg -n --type=ts 'startsWith\("#"\)' srcRepository: ObsidianIRC/ObsidianIRC
Length of output: 1508
🏁 Script executed:
#!/bin/bash
# Check if normalizeChannelName is used in protocol handlers
rg -n --type=ts 'normalizeChannelName|isChannel.*startsWith' srcRepository: ObsidianIRC/ObsidianIRC
Length of output: 757
Handle non-# channel targets when classifying MODE events.
At Line 5669, checking only target.startsWith("#") can misroute channel MODE events on networks using other valid channel prefixes (e.g., &, +, !) into the user-mode path. The codebase acknowledges these prefixes exist (via normalizeChannelName in ircUrlParser.ts), but protocol handlers don't use it. Similar hardcoded checks exist in src/protocol/mode.ts:15 and src/lib/ircClient.ts.
Suggested fix
Replace with a helper or use the pattern from normalizeChannelName:
- if (!target.startsWith("#")) {
+ const isChannelTarget = ["#", "&", "+", "!"].some(prefix => target.startsWith(prefix));
+ if (!isChannelTarget) {Or refactor to reuse the validation logic already defined in src/lib/ircUrlParser.ts.
📝 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
| // Handle user mode responses (channel modes are handled by the protocol handler) | |
| if (!target.startsWith("#")) { | |
| // Handle user mode responses (channel modes are handled by the protocol handler) | |
| const isChannelTarget = ["#", "&", "+", "!"].some(prefix => target.startsWith(prefix)); | |
| if (!isChannelTarget) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/index.ts` around lines 5668 - 5669, The target classification uses
a brittle check target.startsWith("#") which misclassifies channels that use
other valid prefixes; replace this with a channel-validation call (e.g. call
normalizeChannelName from ircUrlParser or create a small isChannelTarget helper
that applies the same prefix rules) and use that helper in the MODE handling
path (and mirror the same change where similar checks exist, e.g. in the
protocol MODE handler and irc client). Ensure the new check returns true for any
valid channel prefix (#, &, +, !) and false for user nicknames so channel MODE
events are routed to the channel-mode branch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/App.tsx (1)
216-258:⚠️ Potential issue | 🟡 MinorRace condition in async cleanup pattern.
If the component unmounts before
onOpenUrlresolves, the cleanup function runs whileunlistenis stillnull, leaving the listener registered. SinceAppis the root component this is unlikely in practice, but the pattern is worth fixing for correctness.🛡️ Proposed fix using a mounted flag
useEffect(() => { let unlisten: (() => void) | null = null; + let isMounted = true; const setupDeepLinkHandler = async () => { if (!isTauri()) { return; } try { // Register handler for when app is already running; store unlisten for cleanup - unlisten = await onOpenUrl((urls) => { + const unlistenFn = await onOpenUrl((urls) => { console.log("Deep link received:", urls); for (const url of urls) { if (url.startsWith("irc://") || url.startsWith("ircs://")) { try { // Parse the IRC URL const parsed = parseIrcUrl(url); // Open the connect modal with pre-filled details toggleAddServerModal(true, { name: parsed.host || "IRC Server", host: parsed.host, port: parsed.port.toString(), nickname: parsed.nick || "user", useWebSocket: false, }); } catch (error) { console.error("Failed to parse IRC URL:", error); } } } }); + if (isMounted) { + unlisten = unlistenFn; + } else { + // Component unmounted while awaiting; clean up immediately + unlistenFn(); + } } catch (error) { console.error("Failed to setup deep link handler:", error); } }; setupDeepLinkHandler(); return () => { + isMounted = false; if (unlisten) { unlisten(); } }; }, [toggleAddServerModal]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 216 - 258, The async setupDeepLinkHandler can leave the listener registered if the component unmounts before onOpenUrl resolves; fix by adding a mounted flag and ensuring any unlisten returned after await is immediately invoked if the component has unmounted. Concretely, keep the outer unlisten variable as-is, introduce a boolean like isMounted = true before calling setupDeepLinkHandler, set isMounted = false in the cleanup, and after awaiting onOpenUrl assign the result to unlisten and immediately call unlisten() if isMounted is false; keep existing try/catch around onOpenUrl and preserve use of parseIrcUrl and toggleAddServerModal inside the listener.src/protocol/mode.ts (1)
82-93:⚠️ Potential issue | 🔴 CriticalTrack current MODE action across the entire run (
+/-) instead of only previous character.With current logic,
-ntparsesnas-buttas+, which is incorrect and breaks compact unsetting.Proposed fix
const changes: ModeChange[] = []; let argIndex = 0; + let currentAction: "+" | "-" = "+"; @@ for (let i = 0; i < modestring.length; i++) { const char = modestring[i]; if (char === "+" || char === "-") { - // This is a mode action + currentAction = char; continue; } - - const action = - i > 0 && (modestring[i - 1] === "+" || modestring[i - 1] === "-") - ? modestring[i - 1] - : "+"; + const action = currentAction;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protocol/mode.ts` around lines 82 - 93, The parser currently infers the mode action from the previous character, causing sequences like "-nt" to be parsed as "-" for 'n' and "+" for 't'; fix this by introducing a persistent currentAction variable (initialize to '+') before the loop that scans modestring, update currentAction whenever you see a '+' or '-' (set currentAction = char and continue), and replace the existing action computation with action = currentAction so every subsequent mode char uses the last seen action across the entire run (refer to modestring, the for loop, and action variable in the diff).src/store/index.ts (1)
5691-5726:⚠️ Potential issue | 🟠 MajorScope
currentUsermode updates to the selected server.
state.currentUseris global, and nearby handlers already treat it as selected-server state. If the same nick is connected on two servers, a MODE from a background server will matchtargetand overwrite the selected server'scurrentUser.modes/isIrcOphere.Suggested fix
useStore.setState((state) => { + const isSelectedServer = state.ui.selectedServerId === serverId; // Check if this is the current user const currentUser = state.currentUser; if ( + isSelectedServer && currentUser && currentUser.username.toLowerCase() === target.toLowerCase() ) { const nextModes = applyUserModeDelta( currentUser.modes || "", modestring, ); return { currentUser: { ...currentUser, modes: nextModes, isIrcOp: nextModes.includes("o"), }, }; } // If no currentUser in store, check if this MODE is for the IRC current user const ircCurrentUser = ircClient.getCurrentUser(serverId); if ( + isSelectedServer && !currentUser && ircCurrentUser && ircCurrentUser.username.toLowerCase() === target.toLowerCase() ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 5691 - 5726, The currentUser mode update must be scoped to the selected server: when inside useStore.setState (and before comparing currentUser.username to target) ensure the stored currentUser belongs to the same serverId (e.g. check state.selectedServerId === serverId or state.currentUser.serverId === serverId) so a MODE from another server cannot overwrite the selected-server currentUser; apply the same serverId check in the fallback branch that uses ircClient.getCurrentUser(serverId) (before applying applyUserModeDelta with modestring) so only the matching server's user gets updated and isIrcOp is set accordingly.
♻️ Duplicate comments (3)
src/components/ui/FloodSettingsModal.tsx (1)
116-119:⚠️ Potential issue | 🟠 MajorFlood type parser still accepts unsupported
emode.Line 118 allows
e, butFloodRule["type"]does not. This can parse invalid values and then force-cast them at Line 141.Proposed fix
- const match = ruleStr.match(/^(\d+)([cjkemntr])(?:#([^#]+))?/); + const match = ruleStr.match(/^(\d+)([cjkmntr])(?:#([^#]+))?/);Also applies to: 141-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/FloodSettingsModal.tsx` around lines 116 - 119, The parser parseFloodRule currently accepts an unsupported 'e' mode via its regex and then force-casts the captured type into FloodRule.type later (around where the FloodRule object is constructed), which can produce invalid values; fix this by restricting the regex character class to only the valid type letters (use the exact set used by FloodRule.type, e.g., [cjkmntr] or the correct allowed chars), and add an explicit runtime check that the captured type (match[2]) is one of the allowed values before creating/returning the FloodRule (otherwise return null) so you can remove the unsafe cast.src/store/index.ts (1)
5688-5689:⚠️ Potential issue | 🟡 MinorHandle non-
#channel targets here too.
&,+, and!channel names still fall into the user-mode path here, so MODE on those networks is misclassified. Reuse the same channel-target helper or prefix set here instead of hardcoding#.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 5688 - 5689, The MODE handling currently checks only target.startsWith("#"), which misclassifies channels that use other prefixes (&,+,!) as user targets; replace this hardcoded check with the existing channel-target helper or the canonical channel prefix set used elsewhere (e.g., the isChannelTarget function or CHANNEL_PREFIXES constant) so the same logic that recognizes all channel prefixes is reused in the MODE branch and non-# channels are routed to channel-mode handling.src/components/ui/ChannelSettingsModal.tsx (1)
975-980:⚠️ Potential issue | 🟡 MinorDuplicate mode-sync
useEffectstill triggers redundant reloads.This is the same duplicate-effect issue previously reported and it still appears unresolved in this revision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ChannelSettingsModal.tsx` around lines 975 - 980, There are two identical useEffect blocks that call loadCurrentChannelModes and cause redundant reloads; remove the duplicate effect so only one useEffect monitors isOpen, channel, and loadCurrentChannelModes (the remaining effect should read: useEffect(() => { if (isOpen && channel) { loadCurrentChannelModes(); } }, [isOpen, channel, loadCurrentChannelModes])); ensure no other useEffect in ChannelSettingsModal.tsx duplicates this behavior and that loadCurrentChannelModes remains memoized or stable to avoid unnecessary triggers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/message/CollapsibleMessage.tsx`:
- Around line 50-69: The ResizeObserver is attached to the overflow-hidden
wrapper so it never fires when scrollHeight grows; update the observer to watch
the inner content element (the element whose scrollHeight changes) instead of
the clipped container: in CollapsibleMessage.tsx locate the measure function and
the ResizeObserver usage and change observe(element) to
observe(innerContentElement) (the inner DOM node that renders htmlContent),
ensure measure reads computedStyle/scrollHeight from that inner node, keep
setContentHeight/setCollapsedMaxHeight/setNeedsCollapsing and call
onNeedsCollapsing with the computed needs, and ensure the observer is
disconnected in the existing cleanup.
In `@src/components/ui/ChannelSettingsModal.tsx`:
- Around line 465-470: The delayed list-refresh calls use addTimeout(...) with
callbacks that call ircClient.sendRaw(serverId, `MODE ${channelName} ...`)
fire-and-forget, so convert those timeout callbacks (the one around clearLists
and the ones at lines similar to 497-502 and 560-565) into async functions that
await each ircClient.sendRaw and wrap the awaits in try/catch; on error call the
existing error handler (e.g., show a toast or processLogger) and/or trigger a UI
re-sync so failures don’t leave the UI desynced. Locate the callbacks
referencing addTimeout, clearLists, and ircClient.sendRaw and add the
await/try-catch logic around those sendRaw calls.
- Around line 57-62: The stored timeouts in timeoutsRef (and the addTimeout
helper) are only cleared on unmount, so when the modal closes or the channel
switches queued callbacks can still run; update ChannelSettingsModal to clear
pending timers immediately on modal close and on channel changes by iterating
timeoutsRef.current and calling clearTimeout for each id and then resetting
timeoutsRef.current = [], and invoke this clearing logic inside the modal close
handler and in a useEffect that watches the modal open state (isOpen) and the
active channel id so timers are cleared whenever the modal is closed or the
channel switches (also apply the same clear-on-close logic to the other timeout
groups referenced around the other timeout refs).
- Around line 199-207: When opening ChannelSettingsModal (useEffect observing
isOpen, userHasOpPermission, supportsMetadata) ensure activeTab is reset if the
previously selected tab is not valid for the current channel/permissions: detect
the current activeTab value and if it is "general"/"settings"/"advanced" but the
corresponding feature is unavailable (e.g., !supportsMetadata or
!userHasOpPermission), call setActiveTab to a safe fallback such as "categories"
(or the first available tab) so the modal never renders an unavailable pane;
update the useEffect that currently setsMobileView and conditionally sets
"general" to also validate and reset activeTab accordingly.
- Around line 316-321: The current check in ChannelSettingsModal that treats
param === "*" as hidden should be removed because "*" can be a legitimate +k
value on some IRCds; change the logic in the block that references mode and
param (the if that checks (mode === "k" || mode === "H" || mode === "L") &&
(param === null || param === "*")) so it only treats param === null as the
hidden marker and does not coerce "*" to "__HIDDEN__"; if server-specific
masking is required, implement a separate server-type-aware check elsewhere
instead of conflating "*" with a hidden value in this function.
In `@src/components/ui/FloodSettingsModal.tsx`:
- Around line 109-112: The modal's local state (floodProfile, floodRules,
seconds, isSaving) must be reset from incoming props each time the modal opens
to avoid cross-channel leakage; in the FloodSettingsModal component, add logic
that on open (watch the prop that controls visibility/opening) you
setFloodProfile(initialFloodProfile), setFloodRules(props.floodRules || []),
setSeconds(props.seconds || 60) and setIsSaving(false), and ensure the existing
branch that skips resetting for channel === "Default" (around the logic
referenced at lines ~186-188) instead explicitly clears stale floodRules when
switching to Default so stale rules are not preserved. Use the component's
open/visible prop and the unique symbols floodProfile, floodRules, seconds,
isSaving, and initialFloodProfile to locate and implement this reset behavior.
- Around line 276-281: The close button in FloodSettingsModal (the button with
onClick={onClose} that renders <FaTimes />) is icon-only and missing an
accessible label; add an accessible name by supplying an aria-label (e.g.,
aria-label="Close") or include visually hidden text inside the button so screen
readers announce its purpose while keeping the same visual icon and behavior.
In `@src/protocol/mode.ts`:
- Around line 318-323: The code builds a modestring starting with "+" and ends
up serializing an empty modes object as "+" when parsedModes is empty; update
the logic in the mode construction (referencing parsedModes, sortedModes,
modestring, and modeArgs) to detect when there are no modes (sortedModes.length
=== 0) and produce an empty string (or omit the "+" prefix) instead of "+";
apply the same guard to the similar construction at the second occurrence
(around the block using the same variables at lines ~340-343) so that when no
mode characters or modeArgs exist the serialized modes value is an empty string
rather than "+".
- Around line 285-291: The parsing and regeneration code
(parseCurrentChannelModes and generateModestringAndArgs) diverge when CHANMODES
is missing, causing dropped mode args; unify behavior by applying the same
fallback CHANMODES semantics in both places: when CHANMODES is undefined or
empty, treat mode classes so that 'k' and 'l' are treated as parameter-required
(groupA/groupB as in existing groupA/groupB arrays) and apply the user/list
convention for groupC (use currentAction === "+" to require an arg only on +),
i.e., centralize the hasParam logic into a single helper (or reuse the same
check) that checks CHANMODES fallback and returns whether a given mode needs an
argument given currentAction, then replace the duplicated hasParam computations
in both parseCurrentChannelModes and generateModestringAndArgs (and the
corresponding block at the other location referenced) to call that helper.
---
Outside diff comments:
In `@src/App.tsx`:
- Around line 216-258: The async setupDeepLinkHandler can leave the listener
registered if the component unmounts before onOpenUrl resolves; fix by adding a
mounted flag and ensuring any unlisten returned after await is immediately
invoked if the component has unmounted. Concretely, keep the outer unlisten
variable as-is, introduce a boolean like isMounted = true before calling
setupDeepLinkHandler, set isMounted = false in the cleanup, and after awaiting
onOpenUrl assign the result to unlisten and immediately call unlisten() if
isMounted is false; keep existing try/catch around onOpenUrl and preserve use of
parseIrcUrl and toggleAddServerModal inside the listener.
In `@src/protocol/mode.ts`:
- Around line 82-93: The parser currently infers the mode action from the
previous character, causing sequences like "-nt" to be parsed as "-" for 'n' and
"+" for 't'; fix this by introducing a persistent currentAction variable
(initialize to '+') before the loop that scans modestring, update currentAction
whenever you see a '+' or '-' (set currentAction = char and continue), and
replace the existing action computation with action = currentAction so every
subsequent mode char uses the last seen action across the entire run (refer to
modestring, the for loop, and action variable in the diff).
In `@src/store/index.ts`:
- Around line 5691-5726: The currentUser mode update must be scoped to the
selected server: when inside useStore.setState (and before comparing
currentUser.username to target) ensure the stored currentUser belongs to the
same serverId (e.g. check state.selectedServerId === serverId or
state.currentUser.serverId === serverId) so a MODE from another server cannot
overwrite the selected-server currentUser; apply the same serverId check in the
fallback branch that uses ircClient.getCurrentUser(serverId) (before applying
applyUserModeDelta with modestring) so only the matching server's user gets
updated and isIrcOp is set accordingly.
---
Duplicate comments:
In `@src/components/ui/ChannelSettingsModal.tsx`:
- Around line 975-980: There are two identical useEffect blocks that call
loadCurrentChannelModes and cause redundant reloads; remove the duplicate effect
so only one useEffect monitors isOpen, channel, and loadCurrentChannelModes (the
remaining effect should read: useEffect(() => { if (isOpen && channel) {
loadCurrentChannelModes(); } }, [isOpen, channel, loadCurrentChannelModes]));
ensure no other useEffect in ChannelSettingsModal.tsx duplicates this behavior
and that loadCurrentChannelModes remains memoized or stable to avoid unnecessary
triggers.
In `@src/components/ui/FloodSettingsModal.tsx`:
- Around line 116-119: The parser parseFloodRule currently accepts an
unsupported 'e' mode via its regex and then force-casts the captured type into
FloodRule.type later (around where the FloodRule object is constructed), which
can produce invalid values; fix this by restricting the regex character class to
only the valid type letters (use the exact set used by FloodRule.type, e.g.,
[cjkmntr] or the correct allowed chars), and add an explicit runtime check that
the captured type (match[2]) is one of the allowed values before
creating/returning the FloodRule (otherwise return null) so you can remove the
unsafe cast.
In `@src/store/index.ts`:
- Around line 5688-5689: The MODE handling currently checks only
target.startsWith("#"), which misclassifies channels that use other prefixes
(&,+,!) as user targets; replace this hardcoded check with the existing
channel-target helper or the canonical channel prefix set used elsewhere (e.g.,
the isChannelTarget function or CHANNEL_PREFIXES constant) so the same logic
that recognizes all channel prefixes is reused in the MODE branch and non-#
channels are routed to channel-mode handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59b83463-0deb-4051-bfd3-ec9d7328b619
⛔ Files ignored due to path filters (1)
src-tauri/gen/apple/ObsidianIRC_iOS/Info.plistis excluded by!**/gen/**
📒 Files selected for processing (12)
src/App.tsxsrc/components/message/CollapsibleMessage.tsxsrc/components/message/MessageReactions.tsxsrc/components/ui/ChannelSettingsModal.tsxsrc/components/ui/ColorPicker.tsxsrc/components/ui/FloodSettingsModal.tsxsrc/components/ui/HomeScreen.tsxsrc/hooks/useJoinAndSelectChannel.tssrc/hooks/useMediaQuery.tssrc/lib/platformUtils.tssrc/protocol/mode.tssrc/store/index.ts
Comment on lines
+57
to
+62
| const timeoutsRef = useRef<ReturnType<typeof setTimeout>[]>([]); | ||
| const addTimeout = useCallback((fn: () => void, delay: number) => { | ||
| const t = setTimeout(fn, delay); | ||
| timeoutsRef.current.push(t); | ||
| return t; | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Clear pending timers on modal close/channel switch, not just unmount.
Timeout callbacks from a previous channel/open can still run and overwrite state after close/reopen. Current cleanup only runs on unmount.
Suggested fix
+const clearPendingTimeouts = useCallback(() => {
+ for (const t of timeoutsRef.current) clearTimeout(t);
+ timeoutsRef.current = [];
+}, []);
+
// Reset fetch state when modal closes
useEffect(() => {
if (!isOpen) {
hasFetchedRef.current = false;
+ clearPendingTimeouts();
}
-}, [isOpen]);
+}, [isOpen, clearPendingTimeouts]);
// Cancel pending timeouts when component unmounts
useEffect(() => {
return () => {
- for (const t of timeoutsRef.current) clearTimeout(t);
- timeoutsRef.current = [];
+ clearPendingTimeouts();
};
-}, []);
+}, [clearPendingTimeouts]);Also applies to: 209-214, 945-951
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/ChannelSettingsModal.tsx` around lines 57 - 62, The stored
timeouts in timeoutsRef (and the addTimeout helper) are only cleared on unmount,
so when the modal closes or the channel switches queued callbacks can still run;
update ChannelSettingsModal to clear pending timers immediately on modal close
and on channel changes by iterating timeoutsRef.current and calling clearTimeout
for each id and then resetting timeoutsRef.current = [], and invoke this
clearing logic inside the modal close handler and in a useEffect that watches
the modal open state (isOpen) and the active channel id so timers are cleared
whenever the modal is closed or the channel switches (also apply the same
clear-on-close logic to the other timeout groups referenced around the other
timeout refs).
Comment on lines
+276
to
+281
| <button | ||
| onClick={onClose} | ||
| className="text-discord-text-muted hover:text-white" | ||
| > | ||
| <FaTimes size={20} /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible label to the icon-only close button.
The close button has no text label, so screen readers won’t announce its purpose.
Proposed fix
<button
onClick={onClose}
+ aria-label="Close flood settings"
className="text-discord-text-muted hover:text-white"
>📝 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
| <button | |
| onClick={onClose} | |
| className="text-discord-text-muted hover:text-white" | |
| > | |
| <FaTimes size={20} /> | |
| </button> | |
| <button | |
| onClick={onClose} | |
| aria-label="Close flood settings" | |
| className="text-discord-text-muted hover:text-white" | |
| > | |
| <FaTimes size={20} /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/FloodSettingsModal.tsx` around lines 276 - 281, The close
button in FloodSettingsModal (the button with onClick={onClose} that renders
<FaTimes />) is icon-only and missing an accessible label; add an accessible
name by supplying an aria-label (e.g., aria-label="Close") or include visually
hidden text inside the button so screen readers announce its purpose while
keeping the same visual icon and behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/ui/FloodSettingsModal.tsx (2)
281-286:⚠️ Potential issue | 🟡 MinorAdd aria-label to icon-only close button for accessibility.
The close button renders only an icon (
<FaTimes />), making it inaccessible to screen readers.♿ Proposed fix
<button onClick={onClose} + aria-label="Close flood settings" className="text-discord-text-muted hover:text-white" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/FloodSettingsModal.tsx` around lines 281 - 286, The close button in FloodSettingsModal is icon-only (renders <FaTimes />) and needs an accessible label; update the button element (the one with onClick={onClose} and className="text-discord-text-muted hover:text-white") to include an appropriate aria-label such as aria-label="Close" or aria-label="Close modal" so screen readers can announce its purpose (you can also add type="button" if missing for clarity).
116-145:⚠️ Potential issue | 🟡 MinorRegex still includes "e" which is not a valid flood type.
The regex at line 118 uses
[cjkemntr]butFloodRule["type"]only allows"c" | "j" | "k" | "m" | "n" | "t" | "r". The "e" character in the regex doesn't match the type union and will cause a type mismatch if parsed. Previous review indicated this was addressed, but the "e" is still present.🐛 Proposed fix
- const match = ruleStr.match(/^(\d+)([cjkemntr])(?:#([^#]+))?/); + const match = ruleStr.match(/^(\d+)([cjkmntr])(?:#([^#]+))?/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/FloodSettingsModal.tsx` around lines 116 - 145, The parseFloodRule function's regex character class includes an invalid "e"; update the regex in parseFloodRule to use the correct set that matches FloodRule["type"] by replacing ([cjkemntr]) with ([cjkmntr]) so parsed types align with the FloodRule union, keep the rest of the parsing logic and the type assertion (type as FloodRule["type"]) unchanged, and consider updating the inline comment to reflect the exact allowed types.
🧹 Nitpick comments (3)
src/components/ui/ChannelSettingsModal.tsx (2)
990-995: Potentially redundant effect; consider consolidating or narrowing dependencies.This effect overlaps with lines 978-988 which also calls
loadCurrentChannelModeswhenisOpenandchannelchange. The difference is this one doesn't reset the baseline. Consider:
- Narrowing this effect to watch
channel.modes/channel.modeArgsspecifically for MODE event sync, or- Consolidating both effects if the behavior should be identical
💡 Option: Watch specific mode fields for sync
- // Update local mode state when channel modes change (e.g., from MODE events) - useEffect(() => { - if (isOpen && channel) { - loadCurrentChannelModes(); - } - }, [isOpen, channel, loadCurrentChannelModes]); + // Sync UI state when channel modes change from external MODE events + const channelModes = channel?.modes; + const channelModeArgs = channel?.modeArgs; + useEffect(() => { + if (isOpen && channel) { + loadCurrentChannelModes(); + } + }, [isOpen, channelModes, channelModeArgs, loadCurrentChannelModes]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ChannelSettingsModal.tsx` around lines 990 - 995, The duplicate useEffect calling loadCurrentChannelModes should be consolidated or narrowed: either merge this effect with the earlier effect around lines 978-988 so both opening and subsequent MODE updates use the same logic, or change this effect’s dependency from broad channel to specific fields like channel.modes and channel.modeArgs (while keeping isOpen) so it only runs for MODE-sync updates; update references to loadCurrentChannelModes, isOpen, channel, and any baseline-reset logic (e.g., resetBaseline or the other effect) accordingly to avoid redundant calls.
27-32: Consider sharing FloodRule type between modals.
FloodRuleis defined both here (withoutid) and inFloodSettingsModal.tsx(withid). The callback signature at line 126 expects the FloodSettingsModal's version withid, but this local type omits it.💡 Share the type definition
Export
FloodRulefromFloodSettingsModal.tsxand import it here, or create a shared types file.-interface FloodRule { - amount: number; - type: "c" | "j" | "k" | "m" | "n" | "t" | "r"; - action?: string; - time?: number; // in minutes -} +import FloodSettingsModal, { type FloodRule } from "./FloodSettingsModal";Then in
FloodSettingsModal.tsx:-interface FloodRule { +export interface FloodRule {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ChannelSettingsModal.tsx` around lines 27 - 32, The local FloodRule type here conflicts with the FloodRule used by FloodSettingsModal (missing id) causing the callback signature mismatch; export the FloodRule type (including id) from FloodSettingsModal.tsx or move it to a shared types file, then import that shared FloodRule into ChannelSettingsModal.tsx and update all references (including the callback used around the FloodSettingsModal invocation) to use the imported/shared FloodRule so both modals share the exact same definition.src/components/ui/FloodSettingsModal.tsx (1)
431-452: Verify type select option labels match definitions.The option labels duplicate the type name and call
getTypeDescription()which returns the same label. For example,"CTCP ({getTypeDescription("c")})"renders as"CTCP (CTCP)".💡 Suggested simplification
- <option value="c"> - CTCP ({getTypeDescription("c")}) - </option> + <option value="c">CTCP</option>Or use only the description:
- <option value="c"> - CTCP ({getTypeDescription("c")}) - </option> + <option value="c">{getTypeDescription("c")}</option>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/FloodSettingsModal.tsx` around lines 431 - 452, The option labels in the FloodSettingsModal select currently duplicate the type name and call getTypeDescription (e.g. "CTCP ({getTypeDescription("c")})") which renders as "CTCP (CTCP)"; update the option inner text for each value ("c","j","k","m","n","t","r") to avoid duplication—either use only getTypeDescription("x") as the label or format as `label (description)` but ensure getTypeDescription returns a different description if you keep both; change the option contents in the select block that renders CTCP/Join/Knock/Messages/Nickchange/Text/Repeat to use the chosen non-duplicating label approach and reference getTypeDescription and the option value attributes when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/ChannelSettingsModal.tsx`:
- Around line 1998-2003: The desktop close button in ChannelSettingsModal (the
<button> with onClick={onClose} rendering <FaTimes />) has no accessible label;
add an appropriate aria-label (e.g., "Close channel settings" or "Close") to
that button so screen readers can announce its purpose, matching the mobile
close button behavior.
- Around line 311-317: The inline hasParam determination in
ChannelSettingsModal.tsx incorrectly assumes CHANMODES groups are present;
update it to either call the centralized modeRequiresArg helper from
src/protocol/mode.ts or replicate its fallback behavior (treat 'bklimnpst' as
default groups when chanmodes is undefined/empty) so modes like 'k' and 'l'
correctly require parameters; locate the hasParam logic (the block checking
groupA/groupB/groupC and currentAction) and replace it with a call to
modeRequiresArg(mode, chanmodes, currentAction) or add the same fallback
semantics before evaluating group membership.
- Around line 801-804: In ChannelSettingsModal update the unset argument
handling for modes: when unsetting history ('H') remove the unsetArgs.push("*")
call so no argument is sent for '-H'; when unsetting link/forward ('L') push the
actual channel name used when setting '+L' (i.e., the same link target variable
used elsewhere in this function) into unsetArgs instead of "*", keeping
unsetModes updated via unsetModes += "L" as before.
---
Duplicate comments:
In `@src/components/ui/FloodSettingsModal.tsx`:
- Around line 281-286: The close button in FloodSettingsModal is icon-only
(renders <FaTimes />) and needs an accessible label; update the button element
(the one with onClick={onClose} and className="text-discord-text-muted
hover:text-white") to include an appropriate aria-label such as
aria-label="Close" or aria-label="Close modal" so screen readers can announce
its purpose (you can also add type="button" if missing for clarity).
- Around line 116-145: The parseFloodRule function's regex character class
includes an invalid "e"; update the regex in parseFloodRule to use the correct
set that matches FloodRule["type"] by replacing ([cjkemntr]) with ([cjkmntr]) so
parsed types align with the FloodRule union, keep the rest of the parsing logic
and the type assertion (type as FloodRule["type"]) unchanged, and consider
updating the inline comment to reflect the exact allowed types.
---
Nitpick comments:
In `@src/components/ui/ChannelSettingsModal.tsx`:
- Around line 990-995: The duplicate useEffect calling loadCurrentChannelModes
should be consolidated or narrowed: either merge this effect with the earlier
effect around lines 978-988 so both opening and subsequent MODE updates use the
same logic, or change this effect’s dependency from broad channel to specific
fields like channel.modes and channel.modeArgs (while keeping isOpen) so it only
runs for MODE-sync updates; update references to loadCurrentChannelModes,
isOpen, channel, and any baseline-reset logic (e.g., resetBaseline or the other
effect) accordingly to avoid redundant calls.
- Around line 27-32: The local FloodRule type here conflicts with the FloodRule
used by FloodSettingsModal (missing id) causing the callback signature mismatch;
export the FloodRule type (including id) from FloodSettingsModal.tsx or move it
to a shared types file, then import that shared FloodRule into
ChannelSettingsModal.tsx and update all references (including the callback used
around the FloodSettingsModal invocation) to use the imported/shared FloodRule
so both modals share the exact same definition.
In `@src/components/ui/FloodSettingsModal.tsx`:
- Around line 431-452: The option labels in the FloodSettingsModal select
currently duplicate the type name and call getTypeDescription (e.g. "CTCP
({getTypeDescription("c")})") which renders as "CTCP (CTCP)"; update the option
inner text for each value ("c","j","k","m","n","t","r") to avoid
duplication—either use only getTypeDescription("x") as the label or format as
`label (description)` but ensure getTypeDescription returns a different
description if you keep both; change the option contents in the select block
that renders CTCP/Join/Knock/Messages/Nickchange/Text/Repeat to use the chosen
non-duplicating label approach and reference getTypeDescription and the option
value attributes when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 465805b1-e61a-4a9d-8870-61ae8adae9a2
📒 Files selected for processing (4)
src/components/message/CollapsibleMessage.tsxsrc/components/ui/ChannelSettingsModal.tsxsrc/components/ui/FloodSettingsModal.tsxsrc/protocol/mode.ts
Comment on lines
+311
to
+317
| // Determine if this mode should have a parameter | ||
| let hasParam = false; | ||
| if (groupA.includes(mode) || groupB.includes(mode)) { | ||
| hasParam = true; | ||
| } else if (groupC.includes(mode)) { | ||
| hasParam = currentAction === "+"; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing fallback logic when CHANMODES is absent.
Unlike src/protocol/mode.ts's modeRequiresArg helper (lines 257-260), this inline implementation lacks fallback semantics for servers without CHANMODES. When chanmodes is undefined/empty, all groups are empty strings, causing modes like k and l to be incorrectly treated as having no parameters.
Consider reusing the centralized helper from mode.ts or duplicating its fallback logic:
🐛 Proposed fix
// Determine if this mode should have a parameter
let hasParam = false;
if (groupA.includes(mode) || groupB.includes(mode)) {
hasParam = true;
} else if (groupC.includes(mode)) {
hasParam = currentAction === "+";
+ } else if (groupD.includes(mode)) {
+ hasParam = false;
+ } else {
+ // Fallback when CHANMODES is absent
+ if ("ovhqa".includes(mode) || "beI".includes(mode)) {
+ hasParam = true;
+ } else if (mode === "k" || mode === "l") {
+ hasParam = currentAction === "+";
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/ChannelSettingsModal.tsx` around lines 311 - 317, The
inline hasParam determination in ChannelSettingsModal.tsx incorrectly assumes
CHANMODES groups are present; update it to either call the centralized
modeRequiresArg helper from src/protocol/mode.ts or replicate its fallback
behavior (treat 'bklimnpst' as default groups when chanmodes is undefined/empty)
so modes like 'k' and 'l' correctly require parameters; locate the hasParam
logic (the block checking groupA/groupB/groupC and currentAction) and replace it
with a call to modeRequiresArg(mode, chanmodes, currentAction) or add the same
fallback semantics before evaluating group membership.
Comment on lines
+801
to
+804
| ) { | ||
| unsetModes += "H"; | ||
| unsetArgs.push("*"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
UnrealIRCd channel mode H L parameter requirements when unsetting
💡 Result:
In UnrealIRCd, whether a channel mode needs a parameter when unsetting is indicated by the server’s 005 CHANMODES token:
- Modes in group B require a parameter to be provided on both set and unset.
- Modes in group C require a parameter only when setting (not when unsetting). [1]
For UnrealIRCd 6.x, CHANMODES is commonly advertised like this (example from an UnrealIRCd-6.1.9.1 server):
CHANMODES=beI,fkL,lFH,... which places:
Lin group B (because it’s infkL)Hin group C (because it’s inlFH) [2]
Practical outcome
+H(history): parameter is required when setting (e.g.MODE#chan+H 15:1440), but no parameter is required when unsetting (MODE#chan-H). [3][2]+L(link/forward): parameter is required when setting (e.g.MODE#chan+L#otherchan), and **a parameter is also required when unsetting**, so use: `MODE `#chan` -L `#otherchan(the channel parameter). [4][2]
Sources:
[1] UnrealIRCd 005/CHANMODES grouping description (A/B/C/D) in 005.txt (UnrealIRCd docs) (git.acid.vegas)
[2] Example UnrealIRCd 6.1.9.1 005 line showing CHANMODES=beI,fkL,lFH,... (joesandbox.com)
[3] UnrealIRCd channel history docs showing +H set syntax with a parameter (unrealircd.org)
[4] UnrealIRCd channel modes list describing +L with a channel parameter (unrealircd.org)
Citations:
- 1: https://git.acid.vegas/unrealircd/file/unrealircd/doc/technical/005.txt.html
- 2: https://www.joesandbox.com/analysis/1625017/0/pdf?download=1&utm_source=openai
- 3: https://www.unrealircd.org/docs/Channel_history
- 4: https://www.unrealircd.org/docs/Channel_modes
Fix -H and -L argument handling: +H requires no unset argument; +L requires the actual channel name.
The code incorrectly sends "*" when unsetting both modes:
+H(history) is CHANMODES group C (parameter on set only): unsetting should NOT include any argument. Remove theunsetArgs.push("*")call.+L(link/forward) is CHANMODES group B (parameter on both set and unset): unsetting requires the actual channel name, not"*". Update to push the channel being unlinked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/ChannelSettingsModal.tsx` around lines 801 - 804, In
ChannelSettingsModal update the unset argument handling for modes: when
unsetting history ('H') remove the unsetArgs.push("*") call so no argument is
sent for '-H'; when unsetting link/forward ('L') push the actual channel name
used when setting '+L' (i.e., the same link target variable used elsewhere in
this function) into unsetArgs instead of "*", keeping unsetModes updated via
unsetModes += "L" as before.
Comment on lines
+1998
to
+2003
| <button | ||
| onClick={onClose} | ||
| className="text-discord-text-muted hover:text-white" | ||
| > | ||
| <FaTimes /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add aria-label to desktop close button for accessibility.
The desktop modal's close button lacks an accessible label, unlike the mobile version.
♿ Proposed fix
<button
onClick={onClose}
+ aria-label="Close"
className="text-discord-text-muted hover:text-white"
>📝 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
| <button | |
| onClick={onClose} | |
| className="text-discord-text-muted hover:text-white" | |
| > | |
| <FaTimes /> | |
| </button> | |
| <button | |
| onClick={onClose} | |
| aria-label="Close" | |
| className="text-discord-text-muted hover:text-white" | |
| > | |
| <FaTimes /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/ChannelSettingsModal.tsx` around lines 1998 - 2003, The
desktop close button in ChannelSettingsModal (the <button> with
onClick={onClose} rendering <FaTimes />) has no accessible label; add an
appropriate aria-label (e.g., "Close channel settings" or "Close") to that
button so screen readers can announce its purpose, matching the mobile close
button behavior.
matheusfillipe
approved these changes
Mar 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
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.
This PR significantly enhances the Channel Settings modal with the following improvements:
🎨 Visual & Theme Improvements
🔧 Functional Enhancements
�� Bug Fixes
📱 User Experience
The modal is now much more functional, visually appealing, and aligned with the application's theme.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Accessibility
Style