Skip to content

Fix Android chat optimistic send handling#90690

Closed
StoneHub wants to merge 1 commit into
openclaw:mainfrom
StoneHub:codex/android-chat-optimistic-send
Closed

Fix Android chat optimistic send handling#90690
StoneHub wants to merge 1 commit into
openclaw:mainfrom
StoneHub:codex/android-chat-optimistic-send

Conversation

@StoneHub

@StoneHub StoneHub commented Jun 5, 2026

Copy link
Copy Markdown

Summary

  • keep Android chat composer text/attachments until chat.send is accepted
  • include the backing gateway sessionId in Android chat.send requests
  • preserve optimistic user messages through terminal event/history refresh until history contains them
  • add focused OpenClawChat log lines for chat.send ACKs, failures, terminal events, and timeouts

Verification

  • Built Play debug APK locally with :app:assemblePlayDebug: BUILD SUCCESSFUL in 7s
  • Verified APK signature with apksigner: APK Signature Scheme v2 true
  • Installed on Pixel 6a via adb install -r: Success
  • Reproduced Main-session send after install: UI kept sent user bubble visible after chat.send status started and terminal final event

Notes

This fixes the Android UI disappearing-message symptom. During validation, Pixel-origin chat.send still finalized without an assistant transcript message, so this PR does not claim to fix the upstream/gateway no-assistant-output path.

@openclaw-barnacle openclaw-barnacle Bot added app: android App: android size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 5, 2026
@clawsweeper

clawsweeper Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 8:59 AM ET / 12:59 UTC.

Summary
The branch changes Android ChatController and ChatScreen to keep optimistic sends visible, pass sessionId, add send lifecycle logging, and clear the full ChatScreen composer only after send acceptance.

PR surface: Other +29. Total +29 across 2 files.

Reproducibility: yes. from source inspection, but not from a live Android run in this review. Current main still clears both manual Android composer surfaces before send acceptance, while current main already handles terminal ACK status in the controller.

Review metrics: 1 noteworthy metric.

  • Manual Android Composer Surfaces: 1 changed, 1 unchanged. Both ChatScreen and ChatSheetContent expose manual sends, so a one-surface composer fix leaves inconsistent Android send lifecycle behavior.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add inspectable redacted real-device or emulator proof showing the composer and sent bubble behavior after the fix.
  • Rebase on current main's terminal ACK parser and preserve status-aware acceptance behavior.
  • Apply acceptance-aware composer clearing to ChatSheetContent as well as ChatScreen, or document why the sheet path is intentionally excluded.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body describes a Pixel 6a validation run, but there is no inspectable screenshot, recording, terminal output, linked artifact, or redacted log showing the after-fix Android behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The branch is draft and GitHub reports it as conflicting, so it cannot land without a rebase that preserves current parseChatSendAck terminal status behavior.
  • [P1] Only ChatScreen is changed; ChatSheetContent still uses fire-and-forget sendChat and clears attachments immediately, leaving a sibling Android composer path with the same lifecycle behavior.
  • [P1] The PR body describes a Pixel 6a run, but it does not include inspectable screenshot, recording, terminal output, linked artifact, or redacted logs showing the after-fix Android behavior.
  • [P1] Passing the stored sessionId into Android chat.send changes session binding, so maintainers need focused proof that it targets the active session and does not mis-associate messages after session switches.

Maintainer options:

  1. Rebase And Cover Both Composers (recommended)
    Resolve the branch against current ChatController ACK handling, keep status-aware terminal branches intact, and apply acceptance-aware clearing to both ChatScreen and ChatSheetContent.
  2. Wait For Inspectable Android Proof
    Keep the PR out of the merge path until a redacted screenshot, recording, terminal capture, or log shows the after-fix composer and sent-bubble behavior on device or emulator.
  3. Replace The Draft If It Stays Conflicted
    If maintainers do not want to repair this draft branch, a new narrow PR can preserve attribution while carrying only the remaining Android composer/sessionId behavior.

Next step before merge

  • [P1] Human review is needed because the branch is conflicting and missing external Android proof; automation should not mark it repair-ready while contributor proof is absent.

Security
Cleared: The diff is limited to Android Kotlin chat UI/controller behavior and logging; no dependency, workflow, package, credential, or external-code-execution surface changes were found.

Review findings

  • [P2] Apply the acceptance-aware send path to the sheet composer — apps/android/app/src/main/java/ai/openclaw/app/ui/chat/ChatScreen.kt:222-226
Review details

Best possible solution:

Rebase or replace this with a narrow Android change that preserves current terminal ACK parsing, updates both manual composer surfaces to clear only after accepted sends, validates the sessionId binding, and includes redacted Android proof.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection, but not from a live Android run in this review. Current main still clears both manual Android composer surfaces before send acceptance, while current main already handles terminal ACK status in the controller.

Is this the best way to solve the issue?

No, not as submitted. The maintainable fix should compose with current status-aware ACK handling and update both Android manual composer surfaces, with sessionId behavior proven on a real device or emulator.

Full review comments:

  • [P2] Apply the acceptance-aware send path to the sheet composer — apps/android/app/src/main/java/ai/openclaw/app/ui/chat/ChatScreen.kt:222-226
    This only changes the full ChatScreen composer, but ChatSheetContent still calls fire-and-forget sendChat and clears attachments immediately. Users sending from the sheet surface would keep the same failed-send or disappearing-composer behavior, so the Android fix needs to update both composer paths or intentionally retire one.
    Confidence: 0.83

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 08d15ec32db3.

Label changes

Label justifications:

  • P2: This is a bounded Android chat lifecycle fix with limited blast radius but visible message and composer impact.
  • merge-risk: 🚨 session-state: The PR changes pending-run, optimistic-message, sessionId, and history-refresh state around Android chat.send.
  • merge-risk: 🚨 message-delivery: The PR changes when sent text and attachments remain visible or are cleared during Android chat.send acceptance and failure.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body describes a Pixel 6a validation run, but there is no inspectable screenshot, recording, terminal output, linked artifact, or redacted log showing the after-fix Android behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Other +29. Total +29 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 2 35 6 +29
Total 2 35 6 +29

What I checked:

Likely related people:

  • joshavant: Commit 485446a changed Android ChatController, ChatScreen, and message-identity tests for sent chat messages in history, directly adjacent to this PR's optimistic-send behavior. (role: recent optimistic-message contributor; confidence: high; commits: 485446af8c95; files: apps/android/app/src/main/java/ai/openclaw/app/chat/ChatController.kt, apps/android/app/src/main/java/ai/openclaw/app/ui/chat/ChatScreen.kt, apps/android/app/src/test/java/ai/openclaw/app/chat/ChatControllerMessageIdentityTest.kt)
  • Ayaan Zaidi: Several earlier Android chat commits changed ChatController, ChatSheetContent, assistant auto-send, and message identity behavior in the affected area. (role: Android chat history contributor; confidence: high; commits: 34a5c47351d1, a41be2585fca; files: apps/android/app/src/main/java/ai/openclaw/app/chat/ChatController.kt, apps/android/app/src/main/java/ai/openclaw/app/ui/chat/ChatSheetContent.kt)
  • nxmxbbd: Authored the merged terminal chat.send ACK PR that current main now uses for status-aware Android ChatController behavior this branch must preserve. (role: adjacent ACK fix owner; confidence: medium; commits: 3ff0c29f9d09, 9cd2b839bbf4; files: apps/android/app/src/main/java/ai/openclaw/app/chat/ChatController.kt, apps/android/app/src/main/java/ai/openclaw/app/gateway/ChatSendAck.kt)
  • Gio Della-Libera: Current blame for the central Android chat files points to commit cb4244f, so this person is relevant for rebasing against current main's file shape. (role: recent area refactor author; confidence: medium; commits: cb4244fe15e3; files: apps/android/app/src/main/java/ai/openclaw/app/chat/ChatController.kt, apps/android/app/src/main/java/ai/openclaw/app/ui/chat/ChatScreen.kt, apps/android/app/src/main/java/ai/openclaw/app/ui/chat/ChatSheetContent.kt)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 5, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 14, 2026
This was referenced Jun 19, 2026
Closed
Merged
Closed
@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

@StoneHub thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@steipete

steipete commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded by the fixes now on main and shipped in v2026.6.11.

  • 485446af8c956e9dfafd24fd02ea6761823b0fc0 preserves optimistic Android user messages until history contains the gateway copy, adds stable idempotency-key reconciliation, and covers the behavior with focused tests.
  • 3ff0c29f9d09cbcbadc7b2c6855bb73bb94364c0 / fix: handle terminal chat send acknowledgements #91049 adds structured terminal chat.send ACK handling across Android and the other chat surfaces. Live Android testing against the older v2026.6.5 Gateway confirmed that current Android now surfaces terminal failures instead of silently treating the persisted user row as success.

Rebasing this branch would regress those newer paths: it removes structured ACK parsing and several current session/history behaviors, while its remaining composer-clear change covers only ChatScreen; ChatSheetContent still clears its internal composer and attachments before acceptance. The PR body also correctly notes that this patch did not fix the missing-assistant-output report tracked in #91872.

A future composer-retention change should be a fresh, focused patch covering both Android composer implementations against current main. Thanks for the original diagnosis and Pixel validation; the optimistic-message work helped identify the UI symptom even though the root failure was elsewhere.

@steipete steipete closed this Jul 2, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 6, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

app: android App: android merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@StoneHub @steipete