Skip to content

Agentic Review [4a]: Dead-code cleanup, filters-ref dedup, and review-mode ADR - #35222

Merged
yannbf merged 2 commits into
yann/agentic-review-mcp-integrationfrom
review-core/04a-cleanups-and-dedup
Jun 22, 2026
Merged

yannbf merged 2 commits into
yann/agentic-review-mcp-integrationfrom
review-core/04a-cleanups-and-dedup

Conversation

@yannbf

@yannbf yannbf commented Jun 18, 2026 •

Copy link
Copy Markdown
Member

What I did

The original PR4 (mount internalization + internal state module + server-cache move) is large and behavior-bearing. Research showed it's safest to split it. This is PR4a — the low-risk, behavior-preserving cleanups carved out so they land independently green and shrink the surface the heavy follow-up has to move. PR4a of the stack, telescoping on PR3 (#35220).

  • Phantom collapseNavOnOpen removed. It was emitted on every DISPLAY_REVIEW payload and asserted in preset.test.ts, but no consumer reads it. Removed from the emit and the test assertions.
  • Dead code deleted: the unused AIBadge component; the unused storyPreviewUrl export; and isReviewStoryRoute/isReviewModeRoute (test-only, no production consumer) plus their tests.
  • useReviewFiltersRef extracted. The identical "snapshot the current sidebar filters into a ref" block was copy-pasted in ReviewProvider, useReviewNavigationInterceptor, and useReviewShortcuts. Now one hook.

Deferred to PR4b (the heavy slice)

Mount internalization (render the review layer + toolbar header directly in core's Layout/Preview/Toolbar, delete persistentRender + TOOLBAR_HEADER, revert render: FC|null → FC), the internal manager-api review state module (retire the standalone store + module-global summaryOverlaySuppressed), the server-cache move into core-server, and the M8 prev/next dedup. Those are cross-package, API-breaking changes that deserve their own review.

Stack position

Base: review-core/03-route-and-rename (PR3, #35220).

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

For a maintainer:

  1. Run a sandbox or cd code && yarn storybook:ui, push a review, and confirm the full flow is unchanged: summary ↔ story navigation, prev/next, keyboard shortcuts (←/→/↑/↓/esc), the collection picker, dismiss, and the "New" badge.
  2. This PR removes only dead code and de-duplicates a hook, so there should be no observable behavior change.

Documentation

  • Add or update documentation reflecting your changes
  • MIGRATION — N/A (unreleased)

Checklist for Maintainers

  • ci:normal
  • qa:needed
  • One changelog label: maintenance

🤖 AI note (Claude Code), not written by Yann:

Why PR4 was split. A research pass found the full PR4 moves ~22 files from addons/review/ into core and changes ~12-15 core files (manager render tree, the public addon-API type surface, and core-server preset composition) at once — too much to land green in one reviewable step. I split it: PR4a (this PR) is pure cleanup with zero cross-package surface change; PR4b will do the mount internalization + state module + server move + API deletion. Each slice telescopes, and PR4a strictly shrinks what PR4b must move.

Decision: isReviewStoryRoute/isReviewModeRoute had tests but no production consumer (the route→mode coupling they implied is exactly what review mode must not do — review mode is interaction-driven, never inferred from the route). I deleted them with their tests rather than keep test-only code alive.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed review state payload handling to preserve navigation behavior settings.
  • Refactor

    • Improved review mode filter state management for better consistency across navigation and shortcuts.
    • Removed unused routing helper functions.
    • Removed AI Badge component.
  • Chores

    • Cleaned up import statements and removed unnecessary code.

@yannbf yannbf added maintenance User-facing maintenance tasks ci:normal Run our default set of CI jobs (choose this for most PRs). qa:needed Pull Requests that will need manual QA prior to release. labels Jun 18, 2026
@yannbf yannbf changed the title Agentic Review: dead-code cleanup, filters-ref dedup, and review-mode ADR (PR4a) Core: Dead-code cleanup, filters-ref dedup, and review-mode ADR (PR4a) Jun 18, 2026
@yannbf
yannbf force-pushed the review-core/03-route-and-rename branch from ab9a58e to bf62f59 Compare June 19, 2026 09:13
@yannbf
yannbf force-pushed the review-core/04a-cleanups-and-dedup branch from 79a3ec6 to fa5a073 Compare June 19, 2026 09:13
@yannbf
yannbf force-pushed the review-core/03-route-and-rename branch from bf62f59 to 4946ed5 Compare June 19, 2026 09:23
@yannbf
yannbf force-pushed the review-core/04a-cleanups-and-dedup branch from fa5a073 to 54aace4 Compare June 19, 2026 09:23
@yannbf yannbf changed the title Core: Dead-code cleanup, filters-ref dedup, and review-mode ADR (PR4a) PR4a: Dead-code cleanup, filters-ref dedup, and review-mode ADR Jun 19, 2026
@yannbf
yannbf marked this pull request as ready for review June 19, 2026 13:27
@coderabbitai

coderabbitai Bot commented Jun 19, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3ccee200-276d-4443-a399-52a3d185a8d9

📥 Commits

Reviewing files that changed from the base of the PR and between 659695f and a117062.

📒 Files selected for processing (1)
  • code/core/src/manager/components/sidebar/review-mode.ts
✅ Files skipped from review due to trivial changes (1)
  • code/core/src/manager/components/sidebar/review-mode.ts

📝 Walkthrough

Walkthrough

The PR extracts a new useReviewFiltersRef hook that centralizes sidebar filter state management, replacing duplicated useRef/useStorybookState wiring in ReviewProvider, useReviewNavigationInterceptor, and useReviewShortcuts. It also removes the exported storyPreviewUrl, isReviewStoryRoute, and isReviewModeRoute helpers, drops the forced collapseNavOnOpen: true override in the PUSH_REVIEW handler, and deletes the AIBadge styled component.

Changes

Review mode hook extraction and cleanup

Layer / File(s) Summary
New useReviewFiltersRef hook
code/addons/review/src/useReviewFiltersRef.ts
Introduces a hook that reads included/excluded status and tag filters from useStorybookState on every render and returns a stable MutableRefObject<ReviewModeFilters>, so callbacks can access up-to-date filter state without rebinding.
Hook consumers migrated to useReviewFiltersRef
code/addons/review/src/ReviewProvider.tsx, code/addons/review/src/useReviewNavigationInterceptor.ts, code/addons/review/src/useReviewShortcuts.ts
All three files replace their local useRef<ReviewModeFilters> + useStorybookState filter construction with a call to useReviewFiltersRef(); redundant imports (useStorybookState, ReviewModeFilters, useRef) are removed and dependency arrays are updated to include filtersRef.
Remove routing helpers, collapseNavOnOpen override, and comment cleanup
code/addons/review/src/review-navigation.ts, code/addons/review/src/review-navigation.test.ts, code/addons/review/src/preset.ts, code/addons/review/src/preset.test.ts, code/core/src/manager/components/sidebar/review-mode.ts
Deletes storyPreviewUrl, isReviewStoryRoute, and isReviewModeRoute from review-navigation.ts; isReviewReturnSearch now uses isReviewSummaryPath and path.startsWith directly. The PUSH_REVIEW handler emits DISPLAY_REVIEW with the cached state directly instead of spreading collapseNavOnOpen: true. Tests are updated to match the removed helpers and changed payload. Import order and a comment in review-mode.ts are cleaned up.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • storybookjs/storybook#35110: Directly related — touches the same review-navigation.ts routing helpers (isReviewStoryRoute/isReviewModeRoute) and filter state wiring in ReviewProvider and review navigation hooks that this PR removes or refactors.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

@yannbf yannbf changed the title PR4a: Dead-code cleanup, filters-ref dedup, and review-mode ADR Agentic Review [4a]: Dead-code cleanup, filters-ref dedup, and review-mode ADR Jun 19, 2026
@yannbf
yannbf force-pushed the review-core/03-route-and-rename branch from 91e8783 to 037a9fb Compare June 19, 2026 14:50
@yannbf
yannbf force-pushed the review-core/04a-cleanups-and-dedup branch from 54aace4 to a38a7d3 Compare June 19, 2026 14:50
@yannbf
yannbf force-pushed the review-core/03-route-and-rename branch from 037a9fb to 0ac0627 Compare June 19, 2026 15:14
@yannbf
yannbf force-pushed the review-core/04a-cleanups-and-dedup branch from a38a7d3 to 231d109 Compare June 19, 2026 15:14
@storybook-app-bot

storybook-app-bot Bot commented Jun 19, 2026 •

Copy link
Copy Markdown
Contributor

Package Benchmarks

Commit: a117062, ran on 19 June 2026 at 19:51:06 UTC

No significant changes detected, all good. 👏

Base automatically changed from review-core/03-route-and-rename to yann/agentic-review-mcp-integration June 19, 2026 16:02
@@ -0,0 +1,51 @@
# 1. Interaction-driven review mode

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

PR Looks good, we should remove this though

…-mode ADR

Lower-risk, behavior-preserving cleanups split out of the larger PR4
mount-internalization work so they land independently green:

- Remove the phantom `collapseNavOnOpen` field (emitted on DISPLAY_REVIEW and
  asserted in tests, read by nobody).
- Delete dead code: the unused `AIBadge` component and the unused
  `storyPreviewUrl`/`isReviewStoryRoute`/`isReviewModeRoute` exports (the latter
  two were test-only with no production consumer) plus their tests.
- Extract `useReviewFiltersRef`, collapsing the identical sidebar-filters
  snapshot that was triplicated across the provider, nav interceptor, and
  shortcuts hook.
- Add the `docs/adr/0001-interaction-driven-review-mode.md` ADR that the code
  already referenced but which did not exist.

The mount internalization, internal manager-api state module, server-cache move,
and M8 prev/next dedup remain for the follow-up (PR4b).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yannbf
yannbf force-pushed the review-core/04a-cleanups-and-dedup branch from 231d109 to 659695f Compare June 19, 2026 16:37
Remove docs/adr/0001-interaction-driven-review-mode.md and the comment
pointer to it; the interaction-driven rationale stays inline on the
review-mode session key.
@yannbf
yannbf merged commit 21f1e74 into yann/agentic-review-mcp-integration Jun 22, 2026
145 of 147 checks passed
@yannbf
yannbf deleted the review-core/04a-cleanups-and-dedup branch June 22, 2026 08:15
@Sidnioulz Sidnioulz added qa:success Pull Requests that were successfully QA'ed by the release team. and removed qa:needed Pull Requests that will need manual QA prior to release. labels Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:normal Run our default set of CI jobs (choose this for most PRs). maintenance User-facing maintenance tasks qa:success Pull Requests that were successfully QA'ed by the release team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants