Add configurable GUI service and migrate menu flows to YAML-driven definitions#46
Add configurable GUI service and migrate menu flows to YAML-driven definitions#46
Conversation
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
📝 WalkthroughWalkthroughA new config-driven GUI subsystem (ConfigurableGuiService + GuiConfigManager + GuiContext + guis.yml) is added and wired into the plugin. GUI.java and callers (PerPlayerKit, EnderchestCommand) are refactored to delegate GUI operations to the new service. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as Player
participant GUI as GUI (Facade)
participant Service as ConfigurableGuiService
participant Manager as GuiConfigManager
participant YAML as guis.yml
Player->>GUI: OpenKitMenu(player, slot)
GUI->>Service: openPlayerKitEditor(player, slot)
Service->>Manager: getGuiSection("player-kit-editor")
Manager->>YAML: read gui definition
YAML-->>Manager: GUI section
Manager-->>Service: ConfigurationSection
Service->>Service: createMenu(viewer, context)
Service->>Player: open Inventory (menu)
Player->>Service: click slot -> action
Service->>Service: execute action (e.g., load-player-kit / open-gui)
Service->>Player: update/close menu, play sound
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/dev/noah/perplayerkit/gui/GUI.java (1)
32-40: Don't keep the legacy state helpers as silent no-ops.If any legacy caller survives the migration, these methods now fail silently instead of surfacing the bad path.
src/main/java/dev/noah/perplayerkit/listeners/KitMenuCloseListener.javastill depends on this API, so I'd either remove the compatibility surface or deprecate it with an explicit failure/message while the old flow is being retired.🤖 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 32 - 40, These legacy state helper methods (setInspectTarget, getAndRemoveInspectTarget, removeKitDeletionFlag) currently no-op and silently hide callers like KitMenuCloseListener; update them to explicitly fail fast by throwing an UnsupportedOperationException (or at minimum log a clear warning and throw) with a message directing callers to the new API/flow so surviving callers are surfaced during testing, or alternatively implement the original behavior if backward compatibility must be preserved; ensure the exception message names the method (e.g., "GUI.setInspectTarget") and references KitMenuCloseListener migration guidance.
🤖 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/gui/configurable/ConfigurableGuiService.java`:
- Around line 266-287: renderPublicKitList currently only displays up to
slots.size() items, leaving PublicKit entries past that inaccessible; modify
renderPublicKitList to support pagination by reading a page index from
baseContext (e.g., "public_kit_page"), computing int start = page * slots.size()
and iterating from start to Math.min(start + slots.size(), publicKits.size())
when pulling from KitManager.get().getPublicKitList(), and render Prev/Next
controls (using applyVariant on dedicated control slots) that reopen the GUI
with updated "public_kit_page" in the GuiContext; alternatively, if you prefer
rejecting oversized configs, add a validation path that checks if
publicKits.size() > maxVisible (slots.size()) and logs/throws a clear config
validation error instead of silently truncating.
- Around line 1066-1077: resolveInt currently parses string values literally and
immediately returns defaultValue on NumberFormatException; instead, if the
string isn't a plain integer (e.g. contains a placeholder like "%slot%") or
parsing fails, try resolving the value from the GuiContext using the provided
fallbackContextKey before returning defaultValue. Update resolveInt to: when
rawValue is a String that fails Integer.parseInt, call
context.get(fallbackContextKey) (or the appropriate accessor on GuiContext) and
attempt to parse that result (or its toString()) to an int; also if rawValue is
null, try the same context lookup; only return defaultValue if both direct
parsing and the context-resolved value are unavailable or non-numeric. Use the
resolveInt function name and the fallbackContextKey/GuiContext symbols to locate
and modify the logic.
In `@src/main/resources/guis.yml`:
- Around line 48-49: The two label strings "<gray>● Left click to load
kit</gray>" and "<gray>● Right click to edit kit</gray>" are incorrect for the
enderchest menu; update them to say "enderchest" (e.g., "<gray>● Left click to
load enderchest</gray>" and "<gray>● Right click to edit enderchest</gray>") and
search the file for any other occurrences of "kit" in enderchest-related labels
(notably the occurrence around line 387 mentioned in the comment) and replace
them with the appropriate "enderchest" wording so the menu copy matches the
enderchest flow.
---
Nitpick comments:
In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java`:
- Around line 32-40: These legacy state helper methods (setInspectTarget,
getAndRemoveInspectTarget, removeKitDeletionFlag) currently no-op and silently
hide callers like KitMenuCloseListener; update them to explicitly fail fast by
throwing an UnsupportedOperationException (or at minimum log a clear warning and
throw) with a message directing callers to the new API/flow so surviving callers
are surfaced during testing, or alternatively implement the original behavior if
backward compatibility must be preserved; ensure the exception message names the
method (e.g., "GUI.setInspectTarget") and references KitMenuCloseListener
migration guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9690b1f1-d594-404d-9794-884b4beec603
📒 Files selected for processing (7)
src/main/java/dev/noah/perplayerkit/PerPlayerKit.javasrc/main/java/dev/noah/perplayerkit/commands/EnderchestCommand.javasrc/main/java/dev/noah/perplayerkit/gui/GUI.javasrc/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.javasrc/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.javasrc/main/java/dev/noah/perplayerkit/gui/configurable/GuiContext.javasrc/main/resources/guis.yml
| private void renderPublicKitList(Menu menu, Player viewer, ConfigurationSection elementSection, GuiContext baseContext, List<Integer> slots, AtomicBoolean skipCloseSave) { | ||
| List<PublicKit> publicKits = KitManager.get().getPublicKitList(); | ||
| boolean admin = viewer.hasPermission("perplayerkit.admin"); | ||
|
|
||
| for (int i = 0; i < Math.min(slots.size(), publicKits.size()); i++) { | ||
| PublicKit publicKit = publicKits.get(i); | ||
| boolean assigned = KitManager.get().hasPublicKit(publicKit.id); | ||
| String variant; | ||
|
|
||
| if (admin) { | ||
| variant = assigned ? "admin_assigned" : "admin_unassigned"; | ||
| } else { | ||
| variant = assigned ? "assigned" : "unassigned"; | ||
| } | ||
|
|
||
| GuiContext slotContext = baseContext | ||
| .with("public_kit_id", publicKit.id) | ||
| .with("public_kit_name", publicKit.name) | ||
| .with("public_kit_icon", publicKit.icon); | ||
|
|
||
| applyVariant(menu.getSlot(slots.get(i)), viewer, elementSection, variant, slotContext, skipCloseSave); | ||
| } |
There was a problem hiding this comment.
Public kits past the visible slot count are unreachable.
This renderer stops at slots.size() with no paging or warning. src/main/java/dev/noah/perplayerkit/PerPlayerKit.java Lines 243-256 still loads every configured public kit, but the default public-kit-menu in src/main/resources/guis.yml Lines 562-566 only exposes 18 list slots, so kit 19+ has no load/view/edit path. Please add pagination here or reject oversized public-kit configs explicitly.
🤖 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/configurable/ConfigurableGuiService.java`
around lines 266 - 287, renderPublicKitList currently only displays up to
slots.size() items, leaving PublicKit entries past that inaccessible; modify
renderPublicKitList to support pagination by reading a page index from
baseContext (e.g., "public_kit_page"), computing int start = page * slots.size()
and iterating from start to Math.min(start + slots.size(), publicKits.size())
when pulling from KitManager.get().getPublicKitList(), and render Prev/Next
controls (using applyVariant on dedicated control slots) that reopen the GUI
with updated "public_kit_page" in the GuiContext; alternatively, if you prefer
rejecting oversized configs, add a validation path that checks if
publicKits.size() > maxVisible (slots.size()) and logs/throws a clear config
validation error instead of silently truncating.
| private int resolveInt(Map<?, ?> action, String key, GuiContext context, String fallbackContextKey, int defaultValue) { | ||
| Object rawValue = action.get(key); | ||
| if (rawValue instanceof Number number) { | ||
| return number.intValue(); | ||
| } | ||
| if (rawValue instanceof String stringValue) { | ||
| try { | ||
| return Integer.parseInt(stringValue); | ||
| } catch (NumberFormatException ignored) { | ||
| return defaultValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolve placeholder-backed ints from context before defaulting.
Line 1073 parses "%slot%" literally, and Line 1075 immediately falls back to defaultValue. That breaks the inspect delete actions in src/main/resources/guis.yml Lines 476-480 and 527-531: deleting slot 5/6/etc. will still target slot 1.
Proposed fix
private int resolveInt(Map<?, ?> action, String key, GuiContext context, String fallbackContextKey, int defaultValue) {
Object rawValue = action.get(key);
if (rawValue instanceof Number number) {
return number.intValue();
}
if (rawValue instanceof String stringValue) {
+ if (stringValue.startsWith("%") && stringValue.endsWith("%")
+ && stringValue.indexOf('%', 1) == stringValue.length() - 1) {
+ Integer placeholderValue = context.getInt(stringValue.substring(1, stringValue.length() - 1));
+ if (placeholderValue != null) {
+ return placeholderValue;
+ }
+ }
try {
return Integer.parseInt(stringValue);
} catch (NumberFormatException ignored) {
- return defaultValue;
+ Integer contextValue = context.getInt(fallbackContextKey);
+ return contextValue != null ? contextValue : defaultValue;
}
}
Integer contextValue = context.getInt(fallbackContextKey);
return contextValue != null ? contextValue : defaultValue;
}🤖 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/configurable/ConfigurableGuiService.java`
around lines 1066 - 1077, resolveInt currently parses string values literally
and immediately returns defaultValue on NumberFormatException; instead, if the
string isn't a plain integer (e.g. contains a placeholder like "%slot%") or
parsing fails, try resolving the value from the GuiContext using the provided
fallbackContextKey before returning defaultValue. Update resolveInt to: when
rawValue is a String that fails Integer.parseInt, call
context.get(fallbackContextKey) (or the appropriate accessor on GuiContext) and
attempt to parse that result (or its toString()) to an int; also if rawValue is
null, try the same context lookup; only return defaultValue if both direct
parsing and the context-resolved value are unavailable or non-numeric. Use the
resolveInt function name and the fallbackContextKey/GuiContext symbols to locate
and modify the logic.
| - "<gray>● Left click to load kit</gray>" | ||
| - "<gray>● Right click to edit kit</gray>" |
There was a problem hiding this comment.
Fix the remaining enderchest labels.
These strings still say "kit", which makes the enderchest flow read like the wrong menu.
Proposed copy fix
- - "<gray>● Left click to load kit</gray>"
- - "<gray>● Right click to edit kit</gray>"
+ - "<gray>● Left click to load enderchest</gray>"
+ - "<gray>● Right click to edit enderchest</gray>"
...
- name: "<red><b>CLEAR KIT</b></red>"
+ name: "<red><b>CLEAR ENDERCHEST</b></red>"Also applies to: 387-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/guis.yml` around lines 48 - 49, The two label strings
"<gray>● Left click to load kit</gray>" and "<gray>● Right click to edit
kit</gray>" are incorrect for the enderchest menu; update them to say
"enderchest" (e.g., "<gray>● Left click to load enderchest</gray>" and "<gray>●
Right click to edit enderchest</gray>") and search the file for any other
occurrences of "kit" in enderchest-related labels (notably the occurrence around
line 387 mentioned in the comment) and replace them with the appropriate
"enderchest" wording so the menu copy matches the enderchest flow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/gui/GUI.java`:
- Around line 85-90: The method ViewPublicKitMenu currently returns a
possibly-null Menu from ConfigurableGuiService.createPublicKitViewer; make this
contract explicit by annotating the method return type with a nullable
annotation (e.g., `@Nullable` from javax.annotation or org.jetbrains.annotations)
and add the corresponding import, keeping the current behavior (open if
non-null, return value unchanged). Reference: the method ViewPublicKitMenu, the
call to ConfigurableGuiService.createPublicKitViewer(Player, String), and the
Menu/Player types so reviewers can locate and update the signature and imports
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dbd2f51-7acb-48c8-b2ea-f666affffc07
📒 Files selected for processing (3)
src/main/java/dev/noah/perplayerkit/PerPlayerKit.javasrc/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.javasrc/main/java/dev/noah/perplayerkit/gui/GUI.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/dev/noah/perplayerkit/PerPlayerKit.java
- introduce `ConfigurableGuiService`, `GuiConfigManager`, and `GuiContext` - move GUI behavior to config-backed definitions in `guis.yml` - update existing GUI entry points and `/enderchest` view to delegate to the new service - initialize the new GUI service during plugin startup and remove legacy GUI listeners
8e574ef to
1fe3d84
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java (2)
912-922: Consider caching PlaceholderAPI availability check.
Bukkit.getPluginManager().getPlugin("PlaceholderAPI")is called on every text resolution. Since plugin presence doesn't change at runtime, caching this once during initialization would improve performance.Proposed enhancement
Add a field and initialize it in the constructor:
private final boolean hasPlaceholderApi; public ConfigurableGuiService(Plugin plugin) { this.plugin = plugin; this.guiConfigManager = new GuiConfigManager(plugin); this.hasPlaceholderApi = Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null; instance = this; }Then use it in
resolveText:private String resolveText(String value, Player viewer, GuiContext context) { if (value == null) { return null; } String resolved = resolvePlainValue(value, viewer, context); - if (Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null) { + if (hasPlaceholderApi) { resolved = PlaceholderAPI.setPlaceholders(viewer, resolved); } return StyleManager.convertMiniMessage(resolved); }🤖 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/configurable/ConfigurableGuiService.java` around lines 912 - 922, The resolveText method repeatedly calls Bukkit.getPluginManager().getPlugin("PlaceholderAPI") on every invocation; add a private final boolean field hasPlaceholderApi to the ConfigurableGuiService class, initialize it once in the constructor (ConfigurableGuiService(...)) by checking Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null, and then replace the runtime check in resolveText with a check of hasPlaceholderApi so PlaceholderAPI.setPlaceholders(viewer, resolved) is only called when hasPlaceholderApi is true.
924-935: PreferStringBuilderoverStringBufferfor single-threaded code.
StringBufferis synchronized, adding unnecessary overhead. Since this method runs on the main thread per call,StringBuilderis more appropriate.Proposed fix
private String resolvePlainValue(String value, Player viewer, GuiContext context) { Matcher matcher = PLACEHOLDER_PATTERN.matcher(value); - StringBuffer buffer = new StringBuffer(); + StringBuilder buffer = new StringBuilder(); while (matcher.find()) { String replacement = getPlaceholderValue(matcher.group(1), viewer, context); matcher.appendReplacement(buffer, Matcher.quoteReplacement(replacement)); } matcher.appendTail(buffer); return buffer.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/gui/configurable/ConfigurableGuiService.java` around lines 924 - 935, resolvePlainValue currently uses synchronized StringBuffer via Matcher.appendReplacement/appendTail; replace it with an unsynchronized StringBuilder to avoid unnecessary overhead: keep PLACEHOLDER_PATTERN and the Matcher logic but change the loop to append the literal text between matches and the replacement into a StringBuilder using matcher.start()/matcher.end() (append the substring from the previous end index to matcher.start(), then append the replacement from getPlaceholderValue), track the last appended index, and after the loop append the remaining tail substring before returning builder.toString(); update the method resolvePlainValue accordingly.src/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.java (1)
21-26: Consider handling missing or malformed configuration more explicitly.If
guis.ymlexists but contains invalid YAML,YamlConfiguration.loadConfiguration()returns an empty configuration without throwing. This could cause silent failures downstream when GUI sections are requested. Consider logging a warning when the file exists but produces an empty/invalid configuration.Proposed enhancement
public final void load() { if (!guiConfigFile.exists()) { plugin.saveResource("guis.yml", false); } guiConfiguration = YamlConfiguration.loadConfiguration(guiConfigFile); + if (guiConfiguration.getKeys(false).isEmpty()) { + plugin.getLogger().warning("guis.yml appears to be empty or invalid"); + } }🤖 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/configurable/GuiConfigManager.java` around lines 21 - 26, The load() method currently calls YamlConfiguration.loadConfiguration(guiConfigFile) but doesn’t detect when the file exists yet yields an empty/invalid configuration; update load() to check the loaded guiConfiguration for emptiness (e.g., no top-level keys/sections) after YamlConfiguration.loadConfiguration(guiConfigFile) and if empty: log a warning via the plugin logger including the file name (guiConfigFile) and that the YAML appears invalid, then either attempt to restore the default with plugin.saveResource("guis.yml", true) or copy the bundled default into place and re-load; reference the existing symbols load(), guiConfigFile, guiConfiguration, plugin.saveResource and YamlConfiguration.loadConfiguration when implementing this check and recovery.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java`:
- Around line 912-922: The resolveText method repeatedly calls
Bukkit.getPluginManager().getPlugin("PlaceholderAPI") on every invocation; add a
private final boolean field hasPlaceholderApi to the ConfigurableGuiService
class, initialize it once in the constructor (ConfigurableGuiService(...)) by
checking Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null, and then
replace the runtime check in resolveText with a check of hasPlaceholderApi so
PlaceholderAPI.setPlaceholders(viewer, resolved) is only called when
hasPlaceholderApi is true.
- Around line 924-935: resolvePlainValue currently uses synchronized
StringBuffer via Matcher.appendReplacement/appendTail; replace it with an
unsynchronized StringBuilder to avoid unnecessary overhead: keep
PLACEHOLDER_PATTERN and the Matcher logic but change the loop to append the
literal text between matches and the replacement into a StringBuilder using
matcher.start()/matcher.end() (append the substring from the previous end index
to matcher.start(), then append the replacement from getPlaceholderValue), track
the last appended index, and after the loop append the remaining tail substring
before returning builder.toString(); update the method resolvePlainValue
accordingly.
In `@src/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.java`:
- Around line 21-26: The load() method currently calls
YamlConfiguration.loadConfiguration(guiConfigFile) but doesn’t detect when the
file exists yet yields an empty/invalid configuration; update load() to check
the loaded guiConfiguration for emptiness (e.g., no top-level keys/sections)
after YamlConfiguration.loadConfiguration(guiConfigFile) and if empty: log a
warning via the plugin logger including the file name (guiConfigFile) and that
the YAML appears invalid, then either attempt to restore the default with
plugin.saveResource("guis.yml", true) or copy the bundled default into place and
re-load; reference the existing symbols load(), guiConfigFile, guiConfiguration,
plugin.saveResource and YamlConfiguration.loadConfiguration when implementing
this check and recovery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93237830-abcf-436c-9a09-d1d43b4d6004
📒 Files selected for processing (7)
src/main/java/dev/noah/perplayerkit/PerPlayerKit.javasrc/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.javasrc/main/java/dev/noah/perplayerkit/gui/GUI.javasrc/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.javasrc/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.javasrc/main/java/dev/noah/perplayerkit/gui/configurable/GuiContext.javasrc/main/resources/guis.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/resources/guis.yml
- src/main/java/dev/noah/perplayerkit/gui/GUI.java
- src/main/java/dev/noah/perplayerkit/PerPlayerKit.java
- src/main/java/dev/noah/perplayerkit/gui/configurable/GuiContext.java
Summary
ConfigurableGuiServiceas the central GUI engine and initialize it during plugin startup.GUIandEnderchestCommandthrough the new service, replacing hardcoded menu construction.GuiConfigManager,GuiContext, and a newguis.ymlwith menu layouts, items, and actions.Testing
ConfigurableGuiServicefor main menu, kit editors, inspect views, public kits, and view-only ender chest.Summary by CodeRabbit