fix: Migration wizard now opens reliably right after a compile-error restart#1955
Merged
hatayama merged 11 commits intoJul 23, 2026
Merged
Conversation
… of always scanning (#1954)
… dialog EditorApplication.delayCall permanently stops flushing for the rest of an Editor process once the native "Scripts have compiler errors" dialog has appeared at cold startup (-ignorecompilererrors does not suppress this dialog). Since that startup scenario is exactly when the compile-error-driven migration auto-scan needs to run, both the SetupWizard version-change check and the migration auto-scan trigger now ride on a self-unsubscribing EditorApplication.update tick / poll loop instead of delayCall. - SetupWizardWindow.InitializeForEditorStartup now registers a one-shot EditorApplication.update callback instead of delayCall. - SetupWizardStartupFlow.MaybeScheduleThirdPartyToolMigrationAutoScan polls compile state via EditorApplication.update (DecideMigrationAutoScanPollAction) and falls back to a full project scan if compilation stays failed past a timeout (SetupWizardStartupFlowConstants.MigrationAutoScanPollTimeoutSeconds). - ThirdPartyToolMigrationFileService now reads error logs through LogGetter instead of ConsoleLogRetriever directly. - Updated OnionAssemblyDependencyTests to assert the new EditorApplication.update-based startup wiring and the Infrastructure assembly's new Common.Console reference. Verified live via a cold Editor restart with pre-existing compile errors: the migration wizard now opens and completes detection almost instantly, where it previously never fired.
csc.rsp is the response file, not the compiler itself; the comment should refer to csc diagnostics. Pointed out during advisor review.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe migration auto-scan flow detects legacy APIs from compile errors, polls through ChangesThird-party migration auto-scan
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UnityEditor
participant SetupWizardStartupFlow
participant MigrationUseCase
participant SeedRepository
participant MigrationWizard
UnityEditor->>SetupWizardStartupFlow: invoke update polling callback
SetupWizardStartupFlow->>MigrationUseCase: detect targets from compile errors
MigrationUseCase-->>SetupWizardStartupFlow: return target file paths
SetupWizardStartupFlow->>SeedRepository: store seed paths
SetupWizardStartupFlow->>MigrationWizard: open auto-scan window
MigrationWizard->>SeedRepository: read and clear seed paths
MigrationWizard->>MigrationWizard: show detected migration state
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs (1)
231-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve auto-scan seeds when reusing an open wizard.
At Line 235, the existing-window branch ignores
seedFilePaths. They were already consumed and cleared at Lines 69-78, so the subsequent refresh runs with the controller’s old/empty seeds and falls back to a full-project scan. Pass the new seeds into the existing window/controller beforeTryStartInitialRefresh().🤖 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/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs` around lines 231 - 245, Update the existing-window branch in ShowWindowInternal to pass seedFilePaths into the already open wizard and its controller before FocusExistingWindow triggers TryStartInitialRefresh(). Preserve the current focus-and-return flow while ensuring the refresh uses the new auto-scan seeds instead of falling back to a full-project scan.
🧹 Nitpick comments (1)
Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs (1)
55-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s assert-only programmer-precondition convention. The new migration seed path adds explicit runtime null exceptions where internal invalid inputs are intended to fail fast through
Debug.Assert.
Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs#L55-L76: retain the assertions and assign the new dependencies directly rather than adding?? throw.Packages/src/Editor/Infrastructure/Settings/UnityCliLoopThirdPartyToolMigrationAutoScanSeedRepository.cs#L18-L23: add aDebug.Assert(filePaths != null, ...)precondition and remove the explicitArgumentNullExceptionbranch.Based on learnings: invalid internal inputs are programmer errors and should use
Debug.Assertrather than explicit runtime exceptions.🤖 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/Presentation/Setup/SetupWizardStartupFlow.cs` around lines 55 - 76, The new internal dependencies should follow the repository’s assert-only precondition convention. In Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs lines 55-76, retain the Debug.Assert checks and assign dependencies directly without ?? throw or ArgumentNullException handling. In Packages/src/Editor/Infrastructure/Settings/UnityCliLoopThirdPartyToolMigrationAutoScanSeedRepository.cs lines 18-23, add a Debug.Assert precondition for filePaths and remove the explicit ArgumentNullException branch.Source: Learnings
🤖 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/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationLightweightAssemblyWalker.cs`:
- Around line 20-50: Update DiscoverAssemblyStructure to replace both raw
recursive Directory.GetFiles calls with a guarded directory traversal helper.
Add a helper that mirrors ThirdPartyToolMigrationProjectFileInventory’s walk,
checks
ThirdPartyToolMigrationProjectFileInventory.ShouldExcludeDirectory(projectRoot,
childDirectoryPath) before descending, and skips reparse-point or excluded
directories while collecting matching asmdef and asmref files.
In `@Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs`:
- Around line 386-397: Update the MigrationAutoScanPollAction handler in the
RunDetection and FallBackToFullScan cases to guarantee polling cleanup on
terminal actions. Wrap TryRunThirdPartyToolMigrationAutoScanDetection in
try-finally so StopThirdPartyToolMigrationAutoScanPolling always executes while
exceptions propagate, and unsubscribe before calling
ScheduleThirdPartyToolMigrationFallbackFullScan.
---
Outside diff comments:
In
`@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs`:
- Around line 231-245: Update the existing-window branch in ShowWindowInternal
to pass seedFilePaths into the already open wizard and its controller before
FocusExistingWindow triggers TryStartInitialRefresh(). Preserve the current
focus-and-return flow while ensuring the refresh uses the new auto-scan seeds
instead of falling back to a full-project scan.
---
Nitpick comments:
In `@Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs`:
- Around line 55-76: The new internal dependencies should follow the
repository’s assert-only precondition convention. In
Packages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.cs lines 55-76,
retain the Debug.Assert checks and assign dependencies directly without ?? throw
or ArgumentNullException handling. In
Packages/src/Editor/Infrastructure/Settings/UnityCliLoopThirdPartyToolMigrationAutoScanSeedRepository.cs
lines 18-23, add a Debug.Assert precondition for filePaths and remove the
explicit ArgumentNullException branch.
🪄 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 Plus
Run ID: 3b5f9e3f-6851-477c-a15d-cbb5f6432801
⛔ Files ignored due to path filters (15)
Assets/Tests/Editor/ThirdPartyToolMigrationCompileErrorLogMatcherTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationConsoleErrorParserTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationDetectionRulesTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationLightweightAssemblyWalkerTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationScanScopeResolverTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationScopedPreviewTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationWizardWorkflowControllerTests.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/IThirdPartyToolMigrationAutoScanSeedRepository.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/Settings/UnityCliLoopThirdPartyToolMigrationAutoScanSeedRepository.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationCompileErrorLogMatcher.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationConsoleErrorParser.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationLightweightAssemblyWalker.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationScanScopeResolver.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/UnityCLILoop.Infrastructure.asmdefis excluded by none and included by nonePackages/src/Editor/Presentation/Setup/SetupWizardStartupFlowConstants.cs.metais excluded by none and included by none
📒 Files selected for processing (29)
Assets/Tests/Editor/OnionAssemblyDependencyTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/ThirdPartyToolMigrationCompileErrorLogMatcherTests.csAssets/Tests/Editor/ThirdPartyToolMigrationConsoleErrorParserTests.csAssets/Tests/Editor/ThirdPartyToolMigrationDetectionRulesTests.csAssets/Tests/Editor/ThirdPartyToolMigrationLightweightAssemblyWalkerTests.csAssets/Tests/Editor/ThirdPartyToolMigrationScanScopeResolverTests.csAssets/Tests/Editor/ThirdPartyToolMigrationScopedPreviewTests.csAssets/Tests/Editor/ThirdPartyToolMigrationWizardWorkflowControllerTests.csPackages/src/Editor/Application/UseCases/ThirdPartyToolMigrationUseCase.csPackages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.csPackages/src/Editor/CompositionRoot/UnityCliLoopEditorBootstrapper.csPackages/src/Editor/Domain/IThirdPartyToolMigrationAutoScanSeedRepository.csPackages/src/Editor/Domain/ThirdPartyToolMigrationData.csPackages/src/Editor/Infrastructure/Settings/UnityCliLoopThirdPartyToolMigrationAutoScanSeedRepository.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationCompileErrorLogMatcher.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationConsoleErrorParser.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationDetectionRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationLightweightAssemblyWalker.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationPlanBuilder.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationProjectFileInventory.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationScanScopeResolver.csPackages/src/Editor/Presentation/PresentationEditorStartup.csPackages/src/Editor/Presentation/Setup/SetupWizardStartupFlow.csPackages/src/Editor/Presentation/Setup/SetupWizardStartupFlowConstants.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWorkflowController.cs
…ning Manual verification found that clicking "Migrate" after the fast auto-scan detection triggered a second, slower, full-project scan, hurting perceived responsiveness. Root cause: the scoped preview never populated the full-scan plan cache (by design, since a scope-limited plan is unsafe to substitute for the full plan), so ApplyMigration always fell back to a fresh full scan. Per the user's decision (after advisor consultation confirming Migrate must keep doing a full scan for correctness — cascading compile-skip across dependent assemblies could otherwise hide legitimate migration targets), the fix is to stop scanning at auto-scan-open time entirely: the window now displays the compile-error-matched seed file list directly (already known for free from log matching), with Migrate remaining the sole scan+rewrite step. This removes the now-unused scoped-scan infrastructure (ScanScopeResolver, LightweightAssemblyWalker, PreviewMigrationForSeedFilesAsync, PreviewMigrationInScopeAsync, PlanBuilder.CreateInScopeAsync, ProjectFileInventory.CreateFromDirectoriesAsync) and their dedicated tests.
The advisor review found two issues in the zero-scan auto-detection change: "legacy V3 custom tool API" had the migration direction backwards (V2 is legacy, V3 is the target), and the Migrate confirm dialog showed the seed-derived file count even when migrating directly from the auto-scan-detected state, understating scope since a cascading compile-skip can make the real full scan touch more files than the compile-error seeds ever revealed. Corrected the wording in ThirdPartyToolMigrationWizardText, and added GetMigrationConfirmDialogFileCount (StateRules + Window wrapper) so the confirm dialog only shows an exact count when it came from a verified full-project scan, falling back to the generic message otherwise.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs (1)
231-247: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve seeds when focusing an already-open window.
ShowWindowInternalconsumesseedFilePathsbut drops them when an instance already exists.FocusExistingWindowthen renders using the old window list, so a manually opened wizard can show zero or stale files and the consumed seeds cannot be recovered.Proposed fix
- FocusExistingWindow(shouldRefreshAfterCreateGui); + FocusExistingWindow(shouldRefreshAfterCreateGui, seedFilePaths); -private static void FocusExistingWindow(bool shouldRefreshAfterCreateGui) +private static void FocusExistingWindow( + bool shouldRefreshAfterCreateGui, + List<string> seedFilePaths) { ... if (!shouldRefreshAfterCreateGui) { return; } + window._autoScanSeedFilePaths = seedFilePaths; window.TryShowAutoScanDetectedState(); }Also applies to: 310-310
🤖 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/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs` around lines 231 - 247, Update ShowWindowInternal so seedFilePaths are applied to the existing ThirdPartyToolMigrationWizardWindow before calling FocusExistingWindow, preserving the supplied seeds when the wizard is already open. Keep the current new-window initialization through _autoScanSeedFilePaths unchanged.
🧹 Nitpick comments (1)
Assets/Tests/Editor/ThirdPartyToolMigrationWizardWorkflowControllerTests.cs (1)
23-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the seeded detected state is actually rendered.
This test only proves that no preview scan runs. A regression that ignores the seed paths or renders the NotChecked state would still pass. Assert the status text/state through the constructed UI root, including the expected detected file count.
🤖 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/ThirdPartyToolMigrationWizardWorkflowControllerTests.cs` around lines 23 - 34, Extend ShowInitialState_WhenShouldShowAutoScanDetectedState_DoesNotTriggerAnyPreviewCall to inspect the constructed UI root after ShowInitialState, asserting that the auto-scan detected state is rendered with the seeded detected-file count of one. Keep the existing PreviewMigrationAsyncCallCount assertion and use the controller’s existing UI/status symbols rather than validating only the migration port.
🤖 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/Presentation/Setup/ThirdPartyToolMigrationWizardText.cs`:
- Around line 57-76: Update GetAutoScanDetectedStatusText and its summary to use
“legacy V2” for compile-error detections. For the fileCount == 0 fallback, use
neutral wording that does not claim a compile-error match while still
instructing the user to click Migrate to scan and update the project; retain the
existing singular/plural wording for known file counts.
- Around line 78-87: Update GetMigrationConfirmDialogMessage so the nonzero
fileCount path uses the generic scan-and-rewrite confirmation rather than
presenting fileCount as the exact rewrite count. Preserve the existing zero-file
message and fileCount validation, while ensuring seeded compile-error state does
not promise an exact number of rewritten files.
In
`@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWorkflowController.cs`:
- Around line 87-93: The ShowAutoScanDetectedState method should not assign
seedFilePaths to _pendingMigrationFilePaths, since those paths are only
preliminary matches and can undercount the later full scan. Track the seeded
detection state separately, keep the confirmation dialog on the generic message
until HandleMigrateThirdPartyTools completes its full scan, and reserve
_pendingMigrationFilePaths for confirmed scan results.
---
Outside diff comments:
In
`@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs`:
- Around line 231-247: Update ShowWindowInternal so seedFilePaths are applied to
the existing ThirdPartyToolMigrationWizardWindow before calling
FocusExistingWindow, preserving the supplied seeds when the wizard is already
open. Keep the current new-window initialization through _autoScanSeedFilePaths
unchanged.
---
Nitpick comments:
In `@Assets/Tests/Editor/ThirdPartyToolMigrationWizardWorkflowControllerTests.cs`:
- Around line 23-34: Extend
ShowInitialState_WhenShouldShowAutoScanDetectedState_DoesNotTriggerAnyPreviewCall
to inspect the constructed UI root after ShowInitialState, asserting that the
auto-scan detected state is rendered with the seeded detected-file count of one.
Keep the existing PreviewMigrationAsyncCallCount assertion and use the
controller’s existing UI/status symbols rather than validating only the
migration port.
🪄 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 Plus
Run ID: 9b479e0b-26a9-4c76-8db3-00730c85a110
📒 Files selected for processing (10)
Assets/Tests/Editor/ThirdPartyToolMigrationWizardWorkflowControllerTests.csPackages/src/Editor/Application/UseCases/ThirdPartyToolMigrationUseCase.csPackages/src/Editor/Domain/ThirdPartyToolMigrationData.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationPlanBuilder.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationProjectFileInventory.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardText.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardView.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWorkflowController.cs
💤 Files with no reviewable changes (4)
- Packages/src/Editor/Domain/ThirdPartyToolMigrationData.cs
- Packages/src/Editor/Application/UseCases/ThirdPartyToolMigrationUseCase.cs
- Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.cs
- Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationPlanBuilder.cs
ProjectFileInventory reports TotalItemCount as 0 while it is still counting files (the real total isn't known yet). The progress bar fell back to a highValue of 1 for that unknown-total case, which clamped any nonzero inspected-file count up to 1 — so the bar rendered as fully filled the moment Migrate was clicked, well before the real scan (with a known total) began. Added GetMigrationProgressBarRange (StateRules + Window wrapper, matching this file's existing pure-rule pattern) so the bar reports empty progress until a real total is known, instead of a false 100%.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs (1)
249-264: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve auto-scan seeds when focusing an existing window.
ShowWindowForAutoScanconsumes and clears the repository before this method runs. If a wizard instance already exists, this branch callsFocusExistingWindowwithout assigningseedFilePaths, soTryShowAutoScanDetectedStaterenders the old list—often empty—and the newly detected files are lost.Pass the seeds into
FocusExistingWindowand assign them before invoking the controller; add a regression test for the already-open-window path.Suggested fix
private static void ShowWindowInternal(bool shouldRefreshAfterCreateGui, List<string> seedFilePaths) { if (HasOpenInstances<ThirdPartyToolMigrationWizardWindow>()) { - FocusExistingWindow(shouldRefreshAfterCreateGui); + FocusExistingWindow(shouldRefreshAfterCreateGui, seedFilePaths); return; } -private static void FocusExistingWindow(bool shouldRefreshAfterCreateGui) +private static void FocusExistingWindow( + bool shouldRefreshAfterCreateGui, + List<string> seedFilePaths) { ... if (!shouldRefreshAfterCreateGui) { return; } + window._autoScanSeedFilePaths = new List<string>(seedFilePaths); window._shouldRefreshAfterCreateGui = true; window.TryShowAutoScanDetectedState(); }Also applies to: 328-328
🤖 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/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs` around lines 249 - 264, Update ShowWindowInternal and FocusExistingWindow so seedFilePaths are passed through the already-open-window branch and assigned to the existing wizard instance before invoking its controller. Preserve the new-window assignment behavior, and add a regression test covering ShowWindowForAutoScan when a wizard is already open, verifying the detected seeds are rendered.
🤖 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.
Outside diff comments:
In
`@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs`:
- Around line 249-264: Update ShowWindowInternal and FocusExistingWindow so
seedFilePaths are passed through the already-open-window branch and assigned to
the existing wizard instance before invoking its controller. Preserve the
new-window assignment behavior, and add a regression test covering
ShowWindowForAutoScan when a wizard is already open, verifying the detected
seeds are rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b23ce967-b9f1-49f5-a776-d263f7114a3c
📒 Files selected for processing (6)
Assets/Tests/Editor/ThirdPartyToolMigrationWizardWindowTests.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardStateRules.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardText.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardView.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWorkflowController.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardText.cs
- Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardView.cs
After a successful Migrate, the window showed a generic "Nothing to migrate" text that read as an idle/unchanged state, ambiguous with the background compile Migrate had just triggered via AssetDatabase.Refresh(). It also left both Migrate and Check simultaneously enabled in the auto-scan-detected state, letting either be clicked at once, and disabled buttons only looked like a dimmed green rather than a clear gray. - Add ShowMigrationCompleteState with distinct "Migration complete" wording, used only for the post-Migrate path (the manual-Check no-targets path keeps its existing generic text) - Disable the Check button in ShowAutoScanDetectedState while Migrate is active, mirroring ShowMigrationTargetsState's existing convention - Add a `.setup-button--primary:disabled` style matching the Settings window's gray disabled-button look
… window ShowWindowInternal's existing-window branch called FocusExistingWindow without the newly detected seedFilePaths, so a re-triggered auto-scan while the wizard was already open re-rendered the controller's stale constructor-time seed list (or an empty one) instead of the fresh detection. Flagged by CodeRabbit as a functional-correctness issue on PR #1955; confirmed still present and fixed before merge. - Thread seedFilePaths through FocusExistingWindow and Window.TryShowAutoScanDetectedState into the controller - Change WorkflowController.TryShowAutoScanDetectedState to render the seeds passed to the call instead of the constructor-captured field - Add a test asserting the freshly passed seeds are what gets rendered
Contributor
✅ Action performedReview finished.
|
Repository owner
deleted a comment from
qua-hatayama
Jul 23, 2026
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
PollThirdPartyToolMigrationAutoScan's RunDetection case only stopped polling on a successful detection. If TryRunThirdPartyToolMigrationAutoScanDetection threw, the EditorApplication.update subscription stayed registered and would retry the same throwing call every editor frame. Also reordered FallBackToFullScan to unsubscribe before scheduling the fallback scan, matching the same cleanup-first ordering. Flagged by CodeRabbit on PR #1955.
Owner
Author
|
Addressed the outside-diff finding (auto-scan seeds dropped when reusing an already-open wizard window, Also fixed the exception-safety issue on the auto-scan poll callback (SetupWizardStartupFlow.cs:386-397) in commit 0e07476, per your suggested try-finally ordering. |
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
User Impact
Changes
EditorApplication.delayCallstops firing for the rest of that process's lifetime (confirmed via repeated live probes;EditorApplication.updatekeeps working normally). Since compile-error-driven detection needs to run in exactly this scenario, both the Setup Wizard startup check and the migration auto-scan trigger were moved fromdelayCallto a self-unsubscribingEditorApplication.updatetick / poll loop, removing this dependency from the startup critical path entirely.get-logstool, so compiler diagnostics are classified consistently.Verification
uloop compile: 0 errors, 0 warnings.uloop run-tests(full suite, 2336 tests): 2325 passed, 4 failed, 7 skipped. The 4 failures are pre-existing and unrelated to this change (confirmed identical on the currentv3-betabaseline, touching unrelated files/mechanisms).uloop launch -r):lastSeenSetupWizardVersion: migration wizard opens automatically and immediately lists the correct affected files, with no scan delay.lastSeenSetupWizardVersionon the same major version: no regression — the migration wizard does not spuriously appear,lastSeenVersionupdates correctly, and there is no duplicate/double-firing of the startup check.Known limitations
Manual verification steps for reviewers
.csfile underAssets/that references a legacy (pre-migration) third-party tool API, to force a compile error.