Agentic Review [1]: Standalone review core quality fixes

[yannbf]

What I did

Standalone core quality fixes for the manager — net-positive regardless of the review integration, no breaking changes. This is PR1 of a 5-PR stack integrating the review feature into core; it telescopes off yann/agentic-review-mcp-integration (#34837) and resolves review findings M5, M7, M10/B3-residual, M11.

• M5 — single source for customQueryParams (the bug fix). A new canonical helper getCustomQueryParams in manager-api/modules/url.ts strips the layout params (full, panel, nav, shortcuts, addonPanel, tabs, path) in exactly one place. Previously root.tsx's getDerivedStateFromProps re-derived customQueryParams stripping only path, so layout params leaked into the params forwarded to the preview iframe — blanking the canvas when navigating out of a review. Both init and navigation now use the same helper, so they can't diverge.
• M7 — dedupe sidebar status-icon logic. Extracted a shared shouldShowChangeStatus(changeStatus, isModifiedFilterActive) predicate so Tree.tsx and getSidebarVisibleStatus no longer each re-implement the change-vs-test icon decision.
• M10 / B3-residual — unify the rollup exclusion mechanism. Introduced one NON_ROLLUP_STATUS_TYPE_IDS set (in the canonical shared/status-store) so change-detection and review are excluded from the test-status rollup by the same mechanism. The single group rollup (getGroupStatus) intentionally still keeps change-detection — documented inline.
• M11 — Card color resolution. Replaced the three in-probe helpers (cast-heavy, silent undefined) with a single typed resolveThemeColor accessor — zero casts, explicit fallbacks.

Stack position

Base: yann/agentic-review-mcp-integration (#34837). This is the bottom of the stack; PR2–PR5 telescope on top. Review bottom-up.

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

Steps for a maintainer to verify the M5 fix (canvas no longer blanks):

1. Run a sandbox: yarn task sandbox --template react-vite/default-ts --start-from auto (or cd code && yarn storybook:ui).
2. Push a review so the review summary appears (via the display-review MCP tool / scripts/display-review.ts), enter review mode, and open one of the curated stories.
3. Navigate back out to a normal story.
4. Expected: the canvas renders the story normally. Before this fix, layout params leaked into the URL's custom params and the canvas blanked.
5. Sanity-check unrelated URL behavior: open a story with ?full=1 / ?nav=0 — fullscreen/nav still toggle, and those params are not echoed into preview-forwarded query params.
6. Sidebar: confirm status icons (change/test/reviewing) on stories, components and groups are unchanged. Card stories render identically in light/dark (including the agentic color).

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update MIGRATION.MD

No docs/MIGRATION changes — internal manager refactors with a single user-visible bug fix.

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it
• Declare whether manual QA will be needed (qa:needed / qa:skip)
• Make sure this PR contains one of the changelog labels (bug)

---

🤖 AI note (Claude Code), not written by Yann — Decisions I made / open questions:

• M5 deviation from the PRD wording. The PRD suggested removing the customQueryParams derivation from getDerivedStateFromProps entirely and relocating it "into the url module." Review genuinely needs customQueryParams to update on URL navigation (it reads customQueryParams.collection), and the url module has no subscription to router/location changes — only the React layer (getDerivedStateFromProps) observes them. So I kept the recomputation but pointed it at the single canonical getCustomQueryParams helper, eliminating the divergence (the actual bug) without inventing a new location-subscription mechanism. The derivation logic is single-sourced; only the trigger stays at the React layer.
• lib/url.ts k !== 'path' filter — kept. With the single-source fix, customQueryParams can no longer contain path, so the filter is now unreachable for that flow. I kept it because buildNavigationUrl is a generic URL builder where path is the positional first arg; guarding against a path key in queryParams is intrinsic to the helper's contract, not review-specific debt. Removing it would couple the helper's correctness to upstream stripping.
• M10 asymmetry is deliberate, not a miss. getGroupStatus (the single group rollup feeding StatusContext + search-result fallback) still excludes only review and keeps change-detection — otherwise a component with only "modified" changes would lose its rollup indicator. Unifying it under the full non-rollup set would be a behavior change, which PR1 explicitly avoids. This is the documented B3-residual.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Separated layout/navigation URL query parameters from custom parameters and reused the same derivation across initialization and derived state.
  • Centralized sidebar change-status icon visibility logic for both leaf and branch nodes.
  • Improved theme color resolution for card content/outline styling.
• Bug Fixes
  • Corrected test-status rollup and change-detection behavior to better align with expected status visibility.
• Tests
  • Added coverage confirming layout-related URL parameters are consumed and omitted from customQueryParams.

[yannbf]

🤖 AI note (Claude Code), not written by Yann: This review-only exclusion is deliberately not unified with NON_ROLLUP_STATUS_TYPE_IDS. getGroupStatus produces the single catch-all rollup that feeds StatusContext and the search-result fallback status; if it also excluded change-detection, a component whose only signal is "modified" would show no rollup indicator at all. The test-slot rollups (which exclude both) are the ones unified. This is the documented B3-residual asymmetry — flagging it so a reviewer doesn't "fix" it into a behavior change.

[coderabbitai]

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: a0bad78d-3a76-444c-9f37-7d7bde7737f0

📥 Commits

Reviewing files that changed from the base of the PR and between 396b6cf and 73dd7c5.

📒 Files selected for processing (9)

• code/core/src/components/components/Card/Card.tsx
• code/core/src/manager-api/modules/url.ts
• code/core/src/manager-api/root.tsx
• code/core/src/manager-api/tests/url.test.js
• code/core/src/manager/components/sidebar/Tree.tsx
• code/core/src/manager/globals/exports.ts
• code/core/src/manager/utils/status.tsx
• code/core/src/shared/status-store/index.ts
• code/core/src/types/modules/status.ts

✅ Files skipped from review due to trivial changes (1)

• code/core/src/manager/globals/exports.ts

🚧 Files skipped from review as they are similar to previous changes (6)

• code/core/src/manager-api/tests/url.test.js
• code/core/src/components/components/Card/Card.tsx
• code/core/src/manager/components/sidebar/Tree.tsx
• code/core/src/manager-api/modules/url.ts
• code/core/src/manager-api/root.tsx
• code/core/src/manager/utils/status.tsx

---

📝 Walkthrough

Walkthrough

The PR introduces NON_AGGREGATED_STATUS_TYPE_IDS to exclude change-detection and review statuses from test-status aggregation, adds shouldShowChangeStatus and isAggregatedTestStatus helpers used across status.tsx and Tree.tsx, centralizes custom URL query param derivation into getCustomQueryParams in url.ts (consumed by root.tsx), and refactors Card theme color lookups into a resolveThemeColor utility.

Changes

Status Aggregation and Change-Detection Icon Refactoring

Layer / File(s)	Summary
NON_AGGREGATED_STATUS_TYPE_IDS constant and public exports code/core/src/shared/status-store/index.ts, code/core/src/types/modules/status.ts, code/core/src/manager/globals/exports.ts	Defines and exports NON_AGGREGATED_STATUS_TYPE_IDS (containing CHANGE_DETECTION_STATUS_TYPE_ID and REVIEW_STATUS_TYPE_ID) in the status store, re-exports it from the types module, and registers it in the globals map.
shouldShowChangeStatus, isAggregatedTestStatus helpers and status utility refactoring code/core/src/manager/utils/status.tsx	Imports NON_AGGREGATED_STATUS_TYPE_IDS, introduces exported shouldShowChangeStatus and local isAggregatedTestStatus predicates, privatizes statusesExcludingReview, and updates getSidebarVisibleStatus, getChangeDetectionStatus, and getGroupDualStatus to use the new predicates.
Tree.tsx consuming shouldShowChangeStatus code/core/src/manager/components/sidebar/Tree.tsx	Imports shouldShowChangeStatus and replaces inline change-icon conditionals for both leaf and branch nodes with calls to the shared helper.

Custom Query Params Centralization

Layer / File(s)	Summary
getCustomQueryParams helper and url.ts refactoring code/core/src/manager-api/modules/url.ts	Defines LAYOUT_QUERY_PARAM_KEYS and exports getCustomQueryParams(location), updates initialUrlSupport to stop spreading queryFromLocation for custom params, and types prevParams as QueryParams.
root.tsx integration and test coverage code/core/src/manager-api/root.tsx, code/core/src/manager-api/tests/url.test.js	Removes the queryFromLocation import from root.tsx, updates getDerivedStateFromProps to call url.getCustomQueryParams for customQueryParams, and adds a test verifying layout params (full, nav, panel, etc.) are excluded from customQueryParams.

Card Theme Color Helper Refactor

Layer / File(s)	Summary
resolveThemeColor utility and color helper refactoring code/core/src/components/components/Card/Card.tsx	Introduces resolveThemeColor with presence checks across theme.color/fgColor/bgColor/borderColor, rewrites getColor, getBorderColor, and getBackgroundColor to use it, with theme.background.content as the background fallback.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

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

[coderabbitai]

🧹 Nitpick comments (1)

code/core/src/manager-api/tests/url.test.js (1)

123-138: ⚡ Quick win

Broaden this regression test to cover the full layout-key boundary.

This case validates only full/nav, but getCustomQueryParams excludes seven layout keys. Including panel, shortcuts, addonPanel, tabs, and path here will catch partial regressions in the canonical filter.

Suggested test hardening

-          new URLSearchParams({ full: '1', nav: '0', collection: '2', tags: 'a11y' }).toString(),
+          new URLSearchParams({
+            full: '1',
+            panel: 'bottom',
+            nav: '0',
+            shortcuts: '1',
+            addonPanel: 'controls',
+            tabs: '1',
+            path: '/story/button--primary',
+            collection: '2',
+            tags: 'a11y',
+          }).toString(),

🤖 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/core/src/manager-api/tests/url.test.js` around lines 123 - 138, The test
case for the initURL function currently only validates that the layout keys full
and nav are filtered out from customQueryParams, but the actual
getCustomQueryParams implementation excludes seven layout keys total. Expand the
URLSearchParams object in the location.search to include the additional layout
keys panel, shortcuts, addonPanel, tabs, and path alongside the existing full
and nav parameters. The customQueryParams expectation should remain unchanged,
verifying that only the genuinely custom params (collection and tags) are
retained while all seven layout keys are properly consumed by the manager and
excluded from the preview iframe params.

🤖 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.

Nitpick comments:
In `@code/core/src/manager-api/tests/url.test.js`:
- Around line 123-138: The test case for the initURL function currently only
validates that the layout keys full and nav are filtered out from
customQueryParams, but the actual getCustomQueryParams implementation excludes
seven layout keys total. Expand the URLSearchParams object in the
location.search to include the additional layout keys panel, shortcuts,
addonPanel, tabs, and path alongside the existing full and nav parameters. The
customQueryParams expectation should remain unchanged, verifying that only the
genuinely custom params (collection and tags) are retained while all seven
layout keys are properly consumed by the manager and excluded from the preview
iframe params.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5e47ed98-8d77-47fd-b9b1-486f8525297b

📥 Commits

Reviewing files that changed from the base of the PR and between 1ee29a4 and b9ebd60.

📒 Files selected for processing (9)

• code/core/src/components/components/Card/Card.tsx
• code/core/src/manager-api/modules/url.ts
• code/core/src/manager-api/root.tsx
• code/core/src/manager-api/tests/url.test.js
• code/core/src/manager/components/sidebar/Tree.tsx
• code/core/src/manager/globals/exports.ts
• code/core/src/manager/utils/status.tsx
• code/core/src/shared/status-store/index.ts
• code/core/src/types/modules/status.ts

[ghengeveld]

The term "rollup" is confusing and does not intuitively convey its meaning to us. Use another term.