fix: microsoft clarity load and handle promise for identify #1964
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js (1)
58-68: Consider propagating the Promise rejectionThe Promise handling looks good overall - it ensures traits are set only after successful identification and includes error logging. However, the error is caught and logged but not propagated, which might hide issues from the application.
Consider propagating the error:
- window.clarity('identify', userId, sessionId, customPageId).then(() => { - if (context?.traits) { - const { traits } = context; - const keys = Object.keys(traits); - keys.forEach(key => { - window.clarity('set', key, traits[key]); - }); - } - }).catch(error => { - logger.error('[MicrosoftClarity] Error in identify call', error); - }); + return window.clarity('identify', userId, sessionId, customPageId) + .then(() => { + if (context?.traits) { + const { traits } = context; + const keys = Object.keys(traits); + keys.forEach(key => { + window.clarity('set', key, traits[key]); + }); + } + }) + .catch(error => { + logger.error('[MicrosoftClarity] Error in identify call', error); + throw error; // Propagate the error + });The trait handling implementation aligns with the previous learnings that validation/sanitization isn't required and
clarity('set')doesn't return a Promise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js (2)
Learnt from: sanpj2292
PR: rudderlabs/rudder-sdk-js#1948
File: packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js:59-65
Timestamp: 2024-11-25T11:33:39.706Z
Learning: In the Microsoft Clarity integration (`packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js`), adding validation or sanitization of trait values before sending them to Microsoft Clarity's SDK is not required at this time.
Learnt from: sanpj2292
PR: rudderlabs/rudder-sdk-js#1948
File: packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js:58-68
Timestamp: 2024-11-25T11:21:01.798Z
Learning: In the MicrosoftClarity integration (`packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js`), the `window.clarity('set', key, value)` method does not return a Promise and does not require error handling.
🔇 Additional comments (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js (1)
36-36:
Reconsider the SDK loading check implementation
The current implementation calls clarity('identify') to verify SDK loading, which could have unintended consequences:
- It might trigger unnecessary identify events
- It's inefficient as this check may be called frequently
- It could interfere with actual identify calls
Consider this alternative implementation that checks for the function's existence without calling it:
- return !!window.clarity && window.clarity('identify') instanceof Promise;
+ return typeof window.clarity === 'function';Let's verify if clarity is always a function when loaded:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1964 +/- ##
===========================================
- Coverage 61.89% 61.88% -0.01%
===========================================
Files 484 484
Lines 16610 16612 +2
Branches 3351 3338 -13
===========================================
Hits 10280 10280
- Misses 5083 5111 +28
+ Partials 1247 1221 -26 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js (1)
Line range hint
64-71: Consider moving traits setting into Promise chain.The traits are currently set regardless of whether the identify call succeeds. This could lead to inconsistent state in Clarity.
Consider this approach:
const identifyPromise = window.clarity('identify', userId, sessionId, customPageId); - if (identifyPromise) { // Clarity SDK is ready - identifyPromise.catch(logger.error); - } - if (context?.traits) { - const { traits } = context; - const keys = Object.keys(traits); - keys.forEach(key => { - window.clarity('set', key, traits[key]); - }); - } + if (identifyPromise) { // Clarity SDK is ready + identifyPromise + .then(() => { + logger.debug('Successfully identified user in Microsoft Clarity'); + if (context?.traits) { + const { traits } = context; + Object.entries(traits).forEach(([key, value]) => { + window.clarity('set', key, value); + }); + } + }) + .catch(error => { + logger.error('Failed to identify user in Microsoft Clarity:', error); + }); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js (1)
Learnt from: sanpj2292
PR: rudderlabs/rudder-sdk-js#1948
File: packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js:59-65
Timestamp: 2024-11-25T11:33:39.706Z
Learning: In the Microsoft Clarity integration (`packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js`), adding validation or sanitization of trait values before sending them to Microsoft Clarity's SDK is not required at this time.
🪛 Biome (1.9.4)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js
[error] 61-61: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (1)
packages/analytics-js-integrations/src/integrations/MicrosoftClarity/browser.js (1)
36-37: LGTM! Enhanced SDK loading verification.
The improved loading check is more robust as it verifies both the SDK's presence and its initialization state through the queue.
Let's verify the loading queue behavior:
✅ Verification successful
Let me gather more information about the SDK initialization pattern to verify this thoroughly.
Let me check one more aspect of the SDK loading to verify the queue pattern completely.
Loading check implementation correctly verifies SDK initialization state
The verification confirms that the loading check is accurate and well-implemented:
- The native SDK loader shows that
clarity.qis used as a command queue during initialization:(c[a].q = c[a].q || []).push(arguments) - Once the SDK is fully loaded, this queue is cleared, making
window.clarity.qundefined - The
isLoaded()check!!window.clarity && !window.clarity.qcorrectly validates both SDK presence and complete initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Microsoft Clarity SDK loading queue pattern
# Expected: Find references to clarity.q in the SDK's initialization code
# Search for clarity.q references in the codebase
rg -A 5 'clarity\.q'
# Search for SDK loading patterns
ast-grep --pattern 'window.clarity = function() { clarity.q.push($$$) }'
Length of output: 678
Script:
#!/bin/bash
# Search for clarity initialization patterns in the codebase
rg -B 5 -A 5 'window\.clarity'
# Look for any SDK loading related code
ast-grep --pattern 'function() {
$$$
clarity.q
$$$
}'
# Check the official SDK snippet pattern
rg -l 'clarity' packages/analytics-js-integrations/src/integrations/MicrosoftClarity/
Length of output: 4605
Script:
#!/bin/bash
# Check the SDK loader implementation
cat packages/analytics-js-integrations/src/integrations/MicrosoftClarity/nativeSdkLoader.js
# Look for any queue initialization patterns
ast-grep --pattern '(function(c,l,a,r,i,t,y){
$$$
}'
Length of output: 784
|
|
||
| const identifyPromise = window.clarity('identify', userId, sessionId, customPageId); | ||
| if (!!identifyPromise) { // Clarity SDK is ready | ||
| identifyPromise.catch(logger.error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Promise check and improve error handling.
The Promise existence check can be simplified, and the Promise chain could be more robust.
Apply this diff to improve the code:
- const identifyPromise = window.clarity('identify', userId, sessionId, customPageId);
- if (!!identifyPromise) { // Clarity SDK is ready
- identifyPromise.catch(logger.error);
- }
+ const identifyPromise = window.clarity('identify', userId, sessionId, customPageId);
+ if (identifyPromise) { // Clarity SDK is ready
+ identifyPromise
+ .then(() => {
+ logger.debug('Successfully identified user in Microsoft Clarity');
+ })
+ .catch(error => {
+ logger.error('Failed to identify user in Microsoft Clarity:', error);
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const identifyPromise = window.clarity('identify', userId, sessionId, customPageId); | |
| if (!!identifyPromise) { // Clarity SDK is ready | |
| identifyPromise.catch(logger.error); | |
| } | |
| const identifyPromise = window.clarity('identify', userId, sessionId, customPageId); | |
| if (identifyPromise) { // Clarity SDK is ready | |
| identifyPromise | |
| .then(() => { | |
| logger.debug('Successfully identified user in Microsoft Clarity'); | |
| }) | |
| .catch(error => { | |
| logger.error('Failed to identify user in Microsoft Clarity:', error); | |
| }); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
|
PR Description
Microsoft Clarity destination in Rudderstack only supports
identifyevent & when we try to sendidentifyevent to Clarity would return a PromiseBut while SDK is upgrading/loading, seems like the current logic of validating if the SDK has been loaded is not complete & caused unhandled exceptions(here)
Through this PR, we are planning to fix the issue
Linear task (optional)
Resolves INT-2937
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Bug Fixes