review: safe JoinServer handoff, ScreenGrabber visibility, deprecate CoreRegistry constructor

[agent-refr]

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

Summary

Sixth review follow-up to #5299 (gestalt-di migration). The last critical fix, plus cleanup and a partial fix for save previews.

Changes

• JoinServer: Replace bare Thread with FutureTask for safe thread handoff. Exceptions are now surfaced instead of silently treated as success. Unsafe .stream().findFirst().get() replaced with orElseThrow(). CoreRegistry is now set from the main thread after the task completes, not from the worker.
• InitialiseWorld: Clarify seed origin with a comment — seed is carried by the GameManifest from the UI, with a random fallback.
• ReadWriteStorageManager: Deprecate the old constructor that relies on CoreRegistry for implicit dependencies.
• WorldRendererImpl: Propagate ScreenGrabber to CoreRegistry so save preview image capture can find it. This fixed one part of Save preview images are always black — framebuffer empty at capture time #5321 — the screenshot is now taken and saved to disk, but the framebuffer is empty at capture time (render loop has stopped). The remaining timing issue is tracked in Save preview images are always black — framebuffer empty at capture time #5321.

Related

• Save preview images are always black — framebuffer empty at capture time #5321 (black save preview images — framebuffer timing)
• Overlapping NUIManager instances during StateLoading context switch #5316 (overlapping NUIManagers)

Test Plan

• Engine compilation passes
• Screenshot IS saved to disk (confirmed in logs)
• Single-player game starts, multiplayer join works
• Save preview still black due to framebuffer timing (Save preview images are always black — framebuffer empty at capture time #5321)

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b37962ab-6d38-48fa-9ed3-1f20100acee5

📥 Commits

Reviewing files that changed from the base of the PR and between f8dbec7 and 1d98bac.

📒 Files selected for processing (1)

• engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.java

🚧 Files skipped from review as they are similar to previous changes (1)

• engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.java

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Improved world seed initialization to properly handle missing seeds with automatic fallback generation
  • Enhanced server joining process and module environment application for improved stability
  • Added proper screen grabber initialization to ensure rendering features are available

• Chores

  • Deprecated legacy constructor pattern in storage management in favor of dependency injection

Walkthrough

Fixes and small refactors across engine initialization and persistence: fall back to a generated world seed if missing, replace raw Thread usage with FutureTask for module environment application, deprecate a legacy ReadWriteStorageManager constructor, and register ScreenGrabber into CoreRegistry during renderer init.

Changes

Cohort / File(s)	Summary
World Seed Generation engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseWorld.java	Adds explicit null/empty check for world seed from GameManifest; generates and sets a random 16-character seed via FastRandom when missing.
Async Module Application engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.java	Replaces Thread lifecycle tracking with FutureTask<Context>, retrieves Context via get(), adds ExecutionException/InterruptedException handling, returns ContextImpl from background task, and enforces presence of BeanContext with orElseThrow().
Constructor Deprecation engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java	Marks the parameterized constructor as @Deprecated and documents it as superseded by the @Inject constructor.
Renderer Initialization engine/src/main/java/org/terasology/engine/rendering/world/WorldRendererImpl.java	Retrieves a ScreenGrabber from renderer context during init() and registers it into CoreRegistry when present.

Sequence Diagram(s)

sequenceDiagram
    participant Loader as Load Process (JoinServer)
    participant Future as Background Task (FutureTask)
    participant ModuleEnv as Module Environment Builder
    participant Core as CoreRegistry
    participant Network as NetworkSystem
    Loader->>Future: submit module-environment task
    Future-->>Loader: done
    Loader->>Future: get() (may throw ExecutionException / InterruptedException)
    alt success
        Future--)ModuleEnv: returns ContextImpl
        Loader->>Core: CoreRegistry.set(Context)
    else ExecutionException
        Loader->>Core: CoreRegistry.set(StateMainMenu)
        Loader->>Network: shutdown()
    else InterruptedException
        Loader->>Loader: Thread.currentThread().interrupt()
        Loader-->>Loader: abort load (return false)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• BenjaminAmos

Poem

🐰
I hopped through seeds both lost and found,
Swapped tangled threads for futures sound,
I nudged a grabber into view,
Deprecated an old constructor too,
A tiny hop — the engine bounds.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% 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 three main changes: JoinServer thread handoff safety, ScreenGrabber registry visibility, and ReadWriteStorageManager constructor deprecation.
Description check	✅ Passed	The description clearly relates to the changeset, detailing the JoinServer improvements, InitialiseWorld clarification, deprecation, and ScreenGrabber propagation with relevant issue references.

_{✏️ 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: 1

🤖 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/modes/loadProcesses/JoinServer.java`:
- Around line 72-77: The catch block for ExecutionException in JoinServer (where
you construct StateMainMenu and call
context.get(GameEngine.class).changeState(mainMenu)) should end the current load
step so it isn't re-run; change the final return value to true (matching
InitialiseWorld.step() pattern) after switching to StateMainMenu to signal the
step is complete and avoid re-entering the same catch on the next step, keeping
the existing networkSystem.shutdown() call and logger.error call intact.

🪄 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: 98dd2914-88a4-448d-b75b-080795e0ceab

📥 Commits

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

📒 Files selected for processing (4)

• engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseWorld.java
• engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.java
• engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java
• engine/src/main/java/org/terasology/engine/rendering/world/WorldRendererImpl.java