review: remove null CoreRegistry, guard null binds/providers, fix Mermaid

[agent-refr]

AI-assisted PR. Filed by agent driven by @Cervator via GDD.

Summary

Fifth review follow-up to #5299 (gestalt-di migration). One critical fix, three defensive guards, one doc fix.

Changes

• TerasologyEngine constructor: Remove premature CoreRegistry.setContext() call that passed null rootContext. The correct call happens in initialize() after rootContext is created.
• BindsSubsystem: Null-check mouseWheelUpBind/mouseWheelDownBind before accessing getId() — they can be null after clearBinds().
• ReadWriteStorageManager: Guard ChunkProvider and NetworkSystem provider access in createSaveTransaction() with separate null checks and specific error messages.
• Engine-States.md: Fix Mermaid subgraph ID collision — LoadingStepUpdate was used as both node and subgraph ID.

Test Plan

• Engine compilation passes
• Single-player game starts

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

Deferred CoreRegistry context initialization from the constructor to initialize(); added null guards for mouse wheel binds and for resolved ChunkProvider/NetworkSystem with explicit errors; and relinked a Mermaid edge to target a labeled subgraph node in documentation.

Changes

Cohort / File(s)	Summary
Documentation docs/EngineStartUp/Engine-States.md	Relinked Mermaid edge to target the labeled LoadingStepUpdate[Step Update] subgraph node.
Engine initialization engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java	Replaced early CoreRegistry.setContext(rootContext) in constructor with CoreRegistry.setContext(null) so the context is set later in initialize().
Input binding null-safety engine/src/main/java/org/terasology/engine/core/subsystem/config/BindsSubsystem.java	Added null checks for mouseWheelUpBind and mouseWheelDownBind before calling getId() to avoid NPEs.
Save transaction validation engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java	Eagerly resolve ChunkProvider and NetworkSystem to local instances, validate non-null with explicit IllegalStateException messages, use locals when building save transactions; isRunModeAllowSaving() now early-returns false if either is null and re-resolves instances for remaining checks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• BenjaminAmos

Poem

🐰 I hopped through lines both small and neat,
I nudged a guard so code won't meet defeat,
Held context back until its cue,
Linked the step so charts stay true,
Hop—safe builds and tasty root! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: CoreRegistry removal, null guards for binds/providers, and Mermaid diagram fix.
Description check	✅ Passed	The pull request description is directly related to the changeset, providing clear summaries of all four changes (TerasologyEngine, BindsSubsystem, ReadWriteStorageManager, Engine-States.md) with appropriate context about the gestalt-di migration follow-up.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java`:
- Line 185: The constructor for TerasologyEngine must explicitly clear the
global CoreRegistry to avoid leaking a previous engine/context between
builder.build() and initialize(); update the TerasologyEngine constructor to
call the appropriate CoreRegistry reset/clear method (e.g., CoreRegistry.clear()
or equivalent) immediately on construction (before any listeners, subscriptions,
or early callbacks are registered) so the old services are not visible until
initialize() repopulates the registry.

In
`@engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java`:
- Around line 270-277: The guard checks in createSaveTransaction() are bypassed
because update() calls isRunModeAllowSaving() which dereferences
networkSystem.get() and chunkProvider.get() without null checks; change
isRunModeAllowSaving() to avoid directly calling networkSystem.get() and
chunkProvider.get() (use Optional.isPresent()/Supplier#orElse(null) or retrieve
via the same null-safe pattern used in createSaveTransaction()), or have
isRunModeAllowSaving() accept the already-resolved ChunkProvider and
NetworkSystem parameters so it only examines non-null values; update references
to isRunModeAllowSaving(), update(), networkSystem, chunkProvider, and
createSaveTransaction() accordingly to ensure no unchecked .get() is performed
before null-safe guards run.

🪄 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: 8885dd26-c626-4b14-8588-1327250b2186

📥 Commits

Reviewing files that changed from the base of the PR and between a1d4f8a and afe24b8.

📒 Files selected for processing (4)

• docs/EngineStartUp/Engine-States.md
• engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java
• engine/src/main/java/org/terasology/engine/core/subsystem/config/BindsSubsystem.java
• engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java

[Copilot]

Pull request overview

Follow-up hardening after the gestalt-di migration to prevent null-related issues during engine bring-up, saving, and input binding enumeration, plus a Mermaid diagram fix in engine startup docs.

Changes:

• Adjust TerasologyEngine constructor behavior around CoreRegistry context initialization/clearing.
• Add defensive null-guards for mouse wheel binds and save-transaction dependencies (ChunkProvider, NetworkSystem).
• Fix a Mermaid subgraph ID collision in engine startup state documentation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java	Guard ChunkProvider/NetworkSystem access during save transaction creation; add a defensive early-return in save eligibility check.
engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java	Replace a premature CoreRegistry.setContext(rootContext) (where rootContext is not yet created) with an explicit context clear.
engine/src/main/java/org/terasology/engine/core/subsystem/config/BindsSubsystem.java	Prevent NPE when mouse wheel binds are unset (e.g., after clearBinds()).
docs/EngineStartUp/Engine-States.md	Resolve Mermaid ID collision by separating node/subgraph labeling.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.