feat (clan): allow co leaders to view/accept/reject clan requests#1137
Open
ammarjai wants to merge 6 commits intostudie-tech:mainfrom
Open
feat (clan): allow co leaders to view/accept/reject clan requests#1137ammarjai wants to merge 6 commits intostudie-tech:mainfrom
ammarjai wants to merge 6 commits intostudie-tech:mainfrom
Conversation
|
@ammarjai is attempting to deploy a commit to the TheNinja-RPG Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
WalkthroughFrontend props and backend clan routes were changed to enforce leader/co‑leader authorization: ClanRequests now receives Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Frontend as "Frontend\n(app/src/layout/Clan.tsx)"
participant Server as "Server API\n(app/src/server/api/routers/clan.ts)"
participant DB as "DB / Services"
Client->>Frontend: open Requests tab / click accept/reject
Frontend->>Server: getRequests({ clanLeaderId })
Server->>DB: fetch clan by leader, fetch users (leader/requesters)
DB-->>Server: clan & user records
Server-->>Frontend: filtered requests based on isLeader/isColeader
Frontend->>Server: acceptRequest / rejectRequest (target userId)
Server->>DB: validate leadership (leader/co‑leader) via fetched records
DB-->>Server: mutation result
Server-->>Frontend: success or error (with groupLabel)
Frontend->>Client: render updated requests or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/layout/Clan.tsx (1)
722-726: TheuserIdoverride toclanLeaderIdis fragile — consider a dedicated prop instead.Passing
clanLeaderIdasuserIdtoUserRequestSystemtricks it into showing accept/reject buttons (since the requests havereceiverId === leaderId). This works but couples the behavior to an implementation detail ofUserRequestSystem's logic. If that component ever usesuserIdfor authorization or display purposes beyond button visibility, this will break silently.Consider adding an explicit
canManageRequestsboolean prop toUserRequestSystemto decouple this, or at minimum add a comment explaining why the leader's ID is passed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/layout/Clan.tsx` around lines 722 - 726, The current hack of passing clanLeaderId into UserRequestSystem via the userId prop is fragile; change the call site in Clan.tsx to pass the real current user (userData.userId) as userId and introduce a new explicit boolean prop (e.g., canManageRequests) on UserRequestSystem, setting it to isLeaderOrColeader so the component can show accept/reject controls without overloading userId; update the UserRequestSystem prop usage (and its props type/interface) to accept canManageRequests and use that flag for button visibility/management logic instead of comparing receiverId to userId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/server/api/routers/clan.ts`:
- Around line 445-455: The error message "Only clan leaders can reject requests"
is misleading because co-leaders (determined via checkCoLeader and the
isColeader flag) are also allowed; update the errorResponse call that follows
the isLeader/isColeader check to use a wording that includes co-leaders (e.g.,
"Only clan leaders or co-leaders can reject requests"), and make the identical
wording change in the corresponding accept path where the same
isLeader/isColeader authorization check and errorResponse are used; reference
the isLeader, isColeader, checkCoLeader, and errorResponse symbols when locating
the two spots to update.
- Around line 376-381: The getRequests query currently trusts the
client-supplied clanLeaderId and returns pending requests via fetchRequests,
allowing any authenticated user to enumerate requests; update getRequests to
perform an authorization guard before calling fetchRequests by verifying
ctx.userId is the clan leader or has a leader/co-leader role for that clan
(e.g., call an existing lookup helper or query the DB using ctx.drizzle to fetch
the clan/role for input.clanLeaderId and ensure it matches ctx.userId or a
co-leader role), and throw an unauthorized error if the check fails; only after
this guard succeeds should you call fetchRequests(ctx.drizzle, ["CLAN"], 3600 *
12, input.clanLeaderId).
---
Nitpick comments:
In `@app/src/layout/Clan.tsx`:
- Around line 722-726: The current hack of passing clanLeaderId into
UserRequestSystem via the userId prop is fragile; change the call site in
Clan.tsx to pass the real current user (userData.userId) as userId and introduce
a new explicit boolean prop (e.g., canManageRequests) on UserRequestSystem,
setting it to isLeaderOrColeader so the component can show accept/reject
controls without overloading userId; update the UserRequestSystem prop usage
(and its props type/interface) to accept canManageRequests and use that flag for
button visibility/management logic instead of comparing receiverId to userId.
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CL as Co-Leader/Leader (Client)
participant FE as ClanRequests Component
participant API as tRPC API
participant DB as Database
CL->>FE: View Requests Tab
FE->>API: getRequests({ clanLeaderId })
API->>DB: fetchUser(ctx.userId) || fetchClanByLeader(clanLeaderId)
DB-->>API: user, fetchedClan
alt User has no clan
API->>DB: fetchRequests(ctx.userId)
DB-->>API: user's own requests
else User is leader/co-leader
API->>DB: fetchRequests(clanLeaderId)
DB-->>API: clan requests (scoped to leader)
else User is not leader/co-leader
API-->>FE: empty array []
end
API-->>FE: requests[]
FE-->>CL: Display requests with Accept/Reject buttons
CL->>FE: Accept/Reject Request
FE->>API: acceptRequest/rejectRequest({ id })
API->>DB: fetchRequest(id)
API->>DB: fetchClanByLeader || fetchUser(ctx.userId)
DB-->>API: clan, user
API->>API: Check isLeader or isColeader
alt Authorized
API->>DB: updateRequestState
DB-->>API: success
API-->>FE: { success: true }
else Not Authorized
API-->>FE: error response
end
Last reviewed commit: cec1f5a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
376
to
381
| getRequests: protectedProcedure | ||
| .meta({ mcp: { enabled: true, description: "Get pending clan join requests" } }) | ||
| .query(async ({ ctx }) => { | ||
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId); | ||
| .input(z.object({ clanLeaderId: z.string() })) | ||
| .query(async ({ ctx, input }) => { | ||
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, input.clanLeaderId); | ||
| }), |
There was a problem hiding this comment.
Missing server-side authorization check
The getRequests endpoint now accepts clanLeaderId as client-provided input and passes it directly to fetchRequests, with no verification that the calling user (ctx.userId) is actually the leader or a co-leader of that clan. Previously, ctx.userId was used directly, which was inherently secure since it came from the authenticated session.
Any authenticated user can now call this endpoint with any clanLeaderId value and retrieve all clan join requests for that clan. While the data exposed (usernames, levels, ranks) is relatively low-sensitivity, this still violates the principle of least privilege.
Consider adding an authorization check:
Suggested change
| getRequests: protectedProcedure | |
| .meta({ mcp: { enabled: true, description: "Get pending clan join requests" } }) | |
| .query(async ({ ctx }) => { | |
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId); | |
| .input(z.object({ clanLeaderId: z.string() })) | |
| .query(async ({ ctx, input }) => { | |
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, input.clanLeaderId); | |
| }), | |
| getRequests: protectedProcedure | |
| .meta({ mcp: { enabled: true, description: "Get pending clan join requests" } }) | |
| .input(z.object({ clanLeaderId: z.string() })) | |
| .query(async ({ ctx, input }) => { | |
| const fetchedClan = await fetchClanByLeader(ctx.drizzle, input.clanLeaderId); | |
| const isLeader = ctx.userId === fetchedClan?.leaderId; | |
| const isColeader = checkCoLeader(ctx.userId, fetchedClan); | |
| if (!isLeader && !isColeader) { | |
| throw new Error("Only clan leaders or co-leaders can view requests"); | |
| } | |
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, input.clanLeaderId); | |
| }), |
This does add an extra DB query. An alternative approach that avoids the extra roundtrip: look up the clan by ctx.userId's membership and verify the leader ID matches, or restructure the query to derive clanLeaderId server-side from ctx.userId.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/clan.ts
Line: 376:381
Comment:
**Missing server-side authorization check**
The `getRequests` endpoint now accepts `clanLeaderId` as client-provided input and passes it directly to `fetchRequests`, with no verification that the calling user (`ctx.userId`) is actually the leader or a co-leader of that clan. Previously, `ctx.userId` was used directly, which was inherently secure since it came from the authenticated session.
Any authenticated user can now call this endpoint with any `clanLeaderId` value and retrieve all clan join requests for that clan. While the data exposed (usernames, levels, ranks) is relatively low-sensitivity, this still violates the principle of least privilege.
Consider adding an authorization check:
```suggestion
getRequests: protectedProcedure
.meta({ mcp: { enabled: true, description: "Get pending clan join requests" } })
.input(z.object({ clanLeaderId: z.string() }))
.query(async ({ ctx, input }) => {
const fetchedClan = await fetchClanByLeader(ctx.drizzle, input.clanLeaderId);
const isLeader = ctx.userId === fetchedClan?.leaderId;
const isColeader = checkCoLeader(ctx.userId, fetchedClan);
if (!isLeader && !isColeader) {
throw new Error("Only clan leaders or co-leaders can view requests");
}
return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, input.clanLeaderId);
}),
```
This does add an extra DB query. An alternative approach that avoids the extra roundtrip: look up the clan by `ctx.userId`'s membership and verify the leader ID matches, or restructure the query to derive `clanLeaderId` server-side from `ctx.userId`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+494
to
+495
| const isLeader = user.userId === fetchedClan?.leaderId; | ||
| const isColeader = checkCoLeader(user.userId, fetchedClan); |
There was a problem hiding this comment.
Null safety: fetchedClan used before guard
isLeader and isColeader are derived from fetchedClan on lines 494-495, but fetchedClan can be null (it's checked on line 497: if (!fetchedClan) return errorResponse(...)). If fetchedClan is null, then fetchedClan?.leaderId is undefined, so isLeader will be false, and checkCoLeader handles null/undefined gracefully by returning false. The guard on line 501 would then return "Only clan leaders can accept requests" — which is a misleading error message when the actual issue is "clan not found."
Consider moving the !fetchedClan guard before the leader/co-leader checks to ensure the correct error message is returned:
Suggested change
| const isLeader = user.userId === fetchedClan?.leaderId; | |
| const isColeader = checkCoLeader(user.userId, fetchedClan); | |
| const isLeader = user.userId === fetchedClan?.leaderId; | |
| const isColeader = checkCoLeader(user.userId, fetchedClan); | |
| // Guards | |
| if (!fetchedClan) return errorResponse(`${groupLabel} not found`); | |
| if (!requester) return errorResponse("Requester not found"); | |
| if (!leader) return errorResponse("Leader not found"); | |
| if (nMembers >= CLAN_MAX_MEMBERS) return errorResponse(`${groupLabel} is full`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/clan.ts
Line: 494:495
Comment:
**Null safety: `fetchedClan` used before guard**
`isLeader` and `isColeader` are derived from `fetchedClan` on lines 494-495, but `fetchedClan` can be `null` (it's checked on line 497: `if (!fetchedClan) return errorResponse(...)`). If `fetchedClan` is `null`, then `fetchedClan?.leaderId` is `undefined`, so `isLeader` will be `false`, and `checkCoLeader` handles `null`/`undefined` gracefully by returning `false`. The guard on line 501 would then return `"Only clan leaders can accept requests"` — which is a misleading error message when the actual issue is "clan not found."
Consider moving the `!fetchedClan` guard before the leader/co-leader checks to ensure the correct error message is returned:
```suggestion
const isLeader = user.userId === fetchedClan?.leaderId;
const isColeader = checkCoLeader(user.userId, fetchedClan);
// Guards
if (!fetchedClan) return errorResponse(`${groupLabel} not found`);
if (!requester) return errorResponse("Requester not found");
if (!leader) return errorResponse("Leader not found");
if (nMembers >= CLAN_MAX_MEMBERS) return errorResponse(`${groupLabel} is full`);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
449
to
452
| ]); | ||
| // Derived | ||
| const isLeader = user.userId === fetchedClan?.leaderId; | ||
| const isColeader = checkCoLeader(user.userId, fetchedClan); |
There was a problem hiding this comment.
Same null-safety concern in rejectRequest
Same pattern as in acceptRequest: fetchedClan can be null from fetchClanByLeader, and if it is, both isLeader and isColeader will be false, leading to the error "Only clan leaders can reject requests" — which is misleading when the actual problem is that no clan was found for the given leader ID.
Consider adding a !fetchedClan guard before using it:
Suggested change
| ]); | |
| // Derived | |
| const isLeader = user.userId === fetchedClan?.leaderId; | |
| const isColeader = checkCoLeader(user.userId, fetchedClan); | |
| const isLeader = user.userId === fetchedClan?.leaderId; | |
| const isColeader = checkCoLeader(user.userId, fetchedClan); | |
| if (!fetchedClan) { | |
| return errorResponse("Clan not found"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/clan.ts
Line: 449:452
Comment:
**Same null-safety concern in `rejectRequest`**
Same pattern as in `acceptRequest`: `fetchedClan` can be `null` from `fetchClanByLeader`, and if it is, both `isLeader` and `isColeader` will be `false`, leading to the error "Only clan leaders can reject requests" — which is misleading when the actual problem is that no clan was found for the given leader ID.
Consider adding a `!fetchedClan` guard before using it:
```suggestion
const isLeader = user.userId === fetchedClan?.leaderId;
const isColeader = checkCoLeader(user.userId, fetchedClan);
if (!fetchedClan) {
return errorResponse("Clan not found");
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
501
to
503
| if (!isLeader && !isColeader) { | ||
| return errorResponse("Only clan leaders can accept requests"); | ||
| } |
There was a problem hiding this comment.
Error message excludes co-leaders
The error message says "Only clan leaders can accept requests" but this PR specifically adds co-leader support. If a non-leader, non-co-leader user hits this guard, they'll see a message that doesn't mention co-leaders can also perform this action. The same issue exists in rejectRequest on line 454.
Suggested change
| if (!isLeader && !isColeader) { | |
| return errorResponse("Only clan leaders can accept requests"); | |
| } | |
| if (!isLeader && !isColeader) { | |
| return errorResponse("Only clan leaders or co-leaders can accept requests"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/clan.ts
Line: 501:503
Comment:
**Error message excludes co-leaders**
The error message says "Only clan leaders can accept requests" but this PR specifically adds co-leader support. If a non-leader, non-co-leader user hits this guard, they'll see a message that doesn't mention co-leaders can also perform this action. The same issue exists in `rejectRequest` on line 454.
```suggestion
if (!isLeader && !isColeader) {
return errorResponse("Only clan leaders or co-leaders can accept requests");
}
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/server/api/routers/clan.ts`:
- Around line 386-400: The code performs an unnecessary DB call fetching
`leader` via `fetchUser(ctx.drizzle, input.clanLeaderId)` only to compute
`groupLabel`, which is never used; remove the unused fetch and `groupLabel`
derivation by replacing the Promise.all with a single await for
`fetchClanByLeader(ctx.drizzle, input.clanLeaderId)` (keep `fetchedClan`), and
leave the existing guards and logic that use `fetchedClan`, `isLeader`
(user.userId === fetchedClan?.leaderId), `isColeader`
(checkCoLeader(user.userId, fetchedClan)), and the final
`fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, input.clanLeaderId)` call
unchanged.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/server/api/routers/clan.ts (1)
380-397: Sequential DB calls can be parallelized; single-itemPromise.allis redundant.Two observations:
Promise.all([singlePromise])on line 386 adds no concurrency benefit — just unwrap the await directly.- The
userfetch (line 381) andfetchClanByLeader(line 386) are sequential. SincefetchClanByLeaderdoesn't depend onuser, you can run them in parallel and branch afterward, saving a DB roundtrip for clan members.Suggested refactor
.query(async ({ ctx, input }) => { - // Main fetch for non clan user - const user = await fetchUser(ctx.drizzle, ctx.userId); - if (!user.clanId) { - return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId); - } - // Fetch for clan user with guards - const [fetchedClan] = await Promise.all([ - fetchClanByLeader(ctx.drizzle, input.clanLeaderId), - ]); + // Fetch user and clan in parallel + const [user, fetchedClan] = await Promise.all([ + fetchUser(ctx.drizzle, ctx.userId), + fetchClanByLeader(ctx.drizzle, input.clanLeaderId), + ]); + // Non-clan user: return their own requests + if (!user.clanId) { + return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId); + } // Derived const isLeader = user.userId === fetchedClan?.leaderId; const isColeader = checkCoLeader(user.userId, fetchedClan);As per coding guidelines: "Minimize DB roundtrips by preferring to reduce the number of database roundtrips over reducing the amount of data fetched; run queries in parallel with Promise.all()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/clan.ts` around lines 380 - 397, Run fetchUser and fetchClanByLeader in parallel instead of sequentially and remove the redundant Promise.all([...singlePromise]) wrapper: kick off both promises (fetchUser(ctx.drizzle, ctx.userId) and fetchClanByLeader(ctx.drizzle, input.clanLeaderId)) concurrently with Promise.all, then await their results into user and fetchedClan; keep the existing guards (checkCoLeader, isLeader, user.clanId) and return fetchRequests(...) for non-clan users (using ctx.userId) or for authorized clan members (using input.clanLeaderId). Ensure you replace the single-item Promise.all around fetchClanByLeader with the parallel pattern and update references to user and fetchedClan accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/server/api/routers/clan.ts`:
- Around line 380-397: Run fetchUser and fetchClanByLeader in parallel instead
of sequentially and remove the redundant Promise.all([...singlePromise])
wrapper: kick off both promises (fetchUser(ctx.drizzle, ctx.userId) and
fetchClanByLeader(ctx.drizzle, input.clanLeaderId)) concurrently with
Promise.all, then await their results into user and fetchedClan; keep the
existing guards (checkCoLeader, isLeader, user.clanId) and return
fetchRequests(...) for non-clan users (using ctx.userId) or for authorized clan
members (using input.clanLeaderId). Ensure you replace the single-item
Promise.all around fetchClanByLeader with the parallel pattern and update
references to user and fetchedClan accordingly.
app/src/server/api/routers/clan.ts
Outdated
Comment on lines
380
to
386
| // Main fetch for non clan user | ||
| const user = await fetchUser(ctx.drizzle, ctx.userId); | ||
| if (!user.clanId) { | ||
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId); | ||
| } | ||
| // Fetch for clan user with guards | ||
| const fetchedClan = await fetchClanByLeader(ctx.drizzle, input.clanLeaderId); |
There was a problem hiding this comment.
Sequential DB queries can be parallelized
For clan users, fetchUser and fetchClanByLeader are called sequentially but don't depend on each other — input.clanLeaderId comes from the client, not from user. Per the project's "Minimize DB Roundtrips" guideline, these can be parallelized with Promise.all to save a roundtrip. The trade-off is that fetchClanByLeader would also run for non-clan users (wasted query), but you could restructure to handle that:
Suggested change
| // Main fetch for non clan user | |
| const user = await fetchUser(ctx.drizzle, ctx.userId); | |
| if (!user.clanId) { | |
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId); | |
| } | |
| // Fetch for clan user with guards | |
| const fetchedClan = await fetchClanByLeader(ctx.drizzle, input.clanLeaderId); | |
| const [user, fetchedClan] = await Promise.all([ | |
| fetchUser(ctx.drizzle, ctx.userId), | |
| fetchClanByLeader(ctx.drizzle, input.clanLeaderId), | |
| ]); | |
| // Main fetch for non clan user | |
| if (!user.clanId) { | |
| return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId); | |
| } |
Context Used: Context from dashboard - CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/clan.ts
Line: 380:386
Comment:
**Sequential DB queries can be parallelized**
For clan users, `fetchUser` and `fetchClanByLeader` are called sequentially but don't depend on each other — `input.clanLeaderId` comes from the client, not from `user`. Per the project's "Minimize DB Roundtrips" guideline, these can be parallelized with `Promise.all` to save a roundtrip. The trade-off is that `fetchClanByLeader` would also run for non-clan users (wasted query), but you could restructure to handle that:
```suggestion
const [user, fetchedClan] = await Promise.all([
fetchUser(ctx.drizzle, ctx.userId),
fetchClanByLeader(ctx.drizzle, input.clanLeaderId),
]);
// Main fetch for non clan user
if (!user.clanId) {
return await fetchRequests(ctx.drizzle, ["CLAN"], 3600 * 12, ctx.userId);
}
```
**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=0827434e-cdb7-4415-b790-c54f1a8e8feb))
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
app/src/server/api/routers/clan.ts
Outdated
Comment on lines
522
to
523
| if (!isLeader && !isColeader) { | ||
| return errorResponse("Only clan leader or co-leaders can accept requests"); |
There was a problem hiding this comment.
Hardcoded "clan" ignores outlaw factions
The groupLabel variable is already computed on line 513 to differentiate between "clan" and "faction" for outlaws, and it's used in other error messages in this block (e.g., lines 518, 521, 526). However, this error message hardcodes "clan" instead of using groupLabel. The same issue exists in rejectRequest on line 475.
Suggested change
| if (!isLeader && !isColeader) { | |
| return errorResponse("Only clan leader or co-leaders can accept requests"); | |
| if (!isLeader && !isColeader) { | |
| return errorResponse(`Only ${groupLabel} leader or co-leaders can accept requests`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/clan.ts
Line: 522:523
Comment:
**Hardcoded "clan" ignores outlaw factions**
The `groupLabel` variable is already computed on line 513 to differentiate between "clan" and "faction" for outlaws, and it's used in other error messages in this block (e.g., lines 518, 521, 526). However, this error message hardcodes "clan" instead of using `groupLabel`. The same issue exists in `rejectRequest` on line 475.
```suggestion
if (!isLeader && !isColeader) {
return errorResponse(`Only ${groupLabel} leader or co-leaders can accept requests`);
```
How can I resolve this? If you propose a fix, please make it concise.
app/src/server/api/routers/clan.ts
Outdated
Comment on lines
474
to
475
| if (!isLeader && !isColeader) { | ||
| return errorResponse("Only clan leader or co-leaders can reject requests"); |
There was a problem hiding this comment.
Same hardcoded "clan" issue here
Same as in acceptRequest — groupLabel is available on line 468 but not used in this error message. For outlaw faction users, this would incorrectly say "clan" instead of "faction".
Suggested change
| if (!isLeader && !isColeader) { | |
| return errorResponse("Only clan leader or co-leaders can reject requests"); | |
| if (!isLeader && !isColeader) { | |
| return errorResponse(`Only ${groupLabel} leader or co-leaders can reject requests`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/clan.ts
Line: 474:475
Comment:
**Same hardcoded "clan" issue here**
Same as in `acceptRequest` — `groupLabel` is available on line 468 but not used in this error message. For outlaw faction users, this would incorrectly say "clan" instead of "faction".
```suggestion
if (!isLeader && !isColeader) {
return errorResponse(`Only ${groupLabel} leader or co-leaders can reject requests`);
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/server/api/routers/clan.ts (1)
461-478:leaderfetch is an unnecessary DB roundtrip inrejectRequest.
leader(line 464) is used solely forgroupLabelderivation (line 468) and a guard that can never trigger (fetchUserthrows on not-found, soif (!leader)at line 473 is unreachable). You can derivegroupLabelfromuser.isOutlawinstead — all members of an outlaw faction share the same flag — and drop theleaderfetch entirely.This saves one DB call per reject invocation. The same pattern is already used in other endpoints (e.g.,
createRequestat line 423 uses the acting user'sisOutlaw).Proposed fix
const request = await fetchRequest(ctx.drizzle, input.id, "CLAN"); // Secondary fetches - const [fetchedClan, leader, user] = await Promise.all([ + const [fetchedClan, user] = await Promise.all([ fetchClanByLeader(ctx.drizzle, request.receiverId), - fetchUser(ctx.drizzle, request.receiverId), fetchUser(ctx.drizzle, ctx.userId), ]); // Derived - const groupLabel = leader?.isOutlaw ? "faction" : "clan"; + const groupLabel = user?.isOutlaw ? "faction" : "clan"; const isLeader = user.userId === fetchedClan?.leaderId; const isColeader = checkCoLeader(user.userId, fetchedClan); // Guards if (!fetchedClan) return errorResponse(`${groupLabel} not found`); - if (!leader) return errorResponse("Leader not found"); if (!isLeader && !isColeader) { return errorResponse( `Only ${groupLabel} leader or co-leaders can reject requests`, ); }As per coding guidelines: "Minimize DB roundtrips by preferring to reduce the number of database roundtrips over reducing the amount of data fetched."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/clan.ts` around lines 461 - 478, The extra fetchUser call for leader in rejectRequest is unnecessary and causes an extra DB roundtrip; remove the leader variable and its fetchUser call (drop fetchUser(ctx.drizzle, request.receiverId) from the Promise.all alongside fetchClanByLeader/fetchedClan and fetchUser(ctx.drizzle, ctx.userId)), derive groupLabel from the acting user's isOutlaw (use user.isOutlaw instead of leader?.isOutlaw), and delete the now-unreachable guard if (!leader); keep existing checks using fetchedClan, user, isLeader (user.userId === fetchedClan?.leaderId) and isColeader (checkCoLeader) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/server/api/routers/clan.ts`:
- Around line 461-478: The extra fetchUser call for leader in rejectRequest is
unnecessary and causes an extra DB roundtrip; remove the leader variable and its
fetchUser call (drop fetchUser(ctx.drizzle, request.receiverId) from the
Promise.all alongside fetchClanByLeader/fetchedClan and fetchUser(ctx.drizzle,
ctx.userId)), derive groupLabel from the acting user's isOutlaw (use
user.isOutlaw instead of leader?.isOutlaw), and delete the now-unreachable guard
if (!leader); keep existing checks using fetchedClan, user, isLeader
(user.userId === fetchedClan?.leaderId) and isColeader (checkCoLeader) intact.
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.
Pull Request
Allow co leaders to view/accept/reject clan requests. No breaking changes. Not really sure this is the best way to do this, but I implement this hack(?) somewhat due to limitation in DB schema and I don't want to change that.
Output
License
By making this pull request, I confirm that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the Studie-Tech ApS organization has the copyright to use and modify my contribution for perpetuity.
Summary by CodeRabbit
New Features
Bug Fixes