Menu focus-first-item never fires (CreateMenu + IdentityMenu dead branch) #477
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#477
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Follow-up flagged by the #466 review (PR #476 findings). The focus-first-item if (!getOpen()) after setOpen in CreateMenu.jsx reads the new value so it never fires on open — and IdentityMenu.jsx carries the identical dead line (the clone is faithful). Fix both menus together: focus the first menuitem on open (or drop the dead branch with a note).
Fixed by #478 — both menus now capture the pre-toggle open state so the focus-first-item branch fires on open. Tests: targeted 53/53 green, full suite matches pristine main (15 pre-existing unrelated failures), vite build green.
REVIEW PR #478 (fix/issue-477) — verified in scratch worktrees, main worktree untouched.
(1) Fix correct in both menus: CreateMenu.jsx:39-41 and IdentityMenu.jsx:43-45 both capture const opening = !getOpen() BEFORE setOpen(opening) and guard the queueMicrotask first-item focus on if (opening). Correct — Solid signals update synchronously, so the old post-setOpen if (!getOpen()) read could never fire on open. queueMicrotask scheduling mechanism is unchanged from the #466-reviewed pattern; only the guard condition changed. Focus now schedules on open only, never on close.
(2) Esc/outside-click/arrows/Tab intact: diff touches only toggle(); onDocClick, onMenuKey, close(refocus) byte-identical. Test pins all handlers present in both files.
(3) No other focus behavior changed: close(true) trigger-refocus untouched; panel roles/classes untouched (incl. mobile max-w rules).
(4) Tests meaningful: menu-focus-477.test.js pins the fix pattern per-menu, pins the dead setOpen((o)=>!o)+read shape absent, pins the popover contract per-file, and the closure-signal simulation proves schedule-on-open / never-on-close plus documents the old dead-branch shape. Mirrors the create-menu-466.test.js source-text convention (no DOM). Trivial nit, not blocking: PR desc says '9 tests' — the file has 8 test() calls (3+3 per-menu + 2 simulation); targeted run create-menu-466+identity-nav+menu-focus-477 = 53/53 green confirmed.
(5) No backend change (diff is 2 JSX + 1 test file only), no new deps (package.json/lock untouched; laws 1/7/8/12 clean — no seam, no long-work, no doc deviation).
Head-to-head full node suite (symlinked node_modules, scratch worktrees): pristine main = 1012 tests / 2 fail; PR branch = 1020 tests (+8 new) / 2 fail. Failing sets identical file-by-file (smoke.test.js 'built SPA shell' + 'hashed assets' — both hit whatever is listening on 127.0.0.1:8080 in this environment, unrelated to the PR). Note: this contradicts the fix agent's '885/900 with 15 failures' claim — actual counts are 1012/1020 with 2 env-caused failures on both sides. No PR-caused failure; nothing to block or fix.
vite build (PR scratch): green in 2.17s (chunk-size warning only, pre-existing). No browser drive per task (node tests + reasoning).
MERGE RECOMMENDATION: ready to merge.
Fixed by PR #478 (review clean; fix verified in both menus, head-to-head shows only the 2 pre-existing smoke failures), merged. Closing.