Claim bug fixes #2362
Claim bug fixes #2362
Conversation
nenadV91
requested review from
alfetopito,
anxolin and
elena-zh
and removed request for
anxolin
|
| @@ -279,6 +279,17 @@ export function useUserClaims(account: Account, optionalChainId?: SupportedChain | |||
| return { claims: claimKey ? claimInfo[claimKey] : null, isLoading } | |||
| } | |||
|
|
|||
| let FETCH_DEPLOYMENT_TIMESTAMP_PROMISE: Promise<number> | null = null | |||
There was a problem hiding this comment.
why CAPITAL letters if its not a constant?
There was a problem hiding this comment.
The same thing they used here
cowswap/src/state/claim/hooks.ts
Line 28 in 1c086d4
There was a problem hiding this comment.
well, it sounds like also bad there :)
anyways, just a nit. Constants are capitalised, but this is not one
| function fetchDeploymentTimestamp(vCowContract: VCowType) { | ||
| return ( | ||
| FETCH_DEPLOYMENT_TIMESTAMP_PROMISE ?? | ||
| (FETCH_DEPLOYMENT_TIMESTAMP_PROMISE = vCowContract.deploymentTimestamp().then((ts) => { |
There was a problem hiding this comment.
I would keep then here.
There was a problem hiding this comment.
Not against it, but any reason in particular?
it seems more readable to use await
|
Hey @nenadV91 , I see an infinite loader when I open the Claim page as a not connected user: |
|
@elena-zh Good catch, should be fixed now. |
|
@nenadV91 , works fine. |
| @@ -288,17 +304,28 @@ function useDeploymentTimestamp(): number | null { | |||
| const { chainId } = useActiveWeb3React() | |||
| const vCowContract = useVCowContract() | |||
| const [timestamp, setTimestamp] = useState<number | null>(null) | |||
| const [isMounted, setIsMounted] = useState(true) | |||
There was a problem hiding this comment.
I think you should use a useRef instead.
Otherwise the isMounted flag will cause the issue you are trying to prevent here.
There was a problem hiding this comment.
Good suggestion, looks like its working great.
| console.log('vCowContract Deployment Timestamp fetch failed') | ||
| } | ||
| }) | ||
| }, [chainId, isMounted, timestamp, vCowContract]) |
There was a problem hiding this comment.
It's still depending on timestamp from outer context which it shouldn't
| @@ -279,6 +279,17 @@ export function useUserClaims(account: Account, optionalChainId?: SupportedChain | |||
| return { claims: claimKey ? claimInfo[claimKey] : null, isLoading } | |||
| } | |||
|
|
|||
| let FETCH_DEPLOYMENT_TIMESTAMP_PROMISE: Promise<number> | null = null | |||
There was a problem hiding this comment.
well, it sounds like also bad there :)
anyways, just a nit. Constants are capitalised, but this is not one
nenadV91
force-pushed
the
claim-bug-fixes
branch
2 times, most recently
from
ca1caa2 to
abd4025
Compare
nenadV91
force-pushed
the
claim-bug-fixes
branch
from
abd4025 to
5272389
Compare
| * Creates a ref that can be used to solve the issue of | ||
| * "Can't perform a React state update on an unmounted component." | ||
| */ | ||
| function useIsMounted() { |
There was a problem hiding this comment.
why not do this in a more general place and export it. Its a general solution, right?
Summary
This fixes few existing errors in console
issue with key props
issue with calling fetch deployment timestamp multiple times and also memory leak issues
issue with flashing screen before loader shows up
To test
Edited anxo:
Just some test. Pls nenad ignore
LINEAR-13