Refactor GUI and commands with improved test coverage#45
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReorganizes command packages, adds reusable command utilities and abstract bases, refactors GUI into factory/layout helpers, centralizes Redis logging and storage creation, updates POM/CI and test deps, removes legacy command classes, and adds extensive unit tests across commands and storage. Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as CommandSender
participant Guard as CommandGuards
participant Parser as SlotArgumentParser
participant Manager as KitManager
participant Player as Player
Sender->>Guard: requirePlayerInEnabledWorld(sender)
Guard-->>Sender: Player or null
alt Player returned
Sender->>Parser: parseSlotInRange(arg, min, max)
Parser-->>Sender: slot or null
alt slot valid
Sender->>Manager: perform operation (load/delete/share/etc)
Manager-->>Player: open GUI / apply change / send feedback
else slot invalid
Sender-->>Player: send error message
end
else null
Sender-->>Player: guard error message (only players / blocked world)
end
sequenceDiagram
participant Operator as CommandSender
participant Migrator as StorageMigrator
participant Selector as StorageSelector
participant Src as SourceStorageManager
participant Dst as DestinationStorageManager
Operator->>Migrator: /perplayerkit migrate src dst
Migrator->>Selector: createStorageManager(src)
Selector-->>Migrator: Src (StorageManager)
Migrator->>Selector: createStorageManager(dst)
Selector-->>Migrator: Dst (StorageManager)
Migrator->>Src: connect(), init()
Src-->>Migrator: connected
Migrator->>Src: list entries
loop per entry
Migrator->>Src: get(entry)
Src-->>Migrator: entryData
Migrator->>Dst: save(entryData)
end
Migrator->>Src: close()
Migrator->>Dst: close()
Migrator-->>Operator: report migrated/failed counts or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (19)
src/main/java/dev/noah/perplayerkit/PerPlayerKit.java (1)
42-42: Consider replacing the remaining wildcard import for consistency.The
listenerspackage still uses a wildcard import while all command package imports were made explicit. For consistency with the refactoring approach, consider replacing this with explicit imports forJoinListener,QuitListener,KitMenuCloseListener,KitRoomSaveListener, andAutoRekitListener.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/PerPlayerKit.java` at line 42, Replace the wildcard import "import dev.noah.perplayerkit.listeners.*;" with explicit imports for each listener class to match the refactor style used for commands: add individual imports for JoinListener, QuitListener, KitMenuCloseListener, KitRoomSaveListener, and AutoRekitListener and remove the wildcard; locate the import line in PerPlayerKit and update it to import those five listener classes explicitly.src/test/java/dev/noah/perplayerkit/storage/SQLStorageTest.java (2)
56-65: Consider adding an assertion to strengthen the test.This test only implicitly verifies that
keepAlive()doesn't throw an exception. Adding an explicit assertion (e.g.,assertTrue(storage.isConnected())afterkeepAlive()) would make the test's intent clearer and verify the expected post-condition.💡 Suggested improvement
`@Test` void keepAliveSucceedsWhenConnected() throws Exception { InMemorySQLiteDatabase db = new InMemorySQLiteDatabase(); SQLStorage storage = new SQLStorage(db); storage.connect(); storage.keepAlive(); + assertTrue(storage.isConnected()); storage.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/noah/perplayerkit/storage/SQLStorageTest.java` around lines 56 - 65, Add an explicit assertion to the keepAliveSucceedsWhenConnected test so it verifies state after calling SQLStorage.keepAlive(); specifically, in SQLStorageTest#keepAliveSucceedsWhenConnected assert that the storage reports a connected state (e.g., call SQLStorage.isConnected() and assertTrue on its result) after storage.keepAlive() and before storage.close(), to make the expected post-condition explicit.
125-131: Note:getConnection()returns new connections without tracking.Each call to
getConnection()creates a newConnectioninstance. For in-memory SQLite withcache=shared, this works correctly since all connections share the same database. However, relying on the caller (SQLStorage) to close connections properly is implicit. IfSQLStorageleaks connections, this test won't catch it.For more robust testing, consider tracking returned connections and verifying they're closed, or documenting this expectation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/noah/perplayerkit/storage/SQLStorageTest.java` around lines 125 - 131, The test currently returns fresh JDBC Connections from getConnection() (DriverManager.getConnection(jdbcUrl)) without tracking, so leaks in SQLStorage won't be detected; update the test to wrap/trap connections returned by getConnection()—for example, in the overridden getConnection() store each new Connection in a List<Connection> and return a proxy/decorator that intercepts close() to mark it closed (or simply add the raw Connection to the list and assert they are closed in tearDown) so you can verify all connections created by getConnection() were closed by SQLStorage; reference getConnection(), jdbcUrl, DriverManager and the SQLStorage usage in assertions.pom.xml (1)
82-88: Append to SurefireargLineinstead of replacing it.Mockito's Maven example prepends
@{argLine}when adding the agent. Using the same pattern here is safer, because it preserves any JVM args that may already be contributed to Surefire. (javadoc.io)Proposed fix
- <argLine>-javaagent:${settings.localRepository}/org/mockito/mockito-core/${mockito.version}/mockito-core-${mockito.version}.jar --enable-native-access=ALL-UNNAMED -Xshare:off</argLine> + <argLine>@{argLine} -javaagent:${settings.localRepository}/org/mockito/mockito-core/${mockito.version}/mockito-core-${mockito.version}.jar --enable-native-access=ALL-UNNAMED -Xshare:off</argLine>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 82 - 88, The Surefire plugin configuration currently sets argLine directly which overwrites any existing JVM args; update the maven-surefire-plugin configuration for the argLine to prepend/inject the Mockito javaagent while preserving existing args by including the existing ${argLine} placeholder (i.e., use the pattern that references @{argLine} or ${argLine} depending on your pom style) and still reference ${settings.localRepository}, ${mockito.version} and the mockito-core jar so the javaagent is appended without discarding pre-existing JVM arguments.src/test/java/dev/noah/perplayerkit/storage/StorageMigratorTest.java (1)
88-88: PreferassertNulloverassertEquals(null, ...).Using
assertNullprovides clearer intent and better failure messages.♻️ Suggested fix
- assertEquals(null, result.getErrorMessage()); + assertNull(result.getErrorMessage());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/noah/perplayerkit/storage/StorageMigratorTest.java` at line 88, Replace the assertion that checks result.getErrorMessage() using assertEquals(null, ...) with a direct null-check using assertNull to convey intent and improve failure output; update the test in StorageMigratorTest (the assertion in the test method referencing result.getErrorMessage()) to call assertNull(result.getErrorMessage()) and ensure the appropriate JUnit assertNull import or static import is present.src/test/java/dev/noah/perplayerkit/commands/SlotArgumentParserTest.java (1)
21-29: Consider adding boundary and edge case tests.The current tests cover basic cases well. Consider adding tests for:
- Boundary values:
parseSlotInRange("1", 1, 9)andparseSlotInRange("9", 1, 9)(both should return the value)- Edge cases: empty string
"", whitespace" ", negative numbers"-1"📝 Example additional tests
`@Test` void parseSlotInRangeReturnsIntegerAtMinBoundary() { assertEquals(1, SlotArgumentParser.parseSlotInRange("1", 1, 9)); } `@Test` void parseSlotInRangeReturnsIntegerAtMaxBoundary() { assertEquals(9, SlotArgumentParser.parseSlotInRange("9", 1, 9)); } `@Test` void parseSlotReturnsNullForEmptyString() { assertNull(SlotArgumentParser.parseSlot("")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/noah/perplayerkit/commands/SlotArgumentParserTest.java` around lines 21 - 29, Add unit tests covering boundary values and edge cases for SlotArgumentParser: create tests that assert parseSlotInRange("1", 1, 9) returns 1 and parseSlotInRange("9", 1, 9) returns 9, and add tests asserting null for invalid inputs such as empty string "", whitespace " ", negative numbers like "-1" and non-numeric input (e.g., "a"); use method names like parseSlotInRangeReturnsIntegerAtMinBoundary, parseSlotInRangeReturnsIntegerAtMaxBoundary, and parseSlotReturnsNullForInvalidInputs and call SlotArgumentParser.parseSlotInRange and SlotArgumentParser.parseSlot to validate behavior.src/test/java/dev/noah/perplayerkit/util/CooldownManagerTest.java (1)
26-34: Consider time-controllable testing for reliability.Using
Thread.sleep(1200)for a 1-second cooldown makes the test slow and potentially flaky in constrained CI environments. Consider injecting aClockorTimeProviderintoCooldownManagerto allow deterministic time control in tests.That said, the 200ms buffer should be sufficient for most environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/noah/perplayerkit/util/CooldownManagerTest.java` around lines 26 - 34, The test keyExpiresAfterCooldownWindow uses Thread.sleep which is slow and flaky; modify CooldownManager to accept a time source (e.g., java.time.Clock or a TimeProvider interface) and have its methods setCooldown and isOnCooldown use that clock for time checks; update the production constructor to default to Clock.systemUTC() but add an overloaded constructor or setter for injecting a test Clock, then change the test to create a controllable Clock (or fake TimeProvider), call cooldownManager.setCooldown("alpha"), advance the fake clock by >1s, and assertFalse(cooldownManager.isOnCooldown("alpha")) without any Thread.sleep.src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java (1)
117-119: Log the throwable here, not onlyex.getMessage().Async failures are often wrapped, and the message is frequently empty. Logging only the string loses the stack trace you need to debug production load failures.
🪵 Suggested logging fix
+import java.util.logging.Level; ... - plugin.getLogger().severe(loadErrorLogMessage() + ": " + ex.getMessage()); + plugin.getLogger().log(Level.SEVERE, loadErrorLogMessage(), ex);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java` around lines 117 - 119, The current error logging uses only ex.getMessage(), losing the stack trace; change the call that now uses plugin.getLogger().severe(loadErrorLogMessage() + ": " + ex.getMessage()) to log the throwable itself (e.g. plugin.getLogger().log(Level.SEVERE, loadErrorLogMessage(), ex) or similar) so the full exception and stacktrace are recorded; keep the user-facing BroadcastManager.get() / ERROR_PREFIX message as-is. Ensure you add/import java.util.logging.Level if using Logger.log.src/main/java/dev/noah/perplayerkit/commands/share/AbstractShareSlotCommand.java (1)
39-43: Don't start the cooldown unless the share actually succeeds.The new abstraction gives the base class no signal about whether the share step worked, so a handled failure path will still consume the 5-second cooldown. Returning a boolean/result from the callback would keep the cooldown aligned with the real outcome.
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/share/AbstractShareSlotCommand.java` around lines 39 - 43, The shareAction currently typed as BiConsumer<Player,Integer> provides no success signal so the base class starts the cooldown even when sharing fails; change the field and constructor parameter to a BiFunction<Player,Integer,Boolean> (or similar Result-returning functional type) named shareAction in AbstractShareSlotCommand, update any lambda callers to return true/false on success/failure, and then only invoke startCooldown/trigger the 5-second cooldown when shareAction.apply(player, slot) returns true; also update any usages referenced in the class that called shareAction.accept(...) to use apply(...) and branch on its boolean result.src/main/java/dev/noah/perplayerkit/gui/GuiLayoutUtils.java (1)
43-60: Fail fast on invalid ranges and menu sizes.These helpers are now shared layout primitives, but they assume the caller passes valid slot bounds and, for Lines 55-60, a 54-slot menu. One bad call will blow up inside Canvas with little context. Adding explicit precondition checks here would make GUI regressions much easier to diagnose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/gui/GuiLayoutUtils.java` around lines 43 - 60, The helper methods allowModificationRange, setGlassPaneRange and setArmorAndOffhandIndicators currently assume valid slot ranges and a specific menu size; add defensive precondition checks to fail fast: validate that startInclusive <= endExclusive, that startInclusive and endExclusive are within menu size (use menu.size() or equivalent) before looping in allowModificationRange and setGlassPaneRange, and in setArmorAndOffhandIndicators verify menu.size() >= 54 and that ARMOR_INDICATOR_START..ARMOR_INDICATOR_START+3 and OFFHAND_INDICATOR_SLOT are within bounds; if any check fails, throw an IllegalArgumentException with a clear message referencing the offending symbol (e.g. allowModificationRange, setGlassPaneRange, setArmorAndOffhandIndicators, ARMOR_INDICATOR_START, OFFHAND_INDICATOR_SLOT) so callers get immediate, actionable errors instead of Canvas-level failures.src/test/java/dev/noah/perplayerkit/commands/AbstractShareSlotCommandTest.java (1)
48-60: Don't bake kit-specific wording into the shared base test.Lines 58-59 lock
AbstractShareSlotCommandto"Select a valid kit slot", but this base now also backsShareECKitCommand. That makes the shared contract too specific and either forces the EC flow to show the wrong noun or makes the base harder to reuse. Prefer asserting a generic validation failure here and keep wording checks in concrete-command tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/noah/perplayerkit/commands/AbstractShareSlotCommandTest.java` around lines 48 - 60, The test invalidSlotDoesNotExecuteAction currently asserts the exact kit-specific message ("Select a valid kit slot") which couples the shared AbstractShareSlotCommand test to a concrete wording; update the assertion on player.sendMessage in AbstractShareSlotCommandTest (the TestShareSlotCommand usage) to check for a generic validation failure (e.g., assert sendMessage was called with a message containing "Select a valid" or simply any non-empty validation message) and leave the exact "Select a valid kit slot" wording assertion to the concrete command tests (e.g., ShareECKitCommandTest); keep the SoundManager.playFailure verification as-is.src/main/java/dev/noah/perplayerkit/gui/GUI.java (2)
381-391: Consider usingKIT_CONTENT_ENDconstant instead of hardcoded41.The loop at line 386 uses hardcoded
41, but other similar loops in this refactor useKIT_CONTENT_END. For consistency and maintainability, consider using the constant here as well.♻️ Suggested fix
public void addClear(Slot slot) { slot.setClickHandler((player, info) -> { SoundManager.playClick(player); if (info.getClickType().isShiftClick()) { Menu m = info.getClickedMenu(); - for (int i = 0; i < 41; i++) { + for (int i = 0; i < KIT_CONTENT_END; i++) { m.getSlot(i).setItem((org.bukkit.inventory.ItemStack) null); } } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java` around lines 381 - 391, The loop in addClear uses a hardcoded 41; replace that magic number with the KIT_CONTENT_END constant for consistency and maintainability: in the addClear(Slot slot) click handler (inside Slot.setClickHandler), use KIT_CONTENT_END in the for loop boundary when iterating over m.getSlot(i) so it matches other loops and avoids the hardcoded value.
531-544: Consider usingKIT_CONTENT_ENDconstant for consistency.Similar to
addClear, this method uses hardcoded41. Using the constant would improve maintainability.♻️ Suggested fix
public void addImport(Slot slot) { slot.setClickHandler((player, info) -> { SoundManager.playClick(player); Menu m = info.getClickedMenu(); ItemStack[] inv; if (filterItemsOnImport) { inv = ItemFilter.get().filterItemStack(player.getInventory().getContents()); } else { inv = player.getInventory().getContents(); } - for (int i = 0; i < 41; i++) { + for (int i = 0; i < KIT_CONTENT_END; i++) { m.getSlot(i).setItem(inv[i]); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java` around lines 531 - 544, The loop in addImport uses a hardcoded 41; update it to use the KIT_CONTENT_END constant (the same constant used in addClear) to improve maintainability: inside addImport (the Slot click handler that reads filterItemsOnImport and inv from ItemFilter.get()/player.getInventory()), replace the for-loop bound of 41 with KIT_CONTENT_END and ensure you still guard against inv length (e.g., only access inv[i] if i < inv.length) before calling m.getSlot(i).setItem(inv[i]).src/main/java/dev/noah/perplayerkit/commands/inspect/InspectKitCommand.java (1)
25-26: Minor: Missing blank line before class declaration.Same formatting suggestion as
InspectEcCommandfor consistency.🔧 Suggested fix
import java.util.UUID; + public class InspectKitCommand extends AbstractInspectCommand {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectKitCommand.java` around lines 25 - 26, Add a blank line between the import block and the class declaration to match project formatting (same as InspectEcCommand); update the file containing the InspectKitCommand class so there is an empty line after the import java.util.UUID; line and before the "public class InspectKitCommand extends AbstractInspectCommand {" declaration to keep consistent spacing/style.src/main/java/dev/noah/perplayerkit/commands/inspect/InspectEcCommand.java (1)
25-26: Minor: Missing blank line before class declaration.A blank line between imports and the class declaration improves readability.
🔧 Suggested fix
import java.util.UUID; + public class InspectEcCommand extends AbstractInspectCommand {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectEcCommand.java` around lines 25 - 26, Add a single blank line between the imports and the class declaration in InspectEcCommand to improve readability; specifically, after the import statements (e.g., the import java.util.UUID;) insert one empty line before the "public class InspectEcCommand extends AbstractInspectCommand {" line.src/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.java (1)
46-66: Consider usingGuiMenuFactoryfor consistency.Other GUI menus in this PR use
GuiMenuFactoryfor centralized menu creation. While this view-only enderchest menu is simple, using the factory pattern would maintain consistency across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.java` around lines 46 - 66, The viewOnlyEC method builds its menu directly with ChestMenu.builder; replace this with the centralized GuiMenuFactory to keep menu construction consistent: call an existing GuiMenuFactory method (or add a new factory method like createViewOnlyEnderchest(MenuConfig) if missing) to obtain the Menu instead of using ChestMenu.builder in viewOnlyEC, then keep the rest of the logic (filling panes, copying player.getEnderChest().getContents(), opening menu and playing sound) unchanged; reference the viewOnlyEC method and ChestMenu.builder usage as the location to swap in GuiMenuFactory and update imports/usages accordingly.src/main/java/dev/noah/perplayerkit/commands/shortcuts/AbstractShortSlotCommand.java (1)
35-38: Consider validating prefixes for null.If a subclass inadvertently passes
nullfor either prefix,parseSingleDigitSuffixwill throw aNullPointerExceptiononlabel.startsWith(prefix). Adding a null check or usingObjects.requireNonNullin the constructor would make the failure explicit and early.🛡️ Suggested defensive check
+import java.util.Objects; + public abstract class AbstractShortSlotCommand implements CommandExecutor { private final String shortPrefix; private final String longPrefix; protected AbstractShortSlotCommand(String shortPrefix, String longPrefix) { - this.shortPrefix = shortPrefix; - this.longPrefix = longPrefix; + this.shortPrefix = Objects.requireNonNull(shortPrefix, "shortPrefix"); + this.longPrefix = Objects.requireNonNull(longPrefix, "longPrefix"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/shortcuts/AbstractShortSlotCommand.java` around lines 35 - 38, The constructor in AbstractShortSlotCommand should validate that shortPrefix and longPrefix are non-null to fail fast and avoid a later NPE in parseSingleDigitSuffix; update the AbstractShortSlotCommand(String shortPrefix, String longPrefix) constructor to check both fields (e.g., using Objects.requireNonNull or explicit null checks) and throw a clear IllegalArgumentException/NullPointerException with a message referencing shortPrefix/longPrefix so subclasses cannot pass null silently.src/main/java/dev/noah/perplayerkit/gui/GuiMenuFactory.java (2)
30-32: Verify the intentional difference inredrawsettings.Only
createPublicKitRoomMenu(),createKitRoomMenu(), andcreateViewPublicKitMenu()enable.redraw(true). The other menus do not. If this is intentional (e.g., "room" menus need dynamic updates while others are static), consider adding a brief comment to document the rationale.Also applies to: 58-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/gui/GuiMenuFactory.java` around lines 30 - 32, createPublicKitRoomMenu(), createKitRoomMenu(), and createViewPublicKitMenu() call .redraw(true) while other Menu factory methods do not; confirm whether the difference is intentional and either make behavior consistent or document the rationale: if these "room" menus require dynamic updates keep .redraw(true) and add a brief inline comment in GuiMenuFactory near createPublicKitRoomMenu/createKitRoomMenu/createViewPublicKitMenu explaining that redraw is enabled because these menus update dynamically (e.g., player joins/leaves or kit list changes); if it was accidental, remove .redraw(true) from those three methods so all menus share the same static behavior.
30-64: Consider extracting the repeated row count to a named constant.The value
6appears in every factory method. Extracting it to a constant (e.g.,private static final int MENU_ROWS = 6;) would improve maintainability if the default row count ever needs to change.♻️ Suggested refactor
public final class GuiMenuFactory { + private static final int MENU_ROWS = 6; + private GuiMenuFactory() { } public static Menu createPublicKitRoomMenu() { - return ChestMenu.builder(6).title(StyleManager.get().getPrimaryColor() + "Public Kit Room").redraw(true).build(); + return ChestMenu.builder(MENU_ROWS).title(StyleManager.get().getPrimaryColor() + "Public Kit Room").redraw(true).build(); } // ... apply similarly to other methods🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/gui/GuiMenuFactory.java` around lines 30 - 64, The methods createPublicKitRoomMenu, createKitMenu, createPublicKitMenu, createECMenu, createInspectMenu, createInspectEcMenu, createMainMenu, createKitRoomMenu, and createViewPublicKitMenu all hard-code the row count 6; extract that to a single constant (e.g., add private static final int MENU_ROWS = 6) and replace the literal 6 in each ChestMenu.builder(...) call with MENU_ROWS to centralize configuration and simplify future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/noah/perplayerkit/commands/admin/KitRoomCommand.java`:
- Around line 53-56: The code currently calls
KitRoomDataManager.get().saveToDBAsync() and immediately sends success feedback
(sender.sendMessage and SoundManager.playSuccess) before the async save
completes; change this by moving the success message and success sound into the
async completion callback returned by saveToDBAsync() (e.g.,
thenAccept/whenComplete/handle on the returned CompletableFuture or success
callback), and add an error path that sends a failure message and plays a
failure sound if the async task completes exceptionally; update the block around
KitRoomDataManager.get().saveToDBAsync(), sender.sendMessage(...) and
SoundManager.playSuccess(...) in KitRoomCommand to use the future’s completion
handlers so feedback reflects the actual result.
- Around line 46-49: The call to KitRoomDataManager.get().loadFromDB() runs
blocking JDBC queries on the command/main thread; wrap the database work in an
asynchronous task (use a BukkitRunnable or
Bukkit.getScheduler().runTaskAsynchronously) similar to saveToDBAsync(): move
the blocking loop that calls storageManager.getKitDataByID()/SQLStorage methods
into an async runnable inside KitRoomDataManager.loadFromDB() (or create a new
loadFromDBAsync()), and ensure any interactions with the Bukkit API (sending
sender.sendMessage and SoundManager.playSuccess) are executed back on the main
thread via runTask(...) so only DB I/O runs asynchronously.
In `@src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java`:
- Around line 82-88: The current flow calls importer.importFiles() even when
importer.checkForFiles() returns false; modify PerPlayerKitCommand so that after
sending the two error messages (using sender.sendMessage) it returns immediately
(or otherwise skips the import) instead of proceeding to call
importer.importFiles(), ensuring the importFiles() call only runs when
importer.checkForFiles() is true.
In
`@src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java`:
- Around line 180-189: The current flow treats any UUID as a real player and
causes typoed names (which InspectCommandUtil turns into synthetic UUIDs via
UUID.nameUUIDFromBytes) to produce a misleading "missing data" error; fix this
by changing the lookup behavior so unresolved lookups return null (or
alternatively, add an explicit offline-mode path) and update showInspectResult
to handle null/failed lookups before calling hasData/getPlayerName: either make
InspectCommandUtil return null on lookup miss instead of nameUUIDFromBytes, or
add a check in showInspectResult to detect synthetic offline UUIDs (e.g., avoid
UUID.nameUUIDFromBytes-created values) and send a "player not found" message
instead of proceeding to hasData/openInspectGui; update
getPlayerName/InspectCommandUtil accordingly so getPlayerName can return null or
throw a clear NotFound signal that showInspectResult handles.
In `@src/main/java/dev/noah/perplayerkit/commands/share/ShareECKitCommand.java`:
- Around line 25-26: The constructor of ShareECKitCommand passes an awkward
message string to super — change the error text in the super call inside
ShareECKitCommand() (the first argument) from "Error, you must select a EC slot
to share" to use the correct article and clearer wording such as "Error, you
must select an EC slot to share" or "Error, you must select an ender chest slot
to share" so chat displays naturally; update only the message literal in the
ShareECKitCommand constructor where super(...) is invoked.
In `@src/main/java/dev/noah/perplayerkit/storage/RedisStorage.java`:
- Around line 147-152: The logRedisFailure method currently uses
plugin.getLogger().fine(...) and only prints exception.getMessage(); change it
to log at a visible level and include the throwable so the stack trace is
preserved (e.g., use plugin.getLogger().log(Level.SEVERE, "Redis " + operation +
" failed", exception)); add the import/use of java.util.logging.Level if needed
and keep the existing null checks for plugin/getLogger() while referencing the
method name logRedisFailure and the parameters operation and exception.
---
Nitpick comments:
In `@pom.xml`:
- Around line 82-88: The Surefire plugin configuration currently sets argLine
directly which overwrites any existing JVM args; update the
maven-surefire-plugin configuration for the argLine to prepend/inject the
Mockito javaagent while preserving existing args by including the existing
${argLine} placeholder (i.e., use the pattern that references @{argLine} or
${argLine} depending on your pom style) and still reference
${settings.localRepository}, ${mockito.version} and the mockito-core jar so the
javaagent is appended without discarding pre-existing JVM arguments.
In
`@src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java`:
- Around line 117-119: The current error logging uses only ex.getMessage(),
losing the stack trace; change the call that now uses
plugin.getLogger().severe(loadErrorLogMessage() + ": " + ex.getMessage()) to log
the throwable itself (e.g. plugin.getLogger().log(Level.SEVERE,
loadErrorLogMessage(), ex) or similar) so the full exception and stacktrace are
recorded; keep the user-facing BroadcastManager.get() / ERROR_PREFIX message
as-is. Ensure you add/import java.util.logging.Level if using Logger.log.
In `@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectEcCommand.java`:
- Around line 25-26: Add a single blank line between the imports and the class
declaration in InspectEcCommand to improve readability; specifically, after the
import statements (e.g., the import java.util.UUID;) insert one empty line
before the "public class InspectEcCommand extends AbstractInspectCommand {"
line.
In `@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectKitCommand.java`:
- Around line 25-26: Add a blank line between the import block and the class
declaration to match project formatting (same as InspectEcCommand); update the
file containing the InspectKitCommand class so there is an empty line after the
import java.util.UUID; line and before the "public class InspectKitCommand
extends AbstractInspectCommand {" declaration to keep consistent spacing/style.
In `@src/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.java`:
- Around line 46-66: The viewOnlyEC method builds its menu directly with
ChestMenu.builder; replace this with the centralized GuiMenuFactory to keep menu
construction consistent: call an existing GuiMenuFactory method (or add a new
factory method like createViewOnlyEnderchest(MenuConfig) if missing) to obtain
the Menu instead of using ChestMenu.builder in viewOnlyEC, then keep the rest of
the logic (filling panes, copying player.getEnderChest().getContents(), opening
menu and playing sound) unchanged; reference the viewOnlyEC method and
ChestMenu.builder usage as the location to swap in GuiMenuFactory and update
imports/usages accordingly.
In
`@src/main/java/dev/noah/perplayerkit/commands/share/AbstractShareSlotCommand.java`:
- Around line 39-43: The shareAction currently typed as
BiConsumer<Player,Integer> provides no success signal so the base class starts
the cooldown even when sharing fails; change the field and constructor parameter
to a BiFunction<Player,Integer,Boolean> (or similar Result-returning functional
type) named shareAction in AbstractShareSlotCommand, update any lambda callers
to return true/false on success/failure, and then only invoke
startCooldown/trigger the 5-second cooldown when shareAction.apply(player, slot)
returns true; also update any usages referenced in the class that called
shareAction.accept(...) to use apply(...) and branch on its boolean result.
In
`@src/main/java/dev/noah/perplayerkit/commands/shortcuts/AbstractShortSlotCommand.java`:
- Around line 35-38: The constructor in AbstractShortSlotCommand should validate
that shortPrefix and longPrefix are non-null to fail fast and avoid a later NPE
in parseSingleDigitSuffix; update the AbstractShortSlotCommand(String
shortPrefix, String longPrefix) constructor to check both fields (e.g., using
Objects.requireNonNull or explicit null checks) and throw a clear
IllegalArgumentException/NullPointerException with a message referencing
shortPrefix/longPrefix so subclasses cannot pass null silently.
In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java`:
- Around line 381-391: The loop in addClear uses a hardcoded 41; replace that
magic number with the KIT_CONTENT_END constant for consistency and
maintainability: in the addClear(Slot slot) click handler (inside
Slot.setClickHandler), use KIT_CONTENT_END in the for loop boundary when
iterating over m.getSlot(i) so it matches other loops and avoids the hardcoded
value.
- Around line 531-544: The loop in addImport uses a hardcoded 41; update it to
use the KIT_CONTENT_END constant (the same constant used in addClear) to improve
maintainability: inside addImport (the Slot click handler that reads
filterItemsOnImport and inv from ItemFilter.get()/player.getInventory()),
replace the for-loop bound of 41 with KIT_CONTENT_END and ensure you still guard
against inv length (e.g., only access inv[i] if i < inv.length) before calling
m.getSlot(i).setItem(inv[i]).
In `@src/main/java/dev/noah/perplayerkit/gui/GuiLayoutUtils.java`:
- Around line 43-60: The helper methods allowModificationRange,
setGlassPaneRange and setArmorAndOffhandIndicators currently assume valid slot
ranges and a specific menu size; add defensive precondition checks to fail fast:
validate that startInclusive <= endExclusive, that startInclusive and
endExclusive are within menu size (use menu.size() or equivalent) before looping
in allowModificationRange and setGlassPaneRange, and in
setArmorAndOffhandIndicators verify menu.size() >= 54 and that
ARMOR_INDICATOR_START..ARMOR_INDICATOR_START+3 and OFFHAND_INDICATOR_SLOT are
within bounds; if any check fails, throw an IllegalArgumentException with a
clear message referencing the offending symbol (e.g. allowModificationRange,
setGlassPaneRange, setArmorAndOffhandIndicators, ARMOR_INDICATOR_START,
OFFHAND_INDICATOR_SLOT) so callers get immediate, actionable errors instead of
Canvas-level failures.
In `@src/main/java/dev/noah/perplayerkit/gui/GuiMenuFactory.java`:
- Around line 30-32: createPublicKitRoomMenu(), createKitRoomMenu(), and
createViewPublicKitMenu() call .redraw(true) while other Menu factory methods do
not; confirm whether the difference is intentional and either make behavior
consistent or document the rationale: if these "room" menus require dynamic
updates keep .redraw(true) and add a brief inline comment in GuiMenuFactory near
createPublicKitRoomMenu/createKitRoomMenu/createViewPublicKitMenu explaining
that redraw is enabled because these menus update dynamically (e.g., player
joins/leaves or kit list changes); if it was accidental, remove .redraw(true)
from those three methods so all menus share the same static behavior.
- Around line 30-64: The methods createPublicKitRoomMenu, createKitMenu,
createPublicKitMenu, createECMenu, createInspectMenu, createInspectEcMenu,
createMainMenu, createKitRoomMenu, and createViewPublicKitMenu all hard-code the
row count 6; extract that to a single constant (e.g., add private static final
int MENU_ROWS = 6) and replace the literal 6 in each ChestMenu.builder(...) call
with MENU_ROWS to centralize configuration and simplify future changes.
In `@src/main/java/dev/noah/perplayerkit/PerPlayerKit.java`:
- Line 42: Replace the wildcard import "import
dev.noah.perplayerkit.listeners.*;" with explicit imports for each listener
class to match the refactor style used for commands: add individual imports for
JoinListener, QuitListener, KitMenuCloseListener, KitRoomSaveListener, and
AutoRekitListener and remove the wildcard; locate the import line in
PerPlayerKit and update it to import those five listener classes explicitly.
In
`@src/test/java/dev/noah/perplayerkit/commands/AbstractShareSlotCommandTest.java`:
- Around line 48-60: The test invalidSlotDoesNotExecuteAction currently asserts
the exact kit-specific message ("Select a valid kit slot") which couples the
shared AbstractShareSlotCommand test to a concrete wording; update the assertion
on player.sendMessage in AbstractShareSlotCommandTest (the TestShareSlotCommand
usage) to check for a generic validation failure (e.g., assert sendMessage was
called with a message containing "Select a valid" or simply any non-empty
validation message) and leave the exact "Select a valid kit slot" wording
assertion to the concrete command tests (e.g., ShareECKitCommandTest); keep the
SoundManager.playFailure verification as-is.
In `@src/test/java/dev/noah/perplayerkit/commands/SlotArgumentParserTest.java`:
- Around line 21-29: Add unit tests covering boundary values and edge cases for
SlotArgumentParser: create tests that assert parseSlotInRange("1", 1, 9) returns
1 and parseSlotInRange("9", 1, 9) returns 9, and add tests asserting null for
invalid inputs such as empty string "", whitespace " ", negative numbers like
"-1" and non-numeric input (e.g., "a"); use method names like
parseSlotInRangeReturnsIntegerAtMinBoundary,
parseSlotInRangeReturnsIntegerAtMaxBoundary, and
parseSlotReturnsNullForInvalidInputs and call
SlotArgumentParser.parseSlotInRange and SlotArgumentParser.parseSlot to validate
behavior.
In `@src/test/java/dev/noah/perplayerkit/storage/SQLStorageTest.java`:
- Around line 56-65: Add an explicit assertion to the
keepAliveSucceedsWhenConnected test so it verifies state after calling
SQLStorage.keepAlive(); specifically, in
SQLStorageTest#keepAliveSucceedsWhenConnected assert that the storage reports a
connected state (e.g., call SQLStorage.isConnected() and assertTrue on its
result) after storage.keepAlive() and before storage.close(), to make the
expected post-condition explicit.
- Around line 125-131: The test currently returns fresh JDBC Connections from
getConnection() (DriverManager.getConnection(jdbcUrl)) without tracking, so
leaks in SQLStorage won't be detected; update the test to wrap/trap connections
returned by getConnection()—for example, in the overridden getConnection() store
each new Connection in a List<Connection> and return a proxy/decorator that
intercepts close() to mark it closed (or simply add the raw Connection to the
list and assert they are closed in tearDown) so you can verify all connections
created by getConnection() were closed by SQLStorage; reference getConnection(),
jdbcUrl, DriverManager and the SQLStorage usage in assertions.
In `@src/test/java/dev/noah/perplayerkit/storage/StorageMigratorTest.java`:
- Line 88: Replace the assertion that checks result.getErrorMessage() using
assertEquals(null, ...) with a direct null-check using assertNull to convey
intent and improve failure output; update the test in StorageMigratorTest (the
assertion in the test method referencing result.getErrorMessage()) to call
assertNull(result.getErrorMessage()) and ensure the appropriate JUnit assertNull
import or static import is present.
In `@src/test/java/dev/noah/perplayerkit/util/CooldownManagerTest.java`:
- Around line 26-34: The test keyExpiresAfterCooldownWindow uses Thread.sleep
which is slow and flaky; modify CooldownManager to accept a time source (e.g.,
java.time.Clock or a TimeProvider interface) and have its methods setCooldown
and isOnCooldown use that clock for time checks; update the production
constructor to default to Clock.systemUTC() but add an overloaded constructor or
setter for injecting a test Clock, then change the test to create a controllable
Clock (or fake TimeProvider), call cooldownManager.setCooldown("alpha"), advance
the fake clock by >1s, and assertFalse(cooldownManager.isOnCooldown("alpha"))
without any Thread.sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdc241b5-960a-4494-9fb1-da4fbcf29b4d
📒 Files selected for processing (49)
pom.xmlsrc/main/java/dev/noah/perplayerkit/PerPlayerKit.javasrc/main/java/dev/noah/perplayerkit/commands/DeleteKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/InspectEcCommand.javasrc/main/java/dev/noah/perplayerkit/commands/PerPlayerKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/ShortECCommand.javasrc/main/java/dev/noah/perplayerkit/commands/admin/AboutCommandListener.javasrc/main/java/dev/noah/perplayerkit/commands/admin/KitRoomCommand.javasrc/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/admin/SavePublicKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/completion/ECSlotTabCompleter.javasrc/main/java/dev/noah/perplayerkit/commands/completion/KitSlotTabCompleter.javasrc/main/java/dev/noah/perplayerkit/commands/core/CommandGuards.javasrc/main/java/dev/noah/perplayerkit/commands/core/SlotArgumentParser.javasrc/main/java/dev/noah/perplayerkit/commands/features/HealCommand.javasrc/main/java/dev/noah/perplayerkit/commands/features/RegearCommand.javasrc/main/java/dev/noah/perplayerkit/commands/features/RepairCommand.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/InspectEcCommand.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/InspectKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/kits/DeleteKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.javasrc/main/java/dev/noah/perplayerkit/commands/kits/MainMenuCommand.javasrc/main/java/dev/noah/perplayerkit/commands/kits/PublicKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/kits/SwapKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/share/AbstractShareSlotCommand.javasrc/main/java/dev/noah/perplayerkit/commands/share/CopyKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/share/ShareECKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/share/ShareKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/shortcuts/AbstractShortSlotCommand.javasrc/main/java/dev/noah/perplayerkit/commands/shortcuts/ShortECCommand.javasrc/main/java/dev/noah/perplayerkit/commands/shortcuts/ShortKitCommand.javasrc/main/java/dev/noah/perplayerkit/gui/GUI.javasrc/main/java/dev/noah/perplayerkit/gui/GuiLayoutUtils.javasrc/main/java/dev/noah/perplayerkit/gui/GuiMenuFactory.javasrc/main/java/dev/noah/perplayerkit/storage/RedisStorage.javasrc/main/java/dev/noah/perplayerkit/storage/StorageMigrator.javasrc/test/java/dev/noah/perplayerkit/commands/AbstractShareSlotCommandTest.javasrc/test/java/dev/noah/perplayerkit/commands/AbstractShortSlotCommandTest.javasrc/test/java/dev/noah/perplayerkit/commands/CommandGuardsTest.javasrc/test/java/dev/noah/perplayerkit/commands/SlotArgumentParserTest.javasrc/test/java/dev/noah/perplayerkit/storage/RedisStorageTest.javasrc/test/java/dev/noah/perplayerkit/storage/SQLStorageTest.javasrc/test/java/dev/noah/perplayerkit/storage/StorageMigratorTest.javasrc/test/java/dev/noah/perplayerkit/storage/StorageSelectorTest.javasrc/test/java/dev/noah/perplayerkit/storage/YAMLStorageTest.javasrc/test/java/dev/noah/perplayerkit/util/CooldownManagerTest.javasrc/test/java/dev/noah/perplayerkit/util/IDUtilTest.java
💤 Files with no reviewable changes (4)
- src/main/java/dev/noah/perplayerkit/commands/ShortECCommand.java
- src/main/java/dev/noah/perplayerkit/commands/DeleteKitCommand.java
- src/main/java/dev/noah/perplayerkit/commands/PerPlayerKitCommand.java
- src/main/java/dev/noah/perplayerkit/commands/InspectEcCommand.java
src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java
Show resolved
Hide resolved
src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/dev/noah/perplayerkit/commands/share/ShareECKitCommand.java
Outdated
Show resolved
Hide resolved
- Log Redis operation failures at `SEVERE` instead of `FINE` - Include exception string and full stack trace in Redis error logs - Remove the `papermc-repo` Maven repository entry from `pom.xml`
- Run CI on push events only for `main` - Keep pull request CI checks for all branches
- Return early after reporting missing import files - Prevent running the importer when required data is absent
- Reuse a single "player not found" path in inspect command handling - Return null when a UUID cannot be mapped to a known name instead of showing the UUID - Resolve identifiers via cached offline players first, then Mojang, without offline UUID fallback
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/dev/noah/perplayerkit/storage/RedisStorage.java (1)
147-156: Use standard logging API to include the cause chain.The manual stack trace logging doesn't capture chained exceptions (causes), which are often the actual root cause of failures. Additionally, logging both
exception.getMessage()andexception.toString()is redundant.Using
Logger.log(Level, String, Throwable)handles all of this automatically and is the idiomatic approach.♻️ Proposed refactor
+import java.util.logging.Level; + // ... at the method definition private void logRedisFailure(String operation, Exception exception) { if (plugin == null || plugin.getLogger() == null) { return; } - plugin.getLogger().severe("Redis " + operation + " failed: " + exception.getMessage()); - plugin.getLogger().severe(exception.toString()); - for (StackTraceElement element : exception.getStackTrace()) { - plugin.getLogger().severe("\tat " + element); - } + plugin.getLogger().log(Level.SEVERE, "Redis " + operation + " failed", exception); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/storage/RedisStorage.java` around lines 147 - 156, The logRedisFailure method currently prints exception.getMessage(), exception.toString() and iterates the stack trace manually; replace that with a single Logger.log call that passes the Throwable so chained causes are included and formatting is consistent: use plugin.getLogger().log(Level.SEVERE, "Redis " + operation + " failed", exception) inside logRedisFailure (ensure java.util.logging.Level is imported) and remove the manual calls to getMessage(), toString(), and the stack trace loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java`:
- Line 187: The onTabComplete implementation in PerPlayerKitCommand currently
returns null (causing Bukkit to suggest online player names); change the method
(PerPlayerKitCommand.onTabComplete) to return an empty list instead of null for
unhandled argument positions so no player-name suggestions appear (e.g. return
Collections.emptyList() or new ArrayList<>() where null is currently returned).
---
Nitpick comments:
In `@src/main/java/dev/noah/perplayerkit/storage/RedisStorage.java`:
- Around line 147-156: The logRedisFailure method currently prints
exception.getMessage(), exception.toString() and iterates the stack trace
manually; replace that with a single Logger.log call that passes the Throwable
so chained causes are included and formatting is consistent: use
plugin.getLogger().log(Level.SEVERE, "Redis " + operation + " failed",
exception) inside logRedisFailure (ensure java.util.logging.Level is imported)
and remove the manual calls to getMessage(), toString(), and the stack trace
loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54833783-3869-499c-9dc3-045177b6591c
📒 Files selected for processing (4)
.github/workflows/ci.ymlpom.xmlsrc/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.javasrc/main/java/dev/noah/perplayerkit/storage/RedisStorage.java
src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java
Show resolved
Hide resolved
…tCommand.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java
Show resolved
Hide resolved
src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java
Show resolved
Hide resolved
- move player name resolution into `PlayerUtil` with UUID fallback - add offline-mode UUID fallback in inspect identifier resolution - remove unused GUI wrapper methods and imports - add unit tests for inspect UUID selection and player name resolution
src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/dev/noah/perplayerkit/util/PlayerUtil.java (1)
84-85: Consider tighteningfallbackPlayerNamevisibility.If no other class in
dev.noah.perplayerkit.utilneeds this helper directly, make itprivateto keep the utility surface smaller.♻️ Minimal refactor
- static `@NotNull` String fallbackPlayerName(String playerName, `@NotNull` UUID uuid) { + private static `@NotNull` String fallbackPlayerName(String playerName, `@NotNull` UUID uuid) { return playerName != null ? playerName : uuid.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/util/PlayerUtil.java` around lines 84 - 85, The helper method fallbackPlayerName currently has package-private visibility; make it private to reduce the utility surface by changing its declaration to private static `@NotNull` String fallbackPlayerName(String playerName, `@NotNull` UUID uuid) in class PlayerUtil, then run a quick search for any external usages of PlayerUtil.fallbackPlayerName and update those callers (if any) to use a public API or move the logic into the caller; if external usages exist and cannot be removed, keep it package-private instead.src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java (1)
165-170: Prefix-filter the first tab-completion branches too.These branches always return the full option list, so
/perplayerkit m<TAB>still offers unrelated subcommands. Filtering them the same way as themigratebranches would make completion behavior consistent.💡 Suggested refactor
if (args.length == 1) { - return List.of("about", "import", "migrate"); + String prefix = args[0].toLowerCase(); + return List.of("about", "import", "migrate").stream() + .filter(option -> option.startsWith(prefix)) + .collect(Collectors.toList()); } if (args.length == 2 && args[0].equalsIgnoreCase("import")) { - return List.of("kitsx"); + return List.of("kitsx").stream() + .filter(option -> option.startsWith(args[1].toLowerCase())) + .collect(Collectors.toList()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java` around lines 165 - 170, The first-tab completion branches in PerPlayerKitCommand return full option lists unfiltered; update the branches that check args.length == 1 and args.length == 2 && args[0].equalsIgnoreCase("import") to filter the returned lists by the current prefix (args[0] for first-level completions and args[1] for the "import" second-level completion) using a case-insensitive starts-with check so only matching suggestions are returned; locate the tab-completion logic in PerPlayerKitCommand and replace the unfiltered List.of(...) returns with filtered collections based on the user's current partial input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java`:
- Around line 80-89: The KitsX import is run synchronously in
PerPlayerKitCommand (calls KitsXImporter.importFiles()), which can block the
main thread; move the import into an asynchronous task using
Bukkit.getScheduler().runTaskAsynchronously(plugin, ...) to call
importer.importFiles() off the main thread, and then wrap any
sender.sendMessage(...) callbacks that report completion or errors in
Bukkit.getScheduler().runTask(plugin, ...) to reschedule them back to the main
thread; reference PerPlayerKitCommand, KitsXImporter.importFiles, and the
existing migrate pattern that uses runTaskAsynchronously to replicate the same
async/then-main-thread behavior.
In
`@src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java`:
- Around line 98-107: The async completion captures the sender Player and then
uses it later without checking if they are still online; update
AbstractInspectCommand to avoid using the captured player in async callbacks by
re-resolving the sender on the main thread (e.g., call
Bukkit.getPlayer(player.getUniqueId()) or pass only the sender UUID into the
async chain) before calling showPlayerNotFound or scheduling showInspectResult,
and if the re-resolved player is null return early; similarly, before opening
GUIs or sending messages in the thenRun/main-thread scheduler, re-resolve the
target and sender Players (or bail out) and keep calls to
KitManager.get().loadPlayerDataFromDB(targetUuid) confined to non-main thread
only.
In
`@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java`:
- Around line 142-155: The selectResolvedUuid method currently calls
mojangLookup.get() before generating the offline-mode UUID, causing offline
servers to resolve to Mojang UUIDs; change the control flow in
selectResolvedUuid so that after checking cachedOfflinePlayer == null you first
check !onlineMode and if so return
UUID.nameUUIDFromBytes(("OfflinePlayer:"+identifier).getBytes(StandardCharsets.UTF_8))
without invoking mojangLookup, otherwise call mojangLookup.get() and return its
result; keep references to cachedOfflinePlayer, mojangLookup, onlineMode and
identifier to find and update the logic.
In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java`:
- Around line 182-185: InspectEc currently uses getPlayerName(target) and passes
it directly to GuiMenuFactory.createInspectEcMenu, which can be null; update
InspectEc to handle a null playerName (similar to InspectKit) by checking the
result of getPlayerName(target) and providing a safe fallback (e.g.,
target.toString() or "Unknown Player") before calling
GuiMenuFactory.createInspectEcMenu so the menu creation never receives a null
name.
---
Nitpick comments:
In `@src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java`:
- Around line 165-170: The first-tab completion branches in PerPlayerKitCommand
return full option lists unfiltered; update the branches that check args.length
== 1 and args.length == 2 && args[0].equalsIgnoreCase("import") to filter the
returned lists by the current prefix (args[0] for first-level completions and
args[1] for the "import" second-level completion) using a case-insensitive
starts-with check so only matching suggestions are returned; locate the
tab-completion logic in PerPlayerKitCommand and replace the unfiltered
List.of(...) returns with filtered collections based on the user's current
partial input.
In `@src/main/java/dev/noah/perplayerkit/util/PlayerUtil.java`:
- Around line 84-85: The helper method fallbackPlayerName currently has
package-private visibility; make it private to reduce the utility surface by
changing its declaration to private static `@NotNull` String
fallbackPlayerName(String playerName, `@NotNull` UUID uuid) in class PlayerUtil,
then run a quick search for any external usages of PlayerUtil.fallbackPlayerName
and update those callers (if any) to use a public API or move the logic into the
caller; if external usages exist and cannot be removed, keep it package-private
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a090820-c335-4b2f-a88e-7346f4fa4c4a
📒 Files selected for processing (8)
src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.javasrc/main/java/dev/noah/perplayerkit/commands/share/ShareECKitCommand.javasrc/main/java/dev/noah/perplayerkit/gui/GUI.javasrc/main/java/dev/noah/perplayerkit/util/PlayerUtil.javasrc/test/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtilTest.javasrc/test/java/dev/noah/perplayerkit/util/PlayerUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/dev/noah/perplayerkit/commands/share/ShareECKitCommand.java
src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java
Show resolved
Hide resolved
src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java
Outdated
Show resolved
Hide resolved
- replace offline player iteration with reflective `getOfflinePlayerIfCached` lookup - add test to verify `Bukkit.getOfflinePlayers()` is never called
src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java (1)
162-175:⚠️ Potential issue | 🟠 MajorCheck offline-mode before calling Mojang.
Line 168 still invokes
mojangLookupbefore the offline-mode fallback. On offline-mode servers that can resolve to a premium UUID instead of the server’sOfflinePlayer:<name>UUID, so inspect looks under the wrong storage key for valid data.Suggested fix
static `@Nullable` UUID selectResolvedUuid(`@NotNull` String identifier, `@Nullable` UUID cachedOfflinePlayer, `@NotNull` Supplier<UUID> mojangLookup, boolean onlineMode) { if (cachedOfflinePlayer != null) { return cachedOfflinePlayer; } + if (!onlineMode) { + return UUID.nameUUIDFromBytes(("OfflinePlayer:" + identifier).getBytes(StandardCharsets.UTF_8)); + } + UUID mojangUuid = mojangLookup.get(); if (mojangUuid != null) { return mojangUuid; } - - if (!onlineMode) { - return UUID.nameUUIDFromBytes(("OfflinePlayer:" + identifier).getBytes(StandardCharsets.UTF_8)); - } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java` around lines 162 - 175, In selectResolvedUuid, avoid invoking mojangLookup when the server is offline: first return cachedOfflinePlayer if non-null, then if onlineMode is false immediately return the OfflinePlayer UUID (UUID.nameUUIDFromBytes(...)), and only call mojangLookup.get() when onlineMode is true; ensure the method then returns mojangUuid if non-null or null otherwise. This prevents resolving to a premium UUID on offline servers and keeps the logic centered in selectResolvedUuid (referencing cachedOfflinePlayer, onlineMode and mojangLookup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtilTest.java`:
- Around line 42-48: Update the test for InspectCommandUtil.selectResolvedUuid
to ensure offline-mode never consults Mojang by supplying a non-null supplier
that either throws or returns a distinct UUID and then asserting the returned
UUID equals the offline-mode UUID; specifically, call
InspectCommandUtil.selectResolvedUuid(identifier, null, () -> { throw new
RuntimeException("mojang should not be consulted"); }, true) or use a supplier
that returns UUID.randomUUID(), and assert the result equals
UUID.nameUUIDFromBytes(("OfflinePlayer:" +
identifier).getBytes(StandardCharsets.UTF_8)) so the offline UUID always wins
even when a supplier is provided.
---
Duplicate comments:
In
`@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java`:
- Around line 162-175: In selectResolvedUuid, avoid invoking mojangLookup when
the server is offline: first return cachedOfflinePlayer if non-null, then if
onlineMode is false immediately return the OfflinePlayer UUID
(UUID.nameUUIDFromBytes(...)), and only call mojangLookup.get() when onlineMode
is true; ensure the method then returns mojangUuid if non-null or null
otherwise. This prevents resolving to a premium UUID on offline servers and
keeps the logic centered in selectResolvedUuid (referencing cachedOfflinePlayer,
onlineMode and mojangLookup).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00295b64-67c9-4a13-bd87-34ef467f942d
📒 Files selected for processing (2)
src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.javasrc/test/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtilTest.java
src/test/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtilTest.java
Outdated
Show resolved
Hide resolved
- Re-resolve sender/target on main thread before sending inspect results - Load target data only when offline to avoid unnecessary DB work - Fallback to UUID when target name is unavailable in inspect GUIs/messages
- Collapse the offline target data-load lambda to a single-line `runAsync` call - Keep inspect flow unchanged while improving readability around async composition
- Return `null` instead of an empty list when no completions are available - Restrict `/savepublickit` tab suggestions to the first argument only
- Return offline-mode UUID immediately when server is in offline mode - Update test to assert Mojang lookup is never consulted in offline mode
src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java
Show resolved
Hide resolved
src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/dev/noah/perplayerkit/gui/GUI.java (1)
152-153:⚠️ Potential issue | 🟡 MinorAdd the same null-name fallback in
InspectKit.
InspectEcnow guardsgetPlayerName(target), butInspectKitstill passes a nullable name straight into the title builder. For unresolved targets, the GUI title will show"null"instead of a stable fallback.Proposed fix
String playerName = getPlayerName(target); + if (playerName == null) { + playerName = target.toString(); + } Menu menu = GuiMenuFactory.createInspectMenu(slot, playerName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java` around lines 152 - 153, InspectKit is still passing a nullable result from getPlayerName(target) into GuiMenuFactory.createInspectMenu which causes the title to show "null"; update the GUI code path that builds the inspect kit menu to apply the same null-name fallback used by InspectEc (i.e., compute playerName = getPlayerName(target); if null or empty substitute the stable fallback string) before calling GuiMenuFactory.createInspectMenu(slot, playerName) so the title never receives a null value.
🧹 Nitpick comments (1)
src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java (1)
121-160: Reuse OkHttpClient instance and add UUID length validation.Two issues:
Performance: Creating a new
OkHttpClientfor each request is inefficient. OkHttpClient maintains connection pools and thread pools that should be reused.Potential exception: Lines 146-150 assume the raw UUID is exactly 32 characters. If Mojang returns a malformed response,
substring()will throwStringIndexOutOfBoundsException, which isn't caught.♻️ Proposed fix
+ private static final OkHttpClient HTTP_CLIENT = new OkHttpClient(); + private static `@Nullable` UUID lookupPlayerUuidFromMojang(`@NotNull` String identifier) { try { - OkHttpClient client = new OkHttpClient(); Request request = new Request.Builder() .url("https://api.mojang.com/users/profiles/minecraft/" + identifier) .build(); - Response response = client.newCall(request).execute(); + Response response = HTTP_CLIENT.newCall(request).execute(); try { if (!response.isSuccessful() || response.body() == null) { return null; } String body = response.body().string(); int idStart = body.indexOf("\"id\":\""); if (idStart < 0) { return null; } idStart += 6; int idEnd = body.indexOf("\"", idStart); if (idEnd <= idStart) { return null; } String raw = body.substring(idStart, idEnd); + if (raw.length() != 32) { + return null; + } String formatted = raw.substring(0, 8) + "-"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java` around lines 121 - 160, The method lookupPlayerUuidFromMojang currently creates a new OkHttpClient for each call and assumes the returned "id" string is 32 chars; reuse a shared OkHttpClient instance (e.g., a static final OkHttpClient) instead of constructing one per invocation, and add validation for the extracted raw id before calling substring/UUID.fromString (check raw != null and raw.length() == 32) to avoid StringIndexOutOfBoundsException and IllegalArgumentException; ensure response body is closed in a finally block as already present and return null on validation failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/dev/noah/perplayerkit/commands/admin/SavePublicKitCommand.java`:
- Around line 95-99: The current branch in SavePublicKitCommand returns the
mapped list of public kit ids even when empty; change the logic in that
args.length == 1 block to collect the ids from
KitManager.get().getPublicKitList() into a List, and if that list is empty
return null, otherwise return the list (i.e., compute List<String> ids =
...map(kit -> kit.id).toList(); return ids.isEmpty() ? null : ids;). Ensure you
reference the existing mapping expression (kit -> kit.id) and replace the direct
return with the empty-check-and-return.
In
`@src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java`:
- Around line 114-116: The async load in AbstractInspectCommand (the
CompletableFuture.runAsync calling
KitManager.get().loadPlayerDataFromDB(targetUuid)) performs cache mutations to
KitManager.kitByKitIDMap off the main thread while KitManager.hasKit()/hasEC()
read that same HashMap on the main thread, causing races; change the flow so
deserialize/compute work stays async but any modifications to kitByKitIDMap are
applied on the main thread (e.g., return the deserialized slot->kit map from
loadPlayerDataFromDB and schedule a main-thread task to apply the puts before
calling showInspectResult), or alternatively make KitManager.kitByKitIDMap
access thread-safe by synchronizing all mutations/reads in KitManager (affecting
loadPlayerDataFromDB, hasKit, hasEC, and any other direct map access) so
concurrent inspect requests cannot corrupt or see partially-applied state.
In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java`:
- Around line 329-333: The call to setArmorAndOffhandIndicators(menu) is
overwriting the kit preview slots (kit[36..40] mapped to slots 45–49) with
hardcoded labels, so either remove that call from this view or move the
indicator labels to different slots so the actual armor/offhand items remain
visible; update the code around setArmorAndOffhandIndicators(menu) in GUI
(and/or the implementation of setArmorAndOffhandIndicators) to avoid writing
into slots 45–49 (or choose alternative indicator slots) before calling
addLoadPublicKit(menu.getSlot(CLEAR_SLOT), id) so the preview shows the kit
items instead of the labels.
---
Duplicate comments:
In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java`:
- Around line 152-153: InspectKit is still passing a nullable result from
getPlayerName(target) into GuiMenuFactory.createInspectMenu which causes the
title to show "null"; update the GUI code path that builds the inspect kit menu
to apply the same null-name fallback used by InspectEc (i.e., compute playerName
= getPlayerName(target); if null or empty substitute the stable fallback string)
before calling GuiMenuFactory.createInspectMenu(slot, playerName) so the title
never receives a null value.
---
Nitpick comments:
In
`@src/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.java`:
- Around line 121-160: The method lookupPlayerUuidFromMojang currently creates a
new OkHttpClient for each call and assumes the returned "id" string is 32 chars;
reuse a shared OkHttpClient instance (e.g., a static final OkHttpClient) instead
of constructing one per invocation, and add validation for the extracted raw id
before calling substring/UUID.fromString (check raw != null and raw.length() ==
32) to avoid StringIndexOutOfBoundsException and IllegalArgumentException;
ensure response body is closed in a finally block as already present and return
null on validation failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6990f5c0-a628-41c0-81cd-cae8532a3d03
📒 Files selected for processing (6)
src/main/java/dev/noah/perplayerkit/commands/admin/PerPlayerKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/admin/SavePublicKitCommand.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.javasrc/main/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtil.javasrc/main/java/dev/noah/perplayerkit/gui/GUI.javasrc/test/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/dev/noah/perplayerkit/commands/inspect/InspectCommandUtilTest.java
src/main/java/dev/noah/perplayerkit/commands/admin/SavePublicKitCommand.java
Show resolved
Hide resolved
src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java
Show resolved
Hide resolved
- Compute per-file year or year range from git history and update existing headers - Resolve repo root automatically and process Java files from project root - Stop adding armor/offhand indicators in the load-kit GUI view
- Resolve identifiers to a UUID in all cases by using OfflinePlayer fallback after Mojang miss - Remove `onlineMode` branching from `selectResolvedUuid` - Update tests to cover Mojang-miss fallback behavior and non-null resolution
- Store public kit IDs before returning tab-complete results - Return `null` instead of an empty list when no public kits exist
- Replace `HashMap` with `ConcurrentHashMap` for `kitByKitIDMap` - Generalize field type to `Map<String, ItemStack[]>` for cleaner abstraction
- Add `LOAD_PUBLIC_KIT_SLOT` in layout constants - Wire LOAD KIT button and handler to the new slot constant in `GUI`
src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java
Show resolved
Hide resolved
- Check inspect command access via the command's configured permission - Apply armor/offhand indicator setup when opening the public kit load menu
src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java
Show resolved
Hide resolved
- Add `cacheKit` helper in `KitManager` and route cache writes through it - Remove deterministic offline-mode UUID fallback in inspect resolution - Update inspect tests to expect `null` when all UUID lookups miss
src/main/java/dev/noah/perplayerkit/commands/inspect/AbstractInspectCommand.java
Outdated
Show resolved
Hide resolved
- Let command execution and tab completion proceed without local permission gating - Delete unused `hasCommandPermission` helper in `AbstractInspectCommand`
- Avoid deserializing Base64 item arrays twice in kit and EC cache paths
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
src/main/java/dev/noah/perplayerkit/commands/kits/DeleteKitCommand.java
Outdated
Show resolved
Hide resolved
- Use `parseSlotInRange(..., 1, 9)` in `/deletekit` and `/swapkit` - Reject out-of-range slot arguments before command execution
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Touches core runtime paths (command handling, GUI building, kit caching, and storage error handling) and introduces concurrency changes, so regressions are possible despite added test coverage.
Overview
Refactors command and GUI code by reorganizing commands into feature-based packages, adding shared helpers (
CommandGuards,SlotArgumentParser, inspect/share/shortcut abstract bases), and simplifying several command implementations (notablyRegearCommand, inspect, share, and shortcut commands) while keeping behavior consistent.Updates kit caching in
KitManagerto use aConcurrentHashMapwith a smallcacheKithelper, extracts GUI layout/menu creation intoGuiLayoutUtils/GuiMenuFactory, improves Redis error logging, and makesStorageMigratormore testable via an overridable factory method.Build/CI is adjusted to run
mvn verifywith new JUnit/Mockito test dependencies (plus test SQLite), adds substantial unit test coverage across commands, storage, and utilities, and restricts CI push triggers tomain.Written by Cursor Bugbot for commit 1f9bd0d. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores