Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removing enable api testing #338

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
080368b
removing enable api testing
rohitniyerclevertap Jan 21, 2025
517924d
Removed console
rohitniyerclevertap Jan 21, 2025
db719d5
check for swerviceworkerpath and skipdialog
rohitniyerclevertap Jan 22, 2025
7d16a5c
Testing, removed card when ebell icon rejects browser native
rohitniyerclevertap Jan 22, 2025
3a01657
[WEB-3414]: reverted to old api
rohitniyerclevertap Jan 28, 2025
c35a46e
resolved comments
rohitniyerclevertap Jan 28, 2025
2e26039
Merge branch 'master' of github.com:CleverTap/clevertap-web-sdk into …
rohitniyerclevertap Jan 29, 2025
253b652
Fixed appilcationServerKey, subscriptionCallback, rejectCallback, okC…
rohitniyerclevertap Jan 29, 2025
4922ba1
removed console
rohitniyerclevertap Jan 29, 2025
b697bd1
Code rabbit review comments, fixed changes
rohitniyerclevertap Jan 30, 2025
0bf9c64
Added comments
rohitniyerclevertap Jan 30, 2025
f2edf61
renmaed notificationPushCalled variable
rohitniyerclevertap Feb 3, 2025
6683770
Added apnsWebPushId and apnsWebPushServiceUrl
rohitniyerclevertap Feb 4, 2025
55658b5
Merge branch 'develop' of github.com:CleverTap/clevertap-web-sdk into…
rohitniyerclevertap Feb 6, 2025
d6c61b7
took pull from develop
rohitniyerclevertap Feb 6, 2025
e17cac1
Merge branch 'develop' of github.com:CleverTap/clevertap-web-sdk into…
rohitniyerclevertap Feb 6, 2025
8298d99
Created new build
rohitniyerclevertap Feb 6, 2025
5f93fa1
Merge branch 'develop' of github.com:CleverTap/clevertap-web-sdk into…
rohitniyerclevertap Feb 6, 2025
f9fa87d
Created new build
rohitniyerclevertap Feb 6, 2025
cdd46ca
Added apns migration and fcm cases
rohitniyerclevertap Feb 6, 2025
39c2ec9
New build for apns migration and fcm public key
rohitniyerclevertap Feb 6, 2025
91607ea
Fix for safari browser block
rohitniyerclevertap Feb 7, 2025
f4c45c3
debugginf kunal's website
rohitniyerclevertap Feb 21, 2025
9b8908e
soft prompt clevertap inti issue
rohitniyerclevertap Feb 24, 2025
6a70641
Merge branch 'develop' of github.com:CleverTap/clevertap-web-sdk into…
rohitniyerclevertap Feb 24, 2025
dce8cc6
rollup config not found error
rohitniyerclevertap Feb 24, 2025
efe2ff9
minor identation changes
rohitniyerclevertap Feb 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 108 additions & 11 deletions clevertap.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion clevertap.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion clevertap.min.js

Large diffs are not rendered by default.

38 changes: 32 additions & 6 deletions src/modules/notification.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
import {
urlBase64ToUint8Array
} from '../util/encoder'
import { enablePush } from './webPushPrompt/prompt'
import { setNotificationHandlerValues, processSoftPrompt } from './webPushPrompt/prompt'

export default class NotificationHandler extends Array {
#oldValues
Expand All @@ -31,14 +31,23 @@ export default class NotificationHandler extends Array {
this.#account = account
}

push (...displayArgs) {
setupWebPush (displayArgs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to segregate the functionality of handling old/new soft prompt in a new function in the file prompt.js.
In processSoftPrompt function which is in prompt.js we create an instance of notificationHandler class as we need to call setupwebpushnotification method as well as a private method #setupWebPush .

Since we can't access private methods I had to create a method setupWebPush in notification.js which can be accessed from prompt.js file. This method internally calls the private method this.#setupWebPush.

Even if I try to move the logic of rendering old/nwe soft prompt inside of a private method in notificationhandler class, there is a method processWebpushConfig which is called in tr.js and code is written in prompt.js. Then again there will be a problem to call this new private method which takes care of handling of old/new soft prompt from prompt.js as since it will be a private method in notification.js

We were anyway exposing push as a public method before too which calls this.#setupwebpush, I just created another public api setupwebpush which calls this.#setupWebpush.

this.#setUpWebPush(displayArgs)
return 0
}

enable (options = {}) {
const { swPath, skipDialog } = options
enablePush(this.#logger, this.#account, this.#request, swPath, skipDialog)
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

_processOldValues () {
Expand Down Expand Up @@ -138,6 +147,9 @@ export default class NotificationHandler extends Array {
subscribeObj.applicationServerKey = urlBase64ToUint8Array(this.#fcmPublicKey)
}

const softPromptCard = document.getElementById('pnWrapper')
const oldSoftPromptCard = document.getElementById('wzrk_wrapper')

serviceWorkerRegistration.pushManager.subscribe(subscribeObj)
.then((subscription) => {
this.#logger.info('Service Worker registered. Endpoint: ' + subscription.endpoint)
Expand All @@ -160,11 +172,19 @@ export default class NotificationHandler extends Array {
subscriptionCallback()
}
const existingBellWrapper = document.getElementById('bell_wrapper')

if (existingBellWrapper) {
existingBellWrapper.parentNode.removeChild(existingBellWrapper)
}
if (softPromptCard) {
softPromptCard.parentNode.removeChild(softPromptCard)
}
if (oldSoftPromptCard) {
oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard)
}
}).catch((error) => {
// unsubscribe from webpush if error
console.log('rejected subscription')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use logger

serviceWorkerRegistration.pushManager.getSubscription().then((subscription) => {
if (subscription !== null) {
subscription.unsubscribe().then((successful) => {
Expand All @@ -180,6 +200,12 @@ export default class NotificationHandler extends Array {
}
})
this.#logger.error('Error subscribing: ' + error)
if (softPromptCard) {
softPromptCard.parentNode.removeChild(softPromptCard)
}
if (oldSoftPromptCard) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add version Bump and Changelog.

oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard)
}
})
}).catch((err) => {
this.#logger.error('error registering service worker: ' + err)
Expand Down
50 changes: 50 additions & 0 deletions src/modules/webPushPrompt/prompt.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
import { getBellIconStyles, getBoxPromptStyles } from './promptStyles.js'
import { WEBPUSH_CONFIG } from '../../util/constants.js'
import { isObject } from '../../util/datatypes.js'
import { StorageManager, $ct } from '../../util/storage.js'
import NotificationHandler from '../notification.js'
import { BELL_BASE64, PROMPT_BELL_BASE64 } from './promptConstants.js'

let appServerKey = null
let swPath = '/clevertap_sw.js'
let notificationHandler = null
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
}

export const processWebPushConfig = (webPushConfig, logger, request) => {
const _pushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {}
Expand All @@ -21,7 +33,41 @@ export const processWebPushConfig = (webPushConfig, logger, request) => {
enablePush(logger, null, request)
} else if (JSON.stringify(_pushConfig) !== JSON.stringify(webPushConfig)) {
updatePushConfig()
StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true)
const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled')
if (isNotificationPushCalled) {
processSoftPrompt()
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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()
}
}

}

export const processSoftPrompt = () => {
const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {}
const notificationHandler = new NotificationHandler({ logger, session: {}, request, account })
if (!(Object.keys(webPushConfig).length > 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a null check on webPushConfig before the Object.keys as it might lead to runtime error, Also use Optional chaining for failing gracefully. Something like

webPushConfig && Object.keys(webPushConfig)?.length > 0

notificationHandler.setApplicationServerKey(appServerKey)
notificationHandler.setupWebPush(displayArgs)
return 0
Copy link
Contributor

@ThisIsRaghavGupta ThisIsRaghavGupta Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 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)
}
}

export const parseDisplayArgs = (displayArgs) => {
if (displayArgs.length === 1 && isObject(displayArgs[0])) {
const { serviceWorkerPath, skipDialog } = displayArgs[0]
return { serviceWorkerPath, skipDialog }
}

return { serviceWorkerPath: undefined, skipDialog: displayArgs[5] }
}

export const enablePush = (logger, account, request, customSwPath, skipDialog) => {
Expand All @@ -34,6 +80,10 @@ export const enablePush = (logger, account, request, customSwPath, skipDialog) =

if (customSwPath) { swPath = customSwPath }

if (skipDialog === null) {
skipDialog = false
}

notificationHandler = new NotificationHandler({ logger, session: {}, request, account })

if (skipDialog) {
Expand Down