The form submit handler has two types of async operations: loading and validating Stripe elements and making an SSC request.
We handle error and loading states for these two types separately and then use a derived values (isLoading, errorMessage) to define the component return value.

This logic is repeated in BillingAddressForm component and we may want to extract the shared part in future, but IMO it's out of scope of this PR.

I moved the logic for the RenewSubscriptionButton and CancelSubscriptionButton components back to SubscriptionDetails. The error message is rendered within SubscriptionDetails, and passing the mutation as a prop to the button components made the logic less readable. The readability of CancelSubscriptionButton would be even worse, as it would need to pass an onSettled callback to close the modal after the mutation completes.

IMO, this change is out of the scope of this PR. This is more of an FYI comment explaining why this kind of change is in a PR advertising the third-party state management library.

Ignore changes to useApiClient.tsx from this PR. It's just reverted to the main rev.

Custom query hooks (e.g., useCurrentSubscription) and mutation hooks (e.g., useUpdateCurrentSubscription) may need to show custom error messages depending on the context. For example, both the card number form and the cancel subscription button use the useUpdateCurrentSubscription hook, but they may need to display different error messages in case of an error (e.g., "Failed to save credit card" for the former and "Failed to change subscription status" for the latter). Therefore, I leave it to the callers to define the error message.

However, there is a specific case when a request to the Cody Pro backend results in a 401 error due to expired SAMS credentials (we may have more such "special" cases in the future). In this situation, regardless of the caller context, we want to show a generic message such as "You are not authorized. Sign out and sign back in." To handle this, we need callers to distinguish errors returned from the query/mutation. For "special" cases (as identified by the getCodyProApiErrorMessage method), the specific error message should be shown. For any other errors, the custom message should depend on the caller's context.

I am not entirely satisfied with the current approach, where callers must call getCodyProApiErrorMessage to get the message text. However, I haven't found a better solution. Do you have better ideas, @vdavid, @chrsmith?

I'd probably get rid of getCodyProApiErrorMessage and just do

isCodyProApiAuthError(err) ? err.message : 'Custom error message'

whenever I want this logic.

If you don't think that's the right call here, then I suggest renaming the message parameter to defaultMessage, to better communicate its use, and maybe also mention it in getCodyProApiErrorMessage's doc that we intend to make this function more generic once we have more special cases.

Thank you, @vdavid!

isCodyProApiAuthError(err) ? err.message : 'Custom error message'

With this approach to show custom (component-specific) errors in components, we need to throw empty errors in react-query hooks, and it doesn't feel right.

But the defaultErrorMessage approach would work, I think.

With this approach to show custom (component-specific) errors in components, we need to throw empty errors in react-query hook, and it doesn't feel right.

Yeah, that definitely doesn't. But if you change this:

const errorMessage = getCodyProApiErrorMessage(subscriptionQueryResult.error, 'Failed to fetch subscription data')
    if (errorMessage) {
        return <Alert variant="danger">{errorMessage}</Alert>
    }

to

    if (subscriptionQueryResult.error) {
        return <Alert variant="danger">{isCodyProApiAuthError(err) ? err.message : 'Failed to fetch subscription data'}</Alert>
    }

that works, right?

Ah, now I see. Thank you!

I am 100,000% in support of adding more formality / types to describe the errors we receive. But let me suggest something slightly different...

Currently the backend serves 400 errors as a raw string, e.g. "Not Found: No invite with that ID was found."

The problem however, is that without any structure the frontend isn't able to understand what the error is talking about. (There could be different causes for the 400 error, and we may want to show different error messages to match.) And this "401 / auth error" is another instance of that.

So what if we did the following:

1. Update the SSC REST API to no longer return errors (4xx and 5xx responses) as strings, and instead return a well-known error response type. e.g. { message: string, cause: string }.
2. Update the ssc_proxy.go to surface 401/403 errors in this format as well.

That way, we can use a CodyProApiError type consistently, and not need to special case isCodyProApiAuthError. e.g.

if (subscriptionQueryResult.error) {
    if (subscriptionQueryResult.error.cause === 'expired_credentials') {
        return <Alert variant="danger">Could you do me a solid and login again?</Alert>
    }
    return <Alert variant="danger">{ subscriptionQueryResult.error.message }</Alert>
}

Ultimately it would be great if we could enforce that if subscriptionQueryResult.error is truthy, then subscriptionQueryResult.error.message will always be safe to show to end users. And if the client wants to handle specific error cases, they can inspect subscriptionQueryResult.error.cause.

What do you think? (It would be more of a pain to make this type of breaking API change after we launch Teams and Invites, so if we think this is something we would like, we should just go ahead and make the change. (See backend/internal/rest/response.go in the SSC codebase, as that's how we render the HTTP response bodies on the backend.)

So what if we did the following:
Update the SSC REST API to no longer return errors (4xx and 5xx responses) as strings, and instead return a well-known error response type. e.g. { message: string, cause: string }.
Update the ssc_proxy.go to surface 401/403 errors in this format as well.

Sounds good to me! I'd like it implement it in the follow-up PR.

For this PR, I suggest we show custom error messages for each component (example - it what we basically had in the SSC codebase) and handle 401 in a call API utility function (as useApiCaller does). In this way we eliminate weird isCodyProApiAuthError checks. WDYT?

Totally unrelated to this PR: if I understand what the code is doing here, it seems like using a <ButtonLink> is essentially a "foot gun" if you want to record click events using the telemetry recorder.

So thinking out-loud, would a "proper" fix for this to be making this a first-class thing? e.g. Adding a new property to <ButtonLink> like:

   ...
   recordClickEvent={ telemetryRecorder, event: 'cody.manageSubscription' }
   ...

That way, assuming you notice the recordClickEvent property, we don't need to deal with the confusing interactions between setting href AND onClick?

using a is essentially a "foot gun" if you want to record click events using the telemetry recorder

Can you elaborate on why it's dangerous, please?

would a "proper" fix for this to be making this a first-class thing? e.g. Adding a new property to like

ButtonLink is a wildcard component and IMO it's hard to justify introducing such a specific prop. Possible counter arguments: "do we need a record select/hover/focus/blur event handler too?"

😬 Didn't Justin Bieber teach us to "Never Say Never"? 🙃

If we are talking about defaults here, we cannot say this is universally true. Even for basic HTTP GET requests, things may change out from underneath the user. (And 😨 would it ever retry a POST/PATCH request because it thought things were "stale"?!?)

It seems like this could easily lead to bugs and problems.

Thank you for linking to the docs here, super-duper helpful! Perhaps we should instead take a more nuanced stance like:

            // We cache data for only a few minutes before we consider it stale.
            // But the library will refetch it anyways in the event of network
            // reconnect, window refocus, etc.  
            // See: https://tanstack.com/query/latest/docs/framework/react/guides/important-defaults
            staleTime: fiveMinutesInSeconds,

Makes sense.

Query instances via useQuery or useInfiniteQuery by default consider cached data as stale.

Let's stick to ☝🏻 defaults until we need something special here.

I am 100,000% in support of adding more formality / types to describe the errors we receive. But let me suggest something slightly different...

Currently the backend serves 400 errors as a raw string, e.g. "Not Found: No invite with that ID was found."

The problem however, is that without any structure the frontend isn't able to understand what the error is talking about. (There could be different causes for the 400 error, and we may want to show different error messages to match.) And this "401 / auth error" is another instance of that.

So what if we did the following:

1. Update the SSC REST API to no longer return errors (4xx and 5xx responses) as strings, and instead return a well-known error response type. e.g. { message: string, cause: string }.
2. Update the ssc_proxy.go to surface 401/403 errors in this format as well.

That way, we can use a CodyProApiError type consistently, and not need to special case isCodyProApiAuthError. e.g.

if (subscriptionQueryResult.error) {
    if (subscriptionQueryResult.error.cause === 'expired_credentials') {
        return <Alert variant="danger">Could you do me a solid and login again?</Alert>
    }
    return <Alert variant="danger">{ subscriptionQueryResult.error.message }</Alert>
}

Ultimately it would be great if we could enforce that if subscriptionQueryResult.error is truthy, then subscriptionQueryResult.error.message will always be safe to show to end users. And if the client wants to handle specific error cases, they can inspect subscriptionQueryResult.error.cause.

What do you think? (It would be more of a pain to make this type of breaking API change after we launch Teams and Invites, so if we think this is something we would like, we should just go ahead and make the change. (See backend/internal/rest/response.go in the SSC codebase, as that's how we render the HTTP response bodies on the backend.)