codeant-ai-for-open-source[bot] commented on code in PR #38342:
URL: https://github.com/apache/superset/pull/38342#discussion_r2930908265
##########
superset-frontend/src/assets/staticPages/404.html:
##########
@@ -67,7 +67,7 @@
text-align: center;
text-decoration: none;
display: inline-block;
- font-size: 11px;
+ font-size: 0.6875rem; /* WCAG 1.4.4 */
Review Comment:
**Suggestion:** Setting the button text to `0.6875rem` (~11px) makes the
primary action label too small and regresses readability in an
accessibility-focused change. Increase it to a normal readable size to avoid
creating a low-vision usability issue. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Primary "Back to home" action text is tiny.
- ⚠️ Low-vision users struggle recovering from 404 pages.
```
</details>
```suggestion
font-size: 1rem; /* WCAG 1.4.4 */
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Frontend build generates `404.html` from
`superset-frontend/src/assets/staticPages/404.html`
(`superset-frontend/webpack.config.js:161-164`).
2. Trigger a 404 in browser (`Accept: text/html`) while app is not in debug
mode; backend
serves this static page from `show_http_exception`
(`superset/views/error_handling.py:161-170`).
3. Inspect the primary CTA `<a class="button">Back to home</a>`
(`superset-frontend/src/assets/staticPages/404.html:100`) with `.button`
font-size defined
as `0.6875rem` at line 70.
4. The CTA label renders around 11px equivalent, making the main recovery
action hard to
read; changing to `1rem` improves legibility directly on this user path.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/assets/staticPages/404.html
**Line:** 70:70
**Comment:**
*Possible Bug: Setting the button text to `0.6875rem` (~11px) makes the
primary action label too small and regresses readability in an
accessibility-focused change. Increase it to a normal readable size to avoid
creating a low-vision usability issue.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=55a7b5bd358f5c276f9eb667bf86ce2acc71db005c20b5e23380c47f7218d09d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=55a7b5bd358f5c276f9eb667bf86ce2acc71db005c20b5e23380c47f7218d09d&reaction=dislike'>👎</a>
##########
superset-frontend/src/assets/staticPages/500.html:
##########
@@ -66,7 +66,7 @@
text-align: center;
text-decoration: none;
display: inline-block;
- font-size: 11px;
+ font-size: 0.6875rem; /* WCAG 1.4.4 */
Review Comment:
**Suggestion:** The new button text size is set to `0.6875rem` (about 11px),
which makes the primary navigation action hard to read and undermines the
intended accessibility improvement. Increase it to a normal readable size so
users can reliably see and activate the recovery action. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ 500-page recovery CTA readability is significantly reduced.
- ⚠️ HTML error flows hinder quick return-home navigation.
```
</details>
```suggestion
font-size: 1rem; /* WCAG 1.4.4 */
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Superset with `DEBUG=False`, then trigger any backend unhandled
exception in an
HTML request flow; Flask routes this through `show_unexpected_exception` at
`superset/views/error_handling.py:215-223`.
2. The error handler serves the built static page `static/assets/500.html`
via
`send_file(...)` at `superset/views/error_handling.py:222-223` (same
behavior also exists
for command exceptions at `superset/views/error_handling.py:194-199`).
3. On the rendered 500 page, the only recovery CTA (`<a href="/"
class="button">Back to
home</a>`) is styled by `.button` in
`superset-frontend/src/assets/staticPages/500.html:58-72`.
4. The CTA text is explicitly forced to `font-size: 0.6875rem` at
`superset-frontend/src/assets/staticPages/500.html:69` (~11px at default
root size),
making the primary recovery action hard to read on a failure screen.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/assets/staticPages/500.html
**Line:** 69:69
**Comment:**
*Logic Error: The new button text size is set to `0.6875rem` (about
11px), which makes the primary navigation action hard to read and undermines
the intended accessibility improvement. Increase it to a normal readable size
so users can reliably see and activate the recovery action.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=6801ee769ee843211598edf48b0cfa89a03059ff92b772424b13c58d8dd6e8ff&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=6801ee769ee843211598edf48b0cfa89a03059ff92b772424b13c58d8dd6e8ff&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -281,6 +318,21 @@ function Echart(
useEffect(() => () => chartRef.current?.dispose(), []);
+ // WCAG 1.4.13: Dismiss ECharts tooltip on Escape key
+ useEffect(() => {
+ const el = divRef.current;
+ if (!el) return undefined;
+
+ const handleKeyDown = (e: Event) => {
+ if ((e as globalThis.KeyboardEvent).key === 'Escape' &&
chartRef.current) {
+ chartRef.current.dispatchAction({ type: 'hideTip' });
+ }
+ };
+
+ el.addEventListener('keydown', handleKeyDown);
+ return () => el.removeEventListener('keydown', handleKeyDown);
+ }, [didMount]);
Review Comment:
**Suggestion:** The Escape handler is attached to the chart container
element, but that element is not focusable, so it usually never receives
`keydown` events. As a result, the tooltip dismissal behavior will not work for
keyboard users. Attach the listener to `document` (or make the container
focusable) so Escape is captured reliably. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Escape tooltip dismissal fails for keyboard navigation.
- ⚠️ WCAG 1.4.13 keyboard dismissal behavior is unreliable.
- ❌ Shared Echart affects many chart visualization types.
```
</details>
```suggestion
const handleKeyDown = (e: KeyboardEvent) => {
if (e.key === 'Escape' && chartRef.current) {
chartRef.current.dispatchAction({ type: 'hideTip' });
}
};
document.addEventListener('keydown', handleKeyDown);
return () => document.removeEventListener('keydown', handleKeyDown);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any ECharts visualization in Superset (plugin registration is in
`superset-frontend/src/visualizations/presets/MainPreset.ts:151-153` for
Timeseries and
similarly other ECharts types).
2. This renders `EchartsTimeseries`
(`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:365`)
which mounts shared `Echart` component.
3. In `Echart`, Escape listener is bound to `divRef.current` via
`el.addEventListener('keydown', ...)` (`.../components/Echart.tsx:332`), but
the rendered
container only has `ref/height/width/role/aria-label`
(`.../Echart.tsx:363-367`) and no
focusability attribute (`tabIndex`).
4. Hover chart to show tooltip, then press Escape while normal page focus
remains outside
that non-focusable container; `dispatchAction({ type: 'hideTip' })` in key
handler
(`.../Echart.tsx:327-329`) does not run, so tooltip is not dismissed via
keyboard.
```
</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-echarts/src/components/Echart.tsx
**Line:** 323:334
**Comment:**
*Logic Error: The Escape handler is attached to the chart container
element, but that element is not focusable, so it usually never receives
`keydown` events. As a result, the tooltip dismissal behavior will not work for
keyboard users. Attach the listener to `document` (or make the container
focusable) so Escape is captured reliably.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=256704a3dc9291bbb1a93a05406a95bdcccb5b32dab343db36c079bc3abc70e1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=256704a3dc9291bbb1a93a05406a95bdcccb5b32dab343db36c079bc3abc70e1&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-core/src/ui/theme/GlobalStyles.tsx:
##########
@@ -98,6 +104,85 @@ export const GlobalStyles = () => {
[role='button'] {
cursor: pointer;
}
+
+ /* WCAG 1.4.11: Non-text contrast — form field borders need 3:1
against background.
+ Ant Design default colorBorder (#d9d9d9) is only ~1.34:1 on white.
+ Using colorTextTertiary (~#8c8c8c, ~3.54:1 on white) for all input
borders. */
+ .ant-input,
+ .ant-input-affix-wrapper,
+ .ant-select:not(.ant-select-customize-input) .ant-select-selector,
+ .ant-picker,
+ .ant-input-number,
+ .ant-input-number-group-wrapper .ant-input-number {
+ border-color: ${theme.colorTextTertiary};
+ }
+
+ /* Ensure disabled inputs retain a visible (though lighter) border */
+ .ant-input[disabled],
+ .ant-select-disabled .ant-select-selector,
+ .ant-picker-disabled,
+ .ant-input-number-disabled {
+ border-color: ${theme.colorTextQuaternary};
+ }
+
+ /* WCAG 1.4.11: Checkbox/Radio/Switch borders need 3:1 against
background.
+ Ant Design default uses colorBorder (~1.34:1 on white). Override to
+ colorTextTertiary (~3.54:1) for unchecked state borders. */
+ .ant-checkbox .ant-checkbox-inner {
+ border-color: ${theme.colorTextTertiary};
+ }
+ .ant-radio .ant-radio-inner {
+ border-color: ${theme.colorTextTertiary};
+ }
+ .ant-switch {
+ background-color: ${theme.colorTextTertiary};
+ }
+
+ /* WCAG 1.4.11: Divider/separator contrast — colorSplit (~#f0f0f0) is
~1.04:1 on white.
+ Override to colorTextTertiary (~3.54:1). */
+ .ant-divider {
+ border-color: ${theme.colorTextTertiary};
+ }
+
+ /* WCAG 1.4.11: Table borders and progress bars */
+ .ant-table-bordered .ant-table-cell {
+ border-color: ${theme.colorTextQuaternary};
+ }
+ .ant-progress-bg {
+ min-width: 2px;
+ }
Review Comment:
**Suggestion:** Setting a global `min-width: 2px` on progress fills makes 0%
progress bars render as partially filled, which misrepresents actual progress
in places like SQL Lab and export status. Keep the minimum width at 0 (or only
apply a minimum for non-zero widths) so a true zero value remains visually
empty. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL Lab query progress initially appears non-zero.
- ⚠️ Export modal progress can look falsely started.
- ⚠️ User trust drops from inaccurate progress visuals.
```
</details>
```suggestion
.ant-progress-bg {
min-width: 0;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load any Superset page; `SupersetThemeProvider` always mounts
`<GlobalStyles />` at
`superset-frontend/packages/superset-core/src/ui/theme/Theme.tsx:187`, so
`.ant-progress-bg { min-width: 2px; }` is globally active from
`GlobalStyles.tsx:151-153`.
2. Open SQL Lab and run a query; query bootstrap sets `progress: 0` in
`superset-frontend/src/SqlLab/actions/sqlLab.ts:400`.
3. SQL Lab query log renders progress using `<ProgressBar ...
percent={parseInt(progress.toFixed(0), 10)} />` at
`superset-frontend/src/SqlLab/components/QueryTable/index.tsx:325` and
`:331`, so zero
percent reaches Ant Progress.
4. Because the global rule enforces `min-width: 2px`
(`GlobalStyles.tsx:152`), the fill
element `.ant-progress-bg` cannot be visually empty at 0%, so users see a
misleading
non-zero bar.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-core/src/ui/theme/GlobalStyles.tsx
**Line:** 151:153
**Comment:**
*Logic Error: Setting a global `min-width: 2px` on progress fills makes
0% progress bars render as partially filled, which misrepresents actual
progress in places like SQL Lab and export status. Keep the minimum width at 0
(or only apply a minimum for non-zero widths) so a true zero value remains
visually empty.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=38ffc64346adcd4e5154afe69053319b501e12f252db68348030e7bc9a0244cb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=38ffc64346adcd4e5154afe69053319b501e12f252db68348030e7bc9a0244cb&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Popover/index.tsx:
##########
@@ -16,11 +16,56 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { useCallback, useEffect, useState } from 'react';
import { Popover as AntdPopover } from 'antd';
import { PopoverProps as AntdPopoverProps } from 'antd/es/popover';
export interface PopoverProps extends AntdPopoverProps {
forceRender?: boolean;
}
-export const Popover = (props: PopoverProps) => <AntdPopover {...props} />;
+/**
+ * WCAG 1.4.13 compliant Popover wrapper.
+ *
+ * - Dismissable: pressing Escape closes the popover.
+ * - Hoverable: Ant Design 5 popovers already keep the overlay visible
+ * while the pointer is over it.
+ */
+export const Popover = ({ open, onOpenChange, ...props }: PopoverProps) => {
+ const [visible, setVisible] = useState(false);
+
+ const isControlled = open !== undefined;
+ const isVisible = isControlled ? open : visible;
+
+ const handleOpenChange = useCallback(
+ (newOpen: boolean) => {
+ if (!isControlled) {
+ setVisible(newOpen);
+ }
+ onOpenChange?.(newOpen);
+ },
+ [isControlled, onOpenChange],
Review Comment:
**Suggestion:** The wrapper now only treats `open/onOpenChange` as the
control API, but existing callers in this codebase still use
`visible/onVisibleChange`. Because `open` is always passed to `AntdPopover`,
those legacy controlled popovers stop syncing with parent state (and
`defaultOpen` is also ignored), causing incorrect open/close behavior. Support
both APIs and initialize internal state from `defaultOpen`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ `/tasks/list/` popover visibility callbacks stop syncing.
- ⚠️ Legacy `visible/onVisibleChange` Popover contract regresses.
- ⚠️ Uncontrolled `defaultOpen` initialization currently ignored.
```
</details>
```suggestion
export const Popover = ({
open,
visible: legacyVisible,
defaultOpen,
onOpenChange,
onVisibleChange,
...props
}: PopoverProps) => {
const [visible, setVisible] = useState(Boolean(defaultOpen));
const isControlled = open !== undefined || legacyVisible !== undefined;
const isVisible = open ?? legacyVisible ?? visible;
const handleOpenChange = useCallback(
(newOpen: boolean) => {
if (!isControlled) {
setVisible(newOpen);
}
onOpenChange?.(newOpen);
onVisibleChange?.(newOpen);
},
[isControlled, onOpenChange, onVisibleChange],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open route `/tasks/list/` (registered at
`superset-frontend/src/views/routes.tsx:306-307`), with Global Task
Framework enabled
(`src/pages/TaskList/index.tsx:81`).
2. In Task list "Details" column, rows render `<TaskPayloadPopover />` and
`<TaskStackTracePopover />` (`src/pages/TaskList/index.tsx:428-430`).
3. Both components call `Popover` from `@superset-ui/core/components` using
legacy props
`visible` + `onVisibleChange`
(`src/features/tasks/TaskPayloadPopover.tsx:72-77`,
`src/features/tasks/TaskStackTracePopover.tsx:133-138`).
4. Wrapper at
`packages/superset-ui-core/src/components/Popover/index.tsx:34-47` ignores
`onVisibleChange` and only fires `onOpenChange`, while forcing
`open={isVisible}` at line
66; parent `visible` state never syncs, so legacy-controlled behavior is
broken.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Popover/index.tsx
**Line:** 34:47
**Comment:**
*Logic Error: The wrapper now only treats `open/onOpenChange` as the
control API, but existing callers in this codebase still use
`visible/onVisibleChange`. Because `open` is always passed to `AntdPopover`,
those legacy controlled popovers stop syncing with parent state (and
`defaultOpen` is also ignored), causing incorrect open/close behavior. Support
both APIs and initialize internal state from `defaultOpen`.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=1e3b05955217ff09bcd4cc18406f5a45a23be2e5f918d46f28b3273872a6d918&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38342&comment_hash=1e3b05955217ff09bcd4cc18406f5a45a23be2e5f918d46f28b3273872a6d918&reaction=dislike'>👎</a>
--
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]
