-
-
Notifications
You must be signed in to change notification settings - Fork 359
feat(core): Add rage tap detection with ui.frustration breadcrumbs #5992
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
Changes from 7 commits
7d06010
89d1e9d
a34b7f2
12862ac
65fe114
b43a3e8
1eb262c
234b1c6
7cbafd3
ab099f2
caee29c
a8c6294
dd2b845
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,9 @@ public final class RNSentryReplayBreadcrumbConverter extends DefaultReplayBreadc | |
| if ("touch".equals(breadcrumb.getCategory())) { | ||
| return convertTouchBreadcrumb(breadcrumb); | ||
| } | ||
| if ("ui.multiClick".equals(breadcrumb.getCategory())) { | ||
| return convertMultiClickBreadcrumb(breadcrumb); | ||
| } | ||
| if ("navigation".equals(breadcrumb.getCategory())) { | ||
| return convertNavigationBreadcrumb(breadcrumb); | ||
| } | ||
|
|
@@ -72,6 +75,22 @@ public final class RNSentryReplayBreadcrumbConverter extends DefaultReplayBreadc | |
| return rrWebBreadcrumb; | ||
| } | ||
|
|
||
| @TestOnly | ||
| public @Nullable RRWebEvent convertMultiClickBreadcrumb(final @NotNull Breadcrumb breadcrumb) { | ||
| if (breadcrumb.getData("path") == null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: Should we also check the type similar to iOS? |
||
| return null; | ||
| } | ||
|
|
||
| final RRWebBreadcrumbEvent rrWebBreadcrumb = new RRWebBreadcrumbEvent(); | ||
|
|
||
| rrWebBreadcrumb.setCategory("ui.multiClick"); | ||
|
|
||
| rrWebBreadcrumb.setMessage(getTouchPathMessage(breadcrumb.getData("path"))); | ||
|
|
||
| setRRWebEventDefaultsFrom(rrWebBreadcrumb, breadcrumb); | ||
| return rrWebBreadcrumb; | ||
| } | ||
|
|
||
| @TestOnly | ||
| public static @Nullable String getTouchPathMessage(final @Nullable Object maybePath) { | ||
| if (!(maybePath instanceof List)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| import type { SeverityLevel } from '@sentry/core'; | ||
| import { addBreadcrumb, debug } from '@sentry/core'; | ||
|
|
||
| import { getCurrentReactNativeTracingIntegration } from './tracing/reactnativetracing'; | ||
|
|
||
| const DEFAULT_RAGE_TAP_THRESHOLD = 3; | ||
| const DEFAULT_RAGE_TAP_TIME_WINDOW = 1000; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Wdyt of increasing this to 7s like the web?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm curious how this works for the case of app hanging (i.e. dead clicks) -- I'm guessing we won't be able to detect the following touches after the first one that triggered a hang -- the main thread would be occupied/congested, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @romtsn that's the correct assumption β rage taps (rapid taps that register) and dead taps (taps during a hang) are different signals. Dead tap detection would need native-side work.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @antonis the web SDK's 7s is more like "how long to wait before emitting the breadcrumb", not "how close clicks need to be". So the direct comparison of numbers doesn't seem applicable βΒ in our case it's like a "rolling" time window.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good π Let's add a comment explaining the reasoning behind the 1s window if possible π |
||
|
|
||
| export interface TouchedComponentInfo { | ||
| name?: string; | ||
| label?: string; | ||
| element?: string; | ||
| file?: string; | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| export interface RageTapDetectorOptions { | ||
| enabled: boolean; | ||
| threshold: number; | ||
| timeWindow: number; | ||
| } | ||
|
|
||
| interface RecentTap { | ||
| identity: string; | ||
| timestamp: number; | ||
| } | ||
|
|
||
| /** | ||
| * Detects rage taps (repeated rapid taps on the same target) and emits | ||
| * `ui.multiClick` breadcrumbs when the threshold is hit. | ||
| * | ||
| * Uses the same breadcrumb category and data shape as the web JS SDK's | ||
| * rage click detection so the Sentry replay timeline renders the fire | ||
| * icon and "Rage Click" label automatically. | ||
| */ | ||
| export class RageTapDetector { | ||
| private _recentTaps: RecentTap[] = []; | ||
| private _enabled: boolean; | ||
| private _threshold: number; | ||
| private _timeWindow: number; | ||
|
|
||
| public constructor(options?: Partial<RageTapDetectorOptions>) { | ||
| this._enabled = options?.enabled ?? true; | ||
| this._threshold = options?.threshold ?? DEFAULT_RAGE_TAP_THRESHOLD; | ||
| this._timeWindow = options?.timeWindow ?? DEFAULT_RAGE_TAP_TIME_WINDOW; | ||
| } | ||
|
|
||
| /** | ||
| * Update options at runtime (e.g. when React props change). | ||
| */ | ||
| public updateOptions(options: Partial<RageTapDetectorOptions>): void { | ||
|
alwx marked this conversation as resolved.
|
||
| if (options.enabled !== undefined) { | ||
| this._enabled = options.enabled; | ||
| } | ||
| if (options.threshold !== undefined) { | ||
|
sentry[bot] marked this conversation as resolved.
|
||
| this._threshold = options.threshold; | ||
| } | ||
| if (options.timeWindow !== undefined) { | ||
| this._timeWindow = options.timeWindow; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Call after each touch event. If a rage tap is detected, a `ui.multiClick` | ||
| * breadcrumb is emitted automatically. | ||
| */ | ||
| public check(touchPath: TouchedComponentInfo[], label?: string): void { | ||
| if (!this._enabled) { | ||
| return; | ||
| } | ||
|
|
||
| const root = touchPath[0]; | ||
| if (!root) { | ||
| return; | ||
| } | ||
|
|
||
| const identity = getTapIdentity(root, label); | ||
| const now = Date.now(); | ||
| const tapCount = this._detect(identity, now); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent breadcrumb category: code emits 'ui.multiClick' but PR description and type say 'ui.frustration' The PR title and description state that this feature emits VerificationCompared the PR title/description ('ui.frustration breadcrumbs') against the literal category strings in ragetap.ts at lines 30 (JSDoc), 60 (JSDoc), and 81 (addBreadcrumb call). All three use 'ui.multiClick'. No other category is emitted in this file. Identified by Warden |
||
| if (tapCount > 0) { | ||
| const message = buildTouchMessage(root, label); | ||
| const node = buildNodeFromTouchPath(root, label); | ||
|
|
||
| addBreadcrumb({ | ||
| category: 'ui.multiClick', | ||
| type: 'default', | ||
| level: 'warning' as SeverityLevel, | ||
|
antonis marked this conversation as resolved.
Outdated
|
||
| message, | ||
| data: { | ||
| clickCount: tapCount, | ||
| metric: true, | ||
| route: getCurrentRoute(), | ||
| node, | ||
| path: touchPath, | ||
| }, | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rage tap breadcrumb missing
|
||
|
|
||
| debug.log(`[TouchEvents] Rage tap detected: ${tapCount} taps on ${message}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the tap count if rage tap is detected, 0 otherwise. | ||
| */ | ||
| private _detect(identity: string, now: number): number { | ||
| // If the target changed, reset the buffer β only truly consecutive | ||
| // taps on the same target count. This prevents false positives where | ||
| // time-window pruning removes interleaved taps on other targets. | ||
| const lastTap = this._recentTaps[this._recentTaps.length - 1]; | ||
| if (lastTap && lastTap.identity !== identity) { | ||
| this._recentTaps = []; | ||
| } | ||
|
|
||
| this._recentTaps.push({ identity, timestamp: now }); | ||
|
|
||
| // Prune taps outside the time window | ||
| const cutoff = now - this._timeWindow; | ||
| this._recentTaps = this._recentTaps.filter(tap => tap.timestamp >= cutoff); | ||
|
|
||
| if (this._recentTaps.length >= this._threshold) { | ||
| const count = this._recentTaps.length; | ||
| this._recentTaps = []; | ||
| return count; | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| function getTapIdentity(root: TouchedComponentInfo, label?: string): string { | ||
| const base = `name:${root.name ?? ''}|file:${root.file ?? ''}`; | ||
| if (label) { | ||
| return `label:${label}|${base}`; | ||
| } | ||
| return base; | ||
| } | ||
|
|
||
| /** | ||
| * Build a human-readable message matching the touch breadcrumb format. | ||
| */ | ||
| function buildTouchMessage(root: TouchedComponentInfo, label?: string): string { | ||
| if (label) { | ||
| return label; | ||
| } | ||
| return `${root.name}${root.file ? ` (${root.file})` : ''}`; | ||
| } | ||
|
|
||
| /** | ||
| * Build a node object compatible with the web SDK's `ReplayBaseDomFrameData` | ||
| * so that `stringifyNodeAttributes` in the Sentry frontend can render it. | ||
| * | ||
| * Maps the React Native component info to the DOM-like shape: | ||
| * - `tagName` β element type (e.g. "RCTView") or component name | ||
| * - `attributes['data-sentry-component']` β component name from babel plugin | ||
| * - `attributes['data-sentry-source-file']` β source file | ||
| */ | ||
| function buildNodeFromTouchPath( | ||
| root: TouchedComponentInfo, | ||
| label?: string, | ||
| ): { id: number; tagName: string; textContent: string; attributes: Record<string, string> } { | ||
| const attributes: Record<string, string> = {}; | ||
|
|
||
| if (root.name) { | ||
| attributes['data-sentry-component'] = root.name; | ||
| } | ||
| if (root.file) { | ||
| attributes['data-sentry-source-file'] = root.file; | ||
| } | ||
| if (label) { | ||
| attributes['sentry-label'] = label; | ||
| } | ||
|
|
||
| return { | ||
| id: 0, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: why do we pass
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Web SDK uses actual rrweb node IDs for DOM element mapping. On mobile we don't have rrweb node IDs so 0 is no more than just a placeholder (and
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good π We should double check that this get's through on the backend |
||
| tagName: root.element ?? root.name ?? 'unknown', | ||
| textContent: '', | ||
| attributes, | ||
| }; | ||
| } | ||
|
|
||
| function getCurrentRoute(): string | undefined { | ||
| try { | ||
| return getCurrentReactNativeTracingIntegration()?.state.currentRoute; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } | ||
|
sentry[bot] marked this conversation as resolved.
|
||


Uh oh!
There was an error while loading. Please reload this page.