Jon/fix/markets-fiat-change #5512

Jon-edge · 2025-03-22T01:52:32Z

Race conditions
Multiple fetches for the same data
Stale periodic task timers retaining the old fiat
Fetch indexing bugs after changing fiat
Failure to re-render due to global data cache
handleOnEndReached handling after changing fiat

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

Yes
No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

Tested on iOS device
Tested on Android device
Tested on small-screen device (iPod Touch)
Tested on large-screen device (tablet)

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1209331858493618

samholmes

Last commit should be broken into multiple commits for each change in the itemized list in the commit body.

I feel like the code is decaying because we're dealing with unconventional use of React's API. Instead we could consider improving the implementation to fix the issues without the increasing overhead of tech-debt. Though, it's not clear what that may look like until the changes are cleared up by breaking up the commit.

src/components/cards/MarketsCard.tsx

samholmes

I have a strong feeling that this approach is a shotgun approach to solving the issue. The amount of time spent with the shotgun may have been equal or less time if spent with a handgun (refract the imperative mess) or a sniper riffle (surgical change to get it to work). The reason why I say this is because it seems like using a ref for the query function is so that way it prevents it from being a closure, but then it wants a closure on some previous state values. I feel like the right surgical change here is to pick one approach: is it a closure or is it not a closure?

From what I can tell, it has to be a closure in order to abort it, so that's one reason to keep it as a closure. That would greatly simplify things (no more useHandler), no more forceRefresh. If it's no longer a closure (e.g. it's a handler from useHandler), then I'm not sure how to abort it, so it's effects should be noop'd at the end of it's routine (not ideal). So I say keep it a closure and see what you can do with that constraint. I think we'll quickly see this PR line changes go down significantly and the change as a whole be a lot more straight forward.

samholmes · 2025-03-31T21:39:02Z

src/components/scenes/CoinRankingScene.tsx

This seems like an optional change. Potentially drop this commit to reduce adding to the tech debt that is in this component. (We should clean this component up later by removing the dangling state).

samholmes · 2025-03-31T21:42:06Z

src/components/scenes/CoinRankingScene.tsx

+    debugLog(LOG_COINRANK, `handleEndReached. setRequestDataSize ${dataSize + QUERY_PAGE_SIZE}`)
+    setRequestDataSize(dataSize + QUERY_PAGE_SIZE)


Might not be correct. Give valid reason for why we're deriving requestDataSize from dataSize

samholmes · 2025-03-31T21:48:16Z

src/components/scenes/CoinRankingScene.tsx

-    // the first index (initial load/timed refresh)
-    const queryLoop = async (startIndex: number) => {
+    // Define the query function that will be stored in the ref
+    queryLoopRef.current = async (startIndex: number) => {


useHandler is good for this.

samholmes · 2025-03-31T22:06:50Z

src/components/scenes/CoinRankingScene.tsx

+  // Effect to handle changes in requestDataSize and coingeckoFiat
+  React.useEffect(() => {
+    // Always clear any existing timeout when making changes to the query
+    if (timeoutHandler.current) {
+      clearTimeout(timeoutHandler.current)
+      timeoutHandler.current = undefined
+    }
+
+    if (lastFetchedFiat.current !== coingeckoFiat) {
+      // Reset everything when the fiat setting does not match what cached
+      // data we fetched previously.
+      debugLog(LOG_COINRANK, `Fiat changed from ${lastFetchedFiat.current} to ${coingeckoFiat}`)
+      isQuerying.current = false
+      coinRanking.coinRankingDatas = []
+      lastStartIndex.current = 1
+      setDataSize(0)
+      setRequestDataSize(QUERY_PAGE_SIZE)
+
+      // Force a re-render immediately to ensure no stale data is shown while we wait for fetch
+      setForceRefresh(prev => prev + 1)
+
+      lastFetchedFiat.current = coingeckoFiat
+    }
+
+    // Trigger a query with the current parameters
+    if (queryLoopRef.current) {
+      queryLoopRef.current(lastStartIndex.current).catch(e => console.error(`Error in query loop: ${e.message}`))
+    }


After lengthy discussion with jon, I understand why this is needed, and it's a side-effect of how the component is implemented. Later, we should clean this up.

samholmes · 2025-04-01T16:34:34Z

src/components/scenes/CoinRankingScene.tsx

-      lastStartIndex.current = startIndex
+      // Do nothing if fiat was changed while the fetch was happening. There is
+      // another call to queryLoop() handling this.
+      if (lastFetchedFiat.current !== currentFiat && !isHandlingFiatChange) return


You can abort routines with this pattern:

useEffect(() => { let abort = false const cb = () => { await something() if (abort) return await somethingElse() if (abort) return setState(...) } cb().catch(...) return () => abort = true })

Now you can make cb have closure over whatever state was available during it's declaration. If a useEffect(callback)'s callback is called, it'll abort the previous subscription and instantiate a new subscription.

Remember: useEffect should have been named useSubscription (see David Khourshid's excellent presentation on this).

samholmes · 2025-04-01T16:41:22Z

src/components/scenes/CoinRankingScene.tsx

+          if (!isHandlingFiatChange && lastFetchedFiat.current !== currentFiat) {
+            debugLog(LOG_COINRANK, `Stopping stale query loop for ${lastFetchedFiat.current} due to fiat change`)
+            dataUpdated = false
+            break
+          }


if (abort) return

samholmes · 2025-04-01T16:46:17Z

src/components/scenes/CoinRankingScene.tsx

+      if (isHandlingFiatChange) {
+        setForceRefresh(prev => prev + 1)
+      }


Whenever you see this pattern of a counter to force a refresh, you can instead ask "What condition forces the refresh?" "Can that condition be used as a dependency?" If yes, then use the dependency for the refresh:

useEffect(callback, [coingeckoFiat])

There, now whenever coingeckoFiat changes (which is you're condition), we'll invoke callback. Now we have a refresh without extra state forceRefresh.

samholmes · 2025-04-01T16:48:22Z

src/components/scenes/CoinRankingScene.tsx

+    () => ({ assetSubText, supportedFiatSetting: coingeckoFiat, percentChangeTimeFrame, forceRefresh }),
+    [assetSubText, coingeckoFiat, percentChangeTimeFrame, forceRefresh]


It wasn't clear how this forceRefresh will be used in a later commit (which is why it's generally best to include usage and implementation in the same commit, but I digress), so it seems forceRefresh is only updated when coingeckFiat changes, so this is a redundant dependency for extraData memoization.

samholmes · 2025-04-01T16:49:25Z

src/components/scenes/CoinRankingScene.tsx

+    // Always clear any existing timeout when making changes to the query and an
+    // existing timeout is in flight. Running this effect either to grab extra
+    // rows or change fiat always triggers a new query immediately.


So then why isn't it in the cleanup function?

samholmes · 2025-04-01T16:50:19Z

src/components/scenes/CoinRankingScene.tsx

+    timeoutHandler.current = undefined

    if (lastFetchedFiat.current !== coingeckoFiat) {
+      if (coinRanking.coinRankingDatas.length === 0) return // Already handling


Already handling what?

samholmes

Looking good. Just some extra comments from what I noticed while reviewing on my phone.

I’m curious to know how the testing went too.

samholmes · 2025-04-03T00:01:43Z

eslint-local-rules/useAbortable-then-abort.js

    type: 'problem',
    docs: {
-      description: 'Ensure awaited promises within useCancellable are followed by .then(safety)',
+      description: 'Ensure awaited promises within useAbortable are followed by .then(abort)',


Should this be distinct from abort() which is the function that aborts the routine? Instead this is a function that checks if the routine has aborted. What do you think of checkAbort?

I think maybeAbort is better. "Check" doesn't communicate what happens as a result of that check, just says that the abort condition is being checked. "Abort" is what's potentially going to happen.

samholmes · 2025-04-03T00:02:50Z

src/components/scenes/CoinRankingScene.tsx

  const pageQueryCancelRef = React.useRef<() => void>(() => {})
  React.useEffect(() => {
-    const { promise, cancel } = queryLoop(lastStartIndex.current)
+    const { promise, abort: cancel } = queryLoop(lastStartIndex.current)


Missed the words “cancel” in this file. Do a quick find replace with case preserved to fix the issue.

This separates the subscription effect from the pagination effect. With this separation it's clear what dependencies are required for each effect. The `queryLoop` function is not longer responsible for the loop; that's a concern for the subscription effect. The `queryLoop` is now just a query function and could be renamed. Also, the `queryLoop` is a "handler" so it can close-over the component's state completely.

This is more effective because it allows us to abort a specific routine given a reference to an abort handler function. The reason for this is to remove concern regarding shared state for aborting routines (`isQuerying` was shared between separate routine invocations).

Jon-edge force-pushed the jon/fix/markets-fiat-change branch from 1a9d91b to 870421c Compare March 24, 2025 17:28

samholmes requested changes Mar 28, 2025

View reviewed changes

src/components/cards/MarketsCard.tsx Show resolved Hide resolved

src/components/cards/MarketsCard.tsx Show resolved Hide resolved

Jon-edge force-pushed the jon/fix/markets-fiat-change branch 2 times, most recently from 2e44072 to 42220d7 Compare March 31, 2025 21:25

samholmes requested changes Apr 1, 2025

View reviewed changes

Jon-edge force-pushed the jon/fix/markets-fiat-change branch 2 times, most recently from 2e65396 to d3bef26 Compare April 1, 2025 21:54

samholmes force-pushed the jon/fix/markets-fiat-change branch 2 times, most recently from e6be2f1 to 92ce3a3 Compare April 2, 2025 18:01

Jon-edge force-pushed the jon/fix/markets-fiat-change branch from 92ce3a3 to 7b8fdd9 Compare April 2, 2025 21:20

samholmes requested changes Apr 3, 2025

View reviewed changes

samholmes force-pushed the jon/fix/markets-fiat-change branch from 7b8fdd9 to bad644e Compare April 3, 2025 15:41

Jon-edge force-pushed the jon/fix/markets-fiat-change branch 2 times, most recently from 939b89b to 0dc8a35 Compare April 3, 2025 19:53

samholmes approved these changes Apr 3, 2025

View reviewed changes

Jon-edge and others added 6 commits April 4, 2025 15:03

MarketsCard: Fix defaultFiat change refresh

5a04842

Reset subscription when fiat changes

79e80f5

Implement useCancellable.

32a26e9

Add local ESlint rule for useCancellable

4dc4071

Jon-edge force-pushed the jon/fix/markets-fiat-change branch from 0dc8a35 to 4dc4071 Compare April 4, 2025 22:35

Jon-edge merged commit d9b0e55 into develop Apr 4, 2025
2 checks passed

Jon-edge deleted the jon/fix/markets-fiat-change branch April 4, 2025 22:58

		debugLog(LOG_COINRANK, `handleEndReached. setRequestDataSize ${dataSize + QUERY_PAGE_SIZE}`)
		setRequestDataSize(dataSize + QUERY_PAGE_SIZE)

		() => ({ assetSubText, supportedFiatSetting: coingeckoFiat, percentChangeTimeFrame, forceRefresh }),
		[assetSubText, coingeckoFiat, percentChangeTimeFrame, forceRefresh]

Jon/fix/markets-fiat-change #5512

Jon/fix/markets-fiat-change #5512

Uh oh!

Conversation

Jon-edge commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CHANGELOG

Dependencies

Requirements

Uh oh!

samholmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

samholmes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samholmes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jon-edge commented Mar 22, 2025 •

edited

Loading