codeant-ai-for-open-source[bot] commented on code in PR #36347:
URL: https://github.com/apache/superset/pull/36347#discussion_r2583462225
##########
superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx:
##########
@@ -249,26 +257,26 @@ const Row = props => {
);
const handleDeleteComponent = useCallback(() => {
- deleteComponent(rowComponent.id, parentId);
+ deleteComponent(rowComponent.id as string, parentId);
}, [deleteComponent, rowComponent, parentId]);
- const handleMenuHover = useCallback(hovered => {
- const { isHovered } = hovered;
- setHoverMenuHovered(isHovered);
+ const handleMenuHover = useCallback((hover: { isHovered: boolean }) => {
+ setHoverMenuHovered(hover.isHovered);
}, []);
- const rowItems = useMemo(
- () => rowComponent.children || [],
+ const rowItems: string[] = useMemo(
+ () => (rowComponent.children as string[]) ?? [],
[rowComponent.children],
);
const backgroundStyle = backgroundStyleOptions.find(
opt =>
- opt.value === (rowComponent.meta.background || BACKGROUND_TRANSPARENT),
- );
+ opt.value === (rowComponent.meta?.background ?? BACKGROUND_TRANSPARENT),
+ )!;
Review Comment:
**Suggestion:** The `backgroundStyleOptions.find` call assumes a matching
option is always found, so if `rowComponent.meta.background` contains an
unknown or legacy value that isn't in `backgroundStyleOptions`,
`backgroundStyle` will be `undefined` and subsequent property access will throw
at runtime; adding a safe fallback avoids hard failures for corrupted or
migrated layouts. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const backgroundStyle =
backgroundStyleOptions.find(
opt =>
opt.value === (rowComponent.meta?.background ??
BACKGROUND_TRANSPARENT),
) ?? backgroundStyleOptions[0];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code uses a non-null assertion (`!`) after `find`, so if
`rowComponent.meta?.background` somehow contains a value not represented in
`backgroundStyleOptions`, `backgroundStyle` becomes `undefined` and later
property access (`backgroundStyle.className`, `backgroundStyle.value`)
will throw at runtime. The proposed change introduces a concrete fallback
(`?? backgroundStyleOptions[0]`), which eliminates the hard crash while
preserving existing behavior when a match is found. This is a legitimate
robustness fix against malformed or legacy layout data, not just cosmetic
noise.
</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/Row/Row.tsx
**Line:** 272:275
**Comment:**
*Logic Error: The `backgroundStyleOptions.find` call assumes a matching
option is always found, so if `rowComponent.meta.background` contains an
unknown or legacy value that isn't in `backgroundStyleOptions`,
`backgroundStyle` will be `undefined` and subsequent property access will throw
at runtime; adding a safe fallback avoids hard failures for corrupted or
migrated layouts.
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/Row/Row.tsx:
##########
@@ -249,26 +257,26 @@ const Row = props => {
);
const handleDeleteComponent = useCallback(() => {
- deleteComponent(rowComponent.id, parentId);
+ deleteComponent(rowComponent.id as string, parentId);
}, [deleteComponent, rowComponent, parentId]);
- const handleMenuHover = useCallback(hovered => {
- const { isHovered } = hovered;
- setHoverMenuHovered(isHovered);
+ const handleMenuHover = useCallback((hover: { isHovered: boolean }) => {
+ setHoverMenuHovered(hover.isHovered);
}, []);
- const rowItems = useMemo(
- () => rowComponent.children || [],
+ const rowItems: string[] = useMemo(
+ () => (rowComponent.children as string[]) ?? [],
Review Comment:
**Suggestion:** `rowComponent.children` is blindly asserted to `string[]`,
so if the layout state ever provides a non-array value (e.g., null, a string,
or corrupt data), `rowItems.length` and `rowItems.map` will throw; guarding
with `Array.isArray` ensures the component degrades gracefully to an empty row
instead of crashing. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
() =>
Array.isArray(rowComponent.children)
? (rowComponent.children as string[])
: [],
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code performs a type assertion on `rowComponent.children` and
then nullish-coalesces to `[]`; this only protects the `null`/`undefined`
case. If `children` is ever some other non-array value (e.g. a string or
corrupt data from persisted layout), the memo will return that value, and
`rowItems.map(...)` will explode at runtime because `map` is not defined
on those types. Guarding with `Array.isArray` makes the component safely
treat non-array values as "no children", avoiding a crash while keeping
the expected behavior when the data is correct. That is a concrete
correctness/robustness improvement, not just style.
</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/Row/Row.tsx
**Line:** 268:268
**Comment:**
*Possible Bug: `rowComponent.children` is blindly asserted to
`string[]`, so if the layout state ever provides a non-array value (e.g., null,
a string, or corrupt data), `rowItems.length` and `rowItems.map` will throw;
guarding with `Array.isArray` ensures the component degrades gracefully to an
empty row instead of crashing.
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/Row/Row.test.tsx:
##########
@@ -261,7 +307,7 @@ describe('visibility handling for intersection observers',
() => {
});
test('intersection observer callbacks handle entries without errors', () => {
- const callback = ([entry]) => {
+ const callback = ([entry]: [MockIntersectionObserverEntry]) => {
Review Comment:
**Suggestion:** The "intersection observer callbacks" test constructs a
standalone callback instead of using the callback actually passed to
`IntersectionObserver` by the component, so the test will still pass even if
the real callback in the Row implementation starts throwing; it should obtain
and exercise the callback recorded by the `mockIntersectionObserver`. [logic
error]
**Severity Level:** Minor ⚠️
##########
superset-frontend/src/dashboard/components/gridComponents/new/NewRow.test.tsx:
##########
@@ -26,12 +26,12 @@ import { ROW_TYPE } from
'src/dashboard/util/componentTypes';
jest.mock(
'src/dashboard/components/gridComponents/new/DraggableNewComponent',
() =>
- ({ type, id }) => (
+ ({ type, id }: { type: string; id: string }) => (
<div data-test="mock-draggable-new-component">{`${type}:${id}`}</div>
Review Comment:
**Suggestion:** The mocked component uses the `data-test` attribute while
the tests query with `getByTestId`, which looks for `data-testid`, so the
element will not be found and the tests will fail even if the component renders
correctly. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
<div data-testid="mock-draggable-new-component">{`${type}:${id}`}</div>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The tests use `getByTestId('mock-draggable-new-component')`, which relies on
the `data-testid`
attribute, but the mocked component currently renders `data-test`. That
means the queries will
never match, causing the tests to fail even if the component renders.
Changing to
`data-testid` directly fixes a concrete, verifiable issue in the test logic
rather than just
tweaking style.
</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/new/NewRow.test.tsx
**Line:** 30:30
**Comment:**
*Logic Error: The mocked component uses the `data-test` attribute while
the tests query with `getByTestId`, which looks for `data-testid`, so the
element will not be found and the tests will fail even if the component renders
correctly.
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]
