DI PR post-review follow-up 1#5313
Merged
Cervator merged 7 commits intoMovingBlocks:developfrom Apr 2, 2026
Merged
Conversation
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
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 (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBumped Gestalt snapshot versions; set UniverseSetupScreen.universeWrapper before preparing context; made ContextImpl link child beanContexts to parent beanContexts; added unit tests for ContextImpl parent→child resolution; changed StateLoading to track process costs incrementally and re-push the loading screen after context swap. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as StateLoading
participant OldContext as Old Context
participant NewContext as New Context
participant NewNUI as New NUIManager
participant LoadingScreen as LoadingScreen
Loader->>OldContext: initiate context swap
OldContext-->>Loader: provide new child Context
Loader->>NewContext: switch to child context
Loader->>NewNUI: obtain NUIManager from NewContext
alt loadingScreen exists
Loader->>LoadingScreen: re-push to NewNUI
LoadingScreen->>NewNUI: register/display on NewNUI
end
Loader->>Loader: addAndTrack(new LoadProcess)
Loader->>Loader: maxProgress += process.getExpectedCost()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
engine-tests/src/test/java/org/terasology/engine/context/internal/ContextImplTest.java (1)
19-101: Good test coverage for the fix.The tests comprehensively cover:
- Bean resolution via
get()(fallback path)- Bean resolution via
inject()(BeanContext hierarchy path - the broken behavior)- Child override precedence
- Null parent handling
Consider adding a test case for when
parentis aContextimplementation other thanContextImpl(e.g., a mock orImmutableContextImpl) to document the expected behavior when the parent'sBeanContextcannot be extracted. This would serve as regression documentation for theinstanceofcheck behavior.📋 Optional: Add test for non-ContextImpl parent
`@Test` public void childContextWithNonContextImplParentFallsBackToGetOnly() { // Create a mock/wrapper Context that is not a ContextImpl Context nonContextImplParent = new Context() { private final Map<Class<?>, Object> services = new java.util.HashMap<>(); { services.put(TestService.class, new TestService("from-non-contextimpl")); } `@Override` public <T> T get(Class<T> type) { return type.cast(services.get(type)); } `@Override` public <T, U extends T> void put(Class<T> type, U object) { services.put(type, object); } }; ContextImpl child = new ContextImpl(nonContextImplParent); // get() should still work via parent.get() fallback TestService viaGet = child.get(TestService.class); assertNotNull(viaGet, "child.get() should resolve via parent.get() fallback"); assertEquals("from-non-contextimpl", viaGet.name); // inject() will NOT resolve because parent's BeanContext is null TestConsumer consumer = new TestConsumer(); child.inject(consumer); assertNull(consumer.service, "inject() cannot resolve from non-ContextImpl parent's BeanContext"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/context/internal/ContextImplTest.java` around lines 19 - 101, Add a test that ensures ContextImpl constructed with a parent that is not a ContextImpl (e.g., an anonymous Context or ImmutableContextImpl mock) still resolves beans via child.get(...) fallback but does not resolve `@Inject` fields via BeanContext; create a non-ContextImpl parent that implements get/put and registers TestService, construct ContextImpl child with that parent (use ContextImpl(Context) constructor), assert child.get(TestService.class) returns the parent's service and that injecting into TestConsumer leaves consumer.service null, and name the test something like childContextWithNonContextImplParentFallsBackToGetOnly to document the instanceof-based behavior.engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java (1)
394-396: Account only for the processes appended in these steps.Right now this works because both post-load injectors are queued last, but rescanning
loadProcessesbakes that ordering assumption intomaxProgress. If another step is ever queued after either injector, its cost gets counted once ininit()and again here. IncrementmaxProgressas each post-switch process is added instead of summing the whole queue.♻️ Example helper to make the accounting local and order-independent
private void addPostSwitchProcess(LoadProcess process) { loadProcesses.add(process); maxProgress += process.getExpectedCost(); }Then route the
loadProcesses.add(...)calls in both post-load steps throughaddPostSwitchProcess(...)and drop the trailing queue scan.Also applies to: 453-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java` around lines 394 - 396, The code currently recomputes maxProgress by summing loadProcesses (using loadProcesses and maxProgress in init()), which double-counts items added earlier and relies on queue ordering; instead, create a helper (e.g., addPostSwitchProcess(LoadProcess)) that does loadProcesses.add(process) and immediately increments maxProgress by process.getExpectedCost(), then replace direct loadProcesses.add(...) calls in the post-load/post-switch steps with addPostSwitchProcess(...) and remove the subsequent scan that sums the entire loadProcesses queue (also apply the same change for the second occurrence that currently recalculates maxProgress).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@engine-tests/src/test/java/org/terasology/engine/context/internal/ContextImplTest.java`:
- Around line 19-101: Add a test that ensures ContextImpl constructed with a
parent that is not a ContextImpl (e.g., an anonymous Context or
ImmutableContextImpl mock) still resolves beans via child.get(...) fallback but
does not resolve `@Inject` fields via BeanContext; create a non-ContextImpl parent
that implements get/put and registers TestService, construct ContextImpl child
with that parent (use ContextImpl(Context) constructor), assert
child.get(TestService.class) returns the parent's service and that injecting
into TestConsumer leaves consumer.service null, and name the test something like
childContextWithNonContextImplParentFallsBackToGetOnly to document the
instanceof-based behavior.
In `@engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java`:
- Around line 394-396: The code currently recomputes maxProgress by summing
loadProcesses (using loadProcesses and maxProgress in init()), which
double-counts items added earlier and relies on queue ordering; instead, create
a helper (e.g., addPostSwitchProcess(LoadProcess)) that does
loadProcesses.add(process) and immediately increments maxProgress by
process.getExpectedCost(), then replace direct loadProcesses.add(...) calls in
the post-load/post-switch steps with addPostSwitchProcess(...) and remove the
subsequent scan that sums the entire loadProcesses queue (also apply the same
change for the second occurrence that currently recalculates maxProgress).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8f89c45-4524-48a8-ac0e-7ca70e7639f5
📒 Files selected for processing (3)
engine-tests/src/test/java/org/terasology/engine/context/internal/ContextImplTest.javaengine/src/main/java/org/terasology/engine/context/internal/ContextImpl.javaengine/src/main/java/org/terasology/engine/core/modes/StateLoading.java
2 tasks
| // currently not yet for build-logic, see https://github.com/gradle/gradle/issues/15383 , change verisons | ||
| // here and there please. | ||
| val gestalt = version("gestalt", "8.0.0-SNAPSHOT") | ||
| val gestalt = version("gestalt", "8.0.1-SNAPSHOT") |
Contributor
There was a problem hiding this comment.
MovingBlocks/gestalt#160 has still not been merged, whereas it should have been by this point. This dependency may well be manually built based on that pull request but we should probably merge it anyway.
Member
Author
There was a problem hiding this comment.
Yep I took a look, triggered CodeRabbit, and also asked my local Claude. There is a potential issue with an easy fix, although impact is somewhat unknown from me just reading and listening.
2 tasks
Cervator
added a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 31, 2026
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 MovingBlocks#5313. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| loadProcesses.add(new AddHostPostLoadProcessesStep()); | ||
| } | ||
|
|
||
| private void addAndTrack(LoadProcess process) { |
Contributor
There was a problem hiding this comment.
Do we want to use this helper method in initHost() and initClient() as well for consistency?
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>
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 MovingBlocks#5299. 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>
…overflow 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>
- 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>
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 MovingBlocks#5313. 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 MovingBlocks#5313. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
force-pushed
the
review/post-di
branch
from
6e3ed3d to
2f16392
Compare
Member
Author
|
I tested the progress bar and noticed it jumps backwards at one point - poked around at some options but found nothing elegant. It jumps due to additional steps being added which makes the current progress percentage shrink dramatically. Did come up with an idea to just add multiple progress bars if needed to more authentically cover different phases rather than edit the total mid-progress, but .. different PR one day maybe ;-) |
BenjaminAmos
approved these changes
Apr 2, 2026
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
3 participants
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.
Summary
First pass addressing review findings from #5299 (gestalt-di migration). This PR contains only the review-fix delta — not the full 137-file migration.
This is as much a workflow demonstration as a fix PR. We're using a structured review triage process (GDD framework) to systematically work through findings from BSA, Copilot, and CodeRabbit across the massive DI migration PR, phased to avoid another multi-year review cycle.
Approach
We triaged all review findings into two phases:
Review threads on #5299 are intentionally NOT resolved — kept for future reference.
Changes
8.0.0-SNAPSHOT→8.0.1-SNAPSHOTacross settings.gradle.kts and build-logictarget==rootinbindExpression(). Adding explicit.with(Interface).use(Impl)creates duplicate bean keys and breaks resolution.setEnvironment()received aUniverseWrapperbut never assigned it to the instance field. One-line fix.NewGameScreenalways callssetEnvironment()before showing the screen. Deferred to Phase 2 as defensive hardening.ContextImpl(Context)constructor created a standaloneDefaultBeanContextwith no parent, so@Injectresolution in child contexts could not see parent beans. Affects network handlers, NUI manager, and other child-context callers.ContextImpl fix — test-driven validation
The ContextImpl bug was subtle:
context.get()still worked (via fallback toparent.get()), but@Injectresolution through theBeanContexthierarchy was broken. NoBeanResolutionExceptionappeared in logs because the exception was caught and silently fell back to the parentContextlookup.To prove the fix was real, we wrote a unit test (
ContextImplTest.childContextResolvesParentBeansViaInject) that:DependencyResolutionExceptionwhen injecting a bean registered in the parentIt remains uncertain whether this bug was actively breaking anything at runtime, since
context.get()(the common lookup path) worked fine via the fallback. The@Injectpath throughBeanContextis used by constructor injection during DI container resolution, which would only surface as issues in code paths that rely on the DI container creating objects with injected parent-context dependencies. The network connection handlers (ClientConnectionHandler,ServerConnectionHandler) are the most likely candidates, but we have not yet been able to attribute a specific runtime bug to this fix.DI Patterns Harvested
While reviewing we discovered and documented several Gestalt DI conventions:
.with(Impl.class).lifetime(Singleton)auto-maps all implemented interfaces viainterfaceMappinginDefaultBeanContext.bindExpression(). Do NOT add explicit interface bindings — it breaks resolution.@javax.inject.Injecton protected members as compromise,@Inis legacyContextdirectly — inject specific dependenciesServiceRegistryfor registration phase;ImmutableContextImplafter init — catches rogue writesCoreRegistryis being eliminated — no new usesRemaining Work
Phase 2 items tracked in our workspace for a follow-up PR:
Contextinjection cleanup (ConsoleImpl, BlockFamilyLibrary, WorldRendererImpl, ReadWriteStorageManager)UniverseSetupScreen— migrateContext.put()to ServiceRegistryTest Plan
Sample Transcript
As an example of human-agent interaction while working this PR see Gist https://gist.github.com/Cervator/4ef1537704e225c60f66c206ba4f0246
Sample Thalamus Snippet
This is a gitignored "midterm memory" storage document in a GDD user's workspace named
Thalamus.mdfor noting down temporary but at times multi-session topics, both from the human and the agentDI Migration Review
Design spec:
docs/plans/2026-03-28-di-migration-review-design.mdPR under review: #5299 | Branch:
review/post-diPhase 1 — Runtime Correctness (current)
Phase 2 — Code Quality (parked for later PR)
timedContextForModulesWidgetsengine.persistence.internalwhitelist (CodeRabbit)DI Patterns (living reference — harvest as we go)
@javax.inject.Injecton protected members as compromise,@Inis legacyContextdirectly — inject specific depsServiceRegistryfor registration phase;ImmutableContextImplafter — catches rogue writes.with(Impl.class).lifetime(Singleton)auto-maps all interfaces viainterfaceMapping. Do NOT add explicit.with(Interface.class).use(Impl.class)— it creates duplicate bean keys and breaks resolution. Only use explicit interface mapping when target != impl (e.g.,.with(Interface.class).use(() -> specificInstance)).Context.put()should not be called — migrate to ServiceRegistry-based initCoreRegistryis being eliminated — no new uses (test env is reluctant exception)🤖 Generated with Claude Code
(Or it would have been, but apparently GitHub fine-grained tokens cannot auto-create PRs in a repo it doesn't have write access to? Weak. May have to set @agent-refr as a collaborator. But first: sleep!)