Skip to content

Conversation

@Synar
Copy link
Contributor

@Synar Synar commented Jun 15, 2025

What I did

Most modules check both process.env.CI as well as process.env.stdout.isTTY before prompting the user. Telemetry was only checking process.env.CI.

This caused builds in non interactive environments to fail with a success code.

This commit aligns the check with the rest of storybook.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  • Put yourself in a configuration that will make building storybook fail. In our case, this was due to a storybook sub package version not being aligned with the main storybook version in our package.json
  • Build storybook using npm in a docker container without the option CI=true
  • Without this PR, the build should succeed, despite the build actually failing
  • With this PR, the build will properly fail

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

Fixes telemetry prompting behavior by adding a process.stdout.isTTY check, preventing silent build failures in non-interactive environments like Docker containers without CI=true set.

  • Added TTY check in code/core/src/core-server/withTelemetry.ts to align with other Storybook modules
  • Fixed typo in SIGINT event listener removal (currently 'SIGINIT')
  • Important for builds where package versions are misaligned, as it properly surfaces build failures
  • Critical fix for Docker and other non-interactive environments where builds could silently fail
  • Needs additional telemetry-specific unit tests to verify the new behavior

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

1 file reviewed, no comments
Edit PR Review Bot Settings | Greptile

Most modules check both process.env.CI as well as process.env.stdout.isTTY
before prompting the user. Telemetry was only checking process.env.CI.

This caused builds in non interactive environments to fail with a
success code.

This commit aligns the check with the rest of storybook.

Also fix a typo from SIGINIT to SIGINT

Signed-off-by: Alice Khoudli <[email protected]>
@Synar Synar force-pushed the ali/fix-telemetry-not-checking-is-tty branch from dc7559a to 9e15711 Compare June 15, 2025 18:17
@Synar
Copy link
Contributor Author

Synar commented Jun 15, 2025

Fixed typo in SIGINT event listener removal (currently 'SIGINIT')

This comment made it sound like it was already done, but it was not. I've now added the typo fix as requested though.

@emersion
Copy link

Thanks for fixing this! LGTM.

@valentinpalkovic valentinpalkovic changed the title ci: fix telemetry prompting without checking isTTY Telemetry: Fix prompting without checking isTTY Jun 16, 2025
@valentinpalkovic valentinpalkovic self-assigned this Jun 16, 2025
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks so much for the fix. Much appreciated! 🙏

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 17, 2025
@storybook-app-bot
Copy link

Package Benchmarks

Commit: 9e15711, ran on 17 June 2025 at 18:37:11 UTC

The following packages have significant changes to their size or dependencies:

@storybook/nextjs

Before After Difference
Dependency count 531 531 0
Self size 902 KB 217 KB 🎉 -686 KB 🎉
Dependency size 58.92 MB 58.92 MB 🎉 -172 B 🎉
Bundle Size Analyzer Link Link

@storybook/nextjs-vite

Before After Difference
Dependency count 131 131 0
Self size 3.07 MB 2.39 MB 🎉 -685 KB 🎉
Dependency size 22.30 MB 22.30 MB 🎉 -500 B 🎉
Bundle Size Analyzer Link Link

@valentinpalkovic valentinpalkovic merged commit 588b3b1 into storybookjs:next Jun 23, 2025
56 of 60 checks passed
ghengeveld pushed a commit that referenced this pull request Jun 24, 2025
…s-tty

Telemetry: Fix prompting without checking isTTY
(cherry picked from commit 588b3b1)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 24, 2025
@ndelangen ndelangen removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal patch:done Patch/release PRs already cherry-picked to main/release branch telemetry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants