Skip to content

Conversation

@Sidnioulz
Copy link
Member

@Sidnioulz Sidnioulz commented Dec 3, 2025

Closes #33248

What I did

  • Fixed a bug in PopoverProvider's built-in close button, that didn't work with controlled-state PopoverProviders
  • Fixed the HTML markup for the "show more" button in long type enums, to use HTML details for better keyboard and AT support
  • Replaced the type detail tooltip with a popover
    • Fixed up the width calculation logic which was outdated and caused unnecessary horizontal scrollbars
    • Fixed the transparency bug
    • Switched to a popover, because this tooltip sometimes requires scrolling, hinting we need to be able to navigate inside it including with a keyboard (hence it needs to grab keyboard focus)
    • Added a close button, so KB users can easily close the popover

This is a pretty opinionated change. The alternative would be to use aria-describedby (which is the default behaviour of RAC tooltips) but I'm worried the content can be a bit long for ARIA descriptions.

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

  1. Run SB instance
  2. Navigate to the Func story and the new LongFunc story
  3. Navigate to the LongEnum story

Documentation

ø

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

  • Tests

    • Added interactive story examples (LongFunc and LongEnum) demonstrating expanded-detail behavior and automated play routines that validate expand/collapse and content visibility.
  • Refactor

    • Argument-value display now uses a popover/details pattern instead of tooltips, simplifying expansion and rendering of long details.
    • Popover hide/show now flows through the standard open/close handler for consistent visibility.

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

@nx-cloud
Copy link

nx-cloud bot commented Dec 3, 2025

View your CI Pipeline Execution ↗ for commit b21d8fe

Command Status Duration Result
nx run-many -t check -c production --parallel=7 ✅ Succeeded 10s View ↗
nx run-many -t compile -c production --parallel=3 ✅ Succeeded 2s View ↗

☁️ Nx Cloud last updated this comment at 2025-12-10 13:26:03 UTC

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

📝 Walkthrough

Walkthrough

Adds a new ArgRow story (LongFunc) with interactive play tests, refactors ArgValue to use a Popover-based collapsible details layout (details/summary + popover slot), and changes PopoverProvider.onHide to delegate via onOpenChange(false).

Changes

Cohort / File(s) Summary
ArgRow stories (interactive examples)
code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
Added LongFunc story (copies Func.args.row, sets table.defaultValue.detail to a multi-line function string) and added/updated play handlers for LongFunc and LongEnum to perform UI interactions and assertions; added Storybook type imports and updated default export to } satisfies Meta<typeof ArgRow>;.
ArgValue UI restructure
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
Replaced TooltipProvider with PopoverProvider; introduced DetailsContainer and AlignedDetails; switched expansion control to details/summary semantics (summary element used for toggle); removed width calculation and width prop; always render summary items and render expanded items inside the collapsible container; moved detail content into a popover slot and added optional close-button support.
Popover provider behavior
code/core/src/components/components/Popover/PopoverProvider.tsx
onHide now calls onOpenChange(false) and includes onOpenChange in its dependency array instead of calling setIsOpen(false) directly, so hide actions propagate through the open-change handler.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor User
    participant ArgRow as ArgRow / ArgValue
    participant PopProv as PopoverProvider
    participant Popover as Popover (content)
    User->>ArgRow: click summary / "func" / expand control
    ArgRow->>PopProv: request open (show details / render popover slot)
    PopProv->>Popover: render detail content (+ optional close button)
    Popover-->>User: display popover with expandable details
    User->>Popover: interact (show more / click item / close)
    Popover->>PopProv: trigger onHide / close action
    PopProv->>PopProv: onOpenChange(false) (propagate hide)
    PopProv-->>ArgRow: notify closed via onOpenChange/onVisibleChange
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Review code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx for accessibility (details/summary semantics), keyboard/focus handling, and parity with previous tooltip behavior.
  • Verify code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx play handlers and Storybook typing (satisfies Meta<typeof ArgRow>) work in CI.
  • Inspect code/core/src/components/components/Popover/PopoverProvider.tsx for reentrancy and side effects when delegating hide to onOpenChange(false).
✨ 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 1d080ba and b21d8fe.

📒 Files selected for processing (1)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.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). (3)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (1)

39-51: Consider adding cursor: pointer to the summary element.

The DetailsContainer correctly hides the default disclosure marker, but users may not recognize the summary as interactive without a pointer cursor.

 const DetailsContainer = styled.details({
   display: 'flex',
   flexDirection: 'column',
   summary: {
     order: 2,
+    cursor: 'pointer',
   },
   'summary::-webkit-details-marker': {
     display: 'none',
   },
   'summary::marker': {
     content: 'none',
   },
 });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a60c55c and 6a920e5.

📒 Files selected for processing (3)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx (1 hunks)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (7 hunks)
  • code/core/src/components/components/Popover/PopoverProvider.tsx (1 hunks)
🧰 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
  • code/core/src/components/components/Popover/PopoverProvider.tsx
**/*.{ts,tsx}

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

Enable TypeScript strict mode

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
  • code/core/src/components/components/Popover/PopoverProvider.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
  • code/core/src/components/components/Popover/PopoverProvider.tsx
code/**/*.{ts,tsx,js,jsx}

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

Export functions that need to be tested from their modules

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
  • code/core/src/components/components/Popover/PopoverProvider.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
  • code/core/src/components/components/Popover/PopoverProvider.tsx
🧠 Learnings (6)
📓 Common learnings
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.
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: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
🧬 Code graph analysis (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (2)
code/core/src/components/components/Popover/PopoverProvider.tsx (1)
  • PopoverProvider (55-102)
code/core/src/components/index.ts (1)
  • PopoverProvider (71-71)
⏰ 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). (4)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/core/src/components/components/Popover/PopoverProvider.tsx (1)

80-80: LGTM! Proper fix for controlled-state PopoverProviders.

Delegating through onOpenChange ensures both the internal state (setIsOpen) and the external callback (onVisibleChange) are invoked when the close button is used. This correctly fixes the bug where controlled PopoverProviders wouldn't be notified of visibility changes.

code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (2)

192-213: Good migration to PopoverProvider with proper accessibility support.

The controlled state pattern with visible/onVisibleChange is correctly implemented, and hasCloseButton enables keyboard users to close the popover. This aligns with the learnings to prefer PopoverProvider over legacy WithTooltip.


98-111: LGTM!

The Detail styled component is well-structured with sensible constraints (minWidth: 200, maxWidth: 800) and the paddingRight: 16 correctly accommodates the close button. The helpful comment about boxSizing aids future maintainability.

code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx (1)

312-349: Good test coverage for long function details.

This story effectively tests the new popover behavior with extended content, ensuring the close button and scrolling work correctly for lengthy function signatures. The story structure correctly extends the existing Func story while overriding only the defaultValue.detail.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (1)

39-58: Details-based expansion logic looks solid; consider dropping role="button" on <summary>.

The switch to <details>/<summary> plus open={isExpanded} and onToggle={(e) => setIsExpanded(e.currentTarget.open)} keeps the native state and React state in sync and fixes the previous desync issue for the “show more” label. This is a good accessibility-friendly pattern.

One small optional tweak: <summary> already has native button-like semantics and state announcements; overriding it with role="button" can interfere with those semantics in some ATs. You can keep the current behavior and simplify the markup by removing the role:

-          <ExpandButton role="button">
+          <ExpandButton>

Also applies to: 80-87, 140-150, 178-186

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a920e5 and bb9681a.

📒 Files selected for processing (1)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (7 hunks)
🧰 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
**/*.{ts,tsx}

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

Enable TypeScript strict mode

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
code/**/*.{ts,tsx,js,jsx}

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

Export functions that need to be tested from their modules

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
🧠 Learnings (7)
📓 Common learnings
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.
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: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
🧬 Code graph analysis (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (2)
code/core/src/components/components/Popover/PopoverProvider.tsx (1)
  • PopoverProvider (55-102)
code/core/src/components/index.ts (1)
  • PopoverProvider (71-71)
⏰ 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). (3)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
🔇 Additional comments (2)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (2)

99-103: Detail popover width constraints are reasonable for avoiding layout issues.

The fixed minWidth/maxWidth plus added right padding should help avoid the old horizontal scrollbar / full-panel overflow problems while keeping long signatures readable. No changes needed here from my side.


4-4: PopoverProvider integration correctly controls visibility and chevron state.

Using visible={isOpen} with onVisibleChange={setIsOpen} keeps the PopoverProvider and the chevron icon in sync, and hasCloseButton plus the scrollable popover content address the original tooltip usability issues. This fits existing PopoverProvider patterns in the repo; I don’t see further changes needed here.

Also applies to: 194-203, 213-213

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (2)

190-195: State synchronization looks good; remove redundant role="button".

The onToggle handler correctly addresses the previous review concern about state desync. However, the <summary> element already has implicit button semantics (role of "button"), so the explicit role="button" is redundant and may confuse some assistive technologies.

-          <ExpandButton role="button">
+          <ExpandButton>
             {isExpanded ? 'Show less...' : `Show ${itemsCount - ITEMS_BEFORE_EXPANSION} more...`}
           </ExpandButton>

218-221: Add aria-expanded for accessibility.

The Expandable button controls a popover's visibility but doesn't communicate this state to assistive technology users. Adding aria-expanded improves keyboard/screen reader accessibility.

-      <Expandable className="sbdocs-expandable">
+      <Expandable className="sbdocs-expandable" aria-expanded={isOpen}>
         <span>{summaryAsString}</span>
         {isOpen ? <ChevronUpIcon /> : <ChevronDownIcon />}
       </Expandable>
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb9681a and 5862706.

📒 Files selected for processing (1)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (6 hunks)
🧰 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
**/*.{ts,tsx}

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

Enable TypeScript strict mode

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
code/**/*.{ts,tsx,js,jsx}

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

Export functions that need to be tested from their modules

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
🧬 Code graph analysis (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (2)
code/core/src/components/index.ts (2)
  • codeCommon (129-129)
  • PopoverProvider (71-71)
code/core/src/components/components/Popover/PopoverProvider.tsx (1)
  • PopoverProvider (55-102)
⏰ 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). (3)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (6)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (6)

1-11: LGTM!

The migration from TooltipProvider to PopoverProvider aligns with the codebase direction to prefer Popover over the deprecated WithTooltip. Based on learnings, this is the correct approach.


39-58: Good use of native <details> with custom styling.

The marker hiding and CSS ordering to position the summary after the expanded content is a clean approach for this "show more" pattern.


80-87: LGTM!

Using styled.summary for the expand/collapse trigger works correctly with the native <details> element.


108-121: LGTM!

The simplified width constraints and added padding accommodate the popover's close button appropriately.


149-159: LGTM!

The rendering functions correctly split items around the expansion threshold. Using item as the key is safe since getSummaryItems already deduplicates with uniq().


98-101: Invalid HSL syntax: saturation and lightness require percentage units.

The HSL values are missing the % unit for saturation and lightness. In CSS Color Level 4 space-separated syntax, these values must be percentages. Without them, the border may not render correctly across browsers.

Apply this diff:

   '&:hover': {
     border:
-      theme.base === 'light' ? '1px solid hsl(0 0 0 / 0.15)' : '1px solid hsl(0 0 100 / 0.15)',
+      theme.base === 'light' ? '1px solid hsl(0 0% 0% / 0.15)' : '1px solid hsl(0 0% 100% / 0.15)',
   },
⛔ Skipped due to learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/theming/themes/dark.ts:34-36
Timestamp: 2025-09-21T15:08:50.897Z
Learning: In CSS Color Module Level 4, HSL syntax allows both percentage and numeric values for saturation and lightness components. The syntax `hsl(0 0 100 / 0.1)` is valid CSS4 - percentages are not required in modern HSL syntax. Both `hsl(0 0% 100% / 0.1)` and `hsl(0 0 100 / 0.1)` are correct.
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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (1)

220-223: Consider marking decorative icons as aria-hidden.

The chevron icons are purely decorative since the button text already conveys the expand/collapse state. Marking them as aria-hidden="true" prevents screen readers from announcing redundant information.

Apply this diff:

       <Expandable className="sbdocs-expandable">
         <span>{summaryAsString}</span>
-        {isOpen ? <ChevronUpIcon /> : <ChevronDownIcon />}
+        {isOpen ? <ChevronUpIcon aria-hidden="true" /> : <ChevronDownIcon aria-hidden="true" />}
       </Expandable>
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5862706 and 2dc5fa9.

📒 Files selected for processing (1)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (6 hunks)
🧰 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
**/*.{ts,tsx}

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

Enable TypeScript strict mode

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
code/**/*.{ts,tsx,js,jsx}

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

Export functions that need to be tested from their modules

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 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/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.

Applied to files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
🧬 Code graph analysis (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (2)
code/core/src/components/index.ts (2)
  • codeCommon (129-129)
  • PopoverProvider (71-71)
code/core/src/components/components/Popover/PopoverProvider.tsx (1)
  • PopoverProvider (55-102)
⏰ 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). (3)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
🔇 Additional comments (7)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx (7)

4-4: LGTM: Appropriate replacement of tooltip with popover.

The switch from TooltipProvider to PopoverProvider aligns with the PR objective to improve accessibility and scrollability of long type details. Based on learnings, the legacy WithTooltip is deprecated in favor of PopoverProvider.


39-58: LGTM: Semantic HTML with appropriate customization.

The use of native <details> and <summary> elements provides built-in keyboard navigation and accessibility. The marker hiding and visual reordering are appropriate customizations that don't affect the underlying semantics.


80-89: LGTM: Semantic <summary> element is appropriate.

The <summary> element is the correct semantic choice for a disclosure widget within <details>. It provides built-in keyboard navigation (Space/Enter) and is already interactive, so no additional WCAG compliance changes are needed. The past comment about requiring a button for WCAG compliance is resolved by this implementation.


91-108: LGTM: Proper focus and hover styling.

The focus-visible and hover states follow accessibility best practices with visible outlines and appropriate offset. The theme-aware hover border enhances usability.


151-161: LGTM: Clean separation of summary and expanded items.

The rendering functions are straightforward and maintain clear separation between initially visible and expanded items.


192-193: LGTM: State synchronization properly implemented.

The controlled open prop combined with the onToggle handler correctly synchronizes React state with the native <details> element state. This addresses the concern raised in the previous review about potential state desync.


205-224: LGTM: PopoverProvider implementation looks correct.

The PopoverProvider usage aligns with the API shown in the relevant code snippets. The hasCloseButton prop enables keyboard-accessible dismissal, and the controlled visible/onVisibleChange pattern properly manages popover state.

@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-33248 branch from 1d080ba to b21d8fe Compare December 10, 2025 13:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx (1)

278-314: Consider adding a play function to the original Func story.

Per past review feedback requesting play functions for visual testing, LongFunc now has one, but the original Func story still doesn't. Adding a similar play function here would provide consistent test coverage for the popover behavior across both variants.

Based on learnings from past review feedback.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dc5fa9 and 1d080ba.

📒 Files selected for processing (1)
  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx (5 hunks)
🧰 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/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
**/*.{ts,tsx}

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

Enable TypeScript strict mode

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
code/**/*.{ts,tsx,js,jsx}

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

Export functions that need to be tested from their modules

Files:

  • code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
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/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx
🧠 Learnings (1)
📓 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/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.
⏰ 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). (3)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (6)
code/addons/docs/src/blocks/components/ArgsTable/ArgRow.stories.tsx (6)

5-8: LGTM! Type-safe story definitions and testing utilities.

The addition of Meta and StoryObj types, along with testing utilities, properly supports the new type-safe story definitions and interactive play functions introduced later in the file.


17-17: LGTM! Improved type safety.

Removing the explicit : any annotation allows TypeScript to properly infer the decorator parameter type, improving type safety in line with strict mode requirements.


28-28: LGTM! Type constraint added.

The satisfies clause ensures the meta object conforms to the expected type while preserving precise type inference.


316-352: Good addition of test case for long function details.

The LongFunc story effectively tests the popover behavior with substantial content, which helps validate the fix for the tooltip readability issue.


398-404: LGTM! Comprehensive test of expand/collapse behavior.

The play function properly validates both the expansion and collapse of the long enumeration details, using consistent canvas queries throughout. The visibility assertions confirm the interactive behavior works as expected.


353-360: The code correctly mixes canvas and screen queries—no change needed.

The canvas.getByRole call targets the button trigger within the story canvas, while screen.findByText correctly targets the popover content rendered in a portal by PopoverProvider. This mixing is semantically correct and follows the recommended testing pattern for portal-based overlays. The LongEnum reference is not applicable, as it uses a <details> element within the canvas rather than a PopoverProvider.

@storybook-app-bot
Copy link

Package Benchmarks

Commit: b21d8fe, ran on 10 December 2025 at 13:23:03 UTC

The following packages have significant changes to their size or dependencies:

storybook

Before After Difference
Dependency count 39 39 0
Self size 20.54 MB 20.52 MB 🎉 -15 KB 🎉
Dependency size 16.41 MB 16.41 MB 0 B
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 173 173 0
Self size 774 KB 774 KB 🎉 -505 B 🎉
Dependency size 67.61 MB 67.60 MB 🎉 -15 KB 🎉
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 166 166 0
Self size 30 KB 30 KB 🎉 -36 B 🎉
Dependency size 66.19 MB 66.17 MB 🎉 -15 KB 🎉
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 40 40 0
Self size 1000 KB 1000 KB 🚨 +73 B 🚨
Dependency size 36.94 MB 36.93 MB 🎉 -15 KB 🎉
Bundle Size Analyzer node node

@valentinpalkovic valentinpalkovic changed the title Bug: Fix broken tooltip in ArgValue details Docs-Blocks: Fix broken tooltip in ArgValue details Dec 10, 2025
@valentinpalkovic valentinpalkovic merged commit 08d73d9 into next Dec 10, 2025
69 of 70 checks passed
@valentinpalkovic valentinpalkovic deleted the sidnioulz/issue-33248 branch December 10, 2025 20:21
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.

[Bug]: Storybook 10 - type detail tooltip has transparent background

4 participants