Skip to content

chore: Compile session state now flows from a single composition-root instance#1512

Merged
hatayama merged 1 commit into
v3-betafrom
refactor/c2-inject-session-state-service
Jul 5, 2026
Merged

chore: Compile session state now flows from a single composition-root instance#1512
hatayama merged 1 commit into
v3-betafrom
refactor/c2-inject-session-state-service

Conversation

@hatayama

@hatayama hatayama commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • Stops four production code paths from hand-building their own session state service and routes them all to the single instance the composition root already constructs.

User Impact

  • No behavior change (the underlying storage is Unity's SessionState, so all instances always read the same data). The gain is structural: session state now has one owner, so future changes to its wiring happen in one place instead of five.

Changes

  • New UnityCliLoopEditorSessionStateFacade (Domain): exposes the composition-root-owned service to call sites that have no injection seam — static IPC bridge command handlers and tools instantiated via Activator.CreateInstance. Follows the existing facade pattern (RegisterService + ServiceValue + throw-if-unregistered accessor) enforced by StaticFacadeStateGuardTests. Placed in Domain because the compile tool and Infrastructure assemblies reference Domain but not Application.
  • Compile pipeline (CompileToolCompileUseCaseCompilationExecutionServiceCompileController): the tool reads the facade once and threads the service down through constructors; the ad hoc constructions inside CompileUseCase and CompileController are gone.
  • CompileStatusBridgeCommand.Execute: reads the facade instead of newing a service per request.
  • Deleted the parameterless convenience constructors on DomainReloadDetectionFileService, UnityCliLoopBridgeServer, and UnityCliLoopBridgeServerInstanceFactory (grep confirmed no callers; the composition root always used the injected overloads).
  • Dev-only compile window/example under Assets/Editor now pass the facade service explicitly (Dev asmdef gains a Domain reference).

Verification

  • dist/darwin-arm64/uloop compile → Success, 0 errors / 0 warnings
  • uloop run-tests over compile/session-state/server-startup/facade-guard/onion-dependency suites → 122 tests, 119 passed; the 3 failures (MigratedFacadeFiles_WhenScanned_DoNotOwnMutableStaticState, EditorUiFiles_WhenLoaded_DoNotReferenceFacadeInternalsDirectly, SimulateMouseUiUseCase_WhenReturningAfterTimeout_UsesCapturedUnityObjectState) reproduce identically on a clean origin/v3-beta checkout and are unrelated to this change
  • grep confirms no new UnityCliLoopEditorSessionStateService( remains in Packages/src outside the composition root

Review in cubic

Four production sites rebuilt UnityCliLoopEditorSessionStateService with a
fresh UnityCliLoopEditorSessionStateRepository, bypassing the CompositionRoot
-owned shared instance. Introduce UnityCliLoopEditorSessionStateFacade in the
Domain layer as the read seam that CompositionRoot registers once, and thread
the shared service through the compile pipeline via constructor injection.
CompileTool (built by Activator.CreateInstance in the tool registry) and
CompileStatusBridgeCommand (a static bridge dispatched by the IPC router)
both lack an injection seam, so they read from the facade. The pipeline
below CompileTool (CompileUseCase -> CompilationExecutionService ->
CompileController) now takes the service via ctor. Drop the deprecated
parameterless convenience ctors on DomainReloadDetectionFileService,
UnityCliLoopBridgeServer, and UnityCliLoopBridgeServerInstanceFactory, since
they only existed to hide the same ad hoc construction and have no callers
now that CompositionRoot uses the injected paths. The dev editor window and
example add a Domain asmdef reference so they can obtain the shared service
through the facade for local compile checks.

Verification:
- dist/darwin-arm64/uloop compile --project-path <repo> -> Success, 0 errors,
  0 warnings.
- dist/darwin-arm64/uloop run-tests --filter-type regex --filter-value
  "(CompileSessionResultService|CompileStatusBridgeCommand|
  DomainReloadDetectionService|DomainReloadRecoveryUseCase|
  UnityCliLoopServerControllerStartupLock|UnityCliLoopServerStartupProtection|
  StaticFacadeStateGuard|OnionAssemblyDependency)"
  -> 122 tests: 119 pass, 3 baseline failures unrelated to this refactor
  (MigratedFacadeFiles_WhenScanned_DoNotOwnMutableStaticState references a
  stale MainThreadSwitcher path,
  EditorUiFiles_WhenLoaded_DoNotReferenceFacadeInternalsDirectly flags
  pre-existing Presentation references to CliVersionComparer, and
  SimulateMouseUiUseCase_WhenReturningAfterTimeout_UsesCapturedUnityObjectState
  checks unrelated content). All three fail on origin/v3-beta with these
  changes stashed.
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

A new static UnityCliLoopEditorSessionStateFacade is introduced to hold and expose a registered UnityCliLoopEditorSessionStateService. CompileController, CompileUseCase, and CompilationExecutionService are refactored to require this service via constructor injection instead of instantiating it internally. Callers are updated accordingly, and unused parameterless constructors on DomainReloadDetectionFileService, UnityCliLoopBridgeServerInstanceFactory, and UnityCliLoopBridgeServer are removed.

Changes

Session State Dependency Injection

Layer / File(s) Summary
Session state facade and registration
Packages/src/Editor/Domain/UnityCliLoopEditorSessionStateFacade.cs, Packages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.cs
New static facade holds a registered UnityCliLoopEditorSessionStateService, exposed via a Service property with null-checks; registered during application startup.
Constructor injection in compile pipeline
Packages/src/Editor/FirstPartyTools/Compile/CompileController.cs, CompileUseCase.cs, CompilationExecutionService.cs
CompileController, CompileUseCase, and CompilationExecutionService now require an injected UnityCliLoopEditorSessionStateService via constructor; parameterless CompileUseCase constructor removed; internal instantiations pass the injected service through the chain.
Caller updates
Packages/src/Editor/FirstPartyTools/Compile/CompileTool.cs, Packages/src/Editor/Infrastructure/Api/CompileStatusBridgeCommand.cs, Assets/Editor/CompileCheckWindow/CompileEditorWindow.cs, CompileCheckerExample.cs
Callers now fetch the session-state service from UnityCliLoopEditorSessionStateFacade.Service and pass it into constructors instead of direct instantiation.
Removed default constructors
Packages/src/Editor/Infrastructure/Server/DomainReloadDetectionFileService.cs, Packages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs
Public parameterless constructors that previously created a default session-state service are removed, leaving only internal dependency-injected constructors.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Registration as UnityCliLoopApplicationRegistration
  participant Facade as UnityCliLoopEditorSessionStateFacade
  participant Tool as CompileTool
  participant UseCase as CompileUseCase
  participant ExecService as CompilationExecutionService
  participant Controller as CompileController

  Registration->>Facade: RegisterService(sessionStateService)
  Tool->>Facade: Service
  Facade-->>Tool: sessionStateService
  Tool->>UseCase: new CompileUseCase(sessionStateService)
  UseCase->>ExecService: new CompilationExecutionService(_sessionStateService)
  ExecService->>Controller: new CompileController(_sessionStateService)
  Controller->>Controller: RecordCompileResultIfNeeded(_sessionStateService)
Loading

Possibly related PRs

  • hatayama/unity-cli-loop#1156: Both PRs touch CompileCheckerExample.cs's test methods that construct CompileController.
  • hatayama/unity-cli-loop#1282: Both PRs modify the same compile pipeline components (CompileController, CompileUseCase, CompileStatusBridgeCommand, DomainReloadDetectionFileService) tied to session-state wiring.
  • hatayama/unity-cli-loop#1248: Both PRs change how UnityCliLoopEditorSessionStateService is obtained/wired in CompileStatusBridgeCommand and related compile-status components.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: compile session state now comes from one composition-root-owned instance.
Description check ✅ Passed The description is detailed and directly matches the changeset, covering the facade, compile pipeline, bridge command, and constructor cleanup.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/c2-inject-session-state-service

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
Packages/src/Editor/Domain/UnityCliLoopEditorSessionStateFacade.cs (1)

15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Naming inconsistent with codebase convention.

ServiceValue is a private static field but doesn't follow the _camelCase convention used for backing fields elsewhere (_sessionStateService in CompileController, CompileUseCase, CompilationExecutionService).

✏️ Suggested rename
-        private static UnityCliLoopEditorSessionStateService ServiceValue;
+        private static UnityCliLoopEditorSessionStateService s_service;

(and update the two references below)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Packages/src/Editor/Domain/UnityCliLoopEditorSessionStateFacade.cs` at line
15, Rename the private static backing field ServiceValue to follow the
codebase’s _camelCase convention, matching patterns used in CompileController,
CompileUseCase, and CompilationExecutionService. Update all references in
UnityCliLoopEditorSessionStateFacade accordingly so the field declaration and
its two usages stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@Packages/src/Editor/Domain/UnityCliLoopEditorSessionStateFacade.cs`:
- Line 15: Rename the private static backing field ServiceValue to follow the
codebase’s _camelCase convention, matching patterns used in CompileController,
CompileUseCase, and CompilationExecutionService. Update all references in
UnityCliLoopEditorSessionStateFacade accordingly so the field declaration and
its two usages stay consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 087b9a04-bfca-49ad-8c3d-5fa18ab51020

📥 Commits

Reviewing files that changed from the base of the PR and between 2209dec and a8fdbe5.

⛔ Files ignored due to path filters (2)
  • Assets/Editor/UnityCLILoop.Dev.asmdef is excluded by none and included by none
  • Packages/src/Editor/Domain/UnityCliLoopEditorSessionStateFacade.cs.meta is excluded by none and included by none
📒 Files selected for processing (11)
  • Assets/Editor/CompileCheckWindow/CompileCheckerExample.cs
  • Assets/Editor/CompileCheckWindow/CompileEditorWindow.cs
  • Packages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.cs
  • Packages/src/Editor/Domain/UnityCliLoopEditorSessionStateFacade.cs
  • Packages/src/Editor/FirstPartyTools/Compile/CompilationExecutionService.cs
  • Packages/src/Editor/FirstPartyTools/Compile/CompileController.cs
  • Packages/src/Editor/FirstPartyTools/Compile/CompileTool.cs
  • Packages/src/Editor/FirstPartyTools/Compile/CompileUseCase.cs
  • Packages/src/Editor/Infrastructure/Api/CompileStatusBridgeCommand.cs
  • Packages/src/Editor/Infrastructure/Server/DomainReloadDetectionFileService.cs
  • Packages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs
💤 Files with no reviewable changes (2)
  • Packages/src/Editor/Infrastructure/Server/DomainReloadDetectionFileService.cs
  • Packages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.cs

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 13 files

Re-trigger cubic

@hatayama

hatayama commented Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Re: the ServiceValue naming nitpick — keeping the name as is. ServiceValue is the identifier StaticFacadeStateGuardTests explicitly allowlists for mutable static facade fields (alongside RepositoryValue/RegistryValue/RegisteredUseCase), and every existing facade (MainThreadSwitcher, UnityCliLoopServerApplicationFacade, CliSetupApplicationFacade) uses the same name. Renaming it to s_service would fail that guard test and break the established facade convention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant