From 3e0dddaa3e65641f592c64fde49bb7356eea9ab7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:08:07 -0700 Subject: [PATCH] perf(renderer): keep project catalog identity on SSH readoption * perf(renderer): keep project catalog identity on SSH readoption SshPane calls recordSshRepoReadoptions on every Manage-pane importConfig(), often with []. The publisher always rebuilt projects and projectHostSetups via projectCompatibilityFromRepos then mergeProjectHostSetupCompatibility and spread the new objects into set(), so WeakMap and Object.is selectors missed even when nothing was re-adopted. Fetch paths already reconcile through mergeFetchedProjectCompatibilityForHost, but that helper is host-scoped and this write is all-repos. Reconcile the merged rows against the previous catalog with the same identity keys, omit unchanged keys from set(), and skip the work entirely when both the incoming and pending readoption lists are empty. Tests reuse the production-shaped nested repo fixture and the structuredClone fetch path. A pending-only readoption stays identity-stable, and removing the two reconcile calls turns that case red. Co-authored-by: Orca * type(store): mark pending SSH readoptions readonly Widen pendingSshRepoReadoptions, recordSshRepoReadoptions, and the merge/reconcile helpers to readonly so a reused pending queue cannot be mutated in place after a no-op catalog reconcile. Co-authored-by: Orca * perf(renderer): drop redundant catalogRowsUnchanged after reconcile reconcileCatalogRows already hands back the previous array when nothing moved, so the extra element-wise compare could never disagree. Pin the empty-readoption early return with a whole-state identity assertion, which is the only guard the previous test could not distinguish. Co-authored-by: Orca --------- Co-authored-by: Orca --- .../sidebar/add-remote-host-ssh-actions.ts | 4 +- .../slices/repos-refresh-identity.test.ts | 76 ++++++++++++++++++- src/renderer/src/store/slices/repos.ts | 30 ++++++-- .../store/slices/superseded-ssh-repo-rows.ts | 4 +- 4 files changed, 104 insertions(+), 10 deletions(-) diff --git a/src/renderer/src/components/sidebar/add-remote-host-ssh-actions.ts b/src/renderer/src/components/sidebar/add-remote-host-ssh-actions.ts index 7ad8c6f5f86..6b585156749 100644 --- a/src/renderer/src/components/sidebar/add-remote-host-ssh-actions.ts +++ b/src/renderer/src/components/sidebar/add-remote-host-ssh-actions.ts @@ -43,7 +43,7 @@ export async function saveNewSshHostFromForm({ }: { form: EditingTarget ssh: SshApi - recordSshRepoReadoptions: (readoptions: SshRepoReadoption[]) => void + recordSshRepoReadoptions: (readoptions: readonly SshRepoReadoption[]) => void setSshTargetsMetadata: (targets: SshTarget[]) => void recordFeatureInteraction: (feature: 'ssh') => void }): Promise<'saved' | 'validation-failed' | 'failed'> { @@ -170,7 +170,7 @@ export async function addAllSshConfigHostsToOrca({ recordFeatureInteraction }: { ssh: SshApi - recordSshRepoReadoptions: (readoptions: SshRepoReadoption[]) => void + recordSshRepoReadoptions: (readoptions: readonly SshRepoReadoption[]) => void setSshTargetsMetadata: (targets: SshTarget[]) => void recordFeatureInteraction: (feature: 'ssh') => void }): Promise<{ kind: 'added'; count: number } | { kind: 'already-synced' } | { kind: 'failed' }> { diff --git a/src/renderer/src/store/slices/repos-refresh-identity.test.ts b/src/renderer/src/store/slices/repos-refresh-identity.test.ts index 5ebf8b6602d..ecbb8c893bd 100644 --- a/src/renderer/src/store/slices/repos-refresh-identity.test.ts +++ b/src/renderer/src/store/slices/repos-refresh-identity.test.ts @@ -283,7 +283,6 @@ describe('repo filter identity across catalog refreshes', () => { expect(store.getState().filterRepoIds).toBe(first) }) }) - describe('setup-script dismissal identity across catalog refreshes', () => { it('keeps the dismissal array when a refetch prunes nothing', async () => { const store = createTestStore() @@ -301,3 +300,78 @@ describe('setup-script dismissal identity across catalog refreshes', () => { expect(store.getState().setupScriptPromptDismissedRepoIds).toBe(first) }) }) + +describe('SSH readoption catalog identity', () => { + it('keeps projects and host setups across a no-op recordSshRepoReadoptions([])', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const before = store.getState() + const projects = before.projects + const setups = before.projectHostSetups + expect(projects).toHaveLength(1) + expect(setups).toHaveLength(1) + + store.getState().recordSshRepoReadoptions([]) + + // Why: the empty-in/empty-pending call must hand the state object back untouched, or the + // freshly allocated pendingSshRepoReadoptions alone would wake every store subscriber. + expect(store.getState()).toBe(before) + expect(store.getState().pendingSshRepoReadoptions).toBe(before.pendingSshRepoReadoptions) + expect(store.getState().projects).toBe(projects) + expect(store.getState().projects[0]).toBe(projects[0]) + expect(store.getState().projectHostSetups).toBe(setups) + expect(store.getState().projectHostSetups[0]).toBe(setups[0]) + }) + + it('keeps catalog identity for a pending-only readoption while pending updates', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const projects = store.getState().projects + const setups = store.getState().projectHostSetups + const readoption = { oldTargetId: 'ssh-old', newTargetId: 'ssh-new', repoIds: [repo.id] } + + store.getState().recordSshRepoReadoptions([readoption]) + + expect(store.getState().pendingSshRepoReadoptions).toEqual([readoption]) + expect(store.getState().projects).toBe(projects) + expect(store.getState().projects[0]).toBe(projects[0]) + expect(store.getState().projectHostSetups).toBe(setups) + expect(store.getState().projectHostSetups[0]).toBe(setups[0]) + }) + + it('replaces the moved setup on a real prune/rehome', async () => { + const oldHostRepo: Repo = { + ...repo, + connectionId: 'ssh-old', + executionHostId: 'ssh:ssh-old' + } + const newHostRepo: Repo = { + ...repo, + connectionId: 'ssh-new', + executionHostId: 'ssh:ssh-new' + } + mockRepos(oldHostRepo, newHostRepo) + const store = createTestStore() + await store.getState().fetchRepos() + const setups = store.getState().projectHostSetups + expect(setups).toHaveLength(2) + const oldSetup = setups.find((setup) => setup.hostId === 'ssh:ssh-old') + const newSetup = setups.find((setup) => setup.hostId === 'ssh:ssh-new') + expect(oldSetup).toBeDefined() + expect(newSetup).toBeDefined() + + store.getState().recordSshRepoReadoptions([ + { oldTargetId: 'ssh-old', newTargetId: 'ssh-new', repoIds: [repo.id] } + ]) + + const next = store.getState().projectHostSetups + expect(next).not.toBe(setups) + expect(next).toHaveLength(1) + expect(next[0]).not.toBe(oldSetup) + expect(next[0]?.hostId).toBe('ssh:ssh-new') + expect(store.getState().repos).toHaveLength(1) + expect(store.getState().repos[0]?.executionHostId).toBe('ssh:ssh-new') + expect(store.getState().pendingSshRepoReadoptions).toEqual([]) + }) +}) + diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 8bdda9f41e3..df3cef3d7e6 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -1781,8 +1781,8 @@ export type RepoSlice = { activeRepoId: string | null // Monotonic sequence so overlapping catalog fetches can drop stale same-host results (#7020). reposFetchGeneration: number - pendingSshRepoReadoptions: SshRepoReadoption[] - recordSshRepoReadoptions: (readoptions: SshRepoReadoption[]) => void + pendingSshRepoReadoptions: readonly SshRepoReadoption[] + recordSshRepoReadoptions: (readoptions: readonly SshRepoReadoption[]) => void fetchRepos: (options?: RuntimeCatalogFetchOptions) => Promise fetchReposForAllHosts: (options?: AllHostCatalogFetchOptions) => Promise awaitLocalRepoCatalogSettlement: () => Promise @@ -2004,6 +2004,10 @@ export const createRepoSlice: StateCreator = (set, recordSshRepoReadoptions: (readoptions) => set((s) => { + // Why: SshPane importConfig() often reports [] on every Manage-pane open. + if (readoptions.length === 0 && s.pendingSshRepoReadoptions.length === 0) { + return s + } const pendingSshRepoReadoptions = mergeSshRepoReadoptions( s.pendingSshRepoReadoptions, readoptions @@ -2011,19 +2015,35 @@ export const createRepoSlice: StateCreator = (set, const reconciliation = reconcileReadoptedSshRepoRows(s.repos, pendingSshRepoReadoptions) const repos = reconciliation.repos const worktreeState = reconcileReadoptedSshWorktreeState(s, pendingSshRepoReadoptions) - const projectHostSetups = filterSetupsForPrunedRepoRows(s.projectHostSetups, s.repos, repos) + const remainingSetups = filterSetupsForPrunedRepoRows(s.projectHostSetups, s.repos, repos) const compatibility = mergeProjectHostSetupCompatibility( projectCompatibilityFromRepos(repos), { projects: s.projects, - setups: projectHostSetups + setups: remainingSetups } ) + // Why: mergeProjectHostSetupCompatibility always allocates; a no-op readoption + // must not churn catalog identity. This write is all-repos, not host-scoped, + // so it cannot go through mergeFetchedProjectCompatibilityForHost. Reconcile + // hands the store arrays straight back when nothing moved, so writing them + // unconditionally still leaves identity-keyed selectors untouched. + const projects = reconcileCatalogRows( + s.projects, + compatibility.projects, + (project) => project.id + ) + const projectHostSetups = reconcileCatalogRows( + s.projectHostSetups, + compatibility.projectHostSetups, + getProjectHostSetupOwnerKey + ) return { repos, pendingSshRepoReadoptions: reconciliation.pendingReadoptions, ...worktreeState, - ...compatibility + projects, + projectHostSetups } }), diff --git a/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts b/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts index 4b20cfa6edf..c18e0f6e5af 100644 --- a/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts +++ b/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts @@ -4,7 +4,7 @@ import { getRepoExecutionHostId, toSshExecutionHostId } from '../../../../shared export type SshRepoReconciliation = { repos: readonly Repo[] - pendingReadoptions: SshRepoReadoption[] + pendingReadoptions: readonly SshRepoReadoption[] } function repoBelongsToTarget(repo: Repo, targetId: string): boolean { @@ -69,7 +69,7 @@ export function reconcileReadoptedSshRepoRows( export function mergeSshRepoReadoptions( pending: readonly SshRepoReadoption[], incoming: readonly SshRepoReadoption[] -): SshRepoReadoption[] { +): readonly SshRepoReadoption[] { const repoIdsByMigration = new Map>() for (const readoption of [...pending, ...incoming]) { const key = `${readoption.oldTargetId}\0${readoption.newTargetId}`