From 57902152ac3218096fca669d33ff18400a2a86ae Mon Sep 17 00:00:00 2001 From: scotttong Date: Tue, 27 Oct 2026 22:24:05 -0700 Subject: [PATCH] fix: retain partial reviews and open immutable attachments in both interfaces --- .../AttentionInteractionResolver.test.tsx | 26 +++++++++++++++++-- .../AttentionInteractionResolver.tsx | 6 ++--- .../IssueThreadInteractionCard.test.tsx | 10 +++++++ .../components/IssueThreadInteractionCard.tsx | 2 +- .../coordinator/CoordinatorNeedsYou.test.tsx | 22 +++++++++++++--- .../coordinator/CoordinatorNeedsYou.tsx | 5 +++- .../TaskChatCompactInteractionCard.tsx | 7 ++++- .../TaskChatInteractionCard.test.tsx | 12 +++++++++ ui/src/lib/item-verdict-links.test.ts | 2 +- ui/src/lib/item-verdict-links.ts | 4 ++- 10 files changed, 82 insertions(+), 14 deletions(-) diff --git a/ui/src/components/AttentionInteractionResolver.test.tsx b/ui/src/components/AttentionInteractionResolver.test.tsx index 96ef9a9dbd9..7c3f3059898 100644 --- a/ui/src/components/AttentionInteractionResolver.test.tsx +++ b/ui/src/components/AttentionInteractionResolver.test.tsx @@ -5,7 +5,7 @@ import { createRoot } from "react-dom/client"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { afterEach, expect, it, vi } from "vitest"; import { issuesApi } from "../api/issues"; -import { pendingRequestConfirmationInteraction } from "../fixtures/issueThreadInteractionFixtures"; +import { pendingRequestConfirmationInteraction, pendingRequestItemVerdictsInteraction } from "../fixtures/issueThreadInteractionFixtures"; import { ThemeProvider } from "../context/ThemeContext"; import { TooltipProvider } from "./ui/tooltip"; import { AttentionInteractionResolver } from "./AttentionInteractionResolver"; @@ -17,7 +17,7 @@ vi.mock("@/lib/router", () => ({ })); vi.mock("../api/issues", () => ({ - issuesApi: { listInteractions: vi.fn(), acceptInteraction: vi.fn() }, + issuesApi: { listInteractions: vi.fn(), acceptInteraction: vi.fn(), submitInteractionVerdicts: vi.fn() }, })); afterEach(() => { @@ -25,6 +25,28 @@ afterEach(() => { vi.clearAllMocks(); }); +it("passes the canonical pending status back after a partial item review", async () => { + const interaction = pendingRequestItemVerdictsInteraction; + const partial = { ...interaction, status: "pending" as const, result: { version: 1 as const, outcome: "resolved" as const, + complete: false, items: [{ id: interaction.payload.items[0]!.id, verdict: "approve" as const, resolvedAt: new Date().toISOString() }] } }; + vi.mocked(issuesApi.listInteractions).mockResolvedValue([interaction]); + vi.mocked(issuesApi.submitInteractionVerdicts).mockResolvedValue(partial); + const onResolved = vi.fn(); + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const host = document.createElement("div"); document.body.append(host); const root = createRoot(host); + try { + await act(async () => root.render( + + )); + await act(async () => { await new Promise(resolve => setTimeout(resolve, 0)); }); + await act(async () => host.querySelector('button[data-verdict="approve"]')!.click()); + const apply = [...host.querySelectorAll("button")].find(button => button.textContent?.startsWith("Apply")); + expect(apply).toBeTruthy(); + await act(async () => apply!.click()); + expect(onResolved).toHaveBeenCalledWith(partial); + } finally { await act(async () => root.unmount()); host.remove(); client.clear(); } +}); + it("refreshes a preparing approval automatically, then stops polling without accepting it", async () => { vi.useFakeTimers(); const interaction = pendingRequestConfirmationInteraction; diff --git a/ui/src/components/AttentionInteractionResolver.tsx b/ui/src/components/AttentionInteractionResolver.tsx index f1cd21611a3..90c8d7f11ad 100644 --- a/ui/src/components/AttentionInteractionResolver.tsx +++ b/ui/src/components/AttentionInteractionResolver.tsx @@ -28,8 +28,8 @@ interface AttentionInteractionResolverProps { userLabelMap?: ReadonlyMap | null; footerSlot?: ReactNode; presentation?: "default" | "coordinator-decision"; - /** Called after a resolution so the parent can refresh the feed. */ - onResolved?: () => void; + /** Successful updates include partial item reviews; inspect status before settling the whole card. */ + onResolved?: (interaction?: IssueThreadInteraction) => void; } export function replaceResolvedInteraction( @@ -90,7 +90,7 @@ export function AttentionInteractionResolver({ queryClient.invalidateQueries({ queryKey: queryKeys.attention(companyId) }); queryClient.invalidateQueries({ queryKey: ["issues", "pending-questions"] }); queryClient.invalidateQueries({ queryKey: ["issues", "decision-receipts"] }); - onResolved?.(); + onResolved?.(resolvedInteraction); }; const acceptMutation = useMutation({ diff --git a/ui/src/components/IssueThreadInteractionCard.test.tsx b/ui/src/components/IssueThreadInteractionCard.test.tsx index 47650550be5..a91e4d5e2a7 100644 --- a/ui/src/components/IssueThreadInteractionCard.test.tsx +++ b/ui/src/components/IssueThreadInteractionCard.test.tsx @@ -1148,6 +1148,16 @@ describe("IssueThreadInteractionCard", () => { ); }); + it("opens a bound attachment directly without experimental panel routing", () => { + const host = renderCard({ interaction: { ...pendingRequestItemVerdictsInteraction, + payload: { ...pendingRequestItemVerdictsInteraction.payload, items: [{ id: "file", label: "Reviewed file", target: { + type: "issue_attachment", attachmentId: "immutable-file", assetId: "asset", sha256: "hash", + } }] }, + } }); + const link = host.querySelector('a[href="/api/attachments/immutable-file/content"]'); + expect(link?.getAttribute("target")).toBe("_blank"); + }); + it("links a bound review item to its exact document without requiring href", () => { const host = renderCard({ interaction: { ...pendingRequestItemVerdictsInteraction, payload: { ...pendingRequestItemVerdictsInteraction.payload, items: [{ id: "copy", label: "Reviewed copy", target: { diff --git a/ui/src/components/IssueThreadInteractionCard.tsx b/ui/src/components/IssueThreadInteractionCard.tsx index 746f08e02c9..cc4997460d3 100644 --- a/ui/src/components/IssueThreadInteractionCard.tsx +++ b/ui/src/components/IssueThreadInteractionCard.tsx @@ -3243,7 +3243,7 @@ function VerdictConsequenceChip({ verdict }: { verdict: RequestItemVerdictValue function ItemVerdictDeepLink({ item, interaction }: { item: RequestItemVerdictsItem; interaction: RequestItemVerdictsInteraction }) { const href = itemVerdictHref(item, interaction); if (!href) return null; - const isInternal = href.startsWith("/") || href.startsWith("#"); + const isInternal = !href.startsWith("/api/") && (href.startsWith("/") || href.startsWith("#")); const className = "inline-flex items-center gap-1 text-xs font-medium text-primary hover:underline focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-1"; const label = ( diff --git a/ui/src/components/coordinator/CoordinatorNeedsYou.test.tsx b/ui/src/components/coordinator/CoordinatorNeedsYou.test.tsx index cb43df674b7..b42d85c7030 100644 --- a/ui/src/components/coordinator/CoordinatorNeedsYou.test.tsx +++ b/ui/src/components/coordinator/CoordinatorNeedsYou.test.tsx @@ -7,7 +7,7 @@ import type { AttentionItem, DecisionHistoryItem } from "@paperclipai/shared"; import { CoordinatorNeedsYou } from "./CoordinatorNeedsYou"; (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; -const fixture = vi.hoisted(() => ({ pending: [] as AttentionItem[], history: [] as DecisionHistoryItem[], historyFail: false, feedFail: false, openPanel: vi.fn(), closePanel: vi.fn(), setPanelVisible: vi.fn(), navigate: vi.fn(), breadcrumbs: vi.fn() })); +const fixture = vi.hoisted(() => ({ pending: [] as AttentionItem[], history: [] as DecisionHistoryItem[], partial: false, historyFail: false, feedFail: false, openPanel: vi.fn(), closePanel: vi.fn(), setPanelVisible: vi.fn(), navigate: vi.fn(), breadcrumbs: vi.fn() })); vi.mock("@/context/CompanyContext", () => ({ useCompany: () => ({ selectedCompanyId: "company" }) })); vi.mock("@/context/BreadcrumbContext", () => ({ useBreadcrumbs: () => ({ setBreadcrumbs: fixture.breadcrumbs }) })); vi.mock("@/context/PanelContext", () => ({ usePanel: () => ({ openPanel: fixture.openPanel, closePanel: fixture.closePanel, setPanelVisible: fixture.setPanelVisible }) })); @@ -30,11 +30,12 @@ vi.mock("@/components/StalledReviewActions", () => ({ StalledReviewActions: ({ i fixture.pending = fixture.pending.filter((item) => item.subject.id !== issueId); onResolved(); }}>{approveLabel} })); -vi.mock("@/components/AttentionInteractionResolver", () => ({ AttentionInteractionResolver: ({ issueId, interactionId, footerSlot, onResolved }: { issueId: string; interactionId: string; footerSlot?: ReactNode; onResolved: () => void }) => <>{footerSlot} })); let root: Root; @@ -42,7 +43,7 @@ let container: HTMLDivElement; beforeEach(() => { fixture.pending = ["1", "2", "3"].map((id) => ({ id, sourceKind: "issue_thread_interaction", inlineResolvable: true, subject: { id, kind: "interaction", title: `Review ${id}`, metadata: { issueId: `task-${id}` } }, relatedIssue: { id: `task-${id}`, identifier: `PAP-${id}`, title: `Task ${id}`, href: `/PAP/issues/PAP-${id}` }, activityAt: new Date().toISOString() } as unknown as AttentionItem)); fixture.history = []; vi.clearAllMocks(); - fixture.historyFail = false; fixture.feedFail = false; + fixture.historyFail = false; fixture.feedFail = false; fixture.partial = false; container = document.createElement("div"); document.body.appendChild(container); root = createRoot(container); }); afterEach(async () => { await act(async () => root.unmount()); container.remove(); vi.useRealTimers(); vi.restoreAllMocks(); }); @@ -53,6 +54,19 @@ async function click(text: string) { await act(async () => button!.click()); await flush(); } describe("Coordinator Needs You", () => { + it("keeps a partial review actionable even when history is unavailable", async () => { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + await act(async () => root.render()); + await flush(); + fixture.partial = true; fixture.historyFail = true; + await click("Resolve 1"); await flush(); + expect([...container.querySelectorAll("button")].some(button => button.textContent === "Resolve 1")).toBe(true); + expect(container.querySelector('[data-motion="gather"]')).toBeNull(); + fixture.partial = false; + await click("Resolve 1"); await flush(); + expect([...container.querySelectorAll("button")].some(button => button.textContent === "Resolve 1")).toBe(false); + client.clear(); + }); it.each([false, true])("does not restore a saved decision when history refresh fails (stale feed: %s)", async staleFeed => { const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); await act(async () => root.render()); diff --git a/ui/src/components/coordinator/CoordinatorNeedsYou.tsx b/ui/src/components/coordinator/CoordinatorNeedsYou.tsx index 35ffe212d31..cf79201e751 100644 --- a/ui/src/components/coordinator/CoordinatorNeedsYou.tsx +++ b/ui/src/components/coordinator/CoordinatorNeedsYou.tsx @@ -200,7 +200,10 @@ export function CoordinatorNeedsYou() { setSettling(current => current.filter(entry => `${entry.sourceKind}:${entry.subject.id}` !== id))}> {receipt ? :
- {interaction ? onResolved(item)} /> + {interaction ? { + if (!resolved || resolved.status === "pending") { void feed.refetch(); void recent.refetch(); return; } + onResolved(item); + }} /> : decision ? onResolved(item)} /> : stalledReview ? <>

Choose how this task continues

This task is still in review with no active reviewer or continuation. Return it to work, request changes, or mark the task done.

{ void feed.refetch(); void recent.refetch(); }} /> : <>

{item.subject.title ?? item.whyNow}

{attentionDetailLine(item) ?? item.whyNow}

} diff --git a/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx b/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx index 5f2ed841192..a1b63b65801 100644 --- a/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx +++ b/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx @@ -1,4 +1,5 @@ import { itemVerdictHref } from "@/lib/item-verdict-links"; +import { Link } from "@/lib/router"; import { useEffect, useMemo, @@ -1773,9 +1774,13 @@ function ItemVerdictsCard({

) : null} - {itemHref ? ( + {itemHref && !itemHref.startsWith("/api/") && (itemHref.startsWith("/") || itemHref.startsWith("#")) ? ( + Open + ) : itemHref ? ( Open diff --git a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx index 9cbc26bfd28..4f9e8440a70 100644 --- a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx +++ b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx @@ -18,6 +18,10 @@ import { pendingRequestItemVerdictsInteraction, } from "@/fixtures/issueThreadInteractionFixtures"; import { TaskChatInteractionCard } from "./TaskChatInteractionCard"; +vi.mock("@/lib/router", async importOriginal => ({ + ...await importOriginal(), + Link: ({ to, children }: { to: string; children: React.ReactNode }) => {children}, +})); import { TaskChatThreadView } from "./TaskChatThreadView"; import type { TaskChatInteractionItem } from "./task-chat-model"; @@ -838,6 +842,14 @@ describe("TaskChatInteractionCard", () => { }]); }); + it("opens a compact attachment item through the authenticated file endpoint", () => { + const interaction = { ...pendingRequestItemVerdictsInteraction, payload: { ...pendingRequestItemVerdictsInteraction.payload, + items: [{ id: "file", label: "Reviewed file", target: { type: "issue_attachment" as const, attachmentId: "immutable-file", assetId: "asset", sha256: "hash" } }], + } }; + flushSync(() => root.render( undefined} />)); + expect(container.querySelector('a[href="/api/attachments/immutable-file/content"]')?.getAttribute("target")).toBe("_blank"); + }); + it("opens the bound output revision from a compact review item", () => { const interaction = { ...pendingRequestItemVerdictsInteraction, payload: { ...pendingRequestItemVerdictsInteraction.payload, items: [{ id: "copy", label: "Reviewed copy", target: { type: "issue_document" as const, issueId: "owner", key: "copy", revisionId: "reviewed-v1" } }], diff --git a/ui/src/lib/item-verdict-links.test.ts b/ui/src/lib/item-verdict-links.test.ts index 5e671488bf4..b6b9d52bd32 100644 --- a/ui/src/lib/item-verdict-links.test.ts +++ b/ui/src/lib/item-verdict-links.test.ts @@ -10,7 +10,7 @@ describe("review output links", () => { }); it("opens the immutable attachment in its owning task", () => { expect(itemVerdictHref({ id: "art", label: "Artwork", target: { type: "issue_attachment", issueId: "owner", attachmentId: "asset-file", assetId: "asset", sha256: "hash" } }, interaction)) - .toBe("/issues/owner#interaction-review&attachment=asset-file"); + .toBe("/api/attachments/asset-file/content"); }); it("keeps safe legacy links and rejects unsafe custom URLs", () => { expect(itemVerdictHref({ id: "legacy", label: "Legacy", href: "javascript:alert(1)" }, interaction)).toBeNull(); diff --git a/ui/src/lib/item-verdict-links.ts b/ui/src/lib/item-verdict-links.ts index 90424b64916..7d207c9781e 100644 --- a/ui/src/lib/item-verdict-links.ts +++ b/ui/src/lib/item-verdict-links.ts @@ -11,7 +11,9 @@ export function itemVerdictHref(item: RequestItemVerdictsItem, interaction: { is })}`; } if (target?.type === "issue_attachment") { - return `/issues/${encodeURIComponent(target.issueId ?? interaction.issueId)}#interaction-${encodeURIComponent(interaction.id)}&attachment=${encodeURIComponent(target.attachmentId)}`; + // Use the authenticated immutable-content route in both interface modes. + // This does not depend on the experimental task attachment panel. + return `/api/attachments/${encodeURIComponent(target.attachmentId)}/content`; } return item.href ? normalizeRequestConfirmationTargetHref(item.href) : null; }