Conversation
📝 WalkthroughWalkthroughAdds many chat features: virtualized message list (react-virtuoso), admin/moderation UI and hook, multiple chat hooks (websocket, metadata, deep-linking, pending posts, message rendering, post actions, DM warning), several UI components (channel header, mention token, keyboard shortcuts modal), message-input and emoji/GIF picker updates, emoji-utils typings, Mattermost API/ws minor typing tweaks, and image-upload prop/accessibility updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client (UI)
participant Input as MessageInput
participant Mutation as SendMutation
participant Server as Mattermost API
participant WS as useChannelWebSocket
participant UIList as VirtualizedMessageList
User->>Input: submit message
Input->>Mutation: trigger send (optimistic pending id)
Mutation-->>UIList: render pending post (uses pending id)
Mutation->>Server: POST message
Server-->>WS: broadcast "posted" event
WS->>WS: attempt match by pending_post_id or fallback (message/root/timestamp)
alt matched
WS->>UIList: onPostedConfirm (replace pending with confirmed post)
else not matched
WS->>UIList: add incoming post normally
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/app/chats/_components/chats-client.tsx (2)
1043-1045:⚠️ Potential issue | 🟡 Minor
isLoadingshould beisPendingfor mutation status check.All other mutations in this file use
.isPending(e.g.,markChannelViewedMutation.isPending,joinChannelMutation.isPending), butdirectChannelMutationuses.isLoadingon line 1043. In React Query v5, mutations exposeisPendinginstead ofisLoading.Proposed fix
- {directChannelMutation.isLoading + {directChannelMutation.isPending
117-128:⚠️ Potential issue | 🟡 MinorType annotation on
unreadByChannelIdis narrower than the actual value.Line 118 declares the fallback as
Map<string, { mention_count: number; message_count: number }>(missingthread_unread), but the reducer on line 124 populatesthread_unreadand its generic includes it. This means callers going through the fallback type path won't seethread_unread. The two types should be consistent.Proposed fix
- if (!unreadSummary?.channels) return new Map<string, { mention_count: number; message_count: number }>(); + if (!unreadSummary?.channels) return new Map<string, { mention_count: number; message_count: number; thread_unread: number }>();apps/web/src/features/shared/image-upload-button/index.tsx (1)
40-52:⚠️ Potential issue | 🔴 CriticalRemove duplicate: The modern
ImageUploadButtonimplementation is unused and should be consolidated.Two implementations exist in the codebase:
apps/web/src/features/shared/image-upload-button.tsx— modernized version usinguseImageUploadhook (react-query based)apps/web/src/features/shared/image-upload-button/index.tsx— legacy version using manualuploadImage,getAccessToken, anduseStateWhen both are present, module resolution prefers the directory structure, so all consumers import the legacy version. The modern implementation is completely unused dead code and should be removed. Consider either deleting the unused modern version or refactoring consumers to use it instead, then removing the legacy code. Also note that the export appears twice in
apps/web/src/features/shared/index.ts(lines 20 and 52).
🤖 Fix all issues with AI agents
In `@apps/web/src/features/chat/hooks/use-message-rendering.tsx`:
- Around line 134-141: The code currently calls marked.setOptions() which
mutates global marked state; change to create a dedicated Marked instance and
use it in the markdownParser so you don't mutate globals. Import the Marked
class (or factory) from the marked package, then inside the hook create a new
instance (e.g., new Marked(...)) or call the Marked constructor and set options
on that instance (not marked.setOptions). Update markdownParser to call
instance.parse(content) and return that string, keeping the creation local to
the hook (e.g., inside useMemo) so no global marked state is modified.
🧹 Nitpick comments (11)
apps/web/src/app/chats/_components/chats-client.tsx (1)
460-1006: Heavy duplication of channel card rendering across all four sections.The Favorites, Direct Messages, Regular Channels, and Search Results sections each repeat nearly identical channel card markup (avatar, title, unread badge, dropdown actions). Consider extracting a shared
ChannelListItemcomponent to reduce duplication and simplify future changes.apps/web/src/features/chat/hooks/use-deep-linking.ts (1)
10-14:aroundQuery.datais typed asany— consider using the actual response type.Since
MattermostPostsResponseis already imported, thedatafield onaroundQuerycould be typed asMattermostPostsResponse | undefined(or whatever the around-query actually returns) to preserve type safety across callers.♻️ Suggested typing
aroundQuery: { - data: any; + data: MattermostPostsResponse | undefined; isLoading: boolean; error: Error | null; };apps/web/src/features/shared/image-upload-button/index.tsx (1)
22-22: Nit: Destructurearia-labelandtitledirectly instead of usingrestProps.Since
aria-labelandtitleare the only remaining props inrestPropsand are explicitly declared in the interface, destructuring them directly would be clearer and avoid the indirection.Proposed simplification
At line 22:
-export function ImageUploadButton({ onBegin, onEnd, size = "sm", appearance, className, disabled, ...restProps }: UploadButtonProps) { +export function ImageUploadButton({ onBegin, onEnd, size = "sm", appearance, className, disabled, "aria-label": ariaLabel, title }: UploadButtonProps) {At lines 70-71:
- aria-label={restProps["aria-label"]} - title={restProps.title} + aria-label={ariaLabel} + title={title}Also applies to: 70-71
apps/web/src/features/chat/hooks/use-post-actions.ts (1)
93-115:handlePinTogglehas inconsistent return types:string | null | undefined.The function returns a
string(line 97) when pin limit is hit,null(line 114) on success, andundefinedimplicitly (line 94) when!canPin. Callers must handle all three, which is error-prone. Consider normalizing to always returnstring | null.♻️ Suggested fix
const handlePinToggle = useCallback((postId: string, isPinned: boolean) => { - if (!canPin) return; + if (!canPin) return null; if (!isPinned && (pinnedPostsQuery.data?.posts.length ?? 0) >= 5) { return "Cannot pin more than 5 messages per channel"; }apps/web/src/features/chat/components/admin-panel.tsx (1)
53-53: Remove debugconsole.logfrom production code.Line 53 logs the full admin permissions object to the console. Consider removing this or gating it behind a development-only check.
apps/web/src/features/chat/components/virtualized-message-list.tsx (2)
7-7: Consider exportingINITIAL_INDEXas a named constant with documentation.
INITIAL_INDEX = 100000is a Virtuoso pattern for reverse-infinite scrolling. A brief comment explaining why this value exists would help future maintainers.
265-268:followOutputreturn type cast is unnecessary.The
as "smooth" | falsecast can be avoided by typing the return directly, since TypeScript can infer the union from the ternary.♻️ Suggested simplification
const followOutput = useCallback( - (isAtBottom: boolean) => (isAtBottom ? "smooth" : false) as "smooth" | false, + (isAtBottom: boolean): "smooth" | false => (isAtBottom ? "smooth" : false), [] );apps/web/src/features/chat/hooks/use-channel-websocket.ts (1)
12-49: Inline type intersection is harder to maintain than extending the interface.The extra params (
setIsWebSocketActive,setReconnectAttempt,setReconnectDelay,isWebSocketActive) are added via an intersection type at the function signature (line 45) instead of inUseChannelWebSocketParams. This splits the contract across two locations.♻️ Suggested consolidation
interface UseChannelWebSocketParams { channelId: string; queryClient: QueryClient; getCurrentUserId: () => string | undefined; onPostedConfirm: (payload: OnPostedConfirmPayload) => void; pendingPostRefs: { /* ... */ }; usersById: Record<string, MattermostUser>; + setIsWebSocketActive: (active: boolean) => void; + setReconnectAttempt: (attempt: number | null) => void; + setReconnectDelay: (delay: number | null) => void; + isWebSocketActive: boolean; }apps/web/src/features/chat/components/message-item.tsx (1)
104-156: Custom equality function is thorough butreactMutationPending,canPin, andpinMutationPendingtrigger re-renders for all items.Lines 146-148 compare global mutation/state flags that change simultaneously for every item, causing all memoized items to re-render when any reaction or pin mutation is in flight. This reduces the effectiveness of the custom comparator for those state transitions.
Consider moving these checks into per-item derived booleans (e.g., only pass
reactMutationPendingwhen the item's reaction picker is open, or handle the disabled state at the parent level) to avoid invalidating every item's memo.apps/web/src/features/chat/hooks/use-message-rendering.tsx (2)
148-153: RegExp re-created on everyrenderTextWithMentionscall.
renderTextWithMentionsis invoked many times per message (for each text segment, trailing text, etc.), and each call allocates a newRegExpon Line 150. Since the pattern is static, hoist it outside the function.Proposed fix
const renderMessageContent = useMemo(() => { + const mentionMatcher = new RegExp(USER_MENTION_PURE_REGEX.source, "i"); + const renderTextWithMentions = (content: string, keyPrefix = "mention") => { - const mentionMatcher = new RegExp(USER_MENTION_PURE_REGEX.source, "i"); const parts = content.split(
270-310: Redundant conditions inisPlainImageLinkdetection.Lines 282-283:
childText === hrefvschildText === href.trim()— since URLs shouldn't contain leading/trailing whitespace andchildTextis already trimmed, these are effectively identical. Consider simplifying to reduce cognitive load.Simplified check
const isPlainImageLink = !containsImage && isImageUrl(href) && ( childText === href || - childText === href.trim() || !childText || childText === href.replace(/^https?:\/\//, '').trim() || isImageUrl(childText) );
1 participant
Summary by CodeRabbit
New Features
Improvements