codeant-ai-for-open-source[bot] commented on code in PR #40995:
URL: https://github.com/apache/superset/pull/40995#discussion_r3402228980
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -228,6 +248,48 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = 0;
}
+ const visibleColumnKeys = Array.isArray(ownState?.visibleColumns)
+ ? ownState.visibleColumns.map(String)
+ : [];
+ const visibleColumnSet = new Set(visibleColumnKeys);
+
+ let underlyingColumnSet = visibleColumnSet;
+
+ if (isDownloadQuery && visibleColumnSet.size > 0) {
+ if (isTimeComparison(formData, baseQueryObject)) {
+ underlyingColumnSet = new Set(
+ Array.from(visibleColumnSet).map(key => {
+ if (key.startsWith('Main ')) return key.substring(5);
+ if (key.startsWith('# ')) return key.substring(2);
+ if (key.startsWith('△ ')) return key.substring(2);
+ if (key.startsWith('% ')) return key.substring(2);
+ return key;
+ }),
+ );
+ }
+
+ columns = columns.filter(column =>
+ underlyingColumnSet.has(
+ getColumnSelectionKey(column as string | AdhocColumn),
+ ),
+ );
+
+ if (Array.isArray(metrics) && metrics.length > 0) {
+ metrics = metrics.filter(metric =>
+ underlyingColumnSet.has(getMetricLabel(metric)),
+ );
Review Comment:
**🟠 Architect Review — HIGH**
Download-query filtering uses `visibleColumns` keys directly against
`getMetricLabel(metric)`, but percent-metric result columns are keyed as
`%<metric>` while their metric labels are `<metric>`. If a user hides all base
metrics and keeps only percent columns visible, the download query filters
`metrics`/`orderby` down to none of the percent metrics' underlying bases,
causing the contribution post-processing to lack required source metrics and
leading to broken or incomplete exports.
**Suggestion:** When building `underlyingColumnSet` for download queries,
normalize percent-metric column keys (e.g., `%foo` → `foo`) in addition to the
time-comparison prefixes, and use that normalized set when filtering `metrics`
and `orderby`, so percent metrics remain backed by their base metrics even when
only the `%` columns are visible.
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=687188bda86646b0bbbffc099e09ae8d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=687188bda86646b0bbbffc099e09ae8d&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 an **Architect / Logical Review** comment left during a code review.
These reviews are first-class, important findings — not optional suggestions.
Do NOT dismiss this as a 'big architectural change' just because the title says
architect review; most of these can be resolved with a small, localized fix
once the intent is understood.
**Path:** superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts
**Line:** 258:280
**Comment:**
*HIGH: Download-query filtering uses `visibleColumns` keys directly
against `getMetricLabel(metric)`, but percent-metric result columns are keyed
as `%<metric>` while their metric labels are `<metric>`. If a user hides all
base metrics and keeps only percent columns visible, the download query filters
`metrics`/`orderby` down to none of the percent metrics' underlying bases,
causing the contribution post-processing to lack required source metrics and
leading to broken or incomplete exports.
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.
If a suggested approach is provided above, use it as the authoritative
instruction. If no explicit code suggestion is given, you MUST still draft and
apply your own minimal, localized fix — do not punt back with 'no suggestion
provided, review manually'. Keep the change as small as possible: add a guard
clause, gate on a loading state, reorder an await, wrap in a conditional, etc.
Do not refactor surrounding code or expand scope beyond the finding.
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>
##########
superset/mcp_service/dashboard/tool/add_native_filter.py:
##########
@@ -0,0 +1,501 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+MCP tool: add_native_filter
+
+This tool adds a native filter to an existing dashboard by updating
+the dashboard's json_metadata.native_filter_configuration.
+"""
+
+import logging
+import uuid
+from datetime import datetime, timezone
+from typing import Any, Dict, List, Literal
+
+from fastmcp import Context
+from pydantic import BaseModel, Field
+from sqlalchemy.exc import SQLAlchemyError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.commands.exceptions import CommandException
+from superset.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardError,
+ DashboardInfo,
+ NativeFilterSummary,
+)
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+
+# ---------------------------------------------------------------------------
+# Request / Response schemas
+# ---------------------------------------------------------------------------
+
+
+class AddNativeFilterRequest(BaseModel):
+ """Request schema for adding a native filter to a dashboard."""
+
+ dashboard_id: int = Field(
+ ..., description="ID of the dashboard to add the filter to"
+ )
+ filter_name: str = Field(
+ ...,
+ description="Display name for the filter (e.g. 'Ngành hàng', 'Thị
trường')",
+ )
+ filter_type: Literal[
+ "filter_select",
+ "filter_range",
+ "filter_time",
+ "filter_timecolumn",
+ "filter_timegrain",
+ ] = Field(
+ ...,
+ description=(
+ "Type of native filter to create. "
+ "filter_select: dropdown/multi-select for categorical columns. "
+ "filter_range: numeric range slider. "
+ "filter_time: date/time range picker. "
+ "filter_timecolumn: select which time column to use. "
+ "filter_timegrain: select time grain (day/week/month)."
+ ),
+ )
+ dataset_id: int = Field(
+ ..., description="ID of the dataset the filter targets"
+ )
+ column_name: str = Field(
+ ...,
+ description="Name of the column in the dataset to filter on",
+ )
+ default_value: Any = Field(
+ default=None,
+ description=(
+ "Default selected value(s) for the filter. "
+ "For filter_select with multiple_select=True, use a list. "
+ "For filter_range, use a dict like {'min': 0, 'max': 100}."
+ ),
+ )
+ parent_filter_ids: List[str] = Field(
+ default_factory=list,
+ description=(
+ "List of parent native filter IDs for cascading filters. "
+ "The parent filters must already exist on the dashboard."
+ ),
+ )
+ scope_type: Literal["full", "specific"] = Field(
+ default="full",
+ description=(
+ "Filter scope. 'full' applies the filter to all charts on the "
+ "dashboard (default). 'specific' applies only to charts NOT in "
+ "excluded_chart_ids."
+ ),
+ )
+ excluded_chart_ids: List[int] = Field(
+ default_factory=list,
+ description=(
+ "Chart IDs to exclude from this filter's scope. "
+ "Only used when scope_type='specific'."
+ ),
+ )
+ enable_search: bool = Field(
+ default=True,
+ description="Enable search in select filter dropdown (filter_select
only)",
+ )
+ multiple_select: bool = Field(
+ default=True,
+ description="Allow selecting multiple values (filter_select only)",
+ )
+
+
+class AddNativeFilterResponse(BaseModel):
+ """Response schema for adding a native filter."""
+
+ filter_id: str | None = Field(
+ None, description="ID of the newly created native filter"
+ )
+ filter_name: str | None = Field(
+ None, description="Display name of the created filter"
+ )
+ filter_type: str | None = Field(
+ None, description="Type of the created filter"
+ )
+ native_filters: List[NativeFilterSummary] = Field(
+ default_factory=list,
+ description="Updated list of all native filters on the dashboard",
+ )
+ dashboard_id: int | None = Field(
+ None, description="ID of the updated dashboard"
+ )
+ dashboard_url: str | None = Field(
+ None, description="URL to view the updated dashboard"
+ )
+ error: str | None = Field(
+ None, description="Error message if operation failed"
+ )
+
+
+# ---------------------------------------------------------------------------
+# Helper functions
+# ---------------------------------------------------------------------------
+
+
+def _generate_filter_id() -> str:
+ """Generate a unique native filter ID matching Superset's frontend
format."""
+ return f"NATIVE_FILTER-{uuid.uuid4().hex[:20]}"
+
+
+def _build_scope(
+ scope_type: str,
+ excluded_chart_ids: List[int],
+) -> Dict[str, Any]:
+ """Build the scope object for a native filter.
+
+ Args:
+ scope_type: 'full' for all charts, 'specific' for excluding some.
+ excluded_chart_ids: Chart IDs to exclude when scope_type='specific'.
+
+ Returns:
+ Scope dict compatible with Superset's native filter format.
+ """
+ if scope_type == "specific" and excluded_chart_ids:
+ return {
+ "rootPath": ["ROOT_ID"],
+ "excluded": excluded_chart_ids,
+ }
+ # Default: apply to all charts
+ return {
+ "rootPath": ["ROOT_ID"],
+ "excluded": [0],
+ }
+
+
+def _build_control_values(
+ filter_type: str,
+ enable_search: bool,
+ multiple_select: bool,
+) -> Dict[str, Any]:
+ """Build controlValues for the filter based on its type."""
+ if filter_type == "filter_select":
+ return {
+ "enableEmptyFilter": False,
+ "defaultToFirstItem": False,
+ "multiSelect": multiple_select,
+ "searchAllOptions": enable_search,
+ "inverseSelection": False,
+ }
+ elif filter_type == "filter_range":
+ return {
+ "enableEmptyFilter": False,
+ }
+ elif filter_type in ("filter_time", "filter_timecolumn",
"filter_timegrain"):
+ return {}
+ return {}
+
+
+def _build_default_data_mask(
+ filter_type: str,
+ default_value: Any,
+) -> Dict[str, Any]:
+ """Build defaultDataMask for the filter."""
+ if default_value is None:
+ return {
+ "filterState": {},
+ "extraFormData": {},
+ }
+
+ if filter_type == "filter_select":
+ # default_value should be a list or single value
+ values = default_value if isinstance(default_value, list) else
[default_value]
+ return {
+ "filterState": {"value": values},
+ "extraFormData": {
+ "filters": [
+ {
+ "col": None, # Will be set by Superset frontend
+ "op": "IN",
+ "val": values,
+ }
+ ]
+ },
+ }
+ elif filter_type == "filter_range":
+ # default_value should be dict with min/max
+ filter_state = {}
+ if isinstance(default_value, dict):
+ filter_state = {"value": [default_value.get("min"),
default_value.get("max")]}
+ return {
+ "filterState": filter_state,
+ "extraFormData": {},
+ }
+ elif filter_type == "filter_time":
+ return {
+ "filterState": {"value": default_value},
+ "extraFormData": {"time_range": default_value},
+ }
+ return {
+ "filterState": {},
+ "extraFormData": {},
+ }
+
+
+def _build_native_filter_config(
+ request: AddNativeFilterRequest,
+ filter_id: str,
+) -> Dict[str, Any]:
+ """Build the complete native filter configuration dict."""
+ return {
+ "id": filter_id,
+ "name": request.filter_name,
+ "filterType": request.filter_type,
+ "targets": [
+ {
+ "datasetId": request.dataset_id,
+ "column": {"name": request.column_name},
+ }
+ ],
+ "scope": _build_scope(request.scope_type, request.excluded_chart_ids),
+ "controlValues": _build_control_values(
+ request.filter_type, request.enable_search, request.multiple_select
+ ),
+ "defaultDataMask": _build_default_data_mask(
+ request.filter_type, request.default_value
+ ),
+ "cascadeParentIds": list(request.parent_filter_ids),
+ "type": "NATIVE_FILTER",
+ "description": "",
+ "chartsInScope": [],
+ "tabsInScope": [],
+ }
+
+
+def _extract_native_filter_summaries(
+ native_filter_configuration: List[Dict[str, Any]],
+) -> List[NativeFilterSummary]:
+ """Convert raw native filter configs to NativeFilterSummary objects."""
+ summaries = []
+ for f in native_filter_configuration:
+ if not isinstance(f, dict):
+ continue
+ raw_targets = f.get("targets", [])
+ if not isinstance(raw_targets, list):
+ raw_targets = []
+ targets = [t for t in raw_targets if isinstance(t, dict)]
+ summaries.append(
+ NativeFilterSummary(
+ id=f.get("id"),
+ name=f.get("name"),
+ filter_type=f.get("filterType"),
+ targets=targets,
+ )
+ )
+ return summaries
+
+
+# ---------------------------------------------------------------------------
+# MCP Tool
+# ---------------------------------------------------------------------------
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ annotations=ToolAnnotations(
+ title="Add native filter to dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def add_native_filter(
+ request: AddNativeFilterRequest, ctx: Context
+) -> AddNativeFilterResponse:
+ """Add a native filter to an existing dashboard.
+
+ Creates a new native filter (dropdown, range slider, time picker, etc.)
+ on the specified dashboard. The filter targets a specific column in a
+ dataset and can be scoped to all charts or specific charts.
+
+ Supported filter types:
+ - filter_select: Dropdown/multi-select for categorical columns
+ - filter_range: Numeric range slider
+ - filter_time: Date/time range picker
+ - filter_timecolumn: Select which time column to use
+ - filter_timegrain: Select time grain (day/week/month)
+
+ Example usage:
+ ```json
+ {
+ "dashboard_id": 2,
+ "filter_name": "Ngành hàng",
+ "filter_type": "filter_select",
+ "dataset_id": 38,
+ "column_name": "nganh_hang"
+ }
+ ```
+
+ With cascading filters:
+ ```json
+ {
+ "dashboard_id": 2,
+ "filter_name": "Thị trường",
+ "filter_type": "filter_select",
+ "dataset_id": 38,
+ "column_name": "market_type",
+ "parent_filter_ids": ["NATIVE_FILTER-abc123"]
+ }
+ ```
+ """
+ logger.info(
+ "Adding native filter to dashboard: dashboard_id=%s, filter_name=%s, "
+ "filter_type=%s, dataset_id=%s, column=%s",
+ request.dashboard_id,
+ request.filter_name,
+ request.filter_type,
+ request.dataset_id,
+ request.column_name,
+ )
+
+ try:
+ from superset import db, security_manager
+ from superset.commands.dashboard.update import UpdateDashboardCommand
+ from superset.daos.dashboard import DashboardDAO
+ from superset.exceptions import SupersetSecurityException
+
+ # --- Step 1: Validate dashboard exists and user has permission ---
+ with
event_logger.log_context(action="mcp.add_native_filter.validation"):
+ dashboard = DashboardDAO.find_by_id(request.dashboard_id)
+ if not dashboard:
+ return AddNativeFilterResponse(
+ error=f"Dashboard with ID {request.dashboard_id} not found"
+ )
+
+ try:
+ security_manager.raise_for_ownership(dashboard)
+ except SupersetSecurityException:
+ return AddNativeFilterResponse(
+ error=(
+ f"You don't have permission to edit dashboard "
+ f"'{dashboard.dashboard_title}' (ID:
{request.dashboard_id}). "
+ "Only dashboard owners can add filters."
+ )
+ )
+
+ # --- Step 2: Validate dataset exists ---
+ from superset.models.slice import Slice
+ from superset.connectors.sqla.models import SqlaTable
+
+ dataset = db.session.get(SqlaTable, request.dataset_id)
+ if not dataset:
+ return AddNativeFilterResponse(
+ error=f"Dataset with ID {request.dataset_id} not found"
+ )
Review Comment:
**🟠 Architect Review — HIGH**
The `add_native_filter` MCP tool only checks that the referenced dataset
exists via `SqlaTable`, but never verifies that the current user has data-level
access to that dataset before persisting the filter config, unlike sibling MCP
chart tools which call `has_dataset_access`. This allows a user with dashboard
edit rights but without dataset access to create filters targeting unauthorized
datasets.
**Suggestion:** After loading the `SqlaTable`, reuse `has_dataset_access`
(as in `generate_chart`) to enforce dataset-level permissions and return a
clear authorization error if the user cannot read that dataset, keeping MCP
mutate tools' authorization behavior consistent.
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8d99b6cc858d49d4ac2a759bb016e1d0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8d99b6cc858d49d4ac2a759bb016e1d0&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 an **Architect / Logical Review** comment left during a code review.
These reviews are first-class, important findings — not optional suggestions.
Do NOT dismiss this as a 'big architectural change' just because the title says
architect review; most of these can be resolved with a small, localized fix
once the intent is understood.
**Path:** superset/mcp_service/dashboard/tool/add_native_filter.py
**Line:** 399:407
**Comment:**
*HIGH: The `add_native_filter` MCP tool only checks that the referenced
dataset exists via `SqlaTable`, but never verifies that the current user has
data-level access to that dataset before persisting the filter config, unlike
sibling MCP chart tools which call `has_dataset_access`. This allows a user
with dashboard edit rights but without dataset access to create filters
targeting unauthorized datasets.
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.
If a suggested approach is provided above, use it as the authoritative
instruction. If no explicit code suggestion is given, you MUST still draft and
apply your own minimal, localized fix — do not punt back with 'no suggestion
provided, review manually'. Keep the change as small as possible: add a guard
clause, gate on a loading state, reorder an await, wrap in a conditional, etc.
Do not refactor surrounding code or expand scope beyond the finding.
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>
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -737,13 +764,289 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
);
};
- // Compute visible columns before groupHeaderColumns to ensure index
consistency.
- // This filters out columns with config.visible === false.
- const visibleColumnsMeta = useMemo(
+ const selectableColumnsMeta = useMemo(
() => filteredColumnsMeta.filter(col => col.config?.visible !== false),
[filteredColumnsMeta],
);
+ const selectableColumnKeys = useMemo(
+ () => selectableColumnsMeta.map(col => String(col.key)),
+ [selectableColumnsMeta],
+ );
+
+ const visibleColumnsStorageKey = useMemo(() => {
+ if (typeof window === 'undefined') {
+ return null;
+ }
+ const pathname = window.location?.pathname || '';
+ if (!pathname.includes(DASHBOARD_PATH_SEGMENT)) {
+ return null;
+ }
+ return `superset.table.visibleColumns:${pathname}:${slice_id}`;
+ }, [slice_id]);
+
+ const storedVisibleColumnKeys = useMemo(() => {
+ if (!visibleColumnsStorageKey || typeof window === 'undefined') {
+ return [];
+ }
+ try {
+ const raw = window.localStorage.getItem(visibleColumnsStorageKey);
+ if (!raw) {
+ return [];
+ }
+ const parsed = JSON.parse(raw);
+ return sanitizeVisibleColumnSelection(
+ parsed,
+ new Set(selectableColumnKeys),
+ );
+ } catch {
+ return [];
+ }
+ }, [selectableColumnKeys, visibleColumnsStorageKey]);
+
+ const [selectedVisibleColumnKeys, setSelectedVisibleColumnKeys] = useState<
+ string[]
+ >(() => {
+ const availableKeys = new Set(selectableColumnKeys);
+ const selectedFromOwnState = sanitizeVisibleColumnSelection(
+ serverPaginationData?.visibleColumns,
+ availableKeys,
+ );
+ if (selectedFromOwnState.length > 0) {
+ return selectedFromOwnState;
+ }
+ if (storedVisibleColumnKeys.length > 0) {
+ return storedVisibleColumnKeys;
+ }
+ return selectableColumnKeys;
+ });
+
+ const [localVisibleColumnKeys, setLocalVisibleColumnKeys] =
useState<string[]>(selectedVisibleColumnKeys);
+
+ useEffect(() => {
+ setSelectedVisibleColumnKeys(prevSelection => {
+ const availableKeys = new Set(selectableColumnKeys);
+ const sanitizedCurrent = prevSelection.filter(key =>
+ availableKeys.has(key),
+ );
+
+ const selectedFromOwnState = sanitizeVisibleColumnSelection(
+ serverPaginationData?.visibleColumns,
+ availableKeys,
+ );
+ if (selectedFromOwnState.length > 0) {
+ return isEqual(sanitizedCurrent, selectedFromOwnState)
+ ? sanitizedCurrent
+ : selectedFromOwnState;
+ }
+
+ if (storedVisibleColumnKeys.length > 0) {
+ return isEqual(sanitizedCurrent, storedVisibleColumnKeys)
+ ? sanitizedCurrent
+ : storedVisibleColumnKeys;
+ }
+
+ if (sanitizedCurrent.length > 0) {
+ return sanitizedCurrent;
+ }
+
+ return selectableColumnKeys;
+ });
+ }, [
+ selectableColumnKeys,
+ serverPaginationData?.visibleColumns,
+ storedVisibleColumnKeys,
+ ]);
+
+ const persistTableOwnState = useCallback(
+ (updates: Record<string, unknown>) => {
+ const selectedKeys =
+ (updates.visibleColumns as string[] | undefined) ??
+ selectedVisibleColumnKeys;
+ if (visibleColumnsStorageKey && typeof window !== 'undefined') {
+ try {
+ if (selectedKeys?.length) {
+ window.localStorage.setItem(
+ visibleColumnsStorageKey,
+ JSON.stringify(selectedKeys),
+ );
+ } else {
+ window.localStorage.removeItem(visibleColumnsStorageKey);
+ }
+ } catch {
+ // no-op: storage write failures should not block table interactions
+ }
+ }
+ updateTableOwnState(setDataMask, {
+ ...serverPaginationData,
+ ...updates,
+ ...(selectedKeys?.length ? { visibleColumns: selectedKeys } : {}),
+ });
+ },
+ [
+ selectedVisibleColumnKeys,
+ serverPaginationData,
+ setDataMask,
+ visibleColumnsStorageKey,
+ ],
+ );
+
+ useEffect(() => {
+ if (storedVisibleColumnKeys.length === 0) {
+ return;
+ }
+ const selectedFromOwnState = sanitizeVisibleColumnSelection(
+ serverPaginationData?.visibleColumns,
+ new Set(selectableColumnKeys),
+ );
+ if (
+ selectedFromOwnState.length === 0 &&
+ selectedVisibleColumnKeys.length > 0
+ ) {
+ persistTableOwnState({ visibleColumns: selectedVisibleColumnKeys });
+ }
+ }, [
+ persistTableOwnState,
+ selectableColumnKeys,
+ selectedVisibleColumnKeys,
+ serverPaginationData?.visibleColumns,
+ storedVisibleColumnKeys.length,
+ ]);
+
+ const isColumnMenuClick = useRef(false);
+
+ const renderColumnSelectDropdown = (): JSX.Element | null => {
+ if (!selectableColumnsMeta.length) {
+ return null;
+ }
+
+ const getPersistedVisibleColumns = () => {
+ const availableKeys = new Set(selectableColumnKeys);
+ const selectedFromOwnState = sanitizeVisibleColumnSelection(
+ serverPaginationData?.visibleColumns,
+ availableKeys,
+ );
+ if (selectedFromOwnState.length > 0) return selectedFromOwnState;
+ if (storedVisibleColumnKeys.length > 0) return storedVisibleColumnKeys;
+ return selectableColumnKeys;
+ };
+
+ const handleOnClick = ({ key }: { key: string }) => {
+ isColumnMenuClick.current = true;
+ setTimeout(() => {
+ isColumnMenuClick.current = false;
+ }, 200);
+ const targetKey = String(key);
+ const isSelected = localVisibleColumnKeys.includes(targetKey);
+ const nextVisibleColumns = isSelected
+ ? localVisibleColumnKeys.filter(columnKey => columnKey !== targetKey)
+ : [...localVisibleColumnKeys, targetKey];
+
+ // Keep at least one visible column so table structure stays valid.
+ if (!nextVisibleColumns.length) {
+ return;
+ }
+
+ setLocalVisibleColumnKeys(nextVisibleColumns);
+ };
+
+ return (
+ <Dropdown
+ placement="bottomRight"
+ open={showColumnSelectorDropdown}
+ onOpenChange={(flag: boolean, info?: { source: string }) => {
+ if (info && info.source === 'menu') {
+ return;
+ }
+ if (!flag && isColumnMenuClick.current) {
+ isColumnMenuClick.current = false;
+ return;
+ }
+ setShowColumnSelectorDropdown(flag);
+ if (flag) {
+ setLocalVisibleColumnKeys(selectedVisibleColumnKeys);
+ } else {
+ if (!isEqual(localVisibleColumnKeys, selectedVisibleColumnKeys)) {
+ setSelectedVisibleColumnKeys(localVisibleColumnKeys);
+ }
+ const persisted = getPersistedVisibleColumns();
+ if (!isEqual(localVisibleColumnKeys, persisted)) {
+ persistTableOwnState({ visibleColumns: localVisibleColumnKeys });
+ }
+ }
+ }}
+ menu={{
+ selectable: true,
+ multiple: true,
+ onClick: handleOnClick,
+ selectedKeys: localVisibleColumnKeys,
+ items: [
+ {
+ key: 'columns-group',
+ label: (
+ <div
+ css={css`
+ max-width: 260px;
+ padding: 0 ${theme.sizeUnit * 2}px;
+ color: ${theme.colorText};
+ font-size: ${theme.fontSizeSM}px;
+ `}
+ >
+ {t(
+ 'Choose which columns should be visible in this table on
the dashboard.',
+ )}
+ </div>
+ ),
+ type: 'group',
+ children: selectableColumnsMeta.map(column => ({
+ key: String(column.key),
+ label: (
+ <>
+ <span
+ css={css`
+ color: ${theme.colorText};
+ `}
+ >
+ {column.config?.customColumnName ||
+ column.originalLabel ||
+ column.label ||
+ column.key}
+ </span>
+ <span
+ css={css`
+ float: right;
+ font-size: ${theme.fontSizeSM}px;
+ `}
+ >
+ {localVisibleColumnKeys.includes(
+ String(column.key),
+ ) && <CheckOutlined />}
+ </span>
+ </>
+ ),
+ })),
+ },
+ ],
+ }}
+ trigger={['click']}
+ >
+ <span>
+ {t('Columns')} <DownOutlined />
+ </span>
Review Comment:
**🎨 Design Review — MEDIUM**
The new Columns dropdown trigger is rendered as a plain `<span>` with no
role or tabIndex, so it is not keyboard-focusable or operable by default, which
means keyboard-only users cannot reliably open the column selector.
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b41bb1644d0341a183121fe7622bceb0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b41bb1644d0341a183121fe7622bceb0&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 **Design Review** comment — a question about the UX/design of
frontend code. It is intentionally framed as a question, not a prescription.
The author may agree or disagree.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 1033:1035
**Comment:**
*MEDIUM: The new Columns dropdown trigger is rendered as a plain
`<span>` with no role or tabIndex, so it is not keyboard-focusable or operable
by default, which means keyboard-only users cannot reliably open the column
selector.
- If you agree with the proposal, apply a small, localized change (swap a
color token, bump a font size, adjust spacing, add an aria-label, etc.).
- If you disagree, or the answer depends on a design decision a human should
make, explain your reasoning and ask the user how to proceed.
Do NOT refactor surrounding components or apply other design changes that
weren't asked about.
```
</details>
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -778,7 +778,11 @@ const transformProps = (
percentMetrics,
serverPaginationData: serverPagination
? serverPaginationData
- : defaultServerPaginationData,
+ : {
+ ...defaultServerPaginationData,
+ visibleColumns: serverPaginationData?.visibleColumns,
+ clientView: serverPaginationData?.clientView,
+ },
Review Comment:
**Suggestion:** Adding `clientView` into `serverPaginationData` in client
mode introduces large row snapshots into a prop that is serialized in
`DataTable` (`JSON.stringify(serverPaginationData)`), turning every render into
expensive deep serialization work. On large tables this can noticeably degrade
UI responsiveness. Keep `clientView` out of the pagination payload passed to
`DataTable` (or pass a lightweight/memoized field instead). [performance]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Large tables lag from repeated deep JSON serialization.
- ⚠️ Dashboard responsiveness degrades for client-side table views.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load a dashboard with a large client-side table visualization (no
server-side
pagination) so `transformProps()` is used from
`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:22-27`
and returns
props for `TableChart`, with `serverPagination=false` and `ownState` passed
in as
`serverPaginationData` at `transformProps.ts:32-34`.
2. As the user filters or searches within the table, `DataTable` calls
`onFilteredRowsChange` when client-side filtered rows change, via the effect
at
`superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:193-219`,
passing the current `rows.map(r => r.original)` back to `TableChart` as
`setClientViewRows` (wired at `TableChart.tsx:1793-1796`).
3. `TableChart` collects these client-side rows in state `clientViewRows` at
`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:1739-1749`
and, in the
effect at `TableChart.tsx:1755-1773`, calls `persistTableOwnState({
clientView: { rows:
clientViewRows, columns: exportColumns, count: clientViewRows.length } })`,
causing the
chart's `ownState` to now include a potentially large `clientView` snapshot
(rows and
column metadata).
4. On the next render, `transformProps()` receives this enlarged `ownState`
and, in client
mode (`serverPagination=false`), builds `serverPaginationData` for
`DataTable` as `{
...defaultServerPaginationData, visibleColumns:
serverPaginationData?.visibleColumns,
clientView: serverPaginationData?.clientView }` at `transformProps.ts:60-66`
(lines
781-785 in the diff). `DataTable` then computes `const paginationData =
JSON.stringify(serverPaginationData);` at `DataTable.tsx:193`, meaning every
`DataTable`
render serializes the entire `clientView` (all client-side rows and columns)
just to
derive a dependency string for `defaultGetTableSize` at
`DataTable.tsx:195-217`. On large
tables this deep `JSON.stringify` over thousands of rows and nested objects
occurs on
every render, significantly increasing render time and degrading UI
responsiveness, even
though the size calculation only needs lightweight pagination fields, not
the full
`clientView`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3ef7ed454067425393212de31be07b16&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3ef7ed454067425393212de31be07b16&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/plugins/plugin-chart-table/src/transformProps.ts
**Line:** 781:785
**Comment:**
*Performance: Adding `clientView` into `serverPaginationData` in client
mode introduces large row snapshots into a prop that is serialized in
`DataTable` (`JSON.stringify(serverPaginationData)`), turning every render into
expensive deep serialization work. On large tables this can noticeably degrade
UI responsiveness. Keep `clientView` out of the pagination payload passed to
`DataTable` (or pass a lightweight/memoized field 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%2F40995&comment_hash=1c57e5cf7379a491755ba1ba2eb34054407e4f2aa29c9426d9189765c67baba8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40995&comment_hash=1c57e5cf7379a491755ba1ba2eb34054407e4f2aa29c9426d9189765c67baba8&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -172,7 +174,10 @@ export default typedMemo(function DataTable<D extends
object>({
const pageSizeRef = useRef([initialPageSize, resultsSize]);
const hasPagination = initialPageSize > 0 && resultsSize > 0; // pageSize ==
0 means no pagination
const hasGlobalControl =
- hasPagination || !!searchInput || renderTimeComparisonDropdown;
+ hasPagination ||
+ !!searchInput ||
+ renderTimeComparisonDropdown ||
+ renderColumnSelectDropdown;
Review Comment:
**Suggestion:** `hasGlobalControl` now treats `renderColumnSelectDropdown`
as a truthy flag even when the callback returns `null`, so the controls
container renders with no actual controls and still affects table sizing. This
creates a blank toolbar area and can reduce available table height
unexpectedly. Compute this flag from a real availability boolean (for example,
whether selectable columns exist) instead of the callback reference. [css
layout issue]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Blank toolbar renders above table with no controls.
- ⚠️ Reduced table height shows fewer rows to users.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a table chart using `TableChart` at
`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:16-18` so
it instantiates
`<DataTable>` at lines 1768-1784 with `serverPagination={false}` and
`renderColumnSelectDropdown={renderColumnSelectDropdown}`.
2. In `transformProps()` at
`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:473-480`
and
`:792-797`, configure form data so `getPageSize(pageLength, numRecords,
numColumns)`
returns `0` (e.g., `pageLength` unset and `numRecords * numColumns <=
5000`), causing
`pageSize` prop to `DataTable` to be `0` (`DataTable.tsx:114-118`), which
yields
`hasPagination === false` at `DataTable.tsx:171-175`.
3. Configure the chart so client-side search is disabled (`includeSearch`
false), making
`searchInput={includeSearch && SearchInput}` evaluate to `false` in the
`DataTable` call
at `TableChart.tsx:1795-1797`, and ensure time comparison is off so
`renderTimeComparisonDropdown` is passed as `undefined` at
`TableChart.tsx:1799-1806`.
4. Configure the columns such that `selectableColumnsMeta` is empty (e.g.,
`filteredColumnsMeta` has no entries with `config?.visible !== false`),
causing
`renderColumnSelectDropdown` to return `null` early at
`TableChart.tsx:80-83` while still
being passed as a function prop. On render, `DataTable` computes
`hasGlobalControl` as
truthy because `renderColumnSelectDropdown` is a non-null function at
`DataTable.tsx:175-180`, so it renders the controls container at
`DataTable.tsx:559-62`,
but inside that container all children resolve to `null` (no pagination, no
search, no
time comparison, column dropdown returns `null`), resulting in an empty
toolbar area that
still occupies height via `.dt-controls` styling and is subtracted in
`defaultGetTableSize` via `globalControlRef.current?.clientHeight` at
`DataTable.tsx:195-203`, reducing the visible table height with no actual
controls.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=34fa3d3f5f344e3f9029a9a56d689702&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=34fa3d3f5f344e3f9029a9a56d689702&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/plugins/plugin-chart-table/src/DataTable/DataTable.tsx
**Line:** 176:180
**Comment:**
*Css Layout Issue: `hasGlobalControl` now treats
`renderColumnSelectDropdown` as a truthy flag even when the callback returns
`null`, so the controls container renders with no actual controls and still
affects table sizing. This creates a blank toolbar area and can reduce
available table height unexpectedly. Compute this flag from a real availability
boolean (for example, whether selectable columns exist) instead of the callback
reference.
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%2F40995&comment_hash=c4dafaa1533ef738730993430c2418573312490b386c40326f03fbdd69d053f4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40995&comment_hash=c4dafaa1533ef738730993430c2418573312490b386c40326f03fbdd69d053f4&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]
