bito-code-review[bot] commented on code in PR #44678: URL: https://github.com/apache/superset/pull/44678#discussion_r4109358004
########## superset-frontend/src/dashboard/components/dnd/dragDroppableConfig.test.ts: ########## @@ -0,0 +1,268 @@ +/** + * 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 { DragSourceMonitor, DropTargetMonitor } from 'react-dnd'; +import { CHART_TYPE } from '../../util/componentTypes'; +import type { + DragDroppableComponent, + DragDroppableProps, + DropResult, +} from './dragDroppableConfig'; + +jest.mock('./handleHover', () => ({ + __esModule: true, + default: jest.fn(), +})); +jest.mock('./handleDrop', () => ({ + __esModule: true, + default: jest.fn(), +})); + +// imports follow the jest.mock calls to mirror their hoisted order +// eslint-disable-next-line import/first +import { dragConfig, dropConfig } from './dragDroppableConfig'; +// eslint-disable-next-line import/first +import mockedHandleHover from './handleHover'; +// eslint-disable-next-line import/first +import mockedHandleDrop from './handleDrop'; + +const { canDrag, beginDrag } = dragConfig[1]; +const { canDrop, hover, drop } = dropConfig[1]; + +function makeProps( + overrides: Partial<DragDroppableProps> = {}, +): DragDroppableProps { + return { + component: { + id: 'chart-1', + type: CHART_TYPE, + children: [], + meta: {}, + }, + index: 0, + depth: 1, + disableDragDrop: false, + ...overrides, + } as DragDroppableProps; +} + +function makeComponent( + overrides: Partial<DragDroppableComponent> = {}, +): DragDroppableComponent { + return { + mounted: true, + props: makeProps(), + setState: jest.fn(), + ...overrides, + }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('canDrag allows dragging when disableDragDrop is false', () => { + expect(canDrag(makeProps({ disableDragDrop: false }))).toBe(true); +}); + +test('canDrag forbids dragging when disableDragDrop is true', () => { + expect(canDrag(makeProps({ disableDragDrop: true }))).toBe(false); +}); + +test('beginDrag captures the parent id and type when a parent component is present', () => { + const parentComponent = { + id: 'row-1', + type: 'ROW', Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Magic string 'ROW' instead of ROW_TYPE</b></div> <div id="fix"> This fixture hardcodes the component-type string 'ROW' (asserted again as parentType at line 102) while importing CHART_TYPE from the same `../../util/componentTypes` module, which already exports `ROW_TYPE = 'ROW'`. Using the exported constant keeps the fixture consistent with the production `ComponentType` union and survives a future rename. Suggest `import { CHART_TYPE, ROW_TYPE }` and using `ROW_TYPE` in both places. </div> </div> <small><i>Code Review Run #36d094</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/dnd/dragDroppableConfig.test.ts: ########## @@ -0,0 +1,268 @@ +/** + * 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 { DragSourceMonitor, DropTargetMonitor } from 'react-dnd'; +import { CHART_TYPE } from '../../util/componentTypes'; +import type { + DragDroppableComponent, + DragDroppableProps, + DropResult, +} from './dragDroppableConfig'; + +jest.mock('./handleHover', () => ({ + __esModule: true, + default: jest.fn(), +})); +jest.mock('./handleDrop', () => ({ + __esModule: true, + default: jest.fn(), +})); + +// imports follow the jest.mock calls to mirror their hoisted order +// eslint-disable-next-line import/first +import { dragConfig, dropConfig } from './dragDroppableConfig'; +// eslint-disable-next-line import/first +import mockedHandleHover from './handleHover'; +// eslint-disable-next-line import/first +import mockedHandleDrop from './handleDrop'; + +const { canDrag, beginDrag } = dragConfig[1]; +const { canDrop, hover, drop } = dropConfig[1]; + +function makeProps( + overrides: Partial<DragDroppableProps> = {}, +): DragDroppableProps { + return { + component: { + id: 'chart-1', + type: CHART_TYPE, + children: [], + meta: {}, + }, + index: 0, + depth: 1, + disableDragDrop: false, + ...overrides, + } as DragDroppableProps; +} + +function makeComponent( + overrides: Partial<DragDroppableComponent> = {}, +): DragDroppableComponent { + return { + mounted: true, + props: makeProps(), + setState: jest.fn(), + ...overrides, + }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('canDrag allows dragging when disableDragDrop is false', () => { + expect(canDrag(makeProps({ disableDragDrop: false }))).toBe(true); +}); + +test('canDrag forbids dragging when disableDragDrop is true', () => { + expect(canDrag(makeProps({ disableDragDrop: true }))).toBe(false); +}); + +test('beginDrag captures the parent id and type when a parent component is present', () => { + const parentComponent = { + id: 'row-1', + type: 'ROW', + children: [], + meta: {}, + } as DragDroppableProps['parentComponent']; + const props = makeProps({ parentComponent, index: 2 }); + + expect(beginDrag(props)).toEqual({ + type: CHART_TYPE, + id: 'chart-1', + meta: {}, + index: 2, + parentId: 'row-1', + parentType: 'ROW', + }); +}); + +test('beginDrag leaves parent fields undefined when there is no parent component', () => { + const props = makeProps({ parentComponent: undefined }); + + expect(beginDrag(props)).toEqual({ + type: CHART_TYPE, + id: 'chart-1', + meta: {}, + index: 0, + parentId: undefined, + parentType: undefined, + }); +}); + +test('canDrop allows dropping when disableDragDrop is false', () => { + expect(canDrop(makeProps({ disableDragDrop: false }))).toBe(true); +}); + +test('canDrop forbids dropping when disableDragDrop is true', () => { + expect(canDrop(makeProps({ disableDragDrop: true }))).toBe(false); +}); + +test('hover delegates to handleHover when the drop target component is mounted', () => { + const props = makeProps(); + const monitor = {} as DropTargetMonitor; + const component = makeComponent({ mounted: true }); + + hover(props, monitor, component); + + expect(mockedHandleHover).toHaveBeenCalledWith(props, monitor, component); +}); + +test('hover does not call handleHover when the drop target component has unmounted', () => { + const props = makeProps(); + const monitor = {} as DropTargetMonitor; + const component = makeComponent({ mounted: false }); + + hover(props, monitor, component); + + expect(mockedHandleHover).not.toHaveBeenCalled(); +}); + +test('drop delegates to handleDrop when no nested target already produced a result', () => { + const props = makeProps(); + const component = makeComponent({ mounted: true }); + const expected: DropResult = { + source: { id: 'a', type: CHART_TYPE, index: 0 }, + dragging: { id: 'b', type: CHART_TYPE, meta: {} }, + }; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicated DropResult test literal</b></div> <div id="fix"> The `DropResult` literal `{ source: { id: 'a', ... }, dragging: { id: 'b', ... } }` is repeated three times in this diff (lines 150-153, 169-172, 184-188). The file already establishes factory helpers `makeProps` and `makeComponent` for shared test data; a `makeDropResult(overrides?)` helper would keep the three `drop` tests in lockstep if the `DropResult` shape changes and match the file's own convention. </div> </div> <small><i>Code Review Run #36d094</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/dnd/dragDroppableConfig.test.ts: ########## @@ -0,0 +1,268 @@ +/** + * 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 { DragSourceMonitor, DropTargetMonitor } from 'react-dnd'; +import { CHART_TYPE } from '../../util/componentTypes'; +import type { + DragDroppableComponent, + DragDroppableProps, + DropResult, +} from './dragDroppableConfig'; + +jest.mock('./handleHover', () => ({ + __esModule: true, + default: jest.fn(), +})); +jest.mock('./handleDrop', () => ({ + __esModule: true, + default: jest.fn(), +})); + +// imports follow the jest.mock calls to mirror their hoisted order +// eslint-disable-next-line import/first +import { dragConfig, dropConfig } from './dragDroppableConfig'; +// eslint-disable-next-line import/first +import mockedHandleHover from './handleHover'; +// eslint-disable-next-line import/first +import mockedHandleDrop from './handleDrop'; + +const { canDrag, beginDrag } = dragConfig[1]; +const { canDrop, hover, drop } = dropConfig[1]; + +function makeProps( + overrides: Partial<DragDroppableProps> = {}, +): DragDroppableProps { + return { + component: { + id: 'chart-1', + type: CHART_TYPE, + children: [], + meta: {}, + }, + index: 0, + depth: 1, + disableDragDrop: false, + ...overrides, + } as DragDroppableProps; +} + +function makeComponent( + overrides: Partial<DragDroppableComponent> = {}, +): DragDroppableComponent { + return { + mounted: true, + props: makeProps(), + setState: jest.fn(), + ...overrides, + }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('canDrag allows dragging when disableDragDrop is false', () => { + expect(canDrag(makeProps({ disableDragDrop: false }))).toBe(true); +}); + +test('canDrag forbids dragging when disableDragDrop is true', () => { + expect(canDrag(makeProps({ disableDragDrop: true }))).toBe(false); +}); + +test('beginDrag captures the parent id and type when a parent component is present', () => { + const parentComponent = { + id: 'row-1', + type: 'ROW', + children: [], + meta: {}, + } as DragDroppableProps['parentComponent']; + const props = makeProps({ parentComponent, index: 2 }); + + expect(beginDrag(props)).toEqual({ + type: CHART_TYPE, + id: 'chart-1', + meta: {}, + index: 2, + parentId: 'row-1', + parentType: 'ROW', + }); +}); + +test('beginDrag leaves parent fields undefined when there is no parent component', () => { + const props = makeProps({ parentComponent: undefined }); + + expect(beginDrag(props)).toEqual({ + type: CHART_TYPE, + id: 'chart-1', + meta: {}, + index: 0, + parentId: undefined, + parentType: undefined, + }); +}); + +test('canDrop allows dropping when disableDragDrop is false', () => { + expect(canDrop(makeProps({ disableDragDrop: false }))).toBe(true); +}); + +test('canDrop forbids dropping when disableDragDrop is true', () => { + expect(canDrop(makeProps({ disableDragDrop: true }))).toBe(false); +}); + +test('hover delegates to handleHover when the drop target component is mounted', () => { + const props = makeProps(); + const monitor = {} as DropTargetMonitor; + const component = makeComponent({ mounted: true }); + + hover(props, monitor, component); + + expect(mockedHandleHover).toHaveBeenCalledWith(props, monitor, component); +}); + +test('hover does not call handleHover when the drop target component has unmounted', () => { + const props = makeProps(); + const monitor = {} as DropTargetMonitor; + const component = makeComponent({ mounted: false }); + + hover(props, monitor, component); + + expect(mockedHandleHover).not.toHaveBeenCalled(); +}); + +test('drop delegates to handleDrop when no nested target already produced a result', () => { + const props = makeProps(); + const component = makeComponent({ mounted: true }); + const expected: DropResult = { + source: { id: 'a', type: CHART_TYPE, index: 0 }, + dragging: { id: 'b', type: CHART_TYPE, meta: {} }, + }; + (mockedHandleDrop as jest.Mock).mockReturnValueOnce(expected); + const monitor = { + getDropResult: jest.fn(() => null), + } as unknown as DropTargetMonitor; + + const result = drop(props, monitor, component); + + expect(mockedHandleDrop).toHaveBeenCalledWith(props, monitor, component); + expect(result).toBe(expected); +}); + +test('drop delegates to handleDrop when a nested result has no destination', () => { + const props = makeProps(); + const component = makeComponent({ mounted: true }); + const monitor = { + getDropResult: jest.fn(() => ({ + source: { id: 'a', type: CHART_TYPE, index: 0 }, + dragging: { id: 'b', type: CHART_TYPE, meta: {} }, + })), + } as unknown as DropTargetMonitor; + + drop(props, monitor, component); + + expect(mockedHandleDrop).toHaveBeenCalledWith(props, monitor, component); +}); + +test('drop returns undefined and skips handleDrop when a nested target already produced a destination', () => { + const props = makeProps(); + const component = makeComponent({ mounted: true }); + const monitor = { + getDropResult: jest.fn(() => ({ + source: { id: 'a', type: CHART_TYPE, index: 0 }, + dragging: { id: 'b', type: CHART_TYPE, meta: {} }, + destination: { id: 'c', type: CHART_TYPE, index: 1 }, + })), + } as unknown as DropTargetMonitor; + + const result = drop(props, monitor, component); + + expect(result).toBeUndefined(); + expect(mockedHandleDrop).not.toHaveBeenCalled(); +}); + +test('drop returns undefined and skips handleDrop when the component has unmounted', () => { + const props = makeProps(); + const component = makeComponent({ mounted: false }); + const monitor = { + getDropResult: jest.fn(() => null), + } as unknown as DropTargetMonitor; + + const result = drop(props, monitor, component); + + expect(result).toBeUndefined(); + expect(mockedHandleDrop).not.toHaveBeenCalled(); +}); + +test('dragStateToProps reports isDragging and the dragged component identity from the monitor', () => { + const dragStateToProps = dragConfig[2]; + const connect = { + dragSource: jest.fn(() => 'drag-source-ref'), + dragPreview: jest.fn(() => 'drag-preview-ref'), + }; + const monitor = { + isDragging: jest.fn(() => true), + getItem: jest.fn(() => ({ id: 'chart-1', type: CHART_TYPE })), + } as unknown as DragSourceMonitor; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicated drag mock literals</b></div> <div id="fix"> The `connect` and `monitor` mock literals for `dragStateToProps` are duplicated verbatim between this test and the one at lines 252-259. The file already centralizes mocks via `makeProps`/`makeComponent`; a shared factory would keep the two drag-state tests from diverging when `DragStateProps` grows a field. </div> </div> <small><i>Code Review Run #36d094</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/dnd/dragDroppableConfig.test.ts: ########## @@ -0,0 +1,268 @@ +/** + * 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 { DragSourceMonitor, DropTargetMonitor } from 'react-dnd'; +import { CHART_TYPE } from '../../util/componentTypes'; +import type { + DragDroppableComponent, + DragDroppableProps, + DropResult, +} from './dragDroppableConfig'; + +jest.mock('./handleHover', () => ({ + __esModule: true, + default: jest.fn(), +})); +jest.mock('./handleDrop', () => ({ + __esModule: true, + default: jest.fn(), +})); + +// imports follow the jest.mock calls to mirror their hoisted order +// eslint-disable-next-line import/first +import { dragConfig, dropConfig } from './dragDroppableConfig'; +// eslint-disable-next-line import/first +import mockedHandleHover from './handleHover'; +// eslint-disable-next-line import/first +import mockedHandleDrop from './handleDrop'; + +const { canDrag, beginDrag } = dragConfig[1]; +const { canDrop, hover, drop } = dropConfig[1]; + +function makeProps( + overrides: Partial<DragDroppableProps> = {}, +): DragDroppableProps { + return { + component: { + id: 'chart-1', + type: CHART_TYPE, + children: [], + meta: {}, + }, + index: 0, + depth: 1, + disableDragDrop: false, + ...overrides, + } as DragDroppableProps; +} + +function makeComponent( + overrides: Partial<DragDroppableComponent> = {}, +): DragDroppableComponent { + return { + mounted: true, + props: makeProps(), + setState: jest.fn(), + ...overrides, + }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('canDrag allows dragging when disableDragDrop is false', () => { + expect(canDrag(makeProps({ disableDragDrop: false }))).toBe(true); +}); + +test('canDrag forbids dragging when disableDragDrop is true', () => { + expect(canDrag(makeProps({ disableDragDrop: true }))).toBe(false); +}); + +test('beginDrag captures the parent id and type when a parent component is present', () => { + const parentComponent = { + id: 'row-1', + type: 'ROW', + children: [], + meta: {}, + } as DragDroppableProps['parentComponent']; + const props = makeProps({ parentComponent, index: 2 }); + + expect(beginDrag(props)).toEqual({ + type: CHART_TYPE, + id: 'chart-1', + meta: {}, + index: 2, + parentId: 'row-1', + parentType: 'ROW', + }); +}); + +test('beginDrag leaves parent fields undefined when there is no parent component', () => { + const props = makeProps({ parentComponent: undefined }); + + expect(beginDrag(props)).toEqual({ + type: CHART_TYPE, + id: 'chart-1', + meta: {}, + index: 0, + parentId: undefined, + parentType: undefined, + }); +}); + +test('canDrop allows dropping when disableDragDrop is false', () => { + expect(canDrop(makeProps({ disableDragDrop: false }))).toBe(true); +}); + +test('canDrop forbids dropping when disableDragDrop is true', () => { + expect(canDrop(makeProps({ disableDragDrop: true }))).toBe(false); +}); + +test('hover delegates to handleHover when the drop target component is mounted', () => { + const props = makeProps(); + const monitor = {} as DropTargetMonitor; + const component = makeComponent({ mounted: true }); + + hover(props, monitor, component); + + expect(mockedHandleHover).toHaveBeenCalledWith(props, monitor, component); +}); + +test('hover does not call handleHover when the drop target component has unmounted', () => { + const props = makeProps(); + const monitor = {} as DropTargetMonitor; + const component = makeComponent({ mounted: false }); + + hover(props, monitor, component); + + expect(mockedHandleHover).not.toHaveBeenCalled(); +}); + +test('drop delegates to handleDrop when no nested target already produced a result', () => { + const props = makeProps(); + const component = makeComponent({ mounted: true }); + const expected: DropResult = { + source: { id: 'a', type: CHART_TYPE, index: 0 }, + dragging: { id: 'b', type: CHART_TYPE, meta: {} }, + }; + (mockedHandleDrop as jest.Mock).mockReturnValueOnce(expected); + const monitor = { + getDropResult: jest.fn(() => null), + } as unknown as DropTargetMonitor; + + const result = drop(props, monitor, component); + + expect(mockedHandleDrop).toHaveBeenCalledWith(props, monitor, component); + expect(result).toBe(expected); +}); + +test('drop delegates to handleDrop when a nested result has no destination', () => { + const props = makeProps(); + const component = makeComponent({ mounted: true }); + const monitor = { + getDropResult: jest.fn(() => ({ + source: { id: 'a', type: CHART_TYPE, index: 0 }, + dragging: { id: 'b', type: CHART_TYPE, meta: {} }, + })), + } as unknown as DropTargetMonitor; + + drop(props, monitor, component); + + expect(mockedHandleDrop).toHaveBeenCalledWith(props, monitor, component); +}); + +test('drop returns undefined and skips handleDrop when a nested target already produced a destination', () => { + const props = makeProps(); + const component = makeComponent({ mounted: true }); + const monitor = { + getDropResult: jest.fn(() => ({ + source: { id: 'a', type: CHART_TYPE, index: 0 }, + dragging: { id: 'b', type: CHART_TYPE, meta: {} }, + destination: { id: 'c', type: CHART_TYPE, index: 1 }, + })), + } as unknown as DropTargetMonitor; + + const result = drop(props, monitor, component); + + expect(result).toBeUndefined(); + expect(mockedHandleDrop).not.toHaveBeenCalled(); +}); + +test('drop returns undefined and skips handleDrop when the component has unmounted', () => { + const props = makeProps(); + const component = makeComponent({ mounted: false }); + const monitor = { + getDropResult: jest.fn(() => null), + } as unknown as DropTargetMonitor; + + const result = drop(props, monitor, component); + + expect(result).toBeUndefined(); + expect(mockedHandleDrop).not.toHaveBeenCalled(); +}); + +test('dragStateToProps reports isDragging and the dragged component identity from the monitor', () => { + const dragStateToProps = dragConfig[2]; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Magic tuple index accessors</b></div> <div id="fix"> `dragConfig[2]`/`dropConfig[2]` reach the collect functions by bare tuple index, while the existing tests destructure `dragConfig[1]`/`dropConfig[1]` into named consts (`canDrag`, `beginDrag`, ...) at lines 44-45. Named destructuring would match the file's own convention and survive tuple reordering. </div> </div> <small><i>Code Review Run #36d094</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]
