From 447269a65f4b0858b42394bcb88af449befdb860 Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 19 Oct 2026 17:35:24 +0200 Subject: [PATCH 01/11] fix(GenesisHub, prs): a PR link opens any project checked out under the repo roots, by its project name, even before the hub's project index loads --- src/hub/lib/pr/find.ts | 12 +-- src/hub/lib/pr/pr.test.ts | 64 ++++++++++++- src/hub/lib/pr/project-ref.ts | 92 +++++++++++++++++++ src/hub/lib/prs.ts | 8 +- .../Sources/Parts/Hub/Hub/HubPRs.swift | 52 ++++++----- .../GenesisTools/Tests/HubLogicTests.swift | 13 ++- 6 files changed, 208 insertions(+), 33 deletions(-) create mode 100644 src/hub/lib/pr/project-ref.ts diff --git a/src/hub/lib/pr/find.ts b/src/hub/lib/pr/find.ts index bce1d1b9f4..de0cd1c77c 100644 --- a/src/hub/lib/pr/find.ts +++ b/src/hub/lib/pr/find.ts @@ -5,7 +5,6 @@ import { type PrSummary, parseGhPrRows, parseGlabMrRows, - parsePrRef, parsePrUrl, projectRefFromRemote, spawnRunner, @@ -15,6 +14,7 @@ import { GH_LIST_FIELDS } from "@genesiscz/utils/git/origins/prs"; import { type RepoFacts, repoFacts } from "@genesiscz/utils/git/repo-facts"; import { SafeJSON } from "@genesiscz/utils/json"; import { logger } from "@genesiscz/utils/logger"; +import { resolveHubPrRef } from "./project-ref"; import { type FindResult, type FoundPr, HubPrError } from "./types"; const log = logger.child({ component: "hub/pr/find" }); @@ -280,10 +280,10 @@ export async function findPrByRef({ runner?: CommandRunner; readFacts?: FactsReader; }): Promise { - const parsed = parsePrRef(ref); + const parsed = await resolveHubPrRef(ref); - if (!parsed) { - throw new HubPrError("bad-input", `--pr takes a PR/MR URL, # or , got "${ref}"`); + if ("problem" in parsed) { + throw new HubPrError("bad-input", `--pr: ${parsed.problem}`); } let project: ProjectRef | null; @@ -344,9 +344,9 @@ export async function explicitPrTarget({ ref: string; readFacts?: FactsReader; }): Promise { - const parsed = parsePrRef(ref); + const parsed = await resolveHubPrRef(ref); - if (!parsed) { + if ("problem" in parsed) { return null; } diff --git a/src/hub/lib/pr/pr.test.ts b/src/hub/lib/pr/pr.test.ts index 1ec3484147..3651712f63 100644 --- a/src/hub/lib/pr/pr.test.ts +++ b/src/hub/lib/pr/pr.test.ts @@ -4,6 +4,7 @@ import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { defaultReviewCommentClient, type ReviewCommentClient } from "@app/github/lib/review-comments"; +import type { LocalCheckout } from "@genesiscz/utils/git/local-checkouts"; import type { CommandRunner } from "@genesiscz/utils/git/origins"; import type { RepoFacts } from "@genesiscz/utils/git/repo-facts"; import { SafeJSON } from "@genesiscz/utils/json"; @@ -13,6 +14,7 @@ import { findBranchPr, findPrByRef } from "./find"; import { githubBackend, githubThread, githubVersions } from "./github"; import { gitlabBackend, gitlabPosition, gitlabThreads, gitlabVersions } from "./gitlab"; import { type FoundPr, forgetThreads, gitlabBaseUrl, type PrBackend, prThreads, readPrThreads } from "./index"; +import { resolveHubPrRef } from "./project-ref"; // ─── find ───────────────────────────────────────────────────────────────────── @@ -222,7 +224,9 @@ describe("findPrByRef", () => { repoPath: null, }); - await expect(findPrByRef({ ref: "not a ref", runner: refuseRunner })).rejects.toThrow("--pr takes"); + await expect(findPrByRef({ ref: "not a ref", runner: refuseRunner })).rejects.toThrow( + "--pr: expected a PR/MR URL" + ); }); test("a PR from a fork says so, and names the fork, for `tools hub pr find --json` and the window", async () => { @@ -1289,3 +1293,61 @@ describe("PR versions", () => { expect(githubVersions({ headSha: "h9", pushes: [] }).map((version) => version.id)).toEqual(["head"]); }); }); + +// ─── project refs ───────────────────────────────────────────────────────────── + +describe("resolveHubPrRef", () => { + const checkout = (root: string, host: string, path: string, isMain = true): LocalCheckout => ({ + root, + commonDir: `${root}/.git`, + branch: "main", + isMain, + remoteUrl: `https://${host}/${path}.git`, + project: { host, path }, + }); + const source = async () => ({ + roots: ["/work"], + checkouts: [ + checkout("/work/app-wt", "git.example.com", "group/sub/app", false), + checkout("/work/app", "git.example.com", "group/sub/app"), + checkout("/work/web", "github.com", "acme/web"), + checkout("/work/one/tool", "github.com", "acme/tool"), + checkout("/work/two/tool", "git.example.com", "team/tool"), + ], + }); + + test("a GitLab project path with ! finds its main checkout, whatever the letter case or host prefix", async () => { + expect(await resolveHubPrRef("group/sub/app!12", { source })).toEqual({ path: "/work/app", number: 12 }); + expect(await resolveHubPrRef("Group/Sub/App!12", { source })).toEqual({ path: "/work/app", number: 12 }); + expect(await resolveHubPrRef("git.example.com/group/sub/app!12", { source })).toEqual({ + path: "/work/app", + number: 12, + }); + expect(await resolveHubPrRef("app#12", { source })).toEqual({ path: "/work/app", number: 12 }); + }); + + test("a checkout wins over the github.com guess, which stays for a project with none", async () => { + expect(await resolveHubPrRef("acme/web#5", { source })).toEqual({ path: "/work/web", number: 5 }); + expect(await resolveHubPrRef("acme/other#5", { source })).toEqual({ + url: "https://github.com/acme/other/pull/5", + }); + }); + + test("an unknown merge request or a name of two projects is a problem that says why", async () => { + const missing = await resolveHubPrRef("group/none!3", { source }); + expect("problem" in missing && missing.problem).toContain( + "no checkout of group/none under the repo roots (/work)" + ); + const twice = await resolveHubPrRef("tool!3", { source }); + expect("problem" in twice && twice.problem).toContain("names 2 projects"); + }); + + test("a URL never scans the checkouts", async () => { + const never = async () => { + throw new Error("scanned"); + }; + expect(await resolveHubPrRef("https://github.com/acme/web/pull/5", { source: never })).toEqual({ + url: "https://github.com/acme/web/pull/5", + }); + }); +}); diff --git a/src/hub/lib/pr/project-ref.ts b/src/hub/lib/pr/project-ref.ts new file mode 100644 index 0000000000..8f652a2aa7 --- /dev/null +++ b/src/hub/lib/pr/project-ref.ts @@ -0,0 +1,92 @@ +import { existsSync } from "node:fs"; +import { loadConfig } from "@app/browser-extension/lib/config"; +import { configuredCheckouts } from "@app/browser-extension/lib/deps"; +import { type LocalCheckout, rankCheckouts } from "@genesiscz/utils/git/local-checkouts"; +import { parsePrRef } from "@genesiscz/utils/git/origins"; +import { logger } from "@genesiscz/utils/logger"; + +const log = logger.child({ component: "hub/pr/project-ref" }); + +export type HubPrRef = { url: string } | { path: string; number: number }; + +export interface CheckoutSource { + roots: string[]; + checkouts: LocalCheckout[]; +} + +async function liveCheckouts(): Promise { + const config = await loadConfig(); + return { roots: config.repoRoots, checkouts: configuredCheckouts(config) }; +} + +/** + * A PR/MR ref the hub can open, with a project named by its web path looked up among every checkout + * under the repo roots: `group/sub/app!12` (GitLab's form), `owner/repo#42`, `gitlab.example/group/app!12`, + * or only `app!12` when one project has that name. A URL, a folder and a bare number read as `parsePrRef` + * reads them. With no checkout, `owner/repo#42` is still a github.com PR; anything else is a `problem`. + */ +export async function resolveHubPrRef( + ref: string, + { source = liveCheckouts }: { source?: () => Promise } = {} +): Promise { + const trimmed = ref.trim(); + const named = /^([^\s#!]+)([#!])(\d+)$/.exec(trimmed); + + if (!named || /^https?:\/\//i.test(trimmed) || /^[~./]/.test(named[1]) || existsSync(named[1])) { + return ( + parsePrRef(trimmed) ?? { + problem: `expected a PR/MR URL, # or !, got "${ref}"`, + } + ); + } + + const [, name, sign, digits] = named; + const { roots, checkouts } = await source(); + const found = checkoutOf(name, checkouts); + + if ("root" in found) { + log.debug({ ref, root: found.root }, "hub pr: project ref resolved to a checkout"); + return { path: found.root, number: Number(digits) }; + } + + if (found.projects.length > 1) { + return { + problem: `${name} names ${found.projects.length} projects (${found.projects.join(", ")}); use the whole path`, + }; + } + + const github = sign === "#" ? parsePrRef(trimmed) : null; + + if (github && "url" in github) { + log.debug({ ref, url: github.url }, "hub pr: no checkout of the project, read as a github.com PR"); + return github; + } + + log.debug({ ref, roots, checkouts: checkouts.length }, "hub pr: no checkout of the project"); + return { + problem: `no checkout of ${name} under the repo roots (${roots.join(", ")}); open it by its page URL, or clone it there`, + }; +} + +/** + * The best checkout of the one project `name` names: its whole web path (with or without the host), else + * the end of one (`daf-fe` for `group/daf/daf-fe`). Letter case does not matter, as on the forges. + */ +function checkoutOf(name: string, checkouts: LocalCheckout[]): { root: string } | { projects: string[] } { + const wanted = name + .toLowerCase() + .replace(/\.git$/, "") + .replace(/^\/+|\/+$/g, ""); + const key = (checkout: LocalCheckout) => `${checkout.project.host}/${checkout.project.path}`; + const exact = checkouts.filter((checkout) => checkout.project.path === wanted || key(checkout) === wanted); + const pool = + exact.length > 0 ? exact : checkouts.filter((checkout) => checkout.project.path.endsWith(`/${wanted}`)); + const projects = [...new Set(pool.map(key))]; + + if (projects.length !== 1) { + return { projects }; + } + + const best = rankCheckouts({ project: pool[0].project, checkouts: pool })[0]; + return best ? { root: best.root } : { projects }; +} diff --git a/src/hub/lib/prs.ts b/src/hub/lib/prs.ts index b08c08cb5f..17994ff494 100644 --- a/src/hub/lib/prs.ts +++ b/src/hub/lib/prs.ts @@ -13,7 +13,6 @@ import { type ProjectRef, type PrQuery, type PrSummary, - parsePrRef, parsePrUrl, projectRefFromRemote, spawnRunner, @@ -31,6 +30,7 @@ import { logger } from "@genesiscz/utils/logger"; import { profiler } from "@genesiscz/utils/profile"; import { cached } from "@genesiscz/utils/storage/cache-flag"; import { Storage } from "@genesiscz/utils/storage/storage"; +import { resolveHubPrRef } from "./pr/project-ref"; import { type ProposalSummary, proposalFor } from "./proposal"; /** The hub's PR row: the host's PR plus where it lives on this machine. */ @@ -523,10 +523,10 @@ export async function hubPr({ runner?: CommandRunner; storage?: Pick; }): Promise { - const parsedRef = parsePrRef(ref); + const parsedRef = await prof.measureAsync("show.ref", () => resolveHubPrRef(ref)); - if (!parsedRef) { - throw new PrRefError(`expected a PR/MR URL or #, got "${ref}"`); + if ("problem" in parsedRef) { + throw new PrRefError(parsedRef.problem); } let project: ProjectRef; diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift index bf7b9e1fc6..092bdc4f6e 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift @@ -85,13 +85,13 @@ struct HubPRRef: Equatable { /// `group/app!7`: a GitLab merge request, whose page needs a host only its checkout knows. var isMergeRequest = false - /// What `tools hub pr show` can fetch with no local checkout: the page, else a GitHub - /// `owner/repo` PR. Never for a merge request: a github.com guess opened another project or - /// failed with a host error that hid the real cause. - var showURL: String? { + /// What `tools hub pr show` takes: the page, else `!` / `#`, which the CLI + /// looks up among every checkout under the repo roots (and reads an unknown `owner/repo#n` as a + /// github.com PR). Nil for a bare number: only the list's rows can name its project. + var showArg: String? { if let pageURL { return pageURL } - guard !isMergeRequest, let project, project.split(separator: "/").count == 2 else { return nil } - return "https://github.com/\(project)/pull/\(number)" + guard let project else { return nil } + return "\(project)\(isMergeRequest ? "!" : "#")\(number)" } /// The forge host of the page this ref came from; nil for a path-only ref. @@ -151,7 +151,7 @@ struct HubPRRef: Equatable { return pr.repo.lowercased() == wanted || key == wanted || key.hasSuffix("/" + wanted) } - var label: String { project.map { "\($0)#\(number)" } ?? "#\(number)" } + var label: String { "\(project ?? "")\(isMergeRequest ? "!" : "#")\(number)" } } /// A forge project with a checkout under the repo roots (`tools hub pr projects`). @@ -394,6 +394,9 @@ final class PRsModel: ObservableObject { /// The deep-linked PR `openDirect` is fetching, and the selection when it started: its answer /// still selects it after the list cleared `wanted`, unless someone picked another row meanwhile. private var directOpen: (ref: HubPRRef, selectionAtStart: String?)? + /// The last ref `openDirect` answered for, shown or failed: it said so itself, so a list landing + /// after it neither reports the ref missing a second time nor widens and asks again. + private var directAnswered: HubPRRef? /// The selected PR's live threads, started with its detail: the review adopts the store when it /// exists, so the threads never wait for the head fetch and the diff. private var prefetchedThreads: PRThreadsStore? @@ -514,12 +517,12 @@ final class PRsModel: ObservableObject { wantedStarted = nil // A deep-linked PR outside the list is on its way from its checkout (`openDirect`): // that answer selects it, so the list neither reports it missing nor picks another row. - if !select(wanted), directOpen?.ref != wanted { + if !select(wanted), directOpen?.ref != wanted, directAnswered != wanted { if widen(to: wanted) { widening = true return } - errors.append(PRListNotice("\(wanted.label) is not among these PRs")) + errors.append(Self.notListed(wanted)) } } if directOpen == nil, selectedID == nil || selected == nil, @@ -559,15 +562,16 @@ final class PRsModel: ObservableObject { wantedStarted = nil startWanted() } else if !select(ref, opened: true) { - if widen(to: ref) { - reload() - } else { - // Not in the list (newer than it, or of a project it does not hold): straight from its checkout. - wanted = ref - wantedStarted = nil - if !startWanted() { + // Not in the list (newer, merged, or of a project it does not hold): straight from its checkout, + // before any reload. Only a bare number, which only the rows can place, waits for the All list. + wanted = ref + wantedStarted = nil + if !startWanted() { + if widen(to: ref) { + reload() + } else { wanted = nil - errors.append(PRListNotice("\(ref.label) is not among these PRs")) + errors.append(Self.notListed(ref)) } } } @@ -590,10 +594,10 @@ final class PRsModel: ObservableObject { return true } guard matches.isEmpty else { return false } - // A checkout of the project among the list's folders, else the PR's page: `hub pr show` finds - // the checkout under the repo roots itself. Without the page fallback a PR of a project with no - // recent session ("Open in GenesisTools" on Reservine/ReservineBack#815) left another PR open. - guard let arg = ref.pageURL ?? directRoot(ref).map({ "\($0)#\(ref.number)" }) ?? ref.showURL else { return false } + // The PR's page, else a checkout already known here, else the project name: `hub pr show` finds + // its checkout under the repo roots itself. Waiting for this hub's project index lost a link that + // came before it loaded (`group/sub/app!12` from a link, 2026-10-19: "is not among these PRs"). + guard let arg = ref.pageURL ?? directRoot(ref).map({ "\($0)#\(ref.number)" }) ?? ref.showArg else { return false } wantedStarted = ref openDirect(ref, arg: arg) return true @@ -634,6 +638,7 @@ final class PRsModel: ObservableObject { if request?.ref == ref { directOpen = nil } + directAnswered = ref let fresh: (HubPR, HubPRDetail, Data) switch answer { case .success(let value): @@ -745,6 +750,11 @@ final class PRsModel: ObservableObject { return true } + /// Only a ref with no project gets here: one with a project or a page always goes to `openDirect`. + nonisolated static func notListed(_ ref: HubPRRef) -> PRListNotice { + PRListNotice("\(ref.label) is not in this list. Name its project (\(ref.isMergeRequest ? "!" : "#")\(ref.number)) or paste its page URL") + } + /// Selects the one PR `ref` names; false when none matches. A bare number found in several /// projects selects nothing and says which projects, since picking one would be a guess. @discardableResult diff --git a/src/macos/GenesisTools/Tests/HubLogicTests.swift b/src/macos/GenesisTools/Tests/HubLogicTests.swift index 3a569c90f4..06f69fb791 100644 --- a/src/macos/GenesisTools/Tests/HubLogicTests.swift +++ b/src/macos/GenesisTools/Tests/HubLogicTests.swift @@ -69,11 +69,22 @@ final class HubLogicTests: XCTestCase { XCTAssertNil(HubPRRef("#42")?.project) XCTAssertEqual(HubPRRef("group/app#42")?.project, "group/app") XCTAssertEqual(HubPRRef("app!12")?.number, 12) - XCTAssertEqual(HubPRRef("app!12")?.label, "app#12") + XCTAssertEqual(HubPRRef("app!12")?.label, "app!12") + XCTAssertEqual(HubPRRef("#42")?.label, "#42") XCTAssertNil(HubPRRef("app#x")) XCTAssertNil(HubPRRef("seven")) } + /// A ref outside the list goes to `tools hub pr show` as the project name, which the CLI finds among + /// every checkout under the repo roots; the hub's own project index may not have loaded yet. + func testAPRRefOutsideTheListOpensByItsProjectName() { + XCTAssertEqual(HubPRRef("group/sub/app!12")?.showArg, "group/sub/app!12") + XCTAssertEqual(HubPRRef("acme/app#42")?.showArg, "acme/app#42") + XCTAssertEqual(HubPRRef("https://gitlab.example/group/app/-/merge_requests/3")?.showArg, "https://gitlab.example/group/app/-/merge_requests/3") + XCTAssertNil(HubPRRef("#42")?.showArg) + XCTAssertTrue(PRsModel.notListed(HubPRRef("!7")!).text.contains("!7")) + } + /// The browser extension and pasted links hand the hub a page URL, often with a tab suffix. func testAPRRefReadsAPRorMRPageURL() { XCTAssertEqual(HubPRRef("https://github.com/acme/App/pull/424/files")?.project, "acme/App") From 7672f4f4981a1350867bb716024935e97dc8981a Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 19 Oct 2026 17:45:16 +0200 Subject: [PATCH 02/11] fix(GenesisHub, prs): of two clones of one project, the folder named like the project names the PR group and is the checkout a link opens --- src/hub/lib/pr/pr.test.ts | 11 +++++++++++ src/hub/lib/pr/project-ref.ts | 11 ++++++++++- src/hub/lib/prs.ts | 22 ++++++++++++++++++++-- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/hub/lib/pr/pr.test.ts b/src/hub/lib/pr/pr.test.ts index 3651712f63..a270ea95e8 100644 --- a/src/hub/lib/pr/pr.test.ts +++ b/src/hub/lib/pr/pr.test.ts @@ -1342,6 +1342,17 @@ describe("resolveHubPrRef", () => { expect("problem" in twice && twice.problem).toContain("names 2 projects"); }); + test("of two clones of one project, the folder named like the project wins over the first by name", async () => { + const clones = async () => ({ + roots: ["/work"], + checkouts: [ + checkout("/work/aaa/site-v2", "github.com", "acme/site"), + checkout("/work/site", "github.com", "acme/site"), + ], + }); + expect(await resolveHubPrRef("acme/site#4", { source: clones })).toEqual({ path: "/work/site", number: 4 }); + }); + test("a URL never scans the checkouts", async () => { const never = async () => { throw new Error("scanned"); diff --git a/src/hub/lib/pr/project-ref.ts b/src/hub/lib/pr/project-ref.ts index 8f652a2aa7..e5b0b1adb3 100644 --- a/src/hub/lib/pr/project-ref.ts +++ b/src/hub/lib/pr/project-ref.ts @@ -1,4 +1,5 @@ import { existsSync } from "node:fs"; +import { basename } from "node:path"; import { loadConfig } from "@app/browser-extension/lib/config"; import { configuredCheckouts } from "@app/browser-extension/lib/deps"; import { type LocalCheckout, rankCheckouts } from "@genesiscz/utils/git/local-checkouts"; @@ -87,6 +88,14 @@ function checkoutOf(name: string, checkouts: LocalCheckout[]): { root: string } return { projects }; } - const best = rankCheckouts({ project: pool[0].project, checkouts: pool })[0]; + // Of two clones of one project, the folder named like the project (`app`, not `app-v2`). + const ranked = rankCheckouts({ project: pool[0].project, checkouts: pool }); + const best = ranked.find((checkout) => namedLikeProject(checkout.root, checkout.project.path)) ?? ranked[0]; return best ? { root: best.root } : { projects }; } + +/** Whether a checkout's folder carries its project's name (the last segment of the web path), in any case. */ +export function namedLikeProject(root: string, web: string): boolean { + const name = web.replace(/\/+$/, "").split("/").pop() ?? ""; + return basename(root).toLowerCase() === name.toLowerCase(); +} diff --git a/src/hub/lib/prs.ts b/src/hub/lib/prs.ts index 17994ff494..a9fc4a4b1b 100644 --- a/src/hub/lib/prs.ts +++ b/src/hub/lib/prs.ts @@ -30,7 +30,7 @@ import { logger } from "@genesiscz/utils/logger"; import { profiler } from "@genesiscz/utils/profile"; import { cached } from "@genesiscz/utils/storage/cache-flag"; import { Storage } from "@genesiscz/utils/storage/storage"; -import { resolveHubPrRef } from "./pr/project-ref"; +import { namedLikeProject, resolveHubPrRef } from "./pr/project-ref"; import { type ProposalSummary, proposalFor } from "./proposal"; /** The hub's PR row: the host's PR plus where it lives on this machine. */ @@ -237,6 +237,16 @@ function groupByOrigin(facts: RepoFacts[]): { groups: ProjectGroup[]; skipped: H groups.set(key, group); } + // Two clones of one project: the folder named like the project names the group and is its checkout. + for (const group of groups.values()) { + const named = group.facts.findIndex((fact) => fact.root && namedLikeProject(fact.root, group.key)); + + if (named > 0) { + group.facts.unshift(...group.facts.splice(named, 1)); + group.paths.unshift(...group.paths.splice(named, 1)); + } + } + return { groups: [...groups.values()], skipped }; } @@ -476,7 +486,15 @@ export async function hubPrProjects(): Promise { const web = fact.origin?.web; const kind = fact.origin?.kind; - if (!web || !fact.root || (kind !== "github" && kind !== "gitlab") || byProject.has(web)) { + if (!web || !fact.root || (kind !== "github" && kind !== "gitlab")) { + continue; + } + + // Two clones of one project (`daf-app` and `daf-app-v2`): the one named like the project wins, whatever + // order the scan found them in. The first one found had named the PR group after the other folder. + const known = byProject.get(web); + + if (known && (namedLikeProject(known.root, web) || !namedLikeProject(fact.root, web))) { continue; } From 7075baadaa9b05c152568956570c9cb5b4d0487e Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 19 Oct 2026 17:45:23 +0200 Subject: [PATCH 03/11] fix(GenesisHub, prs): a GitLab MR's commits come oldest first, as a GitHub PR's do --- src/utils/git/origins/detector.test.ts | 3 ++- src/utils/git/origins/prs.ts | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/utils/git/origins/detector.test.ts b/src/utils/git/origins/detector.test.ts index 088d74c691..5bb5a420e7 100644 --- a/src/utils/git/origins/detector.test.ts +++ b/src/utils/git/origins/detector.test.ts @@ -864,7 +864,8 @@ describe("GitLab MR list and view", () => { { id: "c2", title: "Tidy", message: "Tidy\n", author_name: "Bob", committed_date: "d" }, ]), }); - expect(detail.commits.map((commit) => commit.body)).toEqual(["The reason.", null]); + // GitLab answers newest first; the detail is oldest first, as gh's. + expect(detail.commits.map((commit) => commit.body)).toEqual([null, "The reason."]); }); }); diff --git a/src/utils/git/origins/prs.ts b/src/utils/git/origins/prs.ts index 37c2308cde..1f05fe9c8d 100644 --- a/src/utils/git/origins/prs.ts +++ b/src/utils/git/origins/prs.ts @@ -840,15 +840,18 @@ export function parseGlabMrView({ reviewDecision, approvals: approvalCount, body: str(row.description) ?? "", + // GitLab lists an MR's commits newest first, gh oldest first: every caller gets the oldest first. commits: commits - ? records(parseJson(commits, "glab commits")).map((commit) => ({ - sha: str(commit.id) ?? "", - title: str(commit.title) ?? "", - author: str(commit.author_name), - authorLogin: null, - date: str(commit.committed_date) ?? str(commit.authored_date), - body: glabCommitBody(str(commit.message)), - })) + ? records(parseJson(commits, "glab commits")) + .toReversed() + .map((commit) => ({ + sha: str(commit.id) ?? "", + title: str(commit.title) ?? "", + author: str(commit.author_name), + authorLogin: null, + date: str(commit.committed_date) ?? str(commit.authored_date), + body: glabCommitBody(str(commit.message)), + })) : [], changedFiles, additions: null, From bfa927689da67f310d4018fa0604fee78cf0bf89 Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 19 Oct 2026 17:45:27 +0200 Subject: [PATCH 04/11] feat(GenesisHub, prs): a hub link's commit=.. opens that run of a PR's commits, both ends included, and marks them in its commits list --- src/hub/lib/open.ts | 7 +- .../Sources/Parts/Hub/Hub/HubCommitLink.swift | 79 ++++++++++++++++--- .../Sources/Parts/Hub/Hub/HubPRs.swift | 71 ++++++++++++++--- .../Sources/Parts/Hub/Hub/HubWindow.swift | 5 +- .../GenesisTools/Tests/HubLogicTests.swift | 35 ++++++++ src/utils/browser-router/presets.ts | 2 +- 6 files changed, 170 insertions(+), 29 deletions(-) diff --git a/src/hub/lib/open.ts b/src/hub/lib/open.ts index 292abc8fe4..e967a88c8a 100644 --- a/src/hub/lib/open.ts +++ b/src/hub/lib/open.ts @@ -26,8 +26,9 @@ export interface OpenHubOptions { /** With `pr`: this repo-relative file opens in that PR's review. */ reveal?: string; /** - * A commit sha (7 to 40 hex characters, a prefix is enough). With `pr` it is selected in that PR's - * review; without it, `repo` names its project and it opens in the Worktrees mode's diff. + * A commit sha (7 to 40 hex characters, a prefix is enough), or `..`: a run of commits with both + * ends included. With `pr` it is selected in that PR's review; without it, `repo` names its project and it + * opens in the Worktrees mode's diff. */ commit?: string; /** With `commit` and no `pr`: the commit's project, a checkout path or its forge path (`owner/repo`). */ @@ -151,7 +152,7 @@ export function hubArgs(options: OpenHubOptions): string[] { * The same target as a link: `genesis-tools://hub?session=&agent=`, with `mode`, * `pr`, `reveal`, `repo`, `commit`, `tab`, `filter` and `worktree` as further query items (keys of * `HubRequest.linkKeys` in the app). A link without `worktree` and `reveal` lost them: `--print-url` - * named neither. A commit link: `?pr=owner%2Frepo%2342&commit=`, or `?repo=&commit=`. + * named neither. A commit link: `?pr=owner%2Frepo%2342&commit=` (or `..`), or `?repo=&commit=`. */ export function hubUrl(options: Pick): string { // The link itself is built in the router utils, beside its https twin, so the two cannot drift. diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift index 369e219615..c6a4e0176b 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift @@ -1,7 +1,7 @@ import Foundation // Commit links. `genesis-tools://hub?pr=&commit=` opens the PR and selects that commit in its review -// (`PRsModel.pickPendingCommit`); `genesis-tools://hub?repo=&commit=` opens the +// (`PRsModel.pickPendingCommit`), and `commit=..` selects that run of its commits, both ends included; `genesis-tools://hub?repo=&commit=` opens the // commit in its project's Worktrees diff, as Activity's commit rows do (`HubModel.showCommit`). The browser-router // preset `genesis-hub` turns `https:///hub?…` into the same link; `tools hub open --print-url` writes it. @@ -20,15 +20,46 @@ enum HubCommitSha { } static func invalid(_ raw: String) -> String { - "\(raw.prefix(48)) is not a commit: a link names one by 7 to 40 hex characters" + "\(raw.prefix(48)) is not a commit: a link names one by 7 to 40 hex characters, or a range as .." } } -/// A commit to select in one PR's review (`PRsModel.request(_:commit:)`). +/// What a link's `commit=` names: one commit, or `..`, a run of commits with both ends included +/// (the diff from the parent of `from` to `to`). Each end is normalized (`HubCommitSha`). +struct HubCommitSpan: Equatable { + let from: String + /// Nil for one commit. + let to: String? + + init(from: String, to: String? = nil) { + self.from = from + self.to = to + } + + init?(_ raw: String) { + let ends = raw.trimmingCharacters(in: .whitespacesAndNewlines).components(separatedBy: "..") + switch ends.count { + case 1: + guard let one = HubCommitSha.normalized(ends[0]) else { return nil } + self.init(from: one) + case 2: + guard let from = HubCommitSha.normalized(ends[0]), let to = HubCommitSha.normalized(ends[1]) else { return nil } + self.init(from: from, to: to) + default: + return nil + } + } + + var label: String { to.map { "\(from.prefix(10))..\($0.prefix(10))" } ?? String(from.prefix(10)) } +} + +/// A commit, or a range of commits, to select in one PR's review (`PRsModel.request(_:commit:)`). struct PRCommitPick: Equatable { let ref: HubPRRef - /// Normalized (`HubCommitSha`): a prefix of the commit's sha. + /// Normalized (`HubCommitSha`): a prefix of the commit's sha; the range's first commit. let sha: String + /// The range's last commit, nil for one commit. + var to: String? = nil /// When it was asked: only a detail fetched after this can say the PR does not hold the commit. let asked: Date } @@ -53,6 +84,8 @@ enum HubCommitLookup { let root: String let sha: String let title: String + /// For a range: the parent of its first commit, so the diff shows every commit of it. + var base: String? = nil } enum Outcome: Equatable { @@ -62,7 +95,7 @@ enum HubCommitLookup { /// `repo` is a checkout path (any folder inside it), or a forge path such as `owner/repo` that `known` (else a /// fresh `tools hub pr projects`) maps to its checkout. Runs git, so off the main thread. - static func resolve(repo: String, sha: String, known: [HubPRProject]) -> Outcome { + static func resolve(repo: String, sha: String, to: String? = nil, known: [HubPRProject]) -> Outcome { let text = repo.trimmingCharacters(in: .whitespacesAndNewlines) let folder: String if text.hasPrefix("/") || text.hasPrefix("~") { @@ -85,12 +118,32 @@ enum HubCommitLookup { guard let root = WorktreeDiscovery.git(folder, ["rev-parse", "--show-toplevel"])?.trimmed, !root.isEmpty else { return .failed("\(folder) is not a git checkout") } + let place = (root as NSString).abbreviatingWithTildeInPath guard let line = WorktreeDiscovery.git(root, ["show", "-s", "--format=%H%x00%s", "\(sha)^{commit}", "--"])?.trimmed, let separator = line.firstIndex(of: "\0") else { - return .failed("Commit \(sha) is not in \((root as NSString).abbreviatingWithTildeInPath) (fetch it first)") + return .failed("Commit \(sha) is not in \(place) (fetch it first)") + } + let first = String(line[..= pending.asked } ?? false) && reviewHeadSha == shown.headSha guard let commit = HubCommitSha.find(pending.sha, in: commits) else { if settled { - showMissingCommit(pending, in: shown, review: review) + showMissingCommit(pending, missing: pending.sha, in: shown, review: review) } return } + var last: HubPRDetail.Commit? + if let to = pending.to { + guard let found = HubCommitSha.find(to, in: commits) else { + if settled { + showMissingCommit(pending, missing: to, in: shown, review: review) + } + return + } + last = found + } if settled { pendingCommit = nil } - HubPerf.log("prs.commit \(shown.label) \(commit.sha.prefix(10)) settled=\(settled)") - showLinkedCommit(commit, prID: id, review: review) + HubPerf.log("prs.commit \(shown.label) \(commit.sha.prefix(10))\(last.map { "..\($0.sha.prefix(10))" } ?? "") settled=\(settled)") + if let last { + showLinkedRange(commit, last, in: commits, pr: shown, review: review) + } else { + showLinkedCommit(commit, prID: id, review: review) + } + } + + /// A link's `commit=..`: those commits of the PR, both ends included, in either order. + func showLinkedRange(_ one: HubPRDetail.Commit, _ other: HubPRDetail.Commit, in commits: [HubPRDetail.Commit], pr: HubPR, review: ReviewModel) { + guard let a = commits.firstIndex(of: one), let b = commits.firstIndex(of: other) else { return } + HubDefaults.store.set(true, forKey: "hub.prs.showDiff") + HubDefaults.store.set(false, forKey: "hub.prs.fold.commits") + commitScroll = PRCommitScroll(prID: pr.id, sha: commits[max(a, b)].sha) + let scope = Self.rangeScope(commits, first: min(a, b), last: max(a, b), pr: pr, whole: Self.scope(pr, detail: details[pr.id], fetch: fetchedHead(pr))) + if review.scope == scope { return } + review.setScope(scope) + } + + /// Commits `first...last` of a PR (oldest first, as the host lists them): from the commit before + /// `first`, or from the PR's own base when `first` is its first commit, to `last`. + nonisolated static func rangeScope(_ commits: [HubPRDetail.Commit], first: Int, last: Int, pr: HubPR, whole: DiffScope) -> DiffScope { + let head = commits[last].sha + let label = "\(pr.label) \(commits[first].sha.prefix(7))..\(head.prefix(7)) (\(Plural.count(last - first + 1, "commit")))" + if first > 0 { + return .range(base: commits[first - 1].sha, head: head, label: label) + } + if case .range(let base, _, _, let fallback) = whole { + return .range(base: base, head: head, label: label, fallbackBase: fallback) + } + return .range(base: "\(commits[first].sha)^", head: head, label: label) + } + + /// The commits of `commits` a range scope shows (`rangeScope`), for the commits list to mark; empty for + /// one commit or the whole PR. + nonisolated static func rangeShas(_ scope: DiffScope, in commits: [HubPRDetail.Commit]) -> Set { + guard case .range(let base, let head, _, _) = scope, let last = commits.firstIndex(where: { $0.sha == head }) else { return [] } + let first = commits.firstIndex(where: { $0.sha == base }).map { $0 + 1 } ?? 0 + guard first <= last, first > 0 || last < commits.count - 1 else { return [] } + return Set(commits[first...last].map(\.sha)) } /// A repeated link reveals the diff and scrolls again even when its commit is already selected. @@ -727,11 +775,12 @@ final class PRsModel: ObservableObject { } /// A settled miss replaces any previously selected commit with the whole PR before saying so. - func showMissingCommit(_ pending: PRCommitPick, in shown: HubPR, review: ReviewModel) { + func showMissingCommit(_ pending: PRCommitPick, missing: String? = nil, in shown: HubPR, review: ReviewModel) { pendingCommit = nil + let sha = missing ?? pending.sha review.setScope(Self.scope(shown, detail: details[shown.id], fetch: fetchedHead(shown))) - HubPerf.log("prs.commit \(shown.label) \(pending.sha) not among its commits") - notice = PRNotice(ref: pending.ref, text: "\(pending.sha) is not a commit of \(shown.label), so the whole \(shown.isGitLab ? "MR" : "PR") shows") + HubPerf.log("prs.commit \(shown.label) \(sha) not among its commits") + notice = PRNotice(ref: pending.ref, text: "\(sha) is not a commit of \(shown.label), so the whole \(shown.isGitLab ? "MR" : "PR") shows") } /// The overview scrolled to `scroll`; a later appearance of the PR does not scroll again. @@ -2301,7 +2350,9 @@ struct PRCommitsSection: View { var body: some View { let shown = shownSha - PRSection(title: "Commits", count: commits.count, folded: $folded, flat: true, trailing: shown == nil ? nil : AnyView( + // A range link's commits, all marked. + let range = PRsModel.rangeShas(review.scope, in: commits) + PRSection(title: "Commits", count: commits.count, folded: $folded, flat: true, trailing: shown == nil && range.isEmpty ? nil : AnyView( Button("Whole PR") { prs.showWholePR() } .buttonStyle(.genHoverPlain()) .font(.system(size: 11.5)) @@ -2309,7 +2360,7 @@ struct PRCommitsSection: View { )) { let authors = PRCommitRow.showsAuthors(commits) ForEach(commits.reversed()) { commit in - PRCommitRow(pr: pr, commit: commit, selected: commit.sha == shown, inApp: true, showsAuthor: authors) { + PRCommitRow(pr: pr, commit: commit, selected: commit.sha == shown || range.contains(commit.sha), inApp: true, showsAuthor: authors) { if prs.showCommit(commit) { revealDiff() } } } diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubWindow.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubWindow.swift index 01f71e172f..faec75e931 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubWindow.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubWindow.swift @@ -962,7 +962,7 @@ final class HubModel: ObservableObject { /// `--repo` + `--commit` link. The checkout is selected before the mode opens, so a first walk of /// the worktrees includes it; a launch that has not settled yet opens the mode when it settles. @MainActor - func showCommit(repo: String, sha: String, title: String) { + func showCommit(repo: String, sha: String, title: String, base: String? = nil) { cancelSessionNavigation() commitLinkRequest = UUID() selectedWorktree = repo @@ -972,7 +972,8 @@ final class HubModel: ObservableObject { } let next = ReviewModel(repo: URL(fileURLWithPath: repo), options: DiffViewOptions.remembered()) next.embedded = true - next.setScope(.commit(sha: sha, title: title)) + // A range link's commits: from the parent of its first commit to its last. + next.setScope(base.map { .range(base: $0, head: sha, label: title) } ?? .commit(sha: sha, title: title)) review = next addWorktreesIfMissing(repo) } diff --git a/src/macos/GenesisTools/Tests/HubLogicTests.swift b/src/macos/GenesisTools/Tests/HubLogicTests.swift index 06f69fb791..1fc99b5c27 100644 --- a/src/macos/GenesisTools/Tests/HubLogicTests.swift +++ b/src/macos/GenesisTools/Tests/HubLogicTests.swift @@ -145,6 +145,31 @@ final class HubLogicTests: XCTestCase { XCTAssertNil(HubCommitSha.find("abcdef9", in: commits)) } + /// `commit=..`: a run of a PR's commits, both ends included, in either order. + func testACommitRangeLinkShowsItsCommitsFromTheParentOfTheFirst() throws { + XCTAssertEqual(HubCommitSpan("abcdef0"), HubCommitSpan(from: "abcdef0")) + XCTAssertEqual(HubCommitSpan("67AD51671E..3cac936e2f"), HubCommitSpan(from: "67ad51671e", to: "3cac936e2f")) + XCTAssertNil(HubCommitSpan("abcdef0..xyz")) + XCTAssertNil(HubCommitSpan("abcdef0...1234567")) + let commit = { (sha: String) in HubPRDetail.Commit(sha: sha, title: sha, author: nil, authorLogin: nil, date: nil, body: nil) } + let commits = ["a000000", "b000000", "c000000", "d000000"].map(commit) + let json = """ + {"repo":"app","repoRoot":"/work/app","origin":{"kind":"gitlab","host":"git.example","web":"https://git.example/group/app"}, + "number":7,"title":"t","state":"OPEN","draft":false,"author":"a","headBranch":"feat/x","baseBranch":"main", + "url":"https://git.example/group/app/-/merge_requests/7","labels":[],"reviewers":[],"headSha":"d000000","crossRepository":false,"headRepo":null} + """ + let pr = try JSONDecoder().decode(HubPR.self, from: Data(json.utf8)) + let whole = DiffScope.range(base: "base000", head: "d000000", label: "whole", fallbackBase: "origin/main") + let middle = PRsModel.rangeScope(commits, first: 1, last: 2, pr: pr, whole: whole) + XCTAssertEqual(middle, .range(base: "a000000", head: "c000000", label: "!7 b000000..c000000 (2 commits)")) + XCTAssertEqual(PRsModel.rangeShas(middle, in: commits), ["b000000", "c000000"]) + let opening = PRsModel.rangeScope(commits, first: 0, last: 1, pr: pr, whole: whole) + XCTAssertEqual(opening, .range(base: "base000", head: "b000000", label: "!7 a000000..b000000 (2 commits)", fallbackBase: "origin/main")) + XCTAssertEqual(PRsModel.rangeShas(opening, in: commits), ["a000000", "b000000"]) + XCTAssertEqual(PRsModel.rangeShas(whole, in: commits), [], "the whole PR marks no commit") + XCTAssertEqual(PRsModel.rangeShas(.commit(sha: "b000000", title: "b"), in: commits), []) + } + func testAForgePathFindsItsCheckoutInAnyCaseAndForm() { let projects = [ HubPRProject(project: "https://github.com/acme/App", repo: "App", root: "/fixture/app", kind: "github"), @@ -181,6 +206,16 @@ final class HubLogicTests: XCTestCase { return XCTFail("a commit the checkout lacks must be said") } XCTAssertTrue(message.contains("is not in"), message) + + // A range, named in either order: from the parent of the older end to the newer one. + for _ in 0..<2 { + XCTAssertNotNil(WorktreeDiscovery.git(path, commit)) + } + let newest = try XCTUnwrap(WorktreeDiscovery.git(path, ["rev-parse", "HEAD"])?.trimmed) + let middle = try XCTUnwrap(WorktreeDiscovery.git(path, ["rev-parse", "HEAD^"])?.trimmed) + let range = HubCommitLookup.Found(root: root, sha: newest, title: "\(middle.prefix(10))..\(newest.prefix(10)) (2 commits)", base: head) + XCTAssertEqual(HubCommitLookup.resolve(repo: path, sha: String(middle.prefix(8)), to: String(newest.prefix(8)), known: []), .found(range)) + XCTAssertEqual(HubCommitLookup.resolve(repo: path, sha: String(newest.prefix(8)), to: String(middle.prefix(8)), known: []), .found(range)) } @MainActor diff --git a/src/utils/browser-router/presets.ts b/src/utils/browser-router/presets.ts index 8492233cef..e7bc187839 100644 --- a/src/utils/browser-router/presets.ts +++ b/src/utils/browser-router/presets.ts @@ -122,7 +122,7 @@ function catalog({ linkHost, options }: CatalogContext): PresetSpec[] { title: "GenesisTools hub", kind: "installable", description: - "https:///hub? opens genesis-tools://hub?: a PR, a commit in it (pr=…&commit=…), a session.", + "https:///hub? opens genesis-tools://hub?: a PR, a commit or a range of commits in it (pr=…&commit= or ..), a session.", enabledIf: ["browser-router:installed"], needsLinkHost: true, probe: linkHost ? `https://${linkHost}/hub?pr=1&commit=0000000` : undefined, From dd8993fb854fc39ae776161cfe6b3df98fa31561 Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 19 Oct 2026 19:09:40 +0200 Subject: [PATCH 05/11] fix(GenesisHub, prs): a long PR description sits outside the overview's lazy stack, so a link to the commit right under it no longer freezes the hub --- .../Sources/Parts/Hub/Hub/HubPRs.swift | 41 +++++++++++-------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift index 32d50622a4..c118a77e99 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift @@ -2126,7 +2126,10 @@ struct PRDetailView: View { // Lazy, rows included: every realized row is a focus responder, and SwiftUI walks all of // them whenever a sidebar group folds (1.1 s of main thread with #424's 92 commits shown). // The PR first (description, commits, checks), the agent sessions that touched it after. - LazyVStack(alignment: .leading, spacing: 0) { + // The description stays outside the lazy stack: as one lazy item, a long one (thousands of + // points of markdown) was made and dropped again on every layout pass once a link scrolled to + // the commit right under it, and the hub froze for good (!7553's newest commit, 2026-10-19). + VStack(alignment: .leading, spacing: 0) { if let body = detail?.body?.trimmed, !body.isEmpty { PRSection(title: "Description", folded: $foldDescription) { MarkdownContentView(markdown: linkedDescription(body), style: .hub) @@ -2149,28 +2152,30 @@ struct PRDetailView: View { .accessibilityElement(children: .ignore) .accessibilityLabel("Loading the description") } - if let commits = detail?.commits, !commits.isEmpty { - if let review = prs.review { - PRCommitsSection(prs: prs, review: review, pr: pr, commits: commits, folded: $foldCommits) { - showDiff = true - } - } else { - PRSection(title: "Commits", count: commits.count, folded: $foldCommits, flat: true) { - let authors = PRCommitRow.showsAuthors(commits) - ForEach(commits.reversed()) { commit in - PRCommitRow(pr: pr, commit: commit, selected: false, inApp: false, showsAuthor: authors) { - if let url = pr.commitURL(commit.sha) { ExternalOpener.open(url) } + LazyVStack(alignment: .leading, spacing: 0) { + if let commits = detail?.commits, !commits.isEmpty { + if let review = prs.review { + PRCommitsSection(prs: prs, review: review, pr: pr, commits: commits, folded: $foldCommits) { + showDiff = true + } + } else { + PRSection(title: "Commits", count: commits.count, folded: $foldCommits, flat: true) { + let authors = PRCommitRow.showsAuthors(commits) + ForEach(commits.reversed()) { commit in + PRCommitRow(pr: pr, commit: commit, selected: false, inApp: false, showsAuthor: authors) { + if let url = pr.commitURL(commit.sha) { ExternalOpener.open(url) } + } } } } } + if let checks = detail?.checks, !checks.isEmpty { + PRChecksSection(model: model, pr: pr, checks: checks, folded: $foldChecks) + .id(Self.checksID) + } + PRSessionsSection(model: model, prs: prs, store: prs.sessions, pr: pr, folded: $foldSessions) + .id(Self.sessionsID) } - if let checks = detail?.checks, !checks.isEmpty { - PRChecksSection(model: model, pr: pr, checks: checks, folded: $foldChecks) - .id(Self.checksID) - } - PRSessionsSection(model: model, prs: prs, store: prs.sessions, pr: pr, folded: $foldSessions) - .id(Self.sessionsID) } .padding(18) .frame(maxWidth: .infinity, alignment: .leading) From ad045096b3a0cb2c2891d8727fbac8614c01a381 Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 19 Oct 2026 19:09:46 +0200 Subject: [PATCH 06/11] fix(GenesisHub, prs): tools hub open --commit takes .., as the hub link does --- src/hub/lib/agents/agents.test.ts | 3 +++ src/hub/lib/pr-ref.ts | 9 ++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/hub/lib/agents/agents.test.ts b/src/hub/lib/agents/agents.test.ts index 70ede97506..5189df315b 100644 --- a/src/hub/lib/agents/agents.test.ts +++ b/src/hub/lib/agents/agents.test.ts @@ -499,6 +499,9 @@ describe("hub open deep link", () => { expect(isHubCommitSha("abc123")).toBe(false); expect(isHubCommitSha("abc123g")).toBe(false); expect(isHubCommitSha("a".repeat(41))).toBe(false); + expect(isHubCommitSha("abc1234..def5678")).toBe(true); + expect(isHubCommitSha("abc1234...def5678")).toBe(false); + expect(isHubCommitSha("abc1234..")).toBe(false); expect(commitFlagsProblem({ pr: "acme/app#7", commit: "abc1234" })).toBeUndefined(); expect(commitFlagsProblem({ repo: "acme/app", commit: "abc1234" })).toBeUndefined(); expect(commitFlagsProblem({})).toBeUndefined(); diff --git a/src/hub/lib/pr-ref.ts b/src/hub/lib/pr-ref.ts index 5879046481..759c715701 100644 --- a/src/hub/lib/pr-ref.ts +++ b/src/hub/lib/pr-ref.ts @@ -11,9 +11,12 @@ export function isHubPrRef(ref: string): boolean { return /^(?:.*[#!])?\d+$/.test(text) || PR_PAGE.test(text); } -/** What the hub's `--commit` takes (`HubCommitSha` in Sources/Hub/HubCommitLink.swift): 7 to 40 hex characters, a prefix of the sha. */ +/** + * What the hub's `--commit` takes (`HubCommitSpan` in Sources/Parts/Hub/Hub/HubCommitLink.swift): 7 to 40 hex + * characters, a prefix of the sha, or `..`, a run of commits with both ends included. + */ export function isHubCommitSha(sha: string): boolean { - return /^[0-9a-f]{7,40}$/i.test(sha.trim()); + return /^[0-9a-f]{7,40}(?:\.\.[0-9a-f]{7,40})?$/i.test(sha.trim()); } /** Why `tools hub open`'s `--commit` / `--repo` cannot open as given; undefined when they can or are absent. */ @@ -27,7 +30,7 @@ export function commitFlagsProblem({ pr?: string; }): string | undefined { if (commit !== undefined && !isHubCommitSha(commit)) { - return `--commit takes a sha of 7 to 40 hex characters, got ${commit}`; + return `--commit takes a sha of 7 to 40 hex characters, or .., got ${commit}`; } if (repo !== undefined && pr !== undefined) { From 5d619dc2bd9404522b799f6c7ccc4edd090ac962 Mon Sep 17 00:00:00 2001 From: Martin Date: Tue, 20 Oct 2026 21:07:27 +0200 Subject: [PATCH 07/11] fix(GenesisHub): a commit link that names the upstream path opens in a fork's checkout with the same project name that holds the commit --- .../Sources/Parts/Hub/Hub/HubCommitLink.swift | 17 ++++++++++++++++- .../GenesisTools/Tests/HubLogicTests.swift | 7 +++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift index c6a4e0176b..79c604a775 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift @@ -107,7 +107,7 @@ enum HubCommitLookup { } catch { return .failed("Could not list the projects to find \(text): \(error)") } - guard let root = PRsModel.projectRoot(text, in: projects) else { + guard let root = PRsModel.projectRoot(text, in: projects) ?? sameName(text, holding: sha, in: projects) else { return .failed("No checkout of \(text) under the repo roots, so commit \(sha) cannot open") } folder = root @@ -142,6 +142,21 @@ enum HubCommitLookup { return .found(Found(root: root, sha: newer, title: label, base: base?.isEmpty == false ? base : emptyTree)) } + /// A fork or a mirror: the link names one path (the upstream `group/sub/app`), the checkout clones another + /// (`me/app`). The projects with the same last name are the candidates, and the first that holds the commit wins. + static func sameName(_ repo: String, holding sha: String, in projects: [HubPRProject]) -> String? { + let name = { (path: String) -> String in + var last = path.lowercased().split(separator: "/").last.map(String.init) ?? "" + if last.hasSuffix(".git") { last.removeLast(4) } + return last + } + let wanted = name(repo) + guard !wanted.isEmpty else { return nil } + return projects.first { candidate in + name(candidate.project) == wanted && WorktreeDiscovery.git(candidate.root, ["cat-file", "-e", "\(sha)^{commit}"]) != nil + }?.root + } + /// Git's empty tree: the base of a range that starts at a root commit. static let emptyTree = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" } diff --git a/src/macos/GenesisTools/Tests/HubLogicTests.swift b/src/macos/GenesisTools/Tests/HubLogicTests.swift index 1fc99b5c27..07973e58bc 100644 --- a/src/macos/GenesisTools/Tests/HubLogicTests.swift +++ b/src/macos/GenesisTools/Tests/HubLogicTests.swift @@ -202,6 +202,13 @@ final class HubLogicTests: XCTestCase { XCTAssertEqual(HubCommitLookup.resolve(repo: path + "/", sha: short, known: []), found) let projects = [HubPRProject(project: "https://example.test/acme/app", repo: "app", root: path, kind: "github")] XCTAssertEqual(HubCommitLookup.resolve(repo: "acme/app", sha: short, known: projects), found) + // The link names the upstream, the checkout is a fork: the same last name with the commit is the checkout. + let fork = [HubPRProject(project: "https://example.test/other/app", repo: "app", root: "/fixture/none", kind: "gitlab"), + HubPRProject(project: "https://example.test/me/app", repo: "app", root: path, kind: "gitlab")] + XCTAssertEqual(HubCommitLookup.resolve(repo: "group/sub/app", sha: short, known: fork), found) + guard case .failed = HubCommitLookup.resolve(repo: "group/sub/app", sha: "0000000", known: fork) else { + return XCTFail("a same-name project without the commit is no match") + } guard case .failed(let message) = HubCommitLookup.resolve(repo: path, sha: "0000000", known: []) else { return XCTFail("a commit the checkout lacks must be said") } From cea5d33b16ab8ae64e3d3c58ee72ffd2c2bf8404 Mon Sep 17 00:00:00 2001 From: Martin Date: Tue, 20 Oct 2026 22:00:48 +0200 Subject: [PATCH 08/11] fix(GenesisHub, prs): GitLab MR commits are reversed with slice().reverse(), since toReversed() is missing from one typecheck config's lib --- src/utils/git/origins/prs.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utils/git/origins/prs.ts b/src/utils/git/origins/prs.ts index 1f05fe9c8d..630e070b30 100644 --- a/src/utils/git/origins/prs.ts +++ b/src/utils/git/origins/prs.ts @@ -843,7 +843,8 @@ export function parseGlabMrView({ // GitLab lists an MR's commits newest first, gh oldest first: every caller gets the oldest first. commits: commits ? records(parseJson(commits, "glab commits")) - .toReversed() + .slice() + .reverse() .map((commit) => ({ sha: str(commit.id) ?? "", title: str(commit.title) ?? "", From 492f7db020224611120daf169c5b4b1fe0053f74 Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 21:30:51 +0200 Subject: [PATCH 09/11] fix(GenesisHub): #557 followup - a ! project ref ignores a same-named folder in the cwd, a deep-link failure survives the next list load, and a range link opens the fork that holds both ends --- src/hub/lib/pr/pr.test.ts | 16 ++++++++++++- src/hub/lib/pr/project-ref.ts | 7 +++++- .../Sources/Parts/Hub/Hub/HubCommitLink.swift | 11 +++++---- .../Sources/Parts/Hub/Hub/HubPRs.swift | 23 +++++++++++++++---- .../GenesisTools/Tests/HubLogicTests.swift | 15 ++++++++++++ 5 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/hub/lib/pr/pr.test.ts b/src/hub/lib/pr/pr.test.ts index a270ea95e8..5faabf57fd 100644 --- a/src/hub/lib/pr/pr.test.ts +++ b/src/hub/lib/pr/pr.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { createHash, randomUUID } from "node:crypto"; -import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { defaultReviewCommentClient, type ReviewCommentClient } from "@app/github/lib/review-comments"; @@ -1326,6 +1326,20 @@ describe("resolveHubPrRef", () => { expect(await resolveHubPrRef("app#12", { source })).toEqual({ path: "/work/app", number: 12 }); }); + test("a ! ref finds its checkout even when the current folder holds a folder of the same name", async () => { + const here = process.cwd(); + const scratch = mkdtempSync(join(tmpdir(), "gt-project-ref-")); + mkdirSync(join(scratch, "app")); + process.chdir(scratch); + + try { + expect(await resolveHubPrRef("app!12", { source })).toEqual({ path: "/work/app", number: 12 }); + } finally { + process.chdir(here); + rmSync(scratch, { recursive: true, force: true }); + } + }); + test("a checkout wins over the github.com guess, which stays for a project with none", async () => { expect(await resolveHubPrRef("acme/web#5", { source })).toEqual({ path: "/work/web", number: 5 }); expect(await resolveHubPrRef("acme/other#5", { source })).toEqual({ diff --git a/src/hub/lib/pr/project-ref.ts b/src/hub/lib/pr/project-ref.ts index e5b0b1adb3..3aa3e6906c 100644 --- a/src/hub/lib/pr/project-ref.ts +++ b/src/hub/lib/pr/project-ref.ts @@ -33,7 +33,12 @@ export async function resolveHubPrRef( const trimmed = ref.trim(); const named = /^([^\s#!]+)([#!])(\d+)$/.exec(trimmed); - if (!named || /^https?:\/\//i.test(trimmed) || /^[~./]/.test(named[1]) || existsSync(named[1])) { + if ( + !named || + /^https?:\/\//i.test(trimmed) || + /^[~./]/.test(named[1]) || + (named[2] === "#" && existsSync(named[1])) + ) { return ( parsePrRef(trimmed) ?? { problem: `expected a PR/MR URL, # or !, got "${ref}"`, diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift index 79c604a775..1915518b38 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift @@ -107,7 +107,7 @@ enum HubCommitLookup { } catch { return .failed("Could not list the projects to find \(text): \(error)") } - guard let root = PRsModel.projectRoot(text, in: projects) ?? sameName(text, holding: sha, in: projects) else { + guard let root = PRsModel.projectRoot(text, in: projects) ?? sameName(text, holding: [sha] + (to.map { [$0] } ?? []), in: projects) else { return .failed("No checkout of \(text) under the repo roots, so commit \(sha) cannot open") } folder = root @@ -143,8 +143,9 @@ enum HubCommitLookup { } /// A fork or a mirror: the link names one path (the upstream `group/sub/app`), the checkout clones another - /// (`me/app`). The projects with the same last name are the candidates, and the first that holds the commit wins. - static func sameName(_ repo: String, holding sha: String, in projects: [HubPRProject]) -> String? { + /// (`me/app`). The projects with the same last name are the candidates, and the first that holds every commit + /// of the link (both ends of a range) wins. + static func sameName(_ repo: String, holding shas: [String], in projects: [HubPRProject]) -> String? { let name = { (path: String) -> String in var last = path.lowercased().split(separator: "/").last.map(String.init) ?? "" if last.hasSuffix(".git") { last.removeLast(4) } @@ -153,7 +154,9 @@ enum HubCommitLookup { let wanted = name(repo) guard !wanted.isEmpty else { return nil } return projects.first { candidate in - name(candidate.project) == wanted && WorktreeDiscovery.git(candidate.root, ["cat-file", "-e", "\(sha)^{commit}"]) != nil + name(candidate.project) == wanted && shas.allSatisfy { sha in + WorktreeDiscovery.git(candidate.root, ["cat-file", "-e", "\(sha)^{commit}"]) != nil + } }?.root } diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift index c118a77e99..e7a5a81ea5 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift @@ -397,6 +397,9 @@ final class PRsModel: ObservableObject { /// The last ref `openDirect` answered for, shown or failed: it said so itself, so a list landing /// after it neither reports the ref missing a second time nor widens and asks again. private var directAnswered: HubPRRef? + /// Why the last `openDirect` failed. Kept apart from the list's own notices, which every list load replaces: + /// a list that lands after the failure would otherwise erase the only word on why the link opened nothing. + private var directFailure: PRListNotice? /// The selected PR's live threads, started with its detail: the review adopts the store when it /// exists, so the threads never wait for the head fetch and the diff. private var prefetchedThreads: PRThreadsStore? @@ -624,6 +627,10 @@ final class PRsModel: ObservableObject { /// its head commit. A cached row (`startWanted`) starts all three at once. private func openDirect(_ ref: HubPRRef, arg: String) { directOpen = (ref, selectedID) + if let failure = directFailure { + errors.removeAll { $0 == failure } + directFailure = nil + } Task { let span = HubPerf.begin("prs.show.direct", arg, awaits: true) let answer = await Task.detached(priority: .userInitiated) { () -> Result<(HubPR, HubPRDetail, Data), Error> in @@ -646,17 +653,22 @@ final class PRsModel: ObservableObject { case .failure(let error): span.end("failed") // Said, not swallowed: the list's own answer no longer reports a ref it handed here. + let notice: PRListNotice if let failure = error as? PRShowFailure, let summary = failure.summary { - errors.append(PRListNotice("Could not open \(ref.label): \(summary)", more: failure.message)) + notice = PRListNotice("Could not open \(ref.label): \(summary)", more: failure.message) } else { - errors.append(PRListNotice.failure("Could not open \(ref.label)", error)) + notice = PRListNotice.failure("Could not open \(ref.label)", error) } + directFailure = notice + errors.append(notice) return } let (row, detail, data) = fresh guard ref.matches(row) else { span.end("another PR") - errors.append(PRListNotice("\(arg) answered \(row.label), not \(ref.label)")) + let notice = PRListNotice("\(arg) answered \(row.label), not \(ref.label)") + directFailure = notice + errors.append(notice) return } span.end(row.label) @@ -936,6 +948,9 @@ final class PRsModel: ObservableObject { func dismissError(_ notice: PRListNotice) { errors.removeAll { $0 == notice } + if directFailure == notice { + directFailure = nil + } } /// A project opened from "More projects" (not a session project, not starred) leaves the list @@ -1006,7 +1021,7 @@ final class PRsModel: ObservableObject { fullProjects = Set(list.repos.compactMap { repo in (repo.count ?? 0) >= limit ? repo.origin?.web : nil }) changed = moved } - errors = PRListNotice.failures(list.repos) + errors = PRListNotice.failures(list.repos) + (directFailure.map { [$0] } ?? []) SWR.fade(moved, current: { [weak self] in self?.changed }, clear: { [weak self] in self?.changed = [] }) } diff --git a/src/macos/GenesisTools/Tests/HubLogicTests.swift b/src/macos/GenesisTools/Tests/HubLogicTests.swift index 07973e58bc..695e47c650 100644 --- a/src/macos/GenesisTools/Tests/HubLogicTests.swift +++ b/src/macos/GenesisTools/Tests/HubLogicTests.swift @@ -223,6 +223,21 @@ final class HubLogicTests: XCTestCase { let range = HubCommitLookup.Found(root: root, sha: newest, title: "\(middle.prefix(10))..\(newest.prefix(10)) (2 commits)", base: head) XCTAssertEqual(HubCommitLookup.resolve(repo: path, sha: String(middle.prefix(8)), to: String(newest.prefix(8)), known: []), .found(range)) XCTAssertEqual(HubCommitLookup.resolve(repo: path, sha: String(newest.prefix(8)), to: String(middle.prefix(8)), known: []), .found(range)) + + // Two same-name forks hold the older end; only the second holds the newer one, so the range opens there. + let stale = path + "-stale" + XCTAssertNotNil(WorktreeDiscovery.git(path, ["clone", "-q", "--no-local", path, stale])) + defer { try? FileManager.default.removeItem(atPath: stale) } + XCTAssertNotNil(WorktreeDiscovery.git(path, commit)) + let latest = try XCTUnwrap(WorktreeDiscovery.git(path, ["rev-parse", "HEAD"])?.trimmed) + let forks = [HubPRProject(project: "https://example.test/old/app", repo: "app", root: stale, kind: "gitlab"), + HubPRProject(project: "https://example.test/me/app", repo: "app", root: path, kind: "gitlab")] + guard case .found(let ranged) = HubCommitLookup.resolve(repo: "group/sub/app", sha: String(middle.prefix(8)), + to: String(latest.prefix(8)), known: forks) else { + return XCTFail("the fork holding both ends of the range must open it") + } + XCTAssertEqual(ranged.root, root) + XCTAssertEqual(ranged.sha, latest) } @MainActor From a5ee2c5be7c789aca4badba36e59520f880d74e5 Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 21:47:31 +0200 Subject: [PATCH 10/11] fix(GenesisHub): #557 followup - a link opens the fork holding its commit, a root-commit range loads, page URLs pick the clone named like the project, full GitLab commit pages, and a failed scan or detail is said --- src/hub/lib/pr/pr.test.ts | 33 ++++++++++++ src/hub/lib/pr/project-ref.ts | 41 +++++++++++---- src/hub/lib/prs.ts | 18 +++++-- .../Diff/Source/GitWorkingTreeSource.swift | 7 +++ .../Sources/Parts/Hub/Hub/HubCommitLink.swift | 28 +++++++--- .../Sources/Parts/Hub/Hub/HubPRs.swift | 41 +++++++++------ .../GenesisTools/Tests/HubLogicTests.swift | 52 +++++++++++++++++-- .../Tests/ReviewVersionsTests.swift | 27 ++++++++++ src/utils/git/origins/detector.test.ts | 16 ++++++ src/utils/git/origins/prs.ts | 15 ++++-- 10 files changed, 231 insertions(+), 47 deletions(-) diff --git a/src/hub/lib/pr/pr.test.ts b/src/hub/lib/pr/pr.test.ts index 5faabf57fd..209e5f08e4 100644 --- a/src/hub/lib/pr/pr.test.ts +++ b/src/hub/lib/pr/pr.test.ts @@ -9,6 +9,7 @@ import type { CommandRunner } from "@genesiscz/utils/git/origins"; import type { RepoFacts } from "@genesiscz/utils/git/repo-facts"; import { SafeJSON } from "@genesiscz/utils/json"; import { Storage } from "@genesiscz/utils/storage"; +import { localCheckoutOf } from "../prs"; import { draftDatesFor, stampDraftDates } from "./draft-dates"; import { findBranchPr, findPrByRef } from "./find"; import { githubBackend, githubThread, githubVersions } from "./github"; @@ -1367,6 +1368,38 @@ describe("resolveHubPrRef", () => { expect(await resolveHubPrRef("acme/site#4", { source: clones })).toEqual({ path: "/work/site", number: 4 }); }); + test("a page URL opens the same clone a named ref does: the folder named like the project", async () => { + const clones = async () => ({ + roots: ["/work"], + checkouts: [ + checkout("/work/aaa/site-v2", "github.com", "acme/site"), + checkout("/work/site", "github.com", "acme/site"), + ], + }); + expect( + await localCheckoutOf( + { kind: "github", host: "github.com", path: "acme/site", web: "https://github.com/acme/site" }, + clones + ) + ).toBe("/work/site"); + }); + + test("a folder written as a path takes ! as a folder, never as a project name", async () => { + expect(await resolveHubPrRef("./app!12", { source })).toEqual({ path: "./app", number: 12 }); + expect(await resolveHubPrRef("~/work/app!12", { source })).toEqual({ path: "~/work/app", number: 12 }); + }); + + test("a failed checkout scan leaves owner/repo#42 a github.com PR and says why a project is missing", async () => { + const broken = async (): Promise<{ roots: string[]; checkouts: LocalCheckout[] }> => { + throw new Error("config unreadable"); + }; + expect(await resolveHubPrRef("acme/other#5", { source: broken })).toEqual({ + url: "https://github.com/acme/other/pull/5", + }); + const missing = await resolveHubPrRef("group/none!3", { source: broken }); + expect("problem" in missing && missing.problem).toContain("no checkout of group/none"); + }); + test("a URL never scans the checkouts", async () => { const never = async () => { throw new Error("scanned"); diff --git a/src/hub/lib/pr/project-ref.ts b/src/hub/lib/pr/project-ref.ts index 3aa3e6906c..df78acc8ab 100644 --- a/src/hub/lib/pr/project-ref.ts +++ b/src/hub/lib/pr/project-ref.ts @@ -15,7 +15,7 @@ export interface CheckoutSource { checkouts: LocalCheckout[]; } -async function liveCheckouts(): Promise { +export async function liveCheckouts(): Promise { const config = await loadConfig(); return { roots: config.repoRoots, checkouts: configuredCheckouts(config) }; } @@ -33,12 +33,14 @@ export async function resolveHubPrRef( const trimmed = ref.trim(); const named = /^([^\s#!]+)([#!])(\d+)$/.exec(trimmed); - if ( - !named || - /^https?:\/\//i.test(trimmed) || - /^[~./]/.test(named[1]) || - (named[2] === "#" && existsSync(named[1])) - ) { + const url = /^https?:\/\//i.test(trimmed); + + // A folder named like `./app!12` is that folder, as `./app#12` is: `parsePrRef` reads only the `#` form. + if (named && !url && named[2] === "!" && /^[~./]/.test(named[1])) { + return { path: named[1], number: Number(named[3]) }; + } + + if (!named || url || /^[~./]/.test(named[1]) || (named[2] === "#" && existsSync(named[1]))) { return ( parsePrRef(trimmed) ?? { problem: `expected a PR/MR URL, # or !, got "${ref}"`, @@ -47,7 +49,16 @@ export async function resolveHubPrRef( } const [, name, sign, digits] = named; - const { roots, checkouts } = await source(); + let roots: string[] = []; + let checkouts: LocalCheckout[] = []; + + // A config or scan failure must not break `owner/repo#42`, which needs no checkout to read as a github.com PR. + try { + ({ roots, checkouts } = await source()); + } catch (err) { + log.warn({ err, ref }, "hub pr: could not list the local checkouts; reading the ref without them"); + } + const found = checkoutOf(name, checkouts); if ("root" in found) { @@ -93,12 +104,20 @@ function checkoutOf(name: string, checkouts: LocalCheckout[]): { root: string } return { projects }; } - // Of two clones of one project, the folder named like the project (`app`, not `app-v2`). - const ranked = rankCheckouts({ project: pool[0].project, checkouts: pool }); - const best = ranked.find((checkout) => namedLikeProject(checkout.root, checkout.project.path)) ?? ranked[0]; + const best = preferredCheckout(rankCheckouts({ project: pool[0].project, checkouts: pool })); return best ? { root: best.root } : { projects }; } +/** + * The checkout a link of one project opens, from its checkouts best first (`rankCheckouts`): a main checkout + * before a worktree, and of two clones the folder named like the project (`app`, not `app-v2`). + */ +export function preferredCheckout(ranked: LocalCheckout[]): LocalCheckout | undefined { + const mains = ranked.filter((checkout) => checkout.isMain); + const pool = mains.length > 0 ? mains : ranked; + return pool.find((checkout) => namedLikeProject(checkout.root, checkout.project.path)) ?? pool[0]; +} + /** Whether a checkout's folder carries its project's name (the last segment of the web path), in any case. */ export function namedLikeProject(root: string, web: string): boolean { const name = web.replace(/\/+$/, "").split("/").pop() ?? ""; diff --git a/src/hub/lib/prs.ts b/src/hub/lib/prs.ts index a9fc4a4b1b..b3becd0cb1 100644 --- a/src/hub/lib/prs.ts +++ b/src/hub/lib/prs.ts @@ -30,7 +30,13 @@ import { logger } from "@genesiscz/utils/logger"; import { profiler } from "@genesiscz/utils/profile"; import { cached } from "@genesiscz/utils/storage/cache-flag"; import { Storage } from "@genesiscz/utils/storage/storage"; -import { namedLikeProject, resolveHubPrRef } from "./pr/project-ref"; +import { + type CheckoutSource, + liveCheckouts, + namedLikeProject, + preferredCheckout, + resolveHubPrRef, +} from "./pr/project-ref"; import { type ProposalSummary, proposalFor } from "./proposal"; /** The hub's PR row: the host's PR plus where it lives on this machine. */ @@ -507,7 +513,10 @@ export async function hubPrProjects(): Promise { } /** The main checkout of a forge project under the configured repo roots (the browser extension's). */ -async function localCheckoutOf(project: ProjectRef): Promise { +export async function localCheckoutOf( + project: ProjectRef, + source: () => Promise = liveCheckouts +): Promise { const wanted = checkoutProjectRef(`https://${project.host}/${project.path}`); if (!wanted) { @@ -515,8 +524,9 @@ async function localCheckoutOf(project: ProjectRef): Promise { } try { - const ranked = rankCheckouts({ project: wanted, checkouts: configuredCheckouts(await loadConfig()) }); - const root = ranked.find((checkout) => checkout.isMain)?.root ?? ranked[0]?.root ?? null; + // The same checkout a named ref (`app!12`) opens: of two clones, the folder named like the project. + const ranked = rankCheckouts({ project: wanted, checkouts: (await source()).checkouts }); + const root = preferredCheckout(ranked)?.root ?? null; log.debug({ project: wanted, root, candidates: ranked.length }, "hub pr: local checkout of a URL ref"); return root; } catch (err) { diff --git a/src/macos/GenesisKit/Sources/GenesisKit/Diff/Source/GitWorkingTreeSource.swift b/src/macos/GenesisKit/Sources/GenesisKit/Diff/Source/GitWorkingTreeSource.swift index 4c1cd03129..4ea42f6599 100644 --- a/src/macos/GenesisKit/Sources/GenesisKit/Diff/Source/GitWorkingTreeSource.swift +++ b/src/macos/GenesisKit/Sources/GenesisKit/Diff/Source/GitWorkingTreeSource.swift @@ -223,6 +223,13 @@ public struct GitWorkingTreeSource { case .range(let preferred, let head, _, let fallback): // A newer push of a PR (the window's Reload) is fetched by id when no ref brought it yet. if Self.isFullObjectID(head) { try? ensureCommit(head) } + // A range from a root commit (a link's `..`): every file of the head is added. + if preferred == Self.emptyTree { + guard (try? git(["cat-file", "-e", "\(head)^{commit}"])) != nil else { + throw ReviewError.git("commit \(head.prefix(10)) is not in \(repo.path); fetch the PR branch first (git fetch origin )") + } + return (Self.emptyTree, head, [Self.emptyTree, head], false, nil) + } let base = [preferred, fallback].compactMap { $0 }.first { (try? git(["cat-file", "-e", "\($0)^{commit}"])) != nil } ?? preferred for sha in [base, head] where (try? git(["cat-file", "-e", "\(sha)^{commit}"])) == nil { throw ReviewError.git("commit \(sha.prefix(10)) is not in \(repo.path); fetch the PR branch first (git fetch origin )") diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift index 1915518b38..83b6085ed6 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift @@ -107,7 +107,11 @@ enum HubCommitLookup { } catch { return .failed("Could not list the projects to find \(text): \(error)") } - guard let root = PRsModel.projectRoot(text, in: projects) ?? sameName(text, holding: [sha] + (to.map { [$0] } ?? []), in: projects) else { + let shas = [sha] + (to.map { [$0] } ?? []) + // The checkout of the path the link names, while it holds the link's commits; else a fork that does. + // With neither, that checkout still answers, so the miss names where the commit was looked for. + let exact = PRsModel.projectRoot(text, in: projects) + guard let root = exact.flatMap({ holds($0, shas) ? $0 : nil }) ?? sameName(text, holding: shas, in: projects) ?? exact else { return .failed("No checkout of \(text) under the repo roots, so commit \(sha) cannot open") } folder = root @@ -136,10 +140,20 @@ enum HubCommitLookup { guard let (older, newer) = ordered else { return .failed("\(sha) and \(to) are not on one line of history in \(place), so they make no range") } - let base = WorktreeDiscovery.git(root, ["rev-parse", "--verify", "--quiet", "\(older)^"])?.trimmed - let count = WorktreeDiscovery.git(root, ["rev-list", "--count", "\(older)^..\(newer)"])?.trimmed ?? "?" + let parent = WorktreeDiscovery.git(root, ["rev-parse", "--verify", "--quiet", "\(older)^"])?.trimmed + let base = parent?.isEmpty == false ? parent : nil + // No parent: a root commit starts from the empty tree, but a shallow clone only hides the parent. + if base == nil, WorktreeDiscovery.git(root, ["rev-parse", "--is-shallow-repository"])?.trimmed != "false" { + return .failed("The parent of \(older.prefix(10)) is not in \(place), a shallow clone (fetch more history first)") + } + let count = WorktreeDiscovery.git(root, ["rev-list", "--count", base.map { "\($0)..\(newer)" } ?? newer])?.trimmed ?? "?" let label = "\(older.prefix(10))..\(newer.prefix(10)) (\(count) commits)" - return .found(Found(root: root, sha: newer, title: label, base: base?.isEmpty == false ? base : emptyTree)) + return .found(Found(root: root, sha: newer, title: label, base: base ?? emptyTree)) + } + + /// Whether the checkout at `root` holds every one of `shas` as a commit. + static func holds(_ root: String, _ shas: [String]) -> Bool { + shas.allSatisfy { sha in WorktreeDiscovery.git(root, ["cat-file", "-e", "\(sha)^{commit}"]) != nil } } /// A fork or a mirror: the link names one path (the upstream `group/sub/app`), the checkout clones another @@ -154,13 +168,11 @@ enum HubCommitLookup { let wanted = name(repo) guard !wanted.isEmpty else { return nil } return projects.first { candidate in - name(candidate.project) == wanted && shas.allSatisfy { sha in - WorktreeDiscovery.git(candidate.root, ["cat-file", "-e", "\(sha)^{commit}"]) != nil - } + name(candidate.project) == wanted && holds(candidate.root, shas) }?.root } - /// Git's empty tree: the base of a range that starts at a root commit. + /// Git's empty tree: the base of a range that starts at a root commit (`GitWorkingTreeSource` diffs from it). static let emptyTree = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" } diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift index e7a5a81ea5..6fcf5fa880 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift @@ -87,10 +87,11 @@ struct HubPRRef: Equatable { /// What `tools hub pr show` takes: the page, else `!` / `#`, which the CLI /// looks up among every checkout under the repo roots (and reads an unknown `owner/repo#n` as a - /// github.com PR). Nil for a bare number: only the list's rows can name its project. + /// github.com PR). Nil for a bare number: only the list's rows can name its project. Nil for a project + /// starting with `-` too: the CLI's parser would read `--flag#12` as an option, not as the ref. var showArg: String? { if let pageURL { return pageURL } - guard let project else { return nil } + guard let project, !project.hasPrefix("-") else { return nil } return "\(project)\(isMergeRequest ? "!" : "#")\(number)" } @@ -339,6 +340,9 @@ final class PRsModel: ObservableObject { private(set) var pendingCommit: PRCommitPick? /// The commit row the overview scrolls to, set when a link selected it. @Published private(set) var commitScroll: PRCommitScroll? + /// The range a link's `commit=..` set, so the commits list marks its commits even when it spans + /// the whole PR; any other scope (the whole PR, one commit) is not it. + @Published private(set) var linkedRange: DiffScope? /// What a link asked of a PR and could not get (its commit is not in the PR), in that PR's header until /// dismissed or another PR opens. Not in `errors`: every list load replaces those. @Published var notice: PRNotice? @@ -749,6 +753,7 @@ final class PRsModel: ObservableObject { HubDefaults.store.set(false, forKey: "hub.prs.fold.commits") commitScroll = PRCommitScroll(prID: pr.id, sha: commits[max(a, b)].sha) let scope = Self.rangeScope(commits, first: min(a, b), last: max(a, b), pr: pr, whole: Self.scope(pr, detail: details[pr.id], fetch: fetchedHead(pr))) + linkedRange = scope if review.scope == scope { return } review.setScope(scope) } @@ -767,12 +772,13 @@ final class PRsModel: ObservableObject { return .range(base: "\(commits[first].sha)^", head: head, label: label) } - /// The commits of `commits` a range scope shows (`rangeScope`), for the commits list to mark; empty for - /// one commit or the whole PR. - nonisolated static func rangeShas(_ scope: DiffScope, in commits: [HubPRDetail.Commit]) -> Set { - guard case .range(let base, let head, _, _) = scope, let last = commits.firstIndex(where: { $0.sha == head }) else { return [] } + /// The commits of `commits` the range a link set (`linked`, from `rangeScope`) shows, for the commits list + /// to mark, all of them for a range over the whole PR; empty for any scope the link did not set. + nonisolated static func rangeShas(_ scope: DiffScope, linked: DiffScope?, in commits: [HubPRDetail.Commit]) -> Set { + guard scope == linked, case .range(let base, let head, _, _) = scope, + let last = commits.firstIndex(where: { $0.sha == head }) else { return [] } let first = commits.firstIndex(where: { $0.sha == base }).map { $0 + 1 } ?? 0 - guard first <= last, first > 0 || last < commits.count - 1 else { return [] } + guard first <= last else { return [] } return Set(commits[first...last].map(\.sha)) } @@ -786,6 +792,17 @@ final class PRsModel: ObservableObject { review.setScope(.commit(sha: commit.sha, title: commit.title)) } + /// The detail of `pr` could not load: a link's commit waiting on it is said, not left waiting, unless the + /// cached commits hold it (a cached list may already have selected it), both ends of a range. + func detailFailed(_ pr: HubPR, cached commits: [HubPRDetail.Commit]?) { + guard let pending = pendingCommit, pending.ref.matches(pr) else { return } + pendingCommit = nil + let ends = [pending.sha] + (pending.to.map { [$0] } ?? []) + if let missing = ends.first(where: { end in commits.flatMap { HubCommitSha.find(end, in: $0) } == nil }) { + notice = PRNotice(ref: pending.ref, text: "Could not load the commits of \(pr.label) to select \(missing)") + } + } + /// A settled miss replaces any previously selected commit with the whole PR before saying so. func showMissingCommit(_ pending: PRCommitPick, missing: String? = nil, in shown: HubPR, review: ReviewModel) { pendingCommit = nil @@ -1295,13 +1312,7 @@ final class PRsModel: ObservableObject { if let detail = details[key] { sessions.load(pr, detail: detail) } - if let pending = pendingCommit, pending.ref.matches(pr) { - // Said, not left waiting: a cached list may already have selected it, nothing else will. - pendingCommit = nil - if details[key]?.commits.flatMap({ HubCommitSha.find(pending.sha, in: $0) }) == nil { - notice = PRNotice(ref: pending.ref, text: "Could not load the commits of \(pr.label) to select \(pending.sha)") - } - } + detailFailed(pr, cached: details[key]?.commits) return } let current = prs.first { $0.id == key } ?? pr @@ -2371,7 +2382,7 @@ struct PRCommitsSection: View { var body: some View { let shown = shownSha // A range link's commits, all marked. - let range = PRsModel.rangeShas(review.scope, in: commits) + let range = PRsModel.rangeShas(review.scope, linked: prs.linkedRange, in: commits) PRSection(title: "Commits", count: commits.count, folded: $folded, flat: true, trailing: shown == nil && range.isEmpty ? nil : AnyView( Button("Whole PR") { prs.showWholePR() } .buttonStyle(.genHoverPlain()) diff --git a/src/macos/GenesisTools/Tests/HubLogicTests.swift b/src/macos/GenesisTools/Tests/HubLogicTests.swift index 695e47c650..b61c3b114b 100644 --- a/src/macos/GenesisTools/Tests/HubLogicTests.swift +++ b/src/macos/GenesisTools/Tests/HubLogicTests.swift @@ -82,6 +82,7 @@ final class HubLogicTests: XCTestCase { XCTAssertEqual(HubPRRef("acme/app#42")?.showArg, "acme/app#42") XCTAssertEqual(HubPRRef("https://gitlab.example/group/app/-/merge_requests/3")?.showArg, "https://gitlab.example/group/app/-/merge_requests/3") XCTAssertNil(HubPRRef("#42")?.showArg) + XCTAssertNil(HubPRRef("--flag#12")?.showArg, "the CLI would read it as an option, not as the ref") XCTAssertTrue(PRsModel.notListed(HubPRRef("!7")!).text.contains("!7")) } @@ -162,12 +163,17 @@ final class HubLogicTests: XCTestCase { let whole = DiffScope.range(base: "base000", head: "d000000", label: "whole", fallbackBase: "origin/main") let middle = PRsModel.rangeScope(commits, first: 1, last: 2, pr: pr, whole: whole) XCTAssertEqual(middle, .range(base: "a000000", head: "c000000", label: "!7 b000000..c000000 (2 commits)")) - XCTAssertEqual(PRsModel.rangeShas(middle, in: commits), ["b000000", "c000000"]) + XCTAssertEqual(PRsModel.rangeShas(middle, linked: middle, in: commits), ["b000000", "c000000"]) let opening = PRsModel.rangeScope(commits, first: 0, last: 1, pr: pr, whole: whole) XCTAssertEqual(opening, .range(base: "base000", head: "b000000", label: "!7 a000000..b000000 (2 commits)", fallbackBase: "origin/main")) - XCTAssertEqual(PRsModel.rangeShas(opening, in: commits), ["a000000", "b000000"]) - XCTAssertEqual(PRsModel.rangeShas(whole, in: commits), [], "the whole PR marks no commit") - XCTAssertEqual(PRsModel.rangeShas(.commit(sha: "b000000", title: "b"), in: commits), []) + XCTAssertEqual(PRsModel.rangeShas(opening, linked: opening, in: commits), ["a000000", "b000000"]) + // A link over every commit of the PR marks them all; the whole PR itself, or another range, marks none. + let span = PRsModel.rangeScope(commits, first: 0, last: 3, pr: pr, whole: whole) + XCTAssertEqual(PRsModel.rangeShas(span, linked: span, in: commits), Set(commits.map(\.sha))) + XCTAssertEqual(PRsModel.rangeShas(whole, linked: span, in: commits), [], "the whole PR marks no commit") + XCTAssertEqual(PRsModel.rangeShas(whole, linked: nil, in: commits), []) + XCTAssertEqual(PRsModel.rangeShas(middle, linked: opening, in: commits), [], "only the range the link set") + XCTAssertEqual(PRsModel.rangeShas(.commit(sha: "b000000", title: "b"), linked: span, in: commits), []) } func testAForgePathFindsItsCheckoutInAnyCaseAndForm() { @@ -238,6 +244,19 @@ final class HubLogicTests: XCTestCase { } XCTAssertEqual(ranged.root, root) XCTAssertEqual(ranged.sha, latest) + + // The upstream itself is checked out but lacks the linked commit: the fork that holds it opens instead. + let upstream = [HubPRProject(project: "https://example.test/group/sub/app", repo: "app", root: stale, kind: "gitlab"), + HubPRProject(project: "https://example.test/me/app", repo: "app", root: path, kind: "gitlab")] + guard case .found(let viaFork) = HubCommitLookup.resolve(repo: "group/sub/app", sha: String(latest.prefix(8)), known: upstream) else { + return XCTFail("the fork holding the commit must open it when the upstream checkout lacks it") + } + XCTAssertEqual(viaFork.root, root) + // No checkout holds it: the miss names the upstream checkout, where the link pointed. + guard case .failed(let lacking) = HubCommitLookup.resolve(repo: "group/sub/app", sha: "0000000", known: upstream) else { + return XCTFail("a commit no checkout holds must be said") + } + XCTAssertTrue(lacking.contains("is not in"), lacking) } @MainActor @@ -435,6 +454,31 @@ final class HubLogicTests: XCTestCase { XCTAssertEqual(model.notice?.ref, missing.ref) } + /// A range link waits for a fresh detail when the cached one predates its newer end; when that detail + /// fails, the missing end is said, never dropped with the old diff left on screen. + @MainActor + func testAFailedDetailSaysTheMissingEndOfARange() throws { + let json = """ + {"repo":"app","repoRoot":"/fixture/app","origin":{"kind":"github","host":"github.com","web":"https://github.com/acme/app"}, + "number":7,"title":"t","state":"OPEN","draft":false,"author":"alice","headBranch":"feature","baseBranch":"develop", + "url":"https://github.com/acme/app/pull/7","labels":[],"reviewers":[],"headSha":"abcdef0123","crossRepository":false,"headRepo":null} + """ + let pr = try JSONDecoder().decode(HubPR.self, from: Data(json.utf8)) + let model = PRsModel() + // A bare ref with no rows or known project starts no CLI fetch in this logic test. + let ref = try XCTUnwrap(HubPRRef("7")) + let cached = [HubPRDetail.Commit(sha: "aaaaaaa111", title: "from", author: nil, authorLogin: nil, date: nil, body: nil)] + + model.request(ref, commit: "aaaaaaa..bbbbbbb") + model.detailFailed(pr, cached: cached) + XCTAssertNil(model.pendingCommit) + XCTAssertEqual(model.notice?.text, "Could not load the commits of #7 to select bbbbbbb") + + model.request(ref, commit: "aaaaaaa") + model.detailFailed(pr, cached: cached) + XCTAssertNil(model.notice, "a cached commit is already selectable, so nothing is missing") + } + func testALiveLaunchIsNotScripted() { let request = HubRequest(["--session", "abc", "--no-activate"]) XCTAssertFalse(request.isScripted) diff --git a/src/macos/GenesisTools/Tests/ReviewVersionsTests.swift b/src/macos/GenesisTools/Tests/ReviewVersionsTests.swift index 057b1794f9..1d41eb7662 100644 --- a/src/macos/GenesisTools/Tests/ReviewVersionsTests.swift +++ b/src/macos/GenesisTools/Tests/ReviewVersionsTests.swift @@ -87,6 +87,33 @@ final class ReviewVersionsTests: XCTestCase { XCTAssertEqual(snapshot.files.map(\.path), ["a.txt"]) } + /// A link's `commit=..`: the range starts from the empty tree and the diff loads it, every file + /// added. A shallow clone hides a parent without being a root, so its range is refused, not shown as added. + func testARangeFromTheRootCommitLoadsAgainstTheEmptyTree() throws { + try write("a.txt", "1\n") + let root = try commit("root") + try write("b.txt", "2\n") + let second = try commit("second") + guard case .found(let found) = HubCommitLookup.resolve(repo: repo.path, sha: root, to: second, known: []) else { + return XCTFail("both ends are in the checkout") + } + XCTAssertEqual(found.base, GitWorkingTreeSource.emptyTree) + XCTAssertTrue(found.title.hasSuffix("(2 commits)"), found.title) + let scope = DiffScope.range(base: try XCTUnwrap(found.base), head: found.sha, label: found.title) + let snapshot = try GitWorkingTreeSource(repo: repo).load(scope: scope) + XCTAssertEqual(snapshot.files.map(\.path).sorted(), ["a.txt", "b.txt"]) + + try write("c.txt", "3\n") + let third = try commit("third") + let shallow = repo.deletingLastPathComponent().appendingPathComponent("\(repo.lastPathComponent)-shallow") + defer { try? FileManager.default.removeItem(at: shallow) } + try git("clone", "-q", "--depth", "2", "file://\(repo.path)", shallow.path) + guard case .failed(let message) = HubCommitLookup.resolve(repo: shallow.path, sha: second, to: third, known: []) else { + return XCTFail("a shallow clone's oldest commit is no root commit") + } + XCTAssertTrue(message.contains("shallow"), message) + } + func testAPushFetchedByIdLeavesFetchHeadToAPullInTheSameCheckout() throws { try write("a.txt", "1\n") let base = try commit("base") diff --git a/src/utils/git/origins/detector.test.ts b/src/utils/git/origins/detector.test.ts index 5bb5a420e7..0fdae82e54 100644 --- a/src/utils/git/origins/detector.test.ts +++ b/src/utils/git/origins/detector.test.ts @@ -850,6 +850,22 @@ describe("GitLab MR list and view", () => { }); }); + it("asks for every page of an MR's commits, so the oldest one listed is the MR's first", async () => { + if (!project) { + throw new Error("fixture remote did not parse"); + } + + const asked: string[][] = []; + const runner: CommandRunner = async (cmd) => { + asked.push(cmd); + return { code: 0, stdout: cmd[4].includes("/commits") ? "[]" : "{}", stderr: "" }; + }; + await viewPr({ project, number: 9, runner }); + const commits = asked.find((cmd) => cmd[4].includes("/commits")); + expect(commits).toContain("--paginate"); + expect(asked.filter((cmd) => cmd.includes("--paginate"))).toHaveLength(1); + }); + it("keeps a commit's message body without the title line GitLab repeats", () => { const detail = parseGlabMrView({ mr: SafeJSON.stringify(SafeJSON.parse(GL_MRS, { strict: true })[0]), diff --git a/src/utils/git/origins/prs.ts b/src/utils/git/origins/prs.ts index 630e070b30..10046099f5 100644 --- a/src/utils/git/origins/prs.ts +++ b/src/utils/git/origins/prs.ts @@ -873,8 +873,8 @@ function glabProject(project: ProjectRef): string { return `projects/${encodeURIComponent(project.path)}`; } -function glabApi(project: ProjectRef, endpoint: string): string[] { - return ["glab", "api", "--hostname", project.host, endpoint]; +function glabApi(project: ProjectRef, endpoint: string, ...flags: string[]): string[] { + return ["glab", "api", "--hostname", project.host, endpoint, ...flags]; } // --------------------------------------------------------------------------- @@ -1281,10 +1281,15 @@ export async function viewPr({ } const base = `${glabProject(project)}/merge_requests/${number}`; + // Every page of commits: with only the newest 100, the oldest one listed read as the MR's first, and a + // range starting there diffed from the MR's base, taking in the commits before it. const [mr, commits, approvals, pipelines] = await Promise.all( - [base, `${base}/commits?per_page=100`, `${base}/approvals`, `${base}/pipelines`].map((endpoint) => - run({ cmd: glabApi(project, endpoint), cwd, runner }) - ) + [ + glabApi(project, base), + glabApi(project, `${base}/commits?per_page=100`, "--paginate"), + glabApi(project, `${base}/approvals`), + glabApi(project, `${base}/pipelines`), + ].map((cmd) => run({ cmd, cwd, runner })) ); if (mr.error) { From d73db2bd20bfdc2e721f4a4aa74d1afb4e97fb3e Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 23:51:40 +0200 Subject: [PATCH 11/11] fix(GenesisHub): #557 followup - a ~ folder ref opens the home checkout, and a deep-link failure survives a failed list load --- src/hub/lib/pr/pr.test.ts | 11 ++++++-- src/hub/lib/pr/project-ref.ts | 12 ++++++-- .../Sources/Parts/Hub/Hub/HubPRs.swift | 28 ++++++++++++++----- .../GenesisTools/Tests/HubLogicTests.swift | 22 +++++++++++++++ 4 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/hub/lib/pr/pr.test.ts b/src/hub/lib/pr/pr.test.ts index 209e5f08e4..69dee060e4 100644 --- a/src/hub/lib/pr/pr.test.ts +++ b/src/hub/lib/pr/pr.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import { createHash, randomUUID } from "node:crypto"; import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { homedir, tmpdir } from "node:os"; import { join } from "node:path"; import { defaultReviewCommentClient, type ReviewCommentClient } from "@app/github/lib/review-comments"; import type { LocalCheckout } from "@genesiscz/utils/git/local-checkouts"; @@ -1386,7 +1386,14 @@ describe("resolveHubPrRef", () => { test("a folder written as a path takes ! as a folder, never as a project name", async () => { expect(await resolveHubPrRef("./app!12", { source })).toEqual({ path: "./app", number: 12 }); - expect(await resolveHubPrRef("~/work/app!12", { source })).toEqual({ path: "~/work/app", number: 12 }); + expect(await resolveHubPrRef("~/work/app!12", { source })).toEqual({ + path: join(homedir(), "work/app"), + number: 12, + }); + expect(await resolveHubPrRef("~/work/app#12", { source })).toEqual({ + path: join(homedir(), "work/app"), + number: 12, + }); }); test("a failed checkout scan leaves owner/repo#42 a github.com PR and says why a project is missing", async () => { diff --git a/src/hub/lib/pr/project-ref.ts b/src/hub/lib/pr/project-ref.ts index df78acc8ab..cc53d7896e 100644 --- a/src/hub/lib/pr/project-ref.ts +++ b/src/hub/lib/pr/project-ref.ts @@ -5,6 +5,7 @@ import { configuredCheckouts } from "@app/browser-extension/lib/deps"; import { type LocalCheckout, rankCheckouts } from "@genesiscz/utils/git/local-checkouts"; import { parsePrRef } from "@genesiscz/utils/git/origins"; import { logger } from "@genesiscz/utils/logger"; +import { expandTilde } from "@genesiscz/utils/paths"; const log = logger.child({ component: "hub/pr/project-ref" }); @@ -37,12 +38,19 @@ export async function resolveHubPrRef( // A folder named like `./app!12` is that folder, as `./app#12` is: `parsePrRef` reads only the `#` form. if (named && !url && named[2] === "!" && /^[~./]/.test(named[1])) { - return { path: named[1], number: Number(named[3]) }; + return { path: expandTilde(named[1]), number: Number(named[3]) }; } if (!named || url || /^[~./]/.test(named[1]) || (named[2] === "#" && existsSync(named[1]))) { + const parsed = parsePrRef(trimmed); + + // `~/work/app#12` is a folder in the home directory: the checkout readers take `~` literally. + if (parsed && "path" in parsed) { + return { path: expandTilde(parsed.path), number: parsed.number }; + } + return ( - parsePrRef(trimmed) ?? { + parsed ?? { problem: `expected a PR/MR URL, # or !, got "${ref}"`, } ); diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift index 6fcf5fa880..15c7a88cc9 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift @@ -543,11 +543,28 @@ final class PRsModel: ObservableObject { } case .failure(let error): span.end("failed") - errors = [PRListNotice.failure("Could not list the PRs/MRs", error)] + listFailed(error) } } } + /// A list load answered with its `own` notices: they replace the last list's, and a deep link's failure + /// stays beside them until dismissed or the next link, the only word on why that link opened nothing. + func listAnswered(_ own: [PRListNotice]) { + errors = own + (directFailure.map { [$0] } ?? []) + } + + /// The list command failed: said in place of the last list's notices, beside a deep link's failure. + func listFailed(_ error: Error) { + listAnswered([PRListNotice.failure("Could not list the PRs/MRs", error)]) + } + + /// Why a deep link opened nothing, kept apart from the list's own notices (`listAnswered`). + func directFailed(_ notice: PRListNotice) { + directFailure = notice + errors.append(notice) + } + /// Selects the PR `ref` names, now when the list has it, else once it loads. `reveal` opens a file /// (and a thread on it) in that PR's review once the review exists: after the list, the head fetch /// of a PR without a worktree, and the diff load, so one click is enough. `commit` (a sha or a prefix @@ -663,16 +680,13 @@ final class PRsModel: ObservableObject { } else { notice = PRListNotice.failure("Could not open \(ref.label)", error) } - directFailure = notice - errors.append(notice) + directFailed(notice) return } let (row, detail, data) = fresh guard ref.matches(row) else { span.end("another PR") - let notice = PRListNotice("\(arg) answered \(row.label), not \(ref.label)") - directFailure = notice - errors.append(notice) + directFailed(PRListNotice("\(arg) answered \(row.label), not \(ref.label)")) return } span.end(row.label) @@ -1038,7 +1052,7 @@ final class PRsModel: ObservableObject { fullProjects = Set(list.repos.compactMap { repo in (repo.count ?? 0) >= limit ? repo.origin?.web : nil }) changed = moved } - errors = PRListNotice.failures(list.repos) + (directFailure.map { [$0] } ?? []) + listAnswered(PRListNotice.failures(list.repos)) SWR.fade(moved, current: { [weak self] in self?.changed }, clear: { [weak self] in self?.changed = [] }) } diff --git a/src/macos/GenesisTools/Tests/HubLogicTests.swift b/src/macos/GenesisTools/Tests/HubLogicTests.swift index b61c3b114b..d7f3136f11 100644 --- a/src/macos/GenesisTools/Tests/HubLogicTests.swift +++ b/src/macos/GenesisTools/Tests/HubLogicTests.swift @@ -479,6 +479,28 @@ final class HubLogicTests: XCTestCase { XCTAssertNil(model.notice, "a cached commit is already selectable, so nothing is missing") } + /// A deep link's failure is the only word on why the link opened nothing: a list load that lands after it, + /// answered or failed, replaces the list's own notices and keeps that one until it is dismissed. + @MainActor + func testADeepLinkFailureSurvivesAnsweredAndFailedListLoads() { + struct ListDown: LocalizedError { var errorDescription: String? { "gh: network down" } } + let model = PRsModel() + let direct = PRListNotice("Could not open #7: no such PR") + let stale = PRListNotice("app: rate limited") + model.listAnswered([stale]) + model.directFailed(direct) + + model.listAnswered([PRListNotice("web: Cannot resolve git.example.org")]) + XCTAssertEqual(model.errors.map(\.text), ["web: Cannot resolve git.example.org", direct.text]) + + model.listFailed(ListDown()) + XCTAssertEqual(model.errors.map(\.text), ["Could not list the PRs/MRs: gh: network down", direct.text]) + + model.dismissError(direct) + model.listFailed(ListDown()) + XCTAssertEqual(model.errors.map(\.text), ["Could not list the PRs/MRs: gh: network down"]) + } + func testALiveLaunchIsNotScripted() { let request = HubRequest(["--session", "abc", "--no-activate"]) XCTAssertFalse(request.isScripted)