codeant-ai-for-open-source[bot] commented on code in PR #42075: URL: https://github.com/apache/superset/pull/42075#discussion_r3599547663
########## superset-frontend/src/dashboard/util/getDefaultActiveTabs.ts: ########## @@ -0,0 +1,54 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { DashboardLayout } from '../types'; +import { TABS_TYPE } from './componentTypes'; +import { DASHBOARD_ROOT_ID } from './constants'; + +// Default active-tab path a freshly-rendered dashboard shows: ROOT → first +// TABS → first TAB, recursing into nested TABS. Empty for non-tabbed layouts +// and for layouts whose top-level TABS is not the first ROOT child (e.g. TABS +// nested under GRID), matching the live Tabs component's default selection. +export default function getDefaultActiveTabs( + layout: DashboardLayout, +): string[] { + const root = layout?.[DASHBOARD_ROOT_ID]; + const top = root?.children?.length ? layout[root.children[0]] : undefined; + if (top?.type !== TABS_TYPE || !top.children?.length) return []; + + const result: string[] = []; + const visited = new Set<string>(); + const queue: string[] = [top.children[0]]; + while (queue.length) { + const tabId = queue.shift() as string; + if (visited.has(tabId)) continue; + visited.add(tabId); + result.push(tabId); + layout[tabId]?.children?.forEach(childId => { + const child = layout[childId]; + if ( + child?.type === TABS_TYPE && + child.children?.length && + !visited.has(child.children[0]) + ) { + queue.push(child.children[0]); + } + }); Review Comment: **Suggestion:** The default-path walk only inspects direct children of the currently selected tab, so it misses nested `TABS` containers that are reachable through intermediate containers (for example `TAB -> ROW -> COLUMN -> TABS`). In those valid layouts, hydration seeds only the outer tab and omits inner active tabs, so first-render consumers still see incomplete tab context. Traverse descendants under the selected tab (until the next `TABS` container) instead of checking only immediate children. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Nested tabs behind containers lose inner default tab. - ⚠️ Native filters mis-scope charts in nested inner tabs. - ⚠️ Screenshot/permalink payloads miss full nested tab path. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The default-tab seeding helper `getDefaultActiveTabs` in `superset-frontend/src/dashboard/util/getDefaultActiveTabs.ts:27-53` walks the layout starting from `ROOT` -> first `TABS` -> first `TAB`, then iterates: ```ts layout[tabId]?.children?.forEach(childId => { const child = layout[childId]; if ( child?.type === TABS_TYPE && child.children?.length && !visited.has(child.children[0]) ) { queue.push(child.children[0]); } }); ``` This logic only considers immediate children of each selected `TAB` and only recurses when one of those children is itself of type `TABS_TYPE`. 2. Consider a dashboard layout structurally similar to the existing `nestedTabsLayout` fixture in `superset-frontend/src/dashboard/components/nativeFilters/state.test.ts:52-80`, but with an intermediate container chain (e.g. `TAB-Outer1` children `['ROW-1']`, `ROW-1` children `['COLUMN-1']`, and `COLUMN-1` children `['TABS-2']`), all valid types per `componentTypes` and `DashboardLayout` in `superset-frontend/src/dashboard/util/componentTypes.ts` and `superset-frontend/src/dashboard/types.ts`. 3. When `hydrateDashboard` in `superset-frontend/src/dashboard/actions/hydrate.ts:46-63` hydrates such a layout without a permalink `activeTabs`, stored `dashboardState.activeTabs`, or `directPathToChild` (matching the conditions described in the comment at hydrate.ts:52-55), it calls `getDefaultActiveTabs(dashboardLayout.present as DashboardLayout)` to seed `dashboardState.activeTabs`. 4. For the described layout, `getDefaultActiveTabs` will push the outer tab id (e.g. `TAB-Outer1`) into `result`, but `layout['TAB-Outer1'].children` contains only `ROW-1`, not a `TABS_TYPE` node, so the while-loop never queues or visits the inner `TAB` under `TABS-2`. The seeded default path is thus missing the inner tab (e.g. `['TAB-Outer1']` instead of `['TAB-Outer1', 'TAB-Inner1']), and first-render consumers like `useIsFilterInScope` in `superset-frontend/src/dashboard/components/nativeFilters/state.ts:56-97`—which require that all TAB ancestors of a chart appear in `activeTabs`—will treat filters scoped to that inner default tab as out of scope until some later interaction repopulates the full path. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b170918e5be54191a6535c6cc7007fab&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=b170918e5be54191a6535c6cc7007fab&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/util/getDefaultActiveTabs.ts **Line:** 42:51 **Comment:** *Incomplete Implementation: The default-path walk only inspects direct children of the currently selected tab, so it misses nested `TABS` containers that are reachable through intermediate containers (for example `TAB -> ROW -> COLUMN -> TABS`). In those valid layouts, hydration seeds only the outer tab and omits inner active tabs, so first-render consumers still see incomplete tab context. Traverse descendants under the selected tab (until the next `TABS` container) instead of checking only immediate children. 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%2F42075&comment_hash=8b4b46b03d470a9d2212e269480e14205b51c659aa693427657d5e9c21ed8b46&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42075&comment_hash=8b4b46b03d470a9d2212e269480e14205b51c659aa693427657d5e9c21ed8b46&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]
