feat: add direct upload for arns logo #886
Conversation
WalkthroughAdds the runtime dependency Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LogoRow
participant UploadLogoModal
participant useUploadArNSLogo
participant TurboClient
participant ArNS
User->>LogoRow: Click Upload icon
LogoRow->>UploadLogoModal: open()
User->>UploadLogoModal: Select image
UploadLogoModal->>UploadLogoModal: show preview
User->>UploadLogoModal: Click Next
UploadLogoModal->>useUploadArNSLogo: handleUploadArNSLogo(image)
useUploadArNSLogo->>TurboClient: init (wallet-dependent: Arconnect/WANDER/ethers)
TurboClient->>ArNS: uploadFile(stream, metadata)
ArNS-->>TurboClient: transaction_id
TurboClient-->>useUploadArNSLogo: return id
useUploadArNSLogo-->>UploadLogoModal: transaction_id
UploadLogoModal->>UploadLogoModal: validate id (isArweaveTransactionID)
UploadLogoModal->>LogoRow: confirm(transaction_id)
LogoRow->>LogoRow: handleSave(transaction_id)
UploadLogoModal->>User: close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/forms/DomainSettings/LogoRow.tsx (1)
45-55: Validate TX ID inside handleSave to prevent invalid commits.Pressing Enter triggers the confirm modal without prior validation;
handleSaveshould enforceisArweaveTransactionIDbefore callingconfirm.async function handleSave(transactionId: string) { try { + if (!isArweaveTransactionID(transactionId)) { + eventEmitter.emit('error', { + name: 'Set Logo', + message: 'Logo must be an Arweave transaction ID', + }); + return; + } await confirm(transactionId); } catch (error) { eventEmitter.emit('error', error); } finally {
🧹 Nitpick comments (9)
src/hooks/useUploadArNSLogo.tsx (3)
39-47: Use named import and guard provider presence.
- Prefer
import { BrowserProvider } from 'ethers'andnew BrowserProvider(window.ethereum)to avoid bundling the full ethers namespace.- Guard
window.ethereumbefore constructing the provider to give a clearer error.- import { ethers } from 'ethers'; + import { BrowserProvider } from 'ethers'; @@ - case WALLET_TYPES.ETHEREUM: { - const ethersProvider = new ethers.BrowserProvider(window.ethereum); + case WALLET_TYPES.ETHEREUM: { + if (typeof window === 'undefined' || !window.ethereum) { + throw new Error('Ethereum provider not found in browser.'); + } + const ethersProvider = new BrowserProvider(window.ethereum); const ethersSigner = await ethersProvider.getSigner(); return TurboFactory.authenticated({ token: 'ethereum', walletAdapter: { getSigner: () => ethersSigner as any }, ...turboAuthenticatedDefaults, }); }Also applies to: 9-10
76-78: Preserve error details; avoid[object Object].Wrap errors safely to keep message context.
- } catch (error) { - throw new Error(`Failed to upload logo: ${error}`); - } + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to upload logo: ${msg}`); + }
25-53: Verify undefined handling and add window guards for robustness.The call site at line 62 explicitly handles undefined with
if (!turbo) return;, so the fallthrough is not truly silent. However, the review's suggestions remain valid:
Missing BEACON implementation: WALLET_TYPES has four cases (WANDER, ARWEAVE_APP, ETHEREUM, BEACON), but only three are handled. The TODO comment indicates BEACON is unfinished. Adding a
defaultcase to throw an error is better than silently returning undefined.window.arweaveWallet guard: Should check existence before passing to
ArconnectSignerconstructor to prevent runtime errors.Ethereum config confirmed correct per TurboFactory documentation:
token: 'ethereum'with ethers Signer fromprovider.getSigner()is the standard pattern.These are prudent defensive programming practices to improve error transparency and prevent silent failures when new wallet types are added or prerequisites are missing.
src/components/forms/DomainSettings/LogoRow.tsx (2)
119-135: Add an accessible name and button type; remove redundant key.
- The upload icon button needs an accessible label.
- Add
type="button"to avoid implicit form submit if nested later.key={1}on Tooltip isn’t needed here.- <Tooltip - key={1} + <Tooltip message={'Upload new logo'} icon={ - <button + <button + type="button" + aria-label="Upload new logo" className="fill-grey hover:fill-white" onClick={() => { setShowUploadLogoModal(true); }} >
64-83: Remove redundant key prop.
key={1}on this Tooltip isn’t part of a list and has no effect. Safe to remove.- <Tooltip - key={1} + <Tooltipsrc/components/modals/UploadLogoModal/UploadLogoModal.tsx (3)
4-4: Revoke object URLs to prevent memory leaks.Call
URL.revokeObjectURLwhen replacing or unmounting the preview URL.-import { ChangeEvent, useState } from 'react'; +import { ChangeEvent, useEffect, useState } from 'react'; @@ const selectImage = (e: ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0]; if (!file) { - setImage(null); - setImageUrl(''); + setImage(null); + if (imageUrl) URL.revokeObjectURL(imageUrl); + setImageUrl(''); return; } - setImage(file); - setImageUrl(URL.createObjectURL(file)); + setImage(file); + if (imageUrl) URL.revokeObjectURL(imageUrl); + setImageUrl(URL.createObjectURL(file)); }; + useEffect(() => { + return () => { + if (imageUrl) URL.revokeObjectURL(imageUrl); + }; + }, [imageUrl]);Also applies to: 15-29
57-63: A11y and double-submit guard while loading.
- Add an accessible label and disable the input while uploading.
- Don’t allow
onNextwhileloadingto avoid duplicate uploads.- <input + <input onChange={selectImage} type="file" accept="image/*" name="logo" + aria-label="Choose logo image" + disabled={loading} /> @@ - onNext={image ? handleUpdateLogo : undefined} - nextText={loading ? '...' : 'Upload'} + onNext={image && !loading ? handleUpdateLogo : undefined} + nextText={loading ? '...' : 'Upload'}Also applies to: 75-77
31-46: Use finally for loading state cleanup.Consolidate
setLoading(false)in afinallyto avoid duplication.const handleUpdateLogo = async () => { if (!image) return; try { setLoading(true); const transaction_id = await handleUploadArNSLogo(); if (!transaction_id || !isArweaveTransactionID(transaction_id)) { throw new Error('Logo upload failed'); } await confirm(transaction_id); - setLoading(false); close(); } catch (error) { eventEmitter.emit('error', error); - setLoading(false); } + finally { + setLoading(false); + } };package.json (1)
53-53: Switch to named import of BrowserProvider to enable tree-shaking.The file imports the entire ethers namespace but uses only
BrowserProvider. Change line 9 from:import { ethers } from 'ethers';to:
import { BrowserProvider } from 'ethers';Then update line 44 from
new ethers.BrowserProvider(window.ethereum)tonew BrowserProvider(window.ethereum). This reduces bundle size and allows Vite's optimizeDeps to work more efficiently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)src/components/forms/DomainSettings/LogoRow.tsx(4 hunks)src/components/modals/UploadLogoModal/UploadLogoModal.tsx(1 hunks)src/hooks/useUploadArNSLogo.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/modals/UploadLogoModal/UploadLogoModal.tsx (2)
src/hooks/useUploadArNSLogo.tsx (1)
useUploadArNSLogo(21-82)src/utils/transactionUtils/transactionUtils.tsx (1)
isArweaveTransactionID(40-48)
src/components/forms/DomainSettings/LogoRow.tsx (3)
src/components/data-display/AntLogoIcon.tsx (1)
AntLogoIcon(7-57)src/services/arweave/ArweaveTransactionID.ts (1)
ArweaveTransactionID(7-31)src/utils/constants.ts (1)
ARNS_TX_ID_ENTRY_REGEX(77-77)
src/hooks/useUploadArNSLogo.tsx (2)
src/utils/constants.ts (1)
NETWORK_DEFAULTS(135-169)src/state/contexts/WalletState.tsx (1)
useWalletState(49-50)
|
@atticusofsparta just added extra tags |
1 participant
Summary by CodeRabbit