If the request to update fails, do we still need to invalidate team members?

Also, as the mutationFn resolves the updated team members list, same as get team members call
https://github.com/sourcegraph/sourcegraph/blob/cc1cd46cfb9acc3225cd9139c5f29a53e463adf6/client/web/src/cody/management/api/client.ts#L51-L57
instead of invalidating team members in React Query cache, we can update it directly, e.g. like here
https://github.com/sourcegraph/sourcegraph/blob/cc1cd46cfb9acc3225cd9139c5f29a53e463adf6/client/web/src/cody/management/api/react-query/subscriptions.ts#L51-L60

If the request to update fails, do we still need to invalidate team members?

It was not a conscious decision, but now after thinking about it, I think it'd hurt us more if we didn't do this.
Like, what if there was an error for some reason but the update still affected the DB?
So it feels safer to invalidate it regardless.
It's a rare edge case, so I'd not think a lot about it.

Also, as the mutationFn resolves the updated team members list, same as get team members call

Thanks, fixed.

Ah, sorry, I misread the second suggestion at first. Good idea, I'll do that.

Is this what you meant, @taras-yemets? https://github.com/sourcegraph/sourcegraph/pull/63267/commits/9311bba7a6c8676beb1e378d3a3d481c57433c3e

TBH, I think we don't need the invalidation line. Am I right?

Is this what you meant, @taras-yemets? https://github.com/sourcegraph/sourcegraph/commit/9311bba7a6c8676beb1e378d3a3d481c57433c3e

onSuccess: (data: TeamMember[]) => {
  queryClient.setQueryData(queryKeys.teams.teamMembers(), data)
}

We already updated the cache record with the key queryKeys.teams.teamMembers(), no need to invalidate it (don't return anything from success handler).

I think it'd hurt us more if we didn't do this.
Like, what if there was an error for some reason but the update still affected the DB?

I expect backend to handle unsuccessful DB transactions (e.g., rollback). But I agree, it doesn't hurt.

TBH, I think we don't need the invalidation line. Am I right?

If we remove the team member (or change their role) in this mutation, useTeamMembers query needs to be invalidated. Otherwise it still keeps the old state with the team member existing (or with "old" role). WDYT?

Oh, I see. Is this even better, then? https://github.com/sourcegraph/sourcegraph/pull/63267/commits/f27539367a4793a6d8ea55f376ac968449d4f8de
(I've also updated the component to simplify it, which I forgot earlier.)

Oh, I see. Is this even better, then? https://github.com/sourcegraph/sourcegraph/commit/f27539367a4793a6d8ea55f376ac968449d4f8de

👍🏻

I've also updated the component to simplify it

someMutation.mutateAsync.call seems overcomplicated to me (more on it in https://github.com/sourcegraph/sourcegraph/pull/63227#discussion_r1644225613).

Similar to https://github.com/sourcegraph/sourcegraph/pull/63267/files#r1642617096

The link is broken but I guess you meant this? https://github.com/sourcegraph/sourcegraph/pull/63267/commits/65b09e80b62199311ae2a196b580732a6075d16f
(the resend endpoint returns an empty response, not the invite)

I think it refers to updating the cache from response instead of invalidating the query (similar to https://github.com/sourcegraph/sourcegraph/pull/63267#discussion_r1642617096).

Done: https://github.com/sourcegraph/sourcegraph/pull/63267/commits/4ba0fd247a208c3cd61802e095dbbd395ba3a560

Just checking: do we have an accompanying PR to SSC repo exposing this field?

It should already expose the field because the old approach was already using it. I'll double-check.

It looks like we don't expose it: convert function, type definition.

Hmm, weird. I'll add it.

Added, PTAL: https://github.com/sourcegraph/self-serve-cody/pull/911

isPro may also be true if subscriptionQueryResult is loading or errored out.
Later in useEffect we use this value to decide whether we need to navigate to "/cody/manage" page. I think the comment may helpf to inform the reader that "data is loaded, we know user's plan, it's not on a Pro, redirect them to another page". WDYT?

Thanks, good catch. Fixed in https://github.com/sourcegraph/sourcegraph/pull/63267/commits/bb2ab106c20cf970466149e34207f2e840f24ce2. Does it make sense now?

Do we need to revisit error-handling logic here after we switch custom mutation hooks?

sendInviteMutation's mutateFn uses callCodyProApi which throws on non-2xx responses:
https://github.com/sourcegraph/sourcegraph/blob/cc1cd46cfb9acc3225cd9139c5f29a53e463adf6/client/web/src/cody/management/api/react-query/callCodyProApi.ts#L51-L61

Promise.all rejects in any of the input promises rejects, thus we won't likely get to the following line:

if (responses.some(response => response.status !== 200)) {

and will get to the catch block (disclaimer: it's an assumption, I haven't run the code myself).

Also, some of the invites may be sent successfully before one fails. We may want to keep track of what invites exactly failed.

Do we still need to keep invitesSendingStatus and invitesSendingErrorMessage in state? Can we derive these values from sendInviteMutation?

Can we derive invitesSentCoiunt from emailAddresses.length and sendInviteMutation status?

Do we need to revisit error-handling logic here after we switch custom mutation hooks?
Also, some of the invites may be sent successfully before one fails. We may want to keep track of what invites exactly failed.

Completely rewrote error handling here: https://github.com/sourcegraph/sourcegraph/pull/63267/commits/22ce799f3485e47c06d01d858e1892eac36400fc

Do we still need to keep invitesSendingStatus and invitesSendingErrorMessage in state? Can we derive these values from sendInviteMutation?

Can we derive invitesSentCount from emailAddresses.length and sendInviteMutation status?

Did both in: https://github.com/sourcegraph/sourcegraph/pull/63267/commits/be05e798a50dcb4a6d0139d832db9b0ab089f20c