Retry async calls if they fail #814
Conversation
Fixing circular dep with logrocket and supabase
No longer clearing the cache on tables
Using more custom notification styling for network
Cleaning up
src/api/draftSpecs.ts
Outdated
There was a problem hiding this comment.
The series of the get draft specs by x property queries make me wonder if separating these functions is the best approach. Since the shape of the response is the same, a single function could be defined (e.g., getDraftSpecs) and the query can be built up depending upon the input parameters.
| // Retry calls | ||
| const RETRY_ATTEMPTS = 2; | ||
|
|
||
| export const supabaseRetry = <T>(makeCall: Function, action: string) => { |
There was a problem hiding this comment.
There are a couple instances where an empty string is supplied to the action input parameter. Would it make sense to default this parameter to an empty string instead?
There was a problem hiding this comment.
No - I'd rather make it clear that the calling function is not passing an action.
| import LogRocket from 'logrocket'; | ||
| import { CustomEvents } from './types'; | ||
|
|
||
| export const logRocketEvent = ( |
There was a problem hiding this comment.
Why was this moved from src/services/logrocket.ts?
There was a problem hiding this comment.
It causes a circular dep issue
| @@ -0,0 +1,23 @@ | |||
| export enum CustomEvents { | |||
There was a problem hiding this comment.
Would we like to list the members of this enumeration in alphabetical order, akin to the TABLES enumeration from src/services/supabase.ts?
There was a problem hiding this comment.
I do not care either way
| @@ -141,6 +143,7 @@ const CTAs: ResolvedIntlConfig['messages'] = { | |||
| 'cta.showAll': `Show All`, | |||
| 'cta.reload': `Reload`, | |||
| 'cta.evolve': `Apply`, | |||
| 'cta.support': `contact support`, | |||
There was a problem hiding this comment.
This CTA appears in six locations outside of 'error.instructions'. Were you thinking of replacing them in a separate PR?
A similar thing can be said for the support email, which has five additional occurrences.
There was a problem hiding this comment.
Yeah - gonna switch 'em over later on.
| () => | ||
| supabaseClient | ||
| .rpc(RPCS.CREATE_REFRESH_TOKEN, { | ||
| multi_use, |
There was a problem hiding this comment.
Where do you stand on using snake case for the input parameters of API-related functions?
There was a problem hiding this comment.
I don't really prefer snake case unless it is for something that is "from the back end". So here it kinda makes sense so that we don't have to translate the data.
| // needs translated. However, Our shortest keys (cta.foo) | ||
| // can also look a lot like how Supabase returns | ||
| // errors when fetching wrong keys from a table (tableName.col). | ||
| const displayOnlyError = |
There was a problem hiding this comment.
This is the smallest of nits, but I believe displayErrorOnly is a better phrasing of this variable name, aligning it with flags of a similar demarcation (e.g., readOnly, nameOnly, etc.).
Issues
Fixes #505
Fixes #818
Changes
505
Misc
Tests
Manually tested
Screenshots
Calls failed and then succeed (success logging is no longer there)
A failed to fetch will show an error message stating they should check their network connection
Any other error will just show the error details and no longer show the "instructions" as we do not have much valuable guidance to give a user.
We will still show logs when available
