-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat: refactor WebSocket #208
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
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 refactors the WebSocket handling from using the react-use-websocket
library to a custom plain/optimized WebSocketManager class. The refactor improves performance through better batching, removes React context dependencies, and adds WebSocket health metrics to the Settings > Health page.
Key Changes
- Complete replacement of React context-based WebSocket implementation with a singleton WebSocketManager
- Removal of
react-use-websocket
dependency and related hooks - Addition of WebSocket health monitoring and metrics display in Settings > Health
Reviewed Changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/websocket/WebSocketManager.ts |
New singleton WebSocket manager with optimized batching and health metrics |
src/store.ts |
Added WebSocket metrics interface and store actions for connection state tracking |
Multiple component files | Removed React context usage and replaced with direct sendMessage imports |
src/components/settings-page/tabs/Health.tsx |
Added WebSocket health metrics display section |
src/Main.tsx |
Simplified initialization to use singleton manager instead of context provider |
package.json |
Removed react-use-websocket dependency |
8576507
to
477ef1e
Compare
@Nerivec happy to test this whenever it's ready. Just let me know. |
f8e91d2
to
cac9734
Compare
Did a pull and can't see any obvious issues. |
cac9734
to
c0e04e9
Compare
c0e04e9
to
bda1ed2
Compare
Does the new webproxy code require both standalone windfront In my setup, the z2m is behind an nginx instance configured to proxy the UI and API. I have created an additional HTTP server, without SSL, for the proxy. I have verified with This configuration works fine without |
WindFront 2.1.0 or current edge should work fine with either Z2M 2.6.1 or 2.6.1-dev. I'm not sure I have a good understanding of your setup. I'm also not sure of the proxying you're describing with built-in and standalone. |
Sorry for the delay -- I've been away and just got back yesterday. Initial testing looks good, I'm not observing any disconnects/reconnects. I'll leave developer tools open for a few hours to be sure. |
After some delay, I did observe disconnects and reconnects. Is there any data I can give you? |
What kind of "delay" are we talking about? Does it keep working if you keep using the UI for that same amount of time (no out of focus, no tab switching, no minimizing, no stillness, etc.)? Is it the same delay if you try to reproduce? Is there a "reason" in the dev tools console logs for the close/error? |
Settings>Health
@Koenkk I won't merge this until after Sep release since it needs a decent amount of testing, but when you have some time, if you can review the new WebSocketManager class 👍
CC: @ams2990 @thargy