Release/v1.0.1 #4914
Release/v1.0.1 #4914
Conversation
Remove deprecated fiftyone-desktop refs
Fix COCO category IDs
* 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
* release notes * negative wait note --------- Co-authored-by: Benjamin Kane <[email protected]>
WalkthroughThe pull request introduces significant changes across various components in the application, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (41)
fiftyone/utils/rerun.py (2)
14-25: LGTM: Class definition and fields are well-structured.The
RrdFileclass is well-defined with appropriate inheritance and a clear docstring. The_MEDIA_FIELDattribute and field definitions are correct for the intended purpose.Consider adding type hints to the
filepathandversionfields for improved code clarity:- filepath = fof.StringField() - version = fof.StringField() + filepath: fof.StringField = fof.StringField() + version: fof.StringField = fof.StringField()
1-25: Overall assessment: Well-implemented utility for Rerun integration.This new file introduces a clean and concise implementation for working with Rerun data files. The
RrdFileclass is well-structured and follows good practices. The addition of this utility aligns with the PR objectives for the release version 1.0.1.As the project continues to evolve, consider the following suggestions for future improvements:
- If the Rerun integration becomes more complex, consider creating a separate module for Rerun-related utilities.
- Implement unit tests for the
RrdFileclass to ensure its functionality and maintain code quality as the project grows.app/packages/utilities/src/schema.ts (2)
55-74: Approve implementation with suggestions for improvementThe
getFieldsWithEmbeddedDocTypefunction is well-implemented and efficiently traverses the schema to find fields with the specifiedembeddedDocType. It adheres to TypeScript best practices with proper type annotations.To further improve the function, consider the following suggestions:
- Add input validation for the
schemaparameter to ensure it's not null or undefined.- Consider adding a type guard for the
field.fieldscheck to improve type safety.Here's an example of how you could implement these suggestions:
export function getFieldsWithEmbeddedDocType( schema: Schema, embeddedDocType: string ): Field[] { if (!schema) { throw new Error("Schema is required"); } const result: Field[] = []; function recurse(schema: Schema) { for (const field of Object.values(schema)) { if (field.embeddedDocType === embeddedDocType) { result.push(field); } if (field.fields && typeof field.fields === "object") { recurse(field.fields); } } } recurse(schema); return result; }These changes will make the function more robust and type-safe.
76-93: Approve implementation with suggestions for minor improvementsThe
doesSchemaContainEmbeddedDocTypefunction is well-implemented, efficiently using recursion andArray.some()to search for the specifiedembeddedDocType. It's concise, readable, and adheres to TypeScript best practices.To further enhance the function, consider the following suggestions:
- Add input validation for the
schemaparameter.- Use a type guard for the
field.fieldscheck to improve type safety.Here's an example of how you could implement these suggestions:
export function doesSchemaContainEmbeddedDocType( schema: Schema, embeddedDocType: string ): boolean { if (!schema) { throw new Error("Schema is required"); } function recurse(schema: Schema): boolean { return Object.values(schema).some((field) => { if (field.embeddedDocType === embeddedDocType) { return true; } if (field.fields && typeof field.fields === "object") { return recurse(field.fields); } return false; }); } return recurse(schema); }These minor changes will make the function more robust and type-safe while maintaining its efficiency.
app/packages/core/src/components/Modal/hooks.ts (1)
22-41: Good implementation of theuseLookerOptionsUpdatehook.The hook is well-structured and follows React and Recoil best practices. It correctly handles asynchronous operations and provides a flexible way to update Looker options.
Consider improving type definitions.
To enhance type safety and code clarity:
- Replace the
{}type with a more specific interface or type for theupdateparameter.- Add a return type for the hook.
Here's a suggested improvement:
interface LookerOptions { // Define the structure of your options here showJSON?: boolean; showHelp?: boolean; // Add other properties as needed } export const useLookerOptionsUpdate = (): ( update: Partial<LookerOptions>, updater?: (updated: LookerOptions) => void ) => Promise<void> => { return useRecoilCallback( ({ snapshot, set }) => async (update: Partial<LookerOptions>, updater?: (updated: LookerOptions) => void) => { // ... rest of the implementation } ); };This change will provide better type checking and autocompletion for the
updateobject and make the hook's return type explicit.🧰 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/common/looker.ts (1)
29-33: Improved state structure in event handlersThe changes to state destructuring in both
keydownandkeyupevent handlers are good improvements:
- Moving
shouldHandleKeyEventsunderoptionssuggests a more organized state structure.- The core logic remains unchanged, preserving existing functionality.
For consistency, consider applying the same destructuring pattern to the
keyuphandler:keyup: ({ event, update, dispatchEvent }) => { // ... update(({ SHORTCUTS, error, options: { shouldHandleKeyEvents } }) => { // ... (rest of the code remains the same) }); },This change would make both event handlers consistent in their state access pattern.
Also applies to: 52-52
e2e-pw/src/oss/poms/modal/imavid-controls.ts (2)
35-42: LGTM! Consider adding a timeout for robustness.The
waitUntilBufferingIsFinishedmethod is well-implemented and uses appropriate data attributes for testing. However, to prevent potential infinite waiting, consider adding a timeout to thewaitForFunctioncall.You could modify the method like this:
private async waitUntilBufferingIsFinished(timeout = 30000) { await this.page.waitForFunction( () => document .querySelector("[data-cy=imavid-playhead]") ?.getAttribute("data-playhead-state") !== "buffering", { timeout } ); }This change allows for a configurable timeout with a default of 30 seconds, enhancing the robustness of the method.
44-50: Good improvement! Consider adding error handling and logging.The addition of
waitUntilBufferingIsFinishedbefore toggling play/pause is a valuable improvement. It ensures that the video is ready before performing the action, which should lead to more reliable behavior.To further enhance the robustness of this method, consider adding error handling and logging:
private async togglePlay() { try { await this.waitUntilBufferingIsFinished(); console.log("Buffering finished, proceeding with toggle play"); let currentPlayHeadStatus = await this.playPauseButton.getAttribute( "data-playhead-state" ); const original = currentPlayHeadStatus; console.log(`Original playhead status: ${original}`); // ... rest of the method ... } catch (error) { console.error("Error in togglePlay:", error); throw error; } }This change adds error handling and logging, which can be valuable for debugging and maintaining the test suite.
fiftyone/utils/quickstart.py (1)
Line range hint
14-37: LGTM: Function signature and body updates are correct.The removal of the
desktopparameter from the function signature and the corresponding simplification of the function body are consistent with the intended changes.Update the docstring to reflect the removed
desktopparameter.The function's docstring should be updated to remove the mention of the
desktopparameter for consistency with the new function signature.app/packages/state/src/recoil/modal.ts (2)
Line range hint
26-30: LGTM! Consider updating documentation.The change in the
modalLookeratom type fromAbstractLooker<BaseState> | nulltoLookers | nullis a positive improvement. It aligns with the import changes and likely enhances type safety and code clarity.If there's any documentation referencing the
modalLookeratom, please ensure it's updated to reflect this type change.
72-75: LGTM! Consider a minor optimization.The introduction of optional chaining in
id?.endsWith("-modal")is a great improvement for null safety. It prevents potential runtime errors ifidis null or undefined.For a slight optimization and improved readability, you could consider using nullish coalescing:
return id?.endsWith("-modal") ? id.replace("-modal", "") : id ?? null;This ensures that if
idis undefined, the selector returnsnullinstead ofundefined, maintaining consistency with the selector's return type.app/packages/utilities/src/schema.test.ts (2)
65-65: LGTM: Test suite updates improve readability and robustnessThe formatting changes in the describe block names enhance readability. The update to the spread operator usage in the
getFieldInfotests improves robustness by handling potentially undefined fields.Consider adding a type assertion to
SCHEMA.embedded.fieldsto avoid the non-null assertion operator (!):expect(schema.getFieldInfo("embedded.field", SCHEMA)).toEqual({ ...(SCHEMA.embedded.fields as NonNullable<typeof SCHEMA.embedded.fields>).field, pathWithDbField: "", });This approach provides better type safety without using the non-null assertion operator.
Also applies to: 82-82, 92-92
146-176: LGTM: Solid test suite fordoesSchemaContainEmbeddedDocTypeThe new test suite for
doesSchemaContainEmbeddedDocTypeis well-structured and covers important scenarios, including top-level fields, nested fields, non-existent types, and empty schemas.Consider adding a test case for a deeply nested embeddedDocType to ensure the function works correctly with more complex schema structures. For example:
it("should return true for deeply nested embeddedDocType", () => { const deepSchema = { level1: { fields: { level2: { fields: { level3: { embeddedDocType: "fiftyone.core.labels.DeepLabel" } } } } } }; expect( schema.doesSchemaContainEmbeddedDocType(deepSchema, "fiftyone.core.labels.DeepLabel") ).toBe(true); });This addition would further strengthen the test coverage.
app/packages/state/src/hooks/useCreateLooker.ts (2)
39-40: Good addition, but documentation neededThe addition of the optional
enableTimelineparameter is a good way to extend the functionality while maintaining backward compatibility. However, it would be beneficial to add a JSDoc comment explaining the purpose and usage of this new parameter.Consider adding a JSDoc comment like this:
/** * @param enableTimeline - Optional. When true, enables timeline-related functionality. */
116-116: Good integration, consider type assertionThe addition of
enableTimelineto theconfigobject correctly integrates the new parameter. For added safety, consider using a type assertion to ensureenableTimelineis always a boolean:enableTimeline: enableTimeline as boolean,This will help catch potential type-related issues early in development.
app/packages/looker/src/state.ts (2)
178-178: LGTM with a minor suggestion.The addition of
shouldHandleKeyEventsas an optional boolean property is a good improvement for more granular control over key event handling. It follows TypeScript best practices and naming conventions.Consider adding a JSDoc comment to explain the purpose and default behavior of this property, which would enhance code readability and maintainability.
222-222: LGTM with a minor suggestion.The addition of
enableTimelineas a required boolean property in theVideoConfiginterface is a good enhancement for controlling timeline functionality in video configurations. It follows TypeScript best practices and naming conventions.Consider adding a JSDoc comment to explain the purpose and impact of this property on the video configuration, which would improve code documentation and assist other developers in understanding its usage.
docs/source/integrations/coco.rst (4)
Line range hint
219-230: Great update to the COCO format example!The addition of more fields in the "images" section provides a more comprehensive and realistic representation of the COCO format. This improvement helps users better understand the structure of COCO-formatted data.
Consider adding a brief explanation of these new fields (especially "license" and "coco_url") in the text preceding or following this example to provide context for users who might be unfamiliar with the COCO format.
240-247: Excellent updates to the annotation example!The changes to the "bbox" values and the addition of the "area" field make this example more realistic and informative. Using floating-point numbers for bounding box coordinates provides a more accurate representation of how annotations are typically stored in COCO format.
Consider adding a brief explanation of the "area" field and its significance in COCO annotations. This would help users understand why this information is included and how it might be used in object detection tasks.
270-272: Great addition showing COCO category import!This new code snippet effectively demonstrates that COCO categories are imported along with the dataset. It's a valuable addition that helps users understand how to access and use the original COCO category information in their FiftyOne datasets.
Consider adding a brief explanation of how users might utilize this category information in their workflows. For example, you could mention that these categories can be useful for filtering samples, generating statistics, or ensuring consistency when working with multiple COCO datasets.
319-328: Excellent updates to the COCO predictions example!The changes in this section improve the clarity and correctness of the example for adding COCO predictions to a dataset. The updated comment about
category_idand the explicit definition of thecategoriesvariable provide a more accurate representation of how to work with COCO categories in FiftyOne.To further enhance clarity, consider adding a brief explanation of why it's important to use
coco_dataset.info["categories"]instead ofcoco_dataset.default_classes. This could help users understand the significance of maintaining consistency between the imported COCO categories and the predictions being added.fiftyone/core/session/session.py (1)
Line range hint
1128-1139: LGTM! Consider adding a docstring update.The changes to the
waitmethod improve its behavior and readability. The use of the_wait_closedflag for non-negative wait times is a good approach.Consider updating the method's docstring to reflect the new behavior for negative
waitvalues, which now wait indefinitely instead of returning immediately.docs/source/user_guide/app.rst (10)
Line range hint
1-27: LGTM! Consider adding a brief example.The introduction and "App environments" section provide a good overview of the FiftyOne App and its flexibility. The mention of the plugin framework is valuable information for users looking to extend the App's functionality.
Consider adding a brief one-line code example of how to launch the App to give readers an immediate sense of its usage.
Line range hint
28-153: LGTM! Consider clarifying thesession.wait()behavior.The "Sessions" section provides excellent guidance on creating and managing App sessions, with helpful code examples for various scenarios. The notes about asynchronous behavior and platform-specific considerations are valuable.
In the note about
session.wait(), consider clarifying what happens when the App is closed manually. Does the script continue execution, or does it terminate? This information could be helpful for users writing scripts that depend on App interaction.
Line range hint
154-516: LGTM! Consider adding a note about performance implications.The "Using the sidebar" section provides comprehensive coverage of the sidebar functionality, including filtering, indexing, and customization options. The explanations and code examples are clear and informative.
Consider adding a brief note about the performance implications of creating indexes, especially for large datasets. This could help users make informed decisions about when and how to use indexing.
Line range hint
517-534: Consider expanding the "Using the view bar" section.While this section provides a basic introduction to the view bar, it could be more informative for users.
Consider expanding this section with the following:
- More detailed explanation of what operations are available in the view bar.
- A code example showing how to access and use the
Session.viewproperty.- A brief explanation of how the view bar interacts with other App features like the sidebar filters.
Line range hint
535-570: LGTM! Consider adding a code example.The "Grouping samples" section effectively explains the dynamic grouping functionality in the App, including navigation options and special features for ordered groups. The accompanying GIFs are helpful for visualizing the process.
Consider adding a brief code example showing how to programmatically create a grouped view of a dataset. This could help users understand how the App's grouping feature relates to the underlying data structure.
Line range hint
571-686: LGTM! Consider adding a note about performance.The "Field visibility" section provides excellent guidance on configuring field visibility in the App, covering both manual selection and filter rules. The explanations are clear and the accompanying visuals are helpful.
Consider adding a brief note about the performance implications of showing/hiding fields, especially for datasets with many fields or large numbers of samples. This could help users optimize their App experience when working with complex datasets.
Line range hint
687-968: LGTM! Consider adding a note about color accessibility.The "Color schemes" section provides excellent guidance on configuring color schemes both in the App and programmatically. The explanations of different customization options are clear and the code examples are helpful.
Consider adding a brief note about color accessibility considerations when choosing custom color schemes. This could include tips for ensuring sufficient contrast and avoiding color combinations that might be difficult for colorblind users to distinguish.
Line range hint
969-1093: LGTM! Consider adding information about view sharing.The "Saving views" and "Viewing a sample" sections effectively explain how to save and load views, and how to interact with individual samples in the App. The information about persisting views and customizing tooltips is particularly valuable.
Consider adding information about whether saved views can be shared between users or exported/imported between different FiftyOne installations. This could be useful for collaborative workflows or for users working across multiple environments.
Line range hint
1094-1604: LGTM! Consider adding information about custom visualizers.The sections on image, video, and 3D visualizers, as well as selecting samples and labels, provide excellent guidance on using these features in the App. The explanations are clear and the inclusion of keyboard shortcuts is very helpful.
Consider adding information about whether users can create custom visualizers or extend the existing ones. This could be valuable for users with specialized visualization needs that aren't covered by the built-in visualizers.
Line range hint
1605-2453: LGTM! Consider adding a troubleshooting section.The final sections covering tagging, object patches, evaluation patches, video clips, similarity sorting, multiple media fields, and App configuration provide excellent coverage of advanced features and customization options. The explanations and code examples are clear and informative.
Consider adding a brief troubleshooting section at the end of the document. This could cover common issues users might encounter when using the App and provide solutions or workarounds. This addition could be very helpful for users, especially those new to FiftyOne.
docs/source/user_guide/export_datasets.rst (2)
Line range hint
4-4: Address the TODO comment for 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.
Would you like assistance in generating unit tests for this function?
Line range hint
12-24: Revise discount and fee structure for better customer experienceThe current implementation has some issues that could lead to customer dissatisfaction:
- The flat $20 fee on discounted bills can result in a higher final price for small purchases, especially with the 10% discount.
- This structure might discourage customer loyalty for those making smaller, frequent purchases.
Consider revising the logic to ensure that discounted prices are always lower than non-discounted prices. Some suggestions:
- Apply the flat fee before the discount.
- Use a percentage-based fee instead of a flat fee.
- Only apply the fee for purchases above a certain threshold.
Would you like assistance in designing and implementing a more customer-friendly pricing structure?
app/packages/core/src/components/Modal/use-key-events.ts (1)
39-39: Removereffrom the dependency arrayIncluding
refin the dependency array is unnecessary because changes toref.currentdo not trigger re-renders, and including it may cause unintended effects.Apply this diff to remove
reffrom the dependency array:- }, [hoveredId, id, looker, ref]); + }, [hoveredId, id, looker]);app/packages/core/src/components/Modal/VideoLooker.tsx (3)
21-23:useMemomay not be necessary forframeRateSince accessing
sample.frameRateis inexpensive and unlikely to cause performance issues, you can simplify the code by directly assigningframeRatewithout usinguseMemo.Apply this diff to simplify:
- const frameRate = useMemo(() => { - return sample.frameRate; - }, [sample]); + const frameRate = sample.frameRate;
61-61:useMemomay not be necessary fortimelineNameCalling
getName()is likely inexpensive, and its return value doesn't change between renders. You can simplify the code by directly assigningtimelineNamewithout usingReact.useMemo.Apply this diff to simplify:
- const timelineName = React.useMemo(() => getName(), [getName]); + const timelineName = getName();
65-70: Simplify theconfigassignment sincetotalFramesis always definedBecause
TimelineControlleris only rendered whentotalFramesis defined, the conditional check is unnecessary. You can simplify the code by directly assigning theconfigobject.Apply this diff to simplify:
- config: totalFrames - ? { - totalFrames, - loop: true, - } - : undefined, + config: { + totalFrames, + loop: true, + },app/packages/core/src/components/Modal/ModalLooker.tsx (1)
86-88: Consider usingelse iffor conditional rendering to improve clarityTo improve readability and ensure that the conditions are mutually exclusive, consider changing the second
ifto anelse if. This ensures that once a condition is met, subsequent conditions are not unnecessarily evaluated.Apply this diff to merge the conditions:
if (shouldRenderImavid) { return <ImaVidLookerReact sample={sample} />; } - if (video) { + else if (video) { return <VideoLookerReact sample={sample} />; } return <ModalLookerNoTimeline sample={sample} />;app/packages/looker/src/elements/common/bubbles.ts (1)
Line range hint
98-102: Fix incorrect parameter passing inunwindfunctionThe
unwindfunction incorrectly passes thedepthparameter to recursive calls due to a misuse of theArray.mapmethod. The second argument ofArray.mapisthisArg, not an argument for the callback function. This can lead to unintended behavior in recursion depth.Apply this diff to correctly pass the
nextdepth parameter to recursive calls:export const unwind = (name: string, value: Data | Data[], depth = 0) => { if (Array.isArray(value)) { const next = depth + 1; - return depth < 2 ? value.map((val) => unwind(name, val), next).flat(3) : []; + return depth < 2 ? value.map((val) => unwind(name, val, next)).flat(3) : []; } const v = value[name];fiftyone/utils/coco.py (1)
132-139: Documentation Update: Ensure Proper Linking and FormattingThe updated documentation for the
categoriesparameter is helpful. Please verify that the reference to:meth:\parse_coco_categories`` correctly links to the intended method and is properly formatted in the generated documentation.
📜 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 (35)
- 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/bubbles.test.ts (2 hunks)
- app/packages/looker/src/elements/common/bubbles.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/plugins/src/externalize.ts (3 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (4 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)
- docs/source/integrations/coco.rst (6 hunks)
- docs/source/release-notes.rst (1 hunks)
- docs/source/user_guide/app.rst (1 hunks)
- docs/source/user_guide/dataset_creation/datasets.rst (2 hunks)
- docs/source/user_guide/export_datasets.rst (2 hunks)
- e2e-pw/src/oss/poms/modal/imavid-controls.ts (1 hunks)
- fiftyone/constants.py (1 hunks)
- fiftyone/core/session/session.py (1 hunks)
- fiftyone/server/metadata.py (2 hunks)
- fiftyone/utils/coco.py (15 hunks)
- fiftyone/utils/quickstart.py (2 hunks)
- fiftyone/utils/rerun.py (1 hunks)
- setup.py (1 hunks)
- tests/unittests/import_export_tests.py (2 hunks)
🧰 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/bubbles.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/looker/src/elements/common/bubbles.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/imavid/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/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.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/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.e2e-pw/src/oss/poms/modal/imavid-controls.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)
🪛 Ruff
tests/unittests/import_export_tests.py
1822-1822: Ambiguous variable name:
l(E741)
🔇 Additional comments (43)
app/packages/core/src/components/Modal/utils.ts (1)
1-7:⚠️ Potential issueImprove type safety and clarity of the
shortcutToHelpItemsfunctionThe current implementation has several areas for improvement:
- Add type annotations to enhance type safety and code clarity.
- Clarify the expected structure of the
SHORTCUTSparameter.- Add error handling for potential edge cases.
- Use modern JavaScript/TypeScript features to make the code more concise and robust.
Here's a suggested refactored version with improvements:
interface ShortcutItem { shortcut: string; [key: string]: any; } export function shortcutToHelpItems(shortcuts: Record<string, ShortcutItem>): ShortcutItem[] { if (!shortcuts || typeof shortcuts !== 'object') { throw new Error('SHORTCUTS must be a non-null object'); } const uniqueShortcuts = new Map<string, ShortcutItem>(); Object.values(shortcuts).forEach(item => { if (typeof item.shortcut !== 'string') { throw new Error('Each shortcut item must have a string "shortcut" property'); } uniqueShortcuts.set(item.shortcut, item); }); return Array.from(uniqueShortcuts.values()); }This refactored version:
- Adds type annotations for better type safety.
- Uses a
Mapto ensure uniqueness based on theshortcutproperty.- Adds error handling for invalid input.
- Uses modern JavaScript features like
Object.values()andArray.from().Please consider implementing these changes to improve the function's robustness and maintainability.
To ensure that this function is used correctly throughout the codebase, we can run the following verification:
This will help us understand how the function is being used and ensure that
SHORTCUTSis properly defined.app/packages/looker/src/index.ts (1)
6-6: New export added: Verify its necessity and usageThe addition of
getFrameNumberto the exports is a minor change that expands the public API of this module. This change appears to be in line with the module's purpose of providing utility functions for video processing.However, to ensure this change aligns with best practices:
- Confirm that
getFrameNumberis indeed intended to be part of the public API.- Verify that this new export is used in other parts of the codebase or is required for external usage.
- Ensure that
getFrameNumberis properly documented in its source file.To verify the usage and necessity of this new export, you can run the following script:
This will help ensure that the new export is necessary and properly integrated into the codebase.
fiftyone/utils/rerun.py (1)
1-11: LGTM: File structure and imports are well-organized.The file structure follows best practices with a clear module docstring, including a copyright notice. The imports are relevant to the functionality being implemented.
app/packages/plugins/src/externalize.ts (4)
6-6: LGTM: New import statement is consistent with existing patterns.The new import for
@fiftyone/pluginsfollows the established naming convention and import style used throughout the file.
23-23: LGTM: New Window interface property is consistent with existing patterns.The addition of
__fop__: typeof fop;to the global Window interface maintains consistency with the existing structure and naming conventions.
41-41: LGTM: New global variable assignment is consistent with existing patterns.The assignment of
window.__fop__ = fop;maintains consistency with the existing structure and is correctly placed within the window existence check.
6-6: Summary: Successful externalization of @fiftyone/plugins moduleThe changes in this file effectively externalize the
@fiftyone/pluginsmodule, making it globally accessible aswindow.__fop__. This modification:
- Imports the module (line 6)
- Declares it in the global Window interface (line 23)
- Assigns it to the window object (line 41)
These changes are consistent with the existing patterns in the file and should allow plugins to access the
@fiftyone/pluginsmodule in the same way as other externalized modules.Also applies to: 23-23, 41-41
fiftyone/constants.py (1)
45-45: LGTM: Version compatibility range updated appropriatelyThe update to
COMPATIBLE_VERSIONSfrom ">=0.19,<0.25" to ">=0.19,<1.1" is appropriate for the 1.0.1 release. It maintains backward compatibility while allowing for future 1.0.x releases. This change aligns well with semantic versioning practices.To ensure this change doesn't introduce any unexpected issues, please run the following verification script:
This script will help identify any places where the old version string might still be used or where
COMPATIBLE_VERSIONSis referenced, ensuring consistency across the codebase.✅ Verification successful
Verified: COMPATIBLE_VERSIONS updated consistently across the codebase
The
COMPATIBLE_VERSIONSconstant has been updated to ">=0.19,<1.1" consistently across all relevant files. No hardcoded instances of the old version string ">=0.19,<0.25" were found, ensuring compatibility updates are correctly applied.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the COMPATIBLE_VERSIONS constant is used consistently across the codebase # Test: Search for uses of COMPATIBLE_VERSIONS echo "Searching for uses of COMPATIBLE_VERSIONS:" rg --type python -A 5 "COMPATIBLE_VERSIONS" # Test: Check if there are any hardcoded version strings that might need updating echo "Checking for hardcoded version strings:" rg --type python '">=0\.19,<0\.25"'Length of output: 327
Script:
#!/bin/bash # Description: Verify that the COMPATIBLE_VERSIONS constant is used consistently across the codebase # Test: Search for uses of COMPATIBLE_VERSIONS echo "Searching for uses of COMPATIBLE_VERSIONS:" rg --type py -A 5 "COMPATIBLE_VERSIONS" # Test: Check if there are any hardcoded version strings that might need updating echo "Checking for hardcoded version strings:" rg --type py '">=0\.19,<0\.25"'Length of output: 2201
app/packages/looker/src/elements/base.ts (1)
67-67: Excellent use of optional chaining operatorThe change from
this.element && this.element.addEventListener(eventType, this.events[eventType]);tothis.element?.addEventListener(eventType, this.events[eventType]);is a great improvement. It simplifies the code while maintaining the same functionality of safely callingaddEventListeneronly ifthis.elementexists.This use of the optional chaining operator (
?.) is a modern TypeScript feature that aligns with best practices. It makes the code more concise and easier to read without changing its behavior.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, andEventshelps with type checking without importing the actual values, which can lead to better tree-shaking in the final bundle.- Separating the import of
ControlEventKeyTypeindicates it's used as a value in the code, not just for type information.These modifications align well with TypeScript best practices and can potentially improve build optimization.
fiftyone/utils/quickstart.py (4)
42-45: LGTM: Function signature and body updates are correct.The removal of the
desktopparameter from the function signature and the corresponding update in the function call are consistent with the intended changes.
48-51: LGTM: Function signature and body updates are correct.The removal of the
desktopparameter from the function signature and the corresponding update in the function call are consistent with the intended changes.
Line range hint
1-162: Summary: Consistent removal ofdesktopparameter improves code simplicity.The changes in this file consistently remove the
desktopparameter from all relevant function signatures and calls. This simplification aligns with the PR objectives and improves the overall clarity of the codebase. The functionality related to launching the app remains intact, with the desktop-specific option removed.Key points:
- All functions (
quickstart,_quickstart,_video_quickstart, and_launch_app) have been updated consistently.- The changes simplify the API by removing the desktop-specific option.
- The docstring for the
quickstartfunction should be updated to reflect the parameter removal.These changes contribute positively to the codebase by streamlining the quickstart functionality.
54-62: LGTM: Function signature and body updates are correct.The removal of the
desktopparameter from the function signature and the corresponding update in thefos.launch_appcall are consistent with the intended changes.Verify that the
fos.launch_appfunction has been updated to remove thedesktopparameter in its definition:✅ Verification successful
Verified:
fos.launch_appno longer accepts the 'desktop' parameter.The removal of the
desktopparameter from thefos.launch_appfunction signature has been confirmed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that fos.launch_app function no longer accepts a 'desktop' parameter # Test: Search for the fos.launch_app function definition rg --type python -A 10 'def launch_app\('Length of output: 74
Script:
#!/bin/bash # Description: Verify that fos.launch_app function no longer accepts a 'desktop' parameter # Test: Search for the fos.launch_app function definition using the correct file type rg --type py -A 10 'def launch_app\('Length of output: 715
app/packages/state/src/utils.ts (3)
21-21: LGTM: Import ofModalSampleis correct and consistent.The addition of
ModalSampleto the import statement is appropriate and aligns with the changes made to thegetStandardizedUrlsfunction. This import follows TypeScript best practices.
Line range hint
1-24: Summary: Minor changes to supportModalSampleintegrationThe changes in this file are minimal and focused on integrating
ModalSample:
- Import of
ModalSamplefrom "./recoil"- Extension of the
urlsparameter type ingetStandardizedUrlsfunctionThese modifications suggest a broader refactoring or feature addition involving
ModalSample. The changes appear to be correct and consistent, but it's important to ensure that the new type is compatible with the existing function logic to maintain type safety.To confirm the extent of
ModalSampleusage, run the following command:#!/bin/bash # Description: Check for other usages of ModalSample in the codebase # Expected result: List of files and contexts where ModalSample is used rg --type typescript 'ModalSample'This will help verify that the integration is consistent across the codebase.
Also applies to: 105-111
108-108: LGTM: Type extension forurlsparameter is appropriate.The addition of
ModalSample["urls"]as a valid type for theurlsparameter enhances the function's flexibility. This change is consistent with the new import ofModalSample.To ensure type safety, please verify that
ModalSample["urls"]is compatible with the existing function logic. Run the following command to check theModalSampletype definition:app/packages/looker/src/lookers/imavid/index.ts (1)
18-18: LGTM. Verify usage of the newly exported constant.The addition of this export is good for code organization. It allows other parts of the application to import
BUFFERING_PAUSE_TIMEOUTdirectly.To ensure this change doesn't introduce any issues, please run the following script to check for any existing imports that might need updating:
If the script returns any results, please review those files to ensure they're updated to use the new export path if necessary.
✅ Verification successful
Please re-run the verification with the updated script below to accurately search for
BUFFERING_PAUSE_TIMEOUTusages in TypeScript files:
Verified: The export of
BUFFERING_PAUSE_TIMEOUTdoes not affect other parts of the codebase, as no existing imports or usages were found outside ofconstants.tsandindex.ts. This change is safe to proceed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing imports of BUFFERING_PAUSE_TIMEOUT that might need updating # Search for imports of BUFFERING_PAUSE_TIMEOUT rg --type typescript "import.*BUFFERING_PAUSE_TIMEOUT.*from" -g '!**/constants.ts' # Search for uses of BUFFERING_PAUSE_TIMEOUT without an import rg --type typescript "BUFFERING_PAUSE_TIMEOUT" -g '!**/constants.ts' -g '!**/index.ts'Length of output: 241
Script:
#!/bin/bash # Description: Check for existing imports of BUFFERING_PAUSE_TIMEOUT that might need updating # Search for imports of BUFFERING_PAUSE_TIMEOUT rg "import.*BUFFERING_PAUSE_TIMEOUT.*from" -g "*.ts" -g "*.tsx" # Search for uses of BUFFERING_PAUSE_TIMEOUT without an import rg "BUFFERING_PAUSE_TIMEOUT" -g "*.ts" -g "*.tsx" -g '!**/constants.ts' -g '!**/index.ts'Length of output: 155
app/packages/state/src/recoil/modal.ts (1)
1-6: LGTM! Consider verifying import usage.The changes in imports align well with the updates in the
modalLookeratom type. The removal ofAbstractLookerandBaseStateimports and the addition ofLookersimport suggest a positive shift towards more specific typing.To ensure these changes don't introduce any issues, please run the following script to verify the usage of imports:
✅ Verification successful
Verification Successful!
Theapp/packages/state/src/recoil/modal.tsfile no longer usesAbstractLookerorBaseState, and the newLookersimport is correctly utilized in themodalLookeratom.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of imports in the file # Test 1: Check for any remaining usage of AbstractLooker or BaseState echo "Checking for any remaining usage of AbstractLooker or BaseState:" rg --type typescript "AbstractLooker|BaseState" app/packages/state/src/recoil/modal.ts # Test 2: Verify the usage of the new Lookers import echo "Verifying the usage of the new Lookers import:" rg --type typescript "Lookers" app/packages/state/src/recoil/modal.tsLength of output: 470
Script:
#!/bin/bash # Description: List all supported file types in ripgrep echo "Listing all supported file types in ripgrep:" rg --type-listLength of output: 4920
Script:
#!/bin/bash # Description: Verify the usage of imports without specifying file type # Test 1: Check for any remaining usage of AbstractLooker or BaseState echo "Checking for any remaining usage of AbstractLooker or BaseState without specifying type:" rg "AbstractLooker|BaseState" app/packages/state/src/recoil/modal.ts # Test 2: Verify the usage of the new Lookers import without specifying type echo "Verifying the usage of the new Lookers import without specifying type:" rg "Lookers" app/packages/state/src/recoil/modal.tsLength of output: 551
app/packages/utilities/src/schema.test.ts (3)
22-23: LGTM: Schema definition updates improve consistencyThe changes to the
nameandpathproperties in the schema definition enhance the clarity and consistency of the embedded fields. This improvement aligns well with best practices in schema design.Also applies to: 33-33, 45-46, 57-57
113-144: LGTM: Comprehensive test suite forgetFieldsWithEmbeddedDocTypeThe new test suite for
getFieldsWithEmbeddedDocTypeis well-structured and provides excellent coverage. It tests various scenarios including top-level fields, nested fields, non-existent types, and empty schemas, which helps ensure the robustness of the function.
Line range hint
1-176: LGTM: Excellent improvements to schema definitions and test coverageThe changes in this file significantly enhance the quality and coverage of the tests for schema-related utilities. Key improvements include:
- Updated schema definitions for better consistency and clarity.
- Refactored existing test suites for improved readability and robustness.
- New comprehensive test suites for
getFieldsWithEmbeddedDocTypeanddoesSchemaContainEmbeddedDocType.These changes contribute to a more maintainable and reliable codebase. Great work on expanding the test coverage!
app/packages/looker/src/elements/common/bubbles.test.ts (1)
Line range hint
1-231: LGTM: Overall structure and consistency maintained.The new test cases for
classificationsandtemporalDetectionsare well-integrated into the existing test suite. The file structure remains consistent, and the necessary imports have been added. Good job on maintaining the test file's organization.app/packages/state/src/hooks/useCreateLooker.ts (1)
12-12: Excellent use of type-only import!The change to a type-only import for
BaseStateandImaVidConfigis a good practice in TypeScript. It clearly separates type imports from value imports, which can potentially improve build performance and reduce bundle size.app/packages/core/src/components/Modal/ImaVidLooker.tsx (2)
20-26: Improved import organizationThe changes in the import statements enhance the code organization by moving hook-related imports to a separate 'hooks' file. This follows the best practice of separating concerns and improving modularity.
136-136: Verify the functionality ofuseKeyEventshookThe addition of the
useKeyEventshook simplifies the event handling logic. However, please ensure that this hook fully replaces the functionality of the removeduseEffectfor handling hovered samples. Verify that all necessary key events are still being handled correctly.To verify the functionality, please run the following script:
app/packages/looker/src/state.ts (1)
462-462: LGTM! Good default configuration.The addition of
shouldHandleKeyEvents: truetoDEFAULT_BASE_OPTIONSis appropriate. It sets a sensible default that maintains backwards compatibility with existing behavior.fiftyone/server/metadata.py (3)
27-27: Import statement for RrdFile looks good.The new import for
RrdFilefromfiftyone.utils.rerunis correctly placed and necessary for the subsequent changes in the file.
37-37: Addition of RrdFile to _ADDITIONAL_MEDIA_FIELDS is appropriate.The new entry for
RrdFilein the_ADDITIONAL_MEDIA_FIELDSdictionary is consistent with the existing structure and correctly associates it with the "filepath" field. This addition enables proper metadata handling forRrdFileobjects.
Line range hint
1-1: Summary of changes and recommendation for further investigation.The visible changes in this file correctly implement support for
RrdFileby adding the necessary import and updating the_ADDITIONAL_MEDIA_FIELDSdictionary. However, the AI-generated summary mentions changes to the_create_media_urlsfunction that are not visible in the provided code snippet. It is recommended to:
- Verify that all necessary changes to support
RrdFilehave been implemented, particularly in the_create_media_urlsfunction.- Update the AI-generated summary if it's inaccurate, or provide the missing code changes if they exist in a different part of the file.
These steps will ensure that the implementation is complete and consistent with the intended changes for the new media type.
To get a comprehensive view of all changes in this file, please run the following script:
docs/source/integrations/coco.rst (1)
199-200: Excellent addition of theseedparameter!The inclusion of the
seedparameter in thetake()method enhances reproducibility. This change allows users to consistently select the same subset of samples across different runs, which is crucial for creating reproducible examples and tests.docs/source/user_guide/app.rst (1)
Line range hint
1-2453: Excellent documentation with room for minor enhancements.This comprehensive guide to the FiftyOne App is well-structured, informative, and provides clear explanations for a wide range of features and functionalities. The inclusion of code examples, visual aids, and keyboard shortcuts enhances its usefulness for users at various levels of expertise.
To further improve this already excellent document, consider the following suggestions:
- Add brief code examples in some sections to illustrate programmatic interaction with the App.
- Include notes on performance implications for certain operations, especially for large datasets.
- Provide information on creating custom visualizers or extending existing ones.
- Add a troubleshooting section to address common issues users might encounter.
- Consider adding information about color accessibility when discussing color schemes.
These additions would make the document even more comprehensive and user-friendly.
docs/source/user_guide/dataset_creation/datasets.rst (3)
1503-1503: LGTM: Minor update to COCO dataset exampleThe change from
"id": 2to"id": 1in the COCO dataset example is correct and consistent with the rest of the documentation. This update likely reflects a more common or standardized way of representing category IDs in COCO format.
1526-1526: LGTM: Consistent update to COCO dataset exampleThe change from
"category_id": 2to"category_id": 1in the COCO dataset example is consistent with the previous change to the category ID. This ensures that the example remains internally consistent and accurately represents a typical COCO format annotation.
Line range hint
1-2000: Overall: Documentation updates are accurate and consistentThe changes made to the COCO dataset import example in this documentation file are minor but important for maintaining consistency. The category ID has been updated from 2 to 1 in both the
categoriesandannotationssections of the JSON example. These modifications improve the clarity of the example and ensure that it accurately represents a typical COCO format dataset.No other changes were observed in the rest of the document. The existing content, including other dataset format descriptions, import instructions, and templates for custom importers, remains accurate and comprehensive.
docs/source/release-notes.rst (2)
6-13: Release notes look good!The release notes for FiftyOne Teams 2.1.1 are clear and informative. They properly indicate the release date, include references to the previous version's updates, and highlight two specific improvements.
15-39: Comprehensive release notes for FiftyOne 1.0.1The release notes for FiftyOne 1.0.1 are well-structured and informative. They clearly separate App and Core updates, providing specific details about improvements and bug fixes. The information is relevant and useful for users updating to this version.
app/packages/core/src/components/Modal/use-looker.ts (4)
59-60: Verify the use ofsample.sample.Ensure that accessing
sample.sampleis correct. Ifsamplealready contains the necessary data, double-check whethersample.sampleis the intended reference.
104-104: Check the parameters passed touseKeyEvents.Verify that passing
initialReftouseKeyEventsis appropriate. Confirm thatuseKeyEventsexpects auseRefobject. If not, consider passinginitialRef.currentor adjusting the parameter as needed.
83-102: Validate event handler for "panels" event.Ensure that all properties used within the event handler (e.g.,
showJSON,showHelp,SHORTCUTS) are correctly defined and that the logic aligns with expected behavior. Also, verify thatupdateLookerOptionsandlooker.updateOptionsare used appropriately.
50-52: 🛠️ Refactor suggestionEnsure
looker.updateOptionsis called on initial render if necessary.The condition
!initialRef.currentpreventslooker.updateOptions(lookerOptions)from running on the initial render. If thelookerrequires options to be set during the initial render, consider adjusting this logic.fiftyone/utils/coco.py (1)
48-48:⚠️ Potential issueParameter Rename May Affect Backward Compatibility
The
add_coco_labelsfunction has renamed theclassesparameter tocategories. This change might break backward compatibility for users who rely on the old parameter name. Consider adding support for the old parameter name or updating the documentation to inform users of this change.To ensure there are no unintended references to the old
classesparameter, you can run the following script:✅ Verification successful
Parameter Rename Verified: No References to
classesFoundThe
add_coco_labelsfunction has been successfully renamed fromclassestocategories. No usages of the oldclassesparameter were found in the codebase, ensuring backward compatibility is maintained.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of `add_coco_labels` with the `classes` parameter. # Test: Search for occurrences of `add_coco_labels` using `classes`. rg --type python 'add_coco_labels\([^\)]*classes='Length of output: 83
Script:
#!/bin/bash # Description: Find usages of `add_coco_labels` with the `classes` parameter using ast-grep. ast-grep --lang python --pattern 'add_coco_labels($_, classes=_)'Length of output: 67
Script:
#!/bin/bash # Description: Find usages of `add_coco_labels` with the `classes` parameter in Python files. # Search for occurrences of `add_coco_labels` using `classes` in Python files. rg 'add_coco_labels\s*\([^)]*classes\s*=' -g "*.py"Length of output: 53
tests/unittests/import_export_tests.py (1)
1320-1378: Tests for COCO Detection Dataset are correctly validating category handlingThe added test cases properly verify the default behavior of alphabetized 1-based categories and the functionality of loading only matching classes using the
classesandonly_matchingparameters.
| @@ -13,7 +13,7 @@ | |||
| from setuptools import setup, find_packages | |||
|
|
|||
|
|
|||
| VERSION = "1.0.0" | |||
| VERSION = "1.0.1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Version update incomplete: "1.0.0" still present in multiple files.
The search revealed that the version "1.0.0" still exists in several files across the codebase. To ensure consistency and prevent potential issues in future releases, please update all remaining instances of "1.0.0" to "1.0.1". Key areas that require attention include:
package.jsonyarn.lock- Test files (e.g.,
tests/unittests/utils/openlabel.py) - Documentation files (e.g.,
docs/source/user_guide/dataset_creation/datasets.rst) - Migration scripts (e.g.,
fiftyone/migrations/revisions/v1_0_0.py)
After making these updates, consider rerunning the version search to confirm that all instances have been addressed.
🔗 Analysis chain
Version update looks good and aligns with PR objectives.
The VERSION variable has been correctly updated from "1.0.0" to "1.0.1", which is consistent with the pull request's objective of finalizing the merge for release version 1.0.1. This change is properly reflected in the get_version() function, ensuring that the correct version is used throughout the setup process.
To ensure this is the only required change for the version update, let's verify that no other occurrences of the old version exist:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of version 1.0.0
# Test: Search for "1.0.0" in all files. Expect: No results.
rg --type-not binary "1\.0\.0"
Length of output: 63
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of version 1.0.0
# Test: Search for "1.0.0" in all files. Expect: No results.
rg "1\.0\.0"
Length of output: 32135
| expect( | ||
| getBubbles( | ||
| "classes", | ||
| { classes: { classifications: [{ label: "label" }] } }, | ||
| { classes: classifications } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Complete the test case with an expectation.
The test case for classifications is incomplete. Add an expectation to verify the output of getBubbles. For example:
expect(
getBubbles(
"classes",
{ classes: { classifications: [{ label: "label" }] } },
{ classes: classifications }
)
).toEqual([classifications, [{ classifications: [{ label: "label" }] }]]);This ensures that the getBubbles function correctly handles the classifications field.
| expect( | ||
| getBubbles( | ||
| "temporal", | ||
| { temporal: { detections: [{ label: "label" }] } }, | ||
| { temporal: temporalDetections } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Complete the test case with an expectation.
The test case for temporalDetections is incomplete. Add an expectation to verify the output of getBubbles. For example:
expect(
getBubbles(
"temporal",
{ temporal: { detections: [{ label: "label" }] } },
{ temporal: temporalDetections }
)
).toEqual([temporalDetections, [{ detections: [{ label: "label" }] }]]);This ensures that the getBubbles function correctly handles the temporalDetections field.
| export default function ( | ||
| ref: MutableRefObject<boolean>, | ||
| id: string, | ||
| looker: Lookers | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Name the custom hook for better identification
Since this is a custom React hook, it's a best practice to name it starting with use to follow React conventions and improve debugging and readability.
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) { |
There was a problem hiding this comment.
Add a cleanup function to useEffect to prevent potential memory leaks
To ensure that the event listener is properly removed if the component unmounts before the 'load' event fires, add a cleanup function to useEffect.
Modify the useEffect hook to return a cleanup function:
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]);📝 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) { | |
| 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]); |
| import { setFrameNumberAtom } from "@fiftyone/playback"; | ||
| import { Schema } from "@fiftyone/utilities"; | ||
| import { getDefaultStore } from "jotai"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider avoiding direct use of getDefaultStore() for state management
Directly manipulating atoms with getDefaultStore().set bypasses React's state management and can lead to unexpected side effects or difficult-to-debug issues. It's recommended to use Jotai's useAtom or useSetAtom hooks within React components to update atom values. If you need to update state from outside a React component, consider refactoring the code to pass the store explicitly or use a context provider.
| 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.
🛠️ Refactor suggestion
Avoid directly setting atom state outside of React components
Setting atom values using getDefaultStore().set outside of React components can result in unpredictable behavior and undermine the benefits of React's controlled state updates. It's preferable to handle state changes within React components using hooks like useAtom or useSetAtom. If altering state from non-React code is necessary, consider redesigning the architecture to align with React's state management patterns.
| getVideo() { | ||
| return this.lookerElement.children[0].element as HTMLVideoElement; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure getVideo() method does not expose internal implementation details
The getVideo() method returns a reference to an internal DOM element via this.lookerElement.children[0].element. Exposing internal structure can make the codebase fragile and harder to maintain if the DOM hierarchy changes. Consider abstracting this interaction or providing a more stable interface to interact with the video element.
| 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.
Potential IndexError When Accessing categories[0]
At line 208, the code checks elif isinstance(categories[0], dict): without verifying that categories is not an empty list. If categories is empty, accessing categories[0] will raise an IndexError. Please add a check to ensure that categories is not empty before accessing its first element.
Apply this diff to prevent the possible IndexError:
elif isinstance(categories, dict):
classes_map = categories
elif not categories:
classes_map = {}
-elif isinstance(categories[0], dict):
+elif categories and isinstance(categories[0], dict):
classes_map = {c["id"]: c["name"] for c in categories}
else:
classes_map = {i: label for i, label in enumerate(categories, 1)}Committable suggestion was skipped due to low confidence.
| classes = dataset.distinct("predictions.detections.label") | ||
| categories = [{"id": i, "name": l} for i, l in enumerate(classes, 1)] |
There was a problem hiding this comment.
Replace ambiguous variable name l
Using the variable name l can lead to confusion with the digit 1 or the uppercase I. It is recommended to use a more descriptive variable name for clarity.
Apply this diff to improve code readability:
-categories = [{"id": i, "name": l} for i, l in enumerate(classes, 1)]
+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)] | |
| categories = [{"id": i, "name": label} for i, label in enumerate(classes, 1)] |
🧰 Tools
🪛 Ruff
1822-1822: Ambiguous variable name:
l(E741)
* move modal looker state to use-looker * fix video update sample * linting
What changes are proposed in this pull request?
final merge for release v1.0.1
DO NOT MERGE UNTIL 7AM on 10/11
Summary by CodeRabbit
Release Notes
New Features
VideoLookerReactcomponent for enhanced video playback.Bug Fixes
ImaVidLookerandModalLookercomponents.Documentation
Tests
Chores