michael-s-molina commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3961275790
##########
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:
Fixed by the same change as the `useColDefs.ts` comment above — the
basic-formatter dependency now stringifies `[conditionalFormatting,
comparisonColorEnabled, comparisonColorScheme]` instead of the computed
formatter objects.
##########
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:
Fixed: added a `visibleRef` set synchronously right before the click (and
kept in sync by `onOpenChange`), and the guard now reads that instead of the
`visible` state, which was still stale across the two calls.
##########
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:
Fixed: `form_data_bak` is now captured immediately after `cls(slc.params)`,
before the `datasource` key is synthesized.
##########
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:
Fixed: the hook now resets to `Loading` (result: null) when `datasetId`
regresses to unresolved, instead of leaving the previous dataset's
`Complete`/`Error` result in place.
##########
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:
Fixed: removed the initial-snapshot skip. `clientView` is excluded from
ownState re-query comparisons on both the Explore and dashboard paths (as your
comment notes), so publishing the first snapshot on mount can't trigger the
requery/remount loop this was guarding against — it was just leaving "Export
Current View" without a snapshot until some later grid event.
--
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]
