feat/PRO-3349/transaction-history-tracking#320
Conversation
WalkthroughThis update introduces user operation status tracking with polling to the send modal components, adds new transaction history UI components ( Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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 (
|
Deploying x with
|
| Latest commit: |
147b333
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://73ff3632.x-e62.pages.dev |
| Branch Preview URL: | https://feat-pro-3349-transaction-hi.x-e62.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
231-236: 🛠️ Refactor suggestionApply the same localStorage constants refactoring as suggested for SendModalBatchesTabView.tsx.
This code has the same issue with hardcoded localStorage keys.
366-444: 🛠️ Refactor suggestionApply the same improvements suggested for the polling mechanism in SendModalBatchesTabView.tsx.
This polling implementation has the same issues:
- Extract magic numbers as constants
- Fix the context value check issue at line 433
- Consider extracting into a shared custom hook
🧹 Nitpick comments (7)
src/services/userOpStatus.ts (1)
8-8: Consider security implications of API key in URL.Embedding the API key directly in the URL could lead to it being logged in server access logs, browser history, or network monitoring tools.
Consider using the Authorization header instead:
- const url = `https://rpc.etherspot.io/v2/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_DATA_API_KEY}`; + const url = `https://rpc.etherspot.io/v2/${chainId}`; const response = await axios.post( url, { method: 'skandha_userOperationStatus', params: [userOpHash], }, { headers: { 'Content-Type': 'application/json', + 'Authorization': `Bearer ${process.env.REACT_APP_ETHERSPOT_DATA_API_KEY}`, }, timeout: 10000, } );Note: Verify that the Etherspot API supports Authorization header before implementing this change.
src/components/BottomMenuModal/HistoryModal/HistoryModal.tsx (3)
9-11: Improve prop documentation.Consider enhancing the interface documentation to be more descriptive about the animation purpose.
- isContentVisible?: boolean; // for animation purpose to not render rest of content and return main wrapper only + /** + * Controls content visibility for animation purposes. + * When false, only renders the wrapper without content to maintain layout during transitions. + */ + isContentVisible?: boolean;
20-35: Consider adding accessibility attributes.The loading skeleton could benefit from accessibility attributes to inform screen readers about the loading state.
- <Wrapper id="history-modal-loader"> + <Wrapper id="history-modal-loader" role="status" aria-label="Loading transaction history"> <HistoryCard>
92-102: Clean up commented code or document future use.The commented
TransactionStatuscomponent should either be removed if not needed or better documented for future implementation.Consider either:
- Removing the commented code if it's not immediately needed
- Adding a more detailed TODO comment explaining when/how it will be used
- Moving it to a separate file if it's part of planned functionality
src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx (3)
1-1: Specify the ESLint rule being disabled.The generic ESLint disable comment should specify which rule is being disabled for better code maintainability.
-/* eslint-disable no-nested-ternary */ +/* eslint-disable no-nested-ternary */
73-84: Ensure consistent chain ID type handling.The chain ID is being converted to string multiple times. Consider storing it as a string from the start for consistency.
if ( latestUserOpChainId && - latestUserOpChainId.toString() !== - localStorage.getItem('latestUserOpChainId') + String(latestUserOpChainId) !== localStorage.getItem('latestUserOpChainId') ) { - localStorage.setItem( - 'latestUserOpChainId', - latestUserOpChainId.toString() - ); - setDisplayChainId(latestUserOpChainId.toString()); + const chainIdStr = String(latestUserOpChainId); + localStorage.setItem('latestUserOpChainId', chainIdStr); + setDisplayChainId(chainIdStr); }
88-93: Consider simplifying the step index calculation.The current IIFE pattern works but could be simplified using an object map for better maintainability.
- const stepIndex = (() => { - if (displayStatus === 'Sending') return 0; - if (displayStatus === 'Sent') return 1; - if (displayStatus === 'Confirmed' || displayStatus === 'Failed') return 2; - return -1; - })(); + const statusToStepIndex: Record<string, number> = { + 'Sending': 0, + 'Sent': 1, + 'Confirmed': 2, + 'Failed': 2, + }; + const stepIndex = statusToStepIndex[displayStatus || ''] ?? -1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/pillarx-app/components/MediaGridCollection/tests/__snapshots__/DisplayCollectionImage.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
src/components/BottomMenuModal/HistoryModal/HistoryModal.tsx(1 hunks)src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx(1 hunks)src/components/BottomMenuModal/HistoryModal/TransactionsHistory.tsx(3 hunks)src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx(4 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(6 hunks)src/components/BottomMenuModal/index.tsx(1 hunks)src/hooks/__tests__/useAccountTransactionHistory.test.tsx(4 hunks)src/hooks/useAccountTransactionHistory.tsx(1 hunks)src/providers/AccountTransactionHistoryProvider.tsx(3 hunks)src/services/userOpStatus.ts(1 hunks)src/utils/common.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/utils/common.ts (5)
src/components/Form/AssetSelect/index.tsx (1)
AssetSelectOption(50-50)src/types/blockchain.ts (1)
ITransaction(22-27)src/types/index.ts (1)
SendModalData(20-22)src/apps/the-exchange/utils/blockchain.ts (1)
processEth(7-13)src/utils/blockchain.ts (1)
decodeSendTokenCallData(204-208)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (11)
src/components/BottomMenuModal/index.tsx (1)
13-13: LGTM! Import path correctly updated for file reorganization.The import path update properly reflects the new file structure where HistoryModal has been moved to its own subdirectory.
src/hooks/useAccountTransactionHistory.tsx (1)
24-24:Details
✅ Verification successful
Verify that this change doesn't break existing hook consumers.
The hook now returns the entire
context.dataobject instead of justcontext.data.history. This supports the new user operation status tracking features but could potentially break existing code that expects only the history data.
🏁 Script executed:
#!/bin/bash # Description: Check for existing usage of useAccountTransactionHistory to ensure backward compatibility # Expected: Find usages and verify they can handle the expanded return object echo "Searching for useAccountTransactionHistory usage patterns..." rg -A 3 -B 1 "useAccountTransactionHistory" --type tsx --type ts echo -e "\n=== Checking for destructuring patterns that might break ===" rg -A 2 "const.*=.*useAccountTransactionHistory" --type tsx --type tsLength of output: 460
Let’s re-run the search without restricting by file type to catch all usages:
🏁 Script executed:
#!/bin/bash echo "🔍 Searching for all uses of useAccountTransactionHistory across the repo" rg -n "useAccountTransactionHistory" -A3 -B1 echo -e "\n🔍 Extracting assignment/destructuring patterns" rg -n "useAccountTransactionHistory" | grep -E "const|let|var"Length of output: 7228
To confirm what the hook now returns and how it’s consumed, let’s inspect the hook implementation and its call sites:
🏁 Script executed:
#!/bin/bash echo "=== useAccountTransactionHistory implementation ===" sed -n '1,200p' src/hooks/useAccountTransactionHistory.tsx echo -e "\n=== SendModalBatchesTabView usage ===" sed -n '65,85p' src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx echo -e "\n=== SendModalTokensTabView usage ===" sed -n '105,135p' src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx echo -e "\n=== TransactionInfo usage ===" sed -n '13,33p' src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsxLength of output: 3704
No breaking changes detected in hook consumers
All existing usages of
useAccountTransactionHistoryhave been updated to destructure the fulldataobject (includinguserOpStatus,setTransactionHash, etc.) rather than relying solely onhistory. No calls remain that expect the hook to return only thehistoryarray.• src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx
• src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
• src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx
• src/hooks/tests/useAccountTransactionHistory.test.tsxAll consumers and tests now access the expanded return shape, so this change is backward-compatible.
src/components/BottomMenuModal/HistoryModal/TransactionsHistory.tsx (2)
10-11: LGTM! Import paths correctly updated for file reorganization.The import path updates properly reflect the new directory structure after moving files to subdirectories.
Also applies to: 14-14, 21-21, 24-27
110-110:Details
✅ Verification successful
Verify that removing the wrapper element doesn't break functionality.
The JSX wrapper was changed from
<Wrapper id="history-modal">to a React fragment<>. This removes a DOM element and itsid="history-modal"attribute, which might be referenced by CSS selectors, tests, or other JavaScript code.Also applies to: 172-172
🏁 Script executed:
#!/bin/bash # Description: Check for references to the removed "history-modal" id # Expected: Find any CSS, test files, or JavaScript that might reference this id echo "Searching for references to 'history-modal' id..." rg -i "history-modal" --type css --type scss --type ts --type tsx --type js --type jsx echo -e "\n=== Checking for querySelector usage ===" rg "getElementById.*history-modal|querySelector.*history-modal"Length of output: 393
🏁 Script executed:
#!/bin/bash # Re-run a global search without restricting file types to ensure we catch CSS/SCSS echo "Searching for any references to 'history-modal' across the repo..." rg -n "history-modal" . echo -e "\n=== Checking for DOM queries referencing 'history-modal' ===" rg -n "getElementById.*history-modal|querySelector.*history-modal" .Length of output: 708
No references to
#history-modalfound—removal is safe
A repo-wide search (including.css,.ts/.tsx/.js/.jsx, and DOM selectors) turned up no occurrences of the removedid="history-modal"outside of the original wrapper inHistoryModal.tsx. Since no CSS, tests, or scripts rely on it, replacing the<Wrapper id="history-modal">with<>in TransactionsHistory.tsx won’t break functionality.src/hooks/__tests__/useAccountTransactionHistory.test.tsx (3)
30-39: LGTM! Proper test mock extension.The mock context has been correctly extended to include all the new state variables and setters that were added to the provider interface. This ensures the tests accurately reflect the updated context structure.
48-50: Good addition of test cleanup.Adding
afterEachto clear all mocks ensures proper test isolation and prevents test interference.
74-74:Details
✅ Verification successful
Verify the hook's return value change.
The assertion now expects the full
dataobject instead of just thehistoryproperty, indicating the hook's return value has changed. Ensure this aligns with the actual hook implementation.
🏁 Script executed:
#!/bin/bash # Description: Verify what the useAccountTransactionHistory hook returns # Expected: Should return the full context data object, not just history rg -A 10 "return.*useContext" src/hooks/useAccountTransactionHistoryLength of output: 149
🏁 Script executed:
#!/bin/bash # Locate the hook file for useAccountTransactionHistory hook_file=$(rg --files-with-matches "useAccountTransactionHistory" -g "*.ts*" src/hooks | head -n1) echo "Found hook file: $hook_file" # Show lines around useContext to see what is being returned rg -n -C3 "useContext" "$hook_file" # Show lines around return statements to confirm the hook's return value rg -n -C3 "return" "$hook_file"Length of output: 1033
No action needed: Test assertion aligns with hook implementation.
- Confirmed in src/hooks/useAccountTransactionHistory.tsx (line 24) that the hook returns
context.data.- The test’s
expect(result.current).toEqual(mockContextValue.data)correctly matches this behavior.src/providers/AccountTransactionHistoryProvider.tsx (3)
40-57: Well-structured context interface extension.The new state variables for user operation tracking are properly typed and follow consistent naming conventions. The union type for
userOpStatusprovides clear state transitions.
83-94: Proper state hook initialization.All new state variables are correctly initialized with appropriate default values and proper TypeScript typing.
184-202: Correct context data and dependency management.The context data object and useMemo dependencies have been properly updated to include all new state variables, ensuring efficient re-rendering and proper context updates.
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
360-362: Good use of shared utility function.The implementation correctly uses the
transactionDescriptionutility for consistent transaction descriptions across the application.
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3349/transaction-history-tracking)
| {/* UserOp status */} | ||
| <p className="text-sm font-medium text-light_purple"> | ||
| Status:{' '} | ||
| <span className="font-normal text-white">{displayStatus ?? 'N/A'}</span> |
There was a problem hiding this comment.
I think if we have no display status available - display something like "Starting..." or something similar, as "N/A" gives off a negative effect
There was a problem hiding this comment.
Yeah agreed, I've changed that now
| Tx info:{' '} | ||
| <span className="font-normal text-white">{displayInfo ?? 'N/A'}</span> |
There was a problem hiding this comment.
This one is less of an issue because it's data - but as a suggestion you could add "Waiting..." here
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
Just FYI i'm approving this because the comments / questions i have i don't deem them to be a showstopper at this stage, but here are my two outstanding quetions that we can talk about outside this PR:
- Is the data API key the one we should be using? I thought it was not used anymore?
- Am i right in thinking the local storage only supports the 1 latest UserOp at a time?
But other than that it LGTM!
| <p className="text-xs text-light_purple font-normal truncate"> | ||
| Tx Hash:{' '} | ||
| <span className="font-normal text-white"> | ||
| {displayHash ?? 'N/A'} |
There was a problem hiding this comment.
As above but we can revisit this later
| <div className="flex text-[10px] italic text-white/[.5] font-normal p-3 border-[1px] border-medium_grey rounded-md"> | ||
| Please note that this information confirms whether the transaction was | ||
| sent but does not indicate its status. To verify the status, please | ||
| check the tx hash on blockscans. |
There was a problem hiding this comment.
| check the tx hash on blockscans. | |
| check the transaction hash on relevant blockchain explorer. |
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3349/transaction-history-tracking)
3 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores