mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🧷 fix: Address Codex Round 13 On The Queued Outbox
Two P2 findings, both valid, both recurrences of a pattern rather than one-off bugs — so both are fixed at the shape, not the instance. The clear-all refusal path hands the ITEM back instead of rebuilding one. That reconstruction has now dropped two fields in two rounds — the predecessor fence, then the interrupt tier — and the next one would have been just as quiet. `requeueCleared` puts the row back whole, order-preserving, so there is no longer a field list to keep in sync. Removing a row whose editor was left empty no longer returns its words to the composer. The queue still holds the pre-edit text, so handing that back resurrected exactly what the user had deleted. Rather than disabling the trash, the restore is skipped: emptying a row and then removing it reads as "delete this", and there is nothing to return.
This commit is contained in:
parent
9d566cfb2e
commit
75f89a8b68
3 changed files with 65 additions and 32 deletions
|
|
@ -350,13 +350,20 @@ function QueuedRowBase({
|
|||
const remove = () => {
|
||||
/* Same safety net as the in-flight cancel: once removal is safely
|
||||
* settled, return the words to the composer when it is free (the
|
||||
* gated restore refuses rather than clobber a draft). */
|
||||
onRestoreToComposer(
|
||||
message.text,
|
||||
message.files,
|
||||
{ quotes: message.quotes, manualSkills: message.manualSkills },
|
||||
conversationId,
|
||||
);
|
||||
* gated restore refuses rather than clobber a draft).
|
||||
*
|
||||
* Skipped when the editor was left empty: the queue still holds the
|
||||
* pre-edit words, so handing them back would resurrect text the
|
||||
* user had visibly deleted. Emptying a row and then removing it
|
||||
* reads as "delete this", and there is nothing to return. */
|
||||
if (!emptyEdit) {
|
||||
onRestoreToComposer(
|
||||
message.text,
|
||||
message.files,
|
||||
{ quotes: message.quotes, manualSkills: message.manualSkills },
|
||||
conversationId,
|
||||
);
|
||||
}
|
||||
steering.removeQueued(message.id);
|
||||
return true;
|
||||
};
|
||||
|
|
@ -511,20 +518,12 @@ function QueuedOutboxBase({
|
|||
);
|
||||
/** The gated restore refuses rather than clobber a draft the user has
|
||||
* since staged. Hand the words back to the queue instead of dropping
|
||||
* them — one row now, since they were folded on the way out. */
|
||||
* them — one row now, since they were folded on the way out. The ITEM
|
||||
* goes back whole: rebuilding one from parts dropped its predecessor
|
||||
* fence once and its interrupt tier once, and the next field would be
|
||||
* just as quiet. */
|
||||
if (!restored) {
|
||||
steering.enqueue(cleared.text, {
|
||||
files: cleared.files,
|
||||
quotes: cleared.quotes,
|
||||
manualSkills: cleared.manualSkills,
|
||||
skipUsageMark: true,
|
||||
id: cleared.id,
|
||||
createdAt: cleared.createdAt,
|
||||
/** Kept explicitly: an idle chat has no active epoch for `enqueue` to
|
||||
* substitute, and an unfenced row can replace a newer generation
|
||||
* instead of taking the predecessor-mismatch recovery path. */
|
||||
expectedPredecessorCreatedAt: cleared.expectedPredecessorCreatedAt,
|
||||
});
|
||||
steering.requeueCleared([cleared]);
|
||||
showToast({ message: localize('com_ui_steer_edit_queued'), status: 'info' });
|
||||
}
|
||||
})();
|
||||
|
|
|
|||
|
|
@ -568,6 +568,7 @@ const mockUpdateQueuedText = jest.fn(() => true);
|
|||
const mockMergeQueued = jest.fn(() => true);
|
||||
const mockCancelQueueDrain = jest.fn();
|
||||
const mockEnqueue = jest.fn();
|
||||
const mockRequeueCleared = jest.fn();
|
||||
let mockClearQueued = jest.fn(async (): Promise<QueuedMessage | null> => null);
|
||||
|
||||
const outboxSteering = (overrides: Partial<SteeringControls> = {}) => ({
|
||||
|
|
@ -577,6 +578,7 @@ const outboxSteering = (overrides: Partial<SteeringControls> = {}) => ({
|
|||
clearQueued: mockClearQueued,
|
||||
cancelQueueDrain: mockCancelQueueDrain,
|
||||
enqueue: mockEnqueue,
|
||||
requeueCleared: mockRequeueCleared,
|
||||
...overrides,
|
||||
});
|
||||
|
||||
|
|
@ -813,6 +815,34 @@ describe('PendingSteerChips — queued outbox group', () => {
|
|||
expect(screen.getByTestId('queue-merge')).not.toBeDisabled();
|
||||
});
|
||||
|
||||
/** The queue still holds the pre-edit words, so handing them back would
|
||||
* resurrect text the user visibly deleted. Emptying a row and removing it
|
||||
* reads as "delete this". */
|
||||
it('does not return stale words to the composer when removing an emptied row', () => {
|
||||
renderChips([twoQueued[0]], { steering: outboxSteering() });
|
||||
|
||||
fireEvent.click(screen.getByText('first thought'));
|
||||
fireEvent.change(screen.getByTestId('queued-message-edit'), { target: { value: '' } });
|
||||
fireEvent.click(screen.getByLabelText('com_ui_remove_queued'));
|
||||
|
||||
expect(mockRestoreToComposer).not.toHaveBeenCalled();
|
||||
expect(mockRemoveQueued).toHaveBeenCalledWith('q1');
|
||||
});
|
||||
|
||||
it('still returns the words when removing a row that was not being emptied', () => {
|
||||
renderChips([twoQueued[0]], { steering: outboxSteering() });
|
||||
|
||||
fireEvent.click(screen.getByLabelText('com_ui_remove_queued'));
|
||||
|
||||
expect(mockRestoreToComposer).toHaveBeenCalledWith(
|
||||
'first thought',
|
||||
undefined,
|
||||
{ quotes: undefined, manualSkills: undefined },
|
||||
CONVO_ID,
|
||||
);
|
||||
expect(mockRemoveQueued).toHaveBeenCalledWith('q1');
|
||||
});
|
||||
|
||||
/** Clear all folds the queue exactly as Merge does, so it takes the same
|
||||
* standdown rather than being a documented exception. */
|
||||
it('refuses to clear all while an inline edit is empty', () => {
|
||||
|
|
@ -904,6 +934,8 @@ describe('PendingSteerChips — queued outbox group', () => {
|
|||
text: 'not lost',
|
||||
createdAt: 1,
|
||||
expectedPredecessorCreatedAt: 4242,
|
||||
priority: true,
|
||||
bumpedAt: 99,
|
||||
};
|
||||
mockClearQueued = jest.fn(async () => folded);
|
||||
mockRestoreToComposer.mockReturnValueOnce(false);
|
||||
|
|
@ -912,18 +944,12 @@ describe('PendingSteerChips — queued outbox group', () => {
|
|||
fireEvent.click(screen.getByTestId('queue-clear-all'));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(mockEnqueue).toHaveBeenCalledWith(
|
||||
'not lost',
|
||||
expect.objectContaining({
|
||||
id: 'q1',
|
||||
createdAt: 1,
|
||||
skipUsageMark: true,
|
||||
/** An idle chat has no active epoch to substitute, so an unfenced
|
||||
* requeue could replace a newer generation from another tab. */
|
||||
expectedPredecessorCreatedAt: 4242,
|
||||
}),
|
||||
);
|
||||
/** The ITEM goes back whole, so no field can be quietly dropped — the
|
||||
* fence and the interrupt tier were each lost once when this path
|
||||
* rebuilt a row from parts. */
|
||||
expect(mockRequeueCleared).toHaveBeenCalledWith([folded]);
|
||||
});
|
||||
expect(mockEnqueue).not.toHaveBeenCalled();
|
||||
expect(mockShowToast).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ message: 'com_ui_steer_edit_queued' }),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -784,7 +784,13 @@ export default function useSteering({
|
|||
[queueKey],
|
||||
);
|
||||
|
||||
/** Puts rows back without clobbering anything queued in the meantime. */
|
||||
/**
|
||||
* Puts rows back without clobbering anything queued in the meantime. Also the
|
||||
* public requeue path for a cleared payload the composer refused: passing the
|
||||
* ITEM back preserves every field it carries, where rebuilding one from parts
|
||||
* has twice now lost a field that mattered (the predecessor fence, then the
|
||||
* interrupt tier).
|
||||
*/
|
||||
const restoreQueuedBatch = useRecoilCallback(
|
||||
({ set }) =>
|
||||
(items: QueuedMessage[]) => {
|
||||
|
|
@ -1647,6 +1653,7 @@ export default function useSteering({
|
|||
convertSteerToQueue,
|
||||
queueReclaimedSteer,
|
||||
enqueue,
|
||||
requeueCleared: restoreQueuedBatch,
|
||||
removeQueued,
|
||||
discardQueued,
|
||||
bumpQueued,
|
||||
|
|
@ -1678,6 +1685,7 @@ export default function useSteering({
|
|||
convertSteerToQueue,
|
||||
queueReclaimedSteer,
|
||||
enqueue,
|
||||
restoreQueuedBatch,
|
||||
removeQueued,
|
||||
discardQueued,
|
||||
bumpQueued,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue