From 228f0e2807c5b59d2aa129cf2d80b9777ebabf07 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Tue, 6 Oct 2026 16:03:13 -0700 Subject: [PATCH] fix(ui): make pull request review waits visible (#15375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip lets people supervise agent work and review its outputs. > - A task can wait for a human to review a pull request while an agent schedules checks. > - The PR work product and detected external links use separate display paths. > - A private PR can disappear from the useful sidebar view when its provider lookup fails. > - The monitor countdown says when the next check runs but omits the saved review request. > - This change shows the saved PR and requested review in the task's existing surfaces. ## Linked Issues or Issue Description **What happened?** A task kept checking whether a private GitHub PR had merged. Its work product requested board review, but the Properties sidebar used detected external objects instead. The PR URL was stored only in metadata, which the PR refresh path did not read. The composer showed a generic monitor countdown with no PR link or review action. **Expected behavior** Show saved PR links even if provider access fails. During a GitHub monitor wait, make outstanding review requests visible with the next check time and a status-check action. Stop requesting review when a PR merges or closes. **Steps to reproduce** 1. Register a PR work product with `reviewState: needs_board_review` and its URL in `metadata.url`. 2. Schedule an external-service monitor for GitHub. 3. Open the task with external-object lookup unavailable or unable to access the private repository. 4. Inspect Properties and the composer wait strip. Related work: #14469 added rich artifact cards. #8759 addresses attention on monitored task blockers. This change uses the existing work products and monitor action; it adds no blocker or approval mechanism. ## What Changed - Show saved PRs in Properties, including metadata-only links, independent of external-object availability. - Deduplicate equivalent GitHub PR URLs while preserving the saved navigation link, and put explicit review requests first. - Keep provider status and freshness on the combined PR row; keep private PR links usable when lookup fails. - Show the saved PR review request above the composer and in the existing monitor banner during a GitHub monitor wait. - Label the existing monitor action `Check status` and show check failures inline. - Suppress review prompts for merged, closed, or archived PRs even if their review flag is stale. - Refresh PR metadata using `metadata.url` and the existing `repository` alias. - Share saved work-product reads across the thread, Properties, and Artifacts. Refresh GitHub in a separate query and enrich only matching PR versions. - Start a fresh saved-row request on live invalidation so late provider responses cannot hide new artifacts or changed review requests. - Cover stalled GitHub lookups in both panels so refresh latency cannot hide saved work. - Scope monitor-check mutation state to the task so failures and late responses do not leak across navigation. - Refresh GitHub status when the displayed run finishes, including when saved PR rows have not changed. - Clarify that PR review and external release handoffs need a saved human-input interaction with an agent assigned for continuation; a flag, monitor, or handoff comment alone does not create that card. ## Verification - 386 tests pass across eleven affected UI and server suites on `184c7d8746`. Regressions cover saved links, provider status, cold-cache loading, panel reopening, monitor errors during navigation, and live updates during provider refresh. - Three live-update regressions and two run-completion regressions fail before their fixes and pass afterward. These tests use the real API client's GET coalescing and abort handling. - Full repository typecheck and build passed on the merged parent `1a49112dd2`. The latest UI changes pass UI typecheck and token gates; CI also verifies the latest build. Capability contract/inventory checks pass. - Storybook build and browser checks passed before the query race fix. Browser checks cover desktop and 390px mobile review waits, plus video, mixed-file, and empty artifact galleries after background refresh. - The earlier full local `pnpm test:run` was stopped under disk pressure. It reported failures outside the changed suites; an isolated skill-cache run reproduced three existing macOS permission failures. The full local suite was not rerun for this follow-up. - Latest-head CI passes on `184c7d8746`, including the browser shards and canary dry run. Apex is 5/5 with no actionable findings and no unresolved threads. All eight reported findings are addressed. ## Risks - This uses saved PR review state. When GitHub access fails, the saved state can remain stale until the agent updates it. The link remains visible and the status check remains available. - `Check status` wakes the existing monitor owner. It does not merge a PR, accept an approval, or mark the task done. - No schema, permissions, or scheduler behavior changes. ## Model Used OpenAI GPT-6 via Codex, with reasoning, repository analysis, code editing, and test execution. The exact deployment model ID and context window were not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass — 277 affected tests - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/execution-semantics.md | 14 ++ server/src/__tests__/work-products.test.ts | 21 +++ server/src/services/work-products.ts | 4 +- skills/paperclip/SKILL.md | 18 +++ ui/src/api/issues.ts | 12 +- ui/src/components/IssueMonitorBanner.test.tsx | 33 +++- ui/src/components/IssueMonitorBanner.tsx | 54 +++++-- ui/src/components/IssueProperties.test.tsx | 117 ++++++++++++++ ui/src/components/IssuePullRequestLinks.tsx | 51 ++++++ .../issue-properties/IssueProperties.tsx | 28 +++- .../IssuePropertiesArtifactsTab.tsx | 6 +- ...ssuePropertiesDocumentAnnotations.test.tsx | 1 + ...ssuePropertiesMarkdownWorkProduct.test.tsx | 19 ++- ui/src/context/LiveUpdatesProvider.test.ts | 4 + ui/src/context/LiveUpdatesProvider.tsx | 1 + ui/src/hooks/useIssueWorkProducts.test.tsx | 150 ++++++++++++++++++ ui/src/hooks/useIssueWorkProducts.ts | 46 ++++++ ui/src/lib/issue-pull-requests.test.ts | 51 ++++++ ui/src/lib/issue-pull-requests.ts | 69 ++++++++ ui/src/lib/queryKeys.ts | 2 + ui/src/pages/IssueDetail.test.tsx | 33 ++++ ui/src/pages/IssueDetail.tsx | 35 +--- .../stories/task-artifact-gallery.stories.tsx | 24 ++- 23 files changed, 734 insertions(+), 59 deletions(-) create mode 100644 ui/src/components/IssuePullRequestLinks.tsx create mode 100644 ui/src/hooks/useIssueWorkProducts.test.tsx create mode 100644 ui/src/hooks/useIssueWorkProducts.ts create mode 100644 ui/src/lib/issue-pull-requests.test.ts create mode 100644 ui/src/lib/issue-pull-requests.ts diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 6377a142b8a..30b9f5a02bf 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -375,6 +375,20 @@ Before a heartbeat finalizes, its issue disposition must therefore be evaluated If useful deliverable work can continue without the external result, the agent should continue that work or delegate it rather than parking the issue. Use `blocked` only for a real dependency that prevents productive progress. Use a monitor when the assignee owns a bounded future check, and use delegated child work when another owner can make progress independently. +Saved pull-request work products remain linked in task properties even when +external-object detection or provider access is unavailable. Live updates read +saved rows independently of GitHub refreshes. A delayed provider response only +enriches matching PR versions; it cannot replace the saved work-product list. +During a scheduled +GitHub monitor wait, an outstanding `needs_board_review` PR also appears beside +the composer with its link and the next check time. **Check status** invokes the +existing bounded monitor check; it does not merge the PR or attest that it merged. +Merged, closed, and archived PRs do not request review even when their saved review +flag is stale. This display is not an approval gate or a new execution path. +When work actually needs a human answer, the agent must still create the existing +durable interaction and leave the task `in_review` rather than relying on a PR +review flag or a monitor comment to request that answer. + Recovery from an invalid external wait is bounded and idempotent: 1. Record bounded evidence that the completed heartbeat left no durable action path, including the terminal run and any reported local watcher metadata without treating that metadata as liveness. diff --git a/server/src/__tests__/work-products.test.ts b/server/src/__tests__/work-products.test.ts index c7f80f53942..476e1156184 100644 --- a/server/src/__tests__/work-products.test.ts +++ b/server/src/__tests__/work-products.test.ts @@ -110,6 +110,27 @@ describe("workProductService", () => { expect(product.metadata.state).toBe("open"); }); + it.each([ + { url: "https://github.com/example/private-repo/pull/42" }, + { repository: "example/private-repo", number: 42 }, + ])("refreshes a PR whose identity is stored only in metadata: %j", async (metadata) => { + const product = createWorkProductRow({ url: null, metadata }) as any; + const resolve = vi.fn(async () => ({ state: "merged" as const, workProductState: "merged" as const, headRef: null, headSha: null })); + const [refreshed] = await refreshPullRequestWorkProductMetadata([product], resolve); + expect(resolve).toHaveBeenCalledWith("company-1", { + host: "github.com", owner: "example", repo: "private-repo", number: 42, + }); + expect(refreshed.metadata).toMatchObject({ state: "merged" }); + expect(product.metadata).toEqual(metadata); + }); + + it("retains the saved PR when the provider cannot access it", async () => { + const product = createWorkProductRow({ url: null, metadata: { url: "https://github.com/example/private-repo/pull/42" } }) as any; + const resolve = vi.fn(async () => { throw new Error("Not found"); }); + const [refreshed] = await refreshPullRequestWorkProductMetadata([product], resolve); + expect(refreshed).toBe(product); + }); + it("resolves GitHub commit stats when runner diff events are unavailable", async () => { const resolveCommitDetails = vi.fn(async () => ({ additions: 13, deletions: 2, changedFiles: 3 })); const svc = workProductService({} as any, { resolveCommitDetails }); diff --git a/server/src/services/work-products.ts b/server/src/services/work-products.ts index 6e6c40a1a9e..0f3dc35ae2e 100644 --- a/server/src/services/work-products.ts +++ b/server/src/services/work-products.ts @@ -92,10 +92,12 @@ export async function refreshPullRequestWorkProductMetadata( return await Promise.all(products.map(async (product) => { if (product.type !== "pull_request") return product; const metadata = product.metadata ?? {}; - const repo = typeof metadata.repo === "string" ? metadata.repo : null; + const repo = typeof metadata.repo === "string" ? metadata.repo + : typeof metadata.repository === "string" ? metadata.repository : null; const number = nonNegativeInteger(metadata.number); const references = extractGitHubPullRequestReferences([ product.url, + typeof metadata.url === "string" ? metadata.url : null, repo && number ? `${repo}#${number}` : null, ]); const reference = references[0]; diff --git a/skills/paperclip/SKILL.md b/skills/paperclip/SKILL.md index 9b887c5d8bc..c7a3cc11e48 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -759,3 +759,21 @@ improvement. This is not a mandatory report. Their shared helper is `scripts/submit-agent-commentary.mjs`, relative to this skill directory. Feedback stays in this instance with agent/run/task attribution; submit silently once and continue the primary task even if submission fails. + +**External review handoffs.** + +For a PR, put its link in the work product's top-level `url` field. If a human +must review or merge it before you can continue, name that action in a durable +human-only interaction with an appropriate continuation policy and leave the +task `in_review`. A `needs_board_review` work-product flag and a periodic monitor +do not create an interaction card. Keep any merge check bounded, record its +purpose in the monitor's `notes`, and verify the actual provider state when you +resume; a confirmation response is not proof of a merge. Update the existing PR +work product when the PR merges or closes instead of registering a duplicate. + +The same rule applies to external release approval gates: link the exact run, +create a human-only confirmation asking whether the user approved it in the +provider, and keep the agent assigned with `continuationPolicy: "wake_assignee"` +so the answer resumes verification. A handoff comment asking the user to comment +back or reassign the task is not a confirmation card. The card records the user's +answer; verify the provider's gate and publish result before continuing. diff --git a/ui/src/api/issues.ts b/ui/src/api/issues.ts index 129961dbb5b..4f1b6d907d5 100644 --- a/ui/src/api/issues.ts +++ b/ui/src/api/issues.ts @@ -36,7 +36,7 @@ import type { UpsertIssueWatchdog, UpsertIssueDocument, } from "@paperclipai/shared"; -import { api, ApiError, type RequestOptions } from "./client"; +import { api, ApiError, detachInflightGet, type RequestOptions } from "./client"; import { CommentSubmissionUnknownError } from "../lib/comment-submit-result"; function hasCommentReceipt(value: unknown): boolean { @@ -596,10 +596,12 @@ export const issuesApi = { api.post(`/issues/${id}/approvals`, { approvalId }), unlinkApproval: (id: string, approvalId: string) => api.delete<{ ok: true }>(`/issues/${id}/approvals/${approvalId}`), - listWorkProducts: (id: string, options?: { refreshPullRequests?: boolean }) => - api.get( - `/issues/${id}/work-products${options?.refreshPullRequests ? "?refreshPullRequests=true" : ""}`, - ), + listWorkProducts: (id: string, options?: { refreshPullRequests?: boolean; signal?: AbortSignal; fresh?: boolean }) => { + const path = `/issues/${id}/work-products${options?.refreshPullRequests ? "?refreshPullRequests=true" : ""}`; + // Query invalidations must not rejoin a request that read rows before the write. + if (options?.fresh) detachInflightGet(path); + return api.get(path, { signal: options?.signal }); + }, ensureWorkProductReviewDocument: (id: string, workProductId: string) => api.post( `/issues/${id}/work-products/${workProductId}/review-document`, diff --git a/ui/src/components/IssueMonitorBanner.test.tsx b/ui/src/components/IssueMonitorBanner.test.tsx index 100be024fd8..07598d50a2f 100644 --- a/ui/src/components/IssueMonitorBanner.test.tsx +++ b/ui/src/components/IssueMonitorBanner.test.tsx @@ -2,7 +2,7 @@ import { flushSync } from "react-dom"; import { createRoot } from "react-dom/client"; -import type { Issue } from "@paperclipai/shared"; +import type { Issue, IssueWorkProduct } from "@paperclipai/shared"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { @@ -145,6 +145,37 @@ describe("IssueMonitorBanner / IssueMonitorComposerStrip rendering", () => { } as unknown as Issue; } + it("shows the saved PR and a status check above the composer during a GitHub review wait", () => { + const issue = issueWithMonitor(new Date(NOW.getTime() + 6 * 60 * 60_000).toISOString()); + issue.executionState!.monitor!.serviceName = "github"; + const product = { + id: "pr-1", type: "pull_request", title: "Update runtime probe", url: null, + metadata: { url: "https://github.com/example/private-repo/pull/42" }, + status: "ready_for_review", reviewState: "needs_board_review", updatedAt: NOW, + } as unknown as IssueWorkProduct; + const onCheckNow = vi.fn(); + const root = createRoot(container); + const render = (products: IssueWorkProduct[], error?: string) => flushSync(() => root.render( + , + )); + render([product]); + expect(container.textContent).toContain("Pull request review requested"); + expect(container.textContent).toContain("Next check in 6h"); + expect(container.textContent).not.toContain("Resumes in"); + expect(container.querySelector("a")?.href).toBe("https://github.com/example/private-repo/pull/42"); + expect(container.querySelector("a")?.textContent).toBe("example/private-repo#42"); + const button = container.querySelector("button")!; + expect(button.textContent).toBe("Check status"); + flushSync(() => button.click()); + expect(onCheckNow).toHaveBeenCalledTimes(1); + render([product], "GitHub status check could not start"); + expect(container.querySelector('[role="alert"]')?.textContent).toContain("could not start"); + render([{ ...product, metadata: { ...product.metadata, state: "merged" } }]); + expect(container.textContent).not.toContain("review requested"); + expect(container.querySelector("a")).toBeNull(); + flushSync(() => root.unmount()); + }); + it("explains automatic workspace waiting without promising that a reply bypasses the lock", () => { const issue = { status: "todo", scheduledRetry: { status: "scheduled_retry", scheduledRetryReason: "workspace_busy", scheduledRetryAt: NOW.toISOString() }, diff --git a/ui/src/components/IssueMonitorBanner.tsx b/ui/src/components/IssueMonitorBanner.tsx index b46223ae87c..035eca29b84 100644 --- a/ui/src/components/IssueMonitorBanner.tsx +++ b/ui/src/components/IssueMonitorBanner.tsx @@ -1,9 +1,11 @@ import { useMemo } from "react"; import { Clock } from "lucide-react"; -import type { Issue } from "@paperclipai/shared"; +import type { Issue, IssueWorkProduct } from "@paperclipai/shared"; import { Button } from "@/components/ui/button"; import { InlineBanner } from "@/components/InlineBanner"; +import { IssuePullRequestLinks } from "@/components/IssuePullRequestLinks"; +import { getIssuePullRequests, pullRequestNeedsReview } from "@/lib/issue-pull-requests"; import { cn } from "@/lib/utils"; import { deriveMonitorState, @@ -50,6 +52,7 @@ export interface MonitorSurfaceCopy { /** `warning` (amber) once overdue, `info` (blue) while still on schedule. */ tone: "info" | "warning"; workspaceWait?: boolean; + pullRequestReview?: boolean; } function capitalize(value: string): string { @@ -65,6 +68,7 @@ export function buildMonitorSurfaceCopy( derived: DerivedMonitorState, now: MonitorDate, scheduledRetryReason?: string | null, + pendingPullRequestReviews = 0, ): MonitorSurfaceCopy | null { if (!isWaitingMonitorState(derived.state) || !derived.nextCheckAt) return null; @@ -81,6 +85,16 @@ export function buildMonitorSurfaceCopy( const eta = formatMonitorEta(derived.nextCheckAt, now); // "in 2h 12m" | "due now" | "overdue by 18m" const absolute = formatMonitorAbsolute(derived.nextCheckAt, {}, now); // local time, e.g. "Today, 4:08 PM" + if (derived.source === "monitor" && derived.serviceName?.toLowerCase() === "github" && pendingPullRequestReviews > 0) { + return { + bannerTitle: "Pull request review requested", + stripTitle: "Pull request review requested", + bannerMeta: [`Next check ${eta} · ${absolute} (your time)`], + stripMeta: [`Next check ${eta} · ${absolute}`], + tone: derived.state === "overdue" ? "warning" : "info", + pullRequestReview: true, + }; + } if (derived.source === "scheduled-retry" && scheduledRetryReason === "ai_connection_pool_wait") { return { bannerTitle: "Pool exhausted", @@ -134,21 +148,23 @@ export function buildMonitorSurfaceCopy( }; } -function useMonitorSurfaceCopy(issue: Issue): MonitorSurfaceCopy | null { +function useMonitorSurfaceCopy(issue: Issue, pendingPullRequestReviews: number): MonitorSurfaceCopy | null { // `nextCheckAt` is stable for a given issue; derive once to seed the ticking // countdown cadence, then re-derive against the live clock so the surfaces // roll scheduled → due → overdue on their own. const nextCheckAt = useMemo(() => deriveMonitorState(issue).nextCheckAt, [issue]); const now = useMonitorCountdown(nextCheckAt); - return useMemo(() => buildMonitorSurfaceCopy(deriveMonitorState(issue, now), now, issue.scheduledRetry?.scheduledRetryReason), [issue, now]); + return useMemo(() => buildMonitorSurfaceCopy(deriveMonitorState(issue, now), now, issue.scheduledRetry?.scheduledRetryReason, pendingPullRequestReviews), [issue, now, pendingPullRequestReviews]); } function CheckNowButton({ onCheckNow, checkingNow, + reviewRequested = false, }: { onCheckNow: () => void; checkingNow: boolean; + reviewRequested?: boolean; }) { return ( ); } @@ -168,6 +184,8 @@ export interface IssueMonitorSurfaceProps { issue: Issue; onCheckNow?: (() => void) | null; checkingNow?: boolean; + workProducts?: IssueWorkProduct[] | null; + checkError?: string | null; } /** @@ -179,8 +197,11 @@ export function IssueMonitorBanner({ issue, onCheckNow = null, checkingNow = false, + workProducts, + checkError, }: IssueMonitorSurfaceProps) { - const copy = useMonitorSurfaceCopy(issue); + const reviews = getIssuePullRequests(workProducts).filter(pullRequestNeedsReview); + const copy = useMonitorSurfaceCopy(issue, reviews.length); if (!copy) return null; return ( @@ -189,9 +210,13 @@ export function IssueMonitorBanner({ icon={Clock} title={copy.bannerTitle} className="my-3" - actions={onCheckNow && !copy.workspaceWait ? : null} + actions={onCheckNow && !copy.workspaceWait ? : null} > - {copy.bannerMeta.join(" · ")} +
+ {copy.pullRequestReview ? : null} + {copy.bannerMeta.join(" · ")} + {checkError ?

{checkError}

: null} +
); } @@ -205,9 +230,12 @@ export function IssueMonitorComposerStrip({ issue, onCheckNow = null, checkingNow = false, + workProducts, + checkError, className, }: IssueMonitorSurfaceProps & { className?: string }) { - const copy = useMonitorSurfaceCopy(issue); + const reviews = getIssuePullRequests(workProducts).filter(pullRequestNeedsReview); + const copy = useMonitorSurfaceCopy(issue, reviews.length); if (!copy) return null; return ( @@ -219,18 +247,22 @@ export function IssueMonitorComposerStrip({

- {copy.workspaceWait + {copy.pullRequestReview + ? "Review on GitHub, then check status to have the agent verify the result." + : copy.workspaceWait ? "You can keep sending instructions while the agent waits." : "Sending a reply wakes the agent now — before the scheduled check."}

+ {checkError ?

{checkError}

: null}
); } diff --git a/ui/src/components/IssueProperties.test.tsx b/ui/src/components/IssueProperties.test.tsx index 4a6b329a045..38a550a2217 100644 --- a/ui/src/components/IssueProperties.test.tsx +++ b/ui/src/components/IssueProperties.test.tsx @@ -465,6 +465,123 @@ function renderProperties(container: HTMLDivElement, props: ComponentProps { let container: HTMLDivElement; + it("surfaces saved PR review links even when external objects are unavailable", async () => { + mockIssuesApi.listWorkProducts.mockResolvedValue([{ + id: "pr-1", type: "pull_request", provider: "github", title: "Update runtime probe", + url: null, metadata: { url: "https://github.com/example/private-repo/pull/42" }, + status: "ready_for_review", reviewState: "needs_board_review", updatedAt: new Date(), + }]); + const root = renderProperties(container, { + issue: createIssue(), childIssues: [], onUpdate: vi.fn(), inline: true, + externalObjectsError: true, + }); + await waitForAssertion(() => { + const link = container.querySelector('a[href="https://github.com/example/private-repo/pull/42"]'); + expect(link?.textContent).toBe("example/private-repo#42"); + expect(container.textContent).toContain("Review requested"); + }); + act(() => root.unmount()); + }); + + it("refreshes PR state when opening Properties over an already loaded task", async () => { + const saved = { + id: "pr-1", type: "pull_request", provider: "github", title: "Update runtime probe", + url: "https://github.com/example/private-repo/pull/42", metadata: {}, + status: "ready_for_review", reviewState: "needs_board_review", updatedAt: new Date(), + }; + mockIssuesApi.listWorkProducts.mockImplementation(async (_id, options) => [ + options?.refreshPullRequests ? { ...saved, metadata: { state: "merged" } } : saved, + ]); + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + queryClient.setQueryData(queryKeys.issues.workProducts("issue-1"), [{ ...saved, metadata: { state: "merged" } }]); + const root = createRoot(container); + act(() => root.render( + + )); + await waitForAssertion(() => { + expect(mockIssuesApi.listWorkProducts).toHaveBeenCalledWith("issue-1", expect.objectContaining({ refreshPullRequests: true })); + expect(container.textContent).toContain("merged"); + expect(container.textContent).not.toContain("Review requested"); + }); + act(() => root.unmount()); + }); + + it("refreshes PRs on first panel open when the thread uses an identifier cache key", async () => { + const saved = { + id: "pr-1", type: "pull_request", title: "Update runtime probe", + url: "https://github.com/example/private-repo/pull/42", metadata: {}, + status: "ready_for_review", reviewState: "needs_board_review", updatedAt: new Date(), + }; + mockIssuesApi.listWorkProducts.mockImplementation(async (_id, options) => [ + options?.refreshPullRequests ? { ...saved, metadata: { state: "merged" } } : saved, + ]); + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + queryClient.setQueryData(queryKeys.issues.workProducts("PAP-1"), [{ ...saved, metadata: { state: "merged" } }]); + const root = createRoot(container); + act(() => root.render( + + )); + await waitForAssertion(() => { + expect(container.textContent).toContain("merged"); + expect(container.textContent).not.toContain("Review requested"); + }); + act(() => root.unmount()); + }); + + it("shows saved PRs while the first provider refresh is stalled", async () => { + const saved = { + id: "pr-1", type: "pull_request", title: "Update runtime probe", + url: "https://github.com/example/private-repo/pull/42", metadata: {}, + status: "ready_for_review", reviewState: "needs_board_review", updatedAt: new Date(), + }; + let finishRefresh!: (products: unknown[]) => void; + const refresh = new Promise((resolve) => { finishRefresh = resolve; }); + mockIssuesApi.listWorkProducts.mockImplementation(async (_id, options) => options?.refreshPullRequests ? refresh : [saved]); + const root = renderProperties(container, { issue: createIssue(), childIssues: [], onUpdate: vi.fn(), inline: true }); + await waitForAssertion(() => { + expect(mockIssuesApi.listWorkProducts).toHaveBeenCalledWith("issue-1", expect.objectContaining({ refreshPullRequests: true })); + expect(container.querySelector(`a[href="${saved.url}"]`)).not.toBeNull(); + expect(container.textContent).toContain("Review requested"); + }); + finishRefresh([{ ...saved, metadata: { state: "merged" } }]); + await waitForAssertion(() => { + expect(container.textContent).toContain("merged"); + expect(container.textContent).not.toContain("Review requested"); + }); + act(() => root.unmount()); + }); + + it.each([ + { statusLabel: "Open", statusCategory: "open", liveness: "stale", statusIconKey: "git-pull-request", review: true }, + { statusLabel: "Merged", statusCategory: "succeeded", liveness: "fresh", statusIconKey: "git-merge", review: false }, + { statusLabel: "Not found", statusCategory: "archived", liveness: "stale", statusIconKey: null, review: true }, + ] as const)("combines a saved PR with its $statusLabel provider status and freshness", async (provider) => { + const canonical = "https://github.com/example/private-repo/pull/42"; + const savedUrl = `${canonical}?diff=split#discussion_r123`; + mockIssuesApi.listWorkProducts.mockResolvedValue([{ + id: "pr-1", type: "pull_request", title: "Update runtime probe", url: savedUrl, + metadata: {}, status: "ready_for_review", reviewState: "needs_board_review", updatedAt: new Date(), + }]); + const root = renderProperties(container, { + issue: createIssue(), childIssues: [], onUpdate: vi.fn(), inline: true, + externalObjects: [{ + pill: { providerKey: "github", objectType: "pull_request", url: canonical, ...provider }, + mentionCount: 1, sourceLabels: ["Comment"], + group: { object: null, mentions: [], mentionCount: 1, sourceLabels: ["Comment"] }, + }], + }); + await waitForAssertion(() => { + const links = container.querySelectorAll('a[href*="/pull/42"]'); + expect(links).toHaveLength(1); + expect(links[0].getAttribute("href")).toBe(savedUrl); + const row = links[0].closest("li")!; + expect(row.textContent).toContain(provider.statusLabel); + expect(row.querySelector("[data-external-liveness]")?.getAttribute("data-external-liveness")).toBe(provider.liveness); + expect(row.textContent?.includes("Review requested")).toBe(provider.review); + }); + act(() => root.unmount()); + }); + beforeEach(() => { mockSidebarState.isMobile = false; container = document.createElement("div"); diff --git a/ui/src/components/IssuePullRequestLinks.tsx b/ui/src/components/IssuePullRequestLinks.tsx new file mode 100644 index 00000000000..99cdccf9a19 --- /dev/null +++ b/ui/src/components/IssuePullRequestLinks.tsx @@ -0,0 +1,51 @@ +import type { IssueWorkProduct } from "@paperclipai/shared"; +import { Badge } from "@/components/ui/badge"; +import { pullRequestHref, pullRequestIdentity, pullRequestLabel, pullRequestNeedsReview, pullRequestState } from "@/lib/issue-pull-requests"; +import type { ExternalObjectPillData } from "./ExternalObjectPill"; +import { ExternalObjectStatusIcon } from "./ExternalObjectStatusIcon"; +import { externalObjectDisplayStatusLabel, externalObjectLivenessLabel } from "@/lib/external-objects"; + +/** Saved PRs remain inspectable even when the provider lookup is unavailable. */ +export function IssuePullRequestLinks({ products, externalObjects = [] }: { + products: IssueWorkProduct[]; + externalObjects?: ExternalObjectPillData[]; +}) { + return ( +
    + {products.map((product) => { + const href = pullRequestHref(product); + const label = pullRequestLabel(product); + const state = pullRequestState(product); + const identity = pullRequestIdentity(href); + const provider = identity ? externalObjects.find((entry) => pullRequestIdentity(entry.url) === identity) : undefined; + const providerStatus = provider ? externalObjectDisplayStatusLabel(provider) : null; + const providerTerminal = provider?.liveness === "fresh" + && (provider.statusIconKey === "git-merge" || ["merged", "closed"].includes(providerStatus?.toLowerCase() ?? "")); + return ( +
  • + {href ? ( + + {label} + + ) : {label} · No PR link provided} + {provider ? ( + + + {providerStatus}{!["fresh", "unknown"].includes(provider.liveness) ? ` (${externalObjectLivenessLabel(provider.liveness)})` : ""} + + ) : null} + {pullRequestNeedsReview(product) && !providerTerminal ? ( + Review requested + ) : ["merged", "closed", "archived", "draft", "changes_requested"].includes(state) + && providerStatus?.toLowerCase() !== state ? ( + {provider ? "Saved: " : ""}{state.replaceAll("_", " ")} + ) : null} +
    • + ); + })} +
+ ); +} diff --git a/ui/src/components/issue-properties/IssueProperties.tsx b/ui/src/components/issue-properties/IssueProperties.tsx index 640ae943bbc..8e2f6763199 100644 --- a/ui/src/components/issue-properties/IssueProperties.tsx +++ b/ui/src/components/issue-properties/IssueProperties.tsx @@ -1,3 +1,6 @@ +import { IssuePullRequestLinks } from "../IssuePullRequestLinks"; +import { useIssueWorkProducts } from "../../hooks/useIssueWorkProducts"; +import { getIssuePullRequests, pullRequestHref, pullRequestIdentity } from "../../lib/issue-pull-requests"; import { useWorkspaceIsolationControls } from "@/hooks/useWorkspaceIsolationControls"; import { AgentIdentity } from "@/components/AgentIdentity"; import { AgentAvatar } from "@/components/AgentAvatar"; @@ -295,11 +298,12 @@ export function IssueProperties({ queryFn: () => issuesApi.listAttachments(issue.id), enabled: taskChatShellEnabled, }); - const { data: paneTabWorkProducts } = useQuery({ - queryKey: queryKeys.issues.workProducts(issue.id), - queryFn: () => issuesApi.listWorkProducts(issue.id), - enabled: taskChatShellEnabled, - }); + const { data: paneTabWorkProducts, isError: workProductsError, refetch: refetchWorkProducts } = useIssueWorkProducts(issue.id); + const pullRequests = useMemo(() => getIssuePullRequests(paneTabWorkProducts), [paneTabWorkProducts]); + const remainingExternalObjects = useMemo(() => { + const identities = new Set(pullRequests.map((product) => pullRequestIdentity(pullRequestHref(product))).filter(Boolean)); + return externalObjects?.filter((entry) => !identities.has(pullRequestIdentity(entry.pill.url))); + }, [externalObjects, pullRequests]); const { data: paneTabDocuments } = useIssueDocuments(taskChatShellEnabled ? issue.id : null); // Proxy `artifact-review-*` documents surface only through their Work // product row, so they must not summon the Plan or Documents surfaces. @@ -2582,8 +2586,20 @@ export function IssueProperties({ ) : null} + {pullRequests.length > 0 || workProductsError ? ( + +
+ entry.pill)} /> + {workProductsError ? ( + + Couldn’t load pull requests. + + ) : null} +
+ + ) : null} issuesApi.listWorkProducts(issue.id), - }); + } = useIssueWorkProducts(issue.id); const { data: documents, isPending: documentsPending, diff --git a/ui/src/components/issue-properties/IssuePropertiesDocumentAnnotations.test.tsx b/ui/src/components/issue-properties/IssuePropertiesDocumentAnnotations.test.tsx index ea3b53e283c..77a38ca2293 100644 --- a/ui/src/components/issue-properties/IssuePropertiesDocumentAnnotations.test.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesDocumentAnnotations.test.tsx @@ -35,6 +35,7 @@ vi.mock("@/hooks/useIssuePlanDocument", () => ({ useIssuePlanDocument: () => ({ data: { ...issueDocument, key: "plan" }, isLoading: false }), })); vi.mock("@/hooks/useIssueDocuments", () => ({ useIssueDocuments: () => ({ data: [issueDocument] }) })); +vi.mock("@/hooks/useIssueWorkProducts", () => ({ useIssueWorkProducts: () => ({ data: [] }) })); vi.mock("@/lib/router", () => ({ Link: ({ children, to }: { children: React.ReactNode; to: string }) => {children}, useLocation: () => ({ hash: "" }), diff --git a/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx index a633cc80de9..0b52378d5e4 100644 --- a/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx @@ -13,7 +13,7 @@ import { RichWorkProductCard } from "@/components/task-chat/RichWorkProductCard" const mockIssuesApi = vi.hoisted(() => ({ listAttachments: vi.fn(async (): Promise => []), - listWorkProducts: vi.fn(async (): Promise => []), + listWorkProducts: vi.fn(async (_issueId: string, _options?: { refreshPullRequests?: boolean }): Promise => []), ensureWorkProductReviewDocument: vi.fn(async (): Promise => ({})), })); vi.mock("@/api/issues", () => ({ issuesApi: mockIssuesApi })); @@ -223,6 +223,23 @@ describe("markdown work product review row", () => { expect(download?.getAttribute("href")).toBe(`/api/attachments/${ATTACHMENT_ID}/content?download=1`); }); + it("shows saved artifacts while the first PR refresh is stalled", async () => { + const saved = makeMarkdownWorkProduct({ + type: "pull_request", provider: "github", title: "Saved PR during refresh", + url: "https://github.com/example/repo/pull/42", metadata: {}, + }); + let finishRefresh!: (products: unknown[]) => void; + const refresh = new Promise((resolve) => { finishRefresh = resolve; }); + mockIssuesApi.listWorkProducts.mockImplementation(async (_id, options) => options?.refreshPullRequests ? refresh : [saved]); + await renderTab({}, saved.title); + await waitForAssertion(() => { + expect(mockIssuesApi.listWorkProducts).toHaveBeenCalledWith(issue.id, expect.objectContaining({ refreshPullRequests: true })); + expect(container.querySelector(`a[href="${saved.url}"]`)).not.toBeNull(); + }); + finishRefresh([{ ...saved, metadata: { state: "merged" } }]); + await waitForAssertion(() => expect(container.textContent?.toLowerCase()).toContain("merged")); + }); + it("expands into the existing document surface without a server call when the document exists", async () => { mockUseIssueDocuments.mockReturnValue({ data: [makeReviewDocument()] }); await renderTab(); diff --git a/ui/src/context/LiveUpdatesProvider.test.ts b/ui/src/context/LiveUpdatesProvider.test.ts index fde36aa5d4e..a51d76423cf 100644 --- a/ui/src/context/LiveUpdatesProvider.test.ts +++ b/ui/src/context/LiveUpdatesProvider.test.ts @@ -937,6 +937,9 @@ describe("LiveUpdatesProvider issue invalidation", () => { expect(invalidations).toContainEqual({ queryKey: queryKeys.issues.comments("issue-1"), }); + for (const ref of ["PAP-759", "issue-1"]) { + expect(invalidations).toContainEqual({ queryKey: queryKeys.issues.workProductPullRequestRefresh(ref) }); + } expect(cache.get(JSON.stringify(queryKeys.issues.activeRun("PAP-759")))).toBeNull(); expect(cache.get(JSON.stringify(queryKeys.issues.liveRuns("PAP-759")))).toEqual([]); expect(cache.get(JSON.stringify(queryKeys.issues.detail("PAP-759")))).toMatchObject({ @@ -977,6 +980,7 @@ describe("LiveUpdatesProvider issue invalidation", () => { )).toBe(true); for (const ref of ["PAP-759", "issue-1"]) { expect(invalidations).not.toContainEqual({ queryKey: queryKeys.issues.comments(ref) }); + expect(invalidations).not.toContainEqual({ queryKey: queryKeys.issues.workProductPullRequestRefresh(ref) }); } }, ); diff --git a/ui/src/context/LiveUpdatesProvider.tsx b/ui/src/context/LiveUpdatesProvider.tsx index 3a38ad3b479..6727ae13837 100644 --- a/ui/src/context/LiveUpdatesProvider.tsx +++ b/ui/src/context/LiveUpdatesProvider.tsx @@ -533,6 +533,7 @@ function invalidateVisibleIssueRunQueries( queryClient.invalidateQueries({ queryKey: queryKeys.issues.comments(issueRef) }); queryClient.invalidateQueries({ queryKey: queryKeys.issues.attachments(issueRef) }); queryClient.invalidateQueries({ queryKey: queryKeys.issues.workProducts(issueRef) }); + queryClient.invalidateQueries({ queryKey: queryKeys.issues.workProductPullRequestRefresh(issueRef) }); queryClient.invalidateQueries({ queryKey: ["issues", "tree-control-state", issueRef] }); } } diff --git a/ui/src/hooks/useIssueWorkProducts.test.tsx b/ui/src/hooks/useIssueWorkProducts.test.tsx new file mode 100644 index 00000000000..b08476b9e5e --- /dev/null +++ b/ui/src/hooks/useIssueWorkProducts.test.tsx @@ -0,0 +1,150 @@ +// @vitest-environment jsdom +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { IssueWorkProduct } from "@paperclipai/shared"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { queryKeys } from "@/lib/queryKeys"; +import { pullRequestNeedsReview } from "@/lib/issue-pull-requests"; +import { __liveUpdatesTestUtils } from "@/context/LiveUpdatesProvider"; +import { useIssueWorkProducts } from "./useIssueWorkProducts"; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +function response(products: IssueWorkProduct[]) { + return { ok: true, status: 200, json: async () => products } as Response; +} + +function product(id: string, overrides: Partial = {}): IssueWorkProduct { + return { + id, type: "pull_request", provider: "github", title: id, + url: "https://github.com/example/repo/pull/42", metadata: {}, + status: "ready_for_review", reviewState: "needs_board_review", + updatedAt: new Date("2026-10-06T12:00:00Z"), ...overrides, + } as IssueWorkProduct; +} + +let client: QueryClient; +let root: Root; +let host: HTMLDivElement; +let issueId: string; +let observed: ReturnType; +let saved: IssueWorkProduct[]; +let requests: Array<{ path: string; signal: AbortSignal; resolve: (products: IssueWorkProduct[]) => void }>; +let stallSaved: boolean; + +function Harness() { observed = useIssueWorkProducts(issueId); return null; } +async function render() { + await act(async () => root.render()); +} +async function waitFor(assertion: () => void) { + await vi.waitFor(async () => { + await act(async () => { await new Promise((resolve) => setTimeout(resolve, 0)); }); + assertion(); + }); +} +async function liveUpdate() { + await act(async () => { void client.invalidateQueries({ queryKey: queryKeys.issues.workProducts(issueId) }); }); +} + +beforeEach(() => { + client = new QueryClient({ defaultOptions: { queries: { retry: false, gcTime: Infinity } } }); + host = document.createElement("div"); + root = createRoot(host); + issueId = "work-product-race"; + saved = [product("pr")]; + requests = []; + stallSaved = false; + // Keep the real API client, including GET coalescing and abort handling. + vi.stubGlobal("fetch", vi.fn((path: string, init: RequestInit) => { + if (!stallSaved && !path.includes("refreshPullRequests=true")) return Promise.resolve(response(saved)); + return new Promise((resolve) => { + requests.push({ path, signal: init.signal as AbortSignal, resolve: (products) => resolve(response(products)) }); + }); + })); +}); +afterEach(async () => { + await act(async () => { root.unmount(); client.clear(); requests.forEach((request) => request.resolve([])); }); + host.remove(); + vi.unstubAllGlobals(); +}); + +it("shows a newly saved artifact during GitHub refresh and keeps it after the old response", async () => { + const pr = saved[0]; + await render(); + await waitFor(() => expect(requests).toHaveLength(1)); + saved = [pr, product("new-artifact", { type: "artifact" })]; + await liveUpdate(); + await waitFor(() => expect(observed.data?.map((row) => row.id)).toEqual(["pr", "new-artifact"])); + // An artifact arrival does not cancel or restart the unrelated provider request. + expect(requests).toHaveLength(1); + await act(async () => requests[0].resolve([{ ...pr, metadata: { state: "merged" } }])); + await waitFor(() => expect(observed.data?.[0].metadata?.state).toBe("merged")); + expect(observed.data?.map((row) => row.id)).toEqual(["pr", "new-artifact"]); +}); + +it("keeps a changed review request when an older provider response arrives last", async () => { + const original = saved[0]; + await render(); + await waitFor(() => expect(requests).toHaveLength(1)); + saved = [{ ...original, status: "changes_requested", reviewState: "changes_requested", updatedAt: new Date("2026-10-06T12:01:00Z") }]; + await liveUpdate(); + await waitFor(() => { + expect(observed.data?.[0].reviewState).toBe("changes_requested"); + expect(requests).toHaveLength(2); + }); + await act(async () => requests[1].resolve([{ ...saved[0], metadata: { state: "open" } }])); + await waitFor(() => expect(observed.data?.[0].metadata?.state).toBe("open")); + await act(async () => requests[0].resolve([{ ...original, metadata: { state: "merged" } }])); + expect(observed.data?.[0].reviewState).toBe("changes_requested"); + expect(observed.data?.[0].metadata?.state).toBe("open"); +}); + +it("starts a fresh stored read when a live update invalidates an in-flight read", async () => { + const old = product("old", { type: "artifact" }); + client.setQueryData(queryKeys.issues.workProducts(issueId), [old]); + stallSaved = true; + await render(); + await waitFor(() => expect(requests).toHaveLength(1)); + await liveUpdate(); + await waitFor(() => expect(requests).toHaveLength(2)); + expect(requests[0].signal.aborted).toBe(true); + const added = product("new", { type: "artifact" }); + await act(async () => requests[1].resolve([old, added])); + await waitFor(() => expect(observed.data?.map((row) => row.id)).toEqual(["old", "new"])); + await act(async () => requests[0].resolve([old])); + expect(observed.data?.map((row) => row.id)).toEqual(["old", "new"]); +}); + +it("does not show an old task's provider response after navigating", async () => { + const original = saved[0]; + await render(); + await waitFor(() => expect(requests).toHaveLength(1)); + issueId = "another-task"; + saved = [product("another-artifact", { type: "artifact" })]; + await render(); + await waitFor(() => expect(observed.data?.[0].id).toBe("another-artifact")); + await act(async () => requests[0].resolve([{ ...original, metadata: { state: "merged" } }])); + expect(observed.data?.map((row) => row.id)).toEqual(["another-artifact"]); +}); + +it.each(["merged", "closed"])("refreshes GitHub after a run finishes with unchanged saved rows (%s)", async (state) => { + client.setQueryData(queryKeys.issues.detail("PAP-42"), { + id: issueId, identifier: "PAP-42", assigneeAgentId: "agent", executionRunId: "run", + }); + await render(); + await waitFor(() => expect(requests).toHaveLength(1)); + await act(async () => requests[0].resolve([{ ...saved[0], metadata: { state: "open" } }])); + await waitFor(() => expect(observed.data?.[0].metadata?.state).toBe("open")); + expect(pullRequestNeedsReview(observed.data![0])).toBe(true); + + await act(async () => { + __liveUpdatesTestUtils.invalidateVisibleIssueRunQueries(client, "/PAP/issues/PAP-42", { + runId: "run", agentId: "agent", status: "succeeded", + }, { isForegrounded: true }); + }); + await waitFor(() => expect(requests).toHaveLength(2)); + await act(async () => requests[1].resolve([{ ...saved[0], metadata: { state } }])); + await waitFor(() => expect(pullRequestNeedsReview(observed.data![0])).toBe(false)); + expect(observed.data?.[0].metadata?.state).toBe(state); +}); diff --git a/ui/src/hooks/useIssueWorkProducts.ts b/ui/src/hooks/useIssueWorkProducts.ts new file mode 100644 index 00000000000..329bb78a1dc --- /dev/null +++ b/ui/src/hooks/useIssueWorkProducts.ts @@ -0,0 +1,46 @@ +import { useMemo } from "react"; +import { useQuery } from "@tanstack/react-query"; +import type { IssueWorkProduct } from "@paperclipai/shared"; +import { issuesApi } from "@/api/issues"; +import { queryKeys } from "@/lib/queryKeys"; +import { keepPreviousDataForSameQueryTail } from "@/lib/query-placeholder-data"; + +/** Saved rows stay authoritative while GitHub enriches matching PR versions. */ +export function useIssueWorkProducts(issueId: string | null | undefined) { + const stored = useQuery({ + queryKey: queryKeys.issues.workProducts(issueId ?? "__none__"), + queryFn: ({ signal }) => issuesApi.listWorkProducts(issueId!, { signal, fresh: true }), + enabled: Boolean(issueId), + refetchOnMount: "always", + placeholderData: keepPreviousDataForSameQueryTail(issueId ?? "__none__"), + }); + const pullRequests = stored.data?.filter((product) => product.type === "pull_request") ?? []; + const hasPullRequests = Boolean(issueId) && pullRequests.length > 0; + const provider = useQuery({ + // A saved PR change gets its own refresh; adding an artifact does not restart GitHub. + queryKey: [...queryKeys.issues.workProductPullRequestRefresh(issueId ?? "__none__"), pullRequests], + queryFn: ({ signal }) => issuesApi.listWorkProducts(issueId!, { refreshPullRequests: true, signal, fresh: true }), + enabled: hasPullRequests, + refetchOnMount: "always", + }); + const data = useMemo(() => { + const refreshed = new Map(provider.data?.map((product) => [product.id, product])); + return stored.data?.map((product) => { + const enriched = refreshed.get(product.id); + if (product.type !== "pull_request" || !enriched + || new Date(enriched.updatedAt).getTime() !== new Date(product.updatedAt).getTime()) return product; + // Never replace the saved list, its review flags, or its links with a provider snapshot. + return { ...product, metadata: enriched.metadata }; + }); + }, [stored.data, provider.data]); + return { + ...stored, + data, + isFetching: stored.isFetching || provider.isFetching, + isError: stored.isError || provider.isError, + error: stored.error ?? provider.error, + refetch: async () => { + await Promise.all([stored.refetch(), ...(hasPullRequests ? [provider.refetch()] : [])]); + }, + }; +} diff --git a/ui/src/lib/issue-pull-requests.test.ts b/ui/src/lib/issue-pull-requests.test.ts new file mode 100644 index 00000000000..55784aaf1f3 --- /dev/null +++ b/ui/src/lib/issue-pull-requests.test.ts @@ -0,0 +1,51 @@ +import type { IssueWorkProduct } from "@paperclipai/shared"; +import { describe, expect, it } from "vitest"; +import { getIssuePullRequests, pullRequestHref, pullRequestLabel, pullRequestNeedsReview } from "./issue-pull-requests"; + +function product(overrides: Partial = {}): IssueWorkProduct { + return { + id: "pr-1", type: "pull_request", title: "Update runtime probe", url: null, + status: "ready_for_review", reviewState: "needs_board_review", isPrimary: true, + metadata: { url: "https://github.com/example/private-repo/pull/42" }, + updatedAt: new Date("2026-10-06T12:00:00Z"), ...overrides, + } as IssueWorkProduct; +} + +describe("saved issue pull requests", () => { + it("keeps a private PR inspectable from its saved metadata without a provider lookup", () => { + const saved = product(); + expect(pullRequestHref(saved)).toBe("https://github.com/example/private-repo/pull/42"); + expect(pullRequestLabel(saved)).toBe("example/private-repo#42"); + expect(pullRequestNeedsReview(saved)).toBe(true); + }); + + it.each(["merged", "closed", "archived"])("does not ask for review of %s work even with a stale review flag", (state) => { + expect(pullRequestNeedsReview(product({ status: state }))).toBe(false); + expect(pullRequestNeedsReview(product({ metadata: { state } }))).toBe(false); + }); + + it("keeps the newer merged record instead of an old duplicate review request", () => { + const old = product({ id: "old", updatedAt: new Date("2026-10-05T12:00:00Z") }); + const merged = product({ status: "merged" }); + expect(getIssuePullRequests([old, merged])).toEqual([merged]); + }); + + it.each(["?diff=split", "#discussion_r123", "/files", "/"])("deduplicates GitHub navigation variants: %s", (suffix) => { + const old = product({ id: "old", updatedAt: new Date("2026-10-05T12:00:00Z") }); + const merged = product({ status: "merged", url: `https://github.com/Example/Private-Repo/pull/42${suffix}` }); + expect(getIssuePullRequests([old, merged])).toEqual([merged]); + expect(pullRequestHref(merged)).toBe(merged.url); + }); + + it("puts actionable reviews before historical PRs and ignores other work products", () => { + const review = product(); + const merged = product({ id: "merged", url: "https://github.com/example/repo/pull/12", status: "merged" }); + expect(getIssuePullRequests([merged, product({ type: "artifact" }), review])).toEqual([review, merged]); + expect(pullRequestNeedsReview(product({ reviewState: "none" }))).toBe(false); + }); + + it.each(["javascript:alert(1)", "https://token@github.com/example/repo/pull/42", "not a URL"])("does not turn an unsafe saved URL into a link: %s", (url) => { + expect(pullRequestHref(product({ metadata: { url } }))).toBeNull(); + expect(getIssuePullRequests([product({ metadata: { url } })])).toHaveLength(1); + }); +}); diff --git a/ui/src/lib/issue-pull-requests.ts b/ui/src/lib/issue-pull-requests.ts new file mode 100644 index 00000000000..c4becf7b667 --- /dev/null +++ b/ui/src/lib/issue-pull-requests.ts @@ -0,0 +1,69 @@ +import type { IssueWorkProduct } from "@paperclipai/shared"; +import { workProductHref } from "./issue-artifacts"; + +const TERMINAL_STATES = new Set(["merged", "closed", "archived"]); + +export function pullRequestState(product: IssueWorkProduct): string { + if (TERMINAL_STATES.has(product.status)) return product.status; + return typeof product.metadata?.state === "string" ? product.metadata.state : product.status; +} + +export function pullRequestNeedsReview(product: IssueWorkProduct): boolean { + const state = pullRequestState(product); + return !TERMINAL_STATES.has(state) + && state !== "changes_requested" + && product.reviewState === "needs_board_review"; +} + +export function pullRequestHref(product: IssueWorkProduct): string | null { + const href = workProductHref(product); + if (!href) return null; + try { + const url = new URL(href); + if (!["http:", "https:"].includes(url.protocol) || url.username || url.password) return null; + return url.href; + } catch { + return null; + } +} + +export function pullRequestLabel(product: IssueWorkProduct): string { + const href = pullRequestHref(product); + if (href) { + const url = new URL(href); + const match = url.pathname.match(/^\/([^/]+)\/([^/]+)\/pull\/(\d+)\/?$/); + if (url.hostname === "github.com" && match) return `${match[1]}/${match[2]}#${match[3]}`; + } + return product.title; +} + +/** GitHub navigation options and comment anchors do not identify a different PR. */ +export function pullRequestIdentity(href: string | null | undefined): string | null { + if (!href) return null; + try { + const url = new URL(href); + if (!["http:", "https:"].includes(url.protocol) || url.username || url.password) return null; + const match = url.pathname.match(/^\/([^/]+)\/([^/]+)\/pull\/(\d+)(?:\/|$)/); + if (url.hostname === "github.com" && !url.port && match) { + return `github.com/${match[1].toLowerCase()}/${match[2].toLowerCase()}/pull/${Number(match[3])}`; + } + return url.href; + } catch { + return null; + } +} + +/** Prefer the newest saved record when an agent registered the same PR twice. */ +export function getIssuePullRequests(products: IssueWorkProduct[] | null | undefined): IssueWorkProduct[] { + const sorted = (products ?? []).filter((product) => product.type === "pull_request") + .sort((a, b) => new Date(b.updatedAt).getTime() - new Date(a.updatedAt).getTime() + || Number(TERMINAL_STATES.has(pullRequestState(b))) - Number(TERMINAL_STATES.has(pullRequestState(a)))); + const seen = new Set(); + return sorted.filter((product) => { + const key = pullRequestIdentity(pullRequestHref(product)) ?? product.id; + if (seen.has(key)) return false; + seen.add(key); + return true; + }).sort((a, b) => Number(pullRequestNeedsReview(b)) - Number(pullRequestNeedsReview(a)) + || Number(b.isPrimary) - Number(a.isPrimary)); +} diff --git a/ui/src/lib/queryKeys.ts b/ui/src/lib/queryKeys.ts index 8e0c8ff2479..27a5a65a438 100644 --- a/ui/src/lib/queryKeys.ts +++ b/ui/src/lib/queryKeys.ts @@ -397,6 +397,8 @@ export const queryKeys = { ["issues", "runner-goal", issueId, agentId ?? "__effective__"] as const, workProducts: (issueId: string) => ["issues", "work-products", issueId] as const, + workProductPullRequestRefresh: (issueId: string) => + ["issues", "work-product-pr-refresh", issueId] as const, fileResources: ( issueId: string, options: { diff --git a/ui/src/pages/IssueDetail.test.tsx b/ui/src/pages/IssueDetail.test.tsx index 1055e4be7c0..af4a0d6cde1 100644 --- a/ui/src/pages/IssueDetail.test.tsx +++ b/ui/src/pages/IssueDetail.test.tsx @@ -54,6 +54,7 @@ const mockIssuesApi = vi.hoisted(() => ({ listComments: vi.fn(), listAttachments: vi.fn(), listWorkProducts: vi.fn(), + checkMonitorNow: vi.fn(), listFeedbackVotes: vi.fn(), listInteractions: vi.fn(), getQueuedComments: vi.fn(), @@ -1330,6 +1331,7 @@ describe("IssueDetail", () => { mockIssuesApi.listComments.mockResolvedValue([]); mockIssuesApi.listAttachments.mockResolvedValue([]); mockIssuesApi.listWorkProducts.mockResolvedValue([]); + mockIssuesApi.checkMonitorNow.mockReset(); mockIssuesApi.listFeedbackVotes.mockResolvedValue([]); mockIssuesApi.listInteractions.mockResolvedValue([]); mockIssuesApi.getQueuedComments.mockResolvedValue( @@ -1420,6 +1422,37 @@ describe("IssueDetail", () => { mockRouteParams.companyPrefix = "PAP"; }); + it.each([false, true])("keeps monitor errors on the checked task (late response: %s)", async (lateResponse) => { + const monitor = { monitor: { status: "scheduled", nextCheckAt: new Date(Date.now() + 60_000).toISOString(), attemptCount: 1, serviceName: "github" } } as Issue["executionState"]; + mockIssuesApi.get.mockResolvedValue(createIssue({ status: "in_progress", executionState: monitor })); + const check = createDeferred(); + mockIssuesApi.checkMonitorNow.mockReturnValue(check.promise); + await act(async () => root.render()); + let button: HTMLButtonElement | undefined; + await waitForAssertion(() => { + button = Array.from(container.querySelectorAll("button")).find((entry) => entry.textContent === "Check now"); + expect(button).toBeTruthy(); + }); + await act(async () => button!.click()); + if (!lateResponse) { + check.reject(new Error("First task monitor failed")); + await waitForAssertion(() => expect(container.textContent).toContain("First task monitor failed")); + } + const second = createIssue({ id: "issue-2", identifier: "PAP-2", title: "Second monitored task", status: "in_progress", executionState: monitor }); + mockIssuesApi.get.mockResolvedValue(second); + queryClient.setQueryData(queryKeys.issues.detail("PAP-2"), second); + mockRouteParams.issueId = "PAP-2"; + mockLocation.pathname = "/issues/PAP-2"; + await act(async () => root.render()); + if (lateResponse) check.reject(new Error("First task monitor failed")); + await flushReact(); + await waitForAssertion(() => { + expect(container.textContent).toContain("Second monitored task"); + expect(container.textContent).not.toContain("First task monitor failed"); + expect(Array.from(container.querySelectorAll("button")).find((entry) => entry.textContent === "Check now")?.disabled).toBe(false); + }); + }); + afterEach(async () => { await act(async () => { root.unmount(); diff --git a/ui/src/pages/IssueDetail.tsx b/ui/src/pages/IssueDetail.tsx index 07b3c07216e..c740003a6b3 100644 --- a/ui/src/pages/IssueDetail.tsx +++ b/ui/src/pages/IssueDetail.tsx @@ -94,6 +94,7 @@ import { } from "../lib/issue-timeline-events"; import { queryKeys } from "../lib/queryKeys"; import { keepPreviousDataForSameQueryTail } from "../lib/query-placeholder-data"; +import { useIssueWorkProducts } from "../hooks/useIssueWorkProducts"; import { mergePendingIssueQueuedComments, normalizeIssueQueuedCommentQueue, @@ -3276,34 +3277,7 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS isLoading: workProductsLoading, isError: workProductsError, refetch: refetchWorkProducts, - } = useQuery({ - queryKey: queryKeys.issues.workProducts(issueId!), - queryFn: () => - issuesApi.listWorkProducts(issueId!, { - // Initial geometry needs stored artifacts, not a network round-trip to - // GitHub. Enrich PR status after the stored list has painted. - refreshPullRequests: - queryClient.getQueryData(queryKeys.issues.workProducts(issueId!)) !== - undefined, - }), - enabled: !!issueId, - refetchOnMount: "always", - placeholderData: keepPreviousDataForSameQueryTail( - issueId ?? "pending", - ), - }); - - const enrichedWorkProductsIssue = useRef(null); - useEffect(() => { - if ( - !issueId || - enrichedWorkProductsIssue.current === issueId || - !workProducts?.some((product) => product.type === "pull_request") - ) - return; - enrichedWorkProductsIssue.current = issueId; - void refetchWorkProducts(); - }, [issueId, workProducts, refetchWorkProducts]); + } = useIssueWorkProducts(issueId); const { data: liveRunCount = 0 } = useQuery( { @@ -4554,6 +4528,7 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS ); const checkIssueMonitorNow = useMutation({ + mutationKey: ["check-issue-monitor-now", issueId], mutationFn: () => issuesApi.checkMonitorNow(issueId!), onSuccess: () => { invalidateIssueDetail(); @@ -7461,6 +7436,8 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS checkIssueMonitorNow.mutate()} checkingNow={checkIssueMonitorNow.isPending} /> @@ -8014,6 +7991,8 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS hasVisibleMonitorSurface(issue) ? ( checkIssueMonitorNow.mutate()} checkingNow={checkIssueMonitorNow.isPending} /> diff --git a/ui/storybook/stories/task-artifact-gallery.stories.tsx b/ui/storybook/stories/task-artifact-gallery.stories.tsx index 6f44a2dd3e6..79a021486e2 100644 --- a/ui/storybook/stories/task-artifact-gallery.stories.tsx +++ b/ui/storybook/stories/task-artifact-gallery.stories.tsx @@ -45,8 +45,11 @@ const images = [manilaImage, nightImage].map((src, index) => product(index ? "Ni })); type Scenario = "videos" | "mixed" | "rows" | "fallback" | "empty"; +function workProductsForScenario(scenario: Scenario = "videos") { + return scenario === "empty" ? [] : scenario === "rows" ? [...videos.slice(0, 1), ...images.slice(0, 1), ...files] : scenario === "mixed" ? [...videos.slice(0, 2), ...images, ...files] : scenario === "fallback" ? [product("Video preview unavailable", 30, { metadata: { contentType: "video/mp4", contentPath: "data:video/mp4;base64,AA==", originalFilename: "unavailable.mp4" } }), product("Image preview unavailable", 31, { metadata: { contentType: "image/png", contentPath: "data:image/png;base64,AA==" } })] : videos; +} function GalleryStory({ scenario = "videos", width = 480 }: { scenario?: Scenario; width?: number }) { - const workProducts = scenario === "empty" ? [] : scenario === "rows" ? [...videos.slice(0, 1), ...images.slice(0, 1), ...files] : scenario === "mixed" ? [...videos.slice(0, 2), ...images, ...files] : scenario === "fallback" ? [product("Video preview unavailable", 30, { metadata: { contentType: "video/mp4", contentPath: "data:video/mp4;base64,AA==", originalFilename: "unavailable.mp4" } }), product("Image preview unavailable", 31, { metadata: { contentType: "image/png", contentPath: "data:image/png;base64,AA==" } })] : videos; + const workProducts = workProductsForScenario(scenario); const attachments = scenario === "mixed" ? [{ id: "loose-image", companyId: issue.companyId, issueId: issue.id, createdByAgentId: storybookAgents[0].id, contentPath: nightImage, contentType: "image/png", originalFilename: "Unpromoted agent attachment.png", objectKey: "cover.png", byteSize: 42000, createdAt: date } as IssueAttachment] : []; const [client] = useState(() => { const cache = new QueryClient({ defaultOptions: { queries: { staleTime: Infinity, retry: false } } }); @@ -89,7 +92,24 @@ function GalleryStory({ scenario = "videos", width = 480 }: { scenario?: Scenari ); } -const meta = { title: "Tasks/Artifact Gallery", component: GalleryStory, parameters: { layout: "fullscreen" }, args: { scenario: "videos", width: 480 }, render: (args, context) => } satisfies Meta; +const meta = { + title: "Tasks/Artifact Gallery", component: GalleryStory, parameters: { layout: "fullscreen" }, + args: { scenario: "videos", width: 480 }, + beforeEach: ({ args }) => { + const originalFetch = window.fetch; + const fixtureFetch: typeof window.fetch = async (input, init) => { + const url = new URL(typeof input === "string" ? input : input instanceof URL ? input.href : input.url, window.location.origin); + const method = (init?.method ?? (input instanceof Request ? input.method : "GET")).toUpperCase(); + if (method === "GET" && url.pathname === `/api/issues/${issue.id}/work-products`) { + return Response.json(workProductsForScenario(args.scenario)); + } + return originalFetch(input, init); + }; + window.fetch = fixtureFetch; + return () => { if (window.fetch === fixtureFetch) window.fetch = originalFetch; }; + }, + render: (args, context) => , +} satisfies Meta; export default meta; type Story = StoryObj; export const EightVideoOutputs: Story = {};