mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-06 23:20:53 +01:00
Surface review-thread access error instead of silent empty state (PAP-10580)
Doc-detail's review-index query is gated by the parent issue's authorization boundary, but the document itself is listable via the company-scoped library. A board user who can browse a doc but not its issue got a 403 that react-query swallowed, falling through to a misleading "No feedback yet" empty state and a Done-reviewing dialog reporting 0 comments / 0 suggestions. Catch the 403 and surface it: the rail renders a "Review thread unavailable" panel naming the parent issue to ask for access, the Done-reviewing CTA is suppressed (no stats to hand off), and the dialog shows the access message instead of misleading zeros. Also stop retrying the query on 403/404.
This commit is contained in:
1 parent
ae8ac332b4
commit
931e7ac40d
4 files changed
+74
-5
No files matched your search
@@ -3,7 +3,7 @@ import type {
|
||||
DocumentReviewIndex,
|
||||
DocumentSuggestionWithComments,
|
||||
} from "@paperclipai/shared";
|
||||
import { CheckCircle2, ChevronDown, ChevronRight, MessageSquarePlus } from "lucide-react";
|
||||
import { CheckCircle2, ChevronDown, ChevronRight, Lock, MessageSquarePlus } from "lucide-react";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import { Textarea } from "@/components/ui/textarea";
|
||||
@@ -46,6 +46,12 @@ function suggestionBucket(suggestion: DocumentSuggestionWithComments): ReviewFil
|
||||
export interface DocumentReviewRailProps {
|
||||
reviewIndex?: DocumentReviewIndex;
|
||||
loading?: boolean;
|
||||
/**
|
||||
* When the viewer can browse the document but can't read its review thread
|
||||
* (the review-index endpoint is gated by parent-issue access), surface this
|
||||
* message instead of a misleading "no feedback" empty state.
|
||||
*/
|
||||
accessError?: string | null;
|
||||
/** Viewer can reply/resolve/accept/reject. */
|
||||
canReview: boolean;
|
||||
/** Viewer has edit rights (gates the Done reviewing CTA). */
|
||||
@@ -105,6 +111,7 @@ export function DocumentReviewRail(props: DocumentReviewRailProps) {
|
||||
function ReviewRailBody({
|
||||
reviewIndex,
|
||||
loading,
|
||||
accessError,
|
||||
canReview,
|
||||
canFinishReview,
|
||||
doneReviewingDisabledReason,
|
||||
@@ -210,6 +217,16 @@ function ReviewRailBody({
|
||||
}
|
||||
};
|
||||
|
||||
if (accessError) {
|
||||
return (
|
||||
<div className="flex h-full min-h-0 flex-col items-center justify-center p-6 text-center" data-testid="rail-access-error">
|
||||
<Lock className="mb-3 h-6 w-6 text-muted-foreground" aria-hidden="true" />
|
||||
<p className="text-sm font-medium text-foreground">Review thread unavailable</p>
|
||||
<p className="mt-1 max-w-[28ch] text-xs text-muted-foreground">{accessError}</p>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<TooltipProvider delayDuration={200}>
|
||||
<div className="flex h-full min-h-0 flex-col">
|
||||
|
||||
@@ -27,6 +27,11 @@ export interface DoneReviewingDialogProps {
|
||||
open: boolean;
|
||||
onOpenChange: (open: boolean) => void;
|
||||
counts?: DocumentReviewIndexCounts;
|
||||
/**
|
||||
* When the review index couldn't be loaded (e.g. the viewer lacks parent-issue
|
||||
* access), surface this message instead of misleading zero stats.
|
||||
*/
|
||||
accessError?: string | null;
|
||||
/** Linked issue identifier, used to label the "comment on issue" toggle. */
|
||||
issueIdentifier?: string | null;
|
||||
/** Owner agent display name, used to label the "wake owner" toggle. */
|
||||
@@ -57,6 +62,7 @@ export function DoneReviewingDialog({
|
||||
open,
|
||||
onOpenChange,
|
||||
counts,
|
||||
accessError,
|
||||
issueIdentifier,
|
||||
ownerName,
|
||||
canWakeOwner = true,
|
||||
@@ -97,7 +103,16 @@ export function DoneReviewingDialog({
|
||||
</DialogDescription>
|
||||
</DialogHeader>
|
||||
|
||||
{/* Review summary */}
|
||||
{/* Review summary — suppressed when the counts couldn't be loaded, since
|
||||
zeros would falsely read as "nothing to hand off". */}
|
||||
{accessError ? (
|
||||
<p
|
||||
className="rounded-md border border-amber-300 bg-amber-50 px-3 py-2 text-xs text-amber-800 dark:border-amber-800 dark:bg-amber-950/40 dark:text-amber-200"
|
||||
data-testid="done-reviewing-access-error"
|
||||
>
|
||||
{accessError}
|
||||
</p>
|
||||
) : (
|
||||
<div className="grid grid-cols-4 gap-2" data-testid="done-reviewing-summary">
|
||||
{stats.map((stat) => (
|
||||
<div
|
||||
@@ -118,7 +133,8 @@ export function DoneReviewingDialog({
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
{!hasUnresolved ? (
|
||||
)}
|
||||
{!accessError && !hasUnresolved ? (
|
||||
<p className="text-xs text-muted-foreground" data-testid="done-reviewing-clean">
|
||||
No open feedback — this is a clean handoff.
|
||||
</p>
|
||||
|
||||
@@ -213,6 +213,23 @@ describe("DocumentDetail", () => {
|
||||
expect(edit?.disabled).toBe(true);
|
||||
});
|
||||
|
||||
it("surfaces a review-access error instead of an empty 'no feedback' state on 403", async () => {
|
||||
mocks.documentsApi.get.mockResolvedValue(makeDoc());
|
||||
mocks.documentReviewsApi.reviewIndex.mockRejectedValue(new ApiError("Forbidden", 403, null));
|
||||
await renderPage();
|
||||
await waitFor(
|
||||
() => container.querySelector('[data-testid="rail-access-error"]') !== null,
|
||||
"review access error",
|
||||
);
|
||||
// References the parent issue so the user knows whom to ask.
|
||||
expect(container.textContent).toContain("don't have access to this document's review thread");
|
||||
expect(container.textContent).toContain("PAP-10520");
|
||||
// The misleading empty state and the Done-reviewing CTA must NOT render —
|
||||
// there are no stats to hand off when the thread can't be read.
|
||||
expect(container.querySelector('[data-testid="rail-empty"]')).toBeNull();
|
||||
expect(container.querySelector('[data-testid="rail-done-reviewing"]')).toBeNull();
|
||||
});
|
||||
|
||||
it("shows a locked-by-other badge and disables Edit", async () => {
|
||||
mocks.documentsApi.get.mockResolvedValue(makeDoc({ lockedByAgentId: "agent-1", lockedAt: new Date() }));
|
||||
await renderPage();
|
||||
|
||||
@@ -167,9 +167,23 @@ export function DocumentDetail() {
|
||||
queryKey: binding ? queryKeys.documents.reviewIndex(binding.issueId, binding.key) : ["documents", "review-index", "none"],
|
||||
queryFn: () => documentReviewsApi.reviewIndex(binding!.issueId, binding!.key, { status: "all", includeComments: true }),
|
||||
enabled: !!binding,
|
||||
retry: (count, error) => !(error instanceof ApiError && [403, 404].includes(error.status)) && count < 2,
|
||||
});
|
||||
const reviewIndex = reviewIndexQuery.data;
|
||||
|
||||
// The review-index endpoint is gated by the parent issue's authorization
|
||||
// boundary, but the document itself is listable via the company-scoped library.
|
||||
// A board user who can browse the doc but not its issue gets a 403 here; surface
|
||||
// that explicitly instead of falling through to a misleading "no feedback" state.
|
||||
const reviewAccessError = useMemo(() => {
|
||||
const error = reviewIndexQuery.error;
|
||||
if (!(error instanceof ApiError) || error.status !== 403) return null;
|
||||
const issueIdentifier = doc?.backlinks.find((b) => b.identifier && b.targetType === "issue")?.identifier;
|
||||
return issueIdentifier
|
||||
? `You don't have access to this document's review thread. Ask the owner of ${issueIdentifier} for access.`
|
||||
: "You don't have access to this document's review thread. Ask the parent issue's owner for access.";
|
||||
}, [reviewIndexQuery.error, doc?.backlinks]);
|
||||
|
||||
const agentMap = useMemo(() => {
|
||||
const map = new Map<string, Pick<Agent, "id" | "name">>();
|
||||
for (const agent of agents ?? []) map.set(agent.id, agent);
|
||||
@@ -638,8 +652,10 @@ export function DocumentDetail() {
|
||||
<DocumentReviewRail
|
||||
reviewIndex={reviewIndex}
|
||||
loading={reviewIndexQuery.isLoading}
|
||||
accessError={reviewAccessError}
|
||||
canReview={canReview}
|
||||
canFinishReview={canEdit}
|
||||
canFinishReview={canEdit && !reviewAccessError}
|
||||
doneReviewingDisabledReason={reviewAccessError}
|
||||
latestRevisionId={doc.latestRevisionId}
|
||||
authorMaps={authorMaps}
|
||||
onReplyThread={replyThread}
|
||||
@@ -677,8 +693,10 @@ export function DocumentDetail() {
|
||||
onOpenChange={setRailOpen}
|
||||
reviewIndex={reviewIndex}
|
||||
loading={reviewIndexQuery.isLoading}
|
||||
accessError={reviewAccessError}
|
||||
canReview={canReview}
|
||||
canFinishReview={canEdit}
|
||||
canFinishReview={canEdit && !reviewAccessError}
|
||||
doneReviewingDisabledReason={reviewAccessError}
|
||||
latestRevisionId={doc.latestRevisionId}
|
||||
authorMaps={authorMaps}
|
||||
onReplyThread={replyThread}
|
||||
@@ -769,6 +787,7 @@ export function DocumentDetail() {
|
||||
open={handoffOpen}
|
||||
onOpenChange={setHandoffOpen}
|
||||
counts={reviewIndex?.counts}
|
||||
accessError={reviewAccessError}
|
||||
issueIdentifier={doc.backlinks.find((b) => b.identifier && b.targetType === "issue")?.identifier ?? null}
|
||||
ownerName={ownerAgent?.name ?? null}
|
||||
canWakeOwner={!!doc.ownerAgentId && !!doc.latestRevisionId}
|
||||
|
||||
Reference in new issue
Block a user