EnxDev commented on PR #42032:
URL: https://github.com/apache/superset/pull/42032#issuecomment-4969280064
## EnxDev's Review Agent โ apache/superset#42032 ยท HEAD 8c9b781
request changes โ the merge is the right root-cause fix for the filter-wipe
crash, but it silently drops the only mechanism that propagated filter
*reordering* to the filter bar.
### ๐ด Functional
- **`superset-frontend/src/dashboard/reducers/nativeFilters.ts:89`** ยท
_High_ โ Reordering filters in the FiltersConfigModal no longer updates the
filter bar until page reload. The backend
(`DashboardDAO.update_native_filters_config`) returns the full config in the
**new** order, and the old rebuild-from-response adopted that key order;
merging into `{ ...state.filters }` keeps existing keys in their old positions
(JS assignment to an existing key preserves its slot). The bar renders
`Object.values(filters)` with no other sort (`useFilterControlFactory.tsx:41`,
`FilterBar/index.tsx:194`), and nothing else re-syncs the map order until
`HYDRATE_DASHBOARD`. Fix: both save paths already have the reordered IDs
(`filterChanges.reordered`, `reorderedIds`) โ pass them into the action and
rebuild the key order from them (or adopt the payload's order for the payload's
subset). **regression test:** reducer test โ initial map `{filter1, filter2}`,
payload `[filter2, filter1]` with the
reorder info, assert `Object.keys(result.filters)` equals `['filter2',
'filter1']`.
### ๐ก Should-fix
- **`superset-frontend/src/dashboard/util/getRelatedCharts.ts:87`** โ the
guard handles `undefined` but the parameter stays typed `Filter`, so the new
test needs `undefined as unknown as Filter` and the real crash site keeps
masking the problem with a cast (`useFilterFocusHighlightStyles.ts:67`,
`filters[id] as Filter`). Type it `filter: Filter | undefined` and drop both
casts so tsc flags future unguarded callers.
### ๐ Praise
- `superset-frontend/src/dashboard/reducers/nativeFilters.ts:81` โ right
call fixing the root cause (merge + explicit `deletedIds` on both save paths)
instead of only guarding the crash, and the updated tests encode the new
merge/delete semantics clearly.
<!-- enxdev-review-agent:8c9b781 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 8c9b781._
--
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]
