From b9686fba0f02e486a6a77f5c429e5603a45342b2 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 13 May 2026 05:22:35 -0700 Subject: [PATCH] fix(gitlab): persist unposted inline comments + split pr-provider from browser-safe pr-types Closes #680. Two changes that landed together because the persistence fix exposed a hidden architectural constraint. 1. GitLab inline comments: when one or more discussion POSTs failed (e.g. transient `i/o timeout`), the failed comment bodies were lost. Now `submitGlMRReview` writes them to `~/.plannotator/failed-comments/{host}-{project}-mr{iid}-{ts}.json` in both the all-fail and partial-fail branches. The throw-vs-warn split is preserved deliberately: all-fail throws so the UI retries from a clean state, partial-fail warns so the UI doesn't resubmit already-posted content. 2. Split `packages/shared/pr-provider.ts` into `pr-types.ts` (browser-safe types + pure label/URL helpers) and `pr-provider.ts` (server-only dispatch that imports pr-github / pr-gitlab). The review-editor browser bundle previously dragged pr-gitlab.ts in as dead code via static imports, which silently constrained the file to never use Node built-ins. Adding `fs`/`os`/`path` for (1) broke the review build until we routed browser imports to pr-types and left server callers on the now server-only pr-provider facade. Server-only `pr-provider.ts` re-exports `pr-types` so existing server-side imports keep working unchanged. --- apps/pi-extension/server/pr.ts | 16 +- apps/pi-extension/server/serverReview.ts | 8 +- apps/pi-extension/vendor.sh | 2 +- packages/review-editor/App.tsx | 4 +- .../review-editor/components/PRChecksTab.tsx | 2 +- .../components/PRCommentsTab.tsx | 2 +- .../review-editor/components/PRSelector.tsx | 2 +- .../review-editor/components/PRSummaryTab.tsx | 2 +- .../components/ReviewSidebar.tsx | 2 +- .../components/StackedPRLabel.tsx | 4 +- .../review-editor/dock/ReviewStateContext.tsx | 2 +- .../hooks/useAnnotationFactory.ts | 4 +- packages/review-editor/hooks/usePRContext.ts | 4 +- packages/review-editor/hooks/usePRSession.ts | 2 +- .../utils/exportFeedback.test.ts | 2 +- .../review-editor/utils/exportFeedback.ts | 4 +- packages/server/pr.ts | 38 +- packages/shared/package.json | 1 + packages/shared/pr-github.ts | 4 +- packages/shared/pr-gitlab.ts | 48 ++- packages/shared/pr-provider.test.ts | 2 +- packages/shared/pr-provider.ts | 334 +----------------- packages/shared/pr-stack.test.ts | 2 +- packages/shared/pr-stack.ts | 4 +- packages/shared/pr-types.ts | 326 +++++++++++++++++ packages/shared/worktree-pool.test.ts | 2 +- packages/shared/worktree-pool.ts | 2 +- 27 files changed, 441 insertions(+), 384 deletions(-) create mode 100644 packages/shared/pr-types.ts diff --git a/apps/pi-extension/server/pr.ts b/apps/pi-extension/server/pr.ts index 0c14b8132..0e7d595c8 100644 --- a/apps/pi-extension/server/pr.ts +++ b/apps/pi-extension/server/pr.ts @@ -5,6 +5,15 @@ import { spawn } from "node:child_process"; +import { + type PRMetadata, + type PRRef, + type PRReviewFileComment, + type PRRuntime, + type PRStackTree, + type PRListItem, + parsePRUrl as parsePRUrlCore, +} from "../generated/pr-types.js"; import { checkAuth as checkAuthCore, fetchPRContext as fetchPRContextCore, @@ -15,13 +24,6 @@ import { fetchPRList as fetchPRListCore, getUser as getUserCore, markPRFilesViewed as markPRFilesViewedCore, - type PRMetadata, - type PRRef, - type PRReviewFileComment, - type PRRuntime, - type PRStackTree, - type PRListItem, - parsePRUrl as parsePRUrlCore, submitPRReview as submitPRReviewCore, } from "../generated/pr-provider.js"; diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 85cda4172..8386cbb98 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -22,7 +22,7 @@ import { type PRMetadata, type PRReviewFileComment, prRefFromMetadata, -} from "../generated/pr-provider.js"; +} from "../generated/pr-types.js"; import { type DiffType, type GitContext, @@ -168,15 +168,15 @@ export async function startReviewServer(options: { ? getPRDiffScopeOptions(prMeta, !!(options.worktreePool || options.agentCwd)) : []; - let prListCache: import("../generated/pr-provider.js").PRListItem[] | null = null; + let prListCache: import("../generated/pr-types.js").PRListItem[] | null = null; let prListCacheTime = 0; const prSwitchCache = new Map(); if (isPRMode && prMeta) prSwitchCache.set(prMeta.url, { metadata: prMeta, rawPatch: options.rawPatch }); - const prStackTreeCache = new Map(); + const prStackTreeCache = new Map(); // Fetch full stack tree (best-effort — always try in PR mode so root PRs // that target the default branch can still discover descendant PRs) - let prStackTree: import("../generated/pr-provider.js").PRStackTree | null = null; + let prStackTree: import("../generated/pr-types.js").PRStackTree | null = null; if (prRef && prMeta) { try { prStackTree = await fetchPRStack(prRef, prMeta); diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index 684c420f1..205a185e4 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -6,7 +6,7 @@ cd "$(dirname "$0")" mkdir -p generated generated/ai/providers -for f in feedback-templates prompts review-core jj-core vcs-core review-args storage draft project pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file config external-annotation agent-jobs worktree worktree-pool html-to-markdown url-to-markdown tour annotate-args at-reference pfm-reminder improvement-hooks; do +for f in feedback-templates prompts review-core jj-core vcs-core review-args storage draft project pr-types pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file config external-annotation agent-jobs worktree worktree-pool html-to-markdown url-to-markdown tour annotate-args at-reference pfm-reminder improvement-hooks; do src="../../packages/shared/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/shared/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts" done diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 6dfdbb04c..aa11e0ced 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -13,7 +13,7 @@ import { GitHubIcon } from '@plannotator/ui/components/GitHubIcon'; import { GitLabIcon } from '@plannotator/ui/components/GitLabIcon'; import { RepoIcon } from '@plannotator/ui/components/RepoIcon'; import { PullRequestIcon } from '@plannotator/ui/components/PullRequestIcon'; -import { getPlatformLabel, getMRLabel, getMRNumberLabel, getDisplayRepo } from '@plannotator/shared/pr-provider'; +import { getPlatformLabel, getMRLabel, getMRNumberLabel, getDisplayRepo } from '@plannotator/shared/pr-types'; import { configStore, useConfigValue } from '@plannotator/ui/config'; import { loadDiffFont } from '@plannotator/ui/utils/diffFonts'; import { getAgentSwitchSettings, getEffectiveAgentName } from '@plannotator/ui/utils/agentSwitch'; @@ -70,7 +70,7 @@ import { } from './dock/reviewPanelTypes'; import type { DiffFile } from './types'; import type { DiffOption, WorktreeInfo, GitContext } from '@plannotator/shared/types'; -import type { PRMetadata } from '@plannotator/shared/pr-provider'; +import type { PRMetadata } from '@plannotator/shared/pr-types'; import type { PRDiffScope, PRDiffScopeOption, PRStackInfo, PRStackTree } from '@plannotator/shared/pr-stack'; import { altKey } from '@plannotator/ui/utils/platform'; import { TourDialog } from './components/tour/TourDialog'; diff --git a/packages/review-editor/components/PRChecksTab.tsx b/packages/review-editor/components/PRChecksTab.tsx index ff785d70e..3199b725d 100644 --- a/packages/review-editor/components/PRChecksTab.tsx +++ b/packages/review-editor/components/PRChecksTab.tsx @@ -1,5 +1,5 @@ import React, { useMemo } from 'react'; -import type { PRContext, PRCheck } from '@plannotator/shared/pr-provider'; +import type { PRContext, PRCheck } from '@plannotator/shared/pr-types'; interface PRChecksTabProps { context: PRContext; diff --git a/packages/review-editor/components/PRCommentsTab.tsx b/packages/review-editor/components/PRCommentsTab.tsx index 83abca350..c0fc16235 100644 --- a/packages/review-editor/components/PRCommentsTab.tsx +++ b/packages/review-editor/components/PRCommentsTab.tsx @@ -1,5 +1,5 @@ import React, { useMemo, useState, useEffect, useRef, useCallback } from 'react'; -import type { PRContext, PRComment, PRReview, PRReviewThread } from '@plannotator/shared/pr-provider'; +import type { PRContext, PRComment, PRReview, PRReviewThread } from '@plannotator/shared/pr-types'; import { MarkdownBody } from './PRSummaryTab'; import { CopyButton } from './CopyButton'; import { DiffHunkPreview } from './DiffHunkPreview'; diff --git a/packages/review-editor/components/PRSelector.tsx b/packages/review-editor/components/PRSelector.tsx index ac1c10827..baab535bf 100644 --- a/packages/review-editor/components/PRSelector.tsx +++ b/packages/review-editor/components/PRSelector.tsx @@ -2,7 +2,7 @@ import React, { useState, useCallback, useEffect, useMemo } from 'react'; import { SearchableSelect } from '@plannotator/ui/components/SearchableSelect'; import { PullRequestIcon } from '@plannotator/ui/components/PullRequestIcon'; import { getItem, setItem } from '@plannotator/ui/utils/storage'; -import type { PRListItem } from '@plannotator/shared/pr-provider'; +import type { PRListItem } from '@plannotator/shared/pr-types'; type PRItem = PRListItem; diff --git a/packages/review-editor/components/PRSummaryTab.tsx b/packages/review-editor/components/PRSummaryTab.tsx index 08cf426f3..a68ca7ede 100644 --- a/packages/review-editor/components/PRSummaryTab.tsx +++ b/packages/review-editor/components/PRSummaryTab.tsx @@ -2,7 +2,7 @@ import React, { useMemo, useRef, useEffect } from 'react'; import DOMPurify from 'dompurify'; import { parseMarkdownToBlocks } from '@plannotator/ui/utils/parser'; import { renderInlineMarkdown } from '../utils/renderInlineMarkdown'; -import type { PRContext, PRMetadata } from '@plannotator/shared/pr-provider'; +import type { PRContext, PRMetadata } from '@plannotator/shared/pr-types'; interface PRSummaryTabProps { context: PRContext; diff --git a/packages/review-editor/components/ReviewSidebar.tsx b/packages/review-editor/components/ReviewSidebar.tsx index ea1c4554c..ae2cb1461 100644 --- a/packages/review-editor/components/ReviewSidebar.tsx +++ b/packages/review-editor/components/ReviewSidebar.tsx @@ -10,7 +10,7 @@ import { renderInlineMarkdown } from '../utils/renderInlineMarkdown'; import { formatRelativeTime } from '../utils/formatRelativeTime'; import { AITab } from './AITab'; import { AgentsTab } from '@plannotator/ui/components/AgentsTab'; -import type { PRMetadata } from '@plannotator/shared/pr-provider'; +import type { PRMetadata } from '@plannotator/shared/pr-types'; import { OverlayScrollArea } from '@plannotator/ui/components/OverlayScrollArea'; import type { AIChatEntry } from '../hooks/useAIChat'; import type { AgentJobInfo, AgentCapabilities } from '@plannotator/ui/types'; diff --git a/packages/review-editor/components/StackedPRLabel.tsx b/packages/review-editor/components/StackedPRLabel.tsx index c5c4a1d6b..a953c8217 100644 --- a/packages/review-editor/components/StackedPRLabel.tsx +++ b/packages/review-editor/components/StackedPRLabel.tsx @@ -1,9 +1,9 @@ import React, { useState } from 'react'; import * as Popover from '@radix-ui/react-popover'; -import { getPlatformLabel } from '@plannotator/shared/pr-provider'; +import { getPlatformLabel } from '@plannotator/shared/pr-types'; import { buildMinimalStackTree } from '@plannotator/shared/pr-stack'; import { getItem, setItem } from '@plannotator/ui/utils/storage'; -import type { PRMetadata } from '@plannotator/shared/pr-provider'; +import type { PRMetadata } from '@plannotator/shared/pr-types'; import type { PRDiffScope, PRDiffScopeOption, PRStackInfo, PRStackTree, PRStackNode } from '@plannotator/shared/pr-stack'; interface StackedPRLabelProps { diff --git a/packages/review-editor/dock/ReviewStateContext.tsx b/packages/review-editor/dock/ReviewStateContext.tsx index 24b47c2e5..2d96a4547 100644 --- a/packages/review-editor/dock/ReviewStateContext.tsx +++ b/packages/review-editor/dock/ReviewStateContext.tsx @@ -4,7 +4,7 @@ import type { AgentJobInfo } from '@plannotator/ui/types'; import type { DiffFile } from '../types'; import type { AIChatEntry } from '../hooks/useAIChat'; import type { ReviewSearchMatch } from '../utils/reviewSearch'; -import type { PRMetadata, PRContext } from '@plannotator/shared/pr-provider'; +import type { PRMetadata, PRContext } from '@plannotator/shared/pr-types'; import type { PRDiffScope } from '@plannotator/shared/pr-stack'; import type { FeedbackDiffContext } from '../utils/exportFeedback'; diff --git a/packages/review-editor/hooks/useAnnotationFactory.ts b/packages/review-editor/hooks/useAnnotationFactory.ts index 8e7254708..0b16e1c4f 100644 --- a/packages/review-editor/hooks/useAnnotationFactory.ts +++ b/packages/review-editor/hooks/useAnnotationFactory.ts @@ -1,6 +1,6 @@ import { useMemo, useCallback } from 'react'; -import { getDisplayRepo } from '@plannotator/shared/pr-provider'; -import type { PRMetadata } from '@plannotator/shared/pr-provider'; +import { getDisplayRepo } from '@plannotator/shared/pr-types'; +import type { PRMetadata } from '@plannotator/shared/pr-types'; import type { PRDiffScope } from '@plannotator/shared/pr-stack'; import type { CodeAnnotation } from '@plannotator/ui/types'; diff --git a/packages/review-editor/hooks/usePRContext.ts b/packages/review-editor/hooks/usePRContext.ts index 30f4ef090..04651f2aa 100644 --- a/packages/review-editor/hooks/usePRContext.ts +++ b/packages/review-editor/hooks/usePRContext.ts @@ -1,6 +1,6 @@ import { useState, useRef, useCallback, useEffect } from 'react'; -import type { PRContext } from '@plannotator/shared/pr-provider'; -import type { PRMetadata } from '@plannotator/shared/pr-provider'; +import type { PRContext } from '@plannotator/shared/pr-types'; +import type { PRMetadata } from '@plannotator/shared/pr-types'; export function usePRContext(prMetadata: PRMetadata | null) { const [prContext, setPRContext] = useState(null); diff --git a/packages/review-editor/hooks/usePRSession.ts b/packages/review-editor/hooks/usePRSession.ts index a940c2612..3401aa549 100644 --- a/packages/review-editor/hooks/usePRSession.ts +++ b/packages/review-editor/hooks/usePRSession.ts @@ -1,5 +1,5 @@ import { useState, useCallback } from 'react'; -import type { PRMetadata } from '@plannotator/shared/pr-provider'; +import type { PRMetadata } from '@plannotator/shared/pr-types'; import type { PRDiffScope, PRDiffScopeOption, PRStackInfo, PRStackTree } from '@plannotator/shared/pr-stack'; export interface PRSessionState { diff --git a/packages/review-editor/utils/exportFeedback.test.ts b/packages/review-editor/utils/exportFeedback.test.ts index 9d9e0095e..ddcdd05fc 100644 --- a/packages/review-editor/utils/exportFeedback.test.ts +++ b/packages/review-editor/utils/exportFeedback.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect } from "bun:test"; import { exportReviewFeedback } from "./exportFeedback"; import type { CodeAnnotation } from "@plannotator/ui/types"; -import type { PRMetadata } from "@plannotator/shared/pr-provider"; +import type { PRMetadata } from "@plannotator/shared/pr-types"; const ann = (overrides: Partial = {}): CodeAnnotation => ({ id: "1", diff --git a/packages/review-editor/utils/exportFeedback.ts b/packages/review-editor/utils/exportFeedback.ts index 89a2ac1bd..5dbea9031 100644 --- a/packages/review-editor/utils/exportFeedback.ts +++ b/packages/review-editor/utils/exportFeedback.ts @@ -1,6 +1,6 @@ import type { CodeAnnotation, ConventionalLabel, ConventionalDecoration } from '@plannotator/ui/types'; -import type { PRMetadata } from '@plannotator/shared/pr-provider'; -import { getMRLabel, getMRNumberLabel, getDisplayRepo } from '@plannotator/shared/pr-provider'; +import type { PRMetadata } from '@plannotator/shared/pr-types'; +import { getMRLabel, getMRNumberLabel, getDisplayRepo } from '@plannotator/shared/pr-types'; /** * Format a conventional comment prefix per the Conventional Comments spec: diff --git a/packages/server/pr.ts b/packages/server/pr.ts index 7a6194b6d..20f547b86 100644 --- a/packages/server/pr.ts +++ b/packages/server/pr.ts @@ -5,15 +5,26 @@ * Pre-binds a Bun-based runtime so consumers get a clean API. */ +import type { + PRRef, + PRMetadata, + PRContext, + PRRuntime, + PRReviewFileComment, + PRStackTree, + PRListItem, +} from "@plannotator/shared/pr-types"; import { - type PRRef, - type PRMetadata, - type PRContext, - type PRRuntime, - type PRReviewFileComment, - type PRStackTree, - type PRListItem, parsePRUrl as parsePRUrlCore, + prRefFromMetadata, + getPlatformLabel, + getMRLabel, + getMRNumberLabel, + getDisplayRepo, + getCliName, + getCliInstallUrl, +} from "@plannotator/shared/pr-types"; +import { checkAuth as checkAuthCore, getUser as getUserCore, fetchPR as fetchPRCore, @@ -24,18 +35,11 @@ import { markPRFilesViewed as markPRFilesViewedCore, fetchPRStack as fetchPRStackCore, fetchPRList as fetchPRListCore, - prRefFromMetadata, - getPlatformLabel, - getMRLabel, - getMRNumberLabel, - getDisplayRepo, - getCliName, - getCliInstallUrl, } from "@plannotator/shared/pr-provider"; -export type { PRRef, PRMetadata, PRContext, PRReviewFileComment, PRStackTree, PRListItem } from "@plannotator/shared/pr-provider"; -export { prRefFromMetadata, isSameProject, getPlatformLabel, getMRLabel, getMRNumberLabel, getDisplayRepo, getCliName, getCliInstallUrl } from "@plannotator/shared/pr-provider"; -export type { GithubPRMetadata } from "@plannotator/shared/pr-provider"; +export type { PRRef, PRMetadata, PRContext, PRReviewFileComment, PRStackTree, PRListItem } from "@plannotator/shared/pr-types"; +export { prRefFromMetadata, isSameProject, getPlatformLabel, getMRLabel, getMRNumberLabel, getDisplayRepo, getCliName, getCliInstallUrl } from "@plannotator/shared/pr-types"; +export type { GithubPRMetadata } from "@plannotator/shared/pr-types"; const runtime: PRRuntime = { async runCommand(cmd, args) { diff --git a/packages/shared/package.json b/packages/shared/package.json index 9159965d4..e408fcf11 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -13,6 +13,7 @@ "./vcs-core": "./vcs-core.ts", "./checklist": "./checklist.ts", "./types": "./types.ts", + "./pr-types": "./pr-types.ts", "./pr-provider": "./pr-provider.ts", "./pr-stack": "./pr-stack.ts", "./project": "./project.ts", diff --git a/packages/shared/pr-github.ts b/packages/shared/pr-github.ts index 9778dd21a..0330a87e9 100644 --- a/packages/shared/pr-github.ts +++ b/packages/shared/pr-github.ts @@ -4,8 +4,8 @@ * All functions use the `gh` CLI via the PRRuntime abstraction. */ -import type { PRRuntime, PRMetadata, PRContext, PRReviewThread, PRThreadComment, PRReviewFileComment, CommandResult, PRStackTree, PRStackNode, PRListItem } from "./pr-provider"; -import { encodeApiFilePath } from "./pr-provider"; +import type { PRRuntime, PRMetadata, PRContext, PRReviewThread, PRThreadComment, PRReviewFileComment, CommandResult, PRStackTree, PRStackNode, PRListItem } from "./pr-types"; +import { encodeApiFilePath } from "./pr-types"; // GitHub-specific PRRef shape (used internally) interface GhPRRef { diff --git a/packages/shared/pr-gitlab.ts b/packages/shared/pr-gitlab.ts index b66cfb173..a539b66be 100644 --- a/packages/shared/pr-gitlab.ts +++ b/packages/shared/pr-gitlab.ts @@ -5,8 +5,11 @@ * Self-hosted instances are supported via the --hostname flag. */ -import type { PRRuntime, PRMetadata, PRContext, PRReviewFileComment, CommandResult } from "./pr-provider"; -import { encodeApiFilePath } from "./pr-provider"; +import { homedir } from "os"; +import { join } from "path"; +import { mkdirSync, writeFileSync } from "fs"; +import type { PRRuntime, PRMetadata, PRContext, PRReviewFileComment, CommandResult } from "./pr-types"; +import { encodeApiFilePath } from "./pr-types"; // GitLab-specific MRRef shape (used internally) interface GlMRRef { @@ -495,13 +498,42 @@ export async function submitGlMRReview( } } - if (errors.length > 0 && errors.length === fileComments.length) { - // All failed — throw - throw new Error(`Failed to post inline comments:\n${errors.join("\n")}`); - } - // Partial failures: some comments posted, some didn't — log but don't throw if (errors.length > 0) { - console.error(`Warning: ${errors.length}/${fileComments.length} inline comments failed:\n${errors.join("\n")}`); + // Persist unposted bodies to disk so the work survives transient GitLab errors. + // We keep the original throw-vs-warn split intentionally: + // - all-fail → throw (nothing was posted, caller retries from clean state) + // - partial-fail → warn only (some discussions + the MR note are already on + // the server; throwing would have the client re-submit the whole review + // and create duplicates). + const failed = results + .map((r, i) => (r.status === "rejected" ? fileComments[i] : null)) + .filter((c): c is PRReviewFileComment => c !== null); + let savedTo: string | null = null; + try { + const dir = join(homedir(), ".plannotator", "failed-comments"); + mkdirSync(dir, { recursive: true }); + const slug = `${ref.host}-${ref.projectPath.replace(/\//g, "_")}-mr${ref.iid}-${Date.now()}`; + savedTo = join(dir, `${slug}.json`); + writeFileSync( + savedTo, + JSON.stringify({ ref, headSha, baseSha, startSha, errors, failedComments: failed }, null, 2), + ); + } catch (writeErr) { + console.error(`[plannotator] Failed to persist unposted comments: ${writeErr instanceof Error ? writeErr.message : String(writeErr)}`); + } + const suffix = savedTo ? ` (unposted bodies saved to ${savedTo})` : ""; + + if (errors.length === fileComments.length) { + // All failed — safe to throw, nothing was posted. + throw new Error( + `Failed to post inline comments${suffix}:\n${errors.join("\n")}`, + ); + } + // Partial failure — some comments and the MR note are already posted. + // Don't throw, or the UI will resubmit the whole review and duplicate them. + console.error( + `[plannotator] ${errors.length}/${fileComments.length} inline comments failed${suffix}:\n${errors.join("\n")}`, + ); } } diff --git a/packages/shared/pr-provider.test.ts b/packages/shared/pr-provider.test.ts index f9a92de2a..21bdd2b47 100644 --- a/packages/shared/pr-provider.test.ts +++ b/packages/shared/pr-provider.test.ts @@ -11,7 +11,7 @@ import { prRefFromMetadata, type PRMetadata, type PRRef, -} from "./pr-provider"; +} from "./pr-types"; import { getPRDiffScopeOptions, getPRStackInfo, diff --git a/packages/shared/pr-provider.ts b/packages/shared/pr-provider.ts index 78da77df2..5680a7b91 100644 --- a/packages/shared/pr-provider.ts +++ b/packages/shared/pr-provider.ts @@ -1,332 +1,24 @@ /** - * Runtime-agnostic PR provider shared by Bun runtimes and Pi. + * Server-only PR/MR dispatch. * - * Dispatches to platform-specific implementations (GitHub, GitLab) - * based on the `platform` field in PRRef/PRMetadata. + * Picks GitHub vs GitLab from PRRef.platform and delegates to the + * platform implementation in pr-github.ts / pr-gitlab.ts. These + * implementations may use Node built-ins (fs, os, path) for things + * like persisting failed comments — they must never be imported + * from browser code. * - * Same pattern as review-core.ts: a runtime interface abstracts subprocess - * execution so the logic is reusable across Bun and Node/jiti. + * Pure types and label helpers live in pr-types.ts, which is + * browser-safe. */ import { checkGhAuth, getGhUser, fetchGhPR, fetchGhPRContext, fetchGhPRFileContent, submitGhPRReview, fetchGhPRViewedFiles, markGhFilesViewed, fetchGhPRStack, fetchGhPRList } from "./pr-github"; import { checkGlAuth, getGlUser, fetchGlMR, fetchGlMRContext, fetchGlFileContent, submitGlMRReview } from "./pr-gitlab"; +import type { PRRuntime, PRRef, PRMetadata, PRContext, PRReviewFileComment, PRStackTree, PRListItem } from "./pr-types"; -// --- Runtime Types --- - -export interface CommandResult { - stdout: string; - stderr: string; - exitCode: number; -} - -export interface PRRuntime { - runCommand: ( - cmd: string, - args: string[], - ) => Promise; - runCommandWithInput?: ( - cmd: string, - args: string[], - input: string, - ) => Promise; -} - -// --- Platform Types --- - -export type Platform = "github" | "gitlab"; - -/** GitHub PR reference */ -export interface GithubPRRef { - platform: "github"; - host: string; - owner: string; - repo: string; - number: number; -} - -/** GitLab MR reference */ -export interface GitlabMRRef { - platform: "gitlab"; - host: string; - projectPath: string; - iid: number; -} - -/** Discriminated union — auto-detected from URL */ -export type PRRef = GithubPRRef | GitlabMRRef; - -/** GitHub PR metadata */ -export interface GithubPRMetadata { - platform: "github"; - host: string; - owner: string; - repo: string; - number: number; - /** GraphQL node ID for the PR — used for markFileAsViewed mutations */ - prNodeId?: string; - title: string; - author: string; - baseBranch: string; - headBranch: string; - /** Repository default branch, used to infer whether this PR targets another PR branch. */ - defaultBranch?: string; - baseSha: string; - headSha: string; - /** Merge-base SHA — the common ancestor commit used to compute the PR diff. Differs from baseSha when the base branch has moved. */ - mergeBaseSha?: string; - url: string; -} - -/** GitLab MR metadata */ -export interface GitlabMRMetadata { - platform: "gitlab"; - host: string; - projectPath: string; - iid: number; - title: string; - author: string; - baseBranch: string; - headBranch: string; - /** Project default branch, used to infer whether this MR targets another MR branch. */ - defaultBranch?: string; - baseSha: string; - headSha: string; - /** Merge-base SHA — the common ancestor commit used to compute the MR diff. */ - mergeBaseSha?: string; - url: string; -} - -/** Discriminated union — downstream gets type narrowing for free */ -export type PRMetadata = GithubPRMetadata | GitlabMRMetadata; - -// --- PR Context Types (platform-agnostic) --- - -export interface PRComment { - id: string; - author: string; - body: string; - createdAt: string; - url: string; -} - -export interface PRReview { - id: string; - author: string; - state: string; - body: string; - submittedAt: string; - url?: string; -} - -export interface PRCheck { - name: string; - status: string; - conclusion: string | null; - workflowName: string; - detailsUrl: string; -} - -export interface PRLinkedIssue { - number: number; - url: string; - repo: string; -} - -export interface PRThreadComment { - id: string; - author: string; - body: string; - createdAt: string; - url: string; - diffHunk?: string; -} - -export interface PRReviewThread { - id: string; - isResolved: boolean; - isOutdated: boolean; - path: string; - line: number | null; - startLine: number | null; - diffSide: 'LEFT' | 'RIGHT' | null; - comments: PRThreadComment[]; -} - -export interface PRContext { - body: string; - state: string; - isDraft: boolean; - labels: Array<{ name: string; color: string }>; - reviewDecision: string; - mergeable: string; - mergeStateStatus: string; - comments: PRComment[]; - reviews: PRReview[]; - reviewThreads: PRReviewThread[]; - checks: PRCheck[]; - linkedIssues: PRLinkedIssue[]; -} - -export interface PRReviewFileComment { - path: string; - line: number; - side: "LEFT" | "RIGHT"; - body: string; - start_line?: number; - start_side?: "LEFT" | "RIGHT"; -} - -export type PRDiffScope = "layer" | "full-stack"; - -export interface PRDiffScopeOption { - id: PRDiffScope; - label: string; - description: string; - enabled: boolean; -} - -export interface PRStackInfo { - isStacked: boolean; - baseBranch: string; - defaultBranch?: string; - label: string; - source: "branch-inferred" | "tree-discovered" | "github-native" | "gitlab-native" | "graphite" | "ghstack"; -} - -export interface PRStackNode { - branch: string; - number?: number; - title?: string; - url?: string; - isCurrent: boolean; - isDefaultBranch: boolean; - state?: 'open' | 'merged' | 'closed'; -} - -export interface PRStackTree { - nodes: PRStackNode[]; -} - -export interface PRListItem { - id: string; - number: number; - title: string; - author: string; - url: string; - baseBranch: string; - state: 'open' | 'closed' | 'merged'; -} - -// --- Label Helpers --- -// Accept either PRRef or PRMetadata (both have `platform` discriminant) - -type HasPlatform = PRRef | PRMetadata; - -/** "GitHub" or "GitLab" */ -export function getPlatformLabel(m: HasPlatform): string { - return m.platform === "github" ? "GitHub" : "GitLab"; -} - -/** "PR" or "MR" */ -export function getMRLabel(m: HasPlatform): string { - return m.platform === "github" ? "PR" : "MR"; -} - -/** "#123" or "!42" */ -export function getMRNumberLabel(m: HasPlatform): string { - if (m.platform === "github") return `#${m.number}`; - return `!${m.iid}`; -} - -/** "owner/repo" or "group/project" */ -export function getDisplayRepo(m: HasPlatform): string { - if (m.platform === "github") return `${m.owner}/${m.repo}`; - return m.projectPath; -} - -/** Reconstruct a PRRef from metadata */ -export function prRefFromMetadata(m: PRMetadata): PRRef { - if (m.platform === "github") { - return { platform: "github", host: m.host, owner: m.owner, repo: m.repo, number: m.number }; - } - return { platform: "gitlab", host: m.host, projectPath: m.projectPath, iid: m.iid }; -} - -export function isSameProject(a: PRRef, b: PRRef): boolean { - if (a.platform !== b.platform) return false; - if (a.platform === "github" && b.platform === "github") { - return a.host === b.host && a.owner === b.owner && a.repo === b.repo; - } - if (a.platform === "gitlab" && b.platform === "gitlab") { - return a.host === b.host && a.projectPath === b.projectPath; - } - return false; -} - -/** CLI tool name for the platform */ -export function getCliName(ref: PRRef): string { - return ref.platform === "github" ? "gh" : "glab"; -} - -/** Install URL for the platform CLI */ -export function getCliInstallUrl(ref: PRRef): string { - return ref.platform === "github" - ? "https://cli.github.com" - : "https://gitlab.com/gitlab-org/cli"; -} - -/** Encode a file path for use in platform API URLs */ -export function encodeApiFilePath(filePath: string): string { - return encodeURIComponent(filePath); -} - -// --- URL Parsing --- - -/** - * Parse a PR/MR URL into its components. Auto-detects platform. - * - * Handles: - * - GitHub: https://github.com/owner/repo/pull/123[/files|/commits] - * - GitHub Enterprise: https://ghe.company.com/owner/repo/pull/123 - * - GitLab: https://gitlab.com/group/subgroup/project/-/merge_requests/42[/diffs] - * - Self-hosted GitLab: https://gitlab.mycompany.com/group/project/-/merge_requests/42 - * - * GitLab is checked first because `/-/merge_requests/` is unambiguous, - * while `/pull/` could theoretically appear on any host. - */ -export function parsePRUrl(url: string): PRRef | null { - if (!url) return null; - - // GitLab: https://{host}/{projectPath}/-/merge_requests/{iid}[/...] - // Checked first — `/-/merge_requests/` is the most specific pattern. - const glMatch = url.match( - /^https?:\/\/([^/]+)\/(.+?)\/-\/merge_requests\/(\d+)/, - ); - if (glMatch) { - return { - platform: "gitlab", - host: glMatch[1], - projectPath: glMatch[2], - iid: parseInt(glMatch[3], 10), - }; - } - - // GitHub (including GHE): https://{host}/{owner}/{repo}/pull/{number}[/...] - const ghMatch = url.match( - /^https?:\/\/([^/]+)\/([^/]+)\/([^/]+)\/pull\/(\d+)/, - ); - if (ghMatch) { - return { - platform: "github", - host: ghMatch[1], - owner: ghMatch[2], - repo: ghMatch[3], - number: parseInt(ghMatch[4], 10), - }; - } - - return null; -} +// Re-export the browser-safe surface so server callers can keep using +// pr-provider as a single facade. Browser code imports from pr-types +// directly to avoid pulling pr-github / pr-gitlab into the client bundle. +export * from "./pr-types"; // --- Dispatch Functions --- diff --git a/packages/shared/pr-stack.test.ts b/packages/shared/pr-stack.test.ts index b664cd526..040d0cbcd 100644 --- a/packages/shared/pr-stack.test.ts +++ b/packages/shared/pr-stack.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import type { PRMetadata } from "./pr-provider"; +import type { PRMetadata } from "./pr-types"; import type { GitCommandResult, ReviewGitRuntime } from "./review-core"; import { runPRFullStackDiff } from "./pr-stack"; diff --git a/packages/shared/pr-stack.ts b/packages/shared/pr-stack.ts index 2c249303a..cb313ddb6 100644 --- a/packages/shared/pr-stack.ts +++ b/packages/shared/pr-stack.ts @@ -5,8 +5,8 @@ import type { PRStackInfo, PRStackTree, PRStackNode, -} from "./pr-provider"; -export type { PRDiffScope, PRDiffScopeOption, PRStackInfo, PRStackTree, PRStackNode } from "./pr-provider"; +} from "./pr-types"; +export type { PRDiffScope, PRDiffScopeOption, PRStackInfo, PRStackTree, PRStackNode } from "./pr-types"; function branchNameIsSafe(branch: string): boolean { return branch.trim().length > 0 && !branch.startsWith("-") && !branch.includes("\0"); diff --git a/packages/shared/pr-types.ts b/packages/shared/pr-types.ts new file mode 100644 index 000000000..af2388bc5 --- /dev/null +++ b/packages/shared/pr-types.ts @@ -0,0 +1,326 @@ +/** + * Browser-safe PR/MR types and pure helpers. + * + * Split out from pr-provider.ts so the review UI can import types, + * label helpers, and URL parsing without dragging the GitHub/GitLab + * server implementations (and their Node-only dependencies) through + * the browser bundle. pr-provider.ts re-exports nothing from here; + * server-side dispatch lives there. + */ + +// --- Runtime Types --- + +export interface CommandResult { + stdout: string; + stderr: string; + exitCode: number; +} + +export interface PRRuntime { + runCommand: ( + cmd: string, + args: string[], + ) => Promise; + runCommandWithInput?: ( + cmd: string, + args: string[], + input: string, + ) => Promise; +} + +// --- Platform Types --- + +export type Platform = "github" | "gitlab"; + +/** GitHub PR reference */ +export interface GithubPRRef { + platform: "github"; + host: string; + owner: string; + repo: string; + number: number; +} + +/** GitLab MR reference */ +export interface GitlabMRRef { + platform: "gitlab"; + host: string; + projectPath: string; + iid: number; +} + +/** Discriminated union — auto-detected from URL */ +export type PRRef = GithubPRRef | GitlabMRRef; + +/** GitHub PR metadata */ +export interface GithubPRMetadata { + platform: "github"; + host: string; + owner: string; + repo: string; + number: number; + /** GraphQL node ID for the PR — used for markFileAsViewed mutations */ + prNodeId?: string; + title: string; + author: string; + baseBranch: string; + headBranch: string; + /** Repository default branch, used to infer whether this PR targets another PR branch. */ + defaultBranch?: string; + baseSha: string; + headSha: string; + /** Merge-base SHA — the common ancestor commit used to compute the PR diff. Differs from baseSha when the base branch has moved. */ + mergeBaseSha?: string; + url: string; +} + +/** GitLab MR metadata */ +export interface GitlabMRMetadata { + platform: "gitlab"; + host: string; + projectPath: string; + iid: number; + title: string; + author: string; + baseBranch: string; + headBranch: string; + /** Project default branch, used to infer whether this MR targets another MR branch. */ + defaultBranch?: string; + baseSha: string; + headSha: string; + /** Merge-base SHA — the common ancestor commit used to compute the MR diff. */ + mergeBaseSha?: string; + url: string; +} + +/** Discriminated union — downstream gets type narrowing for free */ +export type PRMetadata = GithubPRMetadata | GitlabMRMetadata; + +// --- PR Context Types (platform-agnostic) --- + +export interface PRComment { + id: string; + author: string; + body: string; + createdAt: string; + url: string; +} + +export interface PRReview { + id: string; + author: string; + state: string; + body: string; + submittedAt: string; + url?: string; +} + +export interface PRCheck { + name: string; + status: string; + conclusion: string | null; + workflowName: string; + detailsUrl: string; +} + +export interface PRLinkedIssue { + number: number; + url: string; + repo: string; +} + +export interface PRThreadComment { + id: string; + author: string; + body: string; + createdAt: string; + url: string; + diffHunk?: string; +} + +export interface PRReviewThread { + id: string; + isResolved: boolean; + isOutdated: boolean; + path: string; + line: number | null; + startLine: number | null; + diffSide: 'LEFT' | 'RIGHT' | null; + comments: PRThreadComment[]; +} + +export interface PRContext { + body: string; + state: string; + isDraft: boolean; + labels: Array<{ name: string; color: string }>; + reviewDecision: string; + mergeable: string; + mergeStateStatus: string; + comments: PRComment[]; + reviews: PRReview[]; + reviewThreads: PRReviewThread[]; + checks: PRCheck[]; + linkedIssues: PRLinkedIssue[]; +} + +export interface PRReviewFileComment { + path: string; + line: number; + side: "LEFT" | "RIGHT"; + body: string; + start_line?: number; + start_side?: "LEFT" | "RIGHT"; +} + +export type PRDiffScope = "layer" | "full-stack"; + +export interface PRDiffScopeOption { + id: PRDiffScope; + label: string; + description: string; + enabled: boolean; +} + +export interface PRStackInfo { + isStacked: boolean; + baseBranch: string; + defaultBranch?: string; + label: string; + source: "branch-inferred" | "tree-discovered" | "github-native" | "gitlab-native" | "graphite" | "ghstack"; +} + +export interface PRStackNode { + branch: string; + number?: number; + title?: string; + url?: string; + isCurrent: boolean; + isDefaultBranch: boolean; + state?: 'open' | 'merged' | 'closed'; +} + +export interface PRStackTree { + nodes: PRStackNode[]; +} + +export interface PRListItem { + id: string; + number: number; + title: string; + author: string; + url: string; + baseBranch: string; + state: 'open' | 'closed' | 'merged'; +} + +// --- Label Helpers --- +// Accept either PRRef or PRMetadata (both have `platform` discriminant) + +type HasPlatform = PRRef | PRMetadata; + +/** "GitHub" or "GitLab" */ +export function getPlatformLabel(m: HasPlatform): string { + return m.platform === "github" ? "GitHub" : "GitLab"; +} + +/** "PR" or "MR" */ +export function getMRLabel(m: HasPlatform): string { + return m.platform === "github" ? "PR" : "MR"; +} + +/** "#123" or "!42" */ +export function getMRNumberLabel(m: HasPlatform): string { + if (m.platform === "github") return `#${m.number}`; + return `!${m.iid}`; +} + +/** "owner/repo" or "group/project" */ +export function getDisplayRepo(m: HasPlatform): string { + if (m.platform === "github") return `${m.owner}/${m.repo}`; + return m.projectPath; +} + +/** Reconstruct a PRRef from metadata */ +export function prRefFromMetadata(m: PRMetadata): PRRef { + if (m.platform === "github") { + return { platform: "github", host: m.host, owner: m.owner, repo: m.repo, number: m.number }; + } + return { platform: "gitlab", host: m.host, projectPath: m.projectPath, iid: m.iid }; +} + +export function isSameProject(a: PRRef, b: PRRef): boolean { + if (a.platform !== b.platform) return false; + if (a.platform === "github" && b.platform === "github") { + return a.host === b.host && a.owner === b.owner && a.repo === b.repo; + } + if (a.platform === "gitlab" && b.platform === "gitlab") { + return a.host === b.host && a.projectPath === b.projectPath; + } + return false; +} + +/** CLI tool name for the platform */ +export function getCliName(ref: PRRef): string { + return ref.platform === "github" ? "gh" : "glab"; +} + +/** Install URL for the platform CLI */ +export function getCliInstallUrl(ref: PRRef): string { + return ref.platform === "github" + ? "https://cli.github.com" + : "https://gitlab.com/gitlab-org/cli"; +} + +/** Encode a file path for use in platform API URLs */ +export function encodeApiFilePath(filePath: string): string { + return encodeURIComponent(filePath); +} + +// --- URL Parsing --- + +/** + * Parse a PR/MR URL into its components. Auto-detects platform. + * + * Handles: + * - GitHub: https://github.com/owner/repo/pull/123[/files|/commits] + * - GitHub Enterprise: https://ghe.company.com/owner/repo/pull/123 + * - GitLab: https://gitlab.com/group/subgroup/project/-/merge_requests/42[/diffs] + * - Self-hosted GitLab: https://gitlab.mycompany.com/group/project/-/merge_requests/42 + * + * GitLab is checked first because `/-/merge_requests/` is unambiguous, + * while `/pull/` could theoretically appear on any host. + */ +export function parsePRUrl(url: string): PRRef | null { + if (!url) return null; + + // GitLab: https://{host}/{projectPath}/-/merge_requests/{iid}[/...] + // Checked first — `/-/merge_requests/` is the most specific pattern. + const glMatch = url.match( + /^https?:\/\/([^/]+)\/(.+?)\/-\/merge_requests\/(\d+)/, + ); + if (glMatch) { + return { + platform: "gitlab", + host: glMatch[1], + projectPath: glMatch[2], + iid: parseInt(glMatch[3], 10), + }; + } + + // GitHub (including GHE): https://{host}/{owner}/{repo}/pull/{number}[/...] + const ghMatch = url.match( + /^https?:\/\/([^/]+)\/([^/]+)\/([^/]+)\/pull\/(\d+)/, + ); + if (ghMatch) { + return { + platform: "github", + host: ghMatch[1], + owner: ghMatch[2], + repo: ghMatch[3], + number: parseInt(ghMatch[4], 10), + }; + } + + return null; +} diff --git a/packages/shared/worktree-pool.test.ts b/packages/shared/worktree-pool.test.ts index a8fd42c63..63773042a 100644 --- a/packages/shared/worktree-pool.test.ts +++ b/packages/shared/worktree-pool.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import type { ReviewGitRuntime } from "./review-core"; -import type { PRMetadata } from "./pr-provider"; +import type { PRMetadata } from "./pr-types"; import { createWorktreePool } from "./worktree-pool"; function fakeRuntime(): { runtime: ReviewGitRuntime; commands: string[][] } { diff --git a/packages/shared/worktree-pool.ts b/packages/shared/worktree-pool.ts index 706f6254b..72b1effd1 100644 --- a/packages/shared/worktree-pool.ts +++ b/packages/shared/worktree-pool.ts @@ -11,7 +11,7 @@ import { join } from "node:path"; import type { ReviewGitRuntime } from "./review-core"; -import type { PRMetadata } from "./pr-provider"; +import type { PRMetadata } from "./pr-types"; import { createWorktree, removeWorktree, fetchRef, ensureObjectAvailable } from "./worktree"; export interface PoolEntry {