BSA review: injecting into CoreRegistry is counter to the DI migration.
ScreenGrabber is already registered in the game-level ServiceRegistry
by LwjglRenderingSubsystemFactory.registerWorldRenderer(), so it should
be resolvable from CoreRegistry without manual injection. The black
save preview is a separate framebuffer timing issue tracked in #5321.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After changeState(mainMenu) on failure, returning false left the load
step active, causing the error path to repeat on the next tick. Return
true to end the step, matching the pattern in InitialiseWorld.
CodeRabbit review feedback on PR #5322.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ScreenGrabber was registered only in WorldRendererImpl's private child
context, invisible from the shared game context. ReadWriteStorageManager
resolves it via CoreRegistry.get(ScreenGrabber.class) when saving the
game preview image, which returned null — resulting in black preview
screenshots. Propagate the instance to CoreRegistry after init().
CodeRabbit flagged this on PR #5299 (outside-diff-range, major).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Mark the old constructor that resolves dependencies via CoreRegistry as
@Deprecated. The @Inject constructor is the correct DI path. No callers
use the old constructor directly — it exists only as a fallback.
BSA flagged this on PR #5299.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add comment clarifying that the world seed is carried by the
GameManifest from the UI (via GameManifestProvider), with a random
fallback if not specified.
BSA asked "Where is the seed being set now?" on PR #5299.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three issues in the client join path:
1. Dead thread was treated as success — if the environment switch
threw an exception, the thread died and step() returned true.
Now uses FutureTask which surfaces exceptions via get().
2. Unsafe .stream().findFirst().get() for BeanContext extraction
replaced with orElseThrow() and a clear error message.
3. CoreRegistry.setContext() was called from the worker thread.
Now the FutureTask returns the Context and the main thread
sets CoreRegistry after get() completes successfully.
CodeRabbit flagged this on PR #5299 (outside-diff-range, critical).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename short variable names (chunks, network) to descriptive names
(chunkProviderInstance, networkSystemInstance) for consistency across
createSaveTransaction() and isRunModeAllowSaving(). Assign
chunkProvider.get() to a local in isRunModeAllowSaving() to avoid
redundant calls.
Copilot review feedback on PR #5320.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TerasologyEngine: explicitly clear CoreRegistry in constructor to
prevent stale context leakage between build() and initialize().
- ReadWriteStorageManager: guard null providers in isRunModeAllowSaving()
which is called before createSaveTransaction() and could NPE during
state transitions.
CodeRabbit review feedback on PR #5320.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split the combined null check into separate checks with specific error
messages so the failing provider is immediately identifiable.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ChunkProvider and NetworkSystem providers are dereferenced unconditionally
in createSaveTransaction(). A save triggered during a state transition
could NPE if the providers are not yet resolved. Guard with null checks
and fail with a clear message.
CodeRabbit flagged this on PR #5299 (outside-diff-range, major).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
LoadingStepUpdate was used as both a node label and a subgraph ID,
which share the same Mermaid namespace. Move the label to the subgraph
declaration so the ID is only defined once.
CodeRabbit flagged this on PR #5299 (outside-diff-range, minor).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mouseWheelUpBind and mouseWheelDownBind can be null after clearBinds()
and may not be re-assigned if no wheel bindings are registered. Add
null checks before accessing getId().
CodeRabbit flagged this on PR #5299 (outside-diff-range, minor).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The constructor called CoreRegistry.setContext(rootContext) before
rootContext was created, passing null. The correct setContext call
happens in initialize() after rootContext is constructed. Removed the
premature call and updated the comment.
CodeRabbit flagged this on PR #5299 (outside-diff-range, critical).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove unused EntityManager imports left behind after switching to
EngineEntityManager. Align @param Javadoc style (capitalize, drop
trailing periods) to match existing conventions in the file.
CheckStyle + CodeRabbit review feedback on PR #5318.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Javadoc still said "add anything into the root context" when the
methods now receive a ServiceRegistry. Updated to say "register
services" and "build the root context" to match the DI migration.
CodeRabbit flagged this on PR #5299 (minor).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace EntityManager parameter with EngineEntityManager in the @Inject
constructor, eliminating the runtime downcast. Updated the two
WorldProviderCoreWorkAround subclasses to match. Gestalt DI auto-maps
PojoEntityManager to all its interfaces including EngineEntityManager.
CodeRabbit flagged this on PR #5299 (minor).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
forModuleEnvironment() creates an instance via the @Inject constructor
(which already calls populateWithDefaultHandlers), then called it again,
duplicating all default handler/factory registrations. The handlers
append to a list without deduplication.
CodeRabbit flagged this on PR #5299 (outside-diff-range, major).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two bugs in deserialize for private fields:
1. findSetter(field) can return null — no null check (NPE risk)
2. setter.invoke(result) passed no value argument — the setter was
called with zero args, silently failing to set the field
Fixed to match the serialize side's pattern: null-check the setter,
log an error if missing, and pass fieldValue.get() as the argument.
CodeRabbit flagged this on PR #5299 (outside-diff-range, critical).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use addAndTrack() in initHost() and initClient() as well, not just the
post-context-switch steps. This makes maxProgress tracking consistent
across all phases and removes the now-redundant loop in init() that
was double-counting.
BSA review feedback on PR #5313.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents the instanceof-based behavior: when the parent is not a
ContextImpl, context.get() resolves via the parent fallback but @Inject
resolution throws DependencyResolutionException since there is no parent
BeanContext to search.
CodeRabbit nitpick on PR #5313.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix progress bar double-counting: extract addAndTrack() helper that
adds a load process and increments maxProgress in one step, replacing
the full-queue iteration that re-counted pre-existing steps.
- ContextImplTest: fix copyright year (2026), remove commentary about
previous behavior per BSA's guidance to describe expected behavior only.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two related fixes in StateLoading:
1. After SwitchToContextStep replaces the NUI manager with the DI-created
instance, re-push the loading screen onto the new manager. Previously
the screen was orphaned on the old manager, causing flicker and
rendering through a dead manager.
2. Recalculate maxProgress when AddHostPostLoadProcessesStep and
AddClientPostLoadProcessesStep inject additional load steps. The
initial maxProgress only counted pre-context-switch steps, so the
progress bar overflowed once post-switch steps ran.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ContextImpl(Context parent) constructor created a standalone
DefaultBeanContext with no parent, so @Inject dependency resolution in
child contexts could not see beans registered in the parent. This
affected network connection handlers (ClientConnectionHandler,
ServerConnectionHandler), NUIManagerInternal, and other callers.
Fixed by extracting the parent's beanContext when the parent is a
ContextImpl, matching the pattern already used in the ServiceRegistry
variant of the constructor.
Includes a unit test that demonstrates the fix: childContext
ResolvesParentBeansViaInject fails with DependencyResolutionException
before this change and passes after.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
setEnvironment() received a UniverseWrapper parameter but never assigned it
to the instance field. The screen's UI bindings read from the field, not the
context, so seed/generator/server settings from AdvancedGameSetupScreen were
silently discarded.
Flagged independently by both Copilot and CodeRabbit on PR #5299.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Aligns version catalog and build-logic with the 8.0.1 gestalt branch
that contains the DI refactor changes this migration depends on.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Both InitialiseBlocks and ModuleManager.loadEnvironment called
.getClass() on potentially null references in their error messages,
which would throw NPE instead of the intended IllegalStateException.
CodeRabbit review feedback on PR #5317.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ServiceRegistry supplier silently returned null when AutoConfig
construction failed, publishing a null singleton. Later lookups would
get null and NPE far from the root cause. Now logs the error and throws
RuntimeException to surface the problem immediately.
CodeRabbit flagged this on PR #5299.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The loadEnvironment(Context, ...) method requires a ContextImpl to
extract the BeanContext for module DI, but the parameter type is the
broader Context interface. Added Javadoc documenting this requirement
and replaced the blind cast with instanceof + clear error message.
BSA flagged this on PR #5299: "Should this implementation requirement
be documented?"
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace blind cast of BlockManager to BlockManagerImpl with instanceof
pattern matching and a clear error message if the assumption is violated.
The cast is currently always safe (RegisterBlocks always creates
BlockManagerImpl), but making it explicit prevents silent ClassCastException
if the registration ever changes.
BSA flagged this on PR #5299: "Is it safe to assume that this down-cast
will always work?"
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BSA review feedback: the GameManifest parameter was left behind after
removing the commented-out code that used it. Remove from constructor
and update the caller in StateLoading.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The GameManifest import was removed along with the dead code, but the
constructor parameter still references the type. Restores the import.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Context parameter with explicit ReflectFactory and
CopyStrategyLibrary parameters in both constructors. The Context was
only used to pull these two dependencies, making it an unnecessary
intermediary.
BSA flagged this on PR #5299: "We should not be injecting Context
directly unless absolutely necessary."
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Inject NetworkSystem as an explicit constructor parameter instead of
pulling it from Context. Context is retained only for the lazy
PermissionManager lookup (provided by @Share at runtime, not available
at construction time).
BSA flagged this on PR #5299: "This class does not require a full
context. Inject NetworkSystem and PermissionManager instead."
PermissionManager cannot be fully injected yet due to its lifecycle.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename the nonsensical variable name to something descriptive. This is
a per-module child context used to inject widget-specific dependencies
(TypeWidgetLibrary) during control widget initialization.
BSA flagged this on PR #5299.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
