diff --git a/mobile/app/h/[hostId]/tasks.tsx b/mobile/app/h/[hostId]/tasks.tsx index c41b165cbf9..0794c317e41 100644 --- a/mobile/app/h/[hostId]/tasks.tsx +++ b/mobile/app/h/[hostId]/tasks.tsx @@ -165,6 +165,8 @@ type RepoSummary = { kind?: 'git' | 'folder' connectionId?: string | null issueSourcePreference?: IssueSourcePreference + /** Fork parent resolved by the host; drives upstream Project row matching. */ + upstream?: { owner: string; repo: string; host?: string } | null } type IssueSourcePreference = 'upstream' | 'origin' | 'auto' diff --git a/mobile/src/tasks/github-project-repo-match.test.ts b/mobile/src/tasks/github-project-repo-match.test.ts index bce71393dab..58c44a57830 100644 --- a/mobile/src/tasks/github-project-repo-match.test.ts +++ b/mobile/src/tasks/github-project-repo-match.test.ts @@ -115,6 +115,136 @@ describe('GitHub project repo matching', () => { ).toBe(repos[1]) }) + it('matches an upstream project row against a fork clone', () => { + const fork = { + id: 'repo-1', + path: '/Users/me/r2r-mirror', + displayName: 'r2r-mirror', + upstream: { owner: 'SciPhi-AI', repo: 'R2R' } + } + + expect( + findRepoForGitHubProjectRepository('SciPhi-AI/R2R', [fork], { + 'repo-1': { + path: '/Users/me/r2r-mirror', + repository: { owner: 'me', repo: 'r2r-mirror' } + } + }) + ).toBe(fork) + }) + + it('prefers the clone that owns the slug over a fork of it', () => { + const upstreamClone = { id: 'repo-1', path: '/Users/me/r2r', displayName: 'r2r' } + const fork = { + id: 'repo-2', + path: '/Users/me/r2r-mirror', + displayName: 'r2r-mirror', + upstream: { owner: 'SciPhi-AI', repo: 'R2R' } + } + + expect( + findRepoForGitHubProjectRepository('SciPhi-AI/R2R', [upstreamClone, fork], { + 'repo-1': { + path: '/Users/me/r2r', + repository: { owner: 'SciPhi-AI', repo: 'R2R' } + }, + 'repo-2': { + path: '/Users/me/r2r-mirror', + repository: { owner: 'me', repo: 'r2r-mirror' } + } + }) + ).toBe(upstreamClone) + }) + + it('does not pick a repo when two forks share the same upstream', () => { + const forks = [ + { + id: 'repo-1', + path: '/Users/me/a', + displayName: 'a', + upstream: { owner: 'SciPhi-AI', repo: 'R2R' } + }, + { + id: 'repo-2', + path: '/Users/me/b', + displayName: 'b', + upstream: { owner: 'SciPhi-AI', repo: 'R2R' } + } + ] + + expect( + findRepoForGitHubProjectRepository('SciPhi-AI/R2R', forks, { + 'repo-1': { path: '/Users/me/a', repository: { owner: 'me', repo: 'a' } }, + 'repo-2': { path: '/Users/me/b', repository: { owner: 'me', repo: 'b' } } + }) + ).toBeNull() + }) + + it('does not bind a github.com fork parent to a same-named Enterprise row', () => { + const fork = { + id: 'repo-1', + path: '/Users/me/r2r-mirror', + displayName: 'r2r-mirror', + upstream: { owner: 'SciPhi-AI', repo: 'R2R' } + } + + expect( + findRepoForGitHubProjectRepository( + 'SciPhi-AI/R2R', + [fork], + { + 'repo-1': { + path: '/Users/me/r2r-mirror', + repository: { owner: 'me', repo: 'r2r-mirror', host: 'github.com' } + } + }, + 'github.acme-corp.com' + ) + ).toBeNull() + }) + + it('drops the fork alias while its own origin is unresolved', () => { + const fork = { + id: 'repo-1', + path: '/Users/me/widgets-mirror', + displayName: 'widgets-mirror', + upstream: { owner: 'acme', repo: 'widgets' } + } + + for (const slugs of [ + {}, + { 'repo-1': { path: '/Users/me/widgets-mirror', repository: null } }, + { 'repo-1': { path: '/moved', repository: { owner: 'me', repo: 'widgets' } } } + ]) { + expect(findRepoForGitHubProjectRepository('acme/widgets', [fork], slugs)).toBeNull() + } + }) + + it('scopes a host-less fork parent to the host the fork itself was cloned from', () => { + const enterpriseFork = { + id: 'repo-1', + path: '/Users/me/widgets-mirror', + displayName: 'widgets-mirror', + upstream: { owner: 'acme', repo: 'widgets' } + } + const slugs = { + 'repo-1': { + path: '/Users/me/widgets-mirror', + repository: { owner: 'me', repo: 'widgets', host: 'github.acme-corp.com' } + } + } + + expect( + findRepoForGitHubProjectRepository( + 'acme/widgets', + [enterpriseFork], + slugs, + 'github.acme-corp.com' + ) + ).toBe(enterpriseFork) + expect(findRepoForGitHubProjectRepository('acme/widgets', [enterpriseFork], slugs)).toBeNull() + }) + it('does not use hostless path heuristics for Enterprise Project rows', () => { expect( findRepoForGitHubProjectRepository( diff --git a/mobile/src/tasks/github-project-repo-match.ts b/mobile/src/tasks/github-project-repo-match.ts index c4d1e1d2ea6..1e9cd61996a 100644 --- a/mobile/src/tasks/github-project-repo-match.ts +++ b/mobile/src/tasks/github-project-repo-match.ts @@ -7,6 +7,9 @@ export type GitHubProjectRepoMatch = { id: string path: string displayName: string + /** Fork parent resolved by the host and carried on `repo.list`. Absent = not + * a fork or not yet resolved. */ + upstream?: { owner: string; repo: string; host?: string } | null } export type GitHubRepoSlugCacheEntry = { @@ -45,6 +48,27 @@ function cachedSlugStateForRepo( return { status: 'resolved', repository: cached.repository } } +/** Identity key of the repo's fork parent, or null when it is not a fork or its + * origin has not resolved. Why: when `upstream.host` is absent (older persisted + * forks), the fork's origin host is the fallback so GHES parents do not collapse + * into github.com. Unresolved origins refuse the alias. */ +function upstreamIdentityKeyForRepo( + repo: GitHubProjectRepoMatch, + originState: CachedSlugState | undefined +): string | null { + const upstream = repo.upstream + if (!upstream?.owner || !upstream.repo) { + return null + } + if (originState?.status !== 'resolved' || !originState.repository) { + return null + } + return githubRepoIdentityKey({ + ...upstream, + host: upstream.host ?? originState.repository.host + }) +} + export function findRepoForGitHubProjectRepository( repository: string | null | undefined, repos: GitHubProjectRepoMatch[], @@ -79,6 +103,20 @@ export function findRepoForGitHubProjectRepository( return null } + // Why: a Project card references the upstream repo, but a contributor's clone + // has their personal fork as `origin`, so origin-only matching hid every row + // (#12647). Checked after origin so an open clone of the upstream repo itself + // always wins over someone's fork of it. + const upstreamMatches = repos.filter( + (repo) => upstreamIdentityKeyForRepo(repo, slugStates.get(repo.id)) === requestedIdentityKey + ) + if (upstreamMatches.length === 1) { + return upstreamMatches[0]! + } + if (upstreamMatches.length > 1) { + return null + } + if (!isDefaultGitHubHost(projectHost)) { // Why: display names and local paths contain no host evidence, so using // them for GHES rows could bind an Enterprise item to a github.com repo. diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 059402ae9ed..b59ca6f5986 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -4617,6 +4617,43 @@ describe('Store', () => { expect(store.getRepos()[0]!.upstream).toBeUndefined() }) + it('keeps the upstream host across reloads so it is never re-inferred from origin', async () => { + const store = await createStore() + store.addRepo(makeRepo()) + + const updated = store.updateRepo('r1', { + upstream: { owner: ' acme ', repo: ' widgets ', host: ' GHE.example:8443 ' } + }) + expect(updated!.upstream).toEqual({ + owner: 'acme', + repo: 'widgets', + host: 'GHE.example:8443' + }) + + store.flush() + const reloaded = await createStore() + expect(reloaded.getRepo('r1')!.upstream).toEqual({ + owner: 'acme', + repo: 'widgets', + host: 'GHE.example:8443' + }) + }) + + it('leaves a hostless persisted upstream hostless rather than inventing one', async () => { + const store = await createStore() + store.addRepo(makeRepo({ upstream: { owner: 'stablyai', repo: 'orca' } })) + + expect(store.getRepo('r1')!.upstream).toEqual({ owner: 'stablyai', repo: 'orca' }) + expect(store.getRepo('r1')!.upstream).not.toHaveProperty('host') + }) + + it('drops a blank upstream host instead of persisting an empty string', async () => { + const store = await createStore() + store.addRepo(makeRepo({ upstream: { owner: 'acme', repo: 'widgets', host: ' ' } })) + + expect(store.getRepo('r1')!.upstream).toEqual({ owner: 'acme', repo: 'widgets' }) + }) + it('updateRepo returns null for nonexistent id', async () => { const store = await createStore() expect(store.updateRepo('nope', { displayName: 'x' })).toBeNull() diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 5d581a7f6e6..04188206ad7 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -1411,10 +1411,18 @@ function sanitizeRepoUpstream(value: unknown): Repo['upstream'] | undefined { if (!value || typeof value !== 'object') { return undefined } - const candidate = value as { owner?: unknown; repo?: unknown } + const candidate = value as { owner?: unknown; repo?: unknown; host?: unknown } const owner = typeof candidate.owner === 'string' ? candidate.owner.trim() : '' const repo = typeof candidate.repo === 'string' ? candidate.repo.trim() : '' - return owner && repo ? { owner, repo } : undefined + if (!owner || !repo) { + return undefined + } + // Why: an `upstream` remote may live on a different server than `origin`, so + // dropping the host forced consumers to re-infer it from origin and could bind + // a GHES parent to a same-named github.com repo. Absent host stays absent so + // records written before this survive unchanged. + const host = typeof candidate.host === 'string' ? candidate.host.trim() : '' + return host ? { owner, repo, host } : { owner, repo } } function sanitizeGitRemoteIdentity(value: unknown): GitRemoteIdentity | null | undefined { diff --git a/src/renderer/src/components/github-project/ProjectViewWrapper.tsx b/src/renderer/src/components/github-project/ProjectViewWrapper.tsx index 39f43539ee9..59d02e24f31 100644 --- a/src/renderer/src/components/github-project/ProjectViewWrapper.tsx +++ b/src/renderer/src/components/github-project/ProjectViewWrapper.tsx @@ -141,7 +141,7 @@ export default function ProjectViewWrapper({ selectedRepoIds }: Props): React.JS const patchProjectRowIssueType = useAppStore((s) => s.patchProjectRowIssueType) const addRepoFromStore = useAppStore((s) => s.addRepo) const repos = useAppStore((s) => s.repos) - const { lookupSlug, ready: slugIndexReady } = useRepoSlugIndex() + const { lookupSlug, lookupSlugMatches, ready: slugIndexReady } = useRepoSlugIndex() const mountedRef = useMountedRef() const activeProject = settings?.githubProjects?.activeProject ?? null @@ -375,9 +375,14 @@ export default function ProjectViewWrapper({ selectedRepoIds }: Props): React.JS const filteredTable = useMemo( () => table && slugIndexReady - ? filterProjectTableRowsBySelectedRepos(table, lookupSlug, slugIndexReady, selectedRepoIds) + ? filterProjectTableRowsBySelectedRepos( + table, + lookupSlugMatches, + slugIndexReady, + selectedRepoIds + ) : null, - [table, slugIndexReady, lookupSlug, selectedRepoIds] + [table, slugIndexReady, lookupSlugMatches, selectedRepoIds] ) const lastFilteredTableRef = useRef(null) // Why: ref-cache prevents a blank table while the slug index rebuilds, without forcing a second render. @@ -528,7 +533,7 @@ export default function ProjectViewWrapper({ selectedRepoIds }: Props): React.JS } const resolution = resolveSelectedProjectRowRepo({ row, - lookupSlug, + lookupSlugMatches, host: table.project.host, slugIndexReady, selectedRepoIds @@ -584,7 +589,7 @@ export default function ProjectViewWrapper({ selectedRepoIds }: Props): React.JS currentCacheKey, table, buildOrigin, - lookupSlug, + lookupSlugMatches, slugIndexReady, selectedRepoIds, openProjectRowUrlWithToast @@ -602,7 +607,7 @@ export default function ProjectViewWrapper({ selectedRepoIds }: Props): React.JS } const resolution = resolveSelectedProjectRowRepo({ row, - lookupSlug, + lookupSlugMatches, host: table.project.host, slugIndexReady, selectedRepoIds @@ -671,7 +676,7 @@ export default function ProjectViewWrapper({ selectedRepoIds }: Props): React.JS currentCacheKey, table, buildOrigin, - lookupSlug, + lookupSlugMatches, slugIndexReady, selectedRepoIds, openProjectRowUrlWithToast diff --git a/src/renderer/src/components/github-project/project-row-filtering.test.ts b/src/renderer/src/components/github-project/project-row-filtering.test.ts index 708cef2fac5..8871c047f7a 100644 --- a/src/renderer/src/components/github-project/project-row-filtering.test.ts +++ b/src/renderer/src/components/github-project/project-row-filtering.test.ts @@ -17,6 +17,11 @@ function repo(id: string): Repo { } } +/** The common case: every match owns the slug through its own origin remote. */ +function originOnly(matches: Repo[]): { origin: Repo[]; upstream: Repo[] } { + return { origin: matches, upstream: [] } +} + function row(id: string, repository: string | null): GitHubProjectRow { return { id, @@ -102,7 +107,8 @@ describe('filterProjectTableRowsBySelectedRepos', () => { const rows = [row('visible', 'acme/orca'), row('hidden', 'acme/tool')] const filtered = filterProjectTableRowsBySelectedRepos( table(rows), - (slug) => (slug?.toLowerCase() === 'acme/orca' ? [repo('repo-1')] : [repo('repo-2')]), + (slug) => + originOnly(slug?.toLowerCase() === 'acme/orca' ? [repo('repo-1')] : [repo('repo-2')]), true, new Set(['repo-1']) ) @@ -115,7 +121,7 @@ describe('filterProjectTableRowsBySelectedRepos', () => { const rows = [row('hidden', 'acme/orca')] const filtered = filterProjectTableRowsBySelectedRepos( table(rows), - () => [repo('repo-2')], + () => originOnly([repo('repo-2')]), true, new Set(['repo-1']) ) @@ -124,11 +130,24 @@ describe('filterProjectTableRowsBySelectedRepos', () => { expect(filtered.totalCount).toBe(0) }) + it('keeps a row whose only selected match is a fork of the row s repo', () => { + const rows = [row('fork-only', 'acme/orca')] + const filtered = filterProjectTableRowsBySelectedRepos( + table(rows), + () => ({ origin: [], upstream: [repo('fork')] }), + true, + new Set(['fork']) + ) + + expect(filtered.rows.map((r) => r.id)).toEqual(['fork-only']) + expect(filtered.totalCount).toBe(1) + }) + it('keeps a row with multiple selected matches for action ambiguity handling', () => { const rows = [row('ambiguous', 'acme/orca')] const filtered = filterProjectTableRowsBySelectedRepos( table(rows), - () => [repo('repo-1'), repo('repo-2'), repo('repo-3')], + () => originOnly([repo('repo-1'), repo('repo-2'), repo('repo-3')]), true, new Set(['repo-1', 'repo-2']) ) @@ -141,7 +160,7 @@ describe('resolveSelectedProjectRowRepo', () => { it('reports loading without reading stale slug matches', () => { const resolution = resolveSelectedProjectRowRepo({ row: row('loading', 'acme/orca'), - lookupSlug: () => { + lookupSlugMatches: () => { throw new Error('should not read stale matches') }, slugIndexReady: false, @@ -154,7 +173,7 @@ describe('resolveSelectedProjectRowRepo', () => { it('reports invalid slug for rows without a repository', () => { const resolution = resolveSelectedProjectRowRepo({ row: row('missing-slug', null), - lookupSlug: () => [repo('repo-1')], + lookupSlugMatches: () => originOnly([repo('repo-1')]), slugIndexReady: true, selectedRepoIds: new Set(['repo-1']) }) @@ -165,7 +184,7 @@ describe('resolveSelectedProjectRowRepo', () => { it('reports no global match when Orca has no repo for the slug', () => { const resolution = resolveSelectedProjectRowRepo({ row: row('missing', 'acme/orca'), - lookupSlug: () => [], + lookupSlugMatches: () => originOnly([]), slugIndexReady: true, selectedRepoIds: new Set(['repo-1']) }) @@ -176,7 +195,7 @@ describe('resolveSelectedProjectRowRepo', () => { it('reports global-only matches when the repo is not selected', () => { const resolution = resolveSelectedProjectRowRepo({ row: row('unselected', 'acme/orca'), - lookupSlug: () => [repo('repo-2')], + lookupSlugMatches: () => originOnly([repo('repo-2')]), slugIndexReady: true, selectedRepoIds: new Set(['repo-1']) }) @@ -187,7 +206,7 @@ describe('resolveSelectedProjectRowRepo', () => { it('returns the selected match when exactly one matching repo is selected', () => { const resolution = resolveSelectedProjectRowRepo({ row: row('selected', 'acme/orca'), - lookupSlug: () => [repo('repo-1'), repo('repo-2')], + lookupSlugMatches: () => originOnly([repo('repo-1'), repo('repo-2')]), slugIndexReady: true, selectedRepoIds: new Set(['repo-2']) }) @@ -196,28 +215,64 @@ describe('resolveSelectedProjectRowRepo', () => { }) it('passes the project host into repository matching', () => { - const lookupSlug = vi.fn(() => [repo('repo-1')]) + const lookupSlugMatches = vi.fn(() => originOnly([repo('repo-1')])) expect( resolveSelectedProjectRowRepo({ row: row('enterprise', 'acme/orca'), - lookupSlug, + lookupSlugMatches, host: 'ghe.example:8443', slugIndexReady: true, selectedRepoIds: new Set(['repo-1']) }) ).toMatchObject({ status: 'selected_match' }) - expect(lookupSlug).toHaveBeenCalledWith('acme/orca', 'ghe.example:8443') + expect(lookupSlugMatches).toHaveBeenCalledWith('acme/orca', 'ghe.example:8443') }) it('reports ambiguity when multiple matching repos are selected', () => { const resolution = resolveSelectedProjectRowRepo({ row: row('ambiguous', 'acme/orca'), - lookupSlug: () => [repo('repo-1'), repo('repo-2')], + lookupSlugMatches: () => originOnly([repo('repo-1'), repo('repo-2')]), slugIndexReady: true, selectedRepoIds: new Set(['repo-1', 'repo-2']) }) expect(resolution.status).toBe('ambiguous_selected_match') }) + + it('falls through to a selected fork when the upstream clone is unselected', () => { + const resolution = resolveSelectedProjectRowRepo({ + row: row('fork-selected', 'acme/orca'), + lookupSlugMatches: () => ({ origin: [repo('upstream')], upstream: [repo('fork')] }), + slugIndexReady: true, + selectedRepoIds: new Set(['fork']) + }) + + expect(resolution).toMatchObject({ status: 'selected_match', repo: { id: 'fork' } }) + }) + + it('prefers the selected upstream clone over a selected fork of it', () => { + const resolution = resolveSelectedProjectRowRepo({ + row: row('both-selected', 'acme/orca'), + lookupSlugMatches: () => ({ origin: [repo('upstream')], upstream: [repo('fork')] }), + slugIndexReady: true, + selectedRepoIds: new Set(['upstream', 'fork']) + }) + + expect(resolution).toMatchObject({ status: 'selected_match', repo: { id: 'upstream' } }) + }) + + it('still reports no selection when neither the upstream clone nor the fork is selected', () => { + const resolution = resolveSelectedProjectRowRepo({ + row: row('neither', 'acme/orca'), + lookupSlugMatches: () => ({ origin: [repo('upstream')], upstream: [repo('fork')] }), + slugIndexReady: true, + selectedRepoIds: new Set(['other']) + }) + + expect(resolution).toMatchObject({ + status: 'unselected_match', + globalMatches: [{ id: 'upstream' }, { id: 'fork' }] + }) + }) }) diff --git a/src/renderer/src/components/github-project/project-row-filtering.ts b/src/renderer/src/components/github-project/project-row-filtering.ts index 1aa2f4c3ed8..52dd0138274 100644 --- a/src/renderer/src/components/github-project/project-row-filtering.ts +++ b/src/renderer/src/components/github-project/project-row-filtering.ts @@ -6,6 +6,13 @@ export type ProjectRowSlugLookup = ( host?: string ) => readonly Repo[] +/** Origin matches own the slug through their own remote; upstream matches are + * forks whose parent the row names. */ +export type ProjectRowSlugMatchLookup = ( + slug: string | null | undefined, + host?: string +) => { origin: readonly Repo[]; upstream: readonly Repo[] } + export type SelectedProjectRowResolution = | { status: 'loading' } | { status: 'invalid_slug' } @@ -20,7 +27,7 @@ export type SelectedProjectRowResolution = export function resolveSelectedProjectRowRepo(input: { row: GitHubProjectRow - lookupSlug: ProjectRowSlugLookup + lookupSlugMatches: ProjectRowSlugMatchLookup host?: string slugIndexReady: boolean selectedRepoIds: ReadonlySet @@ -38,12 +45,20 @@ export function resolveSelectedProjectRowRepo(input: { return { status: 'invalid_slug' } } - const globalMatches = input.lookupSlug(repository, input.host) + const { origin, upstream } = input.lookupSlugMatches(repository, input.host) + const globalMatches = [...origin, ...upstream] if (globalMatches.length === 0) { return { status: 'no_global_match' } } - const selectedMatches = globalMatches.filter((match) => input.selectedRepoIds.has(match.id)) + // Why: prefer origin only among repos the user actually selected. Applying + // that preference globally let an open-but-unselected clone of the upstream + // repo hide the selected fork, reproducing #12647 for anyone holding both. + const selectedOrigin = origin.filter((match) => input.selectedRepoIds.has(match.id)) + const selectedMatches = + selectedOrigin.length > 0 + ? selectedOrigin + : upstream.filter((match) => input.selectedRepoIds.has(match.id)) if (selectedMatches.length === 0) { return { status: 'unselected_match', globalMatches } } @@ -76,14 +91,14 @@ export function filterProjectTableRowsByOpenRepos( export function filterProjectTableRowsBySelectedRepos( table: GitHubProjectTable, - lookupSlug: ProjectRowSlugLookup, + lookupSlugMatches: ProjectRowSlugMatchLookup, slugIndexReady: boolean, selectedRepoIds: ReadonlySet ): GitHubProjectTable { const rows = table.rows.filter((row) => { const resolution = resolveSelectedProjectRowRepo({ row, - lookupSlug, + lookupSlugMatches, host: table.project.host, slugIndexReady, selectedRepoIds diff --git a/src/renderer/src/lib/repo-slug-cache.test.ts b/src/renderer/src/lib/repo-slug-cache.test.ts index dd0f9e4bd28..d9de7ae3e7b 100644 --- a/src/renderer/src/lib/repo-slug-cache.test.ts +++ b/src/renderer/src/lib/repo-slug-cache.test.ts @@ -46,6 +46,62 @@ describe('repo slug cache host identity', () => { ).toEqual([enterprise]) }) + it('routes an upstream project row to the fork clone that tracks it', () => { + const fork = { ...repo('fork'), upstream: { owner: 'SciPhi-AI', repo: 'R2R' } } + slugByRepoId.set( + slugCacheKey(fork.id, settingsForRepoOwner(fork, null)), + githubRepoIdentityKey({ owner: 'me', repo: 'r2r-mirror' }) + ) + + expect(lookupReposBySlugFromCache([fork], null, 'SciPhi-AI/R2R')).toEqual([fork]) + }) + + it('prefers the clone that owns the slug over a fork of it', () => { + const origin = repo('origin') + const fork = { ...repo('fork'), upstream: { owner: 'SciPhi-AI', repo: 'R2R' } } + slugByRepoId.set( + slugCacheKey(origin.id, settingsForRepoOwner(origin, null)), + githubRepoIdentityKey({ owner: 'SciPhi-AI', repo: 'R2R' }) + ) + slugByRepoId.set( + slugCacheKey(fork.id, settingsForRepoOwner(fork, null)), + githubRepoIdentityKey({ owner: 'me', repo: 'r2r-mirror' }) + ) + + expect(lookupReposBySlugFromCache([origin, fork], null, 'SciPhi-AI/R2R')).toEqual([origin]) + }) + + it('does not route a GHES row to a same-named github.com fork parent', () => { + const fork = { ...repo('fork'), upstream: { owner: 'acme', repo: 'widgets' } } + slugByRepoId.set( + slugCacheKey(fork.id, settingsForRepoOwner(fork, null)), + githubRepoIdentityKey({ owner: 'me', repo: 'widgets' }) + ) + + expect(lookupReposBySlugFromCache([fork], null, 'acme/widgets', 'ghe.example:8443')).toEqual([]) + }) + + it('scopes a host-less fork parent to the host the fork itself was cloned from', () => { + const enterpriseFork = { ...repo('fork'), upstream: { owner: 'acme', repo: 'widgets' } } + slugByRepoId.set( + slugCacheKey(enterpriseFork.id, settingsForRepoOwner(enterpriseFork, null)), + githubRepoIdentityKey({ owner: 'me', repo: 'widgets', host: 'ghe.example:8443' }) + ) + + expect( + lookupReposBySlugFromCache([enterpriseFork], null, 'acme/widgets', 'ghe.example:8443') + ).toEqual([enterpriseFork]) + expect(lookupReposBySlugFromCache([enterpriseFork], null, 'acme/widgets')).toEqual([]) + }) + + it('drops the fork alias while its own origin is unresolved', () => { + const fork = { ...repo('fork'), upstream: { owner: 'acme', repo: 'widgets' } } + + expect(lookupReposBySlugFromCache([fork], null, 'acme/widgets')).toEqual([]) + slugByRepoId.set(slugCacheKey(fork.id, settingsForRepoOwner(fork, null)), null) + expect(lookupReposBySlugFromCache([fork], null, 'acme/widgets')).toEqual([]) + }) + it('expires negative slug resolutions so an external GHES login can recover', () => { const key = slugCacheKey('enterprise', null) rememberRepoSlug(key, null, 1_000) diff --git a/src/renderer/src/lib/repo-slug-cache.ts b/src/renderer/src/lib/repo-slug-cache.ts index 38ea5d32995..cb6552aa572 100644 --- a/src/renderer/src/lib/repo-slug-cache.ts +++ b/src/renderer/src/lib/repo-slug-cache.ts @@ -4,11 +4,40 @@ import type { GlobalSettings, Repo } from '../../../shared/types' import { getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' import { getSettingsForRepoRuntimeOwner } from './repo-runtime-owner' -import { githubRepoIdentityKey } from '../../../shared/github-repository-identity-key' +import { + githubHostFromIdentityKey, + githubRepoIdentityKey +} from '../../../shared/github-repository-identity-key' /** Lowercased `owner/repo` → Repo[]. */ export type SlugIndex = Map +/** The two ways a slug can match a repo, kept apart so callers that also filter + * by repo selection can fall through to `upstream` instead of letting an + * unselected clone of the upstream repo shadow the selected fork. */ +export type RepoSlugMatches = { origin: Repo[]; upstream: Repo[] } + +/** Identity key of a fork's upstream parent — the second identity a Project row + * may legitimately match, since a contributor's clone has the personal fork as + * `origin`. `null` when the repo is not a fork or the key cannot be trusted. + * + * Why `originIdentityKey` is required: when `upstream.host` is absent (older + * persisted forks), the fork's origin host is the fallback so GHES parents do + * not collapse into github.com. Unresolved origins refuse the alias. */ +export function repoUpstreamIdentityKey( + repo: Repo, + originIdentityKey: string | null | undefined +): string | null { + const upstream = repo.upstream + if (!upstream?.owner || !upstream.repo || !originIdentityKey) { + return null + } + return githubRepoIdentityKey({ + ...upstream, + host: upstream.host ?? githubHostFromIdentityKey(originIdentityKey) + }) +} + /** Module-scope cache keyed by runtime scope + repo.id. A Repo that has already * failed resolution is recorded as `null` briefly so it is not retried on every * cell mount, while still recovering after an external GHES auth login. */ @@ -87,7 +116,8 @@ export function settingsForRepoOwner( /** Synchronous slug → Repo lookup against the already-resolved module cache. * Used by store slices (which can't run the async hook-based index) to route * project-row mutations to the matched repo's owner host; callers fall back to - * focused settings when nothing matches. */ + * focused settings when nothing matches. Origin matches win over upstream ones + * so a clone of the upstream repo itself is never shadowed by someone's fork. */ export function lookupReposBySlugFromCache( repos: readonly Repo[], settings: Pick | null | undefined, @@ -100,11 +130,15 @@ export function lookupReposBySlugFromCache( } const target = githubRepoIdentityKey({ owner, repo, host }) const matched: Repo[] = [] + const upstreamMatched: Repo[] = [] for (const repo of repos) { const cacheKey = slugCacheKey(repo.id, settingsForRepoOwner(repo, settings)) - if (slugByRepoId.get(cacheKey) === target) { + const originKey = slugByRepoId.get(cacheKey) + if (originKey === target) { matched.push(repo) + } else if (repoUpstreamIdentityKey(repo, originKey) === target) { + upstreamMatched.push(repo) } } - return matched + return matched.length > 0 ? matched : upstreamMatched } diff --git a/src/renderer/src/lib/repo-slug-index.test.ts b/src/renderer/src/lib/repo-slug-index.test.ts new file mode 100644 index 00000000000..0b710cf5438 --- /dev/null +++ b/src/renderer/src/lib/repo-slug-index.test.ts @@ -0,0 +1,189 @@ +// @vitest-environment happy-dom + +import { act, cleanup, renderHook, waitFor } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../shared/types' +import { useAppStore } from '@/store' +import { useRepoSlugIndex } from './repo-slug-index' +import { REPO_SLUG_FAILURE_TTL_MS, clearRepoSlugCacheValues } from './repo-slug-cache' + +const repoSlug = vi.fn() + +function makeRepo(overrides: Partial & { id: string }): Repo { + return { + path: `/repos/${overrides.id}`, + displayName: overrides.id, + badgeColor: '#000', + addedAt: 1, + executionHostId: 'local', + ...overrides + } +} + +const initialState = useAppStore.getInitialState() + +function setRepos(repos: Repo[]): void { + useAppStore.setState({ ...initialState, repos, settings: initialState.settings }, true) +} + +beforeEach(() => { + clearRepoSlugCacheValues() + repoSlug.mockReset() + // Why: origin resolution goes through the preload bridge; the index under + // test only reads `repo.upstream` from the store for the fork alias. + Object.assign(window, { api: { gh: { repoSlug } } }) +}) + +afterEach(() => { + // Why: React schedules work outside the test tick, so a hook left mounted + // flushes after the DOM environment is torn down and throws "window is not + // defined" as an unhandled error. + cleanup() + setRepos([]) +}) + +async function lookup(slug: string): Promise { + const { result } = renderHook(() => useRepoSlugIndex()) + await waitFor(() => expect(result.current.ready).toBe(true)) + return [...result.current.lookupSlug(slug)] +} + +describe('useRepoSlugIndex fork upstream matching', () => { + it('matches a project row against a fork clone via its upstream parent', async () => { + const fork = makeRepo({ id: 'fork', upstream: { owner: 'SciPhi-AI', repo: 'R2R' } }) + setRepos([fork]) + repoSlug.mockResolvedValue({ owner: 'me', repo: 'r2r-mirror' }) + + expect(await lookup('SciPhi-AI/R2R')).toEqual([fork]) + }) + + it('still matches the fork by its own origin slug', async () => { + const fork = makeRepo({ id: 'fork', upstream: { owner: 'SciPhi-AI', repo: 'R2R' } }) + setRepos([fork]) + repoSlug.mockResolvedValue({ owner: 'me', repo: 'r2r-mirror' }) + + expect(await lookup('me/r2r-mirror')).toEqual([fork]) + }) + + it('prefers the clone that owns the slug over a fork of it', async () => { + const upstreamClone = makeRepo({ id: 'upstream', upstream: null }) + const fork = makeRepo({ id: 'fork', upstream: { owner: 'SciPhi-AI', repo: 'R2R' } }) + setRepos([upstreamClone, fork]) + repoSlug.mockImplementation(async (args: { repoId?: string }) => + args.repoId === 'upstream' + ? { owner: 'SciPhi-AI', repo: 'R2R' } + : { owner: 'me', repo: 'r2r-mirror' } + ) + + expect(await lookup('SciPhi-AI/R2R')).toEqual([upstreamClone]) + }) + + it('does not bind a github.com fork parent to a same-named Enterprise row', async () => { + const fork = makeRepo({ id: 'fork', upstream: { owner: 'SciPhi-AI', repo: 'R2R' } }) + setRepos([fork]) + repoSlug.mockResolvedValue({ owner: 'me', repo: 'r2r-mirror' }) + + const { result } = renderHook(() => useRepoSlugIndex()) + await waitFor(() => expect(result.current.ready).toBe(true)) + expect(result.current.lookupSlug('SciPhi-AI/R2R', 'ghe.example')).toEqual([]) + }) + + it('resolves the slug index without extra upstream IPC', async () => { + const fork = makeRepo({ id: 'fork', upstream: { owner: 'SciPhi-AI', repo: 'R2R' } }) + setRepos([fork]) + repoSlug.mockResolvedValue({ owner: 'me', repo: 'r2r-mirror' }) + + expect(await lookup('SciPhi-AI/R2R')).toEqual([fork]) + expect(repoSlug).toHaveBeenCalledTimes(1) + }) + + it('scopes a host-less fork parent to the host the fork itself was cloned from', async () => { + const fork = makeRepo({ id: 'fork', upstream: { owner: 'acme', repo: 'widgets' } }) + setRepos([fork]) + repoSlug.mockResolvedValue({ owner: 'me', repo: 'widgets', host: 'ghe.example' }) + + const { result } = renderHook(() => useRepoSlugIndex()) + await waitFor(() => expect(result.current.ready).toBe(true)) + expect(result.current.lookupSlug('acme/widgets', 'ghe.example')).toEqual([fork]) + expect(result.current.lookupSlug('acme/widgets')).toEqual([]) + }) + + it('drops the fork alias while its own origin is unresolved', async () => { + const fork = makeRepo({ id: 'fork', upstream: { owner: 'acme', repo: 'widgets' } }) + setRepos([fork]) + repoSlug.mockResolvedValue(null) + + expect(await lookup('acme/widgets')).toEqual([]) + }) + + it('lists a repo once when its upstream is its own origin', async () => { + const selfAliased = makeRepo({ id: 'self', upstream: { owner: 'acme', repo: 'widgets' } }) + setRepos([selfAliased]) + repoSlug.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + + const { result } = renderHook(() => useRepoSlugIndex()) + await waitFor(() => expect(result.current.ready).toBe(true)) + expect(result.current.lookupSlugMatches('acme/widgets')).toEqual({ + origin: [selfAliased], + upstream: [] + }) + }) + + it('reports origin and upstream matches separately for selection filtering', async () => { + const upstreamClone = makeRepo({ id: 'upstream', upstream: null }) + const fork = makeRepo({ id: 'fork', upstream: { owner: 'SciPhi-AI', repo: 'R2R' } }) + setRepos([upstreamClone, fork]) + repoSlug.mockImplementation(async (args: { repoId?: string }) => + args.repoId === 'upstream' + ? { owner: 'SciPhi-AI', repo: 'R2R' } + : { owner: 'me', repo: 'r2r-mirror' } + ) + + const { result } = renderHook(() => useRepoSlugIndex()) + await waitFor(() => expect(result.current.ready).toBe(true)) + expect(result.current.lookupSlugMatches('SciPhi-AI/R2R')).toEqual({ + origin: [upstreamClone], + upstream: [fork] + }) + }) +}) + +describe('useRepoSlugIndex failure retry', () => { + afterEach(() => vi.useRealTimers()) + + it('re-resolves a failed slug after the failure TTL and stops once unmounted', async () => { + vi.useFakeTimers() + const repo = makeRepo({ id: 'flaky' }) + setRepos([repo]) + // Why: a null result is the negative-cached "not a GitHub repo" answer that + // arms the bounded retry. + repoSlug.mockResolvedValueOnce(null) + + const { result } = renderHook(() => useRepoSlugIndex()) + await act(async () => void (await vi.advanceTimersByTimeAsync(0))) + expect(result.current.ready).toBe(true) + expect(result.current.lookupSlug('acme/widgets')).toEqual([]) + expect(repoSlug).toHaveBeenCalledTimes(1) + + repoSlug.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + await act(async () => void (await vi.advanceTimersByTimeAsync(REPO_SLUG_FAILURE_TTL_MS + 10))) + expect(repoSlug.mock.calls.length).toBeGreaterThan(1) + expect(result.current.lookupSlug('acme/widgets')).toEqual([repo]) + }) + + it('clears the pending retry timer on unmount', async () => { + vi.useFakeTimers() + setRepos([makeRepo({ id: 'flaky' })]) + // Why: a permanently failing resolution keeps a retry armed, so an + // uncleaned timer is observable after teardown. The hook mounts once per + // project row, so such a timer would leak per row. + repoSlug.mockResolvedValue(null) + + const { unmount } = renderHook(() => useRepoSlugIndex()) + await act(async () => void (await vi.advanceTimersByTimeAsync(0))) + const armedCount = vi.getTimerCount() + + unmount() + expect(vi.getTimerCount()).toBeLessThan(armedCount) + }) +}) diff --git a/src/renderer/src/lib/repo-slug-index.ts b/src/renderer/src/lib/repo-slug-index.ts index 1d5f0a7f172..d48dbb869f3 100644 --- a/src/renderer/src/lib/repo-slug-index.ts +++ b/src/renderer/src/lib/repo-slug-index.ts @@ -22,9 +22,11 @@ import { nextRepoSlugFailureRetryDelay, readRepoSlugCache, rememberRepoSlug, + repoUpstreamIdentityKey, settingsForRepoOwner, slugByRepoId, slugCacheKey, + type RepoSlugMatches, type SlugIndex } from './repo-slug-cache' import { githubRepoIdentityKey } from '../../../shared/github-repository-identity-key' @@ -126,7 +128,7 @@ async function resolveRepoSlug( async function buildIndex( repos: Repo[], settings: Pick | null | undefined -): Promise<{ index: SlugIndex; retryDelayMs: number | null }> { +): Promise<{ index: SlugIndex; upstreamIndex: SlugIndex; retryDelayMs: number | null }> { // Why: evict cached entries for repos that no longer exist in state so // the cache cannot grow unbounded across long sessions where users add // and remove repos. Without this, every removed repo's id (and its @@ -139,6 +141,7 @@ async function buildIndex( } } const next: SlugIndex = new Map() + const upstreamNext: SlugIndex = new Map() const results = await Promise.all( repos.map(async (r) => ({ repo: r, @@ -151,12 +154,26 @@ async function buildIndex( if (slug) { next.set(slug, [...(next.get(slug) ?? []), repo]) } + // Why: a Project card references the upstream repo, but a contributor's + // clone has their personal fork as `origin`, so the origin-only index + // dropped every row (#12647). `repo.upstream` is already resolved when the + // repo is added, so this costs no extra IPC. + const upstreamKey = repoUpstreamIdentityKey(repo, slug) + if (upstreamKey && upstreamKey !== slug) { + upstreamNext.set(upstreamKey, [...(upstreamNext.get(upstreamKey) ?? []), repo]) + } + } + return { + index: next, + upstreamIndex: upstreamNext, + retryDelayMs: nextRepoSlugFailureRetryDelay(liveKeys) } - return { index: next, retryDelayMs: nextRepoSlugFailureRetryDelay(liveKeys) } } export type RepoSlugIndexState = { + /** Best available matches: origin when anything owns the slug, else forks. */ lookupSlug: (slug: string | null | undefined, host?: string) => Repo[] + lookupSlugMatches: (slug: string | null | undefined, host?: string) => RepoSlugMatches ready: boolean } @@ -167,45 +184,67 @@ export function useRepoSlugIndex(): RepoSlugIndexState { const repos = useAppStore((s) => s.repos) const settings = useAppStore((s) => s.settings) const [index, setIndex] = useState(() => new Map()) + const [upstreamIndex, setUpstreamIndex] = useState(() => new Map()) const [ready, setReady] = useState(false) const [retryGeneration, setRetryGeneration] = useState(0) + // Why: schedule retry in a dedicated effect so setTimeout cleanup is owned + // synchronously (react-doctor effect-needs-cleanup); async .then assignment + // was not statically owned by the buildIndex effect cleanup. + const [retryDelayMs, setRetryDelayMs] = useState(null) // Why: track the current repos snapshot so the effect can ignore stale // resolutions when repos change mid-flight. const generationRef = useRef(0) useEffect(() => { const gen = ++generationRef.current - let retryTimer: ReturnType | undefined setReady(false) - void buildIndex(repos, settings).then(({ index: next, retryDelayMs }) => { - if (gen !== generationRef.current) { - return + setRetryDelayMs(null) + void buildIndex(repos, settings).then( + ({ index: next, upstreamIndex: nextUpstream, retryDelayMs: nextRetryDelayMs }) => { + if (gen !== generationRef.current) { + return + } + setIndex(next) + setUpstreamIndex(nextUpstream) + setReady(true) + setRetryDelayMs(nextRetryDelayMs) } - setIndex(next) - setReady(true) - if (retryDelayMs !== null) { - retryTimer = setTimeout(() => setRetryGeneration((value) => value + 1), retryDelayMs) - } - }) + ) return () => { generationRef.current += 1 - if (retryTimer) { - clearTimeout(retryTimer) - } } }, [repos, retryGeneration, settings]) - return useMemo( - () => ({ + useEffect(() => { + if (retryDelayMs === null) { + return + } + const retryTimer = setTimeout(() => setRetryGeneration((value) => value + 1), retryDelayMs) + return () => { + clearTimeout(retryTimer) + } + }, [retryDelayMs]) + + return useMemo(() => { + const lookupSlugMatches = (slug: string | null | undefined, host?: string): RepoSlugMatches => { + const [owner, repo] = slug?.split('/') ?? [] + if (!owner || !repo) { + return { origin: [], upstream: [] } + } + const key = githubRepoIdentityKey({ owner, repo, host }) + return { origin: index.get(key) ?? [], upstream: upstreamIndex.get(key) ?? [] } + } + return { + lookupSlugMatches, + // Why: origin wins — when the upstream repo itself is open, a row must + // resolve to that clone rather than becoming ambiguous with someone's + // fork of it. Callers that also filter by selection use + // `lookupSlugMatches` so an unselected clone cannot hide a selected fork. lookupSlug: (slug: string | null | undefined, host?: string): Repo[] => { - const [owner, repo] = slug?.split('/') ?? [] - if (!owner || !repo) { - return [] - } - return index.get(githubRepoIdentityKey({ owner, repo, host })) ?? [] + const { origin, upstream } = lookupSlugMatches(slug, host) + return origin.length > 0 ? origin : upstream }, ready - }), - [index, ready] - ) + } + }, [index, upstreamIndex, ready]) } diff --git a/src/shared/github-repository-identity-key.ts b/src/shared/github-repository-identity-key.ts index df9b8152982..ac42fcd8ad6 100644 --- a/src/shared/github-repository-identity-key.ts +++ b/src/shared/github-repository-identity-key.ts @@ -17,3 +17,12 @@ export function githubRepoIdentityKey(repo: { const host = repo.host?.trim().toLowerCase() return host && !isDefaultGitHubHost(host) ? `${host}/${slug}` : slug } + +// Why: callers that only kept the key (not the identity it came from) still need +// its host segment to scope a second, host-less identity into the same namespace. +// `owner` and `repo` never contain `/`, so a three-segment key is host-qualified. +// `undefined` means github.com, so never pass a key that may be unresolved. +export function githubHostFromIdentityKey(key: string | null | undefined): string | undefined { + const segments = key?.split('/') ?? [] + return segments.length === 3 ? segments[0] : undefined +}