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

Add a UI state for an error on the first build #95

Merged
merged 10 commits into from
Oct 4, 2023

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Sep 12, 2023

@MichaelArestad I wasn't sure if there is a component I could use here.

Also I am not sure exactly where this should link to?

📦 Published PR as canary version: 0.0.99--canary.95.19f15a6.0

✨ Test out this PR locally via:

npm install @chromaui/[email protected]
# or 
yarn add @chromaui/[email protected]

@linear
Copy link

linear bot commented Sep 12, 2023

AP-3621 Improve first build error handling

I hit a number of issues when creating my first build for test baselines:

  1. Since my test repo only has one commit, the build appears to have failed when looking at the CLI output (see screenshot). I should be able to run the addon regardless of how many commits I have.
  2. In the UI, the build thinks it's still running. I.e the above error wasn't handled.
  3. The UI is stuck waiting for the build to finish with no way to cancel/retry the build, presumably for ever.

Some of these issues may exist not in the onboarding build creation scenario as well.

image.png

@tmeasday tmeasday force-pushed the tom/ap-3645-dont-switch-to-first-build-until-it-has-completed branch from 9233087 to be34a5c Compare September 14, 2023 11:30
@tmeasday tmeasday force-pushed the tom/ap-3621-improve-first-build-error-handling branch from 75fcc38 to 03e43fe Compare September 14, 2023 11:54
Base automatically changed from tom/ap-3645-dont-switch-to-first-build-until-it-has-completed to main September 15, 2023 02:07
@tmeasday
Copy link
Member Author

@jonniebigodes any suggestion on the best place the link should go? This is when the Chromatic build errors somewhere, probably most likely the SB build itself. @MichaelArestad suggested https://www.chromatic.com/docs/setup/#troubleshooting

@jonniebigodes
Copy link
Contributor

@tmeasday I'm ok with it for now, as the errors are similar for each case. However, as feedback continues to roll in, I'm more than ok with updating the documentation and setting up a section, possibly named "Debugging Tests", with some additional information and instructions on debugging it. I'm more than glad to update the addon to reflect that change. Sounds good?

contents = (
<>
<ErrorContainer>
<b>Build failed:</b> <code>{firstError?.message || "Unknown Error"}</code>{" "}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure, we want the originalError instead of the formattedError so that users see more information?

Copy link
Contributor

@weeksling weeksling left a comment

Choose a reason for hiding this comment

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

This looks good to me. @tmeasday I had trouble reproducing this. For the case in the ticket (running a build in a repo with only one commit) now creates a new build with auto-accepted stories. Is there another cli build error you've used to test it?

@tmeasday
Copy link
Member Author

tmeasday commented Oct 4, 2023

@weeksling hmm, yes we tweaked the CLI to allow single commit repos from the addon. You could try a repo with no commits at all (not no git setup at all, that will trigger a different issue 😉 ).

It's a while ago now, I think I figured out some main.js that would error for sb build but not for sb dev. Another idea that occurs to me is to just put some junk in your build-storybook script in package.json.

@weeksling
Copy link
Contributor

Got it, thanks! @tmeasday this formatting looks a bit off, but it looks like both the originalError and the formattedError contain the same content. I think we should \ remove the unneeded characters in the error message for the experimental_onTaskError formatter.
Screen Shot 2023-10-04 at 2 53 16 PM

@MichaelArestad MichaelArestad merged commit 38c4a67 into main Oct 4, 2023
@MichaelArestad MichaelArestad deleted the tom/ap-3621-improve-first-build-error-handling branch October 4, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants