Telemetry: Fix delayed init events#34670
Conversation
JReinhold
added
bug
ci: do not merge
telemetry
ci:normal
and removed
agent-scan:human
ci: do not merge
labels
There was a problem hiding this comment.
Pull request overview
This PR addresses a regression in create-storybook init telemetry where events were being queued and only flushed at the end of the init flow (meaning mid-flow cancellation could result in no events being sent). It resolves telemetry state earlier so init-time events are emitted when they occur.
Changes:
- Import and invoke
setTelemetryEnabled()at the start of thewithTelemetry('init', ...)run callback. - Ensure init telemetry is not delayed until the end of the init orchestration flow.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughTelemetry initialization is centralized and resolved earlier: CLI opt-in is applied immediately, otherwise main config is loaded (default .storybook) to determine telemetry, with a configurable fallback. Several callers no longer toggle telemetry directly; tests updated to cover resolution ordering and failures. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Init caller
participant WithTelemetry as withTelemetry
participant Config as loadMainConfig
participant Telemetry as setTelemetryEnabled / telemetry
participant Runner as doInitiate / run
rect rgba(0,128,0,0.5)
CLI->>WithTelemetry: invoke withTelemetry(options)
end
opt Telemetry not resolved
WithTelemetry->>WithTelemetry: resolveTelemetryState(options)
alt CLI provides disableTelemetry
WithTelemetry->>Telemetry: setTelemetryEnabled(!options.disableTelemetry)
else CLI no explicit value
WithTelemetry->>Config: loadMainConfig({ configDir: '.storybook' })
alt config loads
Config-->>WithTelemetry: mainConfig
WithTelemetry->>Telemetry: setTelemetryEnabled(!mainConfig.core?.disableTelemetry)
else config fails
WithTelemetry->>Telemetry: setTelemetryEnabled(fallbackTelemetryState)
end
end
end
WithTelemetry->>Runner: proceed to doInitiate / run()
Runner->>Telemetry: emit events (e.g., "init", "build", "version-update")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/lib/create-storybook/src/initiate.ts`:
- Line 210: The call setTelemetryEnabled(!options.disableTelemetry) is
prematurely forcing telemetry state before withTelemetry/doInitiate can resolve
config-based settings; remove this call (or change it to consult
tryResolveTelemetryStateFromConfig) so telemetry state is resolved inside
withTelemetry/doInitiate instead—locate and remove the setTelemetryEnabled
invocation in initiate.ts (the code around doInitiate, withTelemetry,
tryResolveTelemetryStateFromConfig and options.disableTelemetry) so config
(core.disableTelemetry) can correctly override CLI defaults.
- Around line 209-210: The call to the async function
setTelemetryEnabled(!options.disableTelemetry) is a floating Promise and must
handle rejections; keep it non-blocking but attach a .catch to swallow/log any
error (e.g., setTelemetryEnabled(!options.disableTelemetry).catch(err => { /*
log or warn */ })), ensuring you use the existing logger or console to surface
the error, and leave it before the init flow so it remains non-blocking; update
the code around the setTelemetryEnabled call to include this .catch handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb7c597f-0408-4fa7-baaf-90a72efa8950
📒 Files selected for processing (1)
code/lib/create-storybook/src/initiate.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/lib/create-storybook/src/initiate.ts (1)
211-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not override telemetry state after
withTelemetryhas already resolved it.On Line 212, this second
setTelemetryEnabledcall can re-enable telemetry afterwithTelemetryhas resolved a config-based opt-out (fromcore.disableTelemetry). It effectively bypasses the resolution done beforerun().Proposed fix
-import { telemetry, setTelemetryEnabled } from 'storybook/internal/telemetry'; +import { telemetry } from 'storybook/internal/telemetry'; @@ - // we need to explicitly set this before init to not delay the events until the end of the flow - await setTelemetryEnabled(!options.disableTelemetry); - const result = await doInitiate(options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/create-storybook/src/initiate.ts` around lines 211 - 213, The extra call to setTelemetryEnabled(!options.disableTelemetry) can override the telemetry decision already made by withTelemetry/core.disableTelemetry; remove or guard this call so it doesn't re-enable telemetry after withTelemetry has resolved. Specifically, in initiate.ts remove the unconditional await setTelemetryEnabled(!options.disableTelemetry) (or change it to only call setTelemetryEnabled with the resolved value from withTelemetry/run) and ensure the code uses the telemetry state returned by withTelemetry/run instead of options.disableTelemetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/lib/create-storybook/src/initiate.ts`:
- Around line 211-213: The extra call to
setTelemetryEnabled(!options.disableTelemetry) can override the telemetry
decision already made by withTelemetry/core.disableTelemetry; remove or guard
this call so it doesn't re-enable telemetry after withTelemetry has resolved.
Specifically, in initiate.ts remove the unconditional await
setTelemetryEnabled(!options.disableTelemetry) (or change it to only call
setTelemetryEnabled with the resolved value from withTelemetry/run) and ensure
the code uses the telemetry state returned by withTelemetry/run instead of
options.disableTelemetry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6e56f25-55cd-453b-a82b-a0203e8d4308
📒 Files selected for processing (6)
code/core/src/core-server/build-dev.tscode/core/src/core-server/build-static.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/withTelemetry.test.tscode/core/src/core-server/withTelemetry.tscode/lib/create-storybook/src/initiate.ts
| const configDir = options.cliOptions.configDir || options.presetOptions?.configDir; | ||
| async function resolveTelemetryState(options: TelemetryOptions) { | ||
| // 1. If telemetry is explicitly set via CLI options or env var, set and skip loading main config | ||
| if (options.cliOptions.disableTelemetry !== undefined) { |
There was a problem hiding this comment.
does this account for the environment variable as well? should it? (in case resolving main.js fails in the next step)
There was a problem hiding this comment.
Yes it does (the comment even say that 😆)
"CLI options" immediately fallback to env vars too, when they are collected at the very beginning
Closes #
What I did
We found that during init, telemetry events would not be sent until the very end of the flow, where everything would be sent at once. If the flow was canclled mid-way, no events would be sent.
This change ensures that events are sent when they happen.
This was originally a regression from #34485 which was released in 10.4.0-alpha.10, so it's only a small window of bad telemetry.
Now, we immediately resolve telemetry state, instead of relying on the different commands do to it eventually from their loaded presets.
There's a small change here, where previously we would use preset loading mechanism to look up
core.disableTelemetrybut now we just load the main config, because it's faster. So theoretically someone could have an external preset where they set telemetry setting and that wouldn't be supported anymore, now we only support it when it's directly set in main.ts (although it can be imported from elsewhere, etc.)Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit