Add PillBadgeView to Python Panels #4909
Conversation
WalkthroughThe changes introduce a new React component named Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 6
🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1)
11-22: LGTM: Implementation is solid, with a minor suggestion for improvement.The component implementation is well-structured and follows React best practices. The use of
getComponentPropsfor flexible styling is a good approach.One minor suggestion for improvement:
Consider adding default values or null checks for the properties destructured from
viewto handle cases where these properties might be undefined.Here's a suggested minor refactor:
const { text = '', color, variant, showIcon } = view; return ( <Box {...getComponentProps(props, "container")}> <PillBadge text={text} color={color} variant={variant} showIcon={showIcon} {...getComponentProps(props, "pillBadge")} /> </Box> );This ensures that
textwill always have a value, even if it's not provided in theviewobject. You might want to add similar defaults forcolor,variant, andshowIconbased on your specific requirements.e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts (1)
36-36: LGTM: Property name updated correctly.The change from
persisttopersistentappears to be a correct update to the dataset API usage. This ensures that the datasets created for testing are properly saved.Consider adding a brief comment explaining why persistence is necessary for these test datasets. This could improve code readability and maintainability.
+ # Ensure the dataset is saved persistently for the duration of the tests dataset.persistent = Trueapp/packages/core/src/plugins/SchemaIO/components/index.ts (1)
52-52: LGTM! The new PillBadgeView component is correctly exported.The export statement for
PillBadgeViewis correctly added and follows the same pattern as other exports in this file. This change aligns with the PR objectives to introduce the newPillBadgeViewcomponent.Consider maintaining alphabetical order for exports to improve readability and ease of maintenance. The
PillBadgeViewexport could be moved up a few lines to keep the list alphabetized.fiftyone/operators/types.py (1)
1478-1491: Implementation looks good, with a minor suggestion for improvement.The
PillBadgeViewclass is well-structured and inherits appropriately fromReadOnlyView. The docstring provides clear information about the class's purpose and parameters. However, to enhance clarity and maintainability, consider explicitly defining the class-specific attributes (text,color,variant,show_icon) in the__init__method, even if they're handled by the parent class.Example:
def __init__(self, text="Reviewed", color="primary", variant="outlined", show_icon=False, **kwargs): super().__init__(**kwargs) self.text = text self.color = color self.variant = variant self.show_icon = show_iconThis approach would make the class's specific properties more explicit and easier to understand for other developers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- app/packages/components/src/components/PillBadge/PillBadge.tsx (1 hunks)
- app/packages/components/src/components/PillBadge/index.ts (1 hunks)
- app/packages/core/src/components/Modal/use-looker.ts (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/index.ts (1 hunks)
- e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts (1 hunks)
- fiftyone/operators/types.py (1 hunks)
- fiftyone/utils/sam.py (1 hunks)
- fiftyone/utils/sam2.py (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/components/src/components/PillBadge/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/components/src/components/PillBadge/PillBadge.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/use-looker.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/core/src/plugins/SchemaIO/components/PillBadgeView.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/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.e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.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 (6)
app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1)
1-4: LGTM: Import statements are appropriate and concise.The import statements are well-organized and include all necessary dependencies for the component's functionality.
e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts (1)
Line range hint
1-78: Overall file structure and content look good.The test file is well-structured and follows good testing practices:
- It uses Playwright's test framework with custom extensions.
- Creates two datasets with different frame filtering configurations for comprehensive testing.
- Includes separate tests for asserting enabled and disabled frame filtering behaviors.
- Follows TypeScript best practices with appropriate typing.
app/packages/core/src/components/Modal/use-looker.ts (1)
59-59: LGTM. Verify consistency across the codebase.The change from
looker.updateSample(sample)tolooker.updateSample(sample.sample)looks correct and intentional. It suggests that theModalSampletype now has a nestedsampleproperty containing the actual sample data.To ensure consistency, let's verify the usage of
ModalSampleacross the codebase:This will help us confirm that the change is consistent with other parts of the codebase and identify any potential areas that might need similar updates.
✅ Verification successful
Verified: Change is consistent across the codebase.
The update from
looker.updateSample(sample)tolooker.updateSample(sample.sample)aligns with theModalSampletype structure and is consistently applied throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of ModalSample and updateSample method across the codebase # Search for ModalSample type usage echo "ModalSample type usage:" rg --type typescript "ModalSample" # Search for updateSample method calls echo "\nupdateSample method calls:" rg --type typescript "updateSample"Length of output: 264
Script:
#!/bin/bash # Description: Check the usage of ModalSample and updateSample method across the codebase # Search for ModalSample type usage echo "ModalSample type usage:" rg --type ts "ModalSample" # Search for updateSample method calls echo "\nupdateSample method calls:" rg --type ts "updateSample"Length of output: 9487
fiftyone/utils/sam.py (1)
274-286: Excellent addition of prediction scores!The inclusion of
scoresin the output dictionary enhances the model's output by providing confidence metrics for each prediction. This addition aligns well with best practices in machine learning, offering users valuable information about the reliability of each prediction.fiftyone/utils/sam2.py (1)
Line range hint
204-219: LGTM: Enhanced output with prediction scoresThe changes correctly implement the addition of scores to the output of the
_forward_pass_boxesmethod. This enhancement provides valuable confidence information for the predictions.To ensure these changes are fully integrated, please run the following script to check if the
scoresare being used elsewhere in the codebase:If the script doesn't return any results, consider updating other parts of the codebase to utilize these scores for improved functionality.
✅ Verification successful
Action Required: Re-verify Usage of 'scores' in SAM2 Predictions
The previous attempt to search for
scoresusage failed due to an unrecognized file type. Please run the updated script below to accurately locate any references toscoresrelated to SAM2 predictions:
Verification Completed: 'scores' Usage Confirmed
The
scoresvariable is correctly utilized withinfiftyone/utils/sam2.pyat lines 214-222. There are no additional references toscoreselsewhere in the codebase, ensuring that the integration is appropriate and isolated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of 'scores' in relation to SAM2 predictions # Search for potential uses of 'scores' in Python files rg --type python -g '!test_*' -g '!*_test.py' -i '(sam2.*scores|scores.*sam2)' # Search for any modifications to the output structure of SAM2 predictions rg --type python -g '!test_*' -g '!*_test.py' -i '(sam2.*output|output.*sam2)'Length of output: 223
Script:
# #!/bin/bash # Description: Check for usage of 'scores' in relation to SAM2 predictions # Find all Python files excluding tests and search for 'scores' usage fd --extension py --exclude 'test_*' --exclude '*_test.py' | xargs rg -i '(sam2.*scores|scores.*sam2)' # Alternatively, using rg to search within .py files directly rg -t py -g '!test_*' -g '!*_test.py' -i '(sam2.*scores|scores.*sam2)'Length of output: 501
app/packages/components/src/components/PillBadge/PillBadge.tsx (1)
51-71: Component correctly handles stringtextprop.The rendering logic for when
textis a string is appropriately implemented and follows best practices.
| export default function PillBadgeView(props) { | ||
| const { schema } = props; | ||
| const { view = {} } = schema; | ||
| const { text, color, variant, showIcon } = view; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding TypeScript types and using a named export.
While the component structure is good, it could benefit from the following improvements:
- Add TypeScript type annotations for props and destructured values to enhance type safety and developer experience.
- Consider using a named export instead of a default export for better refactoring capabilities and explicit imports.
Here's a suggested refactor:
import { Box } from "@mui/material";
import React from "react";
import { getComponentProps } from "../utils";
import PillBadge from "@fiftyone/components/src/components/PillBadge/PillBadge";
interface PillBadgeViewProps {
schema: {
view?: {
text?: string;
color?: string;
variant?: string;
showIcon?: boolean;
};
};
}
export function PillBadgeView({ schema }: PillBadgeViewProps) {
const { view = {} } = schema;
const { text, color, variant, showIcon } = view;
// ... rest of the component
}| const PillBadge = ({ | ||
| text, | ||
| color = "default", | ||
| variant = "filled", | ||
| showIcon = true, | ||
| }: { | ||
| text: string | string[] | [string, string][]; | ||
| color?: string; | ||
| variant?: "filled" | "outlined"; | ||
| showIcon?: boolean; | ||
| }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the text prop to improve maintainability.
The text prop accepts multiple types (string, string[], and [string, string][]), which increases complexity in the component logic and can lead to potential errors. Simplifying the prop to accept a single data type or splitting the component into separate components for each case could enhance readability and maintainability.
| onChange={(event) => { | ||
| const selectedText = text.find( | ||
| (t) => t[0] === event.target.value | ||
| ); | ||
| setChipSelection(event.target.value); | ||
| setChipColor(selectedText?.[1] ?? "default"); | ||
| }} |
There was a problem hiding this comment.
Add checks for selectedText to prevent undefined access.
In the onChange handler, selectedText may be undefined if text.find does not find a matching element. Accessing selectedText[1] without checking if selectedText exists could lead to runtime errors. Consider adding a check to ensure selectedText is defined before using it.
Here's an example of how to adjust the code:
onChange={(event) => {
const selectedText = text.find(
(t) => t[0] === event.target.value
);
setChipSelection(event.target.value);
- setChipColor(selectedText?.[1] ?? "default");
+ if (selectedText) {
+ setChipColor(selectedText[1]);
+ } else {
+ setChipColor("default");
+ }
}}📝 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.
| onChange={(event) => { | |
| const selectedText = text.find( | |
| (t) => t[0] === event.target.value | |
| ); | |
| setChipSelection(event.target.value); | |
| setChipColor(selectedText?.[1] ?? "default"); | |
| }} | |
| onChange={(event) => { | |
| const selectedText = text.find( | |
| (t) => t[0] === event.target.value | |
| ); | |
| setChipSelection(event.target.value); | |
| if (selectedText) { | |
| setChipColor(selectedText[1]); | |
| } else { | |
| setChipColor("default"); | |
| } | |
| }} |
| const COLORS: { [key: string]: string } = { | ||
| default: "#999999", | ||
| primary: "#FFB682", | ||
| error: "error", | ||
| warning: "warning", | ||
| info: "info", | ||
| success: "#8BC18D", | ||
| }; |
There was a problem hiding this comment.
Ensure COLORS object contains valid CSS color values.
In the COLORS object, the values for 'error', 'warning', and 'info' are strings that may not correspond to valid CSS color values. Assigning these strings to the color style property may not produce the desired effect. Consider replacing them with appropriate color codes or ensuring that the component interprets these string values correctly.
For example, you can update the COLORS object with valid color codes:
const COLORS: { [key: string]: string } = {
default: "#999999",
primary: "#FFB682",
- error: "error",
- warning: "warning",
- info: "info",
+ error: "#f44336",
+ warning: "#ff9800",
+ info: "#2196f3",
success: "#8BC18D",
};📝 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.
| const COLORS: { [key: string]: string } = { | |
| default: "#999999", | |
| primary: "#FFB682", | |
| error: "error", | |
| warning: "warning", | |
| info: "info", | |
| success: "#8BC18D", | |
| }; | |
| const COLORS: { [key: string]: string } = { | |
| default: "#999999", | |
| primary: "#FFB682", | |
| error: "#f44336", | |
| warning: "#ff9800", | |
| info: "#2196f3", | |
| success: "#8BC18D", | |
| }; |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Adds a
PillBadgeViewto Python Panels which is a MuiChip component capable of rendering itself as a dropdown menu of options, color-coded or not (if desired). The indicator icon is also optional and can be enabled or removed with ashowIconflagScreen.Recording.2024-10-10.at.12.54.39.AM.mov
Screen.Recording.2024-10-10.at.12.37.11.AM.mov
Summary by CodeRabbit
New Features
PillBadgecomponent for displaying customizable pill-shaped badges.PillBadgeViewcomponent to renderPillBadgewithin a structured layout.Bug Fixes
Documentation