fix: prevent duplicate review entries from local inserts and id reuse

Author: Dimillian

src/features/threads/hooks/useThreads.ts

@@ -405,27 +405,6 @@ function normalizePlanUpdate(
   };
 }
 
-function formatReviewLabel(target: ReturnType<typeof parseReviewTarget>) {
-  if (target.type === "uncommittedChanges") {
-    return "current changes";
-  }
-  if (target.type === "baseBranch") {
-    return `base branch ${target.branch}`;
-  }
-  if (target.type === "commit") {
-    return target.title
-      ? `commit ${target.sha}: ${target.title}`
-      : `commit ${target.sha}`;
-  }
-  const instructions = target.instructions.trim();
-  if (!instructions) {
-    return "custom review";
-  }
-  return instructions.length > 80
-    ? `${instructions.slice(0, 80)}…`
-    : instructions;
-}
-
 export function useThreads({
   activeWorkspace,
   onWorkspaceConnected,
@@ -1767,16 +1746,6 @@ export function useThreads({
       const target = parseReviewTarget(text);
       markProcessing(threadId, true);
       dispatch({ type: "markReviewing", threadId, isReviewing: true });
-      dispatch({
-        type: "upsertItem",
-        threadId,
-        item: {
-          id: `review-start-${threadId}-${Date.now()}`,
-          kind: "review",
-          state: "started",
-          text: formatReviewLabel(target),
-        },
-      });
       safeMessageActivity();
       onDebug?.({
         id: `${Date.now()}-client-review-start`,

src/features/threads/hooks/useThreadsReducer.test.ts

@@ -126,6 +126,63 @@ describe("threadReducer", () => {
     expect(items[0]?.id).toBe("remote-review-1");
   });
 
+  it("appends review items when ids repeat", () => {
+    const firstReview: ConversationItem = {
+      id: "review-mode",
+      kind: "review",
+      state: "started",
+      text: "Reviewing changes",
+    };
+    const next = threadReducer(
+      {
+        ...initialState,
+        itemsByThread: { "thread-1": [firstReview] },
+      },
+      {
+        type: "upsertItem",
+        threadId: "thread-1",
+        item: {
+          id: "review-mode",
+          kind: "review",
+          state: "completed",
+          text: "Reviewing changes",
+        },
+      },
+    );
+    const items = next.itemsByThread["thread-1"] ?? [];
+    expect(items).toHaveLength(2);
+    expect(items[0]?.id).toBe("review-mode");
+    expect(items[1]?.id).toBe("review-mode-1");
+  });
+
+  it("dedupes review items with identical content", () => {
+    const firstReview: ConversationItem = {
+      id: "review-mode",
+      kind: "review",
+      state: "completed",
+      text: "Reviewing changes",
+    };
+    const next = threadReducer(
+      {
+        ...initialState,
+        itemsByThread: { "thread-1": [firstReview] },
+      },
+      {
+        type: "upsertItem",
+        threadId: "thread-1",
+        item: {
+          id: "review-mode-duplicate",
+          kind: "review",
+          state: "completed",
+          text: "Reviewing changes",
+        },
+      },
+    );
+    const items = next.itemsByThread["thread-1"] ?? [];
+    expect(items).toHaveLength(1);
+    expect(items[0]?.id).toBe("review-mode");
+  });
+
   it("appends reasoning summary and content when missing", () => {
     const withSummary = threadReducer(initialState, {
       type: "appendReasoningSummary",

src/features/threads/hooks/useThreadsReducer.ts

@@ -263,6 +263,36 @@ function dropLatestLocalReviewStart(list: ConversationItem[]) {
   return list;
 }
 
+function findMatchingReview(
+  list: ConversationItem[],
+  target: Extract<ConversationItem, { kind: "review" }>,
+) {
+  const normalizedText = target.text.trim();
+  return list.find(
+    (item) =>
+      item.kind === "review" &&
+      item.state === target.state &&
+      item.text.trim() === normalizedText,
+  );
+}
+
+function ensureUniqueReviewId(list: ConversationItem[], item: ConversationItem) {
+  if (item.kind !== "review") {
+    return item;
+  }
+  if (!list.some((entry) => entry.id === item.id)) {
+    return item;
+  }
+  const existingIds = new Set(list.map((entry) => entry.id));
+  let suffix = 1;
+  let candidate = `${item.id}-${suffix}`;
+  while (existingIds.has(candidate)) {
+    suffix += 1;
+    candidate = `${item.id}-${suffix}`;
+  }
+  return { ...item, id: candidate };
+}
+
 export function threadReducer(state: ThreadState, action: ThreadAction): ThreadState {
   switch (action.type) {
     case "setActiveThreadId":
@@ -616,11 +646,18 @@ export function threadReducer(state: ThreadState, action: ThreadAction): ThreadS
       ) {
         list = dropLatestLocalReviewStart(list);
       }
+      if (item.kind === "review") {
+        const existing = findMatchingReview(list, item);
+        if (existing && existing.id !== item.id) {
+          return state;
+        }
+      }
+      const nextItem = ensureUniqueReviewId(list, item);
       return {
         ...state,
         itemsByThread: {
           ...state.itemsByThread,
-          [action.threadId]: prepareThreadItems(upsertItem(list, item)),
+          [action.threadId]: prepareThreadItems(upsertItem(list, nextItem)),
         },
       };
     }