feat(config): redesign config system around TOML#10
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change migrates the server configuration from legacy JSON-based files ( ChangesTOML Configuration System
Possibly related PRs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/xcore/plugin/command/controller/server/RuntimeToggleConfigService.java (1)
28-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback in-memory toggle changes if persistence fails.
These methods mutate config first and persist second. If
save()throws, the runtime toggle state is changed but not persisted.💡 Suggested fix
ToggleMutationResult disable(ToggleTarget target, String value) { Set<String> values = mutableSet(target); if (!values.add(value)) { return new ToggleMutationResult(false, value); } - save(); - return new ToggleMutationResult(true, value); + try { + save(); + return new ToggleMutationResult(true, value); + } catch (IllegalStateException e) { + values.remove(value); // rollback + throw e; + } } ToggleMutationResult enable(ToggleTarget target, String value) { Set<String> values = mutableSet(target); if (!values.remove(value)) { return new ToggleMutationResult(false, value); } - save(); - return new ToggleMutationResult(true, value); + try { + save(); + return new ToggleMutationResult(true, value); + } catch (IllegalStateException e) { + values.add(value); // rollback + throw e; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/command/controller/server/RuntimeToggleConfigService.java` around lines 28 - 45, The disable(ToggleTarget target, String value) and enable(ToggleTarget target, String value) methods must rollback the in-memory change if save() fails: wrap the save() call in a try/catch, and on any exception revert the mutation made to the Set returned by mutableSet(target) (i.e., if disable() previously added the value, remove it on failure; if enable() previously removed the value, add it back), then either rethrow the exception (or return a failure ToggleMutationResult) so persistence failures are surfaced; keep using ToggleMutationResult to indicate success/failure. Ensure you reference the mutableSet(target), save(), disable(), enable(), and ToggleMutationResult symbols when applying the fix.
🧹 Nitpick comments (2)
src/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.java (1)
279-286: ⚡ Quick winxconfigShow test currently bypasses the controller execution path
This assertion validates
tomlRenderer.render(config)directly, not the output produced bycontroller.xconfigShow(sender). The test can still pass ifxconfigShowregresses. Please assert sender output (or renderer invocation) from the controller call itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.java` around lines 279 - 286, The test is asserting tomlRenderer.render(config) directly instead of verifying the controller call; update the test to invoke controller.xconfigShow(sender) and assert on the sender's received output (or mock/verify that tomlRenderer.render(...) was invoked with the expected config) rather than the direct render call—specifically, replace the direct assertThat(tomlRenderer.render(config)).contains(...) with assertions that examine sender's messages after controller.xconfigShow(sender) (or use a Mockito/verification on tomlRenderer.render(config) to ensure the controller invoked the renderer with the expected config).src/test/java/org/xcore/plugin/config/TomlSecretsConfigTest.java (1)
142-158: 💤 Low valueConsider clarifying the test name.
The test name mentions "forMapperBlankToNull" which suggests blanks will be converted to null, but the test actually verifies that
normalize()preserves blank strings unchanged. Consider renaming to something like "normalize_preservesBlankOptionalStrings_forLaterMapperConversion" to clarify that the conversion happens elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/xcore/plugin/config/TomlSecretsConfigTest.java` around lines 142 - 158, Rename the test method and its DisplayName to clarify intent: change normalize_preservesBlankOptionalStrings_forMapperBlankToNull() and its DisplayName to something like normalize_preservesBlankOptionalStrings_forLaterMapperConversion (or similar), and update the `@DisplayName` text to state that normalize() preserves blank strings and that blank→null conversion happens elsewhere; locate the test in TomlSecretsConfigTest and adjust the method name and `@DisplayName` accordingly so readers see the distinction between TomlSecretsConfig.normalize() and any subsequent mapper behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/xcore/plugin/command/controller/server/DataController.java`:
- Around line 98-100: The code updates in-memory config before calling
tomlStore.write and logs the raw value; change the sequence in DataController so
you call tomlStore.write(updated) first (catching and handling any exception),
and only assign config = updated after a successful write to avoid memory/disk
divergence; also stop logging raw value in Log.info("Config field '@' updated to
'@'.", field, value) — either log only the field or a masked/obfuscated version
of value (or a redact placeholder) to avoid leaking secrets, and ensure
tomlStore.write errors are surfaced/handled (e.g., rethrow or return error) so
callers know persistence failed.
In `@src/main/java/org/xcore/plugin/config/ConfigTomlLoader.java`:
- Around line 221-223: migrateLegacyXcoreConfig and migrateLegacySecretsConfig
open jsonFile.reader() and pass it into gson.fromJson(...) without closing the
Reader; wrap the reader creation in a try-with-resources (or otherwise ensure
the Reader is closed) when calling gson.fromJson so the stream is always closed
even on parse failure — update the methods (migrateLegacyXcoreConfig and
migrateLegacySecretsConfig) to create the Reader in a try-with-resources block
and call gson.fromJson(reader, Config.class) / gson.fromJson(reader,
Secrets.class) inside that block to prevent file-handle leaks.
In `@src/test/java/org/xcore/plugin/config/TomlSecretsConfigTest.java`:
- Around line 50-56: The test and ProviderConfig currently claim
translation.providers.google defaults to OpenAI-like values (baseUrl
"https://api.openai.com/v1" and model "gpt-5.4"), which is misleading; update
TomlSecretsConfig.TranslationSection.ProviderConfig to either set
Google-appropriate defaults (e.g., Google Translate endpoint and no model field)
or leave baseUrl/model unset/null, and then update TomlSecretsConfigTest
assertions to match the chosen approach (adjust assertions referencing
translation.providers.get("google"), google.baseUrl, and google.model) so the
defaults/template reflect a Google provider instead of OpenAI.
---
Outside diff comments:
In
`@src/main/java/org/xcore/plugin/command/controller/server/RuntimeToggleConfigService.java`:
- Around line 28-45: The disable(ToggleTarget target, String value) and
enable(ToggleTarget target, String value) methods must rollback the in-memory
change if save() fails: wrap the save() call in a try/catch, and on any
exception revert the mutation made to the Set returned by mutableSet(target)
(i.e., if disable() previously added the value, remove it on failure; if
enable() previously removed the value, add it back), then either rethrow the
exception (or return a failure ToggleMutationResult) so persistence failures are
surfaced; keep using ToggleMutationResult to indicate success/failure. Ensure
you reference the mutableSet(target), save(), disable(), enable(), and
ToggleMutationResult symbols when applying the fix.
---
Nitpick comments:
In
`@src/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.java`:
- Around line 279-286: The test is asserting tomlRenderer.render(config)
directly instead of verifying the controller call; update the test to invoke
controller.xconfigShow(sender) and assert on the sender's received output (or
mock/verify that tomlRenderer.render(...) was invoked with the expected config)
rather than the direct render call—specifically, replace the direct
assertThat(tomlRenderer.render(config)).contains(...) with assertions that
examine sender's messages after controller.xconfigShow(sender) (or use a
Mockito/verification on tomlRenderer.render(config) to ensure the controller
invoked the renderer with the expected config).
In `@src/test/java/org/xcore/plugin/config/TomlSecretsConfigTest.java`:
- Around line 142-158: Rename the test method and its DisplayName to clarify
intent: change normalize_preservesBlankOptionalStrings_forMapperBlankToNull()
and its DisplayName to something like
normalize_preservesBlankOptionalStrings_forLaterMapperConversion (or similar),
and update the `@DisplayName` text to state that normalize() preserves blank
strings and that blank→null conversion happens elsewhere; locate the test in
TomlSecretsConfigTest and adjust the method name and `@DisplayName` accordingly so
readers see the distinction between TomlSecretsConfig.normalize() and any
subsequent mapper behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0898e335-38f0-4696-988b-1f52c97942fc
📒 Files selected for processing (26)
README.mdbuild.gradle.ktsgradle/libs.versions.tomlsrc/main/java/org/xcore/plugin/command/controller/server/DataController.javasrc/main/java/org/xcore/plugin/command/controller/server/MaintainController.javasrc/main/java/org/xcore/plugin/command/controller/server/RuntimeToggleConfigService.javasrc/main/java/org/xcore/plugin/config/ConfigFactory.javasrc/main/java/org/xcore/plugin/config/ConfigTomlLoader.javasrc/main/java/org/xcore/plugin/config/ConfigTomlMapper.javasrc/main/java/org/xcore/plugin/config/ConfigTomlTemplateWriter.javasrc/main/java/org/xcore/plugin/config/GlobalConfig.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigPathEditor.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigTomlRenderer.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigTomlStore.javasrc/main/java/org/xcore/plugin/config/TomlSecretsConfig.javasrc/main/java/org/xcore/plugin/config/TomlXcoreConfig.javasrc/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/server/MaintainControllerTest.javasrc/test/java/org/xcore/plugin/config/ConfigFactoryTest.javasrc/test/java/org/xcore/plugin/config/ConfigTomlLoaderTest.javasrc/test/java/org/xcore/plugin/config/ConfigTomlMapperTest.javasrc/test/java/org/xcore/plugin/config/GlobalConfigTest.javasrc/test/java/org/xcore/plugin/config/ServerLocalConfigPathEditorTest.javasrc/test/java/org/xcore/plugin/config/ServerLocalConfigTomlStoreTest.javasrc/test/java/org/xcore/plugin/config/TomlSecretsConfigTest.javasrc/test/java/org/xcore/plugin/config/TomlXcoreConfigTest.java
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Complete the structured-config migration away from legacy org.xcore.plugin.config.Config toward org.xcore.plugin.config.TomlXcoreConfig across 28 production files and 21 test files. Migration slices completed: - ModerationService, command controllers, leaf cleanup (Console, MaintainController, HexedRanks) - TranslationProviderFactory, shared menu plumbing (11 menu files) - Isolated gamemode guards (LastStanding, MiniPvP) - Moderate-risk runtime consumers (GameLifecycleHandler, MiniHexedService, VoteKick) - Final consumers (MigrationService, SmartMapSelector) Preserved behavior: server identity publication, moderation protocol payloads, translation pipeline ordering, menu rendering, gamemode guards, DB lock ownership, map rotation, and all existing test assertions. No remaining direct Config imports in production or test code.
Contributor
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 (1)
src/main/java/org/xcore/plugin/map/SmartMapSelector.java (1)
62-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate error message to reflect the new config field name.
The error message still references the old field name
"eventHubMapID"but the code now usesconfig.eventHub.mapId. This inconsistency may confuse administrators troubleshooting configuration issues.🐛 Proposed fix
if (ObjectId.isValid(config.eventHub.mapId)) { Map map = findMindustryMap(new ObjectId(config.eventHub.mapId)); if (map != null) return map; } else { - Log.err("Invalid eventHubMapID"); + Log.err("Invalid eventHub.mapId: @", config.eventHub.mapId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/map/SmartMapSelector.java` around lines 62 - 68, The Log.err message is still using the old field name ("eventHubMapID"); update the error text to reference the new config field (config.eventHub.mapId) so admins see the correct key; locate the check in SmartMapSelector where you validate config.eventHub.mapId (the block containing ObjectId.isValid(config.eventHub.mapId) and Map map = findMindustryMap(...)) and change Log.err("Invalid eventHubMapID") to a message like "Invalid config.eventHub.mapId" (or equivalent) for clarity.
🧹 Nitpick comments (5)
src/main/java/org/xcore/plugin/map/SmartMapSelector.java (1)
44-44: 💤 Low valueConsider extracting the hardcoded server name to a constant.
The magic string
"event"couples the event-mode logic to a specific server name. Similar to the hexed server check inPlayerDisplayService, this reduces maintainability.♻️ Refactor to use a constant
+ private static final String EVENT_SERVER_NAME = "event"; + `@Override` public Map next(Gamemode mode, Map previous) { - if ("event".equals(config.server.name)) { + if (EVENT_SERVER_NAME.equals(config.server.name)) { eventService.checkTimedEvents();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/map/SmartMapSelector.java` at line 44, The hardcoded server name "event" in SmartMapSelector's conditional couples behavior to a magic string; introduce a descriptive constant (e.g., EVENT_SERVER_NAME) in the SmartMapSelector class and replace the literal in the if ("event".equals(config.server.name)) check with that constant, making it private static final and reusing it wherever the same server-name check is needed (or reference the existing constant from PlayerDisplayService if appropriate) to improve maintainability.src/main/java/org/xcore/plugin/service/PlayerDisplayService.java (1)
103-109: 💤 Low valueConsider extracting the hardcoded server name to a constant.
The magic string
"mini-hexed"is now directly embedded in the logic. If the server name changes or additional servers need hexed rank tags, this will require code modification.♻️ Refactor to use a constant
+ private static final String MINI_HEXED_SERVER_NAME = "mini-hexed"; + public String resolveHexedTag(PlayerData data) { if (data == null || !isMiniHexedServer()) return ""; return data.hexedRank().tag == null ? "" : data.hexedRank().tag; } private boolean isMiniHexedServer() { - return "mini-hexed".equals(config.server.name); + return MINI_HEXED_SERVER_NAME.equals(config.server.name); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/service/PlayerDisplayService.java` around lines 103 - 109, Extract the hardcoded "mini-hexed" string into a single constant (e.g., private static final String MINI_HEXED_SERVER_NAME = "mini-hexed") and update isMiniHexedServer() to compare config.server.name against that constant instead of the literal; ensure you reference the constant wherever else the literal might be used so all comparisons use MINI_HEXED_SERVER_NAME (consider making the constant public or configurable later if needed).src/main/java/org/xcore/plugin/config/ServerLocalConfigTomlRenderer.java (1)
18-20: ⚡ Quick winAvoid mutating runtime config during render.
Line 18 makes
render(...)stateful by normalizing the live config object. For an inspection renderer, prefer normalizing a snapshot and serializing that snapshot.♻️ Proposed refactor
public String render(TomlXcoreConfig config) { Objects.requireNonNull(config, "config must not be null"); - config.normalize(); + TomlMapper mapper = createTomlMapper(); + TomlXcoreConfig snapshot = mapper.convertValue(config, TomlXcoreConfig.class); + snapshot.normalize(); try { - return createTomlMapper().writeValueAsString(config).trim(); + return mapper.writeValueAsString(snapshot).trim(); } catch (IOException e) { throw new IllegalStateException("Failed to render server-local config as TOML: " + e.getMessage(), e); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/config/ServerLocalConfigTomlRenderer.java` around lines 18 - 20, The renderer mutates the live config by calling config.normalize() inside ServerLocalConfigTomlRenderer.render; instead create a snapshot (deep copy) of the config, call normalize() on that snapshot, and serialize the snapshot instead of the original. Locate the render method in ServerLocalConfigTomlRenderer and replace the direct config.normalize() + writeValueAsString(config) sequence with: make a defensive copy (e.g., clone/copy/builder or a copy constructor depending on the config type), call normalize() on the copied instance, then call createTomlMapper().writeValueAsString(snapshot).trim() so the runtime config remains unchanged.src/main/java/org/xcore/plugin/event/handler/GameLifecycleHandler.java (1)
90-90: ⚡ Quick winAvoid hard-coded server identity checks in game-over flow.
Line 90 now hard-codes
"mini-hexed"; this can drift from centralized server-mode logic and is brittle to casing/format changes. Prefer a single helper/normalization path for this classification.Suggested patch
- } else if (state.rules.pvp && !"mini-hexed".equals(config.server.name)) { + } else if (state.rules.pvp && !isMiniHexed(config.server.name)) { message = Strings.format( "Game over! Team @ is victorious with @ players online on map @.", event.winner.name, Groups.player.size(), Strings.capitalize(Strings.stripColors(state.map.name()))); } @@ + private boolean isMiniHexed(String serverName) { + return serverName != null && "mini-hexed".equalsIgnoreCase(serverName.trim()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/event/handler/GameLifecycleHandler.java` at line 90, The conditional in GameLifecycleHandler that checks state.rules.pvp && !"mini-hexed".equals(config.server.name) is brittle due to the hard-coded server name; replace this raw string comparison with a centralized, case-normalized helper or enum lookup (e.g., a ServerMode.fromName(String) or ServerConfig.isMode(ServerMode.MINI_HEXED)) and use that helper in the if condition so comparisons use a single normalization/path; update usages referencing state.rules.pvp and config.server.name to call the new helper (or enum) so the server-mode classification is consistent across the codebase.src/test/java/org/xcore/plugin/event/transport/MapTransportHandlerTest.java (1)
183-190: ⚡ Quick winAdd explicit no-response assertion for ignored load command.
This test says the command is ignored for other servers, but it doesn’t assert that no response is sent on the network.
✅ Suggested test hardening
verifyNoInteractions(mapService); verifyNoInteractions(mapDataRepository); + verify(network, never()).respond(any(), any());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/xcore/plugin/event/transport/MapTransportHandlerTest.java` around lines 183 - 190, The test constructs a MapsLoadCommandV1 for a non-matching server and currently verifies only mapService and mapDataRepository interactions; add an explicit assertion that no network response was sent when the command is ignored by verifying the network/response mock was never used (for example, call verifyNoInteractions(responseSender) or verify(responseSender, never()).send(...)) after listener(listeners, MapsLoadCommandV1.class).get(...); locate the network/response mock used in this test (e.g., responseSender, transportClient, or outboundSender) and assert it had no interactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/xcore/plugin/cloud/config/DisabledCommandPolicy.java`:
- Line 83: In DisabledCommandPolicy, guard against a null runtime before
accessing disabledCommands: update the method that currently returns
"config.runtime.disabledCommands != null &&
!config.runtime.disabledCommands.isEmpty()" to first check "config.runtime !=
null" (e.g., "config.runtime != null && config.runtime.disabledCommands != null
&& !config.runtime.disabledCommands.isEmpty()") so the null runtime case is
handled safely and avoids NPEs when evaluating disabled commands.
In `@src/main/java/org/xcore/plugin/event/transport/MapTransportHandler.java`:
- Around line 49-50: The guard comparisons in MapTransportHandler that use
request.server().equals(config.server.name) (and the similar checks at the other
occurrences) are null-unsafe and can throw NPEs; change them to a null-safe
equality check (e.g., use Objects.equals(config.server.name, request.server())
or explicitly null-check both request.server() and config.server.name before
comparing) so that malformed/partial messages or missing config values do not
cause exceptions — update each occurrence in MapTransportHandler (the checks
currently using request.server().equals(...)) to use the null-safe approach.
In `@src/main/java/org/xcore/plugin/service/GameDataService.java`:
- Around line 185-187: The isMiniHexedServer() method is brittle: update it to
safely handle nulls and case/whitespace variance by null-checking config and
config.server and normalizing config.server.name (trim and toLowerCase or use
equalsIgnoreCase) before comparison; specifically, in isMiniHexedServer() use
guarded checks for config and config.server and compare the trimmed,
case-insensitive server name against "mini-hexed" to avoid NPEs and mismatches.
In `@src/main/java/org/xcore/plugin/service/network/RedisConnectionManager.java`:
- Line 39: RedisConnectionManager currently logs the Redis URL via
PLog.info("Redis connected: url=@",
sanitizeRedisUrl(config.transport.redis.url)), but sanitizeRedisUrl still
preserves query and fragment so secrets can leak; update sanitizeRedisUrl (or
replace its usage here) to parse the URL and strip any query string and fragment
before returning (reconstruct only scheme, userinfo without password, host, port
and path as needed) so PLog.info never receives query/fragment tokens; ensure
the function name sanitizeRedisUrl is kept and its contract returns a URL
without query or fragment to avoid breaking callers like RedisConnectionManager.
---
Outside diff comments:
In `@src/main/java/org/xcore/plugin/map/SmartMapSelector.java`:
- Around line 62-68: The Log.err message is still using the old field name
("eventHubMapID"); update the error text to reference the new config field
(config.eventHub.mapId) so admins see the correct key; locate the check in
SmartMapSelector where you validate config.eventHub.mapId (the block containing
ObjectId.isValid(config.eventHub.mapId) and Map map = findMindustryMap(...)) and
change Log.err("Invalid eventHubMapID") to a message like "Invalid
config.eventHub.mapId" (or equivalent) for clarity.
---
Nitpick comments:
In `@src/main/java/org/xcore/plugin/config/ServerLocalConfigTomlRenderer.java`:
- Around line 18-20: The renderer mutates the live config by calling
config.normalize() inside ServerLocalConfigTomlRenderer.render; instead create a
snapshot (deep copy) of the config, call normalize() on that snapshot, and
serialize the snapshot instead of the original. Locate the render method in
ServerLocalConfigTomlRenderer and replace the direct config.normalize() +
writeValueAsString(config) sequence with: make a defensive copy (e.g.,
clone/copy/builder or a copy constructor depending on the config type), call
normalize() on the copied instance, then call
createTomlMapper().writeValueAsString(snapshot).trim() so the runtime config
remains unchanged.
In `@src/main/java/org/xcore/plugin/event/handler/GameLifecycleHandler.java`:
- Line 90: The conditional in GameLifecycleHandler that checks state.rules.pvp
&& !"mini-hexed".equals(config.server.name) is brittle due to the hard-coded
server name; replace this raw string comparison with a centralized,
case-normalized helper or enum lookup (e.g., a ServerMode.fromName(String) or
ServerConfig.isMode(ServerMode.MINI_HEXED)) and use that helper in the if
condition so comparisons use a single normalization/path; update usages
referencing state.rules.pvp and config.server.name to call the new helper (or
enum) so the server-mode classification is consistent across the codebase.
In `@src/main/java/org/xcore/plugin/map/SmartMapSelector.java`:
- Line 44: The hardcoded server name "event" in SmartMapSelector's conditional
couples behavior to a magic string; introduce a descriptive constant (e.g.,
EVENT_SERVER_NAME) in the SmartMapSelector class and replace the literal in the
if ("event".equals(config.server.name)) check with that constant, making it
private static final and reusing it wherever the same server-name check is
needed (or reference the existing constant from PlayerDisplayService if
appropriate) to improve maintainability.
In `@src/main/java/org/xcore/plugin/service/PlayerDisplayService.java`:
- Around line 103-109: Extract the hardcoded "mini-hexed" string into a single
constant (e.g., private static final String MINI_HEXED_SERVER_NAME =
"mini-hexed") and update isMiniHexedServer() to compare config.server.name
against that constant instead of the literal; ensure you reference the constant
wherever else the literal might be used so all comparisons use
MINI_HEXED_SERVER_NAME (consider making the constant public or configurable
later if needed).
In `@src/test/java/org/xcore/plugin/event/transport/MapTransportHandlerTest.java`:
- Around line 183-190: The test constructs a MapsLoadCommandV1 for a
non-matching server and currently verifies only mapService and mapDataRepository
interactions; add an explicit assertion that no network response was sent when
the command is ignored by verifying the network/response mock was never used
(for example, call verifyNoInteractions(responseSender) or
verify(responseSender, never()).send(...)) after listener(listeners,
MapsLoadCommandV1.class).get(...); locate the network/response mock used in this
test (e.g., responseSender, transportClient, or outboundSender) and assert it
had no interactions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d0ca352-12f1-45b2-841f-d0024bebfc26
📒 Files selected for processing (136)
docs/architecture/server-local-config-refactor-spec.mdsrc/main/java/org/xcore/plugin/cloud/config/DisabledCommandPolicy.javasrc/main/java/org/xcore/plugin/command/CloudCommandRegistrar.javasrc/main/java/org/xcore/plugin/command/controller/client/AuthController.javasrc/main/java/org/xcore/plugin/command/controller/client/BadgeController.javasrc/main/java/org/xcore/plugin/command/controller/client/SocialController.javasrc/main/java/org/xcore/plugin/command/controller/server/BadgeAdminController.javasrc/main/java/org/xcore/plugin/command/controller/server/DataController.javasrc/main/java/org/xcore/plugin/command/controller/server/MaintainController.javasrc/main/java/org/xcore/plugin/command/controller/server/RuntimeToggleConfigService.javasrc/main/java/org/xcore/plugin/command/controller/server/TranslationStatsController.javasrc/main/java/org/xcore/plugin/config/ConfigFactory.javasrc/main/java/org/xcore/plugin/config/ConfigTomlLoader.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigPathEditor.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigTomlRenderer.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigTomlStore.javasrc/main/java/org/xcore/plugin/config/TomlSecretsConfig.javasrc/main/java/org/xcore/plugin/database/migration/MigrationService.javasrc/main/java/org/xcore/plugin/event/TransportService.javasrc/main/java/org/xcore/plugin/event/handler/ConnectionHandler.javasrc/main/java/org/xcore/plugin/event/handler/GameLifecycleHandler.javasrc/main/java/org/xcore/plugin/event/handler/MapVoteHandler.javasrc/main/java/org/xcore/plugin/event/net/chat/ChatMessageHandler.javasrc/main/java/org/xcore/plugin/event/transport/ChatTransportHandler.javasrc/main/java/org/xcore/plugin/event/transport/DiscordLinkTransportHandler.javasrc/main/java/org/xcore/plugin/event/transport/MapTransportHandler.javasrc/main/java/org/xcore/plugin/event/transport/ModerationTransportHandler.javasrc/main/java/org/xcore/plugin/gamemode/hexed/HexedRanks.javasrc/main/java/org/xcore/plugin/gamemode/hexed/MiniHexedService.javasrc/main/java/org/xcore/plugin/gamemode/laststanding/LastStanding.javasrc/main/java/org/xcore/plugin/gamemode/pvp/MiniPvP.javasrc/main/java/org/xcore/plugin/integration/Console.javasrc/main/java/org/xcore/plugin/localization/TranslationProviderFactory.javasrc/main/java/org/xcore/plugin/map/SmartMapSelector.javasrc/main/java/org/xcore/plugin/security/ingress/checks/PlayerLimitCheck.javasrc/main/java/org/xcore/plugin/security/ingress/ipreputation/IpReputationOrchestrationService.javasrc/main/java/org/xcore/plugin/security/ingress/ipreputation/IpReputationPolicy.javasrc/main/java/org/xcore/plugin/service/DiscordLinkService.javasrc/main/java/org/xcore/plugin/service/GameDataService.javasrc/main/java/org/xcore/plugin/service/MapService.javasrc/main/java/org/xcore/plugin/service/PlayerDisplayService.javasrc/main/java/org/xcore/plugin/service/PlayerProfileSettingsService.javasrc/main/java/org/xcore/plugin/service/PrivateMessageService.javasrc/main/java/org/xcore/plugin/service/ServerDiscoveryService.javasrc/main/java/org/xcore/plugin/service/TopMenuCacheService.javasrc/main/java/org/xcore/plugin/service/TopMenuService.javasrc/main/java/org/xcore/plugin/service/TranslationCacheService.javasrc/main/java/org/xcore/plugin/service/TranslationMetricsService.javasrc/main/java/org/xcore/plugin/service/TranslationSafetyService.javasrc/main/java/org/xcore/plugin/service/TranslatorService.javasrc/main/java/org/xcore/plugin/service/moderation/DefaultAuditService.javasrc/main/java/org/xcore/plugin/service/moderation/ModerationService.javasrc/main/java/org/xcore/plugin/service/network/RedisConnectionManager.javasrc/main/java/org/xcore/plugin/service/network/RedisDiscordLinkCodeStore.javasrc/main/java/org/xcore/plugin/service/network/RedisEnvelopeFactory.javasrc/main/java/org/xcore/plugin/service/network/RedisIpReputationAllowlist.javasrc/main/java/org/xcore/plugin/service/network/RedisIpReputationCache.javasrc/main/java/org/xcore/plugin/service/network/RedisNetworkBackend.javasrc/main/java/org/xcore/plugin/service/network/RedisObserverStateStore.javasrc/main/java/org/xcore/plugin/service/network/RedisStreamSupport.javasrc/main/java/org/xcore/plugin/ui/menu/AuditHistoryMenu.javasrc/main/java/org/xcore/plugin/ui/menu/BanMenu.javasrc/main/java/org/xcore/plugin/ui/menu/DiscordMenu.javasrc/main/java/org/xcore/plugin/ui/menu/EventMenu.javasrc/main/java/org/xcore/plugin/ui/menu/HelpMenu.javasrc/main/java/org/xcore/plugin/ui/menu/InformationMenu.javasrc/main/java/org/xcore/plugin/ui/menu/MapFlows.javasrc/main/java/org/xcore/plugin/ui/menu/MapMenu.javasrc/main/java/org/xcore/plugin/ui/menu/Menu.javasrc/main/java/org/xcore/plugin/ui/menu/MessageMenu.javasrc/main/java/org/xcore/plugin/ui/menu/PlayerMenu.javasrc/main/java/org/xcore/plugin/ui/menu/TopMenu.javasrc/main/java/org/xcore/plugin/vote/VoteKick.javasrc/test/java/org/xcore/plugin/cloud/CloudCommandPipelineIntegrationTest.javasrc/test/java/org/xcore/plugin/cloud/config/DisabledCommandPolicyTest.javasrc/test/java/org/xcore/plugin/command/CloudCommandRegistrarTest.javasrc/test/java/org/xcore/plugin/command/controller/client/BadgeControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/client/HexedControllerObserverTest.javasrc/test/java/org/xcore/plugin/command/controller/client/SocialControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/server/BadgeAdminControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/server/MaintainControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/server/RuntimeToggleConfigServiceTest.javasrc/test/java/org/xcore/plugin/command/controller/server/TranslationStatsControllerTest.javasrc/test/java/org/xcore/plugin/config/ConfigFactoryTest.javasrc/test/java/org/xcore/plugin/config/ConfigTomlLoaderTest.javasrc/test/java/org/xcore/plugin/config/ServerLocalConfigPathEditorTest.javasrc/test/java/org/xcore/plugin/config/ServerLocalConfigTomlRendererTest.javasrc/test/java/org/xcore/plugin/config/ServerLocalConfigTomlStoreTest.javasrc/test/java/org/xcore/plugin/config/TomlSecretsConfigTest.javasrc/test/java/org/xcore/plugin/event/NetEventServiceTest.javasrc/test/java/org/xcore/plugin/event/TransportServiceTest.javasrc/test/java/org/xcore/plugin/event/handler/ConnectionHandlerTest.javasrc/test/java/org/xcore/plugin/event/transport/ChatTransportHandlerTest.javasrc/test/java/org/xcore/plugin/event/transport/DiscordLinkTransportHandlerTest.javasrc/test/java/org/xcore/plugin/event/transport/MapTransportHandlerTest.javasrc/test/java/org/xcore/plugin/event/transport/ModerationTransportHandlerTest.javasrc/test/java/org/xcore/plugin/gamemode/ObserverFlowRegressionTest.javasrc/test/java/org/xcore/plugin/gamemode/pvp/MiniPvPRoundStateTest.javasrc/test/java/org/xcore/plugin/localization/OpenAIResponseParserTest.javasrc/test/java/org/xcore/plugin/localization/OpenAITranslationProviderTest.javasrc/test/java/org/xcore/plugin/localization/TranslationProviderFactoryTest.javasrc/test/java/org/xcore/plugin/security/ingress/checks/PlayerLimitCheckTest.javasrc/test/java/org/xcore/plugin/security/ingress/ipreputation/IpReputationOrchestrationServiceTest.javasrc/test/java/org/xcore/plugin/security/ingress/ipreputation/IpReputationPolicyTest.javasrc/test/java/org/xcore/plugin/service/DiscordLinkServiceTest.javasrc/test/java/org/xcore/plugin/service/GameDataServiceTest.javasrc/test/java/org/xcore/plugin/service/MapServiceTest.javasrc/test/java/org/xcore/plugin/service/MapServiceVoteNewWaveTest.javasrc/test/java/org/xcore/plugin/service/PlayerDisplayServiceTest.javasrc/test/java/org/xcore/plugin/service/PlayerProfileSettingsServiceTest.javasrc/test/java/org/xcore/plugin/service/PrivateMessageServiceTest.javasrc/test/java/org/xcore/plugin/service/ServerDiscoveryServiceTest.javasrc/test/java/org/xcore/plugin/service/TopMenuCacheServiceTest.javasrc/test/java/org/xcore/plugin/service/TopMenuServiceTest.javasrc/test/java/org/xcore/plugin/service/TranslationCacheServiceTest.javasrc/test/java/org/xcore/plugin/service/TranslationMetricsServiceTest.javasrc/test/java/org/xcore/plugin/service/TranslationSafetyServiceTest.javasrc/test/java/org/xcore/plugin/service/TranslatorServiceTest.javasrc/test/java/org/xcore/plugin/service/moderation/DefaultAuditServiceTest.javasrc/test/java/org/xcore/plugin/service/moderation/ModerationServiceAvajeTest.javasrc/test/java/org/xcore/plugin/service/network/RedisIpReputationAllowlistTest.javasrc/test/java/org/xcore/plugin/service/network/RedisIpReputationCacheTest.javasrc/test/java/org/xcore/plugin/service/network/RedisNetworkBackendIntegrationTest.javasrc/test/java/org/xcore/plugin/service/network/RedisSupportTest.javasrc/test/java/org/xcore/plugin/ui/AuditHistoryMenuFlowTest.javasrc/test/java/org/xcore/plugin/ui/BanMenuTest.javasrc/test/java/org/xcore/plugin/ui/DiscordMenuTest.javasrc/test/java/org/xcore/plugin/ui/EventMenuTest.javasrc/test/java/org/xcore/plugin/ui/HelpMenuTest.javasrc/test/java/org/xcore/plugin/ui/InformationMenuTest.javasrc/test/java/org/xcore/plugin/ui/MessageMenuTest.javasrc/test/java/org/xcore/plugin/ui/menu/MapMenuTest.javasrc/test/java/org/xcore/plugin/ui/menu/MenuTest.javasrc/test/java/org/xcore/plugin/ui/menu/PlayerMenuTest.javasrc/test/java/org/xcore/plugin/ui/menu/TopMenuTest.java
✅ Files skipped from review due to trivial changes (3)
- src/test/java/org/xcore/plugin/service/TranslationMetricsServiceTest.java
- docs/architecture/server-local-config-refactor-spec.md
- src/test/java/org/xcore/plugin/config/ServerLocalConfigTomlRendererTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/test/java/org/xcore/plugin/config/TomlSecretsConfigTest.java
- src/test/java/org/xcore/plugin/config/ServerLocalConfigPathEditorTest.java
- src/test/java/org/xcore/plugin/config/ServerLocalConfigTomlStoreTest.java
- src/main/java/org/xcore/plugin/config/TomlSecretsConfig.java
- src/test/java/org/xcore/plugin/config/ConfigTomlLoaderTest.java
- src/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.java
- src/test/java/org/xcore/plugin/config/ConfigFactoryTest.java
- src/main/java/org/xcore/plugin/config/ConfigTomlLoader.java
|
|
||
| public boolean hasDisabledCommands() { | ||
| return config.disabledCommands != null && !config.disabledCommands.isEmpty(); | ||
| return config.runtime.disabledCommands != null && !config.runtime.disabledCommands.isEmpty(); |
Contributor
There was a problem hiding this comment.
Guard against null runtime before reading disabled commands.
Line 83 can throw when config.runtime is null, which would break command checks at runtime.
Suggested fix
public boolean hasDisabledCommands() {
- return config.runtime.disabledCommands != null && !config.runtime.disabledCommands.isEmpty();
+ return config.runtime != null
+ && config.runtime.disabledCommands != null
+ && !config.runtime.disabledCommands.isEmpty();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/xcore/plugin/cloud/config/DisabledCommandPolicy.java` at
line 83, In DisabledCommandPolicy, guard against a null runtime before accessing
disabledCommands: update the method that currently returns
"config.runtime.disabledCommands != null &&
!config.runtime.disabledCommands.isEmpty()" to first check "config.runtime !=
null" (e.g., "config.runtime != null && config.runtime.disabledCommands != null
&& !config.runtime.disabledCommands.isEmpty()") so the null runtime case is
handled safely and avoids NPEs when evaluating disabled commands.
Comment on lines
+49
to
50
| if (!request.server().equals(config.server.name)) return; | ||
|
|
Contributor
There was a problem hiding this comment.
Use null-safe server comparison in transport guards.
Lines 49, 65, and 82 can throw NullPointerException on malformed/partial messages or missing config values.
Suggested fix
+import java.util.Objects;
...
network.subscribe(MapsListRequestV1.class, request -> {
- if (!request.server().equals(config.server.name)) return;
+ if (!Objects.equals(request.server(), config.server.name)) return;
...
network.subscribe(MapsRemoveRequestV1.class, request -> {
- if (!request.server().equals(config.server.name)) return;
+ if (!Objects.equals(request.server(), config.server.name)) return;
...
network.subscribe(MapsLoadCommandV1.class, e -> {
- if (!config.server.name.equals(e.server())) return;
+ if (!Objects.equals(config.server.name, e.server())) return;Also applies to: 65-66, 82-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/xcore/plugin/event/transport/MapTransportHandler.java`
around lines 49 - 50, The guard comparisons in MapTransportHandler that use
request.server().equals(config.server.name) (and the similar checks at the other
occurrences) are null-unsafe and can throw NPEs; change them to a null-safe
equality check (e.g., use Objects.equals(config.server.name, request.server())
or explicitly null-check both request.server() and config.server.name before
comparing) so that malformed/partial messages or missing config values do not
cause exceptions — update each occurrence in MapTransportHandler (the checks
currently using request.server().equals(...)) to use the null-safe approach.
Comment on lines
+185
to
+187
| private boolean isMiniHexedServer() { | ||
| return "mini-hexed".equals(config.server.name); | ||
| } |
Contributor
There was a problem hiding this comment.
Harden mini-hexed server detection for null/case variance.
"mini-hexed".equals(config.server.name) is brittle: null nested config values or case/whitespace variation can misclassify stats category.
Suggested fix
private boolean isMiniHexedServer() {
- return "mini-hexed".equals(config.server.name);
+ return config != null
+ && config.server != null
+ && config.server.name != null
+ && "mini-hexed".equalsIgnoreCase(config.server.name.trim());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private boolean isMiniHexedServer() { | |
| return "mini-hexed".equals(config.server.name); | |
| } | |
| private boolean isMiniHexedServer() { | |
| return config != null | |
| && config.server != null | |
| && config.server.name != null | |
| && "mini-hexed".equalsIgnoreCase(config.server.name.trim()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/xcore/plugin/service/GameDataService.java` around lines 185
- 187, The isMiniHexedServer() method is brittle: update it to safely handle
nulls and case/whitespace variance by null-checking config and config.server and
normalizing config.server.name (trim and toLowerCase or use equalsIgnoreCase)
before comparison; specifically, in isMiniHexedServer() use guarded checks for
config and config.server and compare the trimmed, case-insensitive server name
against "mini-hexed" to avoid NPEs and mismatches.
| connectionWarningLogged = false; | ||
| transportHealth.markConnected(); | ||
| PLog.info("Redis connected: url=@", sanitizeRedisUrl(config.redisUrl)); | ||
| PLog.info("Redis connected: url=@", sanitizeRedisUrl(config.transport.redis.url)); |
Contributor
There was a problem hiding this comment.
Stop logging Redis query/fragment to avoid secret leakage.
Current sanitization keeps query/fragment, so tokens/password-style parameters can still end up in logs.
Suggested fix
- if (uri.getQuery() != null && !uri.getQuery().isBlank()) {
- sanitized.append('?').append(uri.getQuery());
- }
- if (uri.getFragment() != null && !uri.getFragment().isBlank()) {
- sanitized.append('#').append(uri.getFragment());
- }
+ // Intentionally drop query/fragment to avoid leaking credentials/tokens.
return sanitized.toString();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/xcore/plugin/service/network/RedisConnectionManager.java`
at line 39, RedisConnectionManager currently logs the Redis URL via
PLog.info("Redis connected: url=@",
sanitizeRedisUrl(config.transport.redis.url)), but sanitizeRedisUrl still
preserves query and fragment so secrets can leak; update sanitizeRedisUrl (or
replace its usage here) to parse the URL and strip any query string and fragment
before returning (reconstruct only scheme, userinfo without password, host, port
and path as needed) so PLog.info never receives query/fragment tokens; ensure
the function name sanitizeRedisUrl is kept and its contract returns a URL
without query or fragment to avoid breaking callers like RedisConnectionManager.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/xcore/plugin/localization/TranslationProviderFactory.java (1)
42-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale wording in log message.
Provider configs are now resolved from the TOML secrets config, but the error message still references "global config", which is misleading post-migration.
📝 Proposed wording fix
- PLog.err("Translation provider '@' is referenced in pipeline but missing in global config", providerId); + PLog.err("Translation provider '@' is referenced in pipeline but missing in secrets config", providerId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/localization/TranslationProviderFactory.java` around lines 42 - 45, The log message in TranslationProviderFactory (where providerConfig is checked) is stale: update the PLog.err call that currently says "global config" to reference the new source, e.g., "secrets config (TOML)" or simply "secrets config", so the message accurately reflects that provider configs are resolved from the TOML secrets config when providerConfig == null; keep the existing context and providerId interpolation in the message.
🧹 Nitpick comments (2)
src/main/java/org/xcore/plugin/service/MapService.java (1)
296-298: ⚡ Quick winAlign
MapService.isEvent()with the repo-wide"event"server.name convention
MapService.isEvent()hardcodes"event".equals(config.server.name), and the same convention is used elsewhere (e.g.,Config.isEvent()isserver.equals("event"), plus other gates likeSmartMapSelector/MapFlows/MapVoteHandler). Tests exercise event behavior by settingconfig.server.name = "event"(MapServiceTest,MapMenuTest,InformationMenuTest);ConnectionHandlerTestsetsmini-pvp, but it doesn’t cover event-map gating. Event functionality only won’t trigger if deployments use a different server naming scheme than exactly"event"; consider delegatingMapService.isEvent()toconfig.isEvent()to avoid duplicated literals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/service/MapService.java` around lines 296 - 298, MapService.isEvent() currently hardcodes "event".equals(config.server.name); change it to delegate to the existing config.isEvent() method instead (replace the body of MapService.isEvent() to return config.isEvent()) so the class uses the repo-wide server naming convention and avoids duplicated string literals (referencing MapService.isEvent(), config.isEvent(), and config.server.name to locate the code).src/main/java/org/xcore/plugin/command/controller/client/SocialController.java (1)
50-50: 💤 Low valueRemove unused
chatFormatServiceDI parameter fromSocialController
chatFormatServiceappears only in the constructor parameter list and is never stored or referenced insrc/main/java/org/xcore/plugin/command/controller/client/SocialController.java. Drop it unless it’s needed for wiring/ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/command/controller/client/SocialController.java` at line 50, Constructor for the SocialController contains an unused DI parameter chatFormatService; remove chatFormatService from the SocialController constructor signature and from any places that instantiate/inject this controller (so DI wiring matches), or if it was intended to be used, assign it to a field (e.g., this.chatFormatService) and use it where needed; update imports accordingly and ensure no unused-parameter compiler warnings remain for the SocialController class.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/xcore/plugin/ui/menu/PlayerProfileFlows.java`:
- Line 168: The pagination field name is misleading: PlayersFlow (ROUTE_PLAYERS,
both in the "select:" handler and render() method) reads
menu.secretsConfig.pagination.eventsPerPage; update the configuration and usage
to a dedicated field (e.g. TomlSecretsConfig.PaginationConfig.playersPerPage)
and change PlayersFlow to read menu.secretsConfig.pagination.playersPerPage
instead; keep a backward-compatible fallback to eventsPerPage if playersPerPage
is absent to avoid breaking existing configs and update any references to
eventsPerPage in render()/select: handler to use the new playersPerPage symbol.
---
Outside diff comments:
In `@src/main/java/org/xcore/plugin/localization/TranslationProviderFactory.java`:
- Around line 42-45: The log message in TranslationProviderFactory (where
providerConfig is checked) is stale: update the PLog.err call that currently
says "global config" to reference the new source, e.g., "secrets config (TOML)"
or simply "secrets config", so the message accurately reflects that provider
configs are resolved from the TOML secrets config when providerConfig == null;
keep the existing context and providerId interpolation in the message.
---
Nitpick comments:
In
`@src/main/java/org/xcore/plugin/command/controller/client/SocialController.java`:
- Line 50: Constructor for the SocialController contains an unused DI parameter
chatFormatService; remove chatFormatService from the SocialController
constructor signature and from any places that instantiate/inject this
controller (so DI wiring matches), or if it was intended to be used, assign it
to a field (e.g., this.chatFormatService) and use it where needed; update
imports accordingly and ensure no unused-parameter compiler warnings remain for
the SocialController class.
In `@src/main/java/org/xcore/plugin/service/MapService.java`:
- Around line 296-298: MapService.isEvent() currently hardcodes
"event".equals(config.server.name); change it to delegate to the existing
config.isEvent() method instead (replace the body of MapService.isEvent() to
return config.isEvent()) so the class uses the repo-wide server naming
convention and avoids duplicated string literals (referencing
MapService.isEvent(), config.isEvent(), and config.server.name to locate the
code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e634d35-9b1d-4b73-abb8-3818f8766379
📒 Files selected for processing (91)
src/main/java/org/xcore/plugin/cloud/config/CloudGuardConfigurer.javasrc/main/java/org/xcore/plugin/command/controller/client/SocialController.javasrc/main/java/org/xcore/plugin/command/controller/server/TranslationStatsController.javasrc/main/java/org/xcore/plugin/config/ConfigFactory.javasrc/main/java/org/xcore/plugin/config/ConfigTomlLoader.javasrc/main/java/org/xcore/plugin/database/MongoFactory.javasrc/main/java/org/xcore/plugin/database/migration/MigrationService.javasrc/main/java/org/xcore/plugin/database/repository/AdminDataRepository.javasrc/main/java/org/xcore/plugin/database/repository/AuditRecordRepository.javasrc/main/java/org/xcore/plugin/database/repository/BanDataRepository.javasrc/main/java/org/xcore/plugin/database/repository/DataRepository.javasrc/main/java/org/xcore/plugin/database/repository/EventDataRepository.javasrc/main/java/org/xcore/plugin/database/repository/GameDataRepository.javasrc/main/java/org/xcore/plugin/database/repository/MapDataRepository.javasrc/main/java/org/xcore/plugin/database/repository/MuteDataRepository.javasrc/main/java/org/xcore/plugin/database/repository/PlayerDataRepository.javasrc/main/java/org/xcore/plugin/database/repository/PrivateMessageRepository.javasrc/main/java/org/xcore/plugin/event/handler/ConnectionHandler.javasrc/main/java/org/xcore/plugin/localization/OpenAIRequestFactory.javasrc/main/java/org/xcore/plugin/localization/OpenAITranslationProvider.javasrc/main/java/org/xcore/plugin/localization/TranslationProviderFactory.javasrc/main/java/org/xcore/plugin/security/ingress/checks/BanCheck.javasrc/main/java/org/xcore/plugin/security/ingress/checks/IpReputationCheck.javasrc/main/java/org/xcore/plugin/security/ingress/ipreputation/IpApiProvider.javasrc/main/java/org/xcore/plugin/service/MapService.javasrc/main/java/org/xcore/plugin/service/PlayerActivityService.javasrc/main/java/org/xcore/plugin/service/PrivateMessageService.javasrc/main/java/org/xcore/plugin/session/Session.javasrc/main/java/org/xcore/plugin/ui/menu/AuditHistoryFlows.javasrc/main/java/org/xcore/plugin/ui/menu/AuditHistoryMenu.javasrc/main/java/org/xcore/plugin/ui/menu/BanMenu.javasrc/main/java/org/xcore/plugin/ui/menu/DiscordFlows.javasrc/main/java/org/xcore/plugin/ui/menu/DiscordMenu.javasrc/main/java/org/xcore/plugin/ui/menu/EventDraftFlows.javasrc/main/java/org/xcore/plugin/ui/menu/EventFlows.javasrc/main/java/org/xcore/plugin/ui/menu/EventMenu.javasrc/main/java/org/xcore/plugin/ui/menu/HelpFlows.javasrc/main/java/org/xcore/plugin/ui/menu/HelpMenu.javasrc/main/java/org/xcore/plugin/ui/menu/InformationMenu.javasrc/main/java/org/xcore/plugin/ui/menu/MapFlows.javasrc/main/java/org/xcore/plugin/ui/menu/MapMenu.javasrc/main/java/org/xcore/plugin/ui/menu/Menu.javasrc/main/java/org/xcore/plugin/ui/menu/MessageFlows.javasrc/main/java/org/xcore/plugin/ui/menu/MessageMenu.javasrc/main/java/org/xcore/plugin/ui/menu/PlayerMenu.javasrc/main/java/org/xcore/plugin/ui/menu/PlayerProfileFlows.javasrc/main/java/org/xcore/plugin/ui/menu/TopMenu.javasrc/main/java/org/xcore/plugin/vote/VoteEvent.javasrc/main/java/org/xcore/plugin/vote/VoteKick.javasrc/main/java/org/xcore/plugin/vote/VoteNewWave.javasrc/main/java/org/xcore/plugin/vote/VoteRtv.javasrc/main/java/org/xcore/plugin/vote/VoteSession.javasrc/test/java/org/xcore/plugin/cloud/CloudCommandPipelineIntegrationTest.javasrc/test/java/org/xcore/plugin/command/controller/client/BadgeControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/client/SocialControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/server/BadgeAdminControllerTest.javasrc/test/java/org/xcore/plugin/command/controller/server/TranslationStatsControllerTest.javasrc/test/java/org/xcore/plugin/config/ConfigFactoryTest.javasrc/test/java/org/xcore/plugin/config/ConfigTomlLoaderTest.javasrc/test/java/org/xcore/plugin/database/repository/AuditRecordRepositoryLogicTest.javasrc/test/java/org/xcore/plugin/database/repository/EventDataRepositoryLogicTest.javasrc/test/java/org/xcore/plugin/event/handler/ConnectionHandlerTest.javasrc/test/java/org/xcore/plugin/event/transport/ModerationTransportHandlerTest.javasrc/test/java/org/xcore/plugin/localization/LocalizationUserLanguageTest.javasrc/test/java/org/xcore/plugin/localization/OpenAITranslationProviderTest.javasrc/test/java/org/xcore/plugin/localization/TranslationProviderFactoryTest.javasrc/test/java/org/xcore/plugin/security/ingress/checks/BanCheckTest.javasrc/test/java/org/xcore/plugin/security/ingress/checks/IpReputationCheckTest.javasrc/test/java/org/xcore/plugin/security/ingress/ipreputation/IpApiProviderTest.javasrc/test/java/org/xcore/plugin/service/EventEditorServiceTest.javasrc/test/java/org/xcore/plugin/service/MapServiceTest.javasrc/test/java/org/xcore/plugin/service/MapServiceVoteNewWaveTest.javasrc/test/java/org/xcore/plugin/service/PrivateMessageServiceTest.javasrc/test/java/org/xcore/plugin/session/SessionObserverStateTest.javasrc/test/java/org/xcore/plugin/session/SessionUiStateTest.javasrc/test/java/org/xcore/plugin/ui/AuditHistoryMenuFlowTest.javasrc/test/java/org/xcore/plugin/ui/BanMenuTest.javasrc/test/java/org/xcore/plugin/ui/DiscordMenuTest.javasrc/test/java/org/xcore/plugin/ui/EventMenuTest.javasrc/test/java/org/xcore/plugin/ui/HelpMenuTest.javasrc/test/java/org/xcore/plugin/ui/InformationMenuTest.javasrc/test/java/org/xcore/plugin/ui/MenuBuilderTest.javasrc/test/java/org/xcore/plugin/ui/MenuServiceFlowTest.javasrc/test/java/org/xcore/plugin/ui/MenuServiceTest.javasrc/test/java/org/xcore/plugin/ui/MessageMenuTest.javasrc/test/java/org/xcore/plugin/ui/menu/DateTimePickerFlowTest.javasrc/test/java/org/xcore/plugin/ui/menu/MapMenuTest.javasrc/test/java/org/xcore/plugin/ui/menu/PlayerMenuTest.javasrc/test/java/org/xcore/plugin/ui/menu/TopMenuTest.javasrc/test/java/org/xcore/plugin/vote/VoteNewWaveTest.javasrc/test/java/org/xcore/plugin/vote/VoteServiceAvajeTest.java
🚧 Files skipped from review as they are similar to previous changes (17)
- src/test/java/org/xcore/plugin/ui/AuditHistoryMenuFlowTest.java
- src/test/java/org/xcore/plugin/ui/menu/PlayerMenuTest.java
- src/test/java/org/xcore/plugin/cloud/CloudCommandPipelineIntegrationTest.java
- src/test/java/org/xcore/plugin/service/MapServiceVoteNewWaveTest.java
- src/test/java/org/xcore/plugin/service/MapServiceTest.java
- src/test/java/org/xcore/plugin/event/transport/ModerationTransportHandlerTest.java
- src/test/java/org/xcore/plugin/service/PrivateMessageServiceTest.java
- src/main/java/org/xcore/plugin/ui/menu/InformationMenu.java
- src/test/java/org/xcore/plugin/localization/TranslationProviderFactoryTest.java
- src/main/java/org/xcore/plugin/event/handler/ConnectionHandler.java
- src/test/java/org/xcore/plugin/command/controller/server/BadgeAdminControllerTest.java
- src/test/java/org/xcore/plugin/config/ConfigTomlLoaderTest.java
- src/test/java/org/xcore/plugin/command/controller/client/BadgeControllerTest.java
- src/test/java/org/xcore/plugin/command/controller/client/SocialControllerTest.java
- src/test/java/org/xcore/plugin/ui/menu/MapMenuTest.java
- src/test/java/org/xcore/plugin/config/ConfigFactoryTest.java
- src/main/java/org/xcore/plugin/config/ConfigTomlLoader.java
| int index = Integer.parseInt(indexStr); | ||
| List<Session> onlinePlayers = getFilteredOnlinePlayers(ctx.session(), sessionService, playerDisplayService); | ||
| int perPage = menu.globalConfig.eventsPerPage; | ||
| int perPage = menu.secretsConfig.pagination.eventsPerPage; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the pagination section fields to see if a player-specific page size exists.
fd -i 'TomlSecretsConfig.java' --exec rg -n -C2 'class Pagination|PerPage'
# Check other consumers of pagination.* to compare naming conventions.
rg -nP '\.pagination\.\w+PerPage' --type=javaRepository: XCore-mindustry/XCore-plugin
Length of output: 7233
🏁 Script executed:
#!/bin/bash
# Inspect relevant sections around the pagination usage in PlayerProfileFlows
sed -n '140,220p' src/main/java/org/xcore/plugin/ui/menu/PlayerProfileFlows.java
# Inspect PlayerMenuTest to see how eventsPerPage is used for the player list/menu
sed -n '1,140p' src/test/java/org/xcore/plugin/ui/menu/PlayerMenuTest.javaRepository: XCore-mindustry/XCore-plugin
Length of output: 9731
Clarify/rename pagination.eventsPerPage since it’s used for ROUTE_PLAYERS pagination
PlayersFlow (online players list) paginates with menu.secretsConfig.pagination.eventsPerPage in both the “select:” handler and render(). TomlSecretsConfig.PaginationConfig doesn’t define a playersPerPage field (only events/maps/commands/privateMessagesPerPage), so this is currently the only knob; the name is misleading—either document the reuse or add/rename a dedicated players page-size setting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/xcore/plugin/ui/menu/PlayerProfileFlows.java` at line 168,
The pagination field name is misleading: PlayersFlow (ROUTE_PLAYERS, both in the
"select:" handler and render() method) reads
menu.secretsConfig.pagination.eventsPerPage; update the configuration and usage
to a dedicated field (e.g. TomlSecretsConfig.PaginationConfig.playersPerPage)
and change PlayersFlow to read menu.secretsConfig.pagination.playersPerPage
instead; keep a backward-compatible fallback to eventsPerPage if playersPerPage
is absent to avoid breaking existing configs and update any references to
eventsPerPage in render()/select: handler to use the new playersPerPage symbol.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/xcore/plugin/config/HumanReadableTomlWriter.java (1)
91-111: ⚡ Quick winHandle list-of-object/table values in
HumanReadableTomlWriter(current DTOs are safe, but representation would be wrong if added later).The current TOML DTOs (
TomlXcoreConfig/TomlSecretsConfig) don’t contain collection fields whose elements are objects/tables: they use onlyList<String>/Set<String>, and nested sections are represented asMap<String, ProviderConfig>(whichappendSectionrenders as TOML tables). So there’s no current path where aList<LinkedHashMap>/ map element gets passed throughformatList(...).If a future schema introduces something like
List<SomeConfig>(or any collection whose element is a non-scalar map/object),formatValue(...)will fall to thedefaultcase and render those elements as quotedString.valueOf(map)(e.g."{name=x, port=80}") instead of TOML inline tables / array-of-tables. Consider adding special handling forMapelements insideformatList, or failing fast with a clear error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/xcore/plugin/config/HumanReadableTomlWriter.java` around lines 91 - 111, formatValue currently serializes non-scalar collection elements (maps/objects) incorrectly by falling into the default branch and quoting String.valueOf(...); update formatValue/formatList to detect Map and/or table-like objects and render them as TOML inline tables or array-of-tables (or else throw a clear exception) instead of quoting. Specifically, in formatValue add branches for Map<?,?> (and possibly for POJO/table types used by appendSection) that convert maps to a TOML inline table string, and in formatList ensure elements that are Map are passed through that same map-to-TOML rendering rather than the default; keep iterableToList usage for Iterable but ensure map handling is centralized so future List<SomeConfig>/List<LinkedHashMap> are correctly emitted or fail fast with a descriptive error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/java/org/xcore/plugin/config/HumanReadableTomlWriter.java`:
- Around line 91-111: formatValue currently serializes non-scalar collection
elements (maps/objects) incorrectly by falling into the default branch and
quoting String.valueOf(...); update formatValue/formatList to detect Map and/or
table-like objects and render them as TOML inline tables or array-of-tables (or
else throw a clear exception) instead of quoting. Specifically, in formatValue
add branches for Map<?,?> (and possibly for POJO/table types used by
appendSection) that convert maps to a TOML inline table string, and in
formatList ensure elements that are Map are passed through that same map-to-TOML
rendering rather than the default; keep iterableToList usage for Iterable but
ensure map handling is centralized so future
List<SomeConfig>/List<LinkedHashMap> are correctly emitted or fail fast with a
descriptive error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cdfc252-de63-4a45-ab1c-487b9ab7d4c0
📒 Files selected for processing (8)
src/main/java/org/xcore/plugin/config/ConfigTomlLoader.javasrc/main/java/org/xcore/plugin/config/HumanReadableTomlWriter.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigTomlRenderer.javasrc/main/java/org/xcore/plugin/config/ServerLocalConfigTomlStore.javasrc/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.javasrc/test/java/org/xcore/plugin/config/ConfigTomlLoaderTest.javasrc/test/java/org/xcore/plugin/config/ServerLocalConfigTomlRendererTest.javasrc/test/java/org/xcore/plugin/config/ServerLocalConfigTomlStoreTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/xcore/plugin/config/ServerLocalConfigTomlRenderer.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/org/xcore/plugin/config/ServerLocalConfigTomlRendererTest.java
- src/test/java/org/xcore/plugin/config/ServerLocalConfigTomlStoreTest.java
- src/test/java/org/xcore/plugin/command/controller/server/DataControllerTest.java
- src/main/java/org/xcore/plugin/config/ConfigTomlLoader.java
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
New Features
Documentation
Tests
Chores