Review Changes: Implement review changes screen based on latest designs#34874
Merged
Conversation
ghengeveld
added
feature request
ci:normal
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:
📝 WalkthroughWalkthroughThe PR refactors the review-changes addon UI by splitting a monolithic view into a thin container ( ChangesReview Changes UI Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/review-changes/src/components/ReviewChangesScreen.stories.tsx`:
- Around line 184-185: The test assertion currently matches a combined regex
across sibling nodes and is brittle; update the test in
ReviewChangesScreen.stories.tsx to split the check into two separate assertions:
one that finds the “Showing unstaged changes on” text (from DetailsText) and a
second that finds the branch label like `update/button-styles` (from
BranchCode). Use two separate await canvas.findByText(...) calls or equivalent
for those two distinct elements so each element is asserted independently.
In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx`:
- Line 397: The SearchInput elements currently rely solely on placeholder text;
update both instances of the SearchInput component (the one rendered at the
location using the JSX element SearchInput) to add explicit accessible labels by
adding an aria-label prop (e.g., aria-label="Search stories" or a
context-appropriate label) to each SearchInput so screen readers receive a
reliable name while keeping the placeholder unchanged.
- Line 17: DEFAULT_BRANCH is set to a real-looking branch string which can be
misleading; change it to a neutral fallback (e.g., empty string or 'unknown')
and replace any code that blindly uses DEFAULT_BRANCH as a substitute for
missing state.branchName so it uses a non-deceptive value instead. Update the
constant DEFAULT_BRANCH and any usages in the ReviewChangesScreen component
(places where state.branchName is read/fallback is applied) to use
state.branchName ?? '' (or 'unknown') and ensure UI/metadata displays that
neutral fallback rather than 'update/button-styles'.
In `@code/addons/review-changes/src/review-state.ts`:
- Line 8: The comment in review-state.ts is inverted: it currently says fields
beyond `title` + `narrative` + `clusters` are required while the type marks them
optional; update the comment to state that `title`, `narrative`, and `clusters`
are the required minimal payload and that all other fields are optional (or
clarify which specific fields are required/optional to match the TypeScript
types). Make sure the text around `title`, `narrative`, and `clusters`
explicitly matches the optional `?` markers in the corresponding type/interface
so the doc and the types remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc8db16d-2611-49b0-9b85-f4f2976c0a46
📒 Files selected for processing (5)
code/addons/review-changes/src/components/ReviewChangesPage.tsxcode/addons/review-changes/src/components/ReviewChangesScreen.stories.tsxcode/addons/review-changes/src/components/ReviewChangesScreen.tsxcode/addons/review-changes/src/components/ReviewChangesView.stories.tsxcode/addons/review-changes/src/review-state.ts
💤 Files with no reviewable changes (1)
- code/addons/review-changes/src/components/ReviewChangesView.stories.tsx
| <SearchIconWrap> | ||
| <SearchIcon /> | ||
| </SearchIconWrap> | ||
| <SearchInput type="search" placeholder="Find stories" /> |
Contributor
There was a problem hiding this comment.
Add explicit accessible labels to the search inputs.
Both search fields rely on placeholder text; add aria-label for reliable screen-reader naming.
Suggested patch
-<SearchInput type="search" placeholder="Find stories" />
+<SearchInput type="search" placeholder="Find stories" aria-label="Find stories" />Also applies to: 436-436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx` at line
397, The SearchInput elements currently rely solely on placeholder text; update
both instances of the SearchInput component (the one rendered at the location
using the JSX element SearchInput) to add explicit accessible labels by adding
an aria-label prop (e.g., aria-label="Search stories" or a context-appropriate
label) to each SearchInput so screen readers receive a reliable name while
keeping the placeholder unchanged.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 10 KB | 23 KB | 🚨 +12 KB 🚨 |
| Dependency size | 688 B | 688 B | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 186 | 184 | 🎉 -2 🎉 |
| Self size | 79 KB | 79 KB | 🚨 +48 B 🚨 |
| Dependency size | 33.07 MB | 33.36 MB | 🚨 +293 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 20.30 MB | 20.30 MB | 🚨 +914 B 🚨 |
| Dependency size | 36.17 MB | 36.29 MB | 🚨 +120 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 140 KB | 140 KB | 0 B |
| Dependency size | 30.70 MB | 30.75 MB | 🚨 +41 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 190 | 188 | 🎉 -2 🎉 |
| Self size | 15 KB | 15 KB | 🚨 +18 B 🚨 |
| Dependency size | 29.79 MB | 30.08 MB | 🚨 +294 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 536 | 534 | 🎉 -2 🎉 |
| Self size | 662 KB | 662 KB | 0 B |
| Dependency size | 61.20 MB | 61.49 MB | 🚨 +293 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 93 | 93 | 0 |
| Self size | 1.37 MB | 1.37 MB | 0 B |
| Dependency size | 24.75 MB | 24.79 MB | 🚨 +45 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 122 | 122 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 25.82 MB | 25.86 MB | 🚨 +45 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 83 | 83 | 0 |
| Self size | 36 KB | 36 KB | 0 B |
| Dependency size | 22.52 MB | 22.57 MB | 🚨 +45 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 273 | 271 | 🎉 -2 🎉 |
| Self size | 23 KB | 23 KB | 🎉 -12 B 🎉 |
| Dependency size | 45.73 MB | 46.03 MB | 🚨 +293 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 196 | 🎉 -2 🎉 |
| Self size | 16 KB | 16 KB | 0 B |
| Dependency size | 34.33 MB | 34.63 MB | 🚨 +293 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/tanstack-react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 84 | 84 | 0 |
| Self size | 107 KB | 107 KB | 0 B |
| Dependency size | 22.56 MB | 22.60 MB | 🚨 +45 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 203 | 0 |
| Self size | 908 KB | 908 KB | 🚨 +144 B 🚨 |
| Dependency size | 87.59 MB | 87.73 MB | 🚨 +139 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 86.08 MB | 86.22 MB | 🚨 +139 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.08 MB | 🎉 -66 B 🎉 |
| Dependency size | 56.48 MB | 56.60 MB | 🚨 +121 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 166 | 164 | 🎉 -2 🎉 |
| Self size | 18 KB | 18 KB | 0 B |
| Dependency size | 32.08 MB | 32.37 MB | 🚨 +293 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 59 | 59 | 0 |
| Self size | 1.47 MB | 1.47 MB | 🎉 -12 B 🎉 |
| Dependency size | 13.29 MB | 13.30 MB | 🚨 +18 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
yannbf
approved these changes
May 22, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/addons/review-changes/src/components/ReviewChangesScreen.tsx (1)
396-396:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit accessible labels to the search inputs.
Both search fields rely on placeholder text for labeling. Add
aria-labelattributes for reliable screen-reader naming.Suggested fix
-<SearchInput type="search" placeholder="Find stories" /> +<SearchInput type="search" placeholder="Find stories" aria-label="Find stories" />Apply to both search inputs at lines 396 and 435.
Also applies to: 435-435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx` at line 396, Add explicit aria-label attributes to the SearchInput elements in ReviewChangesScreen so they don’t rely solely on placeholders for screen-reader labeling: locate the SearchInput usages in the ReviewChangesScreen component (the instances around the existing placeholder="Find stories" at line ~396 and the other SearchInput at ~435) and add appropriate aria-label values (e.g. aria-label="Find stories" and a matching label for the second search input) to each SearchInput tag.
🧹 Nitpick comments (2)
code/addons/review-changes/src/components/ReviewChangesScreen.stories.tsx (1)
279-281: ⚡ Quick winAdd a
playassertion forRealAtomicChange.This new scenario currently renders without any assertion coverage in story tests.
Suggested patch
export const RealAtomicChange = meta.story({ args: { state: atomicChange }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect( + await canvas.findByText('Round up Button border-radius: 4px → 12px (3× theme multiplier)') + ).toBeInTheDocument(); + await expect(await canvas.findByText('Button — atomic')).toBeInTheDocument(); + }, });As per coding guidelines,
**/*.stories.tsx: For React components, write Storybook stories withplayfunctions — do NOT write*.test.tsxunit tests; behavior, accessibility, and interaction assertions belong in*.stories.tsxco-located with the component.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/review-changes/src/components/ReviewChangesScreen.stories.tsx` around lines 279 - 281, The RealAtomicChange story currently has no play assertions; add a play function to the meta.story call for RealAtomicChange that mounts the story, queries for a specific rendered element representing the atomic change (e.g., change title/text or a role like "listitem" or a button) using within/@storybook/testing-library, and asserts its presence and expected state with expect from `@storybook/jest`; update the RealAtomicChange declaration (the meta.story({ args: { state: atomicChange } })) to include a play async function that performs these queries and assertions to provide interaction/behavior coverage.code/addons/review-changes/src/components/ReviewChangesScreen.tsx (1)
328-328: "Review all" button appears non-functional.The "Review all {sampleCount}" button currently has no
onClickhandler, making it non-functional. This might be intentional placeholder UI.Would you like me to help implement the click handler to navigate to a view showing all stories in the collection, or should this remain as a placeholder for now?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx` at line 328, The "Review all {sampleCount}" Button in ReviewChangesScreen is missing an onClick handler; add an onClick prop that navigates to the "all stories" view for the current collection (e.g., call a navigation helper like useNavigate()/history.push() or an existing prop handler such as onReviewAll), passing the collection id/context so the target screen can show all stories; also handle the disabled state when sampleCount is 0 and ensure the handler is memoized or wrapped (useCallback) if appropriate (update the Button element to <Button size="medium" onClick={...} disabled={sampleCount===0}>Review all {sampleCount}</Button>).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx`:
- Line 396: Add explicit aria-label attributes to the SearchInput elements in
ReviewChangesScreen so they don’t rely solely on placeholders for screen-reader
labeling: locate the SearchInput usages in the ReviewChangesScreen component
(the instances around the existing placeholder="Find stories" at line ~396 and
the other SearchInput at ~435) and add appropriate aria-label values (e.g.
aria-label="Find stories" and a matching label for the second search input) to
each SearchInput tag.
---
Nitpick comments:
In `@code/addons/review-changes/src/components/ReviewChangesScreen.stories.tsx`:
- Around line 279-281: The RealAtomicChange story currently has no play
assertions; add a play function to the meta.story call for RealAtomicChange that
mounts the story, queries for a specific rendered element representing the
atomic change (e.g., change title/text or a role like "listitem" or a button)
using within/@storybook/testing-library, and asserts its presence and expected
state with expect from `@storybook/jest`; update the RealAtomicChange declaration
(the meta.story({ args: { state: atomicChange } })) to include a play async
function that performs these queries and assertions to provide
interaction/behavior coverage.
In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx`:
- Line 328: The "Review all {sampleCount}" Button in ReviewChangesScreen is
missing an onClick handler; add an onClick prop that navigates to the "all
stories" view for the current collection (e.g., call a navigation helper like
useNavigate()/history.push() or an existing prop handler such as onReviewAll),
passing the collection id/context so the target screen can show all stories;
also handle the disabled state when sampleCount is 0 and ensure the handler is
memoized or wrapped (useCallback) if appropriate (update the Button element to
<Button size="medium" onClick={...} disabled={sampleCount===0}>Review all
{sampleCount}</Button>).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cb5b0f3-b8b7-4a5b-94ae-520f83a5daad
📒 Files selected for processing (5)
code/addons/review-changes/src/components/ReviewChangesPage.tsxcode/addons/review-changes/src/components/ReviewChangesScreen.stories.tsxcode/addons/review-changes/src/components/ReviewChangesScreen.tsxcode/addons/review-changes/src/index.tscode/addons/review-changes/src/review-state.ts
yannbf
added
ci:normal
Contributor
|
Failed to publish canary version of this pull request, triggered by @yannbf. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/26283399989 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/addons/review-changes/src/components/ReviewChangesScreen.tsx (1)
226-234:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNo-op “Mark cluster reviewed” action should not be interactive.
This button advertises a review action but currently only stops propagation and does nothing, which is misleading in keyboard/screen-reader flows. Either wire a real handler or render it disabled until the action exists.
Proposed minimal fix (disable until implemented)
<Button variant="ghost" size="small" padding="small" - ariaLabel="Mark cluster reviewed" - onClick={(event) => event.stopPropagation()} + ariaLabel="Mark cluster reviewed (coming soon)" + disabled > <CheckIcon /> </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx` around lines 226 - 234, The "Mark cluster reviewed" Button currently only calls event.stopPropagation() onClick which is misleading and interactive without functionality; update the Button (rendering the <Button ... ariaLabel="Mark cluster reviewed" ...> with <CheckIcon />) to be non-interactive until implemented by either removing the onClick prop and adding disabled={true} (or the component's equivalent) and ensuring it has appropriate aria-disabled or aria-label adjustments, or wire a real handler (e.g., markClusterReviewed or similar) that performs the action; make the change where the Button with CheckIcon and onClick={(event) => event.stopPropagation()} is defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@code/addons/review-changes/src/components/ReviewChangesDetailsScreen.stories.tsx`:
- Around line 27-29: The story's play step is asserting a stale string ("Core
Button - primary/solid variant") that this screen doesn't render; update the
assertion in ReviewChangesDetailsScreen.stories.tsx to check for the actual
rendered elements instead (use canvas.findByText('Checklist') to assert
collectionTitle or use canvas.findByRole/findByTitle to assert the iframe is
present) by replacing the canvas.findByText call or removing the incorrect
expectation so the play step validates the current UI (reference:
canvas.findByText and the collectionTitle/iframe rendered by the story).
In `@code/addons/review-changes/src/components/ReviewChangesDetailsScreen.tsx`:
- Around line 139-145: The "Mark as viewed" Button (in
ReviewChangesDetailsScreen, the Button JSX using onMarkViewed) should be
disabled when no handler is provided: update the Button to pass
disabled={!onMarkViewed} (and add aria-disabled={true} when disabled for
accessibility) so the control is not actionable if onMarkViewed is undefined;
ensure the onClick remains onMarkViewed and that visual styling still
communicates the disabled state.
In `@code/addons/review-changes/src/components/ReviewCollectionGrid.tsx`:
- Around line 165-170: The current JSX renders a focusable Button even when
reviewAllHref is falsy (Button with asChild={!!reviewAllHref}), producing a
clickable control with no effect; update the ReviewCollectionGrid render so that
when reviewAllHref exists you keep the Button wrapping an anchor (<a
href={reviewAllHref}>Review all {storyIds.length}</a>), and when it does not
exist render either a non-interactive span/text or a Button with disabled={true}
(do not rely on asChild in the falsy branch) so the control is not focusable and
cannot be activated; adjust the conditional around reviewAllHref and the usage
of asChild to ensure the interactive Button is only used for the anchor case and
the non-interactive UI is used otherwise.
In `@code/addons/review-changes/src/review-navigation.ts`:
- Around line 17-27: The function parseReviewChangesDetailsLocation currently
converts params.get('collection') and params.get('story') to numbers directly,
but URLSearchParams.get returns null for missing keys so Number(null) becomes 0;
update parseReviewChangesDetailsLocation to first verify the query keys exist
and their values are non-null/non-empty (e.g. using params.has('collection') &&
params.get('collection') != null) before converting, or check the raw get result
and return null early if either is null/empty; then convert to integer (e.g.
parseInt with radix or Number) and keep the existing Number.isInteger and >=0
checks on collectionIndex and storyIndex to decide whether to return null or the
location.
---
Outside diff comments:
In `@code/addons/review-changes/src/components/ReviewChangesScreen.tsx`:
- Around line 226-234: The "Mark cluster reviewed" Button currently only calls
event.stopPropagation() onClick which is misleading and interactive without
functionality; update the Button (rendering the <Button ... ariaLabel="Mark
cluster reviewed" ...> with <CheckIcon />) to be non-interactive until
implemented by either removing the onClick prop and adding disabled={true} (or
the component's equivalent) and ensuring it has appropriate aria-disabled or
aria-label adjustments, or wire a real handler (e.g., markClusterReviewed or
similar) that performs the action; make the change where the Button with
CheckIcon and onClick={(event) => event.stopPropagation()} is defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ec466d4-5f7b-4962-8ef3-422bab6cb9d6
📒 Files selected for processing (8)
code/addons/review-changes/src/components/ReviewChangesDetailsScreen.stories.tsxcode/addons/review-changes/src/components/ReviewChangesDetailsScreen.tsxcode/addons/review-changes/src/components/ReviewChangesPage.tsxcode/addons/review-changes/src/components/ReviewChangesScreen.stories.tsxcode/addons/review-changes/src/components/ReviewChangesScreen.tsxcode/addons/review-changes/src/components/ReviewCollectionGrid.stories.tsxcode/addons/review-changes/src/components/ReviewCollectionGrid.tsxcode/addons/review-changes/src/review-navigation.ts
Comment on lines
+27
to
+29
| await expect( | ||
| await canvas.findByText('Core Button - primary/solid variant') | ||
| ).toBeInTheDocument(); |
Contributor
There was a problem hiding this comment.
Default story asserts text that this component does not render.
The play step checks for Core Button - primary/solid variant, but this screen renders collectionTitle (Checklist) and an iframe. This assertion is likely stale and can make the story fail.
🔧 Proposed fix
- await expect(
- await canvas.findByText('Core Button - primary/solid variant')
- ).toBeInTheDocument();
+ await expect(await canvas.findByText('Checklist')).toBeInTheDocument();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect( | |
| await canvas.findByText('Core Button - primary/solid variant') | |
| ).toBeInTheDocument(); | |
| await expect(await canvas.findByText('Checklist')).toBeInTheDocument(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@code/addons/review-changes/src/components/ReviewChangesDetailsScreen.stories.tsx`
around lines 27 - 29, The story's play step is asserting a stale string ("Core
Button - primary/solid variant") that this screen doesn't render; update the
assertion in ReviewChangesDetailsScreen.stories.tsx to check for the actual
rendered elements instead (use canvas.findByText('Checklist') to assert
collectionTitle or use canvas.findByRole/findByTitle to assert the iframe is
present) by replacing the canvas.findByText call or removing the incorrect
expectation so the play step validates the current UI (reference:
canvas.findByText and the collectionTitle/iframe rendered by the story).
Comment on lines
+139
to
+145
| <Button | ||
| variant="ghost" | ||
| size="small" | ||
| padding="small" | ||
| ariaLabel="Mark as viewed" | ||
| onClick={onMarkViewed} | ||
| > |
Contributor
There was a problem hiding this comment.
Disable “Mark as viewed” when no handler is provided.
With onMarkViewed undefined, the button still appears actionable but does nothing. Disable it when the callback is absent to avoid false affordance.
🔧 Proposed fix
<Button
variant="ghost"
size="small"
padding="small"
ariaLabel="Mark as viewed"
onClick={onMarkViewed}
+ disabled={!onMarkViewed}
>
<CheckIcon />
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| variant="ghost" | |
| size="small" | |
| padding="small" | |
| ariaLabel="Mark as viewed" | |
| onClick={onMarkViewed} | |
| > | |
| <Button | |
| variant="ghost" | |
| size="small" | |
| padding="small" | |
| ariaLabel="Mark as viewed" | |
| onClick={onMarkViewed} | |
| disabled={!onMarkViewed} | |
| > | |
| <CheckIcon /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/addons/review-changes/src/components/ReviewChangesDetailsScreen.tsx`
around lines 139 - 145, The "Mark as viewed" Button (in
ReviewChangesDetailsScreen, the Button JSX using onMarkViewed) should be
disabled when no handler is provided: update the Button to pass
disabled={!onMarkViewed} (and add aria-disabled={true} when disabled for
accessibility) so the control is not actionable if onMarkViewed is undefined;
ensure the onClick remains onMarkViewed and that visual styling still
communicates the disabled state.
Comment on lines
+165
to
+170
| <Button size="medium" asChild={!!reviewAllHref}> | ||
| {reviewAllHref ? ( | ||
| <a href={reviewAllHref}>Review all {storyIds.length}</a> | ||
| ) : ( | ||
| <span>Review all {storyIds.length}</span> | ||
| )} |
Contributor
There was a problem hiding this comment.
Avoid rendering a clickable control when no review-all destination exists.
In the no-reviewAllHref path this becomes a focusable button with no effect. Render non-interactive text (or a disabled button) instead.
Proposed fix
- <Button size="medium" asChild={!!reviewAllHref}>
- {reviewAllHref ? (
- <a href={reviewAllHref}>Review all {storyIds.length}</a>
- ) : (
- <span>Review all {storyIds.length}</span>
- )}
- </Button>
+ {reviewAllHref ? (
+ <Button size="medium" asChild>
+ <a href={reviewAllHref}>Review all {storyIds.length}</a>
+ </Button>
+ ) : (
+ <span aria-disabled="true">Review all {storyIds.length}</span>
+ )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/addons/review-changes/src/components/ReviewCollectionGrid.tsx` around
lines 165 - 170, The current JSX renders a focusable Button even when
reviewAllHref is falsy (Button with asChild={!!reviewAllHref}), producing a
clickable control with no effect; update the ReviewCollectionGrid render so that
when reviewAllHref exists you keep the Button wrapping an anchor (<a
href={reviewAllHref}>Review all {storyIds.length}</a>), and when it does not
exist render either a non-interactive span/text or a Button with disabled={true}
(do not rely on asChild in the falsy branch) so the control is not focusable and
cannot be activated; adjust the conditional around reviewAllHref and the usage
of asChild to ensure the interactive Button is only used for the anchor case and
the non-interactive UI is used otherwise.
…nused files - Updated import path for ReviewChangesPage component. - Deleted unused ReviewChangesDetailsScreen, ReviewChangesPage, ReviewChangesScreen, and ReviewCollectionGrid components along with their associated stories. - Streamlined the codebase by removing obsolete files to enhance maintainability.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
code/addons/review-changes/src/components/CollectionGrid.tsx (1)
165-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid rendering a focusable button when no reviewAllHref exists.
When
reviewAllHrefis falsy,asChild={false}causes the Button component to render a<button>element wrapping the<span>, creating a focusable control with no action.Proposed fix
- <Button size="medium" asChild={!!reviewAllHref}> - {reviewAllHref ? ( - <a href={reviewAllHref}>Review all {storyIds.length}</a> - ) : ( - <span>Review all {storyIds.length}</span> - )} - </Button> + {reviewAllHref ? ( + <Button size="medium" asChild> + <a href={reviewAllHref}>Review all {storyIds.length}</a> + </Button> + ) : ( + <span>Review all {storyIds.length}</span> + )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/review-changes/src/components/CollectionGrid.tsx` around lines 165 - 171, The Button currently always renders (with asChild toggled) causing a focusable <button> to wrap a non-interactive <span> when reviewAllHref is falsy; update the CollectionGrid rendering so that if reviewAllHref exists you render <Button asChild> with the <a href={reviewAllHref}>Review all {storyIds.length}</a>, but if reviewAllHref is falsy you do NOT render Button at all and instead render a plain non-interactive <span>Review all {storyIds.length}</span>; locate the conditional around reviewAllHref and Button in the component and make the branch return either the Button-asChild + anchor or the bare span to avoid creating a focusable control with no action.code/addons/review-changes/src/screens/DetailsScreen.tsx (1)
139-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDisable "Mark as viewed" when no handler is provided.
The button remains visually actionable when
onMarkViewedis undefined, creating false affordance. Addingdisabled={!onMarkViewed}prevents user confusion.🔧 Proposed fix
<Button variant="ghost" size="small" padding="small" ariaLabel="Mark as viewed" onClick={onMarkViewed} + disabled={!onMarkViewed} > <CheckIcon /> </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/review-changes/src/screens/DetailsScreen.tsx` around lines 139 - 147, The "Mark as viewed" Button in DetailsScreen remains actionable even when no handler is provided; update the Button element (the one with ariaLabel="Mark as viewed" and onClick={onMarkViewed}) to pass a disabled prop when onMarkViewed is falsy (e.g., disabled={!onMarkViewed}) so the control is visually and functionally disabled when no handler exists; ensure you only change the Button props and keep the existing onClick assignment untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@code/addons/review-changes/src/components/CollectionGrid.tsx`:
- Around line 165-171: The Button currently always renders (with asChild
toggled) causing a focusable <button> to wrap a non-interactive <span> when
reviewAllHref is falsy; update the CollectionGrid rendering so that if
reviewAllHref exists you render <Button asChild> with the <a
href={reviewAllHref}>Review all {storyIds.length}</a>, but if reviewAllHref is
falsy you do NOT render Button at all and instead render a plain non-interactive
<span>Review all {storyIds.length}</span>; locate the conditional around
reviewAllHref and Button in the component and make the branch return either the
Button-asChild + anchor or the bare span to avoid creating a focusable control
with no action.
In `@code/addons/review-changes/src/screens/DetailsScreen.tsx`:
- Around line 139-147: The "Mark as viewed" Button in DetailsScreen remains
actionable even when no handler is provided; update the Button element (the one
with ariaLabel="Mark as viewed" and onClick={onMarkViewed}) to pass a disabled
prop when onMarkViewed is falsy (e.g., disabled={!onMarkViewed}) so the control
is visually and functionally disabled when no handler exists; ensure you only
change the Button props and keep the existing onClick assignment untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf24ffeb-b74e-4ea9-95a0-3bdab367f6e1
📒 Files selected for processing (8)
code/addons/review-changes/src/ReviewChangesPage.tsxcode/addons/review-changes/src/components/CollectionGrid.stories.tsxcode/addons/review-changes/src/components/CollectionGrid.tsxcode/addons/review-changes/src/manager.tsxcode/addons/review-changes/src/screens/DetailsScreen.stories.tsxcode/addons/review-changes/src/screens/DetailsScreen.tsxcode/addons/review-changes/src/screens/SummaryScreen.stories.tsxcode/addons/review-changes/src/screens/SummaryScreen.tsx
✅ Files skipped from review due to trivial changes (1)
- code/addons/review-changes/src/manager.tsx
Contributor
|
Failed to publish canary version of this pull request, triggered by @yannbf. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/26288907357 |
1 similar comment
Contributor
|
Failed to publish canary version of this pull request, triggered by @yannbf. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/26288907357 |
This reverts commit e3fcd45.
…handling and add stories - Added location search handling to ReviewChangesPage to parse collection and story parameters. - Introduced ReviewChangesPage stories to demonstrate various states and interactions. - Updated CollectionGrid and SummaryScreen components for improved naming consistency and functionality.
yannbf
merged commit 67d85aa
into
yann/agentic-review-mcp-integration
10 of 12 checks passed
This was referenced May 22, 2026
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 #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
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 pull request has been released as version
0.0.0-pr-34874-sha-b3e479f2. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34874-sha-b3e479f2 sandboxor in an existing project withnpx storybook@0.0.0-pr-34874-sha-b3e479f2 upgrade.More information
0.0.0-pr-34874-sha-b3e479f2addon-review-uib3e479f21779460130)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34874Summary by CodeRabbit
Release Notes
New Features
Refactor