Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 77 additions & 10 deletions e2e/ai-reply.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,31 @@ type MockScriptStep =
| { kind: 'cancelled' }
| { kind: 'pause' }; // hold open until cancel

async function setupWithMock(
// Installs a mock that plays a DIFFERENT script for each successive spawn. The
// Nth spawn (0-indexed) plays scripts[N]; once past the end, the last script
// repeats. This lets a test drive an error-then-success retry flow: the first
// spawn fails, the retry (a second spawn) succeeds.
async function setupWithMockScripts(
page: Page,
script: MockScriptStep[],
scripts: MockScriptStep[][],
sessionOverrides: Record<string, unknown> = {},
): Promise<void> {
await page.addInitScript(
({ steps, overrides }: { steps: MockScriptStep[]; overrides: Record<string, unknown> }) => {
({
scriptList,
overrides,
}: {
scriptList: MockScriptStep[][];
overrides: Record<string, unknown>;
}) => {
type Ev =
| { kind: 'delta'; text: string }
| { kind: 'done' }
| { kind: 'error'; message: string }
| { kind: 'cancelled' };

let nextTokenId = 0;
let spawnIndex = 0;
const pending = new Map<string, () => void>(); // token → cancel resolver

(window as unknown as { __quillTestSession: unknown }).__quillTestSession = {
Expand All @@ -44,6 +55,8 @@ async function setupWithMock(
spawn: (args: unknown, onEvent: (e: Ev) => void) => {
// Exposed so tests can assert on what the app would send the backend.
(window as unknown as { __lastSpawnArgs: unknown }).__lastSpawnArgs = args;
const steps = scriptList[Math.min(spawnIndex, scriptList.length - 1)];
spawnIndex++;
const token = `mock-${++nextTokenId}`;
let cancelled = false;
pending.set(token, () => {
Expand Down Expand Up @@ -75,7 +88,7 @@ async function setupWithMock(
},
};
},
{ steps: script, overrides: sessionOverrides },
{ scriptList: scripts, overrides: sessionOverrides },
);

await page.goto('/');
Expand All @@ -85,6 +98,15 @@ async function setupWithMock(
await page.waitForTimeout(100);
}

// The common case: one script replayed for every spawn.
async function setupWithMock(
page: Page,
script: MockScriptStep[],
sessionOverrides: Record<string, unknown> = {},
): Promise<void> {
await setupWithMockScripts(page, [script], sessionOverrides);
}

async function addCommentWithAIReply(page: Page, anchor: string, replyText: string) {
await page.keyboard.type(anchor);
// Select all
Expand Down Expand Up @@ -228,22 +250,67 @@ test('AI reply: a Quill-created binding spawns with allowCreate', async ({ page
expect(prompt).toContain('Here is the full current document:');
});

test('AI reply: pending → error shows Re-link button', async ({ page }) => {
test('AI reply: a session-loss error shows Re-link primary plus a secondary Retry', async ({
page,
}) => {
// "session not found" classifies as kind:'session' → Re-link is the primary
// affordance, and because a session error is still retryable a secondary
// Retry is offered too.
await setupWithMock(page, [
{ kind: 'delta', text: 'partial...' },
{ kind: 'error', message: 'Session no longer available' },
{ kind: 'error', message: 'session not found' },
]);

await addCommentWithAIReply(page, 'hello world', '@claude help');

const aiReply = page.locator('.comment-reply-ai').first();
await expect(aiReply).toBeVisible({ timeout: 2000 });
await expect(aiReply.locator('.comment-reply-error')).toContainText(
'Session no longer available',
{ timeout: 3000 },
);
await expect(aiReply.locator('.comment-reply-error')).toContainText('session not found', {
timeout: 3000,
});
await expect(aiReply.getByRole('button', { name: /Re-link session/i })).toBeVisible();
await expect(aiReply.getByRole('button', { name: /^Retry$/i })).toBeVisible();
await expect(aiReply.locator('.ai-spinner')).toHaveCount(0);
});

test('AI reply: a transient error shows Retry (no Re-link) and retry succeeds in place', async ({
page,
}) => {
// First spawn fails with a transient API error; the failed reply offers a
// primary Retry and no Re-link. Clicking Retry re-issues the identical
// request against the SAME reply entry, which the second script completes.
await setupWithMockScripts(page, [
[
{ kind: 'delta', text: 'partial...' },
{ kind: 'error', message: 'API Error: overloaded' },
],
[{ kind: 'delta', text: 'Second time worked.' }, { kind: 'done' }],
]);

await addCommentWithAIReply(page, 'hello world', '@claude help');

const aiReply = page.locator('.comment-reply-ai').first();
await expect(aiReply).toBeVisible({ timeout: 2000 });
await expect(aiReply.locator('.comment-reply-error')).toContainText('API Error: overloaded', {
timeout: 3000,
});
// Transient → Retry is the primary action; Re-link is demoted to a ghost
// secondary rather than hidden.
const retryBtn = aiReply.getByRole('button', { name: /^Retry$/i });
await expect(retryBtn).toBeVisible();
await expect(retryBtn).toHaveClass(/btn-primary/);
await expect(aiReply.getByRole('button', { name: /Re-link session/i })).toHaveClass(/btn-ghost/);

await retryBtn.click();

// Same reply entry recovers: error clears and the second script's text lands.
await expect(aiReply.locator('.comment-reply-text')).toContainText('Second time worked.', {
timeout: 3000,
});
await expect(aiReply.locator('.comment-reply-error')).toHaveCount(0);
await expect(aiReply.locator('.ai-spinner')).toHaveCount(0);
// Exactly one AI reply — retry reused the entry, it did not append a new one.
await expect(page.locator('.comment-reply-ai')).toHaveCount(1);
});

// Selects the first `count` characters of the current line (from its start),
Expand Down
5 changes: 5 additions & 0 deletions src/App.css
Original file line number Diff line number Diff line change
Expand Up @@ -1596,6 +1596,11 @@ del.track-delete {
white-space: pre-wrap;
word-break: break-word;
}
.comment-reply-error-actions {
display: flex;
flex-wrap: wrap;
gap: 6px;
}

/* ── Footer bindings (Claude session / reference folder) ───────── */
.footer-ai-binding,
Expand Down
10 changes: 8 additions & 2 deletions src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import FindBar from './components/FindBar';
import AppModal from './components/AppModal';
import ReviewModal from './components/ReviewModal';
import UpdateBanner from './components/UpdateBanner';
import { useFileManager } from './hooks/useFileManager';
import { useFileManager, stripTransientReplyState } from './hooks/useFileManager';
import { useDraftAutosave } from './hooks/useDraftAutosave';
import type { DraftSnapshot } from './hooks/useDraftAutosave';
import { useUpdateCheck } from './hooks/useUpdateCheck';
Expand Down Expand Up @@ -148,6 +148,7 @@ export default function App() {
appendAIReplyChunk,
finishAIReply,
failAIReply,
retryAIReply,
} = useComments();
const { suggestions, setSuggestions } = useSuggestions();

Expand All @@ -160,7 +161,10 @@ export default function App() {
(): DraftSnapshot => ({
filePath,
content: getDocMarkdown(),
comments,
// draft.json is a second on-disk persistence path, so it needs the same
// transient-reply strip the sidecar gets — otherwise a crash mid-stream
// recovers a stuck spinner / dead-Retry card the fresh hook can't drive.
comments: stripTransientReplyState(comments),
suggestions,
aiSession,
contextFolder,
Expand Down Expand Up @@ -244,6 +248,7 @@ export default function App() {
appendAIReplyChunk,
finishAIReply,
failAIReply,
retryAIReply,
getDocMarkdown,
getRangeTexts,
applyTrackedEdits,
Expand Down Expand Up @@ -1144,6 +1149,7 @@ export default function App() {
onReply={(id, text) => addReply(id, text, AUTHOR)}
onAIReplyRequest={handleAIReplyRequest}
onCancelAIReply={claudeReply.cancel}
onRetryAIReply={claudeReply.retry}
onOpenSessionPicker={() => setPickerOpen(true)}
onResolve={handleResolveComment}
onUnresolve={handleUnresolveComment}
Expand Down
60 changes: 57 additions & 3 deletions src/components/CommentCard.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { useState, useRef } from 'react';
import type { Comment, Reply } from '../types';
import { timeAgo } from '../utils/format';
import { classifyReplyError } from '../hooks/useClaudeReply';

interface CommentCardProps {
comment: Comment;
Expand All @@ -9,20 +10,73 @@ interface CommentCardProps {
onReply: (commentId: string, text: string) => void;
onAIReplyRequest: (commentId: string, userText: string) => void;
onCancelAIReply: (replyId: string) => void;
onRetryAIReply: (replyId: string) => void;
onOpenSessionPicker: () => void;
onResolve: (commentId: string) => void;
onUnresolve: (commentId: string) => void;
onDelete: (commentId: string) => void;
onClick: (commentId: string) => void;
}

function ReplyErrorActions({
message,
onRetry,
onRelink,
}: {
message: string;
onRetry: () => void;
onRelink: () => void;
}) {
const { retryable, kind } = classifyReplyError(message);
const retryBtn = (
<button className="btn-primary" onClick={onRetry}>
Retry
</button>
);
const relinkBtn = (primary: boolean) => (
<button className={primary ? 'btn-primary' : 'btn-ghost'} onClick={onRelink}>
Re-link session…
</button>
);

// Which affordances to surface, driven by the classifier:
// auth → not retryable; re-linking is the only path forward.
// session → re-link is primary (session is gone), retry as fallback.
// else → transient/unknown; retry is primary, re-link demoted.
let actions: React.ReactNode;
if (kind === 'auth') {
actions = relinkBtn(true);
} else if (kind === 'session') {
actions = (
<>
{relinkBtn(true)}
{retryable && (
<button className="btn-ghost" onClick={onRetry}>
Retry
</button>
)}
</>
);
} else {
actions = (
<>
{retryable && retryBtn}
{relinkBtn(false)}
</>
);
}
return <div className="comment-reply-error-actions">{actions}</div>;
}

function ReplyView({
reply,
onCancel,
onRetry,
onRelink,
}: {
reply: Reply;
onCancel: () => void;
onRetry: () => void;
onRelink: () => void;
}) {
const isAI = reply.authorKind === 'ai';
Expand All @@ -38,9 +92,7 @@ function ReplyView({
{reply.error ? (
<div className="comment-reply-error">
<p>{reply.error}</p>
<button className="btn-ghost" onClick={onRelink}>
Re-link session…
</button>
<ReplyErrorActions message={reply.error} onRetry={onRetry} onRelink={onRelink} />
</div>
) : (
<>
Expand Down Expand Up @@ -69,6 +121,7 @@ export default function CommentCard({
onReply,
onAIReplyRequest,
onCancelAIReply,
onRetryAIReply,
onOpenSessionPicker,
onResolve,
onUnresolve,
Expand Down Expand Up @@ -156,6 +209,7 @@ export default function CommentCard({
key={reply.id}
reply={reply}
onCancel={() => onCancelAIReply(reply.id)}
onRetry={() => onRetryAIReply(reply.id)}
onRelink={onOpenSessionPicker}
/>
))}
Expand Down
3 changes: 3 additions & 0 deletions src/components/CommentLayer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ interface CommentLayerProps {
onReply: (commentId: string, text: string) => void;
onAIReplyRequest: (commentId: string, userText: string) => void;
onCancelAIReply: (replyId: string) => void;
onRetryAIReply: (replyId: string) => void;
onOpenSessionPicker: () => void;
onResolve: (commentId: string) => void;
onUnresolve: (commentId: string) => void;
Expand Down Expand Up @@ -125,6 +126,7 @@ export default function CommentLayer({
onReply,
onAIReplyRequest,
onCancelAIReply,
onRetryAIReply,
onOpenSessionPicker,
onResolve,
onUnresolve,
Expand Down Expand Up @@ -294,6 +296,7 @@ export default function CommentLayer({
onReply={onReply}
onAIReplyRequest={onAIReplyRequest}
onCancelAIReply={onCancelAIReply}
onRetryAIReply={onRetryAIReply}
onOpenSessionPicker={onOpenSessionPicker}
onResolve={onResolve}
onUnresolve={onUnresolve}
Expand Down
Loading
Loading