Fix/variable input fractional numbers - #170
Merged
Merged
Conversation
A `number` variable input could not carry a fractional value, and any
variable input legitimately set to 0 was indistinguishable from unset.
Binding a plugin arg to a threshold like 0.15 therefore produced
"<name> variable is empty" on every dependent visualization.
Four falsy/integer assumptions caused it:
- VariableInput parsed the value with parseInt on both mount and change,
so 0.15 became 0. Fixed with a shared toNumberOrEmpty helper, which
also maps an unparseable entry to "" rather than NaN — NaN would
otherwise read as a set value.
- checkForEmptyVariableInputs tested truthiness, so a numeric 0 or a
false checkbox reported as empty.
- updateObjectWithVariableInputs used `|| ""` in the type-preserving
branch, collapsing 0 and false to "" — inconsistent with the `?? ""`
in the sibling branch, so plugins silently fell back to their own
defaults instead of receiving the configured 0.
- VariableInput's context-sync effect gated on truthiness, so a 0
arriving from the context never synced into local state.
DashboardLoader now applies the same numeric coercion when it seeds the
context. Without it the boot-time seed was the raw string ("0.3") while
the mount effect published a number (0.3), leaving the context value's
type dependent on which landed last.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Backspacing "0.3" down to "0." deleted the decimal point too, so the next keystroke produced 5 instead of 0.5. The parent normalizes whatever the input publishes and feeds the number back as `value`. "0." parses to 0, so the resync effect echoed "0" and overwrote the entry in progress. Trailing zeros were lost the same way: typing "0.50" published 0.5, which came back as "0.5". The effect now skips the resync when the raw text already denotes the incoming number, so only a genuinely different value overwrites the box. An external update still applies, since its number differs from what is on screen. This also drops an accidental assertion in SettingsPane.test.js. It expected a rejected negative refresh rate to leave "0" in the box, which only held because the mount effect happened to flush after the change event and overwrite the entry. onRefreshRateChange rejects negatives rather than clamping them, so once mounted the box keeps what was typed while the setting stays unchanged -- unaltered by this commit, and the test now states it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A dynamic_map_layer could load with a valid, non-empty response and paint nothing. Changing one of its bound arguments made it appear, which is what made it look intermittent. Map.js builds its layers asynchronously, so on a dashboard load the feature fetch can resolve before the OpenLayers layer it belongs to exists. performFetch looked the layer up once, and silently discarded the payload when it was not there yet. It never recovered: the resolved args are recorded before the request goes out, so the reconciliation effect then saw argsUnchanged and never refetched. Only a genuine argument change produced another fetch -- hence the layer appearing after a threshold edit. The payload is now held and applied on the layer collection's "add" event, so it paints as soon as the layer exists without a second request. The listener is released when it fires, when a newer fetch supersedes it, when the layer is removed, and on unmount. A response arriving with no map at all now clears the recorded args so the next reconciliation retries rather than treating the fetch as done. Also fixes the requestId carrying a literal "undefined" for the grid item: the hook destructured gridItemUuid while its only caller passes gridItemUUID, the name GridItemContext uses. Per-layer progress messages were routed under that malformed id. The hook's own tests passed the lowercase spelling, so nothing caught it -- the new Map test wraps a real GridItemContext and asserts the id is well formed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Raster layers had four ramps: viridis, turbo, RdYlBu and grayscale. That left no single-hue sequential option for depth or precipitation, and only one diverging option for anomaly data. Adds the rest of the perceptually uniform family (magma, inferno, plasma, cividis), three ColorBrewer sequential maps (Blues, YlGnBu, YlOrRd), and three more diverging maps (RdBu, Spectral, BrBG) -- fourteen in total. Keystops were sampled from matplotlib 3.10 at twelve evenly spaced points, the convention viridis and turbo already used, rather than transcribed by hand. The default stays turbo, so existing layers are unaffected. Fourteen unlabelled swatches in a flat column would be worse than four, so the picker now groups them under Perceptually uniform / Sequential / Diverging, names each row, and scrolls within its own box instead of pushing the rest of the Style tab off-screen. RAMP_GROUPS is the single source of order and RAMP_NAMES derives from it, so a ramp cannot be added to the registry and left unreachable in the UI. RAMP_STOPS is unchanged at 32: it bounds the stops in one layer's shader expression, which does not grow with the number of ramps on offer. Tests now iterate the registry rather than a hardcoded list of four, and assert the properties that a mis-sampled table breaks: the perceptually uniform ramps rise monotonically in luminance, the diverging ramps are lightest at their midpoint, and the groups cover exactly the registered names with no duplicates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reversing a ramp is the difference between reading a Blues layer as depth and reading it as elevation, and it was previously only possible by authoring a style expression by hand. Adds a `rampReverse` flag to GeoTIFF and Zarr sources, toggled by a "Reverse ramp" checkbox under the picker. Reversal happens in one place -- resolveRamp in colorRamps.js -- so the five consumers cannot disagree about which end is which: the style expression, its render-time rebuild in ModuleLoader, the save path in MapLayer, the editor's swatches, and the map legend's gradient. The picker previews the reversed direction, so what is selected matches what the map draws. resolveRamp returns the shared array untouched when unreversed and a copy when reversed; an in-place reverse would corrupt the ramp for every other consumer. The flag is persisted only when true, so an unreversed layer's saved config is byte-identical to before this option existed, and it is hidden in categorical mode, where a discrete class list has no direction to flip. Also removes the per-row ramp names from the picker. They were added with the grouping in the previous commit and were not there before; the group headings carry the organisation on their own, and the names are still exposed to assistive tech through each row's aria-label. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fix fractional/zero variable inputs and blank dynamic map layers; expand raster color ramps
Five commits. The first three are bug fixes that compound into one user-visible failure — a dashboard whose thresholds are fractions renders nothing. The last two are the raster color ramp work.
main...HEAD: 22 files, +1076 / −114.The reported symptom
A dashboard binds four probability thresholds (0.3 / 0.2 / 0.1 / 0.15) to
numbervariable inputs and feeds them to plugin args. Every dependent visualization renders"<name> variable is empty". The map layer, separately, stays blank on load despite a valid non-empty response, then appears as soon as any threshold is edited.Three independent defects were behind that, plus one editing bug found while fixing them.
1.
numbervariable inputs could not hold a fraction, or a zeroFour falsy/integer assumptions, each sufficient on its own.
VariableInput.js(mount + change)parseInttruncated the value:0.15→0checkForEmptyVariableInputs0reported as empty rather than as a visibly wrong0— this is the error text the user saw. Also fires for an unchecked checkboxupdateObjectWithVariableInputs|| ""in the branch whose entire purpose is preserving the value's type. A threshold of0reached the plugin as"", which silently fell back to the plugin's own default — a wrong answer with no error. The sibling branch twenty lines below already used?? ""VariableInput.jscontext-sync effect0arriving from the context never synced into local stateTwo shared helpers in
components/visualizations/utilities.js:toNumberOrEmpty(value)—parseFloat, mapping an unparseable entry to""rather thanNaN.NaNis neithernullnor"", so it would satisfy the new presence check and reintroduce the same bug class.hasVariableInputValue(value)— presence, not truthiness.undefined,nulland""are unset;0andfalseare values.DashboardLoadernow applies the same numeric coercion when it seeds the context. Without it the boot-time seed was the raw string"0.3"while the mount effect published the number0.3, leaving the context value's type dependent on which landed last. This surfaced as a genuinely flaky test before being fixed.No change was needed in
NormalInputfor this — it already keeps a raw-text buffer for number inputs.2. Backspacing a decimal deleted the decimal point
0.3→ backspace → expected0., got0, so the next keystroke read5instead of0.5.The parent normalizes what the input publishes and feeds the number back as
value."0."parses to0, soNormalInput's resync effect echoed"0"and overwrote the entry in progress. Trailing zeros were lost the same way: typing"0.50"published0.5, which came back as"0.5".The effect now skips the resync when the raw text already denotes the incoming number, so only a genuinely different value overwrites the box. External updates still apply.
3. Dynamic map layers stayed blank when the fetch won the race
Map.jsbuilds layers asynchronously, so on a dashboard load the feature fetch can resolve before the OpenLayers layer exists.performFetchlooked the layer up once and silently discarded the payload when it was not there yet — no error, no retry.It never recovered: the resolved args are recorded before the request goes out, so the reconciliation effect then saw
argsUnchangedand never refetched. Its deps are[layers, variableInputValues, variableInputDateFormats, refreshTick], none of which change when OL finishes constructing. Only a genuine argument change produced another fetch — hence the layer appearing after a threshold edit.The payload is now held and applied on the layer collection's
"add"event, so it paints as soon as the layer exists with no second request. The listener is released when it fires, when a newer fetch supersedes it, when the layer is removed, and on unmount. A response arriving with no map at all clears the recorded args so the next reconciliation retries.Also fixes a malformed
requestId. The hook destructuredgridItemUuidwhile its only caller passesgridItemUUID, the nameGridItemContextuses — so every dynamic-layer request carried a literal"undefined"for the grid item and per-layer progress messages routed under that id. The hook's own tests passed the lowercase spelling, which is why nothing caught it.4. Ten more continuous color ramps
Raster layers had four: viridis, turbo, RdYlBu, grayscale. No single-hue sequential option for depth or precipitation, no heat/risk ramp, one diverging option for anomaly data, and the viridis family incomplete.
cividisearns its place specifically because it is optimized for red-green color vision deficiency, which turbo handles poorly.Keystops were sampled from matplotlib 3.10 at twelve evenly spaced points — the convention viridis and turbo already used — rather than transcribed by hand. matplotlib's
Blues/YlGnBu/YlOrRd/RdBu/Spectral/BrBGare the ColorBrewer maps of those names.Fourteen unlabelled swatches in a flat column would be worse than four, so the picker groups them by family and scrolls within its own box instead of pushing the rest of the Style tab off-screen.
RAMP_GROUPSis the single source of order andRAMP_NAMESderives from it, so a ramp cannot be registered and left unreachable in the UI.RAMP_STOPSis unchanged at 32. It bounds the stops in one layer's shader expression, which does not grow with how many ramps are on offer, so the WebGL instruction limit is untouched.5. Reverse ramp
Reversing a ramp is the difference between reading a Blues layer as depth and reading it as elevation. It was previously only possible by hand-authoring a style expression.
Adds a
rampReverseflag to GeoTIFF and Zarr sources with a "Reverse ramp" checkbox under the picker. The flip happens in exactly one place —resolveRamp— because five consumers have to agree on which end is which: the style expression, its render-time rebuild inModuleLoader, the save path inMapLayer, the editor's swatches, and the map legend's gradient. Miss one and the legend contradicts the raster, or the layer flips back on reload.resolveRampreturns the shared array untouched when unreversed and a copy when reversed; an in-place.reverse()would corrupt the ramp for every other consumer.The flag persists only when
true, so an unreversed layer's saved config is byte-identical to before this option existed. It is hidden in categorical mode, where a discrete class list has no direction to flip.Verification
Full suite green: 130 suites, 2431 tests, 99.76% statements. Lint and Prettier clean.
Every new test was mutation-checked against the final code — the mutation must fail the suite and the baseline must restore clean:
parseFloat→parseInthasVariableInputValue→!!value??→||in the type-preserving branchNormalInputgridItemUuidcasingRdButo theBluestablegrayscalefrom its groupresolveRampignores the reverse flagrampReverseTwo colormap invariants were added that catch a mis-sampled table rather than merely a missing one: the perceptually uniform ramps rise strictly monotonically in luminance, and the diverging ramps are lightest at their midpoint.
The decimal-editing test was run 8× to confirm the type race is gone.
Things a reviewer should look at
Behaviour change:
falseand0now reach plugin args asfalseand0instead of"". That is the point of fix 1, but a plugin relying on""for an unchecked checkbox will now receivefalse. No existing test depended on the old behaviour.An existing test was passing for the wrong reason.
SettingsPane.test.jsexpected a rejected negative refresh rate to leave"0"in the box. Instrumenting both versions showed that only held because the mount effect happened to flush after the change event and overwrite the entry:onRefreshRateChangerejects negatives rather than clamping them, and nominis passed to the input, so once mounted the box has always kept what was typed while the setting stayed unchanged. This PR does not change that behaviour — it only removes the accidental pass. The assertion now states the real contract, with a comment.A pre-existing wart this exposes, left alone deliberately: the refresh-rate box can display
-2while the stored setting is0. Out of scope here — worth a follow-up that either clamps inonRefreshRateChangeor passesminand rejects below it.A second existing test was quietly encoding bug 3.
"Map runtime layer swap dismisses popup overlay"assertsVectorSource.clearwas not called, which only held because the race was skipping the swap. It still passes, but it was asserting the bug.Scope of the new Map integration test.
components/map/utilitiesisjest.mocked inMap.test.js, soswapVectorLayerFeaturesis a stub there andaddFeaturescan never fire. The test asserts the hand-off — correct target layer, correct payload, well-formedrequestId— not OpenLayers' own parsing. Instrumenting the fetcher confirmed the chain end-to-end (foundLayer: true, featureCount: 1) before I trusted the fix; the deferred path is covered at the hook level.Branch name is now narrower than the contents (
fix/variable-input-fractional-numbers), since the ramp work was added to the same PR by request.