Skip to content

Use Floating UI for stable comment placement - #537

Merged
max-sixty merged 9 commits into
mainfrom
codex/stable-floating-comments
Sep 10, 2026
Merged

Use Floating UI for stable comment placement#537
max-sixty merged 9 commits into
mainfrom
codex/stable-floating-comments

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

Replaces Leaf's custom response-placement collision walker with a vendored Floating UI lifecycle. A comment chooses its side from the compact control and retains it while content grows; Floating UI observes external geometry, shifts and sizes within the reading region, and lets the field scroll when vertical room is exhausted. Page and margin content may be overlaid, so nearby margin actions no longer push a growing draft above its passage or collapse it to one line.

The dependency is lazy-loaded only when a response opens and ships with pinned license notices. Focus waits for asynchronous placement, placement failures withdraw the surface cleanly, and genuine viewport or anchor changes can still choose a better side.

Browser regressions cover the reported release-notes geometry, upward growth and scrolling near the viewport edge, margin-item independence, horizontal and vertical target motion, missing and delayed dependency loads, draft restoration, response expansion, and target transfer.

This was written by Codex on behalf of max-sixty

@leaf-agent leaf-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things to settle before this lands. The rest of the change reads well — the lazy-load, held-module, and aborted-module startup cases all pass here, as do the reactions contracts.

The two new rechooses regressions anchor to the wrong element, and fail. On Playwright's pinned Chromium in a Linux container, test_a_comment_rechooses_after_target_width_reflow and test_an_above_comment_rechooses_after_vertical_target_motion both come up right-start, and the bar reports aria-label="Respond to old · release's only visual change." — so the anchored reference is <lf-old id="rn-console-old">, not #rn-console-why. Which element the centre press lands on turns on where that paragraph's text wraps, so it is a font-metric coin flip rather than a stated arrangement; mechanism and repair inline. nightly skips on the pull-request gate, so a green test here does not cover either case — the daily run is where they land.

@floating-ui/core and @floating-ui/utils belong in HELD_BY. They are @floating-ui/dom's own declared dependencies — npm view @floating-ui/dom@1.8.0 dependencies answers {"@floating-ui/core": "^1.8.0", "@floating-ui/utils": "^0.2.12"} — rather than Leaf's choice of version, which is exactly the case the comment above HELD_BY states for elkjs and entities. Without entries, scripts/vendor.py --pins reads both rows against latest, so the weekly vendored-dependency sweep will report a release the bundle cannot actually take: npm would install the version @floating-ui/dom declares nested under it, esbuild would bundle that one, and the table would say one thing while the bundle carried another. HELD_BY sits outside this diff, so it is not an inline suggestion — happy to push it if you want:

HELD_BY = {
    "elkjs": "beautiful-mermaid",
    "entities": "beautiful-mermaid",
    "@floating-ui/core": "@floating-ui/dom",
    "@floating-ui/utils": "@floating-ui/dom",
}
Run, and the geometry behind the two failures

uv run pytest tests/test_render_aim.py tests/test_render_reactions.py tests/test_render_startup.py --run-nightly2 failed, 193 passed. Both failures are the new tests named above; everything else in those three files passes.

At 700×600 on release-notes, #rn-console-why measures left 24, right 676 and its text fits one line — 604px of text in 652px of column, within about 8% of wrapping. The Alt-press at the element's centre (350, 317) therefore lands inside <lf-old id="rn-console-old">, which measures left 193.5, right 392.3 and leaves a 294px right rail, wider than the 256px compact minimum. right-start is the correct placement for that reference.

Driving computePosition by hand in the page with the same boundary and keepClear set to the paragraph's own box returns top-end, with middlewareData.flip recording right-start and left-start overflowing by 283.7px each — so the middleware composition does what the tests name. It is the reference that is wrong, not the placement.

The closing assertion in each test measures against #rn-console-why (after["x"] >= target_after["x"] + target_after["width"] + 5), which confirms the paragraph is the intended anchor.

Comment thread tests/test_render_aim.py
Comment thread tests/test_render_aim.py
@max-sixty
max-sixty merged commit 992bad9 into main Sep 10, 2026
7 checks passed
@max-sixty
max-sixty deleted the codex/stable-floating-comments branch September 10, 2026 06:20
max-sixty pushed a commit that referenced this pull request Sep 10, 2026
…ule (#543)

`scripts/CLAUDE.md` tells a session that a clean `git status` after
`scripts/vendor.py <bundle>` is the check that a bundle still matches
the script, and that the check holds only where every fetched input is
pinned. It then sorts the bundles into those two classes — and two of
the seven are in neither, so a session rebuilding `floating-ui` or
`mcp-app` has no answer for whether a diff it sees is drift to fix or an
upstream patch to take.

Each belongs in a class the paragraph already describes:

- `floating-ui` reproduces. `@floating-ui/dom`, `@floating-ui/core`, and
`@floating-ui/utils` are all in `PINS`, and that closure is complete:
`dom` depends on `core` and `utils`, `core` on `utils`, `utils` on
nothing. Rebuilding it left `git status` clean.
- `mcp-app` fetches an input `PINS` does not name.
`@modelcontextprotocol/ext-apps@1.7.5` declares `@standard-schema/spec
^1.1.0`, which npm's resolver picks, so the bundle sits with `plot` and
`pierre`. It reproduced when I rebuilt it today — which is exactly the
reading the paragraph exists to keep a session from relying on.

This is drift rather than a deliberate omission. The paragraph dates
from `c7b37062` (2026-08-30); `mcp-app` landed in #188 on 2026-09-01 and
`floating-ui` in #537 last night, and #533 — the most recent commit to
touch the paragraph — added `jsdiff` to it, so it is meant to be kept
current.

The test follows the shape this repo already uses for instruction lists
(#93, #105, #331, #496): it reads the bundle names out of `vendor.py`'s
`BUILDS` and `COPIES` rather than restating them, and asserts each is
named in that one paragraph. On `main` it fails with `unplaced in
scripts/CLAUDE.md: ['floating-ui', 'mcp-app']`.

What it deliberately does not check is *which* class a bundle is placed
in — deciding that means resolving each package's dependency graph,
which is a network call this suite should not make. A name in the wrong
class still needs a reader to catch; a name that is absent no longer
does.

<details><summary>Verification</summary>

- `uv run pytest tests/test_interact_layer.py` — 167 passed, 6 skipped.
- The new case alone, with `scripts/CLAUDE.md` reverted to `main`'s
text: `AssertionError: unplaced in scripts/CLAUDE.md: ['floating-ui',
'mcp-app']`.
- `scripts/vendor.py floating-ui` and `scripts/vendor.py mcp-app` each
left `git status` clean.
- `npm view @floating-ui/dom@1.8.0 dependencies` → `{
'@floating-ui/core': '^1.8.0', '@floating-ui/utils': '^0.2.12' }`;
`@floating-ui/core@1.8.0` → `{ '@floating-ui/utils': '^0.2.12' }`;
`@floating-ui/utils@0.2.12` → none;
`@modelcontextprotocol/ext-apps@1.7.5` → `{ '@standard-schema/spec':
'^1.1.0' }`.

</details>

Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants