codeant-ai-for-open-source[bot] commented on code in PR #37488: URL: https://github.com/apache/superset/pull/37488#discussion_r2802823093
########## 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: **Suggestion:** The undo history is never cleared when a new Explore payload is hydrated, so after opening a different chart or dataset, pressing undo will revert the UI back to the previous chart's configuration instead of staying scoped to the current chart; configure the redux-undo reducer to clear history on the HYDRATE_EXPLORE action. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Explore undo after chart switch restores previous chart configuration. - ⚠️ Users see controls jump to prior chart unexpectedly. ``` </details> ```suggestion import { HYDRATE_EXPLORE } from '../actions/hydrateExplore'; 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]), clearHistoryType: HYDRATE_EXPLORE, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the frontend with this PR, where the Explore reducer is wrapped by the undoable reducer defined in `superset-frontend/src/explore/reducers/undoableExploreReducer.ts:19-38` and registered into the Redux store (per PR description, in `superset-frontend/src/views/store.ts`). 2. Open the Explore view for a saved chart A (e.g. `/explore/?slice_id=10`), which hydrates the explore state using the existing `HYDRATE_EXPLORE` action from `superset-frontend/src/explore/actions/hydrateExplore.ts` handled by `exploreReducer` (the inner reducer passed to `undoable()` at line 28). 3. Change any control that dispatches `SET_FIELD_VALUE` (included in the undo filter at line 34) so that the current configuration of chart A is pushed into the undo history managed by `undoableExploreReducer`. 4. Navigate to a different saved chart B (e.g. `/explore/?slice_id=20`), which dispatches `HYDRATE_EXPLORE` again; because `clearHistoryType` is not configured in `undoable()` options (lines 31–35), redux-undo keeps the past state for chart A, so pressing the new undo shortcut/button in `superset-frontend/src/explore/components/ExploreChartHeader/index.jsx` reverts the Explore controls back to chart A's configuration while you are on chart B, mixing states across charts instead of resetting per hydration. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/explore/reducers/undoableExploreReducer.ts **Line:** 23:34 **Comment:** *Logic Error: The undo history is never cleared when a new Explore payload is hydrated, so after opening a different chart or dataset, pressing undo will revert the UI back to the previous chart's configuration instead of staying scoped to the current chart; configure the redux-undo reducer to clear history on the HYDRATE_EXPLORE action. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37488&comment_hash=ffe07ec1135c3b75d5608fe275db77bb7da00e1ddbb62d73779c14b7ce9ebcf0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37488&comment_hash=ffe07ec1135c3b75d5608fe275db77bb7da00e1ddbb62d73779c14b7ce9ebcf0&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]
