feat: add SIWE mismatch account warning alert and hide duplicate warnings in MultipleAlertModal #25559
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. |
digiwand
force-pushed
the
feat-sign-account-mistmatch-alerts
branch
from
32de38d
to
69cc879
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25559 +/- ##
===========================================
+ Coverage 69.65% 69.66% +0.01%
===========================================
Files 1355 1356 +1
Lines 48007 48029 +22
Branches 13235 13239 +4
===========================================
+ Hits 33437 33459 +22
Misses 14570 14570 ☔ View full report in Codecov by Sentry. |
Builds ready [69cc879]
Page Load Metrics (322 ± 280 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
digiwand
changed the title
Builds ready [23cd578]
Page Load Metrics (91 ± 20 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [23cd578]
Page Load Metrics (91 ± 20 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| }, [isMismatchAccount, t]); | ||
|
|
||
| return alerts; | ||
| } |
There was a problem hiding this comment.
Above hook will re-render only if confirmation changes, thus do we really need useMemo ?
There was a problem hiding this comment.
We currently do this as standard on all alert hooks since they are returning arrays so it ensures any usages and future usages have stable data, at the cost of only around 4 extra lines.
| const alerts = useMemo(() => { | ||
| return allAlerts.reduce((result: Alert[], alert: Alert) => { | ||
| const isDuplicateWarning = result.some((a: Alert) => { | ||
| return a.severity === Severity.Warning && a.message === alert.message; |
There was a problem hiding this comment.
Currently, the only use case is for warning alerts (mismatch account). While this addresses the present scenario, should we extend it to handle duplicates for all alert severities?
There was a problem hiding this comment.
I agree. I think since the danger severity would require confirmation, it would require more updates. Because of this I think it’d be better to extend the functionality when we need it
There was a problem hiding this comment.
Assuming we definitely want duplicate alerts on different fields, would it be cleaner to encapsulate this within useAlerts in a property such as uniqueAlerts?
| severity: Severity.Warning, | ||
| }, | ||
| { | ||
| field: 'signingInWith', |
There was a problem hiding this comment.
Why are we adding a single alert to two fields?
I understood our alert system assumes alerts are general or targeted to a single field?
Can we not pick the most relevant single field such as signing in with?
There was a problem hiding this comment.
oh I think you may be right. I was referring to the design in the screenshot but in Figma, it does indeed look like the alert only exists on "Signing in with". Let me double-check w/ design
| const alerts = useMemo(() => { | ||
| return allAlerts.reduce((result: Alert[], alert: Alert) => { | ||
| const isDuplicateWarning = result.some((a: Alert) => { | ||
| return a.severity === Severity.Warning && a.message === alert.message; |
There was a problem hiding this comment.
Assuming we definitely want duplicate alerts on different fields, would it be cleaner to encapsulate this within useAlerts in a property such as uniqueAlerts?
| @@ -58,9 +58,13 @@ const PersonalSignInfo: React.FC = () => { | |||
| <ConfirmInfoRowUrl url={currentConfirmation.msgParams.origin} /> | |||
| </ConfirmInfoAlertRow> | |||
| {isSIWE && ( | |||
| <ConfirmInfoRow label={t('signingInWith')}> | |||
| <ConfirmInfoAlertRow | |||
| alertKey="signingInWith" | |||
There was a problem hiding this comment.
To avoid duplicate strings, there is now a RowAlertKey enum inside ui/components/app/confirm/info/row/constants.ts.
| reason: t('alertReasonWrongAccount'), | ||
| severity: Severity.Warning, | ||
| }, | ||
| ] as Alert[]; |
There was a problem hiding this comment.
Why do we need to cast? Does this suggest we have bad data or a bad type?
| ] as Alert[]; | ||
| }, [isMismatchAccount, t]); | ||
|
|
||
| return alerts; |
There was a problem hiding this comment.
Minor, but can we return the hook directly to make it explicit we're caching the result?
| }, [isMismatchAccount, t]); | ||
|
|
||
| return alerts; | ||
| } |
There was a problem hiding this comment.
We currently do this as standard on all alert hooks since they are returning arrays so it ensures any usages and future usages have stable data, at the cost of only around 4 extra lines.
Description
Related issues
Fixes: #25317
Manual testing steps
isSIWEcondition inui/pages/confirmations/hooks/useCurrentConfirmation.tsfileyarn startScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist