Conversation
Add zoning alpha by implementing exposure and scroll tracking with
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| /* istanbul ignore next */ | ||
| amplitude?.track('[Amplitude] Viewport Content Updated', eventProperties); | ||
| lastScroll = { maxX: pageScrollMaxState.maxX, maxY: pageScrollMaxState.maxY }; |
There was a problem hiding this comment.
🟡 Medium
packages/plugin-autocapture-browser/src/autocapture/track-viewport-content-updated.ts:66
Reassigning lastScroll on line 66 only updates the local parameter, not the caller's object. Consider mutating the existing object instead (e.g., lastScroll.maxX = ...; lastScroll.maxY = ...) so the caller sees the updated values.
| lastScroll = { maxX: pageScrollMaxState.maxX, maxY: pageScrollMaxState.maxY }; | |
| lastScroll.maxX = pageScrollMaxState.maxX; | |
| lastScroll.maxY = pageScrollMaxState.maxY; |
🚀 Want me to fix this? Reply ex: "fix it for me".
| exposureDuration?: number; | ||
| }) { | ||
| // Track which elements have been marked as exposed (per-element state) | ||
| const exposureMap = new Map<Element, boolean>(); |
There was a problem hiding this comment.
🟢 Low
packages/plugin-autocapture-browser/src/autocapture/track-exposure.ts:20
Using Map<Element, ...> holds strong references to DOM elements, preventing garbage collection even after elements are removed from the DOM. Consider using WeakMap for exposureMap and exposureTimerMap to allow automatic cleanup when elements are garbage collected.
🚀 Want me to fix this? Reply ex: "fix it for me".
|
|
||
| const scrollObservable = createScrollObservable(); | ||
|
|
||
| const exposureObservable = createExposureObservable( |
There was a problem hiding this comment.
🟡 Medium
packages/plugin-autocapture-browser/src/autocapture-plugin.ts:211
Passing an empty cssSelectorAllowlist causes createExposureObservable to call querySelectorAll(""), which throws a DOMException. Consider adding a guard in createExposureObservable to skip observation when the selector list is empty.
🚀 Want me to fix this? Reply ex: "fix it for me".
| .then(() => { | ||
| this.logger?.debug?.('Background capture script loaded (external)'); | ||
| // eslint-disable-next-line | ||
| amplitudeBackgroundCaptureInstance = (window as any)?.amplitudeBackgroundCapture?.({ |
There was a problem hiding this comment.
🟢 Low
packages/plugin-autocapture-browser/src/libs/messenger.ts:236
Consider closing the existing amplitudeBackgroundCaptureInstance before assigning a new one to prevent resource leaks if initialize-background-capture is called multiple times.
| amplitudeBackgroundCaptureInstance = (window as any)?.amplitudeBackgroundCapture?.({ | |
| amplitudeBackgroundCaptureInstance?.close?.(); | |
| amplitudeBackgroundCaptureInstance = (window as any)?.amplitudeBackgroundCapture?.({ |
🚀 Want me to fix this? Reply ex: "fix it for me".
| const exposedString = JSON.stringify(exposedArray); | ||
|
|
||
| if (exposedString.length >= constants.MAX_ELEMENT_EXPOSED_STR_LENGTH) { | ||
| fireViewportContentUpdatedCallback(false); |
There was a problem hiding this comment.
🟡 Medium
packages/plugin-autocapture-browser/src/autocapture/track-viewport-content-updated.ts:96
Calling fireViewportContentUpdatedCallback(false) here sets pageViewEndFired = true in the callback. If a real page-end event occurs within 100ms, it gets skipped, preventing elementExposedForPage.clear(). Consider either not setting the debounce flag for non-page-end flushes, or having onExposure batch without triggering the debounce mechanism.
🚀 Want me to fix this? Reply ex: "fix it for me".
| unsubscribe: () => { | ||
| exposureSubscription.unsubscribe(); | ||
| }, |
There was a problem hiding this comment.
🟢 Low
packages/plugin-autocapture-browser/src/autocapture/track-exposure.ts:59
The unsubscribe method doesn't clear pending timers, so onExposure callbacks may fire after teardown. Consider reusing the timer-clearing logic from reset in unsubscribe.
| unsubscribe: () => { | |
| exposureSubscription.unsubscribe(); | |
| }, | |
| unsubscribe: () => { | |
| exposureTimerMap.forEach((timer) => { | |
| if (timer) { | |
| clearTimeout(timer); | |
| } | |
| }); | |
| exposureTimerMap.clear(); | |
| exposureSubscription.unsubscribe(); | |
| }, |
🚀 Want me to fix this? Reply ex: "fix it for me".
- @amplitude/analytics-browser@2.34.1-feat-zoning-alpha.0 - @amplitude/analytics-client-common@2.4.25-feat-zoning-alpha.0 - @amplitude/analytics-core@2.38.0-feat-zoning-alpha.0 - @amplitude/analytics-node@1.5.35-feat-zoning-alpha.0 - @amplitude/analytics-react-native@1.5.38-feat-zoning-alpha.0 - @amplitude/analytics-types@2.12.0-feat-zoning-alpha.0 - @amplitude/gtm-snippet@2.34.1-feat-zoning-alpha.0 - @amplitude/plugin-autocapture-browser@1.20.0-feat-zoning-alpha.0 - @amplitude/plugin-experiment-browser@1.0.0-feat-zoning-alpha.0 - @amplitude/plugin-global-user-properties@1.2.116-feat-zoning-alpha.0 - @amplitude/plugin-network-capture-browser@1.7.9-feat-zoning-alpha.0 - @amplitude/plugin-page-url-enrichment-browser@0.5.15-feat-zoning-alpha.0 - @amplitude/plugin-page-view-tracking-browser@2.7.0-feat-zoning-alpha.0 - @amplitude/plugin-session-replay-browser@1.25.11-feat-zoning-alpha.0 - @amplitude/plugin-session-replay-react-native@0.4.7-feat-zoning-alpha.0 - @amplitude/plugin-web-attribution-browser@2.1.109-feat-zoning-alpha.0 - @amplitude/plugin-web-vitals-browser@1.1.10-feat-zoning-alpha.0 - @amplitude/segment-session-replay-plugin@0.0.0-feat-zoning-alpha.0 - @amplitude/session-replay-browser@1.30.10-feat-zoning-alpha.0 - @amplitude/unified@1.0.0-feat-zoning-alpha.0
4 participants
Summary
Checklist