Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion apps/web/src/features/compose/useComposeSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,10 @@ export function useComposeSession(
const current = draftRef.current;
if (!current) throw new Error("No draft is open");
const now = new Date().toISOString();
const draftId = crypto.randomUUID();
// Editing an existing stored draft must save it in place; only mint a
// fresh id for a genuinely new compose session (save-local is an
// upsert-by-id, so reusing the id updates rather than duplicates it).
const draftId = intent.draftId ?? crypto.randomUUID();
await saveLocalDraft({
id: draftId,
account_id: current.accountId,
Expand Down
194 changes: 194 additions & 0 deletions crates/compose/src/draft_codec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
//! Bidirectional codec between a stored [`Draft`] and the editor-facing
//! compose-file format (YAML frontmatter + markdown body).
//!
//! Before this module the mapping was open-coded in the daemon, web, and TUI
//! crates (each with its own `format_addresses` and inline `ComposeFrontmatter`
//! construction). Centralizing it here lets every surface load a stored draft
//! into `$EDITOR` and round-trip the edit back to the *same* [`DraftId`] — the
//! basis for editing a draft in place instead of creating a new one and
//! discarding the old.
//!
//! [`DraftId`]: mxr_core::id::DraftId

use crate::frontmatter::{
parse_compose_file, render_compose_file, ComposeError, ComposeFrontmatter,
};
use chrono::{DateTime, Utc};
use mxr_core::types::{Address, Draft, ReplyHeaders};
use std::path::PathBuf;

/// Render a list of addresses back into a `"Name <email>, email"` header
/// string suitable for a compose-file `to:`/`cc:`/`bcc:` field.
pub fn format_addresses(addresses: &[Address]) -> String {
addresses

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: format_addresses emits display names verbatim into a comma-delimited address list without RFC 5322 quoting. For example, the name "Doe, Jane" renders as Doe, Jane <jane@example.com>, which contains an unquoted comma. When parse_address_list processes this on the round-trip, the comma is interpreted as an address separator, typically splitting the name into a malformed extra address or dropping part of the recipient. The mail_parser-based parser can handle quoted commas (proven by existing tests in crates/mail-parse/src/lib.rs and crates/provider-gmail/src/parse.rs), but format_addresses never applies quoted-string escaping, so the round-trip is broken for any contact name containing commas, angle brackets, or similar special characters.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/compose/src/draft_codec.rs, line 23:

<comment>`format_addresses` emits display names verbatim into a comma-delimited address list without RFC 5322 quoting. For example, the name `"Doe, Jane"` renders as `Doe, Jane <jane@example.com>`, which contains an unquoted comma. When `parse_address_list` processes this on the round-trip, the comma is interpreted as an address separator, typically splitting the name into a malformed extra address or dropping part of the recipient. The `mail_parser`-based parser can handle quoted commas (proven by existing tests in `crates/mail-parse/src/lib.rs` and `crates/provider-gmail/src/parse.rs`), but `format_addresses` never applies quoted-string escaping, so the round-trip is broken for any contact name containing commas, angle brackets, or similar special characters.</comment>

<file context>
@@ -0,0 +1,194 @@
+/// Render a list of addresses back into a `"Name <email>, email"` header
+/// string suitable for a compose-file `to:`/`cc:`/`bcc:` field.
+pub fn format_addresses(addresses: &[Address]) -> String {
+    addresses
+        .iter()
+        .map(|address| match address.name.as_deref() {
</file context>

.iter()
.map(|address| match address.name.as_deref() {
Some(name) if !name.trim().is_empty() => format!("{name} <{}>", address.email),
_ => address.email.clone(),
})
.collect::<Vec<_>>()
.join(", ")
}

/// Build editor frontmatter from a stored draft. `from` is the sending
/// account's address — the [`Draft`] stores only an `account_id`, so the
/// caller resolves the email and passes it in.
pub fn frontmatter_from_draft(draft: &Draft, from: &str) -> ComposeFrontmatter {
ComposeFrontmatter {
to: format_addresses(&draft.to),
cc: format_addresses(&draft.cc),
bcc: format_addresses(&draft.bcc),
subject: draft.subject.clone(),
from: from.to_string(),
in_reply_to: draft
.reply_headers
.as_ref()
.map(|headers| headers.in_reply_to.clone()),
intent: draft.intent,
references: draft
.reply_headers
.as_ref()
.map(|headers| headers.references.clone())
.unwrap_or_default(),
thread_id: draft
.reply_headers
.as_ref()
.and_then(|headers| headers.thread_id.clone()),
attach: draft
.attachments
.iter()
.map(|attachment| attachment.display().to_string())
.collect(),
signature: None,
}
}

/// Render a stored draft into the editor-facing compose-file text
/// (frontmatter + markdown body). No context block: an edit reopens the
/// user's own content, not a quoted original.
pub fn draft_to_compose_file(draft: &Draft, from: &str) -> Result<String, ComposeError> {
let frontmatter = frontmatter_from_draft(draft, from);
render_compose_file(&frontmatter, &draft.body_markdown, None)
}

/// Re-assemble an edited compose file back into a [`Draft`], preserving the
/// original `id`, `account_id`, `created_at`, and `inline_calendar_reply`,
/// and stamping `updated_at`. This is what makes "edit in place" possible:
/// the same [`DraftId`] round-trips instead of a new one being minted.
///
/// [`DraftId`]: mxr_core::id::DraftId
pub fn apply_edited_compose_file(
existing: &Draft,
content: &str,
updated_at: DateTime<Utc>,
) -> Result<Draft, ComposeError> {
let (frontmatter, body) = parse_compose_file(content)?;
let reply_headers = frontmatter
.in_reply_to
.as_ref()
.map(|in_reply_to| ReplyHeaders {
in_reply_to: in_reply_to.clone(),
references: frontmatter.references.clone(),
thread_id: frontmatter.thread_id.clone(),
})
.or_else(|| existing.reply_headers.clone());

Ok(Draft {
id: existing.id.clone(),
account_id: existing.account_id.clone(),
reply_headers,
// A user who clears `intent:` back to the default keeps the draft's
// original intent rather than silently downgrading a reply to a new.
intent: if frontmatter.intent == mxr_core::DraftIntent::New {
existing.intent
} else {
frontmatter.intent
},
to: mxr_mail_parse::parse_address_list(&frontmatter.to),
cc: mxr_mail_parse::parse_address_list(&frontmatter.cc),
bcc: mxr_mail_parse::parse_address_list(&frontmatter.bcc),
subject: frontmatter.subject,
body_markdown: body,
attachments: frontmatter.attach.iter().map(PathBuf::from).collect(),
inline_calendar_reply: existing.inline_calendar_reply.clone(),
created_at: existing.created_at,
updated_at,
})
}

#[cfg(test)]
mod tests {
use super::*;
use mxr_core::id::{AccountId, DraftId};
use mxr_core::types::DraftIntent;

fn sample_draft() -> Draft {
Draft {
id: DraftId::new(),
account_id: AccountId::new(),
reply_headers: None,
intent: DraftIntent::New,
to: vec![
Address {
name: Some("Alice Example".into()),
email: "alice@example.com".into(),
},
Address {
name: None,
email: "bob@example.com".into(),
},
],
cc: vec![],
bcc: vec![],
subject: "Q4 plan".into(),
body_markdown: "Body line one.\n\nBody line two.".into(),
attachments: vec![],
inline_calendar_reply: None,
created_at: DateTime::from_timestamp(1_700_000_000, 0).unwrap(),
updated_at: DateTime::from_timestamp(1_700_000_000, 0).unwrap(),
}
}

#[test]
fn format_addresses_renders_name_and_bare() {
let rendered = format_addresses(&sample_draft().to);
assert_eq!(
rendered,
"Alice Example <alice@example.com>, bob@example.com"
);
}

#[test]
fn round_trip_preserves_id_created_at_and_content() {
let original = sample_draft();
let file = draft_to_compose_file(&original, "me@example.com").unwrap();
assert!(file.contains("subject: Q4 plan"));
assert!(file.contains("from: me@example.com"));
assert!(file.contains("Body line one."));

let edited = file.replace("Q4 plan", "Q4 plan (revised)");
let later = DateTime::from_timestamp(1_700_000_999, 0).unwrap();
let result = apply_edited_compose_file(&original, &edited, later).unwrap();

// Identity and creation time survive the edit; only content + updated_at change.
assert_eq!(result.id, original.id);
assert_eq!(result.account_id, original.account_id);
assert_eq!(result.created_at, original.created_at);
assert_eq!(result.updated_at, later);
assert_eq!(result.subject, "Q4 plan (revised)");
assert_eq!(result.to.len(), 2);
assert_eq!(result.to[0].email, "alice@example.com");
}

#[test]
fn absent_intent_falls_back_to_existing() {
let mut original = sample_draft();
original.intent = DraftIntent::Reply;
// A compose file with no `intent:` line — the user never touched it, so
// the parsed frontmatter defaults to New and must fall back to Reply.
let file = "---\nto: a@example.com\nsubject: Hi\nfrom: me@example.com\n---\n\nBody.";
let now = DateTime::from_timestamp(1_700_000_500, 0).unwrap();
let result = apply_edited_compose_file(&original, file, now).unwrap();
assert_eq!(result.intent, DraftIntent::Reply);
}
}
1 change: 1 addition & 0 deletions crates/compose/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
)]

pub mod attachments;
pub mod draft_codec;
pub mod editor;
pub mod email;
pub mod frontmatter;
Expand Down
9 changes: 8 additions & 1 deletion crates/daemon/src/activity/mapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,12 @@ pub fn map_request(
Some(draft.id.as_str().clone()),
None,
),
Request::UpdateDraft { draft } => (
"draft.update",
Some("draft"),
Some(draft.id.as_str().clone()),
None,
),
Request::DeleteDraft { draft_id } => (
"draft.discard",
Some("draft"),
Expand Down Expand Up @@ -611,7 +617,8 @@ pub fn map_request(
| Request::GetDecision { .. }
| Request::ListOwedReplies { .. }
| Request::ListDrafts
| Request::ListOrphanedDrafts => {
| Request::ListOrphanedDrafts
| Request::GetDraft { .. } => {
skip_activity!(
"read_list_status",
"read/list/status requests are not activity entries"
Expand Down
7 changes: 7 additions & 0 deletions crates/daemon/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1689,6 +1689,13 @@ pub enum DraftsAction {
/// Draft ID to delete.
draft_id: String,
},
/// Open an existing draft in `$EDITOR` and re-save it in place under
/// the same draft id. No new draft is created and nothing is
/// discarded.
Edit {
/// Draft ID to edit.
draft_id: String,
},
}

#[derive(Debug, Clone, Subcommand)]
Expand Down
80 changes: 80 additions & 0 deletions crates/daemon/src/commands/mutations/compose.rs
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,86 @@ pub async fn drafts_discard(draft_id: String, account: Option<String>) -> anyhow
}
}

/// CLI surface for `mxr drafts edit <id>`. Opens an existing stored draft
/// in `$EDITOR` and re-saves it in place under the same `DraftId` — no
/// new draft is created and nothing is discarded.
pub async fn drafts_edit(draft_id: String, account: Option<String>) -> anyhow::Result<()> {
let parsed = DraftId::from_uuid(uuid::Uuid::parse_str(&draft_id)?);
let mut client = IpcClient::connect().await?;
let account_id = resolve_optional_account(&mut client, account.as_deref()).await?;

let draft = crate::commands::expect_response(
client
.request(Request::GetDraft {
draft_id: parsed.clone(),
})
.await?,
|response| match response {
Response::Ok {
data: ResponseData::Draft { draft },
} => Some(draft),
_ => None,
},
)?;
if account_id.is_some_and(|account_id| draft.account_id != account_id) {
anyhow::bail!("Draft {parsed} belongs to a different account");
}

let from = resolve_account_email(&mut client, &draft.account_id).await?;

let content = mxr_compose::draft_codec::draft_to_compose_file(&draft, &from)?;
// A draft is unsent mail content; write it to the per-user private 0700
// scratch dir with 0600 perms rather than the world-readable temp root.
let path = mxr_compose::private_tmp::private_scratch_dir()?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Using a deterministic scratch filename per DraftId and unconditionally unlinking it before writing introduces a destructive race. If a prior edit failed, its recovery content (intended to be kept for the user) is deleted on retry. Worse, two concurrent mxr drafts edit <id> processes cause the second invocation to remove the first editor's file from under it, after which both editors may operate on conflicting inodes or the same replacement path. The scratch file should use a unique per-invocation name (e.g. with a random suffix) and cleanup should be scoped to that specific invocation, preserving any unrelated recovery file.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/commands/mutations/compose.rs, line 671:

<comment>Using a deterministic scratch filename per `DraftId` and unconditionally unlinking it before writing introduces a destructive race. If a prior edit failed, its recovery content (intended to be kept for the user) is deleted on retry. Worse, two concurrent `mxr drafts edit <id>` processes cause the second invocation to remove the first editor's file from under it, after which both editors may operate on conflicting inodes or the same replacement path. The scratch file should use a unique per-invocation name (e.g. with a random suffix) and cleanup should be scoped to that specific invocation, preserving any unrelated recovery file.</comment>

<file context>
@@ -638,6 +638,86 @@ pub async fn drafts_discard(draft_id: String, account: Option<String>) -> anyhow
+    let content = mxr_compose::draft_codec::draft_to_compose_file(&draft, &from)?;
+    // A draft is unsent mail content; write it to the per-user private 0700
+    // scratch dir with 0600 perms rather than the world-readable temp root.
+    let path = mxr_compose::private_tmp::private_scratch_dir()?
+        .join(format!("mxr-draft-edit-{}.md", draft.id));
+    // Clear any leftover from a prior aborted edit so the O_EXCL write succeeds.
</file context>

.join(format!("mxr-draft-edit-{}.md", draft.id));
// Clear any leftover from a prior aborted edit so the O_EXCL write succeeds.
let _ = std::fs::remove_file(&path);
mxr_compose::private_tmp::write_private(&path, content.as_bytes())?;

let editor = mxr_compose::editor::resolve_editor(None);
mxr_compose::editor::spawn_editor(&editor, &path, None).await?;

let edited = std::fs::read_to_string(&path)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Stored-draft editing bypasses the snippet expansion that the standard compose-save flow performs. In finalize_compose, expand_compose_snippets_with_context is called before saving, but drafts_edit passes the raw edited text straight into apply_edited_compose_file. Because send_stored_draft also does not expand snippets before sending, any snippet tokens added during an edit can be sent literally to recipients instead of being resolved. The edit path should expand snippets after the editor returns and before constructing the updated Draft, matching the fresh-compose behavior.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/commands/mutations/compose.rs, line 680:

<comment>Stored-draft editing bypasses the snippet expansion that the standard compose-save flow performs. In `finalize_compose`, `expand_compose_snippets_with_context` is called before saving, but `drafts_edit` passes the raw edited text straight into `apply_edited_compose_file`. Because `send_stored_draft` also does not expand snippets before sending, any snippet tokens added during an edit can be sent literally to recipients instead of being resolved. The edit path should expand snippets after the editor returns and before constructing the updated `Draft`, matching the fresh-compose behavior.</comment>

<file context>
@@ -638,6 +638,86 @@ pub async fn drafts_discard(draft_id: String, account: Option<String>) -> anyhow
+    let editor = mxr_compose::editor::resolve_editor(None);
+    mxr_compose::editor::spawn_editor(&editor, &path, None).await?;
+
+    let edited = std::fs::read_to_string(&path)?;
+    let (frontmatter, body) = mxr_compose::frontmatter::parse_compose_file(&edited)?;
+    validate_compose_draft(&frontmatter, &body, false)?;
</file context>

let (frontmatter, body) = mxr_compose::frontmatter::parse_compose_file(&edited)?;
validate_compose_draft(&frontmatter, &body, false)?;

let updated =
mxr_compose::draft_codec::apply_edited_compose_file(&draft, &edited, chrono::Utc::now())?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The CLI edit path parses the compose file twice: once explicitly for validation and again inside draft_codec::apply_edited_compose_file. This is redundant and means parser behavior changes must be consistent across both paths. Consider refactoring apply_edited_compose_file to accept the already-parsed (&ComposeFrontmatter, &str) so the caller can reuse the parse it already performed.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/commands/mutations/compose.rs, line 685:

<comment>The CLI edit path parses the compose file twice: once explicitly for validation and again inside `draft_codec::apply_edited_compose_file`. This is redundant and means parser behavior changes must be consistent across both paths. Consider refactoring `apply_edited_compose_file` to accept the already-parsed `(&ComposeFrontmatter, &str)` so the caller can reuse the parse it already performed.</comment>

<file context>
@@ -638,6 +638,86 @@ pub async fn drafts_discard(draft_id: String, account: Option<String>) -> anyhow
+    validate_compose_draft(&frontmatter, &body, false)?;
+
+    let updated =
+        mxr_compose::draft_codec::apply_edited_compose_file(&draft, &edited, chrono::Utc::now())?;
+
+    expect_ack(
</file context>


expect_ack(
client
.request(Request::UpdateDraft {
draft: updated.clone(),
})
.await?,
)?;

let _ = std::fs::remove_file(&path);
println!("Draft updated: {}", updated.id);
Ok(())
}

/// Resolve the sending address for an account by id. Used when editing a
/// stored draft, where we have `draft.account_id` but not the CLI's
/// `--from`/`--account` selector string that `resolve_compose_account` expects.
async fn resolve_account_email(
client: &mut IpcClient,
account_id: &AccountId,
) -> anyhow::Result<String> {
let resp = client.request(Request::ListAccounts).await?;
let accounts = crate::commands::expect_response(resp, |response| match response {
Response::Ok {
data: ResponseData::Accounts { accounts },
} => Some(accounts),
_ => None,
})?;
accounts
.into_iter()
.find(|account| &account.account_id == account_id)
.map(|account| account.email)
.ok_or_else(|| anyhow::anyhow!("Account {account_id} not found"))
}

pub async fn send_draft(
draft_id: String,
account: Option<String>,
Expand Down
2 changes: 1 addition & 1 deletion crates/daemon/src/commands/mutations/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ mod helpers;

pub use attachments::{attachments_download, attachments_list, attachments_open};
pub use compose::{
cancel_scheduled_send, check_send, compose, compose_check, drafts, drafts_discard,
cancel_scheduled_send, check_send, compose, compose_check, drafts, drafts_discard, drafts_edit,
drafts_recover, drafts_resume, forward, reply, reply_all, schedule_send, send_draft,
ComposeOptions, ForwardCommand, ReplyCommand,
};
Expand Down
Loading
Loading