chore: Session flags use a dedicated repository#1534
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extracts editor session flags into ChangesSession flags repository extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
1 issue found across 31 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Domain/UnityCliLoopSessionFlagsFacade.cs">
<violation number="1" location="Packages/src/Editor/Domain/UnityCliLoopSessionFlagsFacade.cs:13">
P3: Remove the redundant `?? throw ArgumentNullException` after `Debug.Assert` in `RegisterRepository`. Per project convention, internal API boundaries should use `Debug.Assert` for precondition enforcement without widening to runtime exceptions. The `Debug.Assert` already covers the null check in Unity Editor; the `?? throw` is dead code and inconsistent with the fail-fast contract-checking pattern.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| { | ||
| private static ISessionFlagsRepository RepositoryValue; | ||
|
|
||
| internal static void RegisterRepository(ISessionFlagsRepository repository) |
There was a problem hiding this comment.
P3: Remove the redundant ?? throw ArgumentNullException after Debug.Assert in RegisterRepository. Per project convention, internal API boundaries should use Debug.Assert for precondition enforcement without widening to runtime exceptions. The Debug.Assert already covers the null check in Unity Editor; the ?? throw is dead code and inconsistent with the fail-fast contract-checking pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Domain/UnityCliLoopSessionFlagsFacade.cs, line 13:
<comment>Remove the redundant `?? throw ArgumentNullException` after `Debug.Assert` in `RegisterRepository`. Per project convention, internal API boundaries should use `Debug.Assert` for precondition enforcement without widening to runtime exceptions. The `Debug.Assert` already covers the null check in Unity Editor; the `?? throw` is dead code and inconsistent with the fail-fast contract-checking pattern.</comment>
<file context>
@@ -0,0 +1,33 @@
+ {
+ private static ISessionFlagsRepository RepositoryValue;
+
+ internal static void RegisterRepository(ISessionFlagsRepository repository)
+ {
+ Debug.Assert(repository != null, "repository must not be null");
</file context>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/Infrastructure/Settings/UnityCliLoopSessionFlagsRepository.cs (1)
151-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompose
ClearDomainReloadRecoveryFlagsfrom the existing clear methods instead of duplicating the sets.The five
SetXxx(false)calls here re-implementClearDomainReloadFlag,ClearAfterCompileFlag, andClearReconnectingFlags+ClearPostCompileReconnectingUI. If one of those individual clear methods changes, this combined method can silently drift out of sync.♻️ Proposed refactor
public void ClearDomainReloadRecoveryFlags() { - SetIsDomainReloadInProgress(false); - SetIsAfterCompile(false); - SetIsReconnecting(false); - SetShowReconnectingUI(false); - SetShowPostCompileReconnectingUI(false); + ClearDomainReloadFlag(); + ClearAfterCompileFlag(); + ClearReconnectingFlags(); + ClearPostCompileReconnectingUI(); }🤖 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/Infrastructure/Settings/UnityCliLoopSessionFlagsRepository.cs` around lines 151 - 158, ClearDomainReloadRecoveryFlags currently duplicates the individual flag-reset calls instead of reusing the existing clear helpers. Update UnityCliLoopSessionFlagsRepository’s ClearDomainReloadRecoveryFlags to compose the already-defined methods like ClearDomainReloadFlag, ClearAfterCompileFlag, ClearReconnectingFlags, and ClearPostCompileReconnectingUI so the combined path stays aligned if any single clear method changes.
🤖 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/Infrastructure/Settings/UnityCliLoopSessionFlagsRepository.cs`:
- Around line 151-158: ClearDomainReloadRecoveryFlags currently duplicates the
individual flag-reset calls instead of reusing the existing clear helpers.
Update UnityCliLoopSessionFlagsRepository’s ClearDomainReloadRecoveryFlags to
compose the already-defined methods like ClearDomainReloadFlag,
ClearAfterCompileFlag, ClearReconnectingFlags, and
ClearPostCompileReconnectingUI so the combined path stays aligned if any single
clear method changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ec44289-0098-427b-baa8-945af6a67a00
⛔ Files ignored due to path filters (3)
Packages/src/Editor/Domain/ISessionFlagsRepository.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/UnityCliLoopSessionFlagsFacade.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/Settings/UnityCliLoopSessionFlagsRepository.cs.metais excluded by none and included by none
📒 Files selected for processing (28)
Assets/Tests/Editor/CompileSessionResultServiceTests.csAssets/Tests/Editor/CompileStatusBridgeCommandTests.csAssets/Tests/Editor/DomainReloadDetectionServiceTests.csAssets/Tests/Editor/DomainReloadRecoveryUseCaseTests.csAssets/Tests/Editor/OnionAssemblyDependencyTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/StaticFacadeStateGuardTests.csAssets/Tests/Editor/UnityCliLoopEditorSessionStateRepositoryTests.csAssets/Tests/Editor/UnityCliLoopEditorSessionStateTestFactory.csAssets/Tests/Editor/UnityCliLoopEditorSettingsRecoveryTests.csAssets/Tests/Editor/UnityCliLoopServerControllerStartupLockTests.csAssets/Tests/Editor/UnityCliLoopServerStartupProtectionTests.csPackages/src/Editor/Application/SessionRecoveryService.csPackages/src/Editor/Application/UnityCliLoopServerStartupService.csPackages/src/Editor/Application/UseCases/DomainReloadRecoveryUseCase.csPackages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.csPackages/src/Editor/CompositionRoot/UnityCliLoopEditorBootstrapper.csPackages/src/Editor/Domain/ISessionFlagsRepository.csPackages/src/Editor/Domain/UnityCliLoopEditorSessionStateService.csPackages/src/Editor/Domain/UnityCliLoopSessionFlagsFacade.csPackages/src/Editor/Infrastructure/Api/CompileStatusBridgeCommand.csPackages/src/Editor/Infrastructure/Server/DomainReloadDetectionFileService.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.csPackages/src/Editor/Infrastructure/Settings/UnityCliLoopSessionFlagsRepository.csPackages/src/Editor/Presentation/PresentationEditorStartup.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cs
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Domain/UnityCliLoopSessionFlagsFacade.cs">
<violation number="1" location="Packages/src/Editor/Domain/UnityCliLoopSessionFlagsFacade.cs:13">
P3: Remove the redundant `?? throw ArgumentNullException` after `Debug.Assert` in `RegisterRepository`. Per project convention, internal API boundaries should use `Debug.Assert` for precondition enforcement without widening to runtime exceptions. The `Debug.Assert` already covers the null check in Unity Editor; the `?? throw` is dead code and inconsistent with the fail-fast contract-checking pattern.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Summary
Validation
Notes