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".

src/EditModeController_test.cpp (1)

1703-1879: Edge-mode fill still needs a controller test.

The new suite exercises vertex-mode fill, but it never hits the controller-only path that walks a selected boundary-edge loop into an ordered vertex ring. A regression there would still pass these tests and the lower-level HalfEdgeMesh coverage.

🤖 Prompt for AI Agents

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

In `@src/EditModeController_test.cpp` around lines 1703 - 1879, The test suite
lacks a controller-level test for edge-mode fill (the code path that walks a
selected boundary-edge loop into an ordered vertex ring), so add a new TEST_F
(e.g., FillSelectionEdgeModeHandlesBoundaryLoop) that: obtains the singleton
EditModeController::instance(), calls enterEditMode(), sets selection mode to
EditModeController::EdgeMode, selects a real boundary edge via selectEdge(va,vb)
using an edge pulled from extractEntityBuffers(m_entity, pos, tris) (like other
tests do to pick a safe edge), asserts that ctrl->fillSelection() returns > 0
(or the expected triangle count change), and finally verifies the entity buffers
via extractEntityBuffers to ensure triangles increased or the fill effect
occurred; follow the patterns in FillSelectionVertexModeProducesNewTriangle and
SubdivideSelectionEdgeModePromotesIncidentFaces for setup and validation.

src/HalfEdgeMesh_test.cpp (1)

4406-4425: Rename this test or expand it to actually cover all split-mask branches.

The current fixture only drives one neighbor split-mask case on the quad, so the title/comment overstates coverage for splitMask handling. That makes the suite look stronger than it is.

🤖 Prompt for AI Agents

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

In `@src/HalfEdgeMesh_test.cpp` around lines 4406 - 4425, The test named
SubdivideFacesHandlesAllSplitMaskPaths (TEST(HalfEdgeMeshStandalone,
SubdivideFacesHandlesAllSplitMaskPaths)) claims to exercise all splitMask
branches but only drives one neighbor case; either rename the test to accurately
reflect that it only tests a single splitMask path or expand it to cover all
cases by creating fixtures that trigger splitMask values 1,2,4 and combinations
(e.g., build multiple small meshes or adjust selections on
makeQuadMesh/constructed HalfEdgeMesh so subdivideFaces(...) exercises neighbor
edges that produce each splitMask branch), then assert expected face
counts/structure and keep calls to he.subdivideFaces, he.validate, and
activeFaceCount(he) for each scenario.

src/EditModeController.cpp (1)

2993-3004: Tolerance inconsistency: 1e-10f vs 1e-8f used elsewhere.

The position-matching tolerance here (1e-10f) is 100× tighter than findGlobalVertsByPosition (1e-8f) and other similar lookups in this file. While tighter is generally safer for subdivision midpoints (which should be exact), the inconsistency could cause subtle mismatches if floating-point rounding occurs during the HE → EditableMesh round-trip.

Consider using a named constant shared across all position-based vertex lookups for consistency.

♻️ Suggested constant

+namespace {
+constexpr float kPositionMatchEps2 = 1e-8f; // squared distance tolerance for position-based vertex matching
+} // namespace

 // In subdivideSelection and findGlobalVertsByPosition:
-if (sub.vertices[li].position.squaredDistance(tgt) < 1e-10f) {
+if (sub.vertices[li].position.squaredDistance(tgt) < kPositionMatchEps2) {

🤖 Prompt for AI Agents

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

In `@src/EditModeController.cpp` around lines 2993 - 3004, The tolerance 1e-10f
used when comparing sub.vertices[li].position to midpointPositions is
inconsistent with other lookups (e.g., findGlobalVertsByPosition) which use
1e-8f; replace the hardcoded 1e-10f with a shared named constant (e.g.,
POSITION_MATCH_TOLERANCE) declared alongside other position-lookup utilities so
all position-based comparisons (including the loop that uses subs,
midpointPositions and the m_selectedVertices insertion) use the same tolerance
(set to 1e-8f) to avoid subtle mismatches.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes