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: updated permissions header to be consistent #29880

Merged
merged 5 commits into from
Jan 24, 2025

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Jan 23, 2025

This PR is to update the title of the Dapp in all the connection steps

Related issues

Fixes: #29785

Manual testing steps

  1. Connect MM to a Dapp
  2. Check the correct Dapp Url is shown through all screens while connecting

Screenshots/Recordings

Before

404338198-60256b44-59b6-47ec-9c0f-aeeec0ef2b45.mov

After

Dapp Connection

Screen.Recording.2025-01-23.at.5.13.23.PM.mov

Snap Connection

Screen.Recording.2025-01-23.at.5.14.44.PM.mov

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.

@NidhiKJha NidhiKJha requested a review from a team as a code owner January 23, 2025 17:17
@metamaskbot metamaskbot added team-wallet-ux INVALID-PR-TEMPLATE PR's body doesn't match template labels Jan 23, 2025
vinnyhoward
vinnyhoward previously approved these changes Jan 23, 2025
@NidhiKJha NidhiKJha changed the title fix: updated permissions-connect-header to remove the title unknown origin… fix: updated permissions header to be consistent Jan 23, 2025
@NidhiKJha NidhiKJha requested a review from vinnyhoward January 23, 2025 17:49
vinnyhoward
vinnyhoward previously approved these changes Jan 23, 2025
@NidhiKJha NidhiKJha enabled auto-merge January 23, 2025 18:05
@hmalik88
Copy link
Contributor

hmalik88 commented Jan 23, 2025

This breaks currently intended functionality for snaps installation. In your recording, the header changes to the solflare wallet as the origin making the connection request, it should remain the snap.metamask.io origin until the installation screen.

@NidhiKJha
Copy link
Member Author

This breaks currently intended functionality for snaps installation. In your recording, the header changes to the solflare wallet as the origin making the connection request, it should remain the snap.metamask.io origin until the installation screen.

Aah! I see. the npm title shouldn't be there. In this case, should I revert the props for snaps?

@metamaskbot
Copy link
Collaborator

Builds ready [29c534c]
Page Load Metrics (1803 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16192262180317383
domContentLoaded15962070176414369
load16222149180316378
domInteractive25132563316
backgroundConnect9251445225
firstReactRender16111392914
getState45718189
initialActions01000
loadScripts11431553131112962
setupStore683272713
uiStartup183929142105250120
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 388 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@NidhiKJha
Copy link
Member Author

Aah! I see. the npm title shouldn't be there. In this case, should I revert the props for snaps?

I updated the prop value to use siteMetaData, I think it should resolve the issue.

Screen.Recording.2025-01-23.at.6.45.32.PM.mov

vinnyhoward
vinnyhoward previously approved these changes Jan 23, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [60d93e4]
Page Load Metrics (1601 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14391900161010852
domContentLoaded14131881158310751
load14221904160110852
domInteractive237435168
backgroundConnect85622147
firstReactRender15100372613
getState44412115
initialActions00000
loadScripts1007139611579746
setupStore67014199
uiStartup15752454183619694
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 620 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [66b1fcd]
Page Load Metrics (1774 ± 111 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint19522271691418201
domContentLoaded142721621751236113
load143821771774231111
domInteractive24195494220
backgroundConnect105520147
firstReactRender1592342411
getState459192010
initialActions00000
loadScripts10151637130520799
setupStore65714178
uiStartup165726482046276132
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 1.04 KiB (0.02%)
  • ui: 43 Bytes (0.00%)
  • common: -40.26 KiB (-0.44%)

@metamaskbot
Copy link
Collaborator

Builds ready [c3272cc]
Page Load Metrics (1827 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14932116182815775
domContentLoaded14872094179715273
load14912120182716177
domInteractive1997412110
backgroundConnect76334157
firstReactRender17101392512
getState569292311
initialActions01000
loadScripts10681548131413766
setupStore65115147
uiStartup173026882118220106
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 1.04 KiB (0.02%)
  • ui: -98 Bytes (-0.00%)
  • common: -40.26 KiB (-0.44%)

@NidhiKJha NidhiKJha added this pull request to the merge queue Jan 24, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [4f47a96]
Page Load Metrics (1934 ± 98 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31423411851409196
domContentLoaded160022841903208100
load16052339193420498
domInteractive28108502411
backgroundConnect765302010
firstReactRender16101422713
getState483182010
initialActions01000
loadScripts11671751141517081
setupStore676222311
uiStartup184730892278291140
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 1.04 KiB (0.02%)
  • ui: 86 Bytes (0.00%)
  • common: -40.26 KiB (-0.44%)

auto-merge was automatically disabled January 24, 2025 13:26

Pull Request is not mergeable

Merged via the queue into main with commit 2263ce4 Jan 24, 2025
69 checks passed
@NidhiKJha NidhiKJha deleted the permissions-connect-header branch January 24, 2025 13:35
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2025
@metamaskbot metamaskbot added the release-12.12.0 Issue or pull request that will be included in release 12.12.0 label Jan 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-12.12.0 Issue or pull request that will be included in release 12.12.0 team-wallet-ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Dapp url displayed as "Unkown origin" in the connection flow
8 participants