Skip to content

Commit b25ea62

Browse files
committed
🤖 fix: improve pending reviews UX based on feedback
- Use draft construct for error recovery (no duplication) - Hide attached reviews from banner to prevent double-send - Move 'Send to chat' action left, away from delete - Make 'Clear' button smaller (just text link) - Grey chat icon when no pending reviews - Reduce padding/margins in ReviewBlock for compact display - Notify parent when attached reviews change _Generated with mux_
1 parent d508eb5 commit b25ea62

File tree

5 files changed

+111
-80
lines changed

5 files changed

+111
-80
lines changed

src/browser/components/AIView.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ const AIViewInner: React.FC<AIViewProps> = ({
111111

112112
// Pending reviews state
113113
const pendingReviews = usePendingReviews(workspaceId);
114+
const [attachedReviewIds, setAttachedReviewIds] = useState<string[]>([]);
114115
const { options } = useProviderOptions();
115116
const use1M = options.anthropic?.use1MContext ?? false;
116117
// Get pending model for auto-compaction settings (threshold is per-model)
@@ -616,7 +617,11 @@ const AIViewInner: React.FC<AIViewProps> = ({
616617
onCompactClick={handleCompactClick}
617618
/>
618619
)}
619-
<PendingReviewsBanner workspaceId={workspaceId} chatInputAPI={chatInputAPI} />
620+
<PendingReviewsBanner
621+
workspaceId={workspaceId}
622+
chatInputAPI={chatInputAPI}
623+
attachedReviewIds={attachedReviewIds}
624+
/>
620625
<ChatInput
621626
variant="workspace"
622627
workspaceId={workspaceId}
@@ -634,6 +639,7 @@ const AIViewInner: React.FC<AIViewProps> = ({
634639
autoCompactionCheck={autoCompactionResult}
635640
getReview={pendingReviews.getReview}
636641
onReviewsSent={(ids) => ids.forEach((id) => pendingReviews.checkReview(id))}
642+
onAttachedReviewsChange={setAttachedReviewIds}
637643
/>
638644
</div>
639645

src/browser/components/ChatInput/index.tsx

Lines changed: 28 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -154,24 +154,26 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
154154
const inputRef = useRef<HTMLTextAreaElement>(null);
155155
const modelSelectorRef = useRef<ModelSelectorRef>(null);
156156

157-
// Draft state combines text input and image attachments
158-
// Use these helpers to avoid accidentally losing images when modifying text
157+
// Draft state combines text input, image attachments, and attached reviews
158+
// Use these helpers to avoid accidentally losing content when modifying text
159159
interface DraftState {
160160
text: string;
161161
images: ImageAttachment[];
162+
reviewIds: string[];
162163
}
163164
const getDraft = useCallback(
164-
(): DraftState => ({ text: input, images: imageAttachments }),
165-
[input, imageAttachments]
165+
(): DraftState => ({ text: input, images: imageAttachments, reviewIds: attachedReviewIds }),
166+
[input, imageAttachments, attachedReviewIds]
166167
);
167168
const setDraft = useCallback(
168169
(draft: DraftState) => {
169170
setInput(draft.text);
170171
setImageAttachments(draft.images);
172+
setAttachedReviewIds(draft.reviewIds);
171173
},
172174
[setInput]
173175
);
174-
const preEditDraftRef = useRef<DraftState>({ text: "", images: [] });
176+
const preEditDraftRef = useRef<DraftState>({ text: "", images: [], reviewIds: [] });
175177
const { open } = useSettings();
176178
const { selectedWorkspace } = useWorkspaceContext();
177179
const [mode, setMode] = useMode();
@@ -356,6 +358,13 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
356358
// Get currently attached reviews
357359
const getAttachedReviews = useCallback(() => attachedReviewIds, [attachedReviewIds]);
358360

361+
// Notify parent when attached reviews change (workspace variant only)
362+
useEffect(() => {
363+
if (variant === "workspace" && props.onAttachedReviewsChange) {
364+
props.onAttachedReviewsChange(attachedReviewIds);
365+
}
366+
}, [variant, attachedReviewIds, props]);
367+
359368
// Provide API to parent via callback
360369
useEffect(() => {
361370
if (props.onReady) {
@@ -411,7 +420,7 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
411420
useEffect(() => {
412421
if (editingMessage) {
413422
preEditDraftRef.current = getDraft();
414-
setDraft({ text: editingMessage.content, images: [] });
423+
setDraft({ text: editingMessage.content, images: [], reviewIds: [] });
415424
// Auto-resize textarea and focus
416425
setTimeout(() => {
417426
if (inputRef.current) {
@@ -968,8 +977,8 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
968977
}
969978
setIsSending(true);
970979

971-
// Save current state for restoration on error
972-
const previousImageAttachments = [...imageAttachments];
980+
// Save current draft state for restoration on error
981+
const preSendDraft = getDraft();
973982

974983
// Auto-compaction check (workspace variant only)
975984
// Check if we should auto-compact before sending this message
@@ -1002,8 +1011,7 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
10021011

10031012
if (!result.success) {
10041013
// Restore on error
1005-
setInput(messageText);
1006-
setImageAttachments(previousImageAttachments);
1014+
setDraft(preSendDraft);
10071015
setToast({
10081016
id: Date.now().toString(),
10091017
type: "error",
@@ -1020,8 +1028,7 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
10201028
}
10211029
} catch (error) {
10221030
// Restore on unexpected error
1023-
setInput(messageText);
1024-
setImageAttachments(previousImageAttachments);
1031+
setDraft(preSendDraft);
10251032
setToast({
10261033
id: Date.now().toString(),
10271034
type: "error",
@@ -1036,9 +1043,6 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
10361043
return; // Skip normal send
10371044
}
10381045

1039-
// Store attached review IDs before try block so catch block can access them
1040-
const sentReviewIds = [...attachedReviewIds];
1041-
10421046
try {
10431047
// Prepare image parts if any
10441048
const imageParts = imageAttachments.map((img, index) => {
@@ -1135,10 +1139,8 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
11351139
console.error("Failed to send message:", result.error);
11361140
// Show error using enhanced toast
11371141
setToast(createErrorToast(result.error));
1138-
// Restore input, images, and attached reviews on error so user can try again
1139-
setInput(messageText);
1140-
setImageAttachments(previousImageAttachments);
1141-
setAttachedReviewIds(sentReviewIds);
1142+
// Restore draft on error so user can try again
1143+
setDraft(preSendDraft);
11421144
} else {
11431145
// Track telemetry for successful message send
11441146
telemetry.messageSent(
@@ -1151,8 +1153,8 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
11511153
);
11521154

11531155
// Mark attached reviews as completed
1154-
if (sentReviewIds.length > 0) {
1155-
props.onReviewsSent?.(sentReviewIds);
1156+
if (preSendDraft.reviewIds.length > 0) {
1157+
props.onReviewsSent?.(preSendDraft.reviewIds);
11561158
}
11571159

11581160
// Exit editing mode if we were editing
@@ -1170,9 +1172,8 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
11701172
raw: error instanceof Error ? error.message : "Failed to send message",
11711173
})
11721174
);
1173-
setInput(messageText);
1174-
setImageAttachments(previousImageAttachments);
1175-
setAttachedReviewIds(sentReviewIds);
1175+
// Restore draft on error
1176+
setDraft(preSendDraft);
11761177
} finally {
11771178
setIsSending(false);
11781179
}
@@ -1354,7 +1355,7 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
13541355

13551356
{/* Attached reviews preview - show styled blocks with remove buttons */}
13561357
{variant === "workspace" && attachedReviewIds.length > 0 && props.getReview && (
1357-
<div className="border-border max-h-40 space-y-1 overflow-y-auto border-b px-2 py-1.5">
1358+
<div className="border-border max-h-36 space-y-0.5 overflow-y-auto border-b px-1.5 py-1">
13581359
{attachedReviewIds.map((reviewId) => {
13591360
const review = props.getReview!(reviewId);
13601361
if (!review) return null;
@@ -1364,10 +1365,10 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
13641365
<button
13651366
type="button"
13661367
onClick={() => detachReview(reviewId)}
1367-
className="bg-dark/80 text-muted hover:text-error absolute top-3 right-3 rounded-full p-0.5 opacity-0 transition-opacity group-hover:opacity-100"
1368+
className="bg-dark/80 text-muted hover:text-error absolute top-1.5 right-1.5 rounded-full p-0.5 opacity-0 transition-opacity group-hover:opacity-100"
13681369
title="Remove from message"
13691370
>
1370-
<X className="size-3.5" />
1371+
<X className="size-3" />
13711372
</button>
13721373
</div>
13731374
);

src/browser/components/ChatInput/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ export interface ChatInputWorkspaceVariant {
3838
autoCompactionCheck?: AutoCompactionCheckResult; // Computed in parent (AIView) to avoid duplicate calculation
3939
/** Called after reviews are sent in a message - allows parent to mark them as checked */
4040
onReviewsSent?: (reviewIds: string[]) => void;
41+
/** Called when attached reviews change (for syncing with banner) */
42+
onAttachedReviewsChange?: (reviewIds: string[]) => void;
4143
/** Get a pending review by ID (for resolving attached review IDs to data) */
4244
getReview?: (id: string) => PendingReview | undefined;
4345
}

src/browser/components/PendingReviewsBanner.tsx

Lines changed: 68 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import {
2424
} from "lucide-react";
2525
import { cn } from "@/common/lib/utils";
2626
import { Button } from "./ui/button";
27-
import { Tooltip, TooltipWrapper } from "./Tooltip";
27+
import { Tooltip, TooltipTrigger, TooltipContent } from "./ui/tooltip";
2828
import type { PendingReview } from "@/common/types/review";
2929
import { usePendingReviews } from "@/browser/hooks/usePendingReviews";
3030
import type { ChatInputAPI } from "./ChatInput";
@@ -176,17 +176,34 @@ const ReviewItem: React.FC<ReviewItemProps> = ({
176176
</button>
177177

178178
{/* Check/Uncheck button */}
179-
<TooltipWrapper inline>
180-
<Button
181-
variant="ghost"
182-
size="icon"
183-
className={cn("size-5 shrink-0 [&_svg]:size-3", isChecked && "text-success")}
184-
onClick={isChecked ? onUncheck : onCheck}
185-
>
186-
{isChecked ? <Undo2 /> : <Check />}
187-
</Button>
188-
<Tooltip align="center">{isChecked ? "Mark as pending" : "Mark as done"}</Tooltip>
189-
</TooltipWrapper>
179+
<Tooltip>
180+
<TooltipTrigger asChild>
181+
<Button
182+
variant="ghost"
183+
size="icon"
184+
className={cn("size-5 shrink-0 [&_svg]:size-3", isChecked && "text-success")}
185+
onClick={isChecked ? onUncheck : onCheck}
186+
>
187+
{isChecked ? <Undo2 /> : <Check />}
188+
</Button>
189+
</TooltipTrigger>
190+
<TooltipContent>{isChecked ? "Mark as pending" : "Mark as done"}</TooltipContent>
191+
</Tooltip>
192+
193+
{/* Send to chat - near safe actions, away from delete */}
194+
<Tooltip>
195+
<TooltipTrigger asChild>
196+
<Button
197+
variant="ghost"
198+
size="icon"
199+
className="size-5 shrink-0 opacity-0 transition-opacity group-hover:opacity-100 [&_svg]:size-3"
200+
onClick={onSendToChat}
201+
>
202+
<Send />
203+
</Button>
204+
</TooltipTrigger>
205+
<TooltipContent>Send to chat</TooltipContent>
206+
</Tooltip>
190207

191208
{/* File path and age */}
192209
<button
@@ -200,32 +217,20 @@ const ReviewItem: React.FC<ReviewItemProps> = ({
200217
<span className="text-muted shrink-0 text-[10px]">{age}</span>
201218
</button>
202219

203-
{/* Actions */}
204-
<div className="flex shrink-0 items-center gap-0.5 opacity-0 transition-opacity group-hover:opacity-100">
205-
<TooltipWrapper inline>
206-
<Button
207-
variant="ghost"
208-
size="icon"
209-
className="size-5 [&_svg]:size-3"
210-
onClick={onSendToChat}
211-
>
212-
<Send />
213-
</Button>
214-
<Tooltip align="center">Send to chat</Tooltip>
215-
</TooltipWrapper>
216-
217-
<TooltipWrapper inline>
220+
{/* Delete action - separate from safe actions */}
221+
<Tooltip>
222+
<TooltipTrigger asChild>
218223
<Button
219224
variant="ghost"
220225
size="icon"
221-
className="text-error size-5 [&_svg]:size-3"
226+
className="text-error size-5 shrink-0 opacity-0 transition-opacity group-hover:opacity-100 [&_svg]:size-3"
222227
onClick={onRemove}
223228
>
224229
<Trash2 />
225230
</Button>
226-
<Tooltip align="center">Remove</Tooltip>
227-
</TooltipWrapper>
228-
</div>
231+
</TooltipTrigger>
232+
<TooltipContent>Remove</TooltipContent>
233+
</Tooltip>
229234
</div>
230235

231236
{/* Expanded content */}
@@ -300,24 +305,33 @@ const ReviewItem: React.FC<ReviewItemProps> = ({
300305
interface PendingReviewsBannerInnerProps {
301306
workspaceId: string;
302307
chatInputAPI: React.RefObject<ChatInputAPI | null>;
308+
attachedReviewIds: string[];
303309
}
304310

305311
const PendingReviewsBannerInner: React.FC<PendingReviewsBannerInnerProps> = ({
306312
workspaceId,
307313
chatInputAPI,
314+
attachedReviewIds,
308315
}) => {
309316
const pendingReviews = usePendingReviews(workspaceId);
310317
const [isExpanded, setIsExpanded] = useState(false);
311318
const [showAllCompleted, setShowAllCompleted] = useState(false);
312319

313320
const INITIAL_COMPLETED_COUNT = 3;
314321

315-
// Separate pending and completed reviews
322+
// Set of attached review IDs for quick lookup
323+
const attachedSet = useMemo(() => new Set(attachedReviewIds), [attachedReviewIds]);
324+
325+
// Separate pending and completed reviews, excluding those already attached to chat
316326
const { pendingList, completedList } = useMemo(() => {
317-
const pending = pendingReviews.reviews.filter((r) => r.status === "pending");
318-
const completed = pendingReviews.reviews.filter((r) => r.status === "checked");
327+
const pending = pendingReviews.reviews.filter(
328+
(r) => r.status === "pending" && !attachedSet.has(r.id)
329+
);
330+
const completed = pendingReviews.reviews.filter(
331+
(r) => r.status === "checked" && !attachedSet.has(r.id)
332+
);
319333
return { pendingList: pending, completedList: completed };
320-
}, [pendingReviews.reviews]);
334+
}, [pendingReviews.reviews, attachedSet]);
321335

322336
// Completed reviews to display (limited unless expanded)
323337
const displayedCompleted = useMemo(() => {
@@ -358,7 +372,12 @@ const PendingReviewsBannerInner: React.FC<PendingReviewsBannerInnerProps> = ({
358372
onClick={handleToggle}
359373
className="hover:bg-hover flex w-full items-center gap-2 px-3 py-1.5 text-xs transition-colors"
360374
>
361-
<MessageSquare className="size-3.5 text-[var(--color-review-accent)]" />
375+
<MessageSquare
376+
className={cn(
377+
"size-3.5",
378+
pendingReviews.pendingCount > 0 ? "text-[var(--color-review-accent)]" : "text-muted"
379+
)}
380+
/>
362381
<span className="text-secondary">
363382
{pendingReviews.pendingCount > 0 ? (
364383
<>
@@ -415,15 +434,13 @@ const PendingReviewsBannerInner: React.FC<PendingReviewsBannerInnerProps> = ({
415434
Completed ({completedList.length})
416435
</div>
417436
{completedList.length > 0 && (
418-
<Button
419-
variant="ghost"
420-
size="sm"
421-
className="text-error h-5 px-2 text-xs"
437+
<button
438+
type="button"
422439
onClick={pendingReviews.clearChecked}
440+
className="text-muted hover:text-error text-[10px] transition-colors"
423441
>
424-
<Trash2 className="mr-1 size-3" />
425-
Clear all
426-
</Button>
442+
Clear
443+
</button>
427444
)}
428445
</div>
429446
{displayedCompleted.map((review) => (
@@ -467,6 +484,8 @@ const PendingReviewsBannerInner: React.FC<PendingReviewsBannerInnerProps> = ({
467484
interface PendingReviewsBannerProps {
468485
workspaceId: string;
469486
chatInputAPI: React.RefObject<ChatInputAPI | null>;
487+
/** Review IDs currently attached to chat input (hidden from banner to prevent double-send) */
488+
attachedReviewIds: string[];
470489
}
471490

472491
/**
@@ -476,12 +495,17 @@ interface PendingReviewsBannerProps {
476495
export const PendingReviewsBanner: React.FC<PendingReviewsBannerProps> = ({
477496
workspaceId,
478497
chatInputAPI,
498+
attachedReviewIds,
479499
}) => {
480500
const pendingReviews = usePendingReviews(workspaceId);
481501

482502
return (
483503
<BannerErrorBoundary onClear={pendingReviews.clearAll}>
484-
<PendingReviewsBannerInner workspaceId={workspaceId} chatInputAPI={chatInputAPI} />
504+
<PendingReviewsBannerInner
505+
workspaceId={workspaceId}
506+
chatInputAPI={chatInputAPI}
507+
attachedReviewIds={attachedReviewIds}
508+
/>
485509
</BannerErrorBoundary>
486510
);
487511
};

0 commit comments

Comments
 (0)