bito-code-review[bot] commented on code in PR #37681:
URL: https://github.com/apache/superset/pull/37681#discussion_r2834529938
##########
superset-frontend/packages/superset-ui-core/src/components/Tabs/Tabs.tsx:
##########
@@ -17,81 +17,86 @@
* under the License.
*/
import type { FC } from 'react';
-import { css, styled, useTheme } from '@apache-superset/core/ui';
-
-// eslint-disable-next-line no-restricted-imports
+import { css, styled } from '@apache-superset/core/ui';
import { Tabs as AntdTabs, TabsProps as AntdTabsProps } from 'antd';
import { Icons } from '@superset-ui/core/components/Icons';
import type { SerializedStyles } from '@emotion/react';
export interface TabsProps extends AntdTabsProps {
allowOverflow?: boolean;
+ contentHeight?: string | number;
fullHeight?: boolean;
contentStyle?: SerializedStyles;
+ contentPadding?: SerializedStyles;
}
const StyledTabs = ({
animated = false,
allowOverflow = true,
+ contentHeight = '100%',
fullHeight = false,
tabBarStyle,
contentStyle,
+ contentPadding,
...props
-}: TabsProps) => {
- const theme = useTheme();
- const defaultTabBarStyle = { paddingLeft: theme.sizeUnit * 4 };
- const mergedStyle = { ...defaultTabBarStyle, ...tabBarStyle };
-
- return (
- <AntdTabs
- animated={animated}
- {...props}
- tabBarStyle={mergedStyle}
- css={theme => css`
- overflow: ${allowOverflow ? 'visible' : 'hidden'};
+}: TabsProps) => (
+ <AntdTabs
+ animated={animated}
+ {...props}
+ tabBarStyle={tabBarStyle}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>API Behavior Change</b></div>
<div id="fix">
Previously tabBarStyle was merged with default paddingLeft. Now it's passed
directly, changing the API. Existing tests expect the merge. Either restore
merging or update tests.
</div>
</div>
<small><i>Code Review Run #ab7d90</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/packages/superset-ui-core/src/components/Tabs/Tabs.tsx:
##########
@@ -17,81 +17,86 @@
* under the License.
*/
import type { FC } from 'react';
-import { css, styled, useTheme } from '@apache-superset/core/ui';
-
-// eslint-disable-next-line no-restricted-imports
+import { css, styled } from '@apache-superset/core/ui';
import { Tabs as AntdTabs, TabsProps as AntdTabsProps } from 'antd';
import { Icons } from '@superset-ui/core/components/Icons';
import type { SerializedStyles } from '@emotion/react';
export interface TabsProps extends AntdTabsProps {
allowOverflow?: boolean;
+ contentHeight?: string | number;
fullHeight?: boolean;
contentStyle?: SerializedStyles;
+ contentPadding?: SerializedStyles;
}
const StyledTabs = ({
animated = false,
allowOverflow = true,
+ contentHeight = '100%',
fullHeight = false,
tabBarStyle,
contentStyle,
+ contentPadding,
...props
-}: TabsProps) => {
- const theme = useTheme();
- const defaultTabBarStyle = { paddingLeft: theme.sizeUnit * 4 };
- const mergedStyle = { ...defaultTabBarStyle, ...tabBarStyle };
-
- return (
- <AntdTabs
- animated={animated}
- {...props}
- tabBarStyle={mergedStyle}
- css={theme => css`
- overflow: ${allowOverflow ? 'visible' : 'hidden'};
+}: TabsProps) => (
+ <AntdTabs
+ animated={animated}
+ {...props}
+ tabBarStyle={tabBarStyle}
+ css={theme => css`
+ overflow: ${allowOverflow ? 'visible' : 'hidden'};
+ ${fullHeight && 'height: 100%;'}
+
+ .ant-tabs-content-holder {
+ overflow: ${allowOverflow ? 'visible' : 'auto'};
${fullHeight && 'height: 100%;'}
-
- .ant-tabs-content-holder {
- overflow: ${allowOverflow ? 'visible' : 'auto'};
- ${fullHeight && 'height: 100%;'}
- }
- .ant-tabs-content {
- ${fullHeight && 'height: 100%;'}
- }
- .ant-tabs-tabpane {
- ${fullHeight && 'height: 100%;'}
- ${contentStyle}
- }
- .ant-tabs-tab {
- flex: 1 1 auto;
-
- .short-link-trigger.btn {
- padding: 0 ${theme.sizeUnit}px;
- & > .fa.fa-link {
- top: 0;
- }
+ ${contentHeight &&
+ `height: ${typeof contentHeight === 'number' ? `${contentHeight}px` :
contentHeight};`}
+ ${contentPadding}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CSS Injection Bug</b></div>
<div id="fix">
If contentPadding is undefined (default), 'undefined' gets inserted into the
CSS string, creating invalid styles. Add a fallback to empty string.
</div>
</div>
<small><i>Code Review Run #ab7d90</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/packages/superset-ui-core/src/components/Tabs/Tabs.tsx:
##########
@@ -17,81 +17,86 @@
* under the License.
*/
import type { FC } from 'react';
-import { css, styled, useTheme } from '@apache-superset/core/ui';
-
-// eslint-disable-next-line no-restricted-imports
+import { css, styled } from '@apache-superset/core/ui';
import { Tabs as AntdTabs, TabsProps as AntdTabsProps } from 'antd';
import { Icons } from '@superset-ui/core/components/Icons';
import type { SerializedStyles } from '@emotion/react';
export interface TabsProps extends AntdTabsProps {
allowOverflow?: boolean;
+ contentHeight?: string | number;
fullHeight?: boolean;
contentStyle?: SerializedStyles;
+ contentPadding?: SerializedStyles;
}
const StyledTabs = ({
animated = false,
allowOverflow = true,
+ contentHeight = '100%',
fullHeight = false,
tabBarStyle,
contentStyle,
+ contentPadding,
...props
-}: TabsProps) => {
- const theme = useTheme();
- const defaultTabBarStyle = { paddingLeft: theme.sizeUnit * 4 };
- const mergedStyle = { ...defaultTabBarStyle, ...tabBarStyle };
-
- return (
- <AntdTabs
- animated={animated}
- {...props}
- tabBarStyle={mergedStyle}
- css={theme => css`
- overflow: ${allowOverflow ? 'visible' : 'hidden'};
+}: TabsProps) => (
+ <AntdTabs
+ animated={animated}
+ {...props}
+ tabBarStyle={tabBarStyle}
+ css={theme => css`
+ overflow: ${allowOverflow ? 'visible' : 'hidden'};
+ ${fullHeight && 'height: 100%;'}
+
+ .ant-tabs-content-holder {
+ overflow: ${allowOverflow ? 'visible' : 'auto'};
${fullHeight && 'height: 100%;'}
-
- .ant-tabs-content-holder {
- overflow: ${allowOverflow ? 'visible' : 'auto'};
- ${fullHeight && 'height: 100%;'}
- }
- .ant-tabs-content {
- ${fullHeight && 'height: 100%;'}
- }
- .ant-tabs-tabpane {
- ${fullHeight && 'height: 100%;'}
- ${contentStyle}
- }
- .ant-tabs-tab {
- flex: 1 1 auto;
-
- .short-link-trigger.btn {
- padding: 0 ${theme.sizeUnit}px;
- & > .fa.fa-link {
- top: 0;
- }
+ ${contentHeight &&
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Logic Error in CSS Template</b></div>
<div id="fix">
The condition for contentHeight uses && which treats 0 as falsy, preventing
height: 0px from being set. Since 0 is a valid height value, check for !==
undefined instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
${contentHeight !== undefined &&
````
</div>
</details>
</div>
<small><i>Code Review Run #ab7d90</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/FilterBar/utils.test.ts:
##########
@@ -17,402 +17,771 @@
* under the License.
*/
-import { DataMaskStateWithId, Filter } from '@superset-ui/core';
+import {
+ DataMaskStateWithId,
+ DataRecordValue,
+ Filter,
+ FilterState,
+} from '@superset-ui/core';
import {
checkIsApplyDisabled,
checkIsValidateError,
checkIsMissingRequiredValue,
+ getOnlyExtraFormData,
+ getFiltersToApply,
} from './utils';
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('FilterBar Utils - Validation and Apply Logic', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('checkIsValidateError', () => {
- test('should return true when no filters have validation errors', () => {
- const dataMask: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['CA'],
- },
- extraFormData: {},
- },
- 'filter-2': {
- id: 'filter-2',
- filterState: {
- validateStatus: undefined,
- value: ['NY'],
- },
- extraFormData: {},
- },
- };
-
- expect(checkIsValidateError(dataMask)).toBe(true);
- });
-
- test('should return false when any filter has validation error', () => {
- const dataMask: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: 'error',
- value: undefined,
- },
- extraFormData: {},
- },
- 'filter-2': {
- id: 'filter-2',
- filterState: {
- validateStatus: undefined,
- value: ['NY'],
- },
- extraFormData: {},
- },
- };
-
- expect(checkIsValidateError(dataMask)).toBe(false);
- });
-
- test('should handle empty dataMask', () => {
- const dataMask: DataMaskStateWithId = {};
- expect(checkIsValidateError(dataMask)).toBe(true);
- });
-
- test('should handle filters without filterState', () => {
- const dataMask: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- extraFormData: {},
- },
- };
-
- expect(checkIsValidateError(dataMask)).toBe(true);
- });
+// Factory functions for test data
+function createDataMaskEntry(
+ id: string,
+ overrides: {
+ value?: unknown;
+ validateStatus?: 'error' | undefined;
+ extraFormData?: Record<string, unknown>;
+ } = {},
+) {
+ const { value, validateStatus, extraFormData = {} } = overrides;
+ return {
+ id,
+ filterState: {
+ value,
+ validateStatus,
+ },
+ extraFormData,
+ };
+}
+
+function createFilter(
+ id: string,
+ overrides: {
+ enableEmptyFilter?: boolean;
+ controlValues?: Record<string, unknown>;
+ } = {},
+): Filter {
+ const { enableEmptyFilter, controlValues = {} } = overrides;
+ return {
+ id,
+ controlValues: {
+ ...(enableEmptyFilter !== undefined && { enableEmptyFilter }),
+ ...controlValues,
+ },
+ } as unknown as Filter;
+}
+
+function createExtraFormDataWithFilter(
+ col: string,
+ val: DataRecordValue[],
+ op: 'IN' | 'NOT IN' = 'IN',
+) {
+ return {
+ filters: [{ col, op, val }],
+ };
+}
+
+// getOnlyExtraFormData tests
+test('getOnlyExtraFormData extracts extraFormData from all filters when no
filterIds provided', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': {
+ id: 'filter-1',
+ filterState: { value: ['CA'] },
+ extraFormData: { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ },
+ 'filter-2': {
+ id: 'filter-2',
+ filterState: { value: ['NY'] },
+ extraFormData: { filters: [{ col: 'city', op: 'IN', val: ['NY'] }] },
+ },
+ };
+
+ const result = getOnlyExtraFormData(dataMask);
+
+ expect(result).toEqual({
+ 'filter-1': { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ 'filter-2': { filters: [{ col: 'city', op: 'IN', val: ['NY'] }] },
});
+});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('checkIsMissingRequiredValue', () => {
- test('should return true for required filter with undefined value', () => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: undefined,
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(true);
- });
-
- test('should return true for required filter with null value', () => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: null,
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(true);
- });
-
- test('should return false for required filter with valid value', () => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: ['CA'],
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(false);
- });
-
- test('should return false for non-required filter with undefined value',
() => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: false,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: undefined,
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(false);
- });
-
- test('should return false for filter without controlValues', () => {
- const filter = {
- id: 'test-filter',
- } as Filter;
-
- const filterState = {
- value: undefined,
- };
-
- // checkIsMissingRequiredValue returns undefined when controlValues is
missing
- // undefined is falsy, so we check for truthiness instead of exact false
- expect(checkIsMissingRequiredValue(filter, filterState)).toBeFalsy();
- });
+test('getOnlyExtraFormData only extracts extraFormData for specified
filterIds', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': {
+ id: 'filter-1',
+ filterState: { value: ['CA'] },
+ extraFormData: { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ },
+ 'filter-2': {
+ id: 'filter-2',
+ filterState: { value: ['NY'] },
+ extraFormData: { filters: [{ col: 'city', op: 'IN', val: ['NY'] }] },
+ },
+ 'filter-3': {
+ id: 'filter-3',
+ filterState: { value: ['Product'] },
+ extraFormData: {
+ filters: [{ col: 'product', op: 'IN', val: ['Product'] }],
+ },
+ },
+ };
+
+ const filterIds = new Set(['filter-1', 'filter-3']);
+ const result = getOnlyExtraFormData(dataMask, filterIds);
+
+ expect(result).toEqual({
+ 'filter-1': { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ 'filter-3': { filters: [{ col: 'product', op: 'IN', val: ['Product'] }] },
});
+ expect(result).not.toHaveProperty('filter-2');
+});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('checkIsApplyDisabled', () => {
- test('should return true when filters have validation errors', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: 'error',
- value: undefined,
- },
- extraFormData: {},
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should return false when selected and applied states differ', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['NY'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['NY'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: false,
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(false);
- });
-
- test('should return true when selected and applied states are identical',
() => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: false,
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should return true when required filter is missing value in selected
state', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: undefined,
- },
- extraFormData: {},
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: true, // Required filter
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should handle filter count mismatch', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- 'filter-2': {
- id: 'filter-2',
- filterState: {
- validateStatus: undefined,
- value: ['Product A'],
- },
- extraFormData: {
- filters: [{ col: 'product', op: 'IN', val: ['Product A'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- // Missing filter-2
- };
-
- const filters: Filter[] = [
- { id: 'filter-1', controlValues: {} } as unknown as Filter,
- { id: 'filter-2', controlValues: {} } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should handle validation status recalculation scenario', () => {
- // Scenario: Filter was required and had error, then user selected value
- // The validateStatus should be cleared and Apply should be enabled
-
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined, // Error cleared after selection
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: undefined, // Previously cleared
- },
- extraFormData: {},
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter,
- ];
-
- // Should be enabled because states differ and no validation errors
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(false);
- });
- });
+test('getOnlyExtraFormData returns empty object when filterIds is empty set',
() => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': {
+ id: 'filter-1',
+ filterState: { value: ['CA'] },
+ extraFormData: { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ },
+ };
+
+ const filterIds = new Set<string>();
+ const result = getOnlyExtraFormData(dataMask, filterIds);
+
+ expect(result).toEqual({});
+});
+
+// checkIsValidateError tests
+test('checkIsValidateError returns true when no filters have validation
errors', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': createDataMaskEntry('filter-1', { value: ['CA'] }),
+ 'filter-2': createDataMaskEntry('filter-2', { value: ['NY'] }),
+ };
+
+ expect(checkIsValidateError(dataMask)).toBe(true);
+});
+
+test('checkIsValidateError returns false when any filter has validation
error', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': createDataMaskEntry('filter-1', { validateStatus: 'error' }),
+ 'filter-2': createDataMaskEntry('filter-2', { value: ['NY'] }),
+ };
+
+ expect(checkIsValidateError(dataMask)).toBe(false);
+});
+
+test('checkIsValidateError handles empty dataMask', () => {
+ const dataMask: DataMaskStateWithId = {};
+ expect(checkIsValidateError(dataMask)).toBe(true);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Reduced Test Coverage for Utility Functions</b></div>
<div id="fix">
The diff removes test cases for checkIsValidateError,
checkIsMissingRequiredValue, and checkIsApplyDisabled, which are still present
in utils.ts and used in the apply logic. This reduces test coverage for utility
functions that handle validation and apply behavior. While the refactoring to
use test() over describe() follows AGENTS.md guidelines, removing tests for
active code violates the principle of maintaining behavior assertions as per
BITO.md rule [6262]. Consider keeping these tests to prevent regressions in
filter validation and application logic.
</div>
</div>
<small><i>Code Review Run #ab7d90</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]
