Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracker fixes #5199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions tracker/src/plausible.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,10 @@
// prevents registering multiple listeners in those cases.
var listeningOnEngagement = false

// In SPA-s, multiple listeners that trigger the engagement event
// might fire nearly at the same time. E.g. when navigating back
// in browser history while using hash-based routing - a popstate
// and hashchange will be fired in a very quick succession. This
// flag prevents sending multiple engagement events in those cases.
var engagementCooldown = false

// Timestamp indicating when this particular page last became visible.
// Reset during pageviews, set to null when page is closed.
var runningEngagementStart = null

// When page is hidden, this 'engaged' time is saved to this variable
var currentEngagementTime = 0

Expand Down Expand Up @@ -121,13 +115,10 @@

The first engagement event is always sent due to containing at least the initial scroll depth.

We don't send engagements if:
- Less than 300ms have passed since the last engagement event
- The current pageview is ignored (onIgnoredEvent)
Also, we don't send engagements if the current pageview is ignored (onIgnoredEvent)
*/
if (!engagementCooldown && !currentEngagementIgnored && (currentEngagementMaxScrollDepth < maxScrollDepthPx || engagementTime >= 3000)) {
if (!currentEngagementIgnored && (currentEngagementMaxScrollDepth < maxScrollDepthPx || engagementTime >= 3000)) {
currentEngagementMaxScrollDepth = maxScrollDepthPx
setTimeout(function () {engagementCooldown = false}, 300)

var payload = {
n: 'engagement',
Expand Down
10 changes: 1 addition & 9 deletions tracker/test/engagement.spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { expect } = require("@playwright/test")
const { expectPlausibleInAction, engagementCooldown, hideAndShowCurrentTab } = require('./support/test-utils')
const { expectPlausibleInAction, hideAndShowCurrentTab } = require('./support/test-utils')
const { test } = require('@playwright/test')
const { LOCAL_SERVER_ADDR } = require('./support/server')

Expand Down Expand Up @@ -99,8 +99,6 @@ test.describe('engagement events', () => {
expectedRequests: [{n: 'engagement', u: pageBaseURL, h: 1}]
})

await engagementCooldown(page)

// Navigate from ignored page to a tracked page ->
// no engagement from the current page, pageview on the next page
await expectPlausibleInAction(page, {
Expand All @@ -109,8 +107,6 @@ test.describe('engagement events', () => {
refutedRequests: [{n: 'engagement'}]
})

await engagementCooldown(page)

// Navigate from a tracked page to another tracked page ->
// engagement with the last page URL, pageview with the new URL
await expectPlausibleInAction(page, {
Expand Down Expand Up @@ -162,8 +158,6 @@ test.describe('engagement events', () => {
]
})

await engagementCooldown(page)

await expectPlausibleInAction(page, {
action: () => page.click('#jane-post'),
expectedRequests: [
Expand All @@ -172,8 +166,6 @@ test.describe('engagement events', () => {
]
})

await engagementCooldown(page)

await expectPlausibleInAction(page, {
action: () => page.click('#home'),
expectedRequests: [
Expand Down
4 changes: 1 addition & 3 deletions tracker/test/scroll-depth.spec.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { engagementCooldown, expectPlausibleInAction, hideCurrentTab, hideAndShowCurrentTab } = require('./support/test-utils')
const { expectPlausibleInAction, hideCurrentTab, hideAndShowCurrentTab } = require('./support/test-utils')
const { test } = require('@playwright/test')
const { LOCAL_SERVER_ADDR } = require('./support/server')

Expand Down Expand Up @@ -32,8 +32,6 @@ test.describe('scroll depth (engagement events)', () => {
]
})

await engagementCooldown(page)

await expectPlausibleInAction(page, {
action: () => page.click('#home-link'),
expectedRequests: [
Expand Down
6 changes: 0 additions & 6 deletions tracker/test/support/test-utils.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
const { expect, Page } = require("@playwright/test");

// Since engagement events in the Plausible script are throttled to 300ms, we
// often need to wait for an artificial timeout before navigating in tests.
exports.engagementCooldown = async function(page) {
return page.waitForTimeout(400)
}

// Mocks an HTTP request call with the given path. Returns a Promise that resolves to the request
// data. If the request is not made, resolves to null after 3 seconds.
const mockRequest = function (page, path) {
Expand Down
Loading