fix(desktop): cron edit button unclickable in sidebar (#66854) - #66919
fix(desktop): cron edit button unclickable in sidebar (#66854)#66919kyssta-exe wants to merge 1 commit into
Conversation
…6854) The suppressNonKeyboardFocusOpen function called event.preventDefault() on focus events, which in Electron/Chromium prevented subsequent click events from firing on the same element. This broke the cron edit button in the sidebar. Instead of preventDefault(), use a context-based approach: the trigger sets a ref flag when focus is mouse-driven, and the Tooltip Root's onOpenChange handler checks it before opening. This preserves the original fix for stuck tooltips while allowing clicks to work. Fixes NousResearch#66854
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing this to the shared tooltip wrapper; current main still has the affected Tip-wrapped cron manage button at apps/desktop/src/app/chat/sidebar/cron-jobs-section.tsx:250.
Problems
apps/desktop/src/components/ui/tooltip.tsx:60usesonOpenChangeas a cancellation hook, but Radix has already committed an uncontrolled state change before notifying that callback. ItsuseControllableStatecallssetUncontrolledPropfirst (node_modules/@radix-ui/react-use-controllable-state/dist/index.js:76-78) and invokes the callback later (:94-99), so returning attooltip.tsx:50does not keep the tooltip closed.apps/desktop/src/components/ui/tooltip.test.tsx:28and:80reimplement the proposed branches rather than renderTip/Tooltip; they cannot catch that Radix lifecycle behavior.
Suggested changes
- Intercept the state request before Radix commits it, for example by owning controlled open state in the wrapper, then forward accepted transitions to any caller callback.
- Add a rendered interaction test covering non-keyboard focus, click delivery, suppressed tooltip open, and keyboard-focus accessibility.
Automated hermes-sweeper review.
| } | ||
| return ( | ||
| <suppressMouseFocusOpenContext.Provider value={{ suppressNextOpen: suppressNextOpenRef }}> | ||
| <TooltipPrimitive.Root data-slot="tooltip" onOpenChange={handleOpenChange} {...props} /> |
There was a problem hiding this comment.
onOpenChange is a post-transition notification for uncontrolled Radix Tooltip state, not a cancellation hook: setUncontrolledProp(nextValue) runs before its onChange effect. Returning from handleOpenChange therefore suppresses only a consumer callback; it does not prevent the tooltip from opening. Please intercept the request before Radix commits state (for example with controlled open state).
| const suppressNextOpen = { current: true } | ||
| const onOpenChange = vi.fn() | ||
|
|
||
| // Simulate the Tooltip Root onOpenChange handler logic |
There was a problem hiding this comment.
This test reimplements handleOpenChange instead of exercising TooltipPrimitive.Root, so it cannot verify the proposed cancellation mechanism. Render Tip around a button and assert both that mouse-focus/click is delivered and that the tooltip does not open; retain a keyboard-focus case.
teknium1
added
sweeper:risk-compatibility
|
Closing due to merge conflicts. This PR has a |
3 participants
Summary
Fixes #66854 - The cron edit button in the desktop sidebar was unclickable in v0.18.2.
Root Cause
The previous fix for stuck tooltips (commit 7f69494) used
event.preventDefault()on focus events to suppress tooltip opens for mouse-driven focus. However, in Electron/Chromium, callingpreventDefault()on a focus event prevents subsequent click events from firing on the same element, breaking interactive elements wrapped in<Tip>components.Fix
Instead of
preventDefault(), use a context-based approach:TooltipTriggersets a ref flag when focus is mouse-driven (not keyboard-visible)TooltipRoot'sonOpenChangehandler checks this flag before openingChanges
apps/desktop/src/components/ui/tooltip.tsx: ReplacesuppressNonKeyboardFocusOpenwith context-based suppressionapps/desktop/src/components/ui/tooltip.test.tsx: Update tests to cover the new logicTesting
All 5 tests pass:
:focus-visibleis unsupportedonOpenChangesuppresses open when flag is setonOpenChangeallows open when flag is not setVerification
Tested in the desktop app: cron edit button now responds to clicks, and tooltips no longer get stuck after menu interactions.