Various scaffolding of server routes, frontend changes, and database changes #33
Various scaffolding of server routes, frontend changes, and database changes #33christianhelp wants to merge 24 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds API logging, permission helpers, middleware request tracing, expanded team routes (join/admin/members), Zod validation, and new DB migrations; replaces web sidebar with a navbar and introduces many Radix-based UI primitives, theme hook, and query mutations; updates shared constants, validators, and multiple package manifests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant API as API (Hono)
participant Auth as Auth Service
participant DB as Database
Client->>API: POST /team/join?inv=code
API->>Auth: validate session
Auth-->>API: session / user
API->>DB: SELECT team_invite WHERE code
DB-->>API: invite
API->>DB: SELECT user_to_team WHERE userId & teamId
DB-->>API: membership?
alt not member
API->>DB: BEGIN TRANSACTION
API->>DB: INSERT INTO user_to_team
API->>DB: UPDATE team_invite SET accepted_at
API->>DB: COMMIT
DB-->>API: success
API-->>Client: 200 { success: true }
else already member
API-->>Client: 400 { message: "Already member" }
end
sequenceDiagram
participant Request as Incoming Request
participant Middleware as setUserSessionContextMiddleware
participant After as afterRouteLogicMiddleware
participant Logger as logToDb/logInfo/logError
participant DB as Database (log)
participant Console as Console (dev)
Request->>Middleware: attach user, requestId
Middleware->>Logger: logInfo("middleware start", context)
Logger->>Logger: isInDevMode()?
alt dev
Logger->>Console: console.log(...)
else prod
Logger->>DB: INSERT INTO log (...)
DB-->>Logger: inserted id
end
Middleware->>Request: proceed to route handler
Request->>After: afterRouteLogicMiddleware runs post-response
After->>Logger: logInfo("after route", teamId)
Logger-->>After: logged
After->>Request: done
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/routes/team/join.tsx (1)
4-16: Align route description with supported search params
The header comment says users can join via invite code or team ID, but the component now only readsinv. Either handleteamIdhere or update the description/schema to avoid confusion.
🤖 Fix all issues with AI agents
In `@apps/api/src/lib/functions/database.ts`:
- Around line 118-129: getAllContextValues is returning whole objects for user
and session but LoggingOptions/log table expect string IDs, causing a type
mismatch; update getAllContextValues to return userId and sessionId (strings or
null) instead of user/session objects by extracting the identifier properties
(e.g., c.get("user")?.id ?? null and c.get("session")?.id ?? null), keep
route/requestId/teamId as before, and ensure the returned keys match the
LoggingOptions names (userId, sessionId) so inserts use strings not objects.
In `@apps/api/src/lib/functions/middleware.ts`:
- Around line 12-24: Set the requestId before any logging and stop logging raw
emails: call nanoid() and c.set("requestId", requestId) first, then build a user
identifier that excludes the email (e.g., use session?.user.id or an anonymized
token instead of session.user.email), and pass that identifier into the logInfo
call for the middleware path; update usages of requestId, session, userString,
nanoid, and logInfo accordingly so logs include requestId but never persist raw
email addresses.
In `@apps/api/src/routes/log.ts`:
- Around line 19-22: The handler is passing logType as a raw string to logToDb
causing the function to receive the wrong shape; update the call in the route so
you pass an object with the logType property (e.g., logToDb({ logType: logType
as LoggingType }, message, { ...optionals })) or adjust the argument order to
match logToDb's signature; locate the destructuring of logData and the logToDb
invocation in this file and change the first argument to be an object containing
logType (properly typed) instead of the string so the DB NOT NULL constraint is
satisfied.
- Around line 28-30: The route's zValidator call is passing the schema directly
(zValidator("param", teamIdValidator)) but the validator expects an object keyed
by the param name; update the call in the .get("/:teamId", ...) handler to pass
an object like zValidator("param", { teamId: teamIdValidator }) so the "teamId"
param is validated correctly, and apply the same pattern wherever other routes
use zValidator with single param schemas.
In `@apps/api/src/routes/team.ts`:
- Around line 177-187: The db.query.team.findFirst call is not awaited so
allTeamInfo is a Promise; update the handler to await the result (await
db.query.team.findFirst(...)) before returning and ensure the surrounding
function is async if not already, then pass the resolved object to c.json (e.g.,
use allTeamInfo after awaiting) so the response contains the actual team data
rather than a Promise.
- Around line 173-174: The handler currently returns c.json({ message:
API_ERROR_MESSAGES.notAuthorized }) when canUserView is false, which yields a
200; update the response to set an appropriate HTTP status (e.g., 403 Forbidden
or 401 Unauthorized) by returning c.status(403).json({ message:
API_ERROR_MESSAGES.notAuthorized }) so callers receive the correct status code;
locate the check using the canUserView variable in the team route and replace
the c.json return with c.status(403).json(...) accordingly.
- Around line 168-175: The permission check is using
isUserSiteAdminOrQueryHasPermissions which returns a Promise<boolean>, so change
the call to await the result (e.g., const canUserView = await
isUserSiteAdminOrQueryHasPermissions(user.siteRole, getAdminUserForTeam(user.id,
teamId));) so canUserView is a boolean before the if-check; ensure the
surrounding function is async if not already and keep the
getAdminUserForTeam(...) argument as-is unless that helper also requires
awaiting.
- Line 98: The route is passing a string schema to zValidator for params; change
zValidator("param", teamIdValidator) to pass an object schema that matches the
route params shape, e.g. zValidator("param", z.object({ teamId: teamIdValidator
})), and apply the same replacement for the other routes using
zValidator("param", teamIdValidator) so the param validator expects an object
rather than a string.
- Around line 224-261: The route PATCH handler uses zValidator("form",
teamNameValidator) but teamNameValidator is a string schema, so
c.req.valid("form") will return the whole object (or fail) and you then set
name: newTeamName incorrectly; change the form validator to an object schema
(e.g., z.object({ name: teamNameValidator })) when calling zValidator("form",
...), then extract the string via the validated object's property (e.g., const {
name: newTeamName } = c.req.valid("form")) before calling db.update(team).set({
name: newTeamName }).where(eq(team.id, teamId)); ensure you update references to
teamNameValidator usage in this PATCH ("/:teamId/update") handler and keep
permission checks (isUserSiteAdminOrQueryHasPermissions / getAdminUserForTeam)
unchanged.
In `@apps/web/src/components/shared/LeaveTeamDialog.tsx`:
- Around line 18-53: The confirm action currently does nothing: wire up
LeaveTeamDialog so AlertDialogAction invokes the leave-team mutation with both
teamId and the current userId; import and use getUserQueryClient to fetch the
current user (e.g. from its query data) inside LeaveTeamDialog, call the
existing leave mutation with { teamId: _unusedForNowTeamId, userId }, and
disable the AlertDialogAction while the mutation is pending (use the mutation's
isLoading/isPending flag). Ensure you handle success/error states (close dialog
on success, show error feedback) and reference AlertDialogAction,
LeaveTeamDialog, and getUserQueryClient when implementing the changes.
In `@apps/web/src/components/shared/Navbar/ThemeSwitcher.tsx`:
- Around line 11-14: The ThemeSwitcher button is an icon-only control and needs
an accessible name; update the Button in the ThemeSwitcher component to set an
appropriate ARIA label (e.g., aria-label or aria-pressed) based on current theme
so screen readers know the action — use theme/THEME_CONFIG to compute a label
like "Switch to dark theme" when theme === THEME_CONFIG.light and "Switch to
light theme" otherwise, and attach it to the Button that calls switchTheme.
In `@apps/web/src/components/shared/Navbar/UserButton.tsx`:
- Around line 134-147: The logout handler in the DropdownMenuItem (the async
onClick that calls authClient.signOut, toast, invalidate(), and navigate) should
remove cached react-query user data instead of only invalidating route loaders:
obtain the react-query QueryClient (e.g., via useQueryClient) and call
queryClient.removeQueries(["user"]) and
queryClient.removeQueries(["user","teams"]) (or resetQueries/removeQueries for
those keys) after a successful signOut and before navigation so stale user/team
data is cleared from cache; keep the existing toast flows and navigate call but
replace or augment the current invalidate() call with these removeQueries calls.
In `@apps/web/src/lib/hooks/useTheme.ts`:
- Around line 15-23: switchTheme currently overwrites document.body.classList
and uses a potentially stale captured theme; instead derive the active theme at
call time and update classes without clobbering other body classes: read the
current theme from document.body.classList (or use a functional state update
setTheme(prev => ...)) to decide newTheme, call document.body.classList.remove()
for the old theme and document.body.classList.add() for the new theme (do not
assign to classList), then call setTheme(newTheme) (or return newTheme from the
functional updater) and update localStorage with THEME_CONFIG.accessKey;
reference switchTheme, theme, THEME_CONFIG, setTheme, and
document.body.classList.
- Line 10: The code in useTheme (where you set document.body.classList =
storedTheme) incorrectly replaces all body classes; instead, update the body’s
classes without clobbering unrelated classes by removing any previous theme
class and adding the new one (use document.body.classList.remove(...) and
document.body.classList.add(...), or toggle as appropriate). Locate the
assignment to document.body.classList in useTheme.ts, identify the storedTheme
value and any previousTheme variable or mechanism, remove the old theme class if
present, then add storedTheme to classList so other body classes remain intact.
In `@packages/db/drizzle/0009_brainy_power_man.sql`:
- Around line 31-36: Migration makes `route` and `request_id` NOT NULL which
will fail if existing rows contain NULLs; before running the ALTER TABLE
statements that change `"route"` and `"request_id"` to NOT NULL, add a safe
backfill step (e.g., UPDATE log SET "route" = '' WHERE "route" IS NULL and
UPDATE log SET "request_id" = '' WHERE "request_id" IS NULL) or choose a
suitable default, then run the ALTER TABLE ... SET NOT NULL; alternatively
perform the change with a transaction that updates NULLs using COALESCE in-place
so the ALTER COLUMN statements for `"route"` and `"request_id"` succeed.
- Around line 31-37: The ALTER TABLE ... ALTER COLUMN statements for the `log`
table (affecting columns "route", "request_id", "team_id", "user_id") are
invalid in SQLite/Turso; instead, recreate the `log` table with the new column
definitions and the added `time_elapsed_ms` integer: create a new temp table
(e.g., `log_new`) with the desired schema, copy data from `log` to `log_new`
mapping existing columns, drop the old `log`, then rename `log_new` to `log`,
and finally re-create any necessary indexes (ensure this migration still creates
`session_token_unique` and `user_email_unique` on their respective tables).
Ensure you preserve constraints, nullability, and data during the copy and
remove the ALTER COLUMN statements referenced in the diff.
In `@packages/db/schema.ts`:
- Around line 112-114: The current invite schema uses email:
standardVarcharFactory().references(() => user.email, { onDelete: "cascade" }),
which enforces a foreign key to user.email and prevents creating invites for
non-registered users; either remove the .references(...) call so the
invite.email is a plain varchar (allowing invites to unregistered emails), or
replace the email FK with a nullable userId: idType?.references(() => user.id, {
onDelete: "cascade" }) and keep invite.email as an independent varchar — update
the invite model/validation accordingly and remove the email FK reference to
user.email if you choose the first option.
🟡 Minor comments (8)
apps/web/package.json-40-41 (1)
40-41: Align react and react-dom versions.
reactis updated to^19.2.3butreact-domremains at^19.0.0. These packages should have matching versions to avoid potential runtime inconsistencies or subtle bugs.Proposed fix
"next-themes": "^0.4.6", "react": "^19.2.3", - "react-dom": "^19.0.0", + "react-dom": "^19.2.3", "shared": "workspace:*",packages/db/drizzle/meta/0009_snapshot.json-527-540 (1)
527-540: Foreign key on email column is unusual and may cause issues.The
team_invite_email_user_email_fkforeign key referencesuser.emailinstead ofuser.id. This design has implications:
- If a user changes their email, the invite becomes orphaned or the FK constraint fails.
- Email-based FKs couple the invite tightly to the user's current email at invite time.
Consider whether this is intentional for the invite workflow, or if referencing
user.idwould be more robust.apps/web/src/components/shared/Navbar/navbar.tsx-56-63 (1)
56-63: Use routerLinkfor sign-in to avoid full reload.Line 58 uses a raw anchor tag, which forces a full page refresh and bypasses TanStack Router navigation. The sign-up link (line 66) correctly uses
<Link>, so align the sign-in link with it.✅ Proposed fix
- {/* <Link to="/sign-in"> */} - <a href="/sign-in"> + <Link to="/sign-in"> <Button variant="ghost" size="sm"> Login </Button> - </a> - {/* </Link> */} + </Link>apps/api/src/index.ts-32-32 (1)
32-32: Remove debug console.log statements from middleware before production.The
afterRouteLogicMiddleware(inapps/api/src/lib/functions/middleware.tslines 56-60) containsconsole.logstatements that appear to be debug artifacts. These should be removed or converted to proper logging before merging.apps/api/src/routes/team.ts-271-276 (1)
271-276: Typo in response key: "messsage" should be "message".Suggested fix
if (!user) { return c.json( - { messsage: API_ERROR_MESSAGES.notAuthorized }, + { message: API_ERROR_MESSAGES.notAuthorized }, 401, ); }apps/api/src/routes/team.ts-74-76 (1)
74-76: Typo in response key: "messsage" should be "message".Suggested fix
if (!inviteRequest) { - return c.json({ messsage: API_ERROR_MESSAGES.codeNotFound }, 400); + return c.json({ message: API_ERROR_MESSAGES.codeNotFound }, 400); }apps/web/src/lib/utils.ts-20-25 (1)
20-25: Fix potential runtime errors ingetInitialsfor edge cases.The function has issues:
fullName.split(" ")always returns an array with at least one element, sonames.length === 0is unreachable.- When
fullNameis empty (""),names[0]is"", and accessingnames[0][0]returnsundefined, causing.toUpperCase()to throw.- Names with leading/trailing spaces or multiple spaces between words can produce empty strings in the array.
Suggested fix with proper edge case handling
export function getInitials(fullName: string): string { - const names = fullName.split(" "); - if (names.length === 0) return ""; - if (names.length === 1) return names[0][0].toUpperCase(); - return names[0][0].toUpperCase() + names[names.length - 1][0].toUpperCase(); + const names = fullName.trim().split(/\s+/).filter(Boolean); + if (names.length === 0) return ""; + if (names.length === 1) return names[0][0].toUpperCase(); + return names[0][0].toUpperCase() + names[names.length - 1][0].toUpperCase(); }packages/db/schema.ts-182-182 (1)
182-182: Typo in comment: "TOOD" should be "TODO".📝 Suggested fix
- // TOOD: All of these fields are nullable because not all logs have the same info. There might be a better approach. + // TODO: All of these fields are nullable because not all logs have the same info. There might be a better approach.
🧹 Nitpick comments (13)
apps/web/src/components/ui/navigation-menu.tsx (1)
65-83: Duplicate"group"class.The
"group"class is already included innavigationMenuTriggerStyle()(line 62) and doesn't need to be added again on line 73. WhiletwMergehandles this gracefully, the explicit addition is redundant.♻️ Suggested cleanup
function NavigationMenuTrigger({ className, children, ...props }: React.ComponentProps<typeof NavigationMenuPrimitive.Trigger>) { return ( <NavigationMenuPrimitive.Trigger data-slot="navigation-menu-trigger" - className={cn(navigationMenuTriggerStyle(), "group", className)} + className={cn(navigationMenuTriggerStyle(), className)} {...props} >packages/db/drizzle/0005_glorious_dragon_lord.sql (1)
1-9: Consider adding a unique constraint on(team_id, user_id)to prevent duplicate pending requests.Without this constraint, a user could create multiple join requests for the same team. If you want to allow resubmission after rejection, consider making the constraint conditional on status or adding application-level validation.
Additionally, consider adding a
CHECKconstraint to restrictstatusto valid values (e.g.,'PENDING','APPROVED','REJECTED').Suggested schema improvements
CREATE TABLE `team_join_request` ( `id` text PRIMARY KEY NOT NULL, `team_id` text NOT NULL, `user_id` text NOT NULL, `created_at` integer DEFAULT (current_timestamp) NOT NULL, - `status` text DEFAULT 'PENDING' NOT NULL, + `status` text DEFAULT 'PENDING' NOT NULL CHECK (`status` IN ('PENDING', 'APPROVED', 'REJECTED')), FOREIGN KEY (`team_id`) REFERENCES `team`(`id`) ON UPDATE no action ON DELETE cascade, - FOREIGN KEY (`user_id`) REFERENCES `user`(`id`) ON UPDATE no action ON DELETE cascade + FOREIGN KEY (`user_id`) REFERENCES `user`(`id`) ON UPDATE no action ON DELETE cascade, + UNIQUE (`team_id`, `user_id`) );apps/web/src/components/ui/alert.tsx (1)
37-48: Consider using a heading element for AlertTitle.For improved semantic HTML and accessibility, consider using an
h5or allowing a configurable heading level via anasprop, rather than a genericdiv. Screen readers benefit from proper heading hierarchy.♻️ Optional: Use semantic heading
-function AlertTitle({ className, ...props }: React.ComponentProps<"div">) { +function AlertTitle({ className, ...props }: React.ComponentProps<"h5">) { return ( - <div + <h5 data-slot="alert-title" className={cn( "col-start-2 line-clamp-1 min-h-4 font-medium tracking-tight", className, )} {...props} /> ); }apps/web/src/routes/__root.tsx (1)
29-35: Gate router devtools to dev-only builds.
Shipping devtools in production can add bundle weight and expose internals. Consider conditioning on the project’s dev flag.♻️ Proposed change
- <TanStackRouterDevtools /> + {import.meta.env.DEV && <TanStackRouterDevtools />}apps/api/src/lib/auth.ts (1)
36-41: Add a guard against config/DB drift for dynamic fields.
Now thatadditionalFieldscomes from config, a mismatch with actual DB columns will surface only at runtime. Consider a startup assertion or schema test to fail fast when config changes.apps/web/src/components/shared/LeaveTeamDialog.tsx (1)
1-1: Track the owner-leave TODO before release.
This flow can have destructive side‑effects (ownership transfer/deletion). Happy to help flesh out the UX/logic when you’re ready.Also applies to: 27-29
apps/api/src/routes/log.ts (1)
16-26: Consider restricting POST/to authenticated/internal callers.
If this endpoint is public, anyone can spam logs and bloat the DB. Consider auth (session or service token) or stricter rate‑limits for this route.apps/api/src/lib/functions/middleware.ts (1)
57-61: Prefer structured logging (or dev‑only) overconsole.log.
This will spam stdout in production and bypass your logging pipeline.♻️ Example using logInfo
-export async function afterRouteLogicMiddleware(c: ApiContext, next: Next) { - console.log("context before is: ", c.get("teamId")); - await next(); - console.log("context after is: ", c.get("teamId")); -} +export async function afterRouteLogicMiddleware(c: ApiContext, next: Next) { + await logInfo(`context before: teamId=${c.get("teamId")}`, c); + await next(); + await logInfo(`context after: teamId=${c.get("teamId")}`, c); +}apps/web/src/components/shared/Navbar/UserButton.tsx (1)
87-120: UseasChildfor menu items that wrap links.
Nesting<Link>inside<DropdownMenuItem>creates nested interactive elements and breaks keyboard UX and menu behavior. TheasChildprop lets Radix attach menu interactions to the actual link element.Apply this pattern to the three affected menu items:
♻️ Suggested refactor
-<DropdownMenuItem key={userToTeam.team.id}> - <Link - to={"/team/$teamId"} - params={{ teamId: userToTeam.team.id }} - className="underline" - > - <span>{userToTeam.team.name}</span> - </Link> -</DropdownMenuItem> +<DropdownMenuItem key={userToTeam.team.id} asChild> + <Link + to={"/team/$teamId"} + params={{ teamId: userToTeam.team.id }} + className="underline" + > + {userToTeam.team.name} + </Link> +</DropdownMenuItem> @@ -<DropdownMenuItem> - <Plus /> - <Link to="/team/new">Create Team</Link> -</DropdownMenuItem> +<DropdownMenuItem asChild> + <Link to="/team/new"> + <Plus /> + <span>Create Team</span> + </Link> +</DropdownMenuItem> @@ -<DropdownMenuItem> - <UserPlus /> - <Link to="/team/join">Join a Team</Link> -</DropdownMenuItem> +<DropdownMenuItem asChild> + <Link to="/team/join"> + <UserPlus /> + <span>Join a Team</span> + </Link> +</DropdownMenuItem>apps/api/src/index.ts (1)
22-22: Convert empty interface to type alias.The static analysis correctly identifies that an empty interface is equivalent to
{}. Use a type alias for clarity.Suggested fix
-interface Env {} +type Env = Record<string, unknown>;apps/api/src/routes/team.ts (1)
278-286: Self-removal should verify user is actually a member of the team.Currently, if a user requests to remove themselves,
leaveTeamis called without verifying they're actually a member. This could lead to confusing success responses when the user isn't in the team, and potentially masks other issues.Suggested improvement
if (userIdToRemove === user.id) { - await leaveTeam(user.id, teamId); + const membership = await db.query.userToTeam.findFirst({ + where: and( + eq(userToTeam.userId, user.id), + eq(userToTeam.teamId, teamId), + ), + }); + if (!membership) { + return c.json({ message: API_ERROR_MESSAGES.notFound }, 404); + } + await leaveTeam(user.id, teamId); return c.json( { message: "Successfully removed from team." }, 200, ); }apps/web/src/lib/queries.ts (1)
43-43: Inconsistent function naming convention.
joinTeamMutationclientuses lowercase "c" whileleaveTeamMutationClientuses uppercase "C". For consistency, rename tojoinTeamMutationClient.Suggested fix
-export const joinTeamMutationclient = (inviteCode: string) => +export const joinTeamMutationClient = (inviteCode: string) =>packages/db/schema.ts (1)
128-138: Redundant.notNull()calls.
standardVarcharFactory()already includes.notNull()(line 16), so the chained.notNull()on lines 131 and 134 are unnecessary.♻️ Suggested fix
export const teamJoinRequest = sqliteTable("team_join_request", { id: standardIdFactory("tjr_").primaryKey(), teamId: standardVarcharFactory() - .notNull() .references(() => team.id, { onDelete: "cascade" }), userId: standardVarcharFactory() - .notNull() .references(() => user.id, { onDelete: "cascade" }), createdAt: standardDateFactory(), status: teamJoinRequestStatusType.notNull().default("PENDING"), });
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/schema.ts (1)
13-17:⚠️ Potential issue | 🟠 MajorInconsistent DB column naming: factory-created columns will be camelCase while others are snake_case.
standardVarcharFactory()andstandardVarcharFactoryNullable()don't accept a column name argument, so columns using them (e.g.,userToTeam.userId,backupJob.teamId,log.teamId) will have camelCase names in the DB. Meanwhile, theusertable and auth tables explicitly set snake_case names (e.g.,"first_name","user_id","ip_address").This creates a mix of naming conventions in the actual database (e.g.,
"userId"vs"user_id"across tables), which complicates raw queries, migrations, and debugging.Consider either:
- Adding an optional column name parameter to the factories, or
- Not using the factory for FK columns and instead specifying the column name directly (e.g.,
text("user_id", { length: 255 }).notNull().references(...))
🤖 Fix all issues with AI agents
In `@apps/api/src/lib/functions/middleware.ts`:
- Around line 52-59: Replace the debug console.log calls in
afterRouteLogicMiddleware with the project's structured logger (use logInfo) or
remove them; specifically, replace console.log("context before is: ",
c.get("teamId")) and console.log("context after is: ", c.get("teamId")) with
logInfo calls that include a clear message and the teamId (e.g.,
logInfo("afterRouteLogicMiddleware - context before", { teamId: c.get("teamId")
})) or delete both lines if you don't want any logging; ensure you import or
reference the existing logInfo function used by other middleware and keep the
middleware signature (ApiContext, Next) unchanged.
In `@apps/api/src/routes/team.ts`:
- Around line 78-96: The insert into userToTeam inside the transaction
(tx.insert(userToTeam).values(...)) can raise a composite PK conflict when the
user is already a member; change this to a conflict-safe operation by using the
query builder's upsert/conflict handler (e.g.,
tx.insert(userToTeam).values(...).onConflictDoNothing()) or perform a quick
existence check (select from userToTeam where teamId = inviteRequest.teamId and
userId = user.id) before inserting, and ensure the rest of the transaction still
updates teamInvite.acceptedAt and returns the invite_code_success response;
reference the userToTeam insert and the surrounding transaction block and keep
the teamInvite update as-is.
- Around line 51-61: The route registers zValidator("query", joinTeamSchema) but
then reads the raw query with c.req.query("inv"), ignoring the validated result;
change the handler to retrieve the validated/typed query via
c.req.valid("query") and read the invite code from that object (instead of using
c.req.query), keeping the existing checks for user and inv; update references in
this handler (the POST "/join" route, the zValidator usage and joinTeamSchema)
so the invite code comes from the validated payload.
- Around line 208-215: The query uses db.query.team.findMany with where:
eq(team.id, teamId), which returns team objects (and at most one row) rather
than member records; replace this with either db.query.team.findFirst (to fetch
the single team and its members via with: { members: true }) or directly query
the userToTeam relation to return member records, updating the variable
teamMembers and the response to return the actual member list (references:
teamMembers, db.query.team.findMany, db.query.team.findFirst, userToTeam,
team.id, teamId).
- Around line 177-188: The result of db.query.team.findFirst assigned to
allTeamInfo can be null when no team matches; add a guard after the findFirst
call in this route handler to check if allTeamInfo is null and return a 404 JSON
response (e.g., c.json({ message: 'Team not found' }, 404)) instead of returning
{ message: null }, mirroring the not-found check used in the GET /:teamId
handler; ensure the early return prevents the existing c.json({ message:
allTeamInfo }, 200) from running when allTeamInfo is null.
In `@apps/web/src/lib/hooks/useTheme.ts`:
- Around line 7-17: The current useEffect in the useTheme hook only applies a
theme when localStorage has a value, so on first mount the body never receives
the default theme class; update the effect in useTheme to handle the missing
stored theme by removing any existing THEME_CONFIG.light/THEME_CONFIG.dark
classes, adding THEME_CONFIG.light (or THEME_CONFIG.default if available), and
calling setTheme with that default when
localStorage.getItem(THEME_CONFIG.accessKey) is null/undefined; keep the
existing branch that applies storedTheme when present and ensure you reference
THEME_CONFIG, document.body.classList.remove/classList.add, and setTheme in the
fix.
In `@packages/db/drizzle/0010_salty_romulus.sql`:
- Around line 17-22: This migration uses LibSQL/Turso-specific ALTER TABLE ...
ALTER COLUMN statements ("ALTER TABLE `log` ALTER COLUMN \"route\"" and "ALTER
TABLE `log` ALTER COLUMN \"request_id\"") and also recreates unique indexes
(`session_token_unique`, `user_email_unique`); ensure the migration is gated to
run only against LibSQL/Turso-compatible databases (add a runtime/db-engine
check and document this requirement), and reorder the statements so both ALTER
COLUMN operations occur before recreating the unique indexes (perform ALTER
COLUMN "route" and "request_id" first, then CREATE UNIQUE INDEX
`session_token_unique` and `user_email_unique`) to avoid leaving the DB in a
partially-migrated state if ALTER fails.
- Line 6: The DEFAULT for the integer column created_at (Drizzle mode
"timestamp_ms") currently uses current_timestamp which yields a text datetime;
change the DEFAULT expression to produce a millisecond epoch integer (e.g. use
strftime('%s','now') * 1000 for broad SQLite compatibility or unixepoch() * 1000
for LibSQL/Turso) so new rows get integer millisecond timestamps compatible with
the created_at column and Drizzle's timestamp_ms mode.
In `@packages/db/schema.ts`:
- Line 180: Fix the typo in the inline comment in packages/db/schema.ts: change
"TOOD" to "TODO" in the comment that reads "// TOOD: All of these fields are
nullable because not all logs have the same info. There might be a better
approach." so it reads "// TODO: All of these fields are nullable because not
all logs have the same info. There might be a better approach."; this is in the
schema definition area where nullable log fields are documented.
- Line 57: The user table's updatedAt column is missing a default and only has
.notNull(), causing insert failures and inconsistency with createdAt; update the
user table definition (the updatedAt column) to use the same default as
createdAt or the helper standardDateFactory() (whichever pattern the schema uses
elsewhere) so updatedAt has a default timestamp on insert—modify the updatedAt
column declaration to mirror createdAt (or call standardDateFactory()) to ensure
a default current timestamp and consistent behavior with other tables.
🧹 Nitpick comments (5)
apps/web/src/lib/hooks/useTheme.ts (1)
8-15: No validation of the value read fromlocalStorage.
localStoragecan contain any string (user-tampered, leftover from an older version, etc.). IfstoredThemeis neither"light"nor"dark", a bogus class gets added to the body and the toggle logic inswitchThemefalls through toTHEME_CONFIG.darkunexpectedly. Consider validating before use.Proposed fix
useEffect(() => { const storedTheme = localStorage.getItem(THEME_CONFIG.accessKey); - if (storedTheme) { + const validThemes: string[] = [THEME_CONFIG.light, THEME_CONFIG.dark]; + if (storedTheme && validThemes.includes(storedTheme)) { document.body.classList.remove( THEME_CONFIG.light, THEME_CONFIG.dark, ); document.body.classList.add(storedTheme); setTheme(storedTheme); } }, []);packages/db/schema.ts (1)
126-136: Redundant.notNull()onteamIdanduserId.
standardVarcharFactory()already returns a.notNull()column (line 16). The extra.notNull()calls on lines 129 and 132 are redundant and inconsistent with howuserToTeam(lines 83–88) uses the same factory without the extra chain.Suggested fix
export const teamJoinRequest = sqliteTable("team_join_request", { id: standardIdFactory("tjr_").primaryKey(), - teamId: standardVarcharFactory() - .notNull() - .references(() => team.id, { onDelete: "cascade" }), - userId: standardVarcharFactory() - .notNull() - .references(() => user.id, { onDelete: "cascade" }), + teamId: standardVarcharFactory().references(() => team.id, { onDelete: "cascade" }), + userId: standardVarcharFactory().references(() => user.id, { onDelete: "cascade" }), createdAt: standardDateFactory(), status: teamJoinRequestStatusType.notNull().default("PENDING"), });apps/api/src/routes/log.ts (2)
27-46: Logs endpoint lacks pagination — risk of unbounded response.The TODO on line 27 already flags this:
findManyat line 43 has nolimit/offset, so a team with thousands of log entries will return them all in one response. This can cause latency spikes and memory pressure.Consider adding at least a
limitwith a sensible default (e.g., 50–100) and acceptingpage/offsetquery params, even as a quick initial guard.
48-55: Admin route also returns unbounded results.
db.query.log.findMany()at line 53 will return every log in the system. Same pagination concern as the team-scoped route above — this is even more critical since it spans all teams.apps/api/src/routes/team.ts (1)
225-225: Misleading variable namenewTeamNameSchema.This variable holds the validated form data (
{ name: string }), not a schema. A name likevalidatedFormorteamNameDatawould be clearer.♻️ Suggested rename
- const newTeamNameSchema = c.req.valid("form"); + const { name: newTeamName } = c.req.valid("form"); ... - name: newTeamNameSchema.name, + name: newTeamName,
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/schema.ts (1)
25-29:⚠️ Potential issue | 🟠 MajorFix timestamp default to use epoch milliseconds instead of
current_timestamp.
standardDateFactory()and multiple other fields usemode: "timestamp_ms"(which tells Drizzle the stored integer represents milliseconds since epoch) with.default(sql\(current_timestamp)`). However, SQLite'scurrent_timestampreturns a text string like2025-02-14 10:30:45, not milliseconds. When Drizzle reads this text value astimestamp_ms`, it will fail to parse correctly or default to 0 (representing 1970-01-01).Change the default to
sql\(unixepoch() * 1000)`` to generate the correct epoch milliseconds:Fix for standardDateFactory()
function standardDateFactory() { return integer({ mode: "timestamp_ms" }) .notNull() .default(sql`(unixepoch() * 1000)`); }Apply the same fix to all direct
timestamp_msfields:user.createdAt,user.updatedAt,teamMember.expiresAt,teamMember.acceptedAt, andbackupLog.completedAt.
🤖 Fix all issues with AI agents
In `@apps/api/src/lib/functions/database.ts`:
- Around line 95-109: The DB insert in function logToDb currently calls await
db.insert(log).values(...) with no error handling so failures will bubble up;
wrap that production branch in a try/catch around the db.insert(log).values(...)
call (keep the isInDevMode() early return) and swallow or record the error via a
non-throwing logger (e.g., console.error or your processLogger) so logging
remains best-effort and does not crash callers; ensure you still return after
handling the error.
In `@apps/api/src/routes/log.ts`:
- Around line 15-26: The POST handler (logHandler.post using HonoBetterAuth,
zValidator and logToDb) lacks any authentication/authorization check and
therefore allows unauthenticated writes; update the logHandler.post
implementation to perform the same user verification and permission check used
by the GET routes (the pattern used on the other endpoints: verify the
authenticated user exists and has the required permissions), returning the same
401/403 responses when checks fail, then proceed to call logToDb with the
validated payload; ensure you reuse the same auth/context properties and error
responses as the GET routes so behavior is consistent.
In `@apps/api/src/routes/team.ts`:
- Around line 297-341: The self-removal branch currently calls
leaveTeam(user.id, teamId) and always returns success even if no membership
existed; update the handler to verify the user is a member before confirming
removal: either (A) pre-check membership with an existing helper/query (e.g.,
call a function like isUserOnTeam(user.id, teamId) or a direct membership
lookup) and return a 404/appropriate error if not a member, or (B) change
leaveTeam to return the number of rows deleted (or a boolean) and, after await
leaveTeam(...), inspect the result and return a non-success response when no
rows were removed; keep the rest of the permission logic
(isUserSiteAdminOrQueryHasPermissions/getAdminUserForTeam) unchanged.
- Around line 140-153: The catch block handling DatabaseError in the join flow
incorrectly checks for Postgres code "23505" and swallows other DB errors;
update the condition to detect SQLite/libsql unique constraint errors (check
e.code === "SQLITE_CONSTRAINT" and/or e.extendedCode ===
"SQLITE_CONSTRAINT_UNIQUE") so duplicate-member inserts trigger the existing
logWarning and return of API_ERROR_MESSAGES.alreadyMember, and for any other
DatabaseError re-throw or return an appropriate 500 error instead of silently
continuing; modify the catch around DatabaseError (referencing DatabaseError,
e.code, e.extendedCode, logWarning, inviteRequest.teamId, user.id,
API_ERROR_MESSAGES.alreadyMember) to implement these checks and ensure
non-unique errors are not swallowed.
🧹 Nitpick comments (6)
packages/db/schema.ts (2)
127-137: Redundant.notNull()onteamIdanduserId.
standardVarcharFactory()(line 13-17) already chains.notNull(). The additional.notNull()on lines 130 and 133 is redundant. Not harmful, but inconsistent with howstandardVarcharFactory()is used elsewhere in this file (e.g.,userToTeamon lines 84, 87 where no extra.notNull()is added).♻️ Suggested cleanup
export const teamJoinRequest = sqliteTable("team_join_request", { id: standardIdFactory("tjr_").primaryKey(), teamId: standardVarcharFactory() - .notNull() .references(() => team.id, { onDelete: "cascade" }), userId: standardVarcharFactory() - .notNull() .references(() => user.id, { onDelete: "cascade" }), createdAt: standardDateFactory(), status: teamJoinRequestStatusType.notNull().default("PENDING"), });
176-187: Log table nullable fields lack FK references while relations expect them.
teamIdanduserIdin thelogtable (lines 182-183) are plain nullable varchars with no foreign key.references(...), yetlogRelations(lines 189-198) maps them toteam.idanduser.id. Drizzle relations are application-level and work without FK constraints, but this means the database won't enforce referential integrity for log entries — orphaned logs pointing to deleted teams/users will persist silently. If that's intentional (logs should survive entity deletion), consider adding a comment. If not, consider adding.references(() => team.id, { onDelete: "set null" })and similar foruserId.apps/api/src/lib/functions/database.ts (1)
41-45:isSiteAdminUserparameter type differs fromSiteRoleTypeused inisUserSiteAdminOrQueryHasPermissions.Line 42 uses
NonNullable<UserType>["siteRole"]whileisUserSiteAdminOrQueryHasPermissions(line 67) acceptsSiteRoleType(fromdb/types). These should resolve to the same underlying type, but expressing the same thing two different ways is fragile. Consider usingSiteRoleTypeconsistently.♻️ Suggested cleanup
export function isSiteAdminUser( - permissionEnum: NonNullable<UserType>["siteRole"], + permissionEnum: SiteRoleType, ): boolean { return ["ADMIN", "SUPER_ADMIN"].some((role) => role === permissionEnum); }packages/shared/constants.ts (1)
67-75: Centralized error message constants are a good pattern.One note: the keys use camelCase (
noInviteCode,codeNotFound) while values use snake_case (no_invite_code,code_not_found), exceptnotAuthorized→"unauthorized"which breaks the pattern by using a single word instead of snake_case. This is a very minor inconsistency.apps/api/src/routes/team.ts (2)
92-104:new Date(Date.now())is redundant.
new Date()already uses the current timestamp.Date.now()as an argument is unnecessary.♻️ Suggested cleanup
- const compDate = new Date(Date.now()); + const compDate = new Date();
258-296: Variable namednewTeamNameSchemaholds validated data, not a schema.Line 266 assigns the validated form output to
newTeamNameSchema, which is misleading — it's the parsed data, not a Zod schema. Consider renaming toteamNameDataorvalidatedForm.♻️ Suggested cleanup
- const newTeamNameSchema = c.req.valid("form"); + const teamNameData = c.req.valid("form"); // ... - name: newTeamNameSchema.name, + name: teamNameData.name,
| export async function logToDb( | ||
| loggingType: LoggingType, | ||
| message: string, | ||
| options?: LoggingOptions, | ||
| ) { | ||
| if (isInDevMode()) { | ||
| console.log(`[${loggingType}] - ${message} - Options: `, options); | ||
| return; | ||
| } | ||
| await db.insert(log).values({ | ||
| ...options, | ||
| logType: loggingType, | ||
| message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
logToDb DB insert has no error handling — a failed log write will crash the caller.
If the db.insert(log) call fails in production (e.g., DB connection issue, schema mismatch), the error will propagate up and potentially crash the request handler. Since logging should be best-effort and not take down the request, wrap the insert in a try/catch.
🛡️ Proposed fix
export async function logToDb(
loggingType: LoggingType,
message: string,
options?: LoggingOptions,
) {
if (isInDevMode()) {
console.log(`[${loggingType}] - ${message} - Options: `, options);
return;
}
- await db.insert(log).values({
- ...options,
- logType: loggingType,
- message,
- });
+ try {
+ await db.insert(log).values({
+ ...options,
+ logType: loggingType,
+ message,
+ });
+ } catch (e) {
+ console.error(`Failed to write log to DB: ${e}`);
+ }
}📝 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.
| export async function logToDb( | |
| loggingType: LoggingType, | |
| message: string, | |
| options?: LoggingOptions, | |
| ) { | |
| if (isInDevMode()) { | |
| console.log(`[${loggingType}] - ${message} - Options: `, options); | |
| return; | |
| } | |
| await db.insert(log).values({ | |
| ...options, | |
| logType: loggingType, | |
| message, | |
| }); | |
| } | |
| export async function logToDb( | |
| loggingType: LoggingType, | |
| message: string, | |
| options?: LoggingOptions, | |
| ) { | |
| if (isInDevMode()) { | |
| console.log(`[${loggingType}] - ${message} - Options: `, options); | |
| return; | |
| } | |
| try { | |
| await db.insert(log).values({ | |
| ...options, | |
| logType: loggingType, | |
| message, | |
| }); | |
| } catch (e) { | |
| console.error(`Failed to write log to DB: ${e}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/api/src/lib/functions/database.ts` around lines 95 - 109, The DB insert
in function logToDb currently calls await db.insert(log).values(...) with no
error handling so failures will bubble up; wrap that production branch in a
try/catch around the db.insert(log).values(...) call (keep the isInDevMode()
early return) and swallow or record the error via a non-throwing logger (e.g.,
console.error or your processLogger) so logging remains best-effort and does not
crash callers; ensure you still return after handling the error.
| const logHandler = HonoBetterAuth() | ||
| .post("/", zValidator("form", logSchema), async (c) => { | ||
| const logData = c.req.valid("form"); | ||
|
|
||
| const { message, logType, ...optionals } = logData; | ||
| // TODO: Come back and make this less ugly please. | ||
| await logToDb(logType as LoggingType, message, { | ||
| ...optionals, | ||
| }); | ||
|
|
||
| return c.json({ message: "Log endpoint hit" }, 200); | ||
| }) |
There was a problem hiding this comment.
POST / log creation endpoint has no authentication check.
Unlike the GET routes (lines 28, 48) which verify the user exists and has appropriate permissions, the POST route allows unauthenticated requests to insert arbitrary log entries into the database. This could be abused to spam the log table or inject misleading log data.
Add an authentication check consistent with the other endpoints.
🛡️ Proposed fix
.post("/", zValidator("form", logSchema), async (c) => {
+ const user = c.get("user");
+ if (!user) {
+ return c.json({ message: API_ERROR_MESSAGES.notAuthorized }, 401);
+ }
const logData = c.req.valid("form");🤖 Prompt for AI Agents
In `@apps/api/src/routes/log.ts` around lines 15 - 26, The POST handler
(logHandler.post using HonoBetterAuth, zValidator and logToDb) lacks any
authentication/authorization check and therefore allows unauthenticated writes;
update the logHandler.post implementation to perform the same user verification
and permission check used by the GET routes (the pattern used on the other
endpoints: verify the authenticated user exists and has the required
permissions), returning the same 401/403 responses when checks fail, then
proceed to call logToDb with the validated payload; ensure you reuse the same
auth/context properties and error responses as the GET routes so behavior is
consistent.
| } catch (e) { | ||
| if (e instanceof DatabaseError) { | ||
| if (e.code === "23505") { | ||
| await logWarning( | ||
| `User with ID ${user.id} is already a member of team with ID ${inviteRequest.teamId}. Transaction has been rolled back.`, | ||
| c, | ||
| ); | ||
| return c.json( | ||
| { message: API_ERROR_MESSAGES.alreadyMember }, | ||
| 400, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the package.json to see what database client is used
fd package.json -x cat {} | head -50Repository: acmutsa/Fallback
Length of output: 1138
🏁 Script executed:
# Search for database setup and imports in the codebase
rg -n 'libsql|postgres|sqlite|DATABASE|createClient' --type ts -i -A 2Repository: acmutsa/Fallback
Length of output: 8212
🏁 Script executed:
# Look for any existing DatabaseError or error code checks
rg -n 'DatabaseError|e\.code|error.*code' --type ts -C 2Repository: acmutsa/Fallback
Length of output: 1001
🏁 Script executed:
# Check if "23505" appears anywhere in the codebase
rg -n '23505' .Repository: acmutsa/Fallback
Length of output: 123
🌐 Web query:
libsql unique constraint violation error code SQLite
💡 Result:
In SQLite (and therefore libsql, which is SQLite-compatible), a UNIQUE constraint violation maps to:
- Primary result code:
SQLITE_CONSTRAINT= 19 (sqlite.org) - Extended result code (specific to UNIQUE):
SQLITE_CONSTRAINT_UNIQUE= 2067 (sqlite.org)
In libsql’s FFI/Rust layer, this is typically categorized as ErrorCode::ConstraintViolation (with SQLite-specific details available separately depending on the binding). (docs.rs)
Citations:
- 1: https://www.sqlite.org/rescode.html?utm_source=openai
- 2: https://www.sqlite.org/rescode.html?utm_source=openai
- 3: https://docs.rs/libsql-ffi/latest/libsql_ffi/enum.ErrorCode.html?utm_source=openai
🏁 Script executed:
# Check how LibsqlError is structured and what codes it uses
rg -n 'LibsqlError|error\.code|\.code ===' --type ts -B 2 -A 2 | head -60Repository: acmutsa/Fallback
Length of output: 716
🏁 Script executed:
# Look for the libsql client package to understand error structure
fd -e d.ts -e ts node_modules/@libsql -type f | head -10Repository: acmutsa/Fallback
Length of output: 230
🌐 Web query:
@libsql/client error.code property SQLite constraint UNIQUE
💡 Result:
In @libsql/client, query failures throw LibsqlError, which has:
error.code: a machine-readable primary code (string) (e.g."SQLITE_CONSTRAINT") [1]error.extendedCode: an optional, more specific SQLite extended code (string) (e.g."SQLITE_CONSTRAINT_UNIQUE") [1]error.rawCode: an optional numeric SQLite code (number). For UNIQUE violations, SQLite’s extended code is 2067 (SQLITE_CONSTRAINT_UNIQUE) [1][2]
So to detect a UNIQUE constraint violation:
import { LibsqlError } from "@libsql/client";
try {
await db.execute("INSERT INTO users(email) VALUES ('a@b.com')");
} catch (e) {
if (e instanceof LibsqlError) {
if (e.extendedCode === "SQLITE_CONSTRAINT_UNIQUE" || e.rawCode === 2067) {
// UNIQUE constraint failed
}
// fallback (less specific):
// if (e.code === "SQLITE_CONSTRAINT") { ... }
}
throw e;
}Sources: LibsqlError properties (code, extendedCode, rawCode) [1]; SQLite extended code list showing SQLITE_CONSTRAINT_UNIQUE (2067) [2].
[1] (tursodatabase.github.io)
[2] (sqlite.org)
Citations:
- 1: https://tursodatabase.github.io/libsql-client-ts/classes/LibsqlError.html
- 2: https://www.sqlite.org/rescode.html?utm_source=openai
🏁 Script executed:
# Final verification: check if there are any other UNIQUE constraints that might be affected
rg -n 'unique\|UNIQUE' packages/db/schema.ts -i -A 1 | grep -A 1 'userToTeam\|user_to_team'Repository: acmutsa/Fallback
Length of output: 42
🏁 Script executed:
# Verify the exact constraint definition on userToTeam table
sed -n '81,107p' packages/db/schema.tsRepository: acmutsa/Fallback
Length of output: 703
🏁 Script executed:
# Check the full context around the join operation to understand what constraint violation would occur
sed -n '115,155p' apps/api/src/routes/team.tsRepository: acmutsa/Fallback
Length of output: 1079
Wrong error code for libsql — "23505" is PostgreSQL, not SQLite. Non-matching errors are silently swallowed.
Two critical issues in this catch block:
-
Wrong error code:
"23505"is PostgreSQL's unique constraint violation code. This project uses SQLite via libsql (Turso), which returns error codes like"SQLITE_CONSTRAINT"(withextendedCode: "SQLITE_CONSTRAINT_UNIQUE"for UNIQUE/PRIMARY KEY violations). The checke.code === "23505"will never match, so duplicate membership inserts will not be caught. -
Silent error swallowing: If the error doesn't match the condition above, execution continues without re-throwing. Any unexpected database error (connection failure, transaction rollback, schema mismatch, etc.) is silently ignored, and the handler proceeds to line 155 as if the join succeeded.
Fix
} catch (e) {
if (e instanceof DatabaseError) {
- if (e.code === "23505") {
+ if (e.extendedCode === "SQLITE_CONSTRAINT_UNIQUE" || e.rawCode === 2067) {
await logWarning(
`User with ID ${user.id} is already a member of team with ID ${inviteRequest.teamId}. Transaction has been rolled back.`,
c,
);
return c.json(
{ message: API_ERROR_MESSAGES.alreadyMember },
400,
);
}
}
+ throw e;
}🤖 Prompt for AI Agents
In `@apps/api/src/routes/team.ts` around lines 140 - 153, The catch block handling
DatabaseError in the join flow incorrectly checks for Postgres code "23505" and
swallows other DB errors; update the condition to detect SQLite/libsql unique
constraint errors (check e.code === "SQLITE_CONSTRAINT" and/or e.extendedCode
=== "SQLITE_CONSTRAINT_UNIQUE") so duplicate-member inserts trigger the existing
logWarning and return of API_ERROR_MESSAGES.alreadyMember, and for any other
DatabaseError re-throw or return an appropriate 500 error instead of silently
continuing; modify the catch around DatabaseError (referencing DatabaseError,
e.code, e.extendedCode, logWarning, inviteRequest.teamId, user.id,
API_ERROR_MESSAGES.alreadyMember) to implement these checks and ensure
non-unique errors are not swallowed.
| .delete( | ||
| "/:teamId/:userId/remove", | ||
| zValidator("param", userTeamActionSchema), | ||
| async (c) => { | ||
| const teamId = c.req.valid("param").teamId; | ||
| const userIdToRemove = c.req.valid("param").userId; | ||
| const user = c.get("user"); | ||
|
|
||
| if (!user) { | ||
| return c.json( | ||
| { message: API_ERROR_MESSAGES.notAuthorized }, | ||
| 401, | ||
| ); | ||
| } | ||
|
|
||
| // If the user is attempting to remove themselves, we will allow this. | ||
| if (userIdToRemove === user.id) { | ||
| await leaveTeam(user.id, teamId); | ||
|
|
||
| return c.json( | ||
| { message: "Successfully removed from team." }, | ||
| 200, | ||
| ); | ||
| } | ||
|
|
||
| // If not, we know that it is a user attempting to remove another user and we need to ensure they have the right permissions for this. | ||
|
|
||
| const canUserRemove = await isUserSiteAdminOrQueryHasPermissions( | ||
| user.siteRole, | ||
| getAdminUserForTeam(user.id, teamId), | ||
| ); | ||
|
|
||
| if (!canUserRemove) { | ||
| return c.json( | ||
| { message: API_ERROR_MESSAGES.invalidPermissions }, | ||
| 401, | ||
| ); | ||
| } | ||
|
|
||
| // Lastly, if they are good, we can finally remove the user | ||
| await leaveTeam(userIdToRemove, teamId); | ||
|
|
||
| return c.json({ message: "Success" }, 200); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Self-removal path should verify the user is actually a team member before responding with success.
Line 314 calls leaveTeam(user.id, teamId) which runs a DELETE. If the user isn't a member, the DELETE succeeds silently (deletes 0 rows) and the handler returns "Successfully removed from team." Consider checking the delete result or pre-checking membership for accurate feedback.
🤖 Prompt for AI Agents
In `@apps/api/src/routes/team.ts` around lines 297 - 341, The self-removal branch
currently calls leaveTeam(user.id, teamId) and always returns success even if no
membership existed; update the handler to verify the user is a member before
confirming removal: either (A) pre-check membership with an existing
helper/query (e.g., call a function like isUserOnTeam(user.id, teamId) or a
direct membership lookup) and return a 404/appropriate error if not a member, or
(B) change leaveTeam to return the number of rows deleted (or a boolean) and,
after await leaveTeam(...), inspect the result and return a non-success response
when no rows were removed; keep the rest of the permission logic
(isUserSiteAdminOrQueryHasPermissions/getAdminUserForTeam) unchanged.
1 participant
This should 1000% split up into about 5 different PRs, but unfortunately I chipped away at this as I remembered things and just added and added until I created this absolute amalgamation of a PR. This should not be taken as an example of my engineering capabilities nor my organization and I intend to bury this PR deep within the commit history of this project.
(Btw some of this looks a bit incomplete that is because it is. This has no real environments attached to it yet and I cannot stand to add any more code to this than I absolutely have to lol)
...anyways
Satisfies