Dev#97
Conversation
- Reformatted imports for better organization in WebhookListPage and WebhookLogsPage. - Enhanced readability by adjusting spacing and line breaks throughout the components. - Updated state management and event handling for clarity in WebhookListPage. - Implemented consistent error handling and loading states in WebhookLogsPage. - Added axios utility for automatic token refresh on 401 errors. - Introduced telemetry utility for tracking actions and errors. - Adjusted routing structure in MemberRoutes and OrgRoute for better organization. - Integrated SidebarProvider in MemberLayout for improved context management. - Minor adjustments to LoginUserTemplate for role-based redirection. - Updated vitest setup for consistent import style.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces authentication and organization state management using Zustand, implements login logic with React Query, and adds an Axios interceptor for automatic token refreshing. It also includes extensive formatting updates across the documentation and the Tasks and Webhooks features. Feedback focuses on correcting programmatic navigation in components by using useNavigate instead of redirect, preventing infinite loops in the auth interceptor, moving side effects from useMemo to useEffect, and cleaning up debug logs and unused imports.
| import useAuthStore from "../Auth/v1/Store/Auth.Store"; | ||
| import SideBar from "../SideBar/v1/Section/SideBar"; | ||
| import { Outlet } from "react-router"; | ||
| import { Outlet, redirect } from "react-router"; |
There was a problem hiding this comment.
The redirect function from react-router is intended for use in loaders and actions. To perform programmatic navigation within a component, use the useNavigate hook.
| import { Outlet, redirect } from "react-router"; | |
| import { Outlet, useNavigate } from "react-router"; |
| const originalRequest = error.config as RetryAxiosRequestConfig; | ||
|
|
||
| // Check if error is 401 and we haven't already tried to refresh | ||
| if (error.response?.status === 401 && !originalRequest._retry) { |
There was a problem hiding this comment.
This interceptor could cause an infinite loop if the /auth/refresh request itself returns a 401. Add a check to ensure the interceptor does not attempt to refresh when the failed request is the refresh request.
| if (error.response?.status === 401 && !originalRequest._retry) { | |
| if (error.response?.status === 401 && !originalRequest._retry && originalRequest.url !== "/auth/refresh") { |
| let user = useAuthStore((state) => state.user); | ||
|
|
||
| useMemo(() => { | ||
|
|
||
| console.log("User in Organisation_Template-->:", user); | ||
|
|
||
|
|
||
| if (user?.role) { | ||
| if (user.role !== "organization") { | ||
| redirect("/"); | ||
| } | ||
| } | ||
| }, [user]); |
There was a problem hiding this comment.
Handle the navigation side effect using useEffect and useNavigate. Also, remove the console log and use const for the user variable.
const user = useAuthStore((state) => state.user);
const navigate = useNavigate();
useEffect(() => {
if (user?.role && user.role !== "organization") {
navigate("/");
}
}, [user, navigate]);
| import SideBarLink from "../Components/SideBarLink"; | ||
| import { dashboardData } from "@/features/Member/v1/mock/dashboardData"; | ||
| import useAuthStore from "@/features/Auth/v1/Store/Auth.Store"; | ||
| import { useEffect } from "react"; |
There was a problem hiding this comment.
Unused import useEffect.
| const response = await api.post( | ||
| `${baseUrl}${AUTH_ENDPOINTS.LOGIN}`, | ||
| credentials | ||
| ); |
There was a problem hiding this comment.
Use relative paths with the axios instance.
| const response = await api.post( | |
| `${baseUrl}${AUTH_ENDPOINTS.LOGIN}`, | |
| credentials | |
| ); | |
| const response = await api.post( | |
| AUTH_ENDPOINTS.LOGIN, | |
| credentials | |
| ); |
| const response = await api.get( | ||
| `${baseUrl}${AUTH_ENDPOINTS.GET_ORGANIZATION_BY_ID}?ownerId=${_id}` | ||
| ); |
There was a problem hiding this comment.
Use relative paths with the axios instance to leverage the pre-configured baseURL.
| const response = await api.get( | |
| `${baseUrl}${AUTH_ENDPOINTS.GET_ORGANIZATION_BY_ID}?ownerId=${_id}` | |
| ); | |
| const response = await api.get( | |
| `${AUTH_ENDPOINTS.GET_ORGANIZATION_BY_ID}?ownerId=${_id}` | |
| ); |
| import { Outlet, redirect } from "react-router"; | ||
| import BotamNavBar from "../SideBar/v1/Section/BotamNavBar"; | ||
|
|
||
| import { useMemo } from "react"; |
There was a problem hiding this comment.
Side effects like navigation should be handled in useEffect, not useMemo.
| import { useMemo } from "react"; | |
| import { useEffect } from "react"; |
| const baseUrl = | ||
| import.meta.env.VITE_API_BASE_URL || "http://localhost:8000/api/v1"; |
There was a problem hiding this comment.
The api instance (from axios.utils.ts) already has a baseURL configured. Defining baseUrl here and manually prepending it to every request is redundant and can lead to bugs if environment variables are inconsistent. Use relative paths with the axios instance.
| @@ -0,0 +1,43 @@ | |||
| import axios, { AxiosError, AxiosRequestConfig, InternalAxiosRequestConfig } from "axios"; | |||
There was a problem hiding this comment.
Unused import InternalAxiosRequestConfig.
| import axios, { AxiosError, AxiosRequestConfig, InternalAxiosRequestConfig } from "axios"; | |
| import axios, { AxiosError, AxiosRequestConfig } from "axios"; |
| console.log("Login successful:", response); | ||
|
|
||
| // redirect / save token / navigate |
There was a problem hiding this comment.
Remove debug logs and placeholder comments before merging.
1 participant
No description provided.