Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 300
feat: update notification-services-controller to support new Segment schema#8944
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
602241e
feat: update notification-services-controller to support new Segment …
zelkibuilds b765c4a
fix: getNotificationSubtype
zelkibuilds 16656ed
chore: update comments after re-generating types
zelkibuilds e3aabce
test: add missing case to subtype helper suite
zelkibuilds 5ec0e5b
chore: fix CI changelog check
zelkibuilds d94df65
feat: add subtype to base notification
zelkibuilds 3400f69
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds e68d492
chore: cleanup
zelkibuilds 81b5f29
feat: move toPushAnalytics from web to package utils
zelkibuilds 5620ede
feat: update get subtype helper + notifications api schema
zelkibuilds b68e461
chore: update changelog
zelkibuilds 31d7e96
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds 97af197
fix: format CHANGELOG.md
zelkibuilds 3af52b7
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds 8e2437b
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds 6cc5f39
fix: remove profile_id from push analytics
zelkibuilds 8f815b0
test: remove profile_id from missing test
zelkibuilds 24f6466
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds bb35b41
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds 1590150
fix: handle new push while in-flight request
zelkibuilds 3d7cd4b
fix: rename isOnChainNotification to isInboxAPINotification
zelkibuilds 7e1a18c
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds 069d5ce
Merge branch 'main' into feat/update-notifications-controller
zelkibuilds 9211e3b
chore: fix changelog entry
zelkibuilds ae4885f
fix: rename isInboxAPINotification to isAPINotification
zelkibuilds File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
47 changes: 47 additions & 0 deletions
47 ...ices-controller/src/NotificationServicesController/NotificationServicesController.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
27 changes: 25 additions & 2 deletions
27 ...-services-controller/src/NotificationServicesController/NotificationServicesController.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 1 addition & 0 deletions
1 packages/notification-services-controller/src/NotificationServicesController/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2 ...ces-controller/src/NotificationServicesController/processors/process-api-notifications.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2 ...-controller/src/NotificationServicesController/processors/process-feature-announcement.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 2 additions & 2 deletions
4 ...ervices-controller/src/NotificationServicesController/processors/process-notifications.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2 ...es-controller/src/NotificationServicesController/processors/process-snap-notifications.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2 ...services-controller/src/NotificationServicesController/types/notification/notification.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39 ...ices-controller/src/NotificationServicesController/utils/get-notification-subtype.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { TRIGGER_TYPES } from '../constants/notification-schema'; | ||
| import { createMockFeatureAnnouncementRaw } from '../mocks/mock-feature-announcements'; | ||
| import { | ||
| createMockNotificationEthReceived, | ||
| createMockPlatformNotification, | ||
| } from '../mocks/mock-raw-notifications'; | ||
| import { createMockSnapNotification } from '../mocks/mock-snap-notification'; | ||
| import { processNotification } from '../processors/process-notifications'; | ||
| import { getNotificationSubtype } from './get-notification-subtype'; | ||
| describe('getNotificationSubtype', () => { | ||
| it('returns the trigger kind for on-chain notifications', () => { | ||
| const notification = processNotification( | ||
| createMockNotificationEthReceived(), | ||
| ); | ||
| expect(getNotificationSubtype(notification)).toBe( | ||
| TRIGGER_TYPES.ETH_RECEIVED, | ||
| ); | ||
| }); | ||
| it('returns the server-set notification_subtype for platform notifications', () => { | ||
| const notification = processNotification(createMockPlatformNotification()); | ||
| expect(getNotificationSubtype(notification)).toBe('position_liquidated'); | ||
| }); | ||
| it('returns the snap subtype for snap notifications', () => { | ||
| const notification = processNotification(createMockSnapNotification()); | ||
| expect(getNotificationSubtype(notification)).toBe(TRIGGER_TYPES.SNAP); | ||
| }); | ||
| it('returns a stable label for feature-announcement notifications', () => { | ||
| const notification = processNotification( | ||
| createMockFeatureAnnouncementRaw(), | ||
| ); | ||
| expect(getNotificationSubtype(notification)).toBe( | ||
| TRIGGER_TYPES.FEATURES_ANNOUNCEMENT, | ||
| ); | ||
| }); | ||
| }); |
34 changes: 34 additions & 0 deletions
34 ...-services-controller/src/NotificationServicesController/utils/get-notification-subtype.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { isOnChainRawNotification } from '../../shared/is-onchain-notification'; | ||
| import { TRIGGER_TYPES } from '../constants/notification-schema'; | ||
| import type { RawNotificationUnion } from '../types/notification/notification'; | ||
| /** | ||
| * Derives the normalised `notification_subtype` for a processed in-app | ||
| * notification. This is the team-owned axis (e.g. `eth_received`) and is | ||
| * always derivable from an `INotification`, so every consumer (both clients) | ||
| * pulls it from one place rather than recomputing a fallback chain. | ||
| * | ||
| * - on-chain: the trigger kind (`payload.data.kind`, e.g. `eth_received`). | ||
| * - platform: the server-set `notification_subtype` from the inbox API. | ||
| * - everything else (snap, feature-announcement): the top-level `type` | ||
| * (`snap` / `features_announcement`). | ||
| * | ||
| * @param notification - a raw or processed notification. | ||
| * @returns the normalised subtype string. | ||
| */ | ||
| export function getNotificationSubtype( | ||
| notification: RawNotificationUnion, | ||
| ): string { | ||
| // On-chain: the trigger kind (e.g. `eth_received`). | ||
| if (isOnChainRawNotification(notification)) { | ||
| return notification.payload.data.kind; | ||
| } | ||
| // Platform: the server-set `notification_subtype` from the inbox API. | ||
| if (notification.type === TRIGGER_TYPES.PLATFORM) { | ||
| return notification.notification_subtype; | ||
cursor[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| // Fallback (snap, feature-announcement): the top-level `type`. | ||
| return notification.type; | ||
| } | ||
7 changes: 3 additions & 4 deletions
7 ...s-controller/src/NotificationServicesPushController/NotificationServicesPushController.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 1 addition & 0 deletions
1 ...es/notification-services-controller/src/NotificationServicesPushController/types/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| export type * from './firebase'; | ||
| export type * from './push-analytics'; | ||
| export type * from './push-service-interface'; |
20 changes: 20 additions & 0 deletions
20 ...cation-services-controller/src/NotificationServicesPushController/types/push-analytics.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // snake_case mirrors the FCM payload and Segment schema keys | ||
| /* eslint-disable @typescript-eslint/naming-convention */ | ||
| /** | ||
| * Analytics fields carried by the `NotificationServicesPushController` messenger | ||
| * events (`onNewNotifications`, `pushNotificationClicked`). Read directly from | ||
| * top-level FCM payload keys, so clients build Segment events without fallback | ||
| * chains or parsing a `metadata` blob. | ||
| */ | ||
| export type PushAnalyticsPayload = { | ||
| notification_id: string; | ||
| /** Free-form snake_case label set by the producer. */ | ||
| notification_type: string; | ||
| /** Team-owned, open-ended (e.g. `eth_received`). */ | ||
| notification_subtype: string; | ||
| /** Only present when the notification has a chain context. */ | ||
| chain_id?: number; | ||
| /** Platform notifications only; the CTA link to route to on tap. */ | ||
| deeplink?: string; | ||
| }; |
1 change: 1 addition & 0 deletions
1 ...es/notification-services-controller/src/NotificationServicesPushController/utils/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| export * from './get-notification-data'; | ||
| export * from './get-notification-message'; | ||
| export * from './to-push-analytics-payload'; |
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a small getNotificationType helper to avoid the confusion between subtypes and types in the clients?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getNotificationSubtypeexists becausenotification_subtypeneeds to be derived and has to be computed from different source fields depending on the shape.notification.type, on the other hand, is always set upstream and directly readable on every notification shape, so there's nothing to derive unless I'm missing some context.Would
getNotificationTypemainly be for ergonomics? Giving clients a consistent helper to read type the same way they'd callgetNotificationSubtypefor subtype?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's my suggestion. But feel free not to add it if you don't find it relevant