feat: ConnectionStarted/ConnectionReady events#241
Conversation
There was a problem hiding this comment.
Pull request overview
Adds higher-level connection lifecycle events so consumers can react when a Lavalink WebSocket connection is established (“started”) and when the Lavalink node is fully usable after the Ready payload (“ready”).
Changes:
- Introduces
ConnectionStarted/ConnectionReadyevents acrossILavalinkSocket,LavalinkNode, andIAudioService. - Adds new event args types:
ConnectionStartedEventArgsandConnectionReadyEventArgs. - Updates CI build version property to
4.2.0.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Lavalink4NET/Socket/LavalinkSocket.cs | Raises ConnectionStarted after a successful WebSocket connect; adds logging + dispatch helper. |
| src/Lavalink4NET/Socket/ILavalinkSocket.cs | Extends socket interface with ConnectionStarted event. |
| src/Lavalink4NET/LavalinkNode.cs | Propagates socket connection started + ready events through node listener and internal events; includes socket in ready args. |
| src/Lavalink4NET/ILavalinkNodeListener.cs | Adds listener callbacks for connection started/ready. |
| src/Lavalink4NET/IAudioService.cs | Exposes new connection started/ready events at the audio-service API level. |
| src/Lavalink4NET/Events/ConnectionStartedEventArgs.cs | New event args for connection started (socket + endpoint). |
| src/Lavalink4NET/Events/ConnectionReadyEventArgs.cs | New event args for connection ready (socket + session info). |
| src/Lavalink4NET/AudioServiceBase.cs | Implements new listener callbacks and forwards them to public events. |
| .github/workflows/build.yml | Bumps build /property:Version from 4.1.1 to 4.2.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| event AsyncEventHandler<ConnectionClosedEventArgs>? ConnectionClosed; | ||
|
|
||
| event AsyncEventHandler<ConnectionStartedEventArgs>? ConnectionStarted; | ||
|
|
There was a problem hiding this comment.
Adding ConnectionStarted to ILavalinkSocket requires updating all ILavalinkSocket implementations. The test socket in tests/Lavalink4NET.Tests/LavalinkNodeTests.cs (file-local class LavalinkSocket) currently only implements ConnectionClosed and will fail to compile until it also declares ConnectionStarted (and raises it if the tests rely on it).
| attempt = 0; | ||
|
|
||
| _logger.ConnectionEstablished(Label); | ||
| await InvokeConnectionStartedAsync(new ConnectionStartedEventArgs(this, _options.Uri)); |
There was a problem hiding this comment.
This new await is missing ConfigureAwait(false), while other awaits in ConnectWithRetryAsync use it. In a library context this can inadvertently capture a SynchronizationContext and cause deadlocks; consider adding ConfigureAwait(false) here for consistency and safety.
| await InvokeConnectionStartedAsync(new ConnectionStartedEventArgs(this, _options.Uri)); | |
| await InvokeConnectionStartedAsync(new ConnectionStartedEventArgs(this, _options.Uri)).ConfigureAwait(false); |
| _logger.Ready(Label, SessionId); | ||
| await SocketConnectionReady.InvokeAsync(this, EventArgs.Empty); | ||
| await InvokeConnectionReadyAsync(this, new ConnectionReadyEventArgs(socket, readyPayload.SessionId, readyPayload.SessionResumed)); | ||
| } |
There was a problem hiding this comment.
New behavior (ConnectionStarted/ConnectionReady propagation via ILavalinkNodeListener and the internal SocketConnection* events) doesn’t appear to be covered by tests. Consider updating/adding LavalinkNodeTests to assert that OnConnectionStartedAsync is called after socket connect and OnConnectionReadyAsync is called on ReadyPayload with the expected SessionId/WasResumed values.
No description provided.