perf(mobile): reuse numeric collators across source control sorts (#20224)

config/scripts/mobile-source-control-collation-benchmark.mjs

@@ -0,0 +1,110 @@
+import assert from 'node:assert/strict'
+import { execFileSync } from 'node:child_process'
+import { readFileSync } from 'node:fs'
+import { dirname, resolve } from 'node:path'
+import { performance } from 'node:perf_hooks'
+import { build } from 'esbuild'
+import { buildCounterbalancedSchedule } from './counterbalanced-benchmark-schedule.mjs'
+
+const baseline = process.argv[2]
+if (!baseline) {
+  throw new Error(
+    'Usage: node config/scripts/mobile-source-control-collation-benchmark.mjs <baseline-ref>'
+  )
+}
+async function load(file, contents, name) {
+  const result = await build({
+    stdin: { contents, loader: 'ts', resolveDir: dirname(resolve(file)) },
+    bundle: true,
+    platform: 'node',
+    format: 'esm',
+    write: false,
+    logLevel: 'silent',
+    tsconfigRaw: {}
+  })
+  return (
+    await import(
+      `data:text/javascript;base64,${Buffer.from(result.outputFiles[0].text).toString('base64')}`
+    )
+  )[name]
+}
+// Match git's path order, including its numeric-looking names, instead of inflating sort work with a shuffle.
+const paths = execFileSync('git', ['ls-files', '-z'], { maxBuffer: 16 * 1024 * 1024 })
+  .toString()
+  .split('\0')
+  .filter(Boolean)
+const results = []
+for (const [file, name] of [
+  ['mobile/src/source-control/mobile-git-status.ts', 'buildMobileSourceControlSections'],
+  ['mobile/src/source-control/mobile-branch-compare.ts', 'buildMobileBranchCompareSection'],
+  ['mobile/src/session/mobile-diff-review-queue.ts', 'buildMobileDiffReviewQueue']
+]) {
+  const arms = {
+    before: await load(
+      file,
+      execFileSync('git', ['show', `${baseline}:${file}`], { encoding: 'utf8' }),
+      name
+    ),
+    after: await load(file, readFileSync(file, 'utf8'), name)
+  }
+  for (const count of [0, 1, 17, 63, 1000]) {
+    const step = Math.max(1, Math.floor(paths.length / Math.max(1, count)))
+    const entries = Array.from({ length: count }, (_, index) => ({
+      path: paths[index * step],
+      area: 'unstaged',
+      status: 'modified',
+      ...(index % 37 === 0 ? { conflictStatus: 'unresolved' } : {})
+    }))
+    const input =
+      name === 'buildMobileDiffReviewQueue'
+        ? {
+            worktreeId: 'workspace',
+            statusEntries: entries,
+            branchEntries: [],
+            comments: [],
+            reviewState: { version: 1, files: {} }
+          }
+        : entries
+    assert.deepEqual(arms.after(input), arms.before(input))
+    const iterations = count < 100 ? 100 : 10
+    function run(arm) {
+      const start = performance.now()
+      for (let index = 0; index < iterations; index++) {
+        arms[arm](input)
+      }
+      return (performance.now() - start) / iterations
+    }
+    const samples = { before: [], after: [] }
+    run('before')
+    run('after')
+    for (const pair of buildCounterbalancedSchedule(10, 'before', 'after')) {
+      for (const arm of pair) {
+        samples[arm].push(run(arm))
+      }
+    }
+    function median(values) {
+      const sorted = [...values].sort((a, b) => a - b)
+      return (sorted[4] + sorted[5]) / 2
+    }
+    results.push({
+      name,
+      count,
+      beforeMs: median(samples.before),
+      afterMs: median(samples.after),
+      samples
+    })
+  }
+}
+console.log(
+  JSON.stringify(
+    {
+      baseline,
+      node: process.version,
+      platform: process.platform,
+      locale: new Intl.Collator().resolvedOptions().locale,
+      results
+    },
+    null,
+    2
+  )
+)

mobile/src/session/mobile-diff-review-queue.ts

@@ -233,26 +233,25 @@ function branchEntryToQueueItem(
   }
 }
 
-function compareQueueItems(
-  first: MobileDiffReviewQueueItem,
-  second: MobileDiffReviewQueueItem
-): number {
-  return (
-    SCOPE_SORT_ORDER[first.scope] - SCOPE_SORT_ORDER[second.scope] ||
-    Number(first.isGeneratedOrLockFile) - Number(second.isGeneratedOrLockFile) ||
-    first.filePath.localeCompare(second.filePath, undefined, { numeric: true })
-  )
-}
-
 export function buildMobileDiffReviewQueue(
   input: BuildMobileDiffReviewQueueInput
 ): MobileDiffReviewQueueItem[] {
-  return [
+  const queue = [
     ...input.statusEntries.map((entry) =>
       statusEntryToQueueItem(entry, input.comments, input.reviewState)
     ),
     ...input.branchEntries.map((entry) => branchEntryToQueueItem(entry, input))
-  ].sort(compareQueueItems)
+  ]
+  if (queue.length > 1) {
+    const collator = new Intl.Collator(undefined, { numeric: true })
+    queue.sort(
+      (first, second) =>
+        SCOPE_SORT_ORDER[first.scope] - SCOPE_SORT_ORDER[second.scope] ||
+        Number(first.isGeneratedOrLockFile) - Number(second.isGeneratedOrLockFile) ||
+        collator.compare(first.filePath, second.filePath)
+    )
+  }
+  return queue
 }
 
 export function filterMobileDiffReviewQueue(

mobile/src/source-control/mobile-branch-compare.ts

@@ -15,22 +15,20 @@ export type MobileBranchCompareSection<
   data: TEntry[]
 }
 
-function compareBranchEntries(
-  a: MobileGitBranchChangeEntry,
-  b: MobileGitBranchChangeEntry
-): number {
-  return a.path.localeCompare(b.path, undefined, { numeric: true })
-}
-
 export function buildMobileBranchCompareSection<TEntry extends MobileGitBranchChangeEntry>(
   entries: readonly TEntry[]
 ): MobileBranchCompareSection<TEntry> | null {
   if (entries.length === 0) {
     return null
   }
+  const data = [...entries]
+  if (data.length > 1) {
+    const collator = new Intl.Collator(undefined, { numeric: true })
+    data.sort((a, b) => collator.compare(a.path, b.path))
+  }
   return {
     title: 'Committed on Branch',
-    data: [...entries].sort(compareBranchEntries)
+    data
   }
 }
 

mobile/src/source-control/mobile-git-status.ts

@@ -36,13 +36,6 @@ export const MOBILE_GIT_STATUS_LABELS: Record<MobileGitFileStatus, string> = {
   copied: 'C'
 }
 
-function compareGitStatusEntries(a: MobileGitStatusEntry, b: MobileGitStatusEntry): number {
-  return (
-    getConflictSortRank(a) - getConflictSortRank(b) ||
-    a.path.localeCompare(b.path, undefined, { numeric: true })
-  )
-}
-
 function getConflictSortRank(entry: MobileGitStatusEntry): number {
   if (entry.conflictStatus === 'unresolved') {
     return 0
@@ -56,11 +49,21 @@ function getConflictSortRank(entry: MobileGitStatusEntry): number {
 export function buildMobileSourceControlSections<TEntry extends MobileGitStatusEntry>(
   entries: readonly TEntry[]
 ): MobileSourceControlSection<TEntry>[] {
-  return AREA_ORDER.map((area) => ({
+  const sections = AREA_ORDER.map((area) => ({
     area,
     title: AREA_TITLES[area],
-    data: entries.filter((entry) => entry.area === area).sort(compareGitStatusEntries)
+    data: entries.filter((entry) => entry.area === area)
   })).filter((section) => section.data.length > 0)
+  if (sections.some((section) => section.data.length > 1)) {
+    const collator = new Intl.Collator(undefined, { numeric: true })
+    for (const section of sections) {
+      section.data.sort(
+        (a, b) =>
+          getConflictSortRank(a) - getConflictSortRank(b) || collator.compare(a.path, b.path)
+      )
+    }
+  }
+  return sections
 }
 
 export function countStagedEntries(entries: readonly MobileGitStatusEntry[]): number {

mobile/src/source-control/mobile-path-sort.test.ts

@@ -0,0 +1,114 @@
+import { describe, expect, it, vi } from 'vitest'
+import { buildMobileDiffReviewQueue } from '../session/mobile-diff-review-queue'
+import { buildMobileBranchCompareSection } from './mobile-branch-compare'
+import { buildMobileSourceControlSections, type MobileGitStatusEntry } from './mobile-git-status'
+
+const paths = [
+  'file10.ts',
+  'file2.ts',
+  'file02.ts',
+  'café.ts',
+  'cafe\u0301.ts',
+  'Z.ts',
+  'a.ts',
+  'Ä.ts',
+  'İ.ts',
+  'package-lock.json'
+]
+const entries = paths.flatMap((path, index) =>
+  (['staged', 'untracked', 'unstaged'] as const).map((area) => ({
+    path,
+    oldPath: `old-${index}`,
+    area,
+    status: 'modified' as const,
+    conflictStatus:
+      index % 3 === 0
+        ? ('unresolved' as const)
+        : index % 3 === 1
+          ? ('resolved_locally' as const)
+          : undefined
+  }))
+)
+const reviewInput = {
+  worktreeId: 'workspace',
+  statusEntries: entries,
+  branchEntries: entries.filter((entry) => entry.area === 'staged'),
+  comments: [],
+  reviewState: { version: 1 as const, files: {} }
+}
+const comparePath = (a: string, b: string) => a.localeCompare(b, undefined, { numeric: true })
+const conflictRank = (entry: MobileGitStatusEntry) =>
+  entry.conflictStatus === 'unresolved' ? 0 : entry.conflictStatus === 'resolved_locally' ? 1 : 2
+
+describe('mobile path sort collation', () => {
+  it('preserves numeric ties, Unicode equivalence, conflict rank, and section order', () => {
+    const original = [...entries]
+    const sections = buildMobileSourceControlSections(entries)
+    expect(sections.map((section) => section.area)).toEqual(['unstaged', 'untracked', 'staged'])
+    for (const section of sections) {
+      expect(section.data).toEqual(
+        entries
+          .filter((entry) => entry.area === section.area)
+          .sort((a, b) => conflictRank(a) - conflictRank(b) || comparePath(a.path, b.path))
+      )
+    }
+    expect(buildMobileBranchCompareSection(entries)?.data).toEqual(
+      [...entries].sort((a, b) => comparePath(a.path, b.path))
+    )
+    expect(entries).toEqual(original)
+  })
+
+  it('preserves review scope and generated-file precedence before numeric path order', () => {
+    const unsorted = [
+      ...entries.flatMap((entry) =>
+        buildMobileDiffReviewQueue({ ...reviewInput, statusEntries: [entry], branchEntries: [] })
+      ),
+      ...reviewInput.branchEntries.flatMap((entry) =>
+        buildMobileDiffReviewQueue({ ...reviewInput, statusEntries: [], branchEntries: [entry] })
+      )
+    ]
+    const scopeRank = { unstaged: 0, staged: 1, branch: 2 }
+    const expected = unsorted.sort(
+      (a, b) =>
+        scopeRank[a.scope] - scopeRank[b.scope] ||
+        Number(a.isGeneratedOrLockFile) - Number(b.isGeneratedOrLockFile) ||
+        comparePath(a.filePath, b.filePath)
+    )
+    expect(buildMobileDiffReviewQueue(reviewInput)).toEqual(expected)
+  })
+
+  it('resolves the current locale once per populated sort and never per comparison', () => {
+    const localeCompare = vi.spyOn(String.prototype, 'localeCompare')
+    const NativeCollator = Intl.Collator
+    const collator = vi.spyOn(Intl, 'Collator').mockImplementation(function (locales, options) {
+      return new NativeCollator(locales, options)
+    })
+    try {
+      for (let call = 0; call < 2; call++) {
+        buildMobileSourceControlSections(entries)
+        buildMobileBranchCompareSection(entries)
+        buildMobileDiffReviewQueue(reviewInput)
+      }
+      expect(localeCompare).not.toHaveBeenCalled()
+      expect(collator).toHaveBeenCalledTimes(6)
+      expect(collator).toHaveBeenCalledWith(undefined, { numeric: true })
+    } finally {
+      localeCompare.mockRestore()
+      collator.mockRestore()
+    }
+  })
+
+  it('does not initialize collation for empty or singleton collections', () => {
+    const collator = vi.spyOn(Intl, 'Collator')
+    try {
+      for (const rows of [[], [entries[0]]]) {
+        buildMobileSourceControlSections(rows)
+        buildMobileBranchCompareSection(rows)
+        buildMobileDiffReviewQueue({ ...reviewInput, statusEntries: rows, branchEntries: [] })
+      }
+      expect(collator).not.toHaveBeenCalled()
+    } finally {
+      collator.mockRestore()
+    }
+  })
+})