Repository navigation
feat(dashboard): add optional per-dashboard auto-refresh policy - #46037
DamianPendrak wants to merge 10 commits into
Conversation
Adds an AUTO_REFRESH_POLICY feature flag (off by default). When on, dashboard editors can allow or prohibit auto-refresh and bound viewer refresh intervals to the configured standard intervals; the policy is validated on every dashboard write path. With the flag off, behaviour is unchanged.
github-actions
Bot
added
i18n
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| slug: updates.slug, | ||
| description: updates.description, | ||
| metadata: JSON.parse(updates.jsonMetadata || '{}'), | ||
| metadata: applied, |
There was a problem hiding this comment.
Suggestion: The metadata editor omits shared_label_colors and map_label_colors; replacing Redux metadata here drops them when properties are applied, making chart label colors disappear.
Assessment: 🟠 Major · 🔁 Occurrence: Often · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/Header/index.tsx
**Line:** 609:609
**Comment:**
*Api Mismatch: The metadata editor omits `shared_label_colors` and `map_label_colors`; replacing Redux metadata here drops them when properties are applied, making chart label colors disappear.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| if isinstance(value, bool): | ||
| return None | ||
| if isinstance(value, (int, float)): | ||
| seconds = float(value) |
There was a problem hiding this comment.
Suggestion: A sufficiently large integer in the configured intervals makes float(value) raise OverflowError, causing policy validation to fail with a server error.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Possible bug
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/dashboards/refresh_policy.py
**Line:** 75:75
**Comment:**
*Possible Bug: A sufficiently large integer in the configured intervals makes `float(value)` raise `OverflowError`, causing policy validation to fail with a server error.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b91d79a.
_to_seconds now catches OverflowError from float(value) and returns None for values that cannot be converted.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of a32c793.
_to_seconds now catches OverflowError from float(value) and returns None for values that cannot be converted.
If that's not right, unresolve this thread and CodeAnt will leave it open.
| ) | ||
|
|
||
| # The generic assets error mapping is maintained separately. | ||
| assert response.status_code in {422, 500} |
There was a problem hiding this comment.
Suggestion: Accepting HTTP 500 lets this test pass when invalid-policy import crashes, so it does not verify the promised 422 validation response.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/dashboards/commands/importers/v1/refresh_policy_import_test.py
**Line:** 165:165
**Comment:**
*Api Mismatch: Accepting HTTP 500 lets this test pass when invalid-policy import crashes, so it does not verify the promised 422 validation response.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| metadata[POLICY_KEY] = policy; | ||
| } | ||
| if (policyOn) { | ||
| metadata.refresh_frequency = frequency; |
There was a problem hiding this comment.
Suggestion: When the policy is off, Advanced JSON edits do not update refreshFrequency; enabling the policy writes that stale select value here and discards the edited default.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Incorrect variable usage
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
**Line:** 679:679
**Comment:**
*Incorrect Variable Usage: When the policy is off, Advanced JSON edits do not update `refreshFrequency`; enabling the policy writes that stale select value here and discards the edited default.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| (minimum === null || value >= minimum) && | ||
| (maximum === null || value <= maximum), |
There was a problem hiding this comment.
Suggestion: A malformed bound saved while the feature flag was off becomes null here and is treated as unbounded, letting viewers refresh outside the intended policy.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Incorrect condition logic
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx
**Line:** 206:207
**Comment:**
*Incorrect Condition Logic: A malformed bound saved while the feature flag was off becomes `null` here and is treated as unbounded, letting viewers refresh outside the intended policy.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #46037 +/- ##
==========================================
+ Coverage 65.35% 65.42% +0.07%
==========================================
Files 3083 3086 +3
Lines 211328 212212 +884
Branches 49818 50165 +347
==========================================
+ Hits 138105 138832 +727
- Misses 70041 70184 +143
- Partials 3182 3196 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
requires:rebase
…frequency group Refresh settings errors sat in a polite live region, and the refresh frequency radio group had no aria-invalid while its error showed. The errors are now in alert regions, and the group carries aria-invalid with its error.
An invalid refresh policy only disabled Save in Dashboard Properties, so a keyboard user could never try to save and focus never reached the field. With refresh setting errors alone, Save stays enabled: pressing it opens Refresh settings, focuses the first invalid field and saves nothing. Other section errors still disable Save.
SUMMARY
Adds an optional per-dashboard auto-refresh policy behind a new feature flag,
AUTO_REFRESH_POLICY(defaultFalse). With the flag off, policy metadata is accepted and stored without validation (includingnull), but ignored by refresh behavior.With the flag on, dashboard editors can turn on Use a refresh policy in Dashboard properties → Refresh settings and set:
DASHBOARD_AUTO_REFRESH_INTERVALS. Viewers can only pick intervals inside that range, and the default refresh frequency must be inside it too.The policy is stored in
json_metadataunderauto_refresh_policy:{ "refresh_frequency": 60, "auto_refresh_policy": { "auto_refresh_allowed": true, "minimum_refresh_interval": {"value": 30, "unit": "seconds"}, "maximum_refresh_interval": {"value": 1, "unit": "hours"} } }With the flag on, dashboard API writes, copy, imports, MCP tools and version restore validate refresh settings. REST dashboard endpoints and version restore return field errors as an
errorslist of{field, code, message}. Imports return ordinary validation messages; MCP returns an error string. REST PUT, Dashboard properties saves, MCP updates and version restore validate refresh rules only when the policy or refresh frequency changes; title-only, Published/Draft and layout saves preserve unchanged refresh settings even if an operator has removed their default interval from the list.An update that omits
auto_refresh_policykeeps the stored policy. With the flag on,nullremoves it and policy changes are recorded in the existingLogtable asdashboard_refresh_policy_changed. No DB migration is required. With a policy, viewers cannot select Custom intervals and Custom defaults are rejected.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
FEATURE_FLAGS = {"AUTO_REFRESH_POLICY": True}and restart.?standalone=true): no automatic refresh.PUT /api/v1/dashboard/<id>withjson_metadatacontaining arefresh_frequencybelowminimum_refresh_interval→ 422 with anerrorsentry{"field": "json_metadata.refresh_frequency", "code": "below_minimum", ...}. A PUT that omitsauto_refresh_policykeeps it; sendingnullremoves it with the flag on.Falseand restart: Refresh settings and the header menu are stock again, and saves succeed with the stored policy left injson_metadata.Automated:
TZ=UTC pytest tests/unit_tests/dashboards tests/unit_tests/mcp_service/dashboard tests/unit_tests/subjects/test_creator_group_call_sites.pyand, insuperset-frontend,npm run test -- --maxWorkers=2 src/dashboard/components/PropertiesModal src/dashboard/components/RefreshFrequency src/dashboard/components/Header src/dashboard/components/RefreshIntervalModal src/dashboard/actions/hydrate src/dashboard/components/DashboardBuilder src/dashboard/reducers/dashboardState.ADDITIONAL INFORMATION
AUTO_REFRESH_POLICY(new, defaultFalse)