feat: add pause-point watch expressions#1733
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 36 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 (2)
📝 WalkthroughWalkthroughAdds watch expressions backed by dynamic compilation, baseline and per-frame evaluation, bounded history, Unity CLI tools, shared response contracts, tests, and usage documentation. ChangesWatch Expressions
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant WatchUseCase
participant WatchExpressionCompiler
participant WatchExpressionRegistry
participant UnityEditor
CLI->>WatchUseCase: enable-watch
WatchUseCase->>WatchExpressionCompiler: compile expression
WatchExpressionCompiler-->>WatchUseCase: evaluator or diagnostics
WatchUseCase->>WatchExpressionRegistry: register evaluator
WatchExpressionRegistry-->>CLI: baseline WatchResponse
UnityEditor->>WatchExpressionRegistry: editor update
WatchExpressionRegistry-->>CLI: retained frame history via get-watch-values
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Assets/Tests/Editor/WatchExpressionRegistryTests.cs (1)
19-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for duplicate ID rejection.
The PR objectives explicitly list "duplicate ID rejection" as a feature, and
WatchExpressionRegistry.Registerhandles it by returningWatchRegistrationResult.FailureResult($"Watch expression '{id}' is already registered."). However, no test in this file covers that path. Similarly,maxHistoryboundary validation (≤0 or >MaxHistoryLimit) is untested.If these are already covered in
WatchToolTests.csor another file outside this cohort, feel free to disregard. Otherwise, a simple test would close the gap:💚 Suggested test for duplicate ID rejection
+ /// <summary> + /// Verifies registering a duplicate ID is rejected. + /// </summary> + [Test] + public void Register_DuplicateId_ReturnsFailure() + { + FakeWatchEditorStateProvider stateProvider = new(10, true, true); + WatchExpressionRegistry registry = new(stateProvider); + registry.Register("speed", "speed", new SequenceWatchExpressionEvaluator(1), 20); + + WatchRegistrationResult result = registry.Register( + "speed", "speed", new SequenceWatchExpressionEvaluator(2), 20); + + Assert.That(result.Success, Is.False); + Assert.That(result.ErrorMessage, Does.Contain("already registered")); + }🤖 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 `@Assets/Tests/Editor/WatchExpressionRegistryTests.cs` around lines 19 - 35, Extend the tests for WatchExpressionRegistry.Register to cover duplicate ID rejection, asserting the second registration fails with the expected message while preserving the original registration. If not already covered elsewhere, also add boundary tests verifying maxHistory values at or below zero and above MaxHistoryLimit are rejected.Assets/Tests/Editor/WatchToolTests.cs (1)
14-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a happy-path test for
EnableAsync.Current tests only cover validation failures and the empty
GetValuescase. A test that successfully compiles/registers a watch (e.g.Id="speed", Expression="1 + 2") and asserts onWatchResponse.Watcheswould also exercise theRegister→GetEntries().Single()path flagged inWatchTools.cs.🤖 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 `@Assets/Tests/Editor/WatchToolTests.cs` around lines 14 - 71, Add a happy-path test alongside the existing EnableAsync tests that calls WatchUseCase.EnableAsync with a valid Id and expression, asserts the completed response is successful, and verifies WatchResponse.Watches contains the registered watch details. This should exercise the successful Register-to-GetEntries().Single() flow in WatchTools.cli/dispatcher/internal/dispatcher/help_test.go (1)
276-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend help coverage to
clear-watchandget-watch-values.Only
enable-watch --helpis asserted here. Mirroring this test forclear-watch(--id,--all) andget-watch-values(--id) would close the coverage gap for the other two new default watch tools.🤖 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 `@cli/dispatcher/internal/dispatcher/help_test.go` around lines 276 - 292, The help coverage currently only validates enable-watch; extend TestCommandHelpUsesWatchToolSchemaForDefaultWatchCommands to also invoke clear-watch and get-watch-values, asserting successful handling and that clear-watch exposes --id and --all while get-watch-values exposes --id.Assets/Tests/Editor/WatchResponseContractTests.cs (1)
105-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle
JTokenType.DateinNormalizeScalarTypeto avoid imprecise type normalization.
JObject.Parsewith default settings parses ISO 8601 strings (likeEvaluatedAtUtc) asJTokenType.Date, which falls through to the"unknown"default. Both sides currently match because both are parsed identically, but the normalization is semantically incorrect for date-valued fields. AddingJTokenType.Date => "string"makes the intent explicit.♻️ Proposed fix
JTokenType.Boolean => "boolean", JTokenType.Float => "number", JTokenType.Integer => "number", + JTokenType.Date => "string", JTokenType.Null => "null", JTokenType.String => "string", _ => "unknown"🤖 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 `@Assets/Tests/Editor/WatchResponseContractTests.cs` around lines 105 - 116, Update NormalizeScalarType to explicitly map JTokenType.Date to "string", preserving the existing mappings and unknown fallback for all other token types.
🤖 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.
Inline comments:
In `@Assets/Tests/Editor/WatchResponseContractTests.cs`:
- Around line 57-58: Update the test response fixture in
WatchResponseContractTests so CompilationErrors contains at least one populated
WatchCompilationErrorResponse instance. Set its fields using the actual
WatchCompilationErrorResponse type definition, ensuring the shared contract
comparison validates the nested entry shape.
---
Nitpick comments:
In `@Assets/Tests/Editor/WatchExpressionRegistryTests.cs`:
- Around line 19-35: Extend the tests for WatchExpressionRegistry.Register to
cover duplicate ID rejection, asserting the second registration fails with the
expected message while preserving the original registration. If not already
covered elsewhere, also add boundary tests verifying maxHistory values at or
below zero and above MaxHistoryLimit are rejected.
In `@Assets/Tests/Editor/WatchResponseContractTests.cs`:
- Around line 105-116: Update NormalizeScalarType to explicitly map
JTokenType.Date to "string", preserving the existing mappings and unknown
fallback for all other token types.
In `@Assets/Tests/Editor/WatchToolTests.cs`:
- Around line 14-71: Add a happy-path test alongside the existing EnableAsync
tests that calls WatchUseCase.EnableAsync with a valid Id and expression,
asserts the completed response is successful, and verifies WatchResponse.Watches
contains the registered watch details. This should exercise the successful
Register-to-GetEntries().Single() flow in WatchTools.
In `@cli/dispatcher/internal/dispatcher/help_test.go`:
- Around line 276-292: The help coverage currently only validates enable-watch;
extend TestCommandHelpUsesWatchToolSchemaForDefaultWatchCommands to also invoke
clear-watch and get-watch-values, asserting successful handling and that
clear-watch exposes --id and --all while get-watch-values exposes --id.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4d8266f-37d7-427d-b09e-ee98e66ff39e
⛔ Files ignored due to path filters (24)
Assets/Tests/Editor/UnityCLILoop.Tests.Editor.asmdefis excluded by none and included by noneAssets/Tests/Editor/WatchExpressionCompilerTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/WatchExpressionEvaluationTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/WatchExpressionRegistryTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/WatchResponseContractTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/WatchToolTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/UnityCLILoop.FirstPartyTools.Editor.asmdefis excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/AssemblyInfo.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/CompiledWatchExpressionEvaluator.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/IWatchEditorStateProvider.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/IWatchExpressionEvaluator.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/UnityCLILoop.FirstPartyTools.Watch.Editor.asmdefis excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/UnityCLILoop.FirstPartyTools.Watch.Editor.asmdef.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/UnityWatchEditorStateProvider.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchCompilationResult.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchEvaluationResult.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionCompiler.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionEntry.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionHistoryEntry.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionRegistry.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionStepMonitor.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchRegistrationResult.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchTools.cs.metais excluded by none and included by none
📒 Files selected for processing (29)
.agents/skills/uloop-pause-point/SKILL.md.claude/skills/uloop-pause-point/SKILL.mdAssets/Tests/Editor/WatchExpressionCompilerTests.csAssets/Tests/Editor/WatchExpressionEvaluationTests.csAssets/Tests/Editor/WatchExpressionRegistryTests.csAssets/Tests/Editor/WatchResponseContractTests.csAssets/Tests/Editor/WatchToolTests.csPackages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/Watch/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/Watch/CompiledWatchExpressionEvaluator.csPackages/src/Editor/FirstPartyTools/Watch/IWatchEditorStateProvider.csPackages/src/Editor/FirstPartyTools/Watch/IWatchExpressionEvaluator.csPackages/src/Editor/FirstPartyTools/Watch/UnityWatchEditorStateProvider.csPackages/src/Editor/FirstPartyTools/Watch/WatchCompilationResult.csPackages/src/Editor/FirstPartyTools/Watch/WatchEvaluationResult.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionCompiler.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionEntry.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionHistoryEntry.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionRegistry.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionStepMonitor.csPackages/src/Editor/FirstPartyTools/Watch/WatchRegistrationResult.csPackages/src/Editor/FirstPartyTools/Watch/WatchTools.csPackages/src/Editor/ToolContracts/UnityCliLoopConstants.cscli/common/tools/default-tools.jsoncli/dispatcher/internal/dispatcher/help_test.gocli/project-runner/internal/projectrunner/watch_response_contract_test.gocli/project-runner/internal/projectrunner/watch_types.gotests/contracts/watch_response_contract.json
…rage An empty CompilationErrors array left WatchCompilationErrorResponse unverified on both C# and Go contract tests. Use the real Line/Column/Message/ErrorCode fields so the shared fixture exercises the nested shape. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Phase 2 base PR for pause-point watch expressions (
feat/pause-point-watch→v3-beta).enable-watch/clear-watch/get-watch-valuesas plain default-tools (schema-driven help/completion, shared C#/Go response contract)Included work
ec199761)Validation
uloop compile: 0 errors / 0 warningsscripts/check-go-cli.sh: greendist/darwin-arm64/uloop, SimulateMouseDemoScene Update path):System.FormatExceptionwithout stopping StepTest plan
/review/ autoreview cleanMade with Cursor