fix: respect imperial units in wall panel

[MateoSaettone]

Summary

• add shared linear measurement helpers for meter/imperial formatting and conversion
• reuse the shared formatter across wall, fence, site edge, item placement, and floorplan measurement labels
• refactor the existing imperial-aware MetricControl to use the shared conversion helpers instead of its own feet multiplier
• display/edit wall metadata dimensions in ft when the viewer unit is imperial while keeping scene values in meters

After the fix, imperial mode keeps the selected wall metadata panel in ft while scene values continue to persist internally in meters.

Closes #322

Test Plan

• bun test packages/editor/src/lib/measurements.test.ts
• bunx biome check packages/editor/src/lib/measurements.ts packages/editor/src/lib/measurements.test.ts packages/editor/src/index.tsx packages/editor/src/components/editor/wall-measurement-label.tsx packages/editor/src/components/editor/site-edge-labels.tsx packages/editor/src/components/editor/floorplan-panel.tsx packages/editor/src/components/tools/item/use-placement-coordinator.tsx packages/editor/src/components/ui/controls/metric-control.tsx packages/nodes/src/wall/tool.tsx packages/nodes/src/wall/panel.tsx packages/nodes/src/fence/tool.tsx
• bun run --cwd packages/editor check-types
• bun run check-types

Notes

• Editable numeric controls continue to use decimal feet (for example, 10.50 ft); passive measurement labels continue to use feet/inches (for example, 10'6").
• bun run --cwd packages/nodes build currently fails on an unrelated existing type error in packages/nodes/src/site/renderer.tsx.

[Aymericr]

Solid PR — this is clearly the right architectural direction. Here's the full review.

---

Summary

7/7 tests pass. TypeScript types clean (bun run --cwd packages/editor check-types exits 0). Architecture is sound.

The measurements.ts module is correctly scoped as a pure utility in packages/editor, exported through the editor's public API, and consumed by node tools/panels via @pascal-app/editor. That's the right boundary: node-owned code can call editor public APIs, and the panel-level conversion keeps SliderControl generic.

---

✅ What's good

• No code duplication anymore. Five local formatMeasurement functions that were copy-pasted across wall-measurement-label.tsx, site-edge-labels.tsx, floorplan-panel.tsx, use-placement-coordinator.tsx, fence/tool.tsx, and wall/tool.tsx are all replaced with a single shared implementation.
• Wall panel conversion is correct. Stored scene values stay in meters; displayed values in panel.tsx are converted with metersToLinearUnit and committed back with linearUnitToMeters. The 0.1 ft drag step is a reasonable granularity for imperial editing.
• linearControlValueToMeters + minMeters clamp is a clean way to enforce the panel's physical minimum.
• MetricControl refactor properly replaces the value * multiplier pattern with explicit toDisplayValue/toStoredValue callbacks, and roundStoredValueForDisplayPrecision correctly handles float drift in the round-trip.
• Type safety: LinearUnit = 'metric' | 'imperial' explicitly matches useViewer().unit.

---

🐛 One nit worth fixing: formatLinearMeasurement doesn't guard against non-finite input

// packages/editor/src/lib/measurements.ts

export function formatLinearMeasurement(meters: number, unit: LinearUnit): string {
  if (unit === 'imperial') {
    const feet = metersToLinearUnit(meters, unit)
    const wholeFeet = Math.floor(feet)  // Math.floor(NaN) = NaN
    const inches = Math.round((feet - wholeFeet) * 12)  // NaN
    if (inches === 12) return `${wholeFeet + 1}'0"`
    return `${wholeFeet}'${inches}"`  // "NaN'NaN""
  }
  return `${Number.parseFloat(meters.toFixed(2))}m`  // "NaNm"
}

NaN and Infinity inputs produce garbage strings. The callers in wall-measurement-label.tsx already guard with Number.isFinite(length) && length >= 0.01, so this isn't reachable today — but since the function is exported publicly (@pascal-app/editor), it's worth hardening. Suggested fix:

export function formatLinearMeasurement(meters: number, unit: LinearUnit): string {
  if (!Number.isFinite(meters)) return '--'

  if (unit === 'imperial') {
    const feet = metersToLinearUnit(meters, unit)
    const wholeFeet = Math.floor(feet)
    const inches = Math.round((feet - wholeFeet) * 12)
    if (inches === 12) return `${wholeFeet + 1}'0"`
    return `${wholeFeet}'${inches}"`
  }

  return `${Number.parseFloat(meters.toFixed(2))}m`
}

---

📋 Missing test coverage

The existing 7 tests cover the happy paths well. A few more cases would make the suite robust:

// Suggested additions to measurements.test.ts

test('returns "--" for non-finite inputs', () => {
  expect(formatLinearMeasurement(NaN, 'imperial')).toBe('--')
  expect(formatLinearMeasurement(Infinity, 'imperial')).toBe('--')
  expect(formatLinearMeasurement(NaN, 'metric')).toBe('--')
})

test('formats zero correctly', () => {
  expect(formatLinearMeasurement(0, 'imperial')).toBe(`0'0"`)
  expect(formatLinearMeasurement(0, 'metric')).toBe('0m')
})

test('formats sub-foot imperial values', () => {
  expect(formatLinearMeasurement(0.1524, 'imperial')).toBe(`0'6"`)
})

---

ℹ️ Notes

Merge conflict: There's a single conflict in packages/nodes/src/fence/tool.tsx — it's only in the imports block (main added a WALL_FINE_GRID_STEP import; this PR adds formatLinearMeasurement). Easy to resolve by including both.

packages/nodes build error: The type error in packages/nodes/src/site/renderer.tsx:120 pre-exists on main — it's not introduced by this PR and exits with code 0.

npm status: @pascal-app/core, @pascal-app/viewer, and @pascal-app/editor are all at v0.8.0 on npm. This fix will ship in the next release cycle, which is correct.

step value for Curve: The step={0.1} on the Curve slider is unchanged from the original — that's pre-existing behavior, not a regression here.

---

Verdict

This PR is ready to merge with minor fixes: add the Number.isFinite guard, add the three missing test cases, and resolve the fence imports conflict. The overall approach is clean, well-tested, and correctly scoped.

[Aymericr]

Solid PR — this is clearly the right architectural direction. Here's the full review.

---

Summary

7/7 tests pass. TypeScript types clean (bun run --cwd packages/editor check-types exits 0). Architecture is sound.

The measurements.ts module is correctly scoped as a pure utility in packages/editor, exported through the editor's public API, and consumed by node tools/panels via @pascal-app/editor. That's the right boundary: node-owned code can call editor public APIs, and the panel-level conversion keeps SliderControl generic.

---

✅ What's good

• No code duplication anymore. Five local formatMeasurement functions that were copy-pasted across wall-measurement-label.tsx, site-edge-labels.tsx, floorplan-panel.tsx, use-placement-coordinator.tsx, fence/tool.tsx, and wall/tool.tsx are all replaced with a single shared implementation.
• Wall panel conversion is correct. Stored scene values stay in meters; displayed values in panel.tsx are converted with metersToLinearUnit and committed back with linearUnitToMeters. The 0.1 ft drag step is a reasonable granularity for imperial editing.
• linearControlValueToMeters + minMeters clamp is a clean way to enforce the panel's physical minimum.
• MetricControl refactor properly replaces the value * multiplier pattern with explicit toDisplayValue/toStoredValue callbacks, and roundStoredValueForDisplayPrecision correctly handles float drift in the round-trip.
• Type safety: LinearUnit = 'metric' | 'imperial' explicitly matches useViewer().unit.

---

🐛 One nit worth fixing: formatLinearMeasurement doesn't guard against non-finite input

// packages/editor/src/lib/measurements.ts

export function formatLinearMeasurement(meters: number, unit: LinearUnit): string {
  if (unit === 'imperial') {
    const feet = metersToLinearUnit(meters, unit)
    const wholeFeet = Math.floor(feet)  // Math.floor(NaN) = NaN
    const inches = Math.round((feet - wholeFeet) * 12)  // NaN
    if (inches === 12) return `${wholeFeet + 1}'0"`
    return `${wholeFeet}'${inches}"`  // "NaN'NaN""
  }
  return `${Number.parseFloat(meters.toFixed(2))}m`  // "NaNm"
}

NaN and Infinity inputs produce garbage strings. The callers in wall-measurement-label.tsx already guard with Number.isFinite(length) && length >= 0.01, so this isn't reachable today — but since the function is exported publicly (@pascal-app/editor), it's worth hardening. Suggested fix:

export function formatLinearMeasurement(meters: number, unit: LinearUnit): string {
  if (!Number.isFinite(meters)) return '--'

  if (unit === 'imperial') {
    const feet = metersToLinearUnit(meters, unit)
    const wholeFeet = Math.floor(feet)
    const inches = Math.round((feet - wholeFeet) * 12)
    if (inches === 12) return `${wholeFeet + 1}'0"`
    return `${wholeFeet}'${inches}"`
  }

  return `${Number.parseFloat(meters.toFixed(2))}m`
}

---

📋 Missing test coverage

The existing 7 tests cover the happy paths well. A few more cases would make the suite robust:

// Suggested additions to measurements.test.ts

test('returns "--" for non-finite inputs', () => {
  expect(formatLinearMeasurement(NaN, 'imperial')).toBe('--')
  expect(formatLinearMeasurement(Infinity, 'imperial')).toBe('--')
  expect(formatLinearMeasurement(NaN, 'metric')).toBe('--')
})

test('formats zero correctly', () => {
  expect(formatLinearMeasurement(0, 'imperial')).toBe(`0'0"`)
  expect(formatLinearMeasurement(0, 'metric')).toBe('0m')
})

test('formats sub-foot imperial values', () => {
  expect(formatLinearMeasurement(0.1524, 'imperial')).toBe(`0'6"`)
})

---

ℹ️ Notes

Merge conflict: There's a single conflict in packages/nodes/src/fence/tool.tsx — it's only in the imports block (main added a WALL_FINE_GRID_STEP import; this PR adds formatLinearMeasurement). Easy to resolve by including both.

packages/nodes build error: The type error in packages/nodes/src/site/renderer.tsx:120 pre-exists on main — it's not introduced by this PR and exits with code 0.

npm status: @pascal-app/core, @pascal-app/viewer, and @pascal-app/editor are all at v0.8.0 on npm. This fix will ship in the next release cycle, which is correct.

step value for Curve: The step={0.1} on the Curve slider is unchanged from the original — that's pre-existing behavior, not a regression here.

---

Verdict

This PR is ready to merge with minor fixes: add the Number.isFinite guard, add the three missing test cases, and resolve the fence imports conflict. The overall approach is clean, well-tested, and correctly scoped.

[Aymericr]

Follow-up after Codex 5.5 (GPT-5.5 / xhigh reasoning) adversarial pass on the full codebase:

---

One new blocker found that the initial review missed.

🐛 Blocker: clamp/round ordering lets imperial wall panel controls escape stored meter bounds

packages/nodes/src/wall/panel.tsx:139

<SliderControl
  label="Length"
  max={metersToLinearUnit(20, unit)}
  min={metersToLinearUnit(0.1, unit)}
  onChange={(value) => handleUpdateLength(linearUnitToMeters(value, unit))}
  precision={2}
  step={unit === 'imperial' ? 0.1 : 0.01}
  unit={unitLabel}
  value={displayLength}
/>

SliderControl clamps first, then rounds at stepPrecision. In imperial mode:

• Length minimum is 0.1m = 0.3281ft. Scroll down at the minimum: clamps to 0.3281, rounds to 0.3ft, stores linearUnitToMeters(0.3) = 0.09144m — below 0.1m minimum.
• Height maximum is 6m = 19.685ft. Scroll up at the maximum: clamps to 19.685, rounds to 19.7ft, stores linearUnitToMeters(19.7) = 6.004m — above 6m maximum.

I verified this numerically:

0.1m → 0.3280839895ft → rounded 0.3ft → stored 0.09144m  (delta -0.009m)
6m  → 19.68503937ft → rounded 19.7ft → stored 6.0045m   (delta +0.004m)

Fix: In handleUpdateLength (and the other panel onChange handlers), clamp the returned meters value to [minMeters, maxMeters] before committing. Or add a maxMeters option to linearControlValueToMeters and use it. The root fix is: whatever value eventually gets stored must be clamped in the stored unit (meters), not in the display unit (feet).

---

🐛 Same issue applies to MetricControl

packages/editor/src/components/ui/controls/metric-control.tsx:53

const roundStoredValueForDisplayPrecision = useCallback(
  (storedValue: number) =>
    toStoredValue(Number.parseFloat(toDisplayValue(storedValue).toFixed(precision))),
  [precision, toDisplayValue, toStoredValue],
)

Clamps in the clamp() call site first, then rounds in display units. At a min of 0.04m, the display rounds to 0.13ft = 0.03962m. Same family of bug, same fix.

---

Suggestion: linearControlValueToMeters API gap

The helper currently only supports minMeters. Adding maxMeters would let callers protect both bounds in one call, or alternatively drop the options and do all clamping at each call site.

---

Suggestion: formatLinearMeasurement with negative inputs

formatLinearMeasurement(-0.1524, 'imperial') returns -1'6" (Math.floor(-0.5) === -1). Not a current UI bug since all callers pass distances/dimensions, but the function is exported as a public API — worth guarding or documenting.

---

Type check status

The existing check-types errors on main (ViewerState.theme, preset event types) are pre-existing and unrelated to this PR — confirmed exits code 2 with the same errors on the unmodified main branch.

---

All other findings from the initial review stand. Fix the clamp/round ordering and this PR is solid.

[Aymericr]

Review: fix: respect imperial units in wall panel

Solid contribution. The core approach is right — centralising the scattered formatMeasurement/multiplier logic into a single measurements.ts with named, tested helpers is exactly the kind of consistency fix this codebase needed. The panel-side imperial display is now correct and MetricControl behaves properly across scroll/drag/keyboard interactions. Overall this is in good shape; a few things worth addressing before merge.

---

✅ What's working well

• Single source of truth: Five separate inline formatMeasurement copies are gone. METERS_PER_FOOT = 0.3048 is defined once; downstream callers can't drift from each other.
• Exports from packages/editor/src/index.tsx: packages/nodes imports helpers from @pascal-app/editor (its declared dep), keeping the dependency direction clean — no cycles introduced.
• MetricControl step/scroll/drag semantics are preserved: The refactor from step / multiplier to toStoredValue(step) is algebraically equivalent. shiftKey (×10) and altKey (×0.1) modifiers all correctly delegate to the same helper, and useEffect deps are updated to match. The logic is correct.
• All 7 tests pass ✅ (bun test packages/editor/src/lib/measurements.test.ts)
• linearControlValueToMeters minMeters clamp is a nice defensive detail for the panel onChange callbacks.

---

🔍 Issues worth addressing

1. formatLinearMeasurement doesn't guard against NaN or Infinity (important)

If a wall or site edge ever has a NaN or Infinity length (degenerate geometry, race condition during node creation), Math.floor(NaN) returns NaN and Math.round(NaN) returns NaN, producing the label "NaN'NaN" visible in the UI. The old inline copies had the same bug, but now is a good time to fix it since we're centralising.

export function formatLinearMeasurement(meters: number, unit: LinearUnit): string {
  if (!Number.isFinite(meters)) return unit === 'imperial' ? `0'0"` : '0m'
  if (unit === 'imperial') {

2. Negative values format incorrectly (nit, but visible for curve offset)

formatLinearMeasurement(-0.5, 'imperial') → Math.floor(-1.6404) = -2, inches = Math.round((-1.6404 - (-2)) * 12) = Math.round(4.3) = 4 → displays -2'4". That's mathematically wrong (-0.5m ≈ -1'7.87" → -1'8" or just -1.64 ft). The curve-offset SliderControl in wall/panel.tsx can go negative (it's clamped around ±maxCurveOffset). Worth using Math.abs + a sign prefix, or just displaying as decimal feet for negative values (which is what the curve offset labels show anyway).

3. SliderControl in packages/nodes — imperial not wired up (important, adversarial finding)

wall/panel.tsx correctly converts SliderControl value / min / max / onChange via the new helpers. But SliderControl is also used in door/panel.tsx (29+ instances), roof/panel.tsx, column/panel.tsx, slab/panel.tsx, fence/inspector-editors.tsx — none of those are touched by this PR. Those panels will still show metric values when the viewer is in imperial mode.

This PR closes #322 (wall panel specifically) and that's fair scope. But the PR description says "shared linear measurement helpers so wall labels and related measurement labels do not duplicate imperial formatting logic" — suggesting broader intent. A follow-up issue tracking the remaining panels would be worth filing so contributors know the work isn't done.

4. Test coverage gaps (nit)

Missing cases worth adding:

• formatLinearMeasurement(0, 'imperial') → expect 0'0"
• Negative input (see #2 above)
• NaN / Infinity input
• metersToLinearUnit(0, 'imperial') and linearUnitToMeters(0, 'imperial')
• linearControlValueToMeters(0, 'imperial', { minMeters: 0.1 }) → clamp kicks in

5. packages/nodes build note (informational, pre-existing)

The PR notes bun run --cwd packages/nodes build fails on a pre-existing type error in packages/nodes/src/site/renderer.tsx. We confirmed this error exists on main too — not introduced by this PR. Worth tracking separately before the next npm release.

---

💡 Minor nits

• Number.parseFloat(meters.toFixed(2)) in the metric branch strips trailing zeros (e.g. 3.10 → 3.1). This is fine UX-wise but different from the original inline copies, which did the same thing — just noting it's intentional.
• FEET_PER_METER = 1 / METERS_PER_FOOT introduces a tiny floating-point rounding: 1 / 0.3048 = 3.2808398950131235 vs. the old hardcoded 3.280_84. The difference is ~5e-8 ft, well within display rounding — no practical impact.

---

Summary

The two items most worth addressing before merge: the NaN/Infinity guard (#1) and a follow-up issue for the remaining panels (#3). The negative-value curve offset formatting (#2) is lower priority since curve offsets are small in practice. Everything else is nits.

Happy to help with the follow-up issue for the remaining panels if that's useful.