aminghadersohi commented on code in PR #45021:
URL: https://github.com/apache/superset/pull/45021#discussion_r4196992104
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -298,50 +324,68 @@ def _merge_filter_update(
existing: dict[str, Any],
dashboard_chart_ids: list[int],
) -> dict[str, Any]:
- """Merge a partial update into an existing filter config.
+ """Merge a partial update into an existing filter (or divider) config.
Returns a FULL filter config (the backend command substitutes whole
entries, it does not merge deltas).
"""
merged = copy.deepcopy(existing)
- _validate_update_type_compat(spec, merged.get("filterType"))
+ is_divider = merged.get("type") == "DIVIDER"
+ _validate_update_type_compat(spec, merged.get("filterType"),
is_divider=is_divider)
if spec.name is not None:
- merged["name"] = spec.name
+ # Dividers store their display text under "title", not "name";
+ # writing "name" here would silently fail to update what the
+ # filter bar actually renders.
+ if is_divider:
+ merged["title"] = spec.name
+ else:
+ merged["name"] = spec.name
if spec.description is not None:
merged["description"] = spec.description
if spec.scope_chart_ids is not None:
Review Comment:
P3: `scope_chart_ids` is still applied to dividers here. The frontend always
stores a divider with `excluded: []` and treats it as always in scope, so this
writes a scope the UI never produces (and resets on its next save). Consider
rejecting `scope_chart_ids` for `is_divider` along with the other type-specific
fields, and adding a test.
##########
tests/unit_tests/mcp_service/test_tool_inventory.py:
##########
@@ -123,7 +123,10 @@ def budgeted_bytes(text: str) -> int:
"manage_dashboard_certification": 1_900,
"manage_dashboard_owners": 2_200,
"manage_dashboard_roles": 1_900,
- "manage_native_filters": 9_100,
+ # Includes the divider filter type (title/description-only visual
+ # separator) added alongside filter_select/time/range/timegrain:
+ # 9,649 bytes, rounded up plus the standard 100-byte headroom.
+ "manage_native_filters": 9_800,
Review Comment:
P3: https://github.com/apache/superset/pull/44985 changes this line to 9_600
and also edits `_merge_filter_update`. Whichever PR merges second should
re-measure the budget for the combined schema and keep `_merge_select_default`
inside the `if not is_divider:` branch.
--
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]
