Skip to content

Conversation

@ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 25, 2025

What I did

Update default collapsed state for checklist sections and items so that items are initially collapsed unless they are explicitly linked to. Sections are initially expanded unless all of their items are completed.

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

Release Notes

  • Bug Fixes
    • Improved checklist collapse behavior to be more consistent and predictable. Sections now collapse based on completion status, and item collapse logic is simplified for more reliable interactions.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 25, 2025

📝 Walkthrough

Walkthrough

Simplified collapse logic in Checklist component by removing createRef import and refactoring section and item collapse behavior. Section collapse now depends solely on progress and locationHash absence, while item collapse only depends on locationHash matching.

Changes

Cohort / File(s) Summary
Checklist component refactoring
code/core/src/manager/settings/Checklist/Checklist.tsx
Removed createRef import; simplified section collapse logic to depend on section progress (100%) and locationHash presence; simplified item collapse logic to depend only on locationHash matching; removed index-based logic and checked state dependencies.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that the new collapse logic (based on progress and locationHash) correctly implements the intended UI behavior
  • Confirm that removing the item's checked state from collapse conditions doesn't inadvertently affect collapsible state in edge cases
  • Validate that locationHash comparisons work correctly for both sections and individual items
✨ 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 38d9dec and 33b47ad.

📒 Files selected for processing (1)
  • code/core/src/manager/settings/Checklist/Checklist.tsx (3 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/settings/Checklist/Checklist.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/manager/settings/Checklist/Checklist.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/manager/settings/Checklist/Checklist.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/manager/settings/Checklist/Checklist.tsx
🧠 Learnings (3)
📚 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/settings/Checklist/Checklist.tsx
📚 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/settings/Checklist/Checklist.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/core/src/manager/settings/Checklist/Checklist.tsx
⏰ 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 (3)
code/core/src/manager/settings/Checklist/Checklist.tsx (3)

1-1: LGTM: Clean import statement.

The removal of createRef aligns with the simplified collapse logic that no longer requires ref-based tracking.


316-316: Simplified item collapse logic aligns with PR objectives.

The change correctly implements the requirement that items are collapsed by default unless explicitly linked to. Removing the dependency on isAccepted/isDone ensures consistent behavior where completion status doesn't affect collapse state.


271-272: No issues found with edge case handling.

The useLocationHash hook at git/code/core/src/manager/components/useLocationHash.ts:37-41 always returns a string (either the hash without # or an empty string), making the collapse logic at lines 271-272 safe and predictable. Edge cases are handled correctly:

  • Empty string when no hash: sections collapse if complete
  • Valid ID: matching sections stay expanded
  • Never returns null or undefined

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 c905dbb

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

☁️ Nx Cloud last updated this comment at 2025-11-25 11:55:51 UTC

@ghengeveld ghengeveld merged commit 6e66a2e into next Nov 25, 2025
66 checks passed
@ghengeveld ghengeveld deleted the collapse-checklist-items-by-default branch November 25, 2025 12:22
@github-actions github-actions bot mentioned this pull request Nov 25, 2025
10 tasks
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