I think we can simplify this function by just making the filter not optional and adding a default value as {} so we could use it directly in the URLSearchParams without having to do many ifs.

getOAuth2ProviderApps = async (
	filter: TypesGen.OAuth2ProviderAppFilter = {},
): Promise<TypesGen.OAuth2ProviderApp[]> => {
	const params = new URLSearchParams(filter);
	const resp = await this.axios.get(
		"/api/v2/oauth2-provider/apps", { params },
	);
	return resp.data;
};

Another function we can improve.

export const getApps = (filter: OAuth2ProviderAppFilter = {}) => {
	return {
		queryKey: [...appsKey, filter],
		queryFn: () => API.getOAuth2ProviderApps(filter),
	};
};

Is it possible to have an OAuth2ProviderApp without an user?

yes, that's possible

What is the use case?

Different flows/grant types. Client credential flows have to be owned/tied to a user, while authorization code flow must not be tied to a user.

Do you think would make sense to left a comment in this attribute, in the codersdk type? I think this is something I would easily forget in the future.

My overall feeling is this component is doing way more than a form should do. I think we can improve by:

• Remove props that are not going to change and use the value directly
• Execute the mutation in the form, and only call the onSubmit or onSubmitSuccess if the parent needs that

By applying these, I think we can significantly reduce the complexity of this component. This may require a few changes, so probably will need a second review round.

I know using the WEB API is nice, but it has some gaps and to make the code consistent lets use Formik and Yup for validation.

You can easily find examples on how to use both in the codebase.

⚠️ Blocker

The BE supports filtering a user by username or email, if that is not available in the app, I think we can do.

• Add the username or user email to the app response
• Add an endpoint into the backend to fetch user data by ID

I don't think we should move forward with the current solution.

We should avoid adding a new dialog style only for this case. Did you take a look in the other dialogs? Wondering what you want to achieve here that is not achievable by using the existent dialogs.

I can refactor that into a component if you'd like, but none of the other dialogs allow resizing since the max width is always hardcoded.

When opening the code example in the dialog, I want it to expand both horizontally and vertically so that the word wrap on the curl command in the code example doesn't apply, and users can see the command they are about to copy and paste without having first to copy it and then inspect it in another editor.

Ok, lets keep it as it is - in terms of design. I will give it a try and see what we can do to better support this use case in our design system.

We are not using inline CSS styles anymore. Give a try to TailwindCSS class names. You can see many of them in the code base.

Looks like it should be a button instead of a link. Links should be used for navigation.

Wondering why only these two have end set to false 🤔

None of the other items has nested menus. Only the tokens and OAuth2 app section can navigate to another route, thus making the nav item "unselected".

I will give this a try and see how it behaves.

Let's also add a log here; we probably don't want to leak the error details back in the HTTP response, so let's just say something generic; admins can refer to the logs for details.

You're still returning the error directly; we probably don't want to do that; ditto for the other cases.

Why do we need this?

Nit: while it shouldn't be an issue now, I'd prefer it if we avoid the type assertion, since this will silence any type mismatches in the future. How confident are we that we won't need to support non-strings values in the future?

I feel like the Go way of doing things makes sense here: copy the values from the filter into a new object that's passed directly to the constructor:

new URLSearchParams({
  user_id: filter.user_id ?? "",
  owner_id: filter.owner_id ?? ""
});

Nit: mt-2.5 can be swapped for pt-2.5, and should be less likely to cause layout side effects from margin collapse

This long-form text should be a p tag

Two big problems here:

1. navigate is a side effect function, and shouldn't be called from inside a render like this.
2. The conditional rendering breaks React's rules of hooks. In practice, I don't see this code breaking, because the app ID shouldn't be able to change without a full page change, but breaking this rule in general can cause a React component to completely blow up and trigger our global error boundary. I don't feel comfortable with having code that the React team actively pushes you away from, because the justification here feels like "just trust me bro"

Two changes we can do:

1. Add a useEffect call to handle the navigation

   useEffect(() => {
     if (!appId) {
       navigate("/settings/oauth2-provider");
     }
   }, [navigate, appId]);

2. Anywhere after all the hooks mount, do conditional rendering that instead returns out the Navigate component (which is basically a wrapper over useNavigate+useEffect under the hood)

React Router has a weird policy of not stabilizing their function references from their hooks, so if throwing navigate in the dependency array causes too many renders (or worse, infinite renders), you can do this instead:

const stableNavigate = useEffectEvent(navigate);
useEffect(() => {
  if (!appId) {
    stableNavigate("/settings/oauth2-provider");
  }
}, [stableNavigate, appId]);

This type definition is used in a few different places, and it's complicated enough that I feel like it'd be better to extract it into a custom type alias

Is there a reason for the template literal?