Implemented the group Chat feature#228
Merged
MrImmortal09 merged 11 commits intoiiitl:mainfrom Apr 12, 2026
Merged
Conversation
Contributor
|
@Rohank3 is attempting to deploy a commit to the mrimmortal09's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a real-time group chat: Mongoose Message model, SSE stream endpoint, REST API (GET/POST/PATCH/DELETE) with auth and domain/ownership checks, client hook for SSE and CRUD + UI state, ChatWidget/ChatMessage UI integrated into layout, and middleware allowlist update for public GET messages. Changes
Sequence Diagram(s)sequenceDiagram
participant User as IIITL User (Browser)
participant Widget as ChatWidget (Client)
participant Hook as useChatMessages (Client)
participant SSE as /api/chat/stream (Server SSE)
participant API as /api/chat/messages (Server REST)
participant DB as MongoDB
User->>Widget: Open chat panel
Widget->>Hook: initialize(isOpen)
Hook->>API: GET /api/chat/messages
API->>DB: find messages + populate sender/replyTo
DB-->>API: messages list
API-->>Hook: 200 [messages]
Hook->>Widget: render messages
Widget->>SSE: open GET /api/chat/stream
SSE-->>Hook: event CONNECTED
SSE-->>Hook: HEARTBEAT (periodic)
SSE-->>Hook: chatUpdate events (NEW/UPDATE/DELETE)
User->>Widget: Type & press Enter
Widget->>Hook: handleSend()
Hook->>API: POST /api/chat/messages {content, replyTo?}
API->>DB: create Message
DB-->>API: saved message
API->>lib/eventEmitter: emit chatEmitter NEW_MESSAGE
SSE-->>Hook: chatUpdate NEW_MESSAGE
Hook->>Widget: append message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
middleware.ts (1)
73-78: Consider adding rate limiting for the chat messages endpoint.The
/api/chat/messagesendpoint is not included in therateLimiterMap, meaning there's no protection against spam or abuse. Given that the chat supports public reads and authenticated writes, consider adding rate limiting for POST requests.The PR description acknowledges this as a potential improvement.
Would you like me to propose a rate limiter configuration for the chat endpoint?
Also applies to: 147-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware.ts` around lines 73 - 78, publicApiEndpoints currently lists '/api/chat/messages' but there is no corresponding entry in rateLimiterMap, leaving the chat POST route unthrottled; add a rateLimiterMap entry keyed by '/api/chat/messages' (or the same route constant) with sensible limits for POST (e.g., tighter limits for unauthenticated/public writes and slightly relaxed for authenticated users) and ensure the middleware that selects the limiter (the code that consults rateLimiterMap) applies the POST-specific rule when req.method === 'POST'; update any tests or comments to reflect the new limiter configuration.app/api/chat/messages/[id]/route.ts (2)
96-99: Extract the deleted message marker to a shared constant.The marker string
'🚫 This message was deleted.'is hardcoded here and must match the check incomponents/chat/ChatMessage.tsx:143. Consider extracting it to a shared constant to prevent drift.♻️ Proposed constant extraction
Create a shared constants file:
// lib/constants.ts export const DELETED_MESSAGE_MARKER = '🚫 This message was deleted.'Then import and use it in both locations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/`[id]/route.ts around lines 96 - 99, Extract the hardcoded deleted message marker into a shared constant (e.g., export const DELETED_MESSAGE_MARKER = '🚫 This message was deleted.' in a new lib/constants.ts) and replace the literal in the message soft-delete logic (where message.content is set and message.isDeleted = true) and the check in ChatMessage (the check at components/chat/ChatMessage.tsx around the deleted-message conditional) to import and use DELETED_MESSAGE_MARKER so both places reference the same symbol.
64-66: Remove unusedreqparameter.The
reqparameter is declared but never used in the DELETE handler.♻️ Proposed fix
export async function DELETE( - req: Request, + _req: Request, { params }: { params: Promise<{ id: string }> } ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/`[id]/route.ts around lines 64 - 66, The DELETE handler declares an unused req parameter; remove it from the function signature and accept only the params object to eliminate the unused variable. Update the export async function DELETE signature to take a single argument matching { params }: { params: Promise<{ id: string }> } (i.e. export async function DELETE({ params }: { params: Promise<{ id: string }> }) ) and ensure any internal code references remain unchanged (look for DELETE and its use of params).app/api/chat/messages/route.ts (1)
9-25: Consider cursor-based pagination for scalability.Fetching and reversing 100 messages works for MVP, but as message volume grows, cursor-based pagination would be more efficient and enable infinite scroll. The PR description already notes pagination as a potential improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/route.ts` around lines 9 - 25, The GET handler fetches a fixed 100 messages with Message.find(...).sort(...).limit(100) and reverses them, which won't scale; change GET to accept cursor and limit query params and implement cursor-based pagination using a stable cursor (e.g., timestamp or _id) instead of offset: query Message.find({ timestamp: { $lt: cursor } }) (or _id for ties), sort by timestamp desc, apply the requested limit, populate sender/replyTo as before, and return the page plus a nextCursor (the last item's timestamp/_id) so the client can request the next page; remove the server-side reversal or clearly document order in the response.model/Message.ts (1)
14-26: Consider adding indexes and content length validation.The schema lacks indexes on frequently queried fields, and
contenthas no length limit, which could allow abuse.
- Add an index on
timestampsince messages are sorted by this field.- Consider a
maxlengthoncontentto prevent excessively large messages.♻️ Proposed schema improvements
const MessageSchema = new mongoose.Schema<IMessage>({ sender: { type: mongoose.Schema.Types.ObjectId, ref: 'User', required: true }, email: { type: String, required: true }, - content: { type: String, required: true }, + content: { type: String, required: true, maxlength: 2000 }, isEdited: { type: Boolean, default: false }, replyTo: { type: mongoose.Schema.Types.ObjectId, ref: 'Message', default: null, }, isDeleted: { type: Boolean, default: false }, timestamp: { type: Date, default: Date.now }, }) + +MessageSchema.index({ timestamp: -1 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/Message.ts` around lines 14 - 26, Update the MessageSchema definition to add a database index on the timestamp field and enforce a maximum length on the content field: add an index for timestamp (e.g., via MessageSchema.index({ timestamp: -1 }) or similar) so queries/sorts on timestamp are efficient, and add a maxlength validator to the content field in the MessageSchema (choose an appropriate limit like 5000–10000 chars) to prevent oversized messages; update the IMessage/content constraint accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/chat/messages/`[id]/route.ts:
- Around line 18-19: The PATCH (and similarly DELETE) handler must validate that
the incoming route param id is a valid MongoDB ObjectId before calling
Message.findById; update the code around extracting id (from params) to check
mongoose.Types.ObjectId.isValid(id) (or ObjectId.isValid) and return a 400
response when invalid, then proceed to call Message.findById(id) only for valid
ids to avoid CastError; apply the same validation logic to the DELETE handler
path as well.
In `@app/api/chat/messages/route.ts`:
- Around line 62-67: The code calls Message.create with replyTo possibly
containing an invalid MongoDB ObjectId, causing a CastError; before creating the
message, validate replyTo (e.g., using mongoose.Types.ObjectId.isValid or
equivalent) and only include replyTo in the create payload if it's present and
valid, otherwise set replyTo to null or reject with a 400; update the logic
around Message.create (the newMessage creation block) to perform this check and
handle invalid replyTo appropriately.
In `@app/chat/page.tsx`:
- Line 64: The error handling that does "if (!res.ok) throw new Error((await
res.json()).error)" can itself throw when the response body isn't JSON; update
the error extraction in the request handling (the branches around the failing
checks on response `res` in page.tsx) to attempt JSON parsing inside a
try/catch, and if JSON parsing fails fallback to await res.text() (or a generic
message), then throw a new Error that includes HTTP status and the extracted
message; apply the same safe-extraction pattern to both occurrences (the one at
the existing check and the other at the second check around line 77) so non-JSON
error responses are handled gracefully.
- Around line 125-127: When deleting a message, also clear any reply state that
references that message: in the branch that checks if (editingMessage?._id ===
id) { cancelAction() }, additionally check if replyingTo?._id === id (or the
replyingTo identifier used in state) and clear it (e.g., call the same
cancelAction or invoke setReplyingTo(null)/clearReplyingTo) so both
editingMessage and replyingTo are reset when the replied-to message is deleted;
update the handler where editingMessage, cancelAction and replyingTo are managed
to perform this extra clear.
In `@app/layout.jsx`:
- Line 29: ChatWidget is rendered in the root layout and duplicates polling with
ChatPage by both hitting /api/chat/messages; fix quickly by detecting the
current route inside ChatWidget (usePathname from next/navigation) and return
null when pathname === '/chat' so only ChatPage polls, or alternatively lift
message state into a shared context/provider or switch polling to SWR/React
Query for deduplication (apply change in the ChatWidget component where polling
and rendering occur).
In `@components/chat/ChatMessage.tsx`:
- Around line 53-58: The confirmDelete handler currently sets
setIsDeleting(true) then awaits onDelete(message._id) but doesn't guard against
exceptions, leaving isDeleting stuck; update confirmDelete to wrap the await in
a try/finally so that in finally you always call setIsDeleting(false) and
setShowDeleteModal(false) (i.e., setIsDeleting(true); try { await
onDelete(message._id) } finally { setIsDeleting(false);
setShowDeleteModal(false) }) to ensure consistent cleanup even if onDelete
throws.
- Around line 44-45: The component unsafely casts message.sender to Sender and
then accesses sender._id; change to first normalize and guard the sender value
(e.g., const senderObj = typeof message.sender === 'string' ? null :
message.sender) and compute isMe using a safe check (const isMe = !!senderObj &&
senderObj._id === currentUserId), then replace direct uses of sender with
senderObj and use optional chaining (senderObj?.image, senderObj?.name) or
sensible fallbacks wherever sender is used; update any code referring to sender,
MessageData, isMe, and currentUserId in ChatMessage.tsx to use these guarded
references.
In `@components/chat/ChatWidget.tsx`:
- Around line 9-146: ChatWidget duplicates business logic (polling,
send/edit/reply/delete handlers, state) already present in app/chat/page.tsx;
extract this shared logic into a custom hook (e.g., useChatMessages) and replace
the internal state and functions in ChatWidget (messages, inputText, isLoading,
error, editingMessage, replyingTo, the polling useEffect, handleSend,
handleDeleteMessage, startReply, startEdit, cancelAction) with the hook's
returned state and handler functions so both ChatWidget and ChatPage delegate to
the same implementation; ensure the hook also returns currentUserId and
isIIITLUser derived from useSession and accepts options like pollWhenOpen/isOpen
to preserve existing polling behavior.
- Around line 130-132: When a message being replied to is deleted, clear the
reply state instead of only cancelling edit: in the deletion handler where you
check if (editingMessage?._id === id) { cancelAction() } also check and clear
the replying state (e.g., set replyingTo to null or call the same helper used in
app/chat/page.tsx) so that the UI stops treating a deleted message as the reply
target; update the block referencing editingMessage?._id and id to call
cancelAction() and also reset replyingTo (or invoke the existing clearReply
helper) to fully clear reply state.
---
Nitpick comments:
In `@app/api/chat/messages/`[id]/route.ts:
- Around line 96-99: Extract the hardcoded deleted message marker into a shared
constant (e.g., export const DELETED_MESSAGE_MARKER = '🚫 This message was
deleted.' in a new lib/constants.ts) and replace the literal in the message
soft-delete logic (where message.content is set and message.isDeleted = true)
and the check in ChatMessage (the check at components/chat/ChatMessage.tsx
around the deleted-message conditional) to import and use DELETED_MESSAGE_MARKER
so both places reference the same symbol.
- Around line 64-66: The DELETE handler declares an unused req parameter; remove
it from the function signature and accept only the params object to eliminate
the unused variable. Update the export async function DELETE signature to take a
single argument matching { params }: { params: Promise<{ id: string }> } (i.e.
export async function DELETE({ params }: { params: Promise<{ id: string }> }) )
and ensure any internal code references remain unchanged (look for DELETE and
its use of params).
In `@app/api/chat/messages/route.ts`:
- Around line 9-25: The GET handler fetches a fixed 100 messages with
Message.find(...).sort(...).limit(100) and reverses them, which won't scale;
change GET to accept cursor and limit query params and implement cursor-based
pagination using a stable cursor (e.g., timestamp or _id) instead of offset:
query Message.find({ timestamp: { $lt: cursor } }) (or _id for ties), sort by
timestamp desc, apply the requested limit, populate sender/replyTo as before,
and return the page plus a nextCursor (the last item's timestamp/_id) so the
client can request the next page; remove the server-side reversal or clearly
document order in the response.
In `@middleware.ts`:
- Around line 73-78: publicApiEndpoints currently lists '/api/chat/messages' but
there is no corresponding entry in rateLimiterMap, leaving the chat POST route
unthrottled; add a rateLimiterMap entry keyed by '/api/chat/messages' (or the
same route constant) with sensible limits for POST (e.g., tighter limits for
unauthenticated/public writes and slightly relaxed for authenticated users) and
ensure the middleware that selects the limiter (the code that consults
rateLimiterMap) applies the POST-specific rule when req.method === 'POST';
update any tests or comments to reflect the new limiter configuration.
In `@model/Message.ts`:
- Around line 14-26: Update the MessageSchema definition to add a database index
on the timestamp field and enforce a maximum length on the content field: add an
index for timestamp (e.g., via MessageSchema.index({ timestamp: -1 }) or
similar) so queries/sorts on timestamp are efficient, and add a maxlength
validator to the content field in the MessageSchema (choose an appropriate limit
like 5000–10000 chars) to prevent oversized messages; update the
IMessage/content constraint accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8b4b6bc-cc1b-4db9-8b55-5e51d3642ac7
📒 Files selected for processing (9)
app/api/chat/messages/[id]/route.tsapp/api/chat/messages/route.tsapp/chat/page.tsxapp/layout.jsxcomponents/chat/ChatMessage.tsxcomponents/chat/ChatWidget.tsxcomponents/message.tsxmiddleware.tsmodel/Message.ts
💤 Files with no reviewable changes (1)
- components/message.tsx
anurag2787
previously requested changes
Apr 11, 2026
Collaborator
anurag2787
left a comment
anurag2787
left a comment
There was a problem hiding this comment.
@Rohank3 please address the coderabbitai comments and instead of using polling logic use websockets for the group chat
Collaborator
…nor bugs and refactored chat to Real-Time SSE
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/chat/messages/`[id]/route.ts:
- Around line 24-54: The edit/delete handlers currently only check
authentication and ownership but not the domain-based write rule; add a guard
that rejects PATCH/DELETE unless session.user.email endsWith('@iiitl.ac.in') (or
has an admin flag if your app uses one). Locate the authorization block around
Message.findById / message.sender checks in route.ts and, before applying
updates or deletes, validate session.user.email (and/or session.user.isAdmin)
and return 403 with a clear error like "Read-only: write access restricted to
`@iiitl.ac.in`" when the domain check fails; apply the same change in the DELETE
handler and any similar blocks cited around lines 104-129.
In `@app/api/chat/messages/route.ts`:
- Around line 53-57: The POST handler checks session?.user?.email but later uses
session.user.id without guaranteeing it exists; update the auth guard in the
route using getServerSession(authOptions) to also verify session?.user?.id (or
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })) before any
use of session.user.id, similar to the PATCH route's check, so any downstream
references to session.user.id are safe; locate the guard around the
getServerSession/authOptions call and the code that later reads session.user.id
and add the extra null/undefined check.
In `@app/api/chat/stream/route.ts`:
- Around line 33-34: The current use of a process-local EventEmitter
(chatEmitter) with chatEmitter.on('chatUpdate', onChatUpdate) won't work across
multiple server instances; replace it with a production-safe broadcast layer
(e.g., Redis Pub/Sub, Pusher, or Postgres LISTEN/NOTIFY). Update the stream
route to subscribe to a shared pub/sub channel (e.g., "chatUpdate") using the
global/shared pubsub client instead of chatEmitter and call the existing
onChatUpdate handler when messages arrive; likewise update the parts of the
codebase that currently call chatEmitter.emit to publish to the same pub/sub
channel. Ensure subscription/unsubscription and connection lifecycle are handled
(subscribe on stream open, unsubscribe/close on disconnect) and reuse the same
pubsub client across requests.
- Around line 20-30: The payload parameters currently typed as any in sendEvent
and onChatUpdate should be replaced with a concrete stream event type: define or
import an interface/type (e.g., StreamEvent) that matches the SSE payload shape
and use it for the function signatures (sendEvent(data: StreamEvent) and
onChatUpdate(data: StreamEvent)); ensure JSON.stringify still accepts the
structure and update any related places that emit or construct the event to
conform to StreamEvent so the linter/typechecker is satisfied.
In `@app/chat/page.tsx`:
- Around line 119-150: The composer controls lack accessible names: add an
aria-label to the text input that reflects editing state (e.g., when
editingMessage is true use "Edit your message", otherwise "Type a message to the
group") and ensure the input uses inputRef as before; add explicit aria-label
attributes to the icon-only Buttons (the cancel button that calls cancelAction
should use "Cancel edit" or "Close composer" and the send Button/handleSend
should use "Send message"), and ensure the send Button also reflects disabled
state via aria-disabled when isLoading or inputText.trim() is empty; you can
alternatively add visually-hidden <label> elements and/or title attributes but
prefer aria-labels on the input and both Buttons (Send, cancel) so screen
readers get clear names.
In `@hooks/useChatMessages.ts`:
- Around line 55-118: The SSE is recreated whenever replyingTo changes causing
missed events; stop re-creating the EventSource on replyingTo and instead keep
the EventSource lifecycle tied only to isOpen, and on each SSE open/reconnect
reconcile state by fetching the current message list. Concretely: remove
replyingTo from the useEffect dependency array in hooks/useChatMessages.ts so
the effect depends only on isOpen, move EventSource creation/cleanup to that
effect, and add an eventSource.onopen (or onerror→reconnect handler) that calls
a new or existing fetch endpoint (e.g., fetchLatestMessages or GET
/api/chat/list) to call setMessages(...) and dedupe, and keep the existing logic
that clears replyingTo when a DELETE_MESSAGE arrives.
- Around line 96-102: The delete handler only clears replyingTo; also clear the
edit mode when the deleted message is the one being edited by checking if
editingMessage?._id === data.message._id and then call the edit state resetters
(e.g., setEditingMessage(null) and reset edit text via setEditText('') or
equivalent). Update the DELETE_MESSAGE branch in the same hook (near
replyingTo/setReplyingTo) to clear editingMessage and any edit-related state so
the UI leaves edit mode when a message is deleted remotely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37b5110a-c61e-4374-8dc2-94b65d7790c6
📒 Files selected for processing (8)
app/api/chat/messages/[id]/route.tsapp/api/chat/messages/route.tsapp/api/chat/stream/route.tsapp/chat/page.tsxcomponents/chat/ChatMessage.tsxcomponents/chat/ChatWidget.tsxhooks/useChatMessages.tslib/eventEmitter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/eventEmitter.ts
- components/chat/ChatWidget.tsx
- components/chat/ChatMessage.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
hooks/useChatMessages.ts (2)
86-102: Minor typo in comment.Line 88 has "upate" instead of "update".
✏️ Fix typo
- // Also upate any nested replyTo content if the parent was updated/deleted + // Also update any nested replyTo content if the parent was updated/deleted🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useChatMessages.ts` around lines 86 - 102, The comment above the reply-update block in useChatMessages.ts has a typo: change "Also upate any nested replyTo content if the parent was updated/deleted" to "Also update any nested replyTo content if the parent was updated/deleted" so the comment above the logic that updates updatedMsg.replyTo (references: updatedMsg, replyTo, data.message) is spelled correctly.
213-219: Consider consistent error handling instead ofalert().The
handleDeleteMessagefunction usesalert()for error display, whilehandleSenduses inline error state. For consistent UX, consider setting theerrorstate instead.♻️ Proposed improvement
} catch (err: unknown) { if (err instanceof Error) { - alert(err.message) + setError(err.message) } else { - alert('An error occurred') + setError('An error occurred') } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useChatMessages.ts` around lines 213 - 219, handleDeleteMessage currently uses alert(...) for errors while handleSend uses the component's error state; replace the alert usage with the same error state flow. In the catch block inside handleDeleteMessage (the try/catch shown), call the existing setError (or add one named setError if missing) with err instanceof Error ? err.message : 'An error occurred', and ensure any UI that displays the error reuses the same error state used by handleSend so UX is consistent. Also remove the alert calls so all error presentation is done via the shared error state.components/chat/ChatMessage.tsx (1)
169-174: Hardcoded sentinel string creates fragile coupling with backend.The deleted message sentinel
'🚫 This message was deleted.'is duplicated here and inapp/api/chat/messages/[id]/route.ts(line 151). If either changes independently, the UI will break. Consider extracting to a shared constant.♻️ Proposed improvement
Create a shared constant, e.g., in
lib/constants.ts:export const DELETED_MESSAGE_SENTINEL = '🚫 This message was deleted.'Then import and use in both locations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/chat/ChatMessage.tsx` around lines 169 - 174, The UI is tightly coupled to a hardcoded deleted-message sentinel string used in ChatMessage.tsx (the conditional on message.replyTo.content) which is duplicated in the backend route; extract that sentinel into a shared exported constant (e.g., DELETED_MESSAGE_SENTINEL) in a common module (like lib/constants.ts), replace the literal checks in ChatMessage.tsx (the conditional that compares message.replyTo.content to '🚫 This message was deleted.') with an import of DELETED_MESSAGE_SENTINEL, and update the backend route (route.ts) to import and use the same constant so both sides reference a single source of truth.app/api/chat/stream/route.ts (1)
65-69: Remove unused error variable to satisfy linter.The catch block intentionally ignores the error, but the linter still flags the unused
_closeErr. Consider using an empty catch or removing the variable.♻️ Proposed fix
try { writer.close() - } catch (_closeErr) { + } catch { /* Writer may already be closed; safe to ignore. */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/stream/route.ts` around lines 65 - 69, The catch block around writer.close() in route.ts declares an unused variable (_closeErr) which the linter flags; update the try/catch to use an empty catch (i.e., remove the `_closeErr` identifier) so the error is intentionally ignored—locate the writer.close() call and change "catch (_closeErr) { ... }" to a plain "catch { ... }" preserving the existing comment.app/api/chat/messages/route.ts (1)
22-34: Consider sorting ascending directly instead of reversing.The current approach fetches messages sorted descending, limits to 100, then reverses. Sorting ascending directly with proper limiting would be more efficient.
♻️ Proposed optimization
- // Fetch latest 100 messages - const messages = await Message.find() - .sort({ timestamp: -1 }) + // Count total messages to compute skip for latest 100 + const total = await Message.countDocuments() + const skip = Math.max(0, total - 100) + + const messages = await Message.find() + .sort({ timestamp: 1 }) + .skip(skip) .limit(100) .populate('sender', 'name image email') .populate({ path: 'replyTo', populate: { path: 'sender', select: 'name' }, }) .exec() - // Return messages in chronological order (oldest first) - return NextResponse.json(messages.reverse(), { status: 200 }) + return NextResponse.json(messages, { status: 200 })Alternatively, keep the current approach if simplicity is preferred - the performance difference is negligible for 100 messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/route.ts` around lines 22 - 34, The current code fetches messages sorted descending then reverses (Message.find().sort({ timestamp: -1 }).limit(100) ... .reverse()), which can be replaced by sorting ascending directly to avoid the in-memory reverse: compute total = await Message.countDocuments(queryFilter), set skip = Math.max(0, total - 100), then run Message.find(queryFilter).sort({ timestamp: 1 }).skip(skip).limit(100).populate(...).exec() so the result comes back oldest-first without needing Array.prototype.reverse; update the code around Message.find(), .sort(), .limit(), and the reverse logic accordingly (or keep current approach if you prefer simplicity).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/chat/stream/route.ts`:
- Around line 10-13: Update the ChatEventPayload interface so its type union
uses 'UPDATE_MESSAGE' instead of 'EDIT_MESSAGE' to match the emitted event and
consumers; locate the interface ChatEventPayload and replace 'EDIT_MESSAGE' with
'UPDATE_MESSAGE' so the union becomes: 'CONNECTED' | 'HEARTBEAT' | 'NEW_MESSAGE'
| 'UPDATE_MESSAGE' | 'DELETE_MESSAGE', ensuring type consistency with the
emitter in the message route and the hooks consumer (e.g., useChatMessages.ts).
---
Nitpick comments:
In `@app/api/chat/messages/route.ts`:
- Around line 22-34: The current code fetches messages sorted descending then
reverses (Message.find().sort({ timestamp: -1 }).limit(100) ... .reverse()),
which can be replaced by sorting ascending directly to avoid the in-memory
reverse: compute total = await Message.countDocuments(queryFilter), set skip =
Math.max(0, total - 100), then run Message.find(queryFilter).sort({ timestamp: 1
}).skip(skip).limit(100).populate(...).exec() so the result comes back
oldest-first without needing Array.prototype.reverse; update the code around
Message.find(), .sort(), .limit(), and the reverse logic accordingly (or keep
current approach if you prefer simplicity).
In `@app/api/chat/stream/route.ts`:
- Around line 65-69: The catch block around writer.close() in route.ts declares
an unused variable (_closeErr) which the linter flags; update the try/catch to
use an empty catch (i.e., remove the `_closeErr` identifier) so the error is
intentionally ignored—locate the writer.close() call and change "catch
(_closeErr) { ... }" to a plain "catch { ... }" preserving the existing comment.
In `@components/chat/ChatMessage.tsx`:
- Around line 169-174: The UI is tightly coupled to a hardcoded deleted-message
sentinel string used in ChatMessage.tsx (the conditional on
message.replyTo.content) which is duplicated in the backend route; extract that
sentinel into a shared exported constant (e.g., DELETED_MESSAGE_SENTINEL) in a
common module (like lib/constants.ts), replace the literal checks in
ChatMessage.tsx (the conditional that compares message.replyTo.content to '🚫
This message was deleted.') with an import of DELETED_MESSAGE_SENTINEL, and
update the backend route (route.ts) to import and use the same constant so both
sides reference a single source of truth.
In `@hooks/useChatMessages.ts`:
- Around line 86-102: The comment above the reply-update block in
useChatMessages.ts has a typo: change "Also upate any nested replyTo content if
the parent was updated/deleted" to "Also update any nested replyTo content if
the parent was updated/deleted" so the comment above the logic that updates
updatedMsg.replyTo (references: updatedMsg, replyTo, data.message) is spelled
correctly.
- Around line 213-219: handleDeleteMessage currently uses alert(...) for errors
while handleSend uses the component's error state; replace the alert usage with
the same error state flow. In the catch block inside handleDeleteMessage (the
try/catch shown), call the existing setError (or add one named setError if
missing) with err instanceof Error ? err.message : 'An error occurred', and
ensure any UI that displays the error reuses the same error state used by
handleSend so UX is consistent. Also remove the alert calls so all error
presentation is done via the shared error state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46b54fa8-d917-4f06-a94c-f571287c9bef
📒 Files selected for processing (8)
app/api/chat/messages/[id]/route.tsapp/api/chat/messages/route.tsapp/api/chat/stream/route.tsapp/chat/page.tsxcomponents/chat/ChatMessage.tsxcomponents/chat/ChatWidget.tsxhooks/useChatMessages.tsmodel/Message.ts
✅ Files skipped from review due to trivial changes (1)
- components/chat/ChatWidget.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- model/Message.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/api/chat/messages/[id]/route.ts (1)
172-200: Consider short-circuiting if the message is already deleted.The DELETE handler doesn't check
message.isDeletedbefore applying the soft-delete. While this is idempotent, it results in an unnecessary database write and emits a duplicateDELETE_MESSAGESSE event to all clients.♻️ Proposed fix
const message = await Message.findById(id) if (!message) { return NextResponse.json({ error: 'Message not found' }, { status: 404 }) } + /* ── Already deleted — return success without re-saving ── */ + if (message.isDeleted) { + return NextResponse.json({ success: true }, { status: 200 }) + } + /* * Ownership / admin gate: the user must either be the original sender * or hold the 'admin' role.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/`[id]/route.ts around lines 172 - 200, The DELETE handler should short-circuit when a message is already soft-deleted to avoid unnecessary writes and duplicate SSE events: after fetching the message with Message.findById(id) check message.isDeleted and return a 204 (or appropriate) response immediately if true, skipping the ownership check only if you still want to enforce permissions otherwise you can still verify sender/admin before returning; otherwise, if not deleted, proceed to set message.content = '🚫 This message was deleted.', message.isDeleted = true, await message.save(), and emit the DELETE_MESSAGE SSE event as before.app/api/chat/stream/route.ts (1)
123-128: Remove or rename unused catch variable to satisfy linter.The static analysis flags
'_closeErr' is defined but never used. While the underscore prefix conventionally indicates intentional non-use, the linter still warns. Either omit the binding entirely (ES2019+) or use a comment directive.🧹 Proposed fix
try { writer.close() - } catch (_closeErr) { + } catch { /* Writer may already be closed by the runtime; safe to ignore. */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/stream/route.ts` around lines 123 - 128, The catch clause after the writer.close() call uses an unused bound variable "_closeErr" which triggers the linter; update the catch to remove the unused binding (use "catch { ... }") or replace it with an explicit linter directive/comment so the linter is satisfied. Locate the try/catch around writer.close() in route.ts (the try { writer.close() } catch (_closeErr) { ... } block) and either change it to "catch { /* Writer may already be closed by the runtime; safe to ignore. */ }" or add a single-line eslint/tslint disable comment inside the catch to suppress the unused-variable warning.components/chat/ChatMessage.tsx (1)
248-255: Consider extracting the tombstone string as a shared constant.The tombstone check
'🚫 This message was deleted.'is hardcoded here and also inapp/api/chat/messages/[id]/route.ts(line 198). Extracting this to a shared constant would prevent drift and make updates easier.♻️ Example extraction
// lib/constants.ts export const DELETED_MESSAGE_TOMBSTONE = '🚫 This message was deleted.'Then import and use in both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/chat/ChatMessage.tsx` around lines 248 - 255, Extract the hardcoded tombstone string into a shared exported constant (e.g. DELETED_MESSAGE_TOMBSTONE) and replace the inline literal in ChatMessage (component ChatMessage.tsx, the conditional reading message.replyTo.content) with an import of that constant; also update the other usage in app/api/chat/messages/[id]/route.ts to import and use the same constant from the new module (e.g. lib/constants.ts) so both files reference a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/chat/ChatWidget.tsx`:
- Around line 189-196: The icon-only cancel button in ChatWidget's reply/edit
banner lacks an accessible label; update the Button component (the one rendering
XCircle and using cancelAction) to include an aria-label (e.g., "Cancel reply"
or "Cancel edit" as appropriate) so screen readers can identify the control;
ensure the aria-label text matches the wording used elsewhere (ChatPage) for
consistency.
- Around line 214-220: The icon-only send Button in ChatWidget.tsx (the Button
wrapping the Send icon and calling handleSend) lacks an accessible name; add
aria-label="Send message" to that Button component so screen readers announce it
(matching the full-page chat behavior) while keeping the existing disabled and
className props intact.
- Around line 202-213: The chat input lacks an accessible label; update the
input (the JSX element using ref={inputRef} and props value={inputText},
onChange={e => setInputText(e.target.value)}, onKeyDown={handleKeyDown},
disabled={isLoading}) to include an aria-label (or aria-labelledby) so screen
readers can identify it — e.g. set aria-label to a dynamic string that mirrors
the placeholder (use editingMessage ? 'Edit your message' : 'Type a message') to
preserve context when editing; ensure the label text matches the pattern used in
app/chat/page.tsx for consistency.
---
Nitpick comments:
In `@app/api/chat/messages/`[id]/route.ts:
- Around line 172-200: The DELETE handler should short-circuit when a message is
already soft-deleted to avoid unnecessary writes and duplicate SSE events: after
fetching the message with Message.findById(id) check message.isDeleted and
return a 204 (or appropriate) response immediately if true, skipping the
ownership check only if you still want to enforce permissions otherwise you can
still verify sender/admin before returning; otherwise, if not deleted, proceed
to set message.content = '🚫 This message was deleted.', message.isDeleted =
true, await message.save(), and emit the DELETE_MESSAGE SSE event as before.
In `@app/api/chat/stream/route.ts`:
- Around line 123-128: The catch clause after the writer.close() call uses an
unused bound variable "_closeErr" which triggers the linter; update the catch to
remove the unused binding (use "catch { ... }") or replace it with an explicit
linter directive/comment so the linter is satisfied. Locate the try/catch around
writer.close() in route.ts (the try { writer.close() } catch (_closeErr) { ... }
block) and either change it to "catch { /* Writer may already be closed by the
runtime; safe to ignore. */ }" or add a single-line eslint/tslint disable
comment inside the catch to suppress the unused-variable warning.
In `@components/chat/ChatMessage.tsx`:
- Around line 248-255: Extract the hardcoded tombstone string into a shared
exported constant (e.g. DELETED_MESSAGE_TOMBSTONE) and replace the inline
literal in ChatMessage (component ChatMessage.tsx, the conditional reading
message.replyTo.content) with an import of that constant; also update the other
usage in app/api/chat/messages/[id]/route.ts to import and use the same constant
from the new module (e.g. lib/constants.ts) so both files reference a single
source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac42dd63-d482-409b-8168-2c0977d17165
📒 Files selected for processing (9)
app/api/chat/messages/[id]/route.tsapp/api/chat/messages/route.tsapp/api/chat/stream/route.tsapp/chat/page.tsxcomponents/chat/ChatMessage.tsxcomponents/chat/ChatWidget.tsxhooks/useChatMessages.tslib/eventEmitter.tsmodel/Message.ts
✅ Files skipped from review due to trivial changes (1)
- model/Message.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/eventEmitter.ts
- app/api/chat/messages/route.ts
- hooks/useChatMessages.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Update the description as well. It still mentions the older changes. |
Contributor
|
I am yet not completely convinced with the quality and working of this PR but we are moving with this for now. Below are some most important changes you have to make before requesting the next review:
|
Contributor
Author
I added those comments for the docstring coverage will it be fine if it is below 80%? |
Collaborator
Docstrings are required for the functions which are going to be used somewhere else as well. Yeah its fine below 80% as well. |
Contributor
Author
I've made the required changes based on the review. |
MrImmortal09
approved these changes
Apr 12, 2026
Collaborator
There was a problem hiding this comment.
@Rohank3 I am merging this, there is still a delay in chats when hosted to a vercel, might be the vercel issue or might be some bug in code.
If this still presists, make a new issue(type: bug) regards same.
Nice work, hope so you have learned a few new things.
Collaborator
|
@coderabbitai full-review |
Contributor
|
✅ Actions performedFull review triggered. |
Collaborator
|
Giving +5 for testing efforts, PS: he tested with different laptops as well. |
Collaborator
|
Could have given 10 but code seemed a lot of ai generated and review was a bit long. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware.ts (1)
73-81:⚠️ Potential issue | 🟠 MajorAdd
/api/chat/streamto public GET endpoints.The
/api/chat/messagesendpoint is correctly made public, but/api/chat/stream(the SSE endpoint) is currently blocked by the authentication middleware. Unauthenticated users can fetch messages but cannot receive real-time updates—the middleware will reject the request before reaching the handler. If read-only users should receive live chat updates, add/api/chat/streamto bothpublicApiEndpoints(line 73) andpublicGetEndpoints(line 147):Proposed fix
const publicApiEndpoints = [ '/api/papers', '/api/quick-reads', '/api/quick_read_categories', '/api/chat/messages', + '/api/chat/stream', ]const publicGetEndpoints = [ '/api/papers', '/api/quick-reads', '/api/quick_read_categories', '/api/chat/messages', + '/api/chat/stream', ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware.ts` around lines 73 - 81, The middleware currently treats GETs to '/api/chat/stream' as authenticated-only; add '/api/chat/stream' to the publicApiEndpoints array and to the publicGetEndpoints array so the SSE handler can be reached by unauthenticated read-only users; update the arrays named publicApiEndpoints and publicGetEndpoints (and any isPublicEndpoint usage that checks those arrays) to include the '/api/chat/stream' path.
🧹 Nitpick comments (8)
model/Message.ts (1)
51-70: Add an index ontimestampfor efficient query performance.The
GET /api/chat/messagesendpoint sorts bytimestamp: -1and limits to 100. Without an index, MongoDB performs a collection scan. As the message count grows, this will degrade performance.♻️ Proposed fix
const MessageSchema = new mongoose.Schema<IMessage>({ /** Reference to the User document representing the message author. */ sender: { type: mongoose.Schema.Types.ObjectId, ref: 'User', required: true }, /** Denormalised email of the sender for quick domain validation. */ email: { type: String, required: true }, /** The text body of the message; replaced with tombstone on soft delete. */ content: { type: String, required: true }, /** Whether the message has been edited after initial creation. */ isEdited: { type: Boolean, default: false }, /** Optional self-reference to the parent message in a reply chain. */ replyTo: { type: mongoose.Schema.Types.ObjectId, ref: 'Message', default: null, }, /** Soft-delete flag; when true, the message is rendered as deleted. */ isDeleted: { type: Boolean, default: false }, /** Creation timestamp; auto-set to the current date on insert. */ timestamp: { type: Date, default: Date.now }, }) + +// Index for efficient chronological queries (GET /api/chat/messages) +MessageSchema.index({ timestamp: -1 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/Message.ts` around lines 51 - 70, Add a descending index on the MessageSchema.timestamp field to support the GET /api/chat/messages sort and limit; locate the MessageSchema definition (symbol: MessageSchema) and add an index declaration such as MessageSchema.index({ timestamp: -1 }) so queries sorting by timestamp: -1 use the index and avoid collection scans.lib/eventEmitter.ts (1)
36-36: Consider increasing EventEmitter max listeners to avoid warnings.The default
EventEmittermax listeners limit is 10. With multiple simultaneous SSE connections (one per browser tab/client), you may hit this limit and seeMaxListenersExceededWarningin logs.♻️ Proposed fix
export const chatEmitter = globalForEvents.chatEmitter || new EventEmitter() +chatEmitter.setMaxListeners(100) // Support many concurrent SSE connections🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/eventEmitter.ts` at line 36, The global chatEmitter is created with new EventEmitter() and may hit the default 10-listener limit causing MaxListenersExceededWarning; update the creation (symbol: chatEmitter) to set a higher limit (or unlimited) via chatEmitter.setMaxListeners(N) immediately after instantiation (or before use) so multiple SSE connections/tabs won't trigger warnings, and keep the globalForEvents reuse logic intact to avoid recreating emitters.app/api/chat/messages/route.ts (2)
101-107: Consider adding a maximum content length validation.There's no limit on message content length. Very long messages could impact database storage, UI rendering, and SSE payload sizes. A reasonable limit (e.g., 2000-5000 characters) would prevent abuse.
♻️ Proposed enhancement
+ const MAX_MESSAGE_LENGTH = 2000 + /* ── Input validation: content must be a non-empty trimmed string ── */ if (!content || !content.trim()) { return NextResponse.json( { error: 'Message content is required' }, { status: 400 } ) } + + if (content.trim().length > MAX_MESSAGE_LENGTH) { + return NextResponse.json( + { error: `Message content exceeds maximum length of ${MAX_MESSAGE_LENGTH} characters` }, + { status: 400 } + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/route.ts` around lines 101 - 107, Add a maximum length check to the input validation that currently verifies content and content.trim() in route.ts: after trimming the content variable (used in the existing block around the comment "Input validation: content must be a non-empty trimmed string"), enforce a max length (e.g., 5000 chars) and return NextResponse.json({ error: 'Message content too long' }, { status: 400 }) if exceeded; ensure you use the same trimmed content value for both empty and length checks so the function handling messages uses the validated, trimmed string consistently.
99-99: Wrapreq.json()in try-catch for clearer error responses.If the request body contains malformed JSON,
req.json()throws and the outer catch returns a generic 500 error. A 400 Bad Request with a descriptive message would be more helpful for API consumers.♻️ Proposed fix
- const { content, replyTo } = await req.json() + let content: string, replyTo: string | undefined + try { + const body = await req.json() + content = body.content + replyTo = body.replyTo + } catch { + return NextResponse.json( + { error: 'Invalid JSON in request body' }, + { status: 400 } + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/route.ts` at line 99, The request body parsing at the line using const { content, replyTo } = await req.json() must be wrapped in a try-catch so malformed JSON yields a 400 Bad Request with a descriptive message instead of a generic 500; catch the error around req.json(), log or include a short message tied to the parse failure, and return a Response (status 400) explaining the JSON parse error, ensuring the handler (the POST/route handler where req.json() is called) returns early on parse failure and does not proceed using content or replyTo.app/api/chat/messages/[id]/route.ts (2)
193-200: Double-delete emits duplicate SSE events.If a message is already soft-deleted, calling DELETE again will overwrite the same fields and emit another
DELETE_MESSAGEevent. While not breaking, it causes unnecessary SSE broadcasts. Consider adding an early return ifmessage.isDeletedis alreadytrue.♻️ Optional: skip redundant delete
+ /* Already deleted — no-op to avoid duplicate SSE events */ + if (message.isDeleted) { + return NextResponse.json({ success: true }, { status: 200 }) + } + /* * Soft delete: replace the content with a tombstone indicator... */ message.content = '🚫 This message was deleted.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/`[id]/route.ts around lines 193 - 200, The DELETE handler currently overwrites soft-delete fields and emits a DELETE_MESSAGE SSE even when the message is already soft-deleted; add an early no-op return to avoid duplicate broadcasts by checking message.isDeleted at the start of the delete flow (e.g., if (message.isDeleted) return/respond 204), so skip setting message.content, message.isDeleted, calling message.save(), and emitting the DELETE_MESSAGE event when the flag is already true; update the handler in route.ts where message.isDeleted, message.save(), and the DELETE_MESSAGE emission occur to implement this guard.
53-57: Malformed JSON returns 500 instead of 400.If
req.json()throws due to invalid JSON in the request body, the outercatchblock returns a generic 500 error. Consider wrapping the JSON parse to return a clearer 400 response for client errors.♻️ Proposed fix for JSON parsing
const { id } = await params if (!mongoose.Types.ObjectId.isValid(id)) { return NextResponse.json({ error: 'Invalid message ID' }, { status: 400 }) } - const { content } = await req.json() + + let content: string + try { + const body = await req.json() + content = body.content + } catch { + return NextResponse.json({ error: 'Invalid JSON body' }, { status: 400 }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/chat/messages/`[id]/route.ts around lines 53 - 57, The route handler currently awaits req.json() directly and if the body is malformed the outer catch returns a 500; wrap the JSON parsing so you can detect and handle syntax errors: when extracting const { content } from req.json() (in the same handler that reads const { id } and validates it with mongoose.Types.ObjectId.isValid), perform the JSON parse in a try/catch (or use .catch on the promise) and return NextResponse.json({ error: 'Invalid JSON body' }, { status: 400 }) on parse failures instead of letting the error bubble to the generic 500 handler.hooks/useChatMessages.ts (2)
130-150: Consider surfacing fetch failures to the UI.When the initial message fetch fails, the error is only logged to console. Users see an empty chat with no indication of failure. Consider setting the
errorstate or adding a retry mechanism so users know something went wrong.♻️ Optional: surface fetch errors
const fetchMessages = async () => { try { const res = await fetch('/api/chat/messages') if (res.ok) { const data = await res.json() setMessages(data) + } else { + setError('Failed to load messages') } } catch (err) { console.error('Failed to fetch messages:', err) + setError('Failed to load messages') } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useChatMessages.ts` around lines 130 - 150, The fetchMessages routine inside the useEffect (triggered when isOpen changes) currently only logs failures to console; update it to set and clear a component-level error state (e.g., useError / setError) so the UI can render an error message and offer retry; specifically, in the fetchMessages catch block call setError(err) and when fetch succeeds clear the error via setError(null), and add a retry handler (e.g., retryFetch which calls fetchMessages) that the UI can wire to a "Retry" button or toast.
339-363: Consider using error state instead ofalert()for consistency.
handleDeleteMessageusesalert()for error feedback, whilehandleSenduses theerrorstate. Usingalert()is disruptive and blocks the UI. For consistency, consider using the same error state pattern.♻️ Proposed change to use error state
} catch (err: unknown) { if (err instanceof Error) { - alert(err.message) + setError(err.message) } else { - alert('An error occurred') + setError('Failed to delete message') } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useChatMessages.ts` around lines 339 - 363, handleDeleteMessage currently calls alert(...) on errors; change it to use the same error state used by handleSend instead. Replace the alert branches in handleDeleteMessage's catch with a call to setError(err instanceof Error ? err.message : 'An error occurred'), and ensure any existing error state (e.g., error / setError) is imported/available in this hook; remove the alert calls so error rendering is consistent with handleSend. Keep the rest of the function (editingMessage, cancelAction, replyingTo, setReplyingTo) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/chat/stream/route.ts`:
- Around line 123-127: The catch block around writer.close() is declaring an
unused variable '_closeErr' and causing a lint warning; update the catch to use
an empty-catch pattern (remove the '_closeErr' identifier) so the code reads
catch { /* intentionally ignored */ } while leaving the try { writer.close() }
intact to silently ignore runtime-closed writers.
In `@components/chat/ChatMessage.tsx`:
- Around line 206-223: The Next Image usage in ChatMessage.tsx (Image rendering
sender.image) requires allowing Google profile images; update next.config.js by
adding a remotePatterns entry permitting protocol 'https', hostname
'lh3.googleusercontent.com' and pathname '/**' to the images.remotePatterns
array so Next.js will load sender.image from that domain.
In `@components/chat/ChatWidget.tsx`:
- Around line 114-119: The floating action Button (the Button element that calls
setIsOpen on click and renders the MessageCircle icon) is icon-only and missing
an accessible name; add an aria-label (for example aria-label="Open chat" or a
localized equivalent) to the Button so screen readers announce its purpose, and
consider adding aria-haspopup="dialog" if the chat opens a drawer/dialog for
additional accessibility context.
- Around line 132-134: The close Button currently renders an icon-only control
(Button with onClick={() => setIsOpen(false)} and X) without an accessible
label; update the Button component instance to include an aria-label (e.g.,
aria-label="Close chat" or a localized equivalent) so screen readers can
announce its purpose, ensuring the accessible name matches the visual action and
leaving the onClick and setIsOpen(false) behavior unchanged.
---
Outside diff comments:
In `@middleware.ts`:
- Around line 73-81: The middleware currently treats GETs to '/api/chat/stream'
as authenticated-only; add '/api/chat/stream' to the publicApiEndpoints array
and to the publicGetEndpoints array so the SSE handler can be reached by
unauthenticated read-only users; update the arrays named publicApiEndpoints and
publicGetEndpoints (and any isPublicEndpoint usage that checks those arrays) to
include the '/api/chat/stream' path.
---
Nitpick comments:
In `@app/api/chat/messages/`[id]/route.ts:
- Around line 193-200: The DELETE handler currently overwrites soft-delete
fields and emits a DELETE_MESSAGE SSE even when the message is already
soft-deleted; add an early no-op return to avoid duplicate broadcasts by
checking message.isDeleted at the start of the delete flow (e.g., if
(message.isDeleted) return/respond 204), so skip setting message.content,
message.isDeleted, calling message.save(), and emitting the DELETE_MESSAGE event
when the flag is already true; update the handler in route.ts where
message.isDeleted, message.save(), and the DELETE_MESSAGE emission occur to
implement this guard.
- Around line 53-57: The route handler currently awaits req.json() directly and
if the body is malformed the outer catch returns a 500; wrap the JSON parsing so
you can detect and handle syntax errors: when extracting const { content } from
req.json() (in the same handler that reads const { id } and validates it with
mongoose.Types.ObjectId.isValid), perform the JSON parse in a try/catch (or use
.catch on the promise) and return NextResponse.json({ error: 'Invalid JSON body'
}, { status: 400 }) on parse failures instead of letting the error bubble to the
generic 500 handler.
In `@app/api/chat/messages/route.ts`:
- Around line 101-107: Add a maximum length check to the input validation that
currently verifies content and content.trim() in route.ts: after trimming the
content variable (used in the existing block around the comment "Input
validation: content must be a non-empty trimmed string"), enforce a max length
(e.g., 5000 chars) and return NextResponse.json({ error: 'Message content too
long' }, { status: 400 }) if exceeded; ensure you use the same trimmed content
value for both empty and length checks so the function handling messages uses
the validated, trimmed string consistently.
- Line 99: The request body parsing at the line using const { content, replyTo }
= await req.json() must be wrapped in a try-catch so malformed JSON yields a 400
Bad Request with a descriptive message instead of a generic 500; catch the error
around req.json(), log or include a short message tied to the parse failure, and
return a Response (status 400) explaining the JSON parse error, ensuring the
handler (the POST/route handler where req.json() is called) returns early on
parse failure and does not proceed using content or replyTo.
In `@hooks/useChatMessages.ts`:
- Around line 130-150: The fetchMessages routine inside the useEffect (triggered
when isOpen changes) currently only logs failures to console; update it to set
and clear a component-level error state (e.g., useError / setError) so the UI
can render an error message and offer retry; specifically, in the fetchMessages
catch block call setError(err) and when fetch succeeds clear the error via
setError(null), and add a retry handler (e.g., retryFetch which calls
fetchMessages) that the UI can wire to a "Retry" button or toast.
- Around line 339-363: handleDeleteMessage currently calls alert(...) on errors;
change it to use the same error state used by handleSend instead. Replace the
alert branches in handleDeleteMessage's catch with a call to setError(err
instanceof Error ? err.message : 'An error occurred'), and ensure any existing
error state (e.g., error / setError) is imported/available in this hook; remove
the alert calls so error rendering is consistent with handleSend. Keep the rest
of the function (editingMessage, cancelAction, replyingTo, setReplyingTo)
unchanged.
In `@lib/eventEmitter.ts`:
- Line 36: The global chatEmitter is created with new EventEmitter() and may hit
the default 10-listener limit causing MaxListenersExceededWarning; update the
creation (symbol: chatEmitter) to set a higher limit (or unlimited) via
chatEmitter.setMaxListeners(N) immediately after instantiation (or before use)
so multiple SSE connections/tabs won't trigger warnings, and keep the
globalForEvents reuse logic intact to avoid recreating emitters.
In `@model/Message.ts`:
- Around line 51-70: Add a descending index on the MessageSchema.timestamp field
to support the GET /api/chat/messages sort and limit; locate the MessageSchema
definition (symbol: MessageSchema) and add an index declaration such as
MessageSchema.index({ timestamp: -1 }) so queries sorting by timestamp: -1 use
the index and avoid collection scans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 483411df-3341-4bc9-b9a5-122e1a2da6ce
📒 Files selected for processing (12)
app/api/chat/messages/[id]/route.tsapp/api/chat/messages/route.tsapp/api/chat/stream/route.tsapp/chat/page.tsxapp/layout.jsxcomponents/chat/ChatMessage.tsxcomponents/chat/ChatWidget.tsxcomponents/message.tsxhooks/useChatMessages.tslib/eventEmitter.tsmiddleware.tsmodel/Message.ts
💤 Files with no reviewable changes (1)
- components/message.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #24
Description
Implements a real-time group chat system for IIIT Lucknow students as described in the issue.
This includes:
Features implemented:
Backend:
Implemented:
Frontend:
Live Demo (if any)
Note for Maintainer
Checkout
Summary by CodeRabbit
New Features
Other