fix(13-08): ship a default sidebar that renders without extra plugins - #15
Conversation
… can render Adds a Default Sidebar Renderability nested test class asserting that every placeholder token found in the shipped default lines list is either resolved by this module itself (currently none are — the module's own SideBarService.parsePlaceholders() does nothing but delegate to PlaceholderAPI or return the text unchanged) or is a real, documented PlaceholderAPI placeholder requiring the external provider. The assertion enumerates every %token% found in the defaults rather than grepping for one known-bad string, so a sibling token added later is caught too. A second test proves the fix touches shipped defaults only — an operator-configured lines list (including one still carrying the broken token) passes through unchanged. RED, observed: defaultLinesContainNoTokenThatNothingResolves fails naming "world_name" as the unresolvable token. %world_name% is not a real PlaceholderAPI placeholder syntax at all — PlaceholderAPI's "World" expansion's actual world-name placeholder is %world_name_<world>% (an explicit world argument is mandatory), and the bundled Player expansion's placeholder for "the world the current player is in" is %player_world%. A bare %world_name% resolves under neither, so it stays literal on screen regardless of whether PlaceholderAPI is installed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
Replaces the shipped default sidebar's %world_name% token with %player_world%. %world_name% is not valid PlaceholderAPI syntax at all: the "World" expansion's real world-name placeholder is %world_name_<world>% (an explicit world argument is mandatory), and the bundled Player expansion's placeholder for "the world the current player is in" is %player_world%. The old token therefore stayed literal on every player's screen regardless of whether PlaceholderAPI was installed -- it was never a case of "needs an external plugin", it was simply not a placeholder anything recognizes. Every other token already shipped in the defaults (player_name, server_online, server_max_players, vault_eco_balance_formatted, player_ping, server_time_hh:mm:ss) is real, documented PlaceholderAPI syntax that legitimately requires the external provider to resolve -- left unchanged, per the "resolved by the module or documented as requiring the provider" rule; this module's own SideBarService.parsePlaceholders() resolves nothing itself. Closes #13 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3205b685e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| private List<String> extractTokens(List<String> lines) { | ||
| Pattern tokenPattern = Pattern.compile("%([a-zA-Z0-9_:]+)%"); |
There was a problem hiding this comment.
If a future default contains a malformed placeholder such as %player-world% or %foo.bar%, this restrictive regex ignores it entirely; the existing valid placeholders still satisfy tokens.isNotEmpty(), so the regression test passes while the new token is rendered literally in production. Extract every percent-delimited candidate first (for example, anything between paired % characters), then use the allow-list to reject unsupported syntax.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verdict: real finding, already fixed on this branch.
Verified by reading the code (not inferred): the narrow regex you flagged, %([a-zA-Z0-9_:]+)% at SideBarConfigTest.java:182 on commit 3205b68, would indeed have let a malformed token like %player-world% or %foo.bar% through unextracted while the existing valid placeholders still satisfied tokens.isNotEmpty().
Independently, our own Phase 13 code review flagged the same test method for a related reason (WR-01/IN-02 in 13-REVIEW-UltiSideBar.md: the test checked token names against a hand-authored allow-list mirroring the file's own defaults, rather than the module's actual rendered output). Commit 03f4139 on this branch rewrote defaultLinesContainNoTokenThatNothingResolves to route every default line through SideBarService's real parsePlaceholders() / PlaceholderAPI seam (stubbed to behave like a real installation), and in the same rewrite widened the leftover-token pattern to Pattern.compile("%[^%]+%") — any run of non-% characters between two %, including hyphens and dots. A malformed future token the resolution stub doesn't recognize is left literal in the rendered output and is now caught by that broader pattern. Current line: SideBarConfigTest.java:223.
No further action needed on this thread — thank you for the catch.
…out migration, and drive real init()/placeholder resolution CR-01: AbstractConfigEntity.init() only fills missing keys and never overwrites a persisted "lines" list, so every server that already ran this plugin keeps the old %world_name% default forever. Adds a RED test (SideBarConfig.migrateLegacyWorldNameDefaultLine() does not exist yet) that starts from a persisted sidebar.yml carrying the old default line plus an operator's custom line, and asserts only the stale line is rewritten -- both in memory and on disk -- while the custom line survives untouched, plus a no-op case once already migrated and a case proving a line that only mentions the old token inside other text is left alone. WR-01: rewrites defaultLinesContainNoTokenThatNothingResolves to route every default line through SideBarService's real parsePlaceholders()/PlaceholderAPI seam (stubbed to behave like a real installation: known placeholders substituted, unknown ones left literal) instead of checking token names against a hand-authored allow-list mirroring this same file's own defaults -- closing the self-consistent-but-wrong gap where a broken token and its allow-list entry could land in the same commit. This also widens the leftover-token regex to %[^%]+% (was [a-zA-Z0-9_:]+), incidentally resolving IN-01's narrow-token-shape concern. WR-02: rewrites anOperatorConfiguredLineIsUnaffected to drive a real init() against a temp sidebar.yml instead of round-tripping a bare Lombok setter/getter, which could not fail for any change to SideBarConfig's default-handling behavior. Adds mockPluginBackedBy(), a Mockito default-Answer double, because UltiToolsPlugin's getConfigFolder()/getConfigFile() are protected final and declared outside this test's package -- a normal when(mock.getConfigFolder()) does not compile from here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…es actually reach it CR-01: AbstractConfigEntity.init() only fills a @ConfigEntry key that is MISSING from the persisted file and never overwrites one that already exists, so the earlier %world_name% -> %player_world% default fix (3205b68) never reached any server that had already run this plugin -- exactly the population issue #13 was filed against. Adds SideBarConfig.migrateLegacyWorldNameDefaultLine(), a targeted, exact-match, one-time migration: it rewrites a persisted "lines" entry only when it is byte-identical to the OLD shipped default, leaving any operator customisation -- including a line that merely mentions %world_name% inside other text -- untouched. Wired into SideBarService.init() (called from UltiSideBar.registerSelf() and, via reload(), from reloadSelf()), which is the earliest module-owned hook that runs after AbstractConfigEntity.init() has already populated the config from disk; init() itself is final and cannot be overridden, and a ConfigChangeListener registered here would miss the very first load. Idempotent, so running again on every reload is harmless. Also applies IN-01: %server_time_hh:mm:ss% used a 12-hour pattern with no AM/PM marker; changed to %server_time_HH:mm:ss% (24-hour). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed30860d15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .replace("%player_ping%", "42"); | ||
| // PlaceholderAPI's Server expansion accepts an arbitrary SimpleDateFormat pattern as | ||
| // a dynamic suffix: %server_time_<SimpleDateFormat>%. | ||
| return resolved.replaceAll("%server_time_[^%]+%", "12:00:00"); |
There was a problem hiding this comment.
When a future default contains an invalid SimpleDateFormat suffix such as %server_time_foo%, this regex still replaces it because it accepts every nonempty suffix, even though the real Server expansion cannot format illegal pattern letters. The renderability test would therefore pass while the shipped placeholder fails at runtime. The fresh evidence beyond the earlier token-extraction issue is this newly added permissive substitution; validate the suffix as a real date pattern or substitute only known-valid defaults.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real, fixed in a378d4c.
Measured: new java.text.SimpleDateFormat("foo") throws IllegalArgumentException: Illegal pattern character 'f' — the regex %server_time_[^%]+% -> "12:00:00" accepted any nonempty suffix regardless of whether a real SimpleDateFormat could parse it, so this test would have passed a shipped %server_time_foo% default even though PlaceholderAPI's real Server expansion cannot format it.
stubResolve() now attempts new SimpleDateFormat(suffix).format(new Date()) per %server_time_...% match and only substitutes "12:00:00" when that succeeds; an illegal suffix is left as the original token, mirroring what a real installation would leave unresolved. Added stubResolveRejectsAnInvalidServerTimePattern() to pin both the reject path (%server_time_foo% stays literal) and the accept path (the current shipped %server_time_HH:mm:ss% still resolves).
mvn -B verify: BUILD SUCCESS, 122 tests (3 in DefaultSidebarRenderabilityTests, was 2), coverage checks met.
… stub The stub's %server_time_<suffix>% substitution accepted any nonempty suffix, even one no real SimpleDateFormat pattern can parse (e.g. "foo", since 'f' and 'o' are not SimpleDateFormat pattern letters and the constructor throws IllegalArgumentException). A shipped default with such a suffix would fail at runtime against a real PlaceholderAPI installation while defaultLinesContainNoTokenThatNothingResolves() still passed. The stub now attempts to construct SimpleDateFormat(suffix) and only substitutes a resolved value when that succeeds, leaving an invalid suffix as an unresolved token exactly like the real Server expansion would. A regression test pins both the reject and accept paths. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a378d4cc03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "", | ||
| "&7服务器时间", | ||
| "&f%server_time_hh:mm:ss%", | ||
| "&f%server_time_HH:mm:ss%", |
There was a problem hiding this comment.
On upgrades where sidebar.yml already contains the old shipped &f%server_time_hh:mm:ss% entry, AbstractConfigEntity.init() preserves the persisted lines list, while the new migration rewrites only the world-name entry. Consequently, this HH correction reaches fresh installations only, and existing servers continue displaying an ambiguous 12-hour time without an AM/PM marker; include the byte-identical legacy time entry in the targeted migration as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real finding, fixed in commits 1694c8f (RED test) and e6792ed (fix).
Confirmed by reading git log -p on this file: the pre-6.3-equivalent shipped default was the
byte-identical line "&f%server_time_hh:mm:ss%", and AbstractConfigEntity.init() never
overwrites an existing lines value on disk -- exactly the same persistence gap migrateLegacyWorldNameDefaultLine()
was written for, just not extended to this second entry.
Two new RED tests proved the defect before the fix: a persisted list containing only the legacy
time line was not rewritten, and a real-upgrade scenario (both legacy lines + an operator's
custom line) left the time line stale while migrating the world-name line and preserving the
custom line correctly. Both failed against the unmodified method.
Fix: renamed migrateLegacyWorldNameDefaultLine() to migrateLegacyDefaultLines() and
generalised it to an exact-match lookup table (LEGACY_LINE_REPLACEMENTS) mapping every tracked
legacy default (the %world_name% world line and the hh:mm:ss time line) to its corrected
replacement, so a future shipped-default correction only needs a map entry, not a new loop.
SideBarService.init()'s call site and comment updated to match. Operator-customised lines,
including a line that only mentions a legacy token inside other text, are still left untouched --
covered by the pre-existing exact-match test.
mvn -B verify: Tests run: 124, Failures: 0, Errors: 0, Skipped: 0; BUILD SUCCESS; jacoco "All
coverage checks have been met."
…upgrade PR #15 round-3 review (thread 3944542674): migrateLegacyWorldNameDefaultLine() only rewrites the %world_name% entry. AbstractConfigEntity.init() preserves the whole persisted "lines" list, so a server whose sidebar.yml still holds the byte-identical old shipped "&f%server_time_hh:mm:ss%" line keeps the ambiguous 12-hour time with no AM/PM marker forever, even though the shipped default was corrected to "&f%server_time_HH:mm:ss%". Two new RED tests under SideBarConfigTest$LegacyWorldNameLineMigration: - rewritesLegacyServerTimeLine: a persisted list containing only the legacy time line is not rewritten (asserts true, gets false). - rewritesBothLegacyDefaultsOnRealUpgrade: a real upgrade scenario with both legacy entries plus an operator's custom line -- the time line survives unmigrated while the world-name line and custom line behave correctly. Both fail against current behavior, confirming the defect the reviewer described. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
PR #15 round-3 review (thread 3944542674): the HH:mm:ss correction reached fresh installs only. AbstractConfigEntity.init() preserves a persisted "lines" list wholesale, and migrateLegacyWorldNameDefaultLine() rewrote only the byte-identical %world_name% entry, so an upgrading server kept the old shipped "&f%server_time_hh:mm:ss%" line -- an ambiguous 12-hour time with no AM/PM marker -- forever. Renamed the method to migrateLegacyDefaultLines() and generalised it to an exact-match lookup table (LEGACY_LINE_REPLACEMENTS) covering both tracked legacy defaults: the %world_name% world line and the hh:mm:ss server-time line. A future shipped-default correction extends the map, not the loop. Updated SideBarService.init()'s call site and comment to match. Verified: mvn -B verify -- Tests run: 124, Failures: 0, Errors: 0, Skipped: 0; BUILD SUCCESS; jacoco "All coverage checks have been met." Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
1 participant
Summary
%world_name%token with%player_world%.%world_name%(bare, no world argument) is not valid PlaceholderAPI syntax under anyexpansion — the "World" expansion's real placeholder is
%world_name_<world>%(an explicitworld argument is mandatory), and the bundled Player expansion's placeholder for "the world the
current player is in" is
%player_world%. The old token stayed literal on every player's screenregardless of whether PlaceholderAPI was installed.
player_name,server_online,server_max_players,vault_eco_balance_formatted,player_ping,server_time_hh:mm:ss) is real, documentedPlaceholderAPI syntax that legitimately requires the external provider — left unchanged.
reported string) against an allow-list of real PlaceholderAPI placeholders, so a sibling broken
token added later is caught too.
Issue closure
Closes #13
Issue #9 (raw i18n keys) is not closed by this pull request — it was already closed by
evidence in framework Phase 13 plan 13-03, against the fixed framework's
UltiToolsPluginlanguage resolver (framework issue #412). This module required no code change for #9; see the
## Translation verdictsection of13-LEDGER-UltiSideBar.md(phase evidence tree) for the fullinherited verdict and the residue check performed before this pull request's own fix was written.
Test evidence (falsification, both directions)
RED (before the fix):
SideBarConfigTest$DefaultSidebarRenderabilityTests.defaultLinesContainNoTokenThatNothingResolvesfails, naming
world_nameas the unresolvable token — 17 tests, 1 failure,BUILD FAILURE.GREEN (after the fix): 17 tests, 0 failures,
BUILD SUCCESS.Full module gate:
mvn -B verify— 118 tests, 0 failures, 0 errors, coverage checks met,BUILD SUCCESS. Nomockbukkitdependency was introduced (this module has no test-time serverbootstrap and none was added).
Full transcripts:
13-LEDGER-UltiSideBar.md(phase evidence tree, not part of this repository).Review fixes
Phase 13 code review (
13-REVIEW-UltiSideBar.md, depth: deep) found the fix above real butincomplete for upgrades, plus two test-quality gaps. All addressed on this branch:
AbstractConfigEntity.init()only fills a@ConfigEntrykeythat is missing from the persisted file and never overwrites one that already exists, so the
%world_name%→%player_world%default change above never reached any server that hadalready run this plugin — exactly the population issue Default sidebar ships %world_name%, which no PlaceholderAPI expansion provides #13 was filed against ("visible to
every user who has not hand-edited
sidebar.yml"). AddedSideBarConfig.migrateLegacyWorldNameDefaultLine(): a targeted, exact-match, one-timemigration that rewrites a persisted
linesentry only when it is byte-identical to the oldshipped default, leaving any operator customisation — including a line that merely mentions
%world_name%alongside other text — untouched. Wired intoSideBarService.init()(calledfrom
UltiSideBar.registerSelf()and, viareload(), fromreloadSelf()), the earliestmodule-owned hook that runs after
AbstractConfigEntity.init()has already populated theconfig from disk —
init()itself isfinaland cannot be overridden, and aConfigChangeListenerregistered from this module would miss the very first load. Idempotent,so re-running it on every reload is a no-op once migrated.
defaultLinesContainNoTokenThatNothingResolvesnow routes every defaultline through
SideBarService's realparsePlaceholders()/PlaceholderAPIseam (stubbed tobehave like a real installation: recognized placeholders substituted, unrecognized ones left
literal) instead of checking token names against a hand-authored allow-list mirroring this
same file's own defaults.
anOperatorConfiguredLineIsUnaffectednow drives a realinit()against atemp
sidebar.yml(via a Mockito default-Answerplugin double, sincegetConfigFolder()/getConfigFile()areprotected finaloutside this test's package), instead of round-trippinga bare Lombok setter/getter that could not fail for any change to
SideBarConfig'sdefault-handling behavior.
%vault_eco_balance_formatted%additionally requires Vaultplus a registered economy provider, a materially larger install surface than "PlaceholderAPI is
installed" alone; the other five default tokens need only PlaceholderAPI.
%server_time_hh:mm:ss%was a 12-hour pattern with no AM/PMmarker; changed to
%server_time_HH:mm:ss%(24-hour).test is now
%[^%]+%(any percent-delimited run), replacing the narrower%([a-zA-Z0-9_:]+)%. This is the same gap the third-party review below caught independently.Third-party (Codex) review on this pull request left one inline comment, on the pre-rewrite
version of the regex covered by WR-01/IN-02 above (
SideBarConfigTest.java:182at commit3205b68): real finding, already fixed by the WR-01 rewrite in this same pull request. Repliedin-thread with the verdict.
Updated test count: 121 tests (was 118), 0 failures, 0 errors,
mvn -B verifyBUILD SUCCESS,coverage checks met.
Round 2
a378d4c) —the renderability test's
stubResolve()substituted%server_time_<suffix>%for"12:00:00"for any nonempty suffix, including one no real
java.text.SimpleDateFormatcan parse (e.g.%server_time_foo%—'f'/'o'are notSimpleDateFormatpattern letters, confirmed theconstructor throws
IllegalArgumentException). A shipped default with an invalid suffix wouldhave passed
defaultLinesContainNoTokenThatNothingResolves()while failing at runtime againsta real PlaceholderAPI installation.
stubResolve()now attemptsnew SimpleDateFormat(suffix).format(new Date())per match and only substitutes when thatsucceeds, leaving an invalid suffix as the literal token — matching what a real installation
would leave unresolved. Added
stubResolveRejectsAnInvalidServerTimePattern()to pin both thereject path and the still-valid shipped
%server_time_HH:mm:ss%accept path. Replied in-threadwith the verdict and evidence.
Updated test count: 122 tests (was 121), 0 failures, 0 errors,
mvn -B verifyBUILD SUCCESS,coverage checks met.
Round 3
1694c8f/e6792ed) -- the round-1 IN-01 fix (hh:mm:ss->HH:mm:ss) and the round-1 CR-01migration only covered the
%world_name%entry:AbstractConfigEntity.init()preserves thewhole persisted
lineslist, so an upgrading server that already had the byte-identical oldshipped
"&f%server_time_hh:mm:ss%"line kept displaying an ambiguous 12-hour time with noAM/PM marker forever. Confirmed the byte-for-byte old string via
git log -pbefore writingthe fix, rather than retyping it from memory. Renamed
migrateLegacyWorldNameDefaultLine()tomigrateLegacyDefaultLines()and generalised it to anexact-match lookup table (
LEGACY_LINE_REPLACEMENTS) covering both tracked legacy defaults --the
%world_name%world line and thehh:mm:ssserver-time line -- so a future shipped-defaultcorrection extends the map, not the loop.
SideBarService.init()'s call site and commentupdated to match. Two new RED tests
(
rewritesLegacyServerTimeLine,rewritesBothLegacyDefaultsOnRealUpgrade) proved the defectagainst the pre-fix method before the fix landed. Replied in-thread with the verdict and
evidence.
Updated test count: 124 tests (was 122), 0 failures, 0 errors,
mvn -B verifyBUILD SUCCESS,coverage checks met.
Pre-merge gate statement
Own code review complete (this change). Third-party review here is the external AI (Codex) review
only — Codacy is not onboarded on module repositories (standing deferred item, Phase 13 D-13).
Real-machine UAT is batched with the rest of this phase's module fixes per Phase 13 D-11 and
terminates at
human-uat-pending; this pull request is not merged as part of that batch by thistask. Required and configured CI must be green before merge.
Risks & Dependencies
🤖 Generated with Claude Code
https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv