codeant-ai-for-open-source[bot] commented on code in PR #33002:
URL: https://github.com/apache/superset/pull/33002#discussion_r2844751175
##########
superset-frontend/src/components/ListView/ListView.test.jsx:
##########
@@ -26,6 +26,115 @@ import fetchMock from 'fetch-mock';
// Only import components that are directly referenced in tests
import { ListView } from './ListView';
+jest.mock('@superset-ui/core/components', () => ({
+ Alert: ({ children, message, ...props }) => (
+ <div {...props}>{message || children}</div>
+ ),
+ DropdownButton: ({ children, onClick, 'data-test': dataTest, ...props }) => (
+ <button type="button" onClick={onClick} data-test={dataTest} {...props}>
+ {children}
+ </button>
+ ),
+ Icons: {
+ AppstoreOutlined: props => (
+ <span {...props} role="img" aria-label="appstore" />
+ ),
+ UnorderedListOutlined: props => (
+ <span {...props} role="img" aria-label="unordered-list" />
+ ),
+ DeleteOutlined: props => <span {...props} role="img" aria-label="delete"
/>,
+ },
+ EmptyState: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Checkbox: props => <input type="checkbox" {...props} />,
+ Loading: () => <div>Loading...</div>,
+ Flex: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Menu: {
+ Item: ({ children, ...props }) => <div {...props}>{children}</div>,
+ },
Review Comment:
**Suggestion:** The Jest mock for `Menu` returns a plain object instead of a
React component, so when `ListView` renders `<Menu>` in tests it will crash
with an "element type is invalid" error. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ ListView test suite crashes before any assertions run.
- ⚠️ CI cannot validate bulk select/certify ListView behavior.
```
</details>
```suggestion
Menu: Object.assign(
({ children, onClick, ...props }) => (
<div onClick={onClick} {...props}>
{children}
</div>
),
{
Item: ({ children, ...props }) => <div {...props}>{children}</div>,
},
),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Jest suite that includes `ListView.test.jsx`
(`superset-frontend/src/components/ListView/ListView.test.jsx`), which
mounts `<ListView
/>` in `beforeEach` via `factory()` (see `describe('ListView', () => { ...
}` block around
lines 140–210).
2. During render, `ListView` (in
`superset-frontend/src/components/ListView/ListView.tsx`)
imports `Menu` from `@superset-ui/core/components` and uses it as a
component, e.g.
`<Menu>` with nested `<Menu.Item>` for bulk action menus.
3. The Jest mock at lines 27–73 in `ListView.test.jsx` defines `Menu` as a
plain object `{
Item: (...) => <div>... }` instead of a function/component, because it omits
a callable
`Menu` component and only exposes `Item`.
4. When React reconciles `<Menu>` inside `ListView`, it receives an object
instead of a
valid element type and throws the runtime error "Element type is invalid:
expected a
string (for built-in components) or a class/function but got: object",
causing every test
in this file to error before assertions execute.
```
</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.jsx
**Line:** 51:53
**Comment:**
*Type Error: The Jest mock for `Menu` returns a plain object instead of
a React component, so when `ListView` renders `<Menu>` in tests it will crash
with an "element type is invalid" error.
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%2F33002&comment_hash=8be8cb01d97c3c0e1907ed05c38b7f33bfd2d56b0c46757255d2b05222dc79d7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33002&comment_hash=8be8cb01d97c3c0e1907ed05c38b7f33bfd2d56b0c46757255d2b05222dc79d7&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/bulkUpdate/BulkCertifyModal.tsx:
##########
@@ -0,0 +1,159 @@
+/**
+ * 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. 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 { useState, FC } from 'react';
+
+import { t, SupersetClient } from '@superset-ui/core';
+import Chart from 'src/types/Chart';
+import { Dashboard } from 'src/pages/DashboardList';
+import {
+ Input,
+ Modal,
+ Button,
+ FormLabel,
+ Col,
+ Row,
+} from '@superset-ui/core/components';
+
+interface BulkCertifyModalProps {
+ onHide: () => void;
+ refreshData: () => void;
+ addSuccessToast: (msg: string) => void;
+ addDangerToast: (msg: string) => void;
+ show: boolean;
+ resourceName: 'chart' | 'dashboard';
+ resourceLabel: string;
+ selected: Chart[] | Dashboard[];
+}
+
+const BulkCertifyModal: FC<BulkCertifyModalProps> = ({
+ show,
+ selected = [],
+ resourceName,
+ resourceLabel,
+ onHide,
+ refreshData,
+ addSuccessToast,
+ addDangerToast,
+}) => {
+ const [certifiedBy, setCertifiedBy] = useState<string>('');
+ const [certificationDetails, setCertificationDetails] = useState<string>('');
+
+ const resourceLabelPlural = t(
+ '%s',
+ selected.length > 1 ? `${resourceLabel}s` : resourceLabel,
+ );
+
+ const onSave = async () => {
+ if (!certifiedBy) {
+ addDangerToast(t('Please enter who certified these items'));
+ return;
+ }
+
+ try {
+ await Promise.all(
+ selected.map(item => {
+ const url = `/api/v1/${resourceName}/${item.id}`;
+ const payload = {
+ certified_by: certifiedBy,
+ certification_details: certificationDetails,
+ };
+
+ return SupersetClient.put({
+ url,
+ headers: { 'Content-Type': 'application/json' },
+ jsonPayload: payload,
+ });
+ }),
+ );
+
+ addSuccessToast(t('Successfully certified %s', resourceLabelPlural));
+ refreshData();
+ onHide();
+ setCertifiedBy('');
+ setCertificationDetails('');
+ } catch (error) {
+ addDangerToast(t('Failed to certify %s', resourceLabelPlural));
+ }
+ };
+
+ return (
+ <Modal
+ title={<h4>{t('Bulk certify %s', resourceLabelPlural)}</h4>}
+ show={show}
+ onHide={() => {
+ setCertifiedBy('');
+ setCertificationDetails('');
+ onHide();
+ }}
+ footer={
+ <div>
+ <Button
+ data-test="modal-cancel-certify-button"
+ buttonStyle="secondary"
+ onClick={onHide}
Review Comment:
**Suggestion:** The Cancel button calls the parent hide handler directly and
does not clear the modal's internal certification fields, while the modal's own
onHide handler does clear them, leading to inconsistent state where cancelling
via the button may reopen the modal with stale values if the component remains
mounted. Unify both code paths to use the same hide handler that clears local
state and then invokes the parent onHide. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Bulk certify modal reopens with stale certification fields.
- ⚠️ Inconsistent behavior between Cancel and modal close actions.
- ⚠️ Potential confusion when certifying multiple resource batches.
```
</details>
```suggestion
const handleHide = () => {
setCertifiedBy('');
setCertificationDetails('');
onHide();
};
return (
<Modal
title={<h4>{t('Bulk certify %s', resourceLabelPlural)}</h4>}
show={show}
onHide={handleHide}
footer={
<div>
<Button
data-test="modal-cancel-certify-button"
buttonStyle="secondary"
onClick={handleHide}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the bulk certification UI (charts or dashboards list) and trigger
the bulk certify
action, which mounts `BulkCertifyModal` from
`superset-frontend/src/features/bulkUpdate/BulkCertifyModal.tsx:31` with
`show={true}`.
2. In the modal, type a non-empty value into the "Certified by" input and
optional text
into "Certification details" (state stored in `certifiedBy` and
`certificationDetails` at
lines 51–52).
3. Close the modal by clicking the "Cancel" button defined at lines 103–107,
which calls
the parent `onHide` directly and does *not* execute the local reset logic
present in the
`Modal`'s `onHide` handler at lines 96–100.
4. Reopen the same modal instance by triggering bulk certify again while the
parent reuses
the mounted component and only toggles the `show` prop from `false` back to
`true`;
observe that the inputs still contain the previous values because local
state was never
cleared when Cancel was clicked, whereas closing via the modal's close
behavior
(ESC/header X/overlay, which invokes `Modal`'s `onHide`) would have reset
them.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/bulkUpdate/BulkCertifyModal.tsx
**Line:** 92:106
**Comment:**
*Logic Error: The Cancel button calls the parent hide handler directly
and does not clear the modal's internal certification fields, while the modal's
own onHide handler does clear them, leading to inconsistent state where
cancelling via the button may reopen the modal with stale values if the
component remains mounted. Unify both code paths to use the same hide handler
that clears local state and then invokes the parent onHide.
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%2F33002&comment_hash=555badb6564e9264feb0e2f43cd3bb8afda2575085ab72a4abab014413e4536e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33002&comment_hash=555badb6564e9264feb0e2f43cd3bb8afda2575085ab72a4abab014413e4536e&reaction=dislike'>👎</a>
##########
superset-frontend/cypress-base/cypress/e2e/dashboard_list/list.test.ts:
##########
@@ -53,12 +52,22 @@ function confirmDelete(bulk = false) {
cy.wrap($input).type('DELETE');
});
cy.getBySel('modal-confirm-button').should('be.visible').click();
+ cy.wait('@delete');
+}
+
+function confirmBulkDelete() {
+ interceptBulkDelete();
- if (bulk) {
- cy.wait('@bulkDelete');
- } else {
- cy.wait('@delete');
- }
+ // Wait for modal dialog to be present and visible
+ cy.get('[role="dialog")[aria-modal="true"]').should('be.visible');
Review Comment:
**Suggestion:** The CSS selector used to locate the delete confirmation
dialog is malformed (`[role="dialog")[aria-modal="true"]`), so Cypress will
fail to find the modal and the bulk delete test will hang or error instead of
interacting with the dialog. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Bulk delete dashboard e2e test fails consistently.
- ⚠️ Bulk delete feature lacks reliable automated coverage.
```
</details>
```suggestion
cy.get('[role="dialog"][aria-modal="true"]').should('be.visible');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Cypress spec
`superset-frontend/cypress-base/cypress/e2e/dashboard_list/list.test.ts`
(e.g., `yarn
cypress run --spec cypress/e2e/dashboard_list/list.test.ts`).
2. During execution of the "common actions" suite, the test `it('should bulk
delete
correctly', ...)` (around line 137 in the diff) is executed and calls
`toggleBulkSelect()`, selects dashboards, then triggers the bulk delete UI.
3. After clicking `cy.getBySel('bulk-select-action').trigger('mouseenter');
cy.contains('Delete').click();`, the test calls `confirmBulkDelete()`
defined around line
58 in the same file.
4. Inside `confirmBulkDelete()`, Cypress executes
`cy.get('[role="dialog")[aria-modal="true"]').should('be.visible');` which
uses an invalid
CSS selector, causing Cypress to fail to parse or match the selector and the
test to error
or time out instead of interacting with the confirmation modal.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/cypress-base/cypress/e2e/dashboard_list/list.test.ts
**Line:** 62:62
**Comment:**
*Logic Error: The CSS selector used to locate the delete confirmation
dialog is malformed (`[role="dialog")[aria-modal="true"]`), so Cypress will
fail to find the modal and the bulk delete test will hang or error instead of
interacting with the dialog.
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%2F33002&comment_hash=51d223f5b0c151ed902e536c23b8831cb6342273d34fec818e2bb58c2bbb87a0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33002&comment_hash=51d223f5b0c151ed902e536c23b8831cb6342273d34fec818e2bb58c2bbb87a0&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]
