Skip to content

feat(socket.io): leave multiple rooms at once#5424

Open
chojs23 wants to merge 1 commit into
socketio:mainfrom
chojs23:feat/leave-multiple-rooms-at-once
Open

feat(socket.io): leave multiple rooms at once#5424
chojs23 wants to merge 1 commit into
socketio:mainfrom
chojs23:feat/leave-multiple-rooms-at-once

Conversation

@chojs23

@chojs23 chojs23 commented Dec 1, 2025

Copy link
Copy Markdown

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 Dec 1, 2025
@diffray-bot

Copy link
Copy Markdown

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);
});
});

Copy link
Copy Markdown

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
Copy Markdown

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

2 participants