Address CodeRabbit feedback

Author: rebeccaalpert

packages/react-core/src/helpers/__tests__/util.test.ts

@@ -114,17 +114,64 @@ test('formatBreakpointMods', () => {
   expect(formatBreakpointMods({ default: 'column', lg: 'row' }, styles)).toEqual('pf-m-column pf-m-row-on-lg');
 });
 
-test('parseLocalizedDecimal accepts dot and comma decimal separators', () => {
+test('parseLocalizedDecimal accepts locale-formatted decimals', () => {
   const enFormatter = new Intl.NumberFormat('en');
   const esFormatter = new Intl.NumberFormat('es');
+  const deFormatter = new Intl.NumberFormat('de-DE');
 
   expect(parseLocalizedDecimal('50.2', enFormatter)).toBe(50.2);
   expect(parseLocalizedDecimal('50,2', esFormatter)).toBe(50.2);
-  expect(parseLocalizedDecimal('1.234,56', esFormatter)).toBe(1234.56);
+  expect(parseLocalizedDecimal('12.345,67', esFormatter)).toBe(12345.67);
   expect(parseLocalizedDecimal('1,234.56', enFormatter)).toBe(1234.56);
+  expect(parseLocalizedDecimal('1.234,56', deFormatter)).toBe(1234.56);
   expect(parseLocalizedDecimal('', enFormatter)).toBeNaN();
 });
 
+test('parseLocalizedDecimal rejects mismatched separators', () => {
+  const enFormatter = new Intl.NumberFormat('en-US');
+  const deFormatter = new Intl.NumberFormat('de-DE');
+  const deNoGrouping = new Intl.NumberFormat('de-DE', { useGrouping: false });
+
+  // Comma used as decimal in en-US would otherwise silently merge to 502
+  expect(parseLocalizedDecimal('50,2', enFormatter)).toBeNaN();
+  // Dot used as decimal in de-DE would otherwise silently merge to 502
+  expect(parseLocalizedDecimal('50.2', deFormatter)).toBeNaN();
+  // Mixed separators for de-DE
+  expect(parseLocalizedDecimal('1,234.56', deFormatter)).toBeNaN();
+  // Without a reported group separator, alternate '.' must not be stripped/merged
+  expect(parseLocalizedDecimal('1.234,56', deNoGrouping)).toBeNaN();
+  expect(parseLocalizedDecimal('50,2', deNoGrouping)).toBe(50.2);
+});
+
+test('parseLocalizedDecimal accepts locale-specific grouping patterns', () => {
+  const enINFormatter = new Intl.NumberFormat('en-IN');
+  const enUSFormatter = new Intl.NumberFormat('en-US');
+  const esFormatter = new Intl.NumberFormat('es');
+
+  expect(parseLocalizedDecimal('12,34,567', enINFormatter)).toBe(1234567);
+  expect(parseLocalizedDecimal('12,345', enINFormatter)).toBe(12345);
+  expect(parseLocalizedDecimal('1,234', enINFormatter)).toBe(1234);
+  expect(parseLocalizedDecimal('1,23,4567', enINFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('50,2', enUSFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('1.234,56', esFormatter)).toBeNaN();
+});
+
+test('parseLocalizedDecimal rejects malformed numeric tokens', () => {
+  const enFormatter = new Intl.NumberFormat('en-US');
+
+  expect(parseLocalizedDecimal('12abc', enFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('1,2abc', enFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('12-3', enFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('--12', enFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('+-12', enFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('12.3.4', enFormatter)).toBeNaN();
+  expect(parseLocalizedDecimal('abc', enFormatter)).toBeNaN();
+
+  expect(parseLocalizedDecimal('-50.2', enFormatter)).toBe(-50.2);
+  expect(parseLocalizedDecimal('+12', enFormatter)).toBe(12);
+  expect(parseLocalizedDecimal('1,234.56', enFormatter)).toBe(1234.56);
+});
+
 test('formatLocalizedDecimal uses locale decimal separator', () => {
   expect(formatLocalizedDecimal(50.2, new Intl.NumberFormat('en-US'))).toBe('50.2');
   expect(formatLocalizedDecimal(50.2, new Intl.NumberFormat('de-DE'))).toBe('50,2');

packages/react-core/src/helpers/util.ts

@@ -592,8 +592,8 @@ export const getInlineStartProperty = (
 };
 
 /**
- * Parses a decimal input string, accepting both comma and dot as the decimal separator.
- * Given the locale, it discovers the locale's separators and integers using a reference value.
+ * Parses a decimal input string using the given formatter's locale separators.
+ * Grouping is removed only when formatToParts reports a group separator.
  *
  * @param {string} value - The input string to parse
  * @param {Intl.NumberFormat} formatter - The Intl.NumberFormat instance to use for formatting
@@ -605,31 +605,57 @@ export const parseLocalizedDecimal = (value: string, formatter: Intl.NumberForma
   }
 
   const parts = formatter.formatToParts(12345.6);
-  const groupSymbol = parts.find((p) => p.type === 'group')?.value || ',';
+  // Only strip grouping when the formatter actually reports a group separator.
+  // Never fall back to ',' — that can match comma-decimal locales and eat the decimal.
+  const groupSymbol = parts.find((p) => p.type === 'group')?.value;
   const decimalSymbol = parts.find((p) => p.type === 'decimal')?.value || '.';
+  const normalizedString = value.trim();
 
-  // in case of non-Arabic numerals
-  const digitParts = formatter.formatToParts(1234567890);
-  const localDigits = digitParts
-    .filter((p) => p.type === 'integer')
-    .map((p) => p.value)
-    .join('');
-  let normalizedString = value;
-  if (localDigits.length === 10 && localDigits !== '1234567890') {
-    const standardDigits = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '0'];
-    const digitMap = new Map();
-    [...localDigits].forEach((char, idx) => digitMap.set(char, standardDigits[idx]));
-    normalizedString = [...value].map((char) => digitMap.get(char) || char).join('');
+  // Reject '.' / ',' when they are neither this locale's decimal nor its reported group separator
+  // (e.g. mixed-separator de-DE input like "1,234.56").
+  for (const sep of ['.', ','] as const) {
+    if (sep !== decimalSymbol && sep !== groupSymbol && normalizedString.includes(sep)) {
+      return NaN;
+    }
   }
 
-  const escapeRegex = (str: string) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+  const decimalIndex = normalizedString.indexOf(decimalSymbol);
+  if (decimalIndex !== -1 && normalizedString.indexOf(decimalSymbol, decimalIndex + decimalSymbol.length) !== -1) {
+    return NaN;
+  }
+
+  let intPart = decimalIndex === -1 ? normalizedString : normalizedString.slice(0, decimalIndex);
+  const fracPart = decimalIndex === -1 ? undefined : normalizedString.slice(decimalIndex + decimalSymbol.length);
+
+  let sign = '';
+  if (intPart.startsWith('-') || intPart.startsWith('+')) {
+    sign = intPart[0] === '-' ? '-' : '';
+    intPart = intPart.slice(1);
+  }
+
+  if (fracPart !== undefined && groupSymbol && fracPart.includes(groupSymbol)) {
+    return NaN;
+  }
 
-  const cleaned = normalizedString
-    .replace(new RegExp(escapeRegex(groupSymbol), 'g'), '')
-    .replace(new RegExp(escapeRegex(decimalSymbol), 'g'), '.')
-    .replace(/[^0-9.-]/g, '');
+  // If grouping appears, require locale-valid groups so values like en-US "50,2" are invalid
+  // instead of silently merging into 502, while accepting locales such as en-IN "12,34,567".
+  if (groupSymbol && intPart.includes(groupSymbol)) {
+    const digitsOnly = intPart.split(groupSymbol).join('');
+    if (!/^\d+$/.test(digitsOnly) || formatter.format(Number(digitsOnly)) !== intPart) {
+      return NaN;
+    }
+    intPart = digitsOnly;
+  }
+
+  if (!/^\d*$/.test(intPart) || (fracPart !== undefined && !/^\d*$/.test(fracPart))) {
+    return NaN;
+  }
+
+  if (intPart === '' && (fracPart === undefined || fracPart === '')) {
+    return NaN;
+  }
 
-  return parseFloat(cleaned);
+  return parseFloat(`${sign}${intPart || '0'}${fracPart !== undefined ? `.${fracPart}` : ''}`);
 };
 
 /**