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

feat: user management view #9

Merged
merged 16 commits into from
Jul 24, 2024
Merged

feat: user management view #9

merged 16 commits into from
Jul 24, 2024

Conversation

typeWolffo
Copy link
Member

image
image

@typeWolffo typeWolffo self-assigned this Jul 22, 2024
@typeWolffo typeWolffo force-pushed the jw/user-management branch from 9547281 to b09d11c Compare July 22, 2024 09:30
return useMutation({
mutationFn: async (options: LoginUserOptions) => {
const response = await ApiClient.auth.authControllerLogin(options.data);

if (!response) throw new Error("Invalid username or password");
Copy link
Member Author

Choose a reason for hiding this comment

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

@k1eu any ideas on how to move to onError in this case?
the problem is in return response.data, where the response is undefined, even if the ‘real’ response has an error message.

Copy link
Member

Choose a reason for hiding this comment

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

it should actually go to onError if you throw or the ApiCLient throws here 🤔

@typeWolffo typeWolffo requested review from k1eu and mikoscz July 22, 2024 09:45
Comment on lines 22 to 23
onSuccess: (data) => {
setCurrentUser(data.data);
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a /current-user endpoint and treat useCurrentUser() - as react query hook for that with longer revalidate time, then in the

Comment on lines 13 to 16
const { setCurrentUser, currentUser } = useAuthStore();
if (!currentUser) {
throw new Error("User is not logged in");
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove that, user shouldn't be able to get into that action ui wise, and if SOMEHOW he gets there - he should be blocked on BE roles

@typeWolffo typeWolffo requested a review from k1eu July 22, 2024 13:14
Comment on lines 13 to 16
const { currentUser } = useAuthStore();
if (!currentUser) {
throw new Error("User is not logged in");
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove this, if user is not logged in backend will return 401


type LoginUserOptions = {
data: LoginBody;
};

export function useLoginUser() {
const { setLoggedIn } = useAuthStore();
const setLoggedIn = useAuthStore.getState().setLoggedIn;
Copy link
Member

Choose a reason for hiding this comment

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

Why this change ? Didn't you want to add selector here?

return useQuery(currentUserQueryOptions);
}

export function useSafeCurrentUser() {
Copy link
Member

Choose a reason for hiding this comment

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

useCurrentUserSuspense let's make it clear for what it is

@@ -97,4 +99,16 @@ export class AuthController {

return null;
}

@Get("me")
Copy link
Member

Choose a reason for hiding this comment

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

/current-user

ApiClient.instance.interceptors.response.use(
(response) => response,
async (error) => {
const isLoggedIn = useAuthStore.getState().isLoggedIn;
const originalRequest = error.config;

if (
some(bypassRefreshEndpoints, (endpoint) =>
Copy link
Member

Choose a reason for hiding this comment

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

I think even without it login won't do the refresh token since user is logged out and we check it right? :D

) {
return Promise.reject(error);
}

if (error.response?.status === 401 && !originalRequest._retry) {
originalRequest._retry = true;
if (!isLoggedIn) return;
Copy link
Member

Choose a reason for hiding this comment

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

I think that can be moved up from this if so it exits to normal flow quicker

@typeWolffo typeWolffo requested a review from k1eu July 22, 2024 17:16
Base automatically changed from jw/auth-screens to main July 22, 2024 17:16
Copy link
Member

@k1eu k1eu left a comment

Choose a reason for hiding this comment

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

Overall cool, could you add also Query Client cache purge on logout?

@@ -51,6 +51,15 @@ export type LogoutResponse = null;

export type RefreshTokensResponse = null;

export interface MeResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Please regenerate this file so it's cucrent user response

export function useCurrentUserSuspense() {
const { data, ...rest } = useSuspenseQuery(currentUserQueryOptions);

return { data: data?.data, ...rest };
Copy link
Member

Choose a reason for hiding this comment

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

In suspense query the data shouldn't be optional I guess

@typeWolffo
Copy link
Member Author

Overall cool, could you add also Query Client cache purge on logout?

@k1eu done

@typeWolffo typeWolffo merged commit 0f02e10 into main Jul 24, 2024
@typeWolffo typeWolffo deleted the jw/user-management branch July 24, 2024 09:16
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.

2 participants