fix: Login sometimes fails to redirect to index, requiring reload#80
fix: Login sometimes fails to redirect to index, requiring reload#80
Conversation
Refactor authContext.tsx, moving session storage logic to a separate hook.
There was a problem hiding this comment.
Pull request overview
This PR refactors the authentication flow to fix intermittent login redirect issues. The changes extract session management logic into a reusable hook and ensure proper component mounting before navigation attempts.
Changes:
- Created a new
useStorageStatehook to centralize session storage and validation logic - Modified auth context to use the new hook and simplified login/logout functions
- Updated Login page to check for existing sessions and redirect authenticated users
- Added loading state handling with a new
Checkingcomponent - Simplified backend OAuth callback to use passport's built-in redirect options
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/hooks/useStorageState.ts | New hook that manages session storage, validation, and state synchronization with AsyncStorage |
| frontend/context/authContext.tsx | Refactored to use useStorageState hook, removed redundant session loading logic |
| frontend/components/Checking.tsx | New loading indicator component for auth state checks |
| frontend/app/_layout.tsx | Removed duplicate auth checking logic that caused navigation issues |
| frontend/app/Login.tsx | Added logic to redirect already-authenticated users and changed OAuth redirect mechanism |
| frontend/app/(tabs)/_layout.tsx | Added auth validation on mount and loading state handling |
| backend/src/server.ts | Simplified OAuth callback by using passport's built-in redirect options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ]); | ||
| setUser(userData); | ||
| setUserId(userData.id); | ||
| setSession({ user: userData, userId: userData.id }); |
There was a problem hiding this comment.
There's no validation that userData.id exists before accessing it. If the OAuth response doesn't include an id field, this will result in userId being undefined. Consider adding validation to ensure the user data has the expected structure before storing it.
| setSession({ user: userData, userId: userData.id }); | |
| const userId = | |
| userData && typeof userData === "object" && "id" in userData | |
| ? (userData as { id: string | number }).id | |
| : null; | |
| setSession({ user: userData, userId }); |
| console.log("useStorageState.tsx: set isLoading to false"); | ||
| console.table(action); |
There was a problem hiding this comment.
Using console.log and console.table in production code is not recommended. These debug statements should either be removed or replaced with a proper logging framework that can be disabled in production builds.
| useEffect(() => { | ||
| fetch(`http://localhost:${BACKEND_PORT}/auth/me`, { | ||
| credentials: "include", | ||
| }) | ||
| .then((response) => { | ||
| if (response.ok) { | ||
| response.json().then(login); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.error("Error checking auth:", error); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
This useEffect creates a race condition with the session validation already happening in useStorageState. When the TabLayout mounts, it fetches /auth/me and calls login(), which triggers another session storage update. This is redundant since useStorageState already validates the session on mount. Consider removing this effect and relying on the centralized session management in useStorageState.
| console.table(value); | ||
| setState(value); | ||
| setStorageItem(value); | ||
| }, []); |
There was a problem hiding this comment.
The setValue callback is missing setState in its dependency array. This should be included to ensure the callback has access to the current state setter.
| }, []); | |
| }, [setState, setStorageItem]); |
| await AsyncStorage.removeItem("userId"); | ||
| setUser(null); | ||
| setUserId(null); | ||
| setSession({ user: null, userId: null }); |
There was a problem hiding this comment.
The logout function sets the session to { user: null, userId: null } which is inconsistent with how setStorageItem handles null values. Looking at setStorageItem, passing null removes the items from storage, but passing an object with null properties stores those null values. Consider passing null directly to setSession instead of an object with null properties for consistency with the intended logout behavior.
| setSession({ user: null, userId: null }); | |
| setSession(null); |
| if (!res.ok) throw new Error("Session invalid"); | ||
| return { user: userData, userId: userData.id }; | ||
| } catch (err) { | ||
| await AsyncStorage.clear(); |
There was a problem hiding this comment.
When session validation fails, AsyncStorage.clear() is called, which removes ALL stored data including potential data unrelated to authentication. This is overly aggressive and could lead to data loss. Consider only removing the specific auth-related keys ('user' and 'userId') instead.
| await AsyncStorage.clear(); | |
| await Promise.all( | |
| authUserSessionKeys.map((key) => AsyncStorage.removeItem(key)), | |
| ); |
| export interface AuthUserSession { | ||
| user: any | null; | ||
| userId: any | null; |
There was a problem hiding this comment.
Using any type for the user object defeats TypeScript's type safety benefits. Consider defining a proper User interface or type that describes the expected structure of the user data, including at minimum the id property that's being accessed.
| console.log("userStorageState.tsx: the setter is ran"); | ||
| console.table(value); |
There was a problem hiding this comment.
Using console.log and console.table in production code is not recommended. These debug statements should either be removed or replaced with a proper logging framework that can be disabled in production builds.
1 participant
Changes
Testing