Skip to content

[Maintainer] Reference scale: validate calibration input and harden label fallback - #1170

Merged
Aymericr merged 1 commit into
mainfrom
cursor/reference-scale-validation-2596
Oct 27, 2026
Merged

Aymericr merged 1 commit into
mainfrom
cursor/reference-scale-validation-2596

Conversation

@Aymericr

@Aymericr Aymericr commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

A follow-up to #1164. It adds input validation to reference-scale calibration and makes the legacy label parser more robust.

  • Calibration rejects lengths over 1,000,000. The "Set overlay scale" form used to check only that the length was above 0. The schema caps displayLength at 1e6, so a larger value could be saved and then fail validation on import, paste or patch. The form now shows an inline message ("Length must be at most 1000000 m.", using the unit you picked) and disables Save Scale. createGuideScaleReference is the only code that writes displayLength. It now leaves out displayLength/displayUnit when the value is outside the schema range and keeps only the label, so whatever it returns always passes the schema. The guide move/resize path copies a reference that is already stored and doesn't set these fields.
  • Label fallback no longer calls String() on arbitrary values. If a scene was loaded without validation and a persisted label isn't a string, the label now renders as the number when it is a number and as an empty string otherwise. Odd values such as { toString: null }, null-prototype objects or symbols can no longer throw.
  • Legacy labels accept runs of whitespace between digit groups. '1 234,5 m' parses again. The pattern is now \s+ between digit groups; JavaScript's \s already covers NBSP and narrow NBSP. Whitespace and digit groups can't match the same characters, and the existing 64-character cap still applies before matching.

Deferred: a hand-typed '1.234 m' still reformats to '1234 m'. Only hand-typed labels are affected. The editor never saved a label with a thousands separator or more than two decimals: it used toFixed(2) before #1162, and the shared formatter has had grouping off since #1148. The parser reads three digits after the last separator as grouping, which is what makes a hand-typed '1,234 m' read as 1234. Reading '1.234 m' as 1.234 instead would mean choosing a convention for dot versus comma in free text. That is out of scope for this PR, so behaviour is unchanged.

How to test

  1. Open a floor plan with a reference image and start Set Scale, then draw a line.
  2. Enter 2000000 (meters). An inline error says the length must be at most 1000000 m, and Save Scale is disabled.
  3. Enter 1000000. The error goes away and saving works.
  4. Automated: bun test src/components/editor/floorplan-panel.test.ts and the full packages/editor test suite (2669 pass), plus turbo run check-types --filter=@pascal-app/editor.

Screenshots / screen recording

N/A: the only visible change is the inline validation message.

Checklist

  • I've tested this locally with bun dev
  • My code follows the existing code style (run bun check to verify)
  • I've updated relevant documentation (if applicable)
  • This PR targets the main branch
Open in Web Open in Cursor 

Note

Low Risk
Changes are limited to floor-plan reference-scale validation and display; they reduce invalid persisted data and rendering edge cases without touching auth or core persistence paths.

Reviewed by Cursor Bugbot for commit 9037f09. Bugbot is set up for automated code reviews on this repo. Configure here.

@Aymericr Aymericr added the bot:maintainer label Oct 27, 2026 — with Cursor
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review 🔄 Running since 2026-10-27T02:29:50.004117Z 9037f09 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copy link
Copy Markdown
Member Author

Tooling cross-review at 9037f09: APPROVE

Every check that ran on this exact SHA is green: Changed paths, quality, cli-smoke, and Cursor Bugbot with no issues. mcp-ci is path-filtered to core, MCP and scene APIs, and this PR doesn't touch any of them, so it didn't run. Nothing is pending, and mergeable_state is clean. The touched tests pass locally (14/14), and there are no inline findings.

  • Cap: getReferenceScaleInputError rejects 1000001, 1 000 001 (fr) and 1e7 with an inline error, and still accepts 1000000, 999999.99 in, 2,75 (fr) and 5'11". The save handler returns early above the cap. When it's called directly above the cap, createGuideScaleReference writes the label only, and that save passes both the @pascal-app/core@1.0.6 and head GuideScaleReference/GuideNode schemas in every unit up to 1e21. It displays as e.g. 1000001 m / 5000000000 ft. Saves in range are unchanged.
  • \s+ regex: The whitespace runs and digit groups can't match the same characters, and the whitespace run before the unit can only start after a digit group. On 100k-character adversarial inputs (long space, NBSP or tab runs, or 1 repeated, then a bad tail) each run took 0.5-1.1 ms, and time grew linearly from 6k to 100k characters. Old French and English labels and the ±1.25 and ±1.2499999999999998 rounding cases all match my last pass, with 0 mismatches against Intl in 200k random samples.
  • 1.234 m: I accept keeping this as is, but the stated reason doesn't hold. No version ever wrote a dot thousands separator: before [Maintainer] i18n: fix scoped-locale and decimal Bugbot findings from #1148 #1162 labels used toFixed(2), [Maintainer] i18n: fix scoped-locale and decimal Bugbot findings from #1148 #1162 and [Maintainer] i18n: locale-neutral reference-scale label and half-away-from-zero rounding #1164 format with useGrouping: false, and the locales are only en, zh-CN, fr, es and ja (no de). A label with 3 digits after the separator never came from the app, though, so reading it as thousands is a safe guess. A comment saying that would be clearer than one about German scenes.

This PR doesn't touch packages/core or the scene schema, and the save format is the same as #1164 for every value the form accepts. So Security isn't needed under the merge rule.

@Aymericr
Aymericr merged commit b45dcf6 into main Oct 27, 2026
4 checks passed
@Aymericr
Aymericr deleted the cursor/reference-scale-validation-2596 branch October 27, 2026 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants