Copilot commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3888568427
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:
##########
@@ -240,6 +240,14 @@ export const useColDefs = ({
slice_id,
}: UseColDefsProps) => {
const theme = useTheme();
+ // transformProps.ts computes these fresh on every call (no memoization),
+ // so a reference-based dependency here would recreate getCommonColProps -
+ // and therefore colDefs - on every render regardless of whether the
+ // formatting actually changed. Compare by content instead.
+ const stringifiedColumnColorFormatters = JSON.stringify(
+ columnColorFormatters,
+ );
+ const stringifiedBasicColorFormatters = JSON.stringify(basicColorFormatters);
Review Comment:
`JSON.stringify` omits `getColorFromValue`, the only field that captures a
rule's operator, thresholds, gradient, and resolved color. Two rules for the
same source/target therefore produce the same dependency string even when their
formatting behavior changes, leaving `getCommonColProps` closed over the old
formatter until another dependency changes. Stabilize these formatters upstream
or use a dependency that preserves formatter behavior rather than serializing
away the function.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts:
##########
@@ -354,6 +354,13 @@ export function convertFilterModel(
export function convertAgGridStateToOwnState(
agGridState: AgGridChartState,
): Partial<BackendOwnState> {
+ // In client mode, AG Grid handles sort/filter/pagination locally and none
+ // of it needs to reach the backend query, so folding it into ownState only
+ // triggers an unnecessary requery/remount.
+ if (!agGridState.serverPagination) {
+ return {};
+ }
Review Comment:
Treat only an explicit `false` as client mode. Existing saved
`table_state`/permalink chart states predate the new `serverPagination` field,
so they have `undefined`; returning here makes previously persisted server-side
sort/filter state disappear on restore instead of preserving the old conversion
behavior.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -55,29 +55,75 @@ const getCellStyle = (params: CellStyleParams) => {
let backgroundColor;
let color;
if (hasColumnColorFormatters) {
- columnColorFormatters!
- .filter(formatter => {
- const colTitle = formatter?.column?.includes('Main')
- ? formatter?.column?.replace('Main', '').trim()
- : formatter?.column;
- return colTitle === colDef.field;
- })
- .forEach(formatter => {
- const formatterResult =
- value || value === 0 ? formatter.getColorFromValue(value) : false;
- if (formatterResult) {
- if (
- formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
- formatter.toTextColor
- ) {
- color = formatterResult;
- } else if (
- formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
- ) {
- backgroundColor = formatterResult;
- }
+ const applyFormatter = (
+ formatter: ColorFormatters[number],
+ valueToFormat: typeof value,
+ ) => {
+ const formatterResult =
+ valueToFormat || valueToFormat === 0
+ ? formatter.getColorFromValue(valueToFormat)
+ : false;
+ if (formatterResult) {
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
+ formatter.toTextColor
+ ) {
+ color = formatterResult;
+ } else if (
+ formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
+ ) {
+ backgroundColor = formatterResult;
}
- });
+ }
+ };
+
+ // formatter.column can be a legacy display label ("Main colname") for
+ // time-comparison columns rather than the row's actual data key, so
+ // resolve it to the real field id before using it to read row values.
+ const resolveColumnKey = (columnKey: string) =>
+ columnKey.startsWith('Main ')
+ ? columnKey.slice('Main '.length)
+ : columnKey;
+
+ // Formatters with no formatting target color their own source column,
+ // keyed off this cell's own value.
+ columnColorFormatters!
+ .filter(
+ formatter =>
+ !formatter.columnFormatting &&
+ resolveColumnKey(formatter.column) === colDef.field,
+ )
+ .forEach(formatter => applyFormatter(formatter, value));
+
+ // Formatters with a real target column color that target column,
+ // keyed off the value in the formatter's own (source) column.
+ columnColorFormatters!
+ .filter(
+ formatter =>
+ formatter.columnFormatting &&
+ formatter.columnFormatting !== ObjectFormattingEnum.ENTIRE_ROW &&
+ resolveColumnKey(formatter.columnFormatting) === colDef.field,
+ )
+ .forEach(formatter =>
+ applyFormatter(
+ formatter,
+ node?.data?.[resolveColumnKey(formatter.column)],
+ ),
+ );
+
+ // Entire-row formatters apply to every cell in the row, keyed off the
+ // value in the formatter's own column rather than this cell's column.
+ columnColorFormatters!
+ .filter(
+ formatter =>
+ formatter.columnFormatting === ObjectFormattingEnum.ENTIRE_ROW,
+ )
Review Comment:
Legacy V1 entire-row rules use `toAllRow: true`, and the migration carries
`conditional_formatting` over unchanged. This rendering branch only recognizes
the newer `columnFormatting` value, so those migrated rules color only their
source cell unless the chart is later opened and resaved through the control
panel. Honor `formatter.toAllRow` here as well.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -416,6 +465,80 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps>
= memo(
serverPaginationData?.agGridFilterModel,
]);
+ // Captures the "current view" (post-filter/sort, all rows across all
+ // pages) for the "Export Current View" menu, mirroring Table V1's
+ // clientView snapshot. Client-side mode only: in server pagination mode
+ // the grid only ever holds a single page's rows, so a client-derived
+ // snapshot can't represent the full filtered/sorted result and export
+ // falls back to a fresh backend query instead (see
useExploreAdditionalActionsMenu).
+ const lastClientViewSignatureRef = useRef<string | null>(null);
+ // AG Grid fires onModelUpdated once as it applies the initial row data on
+ // mount, before any user interaction. That first event reflects data the
+ // chart already has - not a view change worth persisting - so it's
+ // skipped rather than compared against `lastClientViewSignatureRef`,
+ // which is always null right after mount. Persisting it anyway would
+ // write an ownState change on every mount, which (if the dashboard
+ // decides that warrants a requery) unmounts/remounts this component and
+ // re-fires onModelUpdated, looping forever.
+ const hasCapturedInitialModelRef = useRef(false);
+ // Debounced (like handleGridStateChange below) because the full
+ // filtered+sorted traversal is O(n) and onModelUpdated can fire rapidly
+ // in succession (e.g. while typing into a quick filter); only the
+ // trailing update needs to recompute the snapshot.
+ const handleModelUpdated = useCallback(
+ debounce(() => {
+ if (serverPagination || !onClientViewChange || !gridRef.current?.api) {
+ return;
+ }
+ const { api } = gridRef.current;
+ const displayedColumns = api
+ .getAllDisplayedColumns()
+ .filter(column => column.getColId() !== ROW_NUMBER_COL_ID);
+ const columns = displayedColumns.map(column => ({
+ key: column.getColId(),
+ label: column.getColDef().headerName || column.getColId(),
+ }));
+
+ const rows: Record<string, unknown>[] = [];
+ api.forEachNodeAfterFilterAndSort(node => {
+ if (node.data) {
+ rows.push(node.data);
+ }
+ });
+
+ // Without a getRowId callback, AG Grid's node ids are purely
+ // positional and reset to 0..n-1 on every setRowData call, so they
+ // don't identify a row's content across a data refresh — hashing
+ // the actual filtered+sorted row content (which this function
+ // already has to visit to build `rows`) is what actually detects
+ // both value changes (e.g. a refresh with the same row count) and
+ // order changes (e.g. a pure sort), not just count/column changes.
+ const signature = `${JSON.stringify(rows)}|${columns.map(c =>
c.key).join(',')}`;
+
+ if (!hasCapturedInitialModelRef.current) {
+ hasCapturedInitialModelRef.current = true;
+ lastClientViewSignatureRef.current = signature;
+ return;
+ }
Review Comment:
Do not discard the initial model snapshot. `clientView` is the data source
for client-side “Export Current View”; until another model event changes the
signature, this return leaves it absent and the export menu falls back to a
backend query that cannot reproduce restored client-side sort/filter state.
Both dashboard and Explore paths now explicitly strip `clientView` from
re-query detection, so publishing the initial snapshot does not create the loop
cited here.
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -156,16 +158,20 @@ def _migrate_temporal_filter(self, rv_data: dict[str,
Any]) -> None:
def upgrade_slice(cls, slc: Slice) -> None:
try:
clz = cls(slc.params)
+ # Some charts don't carry a "datasource" key in params — outside
+ # of migrations, callers always read it via Slice.form_data,
+ # which injects "datasource" from the datasource_id/
+ # datasource_type columns on every access. _build_query() (and
+ # anything else touching self.data) needs that same key, so
+ # synthesize it here the same way for the charts missing it.
+ if "datasource" not in clz.data and slc.datasource_id is not None:
+ clz.data["datasource"] =
f"{slc.datasource_id}__{slc.datasource_type}"
form_data_bak = copy.deepcopy(clz.data)
Review Comment:
Capture the backup before synthesizing `datasource`. As written, a chart
whose original `params` omitted this key stores the injected value in
`form_data_bak`, so `downgrade_slice()` restores different params instead of
reverting cleanly to the original chart.
##########
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:
##########
@@ -390,13 +390,22 @@ const ChartContextMenu = (
filters,
});
- // Since Ant Design's Dropdown does not offer an imperative API
- // and we can't attach event triggers to charts SVG elements, we
- // use a hidden span that gets clicked on when receiving click events
- // from the charts.
- document.getElementById(`hidden-span-${id}`)?.click();
+ // Some chart libraries (e.g. AG Grid) can dispatch a single logical
+ // right-click as two contextmenu events in quick succession, calling
+ // `open()` twice. Since Ant Design's Dropdown treats a click on an
+ // already-open trigger as a toggle-to-close, re-clicking the hidden
+ // span here on the second call would immediately close the menu we
+ // just opened. Only click it when the menu isn't already visible; the
+ // position/filters update above still applies on every call.
+ if (!visible) {
+ // Ant Design's Dropdown does not offer an imperative API and we
+ // can't attach event triggers to charts' SVG elements, so we use a
+ // hidden span that gets clicked on when receiving click events from
+ // the charts.
+ document.getElementById(`hidden-span-${id}`)?.click();
Review Comment:
This guard still allows both rapid calls to click the trigger: both
invocations run against the same render closure where `visible` is `false`,
because the first click's state update does not synchronously recreate `open`.
The duplicate event described above can therefore still toggle the dropdown
closed. Track open state in a ref that is set synchronously before clicking
(and kept in sync by `onOpenChange`).
##########
superset-frontend/src/hooks/apiResources/datasets.ts:
##########
@@ -81,9 +81,16 @@ export const useDatasetDrillInfo = (
});
return;
}
+ const numericDatasetId = getDatasetId(datasetId);
+ if (Number.isNaN(numericDatasetId)) {
+ // datasetId isn't resolved yet (e.g. the dashboard's slice entity hasn't
+ // hydrated after a client-side navigation back from Explore). Stay in
+ // Loading rather than firing a request for dataset "NaN"; the effect
+ // reruns once datasetId settles to a real value.
+ return;
+ }
Review Comment:
Reset the resource when the ID is unresolved. If this hook previously
completed for another dataset and then receives a transient malformed ID, this
early return retains the old `Complete` result rather than the documented
loading state, so the context menu can expose drill metadata from the previous
dataset.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -368,75 +453,203 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
[emitCrossFilters, setDataMask, timeGrain, timestampFormatter],
);
+ const drillColumns = isUsingTimeComparison
+ ? (filteredColumns as InputColumn[])
+ : (columns as InputColumn[]);
+
+ const handleContextMenu = useCallback(
+ (event: CellContextMenuEvent) => {
+ if (!onContextMenu || isRawRecords || !event.column || !event.data) {
+ return;
+ }
+ const nativeEvent = event.event as MouseEvent | null | undefined;
+ if (!nativeEvent) return;
+ nativeEvent.preventDefault();
+ nativeEvent.stopPropagation();
+
+ const rowData = event.data as Record<string, DataRecordValue>;
+ const key = event.column.getColId();
+ const cellValue = event.value as DataRecordValue;
+ const colDef = event.column.getColDef();
+ const isMetric = Boolean(
+ colDef.context?.isMetric || colDef.context?.isPercentMetric,
+ );
+
+ const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
+ drillColumns.forEach(col => {
+ if (col.isMetric || col.isPercentMetric) return;
+ const dataRecordValue = rowData[col.key];
+
+ if (
+ dataRecordValue == null ||
+ (dataRecordValue instanceof DateWithFormatter &&
+ dataRecordValue.input == null)
+ ) {
Review Comment:
Use `isEmptyDateInput` here as the formatting/filtering paths do. A blank
temporal value is wrapped as `DateWithFormatter(input: '')`; this condition
misses it, then constructs an invalid date and emits an equality filter whose
value serializes as null instead of an `IS NULL` filter.
This issue also appears on line 536 of the same file.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/index.ts:
##########
@@ -46,6 +46,7 @@ const metadata = new ChartMetadata({
Behavior.InteractiveChart,
Behavior.DrillToDetail,
Behavior.DrillBy,
+ 'EXPORT_CURRENT_VIEW' as Behavior,
Review Comment:
Before enabling this behavior, handle an empty filtered view. The grid
correctly publishes `clientView.rows = []`, but the shared export menu checks
`rows.length`; it then falls back to an unfiltered backend export, so “Export
Current View” can export the full dataset when the current view has zero rows.
The consumer must distinguish an existing empty snapshot from a missing
snapshot and support an empty/header-only export.
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -654,3 +656,258 @@ def process(base_query_object: dict[str, Any]) ->
list[dict[str, Any]]:
return [result]
return build_query_context(self.data, process)
+
+
+def _get_table_chart_time_offsets(form_data: dict[str, Any]) -> list[Any]:
+ """
+ Resolve time_compare into the list of shifts buildQuery.ts sends as
+ time_offsets. table charts use a single-select time_compare control
+ whose choices include the special 'custom'/'inherit' shifts, which
+ resolve to start_date_offset/'inherit' rather than being used verbatim.
+ """
+ time_compare_shifts = ensure_is_array(form_data.get("time_compare"))
+ non_custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift not in ("custom",
"inherit")
+ ]
+ custom_or_inherit_shifts = [
Review Comment:
The chart-level shifts need the same `isTimeComparison(formData,
baseQueryObject)` gate used by both Table buildQuery implementations. This
helper currently emits `time_offsets` whenever stale `time_compare` data
exists, even if `comparison_type` is absent/invalid or the query is raw mode,
so migrated persisted query contexts can request shifted data that the chart
itself would not request.
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -654,3 +656,258 @@ def process(base_query_object: dict[str, Any]) ->
list[dict[str, Any]]:
return [result]
return build_query_context(self.data, process)
+
+
+def _get_table_chart_time_offsets(form_data: dict[str, Any]) -> list[Any]:
+ """
+ Resolve time_compare into the list of shifts buildQuery.ts sends as
+ time_offsets. table charts use a single-select time_compare control
+ whose choices include the special 'custom'/'inherit' shifts, which
+ resolve to start_date_offset/'inherit' rather than being used verbatim.
+ """
+ time_compare_shifts = ensure_is_array(form_data.get("time_compare"))
+ non_custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift not in ("custom",
"inherit")
+ ]
+ custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift in ("custom",
"inherit")
+ ]
+
+ time_offsets: list[Any] = list(non_custom_or_inherit_shifts)
+ if "custom" in custom_or_inherit_shifts:
+ time_offsets.append(form_data.get("start_date_offset"))
+ if "inherit" in custom_or_inherit_shifts:
+ time_offsets.append("inherit")
+
+ # Dashboard filter override - allows dashboard-level time shifts to
+ # OVERRIDE chart-level time shift settings, mirroring buildQuery.ts.
+ extra_form_data_time_compare = (form_data.get("extra_form_data") or
{}).get(
+ "time_compare"
+ )
+ if extra_form_data_time_compare:
+ # extra_form_data.time_compare is typed as a single string on the
+ # frontend, but self.data comes from deserialized JSON with no
+ # runtime type guarantee — normalize defensively so an already-list
+ # value doesn't get double-nested into [[...]].
+ time_offsets = list(ensure_is_array(extra_form_data_time_compare))
+ return time_offsets
+
+
+def _reorder_table_chart_temporal_column(
+ columns: list[Any],
+ time_grain_sqla: Any,
+ temporal_columns_lookup: dict[str, Any],
+) -> list[Any]:
+ """
+ Move the first physical column with a temporal_columns_lookup entry to
+ the front of the columns list as a BASE_AXIS adhoc column, mirroring
+ buildQuery.ts's temporal-column handling in aggregate mode.
+ """
+ temporal_column = None
+ filtered_columns = []
+ for col in columns:
+ should_be_temporal = (
+ is_physical_column(col)
+ and time_grain_sqla
+ and temporal_columns_lookup.get(col)
+ )
+ if should_be_temporal and temporal_column is None:
+ temporal_column = {
+ "timeGrain": time_grain_sqla,
+ "columnType": "BASE_AXIS",
+ "sqlExpression": col,
+ "label": col,
+ "expressionType": "SQL",
+ }
+ else:
+ filtered_columns.append(col)
+ return [temporal_column] + filtered_columns if temporal_column else
filtered_columns
+
+
+class MigrateTableChart(MigrateViz):
+ source_viz_type = "table"
+ target_viz_type = "ag-grid-table"
+ remove_keys = {"allow_rearrange_columns", "allow_render_html"}
Review Comment:
Dropping both keys changes active V1 settings during migration. V1 defaults
`allow_rearrange_columns` to false and permits `allow_render_html` to be
disabled, while V2 currently hardcodes both behaviors on
(`transformProps.ts:593`, `useColDefs.ts:395`). This means many migrated charts
become rearrangeable and charts that intentionally displayed HTML as text start
rendering it. Either preserve/implement these controls in V2 or explicitly
reject such charts rather than silently changing them.
--
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]
