Skip to content

fix(processing): preserve dissolve attribute groups - #1978

Merged
giswqs merged 3 commits into
mainfrom
fix/discussion-1977-dissolve-groups
Aug 17, 2026
Merged

fix(processing): preserve dissolve attribute groups#1978
giswqs merged 3 commits into
mainfrom
fix/discussion-1977-dissolve-groups

Conversation

@giswqs

@giswqs giswqs commented Aug 17, 2026

Copy link
Copy Markdown
Member

Summary

  • collect disconnected Turf dissolve fragments into one Polygon or MultiPolygon per attribute value
  • report the number of attribute groups instead of the number of disconnected parts
  • add regression coverage for disconnected polygons sharing a dissolve value

Addresses #1977

Verification

  • node --import tsx --test --test-name-pattern="dissolves disconnected polygons" tests/processing.test.ts
  • npm run build
  • pre-commit run --files packages/processing/src/vector-tools.ts tests/processing.test.ts
  • browser-tested the reporter's OTEX-Cher-WGS.geojson: 290 polygons dissolve into 12 features for the 12 unique OTEX values
  • visually verified the result in light and dark themes

Copilot AI lite review requested due to automatic review settings August 17, 2026 14:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

An error occurred during the review process. Please try again later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

🔍 Cloudflare PR preview

Item Value
Site https://31fc3e50.geolibre-preview.pages.dev
Demo app https://31fc3e50.geolibre-preview.pages.dev/demo/
Commit c061471

Comment thread packages/processing/src/vector-tools.ts Outdated
Comment thread packages/processing/src/vector-tools.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Code review

Bugs

  • packages/processing/src/vector-tools.ts:115 — When a dissolve group has multiple disconnected parts, the merged feature's [field] value is set to the stringified Map key rather than the original property value, so a numeric field (e.g. group: 5) becomes a string ("5") only for features that were disconnected, while single-part groups keep the original type. This creates inconsistent typing within the same output layer and diverges from the sidecar engine (gdf.dissolve(by=field) in vector_ops.py), which preserves the column dtype. Medium confidence — posted a suggestion to use parts[0].properties?.[field] instead.
  • packages/processing/src/vector-tools.ts:110-122 — For multi-part groups, properties is rebuilt as field ? { [field]: value } : {}, dropping every attribute other than the dissolve field. Single-part groups (return parts[0]) keep whatever properties Turf attached (normally the first matching feature's full set), so otherwise-identical dissolve groups can end up with very different attribute sets depending only on whether their input polygons happened to be spatially connected. Medium confidence — posted a suggestion to spread parts[0].properties before applying the field override.

Security

  • None found.

Performance

  • None found — collectDissolveParts is a single O(n) pass over the dissolved features; fine for expected layer sizes.

Quality

  • The new regression test only covers a string-valued dissolve field and doesn't exercise: (a) a numeric field value (which would have caught the type-coercion bug above), or (b) the no-field case, where collectDissolveParts now also merges all disconnected parts into one feature under the "" key — a behavior change from the pre-PR code (which previously passed Turf's raw, possibly-fragmented output straight through) that isn't mentioned in the PR description. The no-field merge-all behavior itself looks correct and matches the sidecar's gdf.dissolve() (no by) semantics, but it's worth an explicit test given it's a silent, undocumented change. Low confidence on severity — this is a coverage gap, not a wrong result.

CLAUDE.md

  • No violations found. The change stays within packages/processing/src/vector-tools.ts and its test, doesn't touch any of the mirrored-constant modules called out in CLAUDE.md, and the PR verification steps (unit test, build, targeted pre-commit run) match the documented workflow.

- Preserve each dissolve field value's original type across connected and disconnected groups.
- Cover numeric group values and no-field disconnected dissolves.
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

🔍 GitHub Pages PR preview

Item Value
Site https://opengeos.org/pages-preview/GeoLibre/pr-1978/
Demo app https://opengeos.org/pages-preview/GeoLibre/pr-1978/demo/
Commit c061471

Note

GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating.

Comment thread packages/processing/src/vector-tools.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Code review

Bugs: None found. The reworked dissolve flow (collectDissolveParts + originalValues map) correctly regroups Turf's per-part dissolve output by attribute value, rebuilds disconnected parts into a single MultiPolygon per group, restores the original (possibly non-string) field value type, and reports the group count instead of the fragment count. Traced through both the field and no-field code paths, including the multi-part MultiPolygon coordinate assembly (parts.map(part => part.geometry.coordinates)), which is structurally correct for GeoJSON. The regression tests correctly exercise the numeric-type-preservation case via a strict === .find() before the assert.equal, so they'd actually catch a type-coercion regression. Confidence: high.

Security: No issues — purely client-side geometry processing on already-loaded layer data, no new I/O, injection, or untrusted-input handling. Confidence: high.

Performance: No concerns; the added grouping pass is a single O(n) map over already-in-memory features, negligible next to the dissolve/union work already being done. Confidence: high.

Quality:

  • Minor naming nit in collectDissolveParts: the destructured value from groups.entries() is actually the string group key, not the original field value, which reads confusingly next to originalValues.get(value). Posted an inline suggestion to rename it to key. Confidence: medium (style-only, no behavioral impact).
  • Mixed-type field values that stringify identically (e.g. numeric 5 vs string "5") collapse into one group with only the first-seen value's type retained in the output. This mirrors Turf's own grouping behavior (which already merges such values) rather than being a new issue introduced by this PR, so it's noted only for awareness, not as a defect. Confidence: low.

CLAUDE.md: No violations — log message style matches the rest of the file (untranslated ctx.log strings are the existing convention across this module), and the added test lives in the existing tests/processing.test.ts suite following established patterns. Confidence: high.

- Carry the first source feature's whole attribute set through Dissolve instead
  of rebuilding properties from just the dissolve field, so attributes no longer
  disappear when a group is merged (matching the sidecar's GeoPandas
  `dissolve`, aggfunc="first"). The field's original value/type still wins
  because it comes from that same source feature.
- Copy the properties object so the result layer never aliases the input layer's.
- Extend the dissolve tests to assert a non-dissolve attribute survives, both
  with and without a dissolve field.
@giswqs
giswqs merged commit 917dc58 into main Aug 17, 2026
22 of 25 checks passed
@giswqs
giswqs deleted the fix/discussion-1977-dissolve-groups branch August 17, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants