Skip to content

Conversation

@Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Nov 17, 2025

Description

Removes useRegisterPushNotificationsEffect and related unused code, including:

  • hasInitialNotification from app/util/notifications/methods/common.ts
  • getInitialNotification and onBackgroundEvent from app/util/notifications/services/NotificationService.ts
  • useRegisterPushNotificationsEffect.ts and its test file
  • The usage of useRegisterPushNotificationsEffect in useNotificationHandler

Changelog

CHANGELOG entry: null

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-1552

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Removes useRegisterPushNotificationsEffect and initial/background notification handlers/utilities, updating hooks, tests, and NotificationService accordingly.

  • Notifications hooks:
    • Remove useRegisterPushNotificationsEffect and its test file.
    • Stop invoking it in hooks/index.ts; only call useStartupNotificationsEffect.
    • Update hooks/index.test.tsx to assert only useStartupNotificationsEffect is invoked.
  • NotificationService (services/NotificationService.ts):
    • Remove getInitialNotification and onBackgroundEvent wrappers and related imports/types.
    • Keep onForegroundEvent, badge count helpers, and display flow intact.
  • Methods (methods/common.ts):
    • Remove notifee import and hasInitialNotification helper.
    • Retain withTimeout and other utilities unchanged.

Written by Cursor Bugbot for commit 7b1c858. This will update automatically on new commits. Configure here.

Removes `useRegisterPushNotificationsEffect` and related unused code, including:
- `hasInitialNotification` from `app/util/notifications/methods/common.ts`
- `getInitialNotification` and `onBackgroundEvent` from `app/util/notifications/services/NotificationService.ts`
- `useRegisterPushNotificationsEffect.ts` and its test file
- The usage of `useRegisterPushNotificationsEffect` in `useNotificationHandler`
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review November 18, 2025 09:02
@Prithpal-Sooriya Prithpal-Sooriya requested a review from a team as a code owner November 18, 2025 09:02
* Lists notifications on startup.
*/
const useNotificationHandler = () => {
useRegisterPushNotificationsEffect();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This effect is not needed anymore.
We register push notifications through the FCMService, and this is done at an engine level.

@github-actions
Copy link
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeAccounts, SmokeCore, SmokeConfirmationsRedesigned, SmokeIdentity, SmokeNetworkAbstractions, SmokeNetworkExpansion, SmokeTrade, SmokeWalletPlatform, SmokeWalletUX, SmokeAssets, SmokeSwaps, SmokeStake, SmokeCard, SmokeNotifications, SmokeRewards, SmokePerps, SmokeRamps, SmokeMultiChainPermissions, SmokeAnalytics, SmokeMultiChainAPI, SmokePredictions
  • Risk Level: high
  • AI Confidence: %
click to see 🤖 AI reasoning details

Fallback: AI analysis did not complete successfully. Running all tests.

View GitHub Actions results

return formatAmount(numericAmount);
};

export const hasInitialNotification = async () =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is not used anymore, we've consolidated getting the first initial push notification on the engine level.

observer: (event: NotifeeEvent) => Promise<void>,
): (() => void) => notifee.onForegroundEvent(observer);

onBackgroundEvent = (observer: (event: NotifeeEvent) => Promise<void>) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not used anymore. We are not using notifee for background push notifications but directly firebase.

await notifee.cancelTriggerNotification(id);
};

getInitialNotification = async (): Promise<InitialNotification | null> =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not used anymore. We are not using notifee for getting the initial push notification but directly firebase.

@sonarqubecloud
Copy link

@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Nov 18, 2025
Merged via the queue into main with commit 2ee8a9f Nov 18, 2025
95 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the cleanup/remove-old-notification-code branch November 18, 2025 10:07
@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2025
@metamaskbot metamaskbot added the release-7.61.0 Issue or pull request that will be included in release 7.61.0 label Nov 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-7.61.0 Issue or pull request that will be included in release 7.61.0 size-M team-assets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants