fix(parseMap): keep custom-aggregated channels when the saved field is null [sc-561439] - #328
Merged
padawannn merged 1 commit intoJul 30, 2026
Conversation
…s null A channel styled by a custom aggregation carries its expression in visConfig and can reach parseMap with no visualChannels field: Builder synthesizes one before rendering, but a map persisted by another writer does not. resolveCustomAggregation bailed out in that case, so no accessor was built and the layer fell back to its fixed color — a graduated H3 map rendered as a flat fill through fetchMap while Builder showed it correctly. The compiled alias identifies the aggregated column on its own, so synthesize the field from it instead of dropping the channel. Covers all five custom-aggregation channels and restores the scale that legends read. Story: sc-561439 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
padawannn
marked this pull request as ready for review
July 30, 2026 11:34
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Zulcom
approved these changes
Jul 30, 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
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.
Summary
A spatial-index layer styled with a custom aggregation renders as a flat fixed color through
fetchMapinstead of its graduated palette. Builder shows the same map correctly, so the saved config is fine — the divergence is in this package.Both render paths call the same
parseMap. The difference is what reaches it: Builder pipes its in-memory layer through a serializer that synthesizesvisualChannels.colorFieldfrom the aggregation expression, then callsparseMap.fetchMapreads the saved JSON straight from the API, where that field can benull, andresolveCustomAggregationbailed out on!field— so no accessor was built and the layer fell back to its fixed color.The compiled alias identifies the aggregated column on its own (the function already overwrites
accessorKeywith it), so the field was only a carrier forname/type. Synthesize it from the alias instead of dropping the channel.Story: sc-561439
Builder
FetchMap before
FetchMap after
What changed
src/fetch-map/parse-map.ts— split the!field || !expressionguard so a missing field with a live expression synthesizes{name: alias, type: 'integer', accessorKey: alias}. Empty expression still drops the channel.test/fetch-map/parse-map.spec.ts— two tests in the existingcustom aggregationblock.resolveCustomAggregationis shared by all five custom-aggregation channels (color, strokeColor, size, height, radius), so this covers them all. It also repopulatesscales.*, which was left empty — that's what legends read, so the legend came back with it.Review focus
The guard change is the whole fix; only one input combination behaves differently:
fieldexpressionWorth questioning:
type: 'integer'. It's inert for rendering here (getColorAccessoronly readsname/colorColumn/accessorKey, andcalculateDomainkeys off the name), but it surfaces inscales.*.field, which consumers read for legends. I matched the value Builder persists so both synthesis sites produce identical descriptors — say if you'd rather it werereal, since aSUM(x)/SUM(y)ratio isn't an integer.How to validate
Against a saved H3 map whose layer has
colorAggregation: "custom", a non-emptycolorAggregationExpandvisualChannels.colorField: null:Evaluating the accessor across the configured
colorAggregationDomainyields distinct colors from the palette rather than one repeated value. Verified end-to-end on the reported map (c4625c4a-4a61-4bae-839c-f236dce04a37): flat teal[18,147,154,230]before, the full Global Warming ramp#5A1846 → #FFC300after. Before/after screenshots are on the story.Tests
yarn test— 460 pass, 2 skipped, no type errors. Lint and Prettier clean.The new tests cover the regressing case (accessor built, scale
quantize, and the ramp actually varies — a constant accessor would satisfy a weaker assertion) and the empty-expression case, which must keep dropping the channel.Risk
Downstream consumers were checked against a local build of this branch:
workspace-wwwbuilder suite (2549 tests), maps suite (194), andmcp-app(113) all pass.One behavior change worth flagging beyond the bug being fixed: in
cloud-native,MapPreviewContainercan load a map from the backend and hand it toparseMapwithout the Builder serializer. Custom-aggregated layers whose saved config lacks the field go from flat to graduated there too — which is the intent, and it converges with what the Builder canvas already shows, but it is a visible change on that surface.AI-generated code
Yes, written with Claude Code. The guard change and the
type: 'integer'choice are the parts worth a human eye.