Conversation
📝 WalkthroughWalkthroughThe PR adds optional online-user status fetching to Mattermost posts via an Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component
participant ReactQuery as React Query
participant API as API Route
participant MM as Mattermost Backend
Component->>ReactQuery: request posts (includeOnline?)
ReactQuery->>API: GET /channels/[id]/posts?include_online=1
API->>MM: Fetch posts
MM-->>API: Posts
alt include_online = true
API->>MM: Paginated fetch channel users
MM-->>API: Channel users (paged)
par Parallel status fetch
API->>MM: Fetch statuses for members
MM-->>API: Statuses
end
API->>API: Build users map + onlineUserIds
else
API->>API: Build response without onlineUserIds
end
API-->>ReactQuery: Posts (+ optional onlineUserIds)
ReactQuery-->>Component: Provide cached data
Component->>Component: Render (optionally show online users)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/mattermost/channels/`[channelId]/posts/route.ts:
- Around line 137-168: The channel user fetch in the includeOnline branch
(inside route handler using mmUserFetch in apps/web/src/.../posts/route.ts) is
limited to per_page=200, which will miss members in large channels; update the
logic around the channelUsers fetch (the code that calls mmUserFetch for
`/users?in_channel=${channelId}&per_page=200&page=0`) to paginate: repeatedly
call mmUserFetch with increasing page numbers, accumulate results into the users
variable (used to build the users map), and stop when a page returns fewer than
the page size (or an empty array); ensure the same aggregated list is used when
posting to `/users/status/ids` for statusPromise and preserve the existing error
handling for status fetch.
In `@apps/web/src/features/chat/mattermost-websocket.ts`:
- Around line 478-496: handlePostEdited currently calls upsertPostInCaches which
only checks the first page and causes duplicates when edited posts live on later
pages; change handlePostEdited to call updatePostInCaches (the function that
scans all pages) for edits and keep upsertPostInCaches only for new posts.
Locate the handlePostEdited method and replace the upsertPostInCaches(channelId,
post) invocation with updatePostInCaches(channelId, post); retain the existing
error/invalidateQueries behavior for cases where post JSON is missing or parsing
fails.
🧹 Nitpick comments (4)
apps/web/src/features/chat/mattermost-channel-view.tsx (1)
266-269: Consider using a ref to track previousshowOnlineUsersvalue to avoid redundant invalidations.The current effect triggers invalidation whenever
channelIdchanges whileshowOnlineUsersis true. However, whenchannelIdchanges, React Query already fetches fresh data for the new channel due to the query key change, making this invalidation redundant.♻️ Suggested refinement
+const prevShowOnlineUsersRef = useRef(false); + useEffect(() => { - if (!showOnlineUsers) return; - queryClient.invalidateQueries({ queryKey: ["mattermost-posts-infinite", channelId] }); -}, [showOnlineUsers, channelId, queryClient]); + const wasOff = !prevShowOnlineUsersRef.current; + prevShowOnlineUsersRef.current = showOnlineUsers; + + // Only invalidate when toggling from false to true + if (showOnlineUsers && wasOff) { + queryClient.invalidateQueries({ queryKey: ["mattermost-posts-infinite", channelId] }); + } +}, [showOnlineUsers, channelId, queryClient]);apps/web/src/features/chat/mattermost-api.ts (2)
571-592:includeOnlineoption not included in query key could cause cache inconsistencies.The
includeOnlineoption changes the data returned by the API but isn't part of the query key. If multiple components useuseMattermostPostswith the samechannelIdbut differentincludeOnlinevalues, they would incorrectly share the same cache entry.While the current usage in
mattermost-channel-view.tsxhandles this via manual invalidation, consider includingincludeOnlinein the query key for correctness and to prevent subtle bugs if other consumers are added.♻️ Suggested fix
export function useMattermostPosts( channelId: string | undefined, options?: { includeOnline?: boolean } ) { return useQuery({ - queryKey: ["mattermost-posts", channelId], + queryKey: ["mattermost-posts", channelId, { includeOnline: options?.includeOnline ?? false }], enabled: Boolean(channelId),
594-624: Same concern:includeOnlineshould be in query key foruseMattermostPostsInfinite.For the same reasons as
useMattermostPosts, consider addingincludeOnlineto the query key to ensure cache correctness when multiple consumers exist.♻️ Suggested fix
export function useMattermostPostsInfinite( channelId: string | undefined, options?: { refetchInterval?: number | false; includeOnline?: boolean } ) { return useInfiniteQuery({ - queryKey: ["mattermost-posts-infinite", channelId], + queryKey: ["mattermost-posts-infinite", channelId, { includeOnline: options?.includeOnline ?? false }], enabled: Boolean(channelId),apps/web/src/features/chat/mattermost-websocket.ts (1)
590-631:upsertPostInCachesonly searches first page, which is correct for new posts but not for updates.The method name suggests it handles both insert and update scenarios, but the implementation only searches
firstPagefor existing posts. This works for new posts (which belong in the first/newest page) but will cause issues if used for updates.Consider either:
- Renaming to
insertPostInCachesto clarify its purpose- Or updating the implementation to search all pages
♻️ Option 1: Rename to clarify intent
-private upsertPostInCaches(channelId: string, post: MattermostPost): void { +private insertNewPostInCaches(channelId: string, post: MattermostPost): void {And update
handlePostedto use the renamed method.
1 participant
Summary by CodeRabbit
New Features
Performance
UX
✏️ Tip: You can customize this high-level summary in your review settings.