Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a new block-email compose subsystem under Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
ehayes2000
force-pushed
the
email-compose-refactor
branch
from
69eadb7 to
02a494d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/block-email/component/compose/Compose.tsx`:
- Around line 521-552: The memo previewName calls useDisplayName inside its
callback (via useDisplayName(tryMacroId(...))) which creates new reactive
subscriptions each re-evaluation; move calls to useDisplayName out of the
createMemo and obtain display-name signals at component top-level (e.g. call
useDisplayName for each user id once and store their signal or a map of
id->signal), then have previewName (the createMemo) read those pre-created
signals (or a derived computed that reads the signal values) instead of
instantiating useDisplayName inside the mapping; update places referencing
tryMacroId(r.data.id) to use the precomputed display-name signal or cached value
so the memo only reads signals rather than creating subscriptions.
- Line 74: The module-level mutable variable undoComposeSnapshot (type
UndoComposeSnapshot) is shared across all EmailCompose instances and must be
replaced with a per-draft store; change the single variable to a Map<string,
UndoComposeSnapshot> (keyed by the compose/draft id) and update all places that
read/write undoComposeSnapshot to use map.get(draftId)/map.set(draftId,
snapshot) and to delete the entry on final send/cleanup; ensure EmailCompose (or
its props/state) provides the draft id used as the map key so snapshots are
isolated per compose instance.
- Around line 283-301: The undoSend function currently calls
emailClient.unscheduleMessage directly; replace that call with the instantiated
useUnscheduleMessageMutation and invoke its .mutate() (passing the same draftID
and relying on the hook's onSuccess/onError callbacks) while retaining the
subsequent UI updates (queryClient.invalidateQueries, replaceSplit,
toast.success/failure, invalidateSoupEntity). Likewise, update
handleSendTimeChange to stop calling emailClient.scheduleMessage and
emailClient.flagArchived directly and instead call .mutate() on the
corresponding useScheduleMessageMutation and useArchiveThreadMutation instances
(keeping existing success/error flows and UI updates intact).
In `@js/app/packages/block-email/component/compose/ComposeBody.tsx`:
- Around line 70-74: The code in ComposeBody.tsx is calling makeAttachmentPublic
directly (which uses storageServiceClient.getDocumentPermissions/editDocument)
instead of going through TanStack Query; create a mutation hook in the queries
package (e.g., useMakeAttachmentPublicMutation) that wraps the network logic now
inside makeAttachmentPublic, export it from queries, then replace direct calls
to makeAttachmentPublic in ComposeBody (the uploadedItemIds.forEach block) with
invoking the mutation (mutate or mutateAsync) for each itemId or a single
mutation that accepts an array, and handle success/error via the mutation
callbacks; ensure the mutation uses the same underlying storageServiceClient
methods but lives in queries so all network calls go through TanStack Query.
In `@js/app/packages/block-email/component/compose/ComposeLayout.tsx`:
- Line 12: The import for beveledCorners in ComposeLayout.tsx uses a bare path
'core/signal/beveledCorners' which is inconsistent with other '@' aliased
imports; update the import to use the '@' alias (i.e., import beveledCorners
from '@core/signal/beveledCorners') so it matches the project's path aliasing
and resolves consistently with the other imports in this file.
- Around line 164-182: The Show block conditional is already Show
when={!isMobile()}, so internal checks to isMobile() are redundant; simplify by
removing the nested isMobile() ternaries inside the StaticSplitLabel props: set
iconType to 'email' (or undefined only if you truly want no icon for mobiles)
and only include the badges array with the SplitHeaderBadge unconditionally in
this non-mobile branch (or conditionally include an empty array only if
necessary), updating references in the StaticSplitLabel usage (iconType, badges)
and keeping ctx.subject() || props.previewName?.() || 'Draft email' as the
label.
In `@js/app/packages/block-email/component/compose/ComposeRecipients.tsx`:
- Around line 31-33: The drag-leave handler is clearing isDragOver
unconditionally which causes flicker when moving between child elements; update
the onDragLeave/handleDragLeave in ComposeRecipients.tsx to inspect
event.relatedTarget (or event.nativeEvent?.relatedTarget) and only set
isDragOver = false when the relatedTarget is null or not contained within the
current row element (use rowRef.current.contains(relatedTarget) check),
otherwise do nothing so the drop highlight stays while moving between children.
In `@js/app/packages/block-email/component/compose/ComposeSubject.tsx`:
- Around line 10-26: The "Subject" caption must be a semantic label tied to the
input for accessibility: wrap or replace the plain caption with a <label>
element and link it to the text input by adding an id to the input (e.g.,
subject-input or generated via props.inputRef) and set the label's htmlFor to
that id; also when showing the validation error from
ctx.validationError('no_subject') add an id to the error container and set the
input's aria-describedby to that error id so screen readers will announce the
error; keep existing props.inputRef, ctx.subject()/ctx.setSubject, disabled, and
styling intact.
In `@js/app/packages/block-email/component/compose/ComposeToolbar.tsx`:
- Around line 22-27: Imports defaultSelectionData and
NODE_TRANSFORM/NodeTransformType use bare paths; update those import statements
in ComposeToolbar.tsx to the project's alias-based paths (e.g., prefix with '@'
to match the codebase alias convention) so defaultSelectionData and the
NODE_TRANSFORM/NodeTransformType import from the aliased module locations;
ensure the module specifiers exactly match other files' alias usage to avoid
mixed import styles and rebuild.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e681249-0eb1-489c-9020-bb3b52e7a89a
📒 Files selected for processing (11)
js/app/packages/app/component/split-layout/componentRegistry.tsxjs/app/packages/block-email/component/Email.tsxjs/app/packages/block-email/component/compose/Compose.tsxjs/app/packages/block-email/component/compose/ComposeBody.tsxjs/app/packages/block-email/component/compose/ComposeContext.tsjs/app/packages/block-email/component/compose/ComposeLayout.tsxjs/app/packages/block-email/component/compose/ComposeRecipients.tsxjs/app/packages/block-email/component/compose/ComposeSubject.tsxjs/app/packages/block-email/component/compose/ComposeToolbar.tsxjs/app/packages/block-email/component/compose/index.tsjs/app/packages/entity/src/components/CollapsibleList.tsx
| attachments: DraftFormAttachment[]; | ||
| }; | ||
|
|
||
| let undoComposeSnapshot: UndoComposeSnapshot | null = null; |
Contributor
There was a problem hiding this comment.
Module-level mutable state may cause issues with multiple compose instances.
undoComposeSnapshot is shared across all EmailCompose instances. If a user has multiple compose windows open (e.g., in different split panels), sending from one will overwrite the snapshot intended for another, potentially causing incorrect state restoration on undo.
Consider using a Map<string, UndoComposeSnapshot> keyed by draft ID, or managing this state through a more scoped mechanism.
🔧 Suggested approach
-let undoComposeSnapshot: UndoComposeSnapshot | null = null;
+const undoComposeSnapshots = new Map<string, UndoComposeSnapshot>();
// In onSubmit, when creating snapshot:
-undoComposeSnapshot = { draftId, ... };
+undoComposeSnapshots.set(draftId, { draftId, ... });
// When restoring:
-const restoredSnapshot = undoComposeSnapshot?.draftId === props.draftID ? undoComposeSnapshot : null;
+const restoredSnapshot = props.draftID ? undoComposeSnapshots.get(props.draftID) : null;
// After restoration:
-undoComposeSnapshot = null;
+if (props.draftID) undoComposeSnapshots.delete(props.draftID);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` at line 74, The
module-level mutable variable undoComposeSnapshot (type UndoComposeSnapshot) is
shared across all EmailCompose instances and must be replaced with a per-draft
store; change the single variable to a Map<string, UndoComposeSnapshot> (keyed
by the compose/draft id) and update all places that read/write
undoComposeSnapshot to use map.get(draftId)/map.set(draftId, snapshot) and to
delete the entry on final send/cleanup; ensure EmailCompose (or its props/state)
provides the draft id used as the map key so snapshots are isolated per compose
instance.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Let me pull the full usage of undoComposeSnapshot in the file to confirm the concern.
🏁 Script executed:
#!/bin/bash
rg -n "undoComposeSnapshot" --type ts --type tsx -A2 -B2
Length of output: 27
🏁 Script executed:
#!/bin/bash
rg -n "undoComposeSnapshot" -A2 -B2
Length of output: 1853
@sedson — yes, the concern is valid. Here's the precise failure scenario confirmed by the code:
- Compose A (draftId
"A") sends →undoComposeSnapshot = { draftId: "A", ... } - Compose B (draftId
"B") sends →undoComposeSnapshot = { draftId: "B", ... }(overwrites A's snapshot) - User clicks Undo on Compose A → line 131 checks
undoComposeSnapshot?.draftId === "A"→ it's"B", sorestoredSnapshotisnull→ undo silently no-ops for A
The existing draftId guard does prevent incorrect state from being restored to the wrong window, but only the most recently sent compose window can ever successfully undo. All earlier senders lose their undo capability.
The Map fix resolves this cleanly:
// Module level
const undoComposeSnapshots = new Map<string, UndoComposeSnapshot>();
// On send (line ~349):
undoComposeSnapshots.set(draftId, {
draftId,
recipients: structuredClone(unwrap(form.recipients())),
// ...
});
// On component init / restore (line ~131):
const restoredSnapshot = props.draftID
? (undoComposeSnapshots.get(props.draftID) ?? null)
: null;
if (restoredSnapshot) {
// restore state...
undoComposeSnapshots.delete(restoredSnapshot.draftId); // line ~140
}This ensures each draft ID has its own isolated undo snapshot and multiple concurrent compose windows all work correctly.
🧠 Learnings used
Learnt from: CR
Repo: macro-inc/macro PR: 0
File: js/app/AGENTS.md:0-0
Timestamp: 2026-03-22T01:23:43.487Z
Learning: Applies to js/app/packages/core/component/LexicalMarkdown/**/*.{ts,tsx} : When creating or making breaking changes to a Lexical Node, increment the lexical version counter in packages/core/component/LexicalMarkdown/version.ts with a brief note about changes
Learnt from: CR
Repo: macro-inc/macro PR: 0
File: js/app/AGENTS.md:0-0
Timestamp: 2026-03-22T01:23:43.487Z
Learning: Applies to js/app/**/*.{ts,tsx} : Make primitive UI components pure and prefer composition over props vs complex global state or context
Comment on lines
+283
to
+301
| const undoSend = async (draftId: string) => { | ||
| try { | ||
| await emailClient.unscheduleMessage({ draftID: draftId }); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: emailKeys.previews._def, | ||
| }); | ||
| replaceSplit({ | ||
| content: { | ||
| type: 'component', | ||
| id: 'email-compose', | ||
| params: { draftID: draftId }, | ||
| }, | ||
| }); | ||
| toast.success('Send cancelled'); | ||
| invalidateSoupEntity(draftId); | ||
| } catch { | ||
| toast.failure('Failed to undo send'); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if mutations exist for these operations in the queries package
rg -n "scheduleMessage|flagArchived" --type ts js/app/packages/queries/Repository: macro-inc/macro
Length of output: 834
🏁 Script executed:
# Get the full mutation hook signatures
rg -A 10 "export function useUnscheduleMessageMutation\|export function useScheduleMessageMutation" js/app/packages/queries/email/thread.tsRepository: macro-inc/macro
Length of output: 41
🏁 Script executed:
# Check if there's a mutation for flagArchived
rg -B 5 -A 10 "flagArchived" js/app/packages/queries/email/thread.ts | head -50Repository: macro-inc/macro
Length of output: 605
🏁 Script executed:
# Check what mutations are imported/used in Compose.tsx
rg -n "useUnschedule|useSchedule|useFlagArchived|from.*queries" js/app/packages/block-email/component/compose/Compose.tsxRepository: macro-inc/macro
Length of output: 498
🏁 Script executed:
# Verify the problematic lines in the file
sed -n '283,301p' js/app/packages/block-email/component/compose/Compose.tsxRepository: macro-inc/macro
Length of output: 578
🏁 Script executed:
# Check how unscheduleMessageMutation is used around line 437
sed -n '435,450p' js/app/packages/block-email/component/compose/Compose.tsxRepository: macro-inc/macro
Length of output: 510
🏁 Script executed:
# Get the complete mutation signature
sed -n '346,365p' js/app/packages/queries/email/thread.tsRepository: macro-inc/macro
Length of output: 617
🏁 Script executed:
# Check if there are mutations for scheduleMessage and flagArchived too
rg -B 2 "export function useScheduleMessageMutation\|export function useFlagArchived" js/app/packages/queries/email/thread.tsRepository: macro-inc/macro
Length of output: 41
🏁 Script executed:
# Search for all exported mutation functions in the thread.ts file
rg "export function use.*Mutation" js/app/packages/queries/email/thread.tsRepository: macro-inc/macro
Length of output: 275
🏁 Script executed:
# Check lines around 323 where scheduleMessage is called in thread.ts
sed -n '315,335p' js/app/packages/queries/email/thread.tsRepository: macro-inc/macro
Length of output: 687
🏁 Script executed:
# Check the handleSendTimeChange function to see if it uses mutations
sed -n '450,480p' js/app/packages/block-email/component/compose/Compose.tsxRepository: macro-inc/macro
Length of output: 790
Replace direct emailClient.unscheduleMessage call with the useUnscheduleMessageMutation hook.
The undoSend function violates the coding guideline by calling emailClient.unscheduleMessage directly. The proper mutation hook (useUnscheduleMessageMutation) is already imported and instantiated at line 437 with appropriate callbacks. Use the mutation's .mutate() method instead, consistent with the pattern used in handleSendTimeChange at line 459.
Additionally, handleSendTimeChange (lines 468 and 475) has the same issue with emailClient.scheduleMessage and emailClient.flagArchived—these should use useScheduleMessageMutation and useArchiveThreadMutation respectively.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 283 -
301, The undoSend function currently calls emailClient.unscheduleMessage
directly; replace that call with the instantiated useUnscheduleMessageMutation
and invoke its .mutate() (passing the same draftID and relying on the hook's
onSuccess/onError callbacks) while retaining the subsequent UI updates
(queryClient.invalidateQueries, replaceSplit, toast.success/failure,
invalidateSoupEntity). Likewise, update handleSendTimeChange to stop calling
emailClient.scheduleMessage and emailClient.flagArchived directly and instead
call .mutate() on the corresponding useScheduleMessageMutation and
useArchiveThreadMutation instances (keeping existing success/error flows and UI
updates intact).
Comment on lines
+521
to
+552
| const previewName = createMemo(() => { | ||
| const recipients = form.recipients().to; | ||
| if (recipients.length === 0) { | ||
| return 'Draft email'; | ||
| } | ||
|
|
||
| if (recipients.length === 1) { | ||
| let recipientName = recipients[0].data.email; | ||
|
|
||
| if (recipients[0].kind === 'user') { | ||
| recipientName = useDisplayName(tryMacroId(recipients[0].data.id))[0](); | ||
| } | ||
|
|
||
| return recipientName ? `Email to ${recipientName}` : 'Draft email'; | ||
| } | ||
|
|
||
| const names = recipients | ||
| .slice(0, 2) | ||
| .map((r) => { | ||
| if (r.kind === 'user') { | ||
| return useDisplayName(tryMacroId(r.data.id))[0](); | ||
| } | ||
| return r.data.email || 'Unknown'; | ||
| }) | ||
| .filter(Boolean); | ||
|
|
||
| if (recipients.length > 2) { | ||
| return `Email to ${names.join(', ')}, and others`; | ||
| } | ||
|
|
||
| return `Email to ${names.join(' and ')}`; | ||
| }); |
Contributor
There was a problem hiding this comment.
Calling useDisplayName inside memo may create subscription issues.
useDisplayName is called inside createMemo callbacks (lines 531, 541). In SolidJS, this creates new reactive subscriptions each time the memo re-evaluates, which may lead to memory leaks or unexpected behavior as old subscriptions aren't cleaned up.
Consider restructuring to call useDisplayName at the component's top level or using a different approach for deriving display names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 521 -
552, The memo previewName calls useDisplayName inside its callback (via
useDisplayName(tryMacroId(...))) which creates new reactive subscriptions each
re-evaluation; move calls to useDisplayName out of the createMemo and obtain
display-name signals at component top-level (e.g. call useDisplayName for each
user id once and store their signal or a map of id->signal), then have
previewName (the createMemo) read those pre-created signals (or a derived
computed that reads the signal values) instead of instantiating useDisplayName
inside the mapping; update places referencing tryMacroId(r.data.id) to use the
precomputed display-name signal or cached value so the memo only reads signals
rather than creating subscriptions.
Comment on lines
+70
to
+74
| (uploadedItemIds) => { | ||
| uploadedItemIds.forEach((itemId) => { | ||
| makeAttachmentPublic(itemId); | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify makeAttachmentPublic implementation calls service client directly
cat js/app/packages/block-email/util/makeAttachmentPublic.tsRepository: macro-inc/macro
Length of output: 1153
Move service client calls to TanStack Query mutations.
makeAttachmentPublic directly calls storageServiceClient.getDocumentPermissions() and storageServiceClient.editDocument(). All network calls must go through TanStack Query in the queries package. Create a mutation hook in the queries package and use it here instead of calling the service client directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeBody.tsx` around lines
70 - 74, The code in ComposeBody.tsx is calling makeAttachmentPublic directly
(which uses storageServiceClient.getDocumentPermissions/editDocument) instead of
going through TanStack Query; create a mutation hook in the queries package
(e.g., useMakeAttachmentPublicMutation) that wraps the network logic now inside
makeAttachmentPublic, export it from queries, then replace direct calls to
makeAttachmentPublic in ComposeBody (the uploadedItemIds.forEach block) with
invoking the mutation (mutate or mutateAsync) for each itemId or a single
mutation that accepts an array, and handle success/error via the mutation
callbacks; ensure the mutation uses the same underlying storageServiceClient
methods but lives in queries so all network calls go through TanStack Query.
| import { TOKENS } from '@core/hotkey/tokens'; | ||
| import { isMobile } from '@core/mobile/isMobile'; | ||
| import { WrapUnlessMobile } from '@core/mobile/WrapUnlessMobile'; | ||
| import { beveledCorners } from 'core/signal/beveledCorners'; |
Contributor
There was a problem hiding this comment.
Inconsistent import path alias.
This import uses a bare path 'core/signal/beveledCorners' while all other imports in this file use the @ alias prefix (e.g., @core/component/...). This should be consistent.
🔧 Suggested fix
-import { beveledCorners } from 'core/signal/beveledCorners';
+import { beveledCorners } from '@core/signal/beveledCorners';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { beveledCorners } from 'core/signal/beveledCorners'; | |
| import { beveledCorners } from '@core/signal/beveledCorners'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeLayout.tsx` at line 12,
The import for beveledCorners in ComposeLayout.tsx uses a bare path
'core/signal/beveledCorners' which is inconsistent with other '@' aliased
imports; update the import to use the '@' alias (i.e., import beveledCorners
from '@core/signal/beveledCorners') so it matches the project's path aliasing
and resolves consistently with the other imports in this file.
Comment on lines
+31
to
+33
| const handleDragLeave = () => { | ||
| setIsDragOver(false); | ||
| }; |
Contributor
There was a problem hiding this comment.
Prevent false drag-leave resets inside the same row
Line 31 currently clears isDragOver unconditionally, which can flicker the drop highlight when moving across child elements within the same row.
Suggested fix
- const handleDragLeave = () => {
- setIsDragOver(false);
- };
+ const handleDragLeave = (e: DragEvent) => {
+ const nextTarget = e.relatedTarget;
+ const current = e.currentTarget as HTMLElement | null;
+ if (current && nextTarget instanceof Node && current.contains(nextTarget)) {
+ return;
+ }
+ setIsDragOver(false);
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeRecipients.tsx` around
lines 31 - 33, The drag-leave handler is clearing isDragOver unconditionally
which causes flicker when moving between child elements; update the
onDragLeave/handleDragLeave in ComposeRecipients.tsx to inspect
event.relatedTarget (or event.nativeEvent?.relatedTarget) and only set
isDragOver = false when the relatedTarget is null or not contained within the
current row element (use rowRef.current.contains(relatedTarget) check),
otherwise do nothing so the drop highlight stays while moving between children.
Comment on lines
+10
to
+26
| <div class="w-full flex items-center gap-2 border-b border-edge-muted focus-within:border-accent py-2"> | ||
| <div class="shrink-0 text-ink-placeholder/70">Subject</div> | ||
| <div class="flex-1"> | ||
| <input | ||
| ref={props.inputRef} | ||
| type="text" | ||
| value={ctx.subject()} | ||
| placeholder="Subject" | ||
| class="w-full resize-none placeholder:text-ink-placeholder p-1 ml-1" | ||
| onInput={(e) => ctx.setSubject(e.currentTarget.value)} | ||
| disabled={ctx.disabled()} | ||
| /> | ||
| </div> | ||
| <Show when={ctx.validationError('no_subject')}> | ||
| {(err) => ( | ||
| <div class="text-failure-ink text-sm mt-1">{err().message}</div> | ||
| )} |
Contributor
There was a problem hiding this comment.
Associate the subject text with the input using a semantic <label>.
The current subject caption is not programmatically tied to the input, which weakens accessibility for assistive technologies.
Suggested fix
- <div class="w-full flex items-center gap-2 border-b border-edge-muted focus-within:border-accent py-2">
- <div class="shrink-0 text-ink-placeholder/70">Subject</div>
+ <div class="w-full flex items-center gap-2 border-b border-edge-muted focus-within:border-accent py-2">
+ <label for="compose-subject-input" class="shrink-0 text-ink-placeholder/70">
+ Subject
+ </label>
<div class="flex-1">
<input
+ id="compose-subject-input"
ref={props.inputRef}
type="text"
value={ctx.subject()}
placeholder="Subject"
class="w-full resize-none placeholder:text-ink-placeholder p-1 ml-1"
onInput={(e) => ctx.setSubject(e.currentTarget.value)}
disabled={ctx.disabled()}
/>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="w-full flex items-center gap-2 border-b border-edge-muted focus-within:border-accent py-2"> | |
| <div class="shrink-0 text-ink-placeholder/70">Subject</div> | |
| <div class="flex-1"> | |
| <input | |
| ref={props.inputRef} | |
| type="text" | |
| value={ctx.subject()} | |
| placeholder="Subject" | |
| class="w-full resize-none placeholder:text-ink-placeholder p-1 ml-1" | |
| onInput={(e) => ctx.setSubject(e.currentTarget.value)} | |
| disabled={ctx.disabled()} | |
| /> | |
| </div> | |
| <Show when={ctx.validationError('no_subject')}> | |
| {(err) => ( | |
| <div class="text-failure-ink text-sm mt-1">{err().message}</div> | |
| )} | |
| <div class="w-full flex items-center gap-2 border-b border-edge-muted focus-within:border-accent py-2"> | |
| <label for="compose-subject-input" class="shrink-0 text-ink-placeholder/70"> | |
| Subject | |
| </label> | |
| <div class="flex-1"> | |
| <input | |
| id="compose-subject-input" | |
| ref={props.inputRef} | |
| type="text" | |
| value={ctx.subject()} | |
| placeholder="Subject" | |
| class="w-full resize-none placeholder:text-ink-placeholder p-1 ml-1" | |
| onInput={(e) => ctx.setSubject(e.currentTarget.value)} | |
| disabled={ctx.disabled()} | |
| /> | |
| </div> | |
| <Show when={ctx.validationError('no_subject')}> | |
| {(err) => ( | |
| <div class="text-failure-ink text-sm mt-1">{err().message}</div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeSubject.tsx` around
lines 10 - 26, The "Subject" caption must be a semantic label tied to the input
for accessibility: wrap or replace the plain caption with a <label> element and
link it to the text input by adding an id to the input (e.g., subject-input or
generated via props.inputRef) and set the label's htmlFor to that id; also when
showing the validation error from ctx.validationError('no_subject') add an id to
the error container and set the input's aria-describedby to that error id so
screen readers will announce the error; keep existing props.inputRef,
ctx.subject()/ctx.setSubject, disabled, and styling intact.
Comment on lines
+22
to
+27
| import { defaultSelectionData } from 'core/component/LexicalMarkdown/plugins'; | ||
| import { | ||
| NODE_TRANSFORM, | ||
| type NodeTransformType, | ||
| } from 'core/component/LexicalMarkdown/plugins/node-transform/nodeTransformPlugin'; | ||
| import { |
Contributor
There was a problem hiding this comment.
Inconsistent import path aliases.
These imports use bare paths without the @ prefix, inconsistent with the rest of the codebase:
- Line 22:
'core/component/LexicalMarkdown/plugins' - Line 26-27:
'core/component/LexicalMarkdown/plugins/node-transform/nodeTransformPlugin'
🔧 Suggested fix
-import { defaultSelectionData } from 'core/component/LexicalMarkdown/plugins';
+import { defaultSelectionData } from '@core/component/LexicalMarkdown/plugins';
import {
NODE_TRANSFORM,
type NodeTransformType,
-} from 'core/component/LexicalMarkdown/plugins/node-transform/nodeTransformPlugin';
+} from '@core/component/LexicalMarkdown/plugins/node-transform/nodeTransformPlugin';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { defaultSelectionData } from 'core/component/LexicalMarkdown/plugins'; | |
| import { | |
| NODE_TRANSFORM, | |
| type NodeTransformType, | |
| } from 'core/component/LexicalMarkdown/plugins/node-transform/nodeTransformPlugin'; | |
| import { | |
| import { defaultSelectionData } from '@core/component/LexicalMarkdown/plugins'; | |
| import { | |
| NODE_TRANSFORM, | |
| type NodeTransformType, | |
| } from '@core/component/LexicalMarkdown/plugins/node-transform/nodeTransformPlugin'; | |
| import { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeToolbar.tsx` around
lines 22 - 27, Imports defaultSelectionData and NODE_TRANSFORM/NodeTransformType
use bare paths; update those import statements in ComposeToolbar.tsx to the
project's alias-based paths (e.g., prefix with '@' to match the codebase alias
convention) so defaultSelectionData and the NODE_TRANSFORM/NodeTransformType
import from the aliased module locations; ensure the module specifiers exactly
match other files' alias usage to avoid mixed import styles and rebuild.
02a494d to
b83d00d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
js/app/packages/block-email/component/compose/ComposeBody.tsx (1)
70-74:⚠️ Potential issue | 🟠 MajorDon't call the attachment permission helper directly from the component.
makeAttachmentPublicstill goes straight tostorageServiceClientinjs/app/packages/block-email/util/makeAttachmentPublic.ts:1-35, so this callback is doing network work outside thequeriespackage. Move this flow into a TanStack Query mutation and invoke that here instead of calling the utility directly.As per coding guidelines "All network calls to service clients MUST go through TanStack Query in the
queriespackage. Do NOT call service clients directly from components or other packages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/compose/ComposeBody.tsx` around lines 70 - 74, The component is calling makeAttachmentPublic directly (in the uploadedItemIds callback inside ComposeBody), which performs network I/O; move this work into a TanStack Query mutation in the queries package and call that mutation from the component instead of makeAttachmentPublic. Create a mutation (e.g., useMakeAttachmentPublicMutation) in the queries package that wraps the existing makeAttachmentPublic logic (or calls storageServiceClient) so all network access is centralized, then replace the uploadedItemIds.forEach(...) call in ComposeBody with invoking that mutation (pass itemId(s) and handle optimistic updates/errors via the mutation's onSuccess/onError). Ensure the component imports and uses the new mutation hook rather than calling makeAttachmentPublic directly.js/app/packages/block-email/component/compose/ComposeSubject.tsx (1)
10-27:⚠️ Potential issue | 🟠 MajorWire the subject caption and validation text to the input.
This still uses a plain
<div>for the field label, and theno_subjecterror is not referenced from the<input>. Screen readers will not reliably announce the subject field name or its validation state here, and the error node is currently laid out in the same horizontal row as the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/compose/ComposeSubject.tsx` around lines 10 - 27, Replace the plain div label and inline error with accessible label and error associations: give the input in ComposeSubject.tsx a unique id (e.g., subjectInputId) and replace the "Subject" div with a <label htmlFor={subjectInputId}> so screen readers announce the field; move the Show block so the validation text renders below the input and give it an id (e.g., subjectErrorId) and set input attributes aria-describedby={ctx.validationError('no_subject') ? subjectErrorId : undefined} and aria-invalid={!!ctx.validationError('no_subject')}; continue using props.inputRef for the ref and ctx.setSubject/ctx.subject for value handling.js/app/packages/block-email/component/compose/ComposeLayout.tsx (2)
12-12: 🧹 Nitpick | 🔵 TrivialUse the
@corealias here for consistency.This bare
core/...import is still the odd path in the file and makes the dependency boundary harder to read than the rest of the@core/...imports.Suggested fix
-import { beveledCorners } from 'core/signal/beveledCorners'; +import { beveledCorners } from '@core/signal/beveledCorners';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/compose/ComposeLayout.tsx` at line 12, Replace the bare import path for beveledCorners with the project alias: change the import that references "core/signal/beveledCorners" to use the "@core" alias so it reads the same style as other imports (i.e., import { beveledCorners } from '@core/signal/beveledCorners'); update the import statement where beveledCorners is referenced in ComposeLayout.tsx to keep consistency across the file.
164-179: 🧹 Nitpick | 🔵 TrivialRemove the unreachable mobile branches from the desktop header.
This block is already gated by
Show when={!isMobile()}, soiconTypeandbadgeswill never take the mobile path. Flattening it removes dead code and avoids the extra conditional work here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/compose/ComposeLayout.tsx` around lines 164 - 179, The desktop header inside ComposeLayout.tsx is wrapped with Show when={!isMobile()}, so remove the unreachable mobile branches in the StaticSplitLabel props: replace the ternary for iconType and the badges ternary with the desktop-only values (set iconType to 'email' and badges to the array containing the SplitHeaderBadge). Update references in the StaticSplitLabel invocation (iconType, badges, and the SplitHeaderBadge usage) and remove any isMobile() checks within this block to flatten the code.js/app/packages/block-email/component/compose/ComposeRecipients.tsx (1)
31-33:⚠️ Potential issue | 🟡 MinorDon't clear the drop state on internal drag transitions.
handleDragLeaveresetsisDragOverunconditionally, so the row highlight will flicker when the cursor moves between child elements inside the same recipient row. Guard this with arelatedTargetcontainment check before clearing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/compose/ComposeRecipients.tsx` around lines 31 - 33, handleDragLeave currently clears isDragOver unconditionally causing flicker when dragging between child elements; change handleDragLeave to accept the drag event (e.g., e: React.DragEvent) and only call setIsDragOver(false) when the event.relatedTarget is null or is not contained within the same recipient row element by checking e.currentTarget.contains(e.relatedTarget as Node). Update the handler signature where it's attached to the recipient row so the containment check prevents clearing the highlight on internal transitions.js/app/packages/block-email/component/compose/Compose.tsx (2)
51-52:⚠️ Potential issue | 🟠 MajorMove these email mutations back behind
@queries.
undoSend()andhandleSendTimeChange()still callemailClientdirectly from the component. That bypasses the mutation layer this file already uses elsewhere, so cache invalidation and error handling can drift from the rest of compose. As per coding guidelines "All network calls to service clients MUST go through TanStack Query in thequeriespackage. Do NOT call service clients directly from components or other packages."Also applies to: 285-303, 470-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 51 - 52, undoSend() and handleSendTimeChange() are calling emailClient directly from the component which bypasses the existing TanStack Query mutation layer; move the network calls into the queries package as mutations (e.g., add mutations like undoSendEmail and updateSendTime in `@queries`) and replace direct emailClient usage in Compose.tsx with calls to those mutations, ensuring you reuse the same error handling and cache invalidation logic (invalidate relevant queries) used by the other compose mutations so behavior is consistent with the rest of the component.
74-75:⚠️ Potential issue | 🟠 MajorScope undo snapshots per compose instance.
undoComposeSnapshotis still a module singleton. If two compose panes are open, sending from one can overwrite the snapshot the other instance needs for undo, and the restore path will hydrate whichever draft wrote last. This needs to be instance-local or keyed bydraftId.Also applies to: 129-140, 347-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 74 - 75, undoComposeSnapshot is a module-level singleton causing cross-instance clobbering; make undo snapshots instance-scoped by replacing the single let undoComposeSnapshot: UndoComposeSnapshot | null = null with a per-compose map or instance ref keyed by draftId (e.g., const undoComposeSnapshots = new Map<string,UndoComposeSnapshot>() or useRef<Record<string,UndoComposeSnapshot|null>>()), then update all places that read/write undoComposeSnapshot (the compose snapshot set/restore code paths referenced around the existing uses) to read/write using the current compose's draftId (get/set/delete by key) or use the component instance ref so each Compose instance only touches its own snapshot. Ensure cleanup on unmount/send by deleting the entry for that draftId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/block-email/component/compose/Compose.tsx`:
- Around line 415-432: The send path calls sendMutation.mutate with db_id:
currentDraftID() without ensuring pending local attachments are saved/uploaded,
so attachments can be lost; modify the send flow (the code around
sendMutation.mutate) to first await executeSaveDraft() and the upload process
exposed by useUploadDraftAttachmentsMutation (or wait for its settled/fulfilled
state) so all local attachments are uploaded to the server before calling
sendMutation.mutate; ensure you reference and wait for currentDraftID() only
after executeSaveDraft() completes and the upload mutation has finished so the
sent message includes server-visible attachments.
- Around line 175-226: executeSaveDraft can run concurrently because it only
checks sendMutation.isPending; add serialization so a second debounced call
doesn't start while a save/upload is in flight. Implement a simple reentrancy
guard (e.g., a local boolean inFlightSave) or check saveDraftMutation.isPending
|| uploadAttachmentMutation.isPending at the top of executeSaveDraft and return
early if true; set/clear the guard around the async work (use try/finally).
Update scheduleDraftSave to call executeSaveDraft as before; ensure you still
clear/set currentDraftID() only after the awaited save/upload completes.
Reference executeSaveDraft, scheduleDraftSave, saveDraftMutation,
uploadAttachmentMutation, sendMutation, currentDraftID, and setCurrentDraftID.
- Around line 359-435: The watermark cleanup is only called after
sendMutation.mutate, so validation returns leave the watermark in the editor; to
fix, ensure cleanupWatermark() always runs on early returns by moving the
watermark append (call to $appendWatermarkNodeToLast/current cleanupWatermark
variable) to after all validation checks (after checks using prepareEmailBody,
recipients/form.subject()/form.sendTime(), currentLink) or wrap the entire
validation + send block in try/finally and call cleanupWatermark() in the
finally block; update the function containing prepareEmailBody,
setValidationError, and sendMutation.mutate so that cleanupWatermark is always
invoked regardless of validation failure or mutation path.
- Around line 449-478: The code updates form.sendTime() before network
operations succeed in handleSendTimeChange; instead, remove the preemptive
form.setSendTime(date) calls and only update local state after successful
backend confirmation: for the unschedule path, call
unscheduleMessageMutation.mutate and in its onSuccess handler call
form.setSendTime(null) and in onError do not change the form (or restore
previous value); for the schedule path, await executeSaveDraft() and
emailClient.scheduleMessage(...) (and emailClient.flagArchived(...)) and only
after those succeed call form.setSendTime(date); on any error show the
toast.failure and do not modify form.sendTime so UI always reflects server state
(use the existing functions: handleSendTimeChange, form.setSendTime,
unscheduleMessageMutation.mutate, executeSaveDraft, emailClient.scheduleMessage,
saveDraftMutation, emailClient.flagArchived).
- Around line 189-194: Autosave is clearing a scheduled send because
saveDraftMutation.mutateAsync is sending a draft without the current form's send
time, which causes useSaveDraftMutation to serialize send_time as null; update
the object passed to saveDraftMutation.mutateAsync (the draft built from
draftToSave and db_id/currentDraftID()) to include the scheduled send value from
form.sendTime() when present (e.g., set sendTime or send_time from
form.sendTime()) so the scheduled send is preserved across autosaves.
In `@js/app/packages/block-email/component/compose/ComposeBody.tsx`:
- Around line 121-128: The handlers onFocusLeaveStart and onFocusLeaveEnd in
ComposeBody.tsx call e.preventDefault() unconditionally and can swallow
Tab/Shift+Tab when no sibling or split-panel target exists; change them to call
focusSibling('prev') / focusSibling('next') first (or check its return) and only
call e.preventDefault() when focusSibling actually found and moved focus (i.e.,
when a target exists), so normal tab navigation is preserved when no
sibling/split-panel context is present.
---
Duplicate comments:
In `@js/app/packages/block-email/component/compose/Compose.tsx`:
- Around line 51-52: undoSend() and handleSendTimeChange() are calling
emailClient directly from the component which bypasses the existing TanStack
Query mutation layer; move the network calls into the queries package as
mutations (e.g., add mutations like undoSendEmail and updateSendTime in
`@queries`) and replace direct emailClient usage in Compose.tsx with calls to
those mutations, ensuring you reuse the same error handling and cache
invalidation logic (invalidate relevant queries) used by the other compose
mutations so behavior is consistent with the rest of the component.
- Around line 74-75: undoComposeSnapshot is a module-level singleton causing
cross-instance clobbering; make undo snapshots instance-scoped by replacing the
single let undoComposeSnapshot: UndoComposeSnapshot | null = null with a
per-compose map or instance ref keyed by draftId (e.g., const
undoComposeSnapshots = new Map<string,UndoComposeSnapshot>() or
useRef<Record<string,UndoComposeSnapshot|null>>()), then update all places that
read/write undoComposeSnapshot (the compose snapshot set/restore code paths
referenced around the existing uses) to read/write using the current compose's
draftId (get/set/delete by key) or use the component instance ref so each
Compose instance only touches its own snapshot. Ensure cleanup on unmount/send
by deleting the entry for that draftId.
In `@js/app/packages/block-email/component/compose/ComposeBody.tsx`:
- Around line 70-74: The component is calling makeAttachmentPublic directly (in
the uploadedItemIds callback inside ComposeBody), which performs network I/O;
move this work into a TanStack Query mutation in the queries package and call
that mutation from the component instead of makeAttachmentPublic. Create a
mutation (e.g., useMakeAttachmentPublicMutation) in the queries package that
wraps the existing makeAttachmentPublic logic (or calls storageServiceClient) so
all network access is centralized, then replace the uploadedItemIds.forEach(...)
call in ComposeBody with invoking that mutation (pass itemId(s) and handle
optimistic updates/errors via the mutation's onSuccess/onError). Ensure the
component imports and uses the new mutation hook rather than calling
makeAttachmentPublic directly.
In `@js/app/packages/block-email/component/compose/ComposeLayout.tsx`:
- Line 12: Replace the bare import path for beveledCorners with the project
alias: change the import that references "core/signal/beveledCorners" to use the
"@core" alias so it reads the same style as other imports (i.e., import {
beveledCorners } from '@core/signal/beveledCorners'); update the import
statement where beveledCorners is referenced in ComposeLayout.tsx to keep
consistency across the file.
- Around line 164-179: The desktop header inside ComposeLayout.tsx is wrapped
with Show when={!isMobile()}, so remove the unreachable mobile branches in the
StaticSplitLabel props: replace the ternary for iconType and the badges ternary
with the desktop-only values (set iconType to 'email' and badges to the array
containing the SplitHeaderBadge). Update references in the StaticSplitLabel
invocation (iconType, badges, and the SplitHeaderBadge usage) and remove any
isMobile() checks within this block to flatten the code.
In `@js/app/packages/block-email/component/compose/ComposeRecipients.tsx`:
- Around line 31-33: handleDragLeave currently clears isDragOver unconditionally
causing flicker when dragging between child elements; change handleDragLeave to
accept the drag event (e.g., e: React.DragEvent) and only call
setIsDragOver(false) when the event.relatedTarget is null or is not contained
within the same recipient row element by checking
e.currentTarget.contains(e.relatedTarget as Node). Update the handler signature
where it's attached to the recipient row so the containment check prevents
clearing the highlight on internal transitions.
In `@js/app/packages/block-email/component/compose/ComposeSubject.tsx`:
- Around line 10-27: Replace the plain div label and inline error with
accessible label and error associations: give the input in ComposeSubject.tsx a
unique id (e.g., subjectInputId) and replace the "Subject" div with a <label
htmlFor={subjectInputId}> so screen readers announce the field; move the Show
block so the validation text renders below the input and give it an id (e.g.,
subjectErrorId) and set input attributes
aria-describedby={ctx.validationError('no_subject') ? subjectErrorId :
undefined} and aria-invalid={!!ctx.validationError('no_subject')}; continue
using props.inputRef for the ref and ctx.setSubject/ctx.subject for value
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff152609-7825-41bb-9075-23679fe2b033
📒 Files selected for processing (13)
js/app/packages/app/component/split-layout/componentRegistry.tsxjs/app/packages/block-email/component/Compose.tsxjs/app/packages/block-email/component/ComposeEmailInput.tsxjs/app/packages/block-email/component/Email.tsxjs/app/packages/block-email/component/compose/Compose.tsxjs/app/packages/block-email/component/compose/ComposeBody.tsxjs/app/packages/block-email/component/compose/ComposeContext.tsjs/app/packages/block-email/component/compose/ComposeLayout.tsxjs/app/packages/block-email/component/compose/ComposeRecipients.tsxjs/app/packages/block-email/component/compose/ComposeSubject.tsxjs/app/packages/block-email/component/compose/ComposeToolbar.tsxjs/app/packages/block-email/component/compose/index.tsjs/app/packages/entity/src/components/CollapsibleList.tsx
💤 Files with no reviewable changes (2)
- js/app/packages/block-email/component/Compose.tsx
- js/app/packages/block-email/component/ComposeEmailInput.tsx
Comment on lines
+175
to
+226
| async function executeSaveDraft() { | ||
| if (sendMutation.isPending) { | ||
| return; | ||
| } | ||
| const draftToSave = collectDraft(); | ||
| if (!draftToSave) { | ||
| const draftID = currentDraftID(); | ||
| if (draftID) { | ||
| await deleteDraftMutation.mutateAsync({ draftId: draftID }); | ||
| } | ||
| setCurrentDraftID(undefined); | ||
| return; | ||
| } | ||
|
|
||
| const draftResponse = await saveDraftMutation.mutateAsync({ | ||
| draft: { | ||
| ...draftToSave, | ||
| db_id: currentDraftID(), | ||
| }, | ||
| }); | ||
|
|
||
| const draftId = draftResponse.draft.db_id; | ||
| if (draftId) { | ||
| const attachments = form.attachments | ||
| .list() | ||
| .filter((a) => a.type === 'local' && !a.attachmentID) as Extract< | ||
| DraftFormAttachment, | ||
| { type: 'local' } | ||
| >[]; | ||
|
|
||
| if (attachments.length) { | ||
| const uploaded = await uploadAttachmentMutation.mutateAsync({ | ||
| draftID: draftId, | ||
| attachments: attachments.map((a) => a.file), | ||
| }); | ||
|
|
||
| for (const attachment of uploaded.attachments) { | ||
| form.attachments.assignAttachmentID( | ||
| attachment.file, | ||
| attachment.attachmentID | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| setCurrentDraftID(draftId); | ||
| return draftId; | ||
| } | ||
| } | ||
|
|
||
| const scheduleDraftSave = debounce(() => { | ||
| void executeSaveDraft(); | ||
| }, DRAFT_DEBOUNCE_MS); |
Contributor
There was a problem hiding this comment.
Serialize draft saves.
executeSaveDraft() only guards against sendMutation.isPending. A second debounced run can still start while saveDraftMutation or uploadAttachmentMutation is in flight, and currentDraftID() stays stale until that first round finishes. On a new compose, that can create multiple drafts and race attachment uploads across them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 175 -
226, executeSaveDraft can run concurrently because it only checks
sendMutation.isPending; add serialization so a second debounced call doesn't
start while a save/upload is in flight. Implement a simple reentrancy guard
(e.g., a local boolean inFlightSave) or check saveDraftMutation.isPending ||
uploadAttachmentMutation.isPending at the top of executeSaveDraft and return
early if true; set/clear the guard around the async work (use try/finally).
Update scheduleDraftSave to call executeSaveDraft as before; ensure you still
clear/set currentDraftID() only after the awaited save/upload completes.
Reference executeSaveDraft, scheduleDraftSave, saveDraftMutation,
uploadAttachmentMutation, sendMutation, currentDraftID, and setCurrentDraftID.
Comment on lines
+189
to
+194
| const draftResponse = await saveDraftMutation.mutateAsync({ | ||
| draft: { | ||
| ...draftToSave, | ||
| db_id: currentDraftID(), | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Preserve the scheduled send time on autosave.
useSaveDraftMutation serializes an omitted sendTime as send_time: null in js/app/packages/queries/email/draft.ts:1-30. After a draft has been scheduled, the next autosave here will clear that schedule unless the current form.sendTime() is passed through.
🛠️ Minimal fix
const draftResponse = await saveDraftMutation.mutateAsync({
draft: {
...draftToSave,
db_id: currentDraftID(),
},
+ sendTime: form.sendTime() ?? null,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 189 -
194, Autosave is clearing a scheduled send because saveDraftMutation.mutateAsync
is sending a draft without the current form's send time, which causes
useSaveDraftMutation to serialize send_time as null; update the object passed to
saveDraftMutation.mutateAsync (the draft built from draftToSave and
db_id/currentDraftID()) to include the scheduled send value from form.sendTime()
when present (e.g., set sendTime or send_time from form.sendTime()) so the
scheduled send is preserved across autosaves.
Comment on lines
+359
to
+435
| const cleanupWatermark = $appendWatermarkNodeToLast( | ||
| currentEditor, | ||
| !hasPaidAccess() ? MACRO_EMAIL_SIGNATURE : undefined | ||
| ); | ||
|
|
||
| const prepared = prepareEmailBody(currentEditor, undefined); | ||
| if (!prepared) return; | ||
|
|
||
| const bodyMacro = content(); | ||
|
|
||
| const data = { | ||
| text: prepared.bodyText, | ||
| html: prepared.bodyHtml, | ||
| raw: bodyMacro, | ||
| }; | ||
|
|
||
| const currentLink = link(); | ||
| const recipients = form.recipients(); | ||
|
|
||
| if (!recipients.to.length) { | ||
| setValidationError({ | ||
| type: 'no_recipient', | ||
| message: 'Please select at least one recipient', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!data.raw.trim()) { | ||
| setValidationError({ | ||
| type: 'no_message', | ||
| message: 'Please enter a message', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!form.subject()?.trim()) { | ||
| setValidationError({ | ||
| type: 'no_subject', | ||
| message: 'Please enter a subject', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!currentLink) { | ||
| setValidationError({ | ||
| type: 'no_link', | ||
| message: 'Unable to find linked email account', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Failsafe: don't send if a scheduled send time is set | ||
| if (form.sendTime()) { | ||
| return; | ||
| } | ||
|
|
||
| sendMutation.mutate({ | ||
| message: { | ||
| to: convertToContactInfoArray(recipients.to), | ||
| cc: | ||
| recipients.cc.length > 0 | ||
| ? convertToContactInfoArray(recipients.cc) | ||
| : [], | ||
| bcc: | ||
| recipients.bcc.length > 0 | ||
| ? convertToContactInfoArray(recipients.bcc) | ||
| : [], | ||
| subject: form.subject(), | ||
| body_text: data.text, | ||
| body_html: data.html, | ||
| body_macro: data.raw, | ||
| db_id: currentDraftID(), | ||
| }, | ||
| }); | ||
|
|
||
| cleanupWatermark(); | ||
| }; |
Contributor
There was a problem hiding this comment.
Always clean up the watermark on failed sends.
cleanupWatermark() only runs after sendMutation.mutate(). Any validation return before that leaves the watermark node in the live editor, so a failed send attempt injects the signature into the draft until the user edits or autosave strips it. Move watermark insertion after validation, or wrap this block in try/finally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 359 -
435, The watermark cleanup is only called after sendMutation.mutate, so
validation returns leave the watermark in the editor; to fix, ensure
cleanupWatermark() always runs on early returns by moving the watermark append
(call to $appendWatermarkNodeToLast/current cleanupWatermark variable) to after
all validation checks (after checks using prepareEmailBody,
recipients/form.subject()/form.sendTime(), currentLink) or wrap the entire
validation + send block in try/finally and call cleanupWatermark() in the
finally block; update the function containing prepareEmailBody,
setValidationError, and sendMutation.mutate so that cleanupWatermark is always
invoked regardless of validation failure or mutation path.
Comment on lines
+415
to
+432
| sendMutation.mutate({ | ||
| message: { | ||
| to: convertToContactInfoArray(recipients.to), | ||
| cc: | ||
| recipients.cc.length > 0 | ||
| ? convertToContactInfoArray(recipients.cc) | ||
| : [], | ||
| bcc: | ||
| recipients.bcc.length > 0 | ||
| ? convertToContactInfoArray(recipients.bcc) | ||
| : [], | ||
| subject: form.subject(), | ||
| body_text: data.text, | ||
| body_html: data.html, | ||
| body_macro: data.raw, | ||
| db_id: currentDraftID(), | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Upload pending local attachments before sending.
This path sends immediately with db_id: currentDraftID() but never waits for executeSaveDraft(). Local files only become server-visible through useUploadDraftAttachmentsMutation, and the send payload here does not include attachment data, so a user can lose newly added attachments by clicking Send before the debounced save/upload finishes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 415 -
432, The send path calls sendMutation.mutate with db_id: currentDraftID()
without ensuring pending local attachments are saved/uploaded, so attachments
can be lost; modify the send flow (the code around sendMutation.mutate) to first
await executeSaveDraft() and the upload process exposed by
useUploadDraftAttachmentsMutation (or wait for its settled/fulfilled state) so
all local attachments are uploaded to the server before calling
sendMutation.mutate; ensure you reference and wait for currentDraftID() only
after executeSaveDraft() completes and the upload mutation has finished so the
sent message includes server-visible attachments.
Comment on lines
+449
to
+478
| const handleSendTimeChange = async (date: Date | null) => { | ||
| const currentSendTime = form.sendTime(); | ||
| const currentDraft = currentDraftID(); | ||
|
|
||
| if (!date && currentSendTime && currentDraft) { | ||
| unscheduleMessageMutation.mutate({ | ||
| draftID: currentDraft, | ||
| }); | ||
| form.setSendTime(date); | ||
| return; | ||
| } | ||
|
|
||
| form.setSendTime(date); | ||
|
|
||
| if (date) { | ||
| const draftID = currentDraft ?? (await executeSaveDraft()); | ||
| if (!draftID) { | ||
| toast.failure('Failed to schedule message', 'Draft required'); | ||
| return; | ||
| } | ||
|
|
||
| await emailClient.scheduleMessage({ | ||
| draftID, | ||
| send_time: date.toISOString(), | ||
| }); | ||
|
|
||
| const threadID = saveDraftMutation.data?.draft.thread_db_id; | ||
| if (threadID) { | ||
| await emailClient.flagArchived({ id: threadID, value: true }); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t commit sendTime until the backend accepts it.
Both schedule branches update form.sendTime() before the network work succeeds. If unschedule fails, or draft save / schedule fails, the UI still shows a scheduled or unscheduled state that the server does not match. Apply the local state change from mutation success, or roll it back on error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 449 -
478, The code updates form.sendTime() before network operations succeed in
handleSendTimeChange; instead, remove the preemptive form.setSendTime(date)
calls and only update local state after successful backend confirmation: for the
unschedule path, call unscheduleMessageMutation.mutate and in its onSuccess
handler call form.setSendTime(null) and in onError do not change the form (or
restore previous value); for the schedule path, await executeSaveDraft() and
emailClient.scheduleMessage(...) (and emailClient.flagArchived(...)) and only
after those succeed call form.setSendTime(date); on any error show the
toast.failure and do not modify form.sendTime so UI always reflects server state
(use the existing functions: handleSendTimeChange, form.setSendTime,
unscheduleMessageMutation.mutate, executeSaveDraft, emailClient.scheduleMessage,
saveDraftMutation, emailClient.flagArchived).
Comment on lines
+121
to
+128
| onFocusLeaveStart={(e) => { | ||
| e.preventDefault(); | ||
| focusSibling('prev'); | ||
| }} | ||
| onFocusLeaveEnd={(e) => { | ||
| e.preventDefault(); | ||
| focusSibling('next'); | ||
| }} |
Contributor
There was a problem hiding this comment.
Only cancel focus leave when you actually moved focus.
Both handlers call preventDefault() before checking whether focusSibling(...) found a target. If this compose renders without a next/previous tabbable sibling, or the split-panel context is absent, Tab/Shift+Tab out of the editor gets swallowed and keyboard users are stuck in the body.
Suggested fix
onFocusLeaveStart={(e) => {
- e.preventDefault();
- focusSibling('prev');
+ if (focusSibling('prev')) {
+ e.preventDefault();
+ }
}}
onFocusLeaveEnd={(e) => {
- e.preventDefault();
- focusSibling('next');
+ if (focusSibling('next')) {
+ e.preventDefault();
+ }
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onFocusLeaveStart={(e) => { | |
| e.preventDefault(); | |
| focusSibling('prev'); | |
| }} | |
| onFocusLeaveEnd={(e) => { | |
| e.preventDefault(); | |
| focusSibling('next'); | |
| }} | |
| onFocusLeaveStart={(e) => { | |
| if (focusSibling('prev')) { | |
| e.preventDefault(); | |
| } | |
| }} | |
| onFocusLeaveEnd={(e) => { | |
| if (focusSibling('next')) { | |
| e.preventDefault(); | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeBody.tsx` around lines
121 - 128, The handlers onFocusLeaveStart and onFocusLeaveEnd in ComposeBody.tsx
call e.preventDefault() unconditionally and can swallow Tab/Shift+Tab when no
sibling or split-panel target exists; change them to call focusSibling('prev') /
focusSibling('next') first (or check its return) and only call
e.preventDefault() when focusSibling actually found and moved focus (i.e., when
a target exists), so normal tab navigation is preserved when no
sibling/split-panel context is present.
sedson
approved these changes
Mar 30, 2026
| attachments: DraftFormAttachment[]; | ||
| }; | ||
|
|
||
| let undoComposeSnapshot: UndoComposeSnapshot | null = null; |
| }, | ||
| }} | ||
| > | ||
| <div class={`${!isDragging() && 'hidden'} absolute inset-0`}> |
ehayes2000
force-pushed
the
email-compose-refactor
branch
from
b83d00d to
2a82114
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/block-email/component/compose/Compose.tsx`:
- Around line 464-479: Ensure we always flush/save the current draft before
scheduling: replace the conditional creation of draftID that uses currentDraft
with an unconditional call to executeSaveDraft() and use its returned result for
both the draftID passed to emailClient.scheduleMessage and to obtain the
thread_db_id (instead of relying on saveDraftMutation.data). Specifically, call
executeSaveDraft(), validate the returned draftID, use that ID for
scheduleMessage({ draftID, send_time: ... }), and derive thread_db_id from the
same returned result to then call emailClient.flagArchived({ id: thread_db_id,
value: true }) when present.
- Around line 242-264: The handler handleRemoveAttachment currently mutates
local form state immediately via
form.attachments.removeByFile/removeForwarded/removeByID before the server call,
causing UI desync if the server delete fails; change it to perform the server
mutation first (use currentDraftID() to build payload for
removeAttachmentMutation.mutate or removeForwardedAttachmentMutation.mutate) and
only update form state in the mutation's onSuccess callback, or if you prefer an
optimistic UX, perform the local remove but implement an onError rollback in the
mutation callbacks that re-adds the attachment to form.attachments; update
handleRemoveAttachment to call mutate with appropriate onSuccess/onError
handlers rather than removing from the form upfront.
- Around line 190-220: The draft ID returned by saveDraftMutation.mutateAsync
isn't persisted before uploading attachments, so if attachment upload fails the
in-memory currentDraftID() remains undefined; move the persistence earlier:
after obtaining draftResponse and computing draftId from
draftResponse.draft.db_id, call setCurrentDraftID(draftId) immediately (before
calling uploadAttachmentMutation.mutateAsync), then proceed to upload
attachments and call form.attachments.assignAttachmentID(...) as before; ensure
you still guard on draftId existence and only attempt uploads when draftId is
present.
In `@js/app/packages/block-email/component/compose/ComposeBody.tsx`:
- Around line 155-192: Replace the runtime Switch/Match rendering with a
compile-time exhaustive pattern match using ts-pattern's match on
props.attachment (type DraftFormAttachment) so new attachment types fail at
compile time; locate the Switch/Match block in ComposeBody (the component that
renders EmailAttachmentPill for props.attachment) and change to
match(props.attachment).with({type:'local'}, att => ...).with({type:'remote'},
...).with({type:'forwarded'}, ...).exhaustive(), keeping the same
EmailAttachmentPill props and using the existing handleRemove callback so
behavior is identical but exhaustive checks are enforced.
In `@js/app/packages/block-email/component/compose/ComposeLayout.tsx`:
- Around line 184-200: The nested application of opacity via classList using
hasLinkError() is causing compounded fading; remove the 'pointer-events-none
opacity-50' classList from the inner container (the div with classes "w-full p-4
bg-input...") and keep the disabled styling only on the outermost container that
uses mobileScrollRef (or vice versa) so the disabled state is applied once;
update references where hasLinkError() is used in ComposeLayout.tsx (the
classList on the outer div, the inner div, and any other duplicated usages) so
only one container (preferably the outer mobileScrollRef-wrapped div or the
ClippedPanel wrapper via WrapUnlessMobile) receives the opacity and
pointer-events change.
In `@js/app/packages/block-email/component/compose/ComposeToolbar.tsx`:
- Around line 133-140: The Delete Draft button inside the Show block is using
the wrong prop name so its handler (ctx.onDelete) won't be invoked; update the
Button prop from onclick to camelCase onClick so the Solid.js click event wiring
matches other handlers (see Button usage lines with onClick and the ctx.onDelete
reference) and verify ComposeToolbar's Show/Button block uses
onClick={ctx.onDelete}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9b73170-a540-4fd5-b658-f001f14a683e
📒 Files selected for processing (13)
js/app/packages/app/component/split-layout/componentRegistry.tsxjs/app/packages/block-email/component/Compose.tsxjs/app/packages/block-email/component/ComposeEmailInput.tsxjs/app/packages/block-email/component/Email.tsxjs/app/packages/block-email/component/compose/Compose.tsxjs/app/packages/block-email/component/compose/ComposeBody.tsxjs/app/packages/block-email/component/compose/ComposeContext.tsjs/app/packages/block-email/component/compose/ComposeLayout.tsxjs/app/packages/block-email/component/compose/ComposeRecipients.tsxjs/app/packages/block-email/component/compose/ComposeSubject.tsxjs/app/packages/block-email/component/compose/ComposeToolbar.tsxjs/app/packages/block-email/component/compose/index.tsjs/app/packages/entity/src/components/CollapsibleList.tsx
💤 Files with no reviewable changes (2)
- js/app/packages/block-email/component/ComposeEmailInput.tsx
- js/app/packages/block-email/component/Compose.tsx
Comment on lines
+190
to
+220
| const draftResponse = await saveDraftMutation.mutateAsync({ | ||
| draft: { | ||
| ...draftToSave, | ||
| db_id: currentDraftID(), | ||
| }, | ||
| }); | ||
|
|
||
| const draftId = draftResponse.draft.db_id; | ||
| if (draftId) { | ||
| const attachments = form.attachments | ||
| .list() | ||
| .filter((a) => a.type === 'local' && !a.attachmentID) as Extract< | ||
| DraftFormAttachment, | ||
| { type: 'local' } | ||
| >[]; | ||
|
|
||
| if (attachments.length) { | ||
| const uploaded = await uploadAttachmentMutation.mutateAsync({ | ||
| draftID: draftId, | ||
| attachments: attachments.map((a) => a.file), | ||
| }); | ||
|
|
||
| for (const attachment of uploaded.attachments) { | ||
| form.attachments.assignAttachmentID( | ||
| attachment.file, | ||
| attachment.attachmentID | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| setCurrentDraftID(draftId); |
Contributor
There was a problem hiding this comment.
Persist currentDraftID() before uploading attachments.
If the save succeeds and Lines 207-210 fail, Line 220 never runs. On a brand-new draft with local files, the next autosave/schedule/send goes out with db_id: undefined again and can create a second draft even though the first save already succeeded.
🛠️ Minimal fix
const draftId = draftResponse.draft.db_id;
if (draftId) {
+ setCurrentDraftID(draftId);
+
const attachments = form.attachments
.list()
.filter((a) => a.type === 'local' && !a.attachmentID) as Extract<
DraftFormAttachment,
{ type: 'local' }
>[];
if (attachments.length) {
const uploaded = await uploadAttachmentMutation.mutateAsync({
draftID: draftId,
attachments: attachments.map((a) => a.file),
});
for (const attachment of uploaded.attachments) {
form.attachments.assignAttachmentID(
attachment.file,
attachment.attachmentID
);
}
}
- setCurrentDraftID(draftId);
return draftId;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 190 -
220, The draft ID returned by saveDraftMutation.mutateAsync isn't persisted
before uploading attachments, so if attachment upload fails the in-memory
currentDraftID() remains undefined; move the persistence earlier: after
obtaining draftResponse and computing draftId from draftResponse.draft.db_id,
call setCurrentDraftID(draftId) immediately (before calling
uploadAttachmentMutation.mutateAsync), then proceed to upload attachments and
call form.attachments.assignAttachmentID(...) as before; ensure you still guard
on draftId existence and only attempt uploads when draftId is present.
Comment on lines
+242
to
+264
| const handleRemoveAttachment = (attachment: DraftFormAttachment) => { | ||
| if (attachment.type === 'local') { | ||
| form.attachments.removeByFile(attachment.file); | ||
| } else if (attachment.type === 'forwarded') { | ||
| form.attachments.removeForwarded(attachment.attachmentID); | ||
| } else { | ||
| form.attachments.removeByID(attachment.attachmentID); | ||
| } | ||
|
|
||
| const savedDraftID = currentDraftID(); | ||
| if (!savedDraftID || !attachment.attachmentID) return; | ||
|
|
||
| if (attachment.type === 'forwarded') { | ||
| removeForwardedAttachmentMutation.mutate({ | ||
| draftID: savedDraftID, | ||
| attachmentID: attachment.attachmentID, | ||
| }); | ||
| } else { | ||
| removeAttachmentMutation.mutate({ | ||
| draftID: savedDraftID, | ||
| attachmentID: attachment.attachmentID, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Rollback failed attachment deletes.
This removes the attachment from local form state before the server mutation finishes. If the delete fails, the UI shows the file as gone but the draft on the server still keeps it, so a later send can include an attachment the user already removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 242 -
264, The handler handleRemoveAttachment currently mutates local form state
immediately via form.attachments.removeByFile/removeForwarded/removeByID before
the server call, causing UI desync if the server delete fails; change it to
perform the server mutation first (use currentDraftID() to build payload for
removeAttachmentMutation.mutate or removeForwardedAttachmentMutation.mutate) and
only update form state in the mutation's onSuccess callback, or if you prefer an
optimistic UX, perform the local remove but implement an onError rollback in the
mutation callbacks that re-adds the attachment to form.attachments; update
handleRemoveAttachment to call mutate with appropriate onSuccess/onError
handlers rather than removing from the form upfront.
Comment on lines
+464
to
+479
| if (date) { | ||
| const draftID = currentDraft ?? (await executeSaveDraft()); | ||
| if (!draftID) { | ||
| toast.failure('Failed to schedule message', 'Draft required'); | ||
| return; | ||
| } | ||
|
|
||
| await emailClient.scheduleMessage({ | ||
| draftID, | ||
| send_time: date.toISOString(), | ||
| }); | ||
|
|
||
| const threadID = saveDraftMutation.data?.draft.thread_db_id; | ||
| if (threadID) { | ||
| await emailClient.flagArchived({ id: threadID, value: true }); | ||
| } |
Contributor
There was a problem hiding this comment.
Flush the draft before scheduling existing drafts.
Line 465 only saves when the draft is brand new. If a persisted draft has unsaved edits, scheduling can race ahead of the debounce and queue stale recipients/subject/body; Line 476 also depends on saveDraftMutation.data, which is still unset when an existing draft is opened and scheduled immediately, so archiving can be skipped entirely. Always call executeSaveDraft() here and use its return value for both draftID and thread_db_id.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/Compose.tsx` around lines 464 -
479, Ensure we always flush/save the current draft before scheduling: replace
the conditional creation of draftID that uses currentDraft with an unconditional
call to executeSaveDraft() and use its returned result for both the draftID
passed to emailClient.scheduleMessage and to obtain the thread_db_id (instead of
relying on saveDraftMutation.data). Specifically, call executeSaveDraft(),
validate the returned draftID, use that ID for scheduleMessage({ draftID,
send_time: ... }), and derive thread_db_id from the same returned result to then
call emailClient.flagArchived({ id: thread_db_id, value: true }) when present.
Comment on lines
+155
to
+192
| <Switch> | ||
| <Match when={props.attachment.type === 'local' && props.attachment}> | ||
| {(attachment) => ( | ||
| <EmailAttachmentPill | ||
| attachment={{ | ||
| fileName: attachment().file.name, | ||
| mimeType: attachment().file.type, | ||
| }} | ||
| removable | ||
| onRemove={handleRemove} | ||
| /> | ||
| )} | ||
| </Match> | ||
| <Match when={props.attachment.type === 'remote' && props.attachment}> | ||
| {(attachment) => ( | ||
| <EmailAttachmentPill | ||
| attachment={{ | ||
| fileName: attachment().fileName, | ||
| mimeType: attachment().contentType, | ||
| }} | ||
| removable | ||
| onRemove={handleRemove} | ||
| /> | ||
| )} | ||
| </Match> | ||
| <Match when={props.attachment.type === 'forwarded' && props.attachment}> | ||
| {(attachment) => ( | ||
| <EmailAttachmentPill | ||
| attachment={{ | ||
| fileName: attachment().fileName, | ||
| mimeType: attachment().mimeType, | ||
| }} | ||
| removable | ||
| onRemove={handleRemove} | ||
| /> | ||
| )} | ||
| </Match> | ||
| </Switch> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using match from ts-pattern for exhaustive attachment type handling.
The Switch/Match pattern does not guarantee exhaustiveness. If a new attachment type is added to DraftFormAttachment, this component will silently render nothing for it. Using match from ts-pattern would provide compile-time exhaustiveness checks.
Example refactor using ts-pattern
import { match } from 'ts-pattern';
function AttachmentItem(props: { attachment: DraftFormAttachment }) {
const ctx = useCompose();
const handleRemove = () => {
ctx.onRemoveAttachment(props.attachment);
};
return match(props.attachment)
.with({ type: 'local' }, (att) => (
<EmailAttachmentPill
attachment={{
fileName: att.file.name,
mimeType: att.file.type,
}}
removable
onRemove={handleRemove}
/>
))
.with({ type: 'remote' }, (att) => (
<EmailAttachmentPill
attachment={{
fileName: att.fileName,
mimeType: att.contentType,
}}
removable
onRemove={handleRemove}
/>
))
.with({ type: 'forwarded' }, (att) => (
<EmailAttachmentPill
attachment={{
fileName: att.fileName,
mimeType: att.mimeType,
}}
removable
onRemove={handleRemove}
/>
))
.exhaustive();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeBody.tsx` around lines
155 - 192, Replace the runtime Switch/Match rendering with a compile-time
exhaustive pattern match using ts-pattern's match on props.attachment (type
DraftFormAttachment) so new attachment types fail at compile time; locate the
Switch/Match block in ComposeBody (the component that renders
EmailAttachmentPill for props.attachment) and change to
match(props.attachment).with({type:'local'}, att => ...).with({type:'remote'},
...).with({type:'forwarded'}, ...).exhaustive(), keeping the same
EmailAttachmentPill props and using the existing handleRemove callback so
behavior is identical but exhaustive checks are enforced.
Comment on lines
+184
to
+200
| <div | ||
| ref={mobileScrollRef} | ||
| class="macro-message-width sm:macro-message-padding mx-auto w-full max-h-full my-2 sm:my-12 mobile:my-0 px-2 sm:px-4 mobile:px-0 overflow-hidden mobile:overflow-y-auto mobile:hide-scrollbar mobile:min-h-full" | ||
| classList={{ | ||
| 'pointer-events-none opacity-50': hasLinkError(), | ||
| }} | ||
| > | ||
| <WrapUnlessMobile | ||
| wrapper={(children) => ( | ||
| <ClippedPanel tl={!beveledCorners()}>{children}</ClippedPanel> | ||
| )} | ||
| > | ||
| <div | ||
| class="w-full p-4 bg-input max-h-full mobile:max-h-none overflow-y-auto flex flex-col min-h-0 mobile:min-h-full" | ||
| classList={{ | ||
| 'pointer-events-none opacity-50': hasLinkError(), | ||
| }} |
Contributor
There was a problem hiding this comment.
Stacking opacity creates overly faded content when hasLinkError() is true.
When hasLinkError() returns true, opacity-50 is applied at three nested levels (lines 188, 199, and 269), resulting in a compounded opacity of ~12.5%. This likely makes the content nearly invisible. Consider applying the disabled styling at only one level.
Suggested fix - apply disabled state at the outermost container only
<div
ref={mobileScrollRef}
class="macro-message-width sm:macro-message-padding mx-auto w-full max-h-full my-2 sm:my-12 mobile:my-0 px-2 sm:px-4 mobile:px-0 overflow-hidden mobile:overflow-y-auto mobile:hide-scrollbar mobile:min-h-full"
classList={{
'pointer-events-none opacity-50': hasLinkError(),
}}
>
...
<div
class="w-full p-4 bg-input max-h-full mobile:max-h-none overflow-y-auto flex flex-col min-h-0 mobile:min-h-full"
- classList={{
- 'pointer-events-none opacity-50': hasLinkError(),
- }}
>
...
<div
class="w-full h-full flex flex-col min-h-0 mt-4"
- classList={{
- 'pointer-events-none opacity-50': hasLinkError(),
- }}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeLayout.tsx` around lines
184 - 200, The nested application of opacity via classList using hasLinkError()
is causing compounded fading; remove the 'pointer-events-none opacity-50'
classList from the inner container (the div with classes "w-full p-4
bg-input...") and keep the disabled styling only on the outermost container that
uses mobileScrollRef (or vice versa) so the disabled state is applied once;
update references where hasLinkError() is used in ComposeLayout.tsx (the
classList on the outer div, the inner div, and any other duplicated usages) so
only one container (preferably the outer mobileScrollRef-wrapped div or the
ClippedPanel wrapper via WrapUnlessMobile) receives the opacity and
pointer-events change.
Comment on lines
+133
to
+140
| <Show when={ctx.hasDraft() && !ctx.isDraftSaving()}> | ||
| <Button | ||
| onclick={ctx.onDelete} | ||
| tooltip="Delete draft" | ||
| class="aspect-square *:h-5 p-1" | ||
| > | ||
| <Trash /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
button_file="$(fd -i '^Button\.(ts|tsx)$' js/app | head -n1)"
if [[ -z "${button_file:-}" ]]; then
echo "Button component file not found" >&2
exit 1
fi
echo "=== Button component ==="
sed -n '1,220p' "$button_file"
echo
echo "=== Current call site ==="
sed -n '133,140p' js/app/packages/block-email/component/compose/ComposeToolbar.tsx
echo
echo "=== Button click prop spellings across js/app ==="
rg -n --type tsx '\<Button\b[^>]*\b(onClick|onclick)=' js/app -C1Repository: macro-inc/macro
Length of output: 3473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Lines 120 and 212 in ComposeToolbar.tsx ==="
sed -n '115,145p' js/app/packages/block-email/component/compose/ComposeToolbar.tsx
echo "---"
sed -n '205,220p' js/app/packages/block-email/component/compose/ComposeToolbar.tsx
echo
echo "=== Search for Button usage with click handlers ==="
rg -n 'Button' js/app/packages/block-email/component/compose/ComposeToolbar.tsx -A3 -B1 | head -60Repository: macro-inc/macro
Length of output: 4060
Fix the delete draft button—onclick should be onClick.
Line 135 uses lowercase onclick, while all other Button click handlers in the file use camelCase onClick (lines 120 and 212). Solid.js event handlers require camelCase; the delete action is currently wired to an unrecognized prop and will not fire.
Change onclick={ctx.onDelete} to onClick={ctx.onDelete} on line 135.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-email/component/compose/ComposeToolbar.tsx` around
lines 133 - 140, The Delete Draft button inside the Show block is using the
wrong prop name so its handler (ctx.onDelete) won't be invoked; update the
Button prop from onclick to camelCase onClick so the Solid.js click event wiring
matches other handlers (see Button usage lines with onClick and the ctx.onDelete
reference) and verify ComposeToolbar's Show/Button block uses
onClick={ctx.onDelete}.
This was referenced Apr 1, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Engooden
Compose.tsx