refactor(api)!: borrow topology views from canonical storage (#472) - #474
Merged
Conversation
- Return validated borrowed simplex vertex slices instead of owned or optional detached snapshots. - Split convex hull facet access into detached `facet_handles()` and borrowed `facets(triangulation)` views with freshness checks. - Make vertex and simplex payload setters checked mutations that report typed stale-key errors. - Preserve fallback rebuild payload restoration through typed simplex-data restore errors. BREAKING CHANGE: `Tds::simplex_vertices`, `Triangulation::simplex_vertices`, and `DelaunayTriangulation::simplex_vertices` now return `Result<&[VertexKey], TdsError>`-style borrowed views instead of the previous owned/optional forms. `ConvexHull::facets()` has been split into `facet_handles()` for detached handles and `facets(triangulation)` for borrowed `FacetView` access. `set_vertex_data` and `set_simplex_data` now return checked `Result<Option<_>, TdsMutationError>` values instead of conflating missing keys with empty payloads.
acgetchell
enabled auto-merge (squash)
June 21, 2026 09:52
Contributor
WalkthroughConverts ChangesTyped errors, borrowed slice views, and ConvexHull facet split
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DelaunayTriangulation
participant Triangulation
participant Tds
rect rgba(70, 130, 180, 0.5)
Note over Caller,Tds: set_vertex_data / set_simplex_data (Result path)
Caller->>DelaunayTriangulation: set_vertex_data(key, data)?
DelaunayTriangulation->>Triangulation: set_vertex_data(key, data)
Triangulation->>Tds: set_vertex_data(key, data)
alt key found
Tds-->>Triangulation: Ok(Option~U~)
Triangulation-->>DelaunayTriangulation: Ok(Option~U~)
DelaunayTriangulation-->>Caller: Ok(previous Option~U~)
else key missing
Tds-->>Triangulation: Err(TdsMutationError::VertexNotFound)
Triangulation-->>DelaunayTriangulation: Err(TdsMutationError)
DelaunayTriangulation-->>Caller: Err(DelaunayError::TdsMutation)
end
end
rect rgba(60, 179, 113, 0.5)
Note over Caller,Tds: simplex_vertices (borrowed slice path)
Caller->>DelaunayTriangulation: simplex_vertices(simplex_key)?
DelaunayTriangulation->>Triangulation: simplex_vertices(simplex_key)
Triangulation->>Tds: simplex_vertices(simplex_key)
alt simplex + vertices exist
Tds-->>Triangulation: Ok(&[VertexKey]) borrowed from Simplex
Triangulation-->>DelaunayTriangulation: Ok(&[VertexKey])
DelaunayTriangulation-->>Caller: Ok(&[VertexKey])
else missing
Tds-->>Triangulation: Err(TdsError::SimplexNotFound)
Triangulation-->>DelaunayTriangulation: Err(TdsError)
DelaunayTriangulation-->>Caller: Err(TdsError)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage 96.05% diff coverage · 0.00% coverage variation
Metric Results Coverage variation ✅ 0.00% coverage variation (-1.00%) Diff coverage ✅ 96.05% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (9a973df) 69935 64028 91.55% Head commit (7682fad) 70021 (+86) 64105 (+77) 91.55% (0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#474) 253 243 96.05% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/workflows.md (1)
319-346:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType mismatch in simplex data example:
42cannot be assigned to()type.The triangulation is constructed with
.build::<()>(), specifying the simplex data generic parameterV = ()(unit type). However, line 337 attempts to set simplex data withSome(42)where42is ani32, not().This example would fail to compile. Align the simplex data type with the intended payload:
🔧 Proposed fix to match vertex data pattern
- let mut dt = DelaunayTriangulationBuilder::new(&vertices).build::<()>()?; + let mut dt = DelaunayTriangulationBuilder::new(&vertices).build::<i32>()?;This keeps the simplex data
Some(42)and parallels the vertex data example where data is typei32. Alternatively, changeSome(42)toSome(())and retain.build::<()>(), but the integer-data pattern is more instructive.🤖 Prompt for 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. In `@docs/workflows.md` around lines 319 - 346, The DelaunayTriangulationBuilder is constructed with a generic type parameter of `()` (unit type) in the build call, but the example later attempts to set simplex data with `Some(42)` which is an i32 integer. This type mismatch will cause a compilation error. Change the generic type parameter in the DelaunayTriangulationBuilder::new().build() call from `()` to `i32` to match the simplex data payload type being set with `Some(42)`, which will align the simplex data example with the vertex data example pattern and make the code compile correctly.src/geometry/algorithms/convex_hull.rs (1)
943-951:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck TDS identity before comparing generations.
These guards compare
creation_generationbefore provingtrihas the same TDS identity. For a hull used with a different TDS at a different generation, callers getStaleHullinstead of the more preciseIdentityMismatch, even though generation counters are only meaningful within the same identity.🐛 Proposed fix
- let creation_generation = self.creation_generation.get().copied().unwrap_or(0); - if creation_generation != tri.tds.generation() { - return Err(self.stale_hull_construction_error(tri)); - } let Some(creation_identity) = self.creation_identity.get() else { return Err(self.identity_mismatch_construction_error(tri)); }; if !Arc::ptr_eq(creation_identity, tri.tds.identity()) { return Err(self.identity_mismatch_construction_error(tri)); } + let creation_generation = self.creation_generation.get().copied().unwrap_or(0); + if creation_generation != tri.tds.generation() { + return Err(self.stale_hull_construction_error(tri)); + } Ok(()) } @@ - let creation_generation = self.creation_generation.get().copied().unwrap_or(0); - if creation_generation != tri.tds.generation() { - return Err(self.stale_hull_error(tri)); - } let Some(creation_identity) = self.creation_identity.get() else { return Err(self.identity_mismatch_error(tri)); }; if !Arc::ptr_eq(creation_identity, tri.tds.identity()) { return Err(self.identity_mismatch_error(tri)); } + let creation_generation = self.creation_generation.get().copied().unwrap_or(0); + if creation_generation != tri.tds.generation() { + return Err(self.stale_hull_error(tri)); + } Ok(()) }Also applies to: 968-976
🤖 Prompt for 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. In `@src/geometry/algorithms/convex_hull.rs` around lines 943 - 951, The guard checks are in the wrong order and should verify TDS identity before comparing generations, since generation counters are only meaningful within the same identity. Reorder the checks so that the identity validation (checking if creation_identity exists and comparing it with tri.tds.identity() using Arc::ptr_eq) happens before the generation comparison. This ensures that when using a hull with a different TDS, callers receive the more precise IdentityMismatch error instead of StaleHull. Apply the same reordering to the other similar guard block that also applies these checks (around line 968-976).
🤖 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.
Outside diff comments:
In `@docs/workflows.md`:
- Around line 319-346: The DelaunayTriangulationBuilder is constructed with a
generic type parameter of `()` (unit type) in the build call, but the example
later attempts to set simplex data with `Some(42)` which is an i32 integer. This
type mismatch will cause a compilation error. Change the generic type parameter
in the DelaunayTriangulationBuilder::new().build() call from `()` to `i32` to
match the simplex data payload type being set with `Some(42)`, which will align
the simplex data example with the vertex data example pattern and make the code
compile correctly.
In `@src/geometry/algorithms/convex_hull.rs`:
- Around line 943-951: The guard checks are in the wrong order and should verify
TDS identity before comparing generations, since generation counters are only
meaningful within the same identity. Reorder the checks so that the identity
validation (checking if creation_identity exists and comparing it with
tri.tds.identity() using Arc::ptr_eq) happens before the generation comparison.
This ensures that when using a hull with a different TDS, callers receive the
more precise IdentityMismatch error instead of StaleHull. Apply the same
reordering to the other similar guard block that also applies these checks
(around line 968-976).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bdef4a08-f8e7-4d67-9d06-5b5b5b85e675
📒 Files selected for processing (22)
benches/allocation_hot_paths.rsdocs/api_design.mddocs/dev/rust.mddocs/workflows.mdsrc/core/algorithms/pl_manifold_repair.rssrc/core/query.rssrc/core/simplex.rssrc/core/tds/mutation.rssrc/core/tds/storage.rssrc/core/tds/validation.rssrc/core/triangulation.rssrc/core/util/jaccard.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/query.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/quality.rssrc/lib.rssrc/topology/manifold.rstests/prelude_exports.rstests/public_topology_api.rstests/trait_bound_ergonomics.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
==========================================
- Coverage 91.52% 91.52% -0.01%
==========================================
Files 77 77
Lines 69714 69799 +85
==========================================
+ Hits 63809 63885 +76
- Misses 5905 5914 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
This was referenced Jun 23, 2026
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.
facet_handles()and borrowedfacets(triangulation)views with freshness checks.BREAKING CHANGE:
Tds::simplex_vertices,Triangulation::simplex_vertices, andDelaunayTriangulation::simplex_verticesnow returnResult<&[VertexKey], TdsError>-style borrowed views instead of the previous owned/optional forms.ConvexHull::facets()has been split intofacet_handles()for detached handles andfacets(triangulation)for borrowedFacetViewaccess.set_vertex_dataandset_simplex_datanow return checkedResult<Option<_>, TdsMutationError>values instead of conflating missing keys with empty payloads.Closes #472