Clone menu popover doesn't close on outside click

crueber commented

2026-09-09 20:28:30 +00:00

Owner

What's wrong

After clicking Clone to open the clone popover, clicking anywhere outside the popover does not close it. It stays open until the user presses Escape or toggles the Clone pill again.

Root cause (code evidence)

CloneMenu (web/src/pages/Repo.jsx:39-149) renders the popover as a <details class="clone-menu"> element (:85). It wires an Escape handler (:80-84, onKey → root.open = false) but never registers a document-level outside-click handler — there is no document.addEventListener("click", …) anywhere in the component.
The codebase already has the exact pattern to copy, twice:
- RefPicker (Repo.jsx:240-243): const close = (e) => { if (root && !root.contains(e.target)) { setOpen(false); … } }; document.addEventListener("click", close); with removal in onCleanup.
- TasksOverlay (Repo.jsx:417-420): identical onDoc outside-click close with cleanup.
The bug is visible in the code: CloneMenu manages getOpen via the <details> onToggle event (:85) but nothing closes it. Note the outside-click check must exclude clicks on the trigger itself (the <summary> pill) so the toggle doesn't fight the close — the RefPicker pattern handles this correctly because its trigger is inside root (same shape applies here: summary is inside details, so a plain !root.contains(e.target) check works and the summary's native toggle still fires).
Same class of check: NotificationTray and TasksOverlay — TasksOverlay already closes on outside click; verify NotificationTray does too while in this file (it uses the tray pattern from #133; fix in this issue only if it shares the gap).

Fix

Add the standard outside-click close to CloneMenu: document click listener that sets root.open = false when the click target is outside the details, registered on mount and removed in onCleanup (mirror Repo.jsx:417-420). Keep the Escape handler and focus-return behavior (:80-84) unchanged.

Acceptance criteria

Clicking anywhere outside the open clone popover closes it (including clicks on other header buttons).
Clicking the Clone pill itself still toggles the popover (no double-toggle flicker — the outside handler must not close-then-reopen on the same click).
Escape still closes with focus returned to the trigger (no regression).
Listener is removed on cleanup (no leak across route changes — mirror the existing onCleanup pattern).
NotificationTray checked for the same gap; fixed in the same change if affected (note which way it went in the PR).

## What's wrong After clicking **Clone** to open the clone popover, clicking anywhere outside the popover does not close it. It stays open until the user presses Escape or toggles the Clone pill again. ## Root cause (code evidence) - `CloneMenu` (`web/src/pages/Repo.jsx:39-149`) renders the popover as a `<details class="clone-menu">` element (:85). It wires an Escape handler (:80-84, `onKey` → `root.open = false`) but **never registers a document-level outside-click handler** — there is no `document.addEventListener("click", …)` anywhere in the component. - The codebase already has the exact pattern to copy, twice: - `RefPicker` (Repo.jsx:240-243): `const close = (e) => { if (root && !root.contains(e.target)) { setOpen(false); … } }; document.addEventListener("click", close);` with removal in `onCleanup`. - `TasksOverlay` (Repo.jsx:417-420): identical `onDoc` outside-click close with cleanup. - The bug is visible in the code: `CloneMenu` manages `getOpen` via the `<details>` `onToggle` event (:85) but nothing closes it. Note the outside-click check must exclude clicks on the trigger itself (the `<summary>` pill) so the toggle doesn't fight the close — the RefPicker pattern handles this correctly because its trigger is inside `root` (same shape applies here: `summary` is inside `details`, so a plain `!root.contains(e.target)` check works and the summary's native toggle still fires). - Same class of check: `NotificationTray` and `TasksOverlay` — TasksOverlay already closes on outside click; verify NotificationTray does too while in this file (it uses the tray pattern from #133; fix in this issue only if it shares the gap). ## Fix Add the standard outside-click close to `CloneMenu`: document click listener that sets `root.open = false` when the click target is outside the `details`, registered on mount and removed in `onCleanup` (mirror Repo.jsx:417-420). Keep the Escape handler and focus-return behavior (:80-84) unchanged. ## Acceptance criteria - [ ] Clicking anywhere outside the open clone popover closes it (including clicks on other header buttons). - [ ] Clicking the Clone pill itself still toggles the popover (no double-toggle flicker — the outside handler must not close-then-reopen on the same click). - [ ] Escape still closes with focus returned to the trigger (no regression). - [ ] Listener is removed on cleanup (no leak across route changes — mirror the existing `onCleanup` pattern). - [ ] `NotificationTray` checked for the same gap; fixed in the same change if affected (note which way it went in the PR).

crueber referenced this issue from a commit

2026-09-10 01:43:29 +00:00

Fix #255: clone menu + notification tray close on outside click

crueber referenced this issue from a pull request that will close it,

2026-09-10 01:43:31 +00:00

Fix #255: clone menu outside-click close #265

crueber commented

2026-09-10 01:43:33 +00:00

Author

Owner

Fixed by #265 (branch fix/issue-255): CloneMenu gets the standard outside-click close (document listener → root.open=false, removed in onCleanup), mirroring TasksOverlay/RefPicker. NotificationTray shared the gap and is fixed in the same change. 522/522 web tests green (smoke.test.js hangs identically on main — needs a live server). Browser check open.

crueber commented

2026-09-10 01:55:57 +00:00

Author

Owner

Review of PR #265 (fix/issue-255, 150e2c4) — clone menu + notification tray outside-click close.

What was checked (scratch worktree /tmp/pr265, since removed; main worktree untouched, still clean on main):

web/src/pages/Repo.jsx CloneMenu (lines 53-59), web/src/components/NotificationTray.jsx (lines 25,44-54,75), web/test/unit/clone-outside-close.test.js (new, 4 tests).

Findings:

Repo.jsx:57-59 — listener shape matches RefPicker (Repo.jsx:249-250) / TasksOverlay (Repo.jsx:426-427) precedent exactly: document click + !root.contains(e.target) guard, single onCleanup folding clearTimeout + removeEventListener. Solid runs component setup once per mount, so no double-add on re-render. PASS.
Trigger-inside-root, no toggle fight — CloneMenu
lives inside
(Repo.jsx:97-98); tray bell + dropdown both inside
(NotificationTray.jsx:75-127). Clicks on triggers are inside root so onDoc ignores them. PASS.
Esc + focus-return preserved — CloneMenu onKey untouched (Repo.jsx:90-95, root.open=false + summary focus); tray close()/onKey untouched (NotificationTray.jsx:56-62). PASS.
Open-state re-sync — CloneMenu writes root.open=false on the DOM and getOpen follows via the existing onToggle handler (Repo.jsx:97), same path as the Esc handler, so no signal/DOM desync. Tray writes setOpen(false) directly and renders via , so no split to desync. PASS.
No new deps — diff touches only the two JSX files + one test; package.json/lock untouched; only DOM addEventListener APIs. PASS (AGENTS.md law 1; laws 7/8/12 N/A — UI bugfix following the existing documented pattern, no new seam or doc contract).
Nit (non-blocking): NotificationTray.jsx registers onCleanup (line 44) textually before const onDoc is declared (line 53). Correct at runtime (closure evaluates at cleanup, after init) but inconsistent with the TasksOverlay ordering (handler declared before use). Left as-is; optional reorder.

Tests (no browser per instructions — node tests + reasoning, stated explicitly):

New file alone: 4/4 pass.
Full suite web/test/unit/*.test.js: 525/525 pass, exit 0 (note: scratch worktree needed web/node_modules symlinked from the main checkout for the marked/dompurify imports; without it 7 markdown-related files fail on module resolution — environment-only, unrelated to this PR).
vite build in web/: success (139 modules, dist JS+CSS emitted).
No fixes pushed — nothing broken. MERGE RECOMMENDATION: ready to merge.

Review of PR #265 (fix/issue-255, 150e2c4) — clone menu + notification tray outside-click close. What was checked (scratch worktree /tmp/pr265, since removed; main worktree untouched, still clean on main): - web/src/pages/Repo.jsx CloneMenu (lines 53-59), web/src/components/NotificationTray.jsx (lines 25,44-54,75), web/test/unit/clone-outside-close.test.js (new, 4 tests). Findings: 1. Repo.jsx:57-59 — listener shape matches RefPicker (Repo.jsx:249-250) / TasksOverlay (Repo.jsx:426-427) precedent exactly: document click + !root.contains(e.target) guard, single onCleanup folding clearTimeout + removeEventListener. Solid runs component setup once per mount, so no double-add on re-render. PASS. 2. Trigger-inside-root, no toggle fight — CloneMenu <summary> lives inside <details ref={root}> (Repo.jsx:97-98); tray bell + dropdown both inside <div ref={root}> (NotificationTray.jsx:75-127). Clicks on triggers are inside root so onDoc ignores them. PASS. 3. Esc + focus-return preserved — CloneMenu onKey untouched (Repo.jsx:90-95, root.open=false + summary focus); tray close()/onKey untouched (NotificationTray.jsx:56-62). PASS. 4. Open-state re-sync — CloneMenu writes root.open=false on the DOM and getOpen follows via the existing onToggle handler (Repo.jsx:97), same path as the Esc handler, so no signal/DOM desync. Tray writes setOpen(false) directly and renders via <Show when={getOpen()}>, so no split to desync. PASS. 5. No new deps — diff touches only the two JSX files + one test; package.json/lock untouched; only DOM addEventListener APIs. PASS (AGENTS.md law 1; laws 7/8/12 N/A — UI bugfix following the existing documented pattern, no new seam or doc contract). 6. Nit (non-blocking): NotificationTray.jsx registers onCleanup (line 44) textually before const onDoc is declared (line 53). Correct at runtime (closure evaluates at cleanup, after init) but inconsistent with the TasksOverlay ordering (handler declared before use). Left as-is; optional reorder. Tests (no browser per instructions — node tests + reasoning, stated explicitly): - New file alone: 4/4 pass. - Full suite web/test/unit/*.test.js: 525/525 pass, exit 0 (note: scratch worktree needed web/node_modules symlinked from the main checkout for the marked/dompurify imports; without it 7 markdown-related files fail on module resolution — environment-only, unrelated to this PR). - vite build in web/: success (139 modules, dist JS+CSS emitted). No fixes pushed — nothing broken. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-10 01:56:17 +00:00

crueber commented

2026-09-10 01:56:18 +00:00

Author

Owner

Fixed by PR #265 (review clean incl. NotificationTray same-gap fix; 525/525), merged. Closing.

crueber referenced this issue from a commit

2026-09-10 01:56:19 +00:00

Merge pull request 'Fix #255: clone menu outside-click close' (#265) from fix/issue-255 into main

crueber referenced this issue

2026-09-10 22:10:10 +00:00