codeant-ai-for-open-source[bot] commented on code in PR #36071:
URL: https://github.com/apache/superset/pull/36071#discussion_r2594987765
##########
superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -209,7 +209,7 @@ describe('PropertiesModal', () => {
expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument();
// Only General information section is expanded by default
- expect(screen.getAllByRole('textbox')).toHaveLength(2); // Name and Slug
+ expect(screen.getAllByRole('textbox')).toHaveLength(3); // Name, Slug and
Description
Review Comment:
**Suggestion:** Timing/race issue: the test asserts the number of textboxes
immediately which can run before the DOM has fully updated and cause
intermittent failures; wrap the assertion in an async waiter (use `waitFor` or
`findAllByRole`) so the test waits for the elements to appear. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
await waitFor(() => {
expect(screen.getAllByRole('textbox')).toHaveLength(3); // Name, Slug
and Description
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The synchronous getAllByRole assertion can race with async
rendering/animations in this component and lead to intermittent CI failures.
Wrapping the check in await waitFor(...) or using findAllByRole makes the
test wait for the DOM to stabilize and is a legitimate, minimal fix for
flakiness.
</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/PropertiesModal/PropertiesModal.test.tsx
**Line:** 212:212
**Comment:**
*Race Condition: Timing/race issue: the test asserts the number of
textboxes immediately which can run before the DOM has fully updated and cause
intermittent failures; wrap the assertion in an async waiter (use `waitFor` or
`findAllByRole`) so the test waits for the elements to appear.
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/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -299,7 +299,7 @@ describe('PropertiesModal', () => {
await screen.findByTestId('dashboard-edit-properties-form'),
).toBeInTheDocument();
- expect(screen.getAllByRole('textbox')).toHaveLength(2); // Only Name and
Slug visible initially
+ expect(screen.getAllByRole('textbox')).toHaveLength(3); // Only Name, Slug
and Description visible initially
Review Comment:
**Suggestion:** Flaky/brittle assertion: asserting an exact number of
textboxes without waiting is fragile (additional editors or delayed renders can
change the count); replace the synchronous check with `await waitFor(...)` or
use `await screen.findAllByRole('textbox')` and assert the result length. [race
condition]
**Severity Level:** Minor ⚠️
```suggestion
await waitFor(() => {
expect(screen.getAllByRole('textbox')).toHaveLength(3); // Only Name,
Slug and Description visible initially
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The component may render form fields asynchronously (collapses, animations).
Using await waitFor(...) or await screen.findAllByRole('textbox') ensures
stability and prevents flaky failures.
</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/PropertiesModal/PropertiesModal.test.tsx
**Line:** 302:302
**Comment:**
*Race Condition: Flaky/brittle assertion: asserting an exact number of
textboxes without waiting is fragile (additional editors or delayed renders can
change the count); replace the synchronous check with `await waitFor(...)` or
use `await screen.findAllByRole('textbox')` and assert the result length.
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/PropertiesModal/index.tsx:
##########
@@ -280,7 +282,7 @@ const PropertiesModal = ({
};
const onFinish = () => {
- const { title, slug, certifiedBy, certificationDetails } =
+ const { title, description, slug, certifiedBy, certificationDetails } =
Review Comment:
**Suggestion:** The form value `description` is destructured from
`form.getFieldsValue()` without a default; if the field is absent the value
will be `undefined` and then be passed through to downstream code (and to
`onSubmitProps`) causing inconsistent behavior and making the later
"description || null" normalization required. Provide a default when
destructuring to ensure `description` is a string and avoid undefined leaking
into submit logic. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const { title, description = '', slug, certifiedBy, certificationDetails
} =
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Defaulting `description` to an empty string prevents `undefined` from flowing
into onSubmitProps (where it's later used without normalization). This is a
harmless, defensive change that eliminates inconsistent handling and keeps
payloads predictable. The improved code is simple and correct.
</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/PropertiesModal/index.tsx
**Line:** 285:285
**Comment:**
*Possible Bug: The form value `description` is destructured from
`form.getFieldsValue()` without a default; if the field is absent the value
will be `undefined` and then be passed through to downstream code (and to
`onSubmitProps`) causing inconsistent behavior and making the later
"description || null" normalization required. Provide a default when
destructuring to ensure `description` is a string and avoid undefined leaking
into submit logic.
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/dashboards/api.py:
##########
@@ -168,6 +168,7 @@ def wraps(self: BaseSupersetModelRestApi, id_or_slug: str)
-> Response:
"published",
"status",
"slug",
+ "description",
Review Comment:
**Suggestion:** Duplicate column entry added to the BASE_LIST_COLUMNS list;
adding `"description"` again will produce duplicate column names which can lead
to ambiguous behavior when building queries or serializing results — remove the
newly added duplicate entry. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR added a second "description" entry to BASE_LIST_COLUMNS (the file
already contains "description"). Having duplicate column names in the list can
lead to ambiguous behavior when constructing queries/serializing results.
Removing the duplicated line is a correct, low-risk cleanup that restores the
intended single column entry.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 171:171
**Comment:**
*Logic Error: Duplicate column entry added to the BASE_LIST_COLUMNS
list; adding `"description"` again will produce duplicate column names which
can lead to ambiguous behavior when building queries or serializing results —
remove the newly added duplicate entry.
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/pages/DashboardList/index.tsx:
##########
@@ -118,12 +119,30 @@ const Actions = styled.div`
color: ${({ theme }) => theme.colorIcon};
`;
+const FlexRowContainer = styled.div`
+ align-items: center;
+ display: flex;
+ gap: ${({ theme }) => theme.sizeUnit}px;
+
+ a {
+ overflow: hidden;
Review Comment:
**Suggestion:** The new `FlexRowContainer` CSS attempts to ellipsize the
Link text via `text-overflow: ellipsis`, but anchors are inline by default and
the ellipsis will not work unless the anchor is a block-level (or inline-block)
with constrained width; update the `a` rule to ensure `display` and a max width
so truncation reliably works. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
display: inline-block;
max-width: 100%;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The new UI intends to ellipsize long dashboard titles inside a flex row.
Text-overflow on an inline anchor is unreliable.
Making the anchor behave as a block/inline-block and constraining its width
helps the ellipsis work. This is a practical, low-risk improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/DashboardList/index.tsx
**Line:** 128:128
**Comment:**
*Possible Bug: The new `FlexRowContainer` CSS attempts to ellipsize the
Link text via `text-overflow: ellipsis`, but anchors are inline by default and
the ellipsis will not work unless the anchor is a block-level (or inline-block)
with constrained width; update the `a` rule to ensure `display` and a max width
so truncation reliably works.
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/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -254,7 +254,7 @@ describe('PropertiesModal', () => {
expect(screen.getByText('Certification')).toBeInTheDocument();
// General information section is expanded by default
- expect(screen.getAllByRole('textbox')).toHaveLength(2); // Name and Slug
are visible
+ expect(screen.getAllByRole('textbox')).toHaveLength(3); // Name, Slug and
Description are visible
Review Comment:
**Suggestion:** Timing/race issue: the test checks the number of textboxes
synchronously; this is brittle because UI animations or async renders may delay
inputs — use `await waitFor(...)` or `await screen.findAllByRole('textbox')` to
ensure the DOM is stable before asserting the count. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
await waitFor(() => {
expect(screen.getAllByRole('textbox')).toHaveLength(3); // Name, Slug
and Description are visible
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same as above — the assertion assumes instant rendering of inputs.
Converting to await waitFor(...) or findAllByRole reduces flakiness and aligns
with patterns used elsewhere in the file.
</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/PropertiesModal/PropertiesModal.test.tsx
**Line:** 257:257
**Comment:**
*Race Condition: Timing/race issue: the test checks the number of
textboxes synchronously; this is brittle because UI animations or async renders
may delay inputs — use `await waitFor(...)` or `await
screen.findAllByRole('textbox')` to ensure the DOM is stable before asserting
the count.
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/dashboards/api.py:
##########
@@ -314,6 +315,7 @@ def get_list(self, **kwargs: Any) -> Response:
"certification_details",
"dashboard_title",
"slug",
+ "description",
Review Comment:
**Suggestion:** Duplicate column entry added to the `add_columns` list; the
`"description"` element is already present and re-adding it can cause
duplicated fields in payloads and unexpected behavior — remove this duplicate
line. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The add_columns/edit_columns list already contains "description" and the PR
introduced the same entry again. This is redundant and can cause duplicate
fields in created/edited payloads. Removing the extra line is the proper fix
and has no functional downside.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 318:318
**Comment:**
*Logic Error: Duplicate column entry added to the `add_columns` list;
the `"description"` element is already present and re-adding it can cause
duplicated fields in payloads and unexpected behavior — remove this duplicate
line.
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/pages/DashboardList/DashboardList.test.jsx:
##########
@@ -136,7 +136,7 @@ describe('DashboardList', () => {
const calls = fetchMock.calls(/dashboard\/\?q/);
expect(calls[0][0]).toMatchInlineSnapshot(
-
`"http://localhost/api/v1/dashboard/?q=(order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25,select_columns:!(id,dashboard_title,published,url,slug,changed_by,changed_by.id,changed_by.first_name,changed_by.last_name,changed_on_delta_humanized,owners,owners.id,owners.first_name,owners.last_name,tags.id,tags.name,tags.type,status,certified_by,certification_details,changed_on))"`,
+
`"http://localhost/api/v1/dashboard/?q=(order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25,select_columns:!(id,dashboard_title,published,url,slug,description,changed_by,changed_by.id,changed_by.first_name,changed_by.last_name,changed_on_delta_humanized,owners,owners.id,owners.first_name,owners.last_name,tags.id,tags.name,tags.type,status,certified_by,certification_details,changed_on))"`,
);
Review Comment:
**Suggestion:** Using an exact inline snapshot for the entire request URL is
unnecessarily strict; a smaller, focused assertion (e.g., that the request path
and that it contains the `?q=` query) is more robust and less likely to break
with unrelated changes. Replace the full snapshot with a contains-match on the
API path. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(calls[0][0]).toContain('/api/v1/dashboard/?q=');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Replacing the strict inline snapshot with a contains-match on the API path
(e.g., expect(...).toContain('/api/v1/dashboard/?q=')) reduces brittleness and
still verifies the request was made to the correct endpoint. It's a simple,
practical improvement for test stability and applies directly to the displayed
code.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/DashboardList/DashboardList.test.jsx
**Line:** 138:140
**Comment:**
*Possible Bug: Using an exact inline snapshot for the entire request
URL is unnecessarily strict; a smaller, focused assertion (e.g., that the
request path and that it contains the `?q=` query) is more robust and less
likely to break with unrelated changes. Replace the full snapshot with a
contains-match on the API path.
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]
