Skip to content

Conversation

@jagdish-gh
Copy link

@jagdish-gh jagdish-gh commented Oct 28, 2025

Changes

Fixed a bug in the CleverTap Web SDK notification service where the subscriptionCallback function was not being called during push notification re-registration when a different FCM key exists.

Jira Issue Reference: [Add your Jira issue reference here]

Root Cause

When a different FCM key exists than the current push subscription, the subscription fails and the SDK unregisters and registers again. However, during this re-registration process in the error handling flow, the subscriptionCallback parameter was not being passed to the notifications.push() call, causing the callback function to not be executed after successful re-registration.

Changes Made

  • File: src/modules/notification.js
  • Line 328-331: Added subscriptionCallback: subscriptionCallback parameter to the window.clevertap.notifications.push() call in the unsubscription error handling flow
  • This ensures that when the SDK re-registers after unsubscription due to FCM key mismatch, the subscription callback is properly executed

Changes to Public Facing API if any

No changes to the public facing API. This is a bug fix that ensures existing functionality works as expected.

How Has This Been Tested?

Testing Approach:

  1. FCM Key Mismatch Scenario:

    • Set up a web push subscription with one FCM key
    • Change the FCM key to a different value
    • Trigger push notification registration
    • Verify that the subscription fails, unsubscribes, and re-registers
    • Confirm that the subscriptionCallback function is called after successful re-registration
  2. Regression Testing:

    • Test normal push notification flow without FCM key changes
    • Verify existing functionality remains intact
    • Test with different browsers (Chrome, Firefox, Safari)

Test Configuration:

  • Environment: Staging
  • Platforms: Chrome, Firefox, Safari
  • Test scenarios: FCM key mismatch, normal registration flow

Checklist

  • Code compiles without errors
  • Version Bump added to package.json & CHANGELOG.md
  • All tests pass
  • Build process is successful
  • Documentation has been updated (if needed)
  • Automation tests are passing

Link to Deployed SDK

Use these URLs for testing:

  1. https://static.wizrocket.com/staging/fix/notification-subscription-callback-missing-on-reregister/js/clevertap.min.js
  2. https://static.wizrocket.com/staging/fix/notification-subscription-callback-missing-on-reregister/js/sw_webpush.min.js

How to trigger Automations

Just add an empty commit after all your changes are done in the PR with the command:

git commit --allow-empty -m "[run-test] Testing Automation"

This will trigger the automation suite.

Summary by CodeRabbit

  • Bug Fixes
    • Improved Web Push notification handling for Safari during subscription management.

…e-registration

When a different FCM key exists and push subscription fails, the SDK unregisters
and registers again. However, during the re-registration process, the
subscriptionCallback function was not being passed to the notifications.push()
call, causing the callback to not be executed.

This fix ensures that the subscriptionCallback parameter is included when
calling notifications.push() during the unsubscription error handling flow,
allowing the callback to be properly executed after successful re-registration.

Fixes issue where subscriptionCallback was not called after FCM key change
and subsequent push notification re-registration.
@jagdish-gh jagdish-gh requested a review from a team as a code owner October 28, 2025 02:26
@francispereira
Copy link

francispereira commented Oct 28, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Walkthrough

The code now passes an additional subscriptionCallback field within a notification push during the Safari-based Web Push setup error path when unsubscribing from a subscription, alongside the existing skipDialog: true flag.

Changes

Cohort / File(s) Change Summary
Web Push Unsubscribe Callback
src/modules/notification.js
Added subscriptionCallback parameter to notification push in Safari Web Push error-handling unsubscribe flow alongside skipDialog: true

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Single-file, single-location change adding one field to an existing notification push
  • No control flow modifications or complex logic alterations
  • Straightforward parameter addition in error path

Possibly related issues

Possibly related PRs

  • removing enable api testing #338: Also modifies src/modules/notification.js targeting web-push subscription and error handling, including callback passage during soft-prompt and unsubscribe scenarios.

Suggested reviewers

  • kkyusuftk
  • singhkunal2050

Poem

🐰 A hop, a skip, when subscriptions unwind,
The callback now follows, left none behind,
Safari's soft prompt with a graceful salute,
Error paths tidy, the fix absolute! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: ensure subscriptionCallback is passed during push notification r…" directly summarizes the main change in the pull request. The change adds the subscriptionCallback parameter to the window.clevertap.notifications.push() call in the error handling flow during push notification re-registration, which is exactly what the title conveys. The title is concise, clear, and specific enough for a teammate scanning history to understand the primary fix without being vague or overly broad.
Description Check ✅ Passed The pull request description is mostly complete and follows the required template structure. It includes all major sections: Changes (with detailed explanation), Changes to Public Facing API, How Has This Been Tested (with specific testing scenarios and configuration), Checklist, Link to Deployed SDK, and How to trigger Automations. The only notable gap is that the Jira Issue Reference in the Changes section contains a placeholder "[Add your Jira issue reference here]" rather than an actual issue reference, but this is a non-critical metadata item compared to the substantial technical content provided. The description demonstrates thorough planning and testing approach.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f364359 and fa91668.

📒 Files selected for processing (1)
  • src/modules/notification.js (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/notification.js (1)
clevertap.js (1)
  • subscriptionCallback (11373-11373)
🔇 Additional comments (2)
src/modules/notification.js (2)

328-331: AI summary incorrectly identifies this as Safari code path.

The AI summary states this is the "Safari-based Web Push setup error path," but this code is actually in the #setUpChromeFirefoxNotifications method (line 247), which handles Chrome and Firefox browsers. Safari notifications are handled separately in #setUpSafariNotifications (line 127).


328-331: Fix verified - callback properly flows through re-registration path.

The addition of subscriptionCallback: subscriptionCallback correctly ensures the callback is passed during re-registration. Flow verification shows: the callback is extracted from the push parameters (line 405), routed through setUpWebPushNotifications when skipDialog: true (line 475), and invoked with proper type validation in the browser-specific handlers (lines 155, 307).


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants