Adjustment to clans/hideouts #406
Conversation
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new mutation method Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/server/api/routers/clan.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: analyze
- GitHub Check: build-test
- GitHub Check: Analyze (javascript)
📦 Next.js Bundle Analysis for tnrThis analysis was generated by the Next.js Bundle Analysis action. 🤖 🎉 Global Bundle Size Decreased
DetailsThe global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster. Any third party scripts you have added directly to your app using the If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! |
|
@MathiasGruber Need you to approve this ASAP. We have some bad actors who are taking over clans kicking everyone and trying to sell the clan |
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
📦 Next.js Bundle Analysis for tnrThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
92.63 KB (🟡 +1 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
📦 Next.js Bundle Analysis for tnrThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/server/api/routers/clan.ts (1)
1182-1210: 🛠️ Refactor suggestionThis implementation addresses the urgent security concern but lacks important safeguards.
The new
instantJoinAndLeadmutation provides administrators with a way to take over problematic clans, which directly addresses the issue mentioned in the PR comments about bad actors. However, this implementation is missing several critical components that were suggested in previous reviews:
- No logging of admin actions
- No notifications to clan members
- No handling of existing leadership transition
- Inconsistent terminology between "faction" and "clan"
Consider implementing these improvements:
instantJoinAndLead: protectedProcedure .input(z.object({ clanId: z.string() })) .output(baseServerResponse) .mutation(async ({ ctx, input }) => { const [user, fetchedClan] = await Promise.all([ fetchUser(ctx.drizzle, ctx.userId), fetchClan(ctx.drizzle, input.clanId), ]); if (!fetchedClan) return errorResponse("Faction not found"); if (!user) return errorResponse("User not found"); if (!canEditClans(user.role)) return errorResponse("Permission denied"); if (user.clanId) return errorResponse("Already in a faction"); + const groupLabel = user?.isOutlaw ? "faction" : "clan"; + const prevLeaderId = fetchedClan.leaderId; await ctx.drizzle.transaction(async (tx) => { await tx .update(userData) .set({ clanId: fetchedClan.id, villageId: fetchedClan.village?.id }) .where(eq(userData.userId, user.userId)); await tx .update(clan) .set({ leaderId: user.userId }) .where(eq(clan.id, fetchedClan.id)); + + // Log the action + await tx.insert(actionLog).values({ + id: nanoid(), + userId: user.userId, + tableName: "clan", + changes: [`Admin ${user.username} took leadership of ${fetchedClan.name}`], + relatedId: fetchedClan.id, + relatedMsg: `Admin forced leadership change for ${groupLabel}: ${fetchedClan.name}`, + relatedImage: fetchedClan.image, + }); }); + // Notify clan members of the leadership change + fetchedClan.members.forEach((member) => { + void pusher.trigger(member.userId, "event", { + type: "userMessage", + message: `Admin ${user.username} has taken leadership of your ${groupLabel}.`, + route: "/clanhall", + routeText: `To ${groupLabel} Hall`, + }); + }); + + // Notify previous leader if exists + if (prevLeaderId && prevLeaderId !== user.userId) { + void pusher.trigger(prevLeaderId, "event", { + type: "userMessage", + message: `Admin ${user.username} has taken over your leadership of ${fetchedClan.name}.`, + route: "/clanhall", + routeText: `To ${groupLabel} Hall`, + }); + } return { success: true, - message: `You have instantly joined and taken leadership of ${fetchedClan.name}`, + message: `You have instantly joined and taken leadership of the ${groupLabel}: ${fetchedClan.name}`, }; }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/server/api/routers/clan.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/src/server/api/routers/clan.ts
[error] 1531-1531: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 1532-1562: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: analyze
- GitHub Check: build-test
- GitHub Check: Analyze (javascript)
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
📦 Next.js Bundle Analysis for tnrThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/server/api/routers/clan.ts (1)
1182-1210: 🛠️ Refactor suggestionImprove the instantJoinAndLead mutation with proper logging and notifications.
The new clan administration method lacks several important features present in other clan management functions:
- No action logging to the
actionLogtable- No notifications to clan members about the leadership change
- No check if the clan already has a leader before overwriting
- Inconsistent terminology between "faction" and "clan"
Apply these improvements to properly track changes and notify affected users:
instantJoinAndLead: protectedProcedure .input(z.object({ clanId: z.string() })) .output(baseServerResponse) .mutation(async ({ ctx, input }) => { const [user, fetchedClan] = await Promise.all([ fetchUser(ctx.drizzle, ctx.userId), fetchClan(ctx.drizzle, input.clanId), ]); if (!fetchedClan) return errorResponse("Faction not found"); if (!user) return errorResponse("User not found"); if (!canEditClans(user.role)) return errorResponse("Permission denied"); if (user.clanId) return errorResponse("Already in a faction"); + const groupLabel = user?.isOutlaw ? "faction" : "clan"; + const prevLeaderId = fetchedClan.leaderId; await ctx.drizzle.transaction(async (tx) => { await tx .update(userData) .set({ clanId: fetchedClan.id, villageId: fetchedClan.village?.id }) .where(eq(userData.userId, user.userId)); await tx .update(clan) .set({ leaderId: user.userId }) .where(eq(clan.id, fetchedClan.id)); + + // Log the action + await tx.insert(actionLog).values({ + id: nanoid(), + userId: user.userId, + tableName: "clan", + changes: [`Admin ${user.username} took leadership of ${fetchedClan.name}`], + relatedId: fetchedClan.id, + relatedMsg: `Admin forced leadership change for ${groupLabel}: ${fetchedClan.name}`, + relatedImage: fetchedClan.image, + }); }); + // Notify clan members of the leadership change + fetchedClan.members.forEach((member) => { + void pusher.trigger(member.userId, "event", { + type: "userMessage", + message: `Admin ${user.username} has taken leadership of your ${groupLabel}.`, + route: "/clanhall", + routeText: `To ${groupLabel} Hall`, + }); + }); + + // Notify previous leader if exists + if (prevLeaderId && prevLeaderId !== user.userId) { + void pusher.trigger(prevLeaderId, "event", { + type: "userMessage", + message: `Admin ${user.username} has taken over your leadership of ${fetchedClan.name}.`, + route: "/clanhall", + routeText: `To ${groupLabel} Hall`, + }); + } return { success: true, - message: `You have instantly joined and taken leadership of ${fetchedClan.name}`, + message: `You have instantly joined and taken leadership of the ${groupLabel}: ${fetchedClan.name}`, }; }),
🧹 Nitpick comments (2)
app/src/server/api/routers/clan.ts (2)
1192-1193: Add permission verification message.The error message "Permission denied" is generic. It would be better to provide more specific information about the required permissions.
- if (!canEditClans(user.role)) return errorResponse("Permission denied"); + if (!canEditClans(user.role)) return errorResponse("Admin permissions required to use this function");
1195-1204: Add error handling for the transaction.The current transaction implementation lacks proper error handling. If one of the updates fails, there's no explicit rollback or error reporting.
- await ctx.drizzle.transaction(async (tx) => { - await tx - .update(userData) - .set({ clanId: fetchedClan.id, villageId: fetchedClan.village?.id }) - .where(eq(userData.userId, user.userId)); - await tx - .update(clan) - .set({ leaderId: user.userId }) - .where(eq(clan.id, fetchedClan.id)); - }); + try { + await ctx.drizzle.transaction(async (tx) => { + const userResult = await tx + .update(userData) + .set({ clanId: fetchedClan.id, villageId: fetchedClan.village?.id }) + .where(eq(userData.userId, user.userId)); + + if (userResult.rowsAffected === 0) { + throw new Error("Failed to update user data"); + } + + const clanResult = await tx + .update(clan) + .set({ leaderId: user.userId }) + .where(eq(clan.id, fetchedClan.id)); + + if (clanResult.rowsAffected === 0) { + throw new Error("Failed to update clan leadership"); + } + + // Log the action here + }); + } catch (error) { + console.error("Transaction failed:", error); + return errorResponse(`Failed to join and lead clan: ${error.message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/drizzle/constants.ts(2 hunks)app/src/layout/Clan.tsx(2 hunks)app/src/server/api/routers/clan.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: analyze
- GitHub Check: build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
app/drizzle/constants.ts (1)
347-362:❓ Verification inconclusive
Element consistency validation needed.
The "Boil" element has been added to the
ElementNamesarray, but the additions seem incomplete. Most elements have corresponding image constants and possibly other related values.Ensure that all required constants for the new "Boil" element have been defined and initialized properly throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check for any other references to elements that might need updating grep -r "ElementNames" --include="*.ts" --include="*.tsx" .Length of output: 1868
Action: Validate Complete Integration of "Boil" Element
- In
app/drizzle/constants.ts(lines 347–362), "Boil" has been added to theElementNamesarray.- Our grep search confirms that this array is imported and used across combat types, UI components, and schema definitions.
- However, there’s no evidence in the codebase (e.g., in files like
app/src/layout/ElementImage.tsx) that corresponding image constants or additional properties have been defined for "Boil"—unlike the other elements that likely have associated assets and configurations.Please verify that:
- The "Boil" element also has its related image constant or configuration defined in the appropriate module(s).
- All parts of the code (such as zod schemas in
app/drizzle/schema.tsand UI components) that depend on complete element definitions are updated accordingly.app/src/layout/Clan.tsx (2)
798-807: Well-implemented mutation function.The
instantJoinAndLeadmutation is properly structured with appropriate cache invalidation for user profile and clan data after successful operations. It also includes proper redirection to the clan hall page.
1160-1175: Good security enhancement using role-based access control.Replacing the previous condition with
canEditClans(userData.role)ensures that only users with proper administrative permissions can perform this action. The confirmation dialog is well designed with clear messaging about the action being taken.This implementation addresses the urgent issue mentioned in the PR objectives regarding bad actors taking control of clans. The ability for administrators to instantly take leadership will help safeguard clan communities.
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
📦 Next.js Bundle Analysis for tnrThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
92.63 KB (🟡 +2 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Signed-off-by: Phrosfire <65364337+Phrosfire@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/drizzle/constants.ts(2 hunks)app/src/layout/ElementImage.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: analyze
- GitHub Check: build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
app/src/layout/ElementImage.tsx (2)
37-37: New element constant added.The
IMG_ELEMENT_BOILconstant is correctly imported from the constants file to support the new Boil element.
87-88: New element case handled in switch statement.A case for the "Boil" element has been added to the
getElementImgfunction, which correctly returns the importedIMG_ELEMENT_BOILconstant. This follows the same pattern used for other elements.app/drizzle/constants.ts (1)
361-361: Added "Boil" to ElementNames array."Boil" has been correctly added to the
ElementNamesarray, which defines the available element types in the application. The placement is logical and consistent with the existing array structure.
📦 Next.js Bundle Analysis for tnrThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
92.63 KB (🟡 +2 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
| if (!canEditClans(user.role)) return errorResponse("Permission denied"); | ||
| if (user.clanId) return errorResponse("Already in a faction"); | ||
|
|
||
| await ctx.drizzle.transaction(async (tx) => { |
There was a problem hiding this comment.
We should avoid using transactions on the database; they work with planetscale, but I've consistently experienced when we introduce transactions we also start seeing deadlocks popping up. As for this specific case, I think simply putting the two transactions into a Promise.all is fine :)
| @@ -903,6 +904,8 @@ export const IMG_ELEMENT_DUST = | |||
| "https://utfs.io/f/Hzww9EQvYURJchNmlmSnxBpQqGNDcTHbLmYz8uXAl3oa54ti"; | |||
| export const IMG_ELEMENT_LIGHTNING = | |||
| "https://utfs.io/f/Hzww9EQvYURJ4DIVIclYIif5CL8BKvMsOh2ZnmS7yHt0jTD3"; | |||
| export const IMG_ELEMENT_BOIL = | |||
| "https://i.imgur.com/63KgdPB.png"; | |||
There was a problem hiding this comment.
| @@ -903,6 +904,8 @@ export const IMG_ELEMENT_DUST = | |||
| "https://utfs.io/f/Hzww9EQvYURJchNmlmSnxBpQqGNDcTHbLmYz8uXAl3oa54ti"; | |||
| export const IMG_ELEMENT_LIGHTNING = | |||
| "https://utfs.io/f/Hzww9EQvYURJ4DIVIclYIif5CL8BKvMsOh2ZnmS7yHt0jTD3"; | |||
| export const IMG_ELEMENT_BOIL = | |||
There was a problem hiding this comment.
Also add to assets/elements/ folder for future recoverability :)
Pull Request
Multiple changes to clans/hideouts. Some changes are for admins and others are for the inner workings.
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