Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

ui_files/mainwindow.ui (1)

83-83: Avoid extending the legacy Widgets menu for new UI surface area.

This adds a brand-new control to ui_files/mainwindow.ui, but the repo guidance says new UI should go through QML instead of extending the existing Widgets-based surface. If this toggle needs to stay here for now, I’d at least document it as an exception and move it into the QML settings/debug UI in a follow-up.

As per coding guidelines "UI: QML over Widgets. New UI should be built in QML (Qt Quick), not Qt Widgets. Existing Widget-based UI (ui_files/) remains but should not be extended."

Also applies to: 229-239

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui_files/mainwindow.ui` at line 83, The new Widgets action
actionShow_Mesh_Info added to ui_files/mainwindow.ui violates the "UI: QML over
Widgets" guideline; either remove this addaction and implement the toggle in QML
(e.g., add it to the QML settings/debug UI), or if you must keep it temporarily,
document it as an explicit exception in the commit message/PR and create a
follow-up ticket to migrate actionShow_Mesh_Info (and the related controls
around lines 229-239) into QML; update the UI change to match one of these two
paths and avoid adding new Widget-based controls in ui_files.

src/MCPServer_test.cpp (1)

2518-2532: Replace one of these error-path checks with a real success-path toggle test.

These assertions only prove that toggle_mesh_info is recognized and that the no-MainWindow path returns some error text. They will not catch regressions in the actual behavior—e.g. findChild<MeshInfoOverlay*>() failing or the show argument being ignored. A small fixture with a real MainWindow + MeshInfoOverlay would cover the feature much better.

As per coding guidelines, "Add Google Test unit tests for new functionality."

Also applies to: 2706-2723

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MCPServer_test.cpp` around lines 2518 - 2532, Replace one of the
error-path checks with a real success-path test that constructs a MainWindow
containing a MeshInfoOverlay, then calls server->callTool("toggle_mesh_info",
args) to toggle visibility and asserts the overlay was found and its visibility
changed; specifically, in the TEST_F named ToggleMeshInfoIsRecognizedTool (or a
new TEST_F), create a MainWindow instance, ensure findChild<MeshInfoOverlay*>()
returns a valid pointer, call server->callTool("toggle_mesh_info",
QJsonObject{{"show", true}}) and verify the returned result is not an error and
the MeshInfoOverlay is visible, then call with {"show", false} and verify it
becomes hidden, using server->callTool, MeshInfoOverlay, MainWindow,
getResultText and isError to validate outcomes.

tests/CMakeLists.txt (1)

58-59: Duplicate source file entry.

QMLMaterialHighlighter.cpp appears twice in TEST_SRC_FILES (line 47 and line 59). While CMake typically handles duplicates gracefully, this should be cleaned up to avoid confusion.

Remove duplicate entry

         ${CMAKE_CURRENT_SOURCE_DIR}/../src/MCPSettingsDialog.cpp
         ${CMAKE_CURRENT_SOURCE_DIR}/../src/MeshInfoOverlay.cpp
-        ${CMAKE_CURRENT_SOURCE_DIR}/../src/QMLMaterialHighlighter.cpp
     )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/CMakeLists.txt` around lines 58 - 59, The TEST_SRC_FILES list contains
a duplicate entry for QMLMaterialHighlighter.cpp; open the CMakeLists.txt where
TEST_SRC_FILES is defined and remove the redundant occurrence of
QMLMaterialHighlighter.cpp so it appears only once in TEST_SRC_FILES (ensure
other source filenames such as MeshInfoOverlay.cpp remain unchanged).

src/MeshInfoOverlay.cpp (1)

108-112: Consider using unsigned int for totalBones for consistency.

totalBones is declared as unsigned short while other counters (totalVerts, totalTris, totalSubmeshes) use unsigned int. With multiple skeletal meshes, the sum could theoretically overflow unsigned short (max 65535). While unlikely in practice, using unsigned int would be consistent with the other counters.

Suggested change

     unsigned int totalVerts = 0;
     unsigned int totalTris = 0;
     unsigned int totalSubmeshes = 0;
-    unsigned short totalBones = 0;
+    unsigned int totalBones = 0;
     int totalAnims = 0;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshInfoOverlay.cpp` around lines 108 - 112, Change the type of the
totalBones variable in MeshInfoOverlay.cpp from unsigned short to unsigned int
to match the other counters (totalVerts, totalTris, totalSubmeshes) and avoid
potential overflow when summing bones across multiple meshes; update any related
uses of totalBones in functions or calculations in the same file (e.g., places
referencing totalBones) to work with unsigned int.

src/MeshInfoOverlay_test.cpp (1)

94-96: Test assertions for vertex/triangle counts are somewhat fragile.

Checking result.contains("3") and result.contains("1") could match other numbers in the output (e.g., locale-formatted numbers or other stats). Consider using a more specific pattern or regex to match "Verts: 3" and "Tris: 1" directly.

More specific assertions

-    // 3 vertices, 1 triangle
-    EXPECT_TRUE(result.contains("3")) << "Result: " << result.toStdString();
-    EXPECT_TRUE(result.contains("1")) << "Result: " << result.toStdString();
+    // 3 vertices, 1 triangle - use more specific patterns
+    EXPECT_TRUE(result.contains("Verts: 3")) << "Result: " << result.toStdString();
+    EXPECT_TRUE(result.contains("Tris: 1")) << "Result: " << result.toStdString();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshInfoOverlay_test.cpp` around lines 94 - 96, The assertions using
result.contains("3") and result.contains("1") are too broad; update the test to
assert against more specific text patterns like "Verts: 3" and "Tris: 1" (or use
a QRegularExpression) instead of plain digit checks. Locate the EXPECT_TRUE
lines that reference result and replace them with checks such as
result.contains("Verts: 3") and result.contains("Tris: 1") or use
QRegularExpression/QString::contains with explicit patterns (e.g.,
QRegularExpression(R"(Verts:\s*3)")) to avoid accidental matches.

src/MCPServer_test.cpp (1)

2727-2761: Success path test covers visibility toggling and automatic flip behavior.

The test validates:

• Initial hidden state
• Toggle on with show=true → "shown" message, overlay visible
• Toggle off with show=false → "hidden" message, overlay hidden
• Toggle without show arg → flips to visible

The cleanup correctly resets the server's mainWindow and deletes the overlay. However, if an assertion fails mid-test, the overlay won't be deleted. This is acceptable for tests, but consider using a unique_ptr or RAII wrapper for cleaner exception safety.

🛡️ Optional: Use RAII for exception-safe cleanup

 TEST_F(MCPServerTest, ToggleMeshInfo_SuccessPath)
 {
     // Create a fake MainWindow with a MeshInfoOverlay child so findChild works
     QMainWindow fakeWindow;
-    auto* overlay = new MeshInfoOverlay(reinterpret_cast<MainWindow*>(&fakeWindow));
+    std::unique_ptr<MeshInfoOverlay> overlay(
+        new MeshInfoOverlay(reinterpret_cast<MainWindow*>(&fakeWindow)));
     server->setMainWindow(reinterpret_cast<MainWindow*>(&fakeWindow));

-    EXPECT_FALSE(overlay->isVisible());
+    EXPECT_FALSE(overlay->isVisible());

     // Toggle on
     QJsonObject argsOn;
     argsOn["show"] = true;
     QJsonObject resultOn = server->callTool("toggle_mesh_info", argsOn);
     EXPECT_FALSE(isError(resultOn)) << getResultText(resultOn).toStdString();
     EXPECT_TRUE(getResultText(resultOn).contains("shown"));
     EXPECT_TRUE(overlay->isVisible());

     // ... rest of test ...

     // Clean up
     server->setMainWindow(nullptr);
-    delete overlay;
+    // overlay automatically deleted by unique_ptr
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MCPServer_test.cpp` around lines 2727 - 2761, The test creates a raw
MeshInfoOverlay* (overlay) attached to fakeWindow and manually deletes it at the
end, which leaks if an assertion aborts mid-test; change the allocation to use
RAII (e.g., std::unique_ptr<MeshInfoOverlay> or a scoped guard) and ensure
server->setMainWindow is reset in a destructor or scope guard so that overlay is
automatically deleted even on test failures; update references to overlay to use
the smart pointer (overlay.get()) and keep server->setMainWindow(nullptr) in the
RAII cleanup.

src/MeshInfoOverlay_test.cpp (1)

54-60: Consider documenting the reinterpret_cast usage more explicitly.

The fakeMainWindow helper uses reinterpret_cast to avoid constructing the heavyweight real MainWindow. The comment explains the rationale, but this is a somewhat fragile pattern.

This approach works because MeshInfoOverlay only uses QObject/QWidget methods on the MainWindow*, but if MainWindow ever adds virtual methods or non-standard memory layout, this could cause subtle issues.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshInfoOverlay_test.cpp` around lines 54 - 60, Document the
reinterpret_cast in fakeMainWindow more explicitly: expand the comment in the
fakeMainWindow function to state that we deliberately cast a QMainWindow* to
MainWindow* because MeshInfoOverlay only calls QObject/QWidget methods
(installEventFilter, findChildren, QLabel parent) and therefore no
MainWindow-specific state or virtual dispatch is required, and add a clear
TODO/FIXME warning that this is fragile — if MainWindow acquires virtual
methods, non-trivial data, or changes inheritance, this cast becomes unsafe and
callers should construct a real MainWindow instead; also mention MeshInfoOverlay
by name so future editors see the dependency.