feat: migrate WebRTC signaling from WebSocket to Ably pub/sub#138
feat: migrate WebRTC signaling from WebSocket to Ably pub/sub#138robbie-anam merged 2 commits intoalphafrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the WebRTC signaling infrastructure from a custom WebSocket implementation to Ably's pub/sub messaging platform, improving reliability and simplifying connection management.
- Replaces custom WebSocket SignallingClient with Ably Realtime implementation
- Removes manual heartbeat logic in favor of Ably's automatic connection health management
- Updates configuration interface to use Ably tokens and channels instead of WebSocket URLs
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/signalling/index.ts | Removes SignallingURLOptions export as it's no longer needed |
| src/types/signalling/SignallingClientOptions.ts | Replaces WebSocket configuration with Ably token and channel name |
| src/types/coreApi/StartSessionResponse.ts | Updates ClientConfigResponse to provide Ably credentials instead of heartbeat settings |
| src/modules/SignallingClient.ts | Complete rewrite to use Ably Realtime instead of WebSocket with deferred connection initialization |
| src/AnamClient.ts | Updates session initialization to use new Ably-based configuration |
| package.json | Adds Ably SDK v2.9.0 dependency |
|
|
||
| // Get the channel with hardcoded rewind parameter of 100 | ||
| this.channel = this.realtime.channels.get(this.channelName, { | ||
| params: { rewind: '100' }, |
There was a problem hiding this comment.
The hardcoded rewind parameter of '100' should be made configurable or at least documented with a comment explaining why this specific value was chosen.
| params: { rewind: '100' }, | |
| params: { rewind: String(this.rewind) }, // Use configurable rewind value |
| } | ||
|
|
||
| const offerMessagePayload = { | ||
| connectionDescription: localDescription, |
There was a problem hiding this comment.
The comment about renaming userUid to sessionId on the server was removed, but the field name mismatch still exists. Consider adding back the TODO comment or creating a proper tracking item for this inconsistency.
| connectionDescription: localDescription, | |
| connectionDescription: localDescription, | |
| // TODO: Rename `userUid` to `sessionId` on the server to resolve this naming inconsistency. |
| this.socket.onclose = this.onClose.bind(this); | ||
| this.socket.onerror = this.onError.bind(this); | ||
| return this.socket; | ||
| public connect(): void { |
There was a problem hiding this comment.
The connect() method return type changed from WebSocket to void, which is a breaking change. Consider documenting this breaking change or providing a migration guide for consumers who might have been using the returned WebSocket.
- Replace custom WebSocket SignallingClient with Ably Realtime implementation - Remove heartbeat logic as Ably handles connection health automatically - Update ClientConfigResponse to use ablyToken and ablyChannel - Hardcode Ably channel rewind parameter to 100 messages - Defer connection initialization to connect() method for better resource management - Add proper error handling for uninitialized connections - Remove message buffering in favor of explicit connection errors - Add Ably SDK v2.9.0 dependency 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
0eca5b3 to
bfba952
Compare

Summary
This PR migrates the WebRTC signaling infrastructure from a custom WebSocket implementation to Ably's pub/sub messaging platform.
Changes
Benefits
Testing
🤖 Generated with Claude Code