GH-878 Add vanish permission check for player tab suggestions #878
Conversation
vLuckyyy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java (1)
40-44: Documentation is clear but could be more specificThe
@FeatureDocsprovides good context about the feature. Consider adding JavaDoc to explicitly document:@FeatureDocs( name = "Vanish tabulation", description = "EternalCore prevents non-admin players from seeing vanished players in the commands like /tpa." + " To re-enable this feature for specific players, grant them the eternalcore.vanish.see permission." ) + /** + * Checks if the given sender has permission to see vanished players. + * + * @param sender the command sender to check permissions for + * @return true if the sender can see vanished players, false otherwise + * @throws NullPointerException if sender is null + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java (1)
53-56: LGTM! Clean implementation of vanish permission checks
The implementation correctly filters player suggestions based on the sender's permissions and player's vanish status. The use of method reference Player::getName is a nice touch for cleaner code.
Let's verify the vanish permission implementation:
✅ Verification successful
Based on the search results, I can now generate the final response:
Verified: Vanish permission implementation is consistent across the codebase
The implementation aligns with other vanish-related checks in the codebase:
OnlinePlayersListCommandandOnlinePlayerCountCommandfilter vanished playersVanishServiceprovides consistent permission checking viacanSeeVanishedusing the correct permission constant- The tab completion logic correctly combines both checks (
canSeeVanishedandisVanished) which is more comprehensive than other usages that only checkisVanished
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the vanish permission implementation is consistent
# Look for other places where vanish permissions are checked to ensure consistency
# Search for vanish permission checks
rg -A 3 "canSeeVanished|isVanished"
# Search for similar player suggestion implementations
ast-grep --pattern 'getOnlinePlayers().stream()
.filter($$$)
.map($$$)
.collect($$$)'
Length of output: 4446
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java (2)
8-8: LGTM: Import is correctly placed and necessary
The CommandSender import is required for the new method parameter.
45-47: 🛠️ Refactor suggestion
Add null check and verify permission constant
The method should validate the sender parameter. Also, let's verify the permission constant exists.
public boolean canSeeVanished(CommandSender sender) {
+ if (sender == null) {
+ throw new NullPointerException("sender cannot be null");
+ }
return sender.hasPermission(VanishPermissionConstant.VANISH_SEE_PERMISSION);
}Let's verify the permission constant:
✅ Verification successful
Add null check for sender parameter
The permission constant VANISH_SEE_PERMISSION is properly defined in VanishPermissionConstant.java as a static final String with value "eternalcore.vanish.see". Only the null check for the sender parameter is needed.
public boolean canSeeVanished(CommandSender sender) {
+ if (sender == null) {
+ throw new NullPointerException("sender cannot be null");
+ }
return sender.hasPermission(VanishPermissionConstant.VANISH_SEE_PERMISSION);
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify VanishPermissionConstant exists and contains VANISH_SEE_PERMISSION
# Search for the constant definition
ast-grep --pattern 'class VanishPermissionConstant {
$$$
static final String VANISH_SEE_PERMISSION = $_;
$$$
}'
Length of output: 119
Script:
#!/bin/bash
# Let's try a different approach to find the constant class and its content
# First, let's find the file containing VanishPermissionConstant
fd "VanishPermissionConstant.java"
# Then let's look for any file containing VANISH_SEE_PERMISSION string
rg "VANISH_SEE_PERMISSION" -A 2 -B 2
Length of output: 1256
...nalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java
Outdated
Show resolved
Hide resolved
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java (1)
48-52: Consider enhancing the feature documentationThe documentation is good but could be more explicit about the default behavior.
Consider this improved version:
@FeatureDocs( name = "Vanish tabulation", - description = "EternalCore prevents non-admin players from seeing vanished players in the commands like /tpa." - + " To re-enable this feature for specific players, grant them the eternalcore.vanish.see permission." + description = "By default, EternalCore hides vanished players from command suggestions (e.g., /tpa)." + + " Players with the eternalcore.vanish.see permission can see vanished players in suggestions." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java
🔇 Additional comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java (1)
Line range hint 1-42: Consider separating permission checks into a dedicated service
The VanishService is currently handling both vanish state management and permission checks, which violates the Single Responsibility Principle. Consider:
- Creating a separate
VanishPermissionServicefor permission-related logic - Moving the
canSeeVanishedmethod to this new service - This would make the codebase more maintainable and testable
This addresses the previous review comment about mixing responsibilities between vanish and tabulation concerns.
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java (1)
59-62: Implementation looks good, with a minor suggestion for readability
The vanish permission check implementation is correct and efficient. Consider extracting the filter condition for better readability.
Consider this refactor for improved readability:
CommandSender sender = invocation.sender();
+ boolean canSeeVanished = this.vanishService.canSeeVanished(sender);
return this.server.getOnlinePlayers().stream()
- .filter(player -> this.vanishService.canSeeVanished(sender) || !this.vanishService.isVanished(player.getUniqueId()))
+ .filter(player -> canSeeVanished || !this.vanishService.isVanished(player.getUniqueId()))
.map(Player::getName)
.collect(SuggestionResult.collector());Let's verify the null safety of the vanishService methods:
✅ Verification successful
Implementation is safe and correctly handles null cases
The VanishService implementation shows proper null handling:
isVanished(UUID)safely handles null by checking if player is nullcanSeeVanished(CommandSender)uses Bukkit's permission API which is null-safe- The filter condition in PlayerArgument is safe as both service methods handle null cases appropriately
The suggested readability improvement remains valid but is optional:
CommandSender sender = invocation.sender();
+ boolean canSeeVanished = this.vanishService.canSeeVanished(sender);
return this.server.getOnlinePlayers().stream()
- .filter(player -> this.vanishService.canSeeVanished(sender) || !this.vanishService.isVanished(player.getUniqueId()))
+ .filter(player -> canSeeVanished || !this.vanishService.isVanished(player.getUniqueId()))
.map(Player::getName)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if vanishService methods handle null values properly
# Search for null checks in VanishService implementation
ast-grep --pattern 'boolean canSeeVanished(CommandSender $_) {
$$$
}'
# Search for UUID null handling
ast-grep --pattern 'boolean isVanished(UUID $_) {
$$$
}'
Length of output: 134
Script:
#!/bin/bash
# Let's try a different approach to find VanishService implementations and interfaces
# Find VanishService files
fd -e java "VanishService"
# Search for method implementations with broader pattern
rg "canSeeVanished|isVanished" -A 5
# Look for any null checks related to these methods
rg "Objects\.requireNonNull|@NotNull|@Nullable" -A 2 -B 2
Length of output: 24020
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Documentation