diff --git a/config/scripts/github-opened-issue-repository.test.ts b/config/scripts/github-opened-issue-repository.test.ts new file mode 100644 index 00000000000..b2d0519c5cc --- /dev/null +++ b/config/scripts/github-opened-issue-repository.test.ts @@ -0,0 +1,440 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { createElement } from 'react' +import { renderToStaticMarkup } from 'react-dom/server' +import type * as ReactModule from 'react' +import type * as GhUtils from '../../src/main/github/gh-utils' +import type * as IssueMetadata from '../../src/renderer/src/hooks/useIssueMetadata' +import type { GitHubWorkItem } from '../../src/shared/github/work-item-types' +import type { GitHubOwnerRepo } from '../../src/shared/github/pull-request-types' +import type { TaskSourceContext } from '../../src/shared/task-source-context' +import type { Repo } from '../../src/shared/repo-types' + +const fixture = vi.hoisted(() => { + const state: { + loads: { key: string | null; load: () => Promise }[] + requests: { args: string[]; host?: string }[] + gh: ReturnType + apiUpdate: ReturnType + preference: 'origin' | 'upstream' + } = { loads: [], requests: [], gh: vi.fn(), apiUpdate: vi.fn(), preference: 'upstream' } + return state +}) + +vi.mock('react', async (original) => ({ + ...(await original()), + useState: (initial: unknown) => [typeof initial === 'function' ? initial() : initial, vi.fn()], + useMemo: (value: () => T) => value(), + useCallback: (value: T) => value, + useRef: (initial: T) => ({ current: initial }), + useEffect: vi.fn() +})) +vi.mock('zustand/react/shallow', () => ({ useShallow: (value: T) => value })) +vi.mock('@/store', () => ({ + useAppStore: Object.assign( + (selector: (state: unknown) => unknown) => + selector({ patchWorkItem: vi.fn(), patchProjectRowContent: vi.fn() }), + { getState: () => ({ recordFeatureInteraction: vi.fn() }) } + ) +})) +vi.mock('@/lib/repo-runtime-owner', () => ({ + getSettingsForRepoRuntimeOwner: () => ({ activeRuntimeEnvironmentId: null }) +})) +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) +vi.mock('@/components/github/github-duplicate-issue-candidates', () => ({ + useGitHubDuplicateIssueCandidates: () => [] +})) +vi.mock('@/components/github/github-work-item-comment-mutations', () => ({ + notifyWorkItemDetailsMutation: vi.fn() +})) +vi.mock('@/hooks/useIssueMetadata', async (original) => ({ + ...(await original()), + useImmediateMutation: () => ({ isPending: () => false, run: vi.fn() }) +})) +vi.mock('@/hooks/useMetadataListRequest', () => ({ + useMetadataListRequest: (args: { cacheKey: string | null; load: () => Promise }) => { + fixture.loads.push({ key: args.cacheKey, load: args.load }) + return { data: [], loading: false, error: null } + } +})) +vi.mock('../../src/main/github/gh-utils', async (original) => ({ + ...(await original()), + ghExecFileAsync: fixture.gh, + acquire: vi.fn(), + release: vi.fn(), + getOwnerRepoForRemote: async (_path: string, remote: string) => ({ + owner: remote === 'upstream' ? 'upstream-owner' : 'fork-owner', + repo: 'widgets' + }) +})) +vi.mock('../../src/main/git/remote-name-listing', () => ({ + shouldProbeGitRemote: async () => true +})) + +import { GHEditSection } from '../../src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section' +import { + runGHEditLabelToggle, + runGHEditStateChange +} from '../../src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section-mutations' +import { findTaskPageDialogWorkItem } from '../../src/renderer/src/components/task-page-cache-selectors' +import { getTaskPageRepoSourceContext } from '../../src/renderer/src/components/task-page-source-context' +import { workItemsCacheKey } from '../../src/renderer/src/store/github/cache-identity' +import { createTestStore } from '../../src/renderer/src/store/slices/github-slice-test-harness' +import { getTaskSourceCacheScope } from '../../src/shared/task-source-context' +import { listLabels, listAssignableUsers } from '../../src/main/github/issue-field-options' +import { useRepoLabels, useRepoAssignees } from '../../src/renderer/src/hooks/useIssueMetadata' +import { updateIssue } from '../../src/main/github/issue-update' +import { materializeTaskPageItemList } from '../../src/renderer/src/components/task-page-github-work-item-mutations' +import { + resetTaskPageGitHubMutationRegistryForTests, + setTaskPageGitHubMutationQueryKey +} from '../../src/renderer/src/components/task-page-github-work-item-mutation-registry' + +function renderEditSection(props: Parameters[0]): void { + renderToStaticMarkup(createElement(GHEditSection, props)) +} + +const registeredRepo: Repo = { + id: 'repo-1', + path: join(tmpdir(), 'orca-opened-issue-repository-fixture'), + displayName: 'widgets', + badgeColor: 'primary', + addedAt: 1, + upstream: { owner: 'upstream-owner', repo: 'widgets', host: 'github.com' } +} + +function sourceFor(preference: 'origin' | 'upstream'): TaskSourceContext { + const source = getTaskPageRepoSourceContext( + { ...registeredRepo, issueSourcePreference: preference }, + 'github' + ) + if (!source) { + throw new Error('Registered fixture must produce a source context') + } + return source +} + +const sourceContext = sourceFor('origin') +const fork: GitHubWorkItem = { + id: 'issue:5', + type: 'issue', + number: 5, + title: 'FORK title', + state: 'open', + url: 'https://github.com/fork-owner/widgets/issues/5', + labels: [], + updatedAt: '', + author: null, + repoId: 'repo-1' +} +const upstream: GitHubWorkItem = { + ...fork, + title: 'UPSTREAM title', + url: 'https://github.com/upstream-owner/widgets/issues/5' +} +const issueRepo = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } + +type MetadataArgs = { repoPath: string; ownerRepo?: GitHubOwnerRepo } + +beforeEach(() => { + fixture.preference = 'upstream' + fixture.loads = [] + fixture.requests = [] + resetTaskPageGitHubMutationRegistryForTests() + setTaskPageGitHubMutationQueryKey('current-upstream-list') + fixture.gh.mockReset() + fixture.gh.mockImplementation(async (args: string[], options: { host?: string }) => { + fixture.requests.push({ args, host: options.host }) + return { stdout: '', stderr: '' } + }) + fixture.apiUpdate = vi.fn((args: Parameters[0]) => + updateIssue( + args.repoPath, + args.number, + args.updates, + null, + {}, + fixture.preference, + args.ownerRepo + ) + ) + vi.stubGlobal('window', { + api: { + gh: { + updateIssue: fixture.apiUpdate, + listLabels: (args: MetadataArgs) => + listLabels(args.repoPath, fixture.preference, null, {}, args.ownerRepo), + listAssignableUsers: (args: MetadataArgs) => + listAssignableUsers(args.repoPath, fixture.preference, null, {}, args.ownerRepo) + } + } + }) +}) + +afterEach(() => { + resetTaskPageGitHubMutationRegistryForTests() + vi.unstubAllGlobals() +}) + +it.each([ + { openedItem: fork, listItem: upstream, preference: 'upstream' }, + { openedItem: upstream, listItem: fork, preference: 'origin' }, + { openedItem: fork, listItem: fork, preference: 'origin' } +] as const)( + 'scopes $openedItem.title labels under $preference to its canonical list row', + async ({ openedItem, listItem, preference }) => { + fixture.preference = preference + expect(sourceFor('upstream')).toEqual(sourceContext) + const store = createTestStore() + const key = workItemsCacheKey( + registeredRepo.id, + 36, + '', + getTaskSourceCacheScope(sourceFor('upstream')) + ) + store.setState({ + workItemsCache: { [key]: { data: [listItem], fetchedAt: Date.now() } } + }) + const opened = + findTaskPageDialogWorkItem(store.getState().workItemsCache, { + id: openedItem.id, + repoId: openedItem.repoId, + url: openedItem.url + }) ?? openedItem + expect(opened.url).toBe(openedItem.url) + const target = { ...issueRepo, owner: openedItem === fork ? 'fork-owner' : 'upstream-owner' } + let mutation: Promise = Promise.resolve() + runGHEditLabelToggle({ + itemId: opened.id, + itemNumber: opened.number, + itemRepoId: opened.repoId, + repoPath: registeredRepo.path, + sourceContext, + projectOrigin: undefined, + issueRepo: target, + label: 'fork-only-label', + localLabels: [], + run: async (_key, options) => { + options.onOptimistic?.() + mutation = options.mutate() + await mutation + options.onSuccess?.() + return true + }, + onLabelsChange: vi.fn(), + patchWorkItem: store.getState().patchWorkItem, + patchProjectRowIfNeeded: vi.fn(), + onMutated: vi.fn() + }) + await mutation + expect(fixture.requests[0].args).toContain(`${target.owner}/widgets`) + expect(fixture.apiUpdate).toHaveBeenCalledWith(expect.objectContaining({ ownerRepo: target })) + expect(store.getState().workItemsCache[key]?.data?.[0].labels).toEqual( + listItem.url === openedItem.url ? ['fork-only-label'] : [] + ) + } +) + +it.each([ + { openedItem: fork, owner: 'fork-owner', preference: 'origin' }, + { openedItem: fork, owner: 'fork-owner', preference: 'upstream' }, + { openedItem: upstream, owner: 'upstream-owner', preference: 'origin' }, + { openedItem: upstream, owner: 'upstream-owner', preference: 'upstream' } +] as const)( + 'loads $owner picker candidates while preference=$preference', + async ({ preference, openedItem, owner }) => { + fixture.preference = preference + renderEditSection({ + item: openedItem, + repoPath: registeredRepo.path, + repoId: fork.repoId, + sourceContext, + projectOrigin: undefined, + localState: 'open', + localLabels: [], + assignees: [], + onStateChange: vi.fn(), + onLabelsChange: vi.fn(), + onMutated: vi.fn(), + onUse: vi.fn() + }) + for (const request of fixture.loads.filter((load) => load.key !== null)) { + await request.load() + } + expect( + fixture.requests.map((request) => request.args.find((arg) => arg.startsWith('repos/'))) + ).toEqual([`repos/${owner}/widgets/labels`, `repos/${owner}/widgets/assignees?per_page=100`]) + } +) + +it.each([ + { openedItem: fork, listItem: fork }, + { openedItem: fork, listItem: upstream }, + { openedItem: upstream, listItem: fork }, + { openedItem: upstream, listItem: upstream } +])( + 'a $openedItem.title close only controls its own row while search lags (list=$listItem.title)', + async ({ openedItem, listItem }) => { + const target = { ...issueRepo, owner: openedItem === fork ? 'fork-owner' : 'upstream-owner' } + let pending = Promise.resolve() + runGHEditStateChange({ + newState: 'closed', + localState: 'open', + itemId: fork.id, + itemNumber: fork.number, + itemRepoId: fork.repoId, + repoPath: registeredRepo.path, + sourceContext, + projectOrigin: undefined, + issueRepo: target, + run: (_key, options) => { + pending = (async () => { + options.onOptimistic?.() + await options.mutate() + options.onSuccess?.() + })() + return pending + }, + onStateChange: vi.fn(), + patchWorkItem: vi.fn(), + patchProjectRowIfNeeded: vi.fn(), + onMutated: vi.fn() + }) + await pending + expect(fixture.apiUpdate).toHaveBeenCalledWith(expect.objectContaining({ ownerRepo: target })) + const displayed = materializeTaskPageItemList({ + networkItems: [listItem], + previousItems: [listItem], + queryKey: 'current-upstream-list' + }) + expect(displayed[0]?.state).toBe(listItem.url === openedItem.url ? 'closed' : 'open') + } +) + +it('keeps ordinary metadata caches distinct by canonical repository and host', () => { + const identities = [ + issueRepo, + { ...issueRepo, owner: 'upstream-owner' }, + { ...issueRepo, host: 'ghe.example:8443' } + ] + for (const ownerRepo of identities) { + useRepoLabels(registeredRepo.path, registeredRepo.id, { ownerRepo }) + useRepoAssignees(registeredRepo.path, registeredRepo.id, { ownerRepo }) + } + expect(new Set(fixture.loads.filter((_, i) => i % 2 === 0).map((load) => load.key)).size).toBe(3) + expect(new Set(fixture.loads.filter((_, i) => i % 2 === 1).map((load) => load.key)).size).toBe(3) +}) + +it('keeps metadata requests without an explicit target compatible', async () => { + useRepoLabels(registeredRepo.path, registeredRepo.id) + useRepoAssignees(registeredRepo.path, registeredRepo.id) + for (const request of fixture.loads) { + await request.load() + } + expect(fixture.loads.map((load) => load.key)).toEqual([registeredRepo.id, registeredRepo.id]) + expect( + fixture.requests.map((request) => request.args.find((arg) => arg.startsWith('repos/'))) + ).toEqual([ + 'repos/upstream-owner/widgets/labels', + 'repos/upstream-owner/widgets/assignees?per_page=100' + ]) +}) + +it('keeps Project row metadata on the existing slug route', async () => { + const labels = vi.fn().mockResolvedValue({ ok: true, labels: [] }) + const users = vi.fn().mockResolvedValue({ ok: true, users: [] }) + vi.stubGlobal('window', { + api: { gh: { listLabelsBySlug: labels, listAssignableUsersBySlug: users } } + }) + renderEditSection({ + item: fork, + repoPath: registeredRepo.path, + repoId: fork.repoId, + sourceContext, + projectOrigin: { + owner: 'project-owner', + repo: 'outside', + host: 'ghe.example', + number: fork.number, + type: 'issue', + projectId: 'project-1', + projectItemId: 'row-1', + cacheKey: 'project-key' + }, + localState: 'open', + localLabels: [], + assignees: [], + onStateChange: vi.fn(), + onLabelsChange: vi.fn(), + onMutated: vi.fn(), + onUse: vi.fn() + }) + for (const request of fixture.loads.filter((load) => load.key !== null)) { + await request.load() + } + expect(labels).toHaveBeenCalledWith({ + owner: 'project-owner', + repo: 'outside', + host: 'ghe.example' + }) + expect(users).toHaveBeenCalledWith({ + owner: 'project-owner', + repo: 'outside', + host: 'ghe.example' + }) + expect(fixture.requests).toEqual([]) +}) + +it('rolls back a rejected fork label without changing the upstream row', async () => { + const store = createTestStore() + const key = workItemsCacheKey(registeredRepo.id, 36, '', getTaskSourceCacheScope(sourceContext)) + store.setState({ workItemsCache: { [key]: { data: [upstream, fork], fetchedAt: 1 } } }) + fixture.gh.mockRejectedValueOnce(new Error('Fixture rejects label')) + let pending = Promise.resolve(false) + const observed: string[][][] = [] + runGHEditLabelToggle({ + itemId: fork.id, + itemNumber: fork.number, + itemRepoId: fork.repoId, + repoPath: registeredRepo.path, + sourceContext, + projectOrigin: undefined, + issueRepo, + label: 'fork-only-label', + localLabels: [], + run: (_key, options) => { + pending = (async () => { + options.onOptimistic?.() + observed.push(store.getState().workItemsCache[key]?.data?.map((row) => row.labels) ?? []) + try { + await options.mutate() + return true + } catch { + options.onRevert?.() + observed.push(store.getState().workItemsCache[key]?.data?.map((row) => row.labels) ?? []) + return false + } + })() + return pending + }, + onLabelsChange: vi.fn(), + patchWorkItem: store.getState().patchWorkItem, + patchProjectRowIfNeeded: vi.fn(), + onMutated: vi.fn() + }) + expect(await pending).toBe(false) + expect(observed).toEqual([ + [[], ['fork-only-label']], + [[], []] + ]) +}) + +it('does not fall back to upstream metadata for an invalid explicit repository', async () => { + const invalid = { owner: '../escape', repo: 'widgets', host: 'github.com' } + await expect(listLabels(registeredRepo.path, 'upstream', null, {}, invalid)).resolves.toEqual([]) + await expect( + listAssignableUsers(registeredRepo.path, 'upstream', null, {}, invalid) + ).resolves.toEqual([]) + expect(fixture.requests).toEqual([]) +}) diff --git a/config/scripts/github-pr-assignee-repository.test.ts b/config/scripts/github-pr-assignee-repository.test.ts new file mode 100644 index 00000000000..0265aa92c41 --- /dev/null +++ b/config/scripts/github-pr-assignee-repository.test.ts @@ -0,0 +1,204 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import type * as GhUtils from '../../src/main/github/gh-utils' +import type * as ReactModule from 'react' +import type { GitHubOwnerRepo } from '../../src/shared/github/pull-request-types' +import type { GitHubWorkItem } from '../../src/shared/github/work-item-types' +import type { GitHubIssueUpdate } from '../../src/shared/issue-mutation-types' + +const fixture = vi.hoisted(() => { + const state: { + callbacks: unknown[] + mutation: Promise | null + gh: ReturnType + apiUpdate: ReturnType + patch: ReturnType + preference: 'origin' | 'upstream' + localGitOptions: { wslDistro?: string } + requests: { args: string[]; host?: string; cwd?: string; wslDistro?: string }[] + } = { + callbacks: [], + mutation: null, + gh: vi.fn(), + apiUpdate: vi.fn(), + patch: vi.fn(), + preference: 'origin', + localGitOptions: {}, + requests: [] + } + return state +}) + +vi.mock('react', async (original) => ({ + ...(await original()), + useState: (value: unknown) => [typeof value === 'function' ? value() : value, vi.fn()], + useMemo: (getValue: () => T) => getValue(), + useCallback: (callback: T) => { + fixture.callbacks.push(callback) + return callback + } +})) +vi.mock('zustand/react/shallow', () => ({ useShallow: (selector: T) => selector })) +vi.mock('@/store', () => ({ + useAppStore: Object.assign( + (selector: (state: unknown) => unknown) => + selector({ + patchWorkItem: fixture.patch, + patchProjectRowContent: fixture.patch, + repos: [], + settings: {} + }), + { getState: () => ({ recordFeatureInteraction: vi.fn() }) } + ) +})) +vi.mock('@/lib/repo-runtime-owner', () => ({ + getSettingsForRepoRuntimeOwner: () => ({ activeRuntimeEnvironmentId: null }) +})) +vi.mock('@/components/ui/popover', () => ({ + Popover: vi.fn(), + PopoverContent: vi.fn(), + PopoverTrigger: vi.fn() +})) +vi.mock('@/hooks/useIssueMetadata', () => ({ + useRepoAssignees: () => ({ data: [], loading: false, error: null }), + useImmediateMutation: () => ({ + isPending: () => false, + run: (_key: string, spec: { mutate: () => Promise }) => { + fixture.mutation = spec.mutate() + } + }) +})) +vi.mock('@/hooks/useGitHubSlugMetadata', () => ({ + useRepoAssigneesBySlug: () => ({ + data: [{ login: 'octo', name: null, avatarUrl: '' }], + loading: false, + error: null + }) +})) +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) +vi.mock('@/components/github/work-item-state-presentation', () => ({ ReviewerAvatar: vi.fn() })) +vi.mock('../../src/main/github/gh-utils', async (original) => ({ + ...(await original()), + ghExecFileAsync: fixture.gh, + acquire: vi.fn(), + release: vi.fn(), + getOwnerRepoForRemote: async (_path: string, remote: string) => ({ + owner: remote === 'upstream' ? 'upstream-owner' : 'fork-owner', + repo: 'widgets' + }) +})) +vi.mock('../../src/main/git/remote-name-listing', () => ({ + shouldProbeGitRemote: async () => true +})) + +import { PRAssigneesPanel } from '../../src/renderer/src/components/github/PRAssigneesPanel' +import { updateIssue } from '../../src/main/github/issue-update' +import { _resetOriginGitHubApiRepositoryCache } from '../../src/main/github/github-api-repository' + +const repoPath = join(tmpdir(), 'orca-pr-assignee-repository-fixture') + +beforeEach(() => { + fixture.callbacks = [] + fixture.mutation = null + fixture.requests = [] + fixture.localGitOptions = {} + fixture.gh.mockReset() + fixture.gh.mockImplementation( + async ( + args: string[], + options: { + host?: string + cwd?: string + wslDistro?: string + } + ) => { + fixture.requests.push({ + args, + host: options.host, + cwd: options.cwd, + wslDistro: options.wslDistro + }) + return { stdout: '', stderr: '' } + } + ) + _resetOriginGitHubApiRepositoryCache() + fixture.apiUpdate = vi.fn( + (args: { + repoPath: string + number: number + updates: GitHubIssueUpdate + ownerRepo?: GitHubOwnerRepo + }) => + updateIssue( + args.repoPath, + args.number, + args.updates, + null, + fixture.localGitOptions, + fixture.preference, + args.ownerRepo + ) + ) + vi.stubGlobal('window', { api: { gh: { updateIssue: fixture.apiUpdate } } }) +}) + +afterEach(() => vi.unstubAllGlobals()) + +it.each([ + { owner: 'upstream-owner', preference: 'origin', assigned: false, legacy: false }, + { owner: 'upstream-owner', preference: 'origin', assigned: true, legacy: false }, + { owner: 'fork-owner', preference: 'upstream', assigned: false, legacy: false }, + { owner: 'fork-owner', preference: 'upstream', assigned: true, legacy: false }, + { owner: 'upstream-owner', preference: 'origin', assigned: false, legacy: true } +] as const)( + 'keeps $owner PR assignees under $preference (remove=$assigned, legacy=$legacy)', + async ({ owner, preference, assigned, legacy }) => { + fixture.preference = preference + fixture.localGitOptions = owner === 'fork-owner' ? { wslDistro: 'Ubuntu' } : {} + const item: GitHubWorkItem = { + id: 'pr:5', + type: 'pr', + number: 5, + title: 'Opened PR', + state: 'open', + url: `https://github.com/${owner}/widgets/pull/5`, + prRepo: legacy ? undefined : { owner, repo: 'widgets', host: 'github.com' }, + labels: [], + updatedAt: '', + author: null, + repoId: 'repo-1', + assignees: assigned ? [{ login: 'octo', name: null, avatarUrl: '' }] : [] + } + PRAssigneesPanel({ item, repoPath, projectOrigin: undefined, onMutated: vi.fn() }) + const toggleAssignee = fixture.callbacks.at(-1) + if (typeof toggleAssignee !== 'function') { + throw new Error('PR panel did not create an assignee handler') + } + toggleAssignee('octo') + await fixture.mutation + expect(fixture.requests).toEqual([ + { + args: [ + 'issue', + 'edit', + '5', + '--repo', + `${owner}/widgets`, + assigned ? '--remove-assignee' : '--add-assignee', + 'octo' + ], + host: 'github.com', + cwd: repoPath, + wslDistro: fixture.localGitOptions.wslDistro + } + ]) + expect(fixture.apiUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + repoPath, + repoId: item.repoId, + ownerRepo: { owner, repo: 'widgets', host: 'github.com' } + }) + ) + } +) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index fa331749c8b..ed72bc12232 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -23,7 +23,11 @@ export { type PullRequestPushTarget } from './client/lookup/pull-request-push-target' export { getRepoSlug, getRepoUpstream } from './client/fetch/repo-slug-upstream' -export { getWorkItem, getWorkItemByOwnerRepo } from './client/fetch/get-work-item' +export { + getWorkItem, + getWorkItemByOwnerRepo, + getWorkItemWithRepository +} from './client/fetch/get-work-item' export { listWorkItems } from './client/list/list-work-items' export { mergePR } from './client/merge/merge-pr' export { removePRReviewers, requestPRReviewers } from './client/update/pr-reviewers' diff --git a/src/main/github/client/fetch/get-work-item.ts b/src/main/github/client/fetch/get-work-item.ts index 3178a4c921a..5224061dfa3 100644 --- a/src/main/github/client/fetch/get-work-item.ts +++ b/src/main/github/client/fetch/get-work-item.ts @@ -21,6 +21,30 @@ export async function getWorkItem( localGitOptions: LocalGitExecOptions = {}, preference?: IssueSourcePreference ): Promise { + const { item } = await getWorkItemWithRepository( + repoPath, + number, + type, + connectionId, + localGitOptions, + preference + ) + return item +} + +export type WorkItemRepositoryLookup = { + item: MainWorkItem | null + repository: GitHubApiRepository | null +} + +export async function getWorkItemWithRepository( + repoPath: string, + number: number, + type?: 'issue' | 'pr', + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {}, + preference?: IssueSourcePreference +): Promise { await acquire() try { // Why: listWorkItems uses resolveIssueGitHubApiRepositorySource; open-by-number @@ -35,18 +59,24 @@ export async function getWorkItem( // Why: explicit origin with no origin identity must not bare-lookup ambient gh // (same fail-closed rule as origin-pinned PR candidate resolution). if (!source && preference === 'origin') { - return null + return { item: null, repository: null } + } + return { + item: await fetchIssueWorkItem(repoPath, source, number, connectionId, localGitOptions), + repository: source } - return await fetchIssueWorkItem(repoPath, source, number, connectionId, localGitOptions) } if (type === 'pr') { - return await fetchPullRequestWorkItemFromCandidates( - repoPath, - number, - connectionId, - localGitOptions, - preference - ) + return { + item: await fetchPullRequestWorkItemFromCandidates( + repoPath, + number, + connectionId, + localGitOptions, + preference + ), + repository: null + } } try { @@ -65,7 +95,7 @@ export async function getWorkItem( localGitOptions ) if (issue) { - return issue + return { item: issue, repository: source } } } } catch (err) { @@ -75,15 +105,18 @@ export async function getWorkItem( throw err } } - return await fetchPullRequestWorkItemFromCandidates( - repoPath, - number, - connectionId, - localGitOptions, - preference - ) + return { + item: await fetchPullRequestWorkItemFromCandidates( + repoPath, + number, + connectionId, + localGitOptions, + preference + ), + repository: null + } } catch { - return null + return { item: null, repository: null } } finally { release() } diff --git a/src/main/github/github-api-repository-validation.test.ts b/src/main/github/github-api-repository-validation.test.ts index 81e395a8bdb..7f8eec06602 100644 --- a/src/main/github/github-api-repository-validation.test.ts +++ b/src/main/github/github-api-repository-validation.test.ts @@ -22,4 +22,25 @@ describe('isValidGitHubApiRepository', () => { expect(isValidGitHubApiRepository({ owner: 'acme', repo: '..' })).toBe(false) expect(isValidGitHubApiRepository({ owner: 'acme', repo: 'a/b' })).toBe(false) }) + + it.each([42, false, null, {}])('rejects a non-string repository host: %j', (host) => { + expect(isValidGitHubApiRepository({ owner: 'acme', repo: 'orca', host })).toBe(false) + }) + + it.each([undefined, 'github.com', 'github.enterprise.test:8443'])( + 'accepts supported legacy or explicit repository host types: %j', + (host) => { + expect(isValidGitHubApiRepository({ owner: 'acme', repo: 'orca', host })).toBe(true) + } + ) + + it.each([ + null, + 'acme/orca', + { owner: 42, repo: 'orca' }, + { owner: 'acme', repo: 42 }, + { owner: 'acme' } + ])('rejects malformed runtime repository identities: %j', (repository) => { + expect(isValidGitHubApiRepository(repository)).toBe(false) + }) }) diff --git a/src/main/github/github-api-repository-validation.ts b/src/main/github/github-api-repository-validation.ts index b687a0ebdee..4031e64db13 100644 --- a/src/main/github/github-api-repository-validation.ts +++ b/src/main/github/github-api-repository-validation.ts @@ -11,8 +11,21 @@ export type GitHubApiRepositoryResolution = const OWNER_SLUG_RE = GITHUB_OWNER_SLUG_RE const REPOSITORY_SLUG_RE = /^[A-Za-z0-9._-]+$/ -export function isValidGitHubApiRepository(repository: GitHubOwnerRepo): boolean { +export function isValidGitHubApiRepository(repository: unknown): repository is GitHubOwnerRepo { + if ( + !repository || + typeof repository !== 'object' || + !('owner' in repository) || + !('repo' in repository) + ) { + return false + } return ( + typeof repository.owner === 'string' && + typeof repository.repo === 'string' && + (!('host' in repository) || + repository.host === undefined || + typeof repository.host === 'string') && OWNER_SLUG_RE.test(repository.owner) && REPOSITORY_SLUG_RE.test(repository.repo) && repository.repo !== '.' && diff --git a/src/main/github/issue-comment.ts b/src/main/github/issue-comment.ts index ac3f6a205de..478ac1ce68c 100644 --- a/src/main/github/issue-comment.ts +++ b/src/main/github/issue-comment.ts @@ -1,32 +1,34 @@ import type { GitHubCommentResult, PRComment } from '../../shared/github/comment-types' +import type { IssueSourcePreference } from '../../shared/repo-types' import type { LocalGitExecOptions, OwnerRepo } from './gh-utils' -import { getIssueGitHubApiRepository, resolveGitHubRepoExecution } from './github-api-repository' +import { + resolveGitHubRepoExecution, + resolveIssueGitHubApiRepositorySource +} from './github-api-repository' import { acquire, classifyGhError, ghExecFileAsync, release } from './gh-utils' -/** - * Add a comment to an existing GitHub issue. - * - * Why this path doesn't take a preference (mirrors `getIssue` / `updateIssue`): - * a comment is posted against an issue number already bound to a worktree or - * surfaced from a prior read. Routing through the live per-repo preference - * would let a user read upstream#N, toggle the selector to origin, and have - * their reply silently post on origin#N — a different issue entirely. That - * is the same silent-source-switch class of wrongness #1186 / the parent - * design doc guard against. List and create paths honor preference; - * mutations stay on the heuristic `getIssueOwnerRepo`. - */ +// An explicit target keeps replies bound to the conversation; otherwise use the selected source. export async function addIssueComment( repoPath: string, issueNumber: number, body: string, connectionId?: string | null, ownerRepoOverride?: OwnerRepo | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + preference?: IssueSourcePreference ): Promise { const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, ownerRepoOverride ?? - (() => getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions)), + (async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source), connectionId, localGitOptions ) diff --git a/src/main/github/issue-field-options.ts b/src/main/github/issue-field-options.ts index a4d2aae32f4..77e2337ae8a 100644 --- a/src/main/github/issue-field-options.ts +++ b/src/main/github/issue-field-options.ts @@ -1,4 +1,4 @@ -import type { GitHubAssignableUser } from '../../shared/github/pull-request-types' +import type { GitHubAssignableUser, GitHubOwnerRepo } from '../../shared/github/pull-request-types' import type { IssueSourcePreference } from '../../shared/repo-types' import type { LocalGitExecOptions } from './gh-utils' import { @@ -11,19 +11,21 @@ export async function listLabels( repoPath: string, preference?: IssueSourcePreference, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + repositoryOverride?: GitHubOwnerRepo | null ): Promise { const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, - async () => - ( - await resolveIssueGitHubApiRepositorySource( - repoPath, - preference, - connectionId, - localGitOptions - ) - ).source, + repositoryOverride ?? + (async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source), connectionId, localGitOptions ) @@ -57,19 +59,21 @@ export async function listAssignableUsers( repoPath: string, preference?: IssueSourcePreference, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + repositoryOverride?: GitHubOwnerRepo | null ): Promise { const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, - async () => - ( - await resolveIssueGitHubApiRepositorySource( - repoPath, - preference, - connectionId, - localGitOptions - ) - ).source, + repositoryOverride ?? + (async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source), connectionId, localGitOptions ) diff --git a/src/main/github/issue-mutation-source.test.ts b/src/main/github/issue-mutation-source.test.ts new file mode 100644 index 00000000000..48967198fd1 --- /dev/null +++ b/src/main/github/issue-mutation-source.test.ts @@ -0,0 +1,178 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' +import type * as GhUtils from './gh-utils' + +const { ghExecFileAsyncMock, resolveIssueSourceMock, acquireMock, releaseMock } = vi.hoisted( + () => ({ + ghExecFileAsyncMock: vi.fn(), + resolveIssueSourceMock: vi.fn(), + acquireMock: vi.fn(), + releaseMock: vi.fn() + }) +) + +vi.mock('./gh-utils', async () => { + const actual = await vi.importActual('./gh-utils') + return { + ...actual, + ghExecFileAsync: ghExecFileAsyncMock, + acquire: acquireMock, + release: releaseMock + } +}) + +vi.mock('./github-api-repository', async (importOriginal) => ({ + ...(await importOriginal()), + resolveIssueGitHubApiRepositorySource: resolveIssueSourceMock +})) + +import { addIssueComment } from './issue-comment' +import { updateIssue } from './issue-update' + +const ORIGIN = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } +const UPSTREAM = { owner: 'upstream-owner', repo: 'widgets', host: 'github.com' } + +describe('issue mutations follow the issue source preference', () => { + beforeEach(() => { + ghExecFileAsyncMock.mockReset() + acquireMock.mockReset() + acquireMock.mockResolvedValue(undefined) + releaseMock.mockReset() + resolveIssueSourceMock.mockReset() + resolveIssueSourceMock.mockImplementation(async (_repoPath: string, preference: unknown) => ({ + source: preference === 'origin' ? ORIGIN : UPSTREAM, + fellBack: false + })) + }) + + it('posts issue comments to origin when origin is selected', async () => { + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ id: 1, user: { login: 'octo', avatar_url: '' }, body: 'hi' }) + }) + + await expect( + addIssueComment('/repo-root', 5, 'hi', null, null, {}, 'origin') + ).resolves.toMatchObject({ ok: true }) + + expect(resolveIssueSourceMock).toHaveBeenCalledWith('/repo-root', 'origin', null, {}) + expect(ghExecFileAsyncMock.mock.calls[0][0]).toContain( + 'repos/fork-owner/widgets/issues/5/comments' + ) + }) + + it('keeps an explicit owner/repo override ahead of the preference', async () => { + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ id: 2, user: { login: 'octo', avatar_url: '' }, body: 'hi' }) + }) + + await addIssueComment('/repo-root', 7, 'hi', null, UPSTREAM, {}, 'origin') + + expect(resolveIssueSourceMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock.mock.calls[0][0]).toContain( + 'repos/upstream-owner/widgets/issues/7/comments' + ) + }) + + it('edits issues on origin when origin is selected', async () => { + ghExecFileAsyncMock.mockResolvedValue({ stdout: '' }) + + await expect( + updateIssue('/repo-root', 5, { body: 'Updated', addAssignees: ['octo'] }, null, {}, 'origin') + ).resolves.toEqual({ ok: true }) + + expect(resolveIssueSourceMock).toHaveBeenCalledWith('/repo-root', 'origin', null, {}) + expect(ghExecFileAsyncMock.mock.calls[0][0]).toContain('repos/fork-owner/widgets/issues/5') + expect(ghExecFileAsyncMock.mock.calls[1][0]).toEqual([ + 'issue', + 'edit', + '5', + '--repo', + 'fork-owner/widgets', + '--add-assignee', + 'octo' + ]) + }) + + it('keeps the upstream-first default when no preference is set', async () => { + ghExecFileAsyncMock.mockResolvedValue({ stdout: '' }) + + await updateIssue('/repo-root', 5, { body: 'Updated' }) + + expect(resolveIssueSourceMock).toHaveBeenCalledWith('/repo-root', undefined, undefined, {}) + expect(ghExecFileAsyncMock.mock.calls[0][0]).toContain('repos/upstream-owner/widgets/issues/5') + }) + + it('keeps edits on the opened issue when the saved preference changes', async () => { + ghExecFileAsyncMock.mockResolvedValue({ stdout: '' }) + + await updateIssue( + '/repo-root', + 5, + { body: 'Origin edit', state: 'closed', addLabels: ['bug'], addAssignees: ['octo'] }, + null, + {}, + 'upstream', + ORIGIN + ) + + expect(resolveIssueSourceMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock.mock.calls.map(([args]) => args)).toEqual([ + ['issue', 'close', '5', '--repo', 'fork-owner/widgets'], + [ + 'api', + '-X', + 'PATCH', + 'repos/fork-owner/widgets/issues/5', + '--raw-field', + 'body=Origin edit' + ], + [ + 'issue', + 'edit', + '5', + '--repo', + 'fork-owner/widgets', + '--add-label', + 'bug', + '--add-assignee', + 'octo' + ] + ]) + }) + + it('preserves local WSL execution with an explicit issue repository', async () => { + ghExecFileAsyncMock.mockResolvedValue({ stdout: '' }) + + await updateIssue( + '/home/fixture/widgets', + 5, + { body: 'Origin edit' }, + null, + { wslDistro: 'Ubuntu' }, + 'upstream', + ORIGIN + ) + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['repos/fork-owner/widgets/issues/5']), + expect.objectContaining({ + cwd: '/home/fixture/widgets', + wslDistro: 'Ubuntu', + host: 'github.com' + }) + ) + }) + + it('refuses unresolved Origin mutations instead of using ambient gh', async () => { + resolveIssueSourceMock.mockResolvedValue({ source: null, fellBack: false }) + + await expect( + updateIssue('/repo-root', 5, { body: 'Origin edit' }, null, {}, 'origin') + ).resolves.toMatchObject({ ok: false }) + await expect( + addIssueComment('/repo-root', 5, 'Origin reply', null, null, {}, 'origin') + ).resolves.toMatchObject({ ok: false }) + + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/github/issue-update.ts b/src/main/github/issue-update.ts index 801c12b79c5..8b00b24a148 100644 --- a/src/main/github/issue-update.ts +++ b/src/main/github/issue-update.ts @@ -1,31 +1,35 @@ import type { GitHubIssueUpdate } from '../../shared/issue-mutation-types' +import type { IssueSourcePreference } from '../../shared/repo-types' import type { LocalGitExecOptions } from './gh-utils' -import { getIssueGitHubApiRepository, resolveGitHubRepoExecution } from './github-api-repository' +import { + resolveGitHubRepoExecution, + resolveIssueGitHubApiRepositorySource, + type GitHubApiRepository +} from './github-api-repository' import { acquire, classifyGhError, ghExecFileAsync, release } from './gh-utils' -/** - * Update an existing GitHub issue. Fans out to separate gh commands for - * state changes vs field edits since `gh issue edit` does not support state. - * - * Why this path doesn't take a preference (mirrors `getIssue`): mutations - * target an issue number already bound to a worktree / linked elsewhere in - * the UI. Routing an update through the live per-repo preference would let - * a user open upstream#N, toggle the selector to origin, save, and silently - * write to origin#N — a different issue (or 404). That is the exact - * silent-source-switch class of wrongness #1186 / the parent design doc - * guard against. List and create paths honor preference; mutations stay on - * the heuristic `getIssueOwnerRepo`. - */ +// Use the selected source for issue-number edits. export async function updateIssue( repoPath: string, issueNumber: number, updates: GitHubIssueUpdate, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + preference?: IssueSourcePreference, + ownerRepoOverride?: GitHubApiRepository | null ): Promise<{ ok: true } | { ok: false; error: string }> { const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, - () => getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), + ownerRepoOverride ?? + (async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source), connectionId, localGitOptions ) diff --git a/src/main/github/work-item-details-concurrency.test.ts b/src/main/github/work-item-details-concurrency.test.ts index 3fdbacb6491..d7a0671470e 100644 --- a/src/main/github/work-item-details-concurrency.test.ts +++ b/src/main/github/work-item-details-concurrency.test.ts @@ -1,3 +1,4 @@ +import type * as WorkItemDetailsTestLookup from './work-item-details-test-lookup' import { beforeEach, describe, expect, it, vi } from 'vitest' const { @@ -27,8 +28,11 @@ vi.mock('./gh-utils', () => ({ }) })) -vi.mock('./client', () => ({ +vi.mock('./client', async () => ({ getWorkItem: getWorkItemMock, + getWorkItemWithRepository: ( + await vi.importActual('./work-item-details-test-lookup') + ).makeWorkItemDetailsLookupMock(getWorkItemMock), getPRComments: getPRCommentsMock, getPRChecks: getPRChecksMock })) diff --git a/src/main/github/work-item-details-config-change.test.ts b/src/main/github/work-item-details-config-change.test.ts new file mode 100644 index 00000000000..f10ecfe0046 --- /dev/null +++ b/src/main/github/work-item-details-config-change.test.ts @@ -0,0 +1,207 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { mkdir, mkdtemp, rm } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import type * as NodeOs from 'node:os' +import type * as GitExecError from '../git/exec-error' +import type * as WorkItemLookup from './client/fetch/get-work-item' +import { runProcess } from '../../shared/child-process/run-process' + +const { ghMock, fixture } = vi.hoisted(() => { + const fixture: { + path: string + initialHost: string + replacementUrl: string + requests: { args: string[]; host?: string }[] + } = { path: '', initialHost: '', replacementUrl: '', requests: [] } + return { ghMock: vi.fn(), fixture } +}) + +vi.mock('node:os', async (original) => ({ + ...(await original()), + homedir: () => join(fixture.path, 'isolated-home') +})) + +vi.mock('../git/runner', async () => { + const errors = await vi.importActual('../git/exec-error') + return { + ...errors, + ghExecFileAsync: ghMock, + gitExecFileAsync: async (args: string[], options: { cwd?: string }) => { + const result = await runProcess({ + program: 'git', + args, + cwd: options.cwd, + env: { + ...process.env, + GIT_CONFIG_NOSYSTEM: '1', + GIT_CONFIG_GLOBAL: join(fixture.path, 'isolated-home', '.gitconfig') + } + }) + if (result.code !== 0) { + throw new Error(result.stderr) + } + return { stdout: result.stdout, stderr: result.stderr } + } + } +}) + +vi.mock('./client', async () => ({ + ...(await vi.importActual('./client/fetch/get-work-item')), + getPRChecks: vi.fn(), + getPRComments: vi.fn() +})) + +vi.mock('./rate-limit', () => ({ + repositoryRateLimitGuard: () => ({ blocked: false }), + noteRepositoryRateLimitSpend: vi.fn() +})) + +import { getWorkItemDetails } from './work-item-details' +import { _resetOwnerRepoCache } from './github-repository-identity' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' +import { _resetGitHubHostAuthCache } from './github-enterprise-repository' + +async function fixtureGit(args: string[]): Promise { + const result = await runProcess({ + program: 'git', + args, + cwd: fixture.path, + env: { + ...process.env, + GIT_CONFIG_NOSYSTEM: '1', + GIT_CONFIG_GLOBAL: join(fixture.path, 'isolated-home', '.gitconfig') + } + }) + expect(result.code, result.stderr).toBe(0) +} + +beforeEach(async () => { + fixture.path = await mkdtemp(join(tmpdir(), 'orca-issue-config-change-')) + await mkdir(join(fixture.path, 'isolated-home')) + await fixtureGit(['init', '--quiet']) + await fixtureGit(['remote', 'add', 'origin', 'https://github.com/fork-owner/widgets.git']) + _resetOwnerRepoCache() + _resetOriginGitHubApiRepositoryCache() + _resetGitHubHostAuthCache() + fixture.initialHost = 'github.com' + fixture.replacementUrl = 'https://github.com/replacement-owner/widgets.git' + fixture.requests = [] + ghMock.mockReset() + ghMock.mockImplementation(async (args: string[], options: { host?: string }) => { + fixture.requests.push({ args, host: options.host }) + if (args[0] === 'auth' && args[1] === 'status') { + return { + stdout: ['github.com', 'github.enterprise.test'] + .map((host) => `${host}\n Logged in to ${host} account fixture (keyring)`) + .join('\n'), + stderr: '' + } + } + if (args.includes('repos/fork-owner/widgets/issues/5')) { + await fixtureGit(['remote', 'set-url', 'origin', fixture.replacementUrl]) + return { + stdout: JSON.stringify({ + number: 5, + title: 'ORIGIN title', + state: 'open', + html_url: `https://${fixture.initialHost}/fork-owner/widgets/issues/5`, + labels: [], + updated_at: '2026-10-02T00:00:00Z', + user: { login: 'fork-author' } + }), + stderr: '' + } + } + if (args.includes('graphql')) { + const original = args.includes('owner=fork-owner') && options.host === fixture.initialHost + const marker = original ? 'ORIGIN' : 'REPLACEMENT' + return { + stdout: JSON.stringify({ + data: { + repository: { + issue: { + body: `${marker} body`, + assignees: { nodes: [{ login: `${marker}-assignee` }] }, + participants: { nodes: [] }, + comments: { + nodes: [ + { + databaseId: 1, + body: `${marker} comment`, + createdAt: '2026-10-02T00:00:00Z', + url: `https://${options.host}/fork-owner/widgets/issues/5#issuecomment-1`, + author: { login: `${marker}-author`, avatarUrl: '' } + } + ] + } + } + } + } + }), + stderr: '' + } + } + if (args.some((arg) => arg.includes('/timeline?'))) { + return { stdout: '[]', stderr: '' } + } + throw new Error(`Unexpected fixture gh request: ${args.join(' ')}`) + }) +}) + +afterEach(async () => { + await rm(fixture.path, { recursive: true, force: true }) +}) + +it.each(['issue', undefined] as const)( + 'keeps implicit %s conversation on the repository used for the item fetch', + async (type) => { + const details = await getWorkItemDetails(fixture.path, 5, type, null, {}, 'origin') + expect(details?.item.title).toBe('ORIGIN title') + expect(details?.body).toBe('ORIGIN body') + expect(details?.assignees).toEqual(['ORIGIN-assignee']) + expect(details?.comments.map((comment) => comment.body)).toEqual(['ORIGIN comment']) + expect(fixture.requests[1].args).toContain('owner=fork-owner') + } +) + +it('keeps an explicit local Tasks identity through the same real config change', async () => { + const details = await getWorkItemDetails(fixture.path, 5, 'issue', null, {}, 'origin', { + owner: 'fork-owner', + repo: 'widgets', + host: 'github.com' + }) + expect(details?.item.title).toBe('ORIGIN title') + expect(details?.body).toBe('ORIGIN body') +}) + +it.each([ + { from: 'github.com', to: 'github.enterprise.test', qualified: false }, + { from: 'github.enterprise.test', to: 'github.com', qualified: false }, + { from: 'github.com', to: 'github.enterprise.test', qualified: true }, + { from: 'github.enterprise.test', to: 'github.com', qualified: true } +])( + 'keeps explicit host from $from to $to (qualified=$qualified)', + async ({ from, to, qualified }) => { + fixture.initialHost = from + fixture.replacementUrl = `https://${to}/fork-owner/widgets.git` + await fixtureGit(['remote', 'set-url', 'origin', `https://${from}/fork-owner/widgets.git`]) + const details = await getWorkItemDetails(fixture.path, 5, 'issue', null, {}, 'origin', { + owner: 'fork-owner', + repo: 'widgets', + ...(qualified ? { host: from } : {}) + }) + expect(details?.item.url).toBe(`https://${from}/fork-owner/widgets/issues/5`) + expect(details?.body).toBe('ORIGIN body') + expect(details?.assignees).toEqual(['ORIGIN-assignee']) + expect(details?.comments.map((comment) => comment.body)).toEqual(['ORIGIN comment']) + const conversationRequests = fixture.requests.filter( + ({ args }) => args.includes('graphql') || args.some((arg) => arg.includes('/timeline?')) + ) + expect(conversationRequests.some(({ args }) => args.includes('graphql'))).toBe(true) + expect( + conversationRequests.some(({ args }) => args.some((arg) => arg.includes('/timeline?'))) + ).toBe(true) + expect(conversationRequests.map(({ host }) => host)).toEqual([from, from]) + } +) diff --git a/src/main/github/work-item-details-enterprise-host.test.ts b/src/main/github/work-item-details-enterprise-host.test.ts index be2576051b5..b615f4e566b 100644 --- a/src/main/github/work-item-details-enterprise-host.test.ts +++ b/src/main/github/work-item-details-enterprise-host.test.ts @@ -1,3 +1,4 @@ +import type * as WorkItemDetailsTestLookup from './work-item-details-test-lookup' import { beforeEach, describe, expect, it, vi } from 'vitest' type RateLimitGuardResult = @@ -47,8 +48,11 @@ vi.mock('./gh-utils', () => ({ release: vi.fn() })) -vi.mock('./client', () => ({ +vi.mock('./client', async () => ({ getWorkItem: getWorkItemMock, + getWorkItemWithRepository: ( + await vi.importActual('./work-item-details-test-lookup') + ).makeWorkItemDetailsLookupMock(getWorkItemMock), getWorkItemByOwnerRepo: getWorkItemByOwnerRepoMock, getPRChecks: getPRChecksMock, getPRComments: getPRCommentsMock diff --git a/src/main/github/work-item-details-file-viewed.test.ts b/src/main/github/work-item-details-file-viewed.test.ts index 6075803493c..7cbfa041608 100644 --- a/src/main/github/work-item-details-file-viewed.test.ts +++ b/src/main/github/work-item-details-file-viewed.test.ts @@ -1,3 +1,4 @@ +import type * as WorkItemDetailsTestLookup from './work-item-details-test-lookup' import { beforeEach, describe, expect, it, vi } from 'vitest' type RateLimitGuardResult = @@ -59,8 +60,11 @@ vi.mock('./gh-utils', () => ({ release: releaseMock })) -vi.mock('./client', () => ({ +vi.mock('./client', async () => ({ getWorkItem: getWorkItemMock, + getWorkItemWithRepository: ( + await vi.importActual('./work-item-details-test-lookup') + ).makeWorkItemDetailsLookupMock(getWorkItemMock), getWorkItemByOwnerRepo: vi.fn(), getPRChecks: getPRChecksMock, getPRComments: getPRCommentsMock diff --git a/src/main/github/work-item-details-issue-source.test.ts b/src/main/github/work-item-details-issue-source.test.ts new file mode 100644 index 00000000000..e52de876d18 --- /dev/null +++ b/src/main/github/work-item-details-issue-source.test.ts @@ -0,0 +1,239 @@ +import type * as WorkItemDetailsTestLookup from './work-item-details-test-lookup' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + ghExecFileAsyncMock, + getOwnerRepoMock, + getOwnerRepoForRemoteMock, + getWorkItemMock, + getWorkItemByOwnerRepoMock, + acquireMock, + releaseMock +} = vi.hoisted(() => ({ + ghExecFileAsyncMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + getOwnerRepoForRemoteMock: vi.fn(), + getWorkItemMock: vi.fn(), + getWorkItemByOwnerRepoMock: vi.fn(), + acquireMock: vi.fn(), + releaseMock: vi.fn() +})) + +vi.mock('./gh-utils', () => ({ + ghExecFileAsync: ghExecFileAsyncMock, + getOwnerRepo: getOwnerRepoMock, + getOwnerRepoForRemote: getOwnerRepoForRemoteMock, + ghRepoExecOptions: vi.fn((context) => ({ cwd: context.repoPath })), + githubRepoContext: vi.fn((repoPath, connectionId, localGitOptions) => ({ + repoPath, + connectionId: connectionId ?? null, + ...localGitOptions + })), + acquire: acquireMock, + release: releaseMock +})) + +vi.mock('./client', async () => ({ + getWorkItem: getWorkItemMock, + getWorkItemWithRepository: ( + await vi.importActual('./work-item-details-test-lookup') + ).makeWorkItemDetailsLookupMock(getWorkItemMock), + getWorkItemByOwnerRepo: getWorkItemByOwnerRepoMock, + getPRChecks: vi.fn(), + getPRComments: vi.fn() +})) + +vi.mock('./github-enterprise-repository', () => ({ + getEnterpriseGitHubRepoSlug: vi.fn().mockResolvedValue(null), + getEnterpriseGitHubRepoSlugForRemote: vi.fn().mockResolvedValue(null), + isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true) +})) + +vi.mock('./rate-limit', () => ({ + repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpend: vi.fn() +})) + +vi.mock('../git/remote-name-listing', () => ({ + shouldProbeGitRemote: vi.fn().mockResolvedValue(true) +})) + +import { getWorkItemDetails } from './work-item-details' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +const REMOTES: Record = { + origin: { owner: 'fork-owner', repo: 'widgets' }, + upstream: { owner: 'upstream-owner', repo: 'widgets' } +} + +function graphQLIssueResponse(assignee: string): { stdout: string } { + return { + stdout: JSON.stringify({ + data: { + repository: { + issue: { + body: `${assignee} body`, + assignees: { nodes: [{ login: assignee }] }, + participants: { nodes: [] }, + comments: { nodes: [] } + } + } + } + }) + } +} + +function graphQLOwnerArg(): string | undefined { + const args: string[] = ghExecFileAsyncMock.mock.calls[0][0] + return args.find((arg) => arg.startsWith('owner=')) +} + +describe('getWorkItemDetails issue source', () => { + beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() + ghExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + getOwnerRepoMock.mockResolvedValue(REMOTES.origin) + getOwnerRepoForRemoteMock.mockReset() + getOwnerRepoForRemoteMock.mockImplementation( + async (_repoPath: string, remoteName: string) => REMOTES[remoteName] ?? null + ) + getWorkItemMock.mockReset() + getWorkItemByOwnerRepoMock.mockReset() + getWorkItemMock.mockResolvedValue({ + id: 'issue:5', + type: 'issue', + number: 5, + title: 'Origin issue', + state: 'open', + url: 'https://github.com/fork-owner/widgets/issues/5', + labels: [], + updatedAt: '2026-04-01T00:00:00Z', + author: 'fork-author' + }) + acquireMock.mockReset() + acquireMock.mockResolvedValue(undefined) + releaseMock.mockReset() + }) + + it('reads body, assignees and comments from origin when origin is selected', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce(graphQLIssueResponse('fork-assignee')) + .mockResolvedValueOnce({ stdout: '' }) + + const details = await getWorkItemDetails('/repo-root', 5, 'issue', null, {}, 'origin') + + expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 5, 'issue', null, {}, 'origin') + expect(graphQLOwnerArg()).toBe('owner=fork-owner') + expect(ghExecFileAsyncMock.mock.calls[1][0]).toContain( + 'repos/fork-owner/widgets/issues/5/timeline?per_page=100&page=1' + ) + expect(details?.assignees).toEqual(['fork-assignee']) + }) + + it('keeps the upstream-first default when no preference is set', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce(graphQLIssueResponse('upstream-assignee')) + .mockResolvedValueOnce({ stdout: '' }) + + const details = await getWorkItemDetails('/repo-root', 5, 'issue', null, {}) + + expect(graphQLOwnerArg()).toBe('owner=upstream-owner') + expect(details?.assignees).toEqual(['upstream-assignee']) + }) + + it('keeps a Project row on upstream even when Origin is selected', async () => { + const upstream = { ...REMOTES.upstream, host: 'github.com' } + getWorkItemByOwnerRepoMock.mockResolvedValue({ + id: 'issue:5', + type: 'issue', + number: 5, + title: 'Upstream project issue', + url: 'https://github.com/upstream-owner/widgets/issues/5', + state: 'open', + labels: [], + updatedAt: '', + author: 'upstream-author' + }) + ghExecFileAsyncMock + .mockResolvedValueOnce(graphQLIssueResponse('upstream-assignee')) + .mockResolvedValueOnce({ stdout: '' }) + + const details = await getWorkItemDetails('/repo-root', 5, 'issue', null, {}, 'origin', upstream) + + expect(getWorkItemMock).not.toHaveBeenCalled() + expect(getWorkItemByOwnerRepoMock).toHaveBeenCalledWith( + '/repo-root', + upstream, + 5, + 'issue', + null, + {} + ) + expect(graphQLOwnerArg()).toBe('owner=upstream-owner') + expect(details?.item.title).toBe('Upstream project issue') + expect(details?.assignees).toEqual(['upstream-assignee']) + }) + + it('does not fall back when the selected issue repository is unavailable', async () => { + getWorkItemByOwnerRepoMock.mockResolvedValue(null) + + await expect( + getWorkItemDetails('/repo-root', 5, 'issue', null, {}, 'upstream', { + ...REMOTES.origin, + host: 'github.com' + }) + ).resolves.toBeNull() + + expect(getWorkItemMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('rejects an invalid runtime issue host before calling the repository lookup', async () => { + await expect( + getWorkItemDetails('/repo-root', 5, 'issue', null, {}, 'upstream', { + ...REMOTES.origin, + host: 42 + }) + ).resolves.toBeNull() + + expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() + expect(getWorkItemMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('ignores an issue repository override on an untyped lookup', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce(graphQLIssueResponse('fork-assignee')) + .mockResolvedValueOnce({ stdout: '' }) + + await getWorkItemDetails('/repo-root', 5, undefined, null, {}, 'origin', { + owner: 'untrusted', + repo: 'other', + host: 'untrusted.example' + }) + + expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() + expect(graphQLOwnerArg()).toBe('owner=fork-owner') + expect(ghExecFileAsyncMock.mock.calls[0][1]).toMatchObject({ host: 'github.com' }) + }) + + it('normalizes host-less issue targets before loading the conversation', async () => { + getWorkItemByOwnerRepoMock.mockResolvedValue({ + type: 'issue', + number: 5, + title: 'Origin issue', + url: 'https://github.com/fork-owner/widgets/issues/5', + author: null + }) + ghExecFileAsyncMock + .mockResolvedValueOnce(graphQLIssueResponse('fork-assignee')) + .mockResolvedValueOnce({ stdout: '' }) + + await getWorkItemDetails('/repo-root', 5, 'issue', null, {}, 'upstream', REMOTES.origin) + + expect(graphQLOwnerArg()).toBe('owner=fork-owner') + expect(ghExecFileAsyncMock.mock.calls[0][1]).toMatchObject({ host: 'github.com' }) + expect(ghExecFileAsyncMock.mock.calls[1][1]).toMatchObject({ host: 'github.com' }) + }) +}) diff --git a/src/main/github/work-item-details-pr-files.test.ts b/src/main/github/work-item-details-pr-files.test.ts index d0205332bd5..c7af8abac85 100644 --- a/src/main/github/work-item-details-pr-files.test.ts +++ b/src/main/github/work-item-details-pr-files.test.ts @@ -1,3 +1,4 @@ +import type * as WorkItemDetailsTestLookup from './work-item-details-test-lookup' import { beforeEach, describe, expect, it, vi } from 'vitest' type RateLimitGuardResult = @@ -52,8 +53,11 @@ vi.mock('./gh-utils', () => ({ release: vi.fn() })) -vi.mock('./client', () => ({ +vi.mock('./client', async () => ({ getWorkItem: getWorkItemMock, + getWorkItemWithRepository: ( + await vi.importActual('./work-item-details-test-lookup') + ).makeWorkItemDetailsLookupMock(getWorkItemMock), getWorkItemByOwnerRepo: vi.fn(), getPRChecks: getPRChecksMock, getPRComments: getPRCommentsMock diff --git a/src/main/github/work-item-details-test-lookup.ts b/src/main/github/work-item-details-test-lookup.ts new file mode 100644 index 00000000000..69c1daf0777 --- /dev/null +++ b/src/main/github/work-item-details-test-lookup.ts @@ -0,0 +1,17 @@ +import { resolveIssueGitHubApiRepositorySource } from './github-api-repository' +import type { getWorkItem } from './client' +import type { WorkItemRepositoryLookup } from './client/fetch/get-work-item' + +// Why: detail tests replace item lookup; retain its selected-source metadata. +export function makeWorkItemDetailsLookupMock(lookup: typeof getWorkItem) { + return async (...args: Parameters): Promise => { + const item = await lookup(...args) + return { + item, + repository: + item?.type === 'issue' + ? (await resolveIssueGitHubApiRepositorySource(args[0], args[5], args[3], args[4])).source + : null + } + } +} diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index 3ec1a5a4d9f..ef2d3649818 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -1,3 +1,4 @@ +import type * as WorkItemDetailsTestLookup from './work-item-details-test-lookup' import { beforeEach, describe, expect, it, vi } from 'vitest' type RateLimitGuardResult = @@ -62,8 +63,11 @@ vi.mock('./gh-utils', () => ({ release: releaseMock })) -vi.mock('./client', () => ({ +vi.mock('./client', async () => ({ getWorkItem: getWorkItemMock, + getWorkItemWithRepository: ( + await vi.importActual('./work-item-details-test-lookup') + ).makeWorkItemDetailsLookupMock(getWorkItemMock), getWorkItemByOwnerRepo: getWorkItemByOwnerRepoMock, getPRChecks: getPRChecksMock, getPRComments: getPRCommentsMock diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index dafc4f514d6..385d66c9515 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -1,14 +1,16 @@ import type { PRCheckDetail } from '../../shared/github/check-types' import type { GitHubPRFile, GitHubPRFileContents } from '../../shared/github/pull-request-types' -import type { GitHubWorkItem, GitHubWorkItemDetails } from '../../shared/github/work-item-types' +import type { GitHubWorkItemDetails } from '../../shared/github/work-item-types' import type { IssueSourcePreference } from '../../shared/repo-types' -import { getPRChecks, getPRComments, getWorkItem } from './client' -import { acquire, release, type LocalGitExecOptions } from './gh-utils' import { - getIssueGitHubApiRepository, - resolveGitHubRepoExecution, - type GitHubApiRepository -} from './github-api-repository' + getPRChecks, + getPRComments, + getWorkItemWithRepository, + getWorkItemByOwnerRepo +} from './client' +import { acquire, release, type LocalGitExecOptions } from './gh-utils' +import { resolveGitHubRepoExecution, type GitHubApiRepository } from './github-api-repository' +import { isValidGitHubApiRepository } from './github-api-repository-validation' import { getIssueBodyAndComments, getIssueDetailsViaGraphQL } from './issue-work-item-details' import { getPRFiles, @@ -69,23 +71,49 @@ export async function getWorkItemDetails( type?: 'issue' | 'pr', connectionId?: string | null, localGitOptions: LocalGitExecOptions = {}, - preference?: IssueSourcePreference + preference?: IssueSourcePreference, + repositoryOverride?: unknown ): Promise { - const item: Omit | null = await getWorkItem( - repoPath, - number, - type, - connectionId, - localGitOptions, - preference - ) + let issueRepositoryOverride: GitHubApiRepository | null = null + if (type === 'issue' && repositoryOverride != null) { + if (!isValidGitHubApiRepository(repositoryOverride)) { + return null + } + issueRepositoryOverride = ( + await resolveGitHubRepoExecution(repoPath, repositoryOverride, connectionId, localGitOptions) + ).ownerRepo + if (!issueRepositoryOverride) { + return null + } + } + const { item, repository: selectedRepository } = issueRepositoryOverride + ? { + item: await getWorkItemByOwnerRepo( + repoPath, + issueRepositoryOverride, + number, + 'issue', + connectionId, + localGitOptions + ), + repository: issueRepositoryOverride + } + : await getWorkItemWithRepository( + repoPath, + number, + type, + connectionId, + localGitOptions, + preference + ) if (!item) { return null } + // Keep the conversation in the same repository as the selected issue. const resolvedRepository = item.type === 'issue' - ? await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) + ? selectedRepository : (await resolveGitHubRepoExecution(repoPath, item.prRepo, connectionId, localGitOptions)) .ownerRepo diff --git a/src/main/ipc/github-issue-mutation-handlers.ts b/src/main/ipc/github-issue-mutation-handlers.ts index 992f0c6e11f..8936e2eb15a 100644 --- a/src/main/ipc/github-issue-mutation-handlers.ts +++ b/src/main/ipc/github-issue-mutation-handlers.ts @@ -15,7 +15,14 @@ import { broadcastGitHubWorkItemMutation } from './github-work-item-mutation-eve export function registerGitHubIssueMutationHandlers(store: Store): void { ipcMain.handle( 'gh:updateIssue', - async (event, args: GitHubRepoScopedArgs & { number: number; updates: GitHubIssueUpdate }) => { + async ( + event, + args: GitHubRepoScopedArgs & { + number: number + updates: GitHubIssueUpdate + ownerRepo?: GitHubOwnerRepo + } + ) => { const repo = assertRegisteredGitHubRepo(args, store) if (typeof args.number !== 'number' || !Number.isInteger(args.number) || args.number < 1) { return { ok: false, error: 'Invalid issue number' } @@ -28,7 +35,9 @@ export function registerGitHubIssueMutationHandlers(store: Store): void { args.number, args.updates, getGitHubRepoConnectionId(repo), - ...getGitHubLocalGitOptionArgs(store, repo) + getGitHubLocalGitOptionArgs(store, repo)[0], + repo.issueSourcePreference, + ...(args.ownerRepo ? [args.ownerRepo] : []) ) if (result.ok) { broadcastGitHubWorkItemMutation( @@ -67,7 +76,9 @@ export function registerGitHubIssueMutationHandlers(store: Store): void { args.body.trim(), getGitHubRepoConnectionId(repo), args.prRepo ?? null, - ...getGitHubLocalGitOptionArgs(store, repo) + getGitHubLocalGitOptionArgs(store, repo)[0], + // Why: the issue source selector only scopes issues; PR comments keep their resolution. + args.type === 'pr' ? undefined : repo.issueSourcePreference ) if (result.ok) { broadcastGitHubWorkItemMutation( @@ -84,23 +95,46 @@ export function registerGitHubIssueMutationHandlers(store: Store): void { } ) - ipcMain.handle('gh:listLabels', (_event, args: GitHubRepoScopedArgs) => { - const repo = assertRegisteredGitHubRepo(args, store) - return listLabels( - repo.path, - repo.issueSourcePreference, - getGitHubRepoConnectionId(repo), - ...getGitHubLocalGitOptionArgs(store, repo) - ) - }) + ipcMain.handle( + 'gh:listLabels', + (_event, args: GitHubRepoScopedArgs & { ownerRepo?: GitHubOwnerRepo }) => { + const repo = assertRegisteredGitHubRepo(args, store) + const localGitOptions = getGitHubLocalGitOptionArgs(store, repo) + const connectionId = getGitHubRepoConnectionId(repo) + if (args.ownerRepo) { + return listLabels( + repo.path, + repo.issueSourcePreference, + connectionId, + localGitOptions[0], + args.ownerRepo + ) + } + return listLabels(repo.path, repo.issueSourcePreference, connectionId, ...localGitOptions) + } + ) - ipcMain.handle('gh:listAssignableUsers', (_event, args: GitHubRepoScopedArgs) => { - const repo = assertRegisteredGitHubRepo(args, store) - return listAssignableUsers( - repo.path, - repo.issueSourcePreference, - getGitHubRepoConnectionId(repo), - ...getGitHubLocalGitOptionArgs(store, repo) - ) - }) + ipcMain.handle( + 'gh:listAssignableUsers', + (_event, args: GitHubRepoScopedArgs & { ownerRepo?: GitHubOwnerRepo }) => { + const repo = assertRegisteredGitHubRepo(args, store) + const localGitOptions = getGitHubLocalGitOptionArgs(store, repo) + const connectionId = getGitHubRepoConnectionId(repo) + if (args.ownerRepo) { + return listAssignableUsers( + repo.path, + repo.issueSourcePreference, + connectionId, + localGitOptions[0], + args.ownerRepo + ) + } + return listAssignableUsers( + repo.path, + repo.issueSourcePreference, + connectionId, + ...localGitOptions + ) + } + ) } diff --git a/src/main/ipc/github-issue-source-preference.test.ts b/src/main/ipc/github-issue-source-preference.test.ts index ffef0da244b..eb51af7773e 100644 --- a/src/main/ipc/github-issue-source-preference.test.ts +++ b/src/main/ipc/github-issue-source-preference.test.ts @@ -143,4 +143,124 @@ describe('registerGitHubHandlers', () => { 'origin' ) }) + + it('passes the displayed repository ahead of the current source preference', async () => { + harness.repos[0].issueSourcePreference = 'upstream' + const ownerRepo = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } + mocks.client.updateIssue.mockResolvedValue({ ok: false, error: 'stub' }) + getWorkItemDetailsMock.mockResolvedValue(null) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: These fixtures implement the repo methods used by the handlers; unrelated handlers are only registered. + registerGitHubHandlers(store as never, stats as never) + + await handlers['gh:workItemDetails'](null, { + repoPath: '/workspace/repo', + number: 12, + type: 'issue', + ownerRepo + }) + await handlers['gh:updateIssue'](null, { + repoPath: '/workspace/repo', + number: 12, + updates: { body: 'Origin edit' }, + ownerRepo + }) + await handlers['gh:listLabels'](null, { repoPath: '/workspace/repo', ownerRepo }) + await handlers['gh:listAssignableUsers'](null, { repoPath: '/workspace/repo', ownerRepo }) + + expect(getWorkItemDetailsMock).toHaveBeenCalledWith( + '/workspace/repo', + 12, + 'issue', + null, + undefined, + 'upstream', + ownerRepo + ) + expect(mocks.client.updateIssue).toHaveBeenCalledWith( + '/workspace/repo', + 12, + { body: 'Origin edit' }, + null, + undefined, + 'upstream', + ownerRepo + ) + expect(mocks.client.listLabels).toHaveBeenCalledWith( + '/workspace/repo', + 'upstream', + null, + undefined, + ownerRepo + ) + expect(mocks.client.listAssignableUsers).toHaveBeenCalledWith( + '/workspace/repo', + 'upstream', + null, + undefined, + ownerRepo + ) + }) + + it('pins the repo origin source preference on issue update and comment IPC', async () => { + harness.repos = [ + { + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + issueSourcePreference: 'origin' + } + ] + mocks.client.updateIssue.mockResolvedValue({ ok: false, error: 'stub' }) + mocks.client.addIssueComment.mockResolvedValue({ ok: false, error: 'stub' }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: These fixtures implement the repo methods used by the handlers; unrelated handlers are only registered. + registerGitHubHandlers(store as never, stats as never) + + await handlers['gh:updateIssue'](null, { + repoPath: '/workspace/repo', + number: 5, + updates: { body: 'Updated' } + }) + await handlers['gh:addIssueComment'](null, { + repoPath: '/workspace/repo', + number: 5, + body: 'Issue comment' + }) + await handlers['gh:addIssueComment'](null, { + repoPath: '/workspace/repo', + number: 6, + body: 'PR comment', + type: 'pr' + }) + + expect(mocks.client.updateIssue).toHaveBeenCalledWith( + '/workspace/repo', + 5, + { body: 'Updated' }, + null, + undefined, + 'origin' + ) + expect(mocks.client.addIssueComment).toHaveBeenNthCalledWith( + 1, + '/workspace/repo', + 5, + 'Issue comment', + null, + null, + undefined, + 'origin' + ) + expect(mocks.client.addIssueComment).toHaveBeenNthCalledWith( + 2, + '/workspace/repo', + 6, + 'PR comment', + null, + null, + undefined, + undefined + ) + }) }) diff --git a/src/main/ipc/github-work-item-handlers.ts b/src/main/ipc/github-work-item-handlers.ts index 26c21b3db81..94e982d60dc 100644 --- a/src/main/ipc/github-work-item-handlers.ts +++ b/src/main/ipc/github-work-item-handlers.ts @@ -146,15 +146,20 @@ export function registerGitHubWorkItemHandlers(store: Store): void { } ) - ipcMain.handle('gh:workItemDetails', (_event, args: WorkItemArgs) => { - const repo = assertRegisteredGitHubRepo(args, store) - return dispatchWorkItem( - args, - repo, - getWorkItemDetails, - getGitHubLocalGitOptionArgs(store, repo)[0] - ) - }) + ipcMain.handle( + 'gh:workItemDetails', + (_event, args: WorkItemArgs & { ownerRepo?: GitHubOwnerRepo }) => { + const repo = assertRegisteredGitHubRepo(args, store) + return dispatchWorkItem( + args, + repo, + args.ownerRepo + ? (...params) => getWorkItemDetails(...params, args.ownerRepo) + : getWorkItemDetails, + getGitHubLocalGitOptionArgs(store, repo)[0] + ) + } + ) ipcMain.handle( 'gh:notifyWorkItemMutated', diff --git a/src/main/ipc/github-wsl-runtime-routing.test.ts b/src/main/ipc/github-wsl-runtime-routing.test.ts index e2f30c18eba..977422cb68e 100644 --- a/src/main/ipc/github-wsl-runtime-routing.test.ts +++ b/src/main/ipc/github-wsl-runtime-routing.test.ts @@ -138,6 +138,15 @@ describe('registerGitHubHandlers', () => { ) await handlers['gh:listLabels'](null, { repoPath: '/workspace/repo' }) await handlers['gh:listAssignableUsers'](null, { repoPath: '/workspace/repo' }) + const metadataRepo = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } + await handlers['gh:listLabels'](null, { + repoPath: '/workspace/repo', + ownerRepo: metadataRepo + }) + await handlers['gh:listAssignableUsers'](null, { + repoPath: '/workspace/repo', + ownerRepo: metadataRepo + }) expect(getPRForBranchMock).toHaveBeenCalledWith( '/workspace/repo', @@ -197,7 +206,8 @@ describe('registerGitHubHandlers', () => { 7, { body: 'Updated' }, null, - localGitOptions + localGitOptions, + undefined ) expect(addIssueCommentMock).toHaveBeenCalledWith( '/workspace/repo', @@ -205,9 +215,24 @@ describe('registerGitHubHandlers', () => { 'Comment', null, null, - localGitOptions + localGitOptions, + undefined ) expect(listLabelsMock).toHaveBeenCalledWith('/workspace/repo', undefined, null, localGitOptions) + expect(listLabelsMock).toHaveBeenLastCalledWith( + '/workspace/repo', + undefined, + null, + localGitOptions, + metadataRepo + ) + expect(listAssignableUsersMock).toHaveBeenLastCalledWith( + '/workspace/repo', + undefined, + null, + localGitOptions, + metadataRepo + ) expect(listAssignableUsersMock).toHaveBeenCalledWith( '/workspace/repo', undefined, diff --git a/src/main/runtime/orca-runtime-tests/hooks-and-hosted-review.spec.ts b/src/main/runtime/orca-runtime-tests/hooks-and-hosted-review.spec.ts index 69878468ccb..139e018025a 100644 --- a/src/main/runtime/orca-runtime-tests/hooks-and-hosted-review.spec.ts +++ b/src/main/runtime/orca-runtime-tests/hooks-and-hosted-review.spec.ts @@ -319,7 +319,8 @@ describe('OrcaRuntimeService', () => { 12, issueUpdates, null, - localGitOptions + localGitOptions, + undefined ) expect(addGitHubIssueCommentMock).toHaveBeenCalledWith( TEST_REPO_PATH, @@ -327,7 +328,8 @@ describe('OrcaRuntimeService', () => { 'Comment', null, null, - localGitOptions + localGitOptions, + undefined ) expect(listGitHubLabelsMock).toHaveBeenCalledWith( TEST_REPO_PATH, diff --git a/src/main/runtime/rpc/methods/github-issue-methods.ts b/src/main/runtime/rpc/methods/github-issue-methods.ts index 86300017741..825fe573d2f 100644 --- a/src/main/runtime/rpc/methods/github-issue-methods.ts +++ b/src/main/runtime/rpc/methods/github-issue-methods.ts @@ -35,6 +35,19 @@ export const GITHUB_ISSUE_METHODS = [ name: 'github.addIssueComment', params: IssueComment, handler: async (params, { runtime }) => - runtime.addRepoIssueComment(params.repo, params.number, params.body, params.prRepo ?? null) + params.type + ? runtime.addRepoIssueComment( + params.repo, + params.number, + params.body, + params.prRepo ?? null, + params.type + ) + : runtime.addRepoIssueComment( + params.repo, + params.number, + params.body, + params.prRepo ?? null + ) }) ] diff --git a/src/main/runtime/rpc/methods/github.test.ts b/src/main/runtime/rpc/methods/github.test.ts index d1bf9946806..1ed202d2f57 100644 --- a/src/main/runtime/rpc/methods/github.test.ts +++ b/src/main/runtime/rpc/methods/github.test.ts @@ -626,10 +626,13 @@ describe('github RPC methods', () => { }) ) - expect(runtime.addRepoIssueComment).toHaveBeenCalledWith('repo-1', 3, 'Looks good', { - owner: 'acme', - repo: 'widgets' - }) + expect(runtime.addRepoIssueComment).toHaveBeenCalledWith( + 'repo-1', + 3, + 'Looks good', + { owner: 'acme', repo: 'widgets' }, + 'pr' + ) expect(response).toMatchObject({ ok: true, result: { ok: true, comment: { id: 1 } } }) }) diff --git a/src/main/runtime/runtime-github-issue-comment-commands.ts b/src/main/runtime/runtime-github-issue-comment-commands.ts index fcef9602405..e3f19c36d76 100644 --- a/src/main/runtime/runtime-github-issue-comment-commands.ts +++ b/src/main/runtime/runtime-github-issue-comment-commands.ts @@ -50,7 +50,8 @@ export class RuntimeGitHubIssueCommentCommands { number, updates, repo.connectionId ?? null, - ...this.deps.getLocalGitArgs(repo) + this.deps.getLocalGitArgs(repo)[0], + repo.issueSourcePreference ) } @@ -58,7 +59,8 @@ export class RuntimeGitHubIssueCommentCommands { repoSelector: string, number: number, body: string, - prRepo?: GitHubOwnerRepo | null + prRepo?: GitHubOwnerRepo | null, + type?: 'issue' | 'pr' ): Promise>> { const repo = await this.deps.resolveRepo(repoSelector) return addIssueComment( @@ -67,7 +69,9 @@ export class RuntimeGitHubIssueCommentCommands { body, repo.connectionId ?? null, prRepo ?? null, - ...this.deps.getLocalGitArgs(repo) + this.deps.getLocalGitArgs(repo)[0], + // Why: the issue source selector only scopes issues; PR comments keep their resolution. + type === 'pr' ? undefined : repo.issueSourcePreference ) } diff --git a/src/preload/api/github-work-item-api.ts b/src/preload/api/github-work-item-api.ts index ae95a908fb6..db5842eb2cf 100644 --- a/src/preload/api/github-work-item-api.ts +++ b/src/preload/api/github-work-item-api.ts @@ -75,6 +75,7 @@ export type GithubWorkItemApi = { args: GitHubRepoSelectorArgs & { number: number type?: 'issue' | 'pr' + ownerRepo?: GitHubOwnerRepo } ) => Promise notifyWorkItemMutated: (args: { @@ -106,6 +107,7 @@ export type GithubWorkItemApi = { args: GitHubRepoSelectorArgs & { number: number updates: GitHubIssueUpdate + ownerRepo?: GitHubOwnerRepo } ) => Promise<{ ok: true } | { ok: false; error: string }> addIssueComment: ( @@ -121,11 +123,13 @@ export type GithubWorkItemApi = { repoPath: string repoId?: string sourceContext?: TaskSourceContext | null + ownerRepo?: GitHubOwnerRepo }) => Promise listAssignableUsers: (args: { repoPath: string repoId?: string sourceContext?: TaskSourceContext | null + ownerRepo?: GitHubOwnerRepo }) => Promise /** Subscribe to local-mutation broadcasts so the work-item-drawer cache can invalidate across windows. Returns an unsubscribe. */ onWorkItemMutated: ( diff --git a/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section-mutations.ts b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section-mutations.ts index 42fba184a2e..12d6831f18c 100644 --- a/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section-mutations.ts +++ b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section-mutations.ts @@ -10,7 +10,9 @@ import { import { assertTaskPageGitHubDialogStateAuthority } from '@/components/task-page-github-dialog-state-authority' import { runIssueUpdate } from '@/components/github/github-work-item-edit-mutations' import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' +import type { GitHubOwnerRepo } from '../../../../../shared/github/pull-request-types' import type { TaskSourceContext } from '../../../../../shared/task-source-context' +import type { GitHubPatchWorkItemOptions } from '@/store/github/cache-model' import { translate } from '@/i18n/i18n' import type { GitHubItemDialogProjectOrigin } from '../load-item-details/github-item-dialog-types' @@ -28,6 +30,7 @@ type GHEditMutationBase = { repoPath: string | null sourceContext?: TaskSourceContext | null projectOrigin: GitHubItemDialogProjectOrigin | undefined + issueRepo?: GitHubOwnerRepo | null run: GHEditMutationRun patchProjectRowIfNeeded: (patch: GHEditProjectRowPatch) => void onMutated: () => void @@ -43,6 +46,7 @@ export function runGHEditStateChange({ repoPath, sourceContext, projectOrigin, + issueRepo, run, onStateChange, patchWorkItem, @@ -58,7 +62,7 @@ export function runGHEditStateChange({ id: string, patch: { state: GitHubWorkItem['state'] }, repoId: string | undefined, - options: { sourceContext?: TaskSourceContext | null } + options: GitHubPatchWorkItemOptions ) => void }): void { // Why: a close reason still has to reach GitHub even when the item already reads as closed locally. @@ -76,6 +80,7 @@ export function runGHEditStateChange({ repoPath, sourceContext, projectOrigin, + issueRepo, number: itemNumber, updates: newState === 'closed' && closeAction @@ -87,22 +92,32 @@ export function runGHEditStateChange({ repoId: itemRepoId, itemId, state: newState, - sourceContext + sourceContext, + ownerRepo: projectOrigin ?? issueRepo }) onStateChange(newState) - patchWorkItem(itemId, { state: newState }, itemRepoId, { sourceContext }) + patchWorkItem(itemId, { state: newState }, itemRepoId, { + sourceContext, + ownerRepo: projectOrigin ?? issueRepo + }) patchProjectRowIfNeeded({ state: newState }) }, onRevert: () => { if (authority?.revert()) { onStateChange(prevState) - patchWorkItem(itemId, { state: prevState }, itemRepoId, { sourceContext }) + patchWorkItem(itemId, { state: prevState }, itemRepoId, { + sourceContext, + ownerRepo: projectOrigin ?? issueRepo + }) patchProjectRowIfNeeded({ state: prevState }) } }, onSuccess: () => { useAppStore.getState().recordFeatureInteraction('github-tasks') - patchWorkItem(itemId, { state: newState }, itemRepoId, { sourceContext }) + patchWorkItem(itemId, { state: newState }, itemRepoId, { + sourceContext, + ownerRepo: projectOrigin ?? issueRepo + }) patchProjectRowIfNeeded({ state: newState }) onMutated() }, @@ -145,6 +160,7 @@ export function runGHEditLabelToggle({ repoPath, sourceContext, projectOrigin, + issueRepo, run, onLabelsChange, patchWorkItem, @@ -159,42 +175,13 @@ export function runGHEditLabelToggle({ id: string, patch: { labels: string[] }, repoId: string | undefined, - options: { sourceContext?: TaskSourceContext | null } + options: GitHubPatchWorkItemOptions ) => void }): void { const isAdding = !localLabels.includes(label) const prevLabels = localLabels const newLabels = isAdding ? [...prevLabels, label] : prevLabels.filter((l) => l !== label) - if (isAdding) { - void run('labels', { - mutate: () => - runIssueUpdate({ - repoId: itemRepoId, - repoPath, - sourceContext, - projectOrigin, - number: itemNumber, - updates: { addLabels: [label] } - }), - onOptimistic: () => { - onLabelsChange(newLabels) - patchWorkItem(itemId, { labels: newLabels }, itemRepoId, { sourceContext }) - patchProjectRowIfNeeded({ labels: newLabels }) - }, - onSuccess: () => { - useAppStore.getState().recordFeatureInteraction('github-tasks') - onMutated() - }, - onRevert: () => { - onLabelsChange(prevLabels) - patchWorkItem(itemId, { labels: prevLabels }, itemRepoId, { sourceContext }) - patchProjectRowIfNeeded({ labels: prevLabels }) - }, - onError: (err) => toast.error(err) - }) - return - } void run('labels', { mutate: () => runIssueUpdate({ @@ -202,17 +189,24 @@ export function runGHEditLabelToggle({ repoPath, sourceContext, projectOrigin, + issueRepo, number: itemNumber, - updates: { removeLabels: [label] } + updates: isAdding ? { addLabels: [label] } : { removeLabels: [label] } }), onOptimistic: () => { onLabelsChange(newLabels) - patchWorkItem(itemId, { labels: newLabels }, itemRepoId, { sourceContext }) + patchWorkItem(itemId, { labels: newLabels }, itemRepoId, { + sourceContext, + ownerRepo: projectOrigin ?? issueRepo + }) patchProjectRowIfNeeded({ labels: newLabels }) }, onRevert: () => { onLabelsChange(prevLabels) - patchWorkItem(itemId, { labels: prevLabels }, itemRepoId, { sourceContext }) + patchWorkItem(itemId, { labels: prevLabels }, itemRepoId, { + sourceContext, + ownerRepo: projectOrigin ?? issueRepo + }) patchProjectRowIfNeeded({ labels: prevLabels }) }, onSuccess: () => { @@ -233,6 +227,7 @@ export function runGHEditAssigneeToggle({ repoPath, sourceContext, projectOrigin, + issueRepo, run, setLocalAssignees, patchProjectRowIfNeeded, @@ -260,6 +255,7 @@ export function runGHEditAssigneeToggle({ repoPath, sourceContext, projectOrigin, + issueRepo, number: itemNumber, updates: { removeAssignees: [login] } }), @@ -288,6 +284,7 @@ export function runGHEditAssigneeToggle({ repoPath, sourceContext, projectOrigin, + issueRepo, number: itemNumber, updates: { addAssignees: [login] } }), diff --git a/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.assignee-identity.test.tsx b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.assignee-identity.test.tsx new file mode 100644 index 00000000000..be5ee88f54d --- /dev/null +++ b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.assignee-identity.test.tsx @@ -0,0 +1,364 @@ +// @vitest-environment happy-dom + +import { act, useState } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' +import type { GitHubItemDialogProjectOrigin } from '../load-item-details/github-item-dialog-types' +import { GHEditSection } from './gh-edit-section' +import { resetTaskPageGitHubMutationRegistryForTests } from '@/components/task-page-github-work-item-mutation-registry' + +const mocks = vi.hoisted(() => ({ + update: vi.fn(), + patchWorkItem: vi.fn(), + patchProjectRowContent: vi.fn(), + recordFeatureInteraction: vi.fn(), + onMutated: vi.fn() +})) + +vi.mock('@/store', () => { + const state = { + patchWorkItem: mocks.patchWorkItem, + patchProjectRowContent: mocks.patchProjectRowContent, + recordFeatureInteraction: mocks.recordFeatureInteraction, + repos: [], + settings: null + } + return { + useAppStore: Object.assign((selector: (value: typeof state) => unknown) => selector(state), { + getState: () => state + }) + } +}) + +vi.mock('@/hooks/useIssueMetadata', async () => { + const { useImmediateMutation } = await import('@/hooks/useImmediateMutation') + return { + useImmediateMutation, + useRepoLabels: () => ({ data: [] }), + useRepoAssignees: () => ({ data: [] }) + } +}) +vi.mock('@/hooks/useGitHubSlugMetadata', () => ({ + useRepoLabelsBySlug: () => ({ data: [] }), + useRepoAssigneesBySlug: () => ({ data: [] }) +})) +vi.mock('@/components/github/github-duplicate-issue-candidates', () => ({ + useGitHubDuplicateIssueCandidates: () => [] +})) +vi.mock('@/components/github/github-work-item-edit-mutations', () => ({ + runIssueUpdate: mocks.update +})) +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) +vi.mock('./gh-edit-section-horizontal', () => ({ GHEditSectionHorizontal: () => null })) +vi.mock('./gh-edit-section-top-columns', () => ({ + GHEditSectionTopColumns: ({ + localAssignees, + onAssigneeToggle, + localState, + localLabels, + onStateChange, + onLabelToggle + }: { + localAssignees: string[] + onAssigneeToggle: (login: string) => void + localState: GitHubWorkItem['state'] + localLabels: string[] + onStateChange: (state: 'open' | 'closed') => void + onLabelToggle: (label: string) => void + }) => ( + <> + {localAssignees.join(',')} + {localState} + {localLabels.join(',')} + + + + + + ) +})) + +const item: GitHubWorkItem = { + id: 'issue:5', + repoId: 'registered-repo', + type: 'issue', + number: 5, + title: 'Issue five', + state: 'open', + url: 'https://github.com/upstream/widgets/issues/5', + labels: [], + updatedAt: '', + author: null +} +let root: Root +let container: HTMLDivElement + +async function render( + nextItem = item, + assignees: string[] = [], + projectOrigin?: GitHubItemDialogProjectOrigin +): Promise { + await act(async () => { + root.render( + {}} + onLabelsChange={() => {}} + onMutated={mocks.onMutated} + assignees={assignees} + onUse={() => {}} + layout="top-columns" + /> + ) + }) +} + +async function click(label = 'Assign candidate'): Promise { + const button = [...container.querySelectorAll('button')].find( + (candidate) => candidate.textContent === label + ) + expect(button).toBeDefined() + await act(async () => button?.click()) +} + +function selected(): string | null { + return container.querySelector('[data-field="assignees"]')?.textContent ?? null +} + +let updateParentSelection: (nextItem: GitHubWorkItem) => void = () => {} + +function ParentSelection({ item }: { item: GitHubWorkItem }): React.JSX.Element { + const [localState, setLocalState] = useState(item.state) + const [localLabels, setLocalLabels] = useState(item.labels) + updateParentSelection = (nextItem) => { + setLocalState(nextItem.state) + setLocalLabels(nextItem.labels) + } + return ( + {}} + layout="top-columns" + /> + ) +} + +async function renderParent(nextItem: GitHubWorkItem): Promise { + await act(async () => { + updateParentSelection(nextItem) + root.render() + }) +} + +describe('opened issue assignee ownership', () => { + beforeEach(() => { + vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true) + vi.clearAllMocks() + resetTaskPageGitHubMutationRegistryForTests() + updateParentSelection = () => {} + mocks.update.mockResolvedValue({ ok: true }) + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + }) + + afterEach(() => { + act(() => root.unmount()) + container.remove() + vi.unstubAllGlobals() + resetTaskPageGitHubMutationRegistryForTests() + }) + + it('retires an optimistic selection across fork and return navigation', async () => { + await render() + await click() + expect(selected()).toBe('candidate-user') + await render(item, []) + expect(selected()).toBe('candidate-user') + + await render({ ...item, url: 'https://github.com/fork/widgets/issues/5' }, ['fork-user']) + expect(selected()).toBe('fork-user') + await render(item, ['authoritative-upstream-user']) + expect(selected()).toBe('authoritative-upstream-user') + }) + + it('does not revive an edited guard when returning from a different issue number', async () => { + await render() + await click() + await render({ ...item, id: 'issue:6', number: 6 }, ['other-issue-user']) + expect(selected()).toBe('other-issue-user') + await render(item, ['authoritative-upstream-user']) + expect(selected()).toBe('authoritative-upstream-user') + }) + + it.each(['Assign candidate', 'Toggle original'])( + 'does not roll a late failed %s edit into another opened issue', + async (button) => { + let rejectUpdate: (reason: Error) => void = () => {} + mocks.update.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectUpdate = reject + }) + ) + await render(item, ['original-user']) + await click(button) + await render({ ...item, id: 'issue:6', number: 6 }, ['other-issue-user']) + expect(selected()).toBe('other-issue-user') + await act(async () => rejectUpdate(new Error('held edit failed'))) + expect(selected()).toBe('other-issue-user') + } + ) + + it('treats another GitHub host as a distinct same-number issue', async () => { + await render() + await click() + await render({ ...item, url: 'https://github.example.test/upstream/widgets/issues/5' }, []) + expect(selected()).toBe('') + }) + + it('retires the issue edit when the same legacy id is used by a pull request', async () => { + await render() + await click() + await render({ ...item, type: 'pr' }, []) + await render(item, ['authoritative-upstream-user']) + expect(selected()).toBe('authoritative-upstream-user') + }) + + it('preserves optimistic edits across canonical URL casing and trailing paths', async () => { + await render() + await click() + await render({ ...item, url: 'https://GITHUB.COM/Upstream/Widgets/issues/5#activity' }, []) + expect(selected()).toBe('candidate-user') + }) + + it('uses the Project row repository before an unrelated item URL', async () => { + const projectOrigin: GitHubItemDialogProjectOrigin = { + owner: 'upstream', + repo: 'widgets', + number: 5, + type: 'issue', + projectId: 'project', + projectItemId: 'row', + cacheKey: 'project-cache' + } + await render(item, [], projectOrigin) + await click() + await render( + { ...item, url: 'https://github.com/unrelated/widgets/issues/5' }, + [], + projectOrigin + ) + expect(selected()).toBe('candidate-user') + await render(item, ['fork-user'], { ...projectOrigin, owner: 'fork' }) + expect(selected()).toBe('fork-user') + }) + + it('still rolls back a failed edit while its issue remains open', async () => { + mocks.update.mockRejectedValue(new Error('edit failed')) + await render(item, ['original-user']) + await click() + expect(selected()).toBe('original-user') + await render(item, ['refetched-user']) + expect(selected()).toBe('refetched-user') + }) + + it('keeps a late Project-row rollback scoped to the captured row', async () => { + const projectOrigin: GitHubItemDialogProjectOrigin = { + owner: 'upstream', + repo: 'widgets', + number: 5, + type: 'issue', + projectId: 'project', + projectItemId: 'upstream-row', + cacheKey: 'upstream-cache' + } + let rejectUpdate: (reason: Error) => void = () => {} + mocks.update.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectUpdate = reject + }) + ) + await render(item, ['original-user'], projectOrigin) + await click() + await render(item, ['fork-user'], { + ...projectOrigin, + owner: 'fork', + projectItemId: 'fork-row', + cacheKey: 'fork-cache' + }) + expect(selected()).toBe('fork-user') + await act(async () => rejectUpdate(new Error('held Project edit failed'))) + expect(selected()).toBe('fork-user') + expect(mocks.patchProjectRowContent).toHaveBeenLastCalledWith( + 'upstream-cache', + 'upstream-row', + { assignees: ['original-user'] } + ) + }) + + it.each([ + { button: 'Toggle label', field: 'labels', expected: 'fork-only-label' }, + { button: 'Close issue', field: 'state', expected: 'closed' } + ])( + 'a late failed $field edit cannot overwrite the new parent selection', + async ({ button, field, expected }) => { + let rejectUpdate: (reason: Error) => void = () => {} + mocks.update.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectUpdate = reject + }) + ) + await renderParent({ ...item, labels: ['upstream-original-label'] }) + await click(button) + await renderParent({ + ...item, + state: 'closed', + url: 'https://github.com/fork/widgets/issues/5', + labels: ['fork-only-label'] + }) + expect(container.querySelector(`[data-field="${field}"]`)?.textContent).toBe(expected) + await act(async () => rejectUpdate(new Error('held edit failed'))) + expect(container.querySelector(`[data-field="${field}"]`)?.textContent).toBe(expected) + expect(mocks.patchWorkItem).toHaveBeenLastCalledWith( + item.id, + field === 'labels' ? { labels: ['upstream-original-label'] } : { state: 'open' }, + item.repoId, + { + sourceContext: undefined, + ownerRepo: { owner: 'upstream', repo: 'widgets', host: 'github.com' } + } + ) + } + ) + + it.each([ + { button: 'Toggle label', field: 'labels', expected: 'upstream-original-label' }, + { button: 'Close issue', field: 'state', expected: 'open' } + ])( + 'a same-item failed $field edit still restores its prior parent value', + async ({ button, field, expected }) => { + mocks.update.mockRejectedValue(new Error('same issue edit failed')) + await renderParent({ ...item, labels: ['upstream-original-label'] }) + await click(button) + expect(container.querySelector(`[data-field="${field}"]`)?.textContent).toBe(expected) + } + ) +}) diff --git a/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx index f92c99b2a09..a50f41fc960 100644 --- a/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx +++ b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx @@ -3,11 +3,8 @@ import { useShallow } from 'zustand/react/shallow' import { useAppStore } from '@/store' import { useRepoLabels, useRepoAssignees, useImmediateMutation } from '@/hooks/useIssueMetadata' import { useRepoLabelsBySlug, useRepoAssigneesBySlug } from '@/hooks/useGitHubSlugMetadata' -import { - getTaskSourceRuntimeSettings, - type TaskSourceContext -} from '../../../../../shared/task-source-context' -import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' +import { getTaskSourceRuntimeSettings } from '../../../../../shared/task-source-context' +import { githubRepoIdentityKey } from '../../../../../shared/github/repository-identity-key' import { getSettingsForRepoRuntimeOwner } from '@/lib/repo-runtime-owner' import { getTaskPageGitHubDuplicateCandidates, @@ -17,7 +14,8 @@ import { } from '@/components/task-page-github-status-actions' import { parseOwnerRepoFromItemUrl } from '@/components/github/github-work-item-identity' import { translate } from '@/i18n/i18n' -import type { GitHubItemDialogProjectOrigin } from '../load-item-details/github-item-dialog-types' +import type { GitHubItemDialogEditSectionProps } from '../load-item-details/github-item-dialog-types' +import { useMountedRef } from '@/hooks/useMountedRef' import { getGitHubRepositoryLabelsUrl } from './repository-labels-url' import { closeGHEditAsDuplicate, @@ -29,7 +27,14 @@ import { GHEditSectionTopColumns } from './gh-edit-section-top-columns' import { GHEditSectionHorizontal } from './gh-edit-section-horizontal' import { useGitHubDuplicateIssueCandidates } from '@/components/github/github-duplicate-issue-candidates' -export function GHEditSection({ +export function GHEditSection(props: GitHubItemDialogEditSectionProps): React.JSX.Element | null { + const { item, projectOrigin } = props + const repository = projectOrigin ?? parseOwnerRepoFromItemUrl(item.url) + const itemKey = `${item.repoId}\0${repository ? githubRepoIdentityKey(repository) : item.url}\0${item.type}\0${item.id}` + return +} + +function GHEditSectionItem({ item, repoPath, repoId, @@ -45,25 +50,7 @@ export function GHEditSection({ onOpenOrUse, attachedWorkspaceLabel, layout = 'horizontal' -}: { - item: GitHubWorkItem - repoPath: string | null - repoId: string | null - sourceContext?: TaskSourceContext | null - projectOrigin: GitHubItemDialogProjectOrigin | undefined - localState: GitHubWorkItem['state'] - localLabels: string[] - onStateChange: (state: GitHubWorkItem['state']) => void - onLabelsChange: (labels: string[]) => void - /** Why: lets the parent invalidate its details cache after a mutation, else a reopen within FRESH_MS paints pre-mutation data. */ - onMutated: () => void - assignees: string[] - onUse: (item: GitHubWorkItem) => void - onOpenOrUse?: (item: GitHubWorkItem) => void - attachedWorkspaceLabel?: string | null - /** `horizontal`: compact pill strip for the non-issue drawer/header; `top-columns`: labeled columns above the issue page body. */ - layout?: 'horizontal' | 'top-columns' -}): React.JSX.Element | null { +}: GitHubItemDialogEditSectionProps): React.JSX.Element | null { const [labelPopoverOpen, setLabelPopoverOpen] = useState(false) const [assigneePopoverOpen, setAssigneePopoverOpen] = useState(false) const [statusPopoverOpen, setStatusPopoverOpen] = useState(false) @@ -72,6 +59,7 @@ export function GHEditSection({ const [duplicateError, setDuplicateError] = useState(null) const [localAssignees, setLocalAssignees] = useState(assignees) const editedAssigneesItemKeyRef = useRef(null) + const mountedRef = useMountedRef() const assigneesItemKey = `${item.repoId}\0${item.id}` const patchWorkItem = useAppStore((s) => s.patchWorkItem) const patchProjectRowContent = useAppStore((s) => s.patchProjectRowContent) @@ -101,13 +89,18 @@ export function GHEditSection({ [projectOrigin, patchProjectRowContent] ) - // Why: with projectOrigin set, read labels/assignees from the row's repo, not the workspace path, or popovers list a different repo than writes target. + const issueRepo = useMemo(() => parseOwnerRepoFromItemUrl(item.url), [item.url]) + const metadataOptions = useMemo( + () => ({ ...sourceSettings, ownerRepo: issueRepo }), + [sourceSettings, issueRepo] + ) + // Project metadata comes from the row repository. const slugOwner = projectOrigin?.owner ?? null const slugRepo = projectOrigin?.repo ?? null const repoLabelsByPath = useRepoLabels( projectOrigin ? null : repoPath, projectOrigin ? null : repoId, - sourceSettings + metadataOptions ) const repoLabelsBySlug = useRepoLabelsBySlug( slugOwner, @@ -120,7 +113,7 @@ export function GHEditSection({ const repoAssigneesByPath = useRepoAssignees( projectOrigin ? null : repoPath, projectOrigin ? null : repoId, - sourceSettings + metadataOptions ) const repoAssigneesBySlug = useRepoAssigneesBySlug( slugOwner, @@ -187,8 +180,13 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, run, - onStateChange, + onStateChange: (state) => { + if (mountedRef.current) { + onStateChange(state) + } + }, patchWorkItem, patchProjectRowIfNeeded, onMutated @@ -202,9 +200,11 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, patchWorkItem, patchProjectRowIfNeeded, run, + mountedRef, onStateChange, onMutated ] @@ -253,8 +253,13 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, run, - onLabelsChange, + onLabelsChange: (labels) => { + if (mountedRef.current) { + onLabelsChange(labels) + } + }, patchWorkItem, patchProjectRowIfNeeded, onMutated @@ -268,9 +273,11 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, patchWorkItem, patchProjectRowIfNeeded, run, + mountedRef, onLabelsChange, onMutated ] @@ -288,6 +295,7 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, run, setLocalAssignees, patchProjectRowIfNeeded, @@ -301,6 +309,7 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, localAssignees, patchProjectRowIfNeeded, run, diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts b/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts index c0a6fc7a3b8..9c8e81953b2 100644 --- a/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts +++ b/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts @@ -26,3 +26,23 @@ export type GitHubItemDialogProps = { /** Optional Project-origin context; when set, edits route via slug-addressed IPCs against the row's repo (slug routing wins for writes). */ projectOrigin?: GitHubItemDialogProjectOrigin } + +export type GitHubItemDialogEditSectionProps = { + item: GitHubWorkItem + repoPath: string | null + repoId: string | null + sourceContext?: TaskSourceContext | null + projectOrigin: GitHubItemDialogProjectOrigin | undefined + localState: GitHubWorkItem['state'] + localLabels: string[] + onStateChange: (state: GitHubWorkItem['state']) => void + onLabelsChange: (labels: string[]) => void + /** Why: lets the parent invalidate its details cache after a mutation, else a reopen within FRESH_MS paints pre-mutation data. */ + onMutated: () => void + assignees: string[] + onUse: (item: GitHubWorkItem) => void + onOpenOrUse?: (item: GitHubWorkItem) => void + attachedWorkspaceLabel?: string | null + /** `horizontal`: compact pill strip for the non-issue drawer/header; `top-columns`: labeled columns above the issue page body. */ + layout?: 'horizontal' | 'top-columns' +} diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts b/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts index 0d685da6020..c9bd69d9bb1 100644 --- a/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts +++ b/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts @@ -4,6 +4,7 @@ import { lookupGitHubWorkItemDetailsForSource } from '@/lib/github-work-item-sou import { canUseGitHubRepoContext } from '@/lib/github-source-runtime-context' import { normalizeItemDialogTab, + parseOwnerRepoFromItemUrl, type ItemDialogTab } from '@/components/github/github-work-item-identity' import type { PRComment } from '../../../../../shared/github/comment-types' @@ -61,6 +62,13 @@ export function useGitHubItemDialogDetails({ ?.issueSourcePreference }) const canUseDetailsRepoContext = canUseGitHubRepoContext(repoPath, sourceContext) + const issueRepository = useMemo( + () => + workItem?.type === 'issue' + ? (projectOrigin ?? parseOwnerRepoFromItemUrl(workItem.url)) + : null, + [projectOrigin, workItem] + ) const detailsCacheKey = useMemo(() => { if (!workItem || !effectiveRepoId || !canUseDetailsRepoContext) { return null @@ -69,10 +77,12 @@ export function useGitHubItemDialogDetails({ repoPath: repoPath ?? '', repoId: effectiveRepoId, issueSourcePreference, + sourceContext, sourceCacheScope: sourceContext?.provider === 'github' ? getTaskSourceCacheScope(sourceContext) : null, type: workItem.type, - number: workItem.number + number: workItem.number, + ownerRepo: issueRepository }) }, [ canUseDetailsRepoContext, @@ -80,7 +90,8 @@ export function useGitHubItemDialogDetails({ effectiveRepoId, sourceContext, workItem, - issueSourcePreference + issueSourcePreference, + issueRepository ]) // Why: reset during render so an item switch never paints the previous item's tab. @@ -97,7 +108,7 @@ export function useGitHubItemDialogDetails({ } // Why: hold comments added before the detail fetch resolves so they merge into the result instead of being overwritten. - const optimisticCommentsRef = useRef([]) + const optimisticCommentsRef = useRef(new Map()) // Why: distinguish "reopen same item" from "switch item" — reopen must keep optimistic comments since gh's 60s cache omits the just-posted one. const prevItemIdRef = useRef(null) @@ -116,7 +127,7 @@ export function useGitHubItemDialogDetails({ // Why: key off cachedEntry identity (stable), not the optimistic ref array (fresh each render), to avoid needless recompute. const details = useMemo(() => { const cachedDetails = cachedEntry?.details ?? null - const opt = optimisticCommentsRef.current + const opt = optimisticCommentsRef.current.get(detailsCacheKey ?? '') ?? [] if (!cachedDetails) { // Why: on cold open, details may still be loading — surface optimistic comments via a minimal shell so a pre-fetch comment isn't invisible. if (opt.length > 0 && workItem) { @@ -138,7 +149,7 @@ export function useGitHubItemDialogDetails({ } // Why: optimisticTick forces this ref-reading memo to re-run on cold-open writes; lint can't see the dependency. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [cachedEntry, workItem, optimisticTick]) + }, [cachedEntry, workItem, detailsCacheKey, optimisticTick]) const loading = !!cachedEntry?.pending && !cachedEntry?.details const error = cachedEntry?.error && !cachedEntry?.details ? cachedEntry.error : null @@ -158,7 +169,7 @@ export function useGitHubItemDialogDetails({ } // Why: clear optimistic comments only on item switch — on reopen, gh's 60s cache omits the just-posted comment, so keep the ref to re-merge. if (workItem.id !== prevItemIdRef.current) { - optimisticCommentsRef.current = [] + optimisticCommentsRef.current.clear() } prevItemIdRef.current = workItem.id @@ -178,7 +189,8 @@ export function useGitHubItemDialogDetails({ repoId: effectiveRepoId, sourceContext, number: workItem.number, - type: workItem.type + type: workItem.type, + ownerRepo: issueRepository }) // Why: snapshot the invalidation generation; if it advances before resolve, a mid-flight mutation invalidated the entry — don't write back. @@ -201,6 +213,7 @@ export function useGitHubItemDialogDetails({ sourceContext, workItem, detailsCacheKey, + issueRepository, refetchTick ]) @@ -231,7 +244,9 @@ export function useGitHubItemDialogDetails({ (comment: PRComment) => { useAppStore.getState().recordFeatureInteraction('github-tasks') // Why: skip refreshDetails() — gh's 60s cache would overwrite the optimistic comment; next open picks up the server version. - optimisticCommentsRef.current.push(comment) + const optimisticKey = detailsCacheKey ?? '' + const optimisticComments = optimisticCommentsRef.current.get(optimisticKey) ?? [] + optimisticCommentsRef.current.set(optimisticKey, [...optimisticComments, comment]) // Why: write through the module cache so concurrent drawers re-render; mark fetchedAt stale (0) so next open refetches server fields. if (detailsCacheKey) { const prev = workItemDetailsCache.get(detailsCacheKey) diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts index 36087ef00b2..38811153a28 100644 --- a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts +++ b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts @@ -1,9 +1,13 @@ import type { PRCheckDetail } from '../../../../../shared/github/check-types' import type { GitHubAssignableUser, + GitHubOwnerRepo, GitHubPRFileViewedState } from '../../../../../shared/github/pull-request-types' +import { githubRepoIdentityKey } from '../../../../../shared/github/repository-identity-key' import type { GitHubWorkItemDetails } from '../../../../../shared/github/work-item-types' +import type { TaskSourceContext } from '../../../../../shared/task-source-context' +import { getGitHubSourceRuntimeHost } from '@/lib/github-source-runtime-context' import { onGitHubWorkItemDetailsCacheMutation } from '@/lib/github-work-item-details-cache-events' // Why: SWR cache for work-item details so reopening paints instantly instead of paying IPC + `gh` startup; keyed to avoid source/type collisions, LRU-bounded, FRESH_MS refetch on open. See docs/gh-work-item-drawer-cache.md. @@ -37,20 +41,20 @@ export function getWorkItemDetailsCacheKey(args: { repoId: string issueSourcePreference: string | undefined sourceCacheScope?: string | null + sourceContext?: TaskSourceContext | null type: 'issue' | 'pr' number: number + ownerRepo?: GitHubOwnerRepo | null }): string { // Why: key on every axis that changes which (repo, item) the IPC resolves to; `\0` separator avoids ambiguity with fields containing `:` or `/`. // Why: repoPath is the second part so match-based invalidation can find entries from a cross-window event that carries only the path. + const sourceKey = + args.ownerRepo && !getGitHubSourceRuntimeHost(args.sourceContext) + ? githubRepoIdentityKey(args.ownerRepo) + : (args.issueSourcePreference ?? 'auto') const keyParts = args.sourceCacheScope - ? [ - args.repoId, - args.repoPath, - args.sourceCacheScope, - args.issueSourcePreference ?? 'auto', - args.type - ] - : [args.repoId, args.repoPath, args.issueSourcePreference ?? 'auto', args.type] + ? [args.repoId, args.repoPath, args.sourceCacheScope, sourceKey, args.type] + : [args.repoId, args.repoPath, sourceKey, args.type] return [...keyParts, args.number].join('\0') } diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-issue-target.test.ts b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-issue-target.test.ts new file mode 100644 index 00000000000..a5c7ab456c8 --- /dev/null +++ b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-issue-target.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it, vi } from 'vitest' +import type { TaskSourceContext } from '../../../../../shared/task-source-context' +import { + getWorkItemDetailsCacheKey, + invalidateWorkItemDetailsCacheByMatch, + touchWorkItemDetailsCache, + workItemDetailsCache +} from './work-item-details-cache' + +vi.mock('@/lib/github-work-item-details-cache-events', () => ({ + onGitHubWorkItemDetailsCacheMutation: vi.fn() +})) + +const keyArgs = { + repoId: 'repo-1', + repoPath: '/home/fixture/widgets', + type: 'issue' as const, + number: 12, + issueSourcePreference: 'origin' +} +const ORIGIN = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } +const UPSTREAM = { owner: 'upstream-owner', repo: 'widgets', host: 'github.com' } +const LOCAL_SOURCE: TaskSourceContext = { + kind: 'task-source', + provider: 'github', + projectId: 'project-1', + hostId: 'local', + repoId: 'repo-1' +} + +describe('issue detail cache repository identity', () => { + it('separates equal issue numbers across origin and upstream', () => { + expect(getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: ORIGIN })).not.toBe( + getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: UPSTREAM }) + ) + }) + + it.each([undefined, LOCAL_SOURCE])( + 'keeps a local opened issue key stable when another window changes the selector: %j', + (sourceContext) => { + expect(getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: ORIGIN, sourceContext })).toBe( + getWorkItemDetailsCacheKey({ + ...keyArgs, + issueSourcePreference: 'upstream', + ownerRepo: ORIGIN, + sourceContext + }) + ) + } + ) + + it('keeps RPC detail caches scoped to the preference used by their existing lookup', () => { + const sourceContext: TaskSourceContext = { + kind: 'task-source', + provider: 'github', + projectId: 'project-1', + hostId: 'runtime:env-1', + repoId: 'runtime-repo' + } + const args = { ...keyArgs, ownerRepo: ORIGIN, sourceContext } + + expect(getWorkItemDetailsCacheKey(args)).not.toBe( + getWorkItemDetailsCacheKey({ ...args, issueSourcePreference: 'upstream' }) + ) + expect(getWorkItemDetailsCacheKey(args)).toBe( + getWorkItemDetailsCacheKey({ ...args, ownerRepo: null }) + ) + }) + + it('invalidates both repository variants after a mutation', () => { + const originKey = getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: ORIGIN }) + const upstreamKey = getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: UPSTREAM }) + touchWorkItemDetailsCache(originKey, { details: null, fetchedAt: 0 }) + touchWorkItemDetailsCache(upstreamKey, { details: null, fetchedAt: 0 }) + + invalidateWorkItemDetailsCacheByMatch(keyArgs) + + expect(workItemDetailsCache.has(originKey)).toBe(false) + expect(workItemDetailsCache.has(upstreamKey)).toBe(false) + }) +}) diff --git a/src/renderer/src/components/github/PRAssigneesPanel.tsx b/src/renderer/src/components/github/PRAssigneesPanel.tsx index d759d93880a..916c9f5d413 100644 --- a/src/renderer/src/components/github/PRAssigneesPanel.tsx +++ b/src/renderer/src/components/github/PRAssigneesPanel.tsx @@ -11,6 +11,7 @@ import { useRepoAssigneesBySlug } from '@/hooks/useGitHubSlugMetadata' import { getSettingsForRepoRuntimeOwner } from '@/lib/repo-runtime-owner' import { parseOwnerRepoFromItemUrl, + resolvePullRequestRepo, type GitHubWorkItemProjectOrigin } from '@/components/github/github-work-item-identity' import { runIssueUpdate } from '@/components/github/github-work-item-edit-mutations' @@ -82,6 +83,7 @@ export function PRAssigneesPanel({ ) const assigneeLogins = useMemo(() => localAssignees.map((user) => user.login), [localAssignees]) const assigneeSlug = useMemo(() => parseOwnerRepoFromItemUrl(item.url), [item.url]) + const prRepo = useMemo(() => resolvePullRequestRepo(item, projectOrigin), [item, projectOrigin]) const slugOwner = projectOrigin?.owner ?? assigneeSlug?.owner ?? null const slugRepo = projectOrigin?.repo ?? assigneeSlug?.repo ?? null const repoAssigneesBySlug = useRepoAssigneesBySlug( @@ -118,17 +120,24 @@ export function PRAssigneesPanel({ repoPath, sourceContext, projectOrigin, + issueRepo: prRepo, number: item.number, updates: isAssigned ? { removeAssignees: [login] } : { addAssignees: [login] } }), onOptimistic: () => { setLocalAssignees(nextAssignees) - patchWorkItem(item.id, { assignees: nextAssignees }, item.repoId, { sourceContext }) + patchWorkItem(item.id, { assignees: nextAssignees }, item.repoId, { + sourceContext, + ownerRepo: prRepo + }) patchProjectRowIfNeeded(nextLogins) }, onRevert: () => { setLocalAssignees(prevAssignees) - patchWorkItem(item.id, { assignees: prevAssignees }, item.repoId, { sourceContext }) + patchWorkItem(item.id, { assignees: prevAssignees }, item.repoId, { + sourceContext, + ownerRepo: prRepo + }) patchProjectRowIfNeeded(prevLogins) }, onSuccess: () => { @@ -147,6 +156,7 @@ export function PRAssigneesPanel({ onMutated, patchProjectRowIfNeeded, patchWorkItem, + prRepo, projectOrigin, repoPath, run, diff --git a/src/renderer/src/components/github/github-work-item-comment-mutations.ts b/src/renderer/src/components/github/github-work-item-comment-mutations.ts index f0f6fda3b82..6c7fc7e99ac 100644 --- a/src/renderer/src/components/github/github-work-item-comment-mutations.ts +++ b/src/renderer/src/components/github/github-work-item-comment-mutations.ts @@ -25,6 +25,7 @@ export function addIssueCommentForRepo(args: { repo: getGitHubRuntimeRepoId(args.sourceContext, args.repoId), number: args.number, body: args.body, + ...(args.type ? { type: args.type } : {}), prRepo: args.prRepo ?? null }, { timeoutMs: 30_000 } diff --git a/src/renderer/src/components/github/github-work-item-edit-mutations.ts b/src/renderer/src/components/github/github-work-item-edit-mutations.ts index 5c40aeb2328..543fb2395e8 100644 --- a/src/renderer/src/components/github/github-work-item-edit-mutations.ts +++ b/src/renderer/src/components/github/github-work-item-edit-mutations.ts @@ -29,19 +29,21 @@ export async function runIssueUpdate(args: { repoId?: string | null sourceContext?: TaskSourceContext | null projectOrigin: GitHubWorkItemProjectOrigin | undefined + issueRepo?: GitHubOwnerRepo | null number: number updates: Parameters[0]['updates'] }): Promise { - if (args.projectOrigin) { + const issueRepo = args.projectOrigin + if (issueRepo) { const targetSettings = args.sourceContext?.provider === 'github' ? getTaskSourceRuntimeSettings(args.sourceContext) : getGitHubMutationSettings(args.repoId) const target = getActiveRuntimeTarget(targetSettings) const updateArgs = { - owner: args.projectOrigin.owner, - repo: args.projectOrigin.repo, - host: githubProjectHost(args.projectOrigin.host), + owner: issueRepo.owner, + repo: issueRepo.repo, + host: githubProjectHost(issueRepo.host), number: args.number, updates: args.updates } @@ -59,18 +61,16 @@ export async function runIssueUpdate(args: { if (!res.ok) { throw new Error(res.error.message) } - if (target.kind === 'environment') { - notifyWorkItemDetailsMutation( - { - repoPath: args.repoPath ?? '', - repoId: args.repoId ?? undefined, - sourceContext: args.sourceContext, - type: 'issue', - number: args.number - }, - { local: false } - ) - } + notifyWorkItemDetailsMutation( + { + repoPath: args.repoPath ?? '', + repoId: args.repoId ?? undefined, + sourceContext: args.sourceContext, + type: 'issue', + number: args.number + }, + { local: target.kind !== 'environment' } + ) return } const runtimeHost = getGitHubSourceRuntimeHost(args.sourceContext) @@ -93,7 +93,8 @@ export async function runIssueUpdate(args: { repoId: args.repoId ?? undefined, sourceContext: args.sourceContext, number: args.number, - updates: args.updates + updates: args.updates, + ...(args.issueRepo ? { ownerRepo: args.issueRepo } : {}) }) if (!res.ok) { throw new Error(res.error) @@ -177,6 +178,7 @@ export async function runWorkItemBodyUpdate(args: { repoId: args.item.repoId, sourceContext: args.sourceContext, projectOrigin: args.projectOrigin, + issueRepo: args.parsedSlug, number: args.item.number, updates: { body: args.body } }) diff --git a/src/renderer/src/components/github/github-work-item-issue-target.test.ts b/src/renderer/src/components/github/github-work-item-issue-target.test.ts new file mode 100644 index 00000000000..88056a25cbb --- /dev/null +++ b/src/renderer/src/components/github/github-work-item-issue-target.test.ts @@ -0,0 +1,172 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { runIssueUpdate, runWorkItemBodyUpdate } from './github-work-item-edit-mutations' +import type { GitHubWorkItem } from '../../../../shared/github/work-item-types' +import type { TaskSourceContext } from '../../../../shared/task-source-context' +import { runGHEditLabelToggle } from '../github-item-dialog/edit-item-fields/gh-edit-section-mutations' + +vi.mock('@/store', () => ({ useAppStore: { getState: vi.fn() } })) +vi.mock('@/components/github/github-work-item-comment-mutations', () => ({ + notifyWorkItemDetailsMutation: vi.fn() +})) + +const ORIGIN = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } +const UPSTREAM = { owner: 'upstream-owner', repo: 'widgets', host: 'github.com' } +const projectOrigin = { + ...UPSTREAM, + number: 12, + type: 'issue' as const, + projectId: 'project-1', + projectItemId: 'project-item-12', + cacheKey: 'project-cache' +} +const localSource: TaskSourceContext = { + kind: 'task-source', + provider: 'github', + projectId: 'project-1', + hostId: 'local', + repoId: 'repo-1' +} +const item: GitHubWorkItem = { + id: 'issue:12', + type: 'issue', + number: 12, + title: 'Origin issue', + state: 'open', + url: 'https://github.com/fork-owner/widgets/issues/12', + labels: [], + updatedAt: '', + author: null, + repoId: 'repo-1' +} + +describe('issue edits retain the displayed repository', () => { + beforeEach(() => { + vi.stubGlobal('window', { + api: { + gh: { updateIssue: vi.fn().mockResolvedValue({ ok: true }), updateIssueBySlug: vi.fn() } + } + }) + }) + + it('keeps body edits on the registered repo path and passes the opened issue target', async () => { + await runWorkItemBodyUpdate({ + item, + repoPath: '/home/fixture/widgets', + projectOrigin: undefined, + body: 'Origin edit', + parsedSlug: ORIGIN + }) + + expect(window.api.gh.updateIssue).toHaveBeenCalledWith({ + repoId: 'repo-1', + repoPath: '/home/fixture/widgets', + sourceContext: undefined, + number: 12, + updates: { body: 'Origin edit' }, + ownerRepo: ORIGIN + }) + expect(window.api.gh.updateIssueBySlug).not.toHaveBeenCalled() + }) + + it('saves a Project row body in its upstream repository instead of the fork workspace', async () => { + vi.mocked(window.api.gh.updateIssueBySlug).mockResolvedValue({ ok: true }) + + await runWorkItemBodyUpdate({ + item: { ...item, url: 'https://github.com/upstream-owner/widgets/issues/12' }, + repoPath: '/home/fixture/fork-widgets', + sourceContext: localSource, + projectOrigin, + body: 'Project edit', + parsedSlug: UPSTREAM + }) + + expect(window.api.gh.updateIssueBySlug).toHaveBeenCalledWith({ + ...UPSTREAM, + number: 12, + updates: { body: 'Project edit' } + }) + expect(window.api.gh.updateIssue).not.toHaveBeenCalled() + }) + + it.each([ + { state: 'closed' as const }, + { addLabels: ['bug'] }, + { addAssignees: ['upstream-assignee'] } + ])('keeps Project field edits in the row repository: %j', async (updates) => { + vi.mocked(window.api.gh.updateIssueBySlug).mockResolvedValue({ ok: true }) + + await runIssueUpdate({ + repoPath: '/home/fixture/fork-widgets', + repoId: item.repoId, + sourceContext: localSource, + projectOrigin, + issueRepo: UPSTREAM, + number: 12, + updates + }) + + expect(window.api.gh.updateIssueBySlug).toHaveBeenCalledWith({ + ...UPSTREAM, + number: 12, + updates + }) + expect(window.api.gh.updateIssue).not.toHaveBeenCalled() + }) + + it.each([ + { state: 'closed' as const }, + { addLabels: ['bug'] }, + { addAssignees: ['fork-assignee'] } + ])('pins field edits to the opened issue repository: %j', async (updates) => { + await runIssueUpdate({ + repoPath: 'C:\\workspace\\widgets', + repoId: 'repo-1', + projectOrigin: undefined, + number: 12, + issueRepo: ORIGIN, + updates + }) + + expect(window.api.gh.updateIssue).toHaveBeenCalledWith( + expect.objectContaining({ + repoPath: 'C:\\workspace\\widgets', + ownerRepo: ORIGIN, + updates + }) + ) + expect(window.api.gh.updateIssueBySlug).not.toHaveBeenCalled() + }) + + it.each([ + { localLabels: [], updates: { addLabels: ['bug'] } }, + { localLabels: ['bug'], updates: { removeLabels: ['bug'] } } + ])( + 'retains the target for both label toggle directions: %j', + async ({ localLabels, updates }) => { + let mutation: Promise = Promise.resolve() + runGHEditLabelToggle({ + itemId: item.id, + itemNumber: item.number, + itemRepoId: item.repoId, + repoPath: 'C:\\workspace\\widgets', + projectOrigin: undefined, + issueRepo: ORIGIN, + label: 'bug', + localLabels, + run: async (_key, options) => { + mutation = options.mutate() + await mutation + }, + onLabelsChange: vi.fn(), + patchWorkItem: vi.fn(), + patchProjectRowIfNeeded: vi.fn(), + onMutated: vi.fn() + }) + await mutation + + expect(window.api.gh.updateIssue).toHaveBeenCalledWith( + expect.objectContaining({ ownerRepo: ORIGIN, updates }) + ) + } + ) +}) diff --git a/src/renderer/src/components/task-page-cache-selectors.test.ts b/src/renderer/src/components/task-page-cache-selectors.test.ts index 44777236862..ef1a0e20efe 100644 --- a/src/renderer/src/components/task-page-cache-selectors.test.ts +++ b/src/renderer/src/components/task-page-cache-selectors.test.ts @@ -204,8 +204,35 @@ describe('task page cache selectors', () => { } expect(findTaskPageDialogWorkItem(cache, null)).toBeNull() - expect(findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-1' })).toBe(item) - expect(findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-2' })).toBeNull() + expect( + findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-1', url: item.url }) + ).toBe(item) + expect( + findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-2', url: item.url }) + ).toBeNull() + }) + + it('keeps the clicked repository when a source change refreshes the same issue number', () => { + const origin = { + ...workItem('issue:12', 'repo-1'), + url: 'https://github.com/fork-owner/widgets/issues/12' + } + const upstream = { + ...origin, + url: 'https://github.com/upstream-owner/widgets/issues/12' + } + const clickedIssue = { id: origin.id, repoId: origin.repoId, url: origin.url } + const upstreamCache = { + upstream: entry([upstream]) + } + + expect(findTaskPageDialogWorkItem(upstreamCache, clickedIssue)).toBeNull() + expect( + findTaskPageDialogWorkItem( + { ...upstreamCache, origin: entry([origin]) }, + clickedIssue + ) + ).toBe(origin) }) it('reconciles paged table rows with patched work-item cache entries', () => { diff --git a/src/renderer/src/components/task-page-cache-selectors.ts b/src/renderer/src/components/task-page-cache-selectors.ts index a563dcae7b1..7cab1b34ece 100644 --- a/src/renderer/src/components/task-page-cache-selectors.ts +++ b/src/renderer/src/components/task-page-cache-selectors.ts @@ -28,6 +28,7 @@ export type TaskPageRepoCacheInput = { export type TaskPageDialogWorkItemKey = { id: string repoId: string + url: string } | null export type TaskPageRepoSourceState = { @@ -240,7 +241,10 @@ export function findTaskPageDialogWorkItem( for (const entry of Object.values(workItemsCache)) { const found = entry?.data?.find( - (wi) => wi.id === dialogWorkItemKey.id && wi.repoId === dialogWorkItemKey.repoId + (wi) => + wi.id === dialogWorkItemKey.id && + wi.repoId === dialogWorkItemKey.repoId && + wi.url === dialogWorkItemKey.url ) if (found) { return found diff --git a/src/renderer/src/components/task-page-github-confirmed-client-values.ts b/src/renderer/src/components/task-page-github-confirmed-client-values.ts new file mode 100644 index 00000000000..8b67b25d260 --- /dev/null +++ b/src/renderer/src/components/task-page-github-confirmed-client-values.ts @@ -0,0 +1,61 @@ +import type { GitHubOwnerRepo } from '../../../shared/github/pull-request-types' +import { githubRepoIdentityKey } from '../../../shared/github/repository-identity-key' +import { taskPageGitHubLastConfirmedKey } from './task-page-github-work-item-mutation-keys' + +const lastConfirmedClientValues = new Map< + string, + { value: unknown; ownerRepo?: GitHubOwnerRepo | null } +>() + +export function getLastConfirmedClientValue( + sourceScope: string | null, + repoId: string, + itemId: string, + family: string, + ownerRepo?: GitHubOwnerRepo | null +): unknown { + const entry = lastConfirmedClientValues.get( + taskPageGitHubLastConfirmedKey(sourceScope, repoId, itemId, family) + ) + if ( + ownerRepo !== undefined && + entry?.ownerRepo && + (!ownerRepo || githubRepoIdentityKey(ownerRepo) !== githubRepoIdentityKey(entry.ownerRepo)) + ) { + return undefined + } + return entry?.value +} +export function setLastConfirmedClientValue( + sourceScope: string | null, + repoId: string, + itemId: string, + family: string, + value: unknown, + ownerRepo?: GitHubOwnerRepo | null +): void { + lastConfirmedClientValues.set( + taskPageGitHubLastConfirmedKey(sourceScope, repoId, itemId, family), + { value, ownerRepo } + ) +} +export function deleteLastConfirmedClientValue( + sourceScope: string | null, + repoId: string, + itemId: string, + family: string, + ownerRepo?: GitHubOwnerRepo | null +): void { + if ( + ownerRepo !== undefined && + getLastConfirmedClientValue(sourceScope, repoId, itemId, family, ownerRepo) === undefined + ) { + return + } + lastConfirmedClientValues.delete( + taskPageGitHubLastConfirmedKey(sourceScope, repoId, itemId, family) + ) +} +export function clearLastConfirmedClientValues(): void { + lastConfirmedClientValues.clear() +} diff --git a/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts b/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts index b0bde81eb82..664b8bf7409 100644 --- a/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts +++ b/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts @@ -184,4 +184,69 @@ describe('dialog state authority (STA-3343)', () => { expect(superseded.revert()).toBe(false) expect(getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state')).toBe('merged') }) + + it('holds qualified state only on its canonical repository and releases it on matching search', () => { + const ownerRepo = { owner: 'o', repo: 'r', host: 'github.com' } + setTaskPageGitHubMutationQueryKey('q') + assertTaskPageGitHubDialogStateAuthority({ + repoId: 'repo-1', + itemId: 'issue:1', + state: 'closed', + ownerRepo + }) + const other = item({ url: 'https://github.com/upstream/r/issues/1' }) + const enterprise = item({ url: 'https://ghe.example/o/r/issues/1' }) + expect( + applyPendingTaskPageGitHubMutationsToItems([item(), other, enterprise]).map( + (row) => row.state + ) + ).toEqual(['closed', 'open', 'open']) + adoptQuietSearchFieldsForItem({ + item: other, + serverItem: { ...other, state: 'closed' }, + sourceScope: null, + queryKey: 'q', + fetchStartedAtGeneration: getOrCreateQuietRevalidateState('q').dirtyGeneration, + patchWorkItem: () => {} + }) + expect(getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', ownerRepo)).toBe( + 'closed' + ) + adoptQuietSearchFieldsForItem({ + item: item(), + serverItem: item({ state: 'closed' }), + sourceScope: null, + queryKey: 'q', + fetchStartedAtGeneration: getOrCreateQuietRevalidateState('q').dirtyGeneration, + patchWorkItem: () => {} + }) + expect( + getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', ownerRepo) + ).toBeUndefined() + expect(applyPendingTaskPageGitHubMutationsToItems([item()])[0]?.state).toBe('open') + }) + + it('keeps another repository confirmation when an older qualified edit rolls back', () => { + const forkRepo = { owner: 'o', repo: 'r', host: 'github.com' } + const upstreamRepo = { ...forkRepo, owner: 'upstream' } + const forkAuthority = assertTaskPageGitHubDialogStateAuthority({ + repoId: 'repo-1', + itemId: 'issue:1', + state: 'closed', + ownerRepo: forkRepo + }) + assertTaskPageGitHubDialogStateAuthority({ + repoId: 'repo-1', + itemId: 'issue:1', + state: 'closed', + ownerRepo: upstreamRepo + }) + expect(forkAuthority.revert()).toBe(false) + expect(getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', upstreamRepo)).toBe( + 'closed' + ) + expect( + getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', forkRepo) + ).toBeUndefined() + }) }) diff --git a/src/renderer/src/components/task-page-github-dialog-state-authority.ts b/src/renderer/src/components/task-page-github-dialog-state-authority.ts index 2918e1eb4d0..55bb62aedf1 100644 --- a/src/renderer/src/components/task-page-github-dialog-state-authority.ts +++ b/src/renderer/src/components/task-page-github-dialog-state-authority.ts @@ -1,4 +1,5 @@ import type { GitHubWorkItem } from '../../../shared/github/work-item-types' +import type { GitHubOwnerRepo } from '../../../shared/github/pull-request-types' import { getTaskSourceCacheScope, type TaskSourceContext @@ -32,24 +33,57 @@ export function assertTaskPageGitHubDialogStateAuthority(args: { itemId: string state: GitHubWorkItem['state'] sourceContext?: TaskSourceContext | null + ownerRepo?: GitHubOwnerRepo | null }): { revert: () => boolean } { const sourceScope = args.sourceContext?.provider === 'github' ? getTaskSourceCacheScope(args.sourceContext) : null - const previous = getLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state') - setLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state', args.state) + const previous = getLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.ownerRepo + ) + setLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.state, + args.ownerRepo + ) markStateFamilyDirty(args.repoId, args.itemId) notifyTaskPageGitHubMutationRegistry() return { revert: () => { - const current = getLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state') + const current = getLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.ownerRepo + ) // A matching search adopt or newer mutation owns the state now. if (current !== args.state) { return false } if (previous === undefined) { - deleteLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state') + deleteLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.ownerRepo + ) } else { - setLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state', previous) + setLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + previous, + args.ownerRepo + ) } markStateFamilyDirty(args.repoId, args.itemId) notifyTaskPageGitHubMutationRegistry() diff --git a/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts b/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts index 6e98c29cb9b..d7387e6cfe4 100644 --- a/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts +++ b/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts @@ -1,6 +1,7 @@ import type { ParsedTaskQuery } from '../../../shared/task-query' import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' import type { GitHubWorkItem } from '../../../shared/github/work-item-types' +import { parseGitHubIssueOrPRLink } from '../../../shared/github/links' import { recomputeTaskPageGitHubItemSoftHide, shouldSoftHideTaskPageGitHubWorkItem @@ -98,7 +99,13 @@ export function getRegistryMergedTaskPageGitHubWorkItem( // Why: after confirm, pending is cleared but search may still lag — hold the // last confirmed whole-field values until a matching adopt or newer pending. - const lastState = getLastConfirmedClientValue(sourceScope, item.repoId, item.id, 'state') + const lastState = getLastConfirmedClientValue( + sourceScope, + item.repoId, + item.id, + 'state', + parseGitHubIssueOrPRLink(item.url)?.slug ?? null + ) if (typeof lastState === 'string') { merged = { ...merged, state: lastState as GitHubWorkItem['state'] } } diff --git a/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts b/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts index b8e0ff01c83..1c50e9c547e 100644 --- a/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts +++ b/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts @@ -1,5 +1,6 @@ import type { GitHubWorkItem } from '../../../shared/github/work-item-types' import type { TaskSourceContext } from '../../../shared/task-source-context' +import { parseGitHubIssueOrPRLink } from '../../../shared/github/links' import { getRegistryMergedTaskPageGitHubWorkItem } from './task-page-github-work-item-mutation-composition' import { getStickyHideEntry, @@ -74,7 +75,10 @@ export function reapplyPendingTaskPageGitHubMutationsToCache(args: { autoMergeEnabled: merged.autoMergeEnabled }, item.repoId, - { sourceContext: args.sourceContextByRepoId?.get(item.repoId) } + { + sourceContext: args.sourceContextByRepoId?.get(item.repoId), + ownerRepo: parseGitHubIssueOrPRLink(item.url)?.slug + } ) } } diff --git a/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts b/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts index 6988ff7f7b3..9fdf7760e19 100644 --- a/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts +++ b/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts @@ -1,4 +1,17 @@ -import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' +import { + clearLastConfirmedClientValues, + deleteLastConfirmedClientValue, + getLastConfirmedClientValue, + setLastConfirmedClientValue as storeLastConfirmedClientValue +} from './task-page-github-confirmed-client-values' +export { + deleteLastConfirmedClientValue, + getLastConfirmedClientValue +} from './task-page-github-confirmed-client-values' +import type { + GitHubAssignableUser, + GitHubOwnerRepo +} from '../../../shared/github/pull-request-types' import type { PendingOp, StickyHideEntry, @@ -15,7 +28,6 @@ export type { import { serializeTaskPageGitHubMutationKey, taskPageGitHubItemKey, - taskPageGitHubLastConfirmedKey, taskPageGitHubSnapshotKey } from './task-page-github-work-item-mutation-keys' import { clearTaskPageGitHubQuietStates } from './task-page-github-work-item-quiet-state' @@ -37,7 +49,6 @@ const listeners = new Set() const pendingByKey = new Map() const generations = new Map() const confirmedSnapshots = new Map() -const lastConfirmedClientValues = new Map() /** * Why: after confirm, pending ops are gone but lastConfirmed/snapshots stay keyed * by sourceScope. Overlay must still resolve the same scope or authority is lost. @@ -79,7 +90,7 @@ export function getTaskPageGitHubConfirmedAuthorityItemKeys(): ReadonlySet, repoId?: string, - options?: { sourceContext?: TaskSourceContext | null } + options?: GitHubPatchWorkItemOptions ) => void export type BeginTaskPageGitHubWorkItemMutationArgs = { diff --git a/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts b/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts index 5547986ab2e..769948311d0 100644 --- a/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts +++ b/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts @@ -1,5 +1,6 @@ import type { GitHubWorkItem } from '../../../shared/github/work-item-types' import type { TaskSourceContext } from '../../../shared/task-source-context' +import { parseGitHubIssueOrPRLink } from '../../../shared/github/links' import { loginSetOfUsers, loginSetsEqual } from './task-page-github-work-item-mutation-patches' import { familiesFromPendingOp, @@ -52,6 +53,7 @@ export function adoptQuietSearchFieldsForItem(args: { }): { needTrailing: boolean } { const state = getOrCreateQuietRevalidateState(args.queryKey) const itemKey = taskPageGitHubItemKey(args.item.repoId, args.item.id) + const ownerRepo = parseGitHubIssueOrPRLink(args.item.url)?.slug ?? null let needTrailing = false const G0 = args.fetchStartedAtGeneration const tryFamily = ( @@ -91,7 +93,8 @@ export function adoptQuietSearchFieldsForItem(args: { 'state', () => { args.patchWorkItem(args.item.id, { state: args.serverItem.state }, args.item.repoId, { - sourceContext: args.sourceContext + sourceContext: args.sourceContext, + ownerRepo }) }, () => { @@ -99,14 +102,27 @@ export function adoptQuietSearchFieldsForItem(args: { args.sourceScope, args.item.repoId, args.item.id, - 'state' + 'state', + ownerRepo ) return last === undefined || args.serverItem.state === last }, () => - getLastConfirmedClientValue(args.sourceScope, args.item.repoId, args.item.id, 'state') !== - undefined, - () => deleteLastConfirmedClientValue(args.sourceScope, args.item.repoId, args.item.id, 'state') + getLastConfirmedClientValue( + args.sourceScope, + args.item.repoId, + args.item.id, + 'state', + ownerRepo + ) !== undefined, + () => + deleteLastConfirmedClientValue( + args.sourceScope, + args.item.repoId, + args.item.id, + 'state', + ownerRepo + ) ) tryFamily( 'autoMerge', @@ -115,7 +131,7 @@ export function adoptQuietSearchFieldsForItem(args: { args.item.id, { autoMergeEnabled: args.serverItem.autoMergeEnabled }, args.item.repoId, - { sourceContext: args.sourceContext } + { sourceContext: args.sourceContext, ownerRepo } ) }, () => { @@ -147,7 +163,7 @@ export function adoptQuietSearchFieldsForItem(args: { args.item.id, family === 'assignees' ? { assignees: serverList } : { reviewRequests: serverList }, args.item.repoId, - { sourceContext: args.sourceContext } + { sourceContext: args.sourceContext, ownerRepo } ) }, () => { diff --git a/src/renderer/src/components/use-task-page-github-detail.ts b/src/renderer/src/components/use-task-page-github-detail.ts index b2c61f64bde..655accb61d7 100644 --- a/src/renderer/src/components/use-task-page-github-detail.ts +++ b/src/renderer/src/components/use-task-page-github-detail.ts @@ -49,7 +49,8 @@ export function useTaskPageGitHubDetail(model: TaskPageGitHubListStateModel) { const dialogWorkItemKey = githubTaskDrawerWorkItem ? { id: githubTaskDrawerWorkItem.id, - repoId: githubTaskDrawerWorkItem.repoId + repoId: githubTaskDrawerWorkItem.repoId, + url: githubTaskDrawerWorkItem.url } : null const appliedWorkItemsCacheQuery = useMemo( @@ -67,7 +68,7 @@ export function useTaskPageGitHubDetail(model: TaskPageGitHubListStateModel) { ) ) - // Why: derive the dialog item from the cache for optimistic patches, falling back to the click-time snapshot for new stubs; key by repoId so same-number issues across repos resolve to the clicked row. + // Keep cache patches in the clicked conversation when origin and upstream share an issue number. const cachedDialogWorkItem = useAppStore((s) => findTaskPageDialogWorkItem(s.workItemsCache, dialogWorkItemKey) ) diff --git a/src/renderer/src/hooks/useGitHubRepoMetadata.ts b/src/renderer/src/hooks/useGitHubRepoMetadata.ts new file mode 100644 index 00000000000..8cdb20376bc --- /dev/null +++ b/src/renderer/src/hooks/useGitHubRepoMetadata.ts @@ -0,0 +1,95 @@ +import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' +import type { + GitHubAssignableUser, + GitHubOwnerRepo +} from '../../../shared/github/pull-request-types' +import { githubRepoIdentityKey } from '../../../shared/github/repository-identity-key' +import { createMetadataRequestStore } from './metadata-request-cache' +import { useMetadataListRequest, type MetadataListState } from './useMetadataListRequest' + +type GitHubMetadataOptions = { + runtimeEnvironmentId?: string | null + activeRuntimeEnvironmentId?: string | null + ownerRepo?: GitHubOwnerRepo | null +} + +const ghLabelStore = createMetadataRequestStore() +const ghAssigneeStore = createMetadataRequestStore() + +export function useRepoLabels( + repoPath: string | null, + repoId?: string | null, + options?: GitHubMetadataOptions +): MetadataListState { + const runtimeEnvironmentId = + options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null + const repoSelector = repoId ?? repoPath ?? '' + const ownerRepo = runtimeEnvironmentId ? null : options?.ownerRepo + const repositoryKey = ownerRepo + ? `${repoSelector}::${githubRepoIdentityKey(ownerRepo)}` + : repoSelector + const cacheKey = + repoPath || repoId + ? runtimeEnvironmentId + ? `runtime:${runtimeEnvironmentId}:${repoSelector}` + : repositoryKey + : null + + return useMetadataListRequest({ + cacheKey, + store: ghLabelStore, + errorFallback: 'Failed to load labels', + load: () => + runtimeEnvironmentId + ? callRuntimeRpc( + { kind: 'environment', environmentId: runtimeEnvironmentId }, + 'github.listLabels', + { repo: repoSelector }, + { timeoutMs: 15_000 } + ) + : window.api.gh.listLabels({ + repoPath: repoPath ?? '', + repoId: repoId ?? undefined, + ...(ownerRepo ? { ownerRepo } : {}) + }) + }) +} + +export function useRepoAssignees( + repoPath: string | null, + repoId?: string | null, + options?: GitHubMetadataOptions +): MetadataListState { + const runtimeEnvironmentId = + options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null + const repoSelector = repoId ?? repoPath ?? '' + const ownerRepo = runtimeEnvironmentId ? null : options?.ownerRepo + const repositoryKey = ownerRepo + ? `${repoSelector}::${githubRepoIdentityKey(ownerRepo)}` + : repoSelector + const cacheKey = + repoPath || repoId + ? runtimeEnvironmentId + ? `runtime:${runtimeEnvironmentId}:${repoSelector}` + : repositoryKey + : null + + return useMetadataListRequest({ + cacheKey, + store: ghAssigneeStore, + errorFallback: 'Failed to load assignees', + load: () => + runtimeEnvironmentId + ? callRuntimeRpc( + { kind: 'environment', environmentId: runtimeEnvironmentId }, + 'github.listAssignableUsers', + { repo: repoSelector }, + { timeoutMs: 15_000 } + ) + : window.api.gh.listAssignableUsers({ + repoPath: repoPath ?? '', + repoId: repoId ?? undefined, + ...(ownerRepo ? { ownerRepo } : {}) + }) + }) +} diff --git a/src/renderer/src/hooks/useIssueMetadata.test.tsx b/src/renderer/src/hooks/useIssueMetadata.test.tsx index bff14b1c091..f586e03fada 100644 --- a/src/renderer/src/hooks/useIssueMetadata.test.tsx +++ b/src/renderer/src/hooks/useIssueMetadata.test.tsx @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { clearLinearMetadataCache, useRepoLabels, + useRepoAssignees, useTeamStates, useTeamsStates } from './useIssueMetadata' @@ -17,7 +18,7 @@ const linearMocks = vi.hoisted(() => ({ })) const runtimeMocks = vi.hoisted(() => ({ callRuntimeRpc: vi.fn() })) -const githubMocks = vi.hoisted(() => ({ listLabels: vi.fn() })) +const githubMocks = vi.hoisted(() => ({ listLabels: vi.fn(), listAssignableUsers: vi.fn() })) vi.mock('@/runtime/runtime-linear-project-client', () => ({ linearTeamStates: linearMocks.linearTeamStates, @@ -38,7 +39,12 @@ const roots: Root[] = [] function installWindowApi(): void { Object.defineProperty(window, 'api', { configurable: true, - value: { gh: { listLabels: githubMocks.listLabels } } + value: { + gh: { + listLabels: githubMocks.listLabels, + listAssignableUsers: githubMocks.listAssignableUsers + } + } }) } @@ -67,6 +73,7 @@ describe('useIssueMetadata hooks', () => { linearMocks.linearTeamMembers.mockReset() runtimeMocks.callRuntimeRpc.mockReset() githubMocks.listLabels.mockReset() + githubMocks.listAssignableUsers.mockReset() installWindowApi() }) @@ -97,6 +104,70 @@ describe('useIssueMetadata hooks', () => { expect(runtimeMocks.callRuntimeRpc).not.toHaveBeenCalled() }) + it('keeps concurrent fork and upstream label lists separate when the fork reply is late', async () => { + const fork = { owner: 'fork', repo: 'widgets', host: 'github.com' } + const upstream = { owner: 'upstream', repo: 'widgets', host: 'github.com' } + let finishFork: (labels: string[]) => void = () => {} + const forkReply = new Promise((resolve) => { + finishFork = resolve + }) + githubMocks.listLabels.mockImplementation(({ ownerRepo }) => + ownerRepo.owner === 'fork' ? forkReply : Promise.resolve(['upstream-label']) + ) + let forkLabels: string[] = [] + let upstreamLabels: string[] = [] + function Probe(): null { + forkLabels = useRepoLabels(null, 'concurrent-folder', { ownerRepo: fork }).data + upstreamLabels = useRepoLabels(null, 'concurrent-folder', { ownerRepo: upstream }).data + return null + } + renderProbe() + await flushEffects() + expect(forkLabels).toEqual([]) + expect(upstreamLabels).toEqual(['upstream-label']) + await act(async () => { + finishFork(['fork-label']) + }) + await flushEffects() + expect(forkLabels).toEqual(['fork-label']) + expect(upstreamLabels).toEqual(['upstream-label']) + expect(githubMocks.listLabels).toHaveBeenCalledTimes(2) + }) + + it('ignores a late fork assignee reply after the opened issue switches to upstream', async () => { + const fork = { owner: 'fork', repo: 'widgets', host: 'github.com' } + const upstream = { owner: 'upstream', repo: 'widgets', host: 'github.com' } + let finishFork: (users: { login: string; name: null; avatarUrl: string }[]) => void = () => {} + const forkReply = new Promise<{ login: string; name: null; avatarUrl: string }[]>((resolve) => { + finishFork = resolve + }) + githubMocks.listAssignableUsers.mockImplementation(({ ownerRepo }) => + ownerRepo.owner === 'fork' + ? forkReply + : Promise.resolve([{ login: 'upstream-user', name: null, avatarUrl: '' }]) + ) + let logins: string[] = [] + function Probe({ ownerRepo }: { ownerRepo: typeof fork }): null { + logins = useRepoAssignees(null, 'switch-folder', { ownerRepo }).data.map((user) => user.login) + return null + } + renderProbe() + await flushEffects() + const root = roots.at(-1) + if (!root) { + throw new Error('Expected the rendered probe') + } + act(() => root.render()) + await flushEffects() + expect(logins).toEqual(['upstream-user']) + await act(async () => { + finishFork([{ login: 'fork-user', name: null, avatarUrl: '' }]) + }) + await flushEffects() + expect(logins).toEqual(['upstream-user']) + expect(githubMocks.listAssignableUsers).toHaveBeenCalledTimes(2) + }) + it('prefers an explicit remote environment and repo id', async () => { let labels: string[] = [] runtimeMocks.callRuntimeRpc.mockResolvedValue(['remote']) diff --git a/src/renderer/src/hooks/useIssueMetadata.ts b/src/renderer/src/hooks/useIssueMetadata.ts index a7e55b80700..2ce65c3055f 100644 --- a/src/renderer/src/hooks/useIssueMetadata.ts +++ b/src/renderer/src/hooks/useIssueMetadata.ts @@ -1,12 +1,11 @@ import { useEffect, useMemo, useRef, useState } from 'react' -import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' +import { getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' import { linearTeamLabels, linearTeamMembers, linearTeamStates } from '@/runtime/runtime-linear-project-client' import type { RuntimeLinearSettings } from '@/runtime/runtime-linear-client' -import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' import type { LinearLabel, LinearMember, @@ -23,79 +22,7 @@ import { } from './metadata-request-cache' import { useMetadataListRequest, type MetadataListState } from './useMetadataListRequest' -type GitHubMetadataOptions = { - runtimeEnvironmentId?: string | null - activeRuntimeEnvironmentId?: string | null -} - -const ghLabelStore = createMetadataRequestStore() -const ghAssigneeStore = createMetadataRequestStore() - -export function useRepoLabels( - repoPath: string | null, - repoId?: string | null, - options?: GitHubMetadataOptions -): MetadataListState { - const runtimeEnvironmentId = - options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null - const repoSelector = repoId ?? repoPath ?? '' - const cacheKey = - repoPath || repoId - ? runtimeEnvironmentId - ? `runtime:${runtimeEnvironmentId}:${repoSelector}` - : repoSelector - : null - - return useMetadataListRequest({ - cacheKey, - store: ghLabelStore, - errorFallback: 'Failed to load labels', - load: () => - runtimeEnvironmentId - ? callRuntimeRpc( - { kind: 'environment', environmentId: runtimeEnvironmentId }, - 'github.listLabels', - { repo: repoSelector }, - { timeoutMs: 15_000 } - ) - : window.api.gh - .listLabels({ repoPath: repoPath ?? '', repoId: repoId ?? undefined }) - .then((labels) => labels as string[]) - }) -} - -export function useRepoAssignees( - repoPath: string | null, - repoId?: string | null, - options?: GitHubMetadataOptions -): MetadataListState { - const runtimeEnvironmentId = - options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null - const repoSelector = repoId ?? repoPath ?? '' - const cacheKey = - repoPath || repoId - ? runtimeEnvironmentId - ? `runtime:${runtimeEnvironmentId}:${repoSelector}` - : repoSelector - : null - - return useMetadataListRequest({ - cacheKey, - store: ghAssigneeStore, - errorFallback: 'Failed to load assignees', - load: () => - runtimeEnvironmentId - ? callRuntimeRpc( - { kind: 'environment', environmentId: runtimeEnvironmentId }, - 'github.listAssignableUsers', - { repo: repoSelector }, - { timeoutMs: 15_000 } - ) - : window.api.gh - .listAssignableUsers({ repoPath: repoPath ?? '', repoId: repoId ?? undefined }) - .then((users) => users as GitHubAssignableUser[]) - }) -} +export { useRepoLabels, useRepoAssignees } from './useGitHubRepoMetadata' const linearStateStore = createMetadataRequestStore() const linearLabelStore = createMetadataRequestStore() diff --git a/src/renderer/src/lib/github-work-item-source-lookup.test.ts b/src/renderer/src/lib/github-work-item-source-lookup.test.ts index 336583b1d6d..432401ce3e5 100644 --- a/src/renderer/src/lib/github-work-item-source-lookup.test.ts +++ b/src/renderer/src/lib/github-work-item-source-lookup.test.ts @@ -41,7 +41,8 @@ describe('GitHub source lookup routing', () => { repoId: 'renderer-repo', sourceContext: runtimeSourceContext, number: 42, - type: 'issue' + type: 'issue', + ownerRepo: { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } }) ).resolves.toBeNull() @@ -100,4 +101,22 @@ describe('GitHub source lookup routing', () => { }) expect(callRuntimeRpc).not.toHaveBeenCalled() }) + + it('sends a local issue row repository to the repo-scoped details route', async () => { + const ownerRepo = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } + vi.mocked(window.api.gh.workItemDetails).mockResolvedValue(null) + + await lookupGitHubWorkItemDetailsForSource({ + repoPath: '/home/fixture/widgets', + repoId: 'local-repo', + number: 12, + type: 'issue', + ownerRepo + }) + + expect(window.api.gh.workItemDetails).toHaveBeenCalledWith( + expect.objectContaining({ repoPath: '/home/fixture/widgets', ownerRepo }) + ) + expect(callRuntimeRpc).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/src/lib/github-work-item-source-lookup.ts b/src/renderer/src/lib/github-work-item-source-lookup.ts index 5a49e15ba83..da9e3775767 100644 --- a/src/renderer/src/lib/github-work-item-source-lookup.ts +++ b/src/renderer/src/lib/github-work-item-source-lookup.ts @@ -1,5 +1,6 @@ import type { GitHubWorkItem, GitHubWorkItemDetails } from '../../../shared/github/work-item-types' import type { TaskSourceContext } from '../../../shared/task-source-context' +import type { GitHubOwnerRepo } from '../../../shared/github/pull-request-types' import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import { getGitHubRuntimeRepoId, @@ -28,6 +29,7 @@ type GitHubWorkItemDetailsLookupArgs = { sourceContext?: TaskSourceContext | null number: number type: 'issue' | 'pr' + ownerRepo?: GitHubOwnerRepo | null } function runtimeRepoId(args: Pick): string { @@ -112,6 +114,7 @@ export function lookupGitHubWorkItemDetailsForSource( repoId: args.repoId, sourceContext, number: args.number, - type: args.type + type: args.type, + ...(args.ownerRepo ? { ownerRepo: args.ownerRepo } : {}) }) } diff --git a/src/renderer/src/store/github/cache-model.ts b/src/renderer/src/store/github/cache-model.ts index 27db1bf8167..d1546ff4768 100644 --- a/src/renderer/src/store/github/cache-model.ts +++ b/src/renderer/src/store/github/cache-model.ts @@ -61,4 +61,5 @@ export type ProjectRowContentPatch = { export type GitHubPatchWorkItemOptions = { sourceContext?: TaskSourceContext | null + ownerRepo?: GitHubOwnerRepo | null } diff --git a/src/renderer/src/store/github/work-item-mutation-actions.ts b/src/renderer/src/store/github/work-item-mutation-actions.ts index 97a6d3b66f3..fde46aef606 100644 --- a/src/renderer/src/store/github/work-item-mutation-actions.ts +++ b/src/renderer/src/store/github/work-item-mutation-actions.ts @@ -3,6 +3,8 @@ import type { AppState } from '../types' import type { GitHubSlice } from './slice-types' import { toast } from 'sonner' import type { GitHubWorkItem } from '../../../../shared/github/work-item-types' +import { parseGitHubIssueOrPRLink } from '../../../../shared/github/links' +import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' import { getTaskSourceCacheScope } from '../../../../shared/task-source-context' import { translate } from '@/i18n/i18n' import { getSettingsForRepoRuntimeOwner } from '@/lib/repo-runtime-owner' @@ -24,6 +26,7 @@ export const createWorkItemMutationActions = ( options?.sourceContext?.provider === 'github' ? getTaskSourceCacheScope(options.sourceContext) : null + const repositoryKey = options?.ownerRepo ? githubRepoIdentityKey(options.ownerRepo) : null for (const key of Object.keys(nextCache)) { // Why: don't patch another host/account's visually identical issue/PR cache entry. if (sourceScope && key !== sourceScope && !key.startsWith(`${sourceScope}::`)) { @@ -34,9 +37,18 @@ export const createWorkItemMutationActions = ( continue } // Why: issue/PR ids are only unique within a repo; cross-repo views can share `pr:42`. - const idx = entry.data.findIndex( - (item) => item.id === itemId && (!repoId || item.repoId === repoId) - ) + const idx = entry.data.findIndex((item) => { + if (item.id !== itemId || (repoId && item.repoId !== repoId)) { + return false + } + if (!repositoryKey) { + return true + } + const itemRepository = parseGitHubIssueOrPRLink(item.url)?.slug + return ( + itemRepository !== undefined && githubRepoIdentityKey(itemRepository) === repositoryKey + ) + }) if (idx === -1) { continue } diff --git a/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts b/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts index 77f997083aa..6fe2a9e068a 100644 --- a/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts +++ b/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts @@ -111,6 +111,43 @@ describe('createGitHubSlice.patchWorkItem', () => { }) expect(secondPatched).toBe(secondItem) }) + + it('scopes a canonical repository patch within its existing host and account scope', () => { + const store = createTestStore() + const source = githubSourceContext('local', 'repo-1') + const otherAccount = { ...source, projectHostSetupId: 'another-account-setup' } + const canonical: GitHubWorkItem = { + id: 'issue:42', + repoId: 'repo-1', + type: 'issue', + number: 42, + title: 'Fork issue', + state: 'open', + labels: [], + updatedAt: '', + author: null, + url: 'https://github.com/fork/widgets/issues/42' + } + const upstream = { ...canonical, url: 'https://github.com/upstream/widgets/issues/42' } + const enterprise = { ...canonical, url: 'https://ghe.example:8443/fork/widgets/issues/42' } + const sourceKey = workItemsCacheKey('repo-1', 20, '', getTaskSourceCacheScope(source)) + const otherKey = workItemsCacheKey('repo-1', 20, '', getTaskSourceCacheScope(otherAccount)) + store.setState({ + workItemsCache: { + [sourceKey]: { data: [upstream, enterprise, canonical], fetchedAt: 1 }, + [otherKey]: { data: [canonical], fetchedAt: 1 } + } + }) + store.getState().patchWorkItem(canonical.id, { labels: ['bug'] }, canonical.repoId, { + sourceContext: source, + ownerRepo: { owner: 'FORK', repo: 'Widgets', host: ' GitHub.com ' } + }) + const rows = store.getState().workItemsCache[sourceKey]?.data + expect(rows?.[0]).toBe(upstream) + expect(rows?.[1]).toBe(enterprise) + expect(rows?.[2].labels).toEqual(['bug']) + expect(store.getState().workItemsCache[otherKey]?.data?.[0]).toBe(canonical) + }) }) describe('createGitHubSlice.fetchWorkItems cache identity', () => {