Skip to content

Commit 707e7ab

Browse files
committed
fix(source-control): address Origin review findings
1 parent 00a0cdb commit 707e7ab

4 files changed

Lines changed: 188 additions & 70 deletions

File tree

‎packages/source-control-cursor-origin/src/server/OriginPullRequestProvider.ts‎

Lines changed: 131 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import * as Cache from "effect/Cache";
22
import * as Effect from "effect/Effect";
33
import * as Exit from "effect/Exit";
44
import * as Fiber from "effect/Fiber";
5+
import * as Hash from "effect/Hash";
56
import * as Option from "effect/Option";
67
import * as Result from "effect/Result";
78
import * as Schema from "effect/Schema";
@@ -128,6 +129,7 @@ const editedAt = (comment: OriginComment) =>
128129
function toThreads(
129130
comments: ReadonlyArray<OriginComment>,
130131
avatars: OriginAvatars,
132+
headSha: string | null,
131133
): PullRequestReviewThread[] {
132134
const threads = new Map<string, PullRequestReviewThread>();
133135
for (const comment of comments) {
@@ -140,7 +142,8 @@ function toThreads(
140142
line: line !== null && line > 0 ? line : null,
141143
side: thread.side === "left" ? ("left" as const) : ("right" as const),
142144
isResolved: Boolean(thread.resolvedAt),
143-
isOutdated: false,
145+
// Origin anchors a thread to the version it was left on and does not move it forward.
146+
isOutdated: Boolean(headSha && thread.version?.headSha && thread.version.headSha !== headSha),
144147
comments: [],
145148
};
146149
threads.set(thread.id, {
@@ -335,19 +338,41 @@ export const make = Effect.gen(function* () {
335338
nextCursor: null,
336339
})),
337340
);
341+
const versionKey = (pullRequest: OriginPullRequest) => {
342+
const head = pullRequest.version?.headSha ?? pullRequest.head.sha;
343+
const base = pullRequest.version?.baseSha ?? pullRequest.base.sha;
344+
return head && base ? `${base}...${head}` : null;
345+
};
338346
// Keyed by `cwd, repository, number, base...head` for a pull request's patch, or by
339347
// `cwd, repository, , commit` for one commit's.
340348
const patches = yield* Cache.makeWith(
341349
(key: string) => {
342350
const [cwd = "", repository = "", number = "", commits = ""] = key.split("\n");
343-
return readPatch(
344-
cwd,
345-
number === ""
346-
? `${OriginCli.originRepositoryPath(repository)}/commits/${encodeURIComponent(commits)}/files`
347-
: pullPath(repository, Number(number), "/files"),
351+
if (number === "")
352+
return readPatch(
353+
cwd,
354+
`${OriginCli.originRepositoryPath(repository)}/commits/${encodeURIComponent(commits)}/files`,
355+
).pipe(Effect.map((patch) => ({ ...patch, pinned: true })));
356+
// `/files` always answers for the newest version, so a push during the read must not be
357+
// kept under the version the key names.
358+
return readPatch(cwd, pullPath(repository, Number(number), "/files")).pipe(
359+
Effect.flatMap((patch) =>
360+
recover(
361+
cli
362+
.api(
363+
{ cwd, operation: "getPullRequest", path: pullPath(repository, Number(number)) },
364+
OriginPullRequestSchema,
365+
)
366+
.pipe(Effect.map(versionKey)),
367+
null,
368+
).pipe(Effect.map((version) => ({ ...patch, pinned: version === commits }))),
369+
),
348370
);
349371
},
350-
{ capacity: 64, timeToLive: (exit) => (Exit.isSuccess(exit) ? "1 hour" : 0) },
372+
{
373+
capacity: 64,
374+
timeToLive: (exit) => (Exit.isSuccess(exit) && exit.value.pinned ? "1 hour" : 0),
375+
},
351376
);
352377

353378
// Every pull request into a branch compares against the same tip, so it is read once a moment.
@@ -518,6 +543,33 @@ export const make = Effect.gen(function* () {
518543
),
519544
);
520545

546+
// Origin pages by a token longer than the service's cursor can carry, so the cursor names it by
547+
// hash. A cursor from before a restart finds nothing and ends the list there.
548+
const pageTokens = new Map<string, string>();
549+
const pageTokenKey = (token: string) => {
550+
const key = `${(Hash.string(token) >>> 0).toString(36)}-${token.length}`;
551+
pageTokens.delete(key);
552+
pageTokens.set(key, token);
553+
if (pageTokens.size > 256) pageTokens.delete(pageTokens.keys().next().value ?? "");
554+
return key;
555+
};
556+
557+
// Origin merges a stacked pull request together with every open one below it, which the panel
558+
// does not show, so those merges are left to Origin.
559+
const refuseStackedMerge = Effect.fn("OriginPullRequestProvider.refuseStackedMerge")(function* (
560+
input: ProviderRepositoryRef & { readonly number: number },
561+
) {
562+
const read = (number: number) =>
563+
getPull({ ...input, number }).pipe(Effect.mapError(fail("runAction")));
564+
const parent = (yield* read(input.number)).stack?.parentPullRequest?.number;
565+
if (parent == null) return;
566+
if (originState(yield* read(Number(parent))) !== "open") return;
567+
return yield* unsupported(
568+
"runAction",
569+
`This pull request is stacked on #${parent}. Merge #${parent} first, or merge the stack on Cursor Origin.`,
570+
);
571+
});
572+
521573
const provider: PullRequestProviderApi = {
522574
kind: SourceControlProviderKind.make("cursor-origin"),
523575
capabilities: CAPABILITIES,
@@ -555,11 +607,16 @@ export const make = Effect.gen(function* () {
555607
listChangeRequests: Effect.fn("OriginPullRequestProvider.listChangeRequests")(
556608
function* (input) {
557609
const limit = Math.min(Math.max(input.limit, 1), PAGE_SIZE);
610+
const resume = input.cursor?.resumeAfter;
611+
const pageToken = resume === undefined ? undefined : pageTokens.get(resume);
612+
if (resume !== undefined && pageToken === undefined)
613+
return { items: [], truncated: false, continues: false };
558614
const query = new URLSearchParams({
559615
state: input.state,
560616
pageSize: String(limit),
561617
sortBy: "updated",
562618
...(input.involvement === "authored" ? { author: input.viewer } : {}),
619+
...(pageToken === undefined ? {} : { pageToken }),
563620
});
564621
const avatarArgs = [
565622
"pr",
@@ -596,11 +653,12 @@ export const make = Effect.gen(function* () {
596653
pullRequests.map((pullRequest) => pullRequest.author),
597654
avatarArgs,
598655
);
656+
const next = page.nextPageToken || null;
599657
return {
600658
items: pullRequests.map((pullRequest) => toProviderChangeRequest(pullRequest, avatars)),
601-
truncated: Boolean(page.nextPageToken),
602-
// Pages continue by an opaque token, which the service's date cursor cannot carry.
603-
continues: false,
659+
truncated: next !== null,
660+
continues: next !== null,
661+
...(next === null ? {} : { resumeAfter: pageTokenKey(next) }),
604662
};
605663
},
606664
Effect.mapError(fail("listChangeRequests")),
@@ -713,70 +771,78 @@ export const make = Effect.gen(function* () {
713771
unread,
714772
true,
715773
),
774+
// The detail is read alongside, so this is usually its cached answer.
775+
recover(getPull(input), null),
716776
],
717-
{ concurrency: 3 },
777+
{ concurrency: 4 },
718778
).pipe(
719-
Effect.flatMap(([comments, reviews, commits]) =>
779+
Effect.flatMap(([comments, reviews, commits, pullRequest]) =>
720780
avatarsFor(
721781
input.cwd,
722782
[
723783
...comments.items.map((comment) => comment.author),
724784
...reviews.items.map((review) => review.author),
725785
],
726786
viewArgs(input, "author,reviews,comments"),
727-
).pipe(Effect.map((avatars) => [comments, reviews, commits, avatars] as const)),
787+
).pipe(
788+
Effect.map(
789+
(avatars) =>
790+
[comments, reviews, commits, avatars, pullRequest?.head.sha ?? null] as const,
791+
),
792+
),
728793
),
729794
Effect.mapError(fail("getChangeRequestActivity")),
730-
Effect.map(([comments, reviews, commits, avatars]): ProviderChangeRequestActivity => {
731-
const discussion: PullRequestComment[] = comments.items
732-
.filter((comment) => !isThreadComment(comment))
733-
.map((comment) => ({
734-
id: comment.id,
735-
kind: "issue-comment",
736-
author: originActor(comment.author, avatars),
737-
body: comment.body ?? "",
738-
createdAt: comment.createdAt,
739-
editedAt: editedAt(comment),
740-
url: null,
741-
path: null,
742-
reviewState: null,
743-
}));
744-
const reviewComments: PullRequestComment[] = reviews.items
745-
.filter((review) => !review.dismissal?.dismissedAt)
746-
.map((review) => ({
795+
Effect.map(
796+
([comments, reviews, commits, avatars, headSha]): ProviderChangeRequestActivity => {
797+
const discussion: PullRequestComment[] = comments.items
798+
.filter((comment) => !isThreadComment(comment))
799+
.map((comment) => ({
800+
id: comment.id,
801+
kind: "issue-comment",
802+
author: originActor(comment.author, avatars),
803+
body: comment.body ?? "",
804+
createdAt: comment.createdAt,
805+
editedAt: editedAt(comment),
806+
url: null,
807+
path: null,
808+
reviewState: null,
809+
}));
810+
const reviewComments: PullRequestComment[] = reviews.items.map((review) => ({
747811
id: review.id,
748812
kind: "review",
749813
author: originActor(review.author, avatars),
750814
body: review.body ?? "",
751815
createdAt: review.submittedAt,
752816
url: null,
753817
path: null,
754-
reviewState:
755-
review.verdict === "approve"
818+
reviewState: review.dismissal?.dismissedAt
819+
? "DISMISSED"
820+
: review.verdict === "approve"
756821
? "APPROVED"
757822
: review.verdict === "request_changes"
758823
? "CHANGES_REQUESTED"
759824
: "COMMENTED",
760825
}));
761-
const all = [...discussion, ...reviewComments].toSorted((left, right) =>
762-
left.createdAt.localeCompare(right.createdAt),
763-
);
764-
return {
765-
comments: all,
766-
commentCount: all.length,
767-
commentsTruncated: comments.truncated || reviews.truncated,
768-
reviewThreadsTruncated: comments.truncated,
769-
reviewThreads: toThreads(comments.items, avatars),
770-
commits: commits.items.map((commit) => ({
771-
oid: commit.sha,
772-
messageHeadline: commit.commit.message.split("\n")[0] ?? "",
773-
committedDate:
774-
commit.commit.committer?.date ?? commit.commit.author?.date ?? UNKNOWN_TIME,
775-
...(commit.stats?.additions == null ? {} : { additions: commit.stats.additions }),
776-
...(commit.stats?.deletions == null ? {} : { deletions: commit.stats.deletions }),
777-
})),
778-
};
779-
}),
826+
const all = [...discussion, ...reviewComments].toSorted((left, right) =>
827+
left.createdAt.localeCompare(right.createdAt),
828+
);
829+
return {
830+
comments: all,
831+
commentCount: all.length,
832+
commentsTruncated: comments.truncated || reviews.truncated,
833+
reviewThreadsTruncated: comments.truncated,
834+
reviewThreads: toThreads(comments.items, avatars, headSha),
835+
commits: commits.items.map((commit) => ({
836+
oid: commit.sha,
837+
messageHeadline: commit.commit.message.split("\n")[0] ?? "",
838+
committedDate:
839+
commit.commit.committer?.date ?? commit.commit.author?.date ?? UNKNOWN_TIME,
840+
...(commit.stats?.additions == null ? {} : { additions: commit.stats.additions }),
841+
...(commit.stats?.deletions == null ? {} : { deletions: commit.stats.deletions }),
842+
})),
843+
};
844+
},
845+
),
780846
);
781847
},
782848

@@ -789,18 +855,20 @@ export const make = Effect.gen(function* () {
789855
(input.commit === undefined
790856
? getPull(input).pipe(
791857
Effect.flatMap((pullRequest) => {
792-
const head = pullRequest.version?.headSha ?? pullRequest.head.sha;
793-
const base = pullRequest.version?.baseSha ?? pullRequest.base.sha;
794-
return head && base
858+
const version = versionKey(pullRequest);
859+
return version
795860
? Cache.get(
796861
patches,
797-
`${input.cwd}\n${input.repository}\n${input.number}\n${base}...${head}`,
862+
`${input.cwd}\n${input.repository}\n${input.number}\n${version}`,
798863
)
799864
: readPatch(input.cwd, pullPath(input.repository, input.number, "/files"));
800865
}),
801866
)
802867
: Cache.get(patches, `${input.cwd}\n${input.repository}\n\n${input.commit}`)
803-
).pipe(Effect.mapError(fail("getDiff"))),
868+
).pipe(
869+
Effect.map(({ patch, truncated, nextCursor }) => ({ patch, truncated, nextCursor })),
870+
Effect.mapError(fail("getDiff")),
871+
),
804872

805873
listReviewerCandidates: () =>
806874
unsupported("listReviewerCandidates", "Cursor Origin does not list reviewer candidates."),
@@ -817,9 +885,13 @@ export const make = Effect.gen(function* () {
817885
const action = (() => {
818886
switch (input.action) {
819887
case "merge":
820-
return write(input, "runAction", "POST", `${path}/merge`, {
821-
mergeMethod: input.mergeMethod === "squash" ? "squash" : "merge",
822-
});
888+
return refuseStackedMerge(input).pipe(
889+
Effect.andThen(
890+
write(input, "runAction", "POST", `${path}/merge`, {
891+
mergeMethod: input.mergeMethod === "squash" ? "squash" : "merge",
892+
}),
893+
),
894+
);
823895
case "close":
824896
return write(input, "runAction", "PATCH", path, { state: "closed" });
825897
case "reopen":

‎packages/source-control-cursor-origin/src/server/OriginSourceControlProvider.ts‎

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import * as Effect from "effect/Effect";
22
import * as FileSystem from "effect/FileSystem";
33
import * as Schema from "effect/Schema";
44
import { SourceControlProviderError, SourceControlProviderKind } from "@t3tools/contracts";
5-
import { CURSOR_ORIGIN_WEB_BASE_URL } from "../shared/definition.ts";
5+
import { CURSOR_ORIGIN_WEB_BASE_URL, parseChangeRequestUrl } from "../shared/definition.ts";
66
import * as SourceControlProvider from "@t3tools/source-control-core/server/SourceControlProvider";
77
import * as SourceControlHost from "@t3tools/source-control-core/server/SourceControlHost";
88
import {
@@ -79,6 +79,16 @@ function originPullNumber(reference: string): number | null {
7979
return Number.isSafeInteger(number) && number > 0 ? number : null;
8080
}
8181

82+
/** A cursor.com pull request URL names its own repository, which need not be the checkout's. */
83+
function pullReference(reference: string) {
84+
const trimmed = reference.trim();
85+
const link = URL.canParse(trimmed) ? parseChangeRequestUrl(new URL(trimmed)) : null;
86+
return {
87+
repository: link?.repository ?? null,
88+
number: link?.number ?? originPullNumber(trimmed),
89+
};
90+
}
91+
8292
const cloneUrls = (repository: OriginRepository, fallbackName: string) => {
8393
const nameWithOwner =
8494
repository.fullName ??
@@ -173,8 +183,9 @@ export const make = Effect.gen(function* () {
173183

174184
getChangeRequest: Effect.fn("OriginSourceControlProvider.getChangeRequest")(
175185
function* (input) {
176-
const repository = yield* cli.resolveRepository(input);
177-
const number = originPullNumber(input.reference);
186+
const reference = pullReference(input.reference);
187+
const repository = reference.repository ?? (yield* cli.resolveRepository(input));
188+
const number = reference.number;
178189
if (number === null)
179190
return yield* new OriginCli.OriginCliError({
180191
operation: "getChangeRequest",
@@ -268,8 +279,8 @@ export const make = Effect.gen(function* () {
268279
);
269280
// Origin creates repositories internal (visible to the whole team) and only takes a
270281
// visibility on update, so the repository is made private straight after.
271-
return cloneUrls(
272-
yield* cli.api(
282+
const madePrivate = yield* cli
283+
.api(
273284
{
274285
cwd: input.cwd,
275286
operation: "createRepository",
@@ -278,9 +289,21 @@ export const make = Effect.gen(function* () {
278289
body: { visibility: "private" },
279290
},
280291
OriginRepositorySchema,
281-
),
282-
`${owner}/${name}`,
283-
);
292+
)
293+
.pipe(
294+
Effect.retry({ times: 2 }),
295+
Effect.mapError(
296+
(cause) =>
297+
new OriginCli.OriginCliError({
298+
operation: "createRepository",
299+
cwd: input.cwd,
300+
reason: "failed",
301+
detail: `Created ${owner}/${name}, but could not make it private, so your Cursor team can still see it. Set it to private on cursor.com.`,
302+
cause,
303+
}),
304+
),
305+
);
306+
return cloneUrls(madePrivate, `${owner}/${name}`);
284307
},
285308
(effect, input) =>
286309
effect.pipe(mapError("createRepository", input.cwd, { repository: input.repository })),
@@ -299,7 +322,14 @@ export const make = Effect.gen(function* () {
299322
checkoutChangeRequest: Effect.fn("OriginSourceControlProvider.checkoutChangeRequest")(
300323
function* (input) {
301324
const repository = yield* cli.resolveRepository(input);
302-
const number = originPullNumber(input.reference);
325+
const { repository: linked, number } = pullReference(input.reference);
326+
if (linked !== null && linked !== repository.toLowerCase())
327+
return yield* new OriginCli.OriginCliError({
328+
operation: "checkoutChangeRequest",
329+
cwd: input.cwd,
330+
reason: "not-found",
331+
detail: `That pull request is in ${linked}, not this checkout's ${repository}.`,
332+
});
303333
yield* cli.execute({
304334
cwd: input.cwd,
305335
operation: "checkoutChangeRequest",

0 commit comments

Comments
 (0)