From ef898710331ce8018dfa9b50b12a06377115d907 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 9 Jun 2026 22:32:25 +0000 Subject: [PATCH] Fix bugs and clean up dead code across block-email Bug fixes found in a package-wide review: Components - EmailContext: archiveThread no longer archives twice (the unconditional mutate ran in addition to mark-done, which also archives and whose undo couldn't undo the extra mutate; the else branch was a verbatim duplicate) - EmailContext: make the thread sort comparator transitive by comparing per-message timestamps (internal_date_ts ?? sent_at) instead of requiring both messages to share a field - Block: don't crash rendering threads whose first message has a null subject, and make the 'Email' title fallback reachable - Email: handleTargetMessage no longer double-scrolls (missing else before case 3), polling loops stop on unmount, dead checks removed, and firstUnreadMessageId stops re-sorting the already-sorted list - Email: fix vacuous isDraft check (draft !== null was always true) - EmailMessageBody: fall back to the full sanitized HTML when body_replyless is empty and there is no .macro_quote, instead of rendering a blank body; don't pass undefined body_text to StaticMarkdown - MessageActions: fix inverted allActionsHidden check (it tested whether hidden actions were valid, not whether all actions were hidden) - BaseInput: sends now honor the user-selected "from" inbox (form().selectedLinkId()) like draft saves already did, so the send goes out from the inbox shown in the UI - BaseInput: only clear the module-level undo-send restore slot if this instance still owns it (two inputs can be mounted at once) - BaseInput: re-attach onDirty/onReplyTypeApplied when the form memo re-keys after a send, so autosave keeps working without a remount - BaseInput: quote toggle works when replying to a thread's root message (guard on db_id, not replying_to_id) - BaseInput/Compose: failed schedule-send no longer leaves the composer stuck in a phantom "scheduled" state with Send disabled; schedule errors are caught and surfaced - date-selector: time input validation actually validates (Number.isNaN was called on strings) Utils - prepareEmailBody: body_text now includes all user content (was truncated to the first top-level element after paragraph flattening) while still excluding the quoted thread; reply header no longer renders "undefined" for a missing sender; malformed data-block-params no longer aborts send/draft-save; data-collapsed parsed as === 'true' instead of Boolean('false') being true; $findPreviousEmailNode returns instead of falling through when no ID is given - DocumentMentionNode: export the collapsed state as data-collapsed instead of a bogus DOMConversionMap attribute, so it survives HTML round trips - subjectText: null subjects no longer become "Re: null", and existing Re:/RE:/re: prefixes are detected case-insensitively at the start of the subject only (with tests) - emailUser/recipientConversion/mentionToCc: email comparisons are now case-insensitive via a shared emailsMatch helper, fixing "Me" labels, reply-all self-filtering and CC dedupe for case-differing addresses - plainTextToHtml: a blank source line renders as one blank line, not two - emailHotkeys: remove stubbed reply/reply-all/forward hotkeys that consumed keypresses and showed dead entries in the hotkey UI - scrollToMessage: check for message-not-found before applying the reversed index mapping; drop dead helpers Also removes dead code: _appendItemsAsMacroMentions, attachButtonRef plumbing in ComposeToolbar, unused bodyDiv ref in ComposeBody. https://claude.ai/code/session_01GJDgV1YUdUHqAB8GDXu5QQ --- .../block-email/component/BaseInput.tsx | 69 +++++++++++---- .../packages/block-email/component/Block.tsx | 7 +- .../packages/block-email/component/Email.tsx | 43 +++++---- .../block-email/component/EmailContext.tsx | 34 +++---- .../component/EmailMessageBody.tsx | 14 ++- .../block-email/component/MessageActions.tsx | 4 +- .../block-email/component/compose/Compose.tsx | 36 +++++--- .../component/compose/ComposeBody.tsx | 3 - .../component/compose/ComposeToolbar.tsx | 11 +-- .../block-email/component/date-selector.tsx | 12 +-- .../packages/block-email/util/emailHotkeys.ts | 39 +------- js/app/packages/block-email/util/emailUser.ts | 19 ++-- .../packages/block-email/util/mentionToCc.ts | 3 +- .../block-email/util/plainTextToHtml.test.ts | 8 +- .../block-email/util/plainTextToHtml.ts | 7 +- .../block-email/util/prepareEmailBody.ts | 88 +++++-------------- .../block-email/util/recipientConversion.ts | 28 +++--- .../block-email/util/scrollToMessage.ts | 40 ++------- .../block-email/util/subjectText.test.ts | 40 +++++++++ .../packages/block-email/util/subjectText.ts | 14 +-- js/lexical-core/nodes/DocumentMentionNode.ts | 2 +- 21 files changed, 247 insertions(+), 274 deletions(-) create mode 100644 js/app/packages/block-email/util/subjectText.test.ts diff --git a/js/app/packages/block-email/component/BaseInput.tsx b/js/app/packages/block-email/component/BaseInput.tsx index 8c173a6e3e..8fe5727ccf 100644 --- a/js/app/packages/block-email/component/BaseInput.tsx +++ b/js/app/packages/block-email/component/BaseInput.tsx @@ -501,7 +501,10 @@ export function BaseInput(props: { // Register a callback so stale undoSend closures from a previous mount can // restore state into this (the live) component instance. - restoreUndoCallback = (snapshot, draftId) => { + const ownRestoreUndoCallback: typeof restoreUndoCallback = ( + snapshot, + draftId + ) => { setSavedDraftId(draftId); const currentEditor = editor(); if (currentEditor && snapshot.bodyHtml) { @@ -511,8 +514,14 @@ export function BaseInput(props: { form().attachments.add(attachment); } }; + restoreUndoCallback = ownRestoreUndoCallback; onCleanup(() => { - restoreUndoCallback = null; + // Multiple BaseInputs can be mounted at once (bottom input + inline + // reply); only clear the slot if this instance still owns it so + // unmounting one doesn't break the other's undo-send restore. + if (restoreUndoCallback === ownRestoreUndoCallback) { + restoreUndoCallback = null; + } }); let pendingMentions: { documentId: string }[] = []; @@ -626,8 +635,11 @@ export function BaseInput(props: { } } - // Attach side-effect handlers on mount; they replay against current state - onMount(() => { + // Attach side-effect handlers; they replay against current state. Runs in + // an effect (not onMount) because form() re-keys when the reply context + // changes (e.g. after a send) and the new form instance needs the + // callbacks too. + createEffect(() => { form().setOnDirty(() => { scheduleDraftSave(); }); @@ -944,7 +956,11 @@ export function BaseInput(props: { return; } - let linkId: string | undefined = currentThread?.link_id; + // Same precedence as activeLinkId(): a user-selected "from" inbox wins + // over the thread's inbox, so sends go out from the inbox shown in the UI + // (and match where persistDraftOnSenderSwitch saved the draft). + let linkId: string | undefined = + form().selectedLinkId() ?? currentThread?.link_id ?? props.draft?.link_id; if (newMessage || !linkId) { if (emailLinksQuery.isPending) { toast.alert('Loading email accounts...'); @@ -963,7 +979,7 @@ export function BaseInput(props: { logger.error('No links found'); return; } - linkId = primaryLinkId() ?? linksData.links[0].id; + linkId = linkId ?? primaryLinkId() ?? linksData.links[0].id; } const currentEditor = editor(); @@ -1324,27 +1340,41 @@ export function BaseInput(props: { // Ensure draft is saved before scheduling const draftID = currentDraft ?? (await executeSaveDraft()); if (!draftID) { + // Clear the send time so a failed schedule doesn't leave the + // composer stuck in a phantom "scheduled" state with Send disabled + form().setSendTime(null); toast.failure('Failed to schedule message', { subtext: 'Draft required', }); return; } - await emailClient.scheduleMessage( - { - draftID, - send_time: date.toISOString(), - }, - headerLinkId() - ); + try { + await emailClient.scheduleMessage( + { + draftID, + send_time: date.toISOString(), + }, + headerLinkId() + ); + } catch (error) { + form().setSendTime(null); + logger.error(error); + toast.failure('Failed to schedule message'); + return; + } // Mark the thread as done const threadID = ctx.thread()?.db_id; if (threadID) { - await emailClient.flagArchived( - { id: threadID, value: true }, - headerLinkId() - ); + try { + await emailClient.flagArchived( + { id: threadID, value: true }, + headerLinkId() + ); + } catch (error) { + logger.error(error); + } } } }; @@ -1798,8 +1828,9 @@ export function BaseInput(props: { size="icon-sm" pressed={form().replyAppended()} onChange={() => { - const replyingToID = props.replyingTo()?.replying_to_id; - if (!replyingToID) return; + // Guard on db_id: a thread's root message has no + // replying_to_id but its quoted text can still be toggled + if (!props.replyingTo()?.db_id) return; const currentlyAppended = form().replyAppended(); form().setReplyAppended(!currentlyAppended); diff --git a/js/app/packages/block-email/component/Block.tsx b/js/app/packages/block-email/component/Block.tsx index 104c2b7687..7b2c9ae9f2 100644 --- a/js/app/packages/block-email/component/Block.tsx +++ b/js/app/packages/block-email/component/Block.tsx @@ -23,14 +23,15 @@ export default function BlockEmail() { const title = () => { const data = threadQuery.data; if (!data || !data.thread || data.thread.messages.length === 0) return ''; - if (data.thread.messages[0].subject?.length === 0) return '[No subject]'; + const subject = data.thread.messages[0].subject; + if (!subject) return '[No subject]'; // remove "re:" prefix(es) - return data.thread.messages[0].subject!.replace(/^(re:\s*)+/i, ''); + return subject.replace(/^(re:\s*)+/i, ''); }; return ( - +
diff --git a/js/app/packages/block-email/component/Email.tsx b/js/app/packages/block-email/component/Email.tsx index ec2f704187..9a9e24b6c2 100644 --- a/js/app/packages/block-email/component/Email.tsx +++ b/js/app/packages/block-email/component/Email.tsx @@ -25,6 +25,7 @@ import { createMemo, createSignal, Match, + onCleanup, onMount, Show, Switch, @@ -80,13 +81,18 @@ function EmailContent(props: EmailViewProps) { setIsScrolled(scrollFromTop > 1); }; + let disposed = false; + onCleanup(() => { + disposed = true; + }); + /** * Waits for the query to finish fetching */ const waitForQueryLoad = (): Promise => { return new Promise((resolve) => { const checkInterval = setInterval(() => { - if (!context.query.isFetching()) { + if (disposed || !context.query.isFetching()) { clearInterval(checkInterval); resolve(); } @@ -100,7 +106,7 @@ function EmailContent(props: EmailViewProps) { const loadMessagesUntilFound = async ( targetMessageId: string ): Promise => { - while (true) { + while (!disposed) { const messages = context.messages.unfiltered(); // Check if message exists in current batch @@ -117,6 +123,7 @@ function EmailContent(props: EmailViewProps) { context.query.fetchNextPage(); await waitForQueryLoad(); } + return false; }; /** @@ -187,21 +194,12 @@ function EmailContent(props: EmailViewProps) { performScrollToMessage(lastMessage.db_id, { behavior, focus }); }; + // context.messages.list() is already sorted oldest-first (see EmailContext) const firstUnreadMessageId = createMemo(() => { - const messages = context.messages.list().toSorted((a, b) => { - if (a.internal_date_ts && b.internal_date_ts) { - return ( - new Date(a.internal_date_ts).getTime() - - new Date(b.internal_date_ts).getTime() - ); - } else if (a.sent_at && b.sent_at) { - return new Date(a.sent_at).getTime() - new Date(b.sent_at).getTime(); - } - return 0; - }); - return messages?.find((m) => - m.labels.some((l) => l.provider_label_id === 'UNREAD') - )?.db_id; + return context.messages + .list() + .find((m) => m.labels.some((l) => l.provider_label_id === 'UNREAD')) + ?.db_id; }); const canRunInitialEmailScroll = () => @@ -220,8 +218,6 @@ function EmailContent(props: EmailViewProps) { // Check for target message const targetMessageId_ = context.messages.targetMessageID(); - if (targetMessageId_ && typeof targetMessageId_ !== 'string') return true; - if (targetMessageId_) { handleTargetMessage(targetMessageId_); } else { @@ -279,11 +275,12 @@ function EmailContent(props: EmailViewProps) { performScrollToMessage(messageId, { behavior: 'instant' }) ); } - // Case 3: Message is in current batch with sufficient context - setTimeout(() => - performScrollToMessage(messageId, { behavior: 'instant' }) - ); + else { + setTimeout(() => + performScrollToMessage(messageId, { behavior: 'instant' }) + ); + } } // If there is a focused message id, but it does not currently exist in the message list, it is because the user has just sent a message. When it does come into existence, we want to scroll to the bottom. @@ -527,7 +524,7 @@ function EmailContent(props: EmailViewProps) { title={props.title} isDraft={ emailReplyInfo()?.replyingTo == null && - emailReplyInfo()?.draft !== null + emailReplyInfo()?.draft != null } />
) { select(data) { const messages = data.pages.flatMap((t) => t.messages); - // Sort all messages by recency - messages.sort((a, b) => { - if (a.internal_date_ts && b.internal_date_ts) { - return ( - new Date(a.internal_date_ts).getTime() - - new Date(b.internal_date_ts).getTime() - ); - } - // Below is fallback for when internal_date_ts is not set - else if (a.sent_at && b.sent_at) { - return ( - new Date(a.sent_at).getTime() - new Date(b.sent_at).getTime() - ); - } - return 0; - }); + // Sort all messages by recency, falling back to sent_at when + // internal_date_ts is not set. Comparing per-message keys (instead of + // requiring both messages to have the same field) keeps the + // comparator transitive when timestamp availability is mixed. + const messageTime = (m: ApiMessage) => { + const ts = m.internal_date_ts ?? m.sent_at; + // Messages with no timestamp (e.g. fresh drafts) sort newest + return ts ? new Date(ts).getTime() : Number.POSITIVE_INFINITY; + }; + messages.sort((a, b) => messageTime(a) - messageTime(b)); const filtered = []; const messageDraftMap: Record = {}; @@ -341,14 +335,6 @@ export function EmailProvider(props: FlowProps<{ threadID: string }>) { if (!thread?.db_id) return false; - archiveMutation.mutate({ - threadId: thread.db_id, - archive: thread.inbox_visible, - linkId: toHeaderLinkId(thread.link_id), - }); - - if (!props) return false; - const selectedRow = soup?.items.get(thread.db_id); if (selectedRow) { diff --git a/js/app/packages/block-email/component/EmailMessageBody.tsx b/js/app/packages/block-email/component/EmailMessageBody.tsx index 4f4ab57145..663db7c54e 100644 --- a/js/app/packages/block-email/component/EmailMessageBody.tsx +++ b/js/app/packages/block-email/component/EmailMessageBody.tsx @@ -61,13 +61,11 @@ export function EmailMessageBody(props: EmailMessageBodyProps) { const styleTags = Array.from(doc.head?.querySelectorAll('style') ?? []) .map((style) => style.outerHTML) .join('\n'); - const quoted = doc.body.querySelector('.macro_quote'); - if (quoted) { - quoted?.remove(); - return styleTags - ? `${styleTags}\n${doc.body.innerHTML}` - : doc.body.innerHTML; - } + // If there is no quoted reply, the whole message is the replyless body + doc.body.querySelector('.macro_quote')?.remove(); + return styleTags + ? `${styleTags}\n${doc.body.innerHTML}` + : doc.body.innerHTML; } } return replyless; @@ -327,7 +325,7 @@ export function EmailMessageBody(props: EmailMessageBodyProps) { diff --git a/js/app/packages/block-email/component/MessageActions.tsx b/js/app/packages/block-email/component/MessageActions.tsx index 487af25f02..6e7cf5d9cd 100644 --- a/js/app/packages/block-email/component/MessageActions.tsx +++ b/js/app/packages/block-email/component/MessageActions.tsx @@ -28,8 +28,8 @@ export function MessageActions(props: { const canShowActions = () => { if (!props.showActions) return false; - const allActionsHidden = props.hiddenActions?.every((a) => - EMAIL_MESSAGE_ACTIONS.includes(a) + const allActionsHidden = EMAIL_MESSAGE_ACTIONS.every((a) => + props.hiddenActions?.includes(a) ); return !allActionsHidden; diff --git a/js/app/packages/block-email/component/compose/Compose.tsx b/js/app/packages/block-email/component/compose/Compose.tsx index a9dd7dbbf1..c768966f0d 100644 --- a/js/app/packages/block-email/component/compose/Compose.tsx +++ b/js/app/packages/block-email/component/compose/Compose.tsx @@ -496,26 +496,40 @@ export function EmailCompose(props: EmailComposeProps) { if (date) { const draftID = currentDraft ?? (await executeSaveDraft()); if (!draftID) { + // Clear the send time so a failed schedule doesn't leave the + // composer stuck in a phantom "scheduled" state with Send disabled + form.setSendTime(null); toast.failure('Failed to schedule message', { subtext: 'Draft required', }); return; } - await emailClient.scheduleMessage( - { - draftID, - send_time: date.toISOString(), - }, - headerLinkId() - ); + try { + await emailClient.scheduleMessage( + { + draftID, + send_time: date.toISOString(), + }, + headerLinkId() + ); + } catch (error) { + form.setSendTime(null); + logger.error(error); + toast.failure('Failed to schedule message'); + return; + } const threadID = saveDraftMutation.data?.draft.thread_db_id; if (threadID) { - await emailClient.flagArchived( - { id: threadID, value: true }, - headerLinkId() - ); + try { + await emailClient.flagArchived( + { id: threadID, value: true }, + headerLinkId() + ); + } catch (error) { + logger.error(error); + } } } }; diff --git a/js/app/packages/block-email/component/compose/ComposeBody.tsx b/js/app/packages/block-email/component/compose/ComposeBody.tsx index 8db2fb48d2..2d4ec3f323 100644 --- a/js/app/packages/block-email/component/compose/ComposeBody.tsx +++ b/js/app/packages/block-email/component/compose/ComposeBody.tsx @@ -84,8 +84,6 @@ export function ComposeBody(props: { ); }; - let bodyDiv!: HTMLDivElement; - const logComposeBody = (event: string, details?: Record) => { if (!props.debugName) return; logger.log(`[ComposeBody] ${event}`, { @@ -140,7 +138,6 @@ export function ComposeBody(props: {
{ editor()?.focus(); }} diff --git a/js/app/packages/block-email/component/compose/ComposeToolbar.tsx b/js/app/packages/block-email/component/compose/ComposeToolbar.tsx index 5016adfba4..a7e79cfea7 100644 --- a/js/app/packages/block-email/component/compose/ComposeToolbar.tsx +++ b/js/app/packages/block-email/component/compose/ComposeToolbar.tsx @@ -27,7 +27,6 @@ export function EmailComposeToolbar(props: { }) { const ctx = useCompose(); const [showFormatRibbon, setShowFormatRibbon] = createSignal(false); - let attachButtonRef!: HTMLDivElement; const handleAddAttachments = (files: File[]) => { const currentAttachments = ctx.attachments(); @@ -81,15 +80,12 @@ export function EmailComposeToolbar(props: { + } >
-
+