codeant-ai-for-open-source[bot] commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3664645390
##########
superset/daos/dashboard.py:
##########
@@ -396,15 +396,28 @@ def set_dash_metadata(
else:
md["color_namespace"] = data.get("color_namespace")
- md["expanded_slices"] = data.get("expanded_slices", {})
- if "refresh_frequency" in data:
- md["refresh_frequency"] = data["refresh_frequency"]
- md["color_scheme"] = data.get("color_scheme", "")
- md["label_colors"] = data.get("label_colors", {})
- md["shared_label_colors"] = data.get("shared_label_colors", [])
- md["map_label_colors"] = data.get("map_label_colors", {})
- md["color_scheme_domain"] = data.get("color_scheme_domain", [])
- md["cross_filters_enabled"] = data.get("cross_filters_enabled", True)
+ # Only overwrite these metadata fields when the caller explicitly sends
+ # them. Previously each used ``data.get(key, default)``, which reset a
+ # value to its default whenever it was absent from the payload -- e.g.
a
+ # ``refresh_frequency`` set directly in the Advanced JSON editor got
+ # wiped on save. ``setdefault`` still seeds a default for brand-new
+ # dashboards that have never had the key, keeping the shape stable
+ # without clobbering existing values (#42116).
+ metadata_defaults: dict[str, Any] = {
+ "expanded_slices": {},
+ "refresh_frequency": 0,
+ "color_scheme": "",
+ "label_colors": {},
+ "shared_label_colors": [],
+ "map_label_colors": {},
+ "color_scheme_domain": [],
+ "cross_filters_enabled": True,
+ }
+ for key, default_value in metadata_defaults.items():
+ if key in data:
+ md[key] = data[key]
+ else:
+ md.setdefault(key, default_value)
Review Comment:
**Suggestion:** The new fallback preserves derived metadata fields when they
are absent from the payload, but the Properties modal intentionally omits
`shared_label_colors`, `map_label_colors`, and `color_scheme_domain` from its
JSON payload. Consequently, changing dashboard label colors or schemes can
leave stale derived color mappings and domains in stored metadata instead of
clearing or regenerating them. Preserve only fields that are genuinely
partial-update fields, or regenerate these derived fields from the submitted
metadata. [stale reference]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Saved color changes can retain stale derived dashboard styling.
- ⚠️ Chart rendering may use mappings from the previous color scheme.
- ⚠️ Stored metadata diverges from frontend-generated color state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load Dashboard Properties for an existing dashboard.
`handleDashboardData()`
intentionally removes `shared_label_colors`, `map_label_colors`, and
`color_scheme_domain`
before populating the JSON editor at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:203-211`.
2. Change the dashboard color scheme or custom label colors and save through
the PUT
request constructed at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:374-402`;
the
submitted `json_metadata` therefore omits those three derived fields.
3. The update command parses that payload and calls
`DashboardDAO.set_dash_metadata()` at
`superset/commands/dashboard/update.py:32-37`.
4. At `superset/daos/dashboard.py:416-420`, omitted derived fields now use
`md.setdefault()` instead of being replaced, so existing
`color_scheme_domain`,
`shared_label_colors`, and `map_label_colors` remain associated with the
previous styling.
5. Subsequent dashboard state consumers read these stored values, including
`Chart.tsx:429-437` and `dashboardState.ts:1258-1264`, allowing stale
mappings or domains
to affect later dashboard rendering despite the newly saved styling.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5e9ce711a5f74dd885ea47946d1391a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5e9ce711a5f74dd885ea47946d1391a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dashboard.py
**Line:** 416:420
**Comment:**
*Stale Reference: The new fallback preserves derived metadata fields
when they are absent from the payload, but the Properties modal intentionally
omits `shared_label_colors`, `map_label_colors`, and `color_scheme_domain` from
its JSON payload. Consequently, changing dashboard label colors or schemes can
leave stale derived color mappings and domains in stored metadata instead of
clearing or regenerating them. Preserve only fields that are genuinely
partial-update fields, or regenerate these derived fields from the submitted
metadata.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42142&comment_hash=ae5fbc69a925b09ef4a8e758cbedd150b699fc859ea548ad2197213b46d11f83&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42142&comment_hash=ae5fbc69a925b09ef4a8e758cbedd150b699fc859ea548ad2197213b46d11f83&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -537,8 +542,14 @@ const PropertiesModal = ({
// Section handlers for extracted components
const handleThemeChange = (value: any) => setSelectedThemeId(value || null);
- const handleRefreshFrequencyChange = (value: number) =>
+ const handleRefreshFrequencyChange = (value: number) => {
setRefreshFrequency(value);
+ // Keep the Advanced JSON editor in sync with the dropdown so the two
+ // sources can't diverge, mirroring onColorSchemeChange (#42116).
+ const jsonMetadataObj = getJsonMetadata();
+ jsonMetadataObj.refresh_frequency = value;
+ setJsonMetadata(jsonStringify(jsonMetadataObj));
Review Comment:
**Suggestion:** `getJsonMetadata()` returns an empty object when the JSON
editor currently contains invalid JSON. If the user changes the refresh
dropdown while editing invalid JSON, this handler replaces the entire editor
contents with an object containing only `refresh_frequency`, silently
discarding the user's in-progress metadata edit. Update the parsed state only
when parsing succeeds, or preserve the raw editor text while it is invalid.
[error handling]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ In-progress JSON metadata edits are silently discarded.
- ⚠️ Dropdown interaction destroys unrelated advanced metadata text.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Dashboard Properties and edit the Advanced JSON editor rendered by
`AdvancedSection` at
`superset-frontend/src/dashboard/components/PropertiesModal/sections/AdvancedSection.tsx:78-89`,
leaving the text temporarily invalid JSON while editing.
2. Change the Refresh dropdown, which invokes
`handleRefreshFrequencyChange()` at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:545-552`.
3. `getJsonMetadata()` at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:241-260`
catches the
parse error and returns an empty object.
4. Lines 549-551 then assign only `refresh_frequency` to that empty object
and call
`setJsonMetadata(jsonStringify(jsonMetadataObj))`, replacing the user's
invalid
in-progress text and all other metadata with a one-field valid object before
the user can
finish the edit.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9b68592ca80b43dca2f22b307ea50050&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9b68592ca80b43dca2f22b307ea50050&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/PropertiesModal/index.tsx
**Line:** 549:551
**Comment:**
*Error Handling: `getJsonMetadata()` returns an empty object when the
JSON editor currently contains invalid JSON. If the user changes the refresh
dropdown while editing invalid JSON, this handler replaces the entire editor
contents with an object containing only `refresh_frequency`, silently
discarding the user's in-progress metadata edit. Update the parsed state only
when parsing succeeds, or preserve the raw editor text while it is invalid.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42142&comment_hash=d8f8235586c1d4309e1b496e993529431fcb3594ec52ad8cfa51c3dfd2ab0494&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42142&comment_hash=d8f8235586c1d4309e1b496e993529431fcb3594ec52ad8cfa51c3dfd2ab0494&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -343,7 +343,12 @@ const PropertiesModal = ({
? resettableCustomLabels
: false;
const jsonMetadataObj = getJsonMetadata();
- jsonMetadataObj.refresh_frequency = refreshFrequency;
+ // A refresh_frequency edited directly in the Advanced JSON editor takes
+ // precedence over the Refresh dropdown state, mirroring how color_scheme
is
+ // handled above. Nullish coalescing preserves an explicit 0 ("Don't
+ // refresh") rather than falling through to the dropdown value (#42116).
+ jsonMetadataObj.refresh_frequency =
+ jsonMetadataObj.refresh_frequency ?? refreshFrequency;
Review Comment:
**Suggestion:** Refresh validation still uses the separate
`refreshFrequency` state, but this assignment makes the JSON editor's value
authoritative. A user can enter a value below
`SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT` in the JSON editor while leaving
the dropdown unchanged, causing validation to pass and the invalid value to be
saved. Validate the effective `jsonMetadataObj.refresh_frequency` value
instead. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Advanced JSON edits can save intervals below the configured minimum.
- ⚠️ Dashboard refresh behavior violates configured refresh-frequency policy.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Dashboard Properties through the existing dashboard editing flow and
expand the
Advanced settings editor implemented by `AdvancedSection` at
`superset-frontend/src/dashboard/components/PropertiesModal/sections/AdvancedSection.tsx:63-89`.
2. In an installation whose `SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT` is
positive,
leave the Refresh dropdown at its existing valid value and edit
`refresh_frequency` in the
JSON editor to a positive value below the configured minimum.
3. The refresh validator at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:589-595`
validates
the separate `refreshFrequency` state rather than the JSON value, so it
reports no error
when the dropdown state is valid.
4. Saving reaches `onFinish()` at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:345-351`,
which
preserves the JSON-edited value and submits it at lines 374-402; the invalid
interval is
therefore persisted without the configured minimum validation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=66a606f93b67416ba606fd982f28e535&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=66a606f93b67416ba606fd982f28e535&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/PropertiesModal/index.tsx
**Line:** 350:351
**Comment:**
*Incorrect Condition Logic: Refresh validation still uses the separate
`refreshFrequency` state, but this assignment makes the JSON editor's value
authoritative. A user can enter a value below
`SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT` in the JSON editor while leaving
the dropdown unchanged, causing validation to pass and the invalid value to be
saved. Validate the effective `jsonMetadataObj.refresh_frequency` value instead.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42142&comment_hash=9c989ec368735743db1de5278c6c8e63084f1ff1aa47a314fd761740d60885d3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42142&comment_hash=9c989ec368735743db1de5278c6c8e63084f1ff1aa47a314fd761740d60885d3&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]
