Skip to content

feat: add paginated room message history#11

Open
NandanPaT-eL wants to merge 2 commits into
Aaromalpm:mainfrom
NandanPaT-eL:feature/add-room-message-pagination
Open

feat: add paginated room message history#11
NandanPaT-eL wants to merge 2 commits into
Aaromalpm:mainfrom
NandanPaT-eL:feature/add-room-message-pagination

Conversation

@NandanPaT-eL
Copy link
Copy Markdown
Contributor

Summary

  • add in-memory message storage per room
  • add getRoomMessages(roomId, page, limit)
  • clear stored messages when a room is closed
  • add tests and README docs for pagination

@Aaromalpm
Copy link
Copy Markdown
Owner

please resolve the conflict.

@NandanPaT-eL
Copy link
Copy Markdown
Contributor Author

Have reviewed this as well.

@Aaromal2
Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants