Release 2.0.7.7 #86
Release 2.0.7.7 #86
Conversation
- Adds initial classes for Modrinth API integration and a VirtualThreadScheduler for handling asynchronous tasks. - Relocates all enum classes from the `utils.enums` package to `models.enums` to improve project structure and organization. - Bumped Spigot dependency to `1.21.8-R0.1-SNAPSHOT` - Bumped QS-Hikari dependency to `6.2.0.10`
…into feature/version-upgrades-sept-25
feat: Add Modrinth/async skeletons and relocate enums
WalkthroughRelease 2.0.7.7: bumped versions and dependencies, moved enum classes to models.enums, added Modrinth API client and async Modrinth-based update checking, introduced a VirtualThreadScheduler and migrated async tasks to it, added snapshot warnings and shutdown hook, and updated documentation badges. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S as Server
participant P as FindItemAddOn
participant UC as UpdateChecker
participant MS as ModrinthService
S->>P: onLoad()
P->>P: check snapshot version
S->>P: onEnable()
P->>UC: new UpdateChecker()
P->>UC: isUpdateAvailable(callback)
UC->>MS: getProjectVersions(projectId)
MS-->>UC: List<ProjectDetailsResponse>
UC->>UC: determine update available
UC-->>P: callback(Boolean)
P->>P: set isPluginOutdated
Note over P: onDisable(): VirtualThreadScheduler.shutdown()
sequenceDiagram
autonumber
participant Caller as Caller/Task
participant VT as VirtualThreadScheduler
participant Worker as WorkerCode
Caller->>VT: runTaskAsync(Runnable/Callable)
activate VT
VT->>Worker: execute on virtual thread
activate Worker
Worker-->>VT: complete (result/Future)
deactivate Worker
VT-->>Caller: Future
deactivate VT
Note over VT,Worker: Replaces prior Bukkit async scheduler usages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/io/myzticbean/finditemaddon/utils/json/ShopSearchActivityStorageUtil.java (1)
254-278: Concurrent modification risk on globalShopsList.The async task iterates and mutates globalShopsList without synchronization; concurrent readers/writers can cause data races and CME.
Apply synchronization here:
- VirtualThreadScheduler.runTaskAsync(() -> { - if(handleCooldownIfPresent(shopLocation, visitingPlayer)) { - Iterator<ShopSearchActivityModel> shopSearchActivityIterator = globalShopsList.iterator(); - int i = 0; - while(shopSearchActivityIterator.hasNext()) { - ShopSearchActivityModel shopSearchActivity = shopSearchActivityIterator.next(); - if(shopSearchActivity.compareWith( - shopLocation.getWorld().getName(), - shopLocation.getX(), - shopLocation.getY(), - shopLocation.getZ() - )) { - PlayerShopVisitModel playerShopVisit = new PlayerShopVisitModel(); - playerShopVisit.setPlayerUUID(visitingPlayer.getUniqueId()); - playerShopVisit.setVisitDateTime(); - globalShopsList.get(i).getPlayerVisitList().add(playerShopVisit); - Logger.logDebugInfo("Added new player visit entry at " + shopLocation); - break; - } - i++; - } - } - }); + VirtualThreadScheduler.runTaskAsync(() -> { + synchronized (globalShopsList) { + if (handleCooldownIfPresent(shopLocation, visitingPlayer)) { + Iterator<ShopSearchActivityModel> shopSearchActivityIterator = globalShopsList.iterator(); + int i = 0; + while (shopSearchActivityIterator.hasNext()) { + ShopSearchActivityModel shopSearchActivity = shopSearchActivityIterator.next(); + if (shopSearchActivity.compareWith( + shopLocation.getWorld().getName(), + shopLocation.getX(), + shopLocation.getY(), + shopLocation.getZ() + )) { + PlayerShopVisitModel playerShopVisit = new PlayerShopVisitModel(); + playerShopVisit.setPlayerUUID(visitingPlayer.getUniqueId()); + playerShopVisit.setVisitDateTime(); + globalShopsList.get(i).getPlayerVisitList().add(playerShopVisit); + Logger.logDebugInfo("Added new player visit entry at " + shopLocation); + break; + } + i++; + } + } + } + });Additionally, consider introducing a dedicated lock and using it consistently in readers (handleCooldownIfPresent, getPlayerVisitCount, getShopOwner, etc.):
private static final Object GLOBAL_SHOPS_LOCK = new Object(); // Then replace synchronized (globalShopsList) with synchronized (GLOBAL_SHOPS_LOCK)src/main/java/io/myzticbean/finditemaddon/utils/json/HiddenShopStorageUtil.java (1)
85-105: Concurrent modification risk when toggling hidden flag.Async mutation of the shared shops list without synchronization can race with other readers/writers.
Apply synchronization to protect iteration and mutation:
- public static void handleShopSearchVisibilityAsync(com.ghostchu.quickshop.api.shop.Shop shop, boolean hideShop) { - VirtualThreadScheduler.runTaskAsync(() -> { - Iterator<ShopSearchActivityModel> shopSearchActivityIterator = ShopSearchActivityStorageUtil.getGlobalShopsList().iterator(); - int i = 0; - while(shopSearchActivityIterator.hasNext()) { - ShopSearchActivityModel shopSearchActivity = shopSearchActivityIterator.next(); - Location shopLocation = shop.getLocation(); - if(shopSearchActivity.compareWith( - shopLocation.getWorld().getName(), - shopLocation.getX(), - shopLocation.getY(), - shopLocation.getZ() - )) { - ShopSearchActivityStorageUtil.getGlobalShopsList() - .get(i) - .setHiddenFromSearch(hideShop); - break; - } - i++; - } - }); - } + public static void handleShopSearchVisibilityAsync(com.ghostchu.quickshop.api.shop.Shop shop, boolean hideShop) { + VirtualThreadScheduler.runTaskAsync(() -> { + synchronized (ShopSearchActivityStorageUtil.getGlobalShopsList()) { + Iterator<ShopSearchActivityModel> it = ShopSearchActivityStorageUtil.getGlobalShopsList().iterator(); + int i = 0; + while (it.hasNext()) { + ShopSearchActivityModel s = it.next(); + Location shopLocation = shop.getLocation(); + if (s.compareWith( + shopLocation.getWorld().getName(), + shopLocation.getX(), + shopLocation.getY(), + shopLocation.getZ() + )) { + ShopSearchActivityStorageUtil.getGlobalShopsList().get(i).setHiddenFromSearch(hideShop); + break; + } + i++; + } + } + }); + }
🧹 Nitpick comments (18)
README.md (1)
2-2: Fix heading level (MD001).Change H3 to H2 to keep heading increments by one.
-### Version: 2.0.7.7-RELEASE +## Version: 2.0.7.7-RELEASEsrc/main/java/io/myzticbean/finditemaddon/utils/warp/WarpUtils.java (1)
33-39: Avoid magic numbers for NEAREST_WARP_MODE.Compare against the enum instead of 1/2 to improve readability and prevent config drift. Example: NearestWarpModeEnum.PLAYER_WARPS.value().
src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
42-45: Graceful shutdown: await termination and fallback to shutdownNow.Prevents lingering tasks on disable and handles interruption.
@@ - public static void shutdown() { - Logger.logInfo("Shutting down virtual thread executor..."); - VIRTUAL_EXECUTOR.shutdown(); - } + public static void shutdown() { + Logger.logInfo("Shutting down virtual thread executor..."); + VIRTUAL_EXECUTOR.shutdown(); + try { + if (!VIRTUAL_EXECUTOR.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS)) { + Logger.logInfo("Forcing virtual thread executor shutdown..."); + VIRTUAL_EXECUTOR.shutdownNow(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + VIRTUAL_EXECUTOR.shutdownNow(); + } + }src/main/java/io/myzticbean/finditemaddon/utils/json/HiddenShopStorageUtil.java (1)
58-78: Unify async scheduler (deprecated method still uses Bukkit scheduler).For consistency and to reduce code paths, consider switching the deprecated Reremake method to VirtualThreadScheduler as well.
- @Deprecated(since = "v2.0.7.7") - public static void handleShopSearchVisibilityAsync(Shop shop, boolean hideShop) { - Bukkit.getScheduler().runTaskAsynchronously(FindItemAddOn.getInstance(), () -> { + @Deprecated(since = "v2.0.7.7") + public static void handleShopSearchVisibilityAsync(Shop shop, boolean hideShop) { + VirtualThreadScheduler.runTaskAsync(() -> { Iterator<ShopSearchActivityModel> shopSearchActivityIterator = ShopSearchActivityStorageUtil.getGlobalShopsList().iterator(); int i = 0; while(shopSearchActivityIterator.hasNext()) { ShopSearchActivityModel shopSearchActivity = shopSearchActivityIterator.next(); Location shopLocation = shop.getLocation(); if(shopSearchActivity.compareWith( shopLocation.getWorld().getName(), shopLocation.getX(), shopLocation.getY(), shopLocation.getZ() )) { ShopSearchActivityStorageUtil.getGlobalShopsList().get(i).setHiddenFromSearch(hideShop); break; } i++; } }); }pom.xml (1)
67-83: Relocate shaded Jackson to avoid plugin classpath conflicts.Shading without relocation risks NoSuchMethodError/LinkageErrors when other plugins load different Jackson versions.
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-shade-plugin</artifactId> <version>3.2.4</version> <executions> <execution> <phase>package</phase> <goals> <goal>shade</goal> </goals> <configuration> <createDependencyReducedPom>false</createDependencyReducedPom> + <relocations> + <relocation> + <pattern>com.fasterxml.jackson</pattern> + <shadedPattern>io.myzticbean.shaded.jackson</shadedPattern> + </relocation> + </relocations> </configuration> </execution> </executions> </plugin>src/main/java/io/myzticbean/finditemaddon/listeners/PlayerJoinEventListener.java (1)
34-34: Reuse the shared UpdateChecker instance.Avoid allocating a new UpdateChecker per join; use the plugin’s instance if available.
Apply this diff:
- new UpdateChecker().notifyPlayerAboutUpdateOnJoin(event); + var checker = FindItemAddOn.getUpdateChecker(); + (checker != null ? checker : new UpdateChecker()).notifyPlayerAboutUpdateOnJoin(event);Also verify that notifyPlayerAboutUpdateOnJoin does not block the event thread (network I/O should be off-main-thread, e.g., via VirtualThreadScheduler).
src/main/java/io/myzticbean/finditemaddon/handlers/command/CmdExecutorHandler.java (3)
72-75: Simplify the buy/sell autocomplete checks.Use isBlank and contains for clarity (case-insensitivity isn’t relevant for a space).
Apply this diff:
- if(StringUtils.isEmpty(FindItemAddOn.getConfigProvider().FIND_ITEM_TO_BUY_AUTOCOMPLETE) - || StringUtils.containsIgnoreCase(FindItemAddOn.getConfigProvider().FIND_ITEM_TO_BUY_AUTOCOMPLETE, " ")) { + if (StringUtils.isBlank(FindItemAddOn.getConfigProvider().FIND_ITEM_TO_BUY_AUTOCOMPLETE) + || StringUtils.contains(FindItemAddOn.getConfigProvider().FIND_ITEM_TO_BUY_AUTOCOMPLETE, " ")) {
104-112: Use VirtualThreadScheduler consistently and drop commented code.
- Great switch to VirtualThreadScheduler here.
- Clean up the commented Bukkit async block.
- For consistency, also switch the “search all shops” path (Lines 85-89) to VirtualThreadScheduler.
Apply this diff to remove dead comments and standardize:
if(!FindItemAddOn.isQSReremakeInstalled() && FindItemAddOn.getQsApiInstance().isQSShopCacheImplemented()) { - VirtualThreadScheduler.runTaskAsync(() -> { + VirtualThreadScheduler.runTaskAsync(() -> { List<FoundShopItemModel> searchResultList = FindItemAddOn.getQsApiInstance().findItemBasedOnTypeFromAllShops(new ItemStack(mat), isBuying, player); this.openShopMenu(player, searchResultList, true, FindItemAddOn.getConfigProvider().NO_SHOP_FOUND_MSG); }); -// Bukkit.getScheduler().runTaskAsynchronously(FindItemAddOn.getInstance(), () -> { -// -// List<FoundShopItemModel> searchResultList = FindItemAddOn.getQsApiInstance().findItemBasedOnTypeFromAllShops(new ItemStack(mat), isBuying, player); -// this.openShopMenu(player, searchResultList, true, FindItemAddOn.getConfigProvider().NO_SHOP_FOUND_MSG); -// -// });And update the “all shops” branch:
- Bukkit.getScheduler().runTaskAsynchronously(FindItemAddOn.getInstance(), () -> { + VirtualThreadScheduler.runTaskAsync(() -> { List<FoundShopItemModel> searchResultList = FindItemAddOn.getQsApiInstance().fetchAllItemsFromAllShops(isBuying, player); this.openShopMenu(player, searchResultList, true, FindItemAddOn.getConfigProvider().NO_SHOP_FOUND_MSG); });
120-128: Same here: remove commented legacy block.Keep the file lean.
Apply this diff:
if(!FindItemAddOn.isQSReremakeInstalled() && FindItemAddOn.getQsApiInstance().isQSShopCacheImplemented()) { VirtualThreadScheduler.runTaskAsync(() -> { List<FoundShopItemModel> searchResultList = FindItemAddOn.getQsApiInstance().findItemBasedOnDisplayNameFromAllShops(itemArg, isBuying, player); this.openShopMenu(player, searchResultList, true, FindItemAddOn.getConfigProvider().FIND_ITEM_CMD_INVALID_MATERIAL_MSG); }); -// Bukkit.getScheduler().runTaskAsynchronously(FindItemAddOn.getInstance(), () -> { -// -// List<FoundShopItemModel> searchResultList = FindItemAddOn.getQsApiInstance().findItemBasedOnDisplayNameFromAllShops(itemArg, isBuying, player); -// this.openShopMenu(player, searchResultList, true, FindItemAddOn.getConfigProvider().FIND_ITEM_CMD_INVALID_MATERIAL_MSG); -// -// });src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java (2)
23-27: Add timeouts and a User-Agent header.Prevent hangs and comply with Modrinth API etiquette; include Accept header too.
Apply this diff:
- public ModrinthService() { - this.httpClient = HttpClient.newHttpClient(); + public ModrinthService() { + this.httpClient = HttpClient.newBuilder() + .connectTimeout(java.time.Duration.ofSeconds(5)) + .build(); this.objectMapper = new ObjectMapper(); this.objectMapper.registerModule(new JavaTimeModule()); } @@ - HttpRequest request = HttpRequest.newBuilder() + HttpRequest request = HttpRequest.newBuilder() .uri(URI.create(url)) + .timeout(java.time.Duration.ofSeconds(10)) + .header("User-Agent", "QSFindItemAddOn (+https://github.com/myzticbean/QSFindItemAddOn)") + .header("Accept", "application/json") .build(); - try { + try { Logger.logDebugInfo("Outbound Request: " + url); HttpResponse<String> response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());Also applies to: 31-33, 35-37
41-43: Log details once, no TODO-style comments.Replace inline comments with consistent Logger usage. Optional clean-up.
src/main/java/io/myzticbean/finditemaddon/FindItemAddOn.java (2)
79-82: Static instance set in constructor.Works, though many plugins set it in onLoad/onEnable to make lifecycle intent explicit. Optional.
91-92: Naming: this is a project ID, not a slug.Value “asp13ugE” looks like a Modrinth project ID; consider renaming to MODRINTH_PROJECT_ID and getModrinthProjectId() to avoid confusion.
Also applies to: 299-302
src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java (5)
42-43: Consider removing unused SPIGOT_DOWNLOAD_LINK constant.The SPIGOT_DOWNLOAD_LINK constant is no longer used since the migration to Modrinth. Consider removing it to avoid confusion.
-private static final String SPIGOT_DOWNLOAD_LINK = "https://www.spigotmc.org/resources/" + FindItemAddOn.getPluginID() + "/"; private static final String MODRINTH_DOWNLOAD_LINK = "https://modrinth.com/plugin/shop-search/versions/";
114-115: Remove commented-out legacy code.The commented-out line referencing the old Spigot download link should be removed.
-// player.sendMessage(ColorTranslator.translateColorCodes(prefix + ";b300Download here: &#a3a3c2&n" + SPIGOT_DOWNLOAD_LINK)); player.sendMessage(ColorTranslator.translateColorCodes(prefix + ";b300Download here: &#a3a3c2&n" + MODRINTH_DOWNLOAD_LINK));
70-83: Document deprecation reason and migration path.The deprecated method lacks information about why it's deprecated and what should be used instead.
-@Deprecated(since = "v2.0.7.7") +/** + * @deprecated since v2.0.7.7, use {@link #isUpdateAvailable(Consumer)} instead. + * This method uses the legacy Spigot API which is being phased out in favor of Modrinth. + */ +@Deprecated(since = "v2.0.7.7") public void getLatestVersion(Consumer<String> consumer) {
52-68: Consider version comparison logic for different formats.The current implementation uses simple string equality for version comparison, which may fail for versions like "2.0.7.7-SNAPSHOT" vs "2.0.7.7" or semantic versioning differences.
Consider implementing proper semantic version comparison. Would you like me to generate a utility method that handles different version formats correctly?
53-67: Add timeout and cancellation to async update check
Capture theFuturereturned byrunTaskAsync, enforce a timeout (e.g. viafuture.get(5, TimeUnit.SECONDS)orCompletableFuture.orTimeout), cancel on timeout or during shutdown, and log accordingly. For example:public void isUpdateAvailable(Consumer<Boolean> updateAvailabilityConsumer) { - VirtualThreadScheduler.runTaskAsync(() -> { + var future = VirtualThreadScheduler.runTaskAsync(() -> { // existing update-check logic... }); + // enforce timeout and cancel if unresponsive + VirtualThreadScheduler.runTaskAsync(() -> { + try { + future.get(5, TimeUnit.SECONDS); + } catch (TimeoutException | InterruptedException e) { + future.cancel(true); + Logger.logWarning("Update check timed out"); + } + }); }Also store and cancel any pending
FutureinonDisable()to prevent hanging tasks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
CHANGELOG.md(1 hunks)README.md(1 hunks)pom.xml(6 hunks)src/main/java/io/myzticbean/finditemaddon/FindItemAddOn.java(6 hunks)src/main/java/io/myzticbean/finditemaddon/commands/quickshop/subcommands/FindItemCmdHikariImpl.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/commands/quickshop/subcommands/FindItemCmdReremakeImpl.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/dependencies/PlayerWarpsPlugin.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/handlers/command/CmdExecutorHandler.java(10 hunks)src/main/java/io/myzticbean/finditemaddon/handlers/gui/menus/FoundShopsMenu.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/listeners/PlayerJoinEventListener.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/models/enums/CustomCmdPlaceholdersEnum.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/models/enums/NearestWarpModeEnum.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/models/enums/PlayerPermsEnum.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/models/enums/ShopLorePlaceholdersEnum.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/quickshop/impl/QSHikariAPIHandler.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/quickshop/impl/QSReremakeAPIHandler.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/scheduledtasks/Task15MinInterval.java(2 hunks)src/main/java/io/myzticbean/finditemaddon/utils/LocationUtils.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java(4 hunks)src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/model/ProjectDetailsResponse.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java(1 hunks)src/main/java/io/myzticbean/finditemaddon/utils/json/HiddenShopStorageUtil.java(3 hunks)src/main/java/io/myzticbean/finditemaddon/utils/json/ShopSearchActivityStorageUtil.java(3 hunks)src/main/java/io/myzticbean/finditemaddon/utils/warp/WarpUtils.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/main/java/io/myzticbean/finditemaddon/utils/json/ShopSearchActivityStorageUtil.java (1)
src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
VirtualThreadScheduler(9-46)
src/main/java/io/myzticbean/finditemaddon/utils/json/HiddenShopStorageUtil.java (1)
src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
VirtualThreadScheduler(9-46)
src/main/java/io/myzticbean/finditemaddon/scheduledtasks/Task15MinInterval.java (1)
src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
VirtualThreadScheduler(9-46)
src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java (2)
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java (1)
ModrinthService(17-51)src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
VirtualThreadScheduler(9-46)
src/main/java/io/myzticbean/finditemaddon/utils/warp/WarpUtils.java (2)
src/main/java/io/myzticbean/finditemaddon/dependencies/PlayerWarpsPlugin.java (1)
UtilityClass(36-135)src/main/java/io/myzticbean/finditemaddon/dependencies/EssentialsXPlugin.java (1)
UtilityClass(37-103)
src/main/java/io/myzticbean/finditemaddon/handlers/command/CmdExecutorHandler.java (1)
src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
VirtualThreadScheduler(9-46)
src/main/java/io/myzticbean/finditemaddon/FindItemAddOn.java (1)
src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
VirtualThreadScheduler(9-46)
🪛 markdownlint-cli2 (0.18.1)
README.md
2-2: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (21)
src/main/java/io/myzticbean/finditemaddon/models/enums/CustomCmdPlaceholdersEnum.java (1)
19-19: Enum package move LGTM.Package relocation looks correct; no logic changes.
src/main/java/io/myzticbean/finditemaddon/scheduledtasks/Task15MinInterval.java (1)
35-38: LGTM: migrated periodic job to virtual threads.Change is minimal and consistent with the new scheduler.
src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
9-16: onDisable invokes shutdown and no post-shutdown tasks detected
Executor is closed in onDisable and there are no runTaskAsync calls after shutdown.src/main/java/io/myzticbean/finditemaddon/commands/quickshop/subcommands/FindItemCmdReremakeImpl.java (1)
23-23: Import path update to models.enums — looks goodMatches the enum relocation; no functional impact.
src/main/java/io/myzticbean/finditemaddon/commands/quickshop/subcommands/FindItemCmdHikariImpl.java (1)
24-24: Import path update to models.enums — looks goodConsistent with the project-wide enum move.
src/main/java/io/myzticbean/finditemaddon/quickshop/impl/QSHikariAPIHandler.java (1)
37-37: Import path update to models.enums — LGTMReferences (e.g., FINDITEM_USE) remain valid.
src/main/java/io/myzticbean/finditemaddon/models/enums/NearestWarpModeEnum.java (1)
19-19: Package moved to models.enums — LGTMEnum content unchanged; callers updated elsewhere.
src/main/java/io/myzticbean/finditemaddon/dependencies/PlayerWarpsPlugin.java (1)
24-24: Import path switched to models.enums — LGTMKeeps the configuration check intact.
src/main/java/io/myzticbean/finditemaddon/models/enums/PlayerPermsEnum.java (1)
19-19: Enum package relocation to models.enums — LGTMPublic API unchanged; callers now import from models.enums.
Please confirm no remaining references to io.myzticbean.finditemaddon.utils.enums.PlayerPermsEnum exist (run the repository-wide script shared in QSReremakeAPIHandler comment).
src/main/java/io/myzticbean/finditemaddon/handlers/gui/menus/FoundShopsMenu.java (1)
32-35: Updated enum imports to models.enums — LGTMAll three enums now sourced from models.enums; usage remains the same.
src/main/java/io/myzticbean/finditemaddon/quickshop/impl/QSReremakeAPIHandler.java (1)
27-27: Approve enum import update No remaining imports fromutils.enums; all enums, includingPlayerPermsEnum, now consistently undermodels.enums.src/main/java/io/myzticbean/finditemaddon/utils/LocationUtils.java (1)
22-22: Import path update looks correct.The enum’s new location under models.enums is consistent with the project-wide move.
CHANGELOG.md (1)
1-9: Changelog entry reads well.Clear, actionable bullets for 2.0.7.7. Nothing to fix here.
src/main/java/io/myzticbean/finditemaddon/models/enums/ShopLorePlaceholdersEnum.java (1)
19-19: Package relocation LGTM.No behavioral change; downstream imports should compile as updated elsewhere in the PR.
src/main/java/io/myzticbean/finditemaddon/handlers/command/CmdExecutorHandler.java (4)
25-27: Imports align with new enum location and scheduler usage.Good move toward the shared VirtualThreadScheduler abstraction.
49-50: Centralized NO_PERMISSION string is good.Removes duplication and eases future changes.
63-64: Consistent use of NO_PERMISSION message.Message rendering path is clean and consistent.
175-175: Consistent permission message usage.Good consolidation across hide/reveal/reload flows.
Also applies to: 207-207, 251-251, 293-293
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/model/ProjectDetailsResponse.java (1)
11-83: Model looks compatible with Modrinth v2 version schema.Annotations and nested classes are appropriate. Consider long for downloads if you expect very large counts, but int is likely fine.
If you want, I can validate the field names against Modrinth’s latest API docs.
src/main/java/io/myzticbean/finditemaddon/FindItemAddOn.java (2)
176-176: VirtualThreadScheduler shutdown on disable is good.Prevents thread leaks.
228-252: Asynchronous update check callback usage LGTM.Minimal logging; ensure UpdateChecker runs network I/O off the main thread.
| <jackson.version>2.13.5</jackson.version> | ||
| </properties> |
There was a problem hiding this comment.
Upgrade Jackson to the latest 2.18.x and align via BOM.
2.13.5 is several years old and misses fixes; prefer 2.18.4 for security and compatibility.
- <jackson.version>2.13.5</jackson.version>
+ <jackson.version>2.18.4</jackson.version>Also applies to: 43-53, 173-186
🤖 Prompt for AI Agents
In pom.xml around lines 23-24 (and also apply changes at 43-53 and 173-186), the
project pins jackson.version to 2.13.5; update to the latest 2.18.x (recommended
2.18.4) and align all Jackson dependencies using the official Jackson BOM
(com.fasterxml.jackson:jackson-bom:2.18.4) in the dependencyManagement section
so all Jackson artifacts inherit the same version; remove or override individual
jackson.version properties as needed, import the BOM with scope import/type pom,
and ensure any direct Jackson dependencies no longer specify versions so the BOM
controls them.
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java
Outdated
Show resolved
Hide resolved
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java (1)
37-42: Handle 429 rate limiting and avoid logging full payloads.Better diagnostics and safer logging in production.
Apply this diff:
- Logger.logDebugInfo("Inbound Response: " + response.body()); - if (response.statusCode() == 200) { + String responseBody = response.body(); + String preview = responseBody != null && responseBody.length() > 2048 + ? responseBody.substring(0, 2048) + "...(truncated)" + : responseBody; + Logger.logDebugInfo("Inbound Response (" + response.statusCode() + "): " + preview); + if (response.statusCode() == 200) { return objectMapper.readValue(response.body(), new TypeReference<List<ProjectDetailsResponse>>() {}); - } else { + } else if (response.statusCode() == 429) { + var retryAfter = response.headers().firstValue("Retry-After").orElse("unknown"); + Logger.logWarning("Modrinth API rate-limited the request. Retry-After: " + retryAfter); + } else { Logger.logError("Modrinth API request failed with status code: " + response.statusCode()); }src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java (3)
75-75: Locale-safe lowercase check for “snapshot”.Avoid locale-dependent case conversions.
- if(latestVersion.toLowerCase().contains("snapshot")) { + if (latestVersion.toLowerCase(java.util.Locale.ROOT).contains("snapshot")) {
70-73: Normalize versions before equality to reduce false mismatches.Many ecosystems prefix “v” or vary case. Normalize both sides.
- var latestVersion = latestVersionDetails.getVersionNumber(); - var currentVersion = FindItemAddOn.getInstance().getDescription().getVersion(); - boolean isUpToDate = currentVersion.equals(latestVersion); + var latestVersion = latestVersionDetails.getVersionNumber(); + var currentVersion = FindItemAddOn.getInstance().getDescription().getVersion(); + var normalizedCurrent = currentVersion.replaceFirst("^[vV]", ""); + var normalizedLatest = latestVersion.replaceFirst("^[vV]", ""); + boolean isUpToDate = normalizedCurrent.equals(normalizedLatest);
47-50: Ensure virtual-thread executor is shut down on plugin disable.Confirm that FindItemAddOn’s shutdown hook calls VirtualThreadScheduler.shutdown() to avoid leaked threads.
Also applies to: 100-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java(4 hunks)src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java (2)
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java (1)
ModrinthService(17-51)src/main/java/io/myzticbean/finditemaddon/utils/async/VirtualThreadScheduler.java (1)
VirtualThreadScheduler(9-46)
🔇 Additional comments (4)
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java (3)
39-39: Explicit TypeReference fix looks good.Captures generics correctly for Jackson.
43-48: Correct separation of InterruptedException and IOException.Properly preserves interrupt status only on interrupts.
30-33: Verify slug safety or encode the path segment.If projectId is not guaranteed to match Modrinth’s slug charset, consider encoding/sanitizing to avoid malformed URIs. If it’s a trusted, fixed slug, this is fine.
Would you confirm that FindItemAddOn.getModrinthProjectSlug() always returns a safe slug (letters, numbers, dashes/underscores) as required by Modrinth? If not, we should encode the path segment.
src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java (1)
64-65: JDK baseline check: List.getFirst() requires Java 21.If your plugin targets older Java, use get(0) instead. If you’re on Java 21 (virtual threads suggest so), this is fine.
Optional fallback:
- var latestVersionDetails = projectVersions.getFirst(); + var latestVersionDetails = projectVersions.get(0);
| public ModrinthService() { | ||
| this.httpClient = HttpClient.newHttpClient(); | ||
| this.objectMapper = new ObjectMapper(); | ||
| this.objectMapper.registerModule(new JavaTimeModule()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add HTTP timeouts, a User-Agent, and relax unknown-property failures.
Prevents indefinite blocking on network calls, improves API etiquette, and makes JSON parsing resilient to schema changes.
Apply this diff:
- this.httpClient = HttpClient.newHttpClient();
- this.objectMapper = new ObjectMapper();
- this.objectMapper.registerModule(new JavaTimeModule());
+ this.httpClient = HttpClient.newBuilder()
+ .connectTimeout(Duration.ofSeconds(5))
+ .build();
+ this.objectMapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule());
+ this.objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(url))
+ .timeout(Duration.ofSeconds(10))
+ .header("Accept", "application/json")
+ .header("User-Agent", "QSFindItemAddOn/2.0.7.7 (+https://modrinth.com/plugin/shop-search)")
.build();Add imports:
import java.time.Duration;
import com.fasterxml.jackson.databind.DeserializationFeature;Also applies to: 31-33
🤖 Prompt for AI Agents
In
src/main/java/io/myzticbean/finditemaddon/utils/api/modrinth/ModrinthService.java
around lines 23-27 and 31-33, the HttpClient and ObjectMapper need
configuration: import java.time.Duration and
com.fasterxml.jackson.databind.DeserializationFeature, build the HttpClient with
connect and overall timeouts and a default User-Agent header, and configure the
ObjectMapper to disable FAIL_ON_UNKNOWN_PROPERTIES and keep the JavaTimeModule
registration; update the constructor to create
HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(...)).build() (and set
per-request timeout headers or default headers for User-Agent) and call
objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
before registering modules.
| } else { | ||
| Logger.logWarning("Plugin has a new update available! (Version: " + latestVersion + ")"); | ||
| } | ||
| Logger.logWarning("Download here: https://modrinth.com/plugin/shop-search/version/" + latestVersion); |
There was a problem hiding this comment.
Fix broken Modrinth link (uses version number instead of version ID).
The website route /version/{...} expects a version ID, not version_number. Current link will likely 404. Link to the versions page instead (or use the ID).
Minimal fix (use the versions list link constant):
- Logger.logWarning("Download here: https://modrinth.com/plugin/shop-search/version/" + latestVersion);
+ Logger.logWarning("Download here: " + MODRINTH_DOWNLOAD_LINK);Alternative (if ProjectDetailsResponse exposes getId()):
- var latestVersion = latestVersionDetails.getVersionNumber();
+ var latestVersion = latestVersionDetails.getVersionNumber();
+ var versionId = latestVersionDetails.getId();
...
- Logger.logWarning("Download here: https://modrinth.com/plugin/shop-search/version/" + latestVersion);
+ Logger.logWarning("Download here: https://modrinth.com/plugin/shop-search/version/" + versionId);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/io/myzticbean/finditemaddon/utils/UpdateChecker.java around
line 80, the Modrinth link currently uses the version number which the
/version/{id} route does not accept; replace the URL to point to the project's
versions list (use the existing VERSIONS_LIST_LINK constant) or, if the
ProjectDetailsResponse (or the version response you have) exposes getId(),
construct the /version/{id} URL with that ID instead; update the
Logger.logWarning call to use the correct constant or ID-based URL so the link
does not 404.
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation