Prevent Public Kit load spam and add display name support for %player% - #29
Conversation
WalkthroughThe GUI methods for loading public kits were updated to close the menu immediately after a kit is loaded by the player. Additionally, a new configuration option was added to control whether broadcast messages use the player's display name or username, with corresponding logic updated in the broadcast message method. No public API signatures were changed. Changes
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/dev/noah/perplayerkit/gui/GUI.java (2)
64-66: Consistent GUI close sound on public kit load (addLoadPublicKit)
The menu is closed but no close sound is played; addSoundManager.playCloseGui(player)for parity with other load methods.slot.setClickHandler((player, info) -> { SoundManager.playClick(player); KitManager.get().loadPublicKit(player, id); - info.getClickedMenu().close();+ info.getClickedMenu().close();+ SoundManager.playCloseGui(player); });
478-479: Consistent GUI close sound on public kit load (addPublicKitButton)
After closing the menu on left-click load, the close sound isn't triggered. AddSoundManager.playCloseGui(player)for UX consistency.if (info.getClickType() == ClickType.LEFT) { KitManager.get().loadPublicKit(player, id); - info.getClickedMenu().close();+ info.getClickedMenu().close();+ SoundManager.playCloseGui(player); } else if (info.getClickType() == ClickType.RIGHT) {
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/dev/noah/perplayerkit/util/BroadcastManager.java(1 hunks)src/main/resources/config.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/config.yml
🔇 Additional comments (2)
src/main/java/dev/noah/perplayerkit/util/BroadcastManager.java (2)
98-106: Good implementation of conditional player name handling.The logic correctly implements the feature to conditionally use display names based on configuration. The null safety is adequate since modern Bukkit implementations reliably provide display names.
100-100:Details
✅ Verification successful
Verify configuration key consistency with other files.
The AI summary mentions the configuration key as
publickits.use-display-name, but the code usesuse-display-name. Ensure the configuration key is consistent across all files.🏁 Script executed:
#!/bin/bash# Description: Check for configuration key consistency across the codebase# Expected: Find the correct configuration key name and verify consistency# Search for use-display-name configuration references rg -A 2 -B 2 "use-display-name"# Search for publickits.use-display-name configuration references rg -A 2 -B 2 "publickits\.use-display-name"# Check config.yml files for the actual key definition fd "config.yml" --exec cat {} \;Length of output: 5990
Configuration key consistency confirmed
I’ve verified that
config.ymldefines theuse-display-namekey at the root level (nopublickits.use-display-nameexists) and the code references the same key. No changes are needed.
| String playerName; | ||
| if (plugin.getConfig().getBoolean("use-display-name", false)) { | ||
| playerName = player.getDisplayName(); | ||
| } else { | ||
| playerName = player.getName(); | ||
| } | ||
| message = message.replace("%player%", playerName); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider security implications and performance optimization.
Using player display names introduces potential security risks as display names can contain formatting codes or malicious content set by other plugins. Additionally, the configuration is read on every method call.
Consider these improvements:
+ private final boolean useDisplayName;+
public BroadcastManager(Plugin plugin) {
this.plugin = plugin;
audience = BukkitAudiences.create(plugin);
prefix = MiniMessage.miniMessage().deserialize(plugin.getConfig().getString("prefix", "<gray>[<aqua>Kits</aqua>]</gray> "));
+ useDisplayName = plugin.getConfig().getBoolean("use-display-name", false);
instance = this;
}
- String playerName;- if (plugin.getConfig().getBoolean("use-display-name", false)) {- playerName = player.getDisplayName();- } else {- playerName = player.getName();- }+ String playerName = useDisplayName ? + stripFormattingCodes(player.getDisplayName()) : + player.getName();Add a helper method to strip potentially dangerous formatting:
privateStringstripFormattingCodes(StringdisplayName) {
// Strip common formatting codes to prevent injectionreturndisplayName.replaceAll("§[0-9a-fk-or]", "").replaceAll("&[0-9a-fk-or]", "");
}🤖 Prompt for AI Agents
In src/main/java/dev/noah/perplayerkit/util/BroadcastManager.java around lines
99 to 106, avoid using raw player display names directly due to security risks
from formatting codes or malicious content. Create a helper method to strip
formatting codes from display names, then use this sanitized name in the message
replacement. Also, optimize performance by caching the configuration value for
"use-display-name" instead of reading it on every method call.
Summary by CodeRabbit