Bug fixes and performance enhancements#73
Conversation
ValwareIRC
requested review from
Copilot and
matheusfillipe
and removed request for
Copilot
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR implements bug fixes and performance enhancements for the IRC client, primarily focusing on console log cleanup, SASL authentication improvements, and adding new features like link previews and external link warnings.
- Extensive console log removal across the entire codebase to reduce debugging noise in production
- Fixed SASL authentication to only enable when both account name and password are provided
- Added link preview functionality and external link warning modals for enhanced security
- Improved textarea behavior and message sending logic with better multiline handling
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/index.ts | Added link preview fields to Message interface |
| src/store/index.ts | Removed debug console logs throughout event handlers and improved SASL credential validation |
| src/lib/ircClient.ts | Fixed SASL enablement logic and removed extensive debug logging |
| src/lib/ircUtils.tsx | Cleaned up console logs from parseNamesResponse function |
| src/lib/ignoreUtils.ts | Removed warning console log from invalid ignore pattern handling |
| src/hooks/useKeyboardResize.ts | Removed console warning for platform detection failure |
| src/components/ui/UserSettings.tsx | Removed verbose debugging console logs from metadata handling |
| src/components/ui/LinkWrapper.tsx | Added external link warning modal integration |
| src/components/ui/ExternalLinkWarningModal.tsx | New modal component for external link security warnings |
| src/components/message/index.ts | Added LinkPreview component export |
| src/components/message/MessageItem.tsx | Integrated link preview rendering functionality |
| src/components/message/LinkPreview.tsx | New component for rendering link previews with security warnings |
| src/components/layout/ChatArea.tsx | Performance improvements with memoization and enhanced message sending logic |
| src/components/layout/ChannelList.tsx | Performance optimization with selective state subscriptions and fixed Enter key behavior |
| src/App.tsx | Removed console logs from environment initialization |
| remove-console-logs.mjs | New utility script for automated console log removal |
| INSTALL.md | Enhanced installation documentation with platform-specific guidance |
| BUILD.md | Added distribution packaging instructions and compatibility notes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Empty/null values from server mean "not set", so keep our local value | ||
| if (value !== null && value !== undefined && value !== "") { | ||
| savedMetadata[serverId][resolvedTarget][key] = { value, visibility }; | ||
| saveMetadataToLocalStorage(savedMetadata); |
There was a problem hiding this comment.
The comment on lines 3532-3533 contains a logic inconsistency. The comment says 'keep our local value' but there's no else clause to preserve local values when server value is empty.
| saveMetadataToLocalStorage(savedMetadata); | |
| saveMetadataToLocalStorage(savedMetadata); | |
| } else { | |
| // Server value is empty/null/undefined; keep our local value as per comment above | |
| // No action needed, local value is preserved |
| onCancel={handleCancelOpen} | ||
| /> | ||
| <div | ||
| className={`mt-2 rounded-lg overflow-hidden border border-${theme}-dark-400 bg-${theme}-dark-200 max-w-xs ${firstUrl ? `cursor-pointer hover:bg-${theme}-dark-300 transition-colors` : ""}`} |
There was a problem hiding this comment.
Template literal CSS class construction can break with CSS purging tools. Consider using conditional class concatenation or CSS-in-JS for dynamic theming.
| if (inputRef.current) { | ||
| inputRef.current.value = messageText.trim(); | ||
| } |
There was a problem hiding this comment.
Directly manipulating DOM element value bypasses React's controlled component pattern. Consider updating the state instead of directly setting the input value.
| if (inputRef.current) { | |
| inputRef.current.value = messageText.trim(); | |
| } | |
| setMessageText(messageText.trim()); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/store/index.ts (1)
2986-3056: Fix link-preview reply tag key (still using+reply).This branch still looks up
mtags["+reply"], so it never runs with the TAGMSG payloads our server emits (+draft/reply). Please restore the draft key (optionally falling back to+replyfor safety) as previously requested.- mtags["obsidianirc/link-preview-meta"]) && - mtags["+reply"] + mtags["obsidianirc/link-preview-meta"]) && + (mtags["+draft/reply"] ?? mtags["+reply"]) ) { - const replyMessageId = mtags["+reply"]; + const replyMessageId = + mtags["+draft/reply"] ?? mtags["+reply"];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/layout/ChannelList.tsx(4 hunks)src/store/index.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/layout/ChannelList.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/store/index.ts (1)
src/types/index.ts (2)
Channel(45-58)PrivateChat(60-67)
Major Features: - Link previews with TAGMSG support and external link warnings - METADATA support (IRCv3 draft/metadata-2) with persistence - User avatars, display names, and profile metadata - SASL authentication improvements - Multiline message support with draft/multiline - Message reactions and reply threading - Channel rename support - Nickname retry logic Performance: - Optimize ChatArea re-renders with useMemo for message grouping - Fix input lag with medium/large message history - Reactive metadata updates in User Status widget UI/UX: - JetBrains Mono font throughout - Improved mobile keyboard handling - External link warning modal - Better metadata display in member lists Bug Fixes: - Fix metadata flickering on connect - Prevent empty server responses from clearing local metadata - Fix SASL/CAP negotiation flow - Fix newline handling in message input - Fix metadata reactivity in User Status widget Developer Experience: - Remove debug console logs (kept console.error) - Improve installation documentation - Add ZorinOS/glibc troubleshooting - Reduce bundle size by 6KB
ValwareIRC
force-pushed
the
enhancements
branch
from
884c1a2 to
c2091c8
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds mobile keyboard resize handling, link previews with confirmation modal, multiline message batching, expanded user/settings UI, enhanced store and IRC client capabilities, enriched types, and font/theme updates. Documentation updated for build/install; some docs removed. Multiple components refactored for memoization, event grouping, and cleaner logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ChatArea
participant IRCClient
participant Server
Note over ChatArea: User types a long message
User->>ChatArea: Press Enter
ChatArea->>ChatArea: splitLongMessage(text, target)
ChatArea->>IRCClient: Send (BATCH start) if multiline supported
loop For each part
ChatArea->>IRCClient: PRIVMSG part (with concat flag if applicable)
IRCClient->>Server: Send part
end
alt Multiline supported
ChatArea->>IRCClient: BATCH end
else
Note over ChatArea,IRCClient: Sends as multiple PRIVMSG without BATCH
end
sequenceDiagram
autonumber
participant MessageItem
participant LinkPreview
participant Modal as ExternalLinkWarningModal
participant Browser
MessageItem->>LinkPreview: Render with URL metadata
LinkPreview->>Modal: Open (isOpen = true) on click/Enter
alt User confirms
Modal->>LinkPreview: onConfirm
LinkPreview->>Browser: window.open(url, _blank, noopener,noreferrer)
else User cancels
Modal->>LinkPreview: onCancel (close)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewersPoem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
INSTALL.md (1)
123-124: Docker image name looks misspelled; command will failTypo in image repo: obdisianirc → obsidianirc.
Apply this fix:
-docker run -p 8080:80 mattfly/obdisianirc:latest +docker run -p 8080:80 mattfly/obsidianirc:latestsrc/store/index.ts (1)
2631-2641: Duplicate CAP ACK handlers.Two separate listeners for "CAP ACK"/"CAP_ACKNOWLEDGED" update capabilities and ack; risk of double-processing and inconsistent state.
Unify into a single handler (keep one place to update capabilities and call capAck) or de-duplicate logic to avoid running twice.
Also applies to: 3745-3760
♻️ Duplicate comments (5)
src/components/message/LinkPreview.tsx (1)
5-11: Accept explicit preview URL to avoid incorrect navigation.The component extracts URLs from
messageContentusing a regex, but messages can contain multiple links or use shortened/normalized URLs. The preview card should open the specific URL that the preview represents, not just the first HTTP link found in the text.Add a
urlprop toLinkPreviewPropsand prefer it over the regex extraction:interface LinkPreviewProps { title?: string; snippet?: string; imageUrl?: string; + url?: string; theme: string; messageContent: string; } export const LinkPreview: React.FC<LinkPreviewProps> = ({ title, snippet, imageUrl, + url, theme, messageContent, }) => { const [showWarningModal, setShowWarningModal] = useState(false); // Don't render if there's no content to show if (!title && !snippet && !imageUrl) { return null; } // Extract the first URL from the message content const urlRegex = /\b(?:https?):\/\/[^\s<>"']+/i; const match = messageContent.match(urlRegex); - const firstUrl = match ? match[0] : undefined; + const fallbackUrl = match ? match[0] : undefined; + const previewUrl = url || fallbackUrl; const handleClick = () => { - if (firstUrl) { + if (previewUrl) { setShowWarningModal(true); } }; const handleConfirmOpen = () => { - if (firstUrl) { - window.open(firstUrl, "_blank", "noopener,noreferrer"); + if (previewUrl) { + window.open(previewUrl, "_blank", "noopener,noreferrer"); } setShowWarningModal(false); };Then update all references to
firstUrlwithpreviewUrlin the return statement (lines 51-98), and ensure the caller passesmessage.linkPreviewUrlas theurlprop.Also applies to: 27-56
src/components/layout/ChatArea.tsx (1)
693-696: Avoid direct DOM mutations on controlled input.Set state instead of mutating textarea.value.
Use setMessageText(messageText.trim()). (Based on past review)
src/store/index.ts (3)
2695-2706: Don’t require SASL account name to defer CAP END.This breaks nickname-fallback SASL flows. Guard on saslEnabled + saslPassword and allow either accountName or nickname, as previously suggested.
- if ( - savedServer?.saslEnabled && - savedServer?.saslAccountName && - savedServer?.saslPassword - ) { - preventCapEnd = true; - } + if (savedServer?.saslEnabled && savedServer?.saslPassword) { + const hasAuthUser = + savedServer.saslAccountName?.trim().length || + savedServer.nickname?.trim().length; + if (hasAuthUser) preventCapEnd = true; + }
2986-3056: Fix link preview reply tag key.Server sends +draft/reply; current code checks +reply, so previews won’t attach.
- mtags["+reply"] + (mtags["+draft/reply"] ?? mtags["+reply"]) @@ - const replyMessageId = mtags["+reply"]; + const replyMessageId = mtags["+draft/reply"] ?? mtags["+reply"];
1453-1454: Remove empty else branch.No-op else reduces clarity.
- } else { - } + }
🧹 Nitpick comments (27)
INSTALL.md (2)
78-91: Use headings instead of bold for section titles (markdownlint MD036)Convert the “Solution X” lines to proper headings.
-**Solution 1: Use the AppImage (Recommended)** +### Solution 1: Use the AppImage (Recommended) -**Solution 2: Build from Source** +### Solution 2: Build from Source -**Solution 3: Run with Custom glibc (Not Recommended)** +### Solution 3: Run with Custom glibc (Not Recommended)Based on static analysis hints
10-16: AppImage artifact naming is inconsistent; may confuse usersYou alternate between
ObsidianIRC_*_amd64.AppImageandObsidianIRC-*.AppImage. Pick one pattern throughout to reduce ambiguity in commands/snippets.src/components/message/MessageAvatar.tsx (1)
9-13: theme prop is unused; background is hard-codedYou pass theme but render
bg-black. Either use theme to derive the background or drop the prop.Example options:
- Use theme class:
className={\w-8 h-8 rounded-full ${theme} ...`}`- Or remove
themefrom props and call sites.Also applies to: 46-47
src/components/layout/ChannelList.tsx (1)
189-194: Avoid injecting "false" into className stringsUsing
${cond && "classes"}inside template strings yields a literal "false" class when cond is false. Prefer ternary or a helper like clsx.Example change (one instance shown):
- className={`ml-auto ${!isNarrowView && "opacity-0 group-hover:opacity-100"} cursor-pointer`} + className={`ml-auto ${!isNarrowView ? "opacity-0 group-hover:opacity-100" : ""} cursor-pointer`}Consider adopting
clsxfor cleaner conditional classes.Also applies to: 306-307, 316-318, 330-331
src/lib/ircClient.ts (2)
445-446: Use slice instead of deprecated substr()Avoid deprecated String.prototype.substr.
-const batchId = `ml_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`; +const batchId = `ml_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`;
367-371: Minor cleanup: no-op branches and duplicates
- Empty debug branch in sendRaw (Lines 367-371).
- Empty else after SASL creds set (Lines 282-289).
message-tagslisted twice inourCaps(Lines 1482-1503).Remove no-ops and duplicate to reduce noise.
Also applies to: 282-289, 1482-1503
src/App.tsx (1)
41-45: Consider retaining minimal error logging.While removing console noise is generally good for production, the silent fallback when
hostorportare missing might make debugging difficult. Consider using a logging utility that can be configured per environment, or at minimum retain an error-level log for the missing configuration case.Example approach:
if (!host || !port) { + if (import.meta.env.DEV) { + console.error('Default IRC server configuration incomplete: missing host or port'); + } return; }src/index.css (1)
36-36: Consider usingmax()to prevent negative height.The
calc(100vh - var(--keyboard-height, 0px))could theoretically result in a negative height if--keyboard-heightexceeds100vh(e.g., due to a bug in the resize detection logic). Consider usingmax()to ensure a minimum height:#root { height: 100vh; - height: calc(100vh - var(--keyboard-height, 0px)); + height: max(0px, calc(100vh - var(--keyboard-height, 0px))); transition: height 0.2s ease-in-out; position: relative; }Otherwise, the mobile keyboard handling CSS looks correct.
Also applies to: 66-78
tests/lib/nicknameRetry.test.ts (1)
11-60: Eliminate duplicate mock state setup.The mock state setup is duplicated across both test cases. Extract it into a
beforeEachhook or a helper function to improve maintainability and reduce code duplication.Apply this refactor to use a
beforeEachhook:describe("Nickname retry functionality", () => { + let changeNickSpy: ReturnType<typeof vi.spyOn>; + let mockState: Partial<AppState>; + + beforeEach(() => { + // Mock the changeNick method + changeNickSpy = vi.spyOn(ircClient, "changeNick"); + + // Mock the store state with minimal required properties + mockState = { + servers: [ + { + id: "test-server", + name: "Test Server", + host: "test.com", + port: 6667, + isConnected: true, + channels: [ + { + id: "test-channel", + name: "#test", + isPrivate: false, + serverId: "test-server", + unreadCount: 0, + isMentioned: false, + messages: [], + users: [], + }, + ], + privateChats: [], + users: [], + }, + ], + ui: { + selectedServerId: "test-server", + selectedChannelId: "test-channel", + selectedPrivateChatId: null, + isAddServerModalOpen: false, + isSettingsModalOpen: false, + isUserProfileModalOpen: false, + isDarkMode: false, + isMobileMenuOpen: false, + isMemberListVisible: true, + isChannelListVisible: true, + isChannelListModalOpen: false, + isChannelRenameModalOpen: false, + mobileViewActiveColumn: "chatView", + isServerMenuOpen: false, + contextMenu: { + isOpen: false, + x: 0, + y: 0, + type: "server", + itemId: null, + }, + prefillServerDetails: null, + }, + addGlobalNotification: vi.fn(), + }; + + // Mock useStore.getState to return our mock state + vi.spyOn(useStore, "getState").mockReturnValue(mockState as AppState); + vi.spyOn(useStore, "setState").mockImplementation(() => {}); + }); + + afterEach(() => { + changeNickSpy.mockRestore(); + }); + it("should retry with underscore when receiving 433 error", () => { - // Mock the changeNick method - const changeNickSpy = vi.spyOn(ircClient, "changeNick"); - - // Mock the store state with minimal required properties - const mockState: Partial<AppState> = { - ... - }; - - // Mock useStore.getState to return our mock state - vi.spyOn(useStore, "getState").mockReturnValue(mockState as AppState); - vi.spyOn(useStore, "setState").mockImplementation(() => {}); - // Simulate a 433 error event ... - - // Clean up - changeNickSpy.mockRestore(); }); it("should not retry for other error codes", () => { - // Mock the changeNick method - const changeNickSpy = vi.spyOn(ircClient, "changeNick"); - - // Mock the store state with addGlobalNotification method - const mockState: Partial<AppState> = { - ... - }; - - // Mock useStore methods - vi.spyOn(useStore, "getState").mockReturnValue(mockState as AppState); - vi.spyOn(useStore, "setState").mockImplementation(() => {}); - // Simulate a 432 error event ... - - // Clean up - changeNickSpy.mockRestore(); }); });Also applies to: 93-142
src/components/ui/UserContextMenu.tsx (1)
92-102: Consider passing user/host information if available.The ignore check only uses the
username(nick) without user or host information. If theuserobject contains additional hostmask details, passing them would enable more precise ignore pattern matching.If hostmask details are available in the
userobject, consider this enhancement:const isIgnored = isUserIgnored( username, - undefined, - undefined, + user?.ident || undefined, + user?.host || undefined, globalSettings.ignoreList, );src/components/ui/LoadingSpinner.tsx (1)
14-18: MovesizeClassesoutside the component to avoid recreation on every render.The
sizeClassesobject is recreated on every render. Since it's a static mapping, move it outside the component or useuseMemoto optimize performance.Apply this diff to move the constant outside:
+const sizeClasses = { + sm: "w-4 h-4", + md: "w-8 h-8", + lg: "w-12 h-12", +}; + export const LoadingSpinner: React.FC<LoadingSpinnerProps> = ({ size = "md", className = "", text = "Loading...", }) => { - const sizeClasses = { - sm: "w-4 h-4", - md: "w-8 h-8", - lg: "w-12 h-12", - };src/hooks/useKeyboardResize.ts (3)
8-16: Consider extracting mobile detection logic.The mobile detection logic combines userAgent checking and viewport width. Consider extracting this to a reusable utility function for consistency across the codebase.
Example utility:
// utils/deviceDetection.ts export const isMobileDevice = (): boolean => { return ( /Android|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test( navigator.userAgent, ) || window.innerWidth <= 768 ); };
42-42: Document the 150px threshold rationale.The 150px threshold for keyboard visibility detection may need adjustment based on device testing. Consider adding a comment explaining this magic number or making it configurable.
- isKeyboardVisible = heightDifference > 150; // Adjust threshold as needed + // 150px threshold chosen based on typical mobile keyboard heights + // Smaller values may cause false positives from address bar changes + isKeyboardVisible = heightDifference > 150;
58-63: Clarify the need for delayed resize event.Two resize events are dispatched: one immediately and another after 50ms. Consider adding a comment explaining why the delayed dispatch is necessary for DOM updates.
// Trigger a resize event to force layout recalculation window.dispatchEvent(new Event("resize")); - // Small delay to ensure DOM updates are processed + // Delayed dispatch ensures CSS custom property changes have been applied + // and layout recalculations are complete before final resize setTimeout(() => { window.dispatchEvent(new Event("resize")); }, 50);src/components/ui/ExternalLinkWarningModal.tsx (1)
26-30: Add support for Enter key to confirm.The modal only handles the Escape key for cancellation. Consider adding Enter key support to confirm the action, improving keyboard navigation.
const handleKeyDown = (e: React.KeyboardEvent) => { if (e.key === "Escape") { onCancel(); } + if (e.key === "Enter") { + onConfirm(); + } };test_multiline.md (3)
14-24: Add language specifiers to fenced code blocks.The static analysis tool flags missing language specifiers. Add
ircortextto the code blocks for better syntax highlighting and linting compliance.Based on learnings from static analysis hints.
Apply this diff:
### Case 1: Multi-line message (has newlines) -``` +```irc Input: "Hello\nWorld\nHow are you?" Output: BATCH +abc123 draft/multiline #channel
27-36: Add language specifiers to fenced code blocks.Same as above - add language specifiers for consistency.
### Case 2: Single very long line (over 400 chars) -``` +```irc Input: "This is a very long message..."
39-50: Add language specifiers to fenced code blocks.Same as above - add language specifiers for consistency.
### Case 3: Multi-line with some long lines -``` +```irc Input: "Short line\n..."src/components/message/EventMessage.tsx (1)
44-46: Extract username parsing logic.The username extraction pattern
message.userId.split("-")[0]is repeated twice. Consider extracting it to a variable at the top of the component for DRY.+ const username = message.userId.split("-")[0]; + const handleAvatarClick = (e: React.MouseEvent) => { - const username = message.userId.split("-")[0]; onUsernameContextMenu(e, username, message.serverId, e.currentTarget); }; // ... later in the component - const username = message.userId.split("-")[0]; const displayName =Also applies to: 56-56
src/components/message/CollapsedEventMessage.tsx (1)
72-74: Document the hover z-index behavior.The dynamic z-index manipulation (
style={{ zIndex: 10 - index }}andhover:z-20) is clever for making hovered avatars appear on top, but it may cause unexpected stacking issues with other UI elements. Consider adding a comment explaining this approach.<div key={username} className="w-3 h-3 bg-black border border-discord-dark-200 rounded-full flex items-center justify-center text-white text-xs cursor-pointer hover:opacity-80 overflow-hidden transform transition-all duration-200 hover:w-8 hover:h-8 hover:text-base relative hover:z-20" + // Decreasing z-index ensures rightmost avatars stack on top in default state + // hover:z-20 brings hovered avatar to the front style={{ zIndex: 10 - index }}tests/lib/notificationSounds.test.ts (1)
28-32: Clarify mock Audio volume initialization.The
mockAudio.volumeis initialized to0.5, but line 103 expects it to be0.3after playing a custom sound. This works because the Audio constructor creates a new instance and the volume is set by the code under test. Consider initializing it to a different value or adding a comment to clarify that it will be overwritten by the implementation.// Mock HTML Audio API const mockAudio = { play: vi.fn(() => Promise.resolve()), - volume: 0.5, + volume: 1.0, // Will be set to 0.3 by playNotificationSound };src/lib/eventGrouping.ts (2)
6-7: Narrow eventType typing and make collapsible types explicitUse a precise union to improve type safety and avoid accidental values.
export interface EventGroup { type: "message" | "eventGroup"; messages: Message[]; - eventType?: string; + eventType?: "join" | "part" | "quit"; usernames?: string[]; timestamp: Date; } @@ -export function groupConsecutiveEvents(messages: Message[]): EventGroup[] { +export function groupConsecutiveEvents(messages: Message[]): EventGroup[] { const result: EventGroup[] = []; - const collapsibleEventTypes = ["join", "part", "quit"]; + const collapsibleEventTypes = ["join", "part", "quit"] as const; + type CollapsibleEventType = (typeof collapsibleEventTypes)[number]; @@ - const eventType = currentMessage.type; + const eventType = currentMessage.type as CollapsibleEventType;Also applies to: 17-18, 36-37
58-59: Centralize username extractionusernames are derived via userId.split("-")[0]. This convention appears in multiple files; extract a small helper (e.g., getNickFromUserId) to avoid drift and future breakage.
src/lib/notificationSounds.ts (1)
81-89: Comment vs. logic mismatch for NOTICEComment mentions “messages (PRIVMSG) and notices (NOTICE)” but code blocks non-"message" types. Either include notice or update the comment.
Option A (include notices):
- const isUserMessage = message.type === "message"; + const isUserMessage = message.type === "message" || message.type === "notice";Option B: adjust the comment to remove NOTICE.
src/lib/ignoreUtils.ts (1)
25-35: Mitigate ReDoS risk in dynamic regex constructionAdd simple guards to bound worst-case patterns; consider a safer conversion if feasible.
const regexPattern = normalizedPattern .replace(/[.+^${}()|[\]\\]/g, "\\$&") // Escape regex special chars except * and ? .replace(/\*/g, ".*") // Convert * to .* .replace(/\?/g, "."); // Convert ? to . try { - const regex = new RegExp(`^${regexPattern}$`); + // Basic guards to reduce catastrophic backtracking risks + if (normalizedPattern.length > 1024) return false; + const starCount = (normalizedPattern.match(/\*/g) || []).length; + if (starCount > 50) return false; + const regex = new RegExp(`^${regexPattern}$`); return regex.test(normalizedHostmask); } catch (error) { return false; }Note: For stronger guarantees, consider a RE2-based matcher or a dedicated wildcard-to-regex utility. Based on static analysis hints.
ANDROID_KEYBOARD_FIX.md (1)
25-29: Persisting native changes outside generatedgen/pathsFiles under src-tauri/gen are typically regenerated. Move custom MainActivity changes to the non-generated Android app sources to avoid losing them during upgrades.
src/components/ui/UserSettings.tsx (1)
77-77: Replace onKeyPress with onKeyDown (deprecated).React deprecates onKeyPress. Use onKeyDown and check e.key === "Enter".
Example:
- onKeyPress={handleKeyPress} + onKeyDown={(e) => e.key === "Enter" && handleAddMention()}and similarly in IgnoreList.
Also applies to: 168-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonsrc-tauri/gen/android/app/src/main/AndroidManifest.xmlis excluded by!**/gen/**src-tauri/gen/android/app/src/main/java/com/obsidianirc/dev/MainActivity.ktis excluded by!**/gen/**
📒 Files selected for processing (52)
.github/workflows/publish.yaml(1 hunks)ANDROID_KEYBOARD_FIX.md(1 hunks)BUILD.md(1 hunks)INSTALL.md(1 hunks)index.html(1 hunks)package.json(1 hunks)remove-console-logs.mjs(1 hunks)src/App.tsx(2 hunks)src/components/layout/AppLayout.tsx(2 hunks)src/components/layout/ChannelList.tsx(6 hunks)src/components/layout/ChatArea.tsx(17 hunks)src/components/layout/MemberList.tsx(4 hunks)src/components/message/ActionMessage.tsx(1 hunks)src/components/message/CollapsedEventMessage.tsx(1 hunks)src/components/message/EventMessage.tsx(1 hunks)src/components/message/LinkPreview.tsx(1 hunks)src/components/message/MessageAvatar.tsx(3 hunks)src/components/message/MessageHeader.tsx(3 hunks)src/components/message/MessageItem.tsx(8 hunks)src/components/message/index.ts(1 hunks)src/components/ui/AddPrivateChatModal.tsx(3 hunks)src/components/ui/AddServerModal.tsx(2 hunks)src/components/ui/AutocompleteDropdown.tsx(1 hunks)src/components/ui/EmojiAutocompleteDropdown.tsx(1 hunks)src/components/ui/ExternalLinkWarningModal.tsx(1 hunks)src/components/ui/LinkWrapper.tsx(4 hunks)src/components/ui/LoadingSpinner.tsx(1 hunks)src/components/ui/UserContextMenu.tsx(3 hunks)src/components/ui/UserSettings.tsx(2 hunks)src/hooks/useKeyboardResize.ts(1 hunks)src/index.css(2 hunks)src/lib/eventGrouping.ts(1 hunks)src/lib/ignoreUtils.ts(1 hunks)src/lib/ircClient.ts(35 hunks)src/lib/ircUtils.tsx(2 hunks)src/lib/notificationSounds.ts(1 hunks)src/store/index.ts(51 hunks)src/types/index.ts(6 hunks)tailwind.config.js(1 hunks)test_multiline.md(1 hunks)tests/App.test.tsx(7 hunks)tests/components/ChatArea.test.tsx(2 hunks)tests/components/MetadataDisplay.test.tsx(4 hunks)tests/components/UserSettings.test.tsx(1 hunks)tests/lib/defaultIgnore.test.ts(1 hunks)tests/lib/ignoreUtils.test.ts(1 hunks)tests/lib/ircClient.test.ts(7 hunks)tests/lib/nicknameRetry.test.ts(1 hunks)tests/lib/notificationSounds.test.ts(1 hunks)tests/setup.ts(1 hunks)tsconfig.json(1 hunks)vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- remove-console-logs.mjs
- src/components/message/index.ts
🧰 Additional context used
🧬 Code graph analysis (18)
tests/lib/ignoreUtils.test.ts (1)
src/lib/ignoreUtils.ts (4)
matchesIgnorePattern(14-36)isUserIgnored(45-82)isValidIgnorePattern(87-114)createIgnorePattern(119-125)
tests/lib/nicknameRetry.test.ts (2)
src/lib/ircClient.ts (1)
ircClient(1693-1693)src/store/index.ts (1)
AppState(297-461)
tests/lib/notificationSounds.test.ts (1)
src/lib/notificationSounds.ts (2)
playNotificationSound(6-59)shouldPlayNotificationSound(62-110)
src/components/message/CollapsedEventMessage.tsx (3)
src/lib/eventGrouping.ts (3)
EventGroup(3-9)getEventGroupSummary(84-133)getEventGroupTooltip(138-157)src/types/index.ts (1)
User(1-13)src/lib/ircClient.ts (1)
ircClient(1693-1693)
src/components/ui/AddPrivateChatModal.tsx (1)
src/lib/ircClient.ts (1)
ircClient(1693-1693)
src/components/ui/UserSettings.tsx (2)
src/lib/ignoreUtils.ts (1)
isValidIgnorePattern(87-114)src/lib/ircClient.ts (4)
ircClient(1693-1693)changeNick(545-547)metadataSet(559-576)setName(541-543)
src/App.tsx (1)
src/hooks/useKeyboardResize.ts (1)
useKeyboardResize(5-133)
src/components/message/LinkPreview.tsx (1)
src/components/message/index.ts (1)
LinkPreview(6-6)
src/components/layout/ChannelList.tsx (2)
src/lib/ircClient.ts (1)
ircClient(1693-1693)src/hooks/useMediaQuery.ts (1)
useMediaQuery(3-15)
src/components/ui/UserContextMenu.tsx (1)
src/lib/ignoreUtils.ts (2)
isUserIgnored(45-82)createIgnorePattern(119-125)
src/components/message/EventMessage.tsx (2)
src/types/index.ts (2)
MessageType(119-119)User(1-13)src/lib/ircClient.ts (1)
ircClient(1693-1693)
src/components/message/MessageItem.tsx (7)
src/lib/ircClient.ts (1)
ircClient(1693-1693)src/lib/ircUtils.tsx (1)
isUserVerified(50-60)src/components/message/DateSeparator.tsx (1)
DateSeparator(8-27)src/components/message/EventMessage.tsx (1)
EventMessage(19-121)src/components/ui/LinkWrapper.tsx (1)
EnhancedLinkWrapper(16-124)src/components/message/LinkPreview.tsx (1)
LinkPreview(13-101)src/components/message/MessageReactions.tsx (1)
MessageReactions(16-82)
src/lib/eventGrouping.ts (1)
src/types/index.ts (1)
Message(74-116)
src/store/index.ts (3)
src/lib/ignoreUtils.ts (1)
isUserIgnored(45-82)src/lib/notificationSounds.ts (2)
shouldPlayNotificationSound(62-110)playNotificationSound(6-59)src/types/index.ts (4)
Message(74-116)PrivateChat(60-67)Channel(45-58)User(1-13)
src/components/layout/ChatArea.tsx (8)
src/types/index.ts (1)
User(1-13)src/lib/ircClient.ts (1)
ircClient(1693-1693)src/lib/eventGrouping.ts (1)
groupConsecutiveEvents(15-79)src/store/index.ts (1)
serverSupportsMultiline(129-129)src/lib/messageFormatter.ts (2)
formatMessageForIrc(104-120)getPreviewStyles(128-144)src/components/ui/LoadingSpinner.tsx (1)
LoadingSpinner(9-32)src/components/message/CollapsedEventMessage.tsx (1)
CollapsedEventMessage(22-123)src/components/message/MessageItem.tsx (1)
MessageItem(45-283)
src/lib/ircClient.ts (2)
src/types/index.ts (5)
BaseUserActionEvent(186-188)BaseMessageEvent(179-183)BaseIRCEvent(157-159)User(1-13)Server(15-31)src/lib/ircUtils.tsx (2)
parseMessageTags(33-43)parseIsupport(62-76)
tests/lib/defaultIgnore.test.ts (1)
src/lib/ignoreUtils.ts (1)
isUserIgnored(45-82)
tests/App.test.tsx (1)
src/lib/ircClient.ts (1)
ircClient(1693-1693)
🪛 ast-grep (0.39.5)
src/lib/ignoreUtils.ts
[warning] 30-30: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${regexPattern}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 markdownlint-cli2 (0.18.1)
test_multiline.md
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
INSTALL.md
78-78: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
87-87: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
91-91: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (40)
src/components/ui/EmojiAutocompleteDropdown.tsx (1)
17-17: LGTM! Type broadening is safe and well-aligned with multiline support.The type change from
HTMLInputElement | nulltoHTMLInputElement | HTMLTextAreaElement | nullis backward-compatible and safe. The component only callsgetBoundingClientRect()on theinputElement(line 114), which is available on both element types. This change enables the emoji autocomplete to work with textarea elements, supporting multiline input scenarios mentioned in the PR objectives.src/components/ui/AutocompleteDropdown.tsx (1)
12-12: LGTM! Consistent type broadening across autocomplete components.The type change mirrors the one in
EmojiAutocompleteDropdown.tsxand is equally safe. The component only callsgetBoundingClientRect()on theinputElement(line 161), making the broadening to includeHTMLTextAreaElementa safe, backward-compatible enhancement that supports textarea-based inputs.src/components/ui/AddPrivateChatModal.tsx (1)
18-50: LGTM: Per-server currentUser refactoring is well-implemented.The refactoring correctly:
- Removes the global
currentUserfrom store destructuring- Retrieves the current user for the specific server using
ircClient.getCurrentUser(serverId)within the memo- Updates memo dependencies to remove
currentUser(since it's now derived fromserverId)- Maintains null-safe access with optional chaining on line 40
This aligns with the broader per-server state management pattern across the codebase.
tailwind.config.js (1)
87-88: Clarify default font-sans updateUsing JetBrains Mono (monospace) as the default
sansfont applies to all UI text and body copy and duplicates themonostack. If a terminal/technical aesthetic is intended, confirm; otherwise revertsansto the previous stack:- sans: ["JetBrains Mono", "monospace"], + sans: ["Whitney", "Helvetica Neue", "Helvetica", "Arial", "sans-serif"], mono: ["JetBrains Mono", "monospace"],tsconfig.json (1)
15-16: LGTM!The TypeScript configuration correctly integrates test infrastructure by adding Vitest global types and jest-dom matchers, and includes the tests directory in the compilation scope.
Also applies to: 20-21
package.json (1)
4-4: LGTM!The version bump to 0.1.0 appropriately reflects the new features and enhancements introduced in this PR.
vite.config.ts (1)
2-2: LGTM!The type reference directive correctly enables jest-dom types for TypeScript resolution in the Vite configuration file, aligning with the test infrastructure setup.
BUILD.md (1)
59-71: LGTM!The expanded Linux build documentation provides valuable guidance on glibc compatibility and distribution packaging options, improving the build experience for contributors.
tests/setup.ts (1)
5-9: LGTM!The type assertion correctly reflects that
window.matchMediais a function returningMediaQueryList, not aMediaQueryListobject itself. This fixes the previous type mismatch.src/lib/ircUtils.tsx (2)
18-18: LGTM!Removing the debug console.log statement cleans up the codebase.
45-60: LGTM!The
isUserVerifiedfunction correctly implements IRCv3 account-tag verification with case-insensitive comparison. The documentation clearly explains the verification logic.src/components/message/MessageHeader.tsx (1)
13-13: LGTM!The verification badge implementation is clean and well-integrated. The conditional rendering with appropriate default value and styling provides clear visual feedback for authenticated users.
Also applies to: 25-25, 46-53
src/components/layout/MemberList.tsx (3)
62-62: LGTM!The avatar error handling using React state instead of DOM manipulation is a cleaner approach that follows React best practices.
Also applies to: 90-97
103-105: LGTM!The presence indicator provides clear visual feedback for user availability status with appropriate color coding (green for here, yellow for away).
69-70: LGTM!The bot detection logic and tooltip implementation provides helpful information about bot users. The conditional tooltip display (when
botInfohas meaningful content) is a nice UX touch.Also applies to: 129-140
src/lib/ircClient.ts (1)
566-576: metadataSet ignores visibility argumentYou accept
visibility?: stringbut don’t send it. Confirm protocol expectations fordraft/metadata-2and include visibility if required.tests/components/ChatArea.test.tsx (1)
14-14: LGTM! Test fixtures properly updated.The test fixtures correctly reflect the implementation changes: the
getCurrentUsermock supports the ChatArea's new user derivation logic, and the modal state fields align with the expanded UI state interface.Also applies to: 74-75
src/components/message/ActionMessage.tsx (1)
55-55: LGTM! Prop forwarding is correct.The
isAwayprop is properly forwarded toMessageAvatarwith appropriate optional chaining, consistent with the existing pattern for other metadata props.tests/lib/defaultIgnore.test.ts (1)
1-30: LGTM! Well-structured tests.The tests provide good coverage of the default ignore functionality with both positive (HistServ ignored) and negative (regular user not ignored) cases. The test structure and assertions are correct.
src/components/layout/AppLayout.tsx (2)
72-79: LGTM! Mobile layout logic is correct.The conditional width logic properly handles the mobile view: 72px when the server list is active or in wide view, and 0 when in narrow view and not the active column. This ensures proper column visibility on mobile.
172-173: Good improvement: explicit null return.Returning
nullinstead ofundefinedfor non-active columns in mobile view is semantically clearer for React and explicitly signals non-rendering intent.src/App.tsx (1)
77-79: LGTM! Keyboard resize integration is correct.The
useKeyboardResizehook is properly initialized to handle mobile keyboard layout adjustments.src/index.css (1)
80-87: LGTM! iOS Safari adjustments are appropriate.The use of
-webkit-fill-availableis a well-known workaround for iOS Safari's viewport height quirks and will help ensure proper layout on iOS devices.src/components/ui/AddServerModal.tsx (2)
43-47: LGTM! Sensible defaulting logic.Defaulting the server name to the host and the SASL account name to the nickname provides good UX while maintaining proper validation. The use of
trim()ensures whitespace-only values are handled correctly.
49-52: LGTM! Validation and usage are correct.The validation correctly uses
finalServerNameto ensure non-empty values after defaulting, and theconnectcall properly passes the computed final values.Also applies to: 71-71, 77-77
tests/components/MetadataDisplay.test.tsx (1)
14-14: LGTM!The test updates correctly align with the new message model, including:
getCurrentUsermock for per-server current user derivationtimestampasDateobject instead of ISO stringtypeas const literal- New fields:
reactions,replyMessage,mentionedThese changes ensure test coverage for the enhanced message structure introduced in the PR.
Also applies to: 73-79, 85-91, 325-331
tests/lib/ircClient.test.ts (1)
86-99: LGTM!The test updates correctly reflect the new
connectsignature where the display name is explicitly passed as the first argument. All assertions now verify thatserver.namematches the provided display name rather than deriving it from the host.Also applies to: 134-145, 151-166, 174-179, 219-224
src/components/ui/LinkWrapper.tsx (1)
20-49: LGTM!The centralized link handling approach is well-designed:
- Clean separation of IRC vs HTTP/HTTPS link handling
- Proper confirmation flow before opening external links
- State management with
pendingUrlis straightforward- Modal integration follows best practices
The implementation enhances security by always confirming external link navigation.
Also applies to: 113-123
tests/lib/ignoreUtils.test.ts (1)
1-151: LGTM!The test suite provides excellent coverage of the ignore utility functions:
- Comprehensive edge case coverage (wildcards, case sensitivity, partial information)
- Clear test organization by function
- Good validation of both positive and negative scenarios
- Proper handling of invalid inputs
tests/components/UserSettings.test.tsx (1)
1-140: LGTM!The test suite provides good coverage of UserSettings functionality:
- Proper mocking of dependencies (store, ircClient, Audio API)
- Clear test cases for tab navigation
- Correct handling of hosted chat mode flag
- Uses appropriate async testing patterns with
waitFortests/App.test.tsx (4)
33-86: LGTM! Comprehensive state reset ensures test isolation.The expanded state reset in
afterEachis thorough and includes all UI and global state fields, ensuring proper test isolation across tests.
109-119: LGTM! Mock server object matches expected structure.The mock return value for
ircClient.connectnow includes all required server properties, aligning with the expected Server interface.
172-194: LGTM! Proper async handling with waitFor.The addition of
waitForto ensure the modal is open before interacting with form fields and to verify error messages after async operations is appropriate and prevents flaky tests.
204-204: LGTM! Added isOnline property to currentUser.The addition of
isOnline: truealigns with the User interface requirements.src/components/message/CollapsedEventMessage.tsx (1)
22-26: LGTM! Proper component structure and state management.The component correctly uses state for tooltip visibility and failed avatars, and retrieves the server-specific current user for context.
tests/lib/notificationSounds.test.ts (2)
67-125: LGTM! Comprehensive test coverage for playNotificationSound.The test suite covers all major scenarios: disabled sounds, default beep, custom sound, and error handling. The mock setup is thorough and appropriate.
127-265: LGTM! Thorough coverage of shouldPlayNotificationSound logic.The tests validate all decision paths: disabled sounds, own messages, mentions vs non-mentions, highlights enabled/disabled, case-insensitivity, and null user handling. Well done!
src/hooks/useKeyboardResize.ts (1)
1-1: Plugin-os registration confirmed:.plugin(tauri_plugin_os::init())is present insrc-tauri/src/lib.rsandtauri-plugin-osappears in the Cargo.toml; no further action required.src/lib/eventGrouping.ts (1)
15-16: Assumption: input is chronologically orderedThe algorithm relies on message order for grouping. Please confirm ChatArea (or callers) pass messages sorted by timestamp ascending; otherwise group boundaries may be incorrect.
Also applies to: 39-49, 75-79
src/store/index.ts (1)
1931-1931: Ignore CHANNNOTICE typo warning
The EventMap inircClientdefines and emits “CHANNNOTICE” (three N’s), so the handler is correct as written.Likely an incorrect or invalid review comment.
| - main | ||
| tags: | ||
| - "v*.*.*" | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Manual dispatch can publish from any branch; restrict or require explicit confirmation.
With workflow_dispatch enabled and the downstream guard (Line 13) permitting all manual runs, a full signed release can be created from non-main branches. Tighten scope and/or add inputs.
Suggested update (restrict dispatch to main and add safety inputs):
-on:
+on:
push:
branches:
- main
tags:
- "v*.*.*"
- workflow_dispatch:
+ workflow_dispatch:
+ inputs:
+ allow_non_main:
+ description: "Allow publishing from non-main branch?"
+ type: boolean
+ default: false
+ required: true
+ draft:
+ description: "Create draft release?"
+ type: boolean
+ default: false
+ required: true
+ prerelease:
+ description: "Mark as prerelease?"
+ type: boolean
+ default: true
+ required: trueOptionally, also set outputs from these inputs (and wire them into downstream jobs) if you want operator control rather than hardcoding prerelease/draft. Do you want me to wire these through?
🏁 Script executed:
#!/bin/bash
sed -n '1,30p' .github/workflows/publish.yamlLength of output: 905
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' .github/workflows/publish.yamlLength of output: 7188
Restrict manual dispatch to main or require explicit confirmation. The unqualified workflow_dispatch trigger and collect-version if: allow releases from any branch; scope the trigger to main or add a required boolean input (e.g. allow_non_main) to prevent unintended publishes.
🤖 Prompt for AI Agents
.github/workflows/publish.yaml around line 9: the workflow_dispatch is
unqualified allowing manual runs from any branch and the collect-version `if:`
likewise permits releases from non-main; either scope the trigger to main or add
a required boolean confirmation input. Update workflow_dispatch to include an
inputs block (e.g., allow_non_main with required=true and description) and/or
restrict allowed branches by checking github.ref == 'refs/heads/main'; then
change the collect-version `if:` to require either github.ref ==
'refs/heads/main' OR the input flag (e.g., inputs.allow_non_main == 'true');
ensure the inputs name matches and that string/boolean handling follows GitHub
Actions YAML semantics so only explicit confirmation permits non-main publishes.
| jobs: | ||
| collect-version: | ||
| if: github.ref_name == 'main' || github.ref_type == 'tag' | ||
| if: github.ref_name == 'main' || github.ref_type == 'tag' || github.event_name == 'workflow_dispatch' |
There was a problem hiding this comment.
Tighten job guard to avoid releases from non‑main manual runs.
Current guard allows any workflow_dispatch regardless of branch. Limit to main (still allow tags):
- if: github.ref_name == 'main' || github.ref_type == 'tag' || github.event_name == 'workflow_dispatch'
+ if: github.ref_type == 'tag' || github.ref_name == 'main' || (github.event_name == 'workflow_dispatch' && github.ref_name == 'main')If you adopt workflow_dispatch inputs (draft/prerelease), also replace hardcoded outputs with input-driven values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if: github.ref_name == 'main' || github.ref_type == 'tag' || github.event_name == 'workflow_dispatch' | |
| if: github.ref_type == 'tag' || github.ref_name == 'main' || (github.event_name == 'workflow_dispatch' && github.ref_name == 'main') |
🤖 Prompt for AI Agents
.github/workflows/publish.yaml around line 13: the current if condition allows
any workflow_dispatch to run the publish job (risking releases from non-main
branches); tighten it to only permit runs on the main branch or tags by changing
the condition to require either github.ref_name == 'main' or github.ref_type ==
'tag' or (github.event_name == 'workflow_dispatch' && github.ref_name ==
'main'); if you switch to workflow_dispatch inputs for draft/prerelease flags,
replace any hardcoded output values in the job with values derived from those
inputs.
| - Updated viewport meta tag to include `viewport-fit=cover, user-scalable=no` | ||
| - Provides better mobile viewport handling | ||
|
|
There was a problem hiding this comment.
Avoid user-scalable=no (accessibility)
Disabling zoom hinders users with low vision. Prefer keeping zoom enabled and addressing layout via CSS/JS without restricting scaling.
🤖 Prompt for AI Agents
In ANDROID_KEYBOARD_FIX.md around lines 21 to 23, the meta tag change adds
`user-scalable=no` which disables pinch-zoom and harms accessibility; remove
`user-scalable=no` (or explicitly set `user-scalable=yes`) and avoid any
`maximum-scale` restrictions so users can zoom; keep `viewport-fit=cover`, and
instead fix layout issues with responsive CSS/JS (fluid widths, media queries,
proper input sizing) and touch-action adjustments; test on mobile to verify
pinch-zoom remains functional.
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0, viewport-fit=cover, user-scalable=no" /> |
There was a problem hiding this comment.
Consider the accessibility impact of user-scalable=no.
While user-scalable=no may help with mobile keyboard handling and prevent unintended zoom gestures, it can negatively impact accessibility for users who rely on pinch-to-zoom to read content. WCAG guidelines generally discourage disabling zoom.
Consider whether the keyboard handling benefits outweigh the accessibility trade-off, or if alternative solutions (e.g., using maximum-scale=1.0 with touch-action CSS) could achieve similar results while preserving some zoom capability.
🤖 Prompt for AI Agents
In index.html around line 6, the meta viewport currently uses user-scalable=no
which blocks pinch-to-zoom and harms accessibility; remove user-scalable=no (or
replace it with maximum-scale=1.0 if absolutely needed) and instead address
mobile keyboard/gesture issues via CSS (e.g., appropriate touch-action rules) or
JS focus handling so users retain zoom capability per WCAG recommendations.
| // Reset avatar load failed state when user or server changes | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: We want to reset when user/server changes | ||
| useEffect(() => { | ||
| setAvatarLoadFailed(false); | ||
| }, [currentUser?.username, selectedServerId]); | ||
|
|
There was a problem hiding this comment.
Avatar load reset misses avatar URL changes
If the avatar URL changes for the same user, the failure flag won’t reset.
- useEffect(() => {
- setAvatarLoadFailed(false);
- }, [currentUser?.username, selectedServerId]);
+ useEffect(() => {
+ setAvatarLoadFailed(false);
+ }, [currentUser?.username, currentUser?.metadata?.avatar?.value, selectedServerId]);📝 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.
| // Reset avatar load failed state when user or server changes | |
| // biome-ignore lint/correctness/useExhaustiveDependencies: We want to reset when user/server changes | |
| useEffect(() => { | |
| setAvatarLoadFailed(false); | |
| }, [currentUser?.username, selectedServerId]); | |
| // Reset avatar load failed state when user or server changes | |
| // biome-ignore lint/correctness/useExhaustiveDependencies: We want to reset when user/server changes | |
| useEffect(() => { | |
| setAvatarLoadFailed(false); | |
| }, [currentUser?.username, currentUser?.metadata?.avatar?.value, selectedServerId]); |
🤖 Prompt for AI Agents
In src/components/layout/ChannelList.tsx around lines 89 to 94, the useEffect
that resets avatarLoadFailed only depends on currentUser?.username and
selectedServerId so it won’t run when the current user's avatar URL changes;
update the dependency array to include the user's avatar field (e.g.,
currentUser?.avatarUrl or currentUser?.avatar_url depending on the model) so the
effect resets when the avatar changes, and adjust/remove the biome-ignore lint
comment if necessary to reflect the intended exhaustive dependencies.
| // Don't play sound for our own messages | ||
| if (currentUser && message.userId === currentUser.username) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix own-message detection (prevents unwanted self-notifications)
message.userId likely contains more than the nick. Compare against the nick component.
- // Don't play sound for our own messages
- if (currentUser && message.userId === currentUser.username) {
+ // Don't play sound for our own messages
+ const author = message.userId.split("-")[0];
+ if (currentUser && author === currentUser.username) {
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Don't play sound for our own messages | |
| if (currentUser && message.userId === currentUser.username) { | |
| return false; | |
| } | |
| // Don't play sound for our own messages | |
| const author = message.userId.split("-")[0]; | |
| if (currentUser && author === currentUser.username) { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/lib/notificationSounds.ts around lines 76 to 79, the existing
self-message check compares the full message.userId to currentUser.username but
message.userId can include extra metadata; extract the nick component (the
substring before any separator such as '|' or '@' or ':') from message.userId
and compare that nick to currentUser.username, returning false when they match
so we correctly suppress notifications for our own messages.
| interface GlobalSettings { | ||
| enableNotifications: boolean; | ||
| notificationSound: string; | ||
| enableNotificationSounds: boolean; | ||
| enableHighlights: boolean; | ||
| sendTypingNotifications: boolean; | ||
| // Event visibility settings | ||
| showEvents: boolean; | ||
| showNickChanges: boolean; | ||
| showJoinsParts: boolean; | ||
| showQuits: boolean; | ||
| // Custom mentions | ||
| customMentions: string[]; | ||
| // Ignore list | ||
| ignoreList: string[]; | ||
| // Hosted chat mode settings | ||
| nickname: string; | ||
| accountName: string; | ||
| accountPassword: string; | ||
| // Multiline settings | ||
| enableMultilineInput: boolean; | ||
| multilineOnShiftEnter: boolean; | ||
| autoFallbackToSingleLine: boolean; | ||
| } |
There was a problem hiding this comment.
Global secrets stored in localStorage (security/privacy).
accountPassword and SASL password are persisted (even base64) in localStorage. This is recoverable by scripts and not protected.
Recommendations:
- Avoid storing passwords; prefer OS keychain/secure storage.
- If unavoidable, encrypt at rest with a user-provided key and document risks.
- At minimum, gate features with explicit user consent and provide a “clear credentials” action.
Also applies to: 506-530
| set((state) => { | ||
| const openState = | ||
| isOpen !== undefined ? isOpen : !state.ui.isChannelListVisible; | ||
|
|
||
| // Only change mobileViewActiveColumn if we're not on the serverList view | ||
| // This prevents desktop member list toggles from affecting mobile navigation | ||
| const shouldUpdateMobileColumn = | ||
| state.ui.mobileViewActiveColumn !== "serverList"; | ||
|
|
||
| return { | ||
| ui: { | ||
| ...state.ui, | ||
| isMemberListVisible: | ||
| openState !== undefined ? openState : !state.ui.isMemberListVisible, | ||
| mobileViewActiveColumn: openState ? "memberList" : "chatView", | ||
| mobileViewActiveColumn: shouldUpdateMobileColumn | ||
| ? openState | ||
| ? "memberList" | ||
| : "chatView" | ||
| : state.ui.mobileViewActiveColumn, | ||
| }, |
There was a problem hiding this comment.
toggleMemberList uses channel-list visibility to compute state.
Should flip isMemberListVisible, not isChannelListVisible.
-const openState =
- isOpen !== undefined ? isOpen : !state.ui.isChannelListVisible;
+const openState =
+ isOpen !== undefined ? isOpen : !state.ui.isMemberListVisible;📝 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.
| set((state) => { | |
| const openState = | |
| isOpen !== undefined ? isOpen : !state.ui.isChannelListVisible; | |
| // Only change mobileViewActiveColumn if we're not on the serverList view | |
| // This prevents desktop member list toggles from affecting mobile navigation | |
| const shouldUpdateMobileColumn = | |
| state.ui.mobileViewActiveColumn !== "serverList"; | |
| return { | |
| ui: { | |
| ...state.ui, | |
| isMemberListVisible: | |
| openState !== undefined ? openState : !state.ui.isMemberListVisible, | |
| mobileViewActiveColumn: openState ? "memberList" : "chatView", | |
| mobileViewActiveColumn: shouldUpdateMobileColumn | |
| ? openState | |
| ? "memberList" | |
| : "chatView" | |
| : state.ui.mobileViewActiveColumn, | |
| }, | |
| set((state) => { | |
| - const openState = | |
| const openState = | |
| isOpen !== undefined ? isOpen : !state.ui.isMemberListVisible; | |
| // Only change mobileViewActiveColumn if we're not on the serverList view | |
| // This prevents desktop member list toggles from affecting mobile navigation | |
| const shouldUpdateMobileColumn = | |
| state.ui.mobileViewActiveColumn !== "serverList"; | |
| return { | |
| ui: { | |
| ...state.ui, | |
| isMemberListVisible: | |
| openState !== undefined ? openState : !state.ui.isMemberListVisible, | |
| mobileViewActiveColumn: shouldUpdateMobileColumn | |
| ? openState | |
| ? "memberList" | |
| : "chatView" | |
| : state.ui.mobileViewActiveColumn, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In src/store/index.ts around lines 1256 to 1275, the computation of openState
uses state.ui.isChannelListVisible which is incorrect for toggleMemberList;
change that to reference state.ui.isMemberListVisible so the default toggle
flips the member list visibility. Keep the rest of the logic
(shouldUpdateMobileColumn and mobileViewActiveColumn assignment) intact so
mobile column behavior is unchanged.
| export interface Message { | ||
| id?: string; | ||
| msgid?: string; | ||
| content: string; | ||
| timestamp: Date; | ||
| userId: string; | ||
| channelId: string; | ||
| serverId: string; | ||
| id: string; | ||
| msgid?: string; // IRC message ID from IRCv3 message-ids capability | ||
| multilineMessageIds?: string[]; // For multiline messages: all message IDs that make up this message | ||
| type: | ||
| | "message" | ||
| | "system" | ||
| | "error" | ||
| | "join" | ||
| | "leave" | ||
| | "part" | ||
| | "quit" | ||
| | "nick" | ||
| | "standard-reply"; | ||
| | "leave" | ||
| | "standard-reply" | ||
| | "notice" | ||
| | "netsplit" | ||
| | "netjoin"; | ||
| content: string; | ||
| timestamp: Date; | ||
| userId: string; | ||
| channelId: string; | ||
| serverId: string; | ||
| reactions: Reaction[]; | ||
| replyMessage: Message | null | undefined; | ||
| replyMessage: Message | null; | ||
| mentioned: string[]; |
There was a problem hiding this comment.
🧩 Analysis chain
Message.id is now required; ensure all creators set it
This is a breaking change. Verify all message construction sites provide a stable id and handle new type variants.
Run to locate message object literals and check for id:
🏁 Script executed:
#!/bin/bash
# Find likely message creations and show context
rg -nP -C3 --type=ts '(type:\s*"(message|system|error|join|part|quit|nick|leave|standard-reply|notice|netsplit|netjoin)")|(\bmessages?\s*:\s*\[)' Length of output: 18737
Add missing id to Message object literals
Message.id is now required; update all message construction sites (including tests like tests/lib/notificationSounds.test.ts and component tests) to include a stable id.
🤖 Prompt for AI Agents
In src/types/index.ts around lines 74 to 98, the Message interface now requires
an id field but many message construction sites (including tests like
tests/lib/notificationSounds.test.ts and component tests) do not supply it;
update all places that create Message objects to include a stable id value.
Ensure each created Message includes a deterministic/stable id (for tests use a
fixed string or seeded generator; for runtime use your app's id generation
strategy such as UUID or a composed stable key), update fixtures and mocks
accordingly, and run tests to confirm all Message literals now include the id
property.
| expect(ircClient.connect).toHaveBeenCalledWith( | ||
| "Test Server", | ||
| "irc.test.com", | ||
| 443, | ||
| "tester", | ||
| "", | ||
| "", | ||
| "tester", | ||
| "c3VwZXIgYXdlc29tZSBwYXNzd29yZCBsbWFvIDEyMyAhPyE/IQ==", | ||
| undefined, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the updated connect signature across the codebase.
The test shows ircClient.connect being called with "Test Server" as the first argument, followed by host, port, and authentication details. Ensure all callers have been updated to match this signature.
Run the following script to verify the usage:
🏁 Script executed:
#!/bin/bash
# Description: Find all calls to ircClient.connect and verify signature consistency
# Search for connect calls
rg -nP --type=ts --type=tsx -C3 'ircClient\.connect\s*\('Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for all ircClient.connect calls in TypeScript and TSX files
rg -nP -n -C3 'ircClient\.connect\s*\(' -g '*.ts' -g '*.tsx'Length of output: 309
Update ircClient.connect calls to new signature
The invocation in src/store/index.ts:565 still passes only (name, host, port); it must include the added parameters—userName, password, nick, the base64-encoded token, and the optional last argument—to match the updated signature.
🤖 Prompt for AI Agents
In tests/App.test.tsx around lines 147-156, the test expectation calls
ircClient.connect with only (name, host, port) but the code in
src/store/index.ts (around line 565) still invokes ircClient.connect with the
old 3-arg signature; update that call to pass the additional parameters in the
correct order: userName, password, nick, the base64-encoded token string, and
the optional last argument (or undefined) so the invocation matches the new
signature and the test expectation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/store/index.ts (4)
272-295: Stop storing credentials in localStorage.
accountPassword(and related credentials) now persist in plain text insideglobalSettings; this keeps sensitive auth data in localStorage where any script can read it, repeating the prior security concern. Please remove these secrets from persisted settings (or migrate them to secure storage gated by explicit consent) before shipping.
1256-1275: Fix member list toggle state.The default toggle path still inverts
isChannelListVisible, so the member list button flips the wrong pane. Usestate.ui.isMemberListVisiblewhen derivingopenState.Apply this diff:
- const openState = - isOpen !== undefined ? isOpen : !state.ui.isChannelListVisible; + const openState = + isOpen !== undefined ? isOpen : !state.ui.isMemberListVisible;
2695-2707: Allow nickname fallback for SASL CAP defer.Requiring
saslAccountNamehere still skipspreventCapEndfor users who authenticate with nickname-only SASL, so CAP END fires before AUTHENTICATE. Gate onsaslPasswordplus either account name or nickname as previously requested.Apply this diff:
- if ( - savedServer?.saslEnabled && - savedServer?.saslAccountName && - savedServer?.saslPassword - ) { - preventCapEnd = true; - } + if ( + savedServer?.saslEnabled && + savedServer?.saslPassword && + (savedServer.saslAccountName?.trim().length || + savedServer.nickname?.trim().length) + ) { + preventCapEnd = true; + }
2986-3054: Use+draft/replyfor link previews.This branch still keys off
mtags["+reply"], so previews never attach to server TAGMSG payloads (which only provide+draft/reply). Please switch to the draft key with a fallback to+reply.Apply this diff:
- (mtags["obsidianirc/link-preview-meta"]) && - mtags["+reply"] + (mtags["obsidianirc/link-preview-meta"]) && + (mtags["+draft/reply"] ?? mtags["+reply"]) ) { - const replyMessageId = mtags["+reply"]; + const replyMessageId = + mtags["+draft/reply"] ?? mtags["+reply"];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/store/index.ts(51 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/store/index.ts (3)
src/lib/ignoreUtils.ts (1)
isUserIgnored(45-82)src/lib/notificationSounds.ts (2)
shouldPlayNotificationSound(62-110)playNotificationSound(6-59)src/types/index.ts (4)
Message(74-116)PrivateChat(60-67)Channel(45-58)User(1-13)
| ircClient.on("CHANNNOTICE", (response) => { | ||
| const { mtags, channelName, message, timestamp } = response; | ||
|
|
||
| return { ...server, channels: updatedChannels }; | ||
| } | ||
| return server; | ||
| }); | ||
| // Check if sender is ignored | ||
| const globalSettings = useStore.getState().globalSettings; | ||
| if ( | ||
| isUserIgnored( | ||
| response.sender, | ||
| undefined, | ||
| undefined, | ||
| globalSettings.ignoreList, | ||
| ) | ||
| ) { | ||
| // User is ignored, skip processing this notice | ||
| return; | ||
| } | ||
|
|
||
| return { servers: updatedServers }; | ||
| }); | ||
| // Find the server and channel | ||
| const server = useStore | ||
| .getState() | ||
| .servers.find((s) => s.id === response.serverId); | ||
|
|
||
| // Request metadata for all users in the channel (except current user) | ||
| const currentState = useStore.getState(); | ||
| const currentUser = currentState.currentUser; | ||
| users.forEach((user, index) => { | ||
| if (currentUser && user.username !== currentUser.username) { | ||
| // Stagger requests to avoid overwhelming the server | ||
| setTimeout(() => { | ||
| useStore.getState().metadataList(serverId, user.username); | ||
| }, index * 200); // 200ms delay between requests | ||
| } | ||
| }); | ||
| const usersToFetch = users.filter( | ||
| (u) => u.username !== currentUser?.username, | ||
| ); | ||
| if (!server) return; | ||
|
|
||
| // Process in batches with shorter delays | ||
| const batchSize = 10; | ||
| const batchDelay = 500; // 500ms between batches | ||
|
|
||
| for (let i = 0; i < usersToFetch.length; i += batchSize) { | ||
| const batch = usersToFetch.slice(i, i + batchSize); | ||
| setTimeout( | ||
| () => { | ||
| batch.forEach((user, idx) => { | ||
| setTimeout(() => { | ||
| useStore.getState().metadataList(serverId, user.username); | ||
| }, idx * 50); // 50ms between requests in a batch | ||
| }); | ||
| }, | ||
| Math.floor(i / batchSize) * batchDelay, | ||
| ); | ||
| const channel = server.channels.find((c) => c.name === channelName); | ||
|
|
||
| if (channel) { | ||
| const newMessage: Message = { | ||
| id: uuidv4(), | ||
| type: "notice", // Different message type for notices | ||
| content: message, | ||
| timestamp: timestamp, | ||
| userId: response.sender, | ||
| channelId: channel.id, | ||
| serverId: server.id, | ||
| reactions: [], | ||
| replyMessage: null, | ||
| mentioned: [], | ||
| tags: mtags, | ||
| }; | ||
|
|
||
| useStore.getState().addMessage(newMessage); | ||
|
|
||
| // Play notification sound if appropriate | ||
| const state = useStore.getState(); | ||
| const serverCurrentUser = ircClient.getCurrentUser(response.serverId); | ||
| if ( | ||
| shouldPlayNotificationSound( | ||
| newMessage, | ||
| serverCurrentUser, | ||
| state.globalSettings, | ||
| ) | ||
| ) { | ||
| playNotificationSound(state.globalSettings); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix CHANNOTICE handler name.
The new handler registers "CHANNNOTICE" (three N’s); the client emits CHANNOTICE, so this callback never runs. Rename the event to match the emitter.
Apply this diff:
-ircClient.on("CHANNNOTICE", (response) => {
+ircClient.on("CHANNOTICE", (response) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ircClient.on("CHANNNOTICE", (response) => { | |
| const { mtags, channelName, message, timestamp } = response; | |
| return { ...server, channels: updatedChannels }; | |
| } | |
| return server; | |
| }); | |
| // Check if sender is ignored | |
| const globalSettings = useStore.getState().globalSettings; | |
| if ( | |
| isUserIgnored( | |
| response.sender, | |
| undefined, | |
| undefined, | |
| globalSettings.ignoreList, | |
| ) | |
| ) { | |
| // User is ignored, skip processing this notice | |
| return; | |
| } | |
| return { servers: updatedServers }; | |
| }); | |
| // Find the server and channel | |
| const server = useStore | |
| .getState() | |
| .servers.find((s) => s.id === response.serverId); | |
| // Request metadata for all users in the channel (except current user) | |
| const currentState = useStore.getState(); | |
| const currentUser = currentState.currentUser; | |
| users.forEach((user, index) => { | |
| if (currentUser && user.username !== currentUser.username) { | |
| // Stagger requests to avoid overwhelming the server | |
| setTimeout(() => { | |
| useStore.getState().metadataList(serverId, user.username); | |
| }, index * 200); // 200ms delay between requests | |
| } | |
| }); | |
| const usersToFetch = users.filter( | |
| (u) => u.username !== currentUser?.username, | |
| ); | |
| if (!server) return; | |
| // Process in batches with shorter delays | |
| const batchSize = 10; | |
| const batchDelay = 500; // 500ms between batches | |
| for (let i = 0; i < usersToFetch.length; i += batchSize) { | |
| const batch = usersToFetch.slice(i, i + batchSize); | |
| setTimeout( | |
| () => { | |
| batch.forEach((user, idx) => { | |
| setTimeout(() => { | |
| useStore.getState().metadataList(serverId, user.username); | |
| }, idx * 50); // 50ms between requests in a batch | |
| }); | |
| }, | |
| Math.floor(i / batchSize) * batchDelay, | |
| ); | |
| const channel = server.channels.find((c) => c.name === channelName); | |
| if (channel) { | |
| const newMessage: Message = { | |
| id: uuidv4(), | |
| type: "notice", // Different message type for notices | |
| content: message, | |
| timestamp: timestamp, | |
| userId: response.sender, | |
| channelId: channel.id, | |
| serverId: server.id, | |
| reactions: [], | |
| replyMessage: null, | |
| mentioned: [], | |
| tags: mtags, | |
| }; | |
| useStore.getState().addMessage(newMessage); | |
| // Play notification sound if appropriate | |
| const state = useStore.getState(); | |
| const serverCurrentUser = ircClient.getCurrentUser(response.serverId); | |
| if ( | |
| shouldPlayNotificationSound( | |
| newMessage, | |
| serverCurrentUser, | |
| state.globalSettings, | |
| ) | |
| ) { | |
| playNotificationSound(state.globalSettings); | |
| } | |
| } | |
| }); | |
| ircClient.on("CHANNOTICE", (response) => { | |
| const { mtags, channelName, message, timestamp } = response; | |
| // Check if sender is ignored | |
| const globalSettings = useStore.getState().globalSettings; | |
| if ( | |
| isUserIgnored( | |
| response.sender, | |
| undefined, | |
| undefined, | |
| globalSettings.ignoreList, | |
| ) | |
| ) { | |
| // User is ignored, skip processing this notice | |
| return; | |
| } | |
| // Find the server and channel | |
| const server = useStore | |
| .getState() | |
| .servers.find((s) => s.id === response.serverId); | |
| if (!server) return; | |
| const channel = server.channels.find((c) => c.name === channelName); | |
| if (channel) { | |
| const newMessage: Message = { | |
| id: uuidv4(), | |
| type: "notice", // Different message type for notices | |
| content: message, | |
| timestamp: timestamp, | |
| userId: response.sender, | |
| channelId: channel.id, | |
| serverId: server.id, | |
| reactions: [], | |
| replyMessage: null, | |
| mentioned: [], | |
| tags: mtags, | |
| }; | |
| useStore.getState().addMessage(newMessage); | |
| // Play notification sound if appropriate | |
| const state = useStore.getState(); | |
| const serverCurrentUser = ircClient.getCurrentUser(response.serverId); | |
| if ( | |
| shouldPlayNotificationSound( | |
| newMessage, | |
| serverCurrentUser, | |
| state.globalSettings, | |
| ) | |
| ) { | |
| playNotificationSound(state.globalSettings); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
In src/store/index.ts around lines 1931 to 1987, the event handler is registered
for "CHANNNOTICE" (three N's) but the client emits "CHANNOTICE" (two N's);
update the event name string from "CHANNNOTICE" to "CHANNOTICE" so the callback
is actually invoked by the emitter, leaving the rest of the handler logic
unchanged.
|
Looks weird the new font |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
index.html (1)
6-6: Restore pinch-zoom for accessibility.We still ship
user-scalable=no, which blocks zoom and violates WCAG expectations; please drop that attribute (or adopt a less restrictive alternative).src/store/index.ts (2)
2699-2710: SASL check still blocks nickname fallback despite being marked as addressed.Although this was marked as addressed in commit c2091c8, the condition on lines 2704-2707 still requires
savedServer?.saslAccountNameto be truthy. This preventspreventCapEndfrom being set when the user relies on nickname-based SASL (empty account field), causing CAP END to be sent before AUTHENTICATE starts. The AUTHENTICATE handler on lines 2647-2671 already implements nickname fallback (user = serv.saslAccountName?.length ? serv.saslAccountName : serv.nickname;), but it never runs because this guard blocks it.Apply the previously suggested fix:
if ( savedServer?.saslEnabled && - savedServer?.saslAccountName && savedServer?.saslPassword ) { - preventCapEnd = true; + const hasAuthUser = + savedServer.saslAccountName?.trim().length || + savedServer.nickname?.trim().length; + if (hasAuthUser) { + preventCapEnd = true; + } }
2988-3059: Link preview still checks wrong reply tag key.Lines 2995 and 2997 check
mtags["+reply"], but the server sendsmtags["+draft/reply"](as used by all other handlers like reactions at line 2878 and unreacts at line 2937). This means link previews are never attached because the condition never matches.Apply the previously suggested fix:
if ( mtags && (mtags["obsidianirc/link-preview-title"] || mtags["obsidianirc/link-preview-snippet"] || mtags["obsidianirc/link-preview-meta"]) && - mtags["+reply"] + (mtags["+draft/reply"] || mtags["+reply"]) ) { - const replyMessageId = mtags["+reply"]; + const replyMessageId = + mtags["+draft/reply"] || mtags["+reply"];
🧹 Nitpick comments (1)
src/store/index.ts (1)
3536-3542: Consider explicit else clause for clarity.The logic correctly preserves local metadata when the server returns empty/null values. However, an explicit else clause with a comment would make this intent clearer to future maintainers.
Apply this diff to add the else clause:
if (value !== null && value !== undefined && value !== "") { savedMetadata[serverId][resolvedTarget][key] = { value, visibility }; saveMetadataToLocalStorage(savedMetadata); + } else { + // Server value is empty/null/undefined; keep our local value as per comment above + // No action needed, local value is preserved }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ANDROID_KEYBOARD_FIX.md(0 hunks)index.html(1 hunks)src/index.css(1 hunks)src/lib/ircClient.ts(1 hunks)src/store/index.ts(6 hunks)test_multiline.md(0 hunks)
💤 Files with no reviewable changes (2)
- ANDROID_KEYBOARD_FIX.md
- test_multiline.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/store/index.ts (1)
src/types/index.ts (2)
Channel(45-58)PrivateChat(60-67)
🔇 Additional comments (3)
src/store/index.ts (3)
3558-3568: LGTM! Metadata update logic correctly handles fetch vs update scenarios.The conditional logic properly distinguishes between:
- Fetching our own metadata (preserve local values when server sends empty)
- Receiving metadata updates (remove keys not set on server)
This prevents server empty responses from clearing local values during fetch while still allowing proper cleanup during updates.
3602-3612: LGTM! Consistent with channel user metadata handling.The current user metadata logic mirrors the channel user handling at lines 3558-3568, maintaining consistency across the codebase.
3963-3970: LGTM! Properly restores user metadata from localStorage.The WHO_REPLY handler correctly loads and merges saved metadata for each user, maintaining consistency with the
restoreServerMetadatafunction pattern used elsewhere in the file.
| // Only enable SASL if we have both account name AND password | ||
| this.saslEnabled.set(server.id, !!(_saslAccountName && _saslPassword)); |
There was a problem hiding this comment.
SASL enabled logic should not require account name.
The condition requires both _saslAccountName AND _saslPassword to enable SASL, preventing nickname-based SASL authentication. This is the root cause of the issue flagged in src/store/index.ts at lines 2699-2710. The AUTHENTICATE handler already implements nickname fallback, so SASL should be enabled whenever a password is provided, regardless of whether an explicit account name is given.
Apply this diff:
- // Only enable SASL if we have both account name AND password
- this.saslEnabled.set(server.id, !!(_saslAccountName && _saslPassword));
+ // Enable SASL if password is provided (account name can fallback to nickname)
+ this.saslEnabled.set(server.id, !!_saslPassword);This must be fixed in conjunction with the store fix to enable proper nickname-based SASL authentication.
📝 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.
| // Only enable SASL if we have both account name AND password | |
| this.saslEnabled.set(server.id, !!(_saslAccountName && _saslPassword)); | |
| // Enable SASL if password is provided (account name can fallback to nickname) | |
| this.saslEnabled.set(server.id, !!_saslPassword); |
🤖 Prompt for AI Agents
In src/lib/ircClient.ts around lines 278-279, the current logic enables SASL
only if both _saslAccountName and _saslPassword are present; change it to enable
SASL whenever a password is provided (i.e., base on _saslPassword only) so
nickname-based SASL can be used; update the this.saslEnabled.set(server.id, ...)
call to compute enabled as !!_saslPassword (leave account name usage to the
AUTHENTICATE handler which already falls back to nickname); ensure this change
is coordinated with the store fix mentioned so nickname-based SASL
authentication is fully supported.
* feat: comprehensive IRCv3 enhancements and performance improvements Major Features: - Link previews with TAGMSG support and external link warnings - METADATA support (IRCv3 draft/metadata-2) with persistence - User avatars, display names, and profile metadata - SASL authentication improvements - Multiline message support with draft/multiline - Message reactions and reply threading - Channel rename support - Nickname retry logic Performance: - Optimize ChatArea re-renders with useMemo for message grouping - Fix input lag with medium/large message history - Reactive metadata updates in User Status widget UI/UX: - JetBrains Mono font throughout - Improved mobile keyboard handling - External link warning modal - Better metadata display in member lists Bug Fixes: - Fix metadata flickering on connect - Prevent empty server responses from clearing local metadata - Fix SASL/CAP negotiation flow - Fix newline handling in message input - Fix metadata reactivity in User Status widget Developer Experience: - Remove debug console logs (kept console.error) - Improve installation documentation - Add ZorinOS/glibc troubleshooting - Reduce bundle size by 6KB * Update src/store/index.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Syntax error somehow * Use JetBrains Mono * Remove slop lol * Remove slop lol * Remove slop lol --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary by CodeRabbit
New Features
Enhancements
Style
Documentation