korbit-ai[bot] commented on code in PR #32514:
URL: https://github.com/apache/superset/pull/32514#discussion_r1981743085
##########
superset-frontend/src/components/Select/styles.tsx:
##########
@@ -81,11 +85,16 @@ export const NoElement = styled.span`
`;
export const StyledTag = styled(Tag)`
- ${({ theme }) => `
- background: ${theme.colors.grayscale.light3};
- font-size: ${theme.typography.sizes.m}px;
- border: none;
- `}
+ & .antd5-tag-close-icon {
+ display: inline-flex;
+ align-items: center;
+ margin-left: ${({ theme }) => theme.gridUnit}px;
+ }
+
+ & .tag-content {
+ overflow: hidden;
+ text-overflow: ellipsis;
+ }
Review Comment:
### Ineffective text truncation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The CSS overflow and text-overflow properties are applied without a defined
width or max-width constraint on the tag-content element.
###### Why this matters
Without a width constraint, the ellipsis truncation won't function as
intended, causing unnecessary DOM layout calculations and potential layout
shifts during rendering.
###### Suggested change ∙ *Feature Preview*
Add a width or max-width constraint to ensure proper text truncation:
```css
& .tag-content {
max-width: 100%;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7337d88d-8b25-4990-aa61-266f0203010c?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:768c2d1c-79f9-4d90-97d4-57790f412be2 -->
##########
superset-frontend/src/components/Select/CustomTag.tsx:
##########
@@ -69,7 +55,7 @@ export const customTagRender = (props: CustomTagProps) => {
target.tagName === 'svg' ||
target.tagName === 'path' ||
(target.tagName === 'span' &&
- target.className.includes('ant-tag-close-icon'))
+ target.className.includes('antd5-tag-close-icon'))
Review Comment:
### Incorrect class name for tag close icon <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The class name check for the close icon is incorrect. Ant Design v5 uses
'ant-tag-close-icon' class, not 'antd5-tag-close-icon'.
###### Why this matters
This will cause the click propagation prevention to fail when clicking the
close icon, leading to unexpected behavior where the dropdown opens when
attempting to remove a tag.
###### Suggested change ∙ *Feature Preview*
Update the class name check to use the correct Ant Design v5 class:
```typescript
target.className.includes('ant-tag-close-icon')
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87f89cf9-6123-462a-8a57-6ed54820fee2?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:ed819f84-a51b-465b-aa28-98580d605891 -->
##########
superset-frontend/src/explore/components/controls/SelectAsyncControl/index.tsx:
##########
@@ -97,7 +97,7 @@ const SelectAsyncControl = ({
})
.then(response => {
const data = mutator
- ? mutator(response.json, value)
+ ? mutator(response.json, value!)
: response.json.result;
Review Comment:
### Unsafe non-null assertion in mutator call <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The non-null assertion operator (!) on `value` parameter in the mutator
function call could lead to runtime errors if value is undefined.
###### Why this matters
If `value` is undefined during runtime, forcing it to be treated as non-null
could cause the mutator function to fail unexpectedly, potentially breaking the
data loading functionality.
###### Suggested change ∙ *Feature Preview*
Replace the non-null assertion with safe handling of the potentially
undefined value:
```typescript
? mutator(response.json, value)
: response.json.result;
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a0d4edd-6aa2-4403-97eb-b6e63374a419?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:a8cf5238-839f-4c2a-9ba6-50372ec05e6c -->
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -291,14 +215,17 @@ const Select = forwardRef(
setSelectValue(undefined);
} else {
setSelectValue(
- fullSelectOptions
+ options
.filter(
option => option.disabled && hasOption(option.value,
selectValue),
)
.map(option =>
labelInValue
- ? { label: option.label, value: option.value }
- : option.value,
+ ? ({
+ label: option.label,
+ value: option.value,
+ } as unknown as RawValue) // TODO: @msyavuz Type these
properly
+ : (option.valu as unknown as RawValue),
Review Comment:
### Typo in property access causing undefined value <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
There's a typo in the property name 'valu' instead of 'value' which will
return undefined and cause incorrect behavior.
###### Why this matters
This will result in undefined values being set when clearing selections for
disabled options, potentially corrupting the component's state.
###### Suggested change ∙ *Feature Preview*
Correct the property name from 'valu' to 'value':
```typescript
option.value as unknown as RawValue
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c71268bf-2e7f-4822-9e22-0959ad36e0da?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:cfae05c1-6b3e-4e35-a2b0-40d9978c0365 -->
--
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]
