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.

Sign up for GitHub

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

Jump to bottom

fix: dead click fixes from watching in prod #1508

Merged
merged 2 commits into from
Nov 1, 2024
Merged

fix: dead click fixes from watching in prod #1508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afaa990
fix: dead click fixes from watching in prod
pauldambra Oct 31, 2024
01154fd
don't ignore disabled
pauldambra Nov 1, 2024
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
218 changes: 115 additions & 103 deletions src/__tests__/entrypoints/lazy-loaded-dead-clicks-autocapture.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,32 +211,35 @@ describe('LazyLoadedDeadClicksAutocapture', () => {
lazyLoadedDeadClicksAutocapture['_checkClicks']()

expect(lazyLoadedDeadClicksAutocapture['_clicks']).toHaveLength(0)
expect(fakeInstance.capture).toHaveBeenCalledWith('$dead_click', {
// faked system timestamp isn't moving so this is negative
$dead_click_absolute_delay_ms: -900,
$dead_click_absolute_timeout: false,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: undefined,
$dead_click_last_scroll_timestamp: undefined,
$dead_click_mutation_delay_ms: undefined,
$dead_click_mutation_timeout: false,
$dead_click_scroll_delay_ms: undefined,
$dead_click_scroll_timeout: false,
$dead_click_selection_changed_delay_ms: 100,
$dead_click_selection_changed_timeout: true,
timestamp: 900,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
})
expect(fakeInstance.capture).toHaveBeenCalledWith(
'$dead_click',
{
// faked system timestamp isn't moving so this is negative
$dead_click_absolute_delay_ms: -900,
$dead_click_absolute_timeout: false,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: undefined,
$dead_click_last_scroll_timestamp: undefined,
$dead_click_mutation_delay_ms: undefined,
$dead_click_mutation_timeout: false,
$dead_click_scroll_delay_ms: undefined,
$dead_click_scroll_timeout: false,
$dead_click_selection_changed_delay_ms: 100,
$dead_click_selection_changed_timeout: true,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
},
{ timestamp: new Date(900) }
)
})

it('click followed by a mutation after threshold, dead click', () => {
Expand All @@ -250,32 +253,35 @@ describe('LazyLoadedDeadClicksAutocapture', () => {
lazyLoadedDeadClicksAutocapture['_checkClicks']()

expect(lazyLoadedDeadClicksAutocapture['_clicks']).toHaveLength(0)
expect(fakeInstance.capture).toHaveBeenCalledWith('$dead_click', {
// faked system timestamp isn't moving so this is negative
$dead_click_absolute_delay_ms: -900,
$dead_click_absolute_timeout: false,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: 3401,
$dead_click_last_scroll_timestamp: undefined,
$dead_click_mutation_delay_ms: 2501,
$dead_click_mutation_timeout: true,
$dead_click_scroll_delay_ms: undefined,
$dead_click_scroll_timeout: false,
$dead_click_selection_changed_delay_ms: undefined,
$dead_click_selection_changed_timeout: false,
timestamp: 900,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
})
expect(fakeInstance.capture).toHaveBeenCalledWith(
'$dead_click',
{
// faked system timestamp isn't moving so this is negative
$dead_click_absolute_delay_ms: -900,
$dead_click_absolute_timeout: false,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: 3401,
$dead_click_last_scroll_timestamp: undefined,
$dead_click_mutation_delay_ms: 2501,
$dead_click_mutation_timeout: true,
$dead_click_scroll_delay_ms: undefined,
$dead_click_scroll_timeout: false,
$dead_click_selection_changed_delay_ms: undefined,
$dead_click_selection_changed_timeout: false,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
},
{ timestamp: new Date(900) }
)
})

it('click followed by a scroll after threshold, dead click', () => {
Expand All @@ -290,31 +296,34 @@ describe('LazyLoadedDeadClicksAutocapture', () => {
lazyLoadedDeadClicksAutocapture['_checkClicks']()

expect(lazyLoadedDeadClicksAutocapture['_clicks']).toHaveLength(0)
expect(fakeInstance.capture).toHaveBeenCalledWith('$dead_click', {
// faked system timestamp isn't moving so this is negative
$dead_click_absolute_delay_ms: -900,
$dead_click_absolute_timeout: false,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: undefined,
$dead_click_mutation_delay_ms: undefined,
$dead_click_mutation_timeout: false,
$dead_click_scroll_delay_ms: 2501,
$dead_click_scroll_timeout: true,
$dead_click_selection_changed_delay_ms: undefined,
$dead_click_selection_changed_timeout: false,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
timestamp: 900,
})
expect(fakeInstance.capture).toHaveBeenCalledWith(
'$dead_click',
{
// faked system timestamp isn't moving so this is negative
$dead_click_absolute_delay_ms: -900,
$dead_click_absolute_timeout: false,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: undefined,
$dead_click_mutation_delay_ms: undefined,
$dead_click_mutation_timeout: false,
$dead_click_scroll_delay_ms: 2501,
$dead_click_scroll_timeout: true,
$dead_click_selection_changed_delay_ms: undefined,
$dead_click_selection_changed_timeout: false,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
},
{ timestamp: new Date(900) }
)
})

it('click followed by nothing for too long, dead click', () => {
Expand All @@ -325,35 +334,38 @@ describe('LazyLoadedDeadClicksAutocapture', () => {
})
lazyLoadedDeadClicksAutocapture['_lastMutation'] = undefined

jest.setSystemTime(2501 + 900)
jest.setSystemTime(3001 + 900)
lazyLoadedDeadClicksAutocapture['_checkClicks']()

expect(lazyLoadedDeadClicksAutocapture['_clicks']).toHaveLength(0)
expect(fakeInstance.capture).toHaveBeenCalledWith('$dead_click', {
$dead_click_absolute_delay_ms: 2501,
$dead_click_absolute_timeout: true,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: undefined,
$dead_click_last_scroll_timestamp: undefined,
$dead_click_mutation_delay_ms: undefined,
$dead_click_mutation_timeout: false,
$dead_click_scroll_delay_ms: undefined,
$dead_click_scroll_timeout: false,
$dead_click_selection_changed_delay_ms: undefined,
$dead_click_selection_changed_timeout: false,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
timestamp: 900,
})
expect(fakeInstance.capture).toHaveBeenCalledWith(
'$dead_click',
{
$dead_click_absolute_delay_ms: 3001,
$dead_click_absolute_timeout: true,
$dead_click_event_timestamp: 900,
$dead_click_last_mutation_timestamp: undefined,
$dead_click_last_scroll_timestamp: undefined,
$dead_click_mutation_delay_ms: undefined,
$dead_click_mutation_timeout: false,
$dead_click_scroll_delay_ms: undefined,
$dead_click_scroll_timeout: false,
$dead_click_selection_changed_delay_ms: undefined,
$dead_click_selection_changed_timeout: false,
$ce_version: 1,
$el_text: 'text',
$elements: [
{
$el_text: 'text',
nth_child: 2,
nth_of_type: 1,
tag_name: 'body',
},
],
$event_type: 'click',
},
{ timestamp: new Date(900) }
)
})

it('click not followed by anything within threshold, rescheduled for next check', () => {
Expand Down
42 changes: 1 addition & 41 deletions src/autocapture-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { each, entries, includes, trim } from './utils'
import { isArray, isNullish, isString, isUndefined } from './utils/type-utils'
import { logger } from './utils/logger'
import { window } from './utils/globals'
import { isDocumentFragment, isElementNode, isTag, isTextNode } from './utils/element-utils'

export function splitClassString(s: string): string[] {
return s ? trim(s).split(/\s+/) : []
Expand Down Expand Up @@ -94,47 +95,6 @@ export function getEventTarget(e: Event): Element | null {
}
}

/*
* Check whether an element has nodeType Node.ELEMENT_NODE
* @param {Element} el - element to check
* @returns {boolean} whether el is of the correct nodeType
*/
export function isElementNode(el: Node | Element | undefined | null): el is Element {
return !!el && el.nodeType === 1 // Node.ELEMENT_NODE - use integer constant for browser portability
}

/*
* Check whether an element is of a given tag type.
* Due to potential reference discrepancies (such as the webcomponents.js polyfill),
* we want to match tagNames instead of specific references because something like
* element === document.body won't always work because element might not be a native
* element.
* @param {Element} el - element to check
* @param {string} tag - tag name (e.g., "div")
* @returns {boolean} whether el is of the given tag type
*/
export function isTag(el: Element | undefined | null, tag: string): el is HTMLElement {
return !!el && !!el.tagName && el.tagName.toLowerCase() === tag.toLowerCase()
}

/*
* Check whether an element has nodeType Node.TEXT_NODE
* @param {Element} el - element to check
* @returns {boolean} whether el is of the correct nodeType
*/
export function isTextNode(el: Element | undefined | null): el is HTMLElement {
return !!el && el.nodeType === 3 // Node.TEXT_NODE - use integer constant for browser portability
}

/*
* Check whether an element has nodeType Node.DOCUMENT_FRAGMENT_NODE
* @param {Element} el - element to check
* @returns {boolean} whether el is of the correct nodeType
*/
export function isDocumentFragment(el: Element | ParentNode | undefined | null): el is DocumentFragment {
return !!el && el.nodeType === 11 // Node.DOCUMENT_FRAGMENT_NODE - use integer constant for browser portability
}

export const autocaptureCompatibleElements = ['a', 'button', 'form', 'input', 'select', 'textarea', 'label']

/*
Expand Down
5 changes: 1 addition & 4 deletions src/autocapture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ import {
getEventTarget,
getSafeText,
isAngularStyleAttr,
isDocumentFragment,
isElementNode,
isSensitiveElement,
isTag,
isTextNode,
makeSafeText,
shouldCaptureDomEvent,
shouldCaptureElement,
Expand All @@ -27,6 +23,7 @@ import { isBoolean, isFunction, isNull, isObject } from './utils/type-utils'
import { logger } from './utils/logger'
import { document, window } from './utils/globals'
import { convertToURL } from './utils/request-utils'
import { isDocumentFragment, isElementNode, isTag, isTextNode } from './utils/element-utils'

const COPY_AUTOCAPTURE_EVENT = '$copy_autocapture'

Expand Down
45 changes: 26 additions & 19 deletions src/entrypoints/dead-clicks-autocapture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { assignableWindow, LazyLoadedDeadClicksAutocaptureInterface } from '../utils/globals'
import { PostHog } from '../posthog-core'
import { isNull, isNumber, isUndefined } from '../utils/type-utils'
import { autocaptureCompatibleElements, getEventTarget, isElementNode, isTag } from '../autocapture-utils'
import { autocaptureCompatibleElements, getEventTarget } from '../autocapture-utils'
import { DeadClicksAutoCaptureConfig, Properties } from '../types'
import { autocapturePropertiesForElement } from '../autocapture'
import { isElementInToolbar } from '../utils/element-utils'
import { isElementInToolbar, isElementNode, isTag } from '../utils/element-utils'

const DEFAULT_CONFIG: Required<DeadClicksAutoCaptureConfig> = {
element_attribute_ignorelist: [],
Expand Down Expand Up @@ -205,7 +205,9 @@ class LazyLoadedDeadClicksAutocapture implements LazyLoadedDeadClicksAutocapture
this._config.selection_change_threshold_ms
)
const mutationTimeout = checkTimeout(click.mutationDelayMs, this._config.mutation_threshold_ms)
const absoluteTimeout = checkTimeout(click.absoluteDelayMs, this._config.mutation_threshold_ms)
// we want to timeout eventually even if nothing else catches it...
// we leave a little longer than the maximum threshold to give the other checks a chance to catch it
const absoluteTimeout = checkTimeout(click.absoluteDelayMs, this._config.mutation_threshold_ms * 1.1)
Copy link
Member Author

Choose a reason for hiding this comment

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

we saw absolute timeouts, without seeing mutation timeouts, that also seems wrong so...


const hadScroll = isNumber(click.scrollDelayMs) && click.scrollDelayMs < this._config.scroll_threshold_ms
const hadMutation =
Expand Down Expand Up @@ -244,22 +246,27 @@ class LazyLoadedDeadClicksAutocapture implements LazyLoadedDeadClicksAutocapture
private _captureDeadClick(click: Click, properties: Properties) {
// TODO need to check safe and captur-able as with autocapture
// TODO autocaputure config
this.instance.capture('$dead_click', {
...properties,
...autocapturePropertiesForElement(click.node, {
e: click.originalEvent,
maskAllElementAttributes: this.instance.config.mask_all_element_attributes,
maskAllText: this.instance.config.mask_all_text,
elementAttributeIgnoreList: this._config.element_attribute_ignorelist,
// TRICKY: it appears that we were moving to elementsChainAsString, but the UI still depends on elements, so :shrug:
elementsChainAsString: false,
}).props,
$dead_click_scroll_delay_ms: click.scrollDelayMs,
$dead_click_mutation_delay_ms: click.mutationDelayMs,
$dead_click_absolute_delay_ms: click.absoluteDelayMs,
$dead_click_selection_changed_delay_ms: click.selectionChangedDelayMs,
timestamp: click.timestamp,
})
this.instance.capture(
'$dead_click',
{
...properties,
...autocapturePropertiesForElement(click.node, {
e: click.originalEvent,
maskAllElementAttributes: this.instance.config.mask_all_element_attributes,
maskAllText: this.instance.config.mask_all_text,
elementAttributeIgnoreList: this._config.element_attribute_ignorelist,
// TRICKY: it appears that we were moving to elementsChainAsString, but the UI still depends on elements, so :shrug:
elementsChainAsString: false,
}).props,
$dead_click_scroll_delay_ms: click.scrollDelayMs,
$dead_click_mutation_delay_ms: click.mutationDelayMs,
$dead_click_absolute_delay_ms: click.absoluteDelayMs,
$dead_click_selection_changed_delay_ms: click.selectionChangedDelayMs,
},
{
timestamp: new Date(click.timestamp),
Copy link
Member Author

Choose a reason for hiding this comment

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

turns out this is how you set the timestamp on an event

}
)
}
}

Expand Down
Loading
Loading