Closes#47. Phase 0 of #58, landing first and alone as planned.
Three compounding defects in the measurement layer, all measured against the live Home garden rather than estimated.
1. Entry was quantized to whole centimeters
Every measurement column is REAL and the server's validDimensionCM checks range and finiteness only — the rounding was a client-side choice the schema never asked for. A 12 in grid stored 30 cm, i.e. an 11.811″ grid: every cell short by 0.189″, and because the error is systematic it accumulates to 4.61″ across the 24 ft garden.
cmFromFtIn / cmFromMeters / cmFromSpacing now convert exactly. They round at 1e-6 cm — enough to clear IEEE-754 dust (12 * 2.54 is 30.479999999999997) and ~4 orders of magnitude below anything a garden cares about. Display-side rounding is untouched, so cm quantization still can't show through in a field.
2. The garden grid was entered at spacing scale
Grid size (in) sat directly beneath Width (ft) — two adjacent numeric fields, two different scales, distinguished only by a suffix. Home ended up with gridSizeCm: 3, almost certainly "1" typed meaning one foot.
The garden grid is a layout concern, so it moves to dimension scale and reads Garden grid (ft). The bed grid in the object inspector is a plant-spacing concern and stays at in/cm — the labels are now unambiguous rather than uniform, which is the honest resolution given they really are different scales.
A sub-10 cm garden grid draws a soft warning (That's a 0′ 1″ grid for the whole garden — did you mean ft?) rather than being silently accepted. Soft on purpose: a fine grid is legitimate for a tiny garden.
3. Snap stayed active when the grid stopped drawing
The canvas faded the grid below 6 px per cell. At 3 cm and ~0.84 px/cm that's 2.5 px → opacity 0, so Home ran with snapToGrid: true, no grid drawn, and no snapping you could feel (its bed coordinates are values like 80.90185676392574).
Replaced the fade with coarsening, the issue's preferred option. visibleGridStepCm picks the smallest multiple of the true grid (2×, 5×, 10×…) whose cells clear the legibility floor, so every line drawn is still a real grid line and something aligned to the grid is always visible. Snapping continues to use the true grid — the drawn lines are a subset of the snap positions, never a different grid. When the drawn step isn't the true grid the canvas says grid every 5′ 0″; a degenerate grid it can't draw at any multiple says that rather than silently drawing nothing.
Verification
npm test — 39 passing, including exact-conversion cases, a display → cm → display identity table for both unit systems, and coarsening cases covering the exact 3 cm @ 0.84 px/cm failure.
go test ./..., tsc --noEmit, npm run build all green.
Existing gardens are not migrated (out of scope, per the issue). Home's 3 stays 3 and now displays honestly as 0.1 ft, with the warning explaining it.
Closes #47. Phase 0 of #58, landing first and alone as planned.
Three compounding defects in the measurement layer, all measured against the live Home garden rather than estimated.
### 1. Entry was quantized to whole centimeters
Every measurement column is `REAL` and the server's `validDimensionCM` checks range and finiteness only — the rounding was a client-side choice the schema never asked for. A **12 in** grid stored **30 cm**, i.e. an 11.811″ grid: every cell short by 0.189″, and because the error is systematic it accumulates to **4.61″ across the 24 ft garden**.
`cmFromFtIn` / `cmFromMeters` / `cmFromSpacing` now convert exactly. They round at 1e-6 cm — enough to clear IEEE-754 dust (`12 * 2.54` is `30.479999999999997`) and ~4 orders of magnitude below anything a garden cares about. **Display-side rounding is untouched**, so cm quantization still can't show through in a field.
### 2. The garden grid was entered at spacing scale
`Grid size (in)` sat directly beneath `Width (ft)` — two adjacent numeric fields, two different scales, distinguished only by a suffix. Home ended up with `gridSizeCm: 3`, almost certainly "1" typed meaning one foot.
The garden grid is a *layout* concern, so it moves to dimension scale and reads **`Garden grid (ft)`**. The bed grid in the object inspector is a *plant-spacing* concern and stays at `in`/`cm` — the labels are now unambiguous rather than uniform, which is the honest resolution given they really are different scales.
A sub-10 cm garden grid draws a soft warning (`That's a 0′ 1″ grid for the whole garden — did you mean ft?`) rather than being silently accepted. Soft on purpose: a fine grid is legitimate for a tiny garden.
### 3. Snap stayed active when the grid stopped drawing
The canvas faded the grid below 6 px per cell. At 3 cm and ~0.84 px/cm that's 2.5 px → opacity 0, so Home ran with `snapToGrid: true`, no grid drawn, and no snapping you could feel (its bed coordinates are values like `80.90185676392574`).
Replaced the fade with **coarsening**, the issue's preferred option. `visibleGridStepCm` picks the smallest multiple of the true grid (2×, 5×, 10×…) whose cells clear the legibility floor, so every line drawn is still a real grid line and something aligned to the grid is always visible. Snapping continues to use the true grid — the drawn lines are a subset of the snap positions, never a different grid. When the drawn step isn't the true grid the canvas says `grid every 5′ 0″`; a degenerate grid it can't draw at any multiple says *that* rather than silently drawing nothing.
### Verification
- `npm test` — 39 passing, including exact-conversion cases, a `display → cm → display` identity table for both unit systems, and coarsening cases covering the exact 3 cm @ 0.84 px/cm failure.
- `go test ./...`, `tsc --noEmit`, `npm run build` all green.
- Existing gardens are not migrated (out of scope, per the issue). Home's `3` stays `3` and now displays honestly as `0.1 ft`, with the warning explaining it.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Three compounding defects in the measurement layer (#47), measured against the
live Home garden rather than estimated.
Entry was quantized to whole centimeters even though every measurement column is
REAL and the server only range-checks. A 12in grid stored 30cm — an 11.811in
grid, short by 0.189in per cell, and because the error is systematic it
accumulates: 4.61in of drift across a 24ft garden. cmFromFtIn / cmFromMeters /
cmFromSpacing now convert exactly, rounding only at 1e-6 cm to clear IEEE-754
dust (12 x 2.54 is 30.479999999999997 in binary). Display-side rounding is
untouched, so cm quantization still can't show through in a field.
The garden grid was entered at spacing scale: "Grid size (in)" sat directly under
"Width (ft)", two adjacent numeric fields at two different scales distinguished
only by a suffix. Home ended up on a 3cm grid, almost certainly "1" typed meaning
one foot. The garden grid is a layout concern, so it moves to dimension scale and
reads "Garden grid (ft)"; the bed grid in the object inspector is a plant-spacing
concern and deliberately stays at cm/in. A sub-10cm garden grid now draws a soft
warning rather than being silently accepted.
Snap could stay active while the grid stopped drawing: the canvas faded the grid
out below 6px per cell, so Home ran with snapToGrid on, no grid drawn, and no
snapping you could feel. Replaced the fade with coarsening — visibleGridStepCm
picks the smallest multiple of the true grid (2x, 5x, 10x...) whose cells are
legible, so every line drawn is still a real grid line and something aligned to
the grid is always visible. When the drawn step isn't the true grid the canvas
says so; a degenerate grid it can't draw at all says that instead of nothing.
Closes#47
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-21 04:44:18Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡cmFromDisplay(parseFloat(gridSize), unit) duplicated between warning calc and submit handler
maintainability · flagged by 1 model
web/src/editor/GardenCanvas.tsx:303 & :307 — the exact same badge className string (rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur) is duplicated across the two <span>s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g. const badgeClass = '...') would keep the two badges in sync if the styling ever changes. Trivial. - web/src/editor/GardenCanvas.tsx:118 — gridNote is a three-level nested ter…
🪰 Gadfly · advisory
🟡 **cmFromDisplay(parseFloat(gridSize), unit) duplicated between warning calc and submit handler**
_maintainability · flagged by 1 model_
- `web/src/editor/GardenCanvas.tsx:303` & `:307` — the exact same badge className string (`rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur`) is duplicated across the two `<span>`s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g. `const badgeClass = '...'`) would keep the two badges in sync if the styling ever changes. Trivial. - `web/src/editor/GardenCanvas.tsx:118` — `gridNote` is a three-level nested ter…
<sub>🪰 Gadfly · advisory</sub>
🟠Soft-warning 'did you mean {unitLabel}?' is self-contradictory now that the grid field is already in that unit; the '1 typed meaning one foot' comments are stale under the new dimension scale.
maintainability · flagged by 1 model
web/src/components/gardens/GardenFormModal.tsx:204 — The soft-warning did you mean {unitLabel}? is self-contradictory after the scale migration. The grid field is now labeled Garden grid (ft) / (m) (i.e. unitLabel), so when the warning fires it asks "did you mean ft?" while the user is already typing in a ft field. The warning only made sense when the grid was at spacing scale (in/cm) and the suggestion was to switch to the dimension unit. Under the new dimension scale, the "typed '1…
🪰 Gadfly · advisory
🟠 **Soft-warning 'did you mean {unitLabel}?' is self-contradictory now that the grid field is already in that unit; the '1 typed meaning one foot' comments are stale under the new dimension scale.**
_maintainability · flagged by 1 model_
- `web/src/components/gardens/GardenFormModal.tsx:204` — The soft-warning `did you mean {unitLabel}?` is self-contradictory after the scale migration. The grid field is now labeled `Garden grid (ft)` / `(m)` (i.e. `unitLabel`), so when the warning fires it asks "did you mean ft?" while the user is already typing in a ft field. The warning only made sense when the grid was at spacing scale (in/cm) and the suggestion was to switch to the dimension unit. Under the new dimension scale, the "typed '1…
<sub>🪰 Gadfly · advisory</sub>
🟡3-level nested ternary for gridNote is harder to scan/modify than an if/else or small helper
maintainability · flagged by 2 models
web/src/editor/GardenCanvas.tsx:118-125 — gridNote is a 3-level nested ternary (drawnGridCm == null → garden.snapToGrid → drawnGridCm !== gridCm) picking between three string/null outcomes. It's well-commented, but nested ternaries like this are harder to scan and riskier to extend than an early-return helper or plain if/else. Suggest pulling it into a small named function, e.g. gridNoteFor(drawnGridCm, gridCm, snapToGrid, unit), same logic with explicit branches.
🪰 Gadfly · advisory
🟡 **3-level nested ternary for gridNote is harder to scan/modify than an if/else or small helper**
_maintainability · flagged by 2 models_
- **`web/src/editor/GardenCanvas.tsx:118-125`** — `gridNote` is a 3-level nested ternary (`drawnGridCm == null` → `garden.snapToGrid` → `drawnGridCm !== gridCm`) picking between three string/null outcomes. It's well-commented, but nested ternaries like this are harder to scan and riskier to extend than an early-return helper or plain if/else. Suggest pulling it into a small named function, e.g. `gridNoteFor(drawnGridCm, gridCm, snapToGrid, unit)`, same logic with explicit branches.
<sub>🪰 Gadfly · advisory</sub>
⚪Duplicated badge className string across two overlay spans
maintainability · flagged by 1 model
web/src/editor/GardenCanvas.tsx:303 & :307 — the exact same badge className string (rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur) is duplicated across the two <span>s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g. const badgeClass = '...') would keep the two badges in sync if the styling ever changes. Trivial. - web/src/editor/GardenCanvas.tsx:118 — gridNote is a three-level nested ter…
🪰 Gadfly · advisory
⚪ **Duplicated badge className string across two overlay spans**
_maintainability · flagged by 1 model_
- `web/src/editor/GardenCanvas.tsx:303` & `:307` — the exact same badge className string (`rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur`) is duplicated across the two `<span>`s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g. `const badgeClass = '...'`) would keep the two badges in sync if the styling ever changes. Trivial. - `web/src/editor/GardenCanvas.tsx:118` — `gridNote` is a three-level nested ter…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Minor issues · 5 findings (1 with multi-model agreement)
Finding
Where
Models
Lens
🟡
3-level nested ternary for gridNote is harder to scan/modify than an if/else or small helper
web/src/editor/GardenCanvas.tsx:118
2/5
maintainability
4 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Soft-warning 'did you mean {unitLabel}?' is self-contradictory now that the grid field is already in that unit; the '1 typed meaning one foot' comments are stale under the new dimension scale.
Reviewed through the security lens (injection, XSS, SSRF, deserialization, secrets, input validation, authz).
This PR is confined to client-side arithmetic and display logic for unit conversion (units.ts, geometry.ts) and two React components that render the results. I verified:
No injection sinks: All new/changed code paths (cmFromFtIn, cmFromMeters, cmFromSpacing, cmFromDisplay, roundCm, visibleGridStepCm, formatCm) are pure numeric functions — no string concatenation into SQL, shell, template, or dangerouslySetInnerHTML.
No XSS: formatCm(...) output is rendered as plain JSX text content (GardenFormModal.tsx:204, GardenCanvas.tsx:305), which React escapes automatically. The SVG pattern id (gridId, GardenCanvas.tsx:69) is unchanged by this diff and derived from React's useId(), not user input.
Input validation preserved: onSubmit still runs isValidDimensionCm against the converted widthCm/heightCm/gridSizeCm before submitting (GardenFormModal.tsx:83-93), matching the server's documented [1cm, 100m] bounds (units.ts:11). The new MIN_GARDEN_GRID_CM check is a soft UI warning only (GardenFormModal.tsx:136,202-206) and doesn't weaken that hard validation — it's additive.
No new untrusted-data handling: all values flowing through the changed functions originate from the same trusted client-side form state as before; nothing here reads from the network, deserializes payloads, or handles secrets/credentials.
The roundCm precision fix and the grid/dimension-scale rework don't touch authn/authz, don't introduce new external calls, and don't loosen any existing validation.
🎯 Correctness — Minor issues
Review
VERDICT: Minor issues
I independently re-derived the conversion math and re-read web/src/lib/units.ts and web/src/lib/geometry.ts against the diff.
web/src/lib/units.ts:25-28 — the roundCm doc comment claims "1 nanometer is far below anything a garden cares about," but the actual rounding grain is 1e-6 cm, which is 10 nanometers, not 1 (1 cm = 1e7 nm, so 1e-6 cm = 1e-6 × 1e7 nm = 10 nm). Confirmed: roundCm is defined exactly as Math.round(cm * 1e6) / 1e6 (units.ts:29-31), and the "1 nanometer" phrasing sits on line 26 of that same doc comment. This is a comment-only inaccuracy — the rounding constant itself (1e-6 cm, i.e. sub-micron) is still functionally fine for garden-scale dimensions — with no behavioral effect on any of the conversion functions.
🧹 Code cleanliness & maintainability — Minor issues
Confirmed — the "no dead code" claim and unitPref field both check out. The draft's single finding is legitimate and verifiable in the source. Here's the corrected review:
VERDICT: Minor issues
Lens: 🧹 Code cleanliness & maintainability
web/src/editor/GardenCanvas.tsx:118-125 — gridNote is a 3-level nested ternary (drawnGridCm == null → garden.snapToGrid → drawnGridCm !== gridCm) picking between three string/null outcomes. It's well-commented, but nested ternaries like this are harder to scan and riskier to extend than an early-return helper or plain if/else. Suggest pulling it into a small named function, e.g. gridNoteFor(drawnGridCm, gridCm, snapToGrid, unit), same logic with explicit branches.
Everything else checked out clean:
roundCm (web/src/lib/units.ts:29) is a tightly-scoped private helper with a comment explaining why (IEEE-754 dust vs. real precision loss).
No dead code: spacingFromCm/spacingUnitLabel/cmFromSpacing/formatSpacing were dropped from GardenFormModal.tsx's imports but remain live — confirmed via grep they're still used by Inspector.tsx, PlopInspector.tsx, PlantFormModal.tsx, PlantCard.tsx, and PlantPicker.tsx for the (intentionally separate) bed-grid/plant-spacing scale.
GRID_MAX_OPACITY → GRID_OPACITY rename in GardenCanvas.tsx correctly reflects it's no longer a max (fading was replaced by coarsening).
The convertGrid/convert consolidation in GardenFormModal.tsx:57-68 actually removes prior duplication rather than adding any, now that the grid uses the same dimension-scale helper as width/height.
visibleGridStepCm (web/src/lib/geometry.ts:117-128) is compact, well-commented, and has matching test coverage in geometry.test.ts.
No other maintainability issues in this lens. Outside my lens: correctness/test coverage looked solid on read-through, but that's a different reviewer's job.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls).
visibleGridStepCm (web/src/lib/geometry.ts:117) runs on every GardenCanvas render — including the high-frequency re-renders during a live object/plop drag, per the comment at GardenCanvas.tsx:201. It's an unmemoized nested loop, but bounded to at most 6 × 3 = 18 iterations of trivial multiply/compare — negligible next to the SVG re-render work already happening on the same path (verified by reading the full function; no unbounded loop, no allocation inside it).
bedGridLines / BED_GRID_MAX_LINES cap is unchanged by this diff (GardenCanvas.tsx:27,38-43) — the new grid-coarsening logic doesn't touch the per-bed line generation, so the existing 200-line safety cap still applies there.
The new gridEntered/gridTooFine computation in GardenFormModal.tsx:135-136 re-parses gridSize on every render, but it's a single parseFloat + arithmetic on a form with a handful of fields — not a hot path.
roundCm (units.ts:29-31) adds one Math.round per conversion call; conversions happen only on user input/unit-toggle events, not in any loop or render path.
No N+1s, no missing pagination, no quadratic behavior, no new blocking calls — this PR is UI/unit-conversion logic with no data-fetching or bulk-processing surface. Nothing here rises to a performance finding.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for error handling and edge cases: NaN/invalid input propagation, boundary conditions in the new unit-conversion math, and the new grid-coarsening logic's degenerate cases.
What I checked (verified by reading the actual code, not just the diff):
roundCm/cmFromFtIn/cmFromMeters/cmFromSpacing (web/src/lib/units.ts:29-98): NaN/negative/zero inputs flow through untouched and are caught downstream by isValidDimensionCm's Number.isFinite + range check before ever reaching the server payload. Confirmed isValidDimensionCm is applied to gridSizeCm at submit (GardenFormModal.tsx:89-93), not just width/height.
gridTooFine warning (GardenFormModal.tsx:135-136): guards Number.isFinite(gridEntered) && gridEntered > 0 before comparing to MIN_GARDEN_GRID_CM, so an empty/cleared field (parseFloat('') → NaN) or a zero/negative legacy value correctly suppresses the warning rather than rendering "NaN′ NaN″" — and zero/negative are still hard-blocked by the submit-time isValidDimensionCm check either way, so nothing slips through silently.
visibleGridStepCm (web/src/lib/geometry.ts:117-128): guards gridCm > 0, scale > 0, minCellPx > 0 up front, so corrupt/legacy grid values of 0 or negative (exactly the kind of bad data the PR calls out for Home) return null rather than dividing or looping unboundedly. Loop is bounded (6 decades × 3 multiples = 18 iterations max), so there's no runaway/infinite loop risk. Boundary case (step * scale >= minCellPx, using >= not >) matches the "exactly at the minimum" test case with no off-by-one.
Caller side in GardenCanvas.tsx:118-125,221: drawnGridCm == null is handled explicitly (skips rendering the pattern, shows an explanatory note only when snapToGrid is on) — no crash path when the grid can't be drawn at all, unlike the old fade logic that silently produced 0 opacity with snap still active.
Confirmed EditorGarden.unitPref is a required (non-optional) field in web/src/editor/types.ts:35, so formatCm(drawnGridCm, garden.unitPref) isn't reading from a possibly-undefined value.
Nothing in this diff introduces an unguarded panic, a swallowed error, or a boundary condition that isn't already defended against by an existing check (isValidDimensionCm, Number.isFinite, or the new visibleGridStepCm guards). The validation/warning logic is consistently soft-fail (warn/block via form error) rather than throwing.
The security lens here covers client-side form validation and unit conversion — no auth/authz, injection, or untrusted-deserialization surface in this diff. The form still submits to the same server-side validDimensionCM range check (internal/service/gardens.go:256), which guards NaN/Inf and bounds. The client-side changes are pure numeric conversion/display logic with no taint flow into queries, commands, or paths.
I traced the tainted input path: parseFloat(gridSize) → cmFromDisplay → input.gridSizeCm → create.mutateAsync/update.mutateAsync → server validDimensionCM. No bypass introduced; the server remains the authority. The roundCm helper operates only on already-numeric values; no string templating into SQL/HTML. The formatCm(gridEntered, unit) interpolated into JSX is a number→string formatter, not raw HTML injection (React escapes). Nothing material in my lane.
Verdict: No material issues found
Verified the server-side validation (internal/service/gardens.go:252-258) still gates GridSizeCM via validDimensionCM (finite + [1cm, 100m]) regardless of the client's new exact-conversion logic — no validation bypass.
Verified roundCm/cmFromDisplay/cmFromFtIn/cmFromSpacing are pure numeric transforms over parseFloat output; no string interpolation into queries, commands, paths, or templates.
Verified formatCm(gridEntered, unit) in GardenFormModal.tsx:204 and formatCm(drawnGridCm, ...) in GardenCanvas.tsx:124 render through JSX (auto-escaped), not dangerouslySetInnerHTML — no XSS vector.
No auth/authz, secret, SSRF, or deserialization changes in this diff.
🎯 Correctness — No material issues found
Verdict: No material issues found.
I verified the change through the correctness lens — re-deriving the unit conversions, the coarsening math, and the round-trip identities against the checked-out code.
Exact conversions (web/src/lib/units.ts:29-41, 97-99): roundCm = Math.round(cm*1e6)/1e6. Re-derived: 12 * 2.54 = 30.48 (the IEEE dust 30.479999999999997 rounds back to 30.48); 1 ft → 30.48; 4'6" → 137.16; 0.305 m → 30.5 cm. All match the tests and first principles. The 1e-6 cm floor is ~1 nm — far below garden precision and below the display rounding (0.1 ft ≈ 3 cm, 0.01 m = 1 cm), so it cannot show through in a field.
Round-trip identity (displayFromCm ∘ cmFromDisplay): For imperial, displayFromCm = round(roundCm(ft*30.48)/30.48 * 10)/10. For the tested ft values (0.5, 1, 2.5, 4, 8, 12.5, 24) this returns ft exactly. For metric, round(roundCm(m*100))/100 returns m exactly for the tested set. Confirmed by reading units.ts:44-57.
Scale switch for the garden grid (GardenFormModal.tsx:49, 89, 122): grid now uses cmFromDisplay/displayFromCm (dimension scale, ft/m) instead of cmFromSpacing/spacingFromCm (cm/in). The bed grid in Inspector.tsx still uses the spacing helpers (Inspector.tsx:54,71,98-100), correctly untouched. No cross-file caller of the removed cmFromSpacing/spacingUnitLabel imports in GardenFormModal remains (grep confirms the form no longer references them).
Coarsening (geometry.ts:117-128): visibleGridStepCm returns the smallest gridCm * decade * m (m ∈ {1,2,5}) with step*scale >= minCellPx. Re-derived the test cases: 3 @ 0.84 → 15 (2.52, 5.04 < 6; 12.6 ≥ 6), 1 @ 0.84 → 10 (0.84…4.2 < 6; 8.4 ≥ 6), 10 @ 0.6 → 10 (exactly 6). Decade cap: d=0..5 → decade ∈ {1,10,…,1e5}, max step = 5e5·gridCm, matching the "500,000×" comment. Every returned step is a whole multiple of gridCm, so drawn lines are a subset of snap positions — confirmed snapping still uses gridCm at GardenCanvas.tsx:155, not drawnGridCm.
Soft warning (GardenFormModal.tsx:133-136): MIN_GARDEN_GRID_CM = 10 cm = 3.937″ ≈ the "sub-4″" comment. gridEntered is NaN-safe (Number.isFinite guard). Re-derived Home's case: displayFromCm(3, 'imperial') = round(0.9843)/10 = 0.1 ft (field shows 0.1), and formatCm(3, 'imperial') = "0′ 1″" (warning text) — both match the PR description. Typing "1" in the new ft field → 30.48 cm, correctly not warned.
No logic bugs, incorrect constants, or semantic errors found within the correctness lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues found
Reviewed through the cleanliness/maintainability lens. The change is well-structured overall: the spacing helpers were correctly left in place for the bed-grid/plant-spacing callers, the convertGrid special-case was collapsed into the shared convert once the grid moved to dimension scale, and visibleGridStepCm is a clean, self-contained extraction with a good doc comment. A few small nits:
web/src/editor/GardenCanvas.tsx:303 & :307 — the exact same badge className string (rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur) is duplicated across the two <span>s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g. const badgeClass = '...') would keep the two badges in sync if the styling ever changes. Trivial.
web/src/editor/GardenCanvas.tsx:118 — gridNote is a three-level nested ternary (drawnGridCm == null ? … : drawnGridCm !== gridCm ? … : null). It's readable as written and matches the "two states worth explaining" comment, but a small if/early-return helper or a lookup would flatten it. Not worth churning on its own. Trivial.
web/src/components/gardens/GardenFormModal.tsx:135 — cmFromDisplay(parseFloat(gridSize), unit) is computed inline on every render for the warning, and the same expression reappears in the submit handler (:89). The two are in different scopes so extracting isn't strictly necessary, but a shared const gridEnteredCm derived once would remove the duplication. Small.
No dead code, no leaky abstractions, no patterns broken relative to the surrounding files. The roundCm helper is a nice single-source replacement for the scattered Math.round calls. Nothing here is blocking.
⚡ Performance — No material issues found
No material issues found.
Performance lens: the change replaces a fade-opacity computation with visibleGridStepCm, both O(1) per render — the latter does at most ~18 iterations of trivial arithmetic (3 multipliers × 6 decades), strictly cheaper than nothing material. The grid pattern/rect is only re-rendered when drawnGridCm is non-null, same gating as before. The new gridTooFine check in the form is a couple of parseFloats on keystroke (form modal, not hot path). The formatCm calls on gridNote only fire when the note is shown. No N+1, no unbounded growth, no new per-frame allocation (the useMemos are unchanged). GRID_MAX_COARSEN_DECADES = 5 gives a 500,000× cap that bounds the loop unconditionally. Nothing here is a regression.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling / edge-case lens, the change is careful. Verified by reading the actual checked-out code:
visibleGridStepCm (geometry.ts:117-128) guards all degenerate inputs with !(x > 0), which correctly rejects NaN, 0, and negatives for gridCm, scale, and minCellPx (since NaN > 0 is false, !(false) → returns null). The coarsening loop is bounded (d <= GRID_MAX_COARSEN_DECADES), so no unbounded iteration, and the null result is surfaced by the caller as an explicit "grid too fine to draw — zoom in" note rather than swallowed. The degenerate cases are even unit-tested (geometry.test.ts).
GardenFormModal warning path (GardenFormModal.tsx:135-136): gridEntered = cmFromDisplay(parseFloat(gridSize), unit). parseFloat can return NaN (empty/garbage input) or a negative; gridTooFine guards with Number.isFinite(gridEntered) && gridEntered > 0 && gridEntered < MIN_GARDEN_GRID_CM, so NaN/negative/zero don't produce a spurious warning. The 0 and NaN cases are instead caught at submit by isValidDimensionCm (which checks Number.isFinite and range) with a clear form error — not swallowed.
Submit path (GardenFormModal.tsx:83-93): width/height/grid all run through cmFromDisplay → isValidDimensionCm, which rejects non-finite and out-of-range values with explicit messages; no unchecked parseFloat flows into the mutation.
Canvas render guard (GardenCanvas.tsx:221) switched from an opacity threshold to drawnGridCm != null, consistent with visibleGridStepCm's null contract — a null (degenerate grid) correctly skips the <pattern>/<rect> render rather than rendering with width={null}/NaN.
The roundCm helper (units.ts:29-31) operates on finite products of well-bounded inputs; no overflow concern at the magnitudes involved (max ~10000 cm).
No unhappy-path regressions introduced. The Infinity input case for visibleGridStepCm is theoretically unguarded, but gridCm is server-bounded to [1, 10000] and viewport.scale is a real zoom factor, so it is not reachable here — not a material finding.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The PR is a units-correctness refactor with no security-relevant changes. All user inputs (grid size, width, height) continue to be:
parsed with parseFloat (same as before),
converted through well-bounded helper functions,
validated client-side with isValidDimensionCm (range 1–10,000 cm, finiteness check),
and re-validated server-side with validDimensionCM (identical range and math.IsNaN/math.IsInf guards).
The new soft warning for sub-10 cm garden grids renders via React JSX with formatCm, which returns plain numeric strings—no HTML injection path. No secrets, credentials, SQL, commands, paths, or SSRF surfaces are touched.
🎯 Correctness — No material issues found
No material issues found
I scrutinized the change through the Correctness lens and found the mathematics sound:
roundCm correctly clears IEEE-754 dust at 1e-6 cm (~10 nm) without clipping garden-meaningful precision. cmFromFtIn(0, 12) → 30.48, cmFromMeters(0.305) → 30.5, etc. all verified.
Exact conversion for cmFromFtIn, cmFromMeters, and cmFromSpacing is correct: the server stores REAL, so rounding to whole cm was a client-side bug. Display-side rounding in displayFromCm/spacingFromCm remains unchanged, preserving clean UI round-trips.
visibleGridStepCm coarsening logic verified: it returns the smallest 1×/2×/5×/10×… multiple of the true grid that clears minCellPx, and null only when even 500,000× falls short. Snapping continues to use the true grid, so drawn lines are always a subset of snap positions — never a different grid.
Garden form correctly moves the garden grid to dimension scale (ft/m) while keeping bed/plant spacing at cm/in. The MIN_GARDEN_GRID_CM = 10 soft floor warns without refusing, which matches the issue description.
No regressions in existing callers: PlantFormModal, Inspector, and PlopInspector still use cmFromSpacing for plant spacing; only GardenFormModal switched to cmFromDisplay for the garden grid.
Tests cover the exact-conversion identity table, the #47 coarsening case (3 cm @ 0.84 px/cm → 15 cm), and degenerate-grid null behavior. No logic bugs detected.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
I reviewed all six changed files through the maintainability lens:
units.ts: The new roundCm helper consolidates IEEE-754 dust rounding into one place instead of repeating it across cmFromFtIn, cmFromMeters, and cmFromSpacing. The MIN_GARDEN_GRID_CM constant is well-commented and clearly named.
geometry.ts: visibleGridStepCm is a clean, focused function with a clear docstring. The coarsening constants are module-local and self-documenting.
GardenFormModal.tsx: Removing the separate convertGrid closure and reusing the existing convert function reduces duplication and makes the unit-switching path simpler. The warning markup is a straightforward conditional block.
GardenCanvas.tsx: Replacing the opacity math with a semantic drawnGridCm != null check is clearer. The gridNote ternary, while nested, is small and directly follows the two states the function returns.
Test files: Old whole-cm assertions were updated to exact values, and new tests cover the coarsening and round-trip behavior with descriptive names.
No dead code, no confusing names, no overly long functions, no copy-paste that should be shared, and no leaky abstractions.
⚡ Performance — No material issues found
No material issues found
From a performance lens, the changes are clean:
visibleGridStepCm (geometry.ts:117) is a bounded loop (≤15 iterations of simple arithmetic) called once per GardenCanvas render. It replaces continuous opacity interpolation with discrete step changes, which is neutral-to-positive for rendering cost during zoom.
The SVG <pattern> is only recreated when drawnGridCm actually changes; between threshold crossings React reconciles it away. No excess DOM churn.
formatCm and cmFromDisplay calls in GardenFormModal are form-field-only, not hot-path.
No new unmemoized arrays/objects being passed as props, no added subscribers or resize observers, no layout-thrashing patterns introduced.
🧯 Error handling & edge cases — No material issues found
No material issues found
Through the 🧯 error-handling & edge-cases lens, the diff is clean. I verified the following:
visibleGridStepCm (geometry.ts:117) guards against NaN, 0, negative, and non-finite inputs with !(gridCm > 0) || !(scale > 0) || !(minCellPx > 0) and returns null for all degenerate cases. The loop is capped at 18 iterations, so there is no runaway or overflow risk.
GardenFormModal (GardenFormModal.tsx:135-136) correctly guards the soft-warning check with Number.isFinite(gridEntered) && gridEntered > 0, so empty/invalid/malformed input, zero, and negative values do not spuriously trigger the “did you mean ft?” message. The downstream isValidDimensionCm call still catches invalid values at submit time.
roundCm (units.ts:29-31) safely handles NaN and Infinity by passing them through; the only “gotcha” is sub-nanometer values round to 0, but those are rejected by isValidDimensionCm anyway.
gridNote logic in GardenCanvas (GardenCanvas.tsx:118-125) distinguishes the three states (true grid, coarsened grid, degenerate grid) without crashing on null from visibleGridStepCm.
The test suite explicitly covers the 3 cm @ 0.84 px/cm failure case, whole-multiple invariant, and degenerate/null returns.
No ignored errors, missing cleanup, panics, or unhandled edge cases are introduced by this change.
Reviewed through the security lens: input validation, injection, taint tracing, and defaults.
Input validation intact.GardenFormModal.tsx:89-93 still runs parseFloat → cmFromDisplay → isValidDimensionCm, which checks Number.isFinite(cm) && cm >= 1 && cm <= 10_000 (units.ts:21-22). This mirrors the server's validDimensionCM in internal/service/gardens.go:274-275 (finite + [minGardenCM, maxGardenCM]). NaN from non-numeric input (parseFloat("abc") → NaN → cmFromFtIn(NaN) → roundCm(NaN) → NaN) is rejected by the finite check. Negative values rejected by the >= 1 floor. No path where a bad value bypasses both client and server checks.
No injection surface. The only user-controlled text interpolated into the DOM is the gridTooFine warning (GardenFormModal.tsx:202-206) and the canvas gridNote (GardenCanvas.tsx). Both feed into React JSX as text children ({formatCm(...)}, {gridNote}, {unitLabel}), which React escapes; no dangerouslySetInnerHTML. unitLabel is a constant ('ft'/'m') from dimensionUnitLabel, not user input. formatCm/gridNote are built from numeric conversions of user input, not raw strings.
No SSRF / command / SQL / deserialization / secret paths touched by this diff.
Server-side trust boundary unchanged. The Go gardenFromInput (gardens.go:214-268) still re-validates GridSizeCM via validDimensionCM and UnitPref against the metric/imperial allowlist (gardens.go:244-245); the client's new exact-cm conversion doesn't widen what the server accepts (REAL column already accepted fractional cm — the PR description's point).
Verification: read web/src/lib/units.ts (full), web/src/components/gardens/GardenFormModal.tsx (full), internal/service/gardens.go:230-276, web/src/editor/Inspector.tsx:1-101, and grepped for validDimensionCM, dangerouslySetInnerHTML/innerHTML/eval in the editor, and UnitMetric/UnitImperial definitions. No gaps found in the security lane.
🎯 Correctness — No material issues found
I'll review this through the correctness lens, verifying against the actual code.
Now let me check the conversion in changeUnit to verify whether convert handles the case where the existing width is non-finite.
Let me verify the round-trip identity table and check the cmFromMeters(1.22) case.
Let me re-derive the round-trip claims from the test:
Metric round-trip: m → cm → m via cmFromMeters(m) then displayFromCm(cm, 'metric').
Now let me verify the visibleGridStepCm cap and the degenerate case:
For visibleGridStepCm(1e-9, 1, 6): need step * 1 >= 6, i.e. gridCm * decade * m >= 6, so 1e-9 * decade * m >= 6. Max decade is 1e5 (5 iterations of *= 10), max m=5, so 1e-9 * 1e5 * 5 = 5e-4 — way below 6. Returns null ✓.
For visibleGridStepCm(1, 0.84, 6): 1*0.84=0.84 <6; 2*0.84=1.68 <6; 5*0.84=4.2 <6; next decade 10*0.84=8.4 >=6 → returns 10 ✓.
Now let me verify the concern about the gridTooFine warning. The check is gridEntered > 0 && gridEntered < MIN_GARDEN_GRID_CM (10 cm). But the warning text interpolates formatCm(gridEntered, unit). Let me check the warning text for the actual problem case described in the PR: a 1 ft entry would be 30.48 cm, which is NOT < 10 cm. So the warning only fires for sub-10cm entries. Let me check the PR text claims and the comment.
I have enough to render the verdict. Let me check one thing — whether the round-trip assertion could fail at boundary cases due to displayFromCm rounding at the edge. The test uses [0.5, 1, 2.5, 4, 8, 12.5, 24]. Let me check cmFromFtIn(4) = roundCm(4*12*2.54) = roundCm(121.92) = 121.92; displayFromCm(121.92, 'imperial') = Math.round((121.92/30.48)*10)/10 = Math.round(4*10)/10 = 4 ✓. cmFromFtIn(8) = 243.84; display = Math.round(8*10)/10 = 8 ✓. cmFromFtIn(24) = roundCm(24*12*2.54) = roundCm(731.52) = 731.52; display = Math.round(24*10)/10 = 24 ✓.
All checks pass. I verified the changes against the actual code — the round-trip identity, the conversion exactness, the visibleGridStepCm coarsening and null cases, and the snap-uses-true-grid invariant.
Review
Verdict: No material issues found
I verified the correctness of all three changes against the actual checked-out code:
Exact conversions (web/src/lib/units.ts:29-99): roundCm = Math.round(x*1e6)/1e6 correctly clears IEEE-754 dust (re-derived: 12 * 2.54 = 30.479999999999997 → Math.round(30479999.99999997)/1e6 = 30.48 ✓). The round-trip identity table (units.test.ts:38-46) holds for the asserted values: I traced each imperial value (0.5, 1, 2.5, 4, 8, 12.5, 24) and metric value (0.15, 1, 1.22, 3.05, 10, 24.5) through cmFromDisplay → displayFromCm and every one round-trips exactly within the display precision (0.1 ft / 0.01 m). The 1e-6 cm granularity is ~4 orders below garden precision, as claimed.
Garden grid at dimension scale (GardenFormModal.tsx:48-67, 89, 122, 135): The grid now uses cmFromDisplay/displayFromCm consistently — entry, conversion, conflict-rebase, and save all use the same helpers as width/height. The convert closure in changeUnit is unit-preserved correctly (closes over unit, the current unit, converting to next). The soft-warning floor MIN_GARDEN_GRID_CM = 10 cm ≈ 4″ matches the comment "sub-4″"; gridTooFine requires > 0 and < 10, so a legitimate 1 ft grid (30.48 cm) correctly does NOT trigger the warning, only true mis-entries do.
Coarsening instead of fade (geometry.ts:117-128, GardenCanvas.tsx:114-125, 221-225): I re-derived the key cases: visibleGridStepCm(3, 0.84, 6) → tries 3*0.84=2.52, 6*0.84=5.04, 15*0.84=12.6 ≥ 6 → returns 15 ✓. The drawn lines (drawnGridCm) are always a whole multiple of gridCm (gridCm * decade * m where decade is a power of 10 and m ∈ {1,2,5}), so every drawn line is a real snap position — the "subset of snap positions, never a different grid" invariant holds. Snapping in GardenCanvas.tsx:155 still uses gridCm (the true grid), not drawnGridCm — verified the snap path is unchanged. Degenerate cases (0, negative, NaN, scale 0, sub-nanometer grid beyond the 5-decade cap) all return null, and the canvas renders the "grid too fine to draw" note instead of drawing nothing.
Pattern/grid alignment: The SVG <pattern> uses drawnGridCm (a multiple of gridCm), and the pattern is anchored at the SVG origin which is the garden's (0,0) world origin — same anchor as snapPoint — so drawn lines coincide with snap positions.
No correctness defects found within my lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/components/gardens/GardenFormModal.tsx:204 — The soft-warning did you mean {unitLabel}? is self-contradictory after the scale migration. The grid field is now labeled Garden grid (ft) / (m) (i.e. unitLabel), so when the warning fires it asks "did you mean ft?" while the user is already typing in a ft field. The warning only made sense when the grid was at spacing scale (in/cm) and the suggestion was to switch to the dimension unit. Under the new dimension scale, the "typed '1' meaning one foot" misentry the comments describe cannot produce a too-fine grid: cmFromDisplay(1, 'imperial') = cmFromFtIn(1) = 30.48 cm, well above MIN_GARDEN_GRID_CM = 10 (and even above the "sub-4″" ~10.16 cm the comment invokes), so the warning never fires for that case — it only fires for genuinely tiny values (e.g. 0.1 ft → 3.048 cm, 0.05 m → 5 cm), where asking "did you mean the unit you're already using?" is nonsensical. The comment at GardenFormModal.tsx:133-134 ("A sub-4″ garden grid is almost always '1' typed meaning one foot") and the MIN_GARDEN_GRID_CM comment at web/src/lib/units.ts:15-17 ("typing '1' in a feet field meaning one foot, and landing on a 1 cm grid") both repeat the same impossible scenario — typing 1 in a ft field yields 30.48 cm, not 1 cm. Suggested fix: rewrite the warning to match the new scale (e.g. describe the actual value without the "did you mean" unit prompt, or rethink what misentry it's guarding against), and update both comments so they don't describe a misentry the new scale can't produce.
Verified against units.ts (cmFromFtIn(1) = 30.48, dimensionUnitLabel returns ft/m, MIN_GARDEN_GRID_CM = 10, cmFromDisplay maps through cmFromFtIn/cmFromMeters) and GardenFormModal.tsx (field label Garden grid (${unitLabel}), gridEntered = cmFromDisplay(parseFloat(gridSize), unit), warning uses the same unitLabel).
⚡ Performance — No material issues found
VERDICT: No material issues found
visibleGridStepCm (web/src/lib/geometry.ts:117) runs on every render of GardenCanvas (it's called unconditionally at GardenCanvas.tsx:115). It's a pure loop bounded at GRID_COARSEN_MULTIPLES.length × (GRID_MAX_COARSEN_DECADES + 1) = 3 × 6 = 18 iterations, no allocation, early return on first match — negligible and not a hot loop concern (re-renders are pointer-driven, not per-frame animation). Verified by reading the function body.
The grid pattern/<rect> are rendered via a single SVG <pattern> (one fill), so coarsening does not increase line count or DOM nodes — it actually reduces paint work when zoomed out (larger drawnGridCm → fewer tile repetitions). Confirmed at GardenCanvas.tsx:221-228.
gridNote allocation (a short string or null) is once per render and trivial.
cmFromFtIn/cmFromMeters/cmFromSpacing now go through roundCm (one Math.round + one divide) instead of one Math.round — not a measurable regression; these run on user input, not in loops.
GardenFormModal's gridEntered/gridTooFine are computed on every render of the modal, but the modal is not a hot path and the work is two parseFloats and a comparison.
No N+1, unbounded growth, missing limits, or avoidable quadratic behavior was introduced. The change is performance-neutral-to-positive (coarsening reduces paint load when zoomed out).
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I verified the error-handling and edge-case behavior of every changed function against the checked-out code:
visibleGridStepCm (web/src/lib/geometry.ts:117): The guard !(gridCm > 0) || !(scale > 0) || !(minCellPx > 0) correctly rejects zero, negative, and NaN for all three inputs (verified NaN > 0 is false, so !false... actually !(NaN>0) = !false = true → returns null). The decade loop is bounded (d <= 5), so it always terminates; for a degenerate 1e-9 grid the max tested step is 5e-4 cm, below the floor, so it returns null rather than spinning or drawing nothing silently. Matches the test at geometry.test.ts.
GardenFormModal warning path (GardenFormModal.tsx:135): gridEntered = cmFromDisplay(parseFloat(gridSize), unit). For empty/non-numeric input, parseFloat returns NaN, cmFromDisplay propagates NaN through roundCm (verified: Math.round(NaN*1e6)/1e6 === NaN), and gridTooFine = Number.isFinite(NaN) && ... is false — so no warning renders and formatCm is never called with NaN. For 0 or negative input, gridEntered > 0 short-circuits gridTooFine to false. No panic, no malformed message on the unhappy paths.
onSubmit validation (GardenFormModal.tsx:83-93): parseFloat of an empty/invalid field yields NaN, cmFromDisplay(NaN, …) yields NaN, and isValidDimensionCm checks Number.isFinite first (verified at units.ts:22), so the form shows the range error instead of submitting NaN to the API. No regression vs. the previous cmFromSpacing path.
GardenCanvas null-step path (GardenCanvas.tsx:118-125, 221): When drawnGridCm == null, the <defs>/<pattern> block is skipped entirely (no rendering with null/0 width), and the note branch handles both snapToGrid on and off without falling through to formatCm(null). The drawnGridCm !== gridCm comparison is safe for null only on the already-narrowed non-null branch.
roundCm (units.ts:29): Math.round(NaN * 1e6) / 1e6 is NaN — it does not throw or coerce to 0, so NaN inputs flow through to the finite-checks at the validation layer rather than being silently masked as valid.
The only cosmetic nit (not material, not reported as a finding): for a sub-centimeter positive input like 0.0001, gridTooFine is true and formatCm renders 0′ 0″, an odd-looking but non-crashing message for a value that fails validation anyway.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
No authn/authz changes to review.
No new injection surfaces: all modified inputs are parsed with parseFloat and validated against existing server-side validDimensionCM bounds before submission. The SVG d attribute interpolation in GardenCanvas.tsx uses drawnGridCm, which is produced by visibleGridStepCm and is guarded against non-finite / non-positive values — it is not raw user input.
No SSRF, deserialization, secret leakage, or insecure defaults introduced.
The soft gridTooFine warning is UI-only and does not weaken server-side validation.
🎯 Correctness — No material issues found
No material issues found
I reviewed the change through the correctness lens and verified the actual checked-out code for the key files (web/src/lib/units.ts, web/src/lib/geometry.ts, web/src/components/gardens/GardenFormModal.tsx, web/src/editor/GardenCanvas.tsx, and the test files). All verifications confirm the following:
Exact conversions are correct.cmFromFtIn, cmFromMeters, and cmFromSpacing now use roundCm(cm * 1e6) / 1e6, which correctly removes IEEE-754 representation dust (e.g., 12 * 2.54 = 30.479999999999997 becomes 30.48) without throwing away meaningful precision. The conversion factors (2.54, 12, 100) are applied correctly.
Coarsening logic is correct.visibleGridStepCm returns the smallest multiple of the true grid (2×, 5×, 10×, …) whose cells exceed the legibility floor. Every returned line is still a real grid line, and snapping continues to use the true gridCm, so snapped positions are always a superset of drawn lines. Degenerate cases return null as documented.
Warning logic is correct. The soft-warning threshold (MIN_GARDEN_GRID_CM = 10) correctly identifies sub-10 cm grids. The warning only fires for finite, positive values below the threshold, and the displayed text uses the same formatCm helper used elsewhere.
Grid note logic is correct. The canvas note explains coarsening when drawnGridCm !== gridCm and explains an undrawable grid only when snapToGrid is active (the state where user confusion is highest). The != null check properly handles the null return from visibleGridStepCm.
The only side effect is that pre-existing imperial garden data that was quantized to whole centimeters will display slightly differently and may shift by sub-centimeter amounts on a no-op re-save. This is an explicit and accepted consequence of fixing the systematic quantization bug; the PR description states that existing gardens are intentionally not migrated.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Through the code-cleanliness lens, this PR is well-structured:
It removes a duplicated conversion path (convertGrid / cmFromSpacing / spacingFromCm / spacingUnitLabel) from GardenFormModal.tsx and consolidates grid handling onto the same dimension-scale helpers used for width and height. That reduces future maintenance burden when unit logic changes.
The new visibleGridStepCm helper in geometry.ts is cleanly factored out with a clear docstring and a single, testable responsibility. The coarsening logic is decoupled from React rendering, which keeps GardenCanvas.tsx readable.
roundCm in units.ts is a sensible private helper that replaces three previously copy-pasted Math.round(...) calls, centralizing the “dust-removal” policy.
Comments in both GardenCanvas.tsx and geometry.ts explain why the coarsening exists and what the two gridNote states mean, which prevents the next reader from re-introducing a fade-out or silently drawing nothing.
No dead code, no confusing names, no overly long or deeply nested functions, and no patterns that deviate from the surrounding codebase in a way that would trip up future maintainers.
⚡ Performance — No material issues found
No material issues found
visibleGridStepCm is a trivial fixed-bound loop (at most 18 iterations of multiplies/compares). Called on every viewport update, but the total work is ~40 arithmetic operations — negligible compared to the surrounding React reconciliation and SVG paint.
Pattern re-render frequency actually decreases. The old code continuously mutated strokeOpacity on every zoom frame, forcing the browser to re-paint the pattern. The new code only changes the pattern dimensions at discrete coarsening thresholds, and opacity is now constant. This is a net win for the compositor.
Unit conversions are cold-path. roundCm adds two multiplies and a Math.round versus the old single Math.round, but these functions are only hit on form input and initialization, never inside drag/animation loops.
No new allocations in hot paths. The gridNote string and formatCm call only execute when the coarsening level actually changes; the bedGrid memo is unchanged.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
After reviewing the diff through the error-handling & edge-cases lens, every guard, null/NaN check, and boundary condition appears correctly implemented:
GardenFormModal.tsx:135-136 — gridTooFine safely short-circuits on NaN / non-finite / non-positive input via Number.isFinite(gridEntered) && gridEntered > 0 before comparing against MIN_GARDEN_GRID_CM. Verified by reading the file.
units.ts:29-31 (roundCm) — rounds to 1e-6 cm, well below any garden-relevant precision, and handles the IEEE-754 dust case (12 × 2.54) without introducing overflow/underflow for the valid dimension range [1, 10000]. Verified.
geometry.ts:117-128 (visibleGridStepCm) — returns null on degenerate inputs (gridCm <= 0, scale <= 0, minCellPx <= 0) and after exhausting the coarsening cap, preventing silent no-op drawing. The caller (GardenCanvas.tsx) surfaces the null with an explanatory label rather than drawing nothing. Verified by reading both files.
Canvas grid rendering (GardenCanvas.tsx:221) — uses strict drawnGridCm != null before rendering the <pattern>, so no invalid SVG is produced for degenerate grids. Verified.
snapValue and snapLocalToBedGrid already tolerate step <= 0 by returning the original value, so disabling snapping never produces NaN or crashes. Verified by reading geometry.ts.
No ignored errors, missing clean-up, panics on bad input, or unhandled edge cases were found in this diff.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 5 findings (1 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | 3-level nested ternary for gridNote is harder to scan/modify than an if/else or small helper | `web/src/editor/GardenCanvas.tsx:118` | 2/5 | maintainability |
<details><summary>4 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Soft-warning 'did you mean {unitLabel}?' is self-contradictory now that the grid field is already in that unit; the '1 typed meaning one foot' comments are stale under the new dimension scale. | `web/src/components/gardens/GardenFormModal.tsx:204` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | cmFromDisplay(parseFloat(gridSize), unit) duplicated between warning calc and submit handler | `web/src/components/gardens/GardenFormModal.tsx:135` | glm-5.2:cloud | maintainability |
| ⚪ | Duplicated badge className string across two overlay spans | `web/src/editor/GardenCanvas.tsx:303` | glm-5.2:cloud | maintainability |
| ⚪ | roundCm's doc comment misstates the rounding grain as 1 nm when it's actually 10 nm (1e-6 cm = 10 nm) | `web/src/lib/units.ts:26` | claude-code/sonnet | correctness |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the security lens (injection, XSS, SSRF, deserialization, secrets, input validation, authz).
This PR is confined to client-side arithmetic and display logic for unit conversion (`units.ts`, `geometry.ts`) and two React components that render the results. I verified:
- **No injection sinks**: All new/changed code paths (`cmFromFtIn`, `cmFromMeters`, `cmFromSpacing`, `cmFromDisplay`, `roundCm`, `visibleGridStepCm`, `formatCm`) are pure numeric functions — no string concatenation into SQL, shell, template, or `dangerouslySetInnerHTML`.
- **No XSS**: `formatCm(...)` output is rendered as plain JSX text content (`GardenFormModal.tsx:204`, `GardenCanvas.tsx:305`), which React escapes automatically. The SVG pattern `id` (`gridId`, `GardenCanvas.tsx:69`) is unchanged by this diff and derived from React's `useId()`, not user input.
- **Input validation preserved**: `onSubmit` still runs `isValidDimensionCm` against the converted `widthCm`/`heightCm`/`gridSizeCm` before submitting (`GardenFormModal.tsx:83-93`), matching the server's documented `[1cm, 100m]` bounds (`units.ts:11`). The new `MIN_GARDEN_GRID_CM` check is a soft UI warning only (`GardenFormModal.tsx:136,202-206`) and doesn't weaken that hard validation — it's additive.
- **No new untrusted-data handling**: all values flowing through the changed functions originate from the same trusted client-side form state as before; nothing here reads from the network, deserializes payloads, or handles secrets/credentials.
The `roundCm` precision fix and the grid/dimension-scale rework don't touch authn/authz, don't introduce new external calls, and don't loosen any existing validation.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## Review
**VERDICT: Minor issues**
I independently re-derived the conversion math and re-read `web/src/lib/units.ts` and `web/src/lib/geometry.ts` against the diff.
- `web/src/lib/units.ts:25-28` — the `roundCm` doc comment claims "1 nanometer is far below anything a garden cares about," but the actual rounding grain is `1e-6 cm`, which is **10 nanometers**, not 1 (1 cm = 1e7 nm, so `1e-6 cm = 1e-6 × 1e7 nm = 10 nm`). Confirmed: `roundCm` is defined exactly as `Math.round(cm * 1e6) / 1e6` (units.ts:29-31), and the "1 nanometer" phrasing sits on line 26 of that same doc comment. This is a comment-only inaccuracy — the rounding constant itself (1e-6 cm, i.e. sub-micron) is still functionally fine for garden-scale dimensions — with no behavioral effect on any of the conversion functions.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Confirmed — the "no dead code" claim and `unitPref` field both check out. The draft's single finding is legitimate and verifiable in the source. Here's the corrected review:
## VERDICT: Minor issues
**Lens: 🧹 Code cleanliness & maintainability**
- **`web/src/editor/GardenCanvas.tsx:118-125`** — `gridNote` is a 3-level nested ternary (`drawnGridCm == null` → `garden.snapToGrid` → `drawnGridCm !== gridCm`) picking between three string/null outcomes. It's well-commented, but nested ternaries like this are harder to scan and riskier to extend than an early-return helper or plain if/else. Suggest pulling it into a small named function, e.g. `gridNoteFor(drawnGridCm, gridCm, snapToGrid, unit)`, same logic with explicit branches.
- Everything else checked out clean:
- `roundCm` (`web/src/lib/units.ts:29`) is a tightly-scoped private helper with a comment explaining *why* (IEEE-754 dust vs. real precision loss).
- No dead code: `spacingFromCm`/`spacingUnitLabel`/`cmFromSpacing`/`formatSpacing` were dropped from `GardenFormModal.tsx`'s imports but remain live — confirmed via grep they're still used by `Inspector.tsx`, `PlopInspector.tsx`, `PlantFormModal.tsx`, `PlantCard.tsx`, and `PlantPicker.tsx` for the (intentionally separate) bed-grid/plant-spacing scale.
- `GRID_MAX_OPACITY` → `GRID_OPACITY` rename in `GardenCanvas.tsx` correctly reflects it's no longer a max (fading was replaced by coarsening).
- The `convertGrid`/`convert` consolidation in `GardenFormModal.tsx:57-68` actually *removes* prior duplication rather than adding any, now that the grid uses the same dimension-scale helper as width/height.
- `visibleGridStepCm` (`web/src/lib/geometry.ts:117-128`) is compact, well-commented, and has matching test coverage in `geometry.test.ts`.
No other maintainability issues in this lens. Outside my lens: correctness/test coverage looked solid on read-through, but that's a different reviewer's job.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls).
- `visibleGridStepCm` (`web/src/lib/geometry.ts:117`) runs on every `GardenCanvas` render — including the high-frequency re-renders during a live object/plop drag, per the comment at `GardenCanvas.tsx:201`. It's an unmemoized nested loop, but bounded to at most `6 × 3 = 18` iterations of trivial multiply/compare — negligible next to the SVG re-render work already happening on the same path (verified by reading the full function; no unbounded loop, no allocation inside it).
- `bedGridLines` / `BED_GRID_MAX_LINES` cap is unchanged by this diff (`GardenCanvas.tsx:27,38-43`) — the new grid-coarsening logic doesn't touch the per-bed line generation, so the existing 200-line safety cap still applies there.
- The new `gridEntered`/`gridTooFine` computation in `GardenFormModal.tsx:135-136` re-parses `gridSize` on every render, but it's a single `parseFloat` + arithmetic on a form with a handful of fields — not a hot path.
- `roundCm` (`units.ts:29-31`) adds one `Math.round` per conversion call; conversions happen only on user input/unit-toggle events, not in any loop or render path.
No N+1s, no missing pagination, no quadratic behavior, no new blocking calls — this PR is UI/unit-conversion logic with no data-fetching or bulk-processing surface. Nothing here rises to a performance finding.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed this PR specifically for error handling and edge cases: NaN/invalid input propagation, boundary conditions in the new unit-conversion math, and the new grid-coarsening logic's degenerate cases.
What I checked (verified by reading the actual code, not just the diff):
- `roundCm`/`cmFromFtIn`/`cmFromMeters`/`cmFromSpacing` (`web/src/lib/units.ts:29-98`): NaN/negative/zero inputs flow through untouched and are caught downstream by `isValidDimensionCm`'s `Number.isFinite` + range check before ever reaching the server payload. Confirmed `isValidDimensionCm` is applied to `gridSizeCm` at submit (`GardenFormModal.tsx:89-93`), not just width/height.
- `gridTooFine` warning (`GardenFormModal.tsx:135-136`): guards `Number.isFinite(gridEntered) && gridEntered > 0` before comparing to `MIN_GARDEN_GRID_CM`, so an empty/cleared field (`parseFloat('') → NaN`) or a zero/negative legacy value correctly suppresses the warning rather than rendering "NaN′ NaN″" — and zero/negative are still hard-blocked by the submit-time `isValidDimensionCm` check either way, so nothing slips through silently.
- `visibleGridStepCm` (`web/src/lib/geometry.ts:117-128`): guards `gridCm > 0`, `scale > 0`, `minCellPx > 0` up front, so corrupt/legacy grid values of `0` or negative (exactly the kind of bad data the PR calls out for Home) return `null` rather than dividing or looping unboundedly. Loop is bounded (6 decades × 3 multiples = 18 iterations max), so there's no runaway/infinite loop risk. Boundary case (`step * scale >= minCellPx`, using `>=` not `>`) matches the "exactly at the minimum" test case with no off-by-one.
- Caller side in `GardenCanvas.tsx:118-125,221`: `drawnGridCm == null` is handled explicitly (skips rendering the pattern, shows an explanatory note only when `snapToGrid` is on) — no crash path when the grid can't be drawn at all, unlike the old fade logic that silently produced 0 opacity with snap still active.
- Confirmed `EditorGarden.unitPref` is a required (non-optional) field in `web/src/editor/types.ts:35`, so `formatCm(drawnGridCm, garden.unitPref)` isn't reading from a possibly-undefined value.
Nothing in this diff introduces an unguarded panic, a swallowed error, or a boundary condition that isn't already defended against by an existing check (`isValidDimensionCm`, `Number.isFinite`, or the new `visibleGridStepCm` guards). The validation/warning logic is consistently soft-fail (warn/block via form error) rather than throwing.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
The security lens here covers client-side form validation and unit conversion — no auth/authz, injection, or untrusted-deserialization surface in this diff. The form still submits to the same server-side `validDimensionCM` range check (`internal/service/gardens.go:256`), which guards NaN/Inf and bounds. The client-side changes are pure numeric conversion/display logic with no taint flow into queries, commands, or paths.
I traced the tainted input path: `parseFloat(gridSize)` → `cmFromDisplay` → `input.gridSizeCm` → `create.mutateAsync`/`update.mutateAsync` → server `validDimensionCM`. No bypass introduced; the server remains the authority. The `roundCm` helper operates only on already-numeric values; no string templating into SQL/HTML. The `formatCm(gridEntered, unit)` interpolated into JSX is a number→string formatter, not raw HTML injection (React escapes). Nothing material in my lane.
**Verdict: No material issues found**
- Verified the server-side validation (`internal/service/gardens.go:252-258`) still gates `GridSizeCM` via `validDimensionCM` (finite + [1cm, 100m]) regardless of the client's new exact-conversion logic — no validation bypass.
- Verified `roundCm`/`cmFromDisplay`/`cmFromFtIn`/`cmFromSpacing` are pure numeric transforms over `parseFloat` output; no string interpolation into queries, commands, paths, or templates.
- Verified `formatCm(gridEntered, unit)` in `GardenFormModal.tsx:204` and `formatCm(drawnGridCm, ...)` in `GardenCanvas.tsx:124` render through JSX (auto-escaped), not `dangerouslySetInnerHTML` — no XSS vector.
- No auth/authz, secret, SSRF, or deserialization changes in this diff.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Verdict: No material issues found.
I verified the change through the correctness lens — re-deriving the unit conversions, the coarsening math, and the round-trip identities against the checked-out code.
- **Exact conversions** (`web/src/lib/units.ts:29-41, 97-99`): `roundCm` = `Math.round(cm*1e6)/1e6`. Re-derived: `12 * 2.54 = 30.48` (the IEEE dust `30.479999999999997` rounds back to `30.48`); `1 ft → 30.48`; `4'6" → 137.16`; `0.305 m → 30.5 cm`. All match the tests and first principles. The 1e-6 cm floor is ~1 nm — far below garden precision and below the display rounding (0.1 ft ≈ 3 cm, 0.01 m = 1 cm), so it cannot show through in a field.
- **Round-trip identity** (`displayFromCm ∘ cmFromDisplay`): For imperial, `displayFromCm` = `round(roundCm(ft*30.48)/30.48 * 10)/10`. For the tested ft values (0.5, 1, 2.5, 4, 8, 12.5, 24) this returns ft exactly. For metric, `round(roundCm(m*100))/100` returns m exactly for the tested set. Confirmed by reading `units.ts:44-57`.
- **Scale switch for the garden grid** (`GardenFormModal.tsx:49, 89, 122`): grid now uses `cmFromDisplay`/`displayFromCm` (dimension scale, ft/m) instead of `cmFromSpacing`/`spacingFromCm` (cm/in). The bed grid in `Inspector.tsx` still uses the spacing helpers (`Inspector.tsx:54,71,98-100`), correctly untouched. No cross-file caller of the removed `cmFromSpacing`/`spacingUnitLabel` imports in `GardenFormModal` remains (grep confirms the form no longer references them).
- **Coarsening** (`geometry.ts:117-128`): `visibleGridStepCm` returns the smallest `gridCm * decade * m` (m ∈ {1,2,5}) with `step*scale >= minCellPx`. Re-derived the test cases: `3 @ 0.84 → 15` (2.52, 5.04 < 6; 12.6 ≥ 6), `1 @ 0.84 → 10` (0.84…4.2 < 6; 8.4 ≥ 6), `10 @ 0.6 → 10` (exactly 6). Decade cap: d=0..5 → decade ∈ {1,10,…,1e5}, max step = 5e5·gridCm, matching the "500,000×" comment. Every returned step is a whole multiple of `gridCm`, so drawn lines are a subset of snap positions — confirmed snapping still uses `gridCm` at `GardenCanvas.tsx:155`, not `drawnGridCm`.
- **Soft warning** (`GardenFormModal.tsx:133-136`): `MIN_GARDEN_GRID_CM = 10` cm = 3.937″ ≈ the "sub-4″" comment. `gridEntered` is NaN-safe (`Number.isFinite` guard). Re-derived Home's case: `displayFromCm(3, 'imperial') = round(0.9843)/10 = 0.1` ft (field shows 0.1), and `formatCm(3, 'imperial') = "0′ 1″"` (warning text) — both match the PR description. Typing "1" in the new ft field → 30.48 cm, correctly *not* warned.
No logic bugs, incorrect constants, or semantic errors found within the correctness lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues found**
Reviewed through the cleanliness/maintainability lens. The change is well-structured overall: the spacing helpers were correctly left in place for the bed-grid/plant-spacing callers, the `convertGrid` special-case was collapsed into the shared `convert` once the grid moved to dimension scale, and `visibleGridStepCm` is a clean, self-contained extraction with a good doc comment. A few small nits:
- `web/src/editor/GardenCanvas.tsx:303` & `:307` — the exact same badge className string (`rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur`) is duplicated across the two `<span>`s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g. `const badgeClass = '...'`) would keep the two badges in sync if the styling ever changes. Trivial.
- `web/src/editor/GardenCanvas.tsx:118` — `gridNote` is a three-level nested ternary (`drawnGridCm == null ? … : drawnGridCm !== gridCm ? … : null`). It's readable as written and matches the "two states worth explaining" comment, but a small `if`/early-return helper or a lookup would flatten it. Not worth churning on its own. Trivial.
- `web/src/components/gardens/GardenFormModal.tsx:135` — `cmFromDisplay(parseFloat(gridSize), unit)` is computed inline on every render for the warning, and the same expression reappears in the submit handler (`:89`). The two are in different scopes so extracting isn't strictly necessary, but a shared `const gridEnteredCm` derived once would remove the duplication. Small.
No dead code, no leaky abstractions, no patterns broken relative to the surrounding files. The `roundCm` helper is a nice single-source replacement for the scattered `Math.round` calls. Nothing here is blocking.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
Performance lens: the change replaces a fade-opacity computation with `visibleGridStepCm`, both O(1) per render — the latter does at most ~18 iterations of trivial arithmetic (3 multipliers × 6 decades), strictly cheaper than nothing material. The grid `pattern`/`rect` is only re-rendered when `drawnGridCm` is non-null, same gating as before. The new `gridTooFine` check in the form is a couple of parseFloats on keystroke (form modal, not hot path). The `formatCm` calls on `gridNote` only fire when the note is shown. No N+1, no unbounded growth, no new per-frame allocation (the `useMemo`s are unchanged). `GRID_MAX_COARSEN_DECADES = 5` gives a 500,000× cap that bounds the loop unconditionally. Nothing here is a regression.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
Through the error-handling / edge-case lens, the change is careful. Verified by reading the actual checked-out code:
- `visibleGridStepCm` (geometry.ts:117-128) guards all degenerate inputs with `!(x > 0)`, which correctly rejects `NaN`, `0`, and negatives for `gridCm`, `scale`, and `minCellPx` (since `NaN > 0` is `false`, `!(false)` → returns `null`). The coarsening loop is bounded (`d <= GRID_MAX_COARSEN_DECADES`), so no unbounded iteration, and the `null` result is surfaced by the caller as an explicit "grid too fine to draw — zoom in" note rather than swallowed. The degenerate cases are even unit-tested (geometry.test.ts).
- `GardenFormModal` warning path (GardenFormModal.tsx:135-136): `gridEntered = cmFromDisplay(parseFloat(gridSize), unit)`. `parseFloat` can return `NaN` (empty/garbage input) or a negative; `gridTooFine` guards with `Number.isFinite(gridEntered) && gridEntered > 0 && gridEntered < MIN_GARDEN_GRID_CM`, so `NaN`/negative/zero don't produce a spurious warning. The `0` and `NaN` cases are instead caught at submit by `isValidDimensionCm` (which checks `Number.isFinite` and range) with a clear form error — not swallowed.
- Submit path (GardenFormModal.tsx:83-93): width/height/grid all run through `cmFromDisplay` → `isValidDimensionCm`, which rejects non-finite and out-of-range values with explicit messages; no unchecked `parseFloat` flows into the mutation.
- Canvas render guard (GardenCanvas.tsx:221) switched from an opacity threshold to `drawnGridCm != null`, consistent with `visibleGridStepCm`'s null contract — a null (degenerate grid) correctly skips the `<pattern>`/`<rect>` render rather than rendering with `width={null}`/`NaN`.
- The `roundCm` helper (units.ts:29-31) operates on finite products of well-bounded inputs; no overflow concern at the magnitudes involved (max ~10000 cm).
No unhappy-path regressions introduced. The `Infinity` input case for `visibleGridStepCm` is theoretically unguarded, but `gridCm` is server-bounded to `[1, 10000]` and `viewport.scale` is a real zoom factor, so it is not reachable here — not a material finding.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
The PR is a units-correctness refactor with no security-relevant changes. All user inputs (grid size, width, height) continue to be:
- parsed with `parseFloat` (same as before),
- converted through well-bounded helper functions,
- validated client-side with `isValidDimensionCm` (range 1–10,000 cm, finiteness check),
- and re-validated server-side with `validDimensionCM` (identical range and `math.IsNaN`/`math.IsInf` guards).
The new soft warning for sub-10 cm garden grids renders via React JSX with `formatCm`, which returns plain numeric strings—no HTML injection path. No secrets, credentials, SQL, commands, paths, or SSRF surfaces are touched.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
I scrutinized the change through the **Correctness** lens and found the mathematics sound:
- **`roundCm`** correctly clears IEEE-754 dust at 1e-6 cm (~10 nm) without clipping garden-meaningful precision. `cmFromFtIn(0, 12)` → `30.48`, `cmFromMeters(0.305)` → `30.5`, etc. all verified.
- **Exact conversion** for `cmFromFtIn`, `cmFromMeters`, and `cmFromSpacing` is correct: the server stores `REAL`, so rounding to whole cm was a client-side bug. Display-side rounding in `displayFromCm`/`spacingFromCm` remains unchanged, preserving clean UI round-trips.
- **`visibleGridStepCm`** coarsening logic verified: it returns the smallest 1×/2×/5×/10×… multiple of the true grid that clears `minCellPx`, and `null` only when even 500,000× falls short. Snapping continues to use the true grid, so drawn lines are always a subset of snap positions — never a different grid.
- **Garden form** correctly moves the garden grid to dimension scale (ft/m) while keeping bed/plant spacing at cm/in. The `MIN_GARDEN_GRID_CM = 10` soft floor warns without refusing, which matches the issue description.
- **No regressions** in existing callers: `PlantFormModal`, `Inspector`, and `PlopInspector` still use `cmFromSpacing` for plant spacing; only `GardenFormModal` switched to `cmFromDisplay` for the garden grid.
Tests cover the exact-conversion identity table, the #47 coarsening case (3 cm @ 0.84 px/cm → 15 cm), and degenerate-grid null behavior. No logic bugs detected.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
I reviewed all six changed files through the maintainability lens:
- **`units.ts`**: The new `roundCm` helper consolidates IEEE-754 dust rounding into one place instead of repeating it across `cmFromFtIn`, `cmFromMeters`, and `cmFromSpacing`. The `MIN_GARDEN_GRID_CM` constant is well-commented and clearly named.
- **`geometry.ts`**: `visibleGridStepCm` is a clean, focused function with a clear docstring. The coarsening constants are module-local and self-documenting.
- **`GardenFormModal.tsx`**: Removing the separate `convertGrid` closure and reusing the existing `convert` function reduces duplication and makes the unit-switching path simpler. The warning markup is a straightforward conditional block.
- **`GardenCanvas.tsx`**: Replacing the opacity math with a semantic `drawnGridCm != null` check is clearer. The `gridNote` ternary, while nested, is small and directly follows the two states the function returns.
- **Test files**: Old whole-cm assertions were updated to exact values, and new tests cover the coarsening and round-trip behavior with descriptive names.
No dead code, no confusing names, no overly long functions, no copy-paste that should be shared, and no leaky abstractions.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
From a performance lens, the changes are clean:
- **`visibleGridStepCm`** (geometry.ts:117) is a bounded loop (≤15 iterations of simple arithmetic) called once per `GardenCanvas` render. It replaces continuous opacity interpolation with discrete step changes, which is neutral-to-positive for rendering cost during zoom.
- The SVG `<pattern>` is only recreated when `drawnGridCm` actually changes; between threshold crossings React reconciles it away. No excess DOM churn.
- `formatCm` and `cmFromDisplay` calls in `GardenFormModal` are form-field-only, not hot-path.
- No new unmemoized arrays/objects being passed as props, no added subscribers or resize observers, no layout-thrashing patterns introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
Through the 🧯 error-handling & edge-cases lens, the diff is clean. I verified the following:
- **`visibleGridStepCm`** (geometry.ts:117) guards against `NaN`, `0`, negative, and non-finite inputs with `!(gridCm > 0) || !(scale > 0) || !(minCellPx > 0)` and returns `null` for all degenerate cases. The loop is capped at 18 iterations, so there is no runaway or overflow risk.
- **`GardenFormModal`** (GardenFormModal.tsx:135-136) correctly guards the soft-warning check with `Number.isFinite(gridEntered) && gridEntered > 0`, so empty/invalid/malformed input, zero, and negative values do not spuriously trigger the “did you mean ft?” message. The downstream `isValidDimensionCm` call still catches invalid values at submit time.
- **`roundCm`** (units.ts:29-31) safely handles `NaN` and `Infinity` by passing them through; the only “gotcha” is sub-nanometer values round to `0`, but those are rejected by `isValidDimensionCm` anyway.
- **`gridNote`** logic in `GardenCanvas` (GardenCanvas.tsx:118-125) distinguishes the three states (true grid, coarsened grid, degenerate grid) without crashing on `null` from `visibleGridStepCm`.
- The test suite explicitly covers the 3 cm @ 0.84 px/cm failure case, whole-multiple invariant, and degenerate/null returns.
No ignored errors, missing cleanup, panics, or unhandled edge cases are introduced by this change.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
Reviewed through the security lens: input validation, injection, taint tracing, and defaults.
- **Input validation intact.** `GardenFormModal.tsx:89-93` still runs `parseFloat` → `cmFromDisplay` → `isValidDimensionCm`, which checks `Number.isFinite(cm) && cm >= 1 && cm <= 10_000` (`units.ts:21-22`). This mirrors the server's `validDimensionCM` in `internal/service/gardens.go:274-275` (finite + `[minGardenCM, maxGardenCM]`). NaN from non-numeric input (`parseFloat("abc")` → NaN → `cmFromFtIn(NaN)` → `roundCm(NaN)` → NaN) is rejected by the finite check. Negative values rejected by the `>= 1` floor. No path where a bad value bypasses both client and server checks.
- **No injection surface.** The only user-controlled text interpolated into the DOM is the `gridTooFine` warning (`GardenFormModal.tsx:202-206`) and the canvas `gridNote` (`GardenCanvas.tsx`). Both feed into React JSX as text children (`{formatCm(...)}`, `{gridNote}`, `{unitLabel}`), which React escapes; no `dangerouslySetInnerHTML`. `unitLabel` is a constant (`'ft'`/`'m'`) from `dimensionUnitLabel`, not user input. `formatCm`/`gridNote` are built from numeric conversions of user input, not raw strings.
- **No SSRF / command / SQL / deserialization / secret paths** touched by this diff.
- **Server-side trust boundary unchanged.** The Go `gardenFromInput` (`gardens.go:214-268`) still re-validates `GridSizeCM` via `validDimensionCM` and `UnitPref` against the metric/imperial allowlist (`gardens.go:244-245`); the client's new exact-cm conversion doesn't widen what the server accepts (REAL column already accepted fractional cm — the PR description's point).
Verification: read `web/src/lib/units.ts` (full), `web/src/components/gardens/GardenFormModal.tsx` (full), `internal/service/gardens.go:230-276`, `web/src/editor/Inspector.tsx:1-101`, and grepped for `validDimensionCM`, `dangerouslySetInnerHTML`/`innerHTML`/`eval` in the editor, and `UnitMetric`/`UnitImperial` definitions. No gaps found in the security lane.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I'll review this through the correctness lens, verifying against the actual code.
Now let me check the conversion in changeUnit to verify whether convert handles the case where the existing `width` is non-finite.
Let me verify the round-trip identity table and check the `cmFromMeters(1.22)` case.
Let me re-derive the round-trip claims from the test:
**Metric round-trip**: `m → cm → m` via `cmFromMeters(m)` then `displayFromCm(cm, 'metric')`.
- `cmFromMeters(0.15)` = `roundCm(0.15 * 100)` = `roundCm(15.0...)` = 15
- `displayFromCm(15, 'metric')` = `Math.round((15/100)*100)/100` = `Math.round(15)/100` = 0.15 ✓
- `cmFromMeters(1.22)` = `roundCm(122.0...)` = 122
- `displayFromCm(122, 'metric')` = `Math.round((122/100)*100)/100` = 1.22 ✓
- `cmFromMeters(3.05)` = `roundCm(305.0...)` = 305
- `displayFromCm(305, 'metric')` = 3.05 ✓
But wait — `cmFromMeters(0.305)` = `roundCm(30.4999...)` = 30.5. Test asserts `30.5` ✓.
**Imperial round-trip**: `ft → cm → ft` via `cmFromFtIn(ft)` then `displayFromCm(cm, 'imperial')` (which divides by 30.48 and rounds to 0.1).
- `cmFromFtIn(0.5)` = `roundCm(6 * 2.54)` = 15.24
- `displayFromCm(15.24, 'imperial')` = `Math.round((15.24/30.48) * 10)/10` = `Math.round(0.5 * 10)/10` = 0.5 ✓
- `cmFromFtIn(2.5)` = `roundCm(30 * 2.54)` = 76.2
- `displayFromCm(76.2, 'imperial')` = `Math.round((76.2/30.48)*10)/10` = `Math.round(2.5*10)/10` = 2.5 ✓
- `cmFromFtIn(12.5)` = `roundCm(150 * 2.54)` = 381
- `displayFromCm(381, 'imperial')` = `Math.round((381/30.48)*10)/10` = `Math.round(12.5*10)/10` = 12.5 ✓
These check out.
Now let me verify the `visibleGridStepCm` cap and the degenerate case:
For `visibleGridStepCm(1e-9, 1, 6)`: need `step * 1 >= 6`, i.e. `gridCm * decade * m >= 6`, so `1e-9 * decade * m >= 6`. Max decade is `1e5` (5 iterations of `*= 10`), max m=5, so `1e-9 * 1e5 * 5 = 5e-4` — way below 6. Returns null ✓.
For `visibleGridStepCm(1, 0.84, 6)`: `1*0.84=0.84 <6`; `2*0.84=1.68 <6`; `5*0.84=4.2 <6`; next decade `10*0.84=8.4 >=6` → returns 10 ✓.
For `visibleGridStepCm(3, 0.84, 6)`: `3*0.84=2.52 <6`; `6*0.84=5.04 <6`; `15*0.84=12.6 >=6` → returns 15 ✓.
Now let me verify the concern about the `gridTooFine` warning. The check is `gridEntered > 0 && gridEntered < MIN_GARDEN_GRID_CM` (10 cm). But the warning text interpolates `formatCm(gridEntered, unit)`. Let me check the warning text for the actual problem case described in the PR: a 1 ft entry would be 30.48 cm, which is NOT < 10 cm. So the warning only fires for sub-10cm entries. Let me check the PR text claims and the comment.
I have enough to render the verdict. Let me check one thing — whether the round-trip assertion could fail at boundary cases due to `displayFromCm` rounding at the edge. The test uses `[0.5, 1, 2.5, 4, 8, 12.5, 24]`. Let me check `cmFromFtIn(4)` = `roundCm(4*12*2.54)` = `roundCm(121.92)` = 121.92; `displayFromCm(121.92, 'imperial')` = `Math.round((121.92/30.48)*10)/10` = `Math.round(4*10)/10` = 4 ✓. `cmFromFtIn(8)` = 243.84; display = `Math.round(8*10)/10` = 8 ✓. `cmFromFtIn(24)` = `roundCm(24*12*2.54)` = `roundCm(731.52)` = 731.52; display = `Math.round(24*10)/10` = 24 ✓.
All checks pass. I verified the changes against the actual code — the round-trip identity, the conversion exactness, the `visibleGridStepCm` coarsening and null cases, and the snap-uses-true-grid invariant.
## Review
**Verdict: No material issues found**
I verified the correctness of all three changes against the actual checked-out code:
- **Exact conversions** (`web/src/lib/units.ts:29-99`): `roundCm` = `Math.round(x*1e6)/1e6` correctly clears IEEE-754 dust (re-derived: `12 * 2.54 = 30.479999999999997` → `Math.round(30479999.99999997)/1e6 = 30.48` ✓). The round-trip identity table (`units.test.ts:38-46`) holds for the asserted values: I traced each imperial value (0.5, 1, 2.5, 4, 8, 12.5, 24) and metric value (0.15, 1, 1.22, 3.05, 10, 24.5) through `cmFromDisplay → displayFromCm` and every one round-trips exactly within the display precision (0.1 ft / 0.01 m). The 1e-6 cm granularity is ~4 orders below garden precision, as claimed.
- **Garden grid at dimension scale** (`GardenFormModal.tsx:48-67, 89, 122, 135`): The grid now uses `cmFromDisplay`/`displayFromCm` consistently — entry, conversion, conflict-rebase, and save all use the same helpers as width/height. The `convert` closure in `changeUnit` is unit-preserved correctly (closes over `unit`, the *current* unit, converting to `next`). The soft-warning floor `MIN_GARDEN_GRID_CM = 10` cm ≈ 4″ matches the comment "sub-4″"; `gridTooFine` requires `> 0` and `< 10`, so a legitimate 1 ft grid (30.48 cm) correctly does NOT trigger the warning, only true mis-entries do.
- **Coarsening instead of fade** (`geometry.ts:117-128`, `GardenCanvas.tsx:114-125, 221-225`): I re-derived the key cases: `visibleGridStepCm(3, 0.84, 6)` → tries `3*0.84=2.52`, `6*0.84=5.04`, `15*0.84=12.6 ≥ 6` → returns 15 ✓. The drawn lines (`drawnGridCm`) are always a whole multiple of `gridCm` (`gridCm * decade * m` where decade is a power of 10 and m ∈ {1,2,5}), so every drawn line is a real snap position — the "subset of snap positions, never a different grid" invariant holds. Snapping in `GardenCanvas.tsx:155` still uses `gridCm` (the true grid), not `drawnGridCm` — verified the snap path is unchanged. Degenerate cases (0, negative, NaN, scale 0, sub-nanometer grid beyond the 5-decade cap) all return `null`, and the canvas renders the "grid too fine to draw" note instead of drawing nothing.
- **Pattern/grid alignment**: The SVG `<pattern>` uses `drawnGridCm` (a multiple of `gridCm`), and the pattern is anchored at the SVG origin which is the garden's (0,0) world origin — same anchor as `snapPoint` — so drawn lines coincide with snap positions.
No correctness defects found within my lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Verdict: Minor issues
- `web/src/components/gardens/GardenFormModal.tsx:204` — The soft-warning `did you mean {unitLabel}?` is self-contradictory after the scale migration. The grid field is now labeled `Garden grid (ft)` / `(m)` (i.e. `unitLabel`), so when the warning fires it asks "did you mean ft?" while the user is already typing in a ft field. The warning only made sense when the grid was at spacing scale (in/cm) and the suggestion was to switch to the dimension unit. Under the new dimension scale, the "typed '1' meaning one foot" misentry the comments describe cannot produce a too-fine grid: `cmFromDisplay(1, 'imperial') = cmFromFtIn(1) = 30.48 cm`, well above `MIN_GARDEN_GRID_CM = 10` (and even above the "sub-4″" ~10.16 cm the comment invokes), so the warning never fires for that case — it only fires for genuinely tiny values (e.g. `0.1 ft` → 3.048 cm, `0.05 m` → 5 cm), where asking "did you mean the unit you're already using?" is nonsensical. The comment at `GardenFormModal.tsx:133-134` ("A sub-4″ garden grid is almost always '1' typed meaning one foot") and the `MIN_GARDEN_GRID_CM` comment at `web/src/lib/units.ts:15-17` ("typing '1' in a feet field meaning one foot, and landing on a 1 cm grid") both repeat the same impossible scenario — typing `1` in a ft field yields 30.48 cm, not 1 cm. Suggested fix: rewrite the warning to match the new scale (e.g. describe the actual value without the "did you mean" unit prompt, or rethink what misentry it's guarding against), and update both comments so they don't describe a misentry the new scale can't produce.
Verified against `units.ts` (`cmFromFtIn(1) = 30.48`, `dimensionUnitLabel` returns `ft`/`m`, `MIN_GARDEN_GRID_CM = 10`, `cmFromDisplay` maps through `cmFromFtIn`/`cmFromMeters`) and `GardenFormModal.tsx` (field label `Garden grid (${unitLabel})`, `gridEntered = cmFromDisplay(parseFloat(gridSize), unit)`, warning uses the same `unitLabel`).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
- `visibleGridStepCm` (`web/src/lib/geometry.ts:117`) runs on every render of `GardenCanvas` (it's called unconditionally at `GardenCanvas.tsx:115`). It's a pure loop bounded at `GRID_COARSEN_MULTIPLES.length × (GRID_MAX_COARSEN_DECADES + 1)` = 3 × 6 = **18 iterations**, no allocation, early return on first match — negligible and not a hot loop concern (re-renders are pointer-driven, not per-frame animation). Verified by reading the function body.
- The grid pattern/`<rect>` are rendered via a single SVG `<pattern>` (one fill), so coarsening does not increase line count or DOM nodes — it actually *reduces* paint work when zoomed out (larger `drawnGridCm` → fewer tile repetitions). Confirmed at `GardenCanvas.tsx:221-228`.
- `gridNote` allocation (a short string or `null`) is once per render and trivial.
- `cmFromFtIn`/`cmFromMeters`/`cmFromSpacing` now go through `roundCm` (one `Math.round` + one divide) instead of one `Math.round` — not a measurable regression; these run on user input, not in loops.
- `GardenFormModal`'s `gridEntered`/`gridTooFine` are computed on every render of the modal, but the modal is not a hot path and the work is two `parseFloat`s and a comparison.
No N+1, unbounded growth, missing limits, or avoidable quadratic behavior was introduced. The change is performance-neutral-to-positive (coarsening reduces paint load when zoomed out).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
VERDICT: No material issues found
I verified the error-handling and edge-case behavior of every changed function against the checked-out code:
- **`visibleGridStepCm`** (`web/src/lib/geometry.ts:117`): The guard `!(gridCm > 0) || !(scale > 0) || !(minCellPx > 0)` correctly rejects zero, negative, and `NaN` for all three inputs (verified `NaN > 0` is `false`, so `!false`... actually `!(NaN>0)` = `!false` = `true` → returns `null`). The decade loop is bounded (`d <= 5`), so it always terminates; for a degenerate `1e-9` grid the max tested step is `5e-4 cm`, below the floor, so it returns `null` rather than spinning or drawing nothing silently. Matches the test at `geometry.test.ts`.
- **`GardenFormModal`** warning path (`GardenFormModal.tsx:135`): `gridEntered = cmFromDisplay(parseFloat(gridSize), unit)`. For empty/non-numeric input, `parseFloat` returns `NaN`, `cmFromDisplay` propagates `NaN` through `roundCm` (verified: `Math.round(NaN*1e6)/1e6 === NaN`), and `gridTooFine = Number.isFinite(NaN) && ...` is `false` — so no warning renders and `formatCm` is never called with `NaN`. For `0` or negative input, `gridEntered > 0` short-circuits `gridTooFine` to `false`. No panic, no malformed message on the unhappy paths.
- **`onSubmit`** validation (`GardenFormModal.tsx:83-93`): `parseFloat` of an empty/invalid field yields `NaN`, `cmFromDisplay(NaN, …)` yields `NaN`, and `isValidDimensionCm` checks `Number.isFinite` first (verified at `units.ts:22`), so the form shows the range error instead of submitting `NaN` to the API. No regression vs. the previous `cmFromSpacing` path.
- **`GardenCanvas`** null-step path (`GardenCanvas.tsx:118-125, 221`): When `drawnGridCm == null`, the `<defs>`/`<pattern>` block is skipped entirely (no rendering with `null`/`0` width), and the note branch handles both `snapToGrid` on and off without falling through to `formatCm(null)`. The `drawnGridCm !== gridCm` comparison is safe for `null` only on the already-narrowed non-null branch.
- **`roundCm`** (`units.ts:29`): `Math.round(NaN * 1e6) / 1e6` is `NaN` — it does not throw or coerce to `0`, so NaN inputs flow through to the finite-checks at the validation layer rather than being silently masked as valid.
The only cosmetic nit (not material, not reported as a finding): for a sub-centimeter positive input like `0.0001`, `gridTooFine` is `true` and `formatCm` renders `0′ 0″`, an odd-looking but non-crashing message for a value that fails validation anyway.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
- No authn/authz changes to review.
- No new injection surfaces: all modified inputs are parsed with `parseFloat` and validated against existing server-side `validDimensionCM` bounds before submission. The SVG `d` attribute interpolation in `GardenCanvas.tsx` uses `drawnGridCm`, which is produced by `visibleGridStepCm` and is guarded against non-finite / non-positive values — it is not raw user input.
- No SSRF, deserialization, secret leakage, or insecure defaults introduced.
- The soft `gridTooFine` warning is UI-only and does not weaken server-side validation.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I reviewed the change through the **correctness** lens and verified the actual checked-out code for the key files (`web/src/lib/units.ts`, `web/src/lib/geometry.ts`, `web/src/components/gardens/GardenFormModal.tsx`, `web/src/editor/GardenCanvas.tsx`, and the test files). All verifications confirm the following:
- **Exact conversions are correct.** `cmFromFtIn`, `cmFromMeters`, and `cmFromSpacing` now use `roundCm(cm * 1e6) / 1e6`, which correctly removes IEEE-754 representation dust (e.g., `12 * 2.54 = 30.479999999999997` becomes `30.48`) without throwing away meaningful precision. The conversion factors (`2.54`, `12`, `100`) are applied correctly.
- **Coarsening logic is correct.** `visibleGridStepCm` returns the smallest multiple of the true grid (`2×`, `5×`, `10×`, …) whose cells exceed the legibility floor. Every returned line is still a real grid line, and snapping continues to use the true `gridCm`, so snapped positions are always a superset of drawn lines. Degenerate cases return `null` as documented.
- **Warning logic is correct.** The soft-warning threshold (`MIN_GARDEN_GRID_CM = 10`) correctly identifies sub-10 cm grids. The warning only fires for finite, positive values below the threshold, and the displayed text uses the same `formatCm` helper used elsewhere.
- **Grid note logic is correct.** The canvas note explains coarsening when `drawnGridCm !== gridCm` and explains an undrawable grid only when `snapToGrid` is active (the state where user confusion is highest). The `!= null` check properly handles the `null` return from `visibleGridStepCm`.
The only side effect is that pre-existing imperial garden data that was quantized to whole centimeters will display slightly differently and may shift by sub-centimeter amounts on a no-op re-save. This is an explicit and accepted consequence of fixing the systematic quantization bug; the PR description states that existing gardens are intentionally not migrated.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
Through the code-cleanliness lens, this PR is well-structured:
- It **removes a duplicated conversion path** (`convertGrid` / `cmFromSpacing` / `spacingFromCm` / `spacingUnitLabel`) from `GardenFormModal.tsx` and consolidates grid handling onto the same dimension-scale helpers used for width and height. That reduces future maintenance burden when unit logic changes.
- The new `visibleGridStepCm` helper in `geometry.ts` is cleanly factored out with a clear docstring and a single, testable responsibility. The coarsening logic is decoupled from React rendering, which keeps `GardenCanvas.tsx` readable.
- `roundCm` in `units.ts` is a sensible private helper that replaces three previously copy-pasted `Math.round(...)` calls, centralizing the “dust-removal” policy.
- Comments in both `GardenCanvas.tsx` and `geometry.ts` explain *why* the coarsening exists and what the two `gridNote` states mean, which prevents the next reader from re-introducing a fade-out or silently drawing nothing.
No dead code, no confusing names, no overly long or deeply nested functions, and no patterns that deviate from the surrounding codebase in a way that would trip up future maintainers.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
- **`visibleGridStepCm` is a trivial fixed-bound loop** (at most 18 iterations of multiplies/compares). Called on every viewport update, but the total work is ~40 arithmetic operations — negligible compared to the surrounding React reconciliation and SVG paint.
- **Pattern re-render frequency actually *decreases***. The old code continuously mutated `strokeOpacity` on every zoom frame, forcing the browser to re-paint the pattern. The new code only changes the pattern dimensions at discrete coarsening thresholds, and opacity is now constant. This is a net win for the compositor.
- **Unit conversions are cold-path**. `roundCm` adds two multiplies and a `Math.round` versus the old single `Math.round`, but these functions are only hit on form input and initialization, never inside drag/animation loops.
- **No new allocations in hot paths**. The `gridNote` string and `formatCm` call only execute when the coarsening level actually changes; the `bedGrid` memo is unchanged.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**Verdict:** No material issues found
After reviewing the diff through the error-handling & edge-cases lens, every guard, null/NaN check, and boundary condition appears correctly implemented:
- `GardenFormModal.tsx:135-136` — `gridTooFine` safely short-circuits on `NaN` / non-finite / non-positive input via `Number.isFinite(gridEntered) && gridEntered > 0` before comparing against `MIN_GARDEN_GRID_CM`. Verified by reading the file.
- `units.ts:29-31` (`roundCm`) — rounds to 1e-6 cm, well below any garden-relevant precision, and handles the IEEE-754 dust case (12 × 2.54) without introducing overflow/underflow for the valid dimension range [1, 10000]. Verified.
- `geometry.ts:117-128` (`visibleGridStepCm`) — returns `null` on degenerate inputs (`gridCm <= 0`, `scale <= 0`, `minCellPx <= 0`) and after exhausting the coarsening cap, preventing silent no-op drawing. The caller (`GardenCanvas.tsx`) surfaces the `null` with an explanatory label rather than drawing nothing. Verified by reading both files.
- Canvas grid rendering (`GardenCanvas.tsx:221`) — uses strict `drawnGridCm != null` before rendering the `<pattern>`, so no invalid SVG is produced for degenerate grids. Verified.
- `snapValue` and `snapLocalToBedGrid` already tolerate `step <= 0` by returning the original value, so disabling snapping never produces `NaN` or crashes. Verified by reading `geometry.ts`.
No ignored errors, missing clean-up, panics on bad input, or unhandled edge cases were found in this diff.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Five findings, all fair:
The soft-minimum hint contradicted itself. "did you mean ft?" made sense when
the grid field was labelled inches sitting under a feet field — which is exactly
what this PR removed. The remaining failure mode is different: someone reaching
for plant spacing, which belongs to the bed, not the garden. The hint now says
that, and points at where the bed grid actually lives. The stale "1 typed
meaning one foot" comments went with it.
gridNote was a three-level nested ternary; it is now gridNoteFor, with the same
logic in explicit branches and the reasoning in one doc comment rather than
threaded between the arms.
cmFromDisplay(parseFloat(gridSize), unit) was computed once for the hint and
again in the submit handler. Hoisted to a single per-render value so the two
can't disagree about what was entered.
roundCm's comment called the 1e-6 cm grain "1 nanometer"; it is 10 nm. Corrected
— the constant was always fine, only the comment was wrong.
Extracted the duplicated overlay-badge class string.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The best of them is the one only opencode/glm-5.2 caught: the soft-minimum hint contradicted itself. "did you mean ft?" was written against the old labelling, where the grid field said inches while sitting under a feet field — the exact thing this PR removed. With the field already in feet the question is nonsense.
The remaining failure mode is a different one: someone reaching for plant spacing, which belongs to the bed, not the garden. The hint now says that and points at where the bed grid actually lives (Bed grid in the inspector), which is the confusion this whole issue is really about. The stale "1 typed meaning one foot" comments went with it.
The other four:
gridNote nested ternary (2/5) → extracted gridNoteFor(...), same logic in explicit branches, reasoning in one doc comment instead of threaded between the arms.
cmFromDisplay computed twice → hoisted to one per-render value shared by the hint and the submit handler, so they can't disagree about what was entered.
roundCm comment said "1 nanometer" → it's 10 nm (1e-6 cm). Comment-only; corrected. Good catch — the constant was always fine.
Duplicated badge className → OVERLAY_BADGE_CLASS.
npm test (39), tsc --noEmit, npm run build, go test ./... all green.
All five Gadfly findings addressed in `1cf1e0d`.
The best of them is the one only `opencode/glm-5.2` caught: **the soft-minimum hint contradicted itself.** "did you mean ft?" was written against the *old* labelling, where the grid field said inches while sitting under a feet field — the exact thing this PR removed. With the field already in feet the question is nonsense.
The remaining failure mode is a different one: someone reaching for *plant* spacing, which belongs to the bed, not the garden. The hint now says that and points at where the bed grid actually lives (`Bed grid` in the inspector), which is the confusion this whole issue is really about. The stale "1 typed meaning one foot" comments went with it.
The other four:
- **`gridNote` nested ternary** (2/5) → extracted `gridNoteFor(...)`, same logic in explicit branches, reasoning in one doc comment instead of threaded between the arms.
- **`cmFromDisplay` computed twice** → hoisted to one per-render value shared by the hint and the submit handler, so they can't disagree about what was entered.
- **`roundCm` comment said "1 nanometer"** → it's 10 nm (1e-6 cm). Comment-only; corrected. Good catch — the constant was always fine.
- **Duplicated badge className** → `OVERLAY_BADGE_CLASS`.
`npm test` (39), `tsc --noEmit`, `npm run build`, `go test ./...` all green.
steve
merged commit 8a12069118 into main2026-07-21 04:50:49 +00:00
steve
deleted branch fix/units-correctness2026-07-21 04:50:49 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #47. Phase 0 of #58, landing first and alone as planned.
Three compounding defects in the measurement layer, all measured against the live Home garden rather than estimated.
1. Entry was quantized to whole centimeters
Every measurement column is
REALand the server'svalidDimensionCMchecks range and finiteness only — the rounding was a client-side choice the schema never asked for. A 12 in grid stored 30 cm, i.e. an 11.811″ grid: every cell short by 0.189″, and because the error is systematic it accumulates to 4.61″ across the 24 ft garden.cmFromFtIn/cmFromMeters/cmFromSpacingnow convert exactly. They round at 1e-6 cm — enough to clear IEEE-754 dust (12 * 2.54is30.479999999999997) and ~4 orders of magnitude below anything a garden cares about. Display-side rounding is untouched, so cm quantization still can't show through in a field.2. The garden grid was entered at spacing scale
Grid size (in)sat directly beneathWidth (ft)— two adjacent numeric fields, two different scales, distinguished only by a suffix. Home ended up withgridSizeCm: 3, almost certainly "1" typed meaning one foot.The garden grid is a layout concern, so it moves to dimension scale and reads
Garden grid (ft). The bed grid in the object inspector is a plant-spacing concern and stays atin/cm— the labels are now unambiguous rather than uniform, which is the honest resolution given they really are different scales.A sub-10 cm garden grid draws a soft warning (
That's a 0′ 1″ grid for the whole garden — did you mean ft?) rather than being silently accepted. Soft on purpose: a fine grid is legitimate for a tiny garden.3. Snap stayed active when the grid stopped drawing
The canvas faded the grid below 6 px per cell. At 3 cm and ~0.84 px/cm that's 2.5 px → opacity 0, so Home ran with
snapToGrid: true, no grid drawn, and no snapping you could feel (its bed coordinates are values like80.90185676392574).Replaced the fade with coarsening, the issue's preferred option.
visibleGridStepCmpicks the smallest multiple of the true grid (2×, 5×, 10×…) whose cells clear the legibility floor, so every line drawn is still a real grid line and something aligned to the grid is always visible. Snapping continues to use the true grid — the drawn lines are a subset of the snap positions, never a different grid. When the drawn step isn't the true grid the canvas saysgrid every 5′ 0″; a degenerate grid it can't draw at any multiple says that rather than silently drawing nothing.Verification
npm test— 39 passing, including exact-conversion cases, adisplay → cm → displayidentity table for both unit systems, and coarsening cases covering the exact 3 cm @ 0.84 px/cm failure.go test ./...,tsc --noEmit,npm run buildall green.3stays3and now displays honestly as0.1 ft, with the warning explaining it.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-21 04:44:18Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -136,1 +132,4 @@const unitLabel = dimensionUnitLabel(unit)// Soft floor: warn, don't refuse. A sub-4″ garden grid is almost always "1"// typed meaning one foot, but it's a legitimate choice for a tiny garden.const gridEntered = cmFromDisplay(parseFloat(gridSize), unit)🟡 cmFromDisplay(parseFloat(gridSize), unit) duplicated between warning calc and submit handler
maintainability · flagged by 1 model
web/src/editor/GardenCanvas.tsx:303&:307— the exact same badge className string (rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur) is duplicated across the two<span>s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g.const badgeClass = '...') would keep the two badges in sync if the styling ever changes. Trivial. -web/src/editor/GardenCanvas.tsx:118—gridNoteis a three-level nested ter…🪰 Gadfly · advisory
@@ -199,1 +202,3 @@</label>{gridTooFine && (<p className="mt-1 text-xs text-muted">That's a {formatCm(gridEntered, unit)} grid for the whole garden — did you mean {unitLabel}?🟠 Soft-warning 'did you mean {unitLabel}?' is self-contradictory now that the grid field is already in that unit; the '1 typed meaning one foot' comments are stale under the new dimension scale.
maintainability · flagged by 1 model
web/src/components/gardens/GardenFormModal.tsx:204— The soft-warningdid you mean {unitLabel}?is self-contradictory after the scale migration. The grid field is now labeledGarden grid (ft)/(m)(i.e.unitLabel), so when the warning fires it asks "did you mean ft?" while the user is already typing in a ft field. The warning only made sense when the grid was at spacing scale (in/cm) and the suggestion was to switch to the dimension unit. Under the new dimension scale, the "typed '1…🪰 Gadfly · advisory
@@ -105,0 +115,4 @@const drawnGridCm = visibleGridStepCm(gridCm, viewport.scale, GRID_MIN_CELL_PX)// Two states worth explaining rather than leaving the user to infer: lines that// are every Nth grid cell, and (degenerate) a grid too fine to draw at all.const gridNote =🟡 3-level nested ternary for gridNote is harder to scan/modify than an if/else or small helper
maintainability · flagged by 2 models
web/src/editor/GardenCanvas.tsx:118-125—gridNoteis a 3-level nested ternary (drawnGridCm == null→garden.snapToGrid→drawnGridCm !== gridCm) picking between three string/null outcomes. It's well-commented, but nested ternaries like this are harder to scan and riskier to extend than an early-return helper or plain if/else. Suggest pulling it into a small named function, e.g.gridNoteFor(drawnGridCm, gridCm, snapToGrid, unit), same logic with explicit branches.🪰 Gadfly · advisory
@@ -281,2 +302,2 @@<div className="pointer-events-none absolute left-3 top-3 rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur">{viewport.scale.toFixed(2)} px/cm<div className="pointer-events-none absolute left-3 top-3 flex flex-col items-start gap-1"><span className="rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur">⚪ Duplicated badge className string across two overlay spans
maintainability · flagged by 1 model
web/src/editor/GardenCanvas.tsx:303&:307— the exact same badge className string (rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur) is duplicated across the two<span>s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g.const badgeClass = '...') would keep the two badges in sync if the styling ever changes. Trivial. -web/src/editor/GardenCanvas.tsx:118—gridNoteis a three-level nested ter…🪰 Gadfly · advisory
@@ -21,2 +25,2 @@export function cmFromFtIn(feet: number, inches = 0): number {return Math.round((feet * INCHES_PER_FOOT + inches) * CM_PER_INCH)/** Round away IEEE-754 dust without rounding away precision: 12 × 2.54 is* 30.479999999999997 in binary floating point, and 1 nanometer is far below⚪ roundCm's doc comment misstates the rounding grain as 1 nm when it's actually 10 nm (1e-6 cm = 10 nm)
correctness · flagged by 1 model
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 5 findings (1 with multi-model agreement)
web/src/editor/GardenCanvas.tsx:1184 single-model findings (lower confidence)
web/src/components/gardens/GardenFormModal.tsx:204web/src/components/gardens/GardenFormModal.tsx:135web/src/editor/GardenCanvas.tsx:303web/src/lib/units.ts:26Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens (injection, XSS, SSRF, deserialization, secrets, input validation, authz).
This PR is confined to client-side arithmetic and display logic for unit conversion (
units.ts,geometry.ts) and two React components that render the results. I verified:cmFromFtIn,cmFromMeters,cmFromSpacing,cmFromDisplay,roundCm,visibleGridStepCm,formatCm) are pure numeric functions — no string concatenation into SQL, shell, template, ordangerouslySetInnerHTML.formatCm(...)output is rendered as plain JSX text content (GardenFormModal.tsx:204,GardenCanvas.tsx:305), which React escapes automatically. The SVG patternid(gridId,GardenCanvas.tsx:69) is unchanged by this diff and derived from React'suseId(), not user input.onSubmitstill runsisValidDimensionCmagainst the convertedwidthCm/heightCm/gridSizeCmbefore submitting (GardenFormModal.tsx:83-93), matching the server's documented[1cm, 100m]bounds (units.ts:11). The newMIN_GARDEN_GRID_CMcheck is a soft UI warning only (GardenFormModal.tsx:136,202-206) and doesn't weaken that hard validation — it's additive.The
roundCmprecision fix and the grid/dimension-scale rework don't touch authn/authz, don't introduce new external calls, and don't loosen any existing validation.🎯 Correctness — Minor issues
Review
VERDICT: Minor issues
I independently re-derived the conversion math and re-read
web/src/lib/units.tsandweb/src/lib/geometry.tsagainst the diff.web/src/lib/units.ts:25-28— theroundCmdoc comment claims "1 nanometer is far below anything a garden cares about," but the actual rounding grain is1e-6 cm, which is 10 nanometers, not 1 (1 cm = 1e7 nm, so1e-6 cm = 1e-6 × 1e7 nm = 10 nm). Confirmed:roundCmis defined exactly asMath.round(cm * 1e6) / 1e6(units.ts:29-31), and the "1 nanometer" phrasing sits on line 26 of that same doc comment. This is a comment-only inaccuracy — the rounding constant itself (1e-6 cm, i.e. sub-micron) is still functionally fine for garden-scale dimensions — with no behavioral effect on any of the conversion functions.🧹 Code cleanliness & maintainability — Minor issues
Confirmed — the "no dead code" claim and
unitPreffield both check out. The draft's single finding is legitimate and verifiable in the source. Here's the corrected review:VERDICT: Minor issues
Lens: 🧹 Code cleanliness & maintainability
web/src/editor/GardenCanvas.tsx:118-125—gridNoteis a 3-level nested ternary (drawnGridCm == null→garden.snapToGrid→drawnGridCm !== gridCm) picking between three string/null outcomes. It's well-commented, but nested ternaries like this are harder to scan and riskier to extend than an early-return helper or plain if/else. Suggest pulling it into a small named function, e.g.gridNoteFor(drawnGridCm, gridCm, snapToGrid, unit), same logic with explicit branches.Everything else checked out clean:
roundCm(web/src/lib/units.ts:29) is a tightly-scoped private helper with a comment explaining why (IEEE-754 dust vs. real precision loss).spacingFromCm/spacingUnitLabel/cmFromSpacing/formatSpacingwere dropped fromGardenFormModal.tsx's imports but remain live — confirmed via grep they're still used byInspector.tsx,PlopInspector.tsx,PlantFormModal.tsx,PlantCard.tsx, andPlantPicker.tsxfor the (intentionally separate) bed-grid/plant-spacing scale.GRID_MAX_OPACITY→GRID_OPACITYrename inGardenCanvas.tsxcorrectly reflects it's no longer a max (fading was replaced by coarsening).convertGrid/convertconsolidation inGardenFormModal.tsx:57-68actually removes prior duplication rather than adding any, now that the grid uses the same dimension-scale helper as width/height.visibleGridStepCm(web/src/lib/geometry.ts:117-128) is compact, well-commented, and has matching test coverage ingeometry.test.ts.No other maintainability issues in this lens. Outside my lens: correctness/test coverage looked solid on read-through, but that's a different reviewer's job.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls).
visibleGridStepCm(web/src/lib/geometry.ts:117) runs on everyGardenCanvasrender — including the high-frequency re-renders during a live object/plop drag, per the comment atGardenCanvas.tsx:201. It's an unmemoized nested loop, but bounded to at most6 × 3 = 18iterations of trivial multiply/compare — negligible next to the SVG re-render work already happening on the same path (verified by reading the full function; no unbounded loop, no allocation inside it).bedGridLines/BED_GRID_MAX_LINEScap is unchanged by this diff (GardenCanvas.tsx:27,38-43) — the new grid-coarsening logic doesn't touch the per-bed line generation, so the existing 200-line safety cap still applies there.gridEntered/gridTooFinecomputation inGardenFormModal.tsx:135-136re-parsesgridSizeon every render, but it's a singleparseFloat+ arithmetic on a form with a handful of fields — not a hot path.roundCm(units.ts:29-31) adds oneMath.roundper conversion call; conversions happen only on user input/unit-toggle events, not in any loop or render path.No N+1s, no missing pagination, no quadratic behavior, no new blocking calls — this PR is UI/unit-conversion logic with no data-fetching or bulk-processing surface. Nothing here rises to a performance finding.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for error handling and edge cases: NaN/invalid input propagation, boundary conditions in the new unit-conversion math, and the new grid-coarsening logic's degenerate cases.
What I checked (verified by reading the actual code, not just the diff):
roundCm/cmFromFtIn/cmFromMeters/cmFromSpacing(web/src/lib/units.ts:29-98): NaN/negative/zero inputs flow through untouched and are caught downstream byisValidDimensionCm'sNumber.isFinite+ range check before ever reaching the server payload. ConfirmedisValidDimensionCmis applied togridSizeCmat submit (GardenFormModal.tsx:89-93), not just width/height.gridTooFinewarning (GardenFormModal.tsx:135-136): guardsNumber.isFinite(gridEntered) && gridEntered > 0before comparing toMIN_GARDEN_GRID_CM, so an empty/cleared field (parseFloat('') → NaN) or a zero/negative legacy value correctly suppresses the warning rather than rendering "NaN′ NaN″" — and zero/negative are still hard-blocked by the submit-timeisValidDimensionCmcheck either way, so nothing slips through silently.visibleGridStepCm(web/src/lib/geometry.ts:117-128): guardsgridCm > 0,scale > 0,minCellPx > 0up front, so corrupt/legacy grid values of0or negative (exactly the kind of bad data the PR calls out for Home) returnnullrather than dividing or looping unboundedly. Loop is bounded (6 decades × 3 multiples = 18 iterations max), so there's no runaway/infinite loop risk. Boundary case (step * scale >= minCellPx, using>=not>) matches the "exactly at the minimum" test case with no off-by-one.GardenCanvas.tsx:118-125,221:drawnGridCm == nullis handled explicitly (skips rendering the pattern, shows an explanatory note only whensnapToGridis on) — no crash path when the grid can't be drawn at all, unlike the old fade logic that silently produced 0 opacity with snap still active.EditorGarden.unitPrefis a required (non-optional) field inweb/src/editor/types.ts:35, soformatCm(drawnGridCm, garden.unitPref)isn't reading from a possibly-undefined value.Nothing in this diff introduces an unguarded panic, a swallowed error, or a boundary condition that isn't already defended against by an existing check (
isValidDimensionCm,Number.isFinite, or the newvisibleGridStepCmguards). The validation/warning logic is consistently soft-fail (warn/block via form error) rather than throwing.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
The security lens here covers client-side form validation and unit conversion — no auth/authz, injection, or untrusted-deserialization surface in this diff. The form still submits to the same server-side
validDimensionCMrange check (internal/service/gardens.go:256), which guards NaN/Inf and bounds. The client-side changes are pure numeric conversion/display logic with no taint flow into queries, commands, or paths.I traced the tainted input path:
parseFloat(gridSize)→cmFromDisplay→input.gridSizeCm→create.mutateAsync/update.mutateAsync→ servervalidDimensionCM. No bypass introduced; the server remains the authority. TheroundCmhelper operates only on already-numeric values; no string templating into SQL/HTML. TheformatCm(gridEntered, unit)interpolated into JSX is a number→string formatter, not raw HTML injection (React escapes). Nothing material in my lane.Verdict: No material issues found
internal/service/gardens.go:252-258) still gatesGridSizeCMviavalidDimensionCM(finite + [1cm, 100m]) regardless of the client's new exact-conversion logic — no validation bypass.roundCm/cmFromDisplay/cmFromFtIn/cmFromSpacingare pure numeric transforms overparseFloatoutput; no string interpolation into queries, commands, paths, or templates.formatCm(gridEntered, unit)inGardenFormModal.tsx:204andformatCm(drawnGridCm, ...)inGardenCanvas.tsx:124render through JSX (auto-escaped), notdangerouslySetInnerHTML— no XSS vector.🎯 Correctness — No material issues found
Verdict: No material issues found.
I verified the change through the correctness lens — re-deriving the unit conversions, the coarsening math, and the round-trip identities against the checked-out code.
web/src/lib/units.ts:29-41, 97-99):roundCm=Math.round(cm*1e6)/1e6. Re-derived:12 * 2.54 = 30.48(the IEEE dust30.479999999999997rounds back to30.48);1 ft → 30.48;4'6" → 137.16;0.305 m → 30.5 cm. All match the tests and first principles. The 1e-6 cm floor is ~1 nm — far below garden precision and below the display rounding (0.1 ft ≈ 3 cm, 0.01 m = 1 cm), so it cannot show through in a field.displayFromCm ∘ cmFromDisplay): For imperial,displayFromCm=round(roundCm(ft*30.48)/30.48 * 10)/10. For the tested ft values (0.5, 1, 2.5, 4, 8, 12.5, 24) this returns ft exactly. For metric,round(roundCm(m*100))/100returns m exactly for the tested set. Confirmed by readingunits.ts:44-57.GardenFormModal.tsx:49, 89, 122): grid now usescmFromDisplay/displayFromCm(dimension scale, ft/m) instead ofcmFromSpacing/spacingFromCm(cm/in). The bed grid inInspector.tsxstill uses the spacing helpers (Inspector.tsx:54,71,98-100), correctly untouched. No cross-file caller of the removedcmFromSpacing/spacingUnitLabelimports inGardenFormModalremains (grep confirms the form no longer references them).geometry.ts:117-128):visibleGridStepCmreturns the smallestgridCm * decade * m(m ∈ {1,2,5}) withstep*scale >= minCellPx. Re-derived the test cases:3 @ 0.84 → 15(2.52, 5.04 < 6; 12.6 ≥ 6),1 @ 0.84 → 10(0.84…4.2 < 6; 8.4 ≥ 6),10 @ 0.6 → 10(exactly 6). Decade cap: d=0..5 → decade ∈ {1,10,…,1e5}, max step = 5e5·gridCm, matching the "500,000×" comment. Every returned step is a whole multiple ofgridCm, so drawn lines are a subset of snap positions — confirmed snapping still usesgridCmatGardenCanvas.tsx:155, notdrawnGridCm.GardenFormModal.tsx:133-136):MIN_GARDEN_GRID_CM = 10cm = 3.937″ ≈ the "sub-4″" comment.gridEnteredis NaN-safe (Number.isFiniteguard). Re-derived Home's case:displayFromCm(3, 'imperial') = round(0.9843)/10 = 0.1ft (field shows 0.1), andformatCm(3, 'imperial') = "0′ 1″"(warning text) — both match the PR description. Typing "1" in the new ft field → 30.48 cm, correctly not warned.No logic bugs, incorrect constants, or semantic errors found within the correctness lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues found
Reviewed through the cleanliness/maintainability lens. The change is well-structured overall: the spacing helpers were correctly left in place for the bed-grid/plant-spacing callers, the
convertGridspecial-case was collapsed into the sharedconvertonce the grid moved to dimension scale, andvisibleGridStepCmis a clean, self-contained extraction with a good doc comment. A few small nits:web/src/editor/GardenCanvas.tsx:303&:307— the exact same badge className string (rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur) is duplicated across the two<span>s in the overlay. It's only two instances, but it's a long, magic string now repeated verbatim; a small local const (e.g.const badgeClass = '...') would keep the two badges in sync if the styling ever changes. Trivial.web/src/editor/GardenCanvas.tsx:118—gridNoteis a three-level nested ternary (drawnGridCm == null ? … : drawnGridCm !== gridCm ? … : null). It's readable as written and matches the "two states worth explaining" comment, but a smallif/early-return helper or a lookup would flatten it. Not worth churning on its own. Trivial.web/src/components/gardens/GardenFormModal.tsx:135—cmFromDisplay(parseFloat(gridSize), unit)is computed inline on every render for the warning, and the same expression reappears in the submit handler (:89). The two are in different scopes so extracting isn't strictly necessary, but a sharedconst gridEnteredCmderived once would remove the duplication. Small.No dead code, no leaky abstractions, no patterns broken relative to the surrounding files. The
roundCmhelper is a nice single-source replacement for the scatteredMath.roundcalls. Nothing here is blocking.⚡ Performance — No material issues found
No material issues found.
Performance lens: the change replaces a fade-opacity computation with
visibleGridStepCm, both O(1) per render — the latter does at most ~18 iterations of trivial arithmetic (3 multipliers × 6 decades), strictly cheaper than nothing material. The gridpattern/rectis only re-rendered whendrawnGridCmis non-null, same gating as before. The newgridTooFinecheck in the form is a couple of parseFloats on keystroke (form modal, not hot path). TheformatCmcalls ongridNoteonly fire when the note is shown. No N+1, no unbounded growth, no new per-frame allocation (theuseMemos are unchanged).GRID_MAX_COARSEN_DECADES = 5gives a 500,000× cap that bounds the loop unconditionally. Nothing here is a regression.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling / edge-case lens, the change is careful. Verified by reading the actual checked-out code:
visibleGridStepCm(geometry.ts:117-128) guards all degenerate inputs with!(x > 0), which correctly rejectsNaN,0, and negatives forgridCm,scale, andminCellPx(sinceNaN > 0isfalse,!(false)→ returnsnull). The coarsening loop is bounded (d <= GRID_MAX_COARSEN_DECADES), so no unbounded iteration, and thenullresult is surfaced by the caller as an explicit "grid too fine to draw — zoom in" note rather than swallowed. The degenerate cases are even unit-tested (geometry.test.ts).GardenFormModalwarning path (GardenFormModal.tsx:135-136):gridEntered = cmFromDisplay(parseFloat(gridSize), unit).parseFloatcan returnNaN(empty/garbage input) or a negative;gridTooFineguards withNumber.isFinite(gridEntered) && gridEntered > 0 && gridEntered < MIN_GARDEN_GRID_CM, soNaN/negative/zero don't produce a spurious warning. The0andNaNcases are instead caught at submit byisValidDimensionCm(which checksNumber.isFiniteand range) with a clear form error — not swallowed.cmFromDisplay→isValidDimensionCm, which rejects non-finite and out-of-range values with explicit messages; no uncheckedparseFloatflows into the mutation.drawnGridCm != null, consistent withvisibleGridStepCm's null contract — a null (degenerate grid) correctly skips the<pattern>/<rect>render rather than rendering withwidth={null}/NaN.roundCmhelper (units.ts:29-31) operates on finite products of well-bounded inputs; no overflow concern at the magnitudes involved (max ~10000 cm).No unhappy-path regressions introduced. The
Infinityinput case forvisibleGridStepCmis theoretically unguarded, butgridCmis server-bounded to[1, 10000]andviewport.scaleis a real zoom factor, so it is not reachable here — not a material finding.kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The PR is a units-correctness refactor with no security-relevant changes. All user inputs (grid size, width, height) continue to be:
parseFloat(same as before),isValidDimensionCm(range 1–10,000 cm, finiteness check),validDimensionCM(identical range andmath.IsNaN/math.IsInfguards).The new soft warning for sub-10 cm garden grids renders via React JSX with
formatCm, which returns plain numeric strings—no HTML injection path. No secrets, credentials, SQL, commands, paths, or SSRF surfaces are touched.🎯 Correctness — No material issues found
No material issues found
I scrutinized the change through the Correctness lens and found the mathematics sound:
roundCmcorrectly clears IEEE-754 dust at 1e-6 cm (~10 nm) without clipping garden-meaningful precision.cmFromFtIn(0, 12)→30.48,cmFromMeters(0.305)→30.5, etc. all verified.cmFromFtIn,cmFromMeters, andcmFromSpacingis correct: the server storesREAL, so rounding to whole cm was a client-side bug. Display-side rounding indisplayFromCm/spacingFromCmremains unchanged, preserving clean UI round-trips.visibleGridStepCmcoarsening logic verified: it returns the smallest 1×/2×/5×/10×… multiple of the true grid that clearsminCellPx, andnullonly when even 500,000× falls short. Snapping continues to use the true grid, so drawn lines are always a subset of snap positions — never a different grid.MIN_GARDEN_GRID_CM = 10soft floor warns without refusing, which matches the issue description.PlantFormModal,Inspector, andPlopInspectorstill usecmFromSpacingfor plant spacing; onlyGardenFormModalswitched tocmFromDisplayfor the garden grid.Tests cover the exact-conversion identity table, the #47 coarsening case (3 cm @ 0.84 px/cm → 15 cm), and degenerate-grid null behavior. No logic bugs detected.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
I reviewed all six changed files through the maintainability lens:
units.ts: The newroundCmhelper consolidates IEEE-754 dust rounding into one place instead of repeating it acrosscmFromFtIn,cmFromMeters, andcmFromSpacing. TheMIN_GARDEN_GRID_CMconstant is well-commented and clearly named.geometry.ts:visibleGridStepCmis a clean, focused function with a clear docstring. The coarsening constants are module-local and self-documenting.GardenFormModal.tsx: Removing the separateconvertGridclosure and reusing the existingconvertfunction reduces duplication and makes the unit-switching path simpler. The warning markup is a straightforward conditional block.GardenCanvas.tsx: Replacing the opacity math with a semanticdrawnGridCm != nullcheck is clearer. ThegridNoteternary, while nested, is small and directly follows the two states the function returns.No dead code, no confusing names, no overly long functions, no copy-paste that should be shared, and no leaky abstractions.
⚡ Performance — No material issues found
No material issues found
From a performance lens, the changes are clean:
visibleGridStepCm(geometry.ts:117) is a bounded loop (≤15 iterations of simple arithmetic) called once perGardenCanvasrender. It replaces continuous opacity interpolation with discrete step changes, which is neutral-to-positive for rendering cost during zoom.<pattern>is only recreated whendrawnGridCmactually changes; between threshold crossings React reconciles it away. No excess DOM churn.formatCmandcmFromDisplaycalls inGardenFormModalare form-field-only, not hot-path.🧯 Error handling & edge cases — No material issues found
No material issues found
Through the 🧯 error-handling & edge-cases lens, the diff is clean. I verified the following:
visibleGridStepCm(geometry.ts:117) guards againstNaN,0, negative, and non-finite inputs with!(gridCm > 0) || !(scale > 0) || !(minCellPx > 0)and returnsnullfor all degenerate cases. The loop is capped at 18 iterations, so there is no runaway or overflow risk.GardenFormModal(GardenFormModal.tsx:135-136) correctly guards the soft-warning check withNumber.isFinite(gridEntered) && gridEntered > 0, so empty/invalid/malformed input, zero, and negative values do not spuriously trigger the “did you mean ft?” message. The downstreamisValidDimensionCmcall still catches invalid values at submit time.roundCm(units.ts:29-31) safely handlesNaNandInfinityby passing them through; the only “gotcha” is sub-nanometer values round to0, but those are rejected byisValidDimensionCmanyway.gridNotelogic inGardenCanvas(GardenCanvas.tsx:118-125) distinguishes the three states (true grid, coarsened grid, degenerate grid) without crashing onnullfromvisibleGridStepCm.No ignored errors, missing cleanup, panics, or unhandled edge cases are introduced by this change.
opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens: input validation, injection, taint tracing, and defaults.
GardenFormModal.tsx:89-93still runsparseFloat→cmFromDisplay→isValidDimensionCm, which checksNumber.isFinite(cm) && cm >= 1 && cm <= 10_000(units.ts:21-22). This mirrors the server'svalidDimensionCMininternal/service/gardens.go:274-275(finite +[minGardenCM, maxGardenCM]). NaN from non-numeric input (parseFloat("abc")→ NaN →cmFromFtIn(NaN)→roundCm(NaN)→ NaN) is rejected by the finite check. Negative values rejected by the>= 1floor. No path where a bad value bypasses both client and server checks.gridTooFinewarning (GardenFormModal.tsx:202-206) and the canvasgridNote(GardenCanvas.tsx). Both feed into React JSX as text children ({formatCm(...)},{gridNote},{unitLabel}), which React escapes; nodangerouslySetInnerHTML.unitLabelis a constant ('ft'/'m') fromdimensionUnitLabel, not user input.formatCm/gridNoteare built from numeric conversions of user input, not raw strings.gardenFromInput(gardens.go:214-268) still re-validatesGridSizeCMviavalidDimensionCMandUnitPrefagainst the metric/imperial allowlist (gardens.go:244-245); the client's new exact-cm conversion doesn't widen what the server accepts (REAL column already accepted fractional cm — the PR description's point).Verification: read
web/src/lib/units.ts(full),web/src/components/gardens/GardenFormModal.tsx(full),internal/service/gardens.go:230-276,web/src/editor/Inspector.tsx:1-101, and grepped forvalidDimensionCM,dangerouslySetInnerHTML/innerHTML/evalin the editor, andUnitMetric/UnitImperialdefinitions. No gaps found in the security lane.🎯 Correctness — No material issues found
I'll review this through the correctness lens, verifying against the actual code.
Now let me check the conversion in changeUnit to verify whether convert handles the case where the existing
widthis non-finite.Let me verify the round-trip identity table and check the
cmFromMeters(1.22)case.Let me re-derive the round-trip claims from the test:
Metric round-trip:
m → cm → mviacmFromMeters(m)thendisplayFromCm(cm, 'metric').cmFromMeters(0.15)=roundCm(0.15 * 100)=roundCm(15.0...)= 15displayFromCm(15, 'metric')=Math.round((15/100)*100)/100=Math.round(15)/100= 0.15 ✓cmFromMeters(1.22)=roundCm(122.0...)= 122displayFromCm(122, 'metric')=Math.round((122/100)*100)/100= 1.22 ✓cmFromMeters(3.05)=roundCm(305.0...)= 305displayFromCm(305, 'metric')= 3.05 ✓But wait —
cmFromMeters(0.305)=roundCm(30.4999...)= 30.5. Test asserts30.5✓.Imperial round-trip:
ft → cm → ftviacmFromFtIn(ft)thendisplayFromCm(cm, 'imperial')(which divides by 30.48 and rounds to 0.1).cmFromFtIn(0.5)=roundCm(6 * 2.54)= 15.24displayFromCm(15.24, 'imperial')=Math.round((15.24/30.48) * 10)/10=Math.round(0.5 * 10)/10= 0.5 ✓cmFromFtIn(2.5)=roundCm(30 * 2.54)= 76.2displayFromCm(76.2, 'imperial')=Math.round((76.2/30.48)*10)/10=Math.round(2.5*10)/10= 2.5 ✓cmFromFtIn(12.5)=roundCm(150 * 2.54)= 381displayFromCm(381, 'imperial')=Math.round((381/30.48)*10)/10=Math.round(12.5*10)/10= 12.5 ✓These check out.
Now let me verify the
visibleGridStepCmcap and the degenerate case:For
visibleGridStepCm(1e-9, 1, 6): needstep * 1 >= 6, i.e.gridCm * decade * m >= 6, so1e-9 * decade * m >= 6. Max decade is1e5(5 iterations of*= 10), max m=5, so1e-9 * 1e5 * 5 = 5e-4— way below 6. Returns null ✓.For
visibleGridStepCm(1, 0.84, 6):1*0.84=0.84 <6;2*0.84=1.68 <6;5*0.84=4.2 <6; next decade10*0.84=8.4 >=6→ returns 10 ✓.For
visibleGridStepCm(3, 0.84, 6):3*0.84=2.52 <6;6*0.84=5.04 <6;15*0.84=12.6 >=6→ returns 15 ✓.Now let me verify the concern about the
gridTooFinewarning. The check isgridEntered > 0 && gridEntered < MIN_GARDEN_GRID_CM(10 cm). But the warning text interpolatesformatCm(gridEntered, unit). Let me check the warning text for the actual problem case described in the PR: a 1 ft entry would be 30.48 cm, which is NOT < 10 cm. So the warning only fires for sub-10cm entries. Let me check the PR text claims and the comment.I have enough to render the verdict. Let me check one thing — whether the round-trip assertion could fail at boundary cases due to
displayFromCmrounding at the edge. The test uses[0.5, 1, 2.5, 4, 8, 12.5, 24]. Let me checkcmFromFtIn(4)=roundCm(4*12*2.54)=roundCm(121.92)= 121.92;displayFromCm(121.92, 'imperial')=Math.round((121.92/30.48)*10)/10=Math.round(4*10)/10= 4 ✓.cmFromFtIn(8)= 243.84; display =Math.round(8*10)/10= 8 ✓.cmFromFtIn(24)=roundCm(24*12*2.54)=roundCm(731.52)= 731.52; display =Math.round(24*10)/10= 24 ✓.All checks pass. I verified the changes against the actual code — the round-trip identity, the conversion exactness, the
visibleGridStepCmcoarsening and null cases, and the snap-uses-true-grid invariant.Review
Verdict: No material issues found
I verified the correctness of all three changes against the actual checked-out code:
Exact conversions (
web/src/lib/units.ts:29-99):roundCm=Math.round(x*1e6)/1e6correctly clears IEEE-754 dust (re-derived:12 * 2.54 = 30.479999999999997→Math.round(30479999.99999997)/1e6 = 30.48✓). The round-trip identity table (units.test.ts:38-46) holds for the asserted values: I traced each imperial value (0.5, 1, 2.5, 4, 8, 12.5, 24) and metric value (0.15, 1, 1.22, 3.05, 10, 24.5) throughcmFromDisplay → displayFromCmand every one round-trips exactly within the display precision (0.1 ft / 0.01 m). The 1e-6 cm granularity is ~4 orders below garden precision, as claimed.Garden grid at dimension scale (
GardenFormModal.tsx:48-67, 89, 122, 135): The grid now usescmFromDisplay/displayFromCmconsistently — entry, conversion, conflict-rebase, and save all use the same helpers as width/height. Theconvertclosure inchangeUnitis unit-preserved correctly (closes overunit, the current unit, converting tonext). The soft-warning floorMIN_GARDEN_GRID_CM = 10cm ≈ 4″ matches the comment "sub-4″";gridTooFinerequires> 0and< 10, so a legitimate 1 ft grid (30.48 cm) correctly does NOT trigger the warning, only true mis-entries do.Coarsening instead of fade (
geometry.ts:117-128,GardenCanvas.tsx:114-125, 221-225): I re-derived the key cases:visibleGridStepCm(3, 0.84, 6)→ tries3*0.84=2.52,6*0.84=5.04,15*0.84=12.6 ≥ 6→ returns 15 ✓. The drawn lines (drawnGridCm) are always a whole multiple ofgridCm(gridCm * decade * mwhere decade is a power of 10 and m ∈ {1,2,5}), so every drawn line is a real snap position — the "subset of snap positions, never a different grid" invariant holds. Snapping inGardenCanvas.tsx:155still usesgridCm(the true grid), notdrawnGridCm— verified the snap path is unchanged. Degenerate cases (0, negative, NaN, scale 0, sub-nanometer grid beyond the 5-decade cap) all returnnull, and the canvas renders the "grid too fine to draw" note instead of drawing nothing.Pattern/grid alignment: The SVG
<pattern>usesdrawnGridCm(a multiple ofgridCm), and the pattern is anchored at the SVG origin which is the garden's (0,0) world origin — same anchor assnapPoint— so drawn lines coincide with snap positions.No correctness defects found within my lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/components/gardens/GardenFormModal.tsx:204— The soft-warningdid you mean {unitLabel}?is self-contradictory after the scale migration. The grid field is now labeledGarden grid (ft)/(m)(i.e.unitLabel), so when the warning fires it asks "did you mean ft?" while the user is already typing in a ft field. The warning only made sense when the grid was at spacing scale (in/cm) and the suggestion was to switch to the dimension unit. Under the new dimension scale, the "typed '1' meaning one foot" misentry the comments describe cannot produce a too-fine grid:cmFromDisplay(1, 'imperial') = cmFromFtIn(1) = 30.48 cm, well aboveMIN_GARDEN_GRID_CM = 10(and even above the "sub-4″" ~10.16 cm the comment invokes), so the warning never fires for that case — it only fires for genuinely tiny values (e.g.0.1 ft→ 3.048 cm,0.05 m→ 5 cm), where asking "did you mean the unit you're already using?" is nonsensical. The comment atGardenFormModal.tsx:133-134("A sub-4″ garden grid is almost always '1' typed meaning one foot") and theMIN_GARDEN_GRID_CMcomment atweb/src/lib/units.ts:15-17("typing '1' in a feet field meaning one foot, and landing on a 1 cm grid") both repeat the same impossible scenario — typing1in a ft field yields 30.48 cm, not 1 cm. Suggested fix: rewrite the warning to match the new scale (e.g. describe the actual value without the "did you mean" unit prompt, or rethink what misentry it's guarding against), and update both comments so they don't describe a misentry the new scale can't produce.Verified against
units.ts(cmFromFtIn(1) = 30.48,dimensionUnitLabelreturnsft/m,MIN_GARDEN_GRID_CM = 10,cmFromDisplaymaps throughcmFromFtIn/cmFromMeters) andGardenFormModal.tsx(field labelGarden grid (${unitLabel}),gridEntered = cmFromDisplay(parseFloat(gridSize), unit), warning uses the sameunitLabel).⚡ Performance — No material issues found
VERDICT: No material issues found
visibleGridStepCm(web/src/lib/geometry.ts:117) runs on every render ofGardenCanvas(it's called unconditionally atGardenCanvas.tsx:115). It's a pure loop bounded atGRID_COARSEN_MULTIPLES.length × (GRID_MAX_COARSEN_DECADES + 1)= 3 × 6 = 18 iterations, no allocation, early return on first match — negligible and not a hot loop concern (re-renders are pointer-driven, not per-frame animation). Verified by reading the function body.<rect>are rendered via a single SVG<pattern>(one fill), so coarsening does not increase line count or DOM nodes — it actually reduces paint work when zoomed out (largerdrawnGridCm→ fewer tile repetitions). Confirmed atGardenCanvas.tsx:221-228.gridNoteallocation (a short string ornull) is once per render and trivial.cmFromFtIn/cmFromMeters/cmFromSpacingnow go throughroundCm(oneMath.round+ one divide) instead of oneMath.round— not a measurable regression; these run on user input, not in loops.GardenFormModal'sgridEntered/gridTooFineare computed on every render of the modal, but the modal is not a hot path and the work is twoparseFloats and a comparison.No N+1, unbounded growth, missing limits, or avoidable quadratic behavior was introduced. The change is performance-neutral-to-positive (coarsening reduces paint load when zoomed out).
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I verified the error-handling and edge-case behavior of every changed function against the checked-out code:
visibleGridStepCm(web/src/lib/geometry.ts:117): The guard!(gridCm > 0) || !(scale > 0) || !(minCellPx > 0)correctly rejects zero, negative, andNaNfor all three inputs (verifiedNaN > 0isfalse, so!false... actually!(NaN>0)=!false=true→ returnsnull). The decade loop is bounded (d <= 5), so it always terminates; for a degenerate1e-9grid the max tested step is5e-4 cm, below the floor, so it returnsnullrather than spinning or drawing nothing silently. Matches the test atgeometry.test.ts.GardenFormModalwarning path (GardenFormModal.tsx:135):gridEntered = cmFromDisplay(parseFloat(gridSize), unit). For empty/non-numeric input,parseFloatreturnsNaN,cmFromDisplaypropagatesNaNthroughroundCm(verified:Math.round(NaN*1e6)/1e6 === NaN), andgridTooFine = Number.isFinite(NaN) && ...isfalse— so no warning renders andformatCmis never called withNaN. For0or negative input,gridEntered > 0short-circuitsgridTooFinetofalse. No panic, no malformed message on the unhappy paths.onSubmitvalidation (GardenFormModal.tsx:83-93):parseFloatof an empty/invalid field yieldsNaN,cmFromDisplay(NaN, …)yieldsNaN, andisValidDimensionCmchecksNumber.isFinitefirst (verified atunits.ts:22), so the form shows the range error instead of submittingNaNto the API. No regression vs. the previouscmFromSpacingpath.GardenCanvasnull-step path (GardenCanvas.tsx:118-125, 221): WhendrawnGridCm == null, the<defs>/<pattern>block is skipped entirely (no rendering withnull/0width), and the note branch handles bothsnapToGridon and off without falling through toformatCm(null). ThedrawnGridCm !== gridCmcomparison is safe fornullonly on the already-narrowed non-null branch.roundCm(units.ts:29):Math.round(NaN * 1e6) / 1e6isNaN— it does not throw or coerce to0, so NaN inputs flow through to the finite-checks at the validation layer rather than being silently masked as valid.The only cosmetic nit (not material, not reported as a finding): for a sub-centimeter positive input like
0.0001,gridTooFineistrueandformatCmrenders0′ 0″, an odd-looking but non-crashing message for a value that fails validation anyway.opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
parseFloatand validated against existing server-sidevalidDimensionCMbounds before submission. The SVGdattribute interpolation inGardenCanvas.tsxusesdrawnGridCm, which is produced byvisibleGridStepCmand is guarded against non-finite / non-positive values — it is not raw user input.gridTooFinewarning is UI-only and does not weaken server-side validation.🎯 Correctness — No material issues found
No material issues found
I reviewed the change through the correctness lens and verified the actual checked-out code for the key files (
web/src/lib/units.ts,web/src/lib/geometry.ts,web/src/components/gardens/GardenFormModal.tsx,web/src/editor/GardenCanvas.tsx, and the test files). All verifications confirm the following:cmFromFtIn,cmFromMeters, andcmFromSpacingnow useroundCm(cm * 1e6) / 1e6, which correctly removes IEEE-754 representation dust (e.g.,12 * 2.54 = 30.479999999999997becomes30.48) without throwing away meaningful precision. The conversion factors (2.54,12,100) are applied correctly.visibleGridStepCmreturns the smallest multiple of the true grid (2×,5×,10×, …) whose cells exceed the legibility floor. Every returned line is still a real grid line, and snapping continues to use the truegridCm, so snapped positions are always a superset of drawn lines. Degenerate cases returnnullas documented.MIN_GARDEN_GRID_CM = 10) correctly identifies sub-10 cm grids. The warning only fires for finite, positive values below the threshold, and the displayed text uses the sameformatCmhelper used elsewhere.drawnGridCm !== gridCmand explains an undrawable grid only whensnapToGridis active (the state where user confusion is highest). The!= nullcheck properly handles thenullreturn fromvisibleGridStepCm.The only side effect is that pre-existing imperial garden data that was quantized to whole centimeters will display slightly differently and may shift by sub-centimeter amounts on a no-op re-save. This is an explicit and accepted consequence of fixing the systematic quantization bug; the PR description states that existing gardens are intentionally not migrated.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
Through the code-cleanliness lens, this PR is well-structured:
convertGrid/cmFromSpacing/spacingFromCm/spacingUnitLabel) fromGardenFormModal.tsxand consolidates grid handling onto the same dimension-scale helpers used for width and height. That reduces future maintenance burden when unit logic changes.visibleGridStepCmhelper ingeometry.tsis cleanly factored out with a clear docstring and a single, testable responsibility. The coarsening logic is decoupled from React rendering, which keepsGardenCanvas.tsxreadable.roundCminunits.tsis a sensible private helper that replaces three previously copy-pastedMath.round(...)calls, centralizing the “dust-removal” policy.GardenCanvas.tsxandgeometry.tsexplain why the coarsening exists and what the twogridNotestates mean, which prevents the next reader from re-introducing a fade-out or silently drawing nothing.No dead code, no confusing names, no overly long or deeply nested functions, and no patterns that deviate from the surrounding codebase in a way that would trip up future maintainers.
⚡ Performance — No material issues found
No material issues found
visibleGridStepCmis a trivial fixed-bound loop (at most 18 iterations of multiplies/compares). Called on every viewport update, but the total work is ~40 arithmetic operations — negligible compared to the surrounding React reconciliation and SVG paint.strokeOpacityon every zoom frame, forcing the browser to re-paint the pattern. The new code only changes the pattern dimensions at discrete coarsening thresholds, and opacity is now constant. This is a net win for the compositor.roundCmadds two multiplies and aMath.roundversus the old singleMath.round, but these functions are only hit on form input and initialization, never inside drag/animation loops.gridNotestring andformatCmcall only execute when the coarsening level actually changes; thebedGridmemo is unchanged.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
After reviewing the diff through the error-handling & edge-cases lens, every guard, null/NaN check, and boundary condition appears correctly implemented:
GardenFormModal.tsx:135-136—gridTooFinesafely short-circuits onNaN/ non-finite / non-positive input viaNumber.isFinite(gridEntered) && gridEntered > 0before comparing againstMIN_GARDEN_GRID_CM. Verified by reading the file.units.ts:29-31(roundCm) — rounds to 1e-6 cm, well below any garden-relevant precision, and handles the IEEE-754 dust case (12 × 2.54) without introducing overflow/underflow for the valid dimension range [1, 10000]. Verified.geometry.ts:117-128(visibleGridStepCm) — returnsnullon degenerate inputs (gridCm <= 0,scale <= 0,minCellPx <= 0) and after exhausting the coarsening cap, preventing silent no-op drawing. The caller (GardenCanvas.tsx) surfaces thenullwith an explanatory label rather than drawing nothing. Verified by reading both files.GardenCanvas.tsx:221) — uses strictdrawnGridCm != nullbefore rendering the<pattern>, so no invalid SVG is produced for degenerate grids. Verified.snapValueandsnapLocalToBedGridalready toleratestep <= 0by returning the original value, so disabling snapping never producesNaNor crashes. Verified by readinggeometry.ts.No ignored errors, missing clean-up, panics on bad input, or unhandled edge cases were found in this diff.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
All five Gadfly findings addressed in
1cf1e0d.The best of them is the one only
opencode/glm-5.2caught: the soft-minimum hint contradicted itself. "did you mean ft?" was written against the old labelling, where the grid field said inches while sitting under a feet field — the exact thing this PR removed. With the field already in feet the question is nonsense.The remaining failure mode is a different one: someone reaching for plant spacing, which belongs to the bed, not the garden. The hint now says that and points at where the bed grid actually lives (
Bed gridin the inspector), which is the confusion this whole issue is really about. The stale "1 typed meaning one foot" comments went with it.The other four:
gridNotenested ternary (2/5) → extractedgridNoteFor(...), same logic in explicit branches, reasoning in one doc comment instead of threaded between the arms.cmFromDisplaycomputed twice → hoisted to one per-render value shared by the hint and the submit handler, so they can't disagree about what was entered.roundCmcomment said "1 nanometer" → it's 10 nm (1e-6 cm). Comment-only; corrected. Good catch — the constant was always fine.OVERLAY_BADGE_CLASS.npm test(39),tsc --noEmit,npm run build,go test ./...all green.