bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652241087
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx:
##########
@@ -191,19 +190,25 @@ const FilterTitleContainer = forwardRef<HTMLDivElement,
Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Keyboard accessibility gap</b></div>
<div id="fix">
The removed code at lines 195-203 provided explicit Enter/Space key handling
via `handleKeyboardActivation`. While the native `<button>` element should
handle keyboard activation, the existing test at
FiltersConfigModal.test.tsx:1036 only verifies mouse clicks, not keyboard
events. Add keyboard interaction test to ensure the undo button remains
operable via keyboard.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx:
##########
@@ -184,30 +180,31 @@ export const DataTablesPane = ({
) : (
<Icons.DownOutlined aria-label={t('Expand data panel')} />
);
+ const resetButtonCss = css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ `;
return (
<div>
{panelOpen ? (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(false),
- )}
>
{caretIcon}
- </span>
+ </button>
) : (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(true),
- )}
>
{caretIcon}
- </span>
+ </button>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in conditional buttons</b></div>
<div id="fix">
Both buttons share identical `type`, `css`, and `onClick` props—only the
`handleCollapseChange` argument differs. Future style changes require updating
two locations, increasing maintenance burden and divergence risk. Extract the
shared markup so the conditional only controls the icon/aria-label.
</div>
</div>
<small><i>Code Review Run #68a8a8</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>CWE-1004: Missing aria-label on buttons</b></div>
<div id="fix">
Screen readers will announce these as generic 'button' instead of 'Collapse
data panel' or 'Expand data panel'. The icon inside carries the proper
aria-label but the button wrapping it does not, degrading accessibility for
keyboard and screen reader users. (See also:
[CWE-1004](https://cwe.mitre.org/data/definitions/1004.html))
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx:
##########
@@ -344,29 +349,20 @@ const Thumbnail: FC<ThumbnailProps> = ({
const { key, value: type } = entry;
const isSelected = selectedViz === entry.key;
- const handleKeyDown = (event: React.KeyboardEvent) => {
- if (event.key === 'Enter' || event.key === ' ') {
- event.preventDefault();
- setSelectedViz(key);
- }
- };
-
const handleFocus = () => {
// Auto-select chart when tabbed to
setSelectedViz(key);
};
return (
- <div
- role="button"
+ <button
+ type="button"
// using css instead of a styled component to preserve
// the data-test attribute
css={thumbnailContainerCss(theme)}
- tabIndex={0}
className={isSelected ? 'selected' : ''}
onClick={() => setSelectedViz(key)}
onDoubleClick={onDoubleClick}
- onKeyDown={handleKeyDown}
onFocus={handleFocus}
data-test="viztype-selector-container"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing aria-pressed on toggle button</b></div>
<div id="fix">
The converted button element lacks `aria-pressed={isSelected}` to indicate
its selected/toggle state to assistive technologies. This is a critical
accessibility attribute for toggle buttons per WCAG 4.1.2 (Name, Role, Value).
Without it, screen reader users cannot determine which viz type is currently
selected.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -159,19 +158,25 @@ const ItemTitleContainer = forwardRef<HTMLDivElement,
Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
data-test="undo-button"
- tabIndex={0}
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
- onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Keyboard repeat handler missing</b></div>
<div id="fix">
The new `<button>` is missing the `onKeyDown` handler that prevented
duplicate callbacks on key hold. The original `<span role="button">` had
`handleKeyboardActivation(() => restoreItem(id))` which checked `event.repeat`
to ignore auto-repeat keydown events. Without this, holding Enter/Space fires
multiple undo calls in rapid succession. Re-add the import and handler to match
the original behavior.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControlShared.tsx:
##########
@@ -236,6 +236,10 @@ export const DescriptionToolTip = ({
whiteSpace: 'normal',
}}
>
+ {/* Deliberate role="button" on this tooltip-trigger icon (no click
+ handler) — covered by existing test expectations; not a fit for
+ the linter's suggested <button> tag. */}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>False claim about test coverage</b></div>
<div id="fix">
The comment at lines 239-241 claims this pattern is 'covered by existing
test expectations,' but `DescriptionToolTip` has no tests verifying
`role="button"` — the test fixture uses `description: ''`, so the tooltip never
renders. Only `DeckglLayerVisibilityTooltip` has tests at lines 22-34 of
`FilterControlShared.test.tsx` that check for a button role. Remove the
misleading claim to prevent future developers from assuming test coverage.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx:
##########
@@ -104,24 +110,13 @@ const UrlFiltersVerticalCollapse = (props: {
return (
<div css={sectionContainerStyle}>
- <div
- css={sectionHeaderStyle}
- onClick={toggleSection}
- onKeyDown={e => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- toggleSection();
- }
- }}
- role="button"
- tabIndex={0}
- >
+ <button type="button" css={sectionHeaderStyle} onClick={toggleSection}>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing aria-expanded on toggle</b></div>
<div id="fix">
The toggle button now lacks `aria-expanded` to indicate its current state to
screen readers. Since this button controls visibility of content below it,
adding `aria-expanded={isOpen}` will enable users with assistive technologies
to understand whether the section is currently expanded or collapsed.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.test.tsx:
##########
@@ -26,6 +26,7 @@ jest.mock(
'@superset-ui/core/components/Icons/AsyncIcon',
() =>
({ fileName }: { fileName: string }) => (
+ // eslint-disable-next-line jsx-a11y/prefer-tag-over-role -- mirrors
AsyncIcon's real span+role="img" shape
<span role="img" aria-label={fileName.replace('_', '-')} />
),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Remove non-existent ESLint disable comment</b></div>
<div id="fix">
The eslint-disable-next-line comment references a rule
'jsx-a11y/prefer-tag-over-role' that is not defined in the project's ESLint
configuration, causing the linter to fail. Remove this comment.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
() =>
({ fileName }: { fileName: string }) => (
<span role="img" aria-label={fileName.replace('_', '-')} />
),
````
</div>
</details>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
