diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index 90509e7e..7a19ad01 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -4,6 +4,7 @@ import { ThreadId, type PullRequestComment, type PullRequestListEntry, + type PullRequestListResult, type VcsStatusResult, } from "@threadlines/contracts"; import { describe, expect, it } from "vite-plus/test"; @@ -26,6 +27,7 @@ import { linkThreadsToPullRequests, matchesPullRequestQuery, matchesPullRequestSelection, + mergePullRequestListResults, narrowPullRequests, parsePullRequestSelection, parsePullRequestsSearch, @@ -359,7 +361,28 @@ describe("linkThreadsToPullRequests", () => { expect(linked.has(pullRequestEntryKey(row))).toBe(false); }); - it("ignores archived threads, other branches, other repositories, and other environments", () => { + it("links a thread on another computer whose project points at the same repository", () => { + // The merged list keeps one row per pull request, so the row this device + // listed stands for the work a thread on the laptop is doing on it. + const row = entry(); + const linked = linkThreadsToPullRequests( + [row], + [ + thread({ + id: ThreadId.make("laptop-thread"), + environmentId: OTHER_ENVIRONMENT_ID, + projectId: OTHER_PROJECT_ID, + }), + ], + PROJECTS, + ); + + expect(linked.get(pullRequestEntryKey(row))?.map((match) => match.id)).toEqual([ + "laptop-thread", + ]); + }); + + it("ignores archived threads, other branches, and other repositories", () => { const row = entry(); const linked = linkThreadsToPullRequests( [row], @@ -368,7 +391,6 @@ describe("linkThreadsToPullRequests", () => { thread({ id: ThreadId.make("other-branch"), branch: "main" }), thread({ id: ThreadId.make("no-branch"), branch: null }), thread({ id: ThreadId.make("other-repository"), projectId: OTHER_PROJECT_ID }), - thread({ id: ThreadId.make("other-env"), environmentId: OTHER_ENVIRONMENT_ID }), ], PROJECTS, ); @@ -377,6 +399,61 @@ describe("linkThreadsToPullRequests", () => { }); }); +describe("mergePullRequestListResults", () => { + const listing = ( + environmentId: EnvironmentId, + entries: readonly PullRequestListEntry[], + ): { environmentId: EnvironmentId; environmentLabel: string; data: PullRequestListResult } => ({ + environmentId, + environmentLabel: environmentId === ENVIRONMENT_ID ? "This device" : "Laptop", + data: { viewer: "ada", entries, errors: [] }, + }); + + it("keeps one row for a pull request two environments both list, seated with the first", () => { + const local = entry({ number: 214 }); + const remote = entry({ number: 214, projectId: OTHER_PROJECT_ID }); + const laptopOnly = entry({ + number: 9, + repository: "someone/else", + projectId: OTHER_PROJECT_ID, + }); + + const merged = mergePullRequestListResults({ + state: "open", + results: [ + listing(ENVIRONMENT_ID, [local]), + listing(OTHER_ENVIRONMENT_ID, [remote, laptopOnly]), + ], + }); + + expect(merged.entries.map((row) => [row.number, row.environmentId])).toEqual([ + [214, ENVIRONMENT_ID], + [9, OTHER_ENVIRONMENT_ID], + ]); + }); + + it("lets a listing that holds the repository take a row this device only found by search", () => { + const searched = entry({ number: 538, repository: "someone/else", origin: "authored" }); + const checkedOut = entry({ + number: 538, + repository: "Someone/Else", + projectId: OTHER_PROJECT_ID, + }); + + const merged = mergePullRequestListResults({ + state: "open", + results: [listing(ENVIRONMENT_ID, [searched]), listing(OTHER_ENVIRONMENT_ID, [checkedOut])], + }); + + expect(merged.entries).toHaveLength(1); + expect(merged.entries[0]).toMatchObject({ + origin: "workspace", + environmentId: OTHER_ENVIRONMENT_ID, + projectId: OTHER_PROJECT_ID, + }); + }); +}); + function gitStatus(overrides: Partial = {}): VcsStatusResult { return { isRepo: true, @@ -781,6 +858,17 @@ describe("the pull request a link names", () => { ).toBe(false); }); + it("still marks the row after another environment takes over listing it", () => { + const selection = { + environmentId: OTHER_ENVIRONMENT_ID, + projectId: OTHER_PROJECT_ID, + repository: "threadlines/threadlines", + number: 7, + }; + + expect(matchesPullRequestSelection(selection, entry({ number: 7 }))).toBe(true); + }); + it("drops a value that names no pull request", () => { expect(parsePullRequestSelection("")).toBeNull(); expect(parsePullRequestSelection(`${ENVIRONMENT_ID}:${PROJECT_ID}`)).toBeNull(); diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index a3008fe0..00c07ab4 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -299,9 +299,11 @@ function decodeRepositorySegment(value: string): string | null { } /** - * Whether a row is the one the route names. A link written before the param - * carried a repository names none, and then the project and the number are all - * there is to go on. + * Whether a row is the one the route names. The repository and the number say + * which pull request; the environment in the link only says which computer + * reads it, and the row keeps matching after another one takes that seat. A + * link written before the param carried a repository names none, and then the + * project and the number are all there is to go on. */ export function matchesPullRequestSelection( selection: PullRequestSelection, @@ -312,22 +314,23 @@ export function matchesPullRequestSelection( readonly number: number; }, ): boolean { - return ( - selection.environmentId === entry.environmentId && - selection.projectId === entry.projectId && - selection.number === entry.number && - (selection.repository === null || - repositoryKey(selection.repository) === repositoryKey(entry.repository)) - ); + if (selection.number !== entry.number) { + return false; + } + return selection.repository === null + ? selection.environmentId === entry.environmentId && selection.projectId === entry.projectId + : repositoryKey(selection.repository) === repositoryKey(entry.repository); } /** - * Identifies one row across environments and repositories. The project is - * deliberately left out: a checkout and its worktrees are separate projects on - * one remote, and the pull request is the same one from any of them. + * Identifies one pull request whichever environment or checkout listed it. The + * project is deliberately left out: a checkout and its worktrees are separate + * projects on one remote, and the pull request is the same one from any of + * them. So is the environment: two computers pointed at one remote read the + * same pull request, and {@link mergePullRequestListResults} keeps one row. */ export function pullRequestEntryKey(entry: PullRequestEntry): string { - return `${entry.environmentId}:${repositoryKey(entry.repository)}:${entry.number}`; + return `${repositoryScopeKey(entry.provider, entry.repository)}:${entry.number}`; } /** Repository names are case-insensitive on every host here, so comparisons are too. */ @@ -335,6 +338,11 @@ function repositoryKey(repository: string): string { return repository.toLowerCase(); } +/** One repository on one host: `owner/name` means something else on another host. */ +function repositoryScopeKey(provider: SourceControlProviderKind, repository: string): string { + return `${provider}:${repositoryKey(repository)}`; +} + /** * The repository name the server lists this project under, or null when its * remote is not one we can read pull requests from. @@ -348,6 +356,13 @@ export function projectProviderKind(project: Project): SourceControlProviderKind return toChangeRequestProviderKind(project.repositoryIdentity?.provider); } +/** The project's remote as {@link repositoryScopeKey} spells it, or null when it has none we can read. */ +function projectRepositoryScope(project: Project): string | null { + const provider = projectProviderKind(project); + const repository = projectRepository(project); + return provider === null || repository === null ? null : repositoryScopeKey(provider, repository); +} + function updatedAtMs(value: string): number { const parsed = Date.parse(value); return Number.isNaN(parsed) ? 0 : parsed; @@ -364,6 +379,14 @@ function threadActivityMs(thread: SidebarThreadSummary): number { /** * One listing per environment, merged into the page's row list. * + * Two environments pointed at one remote both list the same pull request, and + * the page keeps one row for it. The first listing to carry it keeps the seat, + * and the results arrive with this device first, so a pull request is read + * and acted on from here whenever here can. The one exception is a row this + * device only found by searching the viewer's own work: a listing whose + * project actually holds the repository can check the branch out and knows + * the threads working it, so that row takes over. + * * The state filter is defensive: a host can answer a "closed" listing with a * merged row, and a row under the wrong tab reads as a bug in the page. */ @@ -379,7 +402,7 @@ export function mergePullRequestListResults(input: { readonly failures: readonly PullRequestProjectFailure[]; readonly viewer: string | null; } { - const entries: PullRequestEntry[] = []; + const entries = new Map(); const failures: PullRequestProjectFailure[] = []; let viewer: string | null = null; @@ -388,11 +411,16 @@ export function mergePullRequestListResults(input: { viewer ??= result.data.viewer; for (const entry of result.data.entries) { if (entry.state !== input.state) continue; - entries.push({ + const scoped = { ...entry, environmentId: result.environmentId, environmentLabel: result.environmentLabel, - }); + }; + const key = pullRequestEntryKey(scoped); + const seated = entries.get(key); + if (seated === undefined || (seated.origin === "authored" && entry.origin !== "authored")) { + entries.set(key, scoped); + } } for (const failure of result.data.errors) { failures.push({ @@ -403,17 +431,18 @@ export function mergePullRequestListResults(input: { } } - return { entries, failures, viewer }; + return { entries: [...entries.values()], failures, viewer }; } /** * The threads working each pull request, keyed by {@link pullRequestEntryKey}. * - * A thread counts when its project points at the pull request's repository on - * the same environment and it is checked out on the head branch. Matching by - * repository rather than project lets a thread in a worktree project claim the - * row its sibling checkout produced. Archived threads are past work, so they - * never claim a row. + * A thread counts when its project points at the pull request's repository and + * it is checked out on the head branch. Matching by repository rather than + * project lets a thread in a worktree project claim the row its sibling + * checkout produced, and a thread on another computer claim the row this one + * listed: the row stands for the pull request, not for the listing that found + * it. Archived threads are past work, so they never claim a row. * * An authored row is on a repository no project here points at, and the project * it names is only the checkout its host tool runs in, so no thread is working @@ -431,9 +460,9 @@ export function linkThreadsToPullRequests( const repositoryByProject = new Map(); for (const project of projects) { - const repository = projectRepository(project); - if (repository !== null) { - repositoryByProject.set(`${project.environmentId}:${project.id}`, repositoryKey(repository)); + const scope = projectRepositoryScope(project); + if (scope !== null) { + repositoryByProject.set(`${project.environmentId}:${project.id}`, scope); } } const candidates = threads.flatMap((thread) => { @@ -445,11 +474,9 @@ export function linkThreadsToPullRequests( }); for (const entry of entries) { if (entry.origin === "authored") continue; - const entryRepository = repositoryKey(entry.repository); + const entryRepository = repositoryScopeKey(entry.provider, entry.repository); const matches = candidates.flatMap((candidate) => - candidate.thread.environmentId === entry.environmentId && - candidate.repository === entryRepository && - candidate.thread.branch === entry.headBranch + candidate.repository === entryRepository && candidate.thread.branch === entry.headBranch ? [candidate.thread] : [], ); @@ -580,13 +607,14 @@ export function resolveThreadPullRequest(input: { if (fromStatus) { return fromStatus; } - if (repository === null) { + const scope = projectRepositoryScope(project); + if (scope === null) { return null; } const entry = - findThreadListEntry(thread, repository, input.openEntries) ?? - findThreadListEntry(thread, repository, input.settledEntries ?? []); + findThreadListEntry(thread, scope, input.openEntries) ?? + findThreadListEntry(thread, scope, input.settledEntries ?? []); if (!entry) { return null; } @@ -601,21 +629,21 @@ export function resolveThreadPullRequest(input: { } /** - * The listing row for a thread's branch on its own repository and computer. An - * authored row is on a repository no project here points at, so it is never a - * thread's own work however its branch happens to be spelled. + * The listing row for a thread's branch on its own repository, whichever + * computer listed it: the merged list keeps one row per pull request, and it + * may sit with another environment than the thread's. An authored row is on a + * repository no project here points at, so it is never a thread's own work + * however its branch happens to be spelled. */ function findThreadListEntry( thread: ThreadPullRequestSubject, - repository: string, + scope: string, entries: readonly PullRequestEntry[], ): PullRequestEntry | undefined { - const entryRepository = repositoryKey(repository); return entries.find( (candidate) => candidate.origin !== "authored" && - candidate.environmentId === thread.environmentId && - repositoryKey(candidate.repository) === entryRepository && + repositoryScopeKey(candidate.provider, candidate.repository) === scope && candidate.headBranch === thread.branch, ); } diff --git a/apps/web/src/lib/pullRequestsReactQuery.ts b/apps/web/src/lib/pullRequestsReactQuery.ts index b617d507..4b519115 100644 --- a/apps/web/src/lib/pullRequestsReactQuery.ts +++ b/apps/web/src/lib/pullRequestsReactQuery.ts @@ -399,23 +399,25 @@ export function useLoadedPullRequestEntries(): readonly PullRequestEntry[] { enabled: false, })), ), + // Merged per state across every environment, the same way the page reads + // its own tab, so a pull request two computers both list is one row here + // too and the Project filter does not offer it twice. combine: (results) => - results.flatMap((result, index) => { - const environment = environments[Math.floor(index / PULL_REQUEST_LIST_STATES.length)]; - const state = PULL_REQUEST_LIST_STATES[index % PULL_REQUEST_LIST_STATES.length]; - return environment && state && !result.isPlaceholderData - ? mergePullRequestListResults({ - state, - results: [ - { - environmentId: environment.environmentId, - environmentLabel: environment.label, - data: result.data, - }, - ], - }).entries - : []; - }), + PULL_REQUEST_LIST_STATES.flatMap( + (state, stateIndex) => + mergePullRequestListResults({ + state, + results: environments.map((environment, environmentIndex) => { + const result = + results[environmentIndex * PULL_REQUEST_LIST_STATES.length + stateIndex]; + return { + environmentId: environment.environmentId, + environmentLabel: environment.label, + data: result && !result.isPlaceholderData ? result.data : undefined, + }; + }), + }).entries, + ), }); }