[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. |
|
Warning Rate limit exceeded@coreyar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
""" 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
✨ Finishing Touches
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
from
6dd909f to
7f4d16b
Compare
coreyar
force-pushed
the
feat/issue-7126-add-default-value-to-auth-options
branch
from
7f4d16b to
ca0b8d3
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 enhances the
InAppWalletfunctionality by introducingAuthOptionWithOptions, improving authentication handling, and refining the UI components to utilize the new types for better management of default values and options.Detailed summary
AuthOptionWithOptionstype to manage options with default values.InAppWalletAuthto support bothAuthOptionandAuthOptionWithOptions.ConnectModal.tsxto useauthProviderNameinstead ofauthProvider.InAppWalletUI.tsxto check for authentication options usinghasAuthOption.PreOtpLoginto accept adefaultValueprop for better state initialization.Summary by CodeRabbit
New Features
Bug Fixes
Refactor