UI: More iterations on review page#34890
Merged
yannbf merged 20 commits intoMay 28, 2026
Merged
Conversation
yannbf
added
maintenance
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:
📝 WalkthroughWalkthroughStorybook addon refactored from ChangesStorybook Review Addon Navigation and UI Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Comment |
yannbf
commented
May 26, 2026
yannbf
commented
May 26, 2026
yannbf
commented
May 26, 2026
![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: 3
🧹 Nitpick comments (4)
code/addons/review-changes/package.json (1)
12-12: ⚡ Quick winHomepage/repository.directory still pointing at
code/addons/review-changesis consistent with the actual folder
code/addons/review-changesstill exists, and nocode/addons/addon-review/code/addons/reviewdirectory was found—so the currenthomepage/repository.directoryreferences to thereview-changespath don’t contradict the on-disk layout. Remaining consideration: whether renaming the folder to match@storybook/addon-reviewis desired for contributor discoverability. [Optional refactor]🤖 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/package.json` at line 12, The package.json "homepage" and "repository.directory" currently reference the folder name "code/addons/review-changes" which matches the on-disk layout; if you plan to rename the package to `@storybook/addon-review` (or otherwise change the folder to code/addons/addon-review or code/addons/review), update the "homepage" and "repository.directory" fields in package.json to the new folder path to keep links accurate, or alternatively rename the folder to review-changes if you want to keep the current fields; ensure consistency between the package name, folder (code/addons/review-changes), and the "homepage"/"repository.directory" values so contributors can find the source.code/addons/review-changes/src/review-grouping.ts (1)
25-26: ⚡ Quick winUse a
Setfor de-duplication to avoid quadratic growth.At Line 25,
stories.includes(storyId)turns grouping into repeated linear scans. Using aSetpreserves order and keeps membership checks constant-time.Proposed refactor
export const groupStoriesByComponent = (collections: ReviewCollection[]): ComponentGroup[] => { const order: string[] = []; - const storiesByComponent = new Map<string, string[]>(); + const storiesByComponent = new Map<string, Set<string>>(); for (const collection of collections) { for (const storyId of collection.storyIds) { const componentId = storyId.split('--')[0]; let stories = storiesByComponent.get(componentId); if (!stories) { - stories = []; + stories = new Set<string>(); storiesByComponent.set(componentId, stories); order.push(componentId); } - if (!stories.includes(storyId)) { - stories.push(storyId); - } + stories.add(storyId); } } return order.map((componentId) => ({ componentId, - storyIds: storiesByComponent.get(componentId) ?? [], + storyIds: [...(storiesByComponent.get(componentId) ?? new Set<string>())], })); };🤖 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/review-grouping.ts` around lines 25 - 26, Replace the O(n) membership checks using stories.includes(storyId) with an O(1) Set-based membership tracker: introduce a Set (e.g., seenStories or storySet) alongside the existing stories array, use storySet.has(storyId) instead of stories.includes, and when a new id is encountered call storySet.add(storyId) and stories.push(storyId) to preserve order; update any logic that mutates or reads stories to keep the Set and array consistent (references: stories, storyId, stories.includes, stories.push).code/addons/review-changes/src/prototype/ReviewFlowDemo.stories.tsx (2)
229-235: ⚡ Quick winVerify the filtered click lands on the intended story.
The current assertion only proves that some detail page opened. It does not cover the contract described in Lines 222-224 that the filtered result maps back to the full collection order. Assert the pager or story label for the
paddingsstory after the click.🧪 Suggested assertion
const [match] = await canvas.findAllByRole('link', { name: /Review story/ }); await userEvent.click(match); + await expect(await canvas.findByRole('button', { name: '3/5' })).toBeInTheDocument(); await expect(await canvas.findByRole('link', { name: 'Back to review' })).toBeInTheDocument();As per coding guidelines,
code/**/*.tsx: For React components, write Storybook stories with play functions — do NOT write *.test.tsx unit tests; behavior, accessibility, and interaction assertions belong in *.stories.tsx co-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/prototype/ReviewFlowDemo.stories.tsx` around lines 229 - 235, The test clicks the first "Review story" link from a filtered search but only asserts that a detail page opened; update the play assertion in ReviewFlowDemo.stories.tsx so after typing into the input and clicking match you verify the opened story is the filtered "paddings" story (e.g., assert the pager label or story title contains "paddings" or the story index that maps from the filtered list), using the existing variables input, match and the same canvas utilities to find the pager/story label and expect it to match "paddings".
175-181: ⚡ Quick winAssert that “Next story” actually advanced the detail view.
This play still passes if Line 180 is a no-op, because it immediately clicks the always-present back link. Add an assertion after the next click so the story really covers the paging contract.
🧪 Suggested assertion
await expect(await canvas.findByRole('link', { name: 'Back to review' })).toBeInTheDocument(); await userEvent.click(await canvas.findByRole('link', { name: 'Next story' })); + await expect(await canvas.findByRole('button', { name: '2/5' })).toBeInTheDocument(); await userEvent.click(await canvas.findByRole('link', { name: 'Back to review' }));As per coding guidelines,
code/**/*.tsx: For React components, write Storybook stories with play functions — do NOT write *.test.tsx unit tests; behavior, accessibility, and interaction assertions belong in *.stories.tsx co-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/prototype/ReviewFlowDemo.stories.tsx` around lines 175 - 181, The test clicks "Next story" but never asserts that the detail view actually changed; after the await userEvent.click(await canvas.findByRole('link', { name: 'Next story' })) call add an assertion that the detail view advanced (for example: verify the currently shown review link/heading is different from the original firstThumb or that the detail title text changed) before clicking the "Back to review" link; target the same accessible elements used here (firstThumb, the 'Next story' link and the detail heading or review link role) to implement the assertion in ReviewFlowDemo.stories.tsx.
🤖 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/CollectionGrid.tsx`:
- Around line 238-240: The link currently exposes the internal storyId in the
ARIA label; update the GridLink aria-label in CollectionGrid to use the
human-readable resolved title (the existing `{component} / {name}`) instead of
`storyId` so assistive tech and keyboard users see the friendly title; locate
the JSX with GridLink (when `href` is truthy) and replace `aria-label={`Review
story ${storyId}`}` with an aria-label that references the resolved title
variables (e.g., `${component} / ${name}`) or the `content` title string used
elsewhere.
In `@code/addons/review-changes/src/ReviewChangesPage.tsx`:
- Around line 109-116: The click handler currently intercepts all matching
anchors (using anchor = (event.target as HTMLElement | null)?.closest('a')) and
always calls event.preventDefault() and navigate(href...), which breaks anchors
with target="_blank" or download; update the logic in ReviewChangesPage's click
handler to only intercept when the anchor has no explicit target or target is
"_self" and does not have a download attribute (e.g., check
anchor.getAttribute('target') and anchor.hasAttribute('download')), otherwise
allow the default browser behavior (do not call event.preventDefault() or
navigate).
In `@code/addons/review-changes/src/screens/SummaryScreen.tsx`:
- Around line 482-490: The SearchInput lacks an accessible label and the
trailing Button (ariaLabel="Filter stories") is a non-functional, focusable
control; update the SearchInput component usage to include an accessible label
(e.g., add aria-label="Search stories" or aria-labelledby pointing to a visible
label) and remove the dead Button (the Button with ariaLabel="Filter stories"
and FilterIcon) unless you actually implement filtering behavior (e.g., tie it
to a toggleFilterPanel handler); ensure any retained icon-only control has an
accessible name and an implemented onClick handler (refer to SearchInput,
onChange, value, Button, and FilterIcon).
---
Nitpick comments:
In `@code/addons/review-changes/package.json`:
- Line 12: The package.json "homepage" and "repository.directory" currently
reference the folder name "code/addons/review-changes" which matches the on-disk
layout; if you plan to rename the package to `@storybook/addon-review` (or
otherwise change the folder to code/addons/addon-review or code/addons/review),
update the "homepage" and "repository.directory" fields in package.json to the
new folder path to keep links accurate, or alternatively rename the folder to
review-changes if you want to keep the current fields; ensure consistency
between the package name, folder (code/addons/review-changes), and the
"homepage"/"repository.directory" values so contributors can find the source.
In `@code/addons/review-changes/src/prototype/ReviewFlowDemo.stories.tsx`:
- Around line 229-235: The test clicks the first "Review story" link from a
filtered search but only asserts that a detail page opened; update the play
assertion in ReviewFlowDemo.stories.tsx so after typing into the input and
clicking match you verify the opened story is the filtered "paddings" story
(e.g., assert the pager label or story title contains "paddings" or the story
index that maps from the filtered list), using the existing variables input,
match and the same canvas utilities to find the pager/story label and expect it
to match "paddings".
- Around line 175-181: The test clicks "Next story" but never asserts that the
detail view actually changed; after the await userEvent.click(await
canvas.findByRole('link', { name: 'Next story' })) call add an assertion that
the detail view advanced (for example: verify the currently shown review
link/heading is different from the original firstThumb or that the detail title
text changed) before clicking the "Back to review" link; target the same
accessible elements used here (firstThumb, the 'Next story' link and the detail
heading or review link role) to implement the assertion in
ReviewFlowDemo.stories.tsx.
In `@code/addons/review-changes/src/review-grouping.ts`:
- Around line 25-26: Replace the O(n) membership checks using
stories.includes(storyId) with an O(1) Set-based membership tracker: introduce a
Set (e.g., seenStories or storySet) alongside the existing stories array, use
storySet.has(storyId) instead of stories.includes, and when a new id is
encountered call storySet.add(storyId) and stories.push(storyId) to preserve
order; update any logic that mutates or reads stories to keep the Set and array
consistent (references: stories, storyId, stories.includes, stories.push).
🪄 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: d9b7b1e8-20f1-49bd-a7ac-bf3ae489cd3c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
code/.storybook/main.tscode/addons/review-changes/package.jsoncode/addons/review-changes/project.jsoncode/addons/review-changes/src/ReviewChangesPage.tsxcode/addons/review-changes/src/components/CollectionGrid.tsxcode/addons/review-changes/src/constants.tscode/addons/review-changes/src/manager.tsxcode/addons/review-changes/src/prototype/ReviewFlowDemo.stories.tsxcode/addons/review-changes/src/review-grouping.tscode/addons/review-changes/src/review-navigation.tscode/addons/review-changes/src/screens/DetailsScreen.stories.tsxcode/addons/review-changes/src/screens/DetailsScreen.tsxcode/addons/review-changes/src/screens/SummaryScreen.tsxcode/core/src/common/versions.tsscripts/build/entry-configs.ts
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/26440722178 |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 43 KB | 🚨 +43 KB 🚨 |
| Dependency size | 0 B | 654 B | 🚨 +654 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 20.30 MB | 20.30 MB | 🚨 +308 B 🚨 |
| Dependency size | 36.30 MB | 36.11 MB | 🎉 -188 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 | 88.68 MB | 88.49 MB | 🎉 -188 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 87.17 MB | 86.98 MB | 🎉 -188 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.60 MB | 56.41 MB | 🎉 -188 KB 🎉 |
| Bundle Size Analyzer | node | node |
… navigation logic, and enhance URL handling for review states. Add new ReviewPage component and associated stories for improved review flow.
ghengeveld
force-pushed
the
agentic-review-design-iterations
branch
from
684ae94 to
4be9f90
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/package.json (1)
57-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate addon display name to match the rename.
Line 57 still says
"Review Changes"after renaming to@storybook/addon-review, which leaves stale UI/package metadata.Suggested fix
- "displayName": "Review Changes", + "displayName": "Review",🤖 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/package.json` at line 57, Update the package display name metadata: change the "displayName" value currently set to "Review Changes" in package.json to reflect the addon rename to `@storybook/addon-review` (e.g., set displayName to "`@storybook/addon-review`" or another matching human-readable name) so UI and package metadata are consistent with the new package name.
♻️ Duplicate comments (2)
code/addons/review/src/screens/SummaryScreen.tsx (1)
480-488:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to search and remove the inert trailing button.
At Line 480–488, the search field has no explicit label, and the “Filter stories” icon button is focusable but does nothing.
As per coding guidelines, "Encode assumptions with static checks first; if an assumption is expected to always hold, prefer making it impossible via TypeScript types and existing lint rules."Suggested fix
<SearchInput type="search" + aria-label="Find stories" placeholder="Find stories" value={value} onChange={(event) => onChange(event.target.value)} /> - <Button variant="ghost" size="small" padding="small" ariaLabel="Filter stories"> + <SearchIconWrap aria-hidden="true"> <FilterIcon /> - </Button> + </SearchIconWrap>🤖 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/src/screens/SummaryScreen.tsx` around lines 480 - 488, Search input is missing an accessible name and the trailing Filter button is inert; add an explicit accessible name to the SearchInput (e.g., pass aria-label="Find stories" or wrap it with a visible/visually-hidden <label> so the SearchInput component receives an accessibleName prop) and remove the no-op Button that renders <FilterIcon /> (delete the Button element with ariaLabel="Filter stories") so there is no focusable dead control; keep the existing value and onChange handlers on SearchInput (value and onChange) and update any related types/interfaces for SearchInput props if needed to enforce the aria-label requirement statically.code/addons/review/src/ReviewPage.ts (1)
129-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t intercept links intended for new-tab/download behavior.
At Line 129–136, matching links are always hijacked. This still captures anchors with non-
_selftargets ordownload, changing expected browser behavior.Suggested fix
- const anchor = (event.target as HTMLElement | null)?.closest('a'); - const href = anchor?.getAttribute('href'); + const anchor = (event.target as HTMLElement | null)?.closest('a'); + if (!anchor) { + return; + } + const target = anchor.getAttribute('target'); + if ((target && target.toLowerCase() !== '_self') || anchor.hasAttribute('download')) { + return; + } + const href = anchor.getAttribute('href'); if (!href || !href.startsWith(`?path=${REVIEW_CHANGES_URL}`)) { return; }🤖 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/src/ReviewPage.ts` around lines 129 - 136, The click handler currently hijacks all matching anchors; update the logic in the handler that uses anchor and navigate so it ignores anchors that should open in a new tab or trigger a download: after resolving anchor and href check anchor.getAttribute('target') and anchor.hasAttribute('download') (or equivalent), and only call event.preventDefault() and navigate(href, { plain: true }) when there is no target or the target is "_self" and there is no download attribute; otherwise return to preserve native new-tab/download behavior.
🧹 Nitpick comments (1)
code/addons/review/src/constants.ts (1)
15-18: ⚡ Quick winMake
EVENTSconst-asserted to enforce the channel contract at compile time.Use
as constso keys/values stay literal and immutable; this prevents accidental mutation/widening of event names used across repos.Suggested diff
export const EVENTS = { DISPLAY_REVIEW, REQUEST_REVIEW, -}; +} as const;As per coding guidelines, "Encode assumptions with static checks first; if an assumption is expected to always hold, prefer making it impossible via TypeScript types and existing lint rules".
🤖 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/src/constants.ts` around lines 15 - 18, The EVENTS object is not const-asserted, allowing its keys/values to widen or be mutated; update the EVENTS export in constants.ts to use a const assertion (e.g., export const EVENTS = { DISPLAY_REVIEW, REQUEST_REVIEW } as const) so the event names remain literal and immutable and consumers get precise types; ensure any imports/usages expecting string literals still compile and update any dependent types to use typeof EVENTS[keyof typeof EVENTS] if needed.
🤖 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/package.json`:
- Line 45: The package lists "http-proxy-middleware" only in devDependencies but
"./preset" (exported as ./dist/preset.js and implemented in src/preset.ts)
requires it at runtime; move "http-proxy-middleware" from devDependencies to
dependencies in package.json so consumers have it installed, and while editing
package.json also verify and update storybook.displayName (e.g., "Review
Changes") to match the addon name (`@storybook/addon-review`) if the display name
is outdated.
In `@code/addons/review/src/preset.ts`:
- Around line 8-13: The proxy created by createProxyMiddleware (proxyRequest)
lacks timeout guards; update the createProxyMiddleware call to add explicit
timeout and proxyTimeout options (e.g., set reasonable ms values) to protect
BASELINE_TARGET_ORIGIN from stalled upstreams and ensure pathRewrite behavior
with BASELINE_PROXY_PATH remains unchanged, and add an onError handler to the
options to log failures consistently via your logger (or processLogger) so
errors are surfaced instead of leaving requests hanging.
---
Outside diff comments:
In `@code/addons/review/package.json`:
- Line 57: Update the package display name metadata: change the "displayName"
value currently set to "Review Changes" in package.json to reflect the addon
rename to `@storybook/addon-review` (e.g., set displayName to
"`@storybook/addon-review`" or another matching human-readable name) so UI and
package metadata are consistent with the new package name.
---
Duplicate comments:
In `@code/addons/review/src/ReviewPage.ts`:
- Around line 129-136: The click handler currently hijacks all matching anchors;
update the logic in the handler that uses anchor and navigate so it ignores
anchors that should open in a new tab or trigger a download: after resolving
anchor and href check anchor.getAttribute('target') and
anchor.hasAttribute('download') (or equivalent), and only call
event.preventDefault() and navigate(href, { plain: true }) when there is no
target or the target is "_self" and there is no download attribute; otherwise
return to preserve native new-tab/download behavior.
In `@code/addons/review/src/screens/SummaryScreen.tsx`:
- Around line 480-488: Search input is missing an accessible name and the
trailing Filter button is inert; add an explicit accessible name to the
SearchInput (e.g., pass aria-label="Find stories" or wrap it with a
visible/visually-hidden <label> so the SearchInput component receives an
accessibleName prop) and remove the no-op Button that renders <FilterIcon />
(delete the Button element with ariaLabel="Filter stories") so there is no
focusable dead control; keep the existing value and onChange handlers on
SearchInput (value and onChange) and update any related types/interfaces for
SearchInput props if needed to enforce the aria-label requirement statically.
---
Nitpick comments:
In `@code/addons/review/src/constants.ts`:
- Around line 15-18: The EVENTS object is not const-asserted, allowing its
keys/values to widen or be mutated; update the EVENTS export in constants.ts to
use a const assertion (e.g., export const EVENTS = { DISPLAY_REVIEW,
REQUEST_REVIEW } as const) so the event names remain literal and immutable and
consumers get precise types; ensure any imports/usages expecting string literals
still compile and update any dependent types to use typeof EVENTS[keyof typeof
EVENTS] if needed.
🪄 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: 07fee0fd-0a8e-4f3b-84e3-5a27396c84c5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
code/addons/review/README.mdcode/addons/review/build-config.tscode/addons/review/package.jsoncode/addons/review/src/ReviewPage.stories.tsxcode/addons/review/src/ReviewPage.tscode/addons/review/src/constants.tscode/addons/review/src/manager.tsxcode/addons/review/src/preset.tscode/addons/review/src/prototype/ReviewFlowDemo.stories.tsxcode/addons/review/src/review-navigation.tscode/addons/review/src/review-state.tscode/addons/review/src/screens/DetailsScreen.stories.tsxcode/addons/review/src/screens/DetailsScreen.tsxcode/addons/review/src/screens/SummaryScreen.tsxcode/core/src/manager/settings/GuidePage.tsx
✅ Files skipped from review due to trivial changes (3)
- code/core/src/manager/settings/GuidePage.tsx
- code/addons/review/src/review-state.ts
- code/addons/review/README.md
…tate` to `request-review`. Update related references in the codebase for consistency.
ghengeveld
force-pushed
the
agentic-review-design-iterations
branch
from
4be9f90 to
f23fa39
Compare
… and TabButton styled components, and update layout for improved tab functionality and accessibility.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR renames the review addon to @storybook/addon-review and expands it into a manager page for agent-pushed review payloads, with summary/detail navigation and server-channel replay support.
Changes:
- Replaces
addon-review-changeswithaddon-reviewacross build, Storybook config, versions, and workspace metadata. - Adds review summary/detail UI with collections/components tabs, searchable story grids, preview iframes, and navigation helpers.
- Adds server-side review payload caching/enrichment, git branch resolution, tests, and stories/prototypes.
Reviewed changes
Copilot reviewed 34 out of 40 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Updates workspace package entry to @storybook/addon-review. |
scripts/build/entry-configs.ts |
Points build entries at the renamed review addon. |
code/core/src/manager/globals/exports.ts |
Adds a manager error export. |
code/core/src/common/versions.ts |
Updates generated package version key for the renamed addon. |
code/addons/review/vitest.config.ts |
Adds Vitest config for the addon. |
code/addons/review/tsconfig.json |
Adds TypeScript config for the addon. |
code/addons/review/src/screens/SummaryScreen.tsx |
Adds the new review summary UI with tabs, search, groups, and grids. |
code/addons/review/src/screens/SummaryScreen.stories.tsx |
Adds Storybook stories for summary states. |
code/addons/review/src/screens/DetailsScreen.tsx |
Updates detail screen props and toolbar actions. |
code/addons/review/src/screens/DetailsScreen.stories.tsx |
Updates detail stories to use new navigation helpers. |
code/addons/review/src/ReviewPage.ts |
Adds manager-page container, channel wiring, routing, and detail resolution. |
code/addons/review/src/ReviewPage.stories.tsx |
Adds stories for review page channel/router flows. |
code/addons/review/src/review-state.ts |
Defines review payload types. |
code/addons/review/src/review-navigation.ts |
Adds summary/detail URL builders and parsers. |
code/addons/review/src/review-grouping.ts |
Adds component grouping utilities. |
code/addons/review/src/prototype/ReviewFlowDemo.stories.tsx |
Adds an end-to-end prototype story for review navigation. |
code/addons/review/src/preset.ts |
Adds server-channel cache/replay and branch enrichment. |
code/addons/review/src/preset.test.ts |
Tests server-channel behavior. |
code/addons/review/src/node/git-branch.ts |
Adds best-effort git branch detection. |
code/addons/review/src/node/git-branch.test.ts |
Tests git branch detection. |
code/addons/review/src/manager.tsx |
Registers the review page and display-review navigation behavior. |
code/addons/review/src/index.ts |
Exports constants and review payload types. |
code/addons/review/src/constants.ts |
Defines addon IDs, route, session key, and channel events. |
code/addons/review/src/components/CollectionGrid.tsx |
Adds lazy story preview grid with labels and overflow handling. |
code/addons/review/src/components/CollectionGrid.stories.tsx |
Adds grid layout stories. |
code/addons/review/README.md |
Documents addon purpose and channel contract. |
code/addons/review/project.json |
Renames Nx project to addon-review. |
code/addons/review/preset.js |
Adds package preset proxy. |
code/addons/review/package.json |
Renames package and exports preset entry. |
code/addons/review/manager.js |
Adds package manager proxy. |
code/addons/review/build-config.ts |
Adds node preset build entry. |
code/addons/review-changes/src/screens/SummaryScreen.tsx |
Removes old summary screen implementation. |
code/addons/review-changes/src/ReviewChangesPage.tsx |
Removes old review page container. |
code/addons/review-changes/src/ReviewChangesPage.stories.tsx |
Removes old review page stories. |
code/addons/review-changes/src/review-state.ts |
Removes old review state types. |
code/addons/review-changes/src/review-navigation.ts |
Removes old navigation helpers. |
code/addons/review-changes/src/manager.tsx |
Removes old manager registration. |
code/addons/review-changes/src/constants.ts |
Removes old constants. |
code/addons/review-changes/src/components/CollectionGrid.tsx |
Removes old collection grid. |
code/.storybook/main.ts |
Points monorepo Storybook stories/addons at addons/review. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/node/git-branch.test.ts`:
- Around line 3-9: The test currently injects a hoisted vi.fn into vi.mock and
mutates mockExecFile inline; change the module mock to
vi.mock('node:child_process', { spy: true }) and obtain the spy via const {
execFile } = vi.mocked(require('node:child_process'), { shallow: true }) or via
vi.mocked(importedModule). Then replace usages of the hoisted mockExecFile with
the mocked execFile spy and move all mockImplementation(...) calls (previously
at lines 21-25, 37-41, 47-51) into a beforeEach (or nested describe beforeEach)
so each test configures execFile behavior in setup rather than inline in test
bodies.
In `@code/addons/review/src/preset.ts`:
- Around line 48-52: The async handler for channel.on(EVENTS.PUSH_REVIEW) can
commit stale results because multiple enrichWithBranch calls may finish
out-of-order; add a monotonic request token (e.g., incrementing counter or
timestamp stored in a module-scoped variable like lastPushToken) and capture a
local token at the start of the handler, pass it through the async flow, then
only set cached and call channel.emit(EVENTS.DISPLAY_REVIEW, ...) if the local
token equals lastPushToken—use the symbols channel.on(EVENTS.PUSH_REVIEW),
enrichWithBranch, cached, resolveBranch, and EVENTS.DISPLAY_REVIEW to locate and
update the handler.
🪄 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: bf1ebc6a-701d-4294-a24c-1892789fb9ec
📒 Files selected for processing (15)
code/addons/review/README.mdcode/addons/review/build-config.tscode/addons/review/package.jsoncode/addons/review/preset.jscode/addons/review/src/ReviewPage.stories.tsxcode/addons/review/src/ReviewPage.tscode/addons/review/src/constants.tscode/addons/review/src/manager.tsxcode/addons/review/src/node/git-branch.test.tscode/addons/review/src/node/git-branch.tscode/addons/review/src/preset.test.tscode/addons/review/src/preset.tscode/addons/review/src/review-state.tscode/addons/review/src/screens/SummaryScreen.tsxcode/addons/review/vitest.config.ts
✅ Files skipped from review due to trivial changes (2)
- code/addons/review/preset.js
- code/addons/review/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- code/addons/review/src/manager.tsx
- code/addons/review/build-config.ts
- code/addons/review/src/screens/SummaryScreen.tsx
- code/addons/review/src/constants.ts
- code/addons/review/src/ReviewPage.stories.tsx
- code/addons/review/src/ReviewPage.ts
Co-authored-by: ghengeveld <321738+ghengeveld@users.noreply.github.com>
yannbf
merged commit 9c4ac02
into
yann/agentic-review-mcp-integration
138 of 139 checks passed
8 tasks
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
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.
What I did
Design iterations on top of the existing codee
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-34890-sha-dad96b83. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34890-sha-dad96b83 sandboxor in an existing project withnpx storybook@0.0.0-pr-34890-sha-dad96b83 upgrade.More information
0.0.0-pr-34890-sha-dad96b83agentic-review-design-iterationsdad96b831779896306)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=34890Summary by CodeRabbit
New Features
Improvements
Documentation