JustaName Integration#563
JustaName Integration#563Ghadi8 wants to merge 8 commits intopeanutprotocol:mainfrom JustaName-id:main
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Ghadi8 is attempting to deploy a commit to the squirrellabs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces several changes across various files, primarily focused on enhancing environment variable configurations, adding new components, and updating existing functionalities. Key modifications include the introduction of new environment variables related to the "JustaName" service, updates to dependency management in Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (13)
src/components/ClaimSubname/index.tsx (1)
200-200: Correct invalid Tailwind CSS classopacity-55The class
opacity-55is not a valid Tailwind CSS class. Tailwind opacity classes increment by 5, such asopacity-50,opacity-60, etc.Replace
opacity-55with the closest valid class, for example,opacity-50:className={`z-20 w-full border border-black bg-primary px-4 py-2 text-h6 text-black ${ isSubnameAvailableFetching || !isSubnameAvailable?.isAvailable - ? 'cursor-not-allowed opacity-55' + ? 'cursor-not-allowed opacity-50' : '' }`}src/utils/general.utils.ts (1)
292-297: MakechainIdconfigurable inresolveFromEnsNamefunctionThe
chainIdis hardcoded to1, which limits the function to Ethereum Mainnet. MakingchainIda parameter enhances the function's flexibility across different networks.Modify the function to accept
chainIdas an argument:-export async function resolveFromEnsName(ensName: string): Promise<string | undefined> { +export async function resolveFromEnsName(ensName: string, chainId: number): Promise<string | undefined> { const records = await JustaName.init().subnames.getRecords({ ens: ensName, - chainId:1, + chainId: chainId, providerUrl: 'https://mainnet.infura.io/v3/' + process.env["NEXT_PUBLIC_INFURA_API_KEY"] })You'll also need to update any calls to
resolveFromEnsNameto pass the appropriatechainId.postcss.config.js (1)
3-10: Ensure custom resolve function handles all import casesThe custom
resolvefunction forpostcss-importmay not handle all possible import paths, potentially causing issues with module resolution.Consider enhancing the
resolvefunction to handle other scoped packages or document its limitations. Alternatively, test thoroughly to ensure no import paths are broken due to this custom resolver..env.example (2)
16-18: Document the purpose and expected values of JustaName environment variablesAdd comments explaining:
- What these variables are used for
- Expected values and formats
- Whether they are required or optional
# JustaName Env Vars +# Required for JustaName integration: +# NEXT_PUBLIC_JUSTANAME_CHAIN_ID: Chain ID for JustaName service (e.g., "1" for mainnet, "11155111" for Sepolia) +# NEXT_PUBLIC_JUSTANAME_ENS_DOMAIN: The ENS domain to use for JustaName service (e.g., "example.eth") export NEXT_PUBLIC_JUSTANAME_CHAIN_ID="" export NEXT_PUBLIC_JUSTANAME_ENS_DOMAIN=""
29-29: Consolidate JustaName environment variablesThe JustaName environment variables are currently split into two sections. Consider consolidating them into a single section for better maintainability.
-export JUSTANAME_CHAIN_ID="1" - -# JustaName Env Vars -export JUSTANAME_ENS_DOMAIN="" -export JUSTANAME_API_KEY="" +# JustaName Env Vars +# JUSTANAME_CHAIN_ID: Default chain ID for JustaName service +# JUSTANAME_ENS_DOMAIN: ENS domain for the service +# JUSTANAME_API_KEY: API key for JustaName service authentication +export JUSTANAME_CHAIN_ID="" # Default: "1" for mainnet +export JUSTANAME_ENS_DOMAIN="" +export JUSTANAME_API_KEY=""Also applies to: 33-35
src/components/Global/AddressLink/index.tsx (1)
22-28: Add error handling for the primaryName hookThe component should handle potential errors from the
usePrimaryNamehook and provide appropriate fallback behavior.// Update display: prefer ENS name, fallback to shortened address - if (primaryName) { + if (primaryName && !error) { setDisplayAddress(primaryName) } else { + // Log error if present + if (error) { + console.error('Error fetching primary name:', error) + } setDisplayAddress(utils.printableAddress(address)) }src/config/justweb3.config.tsx (1)
35-37: Add error boundary for provider initializationThe provider should handle initialization errors gracefully.
Consider wrapping the provider with an error boundary:
+class JustWeb3ProviderErrorBoundary extends React.Component<{children: React.ReactNode}, {hasError: boolean}> { + constructor(props: {children: React.ReactNode}) { + super(props) + this.state = { hasError: false } + } + static getDerivedStateFromError() { + return { hasError: true } + } + render() { + if (this.state.hasError) { + return <div>Error initializing JustWeb3Provider. Please check your configuration.</div> + } + return this.props.children + } +} export const JustWeb3Provider: React.FC<JustaNameProviderConfig> = ({ children }) => { - return <JustWeb3ProviderPrimitive config={justweb3Config}>{children}</JustWeb3ProviderPrimitive> + return ( + <JustWeb3ProviderErrorBoundary> + <JustWeb3ProviderPrimitive config={justweb3Config}>{children}</JustWeb3ProviderPrimitive> + </JustWeb3ProviderErrorBoundary> + ) }src/app/api/subnames/add/route.ts (1)
5-8: Add request size limit and rate limitingThe API endpoint needs protection against abuse.
Consider implementing:
- Request size limits to prevent memory exhaustion
- Rate limiting to prevent abuse
- Input validation middleware
Example implementation using middleware:
import rateLimit from 'express-rate-limit' import { NextResponse } from 'next/server' const MAX_REQUEST_SIZE = '10kb' const RATE_LIMIT = 100 // requests const RATE_LIMIT_WINDOW = 15 * 60 * 1000 // 15 minutes export const config = { api: { bodyParser: { sizeLimit: MAX_REQUEST_SIZE } } } const limiter = rateLimit({ windowMs: RATE_LIMIT_WINDOW, max: RATE_LIMIT })src/components/Create/Link/Initial.view.tsx (1)
Line range hint
119-134: Add error handling for ENS resolution timeoutThe ENS name resolution lacks timeout handling and could potentially hang. Consider adding a timeout and proper error handling.
if (ensRegex.test(inputValue)) { - const _address = await utils.resolveFromEnsName(inputValue) + try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + const _address = await utils.resolveFromEnsName(inputValue, { signal: controller.signal }) + clearTimeout(timeoutId); if (_address) setRecipient({ name: inputValue, address: _address }) else { setErrorState({ showError: true, errorMessage: 'Please enter a valid address or ENS name.', }) setLoadingState('Idle') return } + } catch (error) { + setErrorState({ + showError: true, + errorMessage: 'ENS resolution failed. Please try again.', + }) + setLoadingState('Idle') + return + }src/components/Global/Header/index.tsx (2)
282-286: Optimize hooks usage with proper memoizationThe
useAccountSubnameshook result should be memoized to prevent unnecessary rerenders. Also, consider handling the loading state more gracefully.-const { accountSubnames, isAccountSubnamesPending } = useAccountSubnames() +const { accountSubnames = [], isAccountSubnamesPending } = useAccountSubnames() const router = useRouter() -const peanutSubname = useMemo(() => { - return accountSubnames.find((subname) => subname.ens.endsWith(peanutEnsDomain)) -}, [accountSubnames]) +const peanutSubname = useMemo(() => + accountSubnames.find((subname) => subname.ens.endsWith(peanutEnsDomain)), + [accountSubnames, peanutEnsDomain] +)
318-320: Improve loading state feedbackThe current loading state display is too simple. Consider adding a proper loading spinner or skeleton UI.
-{ - isAccountSubnamesPending ? 'Loading...' : peanutSubname?.ens ?? 'Claim ENS' -} +{isAccountSubnamesPending ? ( + <div className="flex items-center gap-2"> + <span className="loading loading-spinner loading-sm" /> + <span>Loading ENS...</span> + </div> +) : ( + peanutSubname?.ens ?? 'Claim ENS' +)}src/components/Profile/Components/TableComponent.tsx (2)
92-94: Remove commented out codeThe implementation looks good, but there's unnecessary commented code that should be removed if it's no longer needed.
- {/* <th className="th-custom"></th> */}
236-242: Simplify conditional renderingThe nested ternary operator makes the code harder to read. Consider simplifying it using logical AND operator.
- { - data.address ? - <AddressLink address={data.address} /> - : null - } + {data.address && <AddressLink address={data.address} />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.env.example(2 hunks).gitignore(1 hunks)package.json(3 hunks)postcss.config.js(1 hunks)src/app/api/subnames/add/route.ts(1 hunks)src/app/claim-subname/page.tsx(1 hunks)src/components/ClaimSubname/index.tsx(1 hunks)src/components/Create/Link/Initial.view.tsx(3 hunks)src/components/Global/AddressLink/index.tsx(2 hunks)src/components/Global/Header/index.tsx(3 hunks)src/components/Profile/Components/TableComponent.tsx(4 hunks)src/components/index.ts(1 hunks)src/config/justaname.config.ts(1 hunks)src/config/justweb3.config.tsx(1 hunks)src/config/peanut.config.tsx(2 hunks)src/utils/general.utils.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (8)
src/components/ClaimSubname/index.tsx (1)
189-189: Verify peanutEnsDomain is defined and imported correctly
Ensure that peanutEnsDomain is correctly defined and imported to prevent undefined values in the rendered output.
Run the following script to confirm that peanutEnsDomain is properly exported:
✅ Verification successful
Let me verify if this variable is properly imported in the component. I'll generate a script to check this.
peanutEnsDomain is properly defined and imported
The variable is correctly defined in src/config/justweb3.config.tsx and properly imported and used in the ClaimSubname component. It's used consistently in the component for the ENS domain display and configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'peanutEnsDomain' is defined and exported.
# Test: Search for the export of 'peanutEnsDomain'. Expect: At least one match.
rg "export\s+(const|let|var)\s+peanutEnsDomain" src/
Length of output: 167
Script:
#!/bin/bash
# Check if peanutEnsDomain is imported in ClaimSubname component
rg "import.*peanutEnsDomain" src/components/ClaimSubname/index.tsx
# Also check for any usage of peanutEnsDomain in the file
rg "peanutEnsDomain" src/components/ClaimSubname/index.tsx
Length of output: 452
src/utils/general.utils.ts (1)
299-299: Confirm correct coin ID in records lookup
When accessing coins with coin.id === 60, ensure that this ID corresponds to the expected cryptocurrency (e.g., Ethereum).
Run the following script to list available coin IDs and verify that ID 60 is correct:
Replace $ENS_NAME with a valid ENS name to test.
src/components/index.ts (1)
12-12: Export of ClaimSubname component is correct
The new export statement correctly adds ClaimSubname to the components index.
src/app/claim-subname/page.tsx (1)
1-30: New page component is properly implemented
The ProfilePage component correctly imports and renders the ClaimSubname component within the Layout.
src/config/peanut.config.tsx (1)
31-34: Verify provider order and initialization
The provider order looks correct, but we should verify that the JustWeb3Provider doesn't interfere with existing functionality.
Let's check for any potential provider conflicts:
✅ Verification successful
Let's examine the provider implementations to understand their relationships and potential conflicts.
Let's check if there are any dependencies between these providers and if JustWeb3Provider is used correctly in the context hierarchy.
Let's check the app layout and entry points to understand the complete provider hierarchy.
Let's check the config index file and search for the root component where these providers are initialized.
Based on the analysis of the providers and their implementations, I can now provide the final response:
Provider order is correctly implemented
The JustWeb3Provider integration is safe because:
- It's properly imported and doesn't conflict with existing providers
- It wraps ChakraProvider, which is the correct order as it's a UI-level provider
- The implementation is isolated and doesn't interfere with other context providers
- The Analytics component placement after ChakraProvider is maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other providers that might conflict with JustWeb3Provider
rg -l "Provider.*children.*Provider" src/
Length of output: 163
Script:
#!/bin/bash
# Check the implementation of providers and their dependencies
echo "=== contextProvider.tsx ==="
rg -A 5 "export.*Provider" src/context/contextProvider.tsx
echo -e "\n=== wagmi.config.tsx ==="
rg -A 5 "export.*Provider" src/config/wagmi.config.tsx
echo -e "\n=== justweb3.config.tsx ==="
rg -A 5 "export.*Provider" src/config/justweb3.config.tsx
# Check for any imports of these providers
echo -e "\n=== Provider imports in peanut.config.tsx ==="
rg "import.*Provider" src/config/peanut.config.tsx
Length of output: 1438
Script:
#!/bin/bash
# Check for JustWeb3Provider dependencies and usage
echo "=== JustWeb3Provider dependencies ==="
rg -B 3 -A 3 "JustWeb3ProviderPrimitive" src/
# Check for imports and usage of other providers in the app
echo -e "\n=== App entry point providers ==="
rg -B 3 -A 3 "Provider" src/app.tsx src/main.tsx src/index.tsx 2>/dev/null
# Check if ContextProvider or WagmiProvider are used alongside JustWeb3Provider
echo -e "\n=== Provider usage patterns ==="
rg -l "ContextProvider|WagmiProvider" src/
Length of output: 1374
Script:
#!/bin/bash
# Check app layout implementation
echo "=== App layout implementation ==="
rg -B 5 -A 10 "export.*layout" src/app/layout.tsx
# Check where peanut.config is being used
echo -e "\n=== Peanut config usage ==="
rg -l "peanut\.config" src/
# Check the actual provider order in layout files
echo -e "\n=== Provider order in layout files ==="
rg -B 3 -A 3 "Provider.*children" src/app/layout.tsx src/layout.tsx 2>/dev/null
Length of output: 412
Script:
#!/bin/bash
# Check config index implementation
echo "=== Config index implementation ==="
cat src/config/index.ts
# Find the root component or entry point
echo -e "\n=== Root component search ==="
rg -l "ReactDOM.render|createRoot" src/
# Check main entry point implementation
echo -e "\n=== Main entry point implementation ==="
rg -B 5 -A 10 "render|createRoot" src/ --type ts --type tsx
Length of output: 471
package.json (1)
73-74: LGTM: Testing setup looks good
The addition of Jest and related testing libraries with their latest stable versions is a good practice for ensuring code quality.
Also applies to: 83-84
src/components/Profile/Components/TableComponent.tsx (2)
Line range hint 1-272: Consider performance optimization for ENS resolution
Since the table might display multiple ENS names simultaneously, consider implementing batch resolution or caching mechanisms to optimize performance. The AddressLink component should ideally handle this internally.
75-77: Verify ENS column sorting implementation
The ENS column header is correctly added, but we should ensure that the sorting logic properly handles ENS names, which may require special comparison rules (e.g., case-insensitive, handling of special characters).
| } | ||
|
|
||
| return ( | ||
| <div className="relative flex h-full w-full flex-row flex-col items-center justify-start gap-4 px-4"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove conflicting Tailwind CSS classes
The div at line 67 has both flex-row and flex-col classes applied, which are contradictory. This may lead to unexpected layout behavior.
Remove the unnecessary flex-row class:
-<div className="relative flex h-full w-full flex-row flex-col items-center justify-start gap-4 px-4">
+<div className="relative flex h-full w-full flex-col items-center justify-start gap-4 px-4">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="relative flex h-full w-full flex-row flex-col items-center justify-start gap-4 px-4"> | |
| <div className="relative flex h-full w-full flex-col items-center justify-start gap-4 px-4"> |
| const handleClaim = async (data: ISubnameForm) => { | ||
| addSubname({ | ||
| username: data.username, | ||
| ensDomain: peanutEnsDomain, | ||
| chainId: chainId | ||
| }).then(() => { | ||
| router.push('/profile') | ||
| }) | ||
| } |
There was a problem hiding this comment.
Handle errors in handleClaim function
The handleClaim function currently does not handle errors from the addSubname call. This might lead to unhandled promise rejections and a poor user experience if the claim fails.
Consider refactoring to use async/await with a try...catch block to handle errors gracefully:
const handleClaim = async (data: ISubnameForm) => {
- addSubname({
+ try {
+ await addSubname({
username: data.username,
ensDomain: peanutEnsDomain,
chainId: chainId
- }).then(() => {
+ })
+ router.push('/profile')
+ } catch (error) {
+ // Handle error (e.g., display an error message)
+ console.error('Failed to claim subname:', error)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| const { primaryName } = usePrimaryName({ | ||
| address: isAddress(address) ? (address as `0x${string}`) : undefined, | ||
| chainId: 1, // Mainnet for ENS lookups | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use environment variable for chain ID instead of hardcoding
The chain ID is hardcoded to 1 (mainnet) in the usePrimaryName hook. Consider using the NEXT_PUBLIC_JUSTANAME_CHAIN_ID environment variable for flexibility across different environments.
const { primaryName } = usePrimaryName({
address: isAddress(address) ? (address as `0x${string}`) : undefined,
- chainId: 1, // Mainnet for ENS lookups
+ chainId: Number(process.env.NEXT_PUBLIC_JUSTANAME_CHAIN_ID) || 1, // Fallback to mainnet
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { primaryName } = usePrimaryName({ | |
| address: isAddress(address) ? (address as `0x${string}`) : undefined, | |
| chainId: 1, // Mainnet for ENS lookups | |
| }) | |
| const { primaryName } = usePrimaryName({ | |
| address: isAddress(address) ? (address as `0x${string}`) : undefined, | |
| chainId: Number(process.env.NEXT_PUBLIC_JUSTANAME_CHAIN_ID) || 1, // Fallback to mainnet | |
| }) |
| const ensDomain = process.env.JUSTANAME_ENS_DOMAIN as string | ||
| const infuraApiKey = process.env.INFURA_API_KEY as string |
There was a problem hiding this comment.
Add validation for required environment variables
The configuration assumes environment variables are present without validation. Add checks to ensure required variables are available before initialization.
+ if (!ensDomain) {
+ throw new Error('JUSTANAME_ENS_DOMAIN environment variable is not set')
+ }
+ if (!infuraApiKey) {
+ throw new Error('INFURA_API_KEY environment variable is not set')
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ensDomain = process.env.JUSTANAME_ENS_DOMAIN as string | |
| const infuraApiKey = process.env.INFURA_API_KEY as string | |
| const ensDomain = process.env.JUSTANAME_ENS_DOMAIN as string | |
| const infuraApiKey = process.env.INFURA_API_KEY as string | |
| if (!ensDomain) { | |
| throw new Error('JUSTANAME_ENS_DOMAIN environment variable is not set') | |
| } | |
| if (!infuraApiKey) { | |
| throw new Error('INFURA_API_KEY environment variable is not set') | |
| } |
| const chainId = parseInt( | ||
| message.split('Chain ID: ')[1].split('\n')[0], | ||
| ) as ChainId |
There was a problem hiding this comment.
Improve message parsing security
The current message parsing is fragile and could break with malformed input.
Add robust parsing with proper error handling:
- const chainId = parseInt(
- message.split('Chain ID: ')[1].split('\n')[0],
- ) as ChainId
+ const chainId = (() => {
+ try {
+ const match = message.match(/Chain ID: (\d+)/i)
+ if (!match) throw new Error('Chain ID not found in message')
+ const id = parseInt(match[1])
+ if (isNaN(id)) throw new Error('Invalid Chain ID format')
+ return id as ChainId
+ } catch (error) {
+ throw new Error(`Invalid message format: ${error.message}`)
+ }
+ })()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chainId = parseInt( | |
| message.split('Chain ID: ')[1].split('\n')[0], | |
| ) as ChainId | |
| const chainId = (() => { | |
| try { | |
| const match = message.match(/Chain ID: (\d+)/i) | |
| if (!match) throw new Error('Chain ID not found in message') | |
| const id = parseInt(match[1]) | |
| if (isNaN(id)) throw new Error('Invalid Chain ID format') | |
| return id as ChainId | |
| } catch (error) { | |
| throw new Error(`Invalid message format: ${error.message}`) | |
| } | |
| })() |
| <div className={"bg-black h-full flex flex-col justify-center p-2 rounded-full"} | ||
| onClick={(event) => { | ||
| web3modalOpen() | ||
| event.stopPropagation() | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event handling in nested button
The current event handling could lead to confusing UX where both parent and child buttons respond to clicks. Consider restructuring the component.
-<div className={"bg-black h-full flex flex-col justify-center p-2 rounded-full"}
+<button
+ type="button"
+ className={"bg-black h-full flex flex-col justify-center p-2 rounded-full"}
onClick={(event) => {
+ event.preventDefault()
web3modalOpen()
event.stopPropagation()
}}
->📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={"bg-black h-full flex flex-col justify-center p-2 rounded-full"} | |
| onClick={(event) => { | |
| web3modalOpen() | |
| event.stopPropagation() | |
| }} | |
| <button | |
| type="button" | |
| className={"bg-black h-full flex flex-col justify-center p-2 rounded-full"} | |
| onClick={(event) => { | |
| event.preventDefault() | |
| web3modalOpen() | |
| event.stopPropagation() | |
| }} |
| <td className="td-custom font-bold"> | ||
| <AddressLink address={data.tertiaryText} /></td> |
There was a problem hiding this comment.
Add validation and improve formatting
There are several issues with this implementation:
- Missing null check for
tertiaryTextbefore using it as an address - No validation that
tertiaryTextis actually an address - Inconsistent formatting with the closing tag on the same line
- <td className="td-custom font-bold">
- <AddressLink address={data.tertiaryText} /></td>
+ <td className="td-custom font-bold">
+ {utils.isAddress(data.tertiaryText) && (
+ <AddressLink address={data.tertiaryText} />
+ )}
+ </td>Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/Global/GeneralRecipientInput/index.tsx (1)
Line range hint
118-126: Apply learned bank account autocomplete attributeBased on previous learnings, the autocomplete attribute should be conditionally set for bank accounts.
Apply this change:
<ValidatedInput label="To" value={recipient.name ?? recipient.address} placeholder={placeholder} validate={checkAddress} className={className} autoComplete="on" - name="bank-account" + name={recipientType.current === 'us' || recipientType.current === 'iban' ? 'bank-account-number' : undefined} suggestions={getSuggestions(recipientType.current)} infoText={infoText} formatDisplayValue={formatDisplayValue} />
🧹 Nitpick comments (5)
src/components/Global/GeneralRecipientInput/index.tsx (1)
Line range hint
55-64: Enhance error handling for ENS resolutionThe ENS validation logic is sound but could benefit from more specific error messages.
Consider enhancing the error messages:
} else if (validateEnsName(trimmedInput.toLowerCase())) { type = 'ens' const address = await utils.resolveFromEnsName(trimmedInput.toLowerCase()) if (address) { resolvedAddress.current = address isValid = true } else { - errorMessage.current = 'ENS name not found' + errorMessage.current = 'ENS name could not be resolved. Please verify the name or try again later.' isValid = false }src/components/Request/Pay/Pay.tsx (1)
Line range hint
45-52: Improve error handling in ENS name resolutionWhile the ENS validation logic has been improved, the error handling could be more informative.
Consider enhancing the error message to provide more context:
- throw new Error('Failed to resolve ENS name') + throw new Error(`Failed to resolve ENS name: ${address}. Please verify the name is correct.`)src/components/Create/Link/Input.view.tsx (1)
278-279: Consider extracting recipient display logicThe recipient name display logic is duplicated across components. Consider extracting it into a reusable utility function.
Create a new utility function:
// utils/display.utils.ts export const getDisplayName = (recipient: { name?: string, address?: string }): string => { if (recipient?.name && validateEnsName(recipient.name)) { return recipient.name; } return shortenAddressLong(recipient.address ?? ''); };Then use it in the component:
- ? `Send to ${recipient?.name && validateEnsName(recipient?.name) ? recipient.name : shortenAddressLong(recipient.address ?? '')}` + ? `Send to ${getDisplayName(recipient)}`src/components/Create/Link/Confirm.view.tsx (2)
237-238: Consolidate recipient display logicThe recipient name display logic is duplicated from Input.view.tsx. This should use the same utility function suggested earlier.
Apply the same pattern:
- ? `Send to ${recipient?.name && validateEnsName(recipient?.name) ? recipient.name : printableAddress(recipient.address ?? '')}` + ? `Send to ${getDisplayName(recipient)}`
Line range hint
353-367: Address TODO comment about points estimationThere's an unaddressed TODO comment regarding points estimation that should be resolved.
Would you like me to help implement the correct points estimation logic or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.json(3 hunks)src/components/Create/Link/Confirm.view.tsx(2 hunks)src/components/Create/Link/Initial.view.tsx(3 hunks)src/components/Create/Link/Input.view.tsx(2 hunks)src/components/Create/Link/Success.view.tsx(2 hunks)src/components/Global/AddressInput/index.tsx(2 hunks)src/components/Global/GeneralRecipientInput/index.tsx(2 hunks)src/components/Request/Pay/Pay.tsx(2 hunks)src/utils/general.utils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Create/Link/Initial.view.tsx
- package.json
🧰 Additional context used
📓 Learnings (1)
src/components/Global/GeneralRecipientInput/index.tsx (1)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#545
File: src/components/Global/GeneralRecipientInput/index.tsx:118-126
Timestamp: 2024-11-26T12:06:11.603Z
Learning: For bank account input fields, use `autocomplete="bank-account-number"` when the recipient type is `'us'` or `'iban'`.
🔇 Additional comments (7)
src/components/Global/AddressInput/index.tsx (2)
6-6: LGTM: Clean import of new utility function
The import of validateEnsName from utils is properly placed with other utility imports.
Line range hint 18-23: Verify ENS resolution behavior
The ENS validation logic looks correct, but we should ensure proper error handling for network issues during ENS resolution.
Let's check for other ENS resolution implementations:
Consider adding a specific error message for network-related ENS resolution failures:
if (validateEnsName(recipient.toLowerCase())) {
const resolvedAddress = await resolveFromEnsName(recipient.toLowerCase())
return !!resolvedAddress
+ } catch (error) {
+ console.error('ENS resolution failed:', error)
+ return false
} else {
return isAddress(recipient, { strict: false })
}src/components/Global/GeneralRecipientInput/index.tsx (1)
10-10: LGTM: Clean import of validation utility
The import is properly placed with other utility imports.
src/components/Create/Link/Success.view.tsx (1)
11-11: LGTM: Clean import of validation utility
The import is properly placed with other utility imports.
src/utils/general.utils.ts (3)
21-21: LGTM! Improved ENS name validation
The change from a simple .eth suffix check to using a proper regex validation through validateEnsName is a good improvement that makes the validation more robust.
894-894: LGTM! Clean import statement
The import of the JustaName SDK is clean and properly placed at the end of the imports section.
296-302:
Add error handling and environment variable validation
The resolveFromEnsName function has several potential issues:
- No validation of the
NEXT_PUBLIC_INFURA_API_KEYenvironment variable - No error handling for API failures
- No type checking for the returned records
Consider applying this improvement:
export async function resolveFromEnsName(ensName: string): Promise<string | undefined> {
+ const infuraApiKey = process.env['NEXT_PUBLIC_INFURA_API_KEY'];
+ if (!infuraApiKey) {
+ throw new Error('NEXT_PUBLIC_INFURA_API_KEY is not defined');
+ }
+ try {
const records = await JustaName.init().subnames.getRecords({
ens: ensName,
chainId: 1,
- providerUrl: 'https://mainnet.infura.io/v3/' + process.env['NEXT_PUBLIC_INFURA_API_KEY'],
+ providerUrl: `https://mainnet.infura.io/v3/${infuraApiKey}`,
})
+ if (!records?.records?.coins) {
+ return undefined;
+ }
return records?.records?.coins?.find((coin) => coin.id === 60)?.value;
+ } catch (error) {
+ console.error('Error resolving ENS name:', error);
+ return undefined;
+ }
}Likely invalid or redundant comment.
| ? `You have successfully sent the funds to ${recipient?.name && validateEnsName(recipient?.name) ? recipient.name : printableAddress(recipient.address ?? '')}.` | ||
| : 'Share this link or QR code with the recipient. They will be able to claim the funds on any chain in any token.'} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve null safety in recipient name validation
The recipient name validation logic could be more robust with null safety.
Consider this safer implementation:
- ? `You have successfully sent the funds to ${recipient?.name && validateEnsName(recipient?.name) ? recipient.name : printableAddress(recipient.address ?? '')}.`
+ ? `You have successfully sent the funds to ${recipient?.name && recipient.name.trim() && validateEnsName(recipient.name) ? recipient.name : printableAddress(recipient?.address ?? '')}.`This change:
- Ensures name is not empty after trimming
- Maintains consistent null safety for both name and address
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ? `You have successfully sent the funds to ${recipient?.name && validateEnsName(recipient?.name) ? recipient.name : printableAddress(recipient.address ?? '')}.` | |
| : 'Share this link or QR code with the recipient. They will be able to claim the funds on any chain in any token.'} | |
| ? `You have successfully sent the funds to ${recipient?.name && recipient.name.trim() && validateEnsName(recipient.name) ? recipient.name : printableAddress(recipient?.address ?? '')}.` | |
| : 'Share this link or QR code with the recipient. They will be able to claim the funds on any chain in any token.'} |
| export const validateEnsName = (ensName: string): boolean => { | ||
| return /(?:^|[^a-zA-Z0-9-_.])(([^\s.]{1,63}\.)+[^\s.]{2,63})/.test(ensName) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code analysis, I can now provide a final response:
The ENS name validation needs both tests and improvements
The current regex pattern has potential issues:
- It uses a positive lookbehind
(?:^|[^a-zA-Z0-9-_.])which may not work consistently across all JavaScript engines - The pattern may not correctly validate all ENS name requirements (e.g., valid characters, length limits)
- The function is widely used across the codebase for critical functionality like address resolution and recipient validation
Suggested improvements:
- Add comprehensive test cases covering:
- Valid ENS names (e.g., "vitalik.eth", "sub.domain.eth")
- Invalid ENS names (e.g., empty string, no TLD, invalid characters)
- Edge cases (e.g., maximum length names, names with hyphens)
- Consider using established ENS validation libraries or implementing the official ENS naming standards
- Document the validation rules being enforced
🔗 Analysis chain
Consider adding test cases for the ENS name validation regex
The regex pattern for ENS name validation looks comprehensive but should be thoroughly tested with various edge cases to ensure it correctly validates all valid ENS names and rejects invalid ones.
Let's verify the regex pattern against common ENS name patterns:
Let me search for any existing tests for this validation function.
Let me check how validateEnsName is being used in these files to understand the validation requirements better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test the ENS name validation regex against various cases
echo "Testing ENS name validation regex..."
echo "vitalik.eth" | gawk '/(?:^|[^a-zA-Z0-9-_.])(([^\s.]{1,63}\.)+[^\s.]{2,63})/ {print "Valid: vitalik.eth"}'
echo "sub.domain.eth" | gawk '/(?:^|[^a-zA-Z0-9-_.])(([^\s.]{1,63}\.)+[^\s.]{2,63})/ {print "Valid: sub.domain.eth"}'
echo "invalid" | gawk '/(?:^|[^a-zA-Z0-9-_.])(([^\s.]{1,63}\.)+[^\s.]{2,63})/ {print "Should not match: invalid"}'
echo "invalid.." | gawk '/(?:^|[^a-zA-Z0-9-_.])(([^\s.]{1,63}\.)+[^\s.]{2,63})/ {print "Should not match: invalid.."}'
Length of output: 946
Script:
#!/bin/bash
# Search for test files related to general.utils.ts
fd "general.utils.test" --type f
# Search for any test files that might contain ENS validation tests
rg -l "validateEnsName" --type=test
Length of output: 97
Script:
#!/bin/bash
# Search for test files related to general.utils.ts with common test file extensions
fd "general.utils.(test|spec)\.(ts|js|tsx|jsx)$"
# Search for any files containing validateEnsName tests
rg -l "validateEnsName"
Length of output: 409
Script:
#!/bin/bash
# Check the implementation and usage of validateEnsName
rg "validateEnsName" -B 2 -A 2
# Look for any test-related code near validateEnsName usage
rg "test|describe|it\(" --type ts --type tsx -C 5 -l | xargs rg "validateEnsName" -C 5 || true
Length of output: 23819
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Global/Header/index.tsx (1)
322-326:⚠️ Potential issueFix nested button click handling
The current implementation uses a div with onClick which can lead to accessibility issues and event bubbling problems.
- <div className={"bg-black h-full flex flex-col justify-center p-2 rounded-full"} - onClick={(event) => { - web3modalOpen() - event.stopPropagation() - }} + <button + type="button" + className={"bg-black h-full flex flex-col justify-center p-2 rounded-full"} + onClick={(event) => { + event.preventDefault() + web3modalOpen() + event.stopPropagation() + }} + aria-label="Connect wallet"
🧹 Nitpick comments (1)
src/components/Global/Header/index.tsx (1)
308-330: Improve button accessibility and loading stateThe button lacks proper ARIA attributes and the loading state could be enhanced.
<button className="btn btn-large bg-white px-2 py-2 gap-2" + aria-label={isAccountSubnamesPending ? "Loading ENS profile" : "Open ENS profile"} + disabled={isAccountSubnamesPending} onClick={() => { if (peanutSubname) { openEnsProfile(peanutSubname.ens) return } router.push('/claim-subname') }} > - {isAccountSubnamesPending ? 'Loading...' : peanutSubname?.ens ?? 'Claim ENS'} + {isAccountSubnamesPending ? ( + <span className="flex items-center gap-2"> + <span className="loading-spinner" aria-hidden="true" /> + Loading... + </span> + ) : ( + peanutSubname?.ens ?? 'Claim ENS' + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)src/components/Global/GeneralRecipientInput/index.tsx(2 hunks)src/components/Global/Header/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Global/GeneralRecipientInput/index.tsx
🔇 Additional comments (2)
package.json (1)
31-34: Verify compatibility between JustaName and JustWeb3 packages
The integration uses newer versions than previously recommended:
- @justaname.id/react: 0.3.162 (was 0.3.158)
- @justaname.id/sdk: 0.2.159 (was 0.2.155)
- @justweb3/ui and @justweb3/widget: 0.0.98 (was 0.0.94)
✅ Verification successful
Based on the results, I can generate the final response since we have sufficient information about the package releases.
Package versions are compatible - all released together
All package updates were released within a 2-minute window on December 12, 2024:
- @justaname.id/sdk: 09:55:15 UTC
- @justweb3/ui: 09:55:30 UTC
- @justaname.id/react: 09:55:37 UTC
- @justweb3/widget: 09:56:37 UTC
The synchronized release timing strongly indicates these versions were tested and released together as a compatible set.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes or compatibility issues between these versions
# Check the changelog for breaking changes
curl -s https://registry.npmjs.org/@justaname.id/react | jq -r '.versions["0.3.162"].dist.tarball' | xargs curl -s | tar -xzO package/CHANGELOG.md 2>/dev/null || echo "No CHANGELOG.md found"
curl -s https://registry.npmjs.org/@justaname.id/sdk | jq -r '.versions["0.2.159"].dist.tarball' | xargs curl -s | tar -xzO package/CHANGELOG.md 2>/dev/null || echo "No CHANGELOG.md found"
# Check if these versions were released together
curl -s https://registry.npmjs.org/@justaname.id/react | jq -r '.time["0.3.162"]'
curl -s https://registry.npmjs.org/@justaname.id/sdk | jq -r '.time["0.2.159"]'
curl -s https://registry.npmjs.org/@justweb3/ui | jq -r '.time["0.0.98"]'
curl -s https://registry.npmjs.org/@justweb3/widget | jq -r '.time["0.0.98"]'
Length of output: 823
src/components/Global/Header/index.tsx (1)
2-2: LGTM: Clean import organization
The imports are well-organized, with related hooks and configurations grouped together.
Also applies to: 14-16
| const { accountSubnames, isAccountSubnamesPending } = useAccountSubnames() | ||
| const router = useRouter() | ||
| const peanutSubname = useMemo(() => { | ||
| return accountSubnames.find((subname) => subname.ens.endsWith(peanutEnsDomain)) | ||
| }, [accountSubnames]) | ||
| const { openEnsProfile } = useJustWeb3() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for accountSubnames
The useAccountSubnames hook might fail to fetch subnames, but there's no error handling in place.
- const { accountSubnames, isAccountSubnamesPending } = useAccountSubnames()
+ const { accountSubnames, isAccountSubnamesPending, error } = useAccountSubnames()
+ useEffect(() => {
+ if (error) {
+ console.error('Failed to fetch subnames:', error)
+ // Consider showing a user-friendly error message
+ }
+ }, [error])Committable suggestion skipped: line range outside the PR's diff.
2 participants
This is a v1 of the justaname integration into peanut protocol. Subject to change according to received feedback.
Summary by CodeRabbit
Release Notes
New Features
ClaimSubnamecomponent for users to claim subnames.Headercomponent to conditionally render based on wallet connection status and account subnames.TableComponentto display ENS information in the contacts and accounts tabs.Improvements
Configuration Updates
JustaNameandJustWeb3providers to streamline integration.Bug Fixes
Chores
.gitignoreto exclude specific directories from version control.