feat: edit stored drafts in place across CLI, TUI, and web#110
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (32)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/daemon/src/handler/mod.rs">
<violation number="1" location="crates/daemon/src/handler/mod.rs:2215">
P0: `UpdateDraft` derives its authorization scope from the caller-supplied `draft.account_id` rather than the stored draft's actual account. Because `request_account_scope` returns early via `request_account_id` and never reaches the `draft_account_scope` branch for `UpdateDraft`, an agent restricted to account A can submit a draft ID belonging to account B while setting `draft.account_id = A` in the payload. The profile check passes for A, and `Store::update_draft` explicitly SETs `account_id` on the matched row, allowing the draft to be overwritten and reassigned across accounts.
Scope `UpdateDraft` from the stored draft ID instead: look up the existing draft's account in `request_account_scope` (or validate the payload's `account_id` against the stored row before updating), and consider omitting `account_id` from the UPDATE's SET list for in-place edits.</violation>
</file>
<file name="crates/compose/src/draft_codec.rs">
<violation number="1" location="crates/compose/src/draft_codec.rs:23">
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.</violation>
</file>
<file name="crates/tui/src/app/mailbox_actions.rs">
<violation number="1" location="crates/tui/src/app/mailbox_actions.rs:697">
P2: The stored-drafts edit flow clones a draft straight from the modals (`ListDrafts` snapshot) into `pending_draft_edit_request` without fetching the current row via `Request::GetDraft`. After the user finishes in `$EDITOR`, the stale snapshot plus local edits are sent as `UpdateDraft`, and the store's `update_draft` is a plain UPDATE with no compare-and-swap. If another client has modified the draft in the meantime, those changes get silently overwritten. Consider issuing `GetDraft` for the selected `DraftId` right before calling `prepare_draft_edit`, so the editor opens the freshest state rather than a potentially stale `ListDrafts` cache.</violation>
</file>
<file name="crates/store/src/draft.rs">
<violation number="1" location="crates/store/src/draft.rs:160">
P1: `update_draft` performs an unconditional UPDATE with only `id` and `status` in the WHERE clause, so concurrent edits from two clients silently overwrite each other (lost-update). Consider adding an optimistic concurrency check on `updated_at` so the second updater receives a conflict instead of clobbering the first edit.</violation>
</file>
<file name="crates/web/src/routes_v6.rs">
<violation number="1" location="crates/web/src/routes_v6.rs:1966">
P2: The upsert fallback classifies daemon failures by an unstructured error-message substring (`"not found"`) rather than using a typed error variant or structured status code from the IPC layer. The PR description advertises a "precise UpdateDraft taxonomy" on the daemon side, but the web client discards that precision by grepping the display text of `BridgeError::Ipc`. This is fragile: a wording change in the daemon's error message will break the fallback, and any unrelated IPC error containing `"not found"` will incorrectly trigger a `SaveDraft` insert. Consider propagating a structured error code (e.g., an enum or numeric status) through the IPC/bridge layer so the client can match on a guaranteed contract instead of a free-text substring.</violation>
<violation number="2" location="crates/web/src/routes_v6.rs:1966">
P1: The `UpdateDraft`-to-`SaveDraft` fallback in `save_draft_local` can silently resurrect a draft that was deleted in another tab or client. Because the handler only receives a `Draft` payload with no creation intent or version, a \"not found\" error is ambiguous: it may mean the ID is new (intended insert) or that the row was deleted since this editor opened it (unintended resurrection). When multiple compose tabs reuse `intent.draftId` and one deletes the draft, a stale save from another tab will re-insert the deleted row via `SaveDraft`, making deletion non-durable.
**Recommendation**: Pass an explicit upsert intent or guard the insert path so it only runs for client-generated / explicitly-new drafts, not for IDs that originated from a `GetDraft` load.</violation>
</file>
<file name="crates/daemon/src/commands/mutations/compose.rs">
<violation number="1" location="crates/daemon/src/commands/mutations/compose.rs:671">
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.</violation>
<violation number="2" location="crates/daemon/src/commands/mutations/compose.rs:680">
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.</violation>
<violation number="3" location="crates/daemon/src/commands/mutations/compose.rs:685">
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.</violation>
</file>
<file name="crates/tui/src/compose_flow.rs">
<violation number="1" location="crates/tui/src/compose_flow.rs:243">
P2: Using `resolve_compose_account` for draft editing applies a send-capability eligibility check that is unnecessary for updating an existing draft. Drafts from disabled or non-sending accounts are listed by the daemon but cannot be edited in the TUI because this call rejects them. Use an account lookup that does not require `compose_account_eligible` for the edit path, or filter/label such drafts consistently in the modal.</violation>
<violation number="2" location="crates/tui/src/compose_flow.rs:253">
P1: Using a deterministic scratch filename based only on `DraftId` and unconditionally removing it before writing destroys previously preserved recovery files from failed edits and creates a race between concurrent TUI sessions editing the same draft. Consider a unique per-edit filename and retaining failed files instead.</violation>
<violation number="3" location="crates/tui/src/compose_flow.rs:324">
P2: On non-zero editor exit, the flow displays 'Edit cancelled' and queues deletion of the temp file, assuming the user intentionally discarded changes. But editors (or wrappers like `git rebase --exec` pipelines) can write to disk before exiting non-zero, so this path can silently discard actual edits. Also, when the editor fails to launch (`Err(error)`), the error message does not include the temp file path, unlike the parse/update failure paths which explicitly say 'Your edits are still at …'. Since the PR's design intent for this function is to preserve the temp file and surface its path (as evidenced by the careful error handling around parsing and `UpdateDraft` failures), these two branches are inconsistent and leave gaps for silent content loss.</violation>
<violation number="4" location="crates/tui/src/compose_flow.rs:327">
P2: When `UpdateDraft` returns an unexpected successful response shape, the scratch file is preserved but its path is not disclosed in the error message, making recovery impossible for the user. Include `data.path.display()` in this branch, matching the adjacent `Response::Error` and transport-error branches.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| Request::GetSyncStatus { account_id } => Some(account_id), | ||
| Request::SendDraft { draft, .. } | ||
| | Request::SaveDraft { draft } | ||
| | Request::UpdateDraft { draft } |
There was a problem hiding this comment.
P0: UpdateDraft derives its authorization scope from the caller-supplied draft.account_id rather than the stored draft's actual account. Because request_account_scope returns early via request_account_id and never reaches the draft_account_scope branch for UpdateDraft, an agent restricted to account A can submit a draft ID belonging to account B while setting draft.account_id = A in the payload. The profile check passes for A, and Store::update_draft explicitly SETs account_id on the matched row, allowing the draft to be overwritten and reassigned across accounts.
Scope UpdateDraft from the stored draft ID instead: look up the existing draft's account in request_account_scope (or validate the payload's account_id against the stored row before updating), and consider omitting account_id from the UPDATE's SET list for in-place edits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/handler/mod.rs, line 2215:
<comment>`UpdateDraft` derives its authorization scope from the caller-supplied `draft.account_id` rather than the stored draft's actual account. Because `request_account_scope` returns early via `request_account_id` and never reaches the `draft_account_scope` branch for `UpdateDraft`, an agent restricted to account A can submit a draft ID belonging to account B while setting `draft.account_id = A` in the payload. The profile check passes for A, and `Store::update_draft` explicitly SETs `account_id` on the matched row, allowing the draft to be overwritten and reassigned across accounts.
Scope `UpdateDraft` from the stored draft ID instead: look up the existing draft's account in `request_account_scope` (or validate the payload's `account_id` against the stored row before updating), and consider omitting `account_id` from the UPDATE's SET list for in-place edits.</comment>
<file context>
@@ -2204,6 +2212,7 @@ fn request_account_id(req: &Request) -> Option<&mxr_core::AccountId> {
Request::GetSyncStatus { account_id } => Some(account_id),
Request::SendDraft { draft, .. }
| Request::SaveDraft { draft }
+ | Request::UpdateDraft { draft }
| Request::SaveDraftToServer { draft }
| Request::CheckDraftSafety { draft, .. }
</file context>
| // `created_at` is intentionally absent from SET so the original | ||
| // creation time survives the edit. Plain UPDATE (not INSERT OR | ||
| // REPLACE) so the account_id FK's ON DELETE CASCADE is never armed. | ||
| let result = sqlx::query( |
There was a problem hiding this comment.
P1: update_draft performs an unconditional UPDATE with only id and status in the WHERE clause, so concurrent edits from two clients silently overwrite each other (lost-update). Consider adding an optimistic concurrency check on updated_at so the second updater receives a conflict instead of clobbering the first edit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/store/src/draft.rs, line 160:
<comment>`update_draft` performs an unconditional UPDATE with only `id` and `status` in the WHERE clause, so concurrent edits from two clients silently overwrite each other (lost-update). Consider adding an optimistic concurrency check on `updated_at` so the second updater receives a conflict instead of clobbering the first edit.</comment>
<file context>
@@ -133,6 +133,56 @@ impl super::Store {
+ // `created_at` is intentionally absent from SET so the original
+ // creation time survives the edit. Plain UPDATE (not INSERT OR
+ // REPLACE) so the account_id FK's ON DELETE CASCADE is never armed.
+ let result = sqlx::query(
+ "UPDATE drafts
+ SET account_id = ?, in_reply_to = ?, intent = ?,
</file context>
| .await | ||
| { | ||
| Ok(response) => passthrough(response), | ||
| Err(BridgeError::Ipc(message)) if message.to_lowercase().contains("not found") => { |
There was a problem hiding this comment.
P1: The UpdateDraft-to-SaveDraft fallback in save_draft_local can silently resurrect a draft that was deleted in another tab or client. Because the handler only receives a Draft payload with no creation intent or version, a "not found" error is ambiguous: it may mean the ID is new (intended insert) or that the row was deleted since this editor opened it (unintended resurrection). When multiple compose tabs reuse intent.draftId and one deletes the draft, a stale save from another tab will re-insert the deleted row via SaveDraft, making deletion non-durable.
Recommendation: Pass an explicit upsert intent or guard the insert path so it only runs for client-generated / explicitly-new drafts, not for IDs that originated from a GetDraft load.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/web/src/routes_v6.rs, line 1966:
<comment>The `UpdateDraft`-to-`SaveDraft` fallback in `save_draft_local` can silently resurrect a draft that was deleted in another tab or client. Because the handler only receives a `Draft` payload with no creation intent or version, a \"not found\" error is ambiguous: it may mean the ID is new (intended insert) or that the row was deleted since this editor opened it (unintended resurrection). When multiple compose tabs reuse `intent.draftId` and one deletes the draft, a stale save from another tab will re-insert the deleted row via `SaveDraft`, making deletion non-durable.
**Recommendation**: Pass an explicit upsert intent or guard the insert path so it only runs for client-generated / explicitly-new drafts, not for IDs that originated from a `GetDraft` load.</comment>
<file context>
@@ -1942,20 +1942,39 @@ async fn send_stored_draft(
+ .await
+ {
+ Ok(response) => passthrough(response),
+ Err(BridgeError::Ipc(message)) if message.to_lowercase().contains("not found") => {
+ let response = dispatch(
+ &state,
</file context>
| 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()? |
There was a problem hiding this comment.
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>
| // Best-effort: clear a stale file left behind by a previous | ||
| // cancelled/failed edit of this same draft — `write_private` uses | ||
| // O_EXCL and would otherwise refuse to write. | ||
| let _ = tokio::fs::remove_file(&path).await; |
There was a problem hiding this comment.
P1: Using a deterministic scratch filename based only on DraftId and unconditionally removing it before writing destroys previously preserved recovery files from failed edits and creates a race between concurrent TUI sessions editing the same draft. Consider a unique per-edit filename and retaining failed files instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 253:
<comment>Using a deterministic scratch filename based only on `DraftId` and unconditionally removing it before writing destroys previously preserved recovery files from failed edits and creates a race between concurrent TUI sessions editing the same draft. Consider a unique per-edit filename and retaining failed files instead.</comment>
<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+ // Best-effort: clear a stale file left behind by a previous
+ // cancelled/failed edit of this same draft — `write_private` uses
+ // O_EXCL and would otherwise refuse to write.
+ let _ = tokio::fs::remove_file(&path).await;
+ mxr_compose::private_tmp::write_private_async(&path, content.as_bytes())
+ .await
</file context>
| validate_compose_draft(&frontmatter, &body, false)?; | ||
|
|
||
| let updated = | ||
| mxr_compose::draft_codec::apply_edited_compose_file(&draft, &edited, chrono::Utc::now())?; |
There was a problem hiding this comment.
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>
| 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)?; |
There was a problem hiding this comment.
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>
| bg: &mpsc::UnboundedSender<IpcRequest>, | ||
| draft: Draft, | ||
| ) -> Result<DraftEditReadyData, MxrError> { | ||
| let account = resolve_compose_account(bg, Some(&draft.account_id)).await?; |
There was a problem hiding this comment.
P2: Using resolve_compose_account for draft editing applies a send-capability eligibility check that is unnecessary for updating an existing draft. Drafts from disabled or non-sending accounts are listed by the daemon but cannot be edited in the TUI because this call rejects them. Use an account lookup that does not require compose_account_eligible for the edit path, or filter/label such drafts consistently in the modal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 243:
<comment>Using `resolve_compose_account` for draft editing applies a send-capability eligibility check that is unnecessary for updating an existing draft. Drafts from disabled or non-sending accounts are listed by the daemon but cannot be edited in the TUI because this call rejects them. Use an account lookup that does not require `compose_account_eligible` for the edit path, or filter/label such drafts consistently in the modal.</comment>
<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+ bg: &mpsc::UnboundedSender<IpcRequest>,
+ draft: Draft,
+) -> Result<DraftEditReadyData, MxrError> {
+ let account = resolve_compose_account(bg, Some(&draft.account_id)).await?;
+ let content = mxr_compose::draft_codec::draft_to_compose_file(&draft, &account.email)
+ .map_err(|e| MxrError::Ipc(e.to_string()))?;
</file context>
| ), | ||
| ); | ||
| } | ||
| Ok(_) => { |
There was a problem hiding this comment.
P2: On non-zero editor exit, the flow displays 'Edit cancelled' and queues deletion of the temp file, assuming the user intentionally discarded changes. But editors (or wrappers like git rebase --exec pipelines) can write to disk before exiting non-zero, so this path can silently discard actual edits. Also, when the editor fails to launch (Err(error)), the error message does not include the temp file path, unlike the parse/update failure paths which explicitly say 'Your edits are still at …'. Since the PR's design intent for this function is to preserve the temp file and surface its path (as evidenced by the careful error handling around parsing and UpdateDraft failures), these two branches are inconsistent and leave gaps for silent content loss.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 324:
<comment>On non-zero editor exit, the flow displays 'Edit cancelled' and queues deletion of the temp file, assuming the user intentionally discarded changes. But editors (or wrappers like `git rebase --exec` pipelines) can write to disk before exiting non-zero, so this path can silently discard actual edits. Also, when the editor fails to launch (`Err(error)`), the error message does not include the temp file path, unlike the parse/update failure paths which explicitly say 'Your edits are still at …'. Since the PR's design intent for this function is to preserve the temp file and surface its path (as evidenced by the careful error handling around parsing and `UpdateDraft` failures), these two branches are inconsistent and leave gaps for silent content loss.</comment>
<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+ ),
+ );
+ }
+ Ok(_) => {
+ app.report_error(
+ "Update Draft Failed",
</file context>
| Ok(_) => { | ||
| app.report_error( | ||
| "Update Draft Failed", | ||
| "Unexpected daemon response to UpdateDraft".to_string(), |
There was a problem hiding this comment.
P2: When UpdateDraft returns an unexpected successful response shape, the scratch file is preserved but its path is not disclosed in the error message, making recovery impossible for the user. Include data.path.display() in this branch, matching the adjacent Response::Error and transport-error branches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 327:
<comment>When `UpdateDraft` returns an unexpected successful response shape, the scratch file is preserved but its path is not disclosed in the error message, making recovery impossible for the user. Include `data.path.display()` in this branch, matching the adjacent `Response::Error` and transport-error branches.</comment>
<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+ Ok(_) => {
+ app.report_error(
+ "Update Draft Failed",
+ "Unexpected daemon response to UpdateDraft".to_string(),
+ );
+ }
</file context>
| "Unexpected daemon response to UpdateDraft".to_string(), | |
| format!("Unexpected daemon response to UpdateDraft\n\nYour edits are still at {}", data.path.display()), |
bhekanik
force-pushed
the
feat/edit-draft-integration
branch
from
6a69a61 to
89811fa
Compare
…teDraft IPC, compose codec)
save-local now tries UpdateDraft before falling back to SaveDraft, so the endpoint upserts by id. The compose session reuses the restored draft's id when scheduling a send on an edited draft, instead of minting a new UUID that orphaned the original stored draft.
Opens the draft in $EDITOR via draft_to_compose_file/apply_edited_compose_file and saves through Request::UpdateDraft, keeping the same DraftId (created_at preserved, updated_at bumped). Uses the plain std::env::temp_dir() pattern already used elsewhere in compose.rs for the scratch file, since the private_tmp module referenced in the task foundation isn't present on this branch yet.
Add a stored-drafts modal (gE / command palette "Drafts") that lists locally-saved drafts across all accounts via Request::ListDrafts. Pressing e/Enter on a selected draft resolves the sending account, renders it to a compose file via mxr_compose::draft_codec, and opens it in $EDITOR. On save, the edited content is parsed back and applied onto the existing draft (preserving id/account_id/created_at) and saved via Request::UpdateDraft, so editing a draft no longer creates a duplicate. Also brings in the mxr_compose::private_tmp module (private 0700 scratch dir, 0600 O_EXCL file writes) that the edit-draft foundation depends on but that this branch didn't yet have, and fixes a clippy lint (io_other_error) introduced by that addition.
bhekanik
force-pushed
the
feat/edit-draft-integration
branch
from
89811fa to
bbef87e
Compare
1 participant
What
Adds edit-draft-in-place: open an existing stored draft, edit it, and save it back under the same
DraftId— no duplicate draft row, nothing discarded. Wired across all three clients on one daemon surface.Layers
Request::GetDraft/Request::UpdateDraftIPC +ResponseData::Draft;Store::update_draft(plain UPDATE that preservescreated_at, only touches rows still in'draft'status, never arms the FK cascade);draft_codec(render aDraftto a compose file and apply edits back).mxr drafts edit <id>— fetches the draft, opens$EDITOR, validates, saves viaUpdateDraft. Temp file goes to the private 0700/0600 scratch dir.prepare_draft_edit/handle_draft_edit_statuspreserve the temp file and surface its path on any parse/update failure (no silent content loss).save_draft_localis now upsert-by-id (UpdateDraftfirst,SaveDraftfallback on not-found); the compose hook reusesintent.draftIdinstead of minting a new UUID when editing.Error taxonomy
update_draftdistinguishes not-found / mid-send / already-sent, so the daemon returns a precise reason and clients never silently no-op.Integration notes
This branch is the 4 feature branches (foundation + CLI + TUI + web) rebased cleanly onto v0.6.5 (0 conflicts). Verified locally:
cargo build -p mxrok; tests pass for mxr-compose (46), mxr-store (190), mxr-protocol, mxr-tui (22), mxr-web (6), and the mxr daemon/CLI suite (incl. newdrafts editcli-help snapshot);clippyclean on compose/daemon/tui/web.Guardrails honored
New capability is daemon IPC + CLI, with TUI/web on the same surface; compose uses
$EDITOR; unsent draft content is written only to the private scratch dir.Summary by cubic
Edit stored drafts in place across CLI, TUI, and web—updates the same
DraftIdinstead of creating duplicates. AddsGetDraft/UpdateDraftIPC, store update-in-place, a shared compose round-trip codec, and saves temp files to a private 0700/0600 scratch dir.mxr drafts edit <id>opens the draft in $EDITOR and saves viaUpdateDraft(sameDraftId); enforces account match and writes temp files to the private scratch dir.gEor command palette) lists local drafts; Enter/eedits in place and preserves the temp file path on parse/update failure.save_draft_localis now upsert-by-id (tryUpdateDraft, fallback toSaveDraft); compose reusesintent.draftIdwhen editing to avoid duplication.Request::GetDraftandRequest::UpdateDraft;Store::update_draftpreservescreated_at, updates only'draft'rows, and returns precise errors (not-found / sending / sent).draft_codecto render aDraftto compose-file text and apply edits back, shared by CLI/TUI/web.Written for commit bbef87e. Summary will update on new commits.