chore(PLA-2218): enable socket disconnection when page visibility switches to hidden#866
Conversation
🦋 Changeset detectedLatest commit: 47eb095 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.
jareddellitt
requested review from
a team,
mattmikolay and
thomaswhyyou
and removed request for
a team
jareddellitt
force-pushed
the
jdellitt/PLA-2218-V2
branch
from
da539f9 to
456d111
Compare
| }, | ||
| }); | ||
|
|
||
| if (options.disconnectOnPageHidden !== false) { |
There was a problem hiding this comment.
I think turning this on by default will have downstream implications for other modules/clients that rely on the socket connection provided by this API client? For example, I know at least the guide client uses the socket connection to listen for real time updates from the guides channel API.
I'm guessing the right thing to do there is to re-fetch guides when the socket re-connects, not sure if we need to necessarily re-subscribe.
Not saying we need to do all of that in this PR, but just wanted to surface this so we can think about how we go about handling downstream implications.
There was a problem hiding this comment.
I assumed (maybe incorrectly) that this would already be handled if a socket were to become disconnected (like a deploy). Looks like regardless of this change we'd want to add some sort of recovery logic in case of a disconnection?
There was a problem hiding this comment.
if a socket were to become disconnected (like a deploy)
In the case of a deploy that leads to a socket disconnection, can I double check my understanding that the api client automatically tries and reconnects with the new deploy? i.e. if we roll switchboard-sockets, then all the clients with a socket connection ends up disconnecting for a bit (~ minutes) until it reconnects? If that is right, then I don't think it's super critical to do anything more at least with the guide client.
The scenario that i'm thinking about is, where a user has a tab open but in "hidden" state for an extend period of time (~ days) and comes back to it, then we probably want to let the guide client re-fetch to "reset".
So to answer your q: I haven't thought too hard about handling socket disconnection scenarios up until now, assuming the main scenario is a new deploy in which case it auto reconnects. But turning the option on by default to auto disconnect on page visibility does make me think we need to think about handling socket connection scenarios more. Again, don't think that is a blocker to ship. cc: @cjbell
| "@knocklabs/client": patch | ||
| --- | ||
|
|
||
| Disconnects socket after an initial delay when page visibility is hidden |
There was a problem hiding this comment.
We may want to consider mentioning in this changeset that the auto_manage_socket_connection and auto_manage_socket_connection_delay options have been removed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
==========================================
+ Coverage 67.30% 67.41% +0.10%
==========================================
Files 203 204 +1
Lines 8553 8561 +8
Branches 1112 1118 +6
==========================================
+ Hits 5757 5771 +14
+ Misses 2772 2766 -6
Partials 24 24
|
3 participants
Description
Centralizes page visibility handling to a
PageVisibilityclass and adds it directly to theApiClient. Previously this was handled directly by feeds (and only for feeds), and was disabled by default. This enables it by default, manages for all socket usages, and sets a 30s timeout which gives a little bit of flexibility for users switching back and forth between tabs.Checklist