Skip to content

Conversation

@ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 25, 2025

What I did

Chromatic has been complaining for a while about our outdated dependency. So I upgraded it.
I checked the breaking changes in v12 and v13, none of them apply to us.

Screenshot 2025-11-26 at 00 44 50

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

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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 publish.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

  • New Features

    • Checklist now staggers and enables animated enter/exit transitions once ready, providing smoother item animations and particle effect for completed items.
    • Visual status (completed/skipped/accepted) is preserved during exit animations for clearer state continuity.
  • Bug Fixes

    • Prevents premature animations before checklist is initialized, reducing flicker and layout jank.
  • Chores

    • Updated build tooling dependency to a newer Chromatic version.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 25, 2025

📝 Walkthrough

Walkthrough

Adds a readiness gate and debounced initialization to the checklist hook and wires it into the ChecklistWidget to delay and coordinate animated enter/exit transitions; also bumps chromatic in scripts/package.json from ^11.28.2 to ^13.3.4.

Changes

Cohort / File(s) Summary
Dependency version update
scripts/package.json
Bumped chromatic from ^11.28.2 to ^13.3.4.
Checklist UI and animation gating
code/core/src/manager/components/sidebar/ChecklistWidget.tsx
Adds renderItems state, animated flag, delayed animation enable (1s), exit/enter sequencing with timeouts (2s), status-preserving exit visuals, and toggles progress particle animation to require animated + isCompleted.
Checklist hook readiness & init
code/core/src/manager/components/sidebar/useChecklist.ts
Adds initialized/ready state with a debounced ready setter (500ms), subscribes to PREVIEW_INITIALIZED via api.once, gates setup on universal store status === READY, exposes ready in the hook return, and replaces previous debounce/throttle import usage.

Sequence Diagram(s)

sequenceDiagram
    participant Widget as ChecklistWidget (UI)
    participant Hook as useChecklist (hook)
    participant Store as UniversalStore
    participant API as ManagerAPI

    Note over Store,API: App start
    API->>Store: store status becomes READY
    API->>API: emit PREVIEW_INITIALIZED (once)
    API->>Hook: api.once(PREVIEW_INITIALIZED) callback
    Hook->>Hook: set initialized = true
    Hook->>Hook: wait for items and debounce(500ms)
    Hook->>Widget: return ready = true
    Widget->>Widget: after 1s delay set animated = true
    Widget->>Widget: run enter/exit transitions (2s)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review synchronization between PREVIEW_INITIALIZED, store status, and the debounce logic in useChecklist.
  • Verify timing constants (500ms debounce, 1s animation enable, 2s transition) and cleanup of event listeners/timeouts.
  • Check changed render/animation logic in ChecklistWidget.tsx for visual regressions and correctness of preserved status during exit transitions.

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08b56ed and f0f5d63.

📒 Files selected for processing (3)
  • code/core/src/manager/components/sidebar/ChecklistWidget.tsx (2 hunks)
  • code/core/src/manager/components/sidebar/useChecklist.ts (4 hunks)
  • scripts/package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/package.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use ESLint and Prettier configurations that are enforced in the codebase

Files:

  • code/core/src/manager/components/sidebar/ChecklistWidget.tsx
  • code/core/src/manager/components/sidebar/useChecklist.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode

Files:

  • code/core/src/manager/components/sidebar/ChecklistWidget.tsx
  • code/core/src/manager/components/sidebar/useChecklist.ts
code/**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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.tsx
  • code/core/src/manager/components/sidebar/useChecklist.ts
code/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Export functions that need to be tested from their modules

Files:

  • code/core/src/manager/components/sidebar/ChecklistWidget.tsx
  • code/core/src/manager/components/sidebar/useChecklist.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing

Files:

  • code/core/src/manager/components/sidebar/ChecklistWidget.tsx
  • code/core/src/manager/components/sidebar/useChecklist.ts
🧠 Learnings (5)
📚 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
🧬 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: nx
🔇 Additional comments (2)
code/core/src/manager/components/sidebar/ChecklistWidget.tsx (1)

176-217: Readiness-gated animations and renderItems transition logic look consistent.

The way you gate animated on ready (with a 1 s delay), keep renderItems in sync with nextItems while animated is false, and then run a 2 s exit phase for updated items before swapping to the new nextItems is coherent and avoids initial “jank”. The useTransitionArray integration with a dynamic timeout and the conditional particles icon (item.isCompleted && animated) also align well with that state machine. I don’t see any functional issues in this block.

Also applies to: 311-315

code/core/src/manager/components/sidebar/useChecklist.ts (1)

3-4: Based on my verification, I've confirmed the following about the original review comment:

Verification Results:

  1. Debounce cleanup issue (Line 121): ✅ CONFIRMED - The code does use debounce(() => setReady(true), 500) via useMemo without a cleanup effect. No cancellation mechanism is present, and this could theoretically lead to a state update on an unmounted component if the hook unmounts within the debounce window.

  2. PREVIEW_INITIALIZED late-mount issue (Lines 195-197): ✅ CONFIRMED - The code uses api.once(PREVIEW_INITIALIZED, initialize), which relies on the standard event listener pattern. The once() implementation in code/core/src/channels/main.ts (lines 101-104) uses a listener that removes itself after the first call—it does not replay past events. If the useChecklist hook mounts after the preview is already initialized, the initialized state would never become true, leaving ready permanently false.

  3. Code structure: The readiness gating logic (checking loaded and status === experimental_UniversalStore.Status.READY) is correctly implemented, but the late-mount vulnerability remains a concern for manager-only or deferred component loading scenarios.


Readiness gating is sound; confirm debounce cleanup and late-PREVIEW safeguard are needed.

The overall flow (gating on loaded + universalChecklistStore.status === experimental_UniversalStore.Status.READY, then exposing a debounced ready flag) is consistent and should prevent work before the universal store is ready. However, two lifecycle issues warrant attention:

  • Debounce cleanup: debounceReady never gets cancelled. If the hook unmounts within the 500 ms window, the debounced callback can still fire and call setReady(true) on an unmounted component, triggering React warnings. Add a cleanup effect:
  const debounceReady = useMemo(() => debounce(() => setReady(true), 500), []);

+  useEffect(
+    () => () => {
+      debounceReady.cancel?.();
+    },
+    [debounceReady]
+  );
  • Late PREVIEW_INITIALIZED mount: api.once(PREVIEW_INITIALIZED, initialize) does not replay past events. If useChecklist/ChecklistWidget mounts after the preview is already initialized, initialized would never flip to true, leaving ready permanently false. Verify whether this component can mount after preview initialization, and if so, add a fallback check against existing state (e.g., checking whether the API already reports a ready preview).
⛔ Skipped due to learnings
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.

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Nov 25, 2025

View your CI Pipeline Execution ↗ for commit 0aea115

Command Status Duration Result
nx run-many -t compile --parallel=3 ✅ Succeeded 44s View ↗

☁️ Nx Cloud last updated this comment at 2025-12-01 08:42:13 UTC

Comment on lines -176 to -195
const { loaded, allItems, nextItems, progress, accept, mute, items } = useChecklist();
const [renderItems, setItems] = useState<ChecklistItem[]>([]);
const { loaded, ready, allItems, nextItems, progress, accept, mute, items } = useChecklist();
const [renderItems, setRenderItems] = useState<ChecklistItem[]>(nextItems);
const [animated, setAnimated] = useState(false);

const hasItems = renderItems.length > 0;
const transitionItems = useTransitionArray(allItems, renderItems, {
keyFn: (item) => item.id,
timeout: 300,
});
useEffect(() => {
if (ready) {
// Don't animate anything until the checklist items have settled down.
const timeout = setTimeout(setAnimated, 1000, true);
return () => clearTimeout(timeout);
}
}, [ready]);

useEffect(() => {
// Render old items (with updated status) for 2 seconds before
if (!animated) {
setRenderItems(nextItems);
return;
}

// Render outgoing items with updated state for 2 seconds before
// rendering new items, in order to allow exit transition.
setItems((current) =>
current.map((item) => ({
...item,
isCompleted: items[item.id].status === 'accepted' || items[item.id].status === 'done',
isSkipped: items[item.id].status === 'skipped',
}))
);
const timeout = setTimeout(setItems, 2000, nextItems);
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem related to the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants