Units correctness: exact imperial entry, garden grid at dimension scale (#47) #60

Merged
steve merged 2 commits from fix/units-correctness into main 2026-07-21 04:50:49 +00:00
7 changed files with 204 additions and 57 deletions
+5
View File
@@ -0,0 +1,5 @@
{
"enabledPlugins": {
"frontend-design@claude-plugins-official": true
}
}
+45 -34
View File
@@ -9,12 +9,11 @@ import { errorMessage } from '@/lib/api'
import { conflictGarden, useCreateGarden, useUpdateGarden, type Garden } from '@/lib/gardens'
import {
cmFromDisplay,
cmFromSpacing,
dimensionUnitLabel,
displayFromCm,
formatCm,
isValidDimensionCm,
spacingFromCm,
spacingUnitLabel,
MIN_GARDEN_GRID_CM,
type UnitPref,
} from '@/lib/units'
@@ -47,7 +46,7 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
const [width, setWidth] = useState(() => dimString(garden?.widthCm, garden?.unitPref ?? 'metric'))
const [height, setHeight] = useState(() => dimString(garden?.heightCm, garden?.unitPref ?? 'metric'))
const [gridSize, setGridSize] = useState(() =>
String(spacingFromCm(garden?.gridSizeCm ?? DEFAULT_GRID_CM, garden?.unitPref ?? 'metric')),
String(displayFromCm(garden?.gridSizeCm ?? DEFAULT_GRID_CM, garden?.unitPref ?? 'metric')),
)
const [snapToGrid, setSnapToGrid] = useState(garden?.snapToGrid ?? false)
const [notes, setNotes] = useState(garden?.notes ?? '')
@@ -60,18 +59,23 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
const v = parseFloat(s)
return Number.isFinite(v) ? String(displayFromCm(cmFromDisplay(v, unit), next)) : s
}
// The grid is entered at spacing scale (cm/in), so it converts with the
// spacing helpers, not the dimension ones.
const convertGrid = (s: string) => {
const v = parseFloat(s)
return Number.isFinite(v) ? String(spacingFromCm(cmFromSpacing(v, unit), next)) : s
}
setWidth(convert(width))
setHeight(convert(height))
setGridSize(convertGrid(gridSize))
// The garden grid is a layout concern, so it lives at the same scale as the
// garden's own dimensions — same helpers, same unit label. (The *bed* grid in
// the object inspector is a plant-spacing concern and stays at cm/in.)
setGridSize(convert(gridSize))
setUnit(next)
}
// Converted once per render and used by both the submit handler and the
// too-fine hint, so the two can never disagree about what was entered.
const gridSizeCm = cmFromDisplay(parseFloat(gridSize), unit)
// Soft floor: hint, don't refuse. A garden-scale grid this fine is usually
// someone reaching for plant spacing, which lives on the bed instead — but it
// is a legitimate choice for a very small garden, so the save still goes through.
const gridTooFine = Number.isFinite(gridSizeCm) && gridSizeCm > 0 && gridSizeCm < MIN_GARDEN_GRID_CM
async function onSubmit(e: FormEvent) {
e.preventDefault()
setFormError(null)
@@ -90,7 +94,6 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
setFormError('Width and height must be between 1 cm and 100 m.')
return
}
const gridSizeCm = cmFromSpacing(parseFloat(gridSize), unit)
if (!isValidDimensionCm(gridSizeCm)) {
setFormError('Grid size must be between 1 cm and 100 m.')
return
@@ -123,7 +126,7 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
setUnit(current.unitPref)
setWidth(dimString(current.widthCm, current.unitPref))
setHeight(dimString(current.heightCm, current.unitPref))
setGridSize(String(spacingFromCm(current.gridSizeCm, current.unitPref)))
setGridSize(String(displayFromCm(current.gridSizeCm, current.unitPref)))
setSnapToGrid(current.snapToGrid)
setNotes(current.notes)
setConflict('This garden changed elsewhere. The latest values are shown — review and save again.')
1
@@ -175,28 +178,36 @@ export function GardenFormModal({ garden, onClose }: { garden?: Garden; onClose:
/>
</div>
<div className="flex items-end gap-3">
<div className="flex-1">
<TextField
label={`Grid size (${spacingUnitLabel(unit)})`}
name="gridSize"
type="number"
inputMode="decimal"
step="any"
min="0"
value={gridSize}
onChange={(e) => setGridSize(e.target.value)}
/>
<div>
<div className="flex items-end gap-3">
<div className="flex-1">
<TextField
label={`Garden grid (${unitLabel})`}
name="gridSize"
type="number"
inputMode="decimal"
step="any"
min="0"
value={gridSize}
onChange={(e) => setGridSize(e.target.value)}
/>
</div>
<label className="flex h-9 items-center gap-2 whitespace-nowrap text-sm text-fg">
<input
type="checkbox"
checked={snapToGrid}
onChange={(e) => setSnapToGrid(e.target.checked)}
className="h-4 w-4 rounded border-border"
/>
Snap objects
</label>
</div>
Outdated
Review

🟠 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>
<label className="flex h-9 items-center gap-2 whitespace-nowrap text-sm text-fg">
<input
type="checkbox"
checked={snapToGrid}
onChange={(e) => setSnapToGrid(e.target.checked)}
className="h-4 w-4 rounded border-border"
/>
Snap objects
</label>
{gridTooFine && (
<p className="mt-1 text-xs text-muted">
{formatCm(gridSizeCm, unit)} is very fine for a whole-garden grid. Plant spacing lives on each bed
(Bed grid in the inspector), not here.
</p>
)}
</div>
<TextArea label="Notes" name="notes" rows={2} value={notes} onChange={(e) => setNotes(e.target.value)} />
+42 -9
View File
@@ -1,5 +1,14 @@
import { useEffect, useId, useMemo, useRef, useState, type PointerEvent as ReactPointerEvent } from 'react'
import { screenToWorld, snapLocalToBedGrid, snapPoint, worldToLocal, type Rect, type Size } from '@/lib/geometry'
import {
screenToWorld,
snapLocalToBedGrid,
snapPoint,
visibleGridStepCm,
worldToLocal,
type Rect,
type Size,
} from '@/lib/geometry'
import { formatCm, type UnitPref } from '@/lib/units'
import { useCreateObject, useCreatePlanting } from '@/lib/objects'
import type { Plant } from '@/lib/plants'
import type { EditorPlanting } from '@/lib/plantings'
@@ -14,9 +23,29 @@ import { useViewport } from './useViewport'
import type { EditorGarden, EditorObject } from './types'
const GRID_MIN_CELL_PX = 6
const GRID_MAX_OPACITY = 0.18
const GRID_OPACITY = 0.18
const OVERLAY_BADGE_CLASS = 'rounded-md bg-surface/80 px-2 py-1 text-xs text-muted backdrop-blur'
const BED_GRID_MAX_LINES = 200 // safety cap on lines drawn inside one bed
/**
* What to tell the user about the grid they're looking at, or null when the
* drawn lines are simply the garden's own grid and need no explanation. Two
* states are worth naming rather than leaving them to infer: lines drawn every
* Nth grid cell (the grid was too fine to draw at this zoom), and — degenerate —
* a grid so fine no multiple of it is drawable, which matters most when snapping
* is on and would otherwise be invisibly active (#47).
*/
function gridNoteFor(
drawnCm: number | null,
gridCm: number,
snapToGrid: boolean,
unit: UnitPref,
): string | null {
if (drawnCm == null) return snapToGrid ? 'grid too fine to draw — zoom in' : null
if (drawnCm === gridCm) return null
return `grid every ${formatCm(drawnCm, unit)}`
}
/** The world-space bounds rect of an object (center + size → top-left rect). */
function objectRect(o: EditorObject): Rect {
return { x: o.xCm - o.widthCm / 2, y: o.yCm - o.heightCm / 2, w: o.widthCm, h: o.heightCm }
1
@@ -99,9 +128,12 @@ export function GardenCanvas({
fitToRect(target ? objectRect(target) : gardenRect, size)
}, [size, garden.id, garden.widthCm, garden.heightCm, focusedObjectId, objects, gardenRect, fitToRect])
// Draw the true grid when its cells are legible, else the coarsest-but-smallest
// multiple of it that is (see visibleGridStepCm). Snapping still uses gridCm —
// the drawn lines are a subset of the snap positions, never a different grid.
const gridCm = garden.gridSizeCm
const cellPx = gridCm * viewport.scale
const gridOpacity = Math.max(0, Math.min(GRID_MAX_OPACITY, ((cellPx - GRID_MIN_CELL_PX) / GRID_MIN_CELL_PX) * GRID_MAX_OPACITY))
const drawnGridCm = visibleGridStepCm(gridCm, viewport.scale, GRID_MIN_CELL_PX)
const gridNote = gridNoteFor(drawnGridCm, gridCm, garden.snapToGrid, garden.unitPref)
const sorted = useMemo(() => [...objects].sort((a, b) => a.zIndex - b.zIndex), [objects])
const rendered = useMemo(
@@ -197,11 +229,11 @@ export function GardenCanvas({
onPointerDown={onCanvasPointerDown}
>
<g transform={`translate(${viewport.tx} ${viewport.ty}) scale(${viewport.scale})`}>
{gridOpacity > 0.005 && (
{drawnGridCm != null && (
<>
<defs>
<pattern id={gridId} width={gridCm} height={gridCm} patternUnits="userSpaceOnUse">
<path d={`M ${gridCm} 0 L 0 0 0 ${gridCm}`} fill="none" stroke="#808080" strokeOpacity={gridOpacity} strokeWidth={1} vectorEffect="non-scaling-stroke" />
<pattern id={gridId} width={drawnGridCm} height={drawnGridCm} patternUnits="userSpaceOnUse">
<path d={`M ${drawnGridCm} 0 L 0 0 0 ${drawnGridCm}`} fill="none" stroke="#808080" strokeOpacity={GRID_OPACITY} strokeWidth={1} vectorEffect="non-scaling-stroke" />
</pattern>
</defs>
<rect x={0} y={0} width={garden.widthCm} height={garden.heightCm} fill={`url(#${gridId})`} />
1
@@ -278,8 +310,9 @@ export function GardenCanvas({
</g>
</svg>
<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={OVERLAY_BADGE_CLASS}>{viewport.scale.toFixed(2)} px/cm</span>
{gridNote && <span className={OVERLAY_BADGE_CLASS}>{gridNote}</span>}
</div>
<button
type="button"
+32
View File
@@ -6,6 +6,7 @@ import {
snapLocalToBedGrid,
snapPoint,
snapValue,
visibleGridStepCm,
worldToLocal,
worldToScreen,
zoomToFitRect,
@@ -128,3 +129,34 @@ describe('grid snapping', () => {
expect(p.x).toBeGreaterThanOrEqual(-60)
})
})
describe('visibleGridStepCm', () => {
it('draws the true grid when its cells are already legible', () => {
expect(visibleGridStepCm(30.48, 1, 6)).toBe(30.48)
expect(visibleGridStepCm(10, 0.6, 6)).toBe(10) // exactly at the minimum
})
it('coarsens to a multiple rather than fading out', () => {
// The #47 failure: a 3cm grid at 0.84 px/cm is 2.5px per cell. Instead of
// drawing nothing while snapping stays on, draw every 5th line (12.6px).
expect(visibleGridStepCm(3, 0.84, 6)).toBe(15)
expect(visibleGridStepCm(1, 0.84, 6)).toBe(10)
})
it('only ever returns a whole multiple of the true grid', () => {
for (const scale of [0.05, 0.3, 0.84, 1, 4]) {
const step = visibleGridStepCm(7, scale, 6)
expect(step).not.toBeNull()
expect(Math.round(step! / 7) * 7).toBeCloseTo(step!, 9)
expect(step! * scale).toBeGreaterThanOrEqual(6)
}
})
it('reports null for a degenerate grid rather than drawing nothing silently', () => {
expect(visibleGridStepCm(0, 1, 6)).toBeNull()
expect(visibleGridStepCm(-5, 1, 6)).toBeNull()
expect(visibleGridStepCm(NaN, 1, 6)).toBeNull()
expect(visibleGridStepCm(30, 0, 6)).toBeNull()
expect(visibleGridStepCm(1e-9, 1, 6)).toBeNull() // beyond the coarsening cap
})
})
+28
View File
@@ -99,6 +99,34 @@ export function snapLocalToBedGrid(p: Point, step: number, halfW: number, halfH:
return { x: snapAxis(p.x, halfW), y: snapAxis(p.y, halfH) }
}
// Multiples of the true grid tried, in ascending order within each decade, when
// the grid itself is too fine to draw legibly at the current zoom.
const GRID_COARSEN_MULTIPLES = [1, 2, 5]
const GRID_MAX_COARSEN_DECADES = 5 // up to 500,000× the true grid
/**
* The grid step (cm) to actually draw for a grid of `gridCm` at `scale` px/cm,
* so a cell is never smaller than `minCellPx`.
*
* Fading a too-fine grid out is what let a garden sit with snapping on and no
* grid drawn at all (#47). Instead, coarsen: draw 2×, 5×, 10×… the true grid, so
* every line drawn is still a real grid line and *something* aligned to the grid
* is always visible. Returns null only when even the coarsest multiple can't
* reach `minCellPx` — the caller should say so rather than draw nothing silently.
*/
export function visibleGridStepCm(gridCm: number, scale: number, minCellPx: number): number | null {
if (!(gridCm > 0) || !(scale > 0) || !(minCellPx > 0)) return null
let decade = 1
for (let d = 0; d <= GRID_MAX_COARSEN_DECADES; d++) {
for (const m of GRID_COARSEN_MULTIPLES) {
const step = gridCm * decade * m
if (step * scale >= minCellPx) return step
}
decade *= 10
}
return null
}
/** Linear interpolation. */
export function lerp(a: number, b: number, t: number): number {
return a + (b - a) * t
+31 -7
View File
@@ -1,5 +1,6 @@
import { describe, expect, it } from 'vitest'
import {
cmFromDisplay,
cmFromFtIn,
cmFromMeters,
cmFromSpacing,
@@ -11,15 +12,37 @@ import {
} from './units'
describe('cm conversions', () => {
it('feet+inches → whole cm', () => {
expect(cmFromFtIn(4)).toBe(122) // 4ft = 121.92 → 122
expect(cmFromFtIn(8)).toBe(244)
expect(cmFromFtIn(4, 6)).toBe(137) // 4'6" = 137.16 → 137
// Every cm column is REAL, so entry is exact — no whole-cm quantization. A 1 ft
// grid is 30.48 cm, not 30, or the error accumulates across the garden (#47).
it('feet+inches → exact cm', () => {
expect(cmFromFtIn(1)).toBe(30.48)
expect(cmFromFtIn(4)).toBe(121.92)
expect(cmFromFtIn(8)).toBe(243.84)
expect(cmFromFtIn(4, 6)).toBe(137.16)
})
it('meters → whole cm', () => {
it('meters → exact cm', () => {
expect(cmFromMeters(10)).toBe(1000)
expect(cmFromMeters(1.22)).toBe(122)
expect(cmFromMeters(0.305)).toBe(30.5)
})
it('rounds away float dust, not precision', () => {
// 12 × 2.54 is 30.479999999999997 in IEEE-754; 1 ft must read as 30.48.
expect(cmFromFtIn(0, 12)).toBe(30.48)
expect(cmFromSpacing(12, 'imperial')).toBe(30.48)
expect(cmFromSpacing(1, 'imperial')).toBe(2.54)
})
it('round-trips display → cm → display at display precision', () => {
const imperial = [0.5, 1, 2.5, 4, 8, 12.5, 24]
for (const ft of imperial) {
expect(displayFromCm(cmFromDisplay(ft, 'imperial'), 'imperial')).toBe(ft)
}
const metric = [0.15, 1, 1.22, 3.05, 10, 24.5]
for (const m of metric) {
expect(displayFromCm(cmFromDisplay(m, 'metric'), 'metric')).toBe(m)
}
})
})
@@ -59,13 +82,14 @@ describe('plant spacing (small scale)', () => {
expect(formatSpacing(30, 'imperial')).toBe('12″')
})
it('parses an entered value back to whole cm', () => {
it('parses an entered value back to exact cm', () => {
expect(cmFromSpacing(15, 'metric')).toBe(15)
expect(cmFromSpacing(6, 'imperial')).toBe(15) // 6in = 15.24 → 15
expect(cmFromSpacing(6, 'imperial')).toBe(15.24)
})
it('prefills an input from cm', () => {
expect(spacingFromCm(15, 'metric')).toBe(15)
expect(spacingFromCm(30, 'imperial')).toBe(11.8) // 30/2.54 = 11.81 → 11.8
expect(spacingFromCm(30.48, 'imperial')).toBe(12) // an exactly-12in grid reads back as 12
})
})
+21 -7
View File
@@ -12,19 +12,32 @@ const INCHES_PER_FOOT = 12
export const MIN_DIMENSION_CM = 1
export const MAX_DIMENSION_CM = 10_000
// Below this, a garden-scale grid is fine enough to be worth questioning — it
// usually means someone reached for plant spacing, which belongs to the bed, not
// the garden. Soft: the form hints, it doesn't refuse.
export const MIN_GARDEN_GRID_CM = 10
/** Whether a centimeter dimension is within the server's accepted range. */
export function isValidDimensionCm(cm: number): boolean {
return Number.isFinite(cm) && cm >= MIN_DIMENSION_CM && cm <= MAX_DIMENSION_CM
}
/** Feet (+ optional inches) → whole centimeters. */
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 the 1e-6 cm (10 nm) grain
Outdated
Review

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

⚪ **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_ <sub>🪰 Gadfly · advisory</sub>
* here is far below anything a garden cares about. Every cm column is REAL, so
* entry stays exact — this only stops 30.48 being stored as 30.479999999999997. */
function roundCm(cm: number): number {
return Math.round(cm * 1e6) / 1e6
}
/** Meters → whole centimeters. */
/** Feet (+ optional inches) → centimeters, exactly (1 ft → 30.48). */
export function cmFromFtIn(feet: number, inches = 0): number {
return roundCm((feet * INCHES_PER_FOOT + inches) * CM_PER_INCH)
}
/** Meters → centimeters, exactly. */
export function cmFromMeters(meters: number): number {
return Math.round(meters * CM_PER_METER)
return roundCm(meters * CM_PER_METER)
}
/** A value typed in the given unit (meters, or feet) → centimeters. */
@@ -79,9 +92,10 @@ export function formatSpacing(cm: number, unit: UnitPref): string {
return `${Math.round(cm)} cm`
}
/** A spacing value typed in the given unit (cm, or inches) → whole centimeters. */
/** A spacing value typed in the given unit (cm, or inches) → centimeters,
* exactly (12 in → 30.48). */
export function cmFromSpacing(value: number, unit: UnitPref): number {
return unit === 'imperial' ? Math.round(value * CM_PER_INCH) : Math.round(value)
return roundCm(unit === 'imperial' ? value * CM_PER_INCH : value)
}
/** Centimeters → a spacing number in the given unit, for prefilling an input.