korbit-ai[bot] commented on code in PR #34149:
URL: https://github.com/apache/superset/pull/34149#discussion_r2203494362
##########
superset-frontend/src/components/TagsList/index.tsx:
##########
@@ -47,6 +47,15 @@ export const TagsList = ({
onDelete,
maxTags,
}: TagsListProps) => {
+ const sortedTags = useMemo(
+ () =>
+ tags.toSorted((a, b) => {
+ const nameA = a.name ?? '';
+ const nameB = b.name ?? '';
Review Comment:
### Unnecessary intermediary variables <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The null coalescing fallback values are assigned to intermediary variables,
making the sorting logic less concise.
###### Why this matters
Using intermediary variables for simple operations increases the cognitive
load when reading the code, especially when those variables are used only once.
###### Suggested change ∙ *Feature Preview*
```typescript
tags.toSorted((a, b) => (a.name ?? '').localeCompare(b.name ?? ''))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e823dc2-c23f-4098-8b2e-d0f9007e32d4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e823dc2-c23f-4098-8b2e-d0f9007e32d4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e823dc2-c23f-4098-8b2e-d0f9007e32d4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e823dc2-c23f-4098-8b2e-d0f9007e32d4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e823dc2-c23f-4098-8b2e-d0f9007e32d4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:388c637f-4dab-4a43-8241-a3bcbb48914a -->
[](388c637f-4dab-4a43-8241-a3bcbb48914a)
##########
superset-frontend/src/components/TagsList/index.tsx:
##########
@@ -47,6 +47,15 @@
onDelete,
maxTags,
}: TagsListProps) => {
+ const sortedTags = useMemo(
+ () =>
+ tags.toSorted((a, b) => {
+ const nameA = a.name ?? '';
Review Comment:
### Missing tag name validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The default empty string fallback for null/undefined tag names could lead to
inconsistent sorting behavior when some tags have no names.
###### Why this matters
Empty string tags will be sorted to the beginning of the list, which may
confuse users and hide the fact that there are invalid tags in the system.
###### Suggested change ∙ *Feature Preview*
Either filter out invalid tags or throw an error for tags without names:
```typescript
const sortedTags = useMemo(
() =>
tags
.filter(tag => tag.name != null)
.toSorted((a, b) => a.name!.localeCompare(b.name!)),
[tags],
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc56955e-08db-4cb0-abef-02cdac2cd5f7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc56955e-08db-4cb0-abef-02cdac2cd5f7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc56955e-08db-4cb0-abef-02cdac2cd5f7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc56955e-08db-4cb0-abef-02cdac2cd5f7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc56955e-08db-4cb0-abef-02cdac2cd5f7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:43d2d7b8-96c8-4cb0-9a0b-98c2ab4ded75 -->
[](43d2d7b8-96c8-4cb0-9a0b-98c2ab4ded75)
##########
superset-frontend/src/components/TagsList/index.tsx:
##########
@@ -103,7 +114,7 @@
/>
))}
{maxTags ? (
- tags.length > maxTags ? (
+ sortedTags.length > maxTags ? (
<Tag name="..." onClick={collapse} />
) : null
) : null}
Review Comment:
### Complex nested ternary expressions <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Nested ternary expressions with null checks make the collapse button
rendering logic hard to follow.
###### Why this matters
Multiple levels of conditional rendering using ternary operators reduce code
readability and make it difficult to understand the conditions for rendering
the collapse button.
###### Suggested change ∙ *Feature Preview*
```typescript
{maxTags && sortedTags.length > maxTags && (
<Tag name="..." onClick={collapse} />
)}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b4c81b0-8161-48de-b537-1dcfaec50ae0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b4c81b0-8161-48de-b537-1dcfaec50ae0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b4c81b0-8161-48de-b537-1dcfaec50ae0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b4c81b0-8161-48de-b537-1dcfaec50ae0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b4c81b0-8161-48de-b537-1dcfaec50ae0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1530ff95-ec61-4792-9279-3e5382f28215 -->
[](1530ff95-ec61-4792-9279-3e5382f28215)
##########
superset-frontend/src/components/TagsList/index.tsx:
##########
@@ -47,6 +47,15 @@
onDelete,
maxTags,
}: TagsListProps) => {
+ const sortedTags = useMemo(
+ () =>
+ tags.toSorted((a, b) => {
+ const nameA = a.name ?? '';
+ const nameB = b.name ?? '';
+ return nameA.localeCompare(nameB);
+ }),
+ [tags],
+ );
Review Comment:
### Sorting Logic Should Be Extracted from Display Component <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sorting logic is embedded directly in the component, violating the
Single Responsibility Principle. This component is handling both display and
data transformation concerns.
###### Why this matters
This makes the component less reusable and harder to maintain. If sorting
requirements change, we'd need to modify the display component rather than
handling this at a data management level.
###### Suggested change ∙ *Feature Preview*
Move the sorting logic to a separate utility function or handle it at the
data management level before passing to the component:
```typescript
// In a separate utility file
export const sortTagsByName = (tags: TagType[]) =>
tags.toSorted((a, b) => (a.name ?? '').localeCompare(b.name ?? ''));
// In the component
const sortedTags = useMemo(() => sortTagsByName(tags), [tags]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f62fbf17-997e-4005-8974-602ff855f708/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f62fbf17-997e-4005-8974-602ff855f708?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f62fbf17-997e-4005-8974-602ff855f708?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f62fbf17-997e-4005-8974-602ff855f708?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f62fbf17-997e-4005-8974-602ff855f708)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0fb1118f-ce5a-456e-a159-f81816172fe9 -->
[](0fb1118f-ce5a-456e-a159-f81816172fe9)
--
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]
