diff --git a/src/main/repo-git-remote-avatar-refresh.test.ts b/src/main/repo-git-remote-avatar-refresh.test.ts new file mode 100644 index 00000000000..ab5eb7ae114 --- /dev/null +++ b/src/main/repo-git-remote-avatar-refresh.test.ts @@ -0,0 +1,313 @@ +import { afterEach, expect, it, vi } from 'vitest' +import { getRepoExecutionHostId, type ExecutionHostId } from '../shared/execution-host' +import { deriveGitRemoteIdentity } from '../shared/git-remote-identity' +import { projectHostSetupProjectionFromRepos } from '../shared/project-host-setup-projection' +import { githubAvatarIcon, type RepoIcon } from '../shared/repo-icon' +import type { Repo } from '../shared/repo-types' +import { probeGitRemoteIdentity, type GitRemoteIdentityProbe } from './repo-git-remote-identity' +import { + enrichMissingRepoGitRemoteIdentities, + flushRepoGitRemoteIdentityEnrichmentForTests, + resetRepoGitRemoteIdentityEnrichmentForTests +} from './repo-git-remote-identity-enrichment' + +vi.mock('./repo-git-remote-identity', () => ({ probeGitRemoteIdentity: vi.fn() })) + +function identity(remote = 'https://github.com/org-b/app.git') { + const parsed = deriveGitRemoteIdentity(`origin\t${remote} (fetch)`) + if (!parsed) { + throw new Error('Fixture remote must parse') + } + return parsed +} + +function repo(overrides: Partial = {}): Repo { + return { + id: 'app', + path: '/workspace/app', + displayName: 'app', + kind: 'git', + badgeColor: '', + addedAt: 1, + upstream: null, + gitRemoteIdentity: identity(), + repoIcon: githubAvatarIcon({ owner: 'owner-a', repo: 'app' }), + ...overrides + } +} + +function storeFor(repos: Repo[]) { + const getRepo = (id: string, hostId?: ExecutionHostId) => + repos.find((row) => row.id === id && (!hostId || getRepoExecutionHostId(row) === hostId)) + const updateRepo = vi.fn((id: string, updates: Partial, hostId?: ExecutionHostId) => { + const current = getRepo(id, hostId) + if (!current) { + return null + } + Object.assign(current, updates) + return current + }) + return { getRepos: () => repos, getRepo, updateRepo } +} + +async function sweep(store: ReturnType, onChanged = vi.fn()) { + enrichMissingRepoGitRemoteIdentities(store, { onChanged }) + for (let i = 0; i < 8; i++) { + await flushRepoGitRemoteIdentityEnrichmentForTests() + } +} + +async function refresh(store: ReturnType, onChanged = vi.fn()) { + vi.useFakeTimers() + vi.setSystemTime(1_000) + await sweep(store, onChanged) + expect(probeGitRemoteIdentity).not.toHaveBeenCalled() + vi.setSystemTime(301_001) + await sweep(store, onChanged) +} + +afterEach(() => { + resetRepoGitRemoteIdentityEnrichmentForTests() + vi.useRealTimers() + vi.clearAllMocks() +}) + +it.each(['already-current', 'changed'])( + 'repairs a stale avatar when the canonical key is %s', + async (mode) => { + const local = repo({ + gitRemoteIdentity: + mode === 'changed' ? identity('https://github.com/owner-a/app.git') : identity() + }) + const peer = repo({ + id: 'peer', + path: '/workspace/app', + connectionId: 'build', + repoIcon: githubAvatarIcon({ owner: 'org-b', repo: 'app' }) + }) + const store = storeFor([local, peer]) + const onChanged = vi.fn() + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ + status: 'resolved', + identity: identity() + }) + + expect(projectHostSetupProjectionFromRepos(store.getRepos()).projects).toHaveLength(2) + await refresh(store, onChanged) + + expect(local.repoIcon).toEqual(githubAvatarIcon({ owner: 'org-b', repo: 'app' })) + expect(store.updateRepo).toHaveBeenCalledTimes(1) + expect(onChanged).toHaveBeenCalledTimes(1) + const projected = projectHostSetupProjectionFromRepos(store.getRepos()) + expect(projected.projects.map(({ id }) => id)).toEqual(['github:org-b/app']) + expect(projected.setups.map(({ hostId, path }) => ({ hostId, path }))).toEqual([ + { hostId: 'local', path: '/workspace/app' }, + { hostId: 'ssh:build', path: '/workspace/app' } + ]) + vi.setSystemTime(301_001 + 6 * 60 * 60 * 1000) + await sweep(store, onChanged) + expect(store.updateRepo).toHaveBeenCalledTimes(1) + } +) + +it.each([ + 'https://github.company.test:8443/org-b/app.git', + 'ssh://git@ssh.github.com:443/org-b/app.git' +])('preserves GitHub endpoint identity for %s', async (remote) => { + const row = repo({ gitRemoteIdentity: identity(remote) }) + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ + status: 'resolved', + identity: identity(remote) + }) + await refresh(store) + expect(projectHostSetupProjectionFromRepos([row]).projects[0]?.id).toBe( + remote.includes('company') ? 'github:github.company.test:8443/org-b/app' : 'github:org-b/app' + ) +}) + +it.each([ + 'git@github-work:org/app.git', + 'ssh://git@ghe-work/org/app.git', + 'https://gitlab.com/team/sub/app.git', + 'https://forgejo.test/team/app.git', + 'https://gitea.test/team/app.git', + 'https://code.company.test/team/app.git' +])('leaves provider metadata intact for unresolved or other-provider remote %s', async (remote) => { + const row = repo({ gitRemoteIdentity: identity(remote) }) + const original = row.repoIcon + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ + status: 'resolved', + identity: identity(remote) + }) + await refresh(store) + expect(row.repoIcon).toBe(original) + expect(store.updateRepo).not.toHaveBeenCalled() +}) + +const customIcons: (RepoIcon | null | undefined)[] = [ + { type: 'emoji', emoji: '🐙' }, + { type: 'lucide', name: 'Folder' }, + { type: 'image', source: 'upload', src: 'data:image/png;base64,fixture', label: 'custom' }, + { type: 'image', source: 'file', src: 'data:image/png;base64,fixture', label: 'custom' }, + { type: 'image', source: 'favicon', src: 'https://website.test/favicon.png', label: 'custom' }, + null, + undefined +] + +it.each(customIcons)('preserves custom or cleared icon %j', async (repoIcon) => { + const row = repo({ repoIcon }) + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'resolved', identity: identity() }) + await refresh(store) + expect(row.repoIcon).toBe(repoIcon) + expect(store.updateRepo).not.toHaveBeenCalled() +}) + +it('preserves explicit upstream even when the remote disagrees', async () => { + const row = repo({ upstream: { owner: 'parent', repo: 'app', host: 'github.parent.test' } }) + const original = row.repoIcon + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'resolved', identity: identity() }) + await refresh(store) + expect(row.repoIcon).toBe(original) + expect(projectHostSetupProjectionFromRepos([row]).projects[0]?.id).toBe( + 'github:github.parent.test/parent/app' + ) + expect(store.updateRepo).not.toHaveBeenCalled() +}) + +it.each(['unavailable', 'no-remote'] as const)( + 'preserves the last avatar when a refresh is %s', + async (status) => { + const row = repo() + const original = row.repoIcon + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status }) + await refresh(store) + expect(row.repoIcon).toBe(original) + expect(store.updateRepo).not.toHaveBeenCalled() + } +) + +it.each(['same', 'different', 'missing'] as const)( + 'never probes or writes peer-owned metadata when identity is %s', + async (kind) => { + const row = repo({ + executionHostId: 'runtime:peer', + connectionId: 'nested', + gitRemoteIdentity: + kind === 'missing' + ? undefined + : identity(kind === 'different' ? 'https://github.com/peer-only/app.git' : undefined) + }) + const originalIcon = row.repoIcon + const originalIdentity = row.gitRemoteIdentity + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ + status: 'resolved', + identity: identity() + }) + await (kind === 'missing' ? sweep(store) : refresh(store)) + expect(probeGitRemoteIdentity).not.toHaveBeenCalled() + expect(row.repoIcon).toBe(originalIcon) + expect(row.gitRemoteIdentity).toBe(originalIdentity) + expect(store.updateRepo).not.toHaveBeenCalled() + } +) + +it('repairs only the matching owner when repo IDs and paths collide across hosts', async () => { + const local = repo({ repoIcon: githubAvatarIcon({ owner: 'org-b', repo: 'app' }) }) + const ssh = repo({ executionHostId: 'ssh:build' }) + const store = storeFor([local, ssh]) + vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'resolved', identity: identity() }) + await refresh(store) + expect(store.updateRepo).toHaveBeenCalledWith( + 'app', + { repoIcon: githubAvatarIcon({ owner: 'org-b', repo: 'app' }) }, + 'ssh:build' + ) + expect(projectHostSetupProjectionFromRepos([local, ssh]).projects).toHaveLength(1) +}) + +it.each(['ssh:build', 'runtime:peer'] as const)( + 'keeps local metadata writes scoped when a same-id %s row comes first', + async (hostId) => { + const foreignIdentity = identity('https://github.com/foreign/app.git') + const foreignIcon = githubAvatarIcon({ owner: 'foreign', repo: 'app' }) + const foreign = repo({ + executionHostId: hostId, + gitRemoteIdentity: foreignIdentity, + repoIcon: foreignIcon + }) + const local = repo({ gitRemoteIdentity: identity('https://github.com/old-local/app.git') }) + const store = storeFor([foreign, local]) + vi.mocked(probeGitRemoteIdentity).mockImplementation(async (_path, probeHostId) => ({ + status: 'resolved', + identity: probeHostId === 'local' ? identity() : foreignIdentity + })) + await refresh(store) + expect(foreign.gitRemoteIdentity).toBe(foreignIdentity) + expect(foreign.repoIcon).toBe(foreignIcon) + expect(local.gitRemoteIdentity).toEqual(identity()) + expect(local.repoIcon).toEqual(githubAvatarIcon({ owner: 'org-b', repo: 'app' })) + expect(store.updateRepo).toHaveBeenCalledExactlyOnceWith( + 'app', + { gitRemoteIdentity: identity(), repoIcon: local.repoIcon }, + 'local' + ) + } +) + +it('does not write after a pending local probe becomes peer-owned', async () => { + let answer: ((value: GitRemoteIdentityProbe) => void) | undefined + const row = repo({ gitRemoteIdentity: undefined }) + const originalIcon = row.repoIcon + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockImplementation( + () => + new Promise((resolve) => { + answer = resolve + }) + ) + enrichMissingRepoGitRemoteIdentities(store) + row.executionHostId = 'runtime:peer' + if (!answer) { + throw new Error('Expected pending probe') + } + answer({ status: 'resolved', identity: identity() }) + await flushRepoGitRemoteIdentityEnrichmentForTests() + expect(row.gitRemoteIdentity).toBeUndefined() + expect(row.repoIcon).toBe(originalIcon) + expect(store.updateRepo).not.toHaveBeenCalled() +}) + +it('does not overwrite a custom icon selected while the probe is pending', async () => { + let answer: ((value: GitRemoteIdentityProbe) => void) | undefined + const row = repo({ gitRemoteIdentity: undefined }) + const store = storeFor([row]) + vi.mocked(probeGitRemoteIdentity).mockImplementation( + () => + new Promise((resolve) => { + answer = resolve + }) + ) + enrichMissingRepoGitRemoteIdentities(store) + const selected: RepoIcon = { type: 'emoji', emoji: '🐙' } + row.repoIcon = selected + if (!answer) { + throw new Error('Expected pending probe') + } + answer({ status: 'resolved', identity: identity() }) + await flushRepoGitRemoteIdentityEnrichmentForTests() + expect(row.repoIcon).toBe(selected) + expect(store.updateRepo).toHaveBeenCalledWith('app', { gitRemoteIdentity: identity() }, 'local') +}) + +it('never probes folder workspaces for an avatar repair', async () => { + const store = storeFor([repo({ kind: 'folder' })]) + await refresh(store) + expect(probeGitRemoteIdentity).not.toHaveBeenCalled() + expect(store.updateRepo).not.toHaveBeenCalled() +}) diff --git a/src/main/repo-git-remote-identity-enrichment.test.ts b/src/main/repo-git-remote-identity-enrichment.test.ts index a52d110aa02..8012145fdbc 100644 --- a/src/main/repo-git-remote-identity-enrichment.test.ts +++ b/src/main/repo-git-remote-identity-enrichment.test.ts @@ -129,23 +129,16 @@ describe('enrichMissingRepoGitRemoteIdentities', () => { await flushRepoGitRemoteIdentityEnrichmentForTests() }) - it('never hands a runtime row nested SSH target to this client dispatch table', async () => { - // Why: `connectionId` on a `runtime:` row names a target inside that server's namespace, so - // dialing it here reaches a same-named box of ours. The row keeps the probe this process has - // always run for it; what it must never do is dial our same-named target. + it('never probes a runtime row through this client dispatch table', async () => { vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'unavailable' }) const store = makeStore( makeRepo({ connectionId: 'nested-1', executionHostId: 'runtime:env-a' }) ) - enrichMissingRepoGitRemoteIdentities(store) + await sweep(store) - expect(probeGitRemoteIdentity).toHaveBeenCalledWith( - '/workspace/sample-app', - 'local', - expect.objectContaining({ signal: expect.any(AbortSignal) }) - ) - await flushRepoGitRemoteIdentityEnrichmentForTests() + expect(probeGitRemoteIdentity).not.toHaveBeenCalled() + expect(store.updateRepo).not.toHaveBeenCalled() }) it('keeps same-path rows on two different SSH hosts from sharing one backoff', async () => { @@ -208,7 +201,7 @@ describe('enrichMissingRepoGitRemoteIdentities', () => { enrichMissingRepoGitRemoteIdentities(store) await flushRepoGitRemoteIdentityEnrichmentForTests() - expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: null }) + expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: null }, 'local') expect(repo.gitRemoteIdentity).toBeNull() }) @@ -243,7 +236,11 @@ describe('enrichMissingRepoGitRemoteIdentities', () => { enrichMissingRepoGitRemoteIdentities(store) await flushRepoGitRemoteIdentityEnrichmentForTests() - expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: remoteIdentity }) + expect(store.updateRepo).toHaveBeenCalledWith( + 'repo-1', + { gitRemoteIdentity: remoteIdentity }, + 'local' + ) }) it('does not re-probe a resolved identity before the refresh window elapses', async () => { @@ -296,7 +293,11 @@ describe('enrichMissingRepoGitRemoteIdentities', () => { enrichMissingRepoGitRemoteIdentities(store, { onChanged }) await drainEnrichmentSweep() - expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: movedIdentity }) + expect(store.updateRepo).toHaveBeenCalledWith( + 'repo-1', + { gitRemoteIdentity: movedIdentity }, + 'local' + ) expect(repo.gitRemoteIdentity).toEqual(movedIdentity) expect(onChanged).toHaveBeenCalledTimes(1) }) diff --git a/src/main/repo-git-remote-identity-enrichment.ts b/src/main/repo-git-remote-identity-enrichment.ts index a97961fe59f..4c81174a79b 100644 --- a/src/main/repo-git-remote-identity-enrichment.ts +++ b/src/main/repo-git-remote-identity-enrichment.ts @@ -5,6 +5,9 @@ import { type ExecutionHostId } from '../shared/execution-host' import type { Repo } from '../shared/repo-types' +import { githubAvatarIcon, type RepoIcon } from '../shared/repo-icon' +import { isUnresolvedSshHostAlias } from '../shared/git-remote-host-alias' +import { getProjectProviderIdentity } from '../shared/project-host-setup-projection' import { probeGitRemoteIdentity } from './repo-git-remote-identity' const NO_IDENTITY_RETRY_TTL_MS = 5 * 60 * 1000 @@ -21,8 +24,12 @@ const MAX_IDENTITY_REFRESHES_PER_SWEEP = 4 type RepoIdentityStore = { getRepos(): Repo[] - getRepo?(id: string): Repo | undefined - updateRepo(id: string, updates: Pick, 'gitRemoteIdentity'>): Repo | null + getRepo?(id: string, hostId?: ExecutionHostId): Repo | undefined + updateRepo( + id: string, + updates: Pick, 'gitRemoteIdentity' | 'repoIcon'>, + hostId?: ExecutionHostId + ): Repo | null } type EnrichmentOptions = { @@ -51,18 +58,22 @@ function getRepoLocationKey(repo: Pick repo.id === id) +function getCurrentRepo(store: RepoIdentityStore, snapshot: Repo): Repo | undefined { + const hostId = getRepoExecutionHostId(snapshot) + const found = store.getRepo?.(snapshot.id, hostId) + return found && getRepoExecutionHostId(found) === hostId + ? found + : store + .getRepos() + .find((repo) => repo.id === snapshot.id && getRepoExecutionHostId(repo) === hostId) } function isSameProbedRepo(snapshot: Repo, current: Repo | undefined): current is Repo { @@ -87,22 +98,65 @@ function shouldWriteProbedIdentity(current: Repo, probed: Repo['gitRemoteIdentit return !!probed && probed.canonicalKey !== existing.canonicalKey } +function getAutomaticGitHubIconRefresh( + current: Repo, + probed: NonNullable +): RepoIcon | undefined { + if ( + (current.upstream?.owner && current.upstream.repo) || + current.repoIcon?.type !== 'image' || + current.repoIcon.source !== 'github' + ) { + return undefined + } + const identity = getProjectProviderIdentity({ + upstream: null, + repoIcon: undefined, + gitRemoteIdentity: probed + }) + if (!identity || (identity.host && isUnresolvedSshHostAlias(identity.host))) { + return undefined + } + const icon = githubAvatarIcon(identity) + return icon.type === 'image' && + current.repoIcon.src === icon.src && + current.repoIcon.label === icon.label + ? undefined + : icon +} + function writeIdentity( store: RepoIdentityStore, snapshot: Repo, gitRemoteIdentity: Repo['gitRemoteIdentity'] ): boolean { - const current = getCurrentRepo(store, snapshot.id) - if ( - !isSameProbedRepo(snapshot, current) || - !shouldWriteProbedIdentity(current, gitRemoteIdentity) - ) { + // A peer's repo metadata must never be repaired from a client-local probe. + const hostId = getRepoProbeHostId(snapshot) + if (!hostId) { return false } - return !!store.updateRepo(snapshot.id, { gitRemoteIdentity }) + const current = getCurrentRepo(store, snapshot) + if (!isSameProbedRepo(snapshot, current)) { + return false + } + const writeRemote = shouldWriteProbedIdentity(current, gitRemoteIdentity) + const icon = gitRemoteIdentity + ? getAutomaticGitHubIconRefresh(current, gitRemoteIdentity) + : undefined + const update = (updates: Pick, 'gitRemoteIdentity' | 'repoIcon'>): Repo | null => { + return store.updateRepo(snapshot.id, updates, hostId) + } + if (icon) { + return !!update({ ...(writeRemote ? { gitRemoteIdentity } : {}), repoIcon: icon }) + } + return writeRemote && !!update({ gitRemoteIdentity }) } async function enrichRepoGitRemoteIdentity(store: RepoIdentityStore, repo: Repo): Promise { + const hostId = getRepoProbeHostId(repo) + if (!hostId) { + return false + } const locationKey = getRepoLocationKey(repo) const retryAfter = probeRetryAfterByLocation.get(locationKey) ?? 0 if (retryAfter > Date.now()) { @@ -119,7 +173,7 @@ async function enrichRepoGitRemoteIdentity(store: RepoIdentityStore, repo: Repo) : NO_IDENTITY_RETRY_TTL_MS const controller = new AbortController() const promise = (async () => { - const result = await probeGitRemoteIdentity(repo.path, getRepoProbeHostId(repo), { + const result = await probeGitRemoteIdentity(repo.path, hostId, { signal: controller.signal }) // Why the signal and not a catch: probeGitRemoteIdentity swallows the AbortError and RESOLVES @@ -187,7 +241,9 @@ function retireRemovedLocations(allRepos: Repo[]): void { function selectEnrichmentCandidates(store: RepoIdentityStore): Repo[] { const now = Date.now() - const repos = store.getRepos().filter((repo) => repo.kind !== 'folder') + const repos = store + .getRepos() + .filter((repo) => repo.kind !== 'folder' && getRepoProbeHostId(repo) !== null) // Why: the settled `null` marker stays a candidate on purpose — a repo that // gains a remote later must still resolve. Do not tighten this to // `=== undefined`; the retry TTL already bounds the cost and `writeIdentity`