fix(homefeed): ensure that users can scroll to load more #2779

kathaypacific · 2022-08-11T14:04:56Z

Description

Context: https://valora-app.slack.com/archives/C018VLA3YAK/p1660091022594799

The solution chosen for this issue is to continue to fetch transactions from the client until a minimum threshold is met (currently 10) so that the section list is filled and the user can scroll to load more.

In this PR:

To ensure no weird bugs and avoid concurrency issues, i refactored the state variables to keep the cumulative transactions + pageInfo in the same state variable so that they are always in sync.
Added a hook to refetch transactions until we have 10, and refetch transactions if the returned page is empty
Updated tests

Other changes

N/A

Tested

Manually and with unit tests

How others should test

Please try this with many accounts that have varying number of transactions - load the app (from a fresh install, and from cold start) and ensure that the home feed displays transactions. If the account has many transactions, ensure that you can scroll down to load more transactions.

Related issues

N/A

Backwards compatibility

Yes

kathaypacific · 2022-08-11T14:06:42Z

src/transactions/feed/queryHelper.ts

-      if (!paginatedResult) {
+      setFetchedResult((prev) => ({
+        transactions: deduplicateTransactions(prev.transactions, returnedTransactions),
+        pageInfo: returnedPageInfo ?? null,


@dievazqu this is one part of the logic that i changed (compared to line 94 from the previous version), can you think of any places where this condition is not okay? for the few cases i tested, this works okay

This seems ok!

codecov · 2022-08-11T14:23:04Z

Codecov Report

Merging #2779 (5f42507) into main (1922a6c) will increase coverage by 0.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main    #2779      +/-   ##
==========================================
+ Coverage   79.74%   79.75%   +0.01%     
==========================================
  Files         600      600              
  Lines       21729    21739      +10     
  Branches     3948     3958      +10     
==========================================
+ Hits        17327    17338      +11     
+ Misses       4355     4353       -2     
- Partials       47       48       +1

Impacted Files	Coverage Δ
src/transactions/feed/queryHelper.ts	`91.76% <95.00%> (+0.85%)`	⬆️
src/fiatconnect/ReviewScreen.tsx	`80.50% <100.00%> (+0.33%)`	⬆️
src/tokens/utils.ts	`100.00% <0.00%> (+2.94%)`	⬆️

Continue to review full report at Codecov.

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

dievazqu · 2022-08-11T14:36:58Z

src/transactions/feed/queryHelper.ts


    const returnedTransactions = result.data?.tokenTransactionsV2?.transactions
+    const returnedPageInfo = result.data?.tokenTransactionsV2?.pageInfo

    if (returnedTransactions?.length) {


I think we have to change this...
Because if the returned transactions are empty, but it's not the last page (meaning that hasNextPage is true, but the blockchain-api has filtered out all the unknown transactions), we still have to update fetchedResult.pageInfo so when we query again we get a different page.

oh excellent point, i can add a check for pageInfo here

fixed and added a test

dievazqu

LGTM!

Samuel3Shin · 2022-08-11T17:43:13Z

I tested it with my android emulator (Pixel 2 API 29 / 1080 x 1920: 420dpi / Android 10.0 / x86).
I checked that this branch fixed the not loading transaction feed problem in my android emulator for the two addresses 0xf53a7a9a5de7ce39ef259c86fd6128ed045bf01a, 0x849fa48c8caa2907032f45765d7d6f52da13598d.

src/transactions/feed/queryHelper.ts

This reverts commit 9294c9d.

kathaypacific · 2022-08-12T10:00:35Z

src/transactions/feed/queryHelper.ts

        dispatch(showError(ErrorMessages.FETCH_FAILED))
-      } else if (!pageInfo.hasNextPage) {
-        // If the user has a few transactions, don't show any message
-        if (transactions.length > 20) {


i thought this was an odd requirement so i removed it :/

### Description Context: https://valora-app.slack.com/archives/C018VLA3YAK/p1660091022594799 The solution chosen for this issue is to continue to fetch transactions from the client until a minimum threshold is met (currently 10) so that the section list is filled and the user can scroll to load more. In this PR: - To ensure no weird bugs and avoid concurrency issues, i refactored the state variables to keep the cumulative transactions + pageInfo in the same state variable so that they are always in sync. - Added a hook to refetch transactions until we have 10, and refetch transactions if the returned page is empty - Updated tests ### Other changes N/A ### Tested Manually and with unit tests ### How others should test Please try this with many accounts that have varying number of transactions - load the app (from a fresh install, and from cold start) and ensure that the home feed displays transactions. If the account has many transactions, ensure that you can scroll down to load more transactions. ### Related issues N/A ### Backwards compatibility Yes

…w transactions (#2783) ### Description Annoyingly, the `onEndReached` callback is triggered on the SectionList used to render the transaction feed when navigating away from the screen on iOS, when the transaction list is too short. This causes the "no more transactions" toast to appear on the next page that is navigated to, but only once. This check for length of transactions used to exist, and I had removed it thinking it was strange #2779 (comment) - but it seems like we need it. ### Other changes N/A ### Tested Manually ### How others should test N/A ### Related issues N/A ### Backwards compatibility Yes

ValoraQA · 2022-08-16T09:11:57Z

Hey @MuckT, We have verified the above task using Latest Android Internal Release Build, iOS Test Flight Release Build V1.38.2 & observed the following:

Latest Transactions in Recent activity section appears fine when user launches app after fresh install, cold launch, kill & relaunch.
If the account had many transactions, than user can scroll down to load more transactions in recent activity section when user freshly install, cold launch, kill & relaunch the app.

Devices: Google Pixel XL (10.0), iPhone 13 mini(15.1.1), Google Pixel 4a(12.0)

CC: @kathaypacific
Thanks..!!

ValoraQA · 2022-08-23T11:01:25Z

Hey @MuckT we have verified the above task on Latest iOS Test Flight Release build & Android Internal Release Build V 1.39.1 & updated status in Test rails

Observation:

Latest Transactions in Recent activity section appears fine when user launches app after fresh install, cold launch, kill & relaunch.
If the account has many transactions, than user can scroll down to load more transactions when user performs freshly install, cold launch, kill & relaunching the app.

Test rail link:
iOS 14: https://valoraapp.testrail.io/index.php?/tests/view/61319
Android 12: https://valoraapp.testrail.io/index.php?/tests/view/61304
Android 11: https://valoraapp.testrail.io/index.php?/tests/view/61289

Device : iPhone 12(14.7.1), Google Pixel 4a(12.0), Samsung Galaxy F41(11.0)
CC: @kathaypacific
Thanks.!!

ValoraQA · 2022-08-24T11:33:05Z

Hey @MuckT we have verified the above task on Latest iOS Test Flight Release build V 1.39.1 & updated status in Test rails

Observation:

Latest Transactions in Recent activity section appears fine when user launches app after fresh install, cold launch, kill & relaunch.
If the account has many transactions, than user can scroll down to load more transactions when user performs freshly install, cold launch, kill & relaunching the app.

Test rail link:
iOS 15: https://valoraapp.testrail.io/index.php?/tests/view/61845

Device : iPhone 13 mini(15.1.1)

CC: @kathaypacific

Thanks.!!

kathaypacific added 2 commits August 11, 2022 15:27

feat: fetch transactions until there are 10

81cb75c

fix: state management issues

2cd9f93

kathaypacific commented Aug 11, 2022

View reviewed changes

dievazqu reviewed Aug 11, 2022

View reviewed changes

fix: handle zero transactions

9e5bfe8

dievazqu approved these changes Aug 11, 2022

View reviewed changes

MuckT added 2 commits August 11, 2022 18:04

Merge branch 'main' into kathy/fix-homefeed-load

87a857f

Merge branch 'main' into kathy/fix-homefeed-load

3ac0d7b

MuckT reviewed Aug 12, 2022

View reviewed changes

src/transactions/feed/queryHelper.ts Outdated Show resolved Hide resolved

MuckT and others added 4 commits August 11, 2022 20:34

fix: fetching new older transactions after polling

9294c9d

Revert "fix: fetching new older transactions after polling"

0c5e916

This reverts commit 9294c9d.

feat: fetch next page automatically

0daab61

refactor: revert some changes

3cd5db3

kathaypacific commented Aug 12, 2022

View reviewed changes

kathaypacific added 2 commits August 12, 2022 13:16

fix: avoid state issues for polling and fetching next pages

6fb61bf

refactor: improve naming

5f42507

kathaypacific added the automerge Have PR merge automatically when checks pass label Aug 12, 2022

mergify bot merged commit 88fd4a5 into main Aug 12, 2022

mergify bot deleted the kathy/fix-homefeed-load branch August 12, 2022 13:06

kathaypacific mentioned this pull request Aug 12, 2022

fix(homefeed): avoid showing 'no more transactions' toast when too few transactions #2783

Merged

jeanregisser mentioned this pull request Oct 9, 2024

feat(TransactionFeedV2): Add initial implementation of Transaction Feed V2 #6135

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(homefeed): ensure that users can scroll to load more #2779

fix(homefeed): ensure that users can scroll to load more #2779

kathaypacific commented Aug 11, 2022 •

edited

Loading

kathaypacific Aug 11, 2022

dievazqu Aug 11, 2022

codecov bot commented Aug 11, 2022 •

edited

Loading

dievazqu Aug 11, 2022

kathaypacific Aug 11, 2022

kathaypacific Aug 11, 2022

dievazqu left a comment

Samuel3Shin commented Aug 11, 2022

kathaypacific Aug 12, 2022

ValoraQA commented Aug 16, 2022 •

edited

Loading

ValoraQA commented Aug 23, 2022 •

edited

Loading

ValoraQA commented Aug 24, 2022

fix(homefeed): ensure that users can scroll to load more #2779

fix(homefeed): ensure that users can scroll to load more #2779

Conversation

kathaypacific commented Aug 11, 2022 • edited Loading

Description

Other changes

Tested

How others should test

Related issues

Backwards compatibility

kathaypacific Aug 11, 2022

Choose a reason for hiding this comment

dievazqu Aug 11, 2022

Choose a reason for hiding this comment

codecov bot commented Aug 11, 2022 • edited Loading

Codecov Report

dievazqu Aug 11, 2022

Choose a reason for hiding this comment

kathaypacific Aug 11, 2022

Choose a reason for hiding this comment

kathaypacific Aug 11, 2022

Choose a reason for hiding this comment

dievazqu left a comment

Choose a reason for hiding this comment

Samuel3Shin commented Aug 11, 2022

kathaypacific Aug 12, 2022

Choose a reason for hiding this comment

ValoraQA commented Aug 16, 2022 • edited Loading

ValoraQA commented Aug 23, 2022 • edited Loading

ValoraQA commented Aug 24, 2022

kathaypacific commented Aug 11, 2022 •

edited

Loading

codecov bot commented Aug 11, 2022 •

edited

Loading

ValoraQA commented Aug 16, 2022 •

edited

Loading

ValoraQA commented Aug 23, 2022 •

edited

Loading