Repository navigation
Conversation
Hovering project/model selector rows flapped DOM focus between the row, the menu content, and the search input — the caret flashed on every hover and typing could lose its target. Guard the three focus-steal sites in @kobalte/core: while an editable inside the menu has focus, pointer hover only updates visual state. pointerleave is not cancelable, so a defaultPrevented guard alone cannot cover it. Adds an e2e regression test for the project selector.
|
The following comment was made by an LLM, it may be inaccurate: |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
The patch fixes the project-selector focus fight, but it breaks hover in the worktree submenu.
I ran the #56868 reproduction on the base and with this PR. On the base, focus moved between the row, the search input and the menu content on every hover, and your focus test failed. With the PR, the focus log is empty and both new tests pass. new-session, settings-search, model-selection-flow and mobile-model-pickers e2e specs also pass.
The regression: the patch skips the selectionManager().setFocusedKey(...) call, not only the DOM focus call. Kobalte uses that same key for data-highlighted and for Enter. The project and model selectors draw their own highlight (classList plus active state), so they look fine. The worktree submenu (packages/app/src/workspaces/submenu.tsx, with search shown at 10 or more worktrees) uses Kobalte's highlight. I tested it with 12 worktrees and the search focused, then hovered tree03 (screenshots below):
- On the base, the row highlights and Enter selects it.
- With this PR, nothing is highlighted and Enter does not select
tree03; the trigger stays onLocal.
This would hit any Kobalte menu that contains a text field. Please keep the highlight and Enter working while focus stays in the search field, and test the worktree submenu.
The new spec also needs changes; see the inline comments.
Public API: unchanged (adds a patchedDependencies entry for @kobalte/core@0.13.13).
| - focusWithoutScrolling2(e.currentTarget); | ||
| - menuContext.listState().selectionManager().setFocused(true); | ||
| - menuContext.listState().selectionManager().setFocusedKey(key()); | ||
| + // Keep focus on an editable (search input) inside this menu; hover only updates visual state. |
There was a problem hiding this comment.
This guard also skips setFocused(true) and setFocusedKey(key()), and Kobalte uses that focused key for data-highlighted and for Enter. In the worktree submenu (packages/app/src/workspaces/submenu.tsx, 10 or more worktrees so the search field shows), hovering a row with the search focused now leaves no row highlighted, and Enter no longer picks the hovered row. On the base, hover moves focus to the row and highlights it, and Enter selects it. Could the guard skip only the DOM focus call (focusWithoutScrolling) and still update the selection manager? Another option is to fix this in the app for the selectors that own their highlight. Either way, please check the worktree submenu still highlights on hover and that Enter selects the hovered row. The same applies to the sub-trigger hunk below.
There was a problem hiding this comment.
Good catch — fixed in 3e4d1a4. The guard now skips only focusWithoutScrolling and never touches the selection manager.
One thing the suggested split alone wouldn't have covered: create-selectable-item has an effect that DOM-focuses the item as soon as it becomes the focused key, so keeping setFocused/setFocusedKey re-introduced the steal on every hover (measured:6 steals per sweep before I guarded that site too). It is now a fourth guarded site with the same condition.
Verified against your scenario:10+ worktrees, search focused, hover tree03 → the row gets its rendered highlight (the data-highlighted background), focus stays in the field, and Enter selects tree03 — a new case in the new-session keeper covers all three. The submenu field also now forwards Enter to the highlighted row (app side): Kobalte has no content-level Enter, so with focus kept in the field there was no row keydown to fire on. The sub-trigger hunk is restructured the same way.
| @@ -0,0 +1,72 @@ | |||
| import { expect, test } from "@playwright/test" | |||
There was a problem hiding this comment.
packages/app/e2e/AGENTS.md asks for one keeper suite per area: please add this as a case in e2e/regression/new-session.spec.ts instead of a new spec file. One test for the focus log is enough. The second test mostly re-checks existing behaviour (highlight, aria-activedescendant, filtering) and can be dropped or cut down.
There was a problem hiding this comment.
Done in 3e4d1a4: the standalone project-selector.spec.ts is removed, one focus-log case now lives in e2e/regression/new-session.spec.ts, and the second test is dropped.
|
|
||
| for (const label of ["HoverAlpha", "HoverBeta", "HoverGamma"]) { | ||
| await menu.locator("[data-option-key]", { hasText: label }).hover({ steps: 5 }) | ||
| await page.waitForTimeout(60) |
There was a problem hiding this comment.
The e2e guide rules out waitForTimeout. The focus log can be checked straight after the hovers, or wait for a visible state such as the hovered row being active.
This checks a Tailwind class name, not something the user sees. aria-activedescendant (checked next) already shows which row is active, so this line can go.
There was a problem hiding this comment.
Done in 3e4d1a4: the focus log is asserted straight after the hovers with no waitForTimeout, and the class assertion is gone. The remaining highlight check is in the new worktree submenu case and asserts the rendered background change (only data-highlighted paints it), not a class name.
There was a problem hiding this comment.
Thanks, I checked these in 3e4d1a4 in my last review: the focus check now runs right after the hovers with no waitForTimeout, and the class-name check is gone. Comparing the computed background before and after hover is fine for the worktree submenu case. The only open point is dropping the two empty .bun-tag-* file hunks at the top of the patch file.
There was a problem hiding this comment.
That was the last open point — dropped in 04d574d, see the reply above. Nothing else outstanding from my side.
Review follow-up: the guard skipped the selection manager together with the DOM focus call, so Kobalte-driven menus (the worktree submenu) lost data-highlighted and Enter stopped selecting the hovered row. The guard now skips only focusWithoutScrolling, including in create-selectable-item's effect that DOM-focuses an item when it becomes the focused key — a fourth steal site the manager updates would otherwise re-trigger. The worktree submenu forwards Enter from its search field to the highlighted row: Kobalte only handles Enter on the row itself, so with focus staying in the field no row keydown ever fired. Tests move into the new-session keeper per e2e/AGENTS.md: one focus-log case without waitForTimeout or class assertions, and a worktree submenu case covering rendered highlight, retained focus, and Enter selection.
There was a problem hiding this comment.
Both earlier concerns are fixed, and #56868 stays fixed. One small thing is left in the patch file.
I ran the #56868 reproduction and the e2e specs on this commit:
- The reproduction's focus log is empty.
new-session(with the two new cases),settings-search,model-selection-flow,mobile-model-pickersandhomeall pass.- Both new cases fail on the base, so they catch the bug.
- In the worktree submenu with 12 worktrees, hovering
tree03keeps focus in the search field, highlights the row, and Enter selectstree03once. packages/apptypecheck is clean.
Resolved since the last review:
- Worktree submenu: hover highlight and Enter work again.
- Tests: the test is now a case in the
new-sessionsuite, with nowaitForTimeoutand no class-name check.
The new guards only apply when a text field inside the same menu has focus, so menus without a search field keep their current keyboard and pointer focus.
Public API: unchanged (adds a patchedDependencies entry for @kobalte/core@0.13.13).
Still to fix: the patch starts with two empty .bun-tag-* file hunks (inline comment).
| @@ -0,0 +1,156 @@ | |||
| diff --git a/node_modules/@kobalte/core/.bun-tag-18e03a8591dab1bf b/.bun-tag-18e03a8591dab1bf | |||
There was a problem hiding this comment.
These first two hunks only add empty node_modules/@kobalte/core/.bun-tag-* files. They look like leftovers from the tool that generated the patch, and their paths don't match the other hunks. Please drop them so the patch only touches the four dist/chunk files.
There was a problem hiding this comment.
Done in 04d574d: both .bun-tag-* hunks are removed — they were leftovers from bun patch --commit, not part of the fix. The patch now touches only the four dist/chunk files, and bun install --force re-applies it cleanly (guards verified present in the installed package afterwards).
The two empty .bun-tag-* hunks at the top of the patch were artifacts of bun patch --commit, not part of the fix. The patch now only touches the four dist/chunk files; bun install --force re-applies it cleanly.
…-hover-focus Conflict resolutions: - package.json/bun.lock: upstream's patch list plus the @kobalte/core@0.13.13 entry; bun install regenerated the lock and re-applied the patch. - workspaces/submenu.tsx: accepted upstream's deletion; the Enter-forwarding it carried is ported to session/composer/location-missing.tsx, where the searchable worktree menu now lives. - new-session.spec.ts: upstream's imports and cases; our stale project-selector/submenu cases removed (that dropdown was replaced by the location dialog) and rewritten as one case in session-location-sync.spec.ts, the keeper that owns the location-missing worktree menu.
There was a problem hiding this comment.
The new commits fix all four earlier points:
- the guards skip only the DOM focus call and keep Kobalte's highlight state;
- the test is a single case in a keeper suite;
- the test has no
waitForTimeoutor class-name check; - the
.bun-tag-*hunks are gone.
After the merge from dev, the searchable worktree menu lives in location-missing.tsx. Your new session-location-sync case fails on the base (the search field loses focus) and passes with this PR: hover highlights tree03, focus stays in the field, and Enter moves the session to /projects/tree03. The related new-session, model-selection-flow and settings-search specs pass; one new-session test failed once and passed twice on rerun. tsc -b in packages/app is clean.
Public API: unchanged (adds a patchedDependencies entry for @kobalte/core@0.13.13).
One small test cleanup is left; see the inline comment.
packages/app/e2e/regression/session-location-sync.spec.ts:396: This copies the 404LocationNotFoundErrorroute and the/moveroute from the missing-location test at the top of this file.e2e/AGENTS.mdasks to share setup like this, not copy it. Could both tests use one small helper that opens a session whose location is missing and returns the recorded moves? The test would then shrink to the hover, focus and Enter checks.


Issue for this PR
Closes #56868
Type of change
What does this PR do?
Pointer hover steals DOM focus from a search field rendered inside a menu. Four
sites do it: item and sub-trigger
onPointerMovefocus the hovered row,pointerleaverefocuses the content, andcreateSelectableItem's focused-keyeffect refocuses the row whenever
setFocusedKeyruns. The caret restarts itsblink on every mouse move and typing loses its target.
The
@kobalte/corepatch skips only thefocusWithoutScrollingcalls atthose four sites, gated on an editable inside the same
[role="menu"]holdingfocus. The selection manager is never touched, so
data-highlighted,keyboard state, and Enter-to-select behave exactly as before.
This update merges current
devand re-homes the work after the locationredesign. The original repro dropdown and
workspaces/submenu.tsxare gone;the searchable worktree menu now lives in
session/composer/location-missing.tsx, which still fights this bug by hand(see its "Hovering an item moves focus to it, so send typing back to the search
field" keydown listener and its double-rAF autofocus). The Enter-forwarding
that used to be in the submenu moved to that menu's search field, and the stale
keeper cases became one case in
session-location-sync.spec.ts, the keeperthat owns this menu. Final diff against
dev: 5 files, +262/−4 —package.jsonand
bun.lockgain only the patch entry.How did you verify your code works?
Built mode (
PLAYWRIGHT_BUILD=1, what CI runs) against the unpatched base:the new
session-location-synccase fails at its firsttoBeFocused— thefield never holds focus, because the collection's focusin handler marches focus
to the first row as soon as it mounts (which is why
location-missing.tsxyanks typing back on keydown). With the patch restored, the field keeps focus
from open, through the hover sweep, to Enter — the whole keeper passes
10/10, including the move assertion (
Enterselects the highlighted row).Dev-mode batches:
session-location-sync+new-session+home→ 38/39(the one failure is an upstream branch-search test that passes in isolation);
model-selection, mobile-model-pickers, settings-search green.
tsc -b --forceclean; the
typecheck:e2eerrors are all in files this diff doesn't touch.test:unit: 899 pass / 0 fail.Visual
Behavioral only — the caret stays steady while sweeping rows instead of
flashing; no animated SVG warranted for What's new.
Screenshots / recordings
The original flash is described with a repro in #56868; happy to record the
worktree-menu before/after if a reviewer wants to see it.
Checklist