bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3585470176
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Lost data-test attribute on delete button</b></div>
<div id="fix">
The `data-test="dashboard-list-trash-icon"` attribute is now lost — it's
inside the `icon` prop but not passed to ActionButton's `dataTest` prop. The
rendered button will have `data-test="Delete"` instead. This breaks existing
tests in DashboardList, ChartList, and RowLevelSecurityList that query by
`dashboard-list-trash-icon`. Pass `dataTest="dashboard-list-trash-icon"` to
ActionButton and remove `data-test` from the icon.
</div>
</div>
<small><i>Code Review Run #356164</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/MetricControl/AdhocMetricEditPopoverTitle.tsx:
##########
@@ -113,24 +113,30 @@ const AdhocMetricEditPopoverTitle:
FC<AdhocMetricEditPopoverTitleProps> = ({
return (
<Tooltip placement="top" title={t('Click to edit label')}>
- <span
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing pointer cursor on button</b></div>
<div id="fix">
The converted `<button>` lacks `cursor: pointer`, so users won't see the
pointer cursor when hovering. Other interactive elements in
`AdhocMetricEditPopover/index.tsx` (line 503) already have this styling. Add
`cursor: pointer;` to the css block.
</div>
</div>
<small><i>Code Review Run #356164</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/ListView/ListView.tsx:
##########
@@ -184,10 +184,15 @@ const ViewModeContainer = styled.div`
display: inline-block;
.toggle-button {
+ appearance: none;
+ border: none;
+ background: none;
+ font: inherit;
display: inline-block;
border-radius: ${theme.borderRadius}px;
padding: ${theme.sizeUnit}px;
padding-bottom: ${theme.sizeUnit * 0.5}px;
+ cursor: pointer;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in CSS</b></div>
<div id="fix">
Extract the shared reset CSS properties (appearance, border, background,
font, cursor) into the `inlineTextButtonCss` constant and apply it in the
`.toggle-button` styled rules to avoid duplication and ensure maintenance is
centralized.
</div>
</div>
<small><i>Code Review Run #356164</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/features/charts/ChartCard.tsx:
##########
@@ -103,9 +115,9 @@ export default function ChartCard({
menuItems.push({
key: 'export',
label: (
- <div
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={menuItemButtonCss}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test attribute on export button</b></div>
<div id="fix">
The export button lacks a `data-test` attribute unlike the edit
(`chart-list-edit-option`) and delete (`chart-list-delete-option`) buttons in
the same component. Without this attribute, automated tests cannot interact
with the export action in card view.
</div>
</div>
<small><i>Code Review Run #356164</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/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
)}
</ConfirmStatusChange>
)}
{canEdit && (
- <Tooltip
- id="edit-action-tooltip"
- title={t('Edit')}
+ <ActionButton
+ label={t('Edit')}
+ tooltip={t('Edit')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={handleEdit}
- >
+ icon={
<Icons.EditOutlined data-test="edit-alt" iconSize="l" />
- </span>
- </Tooltip>
+ }
+ onClick={handleEdit}
+ />
)}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Lost data-test attribute on edit button</b></div>
<div id="fix">
The `data-test="edit-alt"` attribute is now lost on the edit button —
ActionButton defaults to `label` for `data-test` when `dataTest` is not
provided, resulting in `data-test="Edit"` instead. Existing tests in
DashboardList, ChartList, RowLevelSecurityList, and ChartList.permissions query
by `edit-alt`. Pass `dataTest="edit-alt"` to ActionButton.
</div>
</div>
<small><i>Code Review Run #356164</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/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>
---
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/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -52,12 +68,12 @@ export const ActionButton = ({
}
}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Missing disabled visual styling</b></div>
<div id="fix">
No visual feedback for disabled state. The button should show reduced
opacity and not-allowed cursor when disabled, matching the pattern in
`DataTableControl/index.tsx:87-88`. (See also:
[CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #356164</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]
