fix(TransactionFeedV2): Fix removals and "jumps" of pending transactions #6206
Conversation
| const transactionsFromAllowedNetworks = deduplicatedTransactions.filter((tx) => | ||
| allowedNetworks.includes(tx.networkId) | ||
| ) |
There was a problem hiding this comment.
Moved this to be applied at the end of the processing flow, within sections const. In this case, we can ensure that if somehow some async changes from mutlichain arrive and change showTransfers - it will instantly be reflected with recalculation of sections
sviderock
changed the base branch from
main
to
slava/feed-v2-fix-persisted-feed
| @@ -22,6 +23,24 @@ const standbyTransactionsSelector = createSelector( | |||
| } | |||
| ) | |||
|
|
|||
| export const formattedStandByTransactionsSelector = createSelector( | |||
There was a problem hiding this comment.
I didn't really know how to call this, considering there are already selectors like:
standByTransactionsSelectorallStandByTransactionsSelectorpendingStandByTransactionsSelectorconfirmedStandByTransactionsSelector
The last three are un-memoized and trigger a lot of re-renders so cannot use them.
There was a problem hiding this comment.
what about adding this logic to standByTransactionsSelector? it seems harmless to ensure the correct types are returned?
There was a problem hiding this comment.
@kathaypacific standByTransactionsSelector includes getSupportedNetworkIdsForApprovalTxsInHomefeed which triggers a lot of unnecessary re-renders due to it being unmemoized. This is the reason I didn't use it in the first place and initially used allStandbyTransactionsSelector directly.
There was a problem hiding this comment.
unless it causes noticeable performance degradations, my personal preference would be to use existing functions rather than introduce extra complexity (and potential readability issues). we can go back and improve the memoization for the getSupportedNetworkIds* selectors later and fix the issue
There was a problem hiding this comment.
@kathaypacific my expectation was that once we remove the old feed - we'll adjust all the naming accordingly so there won't be an issue with multiple selectors having similar names and doing similar things.
📸 Snapshot TestNo snapshots generated
🛸 Powered by Emerge Tools |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6206 +/- ##
==========================================
- Coverage 88.93% 88.93% -0.01%
==========================================
Files 737 737
Lines 31378 31379 +1
Branches 5801 5804 +3
==========================================
- Hits 27907 27906 -1
- Misses 3273 3275 +2
Partials 198 198
Continue to review full report in Codecov by Sentry.
|
| @@ -268,7 +239,8 @@ function useNewlyCompletedTransactions( | |||
| useEffect( | |||
| function updatePrevStandBy() { | |||
| setPreviousStandBy((prev) => { | |||
| const confirmedHashes = standByTransactions.confirmed.map((tx) => tx.transactionHash) | |||
| const { pending, confirmed } = categorizeTransactions(standByTransactions) | |||
| const confirmedHashes = confirmed.map((tx) => tx.transactionHash) | |||
There was a problem hiding this comment.
nit: do we need to loop through to get transaction hashes, and then loop through again to use the .includes below? could we instead do confirmed.some(({transactionHash}) => transactionHash === tx.transactionHash) to get newlyCompleted?
There was a problem hiding this comment.
@kathaypacific you're right about replacing .filter with .some! For some reason I interchangeably use both when I do these checks and I don't really know why I always pick one or the other 😄
I'll still need to .map for hashes whether for confirmed or prev.prending cause otherwise I'll need to do:
confirmed.some(({transactionHash}) => {
const pendingTx = prev.prending.find((tx) => tx.transactionHash);
return pendingTx && transactionHash === pendingTx?.transactionHash
}) There was a problem hiding this comment.
@kathaypacific Oh, I've remember why I made it with .filter.
newlyCompleted is also used to calculate newlyCompletedCrossChainSwaps
| @@ -22,6 +23,24 @@ const standbyTransactionsSelector = createSelector( | |||
| } | |||
| ) | |||
|
|
|||
| export const formattedStandByTransactionsSelector = createSelector( | |||
There was a problem hiding this comment.
what about adding this logic to standByTransactionsSelector? it seems harmless to ensure the correct types are returned?
Description
This PR fixes an issue when some pending transactions can be removed from the feed for a short time and re-added back again. This was happening because pending and confirmed stand by transactions were split into 2 states which were independently affecting different parts of the final
sectionsstate.Example:
pendingtransaction appears with a spinning indicator after a Send actionstandByTransactions.pendingandstandByTransactions.confirmedThis causes the following update chains:
standByTransactions.pending->sectionsstandByTransactions.confirmed->updatePaginatedData->paginatedData->confirmedTransactions->sectionsSo in result
sectionsis updated twice with visual feedback in a form of removing and re-adding back transactions.Test plan
All existing tests pass.
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-11-01.at.17.00.26.mp4
Related issues
Backwards compatibility
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: