Repository navigation
fix: remove a saved draft once its email is sent - #170
LUCKYREDDY31 wants to merge 2 commits into
Conversation
Sending a saved draft left it in Drafts after the email went out. The send request did not say which draft it came from, and nothing removed drafts, so the person saw an email that looked unsent and could send it twice. The email.send proposal now carries an optional draftId, and the action service removes that draft only after the send succeeds. A declined, expired, failed or uncertain send keeps the draft. Edit details on the review reopens the same draft instead of creating a copy.
NathanTarbert
left a comment
There was a problem hiding this comment.
Thanks @LUCKYREDDY31. Removing a draft once its email goes out is the right behavior, and keeping the draft when a review is denied or fails makes sense too. A few situations can still lose someone's work or send an email twice, so I'd like to sort those out before merging.
Edits made after starting a review can be lost. Say someone opens a draft and taps Review email, then backs out without deciding. They reopen the draft, rewrite it and save. Later they approve the original review from Activity. The old text gets sent and the draft is deleted, so the rewrite is gone. Nothing checks that the draft still matches what was reviewed before removing it.
A failed cleanup looks like a failed send. The draft is removed after the email is already saved as sent (actions.ts:207), and nothing catches an error there. If removing the draft fails, for example because of a brief database error, the user sees an error even though the email went out. The draft is also still there. The natural next step is to send it again, which is the duplicate this PR is trying to prevent. Treating the removal as best effort, with a try/catch and a log line, would keep a cleanup problem from looking like a send problem.
One draft can be sent twice. Tapping Review email on the same draft twice creates two pending sends. Approving the first sends the email and removes the draft, but the second is still waiting in Activity, and approving it sends the same email again.
Edit details can break afterward. In that same case, opening the second pending send and tapping Edit details opens the editor for a draft that no longer exists. Saving then fails with "Draft not found," and the user has to start over.
I think all four come from the same place. The draft and its pending send aren't linked on the server, so each case needs its own special handling. One way to link them would be to store the pending send's id on the draft. A new review or a new save would then replace the older pending send, and sending would only remove the draft if it's still the version that was reviewed. Happy to talk through other approaches if you see a simpler one.
Two smaller things. propose accepts any draftId without checking that the draft exists for this user, so it's worth validating it there. And the new field uses z.string().uuid(), while the rest of the schema file uses Zod 4's z.uuid() style.
It would also be good to add tests for the cases above, and for an expired review keeping its draft, which the description mentions but no test covers yet.
A draft now records its pending review. Starting a new review or saving the draft again cancels the older review, so one draft has at most one pending send and older text cannot be sent over a rewrite. A send removes the draft only if it still points to that review, and a failed cleanup is logged instead of reported as a failed send. propose rejects a draft that does not exist for the owner, and Edit details opens the editor only when its own tap declined the review.
|
Thanks @NathanTarbert, this was a really useful review. I reproduced the first three cases on the previous commit and went with the linked approach you suggested. The draft now records its pending review:
|
Problem
Sending a saved draft leaves the draft behind. In sample mode: open Mail, tap Compose, write an email and tap Save draft, open it from Drafts, tap Review email, then Approve locally. The email appears in All messages, but Drafts still lists it. Nothing ever removes a draft: the server only lists and saves drafts, and the send request does not say which draft it came from. There is also no way to delete a draft by hand, so the person sees an email that looks unsent and can easily send the same email twice.
What changed
packages/domain/src/index.ts: anemail.sendproposal accepts an optionaldraftId(z.uuid()), andActionProposalkeeps it. It sits next todata, not insideemailDraftSchema, so the model'sprepare_emailtool, which buildsdatafromemailDraftSchema, cannot set it.apps/server/src/actions.ts:proposerejects adraftIdthat does not exist for the owner (404), saves the new review's ID on the draft, and cancels any older pending review of that draft, so one draft has at most one pending send.decideremoves the draft only when the send succeeded and the draft still points to that review. A declined, expired, failed or uncertain send keeps the draft. The removal runs after the send is recorded; if it fails, the error is logged withbackgroundFailureand the send still reports success.apps/server/src/app.ts: saving an existing draft cancels its pending review, because that review no longer matches the draft. The saved draft no longer points to the old review, so even a send already in progress keeps the rewrite.apps/server/src/db.ts:removeIfdeletes a record only while it still contains the expected fields, the delete counterpart ofcompareAndSwap.apps/mobile/src/details.tsx: Review email sends the draft's ID with the proposal. Edit details reopens the same draft, but only when this tap declined the review. If the review was already replaced or sent elsewhere, the screen shows that outcome instead of an editor. This check applies to calendar reviews too.tests/actions.test.ts: the draft is removed after a send, and kept when the send is declined, fails, has an unknown outcome or expires. A rewrite saved after the review started is kept, a newer review replaces the older one so the email is sent once, a failed cleanup still reports the send, and a draft from another owner or an unknown ID is rejected.tests/api.test.ts: saving a draft again through the API cancels its pending review, so the older text is never sent.Screenshots
Sample mode, web, same steps before and after: save a draft, open it from Drafts, tap Review email, tap Approve locally, then Done.
after Save draft
The saved draft
The saved draft
after Review email
Waiting for approval
Waiting for approval
after Approve locally
Saved in local Sent mail
Saved in local Sent mail
after Done
The sent email is still a draft
The draft is removed
Verification
main, the draft-removal test fails. On the first version of this PR (e01080e), the five tests added for the review fail: rewrite kept, newer review replaces the older one, failed cleanup, owner check, and re-save cancels the review. The expired-review test passes on both, because expired reviews already kept the draft.tests/actions.test.tsandtests/api.test.tspassed 20 of 20 repeated runs.pnpm lint,pnpm typecheck,pnpm --dir apps/worker typecheck,pnpm test(491/491) andpnpm build:serverpass on Node 24. The web, iOS and Android exports,pnpm test:browser, and both Docker container suites also pass.mainat 73a7149), the server listed 1 draft after a successful send without the fix and 0 with it. The newer commits onmaindo not change the send, review or drafts code shown there.app.tsandpackages/domain/src/index.ts, and applies together with this change without conflicts. feat: run chat fully offline with CopilotKit Intelligence optional #94 adds a route directly below the draft save route inapp.ts, so the two touch neighboring lines; feat: run chat fully offline with CopilotKit Intelligence optional #94 already conflicts withmainon its own.Integration limits
decidepath removes the draft after the Gmail send succeeds, but I have not run it with a real Google account.Security review
proposeonly accepts adraftIdthat exists for the same owner, so a proposal can only touch that owner's draft.draftIdmust be a UUID. Proposals prepared by agent tasks never carry one, because the model only supplies the email fields.