bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3585470202
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -35,14 +39,26 @@ export const ActionButton = ({
placement,
icon,
onClick,
+ className,
+ disabled = false,
+ dataTest,
}: ActionProps) => {
const theme = useTheme();
const actionButton = (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ aria-disabled={disabled}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-284: Missing tabIndex on disabled button</b></div>
<div id="fix">
Missing `tabIndex` prop for disabled state. When disabled, the button should
have `tabIndex={-1}` to prevent keyboard focus, consistent with the pattern
used in `DataTableControl/index.tsx:72` and `CopyToClipboard/index.tsx:124`.
(See also: [CWE-284](https://cwe.mitre.org/data/definitions/284.html))
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Missing native disabled attribute</b></div>
<div id="fix">
The button uses `aria-disabled={disabled}` but lacks the native HTML
`disabled` attribute. While `aria-disabled` conveys state to assistive tech,
the native `disabled` attribute is required to prevent keyboard activation
(Enter/Space) and show browser-native disabled styling. Without it, disabled
buttons remain keyboard-operable and clickable.
</div>
</div>
<small><i>Code Review Run #68a8a8</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>
---
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/DatasourcePanel/index.tsx:
##########
@@ -291,17 +286,21 @@ export default function DataSourcePanel({
message=""
description={
<>
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
onClick={() => setShowSaveDatasetModal(true)}
- onKeyDown={handleKeyboardActivation(() =>
- setShowSaveDatasetModal(true),
- )}
className="add-dataset-alert-description"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ cursor: pointer;
+ `}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1004: Missing focus styles</b></div>
<div id="fix">
The removed `handleKeyboardActivation` handler provided explicit Enter/Space
key handling with `event.preventDefault()` and `event.repeat` protection.
Native `<button>` handles Enter/Space automatically, but no focus styles are
provided for keyboard users who tab to this element.
([CWE-1004](https://cwe.mitre.org/data/definitions/1004.html))
</div>
</div>
<small><i>Code Review Run #68a8a8</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]
