fix(spx-gui): defer dropdown outside click binding

[nighca]

Summary

• keep dropdown outside-close behavior on document click
• delay outside-click listener binding with timeout(0) so the opening interaction is not treated as an outside click
• use getCleanupSignal(onCleanup) to avoid binding listeners after the effect is cleaned up
• restore click-based outside-close tests and add a regression test for manual mouseup open

Validation

• npm run type-check
• npm run lint
• npm test
• npx vitest --run src/components/ui/UIDropdown.test.ts

[gemini-code-assist]

Code Review

This pull request changes the UIDropdown component to use the mouseup event instead of click for detecting outside clicks, intended to fix timing issues with manual control. However, the review feedback points out that this change introduces significant issues, including broken accessibility for keyboard users, unexpected closures during text selection (drag-out), and potential conflicts if the dropdown is opened on mousedown. It is recommended to revert to the click event and address the timing issue using a setTimeout or by checking event timestamps.

[fennoai]

The deferred-binding approach is cleaner than the previous mouseup revision — it preserves click semantics and AbortSignal-based cleanup eliminates the mismatched-removeEventListener risk. Posted 4 inline comments:

1. UIDropdown.vue:237 — ESC listener is unnecessarily deferred. The mouseup → click race only affects click; keydown has no such timing conflict. Deferring both creates a real (brief) gap where ESC doesn't close the dropdown. Only the click registration needs await timeout(0).

2. UIDropdown.vue:233 — Pass signal to timeout(0). timeout(0) accepts an AbortSignal that calls clearTimeout on abort. Without it, a rapid re-run of the effect leaves an orphaned setTimeout that runs to completion only to find signal.aborted. Use await timeout(0, signal) to cancel it eagerly.

3. UIDropdown.vue:207 — Add a note about async watchEffect and untracked post-await reads. Vue silently discards the returned Promise and stops tracking reactive deps after the first await. The current code is correct (all reads are before await), but a brief comment would guard against future maintainers adding reactive reads after the await.

4. UIDropdown.test.ts:26 — flushDropdown is now coupled to the timeout(0) implementation detail. vi.advanceTimersByTimeAsync(0) only advances by exactly 0 ms; if the delay is ever changed, flushDropdown silently stops flushing it. A comment noting the coupling would help.

View job run

[Copilot]

Pull request overview

This PR adjusts UIDropdown’s outside-close behavior to avoid immediately closing when the dropdown is opened via a mouseup-driven manual control flow, and updates tests to cover the timing regression.

Changes:

• Defer binding the document click outside-close listener to the next macrotask (timeout(0)) and guard against binding after effect cleanup via getCleanupSignal(onCleanup).
• Switch dropdown outside-close tests back to click-based behavior and add a regression test for manual mouseup open.
• Update test flushing to advance timers to accommodate the deferred listener binding.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
spx-gui/src/components/ui/UIDropdown.vue	Defers outside-click listener binding and uses cleanup abort signals to prevent late bindings.
spx-gui/src/components/ui/UIDropdown.test.ts	Adjusts flush helper for timers and adds a regression test for mouseup-opened manual dropdowns.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[fennoai]

The deferred-binding approach is cleaner than the previous mouseup switch — it keeps click semantics and the AbortSignal-based cleanup eliminates the mismatched-removeEventListener risk. A few things worth addressing:

[cn0809]

LGTM