src/addBenchmarkEntry.ts (1)

20-27: Use a more concise previous-bench lookup.

Equivalent and more readable; previously suggested in past reviews.

-        // Get the last suite which has different commit ID for alert comment
-        for (const e of [...suites].reverse()) {
-            if (e.commit.id !== benchEntry.commit.id) {
-                prevBench = e;
-                break;
-            }
-        }
+        // Get the last suite which has different commit ID for alert comment
+        prevBench = suites.slice().reverse().find((e) => e.commit.id !== benchEntry.commit.id) ?? null;

test/extractRangeInfo.spec.ts (1)

24-30: Add coverage for single-digit, decimals, exponent, and surrounding whitespace.

These cases ensure robustness and prevent regressions after tightening the regex.

     it('should NOT extract range with invalid number', () => {
         expect(extractRangeInfo('± boo')).toBeUndefined();
         expect(extractRangeInfo('+- boo')).toBeUndefined();
         expect(extractRangeInfo('± 1boo')).toBeUndefined();
         expect(extractRangeInfo('+- 1boo')).toBeUndefined();
     });
+    it('should extract single-digit integer', () => {
+        expect(extractRangeInfo('±2')).toEqual({ value: 2, prefix: '±' });
+    });
+    it('should extract decimal and preserve prefix space', () => {
+        expect(extractRangeInfo('± 0.5')).toEqual({ value: 0.5, prefix: '± ' });
+    });
+    it('should extract scientific notation', () => {
+        expect(extractRangeInfo('+-1e-3')).toEqual({ value: 1e-3, prefix: '+-' });
+    });
+    it('should tolerate surrounding whitespace', () => {
+        expect(extractRangeInfo('  ±20  ')).toEqual({ value: 20, prefix: '±' });
+    });

src/normalizeValueByUnit.ts (1)

1-14: Support microsecond symbol variants and stray whitespace/case in unit strings.

Many toolchains output µs/μs. Canonicalize inputs before indexing so normalization doesn’t silently no-op.

-export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number {
+export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number {
     for (const units of SUPPORTED_UNITS) {
-        const prevUnitIndex = units.indexOf(prevUnit);
-        const currentUnitIndex = units.indexOf(currentUnit);
+        const prev = canonicalizeUnit(prevUnit);
+        const curr = canonicalizeUnit(currentUnit);
+        const prevUnitIndex = units.indexOf(prev);
+        const currentUnitIndex = units.indexOf(curr);
 
         if (prevUnitIndex >= 0 && currentUnitIndex >= 0) {
             const unitDiff = prevUnitIndex - currentUnitIndex;
 
             return value * UNIT_CONVERSION_MULTIPLIER ** unitDiff;
         }
     }
 
     return value;
 }

Add this helper in the module:

function canonicalizeUnit(u: string): string {
    // normalize case/space and micro symbol in any context (e.g., "µs/iter", "ops/µs")
    return u.trim().toLowerCase().replace(/µs|μs/g, 'us');
}

Please confirm whether your inputs can include "µs/μs" or mixed-case (e.g., "MS"). If yes, this change and accompanying tests are warranted.

test/normalizeValueByUnit.spec.ts (1)

40-42: Add tests for microsecond symbol aliases and whitespace/case tolerance.

Validates the canonicalization change and real-world inputs.

   it('NOT normalize when new unit is not supported', () => {
       expect(normalizeValueByUnit('unknown1', 'unknown2', 12)).toBe(12);
   });
+  it('handles microsecond symbol variants', () => {
+      expect(normalizeValueByUnit('ms', 'µs', 12)).toBe(0.012);
+      expect(normalizeValueByUnit('us', 'μs', 12)).toBe(12);
+      expect(normalizeValueByUnit('ops/µs', 'ops/s', 12_000_000)).toBe(12);
+  });
+  it('tolerates surrounding whitespace and case', () => {
+      expect(normalizeValueByUnit(' MS ', ' S ', 12)).toBe(0.012);
+  });

src/normalizeBenchmark.ts (1)

9-19: Avoid O(n²) lookup by pre-indexing previous benches.

Small win, but cheap and clearer.

-    return {
-        ...currentBench,
-        benches: currentBench.benches
-            .map((currentBenchResult) => ({
-                currentBenchResult,
-                prevBenchResult: prevBench.benches.find((result) => result.name === currentBenchResult.name),
-            }))
-            .map(({ currentBenchResult, prevBenchResult }) =>
-                normalizeBenchmarkResult(prevBenchResult, currentBenchResult),
-            ),
-    };
+    const prevByName = new Map(prevBench.benches.map((b) => [b.name, b]));
+    return {
+        ...currentBench,
+        benches: currentBench.benches.map((curr) =>
+            normalizeBenchmarkResult(prevByName.get(curr.name), curr),
+        ),
+    };

test/normalizeBenchmarkResult.spec.ts (2)

120-127: Keep the range marker consistent within the "+-" section.

This block is under "with range (+-)" but uses "±" in inputs/expected. It still passes (parser accepts both), but for consistency/readability, consider using "+-" here as well.

-                    { name: 'Bench', value: 900, unit: 'unknown1', range: '± 20' },
-                    { name: 'Bench', value: 1.01, unit: 'unknown2', range: '± 0.1' },
+                    { name: 'Bench', value: 900, unit: 'unknown1', range: '+- 20' },
+                    { name: 'Bench', value: 1.01, unit: 'unknown2', range: '+- 0.1' },

---

3-4: Add tests for ops/ normalization (e.g., ops/ms ↔ ops/s).

PR claims support for ops/, but no assertions cover it. Please add cases proving values scale correctly when previous/current units are ops/ns|us|ms|s.

Example to add:

it('normalizes ops/<time> to previous unit', () => {
  // prev uses ops/ms, current uses ops/s → current should scale by 1/1000 to ops/ms
  expect(
    normalizeBenchmarkResult(
      { name: 'Bench', value: 500, unit: 'ops/ms', range: '± 50' },
      { name: 'Bench', value: 600000, unit: 'ops/s', range: '± 60000' },
    ),
  ).toEqual({ name: 'Bench', value: 600, unit: 'ops/ms', range: '± 60' });
});

Also applies to: 83-84, 130-131

src/normalizeBenchmarkResult.ts (3)

7-8: Typo in parameter name: currenBenchResult → currentBenchResult.

Purely a readability/consistency fix; avoids confusion in future diffs and reviews.

-export function normalizeBenchmarkResult(
-    prevBenchResult: BenchmarkResult | undefined | null,
-    currenBenchResult: BenchmarkResult,
-): BenchmarkResult {
+export function normalizeBenchmarkResult(
+    prevBenchResult: BenchmarkResult | undefined | null,
+    currentBenchResult: BenchmarkResult,
+): BenchmarkResult {
-    if (!prevBenchResult) {
-        return currenBenchResult;
+    if (!prevBenchResult) {
+        return currentBenchResult;
     }
-    const currentUnit = currenBenchResult.unit;
-    const currentRange = currenBenchResult.range;
+    const currentUnit = currentBenchResult.unit;
+    const currentRange = currentBenchResult.range;
     const currentRangeInfo = extractRangeInfo(currentRange);
-    const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value);
-    const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit;
+    const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value);
+    const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit;
     return {
-        ...currenBenchResult,
+        ...currentBenchResult,
         value: normalizedValue,
         unit: normalizedUnit,
         range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange,
     };
 }

---

20-25: Consider formatting normalized range values (and possibly values) to stable precision.

Stringifying raw floats can produce long decimals (e.g., 0.30000000000000004). A small formatter (e.g., trim to max 6 significant digits) would make outputs cleaner and comments more readable.

Also applies to: 31-32

---

18-25: Guard: normalize range only when value was normalized.

If unit conversion isn’t applicable (unsupported units) but extractRangeInfo succeeds, we still rebuild range. Harmless, but you can skip rebuilding when normalizedUnit === currentUnit for slightly simpler behavior.

-    const normalizedRangeInfo = currentRangeInfo
+    const normalizedRangeInfo = currentRangeInfo && normalizedUnit !== currentUnit
         ? {
               prefix: currentRangeInfo.prefix,
               value: normalizeValueByUnit(prevUnit, currentUnit, currentRangeInfo.value),
           }
         : undefined;

src/addBenchmarkEntry.ts (1)

15-19: Minor: also normalize when creating the first suite (no behavior change).

Calling normalizeBenchmark with null prev keeps behavior identical but unifies code paths (always push normalizedCurrentBench).

-    if (entries[benchName] === undefined) {
-        entries[benchName] = [benchEntry];
+    if (entries[benchName] === undefined) {
+        normalizedCurrentBench = normalizeBenchmark(null, benchEntry);
+        entries[benchName] = [normalizedCurrentBench];
         core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`);

test/write.spec.ts (1)

420-466: Add ops/ normalization scenarios.

Please add cases where previous is ops/ms (or ops/us) and current is ops/s (and vice versa) to validate conversion direction and range scaling in alerts and failure paths.

Also applies to: 1291-1313

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/canonicalizeUnit.ts (1)

1-3: Nit: simplify the microseconds regex

Use a character class; it’s shorter and equivalent.

-export function canonicalizeUnit(u: string): string {
-    return u.trim().toLowerCase().replace(/µs|μs/g, 'us');
-}
+export function canonicalizeUnit(u: string): string {
+    return u.trim().toLowerCase().replace(/[µμ]s/g, 'us');
+}

src/normalizeValueByUnit.ts (1)

3-15: Minor perf/readability: canonicalize once, outside the loop

No behavior change; avoids redundant work per group.

-export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number {
-    for (const units of SUPPORTED_UNITS) {
-        const prev = canonicalizeUnit(prevUnit);
-        const current = canonicalizeUnit(currentUnit);
-        const prevUnitIndex = units.indexOf(prev);
-        const currentUnitIndex = units.indexOf(current);
+export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number {
+    const prev = canonicalizeUnit(prevUnit);
+    const current = canonicalizeUnit(currentUnit);
+    for (const units of SUPPORTED_UNITS) {
+        const prevUnitIndex = units.indexOf(prev);
+        const currentUnitIndex = units.indexOf(current);

src/normalizeBenchmarkResult.ts (1)

5-11: Typo: rename currenBenchResult → currentBenchResult

Purely cosmetic; improves readability.

 export function normalizeBenchmarkResult(
     prevBenchResult: BenchmarkResult | undefined | null,
-    currenBenchResult: BenchmarkResult,
+    currentBenchResult: BenchmarkResult,
 ): BenchmarkResult {
     if (!prevBenchResult) {
-        return currenBenchResult;
+        return currentBenchResult;
     }
 
     const prevUnit = prevBenchResult.unit;
-    const currentUnit = currenBenchResult.unit;
-    const currentRange = currenBenchResult.range;
+    const currentUnit = currentBenchResult.unit;
+    const currentRange = currentBenchResult.range;
@@
-    const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value);
-    const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit;
+    const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value);
+    const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit;
@@
     return {
-        ...currenBenchResult,
+        ...currentBenchResult,
         value: normalizedValue,
         unit: normalizedUnit,
         range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange,
     };

Also applies to: 13-15, 18-19, 27-31

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.