bito-code-review[bot] commented on code in PR #39417:
URL: https://github.com/apache/superset/pull/39417#discussion_r3352265497
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
@@ -175,22 +177,83 @@ export function useDashboardHasTabs() {
const dashboardLayout = useDashboardLayout();
return useMemo(
() =>
- Object.values(dashboardLayout).some(element => element.type ===
TAB_TYPE),
+ dashboardLayout
+ ? Object.values(dashboardLayout).some(
+ element => element.type === TAB_TYPE,
+ )
+ : false,
[dashboardLayout],
);
}
-function useActiveDashboardTabs() {
- return useSelector<RootState, ActiveTabs>(
+function useActiveDashboardTabs(): ActiveTabs {
+ const reduxTabs = useSelector<RootState, ActiveTabs>(
state => state.dashboardState?.activeTabs,
);
+ const dashboardLayout = useDashboardLayout();
+
+ return useMemo(() => {
+ const reduxList = reduxTabs ?? [];
+ const reduxFallback = reduxList.length ? reduxList : EMPTY_ACTIVE_TABS;
+ if (!dashboardLayout) return reduxFallback;
+
+ // Tabbed dashboards always nest the top-level TABS container as the first
+ // child of ROOT. If that invariant doesn't hold (no-tabs layout), no
+ // fallback applies and we use reduxTabs as-is.
+ const root = dashboardLayout[DASHBOARD_ROOT_ID];
+ if (!root?.children?.length) return reduxFallback;
+ const topContainer = dashboardLayout[root.children[0]];
+ if (topContainer?.type !== TABS_TYPE || !topContainer.children?.length) {
+ return reduxFallback;
+ }
+
+ // Walk every TABS container along the active path. For each container,
+ // pick the child Redux marked active; otherwise pick the first child (the
+ // default the live Tabs component would render). This handles:
+ // - empty reduxTabs (hideTab:true, no permalink) → full default path
+ // - reduxTabs missing an outer ancestor (hideTab:true skipped the
+ // top-level Tabs, but a nested Tabs dispatched setActiveTab) → fill
+ // in the missing ancestor so outer-tab scoping is preserved
+ // - fully populated reduxTabs (normal hydration) → same result
+ const reduxSet = new Set(reduxList);
+ const result: ActiveTabs = [];
+ const queue: string[] = [
+ topContainer.children.find(c => reduxSet.has(c)) ??
+ topContainer.children[0],
+ ];
+ while (queue.length > 0) {
+ const tabId = queue.shift()!;
+ result.push(tabId);
+ const tab = dashboardLayout[tabId];
+ if (!tab?.children) continue;
+ for (const childId of tab.children) {
+ const child = dashboardLayout[childId];
+ if (child?.type !== TABS_TYPE || !child.children?.length) continue;
+ queue.push(
+ child.children.find(c => reduxSet.has(c)) ?? child.children[0],
+ );
+ }
+ }
+
+ // Preserve any reduxTabs entries that fell outside the traversed path so
+ // we never silently drop a redux-marked active tab id.
+ const resultSet = new Set(result);
+ for (const id of reduxList) {
+ if (!resultSet.has(id)) result.push(id);
+ }
+ return result;
+ }, [reduxTabs, dashboardLayout]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new hook</b></div>
<div id="fix">
The rewritten `useActiveDashboardTabs` function lacks direct unit tests;
only indirect coverage via `useIsFilterInScope` tests exists. Per [6262], tests
should verify actual business logic directly, not just component rendering.
</div>
</div>
<small><i>Code Review Run #00fb50</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/state.test.ts:
##########
@@ -601,3 +619,528 @@ test('useChartCustomizationConfiguration ignores
undefined items in metadata', (
expect.objectContaining({ id: 'CHART_CUSTOMIZATION-1' }),
);
});
+
+// --- Embedded / hideTab: activeTabs is empty ---
+// When an embedded dashboard uses hideTab:true, the Tabs component never
+// mounts, so setActiveTab never fires and activeTabs stays []. The same
+// empty state occurs transiently on first render of any tabbed dashboard.
+//
+// useActiveDashboardTabs derives the default first tab from the layout when
+// Redux activeTabs is empty, so scope evaluation uses the correct default
+// tab instead of either "no tabs active" (blank filter bar) or "all tabs"
+// (showing out-of-scope filters).
+
+// Helper: build a layout with ROOT_ID → TABS container → TAB children
+function embeddedLayout(extras: Record<string, Record<string, unknown>> = {}) {
+ return {
+ ROOT_ID: {
+ type: 'ROOT',
+ id: 'ROOT_ID',
+ children: ['TABS-1'],
+ },
+ 'TABS-1': {
+ type: 'TABS',
+ id: 'TABS-1',
+ children: ['TAB-Company', 'TAB-Desktop'],
+ },
+ 'TAB-Company': {
+ type: 'TAB',
+ id: 'TAB-Company',
+ children: ['CHART-1', 'CHART-2'],
+ },
+ 'TAB-Desktop': {
+ type: 'TAB',
+ id: 'TAB-Desktop',
+ children: ['CHART-3'],
+ },
+ 'CHART-1': {
+ type: 'CHART',
+ meta: { chartId: 1 },
+ parents: ['ROOT_ID', 'TAB-Company'],
+ },
+ 'CHART-2': {
+ type: 'CHART',
+ meta: { chartId: 2 },
+ parents: ['ROOT_ID', 'TAB-Company'],
+ },
+ 'CHART-3': {
+ type: 'CHART',
+ meta: { chartId: 3 },
+ parents: ['ROOT_ID', 'TAB-Desktop'],
+ },
+ ...extras,
+ };
+}
+
+test('useIsFilterInScope: filter scoped to default tab is in-scope when
activeTabs is empty', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: { present: embeddedLayout() },
+ };
+ return selector(mockState);
+ });
+
+ const filter: Filter = {
+ id: 'filter_default_tab',
+ name: 'Default Tab Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [1, 2],
+ scope: { rootPath: ['TAB-Company'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'column_name' }, datasetId: 1 }],
+ description: 'Filter scoped to default (first) tab',
+ };
+
+ const { result } = renderHook(() => useIsFilterInScope());
+ expect(result.current(filter)).toBe(true);
+});
+
+test('useIsFilterInScope: filter scoped only to non-default tab is
out-of-scope when activeTabs is empty', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: { present: embeddedLayout() },
+ };
+ return selector(mockState);
+ });
+
+ const filter: Filter = {
+ id: 'filter_other_tab',
+ name: 'Other Tab Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [3],
+ scope: { rootPath: ['TAB-Desktop'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'column_name' }, datasetId: 1 }],
+ description: 'Filter scoped only to non-default tab',
+ };
+
+ const { result } = renderHook(() => useIsFilterInScope());
+ expect(result.current(filter)).toBe(false);
+});
+
+test('useIsFilterInScope: filter with rootPath to default tab is in-scope when
activeTabs is empty', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: { present: embeddedLayout() },
+ };
+ return selector(mockState);
+ });
+
+ const filter: Filter = {
+ id: 'filter_rootpath_default',
+ name: 'RootPath Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ scope: { rootPath: ['TAB-Company'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'column_name' }, datasetId: 1 }],
+ description: 'Filter using rootPath to default tab',
+ };
+
+ const { result } = renderHook(() => useIsFilterInScope());
+ expect(result.current(filter)).toBe(true);
+});
+
+test('useSelectFiltersInScope: only default-tab filters are in scope when
activeTabs is empty (embedded hideTab)', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: { present: embeddedLayout() },
+ };
+ return selector(mockState);
+ });
+
+ const filters: Filter[] = [
+ {
+ id: 'filter_company',
+ name: 'Company Tab Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [1, 2],
+ scope: { rootPath: ['TAB-Company'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'survey_rating' }, datasetId: 1 }],
+ description: 'Filter scoped to default tab',
+ },
+ {
+ id: 'filter_desktop_only',
+ name: 'Desktop Only Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [3],
+ scope: { rootPath: ['TAB-Desktop'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'pool_name' }, datasetId: 2 }],
+ description: 'Filter scoped only to non-default tab',
+ },
+ ];
+
+ const { result } = renderHook(() => useSelectFiltersInScope(filters));
+ expect(result.current[0]).toHaveLength(1);
+ expect(result.current[0][0]).toEqual(
+ expect.objectContaining({ id: 'filter_company' }),
+ );
+ expect(result.current[1]).toHaveLength(1);
+ expect(result.current[1][0]).toEqual(
+ expect.objectContaining({ id: 'filter_desktop_only' }),
+ );
+});
+
+test('useSelectFiltersInScope: dividers are always in scope even when
activeTabs is empty', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: { present: embeddedLayout() },
+ };
+ return selector(mockState);
+ });
+
+ const items: (Filter | Divider)[] = [
+ {
+ id: 'divider_embedded',
+ type: NativeFilterType.Divider,
+ title: 'Section',
+ description: 'Divider in embedded mode',
+ },
+ {
+ id: 'filter_default',
+ name: 'Default Tab Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [1],
+ scope: { rootPath: ['TAB-Company'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'col' }, datasetId: 1 }],
+ description: 'Filter in default tab',
+ },
+ ];
+
+ const { result } = renderHook(() => useSelectFiltersInScope(items));
+ expect(result.current[0]).toHaveLength(2);
+ expect(result.current[1]).toHaveLength(0);
+});
+
+test('useSelectFiltersInScope: correctly scopes chartsInScope filters when
activeTabs is populated', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: ['TAB-Company'] },
+ dashboardLayout: { present: embeddedLayout() },
+ };
+ return selector(mockState);
+ });
+
+ const filters: Filter[] = [
+ {
+ id: 'filter_active_tab',
+ name: 'Active Tab Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [1],
+ scope: { rootPath: ['TAB-Company'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'col' }, datasetId: 1 }],
+ description: 'Filter scoped to active tab',
+ },
+ {
+ id: 'filter_inactive_tab',
+ name: 'Inactive Tab Filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [3],
+ scope: { rootPath: ['TAB-Desktop'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'col' }, datasetId: 2 }],
+ description: 'Filter scoped to inactive tab',
+ },
+ ];
+
+ const { result } = renderHook(() => useSelectFiltersInScope(filters));
+ expect(result.current[0]).toHaveLength(1);
+ expect(result.current[0][0]).toEqual(
+ expect.objectContaining({ id: 'filter_active_tab' }),
+ );
+ expect(result.current[1]).toHaveLength(1);
+ expect(result.current[1][0]).toEqual(
+ expect.objectContaining({ id: 'filter_inactive_tab' }),
+ );
+});
+
+// Layout-derived default-tab fallback edge cases. These pin behavior of
+// useActiveDashboardTabs when activeTabs is empty across structural variants
+// of dashboardLayout, so the fallback can't silently regress.
+
+test('useIsFilterInScope: dashboard with no top-level tabs (root child is
GRID_ID) treats filters as in-scope', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: {
+ present: {
+ ROOT_ID: { type: 'ROOT', id: 'ROOT_ID', children: ['GRID_ID'] },
+ GRID_ID: { type: 'GRID', id: 'GRID_ID', children: ['CHART-1'] },
+ 'CHART-1': {
+ type: 'CHART',
+ meta: { chartId: 1 },
+ parents: ['ROOT_ID', 'GRID_ID'],
+ },
+ },
+ },
+ };
+ return selector(mockState);
+ });
+
+ const filter: Filter = {
+ id: 'filter_no_tabs',
+ name: 'No-tabs filter',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [1],
+ scope: { rootPath: ['ROOT_ID'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'col' }, datasetId: 1 }],
+ description: 'Filter on a no-tabs dashboard',
+ };
+
+ const { result } = renderHook(() => useIsFilterInScope());
+ // Chart has no TAB ancestors → tabParents is empty → considered in-scope.
+ expect(result.current(filter)).toBe(true);
+});
+
+test('useIsFilterInScope: missing dashboardLayout falls back without
crashing', () => {
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: { present: undefined },
+ };
+ return selector(mockState);
+ });
+
+ const filter: Filter = {
+ id: 'filter_no_layout',
+ name: 'No layout',
+ filterType: 'filter_select',
+ type: NativeFilterType.NativeFilter,
+ chartsInScope: [1],
+ scope: { rootPath: ['TAB-A'], excluded: [] },
+ controlValues: {},
+ defaultDataMask: {},
+ cascadeParentIds: [],
+ targets: [{ column: { name: 'col' }, datasetId: 1 }],
+ description: 'Filter when layout is missing',
+ };
+
+ // The hook should not throw when layout is missing. Without the
+ // useActiveDashboardTabs guard, indexing dashboardLayout[ROOT_ID] would
+ // crash here.
+ const { result } = renderHook(() => useIsFilterInScope());
+ expect(() => result.current(filter)).not.toThrow();
+});
+
+test('useIsFilterInScope: deeply nested tabs — default path includes inner-tab
default', () => {
+ // ROOT → TABS-1 → [TAB-Outer1, TAB-Outer2]
+ // └─ TAB-Outer1 → TABS-2 → [TAB-Inner1, TAB-Inner2]
+ // Default path should be ['TAB-Outer1', 'TAB-Inner1'].
+ (useSelector as jest.Mock).mockImplementation((selector: Function) => {
+ const mockState = {
+ dashboardState: { activeTabs: [] },
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate mock dashboard state</b></div>
<div id="fix">
Found syntactic duplication (244 tokens across 56 lines) in test file
superset-frontend/src/dashboard/components/nativeFilters/state.test.ts at lines
967-1022 and 1058-1113. The mock dashboard state with nested tabs and charts
layout is duplicated. Consider extracting to a shared test fixture or helper
function to reduce duplication and improve maintainability.
</div>
</div>
<small><i>Code Review Run #00fb50</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
