fix: address chat composer review findings

This commit is contained in:
Marco Beretta 2026-07-31 16:34:34 +02:00
parent fb49416864
commit ed3b2f858d
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
21 changed files with 753 additions and 129 deletions

View file

@ -0,0 +1,110 @@
import React from 'react';
import { RecoilRoot } from 'recoil';
import { render } from '@testing-library/react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import BadgeRowProvider, { useBadgeRowContext } from '../BadgeRowContext';
/**
* `BadgeRowProvider` is an inline child of `ChatForm`, which re-renders on every
* keystroke in the composer. Its context value is memoized for exactly that
* reason but a memo is only worth as much as its dependencies, and two of them
* were hooks handing back a fresh object literal every render. The value changed
* on each character typed, and every consumer of it rebuilt: the palette's whole
* tool, skill and MCP server catalog, per keystroke.
*/
/* Held outside the factory so it is one function for the whole test, the way
the real `ToastProvider` sits above the composer and does not re-render with
it. A fresh `showToast` per render would be the mock's instability, not the
provider's. */
const mockShowToast = jest.fn();
jest.mock('@librechat/client', () => ({
useToastContext: () => ({ showToast: mockShowToast }),
}));
/* The suite-wide `react-i18next` stub builds a new `t` on every render, which
the real one does not: it holds `t` in state and replaces it only when the
language or namespace changes. Left as-is, every `useLocalize` consumer here
would look unstable for a reason that does not exist outside the tests. */
const mockTranslate = (key: string) => key;
const mockToolAuth = { authenticated: false };
jest.mock('react-i18next', () => ({
...jest.requireActual('react-i18next'),
useTranslation: () => ({ t: mockTranslate, i18n: { changeLanguage: jest.fn() } }),
}));
/* Startup config and the server catalog are external HTTP queries. Their
contents do not matter to this identity test, and leaving them live lets a
rejected `/api/config` request report after Jest has torn the test down. */
jest.mock('~/data-provider', () => ({
...jest.requireActual('~/data-provider'),
useGetEndpointsQuery: () => ({ data: undefined }),
useGetStartupConfig: () => ({ data: undefined }),
useMCPServersQuery: () => ({ data: undefined, isLoading: false }),
useVerifyAgentToolAuth: () => ({ data: mockToolAuth }),
}));
function renderProvider() {
const client = new QueryClient({
defaultOptions: { queries: { retry: false, cacheTime: 0 } },
});
let value: ReturnType<typeof useBadgeRowContext>;
const Consumer = () => {
value = useBadgeRowContext();
return null;
};
const Tree = ({ marker }: { marker: number }) => (
<QueryClientProvider client={client}>
<RecoilRoot>
<BadgeRowProvider conversationId="convo-badge-row">
{/* Re-rendered along with the provider, the way `Bar` is. */}
<span>{marker}</span>
<Consumer />
</BadgeRowProvider>
</RecoilRoot>
</QueryClientProvider>
);
const { rerender } = render(<Tree marker={0} />);
return {
getValue: () => value,
retype: (marker: number) => rerender(<Tree marker={marker} />),
};
}
describe('BadgeRowProvider', () => {
it('hands consumers the same value across a parent re-render', () => {
const { getValue, retype } = renderProvider();
const first = getValue();
retype(1);
expect(getValue()).toBe(first);
retype(2);
expect(getValue()).toBe(first);
});
/* The two that were rebuilding: both are carried into the context value
whole, so an unstable reference from either defeats the memo by itself. */
it('keeps the search form and MCP manager referentially stable', () => {
const { getValue, retype } = renderProvider();
const first = getValue();
retype(1);
const last = getValue();
expect(last?.searchApiKeyForm).toBe(first?.searchApiKeyForm);
expect(last?.mcpServerManager).toBe(first?.mcpServerManager);
});
/* Without this the cases above would still pass against a provider that had
stopped providing: every value would be `undefined`, and identical. */
it('is undefined outside the provider, and an object inside it', () => {
let outside: unknown = 'unset';
const Consumer = () => {
outside = useBadgeRowContext();
return null;
};
render(<Consumer />);
expect(outside).toBeUndefined();
const { getValue } = renderProvider();
expect(getValue()).toEqual(expect.objectContaining({ conversationId: 'convo-badge-row' }));
});
});

View file

@ -4,7 +4,7 @@ import { TextareaAutosize } from '@librechat/client';
import { useRecoilState, useRecoilValue } from 'recoil';
import { Constants, isAssistantsEndpoint, isAgentsEndpoint } from 'librechat-data-provider';
import type { TMessage, TConversation } from 'librechat-data-provider';
import type { ExtendedFile, FileSetter, ConvoGenerator } from '~/common';
import type { ExtendedFile, FileSetter, ConvoGenerator, TAskFunction } from '~/common';
import type { QueuedMessageContext } from '~/hooks/Chat/useSteering';
import {
useTextarea,
@ -295,7 +295,27 @@ const ChatForm = memo(function ChatForm({
const composerItems = useComposerItems(conversationId, quotesEnabled);
const attachTarget = useAttachTarget(conversation, disableInputs);
const dictation = useDictation({ ask: submitMessage, methods, isSubmitting });
const { active: answerModeActive, submitText: submitAnswerText } = answerMode;
/** The same gate `onSubmit` applies: while a question pause is live the
* composer IS the answer box, so a dictated turn has to answer it rather
* than start a turn the paused run would drop. */
const dictationAsk = useCallback<TAskFunction>(
(props) => {
if (answerModeActive && submitAnswerText(props.text)) {
return;
}
return submitMessage({ text: props.text });
},
[answerModeActive, submitAnswerText, submitMessage],
);
const dictation = useDictation({
ask: dictationAsk,
methods,
/* Answer mode leaves the run submitting while handing the composer over,
which is exactly when speech must still reach it the send button is
enabled on the same terms. */
isSubmitting: isSubmitting && !answerModeActive,
});
const uploadingCount = useMemo(() => {
let count = 0;
for (const file of files.values()) {
@ -561,7 +581,12 @@ const ChatForm = memo(function ChatForm({
isRTL={isRTL}
disabled={disableInputs}
agentId={conversation?.agent_id}
endpoint={endpoint}
/* The RAW endpoint, not the effective type above: the attach
destinations resolve the provider from its name, so a
custom endpoint reduced to `custom` loses the uploads its
provider actually takes (OpenRouter's video and audio).
`endpointType` beside it carries the resolved type. */
endpoint={conversation?.endpoint}
endpointType={attachTarget.endpointType}
endpointFileConfig={attachTarget.endpointFileConfig}
useResponsesApi={attachTarget.useResponsesApi}

View file

@ -37,11 +37,12 @@ export type RestoreToComposer = (
interface QueueProps {
steering: SteeringControls;
conversationId: string;
/** Returns whether the composer took the words; see `editToComposer`. */
onEditToComposer: (
text: string,
files?: TMessage['files'],
context?: QueuedMessageContext,
) => void;
) => boolean;
onRestoreToComposer: RestoreToComposer;
}
@ -217,11 +218,21 @@ function QueueRow({
type="button"
aria-label={localize('com_ui_edit_message')}
onClick={() => {
steering.removeQueued(message.id);
onEditToComposer(message.text, message.files, {
/* Same order as the trash below: dropped only once the words are
somewhere else. A paused question owns the composer, and removing
the row anyway would leave the message nowhere at all. */
const taken = onEditToComposer(message.text, message.files, {
quotes: message.quotes,
manualSkills: message.manualSkills,
});
if (taken) {
steering.removeQueued(message.id);
return;
}
showToast({
message: localize('com_ui_queue_edit_blocked'),
status: 'warning',
});
}}
className={ICON_BTN}
>

View file

@ -198,7 +198,7 @@ describe('Queue', () => {
});
it('hands the whole message to the composer to edit', () => {
const onEdit = jest.fn();
const onEdit = jest.fn().mockReturnValue(true);
renderQueue([queued({ id: 'q1', quotes: ['a quote'], manualSkills: ['writer'] })], steering, {
onEditToComposer: onEdit,
});
@ -210,6 +210,20 @@ describe('Queue', () => {
expect(mockRemoveQueued).toHaveBeenCalledWith('q1');
});
/* Edit used to drop the row first and hand the words over second, so a
composer that refuses a paused question owns it destroyed the message
outright. Same restore-then-remove order as the trash. */
it('keeps the message queued when the composer refuses to take it for editing', () => {
const onEdit = jest.fn().mockReturnValue(false);
renderQueue([queued({ id: 'q1' })], steering, { onEditToComposer: onEdit });
fireEvent.click(screen.getByLabelText('com_ui_edit_message'));
expect(onEdit).toHaveBeenCalled();
expect(mockRemoveQueued).not.toHaveBeenCalled();
expect(mockShowToast).toHaveBeenCalledWith(
expect.objectContaining({ message: 'com_ui_queue_edit_blocked' }),
);
});
/* The region is removed with the rail and re-inserted with its old text
still in it, which readers announce on insertion. */
it('forgets its last announcement once the queue empties', () => {
@ -244,6 +258,70 @@ describe('Queue', () => {
expect(screen.getByRole('status')).toHaveTextContent('');
});
/* The rows move as the pointer crosses them, so the queue has already changed
by the time a drag ends. Only a drag that never landed anywhere puts it
back and `didDrop` reports a landing even though the rows declare no
`drop` handler, which is what makes the plain `hover` sortable work. */
describe('drag reordering', () => {
/* The handle only drags on a hover-capable pointer, and the suite's
`matchMedia` answers `false` to everything which is the touch device
the rail deliberately refuses to drag on. */
const realMatchMedia = window.matchMedia;
beforeEach(() => {
window.matchMedia = ((query: string) =>
({
matches: query === '(hover: hover)',
media: query,
onchange: null,
addListener: jest.fn(),
removeListener: jest.fn(),
addEventListener: jest.fn(),
removeEventListener: jest.fn(),
dispatchEvent: jest.fn(),
}) as unknown as MediaQueryList) as typeof window.matchMedia;
});
afterEach(() => {
window.matchMedia = realMatchMedia;
});
/* jsdom has no DataTransfer, and the HTML5 backend reads one off every
event it handles. */
const dataTransfer = () => ({
dropEffect: 'move',
effectAllowed: 'move',
files: [],
items: [],
types: [],
setData: () => undefined,
getData: () => '',
setDragImage: () => undefined,
});
const dragFirstRowOntoSecond = (drop: boolean) => {
renderQueue([queued({ id: 'q1' }), queued({ id: 'q2' })]);
const grip = screen.getAllByTestId('queued-message-grip')[0];
const secondRow = screen.getAllByTestId('queued-message-row')[1];
const dt = dataTransfer();
fireEvent.dragStart(grip, { dataTransfer: dt });
fireEvent.dragOver(secondRow, { dataTransfer: dt });
if (drop) {
fireEvent.drop(secondRow, { dataTransfer: dt });
}
fireEvent.dragEnd(grip, { dataTransfer: dt });
};
it('keeps the new order when the drag is dropped on the rail', () => {
dragFirstRowOntoSecond(true);
expect(mockRestoreQueuedOrder).not.toHaveBeenCalled();
});
it('puts the order back when the drag is abandoned', () => {
dragFirstRowOntoSecond(false);
expect(mockRestoreQueuedOrder).toHaveBeenCalledWith(['q1', 'q2']);
});
});
it('shows an attachment count when files ride along', () => {
renderQueue([queued({ files: [{ file_id: 'f1' }, { file_id: 'f2' }] as never })]);
const attachmentLabel = screen.getByText('com_ui_attachment_count:2');

View file

@ -1,6 +1,9 @@
/**
* @jest-environment jsdom
*/
import { fireEvent, render, screen } from '@testing-library/react';
import type { ReactNode } from 'react';
import type { FileConfigInput } from 'librechat-data-provider';
import type { ReactNode } from 'react';
import UploadSkillDialog from '../UploadSkillDialog';
const mockMutate = jest.fn();
@ -71,8 +74,8 @@ jest.mock('~/utils', () => ({
}));
function getFileInput(container: HTMLElement): HTMLInputElement {
const input = container.querySelector('input[type="file"]');
if (!(input instanceof HTMLInputElement)) {
const input = container.querySelector<HTMLInputElement>('input[type="file"]');
if (input == null) {
throw new Error('Upload input was not rendered');
}
return input;
@ -113,9 +116,7 @@ describe('UploadSkillDialog', () => {
});
fireEvent.change(getFileInput(container), {
target: {
files: [file],
},
target: { files: [file] },
});
expect(mockMutate).not.toHaveBeenCalled();
@ -133,9 +134,7 @@ describe('UploadSkillDialog', () => {
});
fireEvent.change(getFileInput(container), {
target: {
files: [file],
},
target: { files: [file] },
});
expect(mockShowToast).not.toHaveBeenCalled();
@ -152,9 +151,7 @@ describe('UploadSkillDialog', () => {
});
fireEvent.change(getFileInput(container), {
target: {
files: [file],
},
target: { files: [file] },
});
expect(mockShowToast).not.toHaveBeenCalled();

View file

@ -210,6 +210,135 @@ describe('useSteerRecovery', () => {
expect(queue).toEqual([]);
});
/* Navigating to another chat swaps the conversation held in the index slot,
so the run this steer belongs to is in no slot at all. Absence is not an
ending: the server is still injecting the accepted steer, and reading the
empty scan as "run over" queued the same words a second time. */
it('leaves the ack pending when the user has navigated to another chat', async () => {
let settle: (value: unknown) => void = () => undefined;
mockMutateAsync.mockReturnValue(new Promise((resolve) => (settle = resolve)));
let recovery: ReturnType<typeof useSteerRecovery> | undefined;
let navigateAway: (() => void) | undefined;
let chips: unknown[] = [];
let queue: unknown[] = [];
const Tree = () => {
recovery = useSteerRecovery(CONVO_ID);
chips = useRecoilValue(store.pendingSteersByConvoId(CONVO_ID));
queue = useRecoilValue(store.queuedMessagesByConvoId(CONVO_ID));
const setConvo = useSetRecoilState(store.conversationByIndex(0));
navigateAway = () => setConvo({ conversationId: 'a-different-chat' } as never);
return null;
};
render(
<RecoilRoot
initializeState={(snapshot) => {
seedRun(snapshot, true);
snapshot.set(store.pendingSteersByConvoId(CONVO_ID), [
{ steerId: 'local-nav', text: 'still running', status: 'failed', createdAt: 2 },
]);
}}
>
<Tree />
</RecoilRoot>,
);
act(() => {
recovery?.retry('local-nav');
});
act(() => navigateAway?.());
await act(async () => {
settle({ steerId: 'srv-nav', status: 'queued', position: 1, conversationId: CONVO_ID });
});
expect(chips).toEqual([expect.objectContaining({ steerId: 'srv-nav', status: 'pending' })]);
expect(queue).toEqual([]);
});
/* An interrupt-steer that failed has to retry AS an interrupt: resent as an
ordinary steer it lands at the run's next tool step instead of sealing the
stream, which is not the action the chip says it is. */
it('resubmits a failed interrupt-steer as an interrupt', async () => {
mockMutateAsync.mockResolvedValue({
steerId: 'srv-preempt',
status: 'queued',
position: 1,
conversationId: CONVO_ID,
preempt: true,
});
const { result } = setup(({ set }) => {
set(store.pendingSteersByConvoId(CONVO_ID), [
{
steerId: 'local-preempt',
text: 'stop and read this',
status: 'failed',
createdAt: 6,
preempt: true,
},
]);
});
act(() => {
result.current.recovery.retry('local-preempt');
});
await flush();
expect(mockMutateAsync).toHaveBeenCalledWith(
expect.objectContaining({ text: 'stop and read this', preempt: true }),
);
expect(result.current.chips).toEqual([
expect.objectContaining({ steerId: 'srv-preempt', preempt: true }),
]);
});
it('leaves an ordinary steer non-preempting on retry', async () => {
mockMutateAsync.mockResolvedValue({
steerId: 'srv-plain',
status: 'queued',
position: 1,
conversationId: CONVO_ID,
});
const { result } = setup(({ set }) => {
set(store.pendingSteersByConvoId(CONVO_ID), [
{ steerId: 'local-plain', text: 'just a steer', status: 'failed', createdAt: 6 },
]);
});
act(() => {
result.current.recovery.retry('local-plain');
});
await flush();
expect(mockMutateAsync).toHaveBeenCalledWith(expect.not.objectContaining({ preempt: true }));
});
/* Capability degradation is a relabel, never an error: a server without the
seal still queues the steer and echoes `preempt: false`. */
it('follows the server down to an ordinary steer when the seal was not armed', async () => {
mockMutateAsync.mockResolvedValue({
steerId: 'srv-degraded',
status: 'queued',
position: 1,
conversationId: CONVO_ID,
preempt: false,
});
const { result } = setup(({ set }) => {
set(store.pendingSteersByConvoId(CONVO_ID), [
{
steerId: 'local-degraded',
text: 'seal it',
status: 'failed',
createdAt: 6,
preempt: true,
},
]);
});
act(() => {
result.current.recovery.retry('local-degraded');
});
await flush();
expect(result.current.chips).toEqual([
expect.objectContaining({ steerId: 'srv-degraded', status: 'pending', preempt: false }),
]);
});
/* The picks the message was written with have to survive the retry, or the
words are re-sent without the quotes and skills they referred to. */
it('carries the quotes and skills the steer was written with', async () => {

View file

@ -305,19 +305,13 @@ describe('useSteering', () => {
);
});
it('retry resubmits a failed interrupt-steer AS an interrupt', () => {
const { result } = setup();
act(() => {
result.current.retrySteer('chip-1', 'retry me', undefined, undefined, { preempt: true });
});
expect(mockMutate).toHaveBeenCalledWith(
expect.objectContaining({ text: 'retry me', preempt: true }),
expect.anything(),
);
});
/* Retry lives in `useSteerRecovery` now, which owns the chip actions the
thread's pending block renders; its own spec covers the preempt carry. */
it('an ordinary steer does not preempt by default', () => {
const { result } = setup();
const { result } = setup({}, ({ set }) => {
set(store.duringRunDefaultAction, 'steer');
});
act(() => {
result.current.submitDuringRun('just steer');
});
@ -329,6 +323,7 @@ describe('useSteering', () => {
it('steerInterruptsByDefault makes the default Enter route preempt', () => {
const { result } = setup({}, ({ set }) => {
set(store.duringRunDefaultAction, 'steer');
set(store.steerInterruptsByDefault, true);
});
act(() => {

View file

@ -32,7 +32,10 @@ export default function useSteerRecovery(conversationId: string) {
}
return snapshot.getLoadable(store.isSubmittingFamily(key)).getValue() !== true;
}
return true;
/* Held by no slot at all: the user navigated to another chat, which says
nothing about the run they left behind. Reading that as the end is the
same mistake as reading the unmount as the end, one step further out. */
return false;
},
[conversationId],
);
@ -97,9 +100,26 @@ export default function useSteerRecovery(conversationId: string) {
run ends, which is exactly when a retry's ack tends to land: those
callbacks never ran, and the chip was left saying `sending` for the
rest of the conversation with the words neither sent nor queued. */
steerMessage({ conversationId, text: steer.text, files: steer.files })
/* `preempt` rides along: a chip that failed as an interrupt-steer has to
retry AS one. Resent without it the words land at the run's next tool
step instead of sealing the stream, which is a different action than
the one the user asked for and the chip still claims to be. */
steerMessage({
conversationId,
text: steer.text,
files: steer.files,
...(steer.preempt === true && { preempt: true }),
})
.then((response) => {
acknowledgeRetry(steerId, { ...steer, steerId: response.steerId, status: 'pending' });
acknowledgeRetry(steerId, {
...steer,
steerId: response.steerId,
status: 'pending',
/* The server echoes what it actually armed: without the
capability it queues the steer and reports `preempt: false`,
which relabels the chip rather than failing it. */
preempt: response.preempt === true,
});
})
.catch((error: unknown) => {
const code = getSteerErrorCode(error);

View file

@ -352,6 +352,29 @@ describe('useAttachItems', () => {
expect(mockHandleFileChange).toHaveBeenLastCalledWith(expect.anything(), undefined);
});
/* A custom endpoint's own NAME is what identifies its provider; the
resolved `endpointType` flattens every one of them to `custom`. Handing
the palette the type in place of the name cost an OpenRouter endpoint the
video and audio it takes. */
it('reads the provider from a custom endpoint name, not its resolved type', () => {
const acceptFor = (endpoint: string) => {
const { result } = renderAttach({ endpoint, endpointType: EModelEndpoint.custom });
const input = document.createElement('input');
const accepts: string[] = [];
input.click = () => accepts.push(input.accept);
Object.defineProperty(result.current.inputRef, 'current', { value: input, writable: true });
act(() => {
result.current.entries.find((entry) => entry.id === 'local:provider')?.onSelect();
});
return accepts[0];
};
expect(acceptFor('OpenRouter')).toContain('video/');
expect(acceptFor('OpenRouter')).toContain('audio/');
/* The type on its own is a document-and-image provider and nothing more. */
expect(acceptFor(EModelEndpoint.custom)).not.toContain('video/');
});
it('scopes the picker to what the destination can send', () => {
const { result } = renderAttach({ endpointType: EModelEndpoint.bedrock });
const input = document.createElement('input');

View file

@ -15,11 +15,14 @@ const baseState: ComposerHintState = {
enterToSend: true,
};
const hint = (overrides: Partial<ComposerHintState>, isMac = true) =>
composeHint({ ...baseState, ...overrides }, localize, isMac).text;
/** What `useShortcutDisplay('stopGenerating')` resolves to by default on a Mac. */
const STOP = '⌘ ⇧ X';
const hint = (overrides: Partial<ComposerHintState>, isMac = true, stop = STOP) =>
composeHint({ ...baseState, ...overrides }, localize, isMac, stop).text;
const kindOf = (overrides: Partial<ComposerHintState>) =>
composeHint({ ...baseState, ...overrides }, localize, true).kind;
composeHint({ ...baseState, ...overrides }, localize, true, STOP).kind;
describe('composeHint', () => {
it('shows discovery affordances on an untouched composer', () => {
@ -30,8 +33,18 @@ describe('composeHint', () => {
expect(hint({ hasText: true })).toBe('com_ui_composer_hint_typing');
});
it('offers stop while generating with an empty composer', () => {
expect(hint({ isSubmitting: true })).toBe('com_ui_composer_hint_stop');
/* The line used to read "Esc to stop", which no handler anywhere implements:
stopping is bound to the `stopGenerating` shortcut, and the user can rebind
it. Naming a key the composer does not answer to is worse than naming none. */
it('names the live stop binding while generating with an empty composer', () => {
expect(hint({ isSubmitting: true })).toBe('⌘ ⇧ X com_ui_composer_hint_stop');
expect(hint({ isSubmitting: true }, false, 'Ctrl+Shift+X')).toBe(
'Ctrl+Shift+X com_ui_composer_hint_stop',
);
});
it('names no key at all once the binding is cleared', () => {
expect(hint({ isSubmitting: true }, true, '')).toBe('com_ui_composer_hint_running');
});
describe('during a run with text', () => {
@ -64,7 +77,7 @@ describe('composeHint', () => {
it('falls back to stop when the modifiers have no text to act on', () => {
expect(hint({ duringRunActive: true, hasText: false, isSubmitting: true })).toBe(
'com_ui_composer_hint_stop',
'⌘ ⇧ X com_ui_composer_hint_stop',
);
});
});

View file

@ -184,12 +184,42 @@ describe('useComposerRestore', () => {
describe('editToComposer', () => {
it('replaces the draft outright, unlike the guarded restore', () => {
const { result, currentText } = setup({ draft: 'half a thought' });
let taken = false;
act(() => {
result.current.editToComposer('the queued message');
taken = result.current.editToComposer('the queued message');
});
expect(taken).toBe(true);
expect(currentText()).toBe('the queued message');
});
/* The one thing it refuses: while a question pause is live the composer is
the answer box, so a queued message dropped in here reads as a draft and
answers the tool on the next Enter instead of being edited. */
it('refuses while a paused question owns the composer', () => {
const { result, currentText, setFiles } = setup({ answerModeActive: true });
let taken = true;
act(() => {
taken = result.current.editToComposer('the queued message', [
{ file_id: 'f1', filename: 'notes.pdf', filepath: '/f1', type: 'application/pdf' },
]);
});
expect(taken).toBe(false);
expect(currentText()).toBe('');
expect(setFiles).not.toHaveBeenCalled();
});
/* A question can start pausing the run between render and click. */
it('refuses on a pause that started after the last render it was read in', () => {
const { result, rerender, currentText } = setup();
rerender({ conversationId: CONVO_ID, answerModeActive: true });
let taken = true;
act(() => {
taken = result.current.editToComposer('too late');
});
expect(taken).toBe(false);
expect(currentText()).toBe('');
});
it('restores attachments as already-uploaded entries', () => {
const { result, setFiles } = setup();
act(() => {

View file

@ -52,7 +52,11 @@ jest.mock('../../useLocalize', () => ({
const ask = jest.fn(() => true) as unknown as jest.Mock & TAskFunction;
function setup({ autoSendText = -1, draft = '' }: { autoSendText?: number; draft?: string } = {}) {
function setup({
autoSendText = -1,
draft = '',
isSubmitting = false,
}: { autoSendText?: number; draft?: string; isSubmitting?: boolean } = {}) {
let text = draft;
const methods = {
setValue: jest.fn((_name: string, value: string) => {
@ -73,7 +77,7 @@ function setup({ autoSendText = -1, draft = '' }: { autoSendText?: number; draft
useDictation({
ask: ask as unknown as TAskFunction,
methods: methods as never,
isSubmitting: false,
isSubmitting,
}),
{ wrapper },
);
@ -200,4 +204,91 @@ describe('useDictation', () => {
expect(ask).not.toHaveBeenCalled();
expect(currentText()).toBe('my unsent draft');
});
/* A run in flight has nothing to send into, so the take is refused outright.
`ChatForm` reports a paused `ask_user_question` as NOT submitting for
exactly this reason: the run is still open, but the composer has become the
answer box and speech has to reach it the way typing does. */
it('refuses a take while a turn is in flight', async () => {
const { result, rerender, currentText } = setup({ draft: '', isSubmitting: true });
act(() => result.current.start());
mockIsListening = true;
act(() => rerender());
act(() => mockSetTextCallback('answer the question'));
act(() => result.current.stopAndSend());
await settle(rerender);
expect(ask).not.toHaveBeenCalled();
expect(currentText()).toBe('answer the question');
});
it('sends the take when the composer is free to submit', async () => {
const { result, rerender } = setup({ draft: '', isSubmitting: false });
act(() => result.current.start());
mockIsListening = true;
act(() => rerender());
act(() => mockSetTextCallback('answer the question'));
act(() => result.current.stopAndSend());
await settle(rerender);
expect(ask).toHaveBeenCalledWith({ text: 'answer the question' });
});
/* The armed send reads whatever the transcript left in the composer. A take
that produced nothing leaves the draft that was there BEFORE recording, and
sending that is the user's own unfinished words going out as if dictated. */
describe('a take that produced nothing', () => {
it('stands the armed send down when the transcription fails', async () => {
mockSpeechEndpoint = 'external';
const { result, rerender, currentText } = setup({ draft: 'half a thought' });
act(() => result.current.start());
mockIsListening = true;
mockIsLoading = true;
act(() => rerender());
act(() => result.current.stopAndSend());
/* The request fails: the engine toasts, clears its loading flag, and
never reports a transcript. */
mockIsLoading = false;
await settle(rerender);
expect(ask).not.toHaveBeenCalled();
expect(currentText()).toBe('half a thought');
});
it('stands it down for a take too short to reach the engine', async () => {
const { result, rerender, currentText } = setup({ draft: 'half a thought' });
act(() => result.current.start());
mockIsListening = true;
act(() => rerender());
act(() => result.current.stopAndSend());
await settle(rerender);
expect(ask).not.toHaveBeenCalled();
expect(currentText()).toBe('half a thought');
});
/* The gate is per take, not for good. */
it('sends the next take that does produce words', async () => {
const { result, rerender } = setup({ draft: 'half a thought' });
act(() => result.current.start());
mockIsListening = true;
act(() => rerender());
act(() => result.current.stopAndSend());
await settle(rerender);
expect(ask).not.toHaveBeenCalled();
act(() => result.current.start());
mockIsListening = true;
act(() => rerender());
act(() => mockSetTextCallback('this time it heard me'));
act(() => result.current.stopAndSend());
await settle(rerender);
expect(ask).toHaveBeenCalledWith({ text: 'half a thought this time it heard me' });
});
});
});

View file

@ -1,4 +1,5 @@
import type { LocalizeFunction } from '~/common';
import { useShortcutDisplay } from '~/hooks/useKeyboardShortcuts';
import { isMacPlatform } from '~/utils/shortcuts';
import useLocalize from '~/hooks/useLocalize';
@ -44,6 +45,9 @@ export function composeHint(
state: ComposerHintState,
localize: LocalizeFunction,
isMac: boolean,
/** The live binding for `stopGenerating`, which the user can rebind or clear
* outright so the stop line is built from it rather than naming a key. */
stopShortcut: string,
): ComposerHint {
if (state.answerModeActive) {
return { text: localize('com_ui_composer_hint_answer'), kind: 'state' };
@ -85,7 +89,15 @@ export function composeHint(
}
if (state.isSubmitting) {
return { text: localize('com_ui_composer_hint_stop'), kind: 'state' };
/* Nothing to advertise when the binding has been cleared: the stop button
is right there, and naming a key that does nothing is worse than saying
only that a reply is running. */
return {
text: stopShortcut
? `${stopShortcut} ${localize('com_ui_composer_hint_stop')}`
: localize('com_ui_composer_hint_running'),
kind: 'state',
};
}
if (state.hasText) {
@ -107,5 +119,6 @@ export function composeHint(
export default function useComposerHint(state: ComposerHintState): ComposerHint {
const localize = useLocalize();
return composeHint(state, localize, isMacPlatform);
const stopShortcut = useShortcutDisplay('stopGenerating');
return composeHint(state, localize, isMacPlatform, stopShortcut);
}

View file

@ -7,9 +7,14 @@ import type { useChatFormContext } from '~/Providers';
import store from '~/store';
export interface ComposerRestore {
/** Chip "Edit message": replaces the draft outright. The caller has already
* decided this is wanted, so nothing is guarded. */
editToComposer: (text: string, files?: TMessage['files'], context?: QueuedMessageContext) => void;
/** Chip "Edit message": replaces the draft outright, since the caller has
* already decided that is wanted. Returns whether the composer took the
* words only a paused question, which owns the box, refuses. */
editToComposer: (
text: string,
files?: TMessage['files'],
context?: QueuedMessageContext,
) => boolean;
/** The same restore for a steer whose reclaim was a round-trip, guarded
* against everything that can change while it is in flight. Returns whether
* the words were taken, so a refusal can be re-homed rather than dropped. */
@ -68,11 +73,25 @@ export default function useComposerRestore({
[conversationId],
);
/** Read at call time rather than captured, so both the synchronous edit below
* and the round-trip restore further down see the pause that started while
* they were in flight. */
const liveAnswerModeRef = useRef(answerModeActive);
liveAnswerModeRef.current = answerModeActive;
/** The text replaces the composer draft and the chip's attachments merge back
* into the composer file map (already uploaded, so they restore as completed
* entries same shape as draft recovery). */
* entries same shape as draft recovery).
*
* A paused `ask_user_question` is the one thing that refuses: `onSubmit`
* hands the composer's text to `answerMode.submitText` before any send
* routing, so a queued message dropped in here would leave the box on
* screen looking like a draft and answer the tool on the next Enter. */
const editToComposer = useCallback(
(text: string, chipFiles?: TMessage['files'], context?: QueuedMessageContext) => {
(text: string, chipFiles?: TMessage['files'], context?: QueuedMessageContext): boolean => {
if (liveAnswerModeRef.current) {
return false;
}
methods.setValue('text', text, { shouldDirty: true });
if (chipFiles != null && chipFiles.length > 0) {
setFiles((prev) => {
@ -98,6 +117,7 @@ export default function useComposerRestore({
}
restoreComposerContext(context);
textAreaRef.current?.focus();
return true;
},
[methods, setFiles, restoreComposerContext, textAreaRef],
);
@ -111,9 +131,6 @@ export default function useComposerRestore({
/** Same reason: attachments staged after the click must be seen. */
const liveFilesRef = useRef(files);
liveFilesRef.current = files;
/** Same reason: the run can pause on `ask_user_question` mid-reclaim. */
const liveAnswerModeRef = useRef(answerModeActive);
liveAnswerModeRef.current = answerModeActive;
/** A reclaim can resolve after the composer unmounts (left the route, closed
* the pane). Its refs still hold the origin chat, so the restore would pass
* its checks and write into a dead form reporting success and making the
@ -160,12 +177,6 @@ export default function useComposerRestore({
if (originConversationId !== liveConversationId) {
return false;
}
/** Answer mode owns the composer: `onSubmit` hands its text to
* `answerMode.submitText` before any send/steer routing, so restoring
* here would turn the steer into the tool's answer on the next Enter. */
if (liveAnswerModeRef.current) {
return false;
}
if (
(methods.getValues('text') ?? '').trim().length > 0 ||
(liveFilesRef.current?.size ?? 0) > 0 ||
@ -173,8 +184,10 @@ export default function useComposerRestore({
) {
return false;
}
editToComposer(text, steerFiles, context);
return true;
/** The answer-mode refusal lives in `editToComposer`: a question can pause
* the run mid-reclaim, and restoring into the answer box would turn the
* steer into the tool's answer on the next Enter. */
return editToComposer(text, steerFiles, context);
},
[methods, editToComposer, hasStagedContext],
);

View file

@ -68,6 +68,11 @@ export default function useDictation({
* auto-send callback can reach the same transcript, and whichever gets
* there first is the one that spends it. */
const spentRef = useRef(false);
/** Whether this take produced any words at all. A transcription that fails,
* or a take too short to reach the engine, reports nothing back and leaves
* the composer holding the draft that was there before recording which an
* armed stop-and-send would then send as if it had been dictated. */
const heardRef = useRef(false);
const submit = useCallback(
(text: string) => {
@ -101,6 +106,10 @@ export default function useDictation({
(text: string) => {
const mode = modeRef.current;
if (text) {
heardRef.current = true;
}
if (mode === 'cancel') {
/* The draft stays on the ref until a take is actually spent: an
external transcription already in flight cannot be recalled, and
@ -134,6 +143,9 @@ export default function useDictation({
if (modeRef.current === 'cancel') {
return;
}
if (text) {
heardRef.current = true;
}
setValue('text', existingTextRef.current ? `${existingTextRef.current} ${text}` : text, {
shouldValidate: true,
});
@ -190,12 +202,19 @@ export default function useDictation({
return;
}
setPendingSend(false);
/* Nothing was heard: the request failed, or the take never reached the
engine. The engines report that themselves, so the send simply stands
down rather than sending the pre-recording draft in its place. */
if (!heardRef.current) {
return;
}
submit(getValues('text') || '');
}, [pendingSend, active, isLoading, settling, submit, getValues]);
const start = useCallback(() => {
modeRef.current = 'compose';
spentRef.current = false;
heardRef.current = false;
setPendingSend(false);
existingTextRef.current = getValues('text') || '';
startRecording();

View file

@ -114,10 +114,15 @@ export function useMCPServerManager({
[],
);
const reinitializeMutation = useReinitializeMCPServerMutation();
const cancelOAuthMutation = useCancelMCPOAuthMutation();
/* Destructured to the callables: react-query hands back a fresh result
object every render, and the callbacks below that depended on the whole
object were new identities each time which is what kept this hook's
return, and `BadgeRowProvider`'s context value with it, changing on every
keystroke in the composer. */
const { mutateAsync: reinitializeServer } = useReinitializeMCPServerMutation();
const { mutate: cancelMCPOAuth } = useCancelMCPOAuthMutation();
const updateUserPluginsMutation = useUpdateUserPluginsMutation({
const { mutate: updateUserPlugins, isLoading: isUpdatingPlugins } = useUpdateUserPluginsMutation({
onSuccess: async (_data, variables) => {
const isRevoke = variables.action === 'uninstall';
const message = isRevoke
@ -350,7 +355,7 @@ export function useMCPServerManager({
* attempt can never be mistaken for this attempt's outcome. */
updateServerInitState(serverName, { isInitializing: true, connectionDeferred: false });
try {
const response = await reinitializeMutation.mutateAsync(serverName);
const response = await reinitializeServer(serverName);
/** Record whether this attempt deferred to a chat turn (request-scoped
* server) so consumers that didn't await this call e.g. the agent
* builder behind the customUserVars config dialog can react to it. */
@ -411,7 +416,7 @@ export function useMCPServerManager({
},
[
updateServerInitState,
reinitializeMutation,
reinitializeServer,
startServerPolling,
queryClient,
showToast,
@ -424,7 +429,7 @@ export function useMCPServerManager({
const cancelOAuthFlow = useCallback(
(serverName: string) => {
cancelOAuthMutation.mutate(serverName, {
cancelMCPOAuth(serverName, {
onSuccess: () => {
cleanupServerState(serverName);
Promise.all([
@ -448,7 +453,7 @@ export function useMCPServerManager({
},
});
},
[queryClient, cleanupServerState, showToast, localize, cancelOAuthMutation],
[queryClient, cleanupServerState, showToast, localize, cancelMCPOAuth],
);
const isInitializing = useCallback(
@ -521,10 +526,10 @@ export function useMCPServerManager({
action: 'install',
auth: authData,
};
updateUserPluginsMutation.mutate(payload);
updateUserPlugins(payload);
}
},
[selectedToolForConfig, updateUserPluginsMutation],
[selectedToolForConfig, updateUserPlugins],
);
const handleConfigRevoke = useCallback(
@ -535,11 +540,11 @@ export function useMCPServerManager({
action: 'uninstall',
auth: {},
};
updateUserPluginsMutation.mutate(payload);
/** Deselection is now handled centrally in updateUserPluginsMutation.onSuccess */
updateUserPlugins(payload);
/** Deselection is now handled centrally in the mutation's onSuccess */
}
},
[selectedToolForConfig, updateUserPluginsMutation],
[selectedToolForConfig, updateUserPlugins],
);
/** Standalone revoke function for OAuth servers - doesn't require selectedToolForConfig */
@ -550,9 +555,9 @@ export function useMCPServerManager({
action: 'uninstall',
auth: {},
};
updateUserPluginsMutation.mutate(payload);
updateUserPlugins(payload);
},
[updateUserPluginsMutation],
[updateUserPlugins],
);
const handleSave = useCallback(
@ -681,7 +686,7 @@ export function useMCPServerManager({
initialValues,
onSave: handleSave,
onRevoke: handleRevoke,
isSubmitting: updateUserPluginsMutation.isLoading,
isSubmitting: isUpdatingPlugins,
};
}, [
selectedToolForConfig,
@ -690,41 +695,77 @@ export function useMCPServerManager({
handleDialogOpenChange,
handleSave,
handleRevoke,
updateUserPluginsMutation.isLoading,
isUpdatingPlugins,
]);
return {
availableMCPServers,
/** MCP servers filtered for chat menu selection (chatMenu !== false && !consumeOnly) */
selectableServers,
availableMCPServersMap: loadedServers,
isLoading,
connectionStatus,
initializeServer,
cancelOAuthFlow,
isInitializing,
isCancellable,
isConnectionDeferred,
resetConnectionDeferred,
getOAuthUrl,
mcpValues,
setMCPValues,
/* Memoized because `BadgeRowProvider` carries this straight into its context
value: a fresh object here changed that value on every keystroke in the
composer, rebuilding the palette's whole server catalog per character. */
return useMemo(
() => ({
availableMCPServers,
/** MCP servers filtered for chat menu selection (chatMenu !== false && !consumeOnly) */
selectableServers,
availableMCPServersMap: loadedServers,
isLoading,
connectionStatus,
initializeServer,
cancelOAuthFlow,
isInitializing,
isCancellable,
isConnectionDeferred,
resetConnectionDeferred,
getOAuthUrl,
mcpValues,
setMCPValues,
isPinned,
setIsPinned,
placeholderText,
toggleServerSelection,
localize,
isPinned,
setIsPinned,
placeholderText,
toggleServerSelection,
localize,
isConfigModalOpen,
handleDialogOpenChange,
selectedToolForConfig,
setSelectedToolForConfig,
handleSave,
handleRevoke,
revokeOAuthForServer,
getServerStatusIconProps,
getConfigDialogProps,
checkEffectivePermission,
};
isConfigModalOpen,
handleDialogOpenChange,
selectedToolForConfig,
setSelectedToolForConfig,
handleSave,
handleRevoke,
revokeOAuthForServer,
getServerStatusIconProps,
getConfigDialogProps,
checkEffectivePermission,
}),
[
availableMCPServers,
selectableServers,
loadedServers,
isLoading,
connectionStatus,
initializeServer,
cancelOAuthFlow,
isInitializing,
isCancellable,
isConnectionDeferred,
resetConnectionDeferred,
getOAuthUrl,
mcpValues,
setMCPValues,
isPinned,
setIsPinned,
placeholderText,
toggleServerSelection,
localize,
isConfigModalOpen,
handleDialogOpenChange,
selectedToolForConfig,
setSelectedToolForConfig,
handleSave,
handleRevoke,
revokeOAuthForServer,
getServerStatusIconProps,
getConfigDialogProps,
checkEffectivePermission,
],
);
}

View file

@ -23,7 +23,11 @@ export type SearchApiKeyFormData = {
const useAuthSearchTool = (options?: { isEntityTool: boolean }) => {
const queryClient = useQueryClient();
const isEntityTool = options?.isEntityTool ?? true;
const updateUserPlugins = useUpdateUserPluginsMutation({
/* `mutate` rather than the mutation object: react-query hands back a fresh
result object on every render, and depending on it made both callbacks
below new identities each time which travelled up through
`useSearchApiKeyForm` into `BadgeRowProvider`'s context value. */
const { mutate: updateUserPlugins } = useUpdateUserPluginsMutation({
onMutate: (vars) => {
queryClient.setQueryData([QueryKeys.toolAuth, Tools.web_search], () => {
return {
@ -69,7 +73,7 @@ const useAuthSearchTool = (options?: { isEntityTool: boolean }) => {
{} as Record<string, string>,
);
updateUserPlugins.mutate({
updateUserPlugins({
pluginKey: Tools.web_search,
action: 'install',
auth,
@ -80,7 +84,7 @@ const useAuthSearchTool = (options?: { isEntityTool: boolean }) => {
);
const removeTool = useCallback(() => {
updateUserPlugins.mutate({
updateUserPlugins({
pluginKey: Tools.web_search,
action: 'uninstall',
auth: {},

View file

@ -1,7 +1,7 @@
import { useRef, useState, useCallback } from 'react';
import { useRef, useMemo, useState, useCallback } from 'react';
import { useForm } from 'react-hook-form';
import useAuthSearchTool from '~/hooks/Plugins/useAuthSearchTool';
import type { SearchApiKeyFormData } from '~/hooks/Plugins/useAuthSearchTool';
import useAuthSearchTool from '~/hooks/Plugins/useAuthSearchTool';
export default function useSearchApiKeyForm({
onSubmit,
@ -33,13 +33,19 @@ export default function useSearchApiKeyForm({
onRevoke?.();
}, [reset, onRevoke, removeTool]);
return {
methods,
isDialogOpen,
setIsDialogOpen,
handleRevokeApiKey,
onSubmit: onSubmitHandler,
badgeTriggerRef,
menuTriggerRef,
};
/* Memoized because `BadgeRowProvider` carries this straight into its context
value: a fresh object here changed that value on every keystroke in the
composer, which is the one thing the provider's own memo exists to stop. */
return useMemo(
() => ({
methods,
isDialogOpen,
setIsDialogOpen,
handleRevokeApiKey,
onSubmit: onSubmitHandler,
badgeTriggerRef,
menuTriggerRef,
}),
[methods, isDialogOpen, handleRevokeApiKey, onSubmitHandler],
);
}

View file

@ -1,5 +1,5 @@
/**
* @jest-environment @happy-dom/jest-environment
* @jest-environment jsdom
*/
import React from 'react';
import { act, render } from '@testing-library/react';
@ -26,8 +26,7 @@ describe('useIsActiveItem', () => {
await act(async () => {
probe.setAttribute('data-active-item', '');
// Allow the MutationObserver microtask to run
await Promise.resolve();
await new Promise((resolve) => setTimeout(resolve, 0));
});
expect(probe.getAttribute('data-active')).toBe('true');
@ -39,13 +38,13 @@ describe('useIsActiveItem', () => {
await act(async () => {
probe.setAttribute('data-active-item', '');
await Promise.resolve();
await new Promise((resolve) => setTimeout(resolve, 0));
});
expect(probe.getAttribute('data-active')).toBe('true');
await act(async () => {
probe.removeAttribute('data-active-item');
await Promise.resolve();
await new Promise((resolve) => setTimeout(resolve, 0));
});
expect(probe.getAttribute('data-active')).toBe('false');
});
@ -56,7 +55,7 @@ describe('useIsActiveItem', () => {
await act(async () => {
probe.setAttribute('data-something-else', 'x');
await Promise.resolve();
await new Promise((resolve) => setTimeout(resolve, 0));
});
expect(probe.getAttribute('data-active')).toBe('false');

View file

@ -289,6 +289,11 @@ export const EDITING_ALLOWED_SHORTCUTS: ReadonlySet<ShortcutActionId> = new Set(
'focusSearch',
'showShortcuts',
'submitMessage',
/* The composer keeps focus across a send, so this is where a user reads the
hint naming it and where they press it. Filtering it out as an editing
chord made the one shortcut the composer advertises the one that did
nothing; it is a no-op whenever no reply is running. */
'stopGenerating',
]);
export type ShortcutAction = ShortcutDefinition & {

View file

@ -1004,11 +1004,12 @@
"com_ui_composer_hint_queue": "queue",
"com_ui_composer_hint_queue_default": "Enter queues",
"com_ui_composer_hint_queue_verb": "queues",
"com_ui_composer_hint_running": "Generating a reply…",
"com_ui_composer_hint_send": "to send",
"com_ui_composer_hint_send_now": "send now",
"com_ui_composer_hint_steer": "Enter steers",
"com_ui_composer_hint_steer_verb": "steers",
"com_ui_composer_hint_stop": "Esc to stop",
"com_ui_composer_hint_stop": "to stop",
"com_ui_composer_hint_typing": "Enter to send · Shift+Enter for newline",
"com_ui_composer_hint_uploading": "Uploading {{count}} files…",
"com_ui_composer_hint_uploading_one": "Uploading {{count}} file…",
@ -1609,6 +1610,7 @@
"com_ui_question_failed_description": "The agent couldn't show this question and may retry automatically.",
"com_ui_question_unanswered": "No answer was given",
"com_ui_queue": "Queue",
"com_ui_queue_edit_blocked": "Finish answering the question before editing a queued message.",
"com_ui_queue_moved": "Moved to {{0}} of {{1}}",
"com_ui_queue_remove_blocked": "Clear the message box in this chat to take this message back.",
"com_ui_queue_reorder": "Reorder message, {{0}} of {{1}}",