Merge release/v1.1.0 to develop
#5154
Conversation
WalkthroughThe changes in this pull request primarily involve the enhancement of the Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
app/packages/embeddings/src/EmbeddingsCTA.tsx (1)
18-30: LGTM! Consider grouping related props togetherThe changes look good and maintain consistency with the FiftyOne branding. The new props enhance the component's flexibility appropriately.
Consider grouping related props together for better readability:
<PanelCTA label="Embeddings help you explore and understand your dataset" description="You can compute and visualize embeddings for your dataset using a selection of pre-trained models or your own embeddings" + demoLabel="Upgrade to FiftyOne Teams to Create Embeddings" + demoDocCaption="Not ready to upgrade yet? Learn how to create embeddings visualizations via code." + tryLink={TRY_LINK} docLink="https://docs.voxel51.com/user_guide/app.html#embeddings-panel" docCaption="Learn how to create embeddings visualizations via code." - demoLabel="Upgrade to FiftyOne Teams to Create Embeddings" - demoDocCaption="Not ready to upgrade yet? Learn how to create embeddings visualizations via code." icon="workspaces" Actions={Actions} name="Embeddings" onBack={onBack} mode={mode} - tryLink={TRY_LINK} />app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx (1)
89-98: Consider using theme-based stylingThe current implementation uses hardcoded values for styling. Consider leveraging Material-UI's theme system for consistent styling:
- Use theme colors instead of hardcoded values
- Move inline styles to a more maintainable solution
<Chip variant="filled" size="small" label={ <LoadingDots text="Evaluating" - style={{ fontSize: "1rem", paddingLeft: 6, color: "#999999" }} + style={{ + fontSize: theme.typography.body1.fontSize, + paddingLeft: theme.spacing(0.75), + color: theme.palette.text.secondary + }} /> } />app/packages/components/src/components/PanelCTA/index.tsx (1)
48-50: Consider extracting computed values logic to a custom hook.The component has multiple similar computations for different props. Consider extracting these into a custom hook for better reusability and maintainability:
function useComputedProps(props: PanelCTAProps) { const { label, description, caption, docCaption, demoLabel, demoDescription, demoCaption, demoDocCaption, } = props; return { computedLabel: IS_APP_MODE_FIFTYONE ? demoLabel || label : label, computedDescription: IS_APP_MODE_FIFTYONE ? demoDescription || description : description, computedCaption: IS_APP_MODE_FIFTYONE ? demoCaption || caption : caption, computedDocCaption: IS_APP_MODE_FIFTYONE ? demoDocCaption || docCaption : docCaption, }; }fiftyone/core/threed/object_3d.py (1)
Line range hint
341-347: Several issues with default_material implementationThe
default_materialhandling appears incomplete:
- The
set_default_materialmethod is called but not defined in the class.- The constructor (
__init__) should be updated to accept thedefault_materialparameter for consistency.- The docstring should be updated to document the new parameter.
- Type hints should be added for the material parameter.
Here's a suggested implementation:
def __init__( self, name: str, visible=True, position: Optional[Vec3UnionType] = None, scale: Optional[Vec3UnionType] = None, quaternion: Optional[Quaternion] = None, + default_material: Optional[Any] = None, ): """The base class for all 3D objects in the scene. Args: name: the name of the object visible (True): default visibility of the object in the scene position (None): the position of the object in object space quaternion (None): the quaternion of the object in object space scale (None): the scale of the object in object space + default_material (None): the default material for the object """ self.name = name self.visible = visible self._position = normalize_to_vec3(position) if position else Vector3() self._scale = ( coerce_to_vec3(scale) if scale else Vector3(1.0, 1.0, 1.0) ) self._quaternion = quaternion or Quaternion() + self._default_material = default_material # ... rest of the initialization ... + def set_default_material(self, material: Any) -> None: + """Sets the default material for the object. + + Args: + material: The material to set as default + """ + self._default_material = material + def get_default_material(self) -> Optional[Any]: + """Gets the default material for the object. + + Returns: + The default material or None if not set + """ + return self._default_materialfiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
Line range hint
266-281: Consider caching matrix computations for better performanceWhile the return structure is well-organized and complete, the method computes five different confusion matrices and their colorscales. Consider caching these computations, especially for large datasets where matrix calculations could be expensive.
Example optimization:
def get_confusion_matrices(self, results): default_classes = results.classes freq = Counter(results.ytrue) if results.missing in freq: freq.pop(results.missing) + # Cache matrix computations + matrices = {} + colorscales = {} + + def get_or_compute_matrix(classes=None): + key = tuple(classes) if classes is not None else None + if key not in matrices: + matrices[key] = results.confusion_matrix(classes=classes) + colorscales[key] = self.get_confusion_matrix_colorscale(matrices[key]) + return matrices[key], colorscales[key] az_classes = sorted(default_classes) za_classes = sorted(default_classes, reverse=True) mc_classes = sorted(freq, key=freq.get, reverse=True) lc_classes = sorted(freq, key=freq.get) - default_matrix = results.confusion_matrix() - az_matrix = results.confusion_matrix(classes=az_classes) - za_matrix = results.confusion_matrix(classes=za_classes) - mc_matrix = results.confusion_matrix(classes=mc_classes) - lc_matrix = results.confusion_matrix(classes=lc_classes) + default_matrix, default_colorscale = get_or_compute_matrix() + az_matrix, az_colorscale = get_or_compute_matrix(az_classes) + za_matrix, za_colorscale = get_or_compute_matrix(za_classes) + mc_matrix, mc_colorscale = get_or_compute_matrix(mc_classes) + lc_matrix, lc_colorscale = get_or_compute_matrix(lc_classes) - default_colorscale = self.get_confusion_matrix_colorscale(default_matrix) - az_colorscale = self.get_confusion_matrix_colorscale(az_matrix) - za_colorscale = self.get_confusion_matrix_colorscale(za_matrix) - mc_colorscale = self.get_confusion_matrix_colorscale(mc_matrix) - lc_colorscale = self.get_confusion_matrix_colorscale(lc_matrix) return { "default_classes": default_classes.tolist(), "az_classes": az_classes, "za_classes": za_classes, "mc_classes": mc_classes, "lc_classes": lc_classes, "default_matrix": default_matrix.tolist(), "az_matrix": az_matrix.tolist(), "za_matrix": za_matrix.tolist(), "mc_matrix": mc_matrix.tolist(), "lc_matrix": lc_matrix.tolist(), "default_colorscale": default_colorscale, "az_colorscale": az_colorscale, "za_colorscale": za_colorscale, "mc_colorscale": mc_colorscale, "lc_colorscale": lc_colorscale, }app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (7)
329-329: Add comments to explain thehidepropertyWhile using
hideto conditionally render rows is effective, adding comments to explain the purpose of thehideproperty can improve code readability for future maintainers.
645-645: Optimize conditional rendering of table rowsInstead of checking
if (hide) return null;within the map function, consider filtering out hidden rows beforehand. This improves performance and code readability.Apply this diff to filter out hidden rows:
- {summaryRows.map((row) => { - const { hide } = row; - if (hide) return null; - // rest of the code - })} + {summaryRows + .filter((row) => !row.hide) + .map((row) => { + // rest of the code + })}
685-691: Avoid nesting<Typography>components unnecessarilyWrapping the fallback
—in an additional<Typography>component is redundant. Adjust the parent<Typography>to handle styling directly.Apply this diff to simplify the code:
<Typography> {value ? ( formatValue(value) ) : ( - <Typography color="text.tertiary">—</Typography> + <span style={{ color: theme.palette.text.tertiary }}>—</span> )} </Typography>Or conditionally set the color:
-<Typography> +<Typography color={value ? "text.primary" : "text.tertiary"}> {value ? formatValue(value) : "—"} </Typography>
727-733: Avoid nesting<Typography>components unnecessarilySimilar to the previous comment, adjust the rendering of
compareValueto eliminate redundant<Typography>elements.Apply this diff:
{compareValue ? ( formatValue(compareValue) ) : ( - <Typography color="text.tertiary"> - — - </Typography> + <span style={{ color: theme.palette.text.tertiary }}>—</span> )}Or conditionally set the color:
-<Typography> +<Typography color={compareValue ? "text.primary" : "text.tertiary"}> {compareValue ? formatValue(compareValue) : "—"} </Typography>
1585-1592: Consider centralizing common sort optionsBoth
CLASS_PERFORMANCE_SORT_OPTIONSandCONFUSION_MATRIX_SORT_OPTIONSinclude a "Default" option. To maintain consistency and reduce duplication, consider extracting shared sort options into a common constant.
Line range hint
1543-1550: Add TypeScript type annotations to function parametersFor better type safety, add explicit type annotations to the
formatPerClassPerformancefunction parameters.Apply this diff:
-function formatPerClassPerformance(perClassPerformance, barConfig) { +function formatPerClassPerformance( + perClassPerformance: Array<PerClassPerformanceMetric>, + barConfig: PLOT_CONFIG_TYPE +) {Define the
PerClassPerformanceMetricinterface accordingly.
Line range hint
1566-1574: Add TypeScript type annotations to function parametersSimilarly, add type annotations to the
getMatrixfunction parameters for improved type safety.Apply this diff:
-function getMatrix(matrices, config) { +function getMatrix( + matrices: ConfusionMatrices, + config: PLOT_CONFIG_TYPE +): ConfusionMatrixData | undefined {Ensure that
ConfusionMatricesandConfusionMatrixDatainterfaces are properly defined.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
app/packages/components/src/components/PanelCTA/index.tsx(5 hunks)app/packages/core/src/components/Starter/content.ts(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(14 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx(3 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx(3 hunks)app/packages/embeddings/README.md(1 hunks)app/packages/embeddings/src/EmbeddingsCTA.tsx(2 hunks)app/packages/map/README.md(1 hunks)app/packages/operators/README.md(1 hunks)docs/source/tutorials/pointe.ipynb(1 hunks)e2e-pw/scripts/generate-screenshots-docker-image/build-docker-image.sh(1 hunks)fiftyone/core/threed/__init__.py(1 hunks)fiftyone/core/threed/object_3d.py(1 hunks)fiftyone/internal/secrets/secret.py(1 hunks)fiftyone/operators/builtins/panels/model_evaluation/__init__.py(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- app/packages/core/src/components/Starter/content.ts
- app/packages/embeddings/README.md
- app/packages/map/README.md
- app/packages/operators/README.md
- docs/source/tutorials/pointe.ipynb
- e2e-pw/scripts/generate-screenshots-docker-image/build-docker-image.sh
- fiftyone/core/threed/init.py
- fiftyone/internal/secrets/secret.py
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/components/src/components/PanelCTA/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/NativeModelEvaluationView/Evaluation.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/NativeModelEvaluationView/EvaluationPlot.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/NativeModelEvaluationView/Overview.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/NativeModelEvaluationView/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/EmbeddingsCTA.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 (15)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx (1)
34-34: LGTM! The scaleanchor addition improves plot visualization.
The addition of scaleanchor: "x" to the yaxis configuration ensures consistent scale ratios between x and y axes, which is particularly beneficial for maintaining visual accuracy in evaluation plots.
Let's verify the impact on existing evaluation plots:
✅ Verification successful
Let me gather more information about the usage context and any potential layout configurations.
Let me check the EvaluationPlot component implementation to understand how the layout is handled.
The scaleanchor addition is correctly implemented and safe to use
The scaleanchor: "x" property is properly added to the default y-axis configuration within the layoutDefaults object. This ensures consistent scale ratios between axes while:
- Preserving the ability to override through props if needed
- Maintaining compatibility with other layout configurations
- Working harmoniously with the existing auto-margin and theme settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for components using EvaluationPlot to assess the impact
# of the scaleanchor change on existing implementations
# Look for files importing or using EvaluationPlot
rg -l "EvaluationPlot" app/packages/core/src/plugins/SchemaIO/components/
# Check for any custom layout overrides that might conflict
ast-grep --pattern 'layout={$_}'
Length of output: 294
Script:
#!/bin/bash
# Check the content of Evaluation.tsx to see how EvaluationPlot is used
cat app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
# Search for any plotly layout configurations in the codebase
rg -A 5 "layout[=:]" app/packages/core/src/plugins/SchemaIO/components/
Length of output: 59400
Script:
#!/bin/bash
# Check the content of EvaluationPlot.tsx to understand layout handling
cat app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx
Length of output: 2326
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx (2)
8-8: LGTM: Clean import addition
The useTheme hook is properly imported and utilized in the component.
74-75: Verify the necessity of the theme hook
The useTheme hook is imported and initialized but not used in the visible code. If it's not needed, consider removing it to keep the code clean.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (1)
91-95: LGTM! Props changes enhance component flexibility.
The changes improve the component by:
- Correcting branding in
demoLabel - Adding contextual documentation with
demoDocCaption - Providing flexibility with
tryLink
Also applies to: 111-111
app/packages/components/src/components/PanelCTA/index.tsx (3)
36-37: LGTM! Props declaration follows React best practices.
The new props are well-named and properly integrated into the existing props destructuring pattern.
135-138: LGTM! Proper conditional rendering with Material-UI components.
The implementation follows React and Material-UI best practices for conditional rendering and typography.
187-188: LGTM! Type definitions follow TypeScript best practices.
The new props are properly typed as optional string properties, maintaining consistency with the existing type definitions.
fiftyone/core/threed/object_3d.py (2)
2-2: LGTM: Copyright year updated
The copyright notice has been properly updated to include 2024.
Line range hint 341-347: Verify the impact of material handling changes
Let's verify the usage and potential impact of these changes.
✅ Verification successful
Let me gather more specific information about the material handling changes.
Material handling changes appear to be safe and properly integrated
The changes to material handling in Object3D are well-contained and follow existing patterns:
- Material handling is properly integrated with the serialization system via
_from_dict - The
default_materialproperty is consistently used across the codebase - Material implementations are properly imported and used in related classes (Mesh, PointCloud)
- No breaking changes detected in the material handling implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing material-related functionality and potential breaking changes
# Check for existing material handling
echo "Checking for existing material handling..."
rg -l "material" --type py
# Check for Object3D instantiation patterns
echo "Checking Object3D usage patterns..."
ast-grep --pattern 'Object3D($$$)' --lang py
# Check for _from_dict usage
echo "Checking serialization usage..."
rg "_from_dict" --type py
Length of output: 7830
Script:
#!/bin/bash
# Check material-related code in Object3D and its usage
echo "Checking material handling in Object3D..."
rg "material" fiftyone/core/threed/object_3d.py -B 2 -A 2
echo "\nChecking material usage in tests..."
rg "material" tests/unittests/threed/object_3d_tests.py -B 2 -A 2
echo "\nChecking material imports and dependencies..."
rg "from.*material" fiftyone/core/threed/ -g "*.py"
Length of output: 1483
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (2)
245-250: LGTM: Clear variable naming improvement
The renaming from classes to default_classes enhances code clarity by explicitly indicating this is the default set of classes. The subsequent sorting operations are correctly updated to use the new variable name.
253-260: LGTM: Consistent addition of default matrix and colorscale
The introduction of default_matrix and default_colorscale maintains consistency with the naming scheme and follows the established pattern for matrix and colorscale computation.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (4)
52-52: LGTM!
The default sorting configuration is correctly updated.
167-175: Ensure evaluation types are handled correctly
The introduction of evaluationType, compareEvaluationType, isObjectDetection, and isSegmentation improves clarity when handling different evaluation types. This enhances readability and maintainability.
186-187: LGTM!
Properly using evaluationType and compareEvaluationType in infoRows.
1543-1543: Ensure consistent default handling in getConfigLabel
The condition if (!sortBy || sortBy === DEFAULT_BAR_CONFIG.sortBy) returns an empty string for default sorting. Verify that this logic aligns with other parts of the code where sortBy is used.
| @@ -2,6 +2,8 @@ import { PanelCTA, PanelCTAProps } from "@fiftyone/components"; | |||
| import ComputeVisualizationButton from "./ComputeVisualizationButton"; | |||
| import useComputeVisualization from "./useComputeVisualization"; | |||
|
|
|||
| const TRY_LINK = "http://voxel51.com/try-embeddings"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using HTTPS and environment configuration for the URL
The hardcoded URL has two potential issues:
- It uses HTTP instead of HTTPS, which is less secure
- Hardcoding URLs makes maintenance more difficult across environments
Consider this approach:
-const TRY_LINK = "http://voxel51.com/try-embeddings";
+const TRY_EMBEDDINGS_URL = process.env.REACT_APP_EMBEDDINGS_TRY_URL || "https://voxel51.com/try-embeddings";📝 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 TRY_LINK = "http://voxel51.com/try-embeddings"; | |
| const TRY_EMBEDDINGS_URL = process.env.REACT_APP_EMBEDDINGS_TRY_URL || "https://voxel51.com/try-embeddings"; |
| import { constants } from "@fiftyone/utilities"; | ||
| import Evaluate from "./Evaluate"; | ||
|
|
||
| const TRY_LINK = "http://voxel51.com/try-evaluation"; | ||
|
|
||
| export default function NativeModelEvaluationView(props) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type annotations for better type safety.
The component and its props lack TypeScript type annotations. Consider adding proper type definitions to improve type safety and developer experience.
Example improvement:
interface NativeModelEvaluationViewProps {
data?: {
evaluations?: Array<{ key: string; id: string }>;
view?: Record<string, unknown>;
statuses?: Record<string, unknown>;
notes?: Record<string, unknown>;
permissions?: Record<string, unknown>;
pending_evaluations?: Array<unknown>;
};
schema: {
view: {
on_change_view: string;
on_evaluate_model: string;
load_evaluation: string;
load_evaluation_view: string;
set_status: string;
set_note: string;
load_view: string;
};
};
onChange: (path: string, value: unknown) => void;
layout: unknown;
}
export default function NativeModelEvaluationView(props: NativeModelEvaluationViewProps) {
// ... rest of the component
}| import { constants } from "@fiftyone/utilities"; | ||
| import Evaluate from "./Evaluate"; | ||
|
|
||
| const TRY_LINK = "http://voxel51.com/try-evaluation"; |
There was a problem hiding this comment.
💡 Codebase verification
Move URL constant to centralized configuration and ensure URL consistency
The codebase shows inconsistent URL patterns that should be standardized:
- Similar URLs are defined in multiple locations with different formats:
app/packages/utilities/src/constants.ts: Useshttps://and consistent domain formatapp/packages/embeddings/src/EmbeddingsCTA.tsx: Useshttp://with different URL pattern- Current file: Uses
http://with different URL pattern
Recommendations:
- Move the URL to
app/packages/utilities/src/constants.tsalongside other Voxel51 URLs - Use HTTPS protocol consistently
- Follow the URL pattern established in
TRY_IN_BROWSER_LINK
🔗 Analysis chain
Consider moving the URL constant to a centralized configuration.
The hardcoded URL should be moved to a centralized configuration file for better maintainability and consistency.
Let's verify if similar URLs are defined elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar URL patterns across the codebase
rg -i "voxel51\.com/try-" --type ts --type tsx
Length of output: 76
Script:
#!/bin/bash
# Search for similar URL patterns across the codebase with correct file extensions
rg -i "voxel51\.com/try" -t ts -t typescript -t jsx -t tsx
# Search for any configuration files that might contain URLs
fd -e ts -e tsx -e json "config|constants" --exec grep -l "voxel51.com"
# Look for any centralized URL or link configurations
ast-grep --pattern 'const $_ = "https?://voxel51.com/$_"'
Length of output: 269
Script:
#!/bin/bash
# Let's check the content of the constants file that was found
cat ./app/packages/utilities/src/constants.ts
# Also search with ripgrep without file type restrictions
rg -i "voxel51\.com/try"
Length of output: 3109
| @@ -114,7 +119,7 @@ export default function PanelCTA(props: PanelCTAProps) { | |||
| <MuiButton | |||
| variant="contained" | |||
| color="primary" | |||
| href={TRY_IN_BROWSER_LINK} | |||
| href={tryLink || TRY_IN_BROWSER_LINK} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security attributes for external link.
When using target="_blank", it's recommended to add rel="noopener noreferrer" to prevent potential security vulnerabilities:
-href={tryLink || TRY_IN_BROWSER_LINK}
-target="_blank"
+href={tryLink || TRY_IN_BROWSER_LINK}
+target="_blank"
+rel="noopener noreferrer"Committable suggestion skipped: line range outside the PR's diff.
* Updated Tabelview to support tooltip
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (3)
106-106: Optimize tooltip map creation with useMemoThe tooltipMap is currently recreated on every render. Consider memoizing it to improve performance.
- const tooltipMap = getTooltips(tooltips); + const tooltipMap = useMemo(() => getTooltips(tooltips), [tooltips, getTooltips]);
184-185: Remove unnecessary empty linesThese empty lines don't serve any purpose and should be removed for better code readability.
Also applies to: 207-208
198-204: Improve tooltip implementation for better accessibility and maintainabilityThe current tooltip implementation could be enhanced:
- Move inline styles to the sx prop
- Add aria-label for better accessibility
- Consider extracting the cell content rendering logic
- {tooltip ? ( - <Tooltip title={tooltip} arrow> - <span>{formatCellValue(item[key], props)}</span> - </Tooltip> - ) : ( - formatCellValue(item[key], props) - )} + {tooltip ? ( + <Tooltip + title={tooltip} + arrow + placement="top" + sx={{ display: 'inline-block' }} + > + <span aria-label={tooltip}> + {formatCellValue(item[key], props)} + </span> + </Tooltip> + ) : ( + <span>{formatCellValue(item[key], props)}</span> + )}fiftyone/operators/types.py (2)
1841-1858: Consider enhancing the Tooltip class documentation and type safety.While the implementation is clean, consider the following improvements:
- Add usage examples in the docstring
- Add type hints for parameters
- Add validation for required parameters (row, column, value)
class Tooltip(View): """A tooltip (currently supported only in a :class:`TableView`). Args: value: the value of the tooltip row: the row of the tooltip column: the column of the tooltip + + Examples:: + table_view = TableView() + table_view.add_tooltip( + row=0, + column="status", + value="Processing completed successfully" + ) """ - def __init__(self, **kwargs): + def __init__(self, *, row: int, column: str, value: str, **kwargs): super().__init__(**kwargs) + if any(param is None for param in (row, column, value)): + raise ValueError("row, column, and value are required parameters") + self.row = row + self.column = column + self.value = value def clone(self): clone = Tooltip(**self._kwargs) return clone def to_json(self): - return {**super().to_json()} + return { + **super().to_json(), + "row": self.row, + "column": self.column, + "value": self.value + }
1873-1873: Add documentation and type hints for the tooltips feature.The tooltips implementation in TableView is clean, but consider adding parameter documentation and type hints for better maintainability.
class TableView(View): """Displays a table. Args: columns (None): a list of :class:`Column` objects to display row_actions (None): a list of :class:`Action` objects to display + tooltips (None): a list of :class:`Tooltip` objects to display """ def __init__(self, **kwargs): super().__init__(**kwargs) - self.columns = kwargs.get("columns", []) - self.row_actions = kwargs.get("row_actions", []) - self.tooltips = kwargs.get("tooltips", []) + self.columns: list[Column] = kwargs.get("columns", []) + self.row_actions: list[Action] = kwargs.get("row_actions", []) + self.tooltips: list[Tooltip] = kwargs.get("tooltips", []) - def add_tooltip(self, row, column, value, **kwargs): + def add_tooltip(self, row: int, column: str, value: str, **kwargs) -> Tooltip: + """Adds a tooltip to the table. + + Args: + row: the row index for the tooltip + column: the column key for the tooltip + value: the tooltip text to display + **kwargs: additional tooltip properties + + Returns: + The created :class:`Tooltip` instance + """ tooltip = Tooltip(row=row, column=column, value=value, **kwargs) self.tooltips.append(tooltip) return tooltipAlso applies to: 1889-1893, 1899-1899, 1907-1907
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(5 hunks)fiftyone/operators/types.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.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/core/src/plugins/SchemaIO/components/TableView.tsx (1)
13-13: LGTM: Import and prop changes are well-structured
The Tooltip import and prop addition follow the existing patterns and maintain backward compatibility.
Also applies to: 38-38
| const getTooltips = useCallback((tooltipList) => { | ||
| const tooltipDict = {}; | ||
| for (const { value, row, column } of tooltipList) { | ||
| tooltipDict[`${row},${column}`] = value; // Create a key from row and column | ||
| } | ||
| return tooltipDict; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and memoization of getTooltips
The function implementation could be improved in several ways:
- Add dependency array to useCallback to prevent unnecessary recreations
- Add TypeScript type annotations
- Use a more type-safe approach for coordinate keys
Consider this implementation:
- const getTooltips = useCallback((tooltipList) => {
+ type TooltipItem = { value: string; row: number; column: number };
+ type TooltipMap = { [key: string]: string };
+
+ const getTooltips = useCallback((tooltipList: TooltipItem[]): TooltipMap => {
const tooltipDict = {};
for (const { value, row, column } of tooltipList) {
- tooltipDict[`${row},${column}`] = value; // Create a key from row and column
+ tooltipDict[`${row},${column}`] = value;
}
return tooltipDict;
- }, []);
+ }, [tooltipList]);Committable suggestion skipped: line range outside the PR's diff.
Prevent video time from getting assigned NaN when frame rate is undefined
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/elements/util.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/elements/util.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/looker/src/elements/util.ts
[error] 109-109: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
🔇 Additional comments (1)
app/packages/looker/src/elements/util.ts (1)
Line range hint 8-8: LGTM! Good extraction of magic number to constant
The extraction of the frame offset value to a named constant improves code maintainability and readability.
🧰 Tools
🪛 Biome
[error] 109-109: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
| const time = (frameNumber - 1 + 0.01) / frameRate; | ||
| return isFinite(time) ? time : 0.0; |
There was a problem hiding this comment.
Use Number.isFinite for strict type checking
The current implementation uses the global isFinite function which performs type coercion. This could lead to unexpected behavior when handling non-number inputs.
Apply this diff to use the safer Number.isFinite method:
const time = (frameNumber - 1 + 0.01) / frameRate;
- return isFinite(time) ? time : 0.0;
+ return Number.isFinite(time) ? time : 0.0;📝 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 time = (frameNumber - 1 + 0.01) / frameRate; | |
| return isFinite(time) ? time : 0.0; | |
| const time = (frameNumber - 1 + 0.01) / frameRate; | |
| return Number.isFinite(time) ? time : 0.0; |
🧰 Tools
🪛 Biome
[error] 109-109: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
Merge
release/v1.1.0todevelopSummary by CodeRabbit
Release Notes
New Features
tryLinkanddemoDocCaptionprops to thePanelCTAcomponent, enhancing dynamic content display.TableViewcomponent for improved user context on cell values.Tooltipclass added to enhance theTableViewfunctionality.Bug Fixes
Documentation
Chores