Conversation
📝 WalkthroughWalkthroughFilters deactivated direct-message channels by checking Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/features/chat/components/chat-image.tsx`:
- Around line 12-41: Reset isLoaded (and clear any existing zoom) when the src
prop changes so the effect that creates mediumZoom runs for the new image; add a
useEffect that watches src and calls setIsLoaded(false) and detaches/clears
zoomRef.current (the zoomRef used in ChatImage) so the old zoom overlay is
removed before the new image loads, keeping the existing onLoad/useEffect logic
intact.
🧹 Nitpick comments (4)
apps/web/src/app/api/mattermost/channels/route.ts (2)
183-211: DM deactivation filter logic is correct; the type assertion on Line 207 is noisy.The filter correctly excludes channels where the other user has
delete_at > 0. However, the cast on Line 207 is verbose because TypeScript can't narrow the union produced by.map(). A cleaner approach is to storedirectUserin the mapped object for all branches (or chain the filter inside the D-type branch).♻️ Simplify by always including `directUser` in the mapped result
const channelsWithDirectUsers = filteredChannels .map((channel) => { - if (channel.type !== "D") return channel; + if (channel.type !== "D") return { ...channel, directUser: null as MattermostUser | null }; const parts = channel.name?.split("__") ?? []; const otherUserId = @@ ... @@ }) // Filter out DM channels where the other user has been deactivated .filter((channel) => { if (channel.type !== "D") return true; - const directUser = (channel as typeof channel & { directUser: MattermostUser | null }).directUser; + const directUser = channel.directUser; // Keep channel if directUser doesn't exist (can't determine) or if they're not deactivated if (!directUser) return true; return !directUser.delete_at || directUser.delete_at === 0; });
32-32: MultipleMattermostUserinterfaces across the codebase — consider consolidating.There are at least four separate
MattermostUserinterface definitions:apps/web/src/server/mattermost.ts(Lines 14-18),apps/web/src/features/chat/mattermost-api.ts(Lines 546-553), this file, and the unreads route. Thedelete_atfield is now added here and in the unreads route but not in the shared definitions. If any shared code starts consumingdelete_at, it won't be typed.This is low-priority but worth tracking to avoid drift.
apps/web/src/app/api/mattermost/channels/unreads/route.ts (2)
139-186: Extra/users/idsfetch on every unread poll may add latency.This route is likely polled frequently (for badge counts, etc.). The new
POST /users/idscall on every request adds a round-trip to the Mattermost server. For users with many DM channels, this could noticeably increase response time.Consider caching the deactivated-user set (e.g., short TTL in-memory or via a shared utility) so the fetch doesn't happen on every poll. This is not blocking for the current fix but worth considering if you observe latency increases.
139-186: DM channel name-parsing logic is duplicated acrosschannels/route.tsand this file.The
channel.name?.split("__")→parts.find(id => id !== currentUser.id) || parts[0]pattern appears in both routes (and multiple times within each). Extracting a small helper (e.g.,getOtherDmUserId(channelName, currentUserId)) would reduce duplication and the risk of them diverging.
Summary by CodeRabbit
New Features
Bug Fixes