feat(web): pull request files can be marked as viewed (#23)

Author: yordis

apps/server/src/auth/RpcAuthorization.ts

@@ -58,6 +58,7 @@ export const RPC_REQUIRED_SCOPES = {
   [WS_METHODS.pullRequestsActivity]: AuthOrchestrationReadScope,
   [WS_METHODS.pullRequestsThreadComments]: AuthOrchestrationReadScope,
   [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope,
+  [WS_METHODS.pullRequestsFilesViewed]: AuthOrchestrationReadScope,
   [WS_METHODS.pullRequestsRunAction]: AuthOrchestrationOperateScope,
   [WS_METHODS.pullRequestsUpdate]: AuthOrchestrationOperateScope,
   [WS_METHODS.pullRequestsComment]: AuthOrchestrationOperateScope,
@@ -66,6 +67,7 @@ export const RPC_REQUIRED_SCOPES = {
   [WS_METHODS.pullRequestsReplyToThread]: AuthOrchestrationOperateScope,
   [WS_METHODS.pullRequestsSetThreadResolution]: AuthOrchestrationOperateScope,
   [WS_METHODS.pullRequestsSetReaction]: AuthOrchestrationOperateScope,
+  [WS_METHODS.pullRequestsSetFilesViewed]: AuthOrchestrationOperateScope,
   // Read scope like the reads it un-caches: refreshing is part of reading, and a read-only
   // client pressing refresh must not be told it may not look again.
   [WS_METHODS.pullRequestsInvalidate]: AuthOrchestrationReadScope,

apps/server/src/pullRequest/GitHubPullRequestCli.test.ts

@@ -2603,4 +2603,149 @@ layer("GitHubPullRequestCli.layer", (it) => {
       ]);
     }),
   );
+
+  it.effect("reads every page of viewed files, and says so when there are too many", () =>
+    Effect.gen(function* () {
+      const page = (index: number, hasNextPage: boolean) =>
+        Effect.succeed(
+          output(
+            // @effect-diagnostics-next-line preferSchemaOverJson:off
+            JSON.stringify({
+              data: {
+                repository: {
+                  pullRequest: {
+                    files: {
+                      pageInfo: { hasNextPage, endCursor: `cursor-${index}` },
+                      nodes: [
+                        { path: `src/file${index}.ts`, viewerViewedState: "VIEWED" },
+                        { path: `src/other${index}.ts`, viewerViewedState: "UNVIEWED" },
+                      ],
+                    },
+                  },
+                },
+              },
+            }),
+          ),
+        );
+      mockedExecute
+        .mockReturnValueOnce(page(0, true))
+        .mockReturnValueOnce(page(1, true))
+        .mockReturnValueOnce(page(2, false));
+      const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;
+
+      const viewed = yield* cli.getPullRequestFilesViewed({
+        cwd: "/w",
+        repository: "acme/web",
+        host: "github.com",
+        number: 7,
+      });
+
+      assert.strictEqual(mockedExecute.mock.calls.length, 3);
+      // The first page asks from the start; each one after it carries the cursor before it.
+      assert.isFalse(callAt(0).args.some((arg) => arg.startsWith("after=")));
+      expect(callAt(1).args).toContain("after=cursor-0");
+      expect(callAt(2).args).toContain("after=cursor-1");
+      assert.isFalse(viewed.truncated);
+      expect(viewed.files.map((file) => [file.path, file.state])).toEqual([
+        ["src/file0.ts", "viewed"],
+        ["src/other0.ts", "unviewed"],
+        ["src/file1.ts", "viewed"],
+        ["src/other1.ts", "unviewed"],
+        ["src/file2.ts", "viewed"],
+        ["src/other2.ts", "unviewed"],
+      ]);
+    }),
+  );
+
+  it.effect("stops paging viewed files rather than following a change without end", () =>
+    Effect.gen(function* () {
+      mockedExecute.mockReturnValue(
+        Effect.succeed(
+          output(
+            // @effect-diagnostics-next-line preferSchemaOverJson:off
+            JSON.stringify({
+              data: {
+                repository: {
+                  pullRequest: {
+                    files: {
+                      pageInfo: { hasNextPage: true, endCursor: "cursor" },
+                      nodes: [{ path: "src/file.ts", viewerViewedState: "VIEWED" }],
+                    },
+                  },
+                },
+              },
+            }),
+          ),
+        ),
+      );
+      const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;
+
+      const viewed = yield* cli.getPullRequestFilesViewed({
+        cwd: "/w",
+        repository: "acme/web",
+        host: "github.com",
+        number: 7,
+      });
+
+      assert.strictEqual(mockedExecute.mock.calls.length, 5);
+      assert.isTrue(viewed.truncated);
+      assert.strictEqual(viewed.files.length, 5);
+    }),
+  );
+
+  it.effect("clears and restores a burst of files in one request", () =>
+    Effect.gen(function* () {
+      mockedExecute
+        // @effect-diagnostics-next-line preferSchemaOverJson:off
+        .mockReturnValueOnce(
+          Effect.succeed(
+            output(JSON.stringify({ data: { repository: { pullRequest: { id: "PR_1" } } } })),
+          ),
+        )
+        .mockReturnValueOnce(Effect.succeed(output("{}")));
+      const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;
+
+      yield* cli.setPullRequestFilesViewed({
+        cwd: "/w",
+        repository: "acme/web",
+        host: "github.com",
+        number: 7,
+        files: [
+          { path: "src/a.ts", viewed: true },
+          { path: "src/b.ts", viewed: false },
+        ],
+      });
+
+      // One request to learn the pull request's node id, one for every press together.
+      assert.strictEqual(mockedExecute.mock.calls.length, 2);
+      // @effect-diagnostics-next-line preferSchemaOverJson:off
+      const sent = JSON.parse(callAt(1).stdin ?? "") as {
+        query: string;
+        variables: Record<string, string>;
+      };
+      expect(sent.query).toContain("f0: markFileAsViewed");
+      expect(sent.query).toContain("f1: unmarkFileAsViewed");
+      expect(sent.variables).toEqual({
+        pullRequestId: "PR_1",
+        path0: "src/a.ts",
+        path1: "src/b.ts",
+      });
+    }),
+  );
+
+  it.effect("asks the host nothing when nothing was pressed", () =>
+    Effect.gen(function* () {
+      const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;
+
+      yield* cli.setPullRequestFilesViewed({
+        cwd: "/w",
+        repository: "acme/web",
+        host: "github.com",
+        number: 7,
+        files: [],
+      });
+
+      assert.strictEqual(mockedExecute.mock.calls.length, 0);
+    }),
+  );
 });

apps/server/src/pullRequest/GitHubPullRequestCli.ts

@@ -7,6 +7,7 @@ import {
   resolvePullRequestAuthorFilter,
   type PullRequestAction,
   type PullRequestActor,
+  type PullRequestFileViewed,
   type PullRequestInvolvement,
   type PullRequestListFilters,
   type PullRequestListState,
@@ -30,10 +31,12 @@ import {
   ADD_REACTION_GRAPHQL_MUTATION,
   buildReviewSubmissionJson,
   buildReviewerRequestJson,
+  buildSetFilesViewedGraphQlMutation,
   decodeActorAvatarsJson,
   decodePullRequestActivityJson,
   decodePullRequestDetailJson,
   decodePullRequestFilesJson,
+  decodePullRequestFilesViewedJson,
   decodePullRequestListJson,
   decodePullRequestNodeIdJson,
   decodePullRequestSearchJson,
@@ -53,6 +56,7 @@ import {
   decodeBaseComparisonJson,
   PULL_REQUEST_DETAIL_JSON_FIELDS,
   PULL_REQUEST_LIST_JSON_FIELDS,
+  PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY,
   PULL_REQUEST_NODE_ID_GRAPHQL_QUERY,
   REACTION_SUBJECT_PULL_REQUEST_GRAPHQL_QUERY,
   REMOVE_REACTION_GRAPHQL_MUTATION,
@@ -263,6 +267,12 @@ const PULL_REQUEST_FALLBACK_MAX_ROWS = 1_000;
 
 /** What the files API serves at most in one response, which is what one slice is made of. */
 const DIFF_FILES_PAGE_SIZE = 100;
+/**
+ * How many hundred-file pages of viewed state one read will walk. A point of the hourly GraphQL
+ * budget per page, against a change request nobody reviews in one sitting past the first few
+ * hundred files: beyond this the read stops and says it was cut short.
+ */
+const FILES_VIEWED_MAX_PAGES = 5;
 
 /**
  * Pages of review threads to follow before the conversation is reported as truncated. GitHub
@@ -308,6 +318,12 @@ export interface GitHubPullRequestDiffSlice {
   readonly omittedFileStats?: ReadonlyArray<PullRequestOmittedFileStat>;
 }
 
+export interface GitHubPullRequestFilesViewed {
+  readonly files: ReadonlyArray<PullRequestFileViewed>;
+  /** GitHub had more files than the page budget below would read. */
+  readonly truncated: boolean;
+}
+
 export class GitHubPullRequestCli extends Context.Service<
   GitHubPullRequestCli,
   {
@@ -415,6 +431,30 @@ export class GitHubPullRequestCli extends Context.Service<
       GitHubPullRequestCliError
     >;
 
+    /**
+     * Which files of the pull request the signed-in account has cleared, and which of those have
+     * been pushed to since. Read apart from the patch because GitHub only reports it over GraphQL,
+     * and because the two answers go stale at completely different rates.
+     */
+    readonly getPullRequestFilesViewed: (input: {
+      readonly cwd: string;
+      readonly repository: string;
+      readonly host: string;
+      readonly number: number;
+    }) => Effect.Effect<GitHubPullRequestFilesViewed, GitHubPullRequestCliError>;
+
+    /**
+     * Clears files, or puts them back, as one request. GitHub takes a single path per mutation,
+     * so a burst is batched with aliases into one document rather than one subprocess per press.
+     */
+    readonly setPullRequestFilesViewed: (input: {
+      readonly cwd: string;
+      readonly repository: string;
+      readonly host: string;
+      readonly number: number;
+      readonly files: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>;
+    }) => Effect.Effect<void, GitHubPullRequestCliError>;
+
     readonly listReviewThreadComments: (input: {
       readonly cwd: string;
       readonly repository: string;
@@ -912,14 +952,25 @@ export const make = Effect.gen(function* () {
     readonly host: string;
     readonly query: string;
     readonly variables: Readonly<Record<string, string>>;
+    /** What this write is expected to spend, for a batch that carries more than one mutation. */
+    readonly estimatedCost?: number | undefined;
   }) =>
-    github
-      .execute({
-        cwd: input.cwd,
-        args: ["api", "graphql", "--hostname", input.host, "--input", "-"],
-        stdin: encodeGraphQlRequestJson({ query: input.query, variables: input.variables }),
-      })
-      .pipe(Effect.asVoid);
+    graphQlBudget
+      // A write is counted against the hourly budget but never held back by it, so the reserve
+      // that pauses reads is measured against what has really been spent rather than against
+      // reads alone. It cannot fail here: the budget only refuses reads.
+      .query(input.host, input.query, { estimatedCost: input.estimatedCost ?? 1 })
+      .pipe(
+        Effect.orElseSucceed(() => input.query),
+        Effect.flatMap((query) =>
+          github.execute({
+            cwd: input.cwd,
+            args: ["api", "graphql", "--hostname", input.host, "--input", "-"],
+            stdin: encodeGraphQlRequestJson({ query, variables: input.variables }),
+          }),
+        ),
+        Effect.asVoid,
+      );
 
   /** A GraphQL read whose answer is decoded, reporting a failure against the read that made it. */
   const graphqlRead = <A>(input: {
@@ -1764,6 +1815,59 @@ export const make = Effect.gen(function* () {
         variables: { threadId: input.threadId, body: input.body },
       }),
 
+    getPullRequestFilesViewed: (input) => {
+      const { owner, name } = parseRepositorySelector(input.repository);
+      const read = (
+        after: string | null,
+        collected: ReadonlyArray<PullRequestFileViewed>,
+        pagesLeft: number,
+      ): Effect.Effect<GitHubPullRequestFilesViewed, GitHubPullRequestCliError> =>
+        graphqlRead({
+          cwd: input.cwd,
+          host: input.host,
+          operation: "getPullRequestFilesViewed",
+          variables: [
+            ["-f", `owner=${owner}`],
+            ["-f", `name=${name}`],
+            ["-F", `number=${input.number}`],
+            ...(after === null
+              ? []
+              : ([["-f", `after=${after}`]] as ReadonlyArray<readonly [string, string]>)),
+          ],
+          query: PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY,
+          decode: decodePullRequestFilesViewedJson,
+        }).pipe(
+          Effect.flatMap((page) => {
+            const files = [...collected, ...page.files];
+            if (page.nextCursor === null) {
+              return Effect.succeed({ files, truncated: false });
+            }
+            // A change nobody could read in one sitting is not worth a point of budget a page:
+            // the boxes on screen still work, and the count says it is partial rather than lying.
+            return pagesLeft <= 1
+              ? Effect.succeed({ files, truncated: true })
+              : read(page.nextCursor, files, pagesLeft - 1);
+          }),
+        );
+      return read(null, [], FILES_VIEWED_MAX_PAGES);
+    },
+
+    setPullRequestFilesViewed: (input) => {
+      const mutation = buildSetFilesViewedGraphQlMutation(input.files);
+      if (mutation === null) return Effect.void;
+      return pullRequestNodeId({ ...input, operation: "setPullRequestFilesViewed" }).pipe(
+        Effect.flatMap((pullRequestId) =>
+          graphql({
+            cwd: input.cwd,
+            host: input.host,
+            query: mutation.query,
+            variables: { pullRequestId, ...mutation.variables },
+            estimatedCost: input.files.length,
+          }),
+        ),
+      );
+    },
+
     setReviewThreadResolution: (input) =>
       graphql({
         cwd: input.cwd,

apps/server/src/pullRequest/GitHubPullRequestProvider.ts

@@ -33,6 +33,7 @@ const CAPABILITIES: PullRequestCapabilities = {
   updateMethods: ["merge", "rebase"],
   search: true,
   reactions: true,
+  viewedFiles: true,
   review: {
     inlineComment: true,
     reply: true,
@@ -399,6 +400,12 @@ export const make = Effect.gen(function* () {
     getDiffFileContents: (input) =>
       cli.getPullRequestDiffFileContents(input).pipe(Effect.mapError(fail("getDiffFileContents"))),
 
+    getFilesViewed: (input) =>
+      cli.getPullRequestFilesViewed(input).pipe(Effect.mapError(fail("getFilesViewed"))),
+
+    setFilesViewed: (input) =>
+      cli.setPullRequestFilesViewed(input).pipe(Effect.mapError(fail("setFilesViewed"))),
+
     listReviewerCandidates: (input) =>
       cli.listReviewerCandidates(input).pipe(Effect.mapError(fail("listReviewerCandidates"))),