fix: follow-ups of the lot-v018 stack reviews #482

Merged
maximus merged 25 commits from review-followups-lot-v018 into main 2026-10-21 00:31:46 +00:00
5 changed files with 161 additions and 10 deletions
Showing only changes of commit 5f2c4dd5bd - Show all commits

View file

@ -146,9 +146,10 @@ export default function AddKeywordDialog({
<h2 className="text-base font-semibold">{t("reports.keyword.dialogTitle")}</h2>
</header>
{/* The two fields sit outside the scrolling block, so the open category
list is drawn over the preview below instead of being cut by its
scrollport. */}
{/* The two fields sit outside the scrolling block, so they stay in view
while the preview scrolls. The open category list does not depend
on it any more: since #429 it lives in a portal on document.body,
where no scrollport cuts it. */}
<div className="px-5 pt-4 flex flex-col gap-4">
<label className="flex flex-col gap-1">
<span className="text-xs font-medium text-[var(--muted-foreground)] uppercase tracking-wide">

View file

@ -0,0 +1,105 @@
// CategoryCombobox — the open list follows its input (#429, review 25431).
//
// React's hooks come from the shared harness (src/test/hookHarness.ts, #409),
// which refuses `useLayoutEffect` and `useId`: this file alone maps
// `useLayoutEffect` onto the harness's `useEffect` (same dependency rule, run
// by `commit()` after the render, where React runs a layout effect before the
// paint) and `useId` onto a constant. No DOM: the input is a fake whose
// `getBoundingClientRect` the test moves, `window` and `document` are stubs,
// and `createPortal` is the identity. What stays out of reach: the paint, and
// React skipping the render on an unchanged state (here, the same object in
// the state slot stands for it).
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { isValidElement, type ReactElement, type ReactNode } from "react";
vi.mock("react", async (orig) => {
const fake = (await import("../../test/hookHarness")).fakeReact(await orig());
return { ...fake, useLayoutEffect: fake.useEffect, useId: () => "cb" };
});
vi.mock("react-dom", async (orig) => ({
...(await orig<typeof import("react-dom")>()),
createPortal: (children: ReactNode) => children,
}));
vi.mock("react-i18next", async () => (await import("../../test/i18nMock")).i18nMock());
import { hookHarness } from "../../test/hookHarness";
import CategoryCombobox from "./CategoryCombobox";
const harness = hookHarness();
type Props = Record<string, unknown> & { children?: ReactNode };
function find(node: ReactNode, type: unknown): ReactElement<Props>[] {
if (Array.isArray(node)) return node.flatMap((n) => find(n, type));
if (!isValidElement(node)) return [];
const el = node as ReactElement<Props>;
return [...(el.type === type ? [el] : []), ...find(el.props.children, type)];
}
const CATEGORIES = [{ id: 7, name: "Épicerie", parent_id: null, sort_order: 1 }];
/** The input's place on screen, which a render of the parent can change. */
const rect = { top: 100, bottom: 130, left: 50, width: 200 };
const fakeInput = { getBoundingClientRect: () => ({ ...rect }), blur: vi.fn() };
/** One render of the mounted combobox. */
const render = () =>
harness.render(() => CategoryCombobox({ categories: CATEGORIES, value: null, onChange: vi.fn() }) as ReactNode);
/** The open list's style, or null when no list is rendered. */
const listStyle = (tree: ReactNode) => {
const [list] = find(tree, "ul");
return list ? (list.props.style as { top?: number; bottom?: number }) : null;
};
/** Mounts the combobox, hands its input ref the fake input, then focuses it: the list opens. */
function open(): ReactNode {
const closed = render();
const [input] = find(closed, "input");
(input.props.ref as { current: unknown }).current = fakeInput;
harness.commit();
(input.props.onFocus as () => void)();
render();
harness.commit(); // places the list
return render();
}
beforeEach(() => {
harness.clear();
Object.assign(rect, { top: 100, bottom: 130 });
vi.stubGlobal("window", { innerHeight: 800, addEventListener: vi.fn(), removeEventListener: vi.fn() });
vi.stubGlobal("document", { body: {}, addEventListener: vi.fn(), removeEventListener: vi.fn() });
});
afterEach(() => {
harness.unmount();
vi.unstubAllGlobals();
});
describe("CategoryCombobox — the open list follows its input (#429)", () => {
it("places the list under its input once open", () => {
expect(listStyle(render())).toBeNull();
harness.clear();
expect(listStyle(open())).toMatchObject({ top: 134 });
});
it("follows the input moved by a render of the parent, with no scroll nor resize", () => {
open();
// A window centered on screen grows when its content arrives: the input moves down.
Object.assign(rect, { top: 300, bottom: 330 });
render();
harness.commit();
expect(listStyle(render())).toMatchObject({ top: 334 });
});
it("keeps the same placement object when the input did not move, so React renders nothing more", () => {
open();
const placement = listStyle(render());
const before = harness.slots.find((slot) => (slot as { top?: number } | null)?.top === 134);
expect(before).toBeDefined();
render();
harness.commit();
expect(listStyle(render())).toEqual(placement);
expect(harness.slots).toContain(before);
});
});

View file

@ -4,8 +4,8 @@ import { useTranslation } from "react-i18next";
import {
LISTBOX_MAX_HEIGHT,
followAnchor,
keepPlacement,
placeListbox,
samePlacement,
type ListboxPlacement,
} from "./placeListbox";
/**
@ -207,7 +207,7 @@ export default function CategoryCombobox<T extends CategoryLike = CategoryLike>(
anchor: input.getBoundingClientRect(),
viewportHeight: window.innerHeight,
});
setPlacement((prev) => (samePlacement(prev, next) ? prev : next));
setPlacement((prev) => keepPlacement(prev, next));
}, []);
useLayoutEffect(() => {
@ -216,6 +216,15 @@ export default function CategoryCombobox<T extends CategoryLike = CategoryLike>(
return followAnchor(window, updatePlacement, () => listRef.current);
}, [open, updatePlacement]);
// A render can move the input with no scroll nor resize: a window centered
// on screen grows when its content arrives, and its fields move with it.
// While the list is open, its position is read again after every render;
// an unchanged one keeps the same state object, so React renders nothing
// more (`keepPlacement`).
useLayoutEffect(() => {
if (open) updatePlacement();
});
useEffect(() => {
if (!open) return;
const handler = (e: MouseEvent) => {

View file

@ -1,11 +1,17 @@
// placeListbox — where the open list of a category combobox goes (#429).
// The portal itself needs a DOM (manual check); its placement rule and the
// listeners that keep it on its input do not, and are pinned here.
// The portal itself needs a DOM (manual check); its placement rule, the
// listeners that keep it on its input and the state update that keeps an
// unchanged position do not, and are pinned here. What needs a DOM: that the
// combobox reads the position again after a render that moved its input
// (`useLayoutEffect` without dependencies, `getBoundingClientRect`), and that
// this ends, React skipping the render when `keepPlacement` hands back the
// same object; the harness refuses `useLayoutEffect` and `useId`.
import { describe, it, expect, vi } from "vitest";
import {
LISTBOX_GAP,
LISTBOX_MAX_HEIGHT,
followAnchor,
keepPlacement,
placeListbox,
samePlacement,
type ListenerTarget,
@ -31,10 +37,11 @@ describe("placeListbox", () => {
});
it("stays below when the room below is exactly the list's maximum height", () => {
// 130 (input's bottom) + 4 + 240 = 374.
const placement = placeListbox({ anchor: anchorAt(100), viewportHeight: 374 });
// Room below: 574 - 330 - 4 = 240, the list's height; room above: 300 - 4 =
// 296, enough too. Below wins, its usual place.
const placement = placeListbox({ anchor: anchorAt(300), viewportHeight: 574 });
expect(placement.openUpward).toBe(false);
expect(placement.top).toBe(134);
expect(placement.top).toBe(334);
});
it("opens upward when the room below is short and the room above suffices", () => {
@ -88,6 +95,22 @@ describe("samePlacement", () => {
});
});
describe("keepPlacement", () => {
const below: ListboxPlacement = { left: 50, width: 200, openUpward: false, top: 134 };
it("hands back the previous object itself when the position did not change", () => {
// React skips a render on the same object: the reading after each render ends.
const previous = { ...below };
expect(keepPlacement(previous, { ...below })).toBe(previous);
});
it("takes the new position when the input moved, or on the first reading", () => {
const moved = { ...below, top: 334 };
expect(keepPlacement(below, moved)).toBe(moved);
expect(keepPlacement(null, below)).toBe(below);
});
});
/**
* A fake `window` with the DOM's rule: a listener is removed only by the same
* type, the same function and the same capture flag.

View file

@ -72,6 +72,19 @@ export function samePlacement(a: ListboxPlacement | null, b: ListboxPlacement):
);
}
/**
* The state update of a new reading: the previous object itself when the
* position did not change. React then skips the render, which is what lets
* `CategoryCombobox` read the position again after every render of an open
* list without looping.
*/
export function keepPlacement(
previous: ListboxPlacement | null,
next: ListboxPlacement,
): ListboxPlacement {
return samePlacement(previous, next) ? (previous as ListboxPlacement) : next;
}
/** The part of `window` the listeners use, so a test can pass a fake. */
export interface ListenerTarget {
addEventListener(type: string, listener: (event: Event) => void, capture?: boolean): void;