codeant-ai-for-open-source[bot] commented on code in PR #37452:
URL: https://github.com/apache/superset/pull/37452#discussion_r2738523311
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -192,7 +192,21 @@ class Chart extends PureComponent<ChartProps, {}> {
}
}
+ shouldRenderChart() {
+ return (
+ this.props.isInView ||
+ !isFeatureEnabled(FeatureFlag.DashboardVirtualization) ||
+ isCurrentUserBot()
+ );
+ }
+
runQuery() {
+ if (
+ isFeatureEnabled(FeatureFlag.DashboardLoadDataVirtualization) &&
Review Comment:
**Suggestion:** Logic bug: the guard checks
`FeatureFlag.DashboardLoadDataVirtualization` but `shouldRenderChart()` is
gated by `FeatureFlag.DashboardVirtualization`; if the intended behavior is to
avoid loading data when dashboard virtualization is active, use the same
feature flag here so the check actually aligns with `shouldRenderChart()`. With
the current code the early-return may never trigger when virtualization is
enabled (or may trigger unexpectedly) depending on which flag is set. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Offscreen charts trigger unnecessary postChartFormData API calls.
- ⚠️ Dashboard virtualization continues loading hidden chart data.
- ⚠️ Backend concurrency and quota usage increased unexpectedly.
```
</details>
```suggestion
isFeatureEnabled(FeatureFlag.DashboardVirtualization) &&
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard that uses virtualization with the runtime feature flag
DashboardVirtualization set to true in window.featureFlags (feature flag
read at
packages/superset-ui-core/src/utils/featureFlags.ts:103-110 via
isFeatureEnabled()).
2. The Chart component mounts for each slice: Chart.componentDidMount()
(superset-frontend/src/components/Chart/Chart.tsx:183-187) calls
this.runQuery().
3. In Chart.runQuery()
(superset-frontend/src/components/Chart/Chart.tsx:203-215) the
current guard evaluates
isFeatureEnabled(FeatureFlag.DashboardLoadDataVirtualization)
(line 204-206) — if DashboardLoadDataVirtualization is false but
DashboardVirtualization
is true, the guard condition is false.
4. Because the guard is false, runQuery proceeds to call
actions.postChartFormData(...)
(superset-frontend/src/components/Chart/Chart.tsx:211-215 and dispatched to
chartAction.postChartFormData at
superset-frontend/src/components/Chart/chartAction.js:544-553), causing data
to be fetched
for charts that should be non-rendered/offscreen per shouldRenderChart()
(superset-frontend/src/components/Chart/Chart.tsx:195-201). This reproduces
unexpected
data loads for invisible charts.
```
</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/Chart/Chart.tsx
**Line:** 205:205
**Comment:**
*Logic Error: Logic bug: the guard checks
`FeatureFlag.DashboardLoadDataVirtualization` but `shouldRenderChart()` is
gated by `FeatureFlag.DashboardVirtualization`; if the intended behavior is to
avoid loading data when dashboard virtualization is active, use the same
feature flag here so the check actually aligns with `shouldRenderChart()`. With
the current code the early-return may never trigger when virtualization is
enabled (or may trigger unexpectedly) depending on which flag is set.
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/views/base.py:
##########
@@ -95,6 +95,7 @@
"DASHBOARD_AUTO_REFRESH_MODE",
"DASHBOARD_AUTO_REFRESH_INTERVALS",
"DASHBOARD_VIRTUALIZATION",
+ "DASHBOARD_LOAD_DATA_VIRTUALIZATION",
Review Comment:
**Suggestion:** Configuration key naming inconsistency: most runtime flags
that are namespaced for Superset use the "SUPERSET_" prefix; if the deployment
sets this config as "SUPERSET_DASHBOARD_LOAD_DATA_VIRTUALIZATION" (following
convention) the frontend will not receive it because this code reads
"DASHBOARD_LOAD_DATA_VIRTUALIZATION". Rename the key so the tuple matches the
canonical config name used elsewhere. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Frontend bootstrap missing virtualization load flag.
- ❌ Dashboards may load unseen chart data unnecessarily.
- ⚠️ Backend concurrency affected by extra query load.
- ⚠️ Affects SPA payload generation and dashboard rendering.
```
</details>
```suggestion
"SUPERSET_DASHBOARD_LOAD_DATA_VIRTUALIZATION",
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In your deployment config (e.g. config.py) set the canonical flag:
SUPERSET_DASHBOARD_LOAD_DATA_VIRTUALIZATION = True and start the server.
2. Confirm the backend builds the frontend bootstrap keys from
FRONTEND_CONF_KEYS
in superset/views/base.py where the tuple is defined (see file, lines
shown
in the PR hunk around lines 75-106; the new key is at line 98).
3. When cached_common_bootstrap_data() iterates FRONTEND_CONF_KEYS it calls
app.config.get(k). Because the tuple contains
"DASHBOARD_LOAD_DATA_VIRTUALIZATION"
(line 98) and not "SUPERSET_DASHBOARD_LOAD_DATA_VIRTUALIZATION", the call
app.config.get("DASHBOARD_LOAD_DATA_VIRTUALIZATION") returns None.
Function referenced: cached_common_bootstrap_data
(superset/views/base.py),
which builds frontend_config from FRONTEND_CONF_KEYS and is used to
populate
bootstrap payload sent to the client.
4. The SPA receives bootstrap data from get_spa_template_context() ->
get_spa_payload()
-> common_bootstrap_payload() -> cached_common_bootstrap_data(). The
frontend
will therefore not see the intended
SUPERSET_DASHBOARD_LOAD_DATA_VIRTUALIZATION
value and will behave as if the flag is unset (resulting in unchanged
data-loading
behavior for invisible charts).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 98:98
**Comment:**
*Logic Error: Configuration key naming inconsistency: most runtime
flags that are namespaced for Superset use the "SUPERSET_" prefix; if the
deployment sets this config as "SUPERSET_DASHBOARD_LOAD_DATA_VIRTUALIZATION"
(following convention) the frontend will not receive it because this code reads
"DASHBOARD_LOAD_DATA_VIRTUALIZATION". Rename the key so the tuple matches the
canonical config name used elsewhere.
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/config.py:
##########
@@ -696,6 +696,10 @@ class D3TimeFormat(TypedDict, total=False):
# @category: runtime_config
# @docs:
https://superset.apache.org/docs/using-superset/creating-your-first-dashboard
"DASHBOARD_RBAC": False,
+ # Supports simultaneous data and dashboard virtualization for backend
performance
+ # @lifecycle: stable
+ # @category: runtime_config
+ "DASHBOARD_LOAD_DATA_VIRTUALIZATION": False,
Review Comment:
**Suggestion:** Default mismatch risk: `DASHBOARD_VIRTUALIZATION` is enabled
by default (`True`), but the new `DASHBOARD_LOAD_DATA_VIRTUALIZATION` defaults
to `False`, which can produce surprising behavior (virtualized dashboards but
non-virtualized data loading). Set the default of the new flag to match the
dashboard virtualization default to avoid regressions or inconsistent defaults.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Excess backend queries for off-screen dashboard charts.
- ⚠️ Default behavior differs from PR description expectations.
```
</details>
```suggestion
# Default aligns with `DASHBOARD_VIRTUALIZATION` (enabled by default) to
avoid
# inconsistent behavior when virtualization is turned on for dashboards.
"DASHBOARD_LOAD_DATA_VIRTUALIZATION": True,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Deploy Superset with the default configuration (no overrides in
superset_config.py).
Open superset/config.py and inspect DEFAULT_FEATURE_FLAGS and the new flag
at lines
699-702.
2. Observe the existing default: "DASHBOARD_VIRTUALIZATION" is enabled by
default in
DEFAULT_FEATURE_FLAGS (the stable path-to-deprecation flag in the same
feature block),
while the new "DASHBOARD_LOAD_DATA_VIRTUALIZATION" is set to False at line
702.
3. Start the webserver and load a dashboard containing many charts. Because
DASHBOARD_VIRTUALIZATION is True, the frontend will virtualize rendering
(only render
visible chart DOM nodes), but because DASHBOARD_LOAD_DATA_VIRTUALIZATION is
False, backend
data requests for off-screen charts will still be initiated (per the PR
behavior change
description).
4. Observe increased unnecessary backend load (excess concurrent queries)
compared to the
intended behavior described in the PR. This demonstrates the default
mismatch: with
virtualization on by default, leaving the data-load optimization default off
is likely to
surprise operators.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 702:702
**Comment:**
*Logic Error: Default mismatch risk: `DASHBOARD_VIRTUALIZATION` is
enabled by default (`True`), but the new `DASHBOARD_LOAD_DATA_VIRTUALIZATION`
defaults to `False`, which can produce surprising behavior (virtualized
dashboards but non-virtualized data loading). Set the default of the new flag
to match the dashboard virtualization default to avoid regressions or
inconsistent defaults.
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]
