Video playback #4878
Video playback #4878
Conversation
WalkthroughThe changes in this pull request involve significant updates to the modal components within the application. Key modifications include restructuring imports, introducing new hooks for event handling, and enhancing the functionality of video playback components. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 11
🧹 Outside diff range and nitpick comments (9)
app/packages/core/src/components/Modal/hooks.ts (1)
22-41: Good implementation, but type safety can be improvedThe
useLookerOptionsUpdatehook is well-implemented and follows React and Recoil best practices. However, there are a few areas where we can enhance type safety and documentation:
- Improve type safety for the
updateparameter:- async (update: object, updater?: (updated: {}) => void) => { + async (update: Partial<LookerOptions>, updater?: (updated: LookerOptions) => void) => {
- Add JSDoc comments for better documentation:
/** * Hook to update Looker options in Recoil state * @returns A function to update Looker options */ export const useLookerOptionsUpdate = () => { return useRecoilCallback( ({ snapshot, set }) => /** * Updates Looker options * @param update - Partial Looker options to update * @param updater - Optional callback function called with updated options */ async (update: Partial<LookerOptions>, updater?: (updated: LookerOptions) => void) => { // ... (rest of the implementation) } ); };
- Consider defining a type for the return value of the hook for improved clarity:
type UpdateLookerOptions = (update: Partial<LookerOptions>, updater?: (updated: LookerOptions) => void) => Promise<void>; export const useLookerOptionsUpdate = (): UpdateLookerOptions => { // ... (rest of the implementation) };These changes will improve type safety, make the code more self-documenting, and enhance maintainability.
🧰 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 (2)
29-33: Improved state structure in keydown event handlerThe changes to the state destructuring are good:
- Moving
shouldHandleKeyEventsunderoptionsimproves the organization of the state object.- This change makes the code more maintainable by grouping related properties.
For consistency, consider destructuring
optionsseparately:const { SHORTCUTS, error, options } = state; const { shouldHandleKeyEvents } = options;This approach would make it easier to add more options in the future without changing the destructuring pattern.
52-52: Consistent state structure in keyup event handlerThe change to the state destructuring in the keyup handler is consistent with the keydown handler, which is good for maintaining code uniformity.
For consistency with the previous suggestion, consider restructuring this line as follows:
update(({ SHORTCUTS, error, options }) => { const { shouldHandleKeyEvents } = options; // ... rest of the function });This approach would make the code more readable and easier to maintain, especially if more options are added in the future.
app/packages/state/src/recoil/modal.ts (1)
1-6: LGTM! Consider grouping related imports.The changes to the import statements look good and align with the updates in the file. The removal of unused imports and the addition of the
Lookerstype import improve code clarity.Consider grouping related imports together for better organization. For example:
import { PointInfo, type Sample } from "@fiftyone/looker"; import { mainSample, mainSampleQuery } from "@fiftyone/relay"; import { atom, selector } from "recoil"; import { graphQLSelector } from "recoil-relay"; import { VariablesOf } from "relay-runtime"; import type { Lookers } from "../hooks"; import { ComputeCoordinatesReturnType } from "../hooks/useTooltip";app/packages/state/src/hooks/useCreateLooker.ts (1)
39-40: LGTM: Added timeline support. Consider default value.The addition of the
enableTimelineparameter aligns well with the PR objective of adding video looker support for the modal timeline. It provides flexibility while maintaining backward compatibility.Consider providing a default value for
enableTimelineto make the intended default behavior explicit:enableTimeline: boolean = falseapp/packages/core/src/components/Modal/ImaVidLooker.tsx (1)
Line range hint
1-359: Consider further modularization for improved maintainabilityThe
ImaVidLookerReactcomponent is well-structured and uses modern React practices such as hooks and Recoil for state management. However, given its complexity in handling various aspects of video playback and timeline management, consider further modularizing the component by extracting some logic into separate custom hooks or sub-components. This could improve readability and make the component easier to maintain and test.For example, you could create separate hooks for:
- Timeline management
- Frame rendering and buffering
- Event handling
This refactoring would align even more closely with React best practices and the principle of separation of concerns.
app/packages/looker/src/state.ts (3)
178-178: LGTM! Consider adding JSDoc comment for clarity.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.Consider adding a JSDoc comment to explain the purpose and default behavior of this property:
/** * Determines whether key events should be handled. * @default true */ shouldHandleKeyEvents?: boolean;
222-222: LGTM! Consider making the property optional.The addition of
enableTimelineis a good feature for controlling the visibility of the timeline in video configurations. It follows TypeScript naming conventions.Consider making this property optional and providing a default value:
enableTimeline?: boolean;This change would allow for backward compatibility and provide more flexibility in usage. You can then set a default value in the implementation if needed.
Line range hint
465-465: LGTM! Consider adding a JSDoc comment for clarity.The addition of
bufferManagerof typeBufferManageris a good improvement for managing buffered frames in the ImaVid context. It ensures type safety and follows naming conventions.Consider adding a JSDoc comment to explain the purpose and usage of this property:
/** * Manages the buffering of video frames. */ bufferManager: BufferManager;
📜 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 (15)
- 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 (2 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (4 hunks)
- app/packages/state/src/recoil/modal.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/looker/src/lookers/imavid/index.ts
🧰 Additional context used
📓 Path-based instructions (14)
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/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.
🪛 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 (20)
app/packages/looker/src/index.ts (1)
6-6: LGTM! Verify usage of the new export.The addition of
getFrameNumberexport aligns with the PR objective of enhancing video support in the modal timeline. The export statement follows TypeScript best practices.To ensure the new export is being used correctly, run the following script:
This script will help identify where the newly exported function is being imported and used in the codebase outside of the looker package.
✅ Verification successful
Usage of
getFrameNumberis verified.The
getFrameNumberfunction is imported and used in:
app/packages/core/src/components/Modal/VideoLooker.tsx🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly exported 'getFrameNumber' function # Search for imports of 'getFrameNumber' from the looker package echo "Searching for imports of 'getFrameNumber':" rg --type typescript "import.*getFrameNumber.*from.*@fiftyone/looker" -g '!app/packages/looker/**' # Search for usage of 'getFrameNumber' function echo "Searching for usage of 'getFrameNumber' function:" rg --type typescript "getFrameNumber\(" -g '!app/packages/looker/**'Length of output: 457
Script:
#!/bin/bash # Description: Verify the usage of the newly exported 'getFrameNumber' function without using unsupported file types # Search for imports of 'getFrameNumber' from the looker package echo "Searching for imports of 'getFrameNumber':" rg "import.*getFrameNumber.*from.*@fiftyone/looker" --glob '!app/packages/looker/**' --glob '*.ts*' # Search for usage of 'getFrameNumber' function echo "Searching for usage of 'getFrameNumber' function:" rg "getFrameNumber\(" --glob '!app/packages/looker/**' --glob '*.ts*'Length of output: 619
app/packages/looker/src/elements/base.ts (1)
67-67: Excellent use of optional chaining operatorThe change from
this.element &&tothis.element?.is a good improvement. It simplifies the code while maintaining the same functionality. This change aligns with TypeScript best practices by using the optional chaining operator, which makes the code more concise and easier to read.app/packages/looker/src/elements/common/looker.ts (1)
6-9: Improved import statementsThe changes to the import statements are beneficial:
- Using
import typeforBaseState,Control, andEventsis a TypeScript best practice for type-only imports. This can lead to better performance and smaller bundle sizes in production builds.- Separating the
ControlEventKeyTypeimport improves code organization and makes it clear which imports are for types and which are for values.These changes align well with TypeScript and React best practices.
app/packages/state/src/recoil/modal.ts (2)
72-75: LGTM! Improved null safety.The change from
id.endsWith("-modal")toid?.endsWith("-modal")is a good improvement. It adds null safety by using the optional chaining operator, preventing potential runtime errors ifidisundefinedornull.This change aligns with TypeScript best practices and makes the code more robust.
Line range hint
26-30: LGTM! Consider reviewing the need for mutable state.The update to the
modalLookeratom type fromAbstractLooker<BaseState> | nulltoLookers | nullis consistent with the import changes and likely provides a more appropriate type for the modal looker state.However, the use of
dangerouslyAllowMutability: trueraises a potential concern. This option allows the atom's value to be mutated directly, which can lead to unexpected behavior in Recoil. Could you verify if this is necessary? If possible, consider removing this option to ensure immutability.To help verify the usage of
modalLooker, you can run the following script:This will help identify if there are any places where direct mutation might be occurring, justifying the use of
dangerouslyAllowMutability.✅ Verification successful
Remove
dangerouslyAllowMutability: truefrommodalLookerThe search for usages of
modalLookerdid not reveal any direct mutations. Therefore, thedangerouslyAllowMutability: trueoption may be unnecessary and can be removed to ensure immutability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of modalLooker in the codebase rg --type typescript --type tsx 'modalLooker'Length of output: 80
Script:
#!/bin/bash # Search for all usages of modalLooker in TypeScript and TSX files rg --type ts --type tsx 'modalLooker'Length of output: 65
Script:
#!/bin/bash # Search for all usages of modalLooker in TypeScript and TSX files using glob patterns rg 'modalLooker' --glob '*.ts' --glob '*.tsx'Length of output: 835
app/packages/state/src/hooks/useCreateLooker.ts (2)
12-12: LGTM: Improved type import.The change to a type-only import for
BaseStateandImaVidConfigis a good practice in TypeScript. It clearly separates type imports from value imports, potentially improving compilation speed without affecting runtime behavior.
116-116: LGTM: Correctly implemented timeline configuration.The addition of
enableTimelineto theconfigobject correctly implements the new functionality, passing the timeline configuration to the looker instance. This change is consistent with the function signature update and aligns with the PR objectives.app/packages/core/src/components/Modal/ImaVidLooker.tsx (2)
20-26: LGTM: Import statements reorganized for better modularityThe changes in import statements improve the code organization by separating concerns and potentially reducing circular dependencies. The addition of
useKeyEventssuggests a good practice of extracting complex logic into custom hooks.
136-136: LGTM: Improved key event handling with custom hookThe addition of
useKeyEventshook enhances the component's keyboard interaction handling. This change aligns with React best practices by extracting complex logic into a custom hook, improving readability and maintainability.app/packages/looker/src/state.ts (1)
Line range hint
1-639: Overall, good improvements to state management.The changes in this file enhance the flexibility and functionality of the state management system, particularly for video handling and key event management. The additions follow TypeScript best practices and naming conventions. Consider implementing the suggested minor improvements for even better clarity and flexibility.
app/packages/core/src/components/Modal/use-key-events.ts (2)
7-12:hoveredSampleIdselector is correctly implementedThe selector properly retrieves the
_idof the hovered sample using Recoil'sselector, which aligns with best practices.
14-18: Function parameters are appropriately typedThe exported default function correctly defines its parameters with appropriate TypeScript types, ensuring type safety and clarity.
app/packages/core/src/components/Modal/ModalLooker.tsx (6)
2-2: Use ofimport typeforImageLookerGood use of
import typeto importImageLookeras a type-only import, ensuring no unnecessary runtime import.
7-9: Imports added for video support are appropriateThe added imports for
VideoLookerReact,useModalContext, anduseLookercorrectly support the new video playback functionality.
31-31: Confirm the typing inuseLooker<ImageLooker>(props)Using
useLooker<ImageLooker>(props)specifies the type asImageLooker. Ensure that this is appropriate for non-video datasets and doesn't affect video datasets.Verify that
ImageLookeris the correct type here and that video datasets are correctly handled elsewhere.
43-43: Review necessity of type assertion withas fos.LookersThe type assertion
looker as fos.Lookersis used. Confirm whether this assertion is necessary or if the types can be adjusted to avoid it, which can help prevent potential type safety issues.To verify the type of
looker, check the return type ofuseLookerand see if it can be directly typed asfos.Lookers.
80-89: Ensure correct conditional rendering order inModalLookerThe component conditionally renders based on
shouldRenderImavidandvideo. Verify that the conditions are correctly ordered and that there are no scenarios where bothshouldRenderImavidandvideoare true but the wrong component renders.Consider adding unit tests to cover different combinations of
shouldRenderImavidandvideoto ensure the rendering logic functions as intended.
27-27:sampleproperty inLookerPropschanged to requiredThe
sampleproperty inLookerPropsis now required. Please ensure that all components using this interface provide thesampleprop to prevent potential TypeScript errors.To confirm that all usages of
LookerPropshave been updated, run the following script:✅ Verification successful
Verification Successful: All usages of
LookerPropsprovide the requiredsampleprop.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `LookerProps` to verify that `sample` is provided. # Search for all instances of `LookerProps` type. rg --type-add 'tsx' --glob '*.tsx' 'LookerProps' -A 5Length of output: 111
Script:
#!/bin/bash # Description: Find all usages of `LookerProps` to verify that `sample` is provided. # Corrected search for all instances of `LookerProps` type. rg --type-add 'tsx:*.tsx' 'LookerProps' -A 5Length of output: 1444
app/packages/core/src/components/Modal/use-looker.ts (1)
13-101: Well-structured implementation ofuseLookerhookThe
useLookerhook is well-implemented, effectively managing the lifecycle and interactions of the looker component. The use of React hooks, Recoil state, and event handlers adheres to best practices in React and TypeScript.app/packages/looker/src/lookers/video.ts (1)
23-25: Verify the consistency of state management librariesThe code introduces imports from both
jotaiand Recoil (@fiftyone/playback). Specifically,getDefaultStoreis imported fromjotai, andsetFrameNumberAtomis imported from@fiftyone/playback, which suggests the use of Recoil.Please verify if mixing
jotaiand Recoil aligns with our project's state management practices. Using multiple state management libraries can lead to increased complexity and potential inconsistencies.Run the following script to check the usage of
jotaiand Recoil in the codebase:
| 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.
Avoid including mutable refs in the useEffect dependency array
Including ref (a mutable ref object) in the dependency array of useEffect is unnecessary and can lead to unexpected behavior. Changes to ref.current do not trigger re-renders, and the ref object's reference remains constant. It's recommended to remove ref from the dependency array to prevent potential issues.
Apply this diff to remove ref from the dependency array:
- }, [hoveredId, id, looker, ref]);
+ }, [hoveredId, id, looker]);📝 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); | |
| } else if (ready.current) { | |
| looker.updateOptions({ | |
| shouldHandleKeyEvents: id === hoveredId, | |
| }); | |
| } | |
| }, [hoveredId, id, looker]); |
Add a cleanup function to useEffect to prevent memory leaks
Currently, the useEffect hook adds an event listener to looker, but it only removes the listener inside the update function after the 'load' event fires. If the component unmounts before the 'load' event occurs, the listener will not be removed, potentially causing memory leaks. To ensure proper cleanup, consider adding a cleanup function that removes the event listener when the component unmounts.
Apply this diff to add the cleanup function:
useEffect(() => {
+ const update = () => {
+ looker.updateOptions({
+ shouldHandleKeyEvents: id === hoveredId,
+ });
+ ready.current = true;
+ looker.removeEventListener("load", update);
+ };
+
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,
});
}
+ return () => {
+ looker.removeEventListener("load", update);
+ };
- }, [hoveredId, id, looker, ref]);
+ }, [hoveredId, id, looker]);📝 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(() => { | |
| const update = () => { | |
| looker.updateOptions({ | |
| shouldHandleKeyEvents: id === hoveredId, | |
| }); | |
| ready.current = true; | |
| looker.removeEventListener("load", update); | |
| }; | |
| if (ref.current) { | |
| looker.addEventListener("load", update); | |
| } else if (ready.current) { | |
| looker.updateOptions({ | |
| shouldHandleKeyEvents: id === hoveredId, | |
| }); | |
| } | |
| return () => { | |
| looker.removeEventListener("load", update); | |
| }; | |
| }, [hoveredId, id, looker]); |
| 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.
Ensure safe usage of Jotai's default store
In the postProcess method, the code sets the frame number using getDefaultStore().set(...). Directly manipulating the default store may bypass the benefits of using state management patterns like hooks or context providers.
Please ensure that this approach aligns with our application's state management conventions and does not introduce unforeseen side effects.
Consider abstracting state updates through a centralized state management layer or utilizing context to maintain consistency and predictability.
| 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] and this.lookerElement.children[0].element exist. To avoid potential runtime errors when these properties are undefined or null, consider adding checks before accessing them.
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;
+ }
+ 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; | |
| } | |
| return null; | |
| } |
benjaminpkane
force-pushed
the
video-playback
branch
from
10bc67d
to
6724038
Compare
benjaminpkane
dismissed
sashankaryal’s stale review
The base branch was changed.
benjaminpkane
force-pushed
the
video-playback
branch
from
6724038
to
b2a47f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
app/packages/core/src/components/Modal/hooks.ts (1)
22-41: Improve type safety and readability inuseLookerOptionsUpdatehookThe new hook looks good overall, but there are a few improvements we can make to enhance type safety and readability:
- Define a more specific type for the
updateparameter instead of usingobject.- Explicitly type the
updaterfunction parameter.- Consider creating a custom type for the Looker options.
Here's a suggested refactoring:
type LookerOptions = { showJSON: boolean; showHelp: boolean; // Add other option properties here }; export const useLookerOptionsUpdate = () => { return useRecoilCallback( ({ snapshot, set }) => async (update: Partial<LookerOptions>, updater?: (updated: LookerOptions) => void) => { const currentOptions = await snapshot.getPromise( fos.savedLookerOptions ) as LookerOptions; const panels = await snapshot.getPromise(fos.lookerPanels); const updated: LookerOptions = { ...currentOptions, ...update, showJSON: panels.json.isOpen, showHelp: panels.help.isOpen, }; set(fos.savedLookerOptions, updated); if (updater) updater(updated); } ); };These changes will improve type safety and make the code more self-documenting. The
LookerOptionstype can be expanded to include all relevant options, providing better type checking throughout the application.🧰 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/core/src/components/Modal/ModalLooker.tsx (4)
2-2: LGTM! Consider optimizing imports.The new imports for
ImageLookerandVideoLookerReactalign well with the component's enhanced functionality. The addition ofuseEffectanduseMemoto the React import is also appropriate given their usage in the component.Consider using named imports for React hooks to potentially improve bundle size optimization:
import React from "react"; import { useEffect, useMemo } from "react";Also applies to: 4-4, 7-7, 8-8, 9-9
Line range hint
11-15: LGTM! Consider enhancing type safety.The
useShowOverlayshook is well-implemented using Recoil'suseRecoilCallback. It provides a clean way to update theshowOverlaysstate based on a custom event.Consider adding a type for the custom event to enhance type safety:
interface ShowOverlaysEvent extends CustomEvent { detail: boolean; } export const useShowOverlays = () => { return useRecoilCallback(({ set }) => async (event: ShowOverlaysEvent) => { set(fos.showOverlays, event.detail); }); };
30-60: LGTM! Well-refactored component.The refactoring of
ModalLookerNoTimelineis well done. The use ofReact.memofor memoization and theuseLookerhook for managing the looker instance simplifies the component and improves its performance.Consider extracting the inline styles to a separate constant or using a CSS-in-JS solution for better maintainability:
const containerStyle = { width: "100%", height: "100%", position: "relative", }; return ( <div ref={ref} id={id} data-cy="modal-looker-container" style={{ ...containerStyle, background: theme.background.level2, }} /> );
80-80: LGTM! Enhanced support for video datasets.The addition of video dataset support in the
ModalLookercomponent is a great improvement. The conditional rendering ofVideoLookerReactfor video datasets enhances the component's flexibility.Consider using early returns to improve readability:
if (shouldRenderImavid) { return <ImaVidLookerReact sample={sample} />; } if (video) { return <VideoLookerReact sample={sample} />; } return <ModalLookerNoTimeline sample={sample} />;Also applies to: 86-89
📜 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 (15)
- 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/state/src/hooks/useCreateLooker.ts (4 hunks)
- app/packages/state/src/recoil/modal.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- app/packages/core/src/components/Modal/ImaVidLooker.tsx
- app/packages/core/src/components/Modal/utils.ts
- app/packages/looker/src/elements/base.ts
- app/packages/looker/src/elements/common/looker.ts
- app/packages/looker/src/index.ts
- app/packages/looker/src/lookers/imavid/index.ts
- app/packages/looker/src/lookers/video.ts
- app/packages/looker/src/state.ts
- app/packages/state/src/hooks/useCreateLooker.ts
- app/packages/state/src/recoil/modal.ts
🧰 Additional context used
📓 Path-based instructions (5)
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.
🪛 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 (10)
app/packages/core/src/components/Modal/VideoLooker.tsx (6)
1-11: Import statements look goodThe import statements are well-organized and include all necessary dependencies for the component's functionality. They follow best practices for React and TypeScript.
13-15: Interface definition is correctThe
VideoLookerReactPropsinterface is well-defined and provides proper type safety for the component props.
1-80: Overall assessment: Good implementation with room for minor improvementsThe
VideoLookerReactandTimelineControllercomponents are well-structured and implement the required video playback functionality. However, there are a few areas that can be improved:
- Remove unnecessary
useMemoforframeRate.- Handle potential undefined
frameRate.- Ensure handling of already-loaded videos in the
useEffecthook.- Simplify the
configassignment inuseCreateTimeline.Implementing these suggestions will enhance the reliability and performance of the components.
63-72: 🛠️ Refactor suggestionSimplify 'config' assignment in 'useCreateTimeline'
As mentioned in a previous review, the conditional check for
totalFramesis unnecessary since it's always defined when this component is rendered (as checked in the parent component). You can directly assign theconfigwithout the ternary operator.Apply this diff to simplify the code:
useCreateTimeline({ name: timelineName, - config: totalFrames - ? { - totalFrames, - loop: true, - } - : undefined, + config: { + totalFrames, + loop: true, + }, optOutOfAnimation: true, });Likely invalid or redundant comment.
22-23:⚠️ Potential issueHandle potential undefined 'frameRate'
The
frameRatecould potentially be undefined, which may cause issues when callinggetFrameNumber. Consider providing a default value or handling the undefined case to prevent potential runtime errors.You can assign a default frame rate or add a check:
- const frameRate = sample.frameRate; + const frameRate = sample.frameRate ?? DEFAULT_FRAME_RATE;Or update the usage in
getFrameNumber:const duration = looker.getVideo().duration; - setTotalFrames(getFrameNumber(duration, duration, frameRate)); + if (frameRate) { + setTotalFrames(getFrameNumber(duration, duration, frameRate)); + }Likely invalid or redundant comment.
25-33:⚠️ Potential issueEnsure handling of already-loaded videos in 'useEffect'
The current implementation doesn't handle the case where the video has already loaded before the
useEffectruns. This could result intotalFramesremaining undefined if the video loads very quickly.Modify the effect to handle this scenario:
useEffect(() => { const load = () => { const duration = looker.getVideo().duration; setTotalFrames(getFrameNumber(duration, duration, frameRate)); looker.removeEventListener("load", load); }; looker.addEventListener("load", load); + // Check if the video is already loaded + if (looker.getVideo().readyState >= 3) { + load(); + } return () => { looker.removeEventListener("load", load); }; }, [frameRate, looker]);Likely invalid or redundant comment.
app/packages/core/src/components/Modal/ModalLooker.tsx (3)
Line range hint
17-25: LGTM! Well-implemented hook.The
useClearSelectedLabelshook is correctly implemented using Recoil'suseRecoilCallback. It provides a clean way to clear theselectedLabelsstate. The empty dependency array ensures that the callback is only created once, which is an optimization.
Line range hint
1-92: Great job on the refactoring and feature additions!The changes to this file significantly improve the modal looker system:
- Enhanced support for different types of lookers (image, video, ImaVid).
- Improved state management using Recoil and custom hooks.
- Better component structure with clear separation of concerns.
- Consistent use of React best practices (memoization, hooks).
These improvements enhance the flexibility and maintainability of the modal looker system while adding support for video datasets.
27-27: LGTM! Verify impact on existing code.Making the
sampleproperty required in theLookerPropsinterface is a good change that enforces proper usage of the Looker components.Please verify that this change doesn't break existing code. Run the following script to check for any usage of
LookerPropsthat might need updating:✅ Verification successful
Verified! No additional code updates required.
The change to make the
sampleproperty required in theLookerPropsinterface only affectsModalLooker.tsx. There are no other usages ofLookerPropsin the codebase that need updating.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of LookerProps that might need updating # Test: Search for LookerProps usage rg --type typescript "LookerProps" -C 5Length of output: 74
Script:
#!/bin/bash # Description: Find usages of LookerProps that might need updating # Test: Search for LookerProps usage in TypeScript files rg --type ts --type tsx "LookerProps" -C 5Length of output: 70
Script:
#!/bin/bash # Description: Find usages of LookerProps that might need updating # Test: Search for LookerProps usage in .ts and .tsx files rg "LookerProps" --glob "*.ts" --glob "*.tsx" -C 5Length of output: 2109
app/packages/core/src/components/Modal/use-key-events.ts (1)
22-39: Previous review comments are still applicableThe issues raised in previous reviews about including
refin the dependency array ofuseEffectand adding a cleanup function to prevent memory leaks remain unresolved.Please address these concerns to ensure proper functionality and prevent potential issues.
| const frameRate = useMemo(() => { | ||
| return sample.frameRate; | ||
| }, [sample]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary useMemo for 'frameRate'
As mentioned in a previous review, the use of useMemo for frameRate is unnecessary. Accessing sample.frameRate is a simple operation that doesn't require memoization.
Apply this diff to simplify the code:
- const frameRate = useMemo(() => {
- return sample.frameRate;
- }, [sample]);
+ const frameRate = sample.frameRate;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const frameRate = useMemo(() => { | |
| return sample.frameRate; | |
| }, [sample]); | |
| const frameRate = sample.frameRate; |
| export const hoveredSampleId = selector<string>({ | ||
| key: "hoveredSampleId", | ||
| get: ({ get }) => { | ||
| return get(hoveredSample)?._id; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Adjust the selector's return type to account for possible undefined values
The hoveredSampleId selector is currently declared with the type selector<string>, but it may return undefined if hoveredSample is null or undefined. This can lead to type errors when consuming hoveredSampleId.
Consider updating the selector's type to selector<string | undefined> to accurately reflect the possible return values.
Apply this diff to adjust the return type:
-export const hoveredSampleId = selector<string>({
+export const hoveredSampleId = selector<string | undefined>({📝 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 const hoveredSampleId = selector<string>({ | |
| key: "hoveredSampleId", | |
| get: ({ get }) => { | |
| return get(hoveredSample)?._id; | |
| }, | |
| }); | |
| export const hoveredSampleId = selector<string | undefined>({ | |
| key: "hoveredSampleId", | |
| get: ({ get }) => { | |
| return get(hoveredSample)?._id; | |
| }, | |
| }); |
| }: { | ||
| sample: fos.ModalSample; | ||
| }) { | ||
| const [id] = useState(() => uuid()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Use useRef instead of useState for a stable id
Since the id value is intended to remain constant and does not need to trigger re-renders, using useRef is more appropriate than useState.
Apply this diff to implement the change:
- const [id] = useState(() => uuid());
+ const id = useRef(uuid()).current;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [id] = useState(() => uuid()); | |
| const id = useRef(uuid()).current; |
* organizing * rm Tmp * cleanup * add example back * cleaning * useKeyEvents hook * after load handling * fix import * enable timeline * default to true * refreshers
* organizing * rm Tmp * cleanup * add example back * cleaning * useKeyEvents hook * after load handling * fix import * enable timeline * default to true * refreshers
What changes are proposed in this pull request?
Adds video looker support for the modal timeline
Screen.Recording.2024-10-02.at.5.19.37.PM.mov
Animated panel with video
How is this patch tested? If it is not, please explain why.
Existing e2e
Release Notes
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
VideoLookerReactcomponent for enhanced video playback functionality.TimelineControllerfor managing video timelines.useLookeranduseKeyEvents.BUFFERING_PAUSE_TIMEOUTexport for better playback control.Improvements
enableTimelinefor more flexible looker instances.Bug Fixes
Chores