bito-code-review[bot] commented on code in PR #44613:
URL: https://github.com/apache/superset/pull/44613#discussion_r4098517711
##########
superset-frontend/src/dashboard/reducers/dashboardInfo.ts:
##########
@@ -124,6 +126,13 @@ export default function dashboardInfoReducer(
action: DashboardInfoReducerAction,
): DashboardInfoState {
switch (action.type) {
+ case DASHBOARD_SAVE_SUCCEEDED:
+ return (action as DashboardInfoAction).dashboardId === state.id
+ ? {
+ ...state,
+ versionHistoryRevision: (state.versionHistoryRevision ?? 0) + 1,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated revision increment</b></div>
<div id="fix">
The increment `(state.versionHistoryRevision ?? 0) + 1` now appears in the
`DASHBOARD_SAVE_SUCCEEDED` case (line 133), the
`DASHBOARD_INFO_FILTERS_CHANGED` case (line 201), and `dashboardState.ts`'s
save path. Extracting a shared helper keeps the revision semantics
(initialization, bump conditions) in one place as the counter evolves.
</div>
</div>
<small><i>Code Review Run #c04e46</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/features/versionHistory/DashboardVersionHistory.tsx:
##########
@@ -141,44 +141,36 @@ export default function DashboardVersionHistory() {
// "Restored version" entry shows up.
const restoreCount = useSelector(selectVersionRestoreCount);
const lastRestoredUuid = useSelector(selectVersionLastRestoredUuid);
- // Saves made while the panel is open must surface as new timeline
- // entries without reopening it. Saves bump one of two redux signals
- // depending on the path: edit-mode saves round-trip through ON_SAVE
- // (dashboardState.lastModifiedTime), while native-filter and
- // properties saves bump dashboardInfo.last_modified_time.
- const saveSignal = useSelector<RootState, string>(state =>
+ // Only successful writes bump these revisions. Timestamps also change for
+ // local metadata edits and may repeat across multiple saves in one second.
+ const saveRevision = useAppSelector(state =>
[
- state.dashboardState?.lastModifiedTime ?? '',
- state.dashboardInfo?.last_modified_time ?? '',
+ state.dashboardState?.versionHistoryRevision ?? 0,
+ state.dashboardInfo?.versionHistoryRevision ?? 0,
].join('|'),
);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missed save-path refresh</b></div>
<div id="fix">
The new `saveRevision` selector only observes `versionHistoryRevision`, but
`saveChartConfiguration` (dashboard/actions/dashboardInfo.ts:92) saves
cross-filter scoping via `dashboardInfoChanged`, whose `DASHBOARD_INFO_UPDATED`
reducer updates `last_modified_time` without bumping a revision. The removed
`saveSignal` covered this server write; the timeline now goes stale after it.
Consider dispatching `dashboardSaveSucceeded(id)` there, mirroring
`chartCustomizationActions.ts:109`.
</div>
</div>
<small><i>Code Review Run #c04e46</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/reducers/dashboardState.ts:
##########
@@ -294,6 +295,7 @@ export default function dashboardStateReducer(
updatedColorScheme: false,
// server-side returns last_modified_time for latest change
lastModifiedTime: action.lastModifiedTime,
+ versionHistoryRevision: (state.versionHistoryRevision ?? 0) + 1,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated revision counter</b></div>
<div id="fix">
The `(state.versionHistoryRevision ?? 0) + 1` counter is now duplicated in
`dashboardState` (ON_SAVE) and `dashboardInfo` (DASHBOARD_SAVE_SUCCEEDED,
DASHBOARD_INFO_FILTERS_CHANGED), and `DashboardVersionHistory` must join both.
Two counters with different triggers risk divergence (one path bumps only one).
Consider a shared helper so the increment rule stays in one place.
</div>
</div>
<small><i>Code Review Run #c04e46</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/reducers/saveModalReducer.ts:
##########
@@ -56,11 +57,21 @@ export default function saveModalReducer(
return { ...state, saveModalAlert: 'Failed to save slice' };
},
[actions.SAVE_SLICE_SUCCESS]() {
- return { ...state, data: action.data };
+ const { data } = action;
+ const lastSavedChart =
+ typeof data === 'object' &&
+ data !== null &&
+ 'id' in data &&
+ typeof data.id === 'number'
+ ? { id: data.id }
+ : state.lastSavedChart;
+ return { ...state, data, lastSavedChart };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing tests for marker logic</b></div>
<div id="fix">
The new `lastSavedChart` logic — the SAVE_SLICE_SUCCESS validation fallback
and the HYDRATE_EXPLORE preservation (lines 72-74) — ships with no unit tests:
src/explore/reducers has no saveModalReducer.test.ts and no test references
`lastSavedChart`. This race-fix logic is subtle; a regression would silently
break save detection in `ExploreVersionHistory`. Please add reducer tests for
valid/invalid data and hydration preservation.
</div>
</div>
<small><i>Code Review Run #c04e46</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]
