fix(video): Allow seeking in video player progress bar#37197
fix(video): Allow seeking in video player progress bar#37197NAME-ASHWANIYADAV wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 60dbead The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds HTTP Range support and Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Browser / Client
participant Video as VideoElement
participant Server as File Server
rect rgba(245,250,255,0.5)
Note over User,Client: User initiates seek via progress bar
User->>Client: click/drag progress bar
Client->>Video: set currentTime (seeking)
end
rect rgba(240,255,245,0.5)
Note over Video,Server: Range request and guarded recovery
Video->>Server: GET /file (Range: bytes=...)
Server->>Video: 206 Partial Content + `Accept-Ranges: bytes` + range headers
Video->>Client: resume playback at target time
Note over Client: useReloadOnError detects `seeking` and returns early (no reload)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx (1)
79-84: Remove unreachable code.Since
'play'was removed from theeventsarray (line 5),event.typewill never equal'play'and this entire conditional block is now dead code.Apply this diff to remove the unreachable code:
- } else if (event.type === 'play') { - // The user has initiated a playback for the first time, probably we should wait for the stalled or error event - // the url may still be valid since we dont know the expiration time yet - isRecovering.current = false; - return; - } + }
🧹 Nitpick comments (1)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
4-4: Remove unused import.
useRefis imported but never used in the component. ThemediaRefcomes from theuseReloadOnErrorhook, not fromuseRef.Apply this diff to remove the unused import:
-import { useMemo, useRef } from 'react'; +import { useMemo } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/app/file-upload/server/config/FileSystem.ts(2 hunks)apps/meteor/app/file-upload/server/config/GridFS.ts(2 hunks)apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx(2 hunks)apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
apps/meteor/server/ufs/ufs-store.ts (1)
getURL(304-309)
🔇 Additional comments (6)
apps/meteor/app/file-upload/server/config/GridFS.ts (2)
165-165: LGTM! Range request support correctly advertised.The
Accept-Ranges: bytesheader enables browsers to request partial content for video seeking. The underlyingreadFromGridFSfunction (lines 89-100) already handles range requests, so this header completes the implementation.
185-185: LGTM! Consistent range support for user data files.The header addition matches the implementation in
GridFS:Uploadsand is backed by the samereadFromGridFSrange handling logic.apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx (2)
5-5: LGTM! Simplified event handling.Removing the
'play'event from the listener array is appropriate since the first-recovery logic (lines 79-84) is now unreachable and unnecessary for the fix.
57-59: LGTM! Correctly prevents recovery during seeking.The early return when
seekingis true prevents the URL recovery mechanism from interfering with user-initiated seek operations, which was the root cause of the progress bar bug.apps/meteor/app/file-upload/server/config/FileSystem.ts (1)
31-31: LGTM! Range support correctly implemented.The
Accept-Ranges: bytesheader is backed by proper range request handling (lines 36-47), enabling video seeking for filesystem uploads.apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
32-32: LGTM! Cleaner video element structure.The refactor from a
Boxwrapper with nested<source>to a direct<video>element withsrcattribute simplifies the component while maintaining the same functionality. ThemediaReffromuseReloadOnErroris correctly attached, preserving the error recovery behavior.
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
3 times, most recently
from
de55f01 to
05bc50e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (4)
32-32: Use proper video element syntax with fallback content.The
<video>element is not a void element in HTML5 and should use opening and closing tags rather than self-closing syntax. While React tolerates self-closing non-void elements, it's non-standard and may cause issues in SSR contexts or future React versions. Additionally, include fallback content for better UX when video playback is unsupported.Apply this diff:
- <video controls preload='metadata' ref={mediaRef} src={getURL(url)} type={userAgentMIMETypeFallback(type)} style={{ maxWidth: 368, width: '100%' }} /> + <video controls preload='metadata' ref={mediaRef} src={getURL(url)} style={{ maxWidth: 368, width: '100%' }}> + Your browser does not support video playback. + </video>
32-32: Remove non-functionaltypeattribute from video element.The
typeattribute is valid only on<source>elements, not on the<video>element itself. When usingsrcdirectly on the video element, browsers determine the MIME type from HTTP response headers or the file extension. This attribute is non-functional and can be removed.Apply this diff:
- <video controls preload='metadata' ref={mediaRef} src={getURL(url)} type={userAgentMIMETypeFallback(type)} style={{ maxWidth: 368, width: '100%' }}> + <video controls preload='metadata' ref={mediaRef} src={getURL(url)} style={{ maxWidth: 368, width: '100%' }}> Your browser does not support video playback. </video>
31-32: Remove duplicate style properties.The same
maxWidthandwidthstyles are applied to both theMessageGenericPreviewwrapper (line 31) and thevideoelement (line 32). This is redundant; apply the styles only to the video element.Apply this diff:
- <MessageGenericPreview style={{ maxWidth: 368, width: '100%' }}> + <MessageGenericPreview> <video controls preload='metadata' ref={mediaRef} src={getURL(url)} style={{ maxWidth: 368, width: '100%' }}> Your browser does not support video playback. </video>
32-32: Add accessibility label to video element.The video element lacks an accessible label for screen readers. Use the
titleprop to provide anaria-labelattribute for better accessibility.Apply this diff:
- <video controls preload='metadata' ref={mediaRef} src={getURL(url)} style={{ maxWidth: 368, width: '100%' }}> + <video controls preload='metadata' ref={mediaRef} src={getURL(url)} aria-label={title} style={{ maxWidth: 368, width: '100%' }}> Your browser does not support video playback. </video>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/dull-laws-push.md(1 hunks)apps/meteor/app/file-upload/server/config/FileSystem.ts(3 hunks)apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx(2 hunks)apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx
- .changeset/dull-laws-push.md
- apps/meteor/app/file-upload/server/config/FileSystem.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
apps/meteor/server/ufs/ufs-store.ts (1)
getURL(304-309)
🔇 Additional comments (1)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
32-32: Core video element implementation successfully enables seeking.The transition to a native HTML video element with direct
srcattribute aligns well with the server-side Range request support added in this PR. This approach correctly enables browser-native seeking behavior via the progress bar, addressing the root issue. The suggested refactors above focus on HTML best practices and accessibility, but the core fix is sound.
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
from
05bc50e to
ad88db0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/file-upload/server/config/FileSystem.ts (1)
80-103: Consider adding range support to FileSystemAvatars for consistency.
FileSystemAvatarslacks theAccept-Rangesheader and range request handling present in the other two handlers. While avatars are typically small images that don't require range support, adding it would ensure consistency across all file handlers and future-proof the code if larger avatar formats (e.g., animated content) are supported.If desired, apply this diff to add range support:
async get(file, _req, res) { if (!this.store) { return; } const filePath = await this.store.getFilePath(file._id, file); + const options: { start?: number; end?: number } = {}; try { const stat = await fsp.stat(filePath); if (stat?.isFile()) { file = FileUpload.addExtensionTo(file); + res.setHeader('Accept-Ranges', 'bytes'); + + if (_req.headers.range) { + const range = getFileRange(file, _req); + + if (range) { + setRangeHeaders(range, file, res); + if (range.outOfRange) { + return; + } + options.start = range.start; + options.end = range.stop; + } + } + + if (!res.getHeader('Content-Length')) { + res.setHeader('Content-Length', file.size || 0); + } - (await this.store.getReadStream(file._id, file)).pipe(res); + (await this.store.getReadStream(file._id, file, options)).pipe(res); } } catch (e) { res.writeHead(404); res.end(); } },apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx (1)
57-59: Consider adding a debug log when skipping recovery due to seeking.For consistency with other early-return paths (lines 62, 75, 84), a debug log would help troubleshoot scenarios where seeking and recovery logic interact.
Apply this diff:
if ((event.target as HTMLMediaElement)?.seeking) { + console.debug('Media element is seeking, skipping recovery for event:', event.type); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/dull-laws-push.md(1 hunks)apps/meteor/app/file-upload/server/config/FileSystem.ts(3 hunks)apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx(2 hunks)apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/dull-laws-push.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx
🔇 Additional comments (4)
apps/meteor/app/file-upload/server/config/FileSystem.ts (2)
31-31: LGTM! Accept-Ranges header correctly advertises range support.The header is properly set before the range handling logic (lines 36-47), enabling browsers to request partial content for video seeking.
119-139: LGTM! Range support correctly implemented for user data files.This implementation addresses the past review comment and correctly handles HTTP Range requests:
- Advertises range support via
Accept-Ranges: bytesheader- Detects range requests and computes the requested byte range
- Sets appropriate response headers (status 206, Content-Range, Content-Length)
- Streams only the requested portion of the file
- Falls back to full file with 200 OK when no range is requested
The logic mirrors
FileSystemUploads(lines 36-47) and enables video seeking for user data files.apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx (2)
57-59: LGTM! Seeking guard correctly prevents reload during user-initiated seeks.This check directly addresses the PR objective by preventing the recovery logic from interfering when the user seeks through the video progress bar. The optional chaining (
?.) safely handles cases whereevent.targetmight be null, and checking theseekingproperty before theisRecoveringguard ensures seek operations bypass recovery entirely.
5-5: Removal of ‘play’ event is safe Remaining recovery on ‘error’ and ‘stalled’ covers all URL refresh cases; no code or tests bind to ‘play’, so URL recovery remains intact.
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
from
ad88db0 to
d20ae6e
Compare
|
I will ask if someone will look at this. Please keep your code up to date with latest code. Note - there is a very long road to getting code merged and it can take several months/releases. |
|
Pinged Engineering asking to check it out |
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
from
d20ae6e to
2798752
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/dull-laws-push.md(1 hunks)apps/meteor/app/file-upload/server/config/FileSystem.ts(3 hunks)apps/meteor/app/file-upload/server/config/GridFS.ts(2 hunks)apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx(2 hunks)apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx(3 hunks)error.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/dull-laws-push.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx
- apps/meteor/app/file-upload/server/config/GridFS.ts
- apps/meteor/app/file-upload/server/config/FileSystem.ts
🔇 Additional comments (3)
apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx (3)
5-5: Good simplification of event handling.Removing the
'play'event from the recovery trigger list is correct. Recovery should only be triggered by actual error conditions (error,stalled), not by normal playback events.
57-59: Excellent fix for the seeking bug!This early return is the core solution to the reported issue. By checking
seekingstate, the hook correctly avoids triggering URL recovery when the user is actively seeking through the video via the progress bar. This prevents the video source from being reloaded mid-seek, which was causing the playback position to jump back.The use of optional chaining ensures safe access to the
seekingproperty.
73-80: Logic flow is correct after removing play event handling.The conditional block correctly handles the first recovery attempt check, and the removal of the previous
event.type === 'play'branch simplifies the logic appropriately since'play'is no longer in the events list.
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
2 times, most recently
from
9a7bb36 to
1871cbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
22-22: Use the memoizedsrcin the video element.Line 22 creates a memoized
srcvalue, but line 30 callsgetURL(url)again instead of using it. This creates an inconsistency: theuseReloadOnErrorhook on line 23 monitors one URL (src), while the video element may load a different one (though likely the same result). Use the memoizedsrcfor consistency and to avoid redundant computation.Apply this diff:
- <video controls preload='metadata' ref={mediaRef} src={getURL(url)} style={{ maxWidth: 368, width: '100%' }}> + <video controls preload='metadata' ref={mediaRef} src={src} style={{ maxWidth: 368, width: '100%' }}>Also applies to: 30-30
apps/meteor/app/file-upload/server/config/GridFS.ts (1)
199-211: AddAccept-Ranges: bytesheader to GridFS:Avatars handler for consistency.The
GridFS:Avatarshandler omits theAccept-Ranges: bytesheader, unlikeGridFS:Uploads(line 165) andGridFS:UserDataFiles(line 185). All three handlers use the samereadFromGridFS()function, which includes full range request handling (viagetFileRange()andsetRangeHeaders()). The missing header should be added at line 202 to maintain consistency and enable seeking behavior if avatars are served as video or animated content.async get(file, req, res) { file = FileUpload.addExtensionTo(file); res.setHeader('Accept-Ranges', 'bytes'); await readFromGridFS(file.store, file._id, file, req, res); },
♻️ Duplicate comments (2)
apps/meteor/app/file-upload/server/config/GridFS.ts (1)
185-189: Same Content-Length ordering concern applies here.This handler has the same header ordering pattern as
GridFS:Uploads(line 165-169). EnsuresetRangeHeadersproperly overwritesContent-Lengthfor range responses.apps/meteor/app/file-upload/server/config/FileSystem.ts (1)
80-103: Avatars endpoint also lacks range request support.Similar to
GridFS:Avatars, theFileSystemAvatarshandler does not include range request support. This is likely intentional if avatars are exclusively static images, but should be verified if any video or animated content is served via this endpoint.
🧹 Nitpick comments (2)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
29-30: Consider removing duplicate styles.The style
{ maxWidth: 368, width: '100%' }is applied to both theMessageGenericPreviewcontainer and thevideoelement. Since the video already has these constraints, applying them to the parent is redundant.Apply this diff to remove the duplicate style from the parent:
- <MessageGenericPreview style={{ maxWidth: 368, width: '100%' }}> + <MessageGenericPreview> <video controls preload='metadata' ref={mediaRef} src={src} style={{ maxWidth: 368, width: '100%' }}>apps/meteor/app/file-upload/server/config/GridFS.ts (1)
165-169: Inconsistency in Content-Length header handling pattern, but no functional risk.The concern about incorrect content-length reaching browsers is invalid. The
setRangeHeadersfunction explicitly removes theContent-Lengthheader (line 55 in ranges.ts) before setting it to the correct partial content length (line 56), ensuring 206 responses have accurate metadata.However, there is a real inconsistency in approach:
FileSystem.tsuses a defensive pattern with conditional checks (if (!res.getHeader('Content-Length'))at lines 50 and 136), whileGridFS.tssets the header unconditionally (line 169) and relies onsetRangeHeadersto override it. The FileSystem pattern is cleaner since it avoids unnecessary header removal/replacement cycles.For consistency and clarity, consider adopting the FileSystem pattern in GridFS.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/dull-laws-push.md(1 hunks)apps/meteor/app/file-upload/server/config/FileSystem.ts(3 hunks)apps/meteor/app/file-upload/server/config/GridFS.ts(2 hunks)apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx(2 hunks)apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/dull-laws-push.md
- apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
apps/meteor/server/ufs/ufs-store.ts (1)
getURL(304-309)
🔇 Additional comments (2)
apps/meteor/app/file-upload/server/config/FileSystem.ts (2)
31-54: LGTM! Range request handling is correctly implemented.The implementation properly:
- Advertises range support via
Accept-Ranges: bytes- Parses and validates range requests using
getFileRange- Sets appropriate response headers via
setRangeHeaders- Handles out-of-bounds ranges with early return
- Conditionally sets
Content-Lengthonly if not already set (avoiding overwrite of partial content length)- Passes start/end options to the read stream for efficient partial reads
This follows the correct pattern for HTTP 206 Partial Content responses.
119-139: LGTM! Range request handling correctly mirrors FileSystemUploads implementation.The implementation correctly addresses the previous review feedback (past comment on line 119). The range handling logic is now complete and consistent with
FileSystemUploads, properly supporting HTTP 206 Partial Content responses for user data files.
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
from
1871cbd to
67ee0de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
31-31: Remove the empty track element.The
trackelement specifieskind='captions'but lacks asrcattribute. Without a source, this element serves no purpose and should be removed.Apply this diff:
<video controls preload='metadata' ref={mediaRef} src={src}> - <track kind='captions' /> </video>apps/meteor/app/file-upload/server/config/FileSystem.ts (1)
119-139: I need to gather more information to verify the concerns. Let me inspect the implementation files directly.Now let me inspect the ranges.ts file and the FileSystem.ts file more closely:
Let me inspect the ranges.ts file and FileSystem.ts directly:
Based on my verification, I have found that the review comment identifies legitimate code quality and consistency issues. The code under review does not properly align with the same implementation pattern in the Uploads configuration (earlier in the same file) and doesn't follow best practices for HTTP range request handling. The issues are actionable and correct.
Rename
_reqtoreqand update Content-Type default for consistency and clarity.The file
FileSystem.tscontains two separate handlers: one forUploads(lines ~34–36) already applies the suggested improvements, while theUserDataFileshandler (lines 122–139) has not. fs.createReadStream'sendparameter is inclusive, which aligns with range.stop semantics in HTTP range requests. The inconsistency suggests this was an oversight during implementation.Apply these fixes:
- async get(file, _req, res) { + async get(file, req, res) { @@ - res.setHeader('Content-Type', file.type || ''); + res.setHeader('Content-Type', file.type || 'application/octet-stream'); @@ - if (_req.headers.range) { - const range = getFileRange(file, _req); + if (req.headers.range) { + const range = getFileRange(file, req);
🧹 Nitpick comments (3)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (2)
30-30: Use the memoizedsrcvariable instead of callinggetURL(url)again.Line 22 already memoizes
srcviauseMemo(() => getURL(url), [getURL, url]), but line 30 callsgetURL(url)again. This bypasses the memoization and can trigger unnecessary recalculations.Apply this diff:
- <video controls preload='metadata' ref={mediaRef} src={getURL(url)} style={{ maxWidth: 368, width: '100%' }}> + <video controls preload='metadata' ref={mediaRef} src={src} style={{ maxWidth: 368, width: '100%' }}>
29-30: Remove duplicate inline styles.Both
MessageGenericPreview(line 29) and thevideoelement (line 30) specify identical inline styles{{ maxWidth: 368, width: '100%' }}. The parent container's styles should be sufficient.Apply this diff to remove the duplicate:
<MessageGenericPreview style={{ maxWidth: 368, width: '100%' }}> - <video controls preload='metadata' ref={mediaRef} src={src} style={{ maxWidth: 368, width: '100%' }}> + <video controls preload='metadata' ref={mediaRef} src={src}>apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx (1)
57-59: Correct fix: Suppress recovery during media seek operations.The seeking check at lines 57–59 is properly implemented. The
HTMLMediaElement.seekingproperty is standard across all browsers and correctly identifies when a user is actively seeking. The optional chaining safely handles non-media elements, and the early return prevents false error recovery during normal seek operations.However, the test suite lacks an explicit test for this seeking scenario. While the fix is sound, add a test case to verify that seek events don't trigger unnecessary recovery:
it('does not recover on seek (seeking property true)', async () => { const original = '/sampleurl?token=abc'; const { result } = renderHook(() => useReloadOnError(original, 'audio')); const media = makeMediaEl(); act(() => { result.current.mediaRef(media); }); // Mock seeking as true Object.defineProperty(media, 'seeking', { value: true, configurable: true }); await act(async () => { media._emit('stalled'); // Would normally trigger recovery }); // No fetch should occur while seeking is true expect(global.fetch).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/dull-laws-push.md(1 hunks)apps/meteor/app/file-upload/server/config/FileSystem.ts(3 hunks)apps/meteor/app/file-upload/server/config/GridFS.ts(2 hunks)apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx(2 hunks)apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/dull-laws-push.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/file-upload/server/config/GridFS.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
apps/meteor/server/ufs/ufs-store.ts (1)
getURL(304-309)
🔇 Additional comments (4)
apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (1)
2-2: LGTM: Import cleanup is correct.Removing the unused
Boximport is appropriate since the video element is now rendered directly without a Box wrapper.apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx (1)
5-5: LGTM: Correctly removed 'play' from error events.The 'play' event is not an error condition and shouldn't trigger recovery logic. This change makes the event list semantically correct by limiting it to actual error states.
apps/meteor/app/file-upload/server/config/FileSystem.ts (2)
31-31: Good addition: advertise byte ranges on uploads.Setting Accept-Ranges: bytes enables native seeking; placement is correct post file existence check.
159-159: No functional change.Looks like formatting/closing brace movement only; nothing to action.
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
from
67ee0de to
11feff8
Compare
NAME-ASHWANIYADAV
force-pushed
the
fix/video-seeking-bug
branch
from
11feff8 to
d48fc43
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/file-upload/server/config/FileSystem.ts (1)
80-103: Consider range support for consistency.
FileSystemAvatarsdoes not implement range request handling, unlike the other handlers. While avatars are typically small and don't require seeking, adding range support would provide consistency across all file handlers and could benefit clients with slow connections requesting large avatar images.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/dull-laws-push.md(1 hunks)apps/meteor/app/file-upload/server/config/FileSystem.ts(3 hunks)apps/meteor/app/file-upload/server/config/GridFS.ts(2 hunks)apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx(2 hunks)apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/file-upload/server/config/GridFS.ts
- apps/meteor/client/components/message/content/attachments/file/hooks/useReloadOnError.tsx
🔇 Additional comments (5)
.changeset/dull-laws-push.md (1)
1-5: LGTM!The changeset correctly documents this patch release for video seeking support.
apps/meteor/app/file-upload/server/config/FileSystem.ts (2)
31-54: LGTM!The HTTP range request implementation is correct and complete. The handler properly advertises range support, parses range headers, sets 206 status with appropriate headers via
setRangeHeaders, and streams only the requested byte range.
119-139: LGTM! Past review concern addressed.The range request implementation is now complete and matches the
FileSystemUploadshandler. This correctly resolves the critical issue flagged in the previous review whereAccept-Rangeswas advertised without implementation.apps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsx (2)
11-20: LGTM!Removing
video_typefrom the props is correct since the video element now uses a directsrcattribute instead of a nested<source type="...">element. Modern browsers will determine the MIME type automatically.
29-30: LGTM! Cleaner video rendering.The simplified approach using a native
<video>element with a directsrcattribute is more maintainable and aligns with modern HTML5 video practices. The removal of the Box wrapper and nested source element reduces unnecessary complexity.
|
Hello mentors, Thank you for the feedback. I have now addressed all the suggestions from the I believe all the required actions from my side are now complete. As this is my first contribution and I'm new to the open-source process, please let me know if I've missed anything or if there are any further changes needed. I would appreciate it if you could take another look. I'm looking forward to your feedback. Thank you! |
Please note no one is mentoring you. This is not school. Your PR will get reviewed in due course but nothing moves fast here and it may take weeks or even months. |
|
Ok , I will wait and I just said mentors in respect to fellow members or contributes who are more experienced then me , I apologise if it offended anyone |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37197 +/- ##
===========================================
+ Coverage 70.56% 70.60% +0.04%
===========================================
Files 3182 3182
Lines 112405 112396 -9
Branches 20412 20387 -25
===========================================
+ Hits 79316 79358 +42
+ Misses 31033 30987 -46
+ Partials 2056 2051 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4 participants
Closes #22308
Proposed changes
This pull request resolves a long-standing bug that prevented users from seeking through uploaded video files. The core of the issue was twofold, involving both server-side headers and a client-side race condition.
1. The Root Cause: Lack of Server Support for Range Requests
Modern web browsers rely on HTTP Range Requests (sending a
Rangeheader) to stream and seek through video content. When a user clicks on a video progress bar, the browser sends a request to the server asking for only a specific "byte range" of the video file, rather than re-downloading the entire file.For a server to honor these requests, it must first advertise its capability by sending the
Accept-Ranges: bytesheader in its initial response. The Rocket.Chat server was not sending this header for file uploads, causing most browsers to fall back to downloading the entire file from the beginning, thus making seeking impossible.2. The Client-Side Issue: False Error State During Seeking
Simultaneously, a client-side hook,
useReloadOnError, was incorrectly interpreting the browser's attempt to seek as a playback error. When a user tried to seek, the video'sseekingproperty would becometrue. This state was not being handled, causing the error-handling logic to trigger a reload of the video source, which forced the playback position to jump back to where it was.The Solution
This PR implements a comprehensive fix addressing both issues:
Server-Side Fix (
FileSystem.ts&GridFS.ts):The
Accept-Ranges: bytesheader has been added to the response for file uploads. This correctly signals to the browser that our server supports HTTP 206 Partial Content responses, enabling native browser seeking.Client-Side Fix (
useReloadOnError.tsx):The
useReloadOnErrorhook has been updated to check if the video element is currently in aseekingstate (event.target.seeking). If it is, the reload logic is skipped. This prevents the hook from interfering with the browser's native seeking behavior.Together, these changes ensure a smooth and functional seeking experience across all modern browsers.
Steps to test or reproduce
.mp4video file to a channel.Further comments
This is my first contribution to Rocket.Chat. I'm happy to make any changes or improvements based on your feedback.
Summary by CodeRabbit
New Features
Bug Fixes
Style