Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: adding warning for origin on redesigned pages #26306

Merged
merged 31 commits into from
Aug 21, 2024
Merged

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Aug 2, 2024

Description

Adding origin related validation in all redesigned confirmation pages.

Related issues

NA

Manual testing steps

NA

Screenshots/Recordings

NA

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@jpuri jpuri added the team-confirmations Push issues to confirmations team label Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

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.

@sunobun
Copy link

sunobun commented Aug 6, 2024

Into my wallet

@sunobun
Copy link

sunobun commented Aug 6, 2024

@jpuri jpuri marked this pull request as ready for review August 13, 2024 15:50
@jpuri jpuri requested review from a team as code owners August 13, 2024 15:50
@@ -37,8 +37,10 @@ const useAlerts = (ownerId: string) => {
if (!field) {
return [];
}

return alerts.filter((alert) => alert.field === field);
const fields = field.split(',');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following my other comment about alert keys, this logic shouldn't be needed as every field should have a single alert key, and multiple alerts can reference it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it.

app/_locales/en/messages.json Outdated Show resolved Hide resolved
],
},
];
}, [currentConfirmation, t]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than returning a fresh object every time the confirmation changes at all, could we instead use the most stable dependencies possible such as isInvalidAscii, field and t?

It also simplifies the code since we can move the core logic out of the useMemo.

ui/pages/confirmations/utils/confirm.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @matthewwalsh0 that we should update the alertKey values to represent the field references.

Renaming the existing props, as mentioned in a thread above, may be out of scope of this PR and could be followed up separately

@jpuri jpuri requested a review from matthewwalsh0 August 15, 2024 16:13
@metamaskbot
Copy link
Collaborator

Builds ready [2f8bafd]
Page Load Metrics (916 ± 645 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint743791378641
domContentLoaded9105332512
load4846359161343645
domInteractive9105332512
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.68 KiB (0.04%)
  • common: 680 Bytes (0.01%)

Copy link

codecov bot commented Aug 16, 2024

Codecov Report

Attention: Patch coverage is 82.14286% with 5 lines in your changes missing coverage. Please review.

Project coverage is 69.97%. Comparing base (81af2e8) to head (501302a).
Report is 12 commits behind head on develop.

Files Patch % Lines
ui/pages/confirmations/utils/confirm.ts 60.00% 4 Missing ⚠️
...tions/confirmation/templates/add-ethereum-chain.js 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26306      +/-   ##
===========================================
+ Coverage    69.96%   69.97%   +0.01%     
===========================================
  Files         1405     1406       +1     
  Lines        48996    49019      +23     
  Branches     13697    13702       +5     
===========================================
+ Hits         34280    34299      +19     
- Misses       14716    14720       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

digiwand
digiwand previously approved these changes Aug 16, 2024
matthewwalsh0
matthewwalsh0 previously approved these changes Aug 19, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [b76e671]
Page Load Metrics (264 ± 266 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint732911195928
domContentLoaded115326105
load502201264554266
domInteractive115326105
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.76 KiB (0.05%)
  • common: 695 Bytes (0.01%)

Copy link

@metamaskbot
Copy link
Collaborator

Builds ready [501302a]
Page Load Metrics (91 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint683991176833
domContentLoaded48299865426
load54299915325
domInteractive1290322110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.76 KiB (0.05%)
  • common: 695 Bytes (0.01%)

@jpuri jpuri merged commit 5f524f1 into develop Aug 21, 2024
78 checks passed
@jpuri jpuri deleted the add_origin_warning branch August 21, 2024 04:25
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 21, 2024
@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.4.0 Issue or pull request that will be included in release 12.4.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants