sadpandajoe commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3768729746
##########
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:
`getColorFromValue` carries the rule's color/threshold in a closure, and
JSON serialization drops it. Editing a conditional-formatting rule without
changing its column or scope leaves this dependency string unchanged, so the
existing colDefs continue to call the old formatter until another change
rebuilds them. Can this depend on a serializable representation of the
underlying rules instead?
##########
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:
This drops client-mode sort/filter/column state before the dashboard's
export path builds its download query. Dashboard charts do not consume the new
Explore-only `clientView` snapshot, so exporting a client-paginated table after
sorting or filtering now sends the default query rather than the displayed
view. Could the requery suppression be kept separate from state needed for a
download?
##########
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)
Review Comment:
This persists chart-level `time_offsets` whenever stale `time_compare` is
present, even though both Table buildQuery implementations require
`isTimeComparison(...)` before adding those shifts. A raw-mode chart that
retains `time_compare` after switching modes therefore gets offset queries in
its migrated `query_context` that neither runtime chart would send. Should this
helper receive the base query and apply the same gate, while leaving the
`extra_form_data` override ungated?
##########
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:
The initial `onStateUpdated` and the first user action share this trailing
debounce. If a user sorts, filters, or moves a column before the initial 500ms
callback fires, the coalesced callback is still treated as initialization and
returns here, so that first change is never persisted. Could initialization be
distinguished before debouncing, or otherwise avoid dropping a changed state?
##########
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:
A client-side filter that removes every row produces a valid snapshot with
`rows: []`, but the export menu only takes the client path when `rows.length`
is truthy. It then falls back to a backend query that does not know the AG Grid
filter and exports unfiltered rows. Can the client export path handle an empty
snapshot (including a headers-only file) instead?
##########
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:
For comparison columns, `useColDefs` strips `Main ` from the AG Grid column
ID, but the row data retains the value under `Main <metric>`. The client export
reads raw rows by this captured ID, so Export Current View emits a blank main
metric column after a sort or filter. Should the snapshot retain the data key
separately from the grid column ID?
--
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]
