Reduce duplication in gizmos/MCP + raise transform test coverage - #277
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 26 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors CI WinGet step, centralizes CLI extension-to-format mapping, introduces GizmoAxisHelpers with gizmo refactors, centralizes MCPServer tool dispatch, simplifies PrimitivesWidget UI logic, and adds/adjusts broad unit tests across multiple components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
fernandotonon
marked this pull request as ready for review
April 10, 2026 21:52
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline_test.cpp (1)
1093-1125:⚠️ Potential issue | 🟠 MajorThis test encodes the wrong zero-animation contract.
When a skeleton exists but
getNumAnimations() == 0,CLIPipeline::cmdAnim(..., --list)is supposed to succeed and emitNo animations found.for text or[]for JSON. Expecting1here will fail against the current behavior and regress the contract this suite is meant to preserve.Proposed fix
- EXPECT_EQ(CLIPipeline::cmdAnim(textArgs.argc(), textArgs.argv()), 1); + EXPECT_EQ(CLIPipeline::cmdAnim(textArgs.argc(), textArgs.argv()), 0); - EXPECT_EQ(CLIPipeline::cmdAnim(jsonArgs.argc(), jsonArgs.argv()), 1); + EXPECT_EQ(CLIPipeline::cmdAnim(jsonArgs.argc(), jsonArgs.argv()), 0);Based on learnings, "In src/CLIPipeline.cpp, CLIPipeline::cmdAnim list mode returns 0 (success) when a skeleton is present but has zero animations, emitting "No animations found." for text output or "[]" for --json. Tests should expect success for this case."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 1093 - 1125, The test CmdAnimList_NoAnimationsGeneratedMeshReturnsError asserts the wrong exit code for CLIPipeline::cmdAnim in list mode when a skeleton exists but has zero animations; change the two EXPECT_EQ assertions that currently expect 1 (for textArgs and jsonArgs) to expect 0 instead so the test matches the intended contract (CLIPipeline::cmdAnim(..., "--list") returns success and emits "No animations found." or "[]").
🧹 Nitpick comments (1)
src/MCPServer_test.cpp (1)
2902-2902: Avoid pinning an exact tool count in this dynamic-recognition test.Line 2902 (
EXPECT_EQ(tools.size(), 51)) will create churn for legitimate tool additions/removals and weakens the goal of this test (recognition of listed tools). Prefer asserting non-empty (or a lower bound) and keep the per-tool recognition loop as the main contract.♻️ Suggested adjustment
- EXPECT_EQ(tools.size(), 51); + ASSERT_FALSE(tools.isEmpty());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` at line 2902, The test currently pins the exact tool count with EXPECT_EQ(tools.size(), 51) which causes churn; change that assertion to assert non-empty or a sensible lower bound (e.g., EXPECT_GT(tools.size(), 0) or EXPECT_GE(tools.size(), N_MIN)) and keep the existing per-tool recognition loop as the primary contract (leave the loop that validates each tool intact); update the single EXPECT_EQ line to the new non-exact assertion (referencing tools.size() / EXPECT_EQ) so the test no longer depends on an exact count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CLIPipeline.cpp`:
- Around line 148-169: The extension mapping misses the common ".glb" and
".gltf" aliases so path.endsWith in the loop (using extensionFormats[]) never
matches them; add entries for ".glb" -> "glTF 2.0 Binary (*.glb2)" and ".gltf"
-> "glTF 2.0 (*.gltf2)" (or duplicate the same format strings used for ".glb2"
and ".gltf2") to the extensionFormats array so the for-loop in the function that
checks path.endsWith(...) will return the correct glTF exporter string, and
update the parameterized tests to include file names ending with ".glb" and
".gltf" so those cases are covered.
In `@src/GizmoAxisHelpers_test.cpp`:
- Around line 49-56: The test uses fabricated pointers (reinterpret_cast to
0x100/0x200/0x300) which leads to undefined behavior when axisFromObject
performs static_cast<const Ogre::MovableObject*>; fix by replacing those fake
pointers with real objects or stable mocks: create actual Ogre::ManualObject
instances (or test doubles that are real objects deriving from
Ogre::MovableObject) and pass their pointers to
GizmoAxisHelpers::axisFromObject, or alternatively refactor
GizmoAxisHelpers::axisFromObject to take const Ogre::MovableObject* arguments so
the test can supply valid base-class pointers without invoking undefined
behavior. Ensure the pointers you pass are real objects so static_cast in
axisFromObject is valid.
In `@src/GizmoAxisHelpers.h`:
- Around line 37-51: Add an early null check in axisFromObject so a null obj
can't match null axis pointers; specifically, at the top of the function (before
the comparisons against xAxis, yAxis, zAxis) return Axis::None if obj == nullptr
so that nullptr comparisons cannot incorrectly yield Axis::X/Y/Z; keep the
existing pointer-equality checks (obj == static_cast<const
Ogre::MovableObject*>(xAxis), etc.) unchanged aside from this early guard.
In `@src/MCPServer.cpp`:
- Around line 460-468: The breadcrumb in MCPServer::callTool is using the
non-standard category "mcp.tool"; change the SentryReporter::addBreadcrumb call
inside MCPServer::callTool to use the repository-standard category
"ai.tool_call" (keep the breadcrumb message text like "Tool call: <name>" and
leave the transaction creation via SentryReporter::startTransaction unchanged)
so all MCP invocations are tracked under the central tool-call category.
In `@src/PrimitivesWidget.cpp`:
- Around line 693-696: On the no-selection branch that currently calls
setUiEmpty() then returns when SelectionSet::getSingleton()->hasNodes() is
false, clear the selection cache by resetting mSelectedPrimitive (e.g. set to
nullptr or equivalent) and clear or refresh the internal cached list used by
getSelectedPrimitiveList() before returning so the widget no longer holds
references to the prior primitives and any late edit signals won't target stale
selections.
- Around line 704-745: Reset the panel before enabling type-specific controls,
then apply the per-type UI, and only after that populate values: call the reset
method (use setUiMesh() as the reset/common UI) before invoking the chosen
handler (the function pointer assigned from
setUiCube/setUiSphere/.../setUiSpring), then call updateUiFromParams() last;
update the block that assigns setUiForPrimitive so the call order becomes
setUiMesh(); (this->*setUiForPrimitive)(); updateUiFromParams(); while keeping
the existing default branch behavior.
In `@src/ScanEngine_test.cpp`:
- Around line 304-312: The test TEST(ScanEngineTest,
EnumerateFiles_NonexistentRootReturnsEmpty) uses a hardcoded Unix path; change
it to construct a platform-neutral nonexistent path using QTemporaryDir: create
a QTemporaryDir (or QDir::tempPath()), build a child directory name that does
not exist (e.g., append a unique non-existent subfolder) and pass that full path
to ScanEngine::enumerateFiles with the existing ScanConfig; ensure the path
string is converted to QString and the test still asserts files.isEmpty().
- Around line 29-35: testDataDir() assumes a specific build layout which is
brittle in CI; change it to robustly locate fixtures by: 1) first honor an
environment or CMake-provided override (e.g. qgetenv("TEST_DATA_DIR") or
getenv("TEST_DATA_DIR")), 2) if unset, search upward from
QCoreApplication::applicationDirPath() and QDir::currentPath() for a
"media/models" directory (stop at filesystem root), and return an empty QString
if not found; then in the tests that call testDataDir() (see uses around the
block formerly lines 1298-1303) replace ASSERT_TRUE(QFile::exists(...)) with a
runtime check that if testDataDir() returned empty or the fixture file is
missing, call GTEST_SKIP() (or QSKIP if using QTest) to skip the test instead of
failing hard. Ensure references to testDataDir(), QFile::exists, and the test
case that asserted exist are updated accordingly.
In `@src/TranslationGizmo.cpp`:
- Around line 353-357: The bounding box for the Z axis is always built from 0 to
+mScale but in left-handed mode (mLeftHandCs == true) createSolidZaxis() renders
Z from 0 to -mScale, so the pick volume should mirror that; update the lambda
passed to GizmoAxisHelpers::forEachAxisIndexed so when the Axis is
GizmoAxisHelpers::Axis::Z (or the Z enum value used) and mLeftHandCs is true,
call GizmoAxisHelpers::makeAxisBoundingBox with the negative extent (use
-mScale) for the Z side instead of +mScale before passing it to
axisObject->setBoundingBox, otherwise leave other axes unchanged.
---
Outside diff comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 1093-1125: The test
CmdAnimList_NoAnimationsGeneratedMeshReturnsError asserts the wrong exit code
for CLIPipeline::cmdAnim in list mode when a skeleton exists but has zero
animations; change the two EXPECT_EQ assertions that currently expect 1 (for
textArgs and jsonArgs) to expect 0 instead so the test matches the intended
contract (CLIPipeline::cmdAnim(..., "--list") returns success and emits "No
animations found." or "[]").
---
Nitpick comments:
In `@src/MCPServer_test.cpp`:
- Line 2902: The test currently pins the exact tool count with
EXPECT_EQ(tools.size(), 51) which causes churn; change that assertion to assert
non-empty or a sensible lower bound (e.g., EXPECT_GT(tools.size(), 0) or
EXPECT_GE(tools.size(), N_MIN)) and keep the existing per-tool recognition loop
as the primary contract (leave the loop that validates each tool intact); update
the single EXPECT_EQ line to the new non-exact assertion (referencing
tools.size() / EXPECT_EQ) so the test no longer depends on an exact count.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7dcc64c-ce48-47d7-aa18-ce7b2b90b2d4
📒 Files selected for processing (19)
.github/workflows/deploy.ymlsrc/Assimp/BoneProcessor_test.cppsrc/Assimp/MaterialProcessor_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline_test.cppsrc/GizmoAxisHelpers.hsrc/GizmoAxisHelpers_test.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/MCPServer_test.cppsrc/MeshImporterExporter_test.cppsrc/PrimitivesWidget.cppsrc/PrimitivesWidget.hsrc/PropertiesPanelController_test.cppsrc/ScaleGizmo.cppsrc/ScanEngine_test.cppsrc/TransformOperator_test.cppsrc/TranslationGizmo.cppsrc/commands/TransformCommands_test.cpp
💤 Files with no reviewable changes (1)
- src/PrimitivesWidget.h
…-80-again # Conflicts: # src/CLIPipeline_test.cpp # src/ScanEngine_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/CLIPipeline_test.cpp (1)
1147-1178: Rename this test to reflect success semantics.
CmdAnimList_NoAnimationsGeneratedMeshReturnsErrornow asserts success (0), so the test name is misleading. Consider renaming it to something like...ReturnsSuccessto keep intent clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 1147 - 1178, The test name CmdAnimList_NoAnimationsGeneratedMeshReturnsError is misleading because the assertions expect success (return 0); rename the test function in the CLIPipelineCmdTest fixture to reflect success semantics (e.g., CmdAnimList_NoAnimationsGeneratedMeshReturnsSuccess) and update any references/uses of that test name; ensure the test body and expectations (calls to CLIPipeline::cmdAnim and expected return values) remain unchanged when renaming so CLIPipeline::cmdAnim and the TestArgv usage still compile and run.src/commands/TransformCommands_test.cpp (1)
773-1013: Consider extracting shared reparent test setup into a fixture helper.The setup pattern (parent/child creation + local transform capture + initial reparent) is repeated across multiple tests; a helper would reduce maintenance overhead and drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/TransformCommands_test.cpp` around lines 773 - 1013, The tests repeat identical reparent setup steps; add a private helper on the TransformCommandsTests fixture (e.g., CreateReparentScenario or SetupReparentTest) that takes the oldParentName, newParentName, childName (and optionally keepers) and returns a small struct/tuple containing Manager*, sceneMgr, oldParent, newParent, child, and the captured oldLocalPos/oldLocalOrient/oldLocalScale and newLocalPos/newLocalOrient/newLocalScale after calling Manager::reparentNode; replace the duplicated blocks in each test (those that call sceneMgr->getRootSceneNode()->createChildSceneNode, setPosition/setOrientation/setScale, capture old locals, call mgr->reparentNode, capture new locals) with calls to this helper and update each test to use the returned values when constructing ReparentCommand and asserting undo/redo behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 1147-1178: The test name
CmdAnimList_NoAnimationsGeneratedMeshReturnsError is misleading because the
assertions expect success (return 0); rename the test function in the
CLIPipelineCmdTest fixture to reflect success semantics (e.g.,
CmdAnimList_NoAnimationsGeneratedMeshReturnsSuccess) and update any
references/uses of that test name; ensure the test body and expectations (calls
to CLIPipeline::cmdAnim and expected return values) remain unchanged when
renaming so CLIPipeline::cmdAnim and the TestArgv usage still compile and run.
In `@src/commands/TransformCommands_test.cpp`:
- Around line 773-1013: The tests repeat identical reparent setup steps; add a
private helper on the TransformCommandsTests fixture (e.g.,
CreateReparentScenario or SetupReparentTest) that takes the oldParentName,
newParentName, childName (and optionally keepers) and returns a small
struct/tuple containing Manager*, sceneMgr, oldParent, newParent, child, and the
captured oldLocalPos/oldLocalOrient/oldLocalScale and
newLocalPos/newLocalOrient/newLocalScale after calling Manager::reparentNode;
replace the duplicated blocks in each test (those that call
sceneMgr->getRootSceneNode()->createChildSceneNode,
setPosition/setOrientation/setScale, capture old locals, call mgr->reparentNode,
capture new locals) with calls to this helper and update each test to use the
returned values when constructing ReparentCommand and asserting undo/redo
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2023a2cf-cb86-4b7e-9a1e-0267a8e23a9d
📒 Files selected for processing (11)
.github/workflows/deploy.ymlsrc/CLIPipeline.cppsrc/CLIPipeline_test.cppsrc/GizmoAxisHelpers.hsrc/GizmoAxisHelpers_test.cppsrc/MCPServer.cppsrc/MCPServer_test.cppsrc/PrimitivesWidget.cppsrc/ScanEngine_test.cppsrc/TranslationGizmo.cppsrc/commands/TransformCommands_test.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/GizmoAxisHelpers_test.cpp
- src/TranslationGizmo.cpp
- src/CLIPipeline.cpp
- src/MCPServer.cpp
- src/ScanEngine_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/CLIPipeline_test.cpp (1)
1147-1184: Rename this test to match the broadened expected outcomes.The body now intentionally accepts both success and error paths, but
CmdAnimList_NoAnimationsGeneratedMeshReturnsErrorimplies error-only behavior.✏️ Suggested rename
-TEST_F(CLIPipelineCmdTest, CmdAnimList_NoAnimationsGeneratedMeshReturnsError) +TEST_F(CLIPipelineCmdTest, CmdAnimList_NoAnimationsGeneratedMeshHandlesEnvDependentResult)Based on learnings: In
src/CLIPipeline.cpp,CLIPipeline::cmdAnimlist mode returns0when a skeleton exists with zero animations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 1147 - 1184, Rename the test TEST_F(CLIPipelineCmdTest, CmdAnimList_NoAnimationsGeneratedMeshReturnsError) to reflect that it accepts either success or loader error (because CLIPipeline::cmdAnim list mode can return 0 when skeleton has zero animations or 1 when loader fails); update the test identifier to something like CmdAnimList_NoAnimationsGeneratedMeshAcceptsSuccessOrLoaderError (or similar) wherever that test name is declared/used so the test name matches the broadened expected outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 1147-1184: Rename the test TEST_F(CLIPipelineCmdTest,
CmdAnimList_NoAnimationsGeneratedMeshReturnsError) to reflect that it accepts
either success or loader error (because CLIPipeline::cmdAnim list mode can
return 0 when skeleton has zero animations or 1 when loader fails); update the
test identifier to something like
CmdAnimList_NoAnimationsGeneratedMeshAcceptsSuccessOrLoaderError (or similar)
wherever that test name is declared/used so the test name matches the broadened
expected outcomes.
|
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
GizmoAxisHelpersMCPServer::callTooldispatch duplication via a centralized handler map and extracted heavy-tool classificationPrimitivesWidgetKey files
src/GizmoAxisHelpers.hsrc/TranslationGizmo.cppsrc/ScaleGizmo.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/PrimitivesWidget.cppsrc/commands/TransformCommands_test.cppsrc/CLIPipeline_test.cppsrc/MCPServer_test.cppValidation
cmake --build . --target UnitTests -j4./bin/UnitTests --gtest_filter='TransformCommandsTests.*'./bin/UnitTests --gtest_filter='TransformCommandsTests.*:GizmoAxisHelpersTest.*:ScaleGizmoTests.*:TranslationGizmoTests.*:PrimitivesWidgetTest.*:PropertiesPanelControllerTests.*:CLIPipelineFormatTest.*:CLIPipelineSmoke.*:CLIPipelineRun.*:CLIPipelineTelemetryTest.*'(Several environment-gated tests skip when Ogre/GL resources are unavailable.)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests