-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(TransactionFeedV2): Trigger haptic feedback when pending transactions turn to completed #6154
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## slava/feed-v2-no-more-transactions-toast #6154 +/- ##
============================================================================
+ Coverage 88.77% 88.82% +0.05%
============================================================================
Files 727 727
Lines 30791 30807 +16
Branches 5636 5328 -308
============================================================================
+ Hits 27334 27365 +31
+ Misses 3414 3399 -15
Partials 43 43
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
src/redux/migrations.ts
Outdated
|
||
const flattenedTransactions = Object.values( | ||
state.transactions.transactionsByNetworkId | ||
).flat() as TokenTransaction[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As transactionsByNetworkId
were grouped by networkId
- it should be safe to assume that Zerion can also return the same hash for two different networks, considering that it should return identical data to what we were getting with the previous data providers. I think I need to change the whole implementation of knownCompletedTransactionsHashes
to maybe store keys like ${networkId}_${transactionHash}
or an object just like transactionsByNetworkId
but wanted to post this here to confirm. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we should store just the first page with all networks combined.
But because we need to keep the old feed, this should be a new field in the reducer.
useEffect( | ||
function updateKnownCompletedTransactions() { | ||
if (data?.transactions.length) { | ||
dispatch(updateKnownCompletedTransactionsHashes(data.transactions)) | ||
} | ||
}, | ||
[data?.transactions] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we could skip this effect by reacting to the underlying RTK-Query API fetched action, within the transactions reducer, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeanregisser Will investigate and get back to you on this!
src/redux/migrations.ts
Outdated
|
||
const flattenedTransactions = Object.values( | ||
state.transactions.transactionsByNetworkId | ||
).flat() as TokenTransaction[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we should store just the first page with all networks combined.
But because we need to keep the old feed, this should be a new field in the reducer.
src/transactions/reducer.ts
Outdated
@@ -22,11 +22,13 @@ interface State { | |||
// feed instantly. | |||
standbyTransactions: StandbyTransaction[] | |||
transactionsByNetworkId: TransactionsByNetworkId | |||
knownCompletedTransactionsHashes: string[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use a new transactions
field to store the first page (or last X newest items) from the new API?
Then we could avoid the need for knownCompletedTransactionsHashes
and use selectors to derive what we need from it.
Then if we see a new transaction that's newer than the oldest item in the newest list of items, we can vibrate.
This would address this new field proposed here being an ever growing list of TX hashes. And would serve the purpose of showing cached data directly on app load.
Would this all work?
b6bb048
to
e2cf8ff
Compare
@jeanregisser The easiest way to remove the unnecessary redux changes in multiple files was just resetting the commits related to redux changes and force-pushing. I've updated this PR to only implement haptic for newly completed transactions. I'll add haptic for new unknown completed transactions as a follow-up PR to this PR as it implements |
Description
8th PR for RET-1207. Implements triggering of the haptic feedback when transactions turn from pending to completed.
Test plan
Added tests to validate the logic of triggering the haptic feedback
Related issues
Backwards compatibility
Yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: