Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OPIK-448] [FE] UI on editing feedback score is slow #727

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

andriidudar
Copy link
Contributor

@andriidudar andriidudar commented Nov 26, 2024

Details

Add the optimistic update for all cases when the user sets/ edits /deletes the feedback score

Issues

Resolves #694

Testing

Documentation

@andriidudar andriidudar requested a review from a team as a code owner November 26, 2024 17:23
Copy link
Collaborator

@ferc ferc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks very clean and it's great to have optimistic update for these mutations!

For next PR let's add the .cancelQueries call so we don't have to deal with some edge cases here

Comment on lines +65 to +84
// make optimistic update for compare experiments
setExperimentsCompareCache(queryClient, traceParams, deleteMutation);

if (params.spanId) {
// make optimistic update for spans
setSpansCache(
queryClient,
{
traceId: params.traceId,
spanId: params.spanId,
},
deleteMutation,
);
} else {
// make optimistic update for traces
setTracesCache(queryClient, traceParams, deleteMutation);

// make optimistic update for trace
setTraceCache(queryClient, traceParams, deleteMutation);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clean code, love it!

@@ -41,23 +55,51 @@ const useTraceFeedbackScoreDeleteMutation = () => {
variant: "destructive",
});
},
onMutate: async (params: UseTraceFeedbackScoreDeleteMutationParams) => {
Copy link
Collaborator

@ferc ferc Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to add the queryClient.cancelQueries(...) step here -> https://tanstack.com/query/v5/docs/framework/react/guides/optimistic-updates#updating-a-single-todo

If not we could face some race conditions like:

  1. You start the mutation A -> makes optimistic update (update cache) -> triggers request
  2. Mutation A finishes and it's running the onSettled callback (refetch)
  3. You make another mutation B -> makes optimistic update (A + B updates in the cache ✅ ) -> triggers request
  4. Mutation A finishes the refetch -> updates the cache with A change (reverts last mutation from an old server-state) ❌
  5. Eventually mutation B finishes the refetch -> updates the cache and A + B changes are in the cache ✅

So with cancellation of the queries step we remove the misleading old state of the step 4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. Thanks for flagging this scenario. I have created a new PR with suggested improvements.

@ferc ferc merged commit c020c08 into main Nov 26, 2024
1 check passed
@ferc ferc deleted the andriidudar/OPIK-448-optimistic-update branch November 26, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Feedback score take time to appear in the UI
3 participants