Repository navigation
Conversation
…ause points Compiling while Play Mode is active stops Play Mode and triggers a domain reload, silently discarding both the Play session state and every enabled pause-point Harmony patch. Users had no signal this happened until they noticed their pause points stopped firing. CompileUseCase now captures whether Play Mode was active and how many pause points were enabled before the PlayMode-stop step mutates that state, and surfaces a Warning on the compile response describing the loss. UloopPausePointRegistry gains GetActiveCount(); Compile.Editor is wired to it via the same InternalsVisibleTo + asmdef-reference pattern already used by PausePoint.Editor, RunTests.Editor, and other consumers. The decision logic itself lives in a pure CompilePausePointWarningBuilder, unit-tested directly to avoid needing a real Play Mode session in an EditMode test. Also verified the physical/message-callback pause-point warning (OnCollisionEnter, OnTriggerEnter, etc.) is already fully implemented and test-covered from a prior PR; no new code was needed there.
Contributor
📝 WalkthroughWalkthroughCompilation now captures Play Mode pause-point state, builds an optional warning, propagates it through compilation and result persistence, exposes it in ChangesCompile pause-point warning flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CompileUseCase
participant UloopPausePointRegistry
participant CompilePausePointWarningBuilder
participant CompilationExecutionService
participant CompileController
participant CompileResultSessionRecorder
CompileUseCase->>UloopPausePointRegistry: Read active pause-point count
CompileUseCase->>CompilePausePointWarningBuilder: Build pause-point warning
CompileUseCase->>CompilationExecutionService: Execute compilation with warning
CompilationExecutionService->>CompileController: TryCompileAsync with warning
CompileController->>CompileResultSessionRecorder: Record result with warning
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
… polling path CompileUseCase.CompileAsync attached the pause-point Warning only to the CompileResponse it directly returns, but the default `uloop compile` flow (WaitForDomainReload=true) instead polls get-compile-status, which reads a separately constructed CompileResponse stored by CompileController via CompileResultSessionRecorder. Thread the computed warning string through CompilationExecutionService, CompileController, and CompileResultSessionRecorder so both response-construction call sites share the same value, and update the CompileController/ CompileResponseFactory call sites broken by the new required parameter.
hatayama
merged commit Jul 20, 2026
6902039
into
feature/pause-point-round4-integration
1 of 2 checks passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/Compile/CompileController.cs (1)
110-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove state mutation below validation and early returns.
Assigning
_pendingPausePointWarningat the very beginning of the method overwrites the warning for any currently running compilation (if this instance is reused and called concurrently, as it is designed to handle gracefully by awaiting_currentCompileTask). Furthermore, if the method returns early due to a scene change validation failure, this state is never cleared by thefinallyblock.Move the assignment to immediately follow
_isCompiling = true;to ensure it only tracks state for a successfully initiated compile task.♻️ Proposed refactor
- _pendingPausePointWarning = pausePointWarning; - if (_isCompiling) { // If compilation is already in progress, wait for the current task. if (_currentCompileTask != null) { VibeLogger.LogInfo( "compile_controller_wait_existing_task", "Compilation is already in progress; waiting for the existing task.", BuildCompileControllerStateContext(new Dictionary<string, object> { ["requested_force_recompile"] = forceRecompile })); return await _currentCompileTask.Task.ConfigureAwait(false); } throw new InvalidOperationException("Compilation is in progress, but the task could not be found."); } (bool CanProceed, string Message, string[] ScenePaths) sceneChangeResult = ExternalSceneChangeTracker.ResolveForCompile(_reloadExternalSceneChanges); if (!sceneChangeResult.CanProceed) { VibeLogger.LogWarning( "compile_external_scene_change_resolution_failed", sceneChangeResult.Message, new { reload_external_scene_changes = _reloadExternalSceneChanges, scene_paths = sceneChangeResult.ScenePaths }); CompileResult result = CompileResultFactory.CreateExternalSceneChangeFailureResult(sceneChangeResult); RecordCompileResultIfNeeded(result, pausePointWarning: null); return result; } _isCompiling = true; + _pendingPausePointWarning = pausePointWarning; _compileMessages.Clear();🤖 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/FirstPartyTools/Compile/CompileController.cs` around lines 110 - 145, Move the _pendingPausePointWarning assignment in the compile method to immediately after _isCompiling is set to true. Remove the initial assignment so concurrent calls awaiting _currentCompileTask and scene-change validation failures do not overwrite or retain pending warning state.
🤖 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/FirstPartyTools/Compile/CompileController.cs`:
- Around line 110-145: Move the _pendingPausePointWarning assignment in the
compile method to immediately after _isCompiling is set to true. Remove the
initial assignment so concurrent calls awaiting _currentCompileTask and
scene-change validation failures do not overwrite or retain pending warning
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac3052be-ace6-43bb-849e-8d7ac5d40e21
⛔ Files ignored due to path filters (3)
Assets/Tests/Editor/CompilePausePointWarningBuilderTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Compile/CompilePausePointWarningBuilder.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Compile/UnityCLILoop.FirstPartyTools.Compile.Editor.asmdefis excluded by none and included by none
📒 Files selected for processing (16)
Assets/Editor/CompileCheckWindow/CompileCheckerExample.csAssets/Editor/CompileCheckWindow/CompileEditorWindow.csAssets/Tests/Editor/CompilePausePointWarningBuilderTests.csAssets/Tests/Editor/CompileResponseFactoryTests.csAssets/Tests/Editor/CompileSessionResultStoreTests.csAssets/Tests/Editor/CompileUseCaseTests.csPackages/src/Editor/FirstPartyTools/Compile/CompilationExecutionService.csPackages/src/Editor/FirstPartyTools/Compile/CompileController.csPackages/src/Editor/FirstPartyTools/Compile/CompilePausePointWarningBuilder.csPackages/src/Editor/FirstPartyTools/Compile/CompileResponse.csPackages/src/Editor/FirstPartyTools/Compile/CompileResponseFactory.csPackages/src/Editor/FirstPartyTools/Compile/CompileResultSessionRecorder.csPackages/src/Editor/FirstPartyTools/Compile/CompileUseCase.csPackages/src/Runtime/PausePoints/AssemblyInfo.csPackages/src/Runtime/PausePoints/UloopPausePointRegistry.cstests/contracts/compile_status_response_contract.json
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
1 participant
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
uloop compilenow warns when it compiles while Play Mode is active and pause points are still enabled, since the domain reload that follows discards both the Play session state and every pause-point patch.OnCollisionEnter,OnTriggerEnter, etc.) already covers this case fully — no new implementation was needed there.User Impact
Warningfield naming how many pause points were active and explaining that they need to be re-enabled after the compile completes.Changes
CompileResponsegains an additiveWarning: stringfield.CompileUseCasecaptures whether Play Mode was active and the number of enabled pause points before Play Mode gets stopped for compilation, then attaches the warning text on the path where compilation actually runs.UloopPausePointRegistrygainsGetActiveCount().Compile.Editoris wired to the PausePoints runtime assembly using the sameInternalsVisibleTo+ asmdef-reference pattern already used by several other first-party tool assemblies.CompilePausePointWarningBuilder, unit-tested directly (avoids needing a real Play Mode session inside an EditMode test).compile_status_response_contract.jsonfixture updated additively for the new field; both the C# and Go contract tests were re-verified.Verification
dist/darwin-arm64/uloop compile --project-path <repo>→ 0 errors / 0 warnings.dist/darwin-arm64/uloop run-tests --filter-type all→ 2240/2250 passed, 3 pre-existing failures unrelated to this change (dispatcher pin version drift, an unrelated schema-attribute test), 7 skipped.cd cli/project-runner && go test ./internal/projectrunner/... -run TestCompileStatusResponseMatchesSharedContract→ pass.sh scripts/check-go-cli.sh→ all Go modules pass (format, vet, lint, tests, build).