add reload_on_navigation for modal panels #4846
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
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
🧹 Outside diff range and nitpick comments (4)
fiftyone/operators/panel.py (2)
49-49: LGTM: New parameter added correctly.The
reload_on_navigationparameter has been added to the constructor with a default value ofFalse, which is good for backward compatibility.Consider adding type hinting for the
reload_on_navigationparameter:- reload_on_navigation=False, + reload_on_navigation: bool = False,This would improve code readability and help with static type checking.
77-77: LGTM: Consistent updates to serialization and startup configuration.The
reload_on_navigationparameter has been correctly added to both theto_jsonmethod ofPanelConfigand thepanel_configdictionary in theon_startupmethod ofPanel. This ensures that the new configuration option is properly serialized and passed to the panel during startup.Consider adding a comment in the
on_startupmethod to explain the purpose of thereload_on_navigationoption:# Add reload_on_navigation to allow dynamic reloading of non-modal panels "reload_on_navigation": self.config.reload_on_navigation,This would help other developers understand the purpose of this configuration option.
Also applies to: 117-117
app/packages/plugins/src/index.ts (1)
328-333: LGTM! Consider enhancing the documentation slightly.The addition of the
reloadOnNavigationproperty is well-implemented and properly documented. It's an optional boolean property that allows for more dynamic behavior of modal plugins.Consider slightly expanding the documentation to provide more context:
/** * If true, the plugin will be remounted when the user navigates to a different sample or group. * This is only applicable to plugins that are displayed in a modal. * Use this when the plugin needs to reset its state or re-fetch data on navigation. */ reloadOnNavigation?: boolean;This additional information helps developers understand when they might want to use this property.
fiftyone/operators/operations.py (1)
308-308: LGTM! Consider a minor docstring improvement.The implementation of the new
reload_on_navigationparameter looks good. It's correctly added to the method signature and theparamsdictionary, with a sensible default value ofFalseto maintain backward compatibility.The docstring provides clear information about the parameter's purpose and limitations. However, to enhance clarity, consider slightly rewording the docstring:
- reload_on_navigation (False): whether to reload the panel when the - user navigates to a new page. This is only applicable to panels - that are not shown in a modal + reload_on_navigation (False): whether to reload the panel when the + user navigates between samples. This is only applicable to panels + that are not displayed in a modalThis minor change aligns the docstring more closely with the PR description, which mentions navigation between samples rather than pages.
Also applies to: 335-337, 367-367
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- app/packages/operators/src/Panel/register.tsx (1 hunks)
- app/packages/plugins/src/index.ts (1 hunks)
- app/packages/spaces/src/components/Panel.tsx (4 hunks)
- app/packages/state/src/recoil/groups.ts (1 hunks)
- fiftyone/operators/operations.py (3 hunks)
- fiftyone/operators/panel.py (5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/operators/src/Panel/register.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/plugins/src/index.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.app/packages/spaces/src/components/Panel.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/state/src/recoil/groups.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.
🔇 Additional comments not posted (10)
app/packages/operators/src/Panel/register.tsx (1)
26-26: LGTM! The change aligns well with the PR objectives.The addition of the
reloadOnNavigationproperty topanelOptionsis a well-implemented feature that directly addresses the PR's goal of allowing forced remounting of modal panels during navigation between samples. The implementation is clean, focused, and uses the execution context appropriately.fiftyone/operators/panel.py (3)
31-36: LGTM: Clear and concise documentation for new parameters.The additions to the class docstring for
PanelConfigare well-written and accurately describe the newreload_on_navigationandhelp_markdownparameters. This documentation will help developers understand the purpose and usage of these new features.
62-62: LGTM: Attribute correctly set in the initializer.The
self.reload_on_navigationattribute is properly set to the value of thereload_on_navigationparameter in the__init__method. This ensures that the new configuration option is stored in the instance.
Line range hint
31-117: Summary: Well-implemented feature for panel reloading on navigation.The changes introduce a new
reload_on_navigationoption for panel configurations. This feature allows non-modal panels to be reloaded when the user navigates to a new page, potentially improving the user experience by ensuring that panel content stays up-to-date.Key points:
- The feature is well-documented in the class docstring.
- The implementation is consistent across the
PanelConfigandPanelclasses.- The changes maintain backward compatibility with existing code.
Overall, this is a solid implementation that enhances the flexibility of the panel system. Good job!
app/packages/state/src/recoil/groups.ts (1)
392-392: Approve the change with a suggestion for further investigation.The introduction of optional chaining (
?.) is a good improvement. It makes the code more robust by safely handling cases whereget(modalSelector)might returnnullorundefined.However, it would be beneficial to investigate why
modalSelectorcould potentially returnnullorundefined. This change suggests a possible edge case that wasn't handled before. Consider adding a comment explaining this scenario or updating themodalSelectordocumentation if necessary.To verify the
modalSelectorimplementation and its potential to returnnullorundefined, you can run the following script:This script will help us understand the implementation of
modalSelectorand how it's used throughout the codebase, which could provide insights into scenarios where it might returnnullorundefined.✅ Verification successful
Verified: Optional chaining in
groupIdselector is appropriateThe
modalSelectorcan indeed returnnull, making the use of optional chaining (?.) in thegroupIdselector correct and necessary to prevent potential runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Investigate the implementation of modalSelector # Search for the modalSelector definition echo "modalSelector definition:" rg --type typescript "const\s+modalSelector\s*=\s*selector" -A 10 # Search for places where modalSelector is used echo "\nUsages of modalSelector:" rg --type typescript "modalSelector" -C 2Length of output: 303
Script:
#!/bin/bash # Description: Investigate the implementation of modalSelector # Search for the modalSelector definition echo "modalSelector definition:" rg "const\s+modalSelector\s*=\s*selector" --glob "*.ts" --glob "*.tsx" -A 10 # Search for places where modalSelector is used echo "\nUsages of modalSelector:" rg "modalSelector" --glob "*.ts" --glob "*.tsx" -C 2Length of output: 15716
Script:
#!/bin/bash # Description: Investigate the implementation of modalSelector # Search for the modalSelector definition echo "modalSelector definition:" rg "const\s+modalSelector\s*=\s*sessionAtom" --glob "*.ts" --glob "*.tsx" -A 10 # Search for places where modalSelector is used echo "\nUsages of modalSelector:" rg "modalSelector" --glob "*.ts" --glob "*.tsx" -C 2Length of output: 16396
app/packages/plugins/src/index.ts (1)
328-333: Verify the impact on existing plugins and usage.The addition of the
reloadOnNavigationproperty is well-isolated and shouldn't break existing functionality. However, it's important to ensure that this new feature is properly integrated into the plugin system.To verify the integration and usage of this new property, please run the following checks:
These checks will help ensure that the new property is properly integrated and identify any areas that might need additional attention.
✅ Verification successful
Impact of adding
reloadOnNavigationon existing plugins and usage verified.No issues found. The new property is properly integrated and does not break existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing usage of reloadOnNavigation and verify plugin system integration # Check for any existing usage of reloadOnNavigation in the codebase echo "Checking for existing usage of reloadOnNavigation:" rg --type typescript 'reloadOnNavigation' # Check the plugin system implementation for handling of this new property echo "Checking plugin system implementation:" rg --type typescript 'reloadOnNavigation' app/packages/plugins/src # Verify if there are any TODOs or FIXMEs related to this feature echo "Checking for related TODOs or FIXMEs:" rg --type typescript -e 'TODO.*reload' -e 'FIXME.*reload' app/packages/plugins/srcLength of output: 581
Script:
#!/bin/bash # Description: Check for existing usage of reloadOnNavigation and verify plugin system integration by searching .ts and .tsx files # Check for any existing usage of reloadOnNavigation in the codebase echo "Checking for existing usage of reloadOnNavigation in .ts and .tsx files:" rg 'reloadOnNavigation' --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx # Check the plugin system implementation for handling of this new property echo "Checking plugin system implementation for reloadOnNavigation in app/packages/plugins/src:" rg 'reloadOnNavigation' --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx app/packages/plugins/src # Verify if there are any TODOs or FIXMEs related to this feature echo "Checking for related TODOs or FIXMEs in .ts and .tsx files within app/packages/plugins/src:" rg -e 'TODO.*reload' -e 'FIXME.*reload' --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx app/packages/plugins/srcLength of output: 1027
Script:
#!/bin/bash # Description: Check for existing usage of reloadOnNavigation and verify plugin system integration by searching .ts and .tsx files # Check for any existing usage of reloadOnNavigation in .ts and .tsx files echo "Checking for existing usage of reloadOnNavigation in .ts and .tsx files:" rg 'reloadOnNavigation' -g '*.ts' -g '*.tsx' # Check the plugin system implementation for handling of this new property echo "Checking plugin system implementation for reloadOnNavigation in app/packages/plugins/src:" rg 'reloadOnNavigation' -g '*.ts' -g '*.tsx' app/packages/plugins/src # Verify if there are any TODOs or FIXMEs related to this feature echo "Checking for related TODOs or FIXMEs in .ts and .tsx files within app/packages/plugins/src:" rg -e 'TODO.*reload' -e 'FIXME.*reload' -g '*.ts' -g '*.tsx' app/packages/plugins/srcLength of output: 1100
app/packages/spaces/src/components/Panel.tsx (4)
23-24: Approved: Correct retrieval ofthisModalUniqueIdThe
thisModalUniqueIdis properly obtained usinguseRecoilValue, ensuring that the component responds to changes in the current modal's unique ID.
45-47: Approved: Proper destructuring ofpanelto includepanelOptionsIncluding
panelOptionsin the destructuring ofpanelallows for conditional logic based on the panel's options, such asreloadOnNavigation.
47-47: Approved: Correctly determining if the component should be re-keyedThe
shouldKeyComponentvariable accurately reflects whether the component should force a remount based onisModalPanelandpanelOptions?.reloadOnNavigation.
58-62: Approved: Proper use of thekeyprop to control component remountingUsing the
keyprop with the value{shouldKeyComponent ? thisModalUniqueId : panelName}effectively forces the component to remount when necessary, aligning with the PR's objective to reload modal panels on navigation.
Makes it possible to force remounting of panels when navigating between samples
Summary by CodeRabbit
New Features
reloadOnNavigationoption for panel behavior during navigation.help_markdownparameter.Bug Fixes
Documentation