Additional Templates and Session concluded Noc #347
Conversation
… WEB-3212-web-native-html-json
Updated version & CHANGELOG.md
…removing from backupEvents if fired
…-web-sdk into WEB-3469 # Conflicts: # CHANGELOG.md # clevertap.js.map # clevertap.min.js
Setting VAPID_MIGRATION_PROMPT_SHOWN correct if notification shown & …
[WEB-3469] Soft prompt Loading twice due to race condition when enablePush called twice
![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 comments (1)
clevertap.js (1)
6976-6983:🧩 Analysis chain
Verify popup loading state management
The code checks for existing popup wrappers before loading new ones, but there could be race conditions.
Run this script to check for potential race conditions in popup loading:
🏁 Script executed:
#!/bin/bash # Search for all popup loading state checks and their corresponding state updates rg -A 5 "readFromLSorCookie\(POPUP_LOADING\)"Length of output: 67247
Potential Race Condition in Popup Loading State Management
The search output confirms that both in
notification.jsandclevertap.jsthe popup loading state is managed by checking for an existing state viaStorageManager.readFromLSorCookie(POPUP_LOADING)(or by checking for the old soft prompt wrapper) and then, if not found, setting it withStorageManager.saveToLSorCookie(POPUP_LOADING, true). Since these operations are performed in separate steps rather than atomic operations, concurrent popup load requests might both pass the check and proceed to load popups, potentially causing race conditions.
- The popup loading check and state update are non-atomic.
- Multiple asynchronous calls can potentially bypass the loading state check if they run concurrently.
🧹 Nitpick comments (7)
src/modules/webPushPrompt/prompt.js (2)
165-165: Consider moving the GIF URL to constants.The hardcoded GIF URL should be moved to constants to improve maintainability and make it easier to update if the CDN changes.
- src: 'https://d2r1yp2w7bby2u.cloudfront.net/js/permission_grant.gif' + src: GIF_URLAdd to constants.js:
export const GIF_URL = 'https://d2r1yp2w7bby2u.cloudfront.net/js/permission_grant.gif'
69-110: Consider using DocumentFragment for better performance.When creating multiple DOM elements, using a DocumentFragment can improve performance by reducing reflows and repaints.
- const wrapper = createElementWithAttributes('div', { id: NEW_SOFT_PROMPT_SELCTOR_ID }) + const wrapper = createElementWithAttributes('div', { id: NEW_SOFT_PROMPT_SELCTOR_ID }) + const fragment = document.createDocumentFragment() // ... create other elements ... - wrapper.appendChild(styleElement) - wrapper.appendChild(pnCard) - wrapper.appendChild(overlayDiv) + fragment.appendChild(styleElement) + fragment.appendChild(pnCard) + fragment.appendChild(overlayDiv) + wrapper.appendChild(fragment)src/modules/notification.js (1)
40-47: Add error handling for missing account ID.The check for
ACCOUNT_IDis good, but consider enhancing the error handling:
- Log more details about the error
- Return a specific error code or throw an error for better error handling by callers
push (...displayArgs) { if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { this.#setUpWebPush(displayArgs) return 0 } else { - this.#logger.error('Account ID is not set') + const error = new Error('Account ID is not set. Web push notifications require a valid account ID.') + this.#logger.error(error.message) + return -1 } }src/clevertap.js (2)
614-618: Consider adding null check for account ID.While the code correctly stores the account ID, it should validate that the ID is not null or empty before saving.
if (clevertap.account?.[0].id) { + const accountId = clevertap.account[0].id + if (!accountId || accountId.trim() === '') { + this.#logger.error('Invalid account ID provided') + return + } StorageManager.saveToLSorCookie(ACCOUNT_ID, clevertap.account?.[0].id) this.init() }
913-924: Improved offline mode handling.The code now correctly processes cached events only when transitioning from offline to online state.
However, consider adding debug logging for state transitions:
setOffline (arg) { if (typeof arg !== 'boolean') { console.error('setOffline should be called with a value of type boolean') return } // Check if the offline state is changing from true to false // If offline is being disabled (arg is false), process any cached events if ($ct.offline !== arg && !arg) { + this.#logger.debug('Transitioning from offline to online mode. Processing cached events...') this.#request.processBackupEvents() } $ct.offline = arg + this.#logger.debug(`Offline mode ${arg ? 'enabled' : 'disabled'}`) }clevertap.js (2)
976-984: Improve event push error handlingThe event push implementation has been updated to check for account ID before processing events. However, the error handling could be improved.
Consider adding more descriptive error messages and logging:
if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { for (var _len = arguments.length, eventsArr = new Array(_len), _key = 0; _key < _len; _key++) { eventsArr[_key] = arguments[_key]; } _classPrivateFieldLooseBase(this, _processEventArray)[_processEventArray](eventsArr); return 0; } else { - _classPrivateFieldLooseBase(this, _logger$5)[_logger$5].error('Account ID is not set'); + _classPrivateFieldLooseBase(this, _logger$5)[_logger$5].error('Cannot push events - Account ID is not set. Please initialize the SDK with a valid account ID.'); }Also applies to: 1135-1145
9515-9523: Improve offline mode state transition handlingThe offline mode state transition logic has been improved to process cached events when going from offline to online. However, the comment placement could be improved for better readability.
Consider reorganizing the comments:
- } // Check if the offline state is changing from true to false - // If offline is being disabled (arg is false), process any cached events + } + // Check if the offline state is changing from true to false + // If offline is being disabled (arg is false), process any cached events if ($ct.offline !== arg && !arg) { _classPrivateFieldLooseBase(this, _request$7)[_request$7].processBackupEvents(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clevertap.js.mapis excluded by!**/*.mapclevertap.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)clevertap.js(23 hunks)src/clevertap.js(4 hunks)src/modules/event.js(2 hunks)src/modules/notification.js(5 hunks)src/modules/privacy.js(1 hunks)src/modules/profile.js(2 hunks)src/modules/webPushPrompt/prompt.js(2 hunks)src/util/constants.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/modules/privacy.js
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 Biome (1.9.4)
clevertap.js
[error] 4595-4595: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 9307-9307: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (10)
src/util/constants.js (1)
17-17: LGTM! Constants are well-defined and follow naming conventions.The new constants are appropriately named and their values are meaningful for their intended use across the notification handling system.
Also applies to: 64-66
src/modules/event.js (1)
21-27: LGTM! Good security check for account ID.The check ensures events are only processed when a valid account ID is set, with appropriate error logging.
src/modules/profile.js (1)
46-53: LGTM! Consistent account ID validation.The implementation matches the security check in event.js, ensuring consistent behavior across modules.
src/modules/notification.js (2)
1-10: LGTM! Constants are properly imported and organized.The new constants
ACCOUNT_ID,POPUP_LOADING, andOLD_SOFT_PROMPT_SELCTOR_IDare correctly imported from the constants file.
441-444: Good implementation of popup loading state management.The code correctly prevents multiple popups from loading simultaneously by:
- Checking both
POPUP_LOADINGstate and existing popup element- Using proper debug logging
- Early return to prevent further execution
Also applies to: 476-479
src/clevertap.js (2)
1-35: LGTM! Constants are properly imported.The new constant
ACCOUNT_IDis correctly imported along with other constants.
639-642: Good implementation of account ID storage with logging.The code correctly:
- Stores the account ID
- Adds debug logging for initialization
clevertap.js (3)
4424-4424: Version update to 1.12.1The SDK version has been updated from 1.12.0 to 1.12.1 across multiple locations in the code.
Also applies to: 4795-4795, 7795-7795
5212-5230: 🛠️ Refactor suggestionNew function: handleJson - Add error handling
The new
handleJsonfunction handles type 6 messages by dispatching JSON data. The implementation needs better error handling.Consider adding:
const inaObj = {}; inaObj.msgId = targetingMsgJson.wzrk_id; const details = targetingMsgJson.display.details[0]; const json = details.json; +if (!json) { + logger.error('No JSON data found in message'); + return; +} + +try { + // Validate JSON data if needed + JSON.parse(JSON.stringify(json)); +} catch (e) { + logger.error('Invalid JSON data:', e); + return; +}Likely invalid or redundant comment.
5166-5211: 🛠️ Refactor suggestionNew function: renderCustomHtml - Verify error handling
The new
renderCustomHtmlfunction handles type 5 messages by rendering custom HTML content. While the implementation looks good, there are a few concerns:
- The error handling could be improved to handle more edge cases
- The function should validate the HTML content before rendering to prevent XSS attacks
Consider adding:
const { display, wzrk_id: wzrkId, wzrk_pivot: wzrkPivot } = targetingMsgJson || {}; const divId = display.divId || {}; const details = display.details[0]; const html = details.html; +// Add input validation +if (!html || typeof html !== 'string') { + logger.error('Invalid HTML content'); + return; +} + +// Add basic XSS protection +const sanitizedHtml = html.replace(/<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi, '');Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/modules/notification.js (1)
441-449: Consider using a more robust state management approach.The current implementation uses localStorage/cookies for managing popup loading state. This could lead to race conditions in edge cases where multiple notifications are triggered simultaneously.
Consider using a more robust state management approach:
-if (StorageManager.readFromLSorCookie(POPUP_LOADING) || document.getElementById(OLD_SOFT_PROMPT_SELCTOR_ID)) { +const isPopupLoading = StorageManager.readFromLSorCookie(POPUP_LOADING) +const existingSoftPrompt = document.getElementById(OLD_SOFT_PROMPT_SELCTOR_ID) +if (isPopupLoading || existingSoftPrompt) { this.#logger.debug('Soft prompt wrapper is already loading or loaded') return } -StorageManager.saveToLSorCookie(POPUP_LOADING, true) +try { + StorageManager.saveToLSorCookie(POPUP_LOADING, true) +} catch (error) { + this.#logger.error('Failed to set popup loading state:', error) + return +}Also applies to: 476-484
clevertap.js (3)
1134-1145: Add error handling for account ID checkThe account ID check should include error handling for invalid or malformed IDs.
if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { + const accountId = StorageManager.readFromLSorCookie(ACCOUNT_ID); + if (!accountId || typeof accountId !== 'string') { + _classPrivateFieldLooseBase(this, _logger$2)[_logger$2].error('Invalid Account ID format'); + return; + } for (var _len = arguments.length, eventsArr = new Array(_len), _key = 0; _key < _len; _key++) { eventsArr[_key] = arguments[_key]; } _classPrivateFieldLooseBase(this, _processEventArray)[_processEventArray](eventsArr); return 0;
6976-6983: Add validation for popup loading stateThe popup loading state check should include cleanup of stale states.
if (StorageManager.readFromLSorCookie(POPUP_LOADING) || document.getElementById(OLD_SOFT_PROMPT_SELCTOR_ID)) { _classPrivateFieldLooseBase(this, _logger$5)[_logger$5].debug('Soft prompt wrapper is already loading or loaded'); + // Clear stale popup loading state after timeout + setTimeout(() => { + StorageManager.saveToLSorCookie(POPUP_LOADING, false); + }, 10000); // 10 second timeout return; }
9515-9523: Improve offline mode state transitionThe offline mode state transition logic should include cleanup of pending requests.
// Check if the offline state is changing from true to false // If offline is being disabled (arg is false), process any cached events if ($ct.offline !== arg && !arg) { + // Clear any pending timeouts or intervals + if ($ct.pendingTimeouts) { + $ct.pendingTimeouts.forEach(clearTimeout); + $ct.pendingTimeouts = []; + } _classPrivateFieldLooseBase(this, _request$7)[_request$7].processBackupEvents(); } $ct.offline = arg;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clevertap.js.mapis excluded by!**/*.mapclevertap.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (2)
clevertap.js(23 hunks)src/modules/notification.js(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
clevertap.js
[error] 4595-4595: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 9307-9307: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (7)
src/modules/notification.js (3)
6-9: LGTM! Constants are well-named and follow conventions.The new constants ACCOUNT_ID, POPUP_LOADING, and OLD_SOFT_PROMPT_SELCTOR_ID are appropriately named and follow the existing naming convention.
362-364: LGTM! Safari VAPID migration handling is correct.The code correctly handles VAPID migration for Safari browsers by setting the migration flag when native web push is supported and FCM public key is available.
1-15: Verify ACCOUNT_ID usage across the codebase.Let's verify that ACCOUNT_ID is consistently used across the codebase to prevent unauthorized operations.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for ACCOUNT_ID usage across the codebase echo "Searching for ACCOUNT_ID usage..." rg "ACCOUNT_ID" --type js --type ts # Search for potential direct account ID access without validation echo "Checking for potential direct account ID access..." rg "readFromLSorCookie\([^)]*\)" --type js --type tsLength of output: 10858
ACCOUNT_ID Usage Verified Across the Codebase
- The search results confirm that
ACCOUNT_IDis consistently imported and used viaStorageManager.readFromLSorCookie(ACCOUNT_ID)in multiple modules (e.g.,event.js,notification.js,profile.js, andclevertap.js), aligning with secure practices.- There is no evidence of uncontrolled direct access or validation bypass, reducing the risk of unauthorized operations.
- The conditional check for popup loading state (using
POPUP_LOADINGand checking for the existence ofOLD_SOFT_PROMPT_SELCTOR_ID) appears straightforward and does not indicate any obvious race conditions.Overall, the changes introducing the new constants and state management do not adversely affect the usage of
ACCOUNT_IDand related state checks.clevertap.js (4)
214-216: LGTM: Constants for soft prompt selectorsThe new constants for soft prompt selectors are well-defined and follow the existing naming convention.
5166-5211: LGTM: Custom HTML rendering implementationThe
renderCustomHtmlfunction is well-structured with proper error handling and validation.
5212-5230: LGTM: JSON data handlingThe
handleJsonfunction correctly processes and dispatches JSON data through custom events.
4424-4424: Version update consistencyThe SDK version has been updated to '1.12.1' consistently across all references.
Also applies to: 4425-4425, 4795-4795, 4796-4796, 9527-9527
| push (...displayArgs) { | ||
| this.#setUpWebPush(displayArgs) | ||
| return 0 | ||
| if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { | ||
| this.#setUpWebPush(displayArgs) | ||
| return 0 | ||
| } else { | ||
| this.#logger.error('Account ID is not set') | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for missing account ID.
The current implementation logs an error but doesn't provide a return value when the account ID is missing. This could lead to undefined behavior.
Apply this diff to handle the error case:
push (...displayArgs) {
if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) {
this.#setUpWebPush(displayArgs)
return 0
} else {
this.#logger.error('Account ID is not set')
+ return -1
}
}📝 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.
| push (...displayArgs) { | |
| this.#setUpWebPush(displayArgs) | |
| return 0 | |
| if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { | |
| this.#setUpWebPush(displayArgs) | |
| return 0 | |
| } else { | |
| this.#logger.error('Account ID is not set') | |
| } | |
| } | |
| push (...displayArgs) { | |
| if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { | |
| this.#setUpWebPush(displayArgs) | |
| return 0 | |
| } else { | |
| this.#logger.error('Account ID is not set') | |
| return -1 | |
| } | |
| } |
| this.#addWizAlertJS().onload = () => { | ||
| StorageManager.saveToLSorCookie(POPUP_LOADING, false) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for popup loading state cleanup.
The popup loading state should be cleaned up even if the script fails to load to prevent the popup from being permanently locked.
Apply this diff to add error handling:
-this.#addWizAlertJS().onload = () => {
+const scriptElement = this.#addWizAlertJS()
+scriptElement.onload = () => {
StorageManager.saveToLSorCookie(POPUP_LOADING, false)
// ... rest of the code
}
+scriptElement.onerror = () => {
+ this.#logger.error('Failed to load notification dialog script')
+ StorageManager.saveToLSorCookie(POPUP_LOADING, false)
+}📝 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.
| this.#addWizAlertJS().onload = () => { | |
| StorageManager.saveToLSorCookie(POPUP_LOADING, false) | |
| const scriptElement = this.#addWizAlertJS() | |
| scriptElement.onload = () => { | |
| StorageManager.saveToLSorCookie(POPUP_LOADING, false) | |
| // ... rest of the code | |
| } | |
| scriptElement.onerror = () => { | |
| this.#logger.error('Failed to load notification dialog script') | |
| StorageManager.saveToLSorCookie(POPUP_LOADING, false) | |
| } |
| /* For extremely slow networks we often recreate the session from the SE hence appending | ||
| the session to the request */ | ||
| const session = JSON.parse(StorageManager.readCookie(SCOOKIE_PREFIX + '_' + _classPrivateFieldLooseBase(this, _account$3)[_account$3].id)); | ||
|
|
||
| if (session === null || session === void 0 ? void 0 : session.s) { | ||
| backupEvent.q = backupEvent.q + '&s=' + session.s; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add session validation for request appending
The session appending logic should validate the session object structure.
/* For extremely slow networks we often recreate the session from the SE hence appending
the session to the request */
const session = JSON.parse(StorageManager.readCookie(SCOOKIE_PREFIX + '_' + _classPrivateFieldLooseBase(this, _account$3)[_account$3].id));
+if (session === null || session === void 0 || typeof session !== 'object') {
+ _classPrivateFieldLooseBase(this, _logger$7)[_logger$7].debug('Invalid session object');
+ return;
+}
if (session === null || session === void 0 ? void 0 : session.s) {
backupEvent.q = backupEvent.q + '&s=' + session.s;
}📝 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.
| /* For extremely slow networks we often recreate the session from the SE hence appending | |
| the session to the request */ | |
| const session = JSON.parse(StorageManager.readCookie(SCOOKIE_PREFIX + '_' + _classPrivateFieldLooseBase(this, _account$3)[_account$3].id)); | |
| if (session === null || session === void 0 ? void 0 : session.s) { | |
| backupEvent.q = backupEvent.q + '&s=' + session.s; | |
| } | |
| /* For extremely slow networks we often recreate the session from the SE hence appending | |
| the session to the request */ | |
| const session = JSON.parse(StorageManager.readCookie(SCOOKIE_PREFIX + '_' + _classPrivateFieldLooseBase(this, _account$3)[_account$3].id)); | |
| if (session === null || session === void 0 || typeof session !== 'object') { | |
| _classPrivateFieldLooseBase(this, _logger$7)[_logger$7].debug('Invalid session object'); | |
| return; | |
| } | |
| if (session === null || session === void 0 ? void 0 : session.s) { | |
| backupEvent.q = backupEvent.q + '&s=' + session.s; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/modules/notification.js (2)
40-47:⚠️ Potential issueAdd error handling for missing account ID.
The current implementation logs an error but doesn't provide a return value when the account ID is missing. This could lead to undefined behavior.
Apply this diff to handle the error case:
push (...displayArgs) { if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { this.#setUpWebPush(displayArgs) return 0 } else { this.#logger.error('Account ID is not set') + return -1 } }
442-450:⚠️ Potential issueAdd error handling for popup loading state cleanup.
The popup loading state should be cleaned up even if the script fails to load to prevent the popup from being permanently locked.
Apply this diff to add error handling:
-this.#addWizAlertJS().onload = () => { +const scriptElement = this.#addWizAlertJS() +scriptElement.onload = () => { StorageManager.saveToLSorCookie(POPUP_LOADING, false) // ... rest of the code } +scriptElement.onerror = () => { + this.#logger.error('Failed to load notification dialog script') + StorageManager.saveToLSorCookie(POPUP_LOADING, false) +}Also applies to: 477-484
clevertap.js (2)
5212-5230:⚠️ Potential issueAdd validation for JSON data integrity.
The function should validate the JSON data structure and handle potential parsing errors before dispatching the event.
Apply this diff to add JSON validation:
const inaObj = {}; inaObj.msgId = targetingMsgJson.wzrk_id; const details = targetingMsgJson.display.details[0]; const json = details.json; +if (!json) { + logger.error('No JSON data found in message'); + return; +} + +try { + // Validate JSON data + JSON.parse(JSON.stringify(json)); +} catch (e) { + logger.error('Invalid JSON data:', e); + return; +}
7749-7755:⚠️ Potential issueAdd null check for session object.
The session appending logic should validate the session object structure.
Apply this diff:
/* For extremely slow networks we often recreate the session from the SE hence appending the session to the request */ const session = JSON.parse(StorageManager.readCookie(SCOOKIE_PREFIX + '_' + _classPrivateFieldLooseBase(this, _account$3)[_account$3].id)); +if (session === null || session === void 0 || typeof session !== 'object') { + _classPrivateFieldLooseBase(this, _logger$7)[_logger$7].debug('Invalid session object'); + return; +} if (session === null || session === void 0 ? void 0 : session.s) { backupEvent.q = backupEvent.q + '&s=' + session.s; }
🧹 Nitpick comments (2)
src/modules/notification.js (1)
482-507: Consider adding a timeout for script loading.The script loading process could potentially hang indefinitely if the script fails to trigger either onload or onerror events.
Add a timeout to ensure the loading state is cleaned up:
+const SCRIPT_LOAD_TIMEOUT = 10000; // 10 seconds +const timeoutId = setTimeout(() => { + this.#logger.error('Notification dialog script load timed out') + StorageManager.saveToLSorCookie(POPUP_LOADING, false) +}, SCRIPT_LOAD_TIMEOUT) const scriptElement = this.#addWizAlertJS() scriptElement.onload = () => { + clearTimeout(timeoutId) StorageManager.saveToLSorCookie(POPUP_LOADING, false) // ... rest of the code } scriptElement.onerror = () => { + clearTimeout(timeoutId) this.#logger.error('Failed to load notification dialog script') StorageManager.saveToLSorCookie(POPUP_LOADING, false) }clevertap.js (1)
6016-6022: Add loading state validation.The popup loading state check should be more robust to prevent race conditions.
Apply this diff:
-if (StorageManager.readFromLSorCookie(POPUP_LOADING) || document.getElementById(OLD_SOFT_PROMPT_SELCTOR_ID)) { +const isLoading = StorageManager.readFromLSorCookie(POPUP_LOADING); +const existingPrompt = document.getElementById(OLD_SOFT_PROMPT_SELCTOR_ID); +if (isLoading || existingPrompt) { _classPrivateFieldLooseBase(this, _logger$5)[_logger$5].debug('Soft prompt wrapper is already loading or loaded'); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clevertap.js.mapis excluded by!**/*.mapclevertap.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (2)
clevertap.js(24 hunks)src/modules/notification.js(6 hunks)
🔇 Additional comments (5)
src/modules/notification.js (1)
361-366: LGTM: Safari VAPID migration support.The implementation correctly handles the VAPID migration for Safari browsers by:
- Setting the migration flag when VAPID is supported
- Checking for VAPID support before proceeding with notifications
clevertap.js (4)
165-165: LGTM! New constant for account ID storage.The addition of
ACCOUNT_IDconstant follows the existing naming convention and is used consistently throughout the code.
1135-1145: LGTM! Improved event handling with account ID validation.The addition of account ID validation before processing events improves robustness and error handling.
9527-9528: LGTM! Version update to 1.12.1.The SDK version has been updated correctly across all references.
5166-5211:⚠️ Potential issueAdd validation for HTML content security.
While the function handles basic error cases, it should validate and sanitize the HTML content before rendering to prevent XSS attacks.
Apply this diff to add HTML sanitization:
const html = details.html; +if (!html || typeof html !== 'string') { + logger.error('Invalid HTML content'); + return; +} + +// Sanitize HTML to prevent XSS +const sanitizedHtml = html.replace(/<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi, '') + .replace(/on\w+="[^"]*"/g, '') + .replace(/on\w+='[^']*'/g, ''); + if (!divId || !html) { logger.error('No div Id or no html found'); return; }Likely invalid or redundant comment.
[WEB-3457] Fixed Unviewed Badge for SPA
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clevertap.js (2)
1134-1145: Add error handling for account ID check.The account ID check is good but could benefit from more robust error handling.
if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { for (var _len = arguments.length, eventsArr = new Array(_len), _key = 0; _key < _len; _key++) { eventsArr[_key] = arguments[_key]; } _classPrivateFieldLooseBase(this, _processEventArray)[_processEventArray](eventsArr); return 0; } else { + const error = new Error('Account ID is not set'); _classPrivateFieldLooseBase(this, _logger$2)[_logger$2].error('Account ID is not set'); + return error; }
2398-2408: Add error handling for profile push.Similar to event push, profile push should have better error handling.
if (StorageManager.readFromLSorCookie(ACCOUNT_ID)) { for (var _len = arguments.length, profilesArr = new Array(_len), _key = 0; _key < _len; _key++) { profilesArr[_key] = arguments[_key]; } _classPrivateFieldLooseBase(this, _processProfileArray)[_processProfileArray](profilesArr); return 0; } else { + const error = new Error('Account ID is not set'); _classPrivateFieldLooseBase(this, _logger$3)[_logger$3].error('Account ID is not set'); + return error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
clevertap.js.mapis excluded by!**/*.map
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)clevertap.js(25 hunks)src/clevertap.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- src/clevertap.js
🔇 Additional comments (4)
clevertap.js (4)
165-165: LGTM: New constant for account ID tracking.The addition of
ACCOUNT_IDconstant follows the existing naming convention and is used consistently.
424-424: Version number updated consistently.The SDK version has been updated to 1.12.1 across all references.
Also applies to: 795-795, 9527-9527
🧰 Tools
🪛 Biome (1.9.4)
[error] 424-424: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
5166-5211: 🛠️ Refactor suggestionVerify error handling in renderCustomHtml function.
The new renderCustomHtml function needs additional validation and error handling.
const renderCustomHtml = (targetingMsgJson, logger) => { const { display, wzrk_id: wzrkId, wzrk_pivot: wzrkPivot } = targetingMsgJson || {}; const divId = display.divId || {}; const details = display.details[0]; const html = details.html; + // Validate input parameters + if (!targetingMsgJson || !display || !details) { + logger.error('Invalid targeting message JSON'); + return; + } + // Sanitize HTML content + const sanitizedHtml = sanitizeHTML(html); if (!divId || !html) { logger.error('No div Id or no html found'); return; }
5212-5230: 🛠️ Refactor suggestionAdd validation in handleJson function.
The new handleJson function should validate the JSON data before processing.
const handleJson = targetingMsgJson => { + if (!targetingMsgJson || !targetingMsgJson.display || !targetingMsgJson.display.details) { + console.error('Invalid targeting message JSON'); + return; + } const inaObj = {}; inaObj.msgId = targetingMsgJson.wzrk_id; const details = targetingMsgJson.display.details[0]; const json = details.json; + if (!json) { + console.error('No JSON data found in message'); + return; + } + try { + // Validate JSON data + JSON.parse(JSON.stringify(json)); + } catch (e) { + console.error('Invalid JSON data:', e); + return; + }Likely invalid or redundant comment.
Changes
#346
#331
#344
Changes to Public Facing API if any
Please list the impact on the public facing API if any
How Has This Been Tested?
Describe the testing approach and any relevant configurations (e.g., environment, platform)
Checklist
Summary by CodeRabbit
Release Notes for CleverTap Web SDK v1.12.2
New Features
Bug Fixes
Version Update