Fixes an issue where splitting the panel space wrongly triggers on_unload #5296
Conversation
WalkthroughThe pull request introduces changes to the Changes
Sequence DiagramsequenceDiagram
participant CustomPanel
participant usePanelEvent
participant PanelHooks
CustomPanel->>usePanelEvent: Initialize triggerPanelEvent
CustomPanel->>PanelHooks: Call onUnLoad
PanelHooks-->>CustomPanel: Handle panel state changes
CustomPanel->>usePanelEvent: Trigger panel unload event
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (1)
app/packages/operators/src/CustomPanel.tsx (1)
40-42: Consider adding documentation for the unload behaviorWhile the implementation is correct, it would be beneficial to add a comment explaining why the
on_unloadtrigger needs to be in the panel close effect specifically, to prevent future regressions.setPanelCloseEffect(() => { clearUseKeyStores(panelId); trackEvent("close_panel", { panel: panelName }); + // Trigger on_unload only during actual panel closure to prevent incorrect triggers during panel splits triggerPanelEvent(panelId, { operator: props.onUnLoad }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/operators/src/CustomPanel.tsx(2 hunks)app/packages/operators/src/useCustomPanelHooks.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/packages/operators/src/useCustomPanelHooks.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/CustomPanel.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/operators/src/CustomPanel.tsx (4)
17-17: LGTM: Clean import of the new hook
The import follows React hook naming conventions and is properly organized with other hook imports.
24-24: LGTM: Proper hook initialization
The hook is correctly initialized at the component's top level, following React hooks rules.
Line range hint 115-166: LGTM: Consistent parameter and prop handling
The on_unload parameter is properly integrated into the function signature and correctly passed as onUnLoad prop, maintaining consistency with React naming conventions.
40-42: Verify the unload event triggering sequence
The change fixes the incorrect on_unload triggering by moving it to the panel close effect. However, we should verify that this doesn't introduce any race conditions with the store clearing operation.
✅ Verification successful
No race conditions found in panel unload sequence
The code analysis shows that clearUseKeyStores is properly synchronized with the panel close effect. The sequence is:
clearUseKeyStoresis called first to clean up the storestrackEventis called to log the panel closuretriggerPanelEventwithonUnLoadis called last
This order ensures that store cleanup happens before any unload handlers are triggered, preventing potential race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential race conditions in panel unload handling
ast-grep --pattern 'useEffect(() => {
$$$
clearUseKeyStores($_)
$$$
})'
Length of output: 79
Script:
#!/bin/bash
# Search for clearUseKeyStores usage and its surrounding context
rg "clearUseKeyStores" -A 5 -B 5
# Search for panel unload related effects
ast-grep --pattern 'useEffect(() => {
$$$
onUnload($$$)
$$$
})'
# Search for panel close related code
rg "close_panel|onUnload|triggerPanelEvent" -A 3 -B 3
Length of output: 18791
manivoxel51
changed the title
I searched for on_unload in teams and oss but didn't find much reference - DQ panel is the only one implementing that from the recent CustomPanels. also made sure all built-in panels (DQ, QP, Lens, etc) can be opened and closed properly after the change. @imanjra @Br2850 if there are other areas I am missing to test lmk. |
What changes are proposed in this pull request?
PanelCloseEffectfunction fixes the issueScreen.Recording.2024-12-18.at.1.10.42.PM.mov
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
onUnLoadprop, streamlining lifecycle interactions.Refactor