This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch viz-pipeline-followups in repository https://gitbox.apache.org/repos/asf/superset.git
commit 1b853d63436a767dd4d19e310ff395913b3e2616 Author: Claude Code <[email protected]> AuthorDate: Mon Jul 27 23:59:05 2026 -0700 fix(deck-multi): guard against stale layer responses, remove dead legacy branch Two issues found in a post-merge self-review of the deck_multi rearchitecture: - loadLayers() had no guard against a layer fetch that resolves after deck_slices (or the visibility filter) has already changed again. A stale response could still write into subSlicesLayers/layerErrors and pollute the autozoom feature accumulator for the new, already-reset generation. Fixed with a generation counter: each loadLayers call bumps it, each in-flight fetch captures the generation it started under, and its callbacks bail out if that generation is no longer current. - deck_multi's buildQuery always returns an empty queries array (it issues no query of its own -- every layer self-fetches), so `payload` is always undefined in production. The `payload?.data?.slices`/`payload?.data?.features` reads in Multi.tsx were therefore dead code, reachable only because the Jest fixtures synthesized a payload shape the real transformProps can never produce. Removed the dead branches, made `payload` optional in DeckMultiProps to match reality, and converted the fixtures in Multi.test.tsx/Multi.color.test.tsx to mock SupersetClient.get/post and drive the real v1 fetch path instead. Added two new tests: one confirming a stale, superseded layer response is ignored rather than corrupting the current render, and one confirming autozoom correctly accumulates points across layers that resolve out of order (the scenario the original v1-autozoom fix was written for, but never had a multi-layer test). Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/Multi/Multi.color.test.tsx | 111 ++-- .../preset-chart-deckgl/src/Multi/Multi.test.tsx | 632 +++++++++------------ .../preset-chart-deckgl/src/Multi/Multi.tsx | 69 ++- 3 files changed, 355 insertions(+), 457 deletions(-) diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.color.test.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.color.test.tsx index c7404b5d0d3..3e9faa760df 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.color.test.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.color.test.tsx @@ -48,6 +48,7 @@ jest.mock('../DeckGLContainer', () => ({ jest.mock('@superset-ui/core', () => ({ ...jest.requireActual('@superset-ui/core'), SupersetClient: { + get: jest.fn(), post: jest.fn(), }, })); @@ -95,6 +96,27 @@ const renderWithProviders = (component: React.ReactElement) => const SCATTER_SLICE_ID = 1; +// Mocks the GET /api/v1/chart/{id} the v1 path uses to resolve each +// sub-slice's saved form_data, keyed by slice_id -> params. +const mockSubsliceParams = (paramsBySliceId: Record<number, JsonObject>) => { + (SupersetClient.get as jest.Mock).mockImplementation( + ({ endpoint }: { endpoint: string }) => { + const sliceId = Number(endpoint.match(/\/chart\/(\d+)/)?.[1]); + const params = paramsBySliceId[sliceId]; + return Promise.resolve({ + json: { + result: { + viz_type: params?.viz_type, + datasource_id: 1, + datasource_type: 'table', + params: JSON.stringify(params || {}), + }, + }, + }); + }, + ); +}; + const props = { formData: { datasource: '1__table', @@ -103,28 +125,7 @@ const props = { autozoom: false, map_style: 'mapbox://styles/mapbox/light-v9', }, - payload: { - data: { - slices: [ - { - slice_id: SCATTER_SLICE_ID, - form_data: { - viz_type: 'deck_scatter', - datasource: '1__table', - slice_id: SCATTER_SLICE_ID, - // categorical color configuration coming from the saved scatter chart - color_scheme_type: 'categorical_palette', - color_scheme: 'supersetColors', - dimension: 'category', - }, - }, - ], - features: { - deck_scatter: [], - }, - mapboxApiKey: 'test-key', - }, - }, + payload: undefined, setControlValue: jest.fn(), viewport: { longitude: 0, latitude: 0, zoom: 1 }, onAddFilter: jest.fn(), @@ -146,6 +147,16 @@ const props = { beforeEach(() => { jest.clearAllMocks(); mockLayerCapture.layers = []; + // categorical color configuration coming from the saved scatter chart + mockSubsliceParams({ + [SCATTER_SLICE_ID]: { + viz_type: 'deck_scatter', + datasource: '1__table', + color_scheme_type: 'categorical_palette', + color_scheme: 'supersetColors', + dimension: 'category', + }, + }); // The scatter sublayer query returns features tagged with a category column. (SupersetClient.post as jest.Mock).mockResolvedValue({ json: { @@ -196,29 +207,16 @@ test('applies categorical colors to scatter subslices saved before the color_sch // Charts saved before the color_scheme_type control existed lack the key in // stored params; the scatter default (categorical_palette) must be resolved // so they keep per-category colors. - const legacyProps = { - ...props, - payload: { - ...props.payload, - data: { - ...props.payload.data, - slices: [ - { - slice_id: SCATTER_SLICE_ID, - form_data: { - viz_type: 'deck_scatter', - datasource: '1__table', - slice_id: SCATTER_SLICE_ID, - color_scheme: 'supersetColors', - dimension: 'category', - }, - }, - ], - }, + mockSubsliceParams({ + [SCATTER_SLICE_ID]: { + viz_type: 'deck_scatter', + datasource: '1__table', + color_scheme: 'supersetColors', + dimension: 'category', }, - }; + }); - renderWithProviders(<DeckMulti {...legacyProps} />); + renderWithProviders(<DeckMulti {...props} />); await expectDistinctCategoricalColors(); }); @@ -228,28 +226,17 @@ test('keeps fixed source and target colors for arc subslices saved before the co // target pickers directly; resolving the default must not stamp a single // per-feature color over the target color. const ARC_SLICE_ID = 2; + mockSubsliceParams({ + [ARC_SLICE_ID]: { + viz_type: 'deck_arc', + datasource: '1__table', + color_picker: { r: 10, g: 20, b: 30, a: 1 }, + target_color_picker: { r: 40, g: 50, b: 60, a: 1 }, + }, + }); const arcProps = { ...props, formData: { ...props.formData, deck_slices: [ARC_SLICE_ID] }, - payload: { - ...props.payload, - data: { - ...props.payload.data, - slices: [ - { - slice_id: ARC_SLICE_ID, - form_data: { - viz_type: 'deck_arc', - datasource: '1__table', - slice_id: ARC_SLICE_ID, - color_picker: { r: 10, g: 20, b: 30, a: 1 }, - target_color_picker: { r: 40, g: 50, b: 60, a: 1 }, - }, - }, - ], - features: { deck_arc: [] }, - }, - }, }; (SupersetClient.post as jest.Mock).mockResolvedValue({ json: { diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx index a916f782aac..ae7d96ba519 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx @@ -47,7 +47,10 @@ jest.mock('@superset-ui/core', () => ({ }, })); -// register stub buildQuery/transformProps for the layer types the tests use +// register stub buildQuery/transformProps for the layer types the tests use. +// transformProps passes the mocked POST response's features straight through, +// so each test controls layer content via the POST mock, exactly as the real +// per-layer buildQuery/transformProps registry would. const { getChartBuildQueryRegistry, getChartTransformPropsRegistry } = jest.requireActual('@superset-ui/core'); ['deck_scatter', 'deck_polygon'].forEach(vizType => { @@ -59,11 +62,18 @@ const { getChartBuildQueryRegistry, getChartTransformPropsRegistry } = form_data: formData, }), ); - getChartTransformPropsRegistry().registerValue(vizType, () => ({ - payload: { - data: { features: [], mapboxApiKey: 'test-key', metricLabels: [] }, - }, - })); + getChartTransformPropsRegistry().registerValue( + vizType, + (chartProps: any) => ({ + payload: { + data: { + features: chartProps.queriesData?.[0]?.data ?? [], + mapboxApiKey: 'test-key', + metricLabels: [], + }, + }, + }), + ); }); const mockStore = configureStore({ @@ -72,6 +82,51 @@ const mockStore = configureStore({ }, }); +// The two sub-slices every test in this file fetches: slice 1 is a scatter +// layer, slice 2 is a polygon layer -- mirroring the shape the old +// legacy-payload fixture hardcoded, but now resolved through the real +// GET /api/v1/chart/{id} -> POST /api/v1/chart/data v1 path. +const SUBSLICES: Record<number, { vizType: string }> = { + 1: { vizType: 'deck_scatter' }, + 2: { vizType: 'deck_polygon' }, +}; + +// Keyed by viz_type so a test can control exactly what features each layer's +// POST resolves with; defaults to empty so unconfigured layers are inert. +let featuresByVizType: Record<string, unknown[]> = {}; + +const mockFetchesFor = (subslices: Record<number, { vizType: string }>) => { + (SupersetClient.get as jest.Mock).mockImplementation( + ({ endpoint }: { endpoint: string }) => { + const sliceId = Number(endpoint.match(/\/chart\/(\d+)/)?.[1]); + const subslice = subslices[sliceId]; + return Promise.resolve({ + json: { + result: { + viz_type: subslice?.vizType, + datasource_id: 1, + datasource_type: 'table', + params: JSON.stringify({ + viz_type: subslice?.vizType, + datasource: 'test_datasource', + }), + }, + }, + }); + }, + ); + (SupersetClient.post as jest.Mock).mockImplementation( + ({ jsonPayload }: { jsonPayload: { form_data: { viz_type: string } } }) => + Promise.resolve({ + json: { + result: [ + { data: featuresByVizType[jsonPayload.form_data.viz_type] || [] }, + ], + }, + }), + ); +}; + const baseMockProps = { formData: { datasource: 'test_datasource', @@ -80,46 +135,7 @@ const baseMockProps = { autozoom: false, map_style: 'mapbox://styles/mapbox/light-v9', }, - payload: { - data: { - slices: [ - { - slice_id: 1, - form_data: { - viz_type: 'deck_scatter', - datasource: 'test_datasource', - }, - }, - { - slice_id: 2, - form_data: { - viz_type: 'deck_polygon', - datasource: 'test_datasource', - }, - }, - ], - features: { - deck_scatter: [{ position: [0, 0] }], - deck_polygon: [ - { - polygon: [ - [1, 1], - [2, 2], - ], - }, - ], - deck_path: [], - deck_grid: [], - deck_contour: [], - deck_heatmap: [], - deck_hex: [], - deck_arc: [], - deck_geojson: [], - deck_screengrid: [], - }, - mapboxApiKey: 'test-key', - }, - }, + payload: undefined, setControlValue: jest.fn(), viewport: { longitude: 0, latitude: 0, zoom: 1 }, onAddFilter: jest.fn(), @@ -148,346 +164,291 @@ const renderWithProviders = (component: React.ReactElement) => describe('DeckMulti Autozoom Functionality', () => { beforeEach(() => { jest.clearAllMocks(); - (SupersetClient.post as jest.Mock).mockResolvedValue({ - json: { - result: [{ data: [] }], - }, - }); - (SupersetClient.get as jest.Mock).mockResolvedValue({ - json: { - data: { - features: [], - }, - }, - }); + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); }); - test('should NOT apply autozoom when autozoom is false', () => { + test('should NOT apply autozoom when autozoom is false', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); + featuresByVizType = { deck_scatter: [{ position: [1, 1] }] }; const props = { ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: false, - }, + formData: { ...baseMockProps.formData, autozoom: false }, }; renderWithProviders(<DeckMulti {...props} />); - // fitViewport should not be called when autozoom is false + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); expect(fitViewportSpy).not.toHaveBeenCalled(); fitViewportSpy.mockRestore(); }); - test('should apply autozoom when autozoom is true', () => { + test('should apply autozoom to points fetched from each layer when autozoom is true', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); fitViewportSpy.mockReturnValue({ longitude: -122.4, latitude: 37.8, zoom: 10, }); + featuresByVizType = { + deck_scatter: [{ position: [1, 1] }, { position: [2, 2] }], + deck_polygon: [ + { + polygon: [ + [3, 3], + [4, 4], + ], + }, + ], + }; const props = { ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, + formData: { ...baseMockProps.formData, autozoom: true }, }; renderWithProviders(<DeckMulti {...props} />); - // fitViewport should be called with the points from all layers - expect(fitViewportSpy).toHaveBeenCalledWith( - expect.objectContaining({ - longitude: 0, - latitude: 0, - zoom: 1, - }), - expect.objectContaining({ - width: 800, - height: 600, - points: expect.any(Array), - }), + await waitFor(() => { + expect(fitViewportSpy).toHaveBeenCalledWith( + expect.objectContaining({ longitude: 0, latitude: 0, zoom: 1 }), + expect.objectContaining({ + width: 800, + height: 600, + points: expect.any(Array), + }), + ); + }); + // Points from both layers should have been collected by the time the + // second (or later) refit happens -- this exercises the async, per-layer + // accumulation added to fix the "autozoom dead in the v1 path" bug. + const callWithBothLayers = fitViewportSpy.mock.calls.find( + call => call[1].points.length >= 4, ); + expect(callWithBothLayers).toBeDefined(); fitViewportSpy.mockRestore(); }); - test('should use adjusted viewport when autozoom is enabled', async () => { + test('should refit as each layer arrives, even when they resolve out of order', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const adjustedViewport = { - longitude: -122.4, - latitude: 37.8, - zoom: 12, - }; - fitViewportSpy.mockReturnValue(adjustedViewport); - - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, + fitViewportSpy.mockReturnValue({ longitude: 0, latitude: 0, zoom: 8 }); + + // The polygon layer (slice 2) resolves before the scatter layer (slice 1), + // the opposite of deck_slices order -- accumulation must not assume + // layers arrive in request order. + let resolveScatter: (value: unknown) => void = () => {}; + (SupersetClient.post as jest.Mock).mockImplementation( + ({ + jsonPayload, + }: { + jsonPayload: { form_data: { viz_type: string } }; + }) => { + if (jsonPayload.form_data.viz_type === 'deck_scatter') { + return new Promise(resolve => { + resolveScatter = resolve; + }); + } + return Promise.resolve({ + json: { + result: [ + { + data: [ + { + polygon: [ + [3, 3], + [4, 4], + ], + }, + ], + }, + ], + }, + }); }, - }; + ); - renderWithProviders(<DeckMulti {...props} />); + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: true }} + />, + ); + // Only the polygon layer's points have arrived so far. await waitFor(() => { - const container = screen.getByTestId('deckgl-container'); - const viewportData = JSON.parse( - container.getAttribute('data-viewport') || '{}', - ); + const call = fitViewportSpy.mock.calls.at(-1); + expect(call?.[1].points.length).toBe(2); + }); + + resolveScatter({ + json: { + result: [{ data: [{ position: [1, 1] }, { position: [2, 2] }] }], + }, + }); - expect(viewportData.longitude).toBe(adjustedViewport.longitude); - expect(viewportData.latitude).toBe(adjustedViewport.latitude); - expect(viewportData.zoom).toBe(adjustedViewport.zoom); + // Once the scatter layer resolves, its points are added to the same + // accumulator rather than replacing the polygon layer's. + await waitFor(() => { + const call = fitViewportSpy.mock.calls.at(-1); + expect(call?.[1].points.length).toBe(4); }); fitViewportSpy.mockRestore(); }); - test('should set zoom to 0 when calculated zoom is negative', async () => { + test('should set zoom to 0 when the fitted zoom is negative', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - fitViewportSpy.mockReturnValue({ - longitude: 0, - latitude: 0, - zoom: -5, // negative zoom - }); - - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, - }; - - renderWithProviders(<DeckMulti {...props} />); + fitViewportSpy.mockReturnValue({ longitude: 0, latitude: 0, zoom: -5 }); + featuresByVizType = { deck_scatter: [{ position: [1, 1] }] }; + + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: true }} + />, + ); await waitFor(() => { const container = screen.getByTestId('deckgl-container'); const viewportData = JSON.parse( container.getAttribute('data-viewport') || '{}', ); - - // Zoom should be 0, not negative expect(viewportData.zoom).toBe(0); }); fitViewportSpy.mockRestore(); }); - test('should handle empty features gracefully when autozoom is enabled', () => { + test('should not refit when a layer resolves with no features', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, - payload: { - ...baseMockProps.payload, - data: { - ...baseMockProps.payload.data, - features: { - deck_scatter: [], - deck_polygon: [], - deck_path: [], - deck_grid: [], - deck_contour: [], - deck_heatmap: [], - deck_hex: [], - deck_arc: [], - deck_geojson: [], - deck_screengrid: [], - }, - }, - }, - }; - - renderWithProviders(<DeckMulti {...props} />); + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: true }} + />, + ); - // fitViewport should not be called when there are no points + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalledTimes(2)); expect(fitViewportSpy).not.toHaveBeenCalled(); fitViewportSpy.mockRestore(); }); - test('should collect points from all layer types when autozoom is enabled', () => { + test('should use the original viewport when autozoom is disabled', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - fitViewportSpy.mockReturnValue({ - longitude: 0, - latitude: 0, - zoom: 10, - }); - - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, - payload: { - ...baseMockProps.payload, - data: { - ...baseMockProps.payload.data, - features: { - deck_scatter: [{ position: [1, 1] }, { position: [2, 2] }], - deck_polygon: [ - { - polygon: [ - [3, 3], - [4, 4], - ], - }, - ], - deck_arc: [{ sourcePosition: [5, 5], targetPosition: [6, 6] }], - deck_path: [], - deck_grid: [], - deck_contour: [], - deck_heatmap: [], - deck_hex: [], - deck_geojson: [], - deck_screengrid: [], - }, - }, - }, - }; - - renderWithProviders(<DeckMulti {...props} />); - - expect(fitViewportSpy).toHaveBeenCalled(); - const callArgs = fitViewportSpy.mock.calls[0]; - const { points } = callArgs[1]; - - // Should have points from scatter (2), polygon (2), and arc (2) = 6 points total - expect(points.length).toBeGreaterThan(0); - - fitViewportSpy.mockRestore(); - }); - - test('should use original viewport when autozoom is disabled', async () => { - const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const originalViewport = { longitude: -100, latitude: 40, zoom: 5 }; - const props = { - ...baseMockProps, - viewport: originalViewport, - formData: { - ...baseMockProps.formData, - autozoom: false, - }, - }; - renderWithProviders(<DeckMulti {...props} />); + renderWithProviders( + <DeckMulti + {...baseMockProps} + viewport={originalViewport} + formData={{ ...baseMockProps.formData, autozoom: false }} + />, + ); await waitFor(() => { const container = screen.getByTestId('deckgl-container'); const viewportData = JSON.parse( container.getAttribute('data-viewport') || '{}', ); - - // Should use original viewport without modification - expect(viewportData.longitude).toBe(originalViewport.longitude); - expect(viewportData.latitude).toBe(originalViewport.latitude); - expect(viewportData.zoom).toBe(originalViewport.zoom); + expect(viewportData).toMatchObject(originalViewport); }); - - // fitViewport should not have been called expect(fitViewportSpy).not.toHaveBeenCalled(); fitViewportSpy.mockRestore(); }); - test('should apply autozoom when autozoom is undefined (backward compatibility)', () => { + test('should apply autozoom when autozoom is undefined (backward compatibility)', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); fitViewportSpy.mockReturnValue({ longitude: -122.4, latitude: 37.8, zoom: 10, }); + featuresByVizType = { deck_scatter: [{ position: [1, 1] }] }; - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: undefined, // Simulating existing charts created before this feature - }, - }; - - renderWithProviders(<DeckMulti {...props} />); - - // fitViewport should be called for backward compatibility with existing charts - expect(fitViewportSpy).toHaveBeenCalledWith( - expect.objectContaining({ - longitude: 0, - latitude: 0, - zoom: 1, - }), - expect.objectContaining({ - width: 800, - height: 600, - points: expect.any(Array), - }), + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: undefined }} + />, ); + await waitFor(() => expect(fitViewportSpy).toHaveBeenCalled()); + fitViewportSpy.mockRestore(); }); +}); - test('should use adjusted viewport when autozoom is undefined', async () => { - const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const adjustedViewport = { - longitude: -122.4, - latitude: 37.8, - zoom: 12, - }; - fitViewportSpy.mockReturnValue(adjustedViewport); +describe('DeckMulti stale-response guard', () => { + beforeEach(() => { + jest.clearAllMocks(); + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); + }); - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: undefined, // Simulating existing charts - }, - }; + test('ignores a layer response that resolves after deck_slices has already changed again', async () => { + let resolveFirstLoad: (value: unknown) => void = () => {}; + (SupersetClient.post as jest.Mock) + .mockImplementationOnce( + () => + new Promise(resolve => { + resolveFirstLoad = resolve; + }), + ) + .mockImplementationOnce( + () => + new Promise(resolve => { + resolveFirstLoad = resolve; + }), + ) + .mockImplementation(() => + Promise.resolve({ json: { result: [{ data: [] }] } }), + ); - renderWithProviders(<DeckMulti {...props} />); + const { rerender } = renderWithProviders(<DeckMulti {...baseMockProps} />); - await waitFor(() => { - const container = screen.getByTestId('deckgl-container'); - const viewportData = JSON.parse( - container.getAttribute('data-viewport') || '{}', - ); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalledTimes(2)); - // Should use adjusted viewport for backward compatibility - expect(viewportData.longitude).toBe(adjustedViewport.longitude); - expect(viewportData.latitude).toBe(adjustedViewport.latitude); - expect(viewportData.zoom).toBe(adjustedViewport.zoom); - }); + // deck_slices changes to a single, different layer before the first + // load's requests resolve -- this starts a new generation. + rerender( + <Provider store={mockStore}> + <ThemeProvider theme={supersetTheme}> + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, deck_slices: [2] }} + /> + </ThemeProvider> + </Provider>, + ); - fitViewportSpy.mockRestore(); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalledTimes(3)); + + // The abandoned first-generation slice-1 response now resolves. If it + // were not ignored, the container would show 2 layers (the stale slice 1 + // plus the new slice 2) instead of just the current generation's 1. + resolveFirstLoad({ json: { result: [{ data: [] }] } }); + + await waitFor(() => { + expect( + screen + .getByTestId('deckgl-container') + .getAttribute('data-layers-count'), + ).toBe('1'); + }); }); }); describe('DeckMulti Component Rendering', () => { beforeEach(() => { jest.clearAllMocks(); - (SupersetClient.post as jest.Mock).mockResolvedValue({ - json: { - result: [{ data: [] }], - }, - }); - (SupersetClient.get as jest.Mock).mockResolvedValue({ - json: { - data: { - features: [], - }, - }, - }); + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); }); test('should render DeckGLContainer', async () => { @@ -498,7 +459,7 @@ describe('DeckMulti Component Rendering', () => { }); }); - test('should pass correct props to DeckGLContainer', async () => { + test('should pass the base viewport through to DeckGLContainer', async () => { renderWithProviders(<DeckMulti {...baseMockProps} />); await waitFor(() => { @@ -526,12 +487,8 @@ describe('DeckMulti Component Rendering', () => { renderWithProviders(<DeckMulti {...props} />); - // Wait for child slice requests - await waitFor(() => { - expect(SupersetClient.post).toHaveBeenCalled(); - }); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); - // Check that all requests include the dashboardId const { calls } = (SupersetClient.post as jest.Mock).mock; calls.forEach(call => { const formData = call[0].jsonPayload?.form_data || {}; @@ -540,22 +497,10 @@ describe('DeckMulti Component Rendering', () => { }); test('should not include dashboardId when not present', async () => { - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - // No dashboardId - }, - }; - - renderWithProviders(<DeckMulti {...props} />); + renderWithProviders(<DeckMulti {...baseMockProps} />); - // Wait for child slice requests - await waitFor(() => { - expect(SupersetClient.post).toHaveBeenCalled(); - }); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); - // Check that requests don't include dashboardId const { calls } = (SupersetClient.post as jest.Mock).mock; calls.forEach(call => { const formData = call[0].jsonPayload?.form_data || {}; @@ -575,12 +520,8 @@ describe('DeckMulti Component Rendering', () => { renderWithProviders(<DeckMulti {...props} />); - // Wait for child slice requests - await waitFor(() => { - expect(SupersetClient.post).toHaveBeenCalled(); - }); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); - // Verify dashboardId is preserved with filters const { calls } = (SupersetClient.post as jest.Mock).mock; calls.forEach(call => { const formData = call[0].jsonPayload?.form_data || {}; @@ -589,52 +530,33 @@ describe('DeckMulti Component Rendering', () => { }); }); - test('should handle viewport changes', async () => { + test('should reload layers when deck_slices changes', async () => { const { rerender } = renderWithProviders(<DeckMulti {...baseMockProps} />); - // Wait for initial render await waitFor(() => { expect(screen.getByTestId('deckgl-container')).toBeInTheDocument(); }); - - const newViewport = { longitude: 10, latitude: 20, zoom: 8 }; - const updatedProps = { - ...baseMockProps, - viewport: newViewport, - formData: { - ...baseMockProps.formData, - deck_slices: [1, 2, 3], // Change deck_slices to trigger loadLayers - }, - }; + expect(SupersetClient.get).toHaveBeenCalledTimes(2); rerender( <Provider store={mockStore}> <ThemeProvider theme={supersetTheme}> - <DeckMulti {...updatedProps} /> + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, deck_slices: [1, 2, 3] }} + /> </ThemeProvider> </Provider>, ); - await waitFor(() => { - const container = screen.getByTestId('deckgl-container'); - const viewportData = JSON.parse( - container.getAttribute('data-viewport') || '{}', - ); - - expect(viewportData.longitude).toBe(newViewport.longitude); - expect(viewportData.latitude).toBe(newViewport.latitude); - }); + await waitFor(() => expect(SupersetClient.get).toHaveBeenCalledTimes(5)); }); }); test('includes parent_slice_id in child slice requests when parent has slice_id', async () => { jest.clearAllMocks(); - const mockPost = jest.fn().mockResolvedValue({ - json: { - result: [{ data: [] }], - }, - }); - (SupersetClient.post as jest.Mock) = mockPost; + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); const parentSliceId = 99; const dashboardId = 5; @@ -649,12 +571,9 @@ test('includes parent_slice_id in child slice requests when parent has slice_id' renderWithProviders(<DeckMulti {...props} />); - await waitFor(() => { - expect(mockPost).toHaveBeenCalled(); - }); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); - // Check that the child slice requests include parent_slice_id - const { calls } = mockPost.mock; + const { calls } = (SupersetClient.post as jest.Mock).mock; calls.forEach(call => { const formData = call[0].jsonPayload?.form_data || {}; expect(formData).toMatchObject({ @@ -666,12 +585,8 @@ test('includes parent_slice_id in child slice requests when parent has slice_id' test('includes parent_slice_id in embedded mode', async () => { jest.clearAllMocks(); - const mockPost = jest.fn().mockResolvedValue({ - json: { - result: [{ data: [] }], - }, - }); - (SupersetClient.post as jest.Mock) = mockPost; + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); const parentSliceId = 200; const dashboardId = 10; @@ -687,12 +602,9 @@ test('includes parent_slice_id in embedded mode', async () => { renderWithProviders(<DeckMulti {...props} />); - await waitFor(() => { - expect(mockPost).toHaveBeenCalled(); - }); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); - // Verify parent_slice_id is included in embedded mode - const { calls } = mockPost.mock; + const { calls } = (SupersetClient.post as jest.Mock).mock; calls.forEach(call => { const formData = call[0].jsonPayload?.form_data || {}; expect(formData.parent_slice_id).toBe(parentSliceId); @@ -701,30 +613,22 @@ test('includes parent_slice_id in embedded mode', async () => { test('does not include parent_slice_id when parent has no slice_id', async () => { jest.clearAllMocks(); - const mockPost = jest.fn().mockResolvedValue({ - json: { - result: [{ data: [] }], - }, - }); - (SupersetClient.post as jest.Mock) = mockPost; + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); const props = { ...baseMockProps, formData: { ...baseMockProps.formData, - // No slice_id in parent dashboardId: 5, }, }; renderWithProviders(<DeckMulti {...props} />); - await waitFor(() => { - expect(mockPost).toHaveBeenCalled(); - }); + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); - // Verify parent_slice_id is not included when parent has no slice_id - const { calls } = mockPost.mock; + const { calls } = (SupersetClient.post as jest.Mock).mock; calls.forEach(call => { const formData = call[0].jsonPayload?.form_data || {}; expect(formData.parent_slice_id).toBeUndefined(); diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx index b5033e13080..b07e96e259c 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx @@ -81,7 +81,11 @@ type DataMaskState = Record< export type DeckMultiProps = { formData: QueryFormData; - payload: JsonObject; + // deck_multi's buildQuery always returns an empty queries array (every + // layer self-fetches instead), so this is always undefined in practice -- + // kept optional rather than dropped to match the shared transformProps + // shape used across the deck.gl chart family. + payload?: JsonObject; setControlValue: (control: string, value: JsonValue) => void; viewport: Viewport; onAddFilter: HandlerFunction; @@ -150,30 +154,18 @@ const DeckMulti = (props: DeckMultiProps) => { const visibleDeckLayersFromRedux = layerVisibilityFilter?.extraFormData?.visible_deckgl_layers; + // The v1 path fetches every layer client-side (see loadSingleLayer below), + // so there is never pre-merged feature data available at mount or at the + // start of a reload -- only the base viewport, clamped to a non-negative + // zoom. Autozoom itself happens incrementally as each layer's features + // arrive (see the refit in loadSingleLayer). const getAdjustedViewport = useCallback(() => { - let viewport = { ...props.viewport }; - - // Default to autozoom enabled for backward compatibility (undefined treated as true) - // legacy container payloads carried pre-merged features; the v1 path - // fetches every layer client-side, so there may be none here - const features = props.payload?.data?.features || {}; - if (props.formData.autozoom !== false) { - const points = collectPoints(features); - - if (props.formData && points.length > 0) { - viewport = fitViewport(viewport, { - width: props.width, - height: props.height, - points, - }); - } - } - + const viewport = { ...props.viewport }; if (viewport.zoom < 0) { viewport.zoom = 0; } return viewport; - }, [props]); + }, [props.viewport]); const [viewport, setViewport] = useState<Viewport>(getAdjustedViewport()); const [subSlicesLayers, setSubSlicesLayers] = useState<Record<number, Layer>>( @@ -189,6 +181,12 @@ const DeckMulti = (props: DeckMultiProps) => { // getAdjustedViewport has nothing to fit to and the viewport is recomputed // here as each layer arrives. const layerFeaturesRef = useRef<JsonObject>({}); + // Bumped at the start of every loadLayers call. Each in-flight layer fetch + // captures the generation it was started under; if deck_slices (or the + // layer-visibility filter) changes again before that fetch resolves, its + // callback checks the ref and bails out instead of writing a stale layer + // or stale accumulated features into the current (already-reset) state. + const loadGenerationRef = useRef(0); const setTooltip = useCallback((tooltip: TooltipProps['tooltip']) => { const { current } = containerRef; @@ -304,6 +302,7 @@ const DeckMulti = (props: DeckMultiProps) => { subslice: JsonObject, formData: QueryFormData, payloadIndex: number, + generation: number, ): void => { const layerIndex = getLayerIndex( subslice.slice_id, @@ -381,6 +380,12 @@ const DeckMulti = (props: DeckMultiProps) => { result_type: 'full', }, }).then(({ json }) => { + // A newer loadLayers call (deck_slices or the visibility filter + // changed again) has already reset state; this response belongs + // to an abandoned generation and must not write into it. + if (loadGenerationRef.current !== generation) { + return; + } const chartProps = new ChartProps({ width: props.width, height: props.height, @@ -420,6 +425,9 @@ const DeckMulti = (props: DeckMultiProps) => { }); }) .catch(async error => { + if (loadGenerationRef.current !== generation) { + return; + } // Surface the failure in the chart (e.g. a layer bound to a dataset // that is missing the columns it needs) rather than only throwing to // the console. @@ -450,6 +458,8 @@ const DeckMulti = (props: DeckMultiProps) => { slices: ({ slice_id: number } & JsonObject)[], visibleLayers?: number[], ): void => { + loadGenerationRef.current += 1; + const { current: generation } = loadGenerationRef; setViewport(getAdjustedViewport()); setSubSlicesLayers({}); setLayerErrors({}); @@ -476,7 +486,7 @@ const DeckMulti = (props: DeckMultiProps) => { } } - loadSingleLayer(subslice, formData, payloadIndex); + loadSingleLayer(subslice, formData, payloadIndex, generation); }, ); @@ -542,7 +552,7 @@ const DeckMulti = (props: DeckMultiProps) => { ); useEffect(() => { - const { formData, payload } = props; + const { formData } = props; const deckSlicesChanged = !isEqual(prevDeckSlices, formData.deck_slices); const visibilityFilterChanged = !isEqual( @@ -551,15 +561,12 @@ const DeckMulti = (props: DeckMultiProps) => { ); if (deckSlicesChanged || visibilityFilterChanged) { - // legacy explore_json payloads already carried the subslice metadata - const legacySlices = payload?.data?.slices; - if (legacySlices) { - loadLayers(formData, legacySlices, visibleDeckLayersFromRedux); - } else { - fetchSubslices(ensureIsArray(formData.deck_slices) as number[]).then( - slices => loadLayers(formData, slices, visibleDeckLayersFromRedux), - ); - } + // deck_multi issues no query of its own (see buildQuery.ts), so each + // sub-slice's saved form_data is always fetched client-side here -- + // there is no pre-merged payload to read subslice metadata from. + fetchSubslices(ensureIsArray(formData.deck_slices) as number[]).then( + slices => loadLayers(formData, slices, visibleDeckLayersFromRedux), + ); } }, [ loadLayers,
