feat: lazy load autocapture and heatmaps for faster start and since not everyone has it turned on #2748
Conversation
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
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset (and the release label) is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
a076147 to
31ac90b
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
31ac90b to
94d85c2
Compare
Contributor
Contributor
|
Size Change: -77.1 kB (-1.44%) Total Size: 5.27 MB
ℹ️ View Unchanged
|
pauldambra
force-pushed
the
feat/more-defer
branch
from
470810c to
557fbbc
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
94d85c2 to
cb93a66
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
557fbbc to
dce124e
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
cb93a66 to
1263387
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
1263387 to
f260693
Compare
pauldambra
changed the title
pauldambra
changed the title
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
f260693 to
f3292d5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements lazy loading for autocapture and heatmaps features to reduce initial bundle size and improve startup performance. The changes introduce a queuing mechanism that buffers events until the lazy-loaded modules finish loading, ensuring no events are lost during the startup phase.
Key changes:
- Split autocapture and heatmaps into lightweight wrapper classes and lazy-loaded implementations
- Added event queuing with a 1000-event cap to handle events before lazy modules load
- Reorganized utility functions into shared
dom-event-utils.tsmodule - Updated test infrastructure to support lazy loading pattern
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
packages/browser/src/extensions/autocapture/index.ts |
New wrapper class that queues events and lazy loads the full autocapture implementation |
packages/browser/src/extensions/heatmaps/index.ts |
New wrapper class that queues heatmap events and lazy loads the full heatmaps implementation |
packages/browser/src/entrypoints/autocapture.ts |
Lazy-loaded autocapture implementation moved from main bundle |
packages/browser/src/entrypoints/heatmaps.ts |
Lazy-loaded heatmaps implementation moved from main bundle |
packages/browser/src/extensions/autocapture/external.ts |
Autocapture property extraction functions (previously in autocapture.ts) |
packages/browser/src/extensions/autocapture/autocapture-utils.ts |
Autocapture utility functions with updated imports and re-exports |
packages/browser/src/utils/dom-event-utils.ts |
Shared DOM event utilities extracted for reuse across modules |
packages/browser/src/heatmaps.ts |
Backwards-compatible export wrapper for the new heatmaps module |
packages/browser/src/autocapture.ts |
Removed, replaced by modular structure |
packages/browser/src/__tests__/extensions/autocapture-wrapper.test.ts |
New tests for autocapture queuing behavior |
packages/browser/src/__tests__/heatmaps.test.ts |
Updated to mock lazy loading infrastructure |
packages/browser/src/__tests__/autocapture.test.ts |
Updated imports and test expectations for new lazy loading behavior |
packages/browser/rollup.config.mjs |
Added reserved names for cross-bundle function calls |
packages/browser/terser-mangled-names.json |
Updated mangled names list for new architecture |
playground/nextjs/pages/inp-test.tsx |
New test page for measuring Interaction to Next Paint performance |
playground/nextjs/package.json |
Updated to use local browser package for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+151
to
+152
| if (this._eventQueue.length < MAX_QUEUED_EVENTS) { | ||
| this._eventQueue.push({ event: e, timestamp: Date.now() }) |
There was a problem hiding this comment.
Queuing Event objects could lead to memory leaks if events are not processed quickly. Event objects can hold references to DOM elements and other objects through their target property and event bubbling paths. If many events accumulate in the queue (up to 1000), this could consume significant memory.
Consider extracting only the necessary data from the event (like type, target, timeStamp, clientX/clientY for MouseEvents) and storing that in the queue instead of the full event object. This would reduce memory pressure and prevent potential leaks from long-lived references to DOM elements.
Comment on lines
+131
to
+133
| _onClick(event: MouseEvent, type: string = 'click'): void { | ||
| this._queueEvent(event, type) | ||
| } |
There was a problem hiding this comment.
The method _onClick is prefixed with an underscore suggesting it's private, but it's actually part of the public API (exposed to be called from the wrapper). This inconsistency could cause confusion. Consider either:
- Renaming it to
onClick(without underscore) since it's part of the public interface - Or adding a comment explaining why it needs to be accessible despite the private naming convention
| const maxINP = inpValues.length > 0 ? Math.max(...inpValues).toFixed(2) : 'N/A' | ||
|
|
||
| const p75INP = | ||
| inpValues.length > 0 ? inpValues.sort((a, b) => a - b)[Math.floor(inpValues.length * 0.75)]?.toFixed(2) : 'N/A' |
There was a problem hiding this comment.
The p75 calculation has a bug: calling .sort() on the array mutates the original inpValues array, which affects subsequent calls to this getter. This means that avgINP, maxINP, and p75INP will all work on a sorted array after the first access of p75INP, leading to incorrect results.
Create a copy of the array before sorting using [...inpValues].sort((a, b) => a - b) to avoid mutating the state.
Suggested change
| inpValues.length > 0 ? inpValues.sort((a, b) => a - b)[Math.floor(inpValues.length * 0.75)]?.toFixed(2) : 'N/A' | |
| inpValues.length > 0 ? [...inpValues].sort((a, b) => a - b)[Math.floor(inpValues.length * 0.75)]?.toFixed(2) : 'N/A' |
Comment on lines
+156
to
+168
| private _loadScript(cb: () => void): void { | ||
| if (assignableWindow.__PosthogExtensions__?.initAutocapture) { | ||
| cb() | ||
| return | ||
| } | ||
| assignableWindow.__PosthogExtensions__?.loadExternalDependency?.(this.instance, 'autocapture', (err) => { | ||
| if (err) { | ||
| logger.error('failed to load script', err) | ||
| return | ||
| } | ||
| cb() | ||
| }) | ||
| } |
There was a problem hiding this comment.
If loadExternalDependency fails to load the autocapture script (line 162-164), the callback is never called and the queued events remain in the queue indefinitely. This creates a memory leak and means autocapture events will never be processed.
Consider either:
- Clearing the event queue when script loading fails
- Calling the callback with an error parameter so the caller can handle the failure
- Setting a flag to stop queueing new events after a load failure
Comment on lines
+185
to
+187
| for (const { event, type } of queuedEvents) { | ||
| try { | ||
| this._lazyLoadedHeatmaps._onClick(event, type) |
There was a problem hiding this comment.
The queued event timestamp is not being used when processing events. Unlike the autocapture implementation which preserves timestamps for queued events (line 200 in autocapture/index.ts passes new Date(timestamp) as the third parameter), heatmaps discards this information. This could lead to inaccurate timing data for events that were queued before the lazy-loaded module finished loading.
Consider either:
- Passing the timestamp to the lazy-loaded heatmaps implementation (though _onClick doesn't currently accept a timestamp parameter)
- Or if timestamp preservation is not needed for heatmaps, remove the
timestampfield from theQueuedHeatmapEventinterface (line 24) to avoid confusion.
Comment on lines
+141
to
+142
| if (this._eventQueue.length < MAX_QUEUED_EVENTS) { | ||
| this._eventQueue.push({ event, type, timestamp: Date.now() }) |
There was a problem hiding this comment.
Queuing MouseEvent objects could lead to memory leaks if events are not processed quickly. MouseEvent objects can hold references to DOM elements and other objects through their target property and event bubbling paths. If many events accumulate in the queue (up to 1000), this could consume significant memory.
Consider extracting only the necessary data from the event (like clientX, clientY, target, timeStamp) and storing that in the queue instead of the full event object. This matches the pattern commonly used in similar scenarios where event data needs to be preserved but the event object itself should be released.
Comment on lines
+17
to
+22
| beforeEach(() => { | ||
| assignableWindow.__PosthogExtensions__ = assignableWindow.__PosthogExtensions__ || {} | ||
| assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn().mockImplementation((_ph, _name, cb) => { | ||
| cb() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The mock implementation of loadExternalDependency immediately calls the callback, which means tests won't catch race conditions or timing issues in the event queuing mechanism. Consider adding tests that verify:
- Events are properly queued before the lazy module loads
- Queued events are correctly processed after lazy load completes
- Events go directly to the lazy implementation after it's loaded (bypassing the queue)
- Queue size limits are respected
Comment on lines
+27
to
+193
| export class Heatmaps { | ||
| instance: PostHog | ||
| _enabledServerSide: boolean = false | ||
| _initialized = false | ||
|
|
||
| private _lazyLoadedHeatmaps: LazyLoadedHeatmapsInterface | undefined | ||
| private _eventQueue: QueuedHeatmapEvent[] = [] | ||
| private _onClickHandler: ((e: Event) => void) | undefined | ||
| private _onMouseMoveHandler: ((e: Event) => void) | undefined | ||
|
|
||
| constructor(instance: PostHog) { | ||
| this.instance = instance | ||
| this._enabledServerSide = !!this.instance.persistence?.props[HEATMAPS_ENABLED_SERVER_SIDE] | ||
| } | ||
|
|
||
| public get flushIntervalMilliseconds(): number { | ||
| let flushInterval = DEFAULT_FLUSH_INTERVAL | ||
| if ( | ||
| isObject(this.instance.config.capture_heatmaps) && | ||
| this.instance.config.capture_heatmaps.flush_interval_milliseconds | ||
| ) { | ||
| flushInterval = this.instance.config.capture_heatmaps.flush_interval_milliseconds | ||
| } | ||
| return flushInterval | ||
| } | ||
|
|
||
| public get isEnabled(): boolean { | ||
| if (!isUndefined(this.instance.config.capture_heatmaps)) { | ||
| return this.instance.config.capture_heatmaps !== false | ||
| } | ||
| if (!isUndefined(this.instance.config.enable_heatmaps)) { | ||
| return this.instance.config.enable_heatmaps | ||
| } | ||
| return this._enabledServerSide | ||
| } | ||
|
|
||
| public startIfEnabled(): void { | ||
| if (this.isEnabled) { | ||
| if (this._initialized) { | ||
| return | ||
| } | ||
| this._setupListeners() | ||
| this._initialized = true | ||
|
|
||
| this._loadScript(() => { | ||
| this._start() | ||
| }) | ||
| } else { | ||
| this._removeListeners() | ||
| if (this._lazyLoadedHeatmaps) { | ||
| this._lazyLoadedHeatmaps.stop() | ||
| this._lazyLoadedHeatmaps = undefined | ||
| } | ||
| this._eventQueue = [] | ||
| } | ||
| } | ||
|
|
||
| public onRemoteConfig(response: RemoteConfig): void { | ||
| const optIn = !!response['heatmaps'] | ||
|
|
||
| if (this.instance.persistence) { | ||
| this.instance.persistence.register({ | ||
| [HEATMAPS_ENABLED_SERVER_SIDE]: optIn, | ||
| }) | ||
| } | ||
| this._enabledServerSide = optIn | ||
| this.startIfEnabled() | ||
| } | ||
|
|
||
| public getAndClearBuffer(): HeatmapEventBuffer { | ||
| if (this._lazyLoadedHeatmaps) { | ||
| return this._lazyLoadedHeatmaps.getAndClearBuffer() | ||
| } | ||
| return undefined | ||
| } | ||
|
|
||
| private _setupListeners(): void { | ||
| if (!window || !document) { | ||
| return | ||
| } | ||
|
|
||
| this._onClickHandler = (e) => this._queueEvent((e || window?.event) as MouseEvent, 'click') | ||
| addEventListener(document, 'click', this._onClickHandler, { capture: true }) | ||
|
|
||
| this._onMouseMoveHandler = (e) => this._queueEvent((e || window?.event) as MouseEvent, 'mousemove') | ||
| addEventListener(document, 'mousemove', this._onMouseMoveHandler, { capture: true }) | ||
| } | ||
|
|
||
| private _removeListeners(): void { | ||
| if (!window || !document) { | ||
| return | ||
| } | ||
|
|
||
| if (this._onClickHandler) { | ||
| document.removeEventListener('click', this._onClickHandler, { capture: true }) | ||
| } | ||
|
|
||
| if (this._onMouseMoveHandler) { | ||
| document.removeEventListener('mousemove', this._onMouseMoveHandler, { capture: true }) | ||
| } | ||
|
|
||
| this._initialized = false | ||
| } | ||
|
|
||
| _onClick(event: MouseEvent, type: string = 'click'): void { | ||
| this._queueEvent(event, type) | ||
| } | ||
|
|
||
| private _queueEvent(event: MouseEvent, type: string): void { | ||
| if (this._lazyLoadedHeatmaps) { | ||
| this._lazyLoadedHeatmaps._onClick(event, type) | ||
| return | ||
| } | ||
|
|
||
| if (this._eventQueue.length < MAX_QUEUED_EVENTS) { | ||
| this._eventQueue.push({ event, type, timestamp: Date.now() }) | ||
| } | ||
| } | ||
|
|
||
| private _loadScript(cb: () => void): void { | ||
| if (assignableWindow.__PosthogExtensions__?.initHeatmaps) { | ||
| cb() | ||
| return | ||
| } | ||
| assignableWindow.__PosthogExtensions__?.loadExternalDependency?.(this.instance, 'heatmaps', (err) => { | ||
| if (err) { | ||
| logger.error('failed to load script', err) | ||
| return | ||
| } | ||
| cb() | ||
| }) | ||
| } | ||
|
|
||
| private _start(): void { | ||
| if (!document) { | ||
| logger.error('`document` not found. Cannot start.') | ||
| return | ||
| } | ||
|
|
||
| if (!this._lazyLoadedHeatmaps && assignableWindow.__PosthogExtensions__?.initHeatmaps) { | ||
| this._lazyLoadedHeatmaps = assignableWindow.__PosthogExtensions__.initHeatmaps(this.instance) | ||
| this._lazyLoadedHeatmaps.start() | ||
|
|
||
| this._processQueuedEvents() | ||
| logger.info('started') | ||
| } | ||
| } | ||
|
|
||
| private _processQueuedEvents(): void { | ||
| if (!this._lazyLoadedHeatmaps || this._eventQueue.length === 0) { | ||
| return | ||
| } | ||
|
|
||
| logger.info(`processing ${this._eventQueue.length} queued events`) | ||
|
|
||
| const queuedEvents = this._eventQueue | ||
| this._eventQueue = [] | ||
|
|
||
| for (const { event, type } of queuedEvents) { | ||
| try { | ||
| this._lazyLoadedHeatmaps._onClick(event, type) | ||
| } catch (error) { | ||
| logger.error('Failed to process queued event', error) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The event queuing behavior in the heatmaps wrapper class lacks dedicated test coverage. Similar to autocapture which has autocapture-wrapper.test.ts, consider adding heatmaps-wrapper.test.ts to verify:
- Events are properly queued before lazy load completes
- Queued events are correctly processed after lazy load
- Queue size limits (MAX_QUEUED_EVENTS) are respected
- Direct delegation happens after lazy load
Comment on lines
+146
to
+158
| private _loadScript(cb: () => void): void { | ||
| if (assignableWindow.__PosthogExtensions__?.initHeatmaps) { | ||
| cb() | ||
| return | ||
| } | ||
| assignableWindow.__PosthogExtensions__?.loadExternalDependency?.(this.instance, 'heatmaps', (err) => { | ||
| if (err) { | ||
| logger.error('failed to load script', err) | ||
| return | ||
| } | ||
| cb() | ||
| }) | ||
| } |
There was a problem hiding this comment.
If loadExternalDependency fails to load the heatmaps script (line 152-154), the callback is never called and the queued events remain in the queue indefinitely. This creates a memory leak and means heatmap events will never be processed.
Consider either:
- Clearing the event queue when script loading fails
- Calling the callback with an error parameter so the caller can handle the failure
- Setting a flag to stop queueing new events after a load failure
pauldambra
force-pushed
the
feat/more-defer
branch
from
dce124e to
eea8cc5
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
f3292d5 to
670d8bc
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
eea8cc5 to
b72e302
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
670d8bc to
104ff9e
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
104ff9e to
8b37d23
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
b72e302 to
4825227
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
4825227 to
93772f8
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
8b37d23 to
e088b76
Compare
pauldambra
force-pushed
the
feat/more-defer
branch
from
93772f8 to
fa86582
Compare
pauldambra
force-pushed
the
feat/lazy-autocap
branch
from
e088b76 to
0f0727f
Compare
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
we can queue events
and lazy load autocapture
to make library startup cheaper
and bundle smaller