justinpark opened a new pull request, #40680:
URL: https://github.com/apache/superset/pull/40680
### SUMMARY
Fixes invalid multi-sort state in the AG Grid table header component after
upgrading to ag-grid-community v35.
Root cause: passing null via setSort triggered an equality check inside
_areSortDefsEqual that silently skipped dispatchColEvent('sortChanged'), so the
sortChanged listener never fired when clearing a sort.
Changes:
- Replaced setSort with progressSort — AG Grid internally computes the next
sort direction from sortingOrder, eliminating the need for the component to
track sort state via a useRef cycle and avoiding the null-dispatch bug entirely.
- Replaced api.addEventListener('sortChanged', cb) with
column.addEventListener('columnStateUpdated', cb) — columnStateUpdated fires
synchronously via colEventSvc after both the sort direction and sortIndex are
written, whereas sortChanged fires before updateSortIndex completes (stale
getSortIndex()) and is dispatched asynchronously.
- Multi-sort detection changed from iterating getAllDisplayedColumns() with
a falsy getSortIndex() check (broken for 0-based index) to c.getColId() !==
colId && c.getSort() !== null — checks other columns' sort direction rather
than relying on index truthiness.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
- Before
https://github.com/user-attachments/assets/a4ec9f59-2579-419a-b8fe-d77145b916d4
- After
https://github.com/user-attachments/assets/a11ff02b-6e20-4d37-9548-c78370daaab0
### TESTING INSTRUCTIONS
1. Open a table chart with multiple sortable columns.
2. Click a column header to sort ascending → verify sort icon appears.
3. Shift-click a second column → verify both columns show sort icons with
sequence numbers (1, 2).
4. Click the first column again to cycle through desc → no sort → verify the
second column's sort index updates to 1 and its sequence number updates
accordingly.
5. Run npm run test -- Header.test.tsx — all 8 tests should pass.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
