Fix issue #305: Kage can delete Anbu before the 24 hour lock out is up #306
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces a new constant Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
📦 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! 🙌 |
![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 (2)
app/drizzle/constants.ts(1 hunks)app/src/server/api/routers/anbu.ts(3 hunks)
🔇 Additional comments (1)
app/drizzle/constants.ts (1)
445-445: LGTM! The constant is well-defined.The
ANBU_DELAY_SECSconstant is correctly defined with a clear calculation of 24 hours in seconds, properly grouped with other ANBU-related constants.
app/src/server/api/routers/anbu.ts
Outdated
| if (secondsFromDate(ANBU_DELAY_SECS, squad.createdAt) > new Date()) { | ||
| return errorResponse("Must wait 24 hours after squad creation"); | ||
| } |
There was a problem hiding this comment.
Fix the time comparison logic in the disbandment check.
The current time comparison logic is reversed. When secondsFromDate(ANBU_DELAY_SECS, squad.createdAt) returns a future date and it's greater than the current date, it means the waiting period hasn't elapsed yet. The condition should be:
-if (secondsFromDate(ANBU_DELAY_SECS, squad.createdAt) > new Date()) {
+if (new Date() < secondsFromDate(ANBU_DELAY_SECS, squad.createdAt)) {Also, consider enhancing the error message to show the remaining time:
- return errorResponse("Must wait 24 hours after squad creation");
+ const remainingTime = Math.ceil((secondsFromDate(ANBU_DELAY_SECS, squad.createdAt).getTime() - new Date().getTime()) / (1000 * 60 * 60));
+ return errorResponse(`Must wait ${remainingTime} more hours after squad creation`);📝 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.
| if (secondsFromDate(ANBU_DELAY_SECS, squad.createdAt) > new Date()) { | |
| return errorResponse("Must wait 24 hours after squad creation"); | |
| } | |
| if (new Date() < secondsFromDate(ANBU_DELAY_SECS, squad.createdAt)) { | |
| const remainingTime = Math.ceil((secondsFromDate(ANBU_DELAY_SECS, squad.createdAt).getTime() - new Date().getTime()) / (1000 * 60 * 60)); | |
| return errorResponse(`Must wait ${remainingTime} more hours after squad creation`); | |
| } |
|
Solution is incorrect, this check to see if the anbu was created in 24 hours. What should happen is that the Kage should not be able to delete any anbu squads until after the 24 hour period regardless of when the anbu was created @openhands-agent |
…lock out is up
|
Overview of Changes: ✅ Successfully Resolved:
The changes appear to have fully addressed the core issue by properly implementing the intended business rule. The solution is logically sound and there are no apparent remaining issues with the 24-hour restriction functionality. Recommendation: Monitor initial deployment to ensure the leaderUpdatedAt timestamp is being set and tracked correctly in all cases. |
📦 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! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/server/api/routers/anbu.ts (1)
250-252:⚠️ Potential issueFix the time comparison logic and enhance the error message.
The implementation correctly uses the Kage's appointment time instead of the squad's creation time. However, there are two issues to address:
- The time comparison logic is reversed. When
secondsFromDatereturns a future date and it's greater than the current date, it means the waiting period hasn't elapsed yet.- The error message could be more informative by showing the remaining time.
Apply this diff to fix both issues:
- if (user.village && secondsFromDate(ANBU_DELAY_SECS, user.village.leaderUpdatedAt) > new Date()) { - return errorResponse("Must have been kage for 24 hours"); + if (user.village && new Date() < secondsFromDate(ANBU_DELAY_SECS, user.village.leaderUpdatedAt)) { + const remainingTime = Math.ceil( + (secondsFromDate(ANBU_DELAY_SECS, user.village.leaderUpdatedAt).getTime() - new Date().getTime()) / + (1000 * 60 * 60) + ); + return errorResponse(`Must wait ${remainingTime} more hours before you can disband ANBU squads`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/server/api/routers/anbu.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: analyze
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
app/src/server/api/routers/anbu.ts (2)
12-12: LGTM! Import changes are well-organized.The new imports for
secondsFromDateandANBU_DELAY_SECSare correctly placed and necessary for implementing the time-based validation.Also applies to: 21-21
250-252: Verify leaderUpdatedAt is set during Kage appointment.Let's verify that the
leaderUpdatedAttimestamp is being set correctly when a new Kage is appointed.✅ Verification successful
leaderUpdatedAt is properly maintained during Kage changes
The timestamp is correctly set to the current date whenever:
- A new Kage takes the position
- An elder takes over after Kage resignation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for where leaderUpdatedAt is being set in the codebase echo "Searching for leaderUpdatedAt assignments..." rg -l "leaderUpdatedAt.*=" # Search for Kage appointment logic echo "Searching for Kage appointment logic..." ast-grep --pattern 'update($village).set({ kageId: $_, leaderUpdatedAt: $_ })'Length of output: 297
Script:
#!/bin/bash # Search for village updates with broader patterns echo "Searching for village updates..." rg -A 5 "village.*update" # Search for kage-related mutations echo "Searching for kage assignments..." rg -A 5 "kageId.*=" # Search for leader-related updates echo "Searching for leader updates..." rg -A 5 "leader.*update"Length of output: 19768
Script:
#!/bin/bash # Search for kage appointment/resignation logic echo "Searching for kage appointment code..." rg -A 10 "appoint.*kage|resign.*kage" # Search for village table updates with more context echo "Searching for village table updates..." ast-grep --pattern 'update(village).set({ $$$, kageId: $_ })' # Search for transaction blocks containing kage updates echo "Searching for kage update transactions..." rg -A 10 "transaction.*kage"Length of output: 1907
Script:
#!/bin/bash # Get the full implementation of takeKage mutation echo "Searching for takeKage implementation..." rg -B 5 -A 20 "takeKage.*mutation" # Search for any update statements in kage.ts echo "Searching for village updates in kage.ts..." rg -B 2 -A 5 "update.*village.*set" app/src/server/api/routers/kage.tsLength of output: 2312
| if (user.village && secondsFromDate(ANBU_DELAY_SECS, user.village.leaderUpdatedAt) > new Date()) { | ||
| return errorResponse("Must have been kage for 24 hours"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper error handling for missing village data.
The current implementation might fail if the village relation or leaderUpdatedAt field is undefined. Add proper error handling to ensure robustness.
+ if (!user.village?.leaderUpdatedAt) {
+ return errorResponse("Village leader information is not available");
+ }
if (user.village && secondsFromDate(ANBU_DELAY_SECS, user.village.leaderUpdatedAt) > new Date()) {📝 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.
| if (user.village && secondsFromDate(ANBU_DELAY_SECS, user.village.leaderUpdatedAt) > new Date()) { | |
| return errorResponse("Must have been kage for 24 hours"); | |
| } | |
| if (!user.village?.leaderUpdatedAt) { | |
| return errorResponse("Village leader information is not available"); | |
| } | |
| if (user.village && secondsFromDate(ANBU_DELAY_SECS, user.village.leaderUpdatedAt) > new Date()) { | |
| return errorResponse("Must have been kage for 24 hours"); | |
| } |
This pull request fixes #305.
The issue has been successfully resolved based on the changes made. The core problem was that Kages could delete ANBU squads immediately without waiting for the required 24-hour period. The fix directly addresses this by:
ANBU_DELAY_SECSthat defines exactly 24 hours in seconds (24 * 3600)disbandSquadfunction that compares the squad's creation time against the required delay period usingsecondsFromDate(ANBU_DELAY_SECS, squad.createdAt)The implementation uses proper time comparison logic and integrates with existing game systems. The check is performed before any deletion actions can occur, which means it will effectively prevent premature ANBU squad deletion. The error message clearly communicates the restriction to users.
This matches the expected behavior described in the issue where "Kage needs to wait 24 hours before Anbus are deleted" and prevents the reported bug where "Kage can delete Anbu before the 24 hour lock out is up."
Automatic fix generated by OpenHands 🙌
Summary by CodeRabbit
New Features
Bug Fixes