Web 3309 patch - Fixed hardcoded feature identification with API check based logic #345
Conversation
WalkthroughThis pull request introduces version 1.12.0 of the CleverTap Web SDK, focusing on enhancing web push notification support for Safari browsers on Mac and iOS. The changes include new browser detection functions, updates to notification handling methods, and modifications to support Vapid Web Push. The SDK's versioning and changelog have been updated to reflect these improvements, with corrections to previous version dates and the addition of new feature support. Changes
Sequence DiagramsequenceDiagram
participant Browser
participant SDK
participant NotificationService
Browser->>SDK: Request Web Push
SDK->>SDK: Detect Browser Type
alt Safari Browser
SDK->>SDK: Check Native Web Push Support
SDK->>NotificationService: Set Up Safari Notifications
else Chrome/Firefox
SDK->>NotificationService: Set Up Standard Web Push
end
NotificationService->>Browser: Request Notification Permission
Browser-->>NotificationService: Permission Response
NotificationService->>SDK: Subscription Details
SDK->>Browser: Complete Push Setup
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🪛 LanguageToolCHANGELOG.md[inconsistency] ~9-~9: The suffix does not match the ordinal number. (ORDINAL_NUMBER_SUFFIX) 🔇 Additional comments (13)
✨ Finishing Touches
🪧 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🧹 Nitpick comments (11)
src/modules/notification.js (7)
12-12: Consider covering additional browsers
UsingisChrome,isFirefox, andisSafariis a good start for mainstream browsers. Keep in mind that some users may be using browsers like Edge or Opera, which often share the same basic capabilities as Chrome. If possible, handle or gracefully degrade for other browsers.
44-44: Add error handling or feedback
When callingenablePush(...), consider employing additional error handling or logging if there’s a failure during the process. This allows for quicker debugging or fallback logic if push enablement fails.
77-79: Check for older browser prefixes
#isNativeWebPushSupportedchecks for'PushManager' in window, which is typically sufficient, but older or unusual browsers might use prefixed or partial implementations. If you expect broad coverage, consider a fallback or specialized check for older browsers.
81-163: Unify Mac and iOS logic if possible
The#setUpSafariNotificationsmethod contains separate code paths for VAPID-based notifications vs. the legacy Safari web push approach. Consider consolidating common logic (like logging, storing subscription data, or invoking callbacks) across the two flows to reduce duplication and improve maintainability.
217-220: Handle other Chromium-based browsers
You’re settingsubscriptionData.browser = 'Chrome'or'Firefox'upon subscription. Note that browsers like Edge, Opera, and Brave will appear as Chromium-based, so you may want to refine these classifiers or handle them consistently.
344-352: Fallback for Edge or unknown browsers
WhenisChrome()/isFirefox()returns false yet the browser supports web push, the code short-circuits. If you must handle other browsers for a broader user base, consider extending this logic or providing a warning.
392-409: MakeaskAgainTimeInSecondsuser-configurable
You’re defaulting to 7 days ifaskAgainTimeInSecondsis not provided. This is reasonable, but you might consider making this setting easily configurable for more flexible user experiences.clevertap.js (2)
9286-9286: Ensure SDK version consistencyThe SDK version is hardcoded in multiple places. This should be maintained in a single location.
+ const SDK_VERSION = '1.12.0'; - return 'web-sdk-v1.12.0'; + return `web-sdk-v${SDK_VERSION}`; - lib: 'web-sdk-v1.12.0', + lib: `web-sdk-v${SDK_VERSION}`,
5747-5758: Improve notification frequency controlThe notification frequency control logic could be enhanced for better user experience.
- if (now - notifLastTime < askAgainTimeInSeconds) { - if (!isSafari()) { - return; - } - if (vapidSupportedAndMigrated) { - return; - } - } else { - StorageManager.setMetaProp(NOTIF_LAST_TIME, now); - } + const shouldShowNotification = () => { + if (now - notifLastTime < askAgainTimeInSeconds) { + // Only show for Safari with VAPID migration pending + return isSafari() && !vapidSupportedAndMigrated; + } + StorageManager.setMetaProp(NOTIF_LAST_TIME, now); + return true; + }; + + if (!shouldShowNotification()) { + return; + }This refactoring:
- Encapsulates notification timing logic
- Makes the code more readable and maintainable
- Reduces nested conditionals
src/modules/webPushPrompt/prompt.js (1)
130-145: Extract magic number to a named constant.The popup frequency default value of 7 days should be extracted to a named constant:
+const DEFAULT_POPUP_FREQUENCY_DAYS = 7; + if (shouldShowNotification) { if (!isSafari()) { document.body.appendChild(wrapper) if (!configData.isPreview) { StorageManager.setMetaProp('webpush_last_notif_time', now) addEventListeners(wrapper) } } else { const vapidSupportedAndNotMigrated = ('PushManager' in window) && !StorageManager.getMetaProp(VAPID_MIGRATION_PROMPT_SHOWN) && fcmPublicKey !== null if (vapidSupportedAndNotMigrated) { document.body.appendChild(wrapper) if (!configData.isPreview) { addEventListeners(wrapper) StorageManager.setMetaProp('webpush_last_notif_time', now) } } } }CHANGELOG.md (1)
9-9: Fix ordinal number format in date.Correct the date format:
-## [1.11.16] 20nd Jan, 2025 +## [1.11.16] 20th Jan, 2025🧰 Tools
🪛 LanguageTool
[inconsistency] ~9-~9: The suffix does not match the ordinal number.
Context: ...er Identification methods ## [1.11.16] 20nd Jan, 2025 - Fixed Web Inbox Notificatio...(ORDINAL_NUMBER_SUFFIX)
🛑 Comments failed to post (5)
clevertap.js (1)
5233-5245: 🛠️ Refactor suggestion
Improve browser detection functions with more robust checks
The browser detection functions could be enhanced for better reliability and maintainability.
const isChrome = () => { - const ua = navigator.userAgent; - return ua.includes('Chrome') || ua.includes('CriOS'); + // Check for both Chrome and CriOS (Chrome on iOS) but exclude Edge/Opera + return /Chrome|CriOS/.test(navigator.userAgent) && + !/Edg|OPR/.test(navigator.userAgent); }; const isFirefox = () => { - const ua = navigator.userAgent; - return ua.includes('Firefox') || ua.includes('FxiOS'); + // Check for both Firefox and FxiOS (Firefox on iOS) + return /Firefox|FxiOS/.test(navigator.userAgent); }; const isSafari = () => { - const ua = navigator.userAgent; - return ua.includes('Safari') && !ua.includes('CriOS') && !ua.includes('FxiOS') && !ua.includes('Chrome') && !ua.includes('Firefox'); + // More reliable Safari detection + return /^((?!chrome|android|crios|fxios).)*safari/i.test(navigator.userAgent); };The improvements include:
- More precise browser detection using regex patterns
- Better handling of edge cases like Edge/Opera browsers
- More maintainable and readable code structure
- Improved Safari detection to avoid false positives
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const isChrome = () => { // Check for both Chrome and CriOS (Chrome on iOS) but exclude Edge/Opera return /Chrome|CriOS/.test(navigator.userAgent) && !/Edg|OPR/.test(navigator.userAgent); }; const isFirefox = () => { // Check for both Firefox and FxiOS (Firefox on iOS) return /Firefox|FxiOS/.test(navigator.userAgent); }; const isSafari = () => { // More reliable Safari detection return /^((?!chrome|android|crios|fxios).)*safari/i.test(navigator.userAgent); };src/util/helpers.js (3)
6-9: 🛠️ Refactor suggestion
Consider using feature detection for Firefox.
Similar to Chrome, Firefox can be detected more reliably using feature detection:
export const isFirefox = () => { - const ua = navigator.userAgent - return ua.includes('Firefox') || ua.includes('FxiOS') + return typeof InstallTrigger !== 'undefined' }📝 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 isFirefox = () => { return typeof InstallTrigger !== 'undefined' }
1-4: 🛠️ Refactor suggestion
Consider using feature detection for Chrome.
Instead of relying on user agent strings which can be spoofed, consider using feature detection:
export const isChrome = () => { - const ua = navigator.userAgent - return ua.includes('Chrome') || ua.includes('CriOS') + return window.chrome !== undefined && navigator.vendor === 'Google Inc.' }📝 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 isChrome = () => { return window.chrome !== undefined && navigator.vendor === 'Google Inc.' }
11-18: 🛠️ Refactor suggestion
Consider using feature detection for Safari.
The current implementation handles false positives well, but feature detection would be more reliable:
export const isSafari = () => { - const ua = navigator.userAgent - // Ignoring the False Positive of Safari on iOS devices because it gives Safari in all Browsers - return ua.includes('Safari') && - !ua.includes('CriOS') && - !ua.includes('FxiOS') && - !ua.includes('Chrome') && - !ua.includes('Firefox') + return /^((?!chrome|android).)*safari/i.test(navigator.userAgent) && + 'safari' in window }Committable suggestion skipped: line range outside the PR's diff.
src/modules/webPushPrompt/prompt.js (1)
27-27: 🛠️ Refactor suggestion
Validate fcmPublicKey parameter.
Consider adding parameter validation to handle undefined/null cases explicitly:
-export const enablePush = (logger, account, request, customSwPath, skipDialog, fcmPublicKey) => { +export const enablePush = (logger, account, request, customSwPath, skipDialog, fcmPublicKey = null) => {📝 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 enablePush = (logger, account, request, customSwPath, skipDialog, fcmPublicKey = null) => {
Changes
This change fixes the issue where Chrome and Firefox IOS were not able to set up Web Push due to the hardcoded browser version check we were doing. This is replaced by a check which is API based which is more reliable and generic for all browsers.
Changes to Public Facing API if any
NA
How Has This Been Tested?
Dev Tested https://ctwebpush.netlify.app/?region=sk1-staging-4&accountId=W86-7WR-5K7Z on IOS Chrome, Safari and firefox
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores