Repository navigation
fix(native-chat): preserve literal image markers - #14581
brennanb2025 wants to merge 17 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change preserves literal 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
The render path became provenance-aware for `[Image #n]` markers, but the pending-key path kept stripping them unconditionally, so the two disagreed: a standalone marker-only turn rendered its literal text while its content key computed to empty, leaving the optimistic echo unmatchable and the row duplicated on desktop and mobile. Give both sides the same rule via `nativeChatUserMessageMatchText` — a marker run that stripping would erase, on a turn carrying no image, is the user's own text and keys verbatim. Mobile's preview reconciliation also accepted a single marker as proof for any number of pending images, so an incomplete multi-image echo could clear or rebind every preview; it now requires one marker per image.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff0df98d-20cb-4c8a-a4c3-4b15613b651c
📒 Files selected for processing (9)
mobile/src/session/mobile-native-chat-draft-reconcile.tsmobile/src/session/mobile-native-chat-image-transcript-markers.tsmobile/src/session/mobile-native-chat-literal-marker-echo.test.tsmobile/src/session/mobile-native-chat-pending-echo.tsmobile/src/session/use-mobile-native-chat-drafts.tsmobile/src/session/use-mobile-native-chat-message-send.tssrc/renderer/src/components/native-chat/native-chat-literal-marker-echo.test.tssrc/renderer/src/components/native-chat/native-chat-pending-occurrence.tssrc/shared/native-chat-image-transcript-markers.ts
Live send-and-reconcile validation — head
|
…-resume # Conflicts: # mobile/src/session/use-mobile-native-chat-drafts.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mobile/src/session/mobile-native-chat-literal-marker-echo.test.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse a supported React 19 test renderer for this new test.
react-test-rendereris deprecated in React 19. Migrate this new harness to the repository-supported React testing package before this API becomes unsupported. Verify the mobile test renderer that the repository standardizes on.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5238cd44-1308-4e21-9127-3aa152341db6
📒 Files selected for processing (20)
mobile/src/session/mobile-native-chat-draft-reconcile.test.tsmobile/src/session/mobile-native-chat-draft-reconcile.tsmobile/src/session/mobile-native-chat-image-transcript-markers.tsmobile/src/session/mobile-native-chat-literal-marker-echo.test.tsmobile/src/session/mobile-native-chat-pending-echo.tsmobile/src/session/mobile-native-chat-pending-retirement.test.tsmobile/src/session/mobile-native-chat-pending-retirement.tsmobile/src/session/mobile-native-chat-render-data.test.tsmobile/src/session/use-mobile-native-chat-drafts-launch-draft.test.tsmobile/src/session/use-mobile-native-chat-drafts.test.tsmobile/src/session/use-mobile-native-chat-drafts.tsmobile/src/session/use-mobile-native-chat-launch-draft-seed.tsmobile/src/session/use-mobile-native-chat-message-send.test.tsmobile/src/session/use-mobile-native-chat-message-send.tssrc/renderer/src/components/native-chat/native-chat-literal-marker-echo.test.tssrc/renderer/src/components/native-chat/native-chat-pending-occurrence.tssrc/renderer/src/components/native-chat/native-chat-pending.test.tssrc/renderer/src/components/native-chat/native-chat-session-assembler.test.tssrc/shared/native-chat-image-transcript-markers.test.tssrc/shared/native-chat-image-transcript-markers.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/renderer/src/components/native-chat/native-chat-session-assembler.test.ts
- mobile/src/session/use-mobile-native-chat-launch-draft-seed.ts
- mobile/src/session/use-mobile-native-chat-message-send.ts
- mobile/src/session/mobile-native-chat-render-data.test.ts
- mobile/src/session/mobile-native-chat-pending-echo.ts
- src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts
- mobile/src/session/use-mobile-native-chat-drafts.ts
- mobile/src/session/mobile-native-chat-image-transcript-markers.ts
- mobile/src/session/use-mobile-native-chat-drafts-launch-draft.test.ts
- src/renderer/src/components/native-chat/native-chat-literal-marker-echo.test.ts
- src/shared/native-chat-image-transcript-markers.ts
- mobile/src/session/mobile-native-chat-draft-reconcile.ts
| retireLandedMobileNativeChatPending(attachedImage, plainPending, NO_IMAGE_ECHOES).map( | ||
| (item) => item.id | ||
| ) | ||
| ).toEqual(['plain']) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the attached-image marker case.
The final assertion uses plainPending again. It does not verify that an attached-image transcript turn keeps markerPending. Add an assertion with attachedImage and markerPending.
Proposed test
expect(
retireLandedMobileNativeChatPending(attachedImage, plainPending, NO_IMAGE_ECHOES).map(
(item) => item.id
)
).toEqual(['plain'])
+ expect(
+ retireLandedMobileNativeChatPending(attachedImage, markerPending, NO_IMAGE_ECHOES).map(
+ (item) => item.id
+ )
+ ).toEqual(['marker'])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| retireLandedMobileNativeChatPending(attachedImage, plainPending, NO_IMAGE_ECHOES).map( | |
| (item) => item.id | |
| ) | |
| ).toEqual(['plain']) | |
| retireLandedMobileNativeChatPending(attachedImage, plainPending, NO_IMAGE_ECHOES).map( | |
| (item) => item.id | |
| ) | |
| ).toEqual(['plain']) | |
| expect( | |
| retireLandedMobileNativeChatPending(attachedImage, markerPending, NO_IMAGE_ECHOES).map( | |
| (item) => item.id | |
| ) | |
| ).toEqual(['marker']) |
|
QA evidence at head 1. Literal 2. Attached image whose marker refers to it — NOT satisfied in the live flow 3. Three attached images — partial-echo scenario not live-observable
Mobile (Orca Mobile dev build, iOS Simulator) — blocked |
|
OpenCode revalidation — exact head 1. Literal
2. Pasted-image send (real transcript is prompt-before-source) — blocked in live UI. The assistant genuinely received and answered the image, the optimistic echo retired without a duplicate (single row), but the landed row renders the raw marker 3. Three-image send — full-evidence retirement PASS; partial echo unit-test-only. Sent three pasted images; the echo retired only when the full 3-marker turn landed (one row,
Mobile (iOS) — blocked. Housekeeping. |
|
OpenCode final evidence QA — exact head 1. Literal
|
Exact-final-head OpenCode evidence QA —
|
Fresh exact-head QA — commit
|
Fresh final exact-head QA —
|
|
Superseded by #14939, which fixes STA-4363 with a focused, anchored change (+950/-101 vs +2273/-315 here). Why this one is being closed rather than iterated:
The sound work here was kept: #14939 cherry-picks the anchoring fix and the echo-key coherence that must accompany it, with |
1 participant
ELI5
Native chat no longer mistakes literal text such as
[Image #1]for hidden image metadata unless the transcript proves it belongs to a real image upload — and the optimistic "queued" bubble now agrees with what gets rendered, so a marker-only message shows up once instead of twice.What Changed
Original fix (
fab320e42)Follow-up P1 fixes (
7803de9c1b)The first commit made the render path provenance-aware but left the pending-key path stripping markers unconditionally, so the two disagreed. Two P1 regressions followed:
[Image #1]whilenativeChatPendingContentKey/nativeChatUserMessageContentKeynormalized it to empty, producing anullkey. The optimistic echo could never match the landed turn, so the row duplicated on desktop and mobile. Both sides now share one rule (nativeChatUserMessageMatchText): a marker run that stripping would erase, on a turn carrying no image, is the user's own text and keys verbatim. The send side mirrors it —captureSendOriginkeeps literal markers unless the send actually carried images.findLandedImagePreviewEchoesacceptedhasImagePromptMarkeras proof for any number of pending images, so an incomplete multi-image echo could clear the optimistic bubble and rebind every local preview onto a turn that only accounted for one. It now requirescountImagePromptMarkers(message) >= entry.images.length.Not changed: the STA-4363 render behaviour itself. The pending path was moved onto the render path's rule, not the reverse.
Why
The prior unanchored normalization removed
[Image #N]from ordinary user turns even when no image-source record preceded them. Transcript structure is the authoritative proof for stripping; local preview state must not change the displayed user text.Once rendering became provenance-aware, key computation had to move with it — a render path and a reconciliation path that disagree about what counts as a marker cannot reconcile at all.
Pre-fix RED (original):
Pre-fix RED (P1 #1 — duplicate echo). New outcome tests run against
fab320e42; both assert what the transcript actually shows, not what a key function returns:Pre-fix RED (P1 #2 — one marker rebinding every preview). Two images sent, one marker echoed: the optimistic bubble was cleared and both previews rebound onto the incomplete turn.
Linked Issue
STA-4363 — native chat strips literal
[Image #N]from user messages.Visual Proof
Before: the RED assertions above capture the user-visible text loss and the duplicated row. After: Electron CDP validation renders the literal marker intact:
Mobile iOS QA also passed against the current Metro development bundle: standalone
[Image #1]text remained visible while genuine image-source runs continued to fold correctly.Testing
Automated (full suites, follow-up commit included):
pnpm vitest run --config config/vitest.config.ts— 4870 files / 52304 tests passed. The only failures are 13 pre-existing local environment reds insrc/main/ipc/pty.test.tsandsrc/relay/pty-handler.test.ts(WSLENV / ZDOTDIR shell-env), confirmed identical onfab320e42with these changes stashed.pnpm --dir mobile exec vitest run --config vitest.config.ts— 445 files / 3499 tests passed, 0 failures.pnpm typecheck— passed.pnpm --dir mobile exec tsc --noEmit— passed.oxlint— passed (exit 0).audit:code-quality:native,audit:code-quality:type-aware,check:max-lines-ratchet— passed; nomax-linessuppression added or bumped.oxfmt --writeon all touched files; pre-commitoxlint+ react-doctor +oxfmthooks passed.Manual:
orca emulator(original commit).fab320e42first.AI Disclosure
Review
Reviewed for security, performance, cross-platform behavior, SSH/remote use, folder workspaces, mobile parity, and backward compatibility. The change is display-only and leaves stored transcripts and remote wire data unchanged.
Checklist
Author