-
Notifications
You must be signed in to change notification settings - Fork 451
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_middleware
tomiddleware
in 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_middleware
export entirely from client-side route modulesapp/routes/_auth+/onboarding.tsx
andapp/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.$queryRawUnsafe
call for searching users with a type-safeprisma.$queryRawTyped(searchUsers(like))
. I also added an import forsearchUsers
from@prisma/client/sql
to leverage this new typed query functionality, improving query safety and maintainability. - Image Source Type Refinement: In
app/routes/users+/index.tsx
, I simplified thegetUserImgSrc
call by removing the?? undefined
fallback foruser.imageObjectKey
, as the type system now correctly handlesnull
values 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.0
and implements its new middleware feature for authentication. This change centralizesrequireUser
andrequireAnonymous
checks into dedicated middleware functions, replacing scattered direct calls in various routes.Key changes include:
@react-router/*
andreact-router
packages.v8_middleware
feature flag.app/middleware.server.ts
withrequireUserMiddleware
andrequireAnonymousMiddleware
.settings+/profile.tsx
to userequireUserMiddleware
._auth+/login.tsx
,_auth+/signup.tsx
,_auth+/onboarding.tsx
,_auth+/onboarding_.$provider.tsx
, and_auth+/reset-password.tsx
to userequireAnonymousMiddleware
.server/index.ts
to provide theRouterContextProvider
for middleware context._seo+/sitemap[.]xml.ts
to read the build from the new context._auth+/auth.$provider.callback.test.ts
to pass the correct context type.users+/index.tsx
.Test Plan
/login
and successfully log in./settings/profile
(should be accessible)./signup
and successfully create a new account, completing the onboarding flow./settings/profile
(should be accessible)./settings/profile
. You should be redirected to/login
./login
or/signup
. You should be redirected to/
./sitemap.xml
and ensure it loads without errors.npm test
to ensure all existing tests pass.Checklist
Screenshots