add modal context to panel and operator views & fix zIndex of ArrowNavView #5052
Conversation
WalkthroughThe changes introduce a new parameter Changes
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: 2
🧹 Outside diff range and nitpick comments (10)
app/packages/core/src/plugins/OperatorIO/index.tsx (2)
Line range hint
6-20: Add TypeScript type definitions for better type safety.Since this is a TypeScript file, the component and its props should be properly typed. This will provide better type safety and documentation.
+interface OperatorIOProps { + schema: any; // Replace 'any' with actual schema type + onChange: (data: any) => void; + type: 'input' | 'output'; + data: any; + errors?: any; + layout?: any; + onPathChange?: (path: string) => void; + initialData?: any; + id: string; + shouldClearUseKeyStores?: boolean; + isModalPanel?: boolean; + [key: string]: any; +} -function OperatorIOComponent(props) { +function OperatorIOComponent(props: OperatorIOProps) {
Line range hint
23-34: LGTM! Consider destructuring otherProps for clarity.The implementation correctly forwards additional props to SchemaIOComponent, which aligns with the PR's objective of supporting modal context. However, for better clarity, consider destructuring the specific props you're interested in.
- otherProps={otherProps} + otherProps={{ isModalPanel: otherProps.isModalPanel, ...otherProps }}app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
66-66: Add JSDoc documentation for theotherPropsproperty.Similar to how other experimental features are documented (like
relativeLayout), please add JSDoc comments explaining the purpose and usage ofotherProps, particularly mentioning its role in modal context handling.+ /** + * Additional properties passed to the view component. + * Currently used for modal context via isModalPanel. + */ otherProps: { [key: string]: any };app/packages/spaces/src/components/Panel.tsx (3)
Line range hint
15-17: Update TypeScript type definitions for proper type safetyThe
isModalPanelprop is being destructured but isn't properly typed in the component's props interface. This could lead to type-checking issues.Update the type definition:
- function Panel(props: PanelProps) { + function Panel(props: PanelProps & { isModalPanel?: boolean }) {
Line range hint
41-62: Add JSDoc comments to document modal behaviorThe modal-specific logic is well-implemented, but adding documentation would improve maintainability and developer experience.
Add JSDoc comments to explain the modal behavior:
+ /** + * Renders the panel content with modal context support. + * When isModalPanel is true: + * - Component is re-mounted on navigation if panelOptions.reloadOnNavigation is true + * - Specific modal styling is applied + * @param {boolean} isModalPanel - Whether the panel is rendered in a modal context + */ return ( <StyledPanel $isModalPanel={isModalPanel}
Line range hint
69-71: Enhance React.memo comparison functionThe current memo comparison only checks
node.id, butisModalPanelalso affects rendering. This could lead to unnecessary re-renders.Update the memo comparison:
export default React.memo(Panel, (prevProps, nextProps) => { - return prevProps?.node?.id === nextProps?.node?.id; + return prevProps?.node?.id === nextProps?.node?.id && + prevProps?.isModalPanel === nextProps?.isModalPanel; });app/packages/core/src/plugins/SchemaIO/components/ArrowNavView.tsx (1)
23-24: Optimize style composition with useMemo.The style objects are being recreated on every render. Consider memoizing the computed styles for better performance.
Apply this optimization:
+import { useMemo } from 'react'; + export default function ArrowNavView(props: ArrowNavViewProps) { // ... - const backwardStyles = positionBasedStyleBackward[position] || {}; - const forwardStyles = positionBasedStyleForward[position] || {}; + const { backwardStyles, forwardStyles } = useMemo(() => ({ + backwardStyles: { + ...(positionBasedStyleBackward[position] || {}), + zIndex + }, + forwardStyles: { + ...(positionBasedStyleForward[position] || {}), + zIndex + } + }), [position, zIndex]); return ( <> {backward && ( <Arrow - style={{ ...backwardStyles, zIndex }} + style={backwardStyles} // ... /> )} {forward && ( <Arrow $isRight - style={{ ...forwardStyles, zIndex }} + style={forwardStyles} // ... /> )} </> );Also applies to: 32-32, 45-45
app/packages/operators/src/CustomPanel.tsx (2)
19-19: Consider adding runtime type validation for isModalPanel prop.While the TypeScript interface ensures type safety at compile time, consider adding runtime validation for the
isModalPanelprop to prevent potential issues with undefined or non-boolean values.- const { panelId, dimensions, panelName, panelLabel, isModalPanel } = props; + const { panelId, dimensions, panelName, panelLabel, isModalPanel = false } = props;
126-149: Add JSDoc documentation for the isModalPanel prop.The implementation looks good, but consider adding documentation to explain the purpose and usage of the
isModalPanelprop for better maintainability.export function defineCustomPanel({ on_load, on_change, on_unload, on_change_ctx, on_change_view, on_change_dataset, on_change_current_sample, on_change_selected, on_change_selected_labels, on_change_extended_selection, on_change_group_slice, on_change_spaces, panel_name, panel_label, }) { + /** + * @param props - Component props + * @param props.isModalPanel - Indicates if the panel is rendered in a modal context + * @returns React component with modal context awareness + */ return (props) => {app/packages/plugins/src/index.ts (1)
385-385: LGTM! Consider adding JSDoc documentation.The addition of the optional
isModalPanelproperty toPluginComponentPropsis well-typed and backward compatible. However, adding JSDoc documentation would help developers understand its purpose and usage.Consider adding documentation like this:
type PluginComponentProps<T> = T & { panelNode?: unknown; dimensions?: unknown; + /** Indicates whether the component is rendered within a modal panel */ isModalPanel?: boolean; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/packages/core/src/plugins/OperatorIO/index.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/components/ArrowNavView.tsx(3 hunks)app/packages/core/src/plugins/SchemaIO/utils/types.ts(1 hunks)app/packages/operators/src/CustomPanel.tsx(3 hunks)app/packages/operators/src/useCustomPanelHooks.ts(1 hunks)app/packages/plugins/src/index.ts(1 hunks)app/packages/spaces/src/components/Panel.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
app/packages/core/src/plugins/OperatorIO/index.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/plugins/SchemaIO/components/ArrowNavView.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/plugins/SchemaIO/utils/types.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/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.
app/packages/operators/src/useCustomPanelHooks.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/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.
🔇 Additional comments (5)
app/packages/spaces/src/components/Panel.tsx (1)
61-63: LGTM: Clean prop forwarding implementation
The explicit forwarding of isModalPanel to the child component follows React best practices and maintains clear data flow.
app/packages/core/src/plugins/SchemaIO/components/ArrowNavView.tsx (1)
25-26: Verify zIndex values don't conflict with other components.
The zIndex values (1000 and 1501) should be checked against other components to ensure proper layering in the application.
app/packages/operators/src/CustomPanel.tsx (1)
92-92: LGTM! Prop forwarding implemented correctly.
The isModalPanel prop is properly passed down to the OperatorIO component, following React's props drilling pattern.
app/packages/operators/src/useCustomPanelHooks.ts (2)
34-34: LGTM: Clean interface extension.
The addition of the optional isModalPanel property is well-typed and maintains backward compatibility.
34-34: Add documentation and verify modal-specific behavior.
The isModalPanel property is added but:
- Its purpose and usage are not documented
- It might need special handling in modal contexts
Consider:
- Adding JSDoc comments explaining the property's purpose and expected behavior
- Verifying if modal panels require specific state management or event handling
Let's verify the property's usage across the codebase:
✅ Verification successful
Let me try a different search strategy to verify the usage of this property.
The isModalPanel property is correctly used and serves a clear purpose
The property is actively used throughout the codebase to:
- Control z-index values for proper modal layering (ArrowNavView.tsx)
- Determine panel scope and reload behavior (Panel.tsx)
- Apply modal-specific styling via styled components
- Handle modal-specific state management
Key usages found in:
app/packages/core/src/plugins/SchemaIO/components/ArrowNavView.tsx: Controls z-index layeringapp/packages/spaces/src/components/Panel.tsx: Manages modal-specific panel behavior and stylingapp/packages/operators/src/CustomPanel.tsx: Propagates the modal state to child components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how isModalPanel is used across the codebase
# Search for isModalPanel usage
echo "Searching for isModalPanel usage..."
rg "isModalPanel" -t ts -t tsx
# Search for modal-related panel components
echo "Searching for modal-related panel components..."
ast-grep --pattern 'interface $_ extends CustomPanelProps {
$$$
}'
Length of output: 315
Script:
#!/bin/bash
# Search for isModalPanel with ripgrep including TypeScript files
echo "Searching for isModalPanel in TypeScript files..."
rg "isModalPanel" --type-add 'ts:*.{ts,tsx}' -t ts
# Search for modal-related components and interfaces
echo "Searching for modal and panel related components..."
rg "Modal.*Panel|Panel.*Modal" --type-add 'ts:*.{ts,tsx}' -t ts -A 3
# Look for CustomPanel related interfaces and types
echo "Searching for CustomPanel related types..."
rg "CustomPanel" --type-add 'ts:*.{ts,tsx}' -t ts -A 3
Length of output: 16596
| @@ -63,6 +63,7 @@ export type ViewPropsType<Schema extends SchemaType = SchemaType> = { | |||
| ROWS: number; | |||
| }; | |||
| autoFocused?: React.MutableRefObject<boolean>; | |||
| otherProps: { [key: string]: any }; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for otherProps.
Instead of using a loose index signature with any, consider creating a specific interface for the known additional properties. This will provide better type safety and IDE support.
- otherProps: { [key: string]: any };
+ otherProps: {
+ isModalPanel?: boolean;
+ // Add other known properties here
+ };Committable suggestion skipped: line range outside the PR's diff.
| @@ -9,7 +9,7 @@ import React from "react"; | |||
| import { ViewPropsType } from "../utils/types"; | |||
|
|
|||
| export default function ArrowNavView(props: ViewPropsType) { | |||
| const { schema } = props; | |||
| const { schema, otherProps } = props; | |||
There was a problem hiding this comment.
Update type definitions for better type safety.
The component's props type definition doesn't match its usage. The otherProps is implicitly typed as any, which bypasses TypeScript's type checking benefits.
Apply this diff to improve type safety:
-export default function ArrowNavView(props: ViewPropsType) {
+interface ArrowNavViewProps extends ViewPropsType {
+ otherProps: {
+ isModalPanel?: boolean;
+ };
+}
+
+export default function ArrowNavView(props: ArrowNavViewProps) {Also, consider extracting the zIndex values as named constants:
+const Z_INDEX = {
+ DEFAULT: 1000,
+ MODAL: 1501,
+} as const;
+
export default function ArrowNavView(props: ArrowNavViewProps) {
// ...
- const zIndex = isModalPanel ? 1501 : 1000;
+ const zIndex = isModalPanel ? Z_INDEX.MODAL : Z_INDEX.DEFAULT;Also applies to: 25-26
What changes are proposed in this pull request?
props.isModalPanelprops.otherProps.isModalPanelHow is this patch tested? If it is not, please explain why.
Using ArrowNavView
Release 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
isModalPanelproperty to various components, enhancing modal state management.otherPropsto components for greater flexibility in passing additional properties.Bug Fixes
Refactor
Chores