Core: Avoid late layout shift and improve ChecklistWidget perceived performance #33184
Conversation
… animations until settled down and only delay transition for outgoing items
📝 WalkthroughWalkthroughAdds a staged two‑phase render and animation gating to the checklist widget, readiness/init gating in the checklist hook, and persistent collapsible state support; animations are deferred until a debounced ready signal and store READY status, enabling exit transitions before new items appear. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useChecklist
participant Store as UniversalStore
participant Widget as ChecklistWidget
rect `#EBF3FF`
note over Hook,Store: Initialization & readiness gating
Hook->>Store: observe status
Store-->>Hook: status != READY
Hook->>Hook: ready = false
end
rect `#EDFFED`
note over Store,Hook: Store becomes READY + PREVIEW_INITIALIZED
Store->>Hook: PREVIEW_INITIALIZED
Hook->>Hook: initialized = true
Hook->>Hook: debounceReady() (500ms)
Hook-->>Widget: ready = true
end
rect `#FFF5EB`
note over Widget,Hook: Checklist two‑phase animation flow
Widget->>Widget: delay 1s -> animated = true
alt not animated
Widget->>Widget: render nextItems as gate (no transitions, timeout=0ms)
else animated
Widget->>Widget: Phase A: keep current items, set per-item animateOut -> allow exit (2s)
Widget->>Widget: Phase B (after exit): set item flags (isCompleted/isAccepted/isDone/isSkipped), render nextItems (transition timeout 300ms)
Widget->>Widget: show particle icon only if isCompleted && animated
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 1
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
116-121: Good addition of readiness gating with debounce.The state initialization and debounce setup look correct. However, the debounce function created via
useMemois not cancelled on component unmount, which could cause a state update on an unmounted component.Add cleanup for the debounce function:
+ useEffect(() => { + return () => debounceReady.cancel(); + }, [debounceReady]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/sidebar/ChecklistWidget.tsx(2 hunks)code/core/src/manager/components/sidebar/useChecklist.ts(4 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/manager/components/sidebar/ChecklistWidget.tsxcode/core/src/manager/components/sidebar/useChecklist.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/manager/components/sidebar/ChecklistWidget.tsxcode/core/src/manager/components/sidebar/useChecklist.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/manager/components/sidebar/ChecklistWidget.tsxcode/core/src/manager/components/sidebar/useChecklist.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/manager/components/sidebar/ChecklistWidget.tsxcode/core/src/manager/components/sidebar/useChecklist.ts
🧠 Learnings (6)
📚 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/manager/components/sidebar/useChecklist.ts
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/components/sidebar/useChecklist.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/sidebar/useChecklist.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/sidebar/useChecklist.ts
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/sidebar/useChecklist.ts
📚 Learning: 2025-11-24T17:49:47.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.659Z
Learning: The Storybook repository is a TypeScript/React monorepo with main codebase in 'code/' directory and tooling in 'scripts/' directory
Applied to files:
code/core/src/manager/components/sidebar/useChecklist.ts
🧬 Code graph analysis (2)
code/core/src/manager/components/sidebar/ChecklistWidget.tsx (1)
code/core/src/manager/components/sidebar/useChecklist.ts (2)
useChecklist(111-246)ChecklistItem(30-41)
code/core/src/manager/components/sidebar/useChecklist.ts (2)
code/core/src/manager-api/stores/checklist.ts (1)
universalChecklistStore(9-12)code/core/src/manager-api/index.ts (1)
universalChecklistStore(23-23)
⏰ 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 (8)
code/core/src/manager/components/sidebar/useChecklist.ts (3)
195-225: LGTM for subscription gating logic.The addition of
status !== experimental_UniversalStore.Status.READYcheck correctly prevents subscriptions from being set up before the universal store is ready, which aligns with the PR objective of preventing premature state updates.
239-245: LGTM for the extended return value.The
readyboolean is correctly added to the return object, enabling downstream components likeChecklistWidgetto gate their animations.
227-231: I'll help you verify this review comment about the event listener cleanup. Let me investigate how theapi.onceandapi.offmethods are implemented.
<function_calls>
#!/bin/bashFirst, let's locate the manager-api files to understand the api implementation
find . -type f -name "managerapi*" | grep -E ".(ts|tsx|js)$" | head -20
</function_calls>Let me search for the API implementation more broadly:
<function_calls>
#!/bin/bashSearch for the once and off method implementations
rg -n ".once\s*(" --type ts -B 2 -A 5 | head -50
</function_calls>Let me look at the actual file and understand what api object is being used:
<function_calls>
#!/bin/bashRead the useChecklist.ts file to understand the context
cat -n code/core/src/manager/components/sidebar/useChecklist.ts | head -50
</function_calls>Let me get more of the file to see the imports:
<function_calls>
#!/bin/bashGet line count and read the beginning of the file
wc -l code/core/src/manager/components/sidebar/useChecklist.ts
head -40 code/core/src/manager/components/sidebar/useChecklist.ts
</function_calls>Now let me search for the manager-api implementation:
<function_calls>
#!/bin/bashFind manager-api related files
find . -type d -name "manager-api" 2>/dev/null
find . -path "managerapi*" -name ".ts" -o -name ".tsx" 2>/dev/null | grep -v node_modules | head -20
</function_calls>Let me search for the StoryBookAPI or similar class that would have the once and off methods:
<function_calls>
#!/bin/bashSearch for class or interface definitions containing 'once' method
rg -n "^\s*(class|interface|type|export).*API" --type ts code/ | grep -i manager | head -20
</function_calls>Let me look at the specific lines in question first:
<function_calls>
#!/bin/bashGet lines 220-235 from the file
sed -n '220,235p' code/core/src/manager/components/sidebar/useChecklist.ts
</function_calls>Let me also get the imports section:
<function_calls>
#!/bin/bashGet the imports and declarations at the top
head -100 code/core/src/manager/components/sidebar/useChecklist.ts | tail -60
</function_calls>Let me search for where the api object comes from in this file:
<function_calls>
#!/bin/bashSearch for api variable usage in the file
rg -n "api\s*(=|:)" code/core/src/manager/components/sidebar/useChecklist.ts -B 2 -A 2
</function_calls><function_calls>
#!/bin/bashLet's look at the full file to understand context
cat code/core/src/manager/components/sidebar/useChecklist.ts | head -150
</function_calls><function_calls>
#!/bin/bashSearch for addons API or manager API definitions
find code -type f -name "*.ts" -path "api" ! -path "node_modules" | head -20
</function_calls>Let me search for the once and off implementations more directly:
<function_calls>
#!/bin/bashSearch for once method definition (not usage)
rg -n "^\sonce\s(" --type ts -B 3 -A 8 | head -80
</function_calls><function_calls>
#!/bin/bashSearch for Channel or EventEmitter-like patterns
rg -n "(class|export).*Channel" --type ts | head -20
</function_calls><function_calls>
#!/bin/bashSearch for methods named 'once' and 'off' in TypeScript files
rg -n "off\s*(" --type ts -B 5 | grep -A 5 "^\soff\s(" | head -50
</function_calls>Let me try a different approach and search in the API directory:
<function_calls>
#!/bin/bashLook for addons or manager API implementations
fd "addons" --type d code/ | head -5
</function_calls><function_calls>
#!/bin/bashSearch for EventEmitter or similar patterns
rg -n "EventEmitter|on\s*(|once\s*(" --type ts code/ -l | head -10
</function_calls>Let me check the addons API structure:
<function_calls>
#!/bin/bashFind API-related files in addons
find code -type f -path "addons" -name "*.ts" | grep -i api | head -20
</function_calls><function_calls>
#!/bin/bashLet's search in lib or shared directories
find code -type d ( -name "lib" -o -name "shared" -o -name "api" ) | head -10
</function_calls>Let me look at what directory structure exists:
<function_calls>
#!/bin/bashList top-level directories in code/
ls -la code/ | head -30
</function_calls><function_calls>
#!/bin/bashSearch for the api parameter usage pattern
rg -n "api.(once|off)" --type ts -B 2 -A 2 code/ | head -60
</function_calls>Now let me look for test files that might show how these methods work:
<function_calls>
#!/bin/bashSearch for tests related to once/off
fd ".*.test.(ts|tsx)" code/ -x rg -l "once|off" {} ; | head -10
</function_calls><function_calls>
#!/bin/bashLet's look for the specific implementation of event listeners
rg -n "export.*class.*Api" --type ts code/ | head -20
</function_calls>Let me check the addon-api directory specifically:
<function_calls>
#!/bin/bashFind addon-api
find code -type d -name "addon" | head -10
</function_calls><function_calls>
#!/bin/bashSearch for addons.api or manager.api imports
rg -n "from.*addon.*api|from.*manager.*api" --type ts code/ | head -20
</function_calls><function_calls>
#!/bin/bashFind where PREVIEW_INITIALIZED is defined
rg -n "PREVIEW_INITIALIZED" --type ts code/ | head -10
</function_calls>Now let me search for the actual implementation by looking at @storybook packages:
<function_calls>
#!/bin/bashLook at the package structure
find code -type f -name "package.json" | xargs grep -l "storybook/internal" 2>/dev/null | head -5
</function_calls><function_calls>
#!/bin/bashSearch for EventEmitter implementation or Channel class
rg -n "class.*Channel|class.*EventEmitter" --type ts code/ -B 2 -A 15 | head -100
</function_calls>Let me search for the specific addons API implementation:
<function_calls>
#!/bin/bashSearch more broadly for "once" implementations
ast-grep --pattern 'once($, $) { $$$ }'
</function_calls><function_calls>
#!/bin/bashSearch for function definitions of once
rg -n "^\s+(once|off)\s*(" --type ts -A 5 code/ | head -100
</function_calls>Let me try to find the Channel implementation:
<function_calls>
#!/bin/bashLook in the entire codebase for Channel-like classes
rg -n "class Channel" --type ts -A 20 code/ | head -150
</function_calls><function_calls>
#!/bin/bashSearch for on, once, off in object literals or class definitions
rg -n "once\s*[:=].*()" --type ts -B 2 -A 5 code/ | head -100
</function_calls>Let me look at the internal storybook packages:
<function_calls>
#!/bin/bashFind files in internal packages
find code -path "internal" -name "*.ts" | grep -i "api|channel|event" | head -20
</function_calls><function_calls>
#!/bin/bashSearch more carefully for the api object methods
rg -n ".once(" code/core/src/manager/components/sidebar/useChecklist.ts -B 5 -A 3
</function_calls><function_calls>
#!/bin/bashLet's check if it's using a standard EventEmitter pattern
rg -n "EventEmitter" --type ts code/ -B 2 -A 10 | head -100
</function_calls>Let me try searching in shared or lib directories more systematically:
<function_calls>
#!/bin/bashLook for addons API directory more carefully
ls -la code/core/src/addons/ 2>/dev/null || ls -la code/core/src/addon* 2>/dev/null || find code/core/src -maxdepth 2 -type d -name "addon"
</function_calls><function_calls>
#!/bin/bashSearch for where the api comes from in useChecklist
rg -n "useContext.*API|useAddonState|useManager" code/core/src/manager/components/sidebar/useChecklist.ts -B 2 -A 2
</function_calls>Let me get the full file content to understand the context:
<function_calls>
#!/bin/bashGet the entire file
cat code/core/src/manager/components/sidebar/useChecklist.ts
</function_calls>Perfect! Now let me look for the ManagerContext to understand where the api comes from:
<function_calls>
#!/bin/bashSearch for ManagerContext definition
rg -n "ManagerContext" code/ --type ts -B 2 -A 5 | head -50
</function_calls><function_calls>
#!/bin/bashFind the manager context file
find code -type f -name "managercontext*" -o -name "context" | grep manager
</function_calls><function_calls>
#!/bin/bashSearch for manager API or provider
rg -n "createContext.*api|ManagerApi|useApi" code/ --type ts | head -20
</function_calls>Let me search for where the api.once and api.off methods are actually used together:
<function_calls>
#!/bin/bashLook for all usages of api.once and api.off
rg -n "api.(once|off)" code/ --type ts -B 1 -A 1
</function_calls>Now let me check what type of API object this is - search for the actual implementation:
<function_calls>
#!/bin/bashSearch for API-related interfaces or types
rg -n "interface.*API|type.*API" code/ --type ts | grep -v ".test" | head -30
</function_calls><function_calls>
#!/bin/bashLet's look for the ManagerApi specifically
find code -type f -name "*.ts" | xargs grep -l "class ManagerApi|interface ManagerApi" 2>/dev/null | head -5
</function_calls><function_calls>
#!/bin/bashSearch in the manager directory structure
find code/core/src/manager -type f -name "*.ts" | head -30
</function_calls><function_calls>
#!/bin/bashLook for index or provider files in manager
ls -la code/core/src/manager/*.ts 2>/dev/null || ls -la code/core/src/manager/
</function_calls><function_calls>
#!/bin/bashCheck for the provider or main manager files
find code/core/src -name "provider" -o -name "manager.ts" | head -20
</function_calls>Let me look for the ManagerProvider specifically:
<function_calls>
#!/bin/bashSearch for ManagerProvider
rg -n "ManagerProvider|export.*Manager" code/core/src/manager --type ts -A 3 | head -80
</function_calls><function_calls>
#!/bin/bashLet's examine the manager directory structure more carefully
find code/core/src/manager -type f -name "*.ts" -exec basename {} ; | sort | uniq
</function_calls><function_calls>
#!/bin/bashFind the provider file
find code/core/src/manager -name "rovider" -o -name "index.ts"
</function_calls><function_calls>
#!/bin/bashList all files in manager directory
find code/core/src/manager -type f -name ".ts" -o -name ".tsx"
</function_calls><function_calls>
#!/bin/bashRead one of the manager files to understand the pattern
ls code/core/src/manager/.ts
</function_calls><function_calls>
#!/bin/bashGet more details about directory structure
ls -1 code/core/src/manager/
</function_calls><function_calls>
#!/bin/bashLook for ManagerProvider.tsx
cat code/core/src/manager/ManagerProvider.tsx 2>/dev/null | head -100
</function_calls><function_calls>
#!/bin/bashSearch in internal/addons or api modules
find code -path "/internal/" -name "*.ts" | xargs grep -l "once.*off|Channel" 2>/dev/null | head -10
</function_calls>Let me search for the actual Channel implementation across the whole repo:
<function_calls>
#!/bin/bashSearch for Channel class
rg -n "export.*class Channel" code/ --type ts -A 30 | head -150
</function_calls><function_calls>
#!/bin/bashLook for the implementation with a different approach - search for the pattern of method definitions
rg -n "^\s+(once|off)\s*(.)\s{" code/ --type ts -A 3 | head -100
</function_calls>Let me try searching in different packages:
<function_calls>
#!/bin/bashSearch for API implementations using different pattern
python3 << 'EOF'
import subprocess
import jsonSearch for files that have both "once" and "off" methods
result = subprocess.run(
['rg', '-l', 'once', '--type', 'ts', 'code/'],
capture_output=True,
text=True
)files_with_once = result.stdout.strip().split('\n')
for file in files_with_once[:20]:
if file:
# Check if file also has 'off' method
result2 = subprocess.run(
['rg', '-n', 'off\s*(|once\s*(', file],
capture_output=True,
text=True
)
if 'off' in result2.stdout and 'once' in result2.stdout:
print(f"Found in: {file}")
EOF
</function_calls><function_calls>
#!/bin/bashLet's use a simpler approach - look for where the methods are defined together
rg -n -A 15 "^\sonce\s(" code/ --type ts | grep -B 15 -A 5 "off\s*(" | head -100
</function_calls>Let me search more broadly for the EventEmitter-like pattern:
<function_calls>
#!/bin/bashSearch for common event emitter patterns
rg -n "listeners.*Map|_listeners|eventHandlers" code/ --type ts -B 3 -A 3 | head -80
</function_calls><function_calls>
#!/bin/bashLet's check in core/src itself for api-related files
find code/core/src -maxdepth 2 -type f -name "*.ts" | xargs grep -l "once|off" | head -20
</function_calls><function_calls>
#!/bin/bashFind all TypeScript files in code/core/src and search for the pattern
find code/core/src -type f -name ".ts" -o -name ".tsx" | xargs grep -l ".once\s*(" | head -10
</function_calls><function_calls>
#!/bin/bashLook for the actual API type being used
rg -n "api:\s*" code/core/src/manager/components/sidebar/useChecklist.ts -A 1
</function_calls>Let me check what the useContext returns:
<function_calls>
#!/bin/bashLook for ManagerContext type or provider
rg -n "ManagerContext" code/core/src --type ts -B 5 -A 10 | head -100
</function_calls>Let me try a web search to understand the pattern better, but first let me check if there are any comments or tests showing this pattern:
<function_calls>
#!/bin/bashSearch for test files that use api.once/off
find code -name ".test.ts" -o -name ".spec.ts" | xargs grep -l "api.(once|off)" 2>/dev/null | head -5
</function_calls><function_calls>
#!/bin/bashLook at what ManagerProvider.tsx contains to understand the api type
head -200 code/core/src/manager/ManagerProvider.tsx
</function_calls>Great! Now let me look for the API implementation:
<function_calls>
#!/bin/bashSearch for where the api object is created or what type it is
rg -n "new.*Api|createApi|makeApi" code/core/src/manager --type ts -B 2 -A 5 | head -50
</function_calls><function_calls>
#!/bin/bashLook for AddonStore or API manager
rg -n "class.*Store|export.*Store" code/ --type ts | grep -i addon | head -10
</function_calls>Let me search for the Channel implementation in addons API:
<function_calls>
#!/bin/bashSearch for Channel implementation more carefully
find code -type f -name "*.ts" | xargs grep -n "export class Channel|export.*Channel" 2>/dev/null | head -10
</function_calls><function_calls>
#!/bin/bashLet's look at the internal storybook packages
find code -path "/internal/" -type f -name "*.ts" | head -20
</function_calls>Let me examine one of the internal files:
<function_calls>
#!/bin/bashFind and check key files
find code -path "internal" -name "*.ts" -exec rg -l "once|Channel" {} ; | head -5
</function_calls><function_calls>
#!/bin/bashSearch in storybook related files for the implementation
rg -n "once\s*:" code/ --type ts -B 3 -A 8 | head -100
</function_calls>Let me check the storybook package folders structure:
<function_calls>
#!/bin/bashList storybook packages
ls -la code/
</function_calls><function_calls>
#!/bin/bashCheck what's in the lib folder if it exists
ls -la code/lib 2>/dev/null || ls -la code/
</function_calls>Let me look more carefully at what's imported in the manager files:
<function_calls>
#!/bin/bashSearch for all imports in ManagerProvider
cat code/core/src/manager/ManagerProvider.tsx | head -50
</function_calls><function_calls>
#!/bin/bashLet's look at what API is - search for where it comes from
grep -n "import.*api|from.*api" code/core/src/manager/ManagerProvider.tsx
</function_calls><function_calls>
#!/bin/bashFind the createApi function
rg -n "createApi|new API|new.*Api" code/core/src/manager --type ts -B 2 -A 10 | head -100
</function_calls><function_calls>
#!/bin/bashLook for the API implementation in core
ls -la code/core/src/api/ 2>/dev/null || find code/core/src -maxdepth 2 -type d -name "api"
</function_calls><function_calls>
#!/bin/bashCheck what exists in code/core/src
ls -la code/core/src/
</function_calls><function_calls>
#!/bin/bashLook in core for api files
find code/core/src -name "api" -type f
</function_calls>Let me check the addons folder:
<function_calls>
#!/bin/bashLook for addons API
ls -la code/core/src/addons/ 2>/dev/null || find code -type d -name "addon"
</function_calls><function_calls>
#!/bin/bashSearch for where AddonsStore or similar is imported
rg -n "import.*Store|from.*store" code/core/src/manager/ManagerProvider.tsx
</function_calls>Let me check if AddonsStore is the one being used:
<function_calls>
#!/bin/bashFind AddonsStore implementation
rg -n "class AddonsStore|export.*AddonsStore" code/ --type ts -A 30 | head -150
</function_calls>Great! Now let me look at the full AddonsStore implementation to understand the once/off methods:
<function_calls>
#!/bin/bashGet the AddonsStore file location and examine it
find code -name "AddonsStore" -o -name "addonsstore*" | head -5
</function_calls><function_calls>
#!/bin/bashLet's find the file containing AddonsStore
rg -n "class AddonsStore" code/ --type ts -l
</function_calls><function_calls>
#!/bin/bashGet the full AddonsStore implementation
rg -n "class AddonsStore" code/ --type ts -A 200 | head -250
</function_calls>Perfect! Now I can see the AddonsStore implementation. Let me focus on the once and off methods:
<function_calls>
#!/bin/bashGet the complete AddonsStore to understand once/off behavior
rg -n "class AddonsStore" code/ --type ts -A 300 | grep -A 300 "class AddonsStore" | head -350
</function_calls><function_calls>
#!/bin/bashLet's get the file path first
rg -n "class AddonsStore" code/ --type ts -l
</function_calls><function_calls>
#!/bin/bashRead the complete file containing AddonsStore
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
wc -l "$file"
cat -n "$file"
</function_calls>Excellent! Now I have the full AddonsStore implementation. Let me analyze the once and off methods more carefully:
<function_calls>
#!/bin/bashExtract just the once and off methods from AddonsStore
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
sed -n '/^\sonce(/,/^\s}/p' "$file" | head -30
</function_calls><function_calls>
#!/bin/bashGet the off method as well
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
sed -n '/^\soff(/,/^\s}/p' "$file" | head -30
</function_calls>Perfect! Now let me verify my understanding by checking if there are any related test or documentation:
<function_calls>
#!/bin/bashSearch for tests using api.once and api.off together
rg -n "api.once.*api.off|api.off.*once" code/ --type ts -B 3 -A 3 | head -60
</function_calls><function_calls>
#!/bin/bashCheck if there are other usages of this pattern
rg -n ".once(.*initialize)" code/ --type ts -B 2 -A 3
</function_calls><function_calls>
#!/bin/bashLook for other cleanup patterns with once
rg -n "useEffect.*once|once.*api." code/ --type ts -B 2 -A 5 | head -80
</function_calls>Now let me verify my understanding of the listener management:
<function_calls>
#!/bin/bashCheck if listeners are properly tracked
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
Extract the listeners property/field
cat -n "$file" | sed -n '/listeners\s*=/,/;/p' | head -10
</function_calls><function_calls>
#!/bin/bashGet better understanding of the on, once, off implementation
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
Show the relevant methods
awk '/^\s*(on|once|off)\s*(/{flag=1} flag{print NR": "$0} /^\s*}/{if(flag) flag=0}' "$file" | head -100
</function_calls>Let me read the file more carefully to understand the complete implementation:
<function_calls>
#!/bin/bashGet the actual file and look at lines around the methods
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
Find line numbers for the methods
line_on=$(grep -n "^\son(" "$file" | head -1 | cut -d: -f1)
line_once=$(grep -n "^\sonce(" "$file" | head -1 | cut -d: -f1)
line_off=$(grep -n "^\s*off(" "$file" | head -1 | cut -d: -f1)
echo "on at line: $line_on"
echo "once at line: $line_once"
echo "off at line: $line_off"Show each method
if [ ! -z "$line_on" ]; then
echo "=== on method ==="
sed -n "${line_on},$((line_on+15))p" "$file"
fiif [ ! -z "$line_once" ]; then
echo "=== once method ==="
sed -n "${line_once},$((line_once+15))p" "$file"
fiif [ ! -z "$line_off" ]; then
echo "=== off method ==="
sed -n "${line_off},$((line_off+15))p" "$file"
fi
</function_calls>Perfect! Now I have all the information I need to verify the review comment. Let me analyze the findings:
Now let me verify if there are test cases or documentation about this:
<function_calls>
#!/bin/bashSearch for tests of the AddonsStore to understand expected usage
find code -name ".test.ts" -o -name ".spec.ts" | xargs grep -l "AddonsStore|api.once|api.off" 2>/dev/null | head -5
</function_calls><function_calls>
#!/bin/bashLook for any JSDoc or comments explaining the expected behavior
rg -n "once|off" code/core/src/addons/index.ts -B 3 | head -80
</function_calls><function_calls>
#!/bin/bashCheck if there's documentation about proper cleanup pattern
rg -n "cleanup|useEffect" code/core/src/addons/index.ts -B 2 -A 2
</function_calls>Now let me verify my conclusion by testing the actual behavior:
<function_calls>
#!/bin/bashLet's verify the exact implementation once more to be absolutely certain
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
cat "$file" | python3 << 'PYEOF'
import sys
content = sys.stdin.read()Find once method
import re
once_match = re.search(r'once\s*(\sevent\s:\sstring[^{]{([^}](?:{[^}]}[^}]))}', content, re.DOTALL)
off_match = re.search(r'off\s*(\sevent\s:\sstring[^{]{([^}](?:{[^}]}[^}]))}', content, re.DOTALL)if once_match:
print("=== once method ===")
print(once_match.group(0)[:300])
print()if off_match:
print("=== off method ===")
print(off_match.group(0)[:300])
PYEOF
</function_calls>Let me take a simpler approach and just verify the key finding:
<function_calls>
#!/bin/bashVerify the critical difference: once wraps the listener
file=$(rg -n "class AddonsStore" code/ --type ts -l | head -1)
Show the once method - it wraps the callback
echo "=== Key finding: once() wraps the listener ==="
sed -n '/_once/,/^ *}/p' "$file" | head -20echo ""
echo "=== Key finding: off() uses reference equality ==="Show the off method - it looks for exact reference match
sed -n '/^ *off(/,/^ *}/p' "$file" | head -20
</function_calls>Based on my thorough investigation, I can now provide the final rewritten review comment:
The
api.offcall will not remove listeners registered withapi.onceif the event hasn't fired yet.The
AddonsStoreimplementation shows thatapi.once()wraps the provided callback in a separate wrapper function before adding it to the listeners array. Whenapi.off()is called with the originalinitializefunction, it searches for that exact reference in the listeners array. Sinceonce()added a wrapper (not the original function),off()won't find or remove it, leaving the wrapped listener registered. While this doesn't cause a memory leak in practice, it means the cleanup isn't functioning as intended. Consider usingapi.on()with manual cleanup or storing the wrapper function for proper cleanup.code/core/src/manager/components/sidebar/ChecklistWidget.tsx (5)
177-178: Potential issue with initialrenderItemsstate.
useState<ChecklistItem[]>(nextItems)capturesnextItemsonly at initial render. IfnextItemsis empty on first render but populated later,renderItemswon't update until the effect on lines 188-211 runs. This is likely intentional given the PR's goal of preventing layout shifts, but be aware thatrenderItemsmay be empty initially even whennextItemshas values from the second render onward.Is this intentional behavior? The initial render will always show no items until effects run, which could cause a brief flash if
nextItemsis immediately available.
180-186: LGTM for animation delay gating.The 1-second delay after
readybecomes true effectively prevents animations during the initial settling period. The cleanup correctly clears the timeout on unmount or ifreadychanges.
188-211: Two-phase animation logic is well-structured.The logic correctly handles:
- Immediate updates when not animated
- Preserving outgoing items with updated status for exit transitions
- 2-second delay before showing new items
One minor observation: the
animateOutcalculation accumulates across all items, so any completed/skipped item triggers the delay. This appears intentional for batch transitions.
213-217: Good conditional timeout for transition animations.Setting
timeout: 0when!animatedensures immediate rendering without transition delays during the initial load, improving perceived performance as intended by the PR.
311-315: LGTM for conditional particle animation.The particle effect is correctly gated behind
animatedto prevent premature visual effects. TheStatusFailIconfallback ensures a consistent visual state during initial render.
|
View your CI Pipeline Execution ↗ for commit 56da180
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
71-89: Consider guarding key access before parsing.The double non-null assertion
key!on line 74 is a code smell. While the try-catch handles undefined keys, the intent is unclear. Consider explicitly checkingkeybefore accessing sessionStorage:const useSessionState = <T,>(key: string | undefined, initialValue: T) => { const [value, setValue] = useState<T>(() => { + if (!key) return initialValue; try { - return (JSON.parse(sessionStorage.getItem(key!)!) as T) ?? initialValue; + const stored = sessionStorage.getItem(key); + return stored ? (JSON.parse(stored) as T) : initialValue; } catch { return initialValue; } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/components/components/Collapsible/Collapsible.tsx(3 hunks)code/core/src/manager/components/sidebar/ChecklistWidget.tsx(2 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/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/ChecklistWidget.tsx
**/*.{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/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/ChecklistWidget.tsx
**/*.{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/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/ChecklistWidget.tsx
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/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/ChecklistWidget.tsx
🧠 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/manager/components/sidebar/ChecklistWidget.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/ChecklistWidget.tsx (1)
code/core/src/manager/components/sidebar/useChecklist.ts (2)
useChecklist(111-250)ChecklistItem(30-41)
⏰ 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 (7)
code/core/src/components/components/Collapsible/Collapsible.tsx (2)
91-105: LGTM on the hook signature refactor.The object parameter pattern for
useCollapsibleis cleaner and more extensible. The conditional storage key prefixing withuseCollapsible:provides good namespacing for sessionStorage entries.
107-121: LGTM on the effect and toggle logic.The effect correctly syncs external
collapsedprop changes, andtoggleCollapsedproperly respects thedisabledstate. Dependency arrays are correct.code/core/src/manager/components/sidebar/ChecklistWidget.tsx (5)
176-178: LGTM on state initialization.The
renderItemsstate correctly initializes fromnextItems, and theanimatedflag gates transitions until the widget is ready. This aligns with the PR objective to avoid late layout shifts.
180-186: LGTM on animation delay effect.The 1-second delay after
readybecomes true ensures checklist items have settled before enabling animations. Cleanup function properly clears the timeout.
213-217: LGTM on transition array configuration.The conditional timeout (
animated ? 300 : 0) ensures no animation delay during initial render, preventing layout shifts. ThehasItemsderived state simplifies subsequent logic.
222-224: LGTM on Collapsible integration.The
storageKey="checklist-widget"enables persistent collapsed state across sessions, andinitialCollapsed={!hasItems}provides a sensible default based on content availability.
311-316: LGTM on conditional Particles animation.Showing the Particles animation only when
item.isCompleted && animatedprevents visual glitches during the initial render phase before the widget has settled.
What I did
useChecklistto addreadystate in order to prevent updating state while not ready.ChecklistWidgetanimations until settled down to prevent a late layout shift.ChecklistWidgetcollapsible state in sessionStorage.Checklist 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 pull request has been released as version
0.0.0-pr-33184-sha-43ff7fbc. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-33184-sha-43ff7fbcimprove-checklist-widget-transitions43ff7fbc1764155657)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33184Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.