Skip to content

fix(mobile-chat): a failed message's text is added to the draft, and an expired resend no longer blocks that text - #25149

Merged
brennanb2025 merged 4 commits into
mainfrom
brennanb2025/chat-phone-send-text-kept
Oct 5, 2026
Merged

brennanb2025 merged 4 commits into
mainfrom
brennanb2025/chat-phone-send-text-kept

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​64 $\color{#cf222e}{\Huge{\mathbf{−}}}$​11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​53
Prod 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​48 $\color{#cf222e}{\Huge{\mathbf{−}}}$​22 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​26

ELI5

On the phone, two things could go wrong after a chat message failed to send:

  1. If you typed something new while the failed message was still on its way, the failed message's text was thrown away. Now it is added to the end of what you typed, after a blank line.
  2. If you retyped a message whose first send never got an answer, and you did it more than a day later, the computer refused it, and kept refusing that exact text in that chat forever. Now the phone gives the text back once with "Orca couldn't confirm your message reached the agent. Check the chat, then send it again if needed.", and the next send goes through as a new message.

What Changed

The problem. This is about structured (native) chats in the phone app.

  • Text was dropped. When the computer definitely refuses a phone message, the phone puts its text back in the composer. It did that only if the composer was still empty and untouched since you pressed Send. If you had typed anything in the meantime, even if you then deleted it, the failed message's text was silently discarded. Nothing on screen said it was gone.
  • A text could be refused forever. The phone gives each message an id so that the computer can tell a resend from a new message. When a send gets no answer (a dropped connection, for example), the phone remembers that id under the message's exact text. If you type the same text again, it resends under the same id, so the message can't arrive twice. The computer only remembers ids for a day. After that it answers a resent old id with "expired" and never runs it. The phone treated "expired" like any other refusal of a resend and kept the id, so every later send of that same text in that chat reused it and was refused again. You saw "Your message was not sent." each time, with no way out except changing the text.

Before / after (phone, structured chat)

You do Before After
Send a message, type something new while it is in flight, and the computer refuses the message Your new typing stays; the refused message's text is gone Your new typing stays, and the refused message's text is added after it, a blank line apart
Same, but you typed and then cleared the composer The refused text is gone The refused text comes back
The same text comes back twice (could not happen: the second was dropped) It appears once
Retype a message more than a day after its send went unanswered "Your message was not sent." on every try, forever Once: "Orca couldn't confirm your message reached the agent. Check the chat, then send it again if needed." The text is back in the composer, and sending it again goes through as a new message

On the desktop, two places already appended text to the composer after a blank line: a Stop that gives back messages it withdrew, and Edit on a queued message card (the message queue is not switched on for users yet). Both now use the same rule as the phone. The one visible difference: if the composer already ends with exactly that text, it isn't added a second time. Two returned messages with identical text therefore leave one copy, as in the common pattern.

Mechanism.

  • mobile/src/session/mobile-structured-send-delivery.ts: a resend refused as agent_session_operation_expired now marks its id as spent. The phone clears the saved id, so the next send of that text gets a new one. The words are the existing "couldn't confirm" sentence, not "not sent": the first attempt may have reached the chat a day ago.
  • src/shared/returned-draft-text.ts (new): one rule for putting returned text back in a composer. It appends after a blank line, never replaces what's there, and does nothing if the draft already ends with that text.
    • The phone's restoreRejectedDraft (use-mobile-native-chat-drafts.ts) now uses it instead of "only into an empty, untouched composer".
    • The desktop's draft cache (native-chat-draft-cache.ts, use-native-chat-draft.ts) uses it in place of its own copy.
  • No wire, request or host change.

Not in this PR: the phone still hands back the text of a message the computer recorded and then failed to deliver, so that text can appear both in the chat and in the composer. #24918 stops that, as part of showing recorded messages to every viewer. This PR doesn't touch that path; until #24918 lands, that text is appended like any other returned text.

Why

This is the "phone fix" step of the failed-send redesign. The rule there is: a message the computer has no record of, and never will, goes back to the composer, and its text is never lost; an id the computer can no longer accept is used up.

  • Append, not "only if empty". The common pattern for a chat client that keeps its own record of unsent messages appends returned text after a blank line and skips text that's already there. The alternative, returning text only into an empty composer, is what lost text here.
  • Expired means used up. The computer never replays or reruns an expired id (agent-session-operation-ledger.ts), so keeping it can only refuse the message again. Releasing it is the one way out of the loop.
  • No automatic resend under a new id. That could deliver the message twice if the first attempt landed a day ago. The text comes back with words that say to check the chat first.
  • One shared rule. The desktop already appended returned text. Two copies of the same rule would drift, so both now use one function.

Differences from the common pattern

Difference Label
The phone still remembers an unanswered message's id by its exact text, so retyping the text resends it. The common pattern keeps the id on a saved list of unsent messages instead. Temporary. A later step gives the phone a saved list of unsent messages, built from the same pieces as the desktop's, and removes the text-keyed ids. This PR only stops them from blocking a text forever.
An expired resend gives the text back with "couldn't confirm" words instead of resending it under a new id. Intended. The first attempt may already be in the chat; resending could duplicate it.
Trailing whitespace on the draft is trimmed before the blank line, and a whitespace-only draft counts as empty. Intended. The desktop already trimmed. It keeps one blank line between the two texts.
Until #24918 lands, a message the computer recorded and then failed to deliver still comes back to the phone's composer (now appended). Temporary. #24918 removes that hand-back.
The phone's composer text lives in memory for the open chat screen. Text handed back after you leave that chat, or after the app restarts, is still lost. The common pattern saves drafts per chat. Temporary. The later phone step above saves the unsent list and its returned text per chat.
A /command whose outcome the phone couldn't confirm still comes back to the composer (now appended after anything typed, where before it came back only into an empty composer). It may have run. Temporary. The redesign stops handing back anything whose outcome is unknown; that lands with the phone's saved unsent list.
A resend only reaches "expired" when the computer has that chat open. If the computer has closed it (after 30 minutes idle, or a restart), it answers "not attached" before looking the id up, and the phone still shows "Your message was not sent. Send it again." for that text until the chat is open again or you change the text. Temporary. A host change in the same redesign makes the computer look the id up first, so it answers "expired" either way. The phone can't tell this answer from a real "not attached".
Because ids are remembered by text, a new message identical to one whose send went unanswered in the last day (for example "yes") is treated as a resend of it. After a day it now comes back once with the "couldn't confirm" words instead of being refused forever. Temporary. Removed with the text-keyed ids in the later phone step.
The phone's saved ids have no expiry of their own. "Expired" now frees an id when its text is retyped; ids for texts never retyped stay until that later step. Temporary. Same later phone step.

Linked Issue

None. This is step "phone fix" of the native-chat failed-send redesign.

Visual Proof

Phone, iOS Simulator (iPhone 17 Pro, iOS 26.5), driven with orca emulator against an isolated test host running a stand-in Claude. Same app build for both columns: only the two phone files this PR changes were swapped to main for "Before". Steps: send "Please fix the login bug", type "Also check the tests" while it is in flight, and the computer refuses the message (the stand-in agent fails to start).

Before (main) After (this PR)
s1-before s1-after
The refused message's text is gone; only the new typing is left. The new typing stays and the refused text comes back after it, a blank line apart.
  • The same text handed back while the composer already ends with it: one copy (checked on this PR only; on main it is dropped anyway).
  • The refusal used here is one the computer recorded before the agent failed to start: no slow refusal of an unrecorded message could be reached. That kind of message stops coming back to the composer with fix(native-chat): the phone keeps a message the host did not deliver, and a failed /compact is said once #24918; the append rule is the same for every returned text.
  • Not captured: the expired-id case. It needs a message id more than a day old, and nothing was changed to fake one. It is covered by the send-hook test, which runs the real phone send path against the host's "expired" answer.
  • No new notice, label or line appeared on this PR's side. The repeated "Claude stopped before it finished starting" rows are from the earlier QA attempts in the same chat and appear on both sides.
  • Screenshots are taken from the headless simulator's own screen capture (simctl io screenshot), not a desktop window.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • src/shared/returned-draft-text.test.ts (new): append after a blank line, keep typing, skip text that's already there, empty and whitespace-only drafts.

  • mobile/src/session/use-mobile-native-chat-drafts.test.ts: a refused send is appended after typing done while it was in flight, and comes back even after that typing was cleared. Before, both tests asserted that the text was dropped.

  • mobile/src/session/mobile-structured-send-delivery.test.ts, use-mobile-structured-agent-session-send.test.tsx: an expired resend spends its id and shows the "couldn't confirm" words. The next send of the same text goes out under a new id and is accepted. Any other refusal of a resend still keeps its id.

  • structured-agent-session-withdrawn-message-restore.test.tsx: the crash-repeat test now starts from an empty composer, as a real crash does, and gets the text back once.

  • Ablation: with the two phone source files reverted to main, four of the new or changed tests fail.

  • Typecheck: web and node pass. Mobile passes apart from the generated webview modules that this worktree has not built (*.generated); the mobile tests-typecheck ratchet flags only files that import those same modules. CI builds them.

Review

Round 1 (three reviewers: phone, desktop and the shared rule, and an independent design check) found no problem this PR causes at P0–P2. Fixed: the PR text now names every desktop caller of the shared rule, and two comments are more precise. Deferred, with the reason and the later step named under "Differences from the common pattern": an expired resend while the computer has the chat closed, an unconfirmed /command coming back to the composer, and saved ids with no expiry of their own.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Mobile and older computers: no request, response or stream changes. An older computer answers an expired id the same way, so this works against any host version.
  • SSH and folder workspaces: nothing here depends on where the chat runs.
  • Performance: one string comparison per returned message.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review and QA summary

The problem. In a structured chat on the phone, two things went wrong after a send failed:

  • If you typed anything while the message was on its way and the computer then refused it, the refused message's text was thrown away without a word.
  • If a message's first send got no answer, and you retyped that same text more than a day later, the computer answered "expired". The phone kept the old id and was refused again on every try. That exact text could never be sent from the phone in that chat.

What changes for you.

  • Refused text now comes back after whatever is in the composer, a blank line apart, and is never dropped. If the composer already ends with that text, it isn't added again.
  • An expired resend comes back once with "Orca couldn't confirm your message reached the agent. Check the chat, then send it again if needed." The next send goes out as a new message.
  • On the desktop, the two places that already appended returned text (a Stop giving back withdrawn messages, and Edit on a queued message card) now use the same rule. The only visible difference: identical text isn't added twice.

Review.

  • Round 1 used four separate reviewers: the phone send path, the desktop and the shared rule, an independent design check, and a release-readiness checklist. None found a problem this PR causes at P0–P2.
  • Fixed: the description now names every desktop place that uses the shared rule, and two code comments no longer claim more than the code does.
  • Deferred. Each item is labelled in the description, under "Differences from the common pattern", with the later step that removes it:
    • P2, cause is on the computer's side: an expired resend only reaches the new path when the computer has that chat open. A closed chat (30 minutes idle, or a restart) answers "not attached" before checking the id, so the phone still says "Your message was not sent. Send it again." for that text. The planned host change that checks the id first fixes it.
    • P3: a /command whose outcome couldn't be confirmed still comes back to the composer, now after anything typed.
    • P3: the phone's saved ids have no expiry of their own.
    • P3: phone drafts live only in memory for the open chat screen.
    • Duplicate-only nits: in two short windows (during on-screen-keyboard composition on desktop, and during the terminal chat's pre-send step on the phone), returned text can end up in the composer twice. It is never lost.
  • Found in QA, not caused by this PR, left for the phone's saved unsent list: the phone gives up on a send after 15 seconds, but the computer waits up to 2 minutes for the agent to start. If the agent then fails to start, the message disappears from the chat and its text never comes back. It happens the same way on main.

Verified.

  • Unit and hook tests on the real phone send path; with the two phone files reverted to main, four of them fail.
  • Web and node typechecks pass.
  • Mobile typecheck and its test ratchet pass, except for generated webview modules this checkout hadn't built. CI builds those, and CI is green.
  • Phone QA in the iOS Simulator, driven with orca emulator against an isolated test host. Before/after shots are in the description: the refused text is gone on main and appended after the new typing on this PR. No new notice appeared.

Not verified.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 4, 2026 06:57
@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 75af2fc9-2feb-4c1f-b254-b52952816190
📥 Commits

Reviewing files that changed from the base of the PR and between 3cc5e1d and 3a680a8.

📒 Files selected for processing (10)
  • mobile/src/session/mobile-structured-send-delivery.test.ts
  • mobile/src/session/mobile-structured-send-delivery.ts
  • mobile/src/session/use-mobile-native-chat-drafts.test.ts
  • mobile/src/session/use-mobile-native-chat-drafts.ts
  • mobile/src/session/use-mobile-structured-agent-session-send.test.tsx
  • src/renderer/src/components/native-chat/native-chat-draft-cache.ts
  • src/renderer/src/components/native-chat/structured-agent-session-withdrawn-message-restore.test.tsx
  • src/renderer/src/components/native-chat/use-native-chat-draft.ts
  • src/shared/returned-draft-text.test.ts
  • src/shared/returned-draft-text.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Expired operation IDs are now marked spent when a retained send receives an expiration refusal, and the delivery returns a send error notice. A shared helper now combines returned text with existing draft text. Mobile draft restoration and renderer draft handling use this helper. Tests cover expiration and replay, draft appending, and withdrawn-message recovery.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 3a680

The changed send and draft-restoration behavior has no identified issue requiring a fix before merge.

Architecture Summary

Architecture risk: 🟡 Medium · up to 3a680

The change affects 2 systems.

Changed systems: src, mobile

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (ui) was modified; 5 changed files map to changed impact.
  • observed — mobile (service) was modified; 5 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in mobile/src/session/mobile-structured-send-delivery.test.ts: The expired-ID test now expects the ID to remain spent and the error to advise checking the chat, replacing the prior assertion that later refusals or failures never release an ambiguous ID.
  • observed — Modified behavior in mobile/src/session/mobile-structured-send-delivery.test.ts: The test for other later refusals or failures now uses an operation-conflict refusal and expects the ID to remain unspent, rather than expecting an expired-operation refusal to remain unspent.
  • observed — Modified behavior in mobile/src/session/mobile-structured-send-delivery.ts: The refusal documentation now includes expired IDs as a second exception that spends the ID, in addition to replay request-shape refusals.
  • observed — Modified behavior in mobile/src/session/mobile-structured-send-delivery.ts: Imports agentSessionWriteNoticeEnglish for the expired-operation refusal notice.

Reliability and maintainability

  • inferred — Risk-relevant change factors for src: blast_radius_3; direct_dependents_3
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes both main changes: restoring failed message text to the draft and preventing expired resends from blocking later sends. It is specific, though longer than necessary.
Description check ✅ Passed The description covers the required sections with detailed user impact, implementation, rationale, visual proof, and reported testing. It lists no linked issue; an external contributor must add an iss…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Returned text is appended, not dropped — the new shared appendReturnedDraftText puts returned text after whatever the composer holds, a blank line apart, treats a whitespace-only draft as empty, and is idempotent so the same text handed back twice leaves one copy. The phone's restoreRejectedDraft and the desktop draft cache/use-native-chat-draft both route through it, replacing the two divergent copies.
  • An expired resend id is spent — mobileStructuredSendDelivery now releases a retained operation id the host has expired (agent_session_operation_expired) and reports the existing "couldn't confirm" notice, so the next send of the same text mints a fresh id instead of replaying a refused one forever. Every other refusal of a replay still keeps its id.
  • Tests — a new unit test for the shared rule, plus updated phone and desktop tests covering append-after-typing, restore-after-an-explicit-clear, the fresh-id resend, and the crash-repeat restore.

I traced the expired branch past agentSessionRefusalOperationState (expired maps to settled-rejected, so the new branch is reachable), the operationIdSpent → clearMobileStructuredSendOperation release path, and the shared rule's idempotency/whitespace edge cases. The focused suites pass locally (shared 4, mobile 54, desktop 27) and pnpm run check:code-quality:changed is clean.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025
brennanb2025 merged commit b354aab into main Oct 5, 2026
59 checks passed
brennanb2025 added a commit that referenced this pull request Oct 6, 2026
Drafts take #24905's merged store. A hand-back's text follows the shared
returned-text rule (#25149) through returnNativeChatDraftText, which trims
only the end and now trusts the append's durable answer; a composer is told
of an append only when it changed the draft. The withdrawn-message restore
test stays deleted: its one change was the rule's dedupe, which C2's
returning tests already state. messagesUnsettled keeps C2's 'wait' in the
moved reason-words table.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant