Skip to content
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
5 changes: 5 additions & 0 deletions packages/experiment-tag/src/message-bus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,18 @@ export type ElementVisiblePayload = { mutationList: MutationRecord[] };
export type AnalyticsEventPayload = AnalyticsEvent;
export type ManualTriggerPayload = { name: string };
export type UrlChangePayload = { updateActivePages?: boolean };
export type ScrolledToPayload = {
scrollPercentage: number;
elementAndOffset: Set<string>; // Keys like "selector:offsetPx" for elements in range
};

export type MessagePayloads = {
element_appeared: ElementAppearedPayload;
element_visible: ElementVisiblePayload;
url_change: UrlChangePayload;
analytics_event: AnalyticsEventPayload;
manual: ManualTriggerPayload;
scrolled_to: ScrolledToPayload;
};

export type MessageType = keyof MessagePayloads;
Expand Down
120 changes: 120 additions & 0 deletions packages/experiment-tag/src/subscriptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
ElementAppearedPayload,
ManualTriggerPayload,
MessageType,
ScrolledToPayload,
} from './message-bus';
import { DebouncedMutationManager } from './mutation-manager';
import {
Expand All @@ -15,6 +16,7 @@ import {
ManualTriggerValue,
PageObject,
PageObjects,
ScrolledToTriggerValue,
} from './types';

const evaluationEngine = new EvaluationEngine();
Expand Down Expand Up @@ -66,6 +68,7 @@ export class SubscriptionManager {
}
this.setupMutationObserverPublisher();
this.setupVisibilityPublisher();
this.setupScrolledToPublisher();
this.setupPageObjectSubscriptions();
this.setupUrlChangeReset();
// Initial check for elements that already exist
Expand Down Expand Up @@ -413,6 +416,106 @@ export class SubscriptionManager {
wrapHistoryMethods();
};

private setupScrolledToPublisher = () => {
Copy link

Choose a reason for hiding this comment

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

setupScrolledToPublisher adds a new scroll listener every time initSubscriptions() runs, but never removes the old one. This can leak and double-fire in visual editor mode where initSubscriptions() is called repeatedly. Consider removing any previous listener before attaching a new one (or keep a single shared handler).

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.

Copy link

Choose a reason for hiding this comment

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

scrolled_to publishes on every scroll once a target is met, unlike element_visible which de-dupes after first fire. Consider tracking which percentages/elements have already fired and only publish when a new target is first reached to avoid repeatedly triggering applyVariants() during scrolling.

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.

// Collect static list of scroll targets from page objects
let minPercentage: number | undefined = undefined;
const elementTargets: Array<{ selector: string; offsetPx: number }> = [];

for (const pages of Object.values(this.pageObjects)) {
for (const page of Object.values(pages)) {
if (page.trigger_type === 'scrolled_to') {
Comment on lines +424 to +426
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I see a repeated pattern for each trigger types. Consider refactoring.

const triggerValue = page.trigger_value as ScrolledToTriggerValue;

if (triggerValue.mode === 'percent') {
minPercentage =
minPercentage === undefined
? triggerValue.percentage
: Math.min(minPercentage, triggerValue.percentage);
} else if (triggerValue.mode === 'element') {
const offset = triggerValue.offsetPx || 0;
// Add if not already present
if (
!elementTargets.some(
(t) =>
t.selector === triggerValue.selector && t.offsetPx === offset,
)
) {
elementTargets.push({
selector: triggerValue.selector,
offsetPx: offset,
});
}
}
}
}
}

// Skip setup if no scroll triggers
if (minPercentage === undefined && elementTargets.length === 0) {
return;
}

const handleScroll = (
minPercent: number | undefined,
elements: Array<{ selector: string; offsetPx: number }>,
) => {
const scrollPercentage = this.calculateScrollPercentage();
const elementAndOffset = new Set<string>();

// Check which elements are in range
for (const { selector, offsetPx } of elements) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: WDYT about using IntersectionObserver for scrolled to element trigger type?

const element = this.globalScope.document.querySelector(selector);
if (element) {
const elementPosition = element.getBoundingClientRect().top;
const scrollPosition = this.globalScope.scrollY;

if (
scrollPosition + this.globalScope.innerHeight >=
elementPosition + scrollPosition + offsetPx
) {
Comment on lines +470 to +475
Copy link
Collaborator

Choose a reason for hiding this comment

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

scrollPosition would be canceled out. Should remove this.

const key = `${selector}:${offsetPx}`;
elementAndOffset.add(key);
}
}
}

// Publish if minimum percentage threshold is met or any element is in range
const shouldPublish =
(minPercent !== undefined && scrollPercentage >= minPercent) ||
elementAndOffset.size > 0;

if (shouldPublish) {
this.messageBus.publish('scrolled_to', {
scrollPercentage,
elementAndOffset,
});
}
Comment on lines +482 to +492
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that other trigger types evaluate in the subscription, I'd suggest moving this logic. Also you won't have to deal with this if you change the element scroll with IntersectionObserver.

};

this.globalScope.addEventListener(
'scroll',
() => handleScroll(minPercentage, elementTargets),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider throttling (e.g. requestAnimationFrame) since the scroll event can fire a lot. Each fires querySelector + getBoundingClientRect for every element target, forcing a reflow.

{ passive: true },
);

// Check immediately in case user is already scrolled
handleScroll(minPercentage, elementTargets);
};

private calculateScrollPercentage(): number {
const windowHeight = this.globalScope.innerHeight;
const documentHeight =
this.globalScope.document.documentElement.scrollHeight;
const scrollTop = this.globalScope.scrollY;
const scrollableHeight = documentHeight - windowHeight;

if (scrollableHeight <= 0) {
return 100;
}

return (scrollTop / scrollableHeight) * 100;
}

private isPageObjectActive = <T extends MessageType>(
page: PageObject,
message: MessagePayloads[T],
Expand Down Expand Up @@ -501,6 +604,23 @@ export class SubscriptionManager {
return this.elementVisibilityState.get(observerKey) ?? false;
}

case 'scrolled_to': {
const triggerValue = page.trigger_value as ScrolledToTriggerValue;
const scrollPayload = message as ScrolledToPayload;
const currentScrollPercentage = scrollPayload.scrollPercentage;
const elementAndOffset = scrollPayload.elementAndOffset;

if (triggerValue.mode === 'percent') {
return currentScrollPercentage >= triggerValue.percentage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The “scrolled to” condition should not be re-evaluated when the user scrolls back up. Once it’s exposed, it should remain active until the exit condition occurs—in this case, a URL change (the default exit condition).

} else if (triggerValue.mode === 'element') {
const offset = triggerValue.offsetPx || 0;
const key = `${triggerValue.selector}:${offset}`;
return elementAndOffset.has(key);
}

return false;
}

default:
return false;
}
Expand Down
16 changes: 16 additions & 0 deletions packages/experiment-tag/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,21 @@ export interface ManualTriggerValue {
name: string;
}

export interface ScrolledToElementConfig {
mode: 'element';
selector: string;
offsetPx?: number;
}

export interface ScrolledToPercentConfig {
mode: 'percent';
percentage: number;
}

export type ScrolledToTriggerValue =
| ScrolledToElementConfig
| ScrolledToPercentConfig;

export type PageObject = {
id: string;
name: string;
Expand All @@ -54,6 +69,7 @@ export type PageObject = {
| ElementAppearedTriggerValue
| ElementVisibleTriggerValue
| ManualTriggerValue
| ScrolledToTriggerValue
| Record<string, unknown>;
};

Expand Down
Loading