[SDK] Implement allowedSmsCountryCodes option #7090
Conversation
🦋 Changeset detectedLatest commit: 0f4fa25 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
joaquim-verges
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.tsx
Outdated
Show resolved
Hide resolved
|
""" WalkthroughThis update introduces a new configuration option, Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant WalletConfig
participant InputSelectionUI
participant CountrySelector
App->>WalletConfig: Get auth config (with allowedSmsCountryCodes)
App->>InputSelectionUI: Render with allowedSmsCountryCodes prop
InputSelectionUI->>CountrySelector: Pass allowedCountryCodes prop
CountrySelector->>CountrySelector: Filter country list by allowedCountryCodes
CountrySelector-->>InputSelectionUI: Render filtered selector
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.tsx (1)
32-34: Improved validation needed for allowedSmsCountryCodes first element.While the current implementation checks if
allowedSmsCountryCodesexists and is non-empty, it doesn't validate that the first element is a validSupportedSmsCountrybefore passing it togetCountrySelector.Although
getCountrySelectorhas a fallback to "US +1" if the country is not found, it would be better to add explicit validation:- : props.allowedSmsCountryCodes && props.allowedSmsCountryCodes.length > 0 - ? getCountrySelector(props.allowedSmsCountryCodes[0]) + : props.allowedSmsCountryCodes?.length > 0 && props.allowedSmsCountryCodes[0] + ? getCountrySelector(props.allowedSmsCountryCodes[0]) : "US +1",This suggestion is based on a previous review comment by graphite-app[bot] and would ensure robust handling of potentially invalid input.
🧹 Nitpick comments (1)
packages/thirdweb/src/react/web/wallets/in-app/CountrySelector.tsx (1)
32-43: Implementation looks good!The filtering logic correctly handles the
allowedCountryCodesprop to show only allowed countries when specified. The fallback to all countries (or US as a last resort) maintains backward compatibility.A minor readability suggestion:
Consider refactoring the nested ternary expression to a more explicit if-else structure for improved readability:
- const supportedCountriesForSms = - allowedCountryCodes && allowedCountryCodes.length > 0 - ? supportedSmsCountries.filter((c) => - allowedCountryCodes.includes(c.countryIsoCode as SupportedSmsCountry), - ) - : supportedSmsCountries ?? [ - { - countryIsoCode: "US", - countryName: "United States", - phoneNumberCode: 1, - }, - ]; + let supportedCountriesForSms; + if (allowedCountryCodes && allowedCountryCodes.length > 0) { + supportedCountriesForSms = supportedSmsCountries.filter((c) => + allowedCountryCodes.includes(c.countryIsoCode as SupportedSmsCountry) + ); + } else { + supportedCountriesForSms = supportedSmsCountries ?? [ + { + countryIsoCode: "US", + countryName: "United States", + phoneNumberCode: 1, + }, + ]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.changeset/limited-country-dropdown.md(1 hunks)packages/thirdweb/src/react/web/wallets/in-app/CountrySelector.tsx(1 hunks)packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.test.tsx(1 hunks)packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.tsx(2 hunks)packages/thirdweb/src/react/web/wallets/shared/ConnectWalletSocialOptions.tsx(1 hunks)packages/thirdweb/src/wallets/ecosystem/types.ts(1 hunks)packages/thirdweb/src/wallets/in-app/core/wallet/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.tsx (1)
packages/thirdweb/src/react/web/wallets/in-app/CountrySelector.tsx (1)
getCountrySelector(11-19)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
.changeset/limited-country-dropdown.md (1)
1-5: LGTM - Well-structured changeset!The changeset properly documents the new feature that allows limiting selectable countries for SMS login using the
allowedSmsCountryCodesoption, clearly indicating it works alongside the existingdefaultSmsCountryCodeoption.packages/thirdweb/src/react/web/wallets/shared/ConnectWalletSocialOptions.tsx (1)
454-456: LGTM - Properly implemented new feature!The implementation correctly passes the
allowedSmsCountryCodesconfiguration from the wallet to theInputSelectionUIcomponent, following the same pattern as the existingdefaultSmsCountryCodeprop.packages/thirdweb/src/wallets/ecosystem/types.ts (1)
22-26: LGTM - Well-documented type addition!The addition of the
allowedSmsCountryCodesproperty to theEcosystemWalletCreationOptionstype is well-placed and includes clear JSDoc documentation explaining its purpose. The typeSupportedSmsCountry[]is appropriate for this use case.packages/thirdweb/src/wallets/in-app/core/wallet/types.ts (1)
86-90: LGTM - Consistent type implementation!The addition of the
allowedSmsCountryCodesproperty to theInAppWalletCreationOptionstype maintains consistency with the ecosystem wallet implementation. The property is properly typed and documented with clear JSDoc comments.packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.test.tsx (1)
46-67: Well-structured test for the new feature!The test correctly verifies that the country selector filters options based on the
allowedSmsCountryCodesprop, checking both the inclusion of allowed countries (India, Brazil) and the exclusion of non-allowed countries (United States).packages/thirdweb/src/react/web/wallets/in-app/InputSelectionUI.tsx (1)
75-75: LGTM! Properly passing allowedSmsCountryCodes to CountrySelector.The component correctly passes the
allowedSmsCountryCodesprop to theCountrySelectorcomponent asallowedCountryCodes, ensuring the filtering behavior is applied throughout the component hierarchy.
packages/thirdweb/src/react/web/wallets/in-app/CountrySelector.tsx
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (57.14%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7090 +/- ##
==========================================
- Coverage 55.81% 55.63% -0.18%
==========================================
Files 900 902 +2
Lines 57848 58148 +300
Branches 4067 4094 +27
==========================================
+ Hits 32288 32353 +65
- Misses 25454 25690 +236
+ Partials 106 105 -1
🚀 New features to boost your workflow:
|
Fixed TOOL-4529
Summary
allowedSmsCountryCodesoption for in-app and ecosystem walletsTesting
pnpm test(fails: connect EHOSTUNREACH)PR-Codex overview
This PR introduces a new feature to limit selectable countries for SMS login by adding an
allowedSmsCountryCodesoption. This allows customization of the country codes displayed in the country selector.Detailed summary
allowedSmsCountryCodesoption to limit selectable countries for SMS login.ConnectWalletSocialOptions.tsxto utilizeallowedSmsCountryCodes.CountrySelector.tsxto filter supported countries based onallowedCountryCodes.InputSelectionUI.test.tsxto test filtering based onallowedSmsCountryCodes.InputSelectionUI.tsxto initialize country code based onallowedSmsCountryCodes.Summary by CodeRabbit
New Features
Tests