Conversation
Every failsafe duplicated the same `isScriptRunning() || isDevelopmentEnvironment` check as the first line of its event handler. Hoist it into a single `Failsafe.shouldReactToEvents()` so the condition has one source of truth.
The codebase is currently detekt-clean (0 findings after the failsafe guard-clause dedup), so there is no longer a backlog blocking this. Restores failOnSeverity = Warning so future violations get caught by `check` instead of accumulating silently.
The HTTP InputStream was never closed, leaking a connection on every remote
font/image fetch. UpdateChecker already closes the same getInputStream()
result correctly via .use{} -- apply the same pattern here. Also drops the
runBlocking wrapper around what was already a blocking call.
ModuleManager.modules, AddonManager.addons, AudioManager.clips, TickScheduler.taskQueue, and FailsafeManager.ignoreGens were all plain non-thread-safe collections despite being touched from handlers that can run off the client thread (Java Sound line-event callbacks, Multithreading's worker pool). EventBus already established CopyOnWriteArrayList/ ConcurrentHashMap as this codebase's pattern for that; apply it consistently.
- SubCommand.name was declared but never read; buildSubCommand() always uses the Kotlin function name for the brigadier literal. - ButtonSetting.renderSetting() re-bound buttonWidth to itself with no effect. - ModeSetting.mouseClicked()'s options.isEmpty() check was unreachable, since the init block already requires options to be non-empty at construction.
AutoClicker.attackMode and PlayerESP.espType were raw Ints matched positionally against a ModeSetting options array, so reordering the array would silently desync the branches with no compiler warning (unlike Rotations.RotationType, which already models the same kind of concept as an enum). ModeSetting itself still persists a plain Int (its UI/serialization format is unchanged); the enums are a typed view over that Int at the consuming site, making the when-branches exhaustive and named.
Every other dependency is declared via libs.versions.toml; DevAuth was the only one hardcoded as a raw coordinate string in build.gradle.kts.
FailsafeUtils was a single init() with no independent responsibility beyond calling FailsafeManager.ignoreFailsafe() -- an arbitrary Manager/Utils split for what is really one piece of FailsafeManager's own startup behavior.
.editorconfig declared a grouped import layout (non-JDK first, then java/ javax/kotlin/static last) that nothing ever enforced -- the project's own IntelliJ code style is KOTLIN_OFFICIAL (plain alphabetical, single group), which is what every file in the codebase already follows. The declared rule was the stale one; reformatting ~150 files to match a rule that contradicts the project's own configured code style would just fight the IDE on every future edit. Point the config at reality instead.
PADDING (and TopbarComponent's differently-named INNER_PADDING) was independently redeclared at the same 20f value in 7 files. Define it once as a top-level constant in the ui package and have every component/page/setting reference that instead. Purely a constant extraction -- every value stays 20f, so rendering output is unchanged.
…etting Both components duplicated the exact same hover-transition idiom: track wasHovering, restart a ColorAnimation + EaseOutAnimation pair on transition, then derive an overlay alpha and a border/text color from it. Pull that into one HoverFade class. SidebarButton and CheckboxSetting use the same two animation primitives but are driven by different state (selection, toggled value) with different draw structure, so they were left alone rather than forced into this abstraction.
…Setting RangeSetting (two knobs) and SliderSetting (one knob) duplicated identical value-box and track rendering code and six identically-valued constants. Extract the shared rendering (value box, track background/fill/knobs) into TrackSettingSupport, parameterized by knob positions so both the one-knob and two-knob cases go through the same draw calls. Drag-state handling (Knob enum vs plain Boolean) stays in each class since that genuinely differs between them.
…tsPage Both pages flowed items into 2 independently-growing columns (item i in column i % 2, each column advancing its own running Y), but implemented it two different ways -- named leftX/leftY/rightX/rightY pairs with an if/else in one, a FloatArray indexed by index % 2 in the other. Extract the shared algorithm into layoutMasonryColumns(). ThemesPage was left alone: it lays out a fixed-size uniform grid (no per-item height tracking needed), which is a different problem already solved in two lines -- forcing it through this helper would add indirection without removing any real duplication.
Both backends duplicated the same close-and-null-out logic for their DirectContext/BackendRenderTarget/Surface triple, both in close() and in the cache-invalidation branch of surfaceFor(). Extract that into a small SkijaSurfaceCache owned by each backend. GL's extra native cleanup (FBO, depth/stencil renderbuffer) has no Vulkan equivalent and stays in GlSurface.close() -- only the genuinely shared teardown moved. Backend- specific surface construction (BackendRenderTarget.makeGL vs .makeVulkan, cache-key comparisons) is untouched.
No evidence of concurrent access: addons is only ever written during the single-threaded PreLaunch phase (loadAddon/loadDevelopmentAddons, both called synchronously from onPreLaunch) and read once from loadAddons() at init. Unlike AudioManager (Java Sound callback thread) or ModuleManager (read from Multithreading.runAsync in ConfigScreen/HudEditorScreen), there's no actual cross-thread access here, so CopyOnWriteArrayList was unjustified churn.
…relying on ordinal entries.getOrElse(index) mapped the ModeSetting's stored Int to an enum by declaration order, which is exactly the kind of implicit position-coupling that made the raw-Int version risky in the first place: reordering the enum constants would silently desync from the ModeSetting options array again, just one layer removed. Store each constant's intended option index explicitly and look up by that value instead of by position.
This codebase already has an established Helper naming convention for this exact kind of shared UI logic (ScrollHelper, DragHandler, SnapHelper, TextInputHelper). "Support" was an inconsistent one-off; rename to match.
twiston-7
marked this pull request as ready for review
September 22, 2026 12:07
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.
No description provided.