Skip to content

feat: align sdk connection ui with latest permissions system #14605

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: align sdk connection ui with latest permissions system #14605

wants to merge 8 commits into from
+42 −160

Conversation

abretonc7s
Copy link
Contributor

@abretonc7s abretonc7s commented Apr 11, 2025 •
edited
Loading

Description

This PR updates the SDK connection flow to consistently use the latest permissions system UI, removing legacy components and streamlining the connection experience.

  • Removed the legacy AccountConnectSingle component in favor of using the newer PermissionsSummary component consistently
  • Eliminated the conditional rendering between old and new UI based on isSdkUrlUnknown
  • Simplified icon handling and URL state management
  • Improved callback handling with proper memoization using useCallback
  • Consolidated connection confirmation logic into a single handleConfirm function

Related issues

Manual testing steps

  • SDK connection requests from dapps
  • WalletConnect connection flows
  • Permission granting for both known and unknown origins
  • Network switching during connection
  • Multiple account selection flows

Screenshots/Recordings

Before

image

After

image

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.

@github-actions GitHub Actions
Copy link
Contributor

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.

@metamaskbot metamaskbot added the team-sdk SDK team label Apr 11, 2025
@abretonc7s abretonc7s changed the base branch from main to fix/sdkiosconnect April 11, 2025 09:40
@abretonc7s abretonc7s changed the title Sdkui feat: align sdk connection ui with latest permissions system Apr 11, 2025
Base automatically changed from fix/sdkiosconnect to main April 11, 2025 10:48
@abretonc7s abretonc7s added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 11, 2025
@abretonc7s
refactor: clean up imports and optimize AccountConnect component
f380121 
- Removed unused imports and consolidated necessary ones for better readability.
- Refactored the `hideSheet` and `handleConfirm` functions to use `useCallback` for performance optimization.
- Simplified the rendering logic in `renderConnectScreens` by removing unnecessary conditions.
- Enhanced overall code structure for improved maintainability.
@abretonc7s abretonc7s marked this pull request as ready for review April 11, 2025 10:54
@christopherferreira9 christopherferreira9 added the Run Smoke E2E Requires smoke E2E testing label Apr 11, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 11, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: f380121
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/92d21a03-5d45-4f37-a145-7094b2439ae5

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@abretonc7s abretonc7s mentioned this pull request Apr 14, 2025
Draft
7 tasks
@abretonc7s abretonc7s added the no-changelog Indicates no external facing user changes, therefore no changelog documentation needed label Apr 14, 2025
ecp4224
ecp4224 previously approved these changes Apr 14, 2025
View reviewed changes
@christopherferreira9 christopherferreira9 added QA Passed QA testing has been completed and passed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 14, 2025
abretonc7s and others added 3 commits April 15, 2025 23:37
@abretonc7s
Merge branch 'main' into sdkui
94f0fab
@abretonc7s
Merge branch 'main' into sdkui
4656d42
@abretonc7s
fix: handle invalid URLs in PermissionsSummary component
a60e8fc 
Updated the hostname extraction logic in the PermissionsSummary component to gracefully handle invalid URLs by returning the original string or an empty string. This prevents potential errors when processing URLs. Additionally, removed outdated tests related to SDK URL status from AccountConnect.test.tsx to streamline the test suite.
@abretonc7s abretonc7s added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels Apr 16, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 16, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a60e8fc
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/fdde1977-9adc-40a3-ad5d-f12dba919b1b

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@abretonc7s abretonc7s requested a review from ecp4224 April 16, 2025 03:49
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 67.49%. Comparing base (2e4d77b) to head (2d63b56).

Files with missing lines Patch % Lines
...components/Views/AccountConnect/AccountConnect.tsx 55.55% 4 Missing ⚠️
...nents/UI/PermissionsSummary/PermissionsSummary.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #14605      +/-   ##
==========================================
- Coverage   67.52%   67.49%   -0.04%     
==========================================
  Files        2307     2307              
  Lines       49589    49573      -16     
  Branches     7189     7183       -6     
==========================================
- Hits        33487    33459      -28     
- Misses      13983    14003      +20     
+ Partials     2119     2111       -8

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
53.3% Coverage on New Code (required ≥ 80%)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecp4224 ecp4224 ecp4224 approved these changes

Assignees
No one assigned
Labels
no-changelog Indicates no external facing user changes, therefore no changelog documentation needed QA Passed QA testing has been completed and passed Run Smoke E2E Requires smoke E2E testing team-sdk SDK team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abretonc7s @codecov-commenter @ecp4224 @metamaskbot @christopherferreira9