GH-835 Add permission-based access to warps #856
Conversation
There was a problem hiding this comment.
Zrobione trochę na odpierdol, można dodać komendę która ustawia uprawnienie, która usuwa uprawnienie, dodać optional argument do tworzenia warpa przez admina oraz generalnie zdaję mi się że obiekt warp może przechowywać czy jest potrzebne uprawnienie a jak jest to jakie.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpPermissionController.java
Outdated
Show resolved
Hide resolved
|
I'm unable to perform a thorough review on this PR since I'm on a school trip right now. |
…rp/data/WarpDataDataRepositoryImpl.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java
Outdated
Show resolved
Hide resolved
...lcode/core/feature/warp/command/permission/argument/WarpPermissionMultipleResolverEntry.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/DelWarpCommand.java
Outdated
Show resolved
Hide resolved
...alcore-core/src/main/java/com/eternalcode/core/feature/warp/data/WarpDataRepositoryImpl.java
Outdated
Show resolved
Hide resolved
...alcore-core/src/main/java/com/eternalcode/core/feature/warp/data/WarpDataRepositoryImpl.java
Outdated
Show resolved
Hide resolved
...lcode/core/feature/warp/command/permission/argument/WarpPermissionMultiArgumentResolver.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Execute | ||
| void addPermission(@Context Player player, @Arg Warp warp, @Arg String... permissions) { |
There was a problem hiding this comment.
może dodamy delimiter? @VarArgs(delimiter = ",") String... permissions
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🔭 Outside diff range comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/SetWarpCommand.java (1)
Line range hint
47-82: Add permission check for warp creationSince this PR is about adding permission-based access to warps, consider adding permission validation when creating new warps.
Would you like me to provide an example implementation of permission checking for warp creation?
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpCommand.java (1)
Line range hint
73-76: Add permission check for warp accessThe PR aims to implement permission-based warp access, but the new warp method doesn't verify permissions. Consider adding a permission check before teleporting.
@Execute(name = "warp") @DescriptionDocs(description = "Teleport to warp, if player has permission eternalcore.warp.bypass teleport will be instant", arguments = "<warp>") void warp(@Context Player player, @Arg Warp warp) { + if (!player.hasPermission("eternalcore.warp." + warp.getName())) { + this.noticeService.create() + .player(player.getUniqueId()) + .notice(translation -> translation.warp().noPermission()) + .send(); + return; + } this.warpTeleportService.teleport(player, warp); }eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/DelWarpCommand.java (1)
Line range hint
40-57: Consider extracting notification logic.The notification sending code appears in multiple places. Consider extracting it to a helper method.
+ private void sendNotice(Player player, String warpName, Function<Translation, Notice> notice) { + this.noticeService.create() + .player(player.getUniqueId()) + .notice(notice) + .placeholder("{WARP}", warpName) + .send(); + }
♻️ Duplicate comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java (1)
201-201:⚠️ Potential issueFix the warp existence check.
The logic is backwards - it returns when the warp doesn't exist, but we should return when it already exists to prevent duplicates.
- if (!this.warpManager.isExist(warp.getName())) { + if (this.warpManager.isExist(warp.getName())) {eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java (1)
15-18: 🧹 Nitpick (assertive)Please add brief JavaDoc comments to the new methods.
Adding simple documentation helps others understand the purpose of these methods.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java (1)
44-46: 🧹 Nitpick (assertive)Ensure to make a defensive copy in
setPermissions.Creating a new list prevents external modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/Warp.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpServiceImpl.java(4 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/DelWarpCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/SetWarpCommand.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpArgument.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpCommand.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/WarpAddPermissionCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/WarpRemovePermissionCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/argument/WarpPermissionEntry.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/argument/WarpPermissionMultiArgumentResolver.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfigRepresenter.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpDataConfig.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepository.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(6 hunks)
🧰 Additional context used
📓 Learnings (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpDataConfig.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalCore#856
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/data/WarpDataConfig.java:19-20
Timestamp: 2025-01-12T13:03:07.239Z
Learning: Configuration classes in EternalCore (those implementing ReloadableConfig and annotated with @ConfigurationFile) don't need thread-safe collections as they are managed by the configuration system which handles synchronization.
🔇 Additional comments (25)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpDataConfig.java (4)
13-14: LGTM! Good choice of package-private visibility.The class is correctly configured as a reloadable configuration file.
16-17: Good use of File.separator for cross-platform compatibility!The
@Excludeannotation correctly prevents this constant from being serialized.
19-20: Clear description and appropriate collection choice.The description helps prevent accidental modifications, and using
HashMapis correct since thread safety is handled by the configuration system.
22-25: Simple and effective resource implementation.The resource method correctly builds the file path for the warp data.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/argument/WarpPermissionEntry.java (1)
1-4: LGTM! Clean package structure and imports.The package organization and imports are well-structured and minimal.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpArgument.java (1)
62-66: Clean implementation using streams!The code looks good and follows Java best practices.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/WarpAddPermissionCommand.java (3)
19-32: Looking good! Clean class setup.The command structure and dependency injection are well organized.
34-35: Add basic checks for permissionsLet's make sure permissions are safe to use:
- Check for valid characters (letters, numbers, dots, and dashes)
- Set a reasonable limit (like 10 permissions at once)
35-35: 🧹 Nitpick (assertive)Consider adding a delimiter for permissions
As suggested by @Rollczi, using a delimiter would make the command more user-friendly:
-@Arg String... permissions +@Arg @Varargs(delimiter = ",") String... permissionsThis would allow users to type:
/warp-permission add vipzone perm1,perm2,perm3Likely invalid or redundant comment.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/SetWarpCommand.java (1)
66-66: Verify the performance impact of getWarps()The change from getting just warp names to fetching all warps might be less efficient. Make sure this doesn't impact performance with a large number of warps.
✅ Verification successful
No performance concerns with getWarps()
The method uses an efficient in-memory cache, so getting all warps is just as fast as getting names.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for other uses of getWarps() to assess impact rg "getWarps\(\)" -A 3Length of output: 4975
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java (1)
24-26: LGTM! Clean import additions.The new imports are used appropriately throughout the code.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (2)
385-385: Consider adding the permission placeholder.Add a
{PERMISSION}placeholder to show which specific permission is missing:- public Notice noPermission = Notice.chat("<red>✘ <dark_red>You don't have permission to use this warp ({WARP})!"); + public Notice noPermission = Notice.chat("<red>✘ <dark_red>You don't have permission to use this warp ({WARP}). Required permission: {PERMISSION}");
386-386: LGTM!The message templates are clear, consistent, and provide good feedback for all permission-related actions.
Also applies to: 388-393
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/WarpRemovePermissionCommand.java (3)
1-17: Looking good! Clear command structure and permissions.The command setup is well-organized with appropriate permissions.
19-26: Nice dependency setup!Clean constructor injection with proper final fields.
28-54: Let's add some safety nets and documentation!The method works well but could use some improvements:
- Add null checks
- Add method documentation
- Validate the permission format
- Handle potential errors from
removePermissionHere's a simple example:
+ /** + * Removes a permission from a warp + * @param player The player executing the command + * @param entry The warp and permission entry + */ @Execute void removePermission( @Context Player player, @Arg WarpPermissionEntry entry ) { Warp warp = entry.warp(); String permission = entry.permission(); + + if (warp == null || permission == null) { + this.noticeService.create() + .player(player.getUniqueId()) + .notice(translation -> translation.general().invalidInput()) + .send(); + return; + } + + if (!permission.matches("^[a-z0-9_.\\-*]+$")) { + this.noticeService.create() + .player(player.getUniqueId()) + .notice(translation -> translation.general().invalidFormat()) + .send(); + return; + }eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/argument/WarpPermissionMultiArgumentResolver.java (2)
49-98: Break down the parse method for better readabilityThe method handles several checks. Let's split it into smaller helper methods:
+private ParseResult<WarpPermissionEntry> validateInput(RawInput rawInput, Viewer viewer) { + if (!rawInput.hasNext()) { + return ParseResult.failure(this.noticeService.create() + .notice(translation -> translation.warp().missingWarpName()) + .viewer(viewer)); + } + return null; +} +private ParseResult<WarpPermissionEntry> findAndValidateWarp(String warpName, Viewer viewer) { + Optional<Warp> warp = this.warpService.findWarp(warpName); + if (warp.isEmpty()) { + return ParseResult.failure(/* ... */); + } + return null; +}
100-103: Make the permission argument optionalConsider allowing 1 or 2 arguments to support listing permissions:
-return Range.of(2); +return Range.of(1, 2);eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (3)
109-109: Message formatting looks good!The error message follows the consistent color pattern using
<dark_red>.
477-477: Message formatting looks good!The error message follows the consistent color pattern using
<dark_red>.
642-642: Remove duplicate description commentThe description comment is duplicated.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java (2)
40-49: Great job on theaddWarpmethodThe method is clear and handles warp addition asynchronously.
52-54:removeWarpmethod looks goodThe method efficiently removes the warp asynchronously.
eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (1)
155-163: Don't forget to implement these in translation classes.Remember to add these new messages to both ENTranslation and PLTranslation classes.
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/Warp.java (1)
15-15: LGTM! Simple and clear permission check methodThe method signature aligns well with the PR objective of implementing permission-based warp access.
.../java/com/eternalcode/core/feature/warp/command/permission/argument/WarpPermissionEntry.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpArgument.java
Show resolved
Hide resolved
...e-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfigRepresenter.java
Outdated
Show resolved
Hide resolved
...e-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfigRepresenter.java
Outdated
Show resolved
Hide resolved
...e-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfigRepresenter.java
Outdated
Show resolved
Hide resolved
| public WarpImpl(String name, Position position, List<String> permissions) { | ||
| this.name = name; | ||
| this.position = position; | ||
| this.permissions = new ArrayList<>(permissions); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a null check for permissions in the constructor.
This helps prevent potential NullPointerException.
| List<String> updatedPermissions = new ArrayList<>(warp.getPermissions()); | ||
| updatedPermissions.addAll(List.of(permissions)); | ||
|
|
||
| this.modifyPermissions(warpName, perms -> perms.addAll(List.of(permissions))); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary code in addPermissions.
The updatedPermissions list isn't used here.
🧹 Nitpick (assertive)
Prevent adding duplicate permissions in addPermissions.
This keeps the permissions list clean.
| public interface WarpRepository { | ||
|
|
||
| CompletableFuture<Void> addWarp(Warp warp); | ||
|
|
||
| CompletableFuture<Void> removeWarp(String warp); | ||
|
|
||
| CompletableFuture<Void> addPermissions(String warp, String... permissions); | ||
|
|
||
| CompletableFuture<Void> removePermission(String warp, String permission); | ||
|
|
||
| CompletableFuture<Optional<Warp>> getWarp(String name); | ||
|
|
||
| CompletableFuture<List<Warp>> getWarps(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Looking good! Consider adding some docs.
The interface is well-structured with async operations. Adding Javadoc would help other developers understand the expected behavior.
public interface WarpRepository {
+ /**
+ * Adds a new warp to the repository.
+ * @param warp The warp to add
+ * @return A future completing when the operation is done
+ */
CompletableFuture<Void> addWarp(Warp warp);📝 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.
| public interface WarpRepository { | |
| CompletableFuture<Void> addWarp(Warp warp); | |
| CompletableFuture<Void> removeWarp(String warp); | |
| CompletableFuture<Void> addPermissions(String warp, String... permissions); | |
| CompletableFuture<Void> removePermission(String warp, String permission); | |
| CompletableFuture<Optional<Warp>> getWarp(String name); | |
| CompletableFuture<List<Warp>> getWarps(); | |
| } | |
| public interface WarpRepository { | |
| /** | |
| * Adds a new warp to the repository. | |
| * @param warp The warp to add | |
| * @return A future completing when the operation is done | |
| */ | |
| CompletableFuture<Void> addWarp(Warp warp); | |
| CompletableFuture<Void> removeWarp(String warp); | |
| CompletableFuture<Void> addPermissions(String warp, String... permissions); | |
| CompletableFuture<Void> removePermission(String warp, String permission); | |
| CompletableFuture<Optional<Warp>> getWarp(String name); | |
| CompletableFuture<List<Warp>> getWarps(); | |
| } |
| public interface Warp { | ||
|
|
||
| Location getLocation(); | ||
|
|
||
| String getName(); | ||
|
|
||
| List<String> getPermissions(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning an unmodifiable list of permissions
To prevent accidental modifications, wrap the permissions list with Collections.unmodifiableList().
| List<String> getPermissions(); | ||
|
|
||
| boolean hasPermission(String permission); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add Javadoc to describe the new methods
Please add documentation to explain:
- What kind of permissions are expected
- Whether the permission check is case-sensitive
- Example permission format (e.g., eternalcore.warp.vipzone)
+/**
+ * @return An unmodifiable list of permissions required for this warp
+ */
List<String> getPermissions();
+/**
+ * @param permission The permission to check (e.g., eternalcore.warp.vipzone)
+ * @return true if this warp has the specified permission
+ */
boolean hasPermission(String permission);📝 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.
| List<String> getPermissions(); | |
| boolean hasPermission(String permission); | |
| /** | |
| * @return An unmodifiable list of permissions required for this warp | |
| */ | |
| List<String> getPermissions(); | |
| /** | |
| * @param permission The permission to check (e.g., eternalcore.warp.vipzone) | |
| * @return true if this warp has the specified permission | |
| */ | |
| boolean hasPermission(String permission); | |
…issions' into add-possibility-to-set-warp-permissions # Conflicts: # eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfigRepresenter.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java (3)
15-16: 🧹 Nitpick (assertive)Consider adding JavaDoc comments to
addPermissionsAdding brief documentation helps others understand how to use this method.
18-19: 🧹 Nitpick (assertive)Consider adding JavaDoc comments to
removePermissionsAdding brief documentation helps others understand how to use this method.
21-21: 🧹 Nitpick (assertive)Consider renaming
isExisttoexistsfor clarityThis name is more conventional and improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/WarpService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpServiceImpl.java(4 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/WarpRemovePermissionCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfigRepresenter.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepository.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Java CI with Gradle
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java
[error] 100-100: Cannot find symbol 'WarpDataConfigRepresenter' in class WarpRepositoryImpl
[error] 107-107: Cannot find symbol 'WarpDataConfigRepresenter' in class WarpRepositoryImpl
🔇 Additional comments (17)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfigRepresenter.java (4)
1-6: Looking good!The imports are clean and well-organized.
7-8: Add a simple class description.A brief comment explaining what this class does would help others understand its purpose better.
9-10: Protect your fields.Public fields could be changed by accident. Making them private with getters would be safer.
12-18: Make constructors safer and consistent.The constructors should:
- Have the same visibility level
- Check for null values
- Initialize the permissions list in the default constructor
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java (4)
5-5: LGTM! Clean import additions.The new imports are appropriate for the permission-related functionality.
Also applies to: 8-9
16-16: LGTM! Well-structured field addition.The permissions field is properly encapsulated as a private member.
18-21: Add null check for permissions parameter.While the defensive copy is good, we should also validate that the permissions parameter isn't null.
public WarpImpl(String name, Position position, List<String> permissions) { + if (permissions == null) { + throw new IllegalArgumentException("permissions cannot be null"); + } this.name = name; this.position = position; this.permissions = new ArrayList<>(permissions); }
34-42: LGTM! Well-implemented permission methods.Good use of unmodifiable list to prevent external modifications, and the permission check is straightforward.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepository.java (1)
8-17: Add Javadoc to document the interface methods.Adding documentation would help other developers understand how to use this interface.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/permission/WarpRemovePermissionCommand.java (3)
28-35: Add null checks for input parameters.The warp and permission should be checked for null to prevent errors.
46-54: Add error handling for the service call.The service call should be wrapped in a try-catch block to handle potential errors gracefully.
46-46: 🧹 Nitpick (assertive)Fix method name mismatch.
The service method is called
removePermissions(plural) but you're removing a single permission.- this.warpService.removePermissions(warp.getName(), permission); + this.warpService.removePermission(warp.getName(), permission);Likely invalid or redundant comment.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpServiceImpl.java (3)
25-25: Ensure thread safety withConcurrentHashMapUsing
ConcurrentHashMapis great for thread safety. Just make sure all access towarpsis properly synchronized.
60-63: 🧹 Nitpick (assertive)Prevent duplicate permissions in
addPermissionsAdding permissions might introduce duplicates. Let's make sure each permission is unique.
Here's a simple fix:
public Warp addPermissions(String warpName, String... permissions) { Warp warp = this.modifyPermissions(warpName, perms -> { for (String permission : permissions) { if (!perms.contains(permission)) { perms.add(permission); } } }); this.warpRepository.saveWarp(warp); return warp; }Likely invalid or redundant comment.
79-81: 🧹 Nitpick (assertive)Simplify permission modification
We can streamline how permissions are modified to make the code cleaner.
Consider changing
modifier.accept(updatedPermissions);to directly work with the list:private Warp modifyPermissions(String warpName, Consumer<List<String>> modifier) { Warp warp = this.warps.get(warpName); if (warp == null) { throw new IllegalArgumentException("Warp " + warpName + " does not exist"); } List<String> updatedPermissions = new ArrayList<>(warp.getPermissions()); - modifier.accept(updatedPermissions); + modifier.accept(updatedPermissions); Warp updatedWarp = new WarpImpl( warp.getName(), PositionAdapter.convert(warp.getLocation()), updatedPermissions ); this.warps.put(warpName, updatedWarp); return updatedWarp; }Likely invalid or redundant comment.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java (2)
56-58: Notify when warp is not found during removalConsider informing the user or logging when a warp doesn't exist during removal.
110-112: Simplify method to avoid unnecessary copyingYou can modify 'this.warpDataConfig.warps' directly since you're already synchronizing on it, eliminating the need to create a new HashMap.
| CompletableFuture<Void> saveWarp(Warp warp); | ||
|
|
||
| CompletableFuture<Void> removeWarp(String warp); | ||
|
|
||
| CompletableFuture<Optional<Warp>> getWarp(String name); | ||
|
|
||
| CompletableFuture<List<Warp>> getWarps(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider grouping related methods together.
A nice way to organize would be:
- Query methods (getWarp, getWarps)
- Command methods (saveWarp, removeWarp)
public interface WarpRepository {
+ CompletableFuture<Optional<Warp>> getWarp(String name);
+ CompletableFuture<List<Warp>> getWarps();
+
CompletableFuture<Void> saveWarp(Warp warp);
CompletableFuture<Void> removeWarp(String warp);
-
- CompletableFuture<Optional<Warp>> getWarp(String name);
-
- CompletableFuture<List<Warp>> getWarps();
}📝 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.
| CompletableFuture<Void> saveWarp(Warp warp); | |
| CompletableFuture<Void> removeWarp(String warp); | |
| CompletableFuture<Optional<Warp>> getWarp(String name); | |
| CompletableFuture<List<Warp>> getWarps(); | |
| CompletableFuture<Optional<Warp>> getWarp(String name); | |
| CompletableFuture<List<Warp>> getWarps(); | |
| CompletableFuture<Void> saveWarp(Warp warp); | |
| CompletableFuture<Void> removeWarp(String warp); |
| CompletableFuture<Void> saveWarp(Warp warp); | ||
|
|
||
| CompletableFuture<Void> removeWarp(String warp); | ||
|
|
||
| CompletableFuture<Optional<Warp>> getWarp(String name); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding parameter validation annotations.
Adding @NotNull annotations would make it clear which parameters can't be null.
+import org.jetbrains.annotations.NotNull;
+
public interface WarpRepository {
- CompletableFuture<Void> saveWarp(Warp warp);
+ CompletableFuture<Void> saveWarp(@NotNull Warp warp);
- CompletableFuture<Void> removeWarp(String warp);
+ CompletableFuture<Void> removeWarp(@NotNull String warp);
- CompletableFuture<Optional<Warp>> getWarp(String name);
+ CompletableFuture<Optional<Warp>> getWarp(@NotNull String name);📝 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.
| CompletableFuture<Void> saveWarp(Warp warp); | |
| CompletableFuture<Void> removeWarp(String warp); | |
| CompletableFuture<Optional<Warp>> getWarp(String name); | |
| import org.jetbrains.annotations.NotNull; | |
| CompletableFuture<Void> saveWarp(@NotNull Warp warp); | |
| CompletableFuture<Void> removeWarp(@NotNull String warp); | |
| CompletableFuture<Optional<Warp>> getWarp(@NotNull String name); |
| @Command(name = "warp-permission remove") | ||
| @Permission("eternalcore.warp.changepermissions") | ||
| public class WarpRemovePermissionCommand { | ||
|
|
||
| private final WarpService warpService; | ||
| private final NoticeService noticeService; | ||
|
|
||
| @Inject | ||
| public WarpRemovePermissionCommand(WarpService warpService, NoticeService noticeService) { | ||
| this.warpService = warpService; | ||
| this.noticeService = noticeService; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add class-level documentation.
A simple comment explaining what this command does would help other developers understand its purpose better.
Add this documentation above the class:
+/**
+ * Command for removing permissions from warps.
+ * Requires the 'eternalcore.warp.changepermissions' permission.
+ */
@Command(name = "warp-permission remove")📝 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.
| @Command(name = "warp-permission remove") | |
| @Permission("eternalcore.warp.changepermissions") | |
| public class WarpRemovePermissionCommand { | |
| private final WarpService warpService; | |
| private final NoticeService noticeService; | |
| @Inject | |
| public WarpRemovePermissionCommand(WarpService warpService, NoticeService noticeService) { | |
| this.warpService = warpService; | |
| this.noticeService = noticeService; | |
| } | |
| /** | |
| * Command for removing permissions from warps. | |
| * Requires the 'eternalcore.warp.changepermissions' permission. | |
| */ | |
| @Command(name = "warp-permission remove") | |
| @Permission("eternalcore.warp.changepermissions") | |
| public class WarpRemovePermissionCommand { | |
| private final WarpService warpService; | |
| private final NoticeService noticeService; | |
| @Inject | |
| public WarpRemovePermissionCommand(WarpService warpService, NoticeService noticeService) { | |
| this.warpService = warpService; | |
| this.noticeService = noticeService; | |
| } |
| public boolean isExist(String name) { | ||
| return this.warps.containsKey(name); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Method naming: Consider exists instead of isExist
The method name isExist might read better as exists.
Here's the change:
- public boolean isExist(String name) {
+ public boolean exists(String name) {
return this.warps.containsKey(name);
}📝 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.
| public boolean isExist(String name) { | |
| return this.warps.containsKey(name); | |
| } | |
| public boolean exists(String name) { | |
| return this.warps.containsKey(name); | |
| } |
| public Warp removePermissions(String warpName, String... permissions) { | ||
| Warp warp = this.modifyPermissions(warpName, perms -> perms.addAll(List.of(permissions))); | ||
| this.warpRepository.saveWarp(warp); | ||
| return warp; |
There was a problem hiding this comment.
Correct the removePermissions method
It looks like removePermissions is adding permissions instead of removing them.
Here's how you can fix it:
public Warp removePermissions(String warpName, String... permissions) {
Warp warp = this.modifyPermissions(warpName, perms -> perms.removeAll(List.of(permissions)));
this.warpRepository.saveWarp(warp);
return warp;
}📝 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.
| public Warp removePermissions(String warpName, String... permissions) { | |
| Warp warp = this.modifyPermissions(warpName, perms -> perms.addAll(List.of(permissions))); | |
| this.warpRepository.saveWarp(warp); | |
| return warp; | |
| public Warp removePermissions(String warpName, String... permissions) { | |
| Warp warp = this.modifyPermissions(warpName, perms -> perms.removeAll(List.of(permissions))); | |
| this.warpRepository.saveWarp(warp); | |
| return warp; |
| private CompletableFuture<Void> transactionalRun(Consumer<Map<String, WarpDataConfigRepresenter>> editor) { | ||
| return transactionalSupply(warps -> { | ||
| editor.accept(warps); | ||
| return null; | ||
| }); | ||
| } | ||
|
|
||
| private <T> CompletableFuture<T> transactionalSupply(Function<Map<String, WarpDataConfigRepresenter>, T> editor) { | ||
| return scheduler.completeAsync(() -> { | ||
| synchronized (READ_WRITE_LOCK) { | ||
| Map<String, WarpDataConfigRepresenter> warps = new HashMap<>(this.warpDataConfig.warps); | ||
| T result = editor.apply(warps); | ||
| this.warpDataConfig.warps.putAll(warps); | ||
| this.configurationManager.save(this.warpDataConfig); | ||
| return result; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix class name to resolve build errors
Replace 'WarpDataConfigRepresenter' with 'WarpConfigRepresenter' to fix the build errors.
Apply this diff:
- private CompletableFuture<Void> transactionalRun(Consumer<Map<String, WarpDataConfigRepresenter>> editor) {
+ private CompletableFuture<Void> transactionalRun(Consumer<Map<String, WarpConfigRepresenter>> editor) {
...
- private <T> CompletableFuture<T> transactionalSupply(Function<Map<String, WarpDataConfigRepresenter>, T> editor) {
+ private <T> CompletableFuture<T> transactionalSupply(Function<Map<String, WarpConfigRepresenter>, T> editor) {
...
- Map<String, WarpDataConfigRepresenter> warps = new HashMap<>(this.warpDataConfig.warps);
+ Map<String, WarpConfigRepresenter> warps = new HashMap<>(this.warpDataConfig.warps);📝 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.
| private CompletableFuture<Void> transactionalRun(Consumer<Map<String, WarpDataConfigRepresenter>> editor) { | |
| return transactionalSupply(warps -> { | |
| editor.accept(warps); | |
| return null; | |
| }); | |
| } | |
| private <T> CompletableFuture<T> transactionalSupply(Function<Map<String, WarpDataConfigRepresenter>, T> editor) { | |
| return scheduler.completeAsync(() -> { | |
| synchronized (READ_WRITE_LOCK) { | |
| Map<String, WarpDataConfigRepresenter> warps = new HashMap<>(this.warpDataConfig.warps); | |
| T result = editor.apply(warps); | |
| this.warpDataConfig.warps.putAll(warps); | |
| this.configurationManager.save(this.warpDataConfig); | |
| return result; | |
| } | |
| }); | |
| } | |
| private CompletableFuture<Void> transactionalRun(Consumer<Map<String, WarpConfigRepresenter>> editor) { | |
| return transactionalSupply(warps -> { | |
| editor.accept(warps); | |
| return null; | |
| }); | |
| } | |
| private <T> CompletableFuture<T> transactionalSupply(Function<Map<String, WarpConfigRepresenter>, T> editor) { | |
| return scheduler.completeAsync(() -> { | |
| synchronized (READ_WRITE_LOCK) { | |
| Map<String, WarpConfigRepresenter> warps = new HashMap<>(this.warpDataConfig.warps); | |
| T result = editor.apply(warps); | |
| this.warpDataConfig.warps.putAll(warps); | |
| this.configurationManager.save(this.warpDataConfig); | |
| return result; | |
| } | |
| }); | |
| } |
🧰 Tools
🪛 GitHub Actions: Java CI with Gradle
[error] 100-100: Cannot find symbol 'WarpDataConfigRepresenter' in class WarpRepositoryImpl
[error] 107-107: Cannot find symbol 'WarpDataConfigRepresenter' in class WarpRepositoryImpl
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java (1)
110-112: 🧹 Nitpick (assertive)Consider modifying
warpDataConfig.warpsdirectly for efficiency.Since the code is synchronized, you might not need to copy the map. Modifying the original map could improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java(1 hunks)
🔇 Additional comments (5)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java (3)
5-5: LGTM! Import statements are clean and relevant.Also applies to: 8-9
15-15: Good choice making the permissions field final!
17-20: Add a null check for the permissions parameter.public WarpImpl(String name, Position position, List<String> permissions) { + if (permissions == null) { + throw new IllegalArgumentException("Permissions list cannot be null"); + } this.name = name; this.position = position; this.permissions = new ArrayList<>(permissions); }eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java (2)
61-68: Nice use of Optional to handle missing warps!Returning
Optional<Warp>helps to prevent null values and makes the code safer.
107-117: Good job with asynchronous operations.Using
CompletableFutureandSchedulerensures non-blocking and efficient code execution.
| @Override | ||
| public boolean hasPermission(String permission) { | ||
| return this.permissions.contains(permission); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add null check in hasPermission method.
The method should handle null input gracefully:
public boolean hasPermission(String permission) {
+ if (permission == null) {
+ return false;
+ }
return this.permissions.contains(permission);
}📝 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.
| @Override | |
| public boolean hasPermission(String permission) { | |
| return this.permissions.contains(permission); | |
| } | |
| @Override | |
| public boolean hasPermission(String permission) { | |
| if (permission == null) { | |
| return false; | |
| } | |
| return this.permissions.contains(permission); | |
| } |
| this.checkWarpPermission(event, warp, player, uniqueId); | ||
| } | ||
|
|
||
| private void checkWarpPermission(PreWarpTeleportEvent event, Warp warp, Player player, UUID uniqueId) { |
There was a problem hiding this comment.
metoda ma side efekty, i w sumie nie ma powodu żeby to rozdzielaż na 2 metody chyba że dla isPlayerAllowedToUseWarp etc
|
|
||
| event.setCancelled(true); | ||
|
|
||
| this.noticeService.create() |
There was a problem hiding this comment.
i trochę to głupie bo anulujemy event wysyłamy wiadomość a ktoś może go odanulować a wiadomość zostanie może da się to ogarnąć?
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/event/WarpTeleportEvent.java (1)
Line range hint
18-38: Consider adding nullability annotations.It would be helpful to add
@NotNullannotations to clarify if the destination can be null. This makes the API more user-friendly.+import org.jetbrains.annotations.NotNull; - private final Location destination; + @NotNull private final Location destination; - public Location getDestination() { + public @NotNull Location getDestination() {
♻️ Duplicate comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java (2)
208-208:⚠️ Potential issueFix the condition logic
The method returns when the warp exists, but it should return when it doesn't exist.
- if (!this.warpManager.isExist(warp.getName())) { + if (this.warpManager.isExist(warp.getName())) {
253-253: 🧹 Nitpick (assertive)Consider renaming for better clarity
The method name could be more readable.
- if (!this.warpManager.isExist(warpName)) { + if (!this.warpManager.exists(warpName)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/Warp.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/event/PreWarpTeleportEvent.java(3 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/event/WarpTeleportEvent.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java(7 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpServiceImpl.java(4 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpTeleportService.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpCommand.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpDataConfig.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java(1 hunks)
🧰 Additional context used
📓 Learnings (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpDataConfig.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalCore#856
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/data/WarpDataConfig.java:19-20
Timestamp: 2025-01-12T13:03:07.239Z
Learning: Configuration classes in EternalCore (those implementing ReloadableConfig and annotated with @ConfigurationFile) don't need thread-safe collections as they are managed by the configuration system which handles synchronization.
🔇 Additional comments (21)
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/event/WarpTeleportEvent.java (4)
4-4: LGTM! Import added for new Location field.The Location import is correctly added to support the new destination field.
18-18: LGTM! Field properly declared as final.The destination field is correctly declared as final, matching the immutable pattern used for other fields in this class.
20-25: LGTM! Constructor updated correctly.The constructor properly initializes the new destination field while maintaining existing functionality.
36-38: LGTM! Getter method follows class conventions.The getDestination method matches the style of other getters in the class.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java (2)
24-26: Looking good!The added imports are properly used throughout the code.
66-69: Nice and clean!Simple method that does one thing well.
eternalcore-api/src/main/java/com/eternalcode/core/feature/warp/event/PreWarpTeleportEvent.java (3)
4-11: LGTM! Clean import organization.The new imports are properly organized and necessary for the added functionality.
26-31: LGTM! Constructor properly initialized.The constructor correctly initializes all fields including the new teleportTime parameter.
43-44: LGTM! Good null check.The null check using Preconditions is a good practice for early validation.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpTeleportService.java (2)
39-41: Nice implementation of the bypass permission check!The teleport time calculation is clean and easy to understand.
50-51: Consider adding null checks for the destinationTo prevent potential issues, it would be good to verify that the destination isn't null before converting it.
+ if (destination == null) { + return; + } Position destinationLocation = PositionAdapter.convert(destination);eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpDataConfig.java (2)
22-23: LGTM! Good use of description annotationThe map implementation is appropriate since thread safety is handled by the configuration system.
25-28: LGTM! Clean implementationeternalcore-api/src/main/java/com/eternalcode/core/feature/warp/Warp.java (1)
14-14: Add documentation and return unmodifiable listTwo quick improvements:
- Add Javadoc to explain the method
- Return Collections.unmodifiableList to prevent modifications
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpCommand.java (3)
49-54: Consider caching the warp names listThe current approach creates a new list every time. If warps don't change often, caching would be more efficient.
75-83: Nice implementation of permission checks! 👍The permission check is well-implemented with clear error messaging that includes both the warp name and required permissions.
Line range hint
49-87: Verify error messages in translationsPlease ensure that the translation keys for the new error message (
warp().noPermission()) exist in all language files.✅ Verification successful
Translation key exists and is properly implemented ✅
The
warp().noPermission()message is available in both English and Polish translations, with proper formatting and placeholders.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the translation key in language files rg -g "*.yml" "warp:" -A 10 | grep -i "noPermission"Length of output: 199
Script:
#!/bin/bash # First, let's find all YAML files echo "=== Finding YAML files ===" fd -e yml -e yaml echo -e "\n=== Searching for translations ===" # Now search in those files for warp-related translations fd -e yml -e yaml -x rg -l "warp:" {}Length of output: 207
Script:
#!/bin/bash # Look for resource directories first echo "=== Finding resource directories ===" fd resources echo -e "\n=== Looking for translation files in resources ===" # Search for any yml/yaml files in resources directories fd -t f . $(fd resources) -x ls -la {} echo -e "\n=== Checking specific paths ===" # Check specific common paths for translations ls -la eternalcore-core/src/main/resources 2>/dev/null || echo "No main resources directory found"Length of output: 66106
Script:
#!/bin/bash # Search for warp-related translations in the implementation files echo "=== Checking EN translations ===" rg "noPermission" eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java -C 2 echo -e "\n=== Checking PL translations ===" rg "noPermission" eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java -C 2Length of output: 3016
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpServiceImpl.java (2)
25-25: Good use of ConcurrentHashMapSwitching to
ConcurrentHashMapenhances thread safety when accessing warps.
93-95: Consider renamingisExisttoexistsChanging the method name to
existsimproves readability.eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpRepositoryImpl.java (1)
80-98: Migration looks solidThe warp migration process is well-handled.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java (1)
17-20: 🛠️ Refactor suggestionAdd null check for
permissionsChecking if
permissionsis null prevents potential errors.Apply this change:
public WarpImpl(String name, Position position, List<String> permissions) { this.name = name; this.position = position; + if (permissions == null) { + this.permissions = new ArrayList<>(); + } else { + this.permissions = new ArrayList<>(permissions); + } }Likely invalid or redundant comment.
| if (!warp.hasPermissions(player)) { | ||
| return; | ||
| } | ||
|
|
||
| BaseItemBuilder baseItemBuilder = this.createItem(warpItem); | ||
| GuiItem guiItem = baseItemBuilder.asGuiItem(); | ||
|
|
||
| guiItem.setAction(event -> { | ||
| Player player = (Player) event.getWhoClicked(); | ||
| if (!warp.hasPermissions(player)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a comment about the double permission check
While both checks are good for security, it might not be immediately clear why we need to check permissions twice.
Add a comment explaining that the second check handles cases where permissions might change while the inventory is open:
GuiItem guiItem = baseItemBuilder.asGuiItem();
guiItem.setAction(event -> {
+ // Double-check permissions in case they changed while inventory was open
if (!warp.hasPermissions(player)) {
return;
}📝 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 (!warp.hasPermissions(player)) { | |
| return; | |
| } | |
| BaseItemBuilder baseItemBuilder = this.createItem(warpItem); | |
| GuiItem guiItem = baseItemBuilder.asGuiItem(); | |
| guiItem.setAction(event -> { | |
| Player player = (Player) event.getWhoClicked(); | |
| if (!warp.hasPermissions(player)) { | |
| return; | |
| } | |
| if (!warp.hasPermissions(player)) { | |
| return; | |
| } | |
| BaseItemBuilder baseItemBuilder = this.createItem(warpItem); | |
| GuiItem guiItem = baseItemBuilder.asGuiItem(); | |
| guiItem.setAction(event -> { | |
| // Double-check permissions in case they changed while inventory was open | |
| if (!warp.hasPermissions(player)) { | |
| return; | |
| } |
| public Duration getTeleportTime() { | ||
| return this.teleportTime; | ||
| } | ||
|
|
||
| public void setTeleportTime(Duration teleportTime) { | ||
| Preconditions.checkNotNull(teleportTime, "Teleport time cannot be null"); | ||
| this.teleportTime = teleportTime; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing setTeleportTime if field becomes final.
If you make teleportTime final as suggested earlier, this setter would need to be removed. The getter can stay.
| public Location getDestination() { | ||
| return this.destination != null ? this.destination : this.warp.getLocation(); | ||
| } | ||
|
|
||
| public void setDestination(Location destination) { | ||
| Preconditions.checkNotNull(destination, "Destination cannot be null"); | ||
| this.destination = destination; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Smart fallback in getDestination.
Good implementation of the destination getter with fallback to warp location. The null check in the setter is also appropriate.
However, consider documenting the fallback behavior in a Javadoc comment:
/**
* Gets the destination location for this teleport.
* @return the custom destination if set, otherwise falls back to the warp's location
*/| private Duration teleportTime; | ||
| private @Nullable Location destination; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making teleportTime final.
Since teleportTime is a required parameter in the constructor and represents a core property, consider making it final for better immutability.
- private Duration teleportTime;
+ private final Duration teleportTime;📝 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.
| private Duration teleportTime; | |
| private @Nullable Location destination; | |
| private final Duration teleportTime; | |
| private @Nullable Location destination; |
| teleport.getResult().whenComplete((result, throwable) -> { | ||
| this.eventCaller.callEvent(new WarpTeleportEvent(player, destinationWarp, destination)); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add error handling in the completion callback
The completion handler should check for errors (throwable) to ensure proper error reporting.
teleport.getResult().whenComplete((result, throwable) -> {
+ if (throwable != null) {
+ // Log the error or notify the player
+ return;
+ }
this.eventCaller.callEvent(new WarpTeleportEvent(player, destinationWarp, destination));
});📝 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.
| teleport.getResult().whenComplete((result, throwable) -> { | |
| this.eventCaller.callEvent(new WarpTeleportEvent(player, destinationWarp, destination)); | |
| }); | |
| teleport.getResult().whenComplete((result, throwable) -> { | |
| if (throwable != null) { | |
| // Log the error or notify the player | |
| return; | |
| } | |
| this.eventCaller.callEvent(new WarpTeleportEvent(player, destinationWarp, destination)); | |
| }); |
| public WarpConfigEntry(Position position, List<String> permissions) { | ||
| this.position = position; | ||
| this.permissions = permissions; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add parameter validation in constructor
Consider adding null checks:
public WarpConfigEntry(Position position, List<String> permissions) {
+ if (position == null) {
+ throw new IllegalArgumentException("position cannot be null");
+ }
+ if (permissions == null) {
+ throw new IllegalArgumentException("permissions cannot be null");
+ }
this.position = position;
this.permissions = permissions;
}📝 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.
| public WarpConfigEntry(Position position, List<String> permissions) { | |
| this.position = position; | |
| this.permissions = permissions; | |
| } | |
| public WarpConfigEntry(Position position, List<String> permissions) { | |
| if (position == null) { | |
| throw new IllegalArgumentException("position cannot be null"); | |
| } | |
| if (permissions == null) { | |
| throw new IllegalArgumentException("permissions cannot be null"); | |
| } | |
| this.position = position; | |
| this.permissions = permissions; | |
| } |
| @Exclude | ||
| public static final String WARP_DATA_FILE_PATH = "data" + File.separator + "warps.yml"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using Path.of() for better cross-platform compatibility
Instead of manually concatenating with File.separator, you could use:
- public static final String WARP_DATA_FILE_PATH = "data" + File.separator + "warps.yml";
+ public static final String WARP_DATA_FILE_PATH = Path.of("data", "warps.yml").toString();📝 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.
| @Exclude | |
| public static final String WARP_DATA_FILE_PATH = "data" + File.separator + "warps.yml"; | |
| @Exclude | |
| public static final String WARP_DATA_FILE_PATH = Path.of("data", "warps.yml").toString(); |
| default boolean hasPermissions(Permissible permissible) { | ||
| List<String> permissions = this.getPermissions(); | ||
| if (permissions.isEmpty()) { | ||
| return true; | ||
| } | ||
|
|
||
| return permissions | ||
| .stream() | ||
| .anyMatch(permission -> permissible.hasPermission(permission)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using anyMatch directly instead of stream
The current implementation works well, but for better performance you could use:
- return permissions
- .stream()
- .anyMatch(permission -> permissible.hasPermission(permission));
+ return permissions.stream().anyMatch(permissible::hasPermission);📝 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.
| default boolean hasPermissions(Permissible permissible) { | |
| List<String> permissions = this.getPermissions(); | |
| if (permissions.isEmpty()) { | |
| return true; | |
| } | |
| return permissions | |
| .stream() | |
| .anyMatch(permission -> permissible.hasPermission(permission)); | |
| } | |
| default boolean hasPermissions(Permissible permissible) { | |
| List<String> permissions = this.getPermissions(); | |
| if (permissions.isEmpty()) { | |
| return true; | |
| } | |
| return permissions.stream().anyMatch(permissible::hasPermission); | |
| } |
| .send(); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (!this.warpService.hasWarps()) { | ||
| if (this.warpService.getWarps().isEmpty()) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the empty check
The change from hasWarps() to getWarps().isEmpty() works but might be less efficient as it creates a new collection.
- if (this.warpService.getWarps().isEmpty()) {
+ if (!this.warpService.hasWarps()) {Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfig.java(1 hunks)
🧰 Additional context used
📓 Learnings (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfig.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalCore#856
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/data/WarpDataConfig.java:19-20
Timestamp: 2025-01-12T13:03:07.239Z
Learning: Configuration classes in EternalCore (those implementing ReloadableConfig and annotated with @ConfigurationFile) don't need thread-safe collections as they are managed by the configuration system which handles synchronization.
🔇 Additional comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/repository/WarpConfig.java (1)
1-17: Looking good!The class setup and imports are well-organized.
| @Override | ||
| public Resource resource(File folder) { | ||
| return Source.of(folder, "data" + File.separator + "warps.yml"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using Path API for better path handling
Using Path.resolve() might be more reliable than string concatenation with File.separator.
- return Source.of(folder, "data" + File.separator + "warps.yml");
+ return Source.of(folder, folder.toPath().resolve("data").resolve("warps.yml").toString());📝 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.
| @Override | |
| public Resource resource(File folder) { | |
| return Source.of(folder, "data" + File.separator + "warps.yml"); | |
| } | |
| @Override | |
| public Resource resource(File folder) { | |
| return Source.of(folder, folder.toPath().resolve("data").resolve("warps.yml").toString()); | |
| } |
| @Description({"# Warps data", "# These are warp locations, for your own safety, please don't touch it."}) | ||
| public Map<String, WarpConfigEntry> warps = new HashMap<>(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using Map.of() or Collections.emptyMap()
Since this is a configuration class, starting with an empty immutable map might be cleaner. The configuration system will replace it when loading anyway.
- public Map<String, WarpConfigEntry> warps = new HashMap<>();
+ public Map<String, WarpConfigEntry> warps = Map.of();📝 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.
| @Description({"# Warps data", "# These are warp locations, for your own safety, please don't touch it."}) | |
| public Map<String, WarpConfigEntry> warps = new HashMap<>(); | |
| @Description({"# Warps data", "# These are warp locations, for your own safety, please don't touch it."}) | |
| public Map<String, WarpConfigEntry> warps = Map.of(); |
| @Contextual | ||
| public static class WarpConfigEntry { | ||
| public Position position; | ||
| public List<String> permissions; | ||
|
|
||
| public WarpConfigEntry() { | ||
| } | ||
|
|
||
| public WarpConfigEntry(Position position, List<String> permissions) { | ||
| this.position = position; | ||
| this.permissions = permissions; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and initialize collections
The constructor should validate inputs and ensure permissions list isn't null.
public WarpConfigEntry(Position position, List<String> permissions) {
+ if (position == null) {
+ throw new IllegalArgumentException("Position cannot be null");
+ }
this.position = position;
- this.permissions = permissions;
+ this.permissions = permissions != null ? permissions : List.of();
}📝 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.
| @Contextual | |
| public static class WarpConfigEntry { | |
| public Position position; | |
| public List<String> permissions; | |
| public WarpConfigEntry() { | |
| } | |
| public WarpConfigEntry(Position position, List<String> permissions) { | |
| this.position = position; | |
| this.permissions = permissions; | |
| } | |
| } | |
| @Contextual | |
| public static class WarpConfigEntry { | |
| public Position position; | |
| public List<String> permissions; | |
| public WarpConfigEntry() { | |
| } | |
| public WarpConfigEntry(Position position, List<String> permissions) { | |
| if (position == null) { | |
| throw new IllegalArgumentException("Position cannot be null"); | |
| } | |
| this.position = position; | |
| this.permissions = permissions != null ? permissions : List.of(); | |
| } | |
| } |
Add permission-based access to warps.
Fixes: #835