perf: add auth middleware to speed up navigation changes#298
Merged
perf: add auth middleware to speed up navigation changes#298
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements middleware-based authentication to improve page navigation performance by eliminating redundant /user/me API calls. The middleware validates sessions once per navigation and caches user data in headers, while converted pages use server-side data fetching with React Query hydration.
Changes:
- Added authentication middleware with 1-hour session caching to reduce redundant API calls
- Created server-side helper to extract user data from request headers
- Converted multiple pages to server components with proper data prefetching patterns
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend-v2/src/middleware.ts | Implements middleware with session caching and auth enforcement |
| frontend-v2/src/lib/server-user.ts | Provides helper to extract user from middleware-injected headers |
| frontend-v2/src/app/users/user-form.tsx | Fixes controlled/uncontrolled Select component state |
| frontend-v2/src/app/users/page.tsx | Restructures hydration boundary and parallelizes prefetch queries |
| frontend-v2/src/app/users/new/page.tsx | Moves hydration boundary inside ContentWrapper |
| frontend-v2/src/app/users/[id]/page.tsx | Removes redundant chapter list prefetch and restructures hydration |
| frontend-v2/src/app/interest/generator/page.tsx | Removes unnecessary prefetching infrastructure |
| frontend-v2/src/app/event/page.tsx | Removes Suspense wrapper |
| frontend-v2/src/app/event/[id]/page.tsx | Removes prefetching infrastructure |
| frontend-v2/src/app/coaching/[id]/page.tsx | Removes prefetching infrastructure |
| frontend-v2/src/app/authed-page-layout.tsx | Replaces fetchSession with getServerUser helper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
frontend-v2/src/middleware.ts:1
- This simple hash function is vulnerable to collisions and could allow different cookie strings to map to the same cache entry, potentially exposing one user's session to another. Use a cryptographic hash function like SHA-256 or a secure library to hash the cookie string.
import { NextResponse } from 'next/server'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexsapps
reviewed
Feb 6, 2026
| * Only works in server components. | ||
| */ | ||
| export async function getServerUser(): Promise<User> { | ||
| const headersList = await headers() |
Collaborator
There was a problem hiding this comment.
this can be inlined if only used once and the variable's name doesn't provide context
Member
Author
There was a problem hiding this comment.
i think i'd prefer to keep it separate since the SERVER_USER_HEADER is shared w/ another file too.
Collaborator
There was a problem hiding this comment.
i meant const userHeader = (await headers()).get(SERVER_USER_HEADER) but no worries
| <HydrationBoundary state={dehydrate(queryClient)}> | ||
| <Navbar /> | ||
| <ContentWrapper size="sm" className="gap-8"> | ||
| <Navbar /> |
Collaborator
There was a problem hiding this comment.
btw i was thinking of prefetching the chapter list for SSR of the navbar's chapter switcher's chapter list, in which case the Navbar would need to be in HydrationBoundary. but maybe that too should not use tanstack query but get headers set by the middleware? someday
Member
Author
There was a problem hiding this comment.
tbh i think we might have been a bit too heavy handed initially w/ SSR. idk if it actually makes this app feel slower or faster, and not sure how much benefits there are for being so SSR heavy w/ an internal app.
Collaborator
There was a problem hiding this comment.
i've been surprised how little problems it's caused me. all my work on the new users/activist pages so far have not had any issues with SSR. to me it feels faster / i am still enjoying the reduced flickering. but maybe it's not worth spending time on little things like the chapter list switcher. probably most of the pain is with the scaffolding of the site which you did most of so that would make sense if your experience has been different! any new pages we make will probably be with AI now which has no trouble with this sort of thing, fwiw.
i imagine the benefits of SSR may be more exaggerated on poor network connections where the extra round trip is more expensive. for event attendance i imagine that does make a difference since we take attendance in remote places and courthouses with thick walls and no wifi.
Member
Author
There was a problem hiding this comment.
are you referring to locally or in prod? i noticed a lot of slowness on the users page. this pr helped it a bit.
the main thing i've experienced is clicking a button and there being a noticeable delay before there is ANY feedback that something is happening. almira commented on it too w/o me saying anything. i feel like immediately navigating and then having a skeleton or spinner while fetching data "feels" faster in a lot of places b/c it's more responsive.
i haven't used the new activist page much yet.
|
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 Edge middleware that caches sessions and injects a serialized user header; introduces a server helper to read that header. Moves HydrationBoundary scopes inward across multiple pages, adds server-side id validation (notFound on NaN), consolidates SSR prefetch patterns, and normalizes a form field value. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant Middleware as Edge Middleware
participant Cache as In-Memory Cache
participant AuthAPI as Auth API (fetchSession)
participant ServerComp as Server Component
Client->>Middleware: HTTP request (cookies)
Middleware->>Cache: Lookup session by cookie-derived key
alt Cache hit (valid)
Cache-->>Middleware: Return cached session.user
else Cache miss or expired
Middleware->>AuthAPI: fetchSession using cookies
AuthAPI-->>Middleware: session + user (or null)
Middleware->>Cache: Store {user, timestamp}
end
alt user present
Middleware->>Middleware: Serialize user -> `x-user-data` header
Middleware->>ServerComp: Forward request + header
ServerComp->>ServerComp: getServerUser() reads header -> User
ServerComp-->>Client: Rendered response
else no user
Middleware-->>Client: Redirect to /login
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
frontend-v2/src/app/users/[id]/page.tsx (1)
23-27:⚠️ Potential issue | 🟡 MinorMissing chapter list prefetch.
The
UserFormcomponent fetches chapters viauseQuerywithqueryKey: [API_PATH.CHAPTER_LIST]to populate the chapter dropdown. Without server-side prefetching, this data will be fetched client-side, causing a loading state or flash when the form renders.⚡ Proposed fix to add chapter list prefetch
// Prefetch user data on server - await queryClient.prefetchQuery({ - queryKey: [API_PATH.USERS, userId], - queryFn: () => apiClient.getUser(userId), - }) + await Promise.all([ + queryClient.prefetchQuery({ + queryKey: [API_PATH.USERS, userId], + queryFn: () => apiClient.getUser(userId), + }), + queryClient.prefetchQuery({ + queryKey: [API_PATH.CHAPTER_LIST], + queryFn: () => apiClient.getChapterList(), + }), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/app/users/`[id]/page.tsx around lines 23 - 27, Add a server-side prefetch for the chapter list so UserForm's useQuery (queryKey: [API_PATH.CHAPTER_LIST]) is hydrated and avoids a client-side load/flash: call queryClient.prefetchQuery with queryKey [API_PATH.CHAPTER_LIST] and queryFn invoking the same API client method UserForm uses (e.g., apiClient.getChapterList or apiClient.getChapters), placed alongside the existing prefetchQuery for users in the page component so the chapter dropdown is populated on render.
🤖 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-v2/src/app/authed-page-layout.tsx`:
- Line 12: The call to getServerUser() in the Authed page layout can throw when
the auth header is missing, causing a 500; wrap the await getServerUser() call
in a try/catch (inside the AuthedPageLayout or the top-level async function that
calls it), and on error or a falsy user perform a server-side redirect to the
login route (e.g., call your existing redirect helper or return the appropriate
redirect response) instead of letting the exception propagate; ensure you
reference getServerUser and the AuthedPageLayout entry point so the redirect is
executed before rendering.
In `@frontend-v2/src/app/coaching/`[id]/page.tsx:
- Around line 19-27: Validate the route param before converting: in page.tsx,
check that id is a valid integer (e.g. const parsed = parseInt(id, 10); if
(Number.isNaN(parsed) || String(parsed) !== String(id)) return notFound();),
then use parsed as eventId for ApiClient creation and queryClient.prefetchQuery
(references: eventId variable, ApiClient, getCookies, queryClient.prefetchQuery,
API_PATH.EVENT_GET). Use Next.js notFound() (from next/navigation) or throw a
404 Response to return a 404 for invalid IDs.
In `@frontend-v2/src/app/event/`[id]/page.tsx:
- Around line 19-27: Validate the route param `id` before prefetching: check the
original `id` string and bail or render a not-found/error state if it is not a
valid integer, and use the original `id` string as the query key for
consistency. Concretely, before calling queryClient.prefetchQuery, verify
Number(id) is a finite integer (the variable eventId) and if invalid skip
prefetch or throw/return a 404; when valid, keep the queryKey as
[API_PATH.EVENT_GET, String(id)] and call apiClient.getEvent(eventId) so the key
on server and client match and the API receives a proper numeric ID. Ensure the
checks reference eventId, id, API_PATH.EVENT_GET, queryClient.prefetchQuery, and
apiClient.getEvent.
In `@frontend-v2/src/app/users/`[id]/page.tsx:
- Around line 18-19: Validate the route param before calling the API: ensure the
extracted id from params (const { id } = await params) is parsed to a finite
integer (userId) and if it is NaN or not a valid number call notFound() to
return a 404 instead of passing NaN into apiClient.getUser(); add the import for
notFound from 'next/navigation' at the top and replace the current Number(id)
usage in page.tsx with a validation branch that triggers notFound() for invalid
IDs before calling apiClient.getUser().
In `@frontend-v2/src/lib/server-user.ts`:
- Line 20: Guard the JSON.parse(userHeader) call by first validating userHeader
is present and then wrapping the parse in a try/catch; if parsing fails (or
userHeader is falsy) catch the error and return an explicit auth-failure value
(e.g., null/undefined or a specific error result used by your auth path) instead
of allowing the exception to bubble to a 500. Update the function containing the
JSON.parse(userHeader) call to perform this check and safe parse so downstream
auth logic receives a clear auth-failure signal.
In `@frontend-v2/src/middleware.ts`:
- Around line 87-89: The middleware currently serializes the entire session.user
into headers using requestHeaders.set(SERVER_USER_HEADER,
JSON.stringify(session.user)), which exposes PII and risks large headers;
instead extract and serialize only the minimal claims needed downstream (e.g.,
user.id, user.emailVerified, user.role) and set those on SERVER_USER_HEADER (or
a new SERVER_USER_CLAIMS constant) using requestHeaders.set, or better set
separate specific headers for each claim your downstream code expects; update
any code that reads SERVER_USER_HEADER to parse the reduced claims shape
accordingly.
- Around line 78-84: Wrap the call to getCachedSession in a try/catch inside the
middleware so any thrown errors are handled explicitly; on catch, log or record
the error and treat the request as unauthenticated (i.e., perform the same
redirect to the login URL rather than letting the middleware crash), leaving the
existing session.user check and NextResponse.redirect(loginUrl) behavior intact
to implement fail-closed protection for protected routes.
- Around line 13-25: The current getCachedSession uses hashString(cookies) to
build cacheKey which risks collisions; replace that by using the raw cookies
string or, better, a parsed session token/value (not a 32-bit hash) as the cache
key: update getCachedSession to compute cacheKey = cookies (or extract the
session token from cookies) and use that key when calling sessionCache.get/put;
remove or stop using hashString for cache keys and ensure sessionCache
lookups/store use the exact token/string to eliminate hash collisions.
---
Duplicate comments:
In `@frontend-v2/src/app/users/`[id]/page.tsx:
- Around line 23-27: Add a server-side prefetch for the chapter list so
UserForm's useQuery (queryKey: [API_PATH.CHAPTER_LIST]) is hydrated and avoids a
client-side load/flash: call queryClient.prefetchQuery with queryKey
[API_PATH.CHAPTER_LIST] and queryFn invoking the same API client method UserForm
uses (e.g., apiClient.getChapterList or apiClient.getChapters), placed alongside
the existing prefetchQuery for users in the page component so the chapter
dropdown is populated on render.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
frontend-v2/src/app/authed-page-layout.tsxfrontend-v2/src/app/coaching/[id]/page.tsxfrontend-v2/src/app/event/[id]/page.tsxfrontend-v2/src/app/event/page.tsxfrontend-v2/src/app/interest/generator/page.tsxfrontend-v2/src/app/users/[id]/page.tsxfrontend-v2/src/app/users/new/page.tsxfrontend-v2/src/app/users/page.tsxfrontend-v2/src/app/users/user-form.tsxfrontend-v2/src/lib/server-user.tsfrontend-v2/src/middleware.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
frontend-v2/src/lib/server-user.ts (1)
18-18:⚠️ Potential issue | 🟠 MajorGuard header deserialization to avoid auth-path 500s.
Line 18 parses untrusted header text without error handling. A malformed payload will throw and fail the request instead of giving a controlled auth failure.
🔧 Suggested hardening
export async function getServerUser(): Promise<User> { const headersList = await headers() const userHeader = headersList.get(SERVER_USER_HEADER) if (!userHeader) { throw new Error('User not found in headers.') } - return JSON.parse(userHeader) as User + try { + return JSON.parse(userHeader) as User + } catch { + throw new Error('Invalid user header payload.') + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/lib/server-user.ts` at line 18, The code directly JSON.parse(userHeader) (in the routine that returns a User) can throw on malformed input; wrap the parse in a try/catch and handle failures by returning a safe unauthenticated result (e.g., null/undefined) or throwing a controlled auth error instead of letting a 500 bubble up; validate the parsed object shape before casting to User (or use your existing validator/zod schema) so references to userHeader/JSON.parse are guarded and malformed headers produce a controlled auth failure.frontend-v2/src/middleware.ts (3)
72-75:⚠️ Potential issue | 🟠 MajorAvoid forwarding full user objects in request headers.
Line 74 serializes the full user payload into headers, which increases PII exposure and header-size risk. Forward only minimal claims required downstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/middleware.ts` around lines 72 - 75, The middleware currently serializes the entire session.user into request headers (requestHeaders.set(SERVER_USER_HEADER, JSON.stringify(session.user))), which exposes PII and risks oversized headers; instead, create and serialize a minimal claims object (e.g., {id, role, emailVerified} or whatever downstream needs) and set that on SERVER_USER_HEADER, or better yet set a user ID-only header and let server components fetch full profile when necessary; update the code around requestHeaders, SERVER_USER_HEADER, and any downstream code expecting the full user to consume the new minimal claims shape.
65-65:⚠️ Potential issue | 🔴 CriticalHandle session-fetch failures with fail-closed behavior.
Line 65 can throw on upstream/network failures and currently bubbles out of middleware. Catch and redirect to login to avoid broad request failures.
🔧 Fail-closed handling
- const session = await getCachedSession(cookieHeader) + let session: Awaited<ReturnType<typeof getCachedSession>> + try { + session = await getCachedSession(cookieHeader) + } catch { + const loginUrl = new URL('/login', request.url) + return NextResponse.redirect(loginUrl) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/middleware.ts` at line 65, Wrap the await getCachedSession(cookieHeader) call inside a try/catch in the middleware so upstream/network failures are handled fail-closed; on any exception from getCachedSession, treat it as a missing/invalid session and redirect to the login route (or return the same response path you use for unauthenticated sessions) instead of letting the error bubble—adjust the logic that reads the session variable (named session) downstream to assume a redirect has been issued on error.
25-27:⚠️ Potential issue | 🟠 MajorCache key strategy is unsafe for auth identity mapping.
Using
hashString(cookies)as the key can collide across users and also changes when unrelated cookies change. Key this cache by the actual auth session token value instead of a hash of the whole cookie header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/middleware.ts` around lines 25 - 27, getCachedSession currently builds cacheKey with hashString(cookies), which can collide and changes when unrelated cookies change; instead extract the actual auth session token (e.g., the session cookie or Authorization bearer token) and use that token value as the cache key. Update getCachedSession to parse the cookies/header to locate the specific session token name used by your auth system, use that token as the key when accessing sessionCache.get/set, and remove reliance on hashString(cookies); keep sessionCache and getCachedSession function names intact so callers remain unchanged.
🤖 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-v2/src/middleware.ts`:
- Line 59: The current public-route check using pathname.startsWith('/login') ||
pathname.startsWith('/auth') is too broad and can match unintended routes;
update the check in middleware.ts (the code that inspects the pathname variable)
to match entire path segments instead—either replace the startsWith checks with
an exact equality for single-segment paths (e.g., pathname === '/login' or
pathname === '/auth') or use a segment-aware test/regex that ensures '/login' or
'/auth' is followed only by end-of-path or a slash (e.g.,
/^\/(login|auth)(\/|$)/.test(pathname)); adjust the middleware branch that
currently uses pathname.startsWith to this safer match so only intended public
auth routes are allowed.
---
Duplicate comments:
In `@frontend-v2/src/lib/server-user.ts`:
- Line 18: The code directly JSON.parse(userHeader) (in the routine that returns
a User) can throw on malformed input; wrap the parse in a try/catch and handle
failures by returning a safe unauthenticated result (e.g., null/undefined) or
throwing a controlled auth error instead of letting a 500 bubble up; validate
the parsed object shape before casting to User (or use your existing
validator/zod schema) so references to userHeader/JSON.parse are guarded and
malformed headers produce a controlled auth failure.
In `@frontend-v2/src/middleware.ts`:
- Around line 72-75: The middleware currently serializes the entire session.user
into request headers (requestHeaders.set(SERVER_USER_HEADER,
JSON.stringify(session.user))), which exposes PII and risks oversized headers;
instead, create and serialize a minimal claims object (e.g., {id, role,
emailVerified} or whatever downstream needs) and set that on SERVER_USER_HEADER,
or better yet set a user ID-only header and let server components fetch full
profile when necessary; update the code around requestHeaders,
SERVER_USER_HEADER, and any downstream code expecting the full user to consume
the new minimal claims shape.
- Line 65: Wrap the await getCachedSession(cookieHeader) call inside a try/catch
in the middleware so upstream/network failures are handled fail-closed; on any
exception from getCachedSession, treat it as a missing/invalid session and
redirect to the login route (or return the same response path you use for
unauthenticated sessions) instead of letting the error bubble—adjust the logic
that reads the session variable (named session) downstream to assume a redirect
has been issued on error.
- Around line 25-27: getCachedSession currently builds cacheKey with
hashString(cookies), which can collide and changes when unrelated cookies
change; instead extract the actual auth session token (e.g., the session cookie
or Authorization bearer token) and use that token value as the cache key. Update
getCachedSession to parse the cookies/header to locate the specific session
token name used by your auth system, use that token as the key when accessing
sessionCache.get/set, and remove reliance on hashString(cookies); keep
sessionCache and getCachedSession function names intact so callers remain
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend-v2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
frontend-v2/package.jsonfrontend-v2/src/app/users/[id]/page.tsxfrontend-v2/src/lib/server-user.tsfrontend-v2/src/middleware.ts
There was a problem hiding this comment.
♻️ Duplicate comments (5)
frontend-v2/src/app/event/[id]/page.tsx (1)
20-30:⚠️ Potential issue | 🟠 MajorReject non-canonical route IDs before prefetch.
Line 20 uses
parseInt(id), which accepts partial values (e.g.,"12abc"), so invalid routes can resolve to a real event ID. Validate the rawidstring first and only then convert to number.🔧 Proposed fix
- const eventId = parseInt(id) - if (Number.isNaN(eventId)) { + if (!/^[1-9]\d*$/.test(id)) { notFound() } + const eventId = Number(id) @@ - queryKey: [API_PATH.EVENT_GET, String(eventId)], + queryKey: [API_PATH.EVENT_GET, id], queryFn: () => apiClient.getEvent(eventId),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/app/event/`[id]/page.tsx around lines 20 - 30, The route accepts partial numeric strings because parseInt(id) is used; validate the raw id string `id` with a strict numeric check (e.g. match /^\d+$/) before converting to a number, and call `notFound()` immediately for non-matching values; then safely convert to a Number for `eventId` and continue to use `queryClient.prefetchQuery` with the `queryKey` ([API_PATH.EVENT_GET, String(eventId)]) and `queryFn` (apiClient.getEvent(eventId)).frontend-v2/src/middleware.ts (2)
59-60:⚠️ Potential issue | 🟡 MinorMake public-route matching segment-aware.
Line 59 uses broad prefix checks; paths like
/authxyzor/login-oldare treated as public. Match exact segment boundaries to avoid accidental auth bypass.🔧 Proposed fix
- if (pathname.startsWith('/login') || pathname.startsWith('/auth')) { + const isPublicRoute = + pathname === '/login' || + pathname.startsWith('/login/') || + pathname === '/auth' || + pathname.startsWith('/auth/') + + if (isPublicRoute) { return NextResponse.next() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/middleware.ts` around lines 59 - 60, The current middleware conditional uses pathname.startsWith('/login') and pathname.startsWith('/auth') which treats routes like '/login-old' or '/authxyz' as public; update the check around the pathname variable (the if that returns NextResponse.next()) to perform segment-aware matching instead — e.g., test that pathname is exactly '/login' or '/auth' or begins with '/login/' or '/auth/' (or use an equivalent regex like ^/login(/|$) and ^/auth(/|$)) so only the exact segment and its subpaths are allowed as public routes.
15-27:⚠️ Potential issue | 🟠 MajorUse collision-resistant cache keys for session entries.
Line 26 compresses the cookie string into a 32-bit-style hash key; collisions can map different users to the same cache entry and return the wrong
session.user.🔧 Proposed fix
-// Simple hash function for cookie string (for cache key) -function hashString(str: string): string { - let hash = 0 - for (let i = 0; i < str.length; i++) { - const char = str.charCodeAt(i) - hash = (hash << 5) - hash + char - hash = hash & hash // Convert to 32-bit integer - } - return hash.toString(36) -} - async function getCachedSession(cookies: string) { - const cacheKey = hashString(cookies) + const cacheKey = cookies const cached = sessionCache.get(cacheKey)Also applies to: 37-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/middleware.ts` around lines 15 - 27, The current hashString produces a weak 32-bit hash causing cache-key collisions for sessionCache used in getCachedSession; replace hashString with a collision-resistant digest (e.g., use a SHA-256/sha512 hex or base64 HMAC over the full cookie string) and use that digest as the cacheKey, updating any other places that call hashString (including the other occurrence around lines 37-41) to use the new secure hashing function; ensure the implementation uses a cryptographic library (Node's crypto or Web Crypto) and returns a stable string key for sessionCache lookups.frontend-v2/src/app/users/[id]/page.tsx (1)
20-23:⚠️ Potential issue | 🟠 MajorHarden ID parsing to prevent partial-string coercion.
Line 20 accepts values like
"42abc"viaparseInt, which can load an unintended user record. Validateidas a canonical positive integer string before conversion.🔧 Proposed fix
- const userId = parseInt(id) - if (Number.isNaN(userId)) { + if (!/^[1-9]\d*$/.test(id)) { notFound() } + const userId = Number(id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/app/users/`[id]/page.tsx around lines 20 - 23, The current parseInt(userId) allows partial-string coercion (e.g., "42abc"); update the validation in the users/[id]/page.tsx route to first verify that the incoming id string matches a canonical positive integer pattern (e.g., a regex that allows only digits and optionally disallowing leading zero/zero if you require >0) before converting; if the id fails the regex or the parsed numeric value is not a safe positive integer, call notFound(); key symbols to modify are the id parameter handling, the userId variable assignment (where parseInt(id) is used), and the notFound() branch.frontend-v2/src/app/coaching/[id]/page.tsx (1)
20-31:⚠️ Potential issue | 🟠 MajorValidate route IDs strictly before fetching event data.
Line 20 uses
parseInt, which can coerce malformed IDs (e.g.,"9foo"). This can fetch the wrong event and normalize keys unexpectedly. Validate the raw param first, then parse.🔧 Proposed fix
- const eventId = parseInt(id) - if (Number.isNaN(eventId)) { + if (!/^[1-9]\d*$/.test(id)) { notFound() } + const eventId = Number(id) @@ - queryKey: [API_PATH.EVENT_GET, String(eventId)], + queryKey: [API_PATH.EVENT_GET, id], queryFn: () => apiClient.getEvent(eventId),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/app/coaching/`[id]/page.tsx around lines 20 - 31, The route id is being loosely parsed with parseInt (variable eventId), allowing inputs like "9foo" to become 9; first validate the raw id param strictly (e.g., ensure id is a string of only digits via /^\d+$/ or equivalent) and call notFound() if it fails, then safely parse to eventId and proceed to create ApiClient/QueryClient and call queryClient.prefetchQuery (queryKey [API_PATH.EVENT_GET, String(eventId)] with queryFn apiClient.getEvent(eventId)); update the validation around the id before any use to prevent malformed IDs from fetching the wrong event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend-v2/src/app/coaching/`[id]/page.tsx:
- Around line 20-31: The route id is being loosely parsed with parseInt
(variable eventId), allowing inputs like "9foo" to become 9; first validate the
raw id param strictly (e.g., ensure id is a string of only digits via /^\d+$/ or
equivalent) and call notFound() if it fails, then safely parse to eventId and
proceed to create ApiClient/QueryClient and call queryClient.prefetchQuery
(queryKey [API_PATH.EVENT_GET, String(eventId)] with queryFn
apiClient.getEvent(eventId)); update the validation around the id before any use
to prevent malformed IDs from fetching the wrong event.
In `@frontend-v2/src/app/event/`[id]/page.tsx:
- Around line 20-30: The route accepts partial numeric strings because
parseInt(id) is used; validate the raw id string `id` with a strict numeric
check (e.g. match /^\d+$/) before converting to a number, and call `notFound()`
immediately for non-matching values; then safely convert to a Number for
`eventId` and continue to use `queryClient.prefetchQuery` with the `queryKey`
([API_PATH.EVENT_GET, String(eventId)]) and `queryFn`
(apiClient.getEvent(eventId)).
In `@frontend-v2/src/app/users/`[id]/page.tsx:
- Around line 20-23: The current parseInt(userId) allows partial-string coercion
(e.g., "42abc"); update the validation in the users/[id]/page.tsx route to first
verify that the incoming id string matches a canonical positive integer pattern
(e.g., a regex that allows only digits and optionally disallowing leading
zero/zero if you require >0) before converting; if the id fails the regex or the
parsed numeric value is not a safe positive integer, call notFound(); key
symbols to modify are the id parameter handling, the userId variable assignment
(where parseInt(id) is used), and the notFound() branch.
In `@frontend-v2/src/middleware.ts`:
- Around line 59-60: The current middleware conditional uses
pathname.startsWith('/login') and pathname.startsWith('/auth') which treats
routes like '/login-old' or '/authxyz' as public; update the check around the
pathname variable (the if that returns NextResponse.next()) to perform
segment-aware matching instead — e.g., test that pathname is exactly '/login' or
'/auth' or begins with '/login/' or '/auth/' (or use an equivalent regex like
^/login(/|$) and ^/auth(/|$)) so only the exact segment and its subpaths are
allowed as public routes.
- Around line 15-27: The current hashString produces a weak 32-bit hash causing
cache-key collisions for sessionCache used in getCachedSession; replace
hashString with a collision-resistant digest (e.g., use a SHA-256/sha512 hex or
base64 HMAC over the full cookie string) and use that digest as the cacheKey,
updating any other places that call hashString (including the other occurrence
around lines 37-41) to use the new secure hashing function; ensure the
implementation uses a cryptographic library (Node's crypto or Web Crypto) and
returns a stable string key for sessionCache lookups.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
frontend-v2/src/app/coaching/[id]/page.tsxfrontend-v2/src/app/event/[id]/page.tsxfrontend-v2/src/app/users/[id]/page.tsxfrontend-v2/src/lib/server-user.tsfrontend-v2/src/middleware.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-v2/src/middleware.ts`:
- Around line 43-49: The eager full-scan cleanup that iterates
sessionCache.entries() on every cache set (using cutoff = now - CACHE_TTL and
deleting entries whose value.timestamp < cutoff) should be removed or made
probabilistic to avoid O(n) work on each write; update the cache-set path that
performs this loop to instead either drop the loop entirely (relying on
QuickLRU's eviction and the existing TTL read check) or gate the cleanup behind
a low-probability condition (e.g., run only when Math.random() < 0.01) so
expired entries are cleaned infrequently; keep references to sessionCache,
CACHE_TTL, now and the value.timestamp check when editing the code.
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
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.
Performance: Fix slow navigation by implementing middleware-based auth
Problem
Page navigation was taking 500ms-2s because we were hitting the
/user/meAPI endpoint on every single route change. Every page was callingfetchSession()independently, which meant auth overhead on every click.Solution
Implemented proper Next.js App Router authentication pattern using middleware with session caching:
New middleware layer (
middleware.ts):New server helper (
server-user.ts):fetchSession()calls across the appConverted pages to server components:
HydrationBoundaryImpact
/user/mecalls)Other fixes
Summary by CodeRabbit
Bug Fixes
Performance
Chores