refactor: simplify session casting and improve type safety in MyMento…#3505
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces direct session type assertions in MyMentorshipPage with a typed Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3505 +/- ##
=======================================
Coverage 85.56% 85.57%
=======================================
Files 461 461
Lines 14227 14228 +1
Branches 1895 1895
=======================================
+ Hits 12174 12175 +1
Misses 1680 1680
Partials 373 373
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/app/my/mentorship/page.tsx`:
- Around line 86-88: The current loading gate uses only userName which conflates
a true loading state with unauthenticated users; update the component to
destructure status from useSession() (const { data: session, status } =
useSession()), stop using the single check if (!userName) to show
<LoadingSpinner />, and instead explicitly handle status === 'loading' (show
LoadingSpinner), status === 'unauthenticated' (perform a redirect to login or
render a clear unauthenticated message), and status === 'authenticated' (render
the normal UI using session/userName); adjust references to userName and
LoadingSpinner accordingly so unauthenticated users don’t see an indefinite
spinner.
| if (!userName) { | ||
| return <LoadingSpinner /> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for middleware files in the frontend app
echo "=== Looking for middleware files ==="
find frontend -name 'middleware.ts' -o -name 'middleware.js' 2>/dev/null
echo ""
echo "=== Checking root-level middleware ==="
find . -maxdepth 2 -name 'middleware.ts' -o -name 'middleware.js' 2>/dev/null | head -20
echo ""
echo "=== Directory structure of /my/ routes ==="
fd -t d 'my' frontend/src/app/ 2>/dev/null
echo ""
echo "=== Files in /my/ directory ==="
fd -t f frontend/src/app/my/ 2>/dev/null | head -20Repository: OWASP/Nest
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Check the actual page file to see the full implementation
echo "=== Content of frontend/src/app/my/mentorship/page.tsx ==="
cat -n frontend/src/app/my/mentorship/page.tsx | head -100
echo ""
echo "=== Checking for useSession and status usage in the file ==="
rg -n "useSession|status|redirect|authenticated" frontend/src/app/my/mentorship/page.tsxRepository: OWASP/Nest
Length of output: 4325
🏁 Script executed:
#!/bin/bash
# Check how other /my/ pages handle authentication
echo "=== Looking for other pages in /my/ directory ==="
fd -t f '.tsx$' frontend/src/app/my/ --exec basename {} \; | sort -u
echo ""
echo "=== Checking auth patterns in other /my/ pages ==="
fd -t f '.tsx$' frontend/src/app/my/ -exec sh -c 'echo "=== {} ===" && grep -n "useSession\|status\|redirect\|LoadingSpinner" {} | head -15' \;Repository: OWASP/Nest
Length of output: 397
🏁 Script executed:
#!/bin/bash
# Check if there's a layout file that might handle auth at the route level
echo "=== Checking for layout files in /my/ routes ==="
fd -t f 'layout.tsx' frontend/src/app/my/
echo ""
echo "=== Examining /my/ layout if it exists ==="
if [ -f "frontend/src/app/my/layout.tsx" ]; then
cat -n frontend/src/app/my/layout.tsx
fi
if [ -f "frontend/src/app/my/mentorship/layout.tsx" ]; then
echo "=== Examining /my/mentorship/ layout if it exists ==="
cat -n frontend/src/app/my/mentorship/layout.tsx
fiRepository: OWASP/Nest
Length of output: 146
Fix unauthenticated user handling with proper session status check.
Currently, the loading gate !userName cannot distinguish between the loading state (session data still loading) and the unauthenticated state (user not logged in). Since there's no middleware or layout-level route protection for /my/* routes, an unauthenticated user will see the spinner indefinitely.
Verify the session loading state by including status from useSession():
const { data: session, status } = useSession()Then handle all three states explicitly: loading, unauthenticated, and authenticated. For the unauthenticated case, redirect to login or show an appropriate message instead of displaying the spinner.
🤖 Prompt for AI Agents
In `@frontend/src/app/my/mentorship/page.tsx` around lines 86 - 88, The current
loading gate uses only userName which conflates a true loading state with
unauthenticated users; update the component to destructure status from
useSession() (const { data: session, status } = useSession()), stop using the
single check if (!userName) to show <LoadingSpinner />, and instead explicitly
handle status === 'loading' (show LoadingSpinner), status === 'unauthenticated'
(perform a redirect to login or render a clear unauthenticated message), and
status === 'authenticated' (render the normal UI using session/userName); adjust
references to userName and LoadingSpinner accordingly so unauthenticated users
don’t see an indefinite spinner.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/types/auth.ts`:
- Around line 10-14: The intersection on the user type is narrowing next-auth's
nullability for email and image; update the inline overrides in the user?:
Session['user'] & { ... } declaration so that email and image preserve
next-auth's possible null (e.g., use email?: string | null and image?: string |
null) or remove those re-declarations entirely and only add non-conflicting
fields like isLeader?: boolean, ensuring you keep the type name Session['user']
to locate the declaration.
| user?: Session['user'] & { | ||
| email?: string | ||
| expires?: string | ||
| image?: string | ||
| isLeader?: boolean |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/src/types/auth.tsRepository: OWASP/Nest
Length of output: 576
🌐 Web query:
next-auth v4.24.13 Session user email image type definitions TypeScript
💡 Result:
In next-auth v4.24.x (including v4.24.13), the default TypeScript types already define the session’s user as having (optional) name, email, and image:
Userincludes:name?: string | null,email?: string | null,image?: string | null(andid: string). [1]- To keep those defaults when you extend
Session.user, intersect withDefaultSession["user"]. [2]
Create a declaration file (e.g. types/next-auth.d.ts) and add:
import NextAuth, { DefaultSession } from "next-auth"
declare module "next-auth" {
interface Session {
user: {
// your custom fields here, e.g. id: string
} & DefaultSession["user"] // keeps name/email/image types
}
}This ensures session.user.email and session.user.image are typed as string | null | undefined (unless you further narrow them in your own app). [2]
Sources:
[1] next-auth v4.24.x docs “Types” (User/email/image)
[2] NextAuth.js TypeScript docs (module augmentation + DefaultSession["user"])
🏁 Script executed:
rg "session\.user\.email|session\.user\.image" --type ts --type tsx -A 2 -B 2Repository: OWASP/Nest
Length of output: 81
🏁 Script executed:
rg "ExtendedSession" --type ts --type tsx -A 3 | head -100Repository: OWASP/Nest
Length of output: 81
🏁 Script executed:
rg "session\.user\.email|session\.user\.image" -A 2 -B 2Repository: OWASP/Nest
Length of output: 36
🏁 Script executed:
rg "ExtendedSession" -A 3 | head -150Repository: OWASP/Nest
Length of output: 10757
🏁 Script executed:
fd "\.(ts|tsx)$" frontend/src --type f | head -20Repository: OWASP/Nest
Length of output: 933
🏁 Script executed:
rg "email" frontend/src --type-add 'ts:*.{ts,tsx}' -t ts | grep -i "session\|user\|email" | head -40Repository: OWASP/Nest
Length of output: 20371
🏁 Script executed:
rg "\.image" frontend/src --type-add 'ts:*.{ts,tsx}' -t ts | head -40Repository: OWASP/Nest
Length of output: 320
🏁 Script executed:
cat frontend/src/components/UserMenu.tsx | head -80Repository: OWASP/Nest
Length of output: 2804
🏁 Script executed:
rg "session\.user\." frontend/src/components/UserMenu.tsx -B 2 -A 2Repository: OWASP/Nest
Length of output: 36
Avoid narrowing email/image nullability from next-auth.
In next-auth v4.24.13, Session['user'] defines email and image as string | null. Redeclaring them as optional strings in the intersection intersects and narrows the type, removing null from the union. This masks the actual nullability from next-auth and can lead to unsafe assumptions about these fields.
🛠️ Proposed fix
user?: Session['user'] & {
- email?: string
+ email?: Session['user']['email']
expires?: string
- image?: string
+ image?: Session['user']['image']
isLeader?: boolean📝 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.
| user?: Session['user'] & { | |
| email?: string | |
| expires?: string | |
| image?: string | |
| isLeader?: boolean | |
| user?: Session['user'] & { | |
| email?: Session['user']['email'] | |
| expires?: string | |
| image?: Session['user']['image'] | |
| isLeader?: boolean |
🤖 Prompt for AI Agents
In `@frontend/src/types/auth.ts` around lines 10 - 14, The intersection on the
user type is narrowing next-auth's nullability for email and image; update the
inline overrides in the user?: Session['user'] & { ... } declaration so that
email and image preserve next-auth's possible null (e.g., use email?: string |
null and image?: string | null) or remove those re-declarations entirely and
only add non-conflicting fields like isLeader?: boolean, ensuring you keep the
type name Session['user'] to locate the declaration.
2 participants
Resolves #3501
This is the resolution:
https://sonarcloud.io/project/issues?open=AZihbH8RrMCFFhRwcFZN&id=OWASP_Nest
Checklist
make check-testlocally: all warnings addressed, tests passed