Scope the all and withRooms parameters of the onStartup hook to namespace.
#591
Conversation
WalkthroughSwitched room and broadcast subjects from the top-level Changes
Sequence Diagram(s)sequenceDiagram
participant Startup
participant IO as Server (io)
participant NS as Namespace (ns)
participant nsCtx as NamespaceContext
participant Emitter
participant RoomSvc as RoomService
Note over Startup,IO: Old flow — server-scoped emissions
Startup->>IO: create emitter/rooms using Server (io)
IO->>Emitter: makeEmitter({ subject: io })
Emitter->>RoomSvc: server-scoped broadcast/rooms
rect rgb(220,240,250)
Note over Startup,NS: New flow — namespace-scoped emissions
Startup->>NS: initialize namespace
Startup->>nsCtx: construct namespace context using NS
nsCtx->>Emitter: makeEmitter({ subject: NS })
Emitter->>RoomSvc: namespace-scoped broadcast/rooms
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/system/issue590.spec.ts (2)
89-145: LGTM! Comprehensive test for namespace-scoped broadcast from startup hook.The test correctly verifies that
all.broadcastfrom theonStartuphook reaches clients in the namespace. The test setup, execution, and cleanup are all properly implemented.Consider adding a type annotation for
intervalRefto improve type safety:- let intervalRef; + let intervalRef: NodeJS.Timeout | undefined;
146-205: LGTM! Comprehensive test for namespace-scoped room broadcast from startup hook.The test correctly verifies that
withRooms().broadcastfrom theonStartuphook reaches clients in the specified room within the namespace. The test setup, execution, and cleanup are all properly implemented.Consider adding a type annotation for
intervalRefto improve type safety:- let intervalRef; + let intervalRef: NodeJS.Timeout | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/attach.ts(2 hunks)src/emission.ts(1 hunks)tests/system/issue590.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/attach.ts (1)
src/emission.ts (2)
makeRoomService(91-109)makeEmitter(65-89)
src/emission.ts (4)
example/example-client.ts (1)
Socket(40-40)src/async-api/websockets.ts (1)
Server(6-6)src/namespace.ts (1)
Namespace(14-22)src/remote-client.ts (1)
SomeRemoteSocket(6-10)
tests/system/issue590.spec.ts (2)
example/config.ts (1)
config(18-99)src/attach.ts (1)
attachSockets(16-129)
🔇 Additional comments (4)
src/attach.ts (2)
62-62: LGTM! Namespace-scoped room service.The change from
iotonscorrectly scopes the room service to the namespace instead of the entire server, ensuringwithRoomsoperations in theonStartuphook target only clients in this namespace.
71-71: LGTM! Namespace-scoped broadcast.The change from
iotonscorrectly scopes the broadcast emitter to the namespace instead of the entire server, ensuringall.broadcastoperations in theonStartuphook target only clients in this namespace.src/emission.ts (2)
61-63: LGTM! Type signature extended to support Namespace.The overload correctly adds
Namespaceto the broadcaster subject types, enabling namespace-scoped broadcasting operations.
70-70: LGTM! Runtime implementation updated.The implementation signature correctly includes
Namespacein the union, matching the overload signatures and enabling namespace instances to be used as broadcast subjects.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/system/issue590.spec.ts (2)
100-102: Remove unnecessary room join.The test is validating
all.broadcast, which broadcasts to all clients regardless of room membership. Joining "testRoom" is unnecessary and may confuse readers about what's being tested.Consider removing the room join:
hooks: { - onConnection: async ({ client }) => { - await client.join("testRoom"); - }, onStartup: async ({ all }) => {
89-205: Consider extracting shared test setup and teardown.The two new tests share significant boilerplate (server creation, client connection, cleanup). Extracting common logic into helper functions would reduce duplication and improve maintainability.
Example structure:
async function setupTestServer(config: Config) { const httpServer = http.createServer(); const io = new Server(); await attachSockets({ io, config, actions: [], target: httpServer }); await promisify(httpServer.listen.bind(httpServer, port))(); return { httpServer, io }; } async function createTestClient(path: string) { const clientSocket = ioClient(`http://localhost:${port}${path}`, { transports: ["websocket"], }); await vi.waitFor(() => assert(clientSocket.connected)); return clientSocket; } async function cleanupServer(io: Server, httpServer: http.Server) { await promisify(io.close.bind(io))(); if (httpServer.listening) { await promisify(httpServer.close.bind(httpServer))(); } }Then each test focuses on its unique configuration and assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/system/issue590.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/system/issue590.spec.ts (3)
src/async-api/websockets.ts (1)
Server(6-6)example/config.ts (1)
config(18-99)src/attach.ts (1)
attachSockets(16-129)
tests/system/issue590.spec.ts
Outdated
| test("should broadcast to all from startup hook using all.broadcast", async () => { | ||
| const httpServer = http.createServer(); | ||
| const io = new Server(); | ||
| let intervalRef: NodeJS.Timeout | undefined; |
There was a problem hiding this comment.
Ensure interval cleanup in all code paths.
If the test throws an error or assertion fails before line 142, the interval will continue running and may cause test pollution or resource leaks.
Wrap the test body in a try-finally block or store the interval reference and clear it in an afterEach hook:
+ let intervalRef: NodeJS.Timeout | undefined;
+
+ afterEach(() => {
+ if (intervalRef) clearInterval(intervalRef);
+ });
+
test("should broadcast to all from startup hook using all.broadcast", async () => {
const httpServer = http.createServer();
const io = new Server();
- let intervalRef: NodeJS.Timeout | undefined;
// ... test body ...
clientSocket.disconnect();
await promisify(io.close.bind(io))();
- clearInterval(intervalRef);
if (httpServer.listening)
await promisify(httpServer.close.bind(httpServer))();
});Or use a try-finally within the test itself.
Also applies to: 104-106, 142-142
🤖 Prompt for AI Agents
In tests/system/issue590.spec.ts around lines 92-92 (and also ensure coverage
for 104-106 and 142-142), the interval referenced by `intervalRef` may not be
cleared if the test throws or an assertion fails; wrap the test body in a
try-finally that clears the interval in the finally block (or register an
afterEach hook that checks and clears the intervalRef), ensuring intervalRef is
set before the finally/afterEach check and cleared with clearInterval to prevent
test pollution and resource leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/system/issue590.spec.ts (1)
104-106: Consider removing unnecessary room join.The
client.join("testRoom")call is not needed for this test sinceall.broadcastreaches all connected clients regardless of room membership. The room join is only relevant for the third test that useswithRooms.Apply this diff if you want to keep the test focused:
hooks: { - onConnection: async ({ client }) => { - await client.join("testRoom"); - }, onStartup: async ({ all }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/system/issue590.spec.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/system/issue590.spec.ts (2)
example/config.ts (1)
config(18-99)src/attach.ts (1)
attachSockets(16-129)
🔇 Additional comments (3)
tests/system/issue590.spec.ts (3)
5-5: LGTM: Import changes are appropriate.The addition of
ActionsFactoryto the imports supports the test cases below.
20-23: Good: Interval cleanup properly implemented.The
afterEachhook ensures the interval is cleaned up after each test, preventing resource leaks and test pollution. This addresses the previous review feedback.
149-206: LGTM: Test correctly validates namespace-scoped room broadcasts from startup hook.This test properly verifies that
withRooms().broadcast()from theonStartuphook reaches clients in the specified room within the namespace. The test structure is sound:
- Client joins
testRoomviaonConnectionhook- Startup hook broadcasts to
testRoomevery 100ms- Test confirms the client receives the broadcast
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/system/issue590.spec.ts (1)
104-106: Optional: Remove unnecessary room join.Since this test uses
all.broadcast(which broadcasts to all clients regardless of room membership), theclient.join("testRoom")call is unnecessary. Consider removing it to simplify the test.Apply this diff:
hooks: { - onConnection: async ({ client }) => { - await client.join("testRoom"); - }, onStartup: async ({ all }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/system/issue590.spec.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/system/issue590.spec.ts (1)
src/attach.ts (1)
attachSockets(16-129)
🔇 Additional comments (3)
tests/system/issue590.spec.ts (3)
20-23: Good practice: interval cleanup properly implemented.The
afterEachhook ensures intervals are cleared after each test, preventing resource leaks and test pollution. This addresses the previous review concern effectively.
137-148: LGTM: Test logic and cleanup are correct.The test properly validates that
all.broadcastfrom theonStartuphook reaches the connected client. The comment accurately describes the test scenario, and cleanup is thorough.
149-206: LGTM: Test correctly validates room-scoped broadcasting.This test properly validates that
withRooms().broadcastfrom theonStartuphook reaches clients in the specified room. The client correctly joins "testRoom" (required for room-based broadcasts), and all assertions are accurate.
|
Hi @RobinTail can you please review this PR? |
1 participant
Hello, apologies for this almost duplicate PR of #590.
Problem
The
allorwithRoomsemitter/broadcasters parameters of theonStartupdo not broadcast to any clients joined under a specific namespace.Solution
Scope the
allandwithRoomsparameters of theonStartuphook to namespace.Updated the system tests under issue#590 to include these cases as well.
Summary by CodeRabbit
New Features
Tests