Skip to content

chore: refactors metametrics to common function#13963

Merged
bergarces merged 2 commits into
mainfrom
fix/switch-network-metric-refactor
Mar 14, 2025
Merged

chore: refactors metametrics to common function#13963
bergarces merged 2 commits into
mainfrom
fix/switch-network-metric-refactor

Conversation

@bergarces

@bergarces bergarces commented Mar 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This PR aims to address some review comments from a previous PR that fixed a bug and that needed to be merged fast. No changes in behaviour should occur from this PR.

Specifically:

  • Typo in a variable name
  • Move switch network event tracking to the function that performs the action
  • Added a test file for the utils code (it does not exhaustively test the file, it just adds a test for the change)

#13883

Related issues

Fixes: See Description

Manual testing steps

  1. Open this dapp on the inapp browser
  2. If required, tap request accounts and connect while having Mainnet as the active chain
  3. Tap Add Polygon Chain
  4. A confirmation should show allowing the user to review the update
  5. The chain should have changed to Polygon

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.

@github-actions

github-actions Bot commented Mar 11, 2025

Copy link
Copy Markdown
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.

@metamaskbot metamaskbot added the mmi DEPRECATED: The product got sunset label Mar 11, 2025
@bergarces bergarces force-pushed the fix/switch-network-metric-refactor branch from 701049e to 40ebe22 Compare March 12, 2025 10:24
@bergarces bergarces force-pushed the fix/switch-network-metric-refactor branch from 40ebe22 to af3d497 Compare March 12, 2025 10:31
@bergarces bergarces added team-assets Run Smoke E2E and removed mmi DEPRECATED: The product got sunset labels Mar 12, 2025
@github-actions

github-actions Bot commented Mar 12, 2025 •
edited by metamaskbot
Loading

Copy link
Copy Markdown
Contributor

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: af3d497
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b716fd99-fea4-4500-a2eb-1a3b12fe95ff

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@bergarces bergarces marked this pull request as ready for review March 12, 2025 10:42
@bergarces bergarces requested a review from a team as a code owner March 12, 2025 10:42
@github-project-automation github-project-automation Bot moved this to Needs dev review in PR review queue Mar 13, 2025
Copilot AI reviewed Mar 14, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the MetaMetrics analytics tracking by moving event tracking into the common function switchToNetwork and fixes a variable naming typo. It also adds a new test for the updated util code.

  • Moves MetaMetrics tracking into switchToNetwork.
  • Removes duplicate tracking calls from wallet_switchEthereumChain and wallet_addEthereumChain.
  • Fixes a typo in a variable name and adds test coverage for the util code.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
app/core/RPCMethods/lib/ethereum-chain-utils.test.js Adds tests for the network switch event tracking in the utils function
app/core/RPCMethods/lib/ethereum-chain-utils.js Consolidates MetaMetrics event tracking inside switchToNetwork
app/core/RPCMethods/wallet_switchEthereumChain.js Removes duplicate tracking call after refactoring into the common function
app/core/RPCMethods/wallet_addEthereumChain.js Removes duplicate tracking call and fixes a typo in a variable name
app/core/RPCMethods/wallet_switchEthereumChain.test.js Updates tests by removing unnecessary MetaMetrics mocks
Comments suppressed due to low confidence (2)

app/core/RPCMethods/lib/ethereum-chain-utils.js:311

  • Consider adding error handling around the MetaMetrics tracking call to prevent potential runtime issues if the analytics service is unavailable.
MetaMetrics.getInstance().trackEvent(

app/core/RPCMethods/wallet_switchEthereumChain.test.js:1

  • [nitpick] Ensure that the tests are updated to verify the new analytics tracking behavior now handled by switchToNetwork, as the MetaMetrics mocks have been removed.
import MetaMetrics from '../Analytics/MetaMetrics';

NicolasMassart
NicolasMassart reviewed Mar 14, 2025
View reviewed changes

@NicolasMassart NicolasMassart left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few questions about typing and why not using TS for new files

Comment thread app/core/RPCMethods/lib/ethereum-chain-utils.test.js Outdated
Comment thread app/core/RPCMethods/lib/ethereum-chain-utils.test.js
Comment thread app/core/RPCMethods/lib/ethereum-chain-utils.test.js
@github-actions

github-actions Bot commented Mar 14, 2025 •
edited by metamaskbot
Loading

Copy link
Copy Markdown
Contributor

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: ced37ad
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/6df267d9-6b42-4899-9b27-a04034fe0a18

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@sonarqubecloud

sonarqubecloud Bot commented Mar 14, 2025

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

NicolasMassart
NicolasMassart approved these changes Mar 14, 2025
View reviewed changes

@NicolasMassart NicolasMassart left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

adonesky1
adonesky1 approved these changes Mar 14, 2025
View reviewed changes

@adonesky1 adonesky1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@github-project-automation github-project-automation Bot moved this from Needs dev review to Review finalised - Ready to be merged in PR review queue Mar 14, 2025
@bergarces bergarces added this pull request to the merge queue Mar 14, 2025
Merged via the queue into main with commit 1391451 Mar 14, 2025
@bergarces bergarces deleted the fix/switch-network-metric-refactor branch March 14, 2025 16:34
@github-project-automation github-project-automation Bot moved this from Review finalised - Ready to be merged to Merged, Closed or Archived in PR review queue Mar 14, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 14, 2025
@metamaskbot metamaskbot added the release-7.44.0 Issue or pull request that will be included in release 7.44.0 label Mar 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments

@NicolasMassart NicolasMassart NicolasMassart approved these changes

@adonesky1 adonesky1 adonesky1 approved these changes

Assignees

No one assigned

Labels

release-7.44.0 Issue or pull request that will be included in release 7.44.0 team-assets

Projects

PR review queue
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bergarces @NicolasMassart @adonesky1 @metamaskbot