feat: add video preview support #1178
Conversation
WalkthroughAdds an ffmpeg-based ThumbnailGenerator, integrates it into the preview Model to generate and cache video thumbnails during rendering, and ensures preview temporary files are cleaned up when quitting. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Preview UI
participant Model as Preview Model
participant Gen as ThumbnailGenerator
participant FFmpeg as ffmpeg
participant FS as Filesystem
UI->>Model: RenderWithPath(videoPath)
Model->>Model: isVideoFile(videoPath)?
alt is video
Model->>Gen: GetThumbnailOrGenerate(videoPath)
alt cached & exists
Gen->>FS: stat(cachedThumbnail)
FS-->>Gen: exists
Gen-->>Model: cachedPath
else generate
Gen->>Gen: create temp output path
Gen->>FFmpeg: run ffmpeg to extract frame -> write file
FFmpeg-->>Gen: success (thumbnail path)
Gen->>Gen: store in cache
Gen-->>Model: generatedPath
end
Model->>FS: read thumbnail image
FS-->>Model: image data
Model-->>UI: render image preview
else not video
Model-->>UI: render normal preview
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/internal/ui/preview/model.go (3)
41-41: Consider making the field unexported.The
thumbnailGeneratorfield is currently exported (public). Since it's only used internally by the Model and accessed through the CleanUp() method, consider making it unexported (thumbnailGenerator) to better encapsulate the implementation details.Apply this diff to unexport the field:
- thumbnailGenerator *filepreview.ThumbnailGenerator + thumbnailGenerator *filepreview.ThumbnailGeneratorNote: This is a minor encapsulation improvement and can be deferred if there are plans to access this field externally.
295-302: Consider logging thumbnail generation errors.The code handles thumbnail generation errors by returning an unsupported format message, but the actual error from
GetThumbnailOrGenerateis not logged. This could make debugging difficult when thumbnail generation fails.Apply this diff to add error logging:
if isVideoFile(itemPath) && m.thumbnailGenerator != nil { thumbnailPath, err := m.thumbnailGenerator.GetThumbnailOrGenerate(itemPath) if err != nil { + slog.Error("Failed to generate video thumbnail", "path", itemPath, "error", err) return renderUnsupportedFormat(box) + clearCmd } return m.renderImagePreview(box, thumbnailPath, previewWidth, previewHeight, fullModelWidth-previewWidth+1) }
379-393: Consider adding support for additional video formats.The current implementation covers common video formats, but several widely-used formats are missing. Consider adding support for:
.m4v(Apple QuickTime).mpeg,.mpg(MPEG).3gp(mobile video).ogv(Ogg Video).ts,.m2ts(transport stream)Apply this diff to add additional video formats:
func isVideoFile(filename string) bool { videoExtensions := map[string]bool{ ".mkv": true, ".mp4": true, ".mov": true, ".avi": true, ".flv": true, ".webm": true, ".wmv": true, + ".m4v": true, + ".mpeg": true, + ".mpg": true, + ".3gp": true, + ".ogv": true, + ".ts": true, + ".m2ts": true, } ext := strings.ToLower(filepath.Ext(filename)) return videoExtensions[ext] }src/pkg/file_preview/thumbnail_generator.go (2)
38-58: Consider adding cache management policies.The thumbnail cache has no size limits or TTL, which could lead to unbounded memory growth and disk usage over time. Consider implementing:
- Maximum cache size (evict oldest entries when limit reached)
- TTL for cache entries (regenerate thumbnails after expiration)
- Monitoring of temp directory disk usage
This is not critical for initial release but should be tracked for future improvement. The current implementation is acceptable for initial MVP.
98-106: LGTM! Consider adding timeout to ffmpeg availability check.The cleanup implementation correctly removes the entire temp directory. The ffmpeg availability check is appropriate, though it could benefit from a timeout to prevent hanging during initialization.
Apply this diff to add a timeout to the ffmpeg check:
func isFFmpegInstalled() bool { - err := exec.Command("ffmpeg", "-version").Run() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + err := exec.CommandContext(ctx, "ffmpeg", "-version").Run() return err == nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/internal/model.go(1 hunks)src/internal/ui/preview/model.go(6 hunks)src/pkg/file_preview/thumbnail_generator.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
Applied to files:
src/internal/model.gosrc/internal/ui/preview/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
Applied to files:
src/internal/model.gosrc/internal/ui/preview/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-06T13:42:44.590Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1039
File: src/internal/ui/zoxide/model.go:53-54
Timestamp: 2025-09-06T13:42:44.590Z
Learning: The zoxide modal in src/internal/ui/zoxide/model.go is missing handling for common.Hotkeys.Quit when zClient is nil (lines 52-54), only handling ConfirmTyping and CancelTyping. This creates inconsistency with other modals like sort options menu, help menu, and notify model which all properly handle the Quit hotkey. The prompt modal has the same inconsistency.
Applied to files:
src/internal/model.go
🧬 Code graph analysis (1)
src/internal/ui/preview/model.go (2)
src/pkg/file_preview/image_preview.go (3)
ImagePreviewer(45-48)NewImagePreviewer(51-53)Preview(37-42)src/pkg/file_preview/thumbnail_generator.go (2)
ThumbnailGenerator(15-18)NewThumbnailGenerator(20-36)
🔇 Additional comments (4)
src/internal/model.go (1)
764-764: LGTM! Proper cleanup on application exit.The cleanup call is appropriately placed to ensure thumbnail generator resources are freed before the application quits. The CleanUp() method handles nil checks and error logging internally.
src/internal/ui/preview/model.go (2)
122-129: LGTM! Proper nil check and error handling.The CleanUp method correctly checks for nil before calling the thumbnail generator's cleanup, and logs errors appropriately. This ensures safe cleanup even when thumbnail generator creation fails.
45-53: Remove reference to non-existent ThumbnailGenerator code.The review comment references ThumbnailGenerator instantiation, but the actual code uses ImagePreviewer (line 46 in src/internal/ui/preview/model.go). There is no NewThumbnailGenerator() function or thumbnailGenerator field in the codebase. The code correctly creates a single ImagePreviewer instance in New() and defaultModelConfig does not duplicate this initialization.
Likely an incorrect or invalid review comment.
src/pkg/file_preview/thumbnail_generator.go (1)
10-13: LGTM! Consider if 100MB is appropriate for thumbnail size.The constants are well-defined. However, note that 100MB is quite large for a thumbnail image. Typical thumbnails are much smaller (< 1MB). Consider whether this limit is intentional or if it should be reduced for better performance and disk usage.
| func NewThumbnailGenerator() (*ThumbnailGenerator, error) { | ||
| if !isFFmpegInstalled() { | ||
| return nil, errors.New("ffmpeg is not installed") | ||
| } | ||
|
|
||
| tmp, err := os.MkdirTemp("", "superfiles-*") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| thumbnailGenerator := &ThumbnailGenerator{ | ||
| tempFiles: make(map[string]string), | ||
| tempDirectory: tmp, | ||
| } | ||
|
|
||
| return thumbnailGenerator, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for temp directory patterns and application naming conventions
echo "=== Searching for MkdirTemp and CreateTemp patterns ==="
rg -n "MkdirTemp|CreateTemp" --type=go -A 1 -B 1
echo -e "\n=== Searching for 'superfile' or 'superfiles' patterns ==="
rg -n "superfile" --type=go -i
echo -e "\n=== Searching for 'superfiles' in all file types ==="
rg -n "superfiles" --ignore-caseRepository: yorukot/superfile
Length of output: 21785
Change temp directory pattern from "superfiles-*" to "superfile-*".
The temporary directory pattern uses the plural form "superfiles-*" (line 25), but the application consistently uses the singular form "superfile" throughout the codebase in all configuration directories, command names, and references. Update to "superfile-*" for consistency.
🤖 Prompt for AI Agents
In src/pkg/file_preview/thumbnail_generator.go around lines 20 to 36, the temp
directory pattern uses the plural "superfiles-*" but the codebase standard is
the singular "superfile-*"; update the os.MkdirTemp call to use "superfile-*" so
the temporary directory name matches the rest of the application and maintain
consistency across configs and references.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/internal/ui/preview/model.go (2)
295-302: Consider improving error handling for thumbnail generation failures.When thumbnail generation fails, the code returns
renderUnsupportedFormat. This provides limited context to users. Consider either:
- Showing a more specific error message indicating thumbnail generation failed
- Falling back to text preview with file metadata
Example improvement:
if isVideoFile(itemPath) && m.thumbnailGenerator != nil { thumbnailPath, err := m.thumbnailGenerator.GetThumbnailOrGenerate(itemPath) if err != nil { - return renderUnsupportedFormat(box) + clearCmd + slog.Error("Failed to generate video thumbnail", "error", err, "path", itemPath) + return box.Render("\n --- " + icon.Error + " Could not generate video preview ---") + clearCmd } return m.renderImagePreview(box, thumbnailPath, previewWidth, previewHeight, fullModelWidth-previewWidth+1) }
386-400: Implementation is correct; consider adding more video formats.The
isVideoFilehelper is well-implemented and follows the same pattern asisImageFile. However, it's missing several common video formats that were incorrectly added toisImageFile(see previous comment). Consider adding.m4v,.mpeg,.mpg,.3gp,.ogv,.ts, and.m2tsto thevideoExtensionsmap for comprehensive video format support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/internal/ui/preview/model.go(6 hunks)src/pkg/file_preview/thumbnail_generator.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/file_preview/thumbnail_generator.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
Applied to files:
src/internal/ui/preview/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
Applied to files:
src/internal/ui/preview/model.go
🧬 Code graph analysis (1)
src/internal/ui/preview/model.go (2)
src/pkg/file_preview/image_preview.go (2)
ImagePreviewer(45-48)NewImagePreviewer(51-53)src/pkg/file_preview/thumbnail_generator.go (2)
ThumbnailGenerator(18-22)NewThumbnailGenerator(24-40)
🔇 Additional comments (4)
src/internal/ui/preview/model.go (4)
33-42: LGTM!The addition of the
thumbnailGeneratorfield to the Model struct is appropriate and follows the existing pattern used forimagePreviewer.
44-57: Initialization looks good; no duplication risk detected.The thumbnailGenerator is initialized once in New(), which avoids the duplicate initialization pattern that caused crashes in PR #1021. The error handling allows graceful degradation when ffmpeg is unavailable.
Based on learnings, previous issues arose from multiple ImagePreviewer instances; this implementation correctly creates a single ThumbnailGenerator instance.
44-57: This review comment is incorrect and addresses code that does not exist in the codebase.The code snippet shown in the review references a
thumbnailGeneratorfield and error handling for ffmpeg, but the actualModelstruct andNew()function insrc/internal/ui/preview/model.gocontains onlyimagePreviewerandbatCmdfields. There is no thumbnail generator, no video preview feature, and no ffmpeg dependency in the codebase. The filepreview package handles only static image preview functionality. The review appears to be based on a different version or incorrect assumptions about the code.Likely an incorrect or invalid review comment.
122-129: The review comment references non-existent code. Lines 122-129 insrc/internal/ui/preview/model.godo not contain aCleanUp()method. The file'sModelstruct has nothumbnailGeneratorfield (onlyimagePreviewer), and theImagePreviewertype has noCleanUp()method. The code snippet provided does not match the actual codebase.Likely an incorrect or invalid review comment.
allow only supported video formats found in (ffmpeg -formats)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/internal/ui/preview/model.go (4)
45-48: Clarify log message when thumbnail generator initialization fails.The message
"Could not create TempDirectory"is misleading whenNewThumbnailGeneratorfails because ffmpeg is not installed. Consider a more generic message and keep the specific error in the structured field.- generator, err := filepreview.NewThumbnailGenerator() - if err != nil { - slog.Error("Could not create TempDirectory", "error", err) - } + generator, err := filepreview.NewThumbnailGenerator() + if err != nil { + slog.Error("Could not initialize video thumbnail generator", "error", err) + }
122-129: Align slog key and message style inCleanUp.Functionality is fine, but the log key
"Error:"is inconsistent with the rest of the file (which uses"error"), and the message can be slightly tidied up.func (m *Model) CleanUp() { if m.thumbnailGenerator != nil { - err := m.thumbnailGenerator.CleanUp() - if err != nil { - slog.Error("Error While cleaning up TempDirectory", "Error:", err) - } + if err := m.thumbnailGenerator.CleanUp(); err != nil { + slog.Error("Error while cleaning up thumbnail temp directory", "error", err) + } } }
295-303: Add logging on thumbnail generation failure and verify the intended fallback behavior.The video branch nicely mirrors the image path, but when
GetThumbnailOrGeneratefails you return an “unsupported format” message without logging. That will make diagnosing ffmpeg/path issues harder, and the user won’t know why a supported video suddenly stopped previewing.- if isVideoFile(itemPath) && m.thumbnailGenerator != nil { - thumbnailPath, err := m.thumbnailGenerator.GetThumbnailOrGenerate(itemPath) - if err != nil { - return renderUnsupportedFormat(box) + clearCmd - } - - return m.renderImagePreview(box, thumbnailPath, previewWidth, previewHeight, fullModelWidth-previewWidth+1) - } + if isVideoFile(itemPath) && m.thumbnailGenerator != nil { + thumbnailPath, err := m.thumbnailGenerator.GetThumbnailOrGenerate(itemPath) + if err != nil { + slog.Error("Error generating video thumbnail", "error", err, "path", itemPath) + return renderUnsupportedFormat(box) + clearCmd + } + + return m.renderImagePreview( + box, + thumbnailPath, + previewWidth, + previewHeight, + fullModelWidth-previewWidth+1, + ) + }Also, double‑check that the UX you want when ffmpeg is missing is indeed “unsupported format” (via the text path) rather than a more explicit “ffmpeg not installed” style message.
379-397: Video extension set looks reasonable; consider extending/centralizing it if needed.The
isVideoFilehelper is straightforward and fixes the earlier misclassification of video files as images. If you expect to support additional formats like.mpg,.ts, or.m2ts(which were mentioned in earlier iterations), you may want to add them here; otherwise they’ll fall back to the text/unsupported path. For bothisImageFileandisVideoFile, you could also hoist these maps to package-level vars to avoid rebuilding them on every call if this ever becomes hot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/internal/ui/preview/model.go(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
Applied to files:
src/internal/ui/preview/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
Applied to files:
src/internal/ui/preview/model.go
🧬 Code graph analysis (1)
src/internal/ui/preview/model.go (2)
src/pkg/file_preview/image_preview.go (3)
ImagePreviewer(45-48)NewImagePreviewer(51-53)Preview(37-42)src/pkg/file_preview/thumbnail_generator.go (2)
ThumbnailGenerator(18-22)NewThumbnailGenerator(24-40)
🔇 Additional comments (3)
src/internal/ui/preview/model.go (3)
182-184: Signature wrap change inrenderImagePreviewis a no-op.Only formatting/line wrapping changed; behavior is unchanged and consistent with
RenderWithPathcall sites.
216-218: Signature wrap change inrenderTextPreviewis a no-op.Again, just a formatting/line-wrap change; behavior remains the same.
34-41: Review comment is based on proposed code changes that do not exist in the current codebase.The review comment references a
thumbnailGeneratorfield, aCleanUp()method, video file preview functionality withisVideoFile()helper, and related changes throughout the file. However, the actual code insrc/internal/ui/preview/model.gocontains none of these changes. The Model struct only contains:open,width,height,location,content,imagePreviewer, andbatCmdfields.Verify that the correct code changes are in the pull request before proceeding with this review feedback. If these changes were intentionally reverted or deferred, this review comment should be discarded.
Likely an incorrect or invalid review comment.
1 participant
This PR aims to add Video preview support by using ffmpeg to generate a thumbnail from the video.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.