fix(webmail): Undo send really undoes, and a send is not made twice
The countdown sent from inside a state updater and Undo's button submitted the form, so Undo stopped the timer and then started the send over. The timer is now held in a ref, Undo is type=button, and leaving the screen flushes a pending send once. Snippets drop a stylesheet the preview was cut inside of. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
2080489319
commit
897f0122f4
5 files changed
+96
-11
No files matched your search
@@ -35,6 +35,17 @@
|
|||||||
"drafts"
|
"drafts"
|
||||||
],
|
],
|
||||||
"date": "2026-10-22"
|
"date": "2026-10-22"
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"error_message": "Undo send did not undo: the message was sent anyway (and twice in development)",
|
||||||
|
"root_cause": "The 10-second countdown sent the message from inside a setUndoSeconds updater. Undo only set the number to null; the interval kept running, its next tick saw prev === null and sent. React may also run an updater twice",
|
||||||
|
"fix": "The countdown is counted in a plain closure with the timer kept in pendingSend; handleUndo clears it; leaving the screen flushes a pending send once (and does not navigate). Covered by compose-draft-integrity.test.ts",
|
||||||
|
"tags": [
|
||||||
|
"webmail",
|
||||||
|
"compose",
|
||||||
|
"undo-send"
|
||||||
|
],
|
||||||
|
"date": "2026-10-22"
|
||||||
}
|
}
|
||||||
]
|
]
|
||||||
}
|
}
|
||||||
@@ -87,6 +87,11 @@ export function Composer({ initial, variant = "page", onSent, onDiscarded }: Com
|
|||||||
// way when that happens would otherwise land afterwards and bring the draft
|
// way when that happens would otherwise land afterwards and bring the draft
|
||||||
// back, so every save checks this before and after it talks to the server.
|
// back, so every save checks this before and after it talks to the server.
|
||||||
const discardedRef = useRef(false);
|
const discardedRef = useRef(false);
|
||||||
|
// The send waiting out its ten seconds, so Undo can stop it and leaving the
|
||||||
|
// screen cannot lose it.
|
||||||
|
const pendingSend = useRef<{ timer: ReturnType<typeof setInterval>; html: string } | null>(null);
|
||||||
|
const sendRef = useRef<(html: string) => Promise<void>>(async () => undefined);
|
||||||
|
const unmounted = useRef(false);
|
||||||
|
|
||||||
const editorRef = useRef<HTMLDivElement>(null);
|
const editorRef = useRef<HTMLDivElement>(null);
|
||||||
// The body, mirrored into React state.
|
// The body, mirrored into React state.
|
||||||
@@ -98,6 +103,21 @@ export function Composer({ initial, variant = "page", onSent, onDiscarded }: Com
|
|||||||
// that is why a draft left behind by a sent message was empty.
|
// that is why a draft left behind by a sent message was empty.
|
||||||
const [body, setBody] = useState("");
|
const [body, setBody] = useState("");
|
||||||
|
|
||||||
|
// Leaving with a send still counting down sends it now. Dropping it would lose
|
||||||
|
// a message the writer was told was on its way; Undo is the way to cancel.
|
||||||
|
useEffect(() => {
|
||||||
|
unmounted.current = false;
|
||||||
|
return () => {
|
||||||
|
unmounted.current = true;
|
||||||
|
const pending = pendingSend.current;
|
||||||
|
if (pending) {
|
||||||
|
clearInterval(pending.timer);
|
||||||
|
pendingSend.current = null;
|
||||||
|
void sendRef.current(pending.html);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
}, []);
|
||||||
|
|
||||||
// An inline reply is opened to be written in, so the cursor starts in it.
|
// An inline reply is opened to be written in, so the cursor starts in it.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (inline) editorRef.current?.focus();
|
if (inline) editorRef.current?.focus();
|
||||||
@@ -290,16 +310,24 @@ export function Composer({ initial, variant = "page", onSent, onDiscarded }: Com
|
|||||||
|
|
||||||
const bodyHtml = editorRef.current?.innerHTML || body;
|
const bodyHtml = editorRef.current?.innerHTML || body;
|
||||||
|
|
||||||
const interval = setInterval(() => {
|
// The countdown is counted here and not inside a state updater. The send
|
||||||
setUndoSeconds((prev) => {
|
// used to be a side effect of that updater, which React may run twice (the
|
||||||
if (prev === null || prev <= 1) {
|
// message went out twice) and which Undo could not stop: Undo only cleared
|
||||||
clearInterval(interval);
|
// the number, and the next tick - finding it empty - sent the message
|
||||||
void sendActualMessage(bodyHtml);
|
// anyway.
|
||||||
return null;
|
let remaining = 10;
|
||||||
|
const timer = setInterval(() => {
|
||||||
|
remaining -= 1;
|
||||||
|
if (remaining > 0) {
|
||||||
|
setUndoSeconds(remaining);
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
return prev - 1;
|
clearInterval(timer);
|
||||||
});
|
pendingSend.current = null;
|
||||||
|
setUndoSeconds(null);
|
||||||
|
void sendRef.current(bodyHtml);
|
||||||
}, 1000);
|
}, 1000);
|
||||||
|
pendingSend.current = { timer, html: bodyHtml };
|
||||||
};
|
};
|
||||||
|
|
||||||
const sendActualMessage = async (htmlBody: string) => {
|
const sendActualMessage = async (htmlBody: string) => {
|
||||||
@@ -329,14 +357,22 @@ export function Composer({ initial, variant = "page", onSent, onDiscarded }: Com
|
|||||||
setSending(false);
|
setSending(false);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
onSent();
|
// A flush after leaving must not drag the writer back to Sent.
|
||||||
|
if (!unmounted.current) onSent();
|
||||||
} catch {
|
} catch {
|
||||||
setSendError(t("errors.serverError"));
|
setSendError(t("errors.serverError"));
|
||||||
setSending(false);
|
setSending(false);
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
sendRef.current = sendActualMessage;
|
||||||
|
|
||||||
const handleUndo = () => {
|
const handleUndo = () => {
|
||||||
|
// Stops the timer itself: clearing the number on screen is not enough.
|
||||||
|
if (pendingSend.current) {
|
||||||
|
clearInterval(pendingSend.current.timer);
|
||||||
|
pendingSend.current = null;
|
||||||
|
}
|
||||||
setUndoSeconds(null);
|
setUndoSeconds(null);
|
||||||
setSending(false);
|
setSending(false);
|
||||||
};
|
};
|
||||||
@@ -412,7 +448,9 @@ export function Composer({ initial, variant = "page", onSent, onDiscarded }: Com
|
|||||||
remaining) (9s remaining)". */}
|
remaining) (9s remaining)". */}
|
||||||
<span>{t("mail.undoSend", { seconds: undoSeconds })}</span>
|
<span>{t("mail.undoSend", { seconds: undoSeconds })}</span>
|
||||||
</div>
|
</div>
|
||||||
<Button variant="primary" size="sm" onClick={handleUndo}>
|
{/* type="button": inside this form a button submits it, so Undo used to
|
||||||
|
stop the countdown and then send the message again. */}
|
||||||
|
<Button type="button" variant="primary" size="sm" onClick={handleUndo}>
|
||||||
<Undo2 className="h-3.5 w-3.5" />
|
<Undo2 className="h-3.5 w-3.5" />
|
||||||
{t("mail.undoSendAction")}
|
{t("mail.undoSendAction")}
|
||||||
</Button>
|
</Button>
|
||||||
|
|||||||
@@ -119,3 +119,32 @@ describe("discarding a draft", () => {
|
|||||||
expect(compose).toMatch(/discardedRef\.current && draftUidRef\.current > 0/);
|
expect(compose).toMatch(/discardedRef\.current && draftUidRef\.current > 0/);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("undoing a send", () => {
|
||||||
|
// The countdown sent the message from inside a state updater. Undo cleared the
|
||||||
|
// number on screen but not the timer, whose next tick found it empty and sent
|
||||||
|
// the message anyway - so "Undo" undid nothing. React may also run an updater
|
||||||
|
// twice, which sent it twice.
|
||||||
|
it("does not send from inside a state updater", () => {
|
||||||
|
const compose = code(COMPOSE);
|
||||||
|
expect(compose).not.toMatch(/setUndoSeconds\(\(prev\)/);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("stops the timer when the writer undoes", () => {
|
||||||
|
const compose = code(COMPOSE);
|
||||||
|
expect(compose).toMatch(/const handleUndo = \(\) => \{[\s\S]*?clearInterval\(pendingSend\.current\.timer\)/);
|
||||||
|
});
|
||||||
|
|
||||||
|
// A button inside a form submits it unless it says otherwise. Undo stopped the
|
||||||
|
// timer and then submitted the form, which started the send over.
|
||||||
|
it("is a button that does not submit the form", () => {
|
||||||
|
const compose = code(COMPOSE);
|
||||||
|
expect(compose).toMatch(/<Button type="button" variant="primary" size="sm" onClick=\{handleUndo\}>/);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("sends a message still counting down when the screen is left", () => {
|
||||||
|
const compose = code(COMPOSE);
|
||||||
|
expect(compose).toMatch(/sendRef\.current\(pending\.html\)/);
|
||||||
|
expect(compose).toMatch(/if \(!unmounted\.current\) onSent\(\)/);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -31,7 +31,10 @@ export function plainSnippet(raw: string | null | undefined): string {
|
|||||||
const withoutTags = raw
|
const withoutTags = raw
|
||||||
// A block boundary is a space, or "Hello,</p><p>You" would read "Hello,You".
|
// A block boundary is a space, or "Hello,</p><p>You" would read "Hello,You".
|
||||||
.replace(/<\/(?:p|div|li|tr|h[1-6])>|<br\s*\/?>/gi, " ")
|
.replace(/<\/(?:p|div|li|tr|h[1-6])>|<br\s*\/?>/gi, " ")
|
||||||
.replace(/<(style|script|head)\b[^>]*>[\s\S]*?<\/\1>/gi, " ")
|
// A stylesheet or script is not text. The preview is cut at a fixed
|
||||||
|
// length, so the closing tag may be missing: then everything after the
|
||||||
|
// opening one is the rule set, and all of it goes.
|
||||||
|
.replace(/<(style|script|head)\b[^>]*>[\s\S]*?(?:<\/\1>|$)/gi, " ")
|
||||||
.replace(/<[^>]*>/g, "")
|
.replace(/<[^>]*>/g, "")
|
||||||
// The server cuts the preview at a fixed length, which can leave the start
|
// The server cuts the preview at a fixed length, which can leave the start
|
||||||
// of a tag with no end ("...open</p").
|
// of a tag with no end ("...open</p").
|
||||||
|
|||||||
@@ -19,6 +19,10 @@ describe("plainSnippet", () => {
|
|||||||
it("drops a tag the preview was cut in the middle of", () => {
|
it("drops a tag the preview was cut in the middle of", () => {
|
||||||
expect(plainSnippet("<p>Open cyber model access</p")).toBe("Open cyber model access");
|
expect(plainSnippet("<p>Open cyber model access</p")).toBe("Open cyber model access");
|
||||||
});
|
});
|
||||||
|
it("drops a stylesheet the preview was cut inside of", () => {
|
||||||
|
expect(plainSnippet("<style>.card{border:2px solid #0a7;padding:16px")).toBe("");
|
||||||
|
expect(plainSnippet("<style>.a{color:red}</style><p>Hello")).toBe("Hello");
|
||||||
|
});
|
||||||
it("keeps an entity it does not know", () => {
|
it("keeps an entity it does not know", () => {
|
||||||
expect(plainSnippet("a &madeup; b")).toBe("a &madeup; b");
|
expect(plainSnippet("a &madeup; b")).toBe("a &madeup; b");
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in new issue
Block a user