Ios player v2 #1901
Ios player v2 #1901
Conversation
|
Warning Rate Limit Exceeded@nick-delirium has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 42 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates focus on enhancing the mobile player's functionality in a web application. Key improvements include handling video and snapshot modes, unpacking tarball files, and updating UI components for a better mobile experience. The introduction of Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
frontend/package.jsonis excluded by:!**/*.jsonfrontend/yarn.lockis excluded by:!**/*.lock
Files selected for processing (10)
- frontend/app/components/Session/Player/MobilePlayer/ReplayWindow.tsx (2 hunks)
- frontend/app/player/common/common.d.ts (1 hunks)
- frontend/app/player/common/tarball.ts (1 hunks)
- frontend/app/player/common/types.ts (1 hunks)
- frontend/app/player/common/unpack.ts (1 hunks)
- frontend/app/player/mobile/IOSMessageManager.ts (11 hunks)
- frontend/app/player/mobile/IOSPlayer.ts (1 hunks)
- frontend/app/player/mobile/managers/SnapshotManager.ts (1 hunks)
- frontend/app/player/web/MessageLoader.ts (5 hunks)
- frontend/app/player/web/network/loadFiles.ts (2 hunks)
Additional comments: 12
frontend/app/player/common/common.d.ts (1)
- 2-10: The TypeScript definitions for the 'js-untar' module are correctly implemented, providing clear interfaces and function declarations for working with tar files.
frontend/app/player/common/types.ts (1)
- 42-42: The addition of the
videoURLproperty to theSessionFilesInfointerface is correctly implemented, enhancing the interface to include multiple video URLs.frontend/app/player/mobile/IOSMessageManager.ts (10)
- 2-2: Importing
TarFilefrom "js-untar" introduces a dependency on thejs-untarlibrary for handling tar files. Ensure that this library is included in the project's dependencies and is compatible with the project's existing infrastructure.- 20-20: The addition of
SnapshotManagerfrom 'Player/mobile/managers/SnapshotManager' is crucial for managing snapshots. Verify thatSnapshotManageris implemented correctly and efficiently handles snapshot data, especially in terms of memory management and performance.- 31-37: The expansion of
performanceWarningsto include new warnings like 'lowDiskSpace' and 'isLowPowerModeEnabled' is a good practice for covering more performance-related issues. Ensure that these new warnings are properly documented and that their handling is consistent across the application.- 77-77: Adding
currentSnapshotto theStateinterface with a type ofTarFile | nullis a significant change. It's important to ensure that all parts of the application that interact with theStateobject are updated to consider this new property. Additionally, consider if there are any nullability concerns or edge cases that need to be handled whencurrentSnapshotis null.- 133-140: The updates to the
updateListsmethod to handle exceptions and frustrations differently show an attempt to refine the logic for updating lists. However, ensure that the method's error handling is robust, especially in cases whereexceptionsorfrustrationsmight be undefined or have unexpected values.- 166-170: The
onFileReadSuccessmethod's logic for updating state based on file read success is crucial for the application's data flow. Ensure that the method correctly handles all possible states and that the transition from waiting for files to having them processed is seamless and error-free.- 216-221: The handling of snapshots in the
movemethod with the introduction ofcurrentSnapshotstate update is a significant feature. Ensure that the logic for determining when to updatecurrentSnapshotis accurate and that it does not introduce performance issues, especially in scenarios with frequent timeline movements.- 237-250: The handling of iOS performance events in the
distributeMessagemethod is well-structured. However, ensure that the mapping between message names and performance stats is comprehensive and that all relevant performance events are captured and processed correctly.- 285-285: The
setMessagesLoadingmethod's approach to updating the loading state and readiness of the screen is straightforward. However, ensure that the interplay betweenmessagesLoadingandcssLoadingis clear and that there are no edge cases where the screen might incorrectly display its state.- 288-290: The
setSizemethod correctly updates the screen scaling and state. Ensure that this method is called appropriately throughout the application to maintain a consistent and responsive UI, especially on iOS devices with varying screen sizes.
| @@ -92,6 +109,7 @@ export default class IOSMessageManager implements IMessageManager { | |||
| private lastMessageTime: number = 0; | |||
| private touchManager: TouchManager; | |||
| private lists: Lists; | |||
| public snapshotManager: SnapshotManager; | |||
There was a problem hiding this comment.
The introduction of snapshotManager as a public property of IOSMessageManager class indicates a design decision to allow external access to snapshot management functionalities. Ensure that this aligns with the class's responsibility and does not expose internal mechanisms unnecessarily. Consider encapsulating snapshot management within methods if direct access to snapshotManager is not required externally.
Consider encapsulating snapshot management functionalities within methods of IOSMessageManager to avoid direct external access to snapshotManager, enhancing encapsulation and potentially simplifying future refactoring.
| } | ||
| this.state.update(newState); | ||
| }; | ||
|
|
||
| public onFileReadFailed = (...e: any[]) => { | ||
| logger.error(e); | ||
| this.state.update({error: true}); | ||
| this.state.update({ error: true }); |
There was a problem hiding this comment.
The onFileReadFailed method correctly updates the state to reflect an error condition. However, ensure that all possible error information is logged or handled appropriately to aid in debugging and user feedback.
Consider enhancing error logging in the onFileReadFailed method by including more detailed information about the failure, which could be beneficial for debugging purposes.
| const unpackTar = (data: Uint8Array): Promise<TarFile[]> => { | ||
| const isTar = true | ||
| // tarball ustar starts from 257, 75 73 74 61 72, but this is getting lost here for some reason | ||
| // so we rely on try catch | ||
| // data[257] === 0x75 && | ||
| // data[258] === 0x73 && | ||
| // data[259] === 0x74 && | ||
| // data[260] === 0x61 && | ||
| // data[261] === 0x72 && | ||
| // data[262] === 0x00; | ||
|
|
||
| if (isTar) { | ||
| const now = performance.now(); | ||
| return untar(data.buffer).then((files) => { | ||
| console.debug('Tar unpack time', Math.floor(performance.now() - now) + 'ms'); | ||
| return files; | ||
| }); | ||
| } else { | ||
| return Promise.reject('Not a tarball file'); | ||
| } |
There was a problem hiding this comment.
The unpackTar function uses a hardcoded isTar boolean and includes comments about tarball identification that are not utilized. Consider removing these redundant parts or implementing actual validation logic for tarballs. Additionally, ensure that the performance measurement and promise-based structure are maintained for efficiency and clarity.
- const isTar = true
- // tarball ustar starts from 257, 75 73 74 61 72, but this is getting lost here for some reason
- // so we rely on try catch
- // data[257] === 0x75 &&
- // data[258] === 0x73 &&
- // data[259] === 0x74 &&
- // data[260] === 0x61 &&
- // data[261] === 0x72 &&
- // data[262] === 0x00;
+ // Implement actual validation logic for tarballs or clarify why it's omitted.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.
| const unpackTar = (data: Uint8Array): Promise<TarFile[]> => { | |
| const isTar = true | |
| // tarball ustar starts from 257, 75 73 74 61 72, but this is getting lost here for some reason | |
| // so we rely on try catch | |
| // data[257] === 0x75 && | |
| // data[258] === 0x73 && | |
| // data[259] === 0x74 && | |
| // data[260] === 0x61 && | |
| // data[261] === 0x72 && | |
| // data[262] === 0x00; | |
| if (isTar) { | |
| const now = performance.now(); | |
| return untar(data.buffer).then((files) => { | |
| console.debug('Tar unpack time', Math.floor(performance.now() - now) + 'ms'); | |
| return files; | |
| }); | |
| } else { | |
| return Promise.reject('Not a tarball file'); | |
| } | |
| const unpackTar = (data: Uint8Array): Promise<TarFile[]> => { | |
| // Implement actual validation logic for tarballs or clarify why it's omitted. | |
| if (isTar) { | |
| const now = performance.now(); | |
| return untar(data.buffer).then((files) => { | |
| console.debug('Tar unpack time', Math.floor(performance.now() - now) + 'ms'); | |
| return files; | |
| }); | |
| } else { | |
| return Promise.reject('Not a tarball file'); | |
| } |
| public mapToSnapshots(files: TarFile[]) { | ||
| const filenameRegexp = /(\d+)_1_(\d+)\.jpeg$/; | ||
| const firstPair = files[0].name.match(filenameRegexp) | ||
| const sessionStart = firstPair ? parseInt(firstPair[1], 10) : 0 | ||
| files.forEach(file => { | ||
| const [_, _2, imageTimestamp] = file | ||
| .name | ||
| .match(filenameRegexp) | ||
| ?.map(n => parseInt(n, 10)) ?? [0, 0, 0] | ||
| const messageTime = imageTimestamp - sessionStart | ||
| this.snapshots[messageTime] = file | ||
| this.append({ time: messageTime }) | ||
| }) |
There was a problem hiding this comment.
In the mapToSnapshots method, there's an issue with destructuring the file object directly instead of its name property. This could lead to incorrect parsing of filenames. Ensure that the filename is correctly extracted from the file object before applying the regular expression match.
- const [_, _2, imageTimestamp] = file
+ const [_, _2, imageTimestamp] = file.nameCommittable 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.
| public mapToSnapshots(files: TarFile[]) { | |
| const filenameRegexp = /(\d+)_1_(\d+)\.jpeg$/; | |
| const firstPair = files[0].name.match(filenameRegexp) | |
| const sessionStart = firstPair ? parseInt(firstPair[1], 10) : 0 | |
| files.forEach(file => { | |
| const [_, _2, imageTimestamp] = file | |
| .name | |
| .match(filenameRegexp) | |
| ?.map(n => parseInt(n, 10)) ?? [0, 0, 0] | |
| const messageTime = imageTimestamp - sessionStart | |
| this.snapshots[messageTime] = file | |
| this.append({ time: messageTime }) | |
| }) | |
| public mapToSnapshots(files: TarFile[]) { | |
| const filenameRegexp = /(\d+)_1_(\d+)\.jpeg$/; | |
| const firstPair = files[0].name.match(filenameRegexp) | |
| const sessionStart = firstPair ? parseInt(firstPair[1], 10) : 0 | |
| files.forEach(file => { | |
| const [_, _2, imageTimestamp] = file.name | |
| .match(filenameRegexp) | |
| ?.map(n => parseInt(n, 10)) ?? [0, 0, 0] | |
| const messageTime = imageTimestamp - sessionStart | |
| this.snapshots[messageTime] = file | |
| this.append({ time: messageTime }) | |
| }) |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #1901 +/- ##
===========================================
- Coverage 27.26% 13.31% -13.95%
===========================================
Files 67 179 +112
Lines 5429 12799 +7370
Branches 1366 2732 +1366
===========================================
+ Hits 1480 1704 +224
- Misses 3543 10270 +6727
- Partials 406 825 +419
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- frontend/app/components/Session/Player/MobilePlayer/ReplayWindow.tsx (1 hunks)
- frontend/app/player/mobile/IOSPlayer.ts (1 hunks)
- frontend/app/player/web/MessageLoader.ts (5 hunks)
- frontend/app/player/web/network/loadFiles.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- frontend/app/player/web/MessageLoader.ts
- frontend/app/player/web/network/loadFiles.ts
Additional comments: 6
frontend/app/player/mobile/IOSPlayer.ts (4)
- 9-11: The introduction of the
PlayerModeenum withVIDEOandSNAPSvalues is a good practice for managing different player modes. This approach enhances readability and maintainability by using meaningful constants instead of hard-coded strings.- 78-98: The
updateListsmethod's logic for transforming event names and handling exceptions is a good example of data transformation in the application layer. However, ensure that the transformation rules (e.g., 'Click' to 'Touch') are documented and validated against all possible event types to avoid unexpected behavior.- 104-104: The
updateOverlayStylemethod correctly delegates the style update to thescreenobject. This encapsulation of screen-related operations within thescreenobject adheres to the Single Responsibility Principle.- 118-132: The scaling methods (
scaleandcustomScale) provide flexibility in adjusting the player size. It's important to ensure that these methods are called appropriately in response to relevant events (e.g., window resize, orientation change) to maintain the correct player size.frontend/app/components/Session/Player/MobilePlayer/ReplayWindow.tsx (2)
- 1-1: Importing the
PlayerModeenum from 'Player' is a good practice for consistency and maintainability. It ensures that the component uses the same definitions as the rest of the application.- 142-158: The styling functions
mobileLoadingBarStyleandmobileIconStyleare a clean way to separate styling logic from the component structure. This approach enhances the readability of the component and makes it easier to update styles in the future.
| public screen: Screen | ||
| protected messageManager: IOSMessageManager | ||
| protected readonly messageLoader: MessageLoader | ||
| mode: null, |
There was a problem hiding this comment.
The mode property is initialized as null in the INITIAL_STATE. It's important to ensure that all usages of mode properly handle the null case to avoid runtime errors. Consider initializing mode with a default value if applicable.
- mode: null,
+ mode: PlayerMode.VIDEO, // or another appropriate default valueCommittable 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.
| mode: null, | |
| mode: PlayerMode.VIDEO, // or another appropriate default value |
| const hasTar = session.videoURL.some((url) => url.includes('.tar.')); | ||
| const screen = new Screen(true, ScaleMode.Embed); | ||
| const messageManager = new IOSMessageManager(session, wpState, screen, uiErrorHandler); | ||
| const messageLoader = new MessageLoader( | ||
| session, | ||
| wpState, | ||
| messageManager, | ||
| false, | ||
| uiErrorHandler | ||
| ) | ||
| ); | ||
| super(wpState, messageManager); | ||
| this.screen = screen | ||
| this.messageManager = messageManager | ||
| this.messageLoader = messageLoader | ||
|
|
||
| void messageLoader.loadFiles() | ||
| const endTime = session.duration?.valueOf() || 0 | ||
| this.screen = screen; | ||
| this.messageManager = messageManager; | ||
| this.messageLoader = messageLoader; | ||
|
|
||
| if (hasTar) { | ||
| messageLoader | ||
| .loadTarball(session.videoURL.find((url) => url.includes('.tar.'))!) | ||
| .then((files) => { | ||
| if (files) { | ||
| this.wpState.update({ mode: PlayerMode.SNAPS }); | ||
| this.messageManager.snapshotManager.mapToSnapshots(files); | ||
| this.play(); | ||
| } | ||
| }) | ||
| .catch((e) => { | ||
| this.wpState.update({ mode: PlayerMode.VIDEO }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The constructor logic to determine the mode based on the presence of a .tar. URL is clear and concise. However, ensure that the .catch block in the promise chain properly logs the error or notifies the user, as it currently only switches the mode without any error handling.
.catch((e) => {
+ Log.error(e); // Ensure to log the error or handle it appropriately
this.wpState.update({ mode: PlayerMode.VIDEO });
});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.
| const hasTar = session.videoURL.some((url) => url.includes('.tar.')); | |
| const screen = new Screen(true, ScaleMode.Embed); | |
| const messageManager = new IOSMessageManager(session, wpState, screen, uiErrorHandler); | |
| const messageLoader = new MessageLoader( | |
| session, | |
| wpState, | |
| messageManager, | |
| false, | |
| uiErrorHandler | |
| ) | |
| ); | |
| super(wpState, messageManager); | |
| this.screen = screen | |
| this.messageManager = messageManager | |
| this.messageLoader = messageLoader | |
| void messageLoader.loadFiles() | |
| const endTime = session.duration?.valueOf() || 0 | |
| this.screen = screen; | |
| this.messageManager = messageManager; | |
| this.messageLoader = messageLoader; | |
| if (hasTar) { | |
| messageLoader | |
| .loadTarball(session.videoURL.find((url) => url.includes('.tar.'))!) | |
| .then((files) => { | |
| if (files) { | |
| this.wpState.update({ mode: PlayerMode.SNAPS }); | |
| this.messageManager.snapshotManager.mapToSnapshots(files); | |
| this.play(); | |
| } | |
| }) | |
| .catch((e) => { | |
| this.wpState.update({ mode: PlayerMode.VIDEO }); | |
| }); | |
| } | |
| const hasTar = session.videoURL.some((url) => url.includes('.tar.')); | |
| const screen = new Screen(true, ScaleMode.Embed); | |
| const messageManager = new IOSMessageManager(session, wpState, screen, uiErrorHandler); | |
| const messageLoader = new MessageLoader( | |
| session, | |
| wpState, | |
| messageManager, | |
| false, | |
| uiErrorHandler | |
| ); | |
| super(wpState, messageManager); | |
| this.screen = screen; | |
| this.messageManager = messageManager; | |
| this.messageLoader = messageLoader; | |
| if (hasTar) { | |
| messageLoader | |
| .loadTarball(session.videoURL.find((url) => url.includes('.tar.'))!) | |
| .then((files) => { | |
| if (files) { | |
| this.wpState.update({ mode: PlayerMode.SNAPS }); | |
| this.messageManager.snapshotManager.mapToSnapshots(files); | |
| this.play(); | |
| } | |
| }) | |
| .catch((e) => { | |
| Log.error(e); // Ensure to log the error or handle it appropriately | |
| this.wpState.update({ mode: PlayerMode.VIDEO }); | |
| }); | |
| } |
| this.screen.addToBody(player); | ||
| this.screen.addMobileStyles(); | ||
| window.addEventListener('resize', () => | ||
| this.customScale(this.customConstrains.width, this.customConstrains.height) | ||
| ) | ||
| } | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The injectPlayer method effectively adds the player to the screen and sets up a resize listener for custom scaling. However, consider removing the event listener on component unmount or cleanup to prevent potential memory leaks.
+ componentWillUnmount() {
+ window.removeEventListener('resize', this.customScale);
+ }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.
| this.screen.addToBody(player); | |
| this.screen.addMobileStyles(); | |
| window.addEventListener('resize', () => | |
| this.customScale(this.customConstrains.width, this.customConstrains.height) | |
| ) | |
| } | |
| ); | |
| }; | |
| this.screen.addToBody(player); | |
| this.screen.addMobileStyles(); | |
| window.addEventListener('resize', () => | |
| this.customScale(this.customConstrains.width, this.customConstrains.height) | |
| ); | |
| }; | |
| componentWillUnmount() { | |
| window.removeEventListener('resize', this.customScale); | |
| } |
| super.clean(); | ||
| this.screen.clean(); | ||
| // @ts-ignore | ||
| this.screen = undefined; | ||
| this.messageLoader.clean() | ||
| this.messageLoader.clean(); | ||
| // @ts-ignore | ||
| this.messageManager = undefined; | ||
| window.removeEventListener('resize', this.scale) | ||
| } | ||
|
|
||
|
|
||
| window.removeEventListener('resize', this.scale); | ||
| }; |
There was a problem hiding this comment.
The clean method demonstrates good cleanup practices by calling the clean method of superclass and associated objects, and removing event listeners. However, setting objects to undefined using @ts-ignore is not a best practice. Consider setting them to null instead or ensuring they are properly disposed of if they are no longer needed.
- // @ts-ignore
- this.screen = undefined;
+ this.screen = null;
- // @ts-ignore
- this.messageManager = undefined;
+ this.messageManager = 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.
| super.clean(); | |
| this.screen.clean(); | |
| // @ts-ignore | |
| this.screen = undefined; | |
| this.messageLoader.clean() | |
| this.messageLoader.clean(); | |
| // @ts-ignore | |
| this.messageManager = undefined; | |
| window.removeEventListener('resize', this.scale) | |
| } | |
| window.removeEventListener('resize', this.scale); | |
| }; | |
| super.clean(); | |
| this.screen.clean(); | |
| this.screen = null; | |
| this.messageLoader.clean(); | |
| this.messageManager = null; | |
| window.removeEventListener('resize', this.scale); | |
| }; |
| const imageRef = React.useRef<HTMLImageElement>(); | ||
| const containerRef = React.useRef<HTMLDivElement>(); | ||
|
|
||
| const time = playerContext.store.get().time | ||
| const { time, currentSnapshot, mode } = playerContext.store.get(); | ||
|
|
||
| React.useEffect(() => { | ||
| if (videoRef.current) { | ||
| const timeSecs = time / 1000 | ||
| const delta = videoRef.current.currentTime - timeSecs | ||
| if (videoRef.current && mode === PlayerMode.VIDEO) { | ||
| const timeSecs = time / 1000; | ||
| const delta = videoRef.current.currentTime - timeSecs; | ||
| if (videoRef.current.duration >= timeSecs && Math.abs(delta) > 0.1) { | ||
| videoRef.current.currentTime = timeSecs | ||
| videoRef.current.currentTime = timeSecs; | ||
| } | ||
| } | ||
| }, [time]) | ||
| }, [time, mode]); | ||
| React.useEffect(() => { | ||
| if (currentSnapshot && mode === PlayerMode.SNAPS) { | ||
| const blob = currentSnapshot.getBlobUrl(); | ||
| if (imageRef.current) { | ||
| imageRef.current.src = blob; | ||
| } | ||
| } | ||
| }, [currentSnapshot, mode]); |
There was a problem hiding this comment.
The use of useRef for imageRef and the new useEffect hook for displaying the current snapshot image are appropriate for managing DOM elements and side effects in React. Ensure that the blob URL is properly revoked when the component unmounts or the snapshot changes to avoid memory leaks.
+ useEffect(() => {
+ return () => {
+ if (imageRef.current && imageRef.current.src) {
+ URL.revokeObjectURL(imageRef.current.src);
+ }
+ };
+ }, [currentSnapshot]);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.
| const imageRef = React.useRef<HTMLImageElement>(); | |
| const containerRef = React.useRef<HTMLDivElement>(); | |
| const time = playerContext.store.get().time | |
| const { time, currentSnapshot, mode } = playerContext.store.get(); | |
| React.useEffect(() => { | |
| if (videoRef.current) { | |
| const timeSecs = time / 1000 | |
| const delta = videoRef.current.currentTime - timeSecs | |
| if (videoRef.current && mode === PlayerMode.VIDEO) { | |
| const timeSecs = time / 1000; | |
| const delta = videoRef.current.currentTime - timeSecs; | |
| if (videoRef.current.duration >= timeSecs && Math.abs(delta) > 0.1) { | |
| videoRef.current.currentTime = timeSecs | |
| videoRef.current.currentTime = timeSecs; | |
| } | |
| } | |
| }, [time]) | |
| }, [time, mode]); | |
| React.useEffect(() => { | |
| if (currentSnapshot && mode === PlayerMode.SNAPS) { | |
| const blob = currentSnapshot.getBlobUrl(); | |
| if (imageRef.current) { | |
| imageRef.current.src = blob; | |
| } | |
| } | |
| }, [currentSnapshot, mode]); | |
| const imageRef = React.useRef<HTMLImageElement>(); | |
| const containerRef = React.useRef<HTMLDivElement>(); | |
| const { time, currentSnapshot, mode } = playerContext.store.get(); | |
| React.useEffect(() => { | |
| if (videoRef.current && mode === PlayerMode.VIDEO) { | |
| const timeSecs = time / 1000; | |
| const delta = videoRef.current.currentTime - timeSecs; | |
| if (videoRef.current.duration >= timeSecs && Math.abs(delta) > 0.1) { | |
| videoRef.current.currentTime = timeSecs; | |
| } | |
| } | |
| }, [time, mode]); | |
| React.useEffect(() => { | |
| if (currentSnapshot && mode === PlayerMode.SNAPS) { | |
| const blob = currentSnapshot.getBlobUrl(); | |
| if (imageRef.current) { | |
| imageRef.current.src = blob; | |
| } | |
| } | |
| }, [currentSnapshot, mode]); | |
| useEffect(() => { | |
| return () => { | |
| if (imageRef.current && imageRef.current.src) { | |
| URL.revokeObjectURL(imageRef.current.src); | |
| } | |
| }; | |
| }, [currentSnapshot]); |
| const { svg, styles } = mapIphoneModel(userDevice); | ||
| if (!containerRef.current && mode) { | ||
| const host = document.createElement('div'); | ||
| const shell = document.createElement('div'); | ||
| const icon = document.createElement('div'); | ||
| const videoContainer = document.createElement('div'); | ||
|
|
||
| videoContainer.style.borderRadius = '10px'; | ||
| videoContainer.style.overflow = 'hidden'; | ||
| videoContainer.style.margin = styles.margin; | ||
| videoContainer.style.display = 'none'; | ||
| videoContainer.style.width = styles.screen.width + 'px'; | ||
| videoContainer.style.height = styles.screen.height + 'px'; | ||
|
|
||
| shell.innerHTML = svg; | ||
| Object.assign(icon.style, mobileIconStyle(styles)); | ||
| const spacer = document.createElement('div'); | ||
| spacer.style.width = '60px'; | ||
| spacer.style.height = '60px'; | ||
|
|
||
| const loadingBar = document.createElement('div'); | ||
|
|
||
| Object.assign(loadingBar.style, mobileLoadingBarStyle(styles)); | ||
| icon.innerHTML = appleIcon; | ||
|
|
||
| shell.style.position = 'absolute'; | ||
| shell.style.top = '0'; | ||
|
|
||
| host.appendChild(videoContainer); | ||
| host.appendChild(shell); | ||
|
|
||
| icon.appendChild(spacer); | ||
| icon.appendChild(loadingBar); | ||
| host.appendChild(icon); | ||
|
|
||
| containerRef.current = host; | ||
|
|
||
| playerContext.player.injectPlayer(host); | ||
| playerContext.player.customScale(styles.shell.width, styles.shell.height); | ||
| playerContext.player.updateDimensions({ | ||
| width: styles.screen.width, | ||
| height: styles.screen.height, | ||
| }) | ||
| }); | ||
| playerContext.player.updateOverlayStyle({ | ||
| margin: styles.margin, | ||
| width: styles.screen.width + 'px', | ||
| height: styles.screen.height + 'px', | ||
| }) | ||
| }); | ||
|
|
||
| if (mode === PlayerMode.SNAPS) { | ||
| const imagePlayer = document.createElement('img'); | ||
| imagePlayer.style.width = styles.screen.width + 'px'; | ||
| imagePlayer.style.height = styles.screen.height + 'px'; | ||
| imagePlayer.style.backgroundColor = '#333'; | ||
|
|
||
| videoContainer.appendChild(imagePlayer); | ||
| const removeLoader = () => { | ||
| videoContainer.style.display = 'block'; | ||
| icon.style.display = 'none'; | ||
| host.removeChild(icon); | ||
| playerContext.player.play(); | ||
| imagePlayer.removeEventListener('load', removeLoader); | ||
| }; | ||
| imagePlayer.addEventListener('load', removeLoader); | ||
| imageRef.current = imagePlayer; | ||
| } | ||
| if (mode === PlayerMode.VIDEO) { | ||
| const mp4URL = videoURL.find((url) => url.includes('.mp4')); | ||
| if (mp4URL) { | ||
| const videoEl = document.createElement('video'); | ||
| const sourceEl = document.createElement('source'); | ||
|
|
||
| videoContainer.appendChild(videoEl); | ||
|
|
||
| videoEl.width = styles.screen.width; | ||
| videoEl.height = styles.screen.height; | ||
| videoEl.style.backgroundColor = '#333'; | ||
|
|
||
| sourceEl.setAttribute('src', mp4URL); | ||
| sourceEl.setAttribute('type', 'video/mp4'); | ||
| videoEl.appendChild(sourceEl); | ||
|
|
||
| videoEl.addEventListener('loadeddata', () => { | ||
| videoContainer.style.display = 'block'; | ||
| icon.style.display = 'none'; | ||
| host.removeChild(icon); | ||
| playerContext.player.play(); | ||
| }); | ||
|
|
||
| videoRef.current = videoEl; | ||
| } | ||
| } | ||
| } | ||
| }, [videoURL, playerContext.player.screen.document]) | ||
| return ( | ||
| <div /> | ||
| ) | ||
| }, [videoURL, playerContext.player.screen.document, mode]); |
There was a problem hiding this comment.
The logic within the useEffect hook for creating and injecting the player based on the mode is comprehensive and handles both video and snapshot scenarios. However, this hook is quite large and does multiple things, which can make it harder to maintain. Consider breaking it down into smaller, more focused functions or hooks to improve readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- frontend/app/components/Session/Player/MobilePlayer/ReplayWindow.tsx (1 hunks)
- frontend/app/player/mobile/IOSPlayer.ts (1 hunks)
- frontend/app/player/web/Screen/Screen.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- frontend/app/player/mobile/IOSPlayer.ts
Additional comments: 9
frontend/app/components/Session/Player/MobilePlayer/ReplayWindow.tsx (8)
- 1-1: The import of
PlayerModefrom 'Player' is correctly added to support the new player mode functionalities introduced in this PR.- 20-20: The addition of
imageRefusinguseRef<HTMLImageElement>()is appropriate for managing the snapshot image element in a performant and React-friendly way.- 23-23: Destructuring
time,currentSnapshot, andmodefromplayerContext.store.get()is a clean way to access these values. Ensure thatplayerContext.store.get()always returns an object containing these properties to avoid runtime errors.- 26-30: The
useEffecthook for setting the current time of the video is well-implemented. It correctly checks if the mode isPlayerMode.VIDEOand adjusts the video's current time based on thetimevalue. This logic ensures synchronization between the video playback and the player's state.- 35-39: The
useEffecthook for displaying the current snapshot image is correctly implemented. It checks if there is acurrentSnapshotand the mode isPlayerMode.SNAPS, then sets the image source to the blob URL obtained fromcurrentSnapshot.getBlobUrl(). This approach is efficient for dynamically updating the snapshot display.- 44-96: The logic for injecting the player and custom scaling within the
useEffecthook is comprehensive and appears to correctly handle the creation and styling of player elements based on the device model. However, ensure that the elements created (e.g.,div,img,video) are properly cleaned up to prevent memory leaks or unintended side effects when the component unmounts or when the player mode changes.- 99-150: The conditional rendering logic based on
PlayerModewithin theuseEffecthook is well-structured, handling both snapshot and video modes effectively. The creation ofimgandvideoelements dynamically based on the mode is a good approach. However, ensure that theremoveLoaderfunction and event listeners are correctly removed to prevent memory leaks and ensure that elements are properly cleaned up when the mode changes or the component unmounts.- 154-170: The styling functions
mobileLoadingBarStyleandmobileIconStyleare correctly defined and provide a clean way to manage styles based on the device model. This approach enhances maintainability and readability of the styling logic.frontend/app/player/web/Screen/Screen.ts (1)
- 117-118: The addition of a null check for
this.documentbefore attempting to manipulate the DOM is a good practice. It enhances the robustness of the code by preventing potential runtime errors when thedocumentis not available. Logging an error to the console is an appropriate way to notify developers of the issue.
Summary by CodeRabbit