Skip to content

fix(web): pull requests listed by two environments show as one row #218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
badcuban merged 1 commit into from
Sep 5, 2026
Merged

fix(web): pull requests listed by two environments show as one row #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 90 additions & 2 deletions apps/web/src/components/pull-requests/pullRequests.logic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -26,6 +27,7 @@ import {
linkThreadsToPullRequests,
matchesPullRequestQuery,
matchesPullRequestSelection,
mergePullRequestListResults,
narrowPullRequests,
parsePullRequestSelection,
parsePullRequestsSearch,
Expand Down Expand Up @@ -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],
Expand All @@ -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,
);
Expand All @@ -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> = {}): VcsStatusResult {
return {
isRepo: true,
Expand Down Expand Up @@ -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();
Expand Down
108 changes: 68 additions & 40 deletions apps/web/src/components/pull-requests/pullRequests.logic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -312,29 +314,35 @@ 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. */
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.
Expand All @@ -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;
Expand All @@ -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.
*/
Expand All @@ -379,7 +402,7 @@ export function mergePullRequestListResults(input: {
readonly failures: readonly PullRequestProjectFailure[];
readonly viewer: string | null;
} {
const entries: PullRequestEntry[] = [];
const entries = new Map<string, PullRequestEntry>();
const failures: PullRequestProjectFailure[] = [];
let viewer: string | null = null;

Expand All @@ -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({
Expand All @@ -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
Expand All @@ -431,9 +460,9 @@ export function linkThreadsToPullRequests(

const repositoryByProject = new Map<string, string>();
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) => {
Expand All @@ -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]
: [],
);
Expand Down Expand Up @@ -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;
}
Expand All @@ -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,
);
}
Expand Down
34 changes: 18 additions & 16 deletions apps/web/src/lib/pullRequestsReactQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
});
}

Expand Down
Loading