Repository navigation
feat(dashboard): add optional per-dashboard auto-refresh policy - #46037
DamianPendrak wants to merge 6 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.
| ) | ||
|
|
||
| # 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.38% 85.10% +19.71%
===========================================
Files 3080 3083 +3
Lines 209437 210305 +868
Branches 49385 49719 +334
===========================================
+ Hits 136950 178973 +42023
+ Misses 69350 27954 -41396
- Partials 3137 3378 +241
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:
|
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)