-
Notifications
You must be signed in to change notification settings - Fork 1
feat: ably connections #140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the existing WebSocket-based signalling layer with Ably Realtime, updates related types and client initialization, and deprecates the brainType field in favor of llmId.
- Refactor: SignallingClient now uses Ably Realtime channels (
ablyToken,channelName) instead of raw WebSockets. - Types: Updated
SignallingClientOptions, API response types, and client configuration inAnamClientto use Ably parameters; deprecatedbrainTypeinPersonaConfig. - Docs & tooling: Added deprecation warnings in
CoreApiRestClient, adjusted metrics tagging to include boolean, updatedREADME.md, and added theablydependency.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/signalling/index.ts | Simplified exports to only include new signalling options. |
| src/types/signalling/SignallingClientOptions.ts | Switched options interface to require ablyToken and channelName. |
| src/types/coreApi/StartSessionResponse.ts | Replaced heartbeat config with ablyToken and ablyChannel. |
| src/types/PersonaConfig.ts | Deprecated brainType and introduced optional llmId. |
| src/modules/SignallingClient.ts | Refactored signalling client to use Ably Realtime API. |
| src/modules/CoreApiRestClient.ts | Added deprecation warnings for brainType usage. |
| src/lib/constants.ts | Added supportsPubSubSignalling flag to client metadata. |
| src/lib/ClientMetrics.ts | Extended metric tag type to include boolean values. |
| src/AnamClient.ts | Updated initialization to pass Ably parameters from API response. |
| package.json | Added ably as a new dependency. |
| README.md | Documented brainType deprecation and sample updated for llmId. |
Comments suppressed due to low confidence (2)
src/types/coreApi/StartSessionResponse.ts:11
- [nitpick] The API response uses
ablyChannelwhile the client option is namedchannelName. Align naming across API and client to reduce confusion—e.g., renameablyChanneltochannelNamein the response or vice versa.
ablyChannel: string;
src/modules/SignallingClient.ts:1
- The new Ably-based signalling logic is not covered by tests. Consider adding unit tests for connection lifecycle (
connect,onConnected,onDisconnected), message publishing, and error handling to ensure reliability.
import * as Ably from 'ably';
| // Check if brainType is being used and log deprecation warning | ||
| if (personaConfig && 'brainType' in personaConfig) { | ||
| console.warn( | ||
| 'Warning: brainType is deprecated and will be removed in a future version. Please use llmId instead.', | ||
| ); | ||
| } |
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Duplicate deprecation warning for brainType appears in two separate methods. Consider extracting this check and warning into a shared helper or logging it only once to reduce code duplication.
| // Check if brainType is being used and log deprecation warning | |
| if (personaConfig && 'brainType' in personaConfig) { | |
| console.warn( | |
| 'Warning: brainType is deprecated and will be removed in a future version. Please use llmId instead.', | |
| ); | |
| } | |
| // Log deprecation warning for brainType if applicable | |
| this.logBrainTypeDeprecationWarning(personaConfig); |
| if (!this.channel) { | ||
| throw new Error( | ||
| 'SignallingClient - sendSignalMessage: Cannot send message, not connected. Call connect() first.', | ||
| ); | ||
| } | ||
|
|
||
| if (!this.isConnected) { | ||
| throw new Error( | ||
| 'SignallingClient - sendSignalMessage: Cannot send message, connection not established yet.', | ||
| ); | ||
| } | ||
|
|
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Currently sendSignalMessage throws if the client isn't connected. If dropped messages are not acceptable, consider buffering messages until the connection is established or clearly document that callers must wait for onConnected.
| if (!this.channel) { | |
| throw new Error( | |
| 'SignallingClient - sendSignalMessage: Cannot send message, not connected. Call connect() first.', | |
| ); | |
| } | |
| if (!this.isConnected) { | |
| throw new Error( | |
| 'SignallingClient - sendSignalMessage: Cannot send message, connection not established yet.', | |
| ); | |
| } | |
| if (!this.channel || !this.isConnected) { | |
| console.warn( | |
| 'SignallingClient - sendSignalMessage: Not connected. Message will be buffered.', | |
| ); | |
| this.messageQueue.push(message); | |
| return; | |
| } |
|
|
||
| private async onClose() { | ||
| this.wsConnectionAttempts += 1; | ||
| private onDisconnected() { |
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onDisconnected handler updates internal state but does not emit any event. Consider emitting a public or internal event to notify consumers about the disconnection for better observability.
No description provided.