You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hey, nice work on this! The pagination logic is clean and the closeRoom cleanup is a good touch. Left a few things worth looking at before merging:
Deleted validation tests
The validation tests for sendMessage(undefined), empty content, and createRoom('') got removed. Looks like they were dropped to make room for the pagination tests, but those error cases are important to keep — can you add them back alongside the new ones?
Silent failure in editMessage
If the message isn't in the store (e.g., sent before this version was deployed), the in-memory update skips silently while the socket event still fires. Worth at least logging a warning or noting this as a known edge case.
Memory growth roomMessages has no cap per room. A high-traffic room that stays open for a long time will keep accumulating messages until the process restarts. Even a simple max (e.g., last 1000 messages) would help.
No upper bound on limit
A caller can pass limit=999999 and get the full history in one shot. Worth capping it at something reasonable like 100.
Multi-process deployments
Since this is an in-memory Map, it won't work correctly if the library is used in a clustered/multi-worker setup — each process would have its own isolated store. Worth calling this out in the README so users aren't caught off guard.
Minor
const { authMiddleware } = quickSocket is imported in the test file but never used
The README could mention that page 1 returns the most recent messages — the reverse-chronological ordering is the right call for a chat app, just not obvious at first glance
Overall the feature is solid for single-process use. The main things I'd want resolved before merging are restoring the validation tests and documenting the multi-process limitation. The rest can be follow-ups if needed.
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
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.
Summary
getRoomMessages(roomId, page, limit)