Fe/activity pending sdk #1749
Fe/activity pending sdk #1749
Conversation
bigboydiamonds
requested review from
abtestingalpha,
nautsimon and
aureliusbtc
as code owners
WalkthroughThe recent updates involve enhancing the transaction management and UI components of the Synapse interface. A new feature is introduced to display pending transactions conditionally and manage transaction status updates more effectively. The updates integrate the Changes
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's AI:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (13)
- packages/synapse-interface/components/Portfolio/Activity.tsx (2 hunks)
- packages/synapse-interface/components/Portfolio/PortfolioTabManager.tsx (2 hunks)
- packages/synapse-interface/components/_Transaction/_Transaction.tsx (1 hunks)
- packages/synapse-interface/components/_Transaction/_Transactions.tsx (1 hunks)
- packages/synapse-interface/components/_Transaction/helpers/constants.ts (1 hunks)
- packages/synapse-interface/components/_Transaction/helpers/getExplorerAddressLink.ts (1 hunks)
- packages/synapse-interface/components/_Transaction/helpers/getTxBlockExplorerLink.ts (1 hunks)
- packages/synapse-interface/components/_Transaction/helpers/useBridgeTxStatus.tsx (1 hunks)
- packages/synapse-interface/pages/_app.tsx (2 hunks)
- packages/synapse-interface/slices/_transactions/hooks.ts (1 hunks)
- packages/synapse-interface/slices/_transactions/reducer.ts (1 hunks)
- packages/synapse-interface/slices/_transactions/updater.tsx (1 hunks)
- packages/synapse-interface/store/reducer.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/synapse-interface/components/_Transaction/helpers/constants.ts
Additional comments: 8
packages/synapse-interface/slices/_transactions/hooks.ts (1)
- 4-6: The
use_TransactionsStatehook correctly uses theuseAppSelectorcustom hook to access the_transactionsslice of the Redux state.packages/synapse-interface/slices/_transactions/updater.tsx (1)
- 9-37: The
Updatercomponent is implemented correctly, following React and Redux best practices for managing side effects in the application state.packages/synapse-interface/store/reducer.ts (1)
- 24-24: The addition of the
_transactionsreducer to thepersistedReducersobject is correctly implemented for setting up persisted state in Redux.packages/synapse-interface/components/_Transaction/_Transactions.tsx (1)
- 9-60: The
_Transactionscomponent is implemented correctly, using hooks for state and effect management, and conditionally rendering transaction details based on the state.packages/synapse-interface/components/_Transaction/helpers/useBridgeTxStatus.tsx (1)
- 15-79: The
useBridgeTxStatushook is implemented correctly, managing the status of a bridge transaction and following React best practices for custom hooks and state management.packages/synapse-interface/components/Portfolio/PortfolioTabManager.tsx (1)
- 1-15: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-40]
The changes to the
PortfolioTabManagercomponent, including the use ofuseAccountfrom 'wagmi' and the inclusion of the_Transactionscomponent with theconnectedAddressprop, are correctly implemented.packages/synapse-interface/pages/_app.tsx (1)
- 141-141: The addition of the
_TransactionsUpdatercomponent to theUpdatersfunction is correctly implemented, following the common pattern for global updaters in Next.js applications.packages/synapse-interface/components/Portfolio/Activity.tsx (1)
- 232-238: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [235-279]
The commented-out section for rendering pending transactions in the
Activitycomponent may indicate ongoing development or refactoring. Ensure that this is intentional and that any necessary logic is handled elsewhere if this section is to remain commented out.
| export const getExplorerAddressLink = (chainId: number, address: string) => { | ||
| const blockExplorer = ExplorerLinks[chainId] | ||
|
|
||
| if (blockExplorer && address) { | ||
| const explorerUrl = `${blockExplorer}/address/${address}` | ||
| const explorerName = ExplorerNames[chainId] | ||
|
|
||
| return [explorerUrl, explorerName] | ||
| } | ||
|
|
||
| console.error('getExplorerAddressLink: ChainId or Address missing') | ||
| return [null, null] | ||
| } |
There was a problem hiding this comment.
Consider improving the error handling in getExplorerAddressLink by throwing an error or returning undefined instead of [null, null] to avoid potential type issues when the function is used.
| export const getTxBlockExplorerLink = (chainId: number, txHash: string) => { | ||
| const blockExplorer = ExplorerLinks[chainId] | ||
|
|
||
| if (blockExplorer && txHash) { | ||
| const explorerUrl = `${blockExplorer}/tx/${txHash}` | ||
| const explorerName = ExplorerNames[chainId] | ||
|
|
||
| return [explorerUrl, explorerName] | ||
| } | ||
|
|
||
| console.error('getTxBlockExplorerLink: ChainID or Transaction Hash missing') | ||
| return [null, null] | ||
| } |
There was a problem hiding this comment.
Consider improving the error handling in getTxBlockExplorerLink by throwing an error or returning undefined instead of [null, null] to avoid potential type issues when the function is used.
| clearTransactions: (transactions: _TransactionsState) => { | ||
| transactions = {} // eslint-disable-line | ||
| }, |
There was a problem hiding this comment.
The clearTransactions reducer should return a new object instead of directly assigning an empty object to the transactions parameter to maintain Redux's principle of pure functions.
- transactions = {} // eslint-disable-line
+ return {}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.
| clearTransactions: (transactions: _TransactionsState) => { | |
| transactions = {} // eslint-disable-line | |
| }, | |
| clearTransactions: (transactions: _TransactionsState) => { | |
| return {} | |
| }, |
| const handleClearTransaction = useCallback(() => { | ||
| dispatch(removeTransaction({ originTxHash })) | ||
| }, [dispatch]) |
There was a problem hiding this comment.
The useCallback hook for handleClearTransaction should include its dependencies to avoid unnecessary re-creations of the callback function.
- const handleClearTransaction = useCallback(() => {
+ const handleClearTransaction = useCallback(() => {
+ }, [originTxHash, dispatch])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.
| const handleClearTransaction = useCallback(() => { | |
| dispatch(removeTransaction({ originTxHash })) | |
| }, [dispatch]) | |
| const handleClearTransaction = useCallback(() => { | |
| dispatch(removeTransaction({ originTxHash })) | |
| }, [originTxHash, dispatch]) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1749 +/- ##
===================================================
- Coverage 52.26669% 51.80723% -0.45946%
===================================================
Files 372 360 -12
Lines 25588 24651 -937
Branches 285 285
===================================================
- Hits 13374 12771 -603
+ Misses 10906 10638 -268
+ Partials 1308 1242 -66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying with
|
| Latest commit: |
1c5af4f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://af737ff9.sanguine.pages.dev |
| Branch Preview URL: | https://fe-activity-pending-sdk.sanguine.pages.dev |
|
Merged into #1695 |
Add MVP
_Transactioncomponent + flow (to be refactored into existing Activity/Transaction flow) that relies on Synapse SDK to track tx status for #1650Summary by CodeRabbit
New Features
Improvements
Refactor
Chores
0feb777de6897f06836eaa5c4c84623cf31c6473: synapse-interface preview link