feat: Add metrics for alerts (transactions redesign) #26121
Merged
Conversation
This file contains 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
|
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. |
vinistevam
force-pushed
the
feat/redesign-transaction-metrics-alerts
branch
from
ab98c8d to
051fd66
Compare
vinistevam
force-pushed
the
feat/redesign-transaction-metrics-alerts
branch
from
6e49030 to
c948f7f
Compare
Builds ready [c948f7f]
Page Load Metrics (146 ± 169 ms)
Bundle size diffs
|
Builds ready [2609816]
Page Load Metrics (65 ± 6 ms)
Bundle size diffs
|
vinistevam
changed the title
vinistevam
force-pushed
the
feat/redesign-transaction-metrics-alerts
branch
from
9643393 to
5eea34a
Compare
vinistevam
commented
Jul 26, 2024
| return ( | ||
| <ConfirmInfoRow | ||
| <ConfirmInfoAlertRow |
There was a problem hiding this comment.
Using ConfirmInfoAlertRow here to support the inline alert.
Builds ready [4e65436]
Page Load Metrics (152 ± 184 ms)
Bundle size diffs
|
jpuri
reviewed
Jul 26, 2024
| {children} | ||
| </AlertMetricsContext.Provider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
We use context for shared state, but there is no shared state. Can we avoid creating this new context.
There was a problem hiding this comment.
I apologize for any confusion. I have now updated the PR description to explain the rationale behind this approach. The context is necessary to aggregate and persist state across multiple events (e.g., user clicks on inline alerts, visualizations, and action clicks) within the alert system. This ensures that all interactions are captured and combined into a single set of properties, which are then added to the transaction event. Without the context, we would not be able to maintain this aggregated state across different components effectively.
There was a problem hiding this comment.
Since hook useConfirmationAlertMetrics is not used outside the context, may be code can be embedded in context itself.
jpuri
requested changes
Jul 26, 2024
There was a problem hiding this comment.
Plz check this feedback: https://github.com/MetaMask/metamask-extension/pull/26121/files#r1693152887
matthewwalsh0
requested changes
Jul 26, 2024
ui/pages/confirmations/components/confirm/info/shared/edit-gas-fees-row/edit-gas-fees-row.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Matthew Walsh <[email protected]>
legobeat
reviewed
Jul 29, 2024
| }); | ||
| }); | ||
|
|
||
| describe('useConfirmationAlertMetrics', () => { |
There was a problem hiding this comment.
Would it also be beneficial with a test (here or elsewhere) that no alert metrics are sent unless metametrics is enabled / user has opted in to analytics?
There was a problem hiding this comment.
This is encapsulated within the MetaMetricsController in the backend as it won't actually submit anything if the metrics preference is disabled.
Builds ready [32298a7]
Page Load Metrics (264 ± 279 ms)
Bundle size diffs
|
matthewwalsh0
requested changes
Jul 29, 2024
| }); | ||
| }); | ||
|
|
||
| describe('useConfirmationAlertMetrics', () => { |
There was a problem hiding this comment.
This is encapsulated within the MetaMetricsController in the backend as it won't actually submit anything if the metrics preference is disabled.
Builds ready [04900f9]
Page Load Metrics (262 ± 268 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26121 +/- ##
===========================================
+ Coverage 70.10% 70.14% +0.04%
===========================================
Files 1430 1432 +2
Lines 50147 50216 +69
Branches 13872 13881 +9
===========================================
+ Hits 35152 35221 +69
Misses 14995 14995 ☔ View full report in Codecov by Sentry. |
Builds ready [50fd47d]
Page Load Metrics (400 ± 389 ms)
Bundle size diffs
|
Builds ready [dcb1364]
Page Load Metrics (231 ± 245 ms)
Bundle size diffs
|
jpuri
approved these changes
Aug 2, 2024
matthewwalsh0
approved these changes
Aug 2, 2024
|
Builds ready [aa3aae0]
Page Load Metrics (156 ± 159 ms)
Bundle size diffs
|
metamaskbot
added
the
release-12.4.0
gauthierpetetin
added
release-12.3.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Recently the Alert System was introduced to redesigned confirmations with this the alerts for those confirmations were migrated to use the new Alert System. This PR introduces infrastructure to track metrics for alerts in the redesigned confirmation system. It ensures that alert metrics are captured and included in the necessary places for tracking.
The use of a context to manage shared state for alert metrics within the confirmation flow. The context approach was chosen because it allows us to maintain a single instance of the shared state per confirmation. This is crucial for ensuring that clicks on different inline alerts and actions are persisted and aggregated into a unified set of properties. These aggregated properties are then added to the transaction event, providing a comprehensive view of user interactions within the confirmation process.
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2709
Manual testing steps
PPOM - Malicious Transactions and Signatures> mint ERC20Screenshots/Recordings
confirm.webm
reject.webm
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist