fix(dashboard): sync chart description menu state with expanded_slices#41536
fix(dashboard): sync chart description menu state with expanded_slices#41536durgaprasadml wants to merge 2 commits into
Conversation
dosubot
Bot
added
the
dashboard:component
Code Review Agent Run #0fa68aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| expect(screen.getByTestId('SliceHeaderControls')).toHaveAttribute( | ||
| 'data-is-description-expanded', | ||
| 'false', | ||
| ); |
There was a problem hiding this comment.
Suggestion: This assertion only validates the default collapsed state, so it does not cover the regression scenario where a chart description is already expanded from expanded_slices. Add a test case with the expanded state set to true and verify the propagated description-expanded prop is true; otherwise this test can pass while the original menu-state desynchronization bug still exists. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Expanded descriptions menu label can regress unnoticed.
- ⚠️ Dashboard description toggle reliability depends on manual testing.
- ⚠️ Automated tests miss persisted expanded_slices behavior.
- ⚠️ Future refactors may desync description state and menu.Steps of Reproduction ✅
1. Persist a dashboard with an expanded chart description so that its JSON metadata
contains `"expanded_slices": { <slice_id>: true }` (server-side write path in
`superset/daos/dashboard.py:343`), then load the dashboard in the UI so hydration uses
this value.
2. Observe that front-end hydration maps `metadata.expanded_slices` into Redux
`dashboardState.expandedSlices` in
`superset-frontend/src/dashboard/actions/hydrate.ts:377`, and that the Chart component
derives `isExpanded` from this field in `components/gridComponents/Chart/Chart.tsx:211`
and passes it to `SliceHeader` (verified via code analysis).
3. In `superset-frontend/src/dashboard/components/SliceHeader/index.tsx:354-356`,
`SliceHeader` passes `isExpanded` to `SliceHeaderControls` both as `isExpanded` and
`isDescriptionExpanded`, and `SliceHeaderControls` uses `props.isDescriptionExpanded` to
choose between “Hide chart description” and “Show chart description” in
`SliceHeaderControls/index.tsx:445-449`; if a regression reintroduces the earlier bug
(e.g., `isDescriptionExpanded` is incorrectly wired and remains false when the description
DOM is expanded), the dropdown label will be desynchronized from the actual description
state.
4. Run the current test suite: `SliceHeaderControls` tests cover only the “Show chart
description” path (`SliceHeaderControls.test.tsx:43-51` with `isDescriptionExpanded`
false), and the new `SliceHeader` test only asserts `data-is-description-expanded="false"`
(`SliceHeader.test.tsx:427-486`, lines 458-461). Even with the wiring bug reintroduced,
these tests still pass because no test renders a case where `expandedSlices` (and thus the
description) starts true and asserts `isDescriptionExpanded` is true, confirming the
suggestion that the added assertion validates only the collapsed state and misses the
expanded-state regression scenario.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx
**Line:** 458:461
**Comment:**
*Incomplete Implementation: This assertion only validates the default collapsed state, so it does not cover the regression scenario where a chart description is already expanded from `expanded_slices`. Add a test case with the expanded state set to true and verify the propagated description-expanded prop is true; otherwise this test can pass while the original menu-state desynchronization bug still exists.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The current test only verifies the collapsed state ( Here is a concise implementation for the test file: test('Correct props to "SliceHeaderControls" when expanded', () => {
render(<SliceHeader {...defaultProps} isExpanded={true} />);
expect(screen.getByTestId('SliceHeaderControls')).toHaveAttribute(
'data-is-description-expanded',
'true',
);
});There are no other comments on this PR to address. Would you like me to check for any other potential test coverage gaps in this file? superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41536 +/- ##
==========================================
- Coverage 64.57% 64.57% -0.01%
==========================================
Files 2664 2664
Lines 146369 146385 +16
Branches 33844 33847 +3
==========================================
+ Hits 94521 94530 +9
- Misses 50132 50139 +7
Partials 1716 1716
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks @durgaprasadml for digging into #40373. I don't think this changes behavior though... |
durgaprasadml
force-pushed
the
fix-dashboard-expanded-slices-40373
branch
from
19d7a50 to
a2ed94f
Compare
Code Review Agent Run #946e73Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #525fe9Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
This PR fixes a regression introduced in Superset 6.1.0 where dashboard chart descriptions no longer remain expanded despite the chart being present in expanded_slices.
The root cause was that SliceHeaderControls uses the isDescriptionExpanded prop to determine whether to render “Show chart description” or “Hide chart description”, but SliceHeader was only passing isExpanded.
As a result, the menu state became desynchronized from the actual description state, causing the dropdown to always display “Show chart description” and making the toggle behavior appear broken.
BEFORE
{
"expanded_slices": {
"123": true
}
}
Show chart description
AFTER
TESTING
Added regression coverage to verify:
Fixes #40373