Conversation
WalkthroughAdds per-video selection and bulk-action support across the videos UI: new selection checkbox styling and props on VideoCard, selection state and bulk actions (menus, modals, concurrent mutations) in VideoGrid, conditional cache-invalidation flags in playback/videos hooks, and new i18n strings for bulk UI in en/de/uk. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/messages/en.json (1)
406-412:⚠️ Potential issue | 🟡 MinorTypo: "Mark was unwatched" → "Mark as unwatched".
Line 408 has a typo in
AdminVideosPage.bulkActionMenu.markAsUnwatched.📝 Proposed fix
"markAsWatched": "Mark as watched", - "markAsUnwatched": "Mark was unwatched", + "markAsUnwatched": "Mark as unwatched", "lock": "Lock videos",
🤖 Fix all issues with AI agents
In `@frontend/app/components/videos/Grid.tsx`:
- Around line 325-389: The bulk action Menu allows triggering new actions while
bulkActionLoading is true because individual Menu.Item handlers (e.g.,
handleMarkVideosAsWatched, handleMarkVideosAsUnwatched, handleLockVideos,
handleGenerateStaticThumbnails, handleGenerateSpriteThumbnails,
openPlaylistModal, openMultiDeleteModal) are still clickable once the menu is
open; fix this by disabling each Menu.Item when bulkActionLoading is true (add
disabled={bulkActionLoading} to the Menu.Item elements) and optionally close the
menu when a bulk action starts by controlling the Menu open state (introduce a
menuOpened state and call setMenuOpened(false) at the start of each handler),
ensuring all handlers are guarded by bulkActionLoading checks.
In `@frontend/messages/de.json`:
- Line 769: The German string uses the wrong verb form: update the localization
key "unlocked" (used by "videosLockedNotification") from the infinitive
"entsperren" to the correct past participle "entsperrt" so the sentence reads
"Videos wurden entsperrt."; apply the same replacement for the "unlocked" entry
referenced in VideoComponents to keep both translations consistent.
🧹 Nitpick comments (5)
frontend/app/hooks/useVideos.ts (1)
488-515: Broadened cache invalidation scope looks correct.The expanded invalidation (videos, channel_videos, playlist_videos, search) ensures all views stay consistent after lock/unlock, and
Promise.allis appropriate here.Minor style inconsistency: this uses early-return (
=== false → return) whileusePlayback.tsuses the inverse guard (!== false → invalidate). Both work identically, but unifying the style would improve readability.frontend/app/components/videos/Grid.tsx (4)
179-204: Unbounded concurrency in bulk operations could overwhelm the backend.
Promise.allfires every mutation simultaneously. If a user selects 50–100+ videos, this sends that many parallel requests at once, which could trigger rate limits or overload the server.Consider batching with a concurrency limit (e.g., using a simple chunked approach or a library like
p-limit).Also, partial failures are silently swallowed—some mutations may succeed before one throws, but only the error is surfaced. Consider tracking and reporting partial success/failure counts.
Example: simple chunked approach
const runBulkOperation = async ( operation: (video: T) => Promise<unknown>, successMessage: string, onSuccess?: () => Promise<void> | void ) => { if (selectedVideoList.length === 0) return; + const BATCH_SIZE = 5; try { setBulkActionLoading(true); - await Promise.all(selectedVideoList.map((video) => operation(video))); + for (let i = 0; i < selectedVideoList.length; i += BATCH_SIZE) { + const batch = selectedVideoList.slice(i, i + BATCH_SIZE); + await Promise.all(batch.map((video) => operation(video))); + } if (onSuccess) { await onSuccess(); }
67-75: Selection state persists across page navigations — could surprise users.
selectedVideosis never cleared when the user changes pages (viaonPageChange). This means videos selected on page 1 remain selected when navigating to page 2. While cross-page selection can be useful, the "Select all on page" checkbox and count label may confuse users who don't realize they have off-screen selections.Consider either: (a) clearing selection on page change, or (b) making the cross-page behavior explicit in the UI (e.g., showing "X selected across pages").
279-282: Inconsistent selection clearing between modals.
handleCloseMultiDeleteModalclearsselectedVideos(Line 281), but closing the playlist modal (closePlaylistModal, Line 457) does not. After a successful bulk playlist add, stale selections persist. Consider clearing selection after playlist add as well for consistency, or make it intentional so users can chain multiple bulk actions on the same selection.Also applies to: 455-473
37-53: The component is accumulating significant bulk-action logic — consider extracting a custom hook.The selection state, bulk operation runner, and individual action handlers add ~150 lines of logic to this component. Extracting a
useBulkVideoActions(videos)hook would improve readability and testability of both the grid layout and the bulk action logic.Also applies to: 55-81, 153-277
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/components/videos/Grid.tsx`:
- Around line 180-205: The runBulkOperation function currently uses Promise.all
which rejects fast and allows setBulkActionLoading(false) to run while other
mutations are still in-flight; change to Promise.allSettled over
selectedVideoList.map(video => operation(video)) so you wait for every mutation
to complete, then count results to build a success/failure summary (e.g.,
successes vs failures) and call showNotification with that summary; ensure
onSuccess is awaited/executed only after allSettled completes, still handle and
log any unexpected errors, and keep setBulkActionLoading(false) in finally so
the loading guard only clears after all operations have settled.
🧹 Nitpick comments (1)
frontend/app/components/videos/Grid.tsx (1)
186-188: No concurrency limit on bulk mutations.
Promise.all(selectedVideoList.map(…))fires every mutation simultaneously. If a user selects videos across many pages, this could send hundreds of concurrent requests. Consider batching (e.g., chunks of 5–10) or using a concurrency-limited helper likep-limitto avoid overwhelming the backend.
Add bulk video actions on the Video Grid component.