overridable panel not found component #4522
Conversation
WalkthroughThis update introduces a new function Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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 (1)
app/packages/plugins/src/index.ts (1)
Line range hint
273-273: Type Definition IssuesThe use of
{}as a type is discouraged because it is equivalent toany non-nullish value. It is better to explicitly define the shape of objects when possible to enhance type safety and readability.- type PluginComponentProps<T> = T & { - panelNode?: unknown; - dimensions?: unknown; - }; + type PluginComponentProps<T> = T & { + panelNode?: any; // Consider defining a more specific type + dimensions?: any; // Consider defining a more specific type + };Also applies to: 398-398
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/packages/plugins/src/index.ts (1 hunks)
- app/packages/spaces/src/components/Panel.tsx (2 hunks)
- app/packages/spaces/src/components/PanelNotFound.tsx (1 hunks)
Additional context used
Path-based instructions (3)
app/packages/spaces/src/components/PanelNotFound.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/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/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.
Biome
app/packages/plugins/src/index.ts
[error] 66-66: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 273-273: Don't use '{}' as a type. (lint/complexity/noBannedTypes)
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
[error] 398-398: Don't use '{}' as a type. (lint/complexity/noBannedTypes)
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
Additional comments not posted (3)
app/packages/spaces/src/components/PanelNotFound.tsx (2)
5-17: Component Implementation Review:PanelNotFoundThe
PanelNotFoundcomponent correctly uses theusePluginComponenthook to attempt rendering a custom "Panel Not Found" component if one is registered. If not, it defaults to displaying a message. This implementation adheres to the PR's objective of making the "Panel Not Found" component overridable. The use of TypeScript for prop typing enhances type safety, which is a good practice.
18-18: Type Definition Review:PanelNotFoundPropsTypeThe type
PanelNotFoundPropsTypeis well-defined, extendingPanelPropswith an additionalpanelNamestring property. This is a clear and effective way to ensure all necessary props are passed to the component. Good use of TypeScript's intersection types to enhance component functionality while maintaining type safety.app/packages/spaces/src/components/Panel.tsx (1)
Line range hint
8-26: Refactoring and Enhancement ofPanelComponentThe refactoring of the
Panelcomponent to destructure its props enhances code readability and maintainability. The introduction of thePanelNotFoundcomponent to handle cases where a panel is not found is in line with the PR's objectives and improves user experience by providing a clear message when a panel cannot be found. The implementation correctly handles conditional rendering based on the panel loading state and the existence of the panel.
| /** | ||
| * A react hook that returns a component plugin by name if exist. | ||
| * @param name The name of the plugin | ||
| * @param ctx Argument passed to the plugin's activator function | ||
| * @returns The plugin component or `undefined` | ||
| */ | ||
| export function usePluginComponent(name: string, ctx?: unknown) { | ||
| const plugins = useActivePlugins(PluginComponentType.Component, ctx); | ||
| return plugins.find((p) => p.name === name); | ||
| } |
There was a problem hiding this comment.
New Hook Implementation: usePluginComponent
The usePluginComponent hook provides a mechanism to retrieve a component by name from the active plugins, which aligns with the PR's objectives to enhance the plugin system's flexibility. However, consider using optional chaining to handle potential null or undefined values gracefully when accessing properties.
- return plugins.find((p) => p.name === name);
+ return plugins.find((p) => p.name === name)?.component;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.
| /** | |
| * A react hook that returns a component plugin by name if exist. | |
| * @param name The name of the plugin | |
| * @param ctx Argument passed to the plugin's activator function | |
| * @returns The plugin component or `undefined` | |
| */ | |
| export function usePluginComponent(name: string, ctx?: unknown) { | |
| const plugins = useActivePlugins(PluginComponentType.Component, ctx); | |
| return plugins.find((p) => p.name === name); | |
| } | |
| /** | |
| * A react hook that returns a component plugin by name if exist. | |
| * @param name The name of the plugin | |
| * @param ctx Argument passed to the plugin's activator function | |
| * @returns The plugin component or `undefined` | |
| */ | |
| export function usePluginComponent(name: string, ctx?: unknown) { | |
| const plugins = useActivePlugins(PluginComponentType.Component, ctx); | |
| return plugins.find((p) => p.name === name)?.component; | |
| } |
| const CustomPanelNotFound = usePluginComponent("PanelNotFound")?.component; | ||
|
|
||
| if (CustomPanelNotFound) { | ||
| return <CustomPanelNotFound {...props} />; |
There was a problem hiding this comment.
Seems reasonable from a design POV to allow this to be over-ridden...but I'm not sure on the use case. What is the actual use case for this?
There was a problem hiding this comment.
The main use case atm is to enable displaying a more appropriate message in teams environment
What changes are proposed in this pull request?
Overridable panel not found component
How is this patch tested? If it is not, please explain why.
Using plugin component named
PanelNotFoundRelease Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
See above
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
usePluginComponentfunction to retrieve plugin components by name from active plugins.PanelNotFoundcomponent to handle scenarios where a panel is not found, displaying a custom message.Refactor
Panelcomponent to destructure props and utilize the newPanelNotFoundcomponent for better error handling.