Repository navigation
Refactor inbox to unified messaging interface with channels and DMs - #345
Conversation
- Redesign /dashboard/inbox to show message list in center area (like Gmail) - When clicking a conversation, transition to sidebar list + chat in center - Create new inbox routes for DMs (/dashboard/inbox/dm/[id]) and channels (/dashboard/inbox/channel/[id]) - Add "Open in Drive" button for channels to navigate to drive context - Add "New Message" button to start new conversations from inbox - Update InboxSidebar to use new inbox routes - Redirect legacy /dashboard/messages routes to new inbox routes - Remove unused MessagesLeftSidebar from MemoizedSidebar https://claude.ai/code/session_01JH2oYLLDvguLZ5pWCZzttL
- Delete /dashboard/messages directory and all subroutes - Remove MessagesLeftSidebar component (no longer needed) - Update all references to use new /dashboard/inbox routes: - connections page: /dashboard/inbox/dm/[id] - notification dropdown: /dashboard/inbox/dm/[id] - tab-title parsing and tests - sample-dashboard.html - Remove 'messages' from FULL_PAGE_ROUTES in dashboard layout - Clean up MemoizedSidebar (remove legacy messages handling) https://claude.ai/code/session_01JH2oYLLDvguLZ5pWCZzttL
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR consolidates the messaging system into a unified inbox architecture, removing the legacy Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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/dashboard/inbox/channel/`[pageId]/page.tsx:
- Around line 83-92: The fetchMessages useEffect lacks error handling and
doesn't check HTTP status before calling res.json(); update the async
fetchMessages function (used in the useEffect that depends on pageId) to wrap
the fetchWithAuth call and JSON parsing in a try/catch, check res.ok and throw
or handle non-ok responses (e.g., read error body or create a descriptive Error)
before calling res.json(), and on error set a safe state (e.g., setMessages([])
or a separate error state) and optionally log the error so the UI doesn't
receive invalid data.
- Around line 195-203: handleRefresh currently parses the fetch response without
checking HTTP status; update the handleRefresh async function to verify res.ok
after awaiting fetchWithAuth(`/api/channels/${pageId}/messages`) (same pattern
used in the initial fetch), and if not ok throw or handle an error (e.g., throw
new Error(`Failed to fetch messages: ${res.status}`) or call
setMessages([])/log) so the catch block receives a proper error instead of
attempting to parse a non-OK body; ensure you reference the handleRefresh
function and use the pageId dependency already present.
🧹 Nitpick comments (3)
apps/web/src/components/inbox/InboxCenterList.tsx (1)
84-106: Consider deduplicating items when loading more pages.The
loadMorefunction appendsmoreData.itemsdirectly without checking for duplicates. If real-time updates via socket add an item that also appears in the next page, duplicates could occur.♻️ Proposed fix to deduplicate items
const moreData: InboxResponse = await response.json(); - setAllItems((prev) => [...prev, ...moreData.items]); + setAllItems((prev) => { + const existingIds = new Set(prev.map(item => `${item.type}-${item.id}`)); + const newItems = moreData.items.filter( + item => !existingIds.has(`${item.type}-${item.id}`) + ); + return [...prev, ...newItems]; + }); setPagination(moreData.pagination);apps/web/src/app/dashboard/inbox/channel/[pageId]/page.tsx (1)
242-277: Potential stale closure inhandleRemoveReaction.The callback captures
messagesin its dependency array, which means a new function is created on every message update. This is necessary to find the removed reaction for rollback, but could cause stale closures during rapid interactions. Consider using a ref or functional state update to access current messages.♻️ Alternative using ref to avoid stale closure
+ const messagesRef = useRef<MessageWithUser[]>([]); + messagesRef.current = messages; const handleRemoveReaction = useCallback(async (messageId: string, emoji: string) => { if (!user) return; - const removedReaction = messages + const removedReaction = messagesRef.current .find((m) => m.id === messageId) ?.reactions?.find((r) => r.emoji === emoji && r.userId === user.id); // ... rest of the function - }, [pageId, user, messages]); + }, [pageId, user]);apps/web/src/app/dashboard/inbox/dm/[conversationId]/page.tsx (1)
119-132: Consider adding optimistic UI for sent messages.Unlike the channel page which shows optimistic messages immediately, this DM page clears the input and waits for the socket event to display the sent message. This could feel less responsive to users. Consider adding optimistic message display for consistency.
This is a UX enhancement and not critical since the socket response is typically fast.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96d70f738e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useInboxSocket({ driveId }); | ||
|
|
||
| const { data, error, isLoading } = useSWR<InboxResponse>(apiUrl, fetcher, { | ||
| refreshInterval: 0, | ||
| isPaused: () => hasLoadedRef.current && isEditingActive(), |
There was a problem hiding this comment.
The center list calls useInboxSocket, but it never uses the hook’s hasLoadedRef to mark the initial fetch as complete. That ref stays false, so useInboxSocket drops all inbox update events and the main inbox view won’t update in real time (new messages/read status changes won’t appear until a manual refresh). Consider using the hook’s returned hasLoadedRef in the SWR onSuccess handler (as the sidebar does), or change the hook so it doesn’t rely on an external ref.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 13194b2. The issue was that useInboxSocket had its own internal hasLoadedRef that never got set to true, causing all socket events to be dropped.
Fix: Modified the hook to accept an optional external hasLoadedRef parameter:
// useInboxSocket.ts
export function useInboxSocket({ driveId, hasLoadedRef: externalRef }: UseInboxSocketOptions = {}) {
const internalRef = useRef(false);
const hasLoadedRef = externalRef ?? internalRef;
// ...
}// InboxCenterList.tsx
useInboxSocket({ driveId, hasLoadedRef });Now the component's hasLoadedRef (which is set to true in the SWR onSuccess callback) is shared with the socket hook, so socket events are properly processed after initial data loads.
- Remove dead code referencing 'messages-conversation' in useTabMeta.ts (type was removed from PathType during route refactor) - Add error handling for fetchMessages in channel page (check res.ok) - Add response status check in handleRefresh (check res.ok) - Wire useInboxSocket to use component's hasLoadedRef for proper socket event processing after initial data load
Review Feedback AddressedI've addressed all the review comments in commit 13194b2: 1. TypeScript Error Fix (CI Blocker)
2. CodeRabbit: Error handling in fetchMessages
3. CodeRabbit: Status check in handleRefresh
4. Codex: Wire inbox socket updates into center list
All changes verified with:
|
Addresses CodeRabbit nitpick: socket updates could add items that also appear in the next pagination page, causing visual duplicates.
Nitpick Comments Addressed1. InboxCenterList deduplication (commit 4f8d1e0) ✅ setAllItems((prev) => {
const existingIds = new Set(prev.map(item => `${item.type}-${item.id}`));
const newItems = moreData.items.filter(
item => !existingIds.has(`${item.type}-${item.id}`)
);
return [...prev, ...newItems];
});2. handleRemoveReaction stale closure - Intentionally not addressed 3. Optimistic UI for DM sent messages - Deferred |
fix(ai): meter /btw side questions, keep rejected queued sends, close CodeQL #345
2 participants
Summary
This PR refactors the inbox system to provide a unified messaging interface that consolidates channels and direct messages under a single
/dashboard/inboxroute structure. The changes extract messaging logic into reusable components and establish a cleaner separation between channel conversations and direct message conversations.Key Changes
Unified Inbox Structure: Reorganized inbox routes to use
/dashboard/inbox/channel/[pageId]for channels and/dashboard/inbox/dm/[conversationId]for direct messages, replacing the previous/dashboard/messagesrouteComponent Extraction: Created
InboxCenterListcomponent to handle the default inbox view, reducing code duplication across inbox pagesChannel Messaging: Implemented full-featured channel page with:
StreamingMarkdownDirect Messaging: Implemented DM page with:
New Conversation Flow: Added
/dashboard/inbox/newpage to initiate new conversations with:Backward Compatibility: Maintained redirect from old
/dashboard/messages/[conversationId]route to new inbox DM routeImplementation Details
ChannelInputcomponent for both channels and DMsuseSocketStorefor real-time updates across messaging featuresusePermissionshook for granular access control in channelshttps://claude.ai/code/session_01JH2oYLLDvguLZ5pWCZzttL
Summary by CodeRabbit
New Features
User Interface