Merged
Conversation
Collaborator
Builds ready [7f49c26]
Page Load Metrics (454 ± 75 ms)
|
Member
IIRC you did this intentionally because the Sentry docs made it seem as though they'd be implicitly included in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: METAMASK-GH2B
Explanation: Frequent issues related to missing route tracks due to a route not matching. In the implementation of page tracking, I skipped the root '/confirmation' route. The root confirmation route has no UI representation and redirects the user to the more specific confirmation routes based on type. This has been prone to err so far.
My investigation looked at a handful of error instances, and I noticed that these weren't single confirmation flows. Each of them involves clicking something (like the next button on multiple confirmation screens or an unapproved tx in full screen). I was able to reproduce the error in fullscreen mode by clicking on an unapproved signature request. I could also reproduce by stacking multiple confirmation screens in the correct order (but I did not confirm what that order is) inconsistently.
This complexity of implementation and the resulting error is accidental; there was no requirement to not track this route. I tried to prevent noise in the signal, but these pages might hold a value that we do not yet know of. Most certainly, it isn't worth adding additional noise to our error reports.
I noticed that in fullscreen mode, the confirmation-route also gets triggered with the id of the original window that triggered the approval (
confirm-transaction/00000for example), so I added that path as well as the base to the PATH_NAME_MAP, which is used to compute thePATHS_TO_CHECKconstant.Additionally, I realized I made a mistake in implementing the extra data attributes to attach to this error. This has been resolved so that future instances of this error will hopefully signal actual issues with our routing and will be easier to debug.