chore(TransactionFeedV2): Add error handling to Transaction Feed V2 #6151

sviderock · 2024-10-14T09:34:52Z

Description

5th PR for RET-1207. Adds error handling middleware as per the docs and also copies the same logic of error handling from queryHelper.

Test plan

Eveything works as before.

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 · 2024-10-14T09:41:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.77%. Comparing base (76f1605) to head (601dbb1).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6151    +/-   ##
========================================
  Coverage   88.76%   88.77%            
========================================
  Files         727      727            
  Lines       30776    30786    +10     
  Branches     5627     5631     +4     
========================================
+ Hits        27319    27330    +11     
- Misses       3259     3414   +155     
+ Partials      198       42   -156

Files with missing lines	Coverage Δ
src/redux/apiReducersList.ts	`100.00% <100.00%> (ø)`
src/transactions/feed/TransactionFeedV2.tsx	`90.00% <100.00%> (+0.63%)`	⬆️

... and 68 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 76f1605...601dbb1. Read the comment docs.

jeanregisser · 2024-10-14T09:51:42Z

src/redux/apiReducersList.ts

@@ -7,3 +7,5 @@ export const apiReducersList = {
 } as const

 export const apiMiddlewares = [transactionFeedV2Api.middleware]
+
+export const apiReducersKeys = Object.keys(apiReducersList)


nit: Do we need this in more than one location? I couldn't quite see it from this PR.

@jeanregisser Not really, just thought it might be a little more convenient to have api-related stuff exported from this file but I am totally fine with both approaches. I'll revert this one.

We can keep it I guess, I was just wondering why it needed to change now in this PR.

@jeanregisser initially I was also using apiReducersKeys in the error logger to provide more precise info on what specific endpoint caused the error, but as I've figured that I can just pass the whole arg - I removed the usage but kept apiReducersKeys at the new place.

jeanregisser · 2024-10-14T10:04:46Z

src/redux/api.ts

+export const rtkQueryErrorLoggerMiddleware: Middleware =
+  (api: MiddlewareAPI) => (next) => (action) => {
+    if (isRejectedWithValue(action)) {
+      const errorMessage =
+        'data' in action.error
+          ? (action.error.data as { message: string }).message
+          : action.error.message
+
+      Logger.error('RTK-Query', errorMessage || 'An error occured', action)
+      api.dispatch(showError(ErrorMessages.FETCH_FAILED) as Action)
+    }
+
+    return next(action)
+  }


Ah, we don't really want this generic error message anymore at this level.
Could we move it to be at the component level instead?

We now usually prefer showing inline errors, instead of the red banner at the top.
To avoid showing them from background tasks we have in the app, which could show up at inappropriate times (e.g. the user navigated to another screen, so the red banner is not directly relevant for something that's in the TX feed).

FETCH_FAILED is really just used for the TX feed. And we should probably rename it.
I know moving it to the component won't change the fact that it could still trigger at inappropriate times.
But at least we can update the behavior later from the component.

@jeanregisser Gotcha, I will adjust this to the new desirable approach.
It seems like queryHelper only uses red banner for error. Could you please point me at the example with the inline error?

What I meant is to control the error behavior from the component.
i.e. dispatch from the component instead of a generic error hander.
We can't yet switch to an inline error as we first need approved designs for that.

Ah, got it. I'll move it to the component.

sviderock · 2024-10-14T17:02:06Z

@jeanregisser updated

jophish · 2024-10-15T14:13:36Z

src/transactions/feed/TransactionFeedV2.tsx

@@ -39,6 +42,7 @@ type PaginatedData = {
 // Query poll interval
 const POLL_INTERVAL_MS = 10000 // 10 sec
 const FIRST_PAGE_TIMESTAMP = 0
+const LOGGER_TAG = 'transactions/feed/TransactionFeedV2'


nit: can just call this TAG, since it's only ever used for logging purposes anyways (and matches the naming convention we typically use)

@jophish sure! I'll address this in the follow-up PR.

nit: what's the problem with doing it here, since it's this PR that introduces it?

@jeanregisser no problem with doing it here. I thought this should be a small enough change that it can be addressed within the next PR that I was already working on so there's less potential back-and-forth with rebasing all the follow-up PRs. But you're right, it makes sense to make the change here if this change is purely related to this PRs goal.

@jeanregisser updated

jeanregisser

🚀

jeanregisser · 2024-10-16T08:43:25Z

src/transactions/feed/TransactionFeedV2.tsx

@@ -39,6 +42,7 @@ type PaginatedData = {
 // Query poll interval
 const POLL_INTERVAL_MS = 10000 // 10 sec
 const FIRST_PAGE_TIMESTAMP = 0
+const LOGGER_TAG = 'transactions/feed/TransactionFeedV2'


nit: what's the problem with doing it here, since it's this PR that introduces it?

…6151) ### Description 5th PR for RET-1207. Adds error handling middleware as per [the docs](https://redux-toolkit.js.org/rtk-query/usage/error-handling) and also copies the same logic of error handling from [queryHelper](https://github.com/valora-inc/wallet/blob/main/src/transactions/feed/queryHelper.ts). ### Test plan Eveything works as before. ### 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: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

add error handling for RTK-Query endpoints

6bbe2cd

jeanregisser reviewed Oct 14, 2024

View reviewed changes

move error handling to component

14dbf73

sviderock changed the title ~~chore(feed): Add error handling to Transaction Feed V2~~ chore(TransactionFeedV2): Add error handling to Transaction Feed V2 Oct 15, 2024

jophish approved these changes Oct 15, 2024

View reviewed changes

jeanregisser approved these changes Oct 16, 2024

View reviewed changes

sviderock added 2 commits October 16, 2024 14:07

rename LOGGER_TAG to TAG

45c5c66

Merge branch 'main' into slava/feed-v2-error-handling

601dbb1

sviderock added this pull request to the merge queue Oct 16, 2024

Merged via the queue into main with commit 6a8884a Oct 16, 2024
15 checks passed

sviderock deleted the slava/feed-v2-error-handling branch October 16, 2024 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TransactionFeedV2): Add error handling to Transaction Feed V2 #6151

chore(TransactionFeedV2): Add error handling to Transaction Feed V2 #6151

sviderock commented Oct 14, 2024

codecov bot commented Oct 14, 2024 •

edited

Loading

jeanregisser Oct 14, 2024

sviderock Oct 14, 2024

jeanregisser Oct 14, 2024 •

edited

Loading

sviderock Oct 14, 2024 •

edited

Loading

jeanregisser Oct 14, 2024 •

edited

Loading

sviderock Oct 14, 2024

jeanregisser Oct 14, 2024 •

edited

Loading

sviderock Oct 14, 2024

sviderock commented Oct 14, 2024

jophish Oct 15, 2024

sviderock Oct 15, 2024

jeanregisser Oct 16, 2024

sviderock Oct 16, 2024

sviderock Oct 16, 2024

jeanregisser left a comment

jeanregisser Oct 16, 2024

chore(TransactionFeedV2): Add error handling to Transaction Feed V2 #6151

chore(TransactionFeedV2): Add error handling to Transaction Feed V2 #6151

Conversation

sviderock commented Oct 14, 2024

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

sviderock Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

jeanregisser Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2024 •

edited

Loading

jeanregisser Oct 14, 2024 •

edited

Loading

sviderock Oct 14, 2024 •

edited

Loading

jeanregisser Oct 14, 2024 •

edited

Loading

jeanregisser Oct 14, 2024 •

edited

Loading