codeant-ai-for-open-source[bot] commented on code in PR #36882:
URL: https://github.com/apache/superset/pull/36882#discussion_r2655962475
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -395,22 +395,36 @@ const FilterBar: FC<FiltersBarProps> = ({
]);
const handleClearAll = useCallback(() => {
+ dispatch(logEvent(LOG_ACTIONS_CHANGE_DASHBOARD_FILTER, {}));
+ setUpdateKey(1);
const newClearAllTriggers = { ...clearAllTriggers };
// Clear all native filters, not just those in scope
- // This ensures dependent filters are cleared even if parent was cleared
first
+ const updates: Record<string, DataMaskWithId> = {};
nativeFilterValues.forEach(filter => {
const { id } = filter;
if (dataMaskSelected[id]) {
- setDataMaskSelected(draft => {
- if (draft[id].filterState?.value !== undefined) {
- draft[id].filterState!.value = undefined;
- }
- draft[id].extraFormData = {};
- });
+ updates[id] = {
+ ...dataMaskSelected[id],
+ filterState: {
+ ...dataMaskSelected[id].filterState,
+ value: null,
+ },
+ extraFormData: {},
+ };
newClearAllTriggers[id] = true;
}
});
+ setDataMaskSelected(draft => {
+ Object.entries(updates).forEach(([id, mask]) => {
+ draft[id] = mask;
+ });
+ });
+
+ Object.values(updates).forEach(mask => {
+ dispatch(updateDataMask(mask.id, mask));
Review Comment:
**Suggestion:** Wrong id passed to `updateDataMask` when dispatching clears:
the code dispatches `updateDataMask(mask.id, mask)` but `mask.id` may be
undefined or not match the object key; use the entry key (`id`) that was used
to build `updates` to ensure the correct filter id is passed to the action.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
Object.entries(updates).forEach(([id, mask]) => {
dispatch(updateDataMask(id, mask));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code uses mask.id when dispatching updateDataMask, but the
mapping key `id` is the authoritative filter id used to build `updates`.
mask.id could be undefined (depending on what was copied into the object) or
out-of-sync, so dispatching with the entry key is safer and more correct. The
improved code is a small, local logic fix that prevents passing an
incorrect/undefined id to the action.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
**Line:** 424:425
**Comment:**
*Logic Error: Wrong id passed to `updateDataMask` when dispatching
clears: the code dispatches `updateDataMask(mask.id, mask)` but `mask.id` may
be undefined or not match the object key; use the entry key (`id`) that was
used to build `updates` to ensure the correct filter id is passed to the 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>
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -395,22 +395,36 @@ const FilterBar: FC<FiltersBarProps> = ({
]);
const handleClearAll = useCallback(() => {
+ dispatch(logEvent(LOG_ACTIONS_CHANGE_DASHBOARD_FILTER, {}));
+ setUpdateKey(1);
Review Comment:
**Suggestion:** `setUpdateKey(1)` is used to signal updates but setting a
constant value means subsequent calls won't change the state and won't
retrigger effects that depend on `updateKey`; change to increment the previous
value so every call produces a new state and reliably triggers subscribers.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
setUpdateKey(prev => prev + 1);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
setUpdateKey(1) will only change state the first time (0 -> 1). Subsequent
calls when updateKey is already 1 will be no-ops and won't retrigger effects
relying on updateKey (e.g., publishDataMask). Using the functional updater
(prev => prev + 1) guarantees the state changes on every call and reliably
triggers subscribers. This fixes a real behavioral bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
**Line:** 399:399
**Comment:**
*Possible Bug: `setUpdateKey(1)` is used to signal updates but setting
a constant value means subsequent calls won't change the state and won't
retrigger effects that depend on `updateKey`; change to increment the previous
value so every call produces a new state and reliably triggers subscribers.
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>
--
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]
