Conversation
WalkthroughThe changes add push notification tap tracking functionality by introducing Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe670fe2fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| async trackPushTapped(data: Record<string, any>): Promise<void> { | ||
| const clixPayload = this.parseClixPayload(data); |
There was a problem hiding this comment.
trackPushTapped reuses parseClixPayload, which rejects payloads unless message_id, title, and body are all present; for the third-party notification use case this API targets, tap callbacks often provide only tracking identifiers in data while title/body live outside that object, so this path now throws and drops PUSH_NOTIFICATION_TAPPED events instead of recording them. Because eventService.trackEvent only needs the IDs, this validation is stricter than necessary for manual tap tracking and breaks the new feature for those payload shapes.
Useful? React with 👍 / 👎.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/NotificationService.ts`:
- Around line 440-456: trackPushTapped currently always emits
PUSH_NOTIFICATION_TAPPED and can double-count taps already recorded by
handleNotificationOpenedApp / handleInitialNotification /
handleNotificationEvent or by Clix.Notification.trackPushNotificationTapped();
modify trackPushTapped to call a shared dedupe helper (e.g.,
ensureNotificationTapNotTracked(messageId) or reuse existing sessionService
pending-message logic) that checks the messageId and returns early if already
tracked, otherwise marks it tracked and proceeds to call eventService.trackEvent
with the same parameters; reference the trackPushTapped method and the other
handlers (handleNotificationOpenedApp, handleInitialNotification,
handleNotificationEvent, Clix.Notification.trackPushNotificationTapped) so the
dedupe helper is used by both code paths to ensure idempotent tracking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03cb93aa-bf0c-4d9a-a159-97f4a7feea57
📒 Files selected for processing (2)
src/core/ClixNotification.tssrc/services/NotificationService.ts
| async trackPushTapped(data: Record<string, any>): Promise<void> { | ||
| const clixPayload = this.parseClixPayload(data); | ||
| if (!clixPayload) { | ||
| throw new Error( | ||
| 'No valid Clix payload found in notification data. ' + | ||
| "Ensure the notification data contains a 'clix' key with message_id, title, and body." | ||
| ); | ||
| } | ||
| this.sessionService?.setPendingMessageId(clixPayload.messageId); | ||
| await this.eventService.trackEvent( | ||
| 'PUSH_NOTIFICATION_TAPPED', | ||
| {}, | ||
| clixPayload.messageId, | ||
| clixPayload.userJourneyId, | ||
| clixPayload.userJourneyNodeId, | ||
| 'CLIX' | ||
| ); |
There was a problem hiding this comment.
Make tap tracking idempotent across manual and automatic handlers.
This new path always emits PUSH_NOTIFICATION_TAPPED, but the SDK already does the same from handleNotificationOpenedApp, handleInitialNotification, and handleNotificationEvent. Because Clix.Notification.trackPushNotificationTapped() only runs after initialization, a cold-start tap that handleInitialNotification() already captured can be counted a second time when the app also calls this API. Please route both paths through one messageId-based dedupe helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/NotificationService.ts` around lines 440 - 456, trackPushTapped
currently always emits PUSH_NOTIFICATION_TAPPED and can double-count taps
already recorded by handleNotificationOpenedApp / handleInitialNotification /
handleNotificationEvent or by Clix.Notification.trackPushNotificationTapped();
modify trackPushTapped to call a shared dedupe helper (e.g.,
ensureNotificationTapNotTracked(messageId) or reuse existing sessionService
pending-message logic) that checks the messageId and returns early if already
tracked, otherwise marks it tracked and proceeds to call eventService.trackEvent
with the same parameters; reference the trackPushTapped method and the other
handlers (handleNotificationOpenedApp, handleInitialNotification,
handleNotificationEvent, Clix.Notification.trackPushNotificationTapped) so the
dedupe helper is used by both code paths to ensure idempotent tracking.
1 participant
Summary
ClixNotification.trackPushNotificationTapped(data)public API that allows users to manually report push notification tapsgetInitialNotification()returns null because the notification is consumed before the Clix SDK can detect itChanges
NotificationService.trackPushTapped(data)— parses clix payload from raw notification data and tracks the eventClixNotification.trackPushNotificationTapped(data)— public API that delegates to NotificationServiceUsage
Test plan
trackPushNotificationTappedwith valid clix payload — event should be trackedsource_type: CLIXSummary by CodeRabbit