Merge release/v1.0.1 to develop
#4894
Conversation
WalkthroughThe pull request introduces significant updates to the documentation and functionality related to the COCO dataset integration within the FiftyOne framework. Key enhancements include expanded code examples, updated method signatures, and improved clarity on dataset loading and exporting processes. Changes also involve modifications to the handling of category IDs and the introduction of new parameters for dataset manipulation. Additionally, unit tests have been enhanced to ensure comprehensive coverage of the import/export functionalities across various dataset types. Changes
Possibly related PRs
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
|
* add lm operators * add query performance to ctx * send query_performance
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
docs/source/integrations/coco.rst (5)
Line range hint
219-230: Updated example data to reflect current format.The changes in category ID, file name, and image dimensions accurately represent the current state of the example dataset. This update ensures that users following the documentation will see consistent results.
Consider adding a note that the exact values in the example output may vary depending on the version of the dataset or random selection of samples.
240-247: Annotation example updated to match new image data.The changes in category ID, bounding box coordinates, and area value are consistent with the updates in the image information. This ensures that the annotation example accurately reflects the corresponding image data.
Consider adding a cross-reference or note to explicitly link this annotation example with the image information provided earlier, helping users understand the relationship between the two.
270-272: Helpful addition demonstrating COCO category import.This new code snippet effectively shows that COCO categories are imported along with the dataset. It provides users with a clear example of how to access this important metadata.
Consider adding a brief explanation of how users might utilize this category information in their FiftyOne workflows, such as filtering or grouping samples based on categories.
319-323: Updated mock predictions and category reference.The changes accurately reflect the new method of accessing category information using
coco_dataset.info["categories"]. The updated mock predictions provide a more realistic and consistent example.Consider adding a brief explanation of the relationship between the
category_idin the predictions and the categories incoco_dataset.info["categories"]. This could help users understand how to map between prediction outputs and dataset categories.
325-328: Improved handling of COCO categories in label addition.The changes demonstrate the correct way to use the imported COCO categories when adding labels to the dataset. This ensures consistency between the dataset's category information and the added labels.
Consider adding a brief comment explaining the importance of using the dataset's category information when adding labels. This could help users understand why this approach is preferred over hardcoding category information.
docs/source/user_guide/export_datasets.rst (2)
Line range hint
4-4: Address the TODO comment by adding testsThe TODO comment indicates that tests are missing for this function. It's important to add unit tests to ensure the function behaves correctly for various inputs and edge cases.
Would you like assistance in generating unit tests for this function?
Line range hint
12-24: Reconsider the fee structure or discount policyWhile the function's implementation is correct, the business logic may lead to unintended consequences:
- The flat $20 fee applied after the discount can negate the benefit of the discount, especially for smaller purchases or lower discount tiers.
- Customers who barely qualify for a discount (e.g., 3-4 years of loyalty) might end up paying more than if they had no discount at all.
This structure could lead to customer dissatisfaction and potentially discourage loyalty.
Consider revising the policy in one of the following ways:
- Apply the fee before the discount.
- Use a percentage-based fee instead of a flat fee.
- Implement a minimum purchase amount for the fee to apply.
- Adjust the discount percentages to always outweigh the flat fee.
Would you like assistance in implementing a revised version of this function with a more customer-friendly policy?
fiftyone/utils/coco.py (2)
Line range hint
580-587: Change parameterclassestocategoriesin_get_matching_image_idscallThe function
_get_matching_image_idsis called withclasses=self.classes, but after renaming, it should becategories=self.categories.Apply this diff to update the function call:
annotations, image_ids=self.image_ids, - classes=self.classes, + categories=self.categories, shuffle=self.shuffle, seed=self.seed, max_samples=self.max_samples,
Line range hint
1854-1861: Update parameter name tocategoriesin_get_matching_image_idsIn the call to
_get_matching_image_ids, the argumentclasses=classesshould be updated tocategories=categoriesto align with the new naming convention.Apply this diff to fix the parameter name:
annotations, image_ids=image_ids, - classes=classes, + categories=categories, shuffle=shuffle, seed=seed, max_samples=max_samples,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- docs/source/integrations/coco.rst (6 hunks)
- docs/source/user_guide/dataset_creation/datasets.rst (2 hunks)
- docs/source/user_guide/export_datasets.rst (2 hunks)
- fiftyone/utils/coco.py (15 hunks)
- tests/unittests/import_export_tests.py (2 hunks)
🧰 Additional context used
🪛 Ruff
tests/unittests/import_export_tests.py
1822-1822: Ambiguous variable name:
l(E741)
🔇 Additional comments (9)
docs/source/integrations/coco.rst (1)
199-200: Excellent addition of theseedparameter!Adding the
seedparameter to theexportmethod enhances reproducibility. This change allows users to consistently generate the same subset of data, which is crucial for maintaining consistent results across different runs or environments.docs/source/user_guide/dataset_creation/datasets.rst (6)
Line range hint
1-191: Excellent improvements to the introduction and basic recipe sections!The expanded introduction now includes valuable information about the @voxel51/io plugin for importing media and labels from within the FiftyOne App. The basic recipe section has been refined with clearer instructions and more comprehensive examples for both Python and CLI usage. These changes significantly enhance the user's understanding of how to create and import datasets in FiftyOne.
Line range hint
192-279: Great addition of the supported formats table!The new table listing various dataset types, their descriptions, and corresponding import methods is an excellent addition to the documentation. It provides a comprehensive and easily accessible overview of the supported formats in FiftyOne.
Suggestion for further improvement:
Consider adding hyperlinks to each dataset type in the table, linking to their respective detailed sections in the documentation. This would allow users to quickly navigate to more in-depth information about each format.
Line range hint
280-2166: Excellent updates to individual dataset type sections!The documentation for each dataset type has been significantly improved:
- More detailed explanations provide better context for each format.
- Updated code examples offer improved consistency and clarity.
- Addition of new dataset types (e.g., OpenLABEL and GeoTIFF) keeps the documentation current with FiftyOne's capabilities.
These changes greatly enhance the user's ability to understand and work with various dataset formats in FiftyOne. The consistent structure across different dataset types makes it easier for users to find the information they need.
Line range hint
2167-3015: Excellent expansion of the custom formats section!The expanded section on custom dataset formats provides valuable guidance for advanced users:
- Detailed instructions on writing custom dataset importers and types.
- Helpful pseudocode templates for creating custom importers.
- Clear explanations of the different interfaces for various dataset types (unlabeled/labeled, image/video, grouped).
This addition greatly enhances the documentation's usefulness for users working with non-standard data formats.
Suggestion for further improvement:
Consider adding a simple, complete example of a custom importer implementation for a hypothetical dataset format. This would provide users with a concrete reference to build upon when creating their own custom importers.
Line range hint
3016-3046: Valuable addition of dataset-level information import section!The new section on importing dataset-level information is a great addition to the documentation:
- Clear explanation of how to use
has_dataset_infoandget_dataset_info()methods.- Useful description of special cases for storing information in dedicated dataset fields.
This information is crucial for users who need to import metadata or other dataset-level information alongside their samples.
Suggestion for improvement:
Consider adding a small code snippet demonstrating how to implement theget_dataset_info()method in a custom importer, showing how to return a dictionary with some of the special keys mentioned (e.g., "classes" or "default_classes").
Line range hint
1-3046: Outstanding improvements to the dataset creation and import documentation!This comprehensive update to the documentation significantly enhances its value to FiftyOne users:
- Expanded introduction and refined basic recipe sections provide clearer guidance for getting started.
- New table of supported formats offers an excellent quick reference for users.
- Detailed explanations and consistent code examples for each dataset type improve usability.
- Addition of new dataset types (e.g., OpenLABEL, GeoTIFF) keeps the documentation current.
- Expanded custom formats section with pseudocode templates assists advanced users.
- New section on importing dataset-level information covers an important advanced topic.
These improvements make the documentation more comprehensive, accessible, and valuable for users at all levels of expertise. The consistent structure and detailed explanations will greatly aid users in understanding and implementing FiftyOne's dataset creation and import capabilities.
fiftyone/utils/coco.py (2)
941-944: Confirm correctness of category IDs and names in categories creationWhen creating the
categorieslist, ensure that the IDs and names are correctly assigned, especially after renaming variables.Please review the construction of the
categorieslist to ensure that category IDs correspond to the correct names.
1947-1949: Ensure consistency in variable naming within_get_images_with_classesThe variable
labels_map_revis created usingclasses_map.items(). Verify thatclasses_mapis appropriately named and consider renaming it tocategories_mapfor consistency.Run the following script to check for inconsistent usage:
| info["classes"] = _to_classes(classes_map) | ||
|
|
There was a problem hiding this comment.
Update info["classes"] to info["categories"]
In line 578, the code assigns the class mappings to info["classes"]. To reflect the renaming from classes to categories, this should be updated to info["categories"].
Apply this diff to fix the inconsistency:
if classes_map is not None:
- info["classes"] = _to_classes(classes_map)
+ info["categories"] = _to_classes(classes_map)📝 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.
| info["classes"] = _to_classes(classes_map) | |
| info["categories"] = _to_classes(classes_map) | |
| if isinstance(categories, dict): | ||
| classes_map = categories | ||
| elif not categories: | ||
| classes_map = {} | ||
| elif isinstance(categories[0], dict): | ||
| classes_map = {c["id"]: c["name"] for c in categories} | ||
| else: | ||
| classes_map = {i: label for i, label in enumerate(classes)} | ||
| classes_map = {i: label for i, label in enumerate(categories, 1)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename classes_map to categories_map for consistency
Since the parameter has been renamed from classes to categories, it's recommended to rename the variable classes_map to categories_map to maintain consistency and avoid confusion.
Apply this diff to rename the variable:
if isinstance(categories, dict):
- classes_map = categories
+ categories_map = categories
elif not categories:
- classes_map = {}
+ categories_map = {}
elif isinstance(categories[0], dict):
- classes_map = {c["id"]: c["name"] for c in categories}
+ categories_map = {c["id"]: c["name"] for c in categories}
else:
- classes_map = {i: label for i, label in enumerate(categories, 1)}
+ categories_map = {i: label for i, label in enumerate(categories, 1)}Remember to update subsequent usages of classes_map to categories_map.
📝 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.
| if isinstance(categories, dict): | |
| classes_map = categories | |
| elif not categories: | |
| classes_map = {} | |
| elif isinstance(categories[0], dict): | |
| classes_map = {c["id"]: c["name"] for c in categories} | |
| else: | |
| classes_map = {i: label for i, label in enumerate(classes)} | |
| classes_map = {i: label for i, label in enumerate(categories, 1)} | |
| if isinstance(categories, dict): | |
| categories_map = categories | |
| elif not categories: | |
| categories_map = {} | |
| elif isinstance(categories[0], dict): | |
| categories_map = {c["id"]: c["name"] for c in categories} | |
| else: | |
| categories_map = {i: label for i, label in enumerate(categories, 1)} |
| image_ids, annotations, classes, classes_map | ||
| ) | ||
| else: | ||
| all_ids = image_ids |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename parameter classes to categories in _get_images_with_classes definition
The parameter classes in the _get_images_with_classes function should be renamed to categories to reflect the updated terminology.
Apply this diff to rename the parameter:
def _get_images_with_classes(
image_ids, annotations, target_classes, classes_map
- ):
+ ):
+ # Rename target_classes to target_categories
+ target_categories = target_classesAnd update the variable usage within the function accordingly.
Committable suggestion was skipped due to low confidence.
| labels_map_rev = _to_labels_map_rev(sorted(self._classes)) | ||
| for anno in self._annotations: | ||
| anno["category_id"] = labels_map_rev[anno["category_id"]] | ||
| else: | ||
| classes = self.classes | ||
| elif self.categories is None: | ||
| labels_map_rev = _to_labels_map_rev(self.classes) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Multiple occurrences of self.classes found across the codebase
The self.classes attribute is still present in several files, which may lead to potential AttributeError exceptions if not properly updated. Please address the following locations to ensure consistency and prevent errors:
fiftyone/zoo/datasets/base.pyfiftyone/utils/openimages.pyfiftyone/utils/yolo.pyfiftyone/utils/coco.pyfiftyone/utils/transformers.pyfiftyone/utils/tf.pyfiftyone/utils/kinetics.pyfiftyone/utils/torch.pyfiftyone/utils/eval/base.pyfiftyone/utils/clip/zoo.pyfiftyone/utils/data/parsers.pyfiftyone/utils/data/exporters.pyfiftyone/utils/data/importers.pyfiftyone/utils/eval/coco.pyfiftyone/utils/eval/classification.pyfiftyone/utils/eval/activitynet.pyfiftyone/utils/activitynet.pyfiftyone/core/collections.py
Please ensure all instances of self.classes are replaced with self.categories where appropriate.
🔗 Analysis chain
Replace self.classes with self.categories
At line 921, self.classes is used, but it should be self.categories due to the renaming. This prevents potential AttributeError exceptions.
Apply this diff to correct the variable:
elif self.categories is None:
- labels_map_rev = _to_labels_map_rev(self.classes)
+ labels_map_rev = _to_labels_map_rev(self.categories)Run the following script to verify any remaining occurrences of self.classes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of 'self.classes' in the codebase.
# Expected result: No occurrences of 'self.classes' should remain.
rg --type python -F 'self.classes'
Length of output: 64
Script:
#!/bin/bash
# Description: Find all occurrences of 'self.classes' in Python files within the codebase.
rg --type py -F 'self.classes'
Length of output: 8709
| categories = [{"id": i, "name": l} for i, l in enumerate(classes, 1)] | ||
|
|
There was a problem hiding this comment.
Avoid ambiguous variable name l
Using a single-letter variable name like l can be confusing and may be mistaken for the digit 1. It is advisable to use a more descriptive name for better readability.
Apply this diff to improve clarity:
-classes = dataset.distinct("predictions.detections.label")
-categories = [{"id": i, "name": l} for i, l in enumerate(classes, 1)]
+classes = dataset.distinct("predictions.detections.label")
+categories = [{"id": i, "name": label} for i, label in enumerate(classes, 1)]📝 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.
| categories = [{"id": i, "name": l} for i, l in enumerate(classes, 1)] | |
| classes = dataset.distinct("predictions.detections.label") | |
| categories = [{"id": i, "name": label} for i, label in enumerate(classes, 1)] |
🧰 Tools
🪛 Ruff
1822-1822: Ambiguous variable name:
l(E741)
* add dynamic embedded document field type for rrd files * add schema utils to work with embedded doc type fields * add versioned renderer with url+metadata support * remove explicit version
* organizing * rm Tmp * cleanup * add example back * cleaning * useKeyEvents hook * after load handling * fix import * enable timeline * default to true * refreshers
* add dynamic embedded document field type for rrd files * add schema utils to work with embedded doc type fields * add versioned renderer with url+metadata support * remove explicit version
* organizing * rm Tmp * cleanup * add example back * cleaning * useKeyEvents hook * after load handling * fix import * enable timeline * default to true * refreshers
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (20)
fiftyone/utils/rerun.py (2)
14-20: Minor improvement: Refine docstring argument descriptions.The class definition and overall docstring structure look good. However, there's a small inconsistency in how the arguments are described.
Consider updating the argument descriptions in the docstring as follows:
Args: - filepath (None): the path to the rrd file - version (None): the version of the rrd file + filepath (str): the path to the rrd file + version (str): the version of the rrd fileThis change more accurately reflects that these arguments are expected to be strings, rather than suggesting they have a default value of
None.
1-25: Consider future enhancements for the RrdFile class.The overall structure and implementation of the
RrdFileclass are good. For future iterations, consider the following enhancements:
- Add methods for working with rrd files, such as loading, saving, or validating the file contents.
- Implement error handling and validation for the
filepathandversionfields.- Include examples in the class docstring to demonstrate usage.
These additions would make the class more robust and user-friendly.
Would you like assistance in drafting these enhancements?
app/packages/state/src/hooks/useQueryPerformance.ts (2)
9-41: Overall hook structure is well-designed, but consider naming the function.The hook is well-structured, using appropriate React and Recoil hooks. The memoization of the returned object is a good optimization. However, for better debugging and stack traces, consider naming the function:
-export default function () { +export default function useQueryPerformance() {
18-32: Theenablefunction is well-implemented, but consider adding error handling.The function correctly uses
useRecoilCallbackand implements a good fallback logic for determining the threshold value. The use of an optional parameter provides flexibility.Consider adding error handling for cases where both
lightningThresholdConfiganddatasetSampleCountmight be undefined:if (setting === undefined) { try { setting = (await snapshot.getPromise(lightningThresholdConfig)) ?? (await snapshot.getPromise(datasetSampleCount)); if (setting === undefined) { throw new Error("Unable to determine threshold value"); } } catch (error) { console.error("Error enabling performance threshold:", error); return; // or handle the error appropriately } }app/packages/state/src/hooks/index.ts (1)
27-27: LGTM! Consider grouping related hooks.The new export for
useQueryPerformancefollows the established pattern and naming conventions in the file. It adheres to TypeScript and React best practices for exporting hooks.For improved organization, consider grouping related hooks together. For instance, you could move this export near other performance-related hooks if they exist, or create a new section for performance hooks.
app/packages/looker/src/elements/common/looker.ts (1)
29-33: Improved state destructuring for better organizationThe changes in the
getEventsmethod enhance the state structure:
- Moving
shouldHandleKeyEventsinto anoptionsobject improves state organization.- The change is consistently applied in both
keydownandkeyuphandlers.These modifications make the code more maintainable and easier to understand.
For consistency, consider using the same destructuring pattern in the
keyuphandler:keyup: ({ event, update, dispatchEvent }) => { // ... update((state) => { const { SHORTCUTS, error, options: { shouldHandleKeyEvents } } = state; // ... rest of the function }); },This would make both handlers use identical state destructuring patterns.
Also applies to: 52-52
app/packages/state/src/utils.ts (1)
105-110: LGTM: Function signature updated correctlyThe addition of
ModalSample["urls"]as a possible type for theurlsparameter increases the function's flexibility and is consistent with the import statement update. The implementation remains unchanged, which is correct as it can handle the new type without modifications.Consider improving readability by using a type alias for the union type:
type StandardizedUrlsInput = readonly { readonly field: string; readonly url: string }[] | { [field: string]: string } | ModalSample["urls"]; export const getStandardizedUrls = (urls: StandardizedUrlsInput) => { // ... function implementation ... };This change would make the function signature more concise and easier to maintain if more types are added in the future.
app/packages/state/src/recoil/modal.ts (1)
1-1: LGTM! Consider using type-only imports for better performance.The changes in imports reflect a shift towards more specific type definitions, which is a good practice in TypeScript. The removal of
AbstractLookerandBaseStatein favor ofLookerssuggests a refinement in the type system.Consider using type-only imports for
Lookersto potentially improve build performance:import type { Lookers } from "../hooks";Also applies to: 6-6
app/packages/state/src/hooks/useCreateLooker.ts (1)
39-40: Good addition of optional parameterThe introduction of the
enableTimelineoptional parameter enhances the flexibility of the Looker creation process while maintaining backward compatibility. This is a good practice.Consider adding a JSDoc comment to describe the purpose and impact of the
enableTimelineparameter. This would improve the function's documentation and make it easier for other developers to understand its usage.Example:
/** * @param enableTimeline - When true, enables timeline functionality for the created Looker instance. */app/packages/looker/src/state.ts (1)
178-178: LGTM. Consider adding documentation for the new property.The addition of
shouldHandleKeyEvents?: booleanto theBaseOptionsinterface provides more flexibility in controlling key event handling. This change is well-structured and maintains backward compatibility.Consider adding a brief comment to explain the purpose and default behavior of this property for better maintainability.
fiftyone/server/metadata.py (1)
37-37: LGTM: RrdFile added to _ADDITIONAL_MEDIA_FIELDSThe
RrdFile: "filepath"entry has been correctly added to the_ADDITIONAL_MEDIA_FIELDSdictionary. This addition allows the metadata handling logic to work with the new RrdFile type.For consistency with the other entries, consider adding a comment explaining the purpose of this field, similar to the comments for Heatmap and Segmentation above.
app/packages/operators/src/operators.ts (3)
140-143: LGTM! Consider adding a comment for clarity.The new
queryPerformancegetter method is a good addition, aligning with the new property inRawContext.Consider adding a brief comment explaining the purpose of this getter, e.g.:
/** * Returns whether query performance tracking is enabled. */ public get queryPerformance(): boolean { return Boolean(this._currentContext.queryPerformance); }
714-714: LGTM! Consider using optional chaining for consistency.The addition of
queryPerformanceto the request payload is good, ensuring the server is aware of the query performance tracking setting.For consistency with other optional properties, consider using optional chaining:
query_performance: currentContext.queryPerformance?.queryPerformance,This change would make the code more resilient if
queryPerformanceis undefined in some contexts.
Inconsistency in
queryPerformanceInclusion Across MethodsThe
queryPerformanceproperty has been added toexecuteOperatorWithContextand is present inresolveRemoteType, but it is missing inresolveExecutionOptions.
- Affected Method:
resolveExecutionOptionsPlease verify if
queryPerformanceshould be included in theresolveExecutionOptionsmethod as well for consistency. If its omission is intentional, consider adding a comment to explain the rationale.Category:
🔗 Analysis chain
Line range hint
1-1180: Verify ifqueryPerformanceshould be included in other methods.The
queryPerformanceproperty has been added toexecuteOperatorWithContext, but it's not included in some other methods that use the context, such asresolveRemoteTypeandresolveExecutionOptions.Please verify if
queryPerformanceshould be included in these methods as well for consistency. If it's intentionally omitted, consider adding a comment explaining why.To help with this verification, you can run the following script:
This script will help identify methods that use context-related properties and compare them with methods that already include
queryPerformance.
Action Required: Run an Updated Script to Verify
queryPerformanceInclusionThe previous script encountered an error due to an unrecognized file type. Please run the following updated script to check for methods that might need to include
queryPerformance:Please execute this script to gather the necessary information for verification.
Category:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for methods that might need to include queryPerformance # Search for methods that use context-related properties rg --type typescript -e 'current_sample:' -e 'dataset_name:' -e 'delegation_target:' -e 'extended:' -e 'extended_selection:' -e 'filters:' -A 10 app/packages/operators/src/operators.ts # Compare with methods that already include queryPerformance rg --type typescript -e 'query_performance:' app/packages/operators/src/operators.tsLength of output: 329
Script:
# #!/bin/bash # Description: Check for methods that might need to include queryPerformance # Search for methods that use context-related properties in TypeScript files rg -g "*.ts" -g "*.tsx" -e 'current_sample:' -e 'dataset_name:' -e 'delegation_target:' -e 'extended:' -e 'extended_selection:' -e 'filters:' -A 10 app/packages/operators/src/operators.ts # Compare with methods that already include queryPerformance rg -g "*.ts" -g "*.tsx" -e 'query_performance:' app/packages/operators/src/operators.tsLength of output: 4876
fiftyone/operators/executor.py (1)
716-719: Consider enhancing thequery_performanceproperty.While the implementation is correct, consider the following improvements:
- Add type hinting for better code readability and IDE support.
- Include a more detailed docstring explaining the property's purpose and possible values.
- Use a more specific default value (e.g., False) instead of None.
Here's a suggested implementation:
@property def query_performance(self) -> bool: """ Whether query performance is enabled for the current execution context. Returns: bool: True if query performance is enabled, False otherwise. """ return self.request_params.get("query_performance", False)These changes would improve the property's usability and documentation.
app/packages/core/src/components/Modal/use-key-events.ts (1)
39-39: Reconsider includingrefin the dependency arrayThe
refobject itself is typically stable and does not change between renders. However, its.currentproperty can change without causing a re-render. Includingrefin the dependency array may not trigger theuseEffectwhenref.currentchanges. If your effect depends on changes toref.current, you might need to adjust your logic or consider a different approach to track changes.app/packages/core/src/components/Modal/VideoLooker.tsx (2)
21-23: Consider removing unnecessaryuseMemoforframeRateSince
frameRateis a primitive value derived directly fromsample.frameRate, usinguseMemomay be unnecessary. Accessing a property is inexpensive, and memoization might not provide a significant performance benefit here.
60-61: Re-evaluate the necessity ofuseMemofortimelineNameGenerating
timelineNameinvolves callinggetName(), which is likely an inexpensive operation. UsinguseMemomight not be needed unlessgetNameis computationally intensive or if its return value changes frequently.app/packages/core/src/components/Modal/ImaVidLooker.tsx (2)
22-25: Consolidate imports from./hooksConsider combining all imports from
"./hooks"into a single statement for better readability.Apply this diff to consolidate the imports:
-import { - useInitializeImaVidSubscriptions, - useLookerOptionsUpdate, -} from "./hooks"; - -import { useModalContext } from "./hooks"; +import { + useInitializeImaVidSubscriptions, + useLookerOptionsUpdate, + useModalContext, + useClearSelectedLabels, + useShowOverlays, +} from "./hooks";And remove the import from line 20 after moving the hooks.
26-26: Ensure consistent import style foruseKeyEventsIf
useKeyEventsis a named export, consider importing it using destructuring to maintain consistent import styles.Apply this diff if applicable:
-import useKeyEvents from "./use-key-events"; +import { useKeyEvents } from "./use-key-events";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/packages/looker/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (26)
- app/packages/core/src/components/Modal/ImaVidLooker.tsx (2 hunks)
- app/packages/core/src/components/Modal/ModalLooker.tsx (3 hunks)
- app/packages/core/src/components/Modal/VideoLooker.tsx (1 hunks)
- app/packages/core/src/components/Modal/hooks.ts (1 hunks)
- app/packages/core/src/components/Modal/use-key-events.ts (1 hunks)
- app/packages/core/src/components/Modal/use-looker.ts (1 hunks)
- app/packages/core/src/components/Modal/utils.ts (1 hunks)
- app/packages/looker/src/elements/base.ts (1 hunks)
- app/packages/looker/src/elements/common/looker.ts (3 hunks)
- app/packages/looker/src/index.ts (1 hunks)
- app/packages/looker/src/lookers/imavid/index.ts (1 hunks)
- app/packages/looker/src/lookers/video.ts (3 hunks)
- app/packages/looker/src/state.ts (3 hunks)
- app/packages/operators/src/built-in-operators.ts (2 hunks)
- app/packages/operators/src/operators.ts (4 hunks)
- app/packages/operators/src/state.ts (5 hunks)
- app/packages/state/src/hooks/index.ts (1 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (4 hunks)
- app/packages/state/src/hooks/useQueryPerformance.ts (1 hunks)
- app/packages/state/src/recoil/modal.ts (3 hunks)
- app/packages/state/src/utils.ts (2 hunks)
- app/packages/utilities/src/schema.test.ts (7 hunks)
- app/packages/utilities/src/schema.ts (1 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/server/metadata.py (2 hunks)
- fiftyone/utils/rerun.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/looker/src/lookers/imavid/index.ts
🧰 Additional context used
📓 Path-based instructions (22)
app/packages/core/src/components/Modal/ImaVidLooker.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/ModalLooker.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/VideoLooker.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/hooks.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/components/Modal/use-key-events.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/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/components/Modal/utils.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/looker/src/elements/base.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/looker/src/elements/common/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/looker/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/looker/src/lookers/video.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/looker/src/state.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/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/operators/src/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/operators/src/state.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/state/src/hooks/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/state/src/hooks/useCreateLooker.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/state/src/hooks/useQueryPerformance.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/state/src/recoil/modal.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/state/src/utils.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/utilities/src/schema.test.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/utilities/src/schema.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/core/src/components/Modal/hooks.ts
[error] 25-25: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (43)
app/packages/looker/src/index.ts (1)
6-6: LGTM. Verify the impact of the new export.The addition of
getFrameNumberto the exports looks good. This change expands the public API of the module, makinggetFrameNumberavailable to importers alongsidefreeVideos.To ensure this change doesn't unexpectedly affect other parts of the codebase, please run the following script to check for existing uses of
getFrameNumber:This will help identify any places where
getFrameNumbermight already be in use or where this new export might be immediately useful.✅ Verification successful
LGTM. Impact verification successful.
The addition of
getFrameNumberto the exports inapp/packages/looker/src/index.tshas been verified. The function is actively used within the codebase, and there are no existing imports from@fiftyone/lookerthat are affected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing uses of getFrameNumber in the codebase # Search for imports or uses of getFrameNumber rg --type typescript --type javascript 'getFrameNumber' # Search for imports from this specific file rg --type typescript --type javascript 'from\s+[''"]@fiftyone/looker[''"]'Length of output: 198
Script:
#!/bin/bash # Description: Check for existing uses of getFrameNumber in TypeScript and JavaScript files # Search for imports or uses of getFrameNumber in .ts and .tsx files rg 'getFrameNumber' --glob '*.ts' --glob '*.tsx' # Search for imports from this specific module in .ts and .tsx files rg 'from\s+["\']@fiftyone/looker["\']' --glob '*.ts' --glob '*.tsx'Length of output: 4073
Script:
#!/bin/bash # Description: Check for existing uses of getFrameNumber in TypeScript and JavaScript files # Search for imports or uses of getFrameNumber in .ts and .tsx files rg 'getFrameNumber' --glob '*.ts' --glob '*.tsx' # Search for imports from this specific module in .ts and .tsx files rg 'from\s+["@]fiftyone/looker["]' --glob '*.ts' --glob '*.tsx'Length of output: 4045
fiftyone/utils/rerun.py (2)
1-11: LGTM: File header and imports are well-structured.The file header contains an appropriate docstring explaining the purpose of the module, along with necessary copyright information. The import statements are relevant to the functionality being implemented in this file.
22-25: LGTM: Class attributes are well-defined.The class attributes are correctly implemented:
- The
_MEDIA_FIELDclass attribute is properly set to "filepath", indicating the primary media reference.- Both
filepathandversioninstance attributes are appropriately defined asStringFields, which aligns with the class's purpose of storing rrd file information.app/packages/state/src/hooks/useQueryPerformance.ts (3)
1-7: Imports look good and follow best practices.The necessary hooks from React and Recoil are imported, along with the required custom Recoil atoms. All imported items are used in the code, and there are no unused imports.
10-16: Thedisablefunction is correctly implemented.The function properly uses
useRecoilCallbackto set thelightningThresholdtonull, effectively disabling the threshold. The empty dependency array is correct as the function doesn't depend on any external variables.
34-40: The return statement with useMemo is correctly implemented.The use of
useMemoto memoize the returned object is an appropriate optimization. The dependency array correctly includes bothdisableandenablefunctions, ensuring the memoized value updates if either function changes.app/packages/core/src/components/Modal/hooks.ts (1)
22-41: Good implementation, but some improvements neededThe new
useLookerOptionsUpdatehook is well-implemented usinguseRecoilCallbackfor atomic updates to Recoil state. However, there are a few areas for improvement:
- The
updateparameter is typed asobject, which is too broad. Consider using a more specific type to improve type safety.- Error handling is missing for potential issues during state retrieval or update.
- The function could be slightly refactored for better readability.
Here's a suggested refactor addressing these points:
import { SavedLookerOptions } from "@fiftyone/types"; // Assume this type exists export const useLookerOptionsUpdate = () => { return useRecoilCallback( ({ snapshot, set }) => async (update: Partial<SavedLookerOptions>, updater?: (updated: SavedLookerOptions) => void) => { try { const [currentOptions, panels] = await Promise.all([ snapshot.getPromise(fos.savedLookerOptions), snapshot.getPromise(fos.lookerPanels) ]); const updated: SavedLookerOptions = { ...currentOptions, ...update, showJSON: panels.json.isOpen, showHelp: panels.help.isOpen, }; set(fos.savedLookerOptions, updated); updater?.(updated); } catch (error) { console.error("Failed to update Looker options:", error); // Consider adding more robust error handling here } } ); };This refactor:
- Uses a more specific type for the
updateparameter.- Adds basic error handling.
- Uses
Promise.allfor concurrent state retrieval.- Simplifies the updater function call.
To ensure the
SavedLookerOptionstype exists and is correctly defined, run:🧰 Tools
🪛 Biome
[error] 25-25: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
app/packages/looker/src/elements/base.ts (1)
67-67: Approve the use of optional chainingThe change from
this.element &&tothis.element?.is a good improvement. It enhances readability while maintaining the same functionality of only adding the event listener ifthis.elementexists.To ensure consistency across the codebase, let's check if this pattern is used elsewhere:
app/packages/looker/src/elements/common/looker.ts (1)
6-9: Improved import statements for better type handlingThe changes to the import statements are beneficial:
- Using
import typeforBaseState,Control, andEventsensures these are used only for type checking, potentially reducing bundle size.- Separating the import of
ControlEventKeyTypesuggests it's used as a value, which is correct if it's an enum or constant.These modifications align with TypeScript best practices and can lead to more efficient code compilation.
app/packages/state/src/utils.ts (1)
21-21: LGTM: Import statement updated correctlyThe addition of
ModalSampleto the import statement is consistent with its usage in thegetStandardizedUrlsfunction. This change follows TypeScript best practices and doesn't introduce any issues.app/packages/state/src/recoil/modal.ts (2)
72-74: LGTM! Improved null safety with optional chaining.The change from
if (id && id.endsWith("-modal"))toif (id?.endsWith("-modal"))is a good improvement. It uses the optional chaining operator (?.), which is a more concise and idiomatic way in modern TypeScript to handle potential null or undefined values. This change enhances null safety without altering the underlying logic.
Line range hint
26-30: LGTM! Verify usage ofmodalLookeracross the codebase.The update to the
modalLookeratom type fromAbstractLooker<BaseState> | nulltoLookers | nullaligns well with the import changes. This suggests a move towards a more specific and possibly custom type for your application, which is a good practice in TypeScript.To ensure this change doesn't introduce any type errors elsewhere in the codebase, please run the following script:
This will help identify any places where the
modalLookeris used and might need updates due to the type change.app/packages/state/src/hooks/useCreateLooker.ts (3)
12-12: Excellent use of type-only importsThe change to use type-only imports for
BaseStateandImaVidConfigis a good practice in TypeScript. It clearly indicates that these imports are used solely for type information, potentially improving build performance and reducing bundle size.
116-116: Correct implementation of new configuration optionThe addition of
enableTimelineto theconfigobject correctly incorporates the new parameter into the Looker configuration. This implementation ensures that the timeline functionality can be properly controlled when creating Looker instances.
Line range hint
1-270: Summary of changesThe modifications to
useCreateLooker.tsenhance the flexibility of the Looker creation process by introducing an optionalenableTimelineparameter. The changes are well-implemented, maintaining good TypeScript practices such as using type-only imports. The new parameter is correctly incorporated into the configuration object, ensuring proper control over the timeline functionality.Overall, these changes improve the module's functionality while maintaining code quality and backward compatibility.
app/packages/looker/src/state.ts (3)
462-462: LGTM. Default value set appropriately.The addition of
shouldHandleKeyEvents: truetoDEFAULT_BASE_OPTIONSaligns well with the changes made to theBaseOptionsinterface. Setting the default value totrueensures backward compatibility with existing behavior.
Line range hint
1-524: Verify removal ofshouldHandleKeyEventsfromBaseStateThe AI-generated summary mentioned the removal of
shouldHandleKeyEventsfrom theBaseStateinterface, but this change is not visible in the provided code. Please verify if this change has been made or if it's an oversight in the summary.Run the following command to check for the presence of
shouldHandleKeyEventsin theBaseStateinterface:#!/bin/bash # Search for shouldHandleKeyEvents in BaseState interface rg --type typescript "interface BaseState" -A 20 | rg "shouldHandleKeyEvents"If the property is still present, consider removing it to align with the changes made to
BaseOptions.
222-222: LGTM. Add documentation and ensure backward compatibility.The addition of
enableTimeline: booleanto theVideoConfiginterface introduces a new configuration option for video timelines. This is a good enhancement for video functionality.Please add a brief comment explaining the purpose and impact of this property.
Ensure that this change doesn't break existing implementations. Consider the following:
✅ Verification successful
Verified: The addition of
enableTimelineto theVideoConfiginterface does not impact existing implementations, as no usages overriding or settingVideoConfigwere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing uses of VideoConfig to verify backward compatibility rg --type typescript "VideoConfig" -C 5Length of output: 112
Script:
#!/bin/bash # Search for existing uses of VideoConfig to verify backward compatibility rg --glob '*.ts' --glob '*.tsx' "VideoConfig" -C 5Length of output: 1858
fiftyone/server/metadata.py (1)
27-27: LGTM: New import added for RrdFileThe import statement for
RrdFilefromfiftyone.utils.rerunhas been correctly added. This import is necessary for the subsequent changes in the file.app/packages/operators/src/operators.ts (2)
1-2: LGTM! New query performance tracking feature added.The import statements have been reorganized for better clarity. A new
queryPerformanceproperty has been added to theRawContexttype, indicating the introduction of a query performance tracking feature.Also applies to: 94-94
Line range hint
1-1180: LGTM! ThequeryPerformancefeature is well-implemented.The addition of the
queryPerformancefeature is consistent and well-integrated throughout the file. The changes enhance the functionality by allowing performance tracking during operator execution.Great job on implementing this new feature!
app/packages/operators/src/built-in-operators.ts (3)
1316-1332: LGTM: DisableQueryPerformance class implementationThe DisableQueryPerformance class is well-implemented, following the structure of other operator classes. It correctly extends the Operator class, sets the _builtIn property, and implements the necessary config and execute methods. The use of the fos.useQueryPerformance hook to access the disable function is appropriate.
1334-1350: LGTM: EnableQueryPerformance class implementationThe EnableQueryPerformance class is correctly implemented, mirroring the structure of the DisableQueryPerformance class. It properly extends the Operator class, sets the _builtIn property, and implements the required config and execute methods. The use of the fos.useQueryPerformance hook to access the enable function is appropriate.
1401-1402: LGTM: New operators registered correctlyThe DisableQueryPerformance and EnableQueryPerformance operators are correctly registered in the registerBuiltInOperators function. The registration is done using the _registerBuiltInOperator function, which is consistent with how other operators are registered in this function.
fiftyone/operators/executor.py (3)
716-719: LGTM: Newquery_performanceproperty added.The new
query_performanceproperty has been correctly implemented as a read-only property that retrieves the value from therequest_paramsdictionary. This addition allows users to check whether query performance is enabled for the current execution context.
716-719: Summary of changes: New query performance feature added.The addition of the
query_performanceproperty to theExecutionContextclass is a small but potentially significant change. It suggests that FiftyOne is expanding its capabilities in terms of query performance monitoring or optimization.While the implementation is correct, consider the following:
- Enhance the property with type hinting and a more detailed docstring.
- Provide more context on how this property is set and used throughout the codebase.
- Update relevant documentation to explain this new feature and its impact on operator execution.
These changes lay the groundwork for improved query performance monitoring, which could be valuable for users working with large datasets or complex queries.
716-719: Provide more context for thequery_performanceproperty.The addition of the
query_performanceproperty suggests an implementation or improvement in query performance monitoring. To ensure proper integration and usage, please provide more information on:
- How and where is this property set in the
request_params?- Are there any specific operators or functions that utilize this property?
- Is there any documentation or comments that should be added to explain the impact of enabling query performance?
This additional context will help developers understand when and how to use this new feature effectively.
To help verify the usage of this new property, you can run the following script:
app/packages/utilities/src/schema.ts (1)
55-74: Well-implemented recursive functionThe
getFieldsWithEmbeddedDocTypefunction is correctly implemented using recursion to traverse the schema and collect fields with the specifiedembeddedDocType. The code is clear and follows TypeScript best practices.app/packages/core/src/components/Modal/ModalLooker.tsx (4)
2-2: Import statement forImageLookeris appropriate.The
ImageLookertype is correctly imported from@fiftyone/lookerand used appropriately in the code.
30-60:ModalLookerNoTimelinecomponent correctly manages looker state.The component effectively initializes and updates the looker instance using the
useLookerhook. TheuseEffecthooks properly handle the side effects of setting the modal looker and the active looker reference with appropriate dependency arrays.
27-27: Ensure all components provide the requiredsampleprop.By making
samplea required property inLookerProps, this change ensures that all components using this interface receive the necessary data. Please verify that all components implementingLookerPropsare updated to pass thesampleprop to prevent potential TypeScript errors or runtime issues.Run the following script to check for any components using
LookerPropsthat may not provide thesampleprop:✅ Verification successful
All components provide the required
sampleprop. Verification successful.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations of `LookerProps` and check for the `sample` prop. # Test: Search for components using `LookerProps` without passing `sample`. rg --type tsx 'interface\s+LookerProps' --files-with-matches | xargs rg --type tsx -A 5 'LookerProps' | rg -B 5 -A 5 -v 'sample'Length of output: 182
Script:
#!/bin/bash # Description: Find all .tsx files, search for LookerProps interface, and check for missing 'sample' prop. # Find all .tsx files fd --extension tsx | while read -r file; do # Check if the file contains the LookerProps interface if rg 'interface\s+LookerProps' "$file" > /dev/null; then # Check if 'sample' prop is missing in the interface if ! rg 'sample\s*:' "$file" > /dev/null; then echo "$file" fi fi doneLength of output: 44022
80-89: Verify conditional rendering logic inModalLooker.The component conditionally renders different looker components based on
shouldRenderImavidandvideostates. Ensure that the conditions accurately reflect the intended functionality and there are no unintended overlaps. It's important to confirm that the correct component is rendered for each possible combination of these state values.Run the following script to check the possible values and usage contexts of
shouldRenderImaVidLookerandisVideoDataset:app/packages/core/src/components/Modal/use-looker.ts (1)
67-69: Ensurerefis attached to a DOM element before dispatching eventsThe
ref.currentmay benullif it's not assigned to any DOM element. To ensure thelooker-attachedevent is dispatched correctly, make sure that therefis attached to a valid DOM element within your component.app/packages/utilities/src/schema.test.ts (6)
22-23: Updates tonameandpathproperties are correct.The
nameandpathproperties for theembeddedfield have been updated appropriately to reflect the correct structure.
33-33: Updatedpathfor embedded field is accurate.The
pathproperty now correctly represents the nested field asembedded.field.
45-46:embeddedWithDbFieldsproperties updated correctly.The updates to
nameandpathforembeddedWithDbFieldsare consistent and accurate.
57-57:pathforsample_idupdated appropriately.The
pathnow accurately reflects the nested structure asembeddedWithDbFields.sample_id.
113-145: New test cases forgetFieldsWithEmbeddedDocTypeare comprehensive.The added tests effectively cover multiple scenarios, ensuring that the function behaves correctly for existing types, non-existent types, and empty schemas.
146-176: Test cases fordoesSchemaContainEmbeddedDocTypeare thorough.The tests correctly verify the function's behavior with various
embeddedDocTypevalues, including checks for existing types, non-existent types, and empty schemas.app/packages/core/src/components/Modal/ImaVidLooker.tsx (1)
136-136: Verify the dependencies passed touseKeyEventsEnsure that
initialRef,sample._id, andlookerare the correct and necessary dependencies foruseKeyEvents. Confirm thatuseKeyEventsis implemented to handle these arguments appropriately.app/packages/looker/src/lookers/video.ts (1)
23-25: Imports are necessary for added functionalityThe imports of
setFrameNumberAtomfrom@fiftyone/playback,Schemafrom@fiftyone/utilities, andgetDefaultStorefromjotaiare appropriate and required for the new features implemented in the code.app/packages/operators/src/state.ts (2)
97-97: Approved: Addition ofqueryPerformanceto global contextThe inclusion of
queryPerformancein theglobalContextSelectoris appropriate and correctly implemented, allowing performance-related data to be accessible throughout the application context.Also applies to: 109-109
150-150: Approved: PassingqueryPerformanceintoExecutionContextIncluding
queryPerformancein theuseExecutionContextfunction and its dependencies ensures that performance metrics are properly propagated to theExecutionContext, facilitating performance optimizations where necessary.Also applies to: 168-168, 184-184
| export function shortcutToHelpItems(SHORTCUTS) { | ||
| const result = {}; | ||
| for (const k of SHORTCUTS) { | ||
| result[SHORTCUTS[k].shortcut] = SHORTCUTS[k]; | ||
| } | ||
| return Object.values(result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve TypeScript usage and function implementation
The function shortcutToHelpItems could benefit from several improvements:
- Add TypeScript types for better type safety and code clarity.
- Use array methods for a more concise and potentially more efficient implementation.
- Improve naming to better reflect the function's purpose.
- Add explanatory comments.
Here's a suggested refactored version:
/**
* Converts a collection of shortcuts into an array of unique help items.
* @param shortcuts An array of shortcut objects.
* @returns An array of unique shortcut objects.
*/
export function convertShortcutsToUniqueHelpItems(
shortcuts: Array<{ shortcut: string; [key: string]: any }>
): Array<{ shortcut: string; [key: string]: any }> {
return Array.from(
new Map(shortcuts.map(item => [item.shortcut, item])).values()
);
}This refactored version:
- Uses TypeScript types for better type safety.
- Utilizes
Array.fromandMapfor a more concise implementation. - Renames the function to better describe its purpose.
- Includes a JSDoc comment explaining the function's purpose and parameters.
Please consider implementing these changes to improve the code quality and maintainability.
| export default function ( | ||
| ref: MutableRefObject<boolean>, | ||
| id: string, | ||
| looker: Lookers | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Name the default exported function for better clarity and debugging
Exporting an anonymous function as the default can make debugging more difficult and reduce code readability. Naming the function enhances stack traces and aligns with React and TypeScript best practices. Consider naming the function useKeyEvents to reflect that it's a custom hook.
Apply this diff to name the function:
-export default function (
+export default function useKeyEvents(
ref: MutableRefObject<boolean>,
id: string,
looker: Lookers
) {📝 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.
| export default function ( | |
| ref: MutableRefObject<boolean>, | |
| id: string, | |
| looker: Lookers | |
| ) { | |
| export default function useKeyEvents( | |
| ref: MutableRefObject<boolean>, | |
| id: string, | |
| looker: Lookers | |
| ) { |
| useEffect(() => { | ||
| if (ref.current) { | ||
| // initial call should wait for load event | ||
| const update = () => { | ||
| looker.updateOptions({ | ||
| shouldHandleKeyEvents: id === hoveredId, | ||
| }); | ||
| ready.current = true; | ||
|
|
||
| looker.removeEventListener("load", update); | ||
| }; | ||
| looker.addEventListener("load", update); | ||
| } else if (ready.current) { | ||
| looker.updateOptions({ | ||
| shouldHandleKeyEvents: id === hoveredId, | ||
| }); | ||
| } | ||
| }, [hoveredId, id, looker, ref]); |
There was a problem hiding this comment.
Add a cleanup function to remove the event listener in useEffect
Currently, the useEffect hook adds an event listener to looker, but if the component unmounts before the 'load' event fires, the event listener won't be removed, potentially causing a memory leak. It's best practice to clean up event listeners in the useEffect cleanup function.
Apply this diff to add the cleanup:
useEffect(() => {
if (ref.current) {
// initial call should wait for load event
const update = () => {
looker.updateOptions({
shouldHandleKeyEvents: id === hoveredId,
});
ready.current = true;
looker.removeEventListener("load", update);
};
looker.addEventListener("load", update);
+ return () => {
+ looker.removeEventListener("load", update);
+ };
} else if (ready.current) {
looker.updateOptions({
shouldHandleKeyEvents: id === hoveredId,
});
}
}, [hoveredId, id, looker, ref]);This ensures that the event listener is properly removed if the component unmounts or if the dependencies change.
📝 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.
| useEffect(() => { | |
| if (ref.current) { | |
| // initial call should wait for load event | |
| const update = () => { | |
| looker.updateOptions({ | |
| shouldHandleKeyEvents: id === hoveredId, | |
| }); | |
| ready.current = true; | |
| looker.removeEventListener("load", update); | |
| }; | |
| looker.addEventListener("load", update); | |
| } else if (ready.current) { | |
| looker.updateOptions({ | |
| shouldHandleKeyEvents: id === hoveredId, | |
| }); | |
| } | |
| }, [hoveredId, id, looker, ref]); | |
| useEffect(() => { | |
| if (ref.current) { | |
| // initial call should wait for load event | |
| const update = () => { | |
| looker.updateOptions({ | |
| shouldHandleKeyEvents: id === hoveredId, | |
| }); | |
| ready.current = true; | |
| looker.removeEventListener("load", update); | |
| }; | |
| looker.addEventListener("load", update); | |
| return () => { | |
| looker.removeEventListener("load", update); | |
| }; | |
| } else if (ready.current) { | |
| looker.updateOptions({ | |
| shouldHandleKeyEvents: id === hoveredId, | |
| }); | |
| } | |
| }, [hoveredId, id, looker, ref]); |
| export function doesSchemaContainEmbeddedDocType( | ||
| schema: Schema, | ||
| embeddedDocType: string | ||
| ): boolean { | ||
| function recurse(schema: Schema): boolean { | ||
| return Object.values(schema ?? {}).some((field) => { | ||
| if (field.embeddedDocType === embeddedDocType) { | ||
| return true; | ||
| } | ||
| if (field.fields) { | ||
| return recurse(field.fields); | ||
| } | ||
| return false; | ||
| }); | ||
| } | ||
|
|
||
| return recurse(schema); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider abstracting recursion logic into a shared helper function
Both getFieldsWithEmbeddedDocType and doesSchemaContainEmbeddedDocType perform similar recursive traversals of the schema. To reduce code duplication and improve maintainability, consider extracting the recursive traversal into a shared helper function.
You might refactor the code as follows:
+function traverseSchema(
+ schema: Schema,
+ embeddedDocType: string,
+ action: (field: Field) => boolean | void
+): boolean {
+ for (const field of Object.values(schema ?? {})) {
+ if (field.embeddedDocType === embeddedDocType) {
+ const shouldStop = action(field);
+ if (shouldStop) {
+ return true; // Early exit if needed
+ }
+ }
+ if (field.fields) {
+ const shouldStop = traverseSchema(field.fields, embeddedDocType, action);
+ if (shouldStop) {
+ return true;
+ }
+ }
+ }
+ return false;
+}
+
export function getFieldsWithEmbeddedDocType(
schema: Schema,
embeddedDocType: string
): Field[] {
const result: Field[] = [];
- function recurse(schema: Schema) {
- for (const field of Object.values(schema ?? {})) {
- if (field.embeddedDocType === embeddedDocType) {
- result.push(field);
- }
- if (field.fields) {
- recurse(field.fields);
- }
- }
- }
- recurse(schema);
+ traverseSchema(schema, embeddedDocType, (field) => {
+ result.push(field);
+ });
return result;
}
export function doesSchemaContainEmbeddedDocType(
schema: Schema,
embeddedDocType: string
): boolean {
- function recurse(schema: Schema): boolean {
- return Object.values(schema ?? {}).some((field) => {
- if (field.embeddedDocType === embeddedDocType) {
- return true;
- }
- if (field.fields) {
- return recurse(field.fields);
- }
- return false;
- });
- }
- return recurse(schema);
+ let found = false;
+ traverseSchema(schema, embeddedDocType, () => {
+ found = true;
+ return true; // Early exit
+ });
+ return found;
}📝 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.
| export function doesSchemaContainEmbeddedDocType( | |
| schema: Schema, | |
| embeddedDocType: string | |
| ): boolean { | |
| function recurse(schema: Schema): boolean { | |
| return Object.values(schema ?? {}).some((field) => { | |
| if (field.embeddedDocType === embeddedDocType) { | |
| return true; | |
| } | |
| if (field.fields) { | |
| return recurse(field.fields); | |
| } | |
| return false; | |
| }); | |
| } | |
| return recurse(schema); | |
| } | |
| function traverseSchema( | |
| schema: Schema, | |
| embeddedDocType: string, | |
| action: (field: Field) => boolean | void | |
| ): boolean { | |
| for (const field of Object.values(schema ?? {})) { | |
| if (field.embeddedDocType === embeddedDocType) { | |
| const shouldStop = action(field); | |
| if (shouldStop) { | |
| return true; // Early exit if needed | |
| } | |
| } | |
| if (field.fields) { | |
| const shouldStop = traverseSchema(field.fields, embeddedDocType, action); | |
| if (shouldStop) { | |
| return true; | |
| } | |
| } | |
| } | |
| return false; | |
| } | |
| export function getFieldsWithEmbeddedDocType( | |
| schema: Schema, | |
| embeddedDocType: string | |
| ): Field[] { | |
| const result: Field[] = []; | |
| traverseSchema(schema, embeddedDocType, (field) => { | |
| result.push(field); | |
| }); | |
| return result; | |
| } | |
| export function doesSchemaContainEmbeddedDocType( | |
| schema: Schema, | |
| embeddedDocType: string | |
| ): boolean { | |
| let found = false; | |
| traverseSchema(schema, embeddedDocType, () => { | |
| found = true; | |
| return true; // Early exit | |
| }); | |
| return found; | |
| } |
| useEffect(() => { | ||
| const load = () => { | ||
| const duration = looker.getVideo().duration; | ||
| setTotalFrames(getFrameNumber(duration, duration, frameRate)); |
There was a problem hiding this comment.
Possible incorrect parameters in getFrameNumber function call
In line 28, getFrameNumber is called with duration passed twice:
setTotalFrames(getFrameNumber(duration, duration, frameRate));Please verify whether using duration for both the time and duration parameters is intentional. If getFrameNumber expects different values for time and duration, this might lead to incorrect frame calculations.
| ...SCHEMA.embedded.fields!.field, | ||
| pathWithDbField: "", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider avoiding the non-null assertion operator (!).
The use of fields! in ...SCHEMA.embedded.fields!.field assumes that fields is always defined. To enhance code safety, consider using optional chaining to handle cases where fields might be undefined.
Apply this diff to use optional chaining:
- ...SCHEMA.embedded.fields!.field,
+ ...SCHEMA.embedded.fields?.field,📝 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.
| ...SCHEMA.embedded.fields!.field, | |
| pathWithDbField: "", | |
| }); | |
| ...SCHEMA.embedded.fields?.field, | |
| pathWithDbField: "", | |
| }); |
| SCHEMA.embedded.fields!.field, | ||
| SCHEMA.embeddedWithDbFields.fields!.sample_id, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the use of non-null assertion operator in array construction.
Using fields! in SCHEMA.embedded.fields!.field and SCHEMA.embeddedWithDbFields.fields!.sample_id assumes that fields is not null or undefined. If there's a possibility that fields might be undefined, consider using optional chaining to prevent runtime errors.
Apply this diff to use optional chaining:
- SCHEMA.embedded.fields!.field,
- SCHEMA.embeddedWithDbFields.fields!.sample_id,
+ SCHEMA.embedded.fields?.field,
+ SCHEMA.embeddedWithDbFields.fields?.sample_id,📝 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.
| SCHEMA.embedded.fields!.field, | |
| SCHEMA.embeddedWithDbFields.fields!.sample_id, | |
| SCHEMA.embedded.fields?.field, | |
| SCHEMA.embeddedWithDbFields.fields?.sample_id, |
| @@ -17,13 +17,14 @@ import React, { | |||
| import { useErrorHandler } from "react-error-boundary"; | |||
| import { useRecoilValue, useSetRecoilState } from "recoil"; | |||
| import { v4 as uuid } from "uuid"; | |||
| import { useInitializeImaVidSubscriptions, useModalContext } from "./hooks"; | |||
| import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker"; | |||
There was a problem hiding this comment.
Update import path for hooks
useClearSelectedLabels and useShowOverlays should be imported from "./hooks" instead of "./ModalLooker" to reflect their new location.
Apply this diff to fix the import path:
-import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker";
+import { useClearSelectedLabels, useShowOverlays } from "./hooks";📝 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.
| import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker"; | |
| import { useClearSelectedLabels, useShowOverlays } from "./hooks"; |
| if (this.state.config.enableTimeline) { | ||
| getDefaultStore().set(setFrameNumberAtom, { | ||
| name: `timeline-${this.state.config.sampleId}`, | ||
| newFrameNumber: this.state.frameNumber, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid direct manipulation of the Jotai store
Directly accessing and modifying the default Jotai store with getDefaultStore().set can lead to unexpected side effects and bypasses React's state management practices. Instead, consider using Jotai's useSetAtom hook within a React component to update the setFrameNumberAtom.
To align with best practices, refactor the code to use the useSetAtom hook:
- if (this.state.config.enableTimeline) {
- getDefaultStore().set(setFrameNumberAtom, {
- name: `timeline-${this.state.config.sampleId}`,
- newFrameNumber: this.state.frameNumber,
- });
- }Move the state update logic into a React component and update the atom using useSetAtom:
import { useSetAtom } from 'jotai';
const YourComponent = () => {
const setFrameNumber = useSetAtom(setFrameNumberAtom);
useEffect(() => {
if (state.config.enableTimeline) {
setFrameNumber({
name: `timeline-${state.config.sampleId}`,
newFrameNumber: state.frameNumber,
});
}
}, [state.config.enableTimeline, state.frameNumber]);
};| getVideo() { | ||
| return this.lookerElement.children[0].element as HTMLVideoElement; | ||
| } |
There was a problem hiding this comment.
Add null checks to prevent potential runtime errors
The getVideo() method assumes that this.lookerElement.children[0] exists. If children is empty, accessing children[0] will result in an undefined reference error. To enhance robustness, add a check to ensure the child element exists before accessing its element property.
Apply this diff to add safety checks:
getVideo() {
- return this.lookerElement.children[0].element as HTMLVideoElement;
+ const child = this.lookerElement.children[0];
+ if (child && child.element) {
+ return child.element as HTMLVideoElement;
+ } else {
+ return null;
+ }
}📝 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.
| getVideo() { | |
| return this.lookerElement.children[0].element as HTMLVideoElement; | |
| } | |
| getVideo() { | |
| const child = this.lookerElement.children[0]; | |
| if (child && child.element) { | |
| return child.element as HTMLVideoElement; | |
| } else { | |
| return null; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/plugins/src/externalize.ts (1)
6-6: Overall changes look good, but consider long-term implications.The changes to integrate the new plugins module (
@fiftyone/plugins) into the global scope are well-implemented and consistent with the existing patterns in this file. All necessary steps - importing the module, declaring it in the global Window interface, and assigning it to the window object - are correctly done.However, as a future consideration, it might be worth evaluating the long-term strategy for managing these global assignments. As more modules are added, this approach could potentially lead to global namespace pollution. Consider exploring alternative patterns, such as a centralized registry or a more modular approach, to manage plugin dependencies in the future.
Also applies to: 23-23, 41-41
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/packages/plugins/src/externalize.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/plugins/src/externalize.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 (3)
app/packages/plugins/src/externalize.ts (3)
6-6: LGTM: New import statement is consistent with existing patterns.The addition of the import statement for the "@fiftyone/plugins" module follows the established pattern in this file. It's correctly importing all exports and assigning them to the
fopnamespace, which is consistent with other module imports.
23-23: LGTM: Consistent addition to global Window interface.The new property
__fop__is correctly added to the global Window interface. It follows the established pattern in this file, using thetypeofoperator to assign the type of the importedfopmodule. This addition maintains consistency with how other modules are integrated into the global scope.
41-41: LGTM: Consistent assignment of new module to window object.The assignment of the
fopmodule towindow.__fop__is correctly implemented. It follows the established pattern in this file and is properly placed within the check for the existence of the window object. This ensures that the plugins module is accessible globally in a manner consistent with other modules.
Merge
release/v1.0.1todevelopSummary by CodeRabbit
New Features
VideoLookerReactcomponent for video playback with timeline management.RrdFileclass for handling rerun data files, including metadata storage.Bug Fixes
Documentation
Chores