diff --git a/client/src/common/types.ts b/client/src/common/types.ts index d1c53bbfa5..ada674c6ac 100644 --- a/client/src/common/types.ts +++ b/client/src/common/types.ts @@ -330,14 +330,16 @@ export type TAskProps = { export type TOptions = { editedMessageId?: string | null; editedContent?: t.TEditedContent; - editedText?: string | null; isRegenerate?: boolean; isContinued?: boolean; isEdited?: boolean; overrideMessages?: t.TMessage[]; - /** This value is only true when the user submits a message with "Update & rerun" for a user-created message */ - isResubmission?: boolean; - /** Currently only utilized when `isResubmission === true`, uses that message's currently attached files */ + /** + * Authoritative attachment list for this submission: a rerun replays the edited + * message's stored files, and an auto-drained queued message replays the ones taken + * out of the composer when it was queued. Authoritative even when empty, so a drain + * never vacuums up attachments the user staged for their next send. + */ overrideFiles?: t.TMessage['files']; /** * Assistant message being regenerated. Used to derive the optimistic response diff --git a/client/src/components/Chat/Messages/Content/EditContentParts.tsx b/client/src/components/Chat/Messages/Content/EditContentParts.tsx index 32067a2edb..ac227b6d97 100644 --- a/client/src/components/Chat/Messages/Content/EditContentParts.tsx +++ b/client/src/components/Chat/Messages/Content/EditContentParts.tsx @@ -226,23 +226,36 @@ export default function EditContentParts({ updateMessageContentMutation, ]); + /** A rerun with no edits is a first-class action, not a mistake: a cancelled or + * failed response, or a backend restarted on different parameters, has to be + * reissued byte-for-byte. Gating the button on a change only taught people to + * type a space and delete it again. */ const updateAndRerun = useCallback(() => { - const firstChange = changedParts[0]; - if (!firstChange || !editedMessage || rerunRequiresSave || hasBlankEdit || isBusy) { + if (!editedMessage || rerunRequiresSave || hasBlankEdit || isBusy) { return; } + const firstChange = changedParts[0]; const messages = getMessages(); /** `ask` refuses to send while another response is streaming and reports it by * returning false. Closing the editor regardless would throw the drafts away for * a rerun that never started, so a refused send leaves the editor as it was. */ let refused = false; + /** An unedited assistant turn regenerates in place, landing as a sibling of this + * response rather than of the user turn this editor's sibling index walks. */ + let regenerated = false; if (editedMessage.isCreatedByUser === true) { const userText = editableParts .filter((part) => part.type === ContentTypes.TEXT) .map((part) => drafts[part.index]) .join('\n'); + /** Retained attachments make an otherwise textless request submittable, matching + * the composer and the sibling `EditMessage` form's `required` rule. A turn with + * neither text nor files has nothing to send, edited or not. */ + if (userText.trim() === '' && (editedMessage.files?.length ?? 0) === 0) { + return; + } refused = ask( { @@ -264,38 +277,59 @@ export default function EditContentParts({ if (!parentMessage) { return; } - const editedContent = - firstChange.type === ContentTypes.THINK - ? { - index: firstChange.index, - type: ContentTypes.THINK as const, - [ContentTypes.THINK]: drafts[firstChange.index], - } - : { - index: firstChange.index, - type: ContentTypes.TEXT as const, - [ContentTypes.TEXT]: drafts[firstChange.index], - }; - refused = - ask( - { ...parentMessage }, - { - editedContent, - editedMessageId: messageId, - isRegenerate: true, - isEdited: true, - overrideManualSkills: parentMessage.manualSkills, - overrideQuotes: parentMessage.quotes, - addedConvo: getAddedConvo() || undefined, - }, - ) === false; + if (!firstChange) { + /** No edit means no retained prefix to continue from, so rerunning the response + * regenerates it: the same submission the hover action sends. Replaying the + * unchanged part as `editedContent` instead would keep this answer and append + * a second one to it. */ + regenerated = true; + refused = + ask( + { ...parentMessage }, + { + isRegenerate: true, + targetResponseMessageId: messageId, + overrideManualSkills: parentMessage.manualSkills, + overrideQuotes: parentMessage.quotes, + addedConvo: getAddedConvo() || undefined, + }, + ) === false; + } else { + const editedContent = + firstChange.type === ContentTypes.THINK + ? { + index: firstChange.index, + type: ContentTypes.THINK as const, + [ContentTypes.THINK]: drafts[firstChange.index], + } + : { + index: firstChange.index, + type: ContentTypes.TEXT as const, + [ContentTypes.TEXT]: drafts[firstChange.index], + }; + refused = + ask( + { ...parentMessage }, + { + editedContent, + editedMessageId: messageId, + isRegenerate: true, + isEdited: true, + overrideManualSkills: parentMessage.manualSkills, + overrideQuotes: parentMessage.quotes, + addedConvo: getAddedConvo() || undefined, + }, + ) === false; + } } if (refused) { return; } - setSiblingIdx((siblingIdx ?? 0) - 1); + if (!regenerated) { + setSiblingIdx((siblingIdx ?? 0) - 1); + } enterEdit(true); }, [ ask, @@ -436,9 +470,9 @@ export default function EditContentParts({ size="sm" variant="submit" onClick={updateAndRerun} - disabled={changedParts.length === 0 || rerunRequiresSave || hasBlankEdit || isBusy} + disabled={rerunRequiresSave || hasBlankEdit || isBusy} > - {localize('com_ui_update_rerun')} + {changedParts.length > 0 ? localize('com_ui_update_rerun') : localize('com_ui_rerun')} diff --git a/client/src/components/Chat/Messages/Content/EditMessage.tsx b/client/src/components/Chat/Messages/Content/EditMessage.tsx index ab58961a47..976347a6b2 100644 --- a/client/src/components/Chat/Messages/Content/EditMessage.tsx +++ b/client/src/components/Chat/Messages/Content/EditMessage.tsx @@ -29,6 +29,9 @@ const EditMessage = ({ const textAreaRef = useRef(null); const { conversationId, parentMessageId, messageId } = message; + /** Only a user turn's draft becomes the submission; an assistant turn's is discarded + * by the rerun (see `resubmitMessage`), so it must not be labelled as an update. */ + const isUserTurn = message.isCreatedByUser === true; const updateMessageMutation = useUpdateMessageMutation(conversationId ?? ''); const localize = useLocalize(); @@ -62,63 +65,68 @@ const EditMessage = ({ * returning false. Closing the editor regardless would throw the draft away for * a rerun that never started, so a refused send leaves the editor as it was. */ const resubmitMessage = (data: { text: string }) => { - if (message.isCreatedByUser) { - const submitted = ask( - { - text: data.text, - parentMessageId, - conversationId, - }, - { - overrideFiles: message.files, - /** Pills on the edited user message stay visible after save-and-submit; - * carry the picks forward so the new turn primes the same skills - * instead of running unprimed. */ - overrideManualSkills: message.manualSkills, - /** Carry the edited user message's quoted excerpts forward so the new - * turn sends the same referenced context the pills still show. */ - overrideQuotes: message.quotes, - addedConvo: getAddedConvo() || undefined, - }, - ); + const submitted = ask( + { + text: data.text, + parentMessageId, + conversationId, + }, + { + overrideFiles: message.files, + /** Pills on the edited user message stay visible after save-and-submit; + * carry the picks forward so the new turn primes the same skills + * instead of running unprimed. */ + overrideManualSkills: message.manualSkills, + /** Carry the edited user message's quoted excerpts forward so the new + * turn sends the same referenced context the pills still show. */ + overrideQuotes: message.quotes, + addedConvo: getAddedConvo() || undefined, + }, + ); - if (submitted === false) { - return; - } - - setSiblingIdx((siblingIdx ?? 0) - 1); - } else { - const messages = getMessages(); - const parentMessage = messages?.find((msg) => msg.messageId === parentMessageId); - - if (!parentMessage) { - return; - } - const submitted = ask( - { ...parentMessage }, - { - editedText: data.text, - editedMessageId: messageId, - isRegenerate: true, - isEdited: true, - /** Edit-assistant-response flow replays the parent user turn; keep - * the same manual skills so the regenerated response is primed - * identically. */ - overrideManualSkills: parentMessage.manualSkills, - /** Replaying the parent user turn: keep its quoted excerpts so the - * regenerated response is sent the same referenced context. */ - overrideQuotes: parentMessage.quotes, - addedConvo: getAddedConvo() || undefined, - }, - ); - - if (submitted === false) { - return; - } - - setSiblingIdx((siblingIdx ?? 0) - 1); + if (submitted === false) { + return; } + setSiblingIdx((siblingIdx ?? 0) - 1); + enterEdit(true); + }; + + /** No draft reaches the submission: `editedContent` is index-addressed over a content + * array and this editor only opens on messages that have none, so a text edit has + * nothing to target. Rerunning an answer is therefore a plain regeneration, the same + * one the hover action sends, which is why the draft neither gates this nor is + * offered as an update. Deliberately NOT routed through `handleSubmit`: the field is + * required so Save cannot blank a response, and a response that is already empty (a + * cancellation before the first token) is exactly what needs rerunning. */ + const rerunResponse = () => { + const parentMessage = getMessages()?.find((msg) => msg.messageId === parentMessageId); + + if (!parentMessage) { + return; + } + const submitted = ask( + { ...parentMessage }, + { + isRegenerate: true, + /** Name the response being regenerated. Without it the submission resolves the + * NEWEST answer for this turn, so rerunning an older sibling prunes the wrong + * subtree from the optimistic thread. */ + targetResponseMessageId: messageId, + /** Replaying the parent user turn: keep its manual skills and quoted excerpts so + * the regenerated response is primed and given the same context as the first. */ + overrideManualSkills: parentMessage.manualSkills, + overrideQuotes: parentMessage.quotes, + addedConvo: getAddedConvo() || undefined, + }, + ); + + if (submitted === false) { + return; + } + + /** The new answer is a sibling of this one, not of the user turn `siblingIdx` walks, + * so the index stays put and the thread follows the appended child. */ enterEdit(true); }; @@ -229,7 +237,9 @@ const EditMessage = ({ className="line-clamp-2 min-w-0 flex-1 text-xs text-text-secondary" aria-live="polite" > - {isDirty ? localize('com_ui_unsaved_changes') : ''} + {isDirty + ? localize(isUserTurn ? 'com_ui_unsaved_changes' : 'com_ui_rerun_discards_changes') + : ''}
diff --git a/client/src/components/Chat/Messages/Content/__tests__/EditContentParts.spec.tsx b/client/src/components/Chat/Messages/Content/__tests__/EditContentParts.spec.tsx index 40d0cc9d22..4c69c8d49c 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/EditContentParts.spec.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/EditContentParts.spec.tsx @@ -71,6 +71,7 @@ describe('EditContentParts', () => { beforeEach(() => { jest.clearAllMocks(); mockMutateAsync.mockReset(); + mockAsk.mockReset(); mockMutateAsync.mockResolvedValue({}); mockChatDirection = 'LTR'; message.content = undefined; @@ -222,6 +223,96 @@ describe('EditContentParts', () => { expect(enterEdit).not.toHaveBeenCalled(); }); + /** The rerun that matters most needs no edit at all: a cancelled response, or a + * backend restarted on different parameters, has to be reissued untouched. */ + it('reruns an unchanged assistant response as a regeneration', () => { + const enterEdit = jest.fn(); + const setSiblingIdx = jest.fn(); + render( + null} + />, + ); + + const rerun = screen.getByRole('button', { name: 'com_ui_rerun' }); + expect(rerun).toBeEnabled(); + fireEvent.click(rerun); + + expect(mockAsk).toHaveBeenCalledWith( + parentMessage, + expect.objectContaining({ + isRegenerate: true, + targetResponseMessageId: message.messageId, + }), + ); + /** Replaying the untouched part as an edit would retain this answer and append a + * second one to it rather than produce a new one. */ + expect(mockAsk.mock.calls[0][1]).not.toHaveProperty('editedContent'); + /** The regenerated answer is a sibling of this response, not of the user turn the + * sibling index walks. */ + expect(setSiblingIdx).not.toHaveBeenCalled(); + expect(enterEdit).toHaveBeenCalledWith(true); + }); + + it('reruns an unchanged user request with its original text', () => { + const enterEdit = jest.fn(); + const setSiblingIdx = jest.fn(); + const userContent = [ + { type: ContentTypes.TEXT, text: parentMessage.text }, + ] as TMessageContentParts[]; + + render( + null} + />, + ); + + fireEvent.click(screen.getByRole('button', { name: 'com_ui_rerun' })); + + expect(mockAsk).toHaveBeenCalledWith( + expect.objectContaining({ + text: 'Check the service', + conversationId: parentMessage.conversationId, + }), + expect.objectContaining({ overrideFiles: parentMessage.files }), + ); + expect(setSiblingIdx).toHaveBeenCalledWith(-1); + expect(enterEdit).toHaveBeenCalledWith(true); + }); + + it('names the rerun after the edit only once a part differs', () => { + render( + null} + />, + ); + + expect(screen.getByRole('button', { name: 'com_ui_rerun' })).toBeEnabled(); + + fireEvent.change(screen.getByRole('textbox'), { target: { value: 'Changed response' } }); + + expect(screen.queryByRole('button', { name: 'com_ui_rerun' })).toBeNull(); + expect(screen.getByRole('button', { name: 'com_ui_update_rerun' })).toBeEnabled(); + }); + it('requires multi-part assistant edits to be saved before rerunning', () => { const multiPartContent = [ { type: ContentTypes.TEXT, text: 'First response' }, diff --git a/client/src/components/Chat/Messages/Content/__tests__/EditMessage.spec.tsx b/client/src/components/Chat/Messages/Content/__tests__/EditMessage.spec.tsx index f0db525695..a6ba8ea908 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/EditMessage.spec.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/EditMessage.spec.tsx @@ -63,6 +63,14 @@ const assistantMessage = { text: 'Original answer', } as TMessage; +const emptyAssistantMessage = { + messageId: 'assistant-2', + parentMessageId: 'user-1', + conversationId: 'conversation-1', + isCreatedByUser: false, + text: '', +} as TMessage; + function renderEditor({ enterEdit = jest.fn(), ask = jest.fn(), @@ -189,6 +197,46 @@ describe('EditMessage', () => { expect(enterEdit).toHaveBeenCalledWith(true); }); + /** The rerun that matters most needs no edit at all: a cancelled response, or a + * backend restarted on different parameters, has to be reissued untouched. */ + it('reruns an unchanged request without needing a cosmetic edit first', async () => { + const user = userEvent.setup(); + const ask = jest.fn(); + const { enterEdit, setSiblingIdx } = renderEditor({ ask }); + + const rerun = screen.getByRole('button', { name: 'com_ui_rerun' }); + expect(rerun).toBeEnabled(); + /** Saving an untouched draft still has nothing to write. */ + expect(screen.getByRole('button', { name: 'com_ui_save' })).toBeDisabled(); + + await user.click(rerun); + + await waitFor(() => + expect(ask).toHaveBeenCalledWith( + expect.objectContaining({ + text: 'Original message', + parentMessageId: message.parentMessageId, + conversationId: message.conversationId, + }), + expect.objectContaining({ overrideFiles: message.files }), + ), + ); + expect(setSiblingIdx).toHaveBeenCalledWith(-1); + expect(enterEdit).toHaveBeenCalledWith(true); + }); + + it('names the rerun after the edit only once the draft differs', async () => { + const user = userEvent.setup(); + renderEditor(); + + expect(screen.getByRole('button', { name: 'com_ui_rerun' })).toBeInTheDocument(); + + await user.type(screen.getByTestId('message-text-editor'), ' again'); + + expect(screen.queryByRole('button', { name: 'com_ui_rerun' })).toBeNull(); + expect(screen.getByRole('button', { name: 'com_ui_update_rerun' })).toBeEnabled(); + }); + it('keeps the editor open with the draft when a rerun is refused mid-stream', async () => { const user = userEvent.setup(); const ask = jest.fn().mockReturnValue(false); @@ -212,11 +260,69 @@ describe('EditMessage', () => { await user.clear(screen.getByTestId('message-text-editor')); await user.type(screen.getByTestId('message-text-editor'), 'Refused answer edit'); - await user.click(screen.getByRole('button', { name: 'com_ui_update_rerun' })); + await user.click(screen.getByRole('button', { name: 'com_ui_rerun' })); await waitFor(() => expect(ask).toHaveBeenCalled()); expect(screen.getByTestId('message-text-editor')).toHaveValue('Refused answer edit'); expect(setSiblingIdx).not.toHaveBeenCalled(); expect(enterEdit).not.toHaveBeenCalled(); }); + + /** An answer's draft never reaches the submission, so the button must not offer to + * update it, the status slot has to say the edit is about to be dropped, and the + * submission has to be the plain regeneration the hover action sends. */ + it('reruns an assistant response as a regeneration of that response', async () => { + const user = userEvent.setup(); + mockGetMessages.mockReturnValue([message, assistantMessage]); + const ask = jest.fn(); + const { enterEdit, setSiblingIdx } = renderEditor({ ask, editedMessage: assistantMessage }); + + await user.clear(screen.getByTestId('message-text-editor')); + await user.type(screen.getByTestId('message-text-editor'), 'An edited answer'); + + expect(screen.queryByRole('button', { name: 'com_ui_update_rerun' })).toBeNull(); + expect(screen.getByText('com_ui_rerun_discards_changes')).toBeInTheDocument(); + + await user.click(screen.getByRole('button', { name: 'com_ui_rerun' })); + + await waitFor(() => expect(ask).toHaveBeenCalled()); + expect(ask).toHaveBeenCalledWith( + expect.objectContaining({ messageId: message.messageId }), + expect.objectContaining({ + isRegenerate: true, + /** Names this answer, so an older sibling's rerun cannot prune the newest + * answer's subtree out of the optimistic thread. */ + targetResponseMessageId: assistantMessage.messageId, + }), + ); + const [, options] = ask.mock.calls[0] as [unknown, Record]; + /** Edit-resubmission options would replace this row in place instead. */ + expect(options).not.toHaveProperty('editedMessageId'); + expect(options).not.toHaveProperty('isEdited'); + expect(options).not.toHaveProperty('editedText'); + /** The new answer is a sibling of this one, not of the user turn. */ + expect(setSiblingIdx).not.toHaveBeenCalled(); + expect(enterEdit).toHaveBeenCalledWith(true); + }); + + /** A response cancelled before its first token is exactly what needs rerunning, and + * the form marks text required so Save cannot blank a message. Routing the rerun + * through that validation left the enabled button inert. */ + it('reruns an answer that was cancelled before any text arrived', async () => { + const user = userEvent.setup(); + mockGetMessages.mockReturnValue([message, emptyAssistantMessage]); + const ask = jest.fn(); + const { enterEdit } = renderEditor({ ask, editedMessage: emptyAssistantMessage }); + + const rerun = screen.getByRole('button', { name: 'com_ui_rerun' }); + expect(rerun).toBeEnabled(); + await user.click(rerun); + + await waitFor(() => expect(ask).toHaveBeenCalled()); + expect(ask).toHaveBeenCalledWith( + expect.objectContaining({ messageId: message.messageId }), + expect.objectContaining({ targetResponseMessageId: emptyAssistantMessage.messageId }), + ); + expect(enterEdit).toHaveBeenCalledWith(true); + }); }); diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index 1843c97b12..65737a336c 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -1828,6 +1828,8 @@ "com_ui_rename_conversation": "Rename Conversation", "com_ui_rename_failed": "Failed to rename conversation", "com_ui_requires_auth": "Requires Authentication", + "com_ui_rerun": "Rerun", + "com_ui_rerun_discards_changes": "Rerunning discards these changes and generates a new response. Save to keep them.", "com_ui_reset": "Reset", "com_ui_reset_adjustments": "Reset adjustments", "com_ui_reset_var": "Reset {{0}}", diff --git a/e2e/specs/messages.spec.ts b/e2e/specs/messages.spec.ts index f986d1f0e4..caab48b46c 100644 --- a/e2e/specs/messages.spec.ts +++ b/e2e/specs/messages.spec.ts @@ -96,11 +96,12 @@ test.describe('Messaging suite', () => { const updatedTextElement = page.getByText(editText); expect(updatedTextElement).toBeTruthy(); - // Check edit response + // Check edit response. Nothing is typed into the editor, so the submit button reads + // "Rerun": reissuing an untouched request is a supported action, not a disabled one. await page.getByRole('button', { name: 'edit' }).click(); const editResponsePromise = [ page.waitForResponse(waitForServerStream), - await page.getByRole('button', { name: 'Update & rerun' }).click(), + await page.getByRole('button', { name: 'Rerun', exact: true }).click(), ]; const [editResponse] = (await Promise.all(editResponsePromise)) as [Response]; diff --git a/e2e/specs/mock/message-tree.spec.ts b/e2e/specs/mock/message-tree.spec.ts index 922dda45a3..5c412cb869 100644 --- a/e2e/specs/mock/message-tree.spec.ts +++ b/e2e/specs/mock/message-tree.spec.ts @@ -1089,6 +1089,45 @@ test.describe('message tree stream operations', () => { await expectVisibleMessages(page, [editedMiddlePrompt, editedMiddleReply, afterEditReply]); }); + /** Regression: the editor's submit button was disabled until the draft differed, so + * reissuing a cancelled request or one that failed on a since-restarted backend meant + * typing a throwaway character first. An untouched draft reruns as-is. */ + test('reruns an untouched user request from the editor', async ({ page }) => { + const label = uniqueLabel('rerun-untouched'); + const prompt = countedPrompt(label); + const firstReply = countedReplyText(label, 1); + const secondReply = countedReplyText(label, 2); + + await openMockChat(page); + await sendAndExpectReply(page, prompt, firstReply); + const conversationId = await conversationIdFromPage(page); + + await clickMessageTitleButton(page, prompt, 'Edit'); + const editor = page.getByTestId('message-text-editor'); + await expect(editor).toBeVisible(); + await expect(editor).toHaveValue(prompt); + + const rerun = page.getByRole('button', { name: 'Rerun', exact: true }); + await expect(rerun).toBeEnabled(); + /** Nothing to save, so that button stays out of reach; the rerun does not. */ + await expect(page.getByRole('button', { name: 'Save', exact: true })).toBeDisabled(); + await waitForGenerationStart(page, () => rerun.click()); + + await expect(messagesView(page).getByText(secondReply)).toBeVisible({ timeout: 30000 }); + + const messages = await waitForMessages( + page, + conversationId, + (items) => items.some((message) => messageText(message).includes(secondReply)), + 'rerun of an untouched request', + ); + /** Reissued verbatim: a second user turn carrying exactly the original text. */ + const reissued = messages.filter( + (message) => message.isCreatedByUser === true && messageText(message) === prompt, + ); + expect(reissued).toHaveLength(2); + }); + test('error responses remain valid parents for follow-ups', async ({ page }) => { const label = uniqueLabel('error'); const basePrompt = replyPrompt(`${label}-base`);