adding approval gate for closed beta#56
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an approval workflow: a non-null boolean Changes
Sequence DiagramsequenceDiagram
participant Browser as User/Browser
participant FrontSignIn as Frontend SignIn Page
participant BackendAuth as Backend Auth
participant DB as Database
participant Callback as Frontend Callback Page
participant FrontAPI as Frontend Session API
participant TokenLib as Frontend Token Lib
participant Middleware as Frontend Middleware
participant Protected as Protected App
Browser->>FrontSignIn: Click "Continue with Google"
FrontSignIn->>BackendAuth: Redirect to /api/auth/google
BackendAuth->>DB: Query/insert user (include is_approved)
DB-->>BackendAuth: User row with is_approved
alt new user
BackendAuth->>DB: Insert user (is_approved=false)
DB-->>BackendAuth: New user created
BackendAuth-->>Browser: Redirect callback?is_approved=false
else existing user
alt is_approved = true
BackendAuth-->>Browser: Redirect callback?is_approved=true
else is_approved = false
BackendAuth-->>Browser: Redirect callback?error=not_approved
end
end
Browser->>Callback: Visit callback URL with params
Callback->>Callback: Inspect error & is_approved
alt not approved
Callback-->>Browser: Redirect to /pending
else approved
Callback->>FrontAPI: POST /api/auth/session { userId }
FrontAPI->>BackendAuth: GET /auth/me?user_id=...
BackendAuth-->>FrontAPI: 200 + is_approved true
FrontAPI->>TokenLib: signSession(userId)
TokenLib-->>FrontAPI: token
FrontAPI-->>Browser: 200 OK (sets sapling_session cookie)
Callback-->>Browser: Redirect to /dashboard
end
Browser->>Protected: Request protected route
Protected->>Middleware: Evaluate request cookies
Middleware->>TokenLib: verifySession(sapling_session)
alt missing/invalid session
Middleware-->>Browser: Redirect to /signin
else session valid but not approved
Middleware-->>Browser: Redirect to /pending
else approved
Middleware->>Protected: Allow request
Protected-->>Browser: Render protected content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/routes/auth.py (1)
157-169:⚠️ Potential issue | 🟠 MajorGate approval before persisting calendar tokens.
The
oauth_tokensupsert runs before the approval check, so a waitlisted user still stores a refresh token even though access is blocked immediately afterward. Move the upsert below the gate, or explicitly revoke/delete tokens on the unapproved path.🔐 Suggested control-flow change
- # Store OAuth tokens (calendar access included) - table("oauth_tokens").upsert( - { - "user_id": user_id, - "access_token": creds.token, - "refresh_token": creds.refresh_token or "", - "expires_at": creds.expiry.isoformat() if creds.expiry else "", - }, - on_conflict="user_id", - ) - if not is_approved: return RedirectResponse(f"{FRONTEND_URL}/signin?error=not_approved") + + table("oauth_tokens").upsert( + { + "user_id": user_id, + "access_token": creds.token, + "refresh_token": creds.refresh_token or "", + "expires_at": creds.expiry.isoformat() if creds.expiry else "", + }, + on_conflict="user_id", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.py` around lines 157 - 169, The code currently persists OAuth tokens via table("oauth_tokens").upsert before checking is_approved, which allows unapproved users to retain refresh tokens; change the control flow so the is_approved check (the branch that returns RedirectResponse(f"{FRONTEND_URL}/signin?error=not_approved")) runs before calling table("oauth_tokens").upsert, moving the upsert below the approval gate, or alternatively, if you must keep the upsert where it is, call the token cleanup (delete/revoke) path for that user_id when is_approved is False; reference table("oauth_tokens").upsert, is_approved, user_id and RedirectResponse to locate and update the logic.
🧹 Nitpick comments (1)
backend/db/migration_add_is_approved.sql (1)
1-1: Consider encoding the existing-user backfill in SQL.This defaults every current row to
falseuntil someone performs the manual SQL step from the rollout notes. If the rule for retaining access is deterministic, baking that update into the migration is safer than depending on a dashboard action during deploy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/db/migration_add_is_approved.sql` at line 1, The migration adds users.is_approved with a default false but doesn't backfill existing rows; include an UPDATE in the same migration to set is_approved deterministically for current users (e.g., based on the retention rule — replace with the actual condition) so existing users don’t remain incorrectly false; perform the ALTER TABLE and then an UPDATE within the same migration (preferably wrapped in a transaction) targeting the users table and the is_approved column to apply the deterministic retention rule for all preexisting rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes/auth.py`:
- Around line 168-169: The redirect for unapproved OAuth users currently returns
RedirectResponse(f"{FRONTEND_URL}/signin?error=not_approved") which bypasses the
new pending flow; change the unapproved branch in the auth callback to redirect
to the pending page instead (e.g., RedirectResponse(f"{FRONTEND_URL}/pending")
or include any needed query params), updating the code that checks is_approved
so new waitlist users land on FRONTEND_URL + "/pending" rather than the signin
error URL; ensure you update the RedirectResponse call where is_approved is
evaluated to point to "/pending".
In `@frontend/src/context/UserContext.tsx`:
- Around line 90-96: The signOut helper currently only clears React state and
localStorage but leaves authentication cookies alive; update the signOut
function to explicitly expire the middleware cookies (sapling_uid and
sapling_approved) when signing out by setting them with an immediate expiry
(e.g., max-age=0 or a past Expires date) and include path=/ so the middleware no
longer sees them; modify the signOut implementation in UserContext.tsx to
remove/expire both "sapling_uid" and "sapling_approved" (in addition to the
existing state and localStorage clears) so middleware.ts authorization will stop
recognizing the session.
In `@frontend/src/middleware.ts`:
- Around line 14-21: The middleware currently trusts client-set cookies
sapling_uid and sapling_approved (variables uid and approved) to gate access;
replace this with server-issued, HttpOnly/signed session verification or a
server-side approval lookup: stop using sapling_uid/sapling_approved directly,
read a secure session cookie or auth token (e.g., sessionToken),
verify/deserialize it server-side (e.g., verifySessionToken or
getSessionFromStore) to obtain the user id, then query the backend (e.g.,
getUserById / isUserApproved) to determine approval and redirect via
NextResponse.redirect only if the server-validated session is missing or the
backend reports not approved. Ensure the cookie used is HttpOnly and signed (or
verify JWT signature) and remove reliance on client-mintable values.
---
Outside diff comments:
In `@backend/routes/auth.py`:
- Around line 157-169: The code currently persists OAuth tokens via
table("oauth_tokens").upsert before checking is_approved, which allows
unapproved users to retain refresh tokens; change the control flow so the
is_approved check (the branch that returns
RedirectResponse(f"{FRONTEND_URL}/signin?error=not_approved")) runs before
calling table("oauth_tokens").upsert, moving the upsert below the approval gate,
or alternatively, if you must keep the upsert where it is, call the token
cleanup (delete/revoke) path for that user_id when is_approved is False;
reference table("oauth_tokens").upsert, is_approved, user_id and
RedirectResponse to locate and update the logic.
---
Nitpick comments:
In `@backend/db/migration_add_is_approved.sql`:
- Line 1: The migration adds users.is_approved with a default false but doesn't
backfill existing rows; include an UPDATE in the same migration to set
is_approved deterministically for current users (e.g., based on the retention
rule — replace with the actual condition) so existing users don’t remain
incorrectly false; perform the ALTER TABLE and then an UPDATE within the same
migration (preferably wrapped in a transaction) targeting the users table and
the is_approved column to apply the deterministic retention rule for all
preexisting rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc11c568-e857-411b-86a2-eb8b58f3378f
📒 Files selected for processing (8)
backend/db/migration_add_is_approved.sqlbackend/db/supabase_schema.sqlbackend/routes/auth.pyfrontend/src/app/pending/page.tsxfrontend/src/app/signin/callback/page.tsxfrontend/src/app/signin/page.tsxfrontend/src/context/UserContext.tsxfrontend/src/middleware.ts
Comment on lines
90
to
96
| const signOut = () => { | ||
| setUserId(''); | ||
| setUserName(''); | ||
| setAvatarUrl(''); | ||
| setIsAuthenticated(false); | ||
| setIsApproved(false); | ||
| localStorage.removeItem('sapling_user'); |
There was a problem hiding this comment.
Expire the middleware cookies in signOut().
frontend/src/app/signin/callback/page.tsx now creates sapling_uid / sapling_approved, and frontend/src/middleware.ts authorizes off them. This helper only clears React state and localStorage, so any existing logout flow that calls signOut() leaves the cookie session alive.
🐛 Proposed fix
const signOut = () => {
setUserId('');
setUserName('');
setAvatarUrl('');
setIsAuthenticated(false);
setIsApproved(false);
localStorage.removeItem('sapling_user');
+ document.cookie = 'sapling_approved=; path=/; max-age=0; SameSite=Lax';
+ document.cookie = 'sapling_uid=; path=/; max-age=0; SameSite=Lax';
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const signOut = () => { | |
| setUserId(''); | |
| setUserName(''); | |
| setAvatarUrl(''); | |
| setIsAuthenticated(false); | |
| setIsApproved(false); | |
| localStorage.removeItem('sapling_user'); | |
| const signOut = () => { | |
| setUserId(''); | |
| setUserName(''); | |
| setAvatarUrl(''); | |
| setIsAuthenticated(false); | |
| setIsApproved(false); | |
| localStorage.removeItem('sapling_user'); | |
| document.cookie = 'sapling_approved=; path=/; max-age=0; SameSite=Lax'; | |
| document.cookie = 'sapling_uid=; path=/; max-age=0; SameSite=Lax'; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/context/UserContext.tsx` around lines 90 - 96, The signOut
helper currently only clears React state and localStorage but leaves
authentication cookies alive; update the signOut function to explicitly expire
the middleware cookies (sapling_uid and sapling_approved) when signing out by
setting them with an immediate expiry (e.g., max-age=0 or a past Expires date)
and include path=/ so the middleware no longer sees them; modify the signOut
implementation in UserContext.tsx to remove/expire both "sapling_uid" and
"sapling_approved" (in addition to the existing state and localStorage clears)
so middleware.ts authorization will stop recognizing the session.
Deploying web with
|
| Latest commit: |
d36d363
|
| Status: | ⚡️ Build in progress... |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
frontend/src/context/UserContext.tsx (1)
90-98:⚠️ Potential issue | 🟠 MajorAwait cookie deletion during sign-out.
This still fire-and-forgets the
DELETE /api/auth/sessioncall. If a caller routes away right aftersignOut(), the browser can drop the request and leave the HttpOnly cookies behind, so middleware keeps treating the session as active. MakesignOutasync and have logout handlersawaitit before navigating.🐛 Proposed fix
interface UserContextValue { userId: string; userName: string; avatarUrl: string; users: UserOption[]; userReady: boolean; isAuthenticated: boolean; isApproved: boolean; setActiveUser: (id: string, name: string, avatar?: string, approved?: boolean) => void; - signOut: () => void; + signOut: () => Promise<void>; } ... - const signOut = () => { + const signOut = async () => { setUserId(''); setUserName(''); setAvatarUrl(''); setIsAuthenticated(false); setIsApproved(false); localStorage.removeItem('sapling_user'); - fetch('/api/auth/session', { method: 'DELETE' }); + await fetch('/api/auth/session', { method: 'DELETE', keepalive: true }); };Verify which callers need to
await signOut()before redirecting:#!/bin/bash rg -n -C2 '\bsignOut\s*\(' --type=ts --type=tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/context/UserContext.tsx` around lines 90 - 98, The signOut function currently fire-and-forgets the DELETE /api/auth/session request; make signOut async (exported/used where defined) so it returns a Promise, await the fetch call inside signOut (e.g., await fetch('/api/auth/session', { method: 'DELETE' })) and handle errors/response.ok as needed, then update callers to await signOut() before performing any navigation/redirect so the HttpOnly cookie deletion completes; keep or adjust the calls to setUserId/setUserName/setAvatarUrl/setIsAuthenticated/setIsApproved as desired (either before or after awaiting the fetch) but ensure signOut returns a resolved/rejected promise clients can await.
🧹 Nitpick comments (2)
frontend/src/app/api/auth/session/route.ts (2)
12-12: Drop the restating inline comment.Line 12 just narrates the fetch below, so it adds noise more than context. As per coding guidelines, "No docstrings or comments unless the logic is non-obvious".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/api/auth/session/route.ts` at line 12, Remove the redundant inline comment "Verify with the backend that the user exists and is approved." that simply restates the subsequent fetch; edit the route handler in session/route.ts (the code block containing the fetch to the backend for session validation) and delete that line so only meaningful, non-obvious comments remain.
3-4: Fail fast on missing backend configuration.With
API_URLdefaulting to'', a missing env var falls through to the generic502 Backend unreachablepath instead of surfacing a deploy/config error. That makes sign-in failures much harder to diagnose.🐛 Proposed fix
const API_URL = process.env.NEXT_PUBLIC_API_URL ?? ''; const SESSION_MAX_AGE = 2592000; // 30 days export async function POST(request: NextRequest) { + if (!API_URL) { + return NextResponse.json({ error: 'API URL not configured' }, { status: 500 }); + } + const { userId } = await request.json();Also applies to: 15-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/api/auth/session/route.ts` around lines 3 - 4, API_URL is silently defaulting to '' which hides missing config; change the initialization so it fails fast when NEXT_PUBLIC_API_URL is undefined (e.g. throw or return a clear error) instead of using an empty string, and apply the same pattern to other backend-related constants in this file (references: API_URL and any usage in the session route handler) so sign-in failures surface a clear deploy/configuration error rather than falling through to a generic 502.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/api/auth/session/route.ts`:
- Around line 29-41: The handler in route.ts is setting plain cookies
(sapling_session and sapling_approved) which middleware.ts trusts directly;
replace this with a server-verified session mechanism: when creating a session
in this route (the code that currently sets sapling_session and
sapling_approved), generate either a signed session token or store an opaque
session id server-side and set only that token/id as an HttpOnly cookie (e.g.
signedSessionToken or sessionId). Update middleware.ts to validate that
signedSessionToken/sessionId by verifying the signature or looking it up in the
server-backed session store before granting access, and stop relying on raw
sapling_approved cookie values for the approval gate. Ensure SESSION_MAX_AGE and
cookie attributes remain applied to the secure token cookie.
In `@frontend/src/context/UserContext.tsx`:
- Around line 52-57: The user approval flag is being persisted from local
storage too early; update the UserContext setters so setIsApproved is not
restored/written until the server session exists (i.e., after the
/api/auth/session POST succeeds) — either remove persisting of isApproved in the
initial hydration in the functions that call
setUserId/setUserName/setAvatarUrl/setIsAuthenticated, or split approval into a
separate setter (e.g., setIsApprovedFromServer) and only call that after
setActiveUser / the session API resolves successfully; ensure setActiveUser no
longer writes isApproved locally before the session check and only persist
approval when you have a successful session response.
- Around line 81-84: The setActiveUser function leaves previous avatarUrl when
avatar is omitted; update setActiveUser so it explicitly resets avatarUrl when
no avatar is provided (e.g., call setAvatarUrl with an empty string or undefined
in the else branch) instead of only setting it when avatar is truthy; locate
setActiveUser and the setAvatarUrl state updater to apply this change.
---
Duplicate comments:
In `@frontend/src/context/UserContext.tsx`:
- Around line 90-98: The signOut function currently fire-and-forgets the DELETE
/api/auth/session request; make signOut async (exported/used where defined) so
it returns a Promise, await the fetch call inside signOut (e.g., await
fetch('/api/auth/session', { method: 'DELETE' })) and handle errors/response.ok
as needed, then update callers to await signOut() before performing any
navigation/redirect so the HttpOnly cookie deletion completes; keep or adjust
the calls to setUserId/setUserName/setAvatarUrl/setIsAuthenticated/setIsApproved
as desired (either before or after awaiting the fetch) but ensure signOut
returns a resolved/rejected promise clients can await.
---
Nitpick comments:
In `@frontend/src/app/api/auth/session/route.ts`:
- Line 12: Remove the redundant inline comment "Verify with the backend that the
user exists and is approved." that simply restates the subsequent fetch; edit
the route handler in session/route.ts (the code block containing the fetch to
the backend for session validation) and delete that line so only meaningful,
non-obvious comments remain.
- Around line 3-4: API_URL is silently defaulting to '' which hides missing
config; change the initialization so it fails fast when NEXT_PUBLIC_API_URL is
undefined (e.g. throw or return a clear error) instead of using an empty string,
and apply the same pattern to other backend-related constants in this file
(references: API_URL and any usage in the session route handler) so sign-in
failures surface a clear deploy/configuration error rather than falling through
to a generic 502.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7e13b02-1120-45a4-947e-2a9c0d86078d
📒 Files selected for processing (5)
backend/routes/auth.pyfrontend/src/app/api/auth/session/route.tsfrontend/src/app/signin/callback/page.tsxfrontend/src/context/UserContext.tsxfrontend/src/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/app/signin/callback/page.tsx
- backend/routes/auth.py
- frontend/src/middleware.ts
Comment on lines
81
to
84
| const setActiveUser = (id: string, name: string, avatar?: string, approved?: boolean) => { | ||
| setUserId(id); | ||
| setUserName(name); | ||
| if (avatar) setAvatarUrl(avatar); |
There was a problem hiding this comment.
Reset the avatar when the incoming user has none.
If avatar is empty or omitted, Line 84 leaves the previous avatarUrl in state. Switching from a user with an avatar to one without one will keep showing the old image.
🐛 Proposed fix
- if (avatar) setAvatarUrl(avatar);
+ setAvatarUrl(avatar || '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const setActiveUser = (id: string, name: string, avatar?: string, approved?: boolean) => { | |
| setUserId(id); | |
| setUserName(name); | |
| if (avatar) setAvatarUrl(avatar); | |
| const setActiveUser = (id: string, name: string, avatar?: string, approved?: boolean) => { | |
| setUserId(id); | |
| setUserName(name); | |
| setAvatarUrl(avatar || ''); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/context/UserContext.tsx` around lines 81 - 84, The setActiveUser
function leaves previous avatarUrl when avatar is omitted; update setActiveUser
so it explicitly resets avatarUrl when no avatar is provided (e.g., call
setAvatarUrl with an empty string or undefined in the else branch) instead of
only setting it when avatar is truthy; locate setActiveUser and the setAvatarUrl
state updater to apply this change.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
frontend/src/app/api/auth/session/route.ts (1)
31-36:⚠️ Potential issue | 🔴 CriticalAdd
secureflag to session cookies for production security on both set and clear paths.Session cookies must be transmitted over HTTPS only in production. Lines 31-36 and line 42 are missing the
secureflag, which allows cookies to be sent over unencrypted HTTP connections.🔐 Proposed fix
response.cookies.set('sapling_session', token, { httpOnly: true, + secure: process.env.NODE_ENV === 'production', sameSite: 'lax', path: '/', maxAge: SESSION_MAX_AGE, }); @@ - response.cookies.set('sapling_session', '', { httpOnly: true, maxAge: 0, path: '/' }); + response.cookies.set('sapling_session', '', { + httpOnly: true, + secure: process.env.NODE_ENV === 'production', + sameSite: 'lax', + maxAge: 0, + path: '/', + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/api/auth/session/route.ts` around lines 31 - 36, The session cookie set/clear calls in route.ts currently omit the secure flag; update both response.cookies.set calls that reference 'sapling_session' (the one that sets the token and the clear/reset call) to include secure: process.env.NODE_ENV === 'production' so the cookie is sent only over HTTPS in production while remaining usable in development; keep existing options (httpOnly, sameSite, path, maxAge) and add the secure boolean to both places.
🧹 Nitpick comments (1)
frontend/src/lib/sessionToken.ts (1)
25-28: Cache the imported CryptoKey to avoid repeated key imports.
crypto.subtle.importKeyruns on every sign/verify call. Caching improves hot-path latency without behavior changes.♻️ Proposed refactor
+let cachedKeyPromise: Promise<CryptoKey> | null = null; + async function importKey(): Promise<CryptoKey> { - const raw = new TextEncoder().encode(getSecret()); - return crypto.subtle.importKey('raw', raw, { name: 'HMAC', hash: 'SHA-256' }, false, ['sign', 'verify']); + if (!cachedKeyPromise) { + const raw = new TextEncoder().encode(getSecret()); + cachedKeyPromise = crypto.subtle.importKey( + 'raw', + raw, + { name: 'HMAC', hash: 'SHA-256' }, + false, + ['sign', 'verify'] + ); + } + return cachedKeyPromise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/sessionToken.ts` around lines 25 - 28, The importKey function repeatedly calls crypto.subtle.importKey on every invocation; change it to cache the imported CryptoKey (e.g., a module-level variable like cachedKey or cachedKeyPromise) so subsequent calls return the cached key instead of re-importing; update importKey to return the cached CryptoKey (or await the cached Promise) and keep using getSecret() only when initializing the cache to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/api/auth/session/route.ts`:
- Line 12: Remove the inline comment "// Verify with the backend that the user
exists and is approved." from the session route handler code (the comment
directly above the backend user/approval verification block) so the file no
longer contains obvious-following-code comments; leave the surrounding
verification logic (the handler/function that performs the backend user
existence/approval check) unchanged.
- Around line 15-23: The external fetch that checks backend approval (the
fetch(`${API_URL}/auth/me?user_id=${encodeURIComponent(userId)}`) call that sets
`res` and `approved`) needs a timeout to avoid blocking sign-in; wrap the fetch
with an AbortController, pass controller.signal into fetch, start a setTimeout
(e.g. 3000ms) to call controller.abort(), and clear the timeout after a
successful response; catch the abort/timeout and other errors and return the
same NextResponse.json({ error: 'Backend unreachable' }, { status: 502 }) path
so timeouts are handled gracefully.
- Around line 4-5: API_URL is defaulting to '' which causes downstream fetches
to use a relative URL and hide a misconfiguration; change the const API_URL
assignment to require process.env.NEXT_PUBLIC_API_URL (no fallback) and add a
runtime check in the session route handler (the code that calls fetch using
API_URL) that throws or returns a 500 Response with a clear message like
"NEXT_PUBLIC_API_URL not configured" when API_URL is falsy, so the app fails
fast instead of performing a relative fetch and producing misleading 401s.
In `@frontend/src/app/pending/page.tsx`:
- Around line 9-13: The sign-out flow in handleSignOut currently calls
fetch('/api/auth/session', { method: 'DELETE' }) then router.replace('/signin')
so a network exception can prevent redirect; wrap the fetch call in a
try/finally inside handleSignOut so that router.replace('/signin') is executed
in the finally block (optionally capture/log the fetch error in the catch),
ensuring the user is always redirected even if the DELETE fails.
In `@frontend/src/context/UserContext.tsx`:
- Around line 92-100: Make signOut async and await the DELETE to ensure the
session is removed before navigation: change the signOut function (in
UserContext.tsx) to async, await fetch('/api/auth/session', { method: 'DELETE'
}), handle non-OK responses/errors (log or surface), and return a promise so
callers (e.g., Navbar.handleSignOut) can await signOut() before calling
router.push('/'); update Navbar.handleSignOut to await signOut() to prevent
navigation before the session cookie is cleared.
In `@frontend/src/lib/sessionToken.ts`:
- Around line 3-7: getSecret currently accepts any non-empty value; update
getSecret() to enforce a minimum secret strength by validating
Buffer.byteLength(secret, 'utf8') >= 32 (i.e. at least 32 bytes/entropy) and
throw a clear Error if the requirement is not met; include this requirement in
the thrown message so callers know they must provide a longer secret.
- Line 1: Remove the trivial inline comment on the SESSION_MAX_AGE constant;
locate the constant export SESSION_MAX_AGE and delete the trailing "// 30 days
in seconds" comment so the line is just: export const SESSION_MAX_AGE =
2592000;.
---
Duplicate comments:
In `@frontend/src/app/api/auth/session/route.ts`:
- Around line 31-36: The session cookie set/clear calls in route.ts currently
omit the secure flag; update both response.cookies.set calls that reference
'sapling_session' (the one that sets the token and the clear/reset call) to
include secure: process.env.NODE_ENV === 'production' so the cookie is sent only
over HTTPS in production while remaining usable in development; keep existing
options (httpOnly, sameSite, path, maxAge) and add the secure boolean to both
places.
---
Nitpick comments:
In `@frontend/src/lib/sessionToken.ts`:
- Around line 25-28: The importKey function repeatedly calls
crypto.subtle.importKey on every invocation; change it to cache the imported
CryptoKey (e.g., a module-level variable like cachedKey or cachedKeyPromise) so
subsequent calls return the cached key instead of re-importing; update importKey
to return the cached CryptoKey (or await the cached Promise) and keep using
getSecret() only when initializing the cache to preserve behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10340d8d-72e9-44c5-b436-13dc61c58735
📒 Files selected for processing (6)
frontend/src/app/api/auth/session/route.tsfrontend/src/app/pending/page.tsxfrontend/src/app/signin/callback/page.tsxfrontend/src/context/UserContext.tsxfrontend/src/lib/sessionToken.tsfrontend/src/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/signin/callback/page.tsx
- frontend/src/middleware.ts
| return NextResponse.json({ error: 'Missing userId' }, { status: 400 }); | ||
| } | ||
|
|
||
| // Verify with the backend that the user exists and is approved. |
There was a problem hiding this comment.
Remove the obvious inline comment to match repo style.
Line 12 explains directly-following code and is not non-obvious logic.
As per coding guidelines, "No docstrings or comments unless the logic is non-obvious".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/api/auth/session/route.ts` at line 12, Remove the inline
comment "// Verify with the backend that the user exists and is approved." from
the session route handler code (the comment directly above the backend
user/approval verification block) so the file no longer contains
obvious-following-code comments; leave the surrounding verification logic (the
handler/function that performs the backend user existence/approval check)
unchanged.
Comment on lines
+15
to
+23
| const res = await fetch(`${API_URL}/auth/me?user_id=${encodeURIComponent(userId)}`); | ||
| if (!res.ok) { | ||
| return NextResponse.json({ error: 'User not found' }, { status: 401 }); | ||
| } | ||
| const data = await res.json(); | ||
| approved = data.is_approved === true; | ||
| } catch { | ||
| return NextResponse.json({ error: 'Backend unreachable' }, { status: 502 }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant lines
cat -n frontend/src/app/api/auth/session/route.ts | head -30Repository: SaplingLearn/Sapling
Length of output: 1335
🏁 Script executed:
# Search for timeout patterns in the codebase to understand conventions
rg -i "timeout|AbortController|signal" --type ts --type tsx -B 2 -A 2 | head -50Repository: SaplingLearn/Sapling
Length of output: 91
🏁 Script executed:
# Check for any existing fetch wrappers or utilities
fd -e ts -e tsx | xargs rg -l "fetch" | grep -E "(util|helper|fetch|request)" | head -10Repository: SaplingLearn/Sapling
Length of output: 46
Add a timeout to the backend approval check.
Line 15 performs an external fetch call without a timeout, allowing a slow or unresponsive backend to block the entire sign-in flow indefinitely. This degrades authentication reliability.
🐛 Proposed fix
try {
- const res = await fetch(`${API_URL}/auth/me?user_id=${encodeURIComponent(userId)}`);
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 5000);
+ const res = await fetch(`${API_URL}/auth/me?user_id=${encodeURIComponent(userId)}`, {
+ signal: controller.signal,
+ });
+ clearTimeout(timeout);
if (!res.ok) {
return NextResponse.json({ error: 'User not found' }, { status: 401 });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const res = await fetch(`${API_URL}/auth/me?user_id=${encodeURIComponent(userId)}`); | |
| if (!res.ok) { | |
| return NextResponse.json({ error: 'User not found' }, { status: 401 }); | |
| } | |
| const data = await res.json(); | |
| approved = data.is_approved === true; | |
| } catch { | |
| return NextResponse.json({ error: 'Backend unreachable' }, { status: 502 }); | |
| } | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 5000); | |
| const res = await fetch(`${API_URL}/auth/me?user_id=${encodeURIComponent(userId)}`, { | |
| signal: controller.signal, | |
| }); | |
| clearTimeout(timeout); | |
| if (!res.ok) { | |
| return NextResponse.json({ error: 'User not found' }, { status: 401 }); | |
| } | |
| const data = await res.json(); | |
| approved = data.is_approved === true; | |
| } catch { | |
| return NextResponse.json({ error: 'Backend unreachable' }, { status: 502 }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/api/auth/session/route.ts` around lines 15 - 23, The
external fetch that checks backend approval (the
fetch(`${API_URL}/auth/me?user_id=${encodeURIComponent(userId)}`) call that sets
`res` and `approved`) needs a timeout to avoid blocking sign-in; wrap the fetch
with an AbortController, pass controller.signal into fetch, start a setTimeout
(e.g. 3000ms) to call controller.abort(), and clear the timeout after a
successful response; catch the abort/timeout and other errors and return the
same NextResponse.json({ error: 'Backend unreachable' }, { status: 502 }) path
so timeouts are handled gracefully.
| @@ -0,0 +1,65 @@ | |||
| export const SESSION_MAX_AGE = 2592000; // 30 days in seconds | |||
There was a problem hiding this comment.
Drop the trivial inline comment on the constant.
Line 1 comment is explanatory noise for straightforward code.
As per coding guidelines, "No docstrings or comments unless the logic is non-obvious".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/lib/sessionToken.ts` at line 1, Remove the trivial inline
comment on the SESSION_MAX_AGE constant; locate the constant export
SESSION_MAX_AGE and delete the trailing "// 30 days in seconds" comment so the
line is just: export const SESSION_MAX_AGE = 2592000;.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Navbar.tsx (1)
62-69:⚠️ Potential issue | 🟠 MajorMark
/pendingas public in the navbar guard.The new waitlist route is not in
publicPaths, so if this navbar is mounted from the shared layout, unapproved users sent to/pendingwill hit Line 66 and get pushed away before they can see the waitlist screen.frontend/src/app/signin/callback/page.tsxnow routes unapproved users there, so this path needs the same exemption as the other auth pages.Proposed fix
- const publicPaths = ['/', '/signin/callback', '/about', '/terms', '/privacy']; + const publicPaths = ['/', '/pending', '/signin/callback', '/about', '/terms', '/privacy'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Navbar.tsx` around lines 62 - 69, The guard is blocking the new waitlist route; update the publicPaths array used by publicPaths and isPublic in Navbar.tsx to include '/pending' so pathname.startsWith('/careers') still works and unapproved users aren't redirected by the useEffect that calls router.push('/'); ensure the string '/pending' is added to the publicPaths list referenced where isPublic is computed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/pending/page.tsx`:
- Around line 9-16: Replace the custom logout in handleSignOut with the shared
sign-out helper to keep UserContext in sync: import and call useUser().signOut()
inside handleSignOut instead of manually removing 'sapling_user' and calling the
DELETE /api/auth/session; this ensures in-memory
userId/isAuthenticated/isApproved state is updated consistently (matching the
logic in UserContext's signOut) and use the navigation behavior provided by that
helper rather than duplicating router.replace('/signin').
In `@frontend/src/context/UserContext.tsx`:
- Around line 92-101: The signOut function currently clears local auth state in
a finally block regardless of the DELETE result; change it to await
fetch('/api/auth/session', { method: 'DELETE' }) into a response variable, check
response.ok, and only run the state-clearing calls (setUserId, setUserName,
setAvatarUrl, setIsAuthenticated, setIsApproved, localStorage.removeItem) when
res.ok is true; if res.ok is false, handle the failure by throwing or returning
an error (so callers know sign-out failed) and do not clear local session state.
Ensure this logic is applied inside the signOut function around the fetch call
and response handling.
In `@frontend/src/lib/sessionToken.ts`:
- Around line 32-41: Remove the baked-in approval flag from the session token so
approval revocation takes effect immediately: in signSession, stop including
approved (remove the approved property from the JSON payload produced by
signSession) and keep only userId and exp (or reduce SESSION_MAX_AGE if you want
shorter tokens). Then update the middleware that reads the token
(frontend/src/middleware.ts) to ignore any approved flag in the token and
instead call the authoritative check (users.is_approved) using the userId
extracted from the token before allowing access. Ensure signSession,
SESSION_MAX_AGE, and the middleware’s use of users.is_approved are the places
you change.
---
Outside diff comments:
In `@frontend/src/components/Navbar.tsx`:
- Around line 62-69: The guard is blocking the new waitlist route; update the
publicPaths array used by publicPaths and isPublic in Navbar.tsx to include
'/pending' so pathname.startsWith('/careers') still works and unapproved users
aren't redirected by the useEffect that calls router.push('/'); ensure the
string '/pending' is added to the publicPaths list referenced where isPublic is
computed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db021efd-29f0-4248-9fcb-a5087ca8ab56
📒 Files selected for processing (5)
frontend/src/app/api/auth/session/route.tsfrontend/src/app/pending/page.tsxfrontend/src/components/Navbar.tsxfrontend/src/context/UserContext.tsxfrontend/src/lib/sessionToken.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/api/auth/session/route.ts
Comment on lines
+92
to
+101
| const signOut = async () => { | ||
| try { | ||
| await fetch('/api/auth/session', { method: 'DELETE' }); | ||
| } finally { | ||
| setUserId(''); | ||
| setUserName(''); | ||
| setAvatarUrl(''); | ||
| setIsAuthenticated(false); | ||
| setIsApproved(false); | ||
| localStorage.removeItem('sapling_user'); |
There was a problem hiding this comment.
Treat a non-OK DELETE as a failed sign-out.
fetch() only throws on network errors. A 500/404 here still runs the finally block, clears local auth state, and makes callers think logout succeeded while the session cookie can remain valid. Please check res.ok and only finalize sign-out after the session endpoint actually deletes the server session.
Proposed fix
const signOut = async () => {
- try {
- await fetch('/api/auth/session', { method: 'DELETE' });
- } finally {
- setUserId('');
- setUserName('');
- setAvatarUrl('');
- setIsAuthenticated(false);
- setIsApproved(false);
- localStorage.removeItem('sapling_user');
- }
+ const res = await fetch('/api/auth/session', { method: 'DELETE' });
+ if (!res.ok) {
+ throw new Error('Failed to delete auth session');
+ }
+ setUserId('');
+ setUserName('');
+ setAvatarUrl('');
+ setIsAuthenticated(false);
+ setIsApproved(false);
+ localStorage.removeItem('sapling_user');
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/context/UserContext.tsx` around lines 92 - 101, The signOut
function currently clears local auth state in a finally block regardless of the
DELETE result; change it to await fetch('/api/auth/session', { method: 'DELETE'
}) into a response variable, check response.ok, and only run the state-clearing
calls (setUserId, setUserName, setAvatarUrl, setIsAuthenticated, setIsApproved,
localStorage.removeItem) when res.ok is true; if res.ok is false, handle the
failure by throwing or returning an error (so callers know sign-out failed) and
do not clear local session state. Ensure this logic is applied inside the
signOut function around the fetch call and response handling.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/middleware.ts (1)
5-8: Minor redundancy betweenPROTECTEDarray andmatcherconfig.The
matcherconfig determines which routes invoke this middleware, so any request reaching the middleware already matches a protected route. TheisProtectedcheck on line 14 will always be true. Consider removing thePROTECTEDarray and check, or keeping it as intentional defense-in-depth.♻️ Optional simplification
-const PROTECTED = [ - '/dashboard', '/learn', '/study', '/tree', - '/flashcards', '/library', '/calendar', '/social' -] - const API_URL = process.env.NEXT_PUBLIC_API_URL export async function middleware(request: NextRequest) { - const { pathname } = request.nextUrl - const isProtected = PROTECTED.some(p => pathname.startsWith(p)) - if (!isProtected) return NextResponse.next() - const token = request.cookies.get('sapling_session')?.valueAlso applies to: 57-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/middleware.ts` around lines 5 - 8, The PROTECTED array and the runtime isProtected check in middleware are redundant because the middleware's matcher already limits requests to those routes; remove the PROTECTED constant and the isProtected boolean check to simplify the logic (or, if you prefer defense-in-depth, keep PROTECTED but remove the matcher overlap) — update references to PROTECTED and the isProtected conditional in frontend/src/middleware.ts so the middleware no longer performs the always-true route membership test and relies on matcher (or vice versa if you choose to retain the array).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/middleware.ts`:
- Around line 5-8: The PROTECTED array and the runtime isProtected check in
middleware are redundant because the middleware's matcher already limits
requests to those routes; remove the PROTECTED constant and the isProtected
boolean check to simplify the logic (or, if you prefer defense-in-depth, keep
PROTECTED but remove the matcher overlap) — update references to PROTECTED and
the isProtected conditional in frontend/src/middleware.ts so the middleware no
longer performs the always-true route membership test and relies on matcher (or
vice versa if you choose to retain the array).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52c01da3-fdcc-495d-879e-c501fa351d20
📒 Files selected for processing (3)
frontend/src/app/pending/page.tsxfrontend/src/lib/sessionToken.tsfrontend/src/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/pending/page.tsx
- frontend/src/lib/sessionToken.ts
This was referenced Apr 15, 2026
This was referenced May 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
is_approvedcolumn to theuserstable (migration + schema update)/pendinginstead of the appsapling_approved+sapling_uidcookies/pendingwaitlist page and/signinpage with?error=not_approvedbannerPre-deploy checklist
migration_add_is_approved.sqlin Supabase SQL Editor before deploying backendis_approved = truefor existing users you want to keep accessTest plan
/pending, sign-out clears cookies and returns to/signin/dashboard, cookies set correctly/dashboardwithout cookies → redirected to/signin/pendingwithsapling_approved=1cookie → redirected to/dashboard/signin?error=not_approvedshows the pending approval bannerSummary by CodeRabbit