refactor: remove any Zen specific variable naming #1392
Conversation
…tude-TypeScript into no-ticket-zen-observable-dead-click
…tude-TypeScript into no-ticket-zen-observable-dead-click
…tude-TypeScript into no-ticket-zen-observable-dead-click
Co-authored-by: Copilot <[email protected]>
…ack-dead-click.test.ts Co-authored-by: Copilot <[email protected]>
…ack-dead-click.test.ts Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to remove references to "Zen Observable" naming conventions and consolidates the observable implementation. The changes eliminate the dual observable system (RxJS and Zen Observable) by standardizing on the custom Observable implementation from @amplitude/analytics-core.
- Renamed variables removing "Zen" suffix (e.g.,
clickObservableZen→clickObservable) - Removed RxJS dependency from multiple packages
- Refactored click tracking logic to use simpler observable patterns instead of complex RxJS operators
- Enhanced deprecated package checking script to prevent code changes in deprecated directories
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
packages/plugin-autocapture-browser/src/observables.ts |
Removed RxJS imports and consolidated to single Observable implementation, added document.body check for mutation observer |
packages/plugin-autocapture-browser/src/frustration-plugin.ts |
Updated function calls to use renamed observable creation functions without "Zen" suffix |
packages/plugin-autocapture-browser/src/autocapture/track-click.ts |
Simplified click tracking by removing complex RxJS operators (buffer, debounce, pairwise) in favor of simpler observable filtering |
packages/plugin-autocapture-browser/src/autocapture/track-action-click.ts |
Replaced RxJS operators (switchMap, timeout) with custom asyncMap and setTimeout-based logic for action click detection |
packages/plugin-autocapture-browser/src/autocapture-plugin.ts |
Updated observable creation to use Observable from analytics-core, removed RxJS dependencies, updated type definitions |
packages/plugin-autocapture-browser/package.json |
Removed rxjs dependency |
packages/plugin-web-vitals-browser/package.json |
Removed rxjs dependency |
packages/plugin-stub-browser/package.json |
Removed rxjs dependency |
packages/plugin-autocapture-browser/test/autocapture-plugin/track-dead-click.test.ts |
Renamed test variables from "ObservableZen" to "Observable" for consistency |
packages/plugin-autocapture-browser/test/autocapture-plugin/track-action-clicks.test.ts |
Re-enabled navigation event test with mock navigation API implementation |
packages/analytics-core/src/types/element-interactions.ts |
Deprecated debounceTime option with documentation explaining it's replaced by frustrationInteractions |
packages/analytics-core/src/types/client/browser-client.ts |
Made _setDiagnosticsSampleRate method optional |
scripts/check-deprecated-packages.sh |
Enhanced to check for code changes in deprecated package directories, not just dependency additions |
test-server/autocapture/element-interactions.html |
Added div-based content changer and configured debounceTime in test configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { filter, map, merge, switchMap, take, timeout, EMPTY } from 'rxjs'; | ||
| import { BrowserClient, ActionType } from '@amplitude/analytics-core'; | ||
| import { AllWindowObservables, AutoCaptureOptionsWithDefaults } from 'src/autocapture-plugin'; | ||
| import { BrowserClient, ActionType, merge, asyncMap } from '@amplitude/analytics-core'; |
There was a problem hiding this comment.
The asyncMap function is being used but it's not clear if this is exported from @amplitude/analytics-core. Please verify that asyncMap is properly exported and available for import from this package.
| let actionClickTimer: ReturnType<typeof setTimeout> | null = null; | ||
| let lastClickEvent: TimestampedEvent<any> | null = null; | ||
|
|
||
| const actionClickObservable = asyncMap(clickMutationNavigateObservable, (event) => { | ||
| // clear any previous timer | ||
| if (actionClickTimer) { | ||
| clearTimeout(actionClickTimer); | ||
| actionClickTimer = null; | ||
| } | ||
| if (event.type === 'click') { | ||
| // mark the 'last click event' | ||
| lastClickEvent = event; | ||
|
|
||
| // set a timer to clear last click event if no mutation event between now and 500ms | ||
| actionClickTimer = setTimeout(() => { | ||
| actionClickTimer = null; | ||
| lastClickEvent = null; | ||
| return null; | ||
| }, 500); | ||
| return Promise.resolve(null); | ||
| } else { | ||
| // if mutation/navigation + last click event, then it's an action click | ||
| if (lastClickEvent) { | ||
| const event = lastClickEvent; | ||
| lastClickEvent = null; | ||
| return Promise.resolve(event); | ||
| } | ||
| } | ||
| return Promise.resolve(null); | ||
| }); |
There was a problem hiding this comment.
The timeout timer is never cleared on teardown. If the observable is unsubscribed while the timer is active, it will continue to run and try to modify actionClickTimer and lastClickEvent after cleanup. Consider tracking the timer in the subscription cleanup logic to ensure it's cleared when the observable is unsubscribed.
packages/plugin-autocapture-browser/src/autocapture/track-action-click.ts
Show resolved
Hide resolved
| actionClickTimer = setTimeout(() => { | ||
| actionClickTimer = null; | ||
| lastClickEvent = null; | ||
| return null; |
There was a problem hiding this comment.
The setTimeout callback returns null but this value is not captured or used. The return statement inside the setTimeout callback has no effect since it only returns from the callback function, not from the outer function. Consider removing the return null; statement as it serves no purpose.
| return null; |
| 'click', | ||
| (options as AutoCaptureOptionsWithDefaults).cssSelectorAllowlist, | ||
| dataAttributePrefix, | ||
| ) as ElementBasedTimestampedEvent<MouseEvent>, |
There was a problem hiding this comment.
[nitpick] The type cast as ElementBasedTimestampedEvent<MouseEvent> may be unsafe. Verify that addAdditionalEventProperties returns the correct type and consider updating the function's return type signature instead of using a type assertion.
daniel-graham-amplitude
merged commit 2ff771a
into
zen-observable-migration
Summary
Removes references to the underlying library "Zen Observable".
Checklist