Core: Ensure telemetry is never triggered on initial load of checklist data#33918
Core: Ensure telemetry is never triggered on initial load of checklist data#33918ghengeveld merged 2 commits intonextfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit 04fd300
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThe PR modifies telemetry handling in the checklist utility by removing the dequal import and adding an early return guard that skips telemetry processing when loading from persistence during the initial load transition, preventing telemetry events from firing in this scenario. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
✨ Finishing Touches
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/utils/checklist.ts (1)
87-90: Consider narrowing the guard to the explicitfalse → truetransition.The current condition
!previousState.loadedskips telemetry for any state change where the previous state hadloaded: false. In this codebase that only ever happens once (the initialsetStatecall setsloaded: true), so the fix is correct today. However, if a future refactor ever drops theloaded: trueassignment, this guard would silently swallow all telemetry without any obvious signal.Tightening the condition to also assert
state.loadedmakes the intent precise:💡 Proposed refinement
- // Skip telemetry when loading from persistence (first transition to loaded: true) - if (!previousState.loaded) { + // Skip telemetry when loading from persistence (first transition to loaded: true) + if (!previousState.loaded && state.loaded) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/checklist.ts` around lines 87 - 90, The guard currently returns whenever previousState.loaded is falsy, which is too broad; change the check in checklist.ts to only skip telemetry on the explicit false→true transition by testing both sides: return early only when previousState.loaded === false && state.loaded === true (so reference the previousState and state.loaded symbols and update that conditional accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/utils/checklist.ts`:
- Around line 87-90: The guard currently returns whenever previousState.loaded
is falsy, which is too broad; change the check in checklist.ts to only skip
telemetry on the explicit false→true transition by testing both sides: return
early only when previousState.loaded === false && state.loaded === true (so
reference the previousState and state.loaded symbols and update that conditional
accordingly).
What I did
Added a check to prevent triggering telemetry on initial load of checklist data from persistence.
Unfortunately, I was not able to reproduce the problem locally. However, the data suggests that telemetry is triggered when loading from persistence, because
controlsandviewportsappear over-reported. These two items have very high completion rates despite normally requiring a manual user action (changing controls or switching viewports). As such, I want to eliminate the possibility by adding this extra guard.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Unfortunately it's unknown how to reproduce the original issue.
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>