Skip to content
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

fix: Notifications platform links #24708

Merged
merged 30 commits into from
Jun 7, 2024

Conversation

matteoscurati
Copy link
Contributor

@matteoscurati matteoscurati commented May 22, 2024

Description

This PR introduces the logic to handle links within notifications to support the different platforms on which we can use the extension.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Open the notifications list page
  2. Click on a product announcement
  3. Check the link in the detail page

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

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.

@matteoscurati matteoscurati changed the base branch from develop to feat/new-metamask-notifications May 22, 2024 12:36
@matteoscurati matteoscurati changed the title Feat/notifications platform links Notifications platform links May 22, 2024
@matteoscurati matteoscurati marked this pull request as ready for review May 22, 2024 12:36
@matteoscurati matteoscurati requested a review from a team as a code owner May 22, 2024 12:36
@@ -67,7 +69,12 @@ export async function onNotificationClick(event: NotificationEvent) {
const data: OnChainRawNotification = event?.notification?.data;

// Navigate
const destination = `chrome-extension://${sw.location.host}/home.html#notifications/${data.id}`;
const getPlatformResult = getPlatform();
Copy link
Contributor

Choose a reason for hiding this comment

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

Double check this getPlatform code.
This calls window.navigator, and the window does not exist on service workers.
We might want to replace this with global.navigator or self.navigator

@danjm is this function getPlatform used in other controllers? We are not seeing any regressions or errors using this? If it works, then feel free to keep using it @matteoscurati .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Prithpal-Sooriya everything seems to be working correctly. I tried generating the links by also printing the different steps and everything seems to work

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this: https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/init-globals.js#L17

window, in our app/ code under mv3, refers to the ServiceWorkerGlobalScope and not an actual window. The ServiceWorkerGlobalScope has some, but not all, of the apis an actual window has. navigator is one of the APIs it does have, so this should be fine

Base automatically changed from feat/new-metamask-notifications to develop May 23, 2024 12:11
@matteoscurati matteoscurati dismissed Prithpal-Sooriya’s stale review May 23, 2024 12:11

The base branch was changed.

@matteoscurati matteoscurati requested review from a team as code owners May 23, 2024 12:11
@matteoscurati matteoscurati changed the base branch from develop to feat/new-metamask-notifications May 23, 2024 15:32
@matteoscurati matteoscurati force-pushed the feat/notifications-platform-links branch from 2b6e4ac to 4fe125f Compare May 23, 2024 15:40
@matteoscurati matteoscurati requested review from kumavis and a team as code owners May 23, 2024 15:40
@matteoscurati matteoscurati changed the base branch from feat/new-metamask-notifications to develop May 23, 2024 15:41
@Prithpal-Sooriya Prithpal-Sooriya self-requested a review May 28, 2024 17:45
@metamaskbot
Copy link
Collaborator

Builds ready [5ac245e]
Page Load Metrics (1316 ± 596 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint821491122010
domContentLoaded95217115
load68294613161241596
domInteractive95217115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 1.9 KiB (0.03%)
  • common: 1 Bytes (0.00%)

@matteoscurati matteoscurati requested a review from a team as a code owner June 5, 2024 10:35

const blockExplorerUrl =
defaultNetwork?.rpcPrefs?.blockExplorerUrl ??
defaultNetwork?.rpcUrl ??
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think ?? defaultNetwork?.rpcUrl should be here, as rpcUrl should never be a block explorer url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danjm you’re right. This is the type:

export type NetworkConfiguration = {
    rpcUrl: string;
    chainId: Hex;
    ticker: string;
    nickname?: string;
    rpcPrefs?: {
        blockExplorerUrl: string;
    };
};

I removed defaultNetwork?.rpcUrl

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

One more comment from me

danjm
danjm previously approved these changes Jun 5, 2024
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya left a comment

Choose a reason for hiding this comment

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

Ok LGTM. We will get another Notification dev to review. This was previously approved by Dan too.

@matteoscurati matteoscurati merged commit 7e6ab1d into develop Jun 7, 2024
76 checks passed
@matteoscurati matteoscurati deleted the feat/notifications-platform-links branch June 7, 2024 15:09
@github-actions github-actions bot locked and limited conversation to collaborators Jun 7, 2024
@metamaskbot metamaskbot added release-12.1.0 Issue or pull request that will be included in release 12.1.0 release-12.0.0 Issue or pull request that will be included in release 12.0.0 and removed release-12.1.0 Issue or pull request that will be included in release 12.1.0 labels Jun 7, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-12.0.0 on PR. Adding release label release-12.0.0 on PR and removing other release labels(release-12.1.0), as PR was cherry-picked in branch 12.0.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.0.0 Issue or pull request that will be included in release 12.0.0 team-notifications Notifications team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants