diff --git a/config/scripts/mobile-source-control-collation-benchmark.mjs b/config/scripts/mobile-source-control-collation-benchmark.mjs new file mode 100644 index 00000000000..e48d7dbfb0f --- /dev/null +++ b/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 ' + ) +} +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 + ) +) diff --git a/mobile/src/session/mobile-diff-review-queue.ts b/mobile/src/session/mobile-diff-review-queue.ts index 551b9c22a18..68c8e6a17e4 100644 --- a/mobile/src/session/mobile-diff-review-queue.ts +++ b/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( diff --git a/mobile/src/source-control/mobile-branch-compare.ts b/mobile/src/source-control/mobile-branch-compare.ts index 228156b18b7..0cbd7bdab79 100644 --- a/mobile/src/source-control/mobile-branch-compare.ts +++ b/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( entries: readonly TEntry[] ): MobileBranchCompareSection | 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 } } diff --git a/mobile/src/source-control/mobile-git-status.ts b/mobile/src/source-control/mobile-git-status.ts index 65fcf61e211..9c1f033f8b3 100644 --- a/mobile/src/source-control/mobile-git-status.ts +++ b/mobile/src/source-control/mobile-git-status.ts @@ -36,13 +36,6 @@ export const MOBILE_GIT_STATUS_LABELS: Record = { 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( entries: readonly TEntry[] ): MobileSourceControlSection[] { - 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 { diff --git a/mobile/src/source-control/mobile-path-sort.test.ts b/mobile/src/source-control/mobile-path-sort.test.ts new file mode 100644 index 00000000000..ffc18d5aa0d --- /dev/null +++ b/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() + } + }) +})