codeant-ai-for-open-source[bot] commented on code in PR #42530:
URL: https://github.com/apache/superset/pull/42530#discussion_r3664019582


##########
superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx:
##########
@@ -0,0 +1,186 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { act, render } from '@testing-library/react';
+import {
+  CategoricalColorNamespace,
+  getLabelsColorMap,
+} from '@superset-ui/core';
+import { supersetTheme, ThemeProvider } from '@apache-superset/core/theme';
+import EchartsRose from '../../src/Rose/EchartsRose';
+import { RoseChartTransformedProps, RosePeriod } from '../../src/Rose/types';
+import Echart from '../../src/components/Echart';
+
+jest.mock('../../src/components/Echart', () => ({
+  __esModule: true,
+  default: jest.fn(() => null),
+}));
+
+const mockedEchart = jest.mocked(Echart);
+
+// Entries are NOT in seriesNames order -- East/West/North -- deliberately,
+// to prove colors come from priming the scale in seriesNames order rather
+// than from whatever order the drilled pie happens to sort slices into.
+const SERIES_NAMES = ['East', 'West', 'North'];
+const PERIODS: RosePeriod[] = [
+  {
+    time: 1,
+    label: '2021',
+    entries: [
+      { seriesName: 'West', value: 5, increment: 0.1 },
+      { seriesName: 'North', value: 30, increment: 0.5 },
+      { seriesName: 'East', value: 10, increment: 0.3 },
+    ],
+  },
+  {
+    time: 2,
+    label: '2022',
+    entries: [
+      { seriesName: 'East', value: 7, increment: 0.2 },
+      { seriesName: 'West', value: 3, increment: 0.1 },
+      { seriesName: 'North', value: 1, increment: 0.05 },
+    ],
+  },
+];
+
+const ROSE_ECHART_OPTIONS = {
+  legend: { data: SERIES_NAMES },
+  series: [{ id: 'rose', type: 'bar' }],
+};
+
+const baseProps: RoseChartTransformedProps = {
+  height: 400,
+  width: 800,
+  echartOptions: ROSE_ECHART_OPTIONS as any,
+  refs: {},
+  formData: { vizType: 'rose' } as any,
+  periods: PERIODS,
+  seriesNames: SERIES_NAMES,
+  numberFormat: 'SMART_NUMBER',
+  sliceId: 1,
+  colorScheme: '',
+  groupby: [],
+  labelMap: {},
+  setDataMask: jest.fn(),
+  selectedValues: {},
+  emitCrossFilters: false,
+};
+
+function lastEchartCall() {
+  return mockedEchart.mock.calls[mockedEchart.mock.calls.length - 1][0] as any;
+}
+
+function renderRose(props: RoseChartTransformedProps = baseProps) {
+  return render(
+    <ThemeProvider theme={supersetTheme}>
+      <EchartsRose {...props} />
+    </ThemeProvider>,
+  );
+}
+
+beforeEach(() => {
+  mockedEchart.mockClear();
+  // getColor() remembers a label's color for a given sliceId in a
+  // process-wide singleton (so a dashboard's colors stay consistent across
+  // charts); without resetting it here, whichever test runs first would
+  // permanently decide these labels' colors for every later test in this
+  // file, masking a priming-order regression instead of catching it.
+  getLabelsColorMap().reset();
+});
+
+test('renders the rose view unchanged until a period is clicked', () => {
+  renderRose();
+
+  expect(lastEchartCall().echartOptions).toBe(ROSE_ECHART_OPTIONS);
+});
+
+test('clicking a period drills into a pie of that period, sorted 
largest-first', () => {
+  renderRose();
+
+  act(() => {
+    lastEchartCall().eventHandlers.click({ dataIndex: 0 });
+  });
+
+  const { echartOptions } = lastEchartCall();
+  expect(echartOptions.title.text).toContain('2021');
+  expect(echartOptions.series[0].data.map((d: any) => d.name)).toEqual([
+    'North',
+    'East',
+    'West',
+  ]);
+  expect(echartOptions.series[0].data.map((d: any) => d.value)).toEqual([
+    30, 10, 5,
+  ]);
+});
+
+test('drilled slice colors match the rose colors, primed in seriesNames 
order', () => {
+  // Compute the expected mapping first, from a clean slate, priming a scale
+  // in seriesNames order exactly as the rose view does -- independent of
+  // the pie's own value-sorted order (North/East/West by value here, vs.
+  // East/West/North in seriesNames).
+  const expectedColorFn = CategoricalColorNamespace.getScale('');
+  const expectedColors: Record<string, string> = {};
+  SERIES_NAMES.forEach(name => {
+    expectedColors[name] = expectedColorFn(name, baseProps.sliceId);
+  });
+
+  // Reset before rendering so the component primes its own scale from the
+  // same clean slate, rather than inheriting the mapping just computed
+  // above (colors are remembered per sliceId in a process-wide singleton,
+  // so without this reset the comparison below would be tautological).
+  getLabelsColorMap().reset();
+  renderRose();
+  act(() => {
+    lastEchartCall().eventHandlers.click({ dataIndex: 0 });
+  });
+  const { echartOptions } = lastEchartCall();
+
+  echartOptions.series[0].data.forEach((slice: any) => {
+    expect(slice.itemStyle.color).toBe(expectedColors[slice.name]);
+  });
+});
+
+test('clicking again while drilled returns to the rose view', () => {
+  renderRose();
+
+  act(() => {
+    lastEchartCall().eventHandlers.click({ dataIndex: 1 });
+  });
+  expect(lastEchartCall().echartOptions).not.toBe(ROSE_ECHART_OPTIONS);
+
+  act(() => {
+    lastEchartCall().eventHandlers.click({ dataIndex: 0 });
+  });
+  expect(lastEchartCall().echartOptions).toBe(ROSE_ECHART_OPTIONS);
+});
+
+test('clicking a different period while drilled switches to that period', () 
=> {

Review Comment:
   **Suggestion:** The test name says that clicking a different period switches 
to that period, but the assertion expects the component to return to the rose 
view and never checks the second period's pie data. This makes the test 
misleading and would allow a regression where period switching is incorrectly 
implemented to pass; either rename the test to describe the toggle behavior or 
assert the intended second-period result. [inconsistent naming]
   
   <details>
   <summary><b>Severity Level:</b> Minor ๐Ÿงน</summary>
   
   ```mdx
   - โš ๏ธ Test documentation misstates Rose click behavior.
   - โš ๏ธ A period-switching regression is not covered.
   - โš ๏ธ Reviewers may infer unsupported interaction behavior.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction โœ… </b></summary>
   
   ```mdx
   1. Run the Rose test suite containing
   
`superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx:172`;
 the
   test is executed on every suite run.
   
   2. The test clicks period index 0 at
   
`superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx:175-178`
   and verifies the 2021 pie.
   
   3. It then clicks another period at
   
`superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx:182-183`
   but asserts the original rose options at line 185, rather than checking the 
second
   period's pie.
   
   4. This assertion matches the implemented toggle in
   
`superset-frontend/plugins/plugin-chart-echarts/src/Rose/EchartsRose.tsx:119-125`,
 where
   any click while drilled sets `drillIndex` back to `null`; rename the test to 
describe
   returning to the rose view, or change the component and assertions if period 
switching is
   the intended behavior.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9cc12ae740ed489fa936e816fa89b957&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9cc12ae740ed489fa936e816fa89b957&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent ๐Ÿค– </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx
   **Line:** 172:172
   **Comment:**
        *Inconsistent Naming: The test name says that clicking a different 
period switches to that period, but the assertion expects the component to 
return to the rose view and never checks the second period's pie data. This 
makes the test misleading and would allow a regression where period switching 
is incorrectly implemented to pass; either rename the test to describe the 
toggle behavior or assert the intended second-period result.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=273c5d73e4acd38730b495b4aa2ffa496f93c2cb78f9f217bcc4b865c58fb995&reaction=like'>๐Ÿ‘</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=273c5d73e4acd38730b495b4aa2ffa496f93c2cb78f9f217bcc4b865c58fb995&reaction=dislike'>๐Ÿ‘Ž</a>



##########
superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx:
##########
@@ -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;
+          }),
+      )

Review Comment:
   **Suggestion:** The resolver for the first-generation requests is 
overwritten by the second request, so `resolveFirstLoad` only resolves the 
second stale promise. The first layer request remains pending and the test 
never verifies that a stale slice-1 response is ignored; retain separate 
resolvers and explicitly resolve the slice-1 request after the rerender. 
[incomplete implementation]
   
   <details>
   <summary><b>Severity Level:</b> Major โš ๏ธ</summary>
   
   ```mdx
   - โš ๏ธ Stale-response protection lacks effective regression coverage.
   - โš ๏ธ Future changes could reintroduce stale layer contamination.
   - โš ๏ธ The test may pass while the targeted callback remains unverified.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction โœ… </b></summary>
   
   ```mdx
   1. Run 
`superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx`,
   specifically the stale-response test beginning at line 394.
   
   2. The initial `deck_slices: [1, 2]` render starts two POST requests at 
lines 397-408;
   both promises assign their resolver to the same `resolveFirstLoad` variable, 
so the second
   request overwrites the resolver for the first.
   
   3. After rerendering with `deck_slices: [2]` at lines 419-427, the third 
request resolves
   through the default mock at lines 409-411, allowing the current generation 
to produce one
   layer.
   
   4. Calling `resolveFirstLoad(...)` at line 435 resolves only the second 
initial-generation
   promise, not the first slice-1 promise; the slice-1 response remains 
pending, so the test
   can pass without exercising the stale slice-1 callback or proving the 
generation guard in
   `superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx:184-189`.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3fc169fe3d0c4965a4ef41c8f3554783&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3fc169fe3d0c4965a4ef41c8f3554783&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent ๐Ÿค– </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx
   **Line:** 397:408
   **Comment:**
        *Incomplete Implementation: The resolver for the first-generation 
requests is overwritten by the second request, so `resolveFirstLoad` only 
resolves the second stale promise. The first layer request remains pending and 
the test never verifies that a stale slice-1 response is ignored; retain 
separate resolvers and explicitly resolve the slice-1 request after the 
rerender.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=f2688675510cc0349a179d28b6c782cd79673a5776857fd6f50e91f98c5e354e&reaction=like'>๐Ÿ‘</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=f2688675510cc0349a179d28b6c782cd79673a5776857fd6f50e91f98c5e354e&reaction=dislike'>๐Ÿ‘Ž</a>



-- 
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]

Reply via email to