Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe PR replaces Socket.IO with native WebSocket for real-time messaging, updates tRPC chat endpoints to resolve users from the database before operations, and enhances the messages page with conditional query enabling, user derivation, and UI improvements including message input handling and call button states. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as Client App
participant tRPC
participant DB
participant WS as WebSocket<br/>(Python)
participant Recipient
User->>App: Type & send message
App->>tRPC: sendMessage(text, conversationId)
tRPC->>DB: Resolve current user by clerkUid
DB-->>tRPC: Return user record
tRPC->>DB: Persist message with user.id
DB-->>tRPC: Message saved
tRPC-->>App: Success
rect rgb(100, 150, 200)
Note over App,WS: Real-time notification
App->>WS: Send message payload via WebSocket
WS->>Recipient: Deliver message
end
Recipient->>Recipient: Receives WebSocket message
Recipient->>tRPC: Refetch messages
tRPC->>DB: Query latest messages
DB-->>tRPC: Return updated messages
tRPC-->>Recipient: Display new message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
♻️ Duplicate comments (2)
src/server/api/routers/chat.ts (2)
55-62: Duplicate pattern - see middleware suggestion for getConversations.This endpoint repeats the same user lookup pattern. The middleware approach suggested for
getConversationswould eliminate this redundancy.
70-101: Duplicate pattern - see middleware suggestion for getConversations.This endpoint also repeats the user lookup pattern. Implementing the suggested middleware would eliminate this redundancy and improve performance across all chat endpoints.
🧹 Nitpick comments (1)
src/app/dashboard/messages/[conversationId]/page.tsx (1)
126-127: Consider Shift+Enter for multiline messages.The Enter key triggering send is good UX, but users might want to send multiline messages. Consider allowing Shift+Enter to insert a newline.
- onKeyDown={(e) => e.key === 'Enter' && handleSendMessage()} + onKeyDown={(e) => { + if (e.key === 'Enter' && !e.shiftKey) { + e.preventDefault(); + handleSendMessage(); + } + }}If you implement this, also change the input to a
<textarea>to better support multiline content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/dashboard/messages/[conversationId]/page.tsx(3 hunks)src/server/api/routers/chat.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/server/api/routers/chat.ts (1)
src/server/api/trpc.ts (2)
createTRPCRouter(78-78)protectedProcedure(104-104)
src/app/dashboard/messages/[conversationId]/page.tsx (1)
src/trpc/react.tsx (1)
api(25-25)
🔇 Additional comments (6)
src/server/api/routers/chat.ts (2)
2-2: LGTM!The TRPCError import is necessary for the error handling added in this PR.
68-68: LGTM!The comment clarifies that
tutorIdrefers to the database CUID, which is helpful for understanding the data flow.src/app/dashboard/messages/[conversationId]/page.tsx (4)
5-5: LGTM!Using
useRefto store the WebSocket instance is the correct pattern to maintain a persistent connection across re-renders.Also applies to: 12-12
14-17: LGTM!The conditional query enabling prevents unnecessary API calls when
conversationIdis not yet available, which is a good practice.
98-98: LGTM!The fallback text and disabled state improve the user experience by handling edge cases gracefully.
Also applies to: 102-102
107-107: LGTM!Adding
overflow-y-autoenables proper scrolling for long message histories, and the avatar placeholder comment aids maintainability.Also applies to: 110-110
| const currentUser = messages?.find(m => m.sender.clerkUid === user?.id)?.sender; | ||
| const otherUser = messages?.[0]?.conversation?.User.find( | ||
| (u) => u.id !== user?.id | ||
| (u) => u.clerkUid !== user?.id | ||
| ); |
There was a problem hiding this comment.
Critical: currentUser derivation fails in conversations without user's messages.
Deriving currentUser by searching messages where sender.clerkUid === user?.id will return undefined if:
- The conversation has no messages yet
- The current user hasn't sent any messages in this conversation
This breaks the WebSocket connection (line 28-30 checks currentUser?.name), preventing real-time messaging in new conversations.
Use the Clerk user data directly instead:
- const currentUser = messages?.find(m => m.sender.clerkUid === user?.id)?.sender;
+ // Clerk user should have the username/name we need
+ const currentUserName = user?.username ?? user?.fullName ?? user?.id;Then update the WebSocket connection:
- if (!currentUser?.name) {
+ if (!currentUserName) {
return;
}
- const ws = new WebSocket(`ws://localhost:8000/ws/${currentUser.name}`);
+ const ws = new WebSocket(`ws://localhost:8000/ws/${currentUserName}`);Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| // Connect to the Python WebSocket server | ||
| // Assumes the server is running on localhost:8000 | ||
| const ws = new WebSocket(`ws://localhost:8000/ws/${currentUser.name}`); |
There was a problem hiding this comment.
Hardcoded WebSocket URL will break in production.
The WebSocket URL is hardcoded to localhost:8000, which will fail when deployed. Use an environment variable to configure this.
- const ws = new WebSocket(`ws://localhost:8000/ws/${currentUser.name}`);
+ const wsUrl = process.env.NEXT_PUBLIC_WS_URL ?? 'ws://localhost:8000';
+ const ws = new WebSocket(`${wsUrl}/ws/${currentUser.name}`);Add to your .env.local:
NEXT_PUBLIC_WS_URL=ws://localhost:8000
📝 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 ws = new WebSocket(`ws://localhost:8000/ws/${currentUser.name}`); | |
| const wsUrl = process.env.NEXT_PUBLIC_WS_URL ?? 'ws://localhost:8000'; | |
| const ws = new WebSocket(`${wsUrl}/ws/${currentUser.name}`); |
🤖 Prompt for AI Agents
In src/app/dashboard/messages/[conversationId]/page.tsx around line 34, the
WebSocket URL is hardcoded to ws://localhost:8000 which will break in
production; change it to build the URL from an environment variable
(NEXT_PUBLIC_WS_URL) plus the path (/ws/${currentUser.name}) so the base
host/protocol can be configured, add NEXT_PUBLIC_WS_URL=ws://localhost:8000 to
.env.local for local dev, and ensure the code falls back gracefully (e.g., to
window.location-derived ws/wss) if the env var is not set.
| }; | ||
|
|
||
| ws.onmessage = (event) => { | ||
| const message = JSON.parse(event.data as string) as { type: string }; |
There was a problem hiding this comment.
Unsafe type assertion for WebSocket data.
WebSocket event.data can be a string, Blob, or ArrayBuffer. The type assertion as string is unsafe and could fail at runtime if the server sends binary data.
Add runtime validation:
- const message = JSON.parse(event.data as string) as { type: string };
+ if (typeof event.data !== 'string') {
+ console.error('Unexpected WebSocket data type:', typeof event.data);
+ return;
+ }
+ const message = JSON.parse(event.data) as { type: string };📝 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 message = JSON.parse(event.data as string) as { type: string }; | |
| if (typeof event.data !== 'string') { | |
| console.error('Unexpected WebSocket data type:', typeof event.data); | |
| return; | |
| } | |
| const message = JSON.parse(event.data) as { type: string }; |
🤖 Prompt for AI Agents
In src/app/dashboard/messages/[conversationId]/page.tsx around line 42, the code
unsafely asserts WebSocket event.data as string before JSON.parse; replace the
assertion with runtime handling: check if typeof event.data === "string" and
parse directly, else if event.data is a Blob call await event.data.text(), else
if it's an ArrayBuffer decode it to a string (e.g., using TextDecoder), then
wrap JSON.parse in try/catch and validate the parsed object shape (ensure it has
a type string) before using it to avoid runtime errors from binary payloads or
invalid JSON.
| ws.close(); | ||
| }; | ||
| }, [conversationId, refetch]); | ||
| }, [currentUser?.name, refetch]); |
There was a problem hiding this comment.
Remove refetch from dependencies to prevent reconnection loops.
Including refetch in the dependency array can cause the WebSocket to reconnect whenever refetch is called, potentially creating an infinite loop since the WebSocket's onmessage handler itself calls refetch() (line 45).
The WebSocket should only reconnect when the user changes:
- }, [currentUser?.name, refetch]);
+ }, [currentUser?.name]);Note: You can safely call refetch inside the effect without listing it as a dependency since it's a stable function from React Query.
📝 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.
| }, [currentUser?.name, refetch]); | |
| }, [currentUser?.name]); |
🤖 Prompt for AI Agents
In src/app/dashboard/messages/[conversationId]/page.tsx around line 61, the
effect currently lists refetch in its dependency array which causes the
WebSocket to reconnect whenever refetch is called (leading to possible
reconnection loops because onmessage calls refetch). Remove refetch from the
dependency array so the effect only depends on currentUser?.name (or other truly
changing values like conversationId), and keep calling refetch() inside the
effect/onmessage as needed (refetch is a stable React Query function so it does
not need to be listed). If your linter flags this, add a focused
eslint-disable-next-line comment for the specific rule on that line and a short
justification.
| const handleSendMessage = () => { | ||
| if (!content.trim() || !otherUser?.name) return; | ||
|
|
||
| // First, persist the message in our database via tRPC | ||
| sendMessage.mutate( | ||
| { | ||
| conversationId: conversationId as string, | ||
| content, | ||
| }, | ||
| { | ||
| onSuccess: () => { | ||
| socket.emit('message', conversationId, content); | ||
| // After successful persistence, send it via WebSocket for real-time delivery | ||
| const messagePayload = { | ||
| to_username: otherUser.name, | ||
| content: content, | ||
| }; | ||
| socketRef.current?.send(JSON.stringify(messagePayload)); | ||
| setContent(''); | ||
| }, | ||
| onError: (error) => { | ||
| console.error("Failed to send message:", error); | ||
| } | ||
| } | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Check WebSocket connection state before sending.
The code sends messages via WebSocket without checking if the connection is open. If the socket is connecting, closing, or closed, send() will throw an error or silently fail.
Add a ready state check:
{
onSuccess: () => {
// After successful persistence, send it via WebSocket for real-time delivery
+ if (socketRef.current?.readyState !== WebSocket.OPEN) {
+ console.warn('WebSocket not connected, message persisted but not sent in real-time');
+ setContent('');
+ return;
+ }
const messagePayload = {
to_username: otherUser.name,
content: content,
};
socketRef.current?.send(JSON.stringify(messagePayload));
setContent('');
},🤖 Prompt for AI Agents
In src/app/dashboard/messages/[conversationId]/page.tsx around lines 63 to 87,
the onSuccess handler sends over the WebSocket without checking connection state
which can throw or silently fail; update the handler to first verify
socketRef.current exists and its readyState is OPEN (WebSocket.OPEN or numeric
1) before calling send, and if not open, either queue the message/retry or
log/notify the user that the message was persisted but not delivered in real
time; keep the persistence via tRPC, add a clear log or UI feedback path for the
non-open socket case, and avoid calling send when readyState is
CONNECTING/CLOSING/CLOSED.
| const user = await ctx.db.user.findUnique({ where: { clerkUid: ctx.user.id }}); | ||
| if (!user) throw new TRPCError({ code: "NOT_FOUND", message: "User not found." }); | ||
|
|
||
| return ctx.db.conversation.findMany({ | ||
| where: { | ||
| User: { | ||
| some: { | ||
| id: ctx.user.id, | ||
| id: user.id, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider moving user lookup to middleware for performance.
This pattern adds an extra database query to every protected endpoint. If the goal is to map Clerk's authentication ID to your database user record, consider implementing this lookup once in a custom middleware procedure (extending protectedProcedure) and adding the database user to the context. This would eliminate repeated queries and centralize the error handling.
Example middleware approach:
// In src/server/api/trpc.ts
const enforceUserExists = t.middleware(async ({ ctx, next }) => {
if (!ctx.user) {
throw new TRPCError({ code: "UNAUTHORIZED" });
}
const dbUser = await ctx.db.user.findUnique({
where: { clerkUid: ctx.user.id }
});
if (!dbUser) {
throw new TRPCError({ code: "NOT_FOUND", message: "User not found." });
}
return next({
ctx: {
...ctx,
dbUser, // Now available in all procedures
},
});
});
export const authedProcedure = t.procedure
.use(enforceUserIsAuthed)
.use(enforceUserExists);Then use authedProcedure instead of protectedProcedure in chat router to access ctx.dbUser.id directly.
🤖 Prompt for AI Agents
In src/server/api/routers/chat.ts around lines 7-14 the handler performs a
per-request DB lookup to map Clerk's ctx.user.id to your User row; move this
lookup into a reusable middleware so it runs once and populates the context. Add
a middleware (e.g., enforceUserExists) in src/server/api/trpc.ts that checks
ctx.user, queries ctx.db.user.findUnique({ where: { clerkUid: ctx.user.id } }),
throws if missing, and returns next() with ctx.dbUser set; then compose it into
your exported authedProcedure (after existing auth middleware). Finally update
chat router to use authedProcedure instead of protectedProcedure and replace the
local query with ctx.dbUser.id (and remove the duplicate error handling).
|
@Tobshub can I go ahead and deal with the CodeRabbit suggestions? |
2 participants
Summary by CodeRabbit
New Features
UI/UX Improvements