additional panel meta (is_new, beta, category) #5038
Conversation
WalkthroughThe changes in this pull request involve modifications to various components and files within the application, primarily focusing on the addition of categorization properties and enhancements to panel registration. Key updates include the introduction of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
ritch
changed the title
![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 (16)
fiftyone/operators/categories.py (1)
4-8: Add documentation and type hints to improve API usability.The Categories enum is well-structured, but could benefit from additional documentation and type safety.
Consider applying these improvements:
+from enum import Enum, unique +from typing import Literal - - +CategoryType = Literal["import", "curate", "analyze", "custom"] + +@unique class Categories(Enum): + """Categorization options for panels. + + Attributes: + IMPORT: Panels related to data import functionality + CURATE: Panels for data curation and management + ANALYZE: Panels for data analysis and visualization + CUSTOM: Custom or user-defined panels + """ IMPORT = "import" CURATE = "curate" ANALYZE = "analyze" CUSTOM = "custom" + + @classmethod + def to_value(cls, category: "Categories") -> CategoryType: + """Convert enum member to its string value.""" + return category.valuefiftyone/operators/__init__.py (1)
20-22: LGTM! Consider adding docstring for new imports.The new imports align well with the PR objectives of adding metadata attributes for panels. The imports are correctly placed and follow the existing style.
Consider adding a brief docstring above these imports to document their purpose:
from .utils import ProgressHandler, is_new from .panel import Panel, PanelConfig +# Categories enum provides panel categorization options from .categories import Categories🧰 Tools
🪛 Ruff
20-20:
.utils.ProgressHandlerimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
20-20:
.utils.is_newimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
21-21:
.panel.Panelimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
21-21:
.panel.PanelConfigimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
22-22:
.categories.Categoriesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
app/packages/embeddings/src/index.ts (1)
The
isNewandbetaproperties should be added to the panel configurationBased on the codebase analysis, these properties are part of the standard panel configuration as seen in the
PanelOptionstype definition inplugins/src/index.tsand their usage inoperators/src/Panel/register.tsx. They are required metadata properties that help users identify new and beta features.panelOptions: { TabIndicator: EmbeddingsTabIndicator, priority: BUILT_IN_PANEL_PRIORITY_CONST, category: Categories.Curate, + isNew: false, + beta: false, },🔗 Analysis chain
Line range hint
10-22: Consider addingisNewandbetametadata properties.According to the PR objectives, this change introduces three new panel metadata attributes:
is_new,beta, andcategory. While thecategoryproperty has been correctly implemented, the other two properties are missing from this panel's configuration.Let's verify if other panels are using these properties:
Consider adding the missing properties if they apply to this panel:
panelOptions: { TabIndicator: EmbeddingsTabIndicator, priority: BUILT_IN_PANEL_PRIORITY_CONST, category: Categories.Curate, + isNew: false, // or true if this is a new panel + beta: false, // or true if this is in beta },
Let me gather more information about the panel registration pattern and metadata properties.
Let me try a different search pattern to find panel registrations.
Let me check the content of these specific files to understand how these properties are used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any other panels are using isNew or beta properties # Expected: Find other panel configurations using these properties for consistency # Search for panel registrations with isNew or beta rg -A 10 'panelOptions.*\{' | rg -B 10 '(isNew|beta)'Length of output: 1086
Script:
#!/bin/bash # Search for all panel registrations to understand the pattern rg -B 2 -A 10 'registerComponent\(\{.*type:\s*PluginComponentType\.Panel'Length of output: 75
Script:
#!/bin/bash # Search for panel registrations with a more flexible pattern rg -B 2 -A 10 'registerComponent.*Panel' # Also search specifically for the pattern of setting these properties rg -l 'isNew:|beta:' app/packages/Length of output: 159
Script:
#!/bin/bash # Check the content of the files that contain these properties cat app/packages/plugins/src/index.ts cat app/packages/operators/src/Panel/register.tsxLength of output: 15792
app/packages/spaces/src/components/PanelIcon.tsx (1)
14-21: Consider using MUI theme spacing and flexible dimensionsWhile the implementation works, there are some improvements that could make it more maintainable and consistent with MUI best practices:
- Use MUI's spacing theme values instead of hard-coded rem units
- Consider making dimensions more flexible or theme-based
- Consider using Stack or Grid for layout instead of manual margins
Here's a suggested improvement:
- <Box sx={{ mr: "0.75rem", width: "1rem", height: "1.5rem" }}> - <PanelTabIcon - style={{ - width: "1rem", - height: "1rem", - }} - /> - </Box> + <Box + sx={(theme) => ({ + mr: theme.spacing(1), + display: 'flex', + alignItems: 'center', + width: theme.spacing(2), + height: theme.spacing(3) + })} + > + <PanelTabIcon + sx={(theme) => ({ + width: theme.spacing(2), + height: theme.spacing(2) + })} + /> + </Box>This change:
- Uses MUI's theme spacing for consistent spacing across the app
- Makes dimensions theme-dependent for better maintainability
- Uses proper flex layout for alignment
app/packages/operators/src/Panel/register.tsx (1)
28-31: Consider adding TypeScript type definitions for the new properties.The new metadata properties look good and align with the PR objectives. However, to improve type safety and maintainability, consider defining TypeScript interfaces for these properties:
interface PanelMetadata { beta?: boolean; category?: string; isNew?: boolean; priority?: number; } interface PanelParams extends PanelMetadata { // ... existing params }This will provide better type checking and IDE support.
app/packages/spaces/src/types.ts (1)
17-18: LGTM! Consider adding JSDoc comments.The new optional boolean properties follow TypeScript naming conventions and align with their UI rendering purpose.
Consider adding JSDoc comments to improve documentation:
export type AddPanelItemProps = { node: SpaceNode; name: SpaceNodeType; label: string; Icon?: React.ComponentType; onClick?: () => void; spaceId: string; + /** Whether to show a beta indicator for this panel item */ showBeta?: boolean; + /** Whether to show a new feature indicator for this panel item */ showNew?: boolean; };app/packages/spaces/src/components/AddPanelItem.tsx (1)
55-65: Consider refactoring badge styling for better maintainability.The badge implementation could be improved by:
- Extracting duplicate styles into a reusable component
- Using the sx prop instead of inline styles
- Using theme spacing values instead of magic numbers
Consider this refactor:
+ const BadgeText = ({ children }) => ( + <Typography + component="span" + variant="caption" + sx={{ + color: theme.primary.main, + ml: 1, + }} + > + {children} + </Typography> + ); {showNew && ( - <span - style={{ - color: theme.primary.main, - fontSize: "11px", - marginLeft: "6px", - }} - > - NEW - </span> + <BadgeText>NEW</BadgeText> )}fiftyone/operators/utils.py (1)
75-106: Consider timezone handling and input validation improvementsThe function implementation looks good overall, but here are some suggested improvements:
def is_new(release_date, days=30): """ Determines if a feature is considered "new" based on its release date. A feature is considered new if its release date is within the specified number of days. Args: release_date (str or datetime): The release date of the feature. - If a string, it should be in the format "%Y-%m-%d" (e.g., "2024-11-09"). - If a datetime object, it will be used directly without conversion. days (int, optional): The number of days within which the feature is considered new. Defaults to 30. Returns: bool: True if the release date is within the specified number of days, False otherwise. Raises: + ValueError: If release_date is invalid or days is not a positive integer. """ + if not isinstance(days, int) or days <= 0: + raise ValueError("days must be a positive integer") + if isinstance(release_date, str): + try: release_date = datetime.strptime(release_date, "%Y-%m-%d") + except ValueError: + raise ValueError("release_date string must be in format YYYY-MM-DD") elif not isinstance(release_date, datetime): raise ValueError("release_date must be a string or datetime object") - return (datetime.now() - release_date).days <= days + # Cache datetime.now() to ensure consistent results + current_time = datetime.now() + return (current_time - release_date).days <= daysConsider these additional improvements:
- Add timezone awareness if needed in the future
- Cache
datetime.now()at the module level if this function is called frequentlyfiftyone/operators/panel.py (1)
37-38: Enhance parameter documentation.The docstring should include all new parameters and their types:
- Add documentation for
betaandis_newparameters- Specify the full type for
category(e.g.,foo.Categories.ANALYZEas shown in PR description)Args: name: the name of the panel label: the display name for the panel + beta (False): whether the panel is in beta + is_new (False): whether the panel should be marked as new - category (Category): the category id of the panel + category (foo.Categories): the category of the panel (e.g., foo.Categories.ANALYZE) priority (None): the priority of the panel for sorting in the UIapp/packages/plugins/src/index.ts (2)
300-307: LGTM! Consider adding JSDoc comments.The type and enum definitions are well-structured and consistent. Consider adding JSDoc comments to document the purpose and usage of each category.
+/** + * Represents the available categories for panels + */ type CategoryID = "import" | "curate" | "analyze" | "custom"; +/** + * Enum providing string representations of panel categories + */ export enum Categories { Import = "import", Curate = "curate", Analyze = "analyze", Custom = "custom", }
309-324: LGTM! Consider enhancing type safety.The utility functions are well-implemented. Consider using the
Categoriesenum values instead of string literals ingetCategoryLabelfor better type safety and maintainability.export function getCategoryLabel(category: CategoryID): string { switch (category) { - case "import": + case Categories.Import: return "Import"; - case "curate": + case Categories.Curate: return "Curate"; - case "analyze": + case Categories.Analyze: return "Analyze"; - case "custom": + case Categories.Custom: return "Custom"; } }fiftyone/operators/operations.py (2)
Line range hint
305-326: Consider reordering parameters for better readabilityThe new parameters (
category,beta,is_new,priority) should be grouped with related parameters. Consider this ordering:
- Essential parameters first (
name,label)- Display-related parameters (
help_markdown,icon,light_icon,dark_icon)- Feature flags (
beta,is_new)- Behavioral parameters (
category,priority,surfaces,reload_on_navigation,allow_duplicates)- Event handlers (all
on_*parameters)
345-347: Enhance documentation with examplesThe documentation for new parameters is clear, but would benefit from examples showing common use cases, especially for:
- Different category values
- When to use
betavsis_newflags- How
priorityaffects panel orderingAlso applies to: 372-372
app/packages/spaces/src/components/AddPanelButton.tsx (3)
51-61: Handle undefined categories when categorizing panelsIn the
categorizedPanelsreducer,getCategoryForPanel(panel)may returnundefined. To prevent runtime errors or unexpected behavior, consider handling cases where the category isundefined.Suggested change:
const category = getCategoryForPanel(panel) || 'uncategorized';
65-67: Avoid hardcoding category names insortedCategoriesThe
sortedCategoriesarray uses hardcoded category names. For better scalability and maintainability, derive the category names dynamically fromcategorizedPanels.Suggested change:
-const sortedCategories = ["import", "curate", "analyze", "custom"] - .map((cat) => categorizedPanels[cat]) - .filter((c) => c); +const sortedCategories = Object.values(categorizedPanels);
113-119: Add TypeScript types toPanelCategoriescomponentThe
PanelCategoriescomponent lacks explicit prop types. Adding TypeScript type annotations enhances type safety and clarity.Suggested change:
+import { ReactNode } from 'react'; + function PanelCategories({ children }: { children: ReactNode }) { return ( <Grid container gap={1} sx={{ p: 2 }}> {children} </Grid> ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
app/packages/core/src/plugins/histograms.tsx(2 hunks)app/packages/embeddings/src/index.ts(2 hunks)app/packages/operators/src/Panel/register.tsx(1 hunks)app/packages/operators/src/built-in-operators.ts(2 hunks)app/packages/plugins/src/index.ts(2 hunks)app/packages/spaces/src/components/AddPanelButton.tsx(5 hunks)app/packages/spaces/src/components/AddPanelItem.tsx(2 hunks)app/packages/spaces/src/components/PanelIcon.tsx(2 hunks)app/packages/spaces/src/components/PanelTab.tsx(3 hunks)app/packages/spaces/src/types.ts(1 hunks)fiftyone/operators/__init__.py(1 hunks)fiftyone/operators/categories.py(1 hunks)fiftyone/operators/operations.py(6 hunks)fiftyone/operators/panel.py(4 hunks)fiftyone/operators/utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
app/packages/core/src/plugins/histograms.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/embeddings/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/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/operators/src/built-in-operators.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/AddPanelButton.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/AddPanelItem.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/PanelIcon.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/PanelTab.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/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.
🪛 Ruff
fiftyone/operators/__init__.py
20-20: .utils.ProgressHandler imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
20-20: .utils.is_new imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: .panel.Panel imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: .panel.PanelConfig imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
22-22: .categories.Categories imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
fiftyone/operators/utils.py
72-72: Redefinition of unused datetime from line 9
Remove definition: datetime
(F811)
72-72: datetime.timedelta imported but unused
Remove unused import: datetime.timedelta
(F401)
🔇 Additional comments (21)
fiftyone/operators/categories.py (1)
1-3: LGTM!
The import statement and spacing follow PEP 8 guidelines.
fiftyone/operators/__init__.py (1)
20-22: Verify the implementation of new features.
Let's verify the implementation of the new features and their usage in panel configurations.
✅ Verification successful
Let me gather more information about the Categories implementation since it wasn't found in the first search.
New features are properly implemented and used across the codebase
The verification shows:
Categoriesenum is properly implemented incategories.pywith four categories: IMPORT, CURATE, ANALYZE, and CUSTOMis_newutility function is well-implemented inutils.pywith comprehensive documentation and proper datetime handling- Both features are actively used in the codebase:
- Categories are used in panel configurations across multiple packages
- The
is_newfunction is used as a parameter in panel configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of new features
# Check Categories enum implementation
echo "Checking Categories enum implementation..."
ast-grep --pattern 'class Categories:
$$$
'
# Check is_new function implementation
echo "Checking is_new function implementation..."
ast-grep --pattern 'def is_new($$$):
$$$
'
# Check usage in panel configurations
echo "Checking usage in panel configurations..."
rg -A 5 'PanelConfig.*category.*=.*Categories\.'
rg -A 5 'is_new\('
Length of output: 4200
Script:
#!/bin/bash
# Search for Categories in different possible locations
echo "Searching for Categories implementation..."
rg -A 10 "class Categories"
echo "Searching for Categories enum usage..."
rg "Categories\." -A 2
Length of output: 2300
🧰 Tools
🪛 Ruff
20-20: .utils.ProgressHandler imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
20-20: .utils.is_new imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: .panel.Panel imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: .panel.PanelConfig imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
22-22: .categories.Categories imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
app/packages/embeddings/src/index.ts (1)
1-5: LGTM! Clean and well-structured imports.
The import statement follows TypeScript best practices with proper destructuring and organization.
app/packages/spaces/src/components/PanelIcon.tsx (1)
5-5: LGTM: Import statement follows conventions
The import of Box from @mui/material is properly placed and follows standard conventions.
app/packages/spaces/src/components/AddPanelItem.tsx (3)
8-8: LGTM! Props and imports are well-structured.
The new props and imports follow TypeScript/React best practices.
Also applies to: 16-17
19-19: LGTM! Theme hook is properly implemented.
The useTheme hook is correctly used to access theme values.
66-76: Apply the same badge styling refactor here.
The BETA badge has identical styling concerns as the NEW badge.
app/packages/core/src/plugins/histograms.tsx (3)
3-7: LGTM! Import changes are well-structured
The addition of Categories import is properly grouped with related imports from @fiftyone/plugins.
114-116: 🛠️ Refactor suggestion
Consider implementing additional panel metadata
The PR objectives mention support for is_new and beta flags, but these aren't implemented here. Consider adding these properties to maintain consistency with the PR's goals:
panelOptions: {
priority: BUILT_IN_PANEL_PRIORITY_CONST,
category: Categories.Curate,
+ isNew: false, // or date-based check
+ beta: false, // if applicable
},Likely invalid or redundant comment.
115-115: Verify if 'Curate' is the appropriate category for Histograms
Histograms are typically used for data analysis and visualization. Consider if Categories.Analyze might be more appropriate than Categories.Curate for this component's primary use case.
✅ Verification successful
Let me gather more context about the categories and histogram usage.
Based on the code analysis, I can now provide a definitive response about the categorization of the Histograms panel.
The 'Curate' category is appropriate for the Histograms panel
The Histograms panel is correctly categorized under Categories.Curate because:
- It's used alongside other data curation tools, as evidenced by the embeddings plugin also using the same category
- The implementation shows it's primarily used for:
- Field selection and data filtering
- Understanding data distributions for curation decisions
- Interactive data exploration and selection
The panel serves as a data curation tool by helping users understand and filter their dataset's distributions, which aligns with the curation workflow rather than pure analysis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other histogram-related components and their categories
rg -A 5 "Categories\.(Analyze|Curate)" --type ts
Length of output: 383
Script:
#!/bin/bash
# Search for Categories enum definition and usage
ast-grep --pattern 'enum Categories {
$$$
}'
# Search for histogram-related code and documentation
rg -i "histogram" -A 5 --type ts
Length of output: 36765
app/packages/spaces/src/components/PanelTab.tsx (1)
4-4: LGTM!
The Grid import is appropriately grouped with other Material-UI components.
fiftyone/operators/panel.py (4)
46-55: LGTM! Constructor parameters are well-structured.
The new parameters are added with appropriate default values and maintain a logical order.
70-73: LGTM! Instance variables are properly initialized.
The new attributes are correctly assigned from constructor parameters.
132-135: LGTM! Panel registration correctly includes new metadata.
The panel configuration dictionary properly includes all new attributes: category, beta, is_new, and priority.
Line range hint 81-92: Verify category string conversion compatibility.
The category is converted to string using str(self.category). Let's verify this works correctly with the expected foo.Categories enum.
✅ Verification successful
Let me gather more information about the Categories enum and its usage.
Category string conversion is safe and compatible
The str(self.category) conversion is correct and compatible with the codebase because:
- The
Categoriesenum is properly defined with string values infiftyone/operators/categories.py - All enum values (
IMPORT,CURATE,ANALYZE,CUSTOM) are already strings - String conversion of these enum values will return their defined string values (
"import","curate","analyze","custom")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how categories are used across the codebase
# Test 1: Find category enum definition
echo "Looking for Categories enum definition..."
rg -p "class.*Categories.*\(.*Enum.*\)"
# Test 2: Find category usage in panel configurations
echo "Looking for category usage in panel configurations..."
rg "category.*=.*Categories\." -A 2
Length of output: 525
Script:
#!/bin/bash
# Check Categories enum definition and values
echo "=== Categories enum definition ==="
cat fiftyone/operators/categories.py
# Find all imports of Categories
echo -e "\n=== Categories imports ==="
rg "from.*categories.*import.*Categories"
# Find all direct usage of Categories enum
echo -e "\n=== Categories enum usage ==="
rg "Categories\.[A-Z_]+" -B 1 -A 1
Length of output: 1513
app/packages/plugins/src/index.ts (1)
326-330: LGTM! Type definition is clear and focused.
The Category type effectively pairs the category ID with its display label.
fiftyone/operators/operations.py (1)
12-12: LGTM: Clean import addition
The relative import of Categories is properly placed and follows the project's import conventions.
app/packages/operators/src/built-in-operators.ts (2)
1000-1003: LGTM! New panel metadata fields are well-implemented.
The new input fields for panel metadata (help_markdown, category, beta, is_new) are correctly implemented using appropriate types and default values. The implementation aligns with the PR objectives to enhance panel configuration.
1013-1013: LGTM! Priority field addition.
The priority field is correctly implemented as an integer input, which will allow for customizable panel ordering.
app/packages/spaces/src/components/AddPanelButton.tsx (2)
1-7: Potential theme inconsistencies between component libraries
The code imports useTheme from @fiftyone/components and uses Typography and Grid from @mui/material. Ensure that the theme object provided by useTheme is compatible with Material-UI components to prevent styling inconsistencies.
35-35: Corrected missing return statement in useMemo
The addition of the return statement in the nodeTypes computation ensures that the array of node types is correctly returned. This fix prevents nodeTypes from being undefined.
| <PanelTabMeta | ||
| showBeta={panel?.panelOptions?.beta} | ||
| showNew={panel?.panelOptions?.isNew} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript types and consider layout stability
The integration looks good, but there are a few improvements needed:
- TypeScript types are missing for the panelOptions properties
- Consider wrapping in an error boundary to prevent crashes
- Consider adding a fixed width container to prevent layout shifts
Apply this diff:
- showBeta={panel?.panelOptions?.beta}
- showNew={panel?.panelOptions?.isNew}
+ showBeta={Boolean(panel?.panelOptions?.beta)}
+ showNew={Boolean(panel?.panelOptions?.isNew)}Also, update your types file to include:
interface PanelOptions {
beta?: boolean;
isNew?: boolean;
// ... other existing properties
}| function PanelTabMeta({ showBeta, showNew }) { | ||
| return ( | ||
| <Grid container gap={1} sx={{ width: "auto", ml: "6px" }}> | ||
| {showNew && ( | ||
| <Grid | ||
| item | ||
| style={{ color: "var(--fo-palette-primary-main)", fontSize: 11 }} | ||
| > | ||
| NEW | ||
| </Grid> | ||
| )} | ||
| {showBeta && ( | ||
| <Grid | ||
| item | ||
| style={{ color: "var(--fo-palette-primary-main)", fontSize: 11 }} | ||
| > | ||
| BETA | ||
| </Grid> | ||
| )} | ||
| </Grid> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve component implementation with TypeScript and better styling
Several improvements are needed:
- Add TypeScript interface for props
- Extract duplicate styles
- Improve accessibility
- Use theme constants instead of hardcoded values
Apply these changes:
interface PanelTabMetaProps {
showBeta?: boolean;
showNew?: boolean;
}
const MetaLabel = styled(Grid)({
color: 'var(--fo-palette-primary-main)',
fontSize: 11,
fontWeight: 'medium',
});
function PanelTabMeta({ showBeta, showNew }: PanelTabMetaProps) {
return (
<Grid
container
gap={1}
sx={{ width: "auto", ml: "6px" }}
aria-label="Panel status indicators"
>
{showNew && (
<MetaLabel item role="status">
NEW
</MetaLabel>
)}
{showBeta && (
<MetaLabel item role="status">
BETA
</MetaLabel>
)}
</Grid>
);
}| from datetime import datetime, timedelta | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant imports
The datetime module is already imported on line 9, and timedelta is not used in the implementation.
-from datetime import datetime, timedelta📝 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.
| from datetime import datetime, timedelta |
🧰 Tools
🪛 Ruff
72-72: Redefinition of unused datetime from line 9
Remove definition: datetime
(F811)
72-72: datetime.timedelta imported but unused
Remove unused import: datetime.timedelta
(F401)
|
|
||
| /** | ||
| * The category of the plugin | ||
| */ | ||
| category: CategoryID; | ||
|
|
||
| /** | ||
| * Whether the plugin is in beta | ||
| */ | ||
| beta: boolean; | ||
|
|
||
| /** | ||
| * Whether the plugin is new | ||
| */ | ||
| isNew: boolean; |
There was a problem hiding this comment.
Consider making the category property optional.
The category property is currently required, which could break existing panel implementations. Since getCategoryForPanel provides a default value of "custom", consider making this property optional for better backward compatibility.
type PanelOptions = {
// ... existing properties ...
/**
* The category of the plugin
*/
- category: CategoryID;
+ category?: CategoryID;
/**
* Whether the plugin is in beta
*/
beta: boolean;
/**
* Whether the plugin is new
*/
isNew: boolean;
};📝 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.
| /** | |
| * The category of the plugin | |
| */ | |
| category: CategoryID; | |
| /** | |
| * Whether the plugin is in beta | |
| */ | |
| beta: boolean; | |
| /** | |
| * Whether the plugin is new | |
| */ | |
| isNew: boolean; | |
| /** | |
| * The category of the plugin | |
| */ | |
| category?: CategoryID; | |
| /** | |
| * Whether the plugin is in beta | |
| */ | |
| beta: boolean; | |
| /** | |
| * Whether the plugin is new | |
| */ | |
| isNew: boolean; |
| "category": category.value if category is not None else None, | ||
| "beta": beta, | ||
| "is_new": is_new, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation for category parameter
The category value extraction could fail if an invalid type is passed. Consider adding type validation or using a safer conversion method.
- "category": category.value if category is not None else None,
+ "category": category.value if isinstance(category, Categories) else None,Committable suggestion skipped: line range outside the PR's diff.
| function PanelCategory({ label, children }) { | ||
| const theme = useTheme(); | ||
| return ( | ||
| <Grid item> | ||
| <Typography | ||
| variant="subtitle2" | ||
| sx={{ padding: "0 8px", color: theme.text.secondary }} | ||
| > | ||
| {label || "no category"} | ||
| </Typography> | ||
| {children} | ||
| </Grid> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add TypeScript types to PanelCategory component and correct theme usage
The PanelCategory component does not explicitly define its prop types, and theme.text.secondary is used instead of theme.palette.text.secondary. Define prop types and correct the theme property to ensure proper styling.
Suggested changes:
+import { ReactNode } from 'react';
+
+interface PanelCategoryProps {
+ label: string;
+ children: ReactNode;
+}
+
function PanelCategory({ label, children }: PanelCategoryProps) {
const theme = useTheme();
return (
<Grid item>
<Typography
variant="subtitle2"
- sx={{ padding: "0 8px", color: theme.text.secondary }}
+ sx={{ padding: "0 8px", color: theme.palette.text.secondary }}
>
{label || "no category"}
</Typography>
{children}
</Grid>
);
}📝 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.
| function PanelCategory({ label, children }) { | |
| const theme = useTheme(); | |
| return ( | |
| <Grid item> | |
| <Typography | |
| variant="subtitle2" | |
| sx={{ padding: "0 8px", color: theme.text.secondary }} | |
| > | |
| {label || "no category"} | |
| </Typography> | |
| {children} | |
| </Grid> | |
| ); | |
| } | |
| import { ReactNode } from 'react'; | |
| interface PanelCategoryProps { | |
| label: string; | |
| children: ReactNode; | |
| } | |
| function PanelCategory({ label, children }: PanelCategoryProps) { | |
| const theme = useTheme(); | |
| return ( | |
| <Grid item> | |
| <Typography | |
| variant="subtitle2" | |
| sx={{ padding: "0 8px", color: theme.palette.text.secondary }} | |
| > | |
| {label || "no category"} | |
| </Typography> | |
| {children} | |
| </Grid> | |
| ); | |
| } |
| @@ -297,6 +297,37 @@ export enum PluginComponentType { | |||
| */ | |||
| } | |||
|
|
|||
| type CategoryID = "import" | "curate" | "analyze" | "custom"; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
We've been discussing the list in a slack thread
| >>> is_new("2024-11-09") # Using a string date and default days | ||
| True # if today's date is within 30 days after 2024-11-09 | ||
|
|
||
| >>> is_new(datetime(2024, 11, 9), days=15) # Using a datetime object with 15-day threshold |
There was a problem hiding this comment.
That's cool, I like that we can manually define when NEW should disappear 🙌🏽
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/packages/spaces/src/components/AddPanelItem.tsx (1)
57-61: Consider using MUI's styling system.Instead of inline styles, consider using MUI's styling system for consistency with the rest of the codebase.
-style={{ - color: theme.custom.primarySoft, - fontSize: "11px", - marginLeft: "6px", -}} +sx={{ + color: (theme) => theme.custom.primarySoft, + fontSize: "11px", + ml: "6px", +}}Also applies to: 68-72
app/packages/embeddings/src/EmptyEmbeddings.tsx (1)
40-44: LGTM! Consider using sx prop shorthand for margin.The change from hardcoded color to theme.custom.primarySoft improves theme consistency.
Consider using the shorthand
mbinstead ofmarginBottomfor better readability:sx={{ fontSize: 64, color: theme.custom.primarySoft, - marginBottom: 2, + mb: 2, }}app/packages/components/src/components/ThemeProvider/index.tsx (1)
98-98: Consider deduplicating redundant color values.The
primarySoftcolor value is identical to bothprimary.plainColorandlightningin the light theme. Consider using a CSS custom property or a shared constant to maintain consistency and reduce duplication.- primarySoft: "hsl(25, 100%, 51%)", + primarySoft: `var(--fo-primary-color, hsl(25, 100%, 51%))`,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/packages/components/src/components/ThemeProvider/index.tsx(2 hunks)app/packages/embeddings/src/EmptyEmbeddings.tsx(1 hunks)app/packages/spaces/src/components/AddPanelItem.tsx(2 hunks)app/packages/spaces/src/components/PanelTab.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/components/src/components/ThemeProvider/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/embeddings/src/EmptyEmbeddings.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/AddPanelItem.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/PanelTab.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 (1)
app/packages/spaces/src/components/AddPanelItem.tsx (1)
8-8: LGTM: Clean import and prop additions.
The new props and import are properly typed and follow TypeScript best practices.
Also applies to: 16-17
| {showNew && ( | ||
| <span | ||
| style={{ | ||
| color: theme.custom.primarySoft, | ||
| fontSize: "11px", | ||
| marginLeft: "6px", | ||
| }} | ||
| > | ||
| NEW | ||
| </span> | ||
| )} | ||
| {showBeta && ( | ||
| <span | ||
| style={{ | ||
| color: theme.custom.primarySoft, | ||
| fontSize: "11px", | ||
| marginLeft: "6px", | ||
| }} | ||
| > | ||
| BETA | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate label styling logic.
The NEW and BETA label implementations share identical styling logic, which violates the DRY principle.
Consider extracting the shared styling into a reusable component:
+const StatusLabel = ({ children }: { children: React.ReactNode }) => {
+ const theme = useTheme();
+ return (
+ <span
+ style={{
+ color: theme.custom.primarySoft,
+ fontSize: "11px",
+ marginLeft: "6px",
+ }}
+ >
+ {children}
+ </span>
+ );
+};
// In the main component
-{showNew && (
- <span
- style={{
- color: theme.custom.primarySoft,
- fontSize: "11px",
- marginLeft: "6px",
- }}
- >
- NEW
- </span>
-)}
-{showBeta && (
- <span
- style={{
- color: theme.custom.primarySoft,
- fontSize: "11px",
- marginLeft: "6px",
- }}
- >
- BETA
- </span>
-)}
+{showNew && <StatusLabel>NEW</StatusLabel>}
+{showBeta && <StatusLabel>BETA</StatusLabel>}Additionally, consider these accessibility improvements:
- Add appropriate ARIA labels
- Use semantic HTML elements
Consider enhancing accessibility:
const StatusLabel = ({ children }: { children: React.ReactNode }) => {
const theme = useTheme();
return (
<span
+ role="status"
+ aria-label={`${children.toString().toLowerCase()} feature`}
style={{
color: theme.custom.primarySoft,
fontSize: "11px",
marginLeft: "6px",
}}
>
{children}
</span>
);
};📝 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.
| {showNew && ( | |
| <span | |
| style={{ | |
| color: theme.custom.primarySoft, | |
| fontSize: "11px", | |
| marginLeft: "6px", | |
| }} | |
| > | |
| NEW | |
| </span> | |
| )} | |
| {showBeta && ( | |
| <span | |
| style={{ | |
| color: theme.custom.primarySoft, | |
| fontSize: "11px", | |
| marginLeft: "6px", | |
| }} | |
| > | |
| BETA | |
| </span> | |
| )} | |
| const StatusLabel = ({ children }: { children: React.ReactNode }) => { | |
| const theme = useTheme(); | |
| return ( | |
| <span | |
| role="status" | |
| aria-label={`${children.toString().toLowerCase()} feature`} | |
| style={{ | |
| color: theme.custom.primarySoft, | |
| fontSize: "11px", | |
| marginLeft: "6px", | |
| }} | |
| > | |
| {children} | |
| </span> | |
| ); | |
| }; | |
| {showNew && <StatusLabel>NEW</StatusLabel>} | |
| {showBeta && <StatusLabel>BETA</StatusLabel>} |
| function PanelTabMeta({ showBeta, showNew }) { | ||
| return ( | ||
| <Grid container gap={1} sx={{ width: "auto", ml: "6px" }}> | ||
| {showNew && ( | ||
| <Grid | ||
| item | ||
| style={{ | ||
| color: "var(--fo-palette-custom-primarySoft)", | ||
| fontSize: 11, | ||
| }} | ||
| > | ||
| NEW | ||
| </Grid> | ||
| )} | ||
| {showBeta && ( | ||
| <Grid | ||
| item | ||
| style={{ | ||
| color: "var(--fo-palette-custom-primarySoft)", | ||
| fontSize: 11, | ||
| }} | ||
| > | ||
| BETA | ||
| </Grid> | ||
| )} | ||
| </Grid> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve PanelTabMeta component implementation
The component needs several improvements:
- Extract duplicated styles into a styled component
- Add accessibility attributes
- Consider using a more semantic HTML structure for status indicators
Apply this diff:
+const StatusIndicator = styled(Grid)({
+ color: "var(--fo-palette-custom-primarySoft)",
+ fontSize: 11,
+});
-function PanelTabMeta({ showBeta, showNew }) {
+function PanelTabMeta({ showBeta, showNew }: { showBeta?: boolean; showNew?: boolean }) {
return (
<Grid container gap={1} sx={{ width: "auto", ml: "6px" }}
+ role="group"
+ aria-label="Panel status indicators"
>
{showNew && (
- <Grid
- item
- style={{
- color: "var(--fo-palette-custom-primarySoft)",
- fontSize: 11,
- }}
- >
+ <StatusIndicator item component="span" role="status">
NEW
- </Grid>
+ </StatusIndicator>
)}
{showBeta && (
- <Grid
- item
- style={{
- color: "var(--fo-palette-custom-primarySoft)",
- fontSize: 11,
- }}
- >
+ <StatusIndicator item component="span" role="status">
BETA
- </Grid>
+ </StatusIndicator>
)}
</Grid>
);
}📝 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.
| function PanelTabMeta({ showBeta, showNew }) { | |
| return ( | |
| <Grid container gap={1} sx={{ width: "auto", ml: "6px" }}> | |
| {showNew && ( | |
| <Grid | |
| item | |
| style={{ | |
| color: "var(--fo-palette-custom-primarySoft)", | |
| fontSize: 11, | |
| }} | |
| > | |
| NEW | |
| </Grid> | |
| )} | |
| {showBeta && ( | |
| <Grid | |
| item | |
| style={{ | |
| color: "var(--fo-palette-custom-primarySoft)", | |
| fontSize: 11, | |
| }} | |
| > | |
| BETA | |
| </Grid> | |
| )} | |
| </Grid> | |
| ); | |
| } | |
| const StatusIndicator = styled(Grid)({ | |
| color: "var(--fo-palette-custom-primarySoft)", | |
| fontSize: 11, | |
| }); | |
| function PanelTabMeta({ showBeta, showNew }: { showBeta?: boolean; showNew?: boolean }) { | |
| return ( | |
| <Grid container gap={1} sx={{ width: "auto", ml: "6px" }} | |
| role="group" | |
| aria-label="Panel status indicators" | |
| > | |
| {showNew && ( | |
| <StatusIndicator item component="span" role="status"> | |
| NEW | |
| </StatusIndicator> | |
| )} | |
| {showBeta && ( | |
| <StatusIndicator item component="span" role="status"> | |
| BETA | |
| </StatusIndicator> | |
| )} | |
| </Grid> | |
| ); | |
| } |
| @@ -95,6 +95,7 @@ let theme = extendMuiTheme({ | |||
| shadowDark: "hsl(200, 0%, 70%)", | |||
| lightning: "hsl(25, 100%, 51%)", | |||
| toastBackgroundColor: "#FFFFFF", | |||
| primarySoft: "hsl(25, 100%, 51%)", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript interface for custom colors.
The new primarySoft color property should be properly typed in the theme's TypeScript interface to maintain type safety.
Add this interface update at the top of the file:
declare module '@mui/material/styles' {
interface CustomPaletteColor {
primarySoft?: string;
}
}Also applies to: 181-181
Here's an example using all of the new settings in python and JS:
New information is displayed only in the
AddPanel*component.Summary by CodeRabbit
Release Notes
New Features
Improvements
AddPanelButtonandAddPanelItemcomponents for better usability and organization.Bug Fixes