Skip to content

Batch type 1 page_load events + gate observer teardown on success#1878

Merged
subodhr258 merged 3 commits into
feat/type-1-tracking-enhancementfrom
feat/type-1-batching-and-retry
May 22, 2026
Merged

Batch type 1 page_load events + gate observer teardown on success#1878
subodhr258 merged 3 commits into
feat/type-1-tracking-enhancementfrom
feat/type-1-batching-and-retry

Conversation

@subodhr258
Copy link
Copy Markdown
Collaborator

@subodhr258 subodhr258 commented May 20, 2026

Follow-up to #1872 addressing P0#1 and P0#2 from the pre-merge review in rtCamp/godam-core#1083.

What changed

Scope is limited to assets/src/js/godam-player/analytics.js.

1. Batched dispatch for page_load events

The per-instance model in #1872 sends one fetch per intersecting <video>. For surfaces that genuinely render N inline players (Reels feed, etc.) this multiplies request count by N. Replaced the direct window.analytics.track() call inside trackPageLoadForVideo with a debounced queue:

  • Intersection events enqueue [ videoId, jobId ] pairs.
  • The queue auto-flushes when it reaches PAGE_LOAD_BATCH_SIZE (10) entries.
  • Otherwise a setTimeout fires PAGE_LOAD_FLUSH_DELAY_MS (1000ms) after the first pending entry.
  • Flush calls window.analytics.track( 'page_load', { type: 1, videoIds: [[...], ...] } ) — preserving the legacy bulk schema, so the analytics endpoint receives the same payload shape it has always handled.

This is mostly a server-side win: 30× fewer singleton inserts on the ingestion path, less exposure to WAF / rate-limit thresholds. User-facing perf is unchanged on HTTP/2 (both old and new paths multiplex over one connection).

2. Defensive observer gating

Previously the IntersectionObserver callback called trackPageLoadForVideo then observer.unobserve() unconditionally:

trackPageLoadForVideo( entry.target );
observer.unobserve( entry.target );
window.godamObservedPageLoadVideos.delete( entry.target );

If trackPageLoadForVideo returned false (e.g. window.analytics not yet ready), the event was dropped and the observer torn down with no retry. With the bundle structure on this branch the race is unlikely (window.analytics is set synchronously at module-eval in the same file), but the gating is cheap defense-in-depth.

trackPageLoadForVideo now returns:

  • true when the caller should stop observing — event enqueued, duplicate, or malformed metadata
  • false only when the caller should keep observing for a later retry tick — analytics library not yet ready

The IO callback only tears down observation when the return is true.

3. Keepalive flush on page teardown

The new queue is flushed synchronously from the existing visibilitychange === 'hidden' and pagehide / beforeunload handlers via a direct keepalive fetch (mirroring sendPlayerHeatmap). This bypasses the DavidWells async pipeline, so pending batches survive tab close / background even when the library's async queue has not yet drained.

Test plan

Before

Screen.Recording.2026-05-20.at.6.11.58.PM.mov

After (batches of max 10 videos, 1 second delay)

Batch.fix.for.type.1.mov

Test on iOS

Screen.Recording.2026-05-20.at.7.02.54.PM.mov

🤖 Generated with Claude Code

…cess

Address two follow-ups from the pre-merge review of #1872:

1. Batching / debouncing: replace the one-fetch-per-intersection model
   with a debounced queue. Intersection events enqueue [videoId, jobId]
   pairs that flush either after 300ms or when the queue hits 10
   entries, preserving the legacy videoIds: [[id, job], ...] schema.
   Reduces singleton inserts on the ingestion side and protects against
   WAF rate-limit thresholds for surfaces that genuinely render N
   inline players (Reels feed, etc.).

2. Defensive observer gating: only call observer.unobserve when
   trackPageLoadForVideo reports it is done with the element. A
   "retry" return (currently only when window.analytics is unset) now
   leaves the IO entry alive so a subsequent intersection tick can
   retry instead of silently dropping the event.

Also flush pending batches through a synchronous keepalive fetch from
the existing visibilitychange/pagehide handlers so in-flight events
survive page teardown without waiting on the DavidWells async queue.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 20, 2026

🔍 WordPress Plugin Check Report

❌ Status: Failed

📊 Report

🎯 Total Issues ❌ Errors ⚠️ Warnings
15 1 14

❌ Errors (1)

📁 readme.txt (1 error)
📍 Line 🔖 Check 💬 Message
0 outdated_tested_upto_header Tested up to: 6.9 < 7.0. The "Tested up to" value in your plugin is not set to the current version of WordPress. This means your plugin will not show up in searches, as we require plugins to be compatible and documented as tested up to the most recent version of WordPress.

⚠️ Warnings (14)

📁 readme.txt (2 warnings)
📍 Line 🔖 Check 💬 Message
0 mismatched_plugin_name Plugin name "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" is different from the name declared in plugin header "GoDAM".
0 trademarked_term The plugin name includes a restricted term. Your chosen plugin name - "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" - contains the restricted term "wordpress" which cannot be used at all in your plugin name.
📁 composer.json (1 warning)
📍 Line 🔖 Check 💬 Message
0 missing_composer_json_file The "/vendor" directory using composer exists, but "composer.json" file is missing.
📁 assets/build/css/main.css (1 warning)
📍 Line 🔖 Check 💬 Message
0 EnqueuedStylesScope This style is being loaded in all contexts.
📁 assets/src/libs/analytics.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/05/21/demo-post-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-page-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
📁 assets/build/js/main.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/05/21/demo-post-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-page-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.

🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refines GoDAM’s frontend player analytics (page_load type 1) by batching per-video viewport-triggered events into fewer requests, improving ingestion efficiency on pages with many inline players. It also makes the IntersectionObserver teardown conditional on successful enqueueing, and forces a keepalive flush during page teardown to reduce event loss.

Changes:

  • Batch page_load (type 1) events into a debounced queue with size/time-based flushing.
  • Only unobserve a video after trackPageLoadForVideo() indicates it is safe to stop tracking (retry when analytics isn’t ready).
  • Flush pending type 1 events via synchronous keepalive fetch on visibilitychange (hidden) and unload events.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread assets/src/js/godam-player/analytics.js
Comment thread assets/src/js/godam-player/analytics.js
Comment thread assets/src/js/godam-player/analytics.js
Both bail paths in flushPageLoadQueue cleared the debounce timer before
deciding whether to drain, so when a bail returned without sending,
the queue could sit until the next intersection event triggered
another enqueue.

- sync + shouldSkipAnalytics: clear the queue. shouldSkipAnalytics()
  is constant for the page session (isAdminPage / preview query param),
  so these events would never be sendable through the async path either.
  Dropping them avoids unbounded queue growth across bfcache restores.

- async + !window.analytics: reschedule the flush. Bail before the
  splice so we don't need the unshift dance, then schedule a new timer
  so a missing analytics library is retried rather than stranding the
  batch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@subodhr258 subodhr258 merged commit ba7aa65 into feat/type-1-tracking-enhancement May 22, 2026
2 of 3 checks passed
@subodhr258 subodhr258 deleted the feat/type-1-batching-and-retry branch May 22, 2026 08:52
subodhr258 added a commit that referenced this pull request May 22, 2026
)

* feat: implement type 1 video page load tracking with viewport observation

* Batch type 1 page_load events + gate observer teardown on success (#1878)

* feat: batch type 1 page_load events and gate observer teardown on success

Address two follow-ups from the pre-merge review of #1872:

1. Batching / debouncing: replace the one-fetch-per-intersection model
   with a debounced queue. Intersection events enqueue [videoId, jobId]
   pairs that flush either after 300ms or when the queue hits 10
   entries, preserving the legacy videoIds: [[id, job], ...] schema.
   Reduces singleton inserts on the ingestion side and protects against
   WAF rate-limit thresholds for surfaces that genuinely render N
   inline players (Reels feed, etc.).

2. Defensive observer gating: only call observer.unobserve when
   trackPageLoadForVideo reports it is done with the element. A
   "retry" return (currently only when window.analytics is unset) now
   leaves the IO entry alive so a subsequent intersection tick can
   retry instead of silently dropping the event.

Also flush pending batches through a synchronous keepalive fetch from
the existing visibilitychange/pagehide handlers so in-flight events
survive page teardown without waiting on the DavidWells async queue.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix: Increase batch flush delay

* fix: prevent stranded batches when flushPageLoadQueue bails

Both bail paths in flushPageLoadQueue cleared the debounce timer before
deciding whether to drain, so when a bail returned without sending,
the queue could sit until the next intersection event triggered
another enqueue.

- sync + shouldSkipAnalytics: clear the queue. shouldSkipAnalytics()
  is constant for the page session (isAdminPage / preview query param),
  so these events would never be sendable through the async path either.
  Dropping them avoids unbounded queue growth across bfcache restores.

- async + !window.analytics: reschedule the flush. Bail before the
  splice so we don't need the unshift dance, then schedule a new timer
  so a missing analytics library is retried rather than stranding the
  batch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Subodh Rajpopat <subodh.rajpopat@rtcamp.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants