Open
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds invites-based team joining and full join-request lifecycle with auth checks and transactions, new DB helpers and relations, a migration adding joinedOn, schema and validation updates, client route/endpoint adjustments, and new error codes. 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, owner/user match, not expired)
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: fetch findTeamUserFacing(teamId)
DB-->>API: Team (id, name)
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: fetch findTeamUserFacing(teamId)
DB-->>API: Team (id, name)
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.
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
1 participant
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