Add WebSocket adapter support and enhance setup options #135
Conversation
🦋 Changeset detectedLatest commit: 24ad959 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
dependencies
WalkthroughAdds an Adapter interface and plumbing for multi‑instance WebSocket pub/sub, updates setupWebSocketServer to accept an optional adapter, exposes adapter APIs and globals, patches server init to pass an adapter, and adds a Redis-backed example, tests, CI workflow, and workspace/dependency changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Dev / Custom Server
participant Next as NextNodeServer
participant HTTP as HTTP Server
participant WSS as WebSocketServer
participant Ad as Adapter (Redis pub/sub)
Dev->>Ad: new RedisAdapter(REDIS_URL)
Dev->>Next: create NextNodeServer()
Dev->>WSS: create WebSocketServer(noServer)
Dev->>Next: setupWebSocketServer(Next, { adapter: Ad })
Dev->>HTTP: httpServer.listen(PORT)
Note over Next,WSS: Upgrade handler registers route‑scoped client sets and adapter subscriptions
sequenceDiagram
autonumber
participant C1 as Client (Instance A)
participant WS1 as WS Server (Inst A)
participant Ad as Adapter (Redis)
participant WS2 as WS Server (Inst B)
participant C2 as Client (Instance B)
C1->>WS1: send(message, room)
WS1->>Ad: broadcast(room, message)
Ad-->>WS1: onMessage(room, message)
Ad-->>WS2: onMessage(room, message)
WS1-->>C1: deliver(message)
WS2-->>C2: deliver(message)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Possibly related PRs
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 |
|
should be good now, tests are skipped if redis is not detected, it will be still tested via github actions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
examples/redis-adapter/global.js (1)
1-1: Consider using a more defensive approach for the global binding.Directly assigning to
globalThis.AsyncLocalStoragecan conflict with user code that may already define this property or rely on AsyncLocalStorage from a different source. The require statement also lacks error handling.Apply this diff to add a conditional check and error handling:
-globalThis.AsyncLocalStorage = require('node:async_hooks').AsyncLocalStorage; +if (!globalThis.AsyncLocalStorage) { + try { + globalThis.AsyncLocalStorage = require('node:async_hooks').AsyncLocalStorage; + } catch (err) { + console.error('Failed to load AsyncLocalStorage:', err); + } +}examples/redis-adapter/app/layout.tsx (1)
1-17: Consider extracting the duplicated layout to a shared module.This Layout component is identical to those in
examples/chat-room/app/layout.tsx,examples/base-path/app/layout.tsx, andexamples/custom-server/app/layout.tsx. Extracting it to a shared location (e.g.,examples/_shared/src/layout.tsx) would eliminate duplication and simplify maintenance.examples/redis-adapter/app/(simple)/page.tsx (1)
6-8: Use protocol-aware WebSocket URL construction.The hardcoded
ws://protocol won't work correctly in production environments using HTTPS, wherewss://is required. Usewindow.location.protocolto construct the appropriate WebSocket URL.Apply this diff to make the WebSocket URL protocol-aware:
const [messages, sendMessage] = useMessaging( - () => `ws://${window.location.host}/api/ws`, + () => { + const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:'; + return `${protocol}//${window.location.host}/api/ws`; + }, );tests/redis-adapter.test.ts (1)
91-112: Replace fixed sleeps with Playwright’s auto-waiting expectationsHard sleeps make tests flaky/slow. Prefer locator-based waits (toContainText/toHaveText), which auto-retry until assertions pass.
Example change (apply similarly to other sleeps):
// Instead of: await page1.click('button[type=submit]'); await sleep(1500); const page2LastMessage = await page2.textContent('li:last-child'); expect(page2LastMessage).toContain('Hello from instance 1!'); // Prefer: await page1.click('button[type=submit]'); await expect(page2.locator('li:last-child')).toContainText('Hello from instance 1!');Also consider waiting for welcome text directly:
await expect(page1.locator('li:first-child')).toContainText('instance-1'); await expect(page2.locator('li:first-child')).toContainText('instance-2');Also applies to: 148-167, 184-207
src/server/setup.ts (2)
147-151: Clean up empty route sets to prevent unbounded Map growthWhen the last client leaves, delete the entry to avoid memory creep in long-lived servers with many rooms.
Apply this diff:
- client.once('close', () => { - if (adapter) { - routeClients.get(pathname)?.delete(client); - } - }); + client.once('close', () => { + if (adapter) { + const set = routeClients.get(pathname); + if (!set) return; + set.delete(client); + if (set.size === 0) routeClients.delete(pathname); + } + });
51-53: No per-room unsubscribe path → risk of long‑lived subscriptions
adapter.onMessageis registered per room with no way to unsubscribe when no clients remain. Over time (dynamic routes) this can leak subs on the adapter backend.Consider extending the Adapter interface with
offMessage(room, handler)and calling it when a room becomes empty. Alternatively, haveonMessagereturn a disposer you can invoke on cleanup. This keeps Redis (or any backend) subscription cardinality bounded.Also applies to: 94-111, 157-164
examples/redis-adapter/server.ts (2)
27-32: Avoid one 'message' listener per room; consolidate and route to handlersEach
onMessagecall adds a new'message'listener, which can lead to listener explosion as rooms grow. Prefer a single listener that dispatches by channel, and keep per-room handler sets.Example refactor:
class RedisAdapter implements Adapter { private pub = new Redis(process.env.REDIS_URL || 'redis://localhost:6379'); private sub = new Redis(process.env.REDIS_URL || 'redis://localhost:6379'); private handlers = new Map<string, Set<(message: unknown) => void>>(); private wired = false; private wireOnce() { if (this.wired) return; this.wired = true; this.sub.on('message', (channel: string, msg: string) => { const hs = this.handlers.get(channel); if (!hs) return; for (const h of hs) h(msg); }); } onMessage(room: string, handler: (message: unknown) => void): void { this.wireOnce(); if (!this.handlers.has(room)) this.handlers.set(room, new Set()); this.handlers.get(room)!.add(handler); // Optionally await to surface errors and ensure sub is active before use void this.sub.subscribe(room); } }Optionally, return a disposer to remove the handler and
unsubscribewhen the set becomes empty.
27-29: Consider awaiting subscribe to surface errors
this.sub.subscribe(room)returns a promise; awaiting helps fail fast if the subscription cannot be established.await this.sub.subscribe(room);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/yellow-pugs-melt.md(1 hunks).github/workflows/redis-adapter-tests.yml(1 hunks)examples/redis-adapter/app/(simple)/api/ws/route.ts(1 hunks)examples/redis-adapter/app/(simple)/page.tsx(1 hunks)examples/redis-adapter/app/layout.tsx(1 hunks)examples/redis-adapter/global.js(1 hunks)examples/redis-adapter/next-env.d.ts(1 hunks)examples/redis-adapter/package.json(1 hunks)examples/redis-adapter/server.ts(1 hunks)examples/redis-adapter/tsconfig.json(1 hunks)package.json(1 hunks)playwright.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)src/patches/patch-1.ts(1 hunks)src/server/helpers/adapter.ts(1 hunks)src/server/index.ts(1 hunks)src/server/persistent.ts(1 hunks)src/server/setup.ts(6 hunks)tests/redis-adapter.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
examples/redis-adapter/app/layout.tsx (3)
examples/chat-room/app/layout.tsx (1)
Layout(1-17)examples/base-path/app/layout.tsx (1)
Layout(1-17)examples/custom-server/app/layout.tsx (1)
Layout(1-17)
.changeset/yellow-pugs-melt.md (1)
examples/base-path/app/(simple)/api/ws/route.ts (1)
UPGRADE(10-53)
examples/redis-adapter/app/(simple)/page.tsx (3)
examples/_shared/src/chat-room/messaging.ts (1)
useMessaging(11-64)examples/_shared/src/chat-room/message-list.tsx (1)
MessageList(5-40)examples/_shared/src/chat-room/message-submit.tsx (1)
MessageSubmit(8-48)
tests/redis-adapter.test.ts (1)
tests/custom-server.test.ts (5)
code(85-102)test(57-103)test(15-55)code(74-83)page1(30-37)
src/server/persistent.ts (1)
src/server/index.ts (2)
getAdapter(5-5)setAdapter(8-8)
src/server/setup.ts (1)
src/server/helpers/adapter.ts (1)
Adapter(28-47)
examples/redis-adapter/server.ts (1)
src/server/helpers/adapter.ts (1)
Adapter(28-47)
🔇 Additional comments (14)
playwright.config.ts (1)
33-33: LGTM!The comment appropriately clarifies why the Redis adapter tests are not included in the
webServerarray, improving code maintainability.examples/redis-adapter/next-env.d.ts (1)
1-6: LGTM!Standard Next.js TypeScript declaration file. The reference directives and non-editable notice align with Next.js conventions.
.github/workflows/redis-adapter-tests.yml (1)
1-57: LGTM!The workflow is well-structured with:
- Proper Redis service configuration including health checks (lines 18-22)
- Efficient browser installation (only Chromium at line 43)
- Appropriate environment variable configuration (lines 47-49)
- Sensible artifact retention policy (7 days at line 57)
The workflow effectively tests Redis adapter functionality in CI.
examples/redis-adapter/package.json (2)
1-20: Package configuration looks good.The use of workspace and catalog specifiers is appropriate for the monorepo structure. Dependencies are correctly declared.
5-5: global.js presence and initialisation verified
The fileexamples/redis-adapter/global.jsexists and correctly initialisesglobalThis.AsyncLocalStoragefromnode:async_hooks. No further action required..changeset/yellow-pugs-melt.md (1)
1-5: LGTM!The changeset correctly identifies this as a minor version bump with an appropriate description of the new WebSocket adapter feature for multi-instance deployments.
pnpm-workspace.yaml (1)
10-10: LGTM!Adding ioredis 5.8.1 to the workspace catalog is correct. Based on learnings, this is the latest stable version of ioredis with full TypeScript support and comprehensive Redis features including pub/sub required for the adapter implementation.
src/server/index.ts (1)
1-13: LGTM!The new exports properly expose the Adapter interface and related accessors (
getAdapter,setAdapter) along withSetupOptionstype, establishing a clean public API for the WebSocket adapter functionality.package.json (1)
72-72: ioredis should remain a devDependencyioredis is only imported in example/test code (examples/redis-adapter/server.ts) and never used under src, so leaving it in devDependencies is correct.
src/patches/patch-1.ts (1)
24-25: LGTM!The adapter retrieval and conditional passing logic is correct. Optional chaining ensures safe access, and the ternary properly constructs the options object only when an adapter is present.
src/server/persistent.ts (1)
96-120: LGTM!The adapter accessor implementation follows the established pattern for other server components. The use of
useGlobalwith proper typing and JSDoc comments is consistent and correct.examples/redis-adapter/app/(simple)/api/ws/route.ts (1)
10-11: Clarify the purpose of awaiting headers().Line 11 awaits
headers()but doesn't use the returned value. If this is intentional (e.g., to ensure async context is properly established), consider adding a comment explaining why. Otherwise, remove the unused await.Is this await necessary for establishing the request context in Next.js WebSocket routes?
src/server/helpers/adapter.ts (1)
1-47: LGTM!The Adapter interface is well-designed and thoroughly documented. The JSDoc example clearly demonstrates the intended usage pattern with Redis pub/sub, and the method signatures are appropriately typed for async operations.
src/server/setup.ts (1)
89-112: Verify and prevent duplicate broadcasts with adapter enabledRoute handlers may broadcast on client
message, and the adapter’sonMessagerebroadcasts the same payload back to the originating instance—resulting in duplicated sends locally. Verify your UPGRADE/SOCKET handlers don’t perform local broadcasts when an adapter is present, or else:
- Centralise all broadcasts via the adapter (documented contract for routes),
- Add envelope-based de-dup (e.g. include an
instanceIdto suppress local echo), or- Introduce a SetupOptions flag to toggle local echo vs adapter relay.
|
@apteryxxyz is this feature missing something ? |
|
See requested changes from coderabbit |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/redis-adapter.test.ts (2)
59-64: Consider polling over fixed delays for more reliable test timing.The additional 1000ms sleep after server readiness assumes WebSocket initialization time. While this may work, tests would be more reliable and potentially faster with polling logic (e.g., attempting a WebSocket connection and retrying with exponential backoff).
78-211: Consider replacing fixed sleep durations with polling for robustness.The tests use fixed
sleep()durations (1000ms for connections, 1500ms for message propagation) which can be fragile under load or slow Redis instances. More robust alternatives include:
- Playwright's
waitForSelector()with text matching- Custom polling with retry logic and timeout
Example:
// Instead of sleep(1500) after sending await page.waitForSelector('li:last-child:has-text("Hello from instance 1!")', { timeout: 5000 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/redis-adapter/tsconfig.json(1 hunks)src/server/setup.ts(6 hunks)tests/redis-adapter.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: apteryxxyz
Repo: apteryxxyz/next-ws PR: 150
File: .changeset/mk4lP2zgn0Nq9.md:1-5
Timestamp: 2025-11-30T18:15:16.883Z
Learning: In the apteryxxyz/next-ws repository, pull requests that bump Next.js version ranges (e.g., updating to 16.0.6) are created by an automated workflow that triggers when new Next.js versions are released. When reviewing such PRs, assume the Next.js version referenced has already been released and is available.
📚 Learning: 2025-08-22T12:23:23.227Z
Learnt from: apteryxxyz
Repo: apteryxxyz/next-ws PR: 113
File: src/server/helpers/module.ts:29-32
Timestamp: 2025-08-22T12:23:23.227Z
Learning: Next.js builds pages to CommonJS format, so using require() to load built pages via nextServer.getPagePath() is appropriate and ESM compatibility concerns don't apply in this context.
Applied to files:
src/server/setup.ts
📚 Learning: 2025-08-22T05:11:55.871Z
Learnt from: apteryxxyz
Repo: apteryxxyz/next-ws PR: 112
File: src/server/route/match.ts:3-9
Timestamp: 2025-08-22T05:11:55.871Z
Learning: In next-ws route matching, the route patterns passed to compileRoutePattern come from Next.js internal appPathRoutes (filesystem-based route definitions), which are developer-controlled. User input (requestPathname) is matched against these compiled regexes, not used to construct them, so ReDoS concerns don't apply to the regex construction itself.
Applied to files:
src/server/setup.ts
📚 Learning: 2025-08-22T12:10:01.850Z
Learnt from: apteryxxyz
Repo: apteryxxyz/next-ws PR: 113
File: src/patches/helpers/define.ts:3-4
Timestamp: 2025-08-22T12:10:01.850Z
Learning: TSUP bundler resolves path aliases like '~/' during compilation when creating single-file bundles, so alias configuration is not required in the build config for this use case.
Applied to files:
examples/redis-adapter/tsconfig.json
🧬 Code graph analysis (1)
src/server/setup.ts (1)
src/server/helpers/adapter.ts (1)
Adapter(28-47)
🔇 Additional comments (11)
examples/redis-adapter/tsconfig.json (1)
1-55: LGTM! Well-configured TypeScript setup for Next.js.The configuration follows Next.js best practices with appropriate strict settings, Bundler module resolution, and safety checks. The compiler options are well-suited for a modern Next.js 15 project.
src/server/setup.ts (7)
16-18: Good addition of type-safe setup options.The
SetupOptionsinterface with optional adapter field provides clean configuration extensibility for multi-instance WebSocket support.
38-44: Clear differentiation between single and multi-instance modes.The conditional logging appropriately communicates whether the adapter is configured, helping operators understand the deployment mode.
51-52: Appropriate route-scoped client tracking.The
routeClientsmap enables per-route adapter subscriptions and targeted broadcasting, which is correct for multi-instance pub/sub.
89-111: Well-structured adapter subscription with correct WebSocket state check.The per-route subscription logic is efficient (subscribes once when first client connects) and the broadcast handler correctly uses
WebSocket.OPENconstant to filter ready clients. Good use of optional chaining for defensive programming.
129-136: Correct message interception for cross-instance broadcasting.Client messages are properly routed through the adapter with appropriate error handling, ensuring multi-instance message propagation.
147-151: Proper client cleanup on disconnect.The close handler correctly removes disconnected clients from the route-specific set, preventing memory leaks.
157-164: Appropriate adapter lifecycle management.The adapter cleanup on server close ensures graceful shutdown with proper error handling and logging.
tests/redis-adapter.test.ts (3)
11-27: Effective Redis availability check for conditional test execution.The helper properly tests Redis connectivity with appropriate connection options and cleanup, allowing tests to skip gracefully when Redis is unavailable.
214-253: Well-implemented server readiness check.The helper properly monitors stdout for the ready signal with appropriate timeout handling and cleanup, ensuring reliable server startup detection.
255-276: Properly handles process exit races with early exit check.The helper correctly checks if the process has already exited before setting up listeners, avoiding the race condition where exit happens before the listener attaches. The use of
once()and defensive SIGKILL logic ensure reliability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/server/setup.ts (2)
4-5: Minor typing tidy‑up forrouteClientsYou already import
WebSocketfromws, butrouteClientsis typed withSet<import('ws').WebSocket>. For consistency and easier future refactors, you could reuse the imported type:-import { WebSocketServer, WebSocket } from 'ws'; +import { WebSocketServer, WebSocket } from 'ws'; … - // Store route -> clients mapping for adapter broadcasts - const routeClients = new Map<string, Set<import('ws').WebSocket>>(); + // Store route -> clients mapping for adapter broadcasts + const routeClients = new Map<string, Set<WebSocket>>();Purely cosmetic, but keeps the typing story a bit neater.
Also applies to: 51-52
89-111: Consider cleaning up empty route client sets to avoid unbounded growthThe adapter integration and per‑route client tracking are nicely structured, and the
adapter.close()hook on HTTP server shutdown is a good lifecycle safeguard. One small concern: when the last client for apathnamedisconnects, you remove it from theSetbut keep an emptySetinrouteClients(and the adapter subscription remains active for that room). With many dynamic routes/rooms over time, this can lead to unbounded growth of empty entries.You can cheaply clean up empty sets on close:
- client.once('close', () => { - if (adapter) { - routeClients.get(pathname)?.delete(client); - } - }); + client.once('close', () => { + if (!adapter) return; + const clients = routeClients.get(pathname); + if (!clients) return; + clients.delete(client); + if (clients.size === 0) { + routeClients.delete(pathname); + } + });This keeps the in‑memory map bounded even for high‑churn dynamic rooms, while still playing nicely with your existing
adapter.onMessagecallback (which already copes withrouteClients.get(pathname)returningundefined).Also applies to: 129-136, 147-151, 157-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/redis-adapter/tsconfig.json(1 hunks)src/server/setup.ts(6 hunks)tests/redis-adapter.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/redis-adapter.test.ts
- examples/redis-adapter/tsconfig.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: apteryxxyz
Repo: apteryxxyz/next-ws PR: 150
File: .changeset/mk4lP2zgn0Nq9.md:1-5
Timestamp: 2025-11-30T18:15:16.883Z
Learning: In the apteryxxyz/next-ws repository, pull requests that bump Next.js version ranges (e.g., updating to 16.0.6) are created by an automated workflow that triggers when new Next.js versions are released. When reviewing such PRs, assume the Next.js version referenced has already been released and is available.
📚 Learning: 2025-08-22T12:23:23.227Z
Learnt from: apteryxxyz
Repo: apteryxxyz/next-ws PR: 113
File: src/server/helpers/module.ts:29-32
Timestamp: 2025-08-22T12:23:23.227Z
Learning: Next.js builds pages to CommonJS format, so using require() to load built pages via nextServer.getPagePath() is appropriate and ESM compatibility concerns don't apply in this context.
Applied to files:
src/server/setup.ts
📚 Learning: 2025-08-22T05:11:55.871Z
Learnt from: apteryxxyz
Repo: apteryxxyz/next-ws PR: 112
File: src/server/route/match.ts:3-9
Timestamp: 2025-08-22T05:11:55.871Z
Learning: In next-ws route matching, the route patterns passed to compileRoutePattern come from Next.js internal appPathRoutes (filesystem-based route definitions), which are developer-controlled. User input (requestPathname) is matched against these compiled regexes, not used to construct them, so ReDoS concerns don't apply to the regex construction itself.
Applied to files:
src/server/setup.ts
🧬 Code graph analysis (1)
src/server/setup.ts (2)
src/server/index.ts (2)
SetupOptions(12-12)Adapter(1-1)src/server/helpers/adapter.ts (1)
Adapter(28-47)
🔇 Additional comments (1)
src/server/setup.ts (1)
16-18: Adapter options wiring looks goodThe
SetupOptionsinterface and optionaloptionsparameter cleanly introduce adapter injection while keeping the public API extendable; the conditional startup logging also reads clearly. No changes needed here.Also applies to: 23-28, 38-44
|
@apteryxxyz finally! |
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.