[quantity-formatting] Fix five format editing bugs: separators, stale formats, trait casing, DMS sub-units, horizontal-direction azimuths by hl662 · Pull Request #1827 · iTwin/viewer-components-react

hl662 · 2026-10-08T03:00:47Z

TL;DR

Five bugs reported by our users made formats hard or impossible to edit in the quantity formatting UI. Four are fixed in the package. The fifth (#1825) comes from apps changing the format set in place, so activeFormatSet is now documented as immutable and test-viewer publishes a new format set instead. Regression tests fail on the current code. Patch only: no API or peer dependency changes.

Stacked on #1826. Closes #1824, closes #1825.

What was wrong

Problem the user sees	Why it happened	Fix
A comma can be both the decimal and thousands separator (#1824)	The clash check only ran when you changed a separator with grouping already on. Turning grouping on with a comma decimal kept both as commas.	When grouping is on and the separators clash, the other one switches between `.` and `,`.
After Apply, switching to another format and back shows the old settings (#1825)	`FormatSetFormatsProvider.addFormat` updates the format set in place, so the object passed to `FormatSelector` never changes and its memoized list keeps the old definitions.	`activeFormatSet` is documented as immutable. test-viewer publishes a new format set after `onFormatsChanged`; apps holding a `FormatSetFormatsProvider` need to do the same.
"Show unit label" stays on and can't be unchecked	Saved formats can spell traits `ShowUnitLabel`; the checkboxes only matched `showUnitLabel`. Core reads either, so the label kept showing. Same for every trait checkbox.	All checkboxes match traits in any case and remove every spelling when cleared.
Can't build degrees-minutes-seconds for angles or azimuths	"Add sub-unit" offered grad (0.9°) under degrees, and minutes under minutes, because of a rounding error.	Only units that split the parent into whole parts (at least 2) are offered: ° → ′ → ″, ft → in, m → cm.
Switching a horizontal-direction format to Azimuth or Bearing breaks formatting	The type picker always filled in plain angle units, which can't convert to horizontal-direction units, so formatting threw.	It picks matching horizontal-direction units.

What

Asset	Why it exists
`internal/FormatPropsUtils.ts` (new)	Each control had its own exact-case trait check. One shared helper now reads traits case-insensitively and, on change, removes every casing variant and writes one canonical name. It also resolves separator clashes against the formatter's locale defaults.
Trait checkboxes and format panels	Use the shared helper, so PascalCase traits saved by consumers show and clear correctly. Net −186 lines.
`DecimalSeparator` / `ThousandsSeparator`	When grouping is on, turning it on or changing either separator switches the other one if they clash. While grouping is off, the unused thousands separator is left alone. Empty separators never count as a clash.
`FormatSelector` / README	Documents that `activeFormatSet` must be replaced, not changed in place, with an example. Also localizes the "no formats found" message.
test-viewer `FormatManager` / `QuantityFormatButton`	Publish a new format set after `onFormatsChanged`, so the selector shows applied changes.
`FormatUnits`	Offers a sub-unit only when it divides the parent into a whole number of parts, at least two. Degrees → minutes → seconds, feet → inches and metres → centimetres still work; grad and self-matches are excluded. The label field shows the default unit label as a placeholder. Uses `Logger` instead of `console.warn`.
`FormatType` (format type picker)	Picks the revolution and azimuth base units from the format unit's family, so horizontal-direction azimuths use `HORIZONTAL_DIR_*` units. Ignores a unit lookup that finishes after a newer change or after the picker unmounts.
`AppendUnitLabel.tsx` / `UomSeparatorSelector.tsx`	Split from `FormatUnitLabel.tsx`, one component per file.
`misc/UnitDescr`	The component was unused. Only its `getUnitName` helper is kept.
Changesets	Five patch entries, one per user-visible fix.

Tests

The regression tests fail against the original source. Control tests, such as the azimuth defaults being applied when nothing changed, pass on both.

New and updated test coverage

Test	What it covers
`TraitCheckboxes.test.tsx` — PascalCase trait shown checked / cleared (×6 controls)	Every trait checkbox reads and clears traits saved in PascalCase without touching other traits.
`AppendUnitLabel.test.tsx` — PascalCase `ShowUnitLabel`	The "can't uncheck Show unit label" case: the checkbox shows checked, and unchecking removes every spelling.
`FormatPropsUtils.test.ts` — trait helpers	Case-insensitive detection, removing duplicates, string-form traits, other entries kept in order, input not mutated.
`FormatPropsUtils.test.ts` — separator conflicts	Either side can be preserved; locale defaults are used when unset; empty separators and distinct separators are left alone.
`ThousandsSeparator.test.tsx` — enabling with comma decimal	The #1824 repro.
`DecimalSeparator.test.tsx` — clash / grouping off / locale default	Clashes resolve only while grouping is on; the default display follows the locale.
`FormatUnits.test.tsx` — degree→minute, minute→second, horizontal-direction degree→minute	DDMMSS can be built for angles and azimuths.
`FormatUnits.test.tsx` — excludes grad, excludes large non-whole ratio	The sub-unit rule rejects non-whole subdivisions, including near-integer ratios from custom units.
`FormatUnits.test.tsx` — default label placeholder	A blank label shows what core will render instead.
`FormatType.test.tsx` — azimuth/bearing for horizontal directions and angles	Picks the right units and formats 45 as `45` / `N45E` end to end.
`FormatType.test.tsx` — pending azimuth lookup	A lookup that finishes after a props change or unmount does not call `onChange`; one that finishes with nothing changed does.

Validation

tsc, pnpm lint, pnpm test (197/197), pnpm build, extract-api (no API change), and check-extractions all pass. test-viewer typechecks and lints clean; it has not been smoke-tested again since the review changes.

Nambot 🤖 (powered by claude-opus-5.5)

… decimal/thousands separators distinct Show unit label (and every other trait checkbox) could not be cleared when traits were persisted in PascalCase. Fixes #1824 (comma allowed as both separators).

…ter in-place format set updates Fixes #1825.

…units; show default unit label placeholder DDMMSS was unreachable: GRAD was offered under degrees and ARC_MINUTE as its own sub-unit. Removes unused internal UnitDescr component; log through Logger.

…, stricter sub-unit tolerance, test gaps - Resolve separator conflicts against the formatter's locale defaults; empty separators never collide; leave the thousands separator alone while grouping is off - Tighten whole-subdivision tolerance to 1e-9 relative - Tests: PascalCase traits for every checkbox, string formatTraits, non-whole large ratios, deterministic option waits

…uth of horizontal directions FormatTypeOption hard-coded Units.REVOLUTION and Units.ARC_DEG, which cannot convert to HORIZONTAL_DIRECTION units, so switching a horizontal-direction format to bearing/azimuth made the formatter throw.

Copilot

🟡 Changes recommended

Large fractional ratios can pass the subdivision check, and asynchronous type changes can overwrite newer user edits.

2 open findings

Ignore stale unit lookup results to prevent overwriting newer edits · New
Cap tolerance to reject large non-integral unit ratios · New

What changed in this PR

Fixes five quantity-format editing defects involving separators, stale formats, trait casing, composite sub-units, and horizontal-direction formats.

Changes:

Centralizes trait and separator handling.
Corrects format selection, sub-unit filtering, and direction-unit resolution.
Adds localization, regression tests, and patch changesets.

File	Description
`src/test/quantityformat/internal/TraitCheckboxes.test.tsx`	Tests PascalCase traits.
`src/test/quantityformat/internal/ThousandsSeparator.test.tsx`	Tests separator conflicts.
`src/test/quantityformat/internal/ShowTrailingZeros.test.tsx`	Corrects string-trait coverage.
`src/test/quantityformat/internal/FormatUnits.test.tsx`	Tests sub-units and labels.
`src/test/quantityformat/internal/FormatUnitLabel.test.tsx`	Tests unit-label traits.
`src/test/quantityformat/internal/FormatType.test.tsx`	Tests direction units.
`src/test/quantityformat/internal/FormatPropsUtils.test.ts`	Tests shared utilities.
`src/test/quantityformat/internal/DecimalSeparator.test.tsx`	Tests decimal conflicts.
`src/test/quantityformat/FormatSelector.test.tsx`	Tests current format retrieval.
`src/components/quantityformat/panels/Station.tsx`	Uses shared trait and unit logic.
`src/components/quantityformat/panels/Scientific.tsx`	Uses shared trait and unit logic.
`src/components/quantityformat/panels/Ratio.tsx`	Uses shared trait and unit logic.
`src/components/quantityformat/panels/Fractional.tsx`	Uses shared trait and unit logic.
`src/components/quantityformat/panels/Decimal.tsx`	Uses shared trait and unit logic.
`src/components/quantityformat/panels/Bearing.tsx`	Supplies the units provider.
`src/components/quantityformat/panels/Azimuth.tsx`	Uses shared trait and unit logic.
`src/components/quantityformat/internal/ZeroEmpty.tsx`	Centralizes trait updates.
`src/components/quantityformat/internal/ThousandsSeparator.tsx`	Resolves grouping conflicts.
`src/components/quantityformat/internal/ShowTrailingZeros.tsx`	Centralizes trait updates.
`src/components/quantityformat/internal/misc/UnitDescr.tsx`	Removes unused component code.
`src/components/quantityformat/internal/misc/UnitDescr.ts`	Retains the unit-name helper.
`src/components/quantityformat/internal/misc/FormatType.tsx`	Resolves direction-family units.
`src/components/quantityformat/internal/KeepSingleZero.tsx`	Centralizes trait updates.
`src/components/quantityformat/internal/KeepDecimalPoint.tsx`	Centralizes trait updates.
`src/components/quantityformat/internal/FractionDash.tsx`	Centralizes trait updates.
`src/components/quantityformat/internal/FormatUnits.tsx`	Filters sub-units and improves labels.
`src/components/quantityformat/internal/FormatUnitLabel.tsx`	Handles trait casing.
`src/components/quantityformat/internal/FormatPropsUtils.ts`	Adds shared format utilities.
`src/components/quantityformat/internal/DecimalSeparator.tsx`	Resolves decimal conflicts.
`src/components/quantityformat/internal/AzimuthOptions.tsx`	Uses structured logging.
`src/components/quantityformat/FormatSelector.tsx`	Reads current format definitions.
`public/locales/en/QuantityFormat.json`	Adds the empty-search translation.
`.changeset/@itwin-quantity-formatting-react-stale-format-selection.md`	Documents stale-format fix.
`.changeset/@itwin-quantity-formatting-react-horizontal-direction-bearing.md`	Documents direction-unit fix.
`.changeset/@itwin-quantity-formatting-react-format-trait-casing.md`	Documents trait-casing fix.
`.changeset/@itwin-quantity-formatting-react-distinct-separators.md`	Documents separator fix.
`.changeset/@itwin-quantity-formatting-react-composite-sub-units.md`	Documents sub-unit fix.

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

…ups, cap sub-unit tolerance - FormatTypeOption ignores a bearing/azimuth unit lookup that resolves after a newer type change or props change - Cap the whole-subdivision tolerance at 1e-3 parts so very large non-integral ratios are rejected

grigasp · 2026-10-09T05:36:29Z

+afterEach(() => {
+  vi.restoreAllMocks();
+});


shouldn't this be inside the describe block?

oof yep, removed now tks

grigasp · 2026-10-09T05:53:31Z

+
+import type { FormatDefinition } from "@itwin/core-quantity";
+
+describe("AppendUnitLabel", () => {


Should file name of this, as well as FormatUnitLabel.tsx, be changed to match component name?

Yeah it can be aligned, FormatUnitLabel had to be split tho into 2 separate src files, cuz I had AppendUnitLabel and UomSeparatorSelector components in that one source file. I'm keeping the name of the test file same but splitting the source files

grigasp · 2026-10-09T05:59:32Z

    [formatProps, onChange]
  );

  const handleUseFractionDashChange = React.useCallback(


thanks, removed

grigasp · 2026-10-09T06:35:29Z

+  // Not memoized: format set providers (e.g. FormatSetFormatsProvider.addFormat) update `formats` in place, so the
+  // object identity does not change after a format is applied.
+  const formatEntries = Object.entries(activeFormatSet?.formats ?? {})
+    .filter((entry): entry is [string, FormatDefinition] => typeof entry[1] === "object" && entry[1] !== null)
+    .map(([key, formatDef]) => ({ key, formatDef, label: formatDef.label || key }));

-    return Object.entries(activeFormatSet.formats)
-      .filter(([, formatDef]) => typeof formatDef === "object" && formatDef !== null)
-      .map(([key, formatDef]) => ({
-        key,
-        formatDef: formatDef as FormatDefinition,
-        label: (formatDef as FormatDefinition).label || key,
-      }));
-  }, [activeFormatSet?.formats]);
+  const lowerSearchTerm = searchTerm.trim().toLowerCase();
+  const filteredFormats = lowerSearchTerm
+    ? formatEntries.filter(({ label }) => label.toLowerCase().includes(lowerSearchTerm))
+    : formatEntries;


If FormatSetFormatsProvider changes formats in place, there's nothing that would cause FormatSelector component to rerender and cause the format updates to be reflected. In addition, React components by design require props to be immutable, and that should be taken care of at the source rather than in a low level component.

IMO, whoever has access to FormatSetFormatsProvider and passes its format set as the activeFormatSet prop to this component, is also responsible for creating an immutable format set and keeping it up to date with the help of FormatSetFormatsProvider.onFormatsChanged.

That's a good point, okay I reverted this, and documented the instruction in the README. there's also a doccomment on the activeFormatSet prop to recommend devs to pass a new object after FormatSetFormatsProvider.onFormatsChanged

grigasp · 2026-10-09T06:37:37Z

+      await user.click(screen.getByText("Area Format"));
+      await user.click(screen.getByText("Length Format"));
+
+      expect(mockOnListItemChange).toHaveBeenLastCalledWith(updatedDefinition, "length-format");


This validates that FormatSelector raises onListItemChange with "live" format set, but it doesn't verify that it uses "live" format set for rendering. I recommend adding a new format to the format set and validating that its list item is rendered, as well as the onListItemChange is raised for it.

sounds good, I've changed the testing now

grigasp · 2026-10-09T08:16:45Z

+    return onUnitsChange;
+  }
+
+  // Civil-iTwin #2096641: DDMMSS angle formats need arc minutes, not GRAD, as the sub-unit of degrees.


not sure we should be referencing internal repo work items

agh, slip up, thanks

removed all references to internal items

grigasp · 2026-10-09T08:27:22Z

+    fireEvent.click(screen.getByRole("option", { name: "azimuth" }));
+    // Another edit lands while the lookup is pending.
+    rerender(<FormatTypeOption formatProps={{ ...formatProps, precision: 4 }} unitsProvider={unitsProvider} onChange={onChange} />);
+    await waitFor(() => expect(resolveLookup).toBeDefined());


wouldn't it make more sense to wait for resolveLookup as soon as we click on the select option?

soounds good, switched now

grigasp · 2026-10-09T08:28:03Z

+    await new Promise((resolve) => setTimeout(resolve, 0));
+
+    expect(onChange).not.toHaveBeenCalled();


feels a bit fragile, why not use await waitFor(() => expect(onChange).not.toHaveBeenCalled())?

I tried waitFor(() => expect(onChange).not.toHaveBeenCalled()) and it passes on its first try, so it would also pass if the late call never got a chance to run.

But instead I changed this so the test now resolves the lookup inside an act and waits for it to finish

This one was AI tho

grigasp · 2026-10-09T08:31:18Z

+  const requestRef = React.useRef(0);
+  React.useEffect(() => {
+    requestRef.current++;
+  }, [formatProps, unitsProvider]);


The effect invalidates requests when props change, but has no cleanup. Selecting Azimuth/Bearing and then switching to a format of another type unmounts the picker. When its lookup completes, it still calls onChange, overwriting the newly selected draft and enabling Apply.

good point, okay there's a cleanup for the effect now that will invalidate the pending lookup on unmount, and added a test

… publishes a new format set on changes FormatSelector goes back to memoizing by the formats object. Owners of a FormatSetFormatsProvider must pass a new format set after onFormatsChanged, which the README now documents and test-viewer's FormatManager does.

…ker unmounts

…dead FractionDash handler, scope test hooks

hl662 added 6 commits October 7, 2026 19:38

[quantity-formatting] Match format traits case-insensitively and keep…

546b6af

… decimal/thousands separators distinct Show unit label (and every other trait checkbox) could not be cleared when traits were persisted in PascalCase. Fixes #1824 (comma allowed as both separators).

[quantity-formatting] FormatSelector returns the latest definition af…

80dcd1e

…ter in-place format set updates Fixes #1825.

[quantity-formatting] Offer only whole subdivisions as composite sub-…

c7b67a5

…units; show default unit label placeholder DDMMSS was unreachable: GRAD was offered under degrees and ARC_MINUTE as its own sub-unit. Removes unused internal UnitDescr component; log through Logger.

[quantity-formatting] Add changesets for format editing fixes

6ec9ebe

hl662 requested review from a team as code owners October 8, 2026 03:00

hl662 mentioned this pull request Oct 8, 2026

[quantity-formatting] Optional unitsProvider, startup race fix, missing peers, exported prop types #1828

Open

hl662 added this pull request to stack #1829 October 8, 2026 03:01

Merge branch 'nam/qf-1-tooling' into nam/qf-2-format-fixes

90f86c8

Copilot AI balanced review requested due to automatic review settings October 8, 2026 13:55

Copilot started reviewing on behalf of hl662 October 8, 2026 13:56 View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Comment thread packages/quantity-formatting/src/components/quantityformat/internal/misc/FormatType.tsx

Comment thread packages/quantity-formatting/src/components/quantityformat/internal/FormatUnits.tsx Outdated

grigasp reviewed Oct 9, 2026

View reviewed changes

hl662 added 3 commits October 9, 2026 11:20

[quantity-formatting] Drop a pending azimuth lookup when the type pic…

e750024

…ker unmounts

[quantity-formatting] Review cleanups: split FormatUnitLabel, remove …

e92e8de

…dead FractionDash handler, scope test hooks

hl662 mentioned this pull request Oct 9, 2026

[Quantity Formatting] Format Settings not showing the applied/saved value when switching between formats #1825

Open


		import type { FormatDefinition } from "@itwin/core-quantity";

		describe("AppendUnitLabel", () => {

		await new Promise((resolve) => setTimeout(resolve, 0));

		expect(onChange).not.toHaveBeenCalled();

Conversation

hl662 commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

What was wrong

What

Tests

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hl662 commented Oct 8, 2026 •

edited

Loading