General mobile and desktop polishment#135
Conversation
Also fixes some modals with different style
but not narrow enough for mobile view
|
@CodeRabbit ignore |
📝 WalkthroughWalkthroughAdds AppImage .zsync generation/upload in CI, Tauri/platform helpers and openExternalUrl, a BaseModal system, swipe/long-press message UX and bottom sheet, a pointer-driven useDragReorder hook, persisted sidebar widths/visibility, QuickActions types/config, many UI modal refactors, and corresponding test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SwipeableMessage
participant MessageBottomSheet
participant Store
participant IRC
User->>SwipeableMessage: long-press or swipe (narrow view)
SwipeableMessage->>MessageBottomSheet: open(sheet)
MessageBottomSheet-->>User: show actions (reply/react/delete)
User->>MessageBottomSheet: select "Reply"
MessageBottomSheet->>Store: dispatch reply action
Store->>IRC: send reply message
IRC-->>Store: ack/update
Store-->>SwipeableMessage: update UI (close sheet / highlight)
sequenceDiagram
participant User
participant ChannelList
participant useDragReorder
participant DOM
participant Store
User->>ChannelList: pointerdown on item
ChannelList->>useDragReorder: handlePointerDown
useDragReorder->>DOM: capture pointer & track items
User->>ChannelList: pointermove (drag)
ChannelList->>useDragReorder: handlePointerMove
useDragReorder->>DOM: compute drop target & apply transforms
User->>ChannelList: pointerup (drop)
ChannelList->>useDragReorder: handlePointerUp
useDragReorder->>Store: onReorder(newOrder)
Store-->>DOM: re-render list with new order
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/App.test.tsx (1)
161-199:⚠️ Potential issue | 🟡 MinorDuplicate test: "Shows error message when server connection fails" appears twice with identical bodies.
Lines 161–179 and 181–199 define the exact same test name and logic. One should be removed or differentiated.
🧹 Proposed fix
- it("Shows error message when server connection fails", async () => { - render( - <BrowserRouter> - <App /> - </BrowserRouter>, - ); - const user = userEvent.setup(); - - // Mock failed connection - vi.mocked(ircClient.connect).mockRejectedValueOnce( - new Error("Connection failed"), - ); - - // Open modal by clicking add server button - await user.click(screen.getByTestId("server-list-add-button")); - - // Check that toggleAddServerModal was called - expect(mockStoreState.toggleAddServerModal).toHaveBeenCalledWith(true); - });src/store/index.ts (1)
357-409:⚠️ Potential issue | 🟠 MajorDeclare and initialize modal context flags used by the new toggles.
toggleTopicModal/toggleUserProfileModalWithContextwriteui.isTopicModalOpen,ui.topicModalContext, andui.userProfileModalContext, but these fields aren’t declared inUIStateor initialized inui. This causes TS excess-property errors and an inconsistent runtime state shape. Add the fields (or switch the setters to existing request fields).💡 Suggested fix
interface UIState { ... topicModalRequest: { serverId: string; channelId: string } | null; + isTopicModalOpen: boolean; + topicModalContext: { serverId: string; channelId: string } | null; + userProfileModalContext: { serverId: string; username: string } | null; ... isUserProfileModalOpen: boolean; channelSettingsRequest: { serverId: string; channelId: string } | null; inviteUserRequest: { serverId: string; channelId: string } | null; } ui: { ... topicModalRequest: null, + isTopicModalOpen: false, + topicModalContext: null, + userProfileModalContext: null, ... isUserProfileModalOpen: false, channelSettingsRequest: null, inviteUserRequest: null, },Also applies to: 805-852, 675-683, 2574-2591
🤖 Fix all issues with AI agents
In @.github/workflows/publish.yaml:
- Around line 108-128: The published amd64 AppImage is uploaded before the
"Embed update information in AppImages" step runs, so your patch never reaches
the release; modify the workflow so the release action uploads the patched
AppImage (or delay release until after embedding). Specifically, either add the
patched AppImage path
(./src-tauri/target/release/bundle/appimage/*_amd64.AppImage) to the files: list
of the softprops/action-gh-release step named "Upload zsync files", or change
the tauri-action invocation (the earlier "tauri-action" build step) to only
build (use tauriScript or disable automatic release upload) so that the "Embed
update information in AppImages" step runs before softprops/action-gh-release
publishes; ensure APPIMAGE_DIR and the "Embed update information in AppImages"
step still point to the same files so the uploaded artifacts include the
dd-patched AppImage.
In `@src/components/layout/ResizableSidebar.tsx`:
- Around line 39-51: The early return "if (bypass) return children" causes a
rules-of-hooks violation because it short-circuits hooks declared later (e.g.,
validateWidth useCallback and subsequent useState calls) in the ResizableSidebar
component; move the bypass check so that all hooks (including validateWidth, any
useState/useEffect calls) are executed unconditionally at the top of the
component, then perform the bypass branch in the render return (e.g., return
children JSX when bypass is true) so hook order remains stable even if bypass
toggles at runtime.
In `@src/components/ui/AddServerModal.tsx`:
- Around line 101-119: The code in AddServerModal.tsx currently strips only the
scheme from serverHost into cleanHost then always appends :${port}, causing
duplicates when the user included a port (e.g., "irc.example.com:6697"). Update
the logic that computes cleanHost to also remove a trailing :<port> (or parse
host:port) before computing isLocalhost/isSSLPort and building finalHost; adjust
references to serverHost/cleanHost/port/useWebSocket/finalHost so you first
normalize serverHost by removing any scheme and any trailing :number, then
decide isLocalhost and isSSLPort and only append :port if the original input did
not already include a port.
In `@src/hooks/useDragReorder.ts`:
- Around line 36-154: The pointer capture is being set on the down event but
released assuming e.currentTarget is the capturer; update handlePointerDown to
save the capturing element (e.currentTarget) into dragState.current (e.g.,
capturingElement), and update handlePointerUp to read that saved element and
only call releasePointerCapture if that element exists and
element.hasPointerCapture(e.pointerId) is true (then clear the saved
capturingElement). Also ensure dragState is cleared after release so you won't
try to release a stale element.
In `@src/hooks/useLongPress.ts`:
- Around line 18-25: The clear() function currently resets firedRef.current =
false (and is called from onTouchEnd/onTouchCancel/onTouchMove), which
incorrectly clears the flag for consumers that inspect it after gesture end;
remove the firedRef.current = false line from clear() so firedRef is only reset
at the start of onTouchStart; additionally add a useEffect cleanup that clears
timerRef (clearTimeout) on unmount to prevent the pending timeout from firing
after the component unmounts — reference the clear, firedRef, timerRef,
onTouchStart functions/refs and add the effect to tear down timerRef.current on
unmount.
In `@src/lib/modal/components/ModalFooter.tsx`:
- Around line 1-8: Add the missing import of the React type to files using
React.FC: in ModalFooter.tsx, Button.tsx, and Input.tsx add an import like
`import type React from "react"` at the top of each file so the React.FC type
references (e.g. the ModalFooter component declared as `export const
ModalFooter: React.FC<ModalFooterProps>`, and the Button and Input component
declarations) resolve correctly and match the rest of the codebase.
In `@src/lib/platformUtils.ts`:
- Around line 12-25: The functions isTauriPlatform and isMobilePlatform
currently perform an async dynamic import with .then() but return synchronously
(always false); change both to async functions that return Promise<boolean>, use
await import("@tauri-apps/plugin-os") (or the correct plugin) inside a try/catch
so rejected imports are handled, then return the result of comparing platform()
=== platformName (or the mobile checks) directly; update any callers to await
these functions or adapt to Promise<boolean>. Ensure you reference the exported
functions isTauriPlatform and isMobilePlatform when making the changes.
🟡 Minor comments (14)
.github/workflows/publish.yaml-116-117 (1)
116-117:⚠️ Potential issue | 🟡 MinorAdd validation to confirm AppImage update-information write succeeds.
The offset
33651is actually correct and standard for ISO9660-based AppImages (it points to the "Application Used" field in the ISO9660 Primary Volume Descriptor). This approach is not fragile or version-specific—it's the documented standard used by projects like Krita in production.However, there is no validation that the
ddwrite succeeded or that the data was written correctly. Consider adding a read-back verification step to confirm the embedding was successful, for example:# After the dd write written=$(dd if="$appimage" bs=1 skip=33651 count=512 2>/dev/null) if [ "$written" != "$(printf 'gh-releases-zsync|ObsidianIRC|ObsidianIRC|latest|ObsidianIRC_*_amd64.AppImage.zsync')" ]; then echo "Error: Failed to embed update info correctly" >&2 exit 1 fiThis applies to both occurrences (lines 116-117 and 175-176).
src/hooks/useLongPress.ts-9-68 (1)
9-68:⚠️ Potential issue | 🟡 MinorMissing cleanup on unmount — timer can fire after component unmounts.
If the component unmounts while the long-press timer is pending,
onLongPresswill be invoked on a stale/unmounted component. Add auseEffectcleanup to clear the timer on unmount.Proposed fix — add effect cleanup
Add at the end of the hook, before the return statement:
import { useCallback, useEffect, useRef } from "react"; // ... inside useLongPress, before the return: useEffect(() => { return () => { if (timerRef.current !== null) { clearTimeout(timerRef.current); } }; }, []);src/components/ui/UserProfileModal.tsx-227-232 (1)
227-232:⚠️ Potential issue | 🟡 MinorUnhandled rejection leaves the modal stuck open.
If
openExternalUrlthrows (e.g., Tauri plugin failure),setPendingUrl(null)on line 231 is never reached, leaving theExternalLinkWarningModalpermanently open. Wrap intry/finally.🔧 Proposed fix
const handleConfirmOpen = async () => { - if (pendingUrl) { - await openExternalUrl(pendingUrl); + try { + if (pendingUrl) { + await openExternalUrl(pendingUrl); + } + } finally { + setPendingUrl(null); } - setPendingUrl(null); };src/components/ui/LinkWrapper.tsx-84-89 (1)
84-89:⚠️ Potential issue | 🟡 MinorSame unhandled rejection risk as in
UserProfileModal.tsx.If
openExternalUrlrejects,setPendingUrl(null)is skipped and the warning modal stays open. Usetry/finallyfor resilience.🔧 Proposed fix
const handleConfirmOpen = async () => { - if (pendingUrl) { - await openExternalUrl(pendingUrl); + try { + if (pendingUrl) { + await openExternalUrl(pendingUrl); + } + } finally { + setPendingUrl(null); } - setPendingUrl(null); };src/index.css-708-735 (1)
708-735:⚠️ Potential issue | 🟡 Minor
user-dragis not a standard CSS property.Stylelint correctly flags
user-drag(lines 713 and 734) as unknown. Only the vendor-prefixed-webkit-user-dragis recognized by browsers. The unprefixeduser-draghas no effect anywhere and can be removed.🔧 Proposed fix
.draggable-item img, .draggable-item svg { pointer-events: none; -webkit-user-drag: none; - -moz-user-drag: none; - user-drag: none; }.draggable-item.is-dragging * { pointer-events: none; user-select: none; -webkit-user-drag: none; - -moz-user-drag: none; - user-drag: none; }Note:
-moz-user-dragwas also never a real Firefox property — Firefox never supported this. Only-webkit-user-dragis functional (in Chromium/Safari).src/components/message/LinkPreview.tsx-43-48 (1)
43-48:⚠️ Potential issue | 🟡 MinorUnhandled rejection in
handleConfirmOpencould leave the warning modal stuck open.If
openExternalUrlthrows (e.g., Tauri shell API fails),setShowWarningModal(false)on line 47 is never reached, leaving the modal permanently open.Suggested fix
const handleConfirmOpen = async () => { if (firstUrl) { - await openExternalUrl(firstUrl); + try { + await openExternalUrl(firstUrl); + } catch (e) { + console.error("Failed to open URL:", e); + } } setShowWarningModal(false); };src/App.tsx-88-95 (1)
88-95:⚠️ Potential issue | 🟡 MinorRemove unused destructured variables from the UI state object.
Lines 90 and 94 destructure
isUserProfileModalOpenandprefillServerDetailsfrom the store's UI state, but neither is used in App.tsx.
isUserProfileModalOpen: Not referenced anywhere in the component;UserProfileModalis driven by localuserProfileModalStateinstead.prefillServerDetails: Not used in App.tsx; theAddServerModalcomponent accesses it directly from the store (line 15 of AddServerModal.tsx), not from props.Remove both from the destructuring.
src/types/dragReorder.ts-7-14 (1)
7-14:⚠️ Potential issue | 🟡 MinorRemove unused
TouchDragStateinterface.The
TouchDragStateinterface is never imported or used anywhere in the codebase. TheuseDragReorderhook uses an internal ref with a different structure (hasMoved,dragOffset) that doesn't match this interface. Similarly,DragStateis also unused. Remove these unused type exports.src/lib/modal/components/Input.tsx-16-25 (1)
16-25:⚠️ Potential issue | 🟡 MinorLabel is not associated with the input — accessibility gap.
The
<label>and<input>are siblings inside a<div>, but there's nohtmlFor/idlinkage. This means clicking the label won't focus the input, and screen readers won't announce the association.Consider either wrapping the input inside the label or using
htmlFor/id:♿ Proposed fix using htmlFor
-export const Input: React.FC<InputProps> = ({ +export const Input: React.FC<InputProps> = ({ label, error, helperText, className = "", + id, ...props }) => ( <div className="space-y-2"> {label && ( - <label className="block text-discord-text-normal text-sm font-medium"> + <label htmlFor={id} className="block text-discord-text-normal text-sm font-medium"> {label} </label> )} <input + id={id} {...props} className={`w-full bg-discord-dark-400 text-discord-text-normal rounded px-3 py-2 focus:outline-none focus:ring-1 focus:ring-discord-primary ${className}`} />src/components/ui/AddPrivateChatModal.tsx-69-71 (1)
69-71:⚠️ Potential issue | 🟡 MinorStale
searchTermpersists across modal open/close cycles.Unlike the other migrated modals (e.g.,
InviteUserModal,ModerationModal), this component passesonClosedirectly toBaseModalwithout resettingsearchTerm. If the user types a search query and then dismisses the modal, the old query will still be visible when the modal is reopened.🐛 Proposed fix
+ const handleClose = () => { + setSearchTerm(""); + onClose(); + }; + return ( <BaseModal isOpen={isOpen} - onClose={onClose} + onClose={handleClose} title="Start Private Message" maxWidth="md" >Also update the Cancel button on line 122 to use
handleClose.src/components/ui/TopicModal.tsx-46-48 (1)
46-48:⚠️ Potential issue | 🟡 Minor
isEditingstate is not reset when the modal is closed via X / ESC / overlay click.If a user clicks "Edit Topic" and then dismisses the modal without saving or cancelling,
isEditingremainstrue. The next time the modal opens it will show the textarea in edit mode instead of the read-only view.🐛 Proposed fix — wrap `onClose` to reset editing state
+ const handleClose = () => { + setIsEditing(false); + setEditedTopic(channel.topic || ""); + onClose(); + }; + return ( <BaseModal isOpen={isOpen} - onClose={onClose} + onClose={handleClose} title="Channel Topic" maxWidth="md" >Also update the "Close" button on line 105 to use
handleClose.src/components/ui/ImagePreviewModal.tsx-43-45 (1)
43-45:⚠️ Potential issue | 🟡 MinorFile metadata displays poorly when
fileis null.When
fileis null, this renders asFile: undefined (0.0 KB). SincepreviewUrlcan be non-null whilefileis null (they're independent props), consider guarding the metadata line:Proposed fix
- <p className="text-sm text-discord-text-muted"> - File: {file?.name} ({((file?.size || 0) / 1024).toFixed(1)} KB) - </p> + {file && ( + <p className="text-sm text-discord-text-muted"> + File: {file.name} ({(file.size / 1024).toFixed(1)} KB) + </p> + )}src/components/message/MessageItem.tsx-304-310 (1)
304-310:⚠️ Potential issue | 🟡 MinorGuard against unhandled rejections when opening external URLs.
Line 307 awaitsopenExternalUrl(url). If it rejects, React will surface an unhandled promise. A small try/catch avoids noisy errors.🛠️ Proposed fix
- onClick={async (e) => { - e.preventDefault(); - await openExternalUrl(url); - }} + onClick={async (e) => { + e.preventDefault(); + try { + await openExternalUrl(url); + } catch (err) { + console.warn("Failed to open external URL:", err); + } + }}src/store/index.ts-357-360 (1)
357-360:⚠️ Potential issue | 🟡 MinorSync visibility flags when persisting sidebarPreferences.
updateSidebarPreferencespersistsisVisiblebut leavesui.isChannelListVisible/ui.isMemberListVisibleunchanged. If callers toggle visibility through this action, the UI won’t reflect the change until reload.🛠️ Suggested fix
return { ui: { ...state.ui, sidebarPreferences: newPrefs, + isChannelListVisible: newPrefs.channelList.isVisible, + isMemberListVisible: newPrefs.memberList.isVisible, }, };Also applies to: 668-671, 805-808, 820-823, 2520-2541
🧹 Nitpick comments (20)
src/hooks/useScrollToBottom.ts (1)
93-107: Consider reusingscrollToBottomto avoid duplicating scroll logic.The inline
container.scrollTop = container.scrollHeightduplicates whatscrollToBottomalready does. Reusing it keeps both paths in sync if the scroll behavior changes later (e.g., adding smooth scrolling).♻️ Suggested diff
- useEffect(() => { - const container = containerRef.current; - if (!container) return; - - const observer = new ResizeObserver(() => { - if (wasAtBottomRef.current) { - container.scrollTop = container.scrollHeight; - } - }); - - observer.observe(container); - return () => observer.disconnect(); - }, [containerRef, channelId]); + useEffect(() => { + const container = containerRef.current; + if (!container) return; + + const observer = new ResizeObserver(() => { + if (wasAtBottomRef.current) { + scrollToBottom(); + } + }); + + observer.observe(container); + return () => observer.disconnect(); + }, [containerRef, channelId, scrollToBottom]);src/components/ui/ReplyBadge.tsx (1)
16-21: Consider adding anaria-labelto the close button.The button contains only an icon (
FaTimes) with no text content, making it inaccessible to screen readers. This is especially relevant given the PR's move toward accessible naming (e.g., "Close modal" in BaseModal).♿ Suggested fix
<button - className="ml-auto flex-shrink-0 p-1 rounded hover:bg-discord-dark-300 text-discord-text-muted hover:text-discord-text-normal transition-colors" + className="ml-auto flex-shrink-0 p-1 rounded hover:bg-discord-dark-300 text-discord-text-muted hover:text-discord-text-normal transition-colors" + aria-label="Cancel reply" onClick={onClose} >src/index.css (1)
668-685: Stylelint: Use modern color function notation.The linter expects
rgb()with percentage alpha values instead ofrgba()with decimal alpha values (e.g.,rgba(255, 255, 255, 0.2)→rgb(255 255 255 / 20%)). This also applies to line 723.🔧 Proposed fix
textarea.resize-none { scrollbar-width: thin; - scrollbar-color: rgba(255, 255, 255, 0.2) transparent; + scrollbar-color: rgb(255 255 255 / 20%) transparent; }textarea.resize-none::-webkit-scrollbar-thumb { - background-color: rgba(255, 255, 255, 0.2); + background-color: rgb(255 255 255 / 20%); border-radius: 3px; }src/lib/ircUtils.tsx (1)
615-680: IRC and external link handling logic is correct.The expanded URL regex (line 620) and conditional rendering based on
isTauri()and link type are well-structured. IRC links render as plain text in web builds and as clickableirc-linkelements in Tauri — aligning with theLinkWrapper.tsxinterception logic.One subtlety:
fullUrlon line 641 produces a nonsensicalhttps://irc://...for IRC URLs, but this value is harmlessly unused since line 670 correctly choosesurloverfullUrlfor IRC links. Consider guarding line 641 to avoid future confusion:♻️ Optional clarity improvement
- const fullUrl = url.startsWith("http") ? url : `https://${url}`; + const fullUrl = url.startsWith("http") || isIrcLink ? url : `https://${url}`;tests/components/ChannelSettingsModal.test.tsx (1)
88-115: UI mock state missingdefaultUIExtensionsfields, unlike other updated tests.Other test files in this PR (e.g.,
nicknameRetry.test.ts,AppLayout.mobile-init.test.tsx) spread...defaultUIExtensionsinto their mockuistate to includetopicModalRequest,channelSettingsRequest, andinviteUserRequest. This file'screateMockStatedoesn't, which could cause issues ifChannelSettingsModal(or its children) accesses those fields.Consider importing and spreading
defaultUIExtensionshere for consistency:Suggested fix
+import { defaultUIExtensions } from "../fixtures/uiState";Then inside
createMockState'suiobject:linkSecurityWarnings: [], + ...defaultUIExtensions, },src/lib/modal/components/ModalBody.tsx (1)
9-18: Consider addingdisplayNamefor theforwardRefcomponent.React DevTools and some ESLint rules (e.g.,
react/display-name) expect adisplayNameonforwardRefcomponents for better debugging.Suggested fix
export const ModalBody = forwardRef<HTMLDivElement, ModalBodyProps>( ({ children, className = "", scrollable = false }, ref) => ( <div ref={ref} className={`p-6 ${scrollable ? "overflow-y-auto" : ""} ${className}`} > {children} </div> ), ); + +ModalBody.displayName = "ModalBody";src/App.tsx (1)
144-153: Redundant default fornickname.
parseIrcUrl(url, "user")already defaultsnickto"user", soparsed.nick || "user"on line 150 is always truthy. Harmless but redundant — same pattern on line 192.src/components/ui/QuickActions/context.ts (1)
47-56: Redundant user lookup — find the user once.
selectedChannel.users.find(...)is called twice with the same predicate. Extract to a local variable for clarity and efficiency.♻️ Proposed refactor
const isOperator = selectedChannel && currentUser - ? selectedChannel.users - .find((u) => u.username === currentUser.username) - ?.status?.includes("@") || - selectedChannel.users - .find((u) => u.username === currentUser.username) - ?.status?.includes("~") || - false + ? (() => { + const userStatus = selectedChannel.users.find( + (u) => u.username === currentUser.username, + )?.status; + return userStatus?.includes("@") || userStatus?.includes("~") || false; + })() : false;src/lib/modal/BaseModal.tsx (1)
88-96: Staticid="modal-title"could collide if multiple modals render simultaneously.If two
BaseModalinstances are ever open at the same time (e.g., a confirmation dialog over an existing modal), the duplicateid="modal-title"would violate the HTML spec and confuse screen readers viaaria-labelledby. Consider generating a unique ID per modal instance (e.g.,useId()from React 18).tests/components/layout/ChatHeader.memberButton.test.tsx (1)
102-120: Consider extracting the repeatedChatHeaderrender props into a helper.The same ~15 props are duplicated 6 times across the test file. A helper like
renderChatHeader(overrides)would reduce noise and make the tests easier to maintain.Also applies to: 139-157, 179-197, 217-235, 255-273, 283-301, 309-327
src/components/ui/ChannelRenameModal.tsx (1)
32-33: State is not reset when the modal reopens for a different channel.
newNameandreasonare initialized once at mount. If the component stays mounted across modal open/close cycles, stale values from previous rename attempts will persist. Consider adding a reset effect:Proposed fix
const [newName, setNewName] = useState(selectedChannel?.name || ""); const [reason, setReason] = useState(""); + + useEffect(() => { + if (ui.isChannelRenameModalOpen) { + setNewName(selectedChannel?.name || ""); + setReason(""); + } + }, [ui.isChannelRenameModalOpen, selectedChannel?.name]);This requires adding
useEffectto the existinguseStateimport on line 2.src/components/ui/ChannelSettingsModal.tsx (4)
6-9: Inconsistent import style for modal components.Other modals in this PR (e.g.,
ChannelRenameModal.tsx) import from the barrel"../../lib/modal/components", but this file imports each component individually. Consider using the barrel for consistency:Proposed fix
-import { Button } from "../../lib/modal/components/Button"; -import { Input } from "../../lib/modal/components/Input"; -import { ModalFooter } from "../../lib/modal/components/ModalFooter"; +import { Button, Input, ModalFooter } from "../../lib/modal/components";
134-134: Remove debugconsole.logstatements.Lines 134, 141–142, 150, and 162–166 contain debug logging (
"Parsed modes:","fetchChannelModes called","Sending MODE command","Updated channel data") that should be removed or downgraded before merging.Also applies to: 141-142, 150-150, 162-166
154-170: Duplicated setTimeout-based polling pattern.The "clear → send MODE → wait 500ms → re-request beI → wait 1000ms → parse" flow is copy-pasted across
fetchChannelModes,addMode,removeMode, andsaveEdit. This is fragile (hardcoded delays, no error handling on the re-fetch) and duplicated four times.Consider extracting a shared helper, e.g.:
const refetchModes = (delayBeforeRequest = 500, delayForResponse = 1000) => { setTimeout(() => { clearLists(); ircClient.sendRaw(serverId, `MODE ${channelName} +beI`); setTimeout(() => { const server = useStore.getState().servers.find((s) => s.id === serverId); const ch = server?.channels.find((c) => c.name === channelName); if (ch) parseChannelModes(ch); }, delayForResponse); }, delayBeforeRequest); };This would reduce the four occurrences to single calls and make the delays easier to tune.
Also applies to: 183-199, 211-231, 270-285
358-407: Tab badge counts are computed inline with repeated.filter()calls.Each tab button re-scans
modesto count items of its type. Consider pre-computing the counts:Proposed refactor
+ const modeCounts = useMemo(() => ({ + b: modes.filter((m) => m.type === "b").length, + e: modes.filter((m) => m.type === "e").length, + I: modes.filter((m) => m.type === "I").length, + }), [modes]);Then use
modeCounts.b,modeCounts.e,modeCounts.Iin the tab labels.src/components/layout/ChannelList.tsx (2)
496-520: Potential prop collision when spreadinggetItemPropsand overridingclassName/style.
channelDrag.getItemProps(channel.id)is spread first (line 499), thenclassNameandstyleare explicitly set (lines 500–520), which overrides whatevergetItemPropsreturns for those keys. You're compensating by manually re-merginggetItemProps(channel.id).classNameand.style, but this meansgetItemPropsis called three times per channel per render (lines 499, 510, 518).Consider destructuring once:
Proposed refactor
+ const dragProps = channelDrag.getItemProps(channel.id); <div onPointerMove={channelDrag.handlePointerMove} onPointerUp={channelDrag.handlePointerUp} - {...channelDrag.getItemProps(channel.id)} + {...dragProps} className={` group px-2 py-1 mb-1 rounded-md flex items-center justify-between ... - ${channelDrag.getItemProps(channel.id).className} + ${dragProps.className} `} style={{ "--bg-color": ..., - ...channelDrag.getItemProps(channel.id).style, + ...dragProps.style, } as React.CSSProperties}The same applies to
pmDrag.getItemProps(privateChat.id)(lines 816–847).
190-193:useEffectwithout dependency array runs on every render.This effect resets
clickedPMandlastSelectedPMafter every render. Note thatclickedPMstate is set here but never read in the JSX, making it effectively dead state. If the intent is just to resetlastSelectedPM.current, a deps-less effect is an unusual pattern—consider whether this is still needed or can be simplified.src/components/ui/QuickActions/uiActionConfig.tsx (1)
169-178: Dead branch ingetUIActionIcon.The
if (actionId === "toggle-notifications")block on line 173 returnsaction.icon, which is identical to the default return on line 177. This branch has no effect. Remove it or add the intended differentiation (e.g., dynamic icon based on mute state).Simplified version
export function getUIActionIcon(actionId: string): React.ReactNode { const action = UI_ACTIONS.find((a) => a.id === actionId); - if (!action) return null; - - if (actionId === "toggle-notifications") { - return action.icon; - } - - return action.icon; + return action?.icon ?? null; }src/components/mobile/MessageBottomSheet.tsx (1)
67-76: Consider usingFaTrashinstead ofFaTimesfor the Delete action icon.
FaTimes(✕) typically represents "close/cancel" whileFaTrash(🗑) is the conventional delete icon. Other parts of this PR (e.g.,ChannelList.tsx) useFaTrashfor delete actions consistently.src/components/layout/ChatArea.tsx (1)
439-462: Auto‑show on resize can override user intent.
Line 455 auto‑shows the member list when the view becomes wide, even if the user hid it manually. Consider tracking whether the list was auto‑hidden vs. user‑hidden and only restoring when you auto‑hid it.♻️ Suggested adjustment
- const prevIsTooNarrowRef = useRef(isTooNarrowForMemberList); + const prevIsTooNarrowRef = useRef(isTooNarrowForMemberList); + const autoHiddenMemberListRef = useRef(false); - if (wasNarrow && isNowWide && !isMemberListVisible) { - toggleMemberList(true); - } - else if (wasWide && isNowNarrow && isMemberListVisible) { - toggleMemberList(false); - } + if (wasWide && isNowNarrow && isMemberListVisible) { + toggleMemberList(false); + autoHiddenMemberListRef.current = true; + } else if (wasNarrow && isNowWide && autoHiddenMemberListRef.current) { + toggleMemberList(true); + autoHiddenMemberListRef.current = false; + }
| - name: Embed update information in AppImages | ||
| if: matrix.platform == 'ubuntu-22.04' | ||
| run: | | ||
| APPIMAGE_DIR="./src-tauri/target/release/bundle/appimage" | ||
| if [ -d "$APPIMAGE_DIR" ]; then | ||
| for appimage in $APPIMAGE_DIR/*_amd64.AppImage; do | ||
| if [ -f "$appimage" ]; then | ||
| echo "Embedding update info in $appimage" | ||
| printf 'gh-releases-zsync|ObsidianIRC|ObsidianIRC|latest|ObsidianIRC_*_amd64.AppImage.zsync' | \ | ||
| dd of="$appimage" bs=1 seek=33651 count=512 conv=notrunc | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| - name: Upload zsync files | ||
| if: matrix.platform == 'ubuntu-22.04' | ||
| uses: softprops/action-gh-release@v2 | ||
| with: | ||
| tag_name: ${{ needs.collect-version.outputs.tag_name }} | ||
| files: | | ||
| ./src-tauri/target/release/bundle/appimage/*.zsync |
There was a problem hiding this comment.
Embedded update info never reaches the published amd64 AppImage.
The tauri-action at line 83 both builds and uploads the AppImage to the release. By the time the "Embed update information" step (line 108) patches the local file, the unpatched AppImage has already been published. The "Upload zsync files" step only uploads .zsync files — not the modified AppImage.
Compare with the ARM flow (lines 159–193), where npm run tauri build only builds locally, then embed runs, and then softprops/action-gh-release publishes the final artifact — correct order.
To fix this for the amd64 path, you'd need to either:
- Re-upload the patched AppImage after embedding (add it to the
files:list in the upload step), or - Separate the tauri-action build from the release (use
tauriScript/ skip release upload), embed, then publish all artifacts together.
🤖 Prompt for AI Agents
In @.github/workflows/publish.yaml around lines 108 - 128, The published amd64
AppImage is uploaded before the "Embed update information in AppImages" step
runs, so your patch never reaches the release; modify the workflow so the
release action uploads the patched AppImage (or delay release until after
embedding). Specifically, either add the patched AppImage path
(./src-tauri/target/release/bundle/appimage/*_amd64.AppImage) to the files: list
of the softprops/action-gh-release step named "Upload zsync files", or change
the tauri-action invocation (the earlier "tauri-action" build step) to only
build (use tauriScript or disable automatic release upload) so that the "Embed
update information in AppImages" step runs before softprops/action-gh-release
publishes; ensure APPIMAGE_DIR and the "Embed update information in AppImages"
step still point to the same files so the uploaded artifacts include the
dd-patched AppImage.
| if (bypass) { | ||
| return children; | ||
| } | ||
|
|
||
| const [width, setWidth] = useState(defaultWidth); | ||
| const validateWidth = useCallback( | ||
| (width: number, min: number, max: number, fallback: number): number => { | ||
| if (Number.isNaN(width) || width < min || width > max) { | ||
| return fallback; | ||
| } | ||
| return width; | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Pre-existing rules-of-hooks violation: early return before hooks.
The if (bypass) return children on line 39 causes all hooks below it (including the new useCallback on line 43 and useState on line 53) to be skipped when bypass is true. If bypass ever toggles at runtime, React will throw due to inconsistent hook call order.
This is a pre-existing issue, but the PR adds more hooks after the conditional return, increasing the surface area. Consider moving the bypass check after all hooks, returning early from the JSX instead.
♻️ Proposed fix (outline)
}) => {
- if (bypass) {
- return children;
- }
-
const validateWidth = useCallback(/* ... */);
const [width, setWidth] = useState(/* ... */);
// ... all other hooks ...
+ if (bypass) {
+ return <>{children}</>;
+ }
+
return (
<div /* ... */>🤖 Prompt for AI Agents
In `@src/components/layout/ResizableSidebar.tsx` around lines 39 - 51, The early
return "if (bypass) return children" causes a rules-of-hooks violation because
it short-circuits hooks declared later (e.g., validateWidth useCallback and
subsequent useState calls) in the ResizableSidebar component; move the bypass
check so that all hooks (including validateWidth, any useState/useEffect calls)
are executed unconditionally at the top of the component, then perform the
bypass branch in the render return (e.g., return children JSX when bypass is
true) so hook order remains stable even if bypass toggles at runtime.
| const cleanHost = serverHost.replace( | ||
| /^(https?|wss?|ircs?|irc):\/\//, | ||
| "", | ||
| ); | ||
|
|
||
| // Check if this is a localhost connection (case insensitive) | ||
| const isLocalhost = | ||
| cleanHost.toLowerCase() === "localhost" || | ||
| cleanHost === "127.0.0.1" || | ||
| cleanHost === "::1"; | ||
|
|
||
| // Use ircs:// for SSL ports (typically 6697, 9999, etc.) or common SSL ports, but not for localhost | ||
| const isSSLPort = | ||
| !isLocalhost && | ||
| (port === 6697 || port === 9999 || port === 443 || port === 993); | ||
|
|
||
| finalHost = `${isSSLPort ? "ircs" : "irc"}://${cleanHost}:${port}`; | ||
| if (useWebSocket) { | ||
| finalHost = `${isSSLPort ? "wss" : "ws"}://${cleanHost}:${port}`; | ||
| } else { | ||
| finalHost = `${isSSLPort ? "ircs" : "irc"}://${cleanHost}:${port}`; | ||
| } |
There was a problem hiding this comment.
Avoid double-port host strings in Tauri mode.
Line 101 strips only the scheme, then Line 116/118 appends :${port}. If a user enters irc.example.com:6697, the final URL becomes irc.example.com:6697:6697, which breaks connections. Strip a trailing port (or parse the host) before appending.
🛠️ Proposed fix
- const cleanHost = serverHost.replace(
+ const cleanHost = serverHost.replace(
/^(https?|wss?|ircs?|irc):\/\//,
"",
);
+ const hostWithoutPort = cleanHost.replace(
+ /^(\[.*\]|[^:]+):\d+$/,
+ "$1",
+ );
- if (useWebSocket) {
- finalHost = `${isSSLPort ? "wss" : "ws"}://${cleanHost}:${port}`;
- } else {
- finalHost = `${isSSLPort ? "ircs" : "irc"}://${cleanHost}:${port}`;
- }
+ if (useWebSocket) {
+ finalHost = `${isSSLPort ? "wss" : "ws"}://${hostWithoutPort}:${port}`;
+ } else {
+ finalHost = `${isSSLPort ? "ircs" : "irc"}://${hostWithoutPort}:${port}`;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cleanHost = serverHost.replace( | |
| /^(https?|wss?|ircs?|irc):\/\//, | |
| "", | |
| ); | |
| // Check if this is a localhost connection (case insensitive) | |
| const isLocalhost = | |
| cleanHost.toLowerCase() === "localhost" || | |
| cleanHost === "127.0.0.1" || | |
| cleanHost === "::1"; | |
| // Use ircs:// for SSL ports (typically 6697, 9999, etc.) or common SSL ports, but not for localhost | |
| const isSSLPort = | |
| !isLocalhost && | |
| (port === 6697 || port === 9999 || port === 443 || port === 993); | |
| finalHost = `${isSSLPort ? "ircs" : "irc"}://${cleanHost}:${port}`; | |
| if (useWebSocket) { | |
| finalHost = `${isSSLPort ? "wss" : "ws"}://${cleanHost}:${port}`; | |
| } else { | |
| finalHost = `${isSSLPort ? "ircs" : "irc"}://${cleanHost}:${port}`; | |
| } | |
| const cleanHost = serverHost.replace( | |
| /^(https?|wss?|ircs?|irc):\/\//, | |
| "", | |
| ); | |
| const hostWithoutPort = cleanHost.replace( | |
| /^(\[.*\]|[^:]+):\d+$/, | |
| "$1", | |
| ); | |
| const isLocalhost = | |
| cleanHost.toLowerCase() === "localhost" || | |
| cleanHost === "127.0.0.1" || | |
| cleanHost === "::1"; | |
| const isSSLPort = | |
| !isLocalhost && | |
| (port === 6697 || port === 9999 || port === 443 || port === 993); | |
| if (useWebSocket) { | |
| finalHost = `${isSSLPort ? "wss" : "ws"}://${hostWithoutPort}:${port}`; | |
| } else { | |
| finalHost = `${isSSLPort ? "ircs" : "irc"}://${hostWithoutPort}:${port}`; | |
| } |
🤖 Prompt for AI Agents
In `@src/components/ui/AddServerModal.tsx` around lines 101 - 119, The code in
AddServerModal.tsx currently strips only the scheme from serverHost into
cleanHost then always appends :${port}, causing duplicates when the user
included a port (e.g., "irc.example.com:6697"). Update the logic that computes
cleanHost to also remove a trailing :<port> (or parse host:port) before
computing isLocalhost/isSSLPort and building finalHost; adjust references to
serverHost/cleanHost/port/useWebSocket/finalHost so you first normalize
serverHost by removing any scheme and any trailing :number, then decide
isLocalhost and isSSLPort and only append :port if the original input did not
already include a port.
| const dragState = useRef({ | ||
| startY: 0, | ||
| currentY: 0, | ||
| itemId: null as string | null, | ||
| initialIndex: -1, | ||
| hasMoved: false, | ||
| dragOffset: 0, | ||
| }); | ||
|
|
||
| const handlePointerDown = useCallback( | ||
| (e: React.PointerEvent, itemId: string) => { | ||
| if (disabled || e.button !== 0) return; | ||
|
|
||
| const target = e.currentTarget as HTMLElement; | ||
| target.setPointerCapture(e.pointerId); | ||
|
|
||
| const index = items.findIndex((item) => getItemId(item) === itemId); | ||
|
|
||
| dragState.current = { | ||
| startY: e.clientY, | ||
| currentY: e.clientY, | ||
| itemId, | ||
| initialIndex: index, | ||
| hasMoved: false, | ||
| dragOffset: 0, | ||
| }; | ||
| }, | ||
| [disabled, items, getItemId], | ||
| ); | ||
|
|
||
| const handlePointerMove = useCallback( | ||
| (e: React.PointerEvent) => { | ||
| if (disabled || !dragState.current.itemId) return; | ||
|
|
||
| const deltaY = Math.abs(e.clientY - dragState.current.startY); | ||
|
|
||
| if (!dragState.current.hasMoved && deltaY > 5) { | ||
| dragState.current.hasMoved = true; | ||
| setIsDragging(true); | ||
| setDraggedItemId(dragState.current.itemId); | ||
| } | ||
|
|
||
| if (dragState.current.hasMoved) { | ||
| dragState.current.currentY = e.clientY; | ||
| dragState.current.dragOffset = e.clientY - dragState.current.startY; | ||
|
|
||
| // Always find a drop target - no dead zones | ||
| const allItems = Array.from( | ||
| document.querySelectorAll<HTMLElement>("[data-draggable-item]"), | ||
| ); | ||
|
|
||
| if (allItems.length === 0) return; | ||
|
|
||
| let targetId: string | null = null; | ||
| let minDistance = Number.POSITIVE_INFINITY; | ||
|
|
||
| // Find which item zone the cursor is in | ||
| for (const item of allItems) { | ||
| const itemId = item.getAttribute("data-item-id"); | ||
| if (!itemId) continue; | ||
|
|
||
| const rect = item.getBoundingClientRect(); | ||
| const itemCenter = rect.top + rect.height / 2; | ||
| const distance = Math.abs(e.clientY - itemCenter); | ||
|
|
||
| if (distance < minDistance) { | ||
| minDistance = distance; | ||
| targetId = itemId; | ||
| } | ||
| } | ||
|
|
||
| // Always set a target (ensures continuous visual feedback) | ||
| if (targetId) { | ||
| setDragOverItemId(targetId); | ||
| } | ||
| } | ||
| }, | ||
| [disabled], | ||
| ); | ||
|
|
||
| const handlePointerUp = useCallback( | ||
| (e: React.PointerEvent) => { | ||
| if (disabled) return; | ||
|
|
||
| const target = e.currentTarget as HTMLElement; | ||
| target.releasePointerCapture(e.pointerId); | ||
|
|
||
| if ( | ||
| dragState.current.hasMoved && | ||
| dragState.current.itemId && | ||
| dragOverItemId | ||
| ) { | ||
| const draggedId = dragState.current.itemId; | ||
|
|
||
| if (draggedId !== dragOverItemId) { | ||
| const draggedIndex = items.findIndex( | ||
| (item) => getItemId(item) === draggedId, | ||
| ); | ||
| const targetIndex = items.findIndex( | ||
| (item) => getItemId(item) === dragOverItemId, | ||
| ); | ||
|
|
||
| if (draggedIndex !== -1 && targetIndex !== -1) { | ||
| const reordered = [...items]; | ||
| const [removed] = reordered.splice(draggedIndex, 1); | ||
| reordered.splice(targetIndex, 0, removed); | ||
| onReorder(reordered.map(getItemId)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| dragState.current = { | ||
| startY: 0, | ||
| currentY: 0, | ||
| itemId: null, | ||
| initialIndex: -1, | ||
| hasMoved: false, | ||
| dragOffset: 0, | ||
| }; |
There was a problem hiding this comment.
Ensure pointer capture is released from the capturing element.
Line 50 captures the pointer on the item, but Line 121 releases it on e.currentTarget. If handlePointerUp is attached to a container, this can throw NotFoundError and leave the pointer stuck. Store the capturing element and guard with hasPointerCapture before releasing.
🛠️ Proposed fix
- const dragState = useRef({
- startY: 0,
- currentY: 0,
- itemId: null as string | null,
- initialIndex: -1,
- hasMoved: false,
- dragOffset: 0,
- });
+ const dragState = useRef({
+ startY: 0,
+ currentY: 0,
+ itemId: null as string | null,
+ initialIndex: -1,
+ hasMoved: false,
+ dragOffset: 0,
+ captureEl: null as HTMLElement | null,
+ });
const target = e.currentTarget as HTMLElement;
target.setPointerCapture(e.pointerId);
dragState.current = {
startY: e.clientY,
currentY: e.clientY,
itemId,
initialIndex: index,
hasMoved: false,
dragOffset: 0,
+ captureEl: target,
};
- const target = e.currentTarget as HTMLElement;
- target.releasePointerCapture(e.pointerId);
+ const captureEl = dragState.current.captureEl;
+ if (captureEl?.hasPointerCapture(e.pointerId)) {
+ captureEl.releasePointerCapture(e.pointerId);
+ }
dragState.current = {
startY: 0,
currentY: 0,
itemId: null,
initialIndex: -1,
hasMoved: false,
dragOffset: 0,
+ captureEl: null,
};🤖 Prompt for AI Agents
In `@src/hooks/useDragReorder.ts` around lines 36 - 154, The pointer capture is
being set on the down event but released assuming e.currentTarget is the
capturer; update handlePointerDown to save the capturing element
(e.currentTarget) into dragState.current (e.g., capturingElement), and update
handlePointerUp to read that saved element and only call releasePointerCapture
if that element exists and element.hasPointerCapture(e.pointerId) is true (then
clear the saved capturingElement). Also ensure dragState is cleared after
release so you won't try to release a stale element.
| const clear = useCallback(() => { | ||
| if (timerRef.current !== null) { | ||
| clearTimeout(timerRef.current); | ||
| timerRef.current = null; | ||
| } | ||
| startPos.current = null; | ||
| firedRef.current = false; | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the complete useLongPress.ts file
cat -n src/hooks/useLongPress.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 2183
🏁 Script executed:
# Search for usages of firedRef and useLongPress to understand how consumers use it
rg -n 'firedRef|useLongPress' --type=ts --type=tsx -A 3Repository: ObsidianIRC/ObsidianIRC
Length of output: 94
🏁 Script executed:
# Search for usages of useLongPress without file type restriction
rg -n 'useLongPress' --type=ts -A 5 -B 2Repository: ObsidianIRC/ObsidianIRC
Length of output: 1638
🏁 Script executed:
# Also search for how firedRef is accessed by consumers
rg -n 'firedRef\.current' --type=ts -A 3 -B 3Repository: ObsidianIRC/ObsidianIRC
Length of output: 1456
🏁 Script executed:
# Let's see the full context of how SwipeableMessage uses firedRef
cat -n src/components/message/SwipeableMessage.tsx | head -80Repository: ObsidianIRC/ObsidianIRC
Length of output: 2556
🏁 Script executed:
# Check if there are any other places where firedRef is accessed
rg -n 'longPress\.firedRef|firedRef' src/components/message/SwipeableMessage.tsx -B 5 -A 5Repository: ObsidianIRC/ObsidianIRC
Length of output: 339
🏁 Script executed:
# Check if there's a useEffect cleanup or if timer could fire after unmount
rg -n 'useEffect|cleanup|unmount' src/hooks/useLongPress.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
# Check if onLongPress callback dependency is properly handled
# Look at the dependency arrays in the hook
cat -n src/hooks/useLongPress.ts | grep -A 2 -B 2 'useCallback'Repository: ObsidianIRC/ObsidianIRC
Length of output: 858
🏁 Script executed:
# Let's verify what happens if the component unmounts while timer is running
# Check how onLongPress callback is handled with stale closures
rg -n 'onLongPress' src/hooks/useLongPress.ts -B 2 -A 2Repository: ObsidianIRC/ObsidianIRC
Length of output: 520
🏁 Script executed:
# Let's check the actual touch event sequence by looking at what handlers SwipeableMessage attaches
cat -n src/components/message/SwipeableMessage.tsx | tail -30Repository: ObsidianIRC/ObsidianIRC
Length of output: 1026
clear() resets firedRef in an order-dependent way that could cause bugs with different consumer patterns.
The current implementation resets firedRef.current = false in clear(), which is called from onTouchEnd, onTouchCancel, and onTouchMove (if threshold exceeded). While SwipeableMessage specifically checks firedRef during onSwiping (during the gesture, before onTouchEnd fires), the API design is fragile: any consumer checking firedRef after touch ends or in a post-gesture handler would always see false, defeating the purpose of the flag.
Remove firedRef reset from clear() since it's already reset at the start of onTouchStart (line 29), which is the correct place for the next gesture cycle:
Proposed fix
const clear = useCallback(() => {
if (timerRef.current !== null) {
clearTimeout(timerRef.current);
timerRef.current = null;
}
startPos.current = null;
- firedRef.current = false;
}, []);Also add cleanup on unmount: If the component unmounts while the timer is running, the timeout callback will fire and call onLongPress() on an unmounted component. Add a useEffect to clear the timer on unmount.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const clear = useCallback(() => { | |
| if (timerRef.current !== null) { | |
| clearTimeout(timerRef.current); | |
| timerRef.current = null; | |
| } | |
| startPos.current = null; | |
| firedRef.current = false; | |
| }, []); | |
| const clear = useCallback(() => { | |
| if (timerRef.current !== null) { | |
| clearTimeout(timerRef.current); | |
| timerRef.current = null; | |
| } | |
| startPos.current = null; | |
| }, []); |
🤖 Prompt for AI Agents
In `@src/hooks/useLongPress.ts` around lines 18 - 25, The clear() function
currently resets firedRef.current = false (and is called from
onTouchEnd/onTouchCancel/onTouchMove), which incorrectly clears the flag for
consumers that inspect it after gesture end; remove the firedRef.current = false
line from clear() so firedRef is only reset at the start of onTouchStart;
additionally add a useEffect cleanup that clears timerRef (clearTimeout) on
unmount to prevent the pending timeout from firing after the component unmounts
— reference the clear, firedRef, timerRef, onTouchStart functions/refs and add
the effect to tear down timerRef.current on unmount.
| import type { ReactNode } from "react"; | ||
|
|
||
| interface ModalFooterProps { | ||
| children: ReactNode; | ||
| className?: string; | ||
| } | ||
|
|
||
| export const ModalFooter: React.FC<ModalFooterProps> = ({ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check tsconfig for React/JSX settings
fd tsconfig.json --max-depth 3 -type f | head -5Repository: ObsidianIRC/ObsidianIRC
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Check ModalFooter.tsx directly
cat src/lib/modal/components/ModalFooter.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 422
🏁 Script executed:
#!/bin/bash
# Check imports in other modal components
rg -n "import type React|import React" src/lib/modal/components/ | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if React is globally typed or available
rg -n "declare global|declare namespace" --type ts --type tsx | head -10Repository: ObsidianIRC/ObsidianIRC
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Find and read tsconfig.json
find . -maxdepth 3 -name "tsconfig.json" -exec cat {} \;Repository: ObsidianIRC/ObsidianIRC
Length of output: 619
🏁 Script executed:
#!/bin/bash
# Check how other modal components use React
ls -la src/lib/modal/components/ | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 483
🏁 Script executed:
#!/bin/bash
# Search for React imports more broadly
grep -r "import.*React" src/lib/modal/components/ --include="*.tsx" | head -10Repository: ObsidianIRC/ObsidianIRC
Length of output: 314
🏁 Script executed:
#!/bin/bash
# Check one specific component mentioned in the review
cat src/lib/modal/components/ModerationModal.tsx | head -15Repository: ObsidianIRC/ObsidianIRC
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Check Button.tsx to see if it uses React.FC
cat src/lib/modal/components/Button.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 1084
🏁 Script executed:
#!/bin/bash
# Check ModalBody.tsx
cat src/lib/modal/components/ModalBody.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 493
🏁 Script executed:
#!/bin/bash
# Search for React.FC usage in the modal components
grep -n "React.FC" src/lib/modal/components/*.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 337
🏁 Script executed:
#!/bin/bash
# Find where ModerationModal actually is
find . -name "ModerationModal.tsx" -type fRepository: ObsidianIRC/ObsidianIRC
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check ModerationModal.tsx to see how it imports React
head -15 ./src/components/ui/ModerationModal.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 431
🏁 Script executed:
#!/bin/bash
# Check Input.tsx as well since it also uses React.FC
cat src/lib/modal/components/Input.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 904
🏁 Script executed:
#!/bin/bash
# Look for type declaration files that might provide global React
find src -name "*.d.ts" -type fRepository: ObsidianIRC/ObsidianIRC
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Check if there are any global type declarations
grep -r "declare global\|declare namespace React" src --include="*.ts" --include="*.tsx" --include="*.d.ts"Repository: ObsidianIRC/ObsidianIRC
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check vite config to see if there's any type extension
find . -name "vite.config.*" -type f | head -1 | xargs cat 2>/dev/null | head -50Repository: ObsidianIRC/ObsidianIRC
Length of output: 2195
🏁 Script executed:
#!/bin/bash
# Check package.json for type definitions
cat package.json | grep -A 10 '"types"'Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check vite-env.d.ts
cat src/vite-env.d.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 453
🏁 Script executed:
#!/bin/bash
# Check if there's a react global declaration in any file
grep -r "declare.*React" src --include="*.d.ts"Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Try to understand the overall pattern - check all files that use React.FC
grep -r "React\.FC" src --include="*.tsx" -B 5 | head -80Repository: ObsidianIRC/ObsidianIRC
Length of output: 4529
🏁 Script executed:
#!/bin/bash
# Check package.json for `@types/react`
cat package.json | grep -E "react|@types" | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 639
🏁 Script executed:
#!/bin/bash
# Check if there are any typeRoots or global type includes in tsconfig
cat tsconfig.json | grep -E "typeRoots|types"Repository: ObsidianIRC/ObsidianIRC
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Try to find if React is made available globally somewhere
find . -path ./node_modules -prune -o -type f \( -name "*.d.ts" -o -name "*.ts" -o -name "*.tsx" \) -exec grep -l "declare.*React\|global.*React" {} \; 2>/dev/null | grep -v node_modulesRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if the project actually builds/compiles with these files
# Look for build output or any compilation errors
if [ -f "package.json" ]; then
# Check the build command
grep -A 2 '"build"' package.json
fiRepository: ObsidianIRC/ObsidianIRC
Length of output: 201
🏁 Script executed:
#!/bin/bash
# Check if there's a .eslintrc or similar that might affect type resolution
find . -maxdepth 2 -name ".eslintrc*" -o -name "eslint.config.*" | head -5 | xargs cat 2>/dev/nullRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check a few files that use React.FC to see if they import React
head -5 src/App.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Check AppLayout.tsx
head -5 src/components/layout/AppLayout.tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Check if the project type-checks successfully
# Look for any recent type errors or build issues in git
git log --oneline -20 2>/dev/null | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Check if there are any type errors reported in the actual build
# by examining if tsc passes
cd . && tsc --noEmit 2>&1 | head -50Repository: ObsidianIRC/ObsidianIRC
Length of output: 458
🏁 Script executed:
#!/bin/bash
# Check if tsc specifically complains about React not being in scope for React.FC
tsc --noEmit 2>&1 | grep -i "react\|ModalFooter\|Button\|Input" | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
🏁 Script executed:
#!/bin/bash
# List all files using React.FC and check their imports
files=$(grep -r "React\.FC" src --include="*.tsx" -l)
for file in $files; do
echo "=== $file ==="
head -10 "$file"
echo ""
doneRepository: ObsidianIRC/ObsidianIRC
Length of output: 23306
Missing React import for React.FC type reference across three component files.
Three modal components use React.FC without importing React: ModalFooter.tsx, Button.tsx, and Input.tsx. Every other component in the codebase (50+) that uses React.FC explicitly imports React via import type React from "react". Add the import to maintain consistency.
🐛 Proposed fix
For each of the three files, add the React import:
+import type React from "react";
import type { ReactNode } from "react";Apply to:
src/lib/modal/components/ModalFooter.tsxsrc/lib/modal/components/Button.tsxsrc/lib/modal/components/Input.tsx
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { ReactNode } from "react"; | |
| interface ModalFooterProps { | |
| children: ReactNode; | |
| className?: string; | |
| } | |
| export const ModalFooter: React.FC<ModalFooterProps> = ({ | |
| import type React from "react"; | |
| import type { ReactNode } from "react"; | |
| interface ModalFooterProps { | |
| children: ReactNode; | |
| className?: string; | |
| } | |
| export const ModalFooter: React.FC<ModalFooterProps> = ({ |
🤖 Prompt for AI Agents
In `@src/lib/modal/components/ModalFooter.tsx` around lines 1 - 8, Add the missing
import of the React type to files using React.FC: in ModalFooter.tsx,
Button.tsx, and Input.tsx add an import like `import type React from "react"` at
the top of each file so the React.FC type references (e.g. the ModalFooter
component declared as `export const ModalFooter: React.FC<ModalFooterProps>`,
and the Button and Input component declarations) resolve correctly and match the
rest of the codebase.
| export const isTauriPlatform = (platformName: "android" | "ios"): boolean => { | ||
| if (!isTauri()) return false; | ||
|
|
||
| try { | ||
| // Dynamic import to avoid bundling Tauri APIs in web builds | ||
| import("@tauri-apps/plugin-os").then(({ platform }) => { | ||
| return platform() === platformName; | ||
| }); | ||
| } catch { | ||
| return false; | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the exact code at lines 12-25
head -50 src/lib/platformUtils.ts | cat -nRepository: ObsidianIRC/ObsidianIRC
Length of output: 1251
🏁 Script executed:
# Search for all usages of these functions
rg -n 'isTauriPlatform|isMobilePlatform' --type ts --type tsx -A 2 -B 2Repository: ObsidianIRC/ObsidianIRC
Length of output: 94
🏁 Script executed:
# Also check if these functions are exported and used in other files
rg -n 'from.*platformUtils|import.*platformUtils' --type ts --type tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 94
🏁 Script executed:
# Search for usages without the problematic tsx filter
rg -n 'isTauriPlatform' --type ts -A 2 -B 2Repository: ObsidianIRC/ObsidianIRC
Length of output: 322
🏁 Script executed:
# Search for isMobilePlatform usages
rg -n 'isMobilePlatform' --type ts -A 2 -B 2Repository: ObsidianIRC/ObsidianIRC
Length of output: 292
🏁 Script executed:
# Look for imports from platformUtils
rg -n 'from.*platformUtils|import.*platformUtils' --type ts -A 2 -B 2Repository: ObsidianIRC/ObsidianIRC
Length of output: 2573
🏁 Script executed:
# Comprehensive search - check if these functions are called anywhere, even in comments or strings
rg -i 'isTauriPlatform|isMobilePlatform' src/ --type tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 244
🏁 Script executed:
# Check if they're exported and re-exported anywhere
rg 'export.*isTauriPlatform|export.*isMobilePlatform|isTauriPlatform|isMobilePlatform' src/Repository: ObsidianIRC/ObsidianIRC
Length of output: 244
isTauriPlatform and isMobilePlatform always return false — async result is discarded.
Both functions declare synchronous return type boolean but use import().then() pattern. The return statement inside the .then() callback (lines 18 and 33) returns only from the callback, not from the main function. Control synchronously falls through to return false at the end, causing these functions to always return false when isTauri() is true.
Additionally, try/catch cannot catch rejected promises from import() — only synchronous exceptions.
These functions are currently unused (dead code), but they are exported and broken. They must either be converted to async (returning Promise<boolean>) or removed entirely.
🐛 Proposed fix (make async)
-export const isTauriPlatform = (platformName: "android" | "ios"): boolean => {
+export const isTauriPlatform = async (platformName: "android" | "ios"): Promise<boolean> => {
if (!isTauri()) return false;
try {
- // Dynamic import to avoid bundling Tauri APIs in web builds
- import("@tauri-apps/plugin-os").then(({ platform }) => {
- return platform() === platformName;
- });
+ const { platform } = await import("@tauri-apps/plugin-os");
+ return platform() === platformName;
} catch {
return false;
}
-
- return false;
};
-export const isMobilePlatform = (): boolean => {
+export const isMobilePlatform = async (): Promise<boolean> => {
if (!isTauri()) return false;
try {
- import("@tauri-apps/plugin-os").then(({ platform }) => {
- const currentPlatform = platform();
- return currentPlatform === "android" || currentPlatform === "ios";
- });
+ const { platform } = await import("@tauri-apps/plugin-os");
+ const currentPlatform = platform();
+ return currentPlatform === "android" || currentPlatform === "ios";
} catch {
return false;
}
-
- return false;
};🤖 Prompt for AI Agents
In `@src/lib/platformUtils.ts` around lines 12 - 25, The functions isTauriPlatform
and isMobilePlatform currently perform an async dynamic import with .then() but
return synchronously (always false); change both to async functions that return
Promise<boolean>, use await import("@tauri-apps/plugin-os") (or the correct
plugin) inside a try/catch so rejected imports are handled, then return the
result of comparing platform() === platformName (or the mobile checks) directly;
update any callers to await these functions or adapt to Promise<boolean>. Ensure
you reference the exported functions isTauriPlatform and isMobilePlatform when
making the changes.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/layout/ChatArea.tsx`:
- Line 1684: The className template in ChatArea's JSX injects the literal
"false" when isNarrowView is true; change the expression to produce an empty
string instead of false (e.g., replace `${!isNarrowView && "px-4"} pb-4
relative` with a conditional that returns `"px-4"` or `""`, or build classes via
an array/filter/join or a classnames helper) so that isNarrowView and the
surrounding div no longer render a "false" class.
In `@src/hooks/useMessageSending.ts`:
- Around line 278-281: The raw IRC commands built in the ircClient.sendRaw calls
(using messagePrefix + " PRIVMSG ...") produce a leading space when
messagePrefix is empty; change the concatenation to first compute a
prefixWithSpace = messagePrefix ? `${messagePrefix} ` : '' (or equivalent) and
use `${prefixWithSpace}PRIVMSG ${target} :${line}` for the sendRaw calls (the
one calling ircClient.sendRaw and the other similar sendRaw invocations that
build PRIVMSG), and apply the same prefixWithSpace fix to the other occurrences
mentioned so no leading space is emitted when messagePrefix is empty.
- Around line 130-154: The code uses
localReplyTo.tags?.["draft/channel-context"] which is string | undefined and is
passed to ircClient.sendWhisper (expects string); fix by resolving and narrowing
the context before calling sendWhisper: compute a local variable (e.g., ctx)
from localReplyTo?.tags?.["draft/channel-context"] ||
localReplyTo?.tags?.["+draft/channel-context"], return early if ctx is falsy,
and then call ircClient.sendWhisper with ctx (a string); apply the same pattern
or extract a helper like getWhisperContext(replyTo) that returns {
channelContext, whisperTarget } | null and use it in the blocks around
localReplyTo/isWhisperReply and sendMultilineMessage/sendWhisper calls to ensure
channelContext is typed as string.
🧹 Nitpick comments (4)
src/hooks/useMessageSending.ts (1)
238-262: Whisper-reply handling is duplicated across three functions.The whisper-context detection and dispatch logic at lines 130–153, 238–262, and 322–347 is nearly identical. Consider extracting it into a shared helper (e.g.,
trySendWhisperReply) that each function calls before falling through to its normal path. This would also centralize the fix for the TS errors.src/components/layout/ChatArea.tsx (3)
443-470: Effect includesisMemberListVisiblein deps but also mutates it — fragile but safe.The
useEffectat line 443 callstoggleMemberList(true/false)which changesisMemberListVisible, one of its own dependencies. This works becauseprevIsTooNarrowRefis updated before the next run, preventing a cascade. However, this coupling is fragile — a future refactor could easily introduce an infinite loop or stale-ref bug.Consider removing
isMemberListVisiblefrom the dependency array (with a lint suppression comment) and reading it from the store directly inside the effect, or restructuring so the effect only reacts toisTooNarrowForMemberListtransitions.
1515-1523: Member list overlay completely replaces the messages area.When
showMemberListOverlayistrue, the entire messages area (including input, typing indicator, etc.) is hidden and replaced by the member list. This means on a narrow desktop (≤1080px), toggling the member list makes all messages and the input disappear. Is this intentional? A side-panel or modal overlay might be less disruptive than replacing the primary content area.
1584-1668:channelMessages.findIndexon every message could be costly for large histories.For each single-message group in
eventGroups,channelMessages.findIndex(m => m.id === message.id)(line 1617) does a linear scan. With large channel histories and many groups, this is O(n × m). Consider building aMap<string, number>(id → index) fromchannelMessagesonce viauseMemo, then performing O(1) lookups.
| onClose={() => setLocalReplyTo(null)} | ||
| {/* Input area */} | ||
| {(selectedChannel || selectedPrivateChat) && ( | ||
| <div className={`${!isNarrowView && "px-4"} pb-4 relative`}> |
There was a problem hiding this comment.
Bug: "false" string injected into className when isNarrowView is true.
The expression `${!isNarrowView && "px-4"} pb-4 relative` evaluates to "false pb-4 relative" when isNarrowView is true, because false && "px-4" yields false, which is then stringified by the template literal. This inserts the literal CSS class "false" into the DOM.
Proposed fix
- <div className={`${!isNarrowView && "px-4"} pb-4 relative`}>
+ <div className={`${!isNarrowView ? "px-4" : ""} pb-4 relative`}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={`${!isNarrowView && "px-4"} pb-4 relative`}> | |
| <div className={`${!isNarrowView ? "px-4" : ""} pb-4 relative`}> |
🤖 Prompt for AI Agents
In `@src/components/layout/ChatArea.tsx` at line 1684, The className template in
ChatArea's JSX injects the literal "false" when isNarrowView is true; change the
expression to produce an empty string instead of false (e.g., replace
`${!isNarrowView && "px-4"} pb-4 relative` with a conditional that returns
`"px-4"` or `""`, or build classes via an array/filter/join or a classnames
helper) so that isNarrowView and the surrounding div no longer render a "false"
class.
| const isWhisperReply = | ||
| localReplyTo && | ||
| (localReplyTo.tags?.["draft/channel-context"] || | ||
| localReplyTo.tags?.["+draft/channel-context"]); | ||
|
|
||
| if (isWhisperReply) { | ||
| const channelContext = | ||
| localReplyTo.tags?.["draft/channel-context"] || | ||
| localReplyTo.tags?.["+draft/channel-context"]; | ||
| const whisperTarget = localReplyTo.userId; | ||
|
|
||
| lines.forEach((line) => { | ||
| const formattedLine = formatMessageForIrc(line, { | ||
| color: selectedColor || "inherit", | ||
| formatting: selectedFormatting, | ||
| }); | ||
| ircClient.sendWhisper( | ||
| selectedServerId, | ||
| whisperTarget, | ||
| channelContext, | ||
| formattedLine, | ||
| ); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
channelContext is string | undefined, causing the TS2345 build failure (line 149).
The pipeline fails here because localReplyTo.tags?.["draft/channel-context"] evaluates to string | undefined, and sendWhisper expects string. The same issue recurs at lines 257 and 336.
Since this pattern is duplicated three times, consider extracting a helper that resolves the whisper context and returns early if it's absent:
Proposed fix (applied to all three locations)
For sendMultilineMessage (and similarly for the other two):
- const isWhisperReply =
- localReplyTo &&
- (localReplyTo.tags?.["draft/channel-context"] ||
- localReplyTo.tags?.["+draft/channel-context"]);
-
- if (isWhisperReply) {
- const channelContext =
- localReplyTo.tags?.["draft/channel-context"] ||
- localReplyTo.tags?.["+draft/channel-context"];
- const whisperTarget = localReplyTo.userId;
+ const channelContext =
+ localReplyTo?.tags?.["draft/channel-context"] ||
+ localReplyTo?.tags?.["+draft/channel-context"];
+
+ if (channelContext) {
+ const whisperTarget = localReplyTo.userId;This way channelContext is narrowed to string inside the if block, resolving the TS error. Apply the same pattern at lines 238–247 and 322–331.
Alternatively, extract a small helper to eliminate the triplication entirely:
function getWhisperContext(replyTo: Message | null) {
const ctx =
replyTo?.tags?.["draft/channel-context"] ||
replyTo?.tags?.["+draft/channel-context"];
if (!ctx) return null;
return { channelContext: ctx, whisperTarget: replyTo!.userId };
}🧰 Tools
🪛 GitHub Actions: Deploy to Cloudflare Pages
[error] 149-149: TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'. Type 'undefined' is not assignable to type 'string'.
🤖 Prompt for AI Agents
In `@src/hooks/useMessageSending.ts` around lines 130 - 154, The code uses
localReplyTo.tags?.["draft/channel-context"] which is string | undefined and is
passed to ircClient.sendWhisper (expects string); fix by resolving and narrowing
the context before calling sendWhisper: compute a local variable (e.g., ctx)
from localReplyTo?.tags?.["draft/channel-context"] ||
localReplyTo?.tags?.["+draft/channel-context"], return early if ctx is falsy,
and then call ircClient.sendWhisper with ctx (a string); apply the same pattern
or extract a helper like getWhisperContext(replyTo) that returns {
channelContext, whisperTarget } | null and use it in the blocks around
localReplyTo/isWhisperReply and sendMultilineMessage/sendWhisper calls to ensure
channelContext is typed as string.
| ircClient.sendRaw( | ||
| selectedServerId, | ||
| `${localReplyTo ? `@+draft/reply=${localReplyTo.msgid};` : ""} PRIVMSG ${target} :${line}`, | ||
| `${messagePrefix} PRIVMSG ${target} :${line}`, | ||
| ); |
There was a problem hiding this comment.
Leading space in raw IRC command when there's no reply prefix.
When messagePrefix is "" (no reply-to), the template literal produces " PRIVMSG ..." with a leading space. Same issue on line 295 and line 344.
Proposed fix
- `${messagePrefix} PRIVMSG ${target} :${line}`,
+ `${messagePrefix}PRIVMSG ${target} :${line}`,And adjust the prefix to include a trailing space when non-empty:
- const messagePrefix = localReplyTo
- ? `@+draft/reply=${localReplyTo.msgid};`
- : "";
+ const messagePrefix = localReplyTo
+ ? `@+draft/reply=${localReplyTo.msgid}; `
+ : "";Apply the same fix at line 344:
- `${localReplyTo ? `@+draft/reply=${localReplyTo.msgid};` : ""} PRIVMSG ${target} :${line}`,
+ `${localReplyTo ? `@+draft/reply=${localReplyTo.msgid}; ` : ""}PRIVMSG ${target} :${line}`,🤖 Prompt for AI Agents
In `@src/hooks/useMessageSending.ts` around lines 278 - 281, The raw IRC commands
built in the ircClient.sendRaw calls (using messagePrefix + " PRIVMSG ...")
produce a leading space when messagePrefix is empty; change the concatenation to
first compute a prefixWithSpace = messagePrefix ? `${messagePrefix} ` : '' (or
equivalent) and use `${prefixWithSpace}PRIVMSG ${target} :${line}` for the
sendRaw calls (the one calling ircClient.sendRaw and the other similar sendRaw
invocations that build PRIVMSG), and apply the same prefixWithSpace fix to the
other occurrences mentioned so no leading space is emitted when messagePrefix is
empty.
matheusfillipe
force-pushed
the
fix/more-polishment
branch
from
62dfe30 to
615f127
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/layout/ChannelList.tsx (1)
421-421:⚠️ Potential issue | 🟡 MinorBug:
"false"string injected into className whenisNarrowViewis true.
`${!isNarrowView && "opacity-0 group-hover:opacity-100"}`evaluates to"false"(a literal CSS class) whenisNarrowViewistrue. Same pattern on line 748.Proposed fix
- className={`ml-auto ${!isNarrowView && "opacity-0 group-hover:opacity-100"} cursor-pointer`} + className={`ml-auto ${!isNarrowView ? "opacity-0 group-hover:opacity-100" : ""} cursor-pointer`}Apply the same fix on line 748:
- className={`ml-auto ${!isNarrowView && "opacity-0 group-hover:opacity-100"} cursor-pointer`} + className={`ml-auto ${!isNarrowView ? "opacity-0 group-hover:opacity-100" : ""} cursor-pointer`}src/store/index.ts (1)
2482-2487:⚠️ Potential issue | 🟠 Major
toggleUserProfileModalflips the settings modal instead of the profile modal.This toggles
isSettingsModalOpen, leavingisUserProfileModalOpenunchanged, so the profile modal won’t open and settings may flicker.🐛 Suggested fix
toggleUserProfileModal: (isOpen) => { set((state) => ({ ui: { ...state.ui, - isSettingsModalOpen: - isOpen !== undefined ? isOpen : !state.ui.isSettingsModalOpen, + isUserProfileModalOpen: + isOpen !== undefined ? isOpen : !state.ui.isUserProfileModalOpen, }, })); },
🤖 Fix all issues with AI agents
In `@src/App.tsx`:
- Around line 144-152: handleIrcLinkClick currently forces useWebSocket: false
which breaks irc:// and ircs:// deep links for ObsidianIRC; update
handleIrcLinkClick (and the duplicate logic around lines 190-196) to set
useWebSocket: true for deep links or, alternatively, detect parsed.port from
parseIrcUrl and map traditional IRC ports (e.g., 6667, 6697) to WebSocket
defaults (e.g., 8000 or 443) before calling toggleAddServerModal so the
AddServerModal receives the correct useWebSocket flag and a
WebSocket-appropriate port.
In `@src/store/index.ts`:
- Around line 2586-2601: The call to saveUISelections(...) overwrites the
persisted object and drops fields like lastSelection; update each call site (the
block using get() / newState.ui and the two other similar blocks) to first read
the existing persisted UI selections (the current saved object) and merge it
with the new data, e.g. spread the persisted object into the argument to
saveUISelections so existing keys such as lastSelection are preserved while
updating selectedServerId, perServerSelections, and sidebarPreferences (the
memberList/channelList changes derived from newState.ui.isMemberListVisible
etc.). Ensure you reference saveUISelections, get(), newState.ui,
sidebarPreferences, selectedServerId, perServerSelections, and lastSelection
when applying the merge at all three locations.
- Around line 2689-2707: The modal toggle mutations toggleTopicModal and
toggleUserProfileModalWithContext are writing UI keys that don't exist
(isTopicModalOpen, topicModalContext, userProfileModalContext); update them to
use the current UIState keys instead: for toggleTopicModal set
ui.topicModalRequest = isOpen && context ? context : null and
ui.isUserProfileModalOpen remains unchanged (or set appropriately only in the
user profile toggle), and for toggleUserProfileModalWithContext set
ui.profileViewRequest = isOpen && context ? context : null and
ui.isUserProfileModalOpen = isOpen; keep the existing state.ui spread and
replace the non-existent properties with topicModalRequest/profileViewRequest
and isUserProfileModalOpen so the UIState updates are effective.
- Around line 824-833: The current initialization reads
loadUISelections().sidebarPreferences?.memberList.isVisible and
.channelList.isVisible which can throw if memberList or channelList are missing;
update the isMemberListVisible and isChannelListVisible initializers to safely
optional-chain through memberList and channelList (e.g., use
sidebarPreferences?.memberList?.isVisible and
sidebarPreferences?.channelList?.isVisible) and keep the existing nullish
fallback defaults so missing/partial payloads don't cause runtime errors when
calling loadUISelections().
🧹 Nitpick comments (2)
src/components/layout/ChannelList.tsx (2)
496-520:getItemPropscalled three times per channel item — destructure once.
channelDrag.getItemProps(channel.id)is invoked on lines 499, 510, and 518 for the same item in the same render cycle. Each call creates a fresh object. Destructure once to avoid redundant computation and potential subtle inconsistencies.♻️ Suggested refactor
+ {(() => { + const dragProps = channelDrag.getItemProps(channel.id); + return ( <div onPointerMove={channelDrag.handlePointerMove} onPointerUp={channelDrag.handlePointerUp} - {...channelDrag.getItemProps(channel.id)} + {...dragProps} className={` group px-2 py-1 mb-1 rounded-md flex items-center justify-between transition-all duration-200 ease-in-out shadow-sm ${ selectedChannelId === channel.id ? "bg-black text-white" : "bg-discord-dark-400/50 hover:bg-discord-primary/70 hover:text-white" } - ${channelDrag.getItemProps(channel.id).className} + ${dragProps.className} `} style={ { "--bg-color": selectedChannelId === channel.id ? "#000" : "rgba(47, 49, 54, 0.5)", - ...channelDrag.getItemProps(channel.id).style, + ...dragProps.style, } as React.CSSProperties } onClick={() => selectChannel(channel.id, { navigate: true }) } > + {/* ... children ... */} + </div> + ); + })()}The same issue applies to
pmDrag.getItemProps(privateChat.id)at lines 817, 823, and 846.
315-328: Drag hooks created unconditionally — consider disabling on mobile.Both
channelDragandpmDragare initialized without adisabledflag. On mobile (isNarrowView), the context menus are already disabled and actions are handled via buttons, but pointer events from the drag hook are still wired up (lines 497–498). This could cause accidental drag-start gestures competing with scroll on touch devices.♻️ Suggested change
const channelDrag = useDragReorder({ items: sortedChannels.filter((c) => !c.isPrivate), getItemId: (c) => c.id, onReorder: (ids) => selectedServerId && reorderChannels(selectedServerId, ids), + disabled: isNarrowView, }); const pmDrag = useDragReorder({ items: sortedPrivateChats, getItemId: (pm) => pm.id, onReorder: (ids) => selectedServerId && reorderPrivateChats(selectedServerId, ids), + disabled: isNarrowView, });
| const handleIrcLinkClick = (url: string) => { | ||
| // For now, just log. Could be extended to handle IRC links | ||
| console.log("IRC link clicked:", url); | ||
| const parsed = parseIrcUrl(url, "user"); | ||
| toggleAddServerModal(true, { | ||
| name: parsed.host, | ||
| host: parsed.host, | ||
| port: parsed.port.toString(), | ||
| nickname: parsed.nick || "user", | ||
| useWebSocket: false, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for useWebSocket usage to understand its semantics
rg -n "useWebSocket" -A 3 -B 1Repository: ObsidianIRC/ObsidianIRC
Length of output: 3652
🏁 Script executed:
#!/bin/bash
# Look for connection/server config types and how transport is handled
rg -n "interface ConnectionDetails|interface ServerConfig|type.*Server" -A 5Repository: ObsidianIRC/ObsidianIRC
Length of output: 6881
🏁 Script executed:
#!/bin/bash
# Check how the flag is used in connection establishment
rg -n "useWebSocket.*true|useWebSocket.*false" -B 2 -A 2Repository: ObsidianIRC/ObsidianIRC
Length of output: 1324
Set useWebSocket: true for IRC deep links, or handle port conversion to WebSocket defaults.
When processing irc:// and ircs:// deep links, setting useWebSocket: false forces traditional IRC protocol (ports 6667/6697). Since ObsidianIRC uses WebSocket-based connections with ports 443/8000, this creates a protocol mismatch. Either set useWebSocket: true and let AddServerModal auto-adjust the port to 443, or add logic to detect traditional IRC ports and convert them to WebSocket equivalents during parsing.
Also applies to: 190-196
🤖 Prompt for AI Agents
In `@src/App.tsx` around lines 144 - 152, handleIrcLinkClick currently forces
useWebSocket: false which breaks irc:// and ircs:// deep links for ObsidianIRC;
update handleIrcLinkClick (and the duplicate logic around lines 190-196) to set
useWebSocket: true for deep links or, alternatively, detect parsed.port from
parseIrcUrl and map traditional IRC ports (e.g., 6667, 6697) to WebSocket
defaults (e.g., 8000 or 443) before calling toggleAddServerModal so the
AddServerModal receives the correct useWebSocket flag and a
WebSocket-appropriate port.
| isMemberListVisible: | ||
| // Only restore member list on startup if the window is wide enough for the sidebar. | ||
| // At intermediate widths the member list replaces the chat area which is bad UX on startup. | ||
| typeof window !== "undefined" && | ||
| !window.matchMedia("(max-width: 1080px)").matches | ||
| ? (loadUISelections().sidebarPreferences?.memberList.isVisible ?? true) | ||
| : false, | ||
| isChannelListVisible: | ||
| loadUISelections().sidebarPreferences?.channelList.isVisible ?? true, | ||
| isChannelListModalOpen: false, |
There was a problem hiding this comment.
Guard against partial sidebarPreferences payloads.
sidebarPreferences?.memberList.isVisible will throw if memberList is missing (e.g., partial/legacy localStorage). Safer to optional-chain memberList/channelList too.
🔧 Suggested fix
- ? (loadUISelections().sidebarPreferences?.memberList.isVisible ?? true)
+ ? (loadUISelections().sidebarPreferences?.memberList?.isVisible ?? true)
: false,
isChannelListVisible:
- loadUISelections().sidebarPreferences?.channelList.isVisible ?? true,
+ loadUISelections().sidebarPreferences?.channelList?.isVisible ?? true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isMemberListVisible: | |
| // Only restore member list on startup if the window is wide enough for the sidebar. | |
| // At intermediate widths the member list replaces the chat area which is bad UX on startup. | |
| typeof window !== "undefined" && | |
| !window.matchMedia("(max-width: 1080px)").matches | |
| ? (loadUISelections().sidebarPreferences?.memberList.isVisible ?? true) | |
| : false, | |
| isChannelListVisible: | |
| loadUISelections().sidebarPreferences?.channelList.isVisible ?? true, | |
| isChannelListModalOpen: false, | |
| isMemberListVisible: | |
| // Only restore member list on startup if the window is wide enough for the sidebar. | |
| // At intermediate widths the member list replaces the chat area which is bad UX on startup. | |
| typeof window !== "undefined" && | |
| !window.matchMedia("(max-width: 1080px)").matches | |
| ? (loadUISelections().sidebarPreferences?.memberList?.isVisible ?? true) | |
| : false, | |
| isChannelListVisible: | |
| loadUISelections().sidebarPreferences?.channelList?.isVisible ?? true, | |
| isChannelListModalOpen: false, |
🤖 Prompt for AI Agents
In `@src/store/index.ts` around lines 824 - 833, The current initialization reads
loadUISelections().sidebarPreferences?.memberList.isVisible and
.channelList.isVisible which can throw if memberList or channelList are missing;
update the isMemberListVisible and isChannelListVisible initializers to safely
optional-chain through memberList and channelList (e.g., use
sidebarPreferences?.memberList?.isVisible and
sidebarPreferences?.channelList?.isVisible) and keep the existing nullish
fallback defaults so missing/partial payloads don't cause runtime errors when
calling loadUISelections().
| const newState = get(); | ||
| const currentPrefs = newState.ui.sidebarPreferences || { | ||
| channelList: { isVisible: true, width: 264 }, | ||
| memberList: { isVisible: true, width: 280 }, | ||
| }; | ||
| saveUISelections({ | ||
| selectedServerId: newState.ui.selectedServerId, | ||
| perServerSelections: newState.ui.perServerSelections, | ||
| sidebarPreferences: { | ||
| ...currentPrefs, | ||
| memberList: { | ||
| ...currentPrefs.memberList, | ||
| isVisible: newState.ui.isMemberListVisible, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Persisting sidebar preferences clears lastSelection.
saveUISelections() here overwrites the stored object without lastSelection, so the name-based restore fallback is lost after toggling sidebars or resizing. Preserve existing persisted fields when saving.
🛠️ Suggested fix (apply to all three call sites)
- saveUISelections({
- selectedServerId: newState.ui.selectedServerId,
- perServerSelections: newState.ui.perServerSelections,
- sidebarPreferences: {
- ...currentPrefs,
- memberList: {
- ...currentPrefs.memberList,
- isVisible: newState.ui.isMemberListVisible,
- },
- },
- });
+ const existing = loadUISelections();
+ saveUISelections({
+ ...existing,
+ selectedServerId: newState.ui.selectedServerId,
+ perServerSelections: newState.ui.perServerSelections,
+ sidebarPreferences: {
+ ...currentPrefs,
+ memberList: {
+ ...currentPrefs.memberList,
+ isVisible: newState.ui.isMemberListVisible,
+ },
+ },
+ });Also applies to: 2617-2632, 2635-2655
🤖 Prompt for AI Agents
In `@src/store/index.ts` around lines 2586 - 2601, The call to
saveUISelections(...) overwrites the persisted object and drops fields like
lastSelection; update each call site (the block using get() / newState.ui and
the two other similar blocks) to first read the existing persisted UI selections
(the current saved object) and merge it with the new data, e.g. spread the
persisted object into the argument to saveUISelections so existing keys such as
lastSelection are preserved while updating selectedServerId,
perServerSelections, and sidebarPreferences (the memberList/channelList changes
derived from newState.ui.isMemberListVisible etc.). Ensure you reference
saveUISelections, get(), newState.ui, sidebarPreferences, selectedServerId,
perServerSelections, and lastSelection when applying the merge at all three
locations.
| toggleTopicModal: (isOpen, context) => { | ||
| set((state) => ({ | ||
| ui: { | ||
| ...state.ui, | ||
| isTopicModalOpen: isOpen, | ||
| topicModalContext: isOpen && context ? context : null, | ||
| }, | ||
| })); | ||
| }, | ||
|
|
||
| toggleUserProfileModalWithContext: (isOpen, context) => { | ||
| set((state) => ({ | ||
| ui: { | ||
| ...state.ui, | ||
| isSettingsModalOpen: isOpen, | ||
| userProfileModalContext: isOpen && context ? context : null, | ||
| }, | ||
| })); | ||
| }, |
There was a problem hiding this comment.
Modal toggles write to UIState keys that don’t exist.
isTopicModalOpen, topicModalContext, and userProfileModalContext aren’t in UIState, so these updates are effectively “invisible”. Align with the current request-based fields (topicModalRequest, profileViewRequest, isUserProfileModalOpen) or reintroduce the missing UIState fields.
🔧 Suggested alignment with current UIState
toggleTopicModal: (isOpen, context) => {
set((state) => ({
ui: {
...state.ui,
- isTopicModalOpen: isOpen,
- topicModalContext: isOpen && context ? context : null,
+ topicModalRequest: isOpen && context ? context : null,
},
}));
},
toggleUserProfileModalWithContext: (isOpen, context) => {
set((state) => ({
ui: {
...state.ui,
- isSettingsModalOpen: isOpen,
- userProfileModalContext: isOpen && context ? context : null,
+ isUserProfileModalOpen: isOpen,
+ profileViewRequest: isOpen && context ? context : null,
},
}));
},🤖 Prompt for AI Agents
In `@src/store/index.ts` around lines 2689 - 2707, The modal toggle mutations
toggleTopicModal and toggleUserProfileModalWithContext are writing UI keys that
don't exist (isTopicModalOpen, topicModalContext, userProfileModalContext);
update them to use the current UIState keys instead: for toggleTopicModal set
ui.topicModalRequest = isOpen && context ? context : null and
ui.isUserProfileModalOpen remains unchanged (or set appropriately only in the
user profile toggle), and for toggleUserProfileModalWithContext set
ui.profileViewRequest = isOpen && context ? context : null and
ui.isUserProfileModalOpen = isOpen; keep the existing state.ui spread and
replace the non-existent properties with topicModalRequest/profileViewRequest
and isUserProfileModalOpen so the UIState updates are effective.
b901925 to
eb9484f
Compare
matheusfillipe
force-pushed
the
fix/more-polishment
branch
from
eb9484f to
e6de627
Compare
…o fix/more-polishment
matheusfillipe
force-pushed
the
fix/more-polishment
branch
from
0c294fb to
ccdcce8
Compare
2 participants
- [ ] Fix for multiple servers. App is slow and weird sometimes...fixes #128
Summary by CodeRabbit
New Features
Improvements
UI/UX
@coderabbitai ignore