fix(explore): keep the annotation layer modal usable in a small viewport (#42614)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 13:01:33 +00:00 · 2026-07-30 19:09:58 +02:00
parent 59a5ae0df3
commit 473f447c1b
6 changed files with 684 additions and 56 deletions
@@ -17,6 +17,8 @@
 * under the License.
 */
 import {
+  act,
+  fireEvent,
  render,
  screen,
  selectOption,
@@ -28,6 +30,7 @@ import {
  ChartMetadata,
  VizType,
 } from '@superset-ui/core';
+import { logging } from '@apache-superset/core/utils';
 import fetchMock from 'fetch-mock';
 import setupColors from 'src/setup/setupColors';
 import { ANNOTATION_TYPES_METADATA } from './AnnotationTypes';
@@ -43,9 +46,12 @@ const nativeLayerApiRoute = 'glob:*/api/v1/annotation_layer/*';
 const chartApiRoute = /\/api\/v1\/chart\/\?q=.+/;
 const chartApiWithIdRoute = /\/api\/v1\/chart\/\w+\?q=.+/;

+const chartApiWithIdRouteName = 'chart-with-id';
+
 const withIdResult = {
  result: {
    slice_name: 'Mocked Slice',
+    params: JSON.stringify({ groupby: ['country'] }),
    query_context: JSON.stringify({
      form_data: {
        groupby: ['country'],
@@ -55,6 +61,47 @@ const withIdResult = {
  },
 };

+const setViewportWidth = (value: number) =>
+  Object.defineProperty(document.documentElement, 'clientWidth', {
+    configurable: true,
+    value,
+  });
+
+// jsdom serves `clientWidth` from the prototype and `jest.restoreAllMocks` leaves
+// `defineProperty` alone, so drop the override or every later test inherits it.
+const restoreViewportWidth = () =>
+  Reflect.deleteProperty(document.documentElement, 'clientWidth');
+
+const rect = (width: number, right: number) =>
+  ({
+    width,
+    right,
+    left: right - width,
+    top: 0,
+    bottom: 0,
+    height: 0,
+  }) as DOMRect;
+
+const SECTIONS_TEST_ID = 'annotation-layer-sections';
+
+/**
+ * Lays the popover out beside a control panel of the given width. `sectionWidth`
+ * is what each section wants, which decides whether the row fits on one line.
+ */
+const mockLayout = (panelWidth: () => number, sectionWidth = 0) =>
+  jest
+    .spyOn(Element.prototype, 'getBoundingClientRect')
+    .mockImplementation(function (this: Element) {
+      if (this.id === 'controlSections')
+        return rect(panelWidth(), panelWidth());
+      if (this.classList.contains('ant-popover')) return rect(802, 1422);
+      if (this.getAttribute('data-test') === SECTIONS_TEST_ID)
+        return rect(778, 1410);
+      if (this.parentElement?.getAttribute('data-test') === SECTIONS_TEST_ID)
+        return rect(sectionWidth, sectionWidth);
+      return rect(0, 0);
+    });
+
 beforeAll(() => {
  const supportedAnnotationTypes = Object.values(ANNOTATION_TYPES_METADATA).map(
    value => value.value,
@@ -68,7 +115,9 @@ beforeAll(() => {
    result: [{ id: 'a', slice_name: 'Chart A', viz_type: VizType.Table }],
  });

-  fetchMock.get(chartApiWithIdRoute, withIdResult);
+  fetchMock.get(chartApiWithIdRoute, withIdResult, {
+    name: chartApiWithIdRouteName,
+  });

  setupColors();

@@ -231,6 +280,302 @@ test('keeps apply disabled when missing required fields', async () => {
  expect(screen.getByRole('button', { name: 'Apply' })).toBeDisabled();
 });

+test('renders slice configuration for a chart that has no generated query context', async () => {
+  // A chart never opened in Explore has `query_context: null`, so the columns
+  // have to come from its saved `params`.
+  fetchMock.modifyRoute(chartApiWithIdRouteName, {
+    response: {
+      result: {
+        slice_name: 'Mocked Slice',
+        params: JSON.stringify({ groupby: ['country'] }),
+        query_context: null,
+        viz_type: VizType.Line,
+      },
+    },
+  });
+
+  try {
+    await waitForRender({
+      annotationType: ANNOTATION_TYPES_METADATA.EVENT.value,
+      sourceType: 'Table',
+    });
+
+    await selectOption('Chart A', 'Annotation layer value');
+
+    expect(await screen.findByText(/title column/i)).toBeInTheDocument();
+
+    // The column options come from the saved `params` form data.
+    userEvent.click(
+      screen.getByRole('combobox', { name: 'Annotation layer time column' }),
+    );
+    expect(await screen.findByTitle('country')).toBeInTheDocument();
+  } finally {
+    fetchMock.modifyRoute(chartApiWithIdRouteName, { response: withIdResult });
+  }
+});
+
+test('renders slice configuration on mount for a chart with no generated query context', async () => {
+  fetchMock.modifyRoute(chartApiWithIdRouteName, {
+    response: {
+      result: {
+        slice_name: 'Mocked Slice',
+        params: JSON.stringify({ groupby: ['country'] }),
+        query_context: null,
+        viz_type: VizType.Table,
+      },
+    },
+  });
+
+  try {
+    await waitForRender({
+      name: 'Test',
+      value: 'a',
+      annotationType: ANNOTATION_TYPES_METADATA.EVENT.value,
+      sourceType: 'Table',
+    });
+
+    expect(await screen.findByText(/title column/i)).toBeInTheDocument();
+  } finally {
+    fetchMock.modifyRoute(chartApiWithIdRouteName, { response: withIdResult });
+  }
+});
+
+test('survives a chart endpoint that fails', async () => {
+  // The bug this replaces was an unhandled rejection that left the popover in a
+  // half-populated state, so a failure has to stay contained and reported.
+  const logError = jest.spyOn(logging, 'error').mockImplementation(() => {});
+  fetchMock.modifyRoute(chartApiWithIdRouteName, { response: 500 });
+
+  try {
+    await waitForRender({
+      name: 'Test',
+      value: 'a',
+      annotationType: ANNOTATION_TYPES_METADATA.EVENT.value,
+      sourceType: 'Table',
+    });
+
+    expect(screen.getByRole('textbox', { name: 'Name' })).toBeInTheDocument();
+    await waitFor(() =>
+      expect(logError).toHaveBeenCalledWith(
+        expect.stringContaining('Failed to load annotation source chart a'),
+        expect.anything(),
+      ),
+    );
+    expect(screen.queryByText(/title column/i)).not.toBeInTheDocument();
+  } finally {
+    fetchMock.modifyRoute(chartApiWithIdRouteName, { response: withIdResult });
+    logError.mockRestore();
+  }
+});
+
+test('reports a chart that carries no form data at all', async () => {
+  // Neither column is guaranteed: `query_context` is backfilled lazily and
+  // `params` can be empty, and then there is nothing to build the fields from.
+  const logWarn = jest.spyOn(logging, 'warn').mockImplementation(() => {});
+  fetchMock.modifyRoute(chartApiWithIdRouteName, {
+    response: {
+      // `VizType.Table` is the registered one, so the chart clears the
+      // annotation-type check and reaches the form data.
+      result: {
+        ...withIdResult.result,
+        params: null,
+        query_context: null,
+        viz_type: VizType.Table,
+      },
+    },
+  });
+
+  try {
+    await waitForRender({
+      name: 'Test',
+      value: 'a',
+      annotationType: ANNOTATION_TYPES_METADATA.EVENT.value,
+      sourceType: 'Table',
+    });
+
+    await waitFor(() =>
+      expect(logWarn).toHaveBeenCalledWith(
+        expect.stringContaining('has no usable form data'),
+      ),
+    );
+    expect(screen.queryByText(/title column/i)).not.toBeInTheDocument();
+  } finally {
+    fetchMock.modifyRoute(chartApiWithIdRouteName, { response: withIdResult });
+    logWarn.mockRestore();
+  }
+});
+
+test('bounds the section row to the viewport so the footer stays reachable', async () => {
+  // Adding the slice configuration section must not push the display
+  // configuration or the Apply/OK buttons past the viewport.
+  await waitForRender({
+    annotationType: ANNOTATION_TYPES_METADATA.EVENT.value,
+    sourceType: 'Table',
+  });
+
+  const sections = screen.getByTestId('annotation-layer-sections');
+  expect(sections).toHaveStyle('flex-wrap: wrap');
+  expect(sections).toHaveStyle('max-width: calc(100vw - 64px)');
+  // The footer is a sibling of the sections, never inside the row.
+  expect(sections).not.toContainElement(
+    screen.getByRole('button', { name: 'Apply' }),
+  );
+});
+
+test('caps the section row to the room left beside the control panel', async () => {
+  // The cap is measured from the panel's edge, not the popover's own: the
+  // popover moves as the row narrows, so measuring it feeds each cap into the
+  // next one and converges on a column too narrow to lay the sections out in.
+  setViewportWidth(1160);
+  // Two sections at 238 need 508 to stay on one line, which is what fits beside
+  // the panel here.
+  mockLayout(() => 620, 238);
+
+  try {
+    await waitFor(() =>
+      render(
+        <>
+          <div id="controlSections" />
+          <div className="ant-popover">
+            <AnnotationLayer
+              {...defaultProps}
+              annotationType={ANNOTATION_TYPES_METADATA.EVENT.value}
+              sourceType="Table"
+            />
+          </div>
+        </>,
+      ),
+    );
+
+    // 1160 viewport - 620 panel - 24 popover padding - 8 inset
+    expect(screen.getByTestId('annotation-layer-sections')).toHaveStyle(
+      'max-width: 508px',
+    );
+
+    // A cap measured once would go stale across a resize. Here the room beside
+    // the panel drops to 348px, too little to keep both sections on one line, so
+    // the viewport becomes the bound: 1000 - 24 padding - 2 x 8 inset.
+    setViewportWidth(1000);
+    fireEvent(window, new Event('resize'));
+
+    await waitFor(() =>
+      expect(screen.getByTestId('annotation-layer-sections')).toHaveStyle(
+        'max-width: 960px',
+      ),
+    );
+  } finally {
+    restoreViewportWidth();
+    jest.restoreAllMocks();
+  }
+});
+
+test('caps the section row again when the control panel is dragged wider', async () => {
+  // Dragging the panel's resizer moves the edge the cap comes from without
+  // resizing the window, and the jsdom ResizeObserver never calls back, so stand
+  // in for it here and check that the panel is what gets watched.
+  const observed: Element[] = [];
+  let notifyResize = () => {};
+  const NativeResizeObserver = window.ResizeObserver;
+  window.ResizeObserver = class {
+    constructor(callback: ResizeObserverCallback) {
+      notifyResize = () => callback([], this as unknown as ResizeObserver);
+    }
+
+    observe(target: Element) {
+      observed.push(target);
+    }
+
+    unobserve() {}
+
+    disconnect() {}
+  };
+
+  setViewportWidth(1160);
+  let panelWidth = 620;
+  mockLayout(() => panelWidth, 238);
+
+  try {
+    render(
+      <>
+        <div id="controlSections" />
+        <div className="ant-popover">
+          <AnnotationLayer
+            {...defaultProps}
+            annotationType={ANNOTATION_TYPES_METADATA.EVENT.value}
+            sourceType="Table"
+          />
+        </div>
+      </>,
+    );
+
+    await waitFor(() =>
+      expect(screen.getByTestId('annotation-layer-sections')).toHaveStyle(
+        'max-width: 508px',
+      ),
+    );
+    expect(observed).toContain(document.getElementById('controlSections'));
+
+    // 228px is left beside the panel, too little to keep both sections on one
+    // line, so the bound falls back to the viewport: 1160 - 24 - 2 x 8 inset.
+    panelWidth = 900;
+    act(() => notifyResize());
+
+    await waitFor(() =>
+      expect(screen.getByTestId('annotation-layer-sections')).toHaveStyle(
+        'max-width: 1120px',
+      ),
+    );
+  } finally {
+    window.ResizeObserver = NativeResizeObserver;
+    restoreViewportWidth();
+    jest.restoreAllMocks();
+  }
+});
+
+test('leaves the slice configuration section on one line rather than wrapping', async () => {
+  // Wrapping the third section roughly doubles the popover's height - measured at
+  // 620px of sections against 256px unwrapped - and a popover taller than the
+  // viewport cannot be shifted back in, so the footer ends up below the fold.
+  // Room beside the panel is preferred only while the row still fits on one line.
+  setViewportWidth(1160);
+  mockLayout(() => 620, 238);
+  fetchMock.modifyRoute(chartApiWithIdRouteName, {
+    response: {
+      result: { ...withIdResult.result, viz_type: VizType.Table },
+    },
+  });
+
+  try {
+    render(
+      <>
+        <div id="controlSections" />
+        <div className="ant-popover">
+          <AnnotationLayer
+            {...defaultProps}
+            name="Test"
+            value="a"
+            annotationType={ANNOTATION_TYPES_METADATA.EVENT.value}
+            sourceType="Table"
+          />
+        </div>
+      </>,
+    );
+
+    // Three sections need 778px for one line and only 508px is free beside the
+    // panel, so the viewport bounds it instead: 1160 - 24 padding - 2 x 8 inset.
+    expect(await screen.findByText(/title column/i)).toBeInTheDocument();
+    await waitFor(() =>
+      expect(screen.getByTestId('annotation-layer-sections')).toHaveStyle(
+        'max-width: 1120px',
+      ),
+    );
+  } finally {
+    fetchMock.modifyRoute(chartApiWithIdRouteName, { response: withIdResult });
+    restoreViewportWidth();
+    jest.restoreAllMocks();
+  }
+});
+
 test('Disable apply button if formula is incorrect', async () => {
  await waitForRender({ name: 'test' });