diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index 8e33a5bec1007..1bcab04eb3eea 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -3,6 +3,7 @@ import { pullRequestAnchorMatchesUrl, pullRequestDiffPathMatches, pullRequestLinkCommit, + resolvePullRequestAnchorRange, resolvePullRequestReviewAnchor, } from "@t3tools/shared/pullRequestAnchor"; import { AppActivityIndicator } from "../../components/ui/AppActivityIndicator"; @@ -1072,7 +1073,15 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { : !jumpPath || requestedFile ? data.files[0] : undefined); - const range = pickedPath === null && linkAnchor?.kind === "diff" ? linkAnchor.range : null; + const linkRange = pickedPath === null && linkAnchor?.kind === "diff" ? linkAnchor.range : null; + const range = + linkRange && + resolvePullRequestAnchorRange(linkRange, (line) => + data.rows.some( + (row) => + row.fileId === targetFile?.id && row.change === "delete" && row.oldLineNumber === line, + ), + ); const focus = route.params.focus ?? (range diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 49eb9063c50c0..3ea861cbf9ffd 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -2,6 +2,7 @@ import { parsePullRequestAnchor, pullRequestDiffPathMatches, pullRequestLinkCommit, + resolvePullRequestAnchorRange, } from "@t3tools/shared/pullRequestAnchor"; import { capitalizeTerm, @@ -349,6 +350,18 @@ export interface PullRequestAgentSelectionInput { readonly request: string; } +function isRemovedLine(fileDiff: FileDiffMetadata, oldLine: number): boolean { + return fileDiff.hunks.some((hunk) => { + let next = hunk.deletionStart; + return hunk.hunkContent.some((content) => { + const count = content.type === "context" ? content.lines : content.deletions; + const removed = content.type === "change" && oldLine >= next && oldLine < next + count; + next += count; + return removed; + }); + }); +} + function PullRequestCodeTab({ scrollerRef, environmentId, @@ -1556,7 +1569,12 @@ function PullRequestCodeTab({ answeredLink.current = key; setRevealedLink({ scope: scopeKey, path: pendingLinkPath }); onRevealSettled?.(revealUrl); - const range = linkAnchor.range; + const range = + linkAnchor.range && + resolvePullRequestAnchorRange( + linkAnchor.range, + (line) => linkFile !== undefined && isRemovedLine(linkFile, line), + ); revealFile( pendingLinkPath, range diff --git a/packages/shared/src/pullRequestAnchor.test.ts b/packages/shared/src/pullRequestAnchor.test.ts index 5a94201fc61cc..6ea826924a992 100644 --- a/packages/shared/src/pullRequestAnchor.test.ts +++ b/packages/shared/src/pullRequestAnchor.test.ts @@ -4,9 +4,12 @@ import { pullRequestAnchorMatchesUrl, pullRequestDiffPathMatches, pullRequestLinkCommit, + resolvePullRequestAnchorRange, resolvePullRequestReviewAnchor, } from "./pullRequestAnchor.ts"; +const gitlab = "https://gitlab.com/group/repo/-/merge_requests/7"; + const url = "https://github.com/owner/repo/pull/7"; it.each(["discussion_r123", "r123", "discussion-diff-123R5-L9", "discussion-diff-123"])( "resolves review comment alias %s", @@ -53,19 +56,22 @@ it.each([ "src/hello world.ts", "bcea8deb9d67b305e28c4a0b0d021615", "066802cf2cf056706e4e4951cb7dff2717a2ae32fd9797c1f1b41062d57db929", + "f3f78c2026bd34f4ad36bf36e9c557dd9df528fb", ], [ "src/日本語.ts", "5c66d5d565666e46924cf80d7bbfdab3", "191b8ebf2241225db999eaf8b6ff44b07934d577f347d0ff684591eecad34944", + "2ec8ce696c4c6df9419ad640fbc0d6ef4d79ebad", ], [ "old/path.ts", "f5d455478d496859b654884654393a61", "aa38d67014e2c59d51ea456a155e039f4e32f636a4c450714c3b517bb8bde7e7", + "ca0d69af01140f25bafd6ab7aec204ce52a9bbb3", ], -])("matches GitHub path digests for %s", (path, legacy, current) => { - for (const digest of [legacy, current]) { +])("matches GitHub and GitLab path digests for %s", (path, legacy, current, gitLab) => { + for (const digest of [legacy, current, gitLab, gitLab.slice(0, 9)]) { expect(pullRequestDiffPathMatches(digest, path)).toBe(true); expect(pullRequestDiffPathMatches(digest, "other.ts")).toBe(false); } @@ -77,6 +83,19 @@ it("handles encoded hashes and rejects invalid line numbers and URLs", () => { }); expect(parsePullRequestAnchor(`${url}#diff-abcR0`)).toBeNull(); expect(parsePullRequestAnchor(`${url}#diff-abcR9999999999999999999`)).toBeNull(); + const sha = "c".repeat(40); + for (const anchor of [ + `${sha}_0_0`, + `${sha}_1_9999999999999999999`, + `line_${sha.slice(0, 9)}_0`, + ]) { + expect(parsePullRequestAnchor(`${gitlab}/diffs#${anchor}`)).toMatchObject({ range: null }); + } + expect(parsePullRequestAnchor(`${gitlab}/diffs#${sha}_0_5`)).toEqual({ + kind: "diff", + digest: sha, + range: { start: 5, side: "right", end: 5, endSide: "right" }, + }); expect(parsePullRequestAnchor("invalid")).toBeNull(); expect(parsePullRequestAnchor(url)).toBeNull(); expect(parsePullRequestAnchor(`${url}#unknown`)).toEqual({ kind: "summary", anchor: "unknown" }); @@ -89,9 +108,44 @@ it("keeps the commit scope of a PR diff permalink", () => { const sha = "a".repeat(40); expect(pullRequestLinkCommit(`${url}/commits/${sha}#diff-abcR5`)).toBe(sha); expect(pullRequestLinkCommit(`${url}/files#diff-abcR5`)).toBeNull(); + expect(pullRequestLinkCommit(`${gitlab}/diffs?commit_id=${sha}#${"b".repeat(40)}`)).toBe(sha); + expect(pullRequestLinkCommit(`${gitlab}/diffs?commit_id=main`)).toBeNull(); + expect(pullRequestLinkCommit(`${gitlab}?commit_id=${sha}`)).toBeNull(); +}); + +it("reads GitLab file and line anchors, letting a removed old line win", () => { + const digest = "c".repeat(40); + const diff = (range: object | null, at = digest) => ({ kind: "diff", digest: at, range }); + const line = (start: number, side: "left" | "right") => ({ + start, + side, + end: start, + endSide: side, + }); + expect(parsePullRequestAnchor(`${gitlab}/diffs#${digest}`)).toEqual(diff(null)); + expect(parsePullRequestAnchor(`${gitlab}/diffs#diff-content-${digest}`)).toEqual(diff(null)); + expect(parsePullRequestAnchor(`${gitlab}/diffs?file=${digest}`)).toEqual(diff(null)); + expect(parsePullRequestAnchor(`${gitlab}/diffs?file=${digest}#note_5`)).toEqual({ + kind: "timeline", + anchor: "note_5", + }); + const code = parsePullRequestAnchor(`${gitlab}/diffs#${digest}_248_253`); + expect(code).toEqual(diff({ ...line(253, "right"), oldLine: 248 })); + const range = code?.kind === "diff" ? code.range : null; + expect(resolvePullRequestAnchorRange(range!, () => false)).toBe(range); + expect(resolvePullRequestAnchorRange(range!, (old) => old === 248)).toEqual(line(248, "left")); + expect(parsePullRequestAnchor(`${gitlab}/diffs#${digest}_12_0`)).toEqual(diff(line(12, "left"))); + expect(parsePullRequestAnchor(`${gitlab}/diffs#line_${digest.slice(0, 9)}_A12`)).toEqual( + diff(line(12, "right"), digest.slice(0, 9)), + ); + expect(parsePullRequestAnchor(`${gitlab}/diffs#line_${digest.slice(0, 9)}_12`)).toEqual( + diff(line(12, "left"), digest.slice(0, 9)), + ); + expect(parsePullRequestAnchor(`${gitlab}#${digest}_248_253`)).toEqual(code); + expect(parsePullRequestAnchor(`${gitlab}#${digest}`)).toEqual(diff(null)); + expect(parsePullRequestAnchor(`${gitlab}?file=${digest}`)).toBeNull(); }); -const gitlab = "https://gitlab.com/group/repo/-/merge_requests/7"; it.each([ [`${url}/files`, "code"], [`${url}/files/`, "code"], @@ -133,22 +187,3 @@ it("finds a review with no text of its own through the earliest comment it poste ); expect(resolvePullRequestReviewAnchor("issuecomment-9", [], threads)).toBe("issuecomment-9"); }); - -it("reads GitLab merge request file and line anchors", () => { - const path = "internal/commands/issuable/view/issuable_view_test.go"; - const digest = "0cd3702f060909d3161d1095b8347d4d07671427"; - expect(pullRequestDiffPathMatches(digest, path)).toBe(true); - expect(parsePullRequestAnchor(`${gitlab}/diffs#${digest}`)).toEqual({ - kind: "diff", - digest, - range: null, - }); - expect(parsePullRequestAnchor(`${gitlab}/diffs#${digest}_248_253`)).toEqual({ - kind: "diff", - digest, - range: { start: 253, side: "right", end: 253, endSide: "right" }, - }); - expect(parsePullRequestAnchor(`${gitlab}/diffs#${digest}_12_0`)).toMatchObject({ - range: { start: 12, side: "left" }, - }); -}); diff --git a/packages/shared/src/pullRequestAnchor.ts b/packages/shared/src/pullRequestAnchor.ts index a05907c5772d3..a16e7ced305a3 100644 --- a/packages/shared/src/pullRequestAnchor.ts +++ b/packages/shared/src/pullRequestAnchor.ts @@ -7,6 +7,8 @@ export interface PullRequestAnchorRange { readonly side: "left" | "right"; readonly end: number; readonly endSide: "left" | "right"; + /** A GitLab line code's old-side counter: the line it names when that old line was removed. */ + readonly oldLine?: number; } export type PullRequestAnchor = @@ -57,6 +59,55 @@ function pullRequestPathTab(pathname: string): PullRequestAnchor | null { } } +const isLine = (line: number) => Number.isSafeInteger(line) && line >= 1; + +function gitLabLine( + start: number, + side: "left" | "right", + oldLine = 0, +): PullRequestAnchorRange | null { + if (!isLine(start)) return null; + return { start, side, end: start, endSide: side, ...(isLine(oldLine) ? { oldLine } : {}) }; +} + +// GitLab names a file by its path's SHA-1 and a line by `__`, which carries both +// sides' counters. Rapid Diffs writes `line__` or `_A`. +function gitLabDiffAnchor(anchor: string, file: string | null): PullRequestAnchor | null { + const rapid = /^line_([a-f0-9]{9})_(A?)(\d+)$/i.exec(anchor); + if (rapid) { + const line = Number(rapid[3]); + return { + kind: "diff", + digest: (rapid[1] ?? "").toLowerCase(), + range: gitLabLine(line, rapid[2] ? "right" : "left"), + }; + } + const code = /^(?:diff-content-)?([a-f0-9]{40})(?:_(\d+)_(\d+))?$/i.exec(anchor); + // A comment anchor beside `?file=` names the comment. + const digest = + code?.[1] ?? (!anchor && file !== null && /^[a-f0-9]{40}$/i.test(file) ? file : null); + if (digest === null) return null; + const [oldLine, newLine] = [Number(code?.[2]), Number(code?.[3])]; + // A deleted file has no new side, so its new counter is 0. + const range = + code?.[2] === undefined + ? null + : newLine === 0 + ? gitLabLine(oldLine, "left") + : gitLabLine(newLine, "right", oldLine); + return { kind: "diff", digest: digest.toLowerCase(), range }; +} + +/** The range to select, once the diff shows whether a GitLab line code's old line was removed. */ +export function resolvePullRequestAnchorRange( + range: PullRequestAnchorRange, + isRemovedLine: (oldLine: number) => boolean, +): PullRequestAnchorRange { + return range.oldLine !== undefined && isRemovedLine(range.oldLine) + ? (gitLabLine(range.oldLine, "left") ?? range) + : range; +} + export function parsePullRequestAnchor(url: string | null | undefined): PullRequestAnchor | null { let parsed: URL; try { @@ -66,6 +117,13 @@ export function parsePullRequestAnchor(url: string | null | undefined): PullRequ } const fallback = pullRequestPathTab(parsed.pathname); const anchor = pullRequestUrlHash(url); + const gitLab = gitLabDiffAnchor( + anchor, + /\/-\/merge_requests\/\d+\/diffs\/?$/i.test(parsed.pathname) + ? parsed.searchParams.get("file") + : null, + ); + if (gitLab) return gitLab; if (!anchor) return fallback; // GitHub line ranges may cross diff sides; file digests were MD5 before SHA-256. const diff = /^diff-([a-f0-9]+)(?:([LR])(\d+)(?:-([LR])(\d+))?)?$/i.exec(anchor); @@ -90,19 +148,6 @@ export function parsePullRequestAnchor(url: string | null | undefined): PullRequ : null, }; } - // GitLab: SHA-1 of the path, then old and new line counters. A removed line still carries a - // new counter, so the new side wins unless the file has none (a 0 for a deleted file). - const gitlab = /^([a-f0-9]{40})(?:_(\d+)_(\d+))?$/i.exec(anchor); - if (gitlab) { - const digest = (gitlab[1] ?? "").toLowerCase(); - if (!gitlab[2]) return { kind: "diff", digest, range: null }; - const newLine = Number(gitlab[3]); - const oldLine = Number(gitlab[2]); - const side = newLine >= 1 ? "right" : "left"; - const line = side === "right" ? newLine : oldLine; - if (!Number.isSafeInteger(line) || line < 1) return fallback; - return { kind: "diff", digest, range: { start: line, side, end: line, endSide: side } }; - } if ( /^(?:(?:issuecomment-|pullrequestreview-|discussion_r|r|commitcomment-|event-|note_|comment-)\d+|discussion-diff-\d+(?:[LR]\d+(?:-[LR]\d+)?)?)$/i.test( anchor, @@ -149,14 +194,20 @@ export function resolvePullRequestReviewAnchor( export function pullRequestDiffPathMatches(digest: string, path: string): boolean { const bytes = utf8ToBytes(path); - const hash = digest.length === 32 ? md5 : digest.length === 40 ? sha1 : sha256; - return digest === bytesToHex(hash(bytes)); + // GitLab uses SHA-1, abbreviated to 9 characters in Rapid Diffs line anchors. + if (digest.length === 40 || digest.length === 9) + return bytesToHex(sha1(bytes)).startsWith(digest); + return digest === bytesToHex(digest.length === 32 ? md5(bytes) : sha256(bytes)); } export function pullRequestLinkCommit(url: string | null | undefined): string | null { if (!url) return null; try { - return /\/pull\/\d+\/commits\/([a-f0-9]{40})\/?$/i.exec(new URL(url).pathname)?.[1] ?? null; + const { pathname, searchParams } = new URL(url); + const commit = /\/-\/merge_requests\/\d+\/diffs\/?$/i.test(pathname) + ? searchParams.get("commit_id") + : /\/pull\/\d+\/commits\/([a-f0-9]{40})\/?$/i.exec(pathname)?.[1]; + return commit && /^[a-f0-9]{40}$/i.test(commit) ? commit : null; } catch { return null; }