mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🔒 fix: Scope, Cap, and De-Execute the In-Flight Steer Stack (#14310)
* 🔒 fix: Scope, Cap, and De-Execute the In-Flight Steer Stack Codex review on9594ee7146. Three valid P2s, all fallout from moving the steers out of the message region into the composer. - Run scope: the in-thread slot was gated on `effectiveIsSubmitting`, but the new one only checked `steering.enabled` (= steerable endpoint + primary composer), which is true with no run in flight. A chip that outlives its run — cancel's onError restoring one the final event already converted to a queued follow-up — stranded a bubble above the composer, possibly beside the queued row for the same text. Restores the run gate. - Height cap: a steer runs to 16k chars (DEFAULT_STEER_MAX_LENGTH) and a run takes up to 10 (STEER_QUEUE_MAX_DEPTH). Unbounded in the composer, that pushes the input off-screen; the old slot could grow freely because it scrolled with the thread. Caps the stack at 35vh. - Code execution: MarkdownLite defaults `codeExecution` on, but this bubble renders outside MessageContext, so Run Code would fire the tool mutation with no messageId and an empty conversationId. Passes codeExecution={false} — a provisional steer has nothing to run against. * 📜 fix: Keep the Newest In-Flight Steer in View Codex review onde9ede2aad. Valid, and a regression from the 35vh cap in the previous commit: steers append newest-last, so once the stack overflows it sits scrolled to the OLDEST entry. The steer just submitted — and its cancel control — lands below the fold and reads as dropped. The cap traded "composer pushed off-screen" for "newest steer hidden". Sticks the stack to the bottom, keyed on the newest steer id so it fires when one is appended rather than on every render. * 🧹 fix: Don't Restore a Steer That Already Settled Codex review on09c93987a. Valid, and it closes the hole the run gate only hid — I deferred this two rounds ago as pre-existing, which was wrong: the gate hides a stale entry while the run is idle, but useQueueDrain auto-sends the queued follow-up, isSubmitting flips back to true, and the previous run's entry renders as an in-flight bubble beside its own queued copy. Fixes it at the source instead: cancel's onError no longer restores a steer whose id is in appliedSteerIdsByConvoId — the settled set, stamped by both the apply path and the run-end conversion, and deliberately capped rather than cleared so it survives run end for exactly this race (same instrument as #14276). The run gate stays: it's parity with the in-thread slot's effectiveIsSubmitting and still defends against any other leak.
This commit is contained in:
parent
8f712259ea
commit
bd1df30b7d
4 changed files with 114 additions and 8 deletions
|
|
@ -413,7 +413,9 @@ const ChatForm = memo(function ChatForm({
|
|||
{/* Primary composer owns the selection popup so split-view doesn't double it. */}
|
||||
{index === 0 && quotesEnabled && <QuoteButton conversationId={conversationId} />}
|
||||
<div className="flex w-full flex-col">
|
||||
{steering.enabled && <InFlightSteers conversationId={conversationId} />}
|
||||
{/* Run-scoped: `enabled` alone is any primary composer on a steerable
|
||||
endpoint, so a chip that outlives the run would strand a bubble. */}
|
||||
{steering.enabled && isSubmitting && <InFlightSteers conversationId={conversationId} />}
|
||||
<div className={cn('flex w-full items-center', isRTL && 'flex-row-reverse')}>
|
||||
<Mention
|
||||
index={index}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { memo, useMemo, useState, useCallback } from 'react';
|
||||
import { memo, useRef, useMemo, useState, useEffect, useCallback } from 'react';
|
||||
import { X, Zap } from 'lucide-react';
|
||||
import { useRecoilValue } from 'recoil';
|
||||
import type { TFile, TMessage } from 'librechat-data-provider';
|
||||
|
|
@ -103,7 +103,13 @@ const InFlightSteer = memo(function InFlightSteer({
|
|||
!enableUserMsgMarkdown && 'whitespace-pre-wrap',
|
||||
)}
|
||||
>
|
||||
{enableUserMsgMarkdown ? <MarkdownLite content={steer.text} /> : steer.text}
|
||||
{/* No code execution: this bubble sits outside MessageContext, so
|
||||
* Run Code would fire with no message/part to target. */}
|
||||
{enableUserMsgMarkdown ? (
|
||||
<MarkdownLite content={steer.text} codeExecution={false} />
|
||||
) : (
|
||||
steer.text
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
{!sending && (
|
||||
|
|
@ -150,16 +156,32 @@ const InFlightSteers = memo(function InFlightSteers({
|
|||
const steers = useRecoilValue(store.pendingSteersByConvoId(conversationId));
|
||||
const inFlight = useMemo(() => steers.filter((steer) => steer.status !== 'failed'), [steers]);
|
||||
|
||||
/** Steers append newest-last, so an overflowing stack would sit scrolled to
|
||||
* the oldest — the steer just submitted (and its cancel) would be below the
|
||||
* fold and read as dropped. Keyed on the newest id, not every render. */
|
||||
const listRef = useRef<HTMLDivElement>(null);
|
||||
const newestId = inFlight[inFlight.length - 1]?.steerId;
|
||||
useEffect(() => {
|
||||
const list = listRef.current;
|
||||
if (list != null) {
|
||||
list.scrollTop = list.scrollHeight;
|
||||
}
|
||||
}, [newestId]);
|
||||
|
||||
if (inFlight.length === 0) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return (
|
||||
<div
|
||||
ref={listRef}
|
||||
role="list"
|
||||
aria-label={localize('com_ui_steer_in_flight')}
|
||||
data-testid="in-flight-steers"
|
||||
className="flex flex-col items-start gap-2 px-2 pb-2"
|
||||
/* Capped: a steer runs to 16k chars and a run takes up to 10 of them.
|
||||
* Unbounded, the stack would push the composer off-screen — the old
|
||||
* in-thread slot could grow freely because it scrolled with the thread. */
|
||||
className="flex max-h-[35vh] flex-col items-start gap-2 overflow-y-auto px-2 pb-2"
|
||||
>
|
||||
{inFlight.map((steer) => (
|
||||
<InFlightSteer key={steer.steerId} steer={steer} conversationId={conversationId} />
|
||||
|
|
|
|||
|
|
@ -42,18 +42,26 @@ jest.mock('~/components/Chat/Messages/Content/FilePreviewDialog', () => ({
|
|||
|
||||
jest.mock('~/components/Chat/Messages/Content/MarkdownLite', () => ({
|
||||
__esModule: true,
|
||||
default: ({ content }: { content: string }) => (
|
||||
<span data-testid="steer-markdown">{content}</span>
|
||||
default: ({ content, codeExecution }: { content: string; codeExecution?: boolean }) => (
|
||||
<span data-testid="steer-markdown" data-code-execution={String(codeExecution)}>
|
||||
{content}
|
||||
</span>
|
||||
),
|
||||
}));
|
||||
|
||||
const CONVO_ID = 'convo-in-flight';
|
||||
|
||||
function renderSteers(steers: PendingSteer[], options?: { enableUserMsgMarkdown?: boolean }) {
|
||||
function renderSteers(
|
||||
steers: PendingSteer[],
|
||||
options?: { enableUserMsgMarkdown?: boolean; appliedSteerIds?: string[] },
|
||||
) {
|
||||
return render(
|
||||
<RecoilRoot
|
||||
initializeState={({ set }) => {
|
||||
set(store.pendingSteersByConvoId(CONVO_ID), steers);
|
||||
if (options?.appliedSteerIds != null) {
|
||||
set(store.appliedSteerIdsByConvoId(CONVO_ID), options.appliedSteerIds);
|
||||
}
|
||||
if (options?.enableUserMsgMarkdown != null) {
|
||||
set(store.enableUserMsgMarkdown, options.enableUserMsgMarkdown);
|
||||
}
|
||||
|
|
@ -131,6 +139,23 @@ describe('InFlightSteers', () => {
|
|||
expect(screen.getByText('network flake')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('does not restore a steer that settled while the cancel POST was in flight', () => {
|
||||
// The run's final event converted this steer to a queued follow-up, which
|
||||
// stamps its id into the applied set. Restoring it on a failed cancel would
|
||||
// strand a stale entry that the NEXT run — a queue drain auto-sends one —
|
||||
// renders as an in-flight bubble beside that queued copy.
|
||||
renderSteers(
|
||||
[{ steerId: 's-settled', text: 'already queued', status: 'pending', createdAt: 1 }],
|
||||
{ appliedSteerIds: ['s-settled'] },
|
||||
);
|
||||
fireEvent.click(screen.getByTestId('steer-cancel'));
|
||||
expect(screen.queryByText('already queued')).toBeNull();
|
||||
|
||||
const options = mockCancelMutate.mock.calls[0][1] as { onError: () => void };
|
||||
act(() => options.onError());
|
||||
expect(screen.queryByText('already queued')).toBeNull();
|
||||
});
|
||||
|
||||
it('renders images through the composer thumbnail path, not the full-size message image', () => {
|
||||
renderSteers([
|
||||
{
|
||||
|
|
@ -193,6 +218,53 @@ describe('InFlightSteers', () => {
|
|||
expect(screen.getByTestId('steer-markdown')).toHaveTextContent('**bold** steer');
|
||||
});
|
||||
|
||||
it('disables code execution: the bubble has no message/part for Run Code to target', () => {
|
||||
renderSteers([{ steerId: 's1', text: '```js\nrun()\n```', status: 'pending', createdAt: 1 }], {
|
||||
enableUserMsgMarkdown: true,
|
||||
});
|
||||
// This component renders outside MessageContext, so an executable code
|
||||
// block would fire the tool mutation with no messageId/conversationId.
|
||||
expect(screen.getByTestId('steer-markdown')).toHaveAttribute('data-code-execution', 'false');
|
||||
});
|
||||
|
||||
it('keeps the newest steer in view when the capped stack overflows', () => {
|
||||
// jsdom does no layout, so scrollHeight is 0 unless stubbed — without it
|
||||
// the assertion would pass vacuously against a scrollTop of 0.
|
||||
const scrollHeight = jest
|
||||
.spyOn(HTMLElement.prototype, 'scrollHeight', 'get')
|
||||
.mockReturnValue(600);
|
||||
try {
|
||||
const { rerender } = renderSteers([
|
||||
{ steerId: 's1', text: 'first', status: 'pending', createdAt: 1 },
|
||||
]);
|
||||
// A newly submitted steer appends BELOW the existing ones, so a stack
|
||||
// left scrolled to the top would hide it and its cancel control.
|
||||
rerender(
|
||||
<RecoilRoot
|
||||
initializeState={({ set }) => {
|
||||
set(store.pendingSteersByConvoId(CONVO_ID), [
|
||||
{ steerId: 's1', text: 'first', status: 'pending', createdAt: 1 },
|
||||
{ steerId: 's2', text: 'just submitted', status: 'pending', createdAt: 2 },
|
||||
]);
|
||||
}}
|
||||
>
|
||||
<InFlightSteers conversationId={CONVO_ID} />
|
||||
</RecoilRoot>,
|
||||
);
|
||||
expect(screen.getByTestId('in-flight-steers').scrollTop).toBe(600);
|
||||
} finally {
|
||||
scrollHeight.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it('caps the stack so a long steer cannot push the composer off-screen', () => {
|
||||
renderSteers([{ steerId: 's1', text: 'x'.repeat(4000), status: 'pending', createdAt: 1 }]);
|
||||
// A steer runs to 16k chars, and a run takes up to 10 of them.
|
||||
const stack = screen.getByTestId('in-flight-steers');
|
||||
expect(stack.className).toContain('max-h-[35vh]');
|
||||
expect(stack.className).toContain('overflow-y-auto');
|
||||
});
|
||||
|
||||
it('renders raw text when user-message markdown is off', () => {
|
||||
renderSteers([{ steerId: 's1', text: '**bold** steer', status: 'pending', createdAt: 1 }], {
|
||||
enableUserMsgMarkdown: false,
|
||||
|
|
|
|||
|
|
@ -24,8 +24,18 @@ export default function useSteerCancel(conversationId: string) {
|
|||
[conversationId],
|
||||
);
|
||||
const restoreEntry = useRecoilCallback(
|
||||
({ set }) =>
|
||||
({ snapshot, set }) =>
|
||||
(entry: PendingSteer) => {
|
||||
/* A steer that settled while the POST was in flight — applied on the
|
||||
* server, or converted to a queued follow-up at run end — must NOT come
|
||||
* back. The next run (a queue drain auto-sends one) would render this
|
||||
* stale entry as an in-flight bubble beside its own queued copy. */
|
||||
const settled = snapshot
|
||||
.getLoadable(store.appliedSteerIdsByConvoId(conversationId))
|
||||
.getValue();
|
||||
if (settled.includes(entry.steerId)) {
|
||||
return;
|
||||
}
|
||||
set(store.pendingSteersByConvoId(conversationId), (prev) =>
|
||||
prev.some((item) => item.steerId === entry.steerId) ? prev : [...prev, entry],
|
||||
);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue