Fix stale GitHub repo avatars after owner changes#6507
Conversation
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/renderer/src/components/settings/repository-icon-github.ts (1)
104-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
clearMissingIconrationale inline.This guard carries the PR’s “passive refresh must not clear an existing GitHub avatar” rule, but that policy is only implicit here. A short
Why:comment would make the opt-in clear behavior much easier to preserve.As per coding guidelines, "When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does."
Suggested comment
if ( + // Why: passive refresh preserves the current GitHub avatar when live + // identity cannot be resolved; explicit reset opts into clearing it. (resolution.repoIcon || options.clearMissingIcon) && !sameRepoIcon(repo.repoIcon, resolution.repoIcon) ) {Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6db9e2ea-a0ac-4b8d-b47b-92103bcc409f
📒 Files selected for processing (4)
src/renderer/src/components/settings/RepositoryIconPicker.github-avatar-refresh.test.tsxsrc/renderer/src/components/settings/RepositoryIconPicker.tsxsrc/renderer/src/components/settings/repository-icon-github.test.tssrc/renderer/src/components/settings/repository-icon-github.ts
| const upstream = | ||
| repo.upstream !== undefined | ||
| !options.forceLive && repo.upstream !== undefined | ||
| ? repo.upstream | ||
| : await resolveRepositoryUpstreamLive(runtimeTarget, repo).catch(() => null) | ||
| if (upstream) { | ||
| return githubAvatarIcon(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 | ||
| return { repoIcon: githubAvatarIcon(upstream), upstream } | ||
| } | ||
| const slug = await resolveRepositorySlugLive(runtimeTarget, repo) | ||
| return { repoIcon: slug ? githubAvatarIcon(slug) : null, upstream: null } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't collapse upstream lookup errors into null.
Line 51 swallows every resolveRepositoryUpstreamLive() failure and then Lines 55-56 fall back to repoSlug. If repoSlug succeeds, buildRepositoryGitHubAvatarUpdate() can persist upstream: null, which rewrites a still-forked repo as “not a fork” after a transient RPC/IPC failure. Let the error abort the refresh (or preserve the existing upstream) and only slug-fallback on an actual null upstream result.
Proposed fix
export async function resolveRepositoryGitHubAvatar(
runtimeTarget: RuntimeTarget,
repo: Repo,
options: ResolveRepositoryGitHubAvatarOptions = {}
): Promise<RepositoryGitHubAvatarResolution> {
const upstream =
!options.forceLive && repo.upstream !== undefined
? repo.upstream
- : await resolveRepositoryUpstreamLive(runtimeTarget, repo).catch(() => null)
+ : await resolveRepositoryUpstreamLive(runtimeTarget, repo)
if (upstream) {
return { repoIcon: githubAvatarIcon(upstream), upstream }
}
const slug = await resolveRepositorySlugLive(runtimeTarget, repo)
return { repoIcon: slug ? githubAvatarIcon(slug) : null, upstream: null }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const upstream = | |
| repo.upstream !== undefined | |
| !options.forceLive && repo.upstream !== undefined | |
| ? repo.upstream | |
| : await resolveRepositoryUpstreamLive(runtimeTarget, repo).catch(() => null) | |
| if (upstream) { | |
| return githubAvatarIcon(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 | |
| return { repoIcon: githubAvatarIcon(upstream), upstream } | |
| } | |
| const slug = await resolveRepositorySlugLive(runtimeTarget, repo) | |
| return { repoIcon: slug ? githubAvatarIcon(slug) : null, upstream: null } | |
| export async function resolveRepositoryGitHubAvatar( | |
| runtimeTarget: RuntimeTarget, | |
| repo: Repo, | |
| options: ResolveRepositoryGitHubAvatarOptions = {} | |
| ): Promise<RepositoryGitHubAvatarResolution> { | |
| const upstream = | |
| !options.forceLive && repo.upstream !== undefined | |
| ? repo.upstream | |
| : await resolveRepositoryUpstreamLive(runtimeTarget, repo) | |
| if (upstream) { | |
| return { repoIcon: githubAvatarIcon(upstream), upstream } | |
| } | |
| const slug = await resolveRepositorySlugLive(runtimeTarget, repo) | |
| return { repoIcon: slug ? githubAvatarIcon(slug) : null, upstream: null } | |
| } |
| // @ts-expect-error test window mock | ||
| globalThis.window = { api: { gh: apiMocks } } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve happy-dom’s real window object.
Lines 30-31 replace globalThis.window with a plain object, which strips the real happy-dom window of document, timers, constructors, and other DOM state. That makes this test run against a broken browser global and can hide future regressions. Patch window.api instead of replacing window.
Proposed fix
-// `@ts-expect-error` test window mock
-globalThis.window = { api: { gh: apiMocks } }
+Object.defineProperty(window, 'api', {
+ value: { gh: apiMocks },
+ configurable: true
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-expect-error test window mock | |
| globalThis.window = { api: { gh: apiMocks } } | |
| Object.defineProperty(window, 'api', { | |
| value: { gh: apiMocks }, | |
| configurable: true | |
| }) |
1 participant
Author's Note
Hey it's Parker - super random edge case I ran into. If you change repo location from Org -> Personal or vice-versa, the image goes stale.
This keeps the fix lazy instead of checking every repo on startup. Orca only refreshes the GitHub avatar identity when the repo icon settings are opened for a repo already using the GitHub avatar, or when the user explicitly hits Reset / GitHub Avatar.
Decision / Tradeoff
We looked at a couple of broader repair options and kept this intentionally narrow:
gh.repoUpstream/gh.repoSlugcalls.The tradeoff is that the stale image self-heals when the user opens that repo's icon settings or explicitly uses Reset / GitHub Avatar, not immediately when
.git/configchanges behind Orca's back. That seemed like the right balance for this edge case.Summary
Screenshots
No visual change.
Testing
pnpm exec oxlint src/renderer/src/components/settings/repository-icon-github.ts src/renderer/src/components/settings/RepositoryIconPicker.tsx src/renderer/src/components/settings/repository-icon-github.test.ts src/renderer/src/components/settings/RepositoryIconPicker.github-avatar-refresh.test.tsxpnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/settings/repository-icon-github.test.ts src/renderer/src/components/settings/RepositoryIconPicker.github-avatar-refresh.test.tsxpnpm run typecheck:webgit diff --checkLocal note: this shell is on Node
v25.9.0whilepackage.jsondeclares Node24, so pnpm printed engine warnings. The commands above completed successfully.AI Review Report
Codex reviewed the patch while implementing it, with focus on avoiding startup-wide repo scanning, preserving custom icon choices, keeping SSH/runtime routing on the existing repo-owner target path, and preventing passive refresh from clearing a working avatar during transient GitHub/gh failures.
No separate external AI review command was run.
Security Audit
No new IPC channels, command execution, auth scopes, secrets, dependencies, or path handling were added. The change reuses existing
gh.repoUpstream,gh.repoSlug, andrepos:updatepaths, and only writes sanitizedRepoIcon/ upstream metadata already accepted by the repo settings persistence layer.Notes