Skip to content

Add configurable GUI service and migrate menu flows to YAML-driven definitions#46

Open
rossnoah wants to merge 1 commit intomainfrom
t3code/configurable-gui-system-redesign
Open

Add configurable GUI service and migrate menu flows to YAML-driven definitions#46
rossnoah wants to merge 1 commit intomainfrom
t3code/configurable-gui-system-redesign

Conversation

@rossnoah
Copy link
Owner

@rossnoah rossnoah commented Mar 12, 2026

Summary

  • Introduce ConfigurableGuiService as the central GUI engine and initialize it during plugin startup.
  • Route existing GUI entry points in GUI and EnderchestCommand through the new service, replacing hardcoded menu construction.
  • Add GUI configuration support via GuiConfigManager, GuiContext, and a new guis.yml with menu layouts, items, and actions.
  • Remove legacy GUI-specific listener registrations now handled by the configurable GUI flow.

Testing

  • Not run (runtime/manual testing not performed in this environment).
  • Verified statically that GUI open methods now delegate to ConfigurableGuiService for main menu, kit editors, inspect views, public kits, and view-only ender chest.

Summary by CodeRabbit

  • New Features
    • Configuration-driven GUI system for customizable menus.
    • New interfaces for player kits, public kits, enderchests, and view-only enderchest.
    • Kit room with categories and pagination, plus public kit viewer.
    • Admin inspection, import/export, save/delete controls and permission-guarded actions.
    • Dynamic item states and contextual navigation across menus.

@cursor
Copy link

cursor bot commented Mar 12, 2026

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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 12, 2026

📝 Walkthrough

Walkthrough

A 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

Cohort / File(s) Summary
Core Service
src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java
New, feature-complete service implementing config-driven menu creation, rendering, actions, context propagation, item/material resolution, and persistence hooks.
Config & Context
src/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.java, src/main/java/dev/noah/perplayerkit/gui/configurable/GuiContext.java, src/main/resources/guis.yml
Adds config loader, immutable GuiContext helper, and comprehensive guis.yml containing definitions for 10+ GUIs, variants, actions, and permission guards.
Facade / Refactor
src/main/java/dev/noah/perplayerkit/gui/GUI.java
Stripped internal GUI building logic; public methods now delegate to ConfigurableGuiService (open/edit/inspect menus). Constructor body reduced.
Plugin bootstrap
src/main/java/dev/noah/perplayerkit/PerPlayerKit.java
Instantiates ConfigurableGuiService during plugin enable; removed registrations for KitMenuCloseListener and KitRoomSaveListener.
Command delegation
src/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.java
Replaces inline view-only enderchest menu construction with ConfigurableGuiService.get().openViewOnlyEnderchest(player).

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~70 minutes

Possibly related PRs

Poem

🐇 I hopped through YAML, found menus to sew,

Buttons and pages in tidy row,
A service now hums where builders once sprawled,
Players click happily — no hacks to be called,
Hooray for config, the rabbit says so! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: introducing a ConfigurableGuiService and migrating menu flows to YAML-driven definitions, which is the central focus of all file modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch t3code/configurable-gui-system-redesign
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.java still 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1389868 and 270a08f.

📒 Files selected for processing (7)
  • src/main/java/dev/noah/perplayerkit/PerPlayerKit.java
  • src/main/java/dev/noah/perplayerkit/commands/EnderchestCommand.java
  • src/main/java/dev/noah/perplayerkit/gui/GUI.java
  • src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java
  • src/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.java
  • src/main/java/dev/noah/perplayerkit/gui/configurable/GuiContext.java
  • src/main/resources/guis.yml

Comment on lines +266 to +287
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Comment on lines +1066 to +1077
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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Comment on lines +48 to +49
- "<gray>● Left click to load kit</gray>"
- "<gray>● Right click to edit kit</gray>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 270a08f and 8e574ef.

📒 Files selected for processing (3)
  • src/main/java/dev/noah/perplayerkit/PerPlayerKit.java
  • src/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.java
  • src/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
@rossnoah rossnoah force-pushed the t3code/configurable-gui-system-redesign branch from 8e574ef to 1fe3d84 Compare March 13, 2026 02:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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: Prefer StringBuilder over StringBuffer for single-threaded code.

StringBuffer is synchronized, adding unnecessary overhead. Since this method runs on the main thread per call, StringBuilder is 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.yml exists 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8e574ef and 1fe3d84.

📒 Files selected for processing (7)
  • src/main/java/dev/noah/perplayerkit/PerPlayerKit.java
  • src/main/java/dev/noah/perplayerkit/commands/kits/EnderchestCommand.java
  • src/main/java/dev/noah/perplayerkit/gui/GUI.java
  • src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java
  • src/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.java
  • src/main/java/dev/noah/perplayerkit/gui/configurable/GuiContext.java
  • src/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant