feat(processing): enter distances in meters on WGS84 layers - #1560
Merged
Conversation
Vector layers reach a Whitebox tool as GeoJSON, which RFC 7946 fixes to WGS84, so every spacing, radius and tolerance value is measured in degrees with no CRS left on the layer to say so. Distance parameters now carry a unit picker (degrees, meters, kilometers, feet, miles). Entering a metric value converts it to the degrees the tool reads, anchored at the input layer's center latitude, and the field shows the degree value it produced. A note above the form states that the selected layers use a geographic CRS and points at reprojection for exact results. The picker only appears when the tool's coordinates are known to be WGS84: every dataset input is a vector layer with in-memory GeoJSON. A raster or LiDAR input keeps its own CRS, and a file path is read in whatever CRS it carries, so those tools are left alone. Fixes #1540
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProcessing distance fields now detect WGS84 layer context, provide unit selection for eligible parameters, convert selected units to degrees for Whitebox tools, and display localized geographic-distance guidance. Conversion and formatting behavior is covered by new tests. ChangesWGS84 distance unit inputs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessingDialog
participant DistanceInput
participant WhiteboxTool
ProcessingDialog->>DistanceInput: provide WGS84 reference latitude
DistanceInput->>DistanceInput: convert selected unit to degrees
DistanceInput->>WhiteboxTool: submit degree-based distance value
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
🔍 Cloudflare PR preview
|
Contributor
🔍 GitHub Pages PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Performance
Security
Quality
CLAUDE.md
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx`:
- Around line 264-295: Update wgs84ReferenceLatitude so any vector_in parameter
whose value is not a layer token returns null, regardless of whether
param.required is true. Remove the required-only condition and preserve the
existing latitude lookup and averaging behavior for valid layer-token inputs.
- Around line 2421-2492: Update the DistanceInput component to resynchronize its
local unit and draft state when the externally supplied value or latitude
changes, including tool switches, history reruns, and layer changes. Add the
necessary effect or equivalent reconciliation around value, latitude, and the
existing hasDegrees conversion logic so the displayed non-degree draft matches
the current stored degrees value, while preserving user edits and the existing
changeUnit/changeDraft behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee224602-b958-41b6-87e5-cb2e1d8b9027
📒 Files selected for processing (4)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/whitebox-distance-params.tstests/whitebox-distance-params.test.ts
- Re-convert a metric distance when the input layer changes: the field is keyed by parameter name, so switching a tool's vector input from a layer at 44N to one at 9N kept the typed value on screen while the stored degrees still came from the old latitude, and Run sent the stale value. An effect guarded by the previous latitude now pushes the recomputed degrees up. - Measure only the layers the selected tool actually reads instead of running an unbounded turf bbox over every GeoJSON layer in the project on the React commit path. The vector-input helper now returns layer ids, joined into a memo key so the extent scan survives keystrokes.
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 (1)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx (1)
248-286: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOptional
vector_inleft on a path still bypasses the WGS84 safety check.The docstring (lines 251-256) states any vector input "still set to a path" disqualifies the tool from the unit picker, but the loop body (unchanged from the prior commit) only enforces this via
if (param.required) return null;at line 280 — an optionalvector_inexplicitly left on a file path is simplycontinued past. Its geometry is still read from disk in its own CRS byrunSelectedTool(a raw path string, notlayer_inputs), so the picker can end up claiming a WGS84-known unit context based only on the other inputs while this one silently contributes projected coordinates.🐛 Proposed fix
for (const param of vectorInputs) { const value = values[param.name]; - if (typeof value !== "string" || !value.startsWith(LAYER_TOKEN_PREFIX)) { - // A required input still on "Path" (or empty) means the run may read a - // projected file instead; don't claim to know the units. - if (param.required) return null; + if (typeof value !== "string" || value.trim() === "") { + // Nothing selected yet: a required one can't run regardless, and an + // optional one that's genuinely empty contributes no geometry. + if (param.required) return null; continue; } + if (!value.startsWith(LAYER_TOKEN_PREFIX)) { + // Any vector input — required or not — left on a path is read from + // disk in its own CRS, so the picker can't claim to know the units. + return null; + } ids.push(value.slice(LAYER_TOKEN_PREFIX.length)); }This mirrors a previously-flagged concern on
wgs84ReferenceLatitude's equivalent logic, which is still unresolved after the rename towgs84VectorLayerIds.🤖 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 `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx` around lines 248 - 286, Update wgs84VectorLayerIds so any vector_in value that is missing or not a LAYER_TOKEN_PREFIX path, including optional inputs, immediately returns null. Remove the required-only exception while preserving collection of layer IDs for valid layer-token values.
♻️ Duplicate comments (1)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx (1)
2418-2462: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLatitude-staleness fixed; value-staleness on tool switch/rerun still open.
The new
lastLatituderef + effect correctly fixes the previously-flagged bug where switching a WGS84 input layer left the displayed draft/unit converted at a stale latitude — it re-runsunitToDegreeswheneverlatitudechanges whileunit !== "degrees", and the ref-guard prevents any loop fromonChange's new identity each render.However,
unit/draftstill never resync when thevalueprop changes for reasons other than latitude:
- Switching tools:
ParameterField key={param.name}(line 1972) has no tool id, so a different tool reusing a same-named distance param (e.g.radius,distance,tolerance) keeps thisDistanceInputinstance mounted whilevalueresets to the new tool's unrelated default (line 1137-1144'ssetValues(createDefaultValues(selectedTool))).- Re-running from Processing History:
setValues({...defaults, ...rerun.parameters})(line 1167-1171) changesvaluewithout touching local state.In both cases the stored degrees is correct, but the UI can keep showing a stale unit + number pairing that no longer matches it.
🐛 Proposed fix: reconcile local state whenever an externally-pushed value is detected
function DistanceInput({ id, latitude, onChange, value }: DistanceInputProps) { const { t } = useTranslation(); const [unit, setUnit] = useState<DistanceUnit>("degrees"); const [draft, setDraft] = useState(""); + // Tracks the last value *this component* pushed via onChange, so an + // externally-changed `value` (tool switch or rerun prefill) can be told + // apart from our own edits. + const lastPushedRef = useRef<string | null>(null); + + useEffect(() => { + if (value === lastPushedRef.current) return; + setUnit("degrees"); + setDraft(""); + }, [value]);This is the same gap flagged on an earlier commit of this component.
🤖 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 `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx` around lines 2418 - 2462, Update DistanceInput to resynchronize its local unit and draft whenever the externally supplied value changes, not only when latitude changes. Add a value-change effect that detects external updates, resets the display to the degrees representation (clearing draft or converting the new value consistently), and keeps the existing latitude reconversion behavior for metric units without creating an update loop.
🤖 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 `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx`:
- Around line 248-286: Update wgs84VectorLayerIds so any vector_in value that is
missing or not a LAYER_TOKEN_PREFIX path, including optional inputs, immediately
returns null. Remove the required-only exception while preserving collection of
layer IDs for valid layer-token values.
---
Duplicate comments:
In `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx`:
- Around line 2418-2462: Update DistanceInput to resynchronize its local unit
and draft whenever the externally supplied value changes, not only when latitude
changes. Add a value-change effect that detects external updates, resets the
display to the degrees representation (clearing draft or converting the new
value consistently), and keeps the existing latitude reconversion behavior for
metric units without creating an update loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 08e01eff-20fa-45ce-a3d2-b5c98903bbc9
📒 Files selected for processing (1)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
The core conversion math ( |
- Treat any vector input left on a file path as unknown units, not just a required one. An optional input pointing at a projected file is still read from disk in its own CRS, so the picker must not claim to know the tool's units. An empty optional input still contributes nothing. - Key each parameter field by tool id as well as parameter name. Dozens of tools share names like tolerance or radius, so without the tool id React can reuse a field instance across a tool switch and carry a distance field's unit and typed draft onto another tool's default. - Reset a distance field to plain degrees when its stored value is rewritten from outside, such as a re-run pre-filled from Processing History, so the displayed unit and number cannot describe a distance that is no longer stored.
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 (1)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx (1)
2436-2492: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLatitude-reconversion effect can clobber a simultaneous external value rewrite.
The two effects run in declaration order within the same commit, both reading the pre-update
unit/draftcaptured by that render. When a Processing History rerun for the same tool also swaps the wired vector-input layer,value(this param, fromrerun.parameters) andlatitude(viadistanceLatitude, derived from the new layer) change together in onesetValuescommit. The reset effect (2470-2475) correctly detects the external rewrite and clearsunit/draft, but the reconversion effect (2483-2492) still fires becauselatitudeis in its own deps and changed — using the staledraft/unitfrom that same render — and pushes a recomputed value that overwrites the just-restored, correct externally-set value. The tool then silently runs with a wrong distance parameter.🐛 Proposed fix: give the external-rewrite case priority in a single effect
- useEffect(() => { - if (value === lastPushed.current) return; - lastPushed.current = value; - setUnit("degrees"); - setDraft(""); - }, [value]); - - const lastLatitude = useRef(latitude); - useEffect(() => { - if (lastLatitude.current === latitude) return; - lastLatitude.current = latitude; - if (unit === "degrees") return; - const parsed = Number.parseFloat(draft); - if (!Number.isFinite(parsed)) return; - const next = formatDistanceValue(unitToDegrees(parsed, unit, latitude), 8); - lastPushed.current = next; - onChange(next); - }, [draft, latitude, onChange, unit]); + const lastLatitude = useRef(latitude); + useEffect(() => { + lastLatitude.current = latitude; + if (value !== lastPushed.current) { + // External rewrite (e.g. a Processing History re-run, possibly paired + // with a vector-input swap that also shifted `latitude`). Trust it over + // any stale draft/unit rather than re-converting from leftover text. + lastPushed.current = value; + setUnit("degrees"); + setDraft(""); + return; + } + if (unit === "degrees") return; + const parsed = Number.parseFloat(draft); + if (!Number.isFinite(parsed)) return; + const next = formatDistanceValue(unitToDegrees(parsed, unit, latitude), 8); + lastPushed.current = next; + onChange(next); + }, [value, draft, latitude, onChange, unit]);🤖 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 `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx` around lines 2436 - 2492, Consolidate the effects watching value and latitude so the external value-rewrite path has priority over latitude reconversion. In the effect around lastPushed, detect value !== lastPushed.current, update lastPushed, reset unit to degrees and draft to empty, then return before any reconversion or onChange call; only reconvert the existing metric draft when the value was not externally rewritten.
🤖 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 `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx`:
- Around line 2436-2492: Consolidate the effects watching value and latitude so
the external value-rewrite path has priority over latitude reconversion. In the
effect around lastPushed, detect value !== lastPushed.current, update
lastPushed, reset unit to degrees and draft to empty, then return before any
reconversion or onChange call; only reconvert the existing metric draft when the
value was not externally rewritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db62d34d-546d-4f9d-8b24-23a648bc5fb1
📒 Files selected for processing (1)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx
Contributor
Code reviewBugs
Performance
Quality
Security
CLAUDE.md adherence
Overall the core conversion math ( |
- Parse a typed distance with Number instead of parseFloat. parseFloat stops at the first character it cannot read, so "12,000" became 12 and was converted as a distance a thousand times too small while the field still showed the original text. Malformed text is now stored as typed, which fails at the tool, and the field says so instead of claiming a conversion that did not happen. - Extract the per-input WGS84 rule and the draft parser into whitebox-distance-params.ts and unit-test them. That branching is where the optional-path bug lived, and it is now covered directly. - Note the residual false-positive risk of the generic length and resolution name segments, which the vector-only tool gate handles today but a future tool could defeat.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx (1)
2463-2488: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-run + latitude change in the same commit can silently clobber the restored distance value.
When "Edit & re-run" targets the same tool (no
ParameterFieldremount) and the re-run also changes a wired vector-input layer,setValues({...defaults, ...rerun.parameters})updates both this parameter'svalueand — via thedistanceLayerKey/distanceLatitudememos — thelatitudeprop in one render. Both effects then fire in declaration order: the latitude effect reads the stale, pre-resetunit/draft(captured in the same render, before the value-effect'ssetUnit/setDraftcalls have applied) and callsonChangewith a bogus reconversion of leftover draft text — overwriting the just-restored rerun value in the parent'svaluesstate.This is a different combination than the tool-switch/external-rewrite/pure-latitude-change races already fixed in prior rounds.
🐛 Proposed fix
const lastLatitude = useRef(latitude); useEffect(() => { if (lastLatitude.current === latitude) return; lastLatitude.current = latitude; if (unit === "degrees") return; + // If `value` was also rewritten externally in this same render (e.g. a + // same-tool re-run that restores this parameter and shifts the inferred + // latitude together), let the value-reset effect above win instead of + // reconverting stale draft text over the freshly-restored value. + if (value !== lastPushed.current) return; const parsed = parseDistanceInput(draft); if (parsed === null) return; const next = formatDistanceValue(unitToDegrees(parsed, unit, latitude), 8); lastPushed.current = next; onChange(next); - }, [draft, latitude, onChange, unit]); + }, [draft, latitude, onChange, unit, value]);🤖 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 `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx` around lines 2463 - 2488, Coordinate the value-reset and latitude-reconversion effects so a render that changes both value and latitude skips reconversion using the stale unit/draft. In the latitude effect near lastLatitude, detect that value changed in the same render and only update latitude tracking; retain reconversion for genuine latitude-only changes and existing guards.
🤖 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.
Inline comments:
In `@apps/geolibre-desktop/src/i18n/locales/en.json`:
- Line 3450: Update the en.json `convertedEmpty` translation to describe the
degree approximation or interpretation without implying that a conversion has
already occurred, matching its use by `DistanceInput` when `draft.trim() ===
""`.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx`:
- Around line 2463-2488: Coordinate the value-reset and latitude-reconversion
effects so a render that changes both value and latitude skips reconversion
using the stale unit/draft. In the latitude effect near lastLatitude, detect
that value changed in the same render and only update latitude tracking; retain
reconversion for genuine latitude-only changes and existing guards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0ca6f5db-8dd7-4e9f-bd0d-2e9f6ca60fd5
📒 Files selected for processing (4)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/whitebox-distance-params.tstests/whitebox-distance-params.test.ts
- Exclude corridor_tolerance from the distance-name rule. It is a fraction above optimal cost in 0-1, not a width, so the tolerance segment was claiming it. Only the tool-level gate keeps it out of reach today, which is a coincidence rather than a guarantee, so it now has an explicit exclusion list. A catalog scan for a matching double whose description reads as a fraction, ratio, angle or weight turned up no other case. - Move the formatDistanceValue docstring back onto formatDistanceValue; it was left stranded above parseDistanceInput when the helpers were added. - Reword the empty-field note: it is shown before anything is typed, so it now reads "Values will be converted" instead of claiming a conversion that has not happened.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Require a plain decimal before converting a typed distance. Number also
reads the 0x/0b/0o literals and Infinity, so a pasted "0x64" would have
converted as 100 metres instead of passing through untouched like other
text the field cannot read. Partly typed decimals ("5.", ".5") still
convert, so the field keeps up as the user types.
- Anchor a multi-input conversion at the middle of the layers' combined
extent rather than the mean of their centres. Averaging each layer's own
centre weights a city-sized input the same as a country-sized one and can
land on a latitude neither input is near: with a local road network and a
worldwide point set it gave 26.7N, where the combined extent gives 9.3N.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
The metric box records what it pushed so the field can tell an outside rewrite from its own edit, but the degrees stepper was wired straight to the parent, leaving that record stale and making every keystroke look like an outside rewrite to the reconcile effect. It is a no-op today because the reset it triggers matches the state already there, but the invariant should hold before the effect grows.
Contributor
Code reviewBugs: None found. Traced the WGS84-detection gating ( Security: None found. No injected HTML, no eval, purely numeric client-side conversion. Confidence: high. Performance: None found. The extent scan ( Quality:
CLAUDE.md: Followed — new user-facing strings go through |
Redisplaying the stored degrees in a newly picked unit rounded to 6 significant digits while the stored value carries 8, so a value typed as 200.12345 came back as 200.123 after a trip through Degrees. I had assumed matching the digits would make ordinary values read as 199.99999; measuring it shows otherwise, since 200, 2000 and 1.5 all still round-trip exactly.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the core conversion math ( |
An outside rewrite and a latitude change were handled by separate effects, and a Processing History re-run can do both in one setValues: it replaces the stored value and can rebind the input layer. The latitude effect then still saw the pre-reset unit and draft from that render's closure and pushed a reconversion of the stale draft over the value the reset was restoring, so History replayed something other than what ran. One effect now handles both with the precedence stated: an outside rewrite wins. Also label a conversion anchored at 0.0 as "the equator" rather than "0.0°N", which a worldwide layer's combined extent reaches often enough for the hemisphere suffix to read as a mistake.
Comment on lines
+2591
to
+2615
| const lastLatitude = useRef(latitude); | ||
| useEffect(() => { | ||
| const latitudeChanged = lastLatitude.current !== latitude; | ||
| lastLatitude.current = latitude; | ||
| if (value !== lastPushed.current) { | ||
| // Someone else rewrote the parameter, so the typed draft and its unit | ||
| // describe a distance that is no longer stored: show the degrees that are, | ||
| // and let that win over any re-conversion. | ||
| lastPushed.current = value; | ||
| setUnit("degrees"); | ||
| setDraft(""); | ||
| return; | ||
| } | ||
| // The input layer moved under a metric entry: switching a tool's vector | ||
| // input from a layer at 44°N to one at 10°N leaves the typed distance on | ||
| // screen while the stored degrees still come from the old latitude, so the | ||
| // note would show the new latitude beside a value converted at the old one | ||
| // and Run would send the stale degrees. | ||
| if (!latitudeChanged || unit === "degrees") return; | ||
| const parsed = parseDistanceInput(draft); | ||
| if (parsed === null) return; | ||
| const next = formatDistanceValue(unitToDegrees(parsed, unit, latitude), 8); | ||
| lastPushed.current = next; | ||
| onChange(next); | ||
| }, [draft, latitude, onChange, unit, value]); |
Contributor
There was a problem hiding this comment.
The reconcile effect depends on onChange, but the parent passes a fresh inline closure on every render (onChange={(value) => updateValue(param.name, value)}, further down in this file). Since values changes on every keystroke anywhere in the form, ProcessingDialog re-renders and every mounted DistanceInput's onChange identity changes with it — so this effect re-runs for every distance field on every keystroke in the whole dialog, not just its own. The value !== lastPushed.current / latitudeChanged guards make each extra run a no-op, so this isn't a correctness bug, just churn. A useCallback around the per-field onChange (or dropping onChange from the dependency array, since it's stable in spirit) would avoid the redundant invocations.
Confidence: low-medium — purely a performance/quality nit, no observed incorrect behavior since the effect is idempotent when nothing relevant changed.
Member
Author
There was a problem hiding this comment.
Correct analysis, but I am leaving it. Each redundant run is two reference comparisons before the effect returns (value !== lastPushed.current, then latitudeChanged), across the handful of distance fields a tool exposes — that is not a cost worth buying back.
Both fixes have a real downside: dropping onChange from the dependency array makes the list dishonest and trips exhaustive-deps, and routing it through a ref adds indirection to the one effect in this component whose ordering already needed care (see the History re-run bug just above). I would rather the effect stay plainly readable than shave comparisons nobody can measure.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Two inline comments posted (both low/low-medium confidence, non-blocking): the all-or-nothing extent-bailout behavior in |
A layer whose features all carry a null geometry (an attribute table) contributes no coordinates to the operation, so it should not decide the conversion anchor for inputs that do have an extent. Binding one to an optional input was hiding the unit picker for every distance field on the tool. It is now skipped, and only an input with no in-memory GeoJSON at all still disqualifies the tool, since that one is read from a path in its own CRS.
Contributor
|
Both inline comments posted successfully. Now the final summary. Code reviewBugs: None found with high confidence. The core conversion math ( Security: None applicable — purely client-side numeric parsing/formatting and display text, no injected markup or external input handling. Performance: None of concern. The extent scan ( Quality:
CLAUDE.md: The new maintenance-mirror entry for |
- Move the distance branch above isPathParameter, like the CRS and map-extent pickers already are. That fallback matches path/file/folder anywhere in a parameter's name or description and does not exclude a numeric kind, so a distance whose wording mentioned one would have rendered as a file browser with no error. Nothing in the catalog collides today; the other branches guard against this for the same reason. - Show only one coordinate-units note. The older degrees warning and the new geographic-CRS note were stacking in exactly the case this feature targets, saying the same thing except that only the new one tells the user what to do about it.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall this is a carefully engineered change with good attention to edge cases (pole clamping, hemisphere labeling, round-trip precision, note precedence, stale-state resets); only one low-confidence performance nit was worth flagging inline. |
The extent scan reran whenever the layers array was replaced, which any layer mutation in the app does: a visibility toggle, a restyle, an unrelated layer being added. For a large input layer that is a full collection pass on the React commit path each time. A WeakMap keyed on the FeatureCollection itself means the scan happens once per layer and repeats only when that layer's data actually changes.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
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.
Fixes #1540
Problem
Vector layers reach a Whitebox tool as GeoJSON, which RFC 7946 fixes to WGS84, so every
spacing/search_radius/tolerancevalue is measured in degrees. There is no CRS left on the layer to say so, and reprojecting a layer does not help because the reprojected result is converted back to WGS84 when it is added to the map (the exact confusion reported in the issue).What this adds
Distance parameters in the Whitebox toolbox now carry a unit picker: Degrees (the default, unchanged behavior), Meters, Kilometers, Feet, Miles.
The conversion uses the geometric mean of the meridional and parallel degree lengths at that latitude. A single scalar cannot be right in both directions on a geographic CRS, so the mean splits the error instead of favoring one axis, and the field says the result is approximate.
Where it does not appear
The picker is only offered when the tool's coordinates are known to be WGS84, that is, every dataset input is a vector layer with in-memory GeoJSON. A tool with a raster or LiDAR input keeps that dataset's own CRS (GeoLibre never reprojects those), and a vector input still set to a file path is read from disk in whatever CRS it carries. In both cases the field stays a plain number box. Integer parameters are excluded too, since a metric distance almost always converts to a fractional number of degrees.
The name rule (
*_dist,*_radius,spacing,tolerance,*_length,resolution,cell_size, and whole-namewidth/height) lives inwhitebox-distance-params.tsalongside the conversion, so there is no per-tool table to keep in sync with the catalog. Counts, angles, penalties, speeds and times are deliberately left out.Verification
Driven in the real app with Playwright against
r15_osm_roads.geojson(927 OSM roads near Eugene, Oregon, WGS84), using Points Along Lines, the tool named in the issue:200with Meters selected sends0.0021193307to the tool, converted at 44.0 deg N, and the run succeeds (1171 points added to the map).200when switched to Meters again, so the round trip preserves the distance.tests/whitebox-distance-params.test.tscovers the name rule (real parameter names from the catalog on both sides), the latitude scale including the pole clamp, unit round-trips, and the decimal formatting that keeps small degree values out of exponent notation.Full frontend suite (4421 tests) and
npm run buildpass; pre-commit is clean.Summary by CodeRabbit