From e7b29f7bfcc1648127ab61024e4c79ba3cee3ca6 Mon Sep 17 00:00:00 2001 From: Graeme Folk Date: Thu, 10 Sep 2026 12:03:03 -0600 Subject: [PATCH 1/2] refactor(review): move review policy behind VCS providers chore: sync guide viewer manifest fix(review): validate provider diff ids before narrowing --- apps/hook/server/index.ts | 39 +++--- apps/opencode-plugin/commands.ts | 20 +-- apps/pi-extension/plannotator-browser.ts | 31 +++-- apps/pi-extension/review-args-parity.test.ts | 3 +- apps/pi-extension/server.ts | 2 + apps/pi-extension/server/serverReview.ts | 3 +- apps/pi-extension/server/vcs.ts | 25 +++- apps/pi-extension/vendor.sh | 2 +- packages/core/config-types.ts | 9 +- packages/core/guide-viewer-manifest.ts | 4 +- packages/server/review.ts | 3 +- packages/server/vcs.test.ts | 8 +- packages/server/vcs.ts | 33 +++-- packages/shared/call-flow.test.ts | 8 +- packages/shared/config.test.ts | 21 ++-- packages/shared/config.ts | 23 ++-- packages/shared/git-review-policy.ts | 88 +++++++++++++ packages/shared/gitbutler-core.test.ts | 14 ++- packages/shared/gitbutler-review-policy.ts | 28 +++++ packages/shared/jj-review-policy.ts | 41 ++++++ packages/shared/jj-snapshot.test.ts | 3 +- packages/shared/p4-review-policy.ts | 27 ++++ packages/shared/package.json | 5 + packages/shared/review-args.test.ts | 23 +--- packages/shared/review-args.ts | 35 +----- packages/shared/review-open-state.test.ts | 21 ++-- packages/shared/review-open-state.ts | 97 ++++---------- packages/shared/vcs-core.test.ts | 16 ++- packages/shared/vcs-core.ts | 126 +++++++++---------- packages/shared/vcs-review-policy.test.ts | 46 +++++++ packages/shared/vcs-review-policy.ts | 33 +++++ packages/ui/config/index.ts | 1 + packages/ui/config/reviewView.ts | 11 ++ packages/ui/config/settings.ts | 34 ++++- 34 files changed, 588 insertions(+), 295 deletions(-) create mode 100644 packages/shared/git-review-policy.ts create mode 100644 packages/shared/gitbutler-review-policy.ts create mode 100644 packages/shared/jj-review-policy.ts create mode 100644 packages/shared/p4-review-policy.ts create mode 100644 packages/shared/vcs-review-policy.test.ts create mode 100644 packages/shared/vcs-review-policy.ts diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 03b1a06d8..5dac63d5b 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -94,8 +94,8 @@ import { startGoalSetupServer, handleGoalSetupServerReady, } from "@plannotator/server/goal-setup"; -import { type DiffType, detectManagedVcs, prepareLocalReviewDiff, gitRuntime } from "@plannotator/server/vcs"; -import { loadConfig, resolveDefaultDiffType, resolveSharingEnabled } from "@plannotator/shared/config"; +import { type DiffType, type VcsProvider, detectManagedVcs, prepareLocalReviewDiff, resolveConfiguredVcsReviewDefault, gitRuntime } from "@plannotator/server/vcs"; +import { loadConfig, resolveSharingEnabled } from "@plannotator/shared/config"; import { parseReviewArgs, type ParsedReviewArgs } from "@plannotator/shared/review-args"; import { resolveReviewOpenState, type ReviewOpenState } from "@plannotator/shared/review-open-state"; import { listBranches, type AvailableBranches } from "@plannotator/shared/review-core"; @@ -368,7 +368,7 @@ async function resolveCliReviewOpenState( options: { isPRMode: boolean; isWorkspace: boolean; - providerId?: "git" | "gitbutler" | "jj" | "p4"; + provider?: VcsProvider; resolvedDefaultDiffType: DiffType; cwd?: string; }, @@ -382,7 +382,7 @@ async function resolveCliReviewOpenState( reviewArgs.base !== undefined && !options.isPRMode && !options.isWorkspace && - options.providerId === "git" + options.provider?.id === "git" ) { // The probe is the whole point of CLI-side resolution: without it a // typo'd base produces a confidently-mislabelled merge-base→HEAD diff @@ -403,7 +403,9 @@ async function resolveCliReviewOpenState( parsed: reviewArgs, isPRMode: options.isPRMode, isWorkspace: options.isWorkspace, - providerId: options.providerId, + provider: options.provider + ? { resolve: options.provider.reviewPolicy.resolveOpenState } + : undefined, resolvedDefaultDiffType: options.resolvedDefaultDiffType, baseResolves, availableBranches, @@ -837,7 +839,7 @@ if (args[0] === "sessions") { await resolveCliReviewOpenState(reviewArgs, { isPRMode: true, isWorkspace: false, - resolvedDefaultDiffType: resolveDefaultDiffType(loadConfig()), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(loadConfig()), }); const prRef = parsePRUrl(urlArg); if (!prRef) { @@ -1096,13 +1098,14 @@ if (args[0] === "sessions") { isPRMode: false, isWorkspace: false, providerId, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + provider: managedVcs ?? undefined, + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config, providerId), }); const diffResult = await prepareLocalReviewDiff({ vcsType: reviewArgs.vcsType, - requestedDiffType: openState.requestedDiffType, + requestedDiffType: openState.requestedDiffType as DiffType | undefined, requestedBase: openState.requestedBase, - configuredDiffType: resolveDefaultDiffType(config), + configuredDiffType: resolveConfiguredVcsReviewDefault(config, providerId), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); gitContext = diffResult.gitContext; @@ -1121,10 +1124,10 @@ if (args[0] === "sessions") { await resolveCliReviewOpenState(reviewArgs, { isPRMode: false, isWorkspace: true, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config), }); workspace = await buildLocalWorkspaceReview(process.cwd(), { - configuredDiffType: resolveDefaultDiffType(config), + configuredDiffType: resolveConfiguredVcsReviewDefault(config), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); if (workspace.repos.length === 0) { @@ -1856,7 +1859,7 @@ if (args[0] === "sessions") { await resolveCliReviewOpenState(reviewArgs, { isPRMode: true, isWorkspace: false, - resolvedDefaultDiffType: resolveDefaultDiffType(loadConfig()), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(loadConfig()), }); const prRef = parsePRUrl(urlArg); if (!prRef) { @@ -1902,16 +1905,16 @@ if (args[0] === "sessions") { const openState = await resolveCliReviewOpenState(reviewArgs, { isPRMode: false, isWorkspace: false, - providerId, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + provider: managedVcs ?? undefined, + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config, providerId), cwd, }); const diffResult = await prepareLocalReviewDiff({ cwd, vcsType: reviewArgs.vcsType, - requestedDiffType: openState.requestedDiffType, + requestedDiffType: openState.requestedDiffType as DiffType | undefined, requestedBase: openState.requestedBase, - configuredDiffType: resolveDefaultDiffType(config), + configuredDiffType: resolveConfiguredVcsReviewDefault(config, providerId), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); gitContext = diffResult.gitContext; @@ -1925,11 +1928,11 @@ if (args[0] === "sessions") { await resolveCliReviewOpenState(reviewArgs, { isPRMode: false, isWorkspace: true, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config), cwd, }); workspace = await buildLocalWorkspaceReview(cwd, { - configuredDiffType: resolveDefaultDiffType(config), + configuredDiffType: resolveConfiguredVcsReviewDefault(config), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); if (workspace.repos.length === 0) { diff --git a/apps/opencode-plugin/commands.ts b/apps/opencode-plugin/commands.ts index 5952f6f35..c740bacbb 100644 --- a/apps/opencode-plugin/commands.ts +++ b/apps/opencode-plugin/commands.ts @@ -13,11 +13,11 @@ import { startAnnotateServer, handleAnnotateServerReady, } from "@plannotator/server/annotate"; -import { type DiffType, prepareLocalReviewDiff, detectManagedVcs, gitRuntime } from "@plannotator/server/vcs"; +import { type DiffType, prepareLocalReviewDiff, detectManagedVcs, resolveConfiguredVcsReviewDefault, gitRuntime } from "@plannotator/server/vcs"; import { resolveReviewOpenState } from "@plannotator/shared/review-open-state"; import { detectProjectName } from "@plannotator/server/project"; import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; -import { loadConfig, resolveDefaultDiffType, resolveUseJina } from "@plannotator/shared/config"; +import { loadConfig, resolveUseJina } from "@plannotator/shared/config"; import { composeReviewApprovedMessage, getAnnotateApprovedWithNotesPrompt, @@ -102,7 +102,7 @@ export async function handleReviewCommand( parsed: reviewArgs, isPRMode: true, isWorkspace: false, - resolvedDefaultDiffType: resolveDefaultDiffType(loadConfig()), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(loadConfig()), }); if (openState.error) { client.app.log({ level: "error", message: `[Plannotator] ${openState.error}` }); @@ -162,8 +162,10 @@ export async function handleReviewCommand( parsed: reviewArgs, isPRMode: false, isWorkspace: false, - providerId, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + provider: managedVcs + ? { resolve: managedVcs.reviewPolicy.resolveOpenState } + : undefined, + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config, providerId), baseResolves, }); if (openState.error) { @@ -177,9 +179,9 @@ export async function handleReviewCommand( const diffResult = await prepareLocalReviewDiff({ cwd, vcsType: reviewArgs.vcsType, - requestedDiffType: openState.requestedDiffType, + requestedDiffType: openState.requestedDiffType as DiffType | undefined, requestedBase: openState.requestedBase, - configuredDiffType: resolveDefaultDiffType(config), + configuredDiffType: resolveConfiguredVcsReviewDefault(config, providerId), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); gitContext = diffResult.gitContext; @@ -202,7 +204,7 @@ export async function handleReviewCommand( parsed: reviewArgs, isPRMode: false, isWorkspace: true, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config), }); if (openState.error) { client.app.log({ level: "error", message: `[Plannotator] ${openState.error}` }); @@ -210,7 +212,7 @@ export async function handleReviewCommand( } } workspace = await buildLocalWorkspaceReview(cwd, { - configuredDiffType: resolveDefaultDiffType(config), + configuredDiffType: resolveConfiguredVcsReviewDefault(config), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); if (workspace.repos.length === 0) { diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index b1c80a079..d4fcf4ed9 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -6,9 +6,11 @@ import { createWorktreePool, type WorktreePool } from "./generated/worktree-pool import type { ExtensionContext } from "@earendil-works/pi-coding-agent"; import { prepareLocalReviewDiff, + resolveConfiguredVcsReviewDefault, reviewRuntime, detectManagedVcs, getVcsContext, + getVcsReviewPolicy, getVcsDiffFingerprint, getVcsFileContentsForDiff, canStageFiles, @@ -34,7 +36,7 @@ import { } from "./generated/pr-provider.ts"; import { parseRemoteUrl } from "./generated/repo.ts"; import { fetchRef, createWorktree, removeWorktree, ensureObjectAvailable } from "./generated/worktree.ts"; -import { loadConfig, resolveDefaultDiffType, resolveSharingEnabled } from "./generated/config.ts"; +import { loadConfig, resolveSharingEnabled } from "./generated/config.ts"; import { WorkspaceReviewSession, type WorkspaceDiffType, @@ -71,7 +73,7 @@ export interface BrowserDecisionSession { type CodeReviewOptions = { cwd?: string; defaultBranch?: string; - diffType?: DiffType; + diffType?: string; prUrl?: string; vcsType?: VcsSelection; useLocal?: boolean; @@ -412,7 +414,7 @@ async function createCodeReviewBrowserSession( parsed: { base: options.defaultBranch, diffType: options.diffType }, isPRMode: true, isWorkspace: false, - resolvedDefaultDiffType: resolveDefaultDiffType(loadConfig()), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(loadConfig()), }); if (openState.error) throw new Error(openState.error); } @@ -593,7 +595,14 @@ async function createCodeReviewBrowserSession( // resolve the effective requested base/diff type (promotion included). // Programmatic callers keep the verbatim pass-through below. let requestedBase = options.defaultBranch; - let requestedDiffType = options.diffType; + let requestedDiffType: DiffType | undefined; + if (!openStateFromFlags && options.diffType !== undefined) { + const reviewPolicy = managedVcs?.reviewPolicy ?? getVcsReviewPolicy(options.vcsType); + if (!reviewPolicy.ownsDiffType(options.diffType)) { + throw new Error(`Diff type ${options.diffType} is not available for this VCS provider.`); + } + requestedDiffType = options.diffType; + } if (openStateFromFlags) { const { resolveReviewOpenState } = await import("./generated/review-open-state.ts"); if (managedVcs || forcedVcs) { @@ -618,8 +627,10 @@ async function createCodeReviewBrowserSession( parsed: { base: requestedBase, diffType: requestedDiffType }, isPRMode: false, isWorkspace: false, - providerId, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + provider: managedVcs + ? { resolve: managedVcs.reviewPolicy.resolveOpenState } + : undefined, + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config, providerId), baseResolves, }); if (openState.error) throw new Error(openState.error); @@ -631,7 +642,7 @@ async function createCodeReviewBrowserSession( parsed: { base: requestedBase, diffType: requestedDiffType }, isPRMode: false, isWorkspace: true, - resolvedDefaultDiffType: resolveDefaultDiffType(config), + resolvedDefaultDiffType: resolveConfiguredVcsReviewDefault(config), }); if (openState.error) throw new Error(openState.error); } @@ -642,7 +653,7 @@ async function createCodeReviewBrowserSession( vcsType: options.vcsType, requestedDiffType, requestedBase, - configuredDiffType: resolveDefaultDiffType(config), + configuredDiffType: resolveConfiguredVcsReviewDefault(config, (managedVcs?.id as VcsSelection | undefined) ?? options.vcsType), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); gitCtx = result.gitContext; @@ -660,8 +671,8 @@ async function createCodeReviewBrowserSession( initialBaseExplicit = openStateFromFlags && requestedBase !== undefined; } else { workspace = await buildLocalWorkspaceReview(cwd, { - requestedDiffType: options.diffType, - configuredDiffType: resolveDefaultDiffType(config), + requestedDiffType, + configuredDiffType: resolveConfiguredVcsReviewDefault(config), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); if (workspace.repos.length === 0) { diff --git a/apps/pi-extension/review-args-parity.test.ts b/apps/pi-extension/review-args-parity.test.ts index 220c592fc..0fc0b484a 100644 --- a/apps/pi-extension/review-args-parity.test.ts +++ b/apps/pi-extension/review-args-parity.test.ts @@ -28,11 +28,12 @@ describe("vendored review-args parity", () => { // Guards the vendor.sh entry for review-open-state: without it Pi's // review command would crash on import instead of validating. const { resolveReviewOpenState } = await import("./generated/review-open-state.ts"); + const { jjReviewPolicy } = await import("./generated/jj-review-policy.ts"); const state = resolveReviewOpenState({ parsed: { base: "main" }, isPRMode: false, isWorkspace: false, - providerId: "jj", + provider: { resolve: jjReviewPolicy.resolveOpenState }, resolvedDefaultDiffType: "since-base", }); expect(state.error).toContain("--base is not supported in jj sessions"); diff --git a/apps/pi-extension/server.ts b/apps/pi-extension/server.ts index 6f2222b48..1822ff197 100644 --- a/apps/pi-extension/server.ts +++ b/apps/pi-extension/server.ts @@ -32,9 +32,11 @@ export { detectVcs, getGitContext, getVcsContext, + getVcsReviewPolicy, getVcsDiffFingerprint, getVcsFileContentsForDiff, prepareLocalReviewDiff, + resolveConfiguredVcsReviewDefault, resolveInitialDiffType, resolveVcsCwd, reviewRuntime, diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 786c64651..64c972640 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -3207,10 +3207,11 @@ export async function startReviewServer(options: { } } else if (url.pathname === "/api/config" && req.method === "POST") { try { - const body = (await parseBody(req)) as { displayName?: string; diffOptions?: Record; theme?: Record; favicon?: FaviconStyle; reviewAnalysis?: Record; conventionalComments?: boolean }; + const body = (await parseBody(req)) as { displayName?: string; diffOptions?: Record; reviewDefaults?: Record; theme?: Record; favicon?: FaviconStyle; reviewAnalysis?: Record; conventionalComments?: boolean }; const toSave: Record = {}; if (body.displayName !== undefined) toSave.displayName = body.displayName; if (body.diffOptions !== undefined) toSave.diffOptions = body.diffOptions; + if (body.reviewDefaults !== undefined) toSave.reviewDefaults = body.reviewDefaults; if (body.theme !== undefined) toSave.theme = body.theme; if (isFaviconStyle(body.favicon)) toSave.favicon = body.favicon; if (body.reviewAnalysis !== undefined) { diff --git a/apps/pi-extension/server/vcs.ts b/apps/pi-extension/server/vcs.ts index 011b1f35e..88c989e10 100644 --- a/apps/pi-extension/server/vcs.ts +++ b/apps/pi-extension/server/vcs.ts @@ -20,6 +20,10 @@ import { import { type ReviewGitButlerRuntime, } from "../generated/gitbutler-core.ts"; +import type { PlannotatorConfig } from "../generated/config.ts"; +import { gitReviewPolicy } from "../generated/git-review-policy.ts"; +import { gitButlerReviewPolicy } from "../generated/gitbutler-review-policy.ts"; +import { jjReviewPolicy } from "../generated/jj-review-policy.ts"; import { type VcsSelection, createGitButlerProvider, @@ -188,12 +192,14 @@ export const gitButlerRuntime: ReviewGitButlerRuntime = { }; const api = createVcsApi([ - createJjProvider(jjRuntime, reviewRuntime), - createGitButlerProvider(gitButlerRuntime), - createGitProvider(reviewRuntime), -]); + createJjProvider(jjRuntime, reviewRuntime, jjReviewPolicy), + createGitButlerProvider(gitButlerRuntime, gitButlerReviewPolicy), + createGitProvider(reviewRuntime, gitReviewPolicy), +], "git"); export const { + getReviewPolicy: getVcsReviewPolicy, + resolveReviewDefault: resolveVcsReviewDefault, detectVcs, detectManagedVcs, vcsOwnsDiffType, @@ -211,6 +217,17 @@ export const { materializeVcsSnapshot, } = api; +export function resolveConfiguredVcsReviewDefault( + config: PlannotatorConfig | undefined, + vcsType?: VcsSelection, +): DiffType { + return resolveVcsReviewDefault( + vcsType, + config?.reviewDefaults, + config?.diffOptions?.defaultDiffType, + ); +} + export { resolveAvailableDiffType, resolveInitialDiffType }; export type { VcsSelection }; diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index 347a446e3..3ad2d79b1 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -29,7 +29,7 @@ for f in config-types storage-types workspace-status-types; do done # Everything else in the original flat list stays sourced from packages/shared. -for f in prompts review-core generated-files feedback-archive cli-pagination jj-core gitbutler-core vcs-core review-args review-open-state draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common markdown-extensions resolve-file doc-resolve file-browser-watch-core annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node url-to-markdown tour annotate-args annotate-target at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff call-flow-types call-flow-languages call-flow-pack-locks call-flow-install-lock call-flow call-flow-install single-flight source-save-node review-profiles guide-store guide-instructions-store commit-avatars commit-history port-range annotate-client-lease annotate-decision archive-mode tailscale live-proxy-core live-probe live-proxy-node; do +for f in prompts review-core generated-files feedback-archive cli-pagination jj-core gitbutler-core vcs-core vcs-review-policy git-review-policy jj-review-policy gitbutler-review-policy p4-review-policy review-args review-open-state draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common markdown-extensions resolve-file doc-resolve file-browser-watch-core annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node url-to-markdown tour annotate-args annotate-target at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff call-flow-types call-flow-languages call-flow-pack-locks call-flow-install-lock call-flow call-flow-install single-flight source-save-node review-profiles guide-store guide-instructions-store commit-avatars commit-history port-range annotate-client-lease annotate-decision archive-mode tailscale live-proxy-core live-probe live-proxy-node; do src="../../packages/shared/$f.ts" # Shared modules that import browser-safe siblings from @plannotator/core # (e.g. guide-store → core/guide-format): generated/ is flat and vendors the diff --git a/packages/core/config-types.ts b/packages/core/config-types.ts index 5cf5825b2..5551935a1 100644 --- a/packages/core/config-types.ts +++ b/packages/core/config-types.ts @@ -1,4 +1,9 @@ -export type DefaultDiffType = 'since-base' | 'local-vs-remote' | 'uncommitted' | 'unstaged' | 'staged' | 'merge-base' | 'all'; +export interface ProviderReviewDefaults { + defaultDiffType?: string; +} + +export type ReviewDefaults = Record; + export type DiffLineBgIntensity = 'subtle' | 'normal' | 'strong'; /** @@ -24,6 +29,6 @@ export interface DiffOptions { tabSize?: number; hideWhitespace?: boolean; expandUnchanged?: boolean; - defaultDiffType?: DefaultDiffType; + defaultDiffType?: string; lineBgIntensity?: DiffLineBgIntensity; } diff --git a/packages/core/guide-viewer-manifest.ts b/packages/core/guide-viewer-manifest.ts index 7cd073456..e679ed863 100644 --- a/packages/core/guide-viewer-manifest.ts +++ b/packages/core/guide-viewer-manifest.ts @@ -5,9 +5,9 @@ import type { GuideViewerAssets } from "./guide-format"; export const GUIDE_VIEWER_MANIFEST: Omit = { - js: "viewer.C8JgNbZu.js", + js: "viewer.BB4fQouN.js", css: "viewer.KIp-iPxY.css", - jsIntegrity: "sha384-dqRkZ2N5WNwLIuwa+1kdrvswkOVHpkBxrrJ5mTU4YrUWzO/8fRS5MRTlDGV9t7eB", + jsIntegrity: "sha384-VoXz96TINekI60NrEdNdMZAzJ3EuHBKxro8eENNlYvTdjxUS/mu4zOwgOdxAC4cT", cssIntegrity: "sha384-Ty9hGpag8KIAGMDPg7ImsB4L5RabqvonNVjrj/CnYqSJDqIgPRBwjEMoK9/EvgXm", langs: { "astro": "chunks/astro.BykyiR6i.js", diff --git a/packages/server/review.ts b/packages/server/review.ts index 077806404..4b9616e15 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -3276,10 +3276,11 @@ export async function startReviewServer( // API: Update user config (write-back to ~/.plannotator/config.json) if (url.pathname === "/api/config" && req.method === "POST") { try { - const body = (await req.json()) as { displayName?: string; diffOptions?: Record; theme?: Record; favicon?: FaviconStyle; reviewAnalysis?: Record; conventionalComments?: boolean; conventionalLabels?: unknown[] | null }; + const body = (await req.json()) as { displayName?: string; diffOptions?: Record; reviewDefaults?: Record; theme?: Record; favicon?: FaviconStyle; reviewAnalysis?: Record; conventionalComments?: boolean; conventionalLabels?: unknown[] | null }; const toSave: Record = {}; if (body.displayName !== undefined) toSave.displayName = body.displayName; if (body.diffOptions !== undefined) toSave.diffOptions = body.diffOptions; + if (body.reviewDefaults !== undefined) toSave.reviewDefaults = body.reviewDefaults; if (body.theme !== undefined) toSave.theme = body.theme; if (isFaviconStyle(body.favicon)) toSave.favicon = body.favicon; if (body.reviewAnalysis !== undefined) { diff --git a/packages/server/vcs.test.ts b/packages/server/vcs.test.ts index 2d6d578c6..74e15a471 100644 --- a/packages/server/vcs.test.ts +++ b/packages/server/vcs.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import type { GitContext } from "@plannotator/shared/review-core"; -import { resolveInitialDiffType } from "./vcs"; +import { resolveConfiguredVcsReviewDefault, resolveInitialDiffType } from "./vcs"; function context(overrides: Partial): GitContext { return { @@ -22,8 +22,10 @@ describe("resolveInitialDiffType", () => { expect(resolveInitialDiffType(context({}), "merge-base")).toBe("merge-base"); }); - test("uses p4-default for P4 contexts", () => { - expect(resolveInitialDiffType(context({ vcsType: "p4" }), "merge-base")).toBe("p4-default"); + test("uses the P4 provider default instead of a legacy Git setting", () => { + expect(resolveConfiguredVcsReviewDefault({ + diffOptions: { defaultDiffType: "merge-base" }, + }, "p4")).toBe("p4-default"); }); test("ignores saved Git defaults for jj contexts", () => { diff --git a/packages/server/vcs.ts b/packages/server/vcs.ts index ca1448bc9..fbe1fa07a 100644 --- a/packages/server/vcs.ts +++ b/packages/server/vcs.ts @@ -2,6 +2,7 @@ import { type DiffType, type GitDiffOptions, type VcsProvider, + type VcsSelection, createGitButlerProvider, createGitProvider, createJjProvider, @@ -18,17 +19,22 @@ import { import { runtime as gitRuntime } from "./git"; import { runtime as gitButlerRuntime } from "./gitbutler"; import { runtime as jjRuntime } from "./jj"; +import { gitReviewPolicy } from "@plannotator/shared/git-review-policy"; +import { gitButlerReviewPolicy } from "@plannotator/shared/gitbutler-review-policy"; +import { jjReviewPolicy } from "@plannotator/shared/jj-review-policy"; +import { p4ReviewPolicy } from "@plannotator/shared/p4-review-policy"; +import type { PlannotatorConfig } from "@plannotator/shared/config"; const p4Provider: VcsProvider = { id: "p4", + label: "P4", + reviewPolicy: p4ReviewPolicy, async detect(cwd?: string): Promise { return (await detectP4Workspace(cwd)) !== null; }, - ownsDiffType(diffType: string): boolean { - return diffType === "p4-default" || diffType.startsWith("p4-changelist:"); - }, + ownsDiffType: p4ReviewPolicy.ownsDiffType, getContext: getP4Context, @@ -42,13 +48,15 @@ const p4Provider: VcsProvider = { }; const api = createVcsApi([ - createJjProvider(jjRuntime, gitRuntime), - createGitButlerProvider(gitButlerRuntime), - createGitProvider(gitRuntime), + createJjProvider(jjRuntime, gitRuntime, jjReviewPolicy), + createGitButlerProvider(gitButlerRuntime, gitButlerReviewPolicy), + createGitProvider(gitRuntime, gitReviewPolicy), p4Provider, -]); +], "git"); export const { + getReviewPolicy: getVcsReviewPolicy, + resolveReviewDefault: resolveVcsReviewDefault, detectVcs, detectManagedVcs, vcsOwnsDiffType, @@ -66,6 +74,17 @@ export const { materializeVcsSnapshot, } = api; +export function resolveConfiguredVcsReviewDefault( + config: PlannotatorConfig | undefined, + vcsType?: VcsSelection, +): DiffType { + return resolveVcsReviewDefault( + vcsType, + config?.reviewDefaults, + config?.diffOptions?.defaultDiffType, + ); +} + export { resolveAvailableDiffType, resolveInitialDiffType, gitRuntime }; export type { diff --git a/packages/shared/call-flow.test.ts b/packages/shared/call-flow.test.ts index 68ef64649..13bf3a781 100644 --- a/packages/shared/call-flow.test.ts +++ b/packages/shared/call-flow.test.ts @@ -1,3 +1,5 @@ +import { gitReviewPolicy } from "./git-review-policy"; +import { jjReviewPolicy } from "./jj-review-policy"; import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; @@ -75,7 +77,7 @@ const reviewRuntime: ReviewGitRuntime = { async readLink() { return null; }, }; -const gitVcs = createVcsApi([createGitProvider(reviewRuntime)]); +const gitVcs = createVcsApi([createGitProvider(reviewRuntime, gitReviewPolicy)]); function input(snapshotId = "snapshot"): CallFlowAnalysisInput { return { @@ -200,7 +202,7 @@ describe("VCS snapshot materialization", () => { }; }, }; - const jjVcs = createVcsApi([createJjProvider(jjRuntime, reviewRuntime)]); + const jjVcs = createVcsApi([createJjProvider(jjRuntime, reviewRuntime, jjReviewPolicy)]); expect(jjVcs.vcsSupportsSnapshot("jj", "jj-current")).toBe(true); expect(jjVcs.vcsSupportsSnapshot("jj", "jj-all")).toBe(false); const plan = await jjVcs.materializeVcsSnapshot("jj", { @@ -255,7 +257,7 @@ describe("VCS snapshot materialization", () => { return { stdout: "diff --git a/main.ts b/main.ts\n", stderr: "", exitCode: 0, truncated: true }; }, }; - const jjVcs = createVcsApi([createJjProvider(jjRuntime, reviewRuntime)]); + const jjVcs = createVcsApi([createJjProvider(jjRuntime, reviewRuntime, jjReviewPolicy)]); expect(jjVcs.materializeVcsSnapshot("jj", { cwd: repo, diffType: "jj-current", diff --git a/packages/shared/config.test.ts b/packages/shared/config.test.ts index 9a0c525cd..698b9fb45 100644 --- a/packages/shared/config.test.ts +++ b/packages/shared/config.test.ts @@ -26,21 +26,12 @@ import { getServerConfig, resolveGuideShareUrl, resolveSharingEnabled, - resolveDefaultDiffType, DEFAULT_GUIDE_SHARE_URL, __setConfigLockTimingsForTest, __setConfigSaveMergeWindowHookForTest, } from "./config"; import type { PlannotatorConfig } from "./config"; -describe("resolveDefaultDiffType", () => { - test("accepts local-vs-remote as a persisted review default", () => { - expect(resolveDefaultDiffType({ - diffOptions: { defaultDiffType: "local-vs-remote" }, - })).toBe("local-vs-remote"); - }); -}); - describe("parseReviewAnalysisConfig", () => { test("accepts independent boolean analysis flags", () => { expect(parseReviewAnalysisConfig({ semanticDiff: false })).toEqual({ semanticDiff: false }); @@ -360,7 +351,7 @@ describe("config.json boolean coercion", () => { } }); -describe("favicon config persistence", () => { +describe("config persistence", () => { const originalDataDir = process.env.PLANNOTATOR_DATA_DIR; let tempDir: string; @@ -394,6 +385,16 @@ describe("favicon config persistence", () => { expect(loadConfig().favicon).toBe(unknownFavicon); expect(getServerConfig(null).favicon).toBeUndefined(); }); + + test("merges provider review defaults without dropping another provider", () => { + saveConfig({ reviewDefaults: { git: { defaultDiffType: "merge-base" } } }); + saveConfig({ reviewDefaults: { jj: { defaultDiffType: "jj-line" } } }); + + expect(getServerConfig(null).reviewDefaults).toEqual({ + git: { defaultDiffType: "merge-base" }, + jj: { defaultDiffType: "jj-line" }, + }); + }); }); describe("saveConfig write serialization", () => { diff --git a/packages/shared/config.ts b/packages/shared/config.ts index 230e0a7bd..fb033c00d 100644 --- a/packages/shared/config.ts +++ b/packages/shared/config.ts @@ -22,10 +22,10 @@ import { } from "fs"; import { execSync } from "child_process"; -import type { DefaultDiffType, DiffLineBgIntensity, DiffOptions, ThemeConfig } from '@plannotator/core/config-types'; +import type { DiffLineBgIntensity, DiffOptions, ReviewDefaults, ThemeConfig } from '@plannotator/core/config-types'; import { isFaviconStyle, type FaviconStyle } from './favicon'; import { isAnnotateAgentTerminalSide, type AnnotateAgentTerminalSide } from './agent-terminal'; -export type { DefaultDiffType, DiffLineBgIntensity, DiffOptions, ThemeConfig, FaviconStyle }; +export type { DiffLineBgIntensity, DiffOptions, ThemeConfig, FaviconStyle }; /** Single conventional comment label entry stored in config.json */ export interface CCLabelConfig { @@ -103,6 +103,8 @@ export function mergePromptConfig( export interface PlannotatorConfig { displayName?: string; diffOptions?: DiffOptions; + /** Provider-scoped review defaults. Unknown provider keys are preserved. */ + reviewDefaults?: ReviewDefaults; /** Optional analysis layers used by code review. */ reviewAnalysis?: { /** Named-entity semantic diff. Enabled by default for backwards compatibility. */ @@ -508,6 +510,9 @@ export function saveConfig(partial: Partial): void { const mergedReviewAnalysis = (current.reviewAnalysis || partial.reviewAnalysis) ? { ...current.reviewAnalysis, ...partial.reviewAnalysis } : undefined; + const mergedReviewDefaults = (current.reviewDefaults || partial.reviewDefaults) + ? { ...current.reviewDefaults, ...partial.reviewDefaults } + : undefined; const mergedPrompts = mergePromptConfig(current.prompts, partial.prompts); const merged = { ...current, @@ -515,6 +520,7 @@ export function saveConfig(partial: Partial): void { diffOptions: mergedDiffOptions, theme: mergedTheme, reviewAnalysis: mergedReviewAnalysis, + reviewDefaults: mergedReviewDefaults, prompts: mergedPrompts, }; writeConfigAtomic(getConfigPath(), JSON.stringify(merged, null, 2) + "\n"); @@ -545,6 +551,7 @@ export function detectGitUser(): string | null { export function getServerConfig(gitUser: string | null): { displayName?: string; diffOptions?: DiffOptions; + reviewDefaults?: ReviewDefaults; theme?: ThemeConfig; favicon?: FaviconStyle; reviewAnalysis: NonNullable; @@ -558,6 +565,7 @@ export function getServerConfig(gitUser: string | null): { return { displayName: cfg.displayName, diffOptions: cfg.diffOptions, + ...(cfg.reviewDefaults !== undefined && { reviewDefaults: cfg.reviewDefaults }), ...(cfg.theme !== undefined && { theme: cfg.theme }), ...(isFaviconStyle(cfg.favicon) && { favicon: cfg.favicon }), // These values gate server-side work, so always make the resolved defaults @@ -597,17 +605,6 @@ export function isAgentTerminalSide( return isAnnotateAgentTerminalSide(value); } -/** - * Read the user's preferred default diff type from config, falling back to - * 'since-base' (the composite "what would GitHub show" view). Users with an - * explicit defaultDiffType keep their choice. - */ -export function resolveDefaultDiffType(cfg?: PlannotatorConfig): DefaultDiffType { - const v = cfg?.diffOptions?.defaultDiffType as string | undefined; - if (v === 'branch') return 'merge-base'; - return v === 'since-base' || v === 'local-vs-remote' || v === 'uncommitted' || v === 'unstaged' || v === 'staged' || v === 'merge-base' || v === 'all' ? v : 'since-base'; -} - /** * Coerce a config.json value that should be a boolean. JSON parsing preserves * whatever type the user typed, so a hand-edited `"false"` (quoted) arrives as diff --git a/packages/shared/git-review-policy.ts b/packages/shared/git-review-policy.ts new file mode 100644 index 000000000..6cc8e3760 --- /dev/null +++ b/packages/shared/git-review-policy.ts @@ -0,0 +1,88 @@ +import type { DiffType } from "./review-core"; +import { + buildBaseNotFoundError, + type ProviderReviewOpenStateInput, + type ReviewOpenState, +} from "./review-open-state"; +import type { VcsReviewPolicy } from "./vcs-review-policy"; + +export const GIT_DIFF_TYPES = new Set([ + "since-base", + "local-vs-remote", + "uncommitted", + "staged", + "unstaged", + "last-commit", + "branch", + "merge-base", + "all", +]); + +const BASE_RELATIVE_DIFF_TYPES = ["since-base", "branch", "merge-base"] as const; +const BASE_RELATIVE = new Set(BASE_RELATIVE_DIFF_TYPES); + +export const gitReviewPolicy: VcsReviewPolicy = { + defaultDiffType: "since-base", + ownsDiffType(diffType: string): diffType is DiffType { + return GIT_DIFF_TYPES.has(diffType) + || diffType.startsWith("worktree:") + || diffType.startsWith("commit:"); + }, + legacyDefault: { + resolve: resolveGitDefault, + }, + resolveDefault: resolveGitDefault, + resolveOpenState: resolveGitOpenState, + resolveInitialBase(defaultBase, _diffType, requestedBase) { + return requestedBase ?? defaultBase; + }, +}; + +function resolveGitDefault(value: unknown): DiffType | undefined { + if (value === "branch") return "merge-base"; + return typeof value === "string" && GIT_DIFF_TYPES.has(value) + ? value as DiffType + : undefined; +} + +function resolveGitOpenState(input: ProviderReviewOpenStateInput): ReviewOpenState { + const { base, diffType } = input; + if (diffType !== undefined && !GIT_DIFF_TYPES.has(diffType)) { + return fail(`Unknown diff type: ${diffType}. Expected one of: ${[...GIT_DIFF_TYPES].join(", ")}`); + } + if (base !== undefined) { + if (diffType !== undefined && !BASE_RELATIVE.has(diffType)) { + return fail( + `--base has no effect with --diff-type ${diffType}.\n` + + `Base-relative diff types: ${BASE_RELATIVE_DIFF_TYPES.join(", ")}.`, + ); + } + if (input.baseResolves === false) { + return fail(buildBaseNotFoundError(base, input.availableBranches)); + } + } + + const notices: string[] = []; + let requestedDiffType = diffType as DiffType | undefined; + if (base !== undefined && diffType === undefined) { + if (BASE_RELATIVE.has(input.resolvedDefaultDiffType)) { + requestedDiffType = input.resolvedDefaultDiffType; + } else { + requestedDiffType = "since-base"; + notices.push( + `[plannotator] --base ${base} needs a base-relative diff; opening on "since-base" ` + + `for this session (your default stays ${input.resolvedDefaultDiffType}).`, + ); + } + } + + return { + ...(base !== undefined && { requestedBase: base }), + ...(requestedDiffType !== undefined && { requestedDiffType }), + notices, + }; +} + +function fail(error: string): ReviewOpenState { + return { notices: [], error }; +} diff --git a/packages/shared/gitbutler-core.test.ts b/packages/shared/gitbutler-core.test.ts index 9462c8582..8e73d6a80 100644 --- a/packages/shared/gitbutler-core.test.ts +++ b/packages/shared/gitbutler-core.test.ts @@ -1,3 +1,5 @@ +import { gitReviewPolicy } from "./git-review-policy"; +import { gitButlerReviewPolicy } from "./gitbutler-review-policy"; import { describe, expect, test } from "bun:test"; import type { GitCommandOptions, GitCommandResult } from "./review-core"; @@ -331,8 +333,8 @@ describe("GitButler detection and context", () => { test("ordinary Git selection never invokes the GitButler CLI", async () => { const fixture = createRuntime({ activeRef: "refs/heads/main" }); const api = createVcsApi([ - createGitButlerProvider(fixture.runtime), - createGitProvider(fixture.runtime), + createGitButlerProvider(fixture.runtime, gitButlerReviewPolicy), + createGitProvider(fixture.runtime, gitReviewPolicy), ]); await expect(api.detectManagedVcs(ROOT)).resolves.toMatchObject({ id: "git" }); @@ -342,8 +344,8 @@ describe("GitButler detection and context", () => { test("an ordinary Git branch named gitbutler/workspace stays on the Git provider", async () => { const fixture = createRuntime({ configured: false }); const api = createVcsApi([ - createGitButlerProvider(fixture.runtime), - createGitProvider(fixture.runtime), + createGitButlerProvider(fixture.runtime, gitButlerReviewPolicy), + createGitProvider(fixture.runtime, gitReviewPolicy), ]); await expect(api.detectManagedVcs(ROOT)).resolves.toMatchObject({ id: "git" }); @@ -446,8 +448,8 @@ describe("GitButler detection and context", () => { test("an active workspace with a missing CLI does not silently fall back to Git", async () => { const fixture = createRuntime({ version: commandResult("", "but not found", 1) }); const api = createVcsApi([ - createGitButlerProvider(fixture.runtime), - createGitProvider(fixture.runtime), + createGitButlerProvider(fixture.runtime, gitButlerReviewPolicy), + createGitProvider(fixture.runtime, gitReviewPolicy), ]); await expect(api.prepareLocalReviewDiff({ diff --git a/packages/shared/gitbutler-review-policy.ts b/packages/shared/gitbutler-review-policy.ts new file mode 100644 index 000000000..62c84b047 --- /dev/null +++ b/packages/shared/gitbutler-review-policy.ts @@ -0,0 +1,28 @@ +import { parseGitButlerDiffType } from "./gitbutler-core"; +import type { DiffType } from "./review-core"; +import type { ProviderReviewOpenStateInput, ReviewOpenState } from "./review-open-state"; +import type { VcsReviewPolicy } from "./vcs-review-policy"; + +export const gitButlerReviewPolicy: VcsReviewPolicy = { + defaultDiffType: "gitbutler:workspace", + ownsDiffType(diffType: string): diffType is DiffType { + return parseGitButlerDiffType(diffType) !== null; + }, + resolveDefault() { + return undefined; + }, + resolveOpenState(input) { + return fail( + input.base !== undefined + ? "--base is not supported in a GitButler workspace; GitButler derives the merge base from the workspace itself." + : "--diff-type is not supported in a GitButler workspace; GitButler modes are selected in the UI.", + ); + }, + resolveInitialBase(defaultBase) { + return defaultBase; + }, +}; + +function fail(error: string): ReviewOpenState { + return { notices: [], error }; +} diff --git a/packages/shared/jj-review-policy.ts b/packages/shared/jj-review-policy.ts new file mode 100644 index 000000000..bc0a7206e --- /dev/null +++ b/packages/shared/jj-review-policy.ts @@ -0,0 +1,41 @@ +import type { DiffType } from "./review-core"; +import type { ProviderReviewOpenStateInput, ReviewOpenState } from "./review-open-state"; +import type { VcsReviewPolicy } from "./vcs-review-policy"; + +export const JJ_DIFF_TYPES = new Set([ + "jj-current", + "jj-last", + "jj-line", + "jj-evolog", + "jj-all", +]); + +export const jjReviewPolicy: VcsReviewPolicy = { + defaultDiffType: "jj-current", + ownsDiffType(diffType: string): diffType is DiffType { + return JJ_DIFF_TYPES.has(diffType); + }, + resolveDefault(value) { + return typeof value === "string" && JJ_DIFF_TYPES.has(value) + ? value as DiffType + : undefined; + }, + resolveOpenState: rejectJjOpenState, + resolveInitialBase(defaultBase, diffType, requestedBase, ownsRequestedDiffType) { + return diffType === "jj-line" && ownsRequestedDiffType && requestedBase + ? requestedBase + : defaultBase; + }, +}; + +function rejectJjOpenState(input: ProviderReviewOpenStateInput): ReviewOpenState { + return fail( + input.base !== undefined + ? "--base is not supported in jj sessions yet (only the jj-line mode has a base)." + : "--diff-type is not supported in jj sessions; jj modes are selected in the UI.", + ); +} + +function fail(error: string): ReviewOpenState { + return { notices: [], error }; +} diff --git a/packages/shared/jj-snapshot.test.ts b/packages/shared/jj-snapshot.test.ts index 1d7807394..c5c3dcb4b 100644 --- a/packages/shared/jj-snapshot.test.ts +++ b/packages/shared/jj-snapshot.test.ts @@ -1,3 +1,4 @@ +import { jjReviewPolicy } from "./jj-review-policy"; /** * Real-Jujutsu coverage for Call flow snapshot materialization. * @@ -65,7 +66,7 @@ const jjRuntime: ReviewJjRuntime = { }, }; -const vcs = createVcsApi([createJjProvider(jjRuntime, gitRuntime)]); +const vcs = createVcsApi([createJjProvider(jjRuntime, gitRuntime, jjReviewPolicy)]); let workspace = ""; diff --git a/packages/shared/p4-review-policy.ts b/packages/shared/p4-review-policy.ts new file mode 100644 index 000000000..c58e2b0de --- /dev/null +++ b/packages/shared/p4-review-policy.ts @@ -0,0 +1,27 @@ +import type { DiffType } from "./review-core"; +import type { ReviewOpenState } from "./review-open-state"; +import type { VcsReviewPolicy } from "./vcs-review-policy"; + +export const p4ReviewPolicy: VcsReviewPolicy = { + defaultDiffType: "p4-default", + ownsDiffType(diffType: string): diffType is DiffType { + return diffType === "p4-default" || diffType.startsWith("p4-changelist:"); + }, + resolveDefault() { + return undefined; + }, + resolveOpenState(input) { + return fail( + input.base !== undefined + ? "--base is not supported in Perforce sessions." + : "--diff-type is not supported in Perforce sessions.", + ); + }, + resolveInitialBase(defaultBase) { + return defaultBase; + }, +}; + +function fail(error: string): ReviewOpenState { + return { notices: [], error }; +} diff --git a/packages/shared/package.json b/packages/shared/package.json index d8355a0a3..c06ca1e13 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -18,6 +18,11 @@ "./review-args": "./review-args.ts", "./review-open-state": "./review-open-state.ts", "./vcs-core": "./vcs-core.ts", + "./vcs-review-policy": "./vcs-review-policy.ts", + "./git-review-policy": "./git-review-policy.ts", + "./jj-review-policy": "./jj-review-policy.ts", + "./gitbutler-review-policy": "./gitbutler-review-policy.ts", + "./p4-review-policy": "./p4-review-policy.ts", "./checklist": "./checklist.ts", "./types": "./types.ts", "./pr-types": "./pr-types.ts", diff --git a/packages/shared/review-args.test.ts b/packages/shared/review-args.test.ts index 4f000d342..f4362fef7 100644 --- a/packages/shared/review-args.test.ts +++ b/packages/shared/review-args.test.ts @@ -1,6 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { REVIEW_OPEN_DIFF_TYPES, parseReviewArgs } from "./review-args"; -import { GIT_DIFF_TYPES } from "./vcs-core"; +import { parseReviewArgs } from "./review-args"; describe("parseReviewArgs", () => { test("defaults to auto VCS and local PR checkout", () => { @@ -136,16 +135,10 @@ describe("parseReviewArgs", () => { expect(parsed.base).toBe("a"); }); - test("--diff-type rejects unknown ids, listing the valid set", () => { - // Failure caught: an unowned diff type reaching resolveRequestedDiffType, - // which silently falls back to the configured default. - const parsed = parseReviewArgs("--diff-type nonsense"); - expect(parsed.errors).toHaveLength(1); - expect(parsed.errors[0]).toContain("Unknown diff type: nonsense"); - for (const id of REVIEW_OPEN_DIFF_TYPES) { - expect(parsed.errors[0]).toContain(id); - } - expect(parsed.diffType).toBeUndefined(); + test("--diff-type preserves provider-owned ids for validation after detection", () => { + const parsed = parseReviewArgs("--diff-type provider-mode"); + expect(parsed.errors).toEqual([]); + expect(parsed.diffType).toBe("provider-mode"); }); test("rejects base refs carrying range syntax", () => { @@ -156,12 +149,6 @@ describe("parseReviewArgs", () => { ]); }); - test("REVIEW_OPEN_DIFF_TYPES is exactly GIT_DIFF_TYPES", () => { - // Failure caught: a git diff type added to one set and not the other, - // making a valid mode unreachable from (or falsely advertised by) the CLI. - expect(new Set(REVIEW_OPEN_DIFF_TYPES)).toEqual(GIT_DIFF_TYPES); - }); - test("an unknown dashed token cannot shadow a PR URL", () => { // Before the errors[] contract, `--bse` landed in positional[0] and the // real PR URL in positional[1] was never inspected — the PR silently diff --git a/packages/shared/review-args.ts b/packages/shared/review-args.ts index e1decb587..7d0ff8514 100644 --- a/packages/shared/review-args.ts +++ b/packages/shared/review-args.ts @@ -1,29 +1,6 @@ import type { VcsSelection } from "./vcs-core"; -import type { DiffType } from "./review-core"; import { stripWrappingQuotes } from "./resolve-file"; -/** - * The flat git diff ids `review --diff-type` accepts — exactly GIT_DIFF_TYPES - * in vcs-core, pinned by test (a git diff type added to one list and not the - * other would make a valid mode unreachable from the CLI). Kept as a literal - * list here (type-only imports elsewhere) so review-args stays light for - * plugin hosts. Session-navigation states (`commit:`, `worktree:*`, - * `gitbutler:*`, jj/p4 modes) are deliberately not open states. - */ -export const REVIEW_OPEN_DIFF_TYPES = [ - "since-base", - "local-vs-remote", - "uncommitted", - "staged", - "unstaged", - "last-commit", - "branch", - "merge-base", - "all", -] as const; - -export type ReviewOpenDiffType = (typeof REVIEW_OPEN_DIFF_TYPES)[number]; - export interface ParsedReviewArgs { prUrl?: string; vcsType?: VcsSelection; @@ -31,7 +8,7 @@ export interface ParsedReviewArgs { /** Compare target the session opens against (`--base `). */ base?: string; /** Diff mode the session opens in (`--diff-type `). */ - diffType?: DiffType; + diffType?: string; /** * Argument-shape problems the host must surface before starting a session. * Always present; empty means the invocation parsed cleanly. Hosts differ in @@ -49,7 +26,7 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { let vcsType: VcsSelection | undefined; let useLocal = true; let base: string | undefined; - let diffType: DiffType | undefined; + let diffType: string | undefined; const errors: string[] = []; const positional: string[] = []; @@ -103,13 +80,7 @@ export function parseReviewArgs(input: string | string[]): ParsedReviewArgs { errors.push("--diff-type may only be specified once"); break; } - if (!(REVIEW_OPEN_DIFF_TYPES as readonly string[]).includes(value)) { - errors.push( - `Unknown diff type: ${value}. Expected one of: ${REVIEW_OPEN_DIFF_TYPES.join(", ")}`, - ); - break; - } - diffType = value as DiffType; + diffType = value; break; } default: diff --git a/packages/shared/review-open-state.test.ts b/packages/shared/review-open-state.test.ts index 7a3e6fe6e..c3ed8b3c9 100644 --- a/packages/shared/review-open-state.test.ts +++ b/packages/shared/review-open-state.test.ts @@ -1,19 +1,22 @@ import { describe, expect, test } from "bun:test"; import { - BASE_RELATIVE_DIFF_TYPES, buildBaseNotFoundError, resolveReviewOpenState, suggestBaseRefs, } from "./review-open-state"; import type { ReviewOpenStateInput } from "./review-open-state"; import type { DiffType } from "./review-core"; +import { gitReviewPolicy } from "./git-review-policy"; +import { gitButlerReviewPolicy } from "./gitbutler-review-policy"; +import { jjReviewPolicy } from "./jj-review-policy"; +import { p4ReviewPolicy } from "./p4-review-policy"; function input(overrides: Partial = {}): ReviewOpenStateInput { return { parsed: {}, isPRMode: false, isWorkspace: false, - providerId: "git", + provider: { resolve: gitReviewPolicy.resolveOpenState }, resolvedDefaultDiffType: "since-base", ...overrides, }; @@ -45,7 +48,7 @@ describe("resolveReviewOpenState", () => { // (and p4 has no base at all): accepting the flag would quietly review // against the wrong base — the silent lie this matrix exists to prevent. const state = resolveReviewOpenState( - input({ providerId, parsed: { base: "feature/part-1" }, baseResolves: true }), + input({ provider: { resolve: ({ gitbutler: gitButlerReviewPolicy, jj: jjReviewPolicy, p4: p4ReviewPolicy } as const)[providerId].resolveOpenState }, parsed: { base: "feature/part-1" }, baseResolves: true }), ); expect(state.error).toContain("--base is not supported"); expect(state.requestedBase).toBeUndefined(); @@ -58,7 +61,7 @@ describe("resolveReviewOpenState", () => { // ownsDiffType rejects git diff ids on these providers, so the request // would be silently dropped by resolveRequestedDiffType. const state = resolveReviewOpenState( - input({ providerId, parsed: { diffType: "since-base" } }), + input({ provider: { resolve: ({ gitbutler: gitButlerReviewPolicy, jj: jjReviewPolicy, p4: p4ReviewPolicy } as const)[providerId].resolveOpenState }, parsed: { diffType: "since-base" } }), ); expect(state.error).toContain("--diff-type is not supported"); }, @@ -66,22 +69,22 @@ describe("resolveReviewOpenState", () => { test("workspace + either flag errors (a base parameter with nowhere to go)", () => { const base = resolveReviewOpenState( - input({ isWorkspace: true, providerId: undefined, parsed: { base: "main" } }), + input({ isWorkspace: true, parsed: { base: "main" } }), ); expect(base.error).toContain("multi-repo workspace review"); const diffType = resolveReviewOpenState( - input({ isWorkspace: true, providerId: undefined, parsed: { diffType: "uncommitted" } }), + input({ isWorkspace: true, parsed: { diffType: "uncommitted" } }), ); expect(diffType.error).toContain("multi-repo workspace review"); }); test("PR mode + either flag errors (the base comes from the pull request)", () => { const base = resolveReviewOpenState( - input({ isPRMode: true, providerId: undefined, parsed: { base: "main" } }), + input({ isPRMode: true, parsed: { base: "main" } }), ); expect(base.error).toContain("pull request"); const diffType = resolveReviewOpenState( - input({ isPRMode: true, providerId: undefined, parsed: { diffType: "merge-base" } }), + input({ isPRMode: true, parsed: { diffType: "merge-base" } }), ); expect(diffType.error).toContain("pull request"); }); @@ -112,7 +115,7 @@ describe("resolveReviewOpenState", () => { }), ); expect(state.error).toContain("--base has no effect with --diff-type uncommitted"); - expect(state.error).toContain(BASE_RELATIVE_DIFF_TYPES.join(", ")); + expect(state.error).toContain("since-base, branch, merge-base"); }); test("--base with a base-relative resolved default is left alone, no notice", () => { diff --git a/packages/shared/review-open-state.ts b/packages/shared/review-open-state.ts index 9a14c1473..e504561d3 100644 --- a/packages/shared/review-open-state.ts +++ b/packages/shared/review-open-state.ts @@ -12,21 +12,27 @@ import type { ParsedReviewArgs } from "./review-args"; import type { AvailableBranches, DiffType } from "./review-core"; -/** The diff types for which a base ref is meaningful (compareTarget.diffTypes). */ -export const BASE_RELATIVE_DIFF_TYPES = ["since-base", "branch", "merge-base"] as const; - -const BASE_RELATIVE = new Set(BASE_RELATIVE_DIFF_TYPES); +export interface ReviewOpenStatePolicy { + resolve(input: ProviderReviewOpenStateInput): ReviewOpenState; +} export interface ReviewOpenStateInput { parsed: Pick; isPRMode: boolean; isWorkspace: boolean; - providerId?: "git" | "gitbutler" | "jj" | "p4"; - /** resolveDefaultDiffType(config) — the diff the session would open on without flags. */ + provider?: ReviewOpenStatePolicy; + resolvedDefaultDiffType: DiffType; + /** Result of a provider-specific base probe; undefined = not probed. */ + baseResolves?: boolean; + /** Optional names for provider-specific near-match suggestions. */ + availableBranches?: AvailableBranches; +} + +export interface ProviderReviewOpenStateInput { + base?: string; + diffType?: string; resolvedDefaultDiffType: DiffType; - /** Result of the CLI-side `git rev-parse --verify` probe; undefined = not probed. */ baseResolves?: boolean; - /** Optional branch lists for near-match suggestions in the not-found error. */ availableBranches?: AvailableBranches; } @@ -66,74 +72,17 @@ export function resolveReviewOpenState(input: ReviewOpenStateInput): ReviewOpenS ); } - // Provider matrix: on jj/gitbutler `resolveInitialBase` hard-returns the - // detected default and `ownsDiffType` rejects git diff ids, so accepting the - // flags would silently review against the wrong base. Error honestly instead. - if (input.providerId === "gitbutler") { - return fail( - base !== undefined - ? "--base is not supported in a GitButler workspace; GitButler derives the merge base from the workspace itself." - : "--diff-type is not supported in a GitButler workspace; GitButler modes are selected in the UI.", - ); - } - if (input.providerId === "jj") { - return fail( - base !== undefined - ? "--base is not supported in jj sessions yet (only the jj-line mode has a base)." - : "--diff-type is not supported in jj sessions; jj modes are selected in the UI.", - ); - } - if (input.providerId === "p4") { - return fail( - base !== undefined - ? "--base is not supported in Perforce sessions." - : "--diff-type is not supported in Perforce sessions.", - ); - } - - if (base !== undefined) { - // Explicit contradiction: the caller stated both, and the stated diff type - // ignores the base. Fail rather than quietly doing half of what was asked. - if (diffType !== undefined && !BASE_RELATIVE.has(diffType)) { - return fail( - `--base has no effect with --diff-type ${diffType}.\n` + - `Base-relative diff types: ${BASE_RELATIVE_DIFF_TYPES.join(", ")}.`, - ); - } - // The probe is the whole point: without it a typo'd base degrades the - // since-base diff to `merge-base -> HEAD` under a confidently wrong label. - if (input.baseResolves === false) { - return fail(buildBaseNotFoundError(base, input.availableBranches)); - } - } - - const notices: string[] = []; - let requestedDiffType = diffType; - if (base !== undefined && diffType === undefined) { - if (BASE_RELATIVE.has(input.resolvedDefaultDiffType)) { - // Request the resolved default EXPLICITLY: `resolveRequestedDiffType` - // honors an owned request even when `gitContext.diffOptions` omitted - // since-base (undiscoverable trunk), so a probed `--base trunk` makes - // since-base work on a repo where the UI cannot currently offer it. - requestedDiffType = input.resolvedDefaultDiffType; - } else { - // The conflicting value comes from the human's config, which an agent - // running `review --base X` cannot see — promote with a notice rather - // than failing (unreliable flag) or doing nothing (the bug this feature - // exists to fix). - requestedDiffType = "since-base"; - notices.push( - `[plannotator] --base ${base} needs a base-relative diff; opening on "since-base" ` + - `for this session (your default stays ${input.resolvedDefaultDiffType}).`, - ); - } + if (!input.provider) { + return fail("Review options are not available because no VCS provider was selected."); } - return { - ...(base !== undefined && { requestedBase: base }), - ...(requestedDiffType !== undefined && { requestedDiffType }), - notices, - }; + return input.provider.resolve({ + base, + diffType, + resolvedDefaultDiffType: input.resolvedDefaultDiffType, + baseResolves: input.baseResolves, + availableBranches: input.availableBranches, + }); } function fail(error: string): ReviewOpenState { diff --git a/packages/shared/vcs-core.test.ts b/packages/shared/vcs-core.test.ts index c91fe8f59..9ca02761b 100644 --- a/packages/shared/vcs-core.test.ts +++ b/packages/shared/vcs-core.test.ts @@ -5,6 +5,9 @@ import type { GitContext, ReviewGitRuntime, } from "./review-core"; +import { gitReviewPolicy } from "./git-review-policy"; +import { gitButlerReviewPolicy } from "./gitbutler-review-policy"; +import { jjReviewPolicy } from "./jj-review-policy"; import { type VcsProvider, createGitProvider, @@ -37,6 +40,12 @@ function provider( const isDetected = () => typeof detected === "function" ? detected() : detected; return { id, + label: id === "git" ? "Git" : id, + reviewPolicy: id === "jj" + ? jjReviewPolicy + : id === "gitbutler" + ? gitButlerReviewPolicy + : gitReviewPolicy, async detect() { return isDetected(); }, @@ -202,7 +211,7 @@ describe("createVcsApi", () => { }); test("limits Git staging to working-tree diff modes", async () => { - const git = createVcsApi([createGitProvider(gitRuntime)]); + const git = createVcsApi([createGitProvider(gitRuntime, gitReviewPolicy)]); await expect(git.canStageFiles("uncommitted", "/repo")).resolves.toBe(true); await expect(git.canStageFiles("unstaged", "/repo")).resolves.toBe(true); @@ -216,7 +225,7 @@ describe("createVcsApi", () => { }); test("the git provider owns commit: diff types", () => { - const git = createGitProvider(gitRuntime); + const git = createGitProvider(gitRuntime, gitReviewPolicy); expect(git.ownsDiffType("commit:abc1234")).toBe(true); expect(git.ownsDiffType("worktree:/repo:commit:abc1234")).toBe(true); }); @@ -462,9 +471,6 @@ describe("resolveInitialDiffType", () => { expect(resolveInitialDiffType(context({}), "merge-base")).toBe("merge-base"); }); - test("uses p4-default for P4 contexts", () => { - expect(resolveInitialDiffType(context({ vcsType: "p4" }), "merge-base")).toBe("p4-default"); - }); test("ignores saved Git defaults for jj contexts", () => { const jjContext = context({ diff --git a/packages/shared/vcs-core.ts b/packages/shared/vcs-core.ts index 6d9aa4aeb..4a615e11b 100644 --- a/packages/shared/vcs-core.ts +++ b/packages/shared/vcs-core.ts @@ -30,13 +30,13 @@ import { resolveJjSnapshotEndpoint, runJjDiff, } from "./jj-core"; +import { resolveProviderReviewDefault, type VcsReviewPolicy } from "./vcs-review-policy"; import { type ReviewGitButlerRuntime, detectGitButlerWorkspace, getGitButlerContext, getGitButlerDiffFingerprint, getGitButlerFileContentsForDiff, - parseGitButlerDiffType, runGitButlerDiff, } from "./gitbutler-core"; @@ -61,9 +61,11 @@ export { export interface VcsProvider { readonly id: string; + readonly label: string; + readonly reviewPolicy: VcsReviewPolicy; detect(cwd?: string): Promise; getRoot?(cwd?: string): Promise; - ownsDiffType(diffType: string): boolean; + ownsDiffType(diffType: string): diffType is DiffType; canStageFiles?(diffType: string): boolean; getContext(cwd?: string): Promise; runDiff(diffType: DiffType, defaultBranch: string, cwd?: string, options?: GitDiffOptions): Promise; @@ -110,6 +112,12 @@ export interface VcsSnapshot { export type VcsSelection = "auto" | "git" | "gitbutler" | "jj" | "p4"; export interface VcsApi { + getReviewPolicy(vcsType?: VcsSelection): VcsReviewPolicy; + resolveReviewDefault( + vcsType: VcsSelection | undefined, + configuredValues: Record | undefined, + legacyValue: unknown, + ): DiffType; detectVcs(cwd?: string): Promise; detectManagedVcs(cwd?: string, vcsType?: VcsSelection): Promise; vcsOwnsDiffType(vcsType: Exclude, diffType: string): boolean; @@ -168,12 +176,6 @@ export interface PreparedLocalReviewDiff { fingerprint?: string; } -// Exported so review-args can pin REVIEW_OPEN_DIFF_TYPES (the flat ids -// `review --diff-type` accepts) against it — a git diff type added to one set -// and not the other would make a valid mode unreachable from the CLI. -export const GIT_DIFF_TYPES = new Set(["since-base", "local-vs-remote", "uncommitted", "staged", "unstaged", "last-commit", "branch", "merge-base", "all"]); -const JJ_DIFF_TYPES = new Set(["jj-current", "jj-last", "jj-line", "jj-evolog", "jj-all"]); - function selectNearestProvider( candidates: Array<{ provider: VcsProvider; root: string | null; order: number }>, cwd?: string, @@ -205,9 +207,14 @@ function vcsRootDepth(root: string): number { return resolve(root).split(/[\\/]+/).filter(Boolean).length; } -export function createGitProvider(runtime: ReviewGitRuntime): VcsProvider { +export function createGitProvider( + runtime: ReviewGitRuntime, + reviewPolicy: VcsReviewPolicy, +): VcsProvider { return { id: "git", + label: "Git", + reviewPolicy, async detect(cwd?: string): Promise { try { @@ -223,13 +230,7 @@ export function createGitProvider(runtime: ReviewGitRuntime): VcsProvider { return result.exitCode === 0 ? result.stdout.trim() || null : null; }, - ownsDiffType(diffType: string): boolean { - return ( - GIT_DIFF_TYPES.has(diffType) || - diffType.startsWith("worktree:") || - diffType.startsWith("commit:") - ); - }, + ownsDiffType: reviewPolicy.ownsDiffType, canStageFiles(diffType: string): boolean { const effectiveDiffType = parseWorktreeDiffType(diffType)?.subType ?? diffType; @@ -282,9 +283,15 @@ export function createGitProvider(runtime: ReviewGitRuntime): VcsProvider { }; } -export function createJjProvider(runtime: ReviewJjRuntime, gitRuntime: ReviewGitRuntime): VcsProvider { +export function createJjProvider( + runtime: ReviewJjRuntime, + gitRuntime: ReviewGitRuntime, + reviewPolicy: VcsReviewPolicy, +): VcsProvider { return { id: "jj", + label: "JJ", + reviewPolicy, async detect(cwd?: string): Promise { return (await detectJjWorkspace(runtime, cwd)) !== null; @@ -294,9 +301,7 @@ export function createJjProvider(runtime: ReviewJjRuntime, gitRuntime: ReviewGit return detectJjWorkspace(runtime, cwd); }, - ownsDiffType(diffType: string): boolean { - return JJ_DIFF_TYPES.has(diffType); - }, + ownsDiffType: reviewPolicy.ownsDiffType, getContext(cwd?: string): Promise { return getJjContext(runtime, cwd); @@ -323,9 +328,14 @@ export function createJjProvider(runtime: ReviewJjRuntime, gitRuntime: ReviewGit } /** Create the provider for an actively checked-out GitButler workspace. */ -export function createGitButlerProvider(runtime: ReviewGitButlerRuntime): VcsProvider { +export function createGitButlerProvider( + runtime: ReviewGitButlerRuntime, + reviewPolicy: VcsReviewPolicy, +): VcsProvider { return { id: "gitbutler", + label: "GitButler", + reviewPolicy, async detect(cwd?: string): Promise { return (await detectGitButlerWorkspace(runtime, cwd)) !== null; @@ -335,9 +345,7 @@ export function createGitButlerProvider(runtime: ReviewGitButlerRuntime): VcsPro return detectGitButlerWorkspace(runtime, cwd); }, - ownsDiffType(diffType: string): boolean { - return parseGitButlerDiffType(diffType) !== null; - }, + ownsDiffType: reviewPolicy.ownsDiffType, getContext(cwd?: string): Promise { return getGitButlerContext(runtime, cwd); @@ -357,9 +365,12 @@ export function createGitButlerProvider(runtime: ReviewGitButlerRuntime): VcsPro }; } -export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { +export function createVcsApi( + providers: readonly VcsProvider[], + defaultProviderId = providers[0]?.id, +): VcsApi { const providerList = [...providers]; - const defaultProvider = providerList.find((provider) => provider.id === "git") ?? providerList[0]; + const defaultProvider = providerList.find((provider) => provider.id === defaultProviderId) ?? providerList[0]; if (!defaultProvider) { throw new Error("createVcsApi requires at least one provider"); @@ -425,19 +436,6 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { return providerList.find((provider) => provider.id === id) ?? null; } - function formatVcsName(id: Exclude): string { - switch (id) { - case "git": - return "Git"; - case "gitbutler": - return "GitButler"; - case "jj": - return "JJ"; - case "p4": - return "P4"; - } - } - async function getProviderForSelection( vcsType: VcsSelection | undefined, cwd?: string, @@ -447,12 +445,11 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { } const provider = getProviderById(vcsType); - const vcsName = formatVcsName(vcsType); if (!provider) { - throw new Error(`${vcsName} support is not available in this runtime.`); + throw new Error(`${vcsType} support is not available in this runtime.`); } if (!(await provider.detect(cwd))) { - throw new Error(`${vcsName} workspace not found.`); + throw new Error(`${provider.label} workspace not found.`); } return provider; } @@ -481,22 +478,24 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { return resolveInitialDiffType(gitContext, configuredDiffType); } - function resolveInitialBase( - gitContext: GitContext, - diffType: DiffType, - requestedBase: string | undefined, - ownsRequestedDiffType: boolean, - ): string { - if (gitContext.vcsType === "jj" || gitContext.vcsType === "gitbutler") { - if (diffType === "jj-line" && ownsRequestedDiffType && requestedBase) { - return requestedBase; - } - return gitContext.defaultBranch; - } - return requestedBase ?? gitContext.defaultBranch; - } return { + getReviewPolicy(vcsType): VcsReviewPolicy { + if (!vcsType || vcsType === "auto") return defaultProvider.reviewPolicy; + return getProviderById(vcsType)?.reviewPolicy ?? defaultProvider.reviewPolicy; + }, + + resolveReviewDefault(vcsType, configuredValues, legacyValue): DiffType { + const provider = vcsType && vcsType !== "auto" + ? getProviderById(vcsType) ?? defaultProvider + : defaultProvider; + return resolveProviderReviewDefault( + provider.reviewPolicy, + configuredValues?.[provider.id]?.defaultDiffType, + legacyValue, + ); + }, + detectVcs, detectManagedVcs, @@ -526,7 +525,12 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { const resolution = resolveAvailableDiffType(gitContext, requestedDiffType, options.requestedBase !== undefined); const fallback = resolution.fallback; const diffType = resolution.diffType; - const base = resolveInitialBase(gitContext, diffType, options.requestedBase, ownsRequestedDiffType); + const base = provider.reviewPolicy.resolveInitialBase( + gitContext.defaultBranch, + diffType, + options.requestedBase, + ownsRequestedDiffType, + ); const result = await provider.runDiff(diffType, base, gitContext.cwd ?? options.cwd, { hideWhitespace: options.hideWhitespace, }); @@ -628,7 +632,7 @@ export function createVcsApi(providers: readonly VcsProvider[]): VcsApi { ): Promise { const provider = getProviderById(vcsType); if (!provider?.materializeSnapshot || (!options.prCommitPair && !(provider.supportsSnapshot?.(options.diffType) ?? false))) { - throw new Error(`Snapshot materialization does not support the ${options.diffType} ${formatVcsName(vcsType)} review mode.`); + throw new Error(`Snapshot materialization does not support the ${options.diffType} ${provider?.label ?? vcsType} review mode.`); } return provider.materializeSnapshot(options); }, @@ -651,12 +655,6 @@ export function resolveInitialDiffType( gitContext: GitContext, configuredDiffType: DiffType, ): DiffType { - if (gitContext.vcsType === "p4") { - return "p4-default"; - } - if (gitContext.vcsType === "jj") { - return "jj-current"; - } if (gitContext.diffOptions.some((option) => option.id === configuredDiffType)) { return configuredDiffType; } diff --git a/packages/shared/vcs-review-policy.test.ts b/packages/shared/vcs-review-policy.test.ts new file mode 100644 index 000000000..2d39a8187 --- /dev/null +++ b/packages/shared/vcs-review-policy.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, test } from "bun:test"; +import { gitReviewPolicy } from "./git-review-policy"; +import { jjReviewPolicy } from "./jj-review-policy"; +import { resolveReviewOpenState } from "./review-open-state"; +import { resolveProviderReviewDefault } from "./vcs-review-policy"; + +const asOpenStatePolicy = (policy: typeof gitReviewPolicy | typeof jjReviewPolicy) => ({ + resolve: policy.resolveOpenState, +}); + +describe("provider-owned review defaults", () => { + test("Git alone interprets the legacy default and its retired branch alias", () => { + expect(resolveProviderReviewDefault(gitReviewPolicy, undefined, "branch")).toBe("merge-base"); + expect(resolveProviderReviewDefault(jjReviewPolicy, undefined, "merge-base")).toBe("jj-current"); + }); + + test("a provider-scoped value wins over the legacy source", () => { + expect(resolveProviderReviewDefault(gitReviewPolicy, "unstaged", "merge-base")).toBe("unstaged"); + }); +}); + +describe("provider-owned open-state validation", () => { + test("argument parsing can pass an opaque mode to the selected provider", () => { + const result = resolveReviewOpenState({ + parsed: { diffType: "provider-mode" }, + isPRMode: false, + isWorkspace: false, + provider: asOpenStatePolicy(gitReviewPolicy), + resolvedDefaultDiffType: "since-base", + }); + + expect(result.error).toContain("Unknown diff type: provider-mode"); + }); + + test("JJ owns its unsupported-open-state message", () => { + const result = resolveReviewOpenState({ + parsed: { base: "main" }, + isPRMode: false, + isWorkspace: false, + provider: asOpenStatePolicy(jjReviewPolicy), + resolvedDefaultDiffType: "jj-current", + }); + + expect(result.error).toContain("not supported in jj sessions yet"); + }); +}); diff --git a/packages/shared/vcs-review-policy.ts b/packages/shared/vcs-review-policy.ts new file mode 100644 index 000000000..b97eda99e --- /dev/null +++ b/packages/shared/vcs-review-policy.ts @@ -0,0 +1,33 @@ +import type { DiffType } from "./review-core"; +import type { + ProviderReviewOpenStateInput, + ReviewOpenState, +} from "./review-open-state"; + +export interface LegacyReviewDefaultAdapter { + resolve(value: unknown): DiffType | undefined; +} + +export interface VcsReviewPolicy { + readonly defaultDiffType: DiffType; + ownsDiffType(diffType: string): diffType is DiffType; + readonly legacyDefault?: LegacyReviewDefaultAdapter; + resolveDefault(value: unknown): DiffType | undefined; + resolveOpenState(input: ProviderReviewOpenStateInput): ReviewOpenState; + resolveInitialBase( + defaultBase: string, + diffType: DiffType, + requestedBase: string | undefined, + ownsRequestedDiffType: boolean, + ): string; +} + +export function resolveProviderReviewDefault( + policy: VcsReviewPolicy, + configuredValue: unknown, + legacyValue: unknown, +): DiffType { + return policy.resolveDefault(configuredValue) + ?? policy.legacyDefault?.resolve(legacyValue) + ?? policy.defaultDiffType; +} diff --git a/packages/ui/config/index.ts b/packages/ui/config/index.ts index e782bc078..9d1f0cd4b 100644 --- a/packages/ui/config/index.ts +++ b/packages/ui/config/index.ts @@ -4,6 +4,7 @@ export { useConfigValue } from './useConfig'; export { setReviewPanelView, setReviewDefaultDiffType, + setProviderReviewDefaultDiffType, getPersistedReviewPanelView, setReviewAutoViewed, needsAutoViewedNotice, diff --git a/packages/ui/config/reviewView.ts b/packages/ui/config/reviewView.ts index 21fc40b14..8d93ff09b 100644 --- a/packages/ui/config/reviewView.ts +++ b/packages/ui/config/reviewView.ts @@ -68,6 +68,7 @@ export function setReviewDefaultDiffType( store: PanelViewConfigStore = configStore, ): void { store.set('defaultDiffType', value); + setProviderReviewDefaultDiffType('git', value, store); if (value !== 'since-base' && store.get('reviewPanelView') !== 'tree') { store.set('reviewPanelView', 'tree'); // The snap is an explicit-choice consequence (the user picked a classic @@ -76,6 +77,16 @@ export function setReviewDefaultDiffType( } } +export function setProviderReviewDefaultDiffType( + providerId: string, + value: string, + store: PanelViewConfigStore = configStore, +): void { + store.set('reviewDefaults', { + ...store.get('reviewDefaults'), + [providerId]: { defaultDiffType: value }, + }); +} /** * One-time gate for the auto-mark-viewed notice — the toast that fires the diff --git a/packages/ui/config/settings.ts b/packages/ui/config/settings.ts index a748ad912..ea57a9989 100644 --- a/packages/ui/config/settings.ts +++ b/packages/ui/config/settings.ts @@ -13,7 +13,7 @@ import { isAnnotateAgentTerminalSide, type AnnotateAgentTerminalSide, } from '@plannotator/core/agent-terminal'; -import type { DiffLineBgIntensity } from '@plannotator/core/config-types'; +import type { DiffLineBgIntensity, ReviewDefaults } from '@plannotator/core/config-types'; import { isFaviconStyle, type FaviconStyle } from '@plannotator/core/favicon'; import { DEFAULT_TOKEN_HOVER_DELAY_MS, @@ -93,6 +93,19 @@ function isDiffLineBgIntensity(v: unknown): v is DiffLineBgIntensity { return typeof v === 'string' && (DIFF_LINE_BG_INTENSITY_VALUES as readonly string[]).includes(v); } +function parseReviewDefaults(value: unknown): ReviewDefaults | undefined { + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const defaults: ReviewDefaults = {}; + for (const [providerId, providerValue] of Object.entries(value)) { + if (!providerId || !providerValue || typeof providerValue !== 'object' || Array.isArray(providerValue)) continue; + const defaultDiffType = (providerValue as Record).defaultDiffType; + if (typeof defaultDiffType === 'string' && defaultDiffType) { + defaults[providerId] = { defaultDiffType }; + } + } + return defaults; +} + export interface SettingDef { defaultValue: T | (() => T); fromCookie: () => T | undefined; @@ -332,6 +345,25 @@ export const SETTINGS = { serverKey: undefined, fromServer: undefined, toServer: undefined, }, + reviewDefaults: { + defaultValue: {} as ReviewDefaults, + fromCookie: () => { + const value = storage.getItem('plannotator-review-defaults'); + if (!value) return undefined; + try { + return parseReviewDefaults(JSON.parse(value)); + } catch { + return undefined; + } + }, + toCookie: (value: ReviewDefaults) => + storage.setItem('plannotator-review-defaults', JSON.stringify(value)), + serverKey: 'reviewDefaults', + fromServer: (serverConfig: Record) => + parseReviewDefaults(serverConfig.reviewDefaults), + toServer: (value: ReviewDefaults) => ({ reviewDefaults: value }), + }, + defaultDiffType: { defaultValue: 'since-base' as 'since-base' | 'local-vs-remote' | 'uncommitted' | 'unstaged' | 'staged' | 'merge-base' | 'all', fromCookie: () => { From 738d48252c96579749bf440f360999fe6bb7a909 Mon Sep 17 00:00:00 2001 From: Graeme Folk Date: Thu, 10 Sep 2026 09:10:33 -0600 Subject: [PATCH 2/2] feat(review): add JJ review defaults resolve validated diff type stack --- apps/pi-extension/review-args-parity.test.ts | 6 +- apps/pi-extension/server/serverReview.ts | 2 + apps/pi-extension/server/vcs.ts | 1 + packages/core/config-types.ts | 20 ++ packages/review-editor/App.tsx | 6 + packages/server/review.ts | 3 +- packages/server/vcs.test.ts | 3 +- packages/server/vcs.ts | 1 + packages/shared/git-review-policy.ts | 16 ++ packages/shared/jj-review-policy.ts | 38 +++- packages/shared/review-core.ts | 3 + packages/shared/review-open-state.test.ts | 27 ++- packages/shared/vcs-core.test.ts | 41 +++- packages/shared/vcs-core.ts | 18 +- packages/shared/vcs-review-policy.test.ts | 8 +- packages/shared/vcs-review-policy.ts | 2 + packages/ui/components/Settings.tsx | 205 ++++++++++-------- packages/ui/config/index.ts | 1 + .../ui/config/reviewPanelViewLastUsed.test.ts | 46 +++- packages/ui/config/reviewView.ts | 16 ++ 20 files changed, 357 insertions(+), 106 deletions(-) diff --git a/apps/pi-extension/review-args-parity.test.ts b/apps/pi-extension/review-args-parity.test.ts index 0fc0b484a..fc5bb17f4 100644 --- a/apps/pi-extension/review-args-parity.test.ts +++ b/apps/pi-extension/review-args-parity.test.ts @@ -36,7 +36,11 @@ describe("vendored review-args parity", () => { provider: { resolve: jjReviewPolicy.resolveOpenState }, resolvedDefaultDiffType: "since-base", }); - expect(state.error).toContain("--base is not supported in jj sessions"); + expect(state).toEqual({ + requestedBase: "main", + requestedDiffType: "jj-line", + notices: [], + }); }); test("the review command handler forwards the parsed open state", async () => { diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 64c972640..604e85480 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -188,6 +188,7 @@ import { canStageFiles, detectRemoteDefaultCompareTarget, getVcsContext, + getVcsReviewSettings, getVcsDiffFingerprint, getVcsFileContentsForDiff, resolveVcsCwd, @@ -2093,6 +2094,7 @@ export async function startReviewServer(options: { hideWhitespace: servedHideWhitespace, ...(workspace && { diffOptions: workspace.diffOptions }), gitContext: hasLocalAccess ? servedGitContext : undefined, + reviewSettings: getVcsReviewSettings(), sharingEnabled, approvalNotesSupported, // Mount is the only place the pin matters, so it rides /api/diff diff --git a/apps/pi-extension/server/vcs.ts b/apps/pi-extension/server/vcs.ts index 88c989e10..034fa4aa1 100644 --- a/apps/pi-extension/server/vcs.ts +++ b/apps/pi-extension/server/vcs.ts @@ -198,6 +198,7 @@ const api = createVcsApi([ ], "git"); export const { + getReviewSettings: getVcsReviewSettings, getReviewPolicy: getVcsReviewPolicy, resolveReviewDefault: resolveVcsReviewDefault, detectVcs, diff --git a/packages/core/config-types.ts b/packages/core/config-types.ts index 5551935a1..cc37d2111 100644 --- a/packages/core/config-types.ts +++ b/packages/core/config-types.ts @@ -4,6 +4,26 @@ export interface ProviderReviewDefaults { export type ReviewDefaults = Record; +export interface ReviewSettingsDiffOption { + id: string; + label: string; + description: string; +} + +export interface VcsReviewSettingsDescriptor { + id: string; + label: string; + defaultDiffType: string; + diffOptions: ReviewSettingsDiffOption[]; + capabilities: { + statusSections: boolean; + staging: boolean; + compareTarget: boolean; + }; + /** Legacy setting registry key, interpreted generically by the UI. */ + legacyDefaultSetting?: string; +} + export type DiffLineBgIntensity = 'subtle' | 'normal' | 'strong'; /** diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 4bd14f128..16788bc9b 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -7,6 +7,7 @@ import '@plannotator/ui/utils/math-eager'; import '@plannotator/ui/utils/identity-tater'; import React, { useState, useEffect, useCallback, useMemo, useRef } from 'react'; +import type { VcsReviewSettingsDescriptor } from '@plannotator/core/config-types'; import { type Origin, getAgentName } from '@plannotator/shared/agents'; import { ThemeProvider, useTheme } from '@plannotator/ui/components/ThemeProvider'; import { TooltipProvider } from '@plannotator/ui/components/Tooltip'; @@ -560,6 +561,7 @@ const ReviewApp: React.FC = () => { const [reviewMode, setReviewMode] = useState(null); const [diffType, setDiffType] = useState('uncommitted'); const [gitContext, setGitContext] = useState(null); + const [reviewSettings, setReviewSettings] = useState([]); const [workspaceDiffOptions, setWorkspaceDiffOptions] = useState(null); // Two bases: // selectedBase — what the picker is currently showing (UI intent). @@ -1986,6 +1988,7 @@ const ReviewApp: React.FC = () => { diffType?: string; base?: string; gitContext?: GitContext; + reviewSettings?: VcsReviewSettingsDescriptor[]; diffOptions?: DiffOption[]; agentCwd?: string | null; sharingEnabled?: boolean; @@ -2036,6 +2039,7 @@ const ReviewApp: React.FC = () => { setFiles(apiFiles); setReviewMode(data.mode ?? null); setWorkspaceDiffOptions(data.mode === 'workspace' ? (data.diffOptions ?? []) : null); + setReviewSettings(data.reviewSettings ?? data.gitContext?.reviewSettings ?? []); if (data.origin) setOrigin(data.origin); if (data.diffType) setDiffType(data.diffType); if (data.gitContext) { @@ -5417,6 +5421,8 @@ const ReviewApp: React.FC = () => { mode="review" aiProviders={aiProviders} gitUser={gitUser} + reviewSettings={reviewSettings} + activeVcsId={gitContext?.vcsType} externalOpen={openSettingsMenu} onExternalClose={() => setOpenSettingsMenu(false)} // Local git session where since-base isn't offered (base ref diff --git a/packages/server/review.ts b/packages/server/review.ts index 4b9616e15..6244c9b89 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -11,7 +11,7 @@ import { isRemoteSession, getServerHostname, startBunServerOnAvailablePort, buildAdvertisedUrl } from "./remote"; import type { Origin } from "@plannotator/shared/agents"; -import { type DiffType, type GitContext, runVcsDiff, getVcsFileContentsForDiff, getVcsDiffFingerprint, canStageFiles, stageFile, unstageFile, resolveVcsCwd, validateFilePath, getVcsContext, detectRemoteDefaultCompareTarget, resolveAvailableDiffType, vcsOwnsDiffType, vcsSupportsSnapshot, materializeVcsSnapshot, gitRuntime } from "./vcs"; +import { type DiffType, type GitContext, runVcsDiff, getVcsFileContentsForDiff, getVcsDiffFingerprint, canStageFiles, stageFile, unstageFile, resolveVcsCwd, validateFilePath, getVcsContext, getVcsReviewSettings, detectRemoteDefaultCompareTarget, resolveAvailableDiffType, vcsOwnsDiffType, vcsSupportsSnapshot, materializeVcsSnapshot, gitRuntime } from "./vcs"; import { basename } from "node:path"; import { existsSync } from "node:fs"; import { SingleFlight } from "@plannotator/shared/single-flight"; @@ -2078,6 +2078,7 @@ export async function startReviewServer( hideWhitespace: servedHideWhitespace, ...(workspace && { diffOptions: workspace.diffOptions }), gitContext: hasLocalAccess ? servedGitContext : undefined, + reviewSettings: getVcsReviewSettings(), sharingEnabled, approvalNotesSupported, // Mount is the only place the pin matters, so it rides /api/diff diff --git a/packages/server/vcs.test.ts b/packages/server/vcs.test.ts index 74e15a471..6c541dec8 100644 --- a/packages/server/vcs.test.ts +++ b/packages/server/vcs.test.ts @@ -28,7 +28,7 @@ describe("resolveInitialDiffType", () => { }, "p4")).toBe("p4-default"); }); - test("ignores saved Git defaults for jj contexts", () => { + test("uses JJ defaults and ignores saved Git defaults for jj contexts", () => { const jjContext = context({ defaultBranch: "trunk()", diffOptions: [ @@ -39,6 +39,7 @@ describe("resolveInitialDiffType", () => { vcsType: "jj", }); + expect(resolveInitialDiffType(jjContext, "jj-line")).toBe("jj-line"); expect(resolveInitialDiffType(jjContext, "all")).toBe("jj-current"); expect(resolveInitialDiffType(jjContext, "merge-base")).toBe("jj-current"); expect(resolveInitialDiffType(jjContext, "unstaged")).toBe("jj-current"); diff --git a/packages/server/vcs.ts b/packages/server/vcs.ts index fbe1fa07a..fae6b350d 100644 --- a/packages/server/vcs.ts +++ b/packages/server/vcs.ts @@ -55,6 +55,7 @@ const api = createVcsApi([ ], "git"); export const { + getReviewSettings: getVcsReviewSettings, getReviewPolicy: getVcsReviewPolicy, resolveReviewDefault: resolveVcsReviewDefault, detectVcs, diff --git a/packages/shared/git-review-policy.ts b/packages/shared/git-review-policy.ts index 6cc8e3760..1056cdbd4 100644 --- a/packages/shared/git-review-policy.ts +++ b/packages/shared/git-review-policy.ts @@ -23,6 +23,22 @@ const BASE_RELATIVE = new Set(BASE_RELATIVE_DIFF_TYPES); export const gitReviewPolicy: VcsReviewPolicy = { defaultDiffType: "since-base", + settings: { + id: "git", + label: "Git", + defaultDiffType: "since-base", + diffOptions: [ + { id: "since-base", label: "All Changes (Recommended)", description: "Everything since your branch split from main — committed, uncommitted, and untracked" }, + { id: "local-vs-remote", label: "Local vs Remote Branch", description: "Your local branch and working tree compared with its last-fetched remote-tracking branch" }, + { id: "uncommitted", label: "Uncommitted", description: "Everything you've changed since your last commit" }, + { id: "unstaged", label: "Unstaged", description: "Only changes you haven't staged yet" }, + { id: "staged", label: "Staged", description: "Only changes you've staged for commit" }, + { id: "merge-base", label: "Committed changes (PR view)", description: "Everything you've committed on this branch" }, + { id: "all", label: "All Files (HEAD)", description: "Every tracked file at HEAD, shown as additions" }, + ], + capabilities: { statusSections: true, staging: true, compareTarget: true }, + legacyDefaultSetting: "defaultDiffType", + }, ownsDiffType(diffType: string): diffType is DiffType { return GIT_DIFF_TYPES.has(diffType) || diffType.startsWith("worktree:") diff --git a/packages/shared/jj-review-policy.ts b/packages/shared/jj-review-policy.ts index bc0a7206e..ae453f61a 100644 --- a/packages/shared/jj-review-policy.ts +++ b/packages/shared/jj-review-policy.ts @@ -12,6 +12,19 @@ export const JJ_DIFF_TYPES = new Set([ export const jjReviewPolicy: VcsReviewPolicy = { defaultDiffType: "jj-current", + settings: { + id: "jj", + label: "Jujutsu", + defaultDiffType: "jj-current", + diffOptions: [ + { id: "jj-current", label: "Current change", description: "Only the changes in the working-copy change" }, + { id: "jj-line", label: "Line of work", description: "The complete mutable line of work leading to the working copy" }, + { id: "jj-last", label: "Last change", description: "The change immediately before the working copy" }, + { id: "jj-evolog", label: "Evolution diff", description: "How the current change differs from its previous state" }, + { id: "jj-all", label: "All files", description: "Every file at the working-copy revision, shown as additions" }, + ], + capabilities: { statusSections: false, staging: false, compareTarget: true }, + }, ownsDiffType(diffType: string): diffType is DiffType { return JJ_DIFF_TYPES.has(diffType); }, @@ -20,7 +33,7 @@ export const jjReviewPolicy: VcsReviewPolicy = { ? value as DiffType : undefined; }, - resolveOpenState: rejectJjOpenState, + resolveOpenState: resolveJjOpenState, resolveInitialBase(defaultBase, diffType, requestedBase, ownsRequestedDiffType) { return diffType === "jj-line" && ownsRequestedDiffType && requestedBase ? requestedBase @@ -28,12 +41,23 @@ export const jjReviewPolicy: VcsReviewPolicy = { }, }; -function rejectJjOpenState(input: ProviderReviewOpenStateInput): ReviewOpenState { - return fail( - input.base !== undefined - ? "--base is not supported in jj sessions yet (only the jj-line mode has a base)." - : "--diff-type is not supported in jj sessions; jj modes are selected in the UI.", - ); +function resolveJjOpenState(input: ProviderReviewOpenStateInput): ReviewOpenState { + const { base, diffType } = input; + if (diffType !== undefined && !JJ_DIFF_TYPES.has(diffType)) { + return fail(`Unknown diff type: ${diffType}. Expected one of: ${[...JJ_DIFF_TYPES].join(", ")}`); + } + if (base !== undefined && diffType !== undefined && diffType !== "jj-line") { + return fail(`--base has no effect with --diff-type ${diffType}.\nBase-relative Jujutsu diff type: jj-line.`); + } + return { + ...(base !== undefined && { requestedBase: base }), + ...(diffType !== undefined + ? { requestedDiffType: diffType as DiffType } + : base !== undefined + ? { requestedDiffType: "jj-line" } + : {}), + notices: [], + }; } function fail(error: string): ReviewOpenState { diff --git a/packages/shared/review-core.ts b/packages/shared/review-core.ts index 6d42051b9..e91ab6d39 100644 --- a/packages/shared/review-core.ts +++ b/packages/shared/review-core.ts @@ -5,6 +5,7 @@ * self-contained while review diff logic remains sourced from one module. */ +import type { VcsReviewSettingsDescriptor } from "@plannotator/core/config-types"; import { formatDiffMetadataPathToken, formatPatchPathToken, @@ -153,6 +154,8 @@ export interface GitContext { vcsType?: "git" | "gitbutler" | "jj" | "p4"; /** Hash of the exact GitButler branch/commit topology used for this context. */ gitButlerRevision?: string; + /** Review settings contributed by the providers registered in this runtime. */ + reviewSettings?: VcsReviewSettingsDescriptor[]; /** Automatic line-of-work base resolution (jj only). */ jjLineBase?: JjLineBaseResolution; /** Evolution log entries for the current jj change (jj only). */ diff --git a/packages/shared/review-open-state.test.ts b/packages/shared/review-open-state.test.ts index c3ed8b3c9..2dea15797 100644 --- a/packages/shared/review-open-state.test.ts +++ b/packages/shared/review-open-state.test.ts @@ -41,11 +41,11 @@ describe("resolveReviewOpenState", () => { expect(state.notices).toEqual([]); }); - test.each(["gitbutler", "jj", "p4"] as const)( + test.each(["gitbutler", "p4"] as const)( "%s + --base errors instead of accept-and-ignore", (providerId) => { - // resolveInitialBase hard-returns the detected default on jj/gitbutler - // (and p4 has no base at all): accepting the flag would quietly review + // These providers derive their own base (or have none): accepting the + // flag would quietly review // against the wrong base — the silent lie this matrix exists to prevent. const state = resolveReviewOpenState( input({ provider: { resolve: ({ gitbutler: gitButlerReviewPolicy, jj: jjReviewPolicy, p4: p4ReviewPolicy } as const)[providerId].resolveOpenState }, parsed: { base: "feature/part-1" }, baseResolves: true }), @@ -55,7 +55,7 @@ describe("resolveReviewOpenState", () => { }, ); - test.each(["gitbutler", "jj", "p4"] as const)( + test.each(["gitbutler", "p4"] as const)( "%s + --diff-type since-base errors instead of accept-and-ignore", (providerId) => { // ownsDiffType rejects git diff ids on these providers, so the request @@ -67,6 +67,25 @@ describe("resolveReviewOpenState", () => { }, ); + test("JJ flags seed native modes and only allow a base for line-of-work", () => { + expect(resolveReviewOpenState(input({ + provider: { resolve: jjReviewPolicy.resolveOpenState }, + parsed: { diffType: "jj-last" }, + resolvedDefaultDiffType: "jj-current", + }))).toEqual({ requestedDiffType: "jj-last", notices: [] }); + + expect(resolveReviewOpenState(input({ + provider: { resolve: jjReviewPolicy.resolveOpenState }, + parsed: { base: "develop@origin" }, + resolvedDefaultDiffType: "jj-current", + }))).toEqual({ requestedBase: "develop@origin", requestedDiffType: "jj-line", notices: [] }); + + expect(resolveReviewOpenState(input({ + provider: { resolve: jjReviewPolicy.resolveOpenState }, + parsed: { base: "develop@origin", diffType: "jj-current" }, + })).error).toContain("--base has no effect"); + }); + test("workspace + either flag errors (a base parameter with nowhere to go)", () => { const base = resolveReviewOpenState( input({ isWorkspace: true, parsed: { base: "main" } }), diff --git a/packages/shared/vcs-core.test.ts b/packages/shared/vcs-core.test.ts index 9ca02761b..77189d5b3 100644 --- a/packages/shared/vcs-core.test.ts +++ b/packages/shared/vcs-core.test.ts @@ -8,6 +8,7 @@ import type { import { gitReviewPolicy } from "./git-review-policy"; import { gitButlerReviewPolicy } from "./gitbutler-review-policy"; import { jjReviewPolicy } from "./jj-review-policy"; +import { p4ReviewPolicy } from "./p4-review-policy"; import { type VcsProvider, createGitProvider, @@ -45,7 +46,9 @@ function provider( ? jjReviewPolicy : id === "gitbutler" ? gitButlerReviewPolicy - : gitReviewPolicy, + : id === "p4" + ? p4ReviewPolicy + : gitReviewPolicy, async detect() { return isDetected(); }, @@ -92,6 +95,39 @@ describe("createVcsApi", () => { await expect(api.getVcsContext("/repo")).resolves.toMatchObject({ vcsType: "jj" }); }); + test("exposes settings descriptors from registered providers without requiring every provider to implement one", async () => { + const git = { + ...provider("git", true, ["uncommitted"]), + reviewPolicy: { + ...gitReviewPolicy, + settings: { + id: "git", + label: "Git", + defaultDiffType: "uncommitted", + diffOptions: [{ id: "uncommitted", label: "Uncommitted", description: "Working tree changes" }], + capabilities: { statusSections: true, staging: true, compareTarget: true }, + }, + }, + } satisfies VcsProvider; + const plugin = { + ...provider("plugin-vcs", false, ["plugin-diff"]), + reviewPolicy: { + ...gitReviewPolicy, + settings: { + id: "plugin-vcs", + label: "Plugin VCS", + defaultDiffType: "plugin-diff", + diffOptions: [{ id: "plugin-diff", label: "Plugin diff", description: "Plugin changes" }], + capabilities: { statusSections: false, staging: false, compareTarget: false }, + }, + }, + } satisfies VcsProvider; + const p4 = provider("p4", false, ["p4-default"]); + + const context = await createVcsApi([plugin, git, p4]).getVcsContext("/repo"); + expect(context.reviewSettings?.map((descriptor) => descriptor.id)).toEqual(["plugin-vcs", "git"]); + }); + test("selects GitButler ahead of Git without changing JJ precedence", async () => { const jj = provider("jj", false, ["jj-current"], {}, "/repo"); const gitButler = provider("gitbutler", true, ["gitbutler:workspace"], {}, "/repo"); @@ -472,7 +508,7 @@ describe("resolveInitialDiffType", () => { }); - test("ignores saved Git defaults for jj contexts", () => { + test("uses JJ defaults and ignores saved Git defaults for jj contexts", () => { const jjContext = context({ defaultBranch: "trunk()", diffOptions: [ @@ -483,6 +519,7 @@ describe("resolveInitialDiffType", () => { vcsType: "jj", }); + expect(resolveInitialDiffType(jjContext, "jj-line")).toBe("jj-line"); expect(resolveInitialDiffType(jjContext, "all")).toBe("jj-current"); expect(resolveInitialDiffType(jjContext, "merge-base")).toBe("jj-current"); expect(resolveInitialDiffType(jjContext, "unstaged")).toBe("jj-current"); diff --git a/packages/shared/vcs-core.ts b/packages/shared/vcs-core.ts index 4a615e11b..4405baef8 100644 --- a/packages/shared/vcs-core.ts +++ b/packages/shared/vcs-core.ts @@ -1,3 +1,4 @@ +import type { VcsReviewSettingsDescriptor } from "@plannotator/core/config-types"; import { type DiffResult, type DiffType, @@ -112,6 +113,7 @@ export interface VcsSnapshot { export type VcsSelection = "auto" | "git" | "gitbutler" | "jj" | "p4"; export interface VcsApi { + getReviewSettings(): VcsReviewSettingsDescriptor[]; getReviewPolicy(vcsType?: VcsSelection): VcsReviewPolicy; resolveReviewDefault( vcsType: VcsSelection | undefined, @@ -463,7 +465,15 @@ export function createVcsApi( vcsType?: VcsSelection, ): Promise<{ provider: VcsProvider; gitContext: GitContext }> { const provider = await getProviderForSelection(vcsType, cwd); - return { provider, gitContext: await provider.getContext(cwd) }; + return { + provider, + gitContext: { + ...await provider.getContext(cwd), + reviewSettings: providerList.flatMap((candidate) => + candidate.reviewPolicy.settings ? [candidate.reviewPolicy.settings] : [] + ), + }, + }; } function resolveRequestedDiffType( @@ -480,6 +490,12 @@ export function createVcsApi( return { + getReviewSettings(): VcsReviewSettingsDescriptor[] { + return providerList.flatMap((provider) => + provider.reviewPolicy.settings ? [provider.reviewPolicy.settings] : [] + ); + }, + getReviewPolicy(vcsType): VcsReviewPolicy { if (!vcsType || vcsType === "auto") return defaultProvider.reviewPolicy; return getProviderById(vcsType)?.reviewPolicy ?? defaultProvider.reviewPolicy; diff --git a/packages/shared/vcs-review-policy.test.ts b/packages/shared/vcs-review-policy.test.ts index 2d39a8187..069310122 100644 --- a/packages/shared/vcs-review-policy.test.ts +++ b/packages/shared/vcs-review-policy.test.ts @@ -32,7 +32,7 @@ describe("provider-owned open-state validation", () => { expect(result.error).toContain("Unknown diff type: provider-mode"); }); - test("JJ owns its unsupported-open-state message", () => { + test("JJ owns its stable open modes and base-relative line mode", () => { const result = resolveReviewOpenState({ parsed: { base: "main" }, isPRMode: false, @@ -41,6 +41,10 @@ describe("provider-owned open-state validation", () => { resolvedDefaultDiffType: "jj-current", }); - expect(result.error).toContain("not supported in jj sessions yet"); + expect(result).toEqual({ + requestedBase: "main", + requestedDiffType: "jj-line", + notices: [], + }); }); }); diff --git a/packages/shared/vcs-review-policy.ts b/packages/shared/vcs-review-policy.ts index b97eda99e..85eedcff3 100644 --- a/packages/shared/vcs-review-policy.ts +++ b/packages/shared/vcs-review-policy.ts @@ -1,3 +1,4 @@ +import type { VcsReviewSettingsDescriptor } from "@plannotator/core/config-types"; import type { DiffType } from "./review-core"; import type { ProviderReviewOpenStateInput, @@ -10,6 +11,7 @@ export interface LegacyReviewDefaultAdapter { export interface VcsReviewPolicy { readonly defaultDiffType: DiffType; + readonly settings?: VcsReviewSettingsDescriptor; ownsDiffType(diffType: string): diffType is DiffType; readonly legacyDefault?: LegacyReviewDefaultAdapter; resolveDefault(value: unknown): DiffType | undefined; diff --git a/packages/ui/components/Settings.tsx b/packages/ui/components/Settings.tsx index f58a305bf..8191daab8 100644 --- a/packages/ui/components/Settings.tsx +++ b/packages/ui/components/Settings.tsx @@ -2,9 +2,17 @@ import React, { useState, useEffect, useMemo, useRef } from 'react'; import { createPortal } from 'react-dom'; import type { AnnotateAgentTerminalSide } from '@plannotator/core/agent-terminal'; import type { Origin } from '@plannotator/core/agents'; -import type { DiffLineBgIntensity } from '@plannotator/core/config-types'; +import type { DiffLineBgIntensity, VcsReviewSettingsDescriptor } from '@plannotator/core/config-types'; import type { TokenHoverDelay } from '@plannotator/core/token-hover'; -import { configStore, useConfigValue, setReviewPanelView, setReviewDefaultDiffType, setReviewAutoViewed } from '../config'; +import { + configStore, + useConfigValue, + setReviewPanelView, + setReviewDefaultDiffType, + getProviderReviewDefaultDiffType, + setProviderReviewDefaultDiffType, + setReviewAutoViewed, +} from '../config'; import { setWebMcpToolsEnabled, useWebMcpToolsEnabled } from '../webmcp/preference'; import { loadDiffFont } from '../utils/diffFonts'; import { TaterSpritePullup } from './TaterSpritePullup'; @@ -78,7 +86,7 @@ import { import { requestVimDocumentFocus } from '../hooks/useVimDocumentFocus'; import { AnalysisLayerToggle } from './AnalysisLayerToggle'; -type SettingsTab = 'general' | 'theme' | 'git' | 'display' | 'analysis' | 'saving' | 'labels' | 'vim' | 'shortcuts' | 'ai' | 'files' | 'obsidian' | 'bear' | 'octarine' | 'comments' | 'hooks'; +type SettingsTab = 'general' | 'theme' | 'review' | 'display' | 'analysis' | 'saving' | 'labels' | 'vim' | 'shortcuts' | 'ai' | 'files' | 'obsidian' | 'bear' | 'octarine' | 'comments' | 'hooks'; interface SettingsProps { taterMode: boolean; @@ -98,6 +106,10 @@ interface SettingsProps { * (base ref unresolvable) — the Git tab shows a note that the Git-status * preference can't take effect in THIS repo. */ sinceBaseUnavailable?: boolean; + /** Provider-owned defaults available in this review runtime. */ + reviewSettings?: VcsReviewSettingsDescriptor[]; + /** Provider active in the current review. */ + activeVcsId?: string; /** The host is rendering its compact touch shell (review only). Display * settings that the compact shell overrides for the session are hidden * there instead of silently editing the desktop preference. */ @@ -171,18 +183,6 @@ export const LINE_BG_INTENSITY_OPTIONS: { value: DiffLineBgIntensity; label: str { value: 'normal', label: 'Normal' }, { value: 'strong', label: 'Strong' }, ]; -const DEFAULT_DIFF_TYPE_OPTIONS = [ - // "All Changes" belongs to since-base (the flagship composite); uncommitted - // reverts to its plain name so the two stay distinguishable side by side. - { value: 'since-base' as const, label: 'All Changes (Recommended)', description: "Everything since your branch split from main — committed, uncommitted, and untracked" }, - { value: 'local-vs-remote' as const, label: 'Local vs Remote Branch', description: "Your local branch and working tree compared with its last-fetched remote-tracking branch" }, - { value: 'uncommitted' as const, label: 'Uncommitted', description: "Everything you've changed since your last commit" }, - { value: 'unstaged' as const, label: 'Unstaged', description: "Only changes you haven't staged yet" }, - { value: 'staged' as const, label: 'Staged', description: "Only changes you've staged for commit" }, - { value: 'merge-base' as const, label: 'Committed changes (PR view)', description: "Everything you've committed on this branch" }, - { value: 'all' as const, label: 'All Files (HEAD)', description: "Every tracked file at HEAD, shown as additions" }, -]; - const AGENT_TERMINAL_SIDE_OPTIONS: { value: AnnotateAgentTerminalSide; label: string }[] = [ { value: 'left', label: 'Left' }, { value: 'right', label: 'Right' }, @@ -396,87 +396,116 @@ function ReviewAnalysisTab() { ); } -const GitTab: React.FC<{ sinceBaseUnavailable?: boolean }> = ({ sinceBaseUnavailable }) => { - const defaultDiffType = useConfigValue('defaultDiffType'); +const ReviewSettingsTab: React.FC<{ + providers: VcsReviewSettingsDescriptor[]; + activeVcsId?: string; + sinceBaseUnavailable?: boolean; +}> = ({ providers, activeVcsId, sinceBaseUnavailable }) => { + useConfigValue('reviewDefaults'); + useConfigValue('defaultDiffType'); const reviewPanelView = useConfigValue('reviewPanelView'); const reviewAutoViewed = useConfigValue('reviewAutoViewed'); + const [selectedProviderId, setSelectedProviderId] = useState(() => + providers.some((provider) => provider.id === activeVcsId) + ? activeVcsId! + : providers[0]?.id ?? '' + ); + const provider = providers.find((candidate) => candidate.id === selectedProviderId) ?? providers[0]; + const defaultDiffType = provider ? getProviderReviewDefaultDiffType(provider) : ''; + + useEffect(() => { + if (activeVcsId && providers.some((candidate) => candidate.id === activeVcsId)) { + setSelectedProviderId(activeVcsId); + } + }, [activeVcsId, providers]); + return (
Viewed files
- {/* Never write `reviewAutoViewed` directly — setReviewAutoViewed also - consumes the first-time notice, since an explicit toggle is proof - the reviewer already found the switch. */} setReviewAutoViewed(v)} + onChange={setReviewAutoViewed} label="Auto-mark viewed" description="Mark a file viewed when you scroll past it or move on to another file. Files you un-view stay un-viewed, and files that change on refresh become un-viewed." />
-
-
-
Default review view
-
Which panel a code review opens in
- {/* This is a GLOBAL preference — never hide the options because the - CURRENT repo can't serve them; just say so. Without this note, - picking Git status on a repo whose base ref doesn't resolve - silently falls back to Tree and the setting looks broken. */} - {sinceBaseUnavailable && ( -
- Git status view isn't available in this repository (its base branch - couldn't be resolved) — reviews here open in Tree. The preference - still applies in repositories where it works. -
- )} + + {providers.length > 1 && provider && ( +
+
+ Defaults for +
+ ({ value: candidate.id, label: candidate.label }))} + value={provider.id} + onChange={setSelectedProviderId} + />
- {/* No Commits option here: the Commits view is session-only (entered - via the panel toggle) and is never the opening view. */} - -
-
-
-
Default Diff View
-
Which changes to show when you open a code review
-
-
- {DEFAULT_DIFF_TYPE_OPTIONS.map((opt) => ( - - ))} -
-
+ )} + + {provider?.capabilities.statusSections && ( +
+
+
Default review view
+
Which panel a code review opens in
+ {sinceBaseUnavailable && provider.id === activeVcsId && ( +
+ Git status view isn't available in this repository (its base branch + couldn't be resolved) — reviews here open in Tree. The preference + still applies in repositories where it works. +
+ )} +
+ +
+ )} + + {provider && ( +
+
+
Default diff view
+
Which changes to show when you open a code review
+
+
+ {provider.diffOptions.map((option) => ( + + ))} +
+
+ )}
); }; @@ -929,7 +958,7 @@ const CommentsTab: React.FC = () => { ); }; -export const Settings: React.FC = ({ taterMode, onTaterModeChange, onIdentityChange, origin, mode = 'plan', onUIPreferencesChange, externalOpen, onExternalClose, aiProviders = [], gitUser, sinceBaseUnavailable, isCompactTouchLayout = false, onDetectObsidianVaults, agentTerminalAvailable = false, webmcpAvailable = false }) => { +export const Settings: React.FC = ({ taterMode, onTaterModeChange, onIdentityChange, origin, mode = 'plan', onUIPreferencesChange, externalOpen, onExternalClose, aiProviders = [], gitUser, sinceBaseUnavailable, reviewSettings = [], activeVcsId, isCompactTouchLayout = false, onDetectObsidianVaults, agentTerminalAvailable = false, webmcpAvailable = false }) => { const webmcpTools = useWebMcpToolsEnabled(); const [showDialog, setShowDialog] = useState(false); const settingsWasOpenRef = useRef(false); @@ -1005,7 +1034,7 @@ export const Settings: React.FC = ({ taterMode, onTaterModeChange t.push({ id: 'labels', label: 'Labels' }); } if (mode === 'review') { - t.push({ id: 'git', label: 'Git' }); + t.push({ id: 'review', label: 'Review' }); t.push({ id: 'display', label: 'Editor' }); t.push({ id: 'analysis', label: 'Analysis' }); t.push({ id: 'comments', label: 'Comments' }); @@ -1573,9 +1602,13 @@ export const Settings: React.FC = ({ taterMode, onTaterModeChange {/* === THEME TAB === */} {activeTab === 'theme' && { setShowDialog(false); setThemePreview(true); }} />} - {/* === GIT TAB === */} - {activeTab === 'git' && mode === 'review' && ( - + {/* === REVIEW TAB === */} + {activeTab === 'review' && mode === 'review' && ( + )} {/* === DISPLAY TAB === */} diff --git a/packages/ui/config/index.ts b/packages/ui/config/index.ts index 9d1f0cd4b..d97defcdc 100644 --- a/packages/ui/config/index.ts +++ b/packages/ui/config/index.ts @@ -4,6 +4,7 @@ export { useConfigValue } from './useConfig'; export { setReviewPanelView, setReviewDefaultDiffType, + getProviderReviewDefaultDiffType, setProviderReviewDefaultDiffType, getPersistedReviewPanelView, setReviewAutoViewed, diff --git a/packages/ui/config/reviewPanelViewLastUsed.test.ts b/packages/ui/config/reviewPanelViewLastUsed.test.ts index eae188f83..2c3c7c941 100644 --- a/packages/ui/config/reviewPanelViewLastUsed.test.ts +++ b/packages/ui/config/reviewPanelViewLastUsed.test.ts @@ -2,7 +2,12 @@ import { afterEach, describe, expect, test } from 'bun:test'; import { resetStorageBackend, setStorageBackend } from '../utils/storage'; import { SETTINGS } from './settings'; import { ConfigStoreForTest } from './configStore'; -import { setReviewDefaultDiffType, setReviewPanelView } from './reviewView'; +import { + getProviderReviewDefaultDiffType, + setProviderReviewDefaultDiffType, + setReviewDefaultDiffType, + setReviewPanelView, +} from './reviewView'; function installMemoryBackend(): Map { const values = new Map(); @@ -75,6 +80,45 @@ describe('reviewPanelViewLastUsed setting', () => { expect(store.get('reviewPanelViewLastUsed')).toBe('tree'); }); + test('keeps provider defaults independent and falls back to provider metadata', () => { + installMemoryBackend(); + const store = makeStore(); + const jj = { + id: 'jj', + label: 'Jujutsu', + defaultDiffType: 'jj-current', + diffOptions: [ + { id: 'jj-current', label: 'Current', description: 'Current change' }, + { id: 'jj-line', label: 'Line', description: 'Line of work' }, + ], + capabilities: { statusSections: false, staging: false, compareTarget: true }, + }; + + expect(getProviderReviewDefaultDiffType(jj, store)).toBe('jj-current'); + setProviderReviewDefaultDiffType('jj', 'jj-line', store); + expect(getProviderReviewDefaultDiffType(jj, store)).toBe('jj-line'); + expect(store.get('defaultDiffType')).toBe('since-base'); + }); + + test('reads a provider-declared legacy setting without knowing the provider id', () => { + installMemoryBackend(); + const store = makeStore(); + store.set('defaultDiffType', 'merge-base'); + const descriptor = { + id: 'provider-with-legacy-setting', + label: 'Provider', + defaultDiffType: 'provider-default', + diffOptions: [ + { id: 'provider-default', label: 'Default', description: 'Default mode' }, + { id: 'merge-base', label: 'Legacy', description: 'Legacy mode' }, + ], + capabilities: { statusSections: false, staging: false, compareTarget: true }, + legacyDefaultSetting: 'defaultDiffType', + }; + + expect(getProviderReviewDefaultDiffType(descriptor, store)).toBe('merge-base'); + }); + test('local-vs-remote persists as a Tree-compatible default', () => { const values = installMemoryBackend(); const store = makeStore(); diff --git a/packages/ui/config/reviewView.ts b/packages/ui/config/reviewView.ts index 8d93ff09b..29d931f83 100644 --- a/packages/ui/config/reviewView.ts +++ b/packages/ui/config/reviewView.ts @@ -1,6 +1,7 @@ import { configStore } from './configStore'; import { SETTINGS } from './settings'; import { storage } from '../utils/storage'; +import type { VcsReviewSettingsDescriptor } from '@plannotator/core/config-types'; /** * The ONLY writers for the coupled setting pair (reviewPanelView, @@ -77,6 +78,21 @@ export function setReviewDefaultDiffType( } } +export function getProviderReviewDefaultDiffType( + descriptor: VcsReviewSettingsDescriptor, + store: PanelViewConfigStore = configStore, +): string { + const configured = store.get('reviewDefaults')[descriptor.id]?.defaultDiffType; + if (configured && descriptor.diffOptions.some((option) => option.id === configured)) { + return configured; + } + if (descriptor.legacyDefaultSetting) { + const legacy = store.get(descriptor.legacyDefaultSetting as 'defaultDiffType'); + if (descriptor.diffOptions.some((option) => option.id === legacy)) return legacy; + } + return descriptor.defaultDiffType; +} + export function setProviderReviewDefaultDiffType( providerId: string, value: string,