Repository navigation
Conversation
…aper Lock current Rewrite/Analyze/WrapIfNeeded outputs before splitting the god classes so behavior stays pinned across Extract Class refactors. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract Class on DynamicCodeLiteralHoister, SourceShaper, and SharedRoslynCompilerWorkerHost so each file has one clear responsibility while keeping Rewrite/Analyze/WrapIfNeeded and worker host APIs behaviorally unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 1 minute 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 (3)
📝 WalkthroughWalkthroughChangesDynamic code literal rewriting
Source shaping scanner extraction
Shared Roslyn worker lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
Add characterization coverage for terminated and unterminated block comments around the rewritten TryCopy* path, and move SkipWhitespace into SourceTokenScanner so scanning no longer depends back on SourceShaper. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeLiteralHoisterCharacterizationTests.cs (1)
10-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd characterization coverage for escape variants and comments.
Existing tests cover ints/strings/verbatim/char/interpolated/decimal/comments, but the newly extracted
DynamicCodeRegularStringLiteralUnescaperandDynamicCodeLiteralSyntaxScanneralso handle\x,\u,\Uescapes, block comments, and nested-brace interpolation expressions — none of which are pinned here. Given this PR's stated goal is to lock existing behavior before splitting these classes, a few more cases (e.g."\u0041","\x41",/* block */,$"{new[]{1,2}}") would strengthen the safety net for exactly the logic that moved.🤖 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/DynamicCodeToolTests/DynamicCodeLiteralHoisterCharacterizationTests.cs` around lines 10 - 194, Add characterization tests in DynamicCodeLiteralHoisterCharacterizationTests covering regular string literals with \x, \u, and \U escapes and asserting their unescaped binding values; add a block-comment case confirming comments remain intact while literals are hoisted; and add a nested-brace interpolated-string case confirming the complete interpolation remains opaque with no bindings.Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceTokenScanner.cs (1)
33-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate comment-skipping logic between
TryMatch*andTryAdvance*.
TryMatchLineComment/TryMatchBlockComment(Lines 33-58) reimplement the exact same scanning logic as the privateTryAdvanceLineComment/TryAdvanceBlockComment(Lines 156-178), just with an out-param instead of a tuple return. Two independent implementations of the same rule invite silent divergence during future edits.♻️ Suggested consolidation
internal static bool TryMatchLineComment(string s, int pos, out int afterComment) { - afterComment = pos; - if (pos + 1 < s.Length && s[pos] == '/' && s[pos + 1] == '/') - { - int end = pos + 2; - while (end < s.Length && s[end] != '\n') end++; - if (end < s.Length) end++; // skip \n - afterComment = end; - return true; - } - return false; + (bool matched, int nextPosition) = TryAdvanceLineComment(s, pos); + afterComment = matched ? nextPosition : pos; + return matched; }Apply the same pattern to
TryMatchBlockComment/TryAdvanceBlockComment.Also applies to: 134-178
🤖 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/ExecuteDynamicCode/DynamicCompilation/SourceTokenScanner.cs` around lines 33 - 58, Consolidate the duplicate scanning logic by updating TryMatchLineComment and TryMatchBlockComment to reuse the corresponding TryAdvanceLineComment and TryAdvanceBlockComment implementations, adapting their tuple results to the existing out-parameter contract. Remove the independent comment-scanning loops while preserving the current success/failure behavior and computed positions.
🤖 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
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceTokenScanner.cs`:
- Around line 10-31: Move the pure whitespace-scanning implementation into
SourceTokenScanner and update SkipWhitespaceAndComments to call that local
primitive instead of SourceShaper.SkipWhitespace. Retain
SourceShaper.SkipWhitespace as a thin delegate to SourceTokenScanner so existing
callers remain unchanged while the dependency points only from SourceShaper to
the low-level scanner.
- Around line 134-268: Update AdvanceOneToken and the interpolated-string
scanning helpers to recognize combined prefixes $@"...", @$", and raw
interpolated forms with one or more $ characters before triple quotes. Route
verbatim forms through verbatim quote semantics and raw forms through the
appropriate raw interpolated scanner, ensuring each token stops at its actual
closing delimiter while preserving existing handling for regular interpolated
strings.
---
Nitpick comments:
In
`@Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeLiteralHoisterCharacterizationTests.cs`:
- Around line 10-194: Add characterization tests in
DynamicCodeLiteralHoisterCharacterizationTests covering regular string literals
with \x, \u, and \U escapes and asserting their unescaped binding values; add a
block-comment case confirming comments remain intact while literals are hoisted;
and add a nested-brace interpolated-string case confirming the complete
interpolation remains opaque with no bindings.
In
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceTokenScanner.cs`:
- Around line 33-58: Consolidate the duplicate scanning logic by updating
TryMatchLineComment and TryMatchBlockComment to reuse the corresponding
TryAdvanceLineComment and TryAdvanceBlockComment implementations, adapting their
tuple results to the existing out-parameter contract. Remove the independent
comment-scanning loops while preserving the current success/failure behavior and
computed positions.
🪄 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: ef322649-f4fe-4b54-beea-c07d8a89543f
⛔ Files ignored due to path filters (7)
Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeLiteralHoisterCharacterizationTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/DynamicCodeToolTests/SourceShaperCharacterizationTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeLiteralSyntaxScanner.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeRegularStringLiteralUnescaper.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHostProcess.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHostResults.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceTokenScanner.cs.metais excluded by none and included by none
📒 Files selected for processing (10)
Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeLiteralHoisterCharacterizationTests.csAssets/Tests/Editor/DynamicCodeToolTests/SourceShaperCharacterizationTests.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeLiteralHoister.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeLiteralSyntaxScanner.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeRegularStringLiteralUnescaper.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHost.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHostProcess.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SharedRoslynCompilerWorkerHostResults.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceShaper.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceTokenScanner.cs
Review disposition (merge gate)
Merging to |
8027c30
into
feature/god-class-split-integration
1 participant
Summary
User Impact
Changes
DynamicCodeLiteralHoister→ hoisting orchestration +DynamicCodeLiteralSyntaxScanner+DynamicCodeRegularStringLiteralUnescaperSourceShaper→ structure analysis/wrap +SourceTokenScannerSharedRoslynCompilerWorkerHost→ orchestration +SharedRoslynCompilerWorkerHostProcess+SharedRoslynCompilerWorkerHostResultsVerification
dist/darwin-arm64/uloop compile→ 0 errors, 0 warningswc -lon target + extracted files: all < 500uloop run-testsEditMode filter DynamicCode/SourceShaper/SharedRoslynCompilerWorker… → 232 passeddotnet test tests/UnityCliLoop.CodeComplexity.Tests→ 12 passedMade with Cursor