From ce3ec4d5cead8a3877d56e7b757ab020c36068f0 Mon Sep 17 00:00:00 2001 From: Alex Shan Date: Mon, 10 Aug 2026 16:06:32 +0800 Subject: [PATCH] fix(repo-icon): keep a renamed fork's own owner avatar (#12271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(repo-icon): keep a renamed fork's own owner avatar Fork repos always took the upstream owner's avatar, so a renamed fork showed its parent project's logo. Same-name forks (personal copies) still prefer the upstream owner; renamed forks now keep their origin owner across auto-detect, the startup backfill, and the settings avatar refresh. Co-Authored-By: Claude Fable 5 * fix(repo-icon): re-read repo state before backfill avatar write The startup backfill computed icon updates from a pre-loop snapshot, so an icon chosen in settings while the upstream/origin probes were pending could be clobbered. Re-read the repo after the probes and only migrate an icon that is still the auto-detected GitHub avatar. Co-Authored-By: Claude Fable 5 * refactor(repo-icon): own the fork avatar rule in one shared selector The renamed-fork rule was written out twice — once in the main-process auto-detect and once in the renderer refresh — so the two copies could drift. Move it next to `githubAvatarIcon` as `githubAvatarSlug`, which collapses the renderer resolver to a single unbranched path. Also stop swallowing a rejected origin probe: it cannot tell a renamed fork from a same-name one, so degrading to the upstream owner would flip a renamed fork's stored avatar back to the parent's. Letting it propagate keeps the stored icon, matching how the non-fork path already behaved. Adds coverage for the startup backfill, the third decision point the fix claims, which had none. * test(repo-icon): cover pending backfill icon change --------- Co-authored-by: Claude Fable 5 Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> --- src/main/github/client.ts | 3 +- src/main/repo-icon-autodetect.test.ts | 30 +++++ src/main/repo-icon-autodetect.ts | 12 +- src/main/runtime/orca-runtime.ts | 22 ++- .../runtime/repo-icon-fork-backfill.test.ts | 126 ++++++++++++++++++ ...yIconPicker.github-avatar-refresh.test.tsx | 3 +- .../settings/repository-icon-github.test.ts | 59 +++++++- .../settings/repository-icon-github.ts | 19 ++- src/shared/repo-icon.test.ts | 23 +++- src/shared/repo-icon.ts | 18 ++- src/shared/types.ts | 5 +- 11 files changed, 289 insertions(+), 31 deletions(-) create mode 100644 src/main/runtime/repo-icon-fork-backfill.test.ts diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 7518c9d3581..4f95432fcfd 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -1669,7 +1669,8 @@ export async function getRepoSlug( /** * Resolve a fork's upstream/parent owner/repo, or null when not a fork. - * Why: a fork's `origin` is the personal copy, so repo identity (avatar) should prefer upstream. + * Why: drives the fork indicator, and a same-name fork's avatar prefers the + * upstream owner (a renamed fork keeps its own owner). * Best-effort: any failure (offline, unauthed, non-GitHub) resolves to null. */ export async function getRepoUpstream( diff --git a/src/main/repo-icon-autodetect.test.ts b/src/main/repo-icon-autodetect.test.ts index 03de06f15e3..b808dce4483 100644 --- a/src/main/repo-icon-autodetect.test.ts +++ b/src/main/repo-icon-autodetect.test.ts @@ -214,6 +214,36 @@ describe('detectRepoIcon', () => { }) }) + it('keeps the renamed fork own owner avatar while storing the upstream metadata', async () => { + const repoPath = await makeTempRepoDir() + await gitExecFileAsync(['init'], { cwd: repoPath }) + await gitExecFileAsync(['remote', 'add', 'origin', 'git@github.com:acme/rocket-pro.git'], { + cwd: repoPath + }) + await gitExecFileAsync( + ['remote', 'add', 'upstream', 'git@github.com:upstream-org/rocket.git'], + { + cwd: repoPath + } + ) + + await expect(detectRepoIconAndUpstream({ repoPath, kind: 'git' })).resolves.toEqual({ + gitRemoteIdentity: { + canonicalKey: 'github.com/upstream-org/rocket', + remoteName: 'upstream', + remoteUrl: 'git@github.com:upstream-org/rocket.git' + }, + // Why: a renamed fork is its own project, so the avatar stays on the origin owner. + repoIcon: { + type: 'image', + src: 'https://github.com/acme.png?size=64', + source: 'github', + label: 'acme/rocket-pro' + }, + upstream: { owner: 'upstream-org', repo: 'rocket', host: 'github.com' } + }) + }) + it('detects a provider-neutral git remote identity for non-GitHub remotes', async () => { const repoPath = await makeTempRepoDir() await gitExecFileAsync(['init'], { cwd: repoPath }) diff --git a/src/main/repo-icon-autodetect.ts b/src/main/repo-icon-autodetect.ts index e0dcd15c839..5fa931f06c5 100644 --- a/src/main/repo-icon-autodetect.ts +++ b/src/main/repo-icon-autodetect.ts @@ -1,6 +1,11 @@ import { readFile, stat } from 'node:fs/promises' import type { GitHubRepositoryIdentity, RepoKind } from '../shared/types' -import { faviconUrlFromWebsite, githubAvatarIcon, type RepoIcon } from '../shared/repo-icon' +import { + faviconUrlFromWebsite, + githubAvatarIcon, + githubAvatarSlug, + type RepoIcon +} from '../shared/repo-icon' import { getRepoSlug, getRepoUpstream } from './github/client' import { getSshFilesystemProvider } from './providers/ssh-filesystem-dispatch' import type { IFilesystemProvider } from './providers/types' @@ -71,14 +76,13 @@ async function detectRemotePackageHomepageIcon( } } -async function detectGitHubAvatarIcon( +export async function detectGitHubAvatarIcon( repoPath: string, connectionId?: string | null, upstream?: GitHubRepositoryIdentity | null ): Promise { try { - // Why: a fork's origin is the personal copy, so prefer the upstream owner. - const slug = upstream ?? (await getRepoSlug(repoPath, connectionId)) + const slug = githubAvatarSlug(await getRepoSlug(repoPath, connectionId), upstream) return slug ? githubAvatarIcon(slug) : null } catch { return null diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 7c5eeed2c81..10e143df938 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1012,9 +1012,8 @@ import { getSshGitProviderGeneration, requireSshGitProvider } from '../providers/ssh-git-dispatch' -import { detectRepoIconAndUpstream } from '../repo-icon-autodetect' +import { detectGitHubAvatarIcon, detectRepoIconAndUpstream } from '../repo-icon-autodetect' import { enrichMissingRepoGitRemoteIdentities } from '../repo-git-remote-identity-enrichment' -import { githubAvatarIcon } from '../../shared/repo-icon' import type { ClaudeAccountService } from '../claude-accounts/service' import type { CodexAccountService, @@ -19272,10 +19271,25 @@ export class OrcaRuntimeService { } catch { continue } + const repoIcon = + upstream && repo.repoIcon?.type === 'image' && repo.repoIcon.source === 'github' + ? await detectGitHubAvatarIcon(repo.path, null, upstream) + : null + // Why: settings can change the repo while the probes above are pending, so + // re-read it — a stale snapshot must not clobber a user-chosen icon or an + // upstream another path already resolved. + const current = store.getRepos().find((candidate) => candidate.id === repo.id) + if (!current || current.upstream !== undefined) { + continue + } const updates: Partial = { upstream: upstream ?? null } // Only migrate the auto-detected origin avatar; never touch a chosen icon. - if (upstream && repo.repoIcon?.type === 'image' && repo.repoIcon.source === 'github') { - updates.repoIcon = githubAvatarIcon(upstream) + if ( + repoIcon && + current.repoIcon?.type === 'image' && + current.repoIcon.source === 'github' + ) { + updates.repoIcon = repoIcon } store.updateRepo(repo.id, updates) changed = true diff --git a/src/main/runtime/repo-icon-fork-backfill.test.ts b/src/main/runtime/repo-icon-fork-backfill.test.ts new file mode 100644 index 00000000000..dc7ffb85463 --- /dev/null +++ b/src/main/runtime/repo-icon-fork-backfill.test.ts @@ -0,0 +1,126 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../shared/types' +import * as client from '../github/client' +import { OrcaRuntimeService } from './orca-runtime' + +vi.mock('../github/client', async (importOriginal) => ({ + ...(await importOriginal>()), + getRepoUpstream: vi.fn(), + getRepoSlug: vi.fn() +})) + +const getRepoUpstream = vi.mocked(client.getRepoUpstream) +const getRepoSlug = vi.mocked(client.getRepoSlug) + +type BackfillInternals = { backfillForkUpstreams(): Promise } + +function makeRepo(overrides: Partial = {}): Repo { + return { + id: 'repo-1', + path: '/workspace/rocket-pro', + displayName: 'rocket-pro', + badgeColor: '#2563eb', + addedAt: 1, + kind: 'git', + repoIcon: { + type: 'image', + src: 'https://github.com/acme.png?size=64', + source: 'github', + label: 'acme/rocket-pro' + }, + ...overrides + } +} + +function attachStore(runtime: OrcaRuntimeService, repos: Repo[]) { + const updateRepo = vi.fn((repoId: string, updates: Partial) => { + const index = repos.findIndex((repo) => repo.id === repoId) + repos[index] = { ...repos[index], ...updates } + }) + Object.assign(runtime, { store: { getRepos: () => repos, updateRepo } }) + return updateRepo +} + +afterEach(() => { + vi.clearAllMocks() +}) + +describe('startup fork-upstream backfill', () => { + it('keeps a renamed fork own owner avatar while recording the upstream', async () => { + const runtime = new OrcaRuntimeService() + const updateRepo = attachStore(runtime, [makeRepo()]) + getRepoUpstream.mockResolvedValue({ owner: 'upstream-org', repo: 'rocket' }) + getRepoSlug.mockResolvedValue({ owner: 'acme', repo: 'rocket-pro' }) + + await (runtime as unknown as BackfillInternals).backfillForkUpstreams() + + expect(updateRepo).toHaveBeenCalledExactlyOnceWith('repo-1', { + upstream: { owner: 'upstream-org', repo: 'rocket' }, + repoIcon: { + type: 'image', + src: 'https://github.com/acme.png?size=64', + source: 'github', + label: 'acme/rocket-pro' + } + }) + }) + + it('migrates a same-name fork to the upstream owner avatar', async () => { + const runtime = new OrcaRuntimeService() + const updateRepo = attachStore(runtime, [ + makeRepo({ + path: '/workspace/rocket', + repoIcon: { + type: 'image', + src: 'https://github.com/acme.png?size=64', + source: 'github', + label: 'acme/rocket' + } + }) + ]) + getRepoUpstream.mockResolvedValue({ owner: 'upstream-org', repo: 'rocket' }) + getRepoSlug.mockResolvedValue({ owner: 'acme', repo: 'rocket' }) + + await (runtime as unknown as BackfillInternals).backfillForkUpstreams() + + expect(updateRepo).toHaveBeenCalledExactlyOnceWith('repo-1', { + upstream: { owner: 'upstream-org', repo: 'rocket' }, + repoIcon: { + type: 'image', + src: 'https://github.com/upstream-org.png?size=64', + source: 'github', + label: 'upstream-org/rocket' + } + }) + }) + + it('keeps an icon chosen while avatar detection is pending', async () => { + const runtime = new OrcaRuntimeService() + const repo = makeRepo() + const repos = [repo] + const updateRepo = attachStore(runtime, repos) + getRepoUpstream.mockResolvedValue({ owner: 'upstream-org', repo: 'rocket' }) + let resolveSlug!: (value: { owner: string; repo: string }) => void + let markSlugStarted!: () => void + const slugStarted = new Promise((resolve) => { + markSlugStarted = resolve + }) + getRepoSlug.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveSlug = resolve + markSlugStarted() + }) + ) + + const backfill = (runtime as unknown as BackfillInternals).backfillForkUpstreams() + await slugStarted + repos[0] = { ...repo, repoIcon: { type: 'emoji', emoji: '🚀' } } + resolveSlug({ owner: 'acme', repo: 'rocket-pro' }) + await backfill + + expect(updateRepo).toHaveBeenCalledExactlyOnceWith('repo-1', { + upstream: { owner: 'upstream-org', repo: 'rocket' } + }) + }) +}) diff --git a/src/renderer/src/components/settings/RepositoryIconPicker.github-avatar-refresh.test.tsx b/src/renderer/src/components/settings/RepositoryIconPicker.github-avatar-refresh.test.tsx index eaa4fa1130e..996261ab3e9 100644 --- a/src/renderer/src/components/settings/RepositoryIconPicker.github-avatar-refresh.test.tsx +++ b/src/renderer/src/components/settings/RepositoryIconPicker.github-avatar-refresh.test.tsx @@ -106,7 +106,7 @@ describe('RepositoryIconPicker GitHub avatar refresh', () => { label: 'stablyai/orca' } }) - // Offline/unauthed: the parent lookup returns null. The fork's own origin + // Offline/unauthed: the parent lookup returns null. The same-name origin // owner must NOT be persisted over the parent identity. apiMocks.repoUpstream.mockResolvedValueOnce(null) apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'parkerrex', repo: 'orca' }) @@ -117,6 +117,5 @@ describe('RepositoryIconPicker GitHub avatar refresh', () => { await flushEffects() expect(updateRepo).not.toHaveBeenCalled() - expect(apiMocks.repoSlug).not.toHaveBeenCalled() }) }) diff --git a/src/renderer/src/components/settings/repository-icon-github.test.ts b/src/renderer/src/components/settings/repository-icon-github.test.ts index e98eeb44a3e..2926d0fd2b4 100644 --- a/src/renderer/src/components/settings/repository-icon-github.test.ts +++ b/src/renderer/src/components/settings/repository-icon-github.test.ts @@ -36,8 +36,10 @@ describe('repository GitHub avatar resolution', () => { apiMocks.repoUpstream.mockReset() }) - it('uses stored upstream by default to avoid unnecessary live checks', async () => { + it('uses stored upstream by default and keeps the parent avatar for same-name forks', async () => { const repo = makeRepo({ upstream: { owner: 'stablyai', repo: 'orca' } }) + // The fork's own origin owner — same repo name, so the parent avatar wins. + apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'tmchow', repo: 'orca' }) await expect(resolveRepositoryGitHubAvatar({ kind: 'local' }, repo)).resolves.toEqual({ repoIcon: { @@ -50,7 +52,40 @@ describe('repository GitHub avatar resolution', () => { }) expect(apiMocks.repoUpstream).not.toHaveBeenCalled() - expect(apiMocks.repoSlug).not.toHaveBeenCalled() + // Only the origin slug is consulted (for the renamed-fork check). + expect(apiMocks.repoSlug).toHaveBeenCalledExactlyOnceWith({ + repoPath: '/workspace/orca', + repoId: 'repo-1' + }) + }) + + it('prefers the renamed fork own owner over the stored upstream', async () => { + const repo = makeRepo({ upstream: { owner: 'upstream-org', repo: 'rocket' } }) + apiMocks.repoUpstream.mockResolvedValueOnce({ owner: 'upstream-org', repo: 'rocket' }) + apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'acme', repo: 'rocket-pro' }) + + const resolution = await resolveRepositoryGitHubAvatar({ kind: 'local' }, repo, { + forceLive: true + }) + + expect(resolution).toEqual({ + repoIcon: { + type: 'image', + src: 'https://github.com/acme.png?size=64', + source: 'github', + label: 'acme/rocket-pro' + }, + upstream: { owner: 'upstream-org', repo: 'rocket' } + }) + // The fork metadata is untouched; only the avatar moves to the fork's own owner. + expect(buildRepositoryGitHubAvatarUpdate(repo, resolution)).toEqual({ + repoIcon: { + type: 'image', + src: 'https://github.com/acme.png?size=64', + source: 'github', + label: 'acme/rocket-pro' + } + }) }) it('force-resolves the live origin owner when a non-fork repo was transferred', async () => { @@ -140,7 +175,7 @@ describe('repository GitHub avatar resolution', () => { } }) apiMocks.repoUpstream.mockResolvedValueOnce(null) - // The fork's own origin owner — the value we must NOT persist over the parent. + // The fork's own origin owner — same repo name, so it must NOT replace the parent. apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'parkerrex', repo: 'orca' }) const resolution = await resolveRepositoryGitHubAvatar({ kind: 'local' }, repo, { @@ -156,12 +191,26 @@ describe('repository GitHub avatar resolution', () => { }, upstream: { owner: 'stablyai', repo: 'orca' } }) - // The origin slug must never be consulted once we fall back to the known parent. - expect(apiMocks.repoSlug).not.toHaveBeenCalled() // Nothing changed, so no repo write is produced (no sticky null clobber). expect(buildRepositoryGitHubAvatarUpdate(repo, resolution)).toBeNull() }) + it('propagates an ambiguous origin probe failure instead of flipping to the parent avatar', async () => { + // A renamed fork already showing its own owner. A rejected origin probe cannot + // tell renamed from same-name, so it must surface rather than resolve to the + // parent avatar — callers keep the stored icon. + const repo = makeRepo({ + upstream: { owner: 'upstream-org', repo: 'rocket' }, + repoIcon: githubAvatarIcon({ owner: 'acme', repo: 'rocket-pro' }) + }) + apiMocks.repoUpstream.mockResolvedValueOnce({ owner: 'upstream-org', repo: 'rocket' }) + apiMocks.repoSlug.mockRejectedValueOnce(new Error('runtime rpc timeout')) + + await expect( + resolveRepositoryGitHubAvatar({ kind: 'local' }, repo, { forceLive: true }) + ).rejects.toThrow('runtime rpc timeout') + }) + it('persists an upstream change when only the GitHub host differs', () => { const repo = makeRepo({ upstream: { owner: 'acme', repo: 'widgets', host: 'github.com' } diff --git a/src/renderer/src/components/settings/repository-icon-github.ts b/src/renderer/src/components/settings/repository-icon-github.ts index dfaac40c7a7..b5519e36f36 100644 --- a/src/renderer/src/components/settings/repository-icon-github.ts +++ b/src/renderer/src/components/settings/repository-icon-github.ts @@ -1,5 +1,5 @@ import type { GitHubRepositoryIdentity, Repo } from '../../../../shared/types' -import { githubAvatarIcon, type RepoIcon } from '../../../../shared/repo-icon' +import { githubAvatarIcon, githubAvatarSlug, type RepoIcon } from '../../../../shared/repo-icon' import { githubRepoIdentityKey } from '../../../../shared/github-repository-identity-key' import { callRuntimeRpc, type getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' @@ -55,20 +55,17 @@ export async function resolveRepositoryGitHubAvatar( repo: Repo, options: ResolveRepositoryGitHubAvatarOptions = {} ): Promise { - const upstream = + const liveUpstream = !options.forceLive && repo.upstream !== undefined ? repo.upstream : await resolveRepositoryUpstreamLive(runtimeTarget, repo).catch(() => null) - if (upstream) { - return { repoIcon: githubAvatarIcon(upstream), upstream } - } // Why: a null live upstream is ambiguous (offline/unauthed vs. not-a-fork). Keep - // the last-known parent avatar so a transient failure can't clobber fork identity. - if (repo.upstream) { - return { repoIcon: githubAvatarIcon(repo.upstream), upstream: repo.upstream } - } - const slug = await resolveRepositorySlugLive(runtimeTarget, repo) - return { repoIcon: slug ? githubAvatarIcon(slug) : null, upstream: null } + // the last-known parent so a transient failure can't clobber fork identity. + const upstream = liveUpstream ?? repo.upstream ?? null + // Why: a rejected origin probe is also ambiguous, so it propagates — callers keep + // the stored icon rather than flipping a renamed fork back to the parent avatar. + const slug = githubAvatarSlug(await resolveRepositorySlugLive(runtimeTarget, repo), upstream) + return { repoIcon: slug ? githubAvatarIcon(slug) : null, upstream } } function sameRepositoryIdentity( diff --git a/src/shared/repo-icon.test.ts b/src/shared/repo-icon.test.ts index 3875f517427..566d8209e52 100644 --- a/src/shared/repo-icon.test.ts +++ b/src/shared/repo-icon.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { githubAvatarIcon, sanitizeRepoIcon } from './repo-icon' +import { githubAvatarIcon, githubAvatarSlug, sanitizeRepoIcon } from './repo-icon' const PNG_1X1_BASE64 = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII=' @@ -167,3 +167,24 @@ describe('sanitizeRepoIcon', () => { ).toMatchObject({ src: 'https://github.com/acme.png?size=64' }) }) }) + +describe('githubAvatarSlug', () => { + const upstream = { owner: 'upstream-org', repo: 'rocket' } + + it('keeps the upstream owner for a same-name fork, case-insensitively', () => { + expect(githubAvatarSlug({ owner: 'acme', repo: 'rocket' }, upstream)).toEqual(upstream) + expect(githubAvatarSlug({ owner: 'acme', repo: 'RocKet' }, upstream)).toEqual(upstream) + }) + + it('keeps the fork own owner once it has been renamed', () => { + const origin = { owner: 'acme', repo: 'rocket-pro' } + expect(githubAvatarSlug(origin, upstream)).toEqual(origin) + }) + + it('falls back to whichever identity is known', () => { + const origin = { owner: 'acme', repo: 'rocket-pro' } + expect(githubAvatarSlug(origin, null)).toEqual(origin) + expect(githubAvatarSlug(null, upstream)).toEqual(upstream) + expect(githubAvatarSlug(null, undefined)).toBeNull() + }) +}) diff --git a/src/shared/repo-icon.ts b/src/shared/repo-icon.ts index f5520b57032..1973a2227ac 100644 --- a/src/shared/repo-icon.ts +++ b/src/shared/repo-icon.ts @@ -31,8 +31,24 @@ export function faviconUrlFromWebsite(rawUrl: string): string | null { } } +type GitHubAvatarSlug = { owner: string; repo: string; host?: string } + +/** + * Pick the owner whose avatar represents a repo, given its `origin` and fork parent. + * Why: a same-name fork is a personal copy, so it reads as the parent project; a + * renamed fork is its own project and keeps its own owner. + */ +export function githubAvatarSlug( + origin: GitHubAvatarSlug | null | undefined, + upstream: GitHubAvatarSlug | null | undefined +): GitHubAvatarSlug | null { + const renamedFork = + origin && upstream && origin.repo.toLowerCase() !== upstream.repo.toLowerCase() + return renamedFork ? origin : (upstream ?? origin ?? null) +} + // Why: shared default icon URL/label for main auto-detect and the renderer picker. -export function githubAvatarIcon(slug: { owner: string; repo: string; host?: string }): RepoIcon { +export function githubAvatarIcon(slug: GitHubAvatarSlug): RepoIcon { // Why: GHES uses the same /.png avatar path as github.com. const host = normalizeGitHubAvatarHost(slug.host) return { diff --git a/src/shared/types.ts b/src/shared/types.ts index 284782625e9..cd874d94037 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -253,8 +253,9 @@ export type Repo = { badgeColor: string repoIcon?: RepoIcon | null /** Set when the repo is a fork: the upstream/parent owner/repo. Drives the - * default avatar (upstream owner, not the personal fork) and the fork - * indicator. Absent = not a fork, or fork status not yet resolved. */ + * fork indicator and the default avatar of same-name forks (renamed forks + * keep their own owner). Absent = not a fork, or fork status not yet + * resolved. */ upstream?: GitHubRepositoryIdentity | null addedAt: number kind?: RepoKind