Skip to content

Conversation

@chojs23
Copy link

@chojs23 chojs23 commented Dec 1, 2025

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behavior

Can only leave single room with Socket.leave() method

New behavior

Server-side sockets can now leave several rooms in a single socket.leave([...]) call. The in-memory adapter (and tests) were updated so Adapter.del() accepts a Set, keeping server state consistent without per-room calls.

Other information (e.g. related issues)

#5391

@chojs23 chojs23 force-pushed the feat/leave-multiple-rooms-at-once branch from fc979d3 to 61ee20b Compare December 1, 2025 08:08
@chojs23 chojs23 changed the title feat: leave multiple rooms at once feat(socket.io): leave multiple rooms at once Dec 1, 2025
@diffray-bot
Copy link

Changes Summary

This PR extends the Socket.IO socket.leave() method to accept an array of rooms, allowing sockets to leave multiple rooms in a single call. The in-memory adapter's del() method is updated to accept either a single room or a Set of rooms, with corresponding tests added for both the adapter and socket levels.

Type: feature

Components Affected: socket.io-adapter, socket.io

Architecture Impact
  • Coupling: The Socket class now always passes a Set to the adapter's del() method, making the API more consistent with addAll() which already accepts a Set
  • Breaking Changes: The Adapter.del() signature changed from (id, room: Room) to (id, rooms: Room | Set), which may affect custom adapter implementations that override this method

Risk Areas: Custom adapter implementations that extend the base Adapter class will need to update their del() method signature, The Socket.leave() documentation example changed from chaining (.leave('room1').leave('room2')) to array syntax, which is a behavioral change in recommended usage

Suggestions
  • Consider documenting the breaking change for custom adapter implementers in the changelog
  • The delSockets() method in the adapter still iterates with forEach - consider updating it to use the new multi-room leave capability for consistency

Full review in progress... | Powered by diffray

Comment on lines +944 to +959
it("should leave multiple rooms at once", (done) => {
const io = new Server(0);
const client = createClient(io, "/");

io.on("connection", (socket) => {
Promise.resolve(socket.join(["room1", "room2"]))
.then(() => Promise.resolve(socket.leave(["room1", "room2"])))
.then(() => {
const adapter = io.of("/").adapter;
expect(adapter.rooms.has("room1")).to.be(false);
expect(adapter.rooms.has("room2")).to.be(false);
success(done, io, client);
})
.catch(done);
});
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 HIGH - Missing test coverage for single room parameter
Category: quality

Description:
The leave() method now accepts Room | Array<Room>, but the new test only covers array syntax. Single string usage is tested elsewhere in messaging-many.ts, but explicit coverage for backward compatibility would be valuable.

Suggestion:
Add a test case for leaving a single room using socket.leave("room1") syntax in the socket.ts test file to ensure backward compatibility is explicitly tested.

Confidence: 70%
Rule: test_new_parameter_coverage

@diffray-bot
Copy link

Review Summary

Free public review - Want AI code reviews on your PRs? Check out diffray.ai

Validated 10 issues: 4 kept (1 high test coverage gap, 1 high potential null reference, 2 medium code quality), 6 filtered (JSDoc nitpicks, false positive null assertions, minor test improvements)

Issues Found: 4

See 1 individual line comment(s) for details.

📊 2 unique issue type(s) across 4 location(s)

📋 Full issue list (click to expand)

🟠 HIGH - Missing test coverage for single room parameter

Category: quality

File: packages/socket.io/test/socket.ts:944-959

Description: The leave() method now accepts Room | Array<Room>, but the new test only covers array syntax. Single string usage is tested elsewhere in messaging-many.ts, but explicit coverage for backward compatibility would be valuable.

Suggestion: Add a test case for leaving a single room using socket.leave("room1") syntax in the socket.ts test file to ensure backward compatibility is explicitly tested.

Confidence: 70%

Rule: test_new_parameter_coverage


🟠 HIGH - Non-null assertion on optional property (3 occurrences)

Category: bug

📍 View all locations
File Description Suggestion Confidence
packages/socket.io-adapter/lib/in-memory-adapter.ts:520 The code accesses opts.except.has(room) without checking if opts.except is defined. According to... Add optional chaining: sessionRooms.every((room) => !opts.except?.has(room)) 92%
packages/socket.io-adapter/lib/in-memory-adapter.ts:156 The code iterates over this.sids.get(id) at line 156 after checking this.sids.has(id) at line 15... Store the result first for clarity and safety: `const rooms = this.sids.get(id); if (!rooms) return;... 65%
packages/socket.io-adapter/lib/in-memory-adapter.ts:355 The code iterates over this.rooms.get(room) at line 355 after checking this.rooms.has(room) at l... Store the result or use optional chaining: `const roomSet = this.rooms.get(room); if (!roomSet) cont... 65%

Rule: ts_non_null_assertion


Powered by diffray - Multi-Agent Code Review Agent

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.

2 participants