From bd1df30b7d6c445d6c758b163cd23048e2f4b664 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 16 Jul 2026 11:14:29 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20fix:=20Scope,=20Cap,=20and=20De-?= =?UTF-8?q?Execute=20the=20In-Flight=20Steer=20Stack=20(#14310)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿ”’ fix: Scope, Cap, and De-Execute the In-Flight Steer Stack Codex review on 9594ee7146. 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 on de9ede2aad. 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 on 09c93987a. 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. --- client/src/components/Chat/Input/ChatForm.tsx | 4 +- .../components/Chat/Input/InFlightSteers.tsx | 28 ++++++- .../Input/__tests__/InFlightSteers.test.tsx | 78 ++++++++++++++++++- client/src/hooks/Chat/useSteerCancel.ts | 12 ++- 4 files changed, 114 insertions(+), 8 deletions(-) diff --git a/client/src/components/Chat/Input/ChatForm.tsx b/client/src/components/Chat/Input/ChatForm.tsx index 8e601f7adf..8c56e0371e 100644 --- a/client/src/components/Chat/Input/ChatForm.tsx +++ b/client/src/components/Chat/Input/ChatForm.tsx @@ -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 && }
- {steering.enabled && } + {/* 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 && }
- {enableUserMsgMarkdown ? : steer.text} + {/* No code execution: this bubble sits outside MessageContext, so + * Run Code would fire with no message/part to target. */} + {enableUserMsgMarkdown ? ( + + ) : ( + steer.text + )}
{!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(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 (
{inFlight.map((steer) => ( diff --git a/client/src/components/Chat/Input/__tests__/InFlightSteers.test.tsx b/client/src/components/Chat/Input/__tests__/InFlightSteers.test.tsx index 59a2c68e6c..cd4c3695cf 100644 --- a/client/src/components/Chat/Input/__tests__/InFlightSteers.test.tsx +++ b/client/src/components/Chat/Input/__tests__/InFlightSteers.test.tsx @@ -42,18 +42,26 @@ jest.mock('~/components/Chat/Messages/Content/FilePreviewDialog', () => ({ jest.mock('~/components/Chat/Messages/Content/MarkdownLite', () => ({ __esModule: true, - default: ({ content }: { content: string }) => ( - {content} + default: ({ content, codeExecution }: { content: string; codeExecution?: boolean }) => ( + + {content} + ), })); const CONVO_ID = 'convo-in-flight'; -function renderSteers(steers: PendingSteer[], options?: { enableUserMsgMarkdown?: boolean }) { +function renderSteers( + steers: PendingSteer[], + options?: { enableUserMsgMarkdown?: boolean; appliedSteerIds?: string[] }, +) { return render( { 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( + { + set(store.pendingSteersByConvoId(CONVO_ID), [ + { steerId: 's1', text: 'first', status: 'pending', createdAt: 1 }, + { steerId: 's2', text: 'just submitted', status: 'pending', createdAt: 2 }, + ]); + }} + > + + , + ); + 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, diff --git a/client/src/hooks/Chat/useSteerCancel.ts b/client/src/hooks/Chat/useSteerCancel.ts index 3c0ec55c6a..396fc6ee1f 100644 --- a/client/src/hooks/Chat/useSteerCancel.ts +++ b/client/src/hooks/Chat/useSteerCancel.ts @@ -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], );