From 0b8e2150e870e659b08c4b13801187e552a30e83 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:36:08 -0400 Subject: [PATCH] fix(web): pull requests listed by two environments show as one row Two connected environments pointed at the same remote each list the same pull request, and the page concatenated the listings, so every row and the sidebar's needs-you count doubled. Rows are now identified by host, repository and number. The merge keeps one row per pull request, seated with the first environment to list it (this device first), except that a listing whose project holds the repository takes over a row this device only found by searching the viewer's own work. Thread linking, the thread badge lookup and selection matching no longer require the same environment, so a thread on the other computer still claims the row, and the loaded-entries hook merges across environments too so the Project filter offers each project once. --- .../pull-requests/pullRequests.logic.test.ts | 92 ++++++++++++++- .../pull-requests/pullRequests.logic.ts | 108 +++++++++++------- apps/web/src/lib/pullRequestsReactQuery.ts | 34 +++--- 3 files changed, 176 insertions(+), 58 deletions(-) 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, + ), }); }