From b45d1c5d351ec1b4299c9f9eec252f049d7982e6 Mon Sep 17 00:00:00 2001 From: Sam Powers <35611153+sam-powers@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:32:22 -0400 Subject: [PATCH] feat(comments): retry failed @claude replies in place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed @claude reply previously offered only a heavyweight "Re-link session…" button, even when the right fix was a simple retry. Classify the failure and surface the right affordance: - classifyReplyError (in useClaudeReply.ts) maps an error message to { retryable, kind } with ordered rules (session → auth → transient → unknown, unknown defaulting to retryable). 401/login markers are word-boundary-anchored so a port number or hostname can't force a false auth verdict. - retryAIReply (useComments.ts) resets the SAME reply entry to a fresh pending state (never appends); useClaudeReply.retry re-issues the identical spawn, reusing the replyId. - A per-replyId generation guard drops late/stale events and orphan-cancels a superseded spawn, so a retry that supersedes a slow original isn't clobbered by the original's late terminal event. Each spawn cancels by its OWN captured token, not a replyId re-lookup — otherwise a stale original's late event would cancel the live retry. - CommentCard renders Retry/Re-link by classifier kind: transient or unknown → Retry primary; session → Re-link primary, Retry secondary; auth → Re-link only. - stripTransientReplyState drops errored/pending AI replies before serialization, at both the sidecar and draft choke points, so transient UI state never reaches disk. Coverage: vitest (classifyReplyError incl. word-boundary negative controls, retryAIReply, stripTransientReplyState, and a generation-guard race test that reproduces the cross-generation orphan-cancel hazard). The full racing-spawn e2e needs the packaged runtime and is verified manually — agent-browser is not installed. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01ESi6dnK3Wc1jYRpZN5qfZA --- e2e/ai-reply.spec.ts | 87 ++++++++- src/App.css | 5 + src/App.tsx | 10 +- src/components/CommentCard.tsx | 60 +++++- src/components/CommentLayer.tsx | 3 + src/hooks/useClaudeReply.ts | 202 +++++++++++++++++++-- src/hooks/useComments.ts | 20 ++ src/hooks/useFileManager.ts | 23 ++- src/test/hooks/claudeReplyHelpers.test.ts | 91 +++++++++- src/test/hooks/useClaudeReplyRetry.test.ts | 186 +++++++++++++++++++ src/test/hooks/useComments.test.ts | 53 ++++++ src/test/hooks/useFileManager.test.ts | 113 +++++++++++- 12 files changed, 818 insertions(+), 35 deletions(-) create mode 100644 src/test/hooks/useClaudeReplyRetry.test.ts diff --git a/e2e/ai-reply.spec.ts b/e2e/ai-reply.spec.ts index 21e440c..57ceb0a 100644 --- a/e2e/ai-reply.spec.ts +++ b/e2e/ai-reply.spec.ts @@ -16,13 +16,23 @@ 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 = {}, ): Promise { await page.addInitScript( - ({ steps, overrides }: { steps: MockScriptStep[]; overrides: Record }) => { + ({ + scriptList, + overrides, + }: { + scriptList: MockScriptStep[][]; + overrides: Record; + }) => { type Ev = | { kind: 'delta'; text: string } | { kind: 'done' } @@ -30,6 +40,7 @@ async function setupWithMock( | { kind: 'cancelled' }; let nextTokenId = 0; + let spawnIndex = 0; const pending = new Map void>(); // token → cancel resolver (window as unknown as { __quillTestSession: unknown }).__quillTestSession = { @@ -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, () => { @@ -75,7 +88,7 @@ async function setupWithMock( }, }; }, - { steps: script, overrides: sessionOverrides }, + { scriptList: scripts, overrides: sessionOverrides }, ); await page.goto('/'); @@ -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 = {}, +): Promise { + await setupWithMockScripts(page, [script], sessionOverrides); +} + async function addCommentWithAIReply(page: Page, anchor: string, replyText: string) { await page.keyboard.type(anchor); // Select all @@ -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), diff --git a/src/App.css b/src/App.css index 06dc582..61572c3 100644 --- a/src/App.css +++ b/src/App.css @@ -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, diff --git a/src/App.tsx b/src/App.tsx index 5594174..c185624 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -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'; @@ -148,6 +148,7 @@ export default function App() { appendAIReplyChunk, finishAIReply, failAIReply, + retryAIReply, } = useComments(); const { suggestions, setSuggestions } = useSuggestions(); @@ -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, @@ -244,6 +248,7 @@ export default function App() { appendAIReplyChunk, finishAIReply, failAIReply, + retryAIReply, getDocMarkdown, getRangeTexts, applyTrackedEdits, @@ -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} diff --git a/src/components/CommentCard.tsx b/src/components/CommentCard.tsx index 635a3be..5fac9f8 100644 --- a/src/components/CommentCard.tsx +++ b/src/components/CommentCard.tsx @@ -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; @@ -9,6 +10,7 @@ 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; @@ -16,13 +18,65 @@ interface CommentCardProps { onClick: (commentId: string) => void; } +function ReplyErrorActions({ + message, + onRetry, + onRelink, +}: { + message: string; + onRetry: () => void; + onRelink: () => void; +}) { + const { retryable, kind } = classifyReplyError(message); + const retryBtn = ( + + ); + const relinkBtn = (primary: boolean) => ( + + ); + + // 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 && ( + + )} + + ); + } else { + actions = ( + <> + {retryable && retryBtn} + {relinkBtn(false)} + + ); + } + return
{actions}
; +} + function ReplyView({ reply, onCancel, + onRetry, onRelink, }: { reply: Reply; onCancel: () => void; + onRetry: () => void; onRelink: () => void; }) { const isAI = reply.authorKind === 'ai'; @@ -38,9 +92,7 @@ function ReplyView({ {reply.error ? (

{reply.error}

- +
) : ( <> @@ -69,6 +121,7 @@ export default function CommentCard({ onReply, onAIReplyRequest, onCancelAIReply, + onRetryAIReply, onOpenSessionPicker, onResolve, onUnresolve, @@ -156,6 +209,7 @@ export default function CommentCard({ key={reply.id} reply={reply} onCancel={() => onCancelAIReply(reply.id)} + onRetry={() => onRetryAIReply(reply.id)} onRelink={onOpenSessionPicker} /> ))} diff --git a/src/components/CommentLayer.tsx b/src/components/CommentLayer.tsx index 56dcb3a..8ad3f9f 100644 --- a/src/components/CommentLayer.tsx +++ b/src/components/CommentLayer.tsx @@ -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; @@ -125,6 +126,7 @@ export default function CommentLayer({ onReply, onAIReplyRequest, onCancelAIReply, + onRetryAIReply, onOpenSessionPicker, onResolve, onUnresolve, @@ -294,6 +296,7 @@ export default function CommentLayer({ onReply={onReply} onAIReplyRequest={onAIReplyRequest} onCancelAIReply={onCancelAIReply} + onRetryAIReply={onRetryAIReply} onOpenSessionPicker={onOpenSessionPicker} onResolve={onResolve} onUnresolve={onUnresolve} diff --git a/src/hooks/useClaudeReply.ts b/src/hooks/useClaudeReply.ts index 731be92..4f3dd96 100644 --- a/src/hooks/useClaudeReply.ts +++ b/src/hooks/useClaudeReply.ts @@ -19,6 +19,7 @@ interface UseClaudeReplyOptions { appendAIReplyChunk: (commentId: string, replyId: string, chunk: string) => void; finishAIReply: (commentId: string, replyId: string) => void; failAIReply: (commentId: string, replyId: string, message: string) => void; + retryAIReply: (commentId: string, replyId: string) => void; getDocMarkdown: () => string; /** Read the current document text for a comment's range + paragraph. */ getRangeTexts: (comment: Comment) => RangeTexts; @@ -51,6 +52,75 @@ export function detectScope(userText: string): EditScope { return 'highlight'; } +/** How a failed @claude reply should be recovered from, derived from its message. */ +export type ReplyErrorKind = 'transient' | 'session' | 'auth' | 'unknown'; + +export interface ReplyErrorClass { + retryable: boolean; + kind: ReplyErrorKind; +} + +// Ordered specific-before-broad: a message like "No conversation found (timeout)" +// must classify as `session` (re-link is the fix), not `transient`. Each rule's +// `test` runs against a lowercased copy of the message, so patterns are lowercase. +const REPLY_ERROR_RULES: { + kind: ReplyErrorKind; + retryable: boolean; + test: (m: string) => boolean; +}[] = [ + { + kind: 'session', + retryable: true, // retryable, but the UI leads with Re-link + test: (m) => + m.includes('no conversation found') || + m.includes('session id') || + m.includes('session not found'), + }, + { + // Deliberately match the specific longer tokens, never a bare "auth" — + // "author"/"authority" and similar appear in unrelated infra messages. + // `401` and `login` are word-boundary-anchored so a port/line number like + // "24010" or a hostname like "mylogin.example.com" can't force a + // non-retryable auth verdict onto an otherwise retryable failure. + kind: 'auth', + retryable: false, + test: (m) => + m.includes('authentication') || + m.includes('unauthorized') || + /\b401\b/.test(m) || + /\blogin\b/.test(m) || + m.includes('api key') || + m.includes('credentials'), + }, + { + kind: 'transient', + retryable: true, + test: (m) => + m.includes('api error') || + /\b(429|500|502|503|529)\b/.test(m) || + m.includes('overloaded') || + m.includes('timeout') || + m.includes('network') || + m.includes('rate limit') || + m.includes('econn') || + m.includes('thinking.type'), + }, +]; + +/** + * Classify a failed @claude reply's error message to drive recovery UI. + * Matching is case-insensitive and ordered (specific kinds win over broad ones). + * Unmatched or empty input is `unknown` — deliberately retryable, since a wrong + * Retry only costs one re-run while a wrong Re-link misdirects the user. + */ +export function classifyReplyError(message: string): ReplyErrorClass { + const m = (message ?? '').toLowerCase(); + for (const rule of REPLY_ERROR_RULES) { + if (rule.test(m)) return { retryable: rule.retryable, kind: rule.kind }; + } + return { retryable: true, kind: 'unknown' }; +} + /** * Split a raw Claude reply into the user-visible prose and the (optional) * quill-edits JSON. `visible` is everything before the fence (trimmed of the @@ -70,6 +140,15 @@ export function splitVisible(raw: string): { visible: string; block: string | nu interface UseClaudeReplyReturn { ask: (comment: Comment, userText: string, binding: AISessionBinding) => Promise; cancel: (replyId: string) => Promise; + /** Re-issue the identical request for a failed reply, reusing its replyId. */ + retry: (replyId: string) => Promise; +} + +/** The inputs a reply was asked with, stashed so a retry can re-issue them. */ +interface ReplyInputs { + comment: Comment; + userText: string; + binding: AISessionBinding; } interface CompactionInfo { @@ -224,12 +303,54 @@ declare global { } } +// Low-level dispatch to cancel a spawned reply by its token: routes to the e2e +// mock when present, otherwise the Tauri command. Rejects (rather than throwing +// synchronously) so every caller can settle it with its own error posture — +// swallow for orphan cleanup, log for a user-initiated cancel. +async function sendCancel(token: string): Promise { + const mock = typeof window !== 'undefined' ? window.__quillMock : undefined; + if (mock) { + mock.cancel?.(token); + return; + } + await invoke('cancel_claude_resume', { cancelToken: token }); +} + export function useClaudeReply(opts: UseClaudeReplyOptions): UseClaudeReplyReturn { const tokensRef = useRef>(new Map()); + // Transient, in-memory only — NEVER persisted to the sidecar. Keyed by + // replyId so a retry can re-issue the exact same request. + const inputsRef = useRef>(new Map()); + // Per-replyId generation counter. A retry reuses the same replyId, so a late + // event from the superseded original must not clobber the retried reply; each + // spawn captures its generation and drops events once it's no longer current. + const genRef = useRef>(new Map()); + // Guards against a double-fire retry (e.g. an impatient double-click) while a + // retry for the same replyId is already being launched. + const retryingRef = useRef>(new Set()); + + // Best-effort cancel of a still-tracked spawn, e.g. when a stale generation + // terminates or a retry supersedes a slow original. Never throws into a + // stream handler. + const orphanCancel = useCallback((replyId: string) => { + const token = tokensRef.current.get(replyId); + if (!token) return; + tokensRef.current.delete(replyId); + void sendCancel(token).catch(() => { + // ignore — the child may already be gone + }); + }, []); + + // Shared spawn/stream core for both the first ask and a retry. `replyId` is + // already reset to a pending state by the caller (startAIReply / retryAIReply). + const runSpawn = useCallback( + async (replyId: string, comment: Comment, userText: string, binding: AISessionBinding) => { + // Claim the next generation for this replyId. Any spawn still streaming + // under an earlier generation is now stale and its events are dropped. + const generation = (genRef.current.get(replyId) ?? 0) + 1; + genRef.current.set(replyId, generation); + const isCurrent = () => genRef.current.get(replyId) === generation; - const ask = useCallback( - async (comment: Comment, userText: string, binding: AISessionBinding) => { - const replyId = opts.startAIReply(comment.id); const mock = typeof window !== 'undefined' ? window.__quillMock : undefined; // A Quill-minted session never authored the doc, so the compaction @@ -346,7 +467,22 @@ export function useClaudeReply(opts: UseClaudeReplyOptions): UseClaudeReplyRetur } }; + // This spawn's own cancel token, captured as soon as the spawn returns it. + // The dispatch closure cancels/untracks by THIS token, never by replyId — + // after a retry the replyId points at the newer spawn, so a stale event + // resolving orphanCancel(replyId) would tear down the live retry instead. + let spawnToken: string | undefined; + const dispatch = (msg: ChunkEvent) => { + // A superseded spawn (a slower original that a retry replaced) must not + // mutate the reply the newer generation now owns. Drop its events; on a + // terminal event, tear down THIS orphaned child by its own token. + if (!isCurrent()) { + if (msg.kind === 'done' || msg.kind === 'cancelled' || msg.kind === 'error') { + if (spawnToken !== undefined) void sendCancel(spawnToken).catch(() => {}); + } + return; + } if (msg.kind === 'delta') { rawAccum += msg.text; emitVisible(false); @@ -354,15 +490,17 @@ export function useClaudeReply(opts: UseClaudeReplyOptions): UseClaudeReplyRetur emitVisible(true); finalize(); opts.finishAIReply(comment.id, replyId); - tokensRef.current.delete(replyId); + if (tokensRef.current.get(replyId) === spawnToken) tokensRef.current.delete(replyId); + inputsRef.current.delete(replyId); } else if (msg.kind === 'error') { opts.failAIReply(comment.id, replyId, msg.message); - tokensRef.current.delete(replyId); + if (tokensRef.current.get(replyId) === spawnToken) tokensRef.current.delete(replyId); + // Keep the stashed inputs — a retry needs them. } }; if (mock) { - const token = mock.spawn( + spawnToken = mock.spawn( { sessionId: binding.sessionId, cwd: binding.cwd, @@ -372,7 +510,7 @@ export function useClaudeReply(opts: UseClaudeReplyOptions): UseClaudeReplyRetur }, dispatch, ); - tokensRef.current.set(replyId, token); + tokensRef.current.set(replyId, spawnToken); return; } @@ -388,28 +526,62 @@ export function useClaudeReply(opts: UseClaudeReplyOptions): UseClaudeReplyRetur allowCreate: fresh, onEvent: channel, }); + spawnToken = cancelToken; + if (!isCurrent()) { + // A retry superseded this spawn while we awaited — cancel the orphan + // rather than tracking it against a replyId the newer generation owns. + // Route through sendCancel so the e2e mock's cancel fires too (a raw + // invoke would skip it and leak the orphan in mock-driven tests). + void sendCancel(cancelToken).catch(() => {}); + return; + } tokensRef.current.set(replyId, cancelToken); } catch (e) { - opts.failAIReply(comment.id, replyId, String(e)); + if (isCurrent()) opts.failAIReply(comment.id, replyId, String(e)); } }, [opts], ); + const ask = useCallback( + async (comment: Comment, userText: string, binding: AISessionBinding) => { + const replyId = opts.startAIReply(comment.id); + inputsRef.current.set(replyId, { comment, userText, binding }); + await runSpawn(replyId, comment, userText, binding); + }, + [opts, runSpawn], + ); + + const retry = useCallback( + async (replyId: string) => { + if (retryingRef.current.has(replyId)) return; // double-fire guard + const inputs = inputsRef.current.get(replyId); + if (!inputs) return; // nothing to re-issue (no-op, no throw) + retryingRef.current.add(replyId); + try { + // Tear down any orphan the failed original may have left tracked, then + // reset the reply in place (reuses the same entry, clears the error). + orphanCancel(replyId); + opts.retryAIReply(inputs.comment.id, replyId); + // Re-issue the identical request against the same comment. Ranges are + // re-read live inside runSpawn via opts.getRangeTexts. + await runSpawn(replyId, inputs.comment, inputs.userText, inputs.binding); + } finally { + retryingRef.current.delete(replyId); + } + }, + [opts, runSpawn, orphanCancel], + ); + const cancel = useCallback(async (replyId: string) => { const token = tokensRef.current.get(replyId); if (!token) return; - const mock = typeof window !== 'undefined' ? window.__quillMock : undefined; - if (mock) { - mock.cancel?.(token); - return; - } try { - await invoke('cancel_claude_resume', { cancelToken: token }); + await sendCancel(token); } catch (e) { console.error('Failed to cancel claude reply:', e); } }, []); - return { ask, cancel }; + return { ask, cancel, retry }; } diff --git a/src/hooks/useComments.ts b/src/hooks/useComments.ts index 39da539..d0c86f4 100644 --- a/src/hooks/useComments.ts +++ b/src/hooks/useComments.ts @@ -14,6 +14,7 @@ interface UseCommentsReturn { appendAIReplyChunk: (commentId: string, replyId: string, chunk: string) => void; finishAIReply: (commentId: string, replyId: string) => void; failAIReply: (commentId: string, replyId: string, message: string) => void; + retryAIReply: (commentId: string, replyId: string) => void; } export function useComments(): UseCommentsReturn { @@ -121,6 +122,24 @@ export function useComments(): UseCommentsReturn { ); }, []); + // Reset an existing (errored) AI reply in place so a retry reuses the same + // entry rather than appending a new one — clears the error, resets the + // streamed text, and marks it pending again. Unknown ids are a no-op. + const retryAIReply = useCallback((commentId: string, replyId: string) => { + setComments((prev) => + prev.map((c) => + c.id === commentId + ? { + ...c, + replies: c.replies.map((r) => + r.id === replyId ? { ...r, pending: true, error: undefined, text: '' } : r, + ), + } + : c, + ), + ); + }, []); + return { comments, setComments, @@ -133,5 +152,6 @@ export function useComments(): UseCommentsReturn { appendAIReplyChunk, finishAIReply, failAIReply, + retryAIReply, }; } diff --git a/src/hooks/useFileManager.ts b/src/hooks/useFileManager.ts index 14fd2a6..280fdf4 100644 --- a/src/hooks/useFileManager.ts +++ b/src/hooks/useFileManager.ts @@ -14,6 +14,22 @@ function emptySidecar(): SidecarFile { return { version: 2, comments: [], suggestions: [] }; } +/** + * Drop transient AI-reply state before serialization. A pending or errored AI + * reply is in-flight UI state — the request either never completed or failed — + * so it must never reach the on-disk sidecar, where it would resurrect a stuck + * spinner or a stale error on the next open. User replies and finished AI + * replies are kept untouched. Returns a new array; inputs are not mutated. + */ +export function stripTransientReplyState(comments: Comment[]): Comment[] { + return comments.map((c) => { + const kept = c.replies.filter( + (r) => !(r.authorKind === 'ai' && (r.pending || r.error !== undefined)), + ); + return kept.length === c.replies.length ? c : { ...c, replies: kept }; + }); +} + /** * Build a trusted SidecarFile from the raw parsed JSON. The sidecar sits on disk * next to the document and may be hand-edited, corrupted, or supplied by another @@ -162,7 +178,10 @@ export function useFileManager( contextFolder: string | null, ) => { const scPath = sidecarPath(path); - if (comments.length === 0 && suggestions.length === 0 && !aiSession && !contextFolder) { + // Never persist in-flight AI replies (pending/errored) — strip them first + // so an empty doc with only a failed reply still collapses to no sidecar. + const cleanComments = stripTransientReplyState(comments); + if (cleanComments.length === 0 && suggestions.length === 0 && !aiSession && !contextFolder) { // Clean up empty sidecar try { await invoke('delete_file', { path: scPath }); @@ -173,7 +192,7 @@ export function useFileManager( } const sidecar: SidecarFile = { version: 2, - comments, + comments: cleanComments, suggestions, ...(aiSession ? { aiSession } : {}), ...(contextFolder ? { contextFolder } : {}), diff --git a/src/test/hooks/claudeReplyHelpers.test.ts b/src/test/hooks/claudeReplyHelpers.test.ts index 74963bc..35af2ed 100644 --- a/src/test/hooks/claudeReplyHelpers.test.ts +++ b/src/test/hooks/claudeReplyHelpers.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from 'vitest'; -import { buildPrompt, detectScope, splitVisible } from '../../hooks/useClaudeReply'; +import { + buildPrompt, + classifyReplyError, + detectScope, + splitVisible, +} from '../../hooks/useClaudeReply'; import type { Comment, Reply } from '../../types'; describe('detectScope', () => { @@ -181,6 +186,90 @@ describe('buildPrompt context folder', () => { }); }); +describe('classifyReplyError', () => { + it('classifies session-loss errors as session (retryable, re-link primary)', () => { + expect(classifyReplyError('No conversation found for this session')).toEqual({ + retryable: true, + kind: 'session', + }); + expect(classifyReplyError('Invalid session ID')).toEqual({ retryable: true, kind: 'session' }); + expect(classifyReplyError('session not found')).toEqual({ retryable: true, kind: 'session' }); + }); + + it('classifies auth errors as auth and not retryable', () => { + for (const msg of [ + 'Authentication failed', + 'Unauthorized', + 'HTTP 401', + 'Please login again', + 'Invalid API key', + 'missing credentials', + ]) { + expect(classifyReplyError(msg)).toEqual({ retryable: false, kind: 'auth' }); + } + }); + + it('classifies transient/API errors as transient (retryable)', () => { + for (const msg of [ + 'API Error: something went wrong', + 'HTTP 429 Too Many Requests', + 'status 503', + 'Overloaded', + 'request timeout', + 'network unreachable', + 'rate limit exceeded', + 'ECONNRESET', + 'Unsupported parameter: thinking.type', + ]) { + expect(classifyReplyError(msg)).toEqual({ retryable: true, kind: 'transient' }); + } + }); + + it('matches case-insensitively', () => { + expect(classifyReplyError('NO CONVERSATION FOUND').kind).toBe('session'); + expect(classifyReplyError('OVERLOADED').kind).toBe('transient'); + expect(classifyReplyError('UNAUTHORIZED').kind).toBe('auth'); + }); + + it('orders session before transient when both could match', () => { + // A session-loss message that also mentions a timeout must lead with re-link. + expect(classifyReplyError('No conversation found (timeout)').kind).toBe('session'); + }); + + it('does not false-match ordinary words (negative control)', () => { + // "author" contains "auth" but must NOT classify as auth; a plain sentence + // with no error markers is unknown. + expect(classifyReplyError('The author revised the document.')).toEqual({ + retryable: true, + kind: 'unknown', + }); + }); + + it('anchors the 401 and login auth markers to word boundaries', () => { + // A port/line number that merely contains the digits "401", or a hostname + // that contains "login" as a substring, must not force a non-retryable auth + // verdict onto an otherwise-retryable transient failure. + expect(classifyReplyError('connection refused on port 24010')).toEqual({ + retryable: true, + kind: 'unknown', + }); + expect(classifyReplyError('timeout reaching mylogin.internal host')).toEqual({ + retryable: true, + kind: 'transient', + }); + // The real markers still classify as auth. + expect(classifyReplyError('server returned 401').kind).toBe('auth'); + expect(classifyReplyError('please login again').kind).toBe('auth'); + }); + + it('treats empty and whitespace input as unknown without throwing', () => { + expect(classifyReplyError('')).toEqual({ retryable: true, kind: 'unknown' }); + expect(classifyReplyError(' ')).toEqual({ retryable: true, kind: 'unknown' }); + // @ts-expect-error — guarding the runtime nullish path + expect(classifyReplyError(undefined)).toEqual({ retryable: true, kind: 'unknown' }); + }); +}); + describe('splitVisible', () => { it('returns all text as visible when no fence', () => { const { visible, block } = splitVisible('Just a normal reply.'); diff --git a/src/test/hooks/useClaudeReplyRetry.test.ts b/src/test/hooks/useClaudeReplyRetry.test.ts new file mode 100644 index 0000000..972a449 --- /dev/null +++ b/src/test/hooks/useClaudeReplyRetry.test.ts @@ -0,0 +1,186 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; + +// The retry path never reaches the real Tauri IPC: spawns/cancels route through +// window.__quillMock, and the two pre-spawn invokes (check_session_compacted, +// list_context_files) are best-effort and swallowed. Stub the module so the +// import resolves and those probes reject harmlessly. +vi.mock('@tauri-apps/api/core', () => ({ + invoke: vi.fn().mockRejectedValue(new Error('no tauri in test')), + Channel: class { + onmessage: ((e: unknown) => void) | null = null; + }, +})); + +import { useClaudeReply } from '../../hooks/useClaudeReply'; +import type { ChunkEvent, RangeTexts } from '../../hooks/useClaudeReply'; +import type { AISessionBinding, Comment, EditScope, QuillEdit } from '../../types'; + +const BINDING: AISessionBinding = { + provider: 'claude-code', + sessionId: 's1', + cwd: '/tmp', + linkedAt: new Date().toISOString(), +}; + +function makeComment(): Comment { + return { + id: 'c1', + anchorText: 'anchor', + from: 1, + to: 7, + author: 'Sam', + createdAt: new Date().toISOString(), + resolved: false, + replies: [], + }; +} + +/** + * A controllable mock claude spawn. Each spawn stashes its dispatch callback + * keyed by token so the test can deliver stream events on demand, letting a + * "slow original" finish *after* a retry has superseded it. + */ +class MockClaude { + private seq = 0; + readonly dispatchers = new Map void>(); + readonly cancelled: string[] = []; + + install() { + window.__quillMock = { + spawn: (_args, onEvent) => { + const token = `tok-${++this.seq}`; + this.dispatchers.set(token, onEvent); + return token; + }, + cancel: (token: string) => { + this.cancelled.push(token); + }, + }; + window.__quillTestSession = BINDING; + } + + emit(token: string, event: ChunkEvent) { + this.dispatchers.get(token)?.(event); + } +} + +function makeOpts(mock: MockClaude) { + const startAIReply = vi.fn((_commentId: string) => 'r0'); + const appendAIReplyChunk = vi.fn(); + const finishAIReply = vi.fn(); + const failAIReply = vi.fn(); + const retryAIReply = vi.fn(); + const getDocMarkdown = vi.fn(() => 'doc body'); + const getRangeTexts = vi.fn( + (_c: Comment): RangeTexts => ({ highlightText: 'anchor', paragraphText: 'anchor para' }), + ); + const applyTrackedEdits = vi.fn((_c: Comment, _e: QuillEdit[], _s: EditScope) => ({ + applied: 0, + skipped: 0, + })); + const getContextFolder = vi.fn(() => null); + return { + opts: { + startAIReply, + appendAIReplyChunk, + finishAIReply, + failAIReply, + retryAIReply, + getDocMarkdown, + getRangeTexts, + applyTrackedEdits, + getContextFolder, + }, + spies: { startAIReply, appendAIReplyChunk, finishAIReply, failAIReply, retryAIReply }, + mock, + }; +} + +describe('useClaudeReply generation guard (retry vs. slow original)', () => { + let mock: MockClaude; + + beforeEach(() => { + mock = new MockClaude(); + mock.install(); + }); + + afterEach(() => { + delete window.__quillMock; + delete window.__quillTestSession; + vi.clearAllMocks(); + }); + + it("drops a superseded original's late terminal event and cancels the orphan", async () => { + const { opts, spies } = makeOpts(mock); + const { result } = renderHook(() => useClaudeReply(opts)); + const comment = makeComment(); + + // First ask spawns the original (tok-1) — startAIReply mints replyId 'r0'. + await act(async () => { + await result.current.ask(comment, 'fix this', BINDING); + }); + expect(mock.dispatchers.has('tok-1')).toBe(true); + + // The original errors, marking the reply failed (its inputs stay stashed). + act(() => { + mock.emit('tok-1', { kind: 'error', message: 'API Error: overloaded' }); + }); + expect(spies.failAIReply).toHaveBeenCalledWith('c1', 'r0', 'API Error: overloaded'); + + // The user hits Retry: same replyId, new generation, new spawn (tok-2). + await act(async () => { + await result.current.retry('r0'); + }); + expect(spies.retryAIReply).toHaveBeenCalledWith('c1', 'r0'); + expect(mock.dispatchers.has('tok-2')).toBe(true); + + // Now the *original's* late 'done' arrives on tok-1 — after it was superseded. + // It must NOT finish the reply the retry now owns; instead it orphan-cancels. + spies.finishAIReply.mockClear(); + act(() => { + mock.emit('tok-1', { kind: 'done' }); + }); + expect(spies.finishAIReply).not.toHaveBeenCalled(); + expect(mock.cancelled).toContain('tok-1'); + + // The retried spawn (tok-2) is current: its 'done' finishes the reply. + act(() => { + mock.emit('tok-2', { kind: 'done' }); + }); + expect(spies.finishAIReply).toHaveBeenCalledWith('c1', 'r0'); + }); + + it('ignores a double-fire retry while one is already launching', async () => { + const { opts, spies } = makeOpts(mock); + const { result } = renderHook(() => useClaudeReply(opts)); + const comment = makeComment(); + + await act(async () => { + await result.current.ask(comment, 'fix this', BINDING); + }); + act(() => { + mock.emit('tok-1', { kind: 'error', message: 'timeout' }); + }); + + // Two retries fired back-to-back within the same tick — the guard must let + // only one through (one retryAIReply reset, one fresh spawn). + await act(async () => { + await Promise.all([result.current.retry('r0'), result.current.retry('r0')]); + }); + expect(spies.retryAIReply).toHaveBeenCalledTimes(1); + expect(mock.dispatchers.has('tok-2')).toBe(true); + expect(mock.dispatchers.has('tok-3')).toBe(false); + }); + + it('is a no-op retry for a replyId with no stashed inputs', async () => { + const { opts, spies } = makeOpts(mock); + const { result } = renderHook(() => useClaudeReply(opts)); + + await act(async () => { + await result.current.retry('never-asked'); + }); + expect(spies.retryAIReply).not.toHaveBeenCalled(); + expect(mock.dispatchers.size).toBe(0); + }); +}); diff --git a/src/test/hooks/useComments.test.ts b/src/test/hooks/useComments.test.ts index f47a7ad..5d7d04f 100644 --- a/src/test/hooks/useComments.test.ts +++ b/src/test/hooks/useComments.test.ts @@ -172,4 +172,57 @@ describe('useComments', () => { expect(result.current.comments).toHaveLength(1); }); }); + + describe('retryAIReply', () => { + // Set up a comment carrying a single failed AI reply, returning both ids. + function withFailedReply() { + const { result } = renderHook(() => useComments()); + let commentId = ''; + let replyId = ''; + act(() => { + commentId = result.current.addComment('text', 0, 4, 'Alice').id; + }); + act(() => { + replyId = result.current.startAIReply(commentId); + }); + act(() => { + result.current.appendAIReplyChunk(commentId, replyId, 'partial answer'); + }); + act(() => { + result.current.failAIReply(commentId, replyId, 'API Error: overloaded'); + }); + return { result, commentId, replyId }; + } + + it('resets the same reply to a pending state without appending a new one', () => { + const { result, commentId, replyId } = withFailedReply(); + expect(result.current.comments[0].replies).toHaveLength(1); + expect(result.current.comments[0].replies[0].error).toBe('API Error: overloaded'); + + act(() => { + result.current.retryAIReply(commentId, replyId); + }); + + const replies = result.current.comments[0].replies; + expect(replies).toHaveLength(1); // reused, not appended + const r = replies[0]; + expect(r.id).toBe(replyId); // id is stable + expect(r.pending).toBe(true); + expect(r.error).toBeUndefined(); + expect(r.text).toBe(''); // streamed text cleared + expect(r.authorKind).toBe('ai'); + }); + + it('is a no-op for an unknown replyId', () => { + const { result, commentId } = withFailedReply(); + const before = result.current.comments[0].replies[0]; + + act(() => { + result.current.retryAIReply(commentId, 'nonexistent-reply'); + }); + + expect(result.current.comments[0].replies).toHaveLength(1); + expect(result.current.comments[0].replies[0]).toEqual(before); + }); + }); }); diff --git a/src/test/hooks/useFileManager.test.ts b/src/test/hooks/useFileManager.test.ts index 532d834..e6772d4 100644 --- a/src/test/hooks/useFileManager.test.ts +++ b/src/test/hooks/useFileManager.test.ts @@ -6,8 +6,8 @@ vi.mock('@tauri-apps/api/core', () => ({ })); import { invoke } from '@tauri-apps/api/core'; -import { useFileManager } from '../../hooks/useFileManager'; -import type { Comment } from '../../types'; +import { useFileManager, stripTransientReplyState } from '../../hooks/useFileManager'; +import type { Comment, Reply } from '../../types'; const mockInvoke = vi.mocked(invoke); @@ -196,6 +196,50 @@ describe('useFileManager', () => { expect(written.comments).toHaveLength(1); }); + it('strips transient AI replies from the written sidecar', async () => { + mockInvoke.mockResolvedValue(undefined); + const commentWithReplies: Comment = { + ...SAMPLE_COMMENT, + replies: [ + { id: 'u1', author: 'Alice', text: 'nice', createdAt: '', authorKind: 'user' }, + { + id: 'a1', + author: 'Claude', + text: 'done', + createdAt: '', + authorKind: 'ai', + }, + { id: 'a2', author: 'Claude', text: '', createdAt: '', authorKind: 'ai', pending: true }, + { + id: 'a3', + author: 'Claude', + text: 'oops', + createdAt: '', + authorKind: 'ai', + error: 'API Error', + }, + ], + }; + const { result } = renderHook(() => useFileManager()); + await act(async () => { + await result.current.saveFile( + 'content', + [commentWithReplies], + [], + null, + null, + '/docs/t.md', + ); + }); + const sidecarCall = mockInvoke.mock.calls.find( + (call) => + call[0] === 'write_file' && (call[1] as { path: string }).path.endsWith('.comments.json'), + ); + const written = JSON.parse((sidecarCall![1] as { content: string }).content); + const persisted = written.comments[0].replies.map((r: Reply) => r.id); + expect(persisted).toEqual(['u1', 'a1']); // pending a2 + errored a3 dropped + }); + it('does not clobber the sidecar on same-path save when it was corrupt on open', async () => { // Open a file whose sidecar is present but unreadable, then save back to // the same path. The corrupt sidecar must be left untouched (no write, no @@ -442,3 +486,68 @@ describe('useFileManager', () => { }); }); }); + +describe('stripTransientReplyState', () => { + const reply = (over: Partial): Reply => ({ + id: 'r', + author: 'x', + text: 't', + createdAt: '', + authorKind: 'user', + ...over, + }); + + const commentWith = (replies: Reply[]): Comment => ({ ...SAMPLE_COMMENT, replies }); + + it('drops an errored AI reply', () => { + const out = stripTransientReplyState([ + commentWith([reply({ id: 'a', authorKind: 'ai', error: 'API Error' })]), + ]); + expect(out[0].replies).toEqual([]); + }); + + it('drops a pending AI reply', () => { + const out = stripTransientReplyState([ + commentWith([reply({ id: 'a', authorKind: 'ai', text: '', pending: true })]), + ]); + expect(out[0].replies).toEqual([]); + }); + + it('keeps a finished AI reply', () => { + const finished = reply({ id: 'a', authorKind: 'ai', text: 'done' }); + const out = stripTransientReplyState([commentWith([finished])]); + expect(out[0].replies).toEqual([finished]); + }); + + it('keeps user replies, even pending/errored ones (only AI transient state is stripped)', () => { + // pending/error should never appear on a user reply, but the guard is + // scoped to authorKind 'ai' — a user reply is retained regardless. + const userReplies = [ + reply({ id: 'u1', text: 'hi' }), + reply({ id: 'u2', text: 'yo', pending: true }), + ]; + const out = stripTransientReplyState([commentWith(userReplies)]); + expect(out[0].replies).toEqual(userReplies); + }); + + it('returns the same comment reference when nothing is stripped', () => { + const comment = commentWith([ + reply({ id: 'u1' }), + reply({ id: 'a', authorKind: 'ai', text: 'x' }), + ]); + const out = stripTransientReplyState([comment]); + expect(out[0]).toBe(comment); // referential identity preserved — no needless copy + }); + + it('strips only the transient replies from a mixed thread', () => { + const out = stripTransientReplyState([ + commentWith([ + reply({ id: 'u1' }), + reply({ id: 'a1', authorKind: 'ai', text: 'done' }), + reply({ id: 'a2', authorKind: 'ai', text: '', pending: true }), + reply({ id: 'a3', authorKind: 'ai', text: 'oops', error: 'boom' }), + ]), + ]); + expect(out[0].replies.map((r) => r.id)).toEqual(['u1', 'a1']); + }); +});