codeant-ai-for-open-source[bot] commented on code in PR #42475:
URL: https://github.com/apache/superset/pull/42475#discussion_r3672056019
##########
superset-frontend/src/pages/ChartCreation/index.tsx:
##########
@@ -343,6 +343,13 @@ export const ChartCreation = ({
optionFilterProps={['id', 'table_name']}
placeholder={t('Choose a %s', datasetLabelLower())}
showSearch
+ sortComparator={(a, b) => {
+ const aName =
+ (a as { table_name?: string }).table_name ?? '';
+ const bName =
+ (b as { table_name?: string }).table_name ?? '';
+ return aName.localeCompare(bName);
+ }}
Review Comment:
**Suggestion:** The API request already orders datasets by `table_name`, but
this comparator replaces that ordering with the browser's default-locale
`localeCompare` result. Names containing uppercase or accented characters can
therefore appear in a different order from the server result, and the order can
vary with the user's locale/browser. Use the same deterministic ordering
semantics as the API, or avoid re-sorting the server-ordered page when the
comparator cannot preserve that contract. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dataset order varies across browser locales.
- ⚠️ Mixed-case or accented names can disagree with API ordering.
- ⚠️ Users may see inconsistent Chart Creation dropdown ordering.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Chart Creation dataset selector rendered by `ChartCreation` at
`superset-frontend/src/pages/ChartCreation/index.tsx:337-355`.
2. The selector requests `/api/v1/dataset/` through `loadDatasources()` at
`superset-frontend/src/pages/ChartCreation/index.tsx:227-252`, explicitly
requesting
`order_column: 'table_name'` and `order_direction: 'asc'` at lines 243-244.
3. Return or create datasets whose names include case or accents, such as
`apple`, `Zoo`,
or an accented table name. The backend applies its database/SQL collation
through the
dataset API, whose allowed ordering includes `table_name` at
`superset/datasets/api.py:183-185`.
4. `AsyncSelect` then invokes the new comparator at
`superset-frontend/src/pages/ChartCreation/index.tsx:346-352`;
`String.localeCompare()`
uses the browser's default locale and collation, which can rank those names
differently
from the server. The visible dropdown order can consequently vary by
browser/user locale
and disagree with the API's `table_name asc` order. Use deterministic
comparison semantics
matching the backend, or preserve the server ordering.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5c53e4f24f8a49f983e5e4599b598cf7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5c53e4f24f8a49f983e5e4599b598cf7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/ChartCreation/index.tsx
**Line:** 346:352
**Comment:**
*Logic Error: The API request already orders datasets by `table_name`,
but this comparator replaces that ordering with the browser's default-locale
`localeCompare` result. Names containing uppercase or accented characters can
therefore appear in a different order from the server result, and the order can
vary with the user's locale/browser. Use the same deterministic ordering
semantics as the API, or avoid re-sorting the server-ordered page when the
comparator cannot preserve that contract.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42475&comment_hash=8faba6a22957ed6f02cb7f41b69ccc63d5a0cb44165a4ab331377a88e24aeb9e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42475&comment_hash=8faba6a22957ed6f02cb7f41b69ccc63d5a0cb44165a4ab331377a88e24aeb9e&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx:
##########
@@ -333,6 +333,63 @@ test('shows loading spinner when dataset parameter is
present in URL', async ()
locationSpy.mockRestore();
});
+test('dataset dropdown sorts options alphabetically by table name regardless
of id order', async () => {
+ fetchMock.clearHistory().removeRoutes();
+ // IDs are deliberately NOT in alphabetical order of table_name to prove
+ // that sorting is by name, not by creation/id order.
+ fetchMock.get(/\/api\/v1\/dataset\/\?q=.*/, {
+ body: {
+ result: [
+ {
+ id: 3,
+ table_name: 'alpha_table',
+ datasource_type: 'table',
+ database: { database_name: 'test_db' },
+ schema: 'public',
+ },
+ {
+ id: 1,
+ table_name: 'beta_table',
+ datasource_type: 'table',
+ database: { database_name: 'test_db' },
+ schema: 'public',
+ },
+ {
+ id: 2,
+ table_name: 'gamma_table',
+ datasource_type: 'table',
+ database: { database_name: 'test_db' },
+ schema: 'public',
+ },
+ ],
Review Comment:
**Suggestion:** The mocked response is already ordered as `alpha_table`,
`beta_table`, `gamma_table`, so these assertions pass even when
`sortComparator` is ignored and the component simply preserves the server
response order. Supply the fixture in a deliberately unsorted `table_name`
order (while keeping IDs independently unordered) so the test actually verifies
client-side sorting. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ The regression test can pass when sorting is broken.
- ⚠️ Future comparator regressions may reach production undetected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the new test `dataset dropdown sorts options alphabetically by table
name
regardless of id order` at
`superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx:336`.
2. The mocked `GET /api/v1/dataset/` response at
`superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx:340-368`
returns
`alpha_table`, `beta_table`, and `gamma_table` already in alphabetical order.
3. `ChartCreation.loadDatasources()` at
`superset-frontend/src/pages/ChartCreation/index.tsx:227-252` maps
`response.json.result`
without changing its order, so the dropdown receives the same alphabetical
sequence even
if the new comparator is removed or ignored.
4. The DOM-order assertions at
`superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx:384-390`
therefore pass
for an implementation that merely preserves server order; change the fixture
to an
unsorted table-name sequence while retaining independently unordered IDs to
verify
client-side sorting.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8d3b2fff23cb4ec8b37cce21fe0a163f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8d3b2fff23cb4ec8b37cce21fe0a163f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx
**Line:** 342:364
**Comment:**
*Possible Bug: The mocked response is already ordered as `alpha_table`,
`beta_table`, `gamma_table`, so these assertions pass even when
`sortComparator` is ignored and the component simply preserves the server
response order. Supply the fixture in a deliberately unsorted `table_name`
order (while keeping IDs independently unordered) so the test actually verifies
client-side sorting.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42475&comment_hash=f6aff889bc8d1faa96d2209dfac485f88650a645b04caa865882a9171bbd976a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42475&comment_hash=f6aff889bc8d1faa96d2209dfac485f88650a645b04caa865882a9171bbd976a&reaction=dislike'>👎</a>
--
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]
