Fix decouple preview from focus event so virtualized rows don't freeze it#509
Open
Ahmad-kadri wants to merge 6 commits intoealush:masterfrom
Conversation
Add the nonce to the PickerStyleTag
Avoids preview freezing when row virtualization unmounts the button before focusElement's rAF fires (e.g. Mac trackpad inertia scroll). Signed-off-by: Ahmad Kadri <ahmad.kadri@conceptboard.com>
|
Someone is attempting to deploy a commit to the ealush's projects Team on Vercel. A member of the Team first needs to authorize it. |
Review Summary by QodoDecouple emoji preview from focus event for virtualized rows
WalkthroughsDescription• Set emoji preview synchronously on hover to prevent freezing • Decouple preview update from focus event to handle virtualized rows • Rename helper function for clarity on synchronous preview setting • Improve focus handling for partially visible and fully visible emojis Diagramflowchart LR
A["onMouseOver event"] --> B["setPreviewFromButton<br/>synchronously"]
B --> C["Preview updates<br/>immediately"]
A --> D["Check emoji<br/>visibility"]
D --> E["Below fold:<br/>blur only"]
D --> F["Fully visible:<br/>focus element"]
E --> G["No preview freeze<br/>on virtualization"]
F --> G
File Changes1. src/hooks/useEmojiPreviewEvents.ts
|
Code Review by Qodo
1. Preview cleared after blur
|
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates useEmojiPreviewEvents.ts to handle emoji previews synchronously, addressing an issue where virtualized rows could unmount buttons before focus events occur. It also refines the handling of partially visible emojis by blurring the active element instead of focusing it to prevent automatic scrolling. A review comment suggests narrowing the blur operation to only target emoji buttons to avoid accidental focus loss on other UI components like the search input.
| if (belowFoldByPx < buttonHeight) { | ||
| // Partially below the fold: skip focus to avoid auto-scroll-into-view. | ||
| // Manually blur so the previous focus ring doesn't linger. | ||
| (document.activeElement as HTMLElement)?.blur?.(); |
There was a problem hiding this comment.
Blurring the active element unconditionally can cause unexpected focus loss on other UI components, such as the search input, if the mouse happens to hover over a partially visible emoji while the user is interacting with another part of the picker. It is safer to only blur if the currently focused element is an emoji button.
Suggested change
| (document.activeElement as HTMLElement)?.blur?.(); | |
| buttonFromTarget(document.activeElement as HTMLElement)?.blur(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useEmojiPreviewEvents.ts`:
- Around line 87-109: Remove the manual blur in the partially-below-fold branch:
delete the (document.activeElement as HTMLElement)?.blur?.() call in the code
path that calls setPreviewFromButton and detectEmojyPartiallyBelowFold inside
useEmojiPreviewEvents so we don't trigger the global body blur listener; instead
just return without blurring so the synchronous preview update isn't undone.
Also update the onLeave handler to ignore blur events that have relatedTarget
=== null when those blurs originated from this "skip focus" path (i.e., ensure
onLeave doesn't unconditionally clear the preview on null relatedTarget), using
the same identifying logic around
setPreviewFromButton/detectEmojyPartiallyBelowFold/focusElement to detect the
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 143103e4-0188-4927-a0a0-4d255c71ccdb
📒 Files selected for processing (1)
src/hooks/useEmojiPreviewEvents.ts
Comment on lines
+87
to
109
| if (!button) { | ||
| return; | ||
| } | ||
|
|
||
| // Set preview synchronously rather than relying on the focus listener: | ||
| // virtualized rows can unmount the button before focusElement's rAF fires | ||
| // (e.g. Mac trackpad inertia scroll), so button.focus() becomes a no-op | ||
| // and the preview would otherwise freeze. | ||
| setPreviewFromButton(button, setPreviewEmoji); | ||
|
|
||
| focusElement(button); | ||
| const belowFoldByPx = detectEmojyPartiallyBelowFold(button, bodyRef); | ||
| const buttonHeight = button.getBoundingClientRect().height; | ||
|
|
||
| if (belowFoldByPx < buttonHeight) { | ||
| // Partially below the fold: skip focus to avoid auto-scroll-into-view. | ||
| // Manually blur so the previous focus ring doesn't linger. | ||
| (document.activeElement as HTMLElement)?.blur?.(); | ||
| return; | ||
| } | ||
|
|
||
| // Fully visible: focus too, so keyboard navigation continues from here. | ||
| focusElement(button); | ||
| } |
There was a problem hiding this comment.
Don't blur document.activeElement here.
That blur fires the existing body-level blur listener, and onLeave will usually clear the preview again because relatedTarget is null. So the new synchronous hover update gets undone for partially visible rows.
Suggested fix
function useEmojiPreviewEvents(
allow: boolean,
setPreviewEmoji: React.Dispatch<React.SetStateAction<PreviewEmoji>>,
) {
@@
useEffect(() => {
if (!allow) {
return;
}
const bodyRef = BodyRef.current;
+ let suppressLeave = false;
@@
if (belowFoldByPx < buttonHeight) {
// Partially below the fold: skip focus to avoid auto-scroll-into-view.
// Manually blur so the previous focus ring doesn't linger.
+ suppressLeave = true;
(document.activeElement as HTMLElement)?.blur?.();
+ suppressLeave = false;
return;
}and in onLeave:
function onLeave(e?: FocusEvent | MouseEvent) {
+ if (suppressLeave) {
+ return;
+ }
if (e) {
const relatedTarget = e.relatedTarget as HTMLElement;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useEmojiPreviewEvents.ts` around lines 87 - 109, Remove the manual
blur in the partially-below-fold branch: delete the (document.activeElement as
HTMLElement)?.blur?.() call in the code path that calls setPreviewFromButton and
detectEmojyPartiallyBelowFold inside useEmojiPreviewEvents so we don't trigger
the global body blur listener; instead just return without blurring so the
synchronous preview update isn't undone. Also update the onLeave handler to
ignore blur events that have relatedTarget === null when those blurs originated
from this "skip focus" path (i.e., ensure onLeave doesn't unconditionally clear
the preview on null relatedTarget), using the same identifying logic around
setPreviewFromButton/detectEmojyPartiallyBelowFold/focusElement to detect the
case.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+91
to
105
| // Set preview synchronously rather than relying on the focus listener: | ||
| // virtualized rows can unmount the button before focusElement's rAF fires | ||
| // (e.g. Mac trackpad inertia scroll), so button.focus() becomes a no-op | ||
| // and the preview would otherwise freeze. | ||
| setPreviewFromButton(button, setPreviewEmoji); | ||
|
|
||
| focusElement(button); | ||
| const belowFoldByPx = detectEmojyPartiallyBelowFold(button, bodyRef); | ||
| const buttonHeight = button.getBoundingClientRect().height; | ||
|
|
||
| if (belowFoldByPx < buttonHeight) { | ||
| // Partially below the fold: skip focus to avoid auto-scroll-into-view. | ||
| // Manually blur so the previous focus ring doesn't linger. | ||
| (document.activeElement as HTMLElement)?.blur?.(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
1. Preview cleared after blur 🐞 Bug ≡ Correctness
In onMouseOver, the preview is set and then document.activeElement.blur() is called for partially-below-fold emojis; the resulting captured blur event runs onLeave() which clears the preview, so hovering a partially visible emoji can end with the preview immediately becoming null.
Agent Prompt
### Issue description
Hovering a partially-below-fold emoji calls `setPreviewFromButton(...)` and then blurs the current active element. Because the hook also listens for `blur` (capture) and `onLeave()` clears the preview, the blur can clear the just-set preview, leaving the preview blank.
### Issue Context
This only occurs in the `belowFoldByPx < buttonHeight` branch (partially visible), where focus is intentionally skipped.
### Fix Focus Areas
- src/hooks/useEmojiPreviewEvents.ts[91-105]
### Suggested change
Move the blur earlier (before setting the preview) in the partially-visible path, or re-set the preview after blurring, e.g.
- compute belowFold first
- if partially visible: blur active element, then call `setPreviewFromButton(button, setPreviewEmoji)`, then `return`
- else: keep current behavior
This preserves the intent (clear focus ring) without clearing the new preview.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -105,7 +118,7 @@ export function useEmojiPreviewEvents( | |||
| }, [BodyRef, allow, setPreviewEmoji, isMouseDisallowed, allowMouseMove]); | |||
There was a problem hiding this comment.
2. Leaking mouseover listeners 🐞 Bug ☼ Reliability
useEmojiPreviewEvents adds a capturing mouseover listener but removes it without capture=true, so it may not be removed; since the effect depends on unstable function references (recreated each render), repeated re-runs can accumulate listeners and multiply preview updates over time.
Agent Prompt
### Issue description
A capturing `mouseover` listener is added but not removed with matching capture options, which can prevent cleanup. Combined with unstable function dependencies that can cause the effect to re-run on renders, this can accumulate listeners and amplify preview updates.
### Issue Context
`addEventListener('mouseover', ..., true)` must be paired with `removeEventListener('mouseover', ..., true)` (or an equivalent `{ capture: true }` options object).
### Fix Focus Areas
- src/hooks/useEmojiPreviewEvents.ts[36-39]
- src/hooks/useEmojiPreviewEvents.ts[111-117]
- src/hooks/useEmojiPreviewEvents.ts[118-118]
- src/hooks/useDisallowMouseMove.ts[13-25]
### Suggested change
1) Fix cleanup to match capture:
- `bodyRef?.removeEventListener('mouseover', onMouseOver, true);`
2) Reduce effect churn by stabilizing dependencies:
- Wrap `useIsMouseDisallowed()` / `useAllowMouseMove()` returned functions in `useCallback`, or
- Remove `allowMouseMove` from the dependency list in `useEmojiPreviewEvents` (it’s currently unused there), and ensure any remaining function deps are stable.
This prevents repeated re-binding and avoids listener accumulation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
With row virtualization (most reliably triggered by Mac trackpad inertia scroll), hovering emojis can leave the preview frozen on a stale emoji. The last row is unaffected because it uses handlePartiallyVisibleElementFocus, which sets the preview synchronously and bypasses the focus event entirely.
Root cause
onMouseOver updates the preview indirectly: it calls focusElement(button), which queues button.focus() in a requestAnimationFrame; the resulting focus event then triggers onEnter, which calls setPreviewEmoji. If virtualization unmounts the button before the rAF fires, .focus() is a no-op on a detached node — no focus event, no preview update.
Fix
Set the preview synchronously from onMouseOver instead of relying on the focus event. Still call focusElement(button) for fully-visible emojis so existing behaviors are preserved.