[MMI] Fix Connect MMI and Deep link Flows#19881
Conversation
|
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. |
…link-is-not-showing-up
|
@metamaskbot update-policies |
|
Policies updated |
…owing-up' of https://github.com/MetaMask/metamask-extension into feature/MMI-3407-investigate-why-our-deeplink-is-not-showing-up
brad-decker
left a comment
There was a problem hiding this comment.
I haven't done a test of functionality yet just code review and just a couple questions about changes in confirm-transaction-base
| await cancelTransaction(txData); | ||
| history.push(mostRecentOverviewPage); |
There was a problem hiding this comment.
why was this change necessary?
There was a problem hiding this comment.
IMHO, the modified code using await provides a clearer and more concise way of handling this asynchronous operation
There was a problem hiding this comment.
In general we should try to avoid changes unnecessary to the PR, as there may be unintended consequences that then get traced down to this. This is part of our "Auditability" engineering principle. I'm going to approve and just leave this is a critique to apply to future prs.
Builds ready [12bda86]
Page Load Metrics (1518 ± 40 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [2fe2167]
Page Load Metrics (1538 ± 33 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
brad-decker
left a comment
There was a problem hiding this comment.
There's some interesting logic changes to confirmations (approvals) that I think needs a second set of eyes on it from the confirmations team. See @DDDDDanica 's comment
| totalUnapprovedCount, | ||
| hasApprovalFlows: getApprovalFlows(state).length > 0, | ||
| hasApprovalFlows: | ||
| Array.isArray(getApprovalFlows) && getApprovalFlows(state).length > 0, |
There was a problem hiding this comment.
@jpuri I'd actually like to get your eyes on this or someone from confirmations team (if I should be tagging someone else let me know)
…link-is-not-showing-up
Builds ready [72b1915]
Page Load Metrics (1757 ± 55 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| </Box> | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Possible to test cover changes in this component ?
There was a problem hiding this comment.
Thanks for the feedback. I already did it with snapshots
jpuri
left a comment
There was a problem hiding this comment.
Changes look good, just a small feedback I added to it.
…link-is-not-showing-up
e592097
Builds ready [8aa1c72]
Page Load Metrics (1590 ± 40 ms)
|
| setCurrentJwt( | ||
| connectRequestValue.token || | ||
| (await dispatch(mmiActions.getCustodianToken())), | ||
| connectRequestValue.token || dispatch(mmiActions.getCustodianToken()), |
There was a problem hiding this comment.
A dispatch should not be required for getter function I think.
There was a problem hiding this comment.
Thanks, @jpuri. I'll address, if needed, these changes in a following PR.
Explanation
When doing a transaction or signing, the deep link doesn't show up, and no error is emitted in the console.
What we should be seeing:
We also ensured the extension is at the home view when the deep link is visible. (Current behavior)
Last, we improved the styles in the "Connect Custody" flow.
Screen.Recording.2023-07-07.at.13.03.10.mov
AC
The deep link appears after doing an operation.
A PR with the necessary changes has been created for the team to review
Ticket
https://consensyssoftware.atlassian.net/browse/MMI-3407
Pre-merge author checklist
Pre-merge reviewer checklist