You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by en...@apache.org on 2026/07/30 12:35:02 UTC
(superset) 01/01: fix(explore): keep the annotation layer modal usable in a small viewport
This is an automated email from the ASF dual-hosted git repository.
EnxDev pushed a commit to branch enxdev/fix/annotation-layer-modal-overflow-and-slice-config
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 8ed97fcdf53c480a0fe71126ab00393b97d9aca2
Author: Enzo Martellucci <en...@gmail.com>
AuthorDate: Thu Jul 30 11:11:30 2026 +0200
fix(explore): keep the annotation layer modal usable in a small viewport
Two defects in the "Add annotation layer" modal, both surfacing when a chart
is picked as the annotation source.
Annotation Slice Configuration fields never rendered
----------------------------------------------------
`fetchSliceData` and `fetchAppliedChart` read the source chart's form data as
`JSON.parse(result.query_context).form_data`. `query_context` is a derived
cache, backfilled the first time a chart is opened in Explore, so a chart that
has never been opened there has `query_context: null`. `JSON.parse(null)`
returns `null`, reading `.form_data` off it throws, the promise rejects
unhandled and `slice` is never set: the fields stay absent and Apply/OK stay
disabled. Nearly every chart is affected, since the column is only ever
populated as a side effect of visiting Explore.
The form data now resolves through a helper that prefers
`query_context.form_data` and falls back to the chart's saved `params`, which
hold the same form data and are always present. Parsing is guarded and both
requests handle rejection instead of failing silently.
Modal overflowed the viewport, hiding Display configuration and Apply/OK
----------------------------------------------------------------------
The sections were laid out in an unbounded, non-wrapping flex row, so the third
one pushed the popover past a narrow viewport - an embedding iframe, typically.
Nothing could recover from it: antd only configures `shiftX`/`shiftY` for the
four base placements, and ControlPopover computes corner placements, which get
`{adjustX, adjustY}` alone. Those can flip a popup across its trigger but never
nudge it back into view, so when neither side fitted the popover simply stayed
off screen with its footer unreachable.
ControlPopover now opts into shifting, so a popup that does not fit is pulled
back rather than left hanging. That alone still leaves `popupOffsetX` behind,
because the shift discounts it, so the section row is additionally capped to the
room left beside the control panel and wraps within it. The panel's edge is the
anchor rather than the popover's own, which trails its content width by several
frames and would feed each cap into the next measurement.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../AnnotationLayer.test.tsx | 136 ++++++++++++++-
.../AnnotationLayerControl/AnnotationLayer.tsx | 182 ++++++++++++++++-----
.../ControlPopover/ControlPopover.test.tsx | 39 ++++-
.../controls/ControlPopover/ControlPopover.tsx | 13 ++
4 files changed, 326 insertions(+), 44 deletions(-)
diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx
index b9f9f9f1a06..d67ee9b772f 100644
--- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx
@@ -43,9 +43,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'],
@@ -68,7 +71,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 +236,135 @@ 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('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.
+ Object.defineProperty(document.documentElement, 'clientWidth', {
+ configurable: true,
+ value: 1160,
+ });
+ const rect = (width: number, right: number) =>
+ ({
+ width,
+ right,
+ left: right - width,
+ top: 0,
+ bottom: 0,
+ height: 0,
+ }) as DOMRect;
+ jest
+ .spyOn(Element.prototype, 'getBoundingClientRect')
+ .mockImplementation(function (this: Element) {
+ if (this.id === 'controlSections') return rect(620, 620);
+ if (this.classList.contains('ant-popover')) return rect(802, 1422);
+ if (this.getAttribute('data-test') === 'annotation-layer-sections')
+ return rect(778, 1410);
+ return rect(0, 0);
+ });
+
+ 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',
+ );
+ } finally {
+ jest.restoreAllMocks();
+ }
+});
+
test('Disable apply button if formula is incorrect', async () => {
await waitForRender({ name: 'test' });
diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx
index d90765ab0cb..c9e99da6382 100644
--- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx
@@ -16,7 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { useCallback, useEffect, useMemo, useState } from 'react';
+import React, {
+ useCallback,
+ useEffect,
+ useLayoutEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
import rison from 'rison';
import {
Button,
@@ -36,6 +43,7 @@ import {
} from '@superset-ui/core';
import { t } from '@apache-superset/core/translation';
import { styled, useTheme } from '@apache-superset/core/theme';
+import { logging } from '@apache-superset/core/utils';
import SelectControl from 'src/explore/components/controls/SelectControl';
import TextControl from 'src/explore/components/controls/TextControl';
import CheckboxControl from 'src/explore/components/controls/CheckboxControl';
@@ -103,6 +111,57 @@ interface AnnotationLayerProps {
const AUTOMATIC_COLOR = '';
+/** Space between the popover's side-by-side configuration sections. */
+const SECTION_GAP = 32;
+
+/** Explore's control panel, which the popover opens alongside. */
+const CONTROL_SECTIONS_ID = 'controlSections';
+
+/** Breathing room kept between the popover and the viewport edge. */
+const VIEWPORT_INSET = 8;
+
+interface ChartApiResult {
+ params?: string | null;
+ query_context?: string | null;
+}
+
+const parseJsonObject = (
+ raw: string | null | undefined,
+): Record<string, unknown> | null => {
+ if (!raw) {
+ return null;
+ }
+ try {
+ const parsed: unknown = JSON.parse(raw);
+ return parsed && typeof parsed === 'object' && !Array.isArray(parsed)
+ ? (parsed as Record<string, unknown>)
+ : null;
+ } catch {
+ return null;
+ }
+};
+
+// `query_context` is only backfilled once a chart is opened in Explore, so fall
+// back to `params`, which holds the same form data and is always present.
+const getSliceFormData = (
+ result: ChartApiResult,
+): Record<string, unknown> | null => {
+ const formData = parseJsonObject(result.query_context)?.form_data;
+ if (formData && typeof formData === 'object') {
+ return formData as Record<string, unknown>;
+ }
+ return parseJsonObject(result.params);
+};
+
+const toSliceData = (formData: Record<string, unknown>): SliceData => ({
+ data: {
+ ...formData,
+ groupby: (formData.groupby as QueryFormColumn[] | undefined)?.map(column =>
+ getColumnLabel(column),
+ ),
+ },
+});
+
const NotFoundContentWrapper = styled.div`
&& > div:first-of-type {
padding-left: 0;
@@ -220,6 +279,8 @@ function AnnotationLayer({
const [hideLine, setHideLine] = useState(propHideLine ?? false);
const [isNew, setIsNew] = useState(!propName);
const [slice, setSlice] = useState<SliceData | null>(null);
+ const sectionsRef = useRef<HTMLDivElement>(null);
+ const [sectionsMaxWidth, setSectionsMaxWidth] = useState<number>();
const getSupportedSourceTypes = useCallback(
(annoType: string): SelectOption[] => {
@@ -354,58 +415,59 @@ function AnnotationLayer({
const fetchSliceData = useCallback((id: string | number): void => {
const queryParams = rison.encode({
- columns: ['query_context'],
+ columns: ['params', 'query_context'],
});
SupersetClient.get({
endpoint: `/api/v1/chart/${id}?q=${queryParams}`,
- }).then(({ json }) => {
- const { result } = json;
- const queryContext = result.query_context;
- const formData = JSON.parse(queryContext).form_data;
- const dataObject = {
- data: {
- ...formData,
- groupby: formData.groupby?.map((column: QueryFormColumn) =>
- getColumnLabel(column),
- ),
- },
- };
- setSlice(dataObject);
- });
+ })
+ .then(({ json }) => {
+ const formData = getSliceFormData(json.result);
+ if (formData) {
+ setSlice(toSliceData(formData));
+ } else {
+ logging.warn(
+ `Annotation source chart ${id} has no usable form data; ` +
+ 'the slice configuration fields cannot be populated.',
+ );
+ }
+ })
+ .catch(error => {
+ logging.error(`Failed to load annotation source chart ${id}`, error);
+ });
}, []);
const fetchAppliedChart = useCallback(
(id: string | number): void => {
const registry = getChartMetadataRegistry();
const queryParams = rison.encode({
- columns: ['slice_name', 'query_context', 'viz_type'],
+ columns: ['slice_name', 'params', 'query_context', 'viz_type'],
});
SupersetClient.get({
endpoint: `/api/v1/chart/${id}?q=${queryParams}`,
- }).then(({ json }) => {
- const { result } = json;
- const sliceName = result.slice_name;
- const queryContext = result.query_context;
- const chartVizType = result.viz_type;
- const formData = JSON.parse(queryContext).form_data;
- const metadata = registry.get(chartVizType);
- const canBeAnnotationType =
- metadata && metadata.canBeAnnotationType(annotationType);
- if (canBeAnnotationType) {
+ })
+ .then(({ json }) => {
+ const { result } = json;
+ const metadata = registry.get(result.viz_type);
+ if (!metadata?.canBeAnnotationType(annotationType)) {
+ return;
+ }
setValue({
value: id,
- label: sliceName,
- });
- setSlice({
- data: {
- ...formData,
- groupby: formData.groupby?.map((column: QueryFormColumn) =>
- getColumnLabel(column),
- ),
- },
+ label: result.slice_name,
});
- }
- });
+ const formData = getSliceFormData(result);
+ if (formData) {
+ setSlice(toSliceData(formData));
+ } else {
+ logging.warn(
+ `Annotation source chart ${id} has no usable form data; ` +
+ 'the slice configuration fields cannot be populated.',
+ );
+ }
+ })
+ .catch(error => {
+ logging.error(`Failed to load annotation source chart ${id}`, error);
+ });
},
[annotationType],
);
@@ -740,7 +802,7 @@ function AnnotationLayer({
? [{ value: '__timestamp', label: '__timestamp' }].concat(columns)
: columns;
return (
- <div style={{ marginRight: '2rem' }}>
+ <div>
<PopoverSection
isSelected
title={t('Annotation Slice Configuration')}
@@ -1010,6 +1072,29 @@ function AnnotationLayer({
theme,
]);
+ const sliceConfiguration = renderSliceConfiguration();
+ const hasSliceConfiguration = !!sliceConfiguration;
+
+ useLayoutEffect(() => {
+ const row = sectionsRef.current;
+ const popover = row?.closest('.ant-popover');
+ const panel = document.getElementById(CONTROL_SECTIONS_ID);
+ if (!row || !popover || !panel) {
+ return;
+ }
+ // Padding and border the popover adds around the row.
+ const popoverInsetWidth =
+ popover.getBoundingClientRect().width - row.getBoundingClientRect().width;
+ const available =
+ document.documentElement.clientWidth -
+ panel.getBoundingClientRect().right -
+ popoverInsetWidth -
+ VIEWPORT_INSET;
+ if (available > 0) {
+ setSectionsMaxWidth(available);
+ }
+ }, [hasSliceConfiguration]);
+
const isValid = isValidForm();
const metadata = vizType ? getChartMetadataRegistry().get(vizType) : null;
const supportedAnnotationTypes = metadata
@@ -1029,8 +1114,21 @@ function AnnotationLayer({
{t('ERROR')}: {error}
</span>
)}
- <div style={{ display: 'flex', flexDirection: 'row' }}>
- <div style={{ marginRight: '2rem' }}>
+ <div
+ ref={sectionsRef}
+ data-test="annotation-layer-sections"
+ style={{
+ display: 'flex',
+ flexDirection: 'row',
+ // A third section can outgrow a narrow viewport, and an oversized
+ // popover cannot be shrunk by antd, only flipped. Bounding the row
+ // compresses the sections instead of pushing the footer off screen.
+ flexWrap: 'wrap',
+ maxWidth: sectionsMaxWidth ?? `calc(100vw - ${SECTION_GAP * 2}px)`,
+ gap: SECTION_GAP,
+ }}
+ >
+ <div>
<PopoverSection
isSelected
title={t('Layer configuration')}
@@ -1086,7 +1184,7 @@ function AnnotationLayer({
{renderValueConfiguration()}
</PopoverSection>
</div>
- {renderSliceConfiguration()}
+ {sliceConfiguration}
{renderDisplayConfiguration()}
</div>
<div style={{ display: 'flex', justifyContent: 'space-between' }}>
diff --git a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx
index fcb9d07d741..e7b044483bb 100644
--- a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx
+++ b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx
@@ -24,7 +24,14 @@ import {
waitFor,
} from 'spec/helpers/testing-library';
-import ControlPopover, { PopoverProps } from './ControlPopover';
+// Reaching into antd's internals pins the overflow behaviour ControlPopover
+// relies on, so an upgrade that changes it fails here.
+import { getOverflowOptions } from 'antd/lib/_util/placements';
+
+import ControlPopover, {
+ AUTO_ADJUST_OVERFLOW,
+ PopoverProps,
+} from './ControlPopover';
const createProps = (): Partial<PopoverProps> => ({
trigger: 'click',
@@ -182,3 +189,33 @@ test('Controlled mode', async () => {
expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
});
});
+
+test('Keeps an oversized popover reachable inside the viewport', () => {
+ // antd ships shifting only for the base placements, so the corner placements
+ // this component computes have to ask for it explicitly.
+ const arrowOffset = { arrowOffsetHorizontal: 12, arrowOffsetVertical: 12 };
+
+ expect(
+ getOverflowOptions('rightBottom', arrowOffset, 16, true),
+ ).not.toMatchObject({ shiftX: expect.anything() });
+
+ for (const placement of [
+ 'right',
+ 'rightTop',
+ 'rightBottom',
+ 'left',
+ 'leftTop',
+ 'leftBottom',
+ 'top',
+ 'bottom',
+ ]) {
+ expect(
+ getOverflowOptions(placement, arrowOffset, 16, AUTO_ADJUST_OVERFLOW),
+ ).toMatchObject({
+ adjustX: 1,
+ adjustY: 1,
+ shiftX: true,
+ shiftY: true,
+ });
+ }
+});
diff --git a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
index cabd5d0bb07..5f4491ed5b3 100644
--- a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
+++ b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
@@ -48,12 +48,24 @@ export type PopoverProps = BasePopoverProps & {
getVisibilityRatio?: typeof getElementVisibilityRatio;
};
+// antd enables shifting only for the four base placements, so the corner
+// placements computed below can flip a popup across its trigger but never nudge
+// it back into the viewport, leaving an oversized one stranded off screen.
+// `shiftX`/`shiftY` work but are missing from `AdjustOverflow`, hence the cast.
+export const AUTO_ADJUST_OVERFLOW = {
+ adjustX: 1,
+ adjustY: 1,
+ shiftX: true,
+ shiftY: true,
+} as unknown as BasePopoverProps['autoAdjustOverflow'];
+
const ControlPopover: FC<PopoverProps> = ({
getPopupContainer,
getVisibilityRatio = getElementVisibilityRatio,
open: visibleProp,
destroyOnHidden = false,
placement: initialPlacement = 'right',
+ autoAdjustOverflow = AUTO_ADJUST_OVERFLOW,
...props
}) => {
const triggerElementRef = useRef<HTMLElement>();
@@ -197,6 +209,7 @@ const ControlPopover: FC<PopoverProps> = ({
{...props}
open={visible}
arrow={{ pointAtCenter: true }}
+ autoAdjustOverflow={autoAdjustOverflow}
placement={placement}
onOpenChange={handleOnVisibleChange}
getPopupContainer={handleGetPopupContainer}