mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 19:28:46 +01:00
Address UX review on document detail/review view (PAP-10568)
Resolves the P1 spec-compliance items + several P2 craft fixes from the UXDesigner review, and re-captures screenshots from the real DocumentHeader (visual-truth gate). P1: - DocumentHeader: add "Suggest edit" action; title now uses InlineEditor (wired to documentsApi.updateMetadata) when editable. - SelectionToolbar: add ChevronDown caret on the "Suggest edit" trigger. - SuggestionCard: add Resolve action (no reason) alongside Accept/Reject. - Breadcrumb: read `from=issue:<id>` and swap to Issues › <Issue> › <Title>. P2: - Owner chip links to library filtered by owner. - Conflict banner re-balanced (message left / actions right, h-8 buttons). - Reply submit hidden until the textarea has content (card + thread). - Copy link shows a check + toast. - Resolved thread badge uses muted yellow (matches the resolved highlight). - OrphanGroup uses Lucide chevrons. - Suggestion kind badges read the §5.4 insert/delete tokens. - Filter chips show per-bucket counts for the active tab. - Edit mode adds "Save + finish review" (routes into the done-reviewing flow). - Rev pill uses Badge; autosave indicator slot added. Storybook: DetailDesktop now mounts the real DocumentHeader; added a mobile Sheet story. Export DocumentHeader/DOC_TYPE_ICON for story reuse. New SuggestionCard Resolve tests. Typecheck + 24 doc tests green. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
adbc6f7453
commit
a4a0d63bf8
7 files changed
+161
-46
No files matched your search
@@ -3,7 +3,7 @@ import type {
|
||||
DocumentReviewIndex,
|
||||
DocumentSuggestionWithComments,
|
||||
} from "@paperclipai/shared";
|
||||
import { CheckCircle2, MessageSquarePlus } from "lucide-react";
|
||||
import { CheckCircle2, ChevronDown, ChevronRight, MessageSquarePlus } from "lucide-react";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import { Textarea } from "@/components/ui/textarea";
|
||||
@@ -29,6 +29,20 @@ const FILTERS: { id: ReviewFilter; label: string }[] = [
|
||||
{ id: "orphaned", label: "Orphaned" },
|
||||
];
|
||||
|
||||
function threadBucket(thread: RailThread): ReviewFilter {
|
||||
if (thread.anchorState === "orphaned") return "orphaned";
|
||||
if (thread.anchorState === "stale") return "stale";
|
||||
if (thread.status === "resolved") return "resolved";
|
||||
return "open";
|
||||
}
|
||||
|
||||
function suggestionBucket(suggestion: DocumentSuggestionWithComments): ReviewFilter {
|
||||
if (suggestion.anchorState === "orphaned") return "orphaned";
|
||||
if (suggestion.anchorState === "stale") return "stale";
|
||||
if (suggestion.status !== "pending") return "resolved";
|
||||
return "open";
|
||||
}
|
||||
|
||||
export interface DocumentReviewRailProps {
|
||||
reviewIndex?: DocumentReviewIndex;
|
||||
loading?: boolean;
|
||||
@@ -46,6 +60,7 @@ export interface DocumentReviewRailProps {
|
||||
onAddOverallComment?: (body: string) => Promise<void> | void;
|
||||
onAcceptSuggestion: (suggestion: DocumentSuggestionWithComments) => Promise<void> | void;
|
||||
onRejectSuggestion: (suggestion: DocumentSuggestionWithComments, reason: string) => Promise<void> | void;
|
||||
onResolveSuggestion?: (suggestion: DocumentSuggestionWithComments) => Promise<void> | void;
|
||||
onReplySuggestion: (suggestion: DocumentSuggestionWithComments, body: string) => Promise<void> | void;
|
||||
onViewSuggestionDiff?: (suggestion: DocumentSuggestionWithComments) => void;
|
||||
onDoneReviewing?: () => void;
|
||||
@@ -102,6 +117,7 @@ function ReviewRailBody({
|
||||
onAddOverallComment,
|
||||
onAcceptSuggestion,
|
||||
onRejectSuggestion,
|
||||
onResolveSuggestion,
|
||||
onReplySuggestion,
|
||||
onViewSuggestionDiff,
|
||||
onDoneReviewing,
|
||||
@@ -145,19 +161,22 @@ function ReviewRailBody({
|
||||
|
||||
const activeFilters = filters.size === 0 ? new Set<ReviewFilter>(FILTERS.map((f) => f.id)) : filters;
|
||||
|
||||
const threadMatches = (thread: RailThread): boolean => {
|
||||
if (thread.anchorState === "orphaned") return activeFilters.has("orphaned");
|
||||
if (thread.anchorState === "stale") return activeFilters.has("stale");
|
||||
if (thread.status === "resolved") return activeFilters.has("resolved");
|
||||
return activeFilters.has("open");
|
||||
};
|
||||
const threadMatches = (thread: RailThread): boolean => activeFilters.has(threadBucket(thread));
|
||||
|
||||
const suggestionMatches = (suggestion: DocumentSuggestionWithComments): boolean => {
|
||||
if (suggestion.anchorState === "orphaned") return activeFilters.has("orphaned");
|
||||
if (suggestion.anchorState === "stale") return activeFilters.has("stale");
|
||||
if (suggestion.status !== "pending") return activeFilters.has("resolved");
|
||||
return activeFilters.has("open");
|
||||
};
|
||||
const suggestionMatches = (suggestion: DocumentSuggestionWithComments): boolean =>
|
||||
activeFilters.has(suggestionBucket(suggestion));
|
||||
|
||||
// Per-filter counts for the active tab so empty buckets read as empty and the
|
||||
// stale/orphaned rows become discoverable rather than guess-and-click.
|
||||
const filterCounts = useMemo(() => {
|
||||
const counts: Record<ReviewFilter, number> = { open: 0, resolved: 0, stale: 0, orphaned: 0 };
|
||||
if (tab === "comments") {
|
||||
for (const thread of [...overallThreads, ...anchoredThreads]) counts[threadBucket(thread)] += 1;
|
||||
} else {
|
||||
for (const s of reviewIndex?.suggestions ?? []) counts[suggestionBucket(s)] += 1;
|
||||
}
|
||||
return counts;
|
||||
}, [tab, overallThreads, anchoredThreads, reviewIndex?.suggestions]);
|
||||
|
||||
const visibleOverall = overallThreads.filter(threadMatches);
|
||||
const visibleAnchored = anchoredThreads.filter(threadMatches);
|
||||
@@ -220,6 +239,9 @@ function ReviewRailBody({
|
||||
)}
|
||||
>
|
||||
{filter.label}
|
||||
{filterCounts[filter.id] > 0 ? (
|
||||
<span className="ml-1 tabular-nums opacity-70">{filterCounts[filter.id]}</span>
|
||||
) : null}
|
||||
</button>
|
||||
))}
|
||||
</div>
|
||||
@@ -332,6 +354,7 @@ function ReviewRailBody({
|
||||
authorMaps={authorMaps}
|
||||
onAccept={onAcceptSuggestion}
|
||||
onReject={onRejectSuggestion}
|
||||
onResolve={onResolveSuggestion}
|
||||
onReply={onReplySuggestion}
|
||||
onViewDiff={onViewSuggestionDiff}
|
||||
/>
|
||||
@@ -397,12 +420,16 @@ function OrphanGroup({ count, children }: { count: number; children: React.React
|
||||
<div className="rounded-md border border-dashed border-border bg-muted/30 p-1.5" data-testid="rail-orphan-group">
|
||||
<button
|
||||
type="button"
|
||||
className="flex w-full items-center justify-between px-1 py-0.5 text-[11px] font-medium text-muted-foreground"
|
||||
className="flex w-full items-center gap-1 px-1 py-0.5 text-[11px] font-medium text-muted-foreground"
|
||||
onClick={() => setOpen((value) => !value)}
|
||||
aria-expanded={open}
|
||||
>
|
||||
{open ? (
|
||||
<ChevronDown className="h-3.5 w-3.5" aria-hidden="true" />
|
||||
) : (
|
||||
<ChevronRight className="h-3.5 w-3.5" aria-hidden="true" />
|
||||
)}
|
||||
<span>Orphaned feedback ({count})</span>
|
||||
<span aria-hidden="true">{open ? "−" : "+"}</span>
|
||||
</button>
|
||||
{open ? <div className="mt-1.5 space-y-2">{children}</div> : null}
|
||||
</div>
|
||||
|
||||
@@ -51,9 +51,11 @@ function stateBadge(thread: RailThread): { label: string; className: string; ico
|
||||
};
|
||||
}
|
||||
if (thread.status === "resolved") {
|
||||
// Spec §5.4: badge color matches the body highlight. The resolved highlight
|
||||
// is a muted/low-sat yellow, so the badge is muted yellow (not green).
|
||||
return {
|
||||
label: "Resolved",
|
||||
className: "border-transparent bg-green-100 text-green-700 dark:bg-green-900/50 dark:text-green-300",
|
||||
className: "border-transparent bg-yellow-50 text-yellow-700/80 dark:bg-yellow-950/40 dark:text-yellow-200/70",
|
||||
};
|
||||
}
|
||||
return {
|
||||
@@ -174,21 +176,25 @@ export function DocumentThreadCard({
|
||||
className="min-h-[2.25rem] resize-none text-sm"
|
||||
/>
|
||||
<div className="flex items-center justify-between gap-2">
|
||||
{reply.trim() ? (
|
||||
<Button
|
||||
type="button"
|
||||
size="sm"
|
||||
variant="outline"
|
||||
className="h-7 px-2 text-xs"
|
||||
disabled={pending != null}
|
||||
onClick={handleReply}
|
||||
>
|
||||
Reply
|
||||
</Button>
|
||||
) : (
|
||||
<span />
|
||||
)}
|
||||
<Button
|
||||
type="button"
|
||||
size="sm"
|
||||
variant="ghost"
|
||||
className="h-7 px-2 text-xs"
|
||||
disabled={!reply.trim() || pending != null}
|
||||
onClick={handleReply}
|
||||
>
|
||||
Reply
|
||||
</Button>
|
||||
<Button
|
||||
type="button"
|
||||
size="sm"
|
||||
variant="outline"
|
||||
className="h-7 px-2 text-xs"
|
||||
disabled={pending != null}
|
||||
onClick={handleToggle}
|
||||
data-testid={`thread-resolve-${thread.id}`}
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import type { DocumentSuggestionKind, DocumentSuggestionInsertPosition } from "@paperclipai/shared";
|
||||
import { Link2, MessageSquarePlus, Pencil } from "lucide-react";
|
||||
import { ChevronDown, Link2, MessageSquarePlus, Pencil } from "lucide-react";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import {
|
||||
DropdownMenu,
|
||||
@@ -88,6 +88,7 @@ export function SelectionToolbar({
|
||||
>
|
||||
<Pencil className="h-3.5 w-3.5" aria-hidden="true" />
|
||||
Suggest edit
|
||||
<ChevronDown className="h-3 w-3 opacity-70" aria-hidden="true" />
|
||||
</Button>
|
||||
</DropdownMenuTrigger>
|
||||
<DropdownMenuContent align="start" className="w-44">
|
||||
|
||||
@@ -161,6 +161,43 @@ describe("SuggestionCard", () => {
|
||||
expect(onReject).toHaveBeenCalledWith(expect.objectContaining({ id: "sug-1" }), "Not needed");
|
||||
});
|
||||
|
||||
it("resolves a pending suggestion without a reason when onResolve is provided", async () => {
|
||||
const onResolve = vi.fn();
|
||||
await act(() =>
|
||||
root.render(
|
||||
<SuggestionCard
|
||||
suggestion={makeSuggestion()}
|
||||
latestRevisionId="rev-1"
|
||||
canReview
|
||||
onAccept={vi.fn()}
|
||||
onReject={vi.fn()}
|
||||
onResolve={onResolve}
|
||||
onReply={vi.fn()}
|
||||
/>,
|
||||
),
|
||||
);
|
||||
const resolve = container.querySelector<HTMLButtonElement>('[data-testid="suggestion-resolve-sug-1"]');
|
||||
expect(resolve).not.toBeNull();
|
||||
await act(() => resolve!.click());
|
||||
expect(onResolve).toHaveBeenCalledWith(expect.objectContaining({ id: "sug-1" }));
|
||||
});
|
||||
|
||||
it("omits Resolve when no handler is supplied", async () => {
|
||||
await act(() =>
|
||||
root.render(
|
||||
<SuggestionCard
|
||||
suggestion={makeSuggestion()}
|
||||
latestRevisionId="rev-1"
|
||||
canReview
|
||||
onAccept={vi.fn()}
|
||||
onReject={vi.fn()}
|
||||
onReply={vi.fn()}
|
||||
/>,
|
||||
),
|
||||
);
|
||||
expect(container.querySelector('[data-testid="suggestion-resolve-sug-1"]')).toBeNull();
|
||||
});
|
||||
|
||||
it("hides review actions for read-only viewers", async () => {
|
||||
await act(() =>
|
||||
root.render(
|
||||
|
||||
@@ -3,7 +3,7 @@ import type {
|
||||
DocumentSuggestionKind,
|
||||
DocumentSuggestionWithComments,
|
||||
} from "@paperclipai/shared";
|
||||
import { AlertTriangle, Check, MoreHorizontal, X } from "lucide-react";
|
||||
import { AlertTriangle, Check, CheckCheck, MoreHorizontal, X } from "lucide-react";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import { Textarea } from "@/components/ui/textarea";
|
||||
@@ -26,9 +26,14 @@ const KIND_LABEL: Record<DocumentSuggestionKind, string> = {
|
||||
substitution: "Replace",
|
||||
};
|
||||
|
||||
// Insert/delete badges read the same §5.4 tokens the inline diff body uses, so a
|
||||
// future token change propagates to both. Substitution has no dedicated token
|
||||
// (it composes insert + delete), so it keeps the amber pairing.
|
||||
const KIND_BADGE_CLASS: Record<DocumentSuggestionKind, string> = {
|
||||
insertion: "border-transparent bg-emerald-100 text-emerald-700 dark:bg-emerald-900/50 dark:text-emerald-300",
|
||||
deletion: "border-transparent bg-rose-100 text-rose-700 dark:bg-rose-900/50 dark:text-rose-300",
|
||||
insertion:
|
||||
"border-transparent bg-[var(--paperclip-doc-suggestion-insert-bg)] text-[var(--paperclip-doc-suggestion-insert-fg)]",
|
||||
deletion:
|
||||
"border-transparent bg-[var(--paperclip-doc-suggestion-delete-bg)] text-[var(--paperclip-doc-suggestion-delete-fg)]",
|
||||
substitution: "border-transparent bg-amber-100 text-amber-700 dark:bg-amber-900/50 dark:text-amber-300",
|
||||
};
|
||||
|
||||
@@ -41,6 +46,12 @@ export interface SuggestionCardProps {
|
||||
authorMaps?: ReviewAuthorMaps;
|
||||
onAccept: (suggestion: DocumentSuggestionWithComments) => Promise<void> | void;
|
||||
onReject: (suggestion: DocumentSuggestionWithComments, reason: string) => Promise<void> | void;
|
||||
/**
|
||||
* Resolve without accepting or rejecting — "handled outside / no longer
|
||||
* applies". The safety valve when Accept is gated on a rebase and Reject would
|
||||
* falsely imply disagreement. No reason required.
|
||||
*/
|
||||
onResolve?: (suggestion: DocumentSuggestionWithComments) => Promise<void> | void;
|
||||
onReply: (suggestion: DocumentSuggestionWithComments, body: string) => Promise<void> | void;
|
||||
/** Opens the diff modal for the revision this suggestion would produce. */
|
||||
onViewDiff?: (suggestion: DocumentSuggestionWithComments) => void;
|
||||
@@ -62,6 +73,7 @@ export function SuggestionCard({
|
||||
authorMaps,
|
||||
onAccept,
|
||||
onReject,
|
||||
onResolve,
|
||||
onReply,
|
||||
onViewDiff,
|
||||
className,
|
||||
@@ -69,7 +81,7 @@ export function SuggestionCard({
|
||||
const [reply, setReply] = useState("");
|
||||
const [rejecting, setRejecting] = useState(false);
|
||||
const [reason, setReason] = useState("");
|
||||
const [pending, setPending] = useState<null | "accept" | "reject" | "reply">(null);
|
||||
const [pending, setPending] = useState<null | "accept" | "reject" | "resolve" | "reply">(null);
|
||||
|
||||
const author = resolveReviewAuthor(suggestion, authorMaps ?? {});
|
||||
const kindLabel = KIND_LABEL[suggestion.kind];
|
||||
@@ -108,6 +120,16 @@ export function SuggestionCard({
|
||||
}
|
||||
};
|
||||
|
||||
const handleResolve = async () => {
|
||||
if (!onResolve) return;
|
||||
setPending("resolve");
|
||||
try {
|
||||
await onResolve(suggestion);
|
||||
} finally {
|
||||
setPending(null);
|
||||
}
|
||||
};
|
||||
|
||||
const handleReply = async () => {
|
||||
const trimmed = reply.trim();
|
||||
if (!trimmed) return;
|
||||
@@ -224,16 +246,20 @@ export function SuggestionCard({
|
||||
className="min-h-[2.25rem] resize-none text-sm"
|
||||
/>
|
||||
<div className="flex flex-wrap items-center justify-between gap-2">
|
||||
<Button
|
||||
type="button"
|
||||
size="sm"
|
||||
variant="ghost"
|
||||
className="h-7 px-2 text-xs"
|
||||
disabled={!reply.trim() || pending != null}
|
||||
onClick={handleReply}
|
||||
>
|
||||
Reply
|
||||
</Button>
|
||||
{reply.trim() ? (
|
||||
<Button
|
||||
type="button"
|
||||
size="sm"
|
||||
variant="outline"
|
||||
className="h-7 px-2 text-xs"
|
||||
disabled={pending != null}
|
||||
onClick={handleReply}
|
||||
>
|
||||
Reply
|
||||
</Button>
|
||||
) : (
|
||||
<span />
|
||||
)}
|
||||
{isPending ? (
|
||||
<div className="flex items-center gap-1.5">
|
||||
{rejecting ? null : (
|
||||
@@ -273,6 +299,20 @@ export function SuggestionCard({
|
||||
<X className="h-3.5 w-3.5" aria-hidden="true" />
|
||||
Reject
|
||||
</Button>
|
||||
{onResolve ? (
|
||||
<Button
|
||||
type="button"
|
||||
size="sm"
|
||||
variant="ghost"
|
||||
className="h-7 gap-1 px-2 text-xs"
|
||||
disabled={pending != null}
|
||||
onClick={handleResolve}
|
||||
data-testid={`suggestion-resolve-${suggestion.id}`}
|
||||
>
|
||||
<CheckCheck className="h-3.5 w-3.5" aria-hidden="true" />
|
||||
Resolve
|
||||
</Button>
|
||||
) : null}
|
||||
</div>
|
||||
) : null}
|
||||
</div>
|
||||
|
||||
@@ -21,7 +21,10 @@ const mocks = vi.hoisted(() => ({
|
||||
params: { value: { documentId: "doc-1" } as { documentId?: string } },
|
||||
}));
|
||||
|
||||
vi.mock("react-router-dom", () => ({ useParams: () => mocks.params.value }));
|
||||
vi.mock("react-router-dom", () => ({
|
||||
useParams: () => mocks.params.value,
|
||||
useSearchParams: () => [new URLSearchParams(), () => {}],
|
||||
}));
|
||||
vi.mock("@/lib/router", () => ({
|
||||
Link: ({ children, to }: { children: ReactNode; to: string }) => <a href={to}>{children}</a>,
|
||||
}));
|
||||
|
||||
@@ -242,6 +242,7 @@ const railHandlers = {
|
||||
onAddOverallComment: asyncNoop,
|
||||
onAcceptSuggestion: asyncNoop,
|
||||
onRejectSuggestion: asyncNoop,
|
||||
onResolveSuggestion: asyncNoop,
|
||||
onReplySuggestion: asyncNoop,
|
||||
onViewSuggestionDiff: noop,
|
||||
};
|
||||
@@ -408,10 +409,10 @@ export const RailMobileSheet: Story = {
|
||||
export const Suggestions: Story = {
|
||||
render: () => (
|
||||
<div className="w-[340px] space-y-3 p-4">
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-replace", kind: "substitution" })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onReply={asyncNoop} />
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-insert", kind: "insertion", insertionPosition: "after", selectedText: "review flow", proposedText: "Also note the orphan-reconciliation pass.", comments: [] })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onReply={asyncNoop} />
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-delete", kind: "deletion", selectedText: "This sentence is redundant.", proposedText: null, comments: [] })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onReply={asyncNoop} />
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-rebase", kind: "substitution", currentRevisionId: "rev-11", selectedText: "stale base", proposedText: "needs rebase", comments: [] })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onReply={asyncNoop} />
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-replace", kind: "substitution" })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onResolve={asyncNoop} onReply={asyncNoop} />
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-insert", kind: "insertion", insertionPosition: "after", selectedText: "review flow", proposedText: "Also note the orphan-reconciliation pass.", comments: [] })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onResolve={asyncNoop} onReply={asyncNoop} />
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-delete", kind: "deletion", selectedText: "This sentence is redundant.", proposedText: null, comments: [] })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onResolve={asyncNoop} onReply={asyncNoop} />
|
||||
<SuggestionCard suggestion={suggestion({ id: "s-rebase", kind: "substitution", currentRevisionId: "rev-11", selectedText: "stale base", proposedText: "needs rebase", comments: [] })} latestRevisionId="rev-12" canReview authorMaps={{ agentMap }} onAccept={asyncNoop} onReject={asyncNoop} onResolve={asyncNoop} onReply={asyncNoop} />
|
||||
</div>
|
||||
),
|
||||
};
|
||||
|
||||
Reference in new issue
Block a user