[TASK-7278] feat: remember selected address #556
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@jjramirezn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 56 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. WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (1)
src/context/walletContext/walletContext.tsx (1)
156-161: Enhance localStorage managementThe current implementation could be improved by:
- Cleaning up localStorage when address is cleared
- Adding error handling for localStorage operations
- Considering debouncing for frequent updates
Consider this enhanced implementation:
useEffect(() => { - if (selectedAddress) { + try { + if (selectedAddress) { localStorage.setItem('selectedAddress', selectedAddress) + } else { + localStorage.removeItem('selectedAddress') + } + } catch (error) { + console.error('Failed to persist selected address:', error) } }, [selectedAddress])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/context/walletContext/walletContext.tsx(2 hunks)
🔇 Additional comments (1)
src/context/walletContext/walletContext.tsx (1)
Line range hint 54-161: Consider security and testing implications
While the implementation achieves the goal of persisting the selected address, please consider:
- Security: Document the security implications of storing wallet addresses in localStorage
- Privacy: Add a note in the privacy policy about storing wallet information locally
- Testing: Ensure test coverage for:
- localStorage persistence
- Address validation
- SSR compatibility
- Error scenarios
Let's verify the test coverage:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/utils/general.utils.ts (3)
Line range hint
86-97: Consider enhancing data security and error handling insaveToLocalStorage.The function could benefit from several security improvements:
- Remove or conditionally enable debug logging in production
- Add data validation/sanitization
- Consider encrypting sensitive data
- Add size limits to prevent localStorage quota issues
export const saveToLocalStorage = (key: string, data: any) => { try { + if (!key || !data) { + throw new Error('Invalid key or data'); + } + // Add size validation + const serializedData = JSON.stringify(data); + if (serializedData.length > 5242880) { // 5MB limit + throw new Error('Data exceeds size limit'); + } - // Convert the data to a string before storing it in localStorage - const serializedData = JSON.stringify(data) if (typeof localStorage === 'undefined') return localStorage.setItem(key, serializedData) - console.log(`Saved ${key} to localStorage:`, data) + if (process.env.NODE_ENV !== 'production') { + console.log(`Saved ${key} to localStorage`); + } } catch (error) { console.error('Error saving to localStorage:', error) + throw error; // Re-throw to allow caller to handle } }
Line range hint
98-112: Enhance error handling and type safety ingetFromLocalStorage.The function needs improvements in error handling and type safety:
- Remove or conditionally enable debug logging in production
- Add type parameter for better type safety
- Improve error handling for JSON parsing
-export const getFromLocalStorage = (key: string) => { +export const getFromLocalStorage = <T>(key: string): T | null => { try { + if (!key) { + throw new Error('Invalid key'); + } if (typeof localStorage === 'undefined') return null const data = localStorage.getItem(key) if (data === null) { - console.log(`No data found in localStorage for ${key}`) + if (process.env.NODE_ENV !== 'production') { + console.log(`No data found in localStorage for ${key}`); + } return null } - const parsedData = JSON.parse(data) - console.log(`Retrieved ${key} from localStorage:`, parsedData) - return parsedData + try { + const parsedData = JSON.parse(data) as T; + if (process.env.NODE_ENV !== 'production') { + console.log(`Retrieved ${key} from localStorage`); + } + return parsedData; + } catch (parseError) { + console.error('Error parsing JSON from localStorage:', parseError); + return null; + } } catch (error) { console.error('Error getting data from localStorage:', error) + return null; } }
Line range hint
86-112: Consider implementing a centralized storage service.Both functions are part of a larger pattern of localStorage usage throughout the file. Consider creating a centralized storage service that:
- Provides consistent error handling
- Implements encryption for sensitive data
- Manages storage quotas
- Handles data migration
- Provides type safety
Example implementation:
interface StorageService { save<T>(key: string, data: T): Promise<void>; get<T>(key: string): Promise<T | null>; remove(key: string): Promise<void>; clear(): Promise<void>; } class LocalStorageService implements StorageService { private readonly encrypt = (data: string): string => { // Implement encryption return data; }; private readonly decrypt = (data: string): string => { // Implement decryption return data; }; async save<T>(key: string, data: T): Promise<void> { // Implementation with encryption and validation } async get<T>(key: string): Promise<T | null> { // Implementation with decryption and type safety } // ... other methods }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/context/walletContext/walletContext.tsx(3 hunks)src/utils/general.utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/context/walletContext/walletContext.tsx
| getFromLocalStorage, | ||
| saveToLocalStorage, |
There was a problem hiding this comment.
good on using existing functions!
Thought: we should clean up general.utils.ts, we save tons of shit to localStorage in random ways. Need to clean this up - probably same time we redo our tx indexing
There was a problem hiding this comment.
Issue, blocking: in this PR we set a specific key "selectedAddress" to localStorage. I think this can lead to localStorage key pollution, and it's not clear what the item means, on its own.
Since last selected address wont be the only thing we should store for users for convenience, e.g. they might have a preffered token or chain, or they might want dark or light mode, its preferable to group these all in one. We do have a getPeanutPreferences & updatePeanutPreferences. Right now it's pretty empty and the 3 items it has are badly named, but imo we should make use of a user-preferences type of pattern. Wdyt?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/context/tokenSelector.context.tsx (2)
39-48: LGTM! Consider adding type safety.The initialization logic correctly handles user preferences while providing sensible defaults.
Consider adding type definitions for the token data structure:
interface TokenData { address: string; chainId: string; decimals: number; } const initialTokenData: TokenData = { address: '0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85', // USDC chainId: '10', // Optimism decimals: 6, }
Line range hint
76-124: Enhance error logging for better debugging.The error handling in fetchAndSetTokenPrice could be more descriptive to aid in debugging issues.
Consider enhancing the error logging:
} catch (error) { - console.log('error fetching tokenPrice, falling back to tokenDenomination') + console.error('Failed to fetch token price:', { + tokenAddress, + chainId, + error: error instanceof Error ? error.message : String(error) + }) }src/components/Create/Link/Confirm.view.tsx (1)
213-221: LGTM! Consider adding error handling for preferences update.The user preferences update is well-structured with appropriate null checks. However, the update could fail silently.
Consider adding error handling:
if (selectedChainID && selectedTokenAddress && selectedTokenDecimals) { + try { updateUserPreferences({ lastUsedToken: { chainId: selectedChainID, address: selectedTokenAddress, decimals: selectedTokenDecimals, }, }) + } catch (error) { + console.warn('Failed to update user preferences:', error) + // Non-blocking error - don't throw + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/Create/Link/Confirm.view.tsx(2 hunks)src/context/tokenSelector.context.tsx(2 hunks)src/context/walletContext/walletContext.tsx(3 hunks)src/utils/general.utils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/context/walletContext/walletContext.tsx
- src/utils/general.utils.ts
🧰 Additional context used
📓 Learnings (1)
src/context/tokenSelector.context.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
jjramirezn
force-pushed
the
feat/remember-selected-address
branch
from
258e5ad to
c88869d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Create/Link/Confirm.view.tsx (1)
213-221: Consider moving preferences update after all operationsWhile the code correctly updates user preferences with proper null checks, consider moving this update after
refetchBalancesto ensure all operations complete successfully before persisting the preferences.- if (selectedChainID && selectedTokenAddress && selectedTokenDecimals) { - updateUserPreferences({ - lastUsedToken: { - chainId: selectedChainID, - address: selectedTokenAddress, - decimals: selectedTokenDecimals, - }, - }) - } - onNext() - refetchBalances(address ?? '') + await refetchBalances(address ?? '') + + if (selectedChainID && selectedTokenAddress && selectedTokenDecimals) { + updateUserPreferences({ + lastUsedToken: { + chainId: selectedChainID, + address: selectedTokenAddress, + decimals: selectedTokenDecimals, + }, + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/Create/Link/Confirm.view.tsx(2 hunks)src/context/tokenSelector.context.tsx(2 hunks)src/context/walletContext/walletContext.tsx(3 hunks)src/utils/general.utils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/general.utils.ts
- src/context/walletContext/walletContext.tsx
- src/context/tokenSelector.context.tsx
🔇 Additional comments (1)
src/components/Create/Link/Confirm.view.tsx (1)
13-13: LGTM: Import change aligns with preference management refactoring
The rename from updatePeanutPreferences to updateUserPreferences is consistent with the broader refactoring of the preferences management system.
Summary by CodeRabbit
New Features
Bug Fixes