Annotate Server Helper Functions for Clarity#57
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR refactors API utility functions by removing legacy database operations (getDatabaseDumpTurso, pingDatabase) and introducing new team data-access helpers, permission checking, and logging utilities. Two new middleware functions are added to handle user session context and authentication enforcement, along with a development mode detection utility. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant setUserSessionContextMiddleware
participant Auth as Auth API
participant authenticatedMiddleware
participant Route as Route Handler
participant Logger as Logger
Client->>setUserSessionContextMiddleware: HTTP Request
setUserSessionContextMiddleware->>Auth: getSession(headers)
Auth-->>setUserSessionContextMiddleware: session/null
setUserSessionContextMiddleware->>setUserSessionContextMiddleware: Generate requestId
setUserSessionContextMiddleware->>setUserSessionContextMiddleware: Set Context values
setUserSessionContextMiddleware->>Logger: Log request path & user
setUserSessionContextMiddleware->>authenticatedMiddleware: next()
alt Public Route
authenticatedMiddleware->>Route: next()
Route-->>Client: Response
else Protected Route & Authenticated
authenticatedMiddleware->>authenticatedMiddleware: Verify (user, session)
authenticatedMiddleware->>Route: next()
Route-->>Client: Response
else Protected Route & Unauthenticated
authenticatedMiddleware->>Logger: Log unauthorized access
authenticatedMiddleware-->>Client: 401 NOT_AUTHENTICATED
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/api/src/lib/functions/middleware.ts (4)
70-75:⚠️ Potential issue | 🟡 MinorDebug
console.logstatements are a production artifact.Lines 72 and 74 log raw context values to stdout and are gated only by a TODO comment referencing issue
#26. These should be removed or at minimum guarded byisInDevMode()until the proper logging implementation is in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/functions/middleware.ts` around lines 70 - 75, In afterRouteLogicMiddleware, remove the two debug console.log statements that print c.get("teamId") (or at minimum wrap them in a dev-only check); update the function (and any temporary TODO) so production builds do not log raw context to stdout — locate afterRouteLogicMiddleware in middleware.ts and either delete the console.log calls or guard them behind isInDevMode() while preserving the await next() and existing TODO.
44-46:⚠️ Potential issue | 🟠 MajorOverly broad
startsWithcheck can inadvertently bypass authentication.
c.req.path.startsWith("/api/auth")will also match any future route whose path begins with/api/auth— such as/api/authorization,/api/authenticate, etc. — treating them as public even if they are meant to be protected.🛡️ Proposed fix — anchor to path-segment boundary
- const isPublicRoute = MIDDLEWARE_PUBLIC_ROUTES.some((route) => - c.req.path.startsWith(route), - ); + const isPublicRoute = MIDDLEWARE_PUBLIC_ROUTES.some((route) => + c.req.path === route || c.req.path.startsWith(route + "/"), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/functions/middleware.ts` around lines 44 - 46, The current isPublicRoute check uses c.req.path.startsWith(route) which can incorrectly mark routes like "/api/authorization" public when the intent was to only allow the exact segment "/api/auth"; update the matching logic used when computing isPublicRoute (and where MIDDLEWARE_PUBLIC_ROUTES is referenced) to only match whole path segments by anchoring to the segment boundary — e.g., treat a route as public if c.req.path === route or c.req.path.startsWith(route + "/") or use a regex equivalent that enforces ^route(\/|$) — so that only the exact segment or its subpaths are considered public and similarly-named routes remain protected.
23-23:⚠️ Potential issue | 🟠 Major
logInfois fire-and-forget — logs can be silently dropped in serverless runtimes.
logInfo(...)on line 23 (and again on line 53) is not awaited. In serverless environments (e.g., Cloudflare Workers), promises not tied to the response lifecycle can be terminated mid-flight before the DB insert completes, causing logs to vanish without error. The current design also means the initial request log never capturesuserIdorteamIdbecause those context values haven't been set yet at call time.If this is intentional fire-and-forget, consider using a platform-aware mechanism (e.g.,
c.executionCtx.waitUntil(...)on Workers) to ensure log durability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/functions/middleware.ts` at line 23, The request logging calls using logInfo (the initial call that logs `Middleware for request path ${c.req.path} for ${userString}` and the later call) are fire-and-forget and may be dropped in serverless runtimes; update the middleware to either await the returned promise from logInfo or, in platform-specific code paths, schedule it with the execution lifecycle (e.g., use c.executionCtx.waitUntil(...) on Workers) to ensure durability, and move the initial log until after you set c.userId/c.teamId (or reconstruct userString) so the log includes userId/teamId; locate the logInfo calls in the middleware function and replace them with an awaited call or waitUntil scheduling as appropriate.
32-34:⚠️ Potential issue | 🟡 Minor
teamIdis not initialized for authenticated requests — inconsistent with the unauthenticated path.For unauthenticated requests (lines 25-30),
teamIdis explicitly set tonull. For authenticated requests,teamIdis never set, soc.get("teamId")returnsundefineduntil a route handler sets it. Any downstream code that checksc.get("teamId") === nullto mean "no team" will behave differently depending on auth state. Consider explicitly settingc.set("teamId", null)here (mirroring the unauthenticated branch) to establish a consistent baseline.📝 Proposed fix
c.set("user", session.user); c.set("session", session.session); + c.set("teamId", null); await next();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/functions/middleware.ts` around lines 32 - 34, Authenticated branch in middleware.ts never initializes teamId, causing inconsistent downstream behavior versus the unauthenticated path; add c.set("teamId", null) in the authenticated branch (alongside c.set("user", session.user) and c.set("session", session.session")) before calling await next() so handlers can reliably check c.get("teamId") === null as "no team".apps/api/src/lib/functions/database.ts (1)
29-42:⚠️ Potential issue | 🟡 MinorJSDoc return type is inaccurate —
.returning()returns an array.The doc says "Returns the teamId that was deleted", implying a scalar, but
db.delete(...).returning(...)resolves to an array of row objects ({ teamId: string }[]). Callers may be surprised if they expect a single value.📝 Suggested doc fix
- * `@returns` The teamId that was deleted + * `@returns` An array of objects containing the teamId for each deleted row🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/functions/database.ts` around lines 29 - 42, The JSDoc for leaveTeam is wrong: db.delete(...).returning(...) yields an array of row objects, not a single scalar; update the JSDoc for the leaveTeam function to state it returns Promise<{ teamId: string }[]> (or similar phrasing) and mention it returns an array of deleted rows, referencing leaveTeam, db.delete and .returning to locate the code; if the intended behavior is to return a single teamId instead, change the implementation to extract the first row (e.g., await db.delete(...).returning(...); return rows[0]?.teamId) and update the JSDoc/type accordingly.
🧹 Nitpick comments (2)
apps/api/src/lib/functions/middleware.ts (1)
16-18: Redundant optional chain inside a truthy ternary branch.
sessionis already narrowed to truthy by the ternary condition, sosession?.user.idcan simply besession.user.id.♻️ Proposed fix
- ? `Authenticated user (id: ${session?.user.id})` + ? `Authenticated user (id: ${session.user.id})`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/functions/middleware.ts` around lines 16 - 18, The ternary that builds userString redundantly uses an optional chain despite session being truthy; update the expression in the true branch to use session.user.id (replace session?.user.id with session.user.id) so the value is accessed without unnecessary optional chaining—modify the userString assignment where session is checked to remove the redundant ?. and keep the rest of the ternary logic unchanged.apps/api/src/lib/functions/database.ts (1)
108-119: Eager-Promiseargument defeats the short-circuit optimization.When callers pass an already-resolved
Promise<T>(i.e., an invoked query), the DB round-trip has already started regardless of whether the user is a site admin. The short-circuit on line 113 only saves work when the() => Promise<T>form is used. The comment acknowledges this, but it may be worth enforcing the lazy form to prevent accidental eager queries or at minimum adding a note to the JSDoc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/functions/database.ts` around lines 108 - 119, The function isUserSiteAdminOrQueryHasPermissions currently accepts a Promise or a thunk which defeats short-circuiting when callers pass an already-started Promise; change the API to require a thunk (() => Promise<T>) only (remove Promise<T> from the union) so the DB/query is only invoked after the admin check, and update the function signature and all callers to pass a zero-arg function that returns the Promise; alternatively, if you must keep the union, add a runtime guard in isUserSiteAdminOrQueryHasPermissions that throws a clear error when query is a Promise (and update the JSDoc for isUserSiteAdminOrQueryHasPermissions to explicitly require lazy thunks), referencing the function name so reviewers can locate and update callers like places invoking isUserSiteAdminOrQueryHasPermissions with eager Promises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/lib/functions/middleware.ts`:
- Line 43: Remove the stale developer note "// First check if it is a public
route and if so we will return (make sure this works)" from the middleware file
— locate the comment in apps' middleware.ts near the middleware function that
checks public routes (e.g., around any isPublicRoute or matcher logic) and
either delete it or replace it with a concise, production-ready comment like "//
Skip public routes" so no dev-side parentheses remain.
---
Outside diff comments:
In `@apps/api/src/lib/functions/database.ts`:
- Around line 29-42: The JSDoc for leaveTeam is wrong:
db.delete(...).returning(...) yields an array of row objects, not a single
scalar; update the JSDoc for the leaveTeam function to state it returns
Promise<{ teamId: string }[]> (or similar phrasing) and mention it returns an
array of deleted rows, referencing leaveTeam, db.delete and .returning to locate
the code; if the intended behavior is to return a single teamId instead, change
the implementation to extract the first row (e.g., await
db.delete(...).returning(...); return rows[0]?.teamId) and update the JSDoc/type
accordingly.
In `@apps/api/src/lib/functions/middleware.ts`:
- Around line 70-75: In afterRouteLogicMiddleware, remove the two debug
console.log statements that print c.get("teamId") (or at minimum wrap them in a
dev-only check); update the function (and any temporary TODO) so production
builds do not log raw context to stdout — locate afterRouteLogicMiddleware in
middleware.ts and either delete the console.log calls or guard them behind
isInDevMode() while preserving the await next() and existing TODO.
- Around line 44-46: The current isPublicRoute check uses
c.req.path.startsWith(route) which can incorrectly mark routes like
"/api/authorization" public when the intent was to only allow the exact segment
"/api/auth"; update the matching logic used when computing isPublicRoute (and
where MIDDLEWARE_PUBLIC_ROUTES is referenced) to only match whole path segments
by anchoring to the segment boundary — e.g., treat a route as public if
c.req.path === route or c.req.path.startsWith(route + "/") or use a regex
equivalent that enforces ^route(\/|$) — so that only the exact segment or its
subpaths are considered public and similarly-named routes remain protected.
- Line 23: The request logging calls using logInfo (the initial call that logs
`Middleware for request path ${c.req.path} for ${userString}` and the later
call) are fire-and-forget and may be dropped in serverless runtimes; update the
middleware to either await the returned promise from logInfo or, in
platform-specific code paths, schedule it with the execution lifecycle (e.g.,
use c.executionCtx.waitUntil(...) on Workers) to ensure durability, and move the
initial log until after you set c.userId/c.teamId (or reconstruct userString) so
the log includes userId/teamId; locate the logInfo calls in the middleware
function and replace them with an awaited call or waitUntil scheduling as
appropriate.
- Around line 32-34: Authenticated branch in middleware.ts never initializes
teamId, causing inconsistent downstream behavior versus the unauthenticated
path; add c.set("teamId", null) in the authenticated branch (alongside
c.set("user", session.user) and c.set("session", session.session")) before
calling await next() so handlers can reliably check c.get("teamId") === null as
"no team".
---
Nitpick comments:
In `@apps/api/src/lib/functions/database.ts`:
- Around line 108-119: The function isUserSiteAdminOrQueryHasPermissions
currently accepts a Promise or a thunk which defeats short-circuiting when
callers pass an already-started Promise; change the API to require a thunk (()
=> Promise<T>) only (remove Promise<T> from the union) so the DB/query is only
invoked after the admin check, and update the function signature and all callers
to pass a zero-arg function that returns the Promise; alternatively, if you must
keep the union, add a runtime guard in isUserSiteAdminOrQueryHasPermissions that
throws a clear error when query is a Promise (and update the JSDoc for
isUserSiteAdminOrQueryHasPermissions to explicitly require lazy thunks),
referencing the function name so reviewers can locate and update callers like
places invoking isUserSiteAdminOrQueryHasPermissions with eager Promises.
In `@apps/api/src/lib/functions/middleware.ts`:
- Around line 16-18: The ternary that builds userString redundantly uses an
optional chain despite session being truthy; update the expression in the true
branch to use session.user.id (replace session?.user.id with session.user.id) so
the value is accessed without unnecessary optional chaining—modify the
userString assignment where session is checked to remove the redundant ?. and
keep the rest of the ternary logic unchanged.
1 participant
Satisfies
#52
Summary by CodeRabbit
New Features
Chores