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. |
Builds ready [679285a]
Page Load Metrics (1198 ± 242 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [5a17f39]
Page Load Metrics (841 ± 54 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
it's complaining otherwise
There was a problem hiding this comment.
Curious, I didn't get that locally 🤔
Builds ready [6bb81a2]
Page Load Metrics (850 ± 20 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
FrederikBolding
left a comment
There was a problem hiding this comment.
Code and performance LGTM. I'm not entirely sure about the confirmation pages changes, so we should get those looked at.
Let's have:
- Bowen do QA
- Erik do design review
- Someone from confirmations review and QA
| this.setState({ showSignatureInsights: false }); | ||
| }} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Logic around setting showSignatureInsights can be moved into InsightWarnings or another component wrapping InsightWarnings.
This will avoid adding this state handling to all signature request pages individually.
There was a problem hiding this comment.
I had decided to keep logic at the signature request page levels because some of the logic is dependent on other state such as showSignatureRequestWarning and didn't want to introduce more complexity.
| ///: BEGIN:ONLY_INCLUDE_IF(build-flask) | ||
| if (warnings?.length >= 1) { | ||
| return setIsShowingSigInsightWarnings(true); | ||
| } |
There was a problem hiding this comment.
After signing page goes away, just curious is above code needed ?
There was a problem hiding this comment.
This is needed because this would lead to the insight warning modal opening for flask builds
GuillaumeRx
left a comment
There was a problem hiding this comment.
LGTM
(NIT: I wonder if we could extract the insight warning logic in a hook to avoid code duplication. It seems like a lot of code is duplicated across all the different signature screens we have)
157ecfe
Builds ready [f04df1a]
Page Load Metrics (831 ± 31 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
hi there 👋 , QA from Confirmations side is in progress and will take a couple of days. I will update this same comment with the findings. 1. No Insights in the First Signature on Personal SignWhenever I trigger a Personal Signature for the first time, I don't see the signature insights popup: after clicking sign, the signature is done. no-warning-1st-signature.mp4Repro:
2. A Typed signature without
|
|
For 1 I believe this is simply a timing issue, we don't currently block the UI from proceeding while waiting for snaps to respond. We could if that is preferable, but currently don't. For 2 I don't think this is a bug in the implementation but just how the example snap works, without a domain the example snap does not have any insight to show: https://github.com/MetaMask/snaps/blob/main/packages/examples/packages/signature-insights/src/index.ts#L84-L109 |
|
Merging with acknowledgement that the first point raised by @seaona is a prod blocker for transaction insights and signature insights. The UI issues will be addressed in a forthcoming PR. The inconsistency in the snap export event is due to throttling that we will remove for certain exports in another PR: #22917 |
Per SIP-16, the signature insights implementation is being integrated.
Closes https://github.com/MetaMask/MetaMask-planning/issues/1778
Demo:
v2.mov