-
Notifications
You must be signed in to change notification settings - Fork 459
Upgrade react router and add auth middleware #1040
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
base: main
Are you sure you want to change the base?
Upgrade react router and add auth middleware #1040
Conversation
This commit refactors authentication checks to use react-router's middleware system. This centralizes auth logic and improves code organization. Co-authored-by: me <[email protected]>
|
Cursor Agent can help with this pull request. Just |
|
@cursor, the CI build has an error. Please fix this: ✗ Build failed in 1.40s React Router automatically removes server-code from these exports: But other route exports in 'app/routes/settings+/profile.tsx' depend on '#app/middleware.server.ts'. See https://remix.run/docs/en/main/guides/vite#splitting-up-client-and-server-code |
app/routes/_auth+/login.tsx
Outdated
| import { StatusButton } from '#app/components/ui/status-button.tsx' | ||
| import { login, requireAnonymous } from '#app/utils/auth.server.ts' | ||
| import { login } from '#app/utils/auth.server.ts' | ||
| export const unstable_middleware = [ |
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.
@cursor, I don't think this is unstable. Should we remove the prefix?
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.
Co-authored-by: me <[email protected]>
app/routes/users+/index.tsx
Outdated
| const users = await prisma.$queryRawUnsafe<{ id: string; username: string; name: string | null; imageObjectKey: string | null }[]>( | ||
| `SELECT id, username, name, (SELECT "objectKey" FROM "Image" WHERE "userId" = "User"."id" LIMIT 1) as "imageObjectKey" FROM "User" WHERE username ILIKE $1 OR name ILIKE $1 ORDER BY username ASC`, | ||
| like, | ||
| ) |
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.
@cursor, don't do this. Instead, run the setup script which will generate the prisma typed sql. This change should be unnecessary once you do that.
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.
I updated the application to align with recent framework changes by migrating middleware exports from unstable_middleware to middleware across several route modules. Additionally, I enhanced data access safety by replacing an unsafe raw SQL query with a typed Prisma SQL function for user searches.
- Middleware API Update: I updated the middleware export name from
unstable_middlewaretomiddlewarein several server-side route modules, includingapp/routes/_auth+/login.server.ts,app/routes/_auth+/onboarding.server.ts,app/routes/_auth+/onboarding_.$provider.server.ts,app/routes/_auth+/reset-password.server.ts,app/routes/_auth+/signup.server.ts, andapp/routes/settings+/profile.server.ts. - Removed Client-Side Middleware: I removed the
unstable_middlewareexport entirely from client-side route modulesapp/routes/_auth+/onboarding.tsxandapp/routes/_auth+/reset-password.tsx, as middleware is typically a server-side concept. - Typed Prisma SQL Query: In
app/routes/users+/index.tsx, I replaced theprisma.$queryRawUnsafecall for searching users with a type-safeprisma.$queryRawTyped(searchUsers(like)). I also added an import forsearchUsersfrom@prisma/client/sqlto leverage this new typed query functionality, improving query safety and maintainability. - Image Source Type Refinement: In
app/routes/users+/index.tsx, I simplified thegetUserImgSrccall by removing the?? undefinedfallback foruser.imageObjectKey, as the type system now correctly handlesnullvalues for the image object key.
Learn more about Cursor Agents
Co-authored-by: me <[email protected]>
Refactor: Use `prisma.$queryRawTyped` for user search Co-authored-by: me <[email protected]>
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.
@cursor Could you make sure to run linting with --fix, formatting, type checking, testing, and the build?
|
I performed a series of refactors primarily focused on enhancing code readability and consistency across the codebase. This involved reordering imports, standardizing formatting for function calls and complex types, and cleaning up documentation to improve clarity and maintainability.
6 tasks completed
Learn more about Cursor Agents |
This commit refactors middleware and context usage for better code organization and clarity. It also includes minor updates to documentation and tests. Co-authored-by: me <[email protected]>
|
@cursor, several of the playwright tests are failing in CI with the following output. Please fix: 8 failed |
|
I refactored the
Learn more about Cursor Agents |
| }) => { | ||
| const cookie = request.headers.get('cookie') | ||
| const session = await authSessionStorage.getSession(cookie) | ||
| const sessionId = session.get('sessionId') as string | undefined |
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.
Bug: Inconsistent Session Key Causes Authentication Issues
The middleware functions in app/middleware.server.ts use a hardcoded 'sessionId' string for session retrieval. This is inconsistent with the sessionKey constant used throughout the rest of the authentication system, causing authentication failures and incorrect route protection.
| export const serverBuildContext = createContext<Promise<{ | ||
| error: unknown | ||
| build: ServerBuild | ||
| }> | null>(null) |
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.
Bug: Sitemap Route Fails to Access Server Build Context
The sitemap[.]xml.ts route defines its own serverBuildContext instead of importing the one created in server/index.ts. React Router contexts rely on object identity, so this separate context instance prevents the sitemap from accessing the serverBuild data set by the server.
Co-authored-by: me <[email protected]>
|
|
||
| export async function loader({ request, context }: Route.LoaderArgs) { | ||
| const serverBuild = (await context.serverBuild) as { build: ServerBuild } | ||
| const serverBuild = (await (context as Readonly<RouterContextProvider>).get( |
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.
Does this actually work?
I think the routes can accessed like this if I'm not mistaken:
const { routes } = await import('virtual:react-router/server-build')
The loadContext can be ditched completely.
| )) as { build: ServerBuild } | ||
|
|
||
| // TODO: This is typeerror is coming up since of the remix-run/server-runtime package. We might need to remove/update that one. | ||
| // @ts-expect-error |
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.
Could use this modified version https://gist.github.com/hilja/5e6f6ec6a86a3e06113501f8d60e20a4
|
@cursor, could you review the comments and try to get things working. Don't worry about the modified seo suggestion. We can look into that in another PR. |
|
Background Agent has finished! You can now access it through Cursor. Learn more about Cursor Agents |
Summary
This PR upgrades React Router to version
7.9.0and implements its new middleware feature for authentication. This change centralizesrequireUserandrequireAnonymouschecks into dedicated middleware functions, replacing scattered direct calls in various routes.Key changes include:
@react-router/*andreact-routerpackages.v8_middlewarefeature flag.app/middleware.server.tswithrequireUserMiddlewareandrequireAnonymousMiddleware.settings+/profile.tsxto userequireUserMiddleware._auth+/login.tsx,_auth+/signup.tsx,_auth+/onboarding.tsx,_auth+/onboarding_.$provider.tsx, and_auth+/reset-password.tsxto userequireAnonymousMiddleware.server/index.tsto provide theRouterContextProviderfor middleware context._seo+/sitemap[.]xml.tsto read the build from the new context._auth+/auth.$provider.callback.test.tsto pass the correct context type.users+/index.tsx.Test Plan
/loginand successfully log in./settings/profile(should be accessible)./signupand successfully create a new account, completing the onboarding flow./settings/profile(should be accessible)./settings/profile. You should be redirected to/login./loginor/signup. You should be redirected to/./sitemap.xmland ensure it loads without errors.npm testto ensure all existing tests pass.Checklist
Screenshots