[TASK-8898] feat: multi-device request links history#691
[TASK-8898] feat: multi-device request links history#691jjramirezn merged 3 commits intopeanut-wallet-devfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request modifies several components to remove local storage management for request links. In the Changes
Sequence Diagram(s)sequenceDiagram
participant HP as HistoryPage
participant UD as useDashboard Hook
participant API as History API
HP->>UD: Trigger useEffect on mount
UD->>API: Call composeLinkDataArray(address) asynchronously
API-->>UD: Return dashboard data
UD-->>HP: Update dashboard state (if component still mounted)
Possibly related PRs
Poem
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: 2
🔭 Outside diff range comments (2)
src/components/Request/Create/Views/Initial.view.tsx (2)
131-140: 🛠️ Refactor suggestionImprove link generation logic.
The current link generation has several areas for improvement:
- Manual string concatenation is error-prone
- Parameters are not URL encoded
- TODOs indicate need for better structure
Consider creating a utility function like this:
function generateRequestLink(details: { recipientAddress: string, tokenAmount?: string, tokenSymbol?: string, uuid: string }): string { const baseUrl = process.env.NEXT_PUBLIC_BASE_URL; const path = `/${encodeURIComponent(details.recipientAddress)}`; const params = new URLSearchParams(); if (details.tokenAmount) path += `/${encodeURIComponent(details.tokenAmount)}`; if (details.tokenSymbol) path += `/${encodeURIComponent(details.tokenSymbol)}`; params.append('id', details.uuid); return `${baseUrl}${path}?${params.toString()}`; }
174-203: 🛠️ Refactor suggestionRemove setTimeout and add cleanup for useEffect.
Using setTimeout for state updates is an anti-pattern that could lead to race conditions. Consider:
- Removing the artificial delay
- Adding cleanup function
Apply this change:
useEffect(() => { if (!isConnected) { setRecipientAddress('') setIsValidRecipient(false) return } // reset recipient when wallet type changes or when selected wallet changes if (address) { // reset states first setRecipientAddress('') setIsValidRecipient(false) - // set recipient to connected wallet address with a delay - setTimeout(() => { - setRecipientAddress(address) - setIsValidRecipient(true) - }, 100) + // set recipient to connected wallet address + setRecipientAddress(address) + setIsValidRecipient(true) // set chain and token for Peanut Wallet if (isPeanutWallet) { setSelectedChainID(PEANUT_WALLET_CHAIN.id.toString()) setSelectedTokenAddress(PEANUT_WALLET_TOKEN) } else { // set chain and token for external wallet setSelectedChainID(selectedChainID) setSelectedTokenAddress(selectedTokenAddress) } } + + return () => { + // Cleanup function to prevent state updates on unmounted component + setRecipientAddress('') + setIsValidRecipient(false) + } }, [isConnected, isPeanutWallet, address])
🧹 Nitpick comments (5)
src/components/Request/Pay/Views/Initial.view.tsx (3)
58-58: Consider moving the Squid Router URL to configuration.Hardcoding external service URLs makes it difficult to switch environments or handle service outages. Consider moving this URL to an environment configuration file.
- squidRouterUrl: 'https://apiplus.squidrouter.com/v2/route', + squidRouterUrl: config.SQUID_ROUTER_URL,
99-102: Consider defining error state type.Define a type for the error state object to improve type safety and maintainability.
+interface ErrorState { + showError: boolean; + errorMessage: string; +} - const [errorState, setErrorState] = useState<{ - showError: boolean - errorMessage: string - }>({ showError: false, errorMessage: '' }) + const [errorState, setErrorState] = useState<ErrorState>({ showError: false, errorMessage: '' })
523-546: Track points estimation implementation.The TODO comment for points estimation should be tracked in the issue system for proper follow-up.
Would you like me to create an issue to track the implementation of the points estimation feature? I can help draft the issue with the requirements and implementation details.
src/components/Request/Create/Views/Initial.view.tsx (1)
146-154: Enhance error handling.The current error handling uses a generic message and only logs details to console. Consider:
- Providing specific error messages based on error type
- Adding retry mechanism for network failures
- Implementing proper error tracking
Example implementation:
} catch (error) { let errorMessage = 'Failed to create link'; if (error instanceof TypeError) { errorMessage = 'Network error. Please try again.'; } else if (error instanceof Response) { errorMessage = `Server error: ${await error.text()}`; } setErrorState({ showError: true, errorMessage }); // Consider adding error tracking console.error('Failed to create link:', error); // Optional: Implement retry for network errors if (error instanceof TypeError && retryCount < MAX_RETRIES) { setTimeout(() => handleOnNext({...params, retryCount: (retryCount || 0) + 1}), RETRY_DELAY); return; } } finally {src/components/Dashboard/useDashboard.tsx (1)
219-223: Remove unused parameter.The
_itemsPerPageparameter is not used in the function. Consider removing it if it's no longer needed.const filterDashboardData = ( filterValue: string, - dashboardData: interfaces.IDashboardItem[], - _itemsPerPage: number + dashboardData: interfaces.IDashboardItem[] ) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/(mobile-ui)/history/page.tsx(2 hunks)src/components/Dashboard/useDashboard.tsx(6 hunks)src/components/Request/Create/Views/Initial.view.tsx(1 hunks)src/components/Request/Pay/Views/Initial.view.tsx(1 hunks)src/components/Request/Pay/Views/Success.view.tsx(0 hunks)src/utils/general.utils.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/components/Request/Pay/Views/Success.view.tsx
- src/utils/general.utils.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (6)
src/components/Request/Pay/Views/Initial.view.tsx (2)
29-29: LGTM! Import changes align with PR objectives.The removal of the
saveRequestLinkFulfillmentToLocalStorageimport aligns with the PR's goal of removing local storage management for request links.
416-575: LGTM! Well-structured UI implementation.The UI implementation is clean and well-organized with:
- Clear conditional rendering for different wallet types
- Appropriate error message display
- Well-presented network cost and slippage information
src/components/Request/Create/Views/Initial.view.tsx (2)
14-14: LGTM! Removal of local storage management.The removal of local storage utilities aligns with the PR objectives to eliminate local storage management for request links.
205-284: LGTM! Clean and well-structured render logic.The component's render logic is well-organized with:
- Clear component hierarchy
- Proper conditional rendering
- Good error state handling
src/app/(mobile-ui)/history/page.tsx (2)
24-24: LGTM! Good implementation of async data fetching.The stale check pattern is well-implemented to prevent memory leaks during component unmount.
Also applies to: 28-37
50-50: LGTM! Improved id generation.The new id format
${data.link ?? ''}-${data.txHash ?? ''}-${data.date}provides better uniqueness by including the date.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Dashboard/index.tsx (1)
185-185:⚠️ Potential issueReplace Math.random() in keys with stable identifiers.
Using
Math.random()for React keys is an anti-pattern that can cause unnecessary re-renders and performance issues. Use a stable, unique identifier instead.- key={(link.link ?? link.txHash ?? '') + Math.random()} + key={link.link ?? link.txHash ?? `${link.date}-${link.amount}-${link.type}`}- <div key={(link.link ?? link.txHash ?? '') + Math.random()}> + <div key={link.link ?? link.txHash ?? `${link.date}-${link.amount}-${link.type}`}>Also applies to: 235-235
🧹 Nitpick comments (2)
src/components/Dashboard/index.tsx (2)
52-69: Optimize legacy link handling.The legacy link handling logic is nested within the promise chain, which could delay the UI update. Consider extracting this logic to a separate effect or function.
Extract the legacy link handling:
+ const getLegacyLinks = (address: string) => { + const links: string[] = [] + const legacyLinkObject = utils.getAllLinksFromLocalStorage({ address }) + if (legacyLinkObject) { + legacyLinkObject.forEach((obj) => { + links.push(obj.link) + }) + } + const raffleLegacyLinkObject = utils.getAllRaffleLinksFromLocalstorage({ address }) + if (raffleLegacyLinkObject) { + raffleLegacyLinkObject.forEach((obj) => { + links.push(obj.link) + }) + } + return links + } useEffect(() => { let stale = false composeLinkDataArray(address ?? '').then((linkData) => { if (stale) return setTotalPages(Math.ceil(linkData.length / itemsPerPage)) setCurrentPage(1) setDashboardData(/* ... */) - if (address) { - const links: string[] = [] - const legacyLinkObject = utils.getAllLinksFromLocalStorage({ address: address }) - // ... legacy link handling - setLegacyLinks(links) - } else { - setLegacyLinks([]) - } }) return () => { stale = true } }, [address]) + + useEffect(() => { + setLegacyLinks(address ? getLegacyLinks(address) : []) + }, [address])
101-101: Remove commented code.Remove the commented out dependency array as it's no longer needed and could cause confusion.
- // }, [currentPage, dashboardData])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Dashboard/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
| useEffect(() => { | ||
| const linkData = composeLinkDataArray(address ?? '') | ||
| setTotalPages(Math.ceil(linkData.length / itemsPerPage)) | ||
| setCurrentPage(1) | ||
| setDashboardData( | ||
| linkData.sort((a, b) => { | ||
| const dateA = new Date(a.date).getTime() | ||
| const dateB = new Date(b.date).getTime() | ||
| if (dateA === dateB) { | ||
| return new Date(b.date).getTime() - new Date(a.date).getTime() | ||
| } else { | ||
| return dateB - dateA | ||
| } | ||
| }) | ||
| ) | ||
| let stale = false | ||
|
|
||
| if (address) { | ||
| const links: string[] = [] | ||
| const legacyLinkObject = utils.getAllLinksFromLocalStorage({ address: address }) | ||
| if (legacyLinkObject) { | ||
| legacyLinkObject.forEach((obj) => { | ||
| links.push(obj.link) | ||
| }) | ||
| } | ||
| const raffleLegacyLinkObject = utils.getAllRaffleLinksFromLocalstorage({ address: address }) | ||
| if (raffleLegacyLinkObject) { | ||
| raffleLegacyLinkObject.forEach((obj) => { | ||
| links.push(obj.link) | ||
| composeLinkDataArray(address ?? '').then((linkData) => { | ||
| if (stale) return | ||
| setTotalPages(Math.ceil(linkData.length / itemsPerPage)) | ||
| setCurrentPage(1) | ||
| setDashboardData( | ||
| linkData.sort((a, b) => { | ||
| const dateA = new Date(a.date).getTime() | ||
| const dateB = new Date(b.date).getTime() | ||
| if (dateA === dateB) { | ||
| return new Date(b.date).getTime() - new Date(a.date).getTime() | ||
| } else { | ||
| return dateB - dateA | ||
| } | ||
| }) | ||
| ) | ||
| if (address) { | ||
| const links: string[] = [] | ||
| const legacyLinkObject = utils.getAllLinksFromLocalStorage({ address: address }) | ||
| if (legacyLinkObject) { | ||
| legacyLinkObject.forEach((obj) => { | ||
| links.push(obj.link) | ||
| }) | ||
| } | ||
| const raffleLegacyLinkObject = utils.getAllRaffleLinksFromLocalstorage({ address: address }) | ||
| if (raffleLegacyLinkObject) { | ||
| raffleLegacyLinkObject.forEach((obj) => { | ||
| links.push(obj.link) | ||
| }) | ||
| } | ||
| setLegacyLinks(links) | ||
| } else { | ||
| setLegacyLinks([]) | ||
| } | ||
| setLegacyLinks(links) | ||
| } else { | ||
| setLegacyLinks([]) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the async operation.
The async operation lacks error handling, which could lead to unhandled promise rejections and a poor user experience if the data fetching fails.
Add error handling:
- composeLinkDataArray(address ?? '').then((linkData) => {
+ composeLinkDataArray(address ?? '').then((linkData) => {
if (stale) return
setTotalPages(Math.ceil(linkData.length / itemsPerPage))
// ... rest of the code
- })
+ }).catch((error) => {
+ if (stale) return
+ console.error('Failed to fetch link data:', error)
+ // Consider adding user feedback for the error state
+ })📝 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.
| useEffect(() => { | |
| const linkData = composeLinkDataArray(address ?? '') | |
| setTotalPages(Math.ceil(linkData.length / itemsPerPage)) | |
| setCurrentPage(1) | |
| setDashboardData( | |
| linkData.sort((a, b) => { | |
| const dateA = new Date(a.date).getTime() | |
| const dateB = new Date(b.date).getTime() | |
| if (dateA === dateB) { | |
| return new Date(b.date).getTime() - new Date(a.date).getTime() | |
| } else { | |
| return dateB - dateA | |
| } | |
| }) | |
| ) | |
| let stale = false | |
| if (address) { | |
| const links: string[] = [] | |
| const legacyLinkObject = utils.getAllLinksFromLocalStorage({ address: address }) | |
| if (legacyLinkObject) { | |
| legacyLinkObject.forEach((obj) => { | |
| links.push(obj.link) | |
| }) | |
| } | |
| const raffleLegacyLinkObject = utils.getAllRaffleLinksFromLocalstorage({ address: address }) | |
| if (raffleLegacyLinkObject) { | |
| raffleLegacyLinkObject.forEach((obj) => { | |
| links.push(obj.link) | |
| composeLinkDataArray(address ?? '').then((linkData) => { | |
| if (stale) return | |
| setTotalPages(Math.ceil(linkData.length / itemsPerPage)) | |
| setCurrentPage(1) | |
| setDashboardData( | |
| linkData.sort((a, b) => { | |
| const dateA = new Date(a.date).getTime() | |
| const dateB = new Date(b.date).getTime() | |
| if (dateA === dateB) { | |
| return new Date(b.date).getTime() - new Date(a.date).getTime() | |
| } else { | |
| return dateB - dateA | |
| } | |
| }) | |
| ) | |
| if (address) { | |
| const links: string[] = [] | |
| const legacyLinkObject = utils.getAllLinksFromLocalStorage({ address: address }) | |
| if (legacyLinkObject) { | |
| legacyLinkObject.forEach((obj) => { | |
| links.push(obj.link) | |
| }) | |
| } | |
| const raffleLegacyLinkObject = utils.getAllRaffleLinksFromLocalstorage({ address: address }) | |
| if (raffleLegacyLinkObject) { | |
| raffleLegacyLinkObject.forEach((obj) => { | |
| links.push(obj.link) | |
| }) | |
| } | |
| setLegacyLinks(links) | |
| } else { | |
| setLegacyLinks([]) | |
| } | |
| setLegacyLinks(links) | |
| } else { | |
| setLegacyLinks([]) | |
| }) | |
| useEffect(() => { | |
| let stale = false | |
| composeLinkDataArray(address ?? '') | |
| .then((linkData) => { | |
| if (stale) return | |
| setTotalPages(Math.ceil(linkData.length / itemsPerPage)) | |
| setCurrentPage(1) | |
| setDashboardData( | |
| linkData.sort((a, b) => { | |
| const dateA = new Date(a.date).getTime() | |
| const dateB = new Date(b.date).getTime() | |
| if (dateA === dateB) { | |
| return new Date(b.date).getTime() - new Date(a.date).getTime() | |
| } else { | |
| return dateB - dateA | |
| } | |
| }) | |
| ) | |
| if (address) { | |
| const links: string[] = [] | |
| const legacyLinkObject = utils.getAllLinksFromLocalStorage({ address: address }) | |
| if (legacyLinkObject) { | |
| legacyLinkObject.forEach((obj) => { | |
| links.push(obj.link) | |
| }) | |
| } | |
| const raffleLegacyLinkObject = utils.getAllRaffleLinksFromLocalstorage({ address: address }) | |
| if (raffleLegacyLinkObject) { | |
| raffleLegacyLinkObject.forEach((obj) => { | |
| links.push(obj.link) | |
| }) | |
| } | |
| setLegacyLinks(links) | |
| } else { | |
| setLegacyLinks([]) | |
| } | |
| }) | |
| .catch((error) => { | |
| if (stale) return | |
| console.error('Failed to fetch link data:', error) | |
| // Consider adding user feedback for the error state | |
| }) | |
| }, [address, itemsPerPage]) // Make sure to include dependencies if needed |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Dashboard/useDashboard.tsx (1)
140-156: 🛠️ Refactor suggestionAdd type information for history entries.
The TODO comment indicates missing type information. Using
anytype could lead to runtime errors.interface HistoryEntry { uuid: string userRole: 'SENDER' | 'RECEIVER' amount: number tokenSymbol: string chainId: number timestamp: string status: string txHash?: string }Apply this diff to add type safety:
- requestHistory.entries.forEach((entry: any) => { + requestHistory.entries.forEach((entry: HistoryEntry) => {
🧹 Nitpick comments (3)
src/components/Dashboard/useDashboard.tsx (3)
1-13: Clean up unused imports.Consider removing unused utility functions from the imports to maintain code cleanliness.
import { getTokenSymbol, getClaimedLinksFromLocalStorage, getCreatedLinksFromLocalStorage, getDirectSendFromLocalStorage, getOfframpClaimsFromLocalStorage, - getCashoutStatus, } from '@/utils'
34-50: Improve error handling for offramp claims.The current error handling defaults to 'claimed' status when an error occurs, which could be misleading. Consider:
- Using a more appropriate default status like 'error' or 'unknown'
- Adding more descriptive error messages for debugging
} catch (error) { - item.status = 'claimed' + item.status = 'error' - console.error(error) + console.error(`Error fetching offramp claim status for link ${item.link}:`, error) }
163-218: Simplify date sorting logic.The date sorting logic can be simplified by using a single comparison for both date and time.
case 'Date: new to old': _dashboardData.sort((a, b) => { - const dateA = new Date(a.date).getTime() - const dateB = new Date(b.date).getTime() - if (dateA === dateB) { - // If dates are equal, sort by time - return new Date(b.date).getTime() - new Date(a.date).getTime() - } else { - // Otherwise, sort by date - return dateB - dateA - } + return new Date(b.date).getTime() - new Date(a.date).getTime() }) break case 'Date: old to new': _dashboardData.sort((a, b) => { - const dateA = new Date(a.date).getTime() - const dateB = new Date(b.date).getTime() - if (dateA === dateB) { - // If dates are equal, sort by time - return new Date(a.date).getTime() - new Date(b.date).getTime() - } else { - // Otherwise, sort by date - return dateA - dateB - } + return new Date(a.date).getTime() - new Date(b.date).getTime() }) break
| const historyResponse = await fetch(`${PEANUT_API_URL}/users/history`, { | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${Cookies.get('jwt-token')}`, | ||
| }, | ||
| }) | ||
| const requestHistory = await historyResponse.json() |
There was a problem hiding this comment.
Add error handling for the history API call.
The API call lacks error handling which could lead to runtime errors if the request fails.
- const historyResponse = await fetch(`${PEANUT_API_URL}/users/history`, {
- headers: {
- 'Content-Type': 'application/json',
- Authorization: `Bearer ${Cookies.get('jwt-token')}`,
- },
- })
- const requestHistory = await historyResponse.json()
+ let requestHistory = { entries: [] }
+ try {
+ const historyResponse = await fetch(`${PEANUT_API_URL}/users/history`, {
+ headers: {
+ 'Content-Type': 'application/json',
+ Authorization: `Bearer ${Cookies.get('jwt-token')}`,
+ },
+ })
+ if (!historyResponse.ok) {
+ throw new Error(`HTTP error! status: ${historyResponse.status}`)
+ }
+ requestHistory = await historyResponse.json()
+ } catch (error) {
+ console.error('Failed to fetch request history:', error)
+ }📝 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 historyResponse = await fetch(`${PEANUT_API_URL}/users/history`, { | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${Cookies.get('jwt-token')}`, | |
| }, | |
| }) | |
| const requestHistory = await historyResponse.json() | |
| let requestHistory = { entries: [] } | |
| try { | |
| const historyResponse = await fetch(`${PEANUT_API_URL}/users/history`, { | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${Cookies.get('jwt-token')}`, | |
| }, | |
| }) | |
| if (!historyResponse.ok) { | |
| throw new Error(`HTTP error! status: ${historyResponse.status}`) | |
| } | |
| requestHistory = await historyResponse.json() | |
| } catch (error) { | |
| console.error('Failed to fetch request history:', error) | |
| } |
kushagrasarathe
left a comment
kushagrasarathe
left a comment
There was a problem hiding this comment.
lgtm, approved for fast tracking, left a comment/thought
| status: undefined, | ||
| message: link.reference ?? '', | ||
| attachmentUrl: link.attachmentUrl ?? '', | ||
| link: `${process.env.NEXT_PUBLIC_BASE_URL}/request/pay?id=${entry.uuid}`, |
There was a problem hiding this comment.
thought: this /request/pay route might be conflicting with req v2 changes 👀
2 participants
contributes to TASK-8277
Summary by CodeRabbit
Refactor
Dashboardcomponent's logic by adopting asynchronous data handling.Chores