From 5f2c4dd5bda5c25c5732180ac0ec19e5d374e882 Mon Sep 17 00:00:00 2001
From: le king fu <lekingfu@protonmail.com>
Date: Mon, 19 Oct 2026 21:06:22 -0400
Subject: [PATCH] fix(combobox): follow the input when a render moves it (#429)

Review 25431: a window centered on screen grows when its content
arrives, and moves its fields with no scroll nor resize; the open list,
placed in a portal, stayed where it was. While open, the combobox now
reads its position again after every render (useLayoutEffect without
dependencies); `keepPlacement` hands back the same state object for an
unchanged position, so React renders nothing more. Pinned by
CategoryCombobox.placement.test.ts (harness, useLayoutEffect mapped on
its useEffect, fake input) and keepPlacement's own tests. The boundary
test of placeListbox now sits at exactly 240 px below with room above
(point 1), and AddKeywordDialog's comment no longer claims the list is
cut (point 3).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 .../categories/AddKeywordDialog.tsx           |   7 +-
 .../shared/CategoryCombobox.placement.test.ts | 105 ++++++++++++++++++
 src/components/shared/CategoryCombobox.tsx    |  13 ++-
 src/components/shared/placeListbox.test.ts    |  33 +++++-
 src/components/shared/placeListbox.ts         |  13 +++
 5 files changed, 161 insertions(+), 10 deletions(-)
 create mode 100644 src/components/shared/CategoryCombobox.placement.test.ts

diff --git a/src/components/categories/AddKeywordDialog.tsx b/src/components/categories/AddKeywordDialog.tsx
index 69c12ee..a67ef6b 100644
--- a/src/components/categories/AddKeywordDialog.tsx
+++ b/src/components/categories/AddKeywordDialog.tsx
@@ -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">
diff --git a/src/components/shared/CategoryCombobox.placement.test.ts b/src/components/shared/CategoryCombobox.placement.test.ts
new file mode 100644
index 0000000..ae0c5bb
--- /dev/null
+++ b/src/components/shared/CategoryCombobox.placement.test.ts
@@ -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);
+  });
+});
diff --git a/src/components/shared/CategoryCombobox.tsx b/src/components/shared/CategoryCombobox.tsx
index 4a2c869..297f762 100644
--- a/src/components/shared/CategoryCombobox.tsx
+++ b/src/components/shared/CategoryCombobox.tsx
@@ -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) => {
diff --git a/src/components/shared/placeListbox.test.ts b/src/components/shared/placeListbox.test.ts
index ccc9950..dc0127d 100644
--- a/src/components/shared/placeListbox.test.ts
+++ b/src/components/shared/placeListbox.test.ts
@@ -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.
diff --git a/src/components/shared/placeListbox.ts b/src/components/shared/placeListbox.ts
index a3f4ba8..41e7b67 100644
--- a/src/components/shared/placeListbox.ts
+++ b/src/components/shared/placeListbox.ts
@@ -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;