From 337facb4f06161208f63e1910f4d463fd0330f7e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 22 Jul 2026 12:09:53 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=8B=20fix:=20Enable=20Submit=20on=20First?= =?UTF-8?q?=20Tool-Approval=20Decision=20(#14393)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Chat/Messages/Content/ApprovalContext.tsx | 19 +++- .../Content/__tests__/ToolApproval.test.tsx | 105 ++++++++++++++++++ 2 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 client/src/components/Chat/Messages/Content/__tests__/ToolApproval.test.tsx diff --git a/client/src/components/Chat/Messages/Content/ApprovalContext.tsx b/client/src/components/Chat/Messages/Content/ApprovalContext.tsx index cfc301f2bb..7b5bcf9477 100644 --- a/client/src/components/Chat/Messages/Content/ApprovalContext.tsx +++ b/client/src/components/Chat/Messages/Content/ApprovalContext.tsx @@ -50,6 +50,13 @@ export function useAskSubmitStatus(): { } interface ApprovalContextValue { + /** + * Bumped whenever registrations or decisions change. Decisions live in refs + * (synchronous reads), so this is what makes the context value a NEW reference + * on each change; without it, consumers never re-render and never re-read + * `isReady`/`getLeadToolCallId` after an update. + */ + version: number; /** Record (or clear) a card's decision for its tool_call within an action. */ setDecision: ( actionId: string, @@ -88,6 +95,7 @@ export const useApprovalContext = (): ApprovalContextValue => { }; const FALLBACK: ApprovalContextValue = { + version: 0, setDecision: () => undefined, getDecision: () => undefined, getDecisions: () => [], @@ -120,11 +128,14 @@ const isExpiredError = (error: unknown): boolean => { * and the cards only render inside a live chat view where those providers exist. */ export default function ApprovalProvider({ children }: { children: React.ReactNode }) { - /** actionId → (tool_call_id → resolution). Mutable ref + a version bump so - * reads are synchronous for `isReady`/submit while renders stay cheap. */ + /** actionId → (tool_call_id → resolution). Mutable refs so reads are + * synchronous for `isReady`/submit; `version` is threaded into the context + * value so each bump produces a new value reference and consumers re-render + * (the callbacks alone are referentially stable, so without it a bump would + * never propagate past the memoized value). */ const decisionsRef = useRef(new Map>()); const registeredRef = useRef(new Map>()); - const [, bump] = useState(0); + const [version, bump] = useState(0); const rerender = useCallback(() => bump((v) => v + 1), []); const [statusByAction, setStatusByAction] = useState>({}); @@ -220,6 +231,7 @@ export default function ApprovalProvider({ children }: { children: React.ReactNo const value = useMemo( () => ({ + version, setDecision, getDecision, getDecisions, @@ -232,6 +244,7 @@ export default function ApprovalProvider({ children }: { children: React.ReactNo setStatus, }), [ + version, setDecision, getDecision, getDecisions, diff --git a/client/src/components/Chat/Messages/Content/__tests__/ToolApproval.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ToolApproval.test.tsx new file mode 100644 index 0000000000..0b5a2afcdf --- /dev/null +++ b/client/src/components/Chat/Messages/Content/__tests__/ToolApproval.test.tsx @@ -0,0 +1,105 @@ +import React from 'react'; +import { RecoilRoot } from 'recoil'; +import { fireEvent, render, screen } from '@testing-library/react'; +import type { Agents } from 'librechat-data-provider'; +import ApprovalProvider from '../ApprovalContext'; +import ToolApproval from '../ToolApproval'; + +jest.mock('~/hooks', () => ({ + useLocalize: () => (key: string, values?: Record) => { + if (key === 'com_ui_submit_decisions') { + return `Submit ${values?.[0]} decisions`; + } + const map: Record = { + com_ui_approve: 'Approve', + com_ui_reject: 'Reject', + com_ui_edit: 'Edit', + com_ui_respond: 'Respond', + com_ui_submit: 'Submit', + com_ui_submitting: 'Submitting', + com_ui_invalid_json: 'Invalid JSON', + com_ui_reject_reason_placeholder: 'Reason', + com_ui_tool_response_placeholder: 'Response', + }; + return map[key] ?? key; + }, +})); + +jest.mock('~/data-provider', () => ({ + useSubmitToolApprovalMutation: () => ({ mutate: jest.fn() }), + useSubmitAskAnswerMutation: () => ({ mutate: jest.fn() }), +})); + +jest.mock('~/Providers/ChatContext', () => ({ + ChatContext: jest.requireActual('react').createContext(null), +})); + +const approval = ( + allowed: Agents.ToolApprovalDecisionType[] = ['approve', 'reject'], +): NonNullable => ({ + actionId: 'action-1', + allowed_decisions: allowed, +}); + +const renderCards = (cards: React.ReactNode) => + render( + + {cards} + , + ); + +describe('ToolApproval', () => { + test('enables Submit immediately after Approve is the first decision (#14390)', () => { + renderCards(); + + const submit = screen.getByRole('button', { name: 'Submit' }); + expect(submit).toBeDisabled(); + + fireEvent.click(screen.getByRole('button', { name: 'Approve' })); + + expect(submit).toBeEnabled(); + }); + + test('deselecting the active decision disables Submit again', () => { + renderCards(); + + const approve = screen.getByRole('button', { name: 'Approve' }); + fireEvent.click(approve); + expect(screen.getByRole('button', { name: 'Submit' })).toBeEnabled(); + + fireEvent.click(approve); + expect(screen.getByRole('button', { name: 'Submit' })).toBeDisabled(); + }); + + test('a respond decision only counts once its text is non-empty', () => { + renderCards(); + + fireEvent.click(screen.getByRole('button', { name: 'Respond' })); + const submit = screen.getByRole('button', { name: 'Submit' }); + expect(submit).toBeDisabled(); + + fireEvent.change(screen.getByRole('textbox', { name: 'Respond' }), { + target: { value: 'use the staging table' }, + }); + expect(submit).toBeEnabled(); + }); + + test('multiple paused calls share one Submit that requires every decision', () => { + renderCards( + <> + + + , + ); + + const submit = screen.getByRole('button', { name: 'Submit 2 decisions' }); + expect(submit).toBeDisabled(); + + const [approveFirst, approveSecond] = screen.getAllByRole('button', { name: 'Approve' }); + fireEvent.click(approveFirst); + expect(submit).toBeDisabled(); + + fireEvent.click(approveSecond); + expect(submit).toBeEnabled(); + }); +});