From 1f6df6bf1f50ab5b8e3253716773da2571f8243e Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sun, 11 Oct 2026 02:58:31 -0700 Subject: [PATCH] feat(pull-requests): guided reviews, a beta tour of a pull request (#18124) Co-authored-by: Claude Opus 5.5 --- apps/server/src/auth/RpcAuthorization.ts | 1 + apps/server/src/git/GitManager.test.ts | 2 + .../src/observability/RpcInstrumentation.ts | 2 + apps/server/src/persistence/Errors.ts | 1 + apps/server/src/persistence/Migrations.ts | 2 + .../Migrations/055_OrchestrationV2.test.ts | 4 +- .../Migrations/061_PullRequestReviewGuides.ts | 19 + .../persistence/PullRequestReviewGuides.ts | 84 +++ .../reconcileV2PreviewMigration.test.ts | 2 + .../provider/Drivers/CodexManagedProvider.ts | 2 + .../PullRequestReviewGuideService.test.ts | 505 ++++++++++++++++++ .../PullRequestReviewGuideService.ts | 484 +++++++++++++++++ .../pullRequestReviewGuideInput.test.ts | 57 ++ .../pullRequestReviewGuideInput.ts | 47 ++ .../pullRequestReviewGuideNormalize.test.ts | 212 ++++++++ .../pullRequestReviewGuideNormalize.ts | 184 +++++++ apps/server/src/server.ts | 9 +- apps/server/src/serverSettings.ts | 6 +- .../AntigravityTextGeneration.ts | 5 +- .../textGeneration/ClaudeTextGeneration.ts | 33 +- .../src/textGeneration/CodexTextGeneration.ts | 4 +- .../src/textGeneration/TextGeneration.test.ts | 1 + .../src/textGeneration/TextGeneration.ts | 9 +- apps/server/src/ws.ts | 14 + .../components/diffs/StyledDiffCodeView.tsx | 101 +--- .../pullRequest/PullRequestCodeTab.tsx | 45 +- .../pullRequest/PullRequestComposer.tsx | 12 + .../PullRequestDetailPanel.test.tsx | 10 +- .../pullRequest/PullRequestDetailPanel.tsx | 159 +++++- .../pullRequest/PullRequestGuideTab.tsx | 183 +++++++ .../PullRequestReviewAnnotation.tsx | 24 + .../pullRequest/guide/GuideExcerpt.tsx | 362 +++++++++++++ .../pullRequest/guide/GuideFileDiff.tsx | 50 ++ .../pullRequest/guide/GuideNotes.tsx | 58 ++ .../pullRequest/guide/GuideProse.tsx | 40 ++ .../pullRequest/guide/GuideRail.tsx | 108 ++++ .../pullRequest/guide/GuideStop.tsx | 310 +++++++++++ .../pullRequest/guide/GuideTour.tsx | 174 ++++++ .../guide/PullRequestGuideRunCoordinator.tsx | 140 +++++ .../pullRequest/guide/guideRunStore.ts | 86 +++ .../pullRequest/guide/guideTour.logic.test.ts | 178 ++++++ .../pullRequest/guide/guideTour.logic.ts | 224 ++++++++ .../pullRequest/guide/useGuideDiffFiles.ts | 87 +++ .../pullRequest/guide/useReviewGuideWriter.ts | 78 +++ .../pullRequestDetail.logic.test.ts | 49 ++ .../pullRequest/pullRequestDetail.logic.ts | 64 +++ .../settings/SettingInheritance.tsx | 8 +- .../SourceControlWritingSettings.test.tsx | 5 +- .../settings/SourceControlWritingSettings.tsx | 167 +++++- .../settings/settingsSearch.test.ts | 2 + .../src/components/settings/settingsSearch.ts | 26 + apps/web/src/routes/__root.tsx | 2 + apps/web/src/routes/_chat.pull-requests.tsx | 42 +- docs/user/source-control.md | 28 + .../src/state/pullRequestRouting.ts | 2 + .../client-runtime/src/state/pullRequests.ts | 18 + .../contracts/src/clientRpcPermissions.ts | 2 + packages/contracts/src/index.ts | 1 + .../contracts/src/pullRequestReviewGuide.ts | 115 ++++ packages/contracts/src/rpc.ts | 17 + packages/contracts/src/settings.ts | 26 + .../src/server/driver.ts | 1 + .../src/server/textGeneration.ts | 46 ++ .../src/server/textGenerationOperations.ts | 42 ++ .../src/server/textGenerationPrompts.test.ts | 55 ++ .../src/server/textGenerationPrompts.ts | 149 ++++++ .../src/server/textGeneration.ts | 5 +- .../src/server/textGeneration.ts | 5 +- .../src/server/textGeneration.ts | 5 + .../provider-opencode/src/server/driver.ts | 5 + .../src/server/textGeneration.ts | 6 +- .../src/server/v2/textGeneration.ts | 5 +- .../provider-pi/src/server/textGeneration.ts | 5 +- packages/shared/src/serverSettings.ts | 26 + 74 files changed, 4913 insertions(+), 134 deletions(-) create mode 100644 apps/server/src/persistence/Migrations/061_PullRequestReviewGuides.ts create mode 100644 apps/server/src/persistence/PullRequestReviewGuides.ts create mode 100644 apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts create mode 100644 apps/server/src/pullRequest/PullRequestReviewGuideService.ts create mode 100644 apps/server/src/pullRequest/pullRequestReviewGuideInput.test.ts create mode 100644 apps/server/src/pullRequest/pullRequestReviewGuideInput.ts create mode 100644 apps/server/src/pullRequest/pullRequestReviewGuideNormalize.test.ts create mode 100644 apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts create mode 100644 apps/web/src/components/pullRequest/PullRequestGuideTab.tsx create mode 100644 apps/web/src/components/pullRequest/guide/GuideExcerpt.tsx create mode 100644 apps/web/src/components/pullRequest/guide/GuideFileDiff.tsx create mode 100644 apps/web/src/components/pullRequest/guide/GuideNotes.tsx create mode 100644 apps/web/src/components/pullRequest/guide/GuideProse.tsx create mode 100644 apps/web/src/components/pullRequest/guide/GuideRail.tsx create mode 100644 apps/web/src/components/pullRequest/guide/GuideStop.tsx create mode 100644 apps/web/src/components/pullRequest/guide/GuideTour.tsx create mode 100644 apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx create mode 100644 apps/web/src/components/pullRequest/guide/guideRunStore.ts create mode 100644 apps/web/src/components/pullRequest/guide/guideTour.logic.test.ts create mode 100644 apps/web/src/components/pullRequest/guide/guideTour.logic.ts create mode 100644 apps/web/src/components/pullRequest/guide/useGuideDiffFiles.ts create mode 100644 apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts create mode 100644 packages/contracts/src/pullRequestReviewGuide.ts diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 7d67d92f4f713a09f1aef2f04abbebf7bea2a352..5b8d483dc1024d0b1b8378d88009fedaed88e8db 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -132,6 +132,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.pullRequestsPreview]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsChecks]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsActivity]: AuthOrchestrationReadScope, + [WS_METHODS.pullRequestsReviewGuide]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsThreadComments]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsFilesViewed]: AuthOrchestrationReadScope, diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index 53bb49c346e361a26f29bff6d88927128a32cbce..73d23c415c17059c0f5aba436f2e415103674f23 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -347,6 +347,7 @@ function createTextGeneration( Effect.succeed({ title: "Update workflow", }), + generateReviewGuide: () => Effect.succeed({ stops: [] }), ...overrides, }; @@ -395,6 +396,7 @@ function createTextGeneration( }), ), ), + generateReviewGuide: (input) => implementation.generateReviewGuide(input), }; } diff --git a/apps/server/src/observability/RpcInstrumentation.ts b/apps/server/src/observability/RpcInstrumentation.ts index 69b8b3d325ab771a8dba322e6570bbb7a7699d94..167ce8d11fff99b16b4bb9cc81f50f79a627b1fb 100644 --- a/apps/server/src/observability/RpcInstrumentation.ts +++ b/apps/server/src/observability/RpcInstrumentation.ts @@ -107,6 +107,8 @@ const RPC_AGGREGATES = { [WS_METHODS.pullRequestsPreview]: "pull-requests", [WS_METHODS.pullRequestsChecks]: "pull-requests", [WS_METHODS.pullRequestsActivity]: "pull-requests", + [WS_METHODS.pullRequestsReviewGuide]: "pull-requests", + [WS_METHODS.pullRequestsGenerateReviewGuide]: "pull-requests", [WS_METHODS.pullRequestsThreadComments]: "pull-requests", [WS_METHODS.pullRequestsDiffFileContents]: "pull-requests", [WS_METHODS.pullRequestsFilesViewed]: "pull-requests", diff --git a/apps/server/src/persistence/Errors.ts b/apps/server/src/persistence/Errors.ts index 67290369b0f42ff2d4cc27ca631405acb84bf8a2..cf9d743ea45df870ccaf1fba50cf629b68342385 100644 --- a/apps/server/src/persistence/Errors.ts +++ b/apps/server/src/persistence/Errors.ts @@ -129,5 +129,6 @@ export type ProviderSessionRuntimeRepositoryError = PersistenceSqlError | Persis export type AuthPairingLinkRepositoryError = PersistenceSqlError | PersistenceDecodeError; export type AuthSessionRepositoryError = PersistenceSqlError | PersistenceDecodeError; export type PullRequestFilesViewedRepositoryError = PersistenceSqlError | PersistenceDecodeError; +export type PullRequestReviewGuideRepositoryError = PersistenceSqlError | PersistenceDecodeError; export type ProjectionRepositoryError = PersistenceSqlError | PersistenceDecodeError; diff --git a/apps/server/src/persistence/Migrations.ts b/apps/server/src/persistence/Migrations.ts index 35c1a3fdce4bb58ee773a6eac4f4fef1a589a04a..7990c53ac454b4e10ec7cb43267424761200e718 100644 --- a/apps/server/src/persistence/Migrations.ts +++ b/apps/server/src/persistence/Migrations.ts @@ -74,6 +74,7 @@ import Migration0057 from "./Migrations/057_ScheduledTaskWebhooks.ts"; import Migration0058 from "./Migrations/058_WebhookRelayDeliveries.ts"; import Migration0059 from "./Migrations/059_McpAppModelContext.ts"; import Migration0060 from "./Migrations/060_ThreadSnapshotWindowIndexes.ts"; +import Migration0061 from "./Migrations/061_PullRequestReviewGuides.ts"; /** * Migration loader with all migrations defined inline. @@ -148,6 +149,7 @@ export const migrationEntries = [ [58, "WebhookRelayDeliveries", Migration0058], [59, "McpAppModelContext", Migration0059], [60, "ThreadSnapshotWindowIndexes", Migration0060], + [61, "PullRequestReviewGuides", Migration0061], ] as const; export const migrationManifest = migrationEntries.map(([id, name]) => [id, name] as const); diff --git a/apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts b/apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts index 5e45ed05d30241e9d53a244a32610351b731875d..7d5c845cbf22aa8605cd24c96f8d38057af6dddc 100644 --- a/apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts +++ b/apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts @@ -13,7 +13,7 @@ layer("055_OrchestrationV2", (it) => { Effect.sync(() => { assert.deepStrictEqual( migrationEntries.map(([id]) => id), - Array.from({ length: 60 }, (_, index) => index + 1), + Array.from({ length: 61 }, (_, index) => index + 1), ); }), ); @@ -32,6 +32,7 @@ layer("055_OrchestrationV2", (it) => { [58, "WebhookRelayDeliveries"], [59, "McpAppModelContext"], [60, "ThreadSnapshotWindowIndexes"], + [61, "PullRequestReviewGuides"], ]); assert.deepStrictEqual(yield* runMigrations(), []); @@ -58,6 +59,7 @@ layer("055_OrchestrationV2", (it) => { { migration_id: 58, name: "WebhookRelayDeliveries" }, { migration_id: 59, name: "McpAppModelContext" }, { migration_id: 60, name: "ThreadSnapshotWindowIndexes" }, + { migration_id: 61, name: "PullRequestReviewGuides" }, ]); const tables = yield* sql<{ readonly name: string }>` diff --git a/apps/server/src/persistence/Migrations/061_PullRequestReviewGuides.ts b/apps/server/src/persistence/Migrations/061_PullRequestReviewGuides.ts new file mode 100644 index 0000000000000000000000000000000000000000..bf8476670204f76bd47d2f301ebf1a7c043f0f88 --- /dev/null +++ b/apps/server/src/persistence/Migrations/061_PullRequestReviewGuides.ts @@ -0,0 +1,19 @@ +import * as Effect from "effect/Effect"; +import * as SqlClient from "effect/sql/SqlClient"; + +export default Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + + // The latest guide per pull request, keyed by its host-level identity so every project that + // checks out the same repository shares one. Guides cost a model run, so a push leaves the old + // one in place for the page to call stale rather than deleting it. + yield* sql` + CREATE TABLE IF NOT EXISTS pull_request_review_guides ( + host TEXT NOT NULL, + repository TEXT NOT NULL, + number INTEGER NOT NULL, + guide_json TEXT NOT NULL, + PRIMARY KEY (host, repository, number) + ) WITHOUT ROWID + `; +}); diff --git a/apps/server/src/persistence/PullRequestReviewGuides.ts b/apps/server/src/persistence/PullRequestReviewGuides.ts new file mode 100644 index 0000000000000000000000000000000000000000..dd8cd0708046dc0af2dd7c90e8b4e040d9fd10df --- /dev/null +++ b/apps/server/src/persistence/PullRequestReviewGuides.ts @@ -0,0 +1,84 @@ +import * as Context from "effect/Context"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Option from "effect/Option"; +import * as Schema from "effect/Schema"; +import * as SqlClient from "effect/sql/SqlClient"; +import * as SqlSchema from "effect/sql/SqlSchema"; + +import { PullRequestReviewGuide, ThreadPullRequestKey } from "@t3tools/contracts"; + +import { + PersistenceDecodeError, + PersistenceSqlError, + type PullRequestReviewGuideRepositoryError, +} from "./Errors.ts"; + +const GuideRow = Schema.Struct({ + guide: Schema.fromJsonString(PullRequestReviewGuide), +}); + +/** The latest review guide per pull request, keyed by host-level identity. */ +export class PullRequestReviewGuideRepository extends Context.Service< + PullRequestReviewGuideRepository, + { + readonly get: ( + key: ThreadPullRequestKey, + ) => Effect.Effect< + Option.Option, + PullRequestReviewGuideRepositoryError + >; + readonly put: ( + key: ThreadPullRequestKey, + guide: PullRequestReviewGuide, + ) => Effect.Effect; + } +>()("t3/persistence/PullRequestReviewGuides/PullRequestReviewGuideRepository") {} + +const make = Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + + const findGuide = SqlSchema.findOneOption({ + Request: ThreadPullRequestKey, + Result: GuideRow, + execute: ({ host, repository, number }) => + sql` + SELECT guide_json AS "guide" + FROM pull_request_review_guides + WHERE host = ${host} AND repository = ${repository} AND number = ${number} + `, + }); + const encodeGuide = Schema.encodeEffect(Schema.fromJsonString(PullRequestReviewGuide)); + + return PullRequestReviewGuideRepository.of({ + get: (key) => + findGuide(key).pipe( + Effect.map(Option.map((row) => row.guide)), + Effect.mapError((cause) => + Schema.isSchemaError(cause) + ? PersistenceDecodeError.fromSchemaError("PullRequestReviewGuide", cause) + : new PersistenceSqlError({ operation: "getPullRequestReviewGuide", cause }), + ), + ), + put: (key, guide) => + encodeGuide(guide).pipe( + Effect.mapError((cause) => + PersistenceDecodeError.fromSchemaError("PullRequestReviewGuide", cause), + ), + Effect.flatMap((guideJson) => + sql` + INSERT INTO pull_request_review_guides (host, repository, number, guide_json) + VALUES (${key.host}, ${key.repository}, ${key.number}, ${guideJson}) + ON CONFLICT (host, repository, number) DO UPDATE SET guide_json = excluded.guide_json + `.pipe( + Effect.mapError( + (cause) => new PersistenceSqlError({ operation: "putPullRequestReviewGuide", cause }), + ), + ), + ), + Effect.asVoid, + ), + }); +}); + +export const layer = Layer.effect(PullRequestReviewGuideRepository, make); diff --git a/apps/server/src/persistence/reconcileV2PreviewMigration.test.ts b/apps/server/src/persistence/reconcileV2PreviewMigration.test.ts index 675d5f5db0215842459e835ecf0a736a6274e877..5881fdfda408b6c993c18fea6ac2f7fcebd7bf84 100644 --- a/apps/server/src/persistence/reconcileV2PreviewMigration.test.ts +++ b/apps/server/src/persistence/reconcileV2PreviewMigration.test.ts @@ -41,6 +41,7 @@ describe("V2 preview upgrade", () => { [58, "WebhookRelayDeliveries"], [59, "McpAppModelContext"], [60, "ThreadSnapshotWindowIndexes"], + [61, "PullRequestReviewGuides"], ]); assert.deepStrictEqual(yield* runMigrations(), []); assert.deepStrictEqual(yield* sql`SELECT * FROM orchestration_v2_legacy_imports`, imports); @@ -124,6 +125,7 @@ describe("V2 preview upgrade", () => { [58, "WebhookRelayDeliveries"], [59, "McpAppModelContext"], [60, "ThreadSnapshotWindowIndexes"], + [61, "PullRequestReviewGuides"], ]); }).pipe(Effect.provide(NodeSqliteClient.layer({ filename: ":memory:" }))), ); diff --git a/apps/server/src/provider/Drivers/CodexManagedProvider.ts b/apps/server/src/provider/Drivers/CodexManagedProvider.ts index 92c461390aea134bb0e469b6f6ff2832a072dae3..d7b20bf2e00707a110ee5a29268006da685de332 100644 --- a/apps/server/src/provider/Drivers/CodexManagedProvider.ts +++ b/apps/server/src/provider/Drivers/CodexManagedProvider.ts @@ -260,6 +260,8 @@ export const makeManagedCodexProvider = Effect.fn("makeManagedCodexProvider")(fu protect("generateBranchName", nativeGeneration.generateBranchName(value)), generateThreadTitle: (value) => protect("generateThreadTitle", nativeGeneration.generateThreadTitle(value)), + generateReviewGuide: (value) => + protect("generateReviewGuide", nativeGeneration.generateReviewGuide(value)), }; return { instanceId, diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..aa41705db29d8237491ec6b0f9918d8f8b7d62c3 --- /dev/null +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts @@ -0,0 +1,505 @@ +import * as NodeServices from "@effect/platform-node/NodeServices"; +import { assert, it } from "@effect/vitest"; +import * as Context from "effect/Context"; +import * as Deferred from "effect/Deferred"; +import * as Effect from "effect/Effect"; +import * as FileSystem from "effect/FileSystem"; +import * as Layer from "effect/Layer"; +import * as Ref from "effect/Ref"; +import * as Schedule from "effect/Schedule"; +import { + ProjectId, + type PullRequestActivity, + type PullRequestDetail, + type PullRequestStack, + type PullRequestRef, +} from "@t3tools/contracts"; +import type { ReviewGuideGenerationInput } from "@t3tools/provider-core/server/textGeneration"; + +import * as ServerConfig from "../config.ts"; +import * as SqlitePersistence from "../persistence/Sqlite.ts"; +import * as PullRequestReviewGuides from "../persistence/PullRequestReviewGuides.ts"; +import * as ProjectService from "../project/ProjectService.ts"; +import * as ProviderRegistry from "../provider/ProviderRegistry.ts"; +import * as ServerSettings from "../serverSettings.ts"; +import * as TextGeneration from "../textGeneration/TextGeneration.ts"; +import * as GitVcsDriver from "../vcs/GitVcsDriver.ts"; +import * as VcsProcess from "../vcs/VcsProcess.ts"; +import * as PullRequestReviewGuideService from "./PullRequestReviewGuideService.ts"; +import * as PullRequestService from "./PullRequestService.ts"; + +const projectId = ProjectId.make("project-1"); +/** The test project is a checkout of the repository its pull requests come from. */ +const ACME_APP = { + canonicalKey: "github.com/acme/app", + locator: { + source: "git-remote" as const, + remoteName: "origin", + remoteUrl: "https://github.com/acme/app.git", + }, + provider: "github", + displayName: "acme/app", + owner: "acme", + name: "app", +}; + +const reference: PullRequestRef = { + projectId, + host: "github.com", + repository: "acme/app", + number: 7, +}; + +const git = (cwd: string, args: ReadonlyArray) => + Effect.gen(function* () { + const driver = yield* GitVcsDriver.GitVcsDriver; + const result = yield* driver.execute({ operation: "test.git", cwd, args, timeoutMs: 10_000 }); + return result.stdout.trim(); + }); + +/** + * A remote whose `main` has moved on past where the change's base branch left it, with the + * change stacked on that base branch. A guide for the change must see only its own commit. + */ +const makeRepositories = Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const root = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-guide-" }); + const remote = `${root}/remote`; + const workspace = `${root}/workspace`; + yield* fs.makeDirectory(remote); + yield* git(remote, ["init", "--quiet", "--initial-branch=main"]); + yield* git(remote, ["config", "user.email", "test@example.com"]); + yield* git(remote, ["config", "user.name", "Test"]); + const commit = (path: string, message: string, contents = message) => + fs + .writeFileString(`${remote}/${path}`, contents) + .pipe( + Effect.andThen(git(remote, ["add", path])), + Effect.andThen(git(remote, ["commit", "--quiet", "-m", message])), + Effect.andThen(git(remote, ["rev-parse", "HEAD"])), + ); + yield* commit("base.txt", "base"); + yield* git(remote, ["checkout", "--quiet", "-b", "stack-base"]); + const lowerSha = yield* commit("lower.txt", "lower layer"); + yield* git(remote, ["checkout", "--quiet", "-b", "feature"]); + // Ten lines, so an excerpt reaching past them has something to be clamped to. + const headSha = yield* commit( + "feature.txt", + "the change", + Array.from({ length: 10 }, (_, index) => `line ${index + 1}`).join("\n") + "\n", + ); + // A layer above the change, which uses what the change adds. + yield* git(remote, ["checkout", "--quiet", "-b", "upper"]); + const upperSha = yield* commit("upper.txt", "upper layer"); + yield* git(remote, ["checkout", "--quiet", "main"]); + yield* commit("unrelated.txt", "main moved on"); + yield* git(root, ["clone", "--quiet", remote, workspace]); + return { root, workspace, headSha, lowerSha, upperSha }; +}); + +const detail = (workspaceRoot: string, headSha: string): PullRequestDetail => + ({ + number: 7, + title: "Add the feature", + body: "Adds the feature.", + baseBranch: "stack-base", + headBranch: "feature", + headSha, + workspaceRoot, + }) as PullRequestDetail; + +const activity: PullRequestActivity = { + comments: [], + commentCount: 0, + commentsTruncated: false, + reviewThreads: [], + commits: [], +}; + +/** The change as the middle layer of three; the top layer's head is not on any fetched branch. */ +const stack = (repositories: { + readonly headSha: string; + readonly lowerSha: string; + readonly upperSha: string; +}): PullRequestStack => ({ + id: "stack", + number: 1, + url: "https://example.com/stack/1", + base: "main", + layers: [ + { + number: 6, + title: "Lower", + headSha: repositories.lowerSha, + headBranch: "stack-base", + state: "open", + }, + { + number: 7, + title: "Add the feature", + headSha: repositories.headSha, + headBranch: "feature", + state: "open", + }, + { + number: 8, + title: "Upper", + headSha: repositories.upperSha, + headBranch: "upper", + state: "open", + }, + { number: 9, title: "Unpushed", headSha: "f".repeat(40), headBranch: "gone", state: "open" }, + ], +}); + +const makeLayer = (input: { + readonly root: string; + readonly workspace: string; + readonly headSha: string; + readonly lowerSha: string; + readonly upperSha: string; + readonly generate: TextGeneration.TextGeneration["Service"]["generateReviewGuide"]; + /** Completed when a run stores its guide, which is the last thing a run does but forget it. */ + readonly stored: Deferred.Deferred; + /** The server's beta switch; on unless a test is about it being off. */ + readonly enabled?: boolean; + /** The head the host reports now, for a test that pushes between runs. */ + readonly currentHead?: Ref.Ref; + /** The project's own repository; defaults to the one the test reference names. */ + readonly identity?: typeof ACME_APP; +}) => + PullRequestReviewGuideService.layer.pipe( + Layer.provide( + Layer.effect( + PullRequestReviewGuides.PullRequestReviewGuideRepository, + Effect.gen(function* () { + const repository = yield* PullRequestReviewGuides.PullRequestReviewGuideRepository; + return { + get: repository.get, + put: (key, guide) => + repository + .put(key, guide) + .pipe(Effect.tap(() => Deferred.succeed(input.stored, undefined))), + } satisfies PullRequestReviewGuides.PullRequestReviewGuideRepository["Service"]; + }), + ).pipe(Layer.provide(PullRequestReviewGuides.layer)), + ), + Layer.provide( + Layer.succeed(PullRequestService.PullRequestService, { + detail: () => + (input.currentHead === undefined + ? Effect.succeed(input.headSha) + : Ref.get(input.currentHead) + ).pipe(Effect.map((head) => detail(input.workspace, head))), + activity: () => Effect.succeed(activity), + invalidate: () => Effect.void, + stack: () => Effect.succeed(stack(input)), + } as unknown as PullRequestService.PullRequestService["Service"]), + ), + Layer.provide( + Layer.succeed(ProjectService.ProjectService, { + getShell: () => + Effect.succeedSome({ + id: projectId, + workspaceRoot: input.workspace, + repositoryIdentity: input.identity ?? ACME_APP, + }), + } as unknown as ProjectService.ProjectService["Service"]), + ), + Layer.provide( + Layer.succeed(ProviderRegistry.ProviderRegistry, { + getProviders: Effect.succeed([]), + } as unknown as ProviderRegistry.ProviderRegistry["Service"]), + ), + Layer.provide( + Layer.succeed(TextGeneration.TextGeneration, { + generateReviewGuide: input.generate, + } as unknown as TextGeneration.TextGeneration["Service"]), + ), + Layer.provide(ServerSettings.layerTest({ reviewGuidesEnabled: input.enabled ?? true })), + Layer.provide(ServerConfig.layerTest(input.root, { prefix: "t3-review-guide-config-" })), + ); + +const layerBase = Layer.mergeAll( + GitVcsDriver.layer.pipe( + Layer.provide(ServerConfig.layerTest(process.cwd(), { prefix: "t3-review-guide-git-" })), + Layer.provideMerge(VcsProcess.layer), + ), + SqlitePersistence.layerMemory, +).pipe(Layer.provideMerge(NodeServices.layer)); + +// Runs drive real git, which the test clock does not advance, so waits use real time. +it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideService", (it) => { + it.effect("briefs the change against its own base in a checkout it then removes", () => + Effect.scoped( + Effect.gen(function* () { + const repositories = yield* makeRepositories; + const seen = yield* Deferred.make<{ + readonly input: ReviewGuideGenerationInput; + readonly changed: string; + readonly head: string; + readonly upper: string; + }>(); + const finish = yield* Deferred.make(); + // The fake model reads the checkout with the same driver, captured so it needs nothing. + const driver = yield* GitVcsDriver.GitVcsDriver; + const readCheckout = (cwd: string, args: ReadonlyArray) => + git(cwd, args).pipe( + Effect.provideService(GitVcsDriver.GitVcsDriver, driver), + Effect.orDie, + ); + const stored = yield* Deferred.make(); + + // Built in this test's scope: runs are forked into the service's scope and must outlive + // the call that obtains the service. + const context = yield* Layer.build( + makeLayer({ + ...repositories, + stored, + generate: (input) => + Effect.gen(function* () { + const changed = yield* readCheckout(input.cwd, [ + "diff", + "--name-only", + `${input.baseCommit}...HEAD`, + ]); + const head = yield* readCheckout(input.cwd, ["rev-parse", "HEAD"]); + // Another layer's head is readable from the checkout, to check a seam. + const upper = yield* readCheckout(input.cwd, [ + "show", + `${repositories.upperSha}:upper.txt`, + ]); + yield* Deferred.succeed(seen, { input, changed, head, upper }); + yield* Deferred.await(finish); + // Written the way a model gets it wrong: an excerpt past the end of the file, a + // note outside what is left of it, and a changed file left off the tour. + return { + stops: [ + { + title: "The feature", + summary: "The change.", + attention: null, + excerpts: [ + { path: "feature.txt", side: "new", start: 4, end: 40, caption: "" }, + ], + files: [], + notes: [ + { path: "feature.txt", line: 5, title: "Kept", body: "Inside." }, + { path: "feature.txt", line: 30, title: "Dropped", body: "Outside." }, + ], + questions: [], + }, + ], + }; + }), + }), + ); + const service = Context.get( + context, + PullRequestReviewGuideService.PullRequestReviewGuideService, + ); + + // Generate returns while the run is still reading the change. + const started = yield* service.generate(reference); + assert.strictEqual(started.run?._tag, "Running"); + assert.strictEqual(started.guide, null); + + // A run that fails before reaching the model reports why rather than leaving this waiting. + const failedRun = service.get(reference).pipe( + Effect.flatMap((result) => + result.run?._tag === "Failed" + ? Effect.die(`run failed: ${result.run.detail}`) + : Effect.fail("still running" as const), + ), + Effect.retry(Schedule.spaced("20 millis")), + ); + const observed = yield* Effect.raceFirst(Deferred.await(seen), failedRun); + assert.strictEqual(observed.head, repositories.headSha); + assert.strictEqual(observed.changed, "feature.txt"); + assert.strictEqual(observed.input.baseBranch, "stack-base"); + // The stack comes along, bottom first; a layer whose head could not be fetched is named + // without one. + assert.deepStrictEqual(observed.input.stack, [ + { number: 6, title: "Lower", state: "open", headSha: repositories.lowerSha }, + { number: 7, title: "Add the feature", state: "open", headSha: repositories.headSha }, + { number: 8, title: "Upper", state: "open", headSha: repositories.upperSha }, + { number: 9, title: "Unpushed", state: "open", headSha: null }, + ]); + assert.strictEqual(observed.upper, "upper layer"); + + // A second press while running joins the run instead of starting another. + const joined = yield* service.generate(reference); + assert.strictEqual(joined.run?._tag, "Running"); + + yield* Deferred.succeed(finish, undefined); + yield* Deferred.await(stored); + const finished = yield* service.get(reference); + assert.strictEqual(finished.guide?.headSha, repositories.headSha); + const guide = finished.guide; + assert.isNotNull(guide); + const [story] = guide.stops; + assert.deepStrictEqual(story?.excerpts, [ + { path: "feature.txt", side: "new", start: 4, end: 10, caption: "" }, + ]); + assert.deepStrictEqual( + story?.notes.map((note) => note.title), + ["Kept"], + ); + // lower.txt is below the change's base, so it is neither in the change nor on the tour. + assert.strictEqual(guide.stops.length, 1); + + const fs = yield* FileSystem.FileSystem; + assert.isFalse(yield* fs.exists(observed.input.cwd)); + const worktrees = yield* git(repositories.workspace, ["worktree", "list", "--porcelain"]); + assert.notInclude(worktrees, observed.input.cwd); + }), + ), + ); + + it.effect("refuses to write a guide while the server has review guides off", () => + Effect.scoped( + Effect.gen(function* () { + const repositories = yield* makeRepositories; + const stored = yield* Deferred.make(); + const context = yield* Layer.build( + makeLayer({ + ...repositories, + stored, + enabled: false, + generate: () => Effect.die("a guide must not be written while the beta is off"), + }), + ); + const service = Context.get( + context, + PullRequestReviewGuideService.PullRequestReviewGuideService, + ); + const result = yield* Effect.flip(service.generate(reference)); + assert.include(result.message, "Review guides are off"); + }), + ), + ); + + it.effect( + "keeps a newer run's guide when an older run for the same pull request finishes late", + () => + Effect.scoped( + Effect.gen(function* () { + const repositories = yield* makeRepositories; + const stored = yield* Deferred.make(); + const currentHead = yield* Ref.make(repositories.headSha); + // The old run waits on this; the new one answers at once. + const releaseOld = yield* Deferred.make(); + const oldStarted = yield* Deferred.make(); + const answer = (title: string) => ({ + stops: [ + { + title, + summary: "", + attention: null, + excerpts: [], + files: [], + notes: [], + questions: [], + }, + ], + }); + const context = yield* Layer.build( + makeLayer({ + ...repositories, + stored, + currentHead, + generate: (input) => + input.cwd.length > 0 && input.baseCommit.length > 0 + ? Ref.get(currentHead).pipe( + Effect.flatMap((head) => + head === repositories.headSha + ? Deferred.succeed(oldStarted, undefined).pipe( + Effect.andThen(Deferred.await(releaseOld)), + Effect.as(answer("old head")), + ) + : Effect.succeed(answer("new head")), + ), + ) + : Effect.die("unreachable"), + }), + ); + const service = Context.get( + context, + PullRequestReviewGuideService.PullRequestReviewGuideService, + ); + + yield* service.generate(reference); + const firstFailed = service.get(reference).pipe( + Effect.flatMap((result) => + result.run?._tag === "Failed" + ? Effect.die(`first run failed: ${result.run.detail}`) + : Effect.fail("still running" as const), + ), + Effect.retry(Schedule.spaced("20 millis")), + ); + yield* Effect.raceFirst(Deferred.await(oldStarted), firstFailed); + // A push: the host reports the upper layer's commit as the head now, and a new run starts. + yield* Ref.set(currentHead, repositories.upperSha); + const second = yield* service.generate(reference); + assert.strictEqual(second.run?._tag, "Running"); + assert.strictEqual(second.run?.headSha, repositories.upperSha); + // A run that fails reports why rather than leaving this waiting. + const failedRun = service.get(reference).pipe( + Effect.flatMap((result) => + result.run?._tag === "Failed" + ? Effect.die(`run failed: ${result.run.detail}`) + : Effect.fail("still running" as const), + ), + Effect.retry(Schedule.spaced("20 millis")), + ); + yield* Effect.raceFirst(Deferred.await(stored), failedRun); + + // The old run finishes last. Its guide is for a head nobody is looking at any more. + yield* Deferred.succeed(releaseOld, undefined); + const settled = yield* service.get(reference).pipe( + Effect.filterOrFail( + (result) => result.run === null, + () => "still running" as const, + ), + Effect.retry({ schedule: Schedule.spaced("20 millis"), times: 250 }), + ); + assert.strictEqual(settled.guide?.headSha, repositories.upperSha); + // Give the late run every chance to write over the newer guide; it must not. + yield* Effect.sleep("200 millis"); + const after = yield* service.get(reference); + assert.isNull(after.run); + assert.strictEqual(after.guide?.headSha, repositories.upperSha); + assert.strictEqual(after.guide?.stops[0]?.title, "new head"); + }), + ), + ); + + it.effect("refuses a pull request from a repository the project is not a checkout of", () => + Effect.scoped( + Effect.gen(function* () { + const repositories = yield* makeRepositories; + const stored = yield* Deferred.make(); + const context = yield* Layer.build( + makeLayer({ + ...repositories, + stored, + identity: { + ...ACME_APP, + canonicalKey: "github.com/acme/other", + displayName: "acme/other", + name: "other", + }, + generate: () => + Effect.die("a guide must not be written from another repository's checkout"), + }), + ); + const service = Context.get( + context, + PullRequestReviewGuideService.PullRequestReviewGuideService, + ); + const result = yield* Effect.flip(service.generate(reference)); + assert.include(result.message, "in another repository"); + }), + ), + ); +}); diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts new file mode 100644 index 0000000000000000000000000000000000000000..b181926f4e8d375844129119652a57007f3fb9b5 --- /dev/null +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts @@ -0,0 +1,484 @@ +/** + * Guided reviews: a maintainer-facing brief on a pull request, written by the environment's + * source-control writer model. See `PullRequestReviewGuide` in contracts for what a guide is for. + * + * The model reads the change itself, read-only, from a disposable checkout of the head, so a + * guide has no size limit. That takes minutes on a large change, so a run belongs to the server + * rather than to the request that started it: Generate returns as soon as the run is under way + * and readers watch its state through `get`. + * + * Generation is explicit and the result is stored per pull request, so opening a guide never + * spends a model run. A push leaves the stored guide in place and the reader sees it as stale. + * + * @module pullRequest/PullRequestReviewGuideService + */ +import * as Cause from "effect/Cause"; +import * as Clock from "effect/Clock"; +import * as Context from "effect/Context"; +import * as DateTime from "effect/DateTime"; +import * as Effect from "effect/Effect"; +import * as FileSystem from "effect/FileSystem"; +import * as Layer from "effect/Layer"; +import * as Option from "effect/Option"; +import * as Ref from "effect/Ref"; +import * as Semaphore from "effect/Semaphore"; + +import { + PullRequestOperationError, + type PullRequestDetail, + type PullRequestRef, + type PullRequestReviewGuide, + type PullRequestReviewGuideResult, + type PullRequestReviewGuideRun, + type PullRequestStack, + ProjectId, + type RepositoryIdentity, + type ThreadPullRequestKey, +} from "@t3tools/contracts"; +import type { ReviewGuideStackLayer } from "@t3tools/provider-core/server/textGeneration"; +import { resolveProjectSettings } from "@t3tools/shared/projectSettings"; +import { sourceControlRepositorySelector } from "@t3tools/shared/sourceControl"; + +import * as ServerConfig from "../config.ts"; +import * as PullRequestReviewGuides from "../persistence/PullRequestReviewGuides.ts"; +import * as ProjectService from "../project/ProjectService.ts"; +import * as ProviderRegistry from "../provider/ProviderRegistry.ts"; +import * as ServerSettings from "../serverSettings.ts"; +import * as TextGeneration from "../textGeneration/TextGeneration.ts"; +import * as GitVcsDriver from "../vcs/GitVcsDriver.ts"; +import { PATCH_RENDER_PREFIX_ARGS } from "../vcs/GitVcsDriverCore.ts"; +import * as PullRequestService from "./PullRequestService.ts"; +import { pullRequestSyncKey } from "./pullRequestSyncKey.ts"; +import { reviewGuideInputFromPullRequest } from "./pullRequestReviewGuideInput.ts"; +import { normalizeReviewGuide, parseReviewGuideChange } from "./pullRequestReviewGuideNormalize.ts"; + +export class PullRequestReviewGuideService extends Context.Service< + PullRequestReviewGuideService, + { + /** The stored guide, any run in progress, and the head now. Never runs a model. */ + readonly get: ( + input: PullRequestRef, + ) => Effect.Effect; + /** + * Start writing a guide for the current head and return once it is under way. Joins a run + * already going for that head. A stored guide stays readable until the new one replaces it, + * so a reader can rewrite one after changing the guide model or instructions. + */ + readonly generate: ( + input: PullRequestRef, + ) => Effect.Effect; + } +>()("t3/pullRequest/PullRequestReviewGuideService") {} + +const keyString = (key: ThreadPullRequestKey) => `${key.host}/${key.repository}#${key.number}`; + +const nowIso = Clock.currentTimeMillis.pipe( + Effect.map((millis) => DateTime.formatIso(DateTime.makeUnsafe(millis))), +); + +const make = Effect.gen(function* () { + const pullRequests = yield* PullRequestService.PullRequestService; + const guides = yield* PullRequestReviewGuides.PullRequestReviewGuideRepository; + const projects = yield* ProjectService.ProjectService; + const serverSettings = yield* ServerSettings.ServerSettingsService; + const providerRegistry = yield* ProviderRegistry.ProviderRegistry; + const textGeneration = yield* TextGeneration.TextGeneration; + const git = yield* GitVcsDriver.GitVcsDriver; + const fileSystem = yield* FileSystem.FileSystem; + const config = yield* ServerConfig.ServerConfig; + // Runs outlive the request that started them and end with the server. + const serviceScope = yield* Effect.scope; + // In memory on purpose: a restart drops a run in progress, and a reader presses Generate again. + const runs = yield* Ref.make>(new Map()); + // Names each run's private git refs; unique for the server's lifetime, like the runs map. + let runCounter = 0; + // Claiming a run and storing a finished guide take turns, so a run that still owns its pull + // request when it checks is the one whose guide is stored: a newer claim cannot land between. + const ownership = yield* Semaphore.make(1); + + const keyFor = Effect.fn("PullRequestReviewGuideService.keyFor")(function* ( + input: PullRequestRef, + ) { + const project = Option.getOrUndefined( + yield* projects.getShell(input.projectId).pipe(Effect.orElseSucceed(() => Option.none())), + ); + const key = pullRequestSyncKey(input, project?.repositoryIdentity ?? undefined); + if (key === null || project === undefined) { + return yield* new PullRequestOperationError({ + operation: "reviewGuide", + detail: "Could not identify this pull request's host.", + }); + } + return { key, workspaceRoot: project.workspaceRoot, identity: project.repositoryIdentity }; + }); + + /** + * A guide reads the change in a checkout fetched from the project's own remote, so the pull + * request has to be in that repository. One from another repository is refused rather than + * measured against commits that are not its own. + */ + const requireOwnRepository = Effect.fn("PullRequestReviewGuideService.requireOwnRepository")( + function* (key: ThreadPullRequestKey, identity: RepositoryIdentity | null | undefined) { + const own = + identity == null + ? null + : pullRequestSyncKey( + { + projectId: ProjectId.make("own"), + number: key.number, + repository: sourceControlRepositorySelector(identity) ?? "", + }, + identity, + ); + if (own === null || own.host !== key.host || own.repository !== key.repository) { + return yield* new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: + "Guides are written from this project's checkout, and this pull request is in another repository. Write it from a project that is a checkout of that repository.", + }); + } + }, + ); + + const readStored = (key: ThreadPullRequestKey) => + guides.get(key).pipe( + Effect.mapError( + (cause) => + new PullRequestOperationError({ + operation: "reviewGuide", + detail: "Could not read the stored review guide.", + cause, + }), + ), + Effect.map(Option.getOrNull), + ); + + const get: PullRequestReviewGuideService["Service"]["get"] = Effect.fn( + "PullRequestReviewGuideService.get", + )(function* (input) { + const { key } = yield* keyFor(input); + const [guide, detail, current] = yield* Effect.all( + [readStored(key), pullRequests.detail(input), Ref.get(runs)], + { concurrency: "unbounded" }, + ); + return { + guide, + run: current.get(keyString(key)) ?? null, + currentHeadSha: detail.headSha ?? null, + }; + }); + + /** + * A detached checkout of the head beside the project, removed when the run ends. Fetched by + * commit rather than by branch so a fork's head and a branch checked out elsewhere both work, + * and so the checkout is exactly what the host reported. + */ + const checkoutHead = Effect.fn("PullRequestReviewGuideService.checkoutHead")(function* ( + workspaceRoot: string, + detail: PullRequestDetail, + headSha: string, + stack: PullRequestStack | null, + runId: string, + ) { + const remote = yield* git.resolvePrimaryRemoteName(workspaceRoot); + const fetch = (args: ReadonlyArray) => + git.execute({ + operation: "PullRequestReviewGuideService.fetch", + cwd: workspaceRoot, + args: ["fetch", "--quiet", "--no-tags", remote, ...args], + timeoutMs: 5 * 60_000, + }); + // The base branch is what the change is measured from; the head commit is the change. Hosts + // that do not serve an unadvertised commit still serve their pull refs, which GitHub and + // Forgejo name the same way. + // The host names the base branch. A name git rejects, or one that would change the refspec + // (a leading `+`, a `:`), is refused before it reaches `git fetch`. + yield* git + .execute({ + operation: "PullRequestReviewGuideService.checkBaseBranch", + cwd: workspaceRoot, + args: ["check-ref-format", "--branch", detail.baseBranch], + }) + .pipe( + Effect.filterOrFail( + () => !detail.baseBranch.startsWith("+") && !detail.baseBranch.includes(":"), + () => + new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: "The pull request's base branch name is not one git can fetch.", + }), + ), + ); + // Fetched into a ref of this run's own: FETCH_HEAD is shared by every fetch in the + // workspace, so a concurrent run could otherwise measure this change from its base. + const baseRef = `refs/t3/review-guide/${runId}/base`; + yield* fetch([`+refs/heads/${detail.baseBranch}:${baseRef}`]); + yield* Effect.addFinalizer(() => + git + .execute({ + operation: "PullRequestReviewGuideService.dropBaseRef", + cwd: workspaceRoot, + args: ["update-ref", "-d", baseRef], + }) + .pipe(Effect.ignore({ log: true })), + ); + const baseTip = yield* git.resolveCommit({ cwd: workspaceRoot, revision: baseRef }); + const fetchHead = (number: number, sha: string) => + fetch([sha]).pipe(Effect.catch(() => fetch([`refs/pull/${number}/head`]))); + yield* fetchHead(detail.number, headSha); + // The other layers of a stack are context the model may read across a seam. A layer that + // cannot be fetched is still named in the prompt, without a head to read. + const layers: ReadonlyArray = yield* Effect.forEach( + stack?.layers ?? [], + (layer) => { + const described = { number: layer.number, title: layer.title ?? null, state: layer.state }; + if (layer.number === detail.number) return Effect.succeed({ ...described, headSha }); + const sha = layer.headSha; + if (sha === undefined) return Effect.succeed({ ...described, headSha: null }); + return fetchHead(layer.number, sha).pipe( + Effect.andThen(git.resolveCommit({ cwd: workspaceRoot, revision: sha })), + Effect.as({ ...described, headSha: sha }), + Effect.orElseSucceed(() => ({ ...described, headSha: null })), + ); + }, + { concurrency: 4 }, + ); + const directory = yield* fileSystem.makeTempDirectoryScoped({ + directory: config.worktreesDir, + prefix: "review-guide-", + }); + const path = `${directory}/checkout`; + yield* git.execute({ + operation: "PullRequestReviewGuideService.addWorktree", + cwd: workspaceRoot, + args: ["worktree", "add", "--detach", "--quiet", path, headSha], + timeoutMs: 5 * 60_000, + }); + yield* Effect.addFinalizer(() => + git + .removeWorktree({ cwd: workspaceRoot, path, force: true }) + .pipe(Effect.ignore({ log: true })), + ); + // Measured from where the head left the base, so a stacked change is briefed alone rather + // than with everything below it that the base has not taken yet. + const mergeBase = yield* git.execute({ + operation: "PullRequestReviewGuideService.mergeBase", + cwd: path, + args: ["merge-base", baseTip.commitSha, "HEAD"], + }); + return { path, baseCommit: mergeBase.stdout.trim(), stack: layers }; + }); + + /** The changed paths and their hunks, which a generated guide is held to. */ + const readChange = Effect.fn("PullRequestReviewGuideService.readChange")(function* ( + cwd: string, + baseCommit: string, + ) { + const diff = (args: ReadonlyArray) => + git + .execute({ + operation: "PullRequestReviewGuideService.readChange", + cwd, + // Fixed prefixes: a user's diff.noPrefix or custom prefixes would hide the headers the + // parser reads. + args: [ + "-c", + "core.quotePath=false", + "diff", + ...PATCH_RENDER_PREFIX_ARGS, + ...args, + `${baseCommit}...HEAD`, + ], + timeoutMs: 60_000, + // Hunk headers only, but a large change still has many of them. + maxOutputBytes: 64 * 1024 * 1024, + }) + .pipe(Effect.map((result) => result.stdout)); + const [nameOnly, patch] = yield* Effect.all( + [diff(["--name-only", "-z"]), diff(["--unified=0", "--no-color", "--no-ext-diff"])], + { concurrency: "unbounded" }, + ); + return parseReviewGuideChange(nameOnly, patch); + }); + + const write = Effect.fn("PullRequestReviewGuideService.write")(function* ( + input: PullRequestRef, + key: ThreadPullRequestKey, + workspaceRoot: string, + detail: PullRequestDetail, + headSha: string, + runId: string, + owns: Effect.Effect, + ) { + const [activity, settings, providers, stack] = yield* Effect.all( + [ + pullRequests.activity(input), + serverSettings.getSettings, + providerRegistry.getProviders, + // A guide without its stack is still a guide; the stack only adds seams to point at. + pullRequests.stack(input).pipe(Effect.orElseSucceed(() => null)), + ], + { concurrency: "unbounded" }, + ); + const projectSettings = resolveProjectSettings(settings, input.projectId).settings; + const modelSelection = ServerSettings.resolveReviewGuideModelSelection( + projectSettings, + providers, + ); + // The checkout lives only as long as the model is reading it. The change's hunks are read + // from it too, so the guide can be held to the lines the change actually touched. + const { generated, change } = yield* checkoutHead( + workspaceRoot, + detail, + headSha, + stack, + runId, + ).pipe( + Effect.flatMap((checkout) => + Effect.all( + { + generated: textGeneration.generateReviewGuide({ + ...reviewGuideInputFromPullRequest(detail, activity, checkout.baseCommit), + cwd: checkout.path, + modelSelection, + instructions: projectSettings.reviewGuideInstructions, + stack: checkout.stack, + }), + change: readChange(checkout.path, checkout.baseCommit), + }, + { concurrency: "unbounded" }, + ), + ), + Effect.scoped, + ); + const guide: PullRequestReviewGuide = { + headSha, + generatedAt: yield* nowIso, + modelSelection, + ...normalizeReviewGuide(generated, change), + }; + // A newer run for this pull request, say after a push, owns its guide now; this one's result + // is for an older head and is dropped rather than written over it. + yield* ownership.withPermits(1)( + Effect.gen(function* () { + if (yield* owns) yield* guides.put(key, guide); + }), + ); + }); + + /** What a reader is told when a run fails; the full cause goes to the log. */ + const failureDetail = (cause: Cause.Cause): string => { + const error = Cause.squash(cause); + if (error instanceof Error && "detail" in error && typeof error.detail === "string") { + return error.detail; + } + return "The review guide could not be written."; + }; + + const generate: PullRequestReviewGuideService["Service"]["generate"] = Effect.fn( + "PullRequestReviewGuideService.generate", + )(function* (input) { + // A beta the server owns, so every client and agent sees the same answer. + const settings = yield* serverSettings.getSettings.pipe( + Effect.mapError( + (cause) => + new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: "Could not read the server settings.", + cause, + }), + ), + ); + if (!settings.reviewGuidesEnabled) { + return yield* new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: "Review guides are off. Turn them on in Settings → Source Control.", + }); + } + const { key, workspaceRoot, identity } = yield* keyFor(input); + yield* requireOwnRepository(key, identity); + const runKey = keyString(key); + // A press asks for the head as it is now, not a read shared from the last minute, so a guide + // is never started for a commit the pull request has already moved past. + yield* pullRequests.invalidate({ reference: input }); + const [detail, stored] = yield* Effect.all([pullRequests.detail(input), readStored(key)], { + concurrency: "unbounded", + }); + const currentHeadSha = detail.headSha ?? null; + if (currentHeadSha === null) { + return yield* new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: "This host did not report the pull request's head commit.", + }); + } + const running: PullRequestReviewGuideRun = { + _tag: "Running", + headSha: currentHeadSha, + startedAt: yield* nowIso, + }; + // Claimed atomically, so two readers pressing Generate start one run between them. The head + // is read again under the claim: a press that read an older head before a push must not + // replace a run another press started for the new one, so it asks to be pressed again. + const claimed = yield* ownership.withPermits(1)( + pullRequests.detail(input).pipe( + Effect.flatMap((latest) => + (latest.headSha ?? null) !== currentHeadSha + ? Effect.fail( + new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: "The pull request changed while the guide was starting. Try again.", + }), + ) + : Ref.modify(runs, (current) => { + const existing = current.get(runKey); + if (existing?._tag === "Running" && existing.headSha === currentHeadSha) { + return [existing, current] as const; + } + return [running, new Map(current).set(runKey, running)] as const; + }), + ), + ), + ); + if (claimed !== running) return { guide: stored, run: claimed, currentHeadSha }; + + // The run is identified by its own Running value: a later claim replaces it, and from then + // on this run's completion leaves the newer run's state and guide alone. + const owns = Ref.get(runs).pipe(Effect.map((current) => current.get(runKey) === running)); + const finish = (next: PullRequestReviewGuideRun | null) => + Ref.update(runs, (current) => { + if (current.get(runKey) !== running) return current; + const updated = new Map(current); + if (next === null) updated.delete(runKey); + else updated.set(runKey, next); + return updated; + }); + const runId = `${runCounter++}`; + yield* write(input, key, workspaceRoot, detail, currentHeadSha, runId, owns).pipe( + Effect.andThen(finish(null)), + Effect.catchCause((cause) => + Cause.hasInterruptsOnly(cause) + ? finish(null) + : Effect.logWarning("Review guide generation failed", { + key: runKey, + cause: Cause.pretty(cause), + }).pipe( + Effect.andThen(nowIso), + Effect.flatMap((failedAt) => + finish({ + _tag: "Failed", + headSha: currentHeadSha, + failedAt, + detail: failureDetail(cause), + }), + ), + ), + ), + Effect.forkIn(serviceScope), + ); + return { guide: stored, run: running, currentHeadSha }; + }); + + return PullRequestReviewGuideService.of({ get, generate }); +}); + +export const layer = Layer.effect(PullRequestReviewGuideService, make); diff --git a/apps/server/src/pullRequest/pullRequestReviewGuideInput.test.ts b/apps/server/src/pullRequest/pullRequestReviewGuideInput.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..3db791492c6d22634407403a761777131d487dcc --- /dev/null +++ b/apps/server/src/pullRequest/pullRequestReviewGuideInput.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from "vite-plus/test"; +import type { PullRequestActivity, PullRequestReviewThread } from "@t3tools/contracts"; + +import { settledReviewSummary } from "./pullRequestReviewGuideInput.ts"; + +const thread = ( + overrides: Partial & { body?: string; login?: string }, +): PullRequestReviewThread => ({ + id: overrides.id ?? "t1", + path: overrides.path ?? "src/a.ts", + line: 1, + side: "right", + isResolved: overrides.isResolved ?? false, + isOutdated: overrides.isOutdated ?? false, + comments: [ + { + id: "c1", + author: { login: overrides.login ?? "macroscope[bot]", name: null, avatarUrl: null }, + body: overrides.body ?? "Possible null dereference", + createdAt: "2026-10-01T00:00:00.000Z", + url: null, + }, + ], +}); + +const activity = (reviewThreads: ReadonlyArray): PullRequestActivity => ({ + comments: [], + commentCount: 0, + commentsTruncated: false, + reviewThreads, + commits: [], +}); + +describe("settledReviewSummary", () => { + it("lists resolved and outdated threads and leaves open ones for the guide", () => { + const summary = settledReviewSummary( + activity([ + thread({ id: "open", path: "src/open.ts", body: "Still open" }), + thread({ id: "resolved", path: "src/a.ts", isResolved: true }), + thread({ + id: "outdated", + path: "src/b.ts", + isOutdated: true, + login: "julius", + body: "Rename this \n\nplease", + }), + ]), + ); + + expect(summary).toBe( + [ + "- src/a.ts (resolved, macroscope[bot]): Possible null dereference", + "- src/b.ts (outdated, julius): Rename this please", + ].join("\n"), + ); + }); +}); diff --git a/apps/server/src/pullRequest/pullRequestReviewGuideInput.ts b/apps/server/src/pullRequest/pullRequestReviewGuideInput.ts new file mode 100644 index 0000000000000000000000000000000000000000..76d0d4cb40267ac95a8b9b7ec078ff5207f6efaf --- /dev/null +++ b/apps/server/src/pullRequest/pullRequestReviewGuideInput.ts @@ -0,0 +1,47 @@ +import type { PullRequestActivity, PullRequestDetail } from "@t3tools/contracts"; +import type { ReviewGuideGenerationInput } from "@t3tools/provider-core/server/textGeneration"; + +/** Longest a settled remark is quoted. The guide needs what was raised, not the argument. */ +const SETTLED_REMARK_CHARS = 200; + +function oneLine(text: string): string { + const flat = text + .replace(//g, "") + .replace(/\s+/g, " ") + .trim(); + return flat.length > SETTLED_REMARK_CHARS ? `${flat.slice(0, SETTLED_REMARK_CHARS)}…` : flat; +} + +/** + * What review already settled, one line per conversation: resolved threads, and threads whose + * line a later push removed. These are the review loop's finished work; the guide is told about + * them so it does not raise them again. + */ +export function settledReviewSummary(activity: PullRequestActivity): string { + return activity.reviewThreads + .filter((thread) => thread.isResolved || thread.isOutdated) + .flatMap((thread) => { + const opener = thread.comments[0]; + if (opener === undefined) return []; + const who = opener.author?.login ?? "ghost"; + const state = thread.isResolved ? "resolved" : "outdated"; + return [`- ${thread.path} (${state}, ${who}): ${oneLine(opener.body)}`]; + }) + .join("\n"); +} + +/** The prompt input for a guide, minus the checkout and model the caller provides. */ +export function reviewGuideInputFromPullRequest( + detail: PullRequestDetail, + activity: PullRequestActivity, + baseCommit: string, +): Omit { + return { + number: detail.number, + title: detail.title, + description: detail.body, + baseBranch: detail.baseBranch, + baseCommit, + settledReview: settledReviewSummary(activity), + }; +} diff --git a/apps/server/src/pullRequest/pullRequestReviewGuideNormalize.test.ts b/apps/server/src/pullRequest/pullRequestReviewGuideNormalize.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..71bef8c448052e9d411c42f2bf00970588f3a6fa --- /dev/null +++ b/apps/server/src/pullRequest/pullRequestReviewGuideNormalize.test.ts @@ -0,0 +1,212 @@ +import { describe, expect, it } from "vite-plus/test"; +import type { PullRequestReviewGuideStop } from "@t3tools/contracts"; +import type { ReviewGuideGenerationResult } from "@t3tools/provider-core/server/textGeneration"; + +import { normalizeReviewGuide, parseReviewGuideChange } from "./pullRequestReviewGuideNormalize.ts"; + +// `git diff --unified=0` of: src/a.ts with lines 10-14 and 40-59 added and an old line 30 +// removed; src/gone.ts deleted (old lines 1-20); docs/readme.md with line 3 added. +const patch = [ + "diff --git a/src/a.ts b/src/a.ts", + "index 1111111..2222222 100644", + "--- a/src/a.ts", + "+++ b/src/a.ts", + "@@ -9,0 +10,5 @@ export const x = 1;", + "+added", + "@@ -30 +34,0 @@ function y() {", + "-removed", + "@@ -35,0 +40,20 @@", + "+added", + "diff --git a/src/gone.ts b/src/gone.ts", + "deleted file mode 100644", + "--- a/src/gone.ts", + "+++ /dev/null", + "@@ -1,20 +0,0 @@", + "-removed", + "diff --git a/docs/readme.md b/docs/readme.md", + "--- a/docs/readme.md", + "+++ b/docs/readme.md", + "@@ -2,0 +3 @@", + "+added", + "", +].join("\n"); +const change = parseReviewGuideChange("src/a.ts\0src/gone.ts\0docs/readme.md\0", patch); + +const stop = (overrides: Partial): PullRequestReviewGuideStop => ({ + title: "Stop", + summary: "", + attention: null, + excerpts: [], + files: [], + notes: [], + questions: [], + ...overrides, +}); + +const guide = (overrides: Partial): ReviewGuideGenerationResult => ({ + stops: [], + ...overrides, +}); + +const excerpt = (path: string, start: number, end: number, side: "new" | "old" = "new") => ({ + path, + side, + start, + end, + caption: "", +}); + +describe("parseReviewGuideChange", () => { + it("reads a removed or added line that looks like a file header as a source line", () => { + const tricky = [ + "diff --git a/notes.md b/notes.md", + "--- a/notes.md", + "+++ b/notes.md", + "@@ -3,1 +3,2 @@", + "--- a/old-heading", + "+++ b/other.ts", + "+tail", + "@@ -9,0 +10,1 @@", + "+later", + "", + ].join("\n"); + const parsed = parseReviewGuideChange("notes.md\0", tricky); + expect([...parsed.hunks.keys()]).toEqual(["notes.md"]); + expect(parsed.hunks.get("notes.md")).toEqual({ + new: [ + { start: 3, end: 4 }, + { start: 10, end: 10 }, + ], + old: [{ start: 3, end: 3 }], + }); + }); + + it("reads each side's hunks, keying a deleted file by its old path", () => { + expect(change.files).toEqual(["src/a.ts", "src/gone.ts", "docs/readme.md"]); + expect(change.hunks.get("src/a.ts")).toEqual({ + new: [ + { start: 10, end: 14 }, + { start: 40, end: 59 }, + ], + old: [{ start: 30, end: 30 }], + }); + expect(change.hunks.get("src/gone.ts")).toEqual({ new: [], old: [{ start: 1, end: 20 }] }); + expect(change.hunks.get("docs/readme.md")).toEqual({ new: [{ start: 3, end: 3 }], old: [] }); + }); +}); + +describe("normalizeReviewGuide", () => { + it("clamps excerpts to changed lines, merges touching ones, and caps their length", () => { + const result = normalizeReviewGuide( + guide({ + stops: [ + stop({ + excerpts: [ + // Starts on context before the hunk: clamped to the hunk. + excerpt("src/a.ts", 5, 12), + // Touches the previous one once clamped: merged. + excerpt("src/a.ts", 13, 14), + // Spans the whole second hunk and beyond: clamped to it. + excerpt("src/a.ts", 38, 80), + // On no changed line: dropped. + excerpt("src/a.ts", 20, 25), + // Removed code shown from the old side. + excerpt("src/gone.ts", 2, 8, "old"), + ], + files: [{ path: "docs/readme.md", note: "" }], + }), + ], + }), + change, + ); + expect(result.stops[0]?.excerpts).toEqual([ + excerpt("src/a.ts", 10, 14), + excerpt("src/a.ts", 40, 59), + excerpt("src/gone.ts", 2, 8, "old"), + ]); + expect(result.stops[0]?.files).toEqual([{ path: "docs/readme.md" }]); + expect(result.stops).toHaveLength(1); + }); + + it("keeps a long excerpt to its first 25 lines", () => { + const long = parseReviewGuideChange( + "big.ts\0", + "--- a/big.ts\n+++ b/big.ts\n@@ -0,0 +1,100 @@\n", + ); + const result = normalizeReviewGuide( + guide({ stops: [stop({ excerpts: [excerpt("big.ts", 1, 100)] })] }), + long, + ); + expect(result.stops[0]?.excerpts).toEqual([excerpt("big.ts", 1, 25)]); + }); + + it("drops notes outside the stop's remaining excerpts", () => { + const result = normalizeReviewGuide( + guide({ + stops: [ + stop({ + excerpts: [excerpt("src/a.ts", 5, 12)], + notes: [ + { path: "src/a.ts", line: 11, title: "Inside", body: "" }, + { path: "src/a.ts", line: 6, title: "Clamped away", body: "" }, + { path: "src/other.ts", line: 11, title: "Other file", body: "" }, + ], + }), + ], + }), + change, + ); + expect(result.stops[0]?.notes.map((note) => note.title)).toEqual(["Inside"]); + }); + + it("drops excerpts and files outside the change", () => { + const result = normalizeReviewGuide( + guide({ + stops: [ + stop({ + excerpts: [excerpt("src/unchanged.ts", 1, 5)], + files: [{ path: "src/a.ts", note: "edited" }, { path: "src/unchanged.ts" }], + }), + ], + }), + change, + ); + expect(result.stops[0]?.excerpts).toEqual([]); + expect(result.stops[0]?.files).toEqual([{ path: "src/a.ts", note: "edited" }]); + }); + + it("puts changed files the tour left out into a closing stop", () => { + const result = normalizeReviewGuide( + guide({ stops: [stop({ excerpts: [excerpt("src/a.ts", 10, 12)] })] }), + change, + ); + expect(result.stops.at(-1)).toMatchObject({ + title: "Other files", + files: [{ path: "src/gone.ts" }, { path: "docs/readme.md" }], + }); + }); + + it("lets stops show different parts of one file and lists an unshown file once", () => { + const result = normalizeReviewGuide( + guide({ + stops: [ + stop({ + title: "Shape", + excerpts: [excerpt("src/a.ts", 10, 14)], + files: [{ path: "docs/readme.md" }], + }), + stop({ + title: "Implementation", + excerpts: [excerpt("src/a.ts", 40, 50)], + files: [{ path: "src/a.ts" }, { path: "docs/readme.md" }, { path: "src/gone.ts" }], + }), + ], + }), + change, + ); + expect(result.stops.map((s) => [s.title, s.excerpts.map((e) => e.start), s.files])).toEqual([ + ["Shape", [10], [{ path: "docs/readme.md" }]], + ["Implementation", [40], [{ path: "src/gone.ts" }]], + ]); + }); +}); diff --git a/apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts b/apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts new file mode 100644 index 0000000000000000000000000000000000000000..fdcf526b31b38851da603953e79f5bafefe02030 --- /dev/null +++ b/apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts @@ -0,0 +1,184 @@ +import type { PullRequestReviewGuideExcerpt, PullRequestReviewGuideStop } from "@t3tools/contracts"; +import type { ReviewGuideGenerationResult } from "@t3tools/provider-core/server/textGeneration"; + +/** Longest excerpt kept. The reader wants the lines that carry the idea, not the whole hunk. */ +const MAX_EXCERPT_LINES = 25; + +interface LineRange { + readonly start: number; + readonly end: number; +} + +/** The lines a change touched in one file, on each side of the diff. */ +interface FileHunks { + readonly new: Array; + readonly old: Array; +} + +/** What a guide is held to: the changed paths, and the lines each touched. */ +export interface ReviewGuideChange { + /** Every changed path, in diff order, named as `git diff --name-only` names it. */ + readonly files: ReadonlyArray; + readonly hunks: ReadonlyMap; +} + +const HUNK_HEADER = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/; + +const hunkRange = (start: string, count: string | undefined): LineRange | null => { + const lines = count === undefined ? 1 : Number(count); + return lines === 0 ? null : { start: Number(start), end: Number(start) + lines - 1 }; +}; + +/** + * Reads `git diff --name-only -z` and `git diff --unified=0` of the same range. Hunks are keyed + * by the new path, or the old one for a deleted file, so both sides of a renamed file sit under + * the name the model sees in `--stat`. + */ +export function parseReviewGuideChange(nameOnly: string, patch: string): ReviewGuideChange { + const hunks = new Map(); + let oldPath: string | null = null; + let current: FileHunks | null = null; + // Inside a file's hunks a `--- ` or `+++ ` line is a removed or added source line, not a header. + let inHunks = false; + for (const line of patch.split("\n")) { + if (line.startsWith("diff --git ")) { + oldPath = null; + current = null; + inHunks = false; + } else if (!inHunks && line.startsWith("--- ")) { + oldPath = line.startsWith("--- a/") ? line.slice(6) : null; + } else if (!inHunks && line.startsWith("+++ ")) { + const path = line.startsWith("+++ b/") ? line.slice(6) : oldPath; + if (path === null) continue; + current = hunks.get(path) ?? { new: [], old: [] }; + hunks.set(path, current); + } else if (current !== null) { + const header = HUNK_HEADER.exec(line); + if (header === null) continue; + inHunks = true; + const removed = hunkRange(header[1]!, header[2]); + const added = hunkRange(header[3]!, header[4]); + if (removed !== null) current.old.push(removed); + if (added !== null) current.new.push(added); + } + } + return { files: nameOnly.split("\0").filter((path) => path !== ""), hunks }; +} + +/** + * Narrows an excerpt to the changed lines it covers: from the first changed line it overlaps to + * the last. Null when it covers none, which means the model pointed somewhere the change is not. + */ +function clampExcerpt( + excerpt: PullRequestReviewGuideExcerpt, + change: ReviewGuideChange, +): PullRequestReviewGuideExcerpt | null { + const start = Math.min(excerpt.start, excerpt.end); + const end = Math.max(excerpt.start, excerpt.end); + const overlapping = (change.hunks.get(excerpt.path)?.[excerpt.side] ?? []).filter( + (hunk) => hunk.start <= end && start <= hunk.end, + ); + const first = overlapping[0]; + const last = overlapping.at(-1); + if (first === undefined || last === undefined) return null; + return { ...excerpt, start: Math.max(start, first.start), end: Math.min(end, last.end) }; +} + +/** Joins a stop's excerpts that overlap or touch in one file, then caps each one's length. */ +function mergeExcerpts( + excerpts: ReadonlyArray, +): Array { + const merged: Array = []; + for (const excerpt of excerpts) { + const index = merged.findIndex( + (other) => + other.path === excerpt.path && + other.side === excerpt.side && + other.start <= excerpt.end + 1 && + excerpt.start <= other.end + 1, + ); + const other = merged[index]; + if (other === undefined) { + merged.push(excerpt); + continue; + } + merged[index] = { + ...other, + start: Math.min(other.start, excerpt.start), + end: Math.max(other.end, excerpt.end), + caption: other.caption || excerpt.caption, + }; + } + return merged.map((excerpt) => ({ + ...excerpt, + end: Math.min(excerpt.end, excerpt.start + MAX_EXCERPT_LINES - 1), + })); +} + +const nonEmpty = (value: string) => value.trim() !== ""; + +/** + * Holds a generated guide to the change it describes. A model's line numbers and ids are + * suggestions: excerpts are clamped to changed lines, notes must sit inside what is left, and any + * changed file the tour skipped lands in a closing skim stop. + */ +export function normalizeReviewGuide( + guide: ReviewGuideGenerationResult, + change: ReviewGuideChange, +): ReviewGuideGenerationResult { + const changed = new Set(change.files); + const inChange = (path: string) => changed.has(path); + + const shaped: Array = guide.stops + .filter((stop) => nonEmpty(stop.title)) + .map((stop) => { + const excerpts = mergeExcerpts( + stop.excerpts.flatMap((excerpt) => { + const clamped = inChange(excerpt.path) ? clampExcerpt(excerpt, change) : null; + return clamped === null ? [] : [clamped]; + }), + ); + return { + ...stop, + attention: stop.attention !== null && nonEmpty(stop.attention.note) ? stop.attention : null, + excerpts, + files: stop.files + .filter((file) => inChange(file.path)) + .map(({ path, note }) => (note?.trim() ? { path, note } : { path })), + notes: stop.notes.filter( + (note) => + nonEmpty(note.title) && + excerpts.some( + (excerpt) => + excerpt.path === note.path && + excerpt.start <= note.line && + note.line <= excerpt.end, + ), + ), + questions: stop.questions.filter(nonEmpty), + }; + }); + + // A file can be excerpted in several stops, its shape in one and its implementation in another. + // A files entry is for a file the tour never shows, so it is listed once and only then. + const placed = new Set(shaped.flatMap((stop) => stop.excerpts.map((excerpt) => excerpt.path))); + const stops = shaped.map((stop) => { + const files = stop.files.filter((file) => !placed.has(file.path)); + for (const file of files) placed.add(file.path); + return { ...stop, files }; + }); + const unplaced = change.files.filter((path) => !placed.has(path)); + if (unplaced.length > 0) { + stops.push({ + title: "Other files", + summary: "Changed files the tour does not otherwise cover.", + attention: null, + excerpts: [], + files: unplaced.map((path) => ({ path })), + notes: [], + questions: [], + }); + } + + return { ...guide, stops }; +} diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index f1daa720016f0e55db68aba936e58af93fc17c7c..9f997b1aa30f7490585d3594e4e91d234a96b3f1 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -39,6 +39,8 @@ import * as PullRequestProviderRegistry from "./pullRequest/PullRequestProviderR import * as PullRequestService from "./pullRequest/PullRequestService.ts"; import * as SqlitePersistence from "./persistence/Sqlite.ts"; import * as PullRequestFilesViewed from "./persistence/PullRequestFilesViewed.ts"; +import * as PullRequestReviewGuides from "./persistence/PullRequestReviewGuides.ts"; +import * as PullRequestReviewGuideService from "./pullRequest/PullRequestReviewGuideService.ts"; import * as ServerLifecycleEvents from "./serverLifecycleEvents.ts"; import * as AnalyticsService from "./telemetry/AnalyticsService.ts"; import * as ProviderEventIngestor from "./orchestration-v2/ProviderEventIngestor.ts"; @@ -717,8 +719,13 @@ const layerMakeRoutes = Layer.mergeAll( Layer.provide(McpOAuth.layerMcpClientAuthenticator), ), ).pipe( + // Guides read through the same pull request service as every other surface. + Layer.provide( + PullRequestReviewGuideService.layer.pipe(Layer.provide(PullRequestReviewGuides.layer)), + ), // Both transports consume the same service instance, so caches single-flight across clients - // and mutations observed on WebSocket invalidate patches subsequently read over HTTP. + // and mutations observed on WebSocket invalidate patches subsequently read over HTTP. Provided + // after the guide service so it reaches that too. Layer.provide(layerPullRequestService), // The stream route and the WebSocket RPCs share one browser. Layer.provide(ServerBrowser.layer.pipe(Layer.provide(DesktopBrowserChannel.layer))), diff --git a/apps/server/src/serverSettings.ts b/apps/server/src/serverSettings.ts index 5a36f77e4220bf3e7d8859e763e63d698901887b..0d39ee4ee79ef6ac9a6a858f363a30af9306e1ae 100644 --- a/apps/server/src/serverSettings.ts +++ b/apps/server/src/serverSettings.ts @@ -72,7 +72,10 @@ import { writeSecretFieldValues, } from "./sourceControl/sourceControlHostSecrets.ts"; -export { resolveSourceControlWriterModelSelection } from "@t3tools/shared/serverSettings"; +export { + resolveReviewGuideModelSelection, + resolveSourceControlWriterModelSelection, +} from "@t3tools/shared/serverSettings"; const encodeServerSettings = Schema.encodeEffect(ServerSettings); const encodeServerSettingsJson = Schema.encodeUnknownEffect(fromJsonStringPretty(ServerSettings)); @@ -613,6 +616,7 @@ const ATOMIC_SETTINGS_KEYS: ReadonlySet = new Set([ "automaticGitFetchInterval", "providerHealthRefreshInterval", "sourceControlWriterModelSelection", + "reviewGuideModelSelection", "textGenerationModelSelection", "pullRequestMergeMethod", ]); diff --git a/apps/server/src/textGeneration/AntigravityTextGeneration.ts b/apps/server/src/textGeneration/AntigravityTextGeneration.ts index 2c06b082feef44966359ac01ab7bdadb661362ce..b7c2ed4acc62464ed458ba57e1f24499d20f0ecb 100644 --- a/apps/server/src/textGeneration/AntigravityTextGeneration.ts +++ b/apps/server/src/textGeneration/AntigravityTextGeneration.ts @@ -306,5 +306,8 @@ export const makeAntigravityTextGeneration = Effect.fn("makeAntigravityTextGener ), ); - return TextGenerationOperations.fromRunner("AntigravityTextGeneration", runAntigravityJson); + return TextGenerationOperations.fromRunner( + "AntigravityTextGeneration", + TextGenerationOperations.withoutInvestigation("Antigravity", runAntigravityJson), + ); }); diff --git a/apps/server/src/textGeneration/ClaudeTextGeneration.ts b/apps/server/src/textGeneration/ClaudeTextGeneration.ts index 25695f7fdefae498d680a221db7f0fb7df647fb6..7c6eb7532490b6960d9624c62685104d231a71eb 100644 --- a/apps/server/src/textGeneration/ClaudeTextGeneration.ts +++ b/apps/server/src/textGeneration/ClaudeTextGeneration.ts @@ -40,6 +40,22 @@ import { import { makeClaudeEnvironment } from "../provider/Drivers/ClaudeHome.ts"; const CLAUDE_TIMEOUT_MS = 180_000; +/** The tools an investigation can see. `--tools` takes bare names; patterns go in the allow list. */ +const CLAUDE_INVESTIGATION_TOOLS = "Read,Grep,Glob,Bash"; +/** What it may run without asking. In dontAsk mode every other call, Bash included, is refused. */ +const CLAUDE_INVESTIGATION_ALLOWED = [ + "Read", + "Grep", + "Glob", + "Bash(git diff:*)", + "Bash(git log:*)", + "Bash(git show:*)", +].join(","); +/** + * The allowed git commands accept any trailing arguments, and `--output` makes each of them write + * a file anywhere. Deny rules win over allow rules, so this keeps the investigation read-only. + */ +const CLAUDE_INVESTIGATION_DENIED = ["Bash(git * --output*)", "Bash(git * -o *)"].join(","); /** * Schema for the wrapper JSON returned by `claude -p --output-format json`. @@ -112,6 +128,7 @@ export const makeClaudeTextGeneration = Effect.fn("makeClaudeTextGeneration")(fu prompt, outputSchema: outputSchemaJson, modelSelection, + investigate, }: TextGenerationOperations.Request): Effect.fn.Return< S["Type"], TextGenerationError, @@ -189,8 +206,18 @@ export const makeClaudeTextGeneration = Effect.fn("makeClaudeTextGeneration")(fu "--settings", settingsJson, // Metadata prompts need no executable capabilities, even when they contain a skill name. - "--tools", - "", + // An investigation gets the read-only file tools and Git's read commands, and nothing + // that writes: in dontAsk mode anything not allowed here is refused. + ...(investigate + ? [ + "--tools", + CLAUDE_INVESTIGATION_TOOLS, + "--allowedTools", + CLAUDE_INVESTIGATION_ALLOWED, + "--disallowedTools", + CLAUDE_INVESTIGATION_DENIED, + ] + : ["--tools", ""]), "--disable-slash-commands", "--strict-mcp-config", "--permission-mode", @@ -246,7 +273,7 @@ export const makeClaudeTextGeneration = Effect.fn("makeClaudeTextGeneration")(fu const rawStdout = yield* runClaudeCommand().pipe( Effect.scoped, - Effect.timeoutOption(CLAUDE_TIMEOUT_MS), + Effect.timeoutOption(investigate?.timeoutMs ?? CLAUDE_TIMEOUT_MS), Effect.flatMap( Option.match({ onNone: () => diff --git a/apps/server/src/textGeneration/CodexTextGeneration.ts b/apps/server/src/textGeneration/CodexTextGeneration.ts index 3902b0c74d78e300e191ae04656ef6e8be871dcd..16d8bf9a794ed47f2bb80000dee36dec323e2f36 100644 --- a/apps/server/src/textGeneration/CodexTextGeneration.ts +++ b/apps/server/src/textGeneration/CodexTextGeneration.ts @@ -150,6 +150,7 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func outputSchema: outputSchemaJson, modelSelection, attachments, + investigate, }: TextGenerationOperations.Request): Effect.fn.Return< S["Type"], TextGenerationError, @@ -272,7 +273,8 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func return yield* Effect.gen(function* () { yield* runCodexCommand().pipe( Effect.scoped, - Effect.timeoutOption(CODEX_TIMEOUT_MS), + // The read-only sandbox already holds an investigation to reading; it only needs longer. + Effect.timeoutOption(investigate?.timeoutMs ?? CODEX_TIMEOUT_MS), Effect.flatMap( Option.match({ onNone: () => diff --git a/apps/server/src/textGeneration/TextGeneration.test.ts b/apps/server/src/textGeneration/TextGeneration.test.ts index 61baaebe4dc20b916baac9f648a02c8488ea8f05..64d80e1b3e6e41f495b9a3e58710ef549de8eb9c 100644 --- a/apps/server/src/textGeneration/TextGeneration.test.ts +++ b/apps/server/src/textGeneration/TextGeneration.test.ts @@ -24,6 +24,7 @@ const makeStubTextGeneration = ( generatePrContent: () => Effect.die("generatePrContent stub not configured for this test"), generateBranchName: () => Effect.die("generateBranchName stub not configured for this test"), generateThreadTitle: () => Effect.die("generateThreadTitle stub not configured for this test"), + generateReviewGuide: () => Effect.die("generateReviewGuide stub not configured for this test"), ...overrides, }); diff --git a/apps/server/src/textGeneration/TextGeneration.ts b/apps/server/src/textGeneration/TextGeneration.ts index ee6f18e7b217bbde235b26d4a66fc1cdee2f046b..11359145b54b970860a518e255b208869b28855d 100644 --- a/apps/server/src/textGeneration/TextGeneration.ts +++ b/apps/server/src/textGeneration/TextGeneration.ts @@ -17,6 +17,8 @@ export type { CommitMessageGenerationResult, PrContentGenerationInput, PrContentGenerationResult, + ReviewGuideGenerationInput, + ReviewGuideGenerationResult, ThreadTitleGenerationInput, ThreadTitleGenerationResult, } from "@t3tools/provider-core/server/textGeneration"; @@ -32,7 +34,8 @@ type TextGenerationOp = | "generateCommitMessage" | "generatePrContent" | "generateBranchName" - | "generateThreadTitle"; + | "generateThreadTitle" + | "generateReviewGuide"; const resolveInstance = ( registry: ProviderInstanceRegistry.ProviderInstanceRegistry["Service"], @@ -69,6 +72,10 @@ export const make = Effect.gen(function* () { resolveInstance(registry, "generateBranchName", input.modelSelection.instanceId).pipe( Effect.flatMap((textGeneration) => textGeneration.generateBranchName(input)), ), + generateReviewGuide: (input) => + resolveInstance(registry, "generateReviewGuide", input.modelSelection.instanceId).pipe( + Effect.flatMap((textGeneration) => textGeneration.generateReviewGuide(input)), + ), generateThreadTitle: (input) => resolveInstance(registry, "generateThreadTitle", input.modelSelection.instanceId).pipe( Effect.flatMap((textGeneration) => diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 61bad3faf318498c8f901f8d5cb7f1f00640ccb9..80921ec4b5a2837b90ddd4dfb24b3fa545c2be08 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -224,6 +224,7 @@ import * as AnalyticsService from "./telemetry/AnalyticsService.ts"; import * as UsageService from "./usage/UsageService.ts"; import * as TraceDiagnostics from "./diagnostics/TraceDiagnostics.ts"; import * as PullRequestService from "./pullRequest/PullRequestService.ts"; +import * as PullRequestReviewGuideService from "./pullRequest/PullRequestReviewGuideService.ts"; import { listLinkedPullRequestThreads } from "./pullRequest/linkedThreads.ts"; import { pullRequestSyncKey } from "./pullRequest/pullRequestSyncKey.ts"; import * as SqlClient from "effect/sql/SqlClient"; @@ -1225,6 +1226,7 @@ const layerWsRpc = ( const scheduledTasks = yield* ScheduledTasks.ScheduledTaskService; const secretRequests = yield* SecretRequests.SecretRequests; const pullRequests = yield* PullRequestService.PullRequestService; + const reviewGuides = yield* PullRequestReviewGuideService.PullRequestReviewGuideService; const pullRequestSync = yield* PullRequestSyncReactor.PullRequestSyncReactor; const deviceService = yield* DeviceService.DeviceService; const deviceHostContext = @@ -2490,6 +2492,10 @@ const layerWsRpc = ( withPullRequestViewer(input, pullRequests.checks(input)), [WS_METHODS.pullRequestsActivity]: (input) => withPullRequestViewer(input, pullRequests.activity(input)), + [WS_METHODS.pullRequestsReviewGuide]: (input) => + withPullRequestViewer(input, reviewGuides.get(input)), + [WS_METHODS.pullRequestsGenerateReviewGuide]: (input) => + withPullRequestViewer(input, reviewGuides.generate(input)), [WS_METHODS.pullRequestsThreadComments]: (input) => withPullRequestViewer(input, pullRequests.threadComments(input)), [WS_METHODS.pullRequestsDiffFileContents]: (input) => @@ -3138,6 +3144,7 @@ export const layer = Layer.unwrap( const serverBrowser = yield* ServerBrowser.ServerBrowser; const serverSelfUpdate = yield* ServerSelfUpdate.ServerSelfUpdate; const pullRequests = yield* PullRequestService.PullRequestService; + const reviewGuides = yield* PullRequestReviewGuideService.PullRequestReviewGuideService; const sql = yield* SqlClient.SqlClient; return HttpRouter.add( "GET", @@ -3205,6 +3212,13 @@ export const layer = Layer.unwrap( // mutation invalidates the HTTP diff cache that every client reads from. Layer.provide(Layer.succeed(PullRequestService.PullRequestService, pullRequests)), Layer.provide(OtlpEndpointCheck.layer), + // Shared like the pull request service, so concurrent generations single-flight. + Layer.provide( + Layer.succeed( + PullRequestReviewGuideService.PullRequestReviewGuideService, + reviewGuides, + ), + ), Layer.provide( SourceControlDiscovery.layer.pipe( Layer.provide( diff --git a/apps/web/src/components/diffs/StyledDiffCodeView.tsx b/apps/web/src/components/diffs/StyledDiffCodeView.tsx index e18d9ca4ba82eee250ee272908426a000de3a7b3..9522f53b6d1480a6469723cd0aa5f08fb48594af 100644 --- a/apps/web/src/components/diffs/StyledDiffCodeView.tsx +++ b/apps/web/src/components/diffs/StyledDiffCodeView.tsx @@ -12,7 +12,8 @@ import type { Ref } from "react"; import { DIFF_SURFACE_THEME_UNSAFE_CSS } from "~/lib/diffRendering"; import { DiffWorkerPoolProvider } from "../DiffWorkerPoolProvider"; -const DIFF_VIEW_UNSAFE_CSS = `${DIFF_SURFACE_THEME_UNSAFE_CSS} +/** The app's diff stylesheet, shared by single-file renderers so their separators match. */ +export const DIFF_VIEW_UNSAFE_CSS = `${DIFF_SURFACE_THEME_UNSAFE_CSS} :is( [data-line], [data-line-annotation], @@ -98,101 +99,35 @@ const DIFF_VIEW_UNSAFE_CSS = `${DIFF_SURFACE_THEME_UNSAFE_CSS} box-shadow: inset 3px 0 color-mix(in srgb, var(--code-foreground) 24%, transparent); } +/* Hunk separators are the way to the unchanged lines between hunks. They keep the library's + layout, with the expand button in its own cell, but sit one step off the code surface so they + read as quiet controls; hover brings them forward. */ :is([data-separator="line-info"], [data-separator="line-info-basic"]) { + /* Exact and margin-free: the virtualizer places rows from hunkSeparatorHeight below. */ height: 24px !important; margin-block: 0 !important; - background-color: var(--code-background) !important; + --diffs-bg-separator: color-mix(in srgb, var(--code-background) 97%, var(--code-foreground)); } -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-separator-wrapper] { - padding-inline: 8px 12px !important; - background-color: transparent !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-separator-content] { - gap: 8px; - padding-inline: 0 !important; - background-color: transparent !important; - color: color-mix(in srgb, var(--code-foreground) 52%, var(--code-background)) !important; +:is([data-separator="line-info"], [data-separator="line-info-basic"]) [data-separator-content] { + color: color-mix(in srgb, var(--code-foreground) 50%, var(--code-background)) !important; font-family: var(--font-sans) !important; font-size: 11px !important; text-decoration: none !important; } -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines] { - display: flex !important; - min-width: 0; - flex: 1 1 auto; - align-items: center; - gap: 8px; +:is([data-separator="line-info"], [data-separator="line-info-basic"]) [data-expand-button] { + color: color-mix(in srgb, var(--code-foreground) 50%, var(--code-background)) !important; } :is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] - [data-unmodified-lines] { - cursor: pointer; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines]::before, -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines]::after { - width: auto; - height: 1px; - flex: 1 1 auto; - content: ""; - background-color: color-mix(in srgb, var(--code-background) 92%, var(--code-foreground)); -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] - [data-separator-wrapper] { - grid-template-columns: 0 minmax(0, 1fr) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] - [data-separator-content] { - grid-column: 2 !important; -} - -/* Visually hidden rather than display: none so the expand action stays keyboard-reachable. */ -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-expand-button] { - position: absolute !important; - width: 1px !important; - height: 1px !important; - margin: -1px !important; - padding: 0 !important; - overflow: hidden !important; - clip-path: inset(50%) !important; - border: 0 !important; - white-space: nowrap !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ) - [data-separator-content] { - cursor: pointer; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):is(:hover, :focus-within) - [data-separator-content] { - color: color-mix(in srgb, var(--code-foreground) 76%, var(--code-background)) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):is(:hover, :focus-within) - [data-unmodified-lines]::before, -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):is(:hover, :focus-within) - [data-unmodified-lines]::after { - background-color: color-mix(in srgb, var(--code-background) 84%, var(--code-foreground)); + :is([data-separator-content], [data-expand-button]):hover { + color: var(--code-foreground) !important; + background-color: color-mix( + in srgb, + var(--code-background) 92%, + var(--code-foreground) + ) !important; } [data-diffs-header] [data-header-content] { diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 4b2a878a5ca556596c37ba63810ed20818f8d341..25a8c10ad6b21e2ff098ac781103f750b1890ed6 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -81,7 +81,12 @@ import { import { toastManager } from "../ui/toast"; import { Toggle, ToggleGroup } from "../ui/toggle-group"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; -import { PendingReviewCommentCard, ReviewThreadCard } from "./PullRequestReviewAnnotation"; +import { + getReviewPositionAnchor, + PendingReviewCommentCard, + ReviewThreadCard, + toViewerSide, +} from "./PullRequestReviewAnnotation"; import { isFileDiffCollapsed, isLineInFileDiff, @@ -162,28 +167,6 @@ export interface PullRequestAgentSelectionInput { readonly request: string; } -/** The contract's sides named the way the diff viewer names them, and back again. */ -function toViewerSide(side: PullRequestDiffSide) { - return side === "left" ? ("deletions" as const) : ("additions" as const); -} - -function getReviewPositionAnchor(position: PullRequestReviewPosition): { - line: number; - side: PullRequestDiffSide; -} { - switch (position.kind) { - case "added": - return { line: position.newLine, side: "right" }; - case "deleted": - return { line: position.oldLine, side: "left" }; - case "context": - return { - line: position.side === "left" ? position.oldLine : position.newLine, - side: position.side, - }; - } -} - /** * Whether the viewer draws this line at all. A line counts the new file on the right and the old * one on the left, and each hunk covers one run of each; a line outside every run — a @@ -208,6 +191,7 @@ function PullRequestCodeTab({ onAddToAgentSelection, onRefresh, refreshToken = 0, + revealRequest = null, }: { scrollerRef: (node: HTMLDivElement | null) => void; environmentId: EnvironmentId; @@ -225,6 +209,8 @@ function PullRequestCodeTab({ onRefresh: () => void; /** Bumped by the panel's refresh button: drop the accumulated pages and re-read the diff. */ refreshToken?: number; + /** A file another tab asked to show. A new nonce asks again for the same path. */ + revealRequest?: { readonly path: string; readonly nonce: number } | null; }) { const { resolvedTheme } = useTheme(); const settings = useClientSettings(); @@ -694,6 +680,19 @@ function PullRequestCodeTab({ }, [items, requestTreeReveal, toggleFile], ); + // Answered once the requested file has arrived: a large diff may still be paging when the + // request comes in, so it waits for the slice that carries the path rather than dropping it. + const answeredReveal = useRef(null); + useEffect(() => { + if (revealRequest === null || answeredReveal.current === revealRequest.nonce) return; + if (!items.some((item) => resolveFileDiffPath(item.fileDiff) === revealRequest.path)) { + // The reader is not scrolling toward it, so the sentinel would never ask for its slice. + if (canLoadNextSlice) loadNextSlice(); + return; + } + answeredReveal.current = revealRequest.nonce; + revealFile(revealRequest.path); + }, [canLoadNextSlice, items, loadNextSlice, revealFile, revealRequest]); const toggleAllFiles = () => { // Held as an override of the default rather than as the file keys on screen: a diff that is diff --git a/apps/web/src/components/pullRequest/PullRequestComposer.tsx b/apps/web/src/components/pullRequest/PullRequestComposer.tsx index f341f475e366d5a80f52a0eede6d0fa8272199d4..af0d8618539a9ba1e58b1d54e9852b2f178813f9 100644 --- a/apps/web/src/components/pullRequest/PullRequestComposer.tsx +++ b/apps/web/src/components/pullRequest/PullRequestComposer.tsx @@ -32,6 +32,7 @@ export function PullRequestComposer({ onCommentAction, onCommented, onReviewSubmitted, + openReviewRequest, }: { environmentId: EnvironmentId; reference: PullRequestRef; @@ -43,6 +44,11 @@ export function PullRequestComposer({ ) => Promise<{ readonly commentPosted: boolean }>; onCommented: () => void; onReviewSubmitted: () => void; + /** + * Opens the composer on the review, each time the number changes. The guide's last stop asks + * for it, so finishing the tour lands on the comments it collected and the verdict. + */ + openReviewRequest?: number | undefined; }) { const [open, setOpen] = useState(false); const [reviewPending, setReviewPending] = useState(false); @@ -58,6 +64,12 @@ export function PullRequestComposer({ (store) => (store.summaries[reviewKey] ?? "").trim().length > 0, ); const reviewStarted = pendingComments.length > 0 || summaryStarted; + const [answeredOpenRequest, setAnsweredOpenRequest] = useState(openReviewRequest); + if (openReviewRequest !== answeredOpenRequest) { + setAnsweredOpenRequest(openReviewRequest); + setRequestedMode("review"); + setOpen(true); + } // What is offered is the intersection of two different questions: what this host can do at // all, and what this account may do on this repository. Either one saying no means a control diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx index 176009e1a98a5e13acb31c68369acf270fe0270c..f16bbd0aa58c7ffc8fda6ef6e95b5e5a03d7000f 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx @@ -50,7 +50,13 @@ vi.mock("~/lib/sourceControlActions", () => ({ vi.mock("~/state/use-atom-command", () => ({ useAtomCommand: () => vi.fn() })); vi.mock("~/state/pullRequests", async (importOriginal) => ({ ...(await importOriginal()), - pullRequestEnvironment: { detail: () => "detail", activity: () => "activity" }, + pullRequestEnvironment: { + detail: () => "detail", + activity: () => "activity", + // The header menu reads the stored guide to label "Write guide" or "Rewrite guide". + reviewGuide: () => "review-guide", + generateReviewGuide: { permissionAtom: () => "generate-review-guide" }, + }, usePullRequestTurnRefresh: () => 0, useSharedPullRequestSummary: () => null, })); @@ -124,6 +130,8 @@ vi.mock("./PullRequestSummaryTab", () => ({ ), })); +// The guide renders diffs, which start the diff worker; this suite never opens it. +vi.mock("./PullRequestGuideTab", () => ({ PullRequestGuideTab: () => null })); vi.mock("./PullRequestCodeTab", () => ({ default: ({ onAddToAgentSelection, diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 526a1ae9bfbca3ea63fbc050a7d59969b22bf9fd..d68983937f19b903604055f0af5f79b5a9246af4 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -2,7 +2,11 @@ import { parseChangeRequestUrl } from "@t3tools/shared/changeRequestUrl"; import { useAtomValue } from "@effect/atom-react"; import { usePullRequestStack } from "~/state/usePullRequestStack"; import { RefreshIcon } from "~/components/ui/refresh-icon"; -import { scopedThreadKey, scopeProjectRef } from "@t3tools/client-runtime/environment"; +import { + scopedThreadKey, + scopeProjectRef, + scopeThreadRef, +} from "@t3tools/client-runtime/environment"; import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; import { AuthOrchestrationOperateScope, @@ -13,6 +17,7 @@ import { type PullRequestListEntry, type PullRequestUpdateMethod, type PullRequestRef, + type PullRequestReviewGuideStop, resolveEnvironmentMachineKind, type ScopedThreadRef, } from "@t3tools/contracts"; @@ -37,6 +42,7 @@ import { PanelRightIcon, PlayIcon, RotateCcwIcon, + SparklesIcon, TriangleAlertIcon, } from "lucide-react"; import { @@ -54,6 +60,8 @@ import { } from "react"; import { type DraftId, useComposerDraftStore } from "~/composerDraftStore"; +import { useRightPanelStore } from "~/rightPanelStore"; +import { useEnvironmentSettings } from "~/hooks/useSettings"; import { useNewThreadHandler } from "~/hooks/useHandleNewThread"; import { useCopyToClipboard } from "~/hooks/useCopyToClipboard"; import { isCommandPaletteOpen } from "~/commandPaletteBus"; @@ -127,6 +135,7 @@ import { PullRequestSummaryTab } from "./PullRequestSummaryTab"; import { PullRequestTimelineTab } from "./PullRequestTimelineTab"; import { buildAddSelectionToAgentHandoff, + buildAskAboutGuideStopHandoff, buildAskAboutPullRequestHandoff, buildExplainPullRequestHandoff, buildFixFindingHandoff, @@ -175,9 +184,12 @@ import { summarizePullRequestChecks, } from "./pullRequestPresentation"; import { PullRequestGlyph } from "./pullRequestIcons"; +import { PullRequestGuideTab } from "./PullRequestGuideTab"; +import { guideRunKey, useGuideRunStore } from "./guide/guideRunStore"; +import { useReviewGuideWriter } from "./guide/useReviewGuideWriter"; import { useInlineActionConfirm } from "./useInlineActionConfirm"; -type DetailTab = "summary" | "timeline" | "code"; +type DetailTab = "summary" | "guide" | "timeline" | "code"; /** Said as the thing that did not happen, rather than as the operation that returned an error. */ const ACTION_FAILURE_LABELS: Record = { @@ -228,6 +240,7 @@ const UPDATE_BRANCH_REBASE_FAILURE_HINT = const TABS: ReadonlyArray<{ value: DetailTab; label: string }> = [ { value: "summary", label: "Summary" }, + { value: "guide", label: "Guide" }, { value: "timeline", label: "Timeline" }, { value: "code", label: "Code" }, ]; @@ -406,6 +419,7 @@ export function PullRequestDetailPanel({ composerDraftTarget, onBack, onSelectPullRequest, + onGuideShown, }: { environmentId: EnvironmentId; shortcutsEnabled: boolean; @@ -453,6 +467,8 @@ export function PullRequestDetailPanel({ * expects to land on it, with this one still open behind. */ onBack?: (() => void) | undefined; + /** Told when a written guide comes on screen. The page maximizes its panel for the guide. */ + onGuideShown?: (() => void) | undefined; }) { const environmentConfigs = useServerConfigs(); const projects = useProjects(); @@ -484,6 +500,8 @@ export function PullRequestDetailPanel({ ? listEntry : null; const [threadPickerOpen, setThreadPickerOpen] = useState(false); + // Counted up by the guide's last stop to open the composer on the review. + const [openReviewRequest, setOpenReviewRequest] = useState(0); const [tab, setTab] = useState("summary"); const [timelineOrder, setTimelineOrder] = useState<"newest" | "oldest">("newest"); const [codeCommitScope, setCodeCommitScope] = useState<{ @@ -499,6 +517,25 @@ export function PullRequestDetailPanel({ selectCodeCommit(oid); setTab("code"); }; + // The guide names files; following one shows it in the whole diff, not a single commit's. + const [codeRevealRequest, setCodeRevealRequest] = useState<{ + readonly path: string; + readonly nonce: number; + } | null>(null); + const openFileInCode = (path: string) => { + selectCodeCommit(null); + setCodeRevealRequest((previous) => ({ path, nonce: (previous?.nonce ?? 0) + 1 })); + void loadCodeTab(); + setTab("code"); + }; + // A guide notice asks for its pull request's Guide tab once the page has opened it. + const guideOpenRequested = useGuideRunStore( + (state) => state.openRequestKey === guideRunKey(environmentId, reference), + ); + if (guideOpenRequested && tab !== "guide") setTab("guide"); + useEffect(() => { + if (guideOpenRequested) useGuideRunStore.getState().requestOpen(null); + }, [guideOpenRequested]); // Every tab the reader has opened stays mounted behind the active one. The diff viewer // always needed this (it virtualizes against its own scroll position); the trace showed the // summary needs it too — a large description re-parses its whole markdown on every return @@ -1070,6 +1107,17 @@ export function PullRequestDetailPanel({ const canPrepareWorktree = prepareThread.isAllowed && canOperateThread; const canFixFindings = attachTarget !== null || canPrepareWorktree; const handoffLabels = pullRequestHandoffLabels(attachTarget !== null); + // The guide is written from the menu as well as its tab, so a reader can ask for a rewrite + // without the tour carrying a control for it. + const guideWriter = useReviewGuideWriter({ + environmentId, + reference, + title: detail?.title ?? null, + }); + const writeGuide = () => { + void guideWriter.write(); + if (guideWriter.data?.guide == null) setTab("guide"); + }; const writeTaskToComposer = (target: ScopedThreadRef | DraftId, task: ThreadTask) => { const store = useComposerDraftStore.getState(); @@ -1136,8 +1184,16 @@ export function PullRequestDetailPanel({ return session; }; - /** A question about the change, which needs a thread and nothing else. */ - const startAsk = async (kind: string, task: ThreadTask) => { + /** + * A question about the change, which needs a thread and nothing else. `keepGuide` keeps the + * reader's place: the new thread opens with this pull request beside it on the Guide tab, at the + * stop they asked from, so the answer and the tour can be read together. + */ + const startAsk = async ( + kind: string, + task: ThreadTask, + options?: { readonly keepGuide?: boolean }, + ) => { if (!detail || handoff !== null) return; if (attachTarget !== null) { writeTaskToComposer(attachTarget, task); @@ -1155,6 +1211,22 @@ export function PullRequestDetailPanel({ const projectRef = scopeProjectRef(actingEnvironmentId, acting?.projectId ?? detail.projectId); const opened = await openThreadWithTask(projectRef, task); setHandoff(null); + if (opened !== null && options?.keepGuide === true) { + const draft = useComposerDraftStore.getState().getDraftSession(opened.draftId); + if (draft !== null) { + useRightPanelStore + .getState() + .openPullRequest(scopeThreadRef(draft.environmentId, draft.threadId), { + environmentId, + projectId: reference.projectId, + ...(reference.host === undefined ? {} : { host: reference.host }), + repository: reference.repository, + number: reference.number, + url: detail.url, + }); + useGuideRunStore.getState().requestOpen(guideRunKey(environmentId, reference)); + } + } if (opened === null) { toastManager.add({ type: "error", @@ -1353,7 +1425,35 @@ export function PullRequestDetailPanel({ }); }; - const addSelectionToAgent = (selection: PullRequestAgentSelectionInput) => { + const askAboutGuideStop = (stop: PullRequestReviewGuideStop, index: number, question: string) => { + if (!detail) return; + void startAsk( + `guide-stop:${index}`, + buildAskAboutGuideStopHandoff({ + number: detail.number, + title: detail.title, + url: detail.url, + headBranch: detail.headBranch, + baseBranch: detail.baseBranch, + state: detail.state, + isDraft: detail.isDraft, + stop: { + index, + title: stop.title, + summary: stop.summary, + attention: stop.attention?.note ?? null, + excerpts: stop.excerpts, + }, + question, + }), + { keepGuide: true }, + ); + }; + + const addSelectionToAgent = ( + selection: PullRequestAgentSelectionInput, + options?: { readonly keepGuide?: boolean }, + ) => { if (!detail) return; void startAsk( `selection:${selection.comment.id}`, @@ -1368,6 +1468,7 @@ export function PullRequestDetailPanel({ comment: selection.comment, request: selection.request, }), + options, ); }; @@ -1453,8 +1554,15 @@ export function PullRequestDetailPanel({ // A host that cannot produce a patch has no Code tab to open. While detail is loading the ghost // uses this optimistic tab set to reserve the same chrome; a host without a patch removes Code // when its capabilities arrive. + // The Guide tab is a beta the pull request's server opts into in Settings → Source Control. + const reviewGuidesEnabled = useEnvironmentSettings( + environmentId, + (settings) => settings.reviewGuidesEnabled, + ); const visibleTabs = TABS.filter( - (item) => item.value !== "code" || detail === null || detail.capabilities.diff, + (item) => + (item.value !== "code" || detail === null || detail.capabilities.diff) && + (item.value !== "guide" || reviewGuidesEnabled), ); // The Code tab can be opened while the detail is still on its way, and the detail may then say // this host has no patch to show. The tab goes, so whoever was standing on it is moved back to @@ -2117,6 +2225,16 @@ export function PullRequestDetailPanel({ {handoff === "findings" ? "Preparing..." : handoffLabels.fixFindings} + {reviewGuidesEnabled && guideWriter.canWrite ? ( + + + {guideWriter.running + ? "Writing guide..." + : guideWriter.data?.guide == null + ? "Write guide" + : "Rewrite guide"} + + ) : null} {pickableEnvironments.length > 0 ? ( ) : null} + {mountedTabs.has("guide") ? ( +
+ + addSelectionToAgent({ comment, request }, { keepGuide: true }) + } + onAskAboutStop={askAboutGuideStop} + onFinishReview={ + detail.capabilities.review.verdicts.some((verdict) => + detail.viewerPermissions.verdicts.includes(verdict), + ) + ? () => setOpenReviewRequest((count) => count + 1) + : null + } + onOpenFile={openFileInCode} + onTourShown={onGuideShown} + /> +
+ ) : null} {mountedTabs.has("timeline") ? (
{activityPending ? ( @@ -2861,6 +3006,7 @@ export function PullRequestDetailPanel({ {...(canFixFindings ? { onFixFinding: startFixFinding } : {})} onRefresh={refreshDetail} refreshToken={codeRefreshToken} + revealRequest={codeRevealRequest} />
@@ -2887,6 +3033,7 @@ export function PullRequestDetailPanel({ onCommentAction={performCommentAction} onCommented={refreshDetail} onReviewSubmitted={refreshDetail} + openReviewRequest={openReviewRequest} /> ) : null} diff --git a/apps/web/src/components/pullRequest/PullRequestGuideTab.tsx b/apps/web/src/components/pullRequest/PullRequestGuideTab.tsx new file mode 100644 index 0000000000000000000000000000000000000000..1ecfc2d07c2871cfe3d2b87a3dd587fe356badd3 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestGuideTab.tsx @@ -0,0 +1,183 @@ +import type { EnvironmentId, PullRequestRef, PullRequestReviewGuideStop } from "@t3tools/contracts"; +import { SparklesIcon, TriangleAlertIcon } from "lucide-react"; +import { useEffect, useEffectEvent, useMemo } from "react"; + +import type { ReviewCommentContext } from "~/reviewCommentContext"; + +import { Alert, AlertAction, AlertDescription } from "../ui/alert"; +import { Button } from "../ui/button"; +import { Spinner } from "../ui/spinner"; +import type { ExcerptCommenting } from "./guide/GuideExcerpt"; +import { guideRunKey, useGuideRunStore } from "./guide/guideRunStore"; +import { GuideProseContext } from "./guide/GuideProse"; +import { GuideTour } from "./guide/GuideTour"; +import { useReviewGuideWriter } from "./guide/useReviewGuideWriter"; +import { + nextPendingReviewCommentId, + pullRequestReviewKey, + usePendingReviewComments, + usePullRequestReviewStore, +} from "./pullRequestReviewStore"; + +/** + * A maintainer's tour of the pull request: stops through the diff in reading order with the + * calls only a person can make. Generated on request and stored, so opening the tab is free. + * Rewriting a current guide lives in the header menu; the tab only asks when it has a reason. + */ +export function PullRequestGuideTab({ + environmentId, + reference, + title, + workspaceRoot, + active, + canComment, + onAskAboutLines, + onAskAboutStop, + onFinishReview, + onOpenFile, + onTourShown, +}: { + environmentId: EnvironmentId; + reference: PullRequestRef; + /** The pull request's title for the notice when a guide is ready, null until the detail loads. */ + title: string | null; + /** The checkout the guide's prose renders against, as the description does. */ + workspaceRoot: string; + /** Whether this is the tab on screen. */ + active: boolean; + /** Whether this reader may queue line comments for a review on this host. */ + canComment: boolean; + /** Opens an agent thread about lines of the change. Null where no thread can be opened. */ + onAskAboutLines: ((comment: ReviewCommentContext, request: string) => void) | null; + onAskAboutStop: + | ((stop: PullRequestReviewGuideStop, index: number, question: string) => void) + | null; + /** Opens the review composer with the comments queued so far. Null where the reader cannot review. */ + onFinishReview: (() => void) | null; + /** Shows a file in the Code tab. */ + onOpenFile: (path: string) => void; + /** Told when a written guide comes on screen; a run in progress is not one. */ + onTourShown?: (() => void) | undefined; +}) { + const { query, data, running, canWrite, write } = useReviewGuideWriter({ + environmentId, + reference, + title, + }); + const runKey = guideRunKey(environmentId, reference); + // Line comments join the review the Code tab collects, so either place shows and sends them. + const reviewKey = pullRequestReviewKey(reference); + const pending = usePendingReviewComments(reference); + const addComment = usePullRequestReviewStore((store) => store.addComment); + const removeComment = usePullRequestReviewStore((store) => store.removeComment); + const commenting = useMemo( + () => + !canComment && onAskAboutLines === null + ? null + : { + canComment, + pending, + onComment: ({ path, oldPath, position, body }) => + addComment(reviewKey, { + id: nextPendingReviewCommentId(), + path, + ...(oldPath === null ? {} : { oldPath }), + position, + body, + }), + onRemove: (commentId) => removeComment(reviewKey, commentId), + onAskAgent: onAskAboutLines, + }, + [addComment, canComment, onAskAboutLines, pending, removeComment, reviewKey], + ); + useEffect(() => { + if (!active) return; + useGuideRunStore.getState().setShown(runKey); + return () => { + if (useGuideRunStore.getState().shownKey === runKey) + useGuideRunStore.getState().setShown(null); + }; + }, [active, runKey]); + + const guide = data?.guide ?? null; + const tourShown = active && guide !== null; + const reportTourShown = useEffectEvent(() => onTourShown?.()); + useEffect(() => { + if (tourShown) reportTourShown(); + }, [tourShown]); + const runState = data?.run ?? null; + const currentHeadSha = data?.currentHeadSha ?? null; + const stale = guide !== null && currentHeadSha !== null && currentHeadSha !== guide.headSha; + // A failure is shown only while it is about the head the reader is looking at. + const failure = + runState?._tag === "Failed" && runState.headSha === currentHeadSha ? runState.detail : null; + const writeButton = (label: string, size: "sm" | "xs") => + canWrite ? ( + + ) : null; + + if (guide === null) { + if (query.isPending && data === null) { + return ( +
+ +
+ ); + } + return ( +
+

+ {running ? "Writing a review guide" : "No review guide yet"} +

+

+ {failure ?? + query.error ?? + (running + ? "An agent is reading the change. A large one takes a few minutes. You can leave; you will be told when it is ready." + : "A guide is a tour of the change: stops through the code in reading order, with the calls only a maintainer can make.")} +

+ {writeButton(failure === null ? "Write guide" : "Try again", "sm")} +
+ ); + } + + // Only a guide that needs attention says so; a current one is just the tour. + const banner = + stale || failure !== null || running ? ( + + {running ? : } + + {running + ? "Rewriting this guide. You will be told when the new one is ready." + : (failure ?? "Written before the latest push.")} + + {!running && canWrite ? {writeButton("Rewrite", "xs")} : null} + + ) : null; + + return ( + + { + const stop = guide.stops[index]; + if (stop !== undefined) onAskAboutStop(stop, index, question); + } + } + onFinish={onFinishReview} + onOpenFile={onOpenFile} + /> + + ); +} diff --git a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx index fd36435962f1cb474027d342dbcd559b54a14cf6..419dd820bbaf4e604b6d40f5b3d642b2e7358ddf 100644 --- a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx +++ b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx @@ -4,7 +4,9 @@ */ import type { EnvironmentId, + PullRequestDiffSide, PullRequestRef, + PullRequestReviewPosition, PullRequestReviewThread, PullRequestThreadCommentsResult, PullRequestThreadComment, @@ -53,6 +55,28 @@ function submitKeys(input: { }; } +/** The contract's sides named the way the diff viewer names them, and back again. */ +export function toViewerSide(side: PullRequestDiffSide) { + return side === "left" ? ("deletions" as const) : ("additions" as const); +} + +export function getReviewPositionAnchor(position: PullRequestReviewPosition): { + line: number; + side: PullRequestDiffSide; +} { + switch (position.kind) { + case "added": + return { line: position.newLine, side: "right" }; + case "deleted": + return { line: position.oldLine, side: "left" }; + case "context": + return { + line: position.side === "left" ? position.oldLine : position.newLine, + side: position.side, + }; + } +} + /** A comment waiting to be sent with the rest of the review. */ export function PendingReviewCommentCard({ comment, diff --git a/apps/web/src/components/pullRequest/guide/GuideExcerpt.tsx b/apps/web/src/components/pullRequest/guide/GuideExcerpt.tsx new file mode 100644 index 0000000000000000000000000000000000000000..5700272b4e7171639c6bb396fb1fb27b3f15f4ad --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/GuideExcerpt.tsx @@ -0,0 +1,362 @@ +import type { + DiffLineAnnotation, + FileDiffContentsLoader, + FileDiffMetadata, + SelectedLineRange, +} from "@pierre/diffs"; +import { FileDiff } from "@pierre/diffs/react"; +import type { PullRequestReviewGuideExcerpt, PullRequestReviewPosition } from "@t3tools/contracts"; +import { ArrowUpRightIcon } from "lucide-react"; +import { useCallback, useMemo, useState } from "react"; + +import { useTheme } from "~/hooks/useTheme"; +import { + getRenderablePatch, + resolveDiffThemeName, + resolveFileDiffPreviousPath, +} from "~/lib/diffRendering"; +import { PREFERRED_HIGHLIGHTER } from "~/lib/syntaxHighlighting"; +import { + buildDiffReviewComment, + resolveDiffReviewPosition, + type ReviewCommentContext, +} from "~/reviewCommentContext"; + +import { DiffCommentAnnotation } from "../../diffs/DiffCommentAnnotation"; +import { DIFF_VIEW_UNSAFE_CSS } from "../../diffs/StyledDiffCodeView"; +import { Button } from "../../ui/button"; +import { + getReviewPositionAnchor, + PendingReviewCommentCard, + toViewerSide, +} from "../PullRequestReviewAnnotation"; +import type { PendingReviewComment } from "../pullRequestReviewStore"; +import { + excerptExpansionOldContents, + excerptRows, + fileDiffRows, + rowsToPatch, +} from "./guideTour.logic"; +import { GuideNoteMarker } from "./GuideNotes"; +import { GuideProse } from "./GuideProse"; + +// Long lines scroll inside the excerpt. The renderer only shows its scrollbar on hover, which +// reads as code cut off at the edge, so the excerpt keeps it visible. +const EXCERPT_UNSAFE_CSS = `${DIFF_VIEW_UNSAFE_CSS} +[data-code]::-webkit-scrollbar-thumb { + background-color: var(--diffs-bg-context); +} +`; + +export interface ExcerptNoteMarker { + readonly line: number; + readonly markers: ReadonlyArray<{ readonly marker: string; readonly title: string }>; +} + +/** + * What a reader can do with lines of an excerpt, the same two things the Code tab offers: queue a + * comment for the review, or ask an agent about them. Null where neither is possible. + */ +export interface ExcerptCommenting { + /** Whether a comment can be queued for the review; asking an agent needs no host support. */ + readonly canComment: boolean; + /** Comments already queued for the review, placed on the lines they belong to. */ + readonly pending: ReadonlyArray; + readonly onComment: (comment: { + readonly path: string; + readonly oldPath: string | null; + readonly position: PullRequestReviewPosition; + readonly body: string; + }) => void; + readonly onRemove: (commentId: string) => void; + /** Opens an agent thread about the lines; null where no thread can be opened. */ + readonly onAskAgent: ((comment: ReviewCommentContext, request: string) => void) | null; +} + +interface ExcerptAnnotation { + readonly markers: ExcerptNoteMarker["markers"]; + readonly pending: ReadonlyArray; + readonly draft: boolean; +} + +interface ExcerptDraft { + readonly range: SelectedLineRange; + readonly position: PullRequestReviewPosition; +} + +/** + * The lines of one excerpt, drawn by the same renderer as the Code tab from a patch cut down to + * the excerpt's range, so line numbers, colors and highlighting match the full diff. The cut keeps + * the file's real line numbers, so the bars above and below it load the code around the excerpt + * from the file itself, as they do between hunks in the Code tab. + */ +export function GuideExcerpt({ + excerpt, + file, + loading, + notes, + commenting, + loadDiffFiles, + onOpenFile, +}: { + excerpt: PullRequestReviewGuideExcerpt; + /** The file's parsed diff, null while it loads or when the host withheld it. */ + file: FileDiffMetadata | null; + loading: boolean; + notes: ReadonlyArray; + commenting: ExcerptCommenting | null; + /** Loads the file's unchanged lines for the bars around the excerpt. */ + loadDiffFiles: FileDiffContentsLoader; + onOpenFile: (path: string) => void; +}) { + const [draft, setDraft] = useState(null); + const { resolvedTheme } = useTheme(); + const slice = useMemo(() => { + if (file === null) return null; + const rows = fileDiffRows(file); + const shown = excerptRows(rows, excerpt); + if (shown.length === 0) return null; + const parsed = getRenderablePatch( + rowsToPatch(excerpt.path, resolveFileDiffPreviousPath(file), shown), + `pull-request-guide-excerpt:${excerpt.path}:${excerpt.side}:${excerpt.start}-${excerpt.end}`, + ); + const fileDiff = parsed?.kind === "files" ? parsed.files[0] : undefined; + if (fileDiff === undefined) return null; + // Lines the excerpt draws, by side, so only comments that land on one are placed in it. + const drawn = { + additions: new Set(shown.flatMap((row) => (row.newLine === null ? [] : [row.newLine]))), + deletions: new Set(shown.flatMap((row) => (row.oldLine === null ? [] : [row.oldLine]))), + }; + return { fileDiff, rows: shown, shown: shown.length, total: rows.length, drawn }; + }, [excerpt, file]); + + // The excerpt expands against an old side of its own: the file's real old side differs from + // the new one wherever another hunk changed it, and the renderer needs the two to agree around + // the lines it shows. A slice across hunks has nothing consistent to expand into. + const sliceRows = slice?.rows ?? null; + const loadExcerptFiles = useMemo(() => { + if (sliceRows === null || sliceRows.some((row) => row.hunk !== sliceRows[0]?.hunk)) { + return null; + } + return async (fileDiff) => { + const loaded = await loadDiffFiles(fileDiff); + const oldContents = excerptExpansionOldContents(loaded.newFile.contents, sliceRows); + if (oldContents === null) return loaded; + return { + newFile: loaded.newFile, + oldFile: { + name: loaded.oldFile?.name ?? loaded.newFile.name, + contents: oldContents, + cacheKey: `${loaded.newFile.cacheKey ?? loaded.newFile.name}:excerpt:${excerpt.side}:${excerpt.start}-${excerpt.end}`, + }, + }; + }; + }, [excerpt.end, excerpt.side, excerpt.start, loadDiffFiles, sliceRows]); + + // Notes, queued comments and the draft share one annotation per line, as in the Code tab. + const annotations = useMemo(() => { + const groups = new Map>(); + const at = (side: "additions" | "deletions", line: number) => { + const key = `${side}:${line}`; + const existing = groups.get(key); + if (existing !== undefined) return existing; + const created: DiffLineAnnotation = { + side, + lineNumber: line, + metadata: { markers: [], pending: [], draft: false }, + }; + groups.set(key, created); + return created; + }; + const update = ( + side: "additions" | "deletions", + line: number, + change: (metadata: ExcerptAnnotation) => ExcerptAnnotation, + ) => { + const group = at(side, line); + groups.set(`${side}:${line}`, { ...group, metadata: change(group.metadata) }); + }; + const noteSide = excerpt.side === "new" ? "additions" : "deletions"; + for (const note of notes) { + update(noteSide, note.line, (metadata) => ({ + ...metadata, + markers: [...metadata.markers, ...note.markers], + })); + } + for (const comment of commenting?.pending ?? []) { + if (comment.path !== excerpt.path) continue; + const anchor = getReviewPositionAnchor(comment.position); + const side = toViewerSide(anchor.side); + if (!slice?.drawn[side].has(anchor.line)) continue; + update(side, anchor.line, (metadata) => ({ + ...metadata, + pending: [...metadata.pending, comment], + })); + } + if (draft !== null) { + const anchor = getReviewPositionAnchor(draft.position); + update(toViewerSide(anchor.side), anchor.line, (metadata) => ({ ...metadata, draft: true })); + } + return [...groups.values()]; + }, [commenting?.pending, draft, excerpt.path, excerpt.side, notes, slice]); + + // Positions and the agent's copy of the lines come from the whole file's diff, so a comment + // lands where the Code tab would put it rather than on the excerpt's cut-down patch. + const beginComment = useCallback( + (range: SelectedLineRange | null) => { + if (range === null || file === null) return; + const position = resolveDiffReviewPosition(file, range.end, range.endSide ?? range.side); + if (position === null) return; + setDraft({ range, position }); + }, + [file], + ); + const interactive = commenting !== null && draft === null; + const onAskAgent = commenting?.onAskAgent ?? null; + const askAgent = (request: string) => { + if (draft === null || file === null || onAskAgent === null) return; + const comment = buildDiffReviewComment({ + id: `pull-request-guide:${excerpt.path}:${draft.range.start}:${draft.range.end}`, + sectionId: `pull-request-guide:${excerpt.path}`, + sectionTitle: "Review guide", + filePath: excerpt.path, + fileDiff: file, + range: draft.range, + text: request, + }); + setDraft(null); + if (comment !== null) onAskAgent(comment, request); + }; + + const options = useMemo( + () => ({ + diffStyle: "unified" as const, + lineDiffType: "none" as const, + overflow: "scroll" as const, + theme: resolveDiffThemeName(resolvedTheme), + themeType: resolvedTheme, + preferredHighlighter: PREFERRED_HIGHLIGHTER, + unsafeCSS: EXCERPT_UNSAFE_CSS, + // Hover a line for the gutter button, or drag across line numbers, as in the Code tab. + enableGutterUtility: interactive, + enableLineSelection: interactive, + onGutterUtilityClick: beginComment, + onLineSelectionEnd: beginComment, + // Without a loader there is nothing to expand into, so the bars stay plain. + ...(loadExcerptFiles === null + ? { hunkSeparators: "simple" as const } + : { loadDiffFiles: loadExcerptFiles }), + }), + [resolvedTheme, interactive, beginComment, loadExcerptFiles], + ); + + return ( +
+ {excerpt.caption.trim() !== "" ? ( +
+ +
+ ) : null} + {slice !== null ? ( +
+ + fileDiff={slice.fileDiff} + options={options} + lineAnnotations={annotations} + selectedLines={draft?.range ?? null} + renderAnnotation={(annotation) => ( +
+ {annotation.metadata.markers.length > 0 ? ( +
+ {annotation.metadata.markers.map((entry) => ( + + + {entry.title} + + ))} +
+ ) : null} + {annotation.metadata.pending.map((comment) => ( + commenting?.onRemove(comment.id)} + /> + ))} + {annotation.metadata.draft && draft !== null && commenting !== null ? ( + askAgent(text), + }, + } + : {})} + onCancel={() => setDraft(null)} + onComment={(body) => { + if (!commenting.canComment) { + askAgent(body); + return; + } + commenting.onComment({ + path: excerpt.path, + oldPath: file === null ? null : previousPathOf(file, excerpt.path), + position: draft.position, + body, + }); + setDraft(null); + }} + /> + ) : null} +
+ )} + renderHeaderMetadata={() => ( + + + {excerpt.side === "old" ? "removed code, " : ""}showing {slice.shown} of{" "} + {slice.total} diff lines + + + + )} + /> +
+ ) : loading ? ( +

+ Loading {excerpt.path}... +

+ ) : ( +
+ + {excerpt.path} lines {excerpt.start}- + {excerpt.end} are not in the loaded diff. + + +
+ )} +
+ ); +} + +/** A renamed file's comment carries its old path too, which hosts need to place it. */ +function previousPathOf(file: FileDiffMetadata, path: string): string | null { + const previous = resolveFileDiffPreviousPath(file); + return previous === path ? null : previous; +} diff --git a/apps/web/src/components/pullRequest/guide/GuideFileDiff.tsx b/apps/web/src/components/pullRequest/guide/GuideFileDiff.tsx new file mode 100644 index 0000000000000000000000000000000000000000..7079a6c9e4083cc66294f3d281d3f52a6d359bc6 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/GuideFileDiff.tsx @@ -0,0 +1,50 @@ +import { + cloneFileDiffMetadata, + type FileDiffContentsLoader, + type FileDiffMetadata, +} from "@pierre/diffs"; +import { FileDiff } from "@pierre/diffs/react"; +import { useMemo } from "react"; + +import { useTheme } from "~/hooks/useTheme"; +import { resolveDiffThemeName } from "~/lib/diffRendering"; +import { PREFERRED_HIGHLIGHTER } from "~/lib/syntaxHighlighting"; + +import { DIFF_VIEW_UNSAFE_CSS } from "../../diffs/StyledDiffCodeView"; + +/** + * A whole file's diff inside a stop, for a file the guide vouched for without excerpting. The row + * above it names the file, so the renderer's own header is left off. The bars between hunks load + * the unchanged lines, as in the Code tab. + */ +export function GuideFileDiff({ + file, + loadDiffFiles, +}: { + file: FileDiffMetadata; + loadDiffFiles: FileDiffContentsLoader; +}) { + const { resolvedTheme } = useTheme(); + // Loading the unchanged lines rewrites the diff it is given; the excerpts read the same file + // as a patch, so this view expands a copy of its own. + const expandable = useMemo(() => cloneFileDiffMetadata(file), [file]); + const options = useMemo( + () => ({ + diffStyle: "unified" as const, + lineDiffType: "none" as const, + overflow: "scroll" as const, + disableFileHeader: true, + theme: resolveDiffThemeName(resolvedTheme), + themeType: resolvedTheme, + preferredHighlighter: PREFERRED_HIGHLIGHTER, + unsafeCSS: DIFF_VIEW_UNSAFE_CSS, + loadDiffFiles, + }), + [loadDiffFiles, resolvedTheme], + ); + return ( +
+ +
+ ); +} diff --git a/apps/web/src/components/pullRequest/guide/GuideNotes.tsx b/apps/web/src/components/pullRequest/guide/GuideNotes.tsx new file mode 100644 index 0000000000000000000000000000000000000000..9e1a426114461d4e37ec9036997df80d9ae23806 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/GuideNotes.tsx @@ -0,0 +1,58 @@ +import type { PullRequestReviewGuideNote } from "@t3tools/contracts"; + +import { cn } from "~/lib/utils"; + +import { GuideProse } from "./GuideProse"; + +/** The letter that ties a note to its line in an excerpt. */ +export function GuideNoteMarker({ marker }: { marker: string }) { + return ( + + {marker} + + ); +} + +/** A stop's notes, lettered in the order the excerpts mark them. */ +export function GuideNotes({ + notes, + className, +}: { + notes: ReadonlyArray<{ readonly marker: string; readonly note: PullRequestReviewGuideNote }>; + className?: string; +}) { + if (notes.length === 0) return null; + return ( +
+

+ Notes on this stop + + {notes.length === 1 ? "1 note" : `${notes.length} notes`} + +

+
    + {notes.map(({ marker, note }) => ( +
  1. + +
    +

    + {note.title} + + {note.path.split("/").at(-1)}:{note.line} + +

    + {note.body.trim() !== "" ? ( +
    + +
    + ) : null} +
    +
    2. + ))} +
+
+ ); +} diff --git a/apps/web/src/components/pullRequest/guide/GuideProse.tsx b/apps/web/src/components/pullRequest/guide/GuideProse.tsx new file mode 100644 index 0000000000000000000000000000000000000000..7b222753b4c6277e2503e1a1f062dec3e25ef9bc --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/GuideProse.tsx @@ -0,0 +1,40 @@ +import type { EnvironmentId } from "@t3tools/contracts"; +import { createContext, useContext } from "react"; + +import { cn } from "~/lib/utils"; + +import { PullRequestMarkdown } from "../PullRequestMarkdown"; + +/** Where the guide's prose renders from: the pull request's environment and checkout. */ +export const GuideProseContext = createContext<{ + readonly environmentId: EnvironmentId; + readonly cwd: string; +} | null>(null); + +/** + * The guide's sentences, rendered as the pull request's own description is: inline code, and + * `#123` linked to that pull request or issue, opening in the pull request panel like every other + * pull request link in the app. Plain text inside a control, such as a question that opens a + * thread, since a link cannot nest in a button. + */ +export function GuideProse({ + text, + plain = false, + className, +}: { + text: string; + plain?: boolean; + className?: string; +}) { + const context = useContext(GuideProseContext); + if (plain || context === null) return text; + return ( + + ); +} diff --git a/apps/web/src/components/pullRequest/guide/GuideRail.tsx b/apps/web/src/components/pullRequest/guide/GuideRail.tsx new file mode 100644 index 0000000000000000000000000000000000000000..01c17652b9647a5b91922ba55d1676ef2b8e42d5 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/GuideRail.tsx @@ -0,0 +1,108 @@ +import type { PullRequestReviewGuideStop } from "@t3tools/contracts"; +import { CheckIcon } from "lucide-react"; + +import { cn } from "~/lib/utils"; + +import { Badge } from "../../ui/badge"; +import { stopSubline } from "./guideTour.logic"; + +/** + * The tour's table of contents: where the reader is and which stops they have read past. A stop + * with a call to make is marked; that is the only sign of where to slow down. + */ +export function GuideRail({ + stops, + position, + seen, + onSelect, +}: { + stops: ReadonlyArray; + position: number; + /** Stops the reader has moved on from. */ + seen: ReadonlySet; + onSelect: (position: number) => void; +}) { + const reviewed = seen.size; + const renderStop = (stop: PullRequestReviewGuideStop, index: number) => ( +
  • + onSelect(index)} + lead={} + title={stop.title} + detail={ + <> + {stop.attention !== null ? ( + + Look closely + + ) : null} + {stopSubline(stop)} + + } + /> +
    • + ); + + return ( + + ); +} + +/** A semantic row of the tour's list rather than a control styled as a button. */ +function RailRow({ + active, + onClick, + lead, + title, + detail, +}: { + active: boolean; + onClick: () => void; + lead: React.ReactNode; + title: string; + detail: React.ReactNode; +}) { + return ( + + ); +} + +function StopBullet({ number, seen }: { number: number; seen: boolean }) { + return ( + + {seen ? : number} + + ); +} diff --git a/apps/web/src/components/pullRequest/guide/GuideStop.tsx b/apps/web/src/components/pullRequest/guide/GuideStop.tsx new file mode 100644 index 0000000000000000000000000000000000000000..2b6f207f8fdbcd4e84b2ae5edb32b0ed25981584 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/GuideStop.tsx @@ -0,0 +1,310 @@ +import type { FileDiffContentsLoader, FileDiffMetadata } from "@pierre/diffs"; +import type { + PullRequestReviewGuideAttention, + PullRequestReviewGuideStop, +} from "@t3tools/contracts"; +import { + ArrowRightIcon, + ChevronDownIcon, + ChevronRightIcon, + ChevronUpIcon, + EyeIcon, + FileIcon, + MessageCircleQuestionIcon, + MessageSquareIcon, +} from "lucide-react"; +import { useMemo, type ReactNode } from "react"; + +import { getDiffLineStat } from "~/lib/diffRendering"; +import { cn } from "~/lib/utils"; + +import { DiffWorkerPoolProvider } from "../../DiffWorkerPoolProvider"; +import { Alert, AlertDescription, AlertTitle } from "../../ui/alert"; +import { Button } from "../../ui/button"; +import { Kbd } from "../../ui/kbd"; +import { PullRequestDiffStat } from "../pullRequestPresentation"; +import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../../ui/collapsible"; +import { GuideExcerpt, type ExcerptCommenting } from "./GuideExcerpt"; +import { GuideFileDiff } from "./GuideFileDiff"; +import { GuideNotes } from "./GuideNotes"; +import { GuideProse } from "./GuideProse"; +import { letterStopNotes } from "./guideTour.logic"; + +const ATTENTION_LABELS = { + tradeoff: "A tradeoff to weigh", + contract: "A contract to check", + product: "A product call", + risk: "A risk to judge", + accessibility: "An accessibility call", +} as const satisfies Record; + +/** + * One stop of the tour: what it is about, where to look, and the two things a reader does with + * it: comment on its lines for the review, or ask an agent about it. + */ +export function GuideStop({ + stop, + index, + stopCount, + railOpen, + onRailOpenChange, + stopList, + nextTitle, + files, + loadDiffFiles, + diffLoading, + commenting, + onStep, + onAsk, + onFinish, + onOpenFile, +}: { + stop: PullRequestReviewGuideStop; + index: number; + stopCount: number; + /** Whether the stop list is folded open under the header, on a narrow panel. */ + railOpen: boolean; + onRailOpenChange: (open: boolean) => void; + /** The tour's stop list, folded under the header on a narrow panel. */ + stopList: ReactNode; + /** The following stop's title, null on the last stop. */ + nextTitle: string | null; + files: ReadonlyMap; + /** Loads a file's unchanged lines for the bars between hunks. */ + loadDiffFiles: FileDiffContentsLoader; + diffLoading: boolean; + /** Commenting on excerpt lines; null where the reader can neither comment nor ask. */ + commenting: ExcerptCommenting | null; + onStep: (delta: 1 | -1) => void; + /** Opens an agent thread about this stop, with the question prefilled or empty. */ + onAsk: ((question: string) => void) | null; + /** Opens the review composer, on the last stop. Null where the reader cannot review. */ + onFinish: (() => void) | null; + onOpenFile: (path: string) => void; +}) { + const { lettered, byExcerpt } = useMemo(() => letterStopNotes(stop), [stop]); + const excerptPaths = new Set(stop.excerpts.map((excerpt) => excerpt.path)); + // Files the stop owns without excerpting, and excerpted ones listed again with a note. + const otherFiles = stop.files.filter((file) => !excerptPaths.has(file.path)); + + const hasNotes = lettered.length > 0; + + return ( + // On a maximized panel the notes take a sticky column of their own beside the stop; a stop + // without notes keeps the whole width. Narrower, they follow the excerpts. +
    +
    + {/* Narrow, the stop list folds open under this line like an accordion, so one place + says where the reader is. Wide, the list sits beside the stop and this is a label. */} + +
    +

    + Stop {index + 1} of {stopCount} +

    + + } + > + + Stop {index + 1} of {stopCount} + + + +
    + + + + J + K + +
    +
    + +
    {stopList}
    +     +     + +
    +

    {stop.title}

    + {stop.summary.trim() !== "" ? ( +
    + +
    + ) : null} +
    + + {stop.attention !== null ? ( + // A call for the reader to make, not a warning, so it reads as a plain notice. + + + {ATTENTION_LABELS[stop.attention.kind]} + +
    + +
    +     +     + ) : null} + + {stop.excerpts.length > 0 ? ( +
    + + {stop.excerpts.map((excerpt, excerptIndex) => ( + + ))} + +
    + ) : null} + + + {otherFiles.length > 0 ? ( +
    +

    + {stop.excerpts.length > 0 ? "Also in this stop" : "Files in this stop"} +

    + +
      + {otherFiles.map((file) => { + const diff = files.get(file.path); + const stat = diff === undefined ? null : getDiffLineStat([diff]); + return ( +
    • + + + + + + {file.path} + + {file.note !== undefined && file.note.trim() !== "" ? ( + + + + ) : null} + {stat !== null ? ( + + ) : null} + + + {diff !== undefined ? ( + + ) : ( +

      + {diffLoading + ? "Loading the diff..." + : "This file is not in the loaded diff."} +

      + )} +       +       +
      • + ); + })} +
    +     +
    + ) : null} + + {stop.questions.length > 0 || onAsk !== null ? ( +
    + {stop.questions.length > 0 ? ( +

    Questions to consider

    + ) : null} +
    + {stop.questions.map((question) => + onAsk === null ? ( +
    + +
    + ) : ( + // A question is a sentence, so it wraps rather than truncating as a button would. + + ), + )} + {onAsk !== null ? ( + + ) : null} +
    +
    + ) : null} + +
    + {nextTitle !== null ? ( + + ) : onFinish !== null ? ( + + ) : null} +
    +
    + {hasNotes ? ( + + ) : null} +
    + ); +} diff --git a/apps/web/src/components/pullRequest/guide/GuideTour.tsx b/apps/web/src/components/pullRequest/guide/GuideTour.tsx new file mode 100644 index 0000000000000000000000000000000000000000..36effffabb2a7e4e27aeb6298e814358c77c8349 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/GuideTour.tsx @@ -0,0 +1,174 @@ +import type { EnvironmentId, PullRequestRef, PullRequestReviewGuide } from "@t3tools/contracts"; +import { useEffect, useEffectEvent, useMemo, useRef, useState, type ReactNode } from "react"; + +import { createPullRequestDiffFileContentsLoader } from "~/lib/diffFileContents"; +import { isEditableFocused } from "~/lib/editableFocus"; +import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useAtomCommand } from "~/state/use-atom-command"; + +import { isCommandPaletteOpen } from "~/commandPaletteBus"; + +import { GuideRail } from "./GuideRail"; +import { GuideStop } from "./GuideStop"; +import type { ExcerptCommenting } from "./GuideExcerpt"; +import { + guideProgressKey, + guideRunKey, + useGuideProgress, + useGuideRunStore, + type GuideProgress, +} from "./guideRunStore"; +import { stepTourPosition, stopFilePaths } from "./guideTour.logic"; +import { useGuideDiffFiles } from "./useGuideDiffFiles"; + +/** + * The guide as a tour: a rail of stops beside the stop being read. Progress is kept per guide + * head for as long as the app is open, so the same guide in another panel opens where the reader + * is; a new head starts it over. What + * the reader says about the change goes into the review: line comments queue with the Code tab's, + * and the last stop opens the composer that sends them. + */ +export function GuideTour({ + environmentId, + reference, + guide, + banner, + active, + commenting, + onAsk, + onFinish, + onOpenFile, +}: { + environmentId: EnvironmentId; + reference: PullRequestRef; + guide: PullRequestReviewGuide; + /** The stale or failed notice, shown above whichever view is open. */ + banner: ReactNode; + /** Whether the tour is the tab on screen, which is when J and K step through it. */ + active: boolean; + commenting: ExcerptCommenting | null; + /** Opens an agent thread about a stop. Null where the reader cannot start one. */ + onAsk: ((stop: number, question: string) => void) | null; + onFinish: (() => void) | null; + onOpenFile: (path: string) => void; +}) { + const key = guideProgressKey(guideRunKey(environmentId, reference), guide.headSha); + const tour = useGuideProgress(key); + const update = (change: (current: GuideProgress) => Partial) => + useGuideRunStore.getState().setProgress(key, (current) => ({ ...current, ...change(current) })); + const [railOpen, setRailOpen] = useState(false); + + const stopCount = guide.stops.length; + const position = Math.max(Math.min(tour.position, stopCount - 1), 0); + const stop = guide.stops[position] ?? null; + + // Only the open stop's files are read: its excerpts, and the files it lists to open in place. + const wantedPaths = useMemo(() => (stop === null ? [] : stopFilePaths(stop)), [stop]); + const diff = useGuideDiffFiles({ + environmentId, + reference, + headSha: guide.headSha, + wantedPaths, + }); + // The same file reads the Code tab uses, so the bars between hunks load what lies between. + const getDiffFileContents = useAtomCommand(pullRequestEnvironment.diffFileContents); + const loadDiffFiles = useMemo( + () => + createPullRequestDiffFileContentsLoader(getDiffFileContents, { + environmentId, + reference, + commit: null, + cacheKey: `pull-request-guide:${key}`, + }), + [environmentId, getDiffFileContents, key, reference], + ); + + // A new stop starts at its top rather than wherever the last one was scrolled to. + const scrollRef = useRef(null); + const goTo = (next: number) => { + // Leaving a stop for a later one is reading it; jumping back is not. + update((current) => ({ + position: next, + seen: next > position ? new Set(current.seen).add(position) : current.seen, + })); + setRailOpen(false); + scrollRef.current?.scrollTo({ top: 0 }); + }; + const step = (delta: 1 | -1) => goTo(stepTourPosition(position, delta, stopCount)); + + // J and K step through the tour whenever it is the tab on screen, wherever focus is, unless + // the reader is typing: clicking a stop in the rail leaves focus on a button outside any + // element that could hold a key handler of its own. + const stepFromKey = useEffectEvent((event: KeyboardEvent) => { + if (event.defaultPrevented || event.isComposing || isCommandPaletteOpen()) return; + if (event.metaKey || event.ctrlKey || event.altKey || event.shiftKey) return; + if (event.key !== "j" && event.key !== "k") return; + if (isEditableFocused(event.target)) return; + event.preventDefault(); + step(event.key === "j" ? 1 : -1); + }); + useEffect(() => { + if (!active) return; + const onKeyDown = (event: KeyboardEvent) => stepFromKey(event); + document.addEventListener("keydown", onKeyDown); + return () => document.removeEventListener("keydown", onKeyDown); + }, [active]); + + return ( + // Three widths: one column with the rail folded; the rail beside the stop; and, on a + // maximized panel, the stop's notes in a column of their own (see GuideStop). +
    +
    + {/* Beside the stop on a wide panel; narrow, the stop folds the same list open under its + header instead. */} + +
    + {banner} + {diff.error !== null ? ( +

    + The diff could not be loaded, so excerpts are missing: {diff.error} +

    + ) : null} + {stop === null ? ( +

    This change has no files to tour.

    + ) : ( + + } + nextTitle={guide.stops[position + 1]?.title ?? null} + files={diff.files} + loadDiffFiles={loadDiffFiles} + diffLoading={diff.loading} + commenting={commenting} + onStep={step} + onAsk={onAsk === null ? null : (question) => onAsk(position, question)} + onFinish={ + onFinish === null + ? null + : () => { + update((current) => ({ seen: new Set(current.seen).add(position) })); + onFinish(); + } + } + onOpenFile={onOpenFile} + /> + )} +
    +
    +
    + ); +} diff --git a/apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx b/apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx new file mode 100644 index 0000000000000000000000000000000000000000..a4b054215b915fea88072961ddf8b86db64e846b --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx @@ -0,0 +1,140 @@ +import { useNavigate } from "@tanstack/react-router"; +import { CircleAlertIcon, CircleCheckIcon } from "lucide-react"; +import { useEffect, useRef } from "react"; + +import { getClientSettings } from "~/hooks/useSettings"; +import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useEnvironmentQuery } from "~/state/query"; +import { + hasDesktopNotifications, + hasNotificationSound, + playNotificationSound, +} from "~/threadNotifications"; + +import { toastManager } from "../../ui/toast"; +import { readPullRequestListPreferences } from "../pullRequestListPreferences"; +import { useGuideRunStore, type GuideRunWatch } from "./guideRunStore"; + +/** How often a watched run is re-read. A run takes minutes, not seconds. */ +const RUN_POLL_MS = 5_000; +/** A run never seen running after this long was dropped before the first read. */ +const UNSEEN_RUN_MS = 60_000; + +/** + * Watches the guide runs this tab is waiting on and says when each ends, so the reader can leave + * the Guide tab, or the pull request page, while one is written. + */ +export function PullRequestGuideRunCoordinator() { + const watches = useGuideRunStore((state) => state.watches); + return Object.entries(watches).map(([key, watch]) => ( + + )); +} + +function GuideRunWatcher({ runKey, watch }: { runKey: string; watch: GuideRunWatch }) { + const query = useEnvironmentQuery( + pullRequestEnvironment.reviewGuide({ + environmentId: watch.environmentId, + input: watch.reference, + }), + ); + const navigate = useNavigate(); + const refresh = query.refresh; + useEffect(() => { + const timer = setInterval(refresh, RUN_POLL_MS); + return () => clearInterval(timer); + }, [refresh]); + + const data = query.data; + const sawRunning = useRef(false); + useEffect(() => { + if (data === null) return; + if (data.run?._tag === "Running") { + sawRunning.current = true; + return; + } + const failure = + data.run?._tag === "Failed" && data.run.failedAt !== watch.previousFailedAt + ? data.run.detail + : null; + const written = + failure === null && data.guide !== null && data.guide.generatedAt !== watch.previousGuideAt; + // A read from before the run started says nothing yet; a run the server dropped, say on a + // restart, ends with nothing to say. + if (failure === null && !written) { + if (sawRunning.current || Date.now() - watch.since > UNSEEN_RUN_MS) { + useGuideRunStore.getState().settle(runKey); + } + return; + } + const store = useGuideRunStore.getState(); + store.settle(runKey); + const looking = document.visibilityState === "visible" && document.hasFocus(); + if (looking && store.shownKey === runKey) return; + + const title = failure === null ? "Review guide ready" : "Review guide failed"; + const body = watch.title ?? `${watch.reference.repository} #${watch.reference.number}`; + const open = () => { + useGuideRunStore.getState().requestOpen(runKey); + void navigate({ + to: "/pull-requests", + search: { + ...readPullRequestListPreferences(), + repository: watch.reference.repository, + number: watch.reference.number, + selectedProjectId: watch.reference.projectId, + selectedEnvironmentId: watch.environmentId, + ...(watch.reference.host === undefined ? {} : { selectedHost: watch.reference.host }), + }, + }); + }; + const mode = getClientSettings().notificationMode; + if (hasNotificationSound(mode)) { + void playNotificationSound(failure === null ? "completion" : "input", () => + hasNotificationSound(getClientSettings().notificationMode), + ); + } + if (looking) { + const toastId = toastManager.add({ + type: failure === null ? "success" : "error", + title, + description: failure === null ? body : `${body}: ${failure}`, + data: { + hideCopyButton: true, + leadingIcon: + failure === null ? ( + + ) : ( + + ), + }, + actionProps: { + children: "Open guide", + onClick: () => { + toastManager.close(toastId); + open(); + }, + }, + }); + return; + } + if ( + !hasDesktopNotifications(mode) || + typeof Notification === "undefined" || + Notification.permission !== "granted" + ) + return; + try { + const notification = new Notification(title, { body, tag: `guide:${runKey}`, silent: true }); + notification.addEventListener("click", () => { + notification.close(); + window.focus(); + open(); + }); + } catch { + // Some browsers expose Notification but reject desktop presentation. + } + }, [data, navigate, runKey, watch]); + + return null; +} diff --git a/apps/web/src/components/pullRequest/guide/guideRunStore.ts b/apps/web/src/components/pullRequest/guide/guideRunStore.ts new file mode 100644 index 0000000000000000000000000000000000000000..58bcaf9abecf2a03b2ea14b912832b78c8da7cc3 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/guideRunStore.ts @@ -0,0 +1,86 @@ +/** + * Guide runs this tab is waiting on, so the reader can leave and hear when one is done, and where + * the reader is in each guide. + * + * A run takes minutes and finishes on the server. `PullRequestGuideRunCoordinator` watches every + * run listed here, wherever the reader is in the app, and says when it ends. Progress is shared so + * a guide opened beside a new thread picks up at the same stop. Not persisted: a reload forgets + * both, and the guide is still there when the reader comes back. + */ +import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; +import { create } from "zustand"; + +import { pullRequestReviewKey } from "../pullRequestReviewStore"; + +export interface GuideRunWatch { + readonly environmentId: EnvironmentId; + readonly reference: PullRequestRef; + /** The pull request's title, for the notice. */ + readonly title: string | null; + /** The guide and failure on record when the wait began; a different one means the run ended. */ + readonly previousGuideAt: string | null; + readonly previousFailedAt: string | null; + readonly since: number; +} + +/** Where a reader is in one guide, kept per guide head so any panel showing it agrees. */ +export interface GuideProgress { + readonly position: number; + /** Stops the reader has moved on from, which the rail checks off. */ + readonly seen: ReadonlySet; +} + +const NO_PROGRESS: GuideProgress = { position: 0, seen: new Set() }; + +interface GuideRunStoreState { + readonly watches: Readonly>; + /** The guide on screen right now, which needs no notice when it lands. */ + readonly shownKey: string | null; + /** A guide a notice asked to open; its panel switches to the Guide tab and clears this. */ + readonly openRequestKey: string | null; + /** Reading progress by guide, keyed by `guideProgressKey`. */ + readonly progress: Readonly>; + readonly setProgress: (key: string, change: (current: GuideProgress) => GuideProgress) => void; + readonly watch: (watch: GuideRunWatch) => void; + readonly settle: (key: string) => void; + readonly setShown: (key: string | null) => void; + readonly requestOpen: (key: string | null) => void; +} + +export function guideRunKey(environmentId: EnvironmentId, reference: PullRequestRef): string { + return `${environmentId}:${pullRequestReviewKey(reference)}`; +} + +export const useGuideRunStore = create()((set) => ({ + watches: {}, + shownKey: null, + openRequestKey: null, + progress: {}, + setProgress: (key, change) => + set((state) => ({ + progress: { ...state.progress, [key]: change(state.progress[key] ?? NO_PROGRESS) }, + })), + watch: (watch) => + set((state) => { + const key = guideRunKey(watch.environmentId, watch.reference); + if (state.watches[key] !== undefined) return state; + return { watches: { ...state.watches, [key]: watch } }; + }), + settle: (key) => + set((state) => { + if (state.watches[key] === undefined) return state; + const { [key]: _settled, ...watches } = state.watches; + return { watches }; + }), + setShown: (shownKey) => set({ shownKey }), + requestOpen: (openRequestKey) => set({ openRequestKey }), +})); + +/** One guide's progress: a new head is a new tour, since its stops are not the ones read. */ +export function guideProgressKey(runKey: string, headSha: string): string { + return `${runKey}:${headSha}`; +} + +export function useGuideProgress(key: string): GuideProgress { + return useGuideRunStore((state) => state.progress[key] ?? NO_PROGRESS); +} diff --git a/apps/web/src/components/pullRequest/guide/guideTour.logic.test.ts b/apps/web/src/components/pullRequest/guide/guideTour.logic.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..78e6451310f74dcb23f790fed6dfd982ad97cc95 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/guideTour.logic.test.ts @@ -0,0 +1,178 @@ +import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles"; +import type { PullRequestReviewGuideStop } from "@t3tools/contracts"; +import { describe, expect, it } from "vite-plus/test"; + +import { + excerptExpansionOldContents, + excerptRows, + fileDiffRows, + letterStopNotes, + rowsToPatch, + stepTourPosition, +} from "./guideTour.logic"; + +const PATCH = `diff --git a/src/tray.ts b/src/tray.ts +--- a/src/tray.ts ++++ b/src/tray.ts +@@ -10,5 +10,6 @@ function tray() + const a = 1; +-const size = pick(); ++const size = "xs"; ++const variant = "tray"; + const b = 2; + const c = 3; + const d = 4; +@@ -40,3 +41,2 @@ function footer() + keep(); +-dropped(); + tail(); +`; + +function parse(patch: string) { + const file = parsePatchFiles(patch, `test:${patch.length}`)[0]?.files[0]; + if (file === undefined) throw new Error("patch did not parse"); + return file; +} + +const rows = fileDiffRows(parse(PATCH)); + +function stop(overrides: Partial): PullRequestReviewGuideStop { + return { + title: "Stop", + summary: "", + attention: null, + excerpts: [], + files: [], + notes: [], + questions: [], + ...overrides, + }; +} + +describe("stepTourPosition", () => { + it("holds the tour to its first and last stops", () => { + expect(stepTourPosition(0, -1, 3)).toBe(0); + expect(stepTourPosition(0, 1, 3)).toBe(1); + expect(stepTourPosition(2, 1, 3)).toBe(2); + }); +}); + +describe("fileDiffRows", () => { + it("numbers both sides through context, deletions and additions", () => { + expect(rows.map((row) => [row.kind, row.oldLine, row.newLine])).toEqual([ + ["context", 10, 10], + ["deletion", 11, null], + ["addition", null, 11], + ["addition", null, 12], + ["context", 12, 13], + ["context", 13, 14], + ["context", 14, 15], + ["context", 40, 41], + ["deletion", 41, null], + ["context", 42, 42], + ]); + }); +}); + +describe("excerptRows", () => { + it("brings the replaced lines along with an excerpt that starts on an addition", () => { + const shown = excerptRows(rows, { side: "new", start: 11, end: 12 }); + expect(shown.map((row) => row.kind)).toEqual(["deletion", "addition", "addition"]); + }); + + it("selects old-side lines for a removal and stops at the hunk boundary", () => { + const shown = excerptRows(rows, { side: "old", start: 41, end: 41 }); + expect(shown.map((row) => [row.kind, row.oldLine])).toEqual([["deletion", 41]]); + }); + + it("returns nothing for a range outside the diff", () => { + expect(excerptRows(rows, { side: "new", start: 100, end: 120 })).toEqual([]); + }); +}); + +describe("rowsToPatch", () => { + it("cuts a slice that parses back to the same lines and numbers", () => { + const shown = excerptRows(rows, { side: "new", start: 10, end: 13 }); + const slice = parse(rowsToPatch("src/tray.ts", "src/tray.ts", shown)); + expect(fileDiffRows(slice).map((row) => [row.kind, row.oldLine, row.newLine])).toEqual( + shown.map((row) => [row.kind, row.oldLine, row.newLine]), + ); + }); + + it("starts a hunk per source hunk so separate hunks stay separate", () => { + const shown = excerptRows(rows, { side: "old", start: 14, end: 42 }); + const patch = rowsToPatch("src/tray.ts", "src/tray.ts", shown); + expect(patch.match(/^@@ /gm)).toHaveLength(2); + expect(patch).toContain("@@ -14,1 +15,1 @@"); + expect(patch).toContain("@@ -40,3 +41,2 @@"); + }); +}); + +describe("excerptExpansionOldContents", () => { + // The new file behind PATCH: lines 1-9, the hunk at 10-15, then lines up to 41-42 around the + // second hunk, which removed a line. + const newFile = Array.from({ length: 50 }, (_, index) => `line ${index + 1}\n`); + newFile.splice( + 9, + 6, + "const a = 1;\n", + 'const size = "xs";\n', + 'const variant = "tray";\n', + "const b = 2;\n", + "const c = 3;\n", + "const d = 4;\n", + ); + newFile.splice(40, 2, "keep();\n", "tail();\n"); + const newContents = newFile.join(""); + + it("puts back only the excerpt's lines and keeps both sides numbered as in the file", () => { + const shown = excerptRows(rows, { side: "new", start: 11, end: 12 }); + const old = excerptExpansionOldContents(newContents, shown); + if (old === null) throw new Error("expected contents"); + const oldLines = old.split("\n"); + // The excerpt's old side sits at its real old line. + expect(oldLines[10]).toBe("const size = pick();"); + // Around it, the new file's lines, so the two sides agree outside the excerpt. + expect(oldLines[9]).toBe("const a = 1;"); + expect(oldLines[11]).toBe("const b = 2;"); + // One line replaced by two: the old side is a line shorter, everything after lines up. + expect(oldLines.length).toBe(newContents.split("\n").length - 1); + }); + + it("does not try to expand an excerpt that spans hunks", () => { + const shown = excerptRows(rows, { side: "old", start: 14, end: 42 }); + expect(excerptExpansionOldContents(newContents, shown)).toBeNull(); + }); +}); + +describe("letterStopNotes", () => { + it("letters notes in guide order and pins each to the excerpt that holds its line", () => { + const { lettered, byExcerpt } = letterStopNotes( + stop({ + excerpts: [ + { path: "a.ts", side: "new", start: 1, end: 5, caption: "" }, + { path: "b.ts", side: "new", start: 10, end: 12, caption: "" }, + ], + notes: [ + { path: "b.ts", line: 11, title: "First", body: "" }, + { path: "a.ts", line: 2, title: "Second", body: "" }, + { path: "a.ts", line: 2, title: "Third", body: "" }, + { path: "c.ts", line: 1, title: "Unanchored", body: "" }, + ], + }), + ); + expect(lettered.map((entry) => entry.marker)).toEqual(["A", "B", "C", "D"]); + expect(byExcerpt).toEqual([ + [ + { + line: 2, + markers: [ + { marker: "B", title: "Second" }, + { marker: "C", title: "Third" }, + ], + }, + ], + [{ line: 11, markers: [{ marker: "A", title: "First" }] }], + ]); + }); +}); diff --git a/apps/web/src/components/pullRequest/guide/guideTour.logic.ts b/apps/web/src/components/pullRequest/guide/guideTour.logic.ts new file mode 100644 index 0000000000000000000000000000000000000000..653aae14549a19b86f73d4ca1d544a3765f2e0d7 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/guideTour.logic.ts @@ -0,0 +1,224 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; +import type { PullRequestReviewGuideExcerpt, PullRequestReviewGuideStop } from "@t3tools/contracts"; + +/** One step forward or back, held to the tour's first and last stops. */ +export function stepTourPosition(current: number, delta: 1 | -1, stopCount: number): number { + return Math.max(Math.min(current + delta, stopCount - 1), 0); +} + +/** The stop's files in reading order: the excerpted ones first, then the rest it owns. */ +export function stopFilePaths(stop: PullRequestReviewGuideStop): ReadonlyArray { + return [ + ...new Set([ + ...stop.excerpts.map((excerpt) => excerpt.path), + ...stop.files.map((file) => file.path), + ]), + ]; +} + +/** The rail's second line for a stop. */ +export function stopSubline(stop: PullRequestReviewGuideStop): string { + const count = stopFilePaths(stop).length; + if (count === 0) return "Context, no code"; + return count === 1 ? "1 file" : `${count} files`; +} + +/** A note's marker: A, B, C… in the order the guide lists them, numbers past Z. */ +function noteMarker(index: number): string { + return index < 26 ? String.fromCharCode(65 + index) : String(index + 1); +} + +/** One rendered line of a file's diff, with where it sits on each side. */ +export interface DiffRow { + readonly kind: "context" | "addition" | "deletion"; + /** Line number on the old side, null for an added line. */ + readonly oldLine: number | null; + /** Line number on the new side, null for a removed line. */ + readonly newLine: number | null; + /** The next line number on each side when this row is reached, for hunk headers. */ + readonly oldAt: number; + readonly newAt: number; + readonly text: string; + /** Which hunk of the file the row came from; rows from two hunks are not adjacent. */ + readonly hunk: number; +} + +/** Every line of a parsed file diff as the unified view lists it. */ +export function fileDiffRows(file: FileDiffMetadata): ReadonlyArray { + const rows: DiffRow[] = []; + file.hunks.forEach((hunk, hunkIndex) => { + let oldAt = hunk.deletionStart; + let newAt = hunk.additionStart; + for (const content of hunk.hunkContent) { + if (content.type === "context") { + for (let offset = 0; offset < content.lines; offset += 1) { + rows.push({ + kind: "context", + oldLine: oldAt, + newLine: newAt, + oldAt, + newAt, + text: file.additionLines[content.additionLineIndex + offset] ?? "", + hunk: hunkIndex, + }); + oldAt += 1; + newAt += 1; + } + continue; + } + for (let offset = 0; offset < content.deletions; offset += 1) { + rows.push({ + kind: "deletion", + oldLine: oldAt, + newLine: null, + oldAt, + newAt, + text: file.deletionLines[content.deletionLineIndex + offset] ?? "", + hunk: hunkIndex, + }); + oldAt += 1; + } + for (let offset = 0; offset < content.additions; offset += 1) { + rows.push({ + kind: "addition", + oldLine: null, + newLine: newAt, + oldAt, + newAt, + text: file.additionLines[content.additionLineIndex + offset] ?? "", + hunk: hunkIndex, + }); + newAt += 1; + } + } + }); + return rows; +} + +/** + * The rows an excerpt covers. Its range is in one side's line numbers, so the other side's half + * of a replacement at either edge comes along: an excerpt starting on an added line also shows + * the lines it replaced, which is what makes it read as a change rather than as new code. + */ +export function excerptRows( + rows: ReadonlyArray, + excerpt: Pick, +): ReadonlyArray { + const lineOf = (row: DiffRow) => (excerpt.side === "new" ? row.newLine : row.oldLine); + const inRange = (row: DiffRow) => { + const line = lineOf(row); + return line !== null && line >= excerpt.start && line <= excerpt.end; + }; + let first = rows.findIndex(inRange); + if (first === -1) return []; + let last = rows.findLastIndex(inRange); + const sameHunk = (index: number, anchor: number) => rows[index]?.hunk === rows[anchor]?.hunk; + if (excerpt.side === "new" && rows[first]?.kind === "addition") { + while (first > 0 && rows[first - 1]?.kind === "deletion" && sameHunk(first - 1, first)) { + first -= 1; + } + } + if (excerpt.side === "old" && rows[last]?.kind === "deletion") { + while ( + last < rows.length - 1 && + rows[last + 1]?.kind === "addition" && + sameHunk(last + 1, last) + ) { + last += 1; + } + } + return rows.slice(first, last + 1); +} + +const ROW_PREFIX = { context: " ", addition: "+", deletion: "-" } as const; + +/** + * A unified patch of just these rows, so the shared diff renderer can draw a slice of a file + * with its real line numbers instead of a second diff view being written for excerpts. + */ +export function rowsToPatch( + path: string, + previousPath: string, + rows: ReadonlyArray, +): string { + if (rows.length === 0) return ""; + const lines = [ + `diff --git a/${previousPath} b/${path}`, + `--- a/${previousPath}`, + `+++ b/${path}`, + ]; + let start = 0; + while (start < rows.length) { + let end = start; + while (end + 1 < rows.length && rows[end + 1]?.hunk === rows[start]?.hunk) end += 1; + const group = rows.slice(start, end + 1); + const head = group[0]!; + const oldCount = group.filter((row) => row.oldLine !== null).length; + const newCount = group.filter((row) => row.newLine !== null).length; + // Git names the line before an empty side, so a pure insertion after line 9 is `-9,0`. + const oldStart = oldCount === 0 ? Math.max(head.oldAt - 1, 0) : head.oldAt; + const newStart = newCount === 0 ? Math.max(head.newAt - 1, 0) : head.newAt; + lines.push(`@@ -${oldStart},${oldCount} +${newStart},${newCount} @@`); + for (const row of group) lines.push(`${ROW_PREFIX[row.kind]}${row.text.replace(/\r?\n$/, "")}`); + start = end + 1; + } + return `${lines.join("\n")}\n`; +} + +/** + * The old side an excerpt expands against: the new file with only the excerpt's lines put back + * as they were. The renderer fills the code around a hunk from both files and needs them to + * agree outside it, which the real old file does not wherever another hunk changed lines. Only + * the lines the reader can expand into are read, and those are the new file's, so the code + * around the excerpt shows as it reads after the change. Null when the excerpt spans hunks. + */ +export function excerptExpansionOldContents( + newContents: string, + rows: ReadonlyArray, +): string | null { + const head = rows[0]; + if (head === undefined || rows.some((row) => row.hunk !== head.hunk)) return null; + const newLines = newContents.match(/[^\n]*\n|[^\n]+$/g) ?? []; + const before = head.newAt - 1; + const newCount = rows.filter((row) => row.newLine !== null).length; + // The old side keeps its own numbering, so its prefix is as long as the old file's: trimmed + // from the front, or padded there with lines no expansion reaches. + const oldBefore = head.oldAt - 1; + const prefix = newLines.slice(0, before); + const oldPrefix = + oldBefore >= prefix.length + ? [...Array.from({ length: oldBefore - prefix.length }, () => "\n"), ...prefix] + : prefix.slice(prefix.length - oldBefore); + const oldRows = rows + .filter((row) => row.oldLine !== null) + .map((row) => (row.text.endsWith("\n") ? row.text : `${row.text}\n`)); + return [...oldPrefix, ...oldRows, ...newLines.slice(before + newCount)].join(""); +} + +/** A stop's notes lettered in guide order, with the excerpt lines each one marks. */ +export interface LetteredNote { + readonly marker: string; + readonly note: Note; +} + +/** + * Letters a stop's notes and groups them by the excerpt line they sit on. A note outside every + * excerpt keeps its letter in the list; it just has no line to mark. + */ +export function letterStopNotes(stop: PullRequestReviewGuideStop) { + const lettered = stop.notes.map((note, index) => ({ marker: noteMarker(index), note })); + const byExcerpt = stop.excerpts.map((excerpt) => { + const lines = new Map>(); + for (const { marker, note } of lettered) { + if (note.path !== excerpt.path || note.line < excerpt.start || note.line > excerpt.end) { + continue; + } + const entry = { marker, title: note.title }; + const list = lines.get(note.line); + if (list === undefined) lines.set(note.line, [entry]); + else list.push(entry); + } + return [...lines].map(([line, markers]) => ({ line, markers })); + }); + return { lettered, byExcerpt }; +} diff --git a/apps/web/src/components/pullRequest/guide/useGuideDiffFiles.ts b/apps/web/src/components/pullRequest/guide/useGuideDiffFiles.ts new file mode 100644 index 0000000000000000000000000000000000000000..99cae3606636358bfbd5c50fc27b78b1d0f8b443 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/useGuideDiffFiles.ts @@ -0,0 +1,87 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; +import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; +import { useEffect, useState } from "react"; + +import { getRenderablePatch, resolveFileDiffPath } from "~/lib/diffRendering"; +import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useEnvironmentQuery } from "~/state/query"; + +import { pullRequestReviewKey } from "../pullRequestReviewStore"; + +interface LoadedDiff { + readonly key: string; + /** The page asked for next; null is the first. */ + readonly cursor: string | null; + /** No page left to ask for. */ + readonly done: boolean; + readonly files: ReadonlyMap; +} + +const NO_FILES: ReadonlyMap = new Map(); + +/** + * The pull request's parsed file diffs, read through the same paged query the Code tab uses, so + * the pages are shared. Paging stops once every wanted path has arrived: a stop's excerpts are + * usually in the first page, and a large change should not be read whole to show one of them. + * Pages already read are kept, so going back to an earlier stop costs nothing. + */ +export function useGuideDiffFiles({ + environmentId, + reference, + headSha, + wantedPaths, +}: { + environmentId: EnvironmentId; + reference: PullRequestRef; + /** The guide's head. A push writes a new guide, whose excerpts must read the new diff. */ + headSha: string; + wantedPaths: ReadonlyArray; +}) { + const key = `${environmentId}:${pullRequestReviewKey(reference)}:${headSha}`; + const [loaded, setLoaded] = useState({ + key, + cursor: null, + done: false, + files: NO_FILES, + }); + const current: LoadedDiff = + loaded.key === key ? loaded : { key, cursor: null, done: false, files: NO_FILES }; + const missing = wantedPaths.some((path) => !current.files.has(path)); + const loading = missing && !current.done; + const query = useEnvironmentQuery( + loading + ? pullRequestEnvironment.diff({ + environmentId, + input: { ...reference, ...(current.cursor === null ? {} : { cursor: current.cursor }) }, + }) + : null, + ); + + const data = query.data; + const cursor = current.cursor; + useEffect(() => { + if (data === null) return; + setLoaded((previous) => { + const base = + previous.key === key ? previous : { key, cursor: null, done: false, files: NO_FILES }; + // A page is absorbed once; the query hands the same answer back on every render. + if (base.cursor !== cursor || base.done) return base; + const parsed = getRenderablePatch( + data.patch, + `pull-request-guide:${key}:${cursor ?? "first"}`, + ); + const files = new Map(base.files); + if (parsed?.kind === "files") { + for (const file of parsed.files) files.set(resolveFileDiffPath(file), file); + } + return { key, cursor: data.nextCursor, done: data.nextCursor === null, files }; + }); + }, [cursor, data, key]); + + return { + files: current.files, + loading: loading && query.error === null, + error: loading ? query.error : null, + retry: query.refresh, + }; +} diff --git a/apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts b/apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts new file mode 100644 index 0000000000000000000000000000000000000000..d787d8550d295fe3f2993ecc8eccd89caa9ae5a6 --- /dev/null +++ b/apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts @@ -0,0 +1,78 @@ +import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; +import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; +import { useAtomValue } from "@effect/atom-react"; +import { useEffect, useEffectEvent, useState } from "react"; + +import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useEnvironmentQuery } from "~/state/query"; +import { useAtomCommand } from "~/state/use-atom-command"; + +import { toastManager } from "../../ui/toast"; +import { readableFailure } from "../pullRequestDetail.logic"; +import { guideRunKey, useGuideRunStore } from "./guideRunStore"; + +/** + * A pull request's stored guide and the way to write a new one, shared by the Guide tab and the + * header menu. Starting a run hands it to the run coordinator, which re-reads it until it ends + * and says so if the reader is not looking at the guide. + */ +export function useReviewGuideWriter({ + environmentId, + reference, + title, +}: { + environmentId: EnvironmentId; + reference: PullRequestRef; + /** The pull request's title for the notice when the guide is ready. */ + title: string | null; +}) { + const query = useEnvironmentQuery( + pullRequestEnvironment.reviewGuide({ environmentId, input: reference }), + ); + const canWrite = useAtomValue( + pullRequestEnvironment.generateReviewGuide.permissionAtom(environmentId), + ); + const generate = useAtomCommand(pullRequestEnvironment.generateReviewGuide, { + reportFailure: false, + }); + const [starting, setStarting] = useState(false); + const data = query.data; + const running = starting || data?.run?._tag === "Running"; + + const watchRun = useEffectEvent(() => + useGuideRunStore.getState().watch({ + environmentId, + reference, + title, + previousGuideAt: data?.guide?.generatedAt ?? null, + previousFailedAt: data?.run?._tag === "Failed" ? data.run.failedAt : null, + since: Date.now(), + }), + ); + // A run started elsewhere, or before a reload, is watched as soon as it is seen. + useEffect(() => { + if (data?.run?._tag === "Running") watchRun(); + }, [data?.run?._tag]); + + const write = async () => { + if (running) return; + setStarting(true); + watchRun(); + const result = await generate({ environmentId, input: reference }); + setStarting(false); + if (result._tag === "Failure") { + // Nothing started, so there is nothing to watch for and nothing more to announce. + useGuideRunStore.getState().settle(guideRunKey(environmentId, reference)); + toastManager.add({ + type: "error", + title: "Could not start a review guide", + description: readableFailure( + squashAtomCommandFailure(result), + "Check the review guide model in Settings → Source Control.", + ), + }); + } + }; + + return { query, data, running, canWrite, write }; +} diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 7242eac8695a04b11bcf96f655058fe2f5cf818b..f3054b6e9734b9729c196fa7983cc27fbd03522f 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -19,6 +19,7 @@ import { buildMessageContext, reviewCommentContextReference } from "~/lib/compos import { buildAddSelectionToAgentHandoff, + buildAskAboutGuideStopHandoff, classifyPullRequestChecks, groupPullRequestChecks, describePullRequestChecks, @@ -1222,6 +1223,54 @@ describe("asking about a change rather than working on it", () => { }); }); +describe("buildAskAboutGuideStopHandoff", () => { + const base = { + number: 42, + title: "Add search", + url: "https://github.com/acme/web/pull/42", + headBranch: "feat/search", + baseBranch: "main", + state: "open" as const, + isDraft: false, + }; + const stop = { + index: 1, + title: "The index is built on write", + summary: "Writes now update the index.", + attention: "A tradeoff: writes get slower.", + excerpts: [ + { path: "src/index.ts", side: "new" as const, start: 10, end: 20 }, + { path: "src/old.ts", side: "old" as const, start: 3, end: 5 }, + ], + }; + + it("carries the stop beside the pull request and asks only for an answer", () => { + const handoff = buildAskAboutGuideStopHandoff({ ...base, stop, question: "Why on write?" }); + expect(handoff.prompt).toBe("Why on write?"); + expect(handoff.reviewComments.map((entry) => entry.filePath)).toEqual([ + "PR #42", + "Guide stop 2", + ]); + expect(handoff.reviewComments[0]?.text).toContain("Do not change any code"); + const chip = handoff.reviewComments[1]?.text ?? ""; + expect(chip).toContain("Writes now update the index."); + expect(chip).toContain("A tradeoff: writes get slower."); + expect(chip).toContain("- src/index.ts lines 10-20\n"); + expect(chip).toContain("- src/old.ts lines 3-5 (removed; numbered as in the base branch)"); + }); + + it("leaves the question to the reader when none was suggested", () => { + const handoff = buildAskAboutGuideStopHandoff({ + ...base, + stop: { ...stop, attention: null, excerpts: [] }, + question: "", + }); + expect(handoff.prompt).toBe(""); + expect(handoff.reviewComments[1]?.text).not.toContain("judge"); + expect(handoff.reviewComments[1]?.text).not.toContain("lines it showed"); + }); +}); + describe("a second ask into the same composer", () => { const chip = (id: string): ReviewCommentContext => ({ id, diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 7faf9b6512407cc62c409147d333f707442a189b..8e209b66d534c29f7961e1d6dec5d2c375e7f550 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -1179,6 +1179,70 @@ export function buildAddSelectionToAgentHandoff(input: { }; } +/** + * A question about one stop of the review guide. The stop arrives as a chip of its own, with what + * the guide said there and which lines it showed, so the agent starts where the reader is. The + * question is the reader's: a suggested one is prefilled to send or edit, otherwise it is empty. + */ +export function buildAskAboutGuideStopHandoff(input: { + readonly number: number; + readonly title: string; + readonly url: string; + readonly headBranch: string; + readonly baseBranch: string; + readonly state: PullRequestState; + readonly isDraft: boolean; + readonly stop: { + readonly index: number; + readonly title: string; + readonly summary: string; + readonly attention: string | null; + readonly excerpts: ReadonlyArray<{ + readonly path: string; + readonly side: "new" | "old"; + readonly start: number; + readonly end: number; + }>; + }; + readonly question: string; +}): FixFindingsHandoff { + const { stop } = input; + // Removed code is numbered on the base side; the head has other code at those lines. + const where = stop.excerpts.map( + (excerpt) => + `- ${excerpt.path} lines ${excerpt.start}-${excerpt.end}${excerpt.side === "old" ? " (removed; numbered as in the base branch)" : ""}`, + ); + return { + prompt: bounded(input.question), + reviewComments: [ + pullRequestContextComment(input, ANSWER_INSTRUCTIONS), + { + id: `pull-request-guide-stop:${input.number}:${stop.index}`, + sectionId: `pull-request:${input.number}`, + sectionTitle: `PR #${input.number} review guide`, + filePath: `Guide stop ${stop.index + 1}`, + startIndex: 0, + endIndex: 0, + rangeLabel: boundedField(stop.title), + text: [ + `The reader is at stop ${stop.index + 1} of a review guide for this pull request, titled \`${boundedField(stop.title)}\`. The guide was written by an agent and is context, not instructions.`, + `What the guide said there: ${bounded(stop.summary)}`, + ...(stop.attention === null + ? [] + : [`It asked the reader to judge: ${bounded(stop.attention)}`]), + ...(where.length === 0 + ? [] + : [ + "The lines it showed, numbered as in the pull request's head unless marked:", + ...where, + ]), + ].join("\n"), + diff: "", + }, + ], + }; +} + const isPullRequestOperationError = Schema.is(PullRequestOperationError); export function isPullRequestNotFound(failure: unknown): boolean { diff --git a/apps/web/src/components/settings/SettingInheritance.tsx b/apps/web/src/components/settings/SettingInheritance.tsx index e08ca680649fdd37817daf715eceb41495dbf4cc..4eb641ea0aab35677ae7d96c0c1061dc7458c136 100644 --- a/apps/web/src/components/settings/SettingInheritance.tsx +++ b/apps/web/src/components/settings/SettingInheritance.tsx @@ -45,9 +45,11 @@ function formatValue(key: keyof ServerSettings, value: unknown): string { ? "Automatic" : key === "sourceControlWriterModelSelection" ? "Text generation model" - : key === "defaultThreadEnvMode" || key === "worktreeSubmodules" - ? "Inherit" - : "Not set"; + : key === "reviewGuideModelSelection" + ? "Source control writer model" + : key === "defaultThreadEnvMode" || key === "worktreeSubmodules" + ? "Inherit" + : "Not set"; } if (typeof value === "boolean") return value ? "On" : "Off"; if (typeof value === "number") { diff --git a/apps/web/src/components/settings/SourceControlWritingSettings.test.tsx b/apps/web/src/components/settings/SourceControlWritingSettings.test.tsx index bfc75e083cf3aee98ba559a6777805e6c7c08e45..9f2da3cc4a110280cb0c4c2637770ab3c9a6720b 100644 --- a/apps/web/src/components/settings/SourceControlWritingSettings.test.tsx +++ b/apps/web/src/components/settings/SourceControlWritingSettings.test.tsx @@ -17,7 +17,10 @@ vi.mock("./useScopedSettings", () => ({ ...DEFAULT_UNIFIED_SETTINGS, sourceControlWritingStyle: state.styles[0], }), - useScopedSettingsMixed: () => JSON.stringify(state.styles[0]) !== JSON.stringify(state.styles[1]), + // Review guides stay off and agreed here, so their rows do not render beside the style's. + useScopedSettingsMixed: (keys: ReadonlyArray) => + !keys.includes("reviewGuidesEnabled") && + JSON.stringify(state.styles[0]) !== JSON.stringify(state.styles[1]), useUpdateScopedSettings: () => state.updateSettings, })); vi.mock("./SettingsScopeContext", () => ({ diff --git a/apps/web/src/components/settings/SourceControlWritingSettings.tsx b/apps/web/src/components/settings/SourceControlWritingSettings.tsx index 455fae63951b44b88dd03ac0be3c02cd339b1cae..37dcc5981172b40569d3541a7e68569f96dc9f75 100644 --- a/apps/web/src/components/settings/SourceControlWritingSettings.tsx +++ b/apps/web/src/components/settings/SourceControlWritingSettings.tsx @@ -7,7 +7,10 @@ import type { } from "@t3tools/contracts"; import { DEFAULT_UNIFIED_SETTINGS } from "@t3tools/contracts/settings"; import { createModelSelection } from "@t3tools/shared/model"; -import { resolveSourceControlWriterModelSelection } from "@t3tools/shared/serverSettings"; +import { + resolveReviewGuideModelSelection, + resolveSourceControlWriterModelSelection, +} from "@t3tools/shared/serverSettings"; import { useScopedSettings, @@ -61,6 +64,7 @@ const MODE_OPTIONS: Record `${target.environmentId}:${target.projectId}`).join(","); const customInstructionsRef = useRef(null); const [editingAllInstructions, setEditingAllInstructions] = useState(false); const [allInstructions, setAllInstructions] = useState(null); @@ -124,6 +132,24 @@ export function SourceControlWritingSettingsSection() { activeSelection.model, ); const writerModelDisabledReason = useScopedModelDisabledReason(settings, instanceEntries); + const usesReviewGuideModel = settings.reviewGuideModelSelection !== null; + const reviewGuideSelection = resolveAppModelSelectionState( + { + ...settings, + textGenerationModelSelection: resolveReviewGuideModelSelection( + settings, + textGenerationProviders, + ), + }, + textGenerationProviders, + ); + // Its own list: a custom model chosen for guides is not among the writer model's options. + const reviewGuideModelOptionsByInstance = getCustomModelOptionsByInstance( + settings, + textGenerationProviders, + reviewGuideSelection.instanceId, + reviewGuideSelection.model, + ); return ( @@ -357,6 +383,145 @@ export function SourceControlWritingSettingsSection() { ) } /> + + {/* A beta the server owns: off hides the Guide tab in every client and refuses agents. */} + updateSettings({ reviewGuidesEnabled: Boolean(checked) })} + aria-label="Review guides" + /> + } + /> + + {settings.reviewGuidesEnabled || mixedReviewGuidesEnabled ? ( + <> + + Connect an environment to choose its review guide model. + + ) : ( +
    + {(usesReviewGuideModel || mixedReviewGuideModel) && canEnableDedicatedModel ? ( + { + const reason = writerModelDisabledReason(instanceId, model); + if (reason) { + toastManager.add({ + type: "error", + title: "Review guide model not saved", + description: reason, + }); + return; + } + updateSettings({ + reviewGuideModelSelection: createModelSelection(instanceId, model), + }); + }} + /> + ) : null} + { + if (!checked) { + updateSettings({ reviewGuideModelSelection: null }); + return; + } + // Every selected target gets this model, so it must run on all of them. + const reason = writerModelDisabledReason( + activeSelection.instanceId, + activeSelection.model, + ); + if (reason) { + toastManager.add({ + type: "error", + title: "Review guide model not saved", + description: reason, + }); + return; + } + updateSettings({ + reviewGuideModelSelection: createModelSelection( + activeSelection.instanceId, + activeSelection.model, + activeSelection.options, + ), + }); + }} + aria-label="Use a separate review guide model" + /> +
    + ) + } + /> + + updateSettings({ reviewGuideInstructions: "" })} + /> + ) : null + } + > +
    +