chromaui / addon-visual-tests

Visual Tests addon for Storybook
MIT License
28 stars 1 forks source link

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

Closed ghengeveld closed 1 month ago

ghengeveld commented 1 month ago

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
:sparkles: Test out this PR locally via: ```bash 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 ```
JReinhold commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@JReinhold I've updated the PR: