codeant-ai-for-open-source[bot] commented on code in PR #41536:
URL: https://github.com/apache/superset/pull/41536#discussion_r3493466495
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -454,6 +455,10 @@ test('Correct props to "SliceHeaderControls"', () => {
'data-is-expanded',
'false',
);
+ expect(screen.getByTestId('SliceHeaderControls')).toHaveAttribute(
+ 'data-is-description-expanded',
+ 'false',
+ );
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=58262e3ca08b4dba83026e9cc55170c1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=58262e3ca08b4dba83026e9cc55170c1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41536&comment_hash=cd9d981fec7793ad4e440edfad7c1c95588d24d6681765a40873ddd0e81976c0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41536&comment_hash=cd9d981fec7793ad4e440edfad7c1c95588d24d6681765a40873ddd0e81976c0&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
