Skip to content
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

Poll for API connection and show notification when it fails #303

Merged
merged 7 commits into from May 15, 2024

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented May 10, 2024

Runs a (server-side) GraphQL query every 5 seconds to check whether it's connected. If not, it prevents starting a new build:

Screenshot 2024-05-10 at 16 37 15

And shows a warning screen in the addon panel:

Screenshot 2024-05-13 at 14 16 01

These automatically go away when the connection is restored (delayed up to 5 seconds as it polls).

When the automatic retry is aborted after 10 attempts (~50 seconds), the user is prompted to retry manually:

Screenshot 2024-05-13 at 14 16 33

This way we don't just keep trying indefinitely when there is no internet connection.

📦 Published PR as canary version: 1.4.0--canary.303.3053a3b.0

✨ Test out this PR locally via:

npm install @chromatic-com/storybook@1.4.0--canary.303.3053a3b.0
# or 
yarn add @chromatic-com/storybook@1.4.0--canary.303.3053a3b.0

@ghengeveld ghengeveld requested a review from JReinhold May 10, 2024 12:29
@ghengeveld ghengeveld added release Auto: Create a `latest` release when merged minor Auto: Increment the minor version when merged labels May 10, 2024
@ghengeveld ghengeveld linked an issue May 10, 2024 that may be closed by this pull request
@JReinhold
Copy link
Contributor

Does this mean that SB will poll Chromatic every fifth second just by being open, regardless of if the addon is in active use or not?
And by active use I mean the addon's panel is in focus, or the user is waiting for VRT to finish, or similar.

I don't think that's the right trade-off:

  1. it's going to look super annoying in the DevTools Network panel
  2. A notification will open whenever I loose connection to Chromatic, even if I'm not remotely working on that part of my Storybook. As a user I'd find that pretty annoying.
  3. Paranoid users are going to hate this and call this idle tracking

Can't we just poll for network connection when the addon is actually in use? Like when the user is downloading diffs or waiting for results to finish?

@ghengeveld
Copy link
Member Author

ghengeveld commented May 11, 2024

@JReinhold I understand your concern and I think it makes sense to disable it when the addon hasn’t been interacted with for a while. But I’ll also address your specific concerns:

  1. This request is server-side, so it won’t pollute the browser network logs.
  2. It’s pretty unlikely to not be able to connect to the API. It should probably only warn after 2 failed attempts though.
  3. Tracking seems a bit overblown since there’s not really any data being sent. Nevertheless I could be swayed to use a different/substitute internet connection check such as is-online. I do have plans to poll for some actual useful data (project config) as well though.

Do you think we should omit the notification? That wasn’t part of the original design, and isn’t really necessary.

@JReinhold
Copy link
Contributor

Ah okay, sorry for my ignorance, that makes more sense when it's server side.

Then I'm fine with most of this actually, except I agree that the notification should be removed.

I understand that "polling when addon is in use" is then more complex, given you'd need to send "is panel open" data over the channel which might not be worth it.

@ghengeveld
Copy link
Member Author

ghengeveld commented May 13, 2024

@JReinhold I've updated the PR:

  • Removed the notification
  • Stop retrying after 10 failed attempts (~50 seconds)
  • Added a retry button to the UI (which starts another 10 attempts)
  • Require 2 failed attempts before warning about the disconnect
  • Every retry takes an additional second (starting at 1 second)

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✨

@ghengeveld ghengeveld added skip-release Auto: Preserve the current version when merged and removed release Auto: Create a `latest` release when merged labels May 15, 2024
@ghengeveld ghengeveld merged commit 714d2f7 into main May 15, 2024
10 checks passed
@ghengeveld ghengeveld deleted the ghengeveld/251-ui-no-network-issue branch May 15, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Auto: Increment the minor version when merged skip-release Auto: Preserve the current version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI - No network issue
2 participants