refactor: simplify redundant code across routes, types, and config#136
refactor: simplify redundant code across routes, types, and config#136
Conversation
- Extract requireUser auth middleware to eliminate 7 duplicated auth guard blocks - Tighten User type (email/name required), removing 10 defensive fallbacks - Move drizzle-orm from devDependencies to dependencies (runtime dep) - Extract shared DB config to deduplicate db/index.ts and drizzle.config.ts - Simplify messages query from nested subquery to single query + reverse() - Remove ws import used only for WebSocket.OPEN constant
There was a problem hiding this comment.
Pull request overview
Refactors the Hono backend to reduce duplicated auth/DB configuration logic, tighten session User typing, and simplify message fetching—aiming to make runtime dependencies and route code more consistent across the app.
Changes:
- Introduces a shared
requireUsermiddleware and updates routes to use it instead of repeated session guards. - Centralizes Postgres credentials in
hono/db/config.tsand reuses them for both runtime DB and Drizzle Kit config. - Simplifies
/api/messagesretrieval logic and adjusts tests/mocks accordingly; movesdrizzle-ormto runtime dependencies.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Moves drizzle-orm to dependencies so production installs include runtime ORM. |
| hono/types.ts | Makes User.email/User.name required and adds user to Hono context variables. |
| hono/auth/requireUser.ts | Adds shared auth middleware that reads session user and sets c.set("user", ...). |
| hono/routes/auth.ts | Adjusts Google OAuth session user shaping to match tightened User typing. |
| hono/routes/channels.ts | Replaces repeated auth guard blocks with requireUser and uses c.get("user"). |
| hono/routes/messages.ts | Uses requireUser and simplifies message query to single query + reverse. |
| hono/routes/messages.test.ts | Updates DB mock to match new messages query shape. |
| hono/routes/index.tsx | Removes ` |
| hono/routes/ws.ts | Removes ws dependency usage for OPEN constant and removes unknown fallbacks. |
| hono/db/config.ts | Introduces shared DB credential object. |
| hono/db/index.ts | Uses shared dbCredentials when creating the pg Pool. |
| drizzle.config.ts | Reuses shared dbCredentials for Drizzle Kit config. |
| export const requireUser = createMiddleware<{ Variables: Variables }>(async (c, next) => { | ||
| const session = c.get("session"); | ||
| const user = session.get("user") as User | undefined; | ||
|
|
||
| if (!user) { | ||
| return c.json({ error: "Unauthorized" }, 401); | ||
| } | ||
|
|
||
| c.set("user", user); | ||
| await next(); |
There was a problem hiding this comment.
requireUser only checks that a value exists in session, but it doesn’t validate that the stored object actually contains a non-empty email and name (which are now required by the User type). A session containing { email: "", name: "" } (or a legacy session missing fields) will be treated as authenticated and then used as an identity in DB writes. Consider validating user.email/user.name (non-empty strings) and rejecting/clearing the session when invalid before calling c.set("user", user).
|
|
||
| export type Variables = { | ||
| session: Session; | ||
| user: User; |
There was a problem hiding this comment.
Making Variables.user required implies c.get("user") is always safe across the entire app, but the app only installs session middleware globally and does not set user on every request. This makes the context typing unsound and can hide bugs in routes that forget to run requireUser. Consider changing user to optional in Variables (e.g., user?: User) and then narrowing it in handlers via requireUser (or defining a separate AuthedVariables type for routes that require a user).
| user: User; | |
| user?: User; |
| index.get("/", async (c) => { | ||
| const session = c.get("session"); | ||
| const user = session.get("user") as User | undefined; | ||
|
|
||
| if (!user) { | ||
| return c.redirect("/auth/login"); | ||
| } | ||
|
|
||
| const protoHeader = c.req.header("x-forwarded-proto"); | ||
| const protocol = protoHeader === "https" ? "wss:" : "ws:"; | ||
| const url = new URL(c.req.url); | ||
| const wsUrl = `${protocol}//${url.host}/ws`; | ||
|
|
||
| try { | ||
| const [generalChannel] = await db.select().from(channels).where(eq(channels.name, "general")); | ||
|
|
||
| if (generalChannel) { | ||
| const existingMembership = await db | ||
| .select() | ||
| .from(channelMembers) | ||
| .where( | ||
| and(eq(channelMembers.channelId, generalChannel.id), eq(channelMembers.userEmail, user.email || "unknown")), | ||
| ); | ||
| .where(and(eq(channelMembers.channelId, generalChannel.id), eq(channelMembers.userEmail, user.email))); | ||
|
|
||
| if (existingMembership.length === 0) { | ||
| await db.insert(channelMembers).values({ | ||
| channelId: generalChannel.id, | ||
| userEmail: user.email || "unknown", | ||
| userEmail: user.email, | ||
| }); |
There was a problem hiding this comment.
This route now relies on user.email with no fallback, but it still only checks if (!user) after reading from session. If the session contains an invalid/legacy user object (missing email or empty string), the auto-join logic will insert/select memberships using an invalid identity. Consider validating user.email/user.name here (or centralizing validation in shared middleware) and redirecting to login / clearing the session if the session user is malformed.
| onOpen: (_evt: Event, ws: WSContext) => { | ||
| if (!user) { | ||
| ws.close(1008, "Unauthorized"); | ||
| return; | ||
| } | ||
| clients.set(ws, { userEmail: user.email || "unknown" }); | ||
| clients.set(ws, { userEmail: user.email }); | ||
| }, |
There was a problem hiding this comment.
onOpen only checks !user, but after removing the || "unknown" fallback the code assumes user.email/user.name are always valid strings. If the session contains an invalid user object (e.g., empty email/name), the connection will be tracked under an empty key and DB inserts/broadcast filtering will behave incorrectly. Consider validating user.email/user.name in onOpen (and closing with 1008) before storing client info.
* Initial plan * fix: update pnpm-lock.yaml to include drizzle-orm dependency Agent-Logs-Url: https://github.com/mahata/mlack/sessions/6791b2bb-061c-4f6a-b685-3b3ee367c09d Co-authored-by: mahata <23497+mahata@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mahata <23497+mahata@users.noreply.github.com>
Summary
requireUserauth middleware — eliminates 7 copy-pasted auth guard blocks acrosschannels.ts,messages.ts,ws.ts, andindex.tsxUsertype —emailandnameare now required fields, removing all 10|| "unknown"defensive fallbacks that could silently store bad datadrizzle-ormtodependencies— it's imported in 6 runtime files; a production install (--prod) would have failedhono/db/config.ts) — deduplicates identical 6-line credential blocks indb/index.tsanddrizzle.config.ts.reverse()in JS, removing the unusedascimportwsimport inws.ts— was imported only for theWebSocket.OPENconstant (value1), replaced with a local constantNet result: 12 files changed, 80 insertions, 113 deletions. All 61 tests pass, lint clean, build succeeds.