Compare commits

...

1 Commits

Author SHA1 Message Date
sadpandajoe
ca32d9b422 fix(dashboard): keep More filters reachable after applying a cross-filter
When a horizontal filter bar has enough native filters to overflow into the
"More filters" dropdown, applying a cross-filter (which prepends a chip to the
bar's item list) could make the overflowed native filters vanish from BOTH the
bar and the dropdown, while the "More filters" button itself also disappeared,
leaving the hidden filters unreachable. Clearing the cross-filter restored them.

Root cause: when the item set changes, DropdownContainer resets its positional
overflow index and re-measures. If that measurement runs against a transient
mid-reflow layout, it can conclude "nothing overflows" and latch that verdict
(the recalculation effect's dependencies do not change again, so it never
self-corrects). Because the trigger's visibility is derived solely from the
overflow count, that single bad verdict both strands the surplus filters in the
clipped bar and removes the trigger to reach them.

Fix: treat a post-item-change "nothing overflows" read as provisional and run a
single requestAnimationFrame confirmation pass that re-measures once the browser
has reflowed, keeping the trigger mounted across the confirmation window. The
confirmation is armed on every item-set change (so a fit->overflow transition is
covered, not only the already-overflowing case) and is versioned and cancelled
so a superseded frame from a rapid second change cannot clobber the newer state.
This extends the intent of #38193 (which guarded only the transient reset
window) to also cover a settled bad read, and is in the same overflow/button
visibility area as #28060.

Adds DropdownContainer.overflow.test.tsx, which drives the real overflow
recalculation (mocking only the two measurement sources) and covers: a clean
re-measurement after a prepended chip, the transient-latch regression, the
fit->overflow transition, over-correction (genuine fit drops the trigger), and
re-entrant item-set changes.
2026-06-24 02:13:43 +00:00
2 changed files with 490 additions and 37 deletions

View File

@@ -0,0 +1,325 @@
/**
* 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.
*/
/**
* Overflow-engine regression tests for DropdownContainer.
*
* jsdom has no real layout, so these tests drive the component's real overflow
* recalculation by mocking the two measurement sources it reads:
* 1. `useResizeDetector` — supplies the container width.
* 2. `getBoundingClientRect` — supplies per-element geometry. The inner
* `data-test="container"` spans [0, containerRight]; every child is
* ITEM_W wide and laid out left-to-right by its DOM index, so children
* whose right edge exceeds `containerRight` overflow.
*
* This exercises the production code path in DropdownContainer.tsx
* (useLayoutEffect → overflowingIndex → notOverflowedItems/overflowedItems →
* showDropdownButton) rather than mocking the result.
*/
import { screen, render, waitFor, act } from '@superset-ui/core/spec';
import * as resizeDetector from 'react-resize-detector';
import { DropdownContainer } from '..';
const ITEM_W = 100;
// 350px container ⇒ at most 3 items (rights 100/200/300) fit before overflow.
const BAR_WIDTH = 350;
// Mutable so a test can simulate the transient layout window where a freshly
// enlarged item set is momentarily measured as fitting before reflow settles.
let containerRight = BAR_WIDTH;
// Mutable width fed to the component through the mocked resize detector.
let mockWidth = 0;
// Stable ref object React attaches the outer node to (mirrors useResizeDetector).
const fakeRef: { current: HTMLDivElement | null } = { current: null };
const buildRect = (left: number, right: number): DOMRect =>
({
left,
right,
width: right - left,
top: 0,
bottom: 0,
height: 0,
x: left,
y: 0,
toJSON: () => ({}),
}) as DOMRect;
const installLayoutMock = () => {
HTMLElement.prototype.getBoundingClientRect = function mockRect(
this: HTMLElement,
) {
const dataTest = this.getAttribute?.('data-test');
if (dataTest === 'container') {
return buildRect(0, containerRight);
}
const parent = this.parentElement;
if (parent?.getAttribute?.('data-test') === 'container') {
const index = Array.prototype.indexOf.call(parent.children, this);
return buildRect(index * ITEM_W, index * ITEM_W + ITEM_W);
}
// Outer wrapper div (its first child is the inner container).
if (
(this.children[0] as HTMLElement | undefined)?.getAttribute?.(
'data-test',
) === 'container'
) {
return buildRect(0, containerRight);
}
return buildRect(0, 0);
};
};
let resizeSpy: jest.SpyInstance;
let rafSpy: jest.SpyInstance;
let cancelRafSpy: jest.SpyInstance;
// Deterministic requestAnimationFrame: the component schedules a one-shot
// confirmation frame to re-measure after an item-set change. Rather than sleep
// and hope jsdom's timer-backed rAF fires inside the window, we capture the
// callbacks and invoke them explicitly via flushRAF(). cancelAnimationFrame
// removes a queued frame, so the supersession path can be exercised directly.
let rafQueue: Array<{ id: number; cb: FrameRequestCallback }> = [];
let rafSeq = 0;
// Run every currently-queued frame once (frames scheduled during the flush are
// left for the next flush, so a single call models a single browser frame).
const flushRAF = () => {
const pending = rafQueue;
rafQueue = [];
pending.forEach(({ cb }) => cb(0));
};
beforeEach(() => {
containerRight = BAR_WIDTH;
mockWidth = 0;
fakeRef.current = null;
rafQueue = [];
rafSeq = 0;
installLayoutMock();
global.ResizeObserver = jest.fn().mockImplementation(() => ({
observe: jest.fn(),
unobserve: jest.fn(),
disconnect: jest.fn(),
})) as unknown as typeof ResizeObserver;
rafSpy = jest
.spyOn(window, 'requestAnimationFrame')
.mockImplementation((cb: FrameRequestCallback) => {
rafSeq += 1;
rafQueue.push({ id: rafSeq, cb });
return rafSeq;
});
cancelRafSpy = jest
.spyOn(window, 'cancelAnimationFrame')
.mockImplementation((id: number) => {
rafQueue = rafQueue.filter(frame => frame.id !== id);
});
resizeSpy = jest
.spyOn(resizeDetector, 'useResizeDetector')
.mockImplementation(
() =>
({ ref: fakeRef, width: mockWidth, height: 50 }) as ReturnType<
typeof resizeDetector.useResizeDetector
>,
);
});
afterEach(() => {
resizeSpy?.mockRestore();
rafSpy?.mockRestore();
cancelRafSpy?.mockRestore();
});
const makeItem = (id: string, label: string) => ({
id,
element: <div data-test={`item-${id}`}>{label}</div>,
});
const nativeFilters = (count: number) =>
Array.from({ length: count }, (_, i) =>
makeItem(`native-filter-${i + 1}`, `Filter ${i + 1}`),
);
const barItemCount = () => screen.getByTestId('container').children.length;
// Render, then apply a measured width so the overflow layout effect runs with
// the outer node attached (mirrors the first real resize-detector callback).
const renderOverflowing = async (
items: ReturnType<typeof nativeFilters>,
): Promise<{ rerender: (ui: JSX.Element) => void }> => {
const { rerender } = render(<DropdownContainer items={items} />);
await act(async () => {
mockWidth = BAR_WIDTH;
rerender(<DropdownContainer items={items} />);
});
await waitFor(() => expect(screen.getByText('More')).toBeInTheDocument());
return { rerender };
};
test('control: a clean re-measurement keeps overflowed items reachable after a chip is prepended', async () => {
const filters = nativeFilters(8);
const { rerender } = await renderOverflowing(filters);
// 3 of 8 fit in the bar, the rest are reachable via the More button.
expect(barItemCount()).toBe(3);
// Prepend a cross-filter chip, shifting every native-filter index by one.
const withCrossFilterChip = [
makeItem('cross-filter-chip', 'Region'),
...filters,
];
await act(async () => {
rerender(<DropdownContainer items={withCrossFilterChip} />);
});
await act(async () => {
flushRAF();
});
await waitFor(() => expect(barItemCount()).toBe(3));
// With faithful measurement the engine recovers to the exact split: 3 fit,
// the rest stay accessible behind the trigger.
expect(screen.queryByText('More')).toBeInTheDocument();
expect(barItemCount()).toBe(3);
});
test('overflowed-to-true-fit: when items genuinely fit after a set change, all are in the bar and the trigger is gone', async () => {
// Start from an overflowed steady state: 8 items, 3 in bar, More visible.
const filters = nativeFilters(8);
const { rerender } = await renderOverflowing(filters);
expect(barItemCount()).toBe(3);
expect(screen.queryByText('More')).toBeInTheDocument();
// Reduce to 3 items — they all fit inside the 350 px bar without overflow.
const fewFilters = nativeFilters(3);
await act(async () => {
rerender(<DropdownContainer items={fewFilters} />);
});
await act(async () => {
flushRAF();
});
// After measurement (and confirmation pass if any), the trigger is gone and
// all 3 items are in the bar. This guards the fix against over-correction:
// if the confirmation logic erroneously kept the trigger visible when items
// genuinely fit, this assertion would catch it.
await waitFor(() => {
expect(screen.queryByText('More')).not.toBeInTheDocument();
});
expect(barItemCount()).toBe(3);
});
test('prepending a cross-filter chip must not strand overflowed native filters or hide the More button', async () => {
const filters = nativeFilters(8);
const { rerender } = await renderOverflowing(filters);
expect(barItemCount()).toBe(3);
// Simulate the production race: as the cross-filter chip is added the item
// set grows, overflowingIndex is reset to -1 (all items dumped into the bar)
// and the re-measurement runs against a transient layout that momentarily
// reports everything fits. (More filters ⇒ larger reflow ⇒ wider window,
// matching the report's "depends on filter count".)
containerRight = Number.MAX_SAFE_INTEGER;
const withCrossFilterChip = [
makeItem('cross-filter-chip', 'Region'),
...filters,
];
await act(async () => {
rerender(<DropdownContainer items={withCrossFilterChip} />);
});
// The window closes — the filters genuinely overflow the bar again — but no
// resize/width change occurs, so only the scheduled confirmation frame can
// rescue the verdict. Fire it.
containerRight = BAR_WIDTH;
await act(async () => {
flushRAF();
});
// Invariant: overflowed items must remain accessible AND the split must be
// CORRECT. Asserting the exact count (3 fit, the rest behind the trigger),
// not merely `< total`, so an under-detecting confirmation that strands too
// many items in the clipped bar also fails this guard.
expect(barItemCount()).toBe(3);
expect(screen.queryByText('More')).toBeInTheDocument();
});
test('fit-to-overflow: an item-set change that tips a fitting bar into overflow during a transient must not strand items', async () => {
// Start with a bar that FITS: 3 items, no overflow, no trigger. The overflow
// engine settles overflowingIndex === -1 here.
const fewFilters = nativeFilters(3);
const { rerender } = render(<DropdownContainer items={fewFilters} />);
await act(async () => {
mockWidth = BAR_WIDTH;
rerender(<DropdownContainer items={fewFilters} />);
});
await waitFor(() => expect(barItemCount()).toBe(3));
expect(screen.queryByText('More')).not.toBeInTheDocument();
// Grow the set so it now genuinely overflows, but measure it during a
// transient window where the bar momentarily appears to still fit. Because
// the bar was previously fitting, this takes the "measure" path, not the
// reset path — the case the original fix armed NO confirmation for, so a
// transient "-1" would latch (all items crammed, trigger gone) with no
// rescue. The hardened engine arms a confirmation on every item-set change.
containerRight = Number.MAX_SAFE_INTEGER;
const manyFilters = nativeFilters(8);
await act(async () => {
rerender(<DropdownContainer items={manyFilters} />);
});
// Window closes; the scheduled confirmation frame re-measures and corrects.
containerRight = BAR_WIDTH;
await act(async () => {
flushRAF();
});
expect(barItemCount()).toBe(3);
expect(screen.queryByText('More')).toBeInTheDocument();
});
test('a second item-set change before the confirmation frame fires still settles the correct split (re-entrancy regression)', async () => {
// Regression guard for rapid successive changes: prepend two chips in quick
// succession (each during a transient), then let the frame(s) fire. The
// hardened engine supersedes the stale frame and arms a fresh confirmation
// for the latest set; this locks in the correct end state under re-entrancy.
const filters = nativeFilters(8);
const { rerender } = await renderOverflowing(filters);
expect(barItemCount()).toBe(3);
containerRight = Number.MAX_SAFE_INTEGER;
const withOneChip = [makeItem('cross-filter-chip', 'Region'), ...filters];
await act(async () => {
rerender(<DropdownContainer items={withOneChip} />);
});
const withTwoChips = [
makeItem('cross-filter-chip-2', 'Segment'),
...withOneChip,
];
await act(async () => {
rerender(<DropdownContainer items={withTwoChips} />);
});
containerRight = BAR_WIDTH;
await act(async () => {
flushRAF();
});
expect(barItemCount()).toBe(3);
expect(screen.queryByText('More')).toBeInTheDocument();
});

View File

@@ -81,6 +81,53 @@ export const DropdownContainer = forwardRef(
// when nothing actually overflows.
const [recalculating, setRecalculating] = useState(false);
// One-shot confirmation pass: when the layout effect settles on "nothing
// overflows" right after an item-set-change reset, the geometry may still
// be mid-reflow. These refs coordinate a single rAF follow-up measurement
// per item-set change so a transiently-bad "fits" verdict cannot latch.
//
// pendingConfirmForLengthRef: holds the items.length for which a
// confirmation is pending (-1 = none pending). Set in the reset (else)
// branch; cleared by the rAF callback after it settles.
const pendingConfirmForLengthRef = useRef(-1);
// confirmationScheduledRef: true once the rAF has been requested for the
// current pending length, preventing a second rAF on the setItemsWidth
// re-run that follows the first provisional measurement.
const confirmationScheduledRef = useRef(false);
// hadContentAtLastChangeRef: true when the trigger was showing at the
// moment the most recent item-set change was detected. Keeps the trigger
// mounted across the entire confirmation window (not just one render cycle)
// without letting it linger once the rAF callback has settled. Cleared by
// the rAF callback before calling setRecalculating(false).
const hadContentAtLastChangeRef = useRef(false);
// Guards rAF callbacks from firing after the component unmounts.
const mountedRef = useRef(true);
// Stores the pending confirmation rAF handle so it can be cancelled when a
// newer item-set change supersedes it, or on unmount.
const rafIdRef = useRef(0);
// Bumped on every item-set change. A scheduled rAF captures the version at
// schedule time and ignores itself if a newer change has superseded it, so
// a stale frame can never clobber a newer item set's state.
const confirmVersionRef = useRef(0);
// The items.length the layout effect last observed, used to detect a new
// item set (additions/removals) on any measurement path, not just the reset.
const prevItemsLengthRef = useRef(items.length);
useEffect(
() => () => {
mountedRef.current = false;
if (rafIdRef.current) {
cancelAnimationFrame(rafIdRef.current);
rafIdRef.current = 0;
}
},
[],
);
// Persists the inner container element for the rAF confirmation callback.
// Updated each time the layout effect finds a valid container so the rAF
// does not need to re-derive it through ref.current, which may be null by
// the time the callback fires in certain timing / test scenarios.
const containerRef = useRef<Element | null>(null);
// callback to update item widths so that the useLayoutEffect runs whenever
// width of any of the child changes
const recalculateItemWidths = useCallback(() => {
@@ -163,14 +210,66 @@ export const DropdownContainer = forwardRef(
};
}, [items.length, current, recalculateItemWidths]);
const overflowingCount =
overflowingIndex !== -1 ? items.length - overflowingIndex : 0;
const popoverContent = useMemo(
() =>
dropdownContent || overflowingCount ? (
<div
css={css`
display: flex;
flex-direction: column;
gap: ${theme.sizeUnit * 4}px;
`}
data-test="dropdown-content"
style={dropdownStyle}
ref={targetRef}
>
{dropdownContent
? dropdownContent(overflowedItems)
: overflowedItems.map(item => item.element)}
</div>
) : null,
[
dropdownContent,
overflowingCount,
theme.sizeUnit,
dropdownStyle,
overflowedItems,
],
);
useLayoutEffect(() => {
if (popoverVisible) {
return;
}
const container = current?.children.item(0);
if (container) {
containerRef.current = container;
const { children } = container;
const childrenArray = Array.from(children);
// Detect a new item set (additions/removals shift the positional
// measurements the overflow split relies on). Arm a confirmation pass
// for it here so EVERY measurement path below — not just the reset
// branch — gets a follow-up; otherwise a fit->overflow transition (the
// bar was fitting, so the reset branch is skipped) could settle a
// transient "fits" verdict with no rescue. Also supersede any
// confirmation still pending for the previous item set: bump the version
// (so its stale rAF ignores itself) and cancel its frame.
if (prevItemsLengthRef.current !== items.length) {
prevItemsLengthRef.current = items.length;
pendingConfirmForLengthRef.current = items.length;
confirmationScheduledRef.current = false;
hadContentAtLastChangeRef.current = !!popoverContent;
confirmVersionRef.current += 1;
if (rafIdRef.current) {
cancelAnimationFrame(rafIdRef.current);
rafIdRef.current = 0;
}
}
// If items length change, add all items to the container
// and recalculate the widths
if (itemsWidth.length !== items.length) {
@@ -211,6 +310,12 @@ export const DropdownContainer = forwardRef(
// Checks if some elements in the dropdown fits in the remaining space
let sum = 0;
for (let i = childrenArray.length; i < items.length; i += 1) {
// Guard: itemsWidth may be stale when its length doesn't match the
// current item set (its updater bails on a length mismatch). An
// undefined entry would otherwise inject NaN into the sum.
if (itemsWidth[i] === undefined) {
break;
}
sum += itemsWidth[i];
if (sum <= remainingSpace) {
newOverflowingIndex = i + 1;
@@ -220,6 +325,59 @@ export const DropdownContainer = forwardRef(
}
}
// A "nothing overflows" verdict on the pass that consumed an item-set-
// change reset may reflect a transient mid-reflow measurement. When that
// happens, do NOT settle immediately. Instead:
// • If the rAF hasn't been scheduled yet: schedule it (one-shot) and
// return without settling; recalculating stays true so the trigger
// remains mounted throughout the confirmation window.
// • If the rAF is already scheduled (a second layout effect run
// triggered by the setItemsWidth call above): also return without
// settling for the same reason.
// The rAF callback reads the DOM directly at a point where the browser
// has reflowed and calls the setters itself. It also resets the guard
// refs so subsequent effect runs (e.g. from a real resize) behave
// normally.
if (
newOverflowingIndex === -1 &&
pendingConfirmForLengthRef.current === items.length
) {
if (!confirmationScheduledRef.current) {
confirmationScheduledRef.current = true;
const scheduledVersion = confirmVersionRef.current;
rafIdRef.current = requestAnimationFrame(() => {
rafIdRef.current = 0;
if (!mountedRef.current) return;
// A newer item-set change superseded this confirmation while the
// frame was queued; let the newer one's own confirmation settle.
if (confirmVersionRef.current !== scheduledVersion) return;
// Reset guard refs so future layout effect runs are unaffected.
pendingConfirmForLengthRef.current = -1;
confirmationScheduledRef.current = false;
hadContentAtLastChangeRef.current = false;
const el = containerRef.current;
if (!el) {
setOverflowingIndex(-1);
setRecalculating(false);
return;
}
const kids = Array.from(el.children);
const confirmIdx = kids.findIndex(
c =>
c.getBoundingClientRect().right >
el.getBoundingClientRect().right + 1,
);
setOverflowingIndex(confirmIdx);
setRecalculating(false);
});
}
// Either way (just scheduled or already pending): hold off settling so
// recalculating stays true and the button guard keeps the trigger mounted.
return;
}
pendingConfirmForLengthRef.current = -1;
confirmationScheduledRef.current = false;
setOverflowingIndex(newOverflowingIndex);
setRecalculating(false);
}
@@ -242,44 +400,14 @@ export const DropdownContainer = forwardRef(
}
}, [notOverflowedIds, onOverflowingStateChange, overflowedIds]);
const overflowingCount =
overflowingIndex !== -1 ? items.length - overflowingIndex : 0;
const popoverContent = useMemo(
() =>
dropdownContent || overflowingCount ? (
<div
css={css`
display: flex;
flex-direction: column;
gap: ${theme.sizeUnit * 4}px;
`}
data-test="dropdown-content"
style={dropdownStyle}
ref={targetRef}
>
{dropdownContent
? dropdownContent(overflowedItems)
: overflowedItems.map(item => item.element)}
</div>
) : null,
[
dropdownContent,
overflowingCount,
theme.sizeUnit,
dropdownStyle,
overflowedItems,
],
);
// The trigger had content in the previous render if popoverContent was
// truthy then. During the brief mid-recalculation render where
// popoverContent flips to null, this still reflects the prior (non-empty)
// value, letting us keep the trigger mounted across the transient.
const hadPopoverContent = usePrevious(!!popoverContent, false);
// During the rAF confirmation window recalculating stays true (the layout
// effect returns early without settling). hadContentAtLastChangeRef tracks
// whether the trigger was showing when the item-set change was detected; it
// stays true across all renders until the rAF callback clears it. Together
// they keep the trigger mounted for the full confirmation window without
// letting it linger once the rAF has settled.
const showDropdownButton =
!!popoverContent || (recalculating && hadPopoverContent);
!!popoverContent || (recalculating && hadContentAtLastChangeRef.current);
useLayoutEffect(() => {
if (popoverVisible) {