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

apps/mobile/src/features/review/reviewModel.ts

@@ -1,6 +1,7 @@
 import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles";
 import type { ChangeTypes, FileDiffMetadata } from "@pierre/diffs/types";
 import type { OrchestrationCheckpointSummary, ReviewDiffPreviewSource } from "@t3tools/contracts";
+import { unquoteGitPatchPath } from "@t3tools/shared/gitPatchPath";
 import * as Arr from "effect/Array";
 import { pipe } from "effect/Function";
 import * as Order from "effect/Order";
@@ -372,8 +373,8 @@ function buildRenderableRows(file: FileDiffMetadata): ReadonlyArray<ReviewRender
 }
 
 function mapRenderableFile(file: FileDiffMetadata): ReviewRenderableFile {
-  const path = file.name || file.prevName || "";
-  const previousPath = file.prevName || null;
+  const path = unquoteGitPatchPath(file.name || file.prevName || "");
+  const previousPath = file.prevName ? unquoteGitPatchPath(file.prevName) : null;
   const additions = file.hunks.reduce((total, hunk) => total + hunk.additionLines, 0);
   const deletions = file.hunks.reduce((total, hunk) => total + hunk.deletionLines, 0);
   const cacheKey = file.cacheKey ?? `${previousPath ?? "none"}:${path}:${file.type}`;

apps/server/package.json

@@ -28,6 +28,7 @@
     "@effect/platform-node-shared": "catalog:",
     "@ff-labs/fff-node": "0.9.4",
     "@opencode-ai/sdk": "^1.3.15",
+    "diff": "8.0.3",
     "effect": "catalog:",
     "msgpackr-extract": "3.0.4",
     "node-pty": "^1.1.0",

apps/server/src/auth/RpcAuthorization.ts

@@ -76,6 +76,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,
@@ -84,6 +85,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/persistence/Errors.ts

@@ -167,5 +167,6 @@ export type OrchestrationCommandReceiptRepositoryError =
 export type ProviderSessionRuntimeRepositoryError = PersistenceSqlError | PersistenceDecodeError;
 export type AuthPairingLinkRepositoryError = PersistenceSqlError | PersistenceDecodeError;
 export type AuthSessionRepositoryError = PersistenceSqlError | PersistenceDecodeError;
+export type PullRequestFilesViewedRepositoryError = PersistenceSqlError | PersistenceDecodeError;
 
 export type ProjectionRepositoryError = PersistenceSqlError | PersistenceDecodeError;

apps/server/src/persistence/Migrations.ts

@@ -64,6 +64,7 @@ import Migration0049 from "./Migrations/049_ProjectionThreadsActiveOrderKey.ts";
 import Migration0050 from "./Migrations/050_ProjectionThreadPullRequests.ts";
 import Migration0051 from "./Migrations/051_ProjectionThreadMessageContext.ts";
 import Migration0052 from "./Migrations/052_ProjectionThreadTitleState.ts";
+import Migration0053 from "./Migrations/053_PullRequestFilesViewed.ts";
 
 /**
  * Migration loader with all migrations defined inline.
@@ -128,6 +129,7 @@ const migrationEntries = [
   [50, "ProjectionThreadPullRequests", Migration0050],
   [51, "ProjectionThreadMessageContext", Migration0051],
   [52, "ProjectionThreadTitleState", Migration0052],
+  [53, "PullRequestFilesViewed", Migration0053],
 ] as const;
 
 export const migrationManifest = migrationEntries.map(([id, name]) => [id, name] as const);

apps/server/src/persistence/Migrations/053_PullRequestFilesViewed.ts

@@ -0,0 +1,25 @@
+import * as Effect from "effect/Effect";
+import * as SqlClient from "effect/unstable/sql/SqlClient";
+
+export default Effect.gen(function* () {
+  const sql = yield* SqlClient.SqlClient;
+
+  // One row per file a reader has cleared on a host that keeps no record of its own. `revision`
+  // is what the file was when it was cleared, so a push that changes it is reported as changed
+  // rather than silently left ticked, and it is nullable for the reason `PullRequestFileViewedMark`
+  // gives. Unticking deletes the row: absent is the resting state, and a table of "not viewed"
+  // rows would grow with every diff anybody scrolled past.
+  yield* sql`
+    CREATE TABLE IF NOT EXISTS pull_request_files_viewed (
+      provider TEXT NOT NULL,
+      host TEXT NOT NULL,
+      repository TEXT NOT NULL,
+      number INTEGER NOT NULL,
+      viewer TEXT NOT NULL,
+      path TEXT NOT NULL,
+      revision TEXT,
+      viewed_at TEXT NOT NULL,
+      PRIMARY KEY (provider, host, repository, number, viewer, path)
+    ) WITHOUT ROWID
+  `;
+});

apps/server/src/persistence/PullRequestFilesViewed.ts

@@ -0,0 +1,184 @@
+import * as Context from "effect/Context";
+import * as Effect from "effect/Effect";
+import * as Layer from "effect/Layer";
+import * as Schema from "effect/Schema";
+import * as SqlClient from "effect/unstable/sql/SqlClient";
+import * as SqlSchema from "effect/unstable/sql/SqlSchema";
+
+import { SourceControlProviderKind } from "@t3tools/contracts";
+
+import {
+  PersistenceDecodeError,
+  PersistenceSqlError,
+  type PullRequestFilesViewedRepositoryError,
+} from "./Errors.ts";
+
+/**
+ * Which change request, on which host, for which reader. The host is part of it because the same
+ * `group/project` exists on gitlab.com and on a self-managed instance, and the reader because
+ * signing in as somebody else must not inherit their ticks. A host that will not say who the
+ * reader is leaves it empty, which is one reader rather than none.
+ */
+export const PullRequestFilesViewedScope = Schema.Struct({
+  provider: SourceControlProviderKind,
+  host: Schema.String,
+  repository: Schema.String,
+  number: Schema.Int,
+  viewer: Schema.String,
+});
+export type PullRequestFilesViewedScope = typeof PullRequestFilesViewedScope.Type;
+
+/** A file this reader cleared, and what it was when they cleared it. */
+export const PullRequestFileViewedMark = Schema.Struct({
+  path: Schema.String,
+  /**
+   * The host's own name for that version of the file, opaque here. Empty where the host said it
+   * had none to give, which is an answer rather than a gap: a file with no version at the head is
+   * one the change request deletes. Null where the host could not say at all, which is no baseline
+   * rather than an empty one, and such a mark stays cleared until a press replaces it.
+   *
+   * This null is the only one this environment invents; the other two are in
+   * `docs/internals/pull-request-file-revisions.md`.
+   */
+  revision: Schema.NullOr(Schema.String),
+});
+export type PullRequestFileViewedMark = typeof PullRequestFileViewedMark.Type;
+
+export interface SetPullRequestFilesViewedInput extends PullRequestFilesViewedScope {
+  readonly files: ReadonlyArray<PullRequestFileViewedMark & { readonly viewed: boolean }>;
+  /** When the presses landed, as an ISO instant. */
+  readonly viewedAt: string;
+}
+
+/**
+ * How many marks one read of this store carries. Every one of them is a path held in a set and a
+ * map for as long as the caller holds the read, so an unbounded read of a change request with
+ * thousands of marks in it is paid for again per scope the caller is holding. Matched to what the
+ * GitHub reader walks in one go, past what anyone reviews in a sitting.
+ */
+export const MAX_FILES_VIEWED_ROWS = 500;
+
+/** The marks for one scope, and whether the store had more of them than it carried. */
+export interface PullRequestFilesViewedPage {
+  readonly files: ReadonlyArray<PullRequestFileViewedMark>;
+  readonly truncated: boolean;
+}
+
+/**
+ * The marks this environment keeps for hosts that keep none of their own.
+ *
+ * Only cleared files are rows. Unticking deletes rather than writing a "not viewed" row, so the
+ * table holds what a reader has done and not what they have merely scrolled past.
+ */
+export class PullRequestFilesViewedRepository extends Context.Service<
+  PullRequestFilesViewedRepository,
+  {
+    readonly list: (
+      input: PullRequestFilesViewedScope,
+    ) => Effect.Effect<PullRequestFilesViewedPage, PullRequestFilesViewedRepositoryError>;
+    readonly set: (
+      input: SetPullRequestFilesViewedInput,
+    ) => Effect.Effect<void, PullRequestFilesViewedRepositoryError>;
+  }
+>()("t3/persistence/PullRequestFilesViewed/PullRequestFilesViewedRepository") {}
+
+function toSqlOrDecodeError(sqlOperation: string, decodeOperation: string) {
+  return (cause: unknown): PullRequestFilesViewedRepositoryError =>
+    Schema.isSchemaError(cause)
+      ? PersistenceDecodeError.fromSchemaError(decodeOperation, cause)
+      : new PersistenceSqlError({ operation: sqlOperation, cause });
+}
+
+const make = Effect.gen(function* () {
+  const sql = yield* SqlClient.SqlClient;
+
+  const listRows = SqlSchema.findAll({
+    Request: PullRequestFilesViewedScope,
+    Result: PullRequestFileViewedMark,
+    execute: ({ provider, host, repository, number, viewer }) =>
+      sql`
+        SELECT
+          path AS "path",
+          revision AS "revision"
+        FROM pull_request_files_viewed
+        WHERE provider = ${provider}
+          AND host = ${host}
+          AND repository = ${repository}
+          AND number = ${number}
+          AND viewer = ${viewer}
+        ORDER BY path
+        LIMIT ${MAX_FILES_VIEWED_ROWS + 1}
+      `,
+  });
+
+  return PullRequestFilesViewedRepository.of({
+    // Ordered by path and read one row past the ceiling, so the same marks come back on every
+    // read rather than a window that shuffles, and having more than were carried is known rather
+    // than guessed at from a full page.
+    list: (input) =>
+      listRows(input).pipe(
+        Effect.map((rows) => ({
+          files: rows.slice(0, MAX_FILES_VIEWED_ROWS),
+          truncated: rows.length > MAX_FILES_VIEWED_ROWS,
+        })),
+        Effect.mapError(toSqlOrDecodeError("listPullRequestFilesViewed", "PullRequestFileViewed")),
+      ),
+
+    // One statement per file rather than one for the batch: the batch is what a reader ticked in
+    // the last few hundred milliseconds, so it is a handful of rows on a local database, and a
+    // mixed batch of clears and un-clears has no single statement anyway.
+    set: (input) =>
+      // One transaction for the batch. A press is a handful of files, and a failure part way
+      // through would otherwise leave some of them cleared and the rest not, which the reader
+      // sees on the next read as marks they never made.
+      sql
+        .withTransaction(
+          Effect.forEach(
+            input.files,
+            (file) =>
+              file.viewed
+                ? sql`
+                INSERT INTO pull_request_files_viewed (
+                  provider,
+                  host,
+                  repository,
+                  number,
+                  viewer,
+                  path,
+                  revision,
+                  viewed_at
+                )
+                VALUES (
+                  ${input.provider},
+                  ${input.host},
+                  ${input.repository},
+                  ${input.number},
+                  ${input.viewer},
+                  ${file.path},
+                  ${file.revision},
+                  ${input.viewedAt}
+                )
+                ON CONFLICT (provider, host, repository, number, viewer, path)
+                DO UPDATE SET revision = excluded.revision, viewed_at = excluded.viewed_at
+              `
+                : sql`
+                DELETE FROM pull_request_files_viewed
+                WHERE provider = ${input.provider}
+                  AND host = ${input.host}
+                  AND repository = ${input.repository}
+                  AND number = ${input.number}
+                  AND viewer = ${input.viewer}
+                  AND path = ${file.path}
+              `,
+            { discard: true },
+          ),
+        )
+        .pipe(
+          Effect.mapError(
+            (cause) => new PersistenceSqlError({ operation: "setPullRequestFilesViewed", cause }),
+          ),
+        ),
+  });
+});
+
+export const layer = Layer.effect(PullRequestFilesViewedRepository, make);