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: Fix send flow deeplinks #12965

Conversation

OGPoyraz
Copy link
Member

Description

This PR aims to fix deeplinks using Send flow.

Related issues

Fixes: #12689

Manual testing steps

  1. Open safari/chrome in the testing device - navigate https://metamask.github.io/metamask-deeplinks/
  2. Click "Payment Request" then "Eth Payment"
  3. Fill the form - you dont need to put chain id
  4. Click "Generate url"
  5. Click generated url
  6. See that it opens send flow page

Screenshots/Recordings

Simulator.Screen.Recording.-.iPhone.13.Pro.-.2025-01-13.at.22.01.50.mp4

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.

@OGPoyraz OGPoyraz requested a review from a team as a code owner January 13, 2025 21:02
@OGPoyraz OGPoyraz linked an issue Jan 13, 2025 that may be closed by this pull request
@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Jan 13, 2025
@sleepytanya
Copy link
Contributor

Deeplink opens correct send flow screen on iOS:

Bitrise build https://app.bitrise.io/build/2a9d2382-e406-41b7-82dc-22de573091fb?tab=artifacts

ScreenRecording_01-13-2025.18-12-22_1.MP4

Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

Nice!

The related test file only has a single snapshot test. Since we plan to update this confirmation to a redesign confirmation, I think it's fine not including a test now

@sleepytanya
Copy link
Contributor

Bitrise build (Android release) https://app.bitrise.io/build/5fad22ac-97be-4459-b9ea-21eb51b2580b?tab=artifacts

Fix doesn't work on Android, clicking on the link opens MM app but doesn't start the Send flow:

android.mp4

@Unik0rnMaggie
Copy link
Contributor

Bitrise build (Android release)

  • Fix doesn't work when clicking on the deeplink: nothing happens; the send flow does not start

  • Fix works when scanning the QR code

fix.not.working-.click.on.deeplink.mov
fix.working.-.scan.QR.mov

@OGPoyraz
Copy link
Member Author

@Unik0rnMaggie thanks for taking a look at this.
Because of the quality of the video I cannot confirm but can we make sure the link from the first fix.not.working-.click.on.deeplink.mov is actually equal with the link (on top of alert) in the fix.working.-.scan.QR.mov?

There should be no discrepancy between iOS and Android because fix is purely setting a forgotten argument in JS context.

Also this was the PR that we introduce that bug: #12321
(just putting here in case if anyone wants to compare before and after)

@Unik0rnMaggie
Copy link
Contributor

Hi @OGPoyraz sure, glad to help! And apologies for the poor resolution.

Here is another video: I'm using the same payment link for both actions:

  • clicking on the deeplink: not working
  • scanning QR code - working
Same.payment.link.mov

Please let me know if you need anything else!

Thank you!

@OGPoyraz
Copy link
Member Author

Thanks a lot @Unik0rnMaggie, it seems like both are the same links, weird.
I am currently trying to make a local Android build work, if I manage to test it on my end, will update here.

By the way just an info for you, if we are purely testing deeplinks, we should scan that QR code with your camera app, not the QR scanner in the MM app, because they are not the same flow.

@Unik0rnMaggie
Copy link
Contributor

Hi @OGPoyraz thank you very much! And duly noted! I will keep this in mind for future testing!

I tested again using the device camera to scan the QR code and it's the same behavior as when clicking on the deep link:

Nothing happens and the send flow is not starting

fix.not.working.mov

@OGPoyraz OGPoyraz added the No QA Needed Apply this label when your PR does not need any QA effort. label Jan 15, 2025
@OGPoyraz
Copy link
Member Author

Marking this PR as No QA needed to merge it as we discussed in the release thread.

@OGPoyraz OGPoyraz added the No E2E Smoke Needed If the PR does not need E2E smoke test run label Jan 15, 2025
@OGPoyraz OGPoyraz added this pull request to the merge queue Jan 15, 2025
Merged via the queue into main with commit 249e035 Jan 15, 2025
45 of 47 checks passed
@OGPoyraz OGPoyraz deleted the 12689-bug-send-transaction-screen-is-not-fully-loaded-via-deeplink branch January 15, 2025 20:00
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2025
@metamaskbot metamaskbot added the release-7.39.0 Issue or pull request that will be included in release 7.39.0 label Jan 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No E2E Smoke Needed If the PR does not need E2E smoke test run No QA Needed Apply this label when your PR does not need any QA effort. release-7.39.0 Issue or pull request that will be included in release 7.39.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Send transaction screen is not fully loaded via deeplink
6 participants