codeant-ai-for-open-source[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r2990889273
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -523,30 +523,33 @@ const DashboardBuilder = () => {
({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
{dropIndicatorProps && <div {...dropIndicatorProps} />}
- {!isReport && topLevelTabs && !uiConfig.hideTab && !uiConfig.hideNav
&& (
- <WithPopoverMenu
- shouldFocus={shouldFocusTabs}
- menuItems={[
- <IconButton
- key="collapse-tabs"
- icon={<Icons.FallOutlined iconSize="xl" />}
- label={t('Collapse tab content')}
- onClick={handleDeleteTopLevelTabs}
- />,
- ]}
- editMode={editMode}
- >
- <DashboardComponent
- id={topLevelTabs?.id}
- parentId={DASHBOARD_ROOT_ID}
- depth={DASHBOARD_ROOT_DEPTH + 1}
- index={0}
- renderTabContent={false}
- renderHoverMenu={false}
- onChangeTab={handleChangeTab}
- />
- </WithPopoverMenu>
- )}
+ {!isReport &&
+ topLevelTabs &&
+ !uiConfig.hideTab &&
+ !uiConfig.hideNav && (
Review Comment:
**Suggestion:** The new condition incorrectly ties tab visibility to
`hideNav`. In this codebase, `hideTab` is the dedicated flag for tab
visibility, while `hideNav` is used for global navigation (for example, the
home menu). This causes dashboards with `hideNav=true` and `hideTab=false` to
lose their top-level tabs unexpectedly, breaking embedded/dashboard navigation
behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded dashboards can lose top-level tab navigation.
- ⚠️ `uiConfig=4` hides tabs despite `hideTab=false`.
```
</details>
```suggestion
!uiConfig.hideTab && (
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard with root-level tabs (rendered by `DashboardBuilder`),
where tab
rendering is driven by `renderDraggableContent` in
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:522-553`,
and used by `<Droppable>` at `:635-650`.
2. Pass URL query `uiConfig=4` (bitmask parsed in
`EmbeddedUiConfigProvider`), which sets
`hideNav` true via `hideNav: (config & 4) !== 0` in
`superset-frontend/src/components/UiConfigContext/index.tsx:51-56`.
3. `DashboardBuilder` reads `useUiConfig()` at
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:360`
and
evaluates the condition `!uiConfig.hideNav` at `:529`.
4. Observe top-level tabs are not rendered even when `hideTab` is false,
because the new
condition blocks `<DashboardComponent id={topLevelTabs?.id} .../>` in
`DashboardBuilder.tsx:542-550`, removing in-dashboard tab navigation.
```
</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/DashboardBuilder.tsx
**Line:** 528:529
**Comment:**
*Logic Error: The new condition incorrectly ties tab visibility to
`hideNav`. In this codebase, `hideTab` is the dedicated flag for tab
visibility, while `hideNav` is used for global navigation (for example, the
home menu). This causes dashboards with `hideNav=true` and `hideTab=false` to
lose their top-level tabs unexpectedly, breaking embedded/dashboard navigation
behavior.
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%2F38792&comment_hash=7ba894a6434db929d709828072f5882deaebde8f0709d0397ff7c26b75ead18b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=7ba894a6434db929d709828072f5882deaebde8f0709d0397ff7c26b75ead18b&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx:
##########
@@ -188,14 +188,32 @@ test('triggers onChange when the mode changes', async ()
=> {
store,
});
await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
- userEvent.click(screen.getByTitle('Midnight'));
- expect(await screen.findByTitle('Relative Date/Time')).toBeInTheDocument();
- userEvent.click(screen.getByTitle('Relative Date/Time'));
- userEvent.click(screen.getAllByTitle('Now')[1]);
+
+ const midnightOptions = await screen.findAllByTitle('Midnight');
+ userEvent.click(midnightOptions[0]);
+
+ const relativeOptions = await screen.findAllByTitle('Relative Date/Time');
+ userEvent.click(relativeOptions[0]);
+
+ const nowOptions = await screen.findAllByTitle('Now');
+ userEvent.click(nowOptions[0]);
+
expect(
await screen.findByText('Configure custom time range'),
).toBeInTheDocument();
- userEvent.click(screen.getAllByTitle('Specific Date/Time')[1]);
+
+ // Polling condition ONLY - No side effects (clicks) inside here
+ await waitFor(
+ () => {
+ expect(screen.getAllByText('Specific Date/Time')).toHaveLength(2);
+ },
+ { timeout: 3000 },
+ );
+
+ // Perform the click ONCE after the condition is met
+ const tabs = screen.getAllByText('Specific Date/Time');
+ userEvent.click(tabs[1]);
Review Comment:
**Suggestion:** The test is selecting options via
`findAllByTitle(...)[index]`, which is unstable with portal-rendered Ant Select
dropdowns because hidden/stale nodes can match and the wrong element can be
clicked. This can make the mode never change and cause intermittent
failures/timeouts. Target the specific combobox and choose visible options by
role instead of title/index. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ `CustomFrame` mode-change test can be flaky in CI.
- ⚠️ Intermittent failures may block frontend PR merges.
```
</details>
```suggestion
const startModeSelect = screen.getByRole('combobox', {
name: 'Start (inclusive)',
});
userEvent.click(startModeSelect);
userEvent.click(
await screen.findByRole('option', { name: 'Relative Date/Time' }),
);
const endModeSelect = screen.getByRole('combobox', {
name: 'End (exclusive)',
});
userEvent.click(endModeSelect);
userEvent.click(
await screen.findByRole('option', { name: 'Specific Date/Time' }),
);
expect(
await screen.findByText('Configure custom time range'),
).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run test `triggers onChange when the mode changes` in
`superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx:185`
so execution enters the new sequence at lines 192-215.
2. The test queries globally with `screen.findAllByTitle(...)` and clicks
index 0
(`CustomFrame.test.tsx:192-200`), which is not scoped to a specific select
and depends on
DOM ordering.
3. `CustomFrame` renders two `Select` controls (`CustomFrame.tsx:130-135`
and `189-194`)
with overlapping option labels, while `Select` portals dropdowns via
`getPopupContainer`
(`packages/superset-ui-core/src/components/Select/Select.tsx:180-183`), so
title-based
global matches can include non-target nodes.
4. When index 0 resolves to a non-target match, the expected mode transition
is not
applied; downstream assertions can fail (`waitFor` at
`CustomFrame.test.tsx:206-211` or
`toHaveBeenCalledTimes(2)` at line 217), causing intermittent test
instability.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx
**Line:** 192:215
**Comment:**
*Logic Error: The test is selecting options via
`findAllByTitle(...)[index]`, which is unstable with portal-rendered Ant Select
dropdowns because hidden/stale nodes can match and the wrong element can be
clicked. This can make the mode never change and cause intermittent
failures/timeouts. Target the specific combobox and choose visible options by
role instead of title/index.
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%2F38792&comment_hash=99cea0ce05fd19971c44437158ed28e6e7918b87a8537d2d78000f2759e4e17c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=99cea0ce05fd19971c44437158ed28e6e7918b87a8537d2d78000f2759e4e17c&reaction=dislike'>👎</a>
##########
superset/databases/api.py:
##########
@@ -630,8 +630,9 @@ def delete(self, pk: int) -> Response:
@protect()
@statsd_metrics
@event_logger.log_this_with_context(
- action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
- f".sync-permissions",
+ action=lambda self, *args, **kwargs: (
+ f"{self.__class__.__name__}.sync-permissions"
Review Comment:
**Suggestion:** The new event action name uses a hyphen (`sync-permissions`)
while this API and the rest of the codebase use underscore-based action names.
This changes the emitted audit event key from the previous default
(`sync_permissions`) and can break consumers that filter/group logs by the
existing action contract. Keep the action value aligned with the method name to
preserve compatibility. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ sync_permissions audit action key changes unexpectedly.
- ⚠️ Existing action-based log filters can miss events.
```
</details>
```suggestion
f"{self.__class__.__name__}.sync_permissions"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call `POST /api/v1/database/<pk>/sync_permissions/`, which is routed by
`sync_permissions()` at `superset/databases/api.py:629-638`.
2. The decorator at `superset/databases/api.py:632-637` forces action to
`DatabaseRestApi.sync-permissions`.
3. In `superset/utils/log.py:28-30`, `_wrapper()` uses that explicit
`action` instead of
default `f.__name__` (which would be `sync_permissions`).
4. `DBEventLogger.log()` persists this value into `Log.action` at
`superset/utils/log.py:135-137` (`Log` model field defined at
`superset/models/core.py:1369`).
5. Query log views/searches that filter by action (supported in
`superset/views/log/api.py:61-64`) and observe entries now use hyphenated
key, so
underscore-based filters/contracts miss these events.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/databases/api.py
**Line:** 634:634
**Comment:**
*Logic Error: The new event action name uses a hyphen
(`sync-permissions`) while this API and the rest of the codebase use
underscore-based action names. This changes the emitted audit event key from
the previous default (`sync_permissions`) and can break consumers that
filter/group logs by the existing action contract. Keep the action value
aligned with the method name to preserve compatibility.
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%2F38792&comment_hash=9948e076681fa71f6bbba910f7c3b32ddafedc50ab283a9f75398f161c6e916e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=9948e076681fa71f6bbba910f7c3b32ddafedc50ab283a9f75398f161c6e916e&reaction=dislike'>👎</a>
##########
superset/reports/notifications/slack_mixin.py:
##########
@@ -94,7 +94,7 @@ def _get_body(self, content: NotificationContent) -> str:
# need to truncate the data
for i in range(len(df) - 1):
truncated_df = df[: i + 1].fillna("")
- truncated_row = pd.Series({k: "..." for k in df.columns})
+ truncated_row = pd.Series(dict.fromkeys(df.columns, "..."))
Review Comment:
**Suggestion:** Building `truncated_row` with `dict.fromkeys(df.columns,
"...")` drops duplicate column names because dict keys must be unique. If the
dataframe has duplicate columns (which can happen after flattening tuple
headers), the subsequent `pd.concat` raises `InvalidIndexError` at runtime.
Build the series with an explicit index so duplicates are preserved. [type
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Slack TEXT reports can fail before sending.
- ⚠️ Affects Slack v1 and v2 notification paths.
```
</details>
```suggestion
truncated_row = pd.Series(["..."] * len(df.columns),
index=df.columns)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a TEXT report execution path where
`NotificationContent.embedded_data` is
populated (`superset/commands/report/execute.py:642`,
`_get_notification_content` calling
`_get_embedded_data`).
2. Ensure the dataframe has duplicate column labels after flattening in
`SlackMixin._get_body`
(`superset/reports/notifications/slack_mixin.py:75-83`), e.g.
duplicate names from chart metadata.
3. Send Slack notification through `SlackNotification.send`
(`superset/reports/notifications/slack.py:91`) or `SlackV2Notification.send`
(`superset/reports/notifications/slackv2.py:87`), both calling `_get_body`.
4. Execution reaches `truncated_row = pd.Series(dict.fromkeys(df.columns,
"..."))`
(`superset/reports/notifications/slack_mixin.py:97`), which drops duplicate
keys.
5. Next `pd.concat` (`superset/reports/notifications/slack_mixin.py:98-99`)
raises
`InvalidIndexError`; verified with pandas runtime reproduction (`Reindexing
only valid
with uniquely valued Index objects`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/slack_mixin.py
**Line:** 97:97
**Comment:**
*Type Error: Building `truncated_row` with `dict.fromkeys(df.columns,
"...")` drops duplicate column names because dict keys must be unique. If the
dataframe has duplicate columns (which can happen after flattening tuple
headers), the subsequent `pd.concat` raises `InvalidIndexError` at runtime.
Build the series with an explicit index so duplicates are preserved.
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%2F38792&comment_hash=badea1f813b564281ec0da86f0b2c5f8f6cad465a574ad464e46e814fc30fcfa&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=badea1f813b564281ec0da86f0b2c5f8f6cad465a574ad464e46e814fc30fcfa&reaction=dislike'>👎</a>
##########
tests/unit_tests/async_events/test_async_query_cookie_validation.py:
##########
@@ -0,0 +1,45 @@
+import pytest
+from flask import Flask
+
+from superset.async_events.async_query_manager import AsyncQueryManager
+
+
+def create_app(config_overrides=None):
+ app = Flask(__name__)
+ app.config.update(
+ {
+ "CACHE_CONFIG": {"CACHE_TYPE": "RedisCache"},
+ "DATA_CACHE_CONFIG": {"CACHE_TYPE": "RedisCache"},
+ "GLOBAL_ASYNC_QUERIES": True,
Review Comment:
**Suggestion:** `AsyncQueryManager.init_app` reads
`GLOBAL_ASYNC_QUERIES_CACHE_BACKEND`, but this test only sets
`CACHE_CONFIG`/`DATA_CACHE_CONFIG`. That causes `UnsupportedCacheBackendError`
before the cookie validation path is reached. Add the async-queries cache
backend config so the test exercises the intended branch. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ New async cookie unit test fails in CI.
- ⚠️ Intended cookie-scheme validation path remains untested.
```
</details>
```suggestion
"GLOBAL_ASYNC_QUERIES_CACHE_BACKEND": {"CACHE_TYPE":
"RedisCache"},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `test_secure_async_cookie_over_http_fails_fast` in
`tests/unit_tests/async_events/test_async_query_cookie_validation.py:36`,
which builds a
bare Flask app via `create_app()` (`:7-33`).
2. The test calls `AsyncQueryManager.init_app(app)` at
`tests/unit_tests/async_events/test_async_query_cookie_validation.py:45`.
3. `init_app` in `superset/async_events/async_query_manager.py:130` calls
`get_cache_backend(app.config)`.
4. `get_cache_backend` reads `GLOBAL_ASYNC_QUERIES_CACHE_BACKEND` with
default `{}` at
`superset/async_events/async_query_manager.py:84`, then raises
`UnsupportedCacheBackendError` at `:94` because `CACHE_TYPE` is missing.
5. Result: initialization fails before the intended cookie-validation
behavior is
exercised; this is directly caused by missing
`GLOBAL_ASYNC_QUERIES_CACHE_BACKEND` in test
config.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/async_events/test_async_query_cookie_validation.py
**Line:** 13:13
**Comment:**
*Logic Error: `AsyncQueryManager.init_app` reads
`GLOBAL_ASYNC_QUERIES_CACHE_BACKEND`, but this test only sets
`CACHE_CONFIG`/`DATA_CACHE_CONFIG`. That causes `UnsupportedCacheBackendError`
before the cookie validation path is reached. Add the async-queries cache
backend config so the test exercises the intended branch.
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%2F38792&comment_hash=66b39d08773cebe045ea95327c22c09c848eefa1d18f0e97534848d9fc74c64f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=66b39d08773cebe045ea95327c22c09c848eefa1d18f0e97534848d9fc74c64f&reaction=dislike'>👎</a>
##########
superset/utils/excel.py:
##########
@@ -70,9 +70,9 @@ def apply_column_types(
# if the number is too large, convert it to a string
# Excel does not support numbers larger than 10^15
df[column] = df[column].apply(
- lambda x: str(x)
- if isinstance(x, (int, float)) and abs(x) > 10**15
- else x
+ lambda x: (
+ str(x) if isinstance(x, (int, float)) and abs(x) >
10**15 else x
Review Comment:
**Suggestion:** Converting large values with `str(x)` can produce scientific
notation for float-upcasted values (for example when the column contains
nulls), so exported IDs like `10000000000000000` become `1e+16` text and lose
the intended exact representation. Format integer-like floats in fixed-point
form before string conversion to keep stable digit strings. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ XLSX exports can show `1e+16` identifier strings.
- ⚠️ Large numeric IDs lose stable digit formatting.
- ⚠️ Affects explore_json and query-context XLSX downloads.
```
</details>
```suggestion
format(x, ".0f")
if isinstance(x, float) and x.is_integer() and
abs(x) > 10**15
else str(x)
if isinstance(x, (int, float)) and abs(x) > 10**15
else x
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger XLSX export through `explore_json` flow:
`superset/views/core.py:317-320`
accepts `ChartDataResultFormat.XLSX`, then `superset/views/core.py:199-200`
routes to
`_generate_xlsx`.
2. In `_generate_xlsx`, when column types exist,
`superset/views/core.py:228` calls
`apply_column_types(df, coltypes)` before writing the file.
3. In `superset/utils/excel.py:67-75`, numeric columns are converted with
`pd.to_numeric`;
if a numeric column contains nulls, pandas upcasts to `float64`.
4. The PR lambda at `superset/utils/excel.py:73-75` then uses `str(x)` for
values `>
10**15`; for float-upcasted values this yields scientific notation (verified
by execution:
`Series([10**16, None])` becomes `'1e+16'`).
5. The same XLSX conversion path is also used by chart data processing
(`superset/common/query_actions.py:167` →
`superset/common/query_context_processor.py:260-263`), so exported large IDs
can appear as
scientific-notation text instead of stable full digits.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/excel.py
**Line:** 74:74
**Comment:**
*Logic Error: Converting large values with `str(x)` can produce
scientific notation for float-upcasted values (for example when the column
contains nulls), so exported IDs like `10000000000000000` become `1e+16` text
and lose the intended exact representation. Format integer-like floats in
fixed-point form before string conversion to keep stable digit strings.
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%2F38792&comment_hash=4b8697d0c852199baf798abfeb1b7d37378221d92b29d1a6928ee518a7389e91&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=4b8697d0c852199baf798abfeb1b7d37378221d92b29d1a6928ee518a7389e91&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]
