codeant-ai-for-open-source[bot] commented on code in PR #37452:
URL: https://github.com/apache/superset/pull/37452#discussion_r2742668344
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -192,7 +192,21 @@ class Chart extends PureComponent<ChartProps, {}> {
}
}
+ shouldRenderChart() {
+ return (
+ this.props.isInView ||
+ !isFeatureEnabled(FeatureFlag.DashboardVirtualization) ||
Review Comment:
**Suggestion:** The feature-flag checked inside `shouldRenderChart` is
wrong: it uses `FeatureFlag.DashboardVirtualization` while the runtime gating
in `runQuery` checks `FeatureFlag.DashboardVirtualizationDeferData`. This makes
the visibility logic inconsistent and can cause queries to run (or be deferred)
unexpectedly. Align `shouldRenderChart` to check
`FeatureFlag.DashboardVirtualizationDeferData`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unnecessary chart data loads via postChartFormData
- ⚠️ Higher backend concurrency and wasted DB queries
- ⚠️ Dashboard responsiveness degraded under load
```
</details>
```suggestion
!isFeatureEnabled(FeatureFlag.DashboardVirtualizationDeferData) ||
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the frontend with feature flags set so that
DashboardVirtualizationDeferData is
enabled and DashboardVirtualization is disabled (feature configuration
outside this file).
2. Open a dashboard where a Chart component is mounted off-screen (isInView
=== false).
The Chart component lifecycle calls runQuery() from componentDidMount at
`superset-frontend/src/components/Chart/Chart.tsx:183-187`.
3. In runQuery() at
`superset-frontend/src/components/Chart/Chart.tsx:203-209`, the code
checks `isFeatureEnabled(FeatureFlag.DashboardVirtualizationDeferData)` and
`!this.shouldRenderChart()` to decide whether to skip querying.
4. shouldRenderChart() at
`superset-frontend/src/components/Chart/Chart.tsx:195-201`
currently returns true because it checks
`!isFeatureEnabled(FeatureFlag.DashboardVirtualization)` (true when
DashboardVirtualization is disabled), so `!this.shouldRenderChart()` is
false and runQuery
proceeds to call `this.props.actions.postChartFormData(...)` at
`superset-frontend/src/components/Chart/Chart.tsx:210-217`, causing data to
load for an
out-of-view chart.
5. Observe that despite DashboardVirtualizationDeferData being enabled, data
is still
loaded for invisible charts due to the mismatched flag in
shouldRenderChart().
```
</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:** 198:198
**Comment:**
*Logic Error: The feature-flag checked inside `shouldRenderChart` is
wrong: it uses `FeatureFlag.DashboardVirtualization` while the runtime gating
in `runQuery` checks `FeatureFlag.DashboardVirtualizationDeferData`. This makes
the visibility logic inconsistent and can cause queries to run (or be deferred)
unexpectedly. Align `shouldRenderChart` to check
`FeatureFlag.DashboardVirtualizationDeferData`.
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:
##########
@@ -697,6 +697,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_VIRTUALIZATION_DEFER_DATA": False,
Review Comment:
**Suggestion:** The new flag is only set statically in the defaults; there
is no backward-compatible environment override for deployments that may already
use an older env var name. Read the environment variable (if present) so ops
can enable this flag via environment without modifying Python config files.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Operators using unprefixed env var won't enable flag.
- ⚠️ Dashboard deferred-loading remains disabled unexpectedly.
- ⚠️ Deployment docs may confuse ops about env var name.
```
</details>
```suggestion
"DASHBOARD_VIRTUALIZATION_DEFER_DATA": utils.parse_boolean_string(
os.environ.get("SUPERSET_FEATURE_DASHBOARD_VIRTUALIZATION_DEFER_DATA",
os.environ.get("DASHBOARD_VIRTUALIZATION_DEFER_DATA", "False"))
),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect superset/config.py and confirm the flag is defined at lines
700-703 with a
static False value.
2. Attempt to enable the flag by setting the unprefixed environment variable
DASHBOARD_VIRTUALIZATION_DEFER_DATA=True and start Superset with the same
codebase.
3. Observe that DEFAULT_FEATURE_FLAGS.update only reads environment
variables prefixed
with SUPERSET_FEATURE_ (see the DEFAULT_FEATURE_FLAGS.update(...) block in
superset/config.py), therefore the unprefixed
DASHBOARD_VIRTUALIZATION_DEFER_DATA is
ignored and the effective flag remains False at lines 700-703.
4. To enable the flag without changing Python code, set
SUPERSET_FEATURE_DASHBOARD_VIRTUALIZATION_DEFER_DATA=True (the supported
prefix), restart,
and observe the flag becomes True. This shows the code already supports
prefixed env vars;
the suggested change only adds convenience for an alternative env var naming
convention.
```
</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:** 703:703
**Comment:**
*Possible Bug: The new flag is only set statically in the defaults;
there is no backward-compatible environment override for deployments that may
already use an older env var name. Read the environment variable (if present)
so ops can enable this flag via environment without modifying Python config
files.
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]
