-
-
Notifications
You must be signed in to change notification settings - Fork 194
refactor: moved the oauth to useSession #867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: moved the oauth to useSession #867
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR centralises server-side session handling by adding Possibly related PRs
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/atproto/oauth.ts (1)
39-43:⚠️ Potential issue | 🟡 MinorMissing generic type parameter on
SessionManager.Line 42 declares
serverSession: SessionManagerwithout the generic type parameter. For type safety and consistency with the rest of the codebase (e.g.,SessionManager<UserServerSession>in the storage classes), this should be typed explicitly.🔧 Proposed fix
+import type { UserServerSession } from '#shared/types/userSession' + type EventHandlerWithOAuthSession<T extends EventHandlerRequest, D> = ( event: H3Event<T>, session: OAuthSession | undefined, - serverSession: SessionManager, + serverSession: SessionManager<UserServerSession>, ) => Promise<D>
🧹 Nitpick comments (2)
shared/types/userSession.ts (1)
1-14: Consider optional properties instead of| undefined.This reads more idiomatically and avoids explicit
undefinedassignments.Suggested refactor
- oauthSession: NodeSavedSession | undefined - oauthState: NodeSavedState | undefined + oauthSession?: NodeSavedSession + oauthState?: NodeSavedStateserver/utils/atproto/oauth.ts (1)
45-72: RedundantuseServerSessioncalls for the same request.
useServerSession(event)is called twice per request: once ingetOAuthSession(line 47) and again ineventHandlerWithOAuthSession(line 68). Although h3'suseSessiontypically caches by event, this pattern is unclear and potentially inefficient.Consider passing
serverSessionas a parameter togetOAuthSessioninstead of having it fetch its own.♻️ Proposed refactor
-async function getOAuthSession(event: H3Event): Promise<OAuthSession | undefined> { +async function getOAuthSession( + event: H3Event, + serverSession: SessionManager<UserServerSession>, +): Promise<OAuthSession | undefined> { const clientMetadata = getOauthClientMetadata() - const serverSession = await useServerSession(event) const { stateStore, sessionStore } = useOAuthStorage(serverSession) const client = new NodeOAuthClient({ @@ export function eventHandlerWithOAuthSession return defineEventHandler(async event => { const serverSession = await useServerSession(event) - const oAuthSession = await getOAuthSession(event) + const oAuthSession = await getOAuthSession(event, serverSession) return await handler(event, oAuthSession, serverSession) }) }
| // Even tho the signOut also clears part of the server cache should be done in order | ||
| // to let the oAuth package do any other clean up it may need | ||
| await oAuthSession?.signOut() | ||
| await serverSession.clear() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure server session is cleared even when sign‑out fails.
If signOut() throws, serverSession.clear() will never run, leaving stale session state.
Proposed fix
- await oAuthSession?.signOut()
- await serverSession.clear()
+ try {
+ await oAuthSession?.signOut()
+ } finally {
+ await serverSession.clear()
+ }| import { PublicUserSessionSchema } from '#shared/schemas/publicUserSession' | ||
| import { safeParse } from 'valibot' | ||
|
|
||
| export default eventHandlerWithOAuthSession(async (event, oAuthSession, serverSession) => { | ||
| const result = safeParse(UserSessionSchema, serverSession.data) | ||
| const result = safeParse(PublicUserSessionSchema, serverSession.data.public) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against missing session data before validation.
serverSession.data can be undefined; accessing .public will throw and bypass the intended validation.
Proposed fix
- const result = safeParse(PublicUserSessionSchema, serverSession.data.public)
+ const result = safeParse(PublicUserSessionSchema, serverSession.data?.public)📝 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.
| import { PublicUserSessionSchema } from '#shared/schemas/publicUserSession' | |
| import { safeParse } from 'valibot' | |
| export default eventHandlerWithOAuthSession(async (event, oAuthSession, serverSession) => { | |
| const result = safeParse(UserSessionSchema, serverSession.data) | |
| const result = safeParse(PublicUserSessionSchema, serverSession.data.public) | |
| import { PublicUserSessionSchema } from '#shared/schemas/publicUserSession' | |
| import { safeParse } from 'valibot' | |
| export default eventHandlerWithOAuthSession(async (event, oAuthSession, serverSession) => { | |
| const result = safeParse(PublicUserSessionSchema, serverSession.data?.public) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Moved away from using a cookie and tying the oauth session to a kv store and use
useSessionfor all store needsconcerns: