codeant-ai-for-open-source[bot] commented on code in PR #37167:
URL: https://github.com/apache/superset/pull/37167#discussion_r2694632438
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx:
##########
@@ -106,12 +117,22 @@ const DashboardWrapper: React.FC<Props> = ({ children })
=> {
useEffect(() => {
const monitor = dragDropManager.getMonitor();
+ const debouncedSetIsDragged = debounce(setIsDragged, FAST_DEBOUNCE);
Review Comment:
**Suggestion:** Using `debounce(setIsDragged, FAST_DEBOUNCE)` without the
`leading` option delays setting the drag state until events stop (debounce
fires after the last call), which likely makes `isDragged` become true only
after dragging stops; use debounce with `{ leading: true, trailing: false }` or
use `throttle` so the state is applied immediately when dragging starts. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Drag start feedback delayed in dashboard editor.
- ⚠️ Drop-zone highlighting behaves incorrectly.
```
</details>
```suggestion
const debouncedSetIsDragged = debounce(
setIsDragged,
FAST_DEBOUNCE,
{ leading: true, trailing: false },
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard in edit mode so DashboardWrapper mounts; the relevant
logic is in
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx
(useEffect subscribeToStateChange block around lines 117-131).
2. Begin a drag operation on a dashboard component. The drag monitor's
subscribeToStateChange callback (monitor.isDragging() check at ~lines
121-129) calls
`debouncedSetIsDragged(true)` when dragging starts.
3. Because debounce was created without the `leading` option
(`debounce(setIsDragged,
FAST_DEBOUNCE)` at line 120), the call to setIsDragged(true) is scheduled
after
FAST_DEBOUNCE elapses (i.e., after events stop), so isDragged becomes true
only after
dragging stops or after the debounce delay.
4. Observe that the UI feedback that depends on isDragged (class
'dragdroppable--dragging'
applied in the JSX return of DashboardWrapper) is not applied immediately at
drag start —
drop-zone highlighting is delayed or inverted. Using `{ leading: true,
trailing: false }`
or throttle makes the state flip immediately on drag start, reproducing the
intended
immediate highlighting behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx
**Line:** 120:120
**Comment:**
*Logic Error: Using `debounce(setIsDragged, FAST_DEBOUNCE)` without the
`leading` option delays setting the drag state until events stop (debounce
fires after the last call), which likely makes `isDragged` become true only
after dragging stops; use debounce with `{ leading: true, trailing: false }` or
use `throttle` so the state is applied immediately when dragging starts.
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/src/components/Accessibility/SkipLink.tsx:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { styled } from '@superset-ui/core';
+
+/**
+ * SkipLink - WCAG 2.4.1 Bypass Blocks
+ * Allows keyboard users to skip navigation and jump directly to main content.
+ * The link is visually hidden but becomes visible when focused.
+ */
+
+const StyledSkipLink = styled.a`
+ position: absolute;
+ top: -100px;
+ left: 0;
+ background: ${({ theme }) => theme.colors.primary.dark1};
+ color: ${({ theme }) => theme.colors.grayscale.light5};
+ padding: ${({ theme }) => theme.gridUnit * 3}px ${({ theme }) =>
theme.gridUnit * 4}px;
+ z-index: 10000;
+ text-decoration: none;
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ font-size: ${({ theme }) => theme.typography.sizes.m}px;
+ border-radius: 0 0 ${({ theme }) => theme.borderRadius}px ${({ theme }) =>
theme.borderRadius}px;
+ transition: top 0.2s ease-in-out;
+
+ &:focus,
+ &:focus-visible {
+ top: 0 !important;
+ outline: 3px solid ${({ theme }) => theme.colors.primary.light1};
+ outline-offset: 2px;
+ }
+
+ &:hover {
+ background: ${({ theme }) => theme.colors.primary.base};
+ }
+`;
+
+interface SkipLinkProps {
+ targetId?: string;
+ children?: React.ReactNode;
+}
+
+const SkipLink: React.FC<SkipLinkProps> = ({
+ targetId = 'main-content',
+ children = 'Skip to main content',
+}) => (
+ <StyledSkipLink href={`#${targetId}`} className="a11y-skip-link">
+ {children}
+ </StyledSkipLink>
+);
Review Comment:
**Suggestion:** Fragment navigation alone does not reliably move keyboard
focus to the target element for assistive technologies; clicking the link
should programmatically set focus to the target element (and temporarily make
it focusable if needed) to ensure screen reader and keyboard users are moved to
the main content. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Screen reader users not moved to main content
- ❌ Keyboard users lose context after activating skip link
- ⚠️ Affects dashboard navigation flow for assistive tech
```
</details>
```suggestion
}) => {
const handleClick = (e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
const el = document.getElementById(targetId);
if (el && el instanceof HTMLElement) {
// Make sure the element is focusable, focus it, then clean up any
temporary tabindex
const hadTabIndex = el.hasAttribute('tabindex');
if (!hadTabIndex) {
el.setAttribute('tabindex', '-1');
}
el.focus();
if (!hadTabIndex) {
el.removeAttribute('tabindex');
}
} else {
// Fallback to fragment navigation if target not present
window.location.hash = `#${targetId}`;
}
};
return (
<StyledSkipLink href={`#${targetId}`} className="a11y-skip-link"
onClick={handleClick}>
{children}
</StyledSkipLink>
);
};
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard (PR testing instructions: "Open any dashboard"). The
SkipLink
component is declared at
`superset-frontend/src/components/Accessibility/SkipLink.tsx:59-66`.
2. Press Tab to reveal and focus the "Skip to main content" link (the link
is generated at
`SkipLink.tsx:63` with an href fragment `#main-content`).
3. Activate the link with Enter or Space. Current code relies solely on
fragment
navigation (`href="#main-content"`) and does not programmatically set focus
on the target
element.
4. Observe with a screen reader or accessibility inspector: fragment
navigation changes
the URL fragment but does not reliably move keyboard focus to the main
content element.
Because `SkipLink` lacks a click handler to call `element.focus()`, users of
assistive
tech may not be placed into the main content even though the page scrolled
there.
5. The improved code (adds onClick that finds
`document.getElementById(targetId)` at
runtime and focuses it) reproduces the desired fix and ensures the main
content receives
keyboard focus after activation.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Accessibility/SkipLink.tsx
**Line:** 62:66
**Comment:**
*Possible Bug: Fragment navigation alone does not reliably move
keyboard focus to the target element for assistive technologies; clicking the
link should programmatically set focus to the target element (and temporarily
make it focusable if needed) to ensure screen reader and keyboard users are
moved to the main content.
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/src/dashboard/components/gridComponents/ChartHolder.tsx:
##########
@@ -25,7 +25,7 @@ import { LayoutItem, RootState } from 'src/dashboard/types';
import AnchorLink from 'src/dashboard/components/AnchorLink';
import Chart from 'src/dashboard/containers/Chart';
import DeleteComponentButton from
'src/dashboard/components/DeleteComponentButton';
-import DragDroppable from 'src/dashboard/components/dnd/DragDroppable';
+import { Draggable } from 'src/dashboard/components/dnd/DragDroppable';
Review Comment:
**Suggestion:** If `DragDroppable` exports a default `Draggable` component
(common pattern), importing it as a named export (`{ Draggable }`) will make
`Draggable` undefined at runtime causing React to throw "Element type is
invalid" or "TypeError: Draggable is not a function" when rendering; switch to
a default import to match the module's export shape or verify the module's
named export. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard charts fail to render when ChartHolder mounts.
- ⚠️ Console shows React element type invalid error.
```
</details>
```suggestion
import Draggable from 'src/dashboard/components/dnd/DragDroppable';
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the frontend is running (start the dev server or build the
frontend).
2. Open any dashboard page so React mounts ChartHolder (component defined at
superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx,
full file
65..333).
3. ChartHolder imports Draggable from DragDroppable (import statement at
ChartHolder.tsx:28).
4. During render ChartHolder instantiates <Draggable ...> (render begins at
ChartHolder.tsx:246..256). If DragDroppable actually uses a default export
rather than a
named export, the named import on line 28 will be undefined and React will
throw at render
time (console shows "Element type is invalid" or "TypeError: Draggable is
not a
function"), preventing charts from rendering.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx
**Line:** 28:28
**Comment:**
*Type Error: If `DragDroppable` exports a default `Draggable` component
(common pattern), importing it as a named export (`{ Draggable }`) will make
`Draggable` undefined at runtime causing React to throw "Element type is
invalid" or "TypeError: Draggable is not a function" when rendering; switch to
a default import to match the module's export shape or verify the module's
named export.
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/packages/superset-ui-core/src/query/api/v1/makeApi.ts:
##########
@@ -115,11 +115,11 @@ export default function makeApi<
jsonPayload: undefined as JsonObject | undefined,
};
if (requestType === 'search') {
- requestConfig.searchParams = payload as URLSearchParams;
+ requestConfig.searchParams = payload as unknown as URLSearchParams;
Review Comment:
**Suggestion:** Unsafe cast: `payload as unknown as URLSearchParams` only
changes TypeScript typing and does not convert the runtime value. If `payload`
is a plain object, array, or primitive, this will set
`requestConfig.searchParams` to a non-URLSearchParams value (or produce
incorrect query encoding), causing runtime errors or incorrect requests.
Convert or construct a proper `URLSearchParams` at runtime and guard for
null/undefined. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ GET APIs using makeApi may send invalid querystrings.
- ❌ SupersetClient.request() may throw serializer errors.
- ⚠️ Dashboard list and search endpoints affected.
```
</details>
```suggestion
if (payload == null) {
requestConfig.searchParams = undefined;
} else if (payload instanceof URLSearchParams) {
requestConfig.searchParams = payload;
} else if (typeof payload === 'string') {
requestConfig.searchParams = new URLSearchParams(payload);
} else if (typeof payload === 'object') {
requestConfig.searchParams = new URLSearchParams(
Object.entries(payload as Record<string, any>).flatMap(([k, v])
=>
Array.isArray(v) ? v.map(item => [k, String(item)]) : [[k,
String(v)]],
),
);
} else {
requestConfig.searchParams = new URLSearchParams(String(payload));
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the file
`superset-frontend/packages/superset-ui-core/src/query/api/v1/makeApi.ts`
and locate the `request` function around the request-type branching (see
line 117: `if
(requestType === 'search') {` and line 118 where `searchParams` is assigned).
2. Create a small test harness inside the repository that imports the
factory: e.g. in a
new file `packages/superset-ui-core/src/query/api/v1/testMakeApi.ts` call:
- import makeApi from '.../makeApi';
- const getApi = makeApi({ endpoint: '/api/v1/test', method: 'GET',
requestType:
'search' });
- await getApi({ foo: 'bar' } as any);
This will exercise `request()` and hit the branch starting at
`makeApi.ts:117`.
3. Run the test harness (ts-node or via the app dev server) so `request()`
calls
`SupersetClient.request()` with the `requestConfig` produced at
`makeApi.ts:116-125`.
Observe the `searchParams` value passed to `SupersetClient.request()` is the
original
plain object (casted via `as unknown as URLSearchParams`) rather than a real
`URLSearchParams` instance.
4. Observe runtime failure modes: either incorrect querystring serialization
or errors in
`SupersetClient.request()` (when it expects URLSearchParams). This
reproduces the issue
concretely by exercising the GET/search branch in `makeApi.ts:117-118`.
```
</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/query/api/v1/makeApi.ts
**Line:** 118:118
**Comment:**
*Type Error: Unsafe cast: `payload as unknown as URLSearchParams` only
changes TypeScript typing and does not convert the runtime value. If `payload`
is a plain object, array, or primitive, this will set
`requestConfig.searchParams` to a non-URLSearchParams value (or produce
incorrect query encoding), causing runtime errors or incorrect requests.
Convert or construct a proper `URLSearchParams` at runtime and guard for
null/undefined.
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/src/components/ListView/ListView.test.tsx:
##########
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { render, waitFor } from 'spec/helpers/testing-library';
+import ListView from './ListView';
+
+const mockedProps = {
+ title: 'Data Table',
+ columns: [
+ {
+ accessor: 'id',
+ Header: 'ID',
+ sortable: true,
+ },
+ {
+ accessor: 'age',
+ Header: 'Age',
+ },
+ {
+ accessor: 'name',
+ Header: 'Name',
+ },
+ {
+ accessor: 'time',
+ Header: 'Time',
+ },
+ ],
+ data: [
+ { id: 1, name: 'data 1', age: 10, time: '2020-11-18T07:53:45.354Z' },
+ { id: 2, name: 'data 2', age: 1, time: '2020-11-18T07:53:45.354Z' },
+ ],
+ count: 2,
+ pageSize: 1,
+ loading: false,
+ refreshData: jest.fn(),
+ addSuccessToast: jest.fn(),
+ addDangerToast: jest.fn(),
+};
+
+test('redirects to first page when page index is invalid', async () => {
+ const fetchData = jest.fn();
+ window.history.pushState({}, '', '/?pageIndex=9');
+ render(<ListView {...mockedProps} fetchData={fetchData} />, {
+ useRouter: true,
+ useQueryParams: true,
+ });
+ await waitFor(() => {
+ expect(window.location.search).toEqual('?pageIndex=0');
+ expect(fetchData).toBeCalledTimes(2);
Review Comment:
**Suggestion:** Wrong Jest matcher used: `toBeCalledTimes` is not a valid
Jest matcher and will throw at runtime; replace it with
`toHaveBeenCalledTimes`. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test file fails, blocking CI runs.
- ⚠️ Test suite aborts before validating behavior.
- ⚠️ Developers get noisy CI errors for ListView tests.
```
</details>
```suggestion
expect(fetchData).toHaveBeenCalledTimes(2);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test suite that includes this file:
superset-frontend/src/components/ListView/ListView.test.tsx. The test named
"redirects to
first page when page index is invalid" starts at the test declaration on
line 56 in the PR
hunk.
2. The test sets up fetchData as a jest mock at line 57 (const fetchData =
jest.fn();) and
renders the ListView component (render(...) call on line 59). Jest executes
the test
function body.
3. When execution reaches the assertion at line 65
(expect(fetchData).toBeCalledTimes(2);)
Jest attempts to call the non-existent matcher toBeCalledTimes and throws a
runtime error
(TypeError: expect(...).toBeCalledTimes is not a function), causing the test
to fail
immediately.
4. Observed outcome: test run fails with a matcher error before verifying
call counts or
subsequent assertions on lines 66-71. Fixing to toHaveBeenCalledTimes
resolves the runtime
failure and allows the rest of the assertions to run.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ListView/ListView.test.tsx
**Line:** 65:65
**Comment:**
*Type Error: Wrong Jest matcher used: `toBeCalledTimes` is not a valid
Jest matcher and will throw at runtime; replace it with `toHaveBeenCalledTimes`.
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]
