fix tooltip race condition #5030
Conversation
WalkthroughThe changes in this pull request primarily affect the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/components/Modal/ModalSamplePlugin.tsx (1)
Line range hint
31-39: Consider combining tooltip state resetsWhile the cleanup is good, consider combining both tooltip state resets for better consistency.
useEffect(() => { // reset tooltip state when modal is closed setIsTooltipLocked(false); + setTooltipDetail(null); return () => { - setTooltipDetail(null); + setIsTooltipLocked(false); + setTooltipDetail(null); }; }, []);This ensures both states are reset together, both on mount and cleanup, preventing any potential race conditions if the component unmounts during state updates.
app/packages/core/src/components/Modal/Modal.tsx (1)
190-193: Optimize event listener cleanup with optional chaining.The current null check can be simplified using optional chaining.
- removeTooltipEventHanlderRef.current && - removeTooltipEventHanlderRef.current(); + removeTooltipEventHanlderRef.current?.();🧰 Tools
🪛 Biome
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/packages/core/src/components/Modal/Modal.tsx(4 hunks)app/packages/core/src/components/Modal/ModalNavigation.tsx(2 hunks)app/packages/core/src/components/Modal/ModalSamplePlugin.tsx(1 hunks)app/packages/core/src/components/Modal/hooks.ts(2 hunks)app/packages/core/src/components/Modal/modal-context.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/packages/core/src/components/Modal/modal-context.ts
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/core/src/components/Modal/Modal.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/ModalNavigation.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/ModalSamplePlugin.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/hooks.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Biome
app/packages/core/src/components/Modal/Modal.tsx
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
app/packages/core/src/components/Modal/ModalSamplePlugin.tsx (1)
3-4: LGTM! Clean removal of tooltip-related imports
The removal of useModalContext and simplified imports align well with the centralization of tooltip handling in Modal.tsx, helping to prevent race conditions.
app/packages/core/src/components/Modal/hooks.ts (2)
19-27: Well-implemented panel closure logic with proper ref handling!
The renaming from onNavigate to closePanels better reflects the function's purpose, and the use of refs properly handles the referential stability of the panel instances.
82-102: Effective race condition fix with atomic state updates!
The implementation properly handles the tooltip race condition by:
- Using Recoil's atomic state updates
- Checking tooltip lock status before updating coordinates
- Properly managing tooltip state through a centralized handler
app/packages/core/src/components/Modal/ModalNavigation.tsx (4)
47-47: LGTM! Prop rename improves clarity.
The rename from onNavigate to closePanels better reflects the actual purpose of the callback, making the code more maintainable.
78-81: LGTM! Consistent navigation handling.
The nextNavigator implementation correctly calls closePanels before navigation starts, which should help prevent the tooltip race condition. The debouncing mechanism (150ms) will also help prevent rapid navigation issues.
89-92: LGTM! Symmetric implementation.
The previousNavigator implementation mirrors the nextNavigator, ensuring consistent behavior in both directions.
Line range hint 47-92: Verify tooltip race condition fix.
The changes look good and should address the tooltip race condition by ensuring panels are closed before navigation starts. Let's verify there are no other tooltip-related issues in the codebase.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the tooltip race condition fix:
The tooltip race condition fix has been properly implemented and verified.
The code shows a robust implementation where:
- The
closePanelsfunction is properly integrated into the debounced navigation system viaonNavigationStartcallback - The tooltip state is managed through a comprehensive system of atoms and selectors
- The tooltip cleanup is handled both in the navigation flow and when the modal is closed
- The debouncing mechanism includes proper cleanup and state management to prevent race conditions
Key evidence:
- The
createDebouncedNavigatorensures panels are closed before navigation starts through theonNavigationStart: closePanelscallback - Tooltip state is properly managed through
isTooltipLocked,tooltipCoordinates, andtooltipDetailatoms - The implementation includes proper event handling and cleanup for tooltip state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tooltip-related race conditions or similar patterns
# Look for tooltip-related components and their cleanup
rg -g '*.{ts,tsx}' -i 'tooltip.*unmount|tooltip.*cleanup|tooltip.*effect'
# Look for similar panel closing patterns that might need the same fix
ast-grep --pattern 'onNavigate($$$)'
Length of output: 114
Script:
#!/bin/bash
# Search for tooltip components and their usage
rg -g '*.{ts,tsx}' -i 'tooltip' -A 5 -B 5
# Look for navigation-related components that might have similar patterns
rg -g '*.{ts,tsx}' 'createDebouncedNavigator|onNavigationStart' -A 5 -B 5
# Search for panel closing patterns
ast-grep --pattern 'closePanels($$$)'
Length of output: 238277
app/packages/core/src/components/Modal/Modal.tsx (4)
4-4: LGTM: Import changes align with new tooltip handling requirements.
The addition of useRef and useTooltipEventHandler imports supports the new tooltip event handling mechanism.
Also applies to: 10-10
181-196: LGTM: Proper event listener cleanup prevents memory leaks and race conditions.
The implementation correctly manages tooltip event listeners by:
- Storing the cleanup function in a ref
- Cleaning up previous listeners before adding new ones
- Using useCallback to maintain reference stability
🧰 Tools
🪛 Biome
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
222-222: LGTM: Prop rename improves code clarity.
Renaming onNavigate to closePanels better describes the function's purpose and follows React naming conventions.
186-196: Verify race condition fix effectiveness.
The implementation addresses tooltip race conditions through:
- Proper cleanup of previous event listeners before adding new ones
- Stable function references with useCallback
- Ref-based state tracking
However, let's verify there are no remaining race conditions in tooltip handling across the codebase.
✅ Verification successful
Race condition fix is properly implemented
The implementation effectively prevents tooltip-related race conditions through:
- Proper cleanup using
removeTooltipEventHanlderRefto track and remove previous event listeners before adding new ones - Stable function references with
useCallbackfor bothonLookerSetandsetActiveLookerRef - Proper event listener cleanup sequence in
onLookerSet:- Remove previous tooltip event listener if it exists
- Add new event listener
- Store cleanup function for next update
The code follows React best practices for event listener management and race condition prevention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tooltip-related event listeners that might need similar race condition fixes
rg -l "addEventListener.*tooltip" --type ts --type tsx
# Search for potential tooltip-related cleanup patterns
rg -l "removeEventListener.*tooltip" --type ts --type tsx
Length of output: 172
Script:
#!/bin/bash
# Search for tooltip-related event listeners with correct file type flags
rg -l "addEventListener.*tooltip" -t typescript -t tsx
# Search for tooltip-related cleanup patterns
rg -l "removeEventListener.*tooltip" -t typescript -t tsx
# Search for tooltip event handler references
rg -l "TooltipEventHan[dl]" -t typescript -t tsx
# Look for similar race condition prevention patterns
ast-grep --pattern 'useCallback($$$, [$$$])'
Length of output: 260223
Script:
#!/bin/bash
# Search for tooltip-related event listeners and handlers
rg -A 5 "tooltip.*event.*handler" -t js -t jsx -t ts -t tsx
# Search for tooltip-related race condition fixes
rg -A 5 "removeTooltipEventHan[dl].*current" -t js -t jsx -t ts -t tsx
# Search for tooltip-related cleanup patterns
rg -A 5 "removeEventListener.*tooltip" -t js -t jsx -t ts -t tsx
Length of output: 285
Script:
#!/bin/bash
# Search for tooltip event handlers and cleanup patterns
rg -l "tooltip.*event.*handler|removeTooltipEventHan[dl]|removeEventListener.*tooltip"
# Look for race condition prevention patterns in Modal.tsx
rg -A 5 "removeTooltipEventHanlderRef" app/packages/core/src/components/Modal/Modal.tsx
# Search for tooltip-related useCallback hooks
ast-grep --pattern 'useCallback($$$, [$$$])' app/packages/core/src/components/Modal/Modal.tsx
Length of output: 2901
🧰 Tools
🪛 Biome
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| return useCallback( | ||
| (looker: fos.Lookers) => { | ||
| looker.removeEventListener("tooltip", tooltipEventHandler); | ||
| looker.addEventListener("tooltip", tooltipEventHandler); | ||
|
|
||
| return () => { | ||
| looker.removeEventListener("tooltip", tooltipEventHandler); | ||
| }; | ||
| }, | ||
| [tooltipEventHandler] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing event listener cleanup pattern
The current implementation removes and adds the event listener on every call. This could be simplified to avoid unnecessary DOM operations.
Consider this alternative implementation:
return useCallback(
(looker: fos.Lookers) => {
- looker.removeEventListener("tooltip", tooltipEventHandler);
looker.addEventListener("tooltip", tooltipEventHandler);
return () => {
looker.removeEventListener("tooltip", tooltipEventHandler);
};
},
[tooltipEventHandler]
);This change:
- Removes the redundant initial
removeEventListenercall - Still maintains proper cleanup through the returned cleanup function
- Reduces unnecessary DOM operations
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return useCallback( | |
| (looker: fos.Lookers) => { | |
| looker.removeEventListener("tooltip", tooltipEventHandler); | |
| looker.addEventListener("tooltip", tooltipEventHandler); | |
| return () => { | |
| looker.removeEventListener("tooltip", tooltipEventHandler); | |
| }; | |
| }, | |
| [tooltipEventHandler] | |
| ); | |
| return useCallback( | |
| (looker: fos.Lookers) => { | |
| looker.addEventListener("tooltip", tooltipEventHandler); | |
| return () => { | |
| looker.removeEventListener("tooltip", tooltipEventHandler); | |
| }; | |
| }, | |
| [tooltipEventHandler] | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/core/src/components/Modal/Modal.tsx (1)
181-184: Fix typo in ref nameThe implementation is correct, but there's a typo in the ref name:
removeTooltipEventHanlderRefshould beremoveTooltipEventHandlerRef(missing 'd' in 'Handler').- const removeTooltipEventHanlderRef = useRef<ReturnType< + const removeTooltipEventHandlerRef = useRef<ReturnType<
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/core/src/components/Modal/Modal.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/components/Modal/Modal.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
app/packages/core/src/components/Modal/Modal.tsx (4)
4-4: LGTM: Import changes align with tooltip handling requirements
The addition of useTooltipEventHandler and reorganization of imports support the tooltip race condition fix.
Also applies to: 13-14
160-161: LGTM: Improved modal close handling with proper cleanup
The changes properly handle event listener cleanup and add a safeguard for the Escape key handling, which helps prevent race conditions.
186-195: LGTM: Robust tooltip event handling implementation
The implementation effectively prevents tooltip race conditions by:
- Cleaning up previous event listeners before adding new ones
- Properly managing cleanup functions with refs
- Using correct React hooks patterns with appropriate dependencies
221-221: LGTM: Improved prop naming
The rename from onNavigate to closePanels better describes the prop's purpose and maintains consistency with the broader refactoring.
What changes are proposed in this pull request?
Fixes a race condition relating to tooltip
How is this patch tested? If it is not, please explain why.
Smoke test locally.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit