[sui-segment-wrapper] revert to v4.36 and safari fix#1982
Conversation
tomasmax
left a comment
There was a problem hiding this comment.
PR Review: [sui-segment-wrapper] revert to v4.36 and safari fix
Reviewed by 4 specialized agents: code quality, test coverage, error handling, and comment accuracy.
Critical Issues (3)
1. isFirstPageViewSent — untestable, untested, core ITP protection has zero coverage
src/middlewares/source/pageReferrer.js:23
isFirstPageViewSent is a module-level let that flips to true permanently on first page event. It is not exported and has no reset mechanism, so resetReferrerState() in stubs cannot clear it.
The test at segmentWrapperSpec.js:1053 acknowledges this:
// Note: Due to test isolation, isFirstPageViewSent might already be true from other tests
// So we'll just verify the mechanism existsThen it only asserts expect(INITIAL_SEARCH_STRING).to.be.a('string') — a type check that passes even if the entire if (isPageTrack && !isFirstPageViewSent) block is deleted. No test verifies that url and search are actually injected into the first page event payload.
Suggestion: Move isFirstPageViewSent into referrerState (or export a reset function), add it to resetReferrerState(), and write a test that asserts the first page event contains url: INITIAL_URL and search: INITIAL_SEARCH_STRING.
2. getTrackIntegrations catch block swallows all GA4 errors silently
src/segmentWrapper.js:60-65
try {
sessionId = await getGoogleSessionId()
clientId = await getGoogleClientId()
} catch (error) {
console.error(error)
}This broad catch with only console.error:
- No Sentry/monitoring — failures here are invisible to the team in production
- Catches unrelated errors —
TypeError,DOMException(localStorage in private browsing), future programming errors — all silently swallowed triggerGoogleAnalyticsInitEventis called insidegetGoogleSessionIdand itself toucheslocalStorage,window.gtag, anddispatchEvent— any of those can throw and get caught here
Suggestion: At minimum, add a descriptive message: console.error('[segment-wrapper] Failed to retrieve GA4 session/client IDs. Events will be sent without session attribution.', error)
3. GA4 true fallback + undefined sessionId flows into triggerGoogleAnalyticsInitEvent
src/segmentWrapper.js:72-79 + src/repositories/googleRepository.js:190
The change from false to true when sessionId is unavailable:
'Google Analytics 4': clientId && sessionId ? { clientId, sessionId } : trueThis means events are sent to GA4 without session attribution, causing inflated session counts. Additionally, getGoogleSessionId calls triggerGoogleAnalyticsInitEvent(sessionId) before checking if sessionId is defined. When gtag returns undefined, this writes "ga_event_sui_undefined" to localStorage and fires a "sui" GA4 event with no valid session.
Suggestion: Guard the init event:
export const getGoogleSessionId = async () => {
const sessionId = await getGoogleField(FIELDS.sessionId)
if (sessionId) triggerGoogleAnalyticsInitEvent(sessionId)
return sessionId
}Important Issues (5)
4. Safari ITP tests assert types, not behavior
test/segmentWrapperSpec.js:995-1063
All three Safari ITP Protection tests reduce to expect(X).to.be.a('string'). They would pass even if the ITP protection code were deleted. The getActualQueryString Branch C — the actual Safari ITP scenario where Safari stripped params after load — has no coverage.
5. getActualQueryString conflates ITP protection with SPA navigation
src/middlewares/source/pageReferrer.js:43-49
if (typeof window !== 'undefined' && window.location.search && window.location.search !== INITIAL_SEARCH_STRING) {
return window.location.search
}
return INITIAL_SEARCH_STRINGComment says it handles pushState in tests, but in production this means: if the user navigates via SPA to a URL with different query params, the live (potentially ITP-stripped) value is returned. The JSDoc says "captured at module load (protected from Safari ITP)" but the branching logic contradicts that in several paths.
6. getDocumentReferrer is now dead code in production
src/middlewares/source/pageReferrer.js:32
utils.getDocumentReferrer is still declared but no longer called in any production code path (the || utils.getDocumentReferrer() fallback was removed from getPageReferrer). It only survives because stubs.js stubs it — but the stub has no effect on live behavior since referrer now comes from referrerState.referrer. This creates a trap: test readers will assume getDocumentReferrer is part of the referrer resolution chain, when it isn't.
7. rt medium mapping fix has no regression test
src/repositories/googleRepository.js:41
rt: 'display' → rt: 'retargeting' is a real bug fix but no test covers STC strings with the rt medium prefix. It could silently regress.
8. loadGoogleAnalytics comment is stale
src/repositories/googleRepository.js:70
Comment says // Load it and retrieve the 'clientId' from Google but the function only calls loadScript(). clientId is retrieved separately by getGoogleClientId(). This is misleading for future maintainers.
Suggestions (3)
9. universalId catch block drops original error
src/index.js:25-29 — catch (e) is caught but never included in console.error. Add e as second argument.
10. Adobe @deprecated stubs use // comments instead of JSDoc
src/index.js:85-89 — IDE tooling and TypeScript read deprecation from /** @deprecated */ blocks, not // line comments. Consider proper JSDoc with @returns type info.
11. utils.getDocumentReferrer and utils.getActualLocation unguarded for SSR
src/middlewares/source/pageReferrer.js:32-38 — The module-level constants are correctly guarded with typeof document/window !== 'undefined', but these two utils methods access document/window directly. The isClient guard in index.js prevents middleware registration in SSR, but getCampaignDetails in googleRepository.js imports utils directly and could be called in SSR contexts.
Strengths
- Revert is clean — no leftover GA4 cookie references, all related code/tests properly removed
- Adobe removal backward compat is correct — stubs return same Promise shapes as originals
INITIAL_DOCUMENT_REFERRER/INITIAL_SEARCH_STRINGJSDoc blocks are well-written — document both mechanism and business motivation (Safari ITP), which is exactly the "why" that helps future maintainersgetPageReferrerinline comments correctly describe the branching logic and capture the non-obvious invariant that tracks can arrive before the first page event
Recommended Action
- Fix critical #1 — make
isFirstPageViewSentresettable + add real behavioral tests for the ITP first-page-event protection - Fix critical #3 — guard
triggerGoogleAnalyticsInitEventagainstundefinedsessionId - Add regression test for
rt: 'retargeting'fix - Consider the other suggestions as follow-up improvements
🤖 Generated with Claude Code
Response to ReviewThank you for the detailed review! We've addressed several issues while maintaining behavioral compatibility with v4.36.0: Applied ChangesCritical Issue #1 - isFirstPageViewSent testability:
Critical Issue #2 - Error message improvement:
Important Issue #7 - Retargeting regression test:
Not Applied (Intentional)Critical Issue #3 - Guard for undefined sessionId:
Result
The other suggestions (Adobe JSDoc, SSR guards, dead code cleanup) are valuable but outside the scope of this revert+hotfix PR. |
… rt medium regression test
Summary
Reverts segment-wrapper to version 4.36.0, applies Safari ITP fixes, and removes Adobe Analytics integration (no longer used).
Changes
Commit 1: Revert to 4.36.0
Commit 2: Safari ITP Fix
Problem: Safari ITP causes analytics attribution data loss by:
document.referrerafter page loadSolution: Captures critical values at module load time before Safari ITP can clear them:
pageReferrer.js:
INITIAL_DOCUMENT_REFERRER- captured at module loadINITIAL_SEARCH_STRING- captured before parameter strippingINITIAL_URL- captured for first page eventreferrerState.referrerwith captured valuegoogleRepository.js:
rt: 'display'→rt: 'retargeting'test/stubs.js - Updated test stubs for referrer state management
test/segmentWrapperSpec.js - Added Safari ITP protection tests
Commit 3: Remove Adobe Analytics
Backwards Compatible Removal:
src/repositories/adobeRepository.js(117 lines)src/scripts/adobeVisitorApi.js(large minified file ~12KB)segmentWrapper.jsgetAdobeVisitorData()andgetAdobeMCVisitorID()inindex.jsthat return empty promises (prevents breaking existing imports)Testing