Create Join Team Join Request Routes#50
Merged
christianhelp merged 14 commits intomainfrom Feb 21, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds invite-accept and full join-request lifecycle (create/list/approve/reject/rescind) with auth checks and transactions; new DB helper queries and relations; migration adding Changes
Sequence DiagramssequenceDiagram
participant User
participant API as API Server
participant DB as Database
User->>API: POST /invites/:inviteId/accept
API->>DB: Validate invite (exists, not expired, unused)
DB-->>API: Invite record
API->>DB: BEGIN TRANSACTION
API->>DB: Insert `user_to_team` (membership with joined_on)
API->>DB: Update invite status -> ACCEPTED
API->>DB: COMMIT
DB-->>API: Success
API->>DB: findTeam(teamId)
DB-->>API: Team data
API-->>User: 200 + team info
sequenceDiagram
participant Admin
participant API as API Server
participant DB as Database
Admin->>API: POST /team/:teamId/requests/:requestId/approve
API->>DB: Verify admin permissions
DB-->>API: Authorization OK
API->>DB: BEGIN TRANSACTION
API->>DB: Insert `user_to_team` (membership)
API->>DB: Update request status -> APPROVED
API->>DB: COMMIT
DB-->>API: Success
API->>DB: findTeam(teamId)
DB-->>API: Team data
API-->>Admin: 200 + team info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 6
🧹 Nitpick comments (4)
apps/api/src/routes/team.ts (2)
688-697: Redundant ownership check —getJoinTeamRequestalready filters byuserId.Since
getJoinTeamRequestincludeseq(teamJoinRequest.userId, userId)in its where clause, if a result is returned,joinRequest.userId === user.idis guaranteed. This check can never be false. It's harmless but dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/team.ts` around lines 688 - 697, The ownership if-block checking "if (joinRequest.userId !== user.id) { ... }" is redundant because getJoinTeamRequest already filters by eq(teamJoinRequest.userId, userId); remove that entire conditional branch from the handler in team.ts so the flow proceeds directly after the validated joinRequest. Locate the check that references joinRequest.userId and API_ERROR_MESSAGES.NOT_AUTHORIZED and delete the conditional and its return to eliminate dead code.
311-357: Admin route for listing join requests exposes all statuses without filtering.This returns all join requests (PENDING, APPROVED, REJECTED, RESCINDED) for the team. Consider whether the admin view should default to filtering by
PENDINGstatus, since that's the actionable set. If all statuses are intentional, adding a query parameter for status filtering could be useful as the list grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/team.ts` around lines 311 - 357, The current GET "/:teamId/requests" handler returns all join request statuses; update the handler that calls db.query.teamJoinRequest.findMany to default-filter by teamJoinRequest.status === "PENDING" (or enum value used) and/or accept an optional query parameter (e.g., c.req.query.status) to override the filter; specifically modify the where clause passed to db.query.teamJoinRequest.findMany (and adjust any validation/zValidator for an optional status param) so the default admin view shows only PENDING requests while still allowing explicit status filtering when provided.packages/db/schema.ts (1)
74-79:teamRelationsis missing amany(teamJoinRequest)entry.The new
teamJoinRequestRelationscorrectly wires the "many-to-one" side, butteamRelationsdoesn't declare the inversemany(teamJoinRequest). This will prevent relational queries from theteamside (e.g.,db.query.team.findFirst({ with: { joinRequests: true } })). Currently the routes query from theteamJoinRequestside so it works, but the relation graph is incomplete.♻️ Add the reverse relation
export const teamRelations = relations(team, ({ many }) => ({ members: many(userToTeam), invites: many(teamInvite), + joinRequests: many(teamJoinRequest), logs: many(log), backupJobs: many(backupJob), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/schema.ts` around lines 74 - 79, teamRelations is missing the inverse relation for teamJoinRequest; update the relations(team, ...) object (teamRelations) to include the missing many(teamJoinRequest) entry (use the same property name used on the inverse side, e.g. joinRequests) so the team side can load join requests via queries like db.query.team.findFirst({ with: { joinRequests: true } }). Ensure you import or reference teamJoinRequest the same way other relations (members, invites, logs, backupJobs) are declared.apps/api/src/lib/functions/database.ts (1)
47-61: RemovefindTeamor defer its addition until needed.
findTeamis exported but has no callers anywhere in the codebase. OnlyfindTeamUserFacingis imported inteam.ts. If this function isn't used yet, remove it to avoid dead code pollution.🤖 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 47 - 61, Remove the unused exported function findTeam to avoid dead code: delete the findTeam function (the export of findTeam and its db.query.team.findFirst call) and keep the existing findTeamUserFacing function intact; if you prefer to keep a private helper for future use, convert findTeam to an unexported/internal function or add a TODO and comment explaining why it's retained, but do not leave an exported unused symbol.
🤖 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/database.ts`:
- Around line 82-94: The current getJoinTeamRequest function incorrectly filters
by teamJoinRequest.userId (requester's ID) but the approve/reject routes in
team.ts pass the admin's user.id; create a new admin-facing lookup (e.g.,
getJoinTeamRequestForAdmin(requestId, teamId)) that queries
db.query.teamJoinRequest.findFirst without the userId eq filter (only match id
and teamId), and update the approve and reject handlers to call this new
function; keep the existing getJoinTeamRequest(requestId, userId, teamId)
unchanged for rescind/owner-specific checks.
In `@apps/api/src/routes/team.ts`:
- Around line 699-717: Add a guard for joinRequest.status === "RESCINDED" in the
same block that checks APPROVED/REJECTED so the rescind route returns a 400 with
a clear message and an appropriate error code instead of allowing a duplicate
rescind; locate the status checks around joinRequest (in the rescind route in
team.ts) and add a branch returning c.json({ message: "Join request has already
been rescinded and cannot be rescinded.", code: API_ERROR_MESSAGES.RESCINDED },
400) to match the behavior of the approve/reject routes.
- Around line 451-455: The current call to getJoinTeamRequest(requestId,
user.id, teamId) uses user.id (the admin) and will always miss the original
request; replace it with the admin lookup helper
getJoinTeamRequestForAdmin(requestId, teamId) so the query does not filter by
the approver's userId, and make the identical change in the reject route where
getJoinTeamRequest is used; ensure you pass requestId and teamId to
getJoinTeamRequestForAdmin and update any variable names or null-check handling
around the returned joinRequest accordingly.
- Around line 358-416: The handler for POST "/:teamId/requests" inserts into
teamJoinRequest without confirming the team exists, causing an unhandled FK
error; before calling db.insert(teamJoinRequest).values(...).returning(), query
the teams table using the same teamId (e.g. db.query.team.findFirst or
equivalent) and if no team is found return a 404 JSON response with an
appropriate message/code (e.g. API_ERROR_MESSAGES.TEAM_NOT_FOUND), otherwise
proceed to the existing duplicate/PENDING checks and insertion; alternatively,
if you prefer catching DB errors, wrap the insert in a try/catch and translate
FK-violation errors into a friendly 404 response referencing teamId and
API_ERROR_MESSAGES.TEAM_NOT_FOUND.
In `@packages/db/drizzle/0011_robust_sabretooth.sql`:
- Line 1: The migration adds a NOT NULL `joined_on` column with
DEFAULT(current_timestamp) to `user_to_team`, which backfills existing rows with
the migration time; instead, implement a two-step migration: first add
`joined_on` as nullable (ALTER TABLE user_to_team ADD joined_on integer NULL
DEFAULT NULL), then run a backfill step to populate historical values where
available (or decide on a reasonable historical value) using an UPDATE on
`user_to_team`, and finally alter the column to NOT NULL with a
DEFAULT(current_timestamp) (ALTER TABLE ... MODIFY/ALTER COLUMN joined_on
integer NOT NULL DEFAULT (current_timestamp)); reference the `user_to_team`
table and `joined_on` column in the migration to locate and split the change.
In `@packages/db/schema.ts`:
- Line 128: Fix the typo in the inline comment "Team join and team invite use
different pattenrs to track status." by changing "pattenrs" to "patterns" so the
comment reads "Team join and team invite use different patterns to track
status."; locate the comment containing that exact sentence in
packages/db/schema.ts (the one referencing team join and team invite status
tracking) and update only the misspelled word.
---
Nitpick comments:
In `@apps/api/src/lib/functions/database.ts`:
- Around line 47-61: Remove the unused exported function findTeam to avoid dead
code: delete the findTeam function (the export of findTeam and its
db.query.team.findFirst call) and keep the existing findTeamUserFacing function
intact; if you prefer to keep a private helper for future use, convert findTeam
to an unexported/internal function or add a TODO and comment explaining why it's
retained, but do not leave an exported unused symbol.
In `@apps/api/src/routes/team.ts`:
- Around line 688-697: The ownership if-block checking "if (joinRequest.userId
!== user.id) { ... }" is redundant because getJoinTeamRequest already filters by
eq(teamJoinRequest.userId, userId); remove that entire conditional branch from
the handler in team.ts so the flow proceeds directly after the validated
joinRequest. Locate the check that references joinRequest.userId and
API_ERROR_MESSAGES.NOT_AUTHORIZED and delete the conditional and its return to
eliminate dead code.
- Around line 311-357: The current GET "/:teamId/requests" handler returns all
join request statuses; update the handler that calls
db.query.teamJoinRequest.findMany to default-filter by teamJoinRequest.status
=== "PENDING" (or enum value used) and/or accept an optional query parameter
(e.g., c.req.query.status) to override the filter; specifically modify the where
clause passed to db.query.teamJoinRequest.findMany (and adjust any
validation/zValidator for an optional status param) so the default admin view
shows only PENDING requests while still allowing explicit status filtering when
provided.
In `@packages/db/schema.ts`:
- Around line 74-79: teamRelations is missing the inverse relation for
teamJoinRequest; update the relations(team, ...) object (teamRelations) to
include the missing many(teamJoinRequest) entry (use the same property name used
on the inverse side, e.g. joinRequests) so the team side can load join requests
via queries like db.query.team.findFirst({ with: { joinRequests: true } }).
Ensure you import or reference teamJoinRequest the same way other relations
(members, invites, logs, backupJobs) are declared.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/lib/queries.ts (1)
43-60: Non-200 errors swallow the specific error codes added in this PR.The mutation throws a generic
"Something went wrong"for any non-200 response, including the newREJECTEDandJOIN_REQUEST_EXISTSerror codes added topackages/shared/constants.ts. Consumers of this mutation (e.g., the join UI) will have no way to distinguish "you're already in a request queue" from "request was rejected" from a network error.Consider reading the response body before throwing so callers can act on the specific code:
♻️ Suggested improvement
mutationFn: async () => { const response = await apiClient.team.invites[ ":inviteId" ].accept.$post({ param: { inviteId: inviteCode, }, }); if (response?.status === 200) { return response.json(); } - throw new Error("Something went wrong"); + const body = await response.json().catch(() => null); + throw new Error((body as any)?.message ?? "Something went wrong"); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/queries.ts` around lines 43 - 60, The joinTeamMutationclient's mutationFn currently throws a generic Error for any non-200 response, which hides specific error codes (e.g., REJECTED, JOIN_REQUEST_EXISTS); update mutationFn (inside joinTeamMutationclient) to await and parse the response body (call response.json() or similar) for non-200 responses from apiClient.team.invites[":inviteId"].accept.$post, then throw or return a structured error containing the parsed body (or rethrow the parsed error object) so callers can inspect the specific error code instead of always getting "Something went wrong".
🤖 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/web/src/routes/team/join.tsx`:
- Around line 18-22: RouteComponent currently renders a debug stub; replace it
with a real join-team UI that reads inviteId from Route.useSearch and implements
fetching/validating the invite, showing loading/error states, and rendering a
join form or CTA to accept the invite. Specifically, update RouteComponent to:
use Route.useSearch() to get inviteId, call your invite validation API (e.g.,
joinTeamByInvite or fetchInvite) on mount, show a spinner while loading, show a
clear error message if validation fails, and render a form/button to accept the
invite that calls the join API and then navigates on success; ensure form/button
has accessible labels and that inviteId is passed to the API. Keep the component
name RouteComponent and preserve usage of Route.useSearch so reviewers can
locate the change.
---
Nitpick comments:
In `@apps/web/src/lib/queries.ts`:
- Around line 43-60: The joinTeamMutationclient's mutationFn currently throws a
generic Error for any non-200 response, which hides specific error codes (e.g.,
REJECTED, JOIN_REQUEST_EXISTS); update mutationFn (inside
joinTeamMutationclient) to await and parse the response body (call
response.json() or similar) for non-200 responses from
apiClient.team.invites[":inviteId"].accept.$post, then throw or return a
structured error containing the parsed body (or rethrow the parsed error object)
so callers can inspect the specific error code instead of always getting
"Something went wrong".
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/api/src/routes/team.ts (2)
312-358:c.set("teamId", teamId)missing across all new/:teamId/*routes.The existing
GET /:teamIdand the invite-accept route both callc.set("teamId", ...)to populate logging context. The four new routes that receive:teamIdin their params (GET /:teamId/requests,POST /:teamId/requests,/approve,/reject,/rescind) all skip this, so anylogError/logWarningcalls within them won't include team context.♻️ Example fix (apply to each new route after extracting `teamId`)
.get("/:teamId/requests", zValidator("param", teamIdSchema), async (c) => { const teamId = c.req.valid("param").teamId; const user = c.get("user"); + c.set("teamId", teamId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/team.ts` around lines 312 - 358, After extracting the teamId in each new route that uses the :teamId param (specifically the GET handler for "/:teamId/requests" where you have const teamId = c.req.valid("param").teamId, and the corresponding POST "/:teamId/requests" and the approve/reject/rescind handlers), call c.set("teamId", teamId) immediately (i.e., right after you assign teamId and before any permission checks or potential logError/logWarning calls) so the request context includes the teamId for logging; update the same pattern in the functions/handlers handling POST /:teamId/requests, the approve handler, the reject handler, and the rescind handler.
744-744: Inconsistent response shape: bare id vs. named key used by the reject route.The reject route (Line 649) returns
{ data: { joinRequestId: ... } }, but the rescind route returns a bare{ data: rescindedRequest[0].id }. Aligning these makes client handling consistent.♻️ Proposed fix
- return c.json({ data: rescindedRequest[0].id }, 200); + return c.json({ data: { joinRequestId: rescindedRequest[0].id } }, 200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/team.ts` at line 744, The rescind route currently returns a bare id (return c.json({ data: rescindedRequest[0].id }, 200)) which is inconsistent with the reject route that returns { data: { joinRequestId: ... } }; update the rescind route in apps/api/src/routes/team.ts to return the id under the same key (joinRequestId) inside data — e.g., { data: { joinRequestId: rescindedRequest[0].id } } — so client handling is consistent with the reject route.
🤖 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/routes/team.ts`:
- Around line 467-490: When handling joinRequest.status in the approve route,
replace the incorrect error codes: for the branch checking joinRequest.status
=== "APPROVED" use API_ERROR_MESSAGES.APPROVED instead of
API_ERROR_MESSAGES.ALREADY_MEMBER, and for joinRequest.status === "RESCINDED"
use API_ERROR_MESSAGES.RESCINDED instead of API_ERROR_MESSAGES.REJECTED; make
the symmetric changes in the reject route where it currently emits
ALREADY_MEMBER/REJECTED for APPROVED/RESCINDED so both approve and reject
handlers consistently return the new API_ERROR_MESSAGES.APPROVED and
API_ERROR_MESSAGES.RESCINDED codes respectively.
- Around line 107-115: The error message returned when checking
invite.acceptedAt in the team route is misleading; update the response in the
route handler (the block that inspects invite.acceptedAt) to return a precise
message such as "This invite has already been used." instead of "User is already
a member of this team." and, if appropriate, use or add a clearer API error code
constant instead of API_ERROR_MESSAGES.ALREADY_MEMBER; modify the JSON payload
returned by that branch (the object with message and code) in
apps/api/src/routes/team.ts so it accurately reflects a used invite.
- Around line 509-521: The log incorrectly logs the admin's ID (user.id) when
handling the SQLITE_CONSTRAINT branch; update the logWarning call in the error
handling block (where maybeGetDbErrorCode(e) is checked) to reference the
joining user's ID (joinRequest.userId) instead of user.id so the message
correctly states which user was already a member of the team; keep the rest of
the message and context (joinRequest.teamId, transaction rollback) and leave the
c.json response unchanged.
- Around line 687-695: The ownership check branch is dead because
getJoinTeamRequest already filters by user.id and the existing !joinRequest
guard handles non-matching requests; remove the unreachable joinRequest.userId
!== user.id branch (the entire return c.json(...) block) so only the
!joinRequest check remains, keeping getJoinTeamRequest, joinRequest, and user.id
references intact.
---
Duplicate comments:
In `@apps/api/src/routes/team.ts`:
- Around line 359-418: The route handler for .post("/:teamId/requests") performs
an unprotected insert into teamJoinRequest
(db.insert(teamJoinRequest).values(...).returning()) without verifying the team
exists, so an invalid teamId will cause an unhandled FK violation; fix by either
(a) adding an existence check using db.query.team.findFirst (or findUnique) for
teamId early in the handler and return a 404/appropriate API_ERROR_MESSAGES if
not found, or (b) wrap the insert in a try/catch and map FK constraint errors
from the teamJoinRequest insert to a clear 404/validation response; reference
the teamJoinRequest insert and the route handler to locate where to add the
check/try-catch.
---
Nitpick comments:
In `@apps/api/src/routes/team.ts`:
- Around line 312-358: After extracting the teamId in each new route that uses
the :teamId param (specifically the GET handler for "/:teamId/requests" where
you have const teamId = c.req.valid("param").teamId, and the corresponding POST
"/:teamId/requests" and the approve/reject/rescind handlers), call
c.set("teamId", teamId) immediately (i.e., right after you assign teamId and
before any permission checks or potential logError/logWarning calls) so the
request context includes the teamId for logging; update the same pattern in the
functions/handlers handling POST /:teamId/requests, the approve handler, the
reject handler, and the rescind handler.
- Line 744: The rescind route currently returns a bare id (return c.json({ data:
rescindedRequest[0].id }, 200)) which is inconsistent with the reject route that
returns { data: { joinRequestId: ... } }; update the rescind route in
apps/api/src/routes/team.ts to return the id under the same key (joinRequestId)
inside data — e.g., { data: { joinRequestId: rescindedRequest[0].id } } — so
client handling is consistent with the reject route.
mjanderson1227
approved these changes
Feb 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/api/src/routes/team.ts (3)
117-126: Redundantinvite.expiresAt &&guard.
teamInvite.expiresAtis declared.notNull()in the schema, so the truthiness check beforeisPast(...)is alwaystrueand can be removed.♻️ Suggested cleanup
- if (invite.expiresAt && isPast(invite.expiresAt)) { + if (isPast(invite.expiresAt)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/team.ts` around lines 117 - 126, Remove the redundant truthiness check before calling isPast: the schema declares teamInvite.expiresAt as .notNull(), so delete the unnecessary "invite.expiresAt &&" guard and directly call isPast(invite.expiresAt) in the invite expiration check (refer to the invite.expiresAt usage and the isPast(...) call in the route handler within team.ts).
422-428: Missing error handling on the join-request insert.Every other write path in this file (invite-accept, approve) wraps the mutation in
try/catch. A transient DB error here propagates as an unhandled exception and produces an opaque 500. Given that two prior reads have confirmed preconditions, the failure surface is small — but it should still return a structured error.🛡️ Proposed fix
- const newJoinRequest = await db - .insert(teamJoinRequest) - .values({ - teamId, - userId: user.id, - }) - .returning(); - - return c.json({ data: { requestId: newJoinRequest[0].id } }, 200); + try { + const newJoinRequest = await db + .insert(teamJoinRequest) + .values({ + teamId, + userId: user.id, + }) + .returning(); + + return c.json({ data: { requestId: newJoinRequest[0].id } }, 201); + } catch (e) { + logError( + `Error creating join request for user ${user.id} on team ${teamId}: ${e}`, + c, + ); + return c.json( + { + message: "An error occurred while creating the join request. Please try again later.", + code: API_ERROR_MESSAGES.GENERIC_ERROR, + }, + 500, + ); + }(The diff also corrects the status code to
201 Createdfor a new resource.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/team.ts` around lines 422 - 428, Wrap the db.insert call that creates newJoinRequest (db.insert(teamJoinRequest).values({...}).returning()) in a try/catch like the other write paths (invite-accept, approve); on success return the 201 Created response as intended, and on error catch the exception, log or attach the error details, and return a structured JSON error response (e.g., { error: 'failed_to_create_join_request', message: err.message }) with an appropriate 5xx status so the failure is handled consistently.
710-710: Inconsistent response shape — rescind returns a raw string ID, not a named property.The reject route at line 636 returns
{ data: { joinRequestId: ... } }. The rescind route returns{ data: rescindedRequest[0].id }— a bare string. Clients that destructure the response by field name will break on the rescind path.♻️ Proposed fix
- return c.json({ data: rescindedRequest[0].id }, 200); + return c.json({ data: { joinRequestId: rescindedRequest[0].id } }, 200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/team.ts` at line 710, The rescind route returns a bare string id (rescindedRequest[0].id) which causes an inconsistent response shape vs the reject route; update the rescind response to match the reject route by returning an object with a named property (e.g., { data: { joinRequestId: ... } }). Locate the return in apps/api/src/routes/team.ts that references rescindedRequest and replace the raw id payload with the named field joinRequestId so clients receive a consistent { data: { joinRequestId: string } } shape.
🤖 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/routes/team.ts`:
- Around line 674-700: The rescind-route branch handling joinRequest.status uses
incorrect error codes: when status === "APPROVED" it returns
API_ERROR_MESSAGES.ALREADY_MEMBER but should return
API_ERROR_MESSAGES.ALREADY_APPROVED, and when status === "RESCINDED" it returns
API_ERROR_MESSAGES.REJECTED but should return API_ERROR_MESSAGES.RESCINDED;
update the two c.json responses so the returned code values match the status
checks (refer to joinRequest.status comparisons and API_ERROR_MESSAGES
constants) and keep the existing message and 400 status.
- Around line 506-550: The handler returns the pre-transaction joinRequest
variable (status still "PENDING") after updating the row inside db.transaction;
fix by returning the updated state: either use tx.update(teamJoinRequest).set({
status: "APPROVED" }).where(eq(teamJoinRequest.id, requestId)).returning({ id:
teamJoinRequest.id, status: teamJoinRequest.status }) and send that returned row
in c.json (mirrors reject route pattern), or simply override joinRequest.status
= "APPROVED" before the final c.json; update references to joinRequest, the
db.transaction block, tx.update(teamJoinRequest) and the final c.json to ensure
the response reflects the approved status.
In `@packages/db/schema.ts`:
- Line 92: joinedOn column uses standardDateFactory() (DEFAULT
current_timestamp) which will stamp all existing user_to_team rows with the
migration run-time; update the migration SQL that adds joinedOn to include a
clear comment explaining that existing rows are backfilled with the migration
timestamp and why that's acceptable (or note the alternative of adding a
nullable column and backfilling from a source if historical accuracy is
required), and reference the joinedOn definition and standardDateFactory() in
the migration so future maintainers understand the behavior.
---
Duplicate comments:
In `@apps/api/src/routes/team.ts`:
- Around line 601-624: The branch handling joinRequest.status in the reject
route returns incorrect error codes: update the APPROVED branch (where it
currently uses API_ERROR_MESSAGES.ALREADY_MEMBER) to use
API_ERROR_MESSAGES.ALREADY_APPROVED, and update the RESCINDED branch (where it
currently uses API_ERROR_MESSAGES.REJECTED) to use API_ERROR_MESSAGES.RESCINDED;
locate the checks on joinRequest.status in the team route (the reject handler
around the APPROVED/REJECTED/RESCINDED branches) and swap the referenced
API_ERROR_MESSAGES constants accordingly so the JSON responses emit the correct
codes.
---
Nitpick comments:
In `@apps/api/src/routes/team.ts`:
- Around line 117-126: Remove the redundant truthiness check before calling
isPast: the schema declares teamInvite.expiresAt as .notNull(), so delete the
unnecessary "invite.expiresAt &&" guard and directly call
isPast(invite.expiresAt) in the invite expiration check (refer to the
invite.expiresAt usage and the isPast(...) call in the route handler within
team.ts).
- Around line 422-428: Wrap the db.insert call that creates newJoinRequest
(db.insert(teamJoinRequest).values({...}).returning()) in a try/catch like the
other write paths (invite-accept, approve); on success return the 201 Created
response as intended, and on error catch the exception, log or attach the error
details, and return a structured JSON error response (e.g., { error:
'failed_to_create_join_request', message: err.message }) with an appropriate 5xx
status so the failure is handled consistently.
- Line 710: The rescind route returns a bare string id (rescindedRequest[0].id)
which causes an inconsistent response shape vs the reject route; update the
rescind response to match the reject route by returning an object with a named
property (e.g., { data: { joinRequestId: ... } }). Locate the return in
apps/api/src/routes/team.ts that references rescindedRequest and replace the raw
id payload with the named field joinRequestId so clients receive a consistent {
data: { joinRequestId: string } } shape.
Comment on lines
+506
to
+550
| try { | ||
| await db.transaction(async (tx) => { | ||
| await tx.insert(userToTeam).values({ | ||
| teamId, | ||
| userId: joinRequest.userId, | ||
| role: "MEMBER", | ||
| }); | ||
|
|
||
| await tx | ||
| .update(teamJoinRequest) | ||
| .set({ | ||
| status: "APPROVED", | ||
| }) | ||
| .where(eq(teamJoinRequest.id, requestId)); | ||
| }); | ||
| } catch (e) { | ||
| const errorCode = maybeGetDbErrorCode(e); | ||
| if (errorCode === "SQLITE_CONSTRAINT") { | ||
| logWarning( | ||
| `User with ID ${joinRequest.userId} is already a member of team with ID ${joinRequest.teamId}. Transaction has been rolled back.`, | ||
| c, | ||
| ); | ||
| return c.json( | ||
| { | ||
| message: "User is already a member of this team.", | ||
| code: API_ERROR_MESSAGES.ALREADY_MEMBER, | ||
| }, | ||
| 400, | ||
| ); | ||
| } | ||
| logError( | ||
| `Error occurred while user with ID ${joinRequest.userId} was attempting to accept join request for team with ID ${joinRequest.teamId}. Transaction has been rolled back. Error details: ${e}`, | ||
| c, | ||
| ); | ||
| return c.json( | ||
| { | ||
| message: | ||
| "An error occurred while attempting to accept the join request for the team. Please try again later.", | ||
| code: API_ERROR_MESSAGES.GENERIC_ERROR, | ||
| }, | ||
| 500, | ||
| ); | ||
| } | ||
|
|
||
| return c.json({ data: joinRequest }, 200); |
There was a problem hiding this comment.
Approve route returns a stale object — client receives status: "PENDING" after a successful approval.
joinRequest is fetched on line 465 before the transaction. The transaction updates the DB row to APPROVED, but line 550 returns the original, pre-update object, so the caller always sees status: "PENDING".
The simplest fix that stays consistent with how the reject route works is to return only the ID (using .returning() on the update inside the transaction), or to override the status in the returned payload:
🐛 Option A — mirror the reject route pattern (return ID only)
+ let approvedRequestId: string;
try {
await db.transaction(async (tx) => {
await tx.insert(userToTeam).values({
teamId,
userId: joinRequest.userId,
role: "MEMBER",
});
- await tx
+ const updated = await tx
.update(teamJoinRequest)
.set({ status: "APPROVED" })
- .where(eq(teamJoinRequest.id, requestId));
+ .where(eq(teamJoinRequest.id, requestId))
+ .returning();
+ approvedRequestId = updated[0].id;
});
} catch (e) { ... }
- return c.json({ data: joinRequest }, 200);
+ return c.json({ data: { joinRequestId: approvedRequestId! } }, 200);🐛 Option B — inline status override (minimal change)
- return c.json({ data: joinRequest }, 200);
+ return c.json({ data: { ...joinRequest, status: "APPROVED" as const } }, 200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/team.ts` around lines 506 - 550, The handler returns the
pre-transaction joinRequest variable (status still "PENDING") after updating the
row inside db.transaction; fix by returning the updated state: either use
tx.update(teamJoinRequest).set({ status: "APPROVED"
}).where(eq(teamJoinRequest.id, requestId)).returning({ id: teamJoinRequest.id,
status: teamJoinRequest.status }) and send that returned row in c.json (mirrors
reject route pattern), or simply override joinRequest.status = "APPROVED" before
the final c.json; update references to joinRequest, the db.transaction block,
tx.update(teamJoinRequest) and the final c.json to ensure the response reflects
the approved status.
Comment on lines
+674
to
+700
| if (joinRequest.status === "APPROVED") { | ||
| return c.json( | ||
| { | ||
| message: | ||
| "Join request has already been approved and cannot be rescinded.", | ||
| code: API_ERROR_MESSAGES.ALREADY_MEMBER, | ||
| }, | ||
| 400, | ||
| ); | ||
| } else if (joinRequest.status === "REJECTED") { | ||
| return c.json( | ||
| { | ||
| message: | ||
| "Join request has already been rejected and cannot be rescinded.", | ||
| code: API_ERROR_MESSAGES.REJECTED, | ||
| }, | ||
| 400, | ||
| ); | ||
| } else if (joinRequest.status === "RESCINDED") { | ||
| return c.json( | ||
| { | ||
| message: "Join request has already been rescinded.", | ||
| code: API_ERROR_MESSAGES.REJECTED, | ||
| }, | ||
| 400, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Wrong error codes in the rescind route for APPROVED and RESCINDED statuses.
- Line 679: returns
ALREADY_MEMBERwhen status isAPPROVED— should beALREADY_APPROVED. A frontend that handlesALREADY_APPROVEDto display "this request was approved" will fall through to a generic error handler. - Line 696: returns
REJECTEDwhen status is alreadyRESCINDED— should beRESCINDED.
🐛 Proposed fix
if (joinRequest.status === "APPROVED") {
return c.json(
{
message: "Join request has already been approved and cannot be rescinded.",
- code: API_ERROR_MESSAGES.ALREADY_MEMBER,
+ code: API_ERROR_MESSAGES.ALREADY_APPROVED,
},
400,
);
} else if (joinRequest.status === "REJECTED") {
...
} else if (joinRequest.status === "RESCINDED") {
return c.json(
{
message: "Join request has already been rescinded.",
- code: API_ERROR_MESSAGES.REJECTED,
+ code: API_ERROR_MESSAGES.RESCINDED,
},
400,
);
}📝 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.
Suggested change
| if (joinRequest.status === "APPROVED") { | |
| return c.json( | |
| { | |
| message: | |
| "Join request has already been approved and cannot be rescinded.", | |
| code: API_ERROR_MESSAGES.ALREADY_MEMBER, | |
| }, | |
| 400, | |
| ); | |
| } else if (joinRequest.status === "REJECTED") { | |
| return c.json( | |
| { | |
| message: | |
| "Join request has already been rejected and cannot be rescinded.", | |
| code: API_ERROR_MESSAGES.REJECTED, | |
| }, | |
| 400, | |
| ); | |
| } else if (joinRequest.status === "RESCINDED") { | |
| return c.json( | |
| { | |
| message: "Join request has already been rescinded.", | |
| code: API_ERROR_MESSAGES.REJECTED, | |
| }, | |
| 400, | |
| ); | |
| } | |
| const API_ERROR_MESSAGES = { | |
| // ... existing messages | |
| ALREADY_APPROVED: "ALREADY_APPROVED", | |
| RESCINDED: "RESCINDED", | |
| // ... rest of messages | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/team.ts` around lines 674 - 700, The rescind-route branch
handling joinRequest.status uses incorrect error codes: when status ===
"APPROVED" it returns API_ERROR_MESSAGES.ALREADY_MEMBER but should return
API_ERROR_MESSAGES.ALREADY_APPROVED, and when status === "RESCINDED" it returns
API_ERROR_MESSAGES.REJECTED but should return API_ERROR_MESSAGES.RESCINDED;
update the two c.json responses so the returned code values match the status
checks (refer to joinRequest.status comparisons and API_ERROR_MESSAGES
constants) and keep the existing message and 400 status.
| onDelete: "cascade", | ||
| }), | ||
| role: memberRoleType.notNull().default("MEMBER"), | ||
| joinedOn: standardDateFactory(), |
There was a problem hiding this comment.
joinedOn will be stamped with the migration run-time for all existing rows.
standardDateFactory() uses DEFAULT (current_timestamp), so the ALTER TABLE ADD COLUMN migration will backfill every existing user_to_team row with the timestamp of when the migration runs — not the user's actual join date. For a young codebase this is usually acceptable, but it's worth a comment in the migration SQL so future maintainers understand why historical rows have the same joinedOn.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/schema.ts` at line 92, joinedOn column uses standardDateFactory()
(DEFAULT current_timestamp) which will stamp all existing user_to_team rows with
the migration run-time; update the migration SQL that adds joinedOn to include a
clear comment explaining that existing rows are backfilled with the migration
timestamp and why that's acceptable (or note the alternative of adding a
nullable column and backfilling from a source if historical accuracy is
required), and reference the joinedOn definition and standardDateFactory() in
the migration so future maintainers understand the behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Users should be able to request to join a team if they are not a part of it and the frontend will prompt for this.
What
Added the following routes and their purpose:
POST-/team/requests)GET-/team/requests)POST-/team/:teamId/requests/:requestId/rescind)GET-/team/:teamId/requests)POST-/team/:teamId/requests/:requestId/approve)POST-team/:teamId/requests/:requestId/approve)Satisfies
#35
Summary by CodeRabbit
New Features
Clients
Data
Bug Fixes / Validation