feat: replace Fluid Player with Video.js IMA#149
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (8)test/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{vue,ts,tsx,js,jsx,json,css,scss}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,vue}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
app/**/*.{vue,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
app/components/**/*.vue📄 CodeRabbit inference engine (AGENTS.md)
Files:
app/**/*.vue📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-06-22T07:50:54.935ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughChangesThe PR replaces fluid-player with Video.js and Google IMA, adds reusable video ad policy helpers and tests, updates player lifecycle and pause handling, introduces IMA typings and dependencies, tightens CSP configuration, and changes Sentry URL filtering. Video.js IMA ad integration
Sequence Diagram(s)sequenceDiagram
participant Viewer
participant PostMedia
participant VideoJS
participant GoogleIMA
Viewer->>PostMedia: Focus or pointer interaction
PostMedia->>GoogleIMA: Initialize ad display container
PostMedia->>VideoJS: Create player and configure IMA
VideoJS->>GoogleIMA: Load configured ad tag
GoogleIMA-->>PostMedia: Emit ad lifecycle events
PostMedia->>GoogleIMA: Request eligible pause ad
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 1
🤖 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 `@app/components/pages/posts/post/PostMedia.vue`:
- Around line 368-371: Update onVideoUserInteraction so initializeVideoPlayer
runs before attempting IMA setup, then initialize the ad display container on
the created player within the same user-interaction flow. Ensure the container
initialization is not skipped when videoPlayer is initially undefined, while
preserving optional handling for unavailable IMA APIs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54ef8795-a312-4921-877a-8d002701ddea
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
app/assets/js/video-ad-policy.tsapp/components/pages/posts/post/PostMedia.vueapp/types/videojs-ima.d.tsnuxt.config.tspackage.jsonsentry.client.options.tstest/assets/video-ad-policy.test.ts
💤 Files with no reviewable changes (1)
- sentry.client.options.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
app/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
app/**/*.{vue,ts,tsx}: Use Nuxt 4 with Vue 3 and TypeScript for the frontend application
Use useLazyToast() to lazy-load vue-sonner; wait for ClientToaster to mount before calling toast.* using a proper async wait instead of plain nextTick()
Build reordered array synchronously instead of reading VueUse moveArrayElement() result before nextTick for state immediately persisted
Files:
app/assets/js/video-ad-policy.tsapp/types/videojs-ima.d.tsapp/components/pages/posts/post/PostMedia.vue
**/*.{vue,ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier for formatting with 120-char print width, no semicolons, single quotes, trailing commas removed, and single attribute per line in Vue templates
Files:
app/assets/js/video-ad-policy.tsapp/types/videojs-ima.d.tstest/assets/video-ad-policy.test.tsnuxt.config.tsapp/components/pages/posts/post/PostMedia.vuepackage.json
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use Nuxt flat ESLint via
@nuxt/eslintfor linting
Files:
app/assets/js/video-ad-policy.tsapp/types/videojs-ima.d.tstest/assets/video-ad-policy.test.tsnuxt.config.tsapp/components/pages/posts/post/PostMedia.vue
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Define locales in config/i18n.ts as the single source of truth; do not hardcode locale lists in application code
Use PocketBase batch writes for multi-record replacement/reorder operations instead of one HTTP write per changed row
Files:
app/assets/js/video-ad-policy.tsapp/types/videojs-ima.d.tstest/assets/video-ad-policy.test.tsnuxt.config.tsapp/components/pages/posts/post/PostMedia.vue
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: For URL validation/parsing, prefer URL.canParse() or URL.parse() over constructor try/catch
Use URL.parse() when the parsed URL object is needed, with URL.canParse() fallback in browser code if compatibility matters
Files:
app/assets/js/video-ad-policy.tsapp/types/videojs-ima.d.tstest/assets/video-ad-policy.test.tsnuxt.config.ts
test/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.{test,spec}.{ts,tsx}: Use Vitest with@nuxt/test-utilsand Playwright browser mode for testing
Use@nuxt/test-utilswith Playwright inside describe blocks calling await setup({ browser: true })
Mock server-side API calls via test-only Nitro plugin at test/server-mocks/plugin.ts injected through nuxt.config.ts $test.nitro.plugins
Use debugBrowserOptions from test/helper.ts for headful Playwright playback with slowMo in tests
Keep repository/pure modules importable through relative paths in plain Vitest suites that do not get Nuxt's runtime alias resolution
Use fetch from@nuxt/test-utilswith { redirect: 'manual' } to access redirect status and Location headers;@nuxt/test-utils$fetch has no .raw method
Import localeCodes, prefixedLocaleCodes, and removedLocaleCodes from config/i18n in tests instead of hardcoding locale lists
Files:
test/assets/video-ad-policy.test.ts
nuxt.config.ts
📄 CodeRabbit inference engine (AGENTS.md)
nuxt.config.ts: In test mode, set $test.runtimeConfig.public.apiUrl to '' so $fetch(baseURL: '') routes to local Nitro test server
Fully disable Sentry in tests via $test.sentry.enabled: false in nuxt.config.ts
Files:
nuxt.config.ts
app/components/**/*.vue
📄 CodeRabbit inference engine (AGENTS.md)
app/components/**/*.vue: Register Vue components without path prefix using pathPrefix: false in nuxt.config.ts; import them as not <Input/DomainSelector>
Auto-import components from app/components/ flat structure without folder prefix
Use custom imgproxy provider for images; generate images at 1x density only in webp format to reduce bandwidth
Use@nuxt/imagev2 preload API (preload: { fetchPriority: 'high' }) for image preload priority instead of patching rendered HTML in Nitro
Use Nuxt Lazy* components, dynamic imports, or similarly deferred boundaries for components and composables only needed after user interaction (menu, sheet, dialog)
Files:
app/components/pages/posts/post/PostMedia.vue
app/**/*.vue
📄 CodeRabbit inference engine (AGENTS.md)
app/**/*.vue: Use project.urls.production for canonical URLs to point to production (https://r34.app/...) even when served from clone domains
Keep breadcrumb Schema.org source item URLs local/locale-relative; do not convert to project.urls.production since nuxt-schema-org resolves against site.url
When triggering a premium prompt from a nested dialog/bottom sheet, let the sheet owner close local UI, open the prompt, and restore UI after prompt closes
Premium auth transitions are reload-backed in dashboard/sign-in flow; rely on page reload to clear memory state instead of tracking PocketBase user ids in useState
Files:
app/components/pages/posts/post/PostMedia.vue
🧠 Learnings (1)
📚 Learning: 2026-06-22T07:50:54.935Z
Learnt from: AlejandroAkbal
Repo: Rule-34/App PR: 144
File: test/server/fix-canonical-queries.test.ts:174-178
Timestamp: 2026-06-22T07:50:54.935Z
Learning: In this Nuxt 3/4 project using `nuxt-schema-org` (v6.2.1), breadcrumb `source`/`itemListElement` URL values may be provided via `localePath()` (locale-relative in source code), but `nuxt-schema-org` renders JSON-LD with production-absolute URLs using `site.url` during schema rendering. When asserting JSON-LD breadcrumb `itemListElement` URLs in tests, expect production-absolute URLs (e.g., prefix with `project.urls.production.origin`), not the locale-relative `localePath()` forms. Do not flag production-absolute URL strings in JSON-LD breadcrumb tests as incorrect.
Applied to files:
test/assets/video-ad-policy.test.ts
🔇 Additional comments (9)
package.json (1)
59-59: LGTM!Also applies to: 69-71
nuxt.config.ts (1)
447-447: 🔒 Security & PrivacyVerify the nonce chain for the dynamically loaded IMA SDK.
With
'strict-dynamic', confirm thatloadImaSdk()is executed by a nonce-trusted script; otherwise the new policy may block the IMA SDK and silently disable ads because initialization errors are swallowed.app/assets/js/video-ad-policy.ts (1)
1-30: LGTM!test/assets/video-ad-policy.test.ts (1)
1-66: LGTM!app/types/videojs-ima.d.ts (1)
1-16: LGTM!app/components/pages/posts/post/PostMedia.vue (4)
502-514: 🎯 Functional Correctness | ⚡ Quick win
isProgrammaticPauseguard is defeated by microtask/macrotask ordering.
videoPlayer.pause()triggers the mediapauseevent as a task (macrotask), butisProgrammaticPauseis cleared in aqueueMicrotask, which runs first. By the time the'pause'handler callsrequestPauseAd,isProgrammaticis alreadyfalse, so that eligibility check contributes nothing. It's currently harmless only because this same branch also setsisVideoVisible = false, and theisVisiblecheck rejects the offscreen pause — but the intended programmatic-pause guard is effectively dead code and will silently fail if the visibility logic ever changes. Consider clearing the flag after the pause event settles (e.g., in the'pause'/'seeked'handler or asetTimeout(…, 0)), or drop the redundant flag.
156-204: LGTM!
206-272: LGTM!
353-366: LGTM!
|
Addressed the mobile IMA initialization feedback in aaef562: Video.js loads independently, the ad layer initializes after the first site interaction, and later video gestures synchronously initialize the available IMA display container. Also corrected programmatic-pause flag timing. Full |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/components/pages/posts/post/PostMedia.vue`:
- Around line 220-225: Update the dynamic import initialization around
loadImaSdk so loadImaSdk() and videojs-contrib-ads complete before importing
videojs-ima; preserve the CSS import as appropriate, but ensure videojs-ima is
evaluated only after both dependencies have resolved.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b77f2836-a8c0-46f0-8b36-7ca6cb0deb3f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
app/components/pages/posts/post/PostMedia.vuepackage.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{vue,ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier for formatting with 120-char print width, no semicolons, single quotes, trailing commas removed, and single attribute per line in Vue templates
Files:
package.jsonapp/components/pages/posts/post/PostMedia.vue
app/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
app/**/*.{vue,ts,tsx}: Use Nuxt 4 with Vue 3 and TypeScript for the frontend application
Use useLazyToast() to lazy-load vue-sonner; wait for ClientToaster to mount before calling toast.* using a proper async wait instead of plain nextTick()
Build reordered array synchronously instead of reading VueUse moveArrayElement() result before nextTick for state immediately persisted
Files:
app/components/pages/posts/post/PostMedia.vue
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use Nuxt flat ESLint via
@nuxt/eslintfor linting
Files:
app/components/pages/posts/post/PostMedia.vue
app/components/**/*.vue
📄 CodeRabbit inference engine (AGENTS.md)
app/components/**/*.vue: Register Vue components without path prefix using pathPrefix: false in nuxt.config.ts; import them as not <Input/DomainSelector>
Auto-import components from app/components/ flat structure without folder prefix
Use custom imgproxy provider for images; generate images at 1x density only in webp format to reduce bandwidth
Use@nuxt/imagev2 preload API (preload: { fetchPriority: 'high' }) for image preload priority instead of patching rendered HTML in Nitro
Use Nuxt Lazy* components, dynamic imports, or similarly deferred boundaries for components and composables only needed after user interaction (menu, sheet, dialog)
Files:
app/components/pages/posts/post/PostMedia.vue
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Define locales in config/i18n.ts as the single source of truth; do not hardcode locale lists in application code
Use PocketBase batch writes for multi-record replacement/reorder operations instead of one HTTP write per changed row
Files:
app/components/pages/posts/post/PostMedia.vue
app/**/*.vue
📄 CodeRabbit inference engine (AGENTS.md)
app/**/*.vue: Use project.urls.production for canonical URLs to point to production (https://r34.app/...) even when served from clone domains
Keep breadcrumb Schema.org source item URLs local/locale-relative; do not convert to project.urls.production since nuxt-schema-org resolves against site.url
When triggering a premium prompt from a nested dialog/bottom sheet, let the sheet owner close local UI, open the prompt, and restore UI after prompt closes
Premium auth transitions are reload-backed in dashboard/sign-in flow; rely on page reload to clear memory state instead of tracking PocketBase user ids in useState
Files:
app/components/pages/posts/post/PostMedia.vue
🔇 Additional comments (2)
package.json (1)
59-59: LGTM!Also applies to: 70-72
app/components/pages/posts/post/PostMedia.vue (1)
5-5: LGTM!Also applies to: 11-11, 30-36, 39-39, 104-106, 108-110, 112-116, 288-299, 312-312, 332-333, 385-386, 536-538
| await Promise.all([ | ||
| loadImaSdk(), | ||
| import('videojs-contrib-ads'), | ||
| import('videojs-ima'), | ||
| import('videojs-ima/dist/videojs.ima.css') | ||
| ]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fix race condition in dynamic plugin imports.
Using Promise.all to fetch and execute both videojs-contrib-ads and videojs-ima concurrently causes a race condition. videojs-ima modifies the Video.js module under the assumption that videojs-contrib-ads has already been fully evaluated. Furthermore, the IMA plugin requires the window.google.ima object to be populated by the time it executes.
Load videojs-ima sequentially only after videojs-contrib-ads and the loadImaSdk() promise have successfully resolved to prevent initialization crashes.
🐛 Proposed fix to ensure sequential dependency evaluation
- await Promise.all([
- loadImaSdk(),
- import('videojs-contrib-ads'),
- import('videojs-ima'),
- import('videojs-ima/dist/videojs.ima.css')
- ])
+ await Promise.all([
+ loadImaSdk(),
+ import('videojs-contrib-ads'),
+ import('videojs-ima/dist/videojs.ima.css')
+ ])
+ await import('videojs-ima')📝 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.
| await Promise.all([ | |
| loadImaSdk(), | |
| import('videojs-contrib-ads'), | |
| import('videojs-ima'), | |
| import('videojs-ima/dist/videojs.ima.css') | |
| ]) | |
| await Promise.all([ | |
| loadImaSdk(), | |
| import('videojs-contrib-ads'), | |
| import('videojs-ima/dist/videojs.ima.css') | |
| ]) | |
| await import('videojs-ima') |
🤖 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 `@app/components/pages/posts/post/PostMedia.vue` around lines 220 - 225, Update
the dynamic import initialization around loadImaSdk so loadImaSdk() and
videojs-contrib-ads complete before importing videojs-ima; preserve the CSS
import as appropriate, but ensure videojs-ima is evaluated only after both
dependencies have resolved.
1 participant
Summary
unsafe-evalSafety and UX
Verification
pnpm checkpnpm audit --prod— no known vulnerabilitiesDeployment gate
Live ExoClick inventory, blocker extensions, iPhone Safari, Android Chrome, and revenue parity still require a production canary because deterministic repository tests cannot prove third-party creative behavior.
Summary by CodeRabbit
unsafe-evalfromscript-src.