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: () => {