authorgravatar for Taras.Fomin@gmail.comTaras <Taras.Fomin@gmail.com> 2026-08-15 14:37:02+03:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-08-15 13:37:02+02:00
logd94fbda344398bd266b9f6645e531eb17d3c9e4e
tree2d4908efb3c0ef22e4365cce1245ecb1a34b5466
parentb7dbbbaf6c394621cba57cf58dfcc1845f445ef6
signature Signed by PGP key B5690EEEBB952194

fix(gitlab): submit review comments on context lines (#6348)

13 files changed, 338 insertions(+), 54 deletions(-)

apps/server/src/pullRequest/BitbucketPullRequestApi.test.ts+7-1
...@@ -776,7 +776,13 @@ layer("BitbucketPullRequestApi.layer", (it) => {...@@ -776,7 +776,13 @@ layer("BitbucketPullRequestApi.layer", (it) => {
776 number: 7,776 number: 7,
777 verdict: "request-changes",777 verdict: "request-changes",
778 body: "Two things.",778 body: "Two things.",
779 comments: [{ path: "src/a.ts", line: 12, side: "left", body: "why remove?" }],779 comments: [
780 {
781 path: "src/a.ts",
782 position: { kind: "deleted", oldLine: 12 },
783 body: "why remove?",
784 },
785 ],
780 });786 });
781787
782 expect(callAt(0).url).toContain("/pullrequests/7/comments");788 expect(callAt(0).url).toContain("/pullrequests/7/comments");
apps/server/src/pullRequest/BitbucketPullRequestApi.ts+15-1
...@@ -12,6 +12,7 @@ import type {...@@ -12,6 +12,7 @@ import type {
12 PullRequestMergeMethod,12 PullRequestMergeMethod,
13 PullRequestMergeability,13 PullRequestMergeability,
14 PullRequestReviewCommentDraft,14 PullRequestReviewCommentDraft,
15 PullRequestReviewPosition,
15 PullRequestReviewThread,16 PullRequestReviewThread,
16 PullRequestReviewVerdict,17 PullRequestReviewVerdict,
17 PullRequestReviewerCandidateList,18 PullRequestReviewerCandidateList,
...@@ -364,6 +365,19 @@ function mergeStrategy(method: PullRequestMergeMethod | undefined): string {...@@ -364,6 +365,19 @@ function mergeStrategy(method: PullRequestMergeMethod | undefined): string {
364 }365 }
365}366}
366367
368function bitbucketReviewPosition(
369 position: PullRequestReviewPosition,
370): { readonly from: number } | { readonly to: number } {
371 switch (position.kind) {
372 case "added":
373 return { to: position.newLine };
374 case "deleted":
375 return { from: position.oldLine };
376 case "context":
377 return position.side === "left" ? { from: position.oldLine } : { to: position.newLine };
378 }
379}
380
367export const make = Effect.gen(function* () {381export const make = Effect.gen(function* () {
368 const bitbucket = yield* BitbucketApi.BitbucketApi;382 const bitbucket = yield* BitbucketApi.BitbucketApi;
369383
...@@ -794,7 +808,7 @@ export const make = Effect.gen(function* () {...@@ -794,7 +808,7 @@ export const make = Effect.gen(function* () {
794 content: { raw: comment.body },808 content: { raw: comment.body },
795 inline: {809 inline: {
796 path: comment.path,810 path: comment.path,
797 ...(comment.side === "left" ? { from: comment.line } : { to: comment.line }),811 ...bitbucketReviewPosition(comment.position),
798 },812 },
799 }),813 }),
800 }),814 }),
apps/server/src/pullRequest/GitHubPullRequestCli.test.ts+1-1
...@@ -1624,7 +1624,7 @@ layer("GitHubPullRequestCli.layer", (it) => {...@@ -1624,7 +1624,7 @@ layer("GitHubPullRequestCli.layer", (it) => {
1624 number: 7,1624 number: 7,
1625 verdict: "approve",1625 verdict: "approve",
1626 body: "Looks right.",1626 body: "Looks right.",
1627 comments: [{ path: "src/a.ts", line: 4, side: "right", body: "nit" }],1627 comments: [{ path: "src/a.ts", position: { kind: "added", newLine: 4 }, body: "nit" }],
1628 });1628 });
16291629
1630 expect(callAt(0).args).toEqual([1630 expect(callAt(0).args).toEqual([
apps/server/src/pullRequest/GitLabPullRequestCli.test.ts+7-2
...@@ -1018,7 +1018,12 @@ layer("GitLabPullRequestCli.layer", (it) => {...@@ -1018,7 +1018,12 @@ layer("GitLabPullRequestCli.layer", (it) => {
1018 verdict: "approve",1018 verdict: "approve",
1019 body: "Looks right.",1019 body: "Looks right.",
1020 comments: [1020 comments: [
1021 { path: "src/b.ts", oldPath: "src/a.ts", line: 4, side: "left", body: "why remove?" },1021 {
1022 path: "src/b.ts",
1023 oldPath: "src/a.ts",
1024 position: { kind: "deleted", oldLine: 4 },
1025 body: "why remove?",
1026 },
1022 ],1027 ],
1023 });1028 });
10241029
...@@ -1197,7 +1202,7 @@ layer("GitLabPullRequestCli.layer", (it) => {...@@ -1197,7 +1202,7 @@ layer("GitLabPullRequestCli.layer", (it) => {
1197 number: 7,1202 number: 7,
1198 verdict: "comment",1203 verdict: "comment",
1199 body: "",1204 body: "",
1200 comments: [{ path: "src/a.ts", line: 4, side: "right", body: "nit" }],1205 comments: [{ path: "src/a.ts", position: { kind: "added", newLine: 4 }, body: "nit" }],
1201 }),1206 }),
1202 );1207 );
12031208
apps/server/src/pullRequest/GitLabPullRequestCli.ts+18-3
...@@ -14,6 +14,7 @@ import type {...@@ -14,6 +14,7 @@ import type {
14 PullRequestReaction,14 PullRequestReaction,
15 PullRequestReactionContent,15 PullRequestReactionContent,
16 PullRequestReviewCommentDraft,16 PullRequestReviewCommentDraft,
17 PullRequestReviewPosition,
17 PullRequestReviewThread,18 PullRequestReviewThread,
18 PullRequestReviewVerdict,19 PullRequestReviewVerdict,
19 PullRequestReviewerCandidateList,20 PullRequestReviewerCandidateList,
...@@ -400,6 +401,22 @@ function projectPath(repository: string): string {...@@ -400,6 +401,22 @@ function projectPath(repository: string): string {
400 return encodeURIComponent(repository.trim());401 return encodeURIComponent(repository.trim());
401}402}
402403
404function gitLabReviewPositionLines(
405 position: PullRequestReviewPosition,
406):
407 | { readonly new_line: number }
408 | { readonly old_line: number }
409 | { readonly old_line: number; readonly new_line: number } {
410 switch (position.kind) {
411 case "added":
412 return { new_line: position.newLine };
413 case "deleted":
414 return { old_line: position.oldLine };
415 case "context":
416 return { old_line: position.oldLine, new_line: position.newLine };
417 }
418}
419
403function stateParam(state: PullRequestListState): string {420function stateParam(state: PullRequestListState): string {
404 // GitLab's `closed` already excludes merged merge requests, so no extra filter is needed,421 // GitLab's `closed` already excludes merged merge requests, so no extra filter is needed,
405 // and it spans every state under `all`.422 // and it spans every state under `all`.
...@@ -1324,9 +1341,7 @@ export const make = Effect.gen(function* () {...@@ -1324,9 +1341,7 @@ export const make = Effect.gen(function* () {
1324 // draft carries the name the file had before the change.1341 // draft carries the name the file had before the change.
1325 old_path: comment.oldPath ?? comment.path,1342 old_path: comment.oldPath ?? comment.path,
1326 new_path: comment.path,1343 new_path: comment.path,
1327 ...(comment.side === "left"1344 ...gitLabReviewPositionLines(comment.position),
1328 ? { old_line: comment.line }
1329 : { new_line: comment.line }),
1330 },1345 },
1331 }),1346 }),
1332 }),1347 }),
apps/server/src/pullRequest/PullRequestService.test.ts+1-1
...@@ -1534,7 +1534,7 @@ it.effect("refuses line comments on a host that takes only a summary", () =>...@@ -1534,7 +1534,7 @@ it.effect("refuses line comments on a host that takes only a summary", () =>
1534 number: 1,1534 number: 1,
1535 verdict: "comment",1535 verdict: "comment",
1536 body: "",1536 body: "",
1537 comments: [{ path: "src/a.ts", line: 1, side: "right", body: "nit" }],1537 comments: [{ path: "src/a.ts", position: { kind: "added", newLine: 1 }, body: "nit" }],
1538 }),1538 }),
1539 );1539 );
15401540
apps/server/src/pullRequest/gitHubPullRequestJson.test.ts+10-2
...@@ -1147,8 +1147,16 @@ describe("review submission payload", () => {...@@ -1147,8 +1147,16 @@ describe("review submission payload", () => {
1147 verdict: "request-changes",1147 verdict: "request-changes",
1148 body: "Two things.",1148 body: "Two things.",
1149 comments: [1149 comments: [
1150 { path: "src/a.ts", line: 12, side: "right", body: "rename this" },1150 {
1151 { path: "src/b.ts", line: 3, side: "left", body: "why remove?" },1151 path: "src/a.ts",
1152 position: { kind: "added", newLine: 12 },
1153 body: "rename this",
1154 },
1155 {
1156 path: "src/b.ts",
1157 position: { kind: "deleted", oldLine: 3 },
1158 body: "why remove?",
1159 },
1152 ],1160 ],
1153 }),1161 }),
1154 ) as Record<string, unknown>;1162 ) as Record<string, unknown>;
apps/server/src/pullRequest/gitHubPullRequestJson.ts+18-2
...@@ -17,6 +17,7 @@ import type {...@@ -17,6 +17,7 @@ import type {
17 PullRequestReactionContent,17 PullRequestReactionContent,
18 PullRequestReviewCommentDraft,18 PullRequestReviewCommentDraft,
19 PullRequestReviewDecision,19 PullRequestReviewDecision,
20 PullRequestReviewPosition,
20 PullRequestReviewThread,21 PullRequestReviewThread,
21 PullRequestReviewVerdict,22 PullRequestReviewVerdict,
22 PullRequestReviewerCandidate,23 PullRequestReviewerCandidate,
...@@ -933,6 +934,22 @@ export const REVIEW_DISMISSALS_GRAPHQL_QUERY = `query($owner: String!, $name: St...@@ -933,6 +934,22 @@ export const REVIEW_DISMISSALS_GRAPHQL_QUERY = `query($owner: String!, $name: St
933 }934 }
934}`;935}`;
935936
937function gitHubReviewPosition(position: PullRequestReviewPosition): {
938 readonly line: number;
939 readonly side: "LEFT" | "RIGHT";
940} {
941 switch (position.kind) {
942 case "added":
943 return { line: position.newLine, side: "RIGHT" };
944 case "deleted":
945 return { line: position.oldLine, side: "LEFT" };
946 case "context":
947 return position.side === "left"
948 ? { line: position.oldLine, side: "LEFT" }
949 : { line: position.newLine, side: "RIGHT" };
950 }
951}
952
936/** The whole review as one request body, which is how GitHub keeps it invisible until sent. */953/** The whole review as one request body, which is how GitHub keeps it invisible until sent. */
937export function buildReviewSubmissionJson(input: {954export function buildReviewSubmissionJson(input: {
938 readonly verdict: PullRequestReviewVerdict;955 readonly verdict: PullRequestReviewVerdict;
...@@ -944,8 +961,7 @@ export function buildReviewSubmissionJson(input: {...@@ -944,8 +961,7 @@ export function buildReviewSubmissionJson(input: {
944 body: input.body,961 body: input.body,
945 comments: input.comments.map((comment) => ({962 comments: input.comments.map((comment) => ({
946 path: comment.path,963 path: comment.path,
947 line: comment.line,964 ...gitHubReviewPosition(comment.position),
948 side: comment.side === "left" ? ("LEFT" as const) : ("RIGHT" as const),
949 body: comment.body,965 body: comment.body,
950 })),966 })),
951 });967 });
apps/web/src/components/pullRequest/PullRequestCodeTab.tsx+33-12
...@@ -6,6 +6,7 @@ import type {...@@ -6,6 +6,7 @@ import type {
6 PullRequestDiffSide,6 PullRequestDiffSide,
7 PullRequestOmittedFileStat,7 PullRequestOmittedFileStat,
8 PullRequestRef,8 PullRequestRef,
9 PullRequestReviewPosition,
9 PullRequestReviewThread,10 PullRequestReviewThread,
10} from "@t3tools/contracts";11} from "@t3tools/contracts";
11import {12import {
...@@ -43,7 +44,11 @@ import {...@@ -43,7 +44,11 @@ import {
43} from "~/lib/diffRendering";44} from "~/lib/diffRendering";
44import { cn } from "~/lib/utils";45import { cn } from "~/lib/utils";
45import { createPullRequestDiffFileContentsLoader } from "~/lib/diffFileContents";46import { createPullRequestDiffFileContentsLoader } from "~/lib/diffFileContents";
46import { buildDiffReviewComment, type ReviewCommentContext } from "~/reviewCommentContext";47import {
48 buildDiffReviewComment,
49 resolveDiffReviewPosition,
50 type ReviewCommentContext,
51} from "~/reviewCommentContext";
47import { pullRequestEnvironment } from "~/state/pullRequests";52import { pullRequestEnvironment } from "~/state/pullRequests";
48import { useEnvironmentQuery } from "~/state/query";53import { useEnvironmentQuery } from "~/state/query";
49import { useAtomCommand } from "~/state/use-atom-command";54import { useAtomCommand } from "~/state/use-atom-command";
...@@ -129,8 +134,7 @@ interface DraftAnchor {...@@ -129,8 +134,7 @@ interface DraftAnchor {
129 readonly path: string;134 readonly path: string;
130 /** What the file was called before the change, for the hosts that resolve a position by both. */135 /** What the file was called before the change, for the hosts that resolve a position by both. */
131 readonly oldPath: string | null;136 readonly oldPath: string | null;
132 readonly line: number;137 readonly position: PullRequestReviewPosition;
133 readonly side: PullRequestDiffSide;
134 /** The whole selection, which the comment collapses to one line but a question keeps. */138 /** The whole selection, which the comment collapses to one line but a question keeps. */
135 readonly range: SelectedLineRange;139 readonly range: SelectedLineRange;
136}140}
...@@ -148,8 +152,21 @@ function toViewerSide(side: PullRequestDiffSide) {...@@ -148,8 +152,21 @@ function toViewerSide(side: PullRequestDiffSide) {
148 return side === "left" ? ("deletions" as const) : ("additions" as const);152 return side === "left" ? ("deletions" as const) : ("additions" as const);
149}153}
150154
151function fromViewerSide(side: string | undefined): PullRequestDiffSide {155function getReviewPositionAnchor(position: PullRequestReviewPosition): {
152 return side === "deletions" ? "left" : "right";156 line: number;
157 side: PullRequestDiffSide;
158} {
159 switch (position.kind) {
160 case "added":
161 return { line: position.newLine, side: "right" };
162 case "deleted":
163 return { line: position.oldLine, side: "left" };
164 case "context":
165 return {
166 line: position.side === "left" ? position.oldLine : position.newLine,
167 side: position.side,
168 };
169 }
153}170}
154171
155/**172/**
...@@ -442,10 +459,14 @@ export function PullRequestCodeTab({...@@ -442,10 +459,14 @@ export function PullRequestCodeTab({
442 if (commit === null) {459 if (commit === null) {
443 for (const comment of pendingComments) {460 for (const comment of pendingComments) {
444 if (comment.path !== path) continue;461 if (comment.path !== path) continue;
445 groupAt(comment.side, comment.line).pending.push(comment);462 const anchor = getReviewPositionAnchor(comment.position);
463 groupAt(anchor.side, anchor.line).pending.push(comment);
446 }464 }
447 }465 }
448 if (draft?.fileKey === fileKey) groupAt(draft.side, draft.line).draft = true;466 if (draft?.fileKey === fileKey) {
467 const anchor = getReviewPositionAnchor(draft.position);
468 groupAt(anchor.side, anchor.line).draft = true;
469 }
449470
450 const collapsed = isFileDiffCollapsed(fileKey, foldOverride, toggledFiles);471 const collapsed = isFileDiffCollapsed(fileKey, foldOverride, toggledFiles);
451472
...@@ -594,12 +615,13 @@ export function PullRequestCodeTab({...@@ -594,12 +615,13 @@ export function PullRequestCodeTab({
594 // that silently lost its first line on the other hosts would be worse than one line.615 // that silently lost its first line on the other hosts would be worse than one line.
595 const path = resolveFileDiffPath(file);616 const path = resolveFileDiffPath(file);
596 const previousPath = resolveFileDiffPreviousPath(file);617 const previousPath = resolveFileDiffPreviousPath(file);
618 const position = resolveDiffReviewPosition(file, range.end, range.endSide ?? range.side);
619 if (position === null) return;
597 setDraft({620 setDraft({
598 fileKey: item.id,621 fileKey: item.id,
599 path,622 path,
600 oldPath: previousPath === path ? null : previousPath,623 oldPath: previousPath === path ? null : previousPath,
601 line: range.end,624 position,
602 side: fromViewerSide(range.endSide ?? range.side),
603 range,625 range,
604 });626 });
605 },627 },
...@@ -842,7 +864,7 @@ export function PullRequestCodeTab({...@@ -842,7 +864,7 @@ export function PullRequestCodeTab({
842 {annotation.metadata.draft && draft ? (864 {annotation.metadata.draft && draft ? (
843 <DiffCommentAnnotation865 <DiffCommentAnnotation
844 kind="draft"866 kind="draft"
845 rangeLabel={`${draft.path}:${draft.line}`}867 rangeLabel={`${draft.path}:${getReviewPositionAnchor(draft.position).line}`}
846 text=""868 text=""
847 submitLabel="Add to review"869 submitLabel="Add to review"
848 {...(onAskAboutSelection870 {...(onAskAboutSelection
...@@ -864,8 +886,7 @@ export function PullRequestCodeTab({...@@ -864,8 +886,7 @@ export function PullRequestCodeTab({
864 id: nextPendingReviewCommentId(),886 id: nextPendingReviewCommentId(),
865 path: draft.path,887 path: draft.path,
866 ...(draft.oldPath === null ? {} : { oldPath: draft.oldPath }),888 ...(draft.oldPath === null ? {} : { oldPath: draft.oldPath }),
867 line: draft.line,889 position: draft.position,
868 side: draft.side,
869 body,890 body,
870 });891 });
871 setDraft(null);892 setDraft(null);
apps/web/src/components/pullRequest/pullRequestReviewStore.test.ts+1-1
...@@ -3,7 +3,7 @@ import { beforeEach, describe, expect, it } from "vite-plus/test";...@@ -3,7 +3,7 @@ import { beforeEach, describe, expect, it } from "vite-plus/test";
3import { type PendingReviewComment, usePullRequestReviewStore } from "./pullRequestReviewStore";3import { type PendingReviewComment, usePullRequestReviewStore } from "./pullRequestReviewStore";
44
5function comment(id: string, body = id): PendingReviewComment {5function comment(id: string, body = id): PendingReviewComment {
6 return { id, body, path: "src/app.ts", line: 1, side: "right" };6 return { id, body, path: "src/app.ts", position: { kind: "added", newLine: 1 } };
7}7}
88
9describe("pull request review drafts", () => {9describe("pull request review drafts", () => {
apps/web/src/components/pullRequest/pullRequestReviewStore.ts+2-9
...@@ -6,17 +6,10 @@...@@ -6,17 +6,10 @@
6 * hosts that have no pending review of their own. That also means a draft lives only as long6 * hosts that have no pending review of their own. That also means a draft lives only as long
7 * as the tab does, which is why this is deliberately not persisted.7 * as the tab does, which is why this is deliberately not persisted.
8 */8 */
9import type { ProjectId, PullRequestDiffSide, PullRequestRef } from "@t3tools/contracts";9import type { ProjectId, PullRequestRef, PullRequestReviewCommentDraft } from "@t3tools/contracts";
10import { create } from "zustand";10import { create } from "zustand";
1111
12export interface PendingReviewComment {12export type PendingReviewComment = PullRequestReviewCommentDraft & { readonly id: string };
13 readonly id: string;
14 readonly path: string;
15 /** The line in the file the comment's side names: the new file on the right, the old on the left. */
16 readonly line: number;
17 readonly side: PullRequestDiffSide;
18 readonly body: string;
19}
2013
21/**14/**
22 * A counter rather than anything derived from the comment: two remarks on one line can be the15 * A counter rather than anything derived from the comment: two remarks on one line can be the
apps/web/src/reviewCommentContext.ts+204-17
...@@ -1,6 +1,15 @@...@@ -1,6 +1,15 @@
1import type { FileDiffMetadata, SelectedLineRange, SelectionSide } from "@pierre/diffs";1import type { FileDiffMetadata, SelectedLineRange, SelectionSide } from "@pierre/diffs";
2import type { PullRequestReviewPosition } from "@t3tools/contracts";
2import * as Schema from "effect/Schema";3import * as Schema from "effect/Schema";
34
5const ReviewCommentSelectionSchema = Schema.Struct({
6 start: Schema.Number,
7 side: Schema.Literals(["additions", "deletions"]),
8 end: Schema.Number,
9 endSide: Schema.Literals(["additions", "deletions"]),
10});
11type ReviewCommentSelection = typeof ReviewCommentSelectionSchema.Type;
12
4export const ReviewCommentContextSchema = Schema.Struct({13export const ReviewCommentContextSchema = Schema.Struct({
5 id: Schema.String,14 id: Schema.String,
6 sectionId: Schema.String,15 sectionId: Schema.String,
...@@ -12,6 +21,7 @@ export const ReviewCommentContextSchema = Schema.Struct({...@@ -12,6 +21,7 @@ export const ReviewCommentContextSchema = Schema.Struct({
12 text: Schema.String,21 text: Schema.String,
13 diff: Schema.String,22 diff: Schema.String,
14 fenceLanguage: Schema.optional(Schema.String),23 fenceLanguage: Schema.optional(Schema.String),
24 selection: Schema.optional(ReviewCommentSelectionSchema),
15});25});
1626
17export interface ReviewCommentContext {27export interface ReviewCommentContext {
...@@ -25,6 +35,7 @@ export interface ReviewCommentContext {...@@ -25,6 +35,7 @@ export interface ReviewCommentContext {
25 readonly text: string;35 readonly text: string;
26 readonly diff: string;36 readonly diff: string;
27 readonly fenceLanguage?: string | undefined;37 readonly fenceLanguage?: string | undefined;
38 readonly selection?: ReviewCommentSelection | undefined;
28}39}
2940
30interface DiffReviewLine {41interface DiffReviewLine {
...@@ -267,10 +278,44 @@ function stripTrailingNewline(value: string): string {...@@ -267,10 +278,44 @@ function stripTrailingNewline(value: string): string {
267 return value.endsWith("\n") ? value.slice(0, -1) : value;278 return value.endsWith("\n") ? value.slice(0, -1) : value;
268}279}
269280
270function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffReviewLine> {281function buildDiffReviewLines(
282 fileDiff: FileDiffMetadata,
283 includeExpandedContext: boolean,
284 slice?: { readonly startIndex: number; readonly endIndex: number },
285): ReadonlyArray<DiffReviewLine> {
271 const rows: DiffReviewLine[] = [];286 const rows: DiffReviewLine[] = [];
287 let rowIndex = 0;
288 let oldContextStart = 1;
289 let newContextStart = 1;
290 const pushRow = (row: DiffReviewLine) => {
291 if (!slice || (rowIndex >= slice.startIndex && rowIndex <= slice.endIndex)) {
292 rows.push(row);
293 }
294 rowIndex += 1;
295 };
296 const pushContextGap = (oldStart: number, newStart: number, lineCount: number) => {
297 const count = Math.max(0, lineCount);
298 const firstOffset = slice ? Math.max(0, slice.startIndex - rowIndex) : 0;
299 const lastOffset = slice ? Math.min(count - 1, slice.endIndex - rowIndex) : count - 1;
300 for (let offset = firstOffset; offset <= lastOffset; offset += 1) {
301 rows.push({
302 change: "context",
303 oldLineNumber: oldStart + offset,
304 newLineNumber: newStart + offset,
305 content: stripTrailingNewline(fileDiff.additionLines[newStart + offset - 1] ?? ""),
306 });
307 }
308 rowIndex += count;
309 };
272310
273 for (const hunk of fileDiff.hunks) {311 for (const hunk of fileDiff.hunks) {
312 if (includeExpandedContext) {
313 const oldHunkStart = hunk.deletionStart + (hunk.deletionCount === 0 ? 1 : 0);
314 const newHunkStart = hunk.additionStart + (hunk.additionCount === 0 ? 1 : 0);
315 const contextLines = Math.min(oldHunkStart - oldContextStart, newHunkStart - newContextStart);
316 pushContextGap(oldContextStart, newContextStart, contextLines);
317 }
318
274 let oldLineNumber = hunk.deletionStart;319 let oldLineNumber = hunk.deletionStart;
275 let newLineNumber = hunk.additionStart;320 let newLineNumber = hunk.additionStart;
276 let deletionLineIndex = hunk.deletionLineIndex;321 let deletionLineIndex = hunk.deletionLineIndex;
...@@ -279,7 +324,7 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev...@@ -279,7 +324,7 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev
279 for (const segment of hunk.hunkContent) {324 for (const segment of hunk.hunkContent) {
280 if (segment.type === "context") {325 if (segment.type === "context") {
281 for (let index = 0; index < segment.lines; index += 1) {326 for (let index = 0; index < segment.lines; index += 1) {
282 rows.push({327 pushRow({
283 change: "context",328 change: "context",
284 oldLineNumber,329 oldLineNumber,
285 newLineNumber,330 newLineNumber,
...@@ -298,7 +343,7 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev...@@ -298,7 +343,7 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev
298 }343 }
299344
300 for (let index = 0; index < segment.deletions; index += 1) {345 for (let index = 0; index < segment.deletions; index += 1) {
301 rows.push({346 pushRow({
302 change: "delete",347 change: "delete",
303 oldLineNumber,348 oldLineNumber,
304 newLineNumber: null,349 newLineNumber: null,
...@@ -309,7 +354,7 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev...@@ -309,7 +354,7 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev
309 }354 }
310355
311 for (let index = 0; index < segment.additions; index += 1) {356 for (let index = 0; index < segment.additions; index += 1) {
312 rows.push({357 pushRow({
313 change: "add",358 change: "add",
314 oldLineNumber: null,359 oldLineNumber: null,
315 newLineNumber,360 newLineNumber,
...@@ -319,6 +364,19 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev...@@ -319,6 +364,19 @@ function buildDiffReviewLines(fileDiff: FileDiffMetadata): ReadonlyArray<DiffRev
319 additionLineIndex += 1;364 additionLineIndex += 1;
320 }365 }
321 }366 }
367
368 oldContextStart = hunk.deletionStart + hunk.deletionCount;
369 newContextStart = hunk.additionStart + hunk.additionCount;
370 if (hunk.deletionCount === 0) oldContextStart += 1;
371 if (hunk.additionCount === 0) newContextStart += 1;
372 }
373
374 if (includeExpandedContext) {
375 const trailingLines = Math.min(
376 fileDiff.deletionLines.length - oldContextStart + 1,
377 fileDiff.additionLines.length - newContextStart + 1,
378 );
379 pushContextGap(oldContextStart, newContextStart, trailingLines);
322 }380 }
323381
324 return rows;382 return rows;
...@@ -343,9 +401,20 @@ export function restoreDiffReviewCommentRange(...@@ -343,9 +401,20 @@ export function restoreDiffReviewCommentRange(
343 fileDiff: FileDiffMetadata,401 fileDiff: FileDiffMetadata,
344 comment: ReviewCommentContext,402 comment: ReviewCommentContext,
345): SelectedLineRange | null {403): SelectedLineRange | null {
346 const lines = buildDiffReviewLines(fileDiff);404 if (comment.selection) return comment.selection;
347 const startLine = lines[comment.startIndex];405
348 const endLine = lines[comment.endIndex];406 const includeExpandedContext = !fileDiff.isPartial;
407 const startLine = buildDiffReviewLines(fileDiff, includeExpandedContext, {
408 startIndex: comment.startIndex,
409 endIndex: comment.startIndex,
410 })[0];
411 const endLine =
412 comment.endIndex === comment.startIndex
413 ? startLine
414 : buildDiffReviewLines(fileDiff, includeExpandedContext, {
415 startIndex: comment.endIndex,
416 endIndex: comment.endIndex,
417 })[0];
349 if (!startLine || !endLine) return null;418 if (!startLine || !endLine) return null;
350 const start = getDiffReviewSelectionPoint(startLine);419 const start = getDiffReviewSelectionPoint(startLine);
351 const end = getDiffReviewSelectionPoint(endLine);420 const end = getDiffReviewSelectionPoint(endLine);
...@@ -359,15 +428,118 @@ export function restoreDiffReviewCommentRange(...@@ -359,15 +428,118 @@ export function restoreDiffReviewCommentRange(
359}428}
360429
361function findDiffReviewLineIndex(430function findDiffReviewLineIndex(
362 lines: ReadonlyArray<DiffReviewLine>,431 fileDiff: FileDiffMetadata,
363 lineNumber: number,432 lineNumber: number,
364 side: SelectionSide | undefined,433 side: SelectionSide | undefined,
434 includeExpandedContext = !fileDiff.isPartial,
365): number {435): number {
366 const preferredKey = side === "deletions" ? "oldLineNumber" : "newLineNumber";436 const findOnSide = (selectedSide: "left" | "right") => {
367 const preferredIndex = lines.findIndex((line) => line[preferredKey] === lineNumber);437 let rowIndex = 0;
368 if (preferredIndex >= 0) return preferredIndex;438 let oldContextStart = 1;
369 const fallbackKey = preferredKey === "oldLineNumber" ? "newLineNumber" : "oldLineNumber";439 let newContextStart = 1;
370 return lines.findIndex((line) => line[fallbackKey] === lineNumber);440 const findContextIndex = (oldStart: number, newStart: number, lineCount: number) => {
441 const count = Math.max(0, lineCount);
442 const selectedStart = selectedSide === "left" ? oldStart : newStart;
443 const offset = lineNumber - selectedStart;
444 return offset >= 0 && offset < count ? rowIndex + offset : -1;
445 };
446
447 for (const hunk of fileDiff.hunks) {
448 if (includeExpandedContext) {
449 const oldContextEnd = hunk.deletionStart + (hunk.deletionCount === 0 ? 1 : 0);
450 const newContextEnd = hunk.additionStart + (hunk.additionCount === 0 ? 1 : 0);
451 const contextLines = Math.min(
452 oldContextEnd - oldContextStart,
453 newContextEnd - newContextStart,
454 );
455 const contextIndex = findContextIndex(oldContextStart, newContextStart, contextLines);
456 if (contextIndex >= 0) return contextIndex;
457 rowIndex += Math.max(0, contextLines);
458 }
459
460 let oldLineNumber = hunk.deletionStart;
461 let newLineNumber = hunk.additionStart;
462 for (const segment of hunk.hunkContent) {
463 if (segment.type === "context") {
464 const contextIndex = findContextIndex(oldLineNumber, newLineNumber, segment.lines);
465 if (contextIndex >= 0) return contextIndex;
466 rowIndex += segment.lines;
467 oldLineNumber += segment.lines;
468 newLineNumber += segment.lines;
469 continue;
470 }
471
472 if (
473 selectedSide === "left" &&
474 lineNumber >= oldLineNumber &&
475 lineNumber < oldLineNumber + segment.deletions
476 ) {
477 return rowIndex + lineNumber - oldLineNumber;
478 }
479 rowIndex += segment.deletions;
480 oldLineNumber += segment.deletions;
481
482 if (
483 selectedSide === "right" &&
484 lineNumber >= newLineNumber &&
485 lineNumber < newLineNumber + segment.additions
486 ) {
487 return rowIndex + lineNumber - newLineNumber;
488 }
489 rowIndex += segment.additions;
490 newLineNumber += segment.additions;
491 }
492
493 oldContextStart = hunk.deletionStart + hunk.deletionCount;
494 newContextStart = hunk.additionStart + hunk.additionCount;
495 if (hunk.deletionCount === 0) oldContextStart += 1;
496 if (hunk.additionCount === 0) newContextStart += 1;
497 }
498
499 if (!includeExpandedContext) return -1;
500 const trailingLines = Math.min(
501 fileDiff.deletionLines.length - oldContextStart + 1,
502 fileDiff.additionLines.length - newContextStart + 1,
503 );
504 return findContextIndex(oldContextStart, newContextStart, trailingLines);
505 };
506
507 const selectedSide = side === "deletions" ? "left" : "right";
508 const preferredIndex = findOnSide(selectedSide);
509 return preferredIndex >= 0
510 ? preferredIndex
511 : findOnSide(selectedSide === "left" ? "right" : "left");
512}
513
514/** Resolve the host-facing coordinates of a line selected in the diff viewer. */
515export function resolveDiffReviewPosition(
516 fileDiff: FileDiffMetadata,
517 lineNumber: number,
518 side: SelectionSide | undefined,
519): PullRequestReviewPosition | null {
520 const lineIndex = findDiffReviewLineIndex(fileDiff, lineNumber, side);
521 if (lineIndex < 0) return null;
522 const line = buildDiffReviewLines(fileDiff, !fileDiff.isPartial, {
523 startIndex: lineIndex,
524 endIndex: lineIndex,
525 })[0];
526 if (line === undefined) return null;
527
528 switch (line.change) {
529 case "add":
530 return line.newLineNumber === null ? null : { kind: "added", newLine: line.newLineNumber };
531 case "delete":
532 return line.oldLineNumber === null ? null : { kind: "deleted", oldLine: line.oldLineNumber };
533 case "context":
534 return line.oldLineNumber === null || line.newLineNumber === null
535 ? null
536 : {
537 kind: "context",
538 oldLine: line.oldLineNumber,
539 newLine: line.newLineNumber,
540 side: side === "deletions" ? "left" : "right",
541 };
542 }
371}543}
372544
373function getDiffRange(545function getDiffRange(
...@@ -416,18 +588,27 @@ export function buildDiffReviewComment(input: {...@@ -416,18 +588,27 @@ export function buildDiffReviewComment(input: {
416 range: SelectedLineRange;588 range: SelectedLineRange;
417 text: string;589 text: string;
418}): ReviewCommentContext | null {590}): ReviewCommentContext | null {
419 const lines = buildDiffReviewLines(input.fileDiff);591 const includeExpandedContext = !input.fileDiff.isPartial;
420 const startIndex = findDiffReviewLineIndex(lines, input.range.start, input.range.side);592 const startIndex = findDiffReviewLineIndex(
593 input.fileDiff,
594 input.range.start,
595 input.range.side,
596 includeExpandedContext,
597 );
421 const endIndex = findDiffReviewLineIndex(598 const endIndex = findDiffReviewLineIndex(
422 lines,599 input.fileDiff,
423 input.range.end,600 input.range.end,
424 input.range.endSide ?? input.range.side,601 input.range.endSide ?? input.range.side,
602 includeExpandedContext,
425 );603 );
426 if (startIndex < 0 || endIndex < 0) return null;604 if (startIndex < 0 || endIndex < 0) return null;
427605
428 const normalizedStartIndex = Math.min(startIndex, endIndex);606 const normalizedStartIndex = Math.min(startIndex, endIndex);
429 const normalizedEndIndex = Math.max(startIndex, endIndex);607 const normalizedEndIndex = Math.max(startIndex, endIndex);
430 const selectedLines = lines.slice(normalizedStartIndex, normalizedEndIndex + 1);608 const selectedLines = buildDiffReviewLines(input.fileDiff, includeExpandedContext, {
609 startIndex: normalizedStartIndex,
610 endIndex: normalizedEndIndex,
611 });
431 const oldRange = getDiffRange(selectedLines, "oldLineNumber");612 const oldRange = getDiffRange(selectedLines, "oldLineNumber");
432 const newRange = getDiffRange(selectedLines, "newLineNumber");613 const newRange = getDiffRange(selectedLines, "newLineNumber");
433614
...@@ -445,6 +626,12 @@ export function buildDiffReviewComment(input: {...@@ -445,6 +626,12 @@ export function buildDiffReviewComment(input: {
445 ...selectedLines.map((line) => `${getDiffChangeMarker(line.change)}${line.content}`),626 ...selectedLines.map((line) => `${getDiffChangeMarker(line.change)}${line.content}`),
446 ].join("\n"),627 ].join("\n"),
447 fenceLanguage: "diff",628 fenceLanguage: "diff",
629 selection: {
630 start: input.range.start,
631 side: input.range.side ?? "additions",
632 end: input.range.end,
633 endSide: input.range.endSide ?? input.range.side ?? "additions",
634 },
448 };635 };
449}636}
450637
packages/contracts/src/pullRequest.ts+21-2
...@@ -856,6 +856,26 @@ export const PullRequestCommentUpdateInput = Schema.Struct({...@@ -856,6 +856,26 @@ export const PullRequestCommentUpdateInput = Schema.Struct({
856});856});
857export type PullRequestCommentUpdateInput = typeof PullRequestCommentUpdateInput.Type;857export type PullRequestCommentUpdateInput = typeof PullRequestCommentUpdateInput.Type;
858858
859/** The coordinates of one line in a pull request diff. */
860export const PullRequestReviewPosition = Schema.Union([
861 Schema.Struct({
862 kind: Schema.Literal("added"),
863 newLine: PositiveInt,
864 }),
865 Schema.Struct({
866 kind: Schema.Literal("deleted"),
867 oldLine: PositiveInt,
868 }),
869 Schema.Struct({
870 kind: Schema.Literal("context"),
871 oldLine: PositiveInt,
872 newLine: PositiveInt,
873 /** Which copy of an unchanged line the reviewer selected in a split diff. */
874 side: PullRequestDiffSide,
875 }),
876]);
877export type PullRequestReviewPosition = typeof PullRequestReviewPosition.Type;
878
859/** One remark in a review that has not been sent yet, anchored to a line of the diff. */879/** One remark in a review that has not been sent yet, anchored to a line of the diff. */
860export const PullRequestReviewCommentDraft = Schema.Struct({880export const PullRequestReviewCommentDraft = Schema.Struct({
861 path: TrimmedNonEmptyString,881 path: TrimmedNonEmptyString,
...@@ -865,8 +885,7 @@ export const PullRequestReviewCommentDraft = Schema.Struct({...@@ -865,8 +885,7 @@ export const PullRequestReviewCommentDraft = Schema.Struct({
865 * the hosts that address a comment by one path ignore this.885 * the hosts that address a comment by one path ignore this.
866 */886 */
867 oldPath: Schema.optional(TrimmedNonEmptyString),887 oldPath: Schema.optional(TrimmedNonEmptyString),
868 line: PositiveInt,888 position: PullRequestReviewPosition,
869 side: PullRequestDiffSide,
870 body: CommentBody,889 body: CommentBody,
871});890});
872export type PullRequestReviewCommentDraft = typeof PullRequestReviewCommentDraft.Type;891export type PullRequestReviewCommentDraft = typeof PullRequestReviewCommentDraft.Type;