From 1183ea6e8d67e0de6e4f3ebbdf0fec7dffca0690 Mon Sep 17 00:00:00 2001 From: Graeme Folk Date: Fri, 8 May 2026 12:31:17 -0600 Subject: [PATCH] fix(review): resolve jj default target from trunk Resolve the default JJ compare target from trunk() so review UI and diff behavior use the same concrete bookmark when JJ can identify one. --- packages/server/jj.test.ts | 35 ++++++++--------- packages/shared/jj-core.test.ts | 49 +++++++++++++++--------- packages/shared/jj-core.ts | 66 ++++++++++++++++++++++++--------- 3 files changed, 99 insertions(+), 51 deletions(-) diff --git a/packages/server/jj.test.ts b/packages/server/jj.test.ts index 957435ec0..183a074f0 100644 --- a/packages/server/jj.test.ts +++ b/packages/server/jj.test.ts @@ -41,23 +41,24 @@ describe("jj diff args", () => { }); describe("jj compare targets", () => { - test("prefers available default bookmarks before falling back to trunk", () => { - expect(selectDefaultJjCompareTarget({ - local: ["main"], - remote: ["main@origin"], - })).toBe("main@origin"); - expect(selectDefaultJjCompareTarget({ - local: ["main"], - remote: ["main@git"], - })).toBe("main@git"); - expect(selectDefaultJjCompareTarget({ - local: ["main"], - remote: [], - })).toBe("main"); - expect(selectDefaultJjCompareTarget({ - local: ["release"], - remote: ["production@git"], - })).toBe("trunk()"); + test("resolves default target from jj trunk bookmarks", async () => { + await expect(selectDefaultJjCompareTarget({ + async runJj() { + return { stdout: '[{"name":"main"},{"name":"main","remote":"origin"}]\n', stderr: "", exitCode: 0 }; + }, + })).resolves.toBe("main@origin"); + + await expect(selectDefaultJjCompareTarget({ + async runJj() { + return { stdout: '[{"name":"main"}]\n', stderr: "", exitCode: 0 }; + }, + })).resolves.toBe("main"); + + await expect(selectDefaultJjCompareTarget({ + async runJj() { + return { stdout: "[]\n", stderr: "", exitCode: 0 }; + }, + })).resolves.toBe("trunk()"); }); test("treats bookmarks and revsets correctly in line-of-work revsets", () => { diff --git a/packages/shared/jj-core.test.ts b/packages/shared/jj-core.test.ts index 4f6808d1b..3515bc1be 100644 --- a/packages/shared/jj-core.test.ts +++ b/packages/shared/jj-core.test.ts @@ -84,23 +84,38 @@ describe("jj diff args", () => { }); describe("jj compare targets", () => { - test("prefers available default bookmarks before falling back to trunk", () => { - expect(selectDefaultJjCompareTarget({ - local: ["main"], - remote: ["main@origin"], - })).toBe("main@origin"); - expect(selectDefaultJjCompareTarget({ - local: ["main"], - remote: ["main@git"], - })).toBe("main@git"); - expect(selectDefaultJjCompareTarget({ - local: ["main"], - remote: [], - })).toBe("main"); - expect(selectDefaultJjCompareTarget({ - local: ["release"], - remote: ["production@git"], - })).toBe("trunk()"); + test("resolves default target from jj trunk remote bookmarks", async () => { + const calls: string[][] = []; + const runtime: ReviewJjRuntime = { + async runJj(args) { + calls.push(args); + return { stdout: '[{"name":"main"},{"name":"main","remote":"origin"}]\n', stderr: "", exitCode: 0 }; + }, + }; + + await expect(selectDefaultJjCompareTarget(runtime, "/repo")) + .resolves.toBe("main@origin"); + expect(calls).toEqual([[ + "log", + "--no-graph", + "-r", + "trunk()", + "-T", + "json(bookmarks)", + ]]); + }); + + test("falls back to local bookmark then trunk revset", async () => { + const runtimeFor = (stdout: string): ReviewJjRuntime => ({ + async runJj() { + return { stdout, stderr: "", exitCode: 0 }; + }, + }); + + await expect(selectDefaultJjCompareTarget(runtimeFor('[{"name":"develop"}]\n'))) + .resolves.toBe("develop"); + await expect(selectDefaultJjCompareTarget(runtimeFor('[]\n'))) + .resolves.toBe("trunk()"); }); test("treats bookmarks and revsets correctly in line-of-work revsets", () => { diff --git a/packages/shared/jj-core.ts b/packages/shared/jj-core.ts index c4bb8ca9b..21c6e2132 100644 --- a/packages/shared/jj-core.ts +++ b/packages/shared/jj-core.ts @@ -38,7 +38,7 @@ export async function getJjContext( ): Promise { const root = await detectJjWorkspace(runtime, cwd); const targets = await listJjCompareTargets(runtime, root ?? cwd); - const defaultTarget = selectDefaultJjCompareTarget(targets); + const defaultTarget = await selectDefaultJjCompareTarget(runtime, root ?? cwd); const contextCwd = root ?? cwd; return { @@ -178,23 +178,55 @@ export function getJjDiffArgs( } } -export function selectDefaultJjCompareTarget(targets: { local: string[]; remote: string[] }): string { - const preferredNames = ["main", "develop", "master", "trunk"]; - for (const name of preferredNames) { - const preferredRemote = [ - `${name}@origin`, - `${name}@upstream`, - `${name}@git`, - ...targets.remote.filter((target) => target.startsWith(`${name}@`)).sort(), - ].find((target) => targets.remote.includes(target)); - if (preferredRemote) return preferredRemote; - - if (targets.local.includes(name)) return name; - } +export async function selectDefaultJjCompareTarget( + runtime: ReviewJjRuntime, + cwd?: string, +): Promise { + const result = await runtime.runJj([ + "log", + "--no-graph", + "-r", + JJ_TRUNK_REVSET, + "-T", + "json(bookmarks)", + ], { cwd }); + if (result.exitCode !== 0) return JJ_TRUNK_REVSET; + + return parseJjResolvedBookmarks(result.stdout)[0] ?? JJ_TRUNK_REVSET; +} + +function parseJjResolvedBookmarks(value: string): string[] { + try { + const parsed = JSON.parse(value); + if (!Array.isArray(parsed)) return []; + + const local: string[] = []; + const remote: string[] = []; - // `trunk()` honors JJ's repository default bookmark/remote and user aliases - // when the repository has no obvious default bookmark to compare against. - return JJ_TRUNK_REVSET; + for (const bookmark of parsed) { + if (typeof bookmark === "string") { + local.push(bookmark); + continue; + } + + if (!bookmark || typeof bookmark !== "object") continue; + + const name = typeof bookmark.name === "string" ? bookmark.name : null; + if (!name) continue; + + const remoteName = typeof bookmark.remote === "string" ? bookmark.remote : null; + if (remoteName) { + remote.push(`${name}@${remoteName}`); + continue; + } + + local.push(name); + } + + return [...remote, ...local]; + } catch { + return []; + } } async function listJjCompareTargets(