[SDK] feat: support object auth option with default value #7292
Conversation
|
|
@coreyar is attempting to deploy a commit to the thirdweb Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the """ WalkthroughThe changes introduce support for pre-filling authentication input fields (such as email or phone) in the in-app wallet UI by extending the types and logic handling authentication options. Utility functions are added to extract default values, and the UI components are updated to initialize input fields with these values if provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectEmbed
participant InAppWalletUI
participant PreOtpLogin
User->>ConnectEmbed: Opens connect modal
ConnectEmbed->>InAppWalletUI: Renders auth options (with possible default values)
InAppWalletUI->>PreOtpLogin: Passes defaultValue for email/phone if provided
PreOtpLogin->>User: Displays input pre-filled with defaultValue
User->>PreOtpLogin: Submits auth info
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx (1)
478-478: Use optional chaining for cleaner code.The normalization logic is correct, but you can simplify it using optional chaining as suggested by the static analysis tool.
Apply this diff to use optional chaining:
- const authProviderName = typeof authProvider === 'string' ? authProvider : authProvider && authProvider.type; + const authProviderName = typeof authProvider === 'string' ? authProvider : authProvider?.type;🧰 Tools
🪛 Biome (1.9.4)
[error] 478-478: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx(4 hunks)packages/thirdweb/src/react/native/ui/connect/InAppWalletUI.tsx(6 hunks)packages/thirdweb/src/wallets/in-app/core/wallet/types.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx
[error] 478-478: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
packages/thirdweb/src/wallets/in-app/core/wallet/types.ts (1)
42-44: LGTM! Well-designed type extension.The new union type
InAppWalletAuthelegantly supports both simple auth options and enhanced options with default values while maintaining backward compatibility.packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx (1)
495-528: LGTM! Consistent normalization pattern.The normalization of
authProvidertoauthProviderNameand its usage throughout the component correctly handles both string and object forms of auth providers.packages/thirdweb/src/react/native/ui/connect/InAppWalletUI.tsx (3)
96-103: LGTM! Well-implemented utility functions.The
hasAuthOptionandgetAuthOptionDefaultValuefunctions correctly handle both string and object forms of auth options, providing a clean abstraction for the new union type.
219-223: LGTM! Clean default value handling.The
PreOtpLogincomponent properly accepts and uses thedefaultValueprop to initialize the input state, enabling pre-filled authentication fields as intended.
112-146: LGTM! Consistent usage of utility functions.The conditional rendering logic correctly uses the new utility functions to check for auth options and retrieve default values, maintaining consistency with the new type structure.
packages/thirdweb/src/react/native/ui/connect/InAppWalletUI.tsx
Outdated
Show resolved
Hide resolved
coreyar
force-pushed
the
feat/issue-7126-add-default-value-to-auth-options
branch
from
ee56b4d to
6dd909f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx(4 hunks)packages/thirdweb/src/react/native/ui/connect/InAppWalletUI.tsx(6 hunks)packages/thirdweb/src/wallets/in-app/core/wallet/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/thirdweb/src/wallets/in-app/core/wallet/types.ts
- packages/thirdweb/src/react/native/ui/connect/InAppWalletUI.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx
[error] 478-478: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx (1)
495-495: LGTM! Consistent normalization implementation.The consistent replacement of
authProviderwithauthProviderNamethroughout the conditional rendering and display logic properly implements the intended normalization pattern. This ensures uniform handling of authentication provider identification regardless of whether the provider is specified as a string or object.Also applies to: 505-505, 520-521, 526-527
coreyar
force-pushed
the
feat/issue-7126-add-default-value-to-auth-options
branch
2 times, most recently
from
7f4d16b to
ca0b8d3
Compare
coreyar
force-pushed
the
feat/issue-7126-add-default-value-to-auth-options
branch
from
f7591c9 to
3abbf7c
Compare
TOOL-4579 Pre-filled values for
inAppWalletauth optionsNotes for the reviewer
I opened this PR in response to finding issue 7126. I looked through the tests real quick and didn't see any calling
inAppWallet(). I also didn't find tests for<ConnectEmbed/>after a quick pass.If this PR is in the right direction, I can dig a little deeper to add the tests and documentation.
PR-Codex overview
This PR focuses on enhancing the authentication options for the
InAppWalletby introducing a new type for authentication options that includes a default value. It also updates various components to utilize this new functionality for email and phone authentication.Detailed summary
AuthOptionWithOptionstype to includedefaultValue.InAppWalletAuthto support bothAuthOptionandAuthOptionWithOptions.InputSelectionUIto usedefaultValuefrom props.ConnectWalletSocialOptionsto handledefaultValuefor email and phone options.WalletLoadingViewto useauthProviderName.InAppWalletUIto utilizegetAuthOptionDefaultValuefor setting default values inPreOtpLogin.hasAuthOptioninstead of direct inclusion checks.Summary by CodeRabbit
New Features
Bug Fixes
Refactor