Checklist: Fix how state changes are reported and drop some completion restrictions #33217
Conversation
📝 WalkthroughWalkthroughThe changes refactor checklist telemetry tracking to emit two separate events (muted and status) instead of one, update a checklist item's label text, remove ordering dependencies between checklist items, and update the telemetry event type definitions accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
✨ Finishing touches
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (1)
code/core/src/core-server/utils/checklist.ts (1)
102-116: Consider consolidating iterations for efficiency.The telemetry logic is correct, but there are multiple passes over the entries:
- Lines 72-84: persistence
- Lines 89-100: change detection
- Lines 105-110: computing completedItems and skippedItems
These could be consolidated into a single iteration for better performance, though the current code is clearer and the performance impact is likely negligible for typical checklist sizes.
Example consolidated approach
const { projectValues, userValues, mutedItems, statusItems, completedItems, skippedItems } = entries.reduce((acc, [id, { status, mutedAt }]) => { // Persistence if (status === 'done') { acc.projectValues[id] = { status }; } else if (status === 'accepted' || status === 'skipped') { acc.userValues[id] = { status }; } if (mutedAt) { acc.userValues[id] = { ...acc.userValues[id], mutedAt }; } // Change detection const prev = previousState.items[id]; if (mutedAt !== prev?.mutedAt) acc.mutedItems.push(id); if (status !== prev?.status) acc.statusItems.push(id); // Telemetry snapshots if (status === 'done' || status === 'accepted') acc.completedItems.push(id); if (status === 'skipped') acc.skippedItems.push(id); return acc; }, { projectValues: {}, userValues: {}, mutedItems: [], statusItems: [], completedItems: [], skippedItems: [] });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/core-server/utils/checklist.ts(2 hunks)code/core/src/shared/checklist-store/checklistData.tsx(1 hunks)code/core/src/telemetry/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/shared/checklist-store/checklistData.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/telemetry/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/shared/checklist-store/checklistData.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/telemetry/types.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/shared/checklist-store/checklistData.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/telemetry/types.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/shared/checklist-store/checklistData.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/telemetry/types.ts
🧠 Learnings (1)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/shared/checklist-store/checklistData.tsx
🧬 Code graph analysis (1)
code/core/src/core-server/utils/checklist.ts (2)
code/core/src/shared/checklist-store/index.ts (1)
StoreState(12-15)code/core/src/telemetry/index.ts (1)
telemetry(29-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/core/src/shared/checklist-store/checklistData.tsx (1)
198-198: LGTM! Improved label clarity.The updated label "Render your first component" is clearer and more encouraging for new users than "Render a component", emphasizing this as an initial onboarding step.
code/core/src/telemetry/types.ts (1)
36-37: LGTM! Type definitions align with the new telemetry structure.Splitting the single
'onboarding-checklist'event into'onboarding-checklist-muted'and'onboarding-checklist-status'provides better granularity for tracking user interactions with the checklist.code/core/src/core-server/utils/checklist.ts (2)
67-86: LGTM! Persistence logic correctly handles mutedAt tracking.The updated logic properly separates project-local (
done) and user-local (accepted,skipped) persistence while now also trackingmutedAtchanges in user-local storage. The use ofentriesmakes the code clearer.
88-101: LGTM! Change detection logic is correct.The reduce function correctly identifies items with
mutedAtorstatuschanges by comparing withpreviousState. The optional chaining on line 92 safely handles cases where items don't exist in the previous state.
|
View your CI Pipeline Execution ↗ for commit 976e557
☁️ Nx Cloud last updated this comment at |
shilman
left a comment
shilman
left a comment
There was a problem hiding this comment.
Changes look good. Thanks for doing this!
shilman
added
the
patch:yes
Checklist: Fix how state changes are reported and drop some completion restrictions (cherry picked from commit c96f4b1)
github-actions
bot
added
the
patch:done
What I did
renderComponentbefore doing other thingsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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
✏️ Tip: You can customize this high-level summary in your review settings.