diff --git a/client/src/components/Artifacts/ArtifactCodeEditor.test.tsx b/client/src/components/Artifacts/ArtifactCodeEditor.test.tsx new file mode 100644 index 0000000000..cfb6bce9cb --- /dev/null +++ b/client/src/components/Artifacts/ArtifactCodeEditor.test.tsx @@ -0,0 +1,244 @@ +import React from 'react'; +import { render, act } from '@testing-library/react'; +import type { editor } from 'monaco-editor'; +import type { Artifact } from '~/common'; +import { ArtifactCodeEditor } from './ArtifactCodeEditor'; + +interface MutationVars { + updated: string; + messageId: string; + index: number; + original?: string; +} + +interface MutationHandlers { + onMutate?: (vars: MutationVars) => void; + onSuccess?: (data: unknown, vars: MutationVars) => void; + onError?: (error?: unknown) => void; +} + +const mockEditorProps: { onChange?: (value: string | undefined) => void } = {}; +const mockMutationHandlers: MutationHandlers = {}; + +// Calling mutate replays onMutate synchronously so currentUpdateRef reflects the +// in-flight content, matching how react-query drives the real mutation lifecycle. +const mockMutate = jest.fn((vars: MutationVars) => { + mockMutationHandlers.onMutate?.(vars); +}); + +jest.mock('@monaco-editor/react', () => ({ + __esModule: true, + default: (props: { onChange?: (value: string | undefined) => void }) => { + mockEditorProps.onChange = props.onChange; + return null; + }, +})); + +jest.mock('~/Providers/EditorContext', () => { + const ReactModule = jest.requireActual('react'); + return { + useMutationState: () => { + const [isMutating, setIsMutating] = ReactModule.useState(false); + return { isMutating, setIsMutating }; + }, + useCodeState: () => { + const [currentCode, setCurrentCode] = ReactModule.useState(''); + return { currentCode, setCurrentCode }; + }, + }; +}); + +jest.mock('~/Providers', () => ({ + useArtifactsContext: () => ({ isSubmitting: false }), +})); + +jest.mock('~/data-provider', () => ({ + useEditArtifact: (handlers: MutationHandlers) => { + mockMutationHandlers.onMutate = handlers.onMutate; + mockMutationHandlers.onSuccess = handlers.onSuccess; + mockMutationHandlers.onError = handlers.onError; + return { mutate: mockMutate }; + }, +})); + +const ORIGINAL = 'ORIGINAL'; + +const artifact: Artifact = { + id: 'artifact-1', + lastUpdateTime: 0, + index: 0, + messageId: 'msg-1', + content: ORIGINAL, + type: 'text/plain', +}; + +const otherArtifact: Artifact = { + id: 'artifact-2', + lastUpdateTime: 0, + index: 0, + messageId: 'msg-2', + content: 'ORIGINAL-B', + type: 'text/plain', +}; + +const renderEditor = (initial: Artifact = artifact) => { + const monacoRef: React.MutableRefObject = { current: null }; + const utils = render(); + const rerenderWith = (next: Artifact) => + utils.rerender(); + return { ...utils, rerenderWith }; +}; + +const fireEdit = (value: string) => { + act(() => { + mockEditorProps.onChange?.(value); + jest.advanceTimersByTime(500); + }); +}; + +describe('ArtifactCodeEditor retry guard', () => { + beforeEach(() => { + jest.useFakeTimers(); + mockEditorProps.onChange = undefined; + mockMutationHandlers.onMutate = undefined; + mockMutationHandlers.onSuccess = undefined; + mockMutationHandlers.onError = undefined; + }); + + afterEach(() => { + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + }); + + it('does not re-run a mutation for content that just failed', () => { + renderEditor(); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + expect(mockMutate).toHaveBeenLastCalledWith(expect.objectContaining({ updated: 'EDITED' })); + + act(() => { + mockMutationHandlers.onError?.({ status: 400 }); + }); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + }); + + it('blocks the retry for a real AxiosError 400 (production error shape)', () => { + renderEditor(); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + act(() => { + mockMutationHandlers.onError?.({ isAxiosError: true, response: { status: 400 } }); + }); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + }); + + it('blocks the synchronous pending re-run of content that just failed', () => { + renderEditor(); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + act(() => { + mockMutationHandlers.onError?.({ status: 400 }); + }); + + expect(mockMutate).toHaveBeenCalledTimes(1); + }); + + it('clears the guard on success so the same content can run again', () => { + renderEditor(); + + fireEdit('EDITED'); + act(() => { + mockMutationHandlers.onError?.({ status: 400 }); + }); + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + fireEdit('DIFFERENT'); + expect(mockMutate).toHaveBeenCalledTimes(2); + + act(() => { + mockMutationHandlers.onSuccess?.(undefined, { + updated: 'DIFFERENT', + messageId: 'msg-1', + index: 0, + }); + }); + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(3); + expect(mockMutate).toHaveBeenLastCalledWith(expect.objectContaining({ updated: 'EDITED' })); + }); + + it('clears the guard when the artifact changes', () => { + const { rerenderWith } = renderEditor(); + + fireEdit('EDITED'); + act(() => { + mockMutationHandlers.onError?.({ status: 400 }); + }); + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + act(() => { + rerenderWith(otherArtifact); + }); + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(2); + expect(mockMutate).toHaveBeenLastCalledWith(expect.objectContaining({ updated: 'EDITED' })); + }); + + it('re-runs identical content after a non-400 client error (transient, not deterministic)', () => { + renderEditor(); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + act(() => { + mockMutationHandlers.onError?.({ status: 429 }); + }); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(2); + expect(mockMutate).toHaveBeenLastCalledWith(expect.objectContaining({ updated: 'EDITED' })); + }); + + it('re-runs identical content after a 5xx error (transient, not deterministic)', () => { + renderEditor(); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + act(() => { + mockMutationHandlers.onError?.({ status: 503 }); + }); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(2); + expect(mockMutate).toHaveBeenLastCalledWith(expect.objectContaining({ updated: 'EDITED' })); + }); + + it('re-runs identical content after an error with no status (network blip)', () => { + renderEditor(); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(1); + + act(() => { + mockMutationHandlers.onError?.(new Error('Network Error')); + }); + + fireEdit('EDITED'); + expect(mockMutate).toHaveBeenCalledTimes(2); + }); +}); diff --git a/client/src/components/Artifacts/ArtifactCodeEditor.tsx b/client/src/components/Artifacts/ArtifactCodeEditor.tsx index a349f30eda..c8963c2dcc 100644 --- a/client/src/components/Artifacts/ArtifactCodeEditor.tsx +++ b/client/src/components/Artifacts/ArtifactCodeEditor.tsx @@ -5,6 +5,7 @@ import type { Monaco } from '@monaco-editor/react'; import type { editor } from 'monaco-editor'; import type { Artifact } from '~/common'; import { useMutationState, useCodeState } from '~/Providers/EditorContext'; +import { getResponseStatus } from '~/utils/errors'; import { useArtifactsContext } from '~/Providers'; import { useEditArtifact } from '~/data-provider'; @@ -111,10 +112,12 @@ export const ArtifactCodeEditor = function ArtifactCodeEditor({ const { setCurrentCode } = useCodeState(); const [currentUpdate, setCurrentUpdate] = useState(null); const { isMutating, setIsMutating } = useMutationState(); + const [failedContent, setFailedContent] = useState(null); const artifactRef = useRef(artifact); const isMutatingRef = useRef(isMutating); const currentUpdateRef = useRef(currentUpdate); const setCurrentCodeRef = useRef(setCurrentCode); + const failedContentRef = useRef(failedContent); const pendingUpdateRef = useRef(null); const runMutationRef = useRef<(code: string, original?: string) => void>(() => {}); @@ -130,6 +133,7 @@ export const ArtifactCodeEditor = function ArtifactCodeEditor({ currentUpdateRef.current = null; setIsMutating(false); setCurrentUpdate(null); + setFailedContent(null); const pending = pendingUpdateRef.current; pendingUpdateRef.current = null; @@ -148,7 +152,12 @@ export const ArtifactCodeEditor = function ArtifactCodeEditor({ runMutationRef.current(pending.code, original); } }, - onError: () => { + onError: (error) => { + const status = getResponseStatus(error); + if (status === 400 && currentUpdateRef.current != null) { + setFailedContent(currentUpdateRef.current); + failedContentRef.current = currentUpdateRef.current; + } const pending = pendingUpdateRef.current; pendingUpdateRef.current = null; isMutatingRef.current = false; @@ -182,6 +191,7 @@ export const ArtifactCodeEditor = function ArtifactCodeEditor({ currentUpdateRef.current = currentUpdate; editArtifactRef.current = editArtifact; setCurrentCodeRef.current = setCurrentCode; + failedContentRef.current = failedContent; const runMutation = useCallback( (code: string, originalOverride?: string) => { @@ -209,6 +219,10 @@ export const ArtifactCodeEditor = function ArtifactCodeEditor({ return; } + if (failedContentRef.current != null && code.trim() === failedContentRef.current.trim()) { + return; + } + setCurrentCodeRef.current(code); editArtifactRef.current.mutate({ index: target.index, @@ -284,6 +298,7 @@ export const ArtifactCodeEditor = function ArtifactCodeEditor({ } prevArtifactId.current = artifact.id; pendingUpdateRef.current = null; + setFailedContent(null); prevContentRef.current = artifact.content ?? ''; const ed = monacoRef.current; if (ed && artifact.content != null) {