codeant-ai-for-open-source[bot] commented on code in PR #38858:
URL: https://github.com/apache/superset/pull/38858#discussion_r2995841452
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -523,30 +523,33 @@ const DashboardBuilder = () => {
({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
{dropIndicatorProps && <div {...dropIndicatorProps} />}
- {!isReport && topLevelTabs && !uiConfig.hideTab && !uiConfig.hideNav
&& (
- <WithPopoverMenu
- shouldFocus={shouldFocusTabs}
- menuItems={[
- <IconButton
- key="collapse-tabs"
- icon={<Icons.FallOutlined iconSize="xl" />}
- label={t('Collapse tab content')}
- onClick={handleDeleteTopLevelTabs}
- />,
- ]}
- editMode={editMode}
- >
- <DashboardComponent
- id={topLevelTabs?.id}
- parentId={DASHBOARD_ROOT_ID}
- depth={DASHBOARD_ROOT_DEPTH + 1}
- index={0}
- renderTabContent={false}
- renderHoverMenu={false}
- onChangeTab={handleChangeTab}
- />
- </WithPopoverMenu>
- )}
+ {!isReport &&
+ topLevelTabs &&
+ !uiConfig.hideTab &&
+ !uiConfig.hideNav && (
+ <WithPopoverMenu
+ shouldFocus={shouldFocusTabs}
+ menuItems={[
+ <IconButton
+ key="collapse-tabs"
+ icon={<Icons.FallOutlined iconSize="xl" />}
+ label={t('Collapse tab content')}
+ onClick={handleDeleteTopLevelTabs}
Review Comment:
**Suggestion:** The "Collapse tab content" button reuses a handler that
computes the direct path using the pre-collapse layout, so after the tabs are
deleted `directPathToChild` points to tab components that no longer exist,
which can leave the dashboard state inconsistent and break logic that relies on
a valid direct path; instead, the click handler should both delete the
top-level tabs and set the direct path directly to the grid node that replaces
them. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Collapsing tabs leaves directPathToChild referencing deleted tab IDs.
- ⚠️ Filter focus/highlight may target non-existent dashboard components.
- ⚠️ Permalink/share logic may use stale component identifiers.
```
</details>
```suggestion
onClick={() => {
dispatch(deleteTopLevelTabs());
dispatch(setDirectPathToChild([DASHBOARD_ROOT_ID,
DASHBOARD_GRID_ID]));
}}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or open a dashboard with top-level tabs (so that the layout root
child is a TABS
container) and enter edit mode in the dashboard builder
(`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx`).
2. In the dashboard header area, click the "Collapse tab content" button
rendered via the
`IconButton` whose `onClick` calls `handleDeleteTopLevelTabs`
(`DashboardBuilder.tsx`
lines ~394–401 and 533–538 in the PR context).
3. That handler dispatches `deleteTopLevelTabs()` and then immediately
computes `firstTab`
using `getDirectPathToTabIndex(getRootLevelTabsComponent(dashboardLayout),
0)` with the
*pre-collapse* `dashboardLayout` snapshot (`DashboardBuilder.tsx` ~394–400,
`getRootLevelTabsComponent` in `DashboardBuilder/utils.ts` lines 25–31, and
`getDirectPathToTabIndex` in `dashboard/util/getDirectPathToTabIndex.ts`
lines 26–34).
4. The `DELETE_TOP_LEVEL_TABS` reducer
(`superset-frontend/src/dashboard/reducers/dashboardLayout.ts` lines 43–71)
removes the
TABS and TAB components and rewires the root to point directly at
`DASHBOARD_GRID_ID`, but
`dashboardState.directPathToChild` is set to `firstTab`, which still
contains the removed
tab component ids (updated in `reducers/dashboardState.ts` lines 332–18 via
`SET_DIRECT_PATH`). Subsequent consumers like `DashboardContainer`
(`DashboardContainer.tsx` lines 120–27 using `findTabIndexByComponentId`)
and filter/hover
highlighting utilities (`nativeFilters/FilterBar/useFilterOutlined.ts` and
`util/getChartAndLabelComponentIdFromPath.ts`) now operate on a path that
references
layout components which no longer exist, leaving directPath-related state
inconsistent
after collapsing the tabs.
```
</details>
<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/DashboardBuilder/DashboardBuilder.tsx
**Line:** 537:537
**Comment:**
*Logic Error: The "Collapse tab content" button reuses a handler that
computes the direct path using the pre-collapse layout, so after the tabs are
deleted `directPathToChild` points to tab components that no longer exist,
which can leave the dashboard state inconsistent and break logic that relies on
a valid direct path; instead, the click handler should both delete the
top-level tabs and set the direct path directly to the grid node that replaces
them.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38858&comment_hash=55993bcce83eef955f1f0610678b9f9f843b929818ea972c9358a17bf2fa83f9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38858&comment_hash=55993bcce83eef955f1f0610678b9f9f843b929818ea972c9358a17bf2fa83f9&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]
