fix: Address Codex Round 11 On The Queued Outbox

Three P2 findings, all valid, all local to the outbox group.

Merge now stands down on an unresolved empty edit. It reads the queue like
every other sender, so folding while the editor was blank carried the words
the user had just deleted into the merged message. Clear all is left alone
deliberately: its payload lands in the composer, where a reappearing word
is visible rather than silently sent.

Promotion eligibility is computed in one pass. The per-row prefix scan
allocated and walked a slice for every row, and the write-through editor
re-renders this list on every keystroke.

The collapsed disclosure no longer carries an aria-label. Collapsing
unmounts the rows, so the count and next-up preview inside the button are
the queue's only description — and an aria-label overrides exactly that
text. `aria-expanded` already carries the show/hide state, which is what
the label was really conveying.
This commit is contained in:
Danny Avila 2026-08-04 07:29:04 -04:00
parent 6c48c41f8d
commit 4d4d2de1ca
3 changed files with 74 additions and 6 deletions

View file

@ -458,6 +458,15 @@ function QueuedOutboxBase({
const [expanded, setExpanded] = useAtom(queueExpandedFamily(steering.queueKey));
const emptyEditId = useAtomValue(queueEmptyEditFamily(steering.queueKey));
const mergeable = useMemo(() => queued.every(isMergeableQueuedMessage), [queued]);
/** Folding reads the queue, so an unresolved empty edit would carry the words
* the user just deleted into the merged message. Same standdown the senders
* use the row's own controls, the drain, and the shortcut proxy. */
const mergeBlockedReason = (() => {
if (!mergeable) {
return localize('com_ui_queue_merge_blocked');
}
return emptyEditId != null ? localize('com_ui_queue_edit_empty') : undefined;
})();
/** The shortcut's promise is the NEWEST waiting message, which is not the
* last array slot once a promotion has reordered the queue so pick by
* stamp. Recovery-bound rows are skipped because steering refuses them
@ -475,6 +484,14 @@ function QueuedOutboxBase({
return newest;
}, [queued]);
const [next] = queued;
/** Index of the first row a promotion could overtake. Computed once: the
* per-row prefix scan it replaces allocated and walked a slice for every
* row, and the write-through editor re-renders this list on every
* keystroke. */
const firstOvertakeable = useMemo(
() => queued.findIndex((item) => item.priority !== true),
[queued],
);
const clearAll = useCallback(() => {
void (async () => {
@ -528,7 +545,10 @@ function QueuedOutboxBase({
<button
type="button"
aria-expanded={expanded}
aria-label={localize(expanded ? 'com_ui_queue_collapse' : 'com_ui_queue_expand')}
/* Deliberately unlabelled: the accessible name comes from the count and
* next-up preview inside, which is the only description of the queue
* while the rows are unmounted. `aria-expanded` carries the show/hide
* state, so a label would only overwrite that summary. */
data-testid="queue-group-toggle"
onClick={() => setExpanded((prev) => !prev)}
className={cn(ROW_CLASS, 'relative text-left hover:bg-surface-hover')}
@ -565,7 +585,7 @@ function QueuedOutboxBase({
/** A promotion lifts a row to the top of the promotions tier,
* which is still below every interrupt so offering it when
* only interrupts sit ahead would advertise a no-op. */
canBump={queued.slice(0, position).some((ahead) => ahead.priority !== true)}
canBump={firstOvertakeable !== -1 && position > firstOvertakeable}
sendPending={sendPending}
onRestoreToComposer={onRestoreToComposer}
/>
@ -578,8 +598,8 @@ function QueuedOutboxBase({
type="button"
className={PRIMARY_BTN_CLASS}
data-testid="queue-merge"
disabled={!mergeable}
title={mergeable ? undefined : localize('com_ui_queue_merge_blocked')}
disabled={mergeBlockedReason != null}
title={mergeBlockedReason}
onClick={() => steering.mergeQueued()}
>
<Merge className="h-4 w-4" aria-hidden="true" />

View file

@ -767,6 +767,24 @@ describe('PendingSteerChips — queued outbox group', () => {
expect(mockBumpQueued).toHaveBeenCalledWith('second');
});
/** Promotion eligibility is computed in one pass, so verify the rule still
* holds for a queue deep enough that the old per-row prefix scan mattered. */
it('offers Send next on every row past the first movable one, at depth', () => {
const deep = [
{ id: 'armed', text: 'interrupt', createdAt: 9, priority: true },
...Array.from({ length: 5 }, (_, i) => ({
id: `q${i}`,
text: `ordinary ${i}`,
createdAt: i,
})),
];
renderChips(deep, { steering: outboxSteering() });
fireEvent.click(screen.getByTestId('queue-group-toggle'));
// The interrupt and the first ordinary row cannot overtake anything.
expect(screen.getAllByTestId('queued-send-next')).toHaveLength(4);
});
it('merges the batch into one turn', () => {
renderChips(twoQueued, { steering: outboxSteering() });
fireEvent.click(screen.getByTestId('queue-group-toggle'));
@ -775,6 +793,38 @@ describe('PendingSteerChips — queued outbox group', () => {
expect(mockMergeQueued).toHaveBeenCalledTimes(1);
});
/** Folding reads the queue, so an emptied editor would carry the words the
* user just deleted into the merged message. */
it('refuses to merge while an inline edit is empty', () => {
renderChips(twoQueued, { steering: outboxSteering() });
fireEvent.click(screen.getByTestId('queue-group-toggle'));
const rows = screen.getAllByTestId('queued-message-row');
fireEvent.click(rows[1].querySelector('span[title]') as HTMLElement);
fireEvent.change(screen.getByTestId('queued-message-edit'), { target: { value: ' ' } });
const merge = screen.getByTestId('queue-merge');
expect(merge).toBeDisabled();
fireEvent.click(merge);
expect(mockMergeQueued).not.toHaveBeenCalled();
// Resolving the edit releases it.
fireEvent.change(screen.getByTestId('queued-message-edit'), { target: { value: 'rewritten' } });
expect(screen.getByTestId('queue-merge')).not.toBeDisabled();
});
/** Collapsing unmounts the rows, so the disclosure's own text is the queue's
* only description an aria-label would overwrite it. */
it('announces the count and next-up preview as the disclosure name', () => {
renderChips(twoQueued, { steering: outboxSteering() });
const toggle = screen.getByTestId('queue-group-toggle');
expect(toggle).not.toHaveAttribute('aria-label');
expect(toggle).toHaveAccessibleName(/com_ui_queue_count/);
expect(toggle).toHaveAccessibleName(/com_ui_queue_next_up/);
expect(toggle).toHaveAttribute('aria-expanded', 'false');
});
it('refuses to merge while a recovered row holds a parked server source', () => {
renderChips([twoQueued[0], { ...twoQueued[1], recoverySteerId: 'server-source' }], {
steering: outboxSteering(),

View file

@ -1618,12 +1618,10 @@
"com_ui_question_unanswered": "No answer was given",
"com_ui_queue": "Queue",
"com_ui_queue_clear_all": "Clear all",
"com_ui_queue_collapse": "Hide queued messages",
"com_ui_queue_count": "{{0}} queued",
"com_ui_queue_edit_empty": "Type a message, or press Escape to keep the original",
"com_ui_queue_edit_inline": "Edit this queued message",
"com_ui_queue_escalate_newest": "Interrupt the run and send the newest queued message",
"com_ui_queue_expand": "Show all queued messages",
"com_ui_queue_merge": "Merge into one message",
"com_ui_queue_merge_blocked": "Recovered messages can't be merged — remove or send them first",
"com_ui_queue_next_up": "Next: {{0}}",