perf(mobile): precompute task sort keys and reuse repository collation (#20233)

2026-09-21 16:02:20 +00:00 · 2026-09-12 18:15:32 -07:00
parent ef3b7e83b9
commit 701dc2211c
5 changed files with 252 additions and 39 deletions
@@ -0,0 +1,113 @@
+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-task-sort-benchmark.mjs <baseline-ref>')
+}
+async function load(file, contents) {
+  const result = await build({
+    stdin: { contents, loader: 'ts', resolveDir: dirname(resolve(file)) },
+    bundle: true,
+    platform: 'node',
+    format: 'esm',
+    write: false,
+    logLevel: 'silent',
+    tsconfigRaw: {},
+    plugins: [
+      {
+        name: 'theme-only',
+        setup(bundler) {
+          bundler.onResolve({ filter: /mobile-tasks-dependencies$/ }, () => ({
+            path: resolve('mobile/src/theme/mobile-theme.ts')
+          }))
+        }
+      }
+    ]
+  })
+  return await import(
+    `data:text/javascript;base64,${Buffer.from(result.outputFiles[0].text).toString('base64')}`
+  )
+}
+const file = 'mobile/src/tasks/mobile-tasks-repository-presentation.ts'
+const before = await load(
+  file,
+  execFileSync('git', ['show', `${baseline}:${file}`], { encoding: 'utf8' })
+)
+const after = await load(file, readFileSync(file, 'utf8'))
+const repos = new Map()
+const results = []
+for (const count of [0, 1, 25, 1000]) {
+  const items = Array.from({ length: count }, (_, index) => ({
+    key: `item-${index}`,
+    provider: 'github',
+    title: 'task',
+    subtitle: '',
+    status: 'open',
+    updatedAt: new Date(1700000000000 - index * 100000).toISOString(),
+    source: { repoId: `repo-${index % 25}`, repoName: `Repository ${index % 25}` }
+  }))
+  for (const sort of ['updated', 'repository']) {
+    const arms = {
+      before: () =>
+        [...items].sort(
+          sort === 'repository'
+            ? (a, b) => before.compareTasksByRepository(a, b, repos)
+            : before.compareTasksByUpdated
+        ),
+      after: () => after.sortMobileTaskItems(items, sort, repos)
+    }
+    assert.deepEqual(arms.after(), arms.before())
+    const iterations = count < 100 ? 100 : 10
+    function run(arm) {
+      const start = performance.now()
+      for (let i = 0; i < iterations; i++) {
+        arms[arm]()
+      }
+      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
+    }
+    const dateParses = {}
+    const nativeParse = Date.parse
+    for (const arm of ['before', 'after']) {
+      let calls = 0
+      Date.parse = (value) => {
+        calls++
+        return nativeParse(value)
+      }
+      try {
+        arms[arm]()
+      } finally {
+        Date.parse = nativeParse
+      }
+      dateParses[arm] = calls
+    }
+    results.push({
+      count,
+      sort,
+      dateParses,
+      beforeMs: median(samples.before),
+      afterMs: median(samples.after),
+      samples
+    })
+  }
+}
+console.log(
+  JSON.stringify({ baseline, node: process.version, platform: process.platform, results }, null, 2)
+)
@@ -0,0 +1,96 @@
+import { describe, expect, it, vi } from 'vitest'
+import type { TaskItem } from './mobile-tasks-project-workspace-types'
+import type { RepoSummary } from './mobile-tasks-provider-detail-types'
+import { sortMobileTaskItems, taskRepositoryMeta } from './mobile-tasks-repository-presentation'
+import { taskTime } from './mobile-tasks-item-mapping'
+
+vi.mock('./mobile-tasks-dependencies', () => import('../theme/mobile-theme'))
+
+function task(
+  provider: TaskItem['provider'],
+  label: string,
+  updatedAt: string,
+  key: string
+): TaskItem {
+  const source =
+    provider === 'linear'
+      ? { team: { id: label, name: label }, state: { color: '' } }
+      : provider === 'gitlabTodo'
+        ? { projectPath: label }
+        : { repoId: label, repoName: label }
+  return { provider, source, updatedAt, key, title: key, subtitle: '', status: '' } as TaskItem
+}
+const repos = new Map<string, RepoSummary>([
+  ['alias', { id: 'alias', displayName: 'Álpha', path: '/repo' }]
+])
+const items = [
+  task('github', 'alias', '2026-01-01', 'a'),
+  task('gitlab', 'alpha', '2026-02-01', 'b'),
+  task('gitlabTodo', 'Zeta', 'invalid', 'c'),
+  task('linear', 'Älpha', '2026-02-01', 'd'),
+  task('github', 'alpha', '2026-02-01', 'e'),
+  task('linear', 'Zeta', '1970-01-01', 'f')
+]
+
+describe('mobile task sorting', () => {
+  it.each(['repository', 'updated'] as const)(
+    'preserves %s order, ties, fallbacks, and input identity',
+    (sort) => {
+      const compareBefore = (a: TaskItem, b: TaskItem) => {
+        const labelOrder =
+          sort === 'repository'
+            ? taskRepositoryMeta(a, repos).label.localeCompare(
+                taskRepositoryMeta(b, repos).label,
+                undefined,
+                { sensitivity: 'base' }
+              )
+            : 0
+        return labelOrder || taskTime(b.updatedAt) - taskTime(a.updatedAt)
+      }
+      const expected = [...items].sort(compareBefore)
+      const input = Object.freeze([...items])
+      const actual = sortMobileTaskItems(input, sort, repos)
+      expect(actual).toEqual(expected)
+      actual.forEach((item, index) => expect(item).toBe(expected[index]))
+      expect(input).toEqual(items)
+      repos.set('alias', { id: 'alias', displayName: 'zzzz', path: '/repo' })
+      try {
+        expect(sortMobileTaskItems(input, sort, repos)).toEqual([...items].sort(compareBefore))
+      } finally {
+        repos.set('alias', { id: 'alias', displayName: 'Álpha', path: '/repo' })
+      }
+    }
+  )
+
+  it.each(['repository', 'updated'] as const)('computes %s keys only once per item', (sort) => {
+    const parse = vi.spyOn(Date, 'parse')
+    const getRepo = vi.spyOn(repos, 'get')
+    const localeCompare = vi.spyOn(String.prototype, 'localeCompare')
+    try {
+      sortMobileTaskItems(items, sort, repos)
+      expect(parse).toHaveBeenCalledTimes(items.length)
+      expect(getRepo).toHaveBeenCalledTimes(sort === 'repository' ? 3 : 0)
+      expect(localeCompare).not.toHaveBeenCalled()
+    } finally {
+      parse.mockRestore()
+      getRepo.mockRestore()
+      localeCompare.mockRestore()
+    }
+  })
+
+  it('skips setup for empty/singleton arrays while returning a fresh array', () => {
+    const parse = vi.spyOn(Date, 'parse')
+    const collator = vi.spyOn(Intl, 'Collator')
+    try {
+      expect(sortMobileTaskItems([], 'repository', repos)).toEqual([])
+      const one = [items[0]]
+      expect(sortMobileTaskItems(one, 'repository', repos)).toEqual(one)
+      expect(sortMobileTaskItems(one, 'updated', repos)).not.toBe(one)
+      expect(parse).not.toHaveBeenCalled()
+      expect(collator).not.toHaveBeenCalled()
+    } finally {
+      parse.mockRestore()
+      collator.mockRestore()
+    }
+  })
+})
@@ -16,50 +16,51 @@ const hash = (parts: string[] | string): string =>
    .update(Array.isArray(parts) ? parts.join('\n') : parts)
    .digest('hex')

-const PRE_REFACTOR_SCREEN_HOOKS = '42174315a76c475d09dcb7209af4481f01258c4c9dc012127ff07a893d8cd291'
-const PRE_REFACTOR_DIFF_HOOKS = '93c7189b32bed8456cc51814fffa8ce80cf62011ef968a9d53ddec2b9686f58f'
-const PRE_REFACTOR_STATEMENTS = '9323fbee7c3806f37de42578ba73ce659c786c0ed5f8b6bcbc321b201ca50a73'
-const PRE_REFACTOR_DECLARATIONS = 'cff54172af17a877789be1479c2eb6ca97d83c3e31dd831cd59395962f2b4c4a'
-const PRE_REFACTOR_SEMANTICS = '5219d210d6f274e9ce2716a37c4c6fc4860a736a80f059ab6e89da6123043263'
-const PRE_REFACTOR_STYLES = '1db6af69c791d9963928541ad5310942fcbda6d984b422c90b6eb92b6816579a'
-const PRE_REFACTOR_RENDER_TREE = '2111145136b1e4fbca150d4792d735a90e992488e9934cfc1a8b8f3be981f39f'
+// Task-sort key precomputation is covered by mobile-task-sort.test.ts; other source guards remain.
+const EXPECTED_SCREEN_HOOKS = '6bbbbbcea5faec9cef22c9e7b4ada24b69517bd7d6bd03259e5f239eca036a9d'
+const EXPECTED_DIFF_HOOKS = '93c7189b32bed8456cc51814fffa8ce80cf62011ef968a9d53ddec2b9686f58f'
+const EXPECTED_STATEMENTS = '71b6d3e02022568b0300cba3fa33e19b39509b9bfd987af6bb6f7d378a75939c'
+const EXPECTED_DECLARATIONS = 'e79d877078362cef301ac8bdc46fef07eb0ac9414e5b8f752a879559ec6dbde1'
+const EXPECTED_SEMANTICS = '4758ba019e4ff7cadd7ee02338719fa4fc4e1443e34cc290842819cfa1a70181'
+const EXPECTED_STYLES = '1db6af69c791d9963928541ad5310942fcbda6d984b422c90b6eb92b6816579a'
+const EXPECTED_RENDER_TREE = '2111145136b1e4fbca150d4792d735a90e992488e9934cfc1a8b8f3be981f39f'

 describe('Mobile Tasks refactor parity', () => {
  it('preserves recursively flattened hook and dependency order', () => {
    const screenHooks = readFlattenedMobileTasksHookSignatures('MobileTasksScreen')
    expect(screenHooks).toHaveLength(350)
-    expect(hash(screenHooks)).toBe(PRE_REFACTOR_SCREEN_HOOKS)
+    expect(hash(screenHooks)).toBe(EXPECTED_SCREEN_HOOKS)

    const diffHooks = readFlattenedMobileTasksHookSignatures('GitHubPrFileDiff')
    expect(diffHooks).toHaveLength(3)
-    expect(hash(diffHooks)).toBe(PRE_REFACTOR_DIFF_HOOKS)
+    expect(hash(diffHooks)).toBe(EXPECTED_DIFF_HOOKS)
  })

  it('preserves every screen statement in execution order', () => {
    const statements = readFlattenedMobileTasksCoreStatements()
    expect(statements).toHaveLength(417)
-    expect(hash(statements)).toBe(PRE_REFACTOR_STATEMENTS)
+    expect(hash(statements)).toBe(EXPECTED_STATEMENTS)
  })

  it('preserves every moved top-level declaration', () => {
    const declarations = readMobileTasksDeclarationSignatures()
-    expect(declarations).toHaveLength(193)
-    expect(hash(declarations)).toBe(PRE_REFACTOR_DECLARATIONS)
+    expect(declarations).toHaveLength(192)
+    expect(hash(declarations)).toBe(EXPECTED_DECLARATIONS)
  })

  it('preserves RPC calls, runtime strings, and JSX host signatures', () => {
    const semantics = readMobileTasksSemanticSource()
-    expect(semantics.split('\n')).toHaveLength(3_499)
-    expect(hash(semantics)).toBe(PRE_REFACTOR_SEMANTICS)
+    expect(semantics.split('\n')).toHaveLength(3_500)
+    expect(hash(semantics)).toBe(EXPECTED_SEMANTICS)
  })

  it('preserves render expressions and event handlers in tree order', () => {
    const tokens = readFlattenedMobileTasksRenderTokens()
    expect(tokens).toHaveLength(35_195)
-    expect(hash(tokens)).toBe(PRE_REFACTOR_RENDER_TREE)
+    expect(hash(tokens)).toBe(EXPECTED_RENDER_TREE)
  })

  it('preserves every StyleSheet property and value', () => {
-    expect(hash(readMobileTasksStyleSource())).toBe(PRE_REFACTOR_STYLES)
+    expect(hash(readMobileTasksStyleSource())).toBe(EXPECTED_STYLES)
  })
 })
@@ -2,6 +2,7 @@ import { colors } from './mobile-tasks-dependencies'
 import { taskTime } from './mobile-tasks-item-mapping'
 import type { TaskItem } from './mobile-tasks-project-workspace-types'
 import type { RepoSummary } from './mobile-tasks-provider-detail-types'
+import type { TaskSort } from './mobile-tasks-view-state-types'

 export function isFailedGitHubCheck(check: { conclusion?: string | null }): boolean {
  return ['failure', 'cancelled', 'timed_out'].includes(check.conclusion ?? '')
@@ -64,17 +65,25 @@ export function taskRepositoryMeta(
  }
 }

-export function compareTasksByUpdated(a: TaskItem, b: TaskItem): number {
-  return taskTime(b.updatedAt) - taskTime(a.updatedAt)
-}
-
-export function compareTasksByRepository(
-  a: TaskItem,
-  b: TaskItem,
+export function sortMobileTaskItems(
+  items: readonly TaskItem[],
+  sort: TaskSort,
  reposById: Map<string, RepoSummary>
-): number {
-  const aRepo = taskRepositoryMeta(a, reposById)
-  const bRepo = taskRepositoryMeta(b, reposById)
-  const repoComparison = aRepo.label.localeCompare(bRepo.label, undefined, { sensitivity: 'base' })
-  return repoComparison || compareTasksByUpdated(a, b)
+): TaskItem[] {
+  if (items.length < 2) {
+    return [...items]
+  }
+  const byRepository = sort === 'repository'
+  const collator = byRepository ? new Intl.Collator(undefined, { sensitivity: 'base' }) : null
+  return items
+    .map((item) => ({
+      item,
+      updatedAt: taskTime(item.updatedAt),
+      repositoryLabel: byRepository ? taskRepositoryMeta(item, reposById).label : ''
+    }))
+    .sort(
+      (a, b) =>
+        (collator?.compare(a.repositoryLabel, b.repositoryLabel) ?? 0) || b.updatedAt - a.updatedAt
+    )
+    .map(({ item }) => item)
 }
@@ -8,8 +8,7 @@ import {
  type RepoSummary,
  SORT_OPTIONS,
  type TaskListEntry,
-  compareTasksByRepository,
-  compareTasksByUpdated,
+  sortMobileTaskItems,
  getRepoBadgeColor,
  hasGitHubIssueSourceChoice,
  issueSourceSlug,
@@ -133,15 +132,10 @@ export function useMobileTasksPickerProjection(model: DetailCommentRenderersMode
      })),
    [workspaceRepos]
  )
-  const sortedItems = useMemo(() => {
-    const next = [...items]
-    if (taskSort === 'repository') {
-      next.sort((a, b) => compareTasksByRepository(a, b, reposById))
-    } else {
-      next.sort(compareTasksByUpdated)
-    }
-    return next
-  }, [items, reposById, taskSort])
+  const sortedItems = useMemo(
+    () => sortMobileTaskItems(items, taskSort, reposById),
+    [items, reposById, taskSort]
+  )
  const displayedEntries = useMemo<TaskListEntry[]>(() => {
    if (taskSort !== 'repository') {
      return sortedItems.map((item) => ({ type: 'item', key: item.key, item }))