Remove Fluid Player integration from post media #83
Conversation
Eliminated all Fluid Player-related code from PostMedia.vue, including lazy-loading of its CSS, player creation, and ad logic. Removed the fluid-player dependency from package.json and its exclusion from Sentry error reporting. Updated the content security policy in nuxt.config.js to retain 'unsafe-eval' for ads, but removed the specific Fluid Player comment.
Introduces a new 'autoplayVideos' user setting, UI toggle, and backup support. Implements feed-based video autoplay logic so only the most central visible video plays automatically, pausing others. Updates PostMedia.vue to use the new composable and setting, and adjusts preload behavior for videos.
Replaces manual muting in JavaScript with a Vue binding on the 'muted' attribute, ensuring videos are muted until user interaction. This simplifies the logic and leverages Vue's reactivity.
Updated the 'muted' prop on the video element to only set it when videos are autoplaying, preventing unintended muting when autoplay is disabled.
Added support to restore the autoplayVideos user setting when restoring from a v3 backup. This ensures all relevant user preferences are correctly restored.
Replaces module-level variables with useState for activeVideoId, visibleElements, and listening, improving reactivity and SSR compatibility. Refactors related logic to use the new state structure and cleans up global variable usage.
WalkthroughReplaces Fluid Player with native HTML5 video playback, adds an Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Component as PostMedia.vue
participant Observer as IntersectionObserver
participant Autoplay as useFeedAutoplay
participant Viewport as Scroll/Resize Events
User->>Component: Component mounts, video element created
Component->>Observer: Register element with IntersectionObserver
Observer->>Component: Notifies enter/exit (isIntersecting)
Component->>Autoplay: updateCandidate(componentId, element, isIntersecting)
Autoplay->>Autoplay: Add/remove element in visibleElements
Autoplay->>Autoplay: toggleListeners (attach/detach scroll/resize)
Viewport->>Autoplay: scroll/resize events (debounced/throttled)
Autoplay->>Autoplay: calculateBestVideo() -> sets activeVideoId
Autoplay->>Component: Reactive activeVideoId change observed
Component->>Component: Pause other videos, play candidate (muted if no interaction)
Note over Component,User: User interaction sets hasInteracted -> enables unmute/play controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
assets/js/BackupHelper.ts(4 hunks)components/pages/posts/post/PostMedia.vue(6 hunks)composables/useFeedAutoplay.ts(1 hunks)composables/useUserSettings.ts(2 hunks)nuxt.config.js(1 hunks)package.json(0 hunks)pages/settings.vue(2 hunks)sentry.client.config.ts(0 hunks)
💤 Files with no reviewable changes (2)
- sentry.client.config.ts
- package.json
🔇 Additional comments (12)
components/pages/posts/post/PostMedia.vue (4)
201-228: Autoplay watch logic looks correct with proper error handling.The watch correctly handles play/pause based on
activeVideoId, and theAbortErrorcatch is appropriate since it occurs when scrolling fast causesplay()to be interrupted by a subsequentpause().
68-70: Cleanup inonBeforeUnmountproperly deregisters the video candidate.Passing
nullfor the element andfalsefor visibility ensures the component is removed from the visible elements map when unmounted.
93-100: SimplifiedreloadVideoPlayerfunction.The function now correctly triggers play after
nextTickwhenshouldPlayis true. The optional chaining onvideo?.play()safely handles the case wheremediaElementmight not be set.
439-443: Video autoplay attributes are correctly configured.The
preload="metadata"when autoplay is enabled allows fetching video metadata for smoother playback. The muted attribute bound to!hasInteractedcorrectly satisfies browser autoplay policies: videos autoplay muted until user interaction (detected via mousedown/touchstart events), then autoplay with sound is permitted. ThehasInteractedstate is properly tracked across the application using Nuxt'suseState, and event listeners efficiently unregister after the first interaction.composables/useFeedAutoplay.ts (3)
10-33: Center-proximity calculation is well-implemented.The algorithm correctly calculates which video is closest to the viewport center using
getBoundingClientRect(). The early return whenvisibleElementsis empty prevents unnecessary computation.
53-66: Consider guarding against SSR intoggleListenerscall.While
updateCandidatechecksimport.meta.client, thetoggleListenersfunction also has its owntypeof window === 'undefined'check. This is redundant but safe. The logic correctly manages the visible elements map.
6-8: UsinguseStatefor shared reactive state is correct for Nuxt.This ensures the state is properly scoped and SSR-safe. The Map is correctly initialized in a factory function.
assets/js/BackupHelper.ts (2)
30-35: Backup serialization correctly includes the new setting.The
autoplayVideossetting is properly added to the backup payload alongside existing settings.
68-71: Restoration logic correctly handles the new setting with null check.The pattern matches existing settings restoration, ensuring backward compatibility with backups that don't include
autoplayVideos.pages/settings.vue (1)
122-129: New settings toggle follows existing UI patterns.The
autoplayVideosswitch is properly implemented with clear name and description. Good placement before the GIF autoplay setting for logical grouping of media autoplay options.composables/useUserSettings.ts (2)
7-7: Defaultfalsefor autoplay is the right choice.Opt-in autoplay respects user bandwidth and avoids unexpected video playback, aligning with good UX practices.
19-21: LocalStorage persistence follows established pattern.The
writeDefaults: falseoption correctly prevents writing the default value to localStorage, keeping storage clean until the user explicitly changes the setting.
Replaces Sentry DSN with applicationKey in project config and updates all references. Integrates Sentry's thirdPartyErrorFilterIntegration for improved error filtering, removing custom beforeSend logic and extensive ignore lists. Updates Nuxt config to use the new applicationKey for Sentry bundler plugin.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nuxt.config.js (1)
433-441: Remove'unsafe-eval'from CSP—original justification no longer applies.The
'unsafe-eval'directive significantly weakens CSP by allowingeval()and similar dynamic code execution. As noted in the previous review:
- The original justification ("fluid player fullscreen eval") is obsolete since Fluid Player is being removed in this PR
- No
eval()usage was found in the codebase- No ads-related code was found to support the "Fix: ads need this" comment
- Best practices recommend avoiding
'unsafe-eval'Remove this directive to strengthen the security posture. If ads genuinely require it, provide specific evidence (vendor documentation, test results) and add a narrowly-scoped exception instead.
Apply this diff to remove the insecure directive:
'script-src': [ "'self'", 'https:', "'unsafe-inline'", "'strict-dynamic'", "'nonce-{{nonce}}'", - // Fix: ads need this - "'unsafe-eval'" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
composables/useFeedAutoplay.ts(1 hunks)config/project.ts(2 hunks)nuxt.config.js(2 hunks)sentry.client.config.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
composables/useFeedAutoplay.ts (1)
composables/useUserSettings.ts (1)
postFullSizeImages(3-25)
sentry.client.config.ts (1)
config/project.ts (2)
Sentry(25-27)project(66-102)
nuxt.config.js (1)
config/project.ts (1)
project(66-102)
🔇 Additional comments (8)
config/project.ts (2)
25-27: LGTM!The interface refactoring from
dsntoapplicationKeyis clear and consistent with the usage insentry.client.config.tsandnuxt.config.js.
88-90: LGTM!The Sentry configuration is correctly populated with the
applicationKeyvalue that aligns with the interface change and is properly consumed by the thirdPartyErrorFilterIntegration.sentry.client.config.ts (2)
3-3: LGTM!The import is necessary for accessing the
applicationKeyin the thirdPartyErrorFilterIntegration below.
20-24: thirdPartyErrorFilterIntegration is correctly implemented.The
thirdPartyErrorFilterIntegrationallows you to filter out errors originating from third parties, and the configuration usingfilterKeyswithproject.sentry.applicationKeyandbehaviour: 'drop-error-if-contains-third-party-frames'follows the correct API. The integration and parameter names are valid.nuxt.config.js (1)
286-288:unstable_sentryBundlerPluginOptionswithapplicationKeyis properly documented.This option is documented for marking JavaScript files with an application key and is used with Sentry's third-party error filtering integration. However, be aware that the @sentry/nuxt SDK itself is marked as EXPERIMENTAL, so API changes may occur in future releases.
composables/useFeedAutoplay.ts (3)
10-35: LGTM!The center-proximity calculation logic is sound. The function correctly identifies the video element closest to the viewport center and updates the active video only when necessary. The 200ms throttle interval is appropriate for scroll and resize handlers.
37-51: LGTM! Previous concern addressed.The event listener management is correct. The past review comment about passing
optionstoremoveEventListenerhas been properly addressed—bothaddEventListenerandremoveEventListenernow receive the same options object.
4-4: No action required:autoplayVideosis properly defined and exported inuseUserSettings.The
autoplayVideossetting exists incomposables/useUserSettings.ts(line 7), is persisted to localStorage (line 19), and is correctly returned from the composable (line 28). The implementation inuseFeedAutoplay.tsproperly destructures and uses this setting.Likely an incorrect or invalid review comment.
| const updateCandidate = (id: number, element: HTMLElement | null, isVisible: boolean) => { | ||
| if (!import.meta.client || !autoplayVideos.value) { | ||
| return | ||
| } | ||
|
|
||
| if (isVisible && element) { | ||
| visibleElements.value.set(id, element) | ||
| } else { | ||
| visibleElements.value.delete(id) | ||
| } |
There was a problem hiding this comment.
Consider clearing state when autoplay is disabled.
When autoplayVideos is false, the function returns early (line 54) but leaves visibleElements and activeVideoId unchanged. This means:
- The Map retains entries from when autoplay was enabled
activeVideoIdmay reference a stale video- Event listeners remain attached if they were previously enabled
While this might be intentional to preserve state for quick re-enabling, it could lead to confusion or memory accumulation over time.
Consider adding cleanup when autoplay is disabled:
const updateCandidate = (id: number, element: HTMLElement | null, isVisible: boolean) => {
if (!import.meta.client || !autoplayVideos.value) {
+ if (visibleElements.value.size > 0) {
+ visibleElements.value.clear()
+ activeVideoId.value = null
+ toggleListeners(false)
+ }
return
}
if (isVisible && element) {
visibleElements.value.set(id, element)
} else {
visibleElements.value.delete(id)
}
toggleListeners(visibleElements.value.size > 0)
throttledCheck()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateCandidate = (id: number, element: HTMLElement | null, isVisible: boolean) => { | |
| if (!import.meta.client || !autoplayVideos.value) { | |
| return | |
| } | |
| if (isVisible && element) { | |
| visibleElements.value.set(id, element) | |
| } else { | |
| visibleElements.value.delete(id) | |
| } | |
| const updateCandidate = (id: number, element: HTMLElement | null, isVisible: boolean) => { | |
| if (!import.meta.client || !autoplayVideos.value) { | |
| if (visibleElements.value.size > 0) { | |
| visibleElements.value.clear() | |
| activeVideoId.value = null | |
| toggleListeners(false) | |
| } | |
| return | |
| } | |
| if (isVisible && element) { | |
| visibleElements.value.set(id, element) | |
| } else { | |
| visibleElements.value.delete(id) | |
| } |
🤖 Prompt for AI Agents
In composables/useFeedAutoplay.ts around lines 53 to 62, update the early-return
path when autoplayVideos is false to perform cleanup: clear visibleElements
(visibleElements.value.clear()), reset activeVideoId to null (or undefined per
existing type), and detach any autoplay-related event listeners or timers (or
call the existing cleanup/unregister function) so no stale references or
listeners remain when autoplay is disabled; keep the existing guard for
import.meta.client but ensure cleanup runs before returning.
| } | ||
|
|
||
| toggleListeners(visibleElements.value.size > 0) | ||
| throttledCheck() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing redundant throttledCheck() call.
Calling throttledCheck() immediately after every updateCandidate invocation may be redundant because:
- The throttle mechanism already handles rapid successive calls
- Scroll and resize listeners will trigger checks when the user scrolls
- Each visibility change triggers this, potentially causing unnecessary recalculations
The immediate call is only valuable for the first visibility change or when elements are removed. Consider making the immediate call conditional.
toggleListeners(visibleElements.value.size > 0)
- throttledCheck()
+ if (visibleElements.value.size === 0) {
+ // Clear immediately when last element removed
+ calculateBestVideo()
+ } else {
+ // Let throttled mechanism handle recalculation
+ throttledCheck()
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In composables/useFeedAutoplay.ts around line 65, the direct call to
throttledCheck() after every updateCandidate is redundant; change it to call
throttledCheck() immediately only when needed by: add an "initialized" boolean
(or similar) to track whether the first visibility check has run and call
throttledCheck() immediately if not initialized (then set initialized=true), and
detect element removals by comparing previous candidates length (or a removal
flag returned by updateCandidate) to the new candidates length and call
throttledCheck() immediately only when length decreased; otherwise, rely on the
existing throttled calls and event listeners.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.