From 9c55dcb514a952eb69ec657641eaf9bfdd104230 Mon Sep 17 00:00:00 2001 From: Seongmin Lee Date: Fri, 14 Aug 2026 18:02:21 +0900 Subject: [PATCH] web: pr page redesign Signed-off-by: Seongmin Lee --- web/src/lib/api/gitmirror.ts | 19 + web/src/lib/api/pull.ts | 57 +- web/src/lib/api/pullDiff.test.ts | 295 +++++--- web/src/lib/api/pullDiff.ts | 351 +++++---- web/src/lib/api/pullPage.ts | 136 ---- web/src/lib/api/pullRoute.test.ts | 80 +-- web/src/lib/api/pullRoute.ts | 79 +- web/src/lib/api/repo.ts | 9 + .../lib/components/comment/comments.test.ts | 147 ++++ web/src/lib/components/comment/comments.ts | 65 ++ .../repo/pulls/PullActions.stories.svelte | 19 - .../components/repo/pulls/PullActions.svelte | 336 ++------- .../repo/pulls/PullComment.stories.svelte | 49 -- .../components/repo/pulls/PullComment.svelte | 196 ----- .../repo/pulls/PullCommitList.stories.svelte | 45 -- .../repo/pulls/PullCommitList.svelte | 83 --- .../repo/pulls/PullCommitListDiff.svelte | 50 ++ .../repo/pulls/PullCommitListHeader.svelte | 40 ++ .../repo/pulls/PullCommitListInterdiff.svelte | 111 +++ .../repo/pulls/PullCommitRow.svelte | 90 +++ .../lib/components/repo/pulls/PullDiff.svelte | 124 ++++ .../repo/pulls/PullDiffFileCard.svelte | 119 +++ .../repo/pulls/PullDiffFileList.svelte | 104 +++ .../repo/pulls/PullDiffFiles.svelte | 119 --- .../repo/pulls/PullDiffPanel.stories.svelte | 50 -- .../repo/pulls/PullDiffPanel.svelte | 272 ------- .../repo/pulls/PullDiscussion.stories.svelte | 85 --- .../repo/pulls/PullDiscussion.svelte | 482 ++----------- .../repo/pulls/PullInfoBar.stories.svelte | 36 - .../components/repo/pulls/PullInfoBar.svelte | 88 --- .../repo/pulls/PullMetaPanel.svelte | 190 ----- .../pulls/PullReviewComment.stories.svelte | 33 + .../repo/pulls/PullReviewComment.svelte | 110 +++ .../repo/pulls/PullReviewCommentEditor.svelte | 80 +++ .../PullReviewCommentForm.stories.svelte | 51 ++ .../repo/pulls/PullReviewCommentForm.svelte | 61 ++ .../repo/pulls/PullVersionList.stories.svelte | 70 ++ .../repo/pulls/PullVersionList.svelte | 88 +++ .../repo/tickets/Ticket.stories.svelte | 81 +++ .../lib/components/repo/tickets/Ticket.svelte | 113 +++ .../components/repo/tickets/TicketBody.svelte | 17 + .../repo/tickets/TicketForm.stories.svelte | 57 ++ .../components/repo/tickets/TicketForm.svelte | 99 +++ .../repo/tickets/TicketInfoBar.svelte | 44 ++ .../tickets/TicketStatePill.stories.svelte | 27 + .../repo/tickets/TicketStatePill.svelte | 64 ++ .../components/ui/GraphCell.stories.svelte | 125 ++++ web/src/lib/components/ui/GraphCell.svelte | 105 +++ .../ui/RangeSelector.stories.svelte | 68 ++ .../lib/components/ui/RangeSelector.svelte | 244 +++++++ .../lib/components/ui/RangeSelectorRow.svelte | 31 + .../components/ui/SplitButton.stories.svelte | 45 ++ web/src/lib/components/ui/SplitButton.svelte | 84 +++ web/src/lib/components/ui/TabPanel.svelte | 2 +- web/src/lib/components/ui/Tag.svelte | 1 + .../[handle]/[repo]/pulls/[aturi]/+layout.ts | 81 +++ .../[aturi]/[version]/[[range]]/+page.svelte | 677 +++++++++++------- .../[aturi]/[version]/[[range]]/+page.ts | 128 +--- 58 files changed, 3743 insertions(+), 2769 deletions(-) delete mode 100644 web/src/lib/api/pullPage.ts create mode 100644 web/src/lib/components/comment/comments.test.ts delete mode 100644 web/src/lib/components/repo/pulls/PullActions.stories.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullComment.stories.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullComment.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullCommitList.stories.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullCommitList.svelte create mode 100644 web/src/lib/components/repo/pulls/PullCommitListDiff.svelte create mode 100644 web/src/lib/components/repo/pulls/PullCommitListHeader.svelte create mode 100644 web/src/lib/components/repo/pulls/PullCommitListInterdiff.svelte create mode 100644 web/src/lib/components/repo/pulls/PullCommitRow.svelte create mode 100644 web/src/lib/components/repo/pulls/PullDiff.svelte create mode 100644 web/src/lib/components/repo/pulls/PullDiffFileCard.svelte create mode 100644 web/src/lib/components/repo/pulls/PullDiffFileList.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullDiffFiles.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullDiffPanel.stories.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullDiffPanel.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullDiscussion.stories.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullInfoBar.stories.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullInfoBar.svelte delete mode 100644 web/src/lib/components/repo/pulls/PullMetaPanel.svelte create mode 100644 web/src/lib/components/repo/pulls/PullReviewComment.stories.svelte create mode 100644 web/src/lib/components/repo/pulls/PullReviewComment.svelte create mode 100644 web/src/lib/components/repo/pulls/PullReviewCommentEditor.svelte create mode 100644 web/src/lib/components/repo/pulls/PullReviewCommentForm.stories.svelte create mode 100644 web/src/lib/components/repo/pulls/PullReviewCommentForm.svelte create mode 100644 web/src/lib/components/repo/pulls/PullVersionList.stories.svelte create mode 100644 web/src/lib/components/repo/pulls/PullVersionList.svelte create mode 100644 web/src/lib/components/repo/tickets/Ticket.stories.svelte create mode 100644 web/src/lib/components/repo/tickets/Ticket.svelte create mode 100644 web/src/lib/components/repo/tickets/TicketBody.svelte create mode 100644 web/src/lib/components/repo/tickets/TicketForm.stories.svelte create mode 100644 web/src/lib/components/repo/tickets/TicketForm.svelte create mode 100644 web/src/lib/components/repo/tickets/TicketInfoBar.svelte create mode 100644 web/src/lib/components/repo/tickets/TicketStatePill.stories.svelte create mode 100644 web/src/lib/components/repo/tickets/TicketStatePill.svelte create mode 100644 web/src/lib/components/ui/GraphCell.stories.svelte create mode 100644 web/src/lib/components/ui/GraphCell.svelte create mode 100644 web/src/lib/components/ui/RangeSelector.stories.svelte create mode 100644 web/src/lib/components/ui/RangeSelector.svelte create mode 100644 web/src/lib/components/ui/RangeSelectorRow.svelte create mode 100644 web/src/lib/components/ui/SplitButton.stories.svelte create mode 100644 web/src/lib/components/ui/SplitButton.svelte create mode 100644 web/src/routes/[handle]/[repo]/pulls/[aturi]/+layout.ts diff --git a/web/src/lib/api/gitmirror.ts b/web/src/lib/api/gitmirror.ts index faa83692d..900245a68 100644 --- a/web/src/lib/api/gitmirror.ts +++ b/web/src/lib/api/gitmirror.ts @@ -1,6 +1,8 @@ import { jsonGet } from "./_request"; import { splitMessage, type CommitSummary } from "./repo"; +import type { Did } from "@atcute/lexicons/syntax"; import type { BobbinContext, XrpcRequestInit } from "./client"; +import type { Revspec } from "./pullRoute"; import type * as ShTangledGitDefs from "./lexicons/types/sh/tangled/git/defs"; import type * as GetDiff from "./lexicons/types/sh/tangled/git/temp2/getDiff"; import type * as GetInterdiff from "./lexicons/types/sh/tangled/git/temp2/getInterdiff"; @@ -22,6 +24,23 @@ export const listCommits = ( init?: XrpcRequestInit ) => jsonGet(ctx, LIST_COMMITS_NSID, params, init); +const LOG_LIMIT = 100; + +// every commit a pull version brought, newest first +export const listLog = async ( + ctx: BobbinContext, + repo: Did, + range: Revspec, + init?: XrpcRequestInit +): Promise => { + const page = await listCommits( + ctx, + { repo, ranges: [`${range.base}..${range.head}`], limit: LOG_LIMIT }, + init + ); + return page.commits.map(toMirrorCommitSummary); +}; + export type MirrorFileDiff = ShTangledGitDefs.FileDiff; export const getDiff = (ctx: BobbinContext, params: GetDiff.$params, init?: XrpcRequestInit) => diff --git a/web/src/lib/api/pull.ts b/web/src/lib/api/pull.ts index 7db0e8843..92de3ed1b 100644 --- a/web/src/lib/api/pull.ts +++ b/web/src/lib/api/pull.ts @@ -31,10 +31,11 @@ export const putPull = async ( return { uri, cid, value: record }; }; -export const editPull = async ( +/** read-modify-write against the live record */ +const updatePull = async ( agent: OAuthUserAgent, rkey: string, - patch: { title: string; body: string } + mutate: (record: PullRecord) => PullRecord ): Promise> => { const rpc = createClient(agent); const existing = await ok( @@ -46,7 +47,7 @@ export const editPull = async ( } }) ); - const record = { ...(existing.value as PullRecord), ...patch }; + const record = mutate(existing.value as PullRecord); const { uri, cid } = await ok( rpc.call(putRecordSchema, { input: { @@ -61,6 +62,19 @@ export const editPull = async ( return { uri, cid, value: record }; }; +export const editPull = ( + agent: OAuthUserAgent, + rkey: string, + ticket: { title: string; body: string } +): Promise> => updatePull(agent, rkey, (record) => ({ ...record, ...ticket })); + +export const resubmitPull = ( + agent: OAuthUserAgent, + rkey: string, + version: PullRecord["versions"][number] +): Promise> => + updatePull(agent, rkey, (record) => ({ ...record, versions: [...record.versions, version] })); + export const deletePull = async (agent: OAuthUserAgent, rkey: string): Promise => { const rpc = createClient(agent); await ok( @@ -139,3 +153,40 @@ export const keepCommit = async ( if (!response.ok) throw await toResponseError(response); return (await response.json()) as { commit: string }; }; + +const MERGE_COMMIT_NSID = "sh.tangled.git.mergeCommit"; + +export interface MergeCommitInput { + /** the repo and branch being merged into */ + target: { repo: string; branch: string }; + /** the repo the head commit lives in, and the commit itself */ + source: { repo: string; commit: string }; + style: "rebase"; +} + +/** Runs on the target repo's knot, which authorizes it with the repo's push acl. */ +export const mergeCommit = async ( + agent: OAuthUserAgent, + knot: string, + input: MergeCommitInput, + fetch: typeof globalThis.fetch = globalThis.fetch +): Promise => { + const token = await mintServiceAuth(agent, { + aud: serviceDidForHost(knot), + lxm: MERGE_COMMIT_NSID + }); + const response = await fetch(`https://${knot}/xrpc/${MERGE_COMMIT_NSID}`, { + method: "POST", + headers: { + "content-type": "application/json", + accept: "application/json", + authorization: `Bearer ${token}` + }, + body: JSON.stringify({ + target: { $type: `${MERGE_COMMIT_NSID}#target`, ...input.target }, + source: { $type: `${MERGE_COMMIT_NSID}#source`, ...input.source }, + style: input.style + }) + }); + if (!response.ok) throw await toResponseError(response); +}; diff --git a/web/src/lib/api/pullDiff.test.ts b/web/src/lib/api/pullDiff.test.ts index fa1d76fbe..18946fd00 100644 --- a/web/src/lib/api/pullDiff.test.ts +++ b/web/src/lib/api/pullDiff.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import { createBobbinClient } from "./client"; import { gitTarget } from "./gitclient"; -import { loadPullDiff, loadPullInterdiff } from "./pullDiff"; +import { fetchSides, loadDiff, resolveChange } from "./pullDiff"; import type { MirrorFileDiff } from "./gitmirror"; const src = (path: string, extra: Partial = {}) => ({ @@ -23,12 +23,9 @@ const json = (body: unknown) => headers: { "content-type": "application/json" } }); -const params = { - baseRepo: "did:plc:repo" as never, - baseCommit: "base", - headRepo: "did:plc:repo" as never, - headCommit: "head" -}; +const REPO = "did:plc:repo" as never; + +const diffSpec = { kind: "diff", repo: REPO, version: { base: "base", head: "head" } } as const; // contents keyed by `:`, so each side can differ const harness = (diffs: MirrorFileDiff[], contents: Record) => { @@ -45,23 +42,36 @@ const harness = (diffs: MirrorFileDiff[], contents: Record) => { return json({ path: url.searchParams.get("path"), content, encoding: "utf-8", size: 1 }); }); - const ctx = createBobbinClient({ serviceUrl: "https://bobbin.test", fetch: fetchMock }); + const deps = { + ctx: createBobbinClient({ serviceUrl: "https://bobbin.test", fetch: fetchMock }) + }; const git = gitTarget( { bobbinUrl: "https://bobbin.test", knotMirrorUrl: "" } as never, { uri: "at://did:plc:alice/sh.tangled.repo/core", repoDid: "did:plc:repo" }, fetchMock ); - return { ctx, git, calls }; + return { deps: { ...deps, git }, calls }; }; -describe("loadPullDiff", () => { - it("pairs each file's two sides and sums the stat", async () => { - const { ctx, git, calls } = harness( +const interdiffSpec = { + kind: "interdiff", + repo: REPO, + oldVersion: { base: "v1base", head: "v1head" }, + version: { base: "v2base", head: "v2head" } +} as const; + +const blobCalls = (calls: string[]) => calls.filter((path) => path.endsWith("repo.blob")); + +describe("loadDiff", () => { + it("describes every file without reading a single blob", async () => { + const { deps, calls } = harness( [ { lhsSrc: src("a.ts"), rhsSrc: src("a.ts"), - hunks: [{ novelLhs: [0], novelRhs: [0, 1], lines: [{ lhs: 0, rhs: 0 }, { rhs: 1 }] }], + hunks: [ + { novelLhs: [0], novelRhs: [0, 1], lines: [{ lhs: 0, rhs: 0 }, { rhs: 1 }] } + ], hasSyntacticChanges: false }, { @@ -75,48 +85,60 @@ describe("loadPullDiff", () => { rhsSrc: absent("gone.ts"), hunks: [{ novelLhs: [0], novelRhs: [], lines: [{ lhs: 0 }] }], hasSyntacticChanges: false + }, + { + lhsSrc: src("old.ts"), + rhsSrc: src("moved.ts"), + hunks: [], + hasSyntacticChanges: false } ], - { - "base:a.ts": "one\n", - "head:a.ts": "two\nthree\n", - "head:new.ts": "hello\n", - "base:gone.ts": "bye\n" - } + {} ); - const diff = await loadPullDiff(ctx, git, params, "unified"); + const diff = await loadDiff(deps, diffSpec); - expect(diff.stat).toEqual({ insertions: 3, deletions: 2, files_changed: 3 }); + expect(diff.stat).toEqual({ insertions: 3, deletions: 2, files_changed: 4 }); expect(diff.truncated).toBe(0); - expect(diff.files[0]).toMatchObject({ - name: "a.ts", - kind: "changed", - oldFile: { name: "a.ts", contents: "one\n" }, - newFile: { name: "a.ts", contents: "two\nthree\n" } - }); - // a file with no left side gets an empty left side, not a failed fetch - expect(diff.files[1]).toMatchObject({ - name: "new.ts", - kind: "new", - oldFile: { contents: "" }, - newFile: { contents: "hello\n" } - }); - expect(diff.files[2]).toMatchObject({ - name: "gone.ts", - kind: "deleted", - oldFile: { contents: "bye\n" }, - newFile: { contents: "" } - }); - // two for the changed file, one each for the sides that exist: the absent side - // is never fetched, it would 404 - expect(calls.filter((path) => path.endsWith("repo.blob"))).toHaveLength(4); - // the server prerenders each pair so the first paint ships highlighted - expect(diff.prerendered?.["a.ts→a.ts"]).toContain("three"); + expect(diff.contents).toEqual({ oldRef: "base", newRef: "head" }); + expect(diff.files.map((file) => file.kind)).toEqual([ + "changed", + "new", + "deleted", + "renamed" + ]); + expect(diff.files[3].oldName).toBe("old.ts"); + // every renderable file carries the two sides a later fetch needs + expect(diff.files.every((file) => file.sides !== undefined)).toBe(true); + // the whole point: metadata costs one request, no blob reads at all + expect(blobCalls(calls)).toHaveLength(0); + expect(calls).toHaveLength(1); + }); + + it("reserves height from the hunk line count", async () => { + const { deps } = harness( + [ + { + lhsSrc: src("a.ts"), + rhsSrc: src("a.ts"), + hunks: [ + { novelLhs: [], novelRhs: [], lines: [{ lhs: 0, rhs: 0 }, { rhs: 1 }] }, + { novelLhs: [], novelRhs: [], lines: [{ lhs: 5, rhs: 6 }] } + ], + hasSyntacticChanges: false + } + ], + {} + ); + + const diff = await loadDiff(deps, diffSpec); + + // three line pairs plus two lines of per-hunk chrome + expect(diff.files[0].lines).toBe(7); }); - it("notes the files it cannot render instead of dropping them", async () => { - const { ctx, git } = harness( + it("notes the files it will never fetch, and leaves them no sides", async () => { + const { deps } = harness( [ { lhsSrc: src("logo.png", { isBinary: true }), @@ -130,9 +152,11 @@ describe("loadPullDiff", () => { hunks: [], hasSyntacticChanges: false }, + // the payload carries each side's size, so an oversize file is noted + // without ever paying a request the blob endpoint would refuse { - lhsSrc: src("gone.ts"), - rhsSrc: src("gone.ts"), + lhsSrc: src("huge.ts"), + rhsSrc: src("huge.ts", { size: (1 << 20) + 1 }), hunks: [], hasSyntacticChanges: false } @@ -140,30 +164,66 @@ describe("loadPullDiff", () => { {} ); - const diff = await loadPullDiff(ctx, git, params, "unified"); + const diff = await loadDiff(deps, diffSpec); expect(diff.files.map((file) => file.note)).toEqual([ "This is a binary file and will not be displayed.", "Submodule pointer, not shown.", - // both blob fetches 404 - "Contents could not be loaded." + "This file is too large to display." ]); - expect(diff.files.every((file) => file.oldFile === undefined)).toBe(true); + expect(diff.files.every((file) => file.sides === undefined)).toBe(true); + }); + + it("marks an interdiff's left side as arriving inline", async () => { + const { deps, calls } = harness([], {}); + + const diff = await loadDiff(deps, interdiffSpec); + + expect(diff.contents).toEqual({ inlineOld: true, newRef: "v2head" }); + expect(blobCalls(calls)).toHaveLength(0); }); }); -describe("loadPullInterdiff", () => { - const interdiffParams = { - baseRepo: "did:plc:repo" as never, - baseCommit1: "v1base", - baseCommit2: "v1head", - headRepo: "did:plc:repo" as never, - headCommit1: "v2base", - headCommit2: "v2head" - }; +describe("fetchSides", () => { + it("reads both sides of a plain diff, and only the ones that exist", async () => { + const { deps, calls } = harness( + [ + { + lhsSrc: src("a.ts"), + rhsSrc: src("a.ts"), + hunks: [{ novelLhs: [0], novelRhs: [0], lines: [{ lhs: 0, rhs: 0 }] }], + hasSyntacticChanges: false + }, + { + lhsSrc: absent("new.ts"), + rhsSrc: src("new.ts"), + hunks: [{ novelLhs: [], novelRhs: [0], lines: [{ rhs: 0 }] }], + hasSyntacticChanges: false + } + ], + { "base:a.ts": "one\n", "head:a.ts": "two\nthree\n", "head:new.ts": "hello\n" } + ); + + const diff = await loadDiff(deps, diffSpec); + const changed = await fetchSides(deps, diff.contents, diff.files[0]); + const added = await fetchSides(deps, diff.contents, diff.files[1]); + + expect(changed).toEqual({ + oldFile: { name: "a.ts", contents: "one\n" }, + newFile: { name: "a.ts", contents: "two\nthree\n" } + }); + // a file with no left side gets an empty left side, not a failed fetch + expect(added).toEqual({ + oldFile: { name: "new.ts", contents: "" }, + newFile: { name: "new.ts", contents: "hello\n" } + }); + // two for the changed file, one for the added file's right side: the absent + // side is never fetched, it would 404 + expect(blobCalls(calls)).toHaveLength(3); + }); - it("takes the left side inline and fetches only the right", async () => { - const { ctx, git, calls } = harness( + it("takes an interdiff's left side inline and fetches only the right", async () => { + const { deps, calls } = harness( [ { lhsSrc: src("a.ts", { content: "one\n" }), @@ -178,31 +238,106 @@ describe("loadPullInterdiff", () => { rhsSrc: src("b.ts"), hunks: [], hasSyntacticChanges: false - }, - // an added file has no left side to inline in the first place - { - lhsSrc: absent("c.ts"), - rhsSrc: src("c.ts"), - hunks: [{ novelLhs: [], novelRhs: [0], lines: [{ rhs: 0 }] }], - hasSyntacticChanges: false } ], - { "v2head:a.ts": "two\n", "v2head:b.ts": "two\n", "v2head:c.ts": "new\n" } + { "v2head:a.ts": "two\n", "v2head:b.ts": "two\n" } ); - const diff = await loadPullInterdiff(ctx, git, interdiffParams, "unified"); + const diff = await loadDiff(deps, interdiffSpec); - expect(diff.files[0]).toMatchObject({ + expect(await fetchSides(deps, diff.contents, diff.files[0])).toEqual({ oldFile: { name: "a.ts", contents: "one\n" }, newFile: { name: "a.ts", contents: "two\n" } }); - expect(diff.files[1].note).toBe("Contents could not be loaded."); - expect(diff.files[2]).toMatchObject({ - kind: "new", - oldFile: { contents: "" }, - newFile: { name: "c.ts", contents: "new\n" } + expect(await fetchSides(deps, diff.contents, diff.files[1])).toEqual({ + note: "Contents could not be loaded." }); // one blob per file, the right side only - expect(calls.filter((path) => path.endsWith("repo.blob"))).toHaveLength(3); + expect(blobCalls(calls)).toHaveLength(2); + }); + + it("notes a side the blob endpoint cannot serve", async () => { + const { deps } = harness( + [ + { + lhsSrc: src("a.ts"), + rhsSrc: src("a.ts"), + hunks: [], + hasSyntacticChanges: false + } + ], + {} + ); + + const diff = await loadDiff(deps, diffSpec); + + expect(await fetchSides(deps, diff.contents, diff.files[0])).toEqual({ + note: "Contents could not be loaded." + }); + }); + + it("hands back a noted file's note rather than fetching it", async () => { + const { deps, calls } = harness( + [ + { + lhsSrc: src("logo.png", { isBinary: true }), + rhsSrc: src("logo.png", { isBinary: true }), + hunks: [], + hasSyntacticChanges: false + } + ], + {} + ); + + const diff = await loadDiff(deps, diffSpec); + + expect(await fetchSides(deps, diff.contents, diff.files[0])).toEqual({ + note: "This is a binary file and will not be displayed." + }); + expect(blobCalls(calls)).toHaveLength(0); + }); +}); + +describe("resolveChange", () => { + const bases = { + oldVersion: { base: "v1base", head: "v1head" }, + version: { base: "v2base", head: "v2head" } + }; + const older = [{ hash: "aaa", parent: "aaaparent", changeId: "kmzwvxqo" }] as never[]; + const newer = [ + { hash: "bbb", parent: "bbbparent", changeId: "kmzwvxqo" }, + { hash: "ccc", parent: "cccparent", changeId: "nvtsyrpq" } + ] as never[]; + + it("interdiffs the two commits carrying the change", () => { + expect(resolveChange("kmzwvxqo", older, newer, bases)).toEqual({ + kind: "interdiff", + oldVersion: { base: "aaaparent", head: "aaa" }, + version: { base: "bbbparent", head: "bbb" } + }); + }); + + it("plain-diffs a change that only exists in the newer version", () => { + expect(resolveChange("nvtsyrpq", older, newer, bases)).toEqual({ + kind: "diff", + version: { base: "cccparent", head: "ccc" } + }); + }); + + it("reports a change the newer version does not have", () => { + expect(resolveChange("zzzzzzzz", older, newer, bases)).toEqual({ kind: "missing" }); + // a repo without jj change ids can never match + expect(resolveChange("kmzwvxqo", older, [{ hash: "bbb" }] as never[], bases)).toEqual({ + kind: "missing" + }); + }); + + it("falls back to the version base for a root commit", () => { + const rootless = [{ hash: "aaa", changeId: "kmzwvxqo" }] as never[]; + expect(resolveChange("kmzwvxqo", rootless, newer, bases)).toEqual({ + kind: "interdiff", + oldVersion: { base: "v1base", head: "aaa" }, + version: { base: "bbbparent", head: "bbb" } + }); }); }); diff --git a/web/src/lib/api/pullDiff.ts b/web/src/lib/api/pullDiff.ts index cb38b00ad..391f4d613 100644 --- a/web/src/lib/api/pullDiff.ts +++ b/web/src/lib/api/pullDiff.ts @@ -1,4 +1,3 @@ -import { browser } from "$app/environment"; import { MAX_INLINE_SIZE } from "./blob"; import { blob } from "./gitclient"; import { @@ -7,166 +6,270 @@ import { diffSideExists, getDiff, getInterdiff, + listLog, type MirrorFileDiff, type MirrorFileKind } from "./gitmirror"; -import { pierreDiffOptions, type DiffStyle } from "$lib/components/repo/pierre"; import type { FileContents } from "@pierre/diffs"; +import type * as ShTangledGitDefs from "./lexicons/types/sh/tangled/git/defs"; +import type { Did } from "@atcute/lexicons/syntax"; import type { BobbinContext } from "./client"; import type { GitTarget } from "./gitclient"; import type { DiffStat } from "./diff"; -import type * as GetDiff from "./lexicons/types/sh/tangled/git/temp2/getDiff"; -import type * as GetInterdiff from "./lexicons/types/sh/tangled/git/temp2/getInterdiff"; +import type { Revspec } from "./pullRoute"; +import type { CommitSummary } from "./repo"; -const FILE_LIMIT = 25; +type DiffSrc = ShTangledGitDefs.DiffSrc; + +// a sanity ceiling, not a render budget: contents are fetched per card on demand, +// so this only stops a pathological diff from queueing unbounded work +const FILE_LIMIT = 300; + +// pierre renders roughly one row per line pair, plus a little chrome per hunk. only +// used to reserve height under a pending card, so an estimate is enough +const HUNK_CHROME_LINES = 2; + +// what the page asks for: one version's diff, or the interdiff between two of +// them. `repo` is the source repo -- gitmirror unions both dids' object dirs +// into one scratch repo, so a fork's head and its base share one did here +export interface DiffSpec { + kind: "diff"; + repo: Did; + version: Revspec; +} + +export interface InterdiffSpec { + kind: "interdiff"; + repo: Did; + oldVersion: Revspec; + version: Revspec; + // narrows the interdiff to the one commit carrying this change id + changeId?: string; +} + +export type PullDiffSpec = DiffSpec | InterdiffSpec; + +export interface PullDiffDeps { + ctx: BobbinContext; + // where the blob text comes from; the diff payload carries line numbers only + git: GitTarget; +} export interface PullDiffFile { - // stable across sides, used for anchors and the prerender map + // stable across sides, used for anchors key: string; name: string; oldName?: string; kind: MirrorFileKind; stat: { insertions: number; deletions: number }; - // set when there is nothing to render: binary, submodule, or over the cap + // rough rendered height, so a card can reserve space before its text lands + lines: number; + // set when there is nothing to render: binary, submodule, too large, or past + // the ceiling. a file with a note has no `sides` to fetch note?: string; - oldFile?: FileContents; - newFile?: FileContents; + sides?: { lhs: DiffSrc; rhs: DiffSrc }; } +// where the two sides' text comes from. an interdiff's lhs is a tree gitmirror +// rebased in memory for that one request, reachable by no ref, so its text can +// only arrive inlined on the payload +export type ContentsSource = + { inlineOld?: false; oldRef: string; newRef: string } | { inlineOld: true; newRef: string }; + export interface PullDiff { files: PullDiffFile[]; stat: DiffStat; - // how many files were listed but not fetched + // how many files were listed but left unrendered by the ceiling truncated: number; - // server-prerendered shadow dom per file key, only for the initial ssr - prerendered?: Record; + contents: ContentsSource; } -// the diff endpoint answers metadata only — `diffSrc` is path/oid/size/flags and -// a hunk is aligned line numbers — so the text comes from the blob endpoint and +// one file's two sides, ready for pierre to diff +export interface FilePair { + oldFile: FileContents; + newFile: FileContents; +} + +export type FileContentsResult = FilePair | { note: string }; + +export const loadDiff = (deps: PullDiffDeps, spec: PullDiffSpec): Promise => { + if (spec.kind === "diff") return plainDiff(deps, spec.repo, spec.version); + if (!spec.changeId) return interdiff(deps, spec.repo, spec.oldVersion, spec.version); + return oneChange(deps, spec); +}; + +const plainDiff = async (deps: PullDiffDeps, repo: Did, version: Revspec): Promise => { + const { diffs } = await getDiff(deps.ctx, { + baseRepo: repo, + baseCommit: version.base, + headRepo: repo, + headCommit: version.head + }); + // the left side is read at `base`, while gitmirror diffs from + // merge_base(base, head). identical when base is the merge base, which is + // what a pull version stores + return buildDiff(diffs, { oldRef: version.base, newRef: version.head }); +}; + +// an interdiff's right side is the new version's head tree, so it reads like any +// other diff -- its left side comes inlined, see `ContentsSource` +const interdiff = async ( + deps: PullDiffDeps, + repo: Did, + oldVersion: Revspec, + version: Revspec +): Promise => { + const { diffs } = await getInterdiff(deps.ctx, { + baseRepo: repo, + baseCommit1: oldVersion.base, + baseCommit2: oldVersion.head, + headRepo: repo, + headCommit1: version.base, + headCommit2: version.head + }); + return buildDiff(diffs, { inlineOld: true, newRef: version.head }); +}; + +// a change id names the same logical commit in both versions, so narrowing to it +// means interdiffing that one commit's span on each side +const oneChange = async (deps: PullDiffDeps, spec: InterdiffSpec): Promise => { + const [oldCommits, newCommits] = await Promise.all([ + listLog(deps.ctx, spec.repo, spec.oldVersion), + listLog(deps.ctx, spec.repo, spec.version) + ]); + + const target = resolveChange(spec.changeId ?? "", oldCommits, newCommits, spec); + switch (target.kind) { + case "missing": + throw new Error("Can't find a commit with that change-id in this version."); + case "diff": + return plainDiff(deps, spec.repo, target.version); + case "interdiff": + return interdiff(deps, spec.repo, target.oldVersion, target.version); + } +}; + +export type ChangeTarget = + // the change is missing in the newer version, so there is nothing to interdiff + | { kind: "missing" } + // the change is new in the newer version, so we just diff since its parent + | { kind: "diff"; version: Revspec } + | { kind: "interdiff"; oldVersion: Revspec; version: Revspec }; + +// a root commit has no parent to diff against, so it falls back to the base of +// the version it belongs to +const single = (commit: CommitSummary, fallbackBase: string): Revspec => ({ + base: commit.parent ?? fallbackBase, + head: commit.hash +}); + +export const resolveChange = ( + changeId: string, + oldCommits: readonly CommitSummary[], + newCommits: readonly CommitSummary[], + bases: { oldVersion: Revspec; version: Revspec } +): ChangeTarget => { + const find = (commits: readonly CommitSummary[]) => + commits.find((commit) => commit.changeId === changeId); + + const to = find(newCommits); + if (!to) return { kind: "missing" }; + + const from = find(oldCommits); + if (!from) return { kind: "diff", version: single(to, bases.version.base) }; + + return { + kind: "interdiff", + oldVersion: single(from, bases.oldVersion.base), + version: single(to, bases.version.base) + }; +}; + +// the diff endpoint answers metadata only -- `diffSrc` is path/oid/size/flags and +// a hunk is aligned line numbers -- so the text comes from the blob endpoint and // pierre diffs the two sides itself +const buildDiff = (diffs: MirrorFileDiff[], contents: ContentsSource): PullDiff => { + const stat: DiffStat = { insertions: 0, deletions: 0, files_changed: diffs.length }; + const files = diffs.map((file, index): PullDiffFile => { + const fileStat = diffFileStat(file); + stat.insertions += fileStat.insertions; + stat.deletions += fileStat.deletions; + + const kind = diffFileKind(file); + const name = file.rhsSrc.path || file.lhsSrc.path; + const base: PullDiffFile = { + key: `${file.lhsSrc.path}→${file.rhsSrc.path}`, + name, + oldName: kind === "renamed" ? file.lhsSrc.path : undefined, + kind, + stat: fileStat, + lines: renderedLines(file) + }; + + if (file.lhsSrc.isSubmodule || file.rhsSrc.isSubmodule) { + return { ...base, note: "Submodule pointer, not shown." }; + } + if (file.lhsSrc.isBinary || file.rhsSrc.isBinary) { + return { ...base, note: "This is a binary file and will not be displayed." }; + } + // the payload carries each side's size, so a file the blob endpoint would + // refuse never costs a request + if (tooLarge(file.lhsSrc) || tooLarge(file.rhsSrc)) { + return { ...base, note: "This file is too large to display." }; + } + if (index >= FILE_LIMIT) { + return { ...base, note: "Not rendered: too many files in this diff." }; + } + + return { ...base, sides: { lhs: file.lhsSrc, rhs: file.rhsSrc } }; + }); + + return { files, stat, truncated: Math.max(diffs.length - FILE_LIMIT, 0), contents }; +}; + +const tooLarge = (src: DiffSrc): boolean => src.size > MAX_INLINE_SIZE; + +const renderedLines = (file: MirrorFileDiff): number => + file.hunks.reduce((total, hunk) => total + hunk.lines.length + HUNK_CHROME_LINES, 0); + +// one side's text: inlined when gitmirror already sent it, otherwise a blob read. +// `null` is a side that could not be read, which is not the same as an empty side const sideContents = async ( git: GitTarget, - ref: string, - src: MirrorFileDiff["lhsSrc"] + ref: string | undefined, + src: DiffSrc ): Promise => { // a side that doesn't exist at this ref is an empty side, not a failed fetch if (!diffSideExists(src)) return ""; + if (src.content !== undefined) return src.content; + // the rebased tree an interdiff diffs against is reachable by no ref, so a + // missing inline side there can never be recovered + if (ref === undefined) return null; const output = await blob(git, { ref, path: src.path }, MAX_INLINE_SIZE).catch(() => null); if (!output || output.encoding !== "utf-8") return null; return output.content ?? ""; }; -// both sides of one file, `null` for a side whose text could not be read -type Sides = (file: MirrorFileDiff) => Promise<[string | null, string | null]>; +// phase two: one file's text, fetched when its card asks for it +export const fetchSides = async ( + deps: PullDiffDeps, + contents: ContentsSource, + file: PullDiffFile +): Promise => { + const sides = file.sides; + if (!sides) return { note: file.note ?? "Contents could not be loaded." }; -const buildDiff = async ( - diffs: MirrorFileDiff[], - style: DiffStyle, - sides: Sides -): Promise => { - const stat: DiffStat = { insertions: 0, deletions: 0, files_changed: diffs.length }; - const files = await Promise.all( - diffs.map(async (file, index): Promise => { - const fileStat = diffFileStat(file); - stat.insertions += fileStat.insertions; - stat.deletions += fileStat.deletions; - - const kind = diffFileKind(file); - const name = file.rhsSrc.path || file.lhsSrc.path; - const base: PullDiffFile = { - key: `${file.lhsSrc.path}→${file.rhsSrc.path}`, - name, - oldName: kind === "renamed" ? file.lhsSrc.path : undefined, - kind, - stat: fileStat - }; - - if (file.lhsSrc.isSubmodule || file.rhsSrc.isSubmodule) { - return { ...base, note: "Submodule pointer, not shown." }; - } - if (file.lhsSrc.isBinary || file.rhsSrc.isBinary) { - return { ...base, note: "This is a binary file and will not be displayed." }; - } - if (index >= FILE_LIMIT) { - return { ...base, note: "Not rendered: too many files in this diff." }; - } - - const [oldContents, newContents] = await sides(file); - if (oldContents === null || newContents === null) { - return { ...base, note: "Contents could not be loaded." }; - } - - return { - ...base, - // pierre infers the language from the name, same as the blob page - oldFile: { name: file.lhsSrc.path || name, contents: oldContents }, - newFile: { name, contents: newContents } - }; - }) - ); + const [oldContents, newContents] = await Promise.all([ + sideContents(deps.git, contents.inlineOld ? undefined : contents.oldRef, sides.lhs), + sideContents(deps.git, contents.newRef, sides.rhs) + ]); + if (oldContents === null || newContents === null) { + return { note: "Contents could not be loaded." }; + } return { - files, - stat, - truncated: Math.max(diffs.length - FILE_LIMIT, 0), - prerendered: await prerender(files, style) + // pierre infers the language from the name, same as the blob page + oldFile: { name: sides.lhs.path || file.name, contents: oldContents }, + newFile: { name: file.name, contents: newContents } }; }; - -export const loadPullDiff = async ( - ctx: BobbinContext, - git: GitTarget, - params: GetDiff.$params, - style: DiffStyle -): Promise => { - const { diffs } = await getDiff(ctx, params); - return buildDiff(diffs, style, (file) => - Promise.all([ - // the left side is read at baseCommit, while gitmirror diffs from - // merge_base(base, head). identical when base is the merge base, which - // is what a pull version stores - sideContents(git, params.baseCommit, file.lhsSrc), - sideContents(git, params.headCommit, file.rhsSrc) - ]) - ); -}; - -// an interdiff's right side is version2's head tree, so it reads like any other -// diff — but its left side is a tree gitmirror rebased in memory for this one -// request, reachable by no ref, so that text can only arrive inlined -export const loadPullInterdiff = async ( - ctx: BobbinContext, - git: GitTarget, - params: GetInterdiff.$params, - style: DiffStyle -): Promise => { - const { diffs } = await getInterdiff(ctx, params); - return buildDiff(diffs, style, async (file) => [ - diffSideExists(file.lhsSrc) ? (file.lhsSrc.content ?? null) : "", - await sideContents(git, params.headCommit2, file.rhsSrc) - ]); -}; - -// the first paint ships highlighted, like the commit page does -const prerender = async ( - files: PullDiffFile[], - style: DiffStyle -): Promise | undefined> => { - if (browser) return undefined; - const { preloadMultiFileDiff } = await import("@pierre/diffs/ssr"); - const options = pierreDiffOptions(style); - const entries = await Promise.all( - files - .filter((file) => file.oldFile && file.newFile) - .map(async (file) => { - const { prerenderedHTML } = await preloadMultiFileDiff({ - oldFile: file.oldFile as FileContents, - newFile: file.newFile as FileContents, - options - }); - return [file.key, prerenderedHTML] as const; - }) - ); - return Object.fromEntries(entries); -}; diff --git a/web/src/lib/api/pullPage.ts b/web/src/lib/api/pullPage.ts deleted file mode 100644 index d011280b3..000000000 --- a/web/src/lib/api/pullPage.ts +++ /dev/null @@ -1,136 +0,0 @@ -import { listCommits, toMirrorCommitSummary } from "./gitmirror"; -import { loadPullDiff, loadPullInterdiff, type PullDiff } from "./pullDiff"; -import { - resolveChange, - type PullDiffView, - type PullInterdiffView, - type Revspec -} from "./pullRoute"; -import type { Did } from "@atcute/lexicons/syntax"; -import type { BobbinContext } from "./client"; -import type { GitTarget } from "./gitclient"; -import type { CommitSummary } from "./repo"; -import type { DiffStyle } from "$lib/components/repo/pierre"; - -const COMMIT_LIMIT = 100; - -export interface PullPageDeps { - ctx: BobbinContext; - git: GitTarget; - // todo: fork-based pulls read their head side from the source repo - repo: Did; - diffStyle: DiffStyle; -} - -// what either mode hands the page -export interface PullPageDiff { - commits: CommitSummary[]; - commitsError?: string; - diff?: PullDiff; - diffError?: string; -} - -const listLog = async (deps: PullPageDeps, range: Revspec): Promise => { - const page = await listCommits(deps.ctx, { - repo: deps.repo, - ranges: [`${range.base}..${range.head}`], - limit: COMMIT_LIMIT - }); - return page.commits.map(toMirrorCommitSummary); -}; - -const message = (cause: unknown) => (cause instanceof Error ? cause.message : String(cause)); - -const diffOf = (deps: PullPageDeps, spec: Revspec) => - loadPullDiff( - deps.ctx, - deps.git, - { - baseRepo: deps.repo, - baseCommit: spec.base, - headRepo: deps.repo, - headCommit: spec.head - }, - deps.diffStyle - ); - -// both dids feed one scratch repo, so the pair is the same as a diff's -const interdiffOf = (deps: PullPageDeps, a: Revspec, b: Revspec) => - loadPullInterdiff( - deps.ctx, - deps.git, - { - baseRepo: deps.repo, - baseCommit1: a.base, - baseCommit2: a.head, - headRepo: deps.repo, - headCommit1: b.base, - headCommit2: b.head - }, - deps.diffStyle - ); - -// `version` is the whole round being viewed: the commit list always covers it, -// whatever range the diff itself was narrowed to -export const prepareDiff = async ( - deps: PullPageDeps, - view: PullDiffView, - version: Revspec -): Promise => { - const result: PullPageDiff = { commits: [] }; - - // the two failures are independent — a dead log still leaves a readable diff - await Promise.all([ - listLog(deps, version).then( - (commits) => (result.commits = commits), - (cause) => (result.commitsError = message(cause)) - ), - diffOf(deps, view).then( - (diff) => (result.diff = diff), - (cause) => (result.diffError = message(cause)) - ) - ]); - - return result; -}; - -export const prepareInterdiff = async ( - deps: PullPageDeps, - view: PullInterdiffView -): Promise => { - const result: PullPageDiff = { commits: [] }; - - let priorCommits: CommitSummary[] = []; - try { - [result.commits, priorCommits] = await Promise.all([ - listLog(deps, view.to), - view.changeId ? listLog(deps, view.from) : [] - ]); - } catch (cause) { - result.commitsError = message(cause); - } - - try { - if (!view.changeId) { - result.diff = await interdiffOf(deps, view.from, view.to); - } else if (result.commitsError) { - result.diffError = result.commitsError; - } else { - const target = resolveChange(view.changeId, priorCommits, result.commits, { - from: view.from.base, - to: view.to.base - }); - if (target.kind === "missing") { - result.diffError = "Can't find commit with given change-id."; - } else if (target.kind === "diff") { - result.diff = await diffOf(deps, target.spec); - } else { - result.diff = await interdiffOf(deps, target.from, target.to); - } - } - } catch (cause) { - result.diffError = message(cause); - } - - return result; -}; diff --git a/web/src/lib/api/pullRoute.test.ts b/web/src/lib/api/pullRoute.test.ts index bf6e9a077..e10850ffe 100644 --- a/web/src/lib/api/pullRoute.test.ts +++ b/web/src/lib/api/pullRoute.test.ts @@ -1,13 +1,9 @@ import { describe, expect, it } from "vitest"; import { - activeCommitId, - activeVersion, isRedirect, parseDiffRoute, parseInterdiffRoute, parseRange, - resolveChange, - seeAll, type PullView } from "./pullRoute"; @@ -44,36 +40,17 @@ describe("parseRange", () => { describe("parseDiffRoute", () => { it("resolves latest and numbers to the version's own ends", () => { - expect(view("latest")).toEqual({ - mode: "diff", - version: 1, - base: "b1", - head: "h1", - isDiffBase: true, - isDiffHead: true - }); + expect(view("latest")).toEqual({ mode: "diff", version: 1, base: "b1", head: "h1" }); expect(view("0")).toMatchObject({ version: 0, base: "b0", head: "h0" }); }); it("takes an explicit range, and the base/head magic words", () => { - expect(view("1", "c1..c2")).toMatchObject({ - base: "c1", - head: "c2", - isDiffBase: false, - isDiffHead: false - }); + expect(view("1", "c1..c2")).toMatchObject({ base: "c1", head: "c2" }); // a bare rev is a head, and the words fall back to the version's ends - expect(view("1", "c2")).toMatchObject({ base: "b1", head: "c2", isDiffBase: true }); + expect(view("1", "c2")).toMatchObject({ base: "b1", head: "c2" }); expect(view("1", "base..head")).toMatchObject({ base: "b1", head: "h1" }); }); - it("marks see-all only when both sides defaulted", () => { - expect(seeAll(view("1"))).toBe(true); - expect(seeAll(view("1", "base..head"))).toBe(true); - expect(seeAll(view("1", "c1..c2"))).toBe(false); - expect(seeAll(view("1", "..c2"))).toBe(false); - }); - it("redirects a bad version to latest and a bad range to the version", () => { expect(parseDiffRoute(versions, "9")).toEqual({ redirect: "latest" }); expect(parseDiffRoute(versions, "foo")).toEqual({ redirect: "latest" }); @@ -86,8 +63,8 @@ describe("parseInterdiffRoute", () => { it("parses both versions, resolves their ends, and takes the change filter", () => { expect(view("0..1")).toEqual({ mode: "interdiff", - version1: 0, - version2: 1, + oldVersion: 0, + version: 1, from: { base: "b0", head: "h0" }, to: { base: "b1", head: "h1" }, changeId: "" @@ -101,51 +78,4 @@ describe("parseInterdiffRoute", () => { expect(parseInterdiffRoute(versions, "foo..1")).toEqual({ redirect: "latest" }); expect(parseInterdiffRoute(versions, "..1")).toEqual({ redirect: "latest" }); }); - - it("reports the newer version as active, and see-all without a filter", () => { - expect(activeVersion(view("0..1"))).toBe(1); - expect(activeCommitId(view("0..1", "kmzwvxqo"))).toBe("kmzwvxqo"); - expect(seeAll(view("0..1"))).toBe(true); - expect(seeAll(view("0..1", "kmzwvxqo"))).toBe(false); - }); -}); - -describe("resolveChange", () => { - const bases = { from: "v1base", to: "v2base" }; - const older = [{ hash: "aaa", parent: "aaaparent", changeId: "kmzwvxqo" }]; - const newer = [ - { hash: "bbb", parent: "bbbparent", changeId: "kmzwvxqo" }, - { hash: "ccc", parent: "cccparent", changeId: "nvtsyrpq" } - ]; - - it("interdiffs the two commits carrying the change", () => { - expect(resolveChange("kmzwvxqo", older, newer, bases)).toEqual({ - kind: "interdiff", - from: { base: "aaaparent", head: "aaa" }, - to: { base: "bbbparent", head: "bbb" } - }); - }); - - it("plain-diffs a change that only exists in the newer version", () => { - expect(resolveChange("nvtsyrpq", older, newer, bases)).toEqual({ - kind: "diff", - spec: { base: "cccparent", head: "ccc" } - }); - }); - - it("reports a change the newer version does not have", () => { - expect(resolveChange("zzzzzzzz", older, newer, bases)).toEqual({ kind: "missing" }); - // a repo without jj change ids can never match - expect(resolveChange("kmzwvxqo", older, [{ hash: "bbb" }], bases)).toEqual({ kind: "missing" }); - }); - - it("falls back to the version base for a root commit", () => { - expect( - resolveChange("kmzwvxqo", [{ hash: "aaa", changeId: "kmzwvxqo" }], newer, bases) - ).toEqual({ - kind: "interdiff", - from: { base: "v1base", head: "aaa" }, - to: { base: "bbbparent", head: "bbb" } - }); - }); }); diff --git a/web/src/lib/api/pullRoute.ts b/web/src/lib/api/pullRoute.ts index 4d306c11e..3d5770414 100644 --- a/web/src/lib/api/pullRoute.ts +++ b/web/src/lib/api/pullRoute.ts @@ -30,19 +30,14 @@ export interface PullDiffView { version: number; base: string; head: string; - // true when that side was defaulted, which is what makes the commit card's - // "See all changes" row the active one - isDiffBase: boolean; - isDiffHead: boolean; } export interface PullInterdiffView { mode: "interdiff"; - version1: number; - version2: number; - // the two versions' own ends, resolved here so nothing downstream re-indexes - from: Revspec; - to: Revspec; + oldVersion: number; + version: number; + from: Revspec; // oldVersion {base, head} + to: Revspec; // version {base, head} // "" means all changes changeId: string; } @@ -72,17 +67,14 @@ export const parseInterdiffRoute = ( if (!range) return { redirect: "latest" }; const version1 = index(range.base, versions.length); const version2 = index(range.head, versions.length); - // the appview indexes its versions raw here and panics on a bad number if (version1 === null || version2 === null) return { redirect: "latest" }; const changeId = rangeParam === "all" ? "" : (rangeParam ?? ""); return { mode: "interdiff", - version1, - version2, - // copied field by field: a version carries its comments too, and `view` is - // serialized to the page - from: { base: versions[version1].base, head: versions[version1].head }, - to: { base: versions[version2].base, head: versions[version2].head }, + oldVersion: version1, + version: version2, + from: versions[version1], + to: versions[version2], changeId }; }; @@ -100,65 +92,12 @@ export const parseDiffRoute = ( const spec = parseRange(rangeParam ?? ""); if (!spec) return { redirect: "version" }; - // `base` and `head` are the appview's magic words for the version's own ends const isDiffBase = spec.base === "" || spec.base === "base"; const isDiffHead = spec.head === "" || spec.head === "head"; return { mode: "diff", version, base: isDiffBase ? versions[version].base : spec.base, - head: isDiffHead ? versions[version].head : spec.head, - isDiffBase, - isDiffHead + head: isDiffHead ? versions[version].head : spec.head }; }; - -// the version whose commits are listed, and whose card the rail highlights -export const activeVersion = (view: PullView): number => - view.mode === "diff" ? view.version : view.version2; - -// the commit row to mark, compared on short hashes like the appview does -export const activeCommitId = (view: PullView): string => - view.mode === "diff" ? view.head : view.changeId; - -export const seeAll = (view: PullView): boolean => - view.mode === "diff" ? view.isDiffBase && view.isDiffHead : view.changeId === ""; - -// only the fields a change-id lookup reads, so this module keeps no deps -interface ChangeCommit { - hash: string; - parent?: string; - changeId?: string; -} - -export type ChangeTarget = - | { kind: "missing" } - // the change is new in the second version, so there is nothing to interdiff - | { kind: "diff"; spec: Revspec } - | { kind: "interdiff"; from: Revspec; to: Revspec }; - -// `/../` narrows an interdiff to the one commit carrying that -// jj change id on each side, exactly as `appview/pulls/single.go` does -export const resolveChange = ( - changeId: string, - fromCommits: readonly ChangeCommit[], - toCommits: readonly ChangeCommit[], - bases: { from: string; to: string } -): ChangeTarget => { - const find = (commits: readonly ChangeCommit[]) => - commits.find((commit) => commit.changeId === changeId); - const to = find(toCommits); - // the row was picked from the second version's log, so this is a typed url - if (!to) return { kind: "missing" }; - - // the appview takes the first parent, which is the zero hash on a root commit - // and would be rejected; the version's own base stands in for that - const spec = (commit: ChangeCommit, base: string): Revspec => ({ - base: commit.parent ?? base, - head: commit.hash - }); - const from = find(fromCommits); - return from - ? { kind: "interdiff", from: spec(from, bases.from), to: spec(to, bases.to) } - : { kind: "diff", spec: spec(to, bases.to) }; -}; diff --git a/web/src/lib/api/repo.ts b/web/src/lib/api/repo.ts index 6856f7276..57f34a8c7 100644 --- a/web/src/lib/api/repo.ts +++ b/web/src/lib/api/repo.ts @@ -137,6 +137,13 @@ export const resolveForkRepoLabels = async ( return labels; }; +// org.tangled.review.patch record +export interface PullSubmission { + head: string; + base: string; + createdAt: string; +} + export interface CommitSummary { hash: string; shortHash: string; @@ -153,6 +160,8 @@ export interface CommitSummary { changeId?: string; // first parent, only filled in where a caller needs to link a commit range parent?: string; + // pinned comments referencing this commit + commentCount?: number; } export const didFromSignature = (email: string): string | undefined => diff --git a/web/src/lib/components/comment/comments.test.ts b/web/src/lib/components/comment/comments.test.ts new file mode 100644 index 000000000..5b7301339 --- /dev/null +++ b/web/src/lib/components/comment/comments.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, it } from "vitest"; +import { + countCommentsByCommit, + filterCommentsByCommit, + groupCommentsByVersion, + type CommentView +} from "./comments"; +import type { CommitSummary } from "$lib/api/repo"; + +const comment = (rkey: string, createdAt: string): CommentView => ({ + uri: `at://did:plc:alice/sh.tangled.feed.comment/${rkey}`, + rkey, + authorDid: "did:plc:alice", + authorHandle: "alice.test", + createdAt, + body: rkey, + bodyHtml: null +}); + +const versions = [{ createdAt: "2026-01-01T00:00:00Z" }, { createdAt: "2026-01-03T00:00:00Z" }]; + +const layout = (comments: CommentView[]) => + groupCommentsByVersion(comments, versions).map((group) => [ + group.version, + group.comments.map((c) => c.rkey) + ]); + +describe("groupCommentsByVersion", () => { + it("files a comment under the newest version at or before it", () => { + // on the boundary belongs to that version, not the one before + expect( + layout([ + comment("boundary", "2026-01-01T00:00:00Z"), + comment("during-v0", "2026-01-02T00:00:00Z"), + comment("after-v1", "2026-01-04T00:00:00Z") + ]) + ).toEqual([ + [0, ["boundary", "during-v0"]], + [1, ["after-v1"]] + ]); + }); + + it("keeps a version with no comments, so the timeline stays complete", () => { + expect(layout([comment("only", "2026-01-02T00:00:00Z")])).toEqual([ + [0, ["only"]], + [1, []] + ]); + }); + + it("puts anything older than v0 in a headerless leading group", () => { + expect(layout([comment("early", "2025-12-31T00:00:00Z")])).toEqual([ + [undefined, ["early"]], + [0, []], + [1, []] + ]); + }); + + it("orders comments itself rather than trusting the caller", () => { + expect( + layout([ + comment("late", "2026-01-04T00:00:00Z"), + comment("early", "2026-01-02T00:00:00Z") + ]) + ).toEqual([ + [0, ["early"]], + [1, ["late"]] + ]); + }); + + it("has nothing to group without versions", () => { + expect(groupCommentsByVersion([comment("a", "2026-01-02T00:00:00Z")], [])).toEqual([ + { version: undefined, comments: [expect.objectContaining({ rkey: "a" })] } + ]); + }); +}); + +const pinned = (rkey: string, oid: string): CommentView => ({ + ...comment(rkey, "2026-01-02T00:00:00Z"), + embed: { + $type: "sh.tangled.embed.commit", + repo: "did:plc:alice", + commit: { $type: "sh.tangled.git.oid", oid } + } +}); + +describe("countCommentsByCommit", () => { + it("sums the comments pinned to one commit and skips the unpinned ones", () => { + const counts = countCommentsByCommit([ + pinned("a", "cafe1234"), + pinned("b", "cafe1234"), + pinned("c", "beef5678"), + comment("loose", "2026-01-02T00:00:00Z") + ]); + expect([...counts]).toEqual([ + ["cafe1234", 2], + ["beef5678", 1] + ]); + }); + + it("leaves an unpinned commit out rather than reporting 0", () => { + const counts = countCommentsByCommit([pinned("a", "cafe1234")]); + expect(counts.has("beef5678")).toBe(false); + }); +}); + +const commit = (hash: string): CommitSummary => ({ + hash, + shortHash: hash.slice(0, 8), + subject: hash, + body: "", + authorName: "alice", + authorEmail: "alice@test", + when: "2026-01-02T00:00:00Z" +}); + +describe("filterCommentsByCommit", () => { + const commits = [commit("cafe1234"), commit("beef5678")]; + const rkeys = (comments: CommentView[]) => comments.map((c) => c.rkey); + + it("keeps an unpinned comment and drops a pin the filter rejects", () => { + expect( + rkeys( + filterCommentsByCommit( + [ + comment("loose", "2026-01-02T00:00:00Z"), + pinned("in", "cafe1234"), + pinned("out", "beef5678") + ], + commits, + (c) => c.hash === "cafe1234" + ) + ) + ).toEqual(["loose", "in"]); + }); + + it("drops a pin that names no known commit", () => { + // an older version's commit, or one a force-push dropped: never in range + expect( + rkeys(filterCommentsByCommit([pinned("gone", "d00d9999")], commits, () => true)) + ).toEqual([]); + }); + + it("filters nothing without a filter", () => { + const comments = [pinned("gone", "d00d9999"), comment("loose", "2026-01-02T00:00:00Z")]; + expect(filterCommentsByCommit(comments, commits)).toBe(comments); + }); +}); diff --git a/web/src/lib/components/comment/comments.ts b/web/src/lib/components/comment/comments.ts index 1eebfa0f2..525f3069d 100644 --- a/web/src/lib/components/comment/comments.ts +++ b/web/src/lib/components/comment/comments.ts @@ -1,3 +1,5 @@ +import type { CommentRecord } from "$lib/api/comment"; +import type { CommitSummary } from "$lib/api/repo"; import type { ReactionGroup } from "$lib/components/reaction/reactions"; export interface CommentView { @@ -11,6 +13,8 @@ export interface CommentView { editedAt?: string; body: string; bodyHtml: string | null; + // carried so an edit rewrites the record without dropping what it pinned + embed?: CommentRecord["embed"]; reactions?: ReactionGroup[]; deleted?: boolean; } @@ -70,3 +74,64 @@ export function buildCommentThreads(inputs: ThreadInput[]): CommentThread[] { for (const thread of list) thread.replies.sort(byCreatedAt); return list; } + +export interface CommentGroup { + /** version this group follows; undefined for the comments that predate v0 */ + version?: number; + comments: CommentView[]; +} + +/** + * A comment belongs to the newest version created at or before it. There is no + * "commented on version N" field on the record, and there will not be one -- + * createdAt on both sides is the only ordering there is. + * + * `versions` must be in chronological order, as `pull.versions` is. + */ +export function groupCommentsByVersion( + comments: CommentView[], + versions: { createdAt: string }[] +): CommentGroup[] { + // slot 0 holds anything older than v0 and renders without a header + const groups: CommentGroup[] = [ + { comments: [] }, + ...versions.map((_, version) => ({ version, comments: [] })) + ]; + // parsed rather than string-compared: a boundary must not turn on whether the + // two sides happen to share an offset or the same fractional precision + const at = (createdAt: string) => Date.parse(createdAt); + // both sides ascend, so one walk places every comment -- slot never goes back + let slot = 0; + for (const comment of [...comments].sort((a, b) => at(a.createdAt) - at(b.createdAt))) { + while (slot < versions.length && at(versions[slot].createdAt) <= at(comment.createdAt)) + slot++; + groups[slot].comments.push(comment); + } + // v0 lands with the pull itself, so slot 0 is empty in practice -- drop it then + return groups.filter((group) => group.version !== undefined || group.comments.length); +} + +export function countCommentsByCommit(comments: CommentView[]): Map { + const counts = new Map(); + for (const { embed } of comments) { + if (!embed) continue; + const oid = embed.commit.oid; + counts.set(oid, (counts.get(oid) ?? 0) + 1); + } + return counts; +} + +export function filterCommentsByCommit( + comments: CommentView[], + commits: CommitSummary[], + filter?: (commit: CommitSummary) => boolean +): CommentView[] { + if (!filter) return comments; + return comments.filter((comment) => { + // full oids on both sides, so equality is enough -- no prefix matching + const oid = comment.embed?.commit.oid; + if (!oid) return true; + const commit = commits.find((c) => c.hash === oid); + return !!commit && filter(commit); + }); +} diff --git a/web/src/lib/components/repo/pulls/PullActions.stories.svelte b/web/src/lib/components/repo/pulls/PullActions.stories.svelte deleted file mode 100644 index 07b8cc00c..000000000 --- a/web/src/lib/components/repo/pulls/PullActions.stories.svelte +++ /dev/null @@ -1,19 +0,0 @@ - - - - diff --git a/web/src/lib/components/repo/pulls/PullActions.svelte b/web/src/lib/components/repo/pulls/PullActions.svelte index 21f46876f..b3e22e570 100644 --- a/web/src/lib/components/repo/pulls/PullActions.svelte +++ b/web/src/lib/components/repo/pulls/PullActions.svelte @@ -1,293 +1,91 @@ -{#if auth.currentUser} -
- {#if latest && actions?.upToDate === false} -
+ {#if pullState === "open"} + {#if canChangeState} +
+ Close + {/if} - - {#if composing} - { - composing = false; - onsubmitted?.(roundIdx, submitted); - }} - oncancel={() => (composing = false)} + {#if canMerge} + - {:else} -
- - - {#if actions?.branchDelete} - - {/if} - - {#if showRunCi} - - {/if} - - {#if actions?.canPush && isOpen && latest} - - {/if} - - {#if actions?.isAuthor && isOpen && latest} - - {/if} - - {#if canModify && isOpen && latest} - - {/if} - - {#if canModify && isClosed && latest} - - {/if} -
- - {#if transition.error} -
- {/if} {/if} - - - - {#if showRunCi} - -
-
e.preventDefault()} class="flex flex-col gap-2"> - Run CI - -
-
- - {#if workflowsChanged} -
    - {#each actions?.changedWorkflows ?? [] as file (file)} -
  • -
  • - {/each} -
-

- Workflow files changed in this round. Are you sure you want to run CI? -

- {:else} -

- This fork-based pull request needs approval to run CI. Are you sure? -

- {/if} - -
- - -
-
-
+ {:else} + {#if pullState === "closed" && canChangeState} + {/if} -
-{/if} + + + {/if} + + diff --git a/web/src/lib/components/repo/pulls/PullComment.stories.svelte b/web/src/lib/components/repo/pulls/PullComment.stories.svelte deleted file mode 100644 index bf4ece003..000000000 --- a/web/src/lib/components/repo/pulls/PullComment.stories.svelte +++ /dev/null @@ -1,49 +0,0 @@ - - - -{#snippet comment(args: Args)} -
- - - -
-{/snippet} - - - {#snippet template(args)}{@render comment(args)}{/snippet} - - - - - {#snippet template(args)}{@render comment(args)}{/snippet} - - - - {#snippet template(args)}{@render comment(args)}{/snippet} - - - - {#snippet template(args)}{@render comment(args)}{/snippet} - diff --git a/web/src/lib/components/repo/pulls/PullComment.svelte b/web/src/lib/components/repo/pulls/PullComment.svelte deleted file mode 100644 index fd696c8ab..000000000 --- a/web/src/lib/components/repo/pulls/PullComment.svelte +++ /dev/null @@ -1,196 +0,0 @@ - - -
-
- - - -
-
-
- - {comment.authorHandle} - - - - - {#if comment.editedAt && !comment.deleted} - Edited - {:else} - - {/if} - - {#if !comment.deleted} -
- {#if currentUser} - - {/if} - {#if isAuthor} - - - {/if} -
- {/if} -
- -
- {#if comment.deleted} -
- [deleted by author] -
- {:else if editing} - { - editing = false; - onedited?.(edited); - }} - oncancel={() => (editing = false)} - /> - {:else} - {#if comment.bodyHtml} - -
{@html comment.bodyHtml}
- {:else} -
- {comment.body} -
- {/if} - (reactions = next)} - /> - {/if} - {#if remove.error} - - {/if} -
-
-
diff --git a/web/src/lib/components/repo/pulls/PullCommitList.stories.svelte b/web/src/lib/components/repo/pulls/PullCommitList.stories.svelte deleted file mode 100644 index 87bfbc011..000000000 --- a/web/src/lib/components/repo/pulls/PullCommitList.stories.svelte +++ /dev/null @@ -1,45 +0,0 @@ - - - - - diff --git a/web/src/lib/components/repo/pulls/PullCommitList.svelte b/web/src/lib/components/repo/pulls/PullCommitList.svelte deleted file mode 100644 index c8c17da9d..000000000 --- a/web/src/lib/components/repo/pulls/PullCommitList.svelte +++ /dev/null @@ -1,83 +0,0 @@ - - -
-
-
- {#if seeAll} -
-
- - {#each commits as commit (commit.hash)} - {@const active = isActive(commit)} -
-
- {#if active} -
- {#if commit.body && expanded[commit.hash]} -
{commit.body}
- {/if} -
- {/each} -
diff --git a/web/src/lib/components/repo/pulls/PullCommitListDiff.svelte b/web/src/lib/components/repo/pulls/PullCommitListDiff.svelte new file mode 100644 index 000000000..85c18592a --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullCommitListDiff.svelte @@ -0,0 +1,50 @@ + + +{#snippet row(i: number, item: CommitSummary | "base", actions: RowActions)} + {#if item === "base"} +
+ Base +
+ {:else} + + {/if} +{/snippet} + + commitRange, (next) => oncommitCommits(next)} +/> diff --git a/web/src/lib/components/repo/pulls/PullCommitListHeader.svelte b/web/src/lib/components/repo/pulls/PullCommitListHeader.svelte new file mode 100644 index 000000000..9001d4fe8 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullCommitListHeader.svelte @@ -0,0 +1,40 @@ + + +
+
+ + Commits + {commits.length} + {#if isInterdiff} + Interdiff + {/if} +
+
+ {#if actions} + {@render actions()} + + {/if} + +
+
diff --git a/web/src/lib/components/repo/pulls/PullCommitListInterdiff.svelte b/web/src/lib/components/repo/pulls/PullCommitListInterdiff.svelte new file mode 100644 index 000000000..ff716c955 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullCommitListInterdiff.svelte @@ -0,0 +1,111 @@ + + +{#snippet selectAllRow()} + {@const isSelected = selected < 0} +
+ +
+ onselect("")}>All changes +
+
+{/snippet} + +
+ {@render selectAllRow()} +
+ {#each commits as commit, i (i)} + {@const isSelected = selected === i} + {@const focusable = commit.changeId !== undefined} + {@const focus = focusable ? () => onselect(commit.changeId ?? "") : undefined} +
+ + +
+ {/each} +
diff --git a/web/src/lib/components/repo/pulls/PullCommitRow.svelte b/web/src/lib/components/repo/pulls/PullCommitRow.svelte new file mode 100644 index 000000000..e36af1c89 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullCommitRow.svelte @@ -0,0 +1,90 @@ + + +
+
+
+
+ + {commit.shortHash} + + {#if commit.changeId} + {short(commit.changeId)} + {/if} +
+
+ {commit.subject} +
+ {#if commit.body} + + {/if} +
+ + {#if commit.body && expanded} +
{commit.body}
+ {/if} +
+
+ {#if pipelineStatuses} + {#await pipelineStatuses then statuses} + {#if statuses[commit.hash]} + + {/if} + {/await} + + {/if} + {#if commit.commentCount !== undefined} +
+ {commit.commentCount} + +
+ {/if} +
+
diff --git a/web/src/lib/components/repo/pulls/PullDiff.svelte b/web/src/lib/components/repo/pulls/PullDiff.svelte new file mode 100644 index 000000000..aef98f8a1 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullDiff.svelte @@ -0,0 +1,124 @@ + + +
+ + {#if treeOpen} + + {/if} + +
+
+
+ {#if !treeOpen} +
+
+ + + +
+
+ + {#if error} +
+ {error} +
+ {:else if diff && diff.files.length > 0} + + {#key diff} + + {/key} + {:else if diff} +
+ No change between two revisions. +
+ {:else} +
+
+ {/if} +
+
diff --git a/web/src/lib/components/repo/pulls/PullDiffFileCard.svelte b/web/src/lib/components/repo/pulls/PullDiffFileCard.svelte new file mode 100644 index 000000000..42d57ffd6 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullDiffFileCard.svelte @@ -0,0 +1,119 @@ + + +
+
+ + +
+
+
+ +
+
+
+ +
+ {#if note} +
+ {note} +
+ {:else if pair} + + {:else} + +
+
+ {/if} +
+
+
diff --git a/web/src/lib/components/repo/pulls/PullDiffFileList.svelte b/web/src/lib/components/repo/pulls/PullDiffFileList.svelte new file mode 100644 index 000000000..6324a58a6 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullDiffFileList.svelte @@ -0,0 +1,104 @@ + + +
+ {#each diff.files as file (file.key)} + + {/each} +
diff --git a/web/src/lib/components/repo/pulls/PullDiffFiles.svelte b/web/src/lib/components/repo/pulls/PullDiffFiles.svelte deleted file mode 100644 index 731431589..000000000 --- a/web/src/lib/components/repo/pulls/PullDiffFiles.svelte +++ /dev/null @@ -1,119 +0,0 @@ - - -
- {#each files as file (file.key)} -
-
- -
-
-
- -
-
-
- -
- {#if file.note} -
- {file.note} -
- {:else} - - {#if prerendered[file.key]} - - - {/if} - - {/if} -
-
-
- {/each} -
diff --git a/web/src/lib/components/repo/pulls/PullDiffPanel.stories.svelte b/web/src/lib/components/repo/pulls/PullDiffPanel.stories.svelte deleted file mode 100644 index bd8246bfc..000000000 --- a/web/src/lib/components/repo/pulls/PullDiffPanel.stories.svelte +++ /dev/null @@ -1,50 +0,0 @@ - - - - - - - diff --git a/web/src/lib/components/repo/pulls/PullDiffPanel.svelte b/web/src/lib/components/repo/pulls/PullDiffPanel.svelte deleted file mode 100644 index 5fd8576e2..000000000 --- a/web/src/lib/components/repo/pulls/PullDiffPanel.svelte +++ /dev/null @@ -1,272 +0,0 @@ - - - -
-
-
- - - {#if diff} - - {:else} -
- - + - - - -
- {/if} - - - {isInterdiff ? "Interdiff" : "Diff"} - - {#if isInterdiff} - v{version1} - - - {#if baseLabel !== undefined && headLabel !== undefined} - {baseLabel} - - -
- -
- - -
- - - - - - - - - {#if canInterdiff} - - {:else} - - {/if} - - - {#if !discussionOpen} -
-
- -
- - - {#if filesOpen} - - {/if} - -
- {#if diffError} -
- {diffError} -
- {:else if diff && diff.files.length > 0} - - {:else if diff} -
- No change between two revisions. -
- {:else} - - - {/if} -
-
-
diff --git a/web/src/lib/components/repo/pulls/PullDiscussion.stories.svelte b/web/src/lib/components/repo/pulls/PullDiscussion.stories.svelte deleted file mode 100644 index f52a8c099..000000000 --- a/web/src/lib/components/repo/pulls/PullDiscussion.stories.svelte +++ /dev/null @@ -1,85 +0,0 @@ - - - -{#snippet rail(args: Args)} -
- - - -
-{/snippet} - - - {#snippet template(args)}{@render rail(args)}{/snippet} - - - - {#snippet template(args)}{@render rail(args)}{/snippet} - - - - {#snippet template(args)}{@render rail(args)}{/snippet} - - - - - {#snippet template(args)}{@render rail(args)}{/snippet} - - - - - {#snippet template(args)} -
- {/snippet} -
diff --git a/web/src/lib/components/repo/pulls/PullDiscussion.svelte b/web/src/lib/components/repo/pulls/PullDiscussion.svelte index 4a74297d4..94507e078 100644 --- a/web/src/lib/components/repo/pulls/PullDiscussion.svelte +++ b/web/src/lib/components/repo/pulls/PullDiscussion.svelte @@ -1,437 +1,95 @@ -
{ - // the appview's desktop guard: the summary can't collapse the rail - if (desktop && !e.currentTarget.open) open = true; - }} - class="group/history flex flex-col {accent}" -> - -
- -

History

-
-
- {plural(versions.length, "version")} - - {plural(comments, "comment")} -
-
- -
- {#each versions as version, index (index)} - {@const active = index === activeVersion} - {@const versionComments = version.comments ?? []} -
-
-
-
- - - -
-
-
- - - {authorHandle} - - submitted - - #{index} - - - - {#if version.createdAt} - - {/if} - - - -
- - - {#if index === latest} -
- {#if needsWorkflowApproval} - {#if changedWorkflows.length > 0} -
- - -
    - {#each changedWorkflows as file (file)} -
  • -
  • - {/each} -
-
- {:else} -
-
- {/if} - {/if} - - {#if pullState === "open"} - {#if !actions?.mergeCheck} -
-
- {:else if actions.mergeCheck.error} -
-
- {:else if isConflicted} -
- -
-
-
-
    - {#each actions.mergeCheck.conflicts ?? [] as conflict (conflict.filename ?? conflict.reason)} -
  • -
  • - {/each} -
-
- {:else} -
-
- {/if} - {/if} -
+
+ {#if !visible.length} +

No comments yet.

+ {:else} + {#each groups as group (group.version ?? "pre")} + {#if group.version && group.version > 0} + + {/if} +
+ {#if group.version !== undefined} + {@const isActive = group.version === currentVersion} + {@const version = versions[group.version]} +
+
+ {#if isActive} + {/if} + v{group.version} +
-
-
- -
- - - {#if versionComments.length > 0} -
- - - -
+ {#if pipelineStatuses} + {#await pipelineStatuses then statuses} + {#if statuses[version.head]} + + {/if} + {/await} {/if} -
- - -
- {#each versionComments as comment (comment.uri)} - - {/each}
- - {#if versionComments.length > 0} - - {/if} - -
- -
-
+ {/if} + {#each group.comments as comment (comment.uri)} + + {/each}
{/each} -
-
+ {/if} + diff --git a/web/src/lib/components/repo/pulls/PullInfoBar.stories.svelte b/web/src/lib/components/repo/pulls/PullInfoBar.stories.svelte deleted file mode 100644 index d6f158ac8..000000000 --- a/web/src/lib/components/repo/pulls/PullInfoBar.stories.svelte +++ /dev/null @@ -1,36 +0,0 @@ - - - - - - diff --git a/web/src/lib/components/repo/pulls/PullInfoBar.svelte b/web/src/lib/components/repo/pulls/PullInfoBar.svelte deleted file mode 100644 index 481809647..000000000 --- a/web/src/lib/components/repo/pulls/PullInfoBar.svelte +++ /dev/null @@ -1,88 +0,0 @@ - - -
- - - - opened by - - - - {#if createdAt} - - - - {/if} - - - targeting - - {targetBranch} - - - - {#if sourceBranch} - - from - - {#if sourceRepo} - fork: - {/if} - {sourceBranch} - - - {/if} - - {#if actions} -
- {@render actions()} -
- {/if} -
diff --git a/web/src/lib/components/repo/pulls/PullMetaPanel.svelte b/web/src/lib/components/repo/pulls/PullMetaPanel.svelte deleted file mode 100644 index bda019d4c..000000000 --- a/web/src/lib/components/repo/pulls/PullMetaPanel.svelte +++ /dev/null @@ -1,190 +0,0 @@ - - -{#if participants && participants.length > 0} -
-
- Participants - {participants.length} -
-
- {#each shown as person, index (person.did)} - - - - {/each} - {#if overflow > 0} - +{overflow} - {/if} -
-
-{/if} - -{#if canSubscribe} -
- -
-{/if} - -{#if backlinks && backlinks.length > 0} -
-
- - Referenced by - -
- -
-{/if} - -
-
- - AT URI - -
- - - -
-
- - {uri} - -
diff --git a/web/src/lib/components/repo/pulls/PullReviewComment.stories.svelte b/web/src/lib/components/repo/pulls/PullReviewComment.stories.svelte new file mode 100644 index 000000000..81239fac7 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullReviewComment.stories.svelte @@ -0,0 +1,33 @@ + + + + {#snippet template(args)} +
+ +
+ {/snippet} +
+ + + {#snippet template(args)} +
+ +
+ {/snippet} +
diff --git a/web/src/lib/components/repo/pulls/PullReviewComment.svelte b/web/src/lib/components/repo/pulls/PullReviewComment.svelte new file mode 100644 index 000000000..9b11271d7 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullReviewComment.svelte @@ -0,0 +1,110 @@ + + +
+
+ + +
+ + {#if comment.editedAt && !comment.deleted} + Edited + {:else} + + {/if} + + {#if !comment.deleted} + + {/if} +
+
+
+ {#if comment.embed} + {comment.embed.commit.oid.slice(0, 8)} + {/if} + {#if editing} + (editing = false)} + /> + {:else} + {#if comment.bodyHtml} +
+ + {@html comment.bodyHtml} +
+ {:else if comment.body} +
+ {comment.body} +
+ {/if} + {/if} + +
+
diff --git a/web/src/lib/components/repo/pulls/PullReviewCommentEditor.svelte b/web/src/lib/components/repo/pulls/PullReviewCommentEditor.svelte new file mode 100644 index 000000000..b2cb16a0a --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullReviewCommentEditor.svelte @@ -0,0 +1,80 @@ + + +
+ + + +
+ + {#if oncancel} + + {/if} +
+ diff --git a/web/src/lib/components/repo/pulls/PullReviewCommentForm.stories.svelte b/web/src/lib/components/repo/pulls/PullReviewCommentForm.stories.svelte new file mode 100644 index 000000000..4f9b4e5dd --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullReviewCommentForm.stories.svelte @@ -0,0 +1,51 @@ + + + + + + {#snippet template(args)} +
+ { + submitted.push(commit ? `${body} @${commit.slice(0, 8)}` : body); + }} + /> + + {#each submitted as body, i (i)} +

{body}

+ {/each} +
+
+ {/snippet} +
+ + + {#snippet template(args)} +
+ { + throw new Error("nope"); + }} + /> +
+ {/snippet} +
+ + diff --git a/web/src/lib/components/repo/pulls/PullReviewCommentForm.svelte b/web/src/lib/components/repo/pulls/PullReviewCommentForm.svelte new file mode 100644 index 000000000..d709abee1 --- /dev/null +++ b/web/src/lib/components/repo/pulls/PullReviewCommentForm.svelte @@ -0,0 +1,61 @@ + + +
+ + Pin comment to {short(currentCommit)} + + + + +{/snippet} + + + {#snippet trigger()} + + {#if range.end !== range.start} + v{range.start} + diff --git a/web/src/lib/components/repo/tickets/Ticket.stories.svelte b/web/src/lib/components/repo/tickets/Ticket.stories.svelte new file mode 100644 index 000000000..0a8662152 --- /dev/null +++ b/web/src/lib/components/repo/tickets/Ticket.stories.svelte @@ -0,0 +1,81 @@ + + + + + + + {#snippet template(args)} + + + + {/snippet} + + + + {#snippet template(args)} + + { + patch = input; + }} + ondelete={async () => { + throw new Error("no-op"); + }} + > + {#snippet extraActions()} + + {/snippet} + + + {/snippet} + + + + {#snippet template(args)} + + undefined} ondelete={async () => undefined} /> + + {/snippet} + diff --git a/web/src/lib/components/repo/tickets/Ticket.svelte b/web/src/lib/components/repo/tickets/Ticket.svelte new file mode 100644 index 000000000..21152cc44 --- /dev/null +++ b/web/src/lib/components/repo/tickets/Ticket.svelte @@ -0,0 +1,113 @@ + + +{#if editing && onedit} + { + await onedit(input); + editing = false; + }} + oncancel={() => (editing = false)} + /> +{:else} +
+
+
+

+ {ticket.title} + #{ticket.rkey} +

+
+ {#if isAuthor && onedit} + + {/if} + {#if isAuthor && ondelete} + + {/if} + {#if extraActions} +
+ {@render extraActions()} +
+ {/if} +
+
+ +
+
+ + +
+ +
+{/if} diff --git a/web/src/lib/components/repo/tickets/TicketBody.svelte b/web/src/lib/components/repo/tickets/TicketBody.svelte new file mode 100644 index 000000000..1d321bcb2 --- /dev/null +++ b/web/src/lib/components/repo/tickets/TicketBody.svelte @@ -0,0 +1,17 @@ + + +{#if bodyHtml} + +
{@html bodyHtml}
+{:else if body} +
{body}
+{/if} diff --git a/web/src/lib/components/repo/tickets/TicketForm.stories.svelte b/web/src/lib/components/repo/tickets/TicketForm.stories.svelte new file mode 100644 index 000000000..a9a9e26d9 --- /dev/null +++ b/web/src/lib/components/repo/tickets/TicketForm.stories.svelte @@ -0,0 +1,57 @@ + + + + + + + { + const canvas = within(canvasElement); + await userEvent.type(canvas.getByLabelText("Title"), "Something broke"); + await userEvent.type( + canvas.getByPlaceholderText(/describe your ticket/i), + "details{Control>}{Enter}{/Control}" + ); + await waitFor(() => expect(canvas.getByRole("alert")).toHaveTextContent(/no-op/i)); + }} +> + {#snippet template(args)} + + {/snippet} + diff --git a/web/src/lib/components/repo/tickets/TicketForm.svelte b/web/src/lib/components/repo/tickets/TicketForm.svelte new file mode 100644 index 000000000..b9d1a7101 --- /dev/null +++ b/web/src/lib/components/repo/tickets/TicketForm.svelte @@ -0,0 +1,99 @@ + + +
+
+ +
+ + {#if targetBranch} +
todo: target branch selector
+ {/if} + +
+ Body + + +
+ + + +
+ + +
+ diff --git a/web/src/lib/components/repo/tickets/TicketInfoBar.svelte b/web/src/lib/components/repo/tickets/TicketInfoBar.svelte new file mode 100644 index 000000000..11b334a21 --- /dev/null +++ b/web/src/lib/components/repo/tickets/TicketInfoBar.svelte @@ -0,0 +1,44 @@ + + +
+ + + + opened by + + + + + + + + {#if targetBranch} + + targetting {targetBranch} + + {/if} + + {#if extraInfo} + {@render extraInfo()} + {/if} +
diff --git a/web/src/lib/components/repo/tickets/TicketStatePill.stories.svelte b/web/src/lib/components/repo/tickets/TicketStatePill.stories.svelte new file mode 100644 index 000000000..3e3948cf5 --- /dev/null +++ b/web/src/lib/components/repo/tickets/TicketStatePill.stories.svelte @@ -0,0 +1,27 @@ + + + + + + + + diff --git a/web/src/lib/components/repo/tickets/TicketStatePill.svelte b/web/src/lib/components/repo/tickets/TicketStatePill.svelte new file mode 100644 index 000000000..cc7e033ac --- /dev/null +++ b/web/src/lib/components/repo/tickets/TicketStatePill.svelte @@ -0,0 +1,64 @@ + + + + + + diff --git a/web/src/lib/components/ui/GraphCell.stories.svelte b/web/src/lib/components/ui/GraphCell.stories.svelte new file mode 100644 index 000000000..a135c48ee --- /dev/null +++ b/web/src/lib/components/ui/GraphCell.stories.svelte @@ -0,0 +1,125 @@ + + + + {#snippet template(args)} +
+ +
+ {/snippet} +
+ + + + {#snippet template(args)} +
+
+ {#each run as band, i (i)} + + {/each} +
+
+ + +
+
+ {/snippet} +
+ + + + {#snippet template()} +
+ {#each run as band, i (i)} +
+ + +
+ {/each} +
+ {/snippet} +
+ + + + {#snippet template()} +
+ {#each run as band, i (i)} + + {/each} +
+ {/snippet} +
+ + + + {#snippet template()} +
+ + + +
+ {/snippet} +
+ + + + {#snippet template()} +
+ {#each run as band, i (i)} + + {/each} +
+ {/snippet} +
diff --git a/web/src/lib/components/ui/GraphCell.svelte b/web/src/lib/components/ui/GraphCell.svelte new file mode 100644 index 000000000..3068b8942 --- /dev/null +++ b/web/src/lib/components/ui/GraphCell.svelte @@ -0,0 +1,105 @@ + + + + +
+
+ {#if band !== "none"} +
+ {/if} +
+
+ + diff --git a/web/src/lib/components/ui/RangeSelector.stories.svelte b/web/src/lib/components/ui/RangeSelector.stories.svelte new file mode 100644 index 000000000..5a1731744 --- /dev/null +++ b/web/src/lib/components/ui/RangeSelector.stories.svelte @@ -0,0 +1,68 @@ + + + + +{#snippet row(index: number, item: number, actions: RowActions)} +
+
+
+ row {item} (#{index}) +
+
+
+ meta +
+
+{/snippet} + + + {#snippet template()} +
+ + {bound.start}..{bound.end} +
+ {/snippet} +
+ + + {#snippet template()} +
+ +
+ {/snippet} +
+ + + {#snippet template({ muteFullSelection })} +
+ +
+ {/snippet} +
diff --git a/web/src/lib/components/ui/RangeSelector.svelte b/web/src/lib/components/ui/RangeSelector.svelte new file mode 100644 index 000000000..108f236e6 --- /dev/null +++ b/web/src/lib/components/ui/RangeSelector.svelte @@ -0,0 +1,244 @@ + + + + +
+ {#each items as item, i (i)} + + {@render row(i, item, rowActions(i))} + + {/each} + + {#snippet bandBox({ start, end }: Range, styles: string)} +
+ {/snippet} + + {#snippet pill({ start, end }: Range)} + {#if end > start} + {@render bandBox( + { start, end: end - 1 }, + "mt-2.5 -mb-7.5 ml-2.5 w-5 rounded-full bg-(--range-fill) outline-1 outline-border-default" + )} + {:else} + {@render bandBox( + { start: end, end }, + "mt-2.5 ml-2.5 size-5 self-start rounded-full bg-(--range-fill) outline-1 outline-border-default" + )} + {/if} + {/snippet} + + {#snippet handle(id: HandleId, label: string)} +
grab(e, id)} + onkeydown={(e) => handleKeyDown(e, id)} + title="Drag or use arrow keys to adjust the range" + >
+ {/snippet} + + {#if items.length > 0} + {#if interacting} + {@render bandBox(displayRange, "bg-background-subtle rounded")} + {/if} + {#if !muted} + {@render bandBox( + selectedRange, + "bg-background-inset rounded border border-border-default" + )} + {/if} + {@render pill(displayRange)} + {@render handle("a", "Range boundary 1")} + {@render handle("b", "Range boundary 2")} + {/if} +
diff --git a/web/src/lib/components/ui/RangeSelectorRow.svelte b/web/src/lib/components/ui/RangeSelectorRow.svelte new file mode 100644 index 000000000..07fbbc5d6 --- /dev/null +++ b/web/src/lib/components/ui/RangeSelectorRow.svelte @@ -0,0 +1,31 @@ + + +
+
+ + +
+ {@render children()} +
diff --git a/web/src/lib/components/ui/SplitButton.stories.svelte b/web/src/lib/components/ui/SplitButton.stories.svelte new file mode 100644 index 000000000..62ae2e011 --- /dev/null +++ b/web/src/lib/components/ui/SplitButton.stories.svelte @@ -0,0 +1,45 @@ + + + + + + + + + + + + + + diff --git a/web/src/lib/components/ui/SplitButton.svelte b/web/src/lib/components/ui/SplitButton.svelte new file mode 100644 index 000000000..731edef01 --- /dev/null +++ b/web/src/lib/components/ui/SplitButton.svelte @@ -0,0 +1,84 @@ + + + + +{#if current} + + + + + {#snippet trigger()} + + {/snippet} + {#each options as option, index (option.label)} + (selected = index)} + > + {option.label} + + {/each} + + +{/if} diff --git a/web/src/lib/components/ui/TabPanel.svelte b/web/src/lib/components/ui/TabPanel.svelte index c8d3ad0df..9cb8f4150 100644 --- a/web/src/lib/components/ui/TabPanel.svelte +++ b/web/src/lib/components/ui/TabPanel.svelte @@ -6,7 +6,7 @@ variants: { // the common surface padding; turn off to lay out children yourself padded: { - true: "px-6 py-4", + true: "p-4", false: "" } }, diff --git a/web/src/lib/components/ui/Tag.svelte b/web/src/lib/components/ui/Tag.svelte index 9327339bb..de5cb2e56 100644 --- a/web/src/lib/components/ui/Tag.svelte +++ b/web/src/lib/components/ui/Tag.svelte @@ -7,6 +7,7 @@ color: { default: "border border-border-default bg-background-default text-foreground-default", gray: "border border-transparent bg-background-inset text-foreground-default", + highlight: "border border-transparent bg-background-highlight text-foreground-highlight", success: "border border-transparent bg-background-success-subtle text-foreground-success-strong", danger: diff --git a/web/src/routes/[handle]/[repo]/pulls/[aturi]/+layout.ts b/web/src/routes/[handle]/[repo]/pulls/[aturi]/+layout.ts new file mode 100644 index 000000000..449803bf3 --- /dev/null +++ b/web/src/routes/[handle]/[repo]/pulls/[aturi]/+layout.ts @@ -0,0 +1,81 @@ +import { error } from "@sveltejs/kit"; +import { createBobbinClient } from "$lib/api/client"; +import { COMMENT_AUTHOR_DOCS } from "$lib/api/descriptors"; +import { authorOf, enrich, target } from "$lib/api/enrich"; +import { getPullView, type PullState } from "$lib/api/records"; +import { rkeyFromUri } from "$lib/api/uri"; +import { renderMarkup, type MarkupContext } from "$lib/markup"; +import type { CommentListPage } from "$lib/api/comment"; +import type { CommentView } from "$lib/components/comment/comments"; +import type { LayoutLoad } from "./$types"; + +export const load: LayoutLoad = async (event) => { + const parent = await event.parent(); + const uri = event.params.aturi; + if (!uri.startsWith("at://")) error(404, "Pull request not found"); + + const repoDid = parent.repo.repoDid; + if (!repoDid) error(404, "Pull request not found"); + + const ctx = createBobbinClient({ + serviceUrl: parent.publicConfig.bobbinUrl, + fetch: event.fetch + }); + + const markup: MarkupContext = { + repo: `${parent.repo.ownerHandle}/${parent.repo.name}`, + ref: parent.repo.defaultBranch, + host: event.url.host + }; + + const [pullView, page] = await Promise.all([ + getPullView(ctx, uri).catch(() => null), + enrich(ctx, { + xrpc: "sh.tangled.feed.listComments", + params: { subject: uri, order: "asc", limit: 100 }, + enrich: [target(COMMENT_AUTHOR_DOCS, ["items[].uri"])] + }).catch(() => null) + ]); + + // TODO: fetch versions separately from PR(ticket) itself. + // const pullView = getTicketView(aturi) + // const pullVersions = listPullVersions(aturi) + + if (!pullView) error(404, "Pull request not found"); + if (!pullView.versions.length) + error(404, "This pull request has no ref-based versions to show"); + + const body = pullView.body?.text ?? ""; + const bodyHtml = body ? await renderMarkup(body, markup).catch(() => null) : null; + + const comments = await Promise.all( + (page?.output.items ?? []).map(async (item): Promise => { + const author = authorOf(page?.data ?? {}, item.uri, COMMENT_AUTHOR_DOCS); + const body = item.value.body?.text ?? ""; + return { + uri: item.uri, + cid: item.cid, + rkey: rkeyFromUri(item.uri), + authorDid: author.did, + authorHandle: author.handle, + createdAt: item.value.createdAt, + embed: item.value.embed, + body, + bodyHtml: body ? await renderMarkup(body, markup).catch(() => null) : null + }; + }) + ); + + return { + comments, + markup, + sourceRepoDid: pullView.source?.repo?.did ?? repoDid, + pull: { + ...pullView, + rkey: rkeyFromUri(pullView.uri), + state: (pullView.state ?? "open") as PullState, + body, + bodyHtml + } + }; +}; diff --git a/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.svelte b/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.svelte index 6d864e063..a51933765 100644 --- a/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.svelte +++ b/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.svelte @@ -1,32 +1,51 @@ {title} · Pull Request #{pull.rkey} · {data.repo.ownerHandle}/{data.repo + >{pull.title} · Pull Request #{pull.rkey} · {data.repo.ownerHandle}/{data.repo .name} · Tangled -{#snippet pullActions()} - -{/snippet} - - -
-
- -
- - - -
-
- - {#if editing} - (editing = false)} - /> - {:else} - - - - - {/if} - -
- -
- - +
+ + -
+ + {/snippet} + + + {#if isInterdiff} + + {:else} + + {/if}
- - - 0} - {discussionOpen} - onToggleDiscussion={() => (discussionOpen = true)} - /> + + +
- - - - {#if discussionOpen} - +
- (stateOverride = next)} - onsubmitted={insertComment} - onedited={updateComment} - ondeleted={removeComment} - bind:open={railOpen} - onclose={() => (discussionOpen = false)} - /> + + +
+ + + +
{/if}
diff --git a/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.ts b/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.ts index b79d6bea5..98b31e59a 100644 --- a/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.ts +++ b/web/src/routes/[handle]/[repo]/pulls/[aturi]/[version]/[[range]]/+page.ts @@ -1,78 +1,20 @@ -import { error, redirect } from "@sveltejs/kit"; +import { redirect } from "@sveltejs/kit"; import { createBobbinClient } from "$lib/api/client"; -import { COMMENT_AUTHOR_DOCS } from "$lib/api/descriptors"; -import { authorOf, enrich, target } from "$lib/api/enrich"; -import { gitTarget } from "$lib/api/gitclient"; -import { prepareDiff, prepareInterdiff, type PullPageDeps } from "$lib/api/pullPage"; +import { listLog } from "$lib/api/gitmirror"; import { isRedirect, parseDiffRoute, parseInterdiffRoute } from "$lib/api/pullRoute"; -import { - getPullView, - type CommentListPage, - type CommentRecord, - type PullState, - type RecordView -} from "$lib/api/records"; -import { didFromUri, rkeyFromUri } from "$lib/api/uri"; -import { renderMarkup } from "$lib/markup"; +import { countCommentsByCommit } from "$lib/components/comment/comments"; import type { Did } from "@atcute/lexicons/syntax"; -import type { CommentView } from "$lib/components/comment/comments"; -import type { DiffStyle } from "$lib/components/repo/pierre"; -import type { MarkupContext } from "$lib/markup/paths"; import type { PageLoad } from "./$types"; export const load: PageLoad = async (event) => { const parent = await event.parent(); const uri = event.params.aturi; + const { pull, sourceRepoDid } = parent; - if (!uri.startsWith("at://")) error(404, "Pull request not found"); - - const repoDid = parent.repo.repoDid; - if (!repoDid) error(404, "Pull request not found"); - - const ctx = createBobbinClient({ serviceUrl: parent.publicConfig.bobbinUrl, fetch: event.fetch }); - - const viewerDid = parent.auth?.did; - - const [pull, commentPage] = await Promise.all([ - getPullView(ctx, uri).catch(() => null), - enrich(ctx, { - xrpc: "sh.tangled.feed.listComments", - params: { subject: uri, order: "asc", limit: 100 }, - enrich: [target(COMMENT_AUTHOR_DOCS, ["items[].uri"])] - }).catch(() => null) - ]); - if (!pull) error(404, "Pull request not found"); - if (!pull.versions.length) error(404, "This pull request has no ref-based versions to show"); - - const markup: MarkupContext = { - repo: `${parent.repo.ownerHandle}/${parent.repo.name}`, - ref: parent.repo.defaultBranch, - host: event.url.host - }; - - const toCommentView = async (item: RecordView): Promise => { - const author = authorOf(commentPage?.data ?? {}, item.uri, COMMENT_AUTHOR_DOCS); - const body = item.value.body?.text ?? ""; - return { - uri: item.uri, - cid: item.cid, - rkey: rkeyFromUri(item.uri), - authorDid: author.did, - authorHandle: author.handle, - createdAt: item.value.createdAt, - body, - bodyHtml: body ? await renderMarkup(body, markup).catch(() => null) : null - }; - }; - - const versions = pull.versions.map((version) => ({ ...version, comments: [] as CommentView[] })); - for (const item of commentPage?.output.items ?? []) { - const round = Math.min(item.value.pullRoundIdx ?? 0, versions.length - 1); - versions[round].comments.push(await toCommentView(item)); - } - - const body = pull.body?.text ?? ""; - const bodyHtml = body ? await renderMarkup(body, markup).catch(() => null) : null; + const ctx = createBobbinClient({ + serviceUrl: parent.publicConfig.bobbinUrl, + fetch: event.fetch + }); const route = event.params.version.includes("..") ? parseInterdiffRoute(pull.versions, event.params.version, event.params.range) @@ -83,40 +25,30 @@ export const load: PageLoad = async (event) => { redirect(307, `/${event.params.handle}/${event.params.repo}/pulls/${uri}/${target}`); } - const diffStyle: DiffStyle = event.url.searchParams.get("diff") === "split" ? "split" : "unified"; - const deps: PullPageDeps = { - ctx, - git: gitTarget(parent.publicConfig, parent.repo, event.fetch), - repo: repoDid as Did, - diffStyle - }; - - const prepared = - route.mode === "diff" - ? await prepareDiff(deps, route, pull.versions[route.version]) - : await prepareInterdiff(deps, route); - - const sourceRepoDid = pull.source?.repo?.did ?? repoDid; - - const actions = { - isAuthor: Boolean(viewerDid) && viewerDid === didFromUri(pull.uri) - }; + const currentVersion = pull.versions[route.version]; + + // TODO: bobbin should hydrate the commit list with backlinked comment counts + const commits = await listLog(ctx, sourceRepoDid as Did, currentVersion); + const commentCounts = countCommentsByCommit(parent.comments); + + if (route.mode === "diff") { + route.base = + commits.find((commit) => commit.hash.startsWith(route.base))?.hash ?? + currentVersion.base; + route.head = + commits.find((commit) => commit.hash.startsWith(route.head))?.hash ?? + currentVersion.head; + } else if (route.mode === "interdiff" && route.changeId) { + route.changeId = + commits.find((commit) => commit.changeId?.startsWith(route.changeId))?.changeId ?? ""; + } return { - ...prepared, - diffStyle, - actions, - sourceRepoDid, - pull: { - ...pull, - rkey: rkeyFromUri(pull.uri), - state: (pull.state ?? "open") as PullState, - body, - bodyHtml, - versions - }, + commits: commits.map((commit) => ({ + ...commit, + commentCount: commentCounts.get(commit.hash) ?? 0 + })), view: route, - uri, - markup + uri }; }; -- 2.51.2