fix(build): skip never-saved scenes instead of opening the save modal - #1356
Conversation
CoplayDev#1340 added SaveBeforeBuild() so manage_build would not block on Unity's unsaved-scene dialog, but it called EditorSceneManager.SaveOpenScenes() unconditionally. A scene that has never been saved carries an empty path, and handing one to Unity's save API opens the modal "Save Scene" file panel -- the exact block the change was meant to prevent. The warning it logs fires only after SaveOpenScenes() returns, which is after the modal has already stalled the main thread. Saves dirty scenes individually and skips pathless ones with a warning, mirroring the guard TestRunnerService.SaveDirtyScenes has carried since it hit the same problem. ManageScene refuses the same case at ManageScene.cs:492. Follow-up to CoplayDev#1340. Refs CoplayDev#1341
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesBuild scene save handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Builds will no longer open a blocking save dialog for never-saved scenes; those scenes remain unsaved with a warning while eligible scenes continue to save. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides strong technical context, implementation details, and test results, but it does not follow the repository template. It omits the required section headings and checklist details for change type, compatibility/package source, testing, documentation updates, related issues, and additional notes. Resolution Reformat the description using the repository template. Mark Bug fix and Test update, provide Unity versions and package source details or state that they are not applicable, mark the Unity EditMode and compile checks, document the documentation-update status, add formal related-issue references such as "Fixes Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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.
Pull request overview
Updates the Unity-side BuildRunner.SaveBeforeBuild() logic used by the manage_build tool so automated builds don’t trigger Unity’s modal “Save Scene” file picker when an open dirty scene has never been saved (empty scene.path). Adds EditMode regression tests to pin the behavior.
Changes:
- Change
SaveBeforeBuild()to save dirty scenes individually and skip pathless (never-saved) scenes with a warning. - Expose
SaveBeforeBuild()asinternalfor direct EditMode test coverage. - Add new EditMode tests asserting untitled scenes remain dirty/pathless after
SaveBeforeBuild().
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
MCPForUnity/Editor/Tools/Build/BuildRunner.cs |
Adjusts pre-build save behavior to avoid opening modal save dialogs for never-saved scenes. |
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildSaveBeforeBuildTests.cs |
Adds regression tests covering the new guard and non-throw behavior. |
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildSaveBeforeBuildTests.cs.meta |
Adds Unity meta for the new test asset. |
Files not reviewed (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildSaveBeforeBuildTests.cs.meta: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// a save dialog. Scenes that have never been saved carry an empty path, and handing one | ||
| /// to Unity's save API opens the modal "Save Scene" file panel — the exact block this is | ||
| /// meant to prevent — so those are skipped with a warning instead. Mirrors the guard in | ||
| /// TestRunnerService.SaveDirtyScenes. |
| try | ||
| { | ||
| EditorSceneManager.SaveScene(scene); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| McpLog.Warn($"[MCP Build] Failed to save dirty scene '{scene.name}': {ex.Message}"); | ||
| } |
| } | ||
|
|
||
| [Test] | ||
| public void SaveBeforeBuild_WithNoDirtyScenes_IsANoOp() |
Follow-up to #1340, which landed in
betaat 498acd1.#1340 added
SaveBeforeBuild()somanage_buildwould stop blocking on Unity's unsaved-scene dialog (#1341). The intent is right, but it callsEditorSceneManager.SaveOpenScenes()unconditionally:A scene that has never been saved carries an empty path, and handing one to Unity's save API opens the modal "Save Scene" file panel — the exact block this change exists to prevent. The warning cannot help: it runs after
SaveOpenScenes()returns, which is after the modal has already stalled the main thread.Why this is reachable
An untitled scene is routine in agent-driven workflows —
EditorSceneManager.NewSceneviaexecute_code, or a plain Ctrl+N — and the next instruction is often "build it".The repo already solved this twice
TestRunnerService.SaveDirtySceneshit the same problem and guards it:ManageScene.cs:492refuses the same case: "Cannot save an untitled scene without providing a 'name' and 'path'."This change makes
BuildRunnerconsistent with both: save dirty scenes individually, skip pathless ones with a warning that names the scene, and keep a per-scene try/catch so one failure does not abort the rest.Testing
BuildSaveBeforeBuildTests.cs. The sharp assertion is that an untitled scene is left dirty and path-less afterSaveBeforeBuild()— a saved scene would have neither, so this fails if the unconditional save ever comes back.SaveBeforeBuildchanged fromprivatetointernalso the test assembly can call it directly;MCPForUnity.Editoralready hasInternalsVisibleTo("MCPForUnityTests.EditMode").cc @TeapoyY — thanks for the original fix, this only adds the pathless guard on top of it.
Summary by CodeRabbit
Bug Fixes
Tests