Add WebSocketClient package and refactor PriceObserverService #1468
Conversation
Summary of ChangesHello @DRadmir, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, reusable Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring effort that extracts the WebSocket connection logic into a new, reusable WebSocketClient package. This significantly simplifies PriceObserverService and improves the overall architecture by promoting modularity. The new WebSocketClient makes good use of modern Swift concurrency features like actors and AsyncStream.
My review includes one high-severity comment regarding a potential race condition in the new WebSocketConnection implementation. Addressing this will make the new component more robust. Overall, this is a high-quality contribution.
| guard let task, state == .connected else { | ||
| throw WebSocketError.notConnected | ||
| } | ||
| try await task.send(.data(data)) |
There was a problem hiding this comment.
I think you should be able to push data in any state, it should just until connected?
There was a problem hiding this comment.
To support sending data id any state, we'd need to add a pending messages buffer and flush it on connect.
This adds complexity: handling send errors for buffered events, managing buffer grows, and what todo if connections fails? Needs add ping, and after ping decide what to do.
| private func handleStreamTermination() { | ||
| guard state != .disconnected else { return } | ||
|
|
||
| task?.cancel(with: .goingAway, reason: nil) |
There was a problem hiding this comment.
task?.cancel(with: .goingAway, reason: nil) is duplication
gemcoder21
left a comment
gemcoder21
left a comment
There was a problem hiding this comment.
task?.cancel(with: .goingAway, reason: nil) is used in 3 places, you should simplify and consolidate many methods
|
|
||
| // MARK: - URLSessionWebSocketTask.Message | ||
|
|
||
| extension URLSessionWebSocketTask.Message { |
DRadmir
force-pushed
the
websocket-engine
branch
from
861c93a to
bc970be
Compare
Refactor PriceObserverService to extract WebSocket connection logic into a reusable WebSocketClient package. This
enables adding new WebSocket connections in the future (e.g., perpetuals, real-time order updates).
Changes