Sidebar: Show same status icon at story and group level#34702
Conversation
Story leaves now render the change-detection icon alongside the test status icon, matching the dual-slot layout already used at the component/group level. Branch-level test indicators stop using the small colored "dot" sprite and use the same icon shape returned by getStatus(theme, status).icon as the leaf — giving uniform iconography across both depths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors the Storybook sidebar tree UI to separate change-detection and test status indicators into independent computed values and rendered buttons, replacing a single combined status icon. Leaf-node status computation conditions when change icons appear, derives text color from the overall most-critical status, and renders dual status buttons when both exist. Branch-node test icon logic is simplified to direct icon lookup, and related tests are updated to expect multiple status buttons per hierarchy level. ChangesSidebar Tree Status Icon Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
…-detection-uniform-icons
|
@copilot resolve the merge conflicts in this pull request |
…s' into valentin/change-detection-uniform-icons # Conflicts: # code/core/src/manager/components/sidebar/Tree.tsx Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (1)
808-840: 💤 Low valueStory name could be clearer.
The story name
WithCTAInActiveis ambiguous—it could be read as "CTA In Active [state]" or "CTA Inactive". Consider renaming to something clearer likeWithCTAReviewingStateorWithCTAActiveNoExplicitFiltersto match the assertions (checking foraria-checked="true"and "Reviewing new stories").The implementation and assertions look correct.
🤖 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/components/sidebar/Sidebar.stories.tsx` around lines 808 - 840, Rename the ambiguous story export WithCTAInActive to a clearer name that matches its behavior (e.g., WithCTAReviewingState or WithCTAActiveNoExplicitFilters) and update all occurrences: change the export constant name (export const WithCTAInActive) and any references to it in the file (including beforeEach/play test labels if they reference the symbol) so the new identifier is used consistently; ensure the Story type annotation and args/parameters/play/beforeEach blocks remain intact and unchanged except for the identifier rename.
🤖 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/components/sidebar/Sidebar.stories.tsx`:
- Around line 808-840: Rename the ambiguous story export WithCTAInActive to a
clearer name that matches its behavior (e.g., WithCTAReviewingState or
WithCTAActiveNoExplicitFilters) and update all occurrences: change the export
constant name (export const WithCTAInActive) and any references to it in the
file (including beforeEach/play test labels if they reference the symbol) so the
new identifier is used consistently; ensure the Story type annotation and
args/parameters/play/beforeEach blocks remain intact and unchanged except for
the identifier rename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2223e5a9-58e1-4bda-99eb-b2c778e7c9bf
📒 Files selected for processing (3)
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsx
|
@copilot resolve the merge conflicts in this pull request |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
…tion-uniform-icons # Conflicts: # code/core/src/manager/components/sidebar/Sidebar.stories.tsx Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
Resolved in 40f0b29. The conflict in |
The sidebar now shows the same status icon at both story and group/component level, so a single focused-test success matches at the story plus its component and group ancestors (3 elements). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #
What I did
Follow-up to #34701 — unifies the change-detection and test status iconography between the story leaf and the group/component branch in the sidebar.
Story leaf
affectedalways hidden,modifiedgated on the modified status filter,newalways shown).getStatus(theme, testStatus).iconinstead of being folded into a single `storyStatus` slot.Group / component branch
Cleanup
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Run: `cd code && yarn vitest run --config vitest.config.storybook.ts core/src/manager/components/sidebar/{Tree,Sidebar,ReviewChangesButton}.stories.tsx` — 51/51 pass.
Manual testing
Caution
This section is mandatory for all contributions.
Documentation
Checklist for Maintainers
🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the `@storybookjs/core` team here.
Summary by CodeRabbit
Refactor
Tests