bito-code-review[bot] commented on code in PR #43350:
URL: https://github.com/apache/superset/pull/43350#discussion_r3817593086
##########
superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts:
##########
@@ -151,5 +165,23 @@ export const versionSessionLogMiddleware: Middleware =
}),
);
}
+ if (action.type !== HYDRATE_EXPLORE) {
+ const state = store.getState() as SessionLogState;
+ const controls = changedFormDataKeys(before, state.explore?.form_data);
+ if (
+ action.type === SET_FIELD_VALUE &&
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate dispatch for user edits</b></div>
<div id="fix">
The SET_FIELD_VALUE check at line 172 lacks the `!action.programmatic` guard
that the existing session-log block has (line 156). Without it,
non-programmatic SET_FIELD_VALUE actions dispatch BOTH
`APPEND_VERSION_SESSION_LOG` (from the existing block) AND
`invalidateChartNormalizationControls` (from the new block) — two dispatches
instead of one. The test at line 81 covers only the programmatic path, so this
bug is undetected.
</div>
</div>
<small><i>Code Review Run #04462c</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/saveModalActions.ts:
##########
@@ -233,21 +245,69 @@ export const updateSlice =
new?: boolean;
},
) =>
- async (dispatch: Dispatch, getState: () => Partial<QueryFormData>) => {
+ async (
+ dispatch: Dispatch,
+ getState: () => Partial<QueryFormData> & {
+ versionHistory?: {
+ chartNormalization?: ChartNormalizationTrackingState | null;
+ };
+ },
+ ) => {
const { slice_id: sliceId, editors, form_data: formDataFromSlice } = slice;
- const formData = getState().explore?.form_data;
+ const initialState = getState();
+ const formData = JSON.parse(
+ JSON.stringify(initialState.explore?.form_data ?? {}),
+ ) as QueryFormData;
+ const tracking = initialState.versionHistory?.chartNormalization;
+ const saveAttemptId = nanoid();
+ const matchingExclusions = Object.fromEntries(
+ Object.entries(tracking?.exclusions ?? {}).filter(
+ ([control, transition]) =>
+ !tracking?.invalidatedControls[control] &&
+ Object.hasOwn(formData, control) === transition.to_present &&
+ (!transition.to_present ||
+ JSON.stringify(formData[control]) ===
+ JSON.stringify(transition.to_value)),
+ ),
+ ) as AutomaticNormalizationExclusions;
+ const shouldAttachNormalization =
+ isFeatureEnabled(FeatureFlag.VersionHistory) &&
+ tracking?.chartId === sliceId;
+ if (shouldAttachNormalization) {
+ dispatch(
+ beginChartNormalizationSave(
+ sliceId,
+ tracking.hydrationSessionId,
+ saveAttemptId,
+ ),
+ );
+ }
try {
+ const payload = await getSlicePayload(
+ sliceName,
+ formData,
+ dashboards,
+ editors as [],
+ formDataFromSlice,
+ );
+ if (shouldAttachNormalization && Object.keys(matchingExclusions).length)
{
+ payload.normalization_changes = Object.values(matchingExclusions);
+ }
const response = await SupersetClient.put({
endpoint: `/api/v1/chart/${sliceId}`,
- jsonPayload: await getSlicePayload(
- sliceName,
- formData,
- dashboards,
- editors as [],
- formDataFromSlice,
- ),
+ jsonPayload: payload,
});
+ if (shouldAttachNormalization) {
+ dispatch(
+ completeChartNormalizationSave(
+ sliceId,
+ tracking.hydrationSessionId,
+ saveAttemptId,
+ {},
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing exclusions state update</b></div>
<div id="fix">
The `matchingExclusions` computed on lines 263-272 are attached to the API
payload on line 294, but the same value is not passed to
`completeChartNormalizationSave` on line 307 — instead, an empty object `{}` is
passed. This causes the reducer to overwrite the tracking state's `exclusions`
field with `{}`, making the client's exclusion state inconsistent with what was
actually sent to the server.
</div>
</div>
<small><i>Code Review Run #04462c</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]
