Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 5 additions & 1 deletion apps/mobile/src/features/threads/git/GitOverviewSheet.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,11 @@ function GitOverviewSheetContent(props: GitOverviewSheetProps) {
</View>
) : null}
{chain.layers.map((link, index) => (
<View key={threadPullRequestKeyOf(link)}>
<View
key={threadPullRequestKeyOf(link)}
// Steps in per layer like the web list, capped so deep stacks keep their width.
style={{ paddingLeft: Math.min(chain.depths[index]!, 3) * 16 }}
>
{index > 0 && Platform.OS !== "android" ? (
<View className="ml-12 h-px bg-border" />
) : null}
Expand Down
16 changes: 16 additions & 0 deletions apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,22 @@ describe("listThreadPullRequests", () => {
]);
expect(result.chains).toEqual([{ kind: "native", numbers: [1, 2] }]);
});

it("gives pull requests built on the same one the same position", () => {
const result = listThreadPullRequests({
pullRequests: [
makeLink(1, { headBranch: "feat-a", baseBranch: "main" }),
makeLink(2, { headBranch: "feat-b", baseBranch: "feat-a" }),
makeLink(3, { headBranch: "feat-c", baseBranch: "feat-a" }),
],
});
expect(result.pullRequests.map((entry) => [entry.number, entry.stack])).toEqual([
[1, { kind: "derived", position: 1, size: 3 }],
[2, { kind: "derived", position: 2, size: 3 }],
[3, { kind: "derived", position: 2, size: 3 }],
]);
expect(result.chains).toEqual([{ kind: "derived", numbers: [1, 2, 3] }]);
});
});

describe("pull request guide tools", () => {
Expand Down
2 changes: 1 addition & 1 deletion apps/server/src/mcp/toolkits/pullRequests/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ function entryOf(
if (chain.layers.length < 2) continue;
const index = chain.layers.findIndex((layer) => threadPullRequestKeyOf(layer) === key);
if (index !== -1) {
stack = { kind: chain.kind, position: index + 1, size: chain.layers.length };
stack = { kind: chain.kind, position: chain.depths[index]! + 1, size: chain.layers.length };
break;
}
}
Expand Down
4 changes: 2 additions & 2 deletions apps/server/src/mcp/toolkits/pullRequests/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ export const ThreadPullRequestEntry = Schema.Struct({
stack: Schema.NullOr(
Schema.Struct({
kind: Schema.Literals(["native", "derived"]),
/** 1-based, bottom of the stack first. */
/** 1-based layer, bottom of the stack first. Pull requests built on the same one share it. */
position: Schema.Int,
size: Schema.Int,
}),
Expand All @@ -268,7 +268,7 @@ export const ListThreadPullRequestsResult = Schema.Struct({
chains: Schema.Array(
Schema.Struct({
kind: Schema.Literals(["native", "derived"]),
/** Bottom to top. */
/** Bottom to top; each pull request comes before the ones built on it. */
numbers: Schema.Array(Schema.Int),
}),
),
Expand Down
9 changes: 5 additions & 4 deletions apps/web/src/components/pullRequest/pullRequestListLines.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { ThreadPullRequestChain } from "@t3tools/shared/threadPullRequests"
/** One line of a thread's pull-request list: a link plus how deep it sits in its stack. */
export interface PullRequestListLine {
readonly link: ThreadPullRequestLink;
/** 0 for a pull request on the base branch; each layer above steps in by one. */
/** 0 for a pull request on the base branch; each layer above steps in by one. Pull requests
* built on the same parent share a depth. */
readonly depth: number;
/** Which chain the line belongs to, so callers can tell one stack's lines from another's. */
readonly chainKey: string;
Expand Down Expand Up @@ -36,12 +37,12 @@ export function pullRequestListLines(
);
return ordered.flatMap((chain) => {
const chainKey = chainKeyOf(chain);
return chain.layers.map((link, depth) => ({
return chain.layers.map((link, index) => ({
link,
depth,
depth: chain.depths[index]!,
chainKey,
stack:
depth === 0 && chain.layers.length > 1
index === 0 && chain.layers.length > 1
? { kind: chain.kind, size: chain.layers.length }
: null,
}));
Expand Down
55 changes: 55 additions & 0 deletions packages/shared/src/threadPullRequests.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,51 @@ describe("resolveThreadPullRequestChains", () => {
]);
});

it("keeps every link built on the same parent under it", () => {
const links = [
link(3, { snapshot: snapshot({ headBranch: "c", baseBranch: "a" }) }),
link(5, { snapshot: snapshot({ headBranch: "p", baseBranch: "main" }) }),
link(6, { snapshot: snapshot({ headBranch: "q", baseBranch: "p" }) }),
link(1, { snapshot: snapshot({ headBranch: "a", baseBranch: "p" }) }),
link(2, { snapshot: snapshot({ headBranch: "b", baseBranch: "a" }) }),
link(9, { snapshot: snapshot({ headBranch: "solo", baseBranch: "main" }) }),
];
const chains = resolveThreadPullRequestChains(links);
expect(
chains.map((chain) => [chain.layers.map((layer) => layer.number), chain.depths]),
).toEqual([
// #6 and #1 both build on #5; #3 and #2 both build on #1.
[
[5, 6, 1, 3, 2],
[0, 1, 1, 2, 2],
],
[[9], [0]],
]);
expect(resolveThreadPullRequestBadge(links.slice(0, 5))).toEqual({
kind: "stack",
layers: 5,
state: "open",
});
});

it("points at the highest layer of a branched stack, not the last sibling listed", () => {
// #1 → #2 → #3, and #4 also builds on #1.
const branched = (state: ThreadPullRequestSnapshot["state"]) => [
link(1, { snapshot: snapshot({ state, headBranch: "a" }) }),
link(2, { snapshot: snapshot({ state, headBranch: "b", baseBranch: "a" }) }),
link(3, { snapshot: snapshot({ state, headBranch: "c", baseBranch: "b" }) }),
link(4, { snapshot: snapshot({ state, headBranch: "d", baseBranch: "a" }) }),
];
const open = resolveThreadCurrentPullRequest(branched("open"));
expect(open?.kind === "stack" ? open.open.map((entry) => entry.number) : null).toEqual([
3, 4, 2, 1,
]);
expect(resolveThreadCurrentPullRequest(branched("merged"))).toMatchObject({
kind: "single",
link: { number: 3 },
});
});

it("uses the native stack order when the host provides one", () => {
const stack = {
kind: "native" as const,
Expand Down Expand Up @@ -432,6 +477,16 @@ describe("chain selection and badge state", () => {
});
});

it("lists a pull request once when two links share its identity", () => {
const links = [
link(1, { snapshot: snapshot({ headBranch: "a" }) }),
link(1, { snapshot: snapshot({ headBranch: "b", baseBranch: "a" }) }),
];
expect(resolveThreadPullRequestChains(links).map((chain) => chain.layers)).toEqual([
[links[0]],
]);
});

it("does not guess a parent when a head branch was reused", () => {
const links = [
link(1, { snapshot: snapshot({ state: "merged", headBranch: "reused" }) }),
Expand Down
77 changes: 52 additions & 25 deletions packages/shared/src/threadPullRequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,16 @@ export type ThreadCurrentPullRequest =
readonly top: ThreadPullRequestLink;
};

/** A chain's layers from the highest down. Siblings keep the reverse of their listed order. */
function layersFromTop(chain: ThreadPullRequestChain): Array<ThreadPullRequestLink> {
// `.sort()` on a fresh array, not `.toSorted()`: this runs on Hermes, which has no ES2023
// array methods, and a TypeError here is fatal on every mobile launch that renders a stack.
return chain.layers
.map((link, index) => ({ link, index, depth: chain.depths[index]! }))
.sort((left, right) => right.depth - left.depth || right.index - left.index)
.map((entry) => entry.link);
}

/**
* Prefer open work and the highest open layer within a chain. A completed chain still
* points at its top; unrelated terminal links use the most recently updated request.
Expand All @@ -120,10 +130,8 @@ export function resolveThreadCurrentPullRequest(
if (open.length === 1) return { kind: "single", link: open[0]! };
const chains = resolveThreadPullRequestChains(visible);
if (open.length > 1) {
// `.reverse()` on a copy, not `.toReversed()`: this runs on Hermes, which has no ES2023
// array methods, and a TypeError here is fatal on every mobile launch that renders a stack.
const openChains = chains
.map((chain) => [...chain.layers].reverse().filter(isOpen))
.map((chain) => layersFromTop(chain).filter(isOpen))
.filter((layers) => layers.length > 0)
.sort(
(left, right) =>
Expand All @@ -134,7 +142,7 @@ export function resolveThreadCurrentPullRequest(
return { kind: "stack", open: ordered, top: ordered[0]! };
}
if (chains.length === 1) {
return { kind: "single", link: chains[0]!.layers.at(-1)! };
return { kind: "single", link: layersFromTop(chains[0]!)[0]! };
}
const terminal = [...visible].sort(
(left, right) => latestUpdatedAt(right) - latestUpdatedAt(left),
Expand Down Expand Up @@ -204,14 +212,17 @@ export function legacyLinkedPullRequestOf(

export interface ThreadPullRequestChain {
readonly kind: "native" | "derived";
/** Bottom to top. */
/** Bottom to top. Each link comes before the links built on it. */
readonly layers: ReadonlyArray<ThreadPullRequestLink>;
/** One per layer: 0 for the base, parent's depth + 1 above it. Siblings share a depth. */
readonly depths: ReadonlyArray<number>;
}

/**
* Groups a thread's links into stacks. Native stacks come from the host and win; the rest
* are chained by matching one link's base branch to another's head branch within the same
* repository. A link that chains to nothing is a one-layer chain.
* repository. Several links can build on one parent, so a derived stack is a tree, listed
* depth first. A link that chains to nothing is a one-layer chain.
*/
export function resolveThreadPullRequestChains(
links: ReadonlyArray<ThreadPullRequestLink>,
Expand All @@ -233,7 +244,7 @@ export function resolveThreadPullRequestChains(
const order = new Map(members[0]!.stack!.layers.map((layer, index) => [layer.number, index]));
members.sort((left, right) => (order.get(left.number) ?? 0) - (order.get(right.number) ?? 0));
for (const member of members) placed.add(threadPullRequestKeyOf(member));
chains.push({ kind: "native", layers: members });
chains.push({ kind: "native", layers: members, depths: members.map((_, index) => index) });
}

const remaining = visible.filter((link) => !placed.has(threadPullRequestKeyOf(link)));
Expand All @@ -248,31 +259,47 @@ export function resolveThreadPullRequestChains(
const key = branchKey(link, link.snapshot.headBranch);
byHead.set(key, byHead.has(key) ? null : link);
}
const hasChild = new Set<string>();
for (const link of remaining) {
if (link.snapshot === null) continue;
const parentOf = (link: ThreadPullRequestLink) => {
if (link.snapshot === null) return undefined;
const parent = byHead.get(branchKey(link, link.snapshot.baseBranch));
if (parent != null && parent !== link) hasChild.add(threadPullRequestKeyOf(parent));
return parent == null || parent === link ? undefined : parent;
};
const children = new Map<string, Array<ThreadPullRequestLink>>();
for (const link of remaining) {
const parent = parentOf(link);
if (parent === undefined) continue;
const key = threadPullRequestKeyOf(parent);
const siblings = children.get(key) ?? [];
siblings.push(link);
children.set(key, siblings);
}
// Walk from each top (a link nothing builds on) down its base chain.
for (const top of remaining) {
if (hasChild.has(threadPullRequestKeyOf(top))) continue;
// Walk each base (a link that builds on no other link) depth first, so every link a
// parent has sits under it instead of only the first one.
for (const base of remaining) {
if (parentOf(base) !== undefined) continue;
const layers: Array<ThreadPullRequestLink> = [];
let cursor: ThreadPullRequestLink | undefined = top;
while (cursor !== undefined && !placed.has(threadPullRequestKeyOf(cursor))) {
placed.add(threadPullRequestKeyOf(cursor));
layers.unshift(cursor);
cursor =
cursor.snapshot === null
? undefined
: (byHead.get(branchKey(cursor, cursor.snapshot.baseBranch)) ?? undefined);
const depths: Array<number> = [];
const pending: Array<{ readonly link: ThreadPullRequestLink; readonly depth: number }> = [
{ link: base, depth: 0 },
];
for (let next = pending.pop(); next !== undefined; next = pending.pop()) {
const key = threadPullRequestKeyOf(next.link);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Two links with one identity would otherwise share a child list and expand forever.
if (placed.has(key)) continue;
placed.add(key);
layers.push(next.link);
depths.push(next.depth);
const below = children.get(key) ?? [];
for (let index = below.length - 1; index >= 0; index -= 1) {
pending.push({ link: below[index]!, depth: next.depth + 1 });
}
}
if (layers.length > 0) chains.push({ kind: "derived", layers });
chains.push({ kind: "derived", layers, depths });
}
// Cycles have no top. Keep those links visible without inventing a stack order.
// Cycles have no base. Keep those links visible without inventing a stack order.
for (const link of remaining) {
if (!placed.has(threadPullRequestKeyOf(link))) {
chains.push({ kind: "derived", layers: [link] });
chains.push({ kind: "derived", layers: [link], depths: [0] });
}
}
return chains;
Expand Down
Loading