Skip to content

Conversation

@ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 21, 2025

What I did

When the controls, viewport or interactions feature is disabled via the features option in .storybook/main.ts, that makes it impossible to complete certain items on the checklist. This update hides those items when their required feature is disabled.

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 items for Guided Tour, Controls, Viewports, and Interactions are now feature-gated and only shown when the corresponding feature is enabled.
    • Opening the full guide from the sidebar tooltip now triggers the tooltip to close, improving the navigation/UX flow.

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

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

@ghengeveld ghengeveld requested a review from yannbf November 21, 2025 11:37
@ghengeveld ghengeveld self-assigned this Nov 21, 2025
@ghengeveld ghengeveld added bug ci:daily Run the CI jobs that normally run in the daily job. labels Nov 21, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

📝 Walkthrough

Walkthrough

Added feature-flag gating to four checklist items and introduced an optional afterClick callback when opening the guide from the sidebar tooltip to allow side effects (e.g., closing the tooltip) after navigation.

Changes

Cohort / File(s) Change Summary
Feature-gated checklist items
code/core/src/shared/checklist-store/checklistData.tsx
Added available functions to 4 checklist items: guidedTour (basics) now requires globalThis.FEATURES.controls plus existing checks; controls (development) requires globalThis.FEATURES.controls; viewports (development) requires globalThis.FEATURES.viewport; writeInteractions (testing) requires globalThis.FEATURES.interactions.
Sidebar guide open action
code/core/src/manager/components/sidebar/ChecklistWidget.tsx
Extended OpenGuideAction to accept an optional afterClick callback and invoke afterClick?.() after navigating to /settings/guide. Passed onHide as afterClick at the tooltip usage to close the tooltip after opening the guide.

Sequence Diagram(s)

sequenceDiagram
  participant Tooltip as TooltipList
  participant Action as OpenGuideAction
  participant Router as Router
  participant UI as Tooltip (hide)

  Tooltip->>Action: user clicks "Open Guide"
  activate Action
  Action->>Router: navigate('/settings/guide')
  Router-->>Action: navigation complete
  Action->>UI: afterClick?()
  Note right of UI `#DFF2E1`: afterClick triggers tooltip close (onHide)
  deactivate Action
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify feature flag keys (controls, viewport, interactions) and ensure they exist/are intended.
  • Confirm available functions' return semantics integrate correctly with checklist rendering logic.
  • Review OpenGuideAction signature change for backward compatibility and usages beyond this tooltip.
  • Confirm no race conditions or timing issues when calling afterClick after navigation.
✨ 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 db53296 and 61990b0.

📒 Files selected for processing (1)
  • code/core/src/manager/components/sidebar/ChecklistWidget.tsx (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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.
📚 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/ChecklistWidget.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: daily
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/core/src/manager/components/sidebar/ChecklistWidget.tsx (2)

149-172: LGTM! Clean callback enhancement.

The optional afterClick callback is well-implemented with proper typing, optional chaining, and backward compatibility. The execution order (navigation before callback) is appropriate for the use case of closing tooltips after navigation.


241-241: LGTM! Proper tooltip closure on navigation.

Passing afterClick={onHide} correctly ensures the tooltip closes when the user clicks "Open full guide" and navigates to the settings page. The selective usage of this prop (only in the tooltip, not in the header instances at lines 212-216) demonstrates appropriate context-aware implementation.


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

@nx-cloud
Copy link

nx-cloud bot commented Nov 21, 2025

View your CI Pipeline Execution ↗ for commit 61990b0

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

☁️ Nx Cloud last updated this comment at 2025-11-21 14:28:47 UTC

@ghengeveld ghengeveld enabled auto-merge November 21, 2025 15:41
@ghengeveld ghengeveld disabled auto-merge November 21, 2025 15:41
@ghengeveld ghengeveld merged commit 456e21c into next Nov 21, 2025
125 checks passed
@ghengeveld ghengeveld deleted the checklist-consider-features branch November 21, 2025 16:27
@github-actions github-actions bot mentioned this pull request Nov 23, 2025
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:daily Run the CI jobs that normally run in the daily job.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants