Add rage clicks, dead clicks, element visibility, outbound links, and error tracking#19
Merged
Conversation
Implements rageClicks tracker that detects rapid repeated clicks (3+ within 1 second by default) on the same element, with configurable threshold and window. Includes 13 tests with 100% code coverage.
Detects clicks on non-interactive elements (dead clicks) and emits dead_click events with element tag, text, and page name after a 500ms delay, with per-element 1-second debounce.
Implements IntersectionObserver-based tracker that emits element_visible events for elements tracked via data-abs-visible attributes or CSS selector rules in config, with once-per-page firing and route-change reset.
Implements outbound-links tracker that emits outbound_click events for clicks on external links (different hostname). Includes 100% branch/line/function/statement coverage.
WalkthroughThis pull request introduces five new tracker modules to the library: Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (1)
src/trackers/error-tracking.ts (1)
95-98: Clear dedupe timers on route change to avoid stale timer churn.On Line 95, route resets clear state but leave scheduled dedupe timers alive until expiry. Clearing them here keeps state and timer lifecycle fully aligned.
Suggested refactor
onRouteChange(): void { errorCount = 0; + for (const timer of dedupeTimers) { + clearTimeout(timer); + } + dedupeTimers.clear(); recentErrors.clear(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trackers/error-tracking.ts` around lines 95 - 98, The onRouteChange method resets errorCount and recentErrors but doesn't cancel any pending dedupe timers, leaving timers running after state is cleared; update onRouteChange to also cancel all scheduled timers associated with recentErrors (e.g., clearTimeout/clearInterval for any timer IDs stored alongside entries), iterate recentErrors to clear each timer before calling recentErrors.clear(), and ensure any dedicated timer store or fields used for deduping are also emptied so state and timer lifecycle stay aligned (references: onRouteChange, errorCount, recentErrors).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/trackers/error-tracking.test.ts`:
- Around line 258-270: The test only dispatches the 'unhandledrejection' event,
so update the "removes both listeners on destroy" test to dispatch both an
ErrorEvent and a PromiseRejectionEvent after tracker.destroy() to ensure neither
listener triggers ctx.emit; locate the test using errorTracker(),
tracker.init(ctx) and tracker.destroy() and add creation/dispatch of an
ErrorEvent (for the 'error' listener) in addition to the existing
makeRejectionEvent dispatch, then keep the assertion
expect(ctx.emit).not.toHaveBeenCalled().
In `@src/trackers/dead-clicks.ts`:
- Around line 25-27: The INTERACTIVE_SELECTOR currently treats any element with
a role attribute as interactive, causing elements with non-interactive roles
like "presentation" or "none" to suppress dead-click reporting; update
INTERACTIVE_SELECTOR to only match genuinely interactive ARIA roles (or
explicitly exclude non-interactive roles such as "presentation" and "none") and
apply the same change to the other occurrence that checks roles (the second
selector used around lines 42-43) so clicks under non-interactive roles are no
longer skipped.
- Around line 55-75: Rapid repeated clicks enqueue multiple timers because
recentlyReported isn't set until the 500ms timer fires; to fix, mark the element
as "reported" immediately when scheduling the timer and ensure it's removed
during cleanup if the context is destroyed. Concretely, inside the handler
around where you create the timer referenced as timer, call
recentlyReported.add(target) before setTimeout so subsequent clicks return
early, and in both the timer callback and the defensive destroy path ensure you
delete the target from recentlyReported (and pendingTimers) if the scheduled
emission never occurs or ctx is null; keep the existing clearTimer logic that
deletes recentlyReported after 1s so the debounce still expires.
In `@src/trackers/element-visibility.ts`:
- Around line 25-51: The scan() logic must rebuild the observed set on route
changes instead of only resetting per-element "fired" state; update the code so
that on route/navigation changes you clear and rebuild elementEventMap and
reattach the observer: call observer.disconnect() (if set), clear
elementEventMap, then run scan() to query ctx for new [data-abs-visible]
elements and rules (using the existing rules array) and observer.observe() them;
ensure destroy() still cleans up observer and map, and remove any reliance on
only resetting a per-element fired flag (see symbols scan, elementEventMap,
observer, rules, and destroy).
- Around line 69-72: parseDataAttributes can inject reserved keys like
event_name via data-abs-* and because extraProps is spread after eventName in
the ctx.emit call inside the element_visible handler, markup can override the
canonical event_name; fix by preventing reserved keys from extraProps before the
emit (filter or delete reserved keys such as event_name, page_name, etc. from
the object returned by parseDataAttributes or from extraProps) so that
ctx.emit("element_visible", { event_name: eventName, ...extraProps, page_name:
ctx.getPageName() }) always uses the tracker-controlled event_name; update the
code paths around parseDataAttributes, extraProps, and the ctx.emit call to
remove or ignore reserved keys.
- Around line 58-76: The observer callback currently fires on
entry.isIntersecting and ignores the configured threshold, so change the
condition to require entry.intersectionRatio >= threshold before marking
fired/emit; in the IntersectionObserver callback in element-visibility.ts
(referencing observer, fired, elementEventMap, ctx and threshold), replace the
early check `if (!entry.isIntersecting || !ctx) continue;` with a check that
also validates `entry.intersectionRatio >= threshold` (and keep the null check
for ctx), so the code only adds to `fired` and emits the mapped eventName/props
when the intersectionRatio meets or exceeds the configured threshold.
---
Nitpick comments:
In `@src/trackers/error-tracking.ts`:
- Around line 95-98: The onRouteChange method resets errorCount and recentErrors
but doesn't cancel any pending dedupe timers, leaving timers running after state
is cleared; update onRouteChange to also cancel all scheduled timers associated
with recentErrors (e.g., clearTimeout/clearInterval for any timer IDs stored
alongside entries), iterate recentErrors to clear each timer before calling
recentErrors.clear(), and ensure any dedicated timer store or fields used for
deduping are also emptied so state and timer lifecycle stay aligned (references:
onRouteChange, errorCount, recentErrors).
🪄 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: CHILL
Plan: Pro
Run ID: f0786853-4570-4708-a972-267a4c3569f6
📒 Files selected for processing (13)
README.mdpackage.jsonsrc/__tests__/trackers/dead-clicks.test.tssrc/__tests__/trackers/element-visibility.test.tssrc/__tests__/trackers/error-tracking.test.tssrc/__tests__/trackers/outbound-links.test.tssrc/__tests__/trackers/rage-clicks.test.tssrc/index.tssrc/trackers/dead-clicks.tssrc/trackers/element-visibility.tssrc/trackers/error-tracking.tssrc/trackers/outbound-links.tssrc/trackers/rage-clicks.ts
Comment on lines
+258
to
+270
| it("removes both listeners on destroy", () => { | ||
| const tracker = errorTracker(); | ||
| const ctx = createMockContext(); | ||
| tracker.init(ctx); | ||
| tracker.destroy(); | ||
|
|
||
| // After destroy, dispatching error/unhandledrejection events should not | ||
| // call emit since listeners were removed. | ||
| const rejectionEvent = makeRejectionEvent(Promise.resolve(), "gone"); | ||
| window.dispatchEvent(rejectionEvent); | ||
|
|
||
| expect(ctx.emit).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Actually exercise both listener removals.
This test only dispatches unhandledrejection, so a regression that leaves the error listener attached would still pass.
🧪 Suggested test extension
it("removes both listeners on destroy", () => {
const tracker = errorTracker();
const ctx = createMockContext();
tracker.init(ctx);
tracker.destroy();
+ const errorEvent = new ErrorEvent("error", {
+ message: "gone",
+ filename: "app.js",
+ lineno: 1,
+ colno: 1,
+ });
+ window.dispatchEvent(errorEvent);
+
// After destroy, dispatching error/unhandledrejection events should not
// call emit since listeners were removed.
const rejectionEvent = makeRejectionEvent(Promise.resolve(), "gone");
window.dispatchEvent(rejectionEvent);
expect(ctx.emit).not.toHaveBeenCalled();
});📝 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.
Suggested change
| it("removes both listeners on destroy", () => { | |
| const tracker = errorTracker(); | |
| const ctx = createMockContext(); | |
| tracker.init(ctx); | |
| tracker.destroy(); | |
| // After destroy, dispatching error/unhandledrejection events should not | |
| // call emit since listeners were removed. | |
| const rejectionEvent = makeRejectionEvent(Promise.resolve(), "gone"); | |
| window.dispatchEvent(rejectionEvent); | |
| expect(ctx.emit).not.toHaveBeenCalled(); | |
| }); | |
| it("removes both listeners on destroy", () => { | |
| const tracker = errorTracker(); | |
| const ctx = createMockContext(); | |
| tracker.init(ctx); | |
| tracker.destroy(); | |
| const errorEvent = new ErrorEvent("error", { | |
| message: "gone", | |
| filename: "app.js", | |
| lineno: 1, | |
| colno: 1, | |
| }); | |
| window.dispatchEvent(errorEvent); | |
| // After destroy, dispatching error/unhandledrejection events should not | |
| // call emit since listeners were removed. | |
| const rejectionEvent = makeRejectionEvent(Promise.resolve(), "gone"); | |
| window.dispatchEvent(rejectionEvent); | |
| expect(ctx.emit).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/trackers/error-tracking.test.ts` around lines 258 - 270, The
test only dispatches the 'unhandledrejection' event, so update the "removes both
listeners on destroy" test to dispatch both an ErrorEvent and a
PromiseRejectionEvent after tracker.destroy() to ensure neither listener
triggers ctx.emit; locate the test using errorTracker(), tracker.init(ctx) and
tracker.destroy() and add creation/dispatch of an ErrorEvent (for the 'error'
listener) in addition to the existing makeRejectionEvent dispatch, then keep the
assertion expect(ctx.emit).not.toHaveBeenCalled().
Comment on lines
+25
to
+27
| const INTERACTIVE_SELECTOR = | ||
| "a,button,input,select,textarea,label,summary,details," + | ||
| "[role],[onclick],[data-abs-track],[contenteditable='true']"; |
There was a problem hiding this comment.
[role] in the interactive selector is too broad and suppresses valid dead-clicks.
On Line 27 and Line 42, any ancestor with any ARIA role is treated as interactive, so clicks under non-interactive roles (e.g. presentation) are skipped incorrectly.
Suggested fix
const INTERACTIVE_SELECTOR =
"a,button,input,select,textarea,label,summary,details," +
- "[role],[onclick],[data-abs-track],[contenteditable='true']";
+ "[onclick],[data-abs-track],[contenteditable='true']";
@@
- const role = el.getAttribute("role");
+ const role = el.getAttribute("role")?.toLowerCase();
if (role && INTERACTIVE_ROLES.has(role)) return true;
@@
- if (el.closest(INTERACTIVE_SELECTOR)) return true;
+ if (el.closest(INTERACTIVE_SELECTOR)) return true;
+ const ancestorRole = el.closest("[role]")?.getAttribute("role")?.toLowerCase();
+ if (ancestorRole && INTERACTIVE_ROLES.has(ancestorRole)) return true;Also applies to: 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/trackers/dead-clicks.ts` around lines 25 - 27, The INTERACTIVE_SELECTOR
currently treats any element with a role attribute as interactive, causing
elements with non-interactive roles like "presentation" or "none" to suppress
dead-click reporting; update INTERACTIVE_SELECTOR to only match genuinely
interactive ARIA roles (or explicitly exclude non-interactive roles such as
"presentation" and "none") and apply the same change to the other occurrence
that checks roles (the second selector used around lines 42-43) so clicks under
non-interactive roles are no longer skipped.
Comment on lines
+55
to
+75
| if (recentlyReported.has(target)) return; | ||
|
|
||
| const timer = setTimeout(() => { | ||
| pendingTimers.delete(timer); | ||
| /* istanbul ignore if -- defensive guard; destroy() clears timers before nulling ctx */ | ||
| if (!ctx) return; | ||
| recentlyReported.add(target); | ||
| const text = (target.textContent || "").trim().slice(0, 100); | ||
| ctx.emit("dead_click", { | ||
| element_tag: target.tagName, | ||
| element_text: text, | ||
| page_name: ctx.getPageName(), | ||
| }); | ||
| const clearTimer = setTimeout(() => { | ||
| recentlyReported.delete(target); | ||
| pendingTimers.delete(clearTimer); | ||
| }, 1000); | ||
| pendingTimers.add(clearTimer); | ||
| }, 500); | ||
| pendingTimers.add(timer); | ||
| }; |
There was a problem hiding this comment.
Per-element debounce can emit duplicates before the 500ms timer fires.
On Line 55 and Line 57, repeated rapid clicks on the same element can enqueue multiple timers before recentlyReported is set, causing multiple dead_click emissions for one interaction burst.
Suggested fix
export function deadClicks(): Tracker {
let ctx: TrackerContext | null = null;
let handler: ((e: Event) => void) | null = null;
const pendingTimers = new Set<ReturnType<typeof setTimeout>>();
const recentlyReported = new WeakSet<Element>();
+ const pendingElements = new WeakSet<Element>();
@@
- if (recentlyReported.has(target)) return;
+ if (recentlyReported.has(target) || pendingElements.has(target)) return;
const timer = setTimeout(() => {
pendingTimers.delete(timer);
+ pendingElements.delete(target);
/* istanbul ignore if -- defensive guard; destroy() clears timers before nulling ctx */
if (!ctx) return;
recentlyReported.add(target);
@@
}, 500);
+ pendingElements.add(target);
pendingTimers.add(timer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/trackers/dead-clicks.ts` around lines 55 - 75, Rapid repeated clicks
enqueue multiple timers because recentlyReported isn't set until the 500ms timer
fires; to fix, mark the element as "reported" immediately when scheduling the
timer and ensure it's removed during cleanup if the context is destroyed.
Concretely, inside the handler around where you create the timer referenced as
timer, call recentlyReported.add(target) before setTimeout so subsequent clicks
return early, and in both the timer callback and the defensive destroy path
ensure you delete the target from recentlyReported (and pendingTimers) if the
scheduled emission never occurs or ctx is null; keep the existing clearTimer
logic that deletes recentlyReported after 1s so the debounce still expires.
Comment on lines
+25
to
+51
| function scan(): void { | ||
| /* istanbul ignore if -- defensive guard; scan() is only called from init() after ctx is set */ | ||
| if (!ctx) return; | ||
|
|
||
| const attrElements = ctx.querySelectorAll("[data-abs-visible]"); | ||
| for (const el of attrElements) { | ||
| if (!elementEventMap.has(el)) { | ||
| const event = el.getAttribute("data-abs-visible") || "element_visible"; | ||
| const props = parseDataAttributes(el); | ||
| delete props.visible; | ||
| elementEventMap.set(el, { event, props }); | ||
| /* istanbul ignore else -- observer is always set before scan() is called */ | ||
| if (observer) observer.observe(el); | ||
| } | ||
| } | ||
|
|
||
| for (const rule of rules) { | ||
| const elements = ctx.querySelectorAll(rule.selector); | ||
| for (const el of elements) { | ||
| if (!elementEventMap.has(el)) { | ||
| elementEventMap.set(el, { event: rule.event, props: {} }); | ||
| /* istanbul ignore else -- observer is always set before scan() is called */ | ||
| if (observer) observer.observe(el); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Rebuild the observed set on route changes.
Resetting fired alone only works when the next route reuses the same DOM nodes. If a SPA navigation replaces the page content, the new [data-abs-visible] and rule-matched elements are never observed, while elementEventMap keeps the old nodes until destroy().
🔧 Suggested fix
onRouteChange(): void {
fired = new WeakSet<Element>();
+ if (observer) {
+ observer.disconnect();
+ }
+ elementEventMap.clear();
+ scan();
},Also applies to: 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/trackers/element-visibility.ts` around lines 25 - 51, The scan() logic
must rebuild the observed set on route changes instead of only resetting
per-element "fired" state; update the code so that on route/navigation changes
you clear and rebuild elementEventMap and reattach the observer: call
observer.disconnect() (if set), clear elementEventMap, then run scan() to query
ctx for new [data-abs-visible] elements and rules (using the existing rules
array) and observer.observe() them; ensure destroy() still cleans up observer
and map, and remove any reliance on only resetting a per-element fired flag (see
symbols scan, elementEventMap, observer, rules, and destroy).
Comment on lines
+58
to
+76
| observer = new IntersectionObserver( | ||
| (entries) => { | ||
| for (const entry of entries) { | ||
| if (!entry.isIntersecting || !ctx) continue; | ||
| if (fired.has(entry.target)) continue; | ||
| fired.add(entry.target); | ||
|
|
||
| const mapping = elementEventMap.get(entry.target); | ||
| const eventName = mapping?.event || "element_visible"; | ||
| const extraProps = mapping?.props || {}; | ||
|
|
||
| ctx.emit("element_visible", { | ||
| event_name: eventName, | ||
| ...extraProps, | ||
| page_name: ctx.getPageName(), | ||
| }); | ||
| } | ||
| }, | ||
| { threshold }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does IntersectionObserver invoke its callback when a target first becomes intersecting even if the current intersectionRatio is still below the configured threshold (for example threshold 0.5 and ratio 0.1)?
💡 Result:
Yes, the IntersectionObserver callback is invoked when a target first becomes intersecting (i.e., intersectionRatio changes from 0 to >0), even if the current intersectionRatio (e.g., 0.1) is below the configured threshold (e.g., 0.5). The callback fires whenever the intersection ratio crosses any specified threshold value in either direction. Since thresholds like [0.5] implicitly include monitoring the crossing from below 0.5 to above (or vice versa), the initial entry from non-intersecting (ratio=0, below 0.5) to intersecting (ratio=0.1, still below 0.5) constitutes crossing the implicit lower boundary, triggering the callback. The entry's intersectionRatio will be 0.1, and isIntersecting will be true, but developers must check intersectionRatio >= threshold in the callback if they only want action at/above the threshold.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API
- 2: https://stackoverflow.com/questions/46079813/intersectionobserver-callback-called-outside-threshold
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API...
- 4: https://css-tricks.com/an-explanation-of-how-the-intersection-observer-watches/
- 5: https://stackoverflow.com/questions/48837222/intersectionobserver-isintersecting-property-never-is-false-if-threshold-greater
Honour the configured visibility threshold.
IntersectionObserver invokes its callback when isIntersecting transitions from false to true, even if intersectionRatio remains below the configured threshold. As written, a tracker configured for 50% visibility will emit as soon as any pixel appears, then permanently mark the element as fired, ignoring the threshold requirement.
🔧 Suggested fix
observer = new IntersectionObserver(
(entries) => {
for (const entry of entries) {
- if (!entry.isIntersecting || !ctx) continue;
+ if (
+ !entry.isIntersecting ||
+ entry.intersectionRatio < threshold ||
+ !ctx
+ ) {
+ continue;
+ }
if (fired.has(entry.target)) continue;
fired.add(entry.target);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/trackers/element-visibility.ts` around lines 58 - 76, The observer
callback currently fires on entry.isIntersecting and ignores the configured
threshold, so change the condition to require entry.intersectionRatio >=
threshold before marking fired/emit; in the IntersectionObserver callback in
element-visibility.ts (referencing observer, fired, elementEventMap, ctx and
threshold), replace the early check `if (!entry.isIntersecting || !ctx)
continue;` with a check that also validates `entry.intersectionRatio >=
threshold` (and keep the null check for ctx), so the code only adds to `fired`
and emits the mapped eventName/props when the intersectionRatio meets or exceeds
the configured threshold.
Comment on lines
+69
to
+72
| ctx.emit("element_visible", { | ||
| event_name: eventName, | ||
| ...extraProps, | ||
| page_name: ctx.getPageName(), |
There was a problem hiding this comment.
Protect reserved payload keys from data-abs-*.
parseDataAttributes() can produce event_name from data-abs-event-name. Because extraProps is spread after the computed value, markup can silently override the tracker’s canonical event_name.
🔧 Suggested fix
ctx.emit("element_visible", {
- event_name: eventName,
...extraProps,
+ event_name: eventName,
page_name: ctx.getPageName(),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/trackers/element-visibility.ts` around lines 69 - 72, parseDataAttributes
can inject reserved keys like event_name via data-abs-* and because extraProps
is spread after eventName in the ctx.emit call inside the element_visible
handler, markup can override the canonical event_name; fix by preventing
reserved keys from extraProps before the emit (filter or delete reserved keys
such as event_name, page_name, etc. from the object returned by
parseDataAttributes or from extraProps) so that ctx.emit("element_visible", {
event_name: eventName, ...extraProps, page_name: ctx.getPageName() }) always
uses the tracker-controlled event_name; update the code paths around
parseDataAttributes, extraProps, and the ctx.emit call to remove or ignore
reserved keys.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
5 new trackers:
Each tracker follows the existing pattern: factory function, Tracker interface, tree-shakeable sub-path exports, 100% test coverage.
Stats: 297 tests across 20 suites, 100% coverage (statements, branches, functions, lines)
Test plan
bun run test:coverage— 297 tests pass, 100% coveragebun run build— all outputs generatedbun run format:check— cleanSummary by CodeRabbit
New Features
Documentation
Tests