Update Consent Storage #992
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the consent storage mechanism to store consent data using the internal processing format (ConsentData with BooleanFlag values) instead of the external API format (ConsentState with string values). This eliminates redundant conversions and simplifies the consent tracking flow.
Key Changes:
- Changed internal storage type from
ConsentState(string-based) toConsentData(BooleanFlag-based) - Added
getConsentState()helper function to convert internal format to external format when needed - Simplified consent initialization and tracking by removing intermediate conversions
Comments suppressed due to low confidence (1)
packages/clarity-js/src/data/metadata.ts:159
- Avoid automated semicolon insertion (94% of all statements in the enclosing function have an explicit semicolon).
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getConsentState(): ConsentState { | ||
| return { | ||
| source: consentStatus.source ?? ConsentSource.Implicit, | ||
| ad_Storage: consentStatus.ad_Storage === BooleanFlag.True ? Constant.Granted : Constant.Denied, | ||
| analytics_Storage: consentStatus.analytics_Storage === BooleanFlag.True ? Constant.Granted : Constant.Denied, | ||
| }; |
There was a problem hiding this comment.
Potential null reference error: consentStatus can be null, but this function accesses its properties without null checking. This function is called from line 154 in consentv2() before consentStatus is guaranteed to be initialized. Consider adding a null check or defaulting to implicit consent values when consentStatus is null:
function getConsentState(): ConsentState {
if (!consentStatus) {
return {
source: ConsentSource.Implicit,
ad_Storage: Constant.Denied,
analytics_Storage: Constant.Denied
};
}
return {
source: consentStatus.source ?? ConsentSource.Implicit,
ad_Storage: consentStatus.ad_Storage === BooleanFlag.True ? Constant.Granted : Constant.Denied,
analytics_Storage: consentStatus.analytics_Storage === BooleanFlag.True ? Constant.Granted : Constant.Denied,
};
}| function getConsentState(): ConsentState { | ||
| return { | ||
| source: consentStatus.source ?? ConsentSource.Implicit, | ||
| ad_Storage: consentStatus.ad_Storage === BooleanFlag.True ? Constant.Granted : Constant.Denied, | ||
| analytics_Storage: consentStatus.analytics_Storage === BooleanFlag.True ? Constant.Granted : Constant.Denied, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The new getConsentState() function lacks test coverage. Consider adding unit tests to verify:
- Correct conversion from BooleanFlag to Constant.Granted/Denied
- Proper handling of null
consentStatus - Default source value when
consentStatus.sourceis undefined
This is especially important given this function is used in callback flows (lines 130, 252) and in consentv2() (line 154).
| }; | ||
| ad_Storage: normalizeConsent(consentState.ad_Storage, currentState.ad_Storage), | ||
| analytics_Storage: normalizeConsent(consentState.analytics_Storage, currentState.analytics_Storage), | ||
| } |
There was a problem hiding this comment.
Missing semicolon at the end of this object literal. This should be consistent with other object declarations in the codebase.
| } | |
| }; |
| let rootDomain = null; | ||
| let consentStatus: ConsentState = null; | ||
| let consentStatus: ConsentData = null; | ||
| let defaultStatus: ConsentState = { source: ConsentSource.API, ad_Storage: Constant.Denied, analytics_Storage: Constant.Denied }; |
There was a problem hiding this comment.
Should this not be ConsentData type? Maybe also consider giving this a unique source as well (the more the better).
There was a problem hiding this comment.
ConsentData uses boolean flags, the default status uses strings "granted" | "denied" sent to the API
|
|
||
| const currentState = getConsentState(); | ||
| const consentUpdate = { | ||
| source: consentState.source ?? source, |
There was a problem hiding this comment.
Wouldn't this alway mean we keep the last source? Should it not be flipped?
| trackConsent.consent(); | ||
| } | ||
|
|
||
| export function consentv2(consentState: ConsentState = defaultStatus, source: number = ConsentSource.API): void { |
There was a problem hiding this comment.
In this or later PR change to APIV2 so we know more precisely the source.
|
|
||
| function getConsentState(): ConsentState { | ||
| return { | ||
| source: consentStatus.source ?? ConsentSource.Implicit, |
There was a problem hiding this comment.
When can source be null/undefined?
Update to store consent type as the values used for processing instead of object sent by site.