From 2c924bf11a64689d28b9ed54b2befd7eeaa0ef56 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:57:10 +0200 Subject: [PATCH] fix: composer review findings across dictation, uploads and steers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dictation only ever submitted through the speech engines' auto-send callback, which they never fire with the default setting: stop-and-send left the transcript sitting in the composer, and a plain stop stopped honouring Auto Send Text at all. The send is now armed on the stop and spent once the take settles, with a per-take guard so the setting and the button cannot both spend it. Cancel also keeps the draft on its ref until a take is really spent, so an external transcription that was already in flight can no longer wipe the draft it just restored. Assistants get their unfiltered picker back — the provider check does not recognise them, so the palette had scoped them to images and dropped PDF support. Memory honours the user's personalization opt-out. Quote chips are gated on the same flag as the quote button, so an endpoint that cannot transmit an excerpt no longer shows one as staged. The upload-file shortcut targets the palette disclosure that replaced the attach menu, and Open Files opens the file manager dialog now that the side panel link is gone. Retried steers resolve "is the run over" from the conversation's submitting state instead of the block's unmount: navigating away from a live run unmounts it too, and queueing there sent the same words twice. Composer hints follow the Enter-to-send setting rather than always naming Enter as the send key. --- client/src/components/Chat/Input/ChatForm.tsx | 2 +- .../components/Chat/Input/Composer/Hints.tsx | 5 +- .../Chat/Input/Composer/Palette.tsx | 3 + client/src/components/Nav/AccountSettings.tsx | 4 +- .../Chat/__tests__/useSteerRecovery.spec.tsx | 55 ++++-- client/src/hooks/Chat/useSteerRecovery.ts | 47 +++-- .../Input/__tests__/useAttachItems.spec.tsx | 16 ++ .../Input/__tests__/useComposerHint.spec.ts | 24 +++ .../Input/__tests__/useDictation.spec.tsx | 180 ++++++++++++++++++ .../__tests__/usePaletteEntries.spec.tsx | 8 + client/src/hooks/Input/useAttachItems.tsx | 20 ++ client/src/hooks/Input/useComposerHint.ts | 37 +++- client/src/hooks/Input/useComposerItems.ts | 13 +- client/src/hooks/Input/useDictation.ts | 88 ++++++--- client/src/hooks/Input/usePaletteEntries.tsx | 7 +- client/src/hooks/useKeyboardShortcuts.ts | 8 +- client/src/locales/en/translation.json | 4 + client/src/store/misc.ts | 8 + 18 files changed, 458 insertions(+), 71 deletions(-) create mode 100644 client/src/hooks/Input/__tests__/useDictation.spec.tsx diff --git a/client/src/components/Chat/Input/ChatForm.tsx b/client/src/components/Chat/Input/ChatForm.tsx index 17b78dff12..6c3825f821 100644 --- a/client/src/components/Chat/Input/ChatForm.tsx +++ b/client/src/components/Chat/Input/ChatForm.tsx @@ -411,7 +411,7 @@ const ChatForm = memo(function ChatForm({ const isMoreThanThreeRows = visualRowCount > 3; - const composerItems = useComposerItems(conversationId); + const composerItems = useComposerItems(conversationId, quotesEnabled); const attachTarget = useAttachTarget(conversation, disableInputs); const dictation = useDictation({ ask: submitMessage, methods, isSubmitting }); const uploadingCount = useMemo(() => { diff --git a/client/src/components/Chat/Input/Composer/Hints.tsx b/client/src/components/Chat/Input/Composer/Hints.tsx index 846cdbfddf..b4a705cbac 100644 --- a/client/src/components/Chat/Input/Composer/Hints.tsx +++ b/client/src/components/Chat/Input/Composer/Hints.tsx @@ -22,8 +22,9 @@ export const COMPOSER_HINT_ID = 'composer-hint'; * here would re-announce on every keystroke as the hint flips between idle and * typing, so the description channel carries it instead. */ -function Hints(state: ComposerHintState) { - const hint = useComposerHint(state); +function Hints(state: Omit) { + const enterToSend = useRecoilValue(store.enterToSend); + const hint = useComposerHint({ ...state, enterToSend }); const showTips = useRecoilValue(store.showComposerTips); const visible = showTips || hint.kind === 'state'; diff --git a/client/src/components/Chat/Input/Composer/Palette.tsx b/client/src/components/Chat/Input/Composer/Palette.tsx index e90b6b05b8..a25f6e5362 100644 --- a/client/src/components/Chat/Input/Composer/Palette.tsx +++ b/client/src/components/Chat/Input/Composer/Palette.tsx @@ -896,6 +896,9 @@ function Palette({ disclosure's own click handler and the popover never opened. */} (null); diff --git a/client/src/hooks/Chat/__tests__/useSteerRecovery.spec.tsx b/client/src/hooks/Chat/__tests__/useSteerRecovery.spec.tsx index d4cb0f95e4..0a46cacdc9 100644 --- a/client/src/hooks/Chat/__tests__/useSteerRecovery.spec.tsx +++ b/client/src/hooks/Chat/__tests__/useSteerRecovery.spec.tsx @@ -1,6 +1,6 @@ import React from 'react'; import { act, render, renderHook } from '@testing-library/react'; -import { RecoilRoot, useRecoilValue, type MutableSnapshot } from 'recoil'; +import { RecoilRoot, useRecoilValue, useSetRecoilState, type MutableSnapshot } from 'recoil'; import useSteerRecovery from '../useSteerRecovery'; import store from '~/store'; @@ -16,9 +16,25 @@ const flush = () => act(async () => undefined); const CONVO_ID = 'convo-steer-recovery'; +/** The hook resolves "is the run over" from the conversation's own submitting + * state, not from its own unmount, so every case has to place the + * conversation in the store the way `ChatView` does. */ +const seedRun = (snapshot: MutableSnapshot, submitting: boolean) => { + snapshot.set(store.conversationKeysAtom, [0]); + snapshot.set(store.conversationByIndex(0), { conversationId: CONVO_ID } as never); + snapshot.set(store.isSubmittingFamily(0), submitting); +}; + function setup(initialize?: (snapshot: MutableSnapshot) => void) { const wrapper = ({ children }: { children: React.ReactNode }) => ( - {children} + { + seedRun(snapshot, true); + initialize?.(snapshot); + }} + > + {children} + ); return renderHook( () => ({ @@ -125,17 +141,15 @@ describe('useSteerRecovery', () => { expect(result.current.queue).toEqual([]); }); - /* The block this hook lives in unmounts the moment the run ends, which is - exactly when a retry's ack tends to land. It has to survive that: the - words go to the queue rather than leaving the chip saying `sending`. */ - /* The block this hook lives in unmounts the moment the run ends, which is - exactly when a retry's ack tends to land. It has to survive that: the - words go to the queue rather than leaving the chip saying `sending`. */ - it('queues a retry whose ack lands after the run ended', async () => { + /* A retry's ack tends to land right as the run ends, which is also when + the block this hook lives in unmounts. Whether the words go to the queue + turns on the run, not on the unmount. */ + const lateAck = async (endRun: boolean) => { let settle: (value: unknown) => void = () => undefined; mockMutateAsync.mockReturnValue(new Promise((resolve) => (settle = resolve))); let recovery: ReturnType | undefined; + let setSubmitting: ((value: boolean) => void) | undefined; let chips: unknown[] = []; let queue: unknown[] = []; const Recovery = () => { @@ -147,12 +161,14 @@ describe('useSteerRecovery', () => { const Observer = () => { chips = useRecoilValue(store.pendingSteersByConvoId(CONVO_ID)); queue = useRecoilValue(store.queuedMessagesByConvoId(CONVO_ID)); + setSubmitting = useSetRecoilState(store.isSubmittingFamily(0)); return null; }; const Tree = ({ live }: { live: boolean }) => ( { - set(store.pendingSteersByConvoId(CONVO_ID), [ + initializeState={(snapshot) => { + seedRun(snapshot, true); + snapshot.set(store.pendingSteersByConvoId(CONVO_ID), [ { steerId: 'local-late', text: 'landed too late', status: 'failed', createdAt: 3 }, ]); }} @@ -168,15 +184,32 @@ describe('useSteerRecovery', () => { }); expect(chips).toEqual([expect.objectContaining({ status: 'sending' })]); + if (endRun) { + act(() => setSubmitting?.(false)); + } rerender(); await act(async () => { settle({ steerId: 'srv-late', status: 'queued', position: 1, conversationId: CONVO_ID }); }); + return { chips, queue }; + }; + + it('queues a retry whose ack lands after the run ended', async () => { + const { chips, queue } = await lateAck(true); expect(chips).toEqual([]); expect(queue).toEqual([expect.objectContaining({ id: 'srv-late', text: 'landed too late' })]); }); + /* Navigating away unmounts the same block while the resumable run carries + on, and the server will still inject the accepted steer. Queueing it as + a follow-up here is what sent the same words twice. */ + it('leaves the ack pending when the block unmounts on a still-live run', async () => { + const { chips, queue } = await lateAck(false); + expect(chips).toEqual([expect.objectContaining({ steerId: 'srv-late', status: 'pending' })]); + expect(queue).toEqual([]); + }); + /* 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 () => { diff --git a/client/src/hooks/Chat/useSteerRecovery.ts b/client/src/hooks/Chat/useSteerRecovery.ts index 2a24f03272..14b37b8a0d 100644 --- a/client/src/hooks/Chat/useSteerRecovery.ts +++ b/client/src/hooks/Chat/useSteerRecovery.ts @@ -1,5 +1,6 @@ -import { useRef, useEffect, useCallback } from 'react'; +import { useCallback } from 'react'; import { useRecoilCallback } from 'recoil'; +import type { Snapshot } from 'recoil'; import type { PendingSteer } from '~/store/families'; import { getSteerErrorCode, resolveAcknowledgedSteer } from '~/hooks/Chat/useSteering'; import useSteerConvert from '~/hooks/Chat/useSteerConvert'; @@ -16,16 +17,24 @@ export default function useSteerRecovery(conversationId: string) { const { mutateAsync: steerMessage } = useSteerMessageMutation(); const convertSteersToQueued = useSteerConvert(); - /** `PendingSteers` only renders while `isLast && isSubmitting` is true, so - * its unmount IS the run ending. A retry's ack can resolve afterward; - * mirrors `composerMountedRef` in `ChatForm`, which guards the equivalent - * race for a reclaimed steer whose restore resolves post-unmount. */ - const mountedRef = useRef(true); - useEffect( - () => () => { - mountedRef.current = false; + /** Whether the run this steer belongs to has finished, read live at ack time. + * Unmounting is not the same signal: `PendingSteers` also unmounts when the + * user navigates away from a run that is still going, and treating that as + * the end queued the accepted steer as a follow-up on top of the injection + * the server was already making. */ + const isRunOver = useCallback( + (snapshot: Snapshot) => { + const keys = snapshot.getLoadable(store.conversationKeysAtom).getValue(); + for (const key of keys) { + const convo = snapshot.getLoadable(store.conversationByIndex(key)).getValue(); + if (convo?.conversationId !== conversationId) { + continue; + } + return snapshot.getLoadable(store.isSubmittingFamily(key)).getValue() !== true; + } + return true; }, - [], + [conversationId], ); const markStatus = useRecoilCallback( @@ -39,10 +48,16 @@ export default function useSteerRecovery(conversationId: string) { ); const acknowledgeRetry = useRecoilCallback( - (cbInterface) => (localId: string, steer: PendingSteer, runOver: boolean) => { - resolveAcknowledgedSteer(cbInterface, conversationId, localId, steer, runOver); + (cbInterface) => (localId: string, steer: PendingSteer) => { + resolveAcknowledgedSteer( + cbInterface, + conversationId, + localId, + steer, + isRunOver(cbInterface.snapshot), + ); }, - [conversationId], + [conversationId, isRunOver], ); /** Routes a steer straight into the queue: reused for a retry that degrades @@ -84,11 +99,7 @@ export default function useSteerRecovery(conversationId: string) { rest of the conversation with the words neither sent nor queued. */ steerMessage({ conversationId, text: steer.text, files: steer.files }) .then((response) => { - acknowledgeRetry( - steerId, - { ...steer, steerId: response.steerId, status: 'pending' }, - !mountedRef.current, - ); + acknowledgeRetry(steerId, { ...steer, steerId: response.steerId, status: 'pending' }); }) .catch((error: unknown) => { const code = getSteerErrorCode(error); diff --git a/client/src/hooks/Input/__tests__/useAttachItems.spec.tsx b/client/src/hooks/Input/__tests__/useAttachItems.spec.tsx index 4c38c3684f..16716abf34 100644 --- a/client/src/hooks/Input/__tests__/useAttachItems.spec.tsx +++ b/client/src/hooks/Input/__tests__/useAttachItems.spec.tsx @@ -186,6 +186,22 @@ describe('useAttachItems', () => { ).toContain('local:image'); }); + /* Assistants carry their own file configuration and the old menu handed + them an unfiltered picker; scoping them to the image capability, which + the provider check would, silently dropped PDF support. */ + it.each([EModelEndpoint.assistants, EModelEndpoint.azureAssistants])( + 'offers one unfiltered upload for %s and no tool destinations', + (endpoint) => { + const ids = renderEntries({ + endpoint, + contextEnabled: true, + fileSearchEnabled: true, + codeEnabled: true, + }); + expect(ids).toEqual(['local:assistants']); + }, + ); + it('reads a provider whatever its casing, which OpenRouter arrives in', () => { expect(renderEntries({ provider: 'OpenRouter' })).toContain('local:provider'); }); diff --git a/client/src/hooks/Input/__tests__/useComposerHint.spec.ts b/client/src/hooks/Input/__tests__/useComposerHint.spec.ts index d723e7b2d1..4cc7f028ac 100644 --- a/client/src/hooks/Input/__tests__/useComposerHint.spec.ts +++ b/client/src/hooks/Input/__tests__/useComposerHint.spec.ts @@ -12,6 +12,7 @@ const baseState: ComposerHintState = { duringRunAction: 'queue' as const, answerModeActive: false, uploadingCount: 0, + enterToSend: true, }; const hint = (overrides: Partial, isMac = true) => @@ -68,6 +69,29 @@ describe('composeHint', () => { }); }); + describe('with Enter bound to a newline', () => { + it('names the chord as the send key while typing', () => { + const result = hint({ hasText: true, enterToSend: false }); + expect(result).toContain('⌘⏎'); + expect(result).toContain('com_ui_composer_hint_send'); + expect(result).toContain('com_ui_composer_hint_newline'); + expect(result).not.toContain('com_ui_composer_hint_typing'); + }); + + it('drops the alternate during-run action, which the chord no longer reaches', () => { + const result = hint({ + duringRunActive: true, + hasText: true, + isSubmitting: true, + duringRunAction: 'steer', + enterToSend: false, + }); + expect(result).toContain('⌘⏎ com_ui_composer_hint_steer_verb'); + expect(result).toContain('com_ui_composer_hint_interrupt'); + expect(result).not.toContain('com_ui_composer_hint_queue'); + }); + }); + describe('kind', () => { it('marks the ambient copy as a tip, so a conversation can drop it', () => { expect(kindOf({})).toBe('tip'); diff --git a/client/src/hooks/Input/__tests__/useDictation.spec.tsx b/client/src/hooks/Input/__tests__/useDictation.spec.tsx new file mode 100644 index 0000000000..b191b1446f --- /dev/null +++ b/client/src/hooks/Input/__tests__/useDictation.spec.tsx @@ -0,0 +1,180 @@ +import React from 'react'; +import { RecoilRoot } from 'recoil'; +import { act, renderHook } from '@testing-library/react'; +import type { TAskFunction } from '~/common'; +import useDictation from '../useDictation'; +import store from '~/store'; + +/** + * The engines only report a completed transcription when Auto Send Text is + * configured, so the composer's own stop-and-send cannot be built on that + * callback alone. These cases pin the three ways a take can be spent — sent on + * request, sent by the setting, or thrown away — against both engines. + */ + +let mockSpeechEndpoint: 'browser' | 'external'; +let mockSetTextCallback: (text: string) => void; +let mockOnTranscriptionComplete: (text: string) => void; +const mockStart = jest.fn(); +const mockStop = jest.fn(); +const mockAbort = jest.fn(); +let mockIsListening: boolean; +let mockIsLoading: boolean; + +jest.mock('../useSpeechToText', () => ({ + __esModule: true, + default: (setText: (text: string) => void, complete: (text: string) => void) => { + mockSetTextCallback = setText; + mockOnTranscriptionComplete = complete; + return { + isListening: mockIsListening, + isLoading: mockIsLoading, + startRecording: mockStart, + stopRecording: mockStop, + abortRecording: mockAbort, + }; + }, +})); + +jest.mock('../useGetAudioSettings', () => ({ + __esModule: true, + default: () => ({ speechToTextEndpoint: mockSpeechEndpoint }), +})); + +jest.mock('@librechat/client', () => ({ + useToastContext: () => ({ showToast: jest.fn() }), +})); + +jest.mock('../../useLocalize', () => ({ + __esModule: true, + default: () => (key: string) => key, +})); + +const ask = jest.fn(() => true) as unknown as jest.Mock & TAskFunction; + +function setup({ autoSendText = -1, draft = '' }: { autoSendText?: number; draft?: string } = {}) { + let text = draft; + const methods = { + setValue: jest.fn((_name: string, value: string) => { + text = value; + }), + reset: jest.fn((values: { text: string }) => { + text = values.text; + }), + getValues: jest.fn(() => text), + }; + const wrapper = ({ children }: { children: React.ReactNode }) => ( + set(store.autoSendText, autoSendText)}> + {children} + + ); + const view = renderHook( + () => + useDictation({ + ask: ask as unknown as TAskFunction, + methods: methods as never, + isSubmitting: false, + }), + { wrapper }, + ); + return { ...view, methods, currentText: () => text }; +} + +/** Ends the take the way an engine does: listening clears, then the settle + * backstop expires. */ +const settle = async (rerender: () => void) => { + mockIsListening = false; + act(() => { + rerender(); + }); + await act(async () => { + jest.advanceTimersByTime(600); + }); +}; + +describe('useDictation', () => { + beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers(); + mockSpeechEndpoint = 'browser'; + mockIsListening = false; + mockIsLoading = false; + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('sends on stop-and-send even with Auto Send Text off, which reports no transcription', async () => { + const { result, rerender, currentText } = setup(); + act(() => result.current.start()); + mockIsListening = true; + act(() => rerender()); + + act(() => mockSetTextCallback('take me somewhere')); + act(() => result.current.stopAndSend()); + await settle(rerender); + + expect(ask).toHaveBeenCalledWith({ text: 'take me somewhere' }); + expect(currentText()).toBe(''); + }); + + it('leaves a plain stop in the composer', async () => { + const { result, rerender, currentText } = setup(); + act(() => result.current.start()); + mockIsListening = true; + act(() => rerender()); + + act(() => mockSetTextCallback('just a draft')); + act(() => result.current.stopToComposer()); + await settle(rerender); + + expect(ask).not.toHaveBeenCalled(); + expect(currentText()).toBe('just a draft'); + }); + + it('still honours Auto Send Text on a plain stop', async () => { + const { result, rerender } = setup({ autoSendText: 0 }); + act(() => result.current.start()); + mockIsListening = true; + act(() => rerender()); + + act(() => result.current.stopToComposer()); + await settle(rerender); + act(() => mockOnTranscriptionComplete('send this for me')); + + expect(ask).toHaveBeenCalledWith({ text: 'send this for me' }); + }); + + it('spends a take once when the setting and the send button both reach it', async () => { + const { result, rerender } = setup({ autoSendText: 0 }); + act(() => result.current.start()); + mockIsListening = true; + act(() => rerender()); + + act(() => mockSetTextCallback('only once')); + act(() => result.current.stopAndSend()); + await settle(rerender); + act(() => mockOnTranscriptionComplete('only once')); + + expect(ask).toHaveBeenCalledTimes(1); + }); + + it('keeps the draft when a cancelled external transcription lands anyway', async () => { + mockSpeechEndpoint = 'external'; + const { result, rerender, currentText } = setup({ draft: 'my unsent draft' }); + act(() => result.current.start()); + mockIsListening = true; + act(() => rerender()); + + act(() => result.current.cancel()); + expect(currentText()).toBe('my unsent draft'); + + /* The mutation was already in flight, so aborting the recorder cannot stop + its success callback from arriving. */ + act(() => mockOnTranscriptionComplete('words nobody asked for')); + + expect(ask).not.toHaveBeenCalled(); + expect(currentText()).toBe('my unsent draft'); + }); +}); diff --git a/client/src/hooks/Input/__tests__/usePaletteEntries.spec.tsx b/client/src/hooks/Input/__tests__/usePaletteEntries.spec.tsx index 493365696b..33fa809662 100644 --- a/client/src/hooks/Input/__tests__/usePaletteEntries.spec.tsx +++ b/client/src/hooks/Input/__tests__/usePaletteEntries.spec.tsx @@ -18,11 +18,13 @@ let mockContext: Record | null; let mockSkills: Array>; let mockAgentsMap: Record>; let mockSkillsActive: boolean; +let mockUser: { personalization?: { memories?: boolean } } | undefined; jest.mock('~/hooks', () => ({ useHasAccess: ({ permissionType }: { permissionType: string }) => mockPermissions[permissionType] ?? false, useHasMemoryAccess: () => mockMemoryAccess, + useAuthContext: () => ({ user: mockUser }), useAgentCapabilities: () => mockCapabilities, /* The real popover filter is used here, and it drops anything the user cannot invoke or that is not active for them. */ @@ -125,6 +127,7 @@ describe('usePaletteEntries', () => { beforeEach(() => { mockPermissions = { ...allPermissions }; mockMemoryAccess = true; + mockUser = { personalization: { memories: true } }; mockCapabilities = { ...allCapabilities }; mockContext = fullContext(); mockSkills = []; @@ -178,6 +181,11 @@ describe('usePaletteEntries', () => { mockMemoryAccess = false; expect(keysOf(entries().result)).not.toContain('builtin:memory'); }); + + it('withholds memory from a user who opted out in personalization', () => { + mockUser = { personalization: { memories: false } }; + expect(keysOf(entries().result)).not.toContain('builtin:memory'); + }); }); describe('on state', () => { diff --git a/client/src/hooks/Input/useAttachItems.tsx b/client/src/hooks/Input/useAttachItems.tsx index f29a527547..0770a046ba 100644 --- a/client/src/hooks/Input/useAttachItems.tsx +++ b/client/src/hooks/Input/useAttachItems.tsx @@ -14,6 +14,7 @@ import { EModelEndpoint, getConfiguredMimeAccept, bedrockDocumentMimeTypes, + isAssistantsEndpoint, defaultAgentCapabilities, bedrockDocumentExtensions, isDocumentSupportedProvider, @@ -201,6 +202,25 @@ export default function useAttachItems({ const build = (onAction: (fileType?: FileUploadType) => void, prefix: string) => { const items: AttachEntry[] = []; + + /* Assistants own their own file handling: whatever the assistant is + configured to accept goes up unfiltered, and none of the tool + destinations below apply. Scoping this to the image capability, as the + provider check would, silently dropped PDF support. */ + if (isAssistantsEndpoint(endpoint)) { + items.push({ + id: `${prefix}:assistants`, + label: localize('com_sidepanel_attach_files'), + primary: true, + icon: