fix: refactor active project setup to fix potential issues related to using stale client #363
Conversation
| Promise.all([ | ||
| queryClient.invalidateQueries({ | ||
| queryKey: [INVITE_KEY], | ||
| }), | ||
| queryClient.invalidateQueries({ | ||
| queryKey: [ALL_PROJECTS_KEY], | ||
| }), | ||
| ]).then(() => { |
There was a problem hiding this comment.
before, this was improperly trying to update queries with a query key that essentially looked like INVITE_KEY + ALL_PROJECTS_KEY, which leads to no queries being updated. the proper way to invalidate several unrelated queries is to make separate invalidateQueries() calls
|
|
||
| return useMutation({ | ||
| mutationFn: async ({inviteId}: {inviteId: string}) => { | ||
| if (!inviteId) return; | ||
| mapeoApi.invite.accept({inviteId}); | ||
| return mapeoApi.invite.accept({inviteId}); |
There was a problem hiding this comment.
it's important to actually return this value here. otherwise a couple of things will happen:
- You won't get the return value of
invite.accept()after the mutation succeeds (doesn't currently break anything) - You'll potentially run into race conditions because you may try to do work after the mutation call occurs under the assumption that
invite.accept()has resolved. However, without this change, the accept resolves on the next iteration in the event loop, breaking that assumption
| }, | ||
| onSuccess: () => { | ||
| // This is a workaround. There is a race condition where the project in not available when the invite is accepted. This is temporary and is currently being worked on. | ||
| setTimeout(() => { | ||
| queryClient |
There was a problem hiding this comment.
Wondering if the changes here might help with the comment above this call? wasn't too sure so didn't attempt to make any changes to fix it in this PR
| if (!projectId) throw new Error('Active project ID must exist'); | ||
| return api.getProject(projectId); | ||
| }, | ||
| enabled: !!projectId, |
There was a problem hiding this comment.
i think this enabled is useful here. at least in the context of the ActiveProjectContext, my understanding is that it saves an unnecessary call of queryFn (i.e. when there is no persisted project id)
achou11
force-pushed
the
ac/active-project-refactor
branch
2 times, most recently
from
1f01c67
to
51a8f5f
Compare
There was a problem hiding this comment.
There is actually a slight problem (that was also present in the previous implementation, but is a little more obvious in your implementation because of your use of tanstack).
tanstack uses stale while revalidate architecture. So when a user changes to a new project, while it is fetching the new project, the old project is returned. Meaning that the user is able to interact with the old project while the new project in being re-fetched. This is not going to be a problem on most phones because it is really fast. But it might be a problem for slower phones.
We could possibly use the background fetching indicator, to block the UI, but the context lives outside the navigator, so conditionally rendering a loader will unmount the navigator which is not ideal. There are other ways we can block the ui without unmounting but im not sure of the most elegant way of doing that.
Not sure what the preferred solution is, but we can probably use an absolutely positioned translucent loader that overlays the whole app (i.e. conditionally rendered on top of the app) and blocks pointer events, at least to start. A more extreme idea is prompting to restart the app (whether manually or via something like https://github.com/avishayil/react-native-restart), but that's a pretty heavy-handed solution and probably not something to initially consider |
achou11
force-pushed
the
ac/active-project-refactor
branch
from
51a8f5f
to
daf0cec
Compare
|
do you think i should try to tackle that in this PR or do it as a follow up? |
ill create a ticket to tackle in a follow up PR |
Seeing the comment here highlighted issues around the active project setup where one could be working with a stale project client instance. These changes were initially applied to that branch to confirm that they fixed the issue noted there, which it did.
Instead of relying on useEffects and manually fetching new project instances, we now use React Query to handle the fetching. More importantly, this makes it easier to properly invalidate caches when needed, which I believe is the root of the issue that initiated this refactor.
Took some liberties to also clean up some low-hanging fruit in other existing query hooks. Apologies for the extra noise if distracting