fix(scheduling): harden edit/reschedule after review
Address findings from 3 review agents on the edit/reschedule feature: - Race (correctness): handleEdit now marks the old message as cancelling while its cancel is in flight, so its Edit/Cancel buttons are disabled. Previously the old row stayed live during the fire-and-forget cancel, so a fast second edit could orphan a still-scheduled event and send twice. - Durability (correctness): on a failed cancel-old, re-insert the old message if auto-prune removed its row while the modal was open, so the still-live delayed event stays visible and retriable instead of failing silently. Also clear any stale cancel error when starting an edit. - a11y: per-row Edit/Cancel buttons now carry distinct aria-labels that include the message preview and send-time, so screen-reader users can tell which of several scheduled messages each button targets. - UX: modal gains a submitLabel prop; the edit flow shows "Reschedule" instead of "Schedule". Modal now focuses the message body on open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -30,6 +30,8 @@ interface ScheduleMessageModalProps {
|
|||||||
initialSendAt?: number;
|
initialSendAt?: number;
|
||||||
/** Header title; defaults to "Schedule Message". */
|
/** Header title; defaults to "Schedule Message". */
|
||||||
title?: string;
|
title?: string;
|
||||||
|
/** Primary-button label; defaults to "Schedule" (e.g. "Reschedule" when editing). */
|
||||||
|
submitLabel?: string;
|
||||||
onScheduled: (delayId: string, sendAt: number, content: IContent) => void;
|
onScheduled: (delayId: string, sendAt: number, content: IContent) => void;
|
||||||
onClose: () => void;
|
onClose: () => void;
|
||||||
}
|
}
|
||||||
@@ -94,6 +96,7 @@ export function ScheduleMessageModal({
|
|||||||
initialBody,
|
initialBody,
|
||||||
initialSendAt,
|
initialSendAt,
|
||||||
title = 'Schedule Message',
|
title = 'Schedule Message',
|
||||||
|
submitLabel = 'Schedule',
|
||||||
onScheduled,
|
onScheduled,
|
||||||
onClose,
|
onClose,
|
||||||
}: ScheduleMessageModalProps) {
|
}: ScheduleMessageModalProps) {
|
||||||
@@ -179,7 +182,7 @@ export function ScheduleMessageModal({
|
|||||||
<OverlayCenter>
|
<OverlayCenter>
|
||||||
<FocusTrap
|
<FocusTrap
|
||||||
focusTrapOptions={{
|
focusTrapOptions={{
|
||||||
initialFocus: false,
|
initialFocus: '#schedule-message-body',
|
||||||
onDeactivate: onClose,
|
onDeactivate: onClose,
|
||||||
clickOutsideDeactivates: true,
|
clickOutsideDeactivates: true,
|
||||||
escapeDeactivates: stopPropagation,
|
escapeDeactivates: stopPropagation,
|
||||||
@@ -341,7 +344,7 @@ export function ScheduleMessageModal({
|
|||||||
disabled={submitting || !preview}
|
disabled={submitting || !preview}
|
||||||
before={submitting ? <Spinner variant="Primary" size="100" /> : undefined}
|
before={submitting ? <Spinner variant="Primary" size="100" /> : undefined}
|
||||||
>
|
>
|
||||||
<Text size="B400">Schedule</Text>
|
<Text size="B400">{submitLabel}</Text>
|
||||||
</Button>
|
</Button>
|
||||||
</Box>
|
</Box>
|
||||||
</Dialog>
|
</Dialog>
|
||||||
|
|||||||
@@ -115,12 +115,25 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
// cancel leaves it visible (and retriable) instead of letting it silently fire.
|
// cancel leaves it visible (and retriable) instead of letting it silently fire.
|
||||||
const handleEdit = useCallback(
|
const handleEdit = useCallback(
|
||||||
(oldMsg: ScheduledMessage, newDelayId: string, sendAt: number, content: IContent) => {
|
(oldMsg: ScheduledMessage, newDelayId: string, sendAt: number, content: IContent) => {
|
||||||
|
// Add the newly-scheduled message up front (nothing lost yet).
|
||||||
setScheduledMessages((prev) => {
|
setScheduledMessages((prev) => {
|
||||||
const next = new Map(prev);
|
const next = new Map(prev);
|
||||||
const current = (next.get(roomId) ?? []).filter((m) => m.delayId !== newDelayId);
|
const current = (next.get(roomId) ?? []).filter((m) => m.delayId !== newDelayId);
|
||||||
next.set(roomId, [{ delayId: newDelayId, roomId, content, sendAt }, ...current]);
|
next.set(roomId, [{ delayId: newDelayId, roomId, content, sendAt }, ...current]);
|
||||||
return next;
|
return next;
|
||||||
});
|
});
|
||||||
|
// Mark the old message as cancelling so its row's Edit/Cancel buttons are
|
||||||
|
// disabled while we tear it down. Without this the old row stays live during
|
||||||
|
// the in-flight cancel and a second edit could orphan a still-scheduled event
|
||||||
|
// (both would fire). Also clear any stale error from a prior failed cancel.
|
||||||
|
setCancelling((prev) => new Set(prev).add(oldMsg.delayId));
|
||||||
|
setCancelErrors((prev) => {
|
||||||
|
if (!prev.has(oldMsg.delayId)) return prev;
|
||||||
|
const next = new Set(prev);
|
||||||
|
next.delete(oldMsg.delayId);
|
||||||
|
return next;
|
||||||
|
});
|
||||||
|
setEditing(null);
|
||||||
cancelScheduledMessage(mx, oldMsg.delayId)
|
cancelScheduledMessage(mx, oldMsg.delayId)
|
||||||
.then(() => {
|
.then(() => {
|
||||||
setScheduledMessages((prev) => {
|
setScheduledMessages((prev) => {
|
||||||
@@ -131,8 +144,28 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
return next;
|
return next;
|
||||||
});
|
});
|
||||||
})
|
})
|
||||||
.catch(() => setCancelErrors((prev) => new Set(prev).add(oldMsg.delayId)));
|
.catch(() => {
|
||||||
setEditing(null);
|
// Cancel failed — the old delayed event is still live server-side, so it
|
||||||
|
// must stay visible and retriable. Re-insert it if auto-prune removed the
|
||||||
|
// row while the modal was open, otherwise the failure (and the duplicate
|
||||||
|
// it will send) would be invisible.
|
||||||
|
setScheduledMessages((prev) => {
|
||||||
|
const next = new Map(prev);
|
||||||
|
const current = next.get(roomId) ?? [];
|
||||||
|
if (!current.some((m) => m.delayId === oldMsg.delayId)) {
|
||||||
|
next.set(roomId, [...current, oldMsg]);
|
||||||
|
}
|
||||||
|
return next;
|
||||||
|
});
|
||||||
|
setCancelErrors((prev) => new Set(prev).add(oldMsg.delayId));
|
||||||
|
})
|
||||||
|
.finally(() => {
|
||||||
|
setCancelling((prev) => {
|
||||||
|
const next = new Set(prev);
|
||||||
|
next.delete(oldMsg.delayId);
|
||||||
|
return next;
|
||||||
|
});
|
||||||
|
});
|
||||||
},
|
},
|
||||||
[mx, roomId, setScheduledMessages],
|
[mx, roomId, setScheduledMessages],
|
||||||
);
|
);
|
||||||
@@ -147,6 +180,7 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
initialBody={typeof editing.content.body === 'string' ? editing.content.body : ''}
|
initialBody={typeof editing.content.body === 'string' ? editing.content.body : ''}
|
||||||
initialSendAt={editing.sendAt}
|
initialSendAt={editing.sendAt}
|
||||||
title="Edit scheduled message"
|
title="Edit scheduled message"
|
||||||
|
submitLabel="Reschedule"
|
||||||
onScheduled={(newDelayId, sendAt, content) =>
|
onScheduled={(newDelayId, sendAt, content) =>
|
||||||
handleEdit(editing, newDelayId, sendAt, content)
|
handleEdit(editing, newDelayId, sendAt, content)
|
||||||
}
|
}
|
||||||
@@ -183,7 +217,11 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
{/* Tray items */}
|
{/* Tray items */}
|
||||||
{expanded && (
|
{expanded && (
|
||||||
<Box direction="Column">
|
<Box direction="Column">
|
||||||
{messages.map((msg) => (
|
{messages.map((msg) => {
|
||||||
|
const bodyPreview =
|
||||||
|
typeof msg.content.body === 'string' ? (msg.content.body as string) : '(message)';
|
||||||
|
const rowDesc = `${bodyPreview} at ${formatSendAt(msg.sendAt)}`;
|
||||||
|
return (
|
||||||
<Box
|
<Box
|
||||||
key={msg.delayId}
|
key={msg.delayId}
|
||||||
direction="Column"
|
direction="Column"
|
||||||
@@ -203,9 +241,7 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
whiteSpace: 'nowrap',
|
whiteSpace: 'nowrap',
|
||||||
}}
|
}}
|
||||||
>
|
>
|
||||||
{typeof msg.content.body === 'string'
|
{bodyPreview}
|
||||||
? (msg.content.body as string)
|
|
||||||
: '(message)'}
|
|
||||||
</Text>
|
</Text>
|
||||||
<Text size="T200" priority="300" style={{ whiteSpace: 'nowrap', flexShrink: 0 }}>
|
<Text size="T200" priority="300" style={{ whiteSpace: 'nowrap', flexShrink: 0 }}>
|
||||||
{formatSendAt(msg.sendAt)}
|
{formatSendAt(msg.sendAt)}
|
||||||
@@ -214,7 +250,7 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
size="300"
|
size="300"
|
||||||
radii="300"
|
radii="300"
|
||||||
variant="SurfaceVariant"
|
variant="SurfaceVariant"
|
||||||
aria-label="Edit scheduled message"
|
aria-label={`Edit scheduled message: ${rowDesc}`}
|
||||||
disabled={cancelling.has(msg.delayId)}
|
disabled={cancelling.has(msg.delayId)}
|
||||||
onClick={(e) => {
|
onClick={(e) => {
|
||||||
e.stopPropagation();
|
e.stopPropagation();
|
||||||
@@ -227,7 +263,7 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
size="300"
|
size="300"
|
||||||
radii="300"
|
radii="300"
|
||||||
variant="SurfaceVariant"
|
variant="SurfaceVariant"
|
||||||
aria-label="Cancel scheduled message"
|
aria-label={`Cancel scheduled message: ${rowDesc}`}
|
||||||
disabled={cancelling.has(msg.delayId)}
|
disabled={cancelling.has(msg.delayId)}
|
||||||
onClick={(e) => {
|
onClick={(e) => {
|
||||||
e.stopPropagation();
|
e.stopPropagation();
|
||||||
@@ -246,7 +282,8 @@ export function ScheduledMessagesTray({ roomId }: ScheduledMessagesTrayProps) {
|
|||||||
</Text>
|
</Text>
|
||||||
)}
|
)}
|
||||||
</Box>
|
</Box>
|
||||||
))}
|
);
|
||||||
|
})}
|
||||||
</Box>
|
</Box>
|
||||||
)}
|
)}
|
||||||
</Box>
|
</Box>
|
||||||
|
|||||||
Reference in New Issue
Block a user