Repository navigation
fix(native-chat): settle acknowledged messages after a paged reconnect - #28993
Conversation
|
A reconnect to a long chat could leave an extra local message bubble after the host had saved the message, because the newest history page no longer contained its exact row. Desktop and mobile now retain the host’s existing acknowledgement position and remove that redundant bubble once the loaded history reaches that position in the same history version. Earlier messages remain available through history paging, and sends without an acknowledgement keep their existing checks. One full adversarial review round, architecture challenge, desktop/mobile entry-point traces, and readiness/performance source review found no proven defect introduced by this PR. The design is retained; no production changes or additional fix round were needed. Temporary existing differences, deferred to separate follow-ups for replacing old local copies after a history reset and sharing desktop capability lists (not yet opened), are a saved message’s local copy surviving a later rewind and duplicated lists of features that desktop connections support. Intended difference from the common pattern: this client keeps the host’s existing saved position because a bounded history page can omit the acknowledged message; it adds no saved obligation or retry. A narrow hidden-chat queue scenario remains unexecuted residual risk, not a demonstrated regression. Verified the full A fresh desktop comparison of this PR on its own and native mobile UI QA were not run: no QA slot was granted during the capacity wait, and this task had no owned native device. Earlier M4Air and Windows attempts stopped at preflight without launching a workload; their owned worktrees were removed. Live paired/SSH transport and independently updated native peers were not exercised. The existing Windows proof covers the unchanged settlement change; the additional desktop and native-mobile checks were not run, and those coverage limits remain disclosed. No merge was performed. Update: real signed-in agent QAReal signed-in Electron QA passed on exact head
Hook hashes were unchanged: Claude settings |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description is detailed and covers the user impact, implementation, rationale, testing, visual proof, and checklist. However, the required Linked Issue section does not include an actual issue lin… | Add the actual issue reference, such as Fixes #28951, in the Linked Issue section. Keep the existing testing and visual-proof details. |
| Docstring Coverage | |
Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 18 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: settling acknowledged native-chat messages after a paged reconnect. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description is detailed and covers the user impact, implementation, rationale, testing, visual proof, and checklist. However, the required Linked Issue section does not include an actual issue link or a completed Fixes reference.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · 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.
Comment @coderabbitai help to get the list of available commands.
ELI5
A phone can keep showing an old sent message as an extra bubble after reconnecting to a long conversation. Send a message, let the agent continue while the connection is away, then reconnect after the message has moved outside the newest history page: the host confirms the send, but the phone waits forever to see its exact row. Desktop chats retain the same assumption while hidden.
Acknowledged messages now stop appearing as local echoes once the conversation read reaches the position the host confirmed. Earlier messages remain in history, and sends without an acknowledgement keep their existing delivery checks.
What Changed
Keep the journal position already supplied in a successful send reply and use the same comparison on phone and desktop. The comparison requires the same journal epoch and a read at or beyond that position. It also handles a snapshot that arrives before the send reply. Opening messages created with a chat carry their acknowledgement through the same sender.
Why
A local echo describes a send the conversation has not shown yet. Requiring its exact row couples settlement to the loaded history window; a bounded snapshot can legitimately skip that row. The host acknowledgement plus the read position prove the echo is superseded without replaying all missed history or adding an arbitrary receipt limit. This is the prerequisite for #28951.
Linked Issue
Follow-up from the native-chat performance investigation requested by @BrennanKB5.
Visual Proof
The acknowledged-send settlement case passed the Windows rendered stack run described below.
Testing
One phone flow covers an acknowledged send followed by a newest-page snapshot that excludes its row, in both reply/snapshot orders. The existing desktop sender test now checks that an unacknowledged send stays pending, an older read or another epoch cannot settle it, and a covering read retires it. Full PR CI and Mobile Checks passed at
c94bf143db3. All five unit shards, static analysis/typecheck, Linux and Windows packages, and the full PR aggregate passed; Windows passed on retry after PowerShell timeouts in unchanged hook tests. Mobile passed 485 and 10,193 tests in its two runtime groups (six skips), including all 16 tests in the changed send-flow file and both reconnect orderings. No tests, builds, lint, installs, or typechecks ran on the overloaded development Mac. A fresh openclaw connection failed withPermission denied (publickey).Review
Cross-platform and remote hosts use the journal position from the execution host; there is no new wire field or request. Older hosts already return this position. No sender retries or durable state are added.
Agent skill upstream boundary
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Rendered QA
Windows high spec, stack commit
50b4118ccd4c8d2fab2a68e14e4d5729c41df915(includes #28993 atc94bf143db3): private install and e2e build, one hidden Electron app, isolated profile, folder workspace, Claude stream-json stand-in, scripted Playwright over CDP. All captures assertisVisible() === false; no desktop input was used.A send was acknowledged at position 788 while the hidden reader remained at 787. Reopening after 786 missed saved changes produced one newest-page snapshot of 200 items at position 1573; the sent row was outside that snapshot, yet the pending echo cleared. The complete final reply rendered, and loading older history showed the delivered user message. Existing viewport prefetch then loaded 200 additional items; a subsequent real mouse-wheel scroll loaded the remaining 124 (400 → 524), with position 1573 unchanged. A short gap replayed 26 saved changes (526 → 534 items) without replacing the page. Clean quit/relaunch on a fresh CDP port restored the saved tab and all 531 message records with the same SHA-256 text hash.
The original
7fcc8645fadrun exposed a missing capability on the local desktop IPC reader: the same normal-reply fixture replayed 786 changes and retained 524 items; the corrected50b4118ccd4fixture reopened with a 200-item snapshot. The desktop capability advertisement and one subscription-boundary regression were added to #28951. This is a deterministic rendered count comparison, not a new timing benchmark. Live paired/SSH transport and phone UI were not exercised in this Windows run.Final reply after snapshot:
Delivered message after older history loads (same corrected commit, normal-reply fixture):
Older history after actual CDP wheel scrolling:
Transcript after clean restart: