chore(TransactionFeedV2): Implement vibration for new unknown completed transactions

[sviderock]

Description

12th PR of RET-1207. Implements vibration for new unknown completed transactions.

Test plan

Added test to ensure vibrateSuccess is only called once for the same transaction

Related issues

• Relates to RET-1207

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

• Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.88%. Comparing base (6917e4c) to head (62b9311).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6175      +/-   ##
==========================================
+ Coverage   88.87%   88.88%   +0.01%     
==========================================
  Files         730      730              
  Lines       31024    31036      +12     
  Branches     5386     5697     +311     
==========================================
+ Hits        27573    27587      +14     
+ Misses       3409     3253     -156     
- Partials       42      196     +154     

Files with missing lines	Coverage Δ
src/redux/migrations.ts	97.07% <100.00%> (+<0.01%)	⬆️
src/redux/store.ts	80.00% <ø> (ø)
src/transactions/feed/TransactionFeedV2.tsx	92.44% <100.00%> (+0.13%)	⬆️
src/transactions/selectors.ts	100.00% <100.00%> (ø)
src/transactions/slice.ts	98.68% <100.00%> (+0.09%)	⬆️
test/schemas.ts	91.66% <100.00%> (+0.02%)	⬆️

... and 67 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6917e4c...62b9311. Read the comment docs.

[jophish]

Suggested change

	const hasNewUnknownCompletedTransactions = latestTimestamp
	const hasNewUnknownCompletedTransactions = latestTimestamp
	? payload.transactions.some(
	(tx) => tx.status === TransactionStatus.Complete && tx.timestamp > latestTimestamp
	)
	: false

[jophish]

Why do we need this selector? This looks like it's just directly passing the result from hasNewUnknownCompletedTransactions -- could we just export and use that?

[sviderock]

@jophish Sometimes when I retrieve data with the selector that is not using createSelector - it throws the You must pass a selector to useSelector error on tests. This was also the case this time. It seems to work now though after the following changes:

• removed createSelector hook and got You must pass a selector to useSelector error again
• added createSelector back - error disappeared
• removed createSelector again - now it's all good and no errors 🤷

I really have no idea why is this happening. I've also experienced this exact behaviour when using allStandByTransactions selector a few weeks ago and the issue was resolved in the exact same way. It just took a few more back-and-forth tries with removing/adding back the createSelector piece but if there's something obvious that I'm missing - please, let me know!

[kathaypacific]

looks good! for my understanding, how this works is essentially fetch new transactions -> new transactions are compared with persisted transactions in the reducer -> a flag is set to true if there are new transactions -> vibrate is done by the useEffect -> flag is set to false on the next fetch (assuming there are no new transactions) - is that right?

unless i'm missing something, i think there are 2 paths where this doesn't work:

1. fetch transactions -> there is a new transaction + a vibration -> fetch transaction -> there is another new transaction. this time, the flag was already true, and the useEffect would not be triggered, right?
2. fetch transactions -> there is a new transaction + vibration -> user closes the app. on app open, there will be a vibration because there wasn't a fetch to set the flag to false.

i kind of wonder if we should just do the haptic feedback in the reducer (or somewhere where the action payload and state are accessible, id where this is in rtk query but i'm thinking like the saga layer), it's not so nice but there's no real need to do the vibration on the display layer...to make it work predictably on the display layer, you need the selector and some way to reset the state back to false after the vibration, which i feel like is not worth the complexity?

i also wonder if this type of state (which seems transient) should be persisted between app sessions at all...

[kathaypacific]

Suggested change

	hasNewUnknownCompletedTransactions: boolean
	hasNewCompletedTransactions: boolean

maybe we don't need the "unknown" part? i was confused about whether "unknown" is a type of classification

[sviderock]

@kathaypacific it's purely for readability purposes as there's already an existing hasNewlyCompletedTransactions which is for the transactions that have just turned from pending to completed, therefore "newly completed".
So we'll have both:

hasNewCompletedTransactions
hasNewlyCompletedTransactions

Considering that these names are almost identical, I thought that it might introduce a bit of a confusion as it's gonna be really easy to mistype or let IDE autocomplete the wrong one. And in order to not introduce a few extra lines of comments, I figured it might be enough of a distinction to have:

• hasNewlyCompletedTransactions - for transaction that have just turned from pending to completed
• hasNewUnknownCompletedTransactions - for transaction that are new and completed, but unknown as we did not know about them at all (they were not in our pending/failed/completed list previously)

[kathaypacific]

oh sorry i didn't realise we had something like hasNewlyCompletedTransactions! any chance we could just do the vibration when that flag is set, maybe in this hook? as an aside, i haven't tested and it might not be a problem, but i have found in the past that having non primitive state variables results in unnecessary rerenders unless we do some special identity management (i haven't thought about this deeply so i don't have suggestions, but it did occur to me that the vibrations might trigger more than expected due to this if we were to put them here....but it's easy enough to have a different useEffect with carefully chosen dependency array variables to overcome this)

[sviderock]

@kathaypacific I don't think it would be suitable in that particular hook as it's only responsibility is to update the state and side-effects shouldn't be called from within state setters. But there is a way to do it with a separate hook, that will use useEffects to track when the vibration for a specific set of transactions occurred.

I'll try to figure out a different, more reliable solution!

[kathaypacific]

okay sure, sounds good. i do wonder how we should think about haptic feedback though, i can see why you say it is a side effect, but at the same time it is a synchronous action that doesn't trigger any meaningful effects as such...

[sviderock]

@kathaypacific It is indeed a synchronous action, but from the perspective of redux state management and our implementation of it - it has to be reactionary to the changes to the data in redux, therefore async (at least, that's how I see it, maybe I'm wrong on this?).
Unless, there's a way for me to catch a specific action is saga, before the data gets synced up with the persisted feed (as described in the comment below).

[sviderock]

@kathaypacific Yes, the logic is correct! Other than the existing logic for storing stand by transactions (pending/failed) we only persist the first page of the feed. So whenever there's a new update for the first page - we check if there were any new completed transactions between current and previous feed updates (as per this discussion).

Initially, implementation was on the component level but after moving some redux-related side-effects to the reducer (PR) - persisted first page of the feed and polling data are now always in sync with each other on the reducer level, before getting to the component via selectors. So currently, there's no way to detect the change on the component level.

You're completely right that the logic will fail in both scenarios. I'll come up with a few new tests to cover these specifically and fix the logic accordingly.
But I'm not so sure about moving the haptic feedback to the reducer as it would be creating an unwanted side-effect on the redux side. Maybe there's a way to do it with saga but I'll need to check if there's a way to catch the changes to the RTK-Query reducer before it gets to this slice and sync the data. Otherwise, there will be a need to keep the logic on the component level, just having it patched-up to actually consider the scenarios you've described.

[sviderock]

I'm gonna close this for now as the specific implementation for triggering haptic feedback needs to be revisited.