From 0c42285eb2e11e7dd48c5e14d1f900962cb3f7b2 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 12 Oct 2026 20:54:20 -0400 Subject: [PATCH] perf(mobile): skip re-rendering unchanged worktree rows (#28058) * perf(mobile): skip re-rendering unchanged worktree rows Rows reach the host list freshly parsed and copied on every changed worktree.ps poll, so the row memo never skipped and every mounted row re-rendered each poll. The row memo now compares the item by value with the catalog's areWorktreesEqual, extended to every Worktree and agent row field, and every other prop by identity. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): compare worktree rows by structural equality The hand-written Worktree field list needed a completeness guard that no compiler checks, since mobile test files sit outside tsc. A generic structural equality covers every field by construction, so a new field can never be skipped and freeze a row. Rows memo on it directly: the item and repo icon compare by value, callbacks and `now` by identity. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- mobile/src/components/WorktreeListRow.test.ts | 47 +++++- mobile/src/components/WorktreeListRow.tsx | 55 ++----- .../host-workspace-list-row-memo.test.tsx | 125 ++++++++++++++++ .../src/worktree/structural-equality.test.ts | 40 ++++++ mobile/src/worktree/structural-equality.ts | 23 +++ .../worktree/worktree-list-snapshot.test.ts | 7 - mobile/src/worktree/worktree-list-snapshot.ts | 134 +----------------- 7 files changed, 243 insertions(+), 188 deletions(-) create mode 100644 mobile/src/host-screen/host-workspace-list-row-memo.test.tsx create mode 100644 mobile/src/worktree/structural-equality.test.ts create mode 100644 mobile/src/worktree/structural-equality.ts diff --git a/mobile/src/components/WorktreeListRow.test.ts b/mobile/src/components/WorktreeListRow.test.ts index 722202d004b..8111c01cafe 100644 --- a/mobile/src/components/WorktreeListRow.test.ts +++ b/mobile/src/components/WorktreeListRow.test.ts @@ -11,7 +11,8 @@ import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { RuntimeWorktreeAgentRow } from '../../../src/shared/runtime-types' import { WorktreeAgentRow } from './WorktreeAgentRow' -import { WorktreeListRow, type WorktreeListRowItem } from './WorktreeListRow' +import type { Worktree } from '../worktree/workspace-list-types' +import { WorktreeListRow } from './WorktreeListRow' const { agentSpinnerRender, agentStateDotRender } = vi.hoisted(() => ({ agentSpinnerRender: vi.fn(), @@ -56,10 +57,7 @@ vi.mock('./WorktreeMetaGlyphs', () => ({ WorktreeMetaGlyphs: () => null })) -type TestItem = WorktreeListRowItem & { - status: 'working' | 'active' | 'permission' | 'done' | 'inactive' - lastOutputAt: number -} +type TestItem = Worktree & { status: NonNullable } const stableRepoIcon = { type: 'emoji', emoji: 'o' } as const let updateSibling: Dispatch> = () => undefined @@ -113,7 +111,11 @@ function agent(overrides: Partial = {}): RuntimeWorktre const baseItem: TestItem = { worktreeId: 'worktree-1', + repoId: 'repo-1', repo: 'orca', + path: 'mobile-list', + hasAttachedPty: true, + isPinned: false, branch: 'feature/mobile-list', displayName: 'mobile-list', liveTerminalCount: 1, @@ -172,6 +174,41 @@ describe('memoized worktree rows', () => { expect(agentSpinnerRender).toHaveBeenCalledTimes(++expectedRenders) }) + it('skips a value-equal item and re-renders for time, callbacks and nested agent fields', async () => { + const onPress = () => undefined + const row = (item: TestItem, overrides: { now?: number; onPress?: () => void } = {}) => + createElement(WorktreeListRow, { + item, + isReadOnly: false, + now: overrides.now ?? 2_000, + repoColor: '#000000', + repoIcon: { type: 'emoji', emoji: 'o' }, + status: item.status, + onPress: overrides.onPress ?? onPress + }) + await act(async () => { + renderer = create(row(baseItem)) + }) + expect(agentSpinnerRender).toHaveBeenCalledTimes(1) + + // A fresh parse of the same row, with a fresh repo icon object, is not a change. + await act(async () => renderer!.update(row(structuredClone(baseItem)))) + expect(agentSpinnerRender).toHaveBeenCalledTimes(1) + + await act(async () => renderer!.update(row(baseItem, { now: 32_000 }))) + expect(agentSpinnerRender).toHaveBeenCalledTimes(2) + + const nestedChange = structuredClone(baseItem) + nestedChange.agents![0]!.lastAssistantMessage = 'Tests pass' + await act(async () => renderer!.update(row(nestedChange, { now: 32_000 }))) + expect(agentSpinnerRender).toHaveBeenCalledTimes(3) + + await act(async () => + renderer!.update(row(nestedChange, { now: 32_000, onPress: () => undefined })) + ) + expect(agentSpinnerRender).toHaveBeenCalledTimes(4) + }) + it('memoizes agent rows without hiding agent updates', async () => { const firstAgent = agent() await act(async () => { diff --git a/mobile/src/components/WorktreeListRow.tsx b/mobile/src/components/WorktreeListRow.tsx index 0da38990012..c564f6e24fd 100644 --- a/mobile/src/components/WorktreeListRow.tsx +++ b/mobile/src/components/WorktreeListRow.tsx @@ -9,12 +9,12 @@ import { Server } from 'lucide-react-native' import { Pressable, StyleSheet, Text, View } from 'react-native' -import { parseExecutionHostId, type ExecutionHostId } from '../../../src/shared/execution-host' -import type { AgentWorkingMode } from '../../../src/shared/agent-status-types' -import type { RuntimeWorktreeAgentRow } from '../../../src/shared/runtime-types' +import { parseExecutionHostId } from '../../../src/shared/execution-host' import type { MobileRenderableRepoIcon } from '../host-screen/host-screen-reply-schema' import { triggerMediumImpact } from '../platform/haptics' import { colors, radii, spacing, typography } from '../theme/mobile-theme' +import type { Worktree } from '../worktree/workspace-list-types' +import { areStructurallyEqual } from '../worktree/structural-equality' import { AgentSpinner } from './AgentSpinner' import { MobileRepoIcon } from './MobileRepoIcon' import { WorktreeAgentList } from './WorktreeAgentList' @@ -26,43 +26,10 @@ function displayBranch(branch: string): string { return branch.replace(/^refs\/heads\//, '') } -// Minimal row shape needed for rendering — a structural subset of the screen's -// Worktree so this component stays decoupled from the screen's local type. -export type WorktreeListRowItem = { - workspaceKind?: 'git' | 'folder-workspace' - worktreeId: string - hostId?: ExecutionHostId - /** Present only when the list spans hosts; names the host this row runs on. */ - hostContextLabel?: string - /** Resolved host for the display label; present when legacy rows omit hostId. */ - hostContextHostId?: ExecutionHostId - /** The host's health word, e.g. "Disconnected"; absent while the host is healthy. */ - hostContextHealthLabel?: string - repo: string - branch: string - displayName: string - path?: string - liveTerminalCount: number - preview: string - unread: boolean - isActive?: boolean - linkedPR: { number: number; state: string } | null - linkedIssue?: number | null - linkedLinearIssue?: string | null - linkedGitLabMR?: number | null - linkedGitLabIssue?: number | null - comment?: string - lineageDepth?: number - lineageChildCount?: number - lineageCollapsed?: boolean - agents?: RuntimeWorktreeAgentRow[] - workingMode?: AgentWorkingMode -} - type WorktreeRollupStatus = 'working' | 'active' | 'permission' | 'done' | 'inactive' -type Props = { - item: T +type Props = { + item: Worktree isReadOnly: boolean now: number repoColor: string @@ -71,12 +38,12 @@ type Props = { // omits its own repo icon+name to avoid the redundant "📁 orca" on every row. hideRepo?: boolean status: WorktreeRollupStatus - onPress: (item: T) => void - onLongPress?: (item: T) => void - onToggleLineage?: (item: T) => void + onPress: (item: Worktree) => void + onLongPress?: (item: Worktree) => void + onToggleLineage?: (item: Worktree) => void } -function WorktreeListRowComponent({ +function WorktreeListRowComponent({ item, isReadOnly, now, @@ -87,7 +54,7 @@ function WorktreeListRowComponent({ onPress, onLongPress, onToggleLineage -}: Props) { +}: Props) { const isFolderWorkspace = item.workspaceKind === 'folder-workspace' const folderMeta = item.comment?.trim() || item.path || 'Folder' const metaText = isFolderWorkspace ? folderMeta : displayBranch(item.branch) @@ -239,7 +206,7 @@ function WorktreeListRowComponent({ ) } -export const WorktreeListRow = memo(WorktreeListRowComponent) as typeof WorktreeListRowComponent +export const WorktreeListRow = memo(WorktreeListRowComponent, areStructurallyEqual) const styles = StyleSheet.create({ worktreeRow: { diff --git a/mobile/src/host-screen/host-workspace-list-row-memo.test.tsx b/mobile/src/host-screen/host-workspace-list-row-memo.test.tsx new file mode 100644 index 00000000000..01b76916ca1 --- /dev/null +++ b/mobile/src/host-screen/host-workspace-list-row-memo.test.tsx @@ -0,0 +1,125 @@ +import { createElement, Fragment, type ReactNode } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { describe, expect, it, vi } from 'vitest' +import type { Section, Worktree } from '../worktree/workspace-list-types' + +const { rowRenders } = vi.hoisted(() => ({ rowRenders: vi.fn() })) + +vi.mock('react-native', () => ({ + Pressable: 'Pressable', + RefreshControl: 'RefreshControl', + // Renders every row through the list's own renderItem, as a mounted SectionList would. + SectionList: ({ + sections, + keyExtractor, + renderItem + }: { + sections: Section[] + keyExtractor: (item: Worktree) => string + renderItem: (info: { item: Worktree }) => ReactNode + }) => + sections.flatMap((section) => + section.data.map((item) => + createElement(Fragment, { key: keyExtractor(item) }, renderItem({ item })) + ) + ), + StyleSheet: { create: (styles: T) => styles }, + Text: 'Text', + View: 'View' +})) +vi.mock('lucide-react-native', () => ({})) +vi.mock('../platform/haptics', () => ({ triggerMediumImpact: vi.fn() })) +vi.mock('../components/AgentSpinner', () => ({ AgentSpinner: () => null })) +vi.mock('../components/MobileRepoIcon', () => ({ MobileRepoIcon: () => null })) +vi.mock('../components/WorktreeAgentList', () => ({ WorktreeAgentList: () => null })) +// The row always renders the glyphs, so each call is one row render, named by its comment. +vi.mock('../components/WorktreeMetaGlyphs', () => ({ + prStateColor: () => '#000000', + WorktreeMetaGlyphs: ({ comment }: { comment?: string }) => { + rowRenders(comment) + return null + } +})) +// Off this embedded path; mocked for their native imports. +vi.mock('../components/MobileSearchField', () => ({})) +vi.mock('../components/NewWorkspaceFab', () => ({ FAB_SIZE: 56 })) + +import { buildSections } from '../worktree/workspace-list-sections' +import { HostWorkspaceList } from './host-workspace-list' + +function worktree(id: string, overrides: Partial = {}): Worktree { + return { + worktreeId: `repo-1::${id}`, + repoId: 'repo-1', + repo: 'orca', + branch: `feature/${id}`, + displayName: id, + path: id, + liveTerminalCount: 1, + hasAttachedPty: true, + preview: '$ codex', + unread: false, + isPinned: false, + linkedPR: null, + comment: id, + status: 'active', + ...overrides + } +} + +const noop = () => {} +const stable = { ref: { current: null }, repoColors: new Map(), repoIcons: new Map() } + +function listProps(worktrees: Worktree[]) { + // Each poll re-parses the payload, so rows are fresh objects even when nothing changed. + const polled: Worktree[] = JSON.parse(JSON.stringify(worktrees)) + const filters = { + filterRepoIds: new Set(), + hideSleeping: false, + hideDefaultBranch: false + } + const sections = buildSections(polled, 'name', filters, '', 'none', new Set()) + const controller = { + actions: { openWorktreeSession: noop, openNewWorktreeModal: noop }, + activeWorktreeScroll: { sectionListRef: stable.ref, onScrollToIndexFailed: noop }, + catalog: { refreshing: false, onRefresh: noop }, + connState: 'connected', + displayWorktrees: polled, + embedded: true, + insets: { bottom: 0 }, + isReadOnly: false, + now: 1_000, + reconnectAttempts: 0, + relayRecovery: { pairingRejected: false }, + sectionsResult: { rawSections: sections, sections, uniqueRepoColors: stable.repoColors }, + settings: { activeFilterCount: 0, toggleWorktreeLineage: noop }, + state: { + actionError: '', + setActionTarget: noop, + groupMode: 'none', + repoIconsByName: stable.repoIcons, + search: '', + showSearch: false, + worktreesLoaded: true + } + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the list reads only the controller fields above on this connected, embedded, error-free path. + return { controller } as unknown as Parameters[0] +} + +describe('host list rows across polls', () => { + it('re-renders only the row whose worktree changed', () => { + const rows = [worktree('a'), worktree('b'), worktree('c')] + let tree: ReactTestRenderer | null = null + act(() => { + tree = create(createElement(HostWorkspaceList, listProps(rows))) + }) + rowRenders.mockClear() + + const changed = [rows[0]!, { ...rows[1]!, preview: 'Running tests' }, rows[2]!] + act(() => tree!.update(createElement(HostWorkspaceList, listProps(changed)))) + + expect(rowRenders.mock.calls.map(([comment]) => comment)).toEqual(['b']) + act(() => tree!.unmount()) + }) +}) diff --git a/mobile/src/worktree/structural-equality.test.ts b/mobile/src/worktree/structural-equality.test.ts new file mode 100644 index 00000000000..dd1ae264b6b --- /dev/null +++ b/mobile/src/worktree/structural-equality.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from 'vitest' +import { areStructurallyEqual } from './structural-equality' + +describe('areStructurallyEqual', () => { + it('equates value-equal nested objects and arrays', () => { + const value = () => ({ id: 'a', agents: [{ state: 'working', mainAgent: { stopping: true } }] }) + expect(areStructurallyEqual(value(), value())).toBe(true) + expect(areStructurallyEqual([1, [2, { x: null }]], [1, [2, { x: null }]])).toBe(true) + }) + + it('detects a changed nested value', () => { + expect( + areStructurallyEqual({ agents: [{ state: 'working' }] }, { agents: [{ state: 'done' }] }) + ).toBe(false) + expect(areStructurallyEqual([1, 2], [1, 2, 3])).toBe(false) + }) + + it('counts keys, so an added or removed key is a change even when undefined', () => { + expect(areStructurallyEqual({ a: 1 }, { a: 1, b: 2 })).toBe(false) + expect(areStructurallyEqual({ a: 1, b: undefined }, { a: 1 })).toBe(false) + expect(areStructurallyEqual({ a: 1 }, { b: 1 })).toBe(false) + }) + + it('compares functions by identity', () => { + const press = () => {} + expect(areStructurallyEqual({ press }, { press })).toBe(true) + expect(areStructurallyEqual({ press }, { press: () => {} })).toBe(false) + }) + + it('does not equate an array with an object of the same entries', () => { + expect(areStructurallyEqual(['a'], { 0: 'a' })).toBe(false) + expect(areStructurallyEqual([], {})).toBe(false) + }) + + it('compares primitives with Object.is', () => { + expect(areStructurallyEqual(Number.NaN, Number.NaN)).toBe(true) + expect(areStructurallyEqual(null, undefined)).toBe(false) + expect(areStructurallyEqual(0, '0')).toBe(false) + }) +}) diff --git a/mobile/src/worktree/structural-equality.ts b/mobile/src/worktree/structural-equality.ts new file mode 100644 index 00000000000..5e59aa88c4b --- /dev/null +++ b/mobile/src/worktree/structural-equality.ts @@ -0,0 +1,23 @@ +// Why: polls re-parse rows into fresh objects, so lists and rows compare by value to skip renders. +export function areStructurallyEqual(a: unknown, b: unknown): boolean { + if (Object.is(a, b)) { + return true + } + if ( + !a || + !b || + typeof a !== 'object' || + typeof b !== 'object' || + Array.isArray(a) !== Array.isArray(b) + ) { + return false + } + const leftEntries = Object.entries(a) + const rightEntries = new Map(Object.entries(b)) + return ( + leftEntries.length === rightEntries.size && + leftEntries.every( + ([key, value]) => rightEntries.has(key) && areStructurallyEqual(value, rightEntries.get(key)) + ) + ) +} diff --git a/mobile/src/worktree/worktree-list-snapshot.test.ts b/mobile/src/worktree/worktree-list-snapshot.test.ts index b575007b002..80d4a7fcd09 100644 --- a/mobile/src/worktree/worktree-list-snapshot.test.ts +++ b/mobile/src/worktree/worktree-list-snapshot.test.ts @@ -222,11 +222,4 @@ describe('areWorktreeListsEqual', () => { expect(areWorktreeListsEqual(first, second)).toBe(false) }) - - it('treats missing and empty agent arrays as equivalent for rendering', () => { - const first = [worktree({ agents: undefined })] - const second = [worktree({ agents: [] })] - - expect(areWorktreeListsEqual(first, second)).toBe(true) - }) }) diff --git a/mobile/src/worktree/worktree-list-snapshot.ts b/mobile/src/worktree/worktree-list-snapshot.ts index 3fe50cec69a..f28198b0adf 100644 --- a/mobile/src/worktree/worktree-list-snapshot.ts +++ b/mobile/src/worktree/worktree-list-snapshot.ts @@ -1,139 +1,9 @@ -import type { RuntimeWorktreeAgentRow } from '../../../src/shared/runtime-types' +import { areStructurallyEqual } from './structural-equality' import type { Worktree } from './workspace-list-sections' export function areWorktreeListsEqual( left: readonly Worktree[], right: readonly Worktree[] ): boolean { - if (left === right) { - return true - } - if (left.length !== right.length) { - return false - } - for (let index = 0; index < left.length; index += 1) { - if (!areWorktreesEqual(left[index]!, right[index]!)) { - return false - } - } - return true -} - -function areWorktreesEqual(left: Worktree, right: Worktree): boolean { - return ( - left.worktreeId === right.worktreeId && - left.repoId === right.repoId && - left.repo === right.repo && - left.branch === right.branch && - (left.hostId ?? null) === (right.hostId ?? null) && - (left.terminalPlatform ?? null) === (right.terminalPlatform ?? null) && - left.displayName === right.displayName && - (left.workspaceStatus ?? null) === (right.workspaceStatus ?? null) && - (left.sortOrder ?? null) === (right.sortOrder ?? null) && - (left.manualOrder ?? null) === (right.manualOrder ?? null) && - (left.lastActivityAt ?? null) === (right.lastActivityAt ?? null) && - (left.createdAt ?? null) === (right.createdAt ?? null) && - left.path === right.path && - (left.isArchived ?? false) === (right.isArchived ?? false) && - (left.isMainWorktree ?? false) === (right.isMainWorktree ?? false) && - (left.hasHostSidebarActivity ?? false) === (right.hasHostSidebarActivity ?? false) && - (left.worktreeInstanceId ?? null) === (right.worktreeInstanceId ?? null) && - (left.lineageWorktreeInstanceId ?? null) === (right.lineageWorktreeInstanceId ?? null) && - (left.parentWorktreeInstanceId ?? null) === (right.parentWorktreeInstanceId ?? null) && - (left.parentWorktreeId ?? null) === (right.parentWorktreeId ?? null) && - areStringArraysEqual(left.childWorktreeIds ?? [], right.childWorktreeIds ?? []) && - left.liveTerminalCount === right.liveTerminalCount && - left.hasAttachedPty === right.hasAttachedPty && - left.preview === right.preview && - left.unread === right.unread && - (left.lastOutputAt ?? null) === (right.lastOutputAt ?? null) && - left.isPinned === right.isPinned && - (left.isActive ?? false) === (right.isActive ?? false) && - (left.linkedIssue ?? null) === (right.linkedIssue ?? null) && - (left.linkedLinearIssue ?? null) === (right.linkedLinearIssue ?? null) && - (left.linkedGitLabMR ?? null) === (right.linkedGitLabMR ?? null) && - (left.linkedGitLabIssue ?? null) === (right.linkedGitLabIssue ?? null) && - (left.comment ?? '') === (right.comment ?? '') && - (left.status ?? null) === (right.status ?? null) && - (left.workingMode ?? null) === (right.workingMode ?? null) && - arePullRequestsEqual(left.linkedPR, right.linkedPR) && - areAgentRowsEqual(left.agents ?? [], right.agents ?? []) - ) -} - -function areStringArraysEqual(left: readonly string[], right: readonly string[]): boolean { - if (left === right) { - return true - } - if (left.length !== right.length) { - return false - } - for (let index = 0; index < left.length; index += 1) { - if (left[index] !== right[index]) { - return false - } - } - return true -} - -function arePullRequestsEqual(left: Worktree['linkedPR'], right: Worktree['linkedPR']): boolean { - if (left === right) { - return true - } - if (!left || !right) { - return false - } - return left.number === right.number && left.state === right.state -} - -function areAgentRowsEqual( - left: readonly RuntimeWorktreeAgentRow[], - right: readonly RuntimeWorktreeAgentRow[] -): boolean { - if (left === right) { - return true - } - if (left.length !== right.length) { - return false - } - for (let index = 0; index < left.length; index += 1) { - const a = left[index]! - const b = right[index]! - if ( - a.paneKey !== b.paneKey || - a.parentPaneKey !== b.parentPaneKey || - a.state !== b.state || - a.workingMode !== b.workingMode || - a.agentType !== b.agentType || - a.prompt !== b.prompt || - a.lastAssistantMessage !== b.lastAssistantMessage || - a.toolName !== b.toolName || - a.toolInput !== b.toolInput || - a.interrupted !== b.interrupted || - !areMainAgentsEqual(a.mainAgent, b.mainAgent) || - a.stateStartedAt !== b.stateStartedAt || - a.updatedAt !== b.updatedAt - ) { - return false - } - } - return true -} - -function areMainAgentsEqual( - left: RuntimeWorktreeAgentRow['mainAgent'], - right: RuntimeWorktreeAgentRow['mainAgent'] -): boolean { - if (left === right) { - return true - } - if (!left || !right) { - return false - } - return ( - left.state === right.state && - left.outcome === right.outcome && - left.stateStartedAt === right.stateStartedAt && - left.stopping === right.stopping - ) + return areStructurallyEqual(left, right) }