UI: Fix code/copy buttons overlap with content#33889
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 2f69d12
☁️ Nx Cloud last updated this comment at |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Preview/source rendering and action bar layout: adds ActionBar flexLayout, inlines action bar markup, replaces legacy source handling with hasValidSource/hasSourceError flags, adds copyable prop to Source and copy-to-clipboard feedback, and updates SyntaxHighlighter layout to support flexible action bar placement. Changes
Sequence Diagram(s)sequenceDiagram
participant Story as Storybook Story
participant Preview as Preview Component
participant ActionBar as ActionBar
participant Source as Source / SyntaxHighlighter
participant Clipboard as Clipboard
Story->>Preview: render Preview (with additionalActions)
Preview->>ActionBar: render toolbar (flexLayout conditional)
Preview->>Source: request source content / expanded state
Source-->>Preview: return source or error
Note over Preview,Source: hasValidSource / hasSourceError determine rendering
ActionBar->>Clipboard: (user) click Copy
Clipboard-->>ActionBar: copyToClipboard result
ActionBar->>Preview: set copied state ("Copied!" feedback)
Preview->>Story: update UI (Copied badge / source visibility)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/components/components/ActionBar/ActionBar.stories.tsx`:
- Around line 30-32: The Storybook meta uses args.sb11FlexLayout which doesn't
match ActionBar's prop flexLayout and the stories don't pass args through the
render so toggling has no effect; rename the meta arg from sb11FlexLayout to
flexLayout and update the story renders (the story function that returns
<ActionBar ...>) to accept and spread args (e.g., (args) => <ActionBar {...args}
/>) so the flexLayout arg is forwarded to the ActionBar component.
code/core/src/components/components/ActionBar/ActionBar.stories.tsx
Outdated
Show resolved
Hide resolved
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 1.64 MB | 1.66 MB | 🚨 +20 KB 🚨 |
| Dependency size | 9.25 MB | 9.25 MB | 🚨 +12 B 🚨 |
| Bundle Size Analyzer | Link | Link |
Sidnioulz
force-pushed
the
sidnioulz/issue-23642
branch
from
0516ea7 to
6700746
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/components/components/ActionBar/ActionBar.stories.tsx (1)
30-32: Story args don’t reachActionBar(same as prior review).
sb11FlexLayoutdoesn’t match theActionBarpropflexLayout, and the stories don’t spread args into<ActionBar>, so the control has no effect. Align the arg name and pass args through.🔧 Suggested fix
-import { ActionBar } from './ActionBar'; +import { ActionBar, type ActionBarProps } from './ActionBar'; export default { component: ActionBar, ... - args: { - sb11FlexLayout: true, - }, + args: { + flexLayout: true, + }, }; -export const SingleItem = () => <ActionBar actionItems={[{ title: 'Clear', onClick: action1 }]} />; +export const SingleItem = (args: ActionBarProps) => ( + <ActionBar {...args} actionItems={[{ title: 'Clear', onClick: action1 }]} /> +); -export const ManyItems = () => ( +export const ManyItems = (args: ActionBarProps) => ( <ActionBar + {...args} actionItems={[ { title: 'Action string', onClick: action1 }, { title: <div>Action node</div>, onClick: action2 }, { title: 'Long action string', className: 'long-action-button', onClick: action3 }, ]} /> );Please also run the ESLint command for this file:
#!/bin/bash yarn lint:js:cmd code/core/src/components/components/ActionBar/ActionBar.stories.tsxAs per coding guidelines:
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run ESLint checks with the command 'yarn lint:js:cmd ' and fix all errors or warnings before committing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/ActionBar/ActionBar.stories.tsx` around lines 30 - 32, The story args key is wrong and the args are not forwarded to the component: rename the arg from sb11FlexLayout to flexLayout in the Story args and update the story render so it spreads args into the component (use ActionBar and pass {...args} when rendering) so the control affects the component; then run the ESLint check for this file with `yarn lint:js:cmd code/core/src/components/components/ActionBar/ActionBar.stories.tsx` and fix any reported issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/core/src/components/components/ActionBar/ActionBar.stories.tsx`:
- Around line 30-32: The story args key is wrong and the args are not forwarded
to the component: rename the arg from sb11FlexLayout to flexLayout in the Story
args and update the story render so it spreads args into the component (use
ActionBar and pass {...args} when rendering) so the control affects the
component; then run the ESLint check for this file with `yarn lint:js:cmd
code/core/src/components/components/ActionBar/ActionBar.stories.tsx` and fix any
reported issues.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/addons/docs/src/blocks/components/Preview.stories.tsxcode/addons/docs/src/blocks/components/Preview.tsxcode/core/src/components/components/ActionBar/ActionBar.stories.tsxcode/core/src/components/components/ActionBar/ActionBar.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
- code/core/src/components/components/ActionBar/ActionBar.tsx
Sidnioulz
commented
Feb 25, 2026
Sidnioulz
force-pushed
the
sidnioulz/issue-23642
branch
from
0b9644c to
be2b514
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/docs/src/blocks/components/Preview.tsx`:
- Around line 190-194: PreviewContainer's computed border/radius depends on the
isExpanded prop but the component render only spreads withSource/withToolbar and
props; it never passes the current expanded state, so toggling expanded doesn't
change styling. Fix by passing the expanded state into PreviewContainer as
isExpanded (e.g., add isExpanded={expanded} when rendering PreviewContainer) so
its internal logic (isExpanded) receives the live expanded value; ensure the
prop name matches PreviewContainer's expected prop (isExpanded) and remove any
conflicting prop in props if present.
- Around line 160-162: The component currently initializes additionalActionItems
from props using useState which freezes the value at mount and ignores prop
updates; replace the state initialization with a derived value so
additionalActionItems reflects prop changes (e.g., remove the useState usage for
additionalActionItems and compute it from the additionalActions prop each
render, optionally using useMemo if you want to avoid reallocating the array
unnecessarily), updating references to additionalActionItems in Preview.tsx to
use the derived constant.
- Around line 173-181: The handleCopyCode callback currently ignores failures
from copyToClipboard and can produce unhandled promise rejections; wrap the
copyToClipboard(...).then(...) chain with .catch(...) to handle errors, use the
logger from 'storybook/internal/client-logger' to log the error, and ensure
component state is consistent (clear any transient copied state or leave it
false) on failure; update handleCopyCode (and add the logger import if missing)
to call logger.error(...) in the catch and to not leave setCopied(true) when the
copy fails, while keeping the existing success timeout logic using
COPIED_LABEL_ANIMATION_DURATION, setCopied, withSource?.code, and
globalThis.window.setTimeout.
- Around line 229-251: The Button and ToggleButton instances in Preview.tsx (the
disabled Button with "No code available", the ToggleButton controlling expanded
with props pressed/aria-expanded/aria-controls/onClick, and the Button that
calls handleCopyCode showing copied state) need explicit ariaLabel={false} per
repo convention; update each Button and ToggleButton JSX element (the Button
rendering "No code available", the ToggleButton using
expanded/pressed/aria-controls/sourceId/onClick/setExpanded, and the Copy button
invoking handleCopyCode and showing copied) to include ariaLabel={false} so they
rely on their visible children for accessible names.
In `@code/addons/docs/src/blocks/components/Source.tsx`:
- Around line 47-48: The SourceCodeProps interface has multiple members and doc
comments squashed onto single lines — split each property onto its own line with
its preceding JSDoc/comment above it and place the closing brace on its own
line; specifically update the interface named SourceCodeProps so that the
`dark?: boolean;` and `copyable?: boolean;` declarations each sit on separate
lines with their doc comments immediately above them and then terminate the
interface with a standalone `}`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/addons/docs/src/blocks/components/Preview.tsxcode/addons/docs/src/blocks/components/Source.tsx
Sidnioulz
force-pushed
the
sidnioulz/issue-23642
branch
from
64a1f83 to
b0f94e7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/addons/docs/src/blocks/components/Preview.tsx (1)
257-264:⚠️ Potential issue | 🟡 MinorAdd
ariaLabelon mappedButtonactions for consistency.This callsite should explicitly pass
ariaLabel={false}(when visible children provide the name), matching the repo’s Button usage convention.♿ Suggested patch
{additionalActionItems.map(({ title, className, onClick, disabled }, index: number) => ( <Button key={index} + ariaLabel={false} className={className} onClick={onClick} disabled={!!disabled} variant="ghost" > {title} </Button> ))}Based on learnings: "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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/docs/src/blocks/components/Preview.tsx` around lines 257 - 264, The mapped Button instances in the additionalActionItems render (inside the additionalActionItems.map block) are missing the explicit ariaLabel prop; follow the repo convention by adding ariaLabel={false} to the Button call so the children continue to serve as the accessible name. Locate the mapping over additionalActionItems in Preview.tsx (the JSX that returns <Button key={index} ... />) and add ariaLabel={false} to the Button props for consistency and accessibility.
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/components/Preview.stories.tsx (1)
54-65: Nice repro story; consider adding an assertion-backed regression test.This story is a solid manual repro. A follow-up visual/interaction assertion for the wrapped action row would better protect against overlap regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/docs/src/blocks/components/Preview.stories.tsx` around lines 54 - 65, Add an automated assertion-backed regression test for the ActionBarWrapping story to lock in the wrapped action row behavior: create a test (either a Storybook play test or a Jest/React Testing Library test) that mounts the Preview with the same props used in ActionBarWrapping (Preview with inline, isExpanded, withSource set to sourceStories.JSX.args and the Button child) and asserts the visual/DOM outcome you care about (e.g., that the action row wraps and buttons do not overlap or that the Button is visible and positioned on a new line). Reference the existing story name ActionBarWrapping, the Preview component, and the Button element to mirror the repro exactly and include clear assertions for the wrap/overlap condition so future regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/addons/docs/src/blocks/components/Preview.tsx`:
- Around line 257-264: The mapped Button instances in the additionalActionItems
render (inside the additionalActionItems.map block) are missing the explicit
ariaLabel prop; follow the repo convention by adding ariaLabel={false} to the
Button call so the children continue to serve as the accessible name. Locate the
mapping over additionalActionItems in Preview.tsx (the JSX that returns <Button
key={index} ... />) and add ariaLabel={false} to the Button props for
consistency and accessibility.
---
Nitpick comments:
In `@code/addons/docs/src/blocks/components/Preview.stories.tsx`:
- Around line 54-65: Add an automated assertion-backed regression test for the
ActionBarWrapping story to lock in the wrapped action row behavior: create a
test (either a Storybook play test or a Jest/React Testing Library test) that
mounts the Preview with the same props used in ActionBarWrapping (Preview with
inline, isExpanded, withSource set to sourceStories.JSX.args and the Button
child) and asserts the visual/DOM outcome you care about (e.g., that the action
row wraps and buttons do not overlap or that the Button is visible and
positioned on a new line). Reference the existing story name ActionBarWrapping,
the Preview component, and the Button element to mirror the repro exactly and
include clear assertions for the wrap/overlap condition so future regressions
are caught.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/addons/docs/src/blocks/components/Preview.stories.tsxcode/addons/docs/src/blocks/components/Preview.tsxcode/addons/docs/src/blocks/components/Source.tsxcode/core/src/components/components/ActionBar/ActionBar.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
This was referenced Mar 31, 2026
1 task
This was referenced Apr 2, 2026
1 task
This was referenced Apr 5, 2026
1 task
This was referenced Apr 6, 2026
1 task
This was referenced Apr 8, 2026
Closed
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #23642
This is the proper fix for the PR I had to revert earlier today (#33877).
What I did
Implemented the following design changes:
Checklist for Contributors
Testing
A story was added to showcase the wrapping in preview.
The changes in this PR are covered in the following automated tests:
Manual testing
Documentation
ø
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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam 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
Bug Fixes
Improvements