From cefff10e100ef9cc3fefe9c78b2a2610908a98f7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 04:27:20 -0700 Subject: [PATCH] perf(sidebar): stop building host projections the row model throws away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getMixedHostContextLabels built a label map for every visible worktree and then returned undefined unless two distinct hosts existed — so a single-host install, the common case, paid the whole build on every buildRows rebuild. Decide first, then build only when it is mixed. getHostWorktreeCounts repeated getHostWorktreeIds' dedupe walk to compute a number that is exactly the id list's length, and both loops built the identity string twice per row. Derive the counts and hoist the identity. --- .../grouping/host-labels.test.ts | 62 +++++++++++++++++++ .../worktree-list/grouping/host-labels.ts | 24 +++---- .../worktree/host-context-labels.test.ts | 53 ++++++++++++++++ src/shared/worktree/host-context-labels.ts | 28 +++++++-- 4 files changed, 151 insertions(+), 16 deletions(-) create mode 100644 src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.test.ts create mode 100644 src/shared/worktree/host-context-labels.test.ts diff --git a/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.test.ts b/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.test.ts new file mode 100644 index 00000000000..da40bf5cb6f --- /dev/null +++ b/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from 'vitest' +import type { ExecutionHostId } from '../../../../../../shared/execution-host' +import type { Repo } from '../../../../../../shared/repo-types' +import type { Worktree } from '../../../../../../shared/worktree-types' +import { getHostWorktreeCounts, getHostWorktreeIds } from './host-labels' + +const LOCAL = 'local' as ExecutionHostId + +function makeRepos(hostByRepo: Record): Map { + return new Map( + Object.entries(hostByRepo).map(([id, executionHostId]) => [ + id, + { id, path: `/${id}`, ...(executionHostId ? { executionHostId } : {}) } as Repo + ]) + ) +} + +function makeWorktrees(rows: { id: string; repoId: string }[]): Worktree[] { + return rows.map((row) => row as Worktree) +} + +describe('host worktree counts and ids', () => { + it('reports a count equal to the length of each host id list', () => { + const repoMap = makeRepos({ + 'repo-local': undefined, + 'repo-remote': 'ssh:other' as ExecutionHostId + }) + const worktrees = makeWorktrees([ + { id: 'a', repoId: 'repo-local' }, + { id: 'b', repoId: 'repo-local' }, + { id: 'c', repoId: 'repo-remote' } + ]) + + const counts = getHostWorktreeCounts(worktrees, repoMap, LOCAL) + const ids = getHostWorktreeIds(worktrees, repoMap, LOCAL) + + expect(ids).toBeDefined() + expect(counts).toBeDefined() + for (const [hostId, hostIds] of ids ?? []) { + expect(counts?.get(hostId), `count for ${hostId}`).toBe(hostIds.length) + } + expect([...(counts?.keys() ?? [])].sort()).toEqual([...(ids?.keys() ?? [])].sort()) + }) + + it('counts a repeated host identity once', () => { + const repoMap = makeRepos({ 'repo-local': undefined }) + const worktrees = makeWorktrees([ + { id: 'a', repoId: 'repo-local' }, + { id: 'a', repoId: 'repo-local' }, + { id: 'b', repoId: 'repo-local' } + ]) + + expect(getHostWorktreeCounts(worktrees, repoMap, LOCAL)?.get(LOCAL)).toBe(2) + expect(getHostWorktreeIds(worktrees, repoMap, LOCAL)?.get(LOCAL)).toEqual(['a', 'b']) + }) + + it('returns undefined for an empty lane', () => { + const repoMap = makeRepos({}) + expect(getHostWorktreeCounts([], repoMap, LOCAL)).toBeUndefined() + expect(getHostWorktreeIds([], repoMap, LOCAL)).toBeUndefined() + }) +}) diff --git a/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts b/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts index d1c5c8be2d4..925b04eed41 100644 --- a/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts +++ b/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts @@ -153,17 +153,17 @@ export function getHostWorktreeCounts( if (worktrees.length === 0) { return undefined } - const counts = new Map() + // Derived from the id map rather than repeating its dedupe walk: every caller asks for both, + // and a host's count is exactly the length of its id list by construction. // Dedup by host, not by bare id: the same id on two hosts is two workspaces // and has to be counted under each of them (STA-4343). - const seenIdentities = new Set() - for (const worktree of worktrees) { - if (seenIdentities.has(getWorktreeHostIdentity(worktree))) { - continue - } - seenIdentities.add(getWorktreeHostIdentity(worktree)) - const hostId = getWorktreeExecutionHostId(worktree, repoMap.get(worktree.repoId), defaultHostId) - counts.set(hostId, (counts.get(hostId) ?? 0) + 1) + const idsByHost = getHostWorktreeIds(worktrees, repoMap, defaultHostId) + if (!idsByHost) { + return undefined + } + const counts = new Map() + for (const [hostId, ids] of idsByHost) { + counts.set(hostId, ids.length) } return counts } @@ -179,10 +179,12 @@ export function getHostWorktreeIds( const idsByHost = new Map() const seenIdentities = new Set() for (const worktree of worktrees) { - if (seenIdentities.has(getWorktreeHostIdentity(worktree))) { + // Hoisted: the identity string was built twice per row, once to test and once to record. + const identity = getWorktreeHostIdentity(worktree) + if (seenIdentities.has(identity)) { continue } - seenIdentities.add(getWorktreeHostIdentity(worktree)) + seenIdentities.add(identity) const hostId = getWorktreeExecutionHostId(worktree, repoMap.get(worktree.repoId), defaultHostId) const ids = idsByHost.get(hostId) ?? [] ids.push(worktree.id) diff --git a/src/shared/worktree/host-context-labels.test.ts b/src/shared/worktree/host-context-labels.test.ts new file mode 100644 index 00000000000..21523822ebe --- /dev/null +++ b/src/shared/worktree/host-context-labels.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from 'vitest' +import type { ExecutionHostId } from '../execution-host' +import { getMixedHostContextLabels } from './host-context-labels' + +type Item = { id: string; hostId: ExecutionHostId } + +function argsFor(items: Item[], counters: { hostIdReads: number; identityReads: number }) { + return { + getHostId: (item: Item): ExecutionHostId => { + counters.hostIdReads += 1 + return item.hostId + }, + getIdentity: (item: Item): string => { + counters.identityReads += 1 + return item.id + } + } +} + +describe('getMixedHostContextLabels', () => { + it('returns undefined without building any labels when every item shares one host', () => { + const items: Item[] = Array.from({ length: 50 }, (_, index) => ({ + id: `wt-${index}`, + hostId: 'local' as ExecutionHostId + })) + const counters = { hostIdReads: 0, identityReads: 0 } + + expect(getMixedHostContextLabels(items, argsFor(items, counters))).toBeUndefined() + // The label map was built for all 50 and thrown away before; now nothing is built. + expect(counters.identityReads).toBe(0) + }) + + it('labels every item once a second host appears, wherever it appears', () => { + for (const lateIndex of [1, 25, 49]) { + const items: Item[] = Array.from({ length: 50 }, (_, index) => ({ + id: `wt-${index}`, + hostId: (index === lateIndex ? 'ssh:other' : 'local') as ExecutionHostId + })) + const counters = { hostIdReads: 0, identityReads: 0 } + + const labels = getMixedHostContextLabels(items, argsFor(items, counters)) + + expect(labels, `second host at index ${lateIndex}`).toBeDefined() + expect(labels?.size).toBe(50) + expect(labels?.has(`wt-${lateIndex}`)).toBe(true) + } + }) + + it('returns undefined for an empty list', () => { + const counters = { hostIdReads: 0, identityReads: 0 } + expect(getMixedHostContextLabels([], argsFor([], counters))).toBeUndefined() + }) +}) diff --git a/src/shared/worktree/host-context-labels.ts b/src/shared/worktree/host-context-labels.ts index 4b9481c441b..d3ab9ed2ed7 100644 --- a/src/shared/worktree/host-context-labels.ts +++ b/src/shared/worktree/host-context-labels.ts @@ -83,12 +83,30 @@ export function getMixedHostContextLabels( sources?: HostContextLabelSources } ): Map | undefined { - const labelsByIdentity = new Map() - const hostIds = new Set() + // Why two passes: on a single-host install the map is built for every item and then discarded. + // Deciding first costs one getHostId per item and skips the label work entirely. + let firstHostId: ExecutionHostId | undefined + let isMixed = false for (const item of items) { const hostId = args.getHostId(item) - hostIds.add(hostId) - labelsByIdentity.set(args.getIdentity(item), getHostContextLabel(hostId, args.sources)) + if (firstHostId === undefined) { + firstHostId = hostId + continue + } + if (hostId !== firstHostId) { + isMixed = true + break + } } - return hostIds.size > 1 ? labelsByIdentity : undefined + if (!isMixed) { + return undefined + } + const labelsByIdentity = new Map() + for (const item of items) { + labelsByIdentity.set( + args.getIdentity(item), + getHostContextLabel(args.getHostId(item), args.sources) + ) + } + return labelsByIdentity }