From bfe7c6357d1e0c945c8cb84259153cb3b6542d28 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 11 Oct 2026 12:29:05 -0700 Subject: [PATCH] fix(ui): stack confirmation prompt and hide raw custom target keys (#15895) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents ask the board for decisions with `request_confirmation` interaction cards > - The compact task-chat card puts the prompt and the target pill in one `justify-between` flex row > - An agent can send a `custom` target with only a machine `key` (for example `github:org/repo:pull:16`) and no `label` > - The card shows that raw key as a mono pill that does not shrink, so the card splits about 50/50 and the prompt is squeezed into the left half > - This pull request stacks the prompt above the pill and hides custom targets that have no human label > - The benefit is a readable, full-width confirmation prompt without meaningless identifier pills ## Linked Issues or Issue Description **What happened?** A pending confirmation card split its body into two columns. The prompt used the left half. A pill with the raw text `github:paperclipai/paperclip-desktop:pull:16` used the right half. The line breaks in the prompt were also lost. **Steps to reproduce** 1. Create a `request_confirmation` interaction with a multi-line `prompt` and `target: { "type": "custom", "key": "github:org/repo:pull:16" }` (no `label`). 2. Open the task chat for the issue. 3. See that the prompt and the key pill share one row, split about 50/50. **Expected behavior** The prompt uses the full card width and keeps its line breaks. A custom target key is an identifier for idempotency and stale detection, so it does not show as display text. A target pill shows only when the target has a human label. ## What Changed - `TaskChatCompactInteractionCard`: stack the prompt full width with the target pill below it. This applies to the confirmation and the checkbox-confirmation cards. - `TaskChatCompactInteractionCard`: keep line breaks in the prompt (`whitespace-pre-line`, `break-words`). - `TaskChatCompactInteractionCard` and `IssueThreadInteractionCard`: show no target chip for a `custom` target without a `label`. Labeled custom targets (for example connection authorization) and `issue_document` targets do not change. The stale-target chevron shows only when both chips show. - Add regression tests for the stacked layout, the hidden unlabeled key, and the labeled link pill. The tests cover the compact confirmation and checkbox-confirmation cards, and the thread card pending chip and stale-target expiry (with labeled, unlabeled, and mixed custom targets, and the chevron rule). - `CONTRIBUTING.md`: add a "Keep Screenshots Out of the Repo" section. Contributors attach review screenshots to the PR on GitHub. They do not commit them to the repository. ## Verification - `cd ui && npx vitest run src/components/task-chat/TaskChatInteractionCard.test.tsx src/components/IssueThreadInteractionCard.test.tsx`: 2 files, 100 tests pass. If you revert the `isDisplayableRequestConfirmationTarget` guard, 3 of the new thread-card tests fail. - `node scripts/check-token-gates.mjs`: all gates clean. - `pnpm --filter @paperclipai/ui typecheck`: no errors in changed files. The only errors come from `@paperclipai/plugin-sdk/ui`, which had no built `dist` in the local workspace. These errors are not related to this change. - Manual: create a confirmation as in the steps above. The prompt is full width and no key pill shows. Add `label: "PR #16"` and an `href`. A `PR #16` link pill shows below the prompt. ### Screenshots These are Storybook renders of the compact task-chat card at 720 px. The three cards are: a confirmation with an unlabeled custom target (the reported case), a confirmation with a labeled custom target, and a checkbox confirmation with an unlabeled custom target. The images load from an earlier commit of this PR. A later commit deletes them, so they are not part of the merged change. | Before (light) | After (light) | After (dark) | | --- | --- | --- | | ![before](https://raw.githubusercontent.com/paperclipai/paperclip/ea22e6b484f21de1dbed258286bfc271cdbcc9a1/docs/pr-screenshots/pr-15895/before-light.png) | ![after light](https://raw.githubusercontent.com/paperclipai/paperclip/ea22e6b484f21de1dbed258286bfc271cdbcc9a1/docs/pr-screenshots/pr-15895/after-light.png) | ![after dark](https://raw.githubusercontent.com/paperclipai/paperclip/ea22e6b484f21de1dbed258286bfc271cdbcc9a1/docs/pr-screenshots/pr-15895/after-dark.png) | ## Risks - Low risk. This is a UI-only change. No schema or API change. - Agents that send a custom target without a label lose the visible pill. The key was never readable display text. To show a pill, agents set `label`. ## Merge Note The first commit was authored as Paperclip. When you squash-merge, keep this trailer in the squash body: Co-Authored-By: Paperclip ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code agent harness with tool use (shell, file edit). Standard context window. ## 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 - [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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Paperclip Co-authored-by: Claude Opus 5.5 Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> --- CONTRIBUTING.md | 8 ++ .../IssueThreadInteractionCard.test.tsx | 65 ++++++++++++++ .../components/IssueThreadInteractionCard.tsx | 14 ++- .../TaskChatCompactInteractionCard.tsx | 16 ++-- .../TaskChatInteractionCard.test.tsx | 89 +++++++++++++++++++ 5 files changed, 183 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a1dab6f5f3..31033ea98c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -94,6 +94,14 @@ git push -u origin git push origin --delete ``` +### Keep Screenshots Out of the Repo + +Screenshots, screen recordings, and other review evidence belong in the PR description or a PR comment, not in the repository. Do **not** commit them (for example under `docs/pr-screenshots/`). Committed review images land on `master` permanently and make every clone bigger, even though nobody needs them after the PR merges. + +To show a visual change, drag the image into the PR description or a comment on GitHub, which hosts the file for you. If your tooling cannot upload to GitHub, leave the images out and describe what you checked under **Verification**. + +Images that are part of the product documentation itself, such as a screenshot embedded in a guide under `doc/`, are fine to commit. + ### Model Used (Required) Every PR must include a **Model Used** section specifying which AI model produced or assisted with the change. Include the provider, exact model ID/version, context window size, and any relevant capability details (e.g., reasoning mode, tool use). If no AI was used, write "None โ€” human-authored". This applies to all contributors โ€” human and AI alike. diff --git a/ui/src/components/IssueThreadInteractionCard.test.tsx b/ui/src/components/IssueThreadInteractionCard.test.tsx index 2dd3d1ef26..3921e02074 100644 --- a/ui/src/components/IssueThreadInteractionCard.test.tsx +++ b/ui/src/components/IssueThreadInteractionCard.test.tsx @@ -872,9 +872,74 @@ describe("IssueThreadInteractionCard", () => { expect(host.textContent).toContain("Plan v4"); expect(targetLinks[0]?.getAttribute("href")).toContain("#document-plan"); expect(targetLinks[1]?.getAttribute("href")).toContain("#document-plan"); + expect(host.querySelector(".lucide-chevron-right")).not.toBeNull(); expect(host.textContent).not.toContain("Approve plan"); }); + describe("custom request confirmation targets", () => { + const prKey = (pull: number) => `github:paperclipai/paperclip-desktop:pull:${pull}`; + const unlabeled = (pull: number) => ({ type: "custom" as const, key: prKey(pull) }); + const labeled = (pull: number) => ({ + ...unlabeled(pull), + label: `PR #${pull}`, + href: `https://github.com/paperclipai/paperclip-desktop/pull/${pull}`, + }); + const staleTargetChange = ( + staleTarget: ReturnType, + target: ReturnType, + ) => ({ + ...staleTargetRequestConfirmationInteraction, + payload: { ...staleTargetRequestConfirmationInteraction.payload, target }, + result: { version: 1 as const, outcome: "stale_target" as const, staleTarget }, + }); + + it("hides an unlabeled target key on a pending card", () => { + const host = renderCard({ + interaction: { + ...pendingRequestConfirmationInteraction, + payload: { ...pendingRequestConfirmationInteraction.payload, target: unlabeled(16) }, + }, + }); + expect(host.textContent).not.toContain("github:paperclipai"); + }); + + it("links a labeled target on a pending card", () => { + const host = renderCard({ + interaction: { + ...pendingRequestConfirmationInteraction, + payload: { ...pendingRequestConfirmationInteraction.payload, target: labeled(16) }, + }, + }); + const link = host.querySelector( + 'a[href="https://github.com/paperclipai/paperclip-desktop/pull/16"]', + ); + expect(link?.textContent).toContain("PR #16"); + expect(host.textContent).not.toContain("github:paperclipai"); + }); + + it("shows no chips or chevron for an unlabeled stale-target change", () => { + const host = renderCard({ interaction: staleTargetChange(unlabeled(15), unlabeled(16)) }); + expect(host.textContent).toContain("Expired by target change"); + expect(host.textContent).not.toContain("github:paperclipai"); + expect(host.querySelector(".lucide-chevron-right")).toBeNull(); + }); + + it("links both labeled targets across a stale-target chevron", () => { + const host = renderCard({ interaction: staleTargetChange(labeled(15), labeled(16)) }); + expect(host.textContent).toContain("PR #15"); + expect(host.textContent).toContain("PR #16"); + expect(host.textContent).not.toContain("github:paperclipai"); + expect(host.querySelector(".lucide-chevron-right")).not.toBeNull(); + }); + + it("drops the chevron when only one side of the stale-target change is labeled", () => { + const host = renderCard({ interaction: staleTargetChange(unlabeled(15), labeled(16)) }); + expect(host.textContent).toContain("PR #16"); + expect(host.textContent).not.toContain("github:paperclipai"); + expect(host.querySelector(".lucide-chevron-right")).toBeNull(); + }); + }); + it("renders a jump link for confirmations expired by comment", () => { const host = renderCard({ interaction: commentExpiredRequestConfirmationInteraction, diff --git a/ui/src/components/IssueThreadInteractionCard.tsx b/ui/src/components/IssueThreadInteractionCard.tsx index 402e06c1e7..1cc6009d85 100644 --- a/ui/src/components/IssueThreadInteractionCard.tsx +++ b/ui/src/components/IssueThreadInteractionCard.tsx @@ -1478,6 +1478,15 @@ function AskUserQuestionsCard({ ); } +/** A custom target key is a machine identifier (e.g. `github:org/repo:pull:16`), + * not display copy, so unlabeled custom targets render no chip. */ +function isDisplayableRequestConfirmationTarget( + target: RequestConfirmationTarget | null | undefined, +): target is RequestConfirmationTarget { + if (!target) return false; + return target.type !== "custom" || Boolean(target.label); +} + function requestConfirmationTargetLabel(target: RequestConfirmationTarget) { if (target.label) return target.label; const revision = target.revisionNumber ? ` v${target.revisionNumber}` : ""; @@ -1511,7 +1520,7 @@ function RequestConfirmationTargetChip({ target: RequestConfirmationTarget | null | undefined; tone?: "default" | "subtle"; }) { - if (!target) return null; + if (!isDisplayableRequestConfirmationTarget(target)) return null; const href = requestConfirmationTargetHref({ interaction, target }); const className = cn( @@ -1652,7 +1661,8 @@ function RequestConfirmationResolution({ target={staleTarget} tone="subtle" /> - {staleTarget && target ? ( + {isDisplayableRequestConfirmationTarget(staleTarget) + && isDisplayableRequestConfirmationTarget(target) ? ( ) : null} diff --git a/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx b/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx index 8dbcdd52e6..1fca49b253 100644 --- a/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx +++ b/ui/src/components/task-chat/TaskChatCompactInteractionCard.tsx @@ -275,9 +275,11 @@ function targetLabel( ) { const target = interaction.payload.target; if (!target) return null; - const label = - target.label ?? - (target.type === "issue_document" ? target.key : target.key); + // A custom target key is a machine identifier for idempotency and stale + // detection (e.g. `github:org/repo:pull:16`), not display copy. Only show a + // custom target when the agent supplied a human label. + if (target.type === "custom" && !target.label) return null; + const label = target.label ?? target.key; const revision = target.revisionNumber == null ? null : `v${target.revisionNumber}`; return [label, revision].filter(Boolean).join(" ยท "); @@ -862,8 +864,8 @@ function ConfirmationCard({ planDocument={planDocument} /> ) : isPlanConfirmation && rejecting ? null : ( -
-

+

+

{isPlanConfirmation ? "Do you accept this plan?" : interaction.payload.prompt} @@ -1152,8 +1154,8 @@ function CheckboxConfirmationCard({ return (

-
-

+

+

{interaction.payload.prompt}

diff --git a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx index 7a5a4284ec..194855c99c 100644 --- a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx +++ b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx @@ -370,6 +370,95 @@ describe("TaskChatInteractionCard", () => { ).toBeNull(); }); + it("stacks the confirmation prompt full-width and hides unlabeled custom target keys", () => { + const render = (target: RequestConfirmationInteraction["payload"]["target"]) => + flushSync(() => { + root.render( + + + + + , + ); + }); + + render({ type: "custom", key: "github:paperclipai/paperclip-desktop:pull:16" }); + expect(container.textContent).not.toContain("github:paperclipai"); + const prompt = Array.from(container.querySelectorAll("p")).find((node) => + node.textContent?.startsWith("The change is open"), + ); + expect(prompt?.className).not.toContain("flex-1"); + expect(prompt?.className).toContain("whitespace-pre-line"); + expect(prompt?.parentElement?.className).not.toContain("justify-between"); + + render({ + type: "custom", + key: "github:paperclipai/paperclip-desktop:pull:16", + label: "PR #16", + href: "https://github.com/paperclipai/paperclip-desktop/pull/16", + }); + const link = container.querySelector( + 'a[href="https://github.com/paperclipai/paperclip-desktop/pull/16"]', + ); + expect(link?.textContent).toBe("PR #16"); + expect(container.textContent).not.toContain("github:paperclipai"); + }); + + it("stacks the checkbox confirmation prompt full-width and hides unlabeled custom target keys", () => { + const render = (target: RequestConfirmationInteraction["payload"]["target"]) => + flushSync(() => { + root.render( + + + + + , + ); + }); + + render({ type: "custom", key: "github:paperclipai/paperclip-desktop:pull:16" }); + expect(container.textContent).not.toContain("github:paperclipai"); + const prompt = Array.from(container.querySelectorAll("p")).find((node) => + node.textContent?.startsWith("Pick the drafts"), + ); + expect(prompt?.className).not.toContain("flex-1"); + expect(prompt?.className).toContain("whitespace-pre-line"); + expect(prompt?.parentElement?.className).not.toContain("justify-between"); + + render({ + type: "custom", + key: "github:paperclipai/paperclip-desktop:pull:16", + label: "PR #16", + href: "https://github.com/paperclipai/paperclip-desktop/pull/16", + }); + const link = container.querySelector( + 'a[href="https://github.com/paperclipai/paperclip-desktop/pull/16"]', + ); + expect(link?.textContent).toBe("PR #16"); + expect(container.textContent).not.toContain("github:paperclipai"); + }); + it("keeps the plan revision in the header and hides the prompt while requesting changes", async () => { const interaction = createRequestConfirmation({ sourceRunId: "run-plan",