Conversation
There was a problem hiding this comment.
Pull request overview
Merges develop into the auth-integration workstream and introduces initial client-side auth token handling + overlay modal refactor in the web app, along with minor tooling updates.
Changes:
- Add JWT/Zustand-based auth utilities and wire them into login + a protected loader.
- Refactor the “overlay” route modal into a reusable
OverlayModalcomponent used by the add-member flow. - Update dev tooling (DevPod KUBECONFIG path, Husky pre-commit hook) and dependencies (
jwt-decode,zustand, Turbo bump).
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/devpod/setup.sh | Makes KUBECONFIG an absolute path for DevPod usage. |
| pnpm-lock.yaml | Locks new deps (jwt-decode, zustand) and bumps Turbo. |
| apps/web/package.json | Adds jwt-decode and zustand dependencies for auth work. |
| apps/web/app/routes/overlay.tsx | Removes the dedicated overlay route implementation. |
| apps/web/app/routes/login.tsx | Reworks login action to call API and store tokens. |
| apps/web/app/routes/createorg.tsx | Adds auth guarding via loader + runtime checks. |
| apps/web/app/routes/add-member.tsx | Switches overlay usage to OverlayCard + OverlayModal. |
| apps/web/app/routes.ts | Removes overlay route; adds (currently unused) auth loader import. |
| apps/web/app/lib/auth.ts | Adds Zustand-based auth state, token expiry check, refresh + auth loader. |
| apps/web/app/components/OverlayModal.tsx | Adds reusable overlay modal body content using Radix Dialog close. |
| .husky/pre-commit | Adds a pre-commit hook to run formatting and re-stage changes. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const payload: JwtPayload = jwtDecode(token); | ||
| if (!payload || !payload?.exp) return true; |
There was a problem hiding this comment.
jwtDecode(token) can throw for malformed/empty tokens. isTokenExpired should catch decode errors and treat them as expired instead of throwing (which can break routing/guards).
| const payload: JwtPayload = jwtDecode(token); | |
| if (!payload || !payload?.exp) return true; | |
| let payload: JwtPayload; | |
| try { | |
| payload = jwtDecode<JwtPayload>(token); | |
| } catch { | |
| // Treat any decode error (malformed/empty token) as expired | |
| return true; | |
| } | |
| if (!payload || payload.exp == null) return true; |
| @@ -1,4 +1,5 @@ | |||
| import { type RouteConfig, index, route } from "@react-router/dev/routes"; | |||
| import { requireAuthLoader } from "./lib/auth"; | |||
There was a problem hiding this comment.
requireAuthLoader is imported but not used in this route config, which will fail typecheck/lint in most setups. Remove the import, or wire it into the appropriate protected routes if that was the intent.
| import { requireAuthLoader } from "./lib/auth"; |
| rightElement={ | ||
| <Button variant="ghost" className="py-2 px-4" asChild> | ||
| <ChevronDown className="size-4 text-foreground" /> | ||
| </Button> | ||
| } |
There was a problem hiding this comment.
Button is rendered with asChild but the child is a ChevronDown SVG. This turns the SVG into the interactive element (it receives button props/classes), which is not a valid/accessible button and can break click/keyboard behavior. Render a real <button> (no asChild) and place the icon inside it, or use size="icon" on the Button.
| rightElement={ | ||
| <Button variant="ghost" className="py-2 px-4" asChild> | ||
| <ChevronDown className="size-4 text-foreground" /> | ||
| </Button> | ||
| } |
There was a problem hiding this comment.
Same issue as above: Button asChild wraps an SVG icon (ChevronDown), making the SVG the interactive element. Use a real button element and put the icon inside it.
| export async function loginAction({ request }: ActionFunctionArgs) { | ||
| const fd = await request.formData(); |
There was a problem hiding this comment.
routes/login.tsx exports loginAction, but React Router expects an action export for route actions. As-is, the login form POST will not be handled. Rename the function to action or re-export it as action from this module.
| // loader to enforce authentication at route level | ||
| export async function requireAuthLoader() { | ||
| const state = userAuthStore.getState(); | ||
|
|
||
| if (state.isAuthenticated()) return null; | ||
|
|
||
| if (state.refreshToken) { | ||
| try { | ||
| const newAccessToken = await refreshAccessToken(state.refreshToken); | ||
| state.signIn(newAccessToken, state.refreshToken); |
There was a problem hiding this comment.
requireAuthLoader reads auth state from a client-side Zustand store. Loaders run on the server and need to derive auth from the incoming request (e.g., cookies/headers). As written, authenticated requests will still look unauthenticated on the server, causing unwanted redirects.
| const email = fd.get("email") ?? ""; | ||
| const password = fd.get("password") ?? ""; |
There was a problem hiding this comment.
fd.get() returns FormDataEntryValue | null (can be a File), but email/password are sent directly in JSON. Coerce these to strings (and handle null) before JSON.stringify to avoid sending non-string values.
| const email = fd.get("email") ?? ""; | |
| const password = fd.get("password") ?? ""; | |
| const emailEntry = fd.get("email"); | |
| const passwordEntry = fd.get("password"); | |
| const email = typeof emailEntry === "string" ? emailEntry : ""; | |
| const password = typeof passwordEntry === "string" ? passwordEntry : ""; |
| @@ -0,0 +1,76 @@ | |||
| import * as DialogPrimitive from "@radix-ui/react-dialog"; | |||
| import { TextInput } from "~/components/TextInput"; | |||
| import { ChevronDown, X } from "lucide-react"; | |||
There was a problem hiding this comment.
X is imported from lucide-react but never used, which will cause lint/typecheck failures. Remove the unused import.
| import { ChevronDown, X } from "lucide-react"; | |
| import { ChevronDown } from "lucide-react"; |
| import { Link } from "react-router"; | ||
| import { Plus, PlusIcon, Trash2 } from "lucide-react"; | ||
| import { Button } from "~/components/Button"; |
There was a problem hiding this comment.
Plus is imported but never used (only PlusIcon is used). Remove the unused import to keep lint/typecheck clean.
| // Zustand local storage to manage user authentication tokens | ||
| export const userAuthStore = create<AuthType>()( | ||
| persist( | ||
| (set, get) => ({ | ||
| accessToken: null, | ||
| refreshToken: null, | ||
| signIn: (accessToken, refreshToken) => | ||
| set({ accessToken, refreshToken: refreshToken || null }), | ||
| signOut: () => set({ accessToken: null, refreshToken: null }), | ||
| isAuthenticated: () => { | ||
| const { accessToken } = get(); | ||
| return accessToken !== null && !isTokenExpired(accessToken); | ||
| }, | ||
| }), | ||
| { | ||
| name: "auth-storage", // key name to be saved in localStorage | ||
| }, | ||
| ), |
There was a problem hiding this comment.
zustand persist without an SSR-safe storage configuration will attempt to use localStorage, which is unavailable during server rendering and in route loaders/actions. Configure persist with an SSR-safe storage (or make this store client-only) so importing this module on the server does not throw.
Merging the develop branch with my current progress on integrating authentication api.