Restore SnappingOptions runtime setters (4.5.x regression) by magmoe · Pull Request #530 · dymaptic/GeoBlazor

magmoe · 2026-06-17T16:41:01Z

What

Restores the runtime setter methods on SnappingOptions that were accidentally dropped in the 4.5.x line, plus regression guards.

Why

Commit c3ab36411 truncated SnappingOptions.gb.cs right after the property getters, deleting the entire Property Setters and collection-helper regions. That removed SetEnabled, SetGridEnabled, SetDistance, SetFeatureEnabled, SetSelfEnabled, SetAttributeRulesEnabled, and SetFeatureSources, so consumers can no longer toggle snapping / grid snapping at runtime (reported against 4.5.1). It's isolated to this one component and is still broken on develop and in 4.5.1–4.5.3.

Changes

Restore the complete SnappingOptions.gb.cs from f02afefeb (the commit immediately before the truncation) — re-adds exactly the 7 setters + the Add/Remove FeatureSources helpers and nothing else.
SnappingOptionsApiTests — public-API contract test so a future code-generation split can't silently drop these members again.
GeometryExtentSerializationTests + additions to GeometryEngineTests.TestUnion — guards for the separately-reported GeometryEngine.Union extent issue. That one is not reproducible from code (the union/extent path is identical to the working 4.4.4), so these lock in the behavior rather than fix a found bug.

Notes

Targets develop because the regression is present there too; also serves as the cut point for a 4.5.4 hotfix.
11/11 unit tests pass; the browser test project compiles.

🤖 Generated with Claude Code

The 4.5.x release accidentally truncated SnappingOptions.gb.cs right after the property getters (commit c3ab364), dropping the entire Property Setters and collection-helper regions. This removed SetEnabled, SetGridEnabled, SetDistance, SetFeatureEnabled, SetSelfEnabled, SetAttributeRulesEnabled, and SetFeatureSources, breaking runtime snapping/grid toggling for consumers (reported against 4.5.1). Restores the complete generated file from f02afef (the commit immediately before the truncation), which re-adds exactly the 7 setters plus the Add/Remove FeatureSources collection helpers and nothing else. Adds SnappingOptionsApiTests as a public-API contract guard so a future code-generation split cannot silently drop these members again. Versioning/release left to the maintainer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Customer reported (against 4.5.1) that GeometryEngine.Union returned a geometry with a null Extent, breaking map.GoTo(union.Extent). The union/extent code path is unchanged from the working 4.4.4 line, so this is not reproducible from source — these tests lock in the behavior so any future change that drops the extent is caught: - GeometryExtentSerializationTests: unit-level guard deserializing the exact JSON the JS union() produces and asserting Extent (and a full round-trip) survive. - GeometryEngineTests.TestUnion: extends the browser test to assert the unioned geometry has the expected Extent bounds and that GetExtent() returns it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

claude · 2026-06-17T16:41:32Z

Claude encountered an error —— View job

I'll analyze this and get back to you.

Copilot

Pull request overview

Restores the missing runtime setter/collection-helper APIs on SnappingOptions (regressed in 4.5.x due to generated file truncation) and adds regression tests guarding both the snapping API surface and a reported GeometryEngine.Union extent/serialization behavior.

Changes:

Re-add SnappingOptions runtime setters and FeatureSources add/remove helpers in SnappingOptions.gb.cs.
Add unit-level public API contract tests to prevent future silent removal of SnappingOptions members.
Add regression tests asserting GeometryEngine.Union results preserve/populate Extent through serialization and in browser-based union execution.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

File	Description
`src/dymaptic.GeoBlazor.Core/Components/SnappingOptions.gb.cs`	Restores the missing runtime setters and `FeatureSources` collection helper methods.
`test/dymaptic.GeoBlazor.Core.Test.Unit/SnappingOptionsApiTests.cs`	Adds API-surface regression tests for `SnappingOptions` runtime configuration APIs.
`test/dymaptic.GeoBlazor.Core.Test.Unit/GeometryExtentSerializationTests.cs`	Adds serialization regression coverage ensuring `Extent` survives deserialization/round-trip.
`test/dymaptic.GeoBlazor.Core.Test.Blazor.Shared/Components/GeometryEngineTests.cs`	Adds browser test assertions that union results include a populated `Extent` and `GetExtent()` works.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        if (value is not null)
+        {
+            foreach (FeatureSnappingLayerSource item in value)
+            {
+                item.UpdateGeoBlazorReferences(CoreJsModule!, ProJsModule, View, this, Layer);
+            }
+        }


+    [TestMethod]
+    [DataRow("SetEnabled")]
+    [DataRow("SetGridEnabled")]
+    [DataRow("SetFeatureEnabled")]
+    [DataRow("SetSelfEnabled")]
+    public void HasRuntimeNullableBoolSetter(string methodName)
+    {
+        MethodInfo? method = SnappingOptionsType.GetMethod(methodName,
+            BindingFlags.Public | BindingFlags.Instance, [typeof(bool?)]);
+
+        Assert.IsNotNull(method,
+            $"SnappingOptions.{methodName}(bool?) must exist for runtime snapping toggling (regression from 4.5.x).");
+        Assert.AreEqual(typeof(Task), method.ReturnType,
+            $"SnappingOptions.{methodName}(bool?) should return Task.");
+    }


+    [TestMethod]
+    [DataRow("Enabled")]
+    [DataRow("GridEnabled")]
+    [DataRow("FeatureEnabled")]
+    [DataRow("SelfEnabled")]
+    public void HasNullableBoolParameter(string propertyName)
+    {
+        PropertyInfo? property = SnappingOptionsType.GetProperty(propertyName,
+            BindingFlags.Public | BindingFlags.Instance);
+
+        Assert.IsNotNull(property,
+            $"SnappingOptions.{propertyName} property must exist (regression from 4.5.x).");
+        Assert.AreEqual(typeof(bool?), property.PropertyType,
+            $"SnappingOptions.{propertyName} should be a bool? property.");
+        Assert.IsTrue(property.CanRead && property.CanWrite,
+            $"SnappingOptions.{propertyName} should be readable and writable.");
+    }


+    [TestMethod]
+    public void SetEnabled_UpdatesLocalProperty_BeforeRender()
+    {
+        // Without a JS runtime the setter should still update the local value (used for binding /
+        // initial render). The customer reported that even binding Enabled "didn't take effect".
+        SnappingOptions options = new();
+
+        // Should not throw when there is no JS component yet; should set the local property.
+        options.SetEnabled(true).GetAwaiter().GetResult();
+        Assert.IsTrue(options.Enabled.GetValueOrDefault(), "SetEnabled should update the local Enabled value.");
+
+        options.SetGridEnabled(true).GetAwaiter().GetResult();
+        Assert.IsTrue(options.GridEnabled.GetValueOrDefault(), "SetGridEnabled should update the local GridEnabled value.");
+    }


TimPurdum · 2026-06-17T16:50:58Z

@magmoe did you or Claude do a search for any similarly truncated files? It's possible I manually truncated this file to fix a merge conflict, but I want to make sure it's not a larger issue.

Addresses Copilot review: the contract test only guarded the 4 bool? setters and 4 bool? properties. Since the goal is to catch any future truncation of the generated runtime API, cover everything the 4.5.x cut removed: - all 7 setters (SetEnabled/GridEnabled/FeatureEnabled/SelfEnabled/AttributeRulesEnabled, plus SetDistance(double?) and SetFeatureSources(IReadOnlyList<...>)) - all 7 parameters (the 5 bool?, Distance, FeatureSources) - the AddToFeatureSources/RemoveFromFeatureSources collection helpers Also makes the behavioral test async/await instead of blocking on GetResult(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

magmoe · 2026-06-17T17:19:51Z

@magmoe did you or Claude do a search for any similarly truncated files? It's possible I manually truncated this file to fix a merge conflict, but I want to make sure it's not a larger issue.

Yes, it's isolated to the one file. Claude checked to make sure

buildDotNetGeometry now computes a missing extent from the geometry's coordinates, so results from the operator API (e.g. unionOperator) carry an Extent instead of null. Geometry.GetExtent() also falls back to a client-side CalculateExtent() (overridden by Polygon/Polyline) when neither the cached value nor the JS component supplies one. Adds extent assertions to TestUnionWithParamsArray and a GeometryExtentTests unit test for the calculation fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

TimPurdum · 2026-06-17T20:21:11Z

Added the source fix behind the Union/Extent regression guards on this branch (commit c5f7b16) — these were previously assertion-only, so TestUnion's extent check would have failed without a fix.

Root cause: results from the geometry operator API (e.g. unionOperator.executeMany) don't always expose a cached .extent the way the legacy geometryEngine.union did. buildDotNetPolygon/etc. only copy extent when present, so the .NET Geometry.Extent came back null (breaking map.GoTo(extent)), and GetExtent() couldn't recover it because the result isn't a registered JS component. The C#/proto serialization itself was fine — the extent simply never made it into the payload.

Fix (centralized, so every operator benefits — not just Union):

Scripts/geometry.ts — buildDotNetGeometry now populates a missing extent, computing a bounding box from the geometry's rings/paths/points when the geometry doesn't provide one.
Geometry.GetExtent() — falls back to a client-side CalculateExtent() (overridden by Polygon/Polyline via a shared CalculateExtentFromPaths) when neither the cached value nor the JS component supplies one. Also no longer overwrites a good cached extent with a null from a stray JS call.

Tests:

Added extent assertions to TestUnionWithParamsArray (mirrors your TestUnion).
New GeometryExtentTests unit test covering the C# calculation fallback (GetExtent() with no JS module) — complements your GeometryExtentSerializationTests.

Verification: 21/21 unit tests pass on the branch; C#/TS compile clean (ESLint passed). I could not run the browser automation suite (TestUnion/TestUnionWithParamsArray) in my environment — the WASM runner wouldn't start — so those two e2e tests should be run in CI before merge.

Your SnappingOptions setters, contract tests, and version bump are untouched.

🤖 Generated with Claude Code

TimPurdum

Copilot: I found one edge case that should be addressed before merging.

In src/dymaptic.GeoBlazor.Core/Components/Geometries/Geometry.cs around CalculateExtentFromPaths, and the matching fallback in src/dymaptic.GeoBlazor.Core/Scripts/geometry.ts, the newly calculated extent only carries xmin/ymin/xmax/ymax. For paths/rings/points with Z or M coordinates, the fallback drops zmin/zmax and mmin/mmax, even though GeoBlazor's Extent model and ArcGIS Extent both support those values. That makes the fallback less equivalent to geometry.extent for Z/M-enabled geometry results, and can return an incomplete extent for 3D/M geometries when the ArcGIS cached extent is missing.

Suggested fix: while scanning vertices, also track coordinate index 2 when hasZ is true and the M index (3 when hasZ, otherwise 2) when hasM is true, then populate Zmin/Zmax/Mmin/Mmax in C# and zmin/zmax/mmin/mmax in TypeScript. Adding a regression test with 3D or M-valued polygon/polyline coordinates would lock this down.

Addresses PR review: the extent computed by CalculateExtentFromPaths (C#) and calculateGeometryExtent (geometry.ts) only carried x/y. For Z/M-enabled geometries it now also tracks zmin/zmax and mmin/mmax (z at coord index 2 when hasZ; m at index 3 when hasZ, else 2), matching geometry.extent for 3D/M results. Adds Z (polygon) and M (polyline) regression tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

TimPurdum · 2026-06-19T12:01:32Z

Addressed the Z/M edge case in fe23867e5.

CalculateExtentFromPaths (Geometry.cs) and calculateGeometryExtent (geometry.ts) now also track zmin/zmax and mmin/mmax when the geometry has Z/M — z at coordinate index 2 when hasZ, m at index 3 when hasZ else index 2 — and populate the resulting extent's Z/M bounds (and hasZ/hasM flags). When the geometry exposes its own .extent, that's still used directly and is unaffected.

Added regression coverage in GeometryExtentTests: a Z-valued polygon (asserts Zmin/Zmax) and an M-valued polyline (asserts Mmin/Mmax). All 4 GeometryExtentTests pass and the TS recompiles clean (ESLint OK).

The remaining Copilot inline note on SetFeatureSources (calling UpdateGeoBlazorReferences(CoreJsModule!...) before the null-check) is the standard generated pattern shared by every collection setter, not specific to this PR — worth fixing in the code generator rather than hand-editing the .gb.cs here.

🤖 Generated with Claude Code

Per review: rather than calculating the bounding box in C# (CalculateExtent/CalculateExtentFromPaths) or by hand in TypeScript, buildDotNetGeometry now obtains the extent from ArcGIS. When an operator result (e.g. unionOperator) doesn't expose a cached .extent, it rebuilds a typed ArcGIS geometry via buildJsGeometry so the SDK recomputes it (including Z/M bounds). Reverts the C# extent calculation and its Polygon/Polyline overrides, and removes the now-obsolete C# GeometryExtentTests (the calculation is JS-only; covered by GeometryExtentSerializationTests and the browser TestUnion assertions). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

TimPurdum · 2026-06-19T17:52:27Z

Reworked the extent fix (0f42a874e) per feedback: missing extents are now computed by ArcGIS, not by hand-rolled C#/TS bounding-box logic.

geometry.ts buildDotNetGeometry — when an operator result (e.g. unionOperator) doesn't expose a cached .extent, it rebuilds a typed ArcGIS geometry via buildJsGeometry so the SDK recomputes the extent (Z/M bounds included automatically). It uses the geometry's own .extent when present.
Reverted the C# CalculateExtent / CalculateExtentFromPaths and the Polygon/Polyline overrides; Geometry.GetExtent() is back to its original form (the extent now arrives populated from the JS side).
Removed the C# GeometryExtentTests (those exercised the now-deleted C# calculation). Coverage remains via GeometryExtentSerializationTests and the browser TestUnion/TestUnionWithParamsArray extent assertions.

Verified: Core compiles, 19/19 unit tests pass, TS recompiles clean (ESLint OK). The browser e2e still needs a CI run.

🤖 Generated with Claude Code

magmoe and others added 2 commits June 17, 2026 11:49

magmoe requested a review from Copilot June 17, 2026 16:44

Copilot started reviewing on behalf of magmoe June 17, 2026 16:44 View session

Pipeline Build Commit of Version and Docs

55a93a5

Copilot AI reviewed Jun 17, 2026

View reviewed changes

magmoe and others added 2 commits June 17, 2026 13:03

Pipeline Build Commit of Version and Docs

fd74d3f

TimPurdum and others added 2 commits June 17, 2026 15:28

Pipeline Build Commit of Version and Docs

3de5fad

TimPurdum reviewed Jun 19, 2026

View reviewed changes

submodule-validation-for-geoblazor Bot and others added 2 commits June 19, 2026 12:05

Pipeline Build Commit of Version and Docs

ebe16be

submodule-validation-for-geoblazor Bot and others added 2 commits June 19, 2026 17:56

Pipeline Build Commit of Version and Docs

c000659

Merge branch 'develop' into fix/snapping-options-setters

d57792f

TimPurdum merged commit 8502e15 into develop Jun 19, 2026
1 check passed

TimPurdum deleted the fix/snapping-options-setters branch June 19, 2026 18:11

Navigation Menu

Uh oh!

Restore SnappingOptions runtime setters (4.5.x regression) - #530

Restore SnappingOptions runtime setters (4.5.x regression)#530
TimPurdum merged 12 commits into
developfrom
fix/snapping-options-setters

magmoe commented Jun 17, 2026

Uh oh!

claude Bot commented Jun 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

TimPurdum commented Jun 17, 2026

Uh oh!

magmoe commented Jun 17, 2026

Uh oh!

TimPurdum commented Jun 17, 2026

Uh oh!

TimPurdum left a comment

Uh oh!

TimPurdum commented Jun 19, 2026

Uh oh!

TimPurdum commented Jun 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

magmoe commented Jun 17, 2026

What

Why

Changes

Notes

Uh oh!

claude Bot commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

TimPurdum commented Jun 17, 2026

Uh oh!

magmoe commented Jun 17, 2026

Uh oh!

TimPurdum commented Jun 17, 2026

Uh oh!

TimPurdum left a comment

Choose a reason for hiding this comment

Uh oh!

TimPurdum commented Jun 19, 2026

Uh oh!

TimPurdum commented Jun 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

claude Bot commented Jun 17, 2026 •

edited

Loading