Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Release/version bump to 1.23.0, updating addon metadata and refining UI/logging/test behavior to support recent feature work (action bar color support, Nexo integration, locale updates, and code quality improvements).
Changes:
- Bump project version to 1.23.0 and update addon API metadata.
- Refactor
PhasesPanelandOneBlocksManager#getProbs()to reduce complexity and improve maintainability. - Update tests to align with Adventure
Titleusage and add additional safety assertions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/world/bentobox/aoneblock/TestWorldSettings.java | Add explicit no-op comment in stubbed method. |
| src/test/java/world/bentobox/aoneblock/listeners/StartSafetyListenerTest.java | Add explicit no-op comments in stubbed methods. |
| src/test/java/world/bentobox/aoneblock/listeners/CheckPhaseTest.java | Update assertions to verify Adventure Title usage. |
| src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java | Tighten tests with assertDoesNotThrow; minor visibility tweak. |
| src/main/resources/addon.yml | Update declared BentoBox api-version. |
| src/main/java/world/bentobox/aoneblock/panels/PhasesPanel.java | Refactor phase button creation into helper methods; simplify formatting logic. |
| src/main/java/world/bentobox/aoneblock/oneblocks/OneBlocksManager.java | Refactor probability logging into smaller helper methods. |
| src/main/java/world/bentobox/aoneblock/listeners/CheckPhase.java | Switch to Adventure titles and component-based requirement messages. |
| pom.xml | Bump build version to 1.23.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/world/bentobox/aoneblock/listeners/CheckPhase.java
Outdated
Show resolved
Hide resolved
|
@tastybento I've opened a new pull request, #485, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #486, to work on those changes. Once the pull request is ready, I'll request review from you. |
…custom translate() with LegacyComponentSerializer Agent-Logs-Url: https://github.com/BentoBoxWorld/AOneBlock/sessions/3ae91a34-99b8-44ae-bdce-bc776883a2f9 Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
…port Agent-Logs-Url: https://github.com/BentoBoxWorld/AOneBlock/sessions/8ff2ce9e-417f-45e9-92d3-d6649871f7c7 Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
Bump BentoBox dependency to 3.13.0 for MiniMessage support
… LegacyComponentSerializer) Agent-Logs-Url: https://github.com/BentoBoxWorld/AOneBlock/sessions/e8854ea2-7b56-497a-ac76-533846ce7957 Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
…t.sendMessage() Agent-Logs-Url: https://github.com/BentoBoxWorld/AOneBlock/sessions/b792d2e9-3944-46fa-9f8e-2339f94967e8 Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove custom translate() helper from CheckPhase — let BentoBox handle MiniMessage natively
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "[starting-block]", biomeText, | ||
| BIOME, blockText, |
There was a problem hiding this comment.
In the default description branch, the placeholder replacements for [starting-block] and [biome] look swapped: "[starting-block]" is being replaced with biomeText and [biome] is being replaced with blockText. This will render the biome line where the starting-block line should be (and vice versa) in the phases GUI. Swap these arguments so [starting-block] maps to blockText and [biome] maps to biomeText (or to the translated biome line, depending on intended formatting).
| "[starting-block]", biomeText, | |
| BIOME, blockText, | |
| "[starting-block]", blockText, | |
| BIOME, biomeText, |
| <mockito.version>5.11.0</mockito.version> | ||
| <mock-bukkit.version>v1.21-SNAPSHOT</mock-bukkit.version> | ||
| <!-- More visible way how to change dependency versions --> | ||
| <bentobox.version>3.10.0</bentobox.version> | ||
| <bentobox.version>3.13.0</bentobox.version> | ||
| <items-adder.version>4.0.10</items-adder.version> | ||
| <nexo.version>1.8.0</nexo.version> |
There was a problem hiding this comment.
PR description says "BentoBox API upgraded to 3.10.0", but this change bumps the build dependency to 3.13.0. Please update the PR description (or adjust the version here) so the stated API upgrade matches what is actually being shipped.
| // Verify phase name change | ||
| assertEquals("Next Phase", is.getPhaseName()); | ||
| // Verify title shown | ||
| verify(mockPlayer).sendTitle("Next Phase", null, -1, -1, -1); | ||
| verify(mockPlayer).showTitle(any(Title.class)); | ||
|
|
There was a problem hiding this comment.
This verification only checks that some Title was shown, not that it contains the expected phase name. Consider capturing the Title argument and asserting the title component renders to "Next Phase" (and that subtitle/timings are as expected) so the test guards the behavior of setNewPhase() more effectively.
| // Verify phase name change | ||
| assertEquals("Next Phase", is.getPhaseName()); | ||
| // Verify title shown | ||
| verify(mockPlayer).sendTitle("Next Phase", null, -1, -1, -1); | ||
| verify(mockPlayer).showTitle(any(Title.class)); | ||
|
|
There was a problem hiding this comment.
Here the test verifies showTitle(any(Title.class)) but does not assert that the Title contains the expected phase name. Capturing the Title and checking its contents would prevent regressions where the wrong text is shown (or where the call is made with an empty title).
| // Verify phase name change | ||
| assertEquals("Next Phase", is.getPhaseName()); | ||
| // Verify title shown | ||
| verify(mockPlayer).sendTitle("Next Phase", null, -1, -1, -1); | ||
| verify(mockPlayer).showTitle(any(Title.class)); | ||
|
|
There was a problem hiding this comment.
This assertion only checks that showTitle(...) was invoked, but not what was shown. To ensure setNewPhase() displays the correct phase title when player is null (owner fallback), capture the Title argument and assert its title text matches "Next Phase".
| // Verify phase name change | ||
| assertEquals("Next Phase", is.getPhaseName()); | ||
| // Verify title shown | ||
| verify(mockPlayer).sendTitle("Next Phase", null, -1, -1, -1); | ||
| verify(mockPlayer).showTitle(any(Title.class)); | ||
|
|
There was a problem hiding this comment.
Same as the other setNewPhase tests: showTitle(any(Title.class)) doesn't verify the shown title content. Capturing and asserting the Title argument (e.g., that it renders the new phase name) will make this test actually validate the behavior.




Summary
Changes since 1.22.0
Test plan
mvn test)🤖 Generated with Claude Code