Fix stale GitHub repo avatars after owner changes (#6507)

* Fix stale GitHub repo avatars after owner changes * Simplify avatar-refresh effect deps (repo covers its fields) Co-authored-by: Orca <help@stably.ai> * Preserve known fork identity when live upstream lookup is inconclusive Co-authored-by: Orca <help@stably.ai> * Remove dead avatar-icon wrapper; drop no-op update fallbacks Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> Co-authored-by: Orca <help@stably.ai>
2026-09-24 00:02:24 +00:00 · 2026-07-03 14:20:30 -07:00
parent 5124b0d0ad
commit 332490e919
4 changed files with 404 additions and 34 deletions
@@ -0,0 +1,122 @@
+// @vitest-environment happy-dom
+
+import { act } from 'react'
+import { createRoot, type Root } from 'react-dom/client'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import type { Repo } from '../../../../shared/types'
+import { RepositoryIconPicker } from './RepositoryIconPicker'
+
+vi.mock('@/runtime/runtime-rpc-client', () => ({
+  callRuntimeRpc: vi.fn(),
+  getActiveRuntimeTarget: () => ({ kind: 'local' })
+}))
+
+vi.mock('./RepositoryIconColorSection', () => ({
+  RepositoryIconColorSection: () => null
+}))
+
+vi.mock('./RepositoryIconTabs', () => ({
+  RepositoryIconTabs: () => null
+}))
+
+const apiMocks = {
+  repoSlug: vi.fn(),
+  repoUpstream: vi.fn()
+}
+
+let container: HTMLDivElement
+let root: Root
+
+// @ts-expect-error test window mock
+globalThis.window = { api: { gh: apiMocks } }
+
+function makeRepo(overrides: Partial<Repo> = {}): Repo {
+  return {
+    id: 'repo-1',
+    path: '/workspace/orca',
+    displayName: 'orca',
+    badgeColor: '#2563eb',
+    addedAt: 1,
+    kind: 'git',
+    ...overrides
+  }
+}
+
+async function flushEffects(): Promise<void> {
+  await act(async () => {
+    await Promise.resolve()
+    await Promise.resolve()
+  })
+}
+
+describe('RepositoryIconPicker GitHub avatar refresh', () => {
+  beforeEach(() => {
+    apiMocks.repoSlug.mockReset()
+    apiMocks.repoUpstream.mockReset()
+    container = document.createElement('div')
+    document.body.appendChild(container)
+    root = createRoot(container)
+  })
+
+  afterEach(() => {
+    act(() => root.unmount())
+    container.remove()
+    document.body.replaceChildren()
+  })
+
+  it('refreshes stale GitHub avatar metadata lazily when repo settings opens', async () => {
+    const updateRepo = vi.fn()
+    // Non-fork repo (upstream resolved to null) transferred stablyai -> parkerrex.
+    const repo = makeRepo({
+      upstream: null,
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/stablyai.png?size=64',
+        source: 'github',
+        label: 'stablyai/orca'
+      }
+    })
+    apiMocks.repoUpstream.mockResolvedValueOnce(null)
+    apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'parkerrex', repo: 'orca' })
+
+    act(() => {
+      root.render(<RepositoryIconPicker repo={repo} updateRepo={updateRepo} />)
+    })
+    await flushEffects()
+
+    expect(updateRepo).toHaveBeenCalledExactlyOnceWith('repo-1', {
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/parkerrex.png?size=64',
+        source: 'github',
+        label: 'parkerrex/orca'
+      }
+    })
+  })
+
+  it('does not clobber a fork identity when the live upstream lookup fails offline', async () => {
+    const updateRepo = vi.fn()
+    // A fork whose avatar tracks its parent org, resolved earlier while online.
+    const repo = makeRepo({
+      upstream: { owner: 'stablyai', repo: 'orca' },
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/stablyai.png?size=64',
+        source: 'github',
+        label: 'stablyai/orca'
+      }
+    })
+    // Offline/unauthed: the parent lookup returns null. The fork's own origin
+    // owner must NOT be persisted over the parent identity.
+    apiMocks.repoUpstream.mockResolvedValueOnce(null)
+    apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'parkerrex', repo: 'orca' })
+
+    act(() => {
+      root.render(<RepositoryIconPicker repo={repo} updateRepo={updateRepo} />)
+    })
+    await flushEffects()
+
+    expect(updateRepo).not.toHaveBeenCalled()
+    expect(apiMocks.repoSlug).not.toHaveBeenCalled()
+  })
+})
@@ -2,7 +2,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'
 import { toast } from 'sonner'
 import { RotateCcw } from 'lucide-react'
 import type { Repo } from '../../../../shared/types'
-import { githubAvatarIcon, type RepoIcon } from '../../../../shared/repo-icon'
+import type { RepoIcon } from '../../../../shared/repo-icon'
 import { DEFAULT_REPO_BADGE_COLOR } from '../../../../shared/constants'
 import { normalizeRepoBadgeColor } from '../../../../shared/repo-badge-color'
 import { Button } from '../ui/button'
@@ -15,7 +15,8 @@ import { useMountedRef } from '@/hooks/useMountedRef'
 import { RepositoryIconColorSection } from './RepositoryIconColorSection'
 import { RepositoryIconTabs } from './RepositoryIconTabs'
 import {
-  resolveRepositoryGitHubAvatarIcon,
+  buildRepositoryGitHubAvatarUpdate,
+  resolveRepositoryGitHubAvatar,
  resolveRepositoryUpstreamLive
 } from './repository-icon-github'
 import { translate } from '@/i18n/i18n'
@@ -72,19 +73,20 @@ export function RepositoryIconPicker({
    [runtimeTarget, repo]
  )

-  const resolveGitHubAvatarIcon = useCallback(
-    () => resolveRepositoryGitHubAvatarIcon(runtimeTarget, repo),
+  const resolveGitHubAvatar = useCallback(
+    (options?: { forceLive?: boolean }) =>
+      resolveRepositoryGitHubAvatar(runtimeTarget, repo, options),
    [runtimeTarget, repo]
  )

  const handleUseGitHubAvatar = async () => {
    setLoadingGitHub(true)
    try {
-      const icon = await resolveGitHubAvatarIcon()
+      const resolution = await resolveGitHubAvatar({ forceLive: true })
      if (!mountedRef.current) {
        return
      }
-      if (!icon) {
+      if (!resolution.repoIcon) {
        toast.error(
          translate(
            'auto.components.settings.RepositoryIconPicker.f79972271a',
@@ -93,7 +95,11 @@ export function RepositoryIconPicker({
        )
        return
      }
-      setIcon(icon)
+      // A null build means the stored icon/upstream already match — nothing to write.
+      const updates = buildRepositoryGitHubAvatarUpdate(repo, resolution)
+      if (updates) {
+        updateRepo(repo.id, updates)
+      }
    } catch {
      if (mountedRef.current) {
        toast.error(
@@ -113,11 +119,16 @@ export function RepositoryIconPicker({
  const handleResetToDefault = async () => {
    setResetting(true)
    try {
-      const icon = await resolveGitHubAvatarIcon().catch(() => null)
+      const resolution = await resolveGitHubAvatar({ forceLive: true }).catch(() => null)
      if (!mountedRef.current) {
        return
      }
-      setIcon(icon)
+      const updates = resolution
+        ? buildRepositoryGitHubAvatarUpdate(repo, resolution, { clearMissingIcon: true })
+        : { repoIcon: null }
+      if (updates) {
+        updateRepo(repo.id, updates)
+      }
    } finally {
      if (mountedRef.current) {
        setResetting(false)
@@ -125,33 +136,39 @@ export function RepositoryIconPicker({
    }
  }

-  const upstreamBackfilledRef = useRef<string | null>(null)
+  const githubIdentityRefreshedRef = useRef<string | null>(null)
  useEffect(() => {
-    if (repo.upstream !== undefined || upstreamBackfilledRef.current === repo.id) {
+    const hasGitHubAvatar = repo.repoIcon?.type === 'image' && repo.repoIcon.source === 'github'
+    const shouldRefresh = hasGitHubAvatar || repo.upstream === undefined
+    if (!shouldRefresh || githubIdentityRefreshedRef.current === repo.id) {
      return
    }
-    upstreamBackfilledRef.current = repo.id
+    githubIdentityRefreshedRef.current = repo.id
    let cancelled = false
    void (async () => {
-      let upstream
+      let updates: Partial<Repo> | null
      try {
-        upstream = await resolveUpstreamLive()
+        if (hasGitHubAvatar) {
+          // Why: stored upstream/icon metadata can outlive a GitHub repo transfer.
+          // Refresh only when settings opens for the affected GitHub-avatar repo.
+          const resolution = await resolveGitHubAvatar({ forceLive: true })
+          updates = buildRepositoryGitHubAvatarUpdate(repo, resolution)
+        } else {
+          const upstream = await resolveUpstreamLive()
+          updates = { upstream: upstream ?? null }
+        }
      } catch {
        return
      }
-      if (cancelled || !mountedRef.current) {
+      if (cancelled || !mountedRef.current || !updates) {
        return
      }
-      const updates: Partial<Repo> = { upstream: upstream ?? null }
-      if (upstream && repo.repoIcon?.type === 'image' && repo.repoIcon.source === 'github') {
-        updates.repoIcon = githubAvatarIcon(upstream)
-      }
      updateRepo(repo.id, updates)
    })()
    return () => {
      cancelled = true
    }
-  }, [repo.id, repo.upstream, repo.repoIcon, resolveUpstreamLive, updateRepo, mountedRef])
+  }, [repo, resolveGitHubAvatar, resolveUpstreamLive, updateRepo, mountedRef])

  return (
    <div className="space-y-3">
@@ -0,0 +1,163 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+import type { Repo } from '../../../../shared/types'
+import {
+  buildRepositoryGitHubAvatarUpdate,
+  resolveRepositoryGitHubAvatar
+} from './repository-icon-github'
+
+vi.mock('@/runtime/runtime-rpc-client', () => ({
+  callRuntimeRpc: vi.fn()
+}))
+
+const apiMocks = {
+  repoSlug: vi.fn(),
+  repoUpstream: vi.fn()
+}
+
+// @ts-expect-error test window mock
+globalThis.window = { api: { gh: apiMocks } }
+
+function makeRepo(overrides: Partial<Repo> = {}): Repo {
+  return {
+    id: 'repo-1',
+    path: '/workspace/orca',
+    displayName: 'orca',
+    badgeColor: '#2563eb',
+    addedAt: 1,
+    kind: 'git',
+    ...overrides
+  }
+}
+
+describe('repository GitHub avatar resolution', () => {
+  beforeEach(() => {
+    apiMocks.repoSlug.mockReset()
+    apiMocks.repoUpstream.mockReset()
+  })
+
+  it('uses stored upstream by default to avoid unnecessary live checks', async () => {
+    const repo = makeRepo({ upstream: { owner: 'stablyai', repo: 'orca' } })
+
+    await expect(resolveRepositoryGitHubAvatar({ kind: 'local' }, repo)).resolves.toEqual({
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/stablyai.png?size=64',
+        source: 'github',
+        label: 'stablyai/orca'
+      },
+      upstream: { owner: 'stablyai', repo: 'orca' }
+    })
+
+    expect(apiMocks.repoUpstream).not.toHaveBeenCalled()
+    expect(apiMocks.repoSlug).not.toHaveBeenCalled()
+  })
+
+  it('force-resolves the live origin owner when a non-fork repo was transferred', async () => {
+    // Non-fork repo (upstream resolved to null) transferred stablyai -> parkerrex.
+    // The cached avatar is stale; forceLive must consult the live origin slug.
+    const repo = makeRepo({
+      upstream: null,
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/stablyai.png?size=64',
+        source: 'github',
+        label: 'stablyai/orca'
+      }
+    })
+    apiMocks.repoUpstream.mockResolvedValueOnce(null)
+    apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'parkerrex', repo: 'orca' })
+
+    const resolution = await resolveRepositoryGitHubAvatar({ kind: 'local' }, repo, {
+      forceLive: true
+    })
+
+    expect(resolution).toEqual({
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/parkerrex.png?size=64',
+        source: 'github',
+        label: 'parkerrex/orca'
+      },
+      upstream: null
+    })
+    expect(apiMocks.repoUpstream).toHaveBeenCalledExactlyOnceWith({
+      repoPath: '/workspace/orca',
+      repoId: 'repo-1'
+    })
+    expect(apiMocks.repoSlug).toHaveBeenCalledExactlyOnceWith({
+      repoPath: '/workspace/orca',
+      repoId: 'repo-1'
+    })
+    // upstream stays null (unchanged); only the avatar advances to the new owner.
+    expect(buildRepositoryGitHubAvatarUpdate(repo, resolution)).toEqual({
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/parkerrex.png?size=64',
+        source: 'github',
+        label: 'parkerrex/orca'
+      }
+    })
+  })
+
+  it('does not clear a GitHub avatar on passive refresh when live slug is unavailable', async () => {
+    const repo = makeRepo({
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/stablyai.png?size=64',
+        source: 'github',
+        label: 'stablyai/orca'
+      }
+    })
+
+    expect(buildRepositoryGitHubAvatarUpdate(repo, { repoIcon: null, upstream: null })).toEqual({
+      upstream: null
+    })
+    expect(
+      buildRepositoryGitHubAvatarUpdate(
+        repo,
+        { repoIcon: null, upstream: null },
+        {
+          clearMissingIcon: true
+        }
+      )
+    ).toEqual({
+      upstream: null,
+      repoIcon: null
+    })
+  })
+
+  it('preserves a known fork identity when the live upstream lookup fails', async () => {
+    // A fork whose avatar tracks its parent org. The live upstream probe fails
+    // (offline/unauthed → null), which must NOT downgrade to the origin slug.
+    const repo = makeRepo({
+      upstream: { owner: 'stablyai', repo: 'orca' },
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/stablyai.png?size=64',
+        source: 'github',
+        label: 'stablyai/orca'
+      }
+    })
+    apiMocks.repoUpstream.mockResolvedValueOnce(null)
+    // The fork's own origin owner — the value we must NOT persist over the parent.
+    apiMocks.repoSlug.mockResolvedValueOnce({ owner: 'parkerrex', repo: 'orca' })
+
+    const resolution = await resolveRepositoryGitHubAvatar({ kind: 'local' }, repo, {
+      forceLive: true
+    })
+
+    expect(resolution).toEqual({
+      repoIcon: {
+        type: 'image',
+        src: 'https://github.com/stablyai.png?size=64',
+        source: 'github',
+        label: 'stablyai/orca'
+      },
+      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()
+  })
+})
@@ -3,6 +3,14 @@ import { githubAvatarIcon, type RepoIcon } from '../../../../shared/repo-icon'
 import { callRuntimeRpc, type getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client'

 type RuntimeTarget = ReturnType<typeof getActiveRuntimeTarget>
+type ResolveRepositoryGitHubAvatarOptions = {
+  forceLive?: boolean
+}
+
+export type RepositoryGitHubAvatarResolution = {
+  repoIcon: RepoIcon | null
+  upstream: GitHubRepositoryIdentity | null
+}

 export async function resolveRepositoryUpstreamLive(
  runtimeTarget: RuntimeTarget,
@@ -18,25 +26,85 @@ export async function resolveRepositoryUpstreamLive(
    : await window.api.gh.repoUpstream({ repoPath: repo.path, repoId: repo.id })
 }

-export async function resolveRepositoryGitHubAvatarIcon(
+async function resolveRepositorySlugLive(
  runtimeTarget: RuntimeTarget,
  repo: Repo
-): Promise<RepoIcon | null> {
+): Promise<GitHubRepositoryIdentity | null> {
+  return runtimeTarget.kind === 'environment'
+    ? await callRuntimeRpc<GitHubRepositoryIdentity | null>(
+        runtimeTarget,
+        'github.repoSlug',
+        { repo: repo.id },
+        { timeoutMs: 30_000 }
+      )
+    : await window.api.gh.repoSlug({ repoPath: repo.path, repoId: repo.id })
+}
+
+export async function resolveRepositoryGitHubAvatar(
+  runtimeTarget: RuntimeTarget,
+  repo: Repo,
+  options: ResolveRepositoryGitHubAvatarOptions = {}
+): Promise<RepositoryGitHubAvatarResolution> {
  const upstream =
-    repo.upstream !== undefined
+    !options.forceLive && repo.upstream !== undefined
      ? repo.upstream
      : await resolveRepositoryUpstreamLive(runtimeTarget, repo).catch(() => null)
  if (upstream) {
-    return githubAvatarIcon(upstream)
+    return { repoIcon: githubAvatarIcon(upstream), upstream }
  }
-  const slug =
-    runtimeTarget.kind === 'environment'
-      ? await callRuntimeRpc<{ owner: string; repo: string } | null>(
-          runtimeTarget,
-          'github.repoSlug',
-          { repo: repo.id },
-          { timeoutMs: 30_000 }
-        )
-      : await window.api.gh.repoSlug({ repoPath: repo.path, repoId: repo.id })
-  return slug ? githubAvatarIcon(slug) : null
+  // Why: a null live upstream is ambiguous (offline/unauthed vs genuinely not a
+  // fork). Don't downgrade a known fork identity to the origin slug — 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 }
+}
+
+function sameRepositoryIdentity(
+  a: GitHubRepositoryIdentity | null | undefined,
+  b: GitHubRepositoryIdentity | null | undefined
+): boolean {
+  if (!a || !b) {
+    return a === b
+  }
+  return a.owner === b.owner && a.repo === b.repo
+}
+
+function sameRepoIcon(a: RepoIcon | null | undefined, b: RepoIcon | null | undefined): boolean {
+  if (!a || !b) {
+    return a === b
+  }
+  if (a.type !== b.type) {
+    return false
+  }
+  if (a.type === 'image' && b.type === 'image') {
+    return a.src === b.src && a.source === b.source && a.label === b.label
+  }
+  if (a.type === 'emoji' && b.type === 'emoji') {
+    return a.emoji === b.emoji
+  }
+  return a.type === 'lucide' && b.type === 'lucide' && a.name === b.name
+}
+
+export function buildRepositoryGitHubAvatarUpdate(
+  repo: Repo,
+  resolution: RepositoryGitHubAvatarResolution,
+  options: { clearMissingIcon?: boolean } = {}
+): Partial<Repo> | null {
+  const updates: Partial<Repo> = {}
+
+  if (!sameRepositoryIdentity(repo.upstream, resolution.upstream)) {
+    updates.upstream = resolution.upstream
+  }
+
+  if (
+    (resolution.repoIcon || options.clearMissingIcon) &&
+    !sameRepoIcon(repo.repoIcon, resolution.repoIcon)
+  ) {
+    updates.repoIcon = resolution.repoIcon
+  }
+
+  return Object.keys(updates).length > 0 ? updates : null
 }