Feature/pix 58 validate user join#49
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds centralized Socket.IO lobby utilities and introduces server-side validation when a user joins a lobby (banned/capacity), while simplifying draw/batch-draw socket handlers.
Changes:
- Added reusable Socket.IO helpers for lobby user count, user listing, and broadcasting.
- Validated lobby join attempts against banned users and lobby capacity before completing the join flow.
- Refactored draw/batch-draw handling to use
CanvasService.drawBatchand shared broadcast helpers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/utils/socketUtils.ts | Introduces shared Socket.IO room helpers (counts, user listing, broadcast wrappers). |
| server/src/sockets/index.ts | Uses new helpers; adds join validation and refactors draw/draw_batch event logic. |
| server/src/services/lobby.service.ts | Adds reusable validation methods for join access and lobby capacity. |
| server/src/services/canvas.service.ts | Adds drawBatch to process and persist multiple pixel updates efficiently. |
| server/src/middlewares/permissionMiddleware.ts | Reuses lobby access validation from LobbyService (but leaves an unused import). |
| client/src/views/PlayView.vue | Moves onUnmounted import to the top-level import section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| socket.join(lobbyName); // Optimistic Join | ||
|
|
||
| // 3. Send list of connected users to the new user | ||
| // We fetch sockets AFTER joining so the user is included in the list | ||
| const sockets = await io.in(lobbyName).fetchSockets(); | ||
| const users = sockets.map(s => s.data.user).filter(u => u); | ||
| socket.emit(CONFIG.EVENTS.SERVER.LOBBY_USERS, users); | ||
| const currentCount = getLobbyUserCount(io, lobbyName); | ||
| try { | ||
| LobbyService.validateCapacity(lobby, currentCount - 1); | ||
| } catch (e: any) { | ||
| socket.leave(lobbyName); | ||
| return socket.emit(CONFIG.EVENTS.SERVER.ERROR, { message: "Lobby is full" }); | ||
| } |
There was a problem hiding this comment.
Capacity validation uses currentCount - 1 after an optimistic socket.join(), which makes the intent (checking count before vs after the join) hard to follow and easy to get wrong later. Consider computing a clearly named countBeforeJoin (or validating using the post-join count and checking > maxCollaborators) to make the off-by-one behavior explicit.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.