EnxDev commented on PR #42475:
URL: https://github.com/apache/superset/pull/42475#issuecomment-5141247693
## EnxDev's Review Agent — apache/superset#42475 · HEAD 335a118
**comment** — the fix works for the reported case, but the hand-rolled
`<`/`>` comparator sorts by UTF-16 code point, which diverges from the API's
`table_name asc` ordering for mixed-case names.
### On the two conflicting bot suggestions
@SkinnyPigeon — Bito is right, CodeAnt is not, and there's a third option
that settles it.
CodeAnt's objection is that `localeCompare` is locale-dependent and can
disagree with the server's collation. True, but it doesn't favour `<`/`>`:
exact parity with the metadata DB's collation is unreachable from the browser
regardless, and code-point ordering is *further* from every common server
collation, not closer. Postgres (`en_US.UTF-8`) and MySQL (`utf8mb4_*_ci`) both
order `apple` before `Zebra`; `<`/`>` orders `Zebra` before `apple` because `Z`
is 0x5A and `a` is 0x61. Only SQLite's `BINARY` collation matches code-point
order.
Superset already settled this internally: `DEFAULT_SORT_COMPARATOR` uses
`localeCompare` (`Select/constants.ts:101`), as does the exported
`propertyComparator` helper (`Select/utils.tsx:89`). Use the helper instead of
hand-rolling — one line, and it's the established pattern
(`AlertReportModal.tsx:2649` does
`sortComparator={propertyComparator('value')}`):
```tsx
import { propertyComparator } from
'@superset-ui/core/components/Select/utils';
// ...
sortComparator={propertyComparator('table_name')}
```
### 🔴 Functional
- **`superset-frontend/src/pages/ChartCreation/index.tsx:346`** · _High_ —
Code-point comparison groups every capitalized dataset name ahead of every
lowercase one (`Orders`, `Products`, `customers`, `flights`), so the dropdown
contradicts both the API's `table_name asc` result and the Datasets list view —
the consistency #42466 asks for. Second effect: `AsyncSelect` re-sorts the
merged set on every page fetch (`AsyncSelect.tsx:382-392`, `DEFAULT_PAGE_SIZE =
100`), so past 100 datasets, page-2 items whose case ranks them ahead of
already-rendered page-1 items get inserted *above* the visible list and rows
jump while scrolling. Both go away if the client comparator agrees with the
server ordering: `sortComparator={propertyComparator('table_name')}`.
**regression test:** fixture with `table_name` values `Zebra`, `apple`, `Mango`
and asserts `apple` renders before `Mango` before `Zebra` — that fails on
`<`/`>` and passes on `localeCompare`.
### 🟡 Should-fix
- **`superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx:336`**
— the fixture is all-lowercase, so it passes under either comparator and can't
tell the two apart. Add a mixed-case name; that's the only case that
discriminates. Codecov also reports 83.3% patch coverage with 1 uncovered line
in `index.tsx` — one of the comparator's branches is never exercised.
- **PR description** — the stated root cause is wrong, which matters because
it's what motivated the hand-rolled comparator. `DEFAULT_SORT_COMPARATOR`
doesn't fall through to `(a.value as number) - (b.value as number)`: when the
label isn't a string it checks `typeof a.value === 'string'` first
(`Select/constants.ts:92-95`), and `value` *is* a string (`"5__table"`), so it
does `localeCompare("2__table", "5__table")`. No `NaN` — the options are sorted
lexicographically by `"{id}__{type}"`, which is why they come out in ID order
(and why with ≥10 datasets `10__table` sorts before `2__table`).
### 🔵 Nits
- `superset-frontend/src/pages/ChartCreation/index.tsx:346` — the comparator
drops the third `search` argument that `AsyncSelect` passes through
(`Select/utils.tsx:127`). Not a regression — pre-PR the ranking ran against
`"{id}__table"` values and was meaningless — but now that a real name is
available, `rankedSearchCompare(aName, bName, search ?? '')` would put prefix
matches first while typing.
### 🙌 Praise
- `ChartCreation.test.tsx:336` — the fixture (`gamma`/`alpha`/`beta` at ids
2/3/1) is a genuine regression guard: without the comparator the default sorts
by value to `beta, gamma, alpha` and the DOM-order assertions fail. It
addresses the earlier review comment properly rather than just reordering to
look unsorted.
<!-- enxdev-review-agent:335a118 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 335a118._
--
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]
