updated with preview thumbnails#2597
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughBackend video metadata extraction now probes local media files for duration, codec, and container details with a duration fallback. Frontend adds shared playback query helpers, new Plyr scrub/seek/loading modules, and viewer wiring for deferred video loading and query-driven seeking. ChangesBackend video metadata probing
Frontend Plyr playback UX
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Files as extractVideoMetadata
participant Ffmpeg as ffmpeg.Service
participant Probe as ProbeStream
Files->>Ffmpeg: ProbeFile(ctx, mediaPath)
Ffmpeg->>Probe: ProbeStream(file, 30s timeout)
Probe-->>Ffmpeg: duration, videoCodec, audioCodec, formatName
Ffmpeg-->>Files: FileProbeInfo or error
alt probe fails
Files->>Ffmpeg: GetMediaDuration(localPath)
Ffmpeg-->>Files: duration
end
Files->>Files: set item.Metadata when fields are present
sequenceDiagram
participant Route as $route.query
participant Viewer as plyrViewer.vue
participant PlyrExt as scrub/seek/loading modules
participant Player as Plyr player
Route->>Viewer: query change (time/transcode)
Viewer->>Viewer: buildPlaybackQueryPatch / playbackQueryChanged
Viewer->>Player: apply seek on loadedmetadata
Viewer->>PlyrExt: enablePlyrScrubPreview(player)
Viewer->>PlyrExt: enablePlyrSeekOnRelease(player)
Viewer->>PlyrExt: enablePlyrVideoLoadingIndicator(player, onLoadingChange)
PlyrExt-->>Viewer: videoPlaybackLoading updates
Viewer->>Viewer: destroyPlyr() tears down extensions
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
backend/ffmpeg/metadata.go (1)
21-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated Acquire/Release boilerplate.
ProbeFilerepeats the exactnilguard +Acquire/defer Releasepattern already present inGetMediaDuration(Line 49-66 in this file). Extracting a small helper would remove this duplication and keep the guard/lock lifecycle consistent across future ffmpeg service methods.♻️ Proposed consolidation
+func (s *Service) withAcquired(ctx context.Context, fn func() error) error { + if s == nil || s.inner == nil { + return fmt.Errorf("ffmpeg service not available") + } + if err := s.Acquire(ctx); err != nil { + return err + } + defer s.Release() + return fn() +} + func (s *Service) ProbeFile(ctx context.Context, mediaPath string) (*FileProbeInfo, error) { - if s == nil || s.inner == nil { - return nil, fmt.Errorf("ffmpeg service not available") - } - - if err := s.Acquire(ctx); err != nil { - return nil, err - } - defer s.Release() - - info, err := s.inner.ProbeStream(ctx, goffmpeg.ProbeStreamOptions{ - URL: mediaPath, - StreamType: goffmpeg.StreamFile, - Timeout: 30 * time.Second, - }) - if err != nil { - return nil, err - } - - return &FileProbeInfo{ - Duration: info.Duration, - VideoCodec: info.VideoCodec, - AudioCodec: info.AudioCodec, - FormatName: info.FormatName, - }, nil + var result *FileProbeInfo + err := s.withAcquired(ctx, func() error { + info, err := s.inner.ProbeStream(ctx, goffmpeg.ProbeStreamOptions{ + URL: mediaPath, + StreamType: goffmpeg.StreamFile, + Timeout: 30 * time.Second, + }) + if err != nil { + return err + } + result = &FileProbeInfo{ + Duration: info.Duration, + VideoCodec: info.VideoCodec, + AudioCodec: info.AudioCodec, + FormatName: info.FormatName, + } + return nil + }) + return result, err }As per path instructions, "reduce duplication where there is clear duplication found, use consolidated functions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/ffmpeg/metadata.go` around lines 21 - 46, `ProbeFile` duplicates the same nil-check and `Acquire`/`defer Release` lifecycle already used by `GetMediaDuration`; extract that shared ffmpeg service setup into a small helper and have both methods use it so the guard/lock flow stays consistent. Keep the helper tied to `Service` (for example around `Acquire`, `Release`, and the nil/inner availability check) and update `ProbeFile` to focus only on `ProbeStream` and building `FileProbeInfo`.Source: Path instructions
backend/adapters/fs/files/files.go (1)
565-605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the probe-first fallback paths.
extractVideoMetadatahas no coverage for probe success, duration fallback, or the dual-failure path; a small table-driven test inbackend/adapters/fs/files/file_test.gowould protect these branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/adapters/fs/files/files.go` around lines 565 - 605, Add table-driven unit tests for extractVideoMetadata to cover the probe-first paths in files.go. Exercise the case where FFmpegService.ProbeFile succeeds and populates item.Metadata, the case where ProbeFile fails but GetMediaDuration succeeds and sets only Duration, and the case where both fail and the original error is returned. Use the extractVideoMetadata helper with a mocked ffmpeg.FFmpegService (or test double) in file_test.go so these branches are locked down.Source: Path instructions
frontend/src/utils/playbackQuery.js (2)
101-107: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo finite-number guard in
formatTimeForQuery.If called with a non-finite
seconds(e.g.NaN), this produces"NaN:NaN:NaN"instead of failing safely. All current call sites already guard withNumber.isFinitebefore calling, so this isn't exploitable today, but it's a fragile implicit contract for a small, easily-testable utility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/playbackQuery.js` around lines 101 - 107, `formatTimeForQuery` currently assumes `seconds` is finite, which can produce invalid output like `NaN:NaN:NaN` for bad inputs. Add an explicit `Number.isFinite` guard at the start of `formatTimeForQuery` and make it fail safely for non-finite values, while keeping the existing formatting logic unchanged for valid numbers.
13-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify
parsePlainSecondsStringwith existing regex pattern.This manually walks characters to parse an integer/fraction, duplicating logic that's already expressed more simply elsewhere in the file (e.g. the
/^\d+(\.\d+)?$/regex +Number()pattern used inparseClockTimeString). Reusing that pattern here would be shorter and easier to maintain.♻️ Suggested simplification
function parsePlainSecondsString(str) { - if (str.includes(':')) { - return null; - } - let i = 0; - let intPart = 0; - while (i < str.length && str.charAt(i) >= '0' && str.charAt(i) <= '9') { - intPart = (intPart * 10) + (str.charCodeAt(i) - 48); - i += 1; - } - if (i === 0) { - return null; - } - let frac = 0; - let fracDiv = 1; - if (i < str.length && str.charAt(i) === '.') { - i += 1; - while (i < str.length && str.charAt(i) >= '0' && str.charAt(i) <= '9') { - frac = (frac * 10) + (str.charCodeAt(i) - 48); - fracDiv *= 10; - i += 1; - } - } - if (i !== str.length) { - return null; - } - return intPart + (frac / fracDiv); + if (!/^\d+(\.\d+)?$/.test(str)) { + return null; + } + const value = Number(str); + return Number.isFinite(value) ? value : null; }The static-analysis "unsafe regex" warning on the similar pattern in
parseClockTimeString(line 70) is a false positive here too — there's no nested/overlapping quantifier that could cause catastrophic backtracking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/playbackQuery.js` around lines 13 - 40, Simplify parsePlainSecondsString by replacing the manual character-by-character parsing with the same regex-plus-Number approach already used in parseClockTimeString. Keep the existing validation behavior (reject strings with colons, non-numeric input, or trailing junk) by checking the /^\d+(\.\d+)?$/ pattern before converting. This change should be localized to parsePlainSecondsString in playbackQuery.js and should preserve the current return values.frontend/src/plyr/plyrScrubPreview.js (1)
148-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant branch in
getScrubPreviewMount.Both branches of the inner
if(lines 152-155) returnfsEl, so the condition checkingcontainercontainment has no effect on the outcome.♻️ Suggested simplification
export function getScrubPreviewMount(player) { const fsEl = document.fullscreenElement ?? document.webkitFullscreenElement ?? null; const container = player?.elements?.container; if (fsEl instanceof HTMLElement) { - if (container instanceof HTMLElement && (fsEl === container || fsEl.contains(container))) { - return fsEl; - } return fsEl; } if (container instanceof HTMLElement && player?.fullscreen?.active) { return container; } return container instanceof HTMLElement ? container : document.body; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/plyr/plyrScrubPreview.js` around lines 148 - 161, Simplify getScrubPreviewMount by removing the redundant inner conditional on container containment, since both branches return fsEl. Keep the fullscreenElement lookup and the player?.elements?.container / player?.fullscreen?.active fallback behavior intact, but collapse the fsEl instanceof HTMLElement path in plyrScrubPreview.js so the function’s return logic is clearer and has no no-op branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/ffmpeg/metadata.go`:
- Around line 31-35: The ProbeStream call in the metadata lookup path currently
passes mediaPath directly without restricting allowed protocols, so the ffmpeg
probe can use default protocol handling on untrusted uploads. Update the
ProbeStream invocation in the metadata-reading flow to explicitly whitelist only
local file access (or otherwise sandbox the probe) using the existing
ProbeStreamOptions/ProbeStream call site so ProbeStream cannot open arbitrary
protocols from uploaded media.
In `@frontend/src/plyr/plyrScrubPreview.js`:
- Around line 83-99: The scrub percent logic is duplicated between
scrubPercentFromEvent and the local scrubPercentFromPointer inside
enablePlyrScrubPreview. Add an optional step parameter to scrubPercentFromEvent,
use it when calling quantizeScrubPercent, and replace the inner
scrubPercentFromPointer implementation with a call to scrubPercentFromEvent so
both paths share one source of truth.
---
Nitpick comments:
In `@backend/adapters/fs/files/files.go`:
- Around line 565-605: Add table-driven unit tests for extractVideoMetadata to
cover the probe-first paths in files.go. Exercise the case where
FFmpegService.ProbeFile succeeds and populates item.Metadata, the case where
ProbeFile fails but GetMediaDuration succeeds and sets only Duration, and the
case where both fail and the original error is returned. Use the
extractVideoMetadata helper with a mocked ffmpeg.FFmpegService (or test double)
in file_test.go so these branches are locked down.
In `@backend/ffmpeg/metadata.go`:
- Around line 21-46: `ProbeFile` duplicates the same nil-check and
`Acquire`/`defer Release` lifecycle already used by `GetMediaDuration`; extract
that shared ffmpeg service setup into a small helper and have both methods use
it so the guard/lock flow stays consistent. Keep the helper tied to `Service`
(for example around `Acquire`, `Release`, and the nil/inner availability check)
and update `ProbeFile` to focus only on `ProbeStream` and building
`FileProbeInfo`.
In `@frontend/src/plyr/plyrScrubPreview.js`:
- Around line 148-161: Simplify getScrubPreviewMount by removing the redundant
inner conditional on container containment, since both branches return fsEl.
Keep the fullscreenElement lookup and the player?.elements?.container /
player?.fullscreen?.active fallback behavior intact, but collapse the fsEl
instanceof HTMLElement path in plyrScrubPreview.js so the function’s return
logic is clearer and has no no-op branch.
In `@frontend/src/utils/playbackQuery.js`:
- Around line 101-107: `formatTimeForQuery` currently assumes `seconds` is
finite, which can produce invalid output like `NaN:NaN:NaN` for bad inputs. Add
an explicit `Number.isFinite` guard at the start of `formatTimeForQuery` and
make it fail safely for non-finite values, while keeping the existing formatting
logic unchanged for valid numbers.
- Around line 13-40: Simplify parsePlainSecondsString by replacing the manual
character-by-character parsing with the same regex-plus-Number approach already
used in parseClockTimeString. Keep the existing validation behavior (reject
strings with colons, non-numeric input, or trailing junk) by checking the
/^\d+(\.\d+)?$/ pattern before converting. This change should be localized to
parsePlainSecondsString in playbackQuery.js and should preserve the current
return values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4831fb78-98a0-4d84-82af-5d19f7a81e26
⛔ Files ignored due to path filters (3)
backend/swagger/docs/docs.gois excluded by!backend/swagger/**backend/swagger/docs/swagger.jsonis excluded by!backend/swagger/**backend/swagger/docs/swagger.yamlis excluded by!backend/swagger/**
📒 Files selected for processing (9)
backend/adapters/fs/files/files.gobackend/ffmpeg/metadata.gobackend/indexing/iteminfo/fileinfo.gofrontend/src/plyr/plyrScrubPreview.jsfrontend/src/plyr/plyrSeekOnRelease.jsfrontend/src/plyr/plyrVideoLoading.jsfrontend/src/utils/playbackQuery.jsfrontend/src/utils/playbackQuery.test.jsfrontend/src/views/files/plyrViewer.vue
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/utils/playbackQuery.js (1)
185-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize existing playback params before re-emitting them.
buildPlaybackQueryPatch()validates new patch values, but existingquery.timeandquery.transcodeare copied back as-is. An invalid current URL like?transcode=foocan survive unrelated patches instead of being dropped/canonicalized.Proposed fix
- let timeValue = queryTime; - if (timeValue === undefined) { - const parsedTime = parsePlaybackTimeFromQuery(query); - timeValue = parsedTime === null ? undefined : formatTimeForQuery(parsedTime); - } - let transcodeValue = queryTranscode; + const parsedTime = parsePlaybackTimeFromQuery(query); + let timeValue = parsedTime === null ? undefined : formatTimeForQuery(parsedTime); + let transcodeValue = parseTranscodeModeFromQuery(query) ?? undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/playbackQuery.js` around lines 185 - 215, buildPlaybackQueryPatch() only validates values coming from patch, but it reuses existing queryTime and queryTranscode without normalization, so invalid URL params can persist. Update the logic around queryTime/queryTranscode in buildPlaybackQueryPatch() to parse/validate existing playback params the same way as incoming patch values, and only emit canonical values via formatTimeForQuery() and VALID_TRANSCODE_MODES; otherwise drop them from next.
🧹 Nitpick comments (1)
backend/ffmpeg/metadata_test.go (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for the directory-rejection branch.
The table covers empty/remote/protocol/relative-path rejections but not the "media path is a directory" branch (metadata.go Lines 47-49), which is easy to add given the existing structure (
t.TempDir()itself is a directory).As per path instructions, "ensure functional features include new unit tests where its easily possible."
✅ Suggested additional test case
{name: "absolute local file", path: tmpPath}, + {name: "directory path", path: t.TempDir(), wantErr: true}, {name: "empty path", path: "", wantErr: true},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/ffmpeg/metadata_test.go` around lines 19 - 30, Add a unit test case to cover the directory-rejection branch in MetadataInfo; the current table in metadata_test.go covers empty, remote, protocol, and relative paths but not when the input is a directory. Use the existing MetadataInfo table-driven test and add a case that passes a t.TempDir() value (or another directory path) with wantErr true so the directory check in metadata.go is exercised.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/utils/playbackQuery.js`:
- Around line 185-215: buildPlaybackQueryPatch() only validates values coming
from patch, but it reuses existing queryTime and queryTranscode without
normalization, so invalid URL params can persist. Update the logic around
queryTime/queryTranscode in buildPlaybackQueryPatch() to parse/validate existing
playback params the same way as incoming patch values, and only emit canonical
values via formatTimeForQuery() and VALID_TRANSCODE_MODES; otherwise drop them
from next.
---
Nitpick comments:
In `@backend/ffmpeg/metadata_test.go`:
- Around line 19-30: Add a unit test case to cover the directory-rejection
branch in MetadataInfo; the current table in metadata_test.go covers empty,
remote, protocol, and relative paths but not when the input is a directory. Use
the existing MetadataInfo table-driven test and add a case that passes a
t.TempDir() value (or another directory path) with wantErr true so the directory
check in metadata.go is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c999802-729e-457e-a144-d874c593dca7
📒 Files selected for processing (5)
backend/ffmpeg/metadata.gobackend/ffmpeg/metadata_test.gofrontend/src/plyr/plyrScrubPreview.jsfrontend/src/utils/playbackQuery.jsfrontend/src/views/files/ThreeJs.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/plyr/plyrScrubPreview.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
According to the contributing guide, A PR should contain:
Additional Details
Summary by CodeRabbit