codeant-ai-for-open-source[bot] commented on code in PR #36073:
URL: https://github.com/apache/superset/pull/36073#discussion_r2642745052
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -243,12 +242,12 @@ function SearchInput({
inputRef,
}: SearchInputProps) {
return (
- <Space direction="horizontal" size={4} className="dt-global-filter">
+ <Space direction="vertical" size={4} className="dt-global-filter">
{t('Search')}
Review Comment:
**Suggestion:** Accessibility duplication: the visible "Search" label is
rendered as plain text immediately before the input while the input already has
an accessible name via `aria-label`, causing screen readers to announce the
label twice; mark the visible label as presentational (aria-hidden) or
associate it properly with the input via a linked <label> to avoid duplicate
announcements. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<span aria-hidden="true">{t('Search')}</span>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The component currently renders visible text "Search" and also an aria-label
on the input.
Screen readers can end up announcing both the visible text and the
aria-label. Marking the
visible text as aria-hidden (or linking a single label) is an accessible,
minimal fix that
addresses a real duplication problem without breaking behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 246:246
**Comment:**
*Possible Bug: Accessibility duplication: the visible "Search" label is
rendered as plain text immediately before the input while the input already has
an accessible name via `aria-label`, causing screen readers to announce the
label twice; mark the visible label as presentational (aria-hidden) or
associate it properly with the input via a linked <label> to avoid duplicate
announcements.
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.
```
</details>
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx:
##########
@@ -41,10 +36,14 @@ function SearchSelectDropdown({
searchOptions,
}: SearchSelectDropdownProps) {
return (
- <StyledSelect
+ <Select
className="search-select"
+ css={(theme: SupersetTheme) => css`
+ width: ${theme.sizeUnit * 30}px;
+ `}
value={value || (searchOptions?.[0]?.value ?? '')}
Review Comment:
**Suggestion:** Avoid passing an empty string ('') as the Select `value`
fallback — if `searchOptions` is empty this sets the controlled Select value to
'' (a non-existent option) which can cause confusing UI state or
controlled/uncontrolled issues; use nullish coalescing so an undefined value is
passed when nothing is available. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
value={value ?? searchOptions?.[0]?.value}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code forces an empty-string fallback which can be a bad
controlled-value for the Select when no options exist. The proposed
nullish-coalescing approach avoids explicitly injecting '' when both value and
searchOptions are absent and still preserves intentionally empty-string values
from callers. This fixes a plausible UI/controlled-component issue visible in
the diff.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx
**Line:** 44:44
**Comment:**
*Possible Bug: Avoid passing an empty string ('') as the Select `value`
fallback — if `searchOptions` is empty this sets the controlled Select value to
'' (a non-existent option) which can cause confusing UI state or
controlled/uncontrolled issues; use nullish coalescing so an undefined value is
passed when nothing is available.
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.
```
</details>
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -265,18 +264,18 @@ function SelectPageSize({
const { Option } = Select;
return (
- <span className="dt-select-page-size">
+ <Space direction="vertical" size={4} className="dt-select-page-size">
<VisuallyHidden htmlFor="pageSizeSelect">
{t('Select page size')}
</VisuallyHidden>
- {t('Show')}{' '}
+ {t('Entries per page')}
Review Comment:
**Suggestion:** Accessibility/label association bug: code renders a
visually-hidden <label> and a visible text label side-by-side for the page size
select, which results in duplicate labels and may not properly associate the
label with the actual selectable input; replace both with a single label
element linked to the Select (`htmlFor` -> `id`) so screen readers see one
clear label and clicking the label focuses the control. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<label htmlFor="pageSizeSelect">{t('Entries per page')}</label>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The code renders a visually-hidden label (which is a <label>) plus separate
visible text. That can make screen readers read two labels.
Replacing them with a single visible <label htmlFor="pageSizeSelect">Entries
per page</label> both makes the label clickable/focus the Select and avoids
duplicate announcements.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 268:271
**Comment:**
*Possible Bug: Accessibility/label association bug: code renders a
visually-hidden <label> and a visible text label side-by-side for the page size
select, which results in duplicate labels and may not properly associate the
label with the actual selectable input; replace both with a single label
element linked to the Select (`htmlFor` -> `id`) so screen readers see one
clear label and clicking the label focuses the control.
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.
```
</details>
--
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]
