fix(analytics): update storage handling during keepalive flush to prevent duplicate event sends#34877
Conversation
…vent duplicate event sends
Contributor
|
Claude finished @oidacra's task —— View job PR Review: Analytics Storage Handling FixReviewing changes to prevent duplicate analytics event sends during keepalive flushes. Todo List:
Review Analysis:The fix is technically sound and addresses the duplicate event issue correctly. Key Changes Validated:
Technical Assessment:
No Significant Issues Found:
The PR cleanly fixes the duplicate analytics events issue without introducing new risks. |
Closed
6 tasks
github-actions
Bot
added
Area : Frontend
nicobytes
approved these changes
Mar 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes duplicate analytics event re-sends by ensuring the persisted queue in sessionStorage is updated/cleared even when flushing with keepalive during page unload.
Changes:
- Removed the
keepalive-only guard so storage is updated after every batch send. - Updated the keepalive-related unit test to expect storage clearing and documented the reasoning.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core-web/libs/sdk/analytics/src/lib/core/shared/queue/dot-analytics.queue.utils.ts | Always clears/persists sessionStorage after batching to prevent stale events from being replayed on next load. |
| core-web/libs/sdk/analytics/src/lib/core/shared/queue/dot-analytics.queue.utils.spec.ts | Aligns keepalive flush test with the new behavior (storage cleared) to prevent duplicates. |
You can also share your feedback on Copilot code review. Take the survey.
adrianjm-dotCMS
approved these changes
Mar 5, 2026
- Clarify sendBatch comment to reflect fire-and-forget nature of sendAnalyticsEvent - Strengthen test assertions: use toHaveBeenCalledTimes(1) instead of toHaveBeenCalled() - Clear spy before flush in normal flush test to isolate the assertion
Contributor
|
Claude finished @oidacra's task —— View job PR Review: Analytics Storage Handling FixReviewing changes to prevent duplicate analytics event sends during keepalive flushes. Todo List:
Review Analysis:The fix is technically sound and addresses the duplicate event issue correctly. Key Changes Validated:
Technical Assessment:
No Significant Issues Found:
The PR cleanly fixes the duplicate analytics events issue without introducing new risks. |
spbolton
pushed a commit
that referenced
this pull request
Mar 24, 2026
…vent duplicate event sends (#34877) PR Description: ## Summary Fixes duplicate analytics event sends caused by keepalive flushes (page unload) skipping the storage update. The stale events left in `sessionStorage` were re-sent on the next page load. ## Changes Made ### Frontend (SDK) - **`dot-analytics.queue.utils.ts`**: Removed the `if (!keepalive)` guard around storage cleanup after batch sends. Storage is now always updated after flushing — both for normal and keepalive flushes — since `sessionStorage` writes are synchronous and complete before page unload. - **`dot-analytics.queue.utils.spec.ts`**: Updated the keepalive flush test to assert that storage **is** cleared (previously asserted it was not), with comments explaining why. ## Technical Details The previous implementation treated keepalive flushes (triggered during `visibilitychange`/`beforeunload`) as a special case: it intentionally left sent events in `sessionStorage` as a "backup" in case the beacon request failed. However, because `sessionStorage.removeItem()` is synchronous and guaranteed to complete before the page unloads, this backup was unnecessary. Worse, it caused the next page load to pick up the stale persisted events and re-send them, resulting in duplicate analytics data. The fix removes the keepalive branch so that `clearStorage()` / `persistToStorage()` always runs after a successful batch send, regardless of the flush trigger. ## Breaking Changes None ## Testing - [x] Unit tests updated - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] E2E tests added/updated ## Related Issues Closes #34357 ## Additional Notes None
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
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.
PR Description:
Summary
Fixes duplicate analytics event sends caused by keepalive flushes (page unload) skipping the storage update. The stale events left in
sessionStoragewere re-sent on the nextpage load.
Changes Made
Frontend (SDK)
dot-analytics.queue.utils.ts: Removed theif (!keepalive)guard around storage cleanup after batch sends. Storage is now always updated after flushing — both fornormal and keepalive flushes — since
sessionStoragewrites are synchronous and complete before page unload.dot-analytics.queue.utils.spec.ts: Updated the keepalive flush test to assert that storage is cleared (previously asserted it was not), with comments explaining why.Technical Details
The previous implementation treated keepalive flushes (triggered during
visibilitychange/beforeunload) as a special case: it intentionally left sent events insessionStorageas a "backup" in case the beacon request failed. However, becausesessionStorage.removeItem()is synchronous and guaranteed to complete before the pageunloads, this backup was unnecessary. Worse, it caused the next page load to pick up the stale persisted events and re-send them, resulting in duplicate analytics data.
The fix removes the keepalive branch so that
clearStorage()/persistToStorage()always runs after a successful batch send, regardless of the flush trigger.Breaking Changes
None
Testing
Related Issues
Closes #34357
Additional Notes
None
This PR fixes: #34357