Skip to content

Commit ecaa8f2

Browse files
bkboothclaude
andcommitted
fix(pixel): cancel pending rAF on scroll reset; queue all containers per frame
Addresses two Cursor bugbot findings: 1. Stale rAF after reset() (medium): if a scroll event scheduled a rAF just before pixel.page() called resetScrollDepth(), the rAF would read the (still-stale) scrollTop of a reused container and fire milestones against the freshly-cleared set — misattributing scroll depth to the new page view. reset() now cancels any pending rAF. 2. Single pendingEl drops concurrent container scrolls (low): if two large containers fired scroll events in the same frame, only the later target was checked. Replaced pendingEl with a Set so every container that scrolled within the frame is processed on rAF. Adds two regression tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 2dbc7fe commit ecaa8f2

2 files changed

Lines changed: 85 additions & 10 deletions

File tree

packages/audience/pixel/src/autocapture.test.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -928,6 +928,70 @@ describe('autocapture', () => {
928928
expect(enqueue).toHaveBeenCalledTimes(2);
929929
expect(enqueue).toHaveBeenLastCalledWith('scroll_depth', { depth: 25 });
930930
});
931+
932+
it('cancels pending rAF so stale scroll position cannot fire against new page', () => {
933+
const result = setupAutocapture({ scroll: true }, enqueue, () => consent);
934+
teardown = result.teardown;
935+
936+
// User has scrolled to 50% — rAF is scheduled but has not yet fired.
937+
(window as Record<string, unknown>).scrollY = 750;
938+
document.dispatchEvent(new Event('scroll'));
939+
expect(enqueue).not.toHaveBeenCalled(); // rAF not flushed yet
940+
941+
// SPA navigates: pixel.page() calls resetScroll() before the rAF fires.
942+
// The reused container still reports scrollY = 750 momentarily.
943+
result.resetScroll();
944+
945+
// Flushing the (now-cancelled) rAF must not fire any milestone against
946+
// the new page view.
947+
flushRAF();
948+
expect(enqueue).not.toHaveBeenCalled();
949+
});
950+
});
951+
952+
describe('concurrent containers', () => {
953+
function setContainerGeometry(
954+
el: HTMLElement,
955+
scrollHeight: number,
956+
clientHeight: number,
957+
scrollTop: number,
958+
) {
959+
Object.defineProperty(el, 'scrollHeight', { value: scrollHeight, configurable: true });
960+
Object.defineProperty(el, 'clientHeight', { value: clientHeight, configurable: true });
961+
Object.defineProperty(el, 'scrollTop', { value: scrollTop, configurable: true, writable: true });
962+
}
963+
964+
beforeEach(() => {
965+
setScrollGeometry(600, 600, 0);
966+
});
967+
968+
it('processes every container that scrolled within a single rAF', () => {
969+
setup({ scroll: true });
970+
971+
const main = document.createElement('div');
972+
const sidebar = document.createElement('div');
973+
setContainerGeometry(main, 2000, 500, 0);
974+
setContainerGeometry(sidebar, 1000, 500, 0);
975+
document.body.appendChild(main);
976+
document.body.appendChild(sidebar);
977+
978+
// Two large containers scroll in the same frame (before rAF flush).
979+
// main → 30% (450/1500), sidebar → 60% (300/500).
980+
// Without per-container queueing, only the second target would be
981+
// checked and main's milestone would be lost.
982+
(main as Record<string, unknown>).scrollTop = 450;
983+
main.dispatchEvent(new Event('scroll'));
984+
(sidebar as Record<string, unknown>).scrollTop = 300;
985+
sidebar.dispatchEvent(new Event('scroll'));
986+
987+
flushRAF();
988+
989+
// Both 25 (from main) and 50 (from sidebar) should fire — global dedup
990+
// applies across all containers processed in the frame.
991+
expect(enqueue).toHaveBeenCalledTimes(2);
992+
expect(enqueue).toHaveBeenCalledWith('scroll_depth', { depth: 25 });
993+
expect(enqueue).toHaveBeenCalledWith('scroll_depth', { depth: 50 });
994+
});
931995
});
932996
});
933997

packages/audience/pixel/src/autocapture.ts

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ function setupScrollTracking(
7777
getConsent: ConsentFn,
7878
): { teardown: () => void; reset: () => void } {
7979
const fired = new Set<number>();
80+
const pending = new Set<Element>();
8081
let rafId = 0;
81-
let pendingEl: Element | null = null;
8282

8383
const checkAndFire = (el: Element, scrollPos: number): void => {
8484
if (!canTrack(getConsent())) return;
@@ -97,6 +97,14 @@ function setupScrollTracking(
9797
}
9898
};
9999

100+
const cancelPending = (): void => {
101+
pending.clear();
102+
if (rafId) {
103+
cancelAnimationFrame(rafId);
104+
rafId = 0;
105+
}
106+
};
107+
100108
const onScroll = (e: Event): void => {
101109
if (fired.size === SCROLL_MILESTONES.length) return;
102110

@@ -107,18 +115,18 @@ function setupScrollTracking(
107115
// Ignore small containers (dropdowns, tooltips, autocompletes).
108116
if (target.clientHeight <= window.innerHeight * 0.5) return;
109117

110-
pendingEl = target;
118+
pending.add(target);
111119

112120
if (rafId) return;
113121
rafId = requestAnimationFrame(() => {
114122
rafId = 0;
115-
const el = pendingEl;
116-
pendingEl = null;
117-
if (!el) return;
118-
const scrollPos = el === document.documentElement
119-
? window.scrollY
120-
: el.scrollTop;
121-
checkAndFire(el, scrollPos);
123+
pending.forEach((el) => {
124+
const scrollPos = el === document.documentElement
125+
? window.scrollY
126+
: el.scrollTop;
127+
checkAndFire(el, scrollPos);
128+
});
129+
pending.clear();
122130
});
123131
};
124132

@@ -130,10 +138,13 @@ function setupScrollTracking(
130138
return {
131139
teardown: () => {
132140
document.removeEventListener('scroll', onScroll, { capture: true });
133-
if (rafId) cancelAnimationFrame(rafId);
141+
cancelPending();
134142
},
143+
// Cancel any pending rAF so a stale scroll position from the previous
144+
// page view can't fire milestones against the freshly-cleared set.
135145
reset: () => {
136146
fired.clear();
147+
cancelPending();
137148
},
138149
};
139150
}

0 commit comments

Comments
 (0)