chore(PLA-2224): re-fetch guides when the existing socket is disconnected and reconnected#869
chore(PLA-2224): re-fetch guides when the existing socket is disconnected and reconnected#869jareddellitt wants to merge 6 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 58d3def The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #869 +/- ##
==========================================
+ Coverage 67.41% 67.49% +0.08%
==========================================
Files 204 204
Lines 8561 8579 +18
Branches 1118 1127 +9
==========================================
+ Hits 5771 5790 +19
+ Misses 2766 2765 -1
Partials 24 24
|
jareddellitt
requested review from
a team,
kylemcd and
thomaswhyyou
and removed request for
a team
| if (this.socketOpenListenerId) { | ||
| this.socket.off([this.socketOpenListenerId]); | ||
| this.socketOpenListenerId = undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we move this to a private method and re-use here and in unsubscribe?
| // If the socket is already connected, there's no initial open event coming, | ||
| // so we don't need to skip anything. If it's not yet connected, skip the | ||
| // first onOpen since that's the initial connection, not a reconnect. | ||
| let isFirstOpen = !this.socket.isConnected(); |
There was a problem hiding this comment.
Given we call this.socket.connect() if this.socket.isConnected() is not true above, I would think this is always true by the time we get here, but maybe this is just for out of caution?
| // so we don't need to skip anything. If it's not yet connected, skip the | ||
| // first onOpen since that's the initial connection, not a reconnect. | ||
| let isFirstOpen = !this.socket.isConnected(); | ||
| this.socketOpenListenerId = this.socket.onOpen(() => { |
There was a problem hiding this comment.
Is it fair to assume this will fire when we deploy switchboard? I guess switchboard-sockets specifically?
If so, I kinda want to avoid triggering ALL connected clients to re-fetch at the same time from a deploy. Curious if you have any thoughts on targeting re-connect events from the tab visibility state change more granularly? Those will happen more organically and spread out across connected clients, which I think is would be the right thing to do. But i'm a bit hesitant to just blanket re-fetch based on every socket onOpen events.
2 participants
Description
This will force re-fetch guides when the socket is reopened after a disconnection. This handles both deployments of the underlying sockets system and page visibility induced disconnections.
Checklist