fix(godmode): handle case where no provider is available#189
fix(godmode): handle case where no provider is available#189sharrlotte merged 20 commits intomainfrom
Conversation
Set provider to null when neither JS nor internal providers are available to prevent potential null pointer exceptions.
Remove unused import and simplify the formatted string when showing original message. Instead of displaying locale name on a separate line, show translated text inline with original in parentheses.
- Remove unused NoopTranslationProvider class - Simplify provider selection logic by eliminating no-op provider - Update test button disabling condition to only check testing state
This prevents units from moving too close to the enemy when fleeing, which could cause them to get stuck or take unnecessary damage. The previous value of 0 allowed units to approach the target's flee range boundary too closely.
Adds a null check for `e.tile.build` to avoid a potential NullPointerException when handling double-tap detection on a tile that has no building.
- Extract hold detection logic into TapListener for reuse across features - SmartDrillFeature now uses hold gesture instead of double-tap for drill menu - SmartUpgradeFeature uses hold gesture for upgrade menu instead of complex click tracking - Remove duplicate tile tracking logic from both features - Centralize hold duration and tile validation in TapListener
Using Throwable ensures that both Exception and Error types are caught, preventing unhandled errors from crashing the UI when copying schematics.
Add Timer.schedule to call switchProvider every 60 seconds, ensuring consistent god mode state during long play sessions beyond PlayEvent and StateChangeEvent triggers.
Add early return to avoid switching providers when current provider is still available. This prevents potential issues when both providers are available simultaneously.
The TapListener class was previously located in the godmode feature package but is used by multiple features (godmode, smartdrill, smartupgrade). Moving it to a common services package improves code organization and reusability across the codebase.
Only display user avatar and name for the first message in a consecutive series from the same user. Adjust padding between messages to visually group messages from the same user together.
- Add left/right padding to labels in chat settings for better visual spacing - Remove explicit table style and button styles from smart drill menu to use defaults
- Change icon parameter type to TextureRegionDrawable for consistency - Add scalable icon utility to prevent scaling issues in UI - Fix inconsistent spacing in icon method calls - Update progress display icon from Tex.bar to Icon.chartBar - Remove explicit size setting for icons in quick access buttons
- Use consistent Icon.eye for toggle rendering feature metadata - Move enabled/disabled icon before settings button in feature card header to improve visual hierarchy
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCentralized touch/tap/hold handling into a new services.TapListener, removed the old godmode TapListener, added scalable drawable caching, refactored pathfinding caching and settings UI, converted smart features to hold-based interactions, adjusted chat/message grouping and UI padding, fixed autoplay flee logic, improved schematic error reporting, and bumped module version. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (touch)
participant Events as Events/Trigger System
participant Tap as services.TapListener
participant SmartDrill as SmartDrillFeature
participant SmartUpgrade as SmartUpgradeFeature
rect rgba(200,220,100,0.5)
Note over User,Tap: Hold-based flow via new TapListener
User->>Events: Touch start / move / end on Tile
Events->>Tap: TapEvent + update loop signal
Tap->>Tap: track touchTile, accumulate hold time
alt hold threshold reached (e.g., 300ms)
Tap->>SmartDrill: invoke hold callback -> showMenu
Tap->>SmartUpgrade: invoke hold callback -> showMenu
end
User->>Events: Tap or release
Events->>Tap: detect tap/release -> invoke select callbacks or close handlers
Tap->>SmartDrill: invoke tap-close handler
Tap->>SmartUpgrade: invoke tap-close handler
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mindustrytool/features/chat/translation/ChatTranslationFeature.java (1)
119-121:⚠️ Potential issue | 🟠 MajorPersist a valid provider ID when falling back from an unknown one.
When the stored provider ID is invalid (e.g., legacy
"noop"), you correctly fall back in memory, but settings stay stale. Persist the fallback ID here to complete migration and prevent repeated invalid loads.💡 Proposed fix
if (currentProvider == null) { currentProvider = defaultTranslationProvider; + ChatTranslationConfig.setProviderId(defaultTranslationProvider.getId()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/chat/translation/ChatTranslationFeature.java` around lines 119 - 121, When falling back from an unknown provider (currentProvider == null) you must persist the fallback so settings stop containing the stale/legacy ID; after assigning currentProvider = defaultTranslationProvider in ChatTranslationFeature (use currentProvider and defaultTranslationProvider), write the default provider's ID into the settings/config store (e.g. call the project's method that sets the translation provider id such as settings.setTranslationProviderId(defaultTranslationProvider.getId()) or config.setProviderId(...)) and persist it (e.g. settings.save() or config.persist()) so the migrated value is saved for future loads.
🧹 Nitpick comments (7)
src/mindustrytool/features/chat/translation/ChatTranslationFeature.java (2)
93-100: Renameformatedtoformattedfor readability.Small naming typo makes async flow harder to scan.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/chat/translation/ChatTranslationFeature.java` around lines 93 - 100, The variable name "formated" is a typo and should be renamed to "formatted" for clarity; update the local variable assigned from Strings.format (currently "formated") to "formatted" inside the translation lambda and update the thenAccept call to use the same "formatted" identifier (i.e., thenAccept(formatted -> cons.get(formatted)) or the existing consumer usage) so all references match the new name in ChatTranslationFeature.java.
208-209: Avoid using button text as state for disable logic.Disabling based on localized text is brittle; use a boolean state flag instead.
♻️ Proposed refactor
+ final boolean[] isTesting = {false}; TextButton testButton = new TextButton(Core.bundle.get("chat-translation.settings.test-button"), Styles.defaultt); testButton.clicked(() -> { - testButton.setDisabled(true); + isTesting[0] = true; + testButton.setDisabled(true); testButton.setText(Core.bundle.get("chat-translation.settings.testing")); resultLabel.setText(Core.bundle.get("chat-translation.settings.testing-connection")); @@ Core.app.post(() -> { + isTesting[0] = false; testButton.setDisabled(false); testButton.setText(Core.bundle.get("chat-translation.settings.test-button")); resultLabel.setText(Core.bundle.get("chat-translation.settings.success") + result); }); }).exceptionally(e -> { Core.app.post(() -> { + isTesting[0] = false; testButton.setDisabled(false); testButton.setText(Core.bundle.get("chat-translation.settings.test-button")); resultLabel.setText(Core.bundle.get("chat-translation.settings.failed") + e.getMessage()); }); return null; }); }); @@ root.add(testButton).size(250, 50).pad(10) - .disabled(b -> testButton.getText().toString() - .equals(Core.bundle.get("chat-translation.settings.testing"))) + .disabled(b -> isTesting[0]) .row();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/chat/translation/ChatTranslationFeature.java` around lines 208 - 209, The disable predicate for the testButton currently relies on comparing its localized text (the lambda passed to .disabled(...) using testButton.getText().toString().equals(Core.bundle.get("chat-translation.settings.testing"))) which is brittle; introduce a boolean state field (e.g., testInProgress or isTesting) in ChatTranslationFeature, change the .disabled(...) predicate to reference that flag instead of button text, and set that flag to true when the test starts and false when the test finishes (ensure you update the UI after toggling the flag so the button re-evaluates its disabled state).src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java (2)
89-91: Consider clearingpathCacheon world load as well.
reset()clearsspawnPathCachebut notpathCache. After a world load, orphaned unit path entries from the previous world will persist until the 60-second cleanup evicts them. While not a functional bug, clearing both caches ensures immediate memory release and avoids stale data.Proposed fix
public void reset() { spawnPathCache.clear(); + pathCache.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java` around lines 89 - 91, The reset() method in PathfindingDisplay currently clears spawnPathCache but leaves pathCache populated, causing stale unit paths to persist after a world load; update reset() to also clear pathCache (i.e., call pathCache.clear()) so both caches are immediately released on world load and avoid waiting for the 60-second cleanup.
243-268: Opacity gradient can be inconsistent when unit count fluctuates.The
maxStepsparameter controls the fade gradient (line 262), but cached paths may have been computed with a differentmaxStepsvalue than the current draw call uses. If unit count crosses a threshold between cache computation and rendering, the fade effect will be inconsistent.Consider using
totalSegments(derived from actual cache size) for the fade calculation to ensure visual consistency:Proposed fix
for (int i = 0; i < totalSegments; i++) { float nextX = cacheEntry.data[(i + 1) * 2]; float nextY = cacheEntry.data[(i + 1) * 2 + 1]; - Draw.color(pathColor, (1f - ((float) i / maxSteps)) * currentOpacity); + Draw.color(pathColor, (1f - ((float) i / totalSegments)) * currentOpacity); Lines.line(currentX, currentY, nextX, nextY); currentX = nextX; currentY = nextY; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java` around lines 243 - 268, The fade uses the caller-supplied maxSteps which can differ from the cached path length; in drawFromCache(PathfindingCache, Color, int) compute the fade using the actual path length by deriving an effectiveSteps value from totalSegments (e.g., effectiveSteps = Math.max(1, totalSegments) or Math.min(maxSteps, totalSegments) depending on desired behavior) and replace uses of maxSteps in the Draw.color alpha calculation with that effectiveSteps so the opacity gradient is based on the actual cached segment count; ensure you handle zero/one segments to avoid division by zero.src/mindustrytool/features/display/pathfinding/PathfindingSettingsUI.java (1)
96-104: Consider deriving cost type count fromPathfindingConfigto avoid sync issues.The
costNamesarray has 6 elements matchingPathfindingConfig.costTypesEnabled[6], but both are hardcoded. If the config changes, this UI would silently break.Suggested improvement
Add a constant or getter in
PathfindingConfig:// In PathfindingConfig.java public static final int COST_TYPE_COUNT = 6; // or public static int getCostTypeCount() { return costTypesEnabled.length; }Then use it here instead of relying on array length matching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/display/pathfinding/PathfindingSettingsUI.java` around lines 96 - 104, The UI currently hardcodes the costNames array length which must match PathfindingConfig.costTypesEnabled; update PathfindingConfig to expose a stable count (e.g., public static final int COST_TYPE_COUNT or public static int getCostTypeCount() that returns costTypesEnabled.length) and then modify PathfindingSettingsUI to build/iterate costNames using that count (or validate/truncate/extend costNames to that count) so the loop that calls PathfindingConfig.isCostTypeEnabled(index) and PathfindingConfig.setCostTypeEnabled(index, c) always aligns with PathfindingConfig's actual number of cost types.src/mindustrytool/features/chat/global/ui/MessageList.java (1)
165-187: Reuse a singlefindUserByIdfuture for the header.Line 165 and Line 187 both call
UserService.findUserById(msg.createdBy). On a cold cache, that registers two callbacks for the same id insrc/mindustrytool/services/UserService.java:80-100; caching the future once here would cut that churn and keep the avatar/name updates tied to the same lookup result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/chat/global/ui/MessageList.java` around lines 165 - 187, The code calls UserService.findUserById(msg.createdBy) twice (once to set avatar and again to set header label), causing duplicate lookups; capture the returned CompletableFuture once into a local variable (e.g., var userFuture = UserService.findUserById(msg.createdBy)) before the UI blocks and then call userFuture.thenAccept(...) for both the avatar update (avatar.clear()/avatar.add(...)) and the header label update inside entry.table (the Label created for the name), so both UI updates share the same lookup result and avoid registering two callbacks for the same id.src/mindustrytool/Utils.java (1)
45-46: Make the string icon cache atomic too.Switching these maps to
ConcurrentHashMaponly helps if the load path is atomic.icons(String)still does acontainsKey/putcheck-then-act sequence, so concurrent misses can load the sameTexturetwice and keep an extra GPU resource alive. Apply the samecomputeIfAbsentpattern there as well.Also applies to: 214-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/Utils.java` around lines 45 - 46, The string-keyed icon cache (iconCache) is still used with a containsKey/put pattern in icons(String) causing race double-loads; change icons(String) to use iconCache.computeIfAbsent(key, k -> { ...load Texture/TextureRegionDrawable... }) so the load path is atomic and only one Texture is created per key; also apply the same computeIfAbsent pattern for the other occurrence around lines 214-216 (and for scalableIconCache/scalableIcons if a similar check-then-put is present) so all cache insertions are done atomically via computeIfAbsent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mindustrytool/features/browser/schematic/SchematicDialog.java`:
- Around line 405-406: The catch block in SchematicDialog that currently reads
"catch (Throwable e)" should be narrowed to "catch (Exception e)" so fatal JVM
Errors (e.g., OutOfMemoryError, StackOverflowError) are not swallowed; locate
the try/catch containing ui.showException(e) in SchematicDialog.java and replace
the Throwable catch with Exception while keeping the existing
ui.showException(e) call and any surrounding finally/cleanup logic intact.
In `@src/mindustrytool/features/chat/global/ui/MessageList.java`:
- Around line 189-212: The label.setText call can NPE because timeStr may be
null when msg.createdAt is null; in MessageList.java (around the block that
builds timeStr and calls label.setText) ensure timeStr is non-null before
calling isEmpty() — either initialize timeStr to "" instead of msg.createdAt or
change the concatenation to check timeStr != null && !timeStr.isEmpty() before
appending the time. Update the code paths in the Instant.parse try/catch and the
fallback to guarantee timeStr is a safe (possibly empty) string used by
label.setText.
- Around line 231-236: The grouped-top gap is currently decided from msg.replyTo
but the reply preview row is only added when the target message exists, causing
incorrect spacing; modify MessageList rendering so you track whether the reply
preview was actually rendered (e.g., set a boolean like replyRendered when you
add the reply row in the block that calls card.table(...).row()) and then use
that boolean instead of msg.replyTo when computing padTop for the content card
in renderContent/card.table, ensuring padTop uses isSameUser && !replyRendered
(or the inverse as appropriate) to preserve fallback spacing when the reply
target is missing.
In `@src/mindustrytool/features/display/pathfinding/PathfindingSettingsUI.java`:
- Around line 22-25: The Reset button in PathfindingSettingsUI currently only
calls PathfindingConfig.setZoomThreshold(0.5f) so add logic to restore all
settings to their defaults when the user clicks the button: either implement and
call a new PathfindingConfig.resetToDefaults() that sets opacity, draw toggles,
cost types, zoomThreshold, etc. back to their default values, or update the
lambda in settingsDialog.buttons.button(...) to explicitly set each config value
(e.g., opacity, drawFlags, costType, zoomThreshold) back to defaults and then
call rebuildSettings(); ensure you modify PathfindingSettingsUI and/or
PathfindingConfig classes and reference the same public setters/getters used
elsewhere so the UI reflects the reset.
In `@src/mindustrytool/features/display/quickaccess/QuickAccessHud.java`:
- Around line 196-200: The image cell inside the settings button isn't
explicitly sized so Utils.scalable(Icon.settings) with Scaling.fit only fits to
the image's intrinsic bounds; update the button builder where btnRef[0] is
created (the .button(...) call using Utils.scalable(Icon.settings),
Styles.clearNonei and buttonSize) to set the image cell size to the same
buttonSize (or a derived value) before applying Scaling.fit so the icon scales
to the button bounds; ensure the change targets the image actor/cell used in
that button creation so Main.featureSettingDialog.show() behavior stays the
same.
In `@src/mindustrytool/features/FeatureMetadata.java`:
- Around line 71-73: The Builder.icon method currently calls
Utils.scalable(icon) unconditionally which breaks the builder's null validation
path; change Builder.icon(TextureRegionDrawable icon) to guard against null by
not invoking Utils.scalable when icon is null (i.e., set this.icon = null or
leave unchanged) so that null inputs still reach the Builder.build() null check
and throw the original "Icon is required" error; update the Builder.icon method
(and any related field) to preserve the original validation flow rather than
performing scaling eagerly in Utils.scalable.
In `@src/mindustrytool/features/godmode/GodModeFeature.java`:
- Around line 50-61: The current switchProvider() makes a fallback sticky by
early-returning when any available provider is already set; change it to
recompute from the user preference (useJS) instead of honoring the previous
provider blindly: in switchProvider(), determine the preferred provider using
useJS (preferred = useJS ? js : internal), then if preferred.isAvailable() set
provider = preferred; else if the other provider is available set provider =
other; otherwise set provider = null; remove the existing early-return that only
checks provider.isAvailable().
- Around line 47-63: Timer.schedule currently calls switchProvider() on the
timer thread, but switchProvider mutates provider and calls rebuild() which
touches the scene graph via clear()/add(); wrap the callback in
Core.app.post(...) so the provider selection and rebuild run on the app (UI)
thread, add the import for arc.Core, and ensure the scheduled lambda calls
Core.app.post(this::switchProvider) (or posts an inline Runnable that performs
the same provider checks and calls rebuild()) so UI mutations are performed
safely from the app thread; refer to Timer.schedule, switchProvider(), provider,
rebuild(), clear(), and add() when making the change.
---
Outside diff comments:
In `@src/mindustrytool/features/chat/translation/ChatTranslationFeature.java`:
- Around line 119-121: When falling back from an unknown provider
(currentProvider == null) you must persist the fallback so settings stop
containing the stale/legacy ID; after assigning currentProvider =
defaultTranslationProvider in ChatTranslationFeature (use currentProvider and
defaultTranslationProvider), write the default provider's ID into the
settings/config store (e.g. call the project's method that sets the translation
provider id such as
settings.setTranslationProviderId(defaultTranslationProvider.getId()) or
config.setProviderId(...)) and persist it (e.g. settings.save() or
config.persist()) so the migrated value is saved for future loads.
---
Nitpick comments:
In `@src/mindustrytool/features/chat/global/ui/MessageList.java`:
- Around line 165-187: The code calls UserService.findUserById(msg.createdBy)
twice (once to set avatar and again to set header label), causing duplicate
lookups; capture the returned CompletableFuture once into a local variable
(e.g., var userFuture = UserService.findUserById(msg.createdBy)) before the UI
blocks and then call userFuture.thenAccept(...) for both the avatar update
(avatar.clear()/avatar.add(...)) and the header label update inside entry.table
(the Label created for the name), so both UI updates share the same lookup
result and avoid registering two callbacks for the same id.
In `@src/mindustrytool/features/chat/translation/ChatTranslationFeature.java`:
- Around line 93-100: The variable name "formated" is a typo and should be
renamed to "formatted" for clarity; update the local variable assigned from
Strings.format (currently "formated") to "formatted" inside the translation
lambda and update the thenAccept call to use the same "formatted" identifier
(i.e., thenAccept(formatted -> cons.get(formatted)) or the existing consumer
usage) so all references match the new name in ChatTranslationFeature.java.
- Around line 208-209: The disable predicate for the testButton currently relies
on comparing its localized text (the lambda passed to .disabled(...) using
testButton.getText().toString().equals(Core.bundle.get("chat-translation.settings.testing")))
which is brittle; introduce a boolean state field (e.g., testInProgress or
isTesting) in ChatTranslationFeature, change the .disabled(...) predicate to
reference that flag instead of button text, and set that flag to true when the
test starts and false when the test finishes (ensure you update the UI after
toggling the flag so the button re-evaluates its disabled state).
In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java`:
- Around line 89-91: The reset() method in PathfindingDisplay currently clears
spawnPathCache but leaves pathCache populated, causing stale unit paths to
persist after a world load; update reset() to also clear pathCache (i.e., call
pathCache.clear()) so both caches are immediately released on world load and
avoid waiting for the 60-second cleanup.
- Around line 243-268: The fade uses the caller-supplied maxSteps which can
differ from the cached path length; in drawFromCache(PathfindingCache, Color,
int) compute the fade using the actual path length by deriving an effectiveSteps
value from totalSegments (e.g., effectiveSteps = Math.max(1, totalSegments) or
Math.min(maxSteps, totalSegments) depending on desired behavior) and replace
uses of maxSteps in the Draw.color alpha calculation with that effectiveSteps so
the opacity gradient is based on the actual cached segment count; ensure you
handle zero/one segments to avoid division by zero.
In `@src/mindustrytool/features/display/pathfinding/PathfindingSettingsUI.java`:
- Around line 96-104: The UI currently hardcodes the costNames array length
which must match PathfindingConfig.costTypesEnabled; update PathfindingConfig to
expose a stable count (e.g., public static final int COST_TYPE_COUNT or public
static int getCostTypeCount() that returns costTypesEnabled.length) and then
modify PathfindingSettingsUI to build/iterate costNames using that count (or
validate/truncate/extend costNames to that count) so the loop that calls
PathfindingConfig.isCostTypeEnabled(index) and
PathfindingConfig.setCostTypeEnabled(index, c) always aligns with
PathfindingConfig's actual number of cost types.
In `@src/mindustrytool/Utils.java`:
- Around line 45-46: The string-keyed icon cache (iconCache) is still used with
a containsKey/put pattern in icons(String) causing race double-loads; change
icons(String) to use iconCache.computeIfAbsent(key, k -> { ...load
Texture/TextureRegionDrawable... }) so the load path is atomic and only one
Texture is created per key; also apply the same computeIfAbsent pattern for the
other occurrence around lines 214-216 (and for scalableIconCache/scalableIcons
if a similar check-then-put is present) so all cache insertions are done
atomically via computeIfAbsent.
🪄 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
Run ID: 4f612a98-e9b3-46f3-af78-20e80fa65897
📒 Files selected for processing (25)
mod.hjsonsrc/mindustrytool/Main.javasrc/mindustrytool/Utils.javasrc/mindustrytool/features/FeatureMetadata.javasrc/mindustrytool/features/autoplay/tasks/FleeTask.javasrc/mindustrytool/features/browser/schematic/SchematicDialog.javasrc/mindustrytool/features/chat/global/ChatFeature.javasrc/mindustrytool/features/chat/global/ui/MessageList.javasrc/mindustrytool/features/chat/translation/ChatTranslationFeature.javasrc/mindustrytool/features/chat/translation/NoopTranslationProvider.javasrc/mindustrytool/features/display/pathfinding/PathfindingCache.javasrc/mindustrytool/features/display/pathfinding/PathfindingCacheManager.javasrc/mindustrytool/features/display/pathfinding/PathfindingDisplay.javasrc/mindustrytool/features/display/pathfinding/PathfindingSettingsUI.javasrc/mindustrytool/features/display/progress/ProgressDisplay.javasrc/mindustrytool/features/display/quickaccess/QuickAccessHud.javasrc/mindustrytool/features/display/togglerendering/ToggleRenderingFeature.javasrc/mindustrytool/features/display/wavepreview/WavePreviewFeature.javasrc/mindustrytool/features/godmode/GodModeDialogs.javasrc/mindustrytool/features/godmode/GodModeFeature.javasrc/mindustrytool/features/godmode/TapListener.javasrc/mindustrytool/features/settings/FeatureCard.javasrc/mindustrytool/features/smartdrill/SmartDrillFeature.javasrc/mindustrytool/features/smartupgrade/SmartUpgradeFeature.javasrc/mindustrytool/services/TapListener.java
💤 Files with no reviewable changes (2)
- src/mindustrytool/features/chat/translation/NoopTranslationProvider.java
- src/mindustrytool/features/godmode/TapListener.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (15)
mod.hjson (1)
56-56: Version metadata update looks good.The version bump is clear and isolated, with no functional risk in this file.
src/mindustrytool/features/chat/global/ChatFeature.java (1)
148-148: Consistent label spacing improvement looks good.The added symmetric horizontal padding improves alignment/readability across all slider rows without changing behavior.
Also applies to: 167-167, 186-186, 205-205
src/mindustrytool/features/display/pathfinding/PathfindingCache.java (1)
3-10: LGTM!The
@Dataannotation provides useful boilerplate generation. Note that the generatedequals/hashCodewill use array reference comparison fordata, but this is acceptable since instances are stored by explicitlongkeys inPathfindingCacheManagerrather than by object identity.src/mindustrytool/features/display/pathfinding/PathfindingCacheManager.java (1)
6-34: LGTM!The cache manager implementation is clean and efficient:
- Reusing
keysToRemoveavoids repeated allocations during cleanup- Two-phase removal (collect keys, then remove) correctly avoids concurrent modification
- The API is appropriately minimal for its purpose
src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java (4)
36-55: LGTM on the constants and cache managers.The configurable step limits based on unit count and the periodic cleanup mechanism provide good balance between performance and accuracy. The constants are well-documented and the cleanup scheduling (every 60 frames) with 60-second cache ages is reasonable.
110-137: LGTM on draw() method structure.The guard clauses, zoom threshold check, and periodic cleanup scheduling are well-organized. Separating cache cleanup from the drawing logic keeps the frame time predictable.
162-200: LGTM on processUnitPath.The caching strategy with position+type+team composite keys, frame update throttling, and staggered update times (via random offset) effectively prevents thundering herd effects.
324-380: LGTM on spawn path cache updates.The loop detection (line 353-356) and proper termination conditions prevent infinite loops. Dynamic array growth handles edge cases where paths exceed initial capacity.
src/mindustrytool/features/display/pathfinding/PathfindingSettingsUI.java (2)
30-80: LGTM on slider implementations.The zoom and opacity sliders with dynamic labels and appropriate value formatting provide good UX. The stacked layout with disabled touchable content for labels is a clean pattern.
82-91: LGTM on checkbox toggles.Rebuilding the settings UI when spawn-point drawing is toggled ensures the cost-type section visibility stays in sync. This is acceptable for a settings dialog that isn't frequently toggled.
src/mindustrytool/features/chat/global/ui/MessageList.java (1)
153-157: The adjacency checks are solid.Using
Objects.equalshere keeps the grouping null-safe and makes the later spacing branches much easier to follow.src/mindustrytool/features/autoplay/tasks/FleeTask.java (2)
66-66: Nice clarity win here.Passing
smoothandkeepDistanceexplicitly makes the flee behavior much easier to reason about.
37-39: The suggested change is based on a misunderstanding ofUnit.inRange(Position). This method checks againsttype.range, which is the minimum approach range of weapons (often 0), not the maximum weapon range. It does not determine whether a unit "can hit" a target. The premise that a farther enemy with actual weapon range is being ignored is incorrect given the API semantics.> Likely an incorrect or invalid review comment.src/mindustrytool/services/TapListener.java (1)
56-65: No null-check needed one.tile.Mindustry's API guarantees that
TapEvent.tileis non-null by contract: it's declared asfinal Tile tilewith no@Nullableannotation, and the event is only fired when a player taps an actual tile. Other handlers in this codebase (SmartUpgradeFeature, SmartDrillFeature) dereferencee.tiledirectly without null guards, following the same pattern. The code is correct as written.> Likely an incorrect or invalid review comment.src/mindustrytool/features/smartupgrade/SmartUpgradeFeature.java (1)
64-73: Address hold action dispatch ordering.
SmartUpgradeFeatureandSmartDrillFeatureboth register hold listeners with identical parameters (300ms duration, order 10).TapListenerinvokes all listeners whose duration threshold is met in the same update cycle. Both callbacks include acurrentMenu == nullcheck that prevents their menus from being shown simultaneously, but this relies on consistent implementation across all features rather than explicit mutual exclusion inTapListener. If a feature forgets this guard or uses a different order value, overlapping menus become possible. Consider adding explicit exclusivity toTapListener(e.g., consuming holds at the first matching listener, or providing a priority system) to avoid future conflicts as more features register hold actions.
| } catch (Throwable e) { | ||
| ui.showException(e); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether broad Throwable/Error catches exist elsewhere and need review.
rg -nP --type=java -C2 '\bcatch\s*\(\s*(Throwable|Error)\b'Repository: MindustryTool/MindustryToolMod
Length of output: 1756
Narrow the catch type from Throwable to Exception.
Catching Throwable will intercept Error subclasses like OutOfMemoryError and StackOverflowError, which indicate fatal JVM states that should propagate. Since the operations in this block throw standard exceptions, narrowing to Exception preserves the improved error reporting while allowing fatal errors to propagate correctly.
Proposed fix
- } catch (Throwable e) {
+ } catch (Exception e) {
ui.showException(e);
}📝 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.
| } catch (Throwable e) { | |
| ui.showException(e); | |
| } catch (Exception e) { | |
| ui.showException(e); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/browser/schematic/SchematicDialog.java` around
lines 405 - 406, The catch block in SchematicDialog that currently reads "catch
(Throwable e)" should be narrowed to "catch (Exception e)" so fatal JVM Errors
(e.g., OutOfMemoryError, StackOverflowError) are not swallowed; locate the
try/catch containing ui.showException(e) in SchematicDialog.java and replace the
Throwable catch with Exception while keeping the existing ui.showException(e)
call and any surrounding finally/cleanup logic intact.
| String timeStr = msg.createdAt; | ||
| if (msg.createdAt != null) { | ||
| try { | ||
| Instant instant = Instant.parse(msg.createdAt); | ||
| timeStr = DateTimeFormatter.ofPattern("HH:mm") | ||
| .withZone(ZoneId.systemDefault()) | ||
| .format(instant); | ||
| } catch (Throwable err) { | ||
| Log.err(err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Color color = data.getHighestRole() | ||
| .map(r -> { | ||
| try { | ||
| return Color.valueOf(r.getColor()); | ||
| } catch (Exception err) { | ||
| return Color.white; | ||
| } | ||
| }) | ||
| .orElse(Color.white); | ||
|
|
||
| label.setText("[#" + color.toString() + "]" + data.getName() + "[white]" | ||
| + (timeStr.isEmpty() ? "" : " [gray]" + timeStr)); | ||
|
|
||
| Color color = data.getHighestRole() | ||
| .map(r -> { | ||
| try { | ||
| return Color.valueOf(r.getColor()); | ||
| } catch (Exception err) { | ||
| return Color.white; | ||
| } | ||
| }) | ||
| .orElse(Color.white); | ||
|
|
||
| label.setText("[#" + color.toString() + "]" + data.getName() + "[white]" | ||
| + (timeStr.isEmpty() ? "" : " [gray]" + timeStr)); |
There was a problem hiding this comment.
Guard the nullable timestamp before calling isEmpty().
Line 190 already treats msg.createdAt as nullable, but Line 212 still dereferences timeStr. A null createdAt will throw here inside the UI callback.
🐛 Minimal fix
- String timeStr = msg.createdAt;
- if (msg.createdAt != null) {
+ String timeStr = msg.createdAt == null ? "" : msg.createdAt;
+ if (!timeStr.isEmpty()) {
try {
Instant instant = Instant.parse(msg.createdAt);
timeStr = DateTimeFormatter.ofPattern("HH:mm")
.withZone(ZoneId.systemDefault())
.format(instant);📝 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.
| String timeStr = msg.createdAt; | |
| if (msg.createdAt != null) { | |
| try { | |
| Instant instant = Instant.parse(msg.createdAt); | |
| timeStr = DateTimeFormatter.ofPattern("HH:mm") | |
| .withZone(ZoneId.systemDefault()) | |
| .format(instant); | |
| } catch (Throwable err) { | |
| Log.err(err); | |
| } | |
| } | |
| } | |
| Color color = data.getHighestRole() | |
| .map(r -> { | |
| try { | |
| return Color.valueOf(r.getColor()); | |
| } catch (Exception err) { | |
| return Color.white; | |
| } | |
| }) | |
| .orElse(Color.white); | |
| label.setText("[#" + color.toString() + "]" + data.getName() + "[white]" | |
| + (timeStr.isEmpty() ? "" : " [gray]" + timeStr)); | |
| Color color = data.getHighestRole() | |
| .map(r -> { | |
| try { | |
| return Color.valueOf(r.getColor()); | |
| } catch (Exception err) { | |
| return Color.white; | |
| } | |
| }) | |
| .orElse(Color.white); | |
| label.setText("[#" + color.toString() + "]" + data.getName() + "[white]" | |
| + (timeStr.isEmpty() ? "" : " [gray]" + timeStr)); | |
| String timeStr = msg.createdAt == null ? "" : msg.createdAt; | |
| if (!timeStr.isEmpty()) { | |
| try { | |
| Instant instant = Instant.parse(msg.createdAt); | |
| timeStr = DateTimeFormatter.ofPattern("HH:mm") | |
| .withZone(ZoneId.systemDefault()) | |
| .format(instant); | |
| } catch (Throwable err) { | |
| Log.err(err); | |
| } | |
| } | |
| Color color = data.getHighestRole() | |
| .map(r -> { | |
| try { | |
| return Color.valueOf(r.getColor()); | |
| } catch (Exception err) { | |
| return Color.white; | |
| } | |
| }) | |
| .orElse(Color.white); | |
| label.setText("[#" + color.toString() + "]" + data.getName() + "[white]" | |
| (timeStr.isEmpty() ? "" : " [gray]" + timeStr)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/chat/global/ui/MessageList.java` around lines 189
- 212, The label.setText call can NPE because timeStr may be null when
msg.createdAt is null; in MessageList.java (around the block that builds timeStr
and calls label.setText) ensure timeStr is non-null before calling isEmpty() —
either initialize timeStr to "" instead of msg.createdAt or change the
concatenation to check timeStr != null && !timeStr.isEmpty() before appending
the time. Update the code paths in the Instant.parse try/catch and the fallback
to guarantee timeStr is a safe (possibly empty) string used by label.setText.
| }).growX().padTop(isSameUser ? 2 * scale : 0).padBottom(0).row(); | ||
| } | ||
| } | ||
|
|
||
| card.table(c -> renderContent(c, msg.content, scale)).top().left().growX().padTop(6 * scale); | ||
| card.table(c -> renderContent(c, msg.content, scale)).top().left().growX() | ||
| .padTop(isSameUser && (msg.replyTo == null || msg.replyTo.isEmpty()) ? 2 * scale : 0); |
There was a problem hiding this comment.
Base the grouped top gap on whether a reply preview was actually rendered.
Line 231 only adds the reply row when the target message is present, but Line 236 keys off replyTo alone. If the replied message is outside the loaded window, this message loses the fallback isSameUser spacing and collapses into the previous bubble.
🩹 Possible fix
+ boolean hasRenderedReply = false;
if (msg.replyTo != null && !msg.replyTo.isEmpty()) {
ChatMessage repliedMsg = channelMsgs.find(m -> m.id.equals(msg.replyTo));
if (repliedMsg != null) {
+ hasRenderedReply = true;
card.table(replyTable -> {
replyTable.center().left();
replyTable.image(Icon.rightSmall).size(16 * scale).padRight(4 * scale).color(Color.gray);
@@
}
card.table(c -> renderContent(c, msg.content, scale)).top().left().growX()
- .padTop(isSameUser && (msg.replyTo == null || msg.replyTo.isEmpty()) ? 2 * scale : 0);
+ .padTop(isSameUser && !hasRenderedReply ? 2 * scale : 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/chat/global/ui/MessageList.java` around lines 231
- 236, The grouped-top gap is currently decided from msg.replyTo but the reply
preview row is only added when the target message exists, causing incorrect
spacing; modify MessageList rendering so you track whether the reply preview was
actually rendered (e.g., set a boolean like replyRendered when you add the reply
row in the block that calls card.table(...).row()) and then use that boolean
instead of msg.replyTo when computing padTop for the content card in
renderContent/card.table, ensuring padTop uses isSameUser && !replyRendered (or
the inverse as appropriate) to preserve fallback spacing when the reply target
is missing.
| settingsDialog.buttons.button("@reset", Icon.refresh, () -> { | ||
| PathfindingConfig.setZoomThreshold(0.5f); | ||
| rebuildSettings(); | ||
| }).size(250, 64); |
There was a problem hiding this comment.
Reset button only resets zoom threshold, not other settings.
Users clicking "Reset" would likely expect all settings (opacity, draw toggles, cost types) to be restored to defaults. Currently, only zoomThreshold is reset to 0.5f.
Proposed fix to reset all settings
settingsDialog.buttons.button("@reset", Icon.refresh, () -> {
PathfindingConfig.setZoomThreshold(0.5f);
+ PathfindingConfig.setOpacity(1f);
+ PathfindingConfig.setDrawUnitPath(true);
+ PathfindingConfig.setDrawSpawnPointPath(true);
+ for (int i = 0; i < 6; i++) {
+ PathfindingConfig.setCostTypeEnabled(i, true);
+ }
rebuildSettings();
}).size(250, 64);Alternatively, consider adding a PathfindingConfig.resetToDefaults() method to centralize default values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/display/pathfinding/PathfindingSettingsUI.java`
around lines 22 - 25, The Reset button in PathfindingSettingsUI currently only
calls PathfindingConfig.setZoomThreshold(0.5f) so add logic to restore all
settings to their defaults when the user clicks the button: either implement and
call a new PathfindingConfig.resetToDefaults() that sets opacity, draw toggles,
cost types, zoomThreshold, etc. back to their default values, or update the
lambda in settingsDialog.buttons.button(...) to explicitly set each config value
(e.g., opacity, drawFlags, costType, zoomThreshold) back to defaults and then
call rebuildSettings(); ensure you modify PathfindingSettingsUI and/or
PathfindingConfig classes and reference the same public setters/getters used
elsewhere so the UI reflects the reset.
| btnRef[0] = t | ||
| .button(b -> b.image(Utils.scalable(Icon.settings)).scaling(Scaling.fit), Styles.clearNonei, () -> { | ||
| Main.featureSettingDialog.show(); | ||
| }) | ||
| .size(buttonSize) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file to understand the context better
cd src/mindustrytool/features/display/quickaccess && wc -l QuickAccessHud.javaRepository: MindustryTool/MindustryToolMod
Length of output: 97
🏁 Script executed:
# Check the relevant section of the file
cd src/mindustrytool/features/display/quickaccess && sed -n '190,210p' QuickAccessHud.javaRepository: MindustryTool/MindustryToolMod
Length of output: 656
🏁 Script executed:
# Look for imports related to Arc UI to understand the framework
cd src/mindustrytool/features/display/quickaccess && grep -E "^import.*arc|^import.*Scaling" QuickAccessHud.java | head -20Repository: MindustryTool/MindustryToolMod
Length of output: 555
🏁 Script executed:
# Search for other button definitions in the codebase to see patterns
rg "\.button\(" --type java -A 2 | head -80Repository: MindustryTool/MindustryToolMod
Length of output: 6193
🏁 Script executed:
# Search for Arc Image or Scaling documentation/examples in the codebase
rg -i "scaling\.(fit|fill|stretch)" --type java -B 2 -A 2 | head -100Repository: MindustryTool/MindustryToolMod
Length of output: 7930
🏁 Script executed:
# Look at the SmartUpgradeFeature button more closely
sed -n '1,50p' src/mindustrytool/features/smartupgrade/SmartUpgradeFeature.java | grep -A 5 "button"Repository: MindustryTool/MindustryToolMod
Length of output: 56
🌐 Web query:
Arc framework Java UI image scaling behavior Scaling.fit bounds
💡 Result:
In Arc (Mindustry) Scene2D, Image scales its Drawable inside the actor’s bounds using Scaling.apply(sourceW, sourceH, targetW, targetH) during layout(), then draws it at (x + imageX, y + imageY) with size (imageWidth, imageHeight). The source size is drawable.getMinWidth()/getMinHeight() and the target size is the Image element’s current getWidth()/getHeight() (its bounds). The position is then aligned (left/right/center, top/bottom/center). Note imageX/imageY are cast to int, so there’s pixel rounding. [2]
Scaling.fit behavior (the “bounds” part)
Scaling.fit keeps aspect ratio and chooses the smaller uniform scale so the whole source fits within the target bounds:
scale = (targetRatio > sourceRatio) ? targetWidth/sourceWidth : targetHeight/sourceHeight- result size =
(sourceWidth * scale, sourceHeight * scale)
So with fit, the image will be fully contained in the Image bounds, and will often be smaller than the bounds in one dimension (letterboxed/pillarboxed depending on aspect ratios). [1]
Related: Scaling.bounded
Scaling.bounded is “fit but only shrink”: if the source is already smaller than the target bounds, it returns none (no scaling); otherwise it behaves like fit. [1]
Sources: arc.util.Scaling enum + implementation [1], arc.scene.ui.Image layout/draw logic [2].
🏁 Script executed:
# Check if there are any tests or documentation about Arc button image sizing
find . -name "*.md" -o -name "*.txt" | xargs grep -l "image\|button\|scaling" 2>/dev/null | head -10Repository: MindustryTool/MindustryToolMod
Length of output: 56
🏁 Script executed:
# Look at the entire button setup in context - check what comes before and after
sed -n '185,210p' src/mindustrytool/features/display/quickaccess/QuickAccessHud.javaRepository: MindustryTool/MindustryToolMod
Length of output: 776
Restore an explicit size for the settings image.
Lines 197-200 size the button, but not the image cell. With Arc's Scaling.fit, the icon is only fit inside the bounds it already has. Consider adding an explicit size to the image so it scales properly with buttonSize.
Suggested fix
btnRef[0] = t
- .button(b -> b.image(Utils.scalable(Icon.settings)).scaling(Scaling.fit), Styles.clearNonei, () -> {
+ .button(
+ b -> b.image(Utils.scalable(Icon.settings)).scaling(Scaling.fit).size(buttonSize * 0.7f),
+ Styles.clearNonei, () -> {
Main.featureSettingDialog.show();
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/display/quickaccess/QuickAccessHud.java` around
lines 196 - 200, The image cell inside the settings button isn't explicitly
sized so Utils.scalable(Icon.settings) with Scaling.fit only fits to the image's
intrinsic bounds; update the button builder where btnRef[0] is created (the
.button(...) call using Utils.scalable(Icon.settings), Styles.clearNonei and
buttonSize) to set the image cell size to the same buttonSize (or a derived
value) before applying Scaling.fit so the icon scales to the button bounds;
ensure the change targets the image actor/cell used in that button creation so
Main.featureSettingDialog.show() behavior stays the same.
| public Builder icon(TextureRegionDrawable icon) { | ||
| this.icon = Utils.scalable(icon); | ||
| return this; |
There was a problem hiding this comment.
Preserve the builder's null validation path.
icon(null) now fails inside Utils.scalable(...), so callers no longer get the explicit "Icon is required" error from build(). A null guard here keeps the builder behavior consistent.
💡 Suggested fix
- public Builder icon(TextureRegionDrawable icon) {
- this.icon = Utils.scalable(icon);
+ public Builder icon(TextureRegionDrawable icon) {
+ this.icon = icon == null ? null : Utils.scalable(icon);
return this;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/FeatureMetadata.java` around lines 71 - 73, The
Builder.icon method currently calls Utils.scalable(icon) unconditionally which
breaks the builder's null validation path; change
Builder.icon(TextureRegionDrawable icon) to guard against null by not invoking
Utils.scalable when icon is null (i.e., set this.icon = null or leave unchanged)
so that null inputs still reach the Builder.build() null check and throw the
original "Icon is required" error; update the Builder.icon method (and any
related field) to preserve the original validation flow rather than performing
scaling eagerly in Utils.scalable.
| Timer.schedule(this::switchProvider, 60, 60); | ||
| } | ||
|
|
||
| private void switchProvider() { | ||
| if (provider != null && provider.isAvailable()) { | ||
| return; | ||
| } | ||
|
|
||
| if (js.isAvailable()) { | ||
| provider = js; | ||
| } else if (internal.isAvailable()) { | ||
| provider = internal; | ||
| } else { | ||
| provider = null; | ||
| } | ||
|
|
||
| rebuild(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the GodModeFeature.java file
find . -name "GodModeFeature.java" -type fRepository: MindustryTool/MindustryToolMod
Length of output: 130
🏁 Script executed:
# Find TeamResourceFeature.java for comparison
find . -name "TeamResourceFeature.java" -type fRepository: MindustryTool/MindustryToolMod
Length of output: 148
🏁 Script executed:
# Check repository structure
git ls-files | head -20Repository: MindustryTool/MindustryToolMod
Length of output: 754
🏁 Script executed:
# Read GodModeFeature.java
cat -n ./src/mindustrytool/features/godmode/GodModeFeature.javaRepository: MindustryTool/MindustryToolMod
Length of output: 4477
🏁 Script executed:
# Read TeamResourceFeature.java to see the referenced pattern
cat -n ./src/mindustrytool/features/display/teamresource/TeamResourceFeature.java | head -100Repository: MindustryTool/MindustryToolMod
Length of output: 3886
🏁 Script executed:
# Search for Core.app.post usage in the codebase
rg "Core\.app\.post" --context 3Repository: MindustryTool/MindustryToolMod
Length of output: 50386
🏁 Script executed:
# Check if arc.Core is already imported in GodModeFeature.java
grep -n "import.*Core" ./src/mindustrytool/features/godmode/GodModeFeature.javaRepository: MindustryTool/MindustryToolMod
Length of output: 56
🏁 Script executed:
# Verify Timer.schedule threading behavior by checking Arc version/documentation mentions
rg "Timer\.schedule" --context 2 -A 2 | head -50Repository: MindustryTool/MindustryToolMod
Length of output: 3479
🏁 Script executed:
# Verify Timer.schedule method signature to confirm float vs int usage
rg "class Timer" -A 5 | head -30Repository: MindustryTool/MindustryToolMod
Length of output: 56
🏁 Script executed:
# Check if any Timer.schedule calls are already using Core.app.post pattern
rg "Timer\.schedule.*Core\.app\.post"Repository: MindustryTool/MindustryToolMod
Length of output: 56
Route the timer callback through the app thread.
Line 47 schedules switchProvider() directly via timer. That method mutates provider and calls rebuild(), which modifies the scene graph via clear() and add(). The codebase already applies Core.app.post() for UI mutations in asynchronous contexts (see TeamResourceFeature.java line 80), so this path should follow the same pattern for thread safety.
Suggested fix
- Timer.schedule(this::switchProvider, 60, 60);
+ Timer.schedule(() -> Core.app.post(this::switchProvider), 60f, 60f);Add import arc.Core; at the top of the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/godmode/GodModeFeature.java` around lines 47 - 63,
Timer.schedule currently calls switchProvider() on the timer thread, but
switchProvider mutates provider and calls rebuild() which touches the scene
graph via clear()/add(); wrap the callback in Core.app.post(...) so the provider
selection and rebuild run on the app (UI) thread, add the import for arc.Core,
and ensure the scheduled lambda calls Core.app.post(this::switchProvider) (or
posts an inline Runnable that performs the same provider checks and calls
rebuild()) so UI mutations are performed safely from the app thread; refer to
Timer.schedule, switchProvider(), provider, rebuild(), clear(), and add() when
making the change.
| private void switchProvider() { | ||
| if (provider != null && provider.isAvailable()) { | ||
| return; | ||
| } | ||
|
|
||
| if (js.isAvailable()) { | ||
| provider = js; | ||
| } else if (internal.isAvailable()) { | ||
| provider = internal; | ||
| } else { | ||
| provider = null; | ||
| } |
There was a problem hiding this comment.
Recompute from useJS instead of making the fallback sticky.
The new early-return path makes the first fallback provider permanent. Example: useJS == true, JS drops out, provider falls back to internal, then JS comes back; the next refresh returns early because internal is still available, so the feature never switches back to the provider selected in settings.
Suggested fix
private void switchProvider() {
- if (provider != null && provider.isAvailable()) {
- return;
- }
-
- if (js.isAvailable()) {
- provider = js;
- } else if (internal.isAvailable()) {
- provider = internal;
- } else {
- provider = null;
- }
+ GodModeProvider preferred = useJS ? js : internal;
+ GodModeProvider fallback = useJS ? internal : js;
+
+ GodModeProvider next = preferred.isAvailable()
+ ? preferred
+ : fallback.isAvailable() ? fallback : null;
+
+ if (provider == next) {
+ return;
+ }
+
+ provider = next;
rebuild();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/godmode/GodModeFeature.java` around lines 50 - 61,
The current switchProvider() makes a fallback sticky by early-returning when any
available provider is already set; change it to recompute from the user
preference (useJS) instead of honoring the previous provider blindly: in
switchProvider(), determine the preferred provider using useJS (preferred =
useJS ? js : internal), then if preferred.isAvailable() set provider =
preferred; else if the other provider is available set provider = other;
otherwise set provider = null; remove the existing early-return that only checks
provider.isAvailable().
Move path calculation and cache updates from draw method to new update method triggered by update event. This reduces per-frame workload in the render thread and improves performance by processing path updates independently of rendering. Add separate team tracking sets for update and draw phases to prevent concurrent modification issues. Remove unused currentMaxSteps field and rename methods to clarify their purpose (drawUnitPath → drawUnitPaths, etc.).
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/mindustrytool/features/display/pathfinding/PathfindingDisplay.java (1)
309-314:⚠️ Potential issue | 🟡 MinorOpacity calculation may produce negative values when unit count increases.
The opacity calculation
(1f - ((float) i / maxSteps))assumes the cache was built with the samemaxSteps. However,maxStepsvaries based on currenttotalUnits, which can change between cache creation and drawing. If unit count increases (loweringmaxSteps), segments beyondmaxStepswill have negative opacity factors.Consider using the actual segment count for opacity calculation:
Proposed fix
private void drawFromCache(PathfindingCache cacheEntry, Color pathColor, int maxSteps) { if (cacheEntry.size < 4) { return; } Lines.stroke(1f); float currentX = cacheEntry.data[0]; float currentY = cacheEntry.data[1]; int totalSegments = (cacheEntry.size / 2) - 1; if (totalSegments <= 0) { return; } for (int i = 0; i < totalSegments; i++) { float nextX = cacheEntry.data[(i + 1) * 2]; float nextY = cacheEntry.data[(i + 1) * 2 + 1]; - Draw.color(pathColor, (1f - ((float) i / maxSteps)) * currentOpacity); + Draw.color(pathColor, (1f - ((float) i / totalSegments)) * currentOpacity); Lines.line(currentX, currentY, nextX, nextY); currentX = nextX; currentY = nextY; } Draw.reset(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java` around lines 309 - 314, The opacity calculation inside PathfindingDisplay (in the loop using totalSegments, cacheEntry.data and maxSteps) can go negative when maxSteps differs from the cached path length; change the opacity denominator to use the actual drawn segment count (e.g., compute float steps = Math.max(1, totalSegments - 1) or similar based on totalSegments) and compute factor = MathUtils.clamp(1f - ((float)i / steps), 0f, 1f) before multiplying by currentOpacity and passing to Draw.color(pathColor, ...); this ensures no negative opacity and avoids division by zero while keeping the fade relative to the displayed segments.
🧹 Nitpick comments (2)
src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java (2)
438-441: Consider usingLog.warninstead ofLog.errfor loop detection.Loop detection in pathfinding may occur as a normal edge case (e.g., when flowfields are being recalculated or at map boundaries). Using
Log.errsuggests a critical error, which could be misleading and potentially spammy in logs.Proposed fix
if (nextTile == currentTile) { - Log.err("Pathfinder loop detected"); + Log.warn("Pathfinder loop detected at tile @, @", currentTile.x, currentTile.y); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java` around lines 438 - 441, In PathfindingDisplay.java inside the path traversal loop (the block checking if nextTile == currentTile), replace the call to Log.err("Pathfinder loop detected") with Log.warn(...) so loop detection is logged as a warning rather than an error; preserve the same message (or add brief context) and keep the break to stop the loop. This targets the nextTile == currentTile check in the pathfinding method to reduce noisy/critical error logs for expected edge cases.
202-251: Consider extracting duplicated maxSteps calculation.The
maxStepscalculation logic (lines 206-207) is duplicated fromupdateUnitPaths()(lines 156-157). While not a bug, extracting this to a helper method would improve maintainability.Suggested refactor
+ private int calculateMaxSteps(int totalUnits) { + return (totalUnits > 2000) ? MAX_STEPS_VERY_HIGH + : (totalUnits > 1000) ? MAX_STEPS_HIGH + : (totalUnits > 500) ? MAX_STEPS_MEDIUM : MAX_STEPS_LOW; + } + private void updateUnitPaths() { int totalUnits = Groups.unit.size(); - int maxSteps = (totalUnits > 2000) ? MAX_STEPS_VERY_HIGH - : (totalUnits > 1000) ? MAX_STEPS_HIGH : (totalUnits > 500) ? MAX_STEPS_MEDIUM : MAX_STEPS_LOW; + int maxSteps = calculateMaxSteps(totalUnits); // ... rest of method } private void drawUnitPaths() { // ... int totalUnits = Groups.unit.size(); - int maxSteps = (totalUnits > 2000) ? MAX_STEPS_VERY_HIGH - : (totalUnits > 1000) ? MAX_STEPS_HIGH : (totalUnits > 500) ? MAX_STEPS_MEDIUM : MAX_STEPS_LOW; + int maxSteps = calculateMaxSteps(totalUnits); // ... rest of method }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java` around lines 202 - 251, The maxSteps calculation in drawUnitPaths() is duplicated from updateUnitPaths(); extract it into a small helper method (e.g., getMaxStepsForTotal(int totalUnits) or computeMaxSteps) that returns the appropriate MAX_STEPS_* constant based on totalUnits, then replace the inline ternary in drawUnitPaths() and the one in updateUnitPaths() to call this helper; update references to unique symbols drawUnitPaths(), updateUnitPaths(), and the MAX_STEPS_* constants to use the new helper for maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java`:
- Around line 116-120: The spawn-path cache cleanup window is equal to its
update interval so newly created spawn entries can be removed before they’re
next refreshed; update the constant CACHE_CLEANUP_AGE_SPAWN to a value
significantly larger than CACHE_UPDATE_INTERVAL_SPAWN (e.g., change
CACHE_CLEANUP_AGE_SPAWN from 60f to 120f or more) in PathfindingDisplay
(affecting spawnPathCache.cleanup) so spawn entries have a buffer and won't be
immediately purged between create/update cycles.
---
Outside diff comments:
In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java`:
- Around line 309-314: The opacity calculation inside PathfindingDisplay (in the
loop using totalSegments, cacheEntry.data and maxSteps) can go negative when
maxSteps differs from the cached path length; change the opacity denominator to
use the actual drawn segment count (e.g., compute float steps = Math.max(1,
totalSegments - 1) or similar based on totalSegments) and compute factor =
MathUtils.clamp(1f - ((float)i / steps), 0f, 1f) before multiplying by
currentOpacity and passing to Draw.color(pathColor, ...); this ensures no
negative opacity and avoids division by zero while keeping the fade relative to
the displayed segments.
---
Nitpick comments:
In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java`:
- Around line 438-441: In PathfindingDisplay.java inside the path traversal loop
(the block checking if nextTile == currentTile), replace the call to
Log.err("Pathfinder loop detected") with Log.warn(...) so loop detection is
logged as a warning rather than an error; preserve the same message (or add
brief context) and keep the break to stop the loop. This targets the nextTile ==
currentTile check in the pathfinding method to reduce noisy/critical error logs
for expected edge cases.
- Around line 202-251: The maxSteps calculation in drawUnitPaths() is duplicated
from updateUnitPaths(); extract it into a small helper method (e.g.,
getMaxStepsForTotal(int totalUnits) or computeMaxSteps) that returns the
appropriate MAX_STEPS_* constant based on totalUnits, then replace the inline
ternary in drawUnitPaths() and the one in updateUnitPaths() to call this helper;
update references to unique symbols drawUnitPaths(), updateUnitPaths(), and the
MAX_STEPS_* constants to use the new helper for maintainability.
🪄 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
Run ID: d0db82b7-c54b-409f-a77b-1db5da6d0201
📒 Files selected for processing (1)
src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java
📜 Review details
🔇 Additional comments (8)
src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java (8)
37-67: LGTM! Well-structured constants and fields.The separation of
updateActiveTeamsanddrawActiveTeamscorrectly avoids potential concurrent modification issues between the update and draw phases. The cache timing constants are appropriately tuned for different update frequencies between unit paths (15f) and spawn paths (60f).
81-93: LGTM!The event registration correctly separates update logic from draw logic. The
reset()method appropriately only clears spawn path cache on world load since unit path caches are position-dependent and will naturally be invalidated.
131-152: LGTM!Good optimization to cache
currentOpacityonce per draw call instead of querying the config repeatedly. The early-exit on zoom threshold is an efficient culling strategy.
154-200: LGTM!The per-frame update limiting (
MAX_UPDATES_PER_FRAME) and camera-based culling are good performance optimizations. The random jitter added tolastUpdateTime(line 196) effectively distributes cache refresh across frames to avoid update spikes.
253-292: LGTM!The bounds checking (
dataIndex >= cacheEntry.data.length - 2) correctly ensures space for the next coordinate pair. The self-loop detection (nextTile == currentTile) properly handles both reaching the destination and potential pathfinding edge cases.
322-363: LGTM!The cache key composition correctly encodes spawn position, cost type, and team ID into a unique long key. The random jitter on
lastUpdateTimespreads updates across frames effectively.
365-407: LGTM!The draw method correctly updates
lastUsedTimeto prevent cache cleanup of actively rendered paths. Using a separatedrawActiveTeamsset avoids concurrent modification with the update phase.
467-478: LGTM!The spawn path rendering correctly iterates through cached coordinate pairs. Using uniform opacity (unlike the gradient fade for unit paths) is a sensible design choice to clearly show the full spawn-to-core route.
| if (timer.get(TIMER_CLEANUP, CLEANUP_SCHEDULE_FRAMES)) { | ||
| float time = Time.time; | ||
| pathCache.cleanup(time, CACHE_CLEANUP_AGE_UNIT); | ||
| spawnPathCache.cleanup(time, CACHE_CLEANUP_AGE_SPAWN); | ||
| } |
There was a problem hiding this comment.
Spawn path cache entries may be cleaned up before they can be refreshed.
CACHE_CLEANUP_AGE_SPAWN (60f) equals CACHE_UPDATE_INTERVAL_SPAWN (60f). If a spawn path entry is not drawn (and thus lastUsedTime not updated) between update cycles, it could be cleaned up immediately after creation, causing repeated cache misses and recalculations.
Consider increasing CACHE_CLEANUP_AGE_SPAWN to be significantly larger than CACHE_UPDATE_INTERVAL_SPAWN (e.g., 120f or more) to provide a buffer.
Proposed fix
- private static final float CACHE_CLEANUP_AGE_SPAWN = 60f;
+ private static final float CACHE_CLEANUP_AGE_SPAWN = 180f;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/display/pathfinding/PathfindingDisplay.java`
around lines 116 - 120, The spawn-path cache cleanup window is equal to its
update interval so newly created spawn entries can be removed before they’re
next refreshed; update the constant CACHE_CLEANUP_AGE_SPAWN to a value
significantly larger than CACHE_UPDATE_INTERVAL_SPAWN (e.g., change
CACHE_CLEANUP_AGE_SPAWN from 60f to 120f or more) in PathfindingDisplay
(affecting spawnPathCache.cleanup) so spawn entries have a buffer and won't be
immediately purged between create/update cycles.
Add a scheduled task that checks every second if the user is hosting a server and simultaneously joining another one as a client. When both conditions are met, a notification is shown and the local room is automatically closed to prevent conflicts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mindustrytool/features/playerconnect/PlayerConnect.java (1)
66-66: Use a bundle key instead of hard-coded UI text.Line 66 introduces a literal English string, which makes localization harder and inconsistent with existing
@bundle.keyusage in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/features/playerconnect/PlayerConnect.java` at line 66, The literal English string passed to Vars.ui.showInfoFade should be replaced with a bundle key lookup to support localization: change the showInfoFade call in PlayerConnect.java to use the bundle lookup (e.g., Core.bundle.get or the existing project bundle helper) with a new key like "playerconnect.auto_close_on_join" instead of the hard-coded text, and add that key and its English value to the project's bundle properties file(s) so translations can be provided consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mindustrytool/features/playerconnect/PlayerConnect.java`:
- Around line 63-68: The scheduled task calling close() every second can
re-enter while the disconnect is still in progress; modify the logic in the
Timer.schedule block (the isHosting() && Vars.net.client() check) to be one-shot
by introducing a guarded flag (e.g., autoCloseTriggered) or by checking the room
connection state via NetworkProxy (e.g.,
room.isConnected()/NetworkProxy.closeRoom() state) before calling close(), set
the flag immediately before invoking close() so subsequent timer ticks
early-return, and clear/reset it only when the room is fully disconnected if
needed to allow future auto-closes.
---
Nitpick comments:
In `@src/mindustrytool/features/playerconnect/PlayerConnect.java`:
- Line 66: The literal English string passed to Vars.ui.showInfoFade should be
replaced with a bundle key lookup to support localization: change the
showInfoFade call in PlayerConnect.java to use the bundle lookup (e.g.,
Core.bundle.get or the existing project bundle helper) with a new key like
"playerconnect.auto_close_on_join" instead of the hard-coded text, and add that
key and its English value to the project's bundle properties file(s) so
translations can be provided consistently.
🪄 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
Run ID: 3eb91b3c-73bb-4d34-8816-c7415c631e0b
📒 Files selected for processing (1)
src/mindustrytool/features/playerconnect/PlayerConnect.java
| Timer.schedule(() -> { | ||
| if (isHosting() && Vars.net.client()) { | ||
| close(); | ||
| Vars.ui.showInfoFade("Auto close room when join another server"); | ||
| } | ||
| }, 1, 1); |
There was a problem hiding this comment.
Prevent repeated auto-close re-entry while disconnect is still in progress.
At Line 63, this task runs every second and can invoke close() multiple times before room.isConnected() flips false. Given NetworkProxy.closeRoom() (src/mindustrytool/features/playerconnect/NetworkProxy.java, Lines 136-144) sends a closure packet when still connected, this can cause duplicate close requests and repeated toasts.
Suggested guard to make auto-close one-shot per conflict window
@@
- private static boolean isShowingDialog = false;
+ private static boolean isShowingDialog = false;
+ private static boolean autoCloseTriggered = false;
@@
- Timer.schedule(() -> {
- if (isHosting() && Vars.net.client()) {
- close();
- Vars.ui.showInfoFade("Auto close room when join another server");
- }
- }, 1, 1);
+ Timer.schedule(() -> {
+ boolean conflictingSession = isHosting() && Vars.net.client();
+ if (conflictingSession && !autoCloseTriggered) {
+ autoCloseTriggered = true;
+ close();
+ Vars.ui.showInfoFade("@playerconnect.auto-close-room");
+ } else if (!conflictingSession) {
+ autoCloseTriggered = false;
+ }
+ }, 1f, 1f);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/playerconnect/PlayerConnect.java` around lines 63
- 68, The scheduled task calling close() every second can re-enter while the
disconnect is still in progress; modify the logic in the Timer.schedule block
(the isHosting() && Vars.net.client() check) to be one-shot by introducing a
guarded flag (e.g., autoCloseTriggered) or by checking the room connection state
via NetworkProxy (e.g., room.isConnected()/NetworkProxy.closeRoom() state)
before calling close(), set the flag immediately before invoking close() so
subsequent timer ticks early-return, and clear/reset it only when the room is
fully disconnected if needed to allow future auto-closes.
Previously, the condition incorrectly filtered out string icons by checking `icon instanceof String`. This was causing valid string-based icons to be excluded from the browser. The fix removes the string check, allowing both Character and String icons to be displayed correctly based on the name filter.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mindustrytool/IconUtils.java (1)
28-30: Silently swallowing exceptions hides potential issues.Catching
Exceptionbroadly and returningnullmakes debugging difficult if reflection fails for unexpected reasons (e.g., security restrictions, field access issues). Consider logging at debug level or narrowing the catch toIllegalAccessException.Proposed fix to improve exception handling
} catch (Exception e) { + // Log reflection failures for debugging + arc.util.Log.debug("Failed to read Iconc field @: @", f.getName(), e.getMessage()); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mindustrytool/IconUtils.java` around lines 28 - 30, In IconUtils, replace the broad catch(Exception e) that swallows reflection failures with a narrower catch (IllegalAccessException) (and optionally NoSuchFieldException if field lookup is performed) and log the caught exception at debug/trace level via your logger instead of returning null silently; update the method that performs the reflective field access in IconUtils to rethrow or return a meaningful fallback only after logging the exception and avoid hiding other runtime exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mindustrytool/features/settings/IconBrowserDialog.java`:
- Around line 36-37: The button label and clipboard text use
String.valueOf(icon.getValue()) which will produce the string "null" if
icon.getValue() is null; update the IconBrowserDialog button creation (the
lambda and label expression using icon.getValue() and icon.getName()) to guard
against a null Character from icon.getValue() (e.g., choose a safe default like
an empty string or placeholder char when icon.getValue() == null) and use that
safe string both for the displayed label and the argument passed to
Core.app.setClipboardText; note IconUtils already filters nulls but add this
defensive check around icon.getValue() to prevent "null" showing in the UI.
---
Nitpick comments:
In `@src/mindustrytool/IconUtils.java`:
- Around line 28-30: In IconUtils, replace the broad catch(Exception e) that
swallows reflection failures with a narrower catch (IllegalAccessException) (and
optionally NoSuchFieldException if field lookup is performed) and log the caught
exception at debug/trace level via your logger instead of returning null
silently; update the method that performs the reflective field access in
IconUtils to rethrow or return a meaningful fallback only after logging the
exception and avoid hiding other runtime exceptions.
🪄 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
Run ID: 375f52f1-bce9-48ca-b058-7bc94016b406
📒 Files selected for processing (2)
src/mindustrytool/IconUtils.javasrc/mindustrytool/features/settings/IconBrowserDialog.java
📜 Review details
🔇 Additional comments (3)
src/mindustrytool/IconUtils.java (2)
12-17: LGTM on the data class design.Using Lombok's
@Dataand@RequiredArgsConstructoris appropriate for this simple immutable value holder with two fields.
19-31: Commit message claims to support both Character and String icons, but implementation only accepts Character types.The commit message states: "removed the incorrect
icon instanceof Stringcheck so string-based icons are not filtered out," implying both types should be supported. However, line 24 only keepsCharactervalues, and theIconCclass definesvalueasCharacter, not a String or union type.This discrepancy suggests either:
- The commit message is misleading (if
Iconccontains no String fields)- The implementation is incomplete (if
Iconccontains String fields that should be included)Verify against the Mindustry library's
Iconcclass to confirm what field types exist and whether String-type icons should genuinely be supported.src/mindustrytool/features/settings/IconBrowserDialog.java (1)
31-51: Clean refactoring using centralized icon utility.The iteration over
IconUtils.iconcswith case-insensitive name filtering is clean and readable. The UI building logic is correct.
| containers.button(String.valueOf(icon.getValue()) + " " + icon.getName(), () -> { | ||
| Core.app.setClipboardText(String.valueOf(icon.getValue())); |
There was a problem hiding this comment.
Minor: icon.getValue() could be null if IconC allows it.
The IconC.value field is declared as Character (nullable wrapper), and String.valueOf(null) produces "null". If a malformed entry slips through, this could display "null" in the UI. The current IconUtils filters nulls at construction, so this is low risk but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mindustrytool/features/settings/IconBrowserDialog.java` around lines 36 -
37, The button label and clipboard text use String.valueOf(icon.getValue())
which will produce the string "null" if icon.getValue() is null; update the
IconBrowserDialog button creation (the lambda and label expression using
icon.getValue() and icon.getName()) to guard against a null Character from
icon.getValue() (e.g., choose a safe default like an empty string or placeholder
char when icon.getValue() == null) and use that safe string both for the
displayed label and the argument passed to Core.app.setClipboardText; note
IconUtils already filters nulls but add this defensive check around
icon.getValue() to prevent "null" showing in the UI.
Add null check before instanceof to handle cases where static fields may be null during reflection access.
1 participant
Set provider to null when neither JS nor internal providers are available to prevent potential null pointer exceptions.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores