michael-s-molina commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3961274285
##########
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(
Review Comment:
Fixed: the memo now depends on `JSON.stringify(conditionalFormatting)` (the
raw, fully-serializable config) instead of the computed
`columnColorFormatters`, whose `getColorFromValue` closure was the thing being
silently dropped.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -353,6 +378,21 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps>
= memo(
filterModel,
);
+ // AG Grid fires onStateUpdated once as it applies the initial
+ // column/sort/filter state on mount, before any user
+ // interaction. That first event just reflects the state the grid
+ // was initialized with (chartState/gridInitialState) - not a
+ // user-driven change - so it's skipped rather than compared
+ // against `lastCapturedStateRef`, which is always null right
+ // after mount. Persisting it anyway would write a chart-state
+ // change on every mount, which can cascade into a
+ // remount/onStateUpdated loop.
+ if (!hasCapturedInitialGridStateRef.current) {
Review Comment:
Fixed: split the handler so the "is this the initial mount state" check runs
synchronously on every raw `onStateUpdated` call, before debouncing. A real
first user action landing in the same debounce window is no longer coalesced
into that initial-state branch and dropped.
##########
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(),
Review Comment:
Fixed: `useColDefs` now stashes the unstripped field on
`colDef.context.dataKey`, and the client-view snapshot builder reads row values
by that key instead of by the display-stripped `colId`.
##########
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;
+ }
+
+ if (signature === lastClientViewSignatureRef.current) {
+ return;
+ }
+ lastClientViewSignatureRef.current = signature;
+ onClientViewChange({ rows, columns, count: rows.length });
Review Comment:
Fixed: the export menu and the CSV/JSON/XLSX builders now key off
`columns.length` (i.e. "does a snapshot exist") instead of `rows.length`, so a
filter matching zero rows still takes the client path and produces a
header-only file.
##########
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) {
Review Comment:
Fixed: added a `forExport` option threaded through
`ChartStateConverter`/`convertAgGridStateToOwnState`. The live query still
suppresses client-mode state (avoids the requery loop), but a dashboard chart's
download now passes `forExport: true` and gets the sort/filter/columns
converted.
##########
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:
Fixed — folded into the same fix as your
`allow_rearrange_columns`/`allow_render_html` comment below: both are now
implemented in v2 (control panel checkbox + wired into
`transformProps`/`useColDefs`) instead of being dropped by the migration.
##########
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"}
+ rename_keys: dict[str, str] = {} # no renames needed; names match 1:1
+
+ def _pre_action(self) -> None:
+ # page_length: 0 ("All") has no dropdown choice in v2, but the control
+ # is freeForm and 0 still works at runtime — map to v2's largest
+ # PAGE_SIZE_OPTIONS entry (200) so the migrated chart keeps showing as
+ # many rows per page as v2 supports, rather than an arbitrary smaller
+ # value
+ if self.data.get("page_length") in (0, "0"):
+ self.data["page_length"] = 200
Review Comment:
Fixed: `page_length: 0` is now preserved as-is. Confirmed v2 does still run
unpaginated at runtime when pageSize is 0 (`getPageSize`'s own small-table
fallback relies on exactly that) — v2's control panel just doesn't expose 0 as
a dropdown choice.
##########
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:
Fixed: the guard now checks `agGridState.serverPagination === false`
explicitly (instead of falsy), so a legacy persisted state with
`serverPagination: undefined` still gets converted rather than silently losing
its server-side sort/filter.
--
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]
