Conversation
|
|
WalkthroughAdds end-to-end video support: new backend DB/table, utils, and API routes; frontend API clients, UI components and player (Plyr), Videos page, and build config for Plyr. Integrates video processing into folder workflows and initializes the videos table at startup. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Videos Page
participant API as /videos
participant Utils as Video Utils
participant DB as Videos DB
participant FS as File System
User->>Frontend: Open Videos page
Frontend->>API: GET /videos/
API->>DB: db_get_all_videos()
DB-->>API: videos list
API-->>Frontend: GetAllVideosResponse
alt no videos or user requests scan
Frontend->>API: POST /videos/scan
API->>DB: db_get_all_folder_details()
API->>Utils: video_util_process_folder_videos(folder_data)
Utils->>FS: scan folders & read files
loop per file
Utils->>Utils: extract metadata (OpenCV)
Utils->>DB: db_insert_video(record)
end
API->>DB: db_get_all_videos()
API-->>Frontend: updated GetAllVideosResponse
end
User->>Frontend: Click VideoCard
Frontend->>Frontend: show modal & init PlyrPlayer(src)
User->>Frontend: control playback (Plyr)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
frontend/src/components/ui/button.tsx (1)
8-8: Good UX improvement with cursor-pointer, but disabled:cursor-not-allowed is redundant.The
cursor-pointeraddition improves button UX by indicating clickability. However,disabled:cursor-not-allowedhas no effect becausedisabled:pointer-events-noneis already set—when pointer events are disabled, the browser won't display custom cursor styles.If you want to keep the class string cleaner, you can remove the redundant class:
- "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all cursor-pointer disabled:pointer-events-none disabled:opacity-50 disabled:cursor-not-allowed [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive", + "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all cursor-pointer disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive",backend/app/schemas/videos.py (1)
5-13: Consider adding field validators for VideoMetadata.The VideoMetadata model lacks validation constraints. Consider adding validators to ensure data integrity (e.g., positive values for width, height, duration, and file_size).
Example validation:
from pydantic import BaseModel, Field class VideoMetadata(BaseModel): name: str date_created: Optional[str] = None width: int = Field(gt=0) height: int = Field(gt=0) duration: float = Field(gt=0) file_location: str file_size: int = Field(ge=0) item_type: strbackend/app/config/settings.py (1)
27-29: Consider using absolute paths for video storage.The relative paths
./videosand./videos/thumbnailsdepend on the current working directory. While this is consistent with the existingIMAGES_PATHpattern, consider usingpathlib.Pathwith absolute paths or environment variables for more robust path handling across different execution contexts.frontend/src/types/Media.ts (1)
40-48: Consider nullable thumbnailPath for backend consistency.The
thumbnailPathfield is typed asstring(line 43), but the backendVideoData.thumbnailPathisOptional[str]. Consider usingthumbnailPath: string | nullto handle cases where thumbnails might not be available.export interface Video { id: string; path: string; - thumbnailPath: string; + thumbnailPath: string | null; folder_id?: string; metadata?: VideoMetadata; title?: string; tags?: string[]; }backend/app/routes/folders.py (2)
71-73: Consider checking video processing return value.While
video_util_process_folder_videosreturns a boolean indicating success/failure, the return value is not checked. If video processing fails, the function continues silently. Consider logging the result or adding explicit error handling for better observability.# Process images and videos in all folders image_util_process_folder_images(folder_data) -video_util_process_folder_videos(folder_data) +video_success = video_util_process_folder_videos(folder_data) +if not video_success: + logger.warning(f"Video processing encountered errors for folder {folder_path}")
119-121: Consider checking video processing return value.Similar to the add folder sequence, the return value of
video_util_process_folder_videosis not checked here. Consider adding result validation for consistency and better error visibility.# Process images and videos in all folders image_util_process_folder_images(folder_data) -video_util_process_folder_videos(folder_data) +video_success = video_util_process_folder_videos(folder_data) +if not video_success: + logger.warning(f"Video processing encountered errors during sync for folder {folder_path}") image_util_process_untagged_images()frontend/src/components/VideoPlayer/PlyrPlayer.tsx (1)
102-102: Consider supporting multiple video formats.The video source is hardcoded to
type="video/mp4", which may not support other formats like WebM or OGV.If you need to support multiple formats, consider:
- <source src={src} type="video/mp4" /> + <source src={src} />Or detect the format from the file extension and set the appropriate MIME type.
backend/app/routes/videos.py (1)
24-33: Consider extracting duplicate VideoData mapping logic.The same VideoData construction pattern appears in all four endpoints. Extracting this into a helper function would improve maintainability.
Example:
def _map_videos_to_response(videos: List[dict]) -> List[VideoData]: """Helper to map database video dicts to VideoData schema.""" return [ VideoData( id=v["id"], path=v["path"], folder_id=v.get("folder_id"), thumbnailPath=v["thumbnailPath"], metadata=image_util_parse_metadata(v.get("metadata")), ) for v in videos ] # Then use in endpoints: data = _map_videos_to_response(videos)Also applies to: 57-66, 100-109, 133-142
backend/app/utils/videos.py (3)
55-81: Consider refactoring the import and directory check.Two minor inefficiencies:
Import inside function (line 59): The local import of
db_get_video_by_pathsuggests a circular dependency issue. Consider restructuring imports or using TYPE_CHECKING to resolve this more cleanly.Redundant directory check (line 61):
video_util_ensure_dirs()is called for every file registration. While harmless (due toexist_ok=True), it's unnecessary overhead. Consider calling it once during application startup or at the beginning of batch operations likevideo_util_process_folder_videos.For the import issue, consider moving it to the top with TYPE_CHECKING:
from typing import TYPE_CHECKING if TYPE_CHECKING: from app.database.videos import db_get_video_by_pathOr restructure the module dependencies to eliminate the circular import.
90-108: Add consistent error handling for recursive mode.The non-recursive mode catches
OSError(line 106), but the recursive mode usingos.walk(lines 94-99) doesn't. Theos.walkfunction can raiseOSErrorfor permission issues or other filesystem errors when traversing subdirectories, leading to inconsistent behavior.Apply this diff to add consistent error handling:
def video_util_get_videos_from_folder( folder_path: str, recursive: bool = True ) -> List[str]: videos: List[str] = [] if recursive: - for root, _, files in os.walk(folder_path): - for f in files: - p = os.path.join(root, f) - if video_util_is_valid_video(p): - videos.append(p) + try: + for root, _, files in os.walk(folder_path): + for f in files: + p = os.path.join(root, f) + if video_util_is_valid_video(p): + videos.append(p) + except OSError: + pass else: try: for f in os.listdir(folder_path): p = os.path.join(folder_path, f) if os.path.isfile(p) and video_util_is_valid_video(p): videos.append(p) except OSError: pass return videos
114-126: Add logging for top-level exceptions.The top-level exception handler (lines 125-126) silently returns
Falsewithout logging the error. This makes it difficult to diagnose why folder processing failed, especially since the function processes multiple folders and could fail for various reasons (filesystem issues, database errors, etc.).Apply this diff to add error logging:
def video_util_process_folder_videos(folder_data: List[essential_tuple]) -> bool: try: video_util_ensure_dirs() for path, folder_id, recursive in folder_data: file_list = video_util_get_videos_from_folder(path, recursive) for fp in file_list: try: video_util_register_file(fp, folder_id) except Exception: continue return True - except Exception: + except Exception as e: + logger.error(f"Failed to process folder videos: {e}") return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
backend/app/config/settings.py(1 hunks)backend/app/database/videos.py(1 hunks)backend/app/routes/folders.py(3 hunks)backend/app/routes/videos.py(1 hunks)backend/app/schemas/videos.py(1 hunks)backend/app/utils/videos.py(1 hunks)backend/main.py(3 hunks)frontend/package.json(1 hunks)frontend/src/api/api-functions/index.ts(1 hunks)frontend/src/api/api-functions/videos.ts(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Media/VideoCard.tsx(1 hunks)frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx(0 hunks)frontend/src/components/VideoPlayer/PlyrPlayer.tsx(1 hunks)frontend/src/components/ui/button.tsx(1 hunks)frontend/src/pages/VideosPage/Videos.tsx(1 hunks)frontend/src/routes/AppRoutes.tsx(1 hunks)frontend/src/types/Media.ts(2 hunks)frontend/vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/src/api/api-functions/videos.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
videosEndpoints(5-9)
frontend/src/types/Media.ts (1)
backend/app/schemas/videos.py (1)
VideoMetadata(5-13)
frontend/src/routes/AppRoutes.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-11)
frontend/src/pages/VideosPage/Videos.tsx (6)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/api-functions/videos.ts (2)
fetchAllVideos(5-10)scanVideos(25-28)frontend/src/types/Media.ts (1)
Video(40-48)frontend/src/components/ui/LoadingScreen/LoadingScreen.tsx (1)
LoadingScreen(15-64)frontend/src/components/Media/VideoCard.tsx (1)
VideoCard(13-52)frontend/src/components/VideoPlayer/PlyrPlayer.tsx (1)
PlyrPlayer(10-106)
backend/app/schemas/videos.py (1)
frontend/src/types/Media.ts (1)
VideoMetadata(13-22)
backend/app/utils/videos.py (1)
backend/app/database/videos.py (2)
db_insert_video(52-75)db_get_video_by_path(128-156)
backend/app/routes/videos.py (5)
backend/app/database/videos.py (1)
db_get_all_videos(78-125)backend/app/schemas/videos.py (3)
GetAllVideosResponse(24-27)ErrorResponse(30-33)VideoData(16-21)backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)backend/app/utils/videos.py (3)
video_util_register_file(55-81)video_util_ensure_dirs(15-16)video_util_process_folder_videos(114-126)backend/app/database/folders.py (1)
db_get_all_folder_details(397-418)
frontend/src/components/Media/VideoCard.tsx (1)
frontend/src/types/Media.ts (1)
Video(40-48)
backend/app/routes/folders.py (2)
backend/app/utils/videos.py (1)
video_util_process_folder_videos(114-126)backend/app/utils/images.py (1)
image_util_process_folder_images(32-84)
backend/main.py (1)
backend/app/database/videos.py (1)
db_create_videos_table(31-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: sync-labels
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
🔇 Additional comments (18)
frontend/src/routes/AppRoutes.tsx (1)
11-11: LGTM!The Videos component import and route registration follow the existing patterns and integrate cleanly with the route structure.
Also applies to: 19-19
frontend/vite.config.ts (1)
17-19: LGTM!Pre-bundling plyr in
optimizeDepsis the recommended approach for improving dev server startup time and handling potential ESM compatibility issues with the library.frontend/src/api/api-functions/index.ts (1)
4-4: LGTM!The videos module export follows the established pattern for API function organization.
frontend/src/types/Media.ts (1)
13-22: LGTM!The VideoMetadata interface correctly mirrors the backend schema fields and types, ensuring consistent data shape across frontend and backend.
backend/app/routes/folders.py (1)
45-45: LGTM!The import of video processing utilities integrates cleanly with the existing folder processing infrastructure.
frontend/package.json (1)
56-56: plyr 3.8.3 is current and secure.Version 3.8.3 is the latest release with no known security vulnerabilities or CVEs. The package was released August 27, 2025, indicating active maintenance. No action needed.
backend/app/schemas/videos.py (1)
21-21: The code is correct for the project's Python version requirement.The union syntax
Mapping[str, Any] | VideoMetadatais appropriate here. The project's GitHub Actions workflows explicitly specify Python 3.12, which fully supports PEP 604 union syntax (available since Python 3.10). No changes are needed.frontend/src/api/apiEndpoints.ts (1)
5-9: LGTM!The new
videosEndpointsobject follows the established pattern and provides clear endpoint definitions for video operations.backend/main.py (1)
22-22: LGTM!The video module integration follows the established pattern for other entities in the application. The table creation is correctly placed in the startup sequence, and the router is properly mounted with appropriate tags.
Also applies to: 28-28, 58-58, 132-132
frontend/src/api/api-functions/videos.ts (1)
5-28: LGTM!The API functions follow a clean pattern and leverage the existing
apiClientconfiguration. Error handling appears to be delegated to the caller (React Query), which is a reasonable approach for this architecture.frontend/src/pages/VideosPage/Videos.tsx (1)
68-141: LGTM!The UI implementation is well-structured with responsive grid layout, sorting controls, and a modal player. The component provides a good user experience with loading states and empty states.
frontend/src/components/Media/VideoCard.tsx (1)
13-52: Well-structured video card component.The component provides good UX with hover effects, play button overlay, and responsive design. The video metadata fallback chain is robust.
frontend/src/components/VideoPlayer/PlyrPlayer.tsx (1)
10-106: Excellent player implementation with proper cleanup.The component handles Plyr initialization, configuration, and cleanup correctly. The dynamic import approach and effect dependencies are appropriate.
backend/app/database/videos.py (4)
31-49: LGTM!The table schema is well-designed with appropriate constraints. The foreign key relationship with
ON DELETE SET NULLensures data integrity when folders are removed.
52-76: LGTM!The upsert logic using
ON CONFLICTis appropriate for handling duplicate paths, and error handling with rollback is properly implemented.
78-126: Excellent auto-cleanup implementation.The automatic cleanup of database entries for deleted files is a smart approach that keeps the database in sync with the filesystem. The implementation is safe and well-logged.
128-171: LGTM!Both query and delete functions are well-implemented with proper error handling and connection management.
backend/app/utils/videos.py (1)
15-16: Now let me search for broader "thumbnail" references in the codebase:Remove
THUMBNAIL_VIDEOS_PATHfrom settings or implement directory creation.The constant is defined in
backend/app/config/settings.py(line 29) but never used anywhere in the codebase. Since the PR optimizes by avoiding unnecessary thumbnails (line 77 setsthumbnailPathtoNone), either remove this unused constant from settings to prevent confusion, or addos.makedirs(THUMBNAIL_VIDEOS_PATH, exist_ok=True)tovideo_util_ensure_dirs()if thumbnail functionality is planned for the future.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/app/routes/videos.py (4)
9-9: Consider adding tags to the router for better API documentation.Adding tags helps organize endpoints in the automatically generated OpenAPI/Swagger documentation.
Apply this diff:
-router = APIRouter() +router = APIRouter(tags=["videos"])
12-39: Consider pagination for scalability.The endpoint returns all videos without pagination, which could cause performance issues and large response sizes with extensive video libraries.
Consider adding pagination parameters:
@router.get( "/", response_model=GetAllVideosResponse, responses={500: {"model": ErrorResponse}}, ) def get_all_videos(skip: int = 0, limit: int = 100): try: videos = db_get_all_videos() # Apply pagination paginated_videos = videos[skip:skip + limit] data: List[VideoData] = [ VideoData( id=v["id"], path=v["path"], folder_id=v.get("folder_id"), thumbnailPath=v["thumbnailPath"], metadata=image_util_parse_metadata(v.get("metadata")), ) for v in paginated_videos ] return GetAllVideosResponse( success=True, message=f"Retrieved {len(data)} videos (showing {skip}-{skip+len(data)} of {len(videos)} total)", data=data ) except Exception as e: raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( success=False, error="Internal server error", message=str(e) ).model_dump(), )
47-47: Removeasynckeyword or use asynchronous operations.The function is declared as
asyncbut doesn't use anyawaitexpressions. This creates unnecessary overhead and inconsistency withget_all_videos, which is synchronous.Apply this diff:
-async def scan_videos_from_folders(): +def scan_videos_from_folders():
80-80: Removeasynckeyword or use asynchronous operations.Similar to
scan_videos_from_folders, this function is declared asasyncbut doesn't use anyawaitexpressions, creating inconsistency withget_all_videos.Apply this diff:
-async def cleanup_deleted_videos(): +def cleanup_deleted_videos():
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/routes/videos.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/videos.py (5)
backend/app/database/videos.py (1)
db_get_all_videos(78-125)backend/app/schemas/videos.py (3)
GetAllVideosResponse(24-27)ErrorResponse(30-33)VideoData(16-21)backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)backend/app/utils/videos.py (1)
video_util_process_folder_videos(114-126)backend/app/database/folders.py (1)
db_get_all_folder_details(397-418)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (3)
backend/app/routes/videos.py (3)
80-81: LGTM! The cleanup mechanism works via implicit behavior.The endpoint correctly leverages the automatic cleanup behavior in
db_get_all_videos, which removes database entries for videos whose files no longer exist (as shown in the relevant code snippet frombackend/app/database/videos.py). The docstring clearly explains the purpose.
33-39: LGTM! Error handling is consistent and appropriate.The error handling pattern correctly uses
ErrorResponse.model_dump()to convert the Pydantic model to a dictionary for the HTTPException detail field. The consistent structure across all endpoints makes the API predictable.Also applies to: 66-72, 99-105
50-50: Review comment is incorrect—AI_Tagging and recursive scanning are unrelated features.The hardcoded
recursive=Falseis intentional system-wide behavior (replicated inbackend/app/routes/folders.py:68with an explicit comment). The database schema has no recursive field; it's not configurable per folder.AI_Tagging is a separate per-folder feature flag for AI-based image tagging (queried in
backend/app/database/images.py), not a control for scan recursion. It does not indicate or require recursive folder scanning.Likely an incorrect or invalid review comment.
|
@SiddharthJiyani Please resolve conflicts. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/routes/AppRoutes.tsx(1 hunks)frontend/src/types/Media.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/api/apiEndpoints.ts
- frontend/src/routes/AppRoutes.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/types/Media.ts (1)
backend/app/schemas/videos.py (1)
VideoMetadata(5-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (2)
frontend/src/types/Media.ts (2)
13-22: LGTM! VideoMetadata interface aligns with backend schema.The VideoMetadata interface correctly mirrors the backend schema with appropriate TypeScript types and includes the essential
durationfield for video playback.
24-34: The review comment is incorrect.The git diff shows that
folder_id: stringwas already required in the codebase before this PR and was not modified. The line appears as unchanged context in the diff, not as a new addition or modification. The actual changes in this PR are:
- Added
VideoMetadatainterface- Added
isFavourite?: booleanto theImageinterface- Added
Videointerface with optionalfolder_id?: string- Added
images: Image[]property toMediaViewPropsThere is no breaking change to
Image.folder_idin this PR.Likely an incorrect or invalid review comment.
Done |
2 participants
Feat: Implement Videos Page with Dynamic Thumbnails
Overview
Adds full video management functionality with an optimized approach that eliminates unnecessary thumbnail storage and provides automatic database cleanup.
Key Changes
Backend
Frontend
Technical Details
Testing
Demo
video_page_implementation_.1.mp4
Summary by CodeRabbit
New Features
Removed