refactor(mcp): extract TransientImportSession; restore selection in all transient-import tools#572
Conversation
…ll transient-import tools Three MCP tools (`apply_atlas`, `bake_vat`, `list_morph_targets`) needed the same pattern: - Snapshot the pre-import entity set + the user's selection. - Run `MeshImporterExporter::importer(...)`. - Diff to find the imported entities. - On scope exit: destroy the imported scene nodes AND restore the prior selection. PR #571 fixed the selection-restore hole in `list_morph_targets` only; `bake_vat` and `apply_atlas` had the same bug (`MeshImporterExporter::importer` auto-selects the new entities; my cleanup destroyed them, leaving the SelectionSet holding dangling pointers). Extract the whole pattern into a `TransientImportSession` RAII helper (anonymous namespace, MCPServer.cpp) and migrate the three tools to use it. Each tool's body drops from ~50 lines of boilerplate to: ```cpp TransientImportSession session(mgr); if (QString err = session.runImporter(filePath); !err.isEmpty()) return makeErrorResult(err); const auto& imported = session.importedEntities(); ``` The helper: - Filters `getEntities()` by `getMovableType() == "Entity"` — gizmos / brush rings / mask overlays would otherwise crash on the raw cast. - Uses `SelectionSet::clearList()` (not `clear()`) in the destructor — the importer-added selection entries point at entities we're about to destroy; `clear()` would dereference freed memory while bookkeeping. `toolOptimizeMesh` is intentionally NOT migrated in this PR — it passes an extra `animOnlySkeletons` out-parameter to `MeshImporterExporter::importer`, which the helper's `runImporter()` doesn't yet support. That's a separate small extension when we touch optimize-mesh next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughA new ChangesMesh Import Transient Session
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 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 `@src/MCPServer.cpp`:
- Around line 441-455: runImporter currently returns early when
MeshImporterExporter::importer throws, so newly created entities are not
recorded and remain in the live scene; modify the exception handling in
runImporter (around the MeshImporterExporter::importer call) to call
collectEntitiesSafe() and append any entities not present in m_beforeSet to
m_imported before returning from both the std::exception and catch-all handlers,
ensuring the same post-import bookkeeping (the loop using collectEntitiesSafe(),
m_beforeSet and m_imported) runs on the error path as well as the success path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
CodeRabbit major on PR #572: If `MeshImporterExporter::importer()` throws partway through, the prior `runImporter()` returned the error without recording what the importer managed to create — leaving partial state (scene nodes, entities) in the live scene that the destructor never cleaned up. That broke the transient-import contract on the error path. Fix: extract the post-import "diff `getEntities()` against `m_beforeSet`" logic into `captureImportedEntities()` and call it from all three exit paths (success + both catch blocks). The helper dedups against `m_imported` so callers running it twice is safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Three MCP tools — `apply_atlas`, `bake_vat`, `list_morph_targets` — needed the same boilerplate around `MeshImporterExporter::importer`:
PR #571 fixed the selection-restore hole in `list_morph_targets` only. `bake_vat` and `apply_atlas` have the same bug: `MeshImporterExporter::importer` auto-selects the new entities, so the destructor's `destroySceneNode` calls leave the `SelectionSet` holding dangling pointers. Any subsequent selection-touching code dereferences freed memory.
What this PR does
Extract the whole pattern into a `TransientImportSession` RAII helper (anonymous namespace, MCPServer.cpp) and migrate all three tools.
Each tool's body drops from ~50 lines of boilerplate to:
```cpp
TransientImportSession session(mgr);
if (QString err = session.runImporter(filePath); !err.isEmpty())
return makeErrorResult(err);
const auto& imported = session.importedEntities();
```
The helper:
Net −32 lines across the three tools after the extraction.
Out of scope
`toolOptimizeMesh` is NOT migrated. It passes an extra `animOnlySkeletons` out-parameter to `MeshImporterExporter::importer` which `runImporter()` doesn't yet support. Will land as a separate small extension when we touch optimize-mesh next.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit