Skip to content

fix(web): preserve Escape focus over closing menus - #578

Merged
lukemaj merged 3 commits into
mainfrom
fix/576-escape-hover-focus
Oct 20, 2026
Merged

lukemaj merged 3 commits into
mainfrom
fix/576-escape-hover-focus

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What: The Escape focus fix merged into main as c3557ef after exact-head review, CI and actual before/after Browser proof.
Why: A late hover-exit event on a closing popup suppressed return focus to its trigger.
So what: This outcome is closed and its isolated proof resources are retired; no deployment or installed-service restart was performed.

Closes #576. Canonical handoff and exact proof bindings. The existing BaseUI native onFloatingMouseLeave owner now checks its authoritative open state before publishing another close. This preserves the original focus manager's policies rather than forcing focus from a caller. The pnpm patch covers ESM and CommonJS, binds exactly 1.5.0 in the workspace and lockfile, and changes no dependency version.

R2 F1 from the whole review is addressed in 19ecbbfc20306fdd2f69432968fe83f4aaa91df4 (tree bf300c3aa9f5f1992c9304f2ba82c561e86bfce9), preserving ce1f as its parent. The only correction is the upstream edit allowlist plus a narrow menu-escape-focus registry entry owning the manifest, patch and regression test, watching shared lock/workspace refs, and specifying equivalent upstream fix plus dependency-upgrade/unpatched-regression proof before patch retirement. Product, patch, test, manifest and lock bytes are unchanged.

Fresh actual fork-features.mjs check, exact-head fork-check.sh --upstream upstream, scoped format and diff checks pass. Existing upstream/main is e704190e8ce0d15f85e42bc844baad525b2110c1, merge base d0918f1659f4c19172796c8d1910ca6e4bc18b51. Prior behavioral, dependency and typecheck evidence is reused only for byte-identical implementation/runtime inputs, verified by hashes. No prior review approval transfers. Parent owns fresh whole review with route-MEDIUM verification, current CI and integrated images/video. O1 submenu-chain and O3 shared hover-consumer effects remain explicit source-inference limits for that real-client pass.

R1 from the independent review is addressed in ce1f908f8fa588c4f8c90b8753e16bda2d7f957f (tree 605f2be48cbe6e97859bcba591a682082cef4e70), preserving bc03532f97 as its parent. Release Smoke deletes the lockfile before resolution, so the original ^1.4.1 range could bypass the exact patch and fail with ERR_PNPM_UNUSED_PATCH. The correction changes only the manifest specifier to 1.5.0 and the matching lock importer, with no version upgrade or patch/source/test edit.

Fresh frozen offline lock validation, package formatting, web typecheck and exact two-line/runtime-hash invariants pass. An isolated lockless BaseUI/React/patch seam regenerates the correct 1.5.0 patch binding. Full staged offline resolution stopped on uncached tinyps metadata through marketing's Astro dependency; complete Release Smoke remains CI-owned. All probes used explicit --ignore-scripts; the earlier patch-commit prepare incident remains in the handoff. Prior 15/15 behavioral red/green and real/fake pointer-over evidence is reused only for unchanged source, test and installed runtime, not presented as a fresh whole-head run or review verdict. Parent owns fresh whole-candidate review, current CI and integrated visual proof.

A public Menu fixture opens from an input shortcut, places the real pointer over the popup after opening, closes with Escape, and asserts trigger focus after public close completion. It also covers pointer-away, intentional focus elsewhere during closing, click-open/outside-click, genuine hover dismissal, and keyboard entry plus pointer movement into a focused submenu. Native CSS transition completion drives the oracle; no sleeps, polling, added timeouts, retry loops or assertion weakening. The existing RightPanelTabs keyboard file is untouched, including #575's separate ownership.

Proof on pinned Node 24.13.1/pnpm 11.10.0, BaseUI 1.5.0, React/DOM 19.2.6 and existing task-private Chromium revision1223:

  • Before production edits, the public fixture failed pointer-over Escape and focused-submenu Escape, with four controls passing. Final-fixture replay against the retained unpatched 1.5.0 ESM owner reproduces the same two failures.
  • Patched final fixture plus unchanged RightPanelTabs.keyboard.browser.test.tsx: 15/15 passed. Command in apps/web: vp test run src/components/ui/menu.focus.browser.test.tsx src/components/RightPanelTabs.keyboard.browser.test.tsx --project browser --reporter verbose.
  • An instrumented, unchanged RightPanelTabs custom-shortcut case restores trigger focus with a real pointer over the popup on real and fake clocks; the redundant trigger-hover mouseleave notification disappears.
  • Targeted vp fmt --check and vp lint; web tsc-rs --noEmit; frozen offline pnpm install --ignore-scripts: passed. Typecheck retains one unrelated pre-existing Effect suggestion, with no errors.

Upstream checked independently: T3 main d36b7da2485bc889b320a285deea181f637d760a, BaseUI master bf66e3ad88453712acdad9b56f57463e9a2927f4, relevant open/closed Issues and PRs, and complete candidate diffs. BaseUI #3482 previously used this authoritative-open guard in the former hover owner. Merged #4893 and #4906 instead restrict hover-origin behavior and add regression coverage. This patch keeps the installed version's open-menu behavior, with a narrower closed-state guard. T3 #21901 is test isolation, separately owned by #575; #21030/#21118 wait changes are already present.

Limits: component/browser proof only, not product acceptance, CI clearance or independent review. Parent owns the later real-client integration pass, before/after images and interaction video, independent different-family review, CI disposition, and merge gate. No full web/repository suite, Browser panel, Device, product server, live userdata, provider, account, deployment or restart was used. React act warnings remain visible in raw runs; the fixture awaits native completion outside act to allow its opening state transition to commit. The exact prior CI pointer sequence remains unknown.

Scope: the shared BaseUI owner applies to web and the desktop renderer, including shortcut and click callers. Mobile's React Native menus, providers, MCP, contracts and transports are unchanged. The current map asserts the T3 client base as live; no tracked part or architecture is added or changed. No version bump. Website/SEO impact: none, an existing focus contract is repaired without changed routes, content or visuals.

Requirements and who asked: User and #576 require existing-menu Escape focus return with actual pointer-over, preserving hover dismissal, submenus, outside focus and separate merge approval.
Deleted: Caller focus override, duplicate wrapper state, dependency upgrade, unrelated hover-origin policy change, pointer parking as repair, longer waits and #379 scope expansion.
Bottleneck: A closed popup publishes late hover-close and overwrites the focus owner's close policy.
Checked myself: Accepted R2 F1, existing registry schema/owner and exact-head fork checks; accepted R1 against release-smoke deletion/resolution, exact version/specifier repair and isolated lockless seam; frozen main/source identities, actual installed ESM/CommonJS owners and versions, public-component red/green plus policy controls, RightPanelTabs real/fake-clock probes, scoped checks and full relevant upstream diffs.

Implemented with GPT-6.1 Sol, MEDIUM effort, through the Codex harness in T3 Code.

Final merge and cleanup record: #578 (comment)

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Oct 20, 2026
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: f3a399d · PR result: 19ecbbf · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What The #578 review requests a dependency-specifier repair.
Why Release Smoke regenerates the lock and leaves the exact Base UI patch unused.
So what The author will repair the pin and return a new candidate for fresh review.

PR #578 independent review (Issue #576)

Verdict: REQUEST_CHANGES (one required fix; product mechanism itself is sound).

Binding

  • Reviewer: Claude Opus 5.5 (claude-opus-5-5), Claude Code harness inside T3 Code. Requested effort MEDIUM; session ran at a low reasoning-effort setting, so treat depth as medium-or-below. Author: GPT-6.1 Sol (Codex), different family.
  • Base f3a399d40c252730b02b7022b627763f7add91b0, head bc03532f97e7e3a7f7cb2a0a69e38e7f5f6a8584, tree 0d4dbbd135234229ccd790b7b297252fed93f7a1, worktree clean. git diff base..HEAD | sha256sum = 40574059b04c596cab52c7a98ac7fd9911b98506c0ff96ffe3a58a8fc40069ad (matches brief). PR OPEN, MERGEABLE, base main, headRefOid = head.
  • Paths: apps/web/src/components/ui/menu.focus.browser.test.tsx (+223), patches/@base-ui__react@1.5.0.patch (+30), pnpm-lock.yaml (+3/-2), pnpm-workspace.yaml (+1).

Required finding

R1. Release Smoke CI fails because the patch key is exact while the dependency specifier is a range.

  • Location: pnpm-workspace.yaml:339 ("@base-ui/react@1.5.0": patches/...) against apps/web/package.json:15 ("@base-ui/react": "^1.4.1", unchanged).
  • Producer: scripts/release-smoke.ts:220-238 copies manifests and patches/, deletes pnpm-lock.yaml, then runs vp install --lockfile-only --ignore-scripts. Fresh resolution of ^1.4.1 picks the latest release (npm shows 1.9.0), so the 1.5.0 patch is unused.
  • Evidence: CI run 39410801474, job 118587861844 "Release Smoke", step "Exercise release-only workflow steps" failed: [ERR_PNPM_UNUSED_PATCH] The following patches were not used: @base-ui/react@1.5.0 then Command failed: vp install --lockfile-only --ignore-scripts. Saved raw log: release-smoke.log in this directory. Base f3a399d CI on main (run 39407383228) succeeded, so this PR introduced the failure.
  • Effect: red required check now; also a latent break for any lock regeneration (dependency refresh, upstream rebase), where the fix would silently drop or install fail.
  • Repo convention: every other patched dependency has an exact specifier (@pierre/diffs catalog 1.5.2, dbus-next 0.10.2, etc.).
  • Smallest fix: pin apps/web/package.json to "@base-ui/react": "1.5.0" (lock already resolves 1.5.0, so no version change; only the lock importer specifier line updates). This is a specifier pin, not an upgrade, and stays within the brief's "no broad dependency upgrade".

Cleared concerns (checked in source)

  • Mechanism. Installed patched esm/floating-ui-react/hooks/useHoverFloatingInteraction.js has the guard first in onFloatingMouseLeave. FloatingRootStore.setOpen always emits openchange (no same-value short-circuit), and FloatingFocusManager.js:399-400 sets preventReturnFocusRef on triggerHover + mouseleave; its unmount cleanup (:458) then skips return focus. So a late mouseleave after Escape does suppress focus return, and the guard removes exactly that producer.
  • Open-state behavior unchanged. When open is true the handler runs as before: descendant/tree listener, other-trigger, safe-polygon handler, click-like guard, closeWithDelay. The hover-dismiss control asserts false:trigger-hover:mouseleave occurs, so the guard does not swallow genuine dismissal.
  • Skipped side effects when closed are only close-publishing: tree.events.on('floating.closed'), safe-polygon handler, clearPointerEvents (already done by the !open layout effect), closeWithDelay/floating.closed emit. None is needed for an already closed popup.
  • Reference hook (useHoverReferenceInteraction.js:215) already guards on store.select('open'); the floating hook was the unguarded twin.
  • ESM and CJS both patched; runtime hashes match owner-bindings.json (c1191105… ESM, 11aa4108… CJS). Patch file SHA-256 65a7d832…bf2c9d2b equals the lock patchedDependencies hash and the installed .pnpm dir name; single importer (apps/web) for @base-ui/react.
  • Upstream fidelity. #3482 diff applies the same !store.select('open') guard to onScrollMouseLeave, which no longer exists in 1.5.0, so this is an adaptation, as the PR says. #4893's isHoverOpen() gate would also fix this case but changes open-menu semantics; not porting it is a defensible scope choice.
  • Setup incident. The root prepare (effect-tsgo patch && vp config) that patch-commit ran is the same step CI runs on every install (Release Smoke log line 364), so it does not taint proof binding beyond normal install state. I did not independently verify inode/link counts.
  • Test oracles. Pointer-over and submenu cases discriminate (red 2/4 on unpatched owner, green 6/6). Real userEvent.hover, native transition plus onOpenChangeComplete, no sleeps. Intentional-focus control asserts the closing popup is still mounted before moving focus. Reused author logs, not rerun.

Optional / unproven

  • O1 (behavior change, untested). Hover-opened submenu chain: before the patch, Escape on the child with the pointer over it caused the late mouseleave to emit floating.closed, which the parent's onNodeClosed used to cascade-close the parent. After the patch the parent stays open. Probably the desired Escape semantics (close one level), but if the pointer then leaves without re-entering the parent, the parent stays open until outside press or Escape. Inference from source only; worth noting in the integrated client pass.
  • O2 (out of scope, pre-existing). A keyboard- or controlled-opened menu still closes when the pointer enters and leaves it while open (the #4893 bug). Not fix(web): preserve Escape focus return with pointer over menu #576.
  • The test file lives in components/ui/ and tests BaseUI behavior through the public wrapper; acceptable.

Input ledger

Parent-owned

Current CI ("Test", "Test Web", "Test Server 4/8" still in progress at review time), integrated client visual proof, and the separate human merge approval. Nothing here transfers #379 approval.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What R2 requests the fork-owner registration repair on ce1f908.
Why The exact Base UI pin modifies an upstream manifest without its required owner entry.
So what The same author will make the registry-only repair; the parent retains fresh review, CI, visual proof and separate merge approval.

The report below is relayed unchanged. Its explicit low observable effort does not establish the requested MEDIUM review gate; that routing discrepancy remains unresolved and cannot be waived by the useful findings.

PR #578 R2 independent whole review (Issue #576)

Verdict: REQUEST_CHANGES at head ce1f908f8fa588c4f8c90b8753e16bda2d7f957f, base f3a399d40c252730b02b7022b627763f7add91b0. One required fix, introduced by the R1 repair: the Fork Stack Model check fails. The product mechanism, patch, test, and R1 specifier repair are otherwise sound.

Binding

  • Reviewer: Claude Opus 5.5 (claude-opus-5-5), Claude Code harness in T3 Code. Author: GPT-6.1 Sol through Codex, a different family. I did not author any part of it.
  • Effort: MEDIUM was requested. This session's observable reasoning-effort setting is low, not medium, and I cannot change it from inside the run. That is a route constraint, so do not read this report as a MEDIUM-effort review. I compensated by checking source and raw evidence directly instead of relying on summaries.
  • Candidate: worktree clean; HEAD = ce1f908f8f…, tree 605f2be48cbe6e97859bcba591a682082cef4e70, merge-base = base. Two commits: bc03532f97 and ce1f908f8f. git diff base..HEAD | sha256sum = 5a16bdb23b7def9ccd6c32b5f1cb44f17df4036b2fc332d2e637ef91e18a8a9d (matches the brief). git diff bc03532f97..HEAD | sha256sum = 5a5c66ab…0ca8da (matches the handoff).
  • PR fix(web): preserve Escape focus over closing menus #578: OPEN, MERGEABLE, base main, headRefOid = head.
  • Files: apps/web/package.json (1 line), apps/web/src/components/ui/menu.focus.browser.test.tsx (+223), patches/@base-ui__react@1.5.0.patch (+30), pnpm-lock.yaml (+4/-3), pnpm-workspace.yaml (+1).

Required finding

F1. The Fork Stack Model check fails on this head. apps/web/package.json is an upstream file modified without an allowlist entry or an owner.

  • Location: the apps/web/package.json:15 edit ("^1.4.1" to "1.5.0"). scripts/fork-upstream-edits.txt has no apps/web/package.json line. docs/fork-features.md lists it under no feature's upstreamFiles (grep confirms; only apps/desktop/package.json, pnpm-lock.yaml and pnpm-workspace.yaml appear).
  • Reachable sequence: the Fork workflow runs bash scripts/fork-check.sh --upstream upstream. The check compares changed upstream files against the allowlist (scripts/fork-check.sh:60-79). At base, apps/web/package.json is identical to upstream/main (git diff --quiet upstream/main f3a399d40c -- apps/web/package.json returns 0). The R1 commit makes it an unlisted upstream modification.
  • Evidence: Fork run 39412266386 at headSha ce1f908f… ends failure. Its job "Fork Stack Model" logs fork-check: FAIL: upstream file modified but not allowlisted: apps/web/package.json and fork-check: FAIL: list the file in scripts/fork-upstream-edits.txt and assign its owner in docs/fork-features.md, or move the change to a new file. I pulled this with gh run view 39412266386 --log-failed; it matches the parent's 578-ce1f-review/fork-stack.log. The same workflow passed at bc03532f97 (run 39410801538), so R1 introduced the failure.
  • Effect: a red check on the PR. It also breaks the fork's documented rebase model: an upstream BaseUI bump would conflict on this line with no recorded owner.
  • Smallest fix: add apps/web/package.json to scripts/fork-upstream-edits.txt and to the owning feature's upstreamFiles in docs/fork-features.md. That is the remedy the check itself names, and the feature that already owns pnpm-lock.yaml/pnpm-workspace.yaml patch entries is a natural owner. Then run scripts/fork-check.sh (and node scripts/fork-features.mjs check). An alternative is a pnpm-workspace.yaml overrides pin, which is already allowlisted, but it widens lock churn. I recommend the allowlist.
  • Re-review scope: after the fix the head changes, so it needs fresh exact-head review and CI under the verification rules. No product source or proof needs to change.

Cleared (checked myself in source or raw evidence)

  • Mechanism. Patched installed ESM useHoverFloatingInteraction.js has if (!store.select('open')) return; first in onFloatingMouseLeave. CJS is the same; diff against the unpatched 1.5.0 store directory shows exactly these 4 lines in each file and no other file differences.
    • Why the bug happens: MenuRoot.setOpen (esm/menu/root/MenuRoot.js:190-193) short-circuits only when open, trigger and reason are all equal. After escape-key, a late trigger-hover close therefore dispatches openchange. FloatingFocusManager.js:399-401 then sets preventReturnFocusRef for triggerHover plus mouseleave, and the unmount cleanup at :458 skips return focus.
    • ReactStore.select exists in @base-ui/utils@0.2.9. The guard removes only the producer of that late close.
  • Open-state behavior unchanged. When open, every branch runs as before: tree onNodeClosed registration, other-trigger, descendant floating, safe-polygon handler, clearPointerEvents, click-like gate, closeWithDelay. The hover-dismiss control asserts that false:trigger-hover:mouseleave still occurs.
  • Owner choice and upstream. I read the full diffs and bodies myself:
    • #3482 (merged 106d3f6d) put the same !store.select('open') guard on onScrollMouseLeave, which no longer exists in 1.5.0. The candidate is an adaptation, as claimed.
    • #4893 (merged 94a9d1e6) gates closeWithDelay on isHoverOpen(). That also changes eventless/controlled open-menu semantics (O2), so not porting it is a defensible narrower choice.
    • #4899 (closed, unmerged) overrode focus-manager policy, broader than needed.
    • #4906 (merged 2aa1f560) is test-only.
    • The author's retained current-master hoverIntent.ts keeps the hover-origin gate (:423, :499).
    • The PR makes no claim of parity with upstream main, and I make none.
  • Consumers. ui/menu.tsx passes BaseUI Root/Trigger/Popup through without focus overrides. RightPanelTabs.tsx:1009-1031 focuses the trigger and then sets controlled open; the fixture mirrors that shape. ui/menu.tsx, RightPanelTabs.tsx and RightPanelTabs.keyboard.browser.test.tsx are unchanged from base. apps/web is the only @base-ui/react importer in the lock.
  • Dependency identity. The patch file SHA-256 65a7d832…bf2c9d2b equals the lock patchedDependencies hash, the importer/snapshot patch_hash, and the installed .pnpm directory name. Patch, test and pnpm-workspace.yaml bytes are unchanged between bc035 and the head. The R1 delta is exactly the manifest specifier plus the matching lock importer specifier line.
  • R1 repair works in real CI. CI run 39412266251 at ce1f908f… concluded success. Its Release Smoke job 118592710717 passed, which is the release-smoke lockless consumer R1 found failing. The author's offline full lockless scratch (r1/lockless.log, exit1 on uncached tinyps via astro@7.4.0) is correctly labelled a setup limit, not a pass. Their reduced seam (r1/focus-lockless, exit0) regenerates @base-ui/react@1.5.0(patch_hash=65a7d832…), and I confirmed that in the retained lock.
  • Proof bindings. I recomputed every hash in r1/bindings.json (five inputs, six logs/lock) and all match. I recomputed owner-bindings.json (10 baseline/patched runtime files) and patched-runtime-binding.json (3), and every current installed file matches.
    • final-red.json/final-green.json bind the test hash 2bf87e0b… and the lock af6fa42d…, which equals bc035's lock. The current lock differs only by the specifier line, so the installed runtime is unchanged and the reuse is valid for runtime behavior.
    • final-red uses baseline.config.mts, which aliases @base-ui/react/* to the unpatched 1.5.0 ESM directory.
    • Raw logs: final-red has 2 failures (pointer-over, submenu Escape; body active) and 4 passes, exit1. Final-green has 15/15: the 6 fixture tests plus all 9 unchanged RightPanelTabs tests, exit0. rightpanel-over-real passes 1 test with 8 skipped and traces escape-key then MENU_COMPLETE false, with no second hover close.
    • These are author executions, reused. I ran none.
  • Test design. It uses the public wrapper, a real userEvent.hover after open, a native 40ms CSS transition plus onOpenChangeComplete, and no sleeps or added timeouts. The common, failure and awkward cases are present: pointer over and away, focus moved elsewhere during close, outside click, genuine hover dismissal, and a keyboard-entered submenu with pointer movement and Escape. The red/green results show the oracle discriminates.
  • Setup incident. The root prepare that ran during patch-commit is task-local and also runs on every CI install. The current proof binds to hashes, not to that run, so it does not taint the evidence. I did not recheck inodes.
  • test(web): isolate panel keyboard tests from inherited pointer #575 distinction. test(web): isolate panel keyboard tests from inherited pointer #575 is the upstream test-only pointer-isolation port for RightPanelTabs (approved separately). fix(web): preserve Escape focus over closing menus #578 is the product owner repair. The test(web): isolate panel keyboard tests from inherited pointer #575 comment confirms fix(web): preserve Escape focus return with pointer over menu #576 owns this defect. Approval of spec: native cross-machine control across linked environments #379 or test(web): isolate panel keyboard tests from inherited pointer #575 does not cover fix(web): preserve Escape focus over closing menus #578.

Optional / unproven

  • O1 (carried, inference only). In a hover-opened submenu chain, Escape on the child with the pointer over it no longer cascade-closes the parent through a late floating.closed. This is probably the desired one-level Escape, but it is untested. Leave it to the parent's real-client pass.
  • O2 (carried, out of scope). A keyboard- or controlled-opened menu still closes when the pointer enters and then leaves it (the behavior #4893 fixed). This predates fix(web): preserve Escape focus return with pointer over menu #576 and is not part of it.
  • O3 (new, inference). The patched hook is shared by Popover, Tooltip, PreviewCard (used in PullRequestLinkPreview.tsx) and NavigationMenu. While closed, a mouseleave no longer clears a pending openChangeTimeout or emits floating.closed. One effect: re-hovering a trigger during a closing tooltip or preview exit animation is no longer cancelled by the pointer crossing the exiting popup. This looks benign, but no test covers these components.
  • O4 (proof limit). The fixture's press dispatches synthetic, untrusted KeyboardEvents rather than userEvent.keyboard. I did not verify whether the trusted-event path differs; the RightPanelTabs probes use the original helpers.

Input ledger

  • Full: cumulative 5-file diff and R1 delta; the patch bytes; the test file; the installed patched and unpatched ESM+CJS hover hook (whole file); the MenuRoot.setOpen section; the MenuPopup returnFocus section; the FloatingFocusManager openchange/unmount section; ui/menu.tsx (Menu/Trigger/Popup parts); RightPanelTabs shortcut and menu wiring; handoff.md (current); handoff-bc035.md (old canonical); patch-choice.md; r1/bindings.json; owner-bindings.json; patched-runtime-binding.json; baseline.config.mts; final-red.json/final-green.json; r1/focus-lockless.json and its package, workspace, lock and log tail; r1/lockless.log tail; 531-web-focus-diagnosis/handoff.md; R1 report (it matches the PR comment except for the comment's summary header); Issue fix(web): preserve Escape focus return with pointer over menu #576 body and all three comments; PR fix(web): preserve Escape focus over closing menus #578 body and both comments; the test(web): isolate panel keyboard tests from inherited pointer #575 pair comment 6289372521; upstream #3482/#4893/#4899/#4906 bodies and diffs (the #4893 hook change in full, its tests skimmed); the release-smoke.ts staging/deletion path; the fork-check.sh allowlist logic; operations verification, code-review index, review-method, impact-analysis and test-design; Great Library README.
  • Partial: raw final-red, final-green and rightpanel-over-real logs (results, assertions and trace lines grepped, not every line); upstream current hover sources (gates grepped); docs/fork-features.md (structure around the upstreamFiles block); other hover consumers (call sites only).
  • Not read: baseline-v6, patched-v6 and rightpanel-over-fake raw logs; the instrumented-*.tsx and right-panel.config.mts bodies; the author's retained upstream T3 files; the #4893 Popover/PreviewCard test bodies; CONTRIBUTING.md; GLOSSARY.md (no new terms in this diff).
  • Reused, not executed by me: all browser red/green, RightPanelTabs probes, fmt, lint, typecheck and frozen/lockless pnpm runs. CI evidence is GitHub-owned and was read live.

Commands and hashes I checked

git rev-parse HEAD HEAD^{tree}; git merge-base; git diff base..HEAD | sha256sum; git diff bc035..HEAD | sha256sum; git show HEAD:<f> | sha256sum for all 5 inputs; sha256sum on the patch, the r1 logs and lock, and the owner/runtime bindings (Python hashlib recompute); diff of the unpatched and patched .pnpm packages; grep of lock @base-ui/react entries at base and head; gh pr view 578; gh run list --branch fix/576-escape-hover-focus; gh run view 39412266386 --log-failed; Release Smoke job lookup in run 39412266251; gh pr view/diff of mui/base-ui 3482/4893/4899/4906 (stored under upstream/ here); gh issue view 576; gh api reads of comments 6289372521, 6289352308 and 6289344909. Exact candidate proof classes: source and dependency inspection by me (fresh); hash binding by me (fresh); behavioral browser proof (author, reused, bound by hash); CI (GitHub, live: CI success including Release Smoke, Fork failure).

Parent-owned

Status writes and the review relay, the F1 repair dispatch, the fresh exact-head review after repair, current CI (the rollup still showed Check in progress at review time), real-client visual verification (including O1), and the separate human main-merge approval. Nothing here transfers #379 or #575 approval.

Addendum: parent-supplied Fork log and owner inventory (head still ce1f908f8f…, worktree clean)

  • Raw log. Parent log 578-ce1f-review/fork-stack.log: 1261 lines, SHA-256 cdfe36aee12724131b31a2db1235861e7cfaf7f18938085d2be114a3338ea799. Its decisive lines:

    • line 851: fork-check: FAIL: upstream file modified but not allowlisted: apps/web/package.json
    • line 1260: the remedy message
    • line 1261: ##[error]Process completed with exit code 1

    The live record agrees: run 39412266386, job 118592663992 "Fork Stack Model", conclusion failure. It is the only FAIL line. The new files patches/@base-ui__react@1.5.0.patch (line 1242) and the test are reported as "new fork file" and pass.

  • Inventory. scripts/fork-features.mjs:60-78 requires each allowlisted path to have exactly one feature owner in upstreamFiles, and each owned upstream path to be allowlisted.

    • No feature in docs/fork-features.md owns apps/web/package.json, the BaseUI patch, or menu.focus.browser.test.tsx. grep base-ui|menu.focus|576 returns nothing.
    • pnpm-lock.yaml and pnpm-workspace.yaml are owned by fork-maintenance, which is why bc035 passed.
    • So F1 is confirmed. It is not an auto-waivable flake, and the earlier pass does not cover it: the candidate carries a fork edit to an upstream file with no registry owner.
  • Owner of the fix. The fix(web): preserve Escape focus return with pointer over menu #576/fix(web): preserve Escape focus over closing menus #578 delivery: the author lane, dispatched by the parent. The required change is registry-only; product source, patch and test stay unchanged.

    • Recommended shape: a new narrow feature entry, for example menu-escape-focus, following the launcher-test-fixtures pattern. Purpose with a drop condition (drop when upstream BaseUI ships an equivalent closed-popup or hover-origin guard such as #4893 and the dependency is upgraded); issues #576; prs #578; newFiles the patch and the test; upstreamFiles apps/web/package.json; sharedFiles pnpm-lock.yaml and pnpm-workspace.yaml; keywords such as onFloatingMouseLeave and @base-ui/react 1.5.0. Plus one line apps/web/package.json in scripts/fork-upstream-edits.txt.
    • Folding it into fork-maintenance would also pass, but would hide the drop condition. I recommend the separate entry.
    • Proof for the repaired head: bash scripts/fork-check.sh --upstream upstream and node scripts/fork-features.mjs check, then a fresh exact-head review and CI.
  • CI state on ce1f. Release Smoke success; CI workflow 39412266251 success; Fork 39412266386 failure. CI is not all green.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What Fresh whole R3 Opus5.5 review approves #578 at19ecbbfc20306fdd2f69432968fe83f4aaa91df4.
Why Both the exact-version pin and explicit fork-owner registration now match the patched runtime.
So what The parent completes current CI and integrated visual evidence before requesting the separate576 main-merge approval.

Full report follows unchanged. Independent review is separate from readiness and permission to merge. MEDIUM was explicitly selected; app configuration is verified by the parent's exact read-only child/run metadata, while backend internal effort is not independently attestable. The R2 model self-report alone did not establish a routing failure.

PR #578 R3 independent whole review (Issue #576)

Verdict: APPROVE at exact head 19ecbbfc20306fdd2f69432968fe83f4aaa91df4, tree bf300c3aa9f5f1992c9304f2ba82c561e86bfce9, base f3a399d40c252730b02b7022b627763f7add91b0. I found no required finding in the inspected scope. The verdict covers review only. It does not clear current CI, the integrated real-client before/after images and video, or the separate human main-merge approval. All three stay parent-owned.

Binding

  • Reviewer: Claude Opus 5.5 (claude-opus-5-5), Claude Code harness in T3 Code. Author: GPT-6.1 Sol through Codex, a different family. I authored none of it.
  • Effort:
    • Requested: MEDIUM, through an explicit selection array.
    • Configured: the parent reports options[{id:'effort',value:'medium'}] from a read-only SQLite read. I did not re-read that row.
    • Observed: my session context exposes no literal reasoning-effort field. I can neither confirm nor deny the backend setting, so I make no "low" claim and no backend attestation. Output style is not evidence of reasoning effort.
  • Candidate identity, all checked by me:
    • Worktree clean (git status --porcelain empty). HEAD and tree match the brief, and the merge-base equals the base.
    • Three commits: bc03532f97, ce1f908f8f, 19ecbbfc20.
    • git diff base..HEAD | sha256sum = 1e7db9813f269e638dd0caf4492082da47cb751e647d799631518aaf00d6d907. git diff ce1f..HEAD | sha256sum = 59993f67fd606de410012a63f0a0eb388d4b2e41d085340f66b77766f199eecf. Both match r2/bindings.json.
    • Seven files total. The R2 delta touches only docs/fork-features.md (+18) and scripts/fork-upstream-edits.txt (+3).
  • git show HEAD:<f> | sha256sum for all seven inputs matches r2/bindings.json, both the inputHashes and the unchangedProductInputHashes. The product, patch, test, manifest, lock and workspace bytes are identical to ce1f.
  • Live PR: OPEN, MERGEABLE, base main, headRefOid = head.

Required findings

None.

The R2 F1 repair (the only new bytes), checked myself

  • Checker semantics. scripts/fork-check.sh:60-79 requires every changed upstream path to be allowlisted. scripts/fork-features.mjs:60-78 requires each allowlisted path to have exactly one upstreamFiles owner, and each owned path to be allowlisted.
  • New entry menu-escape-focus.
    • upstreamFiles is apps/web/package.json, and it is the only owner (grep finds one registry hit and one allowlist line, at scripts/fork-upstream-edits.txt:488).
    • newFiles are the patch and the regression test.
    • sharedFiles are pnpm-lock.yaml and pnpm-workspace.yaml. fork-maintenance still owns them as upstreamFiles (docs/fork-features.md:328-329), which matches docs/fork.md:16-18 ("other carried features may list it as shared").
    • The issues, prs (/pull/578) and keywords fields are valid for the schema.
  • Retirement condition. The purpose states it explicitly: upgrade to a BaseUI release with equivalent late-hover-close protection (for example #4893), then prove pointer-over Escape plus the hover, submenu and outside-focus controls pass unpatched, before dropping the patch and the exact pin. It keeps regression coverage until upstream has equivalent tests. This follows the launcher-test-fixtures drop-clause pattern and the docs/fork.md:20-22 absorption rule. The @base-ui/react keyword deliberately surfaces upstream BaseUI bumps in the overlap report, which is the retirement trigger.
  • No map edit needed. The fix adds no docs/map.md part. The AGENTS map rule applies only to added, split or removed parts. No other test pins the registry contents or count (scripts/fork-maintenance.test.ts grep).
  • Raw logs (hashes match r2/bindings.json):
    • r2/baseline-fork-check.log:847 contains the single line FAIL: upstream file modified but not allowlisted: apps/web/package.json.
    • r2/fork-check.log has 0 such lines and ends with fork-features: OK (57 features). and fork-check: OK.
    • r2/fork-check.json records exit 0 at the exact head.
  • Live CI at the exact head. Fork run 39414004930: job "Fork Stack Model" success. This is the GitHub-owned confirmation of the repair. The author's local run is corroboration only.

Whole candidate (unchanged bytes, re-inspected rather than inherited)

  • Mechanism. diff -r between the installed unpatched and patched @base-ui/react@1.5.0 directories shows only the 4-line guard, in ESM (esm/.../useHoverFloatingInteraction.js:123) and in CJS. I read the patched onFloatingMouseLeave, closeWithDelay and onNodeClosed in full. While the store is open, every branch is unchanged. While it is closed, the handler skips only close publication: setOpen(false, triggerHover) and floating.closed. That removes the late trigger-hover + mouseleave change that makes FloatingFocusManager prevent focus return. Per the R2 report and fix(orchestration): wake parents for remote pending requests #531 diagnosis trace, read in full, Escape on a keyboard-opened menu with the pointer over it ends with focus on body because of that late change.
  • Patch identity.
    • Patch SHA 65a7d832… = the workspace registration, the lock patchedDependencies entry, the importer and snapshot patch_hash, and the installed .pnpm path.
    • The manifest pin 1.5.0 matches the lock importer specifier, so the R1 repair is intact.
    • Release Smoke is success on the current head (live rollup).
  • Runtime binding. I recomputed all 10 owner-bindings.json installed-file hashes and the patched package.json hash from patched-runtime-binding.json, and all match. The installed runtime that produced the behavioral evidence is therefore unchanged.
  • Behavioral evidence (author executions, reused, bound by hash, not run by me):
    • final-red: 2 failed / 4 passed, exit 1. The failures are pointer-over Escape and submenu Escape, and the assertions show body active (final-red.log:668, :3336).
    • final-green: 15/15, exit 0. That is the 6 fixture tests plus all 9 unchanged RightPanelTabs tests.
    • Both bind the test hash 2bf87e0b… and the bc035 lock af6fa42d…. The current lock differs from that only in the specifier line. Reuse is valid for runtime behavior.
  • Test design.
    • It uses the public wrapper, a real userEvent.hover after open, a native 40ms transition plus onOpenChangeComplete, and no sleeps or timeouts.
    • Controls cover pointer away, intentional focus elsewhere, outside click, genuine hover dismissal (which asserts false:trigger-hover:mouseleave), and a keyboard submenu with pointer movement.
    • It rejects the plausible faulty fix of an unconditional trigger.focus(), via the focus-elsewhere and hover-dismiss controls.
  • Scope and upstream.
    • The fix is one problem. ui/menu.tsx, RightPanelTabs.tsx and its keyboard test are unchanged.
    • Upstream adaptation: #3482 applies the same guard in the former owner. #4893 is a broader hover-origin gate. #4899 is unmerged. #4906 is test-only. No main-parity claim is made.
    • No version bump, so the AGENTS PR rule holds. CONTRIBUTING one-problem and evidence rules are met for review purposes.
  • Issue and PR records. The Issue fix(web): preserve Escape focus return with pointer over menu #576 body and all 5 comments are read. Canonical comment 6289678149 equals handoff.md (diff shows only a trailing blank line). The PR body is current to 19ec.

Optional / proof limits (carried, not new findings)

  • O1 (source inference). A hover-opened submenu chain: Escape on the child with the pointer over it no longer cascade-closes the parent through a late floating.closed. Covering this belongs to the parent's real-client pass.
  • O2 (pre-existing, out of fix(web): preserve Escape focus return with pointer over menu #576). Open-menu hover-dismissal of keyboard- or controlled-opened menus.
  • O3 (source inference, untested). The shared hook also serves Popover, Tooltip, PreviewCard and NavigationMenu. A closed-popup mouseleave no longer clears a pending openChangeTimeout or emits floating.closed during their exit.
  • O4. The fixture's press uses synthetic untrusted KeyboardEvents. The trusted keyboard path is left to the integrated pass.

Input ledger

  • Full:
    • the R3 brief
    • the R2 brief
    • the cumulative and R2 diffs
    • the test file
    • the patch, manifest, lock and workspace hunks
    • scripts/fork-check.sh
    • scripts/fork-features.mjs
    • the new registry entry plus the fork-maintenance and launcher-test-fixtures entries
    • docs/fork.md registry and absorption paragraphs
    • handoff.md (current)
    • handoff-bc035.md
    • patch-choice.md
    • 531-web-focus-diagnosis/handoff.md
    • r2/bindings.json and r2/fork-check.json
    • the r2 registry and format logs
    • the R1 report and the R2 report (including its addendum)
    • Issue fix(web): preserve Escape focus return with pointer over menu #576 body and all comments
    • PR fix(web): preserve Escape focus over closing menus #578 body
    • root AGENTS.md (identical to main) and CONTRIBUTING.md
    • operations SKILL.md, the code-review index, review-method, impact-analysis, structure-and-constraints, verification and test-design references
    • the Great Library README
    • the patched installed hover hook
  • Partial:
    • r2/baseline-fork-check.log and r2/fork-check.log (FAIL and OK lines and tails)
    • final-red.log and final-green.log (result and assertion lines)
    • docs/fork-features.md outside the entries named above
  • Reused from R2, not re-read by me:
    • upstream #3482/#4893/#4899/#4906 diffs and bodies, and current BaseUI master sources
    • the MenuRoot and FloatingFocusManager sections. I re-read only the hook, and the mechanism is consistent with the diagnosis trace.
    • test(web): isolate panel keyboard tests from inherited pointer #575 pair comment 6289372521
    • release-smoke.ts
    • r1 lockless and frozen logs
    • rightpanel-over-* logs
    • handoff-ce1f.md (historical; its content is reproduced in Issue comment 6289428827, which I read)
  • Not executed by me: every test, check, install and format run. No source, Git ref, GitHub or status writes. The only write is this report.

Commands and hashes checked

git rev-parse HEAD HEAD^{tree}, git merge-base, git status --porcelain, git log, git diff --stat, the two diff sha256 values, git show HEAD:<f> | sha256sum for all seven files, sha256sum of the r2 logs, a Python hashlib recompute of the owner-bindings.json paths, diff -r of the installed unpatched and patched BaseUI, gh pr view 578, gh run list and gh run view 39414004930, gh issue view 576, gh api for Issue comments, and the diff of canonical comment 6289678149 against handoff.md.

Proof classes:

  • Fresh by me: source and dependency inspection, and hash binding.
  • Author executions, reused by hash: browser red/green.
  • Live from GitHub: CI.

Current CI at review time (parent-owned disposition)

Head 19ec shows:

  • Success: Fork Stack Model, Release Smoke, Lint, Build and Typecheck, Rust, Test Server 6.
  • In progress: Test, Test Web and Test Server 1-5, 7 and 8.
  • review/independent: pending.

CI is not complete, so this report makes no CI-green claim.

Parent-owned

Relaying this report and writing the status, current CI disposition, the integrated real-client before/after images and video (including O1, O3 and O4), and the separate human approval for the #576 main merge. #379 and #575 approvals do not transfer.

@lukemaj

lukemaj commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

What: The real Right Panel now returns keyboard focus after Escape while the pointer remains over its menu.
Why: Main loses focus to the document body when the closing popup emits a late hover-exit event.
So what: This exact candidate has independent review, passing current CI and isolated before/after product evidence; main merge still needs approval for this separate outcome.

Parent product verification, 20 October 2026, 13:27–13:34 Warsaw:

  • Before: main f3a399d40c252730b02b7022b627763f7add91b0, tree 7cbb439a2fcaf6b20d2c7d7b597e98c44eebb0cb.
  • After: 19ecbbfc20306fdd2f69432968fe83f4aaa91df4, tree bf300c3aa9f5f1992c9304f2ba82c561e86bfce9.
  • Both clean task-owned worktrees, their own dev state and Incognito Browser tabs, 1280×800 viewport. Installed ESM and CJS hover owners inspected at each candidate; baseline lacks the guard and candidate includes it. No source edits for this check.

Steps: focus Add panel surface, press Enter, hover Terminal, press Escape. Real Browser keyboard and pointer events reported isTrusted: true; observation code only recorded events and inspected the DOM, without forcing focus or changing application state.

Before: menu closes, aria-expanded=false, active element is BODY. After: menu closes, aria-expanded=false, active element is the Add panel surface button, whose visible focus ring appears in the after image. The recordings retain the pointer/key interaction.

Additional current control: hover Browser to open its submenu; Escape closes only that submenu, leaving its parent menu open and Browser focused. Clicking the message composer then closes the remaining menu and focuses the composer. No provider was launched and no message sent.

Coverage limits: this is the actual web Right Panel, not a desktop-shell/mobile claim or an exhaustive shared-hover-consumer matrix. A separate tooltip attempt did not observe a tooltip opening and establishes no tooltip result. Earlier synthetic browser tests remain separately reported. Native fingerprint's first cancelled run has a newer successful rerun; all current CI jobs, including Test Web, Release Smoke and Fork Stack Model, passed on this head.

Local raw observations, artifact SHA256s, exact source/runtime identities and clean status are retained under 578-product-proof. Both isolated dev instances remain available for inspection. No installed service, live data, existing grant, credential or account was changed. Images/videos are uploaded here, not committed.

Requirements and who asked: #576 and the project's UI proof rule require preserving Escape focus and before/after interaction evidence.
Deleted: browser login, credential retrieval, external asset hosting, committed PR images and forced-focus substitutes.
Bottleneck: the earlier fixture did not establish actual trusted input in the product; the paired current/main Browser observation now does.
Checked myself: actual product before/after events, DOM focus, visible screenshots, submenu control, source/runtime hashes, clean Git identities and current GitHub CI.

Before and after screenshots, followed by before and after recordings:

Before: menu closed but focus was lost
After: menu closed and Add panel surface retained focus

Before recording:

before.mp4

After recording:

after.mp4

@lukemaj
lukemaj merged commit c3557ef into main Oct 20, 2026
25 of 26 checks passed
@lukemaj
lukemaj deleted the fix/576-escape-hover-focus branch October 20, 2026 12:21
@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: PR #578 merged into main as c3557ef338fce173285b40149fa3c19e49b52e86, and its isolated proof resources are retired.
Why: The reviewed fix prevents late hover exit from suppressing Escape's return focus; exact-head CI and the real before/after Browser check passed.
So what: #576 is closed and this outcome is finalized; #379 continues separately, with no deployment or installed-service restart performed.

User explicitly approved merging PR #578 at reviewed head 19ecbbfc20306fdd2f69432968fe83f4aaa91df4. Final live admission: PR OPEN/MERGEABLE/CLEAN, exact head unchanged, all current required CI passed, newest review/independent was success created by lukemaj and linked to the complete Opus5.5 MEDIUM review. The cancelled first fingerprint run has a successful later rerun. Actual trusted-input before/after images and recordings remain on GitHub.

Merged through the repository-supported squash path with --match-head-commit and no bypass, at 2026-10-20 14:18:08 Warsaw. Verified GitHub MERGED and main ref c3557ef338fce173285b40149fa3c19e49b52e86; its actual parent is 7b01552f1ef0372d0be4e0d1483c965731778257 (#580). Stable patch ID of the applied main delta equals the reviewed f3a399d→19ecbbf delta: cb6deb99c311dc104046fef8d7f4f925bdb98b28. Main's intervening recovery change is preserved.

Finalization completed 14:21 Warsaw: verified author task terminal with no pending child runs, closed both task-owned Incognito tabs, stopped the two captured own supervisors, then stopped the four remaining listeners only after confirming exact port-owner cwd. Verified no listeners on 14514/6474/14119/6079 and no processes left in either task worktree. Removed clean candidate /home/luke/.t3/worktrees/chromeria/fix-576-escape-hover-focus and baseline /home/luke/dev/toolboxmd/chromeria-worktrees/576-before-f3a3; deleted local and remote fix/576-escape-hover-focus with exact-head lease, then verified absence. External proof/review logs and published artifacts are retained; no dependent task required these checkouts.

Machine record pushed to lukemaj/runbook main as 05c5cc1 in bestia/CHANGES.md. Task-local process/port signals and verified after-state are retained in 578-product-proof/cleanup-*.json. No installed service, live database, existing user grant, credentials or other task workspace was changed. Watching is off. Website/SEO impact: none, existing component focus behavior with no public site or URL change. No package version bump per project policy.

#379's driver remains at its own frozen de50 checkpoint; #385/#381 implementation and #581 review remain independent. This separate merge is not #379 acceptance or a deployment.

Requirements and who asked: The user explicitly approved this exact #578 main merge after review/CI/product proof; repository finalization and machine-record rules require retiring disposable proof resources.
Deleted: deployment/restart, main-checkout edits, retained obsolete test processes/worktrees and use of #379 approval for this separate outcome.
Bottleneck: the separate merge gate was cleared by the user's approval; no remaining requirement blocks this focus repair.
Checked myself: live exact-head CI/review creator, merge result/main parent and patch identity, terminal author state, own process/cwd/port after-state, local/remote branch and worktree absence, retained evidence and pushed runbook record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(web): preserve Escape focus return with pointer over menu

1 participant