bito-code-review[bot] commented on code in PR #37488: URL: https://github.com/apache/superset/pull/37488#discussion_r2875435157
########## superset-frontend/src/explore/reducers/undoableExploreReducer.ts: ########## @@ -0,0 +1,38 @@ +/** + * 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 { Reducer } from '@reduxjs/toolkit'; +import undoable, { includeAction, StateWithHistory } from 'redux-undo'; +import { UNDO_LIMIT } from 'src/dashboard/util/constants'; +import { SET_FIELD_VALUE, UPDATE_CHART_TITLE } from '../actions/exploreActions'; +import { ExploreState } from 'src/explore/types'; +import exploreReducer from './exploreReducer'; + +export type UndoableExploreState = StateWithHistory<ExploreState>; + +const undoableReducer: Reducer<UndoableExploreState> = undoable( + exploreReducer, + { + // +1 because length of history seems max out at limit - 1 + // +1 again so we can detect if we've exceeded the limit + limit: UNDO_LIMIT + 2, + filter: includeAction([SET_FIELD_VALUE, UPDATE_CHART_TITLE]), + }, +); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Broken redux-undo filter causes incorrect undo tracking</b></div> <div id="fix"> The redux-undo filter is broken in version 1.0.0-beta9-9-7, causing all actions to be included in undo history instead of just the intended ones. This leads to unexpected undo behavior where non-layout actions like loading states become reversible. Use a reducer wrapper like in undoableDashboardLayout.ts to filter at the reducer level. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const undoableReducer: Reducer<UndoableExploreState> = undoable( (state, action) => { if ([SET_FIELD_VALUE, UPDATE_CHART_TITLE].includes(action.type)) { return exploreReducer(state, action); } return state; }, { // +1 because length of history seems max out at limit - 1 // +1 again so we can detect if we've exceeded the limit limit: UNDO_LIMIT + 2, }, ); ```` </div> </details> </div> <small><i>Code Review Run #dc94c4</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/explore/actions/exploreActions.ts: ########## @@ -153,6 +154,27 @@ export function setForceQuery(force: boolean) { }; } +/** + * Undo an explore action and sync chart state. + */ +export function undoExploreAction() { + return UndoActionCreators.undo(); +} + +/** + * Redo an explore action and sync chart state. + */ +export function redoExploreAction() { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing return type annotation</b></div> <div id="fix"> The new function redoExploreAction lacks an explicit return type annotation, violating TypeScript modernization guidelines that require type hints on all functions for better static checking and consistency. Add : Action to the signature, as it returns a standard Redux action object. </div> </div> <small><i>Code Review Run #dc94c4</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/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx: ########## @@ -1166,3 +1167,105 @@ describe('Additional actions tests', () => { }); }); }); + +const setupUndoRedoTest = () => { + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave: jest.fn(), + }); + + const props = createProps(); + render(<ExploreHeader {...props} />, { useRedux: true }); +}; + +test('Should render undo and redo buttons', async () => { + setupUndoRedoTest(); + + expect(await screen.findByTestId('undo-button')).toBeInTheDocument(); + expect(await screen.findByTestId('redo-button')).toBeInTheDocument(); +}); + +test('Undo button should be disabled initially', async () => { + setupUndoRedoTest(); + + const undoButton = await screen.findByTestId('undo-button'); + expect(undoButton).toBeDisabled(); +}); + +test('Redo button should be disabled initially', async () => { + setupUndoRedoTest(); + + const redoButton = await screen.findByTestId('redo-button'); + expect(redoButton).toBeDisabled(); +}); + +test('Should not dispatch undoExploreAction when undo button is disabled', async () => { + const undoSpy = jest.spyOn(exploreActions, 'undoExploreAction'); + setupUndoRedoTest(); + + const undoButton = await screen.findByTestId('undo-button'); + expect(undoButton).toBeDisabled(); + + userEvent.click(undoButton); + + expect(undoSpy).not.toHaveBeenCalled(); + + undoSpy.mockRestore(); +}); + +test('Should not dispatch redoExploreAction when redo button is disabled', async () => { + const redoSpy = jest.spyOn(exploreActions, 'redoExploreAction'); + setupUndoRedoTest(); + + const redoButton = await screen.findByTestId('redo-button'); + expect(redoButton).toBeDisabled(); + + userEvent.click(redoButton); + + expect(redoSpy).not.toHaveBeenCalled(); + + redoSpy.mockRestore(); +}); + +test('Should call undoExploreAction on Ctrl+Z keyboard shortcut', async () => { + const undoSpy = jest.spyOn(exploreActions, 'undoExploreAction'); + setupUndoRedoTest(); + + // Simulate Ctrl+Z + const event = new KeyboardEvent('keydown', { + key: 'z', + keyCode: 90, + ctrlKey: true, + bubbles: true, + }); + document.dispatchEvent(event); + + await waitFor(() => { + expect(undoSpy).toHaveBeenCalledTimes(1); + }); + + undoSpy.mockRestore(); +}); + +test('Should call redoExploreAction on Ctrl+Y keyboard shortcut', async () => { + const redoSpy = jest.spyOn(exploreActions, 'redoExploreAction'); + setupUndoRedoTest(); + + // Simulate Ctrl+Y + const event = new KeyboardEvent('keydown', { + key: 'y', + keyCode: 89, + ctrlKey: true, + bubbles: true, + }); + document.dispatchEvent(event); + + await waitFor(() => { + expect(redoSpy).toHaveBeenCalledTimes(1); + }); + + redoSpy.mockRestore(); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Tests for unimplemented keyboard shortcuts</b></div> <div id="fix"> The tests for keyboard shortcuts (Ctrl+Z for undo, Ctrl+Y for redo) are asserting behavior that is not implemented in the codebase. No event listeners handle these key combinations in ExploreChartHeader or its parent components. This violates the guideline that tests should verify actual business logic and behavior, not unimplemented features. </div> </div> <small><i>Code Review Run #dc94c4</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/explore/actions/exploreActions.ts: ########## @@ -153,6 +154,27 @@ export function setForceQuery(force: boolean) { }; } +/** + * Undo an explore action and sync chart state. + */ +export function undoExploreAction() { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing return type annotation</b></div> <div id="fix"> The new function undoExploreAction lacks an explicit return type annotation, violating TypeScript modernization guidelines that require type hints on all functions for better static checking and consistency. Add : Action to the signature, as it returns a standard Redux action object. </div> </div> <small><i>Code Review Run #dc94c4</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/explore/components/controls/TextControl/index.tsx: ########## @@ -62,6 +62,14 @@ export default class TextControl< }; } + componentDidUpdate(prevProps: TextControlProps<T>) { + // Sync local state when props change (e.g., from undo/redo) + if (prevProps.value !== this.props.value) { + this.initialValue = this.props.value; + this.setState({ value: safeStringify(this.props.value) }); + } + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Render method has side effects</b></div> <div id="fix"> The render method includes a side effect that mutates this.initialValue when props.value changes. Although the added componentDidUpdate now handles prop changes by syncing state, this logic in render is redundant and incorrect. Render should be pure. Remove the if block in render to fix this. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - render() { - let { value } = this.state; - if (this.initialValue !== this.props.value) { - this.initialValue = this.props.value; - value = safeStringify(this.props.value); - } - return ( + render() { + let { value } = this.state; + return ( ``` </div> </details> </div> <small><i>Code Review Run #dc94c4</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/explore/actions/exploreActions.ts: ########## @@ -153,6 +154,27 @@ export function setForceQuery(force: boolean) { }; } +/** + * Undo an explore action and sync chart state. + */ +export function undoExploreAction() { + return UndoActionCreators.undo(); +} + +/** + * Redo an explore action and sync chart state. + */ +export function redoExploreAction() { + return UndoActionCreators.redo(); +} + +/** + * Clear explore undo/redo history. + */ +export function clearExploreHistory() { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing return type annotation</b></div> <div id="fix"> The new function clearExploreHistory lacks an explicit return type annotation, violating TypeScript modernization guidelines that require type hints on all functions for better static checking and consistency. Add : Action to the signature, as it returns a standard Redux action object. </div> </div> <small><i>Code Review Run #dc94c4</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/explore/components/useExploreAdditionalActionsMenu/index.tsx: ########## @@ -212,10 +197,12 @@ export const useExploreAdditionalActionsMenu = ( dashboardSearchTerm, 300, ); - const chart = useSelector<ExploreState, ChartState | undefined>(state => - state.explore ? state.charts?.[getChartKey(state.explore)] : undefined, + const chart = useSelector<any, ChartState | undefined>(state => + (state.explore as any)?.present + ? state.charts?.[getChartKey((state.explore as any).present)] + : undefined, ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Use proper TypeScript types instead of any</b></div> <div id="fix"> The useSelector calls use 'any' for the state type, which violates the development standards requiring proper TypeScript types. Replace with ExplorePageState for type safety. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - const chart = useSelector<any, ChartState | undefined>(state => - (state.explore as any)?.present - ? state.charts?.[getChartKey((state.explore as any).present)] - : undefined, - ); - const streamingThreshold = useSelector<any, number>( - state => - state.common?.conf?.CSV_STREAMING_ROW_THRESHOLD || - DEFAULT_CSV_STREAMING_ROW_THRESHOLD, - ); + const chart = useSelector<ExplorePageState, ChartState | undefined>(state => + state.explore?.present + ? state.charts?.[getChartKey(state.explore.present)] + : undefined, + ); + const streamingThreshold = useSelector<ExplorePageState, number>( + state => + state.common?.conf?.CSV_STREAMING_ROW_THRESHOLD || + DEFAULT_CSV_STREAMING_ROW_THRESHOLD, + ); ``` </div> </details> </div> <small><i>Code Review Run #dc94c4</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]
