korbit-ai[bot] commented on code in PR #33140:
URL: https://github.com/apache/superset/pull/33140#discussion_r2045630553
##########
superset-frontend/src/explore/components/DatasourcePanel/fixtures.tsx:
##########
@@ -97,7 +97,168 @@ const metricsFiltered = {
],
};
+export const columnsUnsorted = [
+ {
+ column_name: 'ccc',
+ description: null,
+ expression: 'null',
+ filterable: true,
Review Comment:
### Inconsistent null value representation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The expression field contains a string 'null' instead of the JavaScript null
value, which is inconsistent with other null values in the fixtures.
###### Why this matters
This inconsistency makes it harder to understand whether this is intentional
or a mistake, and could lead to confusion when using these test fixtures.
###### Suggested change ∙ *Feature Preview*
Change `expression: 'null'` to `expression: null`
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb257c8f-9b54-4b8d-8a09-aa628e4f2f28/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb257c8f-9b54-4b8d-8a09-aa628e4f2f28?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb257c8f-9b54-4b8d-8a09-aa628e4f2f28?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb257c8f-9b54-4b8d-8a09-aa628e4f2f28?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb257c8f-9b54-4b8d-8a09-aa628e4f2f28)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4c65bac6-3e37-4f56-8232-00f8470f1451 -->
[](4c65bac6-3e37-4f56-8232-00f8470f1451)
##########
superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts:
##########
@@ -25,6 +25,26 @@ import {
MetricItem,
} from './types';
+const sortMetrics = (arr: MetricItem[]) =>
+ arr.sort((a, b) => {
+ const certCmp = Number(b?.is_certified ?? 0) - Number(a?.is_certified ??
0);
+ if (certCmp) return certCmp;
+
+ const aName = (a.verbose_name || a.metric_name) ?? '';
+ const bName = (b.verbose_name || b.metric_name) ?? '';
+ return aName.localeCompare(bName, undefined, { sensitivity: 'base' });
+ });
+
+const sortColumns = (arr: ColumnItem[]) =>
+ arr.sort((a, b) => {
+ const certCmp = Number(b?.is_certified ?? 0) - Number(a?.is_certified ??
0);
+ if (certCmp) return certCmp;
+
+ const aName = (a.verbose_name || a.column_name) ?? '';
+ const bName = (b.verbose_name || b.column_name) ?? '';
+ return aName.localeCompare(bName, undefined, { sensitivity: 'base' });
+ });
Review Comment:
### Duplicate sorting logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sortMetrics and sortColumns functions are nearly identical with only
slight differences in property names, making the code harder to maintain and
understand.
###### Why this matters
Duplicate sorting logic increases cognitive load and makes future
modifications error-prone as changes need to be made in multiple places.
###### Suggested change ∙ *Feature Preview*
Create a single generic sorting function:
```typescript
const sortItems = <T extends { is_certified?: boolean }>(
arr: T[],
getNameFn: (item: T) => string,
) =>
arr.sort((a, b) => {
const certCmp = Number(b?.is_certified ?? 0) - Number(a?.is_certified ??
0);
if (certCmp) return certCmp;
return getNameFn(a).localeCompare(getNameFn(b), undefined, {
sensitivity: 'base' });
});
const sortMetrics = (arr: MetricItem[]) =>
sortItems(arr, item => (item.verbose_name || item.metric_name) ?? '');
const sortColumns = (arr: ColumnItem[]) =>
sortItems(arr, item => (item.verbose_name || item.column_name) ?? '');
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c1b381a1-54dd-4dd8-98d6-671b4067f90c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c1b381a1-54dd-4dd8-98d6-671b4067f90c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c1b381a1-54dd-4dd8-98d6-671b4067f90c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c1b381a1-54dd-4dd8-98d6-671b4067f90c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c1b381a1-54dd-4dd8-98d6-671b4067f90c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3c1a375a-f2db-4951-b60d-59b866dfa500 -->
[](3c1a375a-f2db-4951-b60d-59b866dfa500)
--
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]
