removing enable api testing #338
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant modifications to the web push notification system across multiple files. The changes focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (9)
src/modules/notification.js (5)
38-50: Review the logical flow of push method.The
push(...)method reads thewebPushConfigResponseReceivedflag and either callsprocessSoftPromptor saves anotificationPushCalledflag. This appears consistent with the new approach. Consider adding a short comment explaining why these flags are used, for easier maintainability and onboarding.
150-152: Suggested null-check simplification.Variables
softPromptCardandoldSoftPromptCardmay benullif elements are missing in the DOM, but are guarded later withifstatements. This is fine, though you may consider consolidating these references in a helper function for clarity.
179-184: Consider consolidating DOM removal logic.Removing multiple DOM elements in separate blocks is clear, but you could unify these removals into a small helper to reduce repetition and keep the flow concise.
187-187: Use logger for consistency.Replace
console.log('rejected subscription')with the logger to maintain consistent logging practices.- console.log('rejected subscription') + this.#logger.error('Subscription was rejected')
203-208: Refactor repeated removal logic.Lines 203-208 re-implement the same pattern for removing
softPromptCardandoldSoftPromptCard. Consider a helper method (e.g.#removePromptElements()) to reduce duplication:+ #removePromptElements(element1, element2) { + if (element1) element1.parentNode.removeChild(element1) + if (element2) element2.parentNode.removeChild(element2) + } ... - if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) - } - if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) - } + this.#removePromptElements(softPromptCard, oldSoftPromptCard)src/modules/webPushPrompt/prompt.js (4)
11-14: Consider encapsulating global stateThese module-level variables introduce mutable global state which can make the code harder to maintain and test. Consider encapsulating them within a class or configuration object.
-let logger = null -let account = null -let request = null -let displayArgs = null +class NotificationConfig { + constructor() { + this.logger = null; + this.account = null; + this.request = null; + this.displayArgs = null; + } + + setValues(values = {}) { + Object.assign(this, values); + } +} + +const config = new NotificationConfig();
16-21: Add input validationThe function accepts values without any type checking or validation. Consider adding validation to ensure the required properties have appropriate types and values.
export const setNotificationHandlerValues = (notificationValues = {}) => { + if (!isObject(notificationValues)) { + throw new TypeError('notificationValues must be an object'); + } + logger = notificationValues.logger account = notificationValues.account request = notificationValues.request displayArgs = notificationValues.displayArgs }
64-70: Add input validation and JSDoc documentationThe function handles different argument formats without clear documentation or validation.
+/** + * Parses display arguments for web push setup + * @param {Array|Object} displayArgs - Display arguments in legacy array format or new object format + * @returns {{serviceWorkerPath: string|undefined, skipDialog: boolean|undefined}} + * @throws {TypeError} If displayArgs is neither an array nor an object + */ export const parseDisplayArgs = (displayArgs) => { + if (!Array.isArray(displayArgs) && !isObject(displayArgs)) { + throw new TypeError('displayArgs must be an array or object'); + } + if (displayArgs.length === 1 && isObject(displayArgs[0])) { const { serviceWorkerPath, skipDialog } = displayArgs[0] + if (serviceWorkerPath && typeof serviceWorkerPath !== 'string') { + throw new TypeError('serviceWorkerPath must be a string'); + } + if (skipDialog !== undefined && typeof skipDialog !== 'boolean') { + throw new TypeError('skipDialog must be a boolean'); + } return { serviceWorkerPath, skipDialog } } return { serviceWorkerPath: undefined, skipDialog: displayArgs[5] } }
83-85: Add parameter documentationThe skipDialog parameter's purpose and impact should be documented.
+/** + * @param {boolean|null} skipDialog - When true, bypasses the notification dialog and directly enables push notifications. + * When null, defaults to false. + */ if (skipDialog === null) { skipDialog = false }
📜 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 (3)
clevertap.js(7 hunks)src/modules/notification.js(5 hunks)src/modules/webPushPrompt/prompt.js(3 hunks)
🔇 Additional comments (14)
src/modules/notification.js (2)
9-9: Import path check.The import statement looks correct, and the functions
setNotificationHandlerValuesandprocessSoftPromptappear to be used consistently in this file.
34-34: Method naming consistency.
setupWebPushis descriptive and aligns well with the internal#setUpWebPushmethod. This explicit naming helps clarify the setup process for web push notifications.clevertap.js (11)
5304-5306: LGTM: New method added to handle web push setupThe new
setupWebPushmethod is added to handle web push notification setup by calling the existing_setUpWebPushmethod with display arguments.
5315-5320: LGTM: Proper initialization of notification handler valuesThe code correctly initializes notification handler values by setting logger, account, request and display arguments.
Line range hint
5446-5482: Cleanup of notification UI elementsThe code properly cleans up notification UI elements after successful subscription:
- Removes existing bell wrapper if present
- Removes soft prompt card if present
- Removes old soft prompt card if present
Line range hint
5484-5510: Proper error handling for subscription failuresThe code handles subscription failures appropriately by:
- Unsubscribing from web push if error occurs
- Logging error message
- Cleaning up UI elements
- Retrying subscription with skipDialog flag
5767-5777: LGTM: Notification handler values setterThe
setNotificationHandlerValuesfunction properly sets all required values for notification handling.
5833-5848: LGTM: Display arguments parserThe
parseDisplayArgsfunction properly extracts service worker path and skip dialog options from display arguments.
5864-5866: LGTM: Default skip dialog handlingThe code properly handles the case when skipDialog is null by defaulting it to false.
5309-5310: Verify web push configuration checkThe code checks for web push configuration presence in local storage before proceeding with push notification setup.
✅ Verification successful
Web push configuration check is properly implemented
The code correctly checks for web push configuration presence using localStorage/cookie storage before proceeding with push notification setup. The implementation includes proper configuration processing and state management through the webPushConfigResponseReceived flag.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if web push config is properly stored in localStorage ast-grep --pattern 'StorageManager.readFromLSorCookie("webPushConfigResponseReceived")'Length of output: 89
Script:
#!/bin/bash # Search for webPushConfig related code rg "webPushConfig" -A 2 -B 2 # Try ast-grep with single quotes ast-grep --pattern "StorageManager.readFromLSorCookie('webPushConfigResponseReceived')" # Look for StorageManager write operations for web push rg "StorageManager\.(write|save).*webPushConfig" -A 2 -B 2Length of output: 68081
5799-5832: Verify soft prompt processing logicThe
processSoftPromptfunction:
- Gets web push config from storage
- Creates notification handler
- Handles different scenarios based on config values
- Sets up web push appropriately
✅ Verification successful
Soft prompt processing implementation verified
The implementation correctly handles all scenarios:
- Empty configuration
- New box type with bell icon
- Old box type with direct setup
- Preview mode cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check soft prompt processing implementation ast-grep --pattern 'function processSoftPrompt() { const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {}; $$$ }'Length of output: 142
Script:
#!/bin/bash # Check soft prompt as arrow function ast-grep --pattern 'const processSoftPrompt = () => { const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {}; $$$ }' # Find StorageManager implementation ast-grep --pattern 'class StorageManager { $$$ readFromLSorCookie($$$) { $$$ } $$$ }' # Find NotificationHandler class ast-grep --pattern 'class NotificationHandler { $$$ }' # Find enablePush function ast-grep --pattern 'const enablePush = ($$$) => { $$$ }'Length of output: 30815
5322-5326: Verify soft prompt processing logicThe code handles two scenarios:
- If web push config is present - processes soft prompt immediately
- If not present - saves notification push call flag to localStorage for later processing
✅ Verification successful
Web push configuration and soft prompt handling verified
The code correctly implements a two-path logic for handling web push notifications:
- With web push config present - Immediately processes the soft prompt via NotificationHandler
- Without config - Saves notification state for later processing when config becomes available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if soft prompt processing is properly handled ast-grep --pattern 'if (isWebPushConfigPresent) { processSoftPrompt(); } else { StorageManager.saveToLSorCookie("notificationPushCalled", true); }'Length of output: 153
Script:
#!/bin/bash # Search for processSoftPrompt function and notificationPushCalled usages echo "=== processSoftPrompt function ===" rg "processSoftPrompt" -A 5 echo -e "\n=== notificationPushCalled usages ===" rg "notificationPushCalled" echo -e "\n=== Web push config related code ===" rg "isWebPushConfigPresent"Length of output: 66623
Script:
#!/bin/bash # Search for WEBPUSH_CONFIG constant and related code echo "=== WEBPUSH_CONFIG constant ===" rg "WEBPUSH_CONFIG" -B 2 -A 2 echo -e "\n=== processSoftPrompt implementation ===" rg "processSoftPrompt = " -A 10Length of output: 67399
5791-5797: Verify web push config response handlingThe code saves web push config response flag and processes soft prompt if notification was previously called.
✅ Verification successful
Web push config response handling is correctly implemented
The code follows a proper sequence of saving the web push config response flag and processing soft prompt only when notification was previously called. The implementation includes appropriate state management and handles different notification display types safely.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check web push config response handling ast-grep --pattern 'StorageManager.saveToLSorCookie("webPushConfigResponseReceived", true); const isNotificationPushCalled = StorageManager.readFromLSorCookie("notificationPushCalled"); if (isNotificationPushCalled) { processSoftPrompt(); }'Length of output: 245
Script:
#!/bin/bash # Check StorageManager usage and related functions with more flexible patterns echo "=== StorageManager usage ===" ast-grep --pattern 'StorageManager.saveToLSorCookie($_)' echo -e "\n=== processSoftPrompt implementation ===" ast-grep --pattern 'function processSoftPrompt() { $$$ }' echo -e "\n=== Notification push handling ===" rg "notificationPushCalled" -A 3Length of output: 66498
Script:
#!/bin/bash echo "=== webPushConfigResponseReceived usage ===" rg "webPushConfigResponseReceived" -A 2 echo -e "\n=== processSoftPrompt implementation ===" rg "function processSoftPrompt" -A 10 echo -e "\n=== Notification push handling ===" rg "notificationPushCalled" -B 2 -A 2Length of output: 67099
Script:
#!/bin/bash # Search for processSoftPrompt with more context echo "=== processSoftPrompt definition ===" rg -B 2 -A 15 "processSoftPrompt" echo -e "\n=== Web push config and notification flow ===" rg "webPushConfigResponseReceived|notificationPushCalled|processSoftPrompt" -B 2 -A 2Length of output: 71249
src/modules/webPushPrompt/prompt.js (1)
44-62: 🛠️ Refactor suggestionRefactor for better separation of concerns
The function handles multiple responsibilities (config reading, notification setup, prompt type handling). Consider breaking it down into smaller, focused functions.
export const processSoftPrompt = () => { + const config = getWebPushConfig() + if (!isValidConfig(config)) { + return setupDefaultWebPush() + } + + handlePromptType(config) +} + +const getWebPushConfig = () => { const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {} + return webPushConfig +} + +const isValidConfig = (config) => { + return Object.keys(config).length > 0 +} + +const setupDefaultWebPush = () => { const notificationHandler = new NotificationHandler({ logger, session: {}, request, account }) - if (!(Object.keys(webPushConfig).length > 0)) { - notificationHandler.setApplicationServerKey(appServerKey) - notificationHandler.setupWebPush(displayArgs) - return 0 - } + notificationHandler.setApplicationServerKey(appServerKey) + notificationHandler.setupWebPush(displayArgs) +} + +const handlePromptType = (config) => { + const { showBox, showBellIcon, boxType } = config const { serviceWorkerPath, skipDialog } = parseDisplayArgs(displayArgs) if (showBellIcon || (showBox && boxType === 'new')) { enablePush(logger, account, request, serviceWorkerPath, skipDialog) } if (showBox && boxType === 'old') { notificationHandler.setApplicationServerKey(appServerKey) notificationHandler.setupWebPush(displayArgs) } }Remove magic number
The function returns 0 without explanation. Consider using a named constant or removing the return value if it's not used.
- return 0 + return // Remove if return value isn't used // OR +const SETUP_SUCCESS = 0 + return SETUP_SUCCESSAdd null checks for dependencies
The function creates a NotificationHandler instance without validating its dependencies.
+ if (!logger || !request || !account) { + throw new Error('Missing required notification handler dependencies') + } const notificationHandler = new NotificationHandler({ logger, session: {}, request, account })
src/modules/webPushPrompt/prompt.js
Outdated
| StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true) | ||
| const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') | ||
| if (isNotificationPushCalled) { | ||
| processSoftPrompt() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for storage operations
Storage operations could fail (e.g., in private browsing mode). Consider adding try-catch blocks and fallback behavior.
- StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true)
- const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled')
- if (isNotificationPushCalled) {
- processSoftPrompt()
- }
+ try {
+ StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true)
+ const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled')
+ if (isNotificationPushCalled) {
+ processSoftPrompt()
+ }
+ } catch (error) {
+ logger?.error('Failed to process web push config:', error)
+ // Fallback: Attempt to process soft prompt anyway
+ processSoftPrompt()
+ }📝 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.
| StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true) | |
| const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') | |
| if (isNotificationPushCalled) { | |
| processSoftPrompt() | |
| } | |
| } | |
| try { | |
| StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true) | |
| const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') | |
| if (isNotificationPushCalled) { | |
| processSoftPrompt() | |
| } | |
| } catch (error) { | |
| logger?.error('Failed to process web push config:', error) | |
| // Fallback: Attempt to process soft prompt anyway | |
| processSoftPrompt() | |
| } | |
| } |
src/modules/notification.js
Outdated
| }).catch((error) => { | ||
| // unsubscribe from webpush if error | ||
| console.log('rejected subscription') |
src/modules/webPushPrompt/prompt.js
Outdated
| if (!(Object.keys(webPushConfig).length > 0)) { | ||
| notificationHandler.setApplicationServerKey(appServerKey) | ||
| notificationHandler.setupWebPush(displayArgs) | ||
| return 0 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/modules/webPushPrompt/prompt.js (3)
11-21: Consider refactoring global variables into a class.Global variables can make testing difficult and lead to maintainability issues. Consider encapsulating these variables and the
setNotificationHandlerValuesfunction within a class.-let logger = null -let account = null -let request = null -let displayArgs = null - -export const setNotificationHandlerValues = (notificationValues = {}) => { - logger = notificationValues.logger - account = notificationValues.account - request = notificationValues.request - displayArgs = notificationValues.displayArgs -} +class NotificationPromptHandler { + #logger = null; + #account = null; + #request = null; + #displayArgs = null; + + setValues(notificationValues = {}) { + this.#logger = notificationValues.logger; + this.#account = notificationValues.account; + this.#request = notificationValues.request; + this.#displayArgs = notificationValues.displayArgs; + } +}
50-67: Add error handling for storage operations.Storage operations could fail (e.g., in private browsing mode). Consider adding try-catch blocks.
export const processSoftPrompt = () => { + try { const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {} const notificationHandler = new NotificationHandler({ logger, session: {}, request, account }) if (!(Object.keys(webPushConfig).length > 0)) { notificationHandler.setApplicationServerKey(appServerKey) notificationHandler.setupWebPush(displayArgs) return } const { showBox, showBellIcon, boxType } = webPushConfig const { serviceWorkerPath, skipDialog } = parseDisplayArgs(displayArgs) if (showBellIcon || (showBox && boxType === 'new')) { enablePush(logger, account, request, serviceWorkerPath, skipDialog) } if (showBox && boxType === 'old') { notificationHandler.setApplicationServerKey(appServerKey) notificationHandler.setupWebPush(displayArgs) } + } catch (error) { + logger?.error('Failed to process soft prompt:', error) + } }
70-77: Add input validation for required fields.Consider validating the required fields in the display arguments to prevent runtime errors.
export const parseDisplayArgs = (displayArgs) => { + if (!displayArgs || !Array.isArray(displayArgs)) { + return { serviceWorkerPath: undefined, skipDialog: false } + } + if (displayArgs.length === 1 && isObject(displayArgs[0])) { const { serviceWorkerPath, skipDialog } = displayArgs[0] + if (serviceWorkerPath && typeof serviceWorkerPath !== 'string') { + logger?.warn('Invalid serviceWorkerPath type') + return { serviceWorkerPath: undefined, skipDialog: false } + } return { serviceWorkerPath, skipDialog } } return { serviceWorkerPath: undefined, skipDialog: displayArgs[5] } }src/modules/notification.js (3)
38-59: Add error handling for storage operations in push method.Storage operations could fail in private browsing mode. Consider adding try-catch blocks.
push (...displayArgs) { - const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') - setNotificationHandlerValues({ - logger: this.#logger, - account: this.#account, - request: this.#request, - displayArgs - }) - if (isWebPushConfigPresent) { - processSoftPrompt() - } else { - StorageManager.saveToLSorCookie('notificationPushCalled', true) + try { + const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') + setNotificationHandlerValues({ + logger: this.#logger, + account: this.#account, + request: this.#request, + displayArgs + }) + if (isWebPushConfigPresent) { + processSoftPrompt() + } else { + StorageManager.saveToLSorCookie('notificationPushCalled', true) + } + } catch (error) { + this.#logger?.error('Failed to process push notification:', error) } }
Line range hint
159-193: Refactor duplicate DOM cleanup logic.Consider extracting the repeated DOM cleanup logic into a helper function.
+ const cleanupElement = (elementId) => { + const element = document.getElementById(elementId) + if (element) { + element.parentNode.removeChild(element) + } + } + serviceWorkerRegistration.pushManager.subscribe(subscribeObj) .then((subscription) => { // ... existing code ... - const existingBellWrapper = document.getElementById('bell_wrapper') - if (existingBellWrapper) { - existingBellWrapper.parentNode.removeChild(existingBellWrapper) - } - if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) - } - if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) - } + cleanupElement('bell_wrapper') + cleanupElement('pnWrapper') + cleanupElement('wzrk_wrapper')
211-216: Reuse the cleanup helper function in error handling.The same DOM cleanup logic is duplicated. Use the suggested helper function here as well.
}).catch((error) => { // ... existing code ... - if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) - } - if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) - } + cleanupElement('pnWrapper') + cleanupElement('wzrk_wrapper') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/modules/notification.js(5 hunks)src/modules/webPushPrompt/prompt.js(3 hunks)
🔇 Additional comments (1)
src/modules/webPushPrompt/prompt.js (1)
36-47: LGTM! Good error handling implementation.The addition of try-catch block with fallback behavior improves reliability when storage operations fail.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
clevertap.js (1)
Line range hint
5336-6003: Architecture advice for web push notification flowThe current implementation could benefit from:
- Using a state machine pattern to manage the complex web push notification flow
- Implementing retry mechanisms with exponential backoff for failed web push registrations
- Adding telemetry/monitoring for web push success rates
- Considering a queue-based approach for handling race conditions
Would you like me to provide a detailed proposal for any of these improvements?
🧰 Tools
🪛 Biome (1.9.4)
[error] 5998-5998: 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)
src/modules/webPushPrompt/prompt.js (1)
12-17: Consider using a class to encapsulate global variables.Using global variables can make the code harder to maintain and test. Consider encapsulating these variables within a class.
-let logger = null -let account = null -let request = null -let displayArgs = null -let fcmPublicKey = null +class NotificationConfig { + constructor() { + this.logger = null + this.account = null + this.request = null + this.displayArgs = null + this.fcmPublicKey = null + } +} +const config = new NotificationConfig()src/modules/notification.js (1)
Line range hint
228-264: Consider extracting DOM cleanup logic into a separate function.The DOM cleanup logic is duplicated in both success and error cases. Consider extracting it into a reusable function.
+const cleanupSoftPromptElements = () => { + const softPromptCard = document.getElementById('pnWrapper') + const oldSoftPromptCard = document.getElementById('wzrk_wrapper') + const existingBellWrapper = document.getElementById('bell_wrapper') + + if (existingBellWrapper) { + existingBellWrapper.parentNode.removeChild(existingBellWrapper) + } + if (softPromptCard) { + softPromptCard.parentNode.removeChild(softPromptCard) + } + if (oldSoftPromptCard) { + oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) + } +} - const existingBellWrapper = document.getElementById('bell_wrapper') - if (existingBellWrapper) { - existingBellWrapper.parentNode.removeChild(existingBellWrapper) - } - if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) - } - if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) - } + cleanupSoftPromptElements() - if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) - } - if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) - } + cleanupSoftPromptElements()Also applies to: 282-287
src/clevertap.js (1)
551-557: Add error handling and improve state management.While the logic for handling web push configuration is correct, consider these improvements for better robustness:
- Add try-catch blocks for storage operations
- Add null checks for storage read operations
- Consider using a more atomic approach to handle state changes
Here's a suggested implementation:
- StorageManager.saveToLSorCookie('applicationServerKeyReceived', true) - this.notifications._enableWebPush(enabled, applicationServerKey) - const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') - const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') - if (isWebPushConfigPresent && isNotificationPushCalled) { - processSoftPrompt() - } + try { + StorageManager.saveToLSorCookie('applicationServerKeyReceived', true) + this.notifications._enableWebPush(enabled, applicationServerKey) + + const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') + const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') + + if (isWebPushConfigPresent && isNotificationPushCalled) { + processSoftPrompt() + } + } catch (error) { + this.#logger.error('Error in enableWebPush:', error) + }
📜 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 (4)
clevertap.js(15 hunks)src/clevertap.js(2 hunks)src/modules/notification.js(5 hunks)src/modules/webPushPrompt/prompt.js(10 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
clevertap.js
[error] 5998-5998: 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 (15)
clevertap.js (9)
5336-5338: LGTM: New setupWebPush method addedThe new method provides a clean interface to set up web push notifications by delegating to the private
_setUpWebPushmethod.
5341-5368: Improved web push notification handling with local storage flagsThe push method has been enhanced to handle potential race conditions using two local storage flags:
webPushConfigResponseReceived: Tracks if backend webPushConfig is receivednotificationPushCalled: Tracks if notifications.push was called before configThis ensures proper soft prompt rendering timing.
Line range hint
5559-5599: Cleanup of soft prompt elements on subscriptionThe code now properly cleans up both new and legacy soft prompt elements after successful subscription:
- Removes the new soft prompt card (
pnWrapper)- Removes the legacy soft prompt card (
wzrk_wrapper)This prevents stale UI elements from remaining in the DOM.
5620-5626: Consistent error handling for soft prompt cleanupThe code now handles cleanup of both new and legacy soft prompt elements even in error cases, ensuring a consistent user experience.
5881-5893: Improved notification handler initializationThe
setNotificationHandlerValuesfunction has been added to centralize the initialization of notification handler properties, making the code more maintainable.
5911-5922: Enhanced web push config processing with error handlingThe code now includes proper error handling when processing web push config and checking for notification push calls:
try { StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true); const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled'); if (isNotificationPushCalled) { processSoftPrompt(); } } catch (error) { logger.error('Failed to process web push config:', error); // Fallback: Attempt to process soft prompt anyway processSoftPrompt(); }
5925-5975: Comprehensive soft prompt processing logicThe
processSoftPromptfunction has been added to handle:
- Reading web push config from storage
- Initializing notification handler
- Processing different box types (new/old)
- Handling bell icon display
- Managing notification push state
This centralization improves code organization and maintainability.
5976-6003: Utility function for parsing display argumentsThe
parseDisplayArgsfunction has been added to handle different argument formats consistently:
- Single object with named parameters
- Array of positional parameters
This improves code readability and maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 5998-5998: 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)
9153-9162: Enhanced web push enablement with application server key handlingThe code now:
- Saves the application server key received flag
- Checks for both web push config and notification push call
- Processes soft prompt if both conditions are met
This ensures proper initialization sequence.
src/clevertap.js (3)
46-46: LGTM!The import statement correctly adds the required functions from the web push prompt module.
46-46: LGTM!The addition of
processSoftPromptimport aligns with the PR objective of integrating soft prompt functionality into the existing API.
551-557: Verify removal of notifications.enable API.Let's ensure there are no remaining references to the removed API in the codebase.
✅ Verification successful
Migration to notifications.push API is complete
The old
notifications.enableAPI has been completely removed from the codebase, and the new implementation properly handles web push notifications using the flag-based system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of notifications.enable API echo "Searching for notifications.enable usage..." rg -l "notifications\.enable" || echo "No usage found" # Search for potential migration issues echo "Checking for potential migration patterns..." rg -l "notifications\.push.*webPushConfig" || echo "No migration patterns found"Length of output: 369
Script:
#!/bin/bash # Check implementation in notification.js echo "=== Checking notification.js implementation ===" rg -A 5 "notifications\.push.*webPushConfig" src/modules/notification.js echo -e "\n=== Checking clevertap.js implementation ===" rg -A 5 "notifications\.push.*webPushConfig" clevertap.jsLength of output: 1267
src/modules/webPushPrompt/prompt.js (2)
77-90: LGTM!The parseDisplayArgs function correctly handles both object and array arguments with proper defaults.
53-75: 🛠️ Refactor suggestionAdd error handling for storage operations in processSoftPrompt.
The function performs storage operations without error handling. Consider adding try-catch blocks.
export const processSoftPrompt = () => { + try { const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {} notificationHandler = new NotificationHandler({ logger, session: {}, request, account }) if (!(Object.keys(webPushConfig).length > 0)) { notificationHandler.setApplicationServerKey(appServerKey) notificationHandler.setupWebPush(displayArgs) return } const { showBox, showBellIcon, boxType } = webPushConfig const { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback } = parseDisplayArgs(displayArgs) if (showBellIcon || (showBox && boxType === 'new')) { const enablePushParams = { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback, logger, request, account, fcmPublicKey } enablePush(enablePushParams) } if (showBox && boxType === 'old') { notificationHandler.setApplicationServerKey(appServerKey) notificationHandler.setupWebPush(displayArgs) } StorageManager.saveToLSorCookie('notificationPushCalled', false) StorageManager.saveToLSorCookie('applicationServerKeyReceived', false) + } catch (error) { + logger?.error('Failed to process soft prompt:', error) + } }Likely invalid or redundant comment.
src/modules/notification.js (1)
42-65: LGTM!The push method implementation is well-documented and handles the race condition between web push config and notification push calls effectively.
clevertap.js
Outdated
| serviceWorkerPath: undefined, | ||
| skipDialog: (_displayArgs$ = displayArgs[5]) !== null && _displayArgs$ !== void 0 ? _displayArgs$ : undefined, |
There was a problem hiding this comment.
Fix assignment in expression
The code contains an assignment within an expression which can be confusing and error-prone.
- serviceWorkerPath: (_displayArgs$ = displayArgs[5]) !== null && _displayArgs$ !== void 0 ? _displayArgs$ : undefined,
+ const _displayArgs$ = displayArgs[5];
+ serviceWorkerPath: _displayArgs$ !== null && _displayArgs$ !== void 0 ? _displayArgs$ : undefined,Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 5998-5998: 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)
src/clevertap.js
Outdated
| StorageManager.saveToLSorCookie('applicationServerKeyReceived', true) | ||
| this.notifications._enableWebPush(enabled, applicationServerKey) | ||
| const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') | ||
| const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') | ||
| if (isWebPushConfigPresent && isNotificationPushCalled) { | ||
| processSoftPrompt() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for storage operations.
Storage operations could fail in private browsing mode or when storage is full. Consider wrapping the storage operations in try-catch blocks.
- StorageManager.saveToLSorCookie('applicationServerKeyReceived', true)
- this.notifications._enableWebPush(enabled, applicationServerKey)
- const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived')
- const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled')
- if (isWebPushConfigPresent && isNotificationPushCalled) {
- processSoftPrompt()
- }
+ try {
+ StorageManager.saveToLSorCookie('applicationServerKeyReceived', true)
+ this.notifications._enableWebPush(enabled, applicationServerKey)
+ const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived')
+ const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled')
+ if (isWebPushConfigPresent && isNotificationPushCalled) {
+ processSoftPrompt()
+ }
+ } catch (error) {
+ logger.error('Failed to process web push setup:', error)
+ // Fallback: Enable web push without soft prompt
+ this.notifications._enableWebPush(enabled, applicationServerKey)
+ }📝 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.
| StorageManager.saveToLSorCookie('applicationServerKeyReceived', true) | |
| this.notifications._enableWebPush(enabled, applicationServerKey) | |
| const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') | |
| const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') | |
| if (isWebPushConfigPresent && isNotificationPushCalled) { | |
| processSoftPrompt() | |
| } | |
| try { | |
| StorageManager.saveToLSorCookie('applicationServerKeyReceived', true) | |
| this.notifications._enableWebPush(enabled, applicationServerKey) | |
| const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') | |
| const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') | |
| if (isWebPushConfigPresent && isNotificationPushCalled) { | |
| processSoftPrompt() | |
| } | |
| } catch (error) { | |
| logger.error('Failed to process web push setup:', error) | |
| // Fallback: Enable web push without soft prompt | |
| this.notifications._enableWebPush(enabled, applicationServerKey) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/modules/webPushPrompt/prompt.js (1)
Line range hint
232-268: Add XSS protection for DOM manipulation.The code directly manipulates DOM elements without sanitizing input, which could lead to XSS vulnerabilities.
Add input sanitization:
+const sanitizeId = (id) => { + return id.replace(/[^a-zA-Z0-9-_]/g, '') +} const softPromptCard = document.getElementById('pnWrapper') const oldSoftPromptCard = document.getElementById('wzrk_wrapper') serviceWorkerRegistration.pushManager.subscribe(subscribeObj) .then((subscription) => { // ... existing code ... const existingBellWrapper = document.getElementById('bell_wrapper') if (existingBellWrapper) { - existingBellWrapper.parentNode.removeChild(existingBellWrapper) + existingBellWrapper.remove() } if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) + softPromptCard.remove() } if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) + oldSoftPromptCard.remove() }
🧹 Nitpick comments (2)
src/modules/webPushPrompt/prompt.js (1)
12-16: Consider using TypeScript or JSDoc for better type safety.These global variables could benefit from type definitions to prevent potential type-related bugs and improve maintainability.
Add JSDoc type definitions:
+/** @type {string|null} */ let logger = null +/** @type {object|null} */ let account = null +/** @type {object|null} */ let request = null +/** @type {Array|null} */ let displayArgs = null +/** @type {string|null} */ let fcmPublicKey = nullsrc/modules/notification.js (1)
38-45: Improve method documentation and add return type.The setupWebPush method lacks proper documentation and return type.
Add JSDoc documentation:
+ /** + * Sets up web push notifications by calling the private setUpWebPush method + * @param {Array} displayArgs - Display arguments for web push setup + * @throws {Error} If displayArgs is invalid + * @returns {void} + */ setupWebPush (displayArgs) { /* A method in notification.js which can be accessed in prompt.js file to call the private method this.#setUpWebPush */ this.#setUpWebPush(displayArgs) }🧰 Tools
🪛 ESLint
[error] 39-39: Trailing spaces not allowed.
(no-trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/clevertap.js(2 hunks)src/modules/notification.js(5 hunks)src/modules/webPushPrompt/prompt.js(10 hunks)
🧰 Additional context used
🪛 ESLint
src/clevertap.js
[error] 553-553: Expected indentation of 8 spaces but found 10.
(indent)
[error] 554-554: Expected indentation of 8 spaces but found 10.
(indent)
[error] 555-555: Expected indentation of 8 spaces but found 10.
(indent)
[error] 556-556: Expected indentation of 8 spaces but found 10.
(indent)
[error] 557-557: Expected indentation of 10 spaces but found 12.
(indent)
[error] 558-558: Expected indentation of 8 spaces but found 10.
(indent)
[error] 559-559: Expected indentation of 6 spaces but found 8.
(indent)
[error] 560-560: Expected indentation of 8 spaces but found 10.
(indent)
[error] 561-561: Expected indentation of 6 spaces but found 8.
(indent)
src/modules/notification.js
[error] 39-39: Trailing spaces not allowed.
(no-trailing-spaces)
🔇 Additional comments (3)
src/modules/webPushPrompt/prompt.js (2)
39-49:⚠️ Potential issueFix trailing spaces and improve error handling.
The code contains trailing spaces and could benefit from better error handling.
Apply these fixes:
- /* - A method in notification.js which can be accessed in prompt.js file to call the - private method this.#setUpWebPush - */ + /** + * A method in notification.js which can be accessed in prompt.js file to call the + * private method this.#setUpWebPush + * @param {Array} displayArgs - Display arguments for web push setup + * @throws {Error} If displayArgs is invalid + */ setupWebPush (displayArgs) { + if (!Array.isArray(displayArgs)) { + throw new Error('displayArgs must be an array') + } this.#setUpWebPush(displayArgs) }Likely invalid or redundant comment.
53-75:⚠️ Potential issueAdd error handling for storage operations and improve type safety.
The function performs storage operations without error handling and type checking.
Add error handling and type checking:
export const processSoftPrompt = () => { + try { const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {} notificationHandler = new NotificationHandler({ logger, session: {}, request, account }) if (!(Object.keys(webPushConfig).length > 0)) { notificationHandler.setApplicationServerKey(appServerKey) notificationHandler.setupWebPush(displayArgs) return } const { showBox, showBellIcon, boxType } = webPushConfig const { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback } = parseDisplayArgs(displayArgs) if (showBellIcon || (showBox && boxType === 'new')) { const enablePushParams = { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback, logger, request, account, fcmPublicKey } enablePush(enablePushParams) } if (showBox && boxType === 'old') { notificationHandler.setApplicationServerKey(appServerKey) notificationHandler.setupWebPush(displayArgs) } StorageManager.saveToLSorCookie('notificationPushCalled', false) StorageManager.saveToLSorCookie('applicationServerKeyReceived', false) + } catch (error) { + logger?.error('Failed to process soft prompt:', error) + } }Likely invalid or redundant comment.
src/clevertap.js (1)
552-561:⚠️ Potential issueFix indentation and improve error handling.
The code has inconsistent indentation and could benefit from better error handling.
Fix indentation and improve error handling:
api.enableWebPush = (enabled, applicationServerKey) => { setServerKey(applicationServerKey) this.notifications._enableWebPush(enabled, applicationServerKey) try { - StorageManager.saveToLSorCookie('applicationServerKeyReceived', true) - const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') - const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') - if (isWebPushConfigPresent && isNotificationPushCalled) { - processSoftPrompt() - } + StorageManager.saveToLSorCookie('applicationServerKeyReceived', true) + const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') + const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') + if (isWebPushConfigPresent && isNotificationPushCalled) { + processSoftPrompt() + } } catch (error) { - this.#logger.error('Could not read value from local storage', error) + this.#logger.error('Could not read value from local storage', error) + // Attempt to continue with web push setup without soft prompt + this.notifications._enableWebPush(enabled, applicationServerKey) } }Likely invalid or redundant comment.
🧰 Tools
🪛 ESLint
[error] 553-553: Expected indentation of 8 spaces but found 10.
(indent)
[error] 554-554: Expected indentation of 8 spaces but found 10.
(indent)
[error] 555-555: Expected indentation of 8 spaces but found 10.
(indent)
[error] 556-556: Expected indentation of 8 spaces but found 10.
(indent)
[error] 557-557: Expected indentation of 10 spaces but found 12.
(indent)
[error] 558-558: Expected indentation of 8 spaces but found 10.
(indent)
[error] 559-559: Expected indentation of 6 spaces but found 8.
(indent)
[error] 560-560: Expected indentation of 8 spaces but found 10.
(indent)
[error] 561-561: Expected indentation of 6 spaces but found 8.
(indent)
| export const setNotificationHandlerValues = (notificationValues = {}) => { | ||
| logger = notificationValues.logger | ||
| account = notificationValues.account | ||
| request = notificationValues.request | ||
| displayArgs = notificationValues.displayArgs | ||
| fcmPublicKey = notificationValues.fcmPublicKey | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for notificationValues object.
The function accepts an object but doesn't validate its properties, which could lead to runtime errors.
Add property validation:
export const setNotificationHandlerValues = (notificationValues = {}) => {
+ if (typeof notificationValues !== 'object') {
+ logger?.error('notificationValues must be an object')
+ return
+ }
logger = notificationValues.logger
account = notificationValues.account
request = notificationValues.request
displayArgs = notificationValues.displayArgs
fcmPublicKey = notificationValues.fcmPublicKey
}📝 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.
| export const setNotificationHandlerValues = (notificationValues = {}) => { | |
| logger = notificationValues.logger | |
| account = notificationValues.account | |
| request = notificationValues.request | |
| displayArgs = notificationValues.displayArgs | |
| fcmPublicKey = notificationValues.fcmPublicKey | |
| } | |
| export const setNotificationHandlerValues = (notificationValues = {}) => { | |
| if (typeof notificationValues !== 'object') { | |
| logger?.error('notificationValues must be an object') | |
| return | |
| } | |
| logger = notificationValues.logger | |
| account = notificationValues.account | |
| request = notificationValues.request | |
| displayArgs = notificationValues.displayArgs | |
| fcmPublicKey = notificationValues.fcmPublicKey | |
| } |
src/modules/webPushPrompt/prompt.js
Outdated
| export const parseDisplayArgs = (displayArgs) => { | ||
| if (displayArgs.length === 1 && isObject(displayArgs[0])) { | ||
| const { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback } = displayArgs[0] | ||
| return { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback } | ||
| } | ||
|
|
||
| return { | ||
| serviceWorkerPath: undefined, | ||
| skipDialog: displayArgs[5], | ||
| okCallback: undefined, | ||
| subscriptionCallback: undefined, | ||
| rejectCallback: undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve input validation and error handling in parseDisplayArgs.
The function lacks proper input validation and error handling.
Add input validation and error handling:
export const parseDisplayArgs = (displayArgs) => {
+ if (!displayArgs || !Array.isArray(displayArgs)) {
+ logger?.error('Invalid displayArgs: must be an array')
+ return {
+ serviceWorkerPath: undefined,
+ skipDialog: false,
+ okCallback: undefined,
+ subscriptionCallback: undefined,
+ rejectCallback: undefined
+ }
+ }
if (displayArgs.length === 1 && isObject(displayArgs[0])) {
const { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback } = displayArgs[0]
+ if (okCallback && typeof okCallback !== 'function') {
+ logger?.error('Invalid okCallback: must be a function')
+ return null
+ }
return { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback }
}Committable suggestion skipped: line range outside the PR's diff.
src/modules/notification.js
Outdated
| push (...displayArgs) { | ||
| /* | ||
| To handle a potential race condition, two flags are stored in Local Storage: | ||
| - `webPushConfigResponseReceived`: Indicates if the backend's webPushConfig has been received (set during the initial API call without a session ID). | ||
| - `notificationPushCalled`: Tracks if `clevertap.notifications.push` was called before receiving the webPushConfig. | ||
|
|
||
| This ensures the soft prompt is rendered correctly: | ||
| - If `webPushConfigResponseReceived` is true, the soft prompt is processed immediately. | ||
| - Otherwise, `notificationPushCalled` is set to true, and the rendering is deferred until the webPushConfig is received. | ||
| */ | ||
| const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') | ||
| const isApplicationServerKeyReceived = StorageManager.readFromLSorCookie('applicationServerKeyReceived') | ||
| setNotificationHandlerValues({ | ||
| logger: this.#logger, | ||
| account: this.#account, | ||
| request: this.#request, | ||
| displayArgs, | ||
| fcmPublicKey: this.#fcmPublicKey | ||
| }) | ||
| if (isWebPushConfigPresent && isApplicationServerKeyReceived) { | ||
| processSoftPrompt() | ||
| } else { | ||
| StorageManager.saveToLSorCookie('notificationPushCalled', true) | ||
| } |
There was a problem hiding this comment.
Add error handling for race conditions.
The push method handles race conditions but lacks proper error handling.
Add error handling:
push (...displayArgs) {
+ try {
const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived')
const isApplicationServerKeyReceived = StorageManager.readFromLSorCookie('applicationServerKeyReceived')
setNotificationHandlerValues({
logger: this.#logger,
account: this.#account,
request: this.#request,
displayArgs,
fcmPublicKey: this.#fcmPublicKey
})
if (isWebPushConfigPresent && isApplicationServerKeyReceived) {
processSoftPrompt()
} else {
StorageManager.saveToLSorCookie('notificationPushCalled', true)
}
+ } catch (error) {
+ this.#logger.error('Failed to process push notification:', 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.
| push (...displayArgs) { | |
| /* | |
| To handle a potential race condition, two flags are stored in Local Storage: | |
| - `webPushConfigResponseReceived`: Indicates if the backend's webPushConfig has been received (set during the initial API call without a session ID). | |
| - `notificationPushCalled`: Tracks if `clevertap.notifications.push` was called before receiving the webPushConfig. | |
| This ensures the soft prompt is rendered correctly: | |
| - If `webPushConfigResponseReceived` is true, the soft prompt is processed immediately. | |
| - Otherwise, `notificationPushCalled` is set to true, and the rendering is deferred until the webPushConfig is received. | |
| */ | |
| const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') | |
| const isApplicationServerKeyReceived = StorageManager.readFromLSorCookie('applicationServerKeyReceived') | |
| setNotificationHandlerValues({ | |
| logger: this.#logger, | |
| account: this.#account, | |
| request: this.#request, | |
| displayArgs, | |
| fcmPublicKey: this.#fcmPublicKey | |
| }) | |
| if (isWebPushConfigPresent && isApplicationServerKeyReceived) { | |
| processSoftPrompt() | |
| } else { | |
| StorageManager.saveToLSorCookie('notificationPushCalled', true) | |
| } | |
| push (...displayArgs) { | |
| /* | |
| To handle a potential race condition, two flags are stored in Local Storage: | |
| - `webPushConfigResponseReceived`: Indicates if the backend's webPushConfig has been received (set during the initial API call without a session ID). | |
| - `notificationPushCalled`: Tracks if `clevertap.notifications.push` was called before receiving the webPushConfig. | |
| This ensures the soft prompt is rendered correctly: | |
| - If `webPushConfigResponseReceived` is true, the soft prompt is processed immediately. | |
| - Otherwise, `notificationPushCalled` is set to true, and the rendering is deferred until the webPushConfig is received. | |
| */ | |
| try { | |
| const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') | |
| const isApplicationServerKeyReceived = StorageManager.readFromLSorCookie('applicationServerKeyReceived') | |
| setNotificationHandlerValues({ | |
| logger: this.#logger, | |
| account: this.#account, | |
| request: this.#request, | |
| displayArgs, | |
| fcmPublicKey: this.#fcmPublicKey | |
| }) | |
| if (isWebPushConfigPresent && isApplicationServerKeyReceived) { | |
| processSoftPrompt() | |
| } else { | |
| StorageManager.saveToLSorCookie('notificationPushCalled', true) | |
| } | |
| } catch (error) { | |
| this.#logger.error('Failed to process push notification:', error) | |
| } |
src/modules/notification.js
Outdated
| if (isWebPushConfigPresent && isApplicationServerKeyReceived) { | ||
| processSoftPrompt() | ||
| } else { | ||
| StorageManager.saveToLSorCookie('notificationPushCalled', true) |
There was a problem hiding this comment.
Since this variable is used to defer the push call, consider renaming it to something like isNotificationPushCallDeferred to make its purpose more explicit. Using verb formats for boolean variables improves readability and aligns with common coding conventions.
|
|
||
| const { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback } = parseDisplayArgs(displayArgs) | ||
| if (showBellIcon || (showBox && boxType === 'new')) { | ||
| const enablePushParams = { serviceWorkerPath, skipDialog, okCallback, subscriptionCallback, rejectCallback, logger, request, account, fcmPublicKey } |
There was a problem hiding this comment.
In this function try naming the boolean conditions in variables for better readability and maintenance.
| @@ -34,14 +35,38 @@ export default class NotificationHandler extends Array { | |||
| this.#account = account | |||
| } | |||
|
|
|||
| push (...displayArgs) { | |||
| setupWebPush (displayArgs) { | |||
There was a problem hiding this comment.
Since we are exposing new function in a public API, please check if it can be avoided, in case there are some contraints try adding them in the comments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/modules/notification.js (1)
46-69:⚠️ Potential issueAdd error handling for race conditions.
The push method handles race conditions but lacks proper error handling.
Apply this diff to add error handling:
push (...displayArgs) { + try { const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') const isApplicationServerKeyReceived = StorageManager.readFromLSorCookie('applicationServerKeyReceived') setNotificationHandlerValues({ logger: this.#logger, account: this.#account, request: this.#request, displayArgs, fcmPublicKey: this.#fcmPublicKey }) if (isWebPushConfigPresent && isApplicationServerKeyReceived) { processSoftPrompt() } else { StorageManager.saveToLSorCookie('notificationPushCalled', true) } + } catch (error) { + this.#logger.error('Failed to process push notification:', error) + } }
🧹 Nitpick comments (4)
clevertap.js (2)
5345-5372: Consider adding retry mechanism for config fetchingWhile the current implementation handles the race condition well, consider adding a retry mechanism with timeout for fetching web push config to improve reliability.
const MAX_RETRIES = 3; const RETRY_DELAY = 2000; function fetchWebPushConfigWithRetry(retries = 0) { if (retries >= MAX_RETRIES) { logger.error('Failed to fetch web push config after max retries'); return; } try { // fetch config // on success, process config } catch (error) { setTimeout(() => { fetchWebPushConfigWithRetry(retries + 1); }, RETRY_DELAY); } }
5915-5926: Consider adding telemetry for error trackingTo better understand and debug issues in production, consider adding telemetry for tracking web push configuration errors.
function trackWebPushError(error, context) { const errorData = { error: error.message, context, timestamp: Date.now() }; // Send to analytics/monitoring system }src/modules/notification.js (2)
38-43: Consider reducing method visibility and improving documentation.While this method is needed for prompt.js, exposing a wrapper around a private method could leak implementation details. Consider:
- Documenting the method's purpose, parameters, and return type using JSDoc.
- Evaluating if this method could be made private or moved to a shared utility.
- setupWebPush (displayArgs) { - /* - A method in notification.js which can be accessed in prompt.js file to call the - private method this.#setUpWebPush - */ - this.#setUpWebPush(displayArgs) - } + /** + * Sets up web push notifications with the provided display arguments. + * @param {Object} displayArgs - The arguments for configuring the web push display + * @private + */ + setupWebPush (displayArgs) { + this.#setUpWebPush(displayArgs) + }
232-234: Extract duplicate cleanup code into a helper function.The soft prompt cleanup code is duplicated in both success and error handlers.
+ #cleanupSoftPromptElements() { + const softPromptCard = document.getElementById('pnWrapper') + const oldSoftPromptCard = document.getElementById('wzrk_wrapper') + if (softPromptCard) { + softPromptCard.parentNode.removeChild(softPromptCard) + } + if (oldSoftPromptCard) { + oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) + } + } serviceWorkerRegistration.pushManager.subscribe(subscribeObj) .then((subscription) => { // ... existing success handling code ... - if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) - } - if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) - } + this.#cleanupSoftPromptElements() }).catch((error) => { // ... existing error handling code ... - if (softPromptCard) { - softPromptCard.parentNode.removeChild(softPromptCard) - } - if (oldSoftPromptCard) { - oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) - } + this.#cleanupSoftPromptElements() })Also applies to: 263-268, 286-291
📜 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 (3)
clevertap.js(15 hunks)src/clevertap.js(2 hunks)src/modules/notification.js(5 hunks)
🔇 Additional comments (14)
clevertap.js (10)
5336-5342: LGTM! Method to expose privatesetUpWebPushfunctionalityThe
setupWebPushmethod provides a clean public interface to access the private#setUpWebPushmethod, maintaining proper encapsulation.
5345-5372: Improved web push notification handling with race condition preventionThe code now handles a potential race condition between receiving web push config and notification push calls by:
- Using local storage flags to track config and server key status
- Deferring soft prompt rendering until both are received
- Adding clear documentation of the approach
Line range hint
5563-5603: Cleanup of DOM elements after web push subscriptionThe code properly cleans up UI elements after successful web push subscription by removing:
- The soft prompt card
- The legacy soft prompt wrapper
- The bell icon wrapper
This prevents stale UI elements from persisting.
5624-5630: Proper error handling with UI cleanupThe error handler appropriately cleans up UI elements even when subscription fails, ensuring a good user experience.
5885-5897: Improved notification handler configurationThe
setNotificationHandlerValuesfunction centralizes the configuration of notification handler properties, making it easier to manage and modify notification settings.
5915-5926: Enhanced error handling for web push config processingThe code includes proper error handling when processing web push config:
- Logs errors appropriately
- Has a fallback mechanism to still process soft prompt
- Uses try-catch to prevent crashes
5929-5979: Well-structured soft prompt processing logicThe
processSoftPromptfunction:
- Properly checks for required configurations
- Handles different box types appropriately
- Cleans up storage flags after processing
- Has clear conditional logic for different prompt scenarios
5980-6005: Clean utility function for parsing display argumentsThe
parseDisplayArgsfunction:
- Handles both object and array argument formats
- Provides proper default values
- Has clear type handling
- Returns a consistent structure
6006-6018: Well-structured parameter destructuringThe code uses clean parameter destructuring to extract required values, making the code more readable and maintainable.
9157-9168: Improved web push enablement with proper error handlingThe code:
- Updates storage flags appropriately
- Checks for required conditions before processing
- Has proper error handling and logging
- Maintains backward compatibility
src/modules/notification.js (2)
11-11: LGTM!The import of utility functions from webPushPrompt/prompt aligns with the integration of soft prompt functionality into the push API.
68-68: Consider renaming the flag for better clarity.Since this variable is used to defer the push call, consider renaming it to something like
isNotificationPushCallDeferred.src/clevertap.js (2)
46-46: LGTM!The import statement correctly includes the necessary functions for web push setup.
552-561: LGTM! Error handling is properly implemented.The implementation includes:
- Proper error handling for storage operations
- Appropriate error logging
- Correct checks for web push configuration state
Changes
We had created notifications.enable api to call the new soft prompt, we have reverted back to the old api and made changes in notifications.push api itself.
We did this so that end users don't have to make changes to their code to call new and old api.
We have handled the race condition as well when user calls notification.push and when the sdk hasn't yet received the web push config from the backend
Changes to Public Facing API if any
Removed notifications.enable api
Made changes to notifications.push api
Added checks to see if webpushConfig is received from the backend and based on the web push config we render wither old or the new soft prompt
Please list the impact on the public facing API if any
Notification.enable is no longer supported. It was the new api which we had created to render new soft prompt. Since we removed the new api, rendering of new soft prompt is taken care by .push api.
How Has This Been Tested?
Describe the testing approach and any relevant configurations (e.g., environment, platform)
Tested on CtShoptesting by overriding clevertap.min.js
Tested on chetan/noc-testing website again by overriding clevertap.js
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor